Add ssrf guard to storage URL fetch fallback#3896
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR adds SSRF protection to the URL-to-Blob utility by validating resolved hostnames do not map to private, loopback, link-local, or otherwise internal IPv4/IPv6 addresses before performing fetches; fetching uses ChangesSSRF Prevention for URL-to-Blob Conversion
sequenceDiagram
participant Client
participant urlToBlob
participant assertSafeUrl
participant dns_lookup as dns.promises.lookup
participant FetchAPI as fetch
Client->>urlToBlob: request URL -> Blob
urlToBlob->>assertSafeUrl: assertSafeUrl(url)
assertSafeUrl->>dns_lookup: lookup(hostname) (if needed)
dns_lookup-->>assertSafeUrl: resolved IP
assertSafeUrl-->>urlToBlob: OK / throw on private/internal
urlToBlob->>FetchAPI: fetch(url, { redirect: "error" }) (if OK)
FetchAPI-->>urlToBlob: response Blob
urlToBlob-->>Client: return Blob or throw error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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 docstrings
🧪 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/lib/storage.ts (1)
218-267:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftThe SSRF guard is bypassable via HTTP redirects and DNS rebinding because
fetch()performs independent resolution and follows redirects.A public URL can pass
assertSafeUrl()and then be redirected (via 3xx) to169.254.169.254or RFC1918 space, since Node.js fetch follows HTTP redirects by default. Additionally, a priordns.lookup()does not constrain the address thatfetch()ultimately connects to—fetch()performs its own independent DNS resolution at connection time, allowing an attacker to exploit DNS rebinding or timing-based resolution changes. The security check must enforce policy at the actual connection/dispatch layer using a custom DNS resolver or connection interceptor, not as a one-off preflight lookup.🤖 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/web/lib/storage.ts` around lines 218 - 267, The current SSRF guard in assertSafeUrl() is bypassable because fetch() does its own DNS resolution and follows redirects; modify urlToBlob() to (1) perform fetches with manual redirect handling (no automatic redirects) and validate every Location header/redirect target by calling assertSafeUrl() before following it, and (2) supply fetchWithTimeout (or fetch) with a custom HTTP(S) Agent that implements a restrictive lookup/resolver (or rejects non-whitelisted/resolved private addresses) so the actual TCP connection uses your allowed-IP policy; update references in urlToBlob(), fetchWithTimeout, and assertSafeUrl() to integrate the agent and redirect validation logic.
🤖 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.
Outside diff comments:
In `@apps/web/lib/storage.ts`:
- Around line 218-267: The current SSRF guard in assertSafeUrl() is bypassable
because fetch() does its own DNS resolution and follows redirects; modify
urlToBlob() to (1) perform fetches with manual redirect handling (no automatic
redirects) and validate every Location header/redirect target by calling
assertSafeUrl() before following it, and (2) supply fetchWithTimeout (or fetch)
with a custom HTTP(S) Agent that implements a restrictive lookup/resolver (or
rejects non-whitelisted/resolved private addresses) so the actual TCP connection
uses your allowed-IP policy; update references in urlToBlob(), fetchWithTimeout,
and assertSafeUrl() to integrate the agent and redirect validation logic.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?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/web/lib/storage.ts`:
- Around line 225-226: The DNS validation currently discards the validated IP
(address) and then calls fetch(url) with the original hostname, allowing a
TOCTOU DNS rebind; fix by pinning the validated IP to the outbound connection:
either construct an undici Agent/Dispatcher with a custom lookup that returns
the previously-validated address and pass it to fetch (via the dispatcher
option) so the connection uses the validated IP, or rewrite the fetch target to
use the validated IP (replace hostname in the URL) and set the original hostname
in the Host header so TLS/virtual-hosting still works; apply this change where
hostname and address are resolved and where fetch(url) is invoked so the same
validated address is used for the actual connection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
2 participants
Summary by CodeRabbit