fix(auth)!: second-pass audit + post-fix review (iss bypass + 15 review findings)#51
Open
heznpc wants to merge 5 commits into
Open
fix(auth)!: second-pass audit + post-fix review (iss bypass + 15 review findings)#51heznpc wants to merge 5 commits into
heznpc wants to merge 5 commits into
Conversation
isSessionStillValid had `if (claims.iss && !ALLOWED_ISSUERS.has(claims.iss))`, which short-circuits when iss is missing entirely — an iss-less id_token slipped past the Google-only allowlist that the function's docstring explicitly promises to enforce. Threat model: an attacker who can write to SecureStore (rooted/jailbroken device, shared-keychain entitlement bug, hostile companion app) could craft an iss-less blob and the client would honour it. Real-world likelihood is low for any single user, but this is a starter template — the reference implementation propagates to every downstream fork, so shipping a broken boundary here amplifies the harm. Fix: require `typeof claims.iss === 'string'` AND allowlist membership. Regression coverage: 6 new tests in tests/auth-context.test.js exercising the issuer/expiry boundary directly (the original suite only fed a fixed Google-iss happy-path token, which is why the bug survived). Surfaced by an adversarial second-pass audit on 2026-05-21. Previously declared done by the same session that wrote the function — A3 (happy-path only) catch-rate gap.
Layered on top of the iss-bypass fix in this same branch. All seven
items were surfaced by the 2026-05-21 adversarial second-pass on a
session that had reported "tests pass / CI green" — pure A3/A2/D1
catch-rate gaps in the prior audit.
M1 + M5 — Version bumper had a trivially-passing test (file-existence
check only) and crashed on prerelease versions:
- scripts/bump-version.js: reject anything not strict MAJOR.MINOR.PATCH
(previously `"1.2.3-beta.1".split('.').map(Number)` produced NaN
and would have committed `1.2.NaN` to app.json)
- tests/app.test.js: replace the existence check with 7 behavioural
tests that exercise the bumper in a sandboxed tmpdir
M2 — handleAuthResult silently persisted a `{user:null,tokens:{...:null}}`
blob when the OAuth response was `type:'success'` with no id_token.
Self-healing on next mount but a real state-corruption bug.
- lib/auth-context.js: throw on missing id_token, do not persist
- tests/auth-context.test.js: regression test
M3 — (app)/_layout.js gating ("Currently implemented" in README) was
never exercised by any test. screens.test.js mocked useAuth to always
return a user, so the redirect/loading branches were 0% covered.
- tests/layout.test.js: NEW file. Covers both (app) and (auth)
layouts across loading / signed-out / signed-in states. The "D1
promised, not verified" gap on the auth boundary is now closed.
M4 — assertGoogleEnv throw + signIn body were uncovered:
- tests/env.test.js: NEW. Locks the throw branch contract. Caveat
in the file header: Expo's babel preset inlines EXPO_PUBLIC_* at
compile time, which restricts how we can probe the positive case.
- tests/signin-error.test.js: NEW. Isolated mock of lib/env to
cover signIn's catch → setError → rethrow path.
M6 — ci.yml "Build verification" step swallowed errors via
`2>/dev/null || echo "skipping"`. Removed. Web export is a non-goal
per README, and the step's only effect was producing a false-green
signal.
M7 — ci.yml `npm audit --audit-level=critical` masked 23 production
advisories. Raised to `--audit-level=high`. The fix path is the
Expo SDK 52 → 55 upgrade tracked in the modernization audit; CI
going red here is the intended forcing function until that upgrade
lands. If a single transitive advisory turns out to be unfixable,
document the exception inline rather than re-weakening the gate.
Test results:
- Before second-pass: 19/19, 80.2 / 62.5 / 75 / 85.88
- After (Critical + Major): 43/43, 93.57 / 78.04 / 86.36 / 96.96
CI may go red on the audit step until SDK 52 → 55 lands. Explicit
user decision in the audit session: "둘 다 지금 — CI 빨개질 가능성을 의도된 경고로 받음".
Minor finding from the 2026-05-21 second-pass: lib/auth-context.js:165 swallows a SecureStore JSON.parse failure without deleting the blob. Self-healing on next successful sign-in (setItemAsync overwrites), but the explicit-purge is cheap and the silent-swallow is the kind of thing that hides the next bug. Filed as inline TODO per audit policy — no separate PR.
The /code-review pass over the second-pass audit PR (#46) surfaced 15 defects. All addressed in this commit; one was REFUTED (lockfile drift was a local-pre-merge artifact, not on the tree under review). Code: - F4 lib/auth-context.js: handleAuthResult now enforces the same iss allowlist on acquisition that isSessionStillValid enforces on rehydration. The asymmetric enforcement let a success response with a missing or non-Google iss claim flip the UI to "signed in" and persist a blob that the next mount would delete — a visible flash of signed-in state for a session the system already knew was bad. - F2 app/(auth)/_layout.js: render an ActivityIndicator while loading instead of falling through to <Stack>. Without this, a signed-in user opening a /login deep link (or hitting /login during in-flight nav) flashed the login UI for ~50-200ms before the redirect kicked in. (app)/_layout's spinner does NOT cover this case because the route isn't underneath (app). - F14 lib/auth-context.js: response useEffect now keeps a `useRef` of the last-handled response to guard against re-firing handleAuthResult on StrictMode double-invoke or re-renders that keep `response` identity-stable. Without this, a malformed success response would re-throw on every render with no recovery path. Tests: - F1 tests/env.test.js: deleted. The babel-preset-expo inline-environment-variables plugin captures EXPO_PUBLIC_* at babel transform time, which makes the test non-deterministic between CI (no .env loaded -> compiled WEB === undefined -> throw fires) and local (.env loaded -> compiled WEB === 'real-value' -> throw doesn't fire). Verified by running EXPO_PUBLIC_GOOGLE_WEB_CLIENT_ID=x npx jest tests/env.test.js -> 2 failed; unset -> 2 passed. The assertGoogleEnv throw branch is still locked by signin-error.test.js which mocks lib/env entirely. - F5 + F10 tests/layout.test.js: rewrote the (auth)/_layout "while loading" test as a positive assertion that NEITHER Redirect NOR Stack fires during loading (own spinner). The prior wording cemented the flash-of-login bug as a contract. Also switched the redirect assertions to `expect.objectContaining` so future Redirect prop additions (e.g. `replace`) don't false-alarm the gate test. - F6 tests/auth-context.test.js: signIn happy-path test now hoists promptAsync + assertGoogleEnv to module-level jest.fn references and asserts both were called exactly once. The prior assertion was `expect(captured.error).toBeNull()` which is the initial state AND signIn's first line — passed even if signIn was a no-op. - F8 tests/auth-context.test.js: added an end-to-end test that drives a malformed success response through the mocked useAuthRequest channel and asserts the error surfaces to context.error. Locks the useEffect -> .catch(setError) integration that the security claim depends on. - F9 tests/auth-context.test.js: beforeEach now clears mockPromptAsync + mockAssertGoogleEnv + mockUseAuthRequestResponse. Without this, call counts accumulated across tests and any toHaveBeenCalledTimes(N) assertion would have been order-coupled. - F11 tests/signin-error.test.js: promptAsync mock now returns `Promise<{type:'cancel'}>` instead of bare `undefined`. The original unfaithful shape would silently let future tests pass against a non-Promise stand-in. - F12 tests/app.test.js: bumper sandbox tmpdirs are now tracked and purged in afterEach. Was leaking one tmpdir per test invocation. - F15 tests/auth-context.test.js: the "dismiss is a no-op" test was exercising pre-existing fall-through behaviour, not anything the second-pass audit added; renamed to make explicit that it's a conscious pin against a future "throw on unknown type" refactor, not a regression guard from the audit. Added F4 acquisition-iss regression tests alongside. CI / docs: - F3 .github/workflows/ci.yml: documented the CD / maintenance impact of the `--audit-level=high` raise. cd-android.yml, cd-ios.yml, and maintenance.yml reuse this workflow with `needs: ci`, so all three also fail at the audit step until Expo SDK 52 -> 55 lands. That's intentional; the comment now says so. - F13 CHANGELOG.md: added an [Unreleased] entry covering the security breaking change + migration guidance for users on legacy installs whose persisted session may be invalidated on first launch after upgrade. Refuted in verification: - V5 (package-lock.json out-of-sync) was an artifact of the local pre-merge branch tip. After merging origin/main the lockfile is consistent at eslint 10.4.0. Not addressable in code: - F7 (bump-version.js coverage invisible to istanbul) is real — the bumper runs via child_process so its lines don't count toward the coverage report. The behavioural tests still exercise every branch via stdout / file-state assertions, but the coverage gate has no visibility. Documented here rather than restructured, since the alternative (extracting bumper into a require()-able module) would bloat a 30-line script. Tests: 19 -> 44 (43 net + 1 from F8 integration, minus 4 deleted env tests, plus F4 regression tests). Coverage: 80.2 / 62.5 / 75 / 85.88 -> 94.64 / 80.45 / 90.47 / 98.01.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PR #46 (closed) was the second-pass audit fix; CI went red on the
npm audit --audit-level=highraise (intentional forcing function forthe Expo SDK 52 → 55 upgrade). This is the re-opened replacement with:
/code-reviewpass, addressedCritical (carried from #46) — iss bypass (CWE-345)
isSessionStillValidhadif (claims.iss && !ALLOWED_ISSUERS.has(claims.iss))— the leadingclaims.iss &&short-circuited for tokens missing theissclaim entirely, bypassing the Google allowlist promised by the docstring. Fix: requiretypeof claims.iss === 'string'AND allowlist membership.Major (carried from #46)
tests/app.test.jstrivially-passing replaced with 7 behavioural bumper tests;bump-version.jsrejects non-strict SemVer (was producing1.2.NaNon1.2.3-beta.1).handleAuthResultrejects success-without-id_token instead of persisting a userless blob.tests/layout.test.jscovers (app)/(auth) route-group gating.tests/signin-error.test.jscoversassertGoogleEnvthrow →setErrorintegration.Build verificationstep.Post-fix code-review fixes (this commit)
15 findings, all addressed except V5 (refuted) and F7 (documented):
tests/env.test.jsnon-deterministic (babel inlines EXPO_PUBLIC_* at compile time)tests/signin-error.test.js.(auth)/_layout.jsrendered<Stack>during loading → flash of login on session restore(app)/_layout.--audit-level=highblocks CD/maintenance, not just feature PRsci.ymlcomment +CHANGELOG.md.handleAuthResultacquisition path missing iss check (asymmetric with rehydration)layout.test.jscemented flash-of-login as contracterror===null)bump-version.jscoverage invisible to istanbul (child process)handleAuthResultthrow untested via useEffect integrationmockUseAuthRequestResponse.assertGoogleEnvmock call count not cleared in beforeEachtoHaveBeenCalledWith({href:'/login'})exact-match brittleexpect.objectContaining.signin-error.test.jspromptAsync mock returned undefined not Promisejest.fn(async () => ({type:'cancel'})).fs.mkdtempSyncsandboxes never cleaned upafterEachpurge.fix(auth)!breaking change had no CHANGELOG entry[Unreleased]migration note added.handledResponseRefguard.package-lock.jsonout of sync after mergeTest results
Expected CI behaviour
npm audit --audit-level=highwill fail until Expo SDK 52 → 55 lands.That is the intended forcing function and is now documented inline in
ci.ymlandCHANGELOG.md. The CD pipelines (cd-android,cd-ios)and
maintenance.ymlwill also fail at that step for the same reason— releasing on top of a known-vulnerable transitive tree is exactly
what this gate is meant to prevent.
Two paths to land: