fix: second-pass audit — iss bypass + 7 catch-rate gaps#46
Open
heznpc wants to merge 3 commits into
Open
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 빨개질 가능성을 의도된 경고로 받음".
heznpc
changed the title
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.
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
Two-pass audit on 2026-05-21 of a session that had reported "tests pass / CI green". Caught one Critical (security boundary bypass) and seven Major items (entirely from test/CI catch-rate gaps in the prior audit).
Critical — iss bypass (CWE-345)
lib/auth-context.jsisSessionStillValidhad:The leading
claims.iss &&short-circuits for tokens missing theissclaim entirely, so an iss-less id_token bypassed the Google-only allowlist that the docstring promised. Threat model: an attacker who can write to SecureStore (rooted/jailbroken device, shared-keychain entitlement bug, hostile companion app) crafts an iss-less blob and rides the session.Fix: require
typeof claims.iss === 'string'AND allowlist membership.Major — second commit on this branch
fs.existsSync(bumperPath)was checkedhandleAuthResultrejects success-without-id_token instead of persisting{user:null,...}tests/layout.test.js)tests/env.test.js, NEWtests/signin-error.test.js)expo export --platform web 2>/dev/null || echo "skipping"build-verify stepnpm auditthreshold fromcriticaltohighTest results
Expected CI noise
The audit step will likely fail until the Expo SDK 52 → 55 upgrade lands (tracked in the parallel modernization audit). That is the intended forcing function — do not re-weaken the gate to silence it.
Previously declared done; second-pass caught
feat(privacy)) without negative-path probingtests/app.test.jsfrom initial scaffold, never revisitedchore(security): pin gitleaks) — the rationale ("Expo transitive can't be fixed") is exactly the constraint the SDK upgrade dissolvesFiled as retrospective evidence that "tests pass + CI green" ≠ "the function does what its docstring claims" ≠ "the CI gate is doing what its name suggests."