Skip to content

Stop a redirect replaying the request it was answering - #9

Merged
GunsNR merged 1 commit into
mainfrom
claude/dns-rebinding-socket-pinning-ar8ilh
Aug 28, 2026
Merged

GunsNR merged 1 commit into
mainfrom
claude/dns-rebinding-socket-pinning-ar8ilh

Conversation

@GunsNR

@GunsNR GunsNR commented Aug 28, 2026

Copy link
Copy Markdown
Owner

Closes the three outbound-request gaps recorded in the pre-merge review of #8. Narrow by design: one source file, one test file, two documents.

The gap

#8 stopped a redirect choosing an unchecked destination. It did not stop a redirect choosing what got sent there.

Credentials were stripped across an origin boundary, but the method and body were not. A 307 — or a 302 answering a PUT — re-issued the entire request against whatever host the redirect named. For publishPost that is an article body, delivered to a site chosen by whoever controls the redirect, on behalf of a customer who asked only to publish to their own WordPress.

Two smaller gaps alongside it: Proxy-Authorization was not in the strip list, and the timeout budget was read only after DNS resolution returned, so a hanging name server ran past the timeout it was meant to obey (measured: a 400 ms resolver against a 100 ms budget took 401 ms).

What changed

1. Credential stripping — Proxy-Authorization joins Authorization, Cookie and X-SuperTool-Key, stripped on every cross-origin hop.

2. Redirect method semantics (RFC 9110 §15.4):

Situation Result
303 on a non-GET/HEAD → GET, body dropped
301/302 on a POST → GET, body dropped
Body dropped content-type, content-length, content-encoding, content-language, content-location, transfer-encoding removed
Cross-origin 307/308 refused
Cross-origin 301/302 preserving any other non-GET method refused
Same-origin 307/308 unchanged — method and body preserved

Rewriting fixes the common case, which is what browsers have done for decades. What rewriting cannot fix is 307/308, whose whole definition is that method and body survive — there the only safe answer across an origin boundary is to refuse. A stale content-length on a dropped body is worse than merely wrong: the next request waits for bytes nobody will write.

3. Resolver deadline — resolution races the caller's remaining budget. On timeout the request fails closed having learned no address and opened no socket. The lookup itself cannot be cancelled and may still be in flight afterwards; that is recorded in the runbook rather than glossed over.

Tests — 12 new, all six required proofs

Test Proves
never lets article content reach a cross-origin redirect target A POST body containing THE-ARTICLE-BODY is absent from the cross-origin hop — asserted on the whole serialized request, not just the body field
turns a 303 into a GET and drops the body Method GET, body undefined
refuses a cross-origin 307 or 308 Both statuses throw, and the second host is never contacted (calls has length 1)
strips proxy-authorization along with the other credentials All four credentials gone, X-Keep retained
fails closed when the resolver hangs, without opening a socket Rejects inside the budget; transport never called
gives each redirect hop the remaining budget, never a fresh one Per-hop budgets strictly decreasing across three hops

Plus: body-describing headers removed; cross-origin 301/302 on a PUT refused; same-origin 307 still preserves method and body; cross-origin GET redirect still allowed; a slow resolver's cost is charged to the connection budget; an exhausted budget refuses before resolving again.

The last four exist to catch over-blocking — the failure mode of a change like this is refusing traffic that was always fine.

Preserved

Socket pinning, byte limits (encoded and decoded), TLS/SNI and Host behaviour, and every public signature: safeFetch, checkResolvedHost, resolveAndPin, BlockedRequestError, SafeFetchOptions. resolveAndPin's existing third parameter gained an optional timeoutMs field rather than changing shape. All 56 pre-existing tests across net-fetch, net-pinned, crawler.integration and wordpress.integration pass untouched.

Verification

Run locally against PostgreSQL 16, matching the CI job:

  • npm run typecheck — clean
  • npm run lint — clean
  • npm test — 675 passed / 675, 39 files (was 663)
  • npm run build — compiled successfully
  • npm run db:rehearse — passed

Behavioral change worth knowing

A POST receiving a 301/302 becomes a GET and loses its body. Correct, and what browsers do — but it makes one real case fail differently: a WordPress site stored as http:// that redirects to https:// will now see the publish arrive as a GET. It previously arrived as a POST stripped of its credentials and failed as a 401. Neither is a working publish; the fix in both cases is to store the https:// URL. Documented in ADR-017 and the runbook.

Scope

No capability, provider, roadmap or deployment change. capabilities.ts and roadmap.ts untouched, nothing deployed, Railway unmodified, Phase 2 stays in progress.


Generated by Claude Code

Pinning the socket stopped a redirect choosing an unchecked destination. It
did not stop a redirect choosing what got sent there. Credentials were
stripped across an origin boundary, but the method and the body were not, so
a 307 — or a 302 answering a PUT — re-issued the whole request against
whatever host the redirect named. For publishPost that is an article body,
delivered to a site chosen by whoever controls the redirect, on behalf of a
customer who asked only to publish to their own WordPress.

Redirects now follow RFC 9110 section 15.4 method semantics. A 303, and a
301 or 302 answering a POST, become GET with the body dropped and the
headers that described it removed, because a stale content-length makes the
next request wait for bytes nobody will write. What rewriting cannot fix is
307 and 308, whose definition is that method and body survive; across an
origin boundary those are refused outright, as is a 301 or 302 that would
preserve any other non-GET method. Same-origin redirects still preserve
method and body, so ordinary same-site behaviour is unchanged.

Proxy-Authorization joins Authorization, Cookie and the API key in the
credentials stripped when a hop crosses origin.

DNS resolution now runs inside the caller's remaining timeout. The budget
used to be read only after resolution returned, so a name server that never
answered ran past the timeout it was meant to obey; a 400ms resolver against
a 100ms budget took 401ms. It now races the remaining time and fails closed
without learning an address or opening a socket. The lookup itself cannot be
cancelled and may still be in flight afterwards, which is recorded in the
runbook rather than glossed.

Socket pinning, byte limits, TLS and SNI behaviour and every public
signature are unchanged. No capability, provider, roadmap or deployment
change; Phase 2 stays in progress.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PQ2eShwv2iVM3CQYpjEkxK

GunsNR commented Aug 28, 2026

Copy link
Copy Markdown
Owner Author

Final pre-merge review — findings

Read-only review against seven criteria, with runtime probes for the ones that cannot be settled by reading. No blockers. One non-blocking gap recorded at the end.

1. Origin comparison — scheme, normalized hostname, effective port ✅

URL.origin is the comparison, and it normalizes all three. Probed:

Redirect from → to Same origin? Credentials
https://example.com/a → https://example.com:443/b yes retained
https://example.com/a → HTTPS://EXAMPLE.COM/b yes retained
https://example.com/a → https://example.com:8443/b no stripped

The explicit-default-port and uppercase cases matter in the permissive direction — treating them as cross-origin would strip credentials on harmless same-site redirects. Scheme is part of the origin, so an https→http downgrade is correctly cross-origin.

One asymmetry worth knowing: example.com. (trailing dot) is a distinct origin from example.com even though both resolve identically, so such a hop is treated as cross-origin. That errs toward refusing/stripping, which is the safe direction.

2. Body and header removal on 301/302/303 ⚠️ (non-blocking)

Body dropped and six headers removed: content-type, content-length, content-encoding, content-language, content-location, transfer-encoding.

Every header whose staleness could cause a wire-level failure is covered — a leftover content-length or transfer-encoding would make the next request hang waiting for bytes nobody writes.

Missing: content-range, which RFC 9110 §8 also lists as a representation header. Impact today is nil: no call site sends it (wordpress.ts sends Authorization/Content-Type/Accept, the crawler sends User-Agent/Accept), and unlike content-length a stray content-range on a bodyless GET is meaningless rather than harmful — no hang, no security consequence. Expect: 100-continue is in the same category. Worth a one-line follow-up; not a reason to hold this PR.

3. 307/308 policy consistency ✅

One rule, stated identically in all four places:

  • Implementation — crossOrigin && (!isSafeMethod(nextMethod) || (!dropBody && currentBody !== undefined))
  • Tests — cross-origin 307 and 308 refused; cross-origin 302 on a PUT refused; same-origin 307 allowed; cross-origin GET allowed
  • ADR-017 — "refuses any cross-origin hop that would repeat a non-GET method or a request body"
  • Runbook — "Cross-origin hops that would repeat a non-GET method or a request body are now refused"

No contradiction. The rule is about repeating a request across an origin boundary, not about 307 specifically, so a cross-origin GET carrying a body is refused too.

4. Case-insensitive stripping on every hop, including multi-hop ✅

stripHeaders lowercases each key before matching. Probed a three-hop chain example.com → other.example → example.com with deliberately odd casing:

hop 1  {"AUTHORIZATION":"Bearer S","PROXY-authorization":"Basic P","CoOkIe":"x=1","X-Keep":"y"}
hop 2  {"X-Keep":"y"}
hop 3  {"X-Keep":"y"}      ← back on the ORIGINAL origin

The important result is hop 3: credentials stripped on the way out do not reappear when the chain returns to the origin that legitimately held them. headers is reassigned rather than recomputed per hop, so removal is permanent.

5. DNS deadline draws on the shared budget ✅

const deadline = Date.now() + timeoutMs;      // once, before the loop
const beforeResolving = deadline - Date.now(); // per hop
… resolveAndPin(…, { timeoutMs: beforeResolving })
const remaining = deadline - Date.now();       // after resolving
… transport({ …, timeoutMs: remaining })

Resolution and connection both draw from one clock set once. No hop starts a fresh timeout; a slow resolver's cost is charged to the connection that follows it.

6. Timer lifecycle and late resolver results ✅

  • finally { if (timer) clearTimeout(timer); } clears on success and on timeout — no dangling timer either way.
  • Promise.race leaves a handler attached to the resolver promise, so a late rejection is absorbed rather than surfacing as an unhandled rejection. Probed: a resolver rejecting 120 ms after a 60 ms budget produced no unhandled-rejection output.
  • A late success cannot act: probed, transport call count is 0 after the abandoned resolver resolves. resolveAndPin has already returned {allowed: false} and safeFetch has already thrown, so no address is learned, no socket opens, and no request state is touched.

7. Same-origin body-preserving redirects ✅

  • Probed a 307 → 308 → 200 same-origin chain: method stays POST across all three hops and the body is byte-identical each time.
  • No consumed-body hazard exists by construction. body is typed string | Uint8Array in both SafeFetchOptions and PinnedRequestSpec — no stream, no ReadableStream, nothing single-use. Both types are freely replayable.
  • currentBody is only ever left as the original reference or set to undefined; it is never mutated. Probed with a Uint8Array: same object reference at hop 1 and hop 2, contents unchanged.

Verified locally on b5e87ac: typecheck clean, lint clean, 675/675 tests, against PostgreSQL 16. All probe files removed; working tree clean.


Generated by Claude Code

@GunsNR
GunsNR marked this pull request as ready for review August 28, 2026 20:58
@GunsNR
GunsNR merged commit 0c11367 into main Aug 28, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants