fix(start-server-core): fall back to GET handler for HEAD requests (RFC 9110 §9.3.2)#7325
fix(start-server-core): fall back to GET handler for HEAD requests (RFC 9110 §9.3.2)#7325Zelys-DFKH wants to merge 4 commits intoTanStack:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR makes HEAD requests to exact-match server file routes fall back HEAD → GET → ANY, strips the response body when the handler is not a native HEAD handler while preserving status/headers (including redirects), and adds route examples, tests, generated route-tree entries, and a changeset. ChangesHEAD Request Fallback Implementation
Sequence DiagramsequenceDiagram
participant Client as Client
participant Start as StartHandler
participant Router as Router
participant File as FileRouteHandler
Client->>Start: HEAD /path
Start->>Router: match route & select handler (HEAD → GET → ANY)
Router->>File: invoke chosen handler
File-->>Router: Response (status, headers, maybe body)
Router->>Start: middleware returns Response
Start-->>Client: Response (same headers/status, body null)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: Turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 👉 Get your free trial and get 200 agent minutes per Slack user (a $50 value). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
Bundle Size Benchmarks
Trend sparkline is historical gzip bytes ending with this PR measurement; lower is better. |
|
|
||
| // Strip body from HEAD responses that fell back to the GET handler. | ||
| // RFC 9110 §9.3.2: HEAD must send the same headers as GET, but with no body. | ||
| if (isHeadFallingBackToGet && ctx.response instanceof Response) { |
There was a problem hiding this comment.
the wrapped response does not preserve statusText
better:
return new Response(null, ctx.response)| handlers[requestMethod] ?? | ||
| handlers['ANY'] ?? | ||
| (requestMethod === 'HEAD' ? handlers['GET'] : undefined) | ||
| isHeadFallingBackToGet = |
There was a problem hiding this comment.
should we strip out the body in case ANY handles the HEAD?
| const handler = handlers[requestMethod] ?? handlers['ANY'] | ||
| // Per RFC 9110 §9.3.2, HEAD should return the same header fields as GET. | ||
| // Fall back to the GET handler when no explicit HEAD or ANY handler is defined. | ||
| const handler = |
There was a problem hiding this comment.
if both GET and ANY exist, HEAD uses ANY, not GET.
could be solved via:
const handler =
requestMethod === 'HEAD'
? handlers['HEAD'] ?? handlers['GET'] ?? handlers['ANY']
: handlers[requestMethod] ?? handlers['ANY']There was a problem hiding this comment.
add a separate E2E test route for this
There was a problem hiding this comment.
Fixed. Handler selection is now handlers['HEAD'] ?? handlers['GET'] ?? handlers['ANY'], so GET always wins over ANY for HEAD requests.
Added /api/get-and-any with separate GET and ANY handlers, and a test that sends HEAD /api/get-and-any and asserts x-handler: GET.
| // Strip body from HEAD responses that fell back to the GET handler. | ||
| // RFC 9110 §9.3.2: HEAD must send the same headers as GET, but with no body. | ||
| if (isHeadFallingBackToGet && ctx.response instanceof Response) { | ||
| return new Response(null, { |
There was a problem hiding this comment.
A GET-only handler returning redirect({ to: '/login' }) can become a 307 without Location, because handleRedirectResponse() no longer sees isRedirect(response).
either resolve redirects before stripping the body, or strip the body after handleRedirectResponse().
There was a problem hiding this comment.
Fixed. handleRedirectResponse now runs before the body-stripping step, so Location survives.
Added /api/head-redirect-fallback (GET handler returns redirect({ to: '/api/head-fallback', statusCode: 307 })), with a test that sends HEAD and checks the browser can follow the redirect to a 200. If Location were dropped, the browser would stall on the 307 and the assertion would fail.
Also adopted your new Response(null, resolved) suggestion from the other thread. Cleaner.
|
View your CI Pipeline Execution ↗ for commit 0d3549e
☁️ Nx Cloud last updated this comment at |
Merging this PR will not alter performance
Comparing 
Footnotes
|
- HEAD priority is now HEAD -> GET -> ANY (was HEAD -> ANY -> GET) - body stripping covers ANY fallback, not just GET fallback - resolve redirects before stripping so Location header is preserved - copy statusText to the stripped response - add E2E tests for all three HEAD fallback scenarios
Zelys-DFKH
force-pushed
the
fix/head-request-fallback-to-get-handler
branch
from
0f65fbf to
37da897
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/start-server-core/src/createStartHandler.ts (1)
944-951:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
instanceof Responseguard makeshandleRedirectResponsea no-op and silently skips body-stripping for TanStack redirect objects.Two interconnected effects stem from the
instanceof Responseguard:
Dead call:
redirect()returns a newRedirectobject — a plain object, never a nativeResponse. ThereforeisRedirect(nativeResponse)is alwaysfalsefor values that satisfyinstanceof Response, andhandleRedirectResponsereturns immediately without doing any redirect resolution in every case this branch is entered.resolved === ctx.responseunconditionally.Missing body-strip for TanStack redirects: When the HEAD-fallback GET/ANY handler returns a TanStack
redirect({to: '/login'}),executeMiddlewarecatches it viaisSpecialResponseand stores it inctx.responseas aRedirectobject. BecauseRedirectis notinstanceof Response, the body-stripping block at line 944 is skipped entirely andctx.responsefalls through toreturn ctx.responseat line 953. The outerhandleRedirectResponseat line 774 does correctly resolve it and preserves theLocationheader, but the resolved response's body is not stripped — a technical RFC 9110 §9.3.2 violation ("server MUST NOT send a message body in the response to HEAD"). In practice redirect bodies are empty (e.g.Response.redirect()produces a null body), so the observable impact is low.The fix is to drop the
instanceof Responseguard and lethandleRedirectResponsesee the TanStack redirect directly:🛠️ Proposed fix
- if (isHeadFallback && ctx.response instanceof Response) { - const resolved = await handleRedirectResponse(ctx.response, request, getRouter) + if (isHeadFallback) { + const resolved = await handleRedirectResponse( + ctx.response as Response, + request, + getRouter, + ) return new Response(null, { status: resolved.status, statusText: resolved.statusText, headers: resolved.headers, }) }With this change:
- For native
Responsevalues →handleRedirectResponseearly-returns unchanged, body stripped as before. ✓- For TanStack
redirect()objects →handleRedirectResponseresolves to a proper HTTP Response (preservingLocation), then the body is stripped. ✓- For
undefined→ impossible; middleware always setsctx.responseor throws.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/start-server-core/src/createStartHandler.ts` around lines 944 - 951, The branch that checks "instanceof Response" prevents handleRedirectResponse from processing TanStack Redirect objects and skipping body-stripping; remove the instanceof guard so that when isHeadFallback is true you always call await handleRedirectResponse(ctx.response, request, getRouter) and use its resolved Response to construct the new Response (preserving resolved.status, statusText, headers) so redirect objects returned by middleware are resolved and their bodies get stripped; update the block around isHeadFallback to reference ctx.response and call handleRedirectResponse unconditionally (retain the same usage of request and getRouter).
🧹 Nitpick comments (1)
e2e/react-start/server-routes/tests/server-routes.spec.ts (1)
20-71: ⚡ Quick winThe three core HEAD fallback scenarios are well-covered; consider adding a redirect + HEAD test.
Tests 1–3 validate GET fallback, ANY fallback, and GET-over-ANY preference. What's absent is a test for a GET handler that returns a TanStack
redirect()(the scenario flagged in the past review comment and the code path guarded byhandleRedirectResponseat line 945 ofcreateStartHandler.ts). Due to theinstanceof Responsebug noted above, a TanStack redirect returned by the HEAD-fallback handler currently skips the body-stripping block entirely. A test exercising that path would make the gap immediately visible.Suggestion: add a test fixture route (e.g.,
/api/head-fallback-redirect) whose GET handler returnsredirect({ to: '/api/head-fallback', statusCode: 307 }), then assert thatHEADreceives a307with aLocationheader and an empty body.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/react-start/server-routes/tests/server-routes.spec.ts` around lines 20 - 71, Add a new e2e test and fixture route to exercise the GET-redirect + HEAD fallback path: create an API route (e.g., handler name or path /api/head-fallback-redirect) whose GET handler returns TanStack's redirect({ to: '/api/head-fallback', statusCode: 307 }), then add a Playwright test similar to the other HEAD-fallback tests that does fetch('/api/head-fallback-redirect', { method: 'HEAD' }) and asserts status === 307, that the Location header points to /api/head-fallback and that the response body is an empty string; this will exercise the handleRedirectResponse path in createStartHandler.ts and reveal the instanceof Response issue noted in the review.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/start-server-core/src/createStartHandler.ts`:
- Around line 944-951: The branch that checks "instanceof Response" prevents
handleRedirectResponse from processing TanStack Redirect objects and skipping
body-stripping; remove the instanceof guard so that when isHeadFallback is true
you always call await handleRedirectResponse(ctx.response, request, getRouter)
and use its resolved Response to construct the new Response (preserving
resolved.status, statusText, headers) so redirect objects returned by middleware
are resolved and their bodies get stripped; update the block around
isHeadFallback to reference ctx.response and call handleRedirectResponse
unconditionally (retain the same usage of request and getRouter).
---
Nitpick comments:
In `@e2e/react-start/server-routes/tests/server-routes.spec.ts`:
- Around line 20-71: Add a new e2e test and fixture route to exercise the
GET-redirect + HEAD fallback path: create an API route (e.g., handler name or
path /api/head-fallback-redirect) whose GET handler returns TanStack's
redirect({ to: '/api/head-fallback', statusCode: 307 }), then add a Playwright
test similar to the other HEAD-fallback tests that does
fetch('/api/head-fallback-redirect', { method: 'HEAD' }) and asserts status ===
307, that the Location header points to /api/head-fallback and that the response
body is an empty string; this will exercise the handleRedirectResponse path in
createStartHandler.ts and reveal the instanceof Response issue noted in the
review.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ba0be7a3-20de-49ae-8923-8f92f12870af
📒 Files selected for processing (6)
.changeset/fix-head-request-server-route-fallback.mde2e/react-start/server-routes/src/routeTree.gen.tse2e/react-start/server-routes/src/routes/api/get-and-any.tse2e/react-start/server-routes/src/routes/api/head-fallback.tse2e/react-start/server-routes/tests/server-routes.spec.tspackages/start-server-core/src/createStartHandler.ts
✅ Files skipped from review due to trivial changes (3)
- e2e/react-start/server-routes/src/routes/api/get-and-any.ts
- .changeset/fix-head-request-server-route-fallback.md
- e2e/react-start/server-routes/src/routeTree.gen.ts
…irect+HEAD E2E test The `instanceof Response` check before the HEAD body-stripping block was redundant — `redirect()` returns `new Response(...)` and IS instanceof Response, and `ctx.response` is always a Response at that point in the middleware pipeline. Replacing it with an `as Response` cast eliminates dead branch analysis noise. Also adds a fourth E2E test: HEAD on a route whose GET handler returns a `redirect()` must preserve the Location header so the browser can follow it. The test verifies this by asserting the final status is 200 (meaning the 307 redirect was followed). If the Location header were stripped before the redirect resolved, the browser would stall on the 307 and the assertion would fail.
…iewer suggestion Adopts the tighter form suggested by @schiller-manuel: passing the resolved Response directly as the ResponseInit arg copies status, statusText, and headers in one shot instead of spreading them manually.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/start-server-core/src/createStartHandler.ts`:
- Around line 944-950: The current isHeadFallback branch forces ctx.response
through handleRedirectResponse and always wraps its result into new
Response(...), which converts an undefined/absent response into a synthetic 200
OK; change the logic in the isHeadFallback block (the call to
handleRedirectResponse with ctx.response, request, getRouter) to check the
returned value (from handleRedirectResponse) and if it is undefined/null do not
create a new Response—propagate undefined (or return early) instead; update
types so the variable (e.g., resolved) is typed as ResponseInit | undefined and
the code only calls new Response when resolved is a real ResponseInit object.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 25d3f5d5-4beb-4193-b5dd-cd09db55d827
📒 Files selected for processing (2)
.changeset/fix-head-request-server-route-fallback.mdpackages/start-server-core/src/createStartHandler.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/fix-head-request-server-route-fallback.md
| if (isHeadFallback) { | ||
| const resolved = await handleRedirectResponse( | ||
| ctx.response as Response, | ||
| request, | ||
| getRouter, | ||
| ) | ||
| return new Response(null, resolved) |
There was a problem hiding this comment.
Don't synthesize a 200 OK when no response was produced.
Line 946 forces ctx.response to Response, but if a fallback GET/ANY chain exits without returning or calling next, handleRedirectResponse passes undefined through and Line 950 turns that into a fresh 200 OK. That hides the original handler bug and makes a broken HEAD route look successful.
Suggested fix
if (isHeadFallback) {
+ if (!ctx.response) {
+ throwRouteHandlerError()
+ }
+
const resolved = await handleRedirectResponse(
- ctx.response as Response,
+ ctx.response,
request,
getRouter,
)
return new Response(null, resolved)
}As per coding guidelines, **/*.{ts,tsx}: Use TypeScript strict mode with extensive type safety.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/start-server-core/src/createStartHandler.ts` around lines 944 - 950,
The current isHeadFallback branch forces ctx.response through
handleRedirectResponse and always wraps its result into new Response(...), which
converts an undefined/absent response into a synthetic 200 OK; change the logic
in the isHeadFallback block (the call to handleRedirectResponse with
ctx.response, request, getRouter) to check the returned value (from
handleRedirectResponse) and if it is undefined/null do not create a new
Response—propagate undefined (or return early) instead; update types so the
variable (e.g., resolved) is typed as ResponseInit | undefined and the code only
calls new Response when resolved is a real ResponseInit object.
There was a problem hiding this comment.
this makes sense. please incorporate
There was a problem hiding this comment.
@schiller-manuel, here is the fix incorporated:
- Guard against
ctx.responsebeingundefined/falsy before passing it tohandleRedirectResponse - Remove the unsafe
as Responsecast
Why this matters:
Without the guard, if a fallback GET/ANY handler exits without returning a Response (or calling next), ctx.response is undefined. handleRedirectResponse passes undefined straight through, and new Response(null, undefined) quietly synthesizes a 200 OK — masking the bug entirely. Calling throwRouteHandlerError() instead surfaces the developer mistake with the same clear message used everywhere else in the file.
Failed to handle agent chat message. Please try again.
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nx Cloud has identified a possible root cause for your failed CI:
We've identified the failure in tanstack-solid-start-e2e-serialization-adapters:test:e2e as an environment issue unrelated to this PR — the error originates in a pre-built e2e-utils dist artifact (localDummyServer.js) that is missing the users export, and none of our changes touch that package or the solid-start serialization suite. This appears to be a stale CI build artifact that needs to be rebuilt in the environment.
No code changes were suggested for this issue.
You can trigger a rerun by pushing an empty commit:
git commit --allow-empty -m "chore: trigger rerun"
git push
🎓 Learn more about Self-Healing CI on nx.dev
2 participants
Fixes #7270.
RFC 9110 §9.3.2 requires HEAD responses to match GET in headers and status, with no body. When a route has no explicit
HEADhandler, the server falls back to GET and strips the body.What changed
HEAD → GET → ANY. PreviouslyANYbeatGET, so a route with both a GET and an ANY handler would skip the GET for HEAD requests entirely — wrong per RFC 9110.RedirectResponseto a plainResponsebeforehandleRedirectResponsecould process it. TheLocationheader was lost as a result. The fix resolves the redirect first, then strips.statusTextis now copied to the stripped response.E2E coverage
Three new Playwright tests in
e2e/react-start/server-routes:X-HANDLER: ANYandX-METHOD: HEADpreservedTest plan
pnpm testinpackages/start-server-coree2e/react-start/server-routesSummary by CodeRabbit
New Features
Bug Fixes
Tests