fix(project): expand isSafeUri to block RFC-1918 and cloud metadata IP ranges#256
Conversation
…P ranges
The previous implementation checked only four literal values (localhost,
127.0.0.1, 0.0.0.0, ::1). An attacker could supply a MongoDB URI whose
host is any RFC-1918 private address (e.g. 10.0.0.1, 172.16.0.1,
192.168.1.1) or the cloud instance metadata endpoint (169.254.169.254)
and bypass the check entirely, enabling SSRF against internal services.
Changes:
- Add net module import for IP validation.
- Extract ipv4ToInt() helper that converts a dotted-decimal string to a
32-bit unsigned integer for range comparisons.
- Add isRestrictedIPv4() that checks all restricted IPv4 ranges:
- Loopback: 127.0.0.0/8
- RFC-1918: 10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16
- Link-local /
cloud metadata: 169.254.0.0/16 (covers 169.254.169.254)
- Unspecified: 0.0.0.0/8
- Rewrite isSafeUri to call net.isIPv4() and route numeric hosts through
isRestrictedIPv4(), keeping the hostname denylist for localhost and
metadata.google.internal, and adding an IPv6 loopback check.
Closes geturbackend#245
|
Warning Review limit reached
More reviews will be available in 52 minutes and 2 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRewrites URI safety checks in project.controller.js: imports Node ChangesSSRF Prevention Through URI Safety Checks
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 517-526: isSafeUri currently only blocks literal IPv4/IPv6
loopback and a couple hostnames, so resolve the hostname portion of the provided
dbUri (use DNS lookup/resolve via node's dns.promises) and reject the URI if any
resolved address falls into restricted ranges (RFC1918 private IPv4, link-local
169.254.0.0/16, 127/8, metadata IPs, and IPv6 equivalents including fc00::/7,
fe80::/10, ::1, etc.); make isSafeUri async (or add an async helper
isSafeUriAsync) and update the callsite that does if (!isSafeUri(dbUri)) to
await the check before calling mongoose.createConnection(dbUri, ...), ensuring
you strip brackets for IPv6, handle DNS CNAMEs that resolve to multiple
addresses, and treat any restricted resolved address as unsafe.
🪄 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: ecb91d4e-c147-41ab-8c71-576c19258c7d
📒 Files selected for processing (1)
apps/dashboard-api/src/controllers/project.controller.js
yash-pouranik
left a comment
yash-pouranik
left a comment
There was a problem hiding this comment.
please resolve coderabbits comments @anshul23102
Add async DNS lookups to detect when hostnames resolve to restricted IP ranges. Implement isRestrictedIPv6 to block link-local (fe80::/10) and ULA (fc00::/7) ranges. Update callsite to await the now-async isSafeUri check. Signed-off-by: Anshul Jain <anshul23102@iiitd.ac.in>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/dashboard-api/src/controllers/project.controller.js (2)
575-605:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStop returning raw driver errors from the BYOD validation path.
This branch now sits directly behind the new SSRF check, but it still returns raw
{ error: ... }payloads and appendsconnErr.messageto the client response. That leaks connection internals from attacker-controlled URIs and breaks the controller contract for dashboard APIs.As per coding guidelines, "All API endpoints must return response format:
{ success: bool, data: {}, message: "" }. Use AppError class for errors. Never use raw throw statements or expose MongoDB errors to clients."🤖 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 575 - 605, The BYOD validation currently returns raw driver messages from the mongoose.createConnection error path; change the catch block in the project DB validation (around isSafeUri, encrypt, mongoose.createConnection, getPublicIp) to NOT include connErr.message in client responses: log connErr (e.g., console.error or logger.error) for diagnostics, and return errors using the AppError pattern or the controller's standard response shape — e.g., call next(new AppError("Could not verify MongoDB connection. Please ensure the host is accessible and whitelisted", 400)) or send res.status(400).json({ success: false, data: {}, message: "Could not verify MongoDB connection. Please ensure the host is accessible and whitelisted" }) and only include the whitelisting hint (with getPublicIp()) as a generic instruction, never raw driver text.
526-559:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftSSRF gate misses
mongodb+srv://SRV-discovered targets.In
apps/dashboard-api/src/controllers/project.controller.js(lines ~526-559),isSafeUrionly parses the URI’s provided hostname and blocks it based on direct hostname A/AAAA resolution (plus a small blocked-hostname list). Formongodb+srv://, the MongoDB driver performs SRV seedlist discovery (_mongodb._tcp.<seed>) and connects to the hostnames returned by the SRV records; those SRV-returned targets are not resolved/checked byisSafeUri, so a public “seed” hostname can still yield private/internal SRV targets and bypass the SSRF gate.🤖 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 526 - 559, isSafeUri currently only checks the parsed hostname and its A/AAAA records but misses SRV-discovered targets for mongodb+srv:// URIs; update isSafeUri to detect URL.protocol === "mongodb+srv:" (or scheme startsWith "mongodb+srv") and perform a dns.resolveSrv("_mongodb._tcp." + host), then for each SRV.record.target resolve their A/AAAA addresses (using dns.resolve4/resolve6) and run them through the existing isRestrictedIPv4/isRestrictedIPv6/isRestrictedIP checks; if the SRV lookup or any resolution yields a restricted address, return false, and if SRV lookup fails treat the URI as unsafe (consistent with current A/AAAA failure handling). Ensure you reference and reuse the existing helper functions isRestrictedIP, isRestrictedIPv4, isRestrictedIPv6 and DNS functions (dns.resolveSrv, dns.resolve4, dns.resolve6) and keep the original fallback behavior for non-mongodb+srv schemes.
🤖 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 509-517: The isRestrictedIPv6 function currently misses the full
fe80::/10 range and treats IPv4-mapped IPv6 (::ffff:...) as public; update
isRestrictedIPv6 to (1) normalize the input as done now, (2) early-handle IPv6
loopback variants (::1, 0:0:0:0:0:0:0:1), (3) detect and treat IPv4-mapped
literals starting with ::ffff: by extracting the embedded IPv4 and calling
whatever IPv4-private check you have (or apply RFC1918/loopback rules), (4)
correctly match the link-local fe80::/10 prefix (not just fe80:) and the ULA
fc00::/7 (checking first 7 bits / prefixes fc00..fdff), and (5) keep existing
bracket stripping and lowercasing; update isRestrictedIPv6 accordingly so
::ffff:127.0.0.1 and any fe9*:.. or febf:* addresses are flagged restricted.
- Around line 540-556: The hostname resolution currently only calls
dns.resolve6(host) if dns.resolve4(host) throws, which allows a dual-stack host
with a safe A record but restricted AAAA to bypass checks; in isSafeUri, call
both dns.resolve4(host) and dns.resolve6(host) (independently, catching errors
for each) and inspect every returned address with isRestrictedIPv4 and
isRestrictedIPv6 respectively, returning false if any restricted address is
found; only allow the hostname when neither resolution produced a restricted
address (and handle the case where both lookups fail per existing policy).
---
Outside diff comments:
In `@apps/dashboard-api/src/controllers/project.controller.js`:
- Around line 575-605: The BYOD validation currently returns raw driver messages
from the mongoose.createConnection error path; change the catch block in the
project DB validation (around isSafeUri, encrypt, mongoose.createConnection,
getPublicIp) to NOT include connErr.message in client responses: log connErr
(e.g., console.error or logger.error) for diagnostics, and return errors using
the AppError pattern or the controller's standard response shape — e.g., call
next(new AppError("Could not verify MongoDB connection. Please ensure the host
is accessible and whitelisted", 400)) or send res.status(400).json({ success:
false, data: {}, message: "Could not verify MongoDB connection. Please ensure
the host is accessible and whitelisted" }) and only include the whitelisting
hint (with getPublicIp()) as a generic instruction, never raw driver text.
- Around line 526-559: isSafeUri currently only checks the parsed hostname and
its A/AAAA records but misses SRV-discovered targets for mongodb+srv:// URIs;
update isSafeUri to detect URL.protocol === "mongodb+srv:" (or scheme startsWith
"mongodb+srv") and perform a dns.resolveSrv("_mongodb._tcp." + host), then for
each SRV.record.target resolve their A/AAAA addresses (using
dns.resolve4/resolve6) and run them through the existing
isRestrictedIPv4/isRestrictedIPv6/isRestrictedIP checks; if the SRV lookup or
any resolution yields a restricted address, return false, and if SRV lookup
fails treat the URI as unsafe (consistent with current A/AAAA failure handling).
Ensure you reference and reuse the existing helper functions isRestrictedIP,
isRestrictedIPv4, isRestrictedIPv6 and DNS functions (dns.resolveSrv,
dns.resolve4, dns.resolve6) and keep the original fallback behavior for
non-mongodb+srv schemes.
🪄 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: 6f6d4136-90b9-4540-ab91-2fa5a709c66d
📒 Files selected for processing (1)
apps/dashboard-api/src/controllers/project.controller.js
…k DNS resolution Enhance isRestrictedIPv6 to block full fe80::/10 range (fe80: through febf:), detect IPv4-mapped IPv6 addresses (::ffff:x.x.x.x), and block unspecified address (::). Update isSafeUri to resolve both A and AAAA records simultaneously using Promise.allSettled, ensuring dual-stack hostnames with safe A but restricted AAAA records cannot bypass checks. Signed-off-by: Anshul Jain <anshul23102@iiitd.ac.in>
Add comprehensive JSDoc documentation to all SSRF prevention helper functions: - ipv4ToInt(): converts dotted-decimal IPv4 to 32-bit integer - isRestrictedIPv4(): validates against IPv4 restricted ranges (loopback, RFC-1918, cloud metadata, unspecified) - isRestrictedIPv6(): validates against IPv6 restricted ranges (loopback, link-local, ULA, IPv4-mapped) - isRestrictedIP(): dispatches validation based on IP version - isSafeUri(): async DNS-aware URI validation for SSRF prevention Satisfies CodeRabbit docstring coverage requirement (80% threshold).
|
Resolved CodeRabbit docstring comments: Added comprehensive JSDoc documentation to all SSRF validation helper functions:
Each function now includes:
Docstring coverage requirement (80% threshold) is now satisfied. The push updated the PR branch with the new documentation commit. |
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/dashboard-api/src/controllers/project.controller.js (2)
611-641:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep
updateExternalConfigon the controller error contract.This path still returns raw
{ error: ... }payloads, and the fallback appendsconnErr.messagedirectly into the client response. That both violates the shared API contract and exposes MongoDB driver/server details from a user-supplied URI. Please route these failures throughAppError, or at minimum return{ success: false, data: {}, message }without the raw Mongo error text. As per coding guidelines, "All API endpoints must return response format:{ success: bool, data: {}, message: "" }. Use AppError class for errors. Never use raw throw statements or expose MongoDB errors to clients."Also applies to: 671-677
🤖 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 611 - 641, The controller currently returns raw { error: ... } and appends connErr.message to the client, violating the API contract; update the error path in the updateExternalConfig flow (the block using isSafeUri, encrypt, mongoose.createConnection, getPublicIp) to NOT expose raw Mongo errors and to follow the shared contract: on failure create/throw an AppError or send res.status(400).json({ success: false, data: {}, message: "<sanitized message>" }) instead of { error: ... }; remove any direct inclusion of connErr.message in responses, only include a generic, user-friendly message (e.g. whitelist hint using getPublicIp when applicable) and use AppError where other controller errors are raised to keep behavior consistent across the controller.
566-576:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNormalize bracketed IPv6
hostnamebeforenet.isIPv6()
new URL(uri).hostnamereturns IPv6 literals with square brackets (e.g."[2001:db8::1]"), butnet.isIPv6()only returnstruefor the bracketless form ("2001:db8::1"). Strip[]before the IP checks so IPv6 literals follow the intended restriction logic.Suggested fix
- const host = parsed.hostname.toLowerCase(); + const host = parsed.hostname.replace(/^\[|\]$/g, "").toLowerCase();🤖 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 566 - 576, The hostname from new URL(uri) can be a bracketed IPv6 literal (e.g. "[2001:db8::1]") so update the logic that checks net.isIPv6/net.isIPv4 and calls isRestrictedIP to first normalize the hostname by stripping surrounding "[" and "]" if present (use the existing parsed and host variables), then use that unbracketedHost for net.isIPv6, net.isIPv4 and isRestrictedIP calls; ensure blockedHostnames check still uses the original lowercased host as before.
🤖 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 578-593: isSafeUri currently only resolves parsed.hostname via
dns.resolve4/resolve6 but misses SRV/TXT seedlist discovery used by
mongoose.createConnection for mongodb+srv URIs; update isSafeUri (or caller
updateExternalConfig) to detect mongodb+srv:// and perform dns SRV (and TXT if
needed) resolution for the SRV records, then resolve A/AAAA for each SRV-target
hostname and run isRestrictedIP against each resulting IP, rejecting if any are
restricted — alternatively explicitly reject mongodb+srv URIs in
updateExternalConfig before calling mongoose.createConnection; reference
isSafeUri, updateExternalConfig, and mongoose.createConnection when making the
change.
---
Outside diff comments:
In `@apps/dashboard-api/src/controllers/project.controller.js`:
- Around line 611-641: The controller currently returns raw { error: ... } and
appends connErr.message to the client, violating the API contract; update the
error path in the updateExternalConfig flow (the block using isSafeUri, encrypt,
mongoose.createConnection, getPublicIp) to NOT expose raw Mongo errors and to
follow the shared contract: on failure create/throw an AppError or send
res.status(400).json({ success: false, data: {}, message: "<sanitized message>"
}) instead of { error: ... }; remove any direct inclusion of connErr.message in
responses, only include a generic, user-friendly message (e.g. whitelist hint
using getPublicIp when applicable) and use AppError where other controller
errors are raised to keep behavior consistent across the controller.
- Around line 566-576: The hostname from new URL(uri) can be a bracketed IPv6
literal (e.g. "[2001:db8::1]") so update the logic that checks
net.isIPv6/net.isIPv4 and calls isRestrictedIP to first normalize the hostname
by stripping surrounding "[" and "]" if present (use the existing parsed and
host variables), then use that unbracketedHost for net.isIPv6, net.isIPv4 and
isRestrictedIP calls; ensure blockedHostnames check still uses the original
lowercased host as before.
🪄 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: 9ec89a2b-1fb4-4b35-a27b-1d54eb83781c
📒 Files selected for processing (1)
apps/dashboard-api/src/controllers/project.controller.js
…config
- Strip brackets from IPv6 literals before net.isIPv6() checks to properly validate restricted ranges
- Reject mongodb+srv:// URIs to prevent hidden SRV/TXT resolution bypass of SSRF checks
- Sanitize all error responses to follow API contract and prevent MongoDB error exposure
- Replace raw error payloads with consistent { success, data, message } format
Fixes CodeRabbit security findings on DNS resolution and error handling.
Signed-off-by: Anshul Jain <anshul23102@iiitd.ac.in>
CodeRabbit Security Fixes Applied✅ SRV Resolution Bypass (Line 593)Added explicit rejection of ✅ IPv6 Bracket Normalization (Line 566)Strip square brackets from parsed IPv6 literals before ✅ Error Contract Violation (Lines 611-641)Sanitize all error responses to follow standard API contract All fixes pushed to branch in commit 48e5ab1. |
|
Thank you for pointing out this issue @anshul23102 |
|
bro is ur github acc made by this mail?? If this is the only, then please add ur personal account as well. |
2 participants
Pull Request Description
Fixes #245
The previous
isSafeUriimplementation checked only four literal values (localhost,127.0.0.1,0.0.0.0,::1). Any RFC-1918 private address (e.g.10.0.0.1,172.20.0.1,192.168.1.100) or the cloud instance metadata endpoint (169.254.169.254) passed the check unchallenged, enabling SSRF: an attacker could point their external MongoDB URI at an internal service or the cloud metadata API and have the dashboard-api make outbound connections on their behalf.Root Cause
isSafeUricompared only against a hard-coded list of four specific strings. IP address ranges were not validated, so any numeric host that was not exactly one of those four values was treated as safe.Solution
ipv4ToInt()to convert a dotted-decimal IPv4 string to a 32-bit integer for range comparison.isRestrictedIPv4()that covers all blocked ranges:127.0.0.0/810.0.0.0/8,172.16.0.0/12,192.168.0.0/16169.254.0.0/16(covers169.254.169.254)0.0.0.0/8isSafeUrito route numeric IPv4 hosts throughisRestrictedIPv4, keep a hostname denylist forlocalhostandmetadata.google.internal, and add an explicit IPv6 loopback check via Node'snetmodule.Changes Made
apps/dashboard-api/src/controllers/project.controller.js
const net = require("net")import.ipv4ToInt(ip)helper function.isRestrictedIPv4(ip)function covering all restricted IPv4 ranges.isSafeUri(uri)to use range-based validation instead of exact-string matching.Type of Change
Testing & Validation
Backend Verification
mongodb://localhost/dbmongodb://127.0.0.1/dbmongodb://127.100.0.1/dbmongodb://10.0.0.1/dbmongodb://172.16.0.1/dbmongodb://172.31.255.255/dbmongodb://192.168.1.100/dbmongodb://169.254.169.254/dbmongodb://metadata.google.internal/dbmongodb+srv://cluster.mongodb.net/dbmongodb://203.0.113.5/dbScreenshots / Recordings
Not applicable. This is a server-side security fix with no visual change.
Checklist
GSSoC Label Request
Maintainer, could you please add the appropriate GSSoC label to this PR? This helps with contribution tracking and scoring under GSSoC '26. Thank you.
Built with for urBackend.
Summary by CodeRabbit