diff --git a/.changeset/quiet-webviews-recover.md b/.changeset/quiet-webviews-recover.md new file mode 100644 index 0000000000..b68c82637a --- /dev/null +++ b/.changeset/quiet-webviews-recover.md @@ -0,0 +1,5 @@ +--- +'@shopify/hydrogen': patch +--- + +Recover customer account login when switching browser contexts or opening multiple login tabs. OAuth state values now use cryptographically secure randomness. diff --git a/packages/hydrogen/src/customer/auth.helpers.test.ts b/packages/hydrogen/src/customer/auth.helpers.test.ts index 27dfc9304c..5051a93430 100644 --- a/packages/hydrogen/src/customer/auth.helpers.test.ts +++ b/packages/hydrogen/src/customer/auth.helpers.test.ts @@ -1,7 +1,14 @@ import {describe, it, expect, vi, beforeEach, afterEach} from 'vitest'; import type {HydrogenSession} from '../types'; import {CUSTOMER_ACCOUNT_SESSION_KEY} from './constants'; -import {checkExpires, clearSession, refreshToken} from './auth.helpers'; +import { + checkExpires, + clearSession, + generateState, + refreshToken, +} from './auth.helpers'; + +const OAUTH_STATE_BYTE_LENGTH = 32; vi.mock('./BadRequest', () => { return { @@ -37,6 +44,32 @@ function createFetchResponse(data: T, options: {ok: boolean}) { let session: HydrogenSession; describe('auth.helpers', () => { + describe('generateState', () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + + it('Generates a 256-bit base64url value', () => { + const randomBytes = Uint8Array.from( + {length: OAUTH_STATE_BYTE_LENGTH}, + (_, index) => index, + ); + const getRandomValuesSpy = vi + .spyOn(globalThis.crypto, 'getRandomValues') + .mockImplementation((array: any) => { + array.set(randomBytes); + return array; + }); + const state = generateState(); + const decodedState = Buffer.from(state, 'base64url'); + + expect(state).toBe(Buffer.from(randomBytes).toString('base64url')); + expect(state).toMatch(/^[A-Za-z0-9_-]+$/); + expect(decodedState).toHaveLength(OAUTH_STATE_BYTE_LENGTH); + expect(getRandomValuesSpy).toHaveBeenCalledOnce(); + }); + }); + describe('refreshToken', () => { beforeEach(() => { session = { diff --git a/packages/hydrogen/src/customer/auth.helpers.ts b/packages/hydrogen/src/customer/auth.helpers.ts index e2a3ef2a8e..b8a573d600 100644 --- a/packages/hydrogen/src/customer/auth.helpers.ts +++ b/packages/hydrogen/src/customer/auth.helpers.ts @@ -232,9 +232,7 @@ function convertBufferToString(hash: ArrayBuffer) { } export function generateState() { - const timestamp = Date.now().toString(); - const randomString = Math.random().toString(36).substring(2); - return timestamp + randomString; + return base64UrlEncode(generateRandomCode()); } export async function exchangeAccessToken( diff --git a/packages/hydrogen/src/customer/customer.test.ts b/packages/hydrogen/src/customer/customer.test.ts index b7824c53c0..27e550a4d1 100644 --- a/packages/hydrogen/src/customer/customer.test.ts +++ b/packages/hydrogen/src/customer/customer.test.ts @@ -79,6 +79,7 @@ describe('customer', () => { afterEach(() => { process.env.NODE_ENV = originalNodeEnv; + vi.restoreAllMocks(); vi.clearAllMocks(); }); @@ -1170,7 +1171,7 @@ describe('customer', () => { describe('authorize', () => { describe('using new auth url when shopId is present in env', () => { - it('Throws unauthorized if no code or state params are passed', async () => { + it('Rejects missing callback params without clearing the session', async () => { const customer = createCustomerAccountClient({ session, customerAccountId: 'customerAccountId', @@ -1182,19 +1183,177 @@ describe('customer', () => { await expect(customer.authorize()).rejects.toThrowError( 'Unauthorized No code or state parameter found in the redirect URL.', ); + expect(fetch).not.toHaveBeenCalled(); + expect(session.unset).not.toHaveBeenCalled(); }); - it("Throws unauthorized if state doesn't match session value", async () => { + it('Restarts login if the browser does not have the original session state', async () => { + const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); + session.get = vi.fn(() => undefined) as HydrogenSession['get']; const customer = createCustomerAccountClient({ session, customerAccountId: 'customerAccountId', shopId: '1', - request: new Request('https://localhost?state=nomatch&code=code'), + request: new Request( + 'https://localhost/account/authorize?state=state&code=code', + ), waitUntil: vi.fn(), }); - await expect(customer.authorize()).rejects.toThrowError( - 'Unauthorized The session state does not match the state parameter. Make sure that the session is configured correctly and passed to `createCustomerAccountClient`.', + const response = await customer.authorize(); + + expect(response.status).toBe(302); + expect(response.headers.get('location')).toBe( + '/account/login?return_to=%2Faccount&oauth_recovery=true', + ); + expect(fetch).not.toHaveBeenCalled(); + expect(session.unset).not.toHaveBeenCalled(); + expect(warnSpy).toHaveBeenCalledWith( + '[h2:warn:customerAccount] OAuth callback state mismatch. Restarting customer login once.', + ); + }); + + it('Restarts login if another tab replaced the original session state', async () => { + session.get = vi.fn(() => ({ + ...mockCustomerAccountSession, + state: 'newer-state', + redirectPath: '/account/orders', + })) as HydrogenSession['get']; + const customer = createCustomerAccountClient({ + session, + customerAccountId: 'customerAccountId', + shopId: '1', + request: new Request( + 'https://localhost/account/authorize?state=older-state&code=code', + ), + waitUntil: vi.fn(), + }); + + const response = await customer.authorize(); + + expect(response.status).toBe(302); + expect(response.headers.get('location')).toBe( + '/account/login?return_to=%2Faccount%2Forders&oauth_recovery=true', + ); + expect(fetch).not.toHaveBeenCalled(); + expect(session.unset).not.toHaveBeenCalled(); + }); + + it('Stops after one recovery attempt if the browser still has no session state', async () => { + session.get = vi.fn(() => undefined) as HydrogenSession['get']; + const firstAuthorize = createCustomerAccountClient({ + session, + customerAccountId: 'customerAccountId', + shopId: '1', + request: new Request( + 'https://localhost/account/authorize?state=state&code=code', + ), + waitUntil: vi.fn(), + }); + const firstAuthorizeResponse = await firstAuthorize.authorize(); + const recoveryLoginLocation = + firstAuthorizeResponse.headers.get('location') ?? ''; + const recoveryLogin = createCustomerAccountClient({ + session, + customerAccountId: 'customerAccountId', + shopId: '1', + request: new Request( + new URL(recoveryLoginLocation, 'https://localhost'), + ), + waitUntil: vi.fn(), + }); + const recoveryLoginResponse = await recoveryLogin.login(); + const recoveryState = + new URL( + recoveryLoginResponse.headers.get('location') ?? '', + ).searchParams.get('state') ?? ''; + const secondAuthorize = createCustomerAccountClient({ + session, + customerAccountId: 'customerAccountId', + shopId: '1', + request: new Request( + `https://localhost/account/authorize?${new URLSearchParams({ + state: recoveryState, + code: 'code', + })}`, + ), + waitUntil: vi.fn(), + }); + + expect(recoveryState).not.toBe(''); + await expect(secondAuthorize.authorize()).rejects.toThrowError( + 'Unable to complete sign in. Please try again. The session state does not match the state parameter. Make sure that the session is configured correctly and passed to `createCustomerAccountClient`.', + ); + expect(fetch).not.toHaveBeenCalled(); + }); + + it('Completes authorization after restarting login with a fresh session state', async () => { + session.get = vi.fn(() => undefined) as HydrogenSession['get']; + const firstAuthorize = createCustomerAccountClient({ + session, + customerAccountId: 'customerAccountId', + shopId: '1', + request: new Request( + 'https://localhost/account/authorize?state=state&code=code', + ), + waitUntil: vi.fn(), + }); + const firstAuthorizeResponse = await firstAuthorize.authorize(); + const recoveryLogin = createCustomerAccountClient({ + session, + customerAccountId: 'customerAccountId', + shopId: '1', + request: new Request( + new URL( + firstAuthorizeResponse.headers.get('location') ?? '', + 'https://localhost', + ), + ), + waitUntil: vi.fn(), + }); + const recoveryLoginResponse = await recoveryLogin.login(); + const recoverySession = (session.set as any).mock.calls.at(-1)?.[1]; + + if (!recoverySession) { + throw new Error('Expected recovery login to persist its session'); + } + + session.get = vi.fn(() => recoverySession) as HydrogenSession['get']; + fetch.mockResolvedValue( + createFetchResponse( + { + access_token: 'shcat_access_token', + expires_in: '', + id_token: `${btoa('{}')}.${btoa( + JSON.stringify({nonce: recoverySession.nonce}), + )}.signature`, + refresh_token: 'shcrt_refresh_token', + }, + {ok: true}, + ), + ); + const recoveryState = + new URL( + recoveryLoginResponse.headers.get('location') ?? '', + ).searchParams.get('state') ?? ''; + const secondAuthorize = createCustomerAccountClient({ + session, + customerAccountId: 'customerAccountId', + shopId: '1', + request: new Request( + `https://localhost/account/authorize?${new URLSearchParams({ + state: recoveryState, + code: 'code', + })}`, + ), + waitUntil: vi.fn(), + }); + + const secondAuthorizeResponse = await secondAuthorize.authorize(); + + expect(secondAuthorizeResponse.status).toBe(302); + expect(secondAuthorizeResponse.headers.get('location')).toBe( + '/account', ); }); diff --git a/packages/hydrogen/src/customer/customer.ts b/packages/hydrogen/src/customer/customer.ts index 8cb54587f6..041fc37740 100644 --- a/packages/hydrogen/src/customer/customer.ts +++ b/packages/hydrogen/src/customer/customer.ts @@ -53,6 +53,9 @@ import {warnOnce} from '../utils/warning'; import {LanguageCode} from '@shopify/hydrogen-react/customer-account-api-types'; const HYDROGEN_TUNNEL_DOMAIN_SUFFIX = '.tryhydrogen.dev'; +const OAUTH_RECOVERY_PARAM = 'oauth_recovery'; +// State survives the OAuth round trip even when the browser drops the session cookie. +const OAUTH_RECOVERY_STATE_PREFIX = 'oauth-recovery.'; function checkTunnelDomain( hostname: string, @@ -392,7 +395,11 @@ export function createCustomerAccountClient({ const loginUrl = new URL(getCustomerAccountUrl(URL_TYPE.AUTH)); - const state = generateState(); + const isOAuthRecovery = + requestUrl.searchParams.get(OAUTH_RECOVERY_PARAM) === 'true'; + const state = `${ + isOAuthRecovery ? OAUTH_RECOVERY_STATE_PREFIX : '' + }${generateState()}`; const nonce = generateNonce(); loginUrl.searchParams.set('client_id', customerAccountId); @@ -510,20 +517,34 @@ export function createCustomerAccountClient({ const state = requestUrl.searchParams.get('state'); if (!code || !state) { - clearSession(session); - throw new BadRequest( 'Unauthorized', 'No code or state parameter found in the redirect URL.', ); } - if (session.get(CUSTOMER_ACCOUNT_SESSION_KEY)?.state !== state) { - clearSession(session); + const customerAccountSession = session.get(CUSTOMER_ACCOUNT_SESSION_KEY); - throw new BadRequest( - 'Unauthorized', - 'The session state does not match the state parameter. Make sure that the session is configured correctly and passed to `createCustomerAccountClient`.', + if (customerAccountSession?.state !== state) { + if (state.startsWith(OAUTH_RECOVERY_STATE_PREFIX)) { + throw new BadRequest( + 'Unable to complete sign in. Please try again.', + 'The session state does not match the state parameter. Make sure that the session is configured correctly and passed to `createCustomerAccountClient`.', + ); + } + + warnOnce( + '[h2:warn:customerAccount] OAuth callback state mismatch. Restarting customer login once.', + ); + const recoveryLoginUrl = new URL(loginPath, request.url); + recoveryLoginUrl.searchParams.set( + 'return_to', + customerAccountSession?.redirectPath ?? defaultRedirectPath, + ); + recoveryLoginUrl.searchParams.set(OAUTH_RECOVERY_PARAM, 'true'); + + return redirect( + `${recoveryLoginUrl.pathname}${recoveryLoginUrl.search}`, ); }