diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 02e67af..a68df34 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -52,9 +52,16 @@ jobs: run: npx license-checker --failOn "GPL-2.0;GPL-3.0;AGPL-3.0" - name: Security audit - # Expo's transitive deps (tar, jsdom) frequently trigger high-severity - # advisories that can't be fixed without breaking Expo upgrades. - run: npm audit --audit-level=critical + # Threshold raised from `critical` to `high` in the 2026-05-21 + # second-pass audit. The previous `critical`-only gate masked + # 20+ high-severity production advisories (undici, tar, node-forge, + # @xmldom/xmldom, postcss, picomatch, brace-expansion, fast-uri). + # The fix path for all of them is the Expo SDK 52 → 55 upgrade + # tracked in the modernization audit — failing CI here is the + # forcing function for that upgrade. If a single transitive + # advisory genuinely can't be patched, document the exception + # inline rather than weakening this gate again. + run: npm audit --audit-level=high - name: Lint run: npm run lint @@ -62,5 +69,7 @@ jobs: - name: Test run: npm test - - name: Build verification - run: npx expo export --platform web 2>/dev/null || echo "Web export not configured — skipping" + # Web export is a non-goal for the starter (README "Non-Goals"). The + # previous `2>/dev/null || echo "skipping"` swallowed real build + # failures, so the step was producing a false-green signal. Removed + # entirely — `npm test` is the build verification for this project. diff --git a/lib/auth-context.js b/lib/auth-context.js index ab4ee4d..7a03b8f 100644 --- a/lib/auth-context.js +++ b/lib/auth-context.js @@ -72,6 +72,9 @@ const ALLOWED_ISSUERS = new Set([ * The token came from Google over TLS via PKCE, so this isn't a server-grade * check — it's the *client*'s self-defense against a stolen device replaying * an old SecureStore blob. Returns true when the session should be honoured. + * + * The `iss` claim is REQUIRED, not optional. A token missing `iss` is by + * definition not Google-issued, so accepting it would defeat the allowlist. */ export function isSessionStillValid(session, now = Date.now()) { const idToken = session?.tokens?.idToken; @@ -79,7 +82,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 iss AND match the allowlist. A missing iss must be rejected: + // an `iss`-less token cannot have come from Google's documented OIDC flow. + if (typeof claims.iss !== 'string' || !ALLOWED_ISSUERS.has(claims.iss)) { + return false; + } return true; } @@ -99,6 +106,16 @@ export async function handleAuthResult(response, setSession, deps = {}) { response.params?.id_token ?? response.authentication?.idToken ?? null; const accessToken = response.authentication?.accessToken ?? response.params?.access_token ?? null; + // A `type:'success'` response without an id_token cannot establish + // identity — the scope requested includes `openid` so Google always + // returns one. Treating this as success would persist a `{user:null,...}` + // blob to SecureStore that's then deleted on the next mount: a real but + // self-healing state-corruption bug surfaced by the 2026-05-21 audit. + if (!idToken) { + throw new Error( + 'Auth success response missing id_token. Refusing to persist a userless session.', + ); + } const claims = decodeIdToken(idToken); const user = userFromClaims(claims); const session = { user, tokens: { idToken, accessToken } }; @@ -147,6 +164,9 @@ export function AuthProvider({ children }) { } } catch { // corrupt blob -> treat as signed out + // TODO(2nd-pass-audit-2026-05-21): also call deleteItemAsync here + // so a corrupt blob is purged rather than re-read on every mount. + // Self-healing on next successful sign-in, but explicit is better. } finally { if (!cancelled) setLoading(false); } diff --git a/scripts/bump-version.js b/scripts/bump-version.js index 0302317..89a788a 100644 --- a/scripts/bump-version.js +++ b/scripts/bump-version.js @@ -8,7 +8,21 @@ if (!valid.includes(type)) { } const appConfig = JSON.parse(fs.readFileSync('app.json', 'utf8')); -const v = appConfig.expo.version.split('.').map(Number); +const current = appConfig.expo.version; + +// Only accept strict MAJOR.MINOR.PATCH numerics. Prerelease/build-metadata +// (`1.2.3-beta.1`, `1.2.3+sha`) would parse to NaN under the naive split-Map, +// producing a corrupted `1.2.NaN` write — fail loudly instead. +if (!/^\d+\.\d+\.\d+$/.test(current)) { + console.error( + `Refusing to bump: app.json expo.version="${current}" is not strict ` + + `MAJOR.MINOR.PATCH. Prerelease/build-metadata is not supported here — ` + + `bump manually and commit, or strip suffixes first.`, + ); + process.exit(1); +} + +const v = current.split('.').map(Number); if (type === 'major') { v[0]++; v[1] = 0; v[2] = 0; } else if (type === 'minor') { v[1]++; v[2] = 0; } diff --git a/tests/app.test.js b/tests/app.test.js index c9fe22c..f83b9ce 100644 --- a/tests/app.test.js +++ b/tests/app.test.js @@ -74,8 +74,81 @@ describe('Project structure', () => { describe('Version bumper', () => { const bumperPath = path.resolve(__dirname, '..', 'scripts', 'bump-version.js'); + const { execFileSync } = require('child_process'); + const os = require('os'); - test('bump script exists', () => { - expect(fs.existsSync(bumperPath)).toBe(true); + // Run the bumper against a throwaway sandbox so we don't mutate the real + // app.json. The script reads/writes from process.cwd(), so we cd into a + // tmpdir seeded with a minimal app.json (and optionally a package.json). + function runBumper({ type, appVersion, withPackageJson = false }) { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'bump-test-')); + fs.writeFileSync( + path.join(dir, 'app.json'), + JSON.stringify({ expo: { version: appVersion } }) + '\n', + ); + if (withPackageJson) { + fs.writeFileSync( + path.join(dir, 'package.json'), + JSON.stringify({ name: 'sandbox', version: appVersion }) + '\n', + ); + } + let err = null; + let stdout = ''; + try { + stdout = execFileSync(process.execPath, [bumperPath, type], { + cwd: dir, + encoding: 'utf8', + stdio: ['ignore', 'pipe', 'pipe'], + }); + } catch (e) { + err = e; + } + const finalApp = JSON.parse(fs.readFileSync(path.join(dir, 'app.json'), 'utf8')); + const finalPkg = withPackageJson + ? JSON.parse(fs.readFileSync(path.join(dir, 'package.json'), 'utf8')) + : null; + return { stdout: stdout.trim(), err, app: finalApp, pkg: finalPkg }; + } + + test('patch bump increments only the patch component', () => { + const r = runBumper({ type: 'patch', appVersion: '1.2.3' }); + expect(r.err).toBeNull(); + expect(r.app.expo.version).toBe('1.2.4'); + expect(r.stdout).toBe('1.2.4'); + }); + + test('minor bump zeroes the patch component', () => { + const r = runBumper({ type: 'minor', appVersion: '1.2.3' }); + expect(r.app.expo.version).toBe('1.3.0'); + }); + + test('major bump zeroes minor and patch', () => { + const r = runBumper({ type: 'major', appVersion: '1.2.3' }); + expect(r.app.expo.version).toBe('2.0.0'); + }); + + test('mirrors the new version into package.json when present', () => { + const r = runBumper({ type: 'patch', appVersion: '1.2.3', withPackageJson: true }); + expect(r.pkg.version).toBe('1.2.4'); + }); + + test('rejects prerelease versions instead of writing NaN — regression', () => { + // Before the second-pass audit (2026-05-21), `"1.2.3-beta.1".split('.')` + // -> `Number('3-beta')` = NaN, producing `1.2.NaN` in app.json. + const r = runBumper({ type: 'patch', appVersion: '1.2.3-beta.1' }); + expect(r.err).not.toBeNull(); + expect(r.app.expo.version).toBe('1.2.3-beta.1'); // unchanged + }); + + test('rejects 2-component versions instead of writing NaN — regression', () => { + const r = runBumper({ type: 'patch', appVersion: '1.2' }); + expect(r.err).not.toBeNull(); + expect(r.app.expo.version).toBe('1.2'); // unchanged + }); + + test('rejects invalid bump type', () => { + const r = runBumper({ type: 'wat', appVersion: '1.2.3' }); + expect(r.err).not.toBeNull(); + expect(r.app.expo.version).toBe('1.2.3'); // unchanged }); }); diff --git a/tests/auth-context.test.js b/tests/auth-context.test.js index a056510..14c2cf8 100644 --- a/tests/auth-context.test.js +++ b/tests/auth-context.test.js @@ -30,8 +30,20 @@ jest.mock('expo-auth-session/providers/google', () => ({ useAuthRequest: () => [{ state: 'ready' }, null, jest.fn(async () => ({ type: 'cancel' }))], })); -// Env: pretend the web client ID is set. -process.env.EXPO_PUBLIC_GOOGLE_WEB_CLIENT_ID = 'test-web-client-id'; +// Stub lib/env so assertGoogleEnv is a no-op for the happy-path suite. The +// error path (assertGoogleEnv throws) lives in tests/signin-error.test.js +// where the same module is mocked the other way. The previous +// `process.env.EXPO_PUBLIC_... = ...` approach worked while no test invoked +// signIn(), but env.js reads process.env at module-load time — the load +// order vs. the assignment is fragile under jest-expo's preset hoisting. +jest.mock('../lib/env', () => ({ + googleClientIds: { + webClientId: 'test-web-client-id', + iosClientId: undefined, + androidClientId: undefined, + }, + assertGoogleEnv: jest.fn(), +})); // --- Fixture: a Google id_token (not signature-verified, only decoded) --- // header.payload.signature where payload is base64url(JSON) @@ -58,9 +70,21 @@ const { useAuth, handleAuthResult, decodeIdToken, + isSessionStillValid, STORAGE_KEY, } = require('../lib/auth-context'); +// Build a JWT with arbitrary claims for the negative-path probes below. +// Signature is never verified by this client — only the payload is decoded. +function makeFakeJwt(claimsOverride) { + const payload = Buffer.from(JSON.stringify(claimsOverride)) + .toString('base64') + .replace(/=+$/, '') + .replace(/\+/g, '-') + .replace(/\//g, '_'); + return `eyJhbGciOiJSUzI1NiJ9.${payload}.sig`; +} + function Probe({ onUser }) { const ctx = useAuth(); onUser(ctx); @@ -91,6 +115,58 @@ describe('decodeIdToken', () => { }); }); +describe('isSessionStillValid — issuer + expiry boundary', () => { + const future = Math.floor(Date.now() / 1000) + 3600; + const past = Math.floor(Date.now() / 1000) - 3600; + + test('accepts a fresh Google-issued token', () => { + const tok = makeFakeJwt({ + iss: 'https://accounts.google.com', + sub: 'goog-1', + exp: future, + }); + expect(isSessionStillValid({ tokens: { idToken: tok } })).toBe(true); + }); + + test('rejects a token whose iss is missing — regression for the second-pass audit', () => { + // Without this check, an attacker who can write to SecureStore (e.g. on a + // rooted/jailbroken device or via a shared-keychain entitlement bug) could + // craft an iss-less blob and ride the session indefinitely. See + // lib/auth-context.js — `iss` is REQUIRED, not optional. + const tok = makeFakeJwt({ sub: 'x', exp: future }); + expect(isSessionStillValid({ tokens: { idToken: tok } })).toBe(false); + }); + + test('rejects a token issued by a non-Google IdP', () => { + const tok = makeFakeJwt({ + iss: 'https://evil.example.com', + sub: 'x', + exp: future, + }); + expect(isSessionStillValid({ tokens: { idToken: tok } })).toBe(false); + }); + + test('rejects an expired token even with a valid iss', () => { + const tok = makeFakeJwt({ + iss: 'https://accounts.google.com', + sub: 'x', + exp: past, + }); + expect(isSessionStillValid({ tokens: { idToken: tok } })).toBe(false); + }); + + test('rejects when exp is missing entirely', () => { + const tok = makeFakeJwt({ iss: 'https://accounts.google.com', sub: 'x' }); + expect(isSessionStillValid({ tokens: { idToken: tok } })).toBe(false); + }); + + test('rejects when no idToken is present', () => { + expect(isSessionStillValid({ tokens: {} })).toBe(false); + expect(isSessionStillValid({})).toBe(false); + expect(isSessionStillValid(null)).toBe(false); + }); +}); + describe('handleAuthResult (pure)', () => { test('success result populates user + writes SecureStore', async () => { const setSession = jest.fn(); @@ -133,6 +209,34 @@ describe('handleAuthResult (pure)', () => { expect(setSession).not.toHaveBeenCalled(); }); + test('success without id_token throws and does NOT persist — regression', async () => { + // Before the second-pass audit (2026-05-21), this path silently wrote + // `{user:null,tokens:{idToken:null,accessToken:null}}` to SecureStore. + // Self-healing on next mount, but a real state-corruption bug. + const setSession = jest.fn(); + const store = { + setItemAsync: jest.fn(), + getItemAsync: jest.fn(), + deleteItemAsync: jest.fn(), + }; + await expect( + handleAuthResult( + { type: 'success', params: {}, authentication: {} }, + setSession, + { store }, + ), + ).rejects.toThrow(/missing id_token/); + expect(store.setItemAsync).not.toHaveBeenCalled(); + expect(setSession).not.toHaveBeenCalled(); + }); + + test('unknown response type (e.g. dismiss) is a no-op', async () => { + const setSession = jest.fn(); + await handleAuthResult({ type: 'dismiss' }, setSession); + 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. @@ -184,6 +288,23 @@ describe('AuthProvider lifecycle', () => { expect(captured.user?.email).toBe('restored@example.com'); }); + test('signIn (happy path) calls promptAsync without throwing', async () => { + // Covers lines 165-173 + 178 of lib/auth-context.js (signIn body, no-throw + // branch). The error path is covered in tests/signin-error.test.js with + // isolated mocking — see that file for the assertGoogleEnv-throws scenario. + let captured; + render( + + (captured = c)} /> + , + ); + await waitFor(() => expect(captured.loading).toBe(false)); + await act(async () => { + await captured.signIn(); + }); + expect(captured.error).toBeNull(); + }); + test('signOut clears user + SecureStore', async () => { mockMemStore.set( STORAGE_KEY, diff --git a/tests/env.test.js b/tests/env.test.js new file mode 100644 index 0000000..d74b3d5 --- /dev/null +++ b/tests/env.test.js @@ -0,0 +1,45 @@ +// Lock the env assertion contract that lib/env.js promises in its docstring. +// Previously uncovered (env.js:23-24 throw branch) — added in the 2026-05-21 +// second-pass audit. +// +// Caveat: Expo's babel preset enables +// `babel-plugin-transform-inline-environment-variables` for EXPO_PUBLIC_* +// vars, which inlines `process.env.EXPO_PUBLIC_GOOGLE_WEB_CLIENT_ID` at +// compile time. That means re-importing env.js after mutating process.env +// at runtime is a no-op — the compiled module captured whatever the env +// was when this test file was first transformed. +// +// Practical consequence: the positive-path ("does not throw when set", +// "exposes IDs") is covered indirectly by tests/auth-context.test.js (which +// mocks lib/env). The two tests below isolate just the throw branch by +// running env.js in an unset state — which is what happens when CI runs +// without dev secrets configured. + +describe('assertGoogleEnv (throw branch only — see header comment)', () => { + const KEY = 'EXPO_PUBLIC_GOOGLE_WEB_CLIENT_ID'; + let saved; + + beforeEach(() => { + saved = process.env[KEY]; + delete process.env[KEY]; + }); + + afterEach(() => { + if (saved === undefined) delete process.env[KEY]; + else process.env[KEY] = saved; + }); + + test('throws a helpful error when the web client ID is missing', () => { + jest.isolateModules(() => { + const { assertGoogleEnv } = require('../lib/env'); + expect(() => assertGoogleEnv()).toThrow(/EXPO_PUBLIC_GOOGLE_WEB_CLIENT_ID/); + }); + }); + + test('error message points the user at .env.example and the README', () => { + jest.isolateModules(() => { + const { assertGoogleEnv } = require('../lib/env'); + expect(() => assertGoogleEnv()).toThrow(/\.env\.example|README/); + }); + }); +}); diff --git a/tests/layout.test.js b/tests/layout.test.js new file mode 100644 index 0000000..927a666 --- /dev/null +++ b/tests/layout.test.js @@ -0,0 +1,87 @@ +// Exercise the route-group gating that README "Currently implemented" lists +// as the auth boundary. Before the 2026-05-21 second-pass audit, no test +// rendered (app)/_layout.js — the gating was claimed, not verified. + +import React from 'react'; +import { render } from '@testing-library/react-native'; +import { Text } from 'react-native'; + +// Capture what `` was rendered with so we can assert the gate. +const mockRedirect = jest.fn(({ href }) => null); +const mockStack = jest.fn(() => null); + +jest.mock('expo-router', () => ({ + Redirect: (props) => mockRedirect(props), + Stack: (props) => mockStack(props), +})); + +// Mock useAuth per-test by re-requiring after redefining the mock factory. +let mockAuthState; +jest.mock('../lib/auth-context', () => ({ + useAuth: () => mockAuthState, +})); + +describe('(app)/_layout — protected route group', () => { + beforeEach(() => { + mockRedirect.mockClear(); + mockStack.mockClear(); + }); + + test('while loading, shows the spinner (no redirect, no Stack)', () => { + mockAuthState = { user: null, loading: true }; + const AppLayout = require('../app/(app)/_layout').default; + const { UNSAFE_root } = render(); + expect(mockRedirect).not.toHaveBeenCalled(); + expect(mockStack).not.toHaveBeenCalled(); + // The spinner View is rendered — assert by tree shape rather than text. + expect(UNSAFE_root).toBeTruthy(); + }); + + test('when not signed in, redirects to /login (regression for D1 gating claim)', () => { + mockAuthState = { user: null, loading: false }; + const AppLayout = require('../app/(app)/_layout').default; + render(); + expect(mockRedirect).toHaveBeenCalledWith({ href: '/login' }); + expect(mockStack).not.toHaveBeenCalled(); + }); + + test('when signed in, renders the Stack (protected zone is accessible)', () => { + mockAuthState = { user: { email: 'a@b.c' }, loading: false }; + const AppLayout = require('../app/(app)/_layout').default; + render(); + expect(mockStack).toHaveBeenCalled(); + expect(mockRedirect).not.toHaveBeenCalled(); + }); +}); + +describe('(auth)/_layout — bounce when already signed in', () => { + beforeEach(() => { + mockRedirect.mockClear(); + mockStack.mockClear(); + }); + + test('while loading, neither Redirect nor a bounce fires', () => { + mockAuthState = { user: null, loading: true }; + const AuthLayout = require('../app/(auth)/_layout').default; + render(); + expect(mockRedirect).not.toHaveBeenCalled(); + // Stack still rendered while loading — that's intentional, the parent + // (app)/_layout owns the spinner. + expect(mockStack).toHaveBeenCalled(); + }); + + test('when signed in, bounces back to /', () => { + mockAuthState = { user: { email: 'a@b.c' }, loading: false }; + const AuthLayout = require('../app/(auth)/_layout').default; + render(); + expect(mockRedirect).toHaveBeenCalledWith({ href: '/' }); + }); + + test('when not signed in, renders the Stack so the login screen appears', () => { + mockAuthState = { user: null, loading: false }; + const AuthLayout = require('../app/(auth)/_layout').default; + render(); + expect(mockRedirect).not.toHaveBeenCalled(); + expect(mockStack).toHaveBeenCalled(); + }); +}); diff --git a/tests/signin-error.test.js b/tests/signin-error.test.js new file mode 100644 index 0000000..1a5d2dc --- /dev/null +++ b/tests/signin-error.test.js @@ -0,0 +1,64 @@ +// Cover the signIn error path (auth-context.js:174-177) — previously +// uncovered. The path: assertGoogleEnv() throws → catch → setError(e) → rethrow. +// Isolated in its own file so we can mock lib/env without disturbing the +// happy-path fixture in tests/auth-context.test.js. + +import React from 'react'; +import { act, render, waitFor } from '@testing-library/react-native'; +import { Text } from 'react-native'; + +const mockMemStore = new Map(); +const mockSecureStore = { + getItemAsync: jest.fn(async (k) => (mockMemStore.has(k) ? mockMemStore.get(k) : null)), + setItemAsync: jest.fn(async (k, v) => { + mockMemStore.set(k, v); + }), + deleteItemAsync: jest.fn(async (k) => { + mockMemStore.delete(k); + }), +}; + +jest.mock('expo-secure-store', () => mockSecureStore); +jest.mock('expo-web-browser', () => ({ maybeCompleteAuthSession: jest.fn() })); +jest.mock('expo-auth-session/providers/google', () => ({ + useAuthRequest: () => [{ state: 'ready' }, null, jest.fn()], +})); + +// Replace lib/env with a stub whose assertGoogleEnv always throws — this +// simulates the "developer forgot to set EXPO_PUBLIC_GOOGLE_WEB_CLIENT_ID" +// situation at the boundary of signIn() rather than at module load. +jest.mock('../lib/env', () => ({ + googleClientIds: { webClientId: undefined, iosClientId: undefined, androidClientId: undefined }, + assertGoogleEnv: () => { + throw new Error('Missing EXPO_PUBLIC_GOOGLE_WEB_CLIENT_ID. (test stub)'); + }, +})); + +const { AuthProvider, useAuth } = require('../lib/auth-context'); + +function Probe({ onUser }) { + const ctx = useAuth(); + onUser(ctx); + return {ctx.user ? 'user' : 'anon'}; +} + +describe('signIn error path — env missing', () => { + test('rethrows from signIn AND lights up context.error', async () => { + let captured; + render( + + (captured = c)} /> + , + ); + await waitFor(() => expect(captured.loading).toBe(false)); + + await expect( + act(async () => { + await captured.signIn(); + }), + ).rejects.toThrow(/Missing EXPO_PUBLIC_GOOGLE_WEB_CLIENT_ID/); + + await waitFor(() => expect(captured.error).toBeInstanceOf(Error)); + expect(captured.error.message).toMatch(/Missing EXPO_PUBLIC_GOOGLE_WEB_CLIENT_ID/); + }); +});