fix(security): parse IPs into CIDR ranges instead of matching hostname prefixes (#31) #60

Merged

laoc merged 2 commits from fix/issue-31-ssrf-ip-ranges into dev 2026-07-30 11:26:31 +00:00

Conversation 0 Commits 2 Files changed 7 +624 -56

laoc commented 2026-07-30 10:55:17 +00:00

Owner

Copy link

Closes #31.

isPrivateIp() guarded every server-side fetch path with hostname string prefixes. A verbatim replay of the two old implementations against this PR's fixtures lets 16 of 16 must-block addresses reach fetch, and wrongly blocks 3 of 4 public ones:

--- MUST BLOCK: does the old code let it through? ---
BYPASS   http://169.254.169.254/            shared=false reader=false
BYPASS   http://100.64.0.1/                 shared=false reader=false
BYPASS   http://[::ffff:127.0.0.1]/         shared=false reader=false
BYPASS   http://[::ffff:169.254.169.254]/   shared=false reader=false
BYPASS   http://[fd00::1]/                  shared=false reader=false
BYPASS   http://[fe80::1]/                  shared=false reader=false
BYPASS   http://[ff02::1]/                  shared=false reader=false
BYPASS   http://[64:ff9b::127.0.0.1]/       shared=false reader=false
BYPASS   http://224.0.0.1/                  shared=false reader=false
BYPASS   http://255.255.255.255/            shared=false reader=false
BYPASS   http://[::]/                       shared=false reader=false
BYPASS   http://app.localhost/              shared=false reader=false
BYPASS   http://db.internal/                shared=false reader=false
BYPASS   http://nas.home.arpa/              shared=false reader=false
BYPASS   http://127.255.255.254/            shared=false reader=false
BYPASS   http://[::1]/                      shared=true  reader=false
--- MUST ALLOW: does the old code over-block? ---
OVERBLOCK  http://172.32.0.1/    shared=false reader=true
OVERBLOCK  http://172.255.0.1/   shared=false reader=true
OVERBLOCK  http://172.15.0.1/    shared=false reader=true

What changed

Addresses are parsed with node:net and compared as masked integers against a CIDR table, not string prefixes. IPv4-mapped, IPv4-compatible and NAT64-embedded IPv6 addresses are unwrapped to their v4 payload first, so [::ffff:169.254.169.254] cannot smuggle link-local past an IPv4-only comparison. A syntactically valid but unexpandable v6 address fails closed.

There were two implementations, not the one the issue names

src/routes/api/reader/+server.js carried its own weaker copy: it blocked all of 172.* (breaking public 172.32+) while missing the bracketed [::1] form that parsedUrl.hostname actually returns — so http://[::1]/ was reachable through /api/reader today. Fixing only the shared helper would have left that route on the old heuristics. The copy is deleted; all five call sites (/api/image, /api/oer/asset, /api/pdf, /api/pdf-thumbnail, /api/reader) share one guard.

DNS resolution — the rebinding half

isBlockedHost() runs the synchronous check, then dns.lookup({ all: true }) and rejects if any answer is private.

A lookup failure stays fail-open on purpose: a name that does not resolve cannot be fetched either, so failing closed would only convert resolver blips into rejected requests. The residual TOCTOU gap (fetch resolves a second time) needs connect-time pinning and is documented in place; ASSET_PROXY_ALLOWED_DOMAINS + imgproxy network isolation remain the defence in depth.

Cost note: the guarded routes now perform a DNS lookup per request. Literal IPs short-circuit without resolving.

The "exotic encodings" bullet needed no code

The WHATWG parser normalises all of these before our code sees the hostname — asserted in the tests so it stays true:

http://2130706433/     -> 127.0.0.1     http://017700000001/ -> 127.0.0.1
http://0x7f000001/     -> 127.0.0.1     http://0/            -> 0.0.0.0
http://127.1/          -> 127.0.0.1

How the tests avoid the trap they are testing for

Every fixture goes through new URL() rather than being hand-typed, because the parser hex-compresses IPv4-mapped v6:

http://[::ffff:127.0.0.1]/       -> [::ffff:7f00:1]
http://[::ffff:169.254.169.254]/ -> [::ffff:a9fe:a9fe]

A guard written against '::ffff:' plus a dotted quad passes a hand-typed test and still lets the real URL through. That is the same failure class as the two "test asserts the shape the author imagined" bugs fixed in #58.

Controls, so a green suite means something:

• Boundary controls — 172.15/172.32/172.255, 100.63/100.128, 169.253, 192.0.1, 223.255, [::ffff:8.8.8.8], [fec0::1] must stay reachable, so "blocks everything" cannot pass.
• Redirect tests assert the internal URL was never requested, not merely that a throw happened, and one test follows a public chain to a 200 so "rejects every redirect" cannot pass.
• lookup must not be called for literal IPs and for hosts the sync check already blocks.

Live confirmation that this is not theoretical: http://[::ffff:127.0.0.1]:18108/health returns 200 from the local typesense container.

Verification

At 08f988a4: 5177/5177 tests, 468 files, pnpm lint clean, pnpm check 0 errors.

Out of scope, flagged not fixed

/api/enrich validates URLs with parseHttpUrl() and then hands them to the amb-mcp service, which does the fetching. This app never fetches them, so it is a different trust boundary — but an internal URL passed through could make amb-mcp fetch it. Not touched here; worth a separate decision.

🤖 Generated with Claude Code

Closes #31. `isPrivateIp()` guarded every server-side fetch path with hostname string prefixes. A verbatim replay of the two old implementations against this PR's fixtures lets **16 of 16** must-block addresses reach `fetch`, and wrongly blocks **3 of 4** public ones: ``` --- MUST BLOCK: does the old code let it through? --- BYPASS http://169.254.169.254/ shared=false reader=false BYPASS http://100.64.0.1/ shared=false reader=false BYPASS http://[::ffff:127.0.0.1]/ shared=false reader=false BYPASS http://[::ffff:169.254.169.254]/ shared=false reader=false BYPASS http://[fd00::1]/ shared=false reader=false BYPASS http://[fe80::1]/ shared=false reader=false BYPASS http://[ff02::1]/ shared=false reader=false BYPASS http://[64:ff9b::127.0.0.1]/ shared=false reader=false BYPASS http://224.0.0.1/ shared=false reader=false BYPASS http://255.255.255.255/ shared=false reader=false BYPASS http://[::]/ shared=false reader=false BYPASS http://app.localhost/ shared=false reader=false BYPASS http://db.internal/ shared=false reader=false BYPASS http://nas.home.arpa/ shared=false reader=false BYPASS http://127.255.255.254/ shared=false reader=false BYPASS http://[::1]/ shared=true reader=false --- MUST ALLOW: does the old code over-block? --- OVERBLOCK http://172.32.0.1/ shared=false reader=true OVERBLOCK http://172.255.0.1/ shared=false reader=true OVERBLOCK http://172.15.0.1/ shared=false reader=true ``` ## What changed Addresses are parsed with `node:net` and compared as **masked integers against a CIDR table**, not string prefixes. IPv4-mapped, IPv4-compatible and NAT64-embedded IPv6 addresses are unwrapped to their v4 payload first, so `[::ffff:169.254.169.254]` cannot smuggle link-local past an IPv4-only comparison. A syntactically valid but unexpandable v6 address **fails closed**. ## There were two implementations, not the one the issue names `src/routes/api/reader/+server.js` carried its own weaker copy: it blocked all of `172.*` (breaking public 172.32+) while missing the bracketed `[::1]` form that `parsedUrl.hostname` actually returns — so `http://[::1]/` was reachable through `/api/reader` today. Fixing only the shared helper would have left that route on the old heuristics. The copy is deleted; all five call sites (`/api/image`, `/api/oer/asset`, `/api/pdf`, `/api/pdf-thumbnail`, `/api/reader`) share one guard. ## DNS resolution — the rebinding half `isBlockedHost()` runs the synchronous check, then `dns.lookup({ all: true })` and rejects if **any** answer is private. A lookup *failure* stays fail-open on purpose: a name that does not resolve cannot be fetched either, so failing closed would only convert resolver blips into rejected requests. The residual TOCTOU gap (`fetch` resolves a second time) needs connect-time pinning and is documented in place; `ASSET_PROXY_ALLOWED_DOMAINS` + imgproxy network isolation remain the defence in depth. **Cost note:** the guarded routes now perform a DNS lookup per request. Literal IPs short-circuit without resolving. ## The "exotic encodings" bullet needed no code The WHATWG parser normalises all of these before our code sees the hostname — asserted in the tests so it stays true: ``` http://2130706433/ -> 127.0.0.1 http://017700000001/ -> 127.0.0.1 http://0x7f000001/ -> 127.0.0.1 http://0/ -> 0.0.0.0 http://127.1/ -> 127.0.0.1 ``` ## How the tests avoid the trap they are testing for Every fixture goes through `new URL()` rather than being hand-typed, because the parser **hex-compresses** IPv4-mapped v6: ``` http://[::ffff:127.0.0.1]/ -> [::ffff:7f00:1] http://[::ffff:169.254.169.254]/ -> [::ffff:a9fe:a9fe] ``` A guard written against `'::ffff:'` plus a dotted quad passes a hand-typed test and still lets the real URL through. That is the same failure class as the two "test asserts the shape the author imagined" bugs fixed in #58. Controls, so a green suite means something: - **Boundary controls** — 172.15/172.32/172.255, 100.63/100.128, 169.253, 192.0.1, 223.255, `[::ffff:8.8.8.8]`, `[fec0::1]` must stay reachable, so "blocks everything" cannot pass. - **Redirect tests assert the internal URL was never requested**, not merely that a throw happened, and one test follows a public chain to a 200 so "rejects every redirect" cannot pass. - **`lookup` must not be called** for literal IPs and for hosts the sync check already blocks. Live confirmation that this is not theoretical: `http://[::ffff:127.0.0.1]:18108/health` returns **200** from the local typesense container. ## Verification At `08f988a4`: **5177/5177 tests, 468 files**, `pnpm lint` clean, `pnpm check` **0 errors**. ## Out of scope, flagged not fixed `/api/enrich` validates URLs with `parseHttpUrl()` and then hands them to the **amb-mcp** service, which does the fetching. This app never fetches them, so it is a different trust boundary — but an internal URL passed through could make amb-mcp fetch it. Not touched here; worth a separate decision. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

laoc added 1 commit 2026-07-30 10:55:18 +00:00

fix(security): parse IPs into CIDR ranges instead of matching hostname prefixes (#31) ... 08f988a450

`isPrivateIp()` guarded every server-side fetch path with string
prefixes, so a long list of private ranges walked straight through. A
verbatim replay of the two old implementations against the fixtures in
this commit lets 16 of 16 must-block addresses reach `fetch` —
`169.254.169.254`, CGNAT, `[::ffff:127.0.0.1]`, ULA, v6 link-local,
multicast, NAT64, `127.255.255.254`, `app.localhost`, `db.internal` — and
wrongly blocks 3 of 4 public ones.

Addresses are now parsed with `node:net` and compared as masked integers
against a CIDR table. IPv4-mapped, IPv4-compatible and NAT64-embedded v6
addresses are unwrapped to their v4 payload before the check, so
`[::ffff:169.254.169.254]` cannot smuggle link-local past an IPv4-only
comparison. A syntactically valid but unexpandable v6 address fails
closed.

There were two implementations, not the one the issue names.
`src/routes/api/reader/+server.js` carried its own weaker copy: it
blocked all of `172.*` (breaking public 172.32+) while missing the
bracketed `[::1]` form that `parsedUrl.hostname` actually returns, so
`http://[::1]/` was reachable through /api/reader today. Fixing only the
shared helper would have left that route on the old heuristics. The copy
is deleted; all five call sites — /api/image, /api/oer/asset, /api/pdf,
/api/pdf-thumbnail, /api/reader — now share one guard.

DNS resolution is the other half. `isBlockedHost()` runs the synchronous
check and then `dns.lookup({ all: true })`, rejecting if any answer is
private, which closes the DNS-rebinding hole. A lookup *failure* stays
fail-open on purpose: a name that does not resolve cannot be fetched
either, so failing closed would only turn resolver blips into rejected
requests. The residual TOCTOU gap needs connect-time pinning and is
documented in place.

The issue's "exotic encodings" bullet needed no code. The WHATWG parser
normalises decimal, hex, octal and short-form IPv4 to a dotted quad
before our code sees the hostname; asserted in the tests so it stays
true.

Tests build every fixture by running the string through `new URL()`
rather than hand-typing the expected hostname, because the parser
hex-compresses IPv4-mapped v6 — `http://[::ffff:127.0.0.1]/` arrives as
`[::ffff:7f00:1]`. A guard written against the dotted form passes a
hand-typed test and still lets the real URL through. Boundary controls
(172.15/172.32, 100.63/100.128, `[::ffff:8.8.8.8]`) keep "blocks
everything" from passing, and the redirect tests assert the internal URL
was never requested rather than only that a throw happened.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

laoc added 1 commit 2026-07-30 11:25:59 +00:00

fix(ssrf): close the residual IPv6 embeddings and trailing-dot hostnames (#31) ... c485772a09

Follow-up to the same issue, from TestOER's adversarial pass on the first
commit. None of these were reachable on this host — connect fails outright
for the v6 forms, and every route goes through isBlockedHost, so the
hostname nit exposed nothing — but three of the four are one line each and
the first is a near-miss of a form the original commit did handle.

- `::ffff:0:a.b.c.d` (RFC2765 IPv4-translated, ::ffff:0:0/96) carries its
  0xffff at bytes 8-9 rather than 10-11, so it slipped past both
  zeroPrefix(10) and zeroPrefix(12). Now unwrapped like the mapped form.
- 6to4 (2002::/16) and Teredo (2001::/32) embed an IPv4 destination
  mid-address. Blocked wholesale rather than unwrapped: nothing this app
  legitimately fetches sits behind a v6 transition tunnel, and a blanket
  refusal removes a class of "did we unwrap the right four bytes"
  reasoning error from a security check. 2001:db8:: and 2003:: are pinned
  as controls so the prefix test cannot widen into global unicast.
- `fec0::/10` site-local. Deprecated by RFC3879 but still routed on some
  networks, and previously asserted as *allowed* by the test suite.
- `URL.hostname` preserves a trailing dot, so `localhost.` and
  `printer.local.` — the fully-qualified forms of names this module
  blocks, resolving to the same addresses — missed both the exact match
  and the suffix match. One dot is now stripped before comparison.

`resolvesToPrivateIp` deliberately still passes the hostname to the
resolver with its trailing dot intact: the dot is meaningful there
(absolute vs search-domain-suffixed), so normalising it away could change
which addresses get checked.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

laoc merged commit 184cea1e03 into dev 2026-07-30 11:26:31 +00:00

laoc referenced this pull request from a commit 2026-07-30 11:26:33 +00:00

Merge pull request 'fix(security): parse IPs into CIDR ranges instead of matching hostname prefixes (#31)' (#60) from fix/issue-31-ssrf-ip-ranges into dev

laoc referenced this pull request 2026-07-30 11:29:11 +00:00

SSRF: isPrivateIp() hostname heuristics miss link-local/metadata, CGNAT, IPv4-mapped IPv6, and unresolved DNS #31

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

bug

Something is broken

  

discussion

Needs a decision or concept first

  

enhancement

New feature or improvement

No labels

bug

discussion

enhancement

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

edufeed/edufeed-app!60

No description provided.