Add SSRF protection to read_file URL fetches (#587, #560) - #597
Conversation
…rwhy-er#560) read_file with isUrl passed the URL straight to fetch() with no validation, so it could read cloud instance metadata (169.254.169.254), reach internal services, or scan the private network. Add assertUrlIsFetchable(), run before any network access, which: - allows only http/https schemes, and - rejects hosts that are, or resolve to, non-public addresses — loopback, private, link-local (incl. the cloud metadata endpoint), CGNAT, multicast and reserved ranges, for both IPv4 and IPv6 (including IPv4-mapped). Redirects are now followed manually and re-validated at each hop, so a public URL cannot redirect onto an internal address, and the PDF path downloads the validated final URL. Adds test/test-read-url-ssrf.js covering the metadata endpoint, private and loopback IPs, IPv6, and non-http schemes; the checks short-circuit before any request, so the test is hermetic. Fixes wonderwhy-er#587 Fixes wonderwhy-er#560
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesURL SSRF protection
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant readFileFromUrl
participant fetchUrlValidated
participant assertUrlIsFetchable
participant DNS
participant parsePdfToMarkdown
readFileFromUrl->>fetchUrlValidated: fetch URL with abort signal
fetchUrlValidated->>assertUrlIsFetchable: validate initial and redirect URLs
assertUrlIsFetchable->>DNS: resolve hostname
DNS-->>assertUrlIsFetchable: return validated addresses
fetchUrlValidated-->>readFileFromUrl: response and finalUrl
readFileFromUrl->>parsePdfToMarkdown: pass fetched PDF bytes
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 4
🤖 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 `@src/tools/filesystem.ts`:
- Around line 470-474: Update the PDF handling in readFileFromUrl, including the
fetch loop and the parsePdfToMarkdown call, to read the PDF bytes from the
existing fetch response and pass those bytes to the parser instead of passing
currentUrl. Preserve the existing SSRF/DNS-validated response flow and ensure
both referenced sites in src/tools/filesystem.ts (470-474 and 502-507) use the
single fetched response.
- Around line 393-400: The IPv6 blocking logic in isBlockedAddress must use
CIDR-aware checks to prevent SSRF bypasses: replace the fe80 prefix check with
proper fe80::/10 validation and handle IPv4-mapped IPv6 addresses such as
::ffff:7f00:1, not only dotted-decimal forms. Add regression cases in
test/test-read-url-ssrf.js covering fe90::1 and ::ffff:7f00:1; both listed sites
require changes.
In `@test/test-read-url-ssrf.js`:
- Around line 17-28: Add the two alternate IPv6 regression URLs,
http://[fe90::1]/ and http://[::ffff:7f00:1]/, to the BLOCKED_URLS test fixture
so isBlockedAddress() is verified to reject these non-public destinations.
- Around line 40-56: Add a hermetic redirect test in the existing SSRF test
flow, using a local or mocked allowed URL that redirects to a metadata,
loopback, or private address. Exercise the relevant URL-reading/request function
rather than only assertUrlIsFetchable, and assert the redirect is rejected with
the existing non-public protection while verifying the internal target receives
no request.
🪄 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: ebd58d67-6846-40fa-8551-da802aea8362
📒 Files selected for processing (2)
src/tools/filesystem.tstest/test-read-url-ssrf.js
The hand-rolled IPv6 checks had two bypasses: startsWith('fe80') missed the
rest of the fe80::/10 link-local range (e.g. fe90::1), and the IPv4-mapped
check only matched the dotted-decimal form, so the hex form ::ffff:7f00:1
(127.0.0.1) slipped through as a public address.
Classify addresses with ipaddr.js instead: only the `unicast` range is treated
as publicly routable, and IPv4-mapped IPv6 is unwrapped to its IPv4 address
before the range check. Adds regression cases for fe90::1 and ::ffff:7f00:1.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
package.json (1)
110-110: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winVerify the
ipaddr.jsmajor version.
^1.9.1selects the legacy 1.x line, while the package currently publishes 2.4.0 and recommends 2.x for Node.js 10+. (npmjs.com) If this project targets Node.js 10 or newer, upgrade to^2.4.0and refresh the lockfile; otherwise document why the older major is required for this SSRF boundary.Proposed change
- "ipaddr.js": "^1.9.1", + "ipaddr.js": "^2.4.0",🤖 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 `@package.json` at line 110, Verify the project’s supported Node.js version and update the ipaddr.js dependency from ^1.9.1 to ^2.4.0 when Node.js 10+ is supported, then refresh the lockfile; if the project requires the legacy 1.x line, document that requirement instead.
🤖 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.
Nitpick comments:
In `@package.json`:
- Line 110: Verify the project’s supported Node.js version and update the
ipaddr.js dependency from ^1.9.1 to ^2.4.0 when Node.js 10+ is supported, then
refresh the lockfile; if the project requires the legacy 1.x line, document that
requirement instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ca909566-f782-4c9d-93e6-9dd5dabae2a9
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
package.jsonsrc/tools/filesystem.tstest/test-read-url-ssrf.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/tools/filesystem.ts
- test/test-read-url-ssrf.js
The PDF branch of readFileFromUrl passed the URL to parsePdfToMarkdown, which downloaded it again through an unguarded fetch in loadPdfToBuffer. That second request re-resolved DNS, so a host that passed the SSRF checks could answer the parser's lookup with an internal address. Hand the parser the bytes from the request that was already validated. loadPdfToBuffer now returns byte input as-is; the file and URL paths are unchanged for other callers.
|
Went back through the review findings against the current branch: PDF path (
IPv6 findings ( Test suite is 4/4. The new case builds a minimal one-page PDF inline and asserts it |
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 `@src/tools/pdf/markdown.ts`:
- Around line 266-268: Update parsePdfToMarkdown’s URL handling to prevent
direct fetches of arbitrary http(s) strings: reuse the existing SSRF-guarded URL
reader before obtaining the PDF bytes, or change the parser contract to accept
bytes only and move URL fetching to a guarded caller. Preserve local source
handling and the existing markdown conversion flow.
🪄 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: 66a09a8f-7a7e-48e7-81ca-4e3d80f4a502
📒 Files selected for processing (3)
src/tools/filesystem.tssrc/tools/pdf/markdown.tstest/test-read-url-ssrf.js
🚧 Files skipped from review as they are similar to previous changes (2)
- test/test-read-url-ssrf.js
- src/tools/filesystem.ts
Extract the guard and validated-redirect fetch into src/utils/urlSafety.ts and use it from both readFileFromUrl and loadPdfToBuffer, so the exported parser can no longer fetch URLs the guard would refuse.
|
Valid point — the exported parser was still a way around the guard for any caller passing a URL string. Extracted the guard and the validated-redirect fetch into src/utils/urlSafety.ts and routed loadPdfToBuffer's URL branch through it, so parsePdfToMarkdown now applies the same checks as read_file (assertUrlIsFetchable stays re-exported from filesystem.ts). Verified: all 4 SSRF regression tests pass, and parsePdfToMarkdown('http://169.254.169.254/x.pdf') is refused before any request is made. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/utils/urlSafety.ts (1)
96-115: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDiscarded redirect responses aren't drained/cancelled.
When a hop turns out to be a redirect, the previous
responseis overwritten without consuming or cancelling its body (response.body?.cancel()). Undercross-fetch/undici, an unconsumed body can keep the underlying socket open until GC, which is worth avoiding on a path that fetches attacker-influenced URLs across up to 5 redirect hops.♻️ Proposed fix
const location = response.headers.get('location'); if (!location) { break; // redirect status without a target — treat as final } if (hop >= MAX_URL_REDIRECTS) { throw new Error(`Too many redirects while fetching URL: ${url}`); } + await response.body?.cancel(); const nextUrl = new URL(location, currentUrl).toString();🤖 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 `@src/utils/urlSafety.ts` around lines 96 - 115, In the redirect-handling loop around the response variable, cancel the current response body before following each valid redirect so discarded redirect responses release their resources. Apply this after confirming the redirect target exists and before fetching or assigning the next URL, while preserving the existing redirect-limit and URL-validation behavior.
🤖 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 `@src/utils/urlSafety.ts`:
- Around line 40-118: The validated DNS result in assertUrlIsFetchable must be
reused by fetchUrlValidated instead of allowing fetch to resolve the hostname
again. Refactor the validation/fetch flow to retain the checked address and
configure the underlying connection path with a custom lookup or equivalent
remote-address verification for every initial and redirected request, while
preserving protocol and redirect validation.
---
Nitpick comments:
In `@src/utils/urlSafety.ts`:
- Around line 96-115: In the redirect-handling loop around the response
variable, cancel the current response body before following each valid redirect
so discarded redirect responses release their resources. Apply this after
confirming the redirect target exists and before fetching or assigning the next
URL, while preserving the existing redirect-limit and URL-validation behavior.
🪄 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: fb2ae4ee-14a5-4953-9fdc-3647726b3cbd
📒 Files selected for processing (3)
src/tools/filesystem.tssrc/tools/pdf/markdown.tssrc/utils/urlSafety.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/tools/pdf/markdown.ts
assertUrlIsFetchable() checked dns.lookup() results, but the fetch that followed re-resolved the hostname when opening the socket, so a rebinding DNS server could pass validation and then hand the connection an internal address. The guard now returns the validated addresses and fetchUrlValidated() connects through an agent whose lookup answers only from that set, for the initial request and every redirect hop. TLS SNI and certificate checks still run against the hostname. Also releases the unread body of each discarded redirect response, and adds hermetic tests for redirect-to-internal blocking and lookup pinning (fetch implementation is now injectable for tests); both were observed failing against a mutated guard before the fix.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/test-read-url-ssrf.js (1)
111-123: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider also exercising the
options.alllookup branch.This only calls
agent.options.lookup('rebind.example', {}, ...), which exercises the single-result branch ofpinnedAgentFor's custom lookup. Node's Happy-EyeballsautoSelectFamily(on by default) invokes custom lookups with{ all: true }, so that branch is the one actually reachable in production dual-stack connections; add a call with{ all: true }and assert it returns the full pinned address array.🤖 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 `@test/test-read-url-ssrf.js` around lines 111 - 123, The SSRF pinning test should also cover the `{ all: true }` branch of the custom lookup configured by `pinnedAgentFor`. Add a lookup invocation using `{ all: true }`, assert it succeeds with the complete pinned address array (including the validated address and family), and retain the existing single-result assertion.
🤖 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.
Nitpick comments:
In `@test/test-read-url-ssrf.js`:
- Around line 111-123: The SSRF pinning test should also cover the `{ all: true
}` branch of the custom lookup configured by `pinnedAgentFor`. Add a lookup
invocation using `{ all: true }`, assert it succeeds with the complete pinned
address array (including the validated address and family), and retain the
existing single-result assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b69341b1-021b-4c69-8fef-9edf5e90a5e6
📒 Files selected for processing (2)
src/utils/urlSafety.tstest/test-read-url-ssrf.js
|
Hi 👋 Gentle nudge on this PR — it's mergeable and the required checks are passing (one non-required/optional check shows as unstable, which shouldn't block merge). Would appreciate a review whenever you have a moment, and I'm happy to address anything needed. Thanks for your time and for maintaining this project! 🙏 |
1 participant
Summary
read_filewithisUrl: truerouted straight toreadFileFromUrl(), which passed the URL tofetch()with no validation (#587, #560). That allowed reading cloud instance metadata, reaching internal services, and scanning the private network from the host running the MCP server:{ "name": "read_file", "arguments": { "path": "http://169.254.169.254/latest/meta-data/iam/security-credentials/", "isUrl": true } }Fix
A new
assertUrlIsFetchable()runs before any network access and:http:/https:schemes (sofile://,ftp://, etc. are rejected);169.254.169.254metadata endpoint), CGNAT, multicast and reserved ranges, covering IPv4, IPv6, and IPv4-mapped IPv6.To close the redirect bypass (a public URL redirecting onto an internal address), redirects are now followed manually and re-validated at every hop, up to a small cap. The PDF branch downloads the validated final URL rather than the original.
Testing
test/test-read-url-ssrf.js(new) asserts the guard rejects the cloud metadata endpoint, loopback, private and IPv6 addresses,localhost, and non-http schemes, and that a public address passes. The blocked cases short-circuit before any request is made, so the test is hermetic (no outbound traffic).npm run buildpasses; existing local file tests still pass.Notes / scope
Fixes #587
Fixes #560
Summary by CodeRabbit