refactor: rewrite checkUrlForSsrf and include it in the server-fetch package
#38044
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 |
|
WalkthroughCentralizes SSRF validation into a new packages/server-fetch module with URL/IP validation and pinning in serverFetch, adds ignoreSsrfValidation option, removes legacy checkUrlForSsrf and its tests, and updates multiple call sites to either opt out or rely on the centralized SSRF checks. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ServerFetch
participant Validator
participant DNS as "DNS Resolver"
participant Agent as "Agent Selector"
participant Remote as "Remote Server"
Client->>ServerFetch: serverFetch(url, options)
alt options.ignoreSsrfValidation == true
ServerFetch->>Remote: direct fetch (agent if any)
Remote-->>ServerFetch: response
ServerFetch-->>Client: response
else
ServerFetch->>Validator: checkForSsrfWithIp(url)
Validator->>DNS: nslookup(hostname)
DNS-->>Validator: resolved IP / error
alt resolved IP blocked
Validator-->>ServerFetch: denied
ServerFetch-->>Client: error (SSRF blocked)
else
Validator-->>ServerFetch: allowed + resolved IP
ServerFetch->>Agent: getFetchAgentWithValidation(hostname, resolved IP)
Agent-->>ServerFetch: agent + pin info
ServerFetch->>Remote: fetch pinned IP with Host header
Remote-->>ServerFetch: response (may be 3xx)
loop while 3xx and redirects < max
ServerFetch->>Validator: checkForSsrfWithIp(Location)
Validator-->>ServerFetch: allowed/denied
alt allowed
ServerFetch->>Remote: fetch next URL (apply pinning if any)
Remote-->>ServerFetch: response
else
ServerFetch-->>Client: error (redirect SSRF blocked)
end
end
ServerFetch-->>Client: final response
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 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. ✨ 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #38044 +/- ##
=========================================
Coverage 70.64% 70.65%
=========================================
Files 3133 3135 +2
Lines 108373 108546 +173
Branches 19492 19582 +90
=========================================
+ Hits 76564 76690 +126
- Misses 29813 29851 +38
- Partials 1996 2005 +9
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
📦 Docker Image Size Report➡️ Changes
📊 Historical Trend---
config:
theme: "dark"
xyChart:
width: 900
height: 400
---
xychart
title "Image Size Evolution by Service (Last 30 Days + This PR)"
x-axis ["11/18 22:53", "11/19 23:02", "11/21 16:49", "11/24 17:34", "11/27 22:32", "11/28 19:05", "12/01 23:01", "12/02 21:57", "12/03 21:00", "12/04 18:17", "12/05 21:56", "12/08 20:15", "12/09 22:17", "12/10 23:26", "12/11 21:56", "12/12 22:45", "12/13 01:34", "12/15 22:31", "12/16 22:18", "12/17 21:04", "12/18 23:12", "12/19 23:27", "12/20 21:03", "12/22 18:54", "12/23 16:16", "12/24 19:38", "12/25 17:51", "12/26 13:18", "12/29 19:01", "12/30 20:52", "01/19 17:16 (PR)"]
y-axis "Size (GB)" 0 --> 0.5
line "account-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "authorization-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "ddp-streamer-service" [0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.00]
line "omnichannel-transcript-service" [0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.00]
line "presence-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "queue-worker-service" [0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.00]
line "rocketchat" [0.35, 0.35, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.00]
Statistics (last 30 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Mon, 19 Jan 2026 17:16:40 GMT |
julio-rocketchat
changed the title
checkUrlForSsrf and include it in the server-fetch packagecheckUrlForSsrf and include it in the server-fetch package
…validation Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/meteor/ee/server/apps/communication/rest.ts (2)
260-274: Critical SSRF vulnerability: disabling validation on user-provided URL.
this.bodyParams.urlis user input. Disabling SSRF validation allows an attacker to make the server fetch internal resources, cloud metadata endpoints, or other restricted IPs.Remove
ignoreSsrfValidation: truefor user-provided URLs:🔒 Proposed fix
if (this.bodyParams.url) { try { - const response = await fetch(this.bodyParams.url, { ignoreSsrfValidation: true }); + const response = await fetch(this.bodyParams.url);
801-810: Critical SSRF vulnerability: same issue as the POST handler above.This update endpoint also fetches from
this.bodyParams.urlwith SSRF validation disabled.🔒 Proposed fix
if (this.bodyParams.url) { - const response = await fetch(this.bodyParams.url, { ignoreSsrfValidation: true }); + const response = await fetch(this.bodyParams.url);
🤖 Fix all issues with AI agents
In `@packages/server-fetch/src/checkForSsrf.ts`:
- Around line 43-50: The DNS resolution uses dns.lookup without `{ all: true }`
so it only returns a single address and can be bypassed by dual-stack (A+AAAA)
records; update `nslookup` to call `lookup(hostname, { all: true }, ...)` and
return all resolved addresses (Promise<string[]>), then modify
`checkForSsrfWithIp` to iterate over every resolved address and fail the check
if any address falls into a blocked/ private range (i.e., reject the domain when
any address is disallowed) instead of validating only the first result.
🧹 Nitpick comments (6)
apps/meteor/ee/app/livechat-enterprise/server/api/lib/triggers.ts (1)
17-23: Remove redundant type assertion.The
as ExtendedFetchOptionscast is unnecessary. TheserverFetchfunction signature declaresoptions?: ExtendedFetchOptions, and the object literal's properties (timeout,body,headers,method,ignoreSsrfValidation) are all valid members ofExtendedFetchOptions. TypeScript will infer the correct type without explicit casting.🔧 Suggested fix
const response = await fetch(url, { timeout: timeout || 1000, body, headers, method: 'POST', ignoreSsrfValidation: true, - } as ExtendedFetchOptions); + });packages/server-fetch/src/checkForSsrf.ts (1)
71-72: Consider additional cloud metadata domain blocking.Currently only
metadata.google.internalis blocked by domain name. Consider blocking other cloud provider metadata hostnames for defense in depth:
instance-data(AWS)metadata.azure.com(Azure)While the IP ranges should catch these, explicit domain blocking adds another layer.
packages/server-fetch/src/index.ts (1)
186-201: Clarify redirect counting for maintainability.The loop structure is correct but subtle:
redirectCount <= MAX_REDIRECTSwith the check at line 196 allows exactly 5 redirects. Consider adding a brief comment or restructuring slightly for clarity.Optional: clarify with comment
+ // Loop handles initial request (redirectCount=0) plus up to MAX_REDIRECTS redirects for (let redirectCount = 0; redirectCount <= MAX_REDIRECTS; redirectCount += 1) {packages/server-fetch/tests/checkForSsrf.test.ts (3)
22-28: Consider adding tests for additional SSRF vectors.The PR objectives mention protection against alternative IP representations and cloud metadata endpoints. Consider adding tests for:
- Private network ranges:
10.0.0.1,172.16.0.1,192.168.1.1- AWS metadata endpoint:
169.254.169.254- IPv4-mapped IPv6 bypass:
::ffff:127.0.0.1- Decimal/octal IP representations:
2130706433(decimal for 127.0.0.1),0177.0.0.1(octal)- URLs with ports:
http://127.0.0.1:8080These are common SSRF bypass techniques that align with the linked vulnerability issues.
50-58: Optional: Refactor loop to useit.eachfor better test reporting.Using
it.eachinstead of a loop would provide individual test case names in output, making failures easier to diagnose.♻️ Suggested refactor using it.each
- it('returns false for restricted IPv6 addresses', async () => { - const restrictedIps = ['::1', 'fe80::1', 'fc00::1234', 'ff02::1']; - for (const ip of restrictedIps) { - // eslint-disable-next-line no-await-in-loop - expect(await ssrfModule.checkForSsrf(`[${ip}]`)).toBe(false); - // eslint-disable-next-line no-await-in-loop - expect(await ssrfModule.checkForSsrf(ip)).toBe(false); - } - }); + it.each(['::1', 'fe80::1', 'fc00::1234', 'ff02::1'])('returns false for restricted IPv6 address %s', async (ip) => { + expect(await ssrfModule.checkForSsrf(`[${ip}]`)).toBe(false); + expect(await ssrfModule.checkForSsrf(ip)).toBe(false); + });
3-8: Add tests forcheckForSsrfWithIpfunction.The function is exported from the module and actively used in the fetch implementation for SSRF validation during redirects. It returns
{ allowed: boolean; resolvedIp?: string }, including the resolved IP for IP-pinning validation, but currently has no test coverage.
📜 Review details
Configuration used: Organization 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 (20)
.changeset/eight-games-brake.mdapps/meteor/app/cloud/server/functions/supportedVersionsToken/supportedVersionsToken.tsapps/meteor/app/integrations/server/lib/triggerHandler.tsapps/meteor/app/lib/server/functions/checkUrlForSsrf.tsapps/meteor/app/lib/server/functions/setUserAvatar.tsapps/meteor/app/livechat/imports/server/rest/sms.tsapps/meteor/app/livechat/server/api/v1/webhooks.tsapps/meteor/app/livechat/server/lib/routing/External.tsapps/meteor/app/livechat/server/lib/webhooks.tsapps/meteor/ee/app/livechat-enterprise/server/api/lib/triggers.tsapps/meteor/ee/server/apps/communication/rest.tsapps/meteor/server/services/nps/getAndCreateNpsSurvey.tsapps/meteor/server/services/nps/sendNpsResults.tsapps/meteor/tests/end-to-end/api/chat.tsapps/meteor/tests/unit/app/lib/server/lib/checkUrlForSsrf.tests.tspackages/server-fetch/package.jsonpackages/server-fetch/src/checkForSsrf.tspackages/server-fetch/src/index.tspackages/server-fetch/src/types.tspackages/server-fetch/tests/checkForSsrf.test.ts
💤 Files with no reviewable changes (2)
- apps/meteor/tests/unit/app/lib/server/lib/checkUrlForSsrf.tests.ts
- apps/meteor/app/lib/server/functions/checkUrlForSsrf.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
packages/server-fetch/src/types.tsapps/meteor/app/livechat/server/lib/webhooks.tspackages/server-fetch/src/checkForSsrf.tsapps/meteor/ee/server/apps/communication/rest.tsapps/meteor/app/livechat/server/lib/routing/External.tspackages/server-fetch/tests/checkForSsrf.test.tsapps/meteor/server/services/nps/getAndCreateNpsSurvey.tsapps/meteor/app/cloud/server/functions/supportedVersionsToken/supportedVersionsToken.tsapps/meteor/app/lib/server/functions/setUserAvatar.tspackages/server-fetch/src/index.tsapps/meteor/tests/end-to-end/api/chat.tsapps/meteor/server/services/nps/sendNpsResults.tsapps/meteor/app/livechat/imports/server/rest/sms.tsapps/meteor/app/integrations/server/lib/triggerHandler.tsapps/meteor/app/livechat/server/api/v1/webhooks.tsapps/meteor/ee/app/livechat-enterprise/server/api/lib/triggers.ts
🧠 Learnings (4)
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
packages/server-fetch/tests/checkForSsrf.test.tsapps/meteor/tests/end-to-end/api/chat.ts
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
packages/server-fetch/package.jsonapps/meteor/tests/end-to-end/api/chat.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
packages/server-fetch/package.jsonapps/meteor/tests/end-to-end/api/chat.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Prefer web-first assertions (`toBeVisible`, `toHaveText`, etc.) in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/chat.ts
🧬 Code graph analysis (1)
packages/server-fetch/src/index.ts (2)
packages/server-fetch/src/types.ts (1)
ExtendedFetchOptions(6-14)packages/server-fetch/src/checkForSsrf.ts (1)
checkForSsrfWithIp(91-144)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (19)
apps/meteor/tests/end-to-end/api/chat.ts (1)
1175-1186: LGTM!The URL change from localhost to an external GitHub avatar URL is the correct approach given the new SSRF protections being introduced. GitHub avatars are stable and return
image/pngcontent type, which aligns with the test expectation at line 1223.apps/meteor/app/livechat/imports/server/rest/sms.ts (1)
29-38: No SSRF vulnerability—serverFetchapplies comprehensive SSRF protection by default.The
fileUrlfrom SMS media payloads is protected byserverFetch's built-in SSRF validation, which is enabled by default (without requiring explicit opt-in). The implementation validates the initial request URL and all redirect destinations against restricted IP ranges (IPv4 and IPv6), metadata endpoints, and DNS resolution results. The code is secure without explicitcheckUrlForSsrfcalls orredirect: 'error'constraints.apps/meteor/app/livechat/server/api/v1/webhooks.ts (1)
59-67: LGTM - Appropriate SSRF bypass for admin-configured webhook URL.The
ignoreSsrfValidation: trueis appropriate here since thewebhookUrlis configured by an administrator through settings, and this test endpoint is protected by authentication and theview-livechat-webhookspermission. This aligns with the PR's approach of allowing explicit overrides for trusted contexts.apps/meteor/app/cloud/server/functions/supportedVersionsToken/supportedVersionsToken.ts (1)
114-120: LGTM - Safe SSRF bypass for first-party endpoint.The
ignoreSsrfValidation: trueis appropriate here. ThereleaseEndpointis either the hardcoded Rocket.Chat releases URL (https://releases.rocket.chat/...) or a deployment-controlled environment variable override. Both are trusted sources not influenced by user input.packages/server-fetch/src/types.ts (1)
6-14: LGTM - Clean type extension.The optional
ignoreSsrfValidationfield is well-placed inExtendedFetchOptionsand follows the existing pattern. The default behavior (when omitted) correctly enforces SSRF validation.apps/meteor/server/services/nps/getAndCreateNpsSurvey.ts (1)
35-40: LGTM - Consistent SSRF bypass for admin-configured NPS service URL.The
ignoreSsrfValidation: truefollows the established pattern for admin-configured URLs (Nps_Urlsetting). The request also includes workspace token authentication, indicating this is for trusted first-party service communication.apps/meteor/app/livechat/server/lib/webhooks.ts (1)
23-31: LGTM - Appropriate SSRF bypass for admin-configured webhook.The
ignoreSsrfValidation: trueis appropriate for this webhook handler sinceLivechat_webhookUrlis an administrator-configured setting. This is consistent with the test route inwebhooks.ts(API) and aligns with the PR's trusted context approach.apps/meteor/app/livechat/server/lib/routing/External.ts (1)
54-54: LGTM!The
ignoreSsrfValidation: trueis appropriate here since the URL is sourced from admin-configured settings (Livechat_External_Queue_URL), which represents a trusted context per the PR objectives.apps/meteor/app/integrations/server/lib/triggerHandler.ts (1)
610-619: LGTM!The
ignoreSsrfValidation: trueis appropriate for outgoing webhook integrations where URLs are admin-configured. This aligns with the PR objective to provide secure defaults while allowing explicit overrides for trusted contexts..changeset/eight-games-brake.md (1)
1-6: LGTM!The changeset correctly documents the minor version bumps for both packages. The description accurately summarizes the refactor from
checkUrlForSsrftocheckForSsrfwith added DNS rebinding protection.apps/meteor/server/services/nps/sendNpsResults.ts (1)
21-31: LGTM!The
ignoreSsrfValidation: trueis appropriate here sinceNps_Urlis an admin-configured setting pointing to a trusted NPS service endpoint.apps/meteor/app/lib/server/functions/setUserAvatar.ts (1)
105-113: LGTM — SSRF protection now handled byserverFetchinternally.Since
ignoreSsrfValidationis not specified, the newserverFetchimplementation will apply SSRF validation automatically, including on redirect destinations. This centralizes the security check and is an improvement over the previous per-caller approach.apps/meteor/ee/server/apps/communication/rest.ts (1)
281-301: Marketplace client fetches with disabled SSRF validation are acceptable.These requests go through
Apps.getMarketplaceClient().fetch()which prefixes URLs with a trusted Marketplace base URL. Disabling SSRF validation for first-party controlled endpoints is reasonable.packages/server-fetch/package.json (1)
33-45: LGTM — Jest configuration follows standard patterns.The test configuration uses
ts-jestpreset with appropriate settings. ThetestMatchpattern is consistent with other packages in the monorepo. Based on learnings, this pattern is used across multiple packages.packages/server-fetch/src/checkForSsrf.ts (1)
86-89: LGTM — Clean API wrapper.The
checkForSsrffunction provides a simple boolean interface whilecheckForSsrfWithIpreturns the resolved IP for pinning. Good separation of concerns.packages/server-fetch/src/index.ts (2)
97-125: LGTM — SSRF validation with IP pinning on each redirect.The implementation correctly:
- Validates each URL (including redirect destinations) against SSRF rules
- Pins the request to the resolved IP to prevent DNS rebinding
- Preserves the original hostname for Host header and TLS SNI
This addresses the PR objectives for DNS rebinding mitigation (VLN-178) and redirect validation (VLN-166).
33-47: Certificate validation is correctly preserved.The updated logic no longer disables certificate validation. When
originalHostnameis set, SNI is configured properly withrejectUnauthorized: true. Otherwise, the default agent behavior (full validation) is used.packages/server-fetch/tests/checkForSsrf.test.ts (2)
1-93: Overall: Solid foundation for SSRF protection tests.The test suite covers the core SSRF scenarios well—scheme validation, DNS resolution mocking, IPv4/IPv6 handling, and failure cases. The structure is clean and assertions are clear.
50-58: No action required - tests correctly reflect intended function behavior.The
checkForSsrffunction is explicitly designed to accept both full URLs and raw IP addresses as separate input types. URLs must havehttp:orhttps:protocol (lines 98-101), while raw IP addresses bypass URL parsing and are validated directly (lines 103-143). The tests at lines 56, 65, and 91 are correct and intentional—they verify that raw IPv6 addresses (with or without brackets) are properly rejected when they fall within restricted ranges. No inconsistency exists between the scheme validation tests (lines 10-12) and the raw IP tests.Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 20 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="apps/meteor/ee/server/apps/communication/rest.ts">
<violation number="1" location="apps/meteor/ee/server/apps/communication/rest.ts:262">
P0: Security: User-controlled URL bypasses SSRF protection. The `this.bodyParams.url` is user input that could point to internal services (127.0.0.1, cloud metadata endpoints like 169.254.169.254, etc.). This defeats the purpose of the SSRF protection being added. Remove `ignoreSsrfValidation: true` for user-provided URLs.</violation>
</file>
<file name="packages/server-fetch/src/index.ts">
<violation number="1" location="packages/server-fetch/src/index.ts:146">
P2: `Headers` is not imported but used in `instanceof` check. This will fail for `node-fetch` Headers instances since they differ from the global `Headers` class. Import `Headers` from `node-fetch` to ensure consistent behavior.</violation>
<violation number="2" location="packages/server-fetch/src/index.ts:190">
P1: Response body not consumed during redirect handling. When a 3xx redirect response is received, the code continues to the next iteration without consuming or destroying the response body. In node-fetch, this leads to memory leaks and socket exhaustion. Either consume the body with `response.text()` or destroy it before continuing.</violation>
</file>
<file name="apps/meteor/ee/app/livechat-enterprise/server/api/lib/triggers.ts">
<violation number="1" location="apps/meteor/ee/app/livechat-enterprise/server/api/lib/triggers.ts:22">
P1: Security concern: SSRF validation is disabled for a function that fetches user-configured URLs. Even though this requires `view-livechat-manager` permission, disabling SSRF protection allows the server to be directed to internal IP ranges and cloud metadata endpoints. Consider keeping SSRF validation enabled for external service calls, or add a separate admin setting to explicitly allow internal URLs if that's a legitimate use case.</violation>
</file>
<file name="packages/server-fetch/src/checkForSsrf.ts">
<violation number="1" location="packages/server-fetch/src/checkForSsrf.ts:45">
P1: The `dns.lookup()` call without `{ all: true }` only returns a single resolved IP address, not all of them. An attacker could bypass SSRF protection by registering a domain that resolves to both a safe IP (e.g., public IPv4) and a blocked IP (e.g., internal IPv6 like `::1`). The system may return the safe IP for validation while the actual request uses the blocked one. Use `lookup(hostname, { all: true }, ...)` to retrieve all resolved addresses and validate that every single one passes the range check.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
packages/server-fetch/src/index.ts
Outdated
| const headers: Record<string, string> = {}; | ||
|
|
||
| if (existingHeaders && typeof existingHeaders === 'object') { | ||
| if (existingHeaders instanceof Headers) { |
There was a problem hiding this comment.
P2: Headers is not imported but used in instanceof check. This will fail for node-fetch Headers instances since they differ from the global Headers class. Import Headers from node-fetch to ensure consistent behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/server-fetch/src/index.ts, line 146:
<comment>`Headers` is not imported but used in `instanceof` check. This will fail for `node-fetch` Headers instances since they differ from the global `Headers` class. Import `Headers` from `node-fetch` to ensure consistent behavior.</comment>
<file context>
@@ -45,33 +53,155 @@ function getTimeout(timeout?: number) {
+ const headers: Record<string, string> = {};
+
+ if (existingHeaders && typeof existingHeaders === 'object') {
+ if (existingHeaders instanceof Headers) {
+ existingHeaders.forEach((value, key) => {
+ headers[key] = value;
</file context>
There was a problem hiding this comment.
Not sure this applies here. I've tested it and all requests seem to be working as expected. Do you see a reason to do it this way, @cardoso?
apps/meteor/app/cloud/server/functions/supportedVersionsToken/supportedVersionsToken.ts
Show resolved
Hide resolved
Co-authored-by: Kevin Aleman <kaleman960@gmail.com>
Co-authored-by: Kevin Aleman <kaleman960@gmail.com>
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/server-fetch/src/index.ts">
<violation number="1" location="packages/server-fetch/src/index.ts:21">
P2: The IPv6 regex allows mismatched brackets (e.g., `[::1` or `::1]`). The brackets should be either both present or both absent for valid IPv6 format. Change the alternation to explicitly handle both cases separately.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/server-fetch/src/checkForSsrf.ts`:
- Around line 71-72: The domain validation regex in domainPattern used by
isValidDomain is overly restrictive because the (?!.*--) negative lookahead
blocks valid punycode (xn--) and other hyphenated labels; update domainPattern
to remove the (?!.*--) check while retaining the existing leading/trailing
hyphen guards (^(?!-)(?<!-)), label length limits, and TLD letter requirement so
labels still match [A-Za-z0-9-]{1,63} and overall structure, then run/update
unit tests to ensure xn-- and other hyphenated domains (e.g., xn--nxasmq6b.com)
are accepted and existing invalid cases remain rejected.
In `@packages/server-fetch/src/index.ts`:
- Around line 112-132: The code logs full URLs (url and pinnedUrl) which can
leak secrets; update logs in the block guarded by ignoreSsrfValidation to redact
full URLs and log only the hostname/origin instead: use extractHostname(url) or
originalHostname (after you set it) and fall back to a safe placeholder when
hostname is missing; change logger.error({ msg: 'SSRF validation failed for
URL', url }) to logger.error({ msg: 'SSRF validation failed for host', host:
extractedHostname || 'unknown' }) and change logger.debug({ msg: 'Request not
using SSRF validation', url: pinnedUrl }) to logger.debug({ msg: 'Request not
using SSRF validation', host: extractHostname(pinnedUrl) || 'unknown' }); keep
the other logic (checkForSsrfWithIp, resolvedIp, buildPinnedUrl) unchanged.
♻️ Duplicate comments (1)
packages/server-fetch/src/index.ts (1)
189-200:Headersinstanceof check can drop headers.
Headersisn’t imported, soinstanceof Headerswill only match the global implementation (if present), notnode-fetch’sHeaders. This can silently skip header propagation. ImportHeadersfromnode-fetchor use a duck‑type check.🔧 Proposed fix
-import fetch, { Response } from 'node-fetch'; +import fetch, { Response, Headers } from 'node-fetch';
🧹 Nitpick comments (3)
packages/server-fetch/src/index.ts (2)
65-93:allowSelfSignedCertsis now effectively a no‑op.The branch returns the default agent path regardless, so callers won’t actually get relaxed TLS checks. If this hardening is intentional, consider deprecating or renaming the flag to avoid a misleading API.
88-90: Remove inline implementation comments.Prefer keeping the implementation comment-free and move rationale to docs or commit messages.
As per coding guidelines, avoid code comments in the implementation.🔧 Proposed cleanup
- // For self-signed certificates, do not disable certificate validation. - // Instead, fall back to the default HTTPS agent behavior (full validation). return null; @@ - // https://github.com/node-fetch/node-fetch/issues/1673 - body not consumed == open socket controller.abort();Also applies to: 234-234
packages/server-fetch/src/checkForSsrf.ts (1)
74-140: Inconsistent handling of non-URL host:port inputs.The
parseIpv4WithPortfallback handles IPv4 with optional port (e.g.,192.168.1.1:8443), but rejectsexample.com:8443and[IPv6]:8443inputs without a scheme. TheisValidDomainpattern explicitly rejects colons, so domain:port inputs fail validation. Bracketed IPv6 with port fails because the bracket-stripping logic (lines 112-114) doesn't match when a port is appended ([::1]:8443ends with3, not]).To support non-URL host:port inputs consistently across all three formats, add a parser that handles domains and bracketed IPv6 in addition to IPv4.
🔧 Proposed fix
+const parseHostWithPort = (input: string): { host: string; port?: string } | null => { + const match = input.match(/^\[([0-9a-fA-F:]+)\](?::(\d+))?$|^([A-Za-z0-9.-]+)(?::(\d+))?$/); + if (!match) { + return null; + } + return { host: match[1] ?? match[3], port: match[2] ?? match[4] }; +}; + export const checkForSsrfWithIp = async (input: string): Promise<{ allowed: false } | { allowed: true; resolvedIp: string }> => { let ipOrDomain: string; let port: string | undefined; let wasUrlParsed = false; try { const url = new URL(input); if (url.protocol !== 'http:' && url.protocol !== 'https:') return { allowed: false }; ipOrDomain = url.hostname; port = url.port || undefined; wasUrlParsed = true; } catch { - const parsed = parseIpv4WithPort(input); - if (parsed) { - ipOrDomain = parsed.ip; - port = parsed.port; - } else { - ipOrDomain = input; - } + const parsedIpv4 = parseIpv4WithPort(input); + if (parsedIpv4) { + ipOrDomain = parsedIpv4.ip; + port = parsedIpv4.port; + } else { + const parsedHost = parseHostWithPort(input); + if (parsedHost) { + ipOrDomain = parsedHost.host; + port = parsedHost.port; + } else { + ipOrDomain = input; + } + } }
📜 Review details
Configuration used: Organization 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 ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
packages/server-fetch/package.jsonpackages/server-fetch/src/checkForSsrf.tspackages/server-fetch/src/index.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
packages/server-fetch/src/checkForSsrf.tspackages/server-fetch/src/index.ts
🧠 Learnings (1)
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
packages/server-fetch/package.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (5)
packages/server-fetch/package.json (1)
10-46: Test tooling additions look good.Scripts, devDependencies, and Jest config are consistent with ts-jest usage for this package.
packages/server-fetch/src/checkForSsrf.ts (2)
5-41: Reserved/private range lists look solid.No issues spotted with the blocked range lists.
43-69: DNS and IP helper utilities are straightforward.The lookup and range-check helpers keep the main flow simple and readable.
packages/server-fetch/src/index.ts (2)
20-63: Hostname/IP helper functions look good.The extraction and pinning helpers are clear and self-contained.
144-159: Redirect guardrails are clear and well‑structured.The missing‑location and max‑redirect checks keep the redirect loop predictable.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/server/services/nps/getAndCreateNpsSurvey.ts (1)
35-41: Remove the inline SECURITY comment per coding guidelines.The
ignoreSsrfValidation: truebypass is justified—Nps_Urlis defined as a readonly, hidden setting that cannot be modified via the UI, establishing sufficient trust boundary. However, the inline comment violates the "avoid code comments in the implementation" guideline. Remove line 39's comment entirely; if rationale documentation is needed, document it in an ADR or architecture notes instead.
🧹 Nitpick comments (2)
apps/meteor/app/integrations/server/lib/triggerHandler.ts (1)
614-623: SSRF bypass is unconditional for outgoing integrations — verify that’s intended.
This skips the new SSRF protections for every integration request, andopts.urlcan be computed by scripts (often using message data). Please confirm only highly trusted admins can create/modify outgoing integrations and scripts; otherwise consider making the bypass conditional (e.g., a per‑integration “trusted” flag or allowlist) rather than unconditional. Also remove the inline SECURITY comment per the no‑comments guideline. As per coding guidelines, avoid inline implementation comments.apps/meteor/app/livechat/server/api/v1/webhooks.ts (1)
59-68: Verify trust boundary for livechat webhook test SSRF bypass.
This route now disables SSRF validation while posting to the configuredLivechat_webhookUrl. Please confirm that both the setting and theview-livechat-webhookspermission are restricted to trusted admins; otherwise keep SSRF validation or gate the bypass behind an explicit allowlist/flag. Also remove the inline SECURITY comment per the no‑comments guideline. As per coding guidelines, avoid inline implementation comments.
📜 Review details
Configuration used: Organization 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 (8)
apps/meteor/app/cloud/server/functions/supportedVersionsToken/supportedVersionsToken.tsapps/meteor/app/integrations/server/lib/triggerHandler.tsapps/meteor/app/livechat/server/api/v1/webhooks.tsapps/meteor/app/livechat/server/lib/routing/External.tsapps/meteor/app/livechat/server/lib/webhooks.tsapps/meteor/ee/app/livechat-enterprise/server/api/lib/triggers.tsapps/meteor/server/services/nps/getAndCreateNpsSurvey.tsapps/meteor/server/services/nps/sendNpsResults.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/meteor/app/livechat/server/lib/webhooks.ts
- apps/meteor/app/livechat/server/lib/routing/External.ts
- apps/meteor/ee/app/livechat-enterprise/server/api/lib/triggers.ts
- apps/meteor/app/cloud/server/functions/supportedVersionsToken/supportedVersionsToken.ts
- apps/meteor/server/services/nps/sendNpsResults.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/livechat/server/api/v1/webhooks.tsapps/meteor/server/services/nps/getAndCreateNpsSurvey.tsapps/meteor/app/integrations/server/lib/triggerHandler.ts
🧠 Learnings (1)
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
apps/meteor/app/livechat/server/api/v1/webhooks.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/server-fetch/src/index.ts">
<violation number="1" location="packages/server-fetch/src/index.ts:163">
P0: Moving the timeout initialization outside the loop breaks redirect handling. After the first redirect, `controller.abort()` is called (line 231), and `clearTimeout(timeoutId)` runs in `.finally()`. On subsequent iterations, `fetch()` receives an already-aborted signal and will throw immediately. Each redirect iteration needs its own fresh AbortController.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/server-fetch/src/index.ts`:
- Around line 163-235: The abort controller and its timeout are created once via
getTimeout(...) and reused across redirects, but controller.abort() is called
after each redirect (controller.abort()) which permanently sets signal.aborted
and also clears the single timeout (timeoutId) in the fetch .finally(), so
subsequent fetches immediately abort and lack timeout protection; fix by moving
getTimeout(...) into the redirect loop so each iteration calls getTimeout(...)
to produce a fresh { controller, timeoutId } per request, use that
controller.signal for the fetch, ensure you call clearTimeout(timeoutId) and
controller.abort() only for that iteration (e.g., in the fetch .finally() and
after followRedirect when consuming the body), and remove reliance on the
outer-scope controller/timeoutId so every redirect request has its own
controller and timeout.
📜 Review details
Configuration used: Organization 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 (1)
packages/server-fetch/src/index.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
packages/server-fetch/src/index.ts
🧠 Learnings (1)
📚 Learning: 2025-11-10T19:06:20.146Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37408
File: apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx:53-69
Timestamp: 2025-11-10T19:06:20.146Z
Learning: In the Rocket.Chat repository, do not provide suggestions or recommendations about code sections marked with TODO comments. The maintainers have already identified these as future work and external reviewers lack the full context about implementation plans and timing.
Applied to files:
packages/server-fetch/src/index.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (8)
packages/server-fetch/src/index.ts (8)
15-22: LGTM on constants and helper setup.The
MAX_REDIRECTSlimit andredirectStatusset are appropriate for manual redirect handling. ThecheckDirectIpregex issue was previously discussed and dismissed by the author.
24-34: LGTM!The
extractHostnamefunction correctly parses URLs and handles IPv6 bracket stripping.
36-63: LGTM!The
buildPinnedUrlfunction correctly handles IPv4/IPv6 addresses with and without ports, properly adding brackets for IPv6 when needed.
82-96: Verify interaction betweenoriginalHostnameandallowSelfSignedCerts.When
originalHostnameis provided (IP-pinning scenario),rejectUnauthorizedis always set totrueat line 86, ignoringallowSelfSignedCerts. This means self-signed certificates will be rejected even whenallowSelfSignedCertsis true. If this is intentional for security hardening, consider documenting this behavior; otherwise, the logic may need adjustment.
98-135: LGTM on SSRF validation flow.The function correctly validates URLs against SSRF, extracts hostnames, and pins URLs to resolved IPs to mitigate TOCTOU/DNS rebinding attacks.
144-159: LGTM!The
followRedirectfunction properly validates the redirect response, throws appropriate errors for missing Location headers and excessive redirects, and provides useful debug logging.
184-211: LGTM on header normalization and Host header handling.The header normalization correctly handles the three possible formats (
Headersinstance, array of tuples, and plain object). TheHostheader is properly set when IP-pinning is active to ensure the server receives the original hostname.
240-241: LGTM on exports.Exporting
Responsefromnode-fetchandExtendedFetchOptionsprovides a clean API for consumers.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/server-fetch/src/index.ts">
<violation number="1" location="packages/server-fetch/src/index.ts:231">
P1: Using `void` on a promise without a `.catch()` can cause unhandled promise rejections if `arrayBuffer()` fails (e.g., network errors). This could crash the Node.js process or cause noisy error logs. Add error handling to safely ignore errors while still consuming the body.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
3 participants
Proposed changes (including videos or screenshots)
In short, a server-side request forgery (SSRF) allows the attacker to force the server hosting the vulnerable application to send a request to any domain / IP the attacker desires. In this case, attackers can force the servers hosting Rocket.Chat to send requests to other IPs and domains.
When fixing this kind of issue, one can either allowlist or denylist certain IPs/domains. In Rocket.Chat's use case, allowlisting is generally not an option. Thus, we've decided to use a denylist approach and create the
checkUrlForSsrffunction.When denylisting, we must have layers of security in place:
Do not allow protocol schemes that are not http:// and/or https://. This prevents attackers from using other protocol schemes such as gopher://, ftp://, file://, and so forth.
Do not allow redirects. If we allow redirects, an attacker could register a domain that wouldn’t be in our blacklist and force it to redirect to an IP / domain that is blacklisted - successfully bypassing our filters. For example, an attacker may register evil.io an force any requests to redirect to 169.254.169.254 in order to reach the metadata instance. Since we are using node-fetch, we can have something like fetch(fileUrl, {redirect: 'error'}) to prevent redirects.
Perform a lookup of the domain to block restricted IPs. Attackers can register a domain such as malicious.example.io, creating an entry to resolve to a restricted IP address such as 169.254.169.254 and bypass SSRF filters.
DNS rebinding can become an issue for DNS requests with short-lived TTLs - i.e. an attacker may configure a DNS entry to first respond with a legitimate IP address and then change it to an internal/malicious one, causing a TOCTOU problem.
Block all internal IP ranges and default cloud services IPs. If we only need FQDNs to be accepted, we can block IPs altogether.
We must also take into account that there is more than one way to represent the same thing. For example, localhost can be represented with 127.0.0.1 (IPv4) and [::] or [0000::1] (IPv6), among others. IPs can also be represented with decimals - e.g. 127.0.0.1 becomes 2130706433 - and octals - e.g. 127.0.0.1 becomes 0177.0.0.1.
This pull request changes the old
checkUrlForSsrffunction. It is now inside theserver-fetchpackage and it includes anignoreSsrfValidationoption for use cases we know there's no danger in allowing internal IPs. It also supports IPv6 and URLs containing IP + port, includes a way of protecting against DNS rebinding, and solves the issue we had with the previous version of having to disable redirects (redirects are now handled within the package itself).Issue(s)
VLN-154, VLN-164, VLN-166, VLN-167, VLN-178
Steps to test or reproduce
N/A
Further comments
N/A
Summary by CodeRabbit
Refactor
Bug Fixes
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.
Fixes #36491 (since redirects are now followed)