diff --git a/src/common/net/fetch-client.spec.ts b/src/common/net/fetch-client.spec.ts index a4c283483..b6aeb6588 100644 --- a/src/common/net/fetch-client.spec.ts +++ b/src/common/net/fetch-client.spec.ts @@ -1122,6 +1122,56 @@ describe('request timeout covers the response body (GH-1679)', () => { expect(requestCount).toBe(1); }); + it('does not follow a redirect, so the request body is never re-sent', async () => { + handler = (req, res) => { + if (req.url === '/stolen') { + res.writeHead(200, { 'content-type': 'application/json' }); + res.end('{}'); + return; + } + res.writeHead(307, { location: `${baseURL}/stolen` }); + res.end(); + }; + // Default timeout: the shared 100ms one can retry a slow first request. + const client = new FetchHttpClient(baseURL, { maxRetries: 0 }, fetchImpl); + + await expect( + client.post( + '/user_management/authenticate', + { client_secret: 'sk_test' }, + {}, + ), + ).rejects.toMatchObject({ + name: 'HttpClientError', + message: expect.stringContaining(`redirect (307 to ${baseURL}/stolen)`), + response: { status: 307 }, + }); + expect(requestCount).toBe(1); + }); + + it('releases the connection when a redirect body stalls instead of waiting on it', async () => { + let connectionClosed!: Promise; + handler = (_req, res) => { + connectionClosed = new Promise((resolve) => + res.on('close', () => resolve()), + ); + res.writeHead(307, { location: `${baseURL}/elsewhere` }); + res.write(''); + // Hold the body open; the client must not read it. + }; + const client = new FetchHttpClient(baseURL, { maxRetries: 0 }, fetchImpl); + + await expect(client.get('/users', {})).rejects.toMatchObject({ + name: 'HttpClientError', + message: expect.stringContaining('redirect (307'), + response: { status: 307 }, + }); + // The request was aborted, so the server sees the connection drop + // rather than the SDK sitting on the deadline. + await connectionClosed; + expect(requestCount).toBe(1); + }); + it('does not retry a successful body that times out, even when retries are enabled', async () => { stallBody(200); const { client } = createClient(2); diff --git a/src/common/net/fetch-client.ts b/src/common/net/fetch-client.ts index 1b76c4f41..888d4ff45 100644 --- a/src/common/net/fetch-client.ts +++ b/src/common/net/fetch-client.ts @@ -206,6 +206,8 @@ export class FetchHttpClient extends HttpClient implements HttpClientInterface { }); // Set once the headers arrive. let res: Response | undefined; + // Set when the response is a redirect, whose body is never read. + let redirected = false; try { res = await this._fetchFn(url, { @@ -219,8 +221,36 @@ export class FetchHttpClient extends HttpClient implements HttpClientInterface { }, body: requestBody, signal: abortController.signal, + // The API never redirects. Following one would re-send the body (which + // can carry client_secret) to the redirect target. 'manual' rather than + // 'error': Cloudflare workerd rejects 'error'. + redirect: 'manual', }); + // Browsers report a manual redirect as an opaque response with status 0. + if ( + (res.status >= 300 && res.status < 400) || + res.type === 'opaqueredirect' + ) { + clearTimeout(timeoutId); + redirected = true; + const location = res.headers.get('Location'); + // An opaque redirect hides both the status and the Location. + const what = + res.type === 'opaqueredirect' + ? 'an opaque redirect' + : `a redirect (${res.status}${location ? ` to ${location}` : ''})`; + const message = `WorkOS API responded with ${what}. Redirects are not followed; check the apiHostname, port and https options.`; + throw new HttpClientError({ + message, + response: { + status: res.status, + headers: res.headers, + data: { message }, + }, + }); + } + if (!res.ok) { const requestID = res.headers.get('X-Request-ID') ?? ''; // Read the error body under the same deadline, inside the attempt, @@ -288,6 +318,16 @@ export class FetchHttpClient extends HttpClient implements HttpClientInterface { } throw error; + } finally { + // Release the unread redirect body and its connection. Reading it under + // the deadline would turn a stalled body into a retried 408 that hides + // the redirect, and `body.cancel()` only exists on web streams, whereas + // every fetch implementation honors the signal. This runs after the + // catch above has already checked the signal, so the abort is not + // reported as a timeout. + if (redirected) { + abortController.abort(); + } } } diff --git a/src/factory.spec.ts b/src/factory.spec.ts index 311be4005..ca00786f9 100644 --- a/src/factory.spec.ts +++ b/src/factory.spec.ts @@ -73,6 +73,21 @@ describe('createWorkOS', () => { expect(workos.baseURL).toBeDefined(); }); + it('uses the provided fetchFn', async () => { + const fetchFn = jest.fn( + async () => + new Response('{}', { + status: 200, + headers: { 'content-type': 'application/json' }, + }), + ); + const workos = createWorkOS({ apiKey: 'sk_test_123', fetchFn }); + + await workos.get('/path'); + + expect(fetchFn).toHaveBeenCalledTimes(1); + }); + it('has access to all services', () => { const workos = createWorkOS({ apiKey: 'sk_test_123', diff --git a/src/index.worker.ts b/src/index.worker.ts index f1b0b4bbb..0604e6f6f 100644 --- a/src/index.worker.ts +++ b/src/index.worker.ts @@ -2,9 +2,6 @@ import { Actions } from './actions/actions'; import { CryptoProvider } from './common/crypto/crypto-provider'; import { SubtleCryptoProvider } from './common/crypto/subtle-crypto-provider'; -import { FetchHttpClient } from './common/net/fetch-client'; -import { HttpClient } from './common/net/http-client'; -import { WorkOSOptions } from './index.worker'; import { Webhooks } from './webhooks/webhooks'; import { WorkOS } from './workos'; @@ -34,33 +31,6 @@ export { } from './factory'; class WorkOSWorker extends WorkOS { - /** @override */ - createHttpClient(options: WorkOSOptions, userAgent: string): HttpClient { - const headers: Record = { - 'User-Agent': userAgent, - }; - - const configHeaders = options.config?.headers; - if ( - configHeaders && - typeof configHeaders === 'object' && - !Array.isArray(configHeaders) && - !(configHeaders instanceof Headers) - ) { - Object.assign(headers, configHeaders); - } - - if (this.key) { - headers['Authorization'] = `Bearer ${this.key}`; - } - - return new FetchHttpClient(this.baseURL, { - ...options.config, - maxRetries: options.maxRetries, - headers, - }); - } - /** @override */ createWebhookClient(): Webhooks { return new Webhooks(this); diff --git a/src/workos.spec.ts b/src/workos.spec.ts index 2e61a1107..ec4c0e171 100644 --- a/src/workos.spec.ts +++ b/src/workos.spec.ts @@ -391,6 +391,56 @@ describe('WorkOS', () => { }, 20000); }); + describe('when the api responds with a redirect', () => { + it('throws a GenericServerException that names the redirect', async () => { + const fetchFn = jest.fn( + async () => + new Response(null, { + status: 301, + headers: { Location: 'https://api.workos.com/path' }, + }), + ); + const workos = new WorkOS('sk_test_Sz3IQjepeSWaI4cMS4ms4sMuU', { + fetchFn, + }); + + await expect(workos.post('/path', {})).rejects.toMatchObject({ + name: 'GenericServerException', + status: 301, + message: expect.stringContaining( + 'redirect (301 to https://api.workos.com/path)', + ), + }); + expect(fetchFn).toHaveBeenCalledTimes(1); + }); + + it('throws a GenericServerException for an opaque redirect from a browser fetch', async () => { + // Browsers hide the status and Location of a manual redirect behind + // an opaque response. The Response constructor rejects status 0, so + // this is a bare fake. + const fetchFn = jest.fn( + async () => + ({ + type: 'opaqueredirect', + status: 0, + ok: false, + headers: new Headers(), + body: null, + }) as unknown as Response, + ); + const workos = new WorkOS('sk_test_Sz3IQjepeSWaI4cMS4ms4sMuU', { + fetchFn, + }); + + await expect(workos.post('/path', {})).rejects.toMatchObject({ + name: 'GenericServerException', + status: 0, + message: expect.stringContaining('an opaque redirect'), + }); + expect(fetchFn).toHaveBeenCalledTimes(1); + }); + }); + describe('when the api responds with a 500 and no error/error_description', () => { it('throws an GenericServerException', async () => { fetchOnce( @@ -780,6 +830,43 @@ describe('WorkOS', () => { ).toBeInstanceOf(SubtleCryptoProvider); }); + it('uses the provided fetchFn', async () => { + const fetchFn = jest.fn( + async () => + new Response('{}', { + status: 200, + headers: { 'content-type': 'application/json' }, + }), + ); + const workos = new WorkOSWorker('sk_test_key', { fetchFn }); + + await workos.get('/path'); + + expect(fetchFn).toHaveBeenCalledTimes(1); + }); + + it('honors the timeout option', async () => { + const neverResolvingFetch = jest.fn( + (_url: any, init: any) => + new Promise((_resolve, reject) => { + init?.signal?.addEventListener('abort', () => { + const abortError = new Error('Aborted'); + abortError.name = 'AbortError'; + reject(abortError); + }); + }) as any, + ); + const workos = new WorkOSWorker('sk_test_key', { + timeout: 1, + maxRetries: 0, + fetchFn: neverResolvingFetch as any, + }); + + await expect(workos.post('/path', {})).rejects.toMatchObject({ + status: 408, + }); + }); + it('uses console.warn to emit warnings', () => { const workos = new WorkOSWorker('sk_test_key'); const warnSpy = jest.spyOn(console, 'warn'); diff --git a/src/workos.ts b/src/workos.ts index 72154aada..cec87ef11 100644 --- a/src/workos.ts +++ b/src/workos.ts @@ -202,12 +202,16 @@ export class WorkOS { headers['Authorization'] = `Bearer ${this.key}`; } - return new FetchHttpClient(this.baseURL, { - ...options.config, - timeout: options.timeout, - maxRetries: options.maxRetries, - headers, - }) as HttpClient; + return new FetchHttpClient( + this.baseURL, + { + ...options.config, + timeout: options.timeout, + maxRetries: options.maxRetries, + headers, + }, + options.fetchFn, + ) as HttpClient; } get version() {