test: close the coverage the audit round left open by msalvatti · Pull Request #111 · bymaxone/rust-auth

msalvatti · 2026-08-02T01:22:20Z

7 of 7 in the fifth audit round. Base: audit5/06-nextjs-edge (PR 6).

This one closes the coverage the six commits before it left open. The gate
(--fail-under-lines 100 --fail-under-functions 100) was green before the round and 99.79%
after it — 52 lines across twelve files, every one a path the round's own commits added and
nothing exercised.

Most of them are the refusals that make the round's fixes mean anything

the MFA transitions abandoning when a disable completed under their lock — on all three
paths (the dashboard splice, the platform splice, the retired-key rewrite);
a grace recovery refused after the account was swept, on both planes;
an MFA temp token that stops verifying once the epoch moves — the credential a password
reset used not to reach;
a repository failure propagated rather than read as "no such account", which on a status
gate is the difference between refusing and admitting.

None can be produced against a coherent store single-threaded, so the in-memory doubles gained
narrow knobs that arm each window. The MFA one is tied to the transition lock rather than to
a count of reads: the lock is the boundary itself, so the caller's copy is read before it and
the transition's copy after.

Two were not tests at all

let Ok(x) = expr else { return; } broken across lines puts the return on a line no run
reaches, and llvm-cov counts it. The suite already knew this — token_manager's helpers carry a
comment about it — and the new tests reintroduced it.

The same applies to a matches! that opens its own line inside an assert!. The summary
counts a line by its FIRST segment, so the macro's non-matching arm makes that line uncovered
while every per-line report (lcov, json, text, html) shows the file clean. That divergence cost
several full measurements; the rule is worth knowing before writing the next test.

Verification

At this commit, with the workflow's exact command
(--workspace --all-features --locked --fail-under-lines 100 --fail-under-functions 100 --lcov):
exit 0 — 25442 lines, 0 missed, 100% lines and functions. fmt, clippy -D warnings, and
892 tests pass. Every commit in the stack was also verified in isolation.

Copilot

Pull request overview

This PR closes coverage gaps introduced in the audit5 stack by adding targeted tests for previously-unexercised refusal/error paths and by extending the in-memory testing doubles with knobs to deterministically reproduce race-window outcomes.

Changes:

Extend in-memory repositories/stores with controllable failure/behavior flags to simulate otherwise-unreachable interleavings (MFA transition window, forced read failures, forced recovery refusals, forced epoch bump failures).
Add/adjust tests across auth/session/token/MFA/platform/adapter/rate-limit modules to exercise refusal and error-propagation paths and eliminate llvm-cov “unreachable return line” artifacts.
Minor test refactors to assert intermediate steps explicitly (reducing silent early-returns in multi-step tests).

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
crates/bymax-auth-core/src/testing/mod.rs	Adds new in-memory test-double knobs (tenant-ignore lookup, forced read failures, forced recovery refusal, MFA-vanish window, forced epoch bump failures).
crates/bymax-auth-core/src/services/token_manager.rs	Adds tests for swept-account recovery refusal and MFA temp-token invalidation after epoch bump.
crates/bymax-auth-core/src/services/session.rs	Adds coverage ensuring revoke-all sweeps unused grace pointers and exercises recovery/session paths.
crates/bymax-auth-core/src/services/platform.rs	Refactors tests to use explicit assertions instead of multi-line let-else early returns.
crates/bymax-auth-core/src/services/mfa/tests.rs	Adds tests ensuring MFA transitions/splices/rewrites abandon when MFA vanishes under the transition lock.
crates/bymax-auth-core/src/services/auth/session_ops.rs	Minor test refactor to avoid multi-line let-else coverage artifacts.
crates/bymax-auth-core/src/services/auth/login.rs	Adds a test for refusing mis-scoped tenant results from a misconfigured repository.
crates/bymax-auth-core/src/services/adapter_api.rs	Adds tests for epoch-bump failure propagation, status-gate repository failure propagation, and WS ticket refusal cases.
crates/bymax-auth-axum/src/rate_limit.rs	Adds tests covering key-sweeper spawn behavior inside/outside a Tokio runtime.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+        // Reaching here at all is the assertion: the sweeper ran on a live limiter without
+        // taking the runtime down, which is the failure mode a panic in a detached task has.
+        assert!(config.limiter().len() < usize::MAX);


+        if let Some(flag) = lock(&self.disable_mfa_on_lock).take() {
+            flag.store(true, Ordering::SeqCst);
+        }
        match lock(&self.mfa_locks).entry(lock_id.to_owned()) {
            Entry::Occupied(_) => Ok(false),


Three store operations existed only as sequences of round trips the engine performed itself, and each sequence had a window. `create_recovered_session` writes the session a grace recovery produced in one step, gated on the per-user session index still existing. `refresh_rotate.lua` moved the index bookkeeping inside itself precisely to close the window in which "log out everywhere" could miss a session a rotation had just minted — the GRACE arm was left outside it. The script returned the recovered record and the engine wrote the `rt:`, `sess:`, `sd:` and `fam:` keys several round trips later, so a `revoke_all` landing in between swept an index that did not yet contain the session. The session survived a revocation the user was told had happened, and its access token, signed afterwards, carried the POST-bump epoch and verified. An attacker holding a stolen token gets one grace-eligible token per rotation, so they can keep a continuous stream of these in flight for as long as the victim's reset takes. The witness is the session index: `invalidate_user_sessions.lua` deletes that set once it has removed every member, so its absence is exactly "a revoke-all has run", and the successor the grace pointer named is itself indexed, so a legitimate recovery always finds it present. `sweep_grace_pointers` removes every `rp:` pointer an account holds. `list_sessions` filters them out by construction and `revoke_session` touches only the primary refresh keys, so `revoke_all_except_current` left them behind. For a revoked session that is harmless — the grace branch needs the successor's `rt:` key and it is gone. The gap is the session deliberately KEPT: its predecessor's pointer names a hash that is still alive, so whoever holds that predecessor token could take the grace branch and mint a brand-new full-lifetime session for the rest of the window, right after the user asked to sign out their other devices. `acquire_mfa_lock` / `release_mfa_lock` back the per-account MFA transition lock the engine needs to stop concurrent MFA writes undoing each other. `claim_recovery_code` does not serve: it is keyed on the code, so it serializes two attempts at the SAME code and nothing else. The in-memory ws-ticket double now mints the shape the real store mints — 64 lower-case hex rather than `wst-0`. A double whose output the engine would refuse cannot exercise the path it stands in for.

…y owner Every MFA transition rewrites one repository record carrying `mfa_enabled`, the encrypted secret and the recovery-code digests together, and `update_mfa` replaces all three wholesale — the repository is the consumer's and offers no compare-and-set, so the engine cannot add one. Read-modify-write over that with no serialization is last-write-wins, and three things fell out of it: two challenges spending different recovery codes each wrote the full list minus their own, resurrecting the loser's code; a challenge that read the list before `regenerate_recovery_codes` and spliced after it restored the whole replaced set, unspending codes the user rotated because they had leaked; and a challenge that spliced after `disable` completed wrote `mfa_enabled: true` back with the pre-disable secret. `claim_recovery_code` covers none of them — it is keyed on the code, so it serializes two attempts at the same code and nothing else. The transition now takes a per-account lock and re-reads the record inside it, so a mutation always sees the record as it stands rather than the copy its caller read. A caller that cannot take the lock is refused with `MFA_STATE_CONFLICT` rather than made to wait: concurrent state changes on one account are pathological, and "try again" is the honest answer. The lock carries a per-call token and is released by compare-and-delete. A fixed value would not survive its own TTL: ten seconds is short, the transition calls into the consumer's repository twice, and a run that overruns has already lost the lock when it releases — an unconditional delete then removes whichever transition holds it now, letting a third caller in beside the second. `GET` then `DEL` cannot express the check either, since the key can be retaken between the two round trips, so the comparison and the delete are one script.

…ups to the tenant A refresh rotation re-issued the access token from the session record, which carries the identity but not the authorization. Only some claims were re-read from the account, so a role change, a tenant move or an MFA enrolment landed in the new token for one of them and not the others — and the claim that was not re-read kept its login-time value for as long as the session lived. The condition now covers every claim the guards actually decide on, so a rotation either describes the account as it stands or it re-stamps. The tenant a repository answers with must be the tenant that was asked for. The lookup passes it and the contract says to scope by it, but the repository is the consumer's and a trait can only ask — a single-tenant host writing `find_by_email` that ignores its second argument is the shape nobody notices. Under one, every distinct tenant in the request body resolves the same account while deriving a different lockout counter, so rotating the field yields an unlimited supply of fresh attempt budgets and the lockout never engages. The mismatch is refused, folded into the same branch as the not-found case so the refusal stays indistinguishable.

…ate, cap upstream reads The WebSocket ticket authorizes a socket for its whole lifetime — there is no per-request gate behind it — so its snapshot is the last chance to describe the account correctly. It was copied from the token, whose status claim rotation stamps empty by construction, so every ticket minted from a rotated token carried a blank authorization field for as long as the socket stayed open. It is now read from the account. The email-change confirmation trusted the address recorded when the change was requested. Between the request and the confirmation the account can be banned or the address can be claimed by someone else, and neither was re-checked at the moment the write happened. The HTTP provider read the upstream response without a bound. A body that never ends is a memory exhaustion the consumer cannot see: nothing in the type says the read is unbounded, and the failure arrives as an allocator abort rather than an error the caller can handle.

…er keyspace The adapter picked how the per-route limit is keyed. Neither choice can be a default: reading the socket address behind any proxy gives the proxy's address for every client, so all of them share one bucket and a single caller sending a handful of logins locks out the whole user base with no credential; reading the forwarded address on a directly exposed service gives whatever the caller wrote, and a limiter whose key the attacker picks enforces nothing. Both look like a working limiter at runtime and nothing detects the mismatch, so the deployment states which shape it is or the adapter refuses to build. The limiter's key map also grew without bound: an entry per distinct client address, retained for the process's life, with nothing to remove the ones whose window had long expired. A background sweep now reclaims them.

…t is not one The edge helper fell back to a decode when no secret was configured, and the two branches returned the same shape with the same success flag — so a caller reading it, the natural reading of a function named `verify`, admitted a token an attacker minted the moment the secret went missing. An unset environment variable was enough to arrange that. It now refuses, and the decode-only read stays available under its own correctly-named entry point. `redirectToPath` also states its same-origin invariant explicitly rather than relying on `URL` resolution to imply it. The open redirect was already closed here — resolving against the placeholder base and taking `pathname` drops any authority — but it closed it by substituting a different destination for the one the caller asked for, and reported nothing. Of the shapes now refused, two were reachable before: a bare relative reference, which the browser resolves against the current directory, and a control character, which ends the header and lets what follows be read as another one.

…ould not reach The line-coverage gate was at 100% before this round and at 99.79% after it: 52 lines across twelve files, every one of them a path the round's own commits added and nothing exercised. Most were the refusal arms that make the round's fixes mean anything — the MFA transitions abandoning when a `disable` completed under their lock, on all three paths; the grace recovery refused after the account was swept, on both planes; an MFA temp token that stops verifying once the epoch moves; a repository failure propagated rather than read as "no such account". None can be reached against a coherent store single-threaded, so the doubles gained the narrow knobs that arm each window — the transition lock now raises the flag that makes the re-read report MFA gone, which is the boundary itself rather than a count of reads. Two were not tests at all. `let Ok(x) = expr else { return; }` broken across lines puts the `return` on a line no run reaches and llvm-cov counts; the suite already knew this — `token_manager`'s helpers carry a comment about it — and the new tests had reintroduced it. The same applies to a `matches!` that opens its own line inside an `assert!`: the summary counts a line by its FIRST segment, so the macro's non-matching arm makes that line uncovered even though every per-line report shows the file clean.

Copilot

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

Suppressed comments (3)

crates/bymax-auth-core/src/testing/mod.rs:380

Like the dashboard repository, this MFA-gone simulation only adjusts the returned clone and does not update the stored admin record. That can hide incorrect writes in tests (reads will keep reporting MFA disabled regardless of what was stored).

    async fn find_by_id(&self, id: &str) -> Result<Option<AuthPlatformUser>, RepositoryError> {
        let answer = lock(&self.users).get(id).cloned();
        let gone = lock(&self.mfa_gone_flag)
            .as_ref()
            .is_some_and(|flag| flag.load(Ordering::SeqCst));
        if gone {

crates/bymax-auth-axum/src/rate_limit.rs:575

This assertion is tautological (len() is always <= usize::MAX), so the test doesn’t actually validate that the sweeper ran (or even that the spawned task stayed healthy). If the goal is just “no panic”, the test can simply return successfully without a meaningless assert, or be reworked to assert an observable effect.

        // Reaching here at all is the assertion: the sweeper ran on a live limiter without
        // taking the runtime down, which is the failure mode a panic in a detached task has.
        assert!(config.limiter().len() < usize::MAX);

crates/bymax-auth-core/src/testing/mod.rs:176

mfa_reported_gone() currently only mutates the cloned answer returned from find_by_id and does not update the stored user record. That can mask bugs in tests that assert “no write happened”, because later reads will still be forced to report MFA disabled even if a transition incorrectly re-enabled MFA in storage.

This issue also appears on line 375 of the same file.

        // The flag is raised by the transition lock, so a read taken before it sees the account
        // as it was and a read taken after it sees the `disable` that completed in between.
        if self.mfa_reported_gone() {
            return Ok(answer.map(|mut user| {
                user.mfa_enabled = false;

msalvatti · 2026-08-02T12:22:26Z

Closing: this PR's tests have been redistributed into the commits whose code they cover.

Holding every coverage test at the top of the stack meant each PR below it reported a coverage figure it could not have earned, and #110 also failed the browser E2E for an unrelated reason. Both are addressed by rebuilding the stack:

session.rs tests → fix(redis, core): give the store seam the atomic writes the engine needs #105, mfa/tests.rs + testing/mod.rs → fix(core, redis): serialize MFA state transitions, release the lock by owner #106, login.rs / session_ops.rs / platform.rs / token_manager.rs → fix(core, jwt): re-stamp every authority claim on rotation, bind lookups to the tenant #107, adapter_api.rs → fix(core): scope the WebSocket ticket snapshot and the email-change gate, cap upstream reads #108, rate_limit.rs → fix(axum)!: require the rate-limit client-IP source, reclaim the limiter keyspace #109.
fix(nextjs)!: fail closed without a verifier secret, refuse a path that is not one #110 additionally now emits an absolute Location. Next hands a middleware's Location to new NextURL(location, ...) with no base, so the relative form this stack introduced threw ERR_INVALID_URL before the response was sent — which is what the Playwright job was reporting. Next re-relativises a same-host redirect itself, so the property the relative form was protecting is preserved.

The rebuilt stack tip is byte-identical to this branch apart from that fix, so nothing here is lost.

msalvatti requested a review from Copilot August 2, 2026 01:22

Copilot started reviewing on behalf of msalvatti August 2, 2026 01:22 View session

Copilot AI reviewed Aug 2, 2026

View reviewed changes

msalvatti mentioned this pull request Aug 2, 2026

fix(redis, core): give the store seam the atomic writes the engine needs #105

Merged

msalvatti added 7 commits August 2, 2026 00:52

msalvatti force-pushed the audit5/06-nextjs-edge branch from d6aacd4 to c7cbed2 Compare August 2, 2026 08:27

msalvatti force-pushed the audit5/07-coverage branch from 9a02aa9 to f5650c2 Compare August 2, 2026 08:32

msalvatti requested a review from Copilot August 2, 2026 08:32

Copilot started reviewing on behalf of msalvatti August 2, 2026 08:33 View session

Copilot AI reviewed Aug 2, 2026

View reviewed changes

msalvatti force-pushed the audit5/06-nextjs-edge branch from c7cbed2 to 996e3ef Compare August 2, 2026 12:22

msalvatti closed this Aug 2, 2026

msalvatti deleted the audit5/07-coverage branch August 2, 2026 12:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

test: close the coverage the audit round left open - #111

test: close the coverage the audit round left open#111
msalvatti wants to merge 7 commits into
audit5/06-nextjs-edgefrom
audit5/07-coverage

msalvatti commented Aug 2, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

msalvatti commented Aug 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

msalvatti commented Aug 2, 2026

Most of them are the refusals that make the round's fixes mean anything

Two were not tests at all

Verification

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

msalvatti commented Aug 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants