-
Notifications
You must be signed in to change notification settings - Fork 13.1k
fix: incoming webhooks not unwrapping JSON from x-www-form-urlencoded payload #38319
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
🦋 Changeset detectedLatest commit: 15eee70 The changes in this PR will be included in the next version bump. This PR includes changesets to release 40 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughIncoming webhook handling now attempts to parse JSON contained in a form-encoded Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Client (Webhook sender)
participant Integrations as Integrations API\n(api.ts)
participant Context as Request Context (c)
participant Router as RocketChatAPIRouter\n(router.ts)
participant Parser as parseBodyParams
Client->>Integrations: POST /hooks/:id/:token (form-encoded or JSON)
note right of Integrations: If form-encoded and body.payload is a string\ntry JSON.parse(body.payload)
Integrations->>Context: set('bodyParams-override', parsedPayloadOrOriginal)
Client->>Router: Request reaches router
Router->>Context: c.get('bodyParams-override')
alt override present
Context-->>Router: return override (use as bodyParams)
else override absent
Router->>Parser: parseBodyParams({ request: req })
Parser-->>Router: parsed bodyParams
end
Router->>App: forward request with resolved bodyParams
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #38319 +/- ##
===========================================
+ Coverage 70.84% 70.87% +0.02%
===========================================
Files 3160 3160
Lines 109768 109765 -3
Branches 19702 19727 +25
===========================================
+ Hits 77770 77795 +25
+ Misses 29968 29945 -23
+ Partials 2030 2025 -5
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
b03a4f3 to
92dab1b
Compare
1d45c5c to
6db259d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No issues found across 4 files
edaf8bc to
e50c45c
Compare
e50c45c to
15eee70
Compare
While reviewing incoming webhook integrations, we identified that
application/x-www-form-urlencodedpayloads with a payload field (Slack/GitHub-style webhooks) were not being properly parsed and passed to integrations.We initially thought this scenario was covered since there was an existing test for it - and it was passing. However, upon investigation, we discovered the test itself had a bug: async assertions inside Supertest
.expect()callback were never being awaited, causing the test to pass regardless of whether the assertions succeeded or failed.Proposed changes (including videos or screenshots)
x-www-form-urlencodedrequests.Issue(s)
Steps to test or reproduce
Further comments
When writing API tests with Supertest, be careful with async assertions:
Don't: Return promises inside
.expect()callbacksWhy it fails silently: Supertest
.expect()callback does not await returned promises. The inner request and its assertions may never complete or may fail, but the test passes anyway because the outer chain completed.Do: Separate requests and await them explicitly
Why this works: each request is explicitly awaited, and assertions run synchronously on the resolved response. Any failure will properly fail the test.
Summary by CodeRabbit
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.