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

Request middleware fails on validate set to false #460

Conversation

khalilchoudhry
Copy link
Contributor

X-Twilio-Signature check is added before checking the validate option. The request will fail even if validate option is set to false. If a request is sent from POSTMAN on dev/test environment, which means the validate option is false, so there is no need to check if the header is present or not. Similarly, if a test is written for Express route in my code, the middleware will fail despite the fact that validate option is false.

It also checks for X-Twilio-Signature here which will not fail with validate option set to false. If validate option is true and X-Twilio-Signature is undefined, empty string is set as a default value which will then obviously differ from expected twilio signature and fail.
Contributing to Twilio

All third-party contributors acknowledge that any contributions they provide will be made under the same open-source license that the open-source project is provided under.

  • I acknowledge that all my contributions will be made under the project's license.

@khalilchoudhry khalilchoudhry changed the title redundant x-twillio-signature check removed Request middleware fails on validate set to false Jun 21, 2019
@khalilchoudhry
Copy link
Contributor Author

@childish-sambino Please review this.

@childish-sambino
Copy link
Contributor

Instead of removing the check from webhook(), how about just moving it down to somewhere within the validation block? This would then support the functionality you're looking for and provide a more specific error message and proper response code. I prefer to be more specific in the message as long as it's not a security risk.

@khalilchoudhry
Copy link
Contributor Author

Isn't X-Twilio-Signature is expected from Twilio only? How can someone create or set X-Twilio-Signature on dev environment or during testing? Why is it asked here in docs to disable validation during testing then ?

@childish-sambino
Copy link
Contributor

If you move the header check to within the if (opts.validate) { block, then it will not be checked during dev/test.

@khalilchoudhry
Copy link
Contributor Author

done

Copy link
Contributor

@childish-sambino childish-sambino left a comment

Choose a reason for hiding this comment

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

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug bug in the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants