fix(gate): validate every hop of a redirect, not just the URL in the README - #44
Conversation
…README redirect: 'follow' meant the guard checked only the URL written in the README. A public host is free to answer 302 with Location: http://169.254.169.254/, and the runner would follow it — the exact request unroutableReason exists to stop, reached one hop later. The chain is followed by hand now, each hop validated before it is requested and the depth bounded so a redirect loop cannot spin inside a release gate. A refused hop fails the gate rather than degrading to a note: it is a finding about the link, not somebody else's outage.
There was a problem hiding this comment.
Pull request overview
This PR hardens the published-surface gate that validates README links by preventing redirect-based bypasses of the existing “unroutable/private” URL checks. It does so by manually following redirect chains and validating each hop before issuing the next request, with a bounded redirect depth.
Changes:
- Added a redirect-aware
fetchChecked()wrapper that usesredirect: 'manual'and validates eachLocationhop viaunroutableReason(). - Introduced
UnroutableRedirectto distinguish refused redirect hops from transport failures, making them fail the gate instead of degrading to notes. - Switched the link probe logic from
fetch(..., redirect: 'follow')tofetchChecked(...)for both HEAD and GET fallbacks.
… hop Every 3xx was treated as a redirect, which is wider than the Fetch standard: 301, 302, 303, 307 and 308 are hops, while a 300 or a 304 is a response to return. The set is explicit now. Each intermediate body is cancelled before the next request. undici holds the connection until the body is read or cancelled, and this runs across every README link at once. A Location that does not parse fails the gate instead of falling through to the transport-error note — it is a defect in the redirect chain, not somebody else's outage.
|
Pushed 81e602d with three refinements the review raised on the sibling PRs (bymaxone/nest-queue#65, bymaxone/nest-logger#50) — the gate is one file, identical across the ten libraries:
All red-checked against a local redirect server, including a control so real redirects still follow. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (2)
scripts/check-published-surface.mjs:279
- The timeout comment says the fetch is "Bounded per request", but with manual redirect following the same
AbortSignal.timeout(10_000)applies to the entire redirect chain (and that’s a good property). Updating the comment avoids misleading future changes.
// HEAD first; some hosts answer it with 405, so fall back to GET.
// Bounded per request: this gate runs inside `prepublishOnly`, and one
// hung host must not stall a publish until the job timeout.
scripts/check-published-surface.mjs:195
fetchCheckedonly cancels the response body for intermediate redirect responses. For non-redirect responses (and redirect responses missingLocation), the body is left unread/uncancelled, which can keep undici sockets busy (especially on the GET fallback) while this runsPromise.allacross many links.
if (!REDIRECT_STATUSES.has(res.status)) return res
…bounds Intermediate hops were already cancelled; the response finally handed back was not. Only its status is ever read, and undici holds the connection until the body is read or cancelled — across every README link at once. The timeout comment claimed 'bounded per request' while one signal covers the HEAD, the GET fallback and every redirect hop together. That is the useful bound, so the comment now describes it instead of overstating it.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (1)
scripts/check-published-surface.mjs:276
unroutableReason(url)is checked for the README URL, but the request is made toprobe(which may differ due to the npmjs.com → registry.npmjs.org rewrite). That means the first requested URL in the chain is not guaranteed to be validated byunroutableReason, which weakens the “validate before requesting” invariant if more rewrites are added later (or if this rewrite ever changes). Consider validatingprobe(when it differs) before callingfetchChecked.
const opts = { headers, signal: AbortSignal.timeout(10_000) }
let res = await fetchChecked(probe, { method: 'HEAD', ...opts })
…out covers The npmjs.com to registry.npmjs.org rewrite happens after unroutableReason has run, so the URL actually requested first was never the one validated. Today the rewrite lands on a fixed registry host and is safe; the invariant that nothing is requested before it is validated must not depend on that staying true. The timeout comment claimed the signal covers every hop. It covers the requests: each hop's DNS lookup carries its own 5s bound outside the signal, so a chain's worst case is the timeout plus one lookup per hop. Both are bounded — they are simply not one budget, and the comment now says so.
|
Applied the suppressed comment on the rewrite in 38d4636 — good catch, and the sharpest one in this round. |
Found by Copilot reviewing this same shared script on
bymaxone/nest-realtime#57, and synced here byte-for-byte — the gate is one file, identical across the ten libraries.Why
redirect: 'follow'meant the guard checked only the URL written in the README. A public host is free to answer302withLocation: http://169.254.169.254/…, and the runner would follow it — reaching exactly the endpointunroutableReasonexists to stop, one hop later. Every IP-literal and private-hostname check added earlier was bypassable by a single redirect.What changed
The chain is followed by hand: each hop is validated before it is requested, the depth is bounded at 5 so a redirect loop cannot spin inside a release gate, and a refused hop fails rather than degrading to a note — it is a finding about the link, not somebody else's outage.
Red-check
Against a local server issuing a real
302, with a control so the guard is not simply blocking everything:lintandcheck:publishedare green here.Nothing published changes:
scripts/is outsidepackage.json→files, sodist/and the tarball are untouched and no version bump is needed.