fix(query): follow redirects carried in X-Server-Function-Redirect (#603) - #606
Open
Stephen-Psaradellis wants to merge 1 commit into
Open
Stephen-Psaradellis wants to merge 1 commit into
Stephen-Psaradellis wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: 77721aa The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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.
Summary
Fixes #603.
A
redirect()thrown or returned inside a"use server"function wrapped inquery()redirected on a full page request but not on a client-side navigation. For scripted callers the server-function transport masks the 3xx to a 200, deletesLocation, and carries the target inX-Server-Function-Redirect(<status> <absolute-url>). The client transport hands thatResponseover whole.query'shandleResponseonly checkedLocation, so it fell through, and theResponsebecame the query's value. The navigation then completed as if the guard had passed.handleResponsenow decodes the carrier whenLocationis absent, on the client only, using the runtime's owndecodeRedirectHeaderValue, the same readeraction()uses. A same-origin target becomespathname + search + hashand goes through the existing branch: softnavigate(..., { replace: true }),X-Revalidatekeys invalidated and swept, and the read held pending. A cross-origin target keeps its absolute href and navigates the document, asaction()does.query.ts deliberately does not import
@solidjs/web/server-functionsstatically; see its header comment on keeping the transport and codec out of plain-fetch apps. So the decoder is loaded with a dynamicimport()that only runs when the header is present. A carrier can only come from the transport, which means the module is already loaded wherever that happens. The header name is a local constant next toLocationHeader, with a comment pointing atREDIRECT_HEADER.One behaviour change to weigh: the header name now lives in query.ts as a literal (
"X-Server-Function-Redirect"), because importingREDIRECT_HEADERwould bring back the static import. If core ever renames the header, this check drifts silently; the router's test mocks already mirror the same literal. The redirect path also awaits one dynamic import before it navigates, and only when the carrier is present. A seam-leveldecodeRedirectonServerFunctionRPCwould remove both points, but that is a core change, so I left it out.How did you test this change?
Against
nextat f7602bf, withsolid-js/@solidjs/web2.0.0-rc.8 from the lockfile. On Windows I had to install withpnpm install --frozen-lockfile --config.node-linker=hoisted: the default pnpm layout put vitest's chunk path past 260 characters, and vitest then failed at startup withERR_PACKAGE_IMPORT_NOT_DEFINED. That was local, not a repo problem.New tests in
test/query-redirect.spec.tsx, under a newdescribe("redirects carried by the server-function transport (#603)"). They build the masked shape by hand: a 200 with noLocationandX-Server-Function-Redirect: 302 <absolute-url>, returned from the query the way the client transport returns it.a masked redirect navigates and never reaches consumers: expects the/sign-inroute, with no value observed by consumers and nothing caught.X-Revalidate keys on a masked redirect invalidate and revalidate: expects the/loginroute, 2 session fetches, anduser:anonymous.npx vitest run test/query-redirect.spec.tsxAssertionError: expected '<div><span>account:</span></div>' to contain 'sign-in-page'AssertionError: expected '<section><header>user:expired</header>…' to contain 'login-page', where the received HTML includes<span>files:[object Response]</span>.Full suites:
npx vitest runnpx vitest run --config vitest.config.server.tsnpx tsc --project tsconfig.test.json(test:types)npx tsc --noEmitThe same two jsdom tests fail before and after, both on the 5000 ms timeout:
exact-optional-types.spec.ts › accepts trees with present-but-undefined optional propertiestakes 8.9–12 s on this machine.navigate-commit.spec.tsx › rapid-fire navigation bursts always land on the last targetpasses when its file runs on its own.Neither touches query.ts.
Bundle:
pnpm buildexits 0, and the import stays a lazyawait import('@solidjs/web/server-functions')in bothdist/data/query.jsand the flatdist/index.js.node .shake-check/check.mjs(patched build) exits 0 with every pass ok. In the split pass, which mirrors a Vite app, the eager chunk still reportsquery.ts excludedandaction.ts excluded, and the codec markers stay excluded. The split-pass eager size is 62.9 KB unminified. I didn't build the base for a size comparison.Prettier: the new test file passes
prettier --check.src/data/query.tsalready fails--checkat base, in lines this PR does not touch, so I didn't reformat it; none of the added lines show up in prettier's diff. The PR also adds a patch changeset (.changeset/query-decodes-redirect-carrier.md).Not covered by a test: the cross-origin branch (jsdom cannot navigate the document), and an end-to-end run with a real compiled
"use server"function in a SolidStart app. The tests reproduce the wire shape of solid'smaskRedirectrather than running it.Disclosure. An AI system running an autonomous software project found the issue, wrote the fix and the tests, and wrote this description; I'm the account holder submitting it and I have reviewed the patch. It is free to take, change or drop.
If it's useful: I'd take the next scoped fix like this one for a fixed $400 on Solid's Development Support terms - a reviewed PR with tests, invoiced as one Open Collective expense only after you merge it.