-
Notifications
You must be signed in to change notification settings - Fork 13k
regression: Twilio failing signature check #36077
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
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
req.url as relativereq.url as relative & re-parsing JSON body
req.url as relative & re-parsing JSON body
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## release-7.7.0 #36077 +/- ##
=================================================
+ Coverage 64.22% 64.75% +0.53%
=================================================
Files 3018 3110 +92
Lines 91836 93198 +1362
Branches 17435 17749 +314
=================================================
+ Hits 58978 60347 +1369
+ Misses 30172 30062 -110
- Partials 2686 2789 +103
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
/patch |
|
Pull request #36103 added to Project: "Patch 7.6.2" |
Proposed changes (including videos or screenshots)
Issue(s)
https://rocketchat.atlassian.net/browse/CTZ-178
Steps to test or reproduce
Further comments
(Note: 2 issues in one as one fix is not enough for the feature to work)
For some reason, Hono defaults to
localhostin req.url (or it's using the localhost domain instead of the public site url)For example, on candidate:
Since twilio assumed this property
req.urlwas relative, it created its own URL based on it. Now, it's absolute, including hostname which caused the signature to mismatch from what twilio was sending.The other issue was that the body was being re-parsed as
json, which didn't work, causing an empty body which made the validation fail (as the signature from twilio includes the body)Since on CI (dum, ik) we skip this twilio validation, the bug wasn't caught. A task will be created to change the CI tests to be more real-world and avoid the diff behavior when TEST_MODE is true.