diff --git a/.changeset/issue-79-idempotent-callback.md b/.changeset/issue-79-idempotent-callback.md new file mode 100644 index 00000000..bebd423b --- /dev/null +++ b/.changeset/issue-79-idempotent-callback.md @@ -0,0 +1,21 @@ +--- +"@aws-blocks/bb-auth-oidc": patch +--- + +fix(bb-auth-oidc): make `handleRedirectCallback()` idempotent under double invocation + +`handleRedirectCallback()` consumed the single-use PKCE pending entry from +`sessionStorage` and only removed it **after** the `/aws-blocks/auth/exchange` +round-trip. A second concurrent invocation — most commonly React StrictMode's +mount → unmount → mount, which fires the callback effect twice synchronously — +either replayed the already-consumed code (failing the second exchange) or +found the pending entry gone and resolved `null`, stranding the app on a +signed-out screen despite a successful sign-in. + +The callback now guards on an in-flight promise keyed by the PKCE `code`: +concurrent/duplicate invocations for the same code share the first call's +promise instead of starting a second exchange, so both callers resolve to the +same user and subscribers are notified exactly once. The pending entry is also +consumed up front (before the network round-trip) so a late duplicate can't +replay it, and the guard is released once the exchange settles so a genuinely +new sign-in flow on the same page is never blocked. diff --git a/packages/bb-auth-oidc/DESIGN.md b/packages/bb-auth-oidc/DESIGN.md index ddc13a3b..ea1c6c13 100644 --- a/packages/bb-auth-oidc/DESIGN.md +++ b/packages/bb-auth-oidc/DESIGN.md @@ -264,3 +264,34 @@ encode to ~43 characters, comfortably exceeding the floor. The contract is stated once here: `csrf` MUST be ≥32 characters; SDKs SHOULD generate 32 random bytes (≈43 base64url chars). Source files defer to this decision rather than restating the threshold in bytes. + +### D12 — `handleRedirectCallback` shares one in-flight exchange per `(exchangePath, code)` + +**Status:** Accepted. + +**Rationale.** A PKCE `code` is single-use and must be exchanged exactly once. A +double invocation — React StrictMode's mount → unmount → mount fires the callback +effect twice synchronously — would otherwise race to exchange the same code twice; +the loser finds the pending entry already consumed and resolves `null`, stranding +the app on a signed-out screen despite a successful sign-in. + +`_callbackInflight` (in `index.browser.ts`) is a **module-scoped** variable, so the +guard is shared across every `AuthOIDCClient` instance in the tab — it is not a +per-instance field. It is keyed on `(exchangePath, code)`: + +- **`code`** — distinct sign-in flows carry distinct single-use codes, so they get + distinct exchanges. Two instances in one app (the D4 admin-auth + customer-auth + scenario) share the default `exchangePath`, so their codes being different is the + only thing keeping their in-flight exchanges isolated. +- **`exchangePath`** — two clients configured with different exchange endpoints never + share each other's in-flight exchange, even if a code somehow collided. + +The guard is released in a `finally` once the exchange settles (success or failure), +so a genuinely new flow on the same page is never blocked. + +**Cross-reload fallback.** The module variable does **not** survive a real page +reload. Cross-reload coordination falls back to the up-front `sessionStorage` +removal in `_exchangeCallback`: the pending PKCE entry is consumed *before* the +network round-trip, so a late duplicate in a fresh page load (after the in-flight +guard is gone) finds no pending entry and resolves `null` rather than replaying the +code. diff --git a/packages/bb-auth-oidc/src/index.browser.test.ts b/packages/bb-auth-oidc/src/index.browser.test.ts index 2edcda2f..7af7d7fa 100644 --- a/packages/bb-auth-oidc/src/index.browser.test.ts +++ b/packages/bb-auth-oidc/src/index.browser.test.ts @@ -224,3 +224,158 @@ describe('AuthOIDCClient.handleRedirectCallback — return shape', () => { assert.strictEqual('iss' in lastExchangeBody, false, 'iss should be omitted, not sent as undefined'); }); }); + +describe('AuthOIDCClient.handleRedirectCallback — idempotency under double invocation', () => { + const STATE = 'state-dbl'; + const BARE_USER = { userId: 'iss:sub', username: 'alice', email: 'alice@example.invalid', provider: 'google' }; + let originalFetch: typeof globalThis.fetch; + + beforeEach(() => { + installBrowserGlobals(`http://localhost:3000/spa-callback?code=auth-code-dbl&state=${STATE}`); + process.env.BLOCKS_API_URL = 'http://localhost:3000/aws-blocks/api'; + store.set('__blocks_oidc_pending', JSON.stringify({ + provider: 'google', + verifier: 'v', + state: STATE, + nonce: 'n', + callbackUrl: 'http://localhost:3000/spa-callback', + appState: 'app-state', + })); + originalFetch = globalThis.fetch; + }); + + afterEach(() => { + globalThis.fetch = originalFetch; + delete process.env.BLOCKS_API_URL; + clearBrowserGlobals(); + }); + + test('concurrent double invocation shares one exchange and both resolve to the same user', async () => { + // React StrictMode mounts → unmounts → mounts, firing the callback effect + // twice synchronously. Count the exchange POSTs to prove the single-use + // PKCE code is exchanged exactly once and neither caller is stranded. + let exchangeCalls = 0; + globalThis.fetch = (async (_url: any, init?: any) => { + if (init?.method === 'POST') exchangeCalls++; + // Settle on a later tick so both calls are genuinely in flight together. + await new Promise((r) => setTimeout(r, 5)); + return { ok: true, json: async () => ({ user: BARE_USER }) }; + }) as unknown as typeof globalThis.fetch; + + const client = makeClient(); + let notifyCount = 0; + client.onAuthStateChange(() => { notifyCount++; }); + // onAuthStateChange fires synchronously on subscribe with the last-known + // state; capture that baseline so the assertion below measures only the + // callback-driven notify as a delta, independent of cross-test module state. + const notifyBaseline = notifyCount; + + // Fire twice WITHOUT awaiting the first — the double-mount race. + const [r1, r2] = await Promise.all([ + client.handleRedirectCallback(), + client.handleRedirectCallback(), + ]); + + assert.ok(r1, 'first call must resolve a user'); + assert.ok(r2, 'second (concurrent) call must resolve a user — not null/throw'); + assert.strictEqual(r1!.userId, 'iss:sub'); + assert.strictEqual(r2!.userId, 'iss:sub'); + assert.strictEqual(exchangeCalls, 1, 'single-use PKCE code must be exchanged exactly once'); + assert.strictEqual(notifyCount - notifyBaseline, 1, 'callback should notify subscribers exactly once'); + assert.strictEqual(store.get('__blocks_oidc_pending'), undefined, 'pending entry should be consumed'); + }); + + test('a sequential double invocation also shares the in-flight result', async () => { + // Same race, expressed as two calls captured before awaiting either. + let exchangeCalls = 0; + globalThis.fetch = (async (_url: any, init?: any) => { + if (init?.method === 'POST') exchangeCalls++; + await new Promise((r) => setTimeout(r, 5)); + return { ok: true, json: async () => ({ user: BARE_USER }) }; + }) as unknown as typeof globalThis.fetch; + + const client = makeClient(); + const p1 = client.handleRedirectCallback(); + const p2 = client.handleRedirectCallback(); + const r1 = await p1; + const r2 = await p2; + assert.strictEqual(r1!.userId, 'iss:sub'); + assert.strictEqual(r2!.userId, 'iss:sub'); + assert.strictEqual(exchangeCalls, 1, 'only one exchange for the shared in-flight code'); + }); + + test('releases the guard after settling so a fresh flow on the same page can run', async () => { + let exchangeCalls = 0; + globalThis.fetch = (async (_url: any, init?: any) => { + if (init?.method === 'POST') exchangeCalls++; + return { ok: true, json: async () => ({ user: BARE_USER }) }; + }) as unknown as typeof globalThis.fetch; + + const client = makeClient(); + const first = await client.handleRedirectCallback(); + assert.ok(first, 'first flow resolves'); + assert.strictEqual(exchangeCalls, 1); + + // Simulate a brand-new flow (new code/state + freshly stored pending blob). + installBrowserGlobals('http://localhost:3000/spa-callback?code=auth-code-2&state=state-2'); + process.env.BLOCKS_API_URL = 'http://localhost:3000/aws-blocks/api'; + store.set('__blocks_oidc_pending', JSON.stringify({ + provider: 'google', verifier: 'v', state: 'state-2', nonce: 'n', + callbackUrl: 'http://localhost:3000/spa-callback', + })); + + const second = await client.handleRedirectCallback(); + assert.ok(second, 'second independent flow resolves — guard released after the first settled'); + assert.strictEqual(exchangeCalls, 2, 'the second flow runs its own exchange'); + }); + + test('error path under concurrent double invocation rejects both callers identically and releases the guard', async () => { + // The guard must propagate ONE shared rejection to both callers and + // release on failure. Without this, a refactor that mishandled the shared + // rejection (stranding the page) or failed to release the guard (blocking + // a same-page retry) would keep the success-path tests green. + let exchangeCalls = 0; + globalThis.fetch = (async (_url: any, init?: any) => { + if (init?.method === 'POST') exchangeCalls++; + // Settle on a later tick so both calls are genuinely in flight together. + await new Promise((r) => setTimeout(r, 5)); + return { ok: false, json: async () => ({ error: 'invalid_grant' }) }; + }) as unknown as typeof globalThis.fetch; + + const client = makeClient(); + + // Fire twice WITHOUT awaiting the first; allSettled captures both outcomes. + const [s1, s2] = await Promise.allSettled([ + client.handleRedirectCallback(), + client.handleRedirectCallback(), + ]); + + assert.strictEqual(s1.status, 'rejected', 'first call must reject when the exchange fails'); + assert.strictEqual(s2.status, 'rejected', 'second (concurrent) call must reject too — never resolve null'); + // Both callers share the one in-flight promise, so the rejection is the + // identical Error instance — not two independently-thrown errors. + const reason1 = (s1 as PromiseRejectedResult).reason; + const reason2 = (s2 as PromiseRejectedResult).reason; + assert.strictEqual(reason1, reason2, 'both callers must reject with the identical shared error'); + assert.match(reason1.message, /exchange failed/i); + assert.strictEqual(exchangeCalls, 1, 'single-use PKCE code must be exchanged exactly once, even on failure'); + + // The finally must release the guard on failure: a fresh-code flow on the + // same page runs its own exchange instead of being blocked by a stale entry. + installBrowserGlobals('http://localhost:3000/spa-callback?code=auth-code-retry&state=state-retry'); + process.env.BLOCKS_API_URL = 'http://localhost:3000/aws-blocks/api'; + store.set('__blocks_oidc_pending', JSON.stringify({ + provider: 'google', verifier: 'v', state: 'state-retry', nonce: 'n', + callbackUrl: 'http://localhost:3000/spa-callback', + })); + globalThis.fetch = (async (_url: any, init?: any) => { + if (init?.method === 'POST') exchangeCalls++; + return { ok: true, json: async () => ({ user: BARE_USER }) }; + }) as unknown as typeof globalThis.fetch; + + const retry = await client.handleRedirectCallback(); + assert.ok(retry, 'a fresh-code flow resolves — the guard was released after the failure'); + assert.strictEqual(retry!.userId, 'iss:sub'); + assert.strictEqual(exchangeCalls, 2, 'the fresh flow runs its own second exchange'); + }); +}); diff --git a/packages/bb-auth-oidc/src/index.browser.ts b/packages/bb-auth-oidc/src/index.browser.ts index 91bd9d49..3053e3c6 100644 --- a/packages/bb-auth-oidc/src/index.browser.ts +++ b/packages/bb-auth-oidc/src/index.browser.ts @@ -169,6 +169,27 @@ export type AuthStateHandler = (user: U | null, meta: AuthStateMeta | null) = const subscribers = new Set>(); let lastUser: unknown | null = null; +/** + * In-flight `handleRedirectCallback` guard, keyed by `(exchangePath, code)`. + * + * Scoped to same-tab, in-process concurrency: a double invocation — e.g. React + * StrictMode's mount → unmount → mount, which fires the callback effect twice + * synchronously — shares this one promise instead of racing to exchange the + * single-use code twice. The second exchange would fail (or find the pending + * entry already consumed and return `null`), stranding the app on a signed-out + * screen despite a successful sign-in. + * + * This module variable does not survive a real page reload; cross-reload + * coordination falls back to the up-front `sessionStorage` removal in + * `_exchangeCallback` (a late caller finds no pending entry and resolves + * `null`). Keying on `exchangePath` keeps two clients with distinct exchange + * endpoints from sharing each other's in-flight exchange. + * + * The guard is released once the exchange settles (success or failure) so a + * genuinely new flow on the same page is never blocked. + */ +let _callbackInflight: { exchangePath: string; code: string; promise: Promise } | null = null; + function notify(user: unknown | null, meta: AuthStateMeta | null): void { for (const sub of subscribers) { try { sub(user, meta); } catch { /* swallow handler errors */ } @@ -292,67 +313,103 @@ export class AuthOIDCClient< /** * Complete the IdP redirect callback. Reads `code`/`state` from the URL, * verifies state, and POSTs to `/aws-blocks/auth/exchange`. + * + * Idempotent under double invocation: a second call for the same PKCE + * `code` while the exchange is in flight (e.g. React StrictMode's + * double-mount) shares the first call's promise rather than replaying the + * single-use code, so both callers resolve to the same user. + * * @returns The authenticated user, or `null` if no pending PKCE flow. + * @throws {Error} On state mismatch or a failed `/aws-blocks/auth/exchange` (both concurrent callers reject with the same Error). */ - async handleRedirectCallback(): Promise { - if (typeof window === 'undefined') return null; - const url = new URL(window.location.href); - const code = url.searchParams.get('code'); - const returnedState = url.searchParams.get('state'); - if (!code || !returnedState) return null; - // Forward RFC 9207 `iss` when present — the server-side exchange passes it - // to openid-client, which fails the exchange if the provider sent it and - // we drop it. - const iss = url.searchParams.get('iss') ?? undefined; - - const raw = sessionStorage.getItem(_PENDING_STORAGE_KEY); - if (!raw) return null; - - const pending = JSON.parse(raw) as { - provider: string; - verifier: string; - state: string; - nonce: string; - callbackUrl: string; - appState?: string; - }; - - if (returnedState !== pending.state) { - sessionStorage.removeItem(_PENDING_STORAGE_KEY); - throw new Error('AuthOIDC: state mismatch in callback'); + handleRedirectCallback(): Promise { + if (typeof window === 'undefined') return Promise.resolve(null); + const { searchParams } = new URL(window.location.href); + const code = searchParams.get('code'); + const returnedState = searchParams.get('state'); + if (!code || !returnedState) return Promise.resolve(null); + + // Share an in-flight exchange for this endpoint + code so a concurrent/ + // double invocation can't consume the single-use code twice. + if (_callbackInflight && _callbackInflight.code === code && _callbackInflight.exchangePath === this.exchangePath) { + return _callbackInflight.promise as Promise; } - const baseUrl = await _getBaseUrl(); - const resp = await fetch(`${baseUrl}${this.exchangePath}`, { - method: 'POST', - credentials: 'include', - headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ - code, - verifier: pending.verifier, - state: pending.state, - nonce: pending.nonce, - provider: pending.provider, - callbackUrl: pending.callbackUrl, - ...(iss ? { iss } : {}), - }), - }); - - sessionStorage.removeItem(_PENDING_STORAGE_KEY); - - if (!resp.ok) { - const err = await resp.json().catch(() => ({ error: 'Exchange failed' })); - throw new Error(`AuthOIDC exchange failed: ${(err as any).error ?? resp.status}`); - } + // Forward RFC 9207 `iss` when present — the server-side exchange passes + // it to openid-client, which fails the exchange if the provider sent it + // and we drop it. + const iss = searchParams.get('iss') ?? undefined; + const promise = this._exchangeCallback(code, returnedState, iss); + _callbackInflight = { exchangePath: this.exchangePath, code, promise }; + return promise; + } - const body = await resp.json() as { user?: User } & Partial; - // /aws-blocks/auth/exchange wraps the user (`{ user }`, or `{ user, accessToken, ... }` - // in bearer mode); unwrap to match the declared `User` return type. `?? body` - // guards a future engine path that returns a bare user. - const user = (body.user ?? body) as User; - lastUser = user; - notify(user, { state: pending.appState }); - return user; + /** + * Perform the one-shot PKCE code exchange. Consumes the pending entry up + * front (so a late duplicate can't replay it) and releases the in-flight + * guard once settled. + * + * @param iss - RFC 9207 issuer read from the callback URL (forwarded to the + * server-side exchange), or `undefined` when the provider sent none. + */ + private async _exchangeCallback(code: string, returnedState: string, iss: string | undefined): Promise { + try { + // Consume the single-use pending PKCE entry up front. Removing it before + // the network round-trip (rather than after) means a duplicate call that + // somehow misses the in-flight guard still can't start a second exchange. + const raw = sessionStorage.getItem(_PENDING_STORAGE_KEY); + if (!raw) return null; + sessionStorage.removeItem(_PENDING_STORAGE_KEY); + + const pending = JSON.parse(raw) as { + provider: string; + verifier: string; + state: string; + nonce: string; + callbackUrl: string; + appState?: string; + }; + + if (returnedState !== pending.state) { + throw new Error('AuthOIDC: state mismatch in callback'); + } + + const baseUrl = await _getBaseUrl(); + const resp = await fetch(`${baseUrl}${this.exchangePath}`, { + method: 'POST', + credentials: 'include', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ + code, + verifier: pending.verifier, + state: pending.state, + nonce: pending.nonce, + provider: pending.provider, + callbackUrl: pending.callbackUrl, + ...(iss ? { iss } : {}), + }), + }); + + if (!resp.ok) { + const err = await resp.json().catch(() => ({ error: 'Exchange failed' })); + throw new Error(`AuthOIDC exchange failed: ${(err as any).error ?? resp.status}`); + } + + const body = await resp.json() as { user?: User } & Partial; + // /aws-blocks/auth/exchange wraps the user (`{ user }`, or `{ user, accessToken, ... }` + // in bearer mode); unwrap to match the declared `User` return type. `?? body` + // guards a future engine path that returns a bare user. + const user = (body.user ?? body) as User; + lastUser = user; + notify(user, { state: pending.appState }); + return user; + } finally { + // Release the guard once this exchange settles (success or failure) so a + // brand-new flow on the same page is never blocked by a stale entry. + if (_callbackInflight?.code === code && _callbackInflight?.exchangePath === this.exchangePath) { + _callbackInflight = null; + } + } } /** Sign out and reload the page. */