Repository navigation
Conversation
authenticateWithCode and authenticateWithRefreshToken POST a body containing client_secret. With fetch's default redirect: 'follow', a 307/308 from the API origin re-posts that body to the redirect target (the Authorization header is dropped cross-origin, the body is not). Set redirect: 'manual' so a 3xx fails through the existing error path. Also pass fetchFn through in the worker build and the base WorkOS class (used by createWorkOS). Both ignored it, so a custom fetch was silently dropped on Edge, Cloudflare Workers and createWorkOS clients.
4 tasks done
Contributor
[High impact] The PR appears safe to merge; the previous connection-cleanup issue is fixed. SummaryAPI requests now reject redirects instead of forwarding request bodies.
The previous finding is fixed. No new actionable issues were found. Tests were inspected but not run. Reviews (3) · Last reviewed commit: "fix: release the connection on a redirec..." · Reviewed by Greptile |
…lient A 3xx (or a browser opaqueredirect) now throws an HttpClientError that names the status and Location, surfacing as a GenericServerException, instead of a ParseError from reading an HTML body as JSON. WorkOSWorker's createHttpClient was a copy of the base class method minus timeout. Remove it so the worker build uses the base method, which also makes the timeout option work on Edge and Workers.
The redirect branch threw with the response body still open. On Node that pins the connection until garbage collection, and a proxy that sends redirect headers but stalls the body would hold it indefinitely. Abort the request's controller in a `finally` once the catch has already checked the signal, so the abort releases the socket on every fetch implementation (node-fetch bodies have no `cancel()`) without being reported as a timeout. Reading the body instead would turn a stall into a retried 408 that hides the redirect. A browser reports a manual redirect as an opaque response with status 0 and no headers, which read as "redirect (0)". Say "an opaque redirect" instead, and cover that branch with a test.
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.
Description
API requests follow redirects today (fetch's default
redirect: 'follow').authenticateWithCodeandauthenticateWithRefreshTokensendclient_secretin the request body, so if the API origin (or a configuredapiHostname) ever answers with a 307/308, the SDK re-posts the body, secret included, to wherever the redirect points. TheAuthorizationheader is dropped cross-origin; the body is not. GET responses from the redirect target are also accepted as if they came from the API. Reported against authkit-nextjs in workos/authkit-nextjs#510.Changes
FetchHttpClientsetsredirect: 'manual', so redirects are never followed. I used'manual'rather than'error'because Cloudflare workerd rejects'error'("Invalid redirect value..."), and'error'rejects with aTypeError, whichshouldRetryRequestretries.opaqueredirect) throws anHttpClientErrorthat names the status andLocation, so callers get aGenericServerExceptionlikeWorkOS API responded with a redirect (301 to https://...). Redirects are not followed; check the apiHostname, port and https options.Without this, the HTML redirect body would be parsed as JSON and fail with a confusingParseError. 3xx isn't inRETRY_STATUS_CODES, so it fails right away without retrying.WorkOSclass (used bycreateWorkOS) now passesoptions.fetchFntoFetchHttpClient. It silently ignored it before.WorkOSWorker.createHttpClient(used under theworkerd/edge-lightconditions). It was a copy of the base method minustimeout, and it also ignoredfetchFn. The worker build now uses the base method, so bothfetchFnandtimeoutwork on Edge and Workers.Verification
New real-HTTP test (native fetch and node-fetch): a 307 to
/stolenisn't followed, the error names the redirect, and the request count stays at 1.New tests: a 301 surfaces as a
GenericServerExceptionnaming the redirect; the worker client andcreateWorkOSuse the providedfetchFn; the worker client respectstimeout. All of them fail without the source changes.Repro against the built
lib/with a fake API returning a cross-origin 307 and a sink that records requests:index.mjs(Node)client_secretindex.worker.mjson Nodeclient_secretindex.worker.mjson workerd (wrangler dev)client_secretfetchFnis callednpm test,npm run typecheck,npm run prettierandnpm run build(attw) pass.Compatibility: no change against the real API, which doesn't redirect. Setups that point
apiHostnameat something that answers with a redirect now get an error that says so, instead of having the request silently re-sent. One real case:https: falseagainstapi.workos.com, which answers301to https. GET calls used to follow that; now they fail with the redirect error.Note: the existing GH-1679 real-HTTP timeout tests (
times out a successful JSON body...,surfaces a stalled error body...) flake locally onmainas well. They use a 100ms timeout. The new redirect test uses the default timeout so it doesn't hit the same problem.Documentation
Does this require changes to the WorkOS Docs? E.g. the API Reference or code snippets need updates.