fix: Prevent SSRF in import and app package URL fetches#40338
fix: Prevent SSRF in import and app package URL fetches#40338aditya-c7 wants to merge 2 commits intoRocketChat:developfrom
Conversation
|
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 |
|
WalkthroughUpdates the import file download mechanism to enforce SSRF (Server-Side Request Forgery) validation using an allowlist, and removes SSRF validation bypasses from app-related REST endpoints. The importer now strictly handles HTTP(S) URLs with server fetch and buffers responses, removing local filesystem path support. Changes
Sequence DiagramsequenceDiagram
participant Client
participant ImportMethod as Download Method
participant Validator as URL Validator
participant ServerFetch as Server Fetch
participant Allowlist as SSRF Allowlist
participant Stream as Import Stream
Client->>ImportMethod: Request file download
ImportMethod->>Validator: Parse/validate fileUrl
Validator->>ImportMethod: Validated HTTP(S) URL
ImportMethod->>Allowlist: Check allowed domain
Allowlist->>ServerFetch: Domain allowed
ServerFetch->>ServerFetch: Fetch remote content
ServerFetch->>ImportMethod: Response (arrayBuffer)
ImportMethod->>ImportMethod: Buffer to memory
ImportMethod->>Stream: Write buffered content
Stream->>Stream: Emit finish event
Stream->>Client: Import complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
aditya-c7
changed the title
aditya-c7
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d7f35384b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| throw new Meteor.Error('error-import-file-download-failed', 'Failed to download import file.', 'downloadPublicImportFile'); | ||
| } | ||
|
|
||
| const fileBuffer = Buffer.from(await response.arrayBuffer()); |
There was a problem hiding this comment.
This now reads the full remote import file into memory (response.arrayBuffer() then Buffer.from(...)) before writing it to storage, which is a regression from the previous streaming behavior and can exhaust Node heap on large imports. In production, a sufficiently large CSV/JSON export can cause high memory spikes or process crashes during downloadPublicImportFile, interrupting imports for all users. Keep the SSRF validation, but write from the response stream to writeStream to preserve bounded memory usage.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Hardens two server-side URL fetch flows to reduce SSRF and local file read risk in self-hosted deployments by enforcing @rocket.chat/server-fetch SSRF protections and applying the configured SSRF_Allowlist.
Changes:
- Enforce SSRF validation (and apply
SSRF_Allowlist) when installing/updating private apps from a caller-supplied URL. - Restrict
downloadPublicImportFileto HTTP/HTTPS URLs only (no local filesystem paths) and route downloads through@rocket.chat/server-fetchwith SSRF validation enabled.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| apps/meteor/ee/server/apps/communication/rest.ts | Enforces SSRF validation + allowlist for user-supplied app package URLs in Apps REST endpoints. |
| apps/meteor/app/importer/server/methods/downloadPublicImportFile.ts | Removes local-path import downloads; validates URL scheme and fetches via server-fetch with SSRF allowlist. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const fileBuffer = Buffer.from(await response.arrayBuffer()); | ||
| await new Promise<void>((resolve, reject) => { | ||
| writeStream.once('error', reject); | ||
| writeStream.end(fileBuffer, resolve); | ||
| }); |
There was a problem hiding this comment.
downloadHttpFile loads the full response into memory (response.arrayBuffer() -> Buffer.from(...)) before writing. Import archives can be large, so this risks high memory usage/OOM and makes the server susceptible to large-download DoS. Prefer streaming the response body into the write stream (with backpressure) and enforce a max size (e.g., Content-Length / configured limit) and/or an explicit download timeout.
|
|
Summary
This PR hardens two server-side URL fetch paths that can be security-sensitive in self-hosted deployments:
@rocket.chat/server-fetchwith SSRF validation enabled and the configuredSSRF_Allowlistapplied.downloadPublicImportFile.Security impact
The previous importer flow accepted both remote URLs and local filesystem paths from a user with
run-import, then used rawhttp/httpsorfs.createReadStream. That could allow a privileged importer account to make the Rocket.Chat server reach internal network resources or read local files into the import pipeline.The apps management flow also explicitly bypassed SSRF validation for caller-provided app package URLs. Even though this path is gated by
manage-apps, keeping Rocket.Chat's SSRF protections active for user-controlled destinations reduces blast radius if a privileged session or delegated role is compromised. Intentional exceptions can still use the configuredSSRF_Allowlist.Testing
RocketChat/Rocket.Chatdevelop.git diff --checksuccessfully.Areas touched