fix(auth): scope the status gate to the tenant, and gate MFA on a verified address - #139
Merged
Conversation
…ified address Two findings from a parity audit against nest-auth's local audit branch. The first is ours alone; the second is a gap the audit surfaced. AuthEngine::assert_user_active resolved the account with find_by_id(sub, None), and UserRepository documents None as being for "internal admin flows where cross-tenant access is intentional". A status gate on a request path is not one of those. A repository id is unique only within a tenant, so with a host whose ids are per-tenant serials the gate could resolve a DIFFERENT tenant's account and decide on its status -- admitting a caller whose own account is banned, or refusing one who is fine. The verified token carries tenant_id, so there was nothing to guess. The MFA management routes (setup, verify-enable, disable, recovery-codes) took AuthUser: a valid token and nothing else. An account whose address nobody had proven could enrol, remove or re-roll a second factor -- binding that factor, and the recovery path that runs back through the same address, to a mailbox that may not be the holder's. They now take a new VerifiedUser extractor: account status AND address verification. Deliberate non-changes, each for a reason: - /auth/mfa/challenge is not gated. It is part of signing in, not of managing the factor. - GET /auth/me stays on AuthUser alone. A pending, suspended or unverified client has to read its own profile to render the "verify your email" or "suspended" screen, and SafeAuthUser carries status and emailVerified for exactly that. Gating it leaves that client a 403 and nothing to render. - UserStatus is unchanged and still gates status alone, so ws-ticket, password change and the session routes do not silently acquire a second gate. - The verification half is conditional on email_verification.required, as the login path is: a deployment that never asks for verification would otherwise find these routes permanently unreachable. One test changed meaning and was renamed rather than left to rot. The suite reached the handlers' error arms with a "ghost" token -- one minted for a subject no repository row backs -- which the status gate now turns away before the handler runs. That is the better behaviour, so the ghost test now pins the refusal (renamed to say so) and a new test drives those error arms with a real account that passes every gate and asks for something the engine refuses. The old assertions were assert_ne!(status, <success>), which would not have noticed the response changing from one 4xx to another; the new ones pin the exact error.code. fmt, clippy -D warnings, rustdoc -D warnings, the full suite, and llvm-cov at 100% lines and 100% functions.
There was a problem hiding this comment.
Pull request overview
Scopes status checks by tenant and adds verified-address gating to MFA management.
Changes:
- Adds tenant-aware account status checks.
- Introduces and exports
VerifiedUser. - Updates MFA routes and regression tests.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
CHANGELOG.md |
Documents security changes. |
crates/bymax-auth-core/src/services/adapter_api.rs |
Adds scoped status and verification gates. |
crates/bymax-auth-axum/src/lib.rs |
Exports VerifiedUser. |
crates/bymax-auth-axum/src/extractors/mod.rs |
Re-exports the extractor. |
crates/bymax-auth-axum/src/extractors/status.rs |
Implements tenant-aware extractors and tests. |
crates/bymax-auth-axum/src/routes/mfa.rs |
Gates MFA-management routes. |
crates/bymax-auth-axum/tests/adapter.rs |
Updates MFA error-path coverage. |
Suppressed comments (3)
crates/bymax-auth-axum/src/routes/mfa.rs:113
- This gate verifies a tenant-scoped account, but
mfa_verify_enablereceives only the non-globalsub; the core service subsequently does an unscoped dashboard lookup and uses tenantless MFA keys/writes. A verified account in one tenant can therefore pass this gate while the enable operation targets a same-ID account in another tenant. Pass the verified claims' tenant through the complete dashboard MFA operation.
// Status + email-verified, alongside the token: enrolling or removing a second factor on an
// account whose address nobody has proven binds the factor — and its recovery path — to a
// mailbox that may not be the holder's. `challenge` below is deliberately NOT gated: it is
// part of signing in, not of managing the factor.
_gate: VerifiedUser,
crates/bymax-auth-axum/src/routes/mfa.rs:200
- The new gate is tenant-scoped, but
mfa_disablestill receives onlysub;MfaService::fetch_user_mfareads dashboard users withfind_by_id(user_id, None)and the transition write is tenantless. For per-tenant IDs, this can validate tenant A and disable MFA for the same ID in tenant B. Carrytenant_idthrough the dashboard MFA service and all related keys and writes.
// Status + email-verified, alongside the token: enrolling or removing a second factor on an
// account whose address nobody has proven binds the factor — and its recovery path — to a
// mailbox that may not be the holder's. `challenge` below is deliberately NOT gated: it is
// part of signing in, not of managing the factor.
_gate: VerifiedUser,
crates/bymax-auth-axum/src/routes/mfa.rs:229
VerifiedUservalidates the account withinclaims.tenant_id, but recovery-code regeneration then drops that tenant and calls a core path whose dashboard lookup, lock/store keys, and write are keyed only byuser_id. Under the PR's per-tenant-ID model, the checked account and mutated account can differ. Thread the tenant through the entire regeneration operation.
// Status + email-verified, alongside the token: enrolling or removing a second factor on an
// account whose address nobody has proven binds the factor — and its recovery path — to a
// mailbox that may not be the holder's. `challenge` below is deliberately NOT gated: it is
// part of signing in, not of managing the factor.
_gate: VerifiedUser,
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Four findings, all accepted. The first was the substantive one: VerifiedUser scoped its read to the token's tenant, but the handler then called into MfaService, whose fetch_user_mfa re-read dashboard users with find_by_id(user_id, None). The gate checked one account and the operation could read -- and mutate -- the same id in another tenant, which is worse than no gate at all because it looks correct. tenant_id is now threaded through fetch_user_mfa, transition_mfa_record/transition_locked, the five MfaService entry points, the four engine methods and the routes. The platform plane passes None: an operator is not tenant-scoped. NOT fixed here, and stated so rather than left implied: the MFA Redis keys (mfa:, mfa_setup:, mfalock:) are derived from hmac(plane:user_id) and carry no tenant, so colliding ids across tenants still share a keyspace. Changing that preimage is a shared-Redis contract change that has to land in nest-auth in the same breath, byte-for-byte, or conformance breaks. It needs a cross-repo decision, not a unilateral one. The other three were weaknesses in this PR's own tests and docs: - The ghost-subject test asserted only assert_ne!(status, <success>). Since the handlers refuse a ghost too, it would have stayed green with the gate removed -- the exact claim its new name makes. It now pins UNAUTHORIZED and auth.token_invalid. - The tenant-scoping test called the engine directly with another tenant, which proves the engine scopes but not that the extractor passes the tenant; it would have kept passing if the extractor regressed to None. It now drives the extractor with a token whose claims name a tenant the account does not belong to, and was confirmed to fail when the extractor is reverted to None. - CHANGELOG had a second ### Security heading under [Unreleased], splitting one category in two. fmt, clippy -D warnings, rustdoc -D warnings, the full suite, and llvm-cov at 100% lines and 100% functions.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
crates/bymax-auth-core/src/services/mfa/tests.rs:1385
- This is the only platform MFA call that supplies a tenant, contradicting the documented contract in
services/mfa/mod.rs:554-557and the platform routes, which consistently passNone. Although the platform branch currently ignores this value, the test should exercise the supported API contract rather than mask an invalid context/scope combination.
Some("t1")
| // Re-read inside the lock. The caller's copy was read before the lock existed and may | ||
| // already be stale — reusing it would leave exactly the window this closes. | ||
| let current = self.fetch_user_mfa(user_id, ctx).await?; | ||
| let current = self.fetch_user_mfa(user_id, ctx, tenant_id).await?; |
msalvatti
added a commit
that referenced
this pull request
Aug 10, 2026
…nd write by it (#141) The MFA temp token carried no tenant, so the challenge had none in scope and resolved its subject with `find_by_id(sub, None)` — by id, across every tenant. Everything downstream then ran on whatever row the repository returned: the status gate, `mfa_enabled`, the secret it decrypts, the recovery digests it scans, and the account the session is finally minted for. A library cannot assume the host's ids are unique across tenants — `find_by_id` takes a tenant precisely because they may not be — and under a schema that numbers users per tenant, every tenant has a user `1`. Same `find_by_id(sub, None)` class closed for the status gate in #139, still live on the challenge path because there was no tenant to pass. The token now carries `tenantId` on the dashboard arm: optional on the wire, mandatory in effect. A dashboard token without it is refused, a platform token with one is refused, and both issuance and verification consult one predicate, so a host can no longer be handed a signed credential that can never verify. Falling back would leave the vulnerable derivation reachable by omitting an optional field. RFC 8725 3.9 sets absent-means-reject, 3.12 asks for mutually exclusive validation rules, and ASVS 5.0 6.6.2 requires the out-of-band token be bound to the authentication request that generated it. Eight key derivations were scoped by plane but not by tenant, not five: the transition lock, the pending-enrolment record, the recent-auth marker, the TOTP anti-replay marker, the recovery-code claim, and the challenge, disable and reauth failure counters. The three counters were the worst and it is not close — the other five cost a collision between two accounts sharing an id, while a shared counter is a credential-free cross-tenant lockout, which is not the per-subscriber-account rate limiting NIST SP 800-63B requires. All eight now derive from one `scoped_subject`, driven by the plane rather than by whether a tenant was supplied, so a platform caller passing one cannot move the platform preimage off `platform:{userId}`. Scoping the read alone was not enough. `UserRepository::update_mfa` carried no tenant, so the write every transition performs could land on another tenant's row, or miss the row this flow read and leave a spent recovery code usable once the 300 s `rcu:` claim lapsed — which the existing single-use test could not catch, because the claim refuses the second attempt either way. The trait now takes `tenant_id`, and the regression asserts the persisted digest list shrinks in the tenant's own row. Passing the argument is only a request, so the challenge also filters the answer, as `login` already did. The contract pinned two of the eight, leaving six derivations able to drift undetected — the blind spot that let the OTP stored value move unnoticed. All eight are now named under `mfaSubjectPreimages` and `mfaSubjectDerivedKeys`, enforced by a test that reads the file and compares it against what the code builds. `identifierPreimages` is untouched: it is the email-keyed `lf:` login preimage and was already correct. Also re-anchors the `issue_mfa_temp_token` equivalent-mutant exclusion, stale on `main` since before this change. It surfaced both mutants as survivors, but the sweep runs post-merge, so a broken anchor there does not fail closed. Gates: fmt, clippy -D warnings, rustdoc -D warnings, full suite, llvm-cov lines 100% / functions 100%, cargo-mutants in-diff 71 tested, 64 caught, 7 unviable, 0 survivors. The cross-tenant and recovery-splice tests were red-checked.
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
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.
What
Two findings from auditing rust-auth against the nest-auth security work in flight. The first is ours alone; the second is a gap that audit surfaced.
Important
Read this before recording it as "parity reached". The email-verified half is not rust-auth catching up. nest-auth's
UserStatusGuardon main (ef1c6b3, release 1.3.1) gates account status only — it has no email-verified check. That half exists today only on nest-auth's unmerged branch (their PR #98). This PR therefore puts rust-auth ahead of nest-auth main on that point, deliberately and on its own merits, not behind it. The tenant-scoping fix below is independent of nest-auth entirely.1. The status gate ignored the tenant
AuthEngine::assert_user_activeresolved the account with:UserRepositorydocuments that argument in as many words:A status gate on a request path is not an internal admin flow. A repository id is unique only within a tenant, so with a host whose ids are per-tenant serials the gate could resolve a different tenant's account and decide on that account's status — admitting a caller whose own account is banned, or refusing one who is fine. The verified token carries
tenant_id, so there was nothing to guess.assert_user_activenow takes the tenant, and theUserStatusextractor passes it from the verified claims.2. MFA management accepted an unproven address
POST /auth/mfa/setup,/verify-enable,/disableand/recovery-codestookAuthUser— a valid token and nothing else. An account whose address nobody had proven could enrol, remove or re-roll a second factor. Binding a factor to an unproven address binds it, and the recovery path that runs back through that same address, to a mailbox that may not be the holder's.They now take a new
VerifiedUserextractor: account status and address verification.Deliberate non-changes
POST /auth/mfa/challengeGET /auth/meSafeAuthUsercarriesstatusandemailVerifiedfor exactly that. Gating it leaves that client a 403 and nothing to render.UserStatusThe verification half is conditional on
email_verification.required, exactly as the login path is — a deployment that never asks for verification would otherwise find these routes permanently unreachable.A test that changed meaning, and was not left to rot
The suite reached the handlers' error arms with a ghost token — one minted for a subject no repository row backs — so the engine fetched, failed, and the error arm rendered. The status gate now resolves the account before the handler, so a ghost is turned away at the door. Better behaviour, but it left those four error arms uncovered, and it left a test whose name outlived what it proved.
mfa_handler_error_arms_with_a_ghost_subject→ renameda_ghost_subject_is_turned_away_before_any_mfa_handler_runs, documenting what it actually pins now.mfa_management_error_arms_with_a_real_accountdrives those arms with an account that passes every gate and asks for something the engine refuses.Worth noting how this surfaced: the old assertions were
assert_ne!(status, <success>). Had the gate changed a response from one 4xx to another, nothing would have failed — only the 100% line gate caught it. The new test pins the exacterror.code(auth.invalid_credentials,auth.mfa_setup_required,auth.mfa_not_enabled), which is what proves the request reached the handler rather than stopping at the gate.Verification
cargo fmt --all --checkcargo clippy --workspace --all-targets --all-features -- -D warningsRUSTDOCFLAGS="-D warnings" cargo docllvm-covlinesllvm-covfunctionsRun in an isolated worktree off
main, since the shared checkout carries another session's in-progress work.