Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 470
fix(backend): harden FAPI proxy resilience and spec compliance#8163
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
e7346d828094481fbdcef545a88cccf5a79043272d033084c7ac51f5eff9db8File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| '@clerk/backend': patch | ||
| --- | ||
| Improve the built-in Clerk Frontend API proxy, adding support for abort signals and addressing a number of small edge cases. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -43,7 +43,7 @@ export interface ProxyError { | ||
| } | ||
| // Hop-by-hop headers that should not be forwarded | ||
| const HOP_BY_HOP_HEADERS = [ | ||
| const HOP_BY_HOP_HEADERS = new Set([ | ||
| 'connection', | ||
| 'keep-alive', | ||
| 'proxy-authenticate', | ||
| @@ -52,14 +52,32 @@ const HOP_BY_HOP_HEADERS = [ | ||
| 'trailer', | ||
| 'transfer-encoding', | ||
| 'upgrade', | ||
| ]; | ||
| ]); | ||
| /** | ||
| * Parses the Connection header to extract dynamically-nominated hop-by-hop | ||
| * 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> { | ||
| const connectionValue = headers.get('connection'); | ||
| if (!connectionValue) { | ||
| return new Set(); | ||
| } | ||
| return new Set( | ||
| connectionValue | ||
| .split(',') | ||
| .map(h => h.trim().toLowerCase()) | ||
| .filter(h => h.length > 0), | ||
| ); | ||
| } | ||
| // Headers to strip from proxied responses. fetch() auto-decompresses | ||
| // response bodies, so Content-Encoding no longer describes the body | ||
| // and Content-Length reflects the compressed size. We request identity | ||
| // encoding upstream to avoid the double compression pass, but strip | ||
| // these defensively since servers may ignore Accept-Encoding: identity. | ||
| const RESPONSE_HEADERS_TO_STRIP = ['content-encoding', 'content-length']; | ||
| const RESPONSE_HEADERS_TO_STRIP = new Set(['content-encoding', 'content-length']); | ||
| /** | ||
| * Derives the Frontend API URL from a publishable key. | ||
| @@ -114,6 +132,7 @@ function createErrorResponse(code: ProxyErrorCode, message: string, status: numb | ||
| status, | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| 'Cache-Control': 'no-store', | ||
| }, | ||
| }); | ||
| } | ||
| @@ -230,9 +249,12 @@ export async function clerkFrontendApiProxy(request: Request, options?: Frontend | ||
| // Build headers for the proxied request | ||
| const headers = new Headers(); | ||
| // Copy original headers, excluding hop-by-hop headers | ||
| // Copy original headers, excluding hop-by-hop headers and any | ||
| // dynamically-nominated hop-by-hop headers listed in the Connection header (RFC 7230 Section 6.1). | ||
| const dynamicHopByHop = getDynamicHopByHopHeaders(request.headers); | ||
| request.headers.forEach((value, key) => { | ||
| if (!HOP_BY_HOP_HEADERS.includes(key.toLowerCase())) { | ||
| const lower = key.toLowerCase(); | ||
| if (!HOP_BY_HOP_HEADERS.has(lower) && !dynamicHopByHop.has(lower)) { | ||
| headers.set(key, value); | ||
| } | ||
| }); | ||
| @@ -270,31 +292,39 @@ export async function clerkFrontendApiProxy(request: Request, options?: Frontend | ||
| headers.set('X-Forwarded-For', clientIp); | ||
| } | ||
| // Determine if request has a body | ||
| const hasBody = ['POST', 'PUT', 'PATCH'].includes(request.method); | ||
| // Determine if request has a body (handles DELETE-with-body and any other method) | ||
| const hasBody = request.body !== null; | ||
| try { | ||
| // Make the proxied request | ||
| // TODO: Consider adding AbortSignal.timeout(30_000) via AbortSignal.any() | ||
Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
MemberAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 | ||
| const fetchOptions: RequestInit = { | ||
| method: request.method, | ||
| headers, | ||
| redirect: 'manual', | ||
| // @ts-expect-error - duplex is required for streaming bodies but not in all TS definitions | ||
| duplex: hasBody ? 'half' : undefined, | ||
| signal: request.signal, | ||
| }; | ||
| // Only include body for methods that support it | ||
| 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 present on the RequestInit type from undici | ||
| fetchOptions.duplex = 'half'; | ||
| fetchOptions.body = request.body; | ||
| } | ||
| const response = await fetch(targetUrl.toString(), fetchOptions); | ||
| // Build response headers, excluding hop-by-hop and encoding headers | ||
| // Build response headers, excluding hop-by-hop and encoding headers. | ||
| // Also strip dynamically-nominated hop-by-hop headers from the response Connection header. | ||
| const responseDynamicHopByHop = getDynamicHopByHopHeaders(response.headers); | ||
| const responseHeaders = new Headers(); | ||
| response.headers.forEach((value, key) => { | ||
| const lower = key.toLowerCase(); | ||
| if (!HOP_BY_HOP_HEADERS.includes(lower) && !RESPONSE_HEADERS_TO_STRIP.includes(lower)) { | ||
| if ( | ||
| !HOP_BY_HOP_HEADERS.has(lower) && | ||
| !RESPONSE_HEADERS_TO_STRIP.has(lower) && | ||
| !responseDynamicHopByHop.has(lower) | ||
| ) { | ||
| if (lower === 'set-cookie') { | ||
| responseHeaders.append(key, value); | ||
| } else { | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could probably add a unit test for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
covered implicitly in this test case:
strips dynamic hop-by-hop headers listed in the Connection header from requests