fix(backend): harden FAPI proxy resilience and spec compliance#8163
fix(backend): harden FAPI proxy resilience and spec compliance#8163
Conversation
…ipping, and DELETE body support - Propagate client abort signal to upstream fetch to prevent zombie requests - Strip dynamic hop-by-hop headers listed in the Connection header (RFC 7230) - Support request bodies on DELETE (and any method), not just POST/PUT/PATCH - Add Cache-Control: no-store to error responses to prevent CDN caching - Only set duplex option when request has a body Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 7ac51f5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 packages
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 |
@clerk/agent-toolkit
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/dev-cli
@clerk/expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/hono
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/react
@clerk/react-router
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/ui
@clerk/upgrade
@clerk/vue
commit: |
|
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:
📝 WalkthroughWalkthroughProxy header handling switched from an array to a 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 @.changeset/tough-ghosts-ask.md:
- Line 5: The changeset summary contains a spelling mistake: replace the
misspelled token "aobrt" with "abort" in the summary sentence (the phrase
"adding support for aobrt signals" should read "adding support for abort
signals") so the release notes/changelog shows the correct word; update the
summary text in the changeset file where that token appears.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: a8f5d12c-abae-4778-9f39-edecdb62d566
📒 Files selected for processing (1)
.changeset/tough-ghosts-ask.md
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/backend/src/proxy.ts (1)
320-346:⚠️ Potential issue | 🟠 MajorDon't restore
Locationafter stripping dynamic hop-by-hop headers.Line 320 strips headers named by the upstream
Connectionheader, but Lines 338-346 can addLocationback fromresponse.headers. If FAPI ever sendsConnection: location, this still forwards a hop-by-hop response header and breaks the RFC 7230 behavior this change is adding.🐛 Proposed fix
- if (locationHeader) { + if (locationHeader && !responseDynamicHopByHop.has('location')) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/backend/src/proxy.ts` around lines 320 - 346, The Location rewrite logic can re-add a hop-by-hop header if the upstream Connection header listed "location"; update the Location handling in the response rewrite to check the same dynamic hop-by-hop set before restoring or setting Location: use responseDynamicHopByHop (from getDynamicHopByHopHeaders(response.headers)) and the same lowercase membership tests (HOP_BY_HOP_HEADERS and RESPONSE_HEADERS_TO_STRIP) to skip rewriting/setting Location when it was listed as a dynamic hop-by-hop header; ensure you consult response.headers and lower-case the key like the earlier loop before calling responseHeaders.set('Location', ...).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/backend/src/proxy.ts`:
- Around line 320-346: The Location rewrite logic can re-add a hop-by-hop header
if the upstream Connection header listed "location"; update the Location
handling in the response rewrite to check the same dynamic hop-by-hop set before
restoring or setting Location: use responseDynamicHopByHop (from
getDynamicHopByHopHeaders(response.headers)) and the same lowercase membership
tests (HOP_BY_HOP_HEADERS and RESPONSE_HEADERS_TO_STRIP) to skip
rewriting/setting Location when it was listed as a dynamic hop-by-hop header;
ensure you consult response.headers and lower-case the key like the earlier loop
before calling responseHeaders.set('Location', ...).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: ad9397c6-e66f-4599-85cc-262daab25aac
📒 Files selected for processing (1)
packages/backend/src/proxy.ts
|
|
||
| try { | ||
| // Make the proxied request | ||
| // TODO: Consider adding AbortSignal.timeout(30_000) via AbortSignal.any() |
There was a problem hiding this comment.
AbortSignal.timeout was added in Node 17. What other backend runtime are we waiting for support for?
There was a problem hiding this comment.
Good Q! Seems like it's also supported in CF workers as well, so I don't think we need to worry too much about runtime compat.
I'll handle the generic top-level timeout in a follow-up
packages/backend/src/proxy.ts
Outdated
| if (hasBody && request.body) { | ||
| // Only set duplex when body is present (required for streaming bodies) | ||
| if (hasBody) { | ||
| // @ts-expect-error - duplex is required for streaming bodies but not in all TS definitions |
There was a problem hiding this comment.
which definitions is it not in?
There was a problem hiding this comment.
It's not present on the RequestInit type, I will clarify 👀
| if (!HOP_BY_HOP_HEADERS.includes(lower) && !RESPONSE_HEADERS_TO_STRIP.includes(lower)) { | ||
| if ( | ||
| !HOP_BY_HOP_HEADERS.has(lower) && | ||
| !RESPONSE_HEADERS_TO_STRIP.includes(lower) && |
There was a problem hiding this comment.
[nit] we could also convert RESPONSE_HEADERS_TO_STRIP to a set for the .has() method. But super super minor
| * header names (RFC 7230 Section 6.1). These headers are specific to the | ||
| * current connection and must not be forwarded by proxies. | ||
| */ | ||
| function getDynamicHopByHopHeaders(headers: Headers): Set<string> { |
There was a problem hiding this comment.
We could probably add a unit test for this
jacekradko
left a comment
jacekradko
left a comment
There was a problem hiding this comment.
Just some nits, but overall this is 🔥
3 participants
Summary
fetch()to prevent zombie requests when clients disconnectConnectionheader per RFC 7230 Section 6.1, for both request and response header copyingrequest.body !== nullinstead of a method allowlistCache-Control: no-storeto all error responses to prevent CDN/browser caching of transient errorsduplex: 'half'when the request actually has a body, avoiding unnecessary option on bodyless requestsHOP_BY_HOP_HEADERSfrom array toSetfor O(1) lookupsTest plan
duplex: 'half'Cache-Control: no-storeConnectionheader are stripped from forwarded requests🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
Chores