Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Verify-webhook-signature #60

Merged

Conversation

Icegreeen
Copy link
Contributor

@Icegreeen Icegreeen commented Jun 25, 2024

Pull Request Overview

This PR features two main improvements focused on webhook processing.

1. Payload Validation

WebhookController.php

  • Functionality Added: Early validation in the handleWebhook method to check if the received payload is well-formed and contains all necessary fields.
  • Impact: Returns an HTTP 400 status for malformed or invalid payloads, improving robustness against incorrect data processing.
  • Test Implementation: A new helper function invalidWebhookRequest() was created. This function generates a Request object with a JSON payload that deliberately fails to meet expected criteria, aiding in robust error handling testing.

Webhook Payload Validation Test Screenshot


2. Header Transformation Functionality

VerifyWebhookSignature.php

  • Enhancement: Introduced a function getTransformedHeaders to simplify the transformation of request headers into an associative array.
  • Benefit: Streamlines header handling, enhancing clarity and maintainability.

Test Implementation

  • Test Setup: Developed to confirm that the VerifyWebhookSignature middleware accurately rejects requests with invalid signatures.
    • Request Creation: Simulated a POST request to the /webhook endpoint with test data and an intentionally invalid signature header.
    • Expected Outcome: Proper rejection of unauthorized access through an AccessDeniedHttpException, confirming middleware effectiveness.

Middleware Test Screenshot

@jayanratna

Verified

This commit was signed with the committer’s verified signature.
renovate-bot Mend Renovate
@jayanratna
Copy link
Collaborator

Hey @Icegreeen, thank you for creating this ❤️

Apologies I totally missed this. I'll review it sometime this week and get back to you soon.

@jayanratna jayanratna self-requested a review July 4, 2024 04:16
@jayanratna
Copy link
Collaborator

Looks good overall. Quick question... Any reason why you updated the way headers are transformed? As both approaches do the same thing.

@Icegreeen
Copy link
Contributor Author

Looks good overall. Quick question... Any reason why you updated the way headers are transformed? As both approaches do the same thing.

By extracting the transformation logic into a separate method (getTransformedHeaders), it is easier to understand and modify the code if needed in the future.

This approach also adheres to the Single Responsibility Principle (S.O.L.I.D.) by separating the header transformation logic from the main request handling logic.

Although this adds more lines of code, it is more organized. But feel free to modify or deprecate it if necessary.

Co-authored-by: Jayan Ratna <jayanratna@gmail.com>
Copy link
Collaborator

@jayanratna jayanratna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, really appreciate your contribution 🖤

@jayanratna jayanratna merged commit 43f2740 into resend:main Jul 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants