Decide redirects in one place instead of along a loop - #562
Merged
Conversation
The redirect loop in fetchCore re-sent the original method and body on every hop, so a POST redirected by a 301, 302 or 303 arrived at the target as a POST carrying a body the target did not expect, and was rejected. Only 307 and 308 ask for the method and body again. The loop now tracks the method, body and headers of the request it is about to send rather than reading them from the caller's options each hop. A status other than 307 or 308 turns the next request into a GET with no body and drops the headers that describe a body, so the form content type does not travel on a request that no longer carries a form. Because the downgrade is carried forward, a 302 followed by a 307 keeps the GET instead of resurrecting the POST. Reachable today through a configured sparqlEndpoint behind a redirect, such as an http URL or a bare host: postForm is the only caller that sends a body. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Only Content-Type was covered, so trimming the dropped-header set to that one alone left the suite green. The new test sends Content-Encoding, Content-Language and Content-Location as caller headers and asserts none of them reach the downgraded hop. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
postForm and its only caller arrived with the Wikibase pack in the same unreleased cycle as this fix, so no released version sends a body through the redirect loop. The entry would tell a reader upgrading from 0.16.0 about a failure that version cannot produce. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
node-fetch recomputes Content-Length only for a request that carries a body, so a value the caller set reaches the bodyless GET a redirect downgrade produces, telling the target to expect a body that never arrives. The Fetch standard leaves the header out of its request-body list because there the fetch layer always owns it; here it does not, so the downgrade removes it alongside the other body-describing headers. Also names the downgrade condition rather than testing the preserving set in the negative. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A 301, 302 or 303 asks for the request to be re-sent as a GET with no body. For the one caller that sends a body that is not a smaller request, it is a different one: a SPARQL query travels in the body, so the GET arrives with nothing to run and the service answers about a request the caller never made. Following the redirect and reporting that answer hides the actual fault, which is that the published endpoint URL has moved. Such a hop is now refused. 307 and 308 re-send the method and body as before, and a request with no body — which is every other request this module sends, all of them already GETs — follows any redirect unchanged. The 3xx body is destroyed on the way out, so the refusal does not strand the connection it declined to follow. The Wikibase layer turns the refusal into a message naming the setting to change, and names the target by scheme and host only. A query-service URL can carry a token in its path or query, which is why that layer substitutes the endpoint out of anything it reports; a target derived from that URL inherits the token and, differing in scheme, is not a substring that substitution can find. Scheme and host identify the redirect without carrying a credential. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The redirect loop hand-rolled the Fetch standard's redirect algorithm and implemented part of it, so each rule it got wrong was a separate bug in the same fifteen lines, and none of them could be tested without standing up a server. Four of them were open at once: a body silently discarded, a connection held for every hop followed, credentials re-sent across a change of host, and any 3xx with a Location treated as a destination. The rules now live in requestChain.ts as one pure decision. nextHop reads what was sent and the status and Location that came back, and answers deliver, follow or refuse; fetchCore performs the I/O each answer asks for. Because a refusal is a returned value rather than a throw, every response that is not delivered leaves through one place, which is what disposes of the abandoned bodies rather than a call at each exit that a later edit can forget. What the rules now say: follow only 301, 302, 303, 307 and 308; refuse a hop that would drop the request body; refuse a hop from https to http; drop credential headers across any change of origin; cap the chain; and read a failing source's diagnostics up to a limit rather than whole. Credentials go on a change of origin rather than of host, which is stricter than node-fetch, whose rule keeps them for a subdomain and ignores the port — on a host that gives its tenants subdomains that would hand one tenant's credentials to another. The method is derived from the presence of a body, so a bodyless POST is not representable and the refusal cannot be reached by a request that has nothing to lose. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The rationale for each rule belongs in the commit and the pull request, not beside the rule, and a doc comment that walks through guard clauses the reader can see earns nothing. What is left is the part that cannot be derived: the two places this departs from the Fetch standard, why credentials go on a change of origin rather than of host, why a refusal withholds the target, and the ordering against the transport's own address check.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #549
Fixes #555
Fixes #556
Fixes #559
Supersedes #551, and is built on its branch so its verified work is carried rather than re-derived.
fetchCorehand-rolled the Fetch standard's redirect algorithm and implemented part of it. Each rule it got wrong was a separate open issue in the same fifteen lines, and none could be tested without standing up a server — which is why three of the four went unnoticed until a socket count was taken.The rules now live in
src/transport/requestChain.tsas one pure decision.nextHopreads what was sent and the status and Location that came back, and answers deliver, follow or refuse;fetchCoreperforms the I/O each answer asks for. Because a refusal is a returned value rather than a throw, every response that is not delivered leaves through one place — which is what disposes of the abandoned bodies, instead of a call at each exit that a later edit can forget.What the rules now say
300offers a choice and a305names a proxy, so neither is an addresshttpstohttpTwo of these are deliberate departures from the standard, both because this server fetches URLs a caller chose: the standard re-sends a 301/302/303 as a bodyless GET, and it follows an
https→httphop once it has stripped the credentials.Credentials go on a change of origin, not of host. That is stricter than node-fetch, whose rule keeps them for a subdomain of the current host and ignores the port — on a host that gives its tenants subdomains, that would hand one tenant's credentials to another.
The method is now derived from the presence of a body, so
fetchCorehas nomethodoption and a bodyless POST is not representable. That retires the question #551 left open — whether the refusal should key on the body or the method — by making the two the same thing.What this costs, measured
Unchanged from #551, and still the thing to weigh: a wiki publishing a 301/302/303-redirecting query-service URL loses queries that worked before. Verified live:
http://database.factgrid.de/sparql301→https://database.factgrid.de/sparql200http://dbpedia.org/sparql303→https://dbpedia.org/sparql200Both are plaintext endpoints upgrading to TLS, so the first request already went out in the clear. Refusing surfaces that; following it hides it. Endpoints answering
307/308are unaffected.For the reviewer
requestChain.tsimports nothing. No node-fetch, no DNS, no clock. That is what makes the rules testable, and it is also what would let an eventual move to undici keep this file and delete only the parts that duplicate the standard.https→http://169.254.169.254is reported as insecure rather than as private. Both refuse it. One existing SSRF test's fixture was anhttps→httpredirect and so was testing two rules at once; it now uses anhttpstarget, which is what it meant to test.Verification
tests/transport/requestChain.test.ts— 39 cases, no mocks, no sockets, noResponseobjects. 14 mutations applied one at a time, all 14 caught. The one that initially survived was a real gap: resolving a relativeLocationagainst the start of the chain rather than the current hop passed everything, because every relative-Location case was on the first hop where the two URLs are equal. There is now a second-hop case.tests/transport/httpFetch.disposal.test.tsgained the test #555 needed and had no way to express: a followed hop whose body is never read. It fails when disposal is moved back to the refusal path only. Its harness now records every socket the server served, because the second hop used to overwrite the first and hide exactly the leak being measured.Gates individually, all exit 0:
npm run lint,npm run typecheck,npm run fmt:check,npm test(151 files, 2122 tests).Not in this change
#561(a token in a redirect target escaping the endpoint redaction) is in the Wikibase layer, not here. The uncapped success body —fetchPageHtmlreads an unboundedtext()on the discovery and probe paths — is a different resource on a different path. DuplicateLocationheaders join to a bogus URL that this still follows; it needs its own decision about which to honour.