Extra ssrf guards#3953
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new SSRF-safe fetch utility with private-IP and DNS validation, refactors isIframeable to use caller-provided headers, updates routes and sitemap importer to use safeFetch, and adds/adjusts tests and test wiring for the new behavior. ChangesSSRF-safe fetching implementation and API route adoption
Sequence DiagramsequenceDiagram
participant APIRoute as API Route Handler
participant SafeFetch as safeFetch
participant URLValidator as assertUrlIsSafe
participant DNSResolver as Google DoH
participant Fetch as fetch()
APIRoute->>SafeFetch: safeFetch(url, init, opts)
SafeFetch->>URLValidator: Validate initial URL
URLValidator->>DNSResolver: Resolve A/AAAA
DNSResolver-->>URLValidator: IP addresses
URLValidator->>URLValidator: Check isPrivateIp
URLValidator-->>SafeFetch: Safe or throw DubApiError
SafeFetch->>Fetch: fetch with redirect:manual
Fetch-->>SafeFetch: Response (may redirect)
SafeFetch->>URLValidator: Validate redirect URL
URLValidator-->>SafeFetch: Safe or throw
SafeFetch-->>APIRoute: Final Response or Error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/web/lib/api/safe-fetch.ts (1)
137-151: 💤 Low valueIPv4-compatible addresses (
::/96) not handled.The IPv4-mapped check correctly catches
::ffff:x.x.x.x, but IPv4-compatible addresses like::127.0.0.1(deprecated format where upper 96 bits are zero) would slip through—they're not matched by the IPv4-mapped check and don't fall into any IPv6 CIDR.While IPv4-compatible addresses are deprecated (RFC 4291, 2006) and rarely routed by modern stacks, for a security feature, fail-closed coverage is preferred.
🛡️ Optional: Add IPv4-compatible handling
// IPv4-mapped IPv6 (::ffff:0:0/96): unwrap the last 32 bits and re-check. if (ipBig >> BIG_32 === BIG_FFFF) { const v4Int = Number(ipBig & BIG_FFFFFFFF); const v4Str = `${(v4Int >>> 24) & 0xff}.${(v4Int >>> 16) & 0xff}.${ (v4Int >>> 8) & 0xff }.${v4Int & 0xff}`; return isPrivateIpv4(v4Str); } + // IPv4-compatible (deprecated ::/96): upper 96 bits are zero, last 32 are IPv4. + // Exclude :: itself (already caught by ::/128) to avoid false positives. + if (ipBig >> BIG_32 === BIG_ZERO && ipBig !== BIG_ZERO) { + const v4Int = Number(ipBig & BIG_FFFFFFFF); + const v4Str = `${(v4Int >>> 24) & 0xff}.${(v4Int >>> 16) & 0xff}.${ + (v4Int >>> 8) & 0xff + }.${v4Int & 0xff}`; + return isPrivateIpv4(v4Str); + } + return PRIVATE_IPV6_CIDRS.some((c) => ipv6InCidr(ipBig, c));🤖 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/api/safe-fetch.ts` around lines 137 - 151, The function isPrivateIpv6 misses IPv4-compatible addresses (::/96) like ::127.0.0.1; update isPrivateIpv6 (after computing ipBig and the existing mapped-address block that checks ipBig >> BIG_32 === BIG_FFFF) to also detect the IPv4-compatible case where the upper 96 bits are zero (ipBig >> BIG_32 === 0n) and then unwrap the low 32 bits (ipBig & BIG_FFFFFFFF) into an IPv4 dotted string and call isPrivateIpv4 on it; keep the existing mapped-address logic intact and only add this additional branch before falling back to PRIVATE_IPV6_CIDRS/ipv6InCidr.
🤖 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/app/api/providers/route.ts`:
- Around line 45-53: The request incorrectly places method: "HEAD" inside the
headers object when calling safeFetch (so fetch will ignore it); modify the
safeFetch invocation that builds `headers` from `urlObject.toString()` so that
the RequestInit passed to safeFetch includes method: "HEAD" at the top level
(e.g., alongside headers) rather than inside the headers map; update the call
site where `safeFetch(...)` is invoked and the RequestInit argument so `method`
is a top-level property and only actual header fields remain in the headers
object.
In `@packages/utils/src/functions/is-iframeable.ts`:
- Around line 27-29: The check for X-Frame-Options in is-iframeable currently
does a strict equality against "DENY" and "SAMEORIGIN", which misses lowercase
or whitespace-padded values; update the logic around the xFrameOptions value
(from headers.get("X-Frame-Options")) to normalize it (e.g., trim() and
toUpperCase() or toLowerCase()) before comparing, and then return false when the
normalized value equals DENY or SAMEORIGIN so the iframeability check handles
case and padding variations.
---
Nitpick comments:
In `@apps/web/lib/api/safe-fetch.ts`:
- Around line 137-151: The function isPrivateIpv6 misses IPv4-compatible
addresses (::/96) like ::127.0.0.1; update isPrivateIpv6 (after computing ipBig
and the existing mapped-address block that checks ipBig >> BIG_32 === BIG_FFFF)
to also detect the IPv4-compatible case where the upper 96 bits are zero (ipBig
>> BIG_32 === 0n) and then unwrap the low 32 bits (ipBig & BIG_FFFFFFFF) into an
IPv4 dotted string and call isPrivateIpv4 on it; keep the existing
mapped-address logic intact and only add this additional branch before falling
back to PRIVATE_IPV6_CIDRS/ipv6InCidr.
🪄 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: d3b583fd-5d7a-4d49-b1e2-a6143edb9e0a
📒 Files selected for processing (6)
apps/web/app/api/links/iframeable/route.tsapps/web/app/api/links/metatags/utils.tsapps/web/app/api/providers/route.tsapps/web/lib/api/safe-fetch.tsapps/web/tests/misc/safe-fetch.test.tspackages/utils/src/functions/is-iframeable.ts
|
@coderabbitai full review. any breaking changes? any security issues? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== safe-fetch.ts ==="
cat apps/web/lib/api/safe-fetch.ts
echo ""
echo "=== iframeable route ==="
cat apps/web/app/api/links/iframeable/route.ts
echo ""
echo "=== metatags utils ==="
cat apps/web/app/api/links/metatags/utils.ts
echo ""
echo "=== providers route ==="
cat apps/web/app/api/providers/route.ts
echo ""
echo "=== is-iframeable.ts ==="
cat packages/utils/src/functions/is-iframeable.ts
echo ""
echo "=== safe-fetch tests ==="
cat apps/web/tests/misc/safe-fetch.test.ts
echo ""
echo "=== Any other consumers of isIframeable ==="
rg -n "isIframeable" --type ts -C 2
echo ""
echo "=== Any other consumers of fetchWithTimeout in the changed paths ==="
rg -n "fetchWithTimeout" --type ts -C 2
echo ""
echo "=== Checking for other callers of the old isIframeable signature (url:) ==="
rg -n "isIframeable.*url" --type ts -C 3Length of output: 26632
Breaking Changes1.
|
| Before | After |
|---|---|
async ({ url, requestDomain }: { url: string; requestDomain: string }) |
({ headers, requestDomain }: { headers: Headers; requestDomain: string }) |
This is a breaking change for any external consumer of @dub/utils that calls isIframeable. Internally, only apps/web/app/api/links/iframeable/route.ts uses it, and it has been correctly updated. However, if this package is published to npm, downstream users will break on upgrade.
The function is also no longer async — awaiting it still works (awaiting a non-Promise returns the value), so that part is backward-compatible at the call-site level.
Security Issues
1. ⚠️ DNS Rebinding / TOCTOU Race (Medium–High)
In safe-fetch.ts, assertUrlIsSafe resolves the hostname via Google DoH and checks the returned IPs. Then, separately, the actual TCP connection is made using the OS/runtime resolver. There is a time gap between these two operations:
sequenceDiagram
participant Attacker as Attacker DNS
participant safeFetch
participant Target as Internal Service
safeFetch->>Attacker: DoH resolve → returns 1.2.3.4 (public) ✅
Note over Attacker: Attacker flips DNS TTL to 0 and points to 169.254.169.254
safeFetch->>Target: TCP connect → OS resolves → 169.254.169.254 ❌
An attacker with control over DNS can pass the DoH check (by returning a public IP) and then serve the private IP to the runtime's actual connection. This is a known limitation of DoH-based SSRF guards. The only robust mitigation is binding to a known-good IP after DNS resolution (i.e., connecting directly to the resolved IP, bypassing the OS resolver), which is not straightforward in fetch-based environments.
This is a hard problem in edge runtimes where you can't control the resolver. Worth documenting as a known limitation.
2. providers/route.ts — Leftover fetchWithTimeout for DNS Lookup
// apps/web/app/api/providers/route.ts, line 5 & 21
import { fetchWithTimeout } from "`@dub/utils`";
...
const dns = await fetchWithTimeout(
`https://dns.google/resolve?name=${domain}`,
)fetchWithTimeout (non-SSRF-safe) is still being used here. The target URL is hardcoded to dns.google, and domain is only appended as a query parameter (not the host), so there is no SSRF risk in this specific call. However, it's an inconsistency — the DNS lookup for the provider check continues to use the old unsafe fetcher. Consider using safeFetch here as well for consistency, or at minimum add a comment explaining why fetchWithTimeout is safe in this context.
Other Observations
3. No Response Body Size Limit in getHtml (Pre-existing)
metatags/utils.ts reads the full response body with response.text() — no size cap. A server could return a gigantic response and exhaust memory. This predates this PR, but now that safeFetch is in place, adding a size limit (e.g., abort if Content-Length exceeds a threshold) would be a good follow-up.
4. Test Coverage Gap
safe-fetch.test.ts only tests isPrivateIp. The higher-level safeFetch function (redirect following, DoH resolution, timeout, error paths) has no test coverage. Some important cases:
- Redirect to a private IP should be blocked
- More than
maxRedirectshops should throw - Protocol downgrade redirect (https → http) is currently allowed — intentional?
5. IPv4-mapped IPv6 Detection — Correct ✅
The check ipBig >> BIG_32 === BIG_FFFF correctly identifies ::ffff:x.x.x.x addresses and unwraps them for IPv4 private-range checking. The math holds.
6. isIframeable CSP Parsing (Pre-existing Limitation)
The frame-ancestors check compares bare domains (allowedOrigins.includes(requestDomain)) but some servers emit full origin values like https://dub.co. This is pre-existing and not introduced by this PR.
Summary
| Category | Item | Severity |
|---|---|---|
| Breaking | isIframeable signature + async removal |
Medium (affects external consumers of @dub/utils) |
| Security | DNS rebinding / TOCTOU in assertUrlIsSafe |
Medium (hard to fully mitigate in edge runtimes) |
| Code quality | fetchWithTimeout still used in providers/route.ts DNS call |
Low |
| Pre-existing | No body size limit in getHtml |
Low |
| Testing | safeFetch redirect/DNS paths untested |
Low |
Overall, the SSRF protections are a meaningful improvement. The main architectural concern is the TOCTOU race inherent to the DoH-check-then-connect pattern — worth documenting as a known limitation if it can't be fully mitigated in the edge runtime.
There was a problem hiding this comment.
Actionable comments posted: 0
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/sitemaps/import-tracked-sitemaps.ts (1)
75-87:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTreat non-2xx sitemap responses as errors before parsing.
At Line 75, the code parses the body without checking
response.ok. A404/500can be treated as a successful crawl path, which may incorrectly update crawl metadata.Suggested patch
async function fetchAndParseSitemap( sitemapUrl: string, ): Promise<SitemapXmlResult> { const response = await safeFetch(sitemapUrl, undefined, { maxRedirects: 0 }); + if (!response.ok) { + throw new Error( + `Failed to fetch sitemap: ${response.status} ${response.statusText}`, + ); + } const MAX_SITEMAP_BYTES = 10 * 1024 * 1024; // 10 MB const contentLength = response.headers.get("content-length");🤖 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/sitemaps/import-tracked-sitemaps.ts` around lines 75 - 87, The code reads and parses the sitemap body regardless of HTTP status; update the handler in import-tracked-sitemaps (the block using safeFetch and response) to check response.ok (or response.status) immediately after the fetch and throw a descriptive error for non-2xx responses (include status and statusText) before inspecting headers or calling response.arrayBuffer(), then proceed with the existing content-length/byteLength checks, decompressIfGzip, and parser.parse as before.
🧹 Nitpick comments (1)
apps/web/app/api/domains/[domain]/validate/route.ts (1)
61-65: ⚡ Quick winDisable redirects for the domain
HEADprobe.At Line 61, this check only needs to confirm origin responsiveness. Allowing default redirect hops adds avoidable outbound requests and latency.
Suggested patch
const response = await safeFetch( url, { method: "HEAD" }, - { timeoutMs: 3000 }, + { timeoutMs: 3000, maxRedirects: 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 `@apps/web/app/api/domains/`[domain]/validate/route.ts around lines 61 - 65, The HEAD probe currently follows redirects via safeFetch(url, { method: "HEAD" }, { timeoutMs: 3000 }), causing extra hops and latency; update the RequestInit passed to safeFetch so redirects are disabled (e.g., set redirect:"manual" on the second argument) so the probe only checks the origin responsiveness for the url variable and use the same timeout parameter.
🤖 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/sitemaps/import-tracked-sitemaps.ts`:
- Around line 75-87: The code reads and parses the sitemap body regardless of
HTTP status; update the handler in import-tracked-sitemaps (the block using
safeFetch and response) to check response.ok (or response.status) immediately
after the fetch and throw a descriptive error for non-2xx responses (include
status and statusText) before inspecting headers or calling
response.arrayBuffer(), then proceed with the existing content-length/byteLength
checks, decompressIfGzip, and parser.parse as before.
---
Nitpick comments:
In `@apps/web/app/api/domains/`[domain]/validate/route.ts:
- Around line 61-65: The HEAD probe currently follows redirects via
safeFetch(url, { method: "HEAD" }, { timeoutMs: 3000 }), causing extra hops and
latency; update the RequestInit passed to safeFetch so redirects are disabled
(e.g., set redirect:"manual" on the second argument) so the probe only checks
the origin responsiveness for the url variable and use the same timeout
parameter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d3c5017d-0ccb-46d3-8d3b-af49b019dd5a
📒 Files selected for processing (4)
apps/web/app/api/domains/[domain]/validate/route.tsapps/web/lib/sitemaps/import-tracked-sitemaps.tsapps/web/tests/misc/import-tracked-sitemaps.test.tsapps/web/tests/setupTests.ts
✅ Files skipped from review due to trivial changes (1)
- apps/web/tests/setupTests.ts
|
Actionable comments posted: 0 |
1 participant
Summary by CodeRabbit
Bug Fixes
Refactor
Tests