fix: Enable Twilio MMS inbound by following redirects (#36491) #37376
Conversation
Changes fetch redirect mode from 'error' to 'follow' to handle Twilio's 302 redirects when downloading MMS media attachments. Also appends media URLs to message body for better visibility. - Modified getUploadFile() to follow redirects and added error logging - Enhanced Twilio parse() to append media URLs to message text - Maintains SSRF protection and backward compatibility Resolves: RocketChat#36491
|
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 |
|
WalkthroughThese changes address Twilio MMS inbound media retrieval failures by enabling HTTP redirects during media downloads and appending media URLs to message bodies. A warning log is added to capture fetch failures before throwing errors. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/meteor/server/services/omnichannel-integrations/providers/twilio.ts (1)
102-110: LGTM! Media URLs appended for visibility.The logic correctly collects and appends all non-empty media URLs to the message body, improving visibility of MMS content. The implementation properly handles edge cases (empty body, empty URLs, no media).
Note: The downstream handler in
sms.ts(line 153) currently processes only the first media item for attachment upload, while this change displays all media URLs in the body. This means for multi-media messages, additional URLs will be visible to users but not automatically uploaded. Consider enhancing multi-media support in a future iteration.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/meteor/app/livechat/imports/server/rest/sms.ts(1 hunks)apps/meteor/server/services/omnichannel-integrations/providers/twilio.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx} : Avoid code comments in the implementation
Applied to files:
apps/meteor/app/livechat/imports/server/rest/sms.ts
🔇 Additional comments (2)
apps/meteor/app/livechat/imports/server/rest/sms.ts (2)
36-37: LGTM! Redirect fix correctly addresses the Twilio MMS issue.The change from
redirect: 'error'toredirect: 'follow'properly enables retrieval of Twilio media URLs that return HTTP 302 redirects. Security is preserved since SSRF protection viacheckUrlForSsrf()is enforced at lines 31-34 before the fetch executes, and the built-in fetch API prevents infinite redirect loops.
44-44: LGTM! Improved diagnostics for fetch failures.The warning log captures essential debugging information (URL, HTTP status, content size) before throwing the error, making it easier to diagnose media retrieval issues.
|
Appreciate you doing the PR on this, I never got around to doing one. |
|
Hello maintainers, I opened PR #37376{ which fixes Twilio MMS inbound redirects (a long-standing issue affecting multiple community users). The PR is ready, tested, and has been validated by issue reporters, but no code owner has reviewed it yet. Could someone point me to the right maintainer or help get a review? Thank you! |
|
Any update on this? We'd love to see this implemented. |
|
I know the branch is behind and needs updated, but once that is done again, what would it take to get this into the main branch? |
There was a problem hiding this comment.
Pull request overview
This PR fixes Twilio MMS inbound functionality by enabling HTTP redirect following when fetching media attachments. When customers send MMS messages through Twilio, media URLs respond with HTTP 302 redirects to the actual media location. The previous implementation rejected all redirects, causing MMS attachments to fail.
Changes:
- Modified fetch behavior to follow redirects for Twilio media URLs
- Added media URLs to message body for improved visibility
- Enhanced error logging with URL, status code, and content size
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| apps/meteor/app/livechat/imports/server/rest/sms.ts | Changed fetch redirect mode from 'error' to 'follow' and improved error logging |
| apps/meteor/server/services/omnichannel-integrations/providers/twilio.ts | Added logic to append media URLs to message body for better visibility |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I’ve updated the branch with the latest |
|
Like to see this get put in soon, so we can move back to normal deployments of Rocket.Chat. Currently running self-built versions breaks Push Notifications, which is ultra annoying. |
| const contentSize = content.length; | ||
|
|
||
| if (response.status !== 200 || contentSize === 0) { | ||
| logger.warn(`Failed to fetch file from ${fileUrl}: status ${response.status}, size ${contentSize}`); |
There was a problem hiding this comment.
If you really want to have this log, change it to be an object instead of templates
logger.error({ msg: 'Failed to fetch file', fileUrl, status: response.status, contentSize });
| // Append media URLs to the message body for better visibility | ||
| if (returnData.media.length > 0) { | ||
| const links = returnData.media | ||
| .map((media) => media.url) | ||
| .filter((url) => !!url) // skip empty URLs | ||
| .join('\n'); | ||
|
|
||
| returnData.body = [returnData.body, links].filter(Boolean).join('\n'); | ||
| } | ||
|
|
There was a problem hiding this comment.
No need for this.
| // Append media URLs to the message body for better visibility | |
| if (returnData.media.length > 0) { | |
| const links = returnData.media | |
| .map((media) => media.url) | |
| .filter((url) => !!url) // skip empty URLs | |
| .join('\n'); | |
| returnData.body = [returnData.body, links].filter(Boolean).join('\n'); | |
| } |
|
I'll put this PR on hold for now, there's internal work to address this issue with a more secure PoV. |
|
Thanks for the feedback @KevLehman Given that this PR is now on hold in favor of #38044, |
4 participants
Proposed changes
Fixes Twilio MMS inbound functionality by handling HTTP 302 redirects when fetching media attachments.
Problem
When customers send MMS messages through Twilio, media URLs respond with HTTP 302 redirects to the actual media location. The previous implementation used
redirect: 'error'which caused all MMS attachments to fail with:Users would see "An Attachment was received, but upload to server failed" in the UI, and the following error in logs:
{ "level": 50, "msg": "Attachment upload failed", "err": { "type": "FetchError", "message": "uri requested responds with a redirect, redirect mode is set to error: https://api.twilio.com/..." } }Solution
fetch(fileUrl, { redirect: 'error' })tofetch(fileUrl, { redirect: 'follow' })Technical Details
Files Modified:
apps/meteor/app/livechat/imports/server/rest/sms.ts'error'to'follow'ingetUploadFile()functionapps/meteor/server/services/omnichannel-integrations/providers/twilio.tsparse()method to append media URLs to message bodyCode Changes:
Security Considerations:
checkUrlForSsrf()check before redirect followingBackward Compatibility:
Lines changed: +13, -1 (net: +12 lines)
Issue(s)
Fixes #36491
Steps to test or reproduce
Prerequisites
Configuration Steps
truetwilio[Your Account SID][Your Auth Token]truehttps://your-rocket-chat-server.com/api/v1/livechat/sms-incoming/twilioPOSTTest Cases
Test 1: MMS with Image (Primary Fix Validation)
https://api.twilio.com/.../Media/...)Test 2: Regular SMS (Regression Test)
Test 3: MMS with Video/Audio
Expected Results
Logs to Monitor
Further comments
Community Validation
This fix has been validated by the original issue reporter (@sorscode) in a production Docker environment:
The issue was also confirmed by @alex-powell-1, indicating this affects multiple users.
Why This Solution?
Minimal, Focused Change:
Safe Implementation:
Standard Practice:
redirect: 'follow'is the default and recommended mode for fetchAlternatives Considered
redirect: 'follow'is more robust and handles edge casesAdditional Context
Why was
redirect: 'error'used originally?Impact:
Related:
Checklist
Status: Ready for review and QA verification
Summary by CodeRabbit