Repository navigation
fix: don't follow redirects on WorkOS API requests - #511
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). Pass a fetchFn that sets redirect: 'manual' so a 3xx surfaces as an SDK error instead. Closes #510
[Medium impact] The PR appears safe to merge, within its documented Node-only SDK coverage. SummaryThis PR stops WorkOS API requests from following redirects.
No actionable new issues were found. The documented Edge/workerd limitation still requires the SDK fix. Reviews (3) · Last reviewed commit: "fix: name redirects in the error and sto..." · Reviewed by Greptile |
gjtorikian
left a comment
There was a problem hiding this comment.
Thanks for this. The 'manual' over 'error' reasoning checks out against the SDK's shouldRetryRequest: a 3xx isn't in RETRY_STATUS_CODES, so the request fails fast without a retry, whereas the TypeError from 'error' would have been retried with backoff.
Requesting changes for one gap in the same class, plus one change to make the failure legible.
1. fetchClaimNonce still follows redirects
authkit-nextjs/src/get-authorization-url.ts
Lines 7 to 16 in fd4dff1
This goes through the global fetch, not the SDK client, so the new fetchFn never applies. It POSTs client_id and claim_token to the same configured API origin with the default redirect: 'follow', so a 307/308 re-posts WORKOS_CLAIM_TOKEN to the redirect target exactly as #510 describes for client_secret.
One line closes it. The existing !response.ok branch already handles the 3xx, since 307 is not ok. (GitHub won't let me attach an inline suggestion to a file outside the diff, hence the snippet.)
const response = await fetch(`${baseURL}/x/one-shot-environments/claim-nonces`, {
method: 'POST',
redirect: 'manual',
headers: { 'Content-Type': 'application/json' },2. Throw a named error on 3xx from the wrapper
Inline on src/workos.ts. Without it a misconfigured host (e.g. WORKOS_API_HTTPS=false hitting the 301 from api.workos.com) surfaces as a bare ParseError on every SDK version in the peer range.
| // The WorkOS API never redirects; following one would re-POST client_secret to the redirect target. | ||
| // 'manual' rather than 'error': Cloudflare workerd rejects 'error', and the SDK retries the TypeError it throws. | ||
| // The SDK's worker build (Edge, workerd) ignores fetchFn, so those runtimes need the fix in @workos-inc/node. | ||
| fetchFn: ((input, init) => globalThis.fetch(input, { ...init, redirect: 'manual' })) satisfies typeof fetch, |
There was a problem hiding this comment.
With redirect: 'manual' the 3xx lands in the SDK's !res.ok branch, JSON.parse fails on the empty or HTML body, and the caller gets a bare ParseError that never mentions a redirect. The clear message from workos/workos-node#1752 is unmerged and will never reach peers 9 through 12. The most likely real hit is WORKOS_API_HTTPS=false against api.workos.com, which answers 301 and used to work silently for GETs.
Suggest having the wrapper throw a plain Error that names the status and Location. A plain Error (not a TypeError) is rethrown as-is by fetchRequestWithRetry without a retry, so it still fails fast. opaqueredirect covers runtimes that follow the WHATWG filtered-response behavior.
| // The WorkOS API never redirects; following one would re-POST client_secret to the redirect target. | |
| // 'manual' rather than 'error': Cloudflare workerd rejects 'error', and the SDK retries the TypeError it throws. | |
| // The SDK's worker build (Edge, workerd) ignores fetchFn, so those runtimes need the fix in @workos-inc/node. | |
| fetchFn: ((input, init) => globalThis.fetch(input, { ...init, redirect: 'manual' })) satisfies typeof fetch, | |
| // The WorkOS API never redirects; following one would re-POST client_secret to the redirect target. | |
| // 'manual' rather than 'error': Cloudflare workerd rejects 'error', and the SDK retries the TypeError it throws. | |
| // A 3xx is thrown as a plain Error (not a TypeError) so the SDK surfaces it without retrying. | |
| // The SDK's worker build (Edge, workerd) ignores fetchFn, so those runtimes need the fix in @workos-inc/node. | |
| fetchFn: (async (input, init) => { | |
| const res = await globalThis.fetch(input, { ...init, redirect: 'manual' }); | |
| if (res.type === 'opaqueredirect' || (res.status >= 300 && res.status < 400)) { | |
| const location = res.headers.get('location'); | |
| throw new Error( | |
| `WorkOS API responded with a redirect (${res.status}${location ? ` to ${location}` : ''}). ` + | |
| 'Redirects are not followed; check WORKOS_API_HOSTNAME, WORKOS_API_PORT and WORKOS_API_HTTPS.', | |
| ); | |
| } | |
| return res; | |
| }) satisfies typeof fetch, |
The new test can then tighten rejects.toThrow() to rejects.toThrow('responded with a redirect') so an unrelated throw can't mask a regression.
There was a problem hiding this comment.
Applied as suggested in 516871c, and the test now asserts toThrow('WorkOS API responded with a redirect (307 to https://evil.example/)'). Note the SDK wraps non-HttpClientError throws, so callers see Unexpected error: Error: WorkOS API responded with a redirect ....
The fetchFn wrapper now throws a plain Error naming the status and Location on a 3xx, so a misconfigured host doesn't surface as a bare ParseError on SDK 9 to 12. A plain Error isn't retried by the SDK. fetchClaimNonce uses the global fetch rather than the SDK client, so it still followed redirects and could re-post WORKOS_CLAIM_TOKEN. It now sets redirect: 'manual'; a 3xx falls into the existing !response.ok branch.
|
Thanks @gjtorikian, both addressed in 516871c:
Re-ran the local repro (fake API returning a cross-origin 307): |
Closes #510
What
getWorkOS()now passes the WorkOS client afetchFnthat setsredirect: 'manual', so API requests don't follow redirects.Why
authenticateWithCode(callback) andauthenticateWithRefreshToken(session refresh) POST a body containingclient_secret. With fetch's defaultredirect: 'follow', a 307/308 from the API origin re-posts that body to the redirect target. TheAuthorizationheader is dropped cross-origin; the body is not.Reproduced locally with a fake API (
WORKOS_API_HOSTNAME) that returns a cross-origin 307 and a sink that records requests:{"grant_type":"refresh_token","client_id":"client_123","client_secret":"sk_test_SECRET",...}.getUserresolves with the sink's response.ParseErrorand doesn't retry.'manual'rather than'error': Cloudflare workerd rejectsredirect: 'error'outright, and on other runtimes it rejects with aTypeError, which the SDK retries with backoff.Scope / limitation
This only applies where the SDK's Node entry is loaded: route handlers (the callback) and Next 16
proxy.ts. Under theedge-light/workerdconditions (Edge middleware on Next 15 and earlier, Cloudflare/OpenNext),@workos-inc/node's worker build ignoresfetchFn, so it needs the SDK fix in workos/workos-node#1752. Once that ships, this change is redundant but harmless, and it still covers users on older SDK versions in our peer range (9 to 12, all of which honorfetchFnon the Node entry).Compatibility
No change against the real API, which doesn't redirect. If
WORKOS_API_HOSTNAMEpoints at something that answers with a redirect, requests now fail instead of being re-sent to the redirect target.Checklist
pnpm testpasses (new test insrc/workos.spec.ts; fails without the change)pnpm run buildsucceedspnpm run lintpassesoxfmt --check srcpasses