edufeed/edufeed-app

Watch

Fork 0

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

New issue

Closed

opened 2026-07-10 06:29:17 +00:00 by laoc · 2 comments

laoc commented

2026-07-10 06:29:17 +00:00

Owner

Automated security review finding (HIGH)

isPrivateIp() in src/lib/server/httpUrl.js guards the server-side fetch paths (/api/image, /api/oer/asset, fetchGuardedRedirects) against SSRF using hostname-string heuristics. Several private/internal ranges bypass it:

169.254.0.0/16 (link-local, incl. cloud metadata 169.254.169.254) — passes today
100.64.0.0/10 (CGNAT), 0.0.0.0/8, 224.0.0.0/4, 240.0.0.0/4
IPv6: ::ffff:0:0/96 (IPv4-mapped, e.g. [::ffff:127.0.0.1]), fc00::/7 (ULA), fe80::/10 (link-local)
Exotic encodings: decimal (http://2130706433/ = 127.0.0.1), hex (0x7f000001)
Hostnames are never resolved: a public DNS name pointing at a private IP passes (DNS rebinding)

Suggested fix

Parse the hostname with node:net.isIP (or ipaddr.js); compare parsed integers against CIDR ranges, not string prefixes. For non-IP hostnames, dns.lookup({ all: true }) and validate every returned address. Block 169.254.169.254 explicitly as defense-in-depth.

Mitigating context

The file documents this as a heuristic paired with the ASSET_PROXY_ALLOWED_DOMAINS allowlist and imgproxy network isolation on the homelab, and the deployment isn't on cloud infra with a metadata endpoint — so practical severity is lower than the raw HIGH rating. Still cheap to fix properly.

Found by automated security review during work on #14.

## Automated security review finding (HIGH) `isPrivateIp()` in `src/lib/server/httpUrl.js` guards the server-side fetch paths (`/api/image`, `/api/oer/asset`, `fetchGuardedRedirects`) against SSRF using **hostname-string heuristics**. Several private/internal ranges bypass it: - `169.254.0.0/16` (link-local, incl. cloud metadata `169.254.169.254`) — passes today - `100.64.0.0/10` (CGNAT), `0.0.0.0/8`, `224.0.0.0/4`, `240.0.0.0/4` - IPv6: `::ffff:0:0/96` (IPv4-mapped, e.g. `[::ffff:127.0.0.1]`), `fc00::/7` (ULA), `fe80::/10` (link-local) - Exotic encodings: decimal (`http://2130706433/` = 127.0.0.1), hex (`0x7f000001`) - Hostnames are never resolved: a public DNS name pointing at a private IP passes (DNS rebinding) ### Suggested fix Parse the hostname with `node:net.isIP` (or ipaddr.js); compare parsed integers against CIDR ranges, not string prefixes. For non-IP hostnames, `dns.lookup({ all: true })` and validate every returned address. Block `169.254.169.254` explicitly as defense-in-depth. ### Mitigating context The file documents this as a heuristic paired with the `ASSET_PROXY_ALLOWED_DOMAINS` allowlist and imgproxy network isolation on the homelab, and the deployment isn't on cloud infra with a metadata endpoint — so practical severity is lower than the raw HIGH rating. Still cheap to fix properly. *Found by automated security review during work on #14.*

laoc referenced this issue from a commit

2026-07-30 10:54:37 +00:00

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

laoc referenced this issue from a pull request that will close it,

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

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

laoc referenced this issue

2026-07-30 10:55:43 +00:00

pnpm test on dev is not a trustworthy signal: random teardown error + 2 env-dependent failures #55

laoc referenced this issue from a commit

2026-07-30 11:25:58 +00:00

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

laoc closed this issue

2026-07-30 11:26:32 +00:00

laoc referenced this issue 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 commented

2026-07-30 11:29:11 +00:00

Author

Owner

Fixed and merged to dev @ 184cea1e (PR #60, commits 08f988a4 + c485772a).

This was live, not theoretical

TestOER measured it as HTTP against real loopback services on both trees, using a canary server with a unique path per probe — so "blocked" means the fetch provably never happened rather than "the endpoint returned a 4xx".

On dev before the fix, 69 of 165 must-block probes were rejected and 41 provably reached a loopback listener. /api/image returned HTTP 200 with a transcoded WebP of internal content, and /api/reader returned the internal page body:

{"success":true,"article":{"title":"SSRF CANARY","content":"<DIV …>internal service reached…

That is a read primitive against anything on loopback, not a blind SSRF. On the test host the reachable set included typesense, blossom and strfry.

After the fix: 165/165 blocked, 0 reached the canary, 0 of 65 public addresses wrongly rejected.

What changed

Addresses are parsed with node:net and compared as masked integers against a CIDR table, instead of matching hostname string prefixes. IPv4-mapped, IPv4-compatible, IPv4-translated and NAT64-embedded IPv6 are unwrapped to their v4 payload; 6to4 and Teredo are refused wholesale; a syntactically valid but unexpandable v6 address fails closed. isBlockedHost() adds dns.lookup({ all: true }) and rejects if any answer is private.

The issue named one implementation; there were two. src/routes/api/reader/+server.js had its own private copy that blocked all of 172.* (over-broad — public 172.32+) while missing the [::1] bracket form, which URL.hostname returns. Fixing only the shared helper would have left /api/reader — the route with the read primitive — on the old heuristics. All five call sites now use the shared guard.

Two corrections to the issue text

The "exotic encodings" bullet was already handled. The WHATWG URL parser normalises http://2130706433/, http://0x7f000001/, http://127.1/, http://017700000001/ (octal) and http://0/ to their canonical hostnames before any app code runs. Verified rather than assumed. The same normalisation also covers userinfo, percent-encoded hosts, ideographic full stops and circled digits.

The DNS half is load-bearing, not defence in depth. localhost. — the trailing-dot FQDN — and this machine's own hostname framework (→ 127.0.0.2) both pass every synchronous check and are caught only by dns.lookup. Both fetched loopback on dev.

Residuals closed in the follow-up commit

TestOER's adversarial pass found four IPv6 embeddings still returning false. None reached a listening service on that host, so this was hardening rather than a live bypass — but ::ffff:0:a.b.c.d (RFC2765, ::ffff:0:0/96) is a one-line near-miss of a form that was handled, its 0xffff sitting at bytes 8-9 instead of 10-11. Also closed: 6to4, Teredo, fec0::/10 (previously asserted as allowed by the test suite), and the trailing-dot hostname forms.

Deliberately not fixed

/api/enrich validates URLs and hands them to amb-mcp, which does the fetching. Different trust boundary — this app never fetches them — but an internal URL passed through could make amb-mcp fetch it. Worth its own issue if anyone wants it closed.

Lookup failure stays fail-open. A name that does not resolve cannot be fetched either, so failing closed would only turn resolver blips into 400s. The residual TOCTOU gap between lookup and fetch is documented in place.

Verification

184cea1e: 5180 tests passed / 5180, 468 files, lint clean, pnpm check 0 errors. 53 CIDR boundary assertions (below / first / middle / last / above for each of 11 blocks) with 0 mismatches. Every test fixture is built by running the URL through new URL() rather than hand-typed, because WHATWG hex-compresses [::ffff:127.0.0.1] to [::ffff:7f00:1] and a hand-typed fixture would pass while the real URL got through.

Not proven live: redirect-to-internal end to end (no usable public first hop from the test host — unit tests cover it, including asserting the internal URL was never requested), and the per-request DNS lookup under load.

Fixed and merged to `dev` @ `184cea1e` (PR #60, commits `08f988a4` + `c485772a`). ## This was live, not theoretical TestOER measured it as HTTP against real loopback services on both trees, using a canary server with a unique path per probe — so "blocked" means the fetch provably never happened rather than "the endpoint returned a 4xx". On `dev` before the fix, **69 of 165 must-block probes were rejected and 41 provably reached a loopback listener.** `/api/image` returned **HTTP 200 with a transcoded WebP of internal content**, and `/api/reader` returned the internal page body: ```json {"success":true,"article":{"title":"SSRF CANARY","content":"<DIV …>internal service reached… ``` That is a read primitive against anything on loopback, not a blind SSRF. On the test host the reachable set included typesense, blossom and strfry. After the fix: **165/165 blocked, 0 reached the canary, 0 of 65 public addresses wrongly rejected.** ## What changed Addresses are parsed with `node:net` and compared as masked integers against a CIDR table, instead of matching hostname string prefixes. IPv4-mapped, IPv4-compatible, IPv4-translated and NAT64-embedded IPv6 are unwrapped to their v4 payload; 6to4 and Teredo are refused wholesale; a syntactically valid but unexpandable v6 address fails closed. `isBlockedHost()` adds `dns.lookup({ all: true })` and rejects if any answer is private. **The issue named one implementation; there were two.** `src/routes/api/reader/+server.js` had its own private copy that blocked all of `172.*` (over-broad — public 172.32+) while missing the `[::1]` bracket form, which `URL.hostname` returns. Fixing only the shared helper would have left `/api/reader` — the route with the read primitive — on the old heuristics. All five call sites now use the shared guard. ## Two corrections to the issue text **The "exotic encodings" bullet was already handled.** The WHATWG URL parser normalises `http://2130706433/`, `http://0x7f000001/`, `http://127.1/`, `http://017700000001/` (octal) and `http://0/` to their canonical hostnames before any app code runs. Verified rather than assumed. The same normalisation also covers userinfo, percent-encoded hosts, ideographic full stops and circled digits. **The DNS half is load-bearing, not defence in depth.** `localhost.` — the trailing-dot FQDN — and this machine's own hostname `framework` (→ `127.0.0.2`) both pass every synchronous check and are caught *only* by `dns.lookup`. Both fetched loopback on `dev`. ## Residuals closed in the follow-up commit TestOER's adversarial pass found four IPv6 embeddings still returning false. None reached a listening service on that host, so this was hardening rather than a live bypass — but `::ffff:0:a.b.c.d` (RFC2765, `::ffff:0:0/96`) is a one-line near-miss of a form that *was* handled, its `0xffff` sitting at bytes 8-9 instead of 10-11. Also closed: 6to4, Teredo, `fec0::/10` (previously asserted as *allowed* by the test suite), and the trailing-dot hostname forms. ## Deliberately not fixed **`/api/enrich`** validates URLs and hands them to amb-mcp, which does the fetching. Different trust boundary — this app never fetches them — but an internal URL passed through could make amb-mcp fetch it. Worth its own issue if anyone wants it closed. **Lookup failure stays fail-open.** A name that does not resolve cannot be fetched either, so failing closed would only turn resolver blips into 400s. The residual TOCTOU gap between lookup and fetch is documented in place. ## Verification `184cea1e`: **5180 tests passed / 5180**, 468 files, lint clean, `pnpm check` 0 errors. 53 CIDR boundary assertions (below / first / middle / last / above for each of 11 blocks) with 0 mismatches. Every test fixture is built by running the URL through `new URL()` rather than hand-typed, because WHATWG hex-compresses `[::ffff:127.0.0.1]` to `[::ffff:7f00:1]` and a hand-typed fixture would pass while the real URL got through. **Not proven live:** redirect-to-internal end to end (no usable public first hop from the test host — unit tests cover it, including asserting the internal URL was never *requested*), and the per-request DNS lookup under load.

laoc commented

2026-07-30 11:49:19 +00:00

Author

Owner

Attribution note on the verification figures above.

The two measurement reports this comment draws on — the 165/165 sentinel sweep and the canary run that captured the /api/reader body — came from two concurrent TestOER sessions running in the same worktree, which TestOER identified afterwards. They agree, and each is a real measurement, but they are not two independent replications: same agent, same checkout, overlapping build output.

Nothing in the numbers changes. Recording it because "verified twice" would be the wrong reading, and because the shared worktree is what caused the reverted edits and killed builds reported around the same time.

**Attribution note on the verification figures above.** The two measurement reports this comment draws on — the 165/165 sentinel sweep and the canary run that captured the `/api/reader` body — came from **two concurrent TestOER sessions running in the same worktree**, which TestOER identified afterwards. They agree, and each is a real measurement, but they are **not two independent replications**: same agent, same checkout, overlapping build output. Nothing in the numbers changes. Recording it because "verified twice" would be the wrong reading, and because the shared worktree is what caused the reverted edits and killed builds reported around the same time.

laoc referenced this issue from a commit

2026-07-31 09:31:34 +00:00

feat(cards): PDF page count in the linked-materials badge (#57)

laoc referenced this issue

2026-07-31 09:33:09 +00:00

feat(cards): say what the linked material is — type, size, and PDF page count (#57) #75