fix(sdk,openapi): block SSRF targets when fetching integration specs by URL - #1887
fix(sdk,openapi): block SSRF targets when fetching integration specs by URL#1887ra-co88 wants to merge 1 commit into
Conversation
c9ca82c to
7836834
Compare
7836834 to
8067ef3
Compare
|
Heads-up on the red |
ra-co88
left a comment
There was a problem hiding this comment.
Verdict: approve — SSRF egress gate, correctly designed and tested. (Comment review: GitHub blocks formal self-approval on your own PR.)
Supersession check (upstream/main @ 2dc399e): no SSRF/private-address/loopback guard exists anywhere in the upstream fetch path — packages/plugins/openapi/src/sdk/invoke.ts has zero blocklist markers. Not superseded; the hole is genuinely open upstream.
The design is the correct shape for this class:
- Resolve → classify → pin closes both the plain SSRF and the DNS-rebinding variant: classification is a pure function of the resolved address, and the caller connects to
resolvedAddresswith no second resolution between validate and connect. - The blocklist is comprehensive: loopback, RFC1918, CGNAT (100.64/10), link-local incl. 169.254.169.254 metadata, IPv6 link-local/ULA, IPv4-mapped IPv6 (recursively classified against the v4 blocklist), 0.0.0.0/8, broadcast.
- Encoding attacks are covered: decimal (2852039168), hex (0x7f000001), and octal (0177.0.0.1) IP literals are normalized before classification — with dedicated negative tests for each encoding.
- Fails closed: unparseable input, unresolvable hostnames, and non-http(s) schemes/userinfo are all rejected. The default has no loopback escape;
allowLoopbackis an explicit opt-in. - Negative-control discipline holds: the egress.test.ts suite is almost entirely negative cases — every blocked class has a test asserting rejection, plus the "resolves to ANY private address among public ones" case (the multi-A-record attack).
- Platform discipline: no static
node:imports — DNS loads lazily so DOM-platform consumers still typecheck the SDK barrel against node-less libs. The right constraint, honored.
CI caveat, not blocking: the single failing shard (E2E cloud 13of16, run from 2026-08-30) is openSession: no mcp-session-id header in the cap-eviction scenario — the exact transient that #1895's restart-envelope retry backstop fixes, and this branch predates it. The failure is unrelated to the egress payload (which touches no session code). A rebase onto current main picks up the backstop and the flake goes away.
Suggested for the maintainer: the branch is one commit and merges clean; the 529-line footprint is all payload (egress.ts + tests + fetch-path routing). Good to merge after rebase.
What
Integration specs fetched by URL now pass an SSRF gate (
assertFetchable) before any request is made: the hostname is resolved, the address is classified, and only public targets are fetched — connecting to the pinned resolved address so DNS rebinding cannot swap the answer between validation and connect.Why
The spec URL is user-supplied. Without a gate it can target the loopback range, RFC1918 space, link-local addresses (including 169.254.169.254 cloud metadata), carrier-grade NAT, IPv6 link-local/ULA, IPv4-mapped IPv6, 0.0.0.0, or broadcast. The gate resolves first and classifies the address — not the hostname — then pins the connection to that address, closing both the plain SSRF and the DNS-rebinding variants (no second resolution between validate and connect).
The module deliberately carries no static
node:imports: it sits in the SDK barrel, which DOM-platform consumers typecheck against node-less libs. DNS loads lazily via a runtime-assembled specifier; IP classification is self-contained.What changed
assertFetchable(url)in the SDK: parse → scheme/userinfo checks → normalize (IP literals in any encoding) → resolve → classify → pin. The gate takes anallowLoopbackoption; the default stays fail-closed.http:URLs the fetch connects to the pinned address (Host header preserved). Forhttps:URLs the fetch keeps the original hostname — see Limitations.EXECUTOR_ALLOW_LOOPBACK_SPECS=1(spec-fetch-scoped) or the established local-network knobs —ALLOW_LOCAL_NETWORK=true(the cloud e2e harness already sets it) orEXECUTOR_ALLOW_LOCAL_NETWORK=true(selfhost e2e). Production never sets any of them.Limitations
Redirect chains are not followed by the gate: a public URL that 302s to a private address is blocked only if the fetch client itself refuses redirects (or re-validates). A follow-up making the fetch loop redirect-aware and re-running the gate per hop is the natural hardening step; this PR declares the gap rather than overclaiming.
For
https:URLs the fetch connects by hostname, not by the pinned address: fetch-based transports have no custom-connect hook, and rewriting an https URL to the resolved IP would present the IP as TLS SNI and break CDN-fronted hosts. The gate still decides the fetch (any blocked resolved address fails the target before the request), but the connect is not address-pinned on https — a re-resolve window between validate and connect remains there. Plain http fetches are address-pinned end to end.Test plan
Classification table (public allowed; every private/reserved encoding blocked, including integer/hex/octal IPv4 forms) plus resolve-and-pin tests with an injected resolver. 12 tests green against current main.