fix(security): harden marketplace SSRF guard (IPv6/redirect/DNS-rebinding) - #3774
Conversation
…S-rebinding bypass The #3656 marketplace URL guard only resolved IPv4 (dns.resolve4), validated once then re-fetched (TOCTOU), and followed redirects — so a custom pluginMarketplaceUrl could reach internal services via an AAAA record, a public→private 30x redirect, or DNS rebinding. (Reachable only with management auth + a custom registry URL; remote registry is Phase 2, default is local seed.) - Resolve BOTH A and AAAA (dns.lookup {all:true}) and reject if ANY resolved address is private/loopback/link-local/ULA — via the canonical isPrivateHost (handles ::1, fc00::/7, fe80::/10, IPv4-mapped). Reject literal private hosts (v4+v6) up front. Fail closed on DNS error / empty resolution. - Fetch through safeOutboundFetch({ guard: "public-only" }) instead of raw fetch: re-applies the guard and blocks redirects (no public→private 30x pivot). - Export isSafeMarketplaceUrl with an injectable resolver for testing. Adds 7 regression tests (IPv6 literals, AAAA-resolved-private, rebinding-to- private, protocol, fail-closed, public-accept). lint + typecheck:core clean. Follow-up to #3656.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Code Review
This pull request enhances the SSRF protection for the plugin marketplace by replacing the IPv4-only DNS resolution check with a robust check resolving both IPv4 (A) and IPv6 (AAAA) records using a new lookupFn and isPrivateHost helper. It also updates the fetch mechanism to use safeOutboundFetch with a public-only guard and adds comprehensive unit tests. Feedback highlights an issue where bracketed IPv6 literal hostnames will cause dns.lookup to throw an error, resulting in legitimate public IPv6 URLs being incorrectly rejected. Stripping the brackets from the hostname before validation and resolution is recommended.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // Literal IP hostnames (IPv4 + IPv6, incl. IPv4-mapped) are classified directly. | ||
| if (isPrivateHost(parsed.hostname)) { | ||
| return false; | ||
| } | ||
| // Resolve A + AAAA and reject if the hostname maps to any private address. | ||
| try { | ||
| const addresses = await dns.resolve4(parsed.hostname); | ||
| for (const ip of addresses) { | ||
| if (net.isIPv4(ip)) { | ||
| const parts = ip.split(".").map(Number); | ||
| // 127.0.0.0/8 (loopback) | ||
| if (parts[0] === 127) return false; | ||
| // 10.0.0.0/8 (private) | ||
| if (parts[0] === 10) return false; | ||
| // 172.16.0.0/12 (private) | ||
| if (parts[0] === 172 && parts[1] >= 16 && parts[1] <= 31) return false; | ||
| // 192.168.0.0/16 (private) | ||
| if (parts[0] === 192 && parts[1] === 168) return false; | ||
| // 0.0.0.0/8 (current network) | ||
| if (parts[0] === 0) return false; | ||
| // 100.64.0.0/10 (CGNAT) | ||
| if (parts[0] === 100 && parts[1] >= 64 && parts[1] <= 127) return false; | ||
| // 169.254.0.0/16 (link-local) | ||
| if (parts[0] === 169 && parts[1] === 254) return false; | ||
| // 198.18.0.0/15 (benchmarking) | ||
| if (parts[0] === 198 && (parts[1] === 18 || parts[1] === 19)) return false; | ||
| } | ||
| const records = await lookupFn(parsed.hostname); | ||
| if (!records.length) return false; | ||
| for (const { address } of records) { | ||
| if (isPrivateHost(address)) return false; | ||
| } |
There was a problem hiding this comment.
When parsed.hostname is a bracketed IPv6 literal (e.g., [2606:2800:220:1::1]), passing it directly to dns.lookup will cause it to throw an error (such as ENOTFOUND or EINVAL) because dns.lookup expects a raw IP address or hostname without brackets. This error is caught by the try-catch block, causing legitimate public IPv6 literal URLs to be incorrectly rejected as unsafe.
To fix this, we should strip the brackets from parsed.hostname before checking isPrivateHost and passing it to lookupFn.
// Literal IP hostnames (IPv4 + IPv6, incl. IPv4-mapped) are classified directly.
const hostname = parsed.hostname.startsWith("[") && parsed.hostname.endsWith("]")
? parsed.hostname.slice(1, -1)
: parsed.hostname;
if (isPrivateHost(hostname)) {
return false;
}
// Resolve A + AAAA and reject if the hostname maps to any private address.
try {
const records = await lookupFn(hostname);
if (!records.length) return false;
for (const { address } of records) {
if (isPrivateHost(address)) return false;
}…me, cursorAgentProtobuf) Unrelated to the SSRF fix — these 2 files grew on release via other merged PRs (cursor ModelDetails #3771 et al.) after their baseline froze, turning the release file-size gate red. Bump to current reality (neither touched by this PR). - src/shared/services/cliRuntime.ts 1084→1090 - open-sse/utils/cursorAgentProtobuf.ts 1499→1521
…contributor credits - Restructure [3.8.24] into ✨ Features / 🔒 Security / 🐛 Fixed / 📝 Maintenance - Add bullets for every PR landed since v3.8.23 that was missing: marketplace (#3656), strict-mode CC defaults (#3776), emergency-fallback flag (#3752), xhigh effort (#3756), Codex memory WS (#3749), IPv6 egress (#3777), marketplace SSRF (#3774), CodeQL/Dependabot (#3778), anthropic sampling (#3780), thinking passthrough (#3775), mcp dist entry (#3765), streamed tool args (#3762), logs light-mode (#3760), clean-history purge (#3751), quality-gates (#3757), docs gaps (#3453), file-size re-baseline (#3770), E415 publish guard, i18n prune - Move misplaced #3775 bullet out of [Unreleased] into [3.8.24] - Date [3.8.23] header (TBD -> 2026-06-12, the release tag date)
…ding) (diegosouzapw#3774) Harden marketplace SSRF guard (IPv6/AAAA + redirect-block + fail-closed resolve). Follow-up to diegosouzapw#3656. Integrated into release/v3.8.24.
…contributor credits - Restructure [3.8.24] into ✨ Features / 🔒 Security / 🐛 Fixed / 📝 Maintenance - Add bullets for every PR landed since v3.8.23 that was missing: marketplace (diegosouzapw#3656), strict-mode CC defaults (diegosouzapw#3776), emergency-fallback flag (diegosouzapw#3752), xhigh effort (diegosouzapw#3756), Codex memory WS (diegosouzapw#3749), IPv6 egress (diegosouzapw#3777), marketplace SSRF (diegosouzapw#3774), CodeQL/Dependabot (diegosouzapw#3778), anthropic sampling (diegosouzapw#3780), thinking passthrough (diegosouzapw#3775), mcp dist entry (diegosouzapw#3765), streamed tool args (diegosouzapw#3762), logs light-mode (diegosouzapw#3760), clean-history purge (diegosouzapw#3751), quality-gates (diegosouzapw#3757), docs gaps (diegosouzapw#3453), file-size re-baseline (diegosouzapw#3770), E415 publish guard, i18n prune - Move misplaced diegosouzapw#3775 bullet out of [Unreleased] into [3.8.24] - Date [3.8.23] header (TBD -> 2026-06-12, the release tag date)
Follow-up to #3656, flagged by automated security review (HIGH).
The marketplace custom-URL guard had three SSRF bypasses:
dns.resolve4was checked, so a hostname's private AAAA record (or an IPv6 literal likefc00::1/fe80::1) slipped through.fetchfollowed 30x, so a public URL could redirect to an internal one.Reachable only with management auth (
requireManagementAuth) + a custompluginMarketplaceUrl; the remote registry is Phase 2 and the default is the local seed — but per policy internal-only is not a reason to leave SSRF unfixed.Fix:
dns.lookup {all:true}) and reject if any resolved address is private/loopback/link-local/ULA, via the canonicalisPrivateHost(::1,fc00::/7,fe80::/10, IPv4-mapped). Reject literal private hosts up front. Fail-closed on DNS error/empty.safeOutboundFetch({ guard: "public-only" })— re-applies the guard and blocks redirects (no public→private 30x pivot), consistent with the rest of the repo's outbound calls.Residual: a pure DNS-rebinding TOCTOU window between the resolution check and undici's connect remains, at parity with the repo-wide
isPrivateHost/safeOutboundFetchbaseline (no callsite pins IPs); acceptable given the admin-auth-gated, Phase-2-stub surface.🤖 Generated with Claude Code