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 |
🦋 Changeset detectedLatest commit: 3cac769 The changes in this PR will be included in the next version bump. This PR includes changesets to release 41 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 |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ 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}
| rejectUnauthorized: false, | ||
| }); | ||
| const agentOptions: https.AgentOptions = { | ||
| rejectUnauthorized: false, |
Check failure
Code scanning / CodeQL
Disabling certificate validation High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 17 hours ago
In general, the fix is to stop disabling certificate validation. That means we must avoid rejectUnauthorized: false and instead either (a) use the default HTTPS agent (which validates certificates), or (b) configure a custom agent that still validates certificates but can trust specific non‑standard CAs when needed by providing an explicit CA bundle or similar.
The minimal change that preserves existing functionality while removing the insecure pattern is:
- Do not set
rejectUnauthorized: false. - When
allowSelfSignedCertsisfalse(or unset), keep returningnullso thatnode-fetchuses its default, secure HTTPS agent. - When
allowSelfSignedCertsistrue:- For HTTP, continue to use
new http.Agent()as before. - For HTTPS, replace the custom agent that disables validation with either:
- the default agent (by returning
nullas for the non‑self‑signed case), or - a custom
https.Agentthat still validates certificates (e.g., no special options other than possiblyservername).
- the default agent (by returning
- For HTTP, continue to use
Because originalHostname is used only to set SNI, and SNI can still be set on a validating agent, we can keep that behavior without disabling certificate checks by constructing an https.Agent without overriding rejectUnauthorized. The cleanest change inside getFetchAgent is:
- Remove
rejectUnauthorized: falsefromagentOptions. - Keep
agentOptionsbut make it an empty object initially, and only setservernamewhenoriginalHostnameis provided. - Return a new
https.Agent(agentOptions); this agent will use the defaultrejectUnauthorized: true.
Concretely, within packages/server-fetch/src/index.ts:
- In
getFetchAgent, change theagentOptionsdefinition and remove therejectUnauthorized: falseline. - Leave the function signature and all other logic intact.
No new imports are required; we are only changing the options passed to https.Agent.
| @@ -34,9 +34,7 @@ | ||
| return new http.Agent(); | ||
| } | ||
|
|
||
| const agentOptions: https.AgentOptions = { | ||
| rejectUnauthorized: false, | ||
| }; | ||
| const agentOptions: https.AgentOptions = {}; | ||
|
|
||
| if (originalHostname) { | ||
| agentOptions.servername = originalHostname; |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #38044 +/- ##
===========================================
- Coverage 70.65% 70.62% -0.03%
===========================================
Files 3143 3142 -1
Lines 108684 108631 -53
Branches 19571 19551 -20
===========================================
- Hits 76787 76724 -63
- Misses 29893 29906 +13
+ Partials 2004 2001 -3
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/02 23:20 (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: Fri, 02 Jan 2026 23:20:19 GMT |
julio-rocketchat
changed the title
checkUrlForSsrf and include it in the server-fetch packagecheckUrlForSsrf and include it in the server-fetch package
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments