fix(auth): enforce issuer + validate sign-in tokens + test the session guards

lib/auth-context.js

@@ -79,19 +79,28 @@ 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;
 }
 
 /**
  * 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;

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. ' +

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(
-      <AuthProvider>
-        <Probe onUser={(c) => (captured = c)} />
-      </AuthProvider>,
-    );
-    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(
+        <AuthProvider>
+          <Probe onUser={(c) => (captured = c)} />
+        </AuthProvider>,
+      );
+      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', () => {
         <Probe onUser={(c) => (captured = c)} />
       </AuthProvider>,
     );
-    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', () => {
         <Probe onUser={(c) => (captured = c)} />
       </AuthProvider>,
     );
-    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', () => {
         <Probe onUser={(c) => (captured = c)} />
       </AuthProvider>,
     );
-    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();
   });
 });