feat(auth): harden the login-approval gate (BUNYIP-375) #373

Merged

nrupard merged 1 commit from feat/BUNYIP-375-approval-gate-hardening into main

2026-07-16 16:29:05 +02:00

nrupard commented

2026-07-16 16:11:02 +02:00

Owner

Summary

Follow-up hardening for the suspicious-login approval gate (umbrella issue BUNYIP-375). Collects the four deferred code-review items from #372 (BUNYIP-373, now merged to main) into one pass. Targets main directly; the diff is just the four items.

Changes

Bind each challenge to its own code row. complete_login_approval matched the emitted code against the user's latest valid login_approval_codes row, so with two concurrent gated logins for one user only the newest code could complete. A new migration adds login_approval_codes.challenge_jti; JwtService::create_login_approval_challenge_token now returns its jti; the row stores it and completion resolves by (user_id, challenge_jti). Each emitted code completes only its own challenge.
Fail closed on an approval-email send failure. begin_login_approval logged the failure but still returned ApprovalRequired, leaving the client waiting for a code that never arrives. It now returns a retryable AppError::upstream so the UI can prompt a fresh attempt; the abandoned challenge row expires via TTL and is swept by the cleanup below. (Fail-closed is the correct posture for a security gate; a restart already works by re-POSTing /login.)
Constant-time code comparison via subtle::ConstantTimeEq (the crate bunyip-oidc already uses for PKCE). Defense-in-depth only, documented as such: the compared values are already SHA-256 hashes, so a timing leak would expose a hash prefix that still needs a preimage to yield the code.
Purge spent challenges. cleanup_expired_tokens now deletes expired-or-used login_approval_codes rows. login_devices is intentionally left intact: it is the persistent known-device baseline, and pruning it would re-gate returning users. (A login_devices staleness prune is a deferred YAGNI item, noted on BUNYIP-375.)

Testing

just check-container green (fmt + clippy -D warnings + cargo test --workspace --all-targets). The env-gated integration test (bunyip-api/tests/login_approval.rs, skips without RLS_TEST_DATABASE_URL) gains a direct check of the jti binding: a code cannot complete a different challenge, even the newer one. The fail-closed email branch is not exercised in-test because EmailService::new_dev() suppresses sending (no failure path to trip); the branch is small and covered by review.

Deferred (noted on BUNYIP-375)

login_devices staleness/size bound - left unbounded until real row counts warrant it.

## Summary Follow-up hardening for the suspicious-login approval gate (umbrella issue BUNYIP-375). Collects the four deferred code-review items from #372 (BUNYIP-373, now merged to `main`) into one pass. Targets `main` directly; the diff is just the four items. ## Changes 1. **Bind each challenge to its own code row.** `complete_login_approval` matched the emitted code against the user's *latest* valid `login_approval_codes` row, so with two concurrent gated logins for one user only the newest code could complete. A new migration adds `login_approval_codes.challenge_jti`; `JwtService::create_login_approval_challenge_token` now returns its `jti`; the row stores it and completion resolves by `(user_id, challenge_jti)`. Each emitted code completes only its own challenge. 2. **Fail closed on an approval-email send failure.** `begin_login_approval` logged the failure but still returned `ApprovalRequired`, leaving the client waiting for a code that never arrives. It now returns a retryable `AppError::upstream` so the UI can prompt a fresh attempt; the abandoned challenge row expires via TTL and is swept by the cleanup below. (Fail-closed is the correct posture for a security gate; a restart already works by re-POSTing `/login`.) 3. **Constant-time code comparison** via `subtle::ConstantTimeEq` (the crate `bunyip-oidc` already uses for PKCE). Defense-in-depth only, documented as such: the compared values are already SHA-256 hashes, so a timing leak would expose a hash prefix that still needs a preimage to yield the code. 4. **Purge spent challenges.** `cleanup_expired_tokens` now deletes expired-or-used `login_approval_codes` rows. `login_devices` is intentionally left intact: it is the persistent known-device baseline, and pruning it would re-gate returning users. (A `login_devices` staleness prune is a deferred YAGNI item, noted on BUNYIP-375.) ## Testing `just check-container` green (fmt + clippy `-D warnings` + `cargo test --workspace --all-targets`). The env-gated integration test (`bunyip-api/tests/login_approval.rs`, skips without `RLS_TEST_DATABASE_URL`) gains a direct check of the jti binding: a code cannot complete a different challenge, even the newer one. The fail-closed email branch is not exercised in-test because `EmailService::new_dev()` suppresses sending (no failure path to trip); the branch is small and covered by review. ## Deferred (noted on BUNYIP-375) `login_devices` staleness/size bound - left unbounded until real row counts warrant it.

nrupard added 1 commit

2026-07-16 16:11:02 +02:00

feat(auth): harden the login-approval gate (BUNYIP-375)

E2E / Playwright against deployment (pull_request) Successful in 25s

Details

Check / fmt + clippy + build + tests (pull_request) Successful in 18m0s

Details

Create release / Create release from merged PR (pull_request) Has been skipped

Details

5f186919a5

Follow-up hardening on the BUNYIP-373 suspicious-login gate, stacked on that PR. Four items from code review:

- Bind each challenge to its own code row. complete_login_approval matched the emitted code against the user's LATEST valid login_approval_codes row, so two concurrent gated logins for one user let only the newest code complete. Persist the challenge JWT's jti (new migration adds login_approval_codes.challenge_jti; create_login_approval_challenge_token now returns its jti) and resolve the row by (user_id, jti), so each emitted code completes only its own challenge.

- Fail closed on an approval-email send failure. begin_login_approval logged the failure but still returned ApprovalRequired, leaving the client waiting for a code that never arrives (stuck until the TTL); it now returns a retryable AppError::upstream so the UI can prompt a fresh attempt. The abandoned challenge row expires via TTL and is swept by the cleanup below.

- Constant-time code comparison via subtle::ConstantTimeEq (the crate bunyip-oidc already uses for PKCE). Defense-in-depth only: the compared values are already SHA-256 hashes, so a timing leak would expose a hash prefix that still needs a preimage to yield the code.

- Purge spent challenges. cleanup_expired_tokens now deletes expired-or-used login_approval_codes rows. login_devices is intentionally left intact: it is the persistent known-device baseline, and pruning it would re-gate returning users.

Integration test extended to cover the two-concurrent-challenge binding directly (a code cannot complete a different, even newer, challenge). check-container green.

#BUNYIP-375

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

nrupard scheduled this pull request to auto merge when all checks succeed

2026-07-16 16:21:14 +02:00