diff --git a/lib/auth-context.js b/lib/auth-context.js index ffa0553..3171d2e 100644 --- a/lib/auth-context.js +++ b/lib/auth-context.js @@ -79,7 +79,11 @@ export function isSessionStillValid(session, now = Date.now()) { const claims = decodeIdToken(idToken); if (!claims) return false; if (typeof claims.exp !== 'number' || claims.exp * 1000 <= now) return false; - if (claims.iss && !ALLOWED_ISSUERS.has(claims.iss)) return false; + // Require a present, allow-listed issuer. A falsy/missing `iss` must NOT + // pass: otherwise a token minted without an `iss` claim sails through the + // issuer check, letting a stolen-device replay (or a token from another IdP + // with no issuer) be honoured. + if (!claims.iss || !ALLOWED_ISSUERS.has(claims.iss)) return false; return true; } @@ -87,11 +91,16 @@ export function isSessionStillValid(session, now = Date.now()) { * Pure post-auth handler. Exported so tests can cover the result -> user pipeline * without mocking `useAuthRequest` internals. * - * `setSession` receives `{ user, tokens }` on success, `null` on dismissal, or - * throws on error. + * `setSession` receives `{ user, tokens }` on success, `null` on dismissal. + * Throws on error AND on a `success` whose id_token can't be turned into a + * trustworthy session — the caller's effect maps the throw to `setError`. + * Crucially, nothing is written to SecureStore unless the session passes the + * SAME `isSessionStillValid` gate used when restoring on cold start, so a + * session is never "valid enough to log in but purged next launch". */ export async function handleAuthResult(response, setSession, deps = {}) { const store = deps.store ?? SecureStore; + const isValid = deps.isSessionStillValid ?? isSessionStillValid; if (!response) return; if (response.type === 'success') { @@ -102,6 +111,14 @@ export async function handleAuthResult(response, setSession, deps = {}) { const claims = decodeIdToken(idToken); const user = userFromClaims(claims); const session = { user, tokens: { idToken, accessToken } }; + // A `success` with no decodable / no longer trustworthy id_token must NOT + // be persisted as a `{ user: null }` session. Surface an error and skip + // the SecureStore write entirely — same trust model as restore-on-mount. + if (!user || !isValid(session)) { + throw new Error( + 'Google sign-in returned no usable id_token; not persisting session.', + ); + } await store.setItemAsync(STORAGE_KEY, JSON.stringify(session)); setSession(session); return; diff --git a/lib/env.js b/lib/env.js index 4deb397..df41696 100644 --- a/lib/env.js +++ b/lib/env.js @@ -4,23 +4,49 @@ // so `process.env.EXPO_PUBLIC_FOO` works on device. Missing vars here surface a clear // error at the point of use (sign-in), not a cryptic OAuth failure later. -const WEB = process.env.EXPO_PUBLIC_GOOGLE_WEB_CLIENT_ID; -const IOS = process.env.EXPO_PUBLIC_GOOGLE_IOS_CLIENT_ID; -const ANDROID = process.env.EXPO_PUBLIC_GOOGLE_ANDROID_CLIENT_ID; +/** + * Read the client IDs from the environment. Read LAZILY (per call) rather than + * once at module load so the missing-WEB-client guard in `assertGoogleEnv` is + * actually reachable/testable: a test can clear the var and call the function + * without having to re-import this module before its first read. Expo still + * inlines `EXPO_PUBLIC_*` at build time, so on-device behaviour is unchanged — + * `process.env.EXPO_PUBLIC_*` resolves to the inlined string literals. + * + * `env` is injectable purely for tests; production always passes `process.env`. + */ +export function readGoogleClientIds(env = process.env) { + return { + webClientId: env.EXPO_PUBLIC_GOOGLE_WEB_CLIENT_ID, + iosClientId: env.EXPO_PUBLIC_GOOGLE_IOS_CLIENT_ID, + androidClientId: env.EXPO_PUBLIC_GOOGLE_ANDROID_CLIENT_ID, + }; +} +// Live, lazily-evaluated view of the client IDs. Defined as a getter object so +// existing consumers (`googleClientIds.webClientId`) keep working unchanged +// while each access reflects the current environment. export const googleClientIds = { - webClientId: WEB, - iosClientId: IOS, - androidClientId: ANDROID, + get webClientId() { + return process.env.EXPO_PUBLIC_GOOGLE_WEB_CLIENT_ID; + }, + get iosClientId() { + return process.env.EXPO_PUBLIC_GOOGLE_IOS_CLIENT_ID; + }, + get androidClientId() { + return process.env.EXPO_PUBLIC_GOOGLE_ANDROID_CLIENT_ID; + }, }; /** * Throw if the web client ID is missing. The web client ID is the minimum * required for Expo AuthSession proxy dev flow; native IDs are only needed * for standalone production builds. + * + * `env` is injectable for tests so the missing-var path is reachable without + * mutating global `process.env`; production callers pass nothing. */ -export function assertGoogleEnv() { - if (!WEB) { +export function assertGoogleEnv(env = process.env) { + if (!env.EXPO_PUBLIC_GOOGLE_WEB_CLIENT_ID) { throw new Error( 'Missing EXPO_PUBLIC_GOOGLE_WEB_CLIENT_ID. ' + 'Create OAuth client IDs in Google Cloud Console and add them to .env. ' + diff --git a/tests/auth-context.test.js b/tests/auth-context.test.js index c571df5..fe2932b 100644 --- a/tests/auth-context.test.js +++ b/tests/auth-context.test.js @@ -34,22 +34,29 @@ jest.mock('expo-auth-session/providers/google', () => ({ process.env.EXPO_PUBLIC_GOOGLE_WEB_CLIENT_ID = 'test-web-client-id'; // --- Fixture: a Google id_token (not signature-verified, only decoded) --- -// header.payload.signature where payload is base64url(JSON) -const claims = { - iss: 'https://accounts.google.com', - sub: 'goog-123', - email: 'ada@example.com', - name: 'Ada Lovelace', - picture: 'https://example.com/ada.png', - // Far-future exp so isSessionStillValid() accepts it across CI runs. - exp: Math.floor(Date.now() / 1000) + 60 * 60 * 24 * 365 * 10, -}; -const payloadB64 = Buffer.from(JSON.stringify(claims)) - .toString('base64') - .replace(/=+$/, '') - .replace(/\+/g, '-') - .replace(/\//g, '_'); -const fakeIdToken = `eyJhbGciOiJSUzI1NiJ9.${payloadB64}.sig`; +// header.payload.signature where payload is base64url(JSON). +// Helper so each test can mint a token with exactly the claims it needs. +function makeIdToken(overrides = {}) { + const claims = { + iss: 'https://accounts.google.com', + sub: 'goog-123', + email: 'ada@example.com', + name: 'Ada Lovelace', + picture: 'https://example.com/ada.png', + // Far-future exp so isSessionStillValid() accepts it across CI runs. + exp: Math.floor(Date.now() / 1000) + 60 * 60 * 24 * 365 * 10, + ...overrides, + }; + const payloadB64 = Buffer.from(JSON.stringify(claims)) + .toString('base64') + .replace(/=+$/, '') + .replace(/\+/g, '-') + .replace(/\//g, '_'); + return `eyJhbGciOiJSUzI1NiJ9.${payloadB64}.sig`; +} + +const fakeIdToken = makeIdToken(); +const sessionWith = (idToken) => ({ tokens: { idToken, accessToken: 'a' } }); // --- Consumer probe ----------------------------------------------------- @@ -58,9 +65,12 @@ const { useAuth, handleAuthResult, decodeIdToken, + isSessionStillValid, STORAGE_KEY, } = require('../lib/auth-context'); +const { assertGoogleEnv, readGoogleClientIds } = require('../lib/env'); + function Probe({ onUser }) { const ctx = useAuth(); onUser(ctx); @@ -91,6 +101,54 @@ describe('decodeIdToken', () => { }); }); +describe('isSessionStillValid', () => { + const FIXED_NOW = 1_700_000_000_000; // fixed clock so exp math is deterministic + const futureExp = Math.floor(FIXED_NOW / 1000) + 3600; + const pastExp = Math.floor(FIXED_NOW / 1000) - 3600; + + test('accepts a session with a future exp and an allow-listed issuer', () => { + const s = sessionWith(makeIdToken({ exp: futureExp, iss: 'https://accounts.google.com' })); + expect(isSessionStillValid(s, FIXED_NOW)).toBe(true); + // The bare-host issuer Google also documents must pass too. + const s2 = sessionWith(makeIdToken({ exp: futureExp, iss: 'accounts.google.com' })); + expect(isSessionStillValid(s2, FIXED_NOW)).toBe(true); + }); + + test('rejects an expired token even with a valid issuer', () => { + const s = sessionWith(makeIdToken({ exp: pastExp, iss: 'https://accounts.google.com' })); + expect(isSessionStillValid(s, FIXED_NOW)).toBe(false); + }); + + // --- Replay-bypass guard. These are the tests that FAIL if line ~82 ever + // reverts to `if (claims.iss && !ALLOWED_ISSUERS.has(claims.iss))`, because + // a falsy `iss` would then short-circuit the issuer check and return true. + test('rejects a token whose iss claim is MISSING (replay-bypass guard)', () => { + const s = sessionWith(makeIdToken({ exp: futureExp, iss: undefined })); + expect(isSessionStillValid(s, FIXED_NOW)).toBe(false); + }); + + test('rejects a token whose iss claim is the empty string', () => { + const s = sessionWith(makeIdToken({ exp: futureExp, iss: '' })); + expect(isSessionStillValid(s, FIXED_NOW)).toBe(false); + }); + + test('rejects a token from a foreign (non-Google) issuer', () => { + const s = sessionWith(makeIdToken({ exp: futureExp, iss: 'https://evil.example.com' })); + expect(isSessionStillValid(s, FIXED_NOW)).toBe(false); + }); + + test('rejects when there is no id_token or it is undecodable', () => { + expect(isSessionStillValid(undefined, FIXED_NOW)).toBe(false); + expect(isSessionStillValid({ tokens: {} }, FIXED_NOW)).toBe(false); + expect(isSessionStillValid(sessionWith('not-a-jwt'), FIXED_NOW)).toBe(false); + }); + + test('rejects when exp is non-numeric (no silent pass)', () => { + const s = sessionWith(makeIdToken({ exp: 'soon', iss: 'https://accounts.google.com' })); + expect(isSessionStillValid(s, FIXED_NOW)).toBe(false); + }); +}); + describe('handleAuthResult (pure)', () => { test('success result populates user + writes SecureStore', async () => { const setSession = jest.fn(); @@ -133,6 +191,62 @@ describe('handleAuthResult (pure)', () => { expect(setSession).not.toHaveBeenCalled(); }); + // --- Finding: a `success` with no decodable id_token must surface an error + // and SKIP the SecureStore write, NOT persist a `{ user: null }` session. + // FAILS if handleAuthResult reverts to unconditionally writing the session. + test('success with NO decodable id_token throws and writes nothing', async () => { + const setSession = jest.fn(); + await expect( + handleAuthResult( + // No params.id_token and no authentication.idToken -> idToken === null. + { type: 'success', params: {}, authentication: { accessToken: 'at-1' } }, + setSession, + ), + ).rejects.toThrow(/no usable id_token/i); + expect(setSession).not.toHaveBeenCalled(); + expect(mockSecureStore.setItemAsync).not.toHaveBeenCalled(); + expect(mockMemStore.has(STORAGE_KEY)).toBe(false); + }); + + test('success with a garbage (undecodable) id_token throws and writes nothing', async () => { + const setSession = jest.fn(); + await expect( + handleAuthResult( + { type: 'success', params: { id_token: 'totally.not.ajwt!!!' } }, + setSession, + ), + ).rejects.toThrow(/no usable id_token/i); + expect(setSession).not.toHaveBeenCalled(); + expect(mockSecureStore.setItemAsync).not.toHaveBeenCalled(); + }); + + // --- Finding: live sign-in and restore must share ONE trust model. A + // `success` carrying an EXPIRED token must be rejected at sign-in time too, + // not "valid enough to log in but purged on next launch". + // FAILS if handleAuthResult stops gating on isSessionStillValid. + test('success with an EXPIRED id_token is rejected (unified trust model)', async () => { + const setSession = jest.fn(); + const expired = makeIdToken({ + exp: Math.floor(Date.now() / 1000) - 3600, + iss: 'https://accounts.google.com', + }); + await expect( + handleAuthResult({ type: 'success', params: { id_token: expired } }, setSession), + ).rejects.toThrow(/no usable id_token/i); + expect(setSession).not.toHaveBeenCalled(); + expect(mockSecureStore.setItemAsync).not.toHaveBeenCalled(); + }); + + test('success with a FOREIGN-issuer id_token is rejected at sign-in', async () => { + const setSession = jest.fn(); + const foreign = makeIdToken({ iss: 'https://evil.example.com' }); + await expect( + handleAuthResult({ type: 'success', params: { id_token: foreign } }, setSession), + ).rejects.toThrow(/no usable id_token/i); + expect(setSession).not.toHaveBeenCalled(); + expect(mockSecureStore.setItemAsync).not.toHaveBeenCalled(); + }); + // Mutation-check: if handleAuthResult accidentally skipped persisting the // session, the assertion below would still pass for user population but this // extra check fails. Guards against a common regression. @@ -154,17 +268,25 @@ describe('handleAuthResult (pure)', () => { }); describe('AuthProvider lifecycle', () => { - test('first render: user is null once loading finishes', async () => { - let captured; - render( - - (captured = c)} /> - , - ); - await waitFor(() => expect(captured.loading).toBe(false)); - expect(captured.user).toBeNull(); - expect(mockSecureStore.getItemAsync).toHaveBeenCalledWith(STORAGE_KEY); - }); + // Coverage instrumentation slows the async restore effect on loaded CI + // runners, occasionally pushing this past Jest's default 5s and the default + // 1s waitFor poll window. Give both an explicit, generous budget so + // `jest --coverage` is stable, not flaky. (10s test / 4s waitFor.) + test( + 'first render: user is null once loading finishes', + async () => { + let captured; + render( + + (captured = c)} /> + , + ); + await waitFor(() => expect(captured.loading).toBe(false), { timeout: 4000 }); + expect(captured.user).toBeNull(); + expect(mockSecureStore.getItemAsync).toHaveBeenCalledWith(STORAGE_KEY); + }, + 10000, + ); test('restores session from SecureStore on mount', async () => { mockMemStore.set( @@ -180,9 +302,9 @@ describe('AuthProvider lifecycle', () => { (captured = c)} /> , ); - await waitFor(() => expect(captured.loading).toBe(false)); + await waitFor(() => expect(captured.loading).toBe(false), { timeout: 4000 }); expect(captured.user?.email).toBe('restored@example.com'); - }); + }, 10000); test('purges a corrupt SecureStore blob on mount (parse failure)', async () => { // Not valid JSON -> JSON.parse throws inside the restore effect. @@ -193,12 +315,12 @@ describe('AuthProvider lifecycle', () => { (captured = c)} /> , ); - await waitFor(() => expect(captured.loading).toBe(false)); + await waitFor(() => expect(captured.loading).toBe(false), { timeout: 4000 }); expect(captured.user).toBeNull(); // The bad blob must be deleted so the next cold start doesn't re-fail. expect(mockSecureStore.deleteItemAsync).toHaveBeenCalledWith(STORAGE_KEY); expect(mockMemStore.has(STORAGE_KEY)).toBe(false); - }); + }, 10000); test('signOut clears user + SecureStore', async () => { mockMemStore.set( @@ -214,13 +336,46 @@ describe('AuthProvider lifecycle', () => { (captured = c)} /> , ); - await waitFor(() => expect(captured.user?.email).toBe('x@y.z')); + await waitFor(() => expect(captured.user?.email).toBe('x@y.z'), { timeout: 4000 }); await act(async () => { await captured.signOut(); }); - await waitFor(() => expect(captured.user).toBeNull()); + await waitFor(() => expect(captured.user).toBeNull(), { timeout: 4000 }); expect(mockSecureStore.deleteItemAsync).toHaveBeenCalledWith(STORAGE_KEY); + }, 10000); +}); + +describe('assertGoogleEnv / readGoogleClientIds', () => { + // The throw used to be unreachable: env was read once at module load, so a + // test could never exercise the missing-WEB path. With the env injected, the + // guard is reachable. FAILS if assertGoogleEnv stops validating WEB. + test('throws a helpful error when the web client ID is missing', () => { + expect(() => assertGoogleEnv({})).toThrow(/EXPO_PUBLIC_GOOGLE_WEB_CLIENT_ID/); + expect(() => assertGoogleEnv({ EXPO_PUBLIC_GOOGLE_WEB_CLIENT_ID: '' })).toThrow( + /Missing/, + ); + }); + + test('does not throw when the web client ID is present', () => { + expect(() => + assertGoogleEnv({ EXPO_PUBLIC_GOOGLE_WEB_CLIENT_ID: 'web-123' }), + ).not.toThrow(); + }); + + test('readGoogleClientIds reflects the injected environment', () => { + const ids = readGoogleClientIds({ + EXPO_PUBLIC_GOOGLE_WEB_CLIENT_ID: 'web-1', + EXPO_PUBLIC_GOOGLE_IOS_CLIENT_ID: 'ios-1', + EXPO_PUBLIC_GOOGLE_ANDROID_CLIENT_ID: 'and-1', + }); + expect(ids).toEqual({ + webClientId: 'web-1', + iosClientId: 'ios-1', + androidClientId: 'and-1', + }); + // Missing native IDs surface as undefined (web is the only hard requirement). + expect(readGoogleClientIds({}).webClientId).toBeUndefined(); }); });