Fix SSRF vulnerability in isSafeUri validation (issue #245)#268
Open
anshul23102 wants to merge 1 commit into
Open
Fix SSRF vulnerability in isSafeUri validation (issue #245)#268anshul23102 wants to merge 1 commit into
anshul23102 wants to merge 1 commit into
Conversation
…tadata IPs Issue geturbackend#245: The isSafeUri validation only blocked localhost and 127.0.0.1, allowing attackers to supply MongoDB URIs pointing to AWS metadata endpoints (169.254.169.254), private IP ranges (10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16), or link-local addresses, causing the server to initiate connections to internal infrastructure. Changes: - Add validation for AWS metadata IPs (169.254.169.254, fd00:ec2::254) - Block all RFC-1918 private address ranges using regex patterns - Block IPv6 link-local addresses (fe80::/10) - Block IPv6 unique local addresses (fc00::/7) - Prevents SSRF attacks via external database URI configuration This prevents authenticated developers from using SSRF to probe internal services or exfiltrate metadata through crafted MongoDB wire protocol responses. Fixes geturbackend#245
Contributor
📝 WalkthroughWalkthroughThe PR enhances the ChangesSSRF Mitigation Enhancement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/dashboard-api/src/controllers/project.controller.js`:
- Around line 491-533: The current isSafeUri only inspects the hostname string
and ignores DNS resolution, allowing DNS-rebinding/internal hostnames to pass;
update isSafeUri to perform asynchronous DNS resolution (e.g.,
dns.promises.lookup / resolve with all records) for the parsed hostname and
validate every resolved IP (IPv4 and IPv6) against the same RFC1918, link-local,
loopback, metadata and unique-local checks, returning false if any resolved
address is unsafe; make isSafeUri async and update all call sites (including the
mongoose.createConnection call path) to await it and reject/throw when a URI is
not safe.
- Around line 522-525: The current check /^fe80:/i only blocks fe80:: addresses
but the IPv6 link-local range is fe80::/10 (fe80 through febf); update the
rejection regex that tests ipAddress to match any address whose second hex byte
is 0x80–0xbf, e.g. replace /^fe80:/i with a pattern like
/^fe(?:8|9|a|b)[0-9a-f]:/i so ipAddress values fe90::, fea0::, febf:: etc. are
also rejected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 19cf3cc9-0cdd-4465-ab5f-4316b67b2fce
📒 Files selected for processing (1)
apps/dashboard-api/src/controllers/project.controller.js
Comment on lines
491
to
533
| const isSafeUri = (uri) => { | ||
| try { | ||
| const parsed = new URL(uri); | ||
| const host = parsed.hostname.toLowerCase(); | ||
| const badHosts = ["localhost", "127.0.0.1", "0.0.0.0", "::1"]; | ||
| return !badHosts.includes(host); | ||
|
|
||
| // Blocked hostnames | ||
| const blockedHosts = [ | ||
| "localhost", | ||
| "127.0.0.1", | ||
| "0.0.0.0", | ||
| "::1", | ||
| "169.254.169.254", // AWS metadata service | ||
| "fd00:ec2::254", // IPv6 AWS metadata | ||
| ]; | ||
|
|
||
| if (blockedHosts.includes(host)) { | ||
| return false; | ||
| } | ||
|
|
||
| // Parse IP address if hostname is an IP | ||
| const ipAddress = host; | ||
|
|
||
| // Reject RFC-1918 private address ranges | ||
| if ( | ||
| /^10\.\d{1,3}\.\d{1,3}\.\d{1,3}$/.test(ipAddress) || // 10.0.0.0/8 | ||
| /^172\.(1[6-9]|2\d|3[01])\.\d{1,3}\.\d{1,3}$/.test(ipAddress) || // 172.16.0.0/12 | ||
| /^192\.168\.\d{1,3}\.\d{1,3}$/.test(ipAddress) // 192.168.0.0/16 | ||
| ) { | ||
| return false; | ||
| } | ||
|
|
||
| // Reject IPv6 link-local addresses (fe80::/10) | ||
| if (/^fe80:/i.test(ipAddress)) { | ||
| return false; | ||
| } | ||
|
|
||
| // Reject IPv6 unique local addresses (fc00::/7) | ||
| if (/^f[cd][0-9a-f]{2}:/i.test(ipAddress)) { | ||
| return false; | ||
| } | ||
|
|
||
| return true; | ||
| } catch (e) { |
Contributor
There was a problem hiding this comment.
Hostname inputs can still SSRF internal targets (DNS rebinding/internal DNS).
Line 493/494 validates only the hostname string; it does not resolve hostnames and validate resolved addresses. An attacker-controlled hostname that resolves to RFC1918/link-local/metadata IPs will pass isSafeUri and still reach mongoose.createConnection (Line 559).
Suggested direction
+const dns = require("dns").promises;
+const net = require("net");
-const isSafeUri = (uri) => {
+const isSafeUri = async (uri) => {
try {
const parsed = new URL(uri);
const host = parsed.hostname.toLowerCase();
+ const resolved = net.isIP(host)
+ ? [{ address: host }]
+ : await dns.lookup(host, { all: true, verbatim: true });
- const ipAddress = host;
+ for (const { address: ipAddress } of resolved) {
+ // apply blockedHosts / RFC1918 / metadata / IPv6 checks to ipAddress
+ // return false on first unsafe address
+ }
return true;
} catch (e) {
return false;
}
};
- if (!isSafeUri(dbUri))
+ if (!(await isSafeUri(dbUri)))
return res.status(400).json({Also applies to: 547-549
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/dashboard-api/src/controllers/project.controller.js` around lines 491 -
533, The current isSafeUri only inspects the hostname string and ignores DNS
resolution, allowing DNS-rebinding/internal hostnames to pass; update isSafeUri
to perform asynchronous DNS resolution (e.g., dns.promises.lookup / resolve with
all records) for the parsed hostname and validate every resolved IP (IPv4 and
IPv6) against the same RFC1918, link-local, loopback, metadata and unique-local
checks, returning false if any resolved address is unsafe; make isSafeUri async
and update all call sites (including the mongoose.createConnection call path) to
await it and reject/throw when a URI is not safe.
Comment on lines
+522
to
+525
| // Reject IPv6 link-local addresses (fe80::/10) | ||
| if (/^fe80:/i.test(ipAddress)) { | ||
| return false; | ||
| } |
Contributor
There was a problem hiding this comment.
fe80::/10 blocking is incomplete and bypassable.
Line 523 only blocks fe80:*, but link-local IPv6 spans fe80 through febf. This still allows SSRF to link-local addresses like fe90::1.
Suggested fix
- // Reject IPv6 link-local addresses (fe80::/10)
- if (/^fe80:/i.test(ipAddress)) {
+ // Reject IPv6 link-local addresses (fe80::/10 => fe80..febf)
+ if (/^fe[89ab][0-9a-f]:/i.test(ipAddress)) {
return false;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Reject IPv6 link-local addresses (fe80::/10) | |
| if (/^fe80:/i.test(ipAddress)) { | |
| return false; | |
| } | |
| // Reject IPv6 link-local addresses (fe80::/10 => fe80..febf) | |
| if (/^fe[89ab][0-9a-f]:/i.test(ipAddress)) { | |
| return false; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/dashboard-api/src/controllers/project.controller.js` around lines 522 -
525, The current check /^fe80:/i only blocks fe80:: addresses but the IPv6
link-local range is fe80::/10 (fe80 through febf); update the rejection regex
that tests ipAddress to match any address whose second hex byte is 0x80–0xbf,
e.g. replace /^fe80:/i with a pattern like /^fe(?:8|9|a|b)[0-9a-f]:/i so
ipAddress values fe90::, fea0::, febf:: etc. are also rejected.
Contributor
Author
|
@geturbackend Could you please add the appropriate labels ( |
Collaborator
|
I only add labels after merging. |
Collaborator
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #245: isSafeUri now properly blocks RFC-1918 private ranges, AWS metadata endpoints, and IPv6 link-local addresses. Prevents Server-Side Request Forgery attacks via external database URI configuration.
Summary by CodeRabbit