fix(security): close the second audit's findings, and align the credential path with nest-auth by msalvatti · Pull Request #131 · bymaxone/rust-auth

msalvatti · 2026-08-06T10:22:53Z

Second cross-implementation security audit of this library and its NestJS sibling: six fresh-context agents plus a lane covering ASVS V14/V16 and cross-library parity, aimed at what the first audit's lenses did not reach.

Held in step with bymaxone/nest-auth (PR #71). The wire contract stays byte-identical between the two repositories.

What was wrong

oauth_initiate was the one flow still reading the caller's tenant verbatim. Login, register, all four password-reset steps and both email-verification steps open with resolve_tenant. This one took ?tenantId= straight from the query string and wrote it into the single-use os: state, which the callback then reads back for find_by_oauth_id, CreateWithOAuthData and the HookContext.

It is also the worst flow to leave open, because it is the one that decides which tenant an account is provisioned into. Against a deployment configuring a TenantIdResolver — host or subdomain tenancy, the reason the resolver exists — an unauthenticated caller could name any tenant, complete a normal sign-in with their own provider account, and be created or linked inside it.

The doc comment rationalised it: "it is not validated here (the on_oauth_login hook enforces tenant membership)". That did not hold — the hook is handed the same value through its HookContext, so a hook deciding on the profile alone admitted it. nest-auth closed this on its side and its comment names the stakes exactly; this side never got the fix.

Four routes carried the wrong rate limit or none. POST /auth/platform/logout and DELETE /auth/platform/sessions were mounted unthrottled, and both recovery-codes routes were served under mfa_setup (5/60) instead of mfa_disable (3/300) — 25x more permissive than the sibling on a route that is TOTP-gated and can repeatedly invalidate a victim's recovery codes.

/auth/platform/logout is the one that mattered. It is public by deliberate design — a caller whose access token expired must still be able to kill their refresh session — which is exactly why it needs a limit. Unauthenticated and unthrottled, a loop of invented 64-hex strings drives find_session, an HMAC verify, revoke_session and delete_grace_pointer: two to four round trips each holding one of the pool's connections.

Nothing could catch this class. All 27 limit values are pinned against the shared contract and the contract is checked for extra names, but no test asserted which limit each route wears — wiring login to register's 10/3600 would have passed the entire suite.

The default breach screen refused every non-Latin password. reduce_to_base_word filtered to is_ascii_alphanumeric, so a password in Cyrillic, Han, Kana, Hangul, Greek, Arabic, Hebrew or Thai reduced to the empty string — below MIN_BASE_LENGTH, which is_breached answers true for. Those users were refused on register, on reset and on change, and told their strong password was commonly used, pushing them onto the strictly smaller ASCII keyspace.

tenant_id accepted control characters. It arrives in the body of five public routes and is the caller's own value whenever no resolver is configured — the default — then reaches a tracing event, a Redis key segment and an HMAC preimage. nest-auth has always rejected these, so this was also a wire divergence: the same request was a 400 on one backend and a 200 on the other, which is precisely what requestFieldBounds exists to prevent. A newline forges a record on a plain-text subscriber (ASVS 16.4.1).

The strongest compromise signal was anonymous. Refresh-token reuse — a token already exchanged, presented again — was logged as bare prose, with the account reaching only a consumer who had wired on_refresh_token_reuse_detected. The shipped hooks are no-ops. login: invalid credentials logged neither the address nor the tenant, with both in scope as parameters of the enclosing function.

The session index grew one permanent member per refresh. A rotation adds rp:{old} alongside rt:{new}, only a full revoke-all ever removed it, and refresh_rotate re-arms the set's TTL each time. Every reader is linear in its size, including invalidate_user_sessions, which walks it inside a script and so blocks the whole single-threaded store.

What changed

Each fix carries a test that fails when the fix is reverted.

oauth_initiate resolves the tenant before anything is minted, and the resolved value is what reaches the state — so the callback cannot be talked into a different one either.
The four routes carry the limits nest-auth serves them under, and a new test bottles one named limit down to a single request at a time and asserts what 429s under it, with a negative case so an assertion cannot pass for a route wired to either name.
The breach reduction keeps letters and numbers in any script, and the floor counts characters rather than UTF-8 bytes — the second half is load-bearing, since without it a two-character Han base would clear a byte-based floor of four while being genuinely weak.
no_control_characters on all nine tenant_id fields, and log_safe at every one of the ten tracing sites that interpolate one. The DTO is the boundary; log_safe is the second lock for a value that reached a log line without passing one, which is what a host-supplied resolver returns.
Reuse detection names the family on detection and the owner on revocation, as two events, so a failed revocation cannot take the finding down with it.
list_sessions_inner drops the grace members whose pointer has expired, in the walk it was already doing. Pointers still inside their window are left alone — those are what let a revoke-all also kill a token rotated away moments earlier.

The password-hash format is PHC on both sides, and only PHC. The two libraries could not read each other's hashes at all: this one writes PHC, nest-auth wrote scrypt:N:r:p:{saltHex}:{derivedHex}. Because verification is total, the failure surfaced as invalid_credentials rather than a parse error, and the lf: counter is keyed identically on both — so five correct attempts by the owner locked the account out of the pair. nest-auth now writes PHC, and passwordHashFormat pins the encoding with a known-answer vector from each implementation, replacing prose both sides could satisfy alone.

There is deliberately no compatibility reader. Both libraries are new and have never backed a deployment, so one would be an unused branch in the credential-verification core — the same reasoning 6da0382 gave when it removed the earlier compatibility paths. This branch briefly added one and then removed it again; what it also cleans up is what that 2026-07-29 commit left behind: a specification paragraph describing the deleted shim, and two exclude_re patterns naming functions that no longer exist. A stale exclusion is worse than none — it silently suppresses a future function that happens to share the name.

Review findings, closed

A Rust code review and a security review over the full diff found five real defects — three of them in this branch's own work:

log_safe was applied at one tracing site out of ten, while its own doc comment describes it as the second lock for exactly the other nine.
The || c == '\u{7f}' clause was dead and its comment stated the opposite: is_control does cover DEL (Unicode category Cc), confirmed by running it.
A #[allow(clippy::type_complexity)] this branch introduced — itself a finding under the workspace rules, and avoidable with a type alias that removes the complexity rather than hiding it. The workspace is back to zero #[allow]/#[expect].
Two pieces of new security code shipped with no test: no_control_characters across nine DTOs, and the grace-pointer pruning. Both have one now; the pruning test runs against real Redis and lets the short pointer expire rather than deleting it, asserting both halves — the dead member is dropped and the live one survives.
The OAuthInitiateQuery::tenant_id doc still cited the rationale the OAuth fix records as false.

Verification

cargo fmt --check, clippy -D warnings --workspace --all-targets --all-features, workspace lib tests, the adapter integration tier and the real-Redis redis_stores tier all clean. Mutation over the password module: 20 caught, 0 missed, 1 unviable.

…rmat by vector The second cross-implementation audit found that the two libraries cannot verify each other's stored password hashes. nest-auth wrote `scrypt:N:r:p:{saltHex}:{derivedHex}`; this crate writes and reads PHC. `PasswordHash::new` rejects the colon encoding, and because `verify` is total the rejection collapses to `Ok(false)` — so the engine answers `auth.invalid_credentials`, indistinguishable from a wrong password. Five of those trip the `lf:` brute-force counter, which the two backends key identically, and the account is locked out of both by its owner's own correct password. The module docs already claimed "a compatibility parser" and the technical specification described a shim that parses the legacy format and treats it as needing a rehash. Neither existed. `legacy.rs` adds the read path: `verify_phc` falls back to it when `PasswordHash::new` fails, deriving under the parameters the record carries and comparing with `subtle::ConstantTimeEq`. Nothing mints the shape, and `needs_rehash` already reports it stale, so a stored corpus migrates on each owner's next successful sign-in. `credentialFormats.passwordHash` read "self-describing: the parameters the hash was written under travel with it" — which BOTH encodings satisfy. That is why the divergence survived a release: prose each side could satisfy alone, with neither suite testing against the other's output. It is replaced by a `passwordHashFormat` section pinning the encoding, the B64 alphabet, the parameter-lookup rule, the accepted derived-key range, and the staleness triggers, with three known-answer vectors — one written by each implementation and one legacy. Every string in it is real emitted output. Both suites verify all three, so a drift in either encoder, parser, alphabet or parameter ordering turns that side red. nest-auth writes PHC as of the paired change and keeps a mirror-image read path for this crate's output, so the compatibility is bidirectional. Its 64-byte derived key and this crate's 32-byte one both verify here: the length travels with the hash, and it is deliberately not a staleness trigger, or every hash would rehash on every crossing of a shared user table and never converge. The contract file stays byte-identical between the two repositories.

…sswords Two findings from the second cross-implementation audit. **`oauth_initiate` was the one flow still reading the caller's tenant verbatim.** Login, register, all four password-reset steps and both email verification steps open with `resolve_tenant`; this one took `?tenantId=` straight from the query string and wrote it into the single-use `os:` state record, which the callback then reads back for `find_by_oauth_id`, `CreateWithOAuthData` and the `HookContext`. It is also the worst flow to leave open, because it is the one that decides which tenant an account is *provisioned into*. Against a deployment that configures a `TenantIdResolver` — host or subdomain tenancy, the reason the resolver exists — an unauthenticated caller could name any tenant, complete a normal sign-in with their OWN provider account, and be created or linked inside it. The doc comment's rationale, that `on_oauth_login` enforces tenant membership, did not hold: the hook is handed the same spoofed value through its `HookContext`, so a hook deciding on the profile alone admitted it. nest-auth closed this on its side and its comment names the stakes exactly — "the one door that still took it verbatim... strictly more than the others were protecting". This side never got the fix, which is the drift the parity work exists to end. The resolved value is what goes into the state, so the callback cannot be talked into a different one either. The regression test lives beside the other OAuth tests because it needs a configured provider to reach the resolver at all, with a pointer left in `every_tenant_scoped_flow_honours_the_resolver` — the checklist where a missing flow is a flow nobody notices. **The default breach screen refused every non-Latin password.** `reduce_to_base_word` filtered to `is_ascii_alphanumeric`, so a password in Cyrillic, Han, Kana, Hangul, Greek, Arabic, Hebrew or Thai reduced to the empty string — below `MIN_BASE_LENGTH`, which `is_breached` answers `true` for. Users of those scripts were refused on register, on reset and on change, and told their strong password was commonly used, which pushes a whole class of users toward the strictly smaller ASCII keyspace. That inverts the purpose of a breach screen. The filter now keeps letters and numbers in any script, and the floor counts characters rather than UTF-8 bytes. The weak ASCII shapes the floor exists for are still refused, and a repeated non-ASCII character is now caught by the repetition rule rather than by collapsing to "" — the same answer, reached for a reason that keeps holding when the script changes. Keeping the characters also makes a consumer's non-Latin extra word reachable, since extras are normalized through the same function and previously all became "". nest-auth carried the identical defect through a different pair of ASCII filters and is fixed in the same round.

…t in the logs **Four routes carried the wrong limit or none.** `POST /auth/platform/logout` and `DELETE /auth/platform/sessions` were mounted with no throttle at all, and both `recovery-codes` routes were served under `mfa_setup` (5/60) instead of `mfa_disable` (3/300) — 25x more permissive than the sibling backend on a route that is TOTP-gated and can repeatedly invalidate a victim's recovery codes. nest-auth states the intent this diverged from: regeneration shares the disable throttle "because the security posture is identical". `/auth/platform/logout` is the one that mattered. It is public by deliberate design — a caller whose access token expired must still be able to kill their refresh session — which is exactly why it needs a limit. Unauthenticated and unthrottled, a loop of invented 64-hex strings drives `find_session`, an HMAC verify, `revoke_session` and `delete_grace_pointer`: two to four round trips each holding one of the pool's connections, against a pool with no wait timeout. Nothing could catch this class. All 27 limit VALUES are pinned against the shared contract and the contract is checked for extra names, but no test asserted which limit each ROUTE wears — wiring `login` to `register`'s 10/3600 would have passed the entire suite. The new test bottles one named limit down to a single request at a time and asserts what 429s under it, with a negative case so an assertion cannot pass for a route wired to either name. Writing that test surfaced two ways it could have been vacuously green: the whole `adapter.rs` file is gated on every optional feature, so `cargo test --test adapter` without `--all-features` compiles zero tests and exits 0; and the default `EngineSpec` mounts no platform group, so the first version asserted against a 404. The helper now refuses a 404 explicitly. **Reuse detection and failed logins were anonymous.** Refresh-token reuse is the strongest compromise signal the library produces, and it was logged as bare prose — the account reached only a consumer who had wired `on_refresh_token_reuse_detected`, and the shipped hooks are no-ops. Both planes now name the family on detection and the owner on revocation, as two events, so a `revoke_family` that fails cannot take the finding down with it. `login: invalid credentials` logged neither the address nor the tenant, with both in scope as parameters of the enclosing function. nest-auth logs a masked address and a sanitized tenant; this side now matches. **`log_safe`, and the `tenant_id` charset.** `tenant_id` arrives in the body of five public routes and is the caller's own value whenever no `TenantIdResolver` is configured — the default — then reaches a tracing event, a Redis key segment and an HMAC preimage. nest-auth has always rejected control characters in it; this side accepted them, so the same request was a 400 on one backend and a 200 on the other, which is precisely what `requestFieldBounds` exists to prevent. It also let a caller forge a record in a plain-text log pipeline (ASVS 16.4.1). All nine `tenant_id` fields now carry the check, and `log_safe` is the second lock at the log site for values that reach one without passing a DTO. **The session index no longer grows a permanent member per refresh.** A rotation adds `rp:{old}` alongside `rt:{new}`, and only a full revoke-all ever removed it, while `refresh_rotate` re-arms the set's TTL each time. Every reader is linear in the set's size, including `invalidate_user_sessions`, which iterates it inside a script and blocks the whole store. `list_sessions_inner` now drops the members whose pointer has already expired, in the walk it was doing anyway; pointers still inside their window are left alone. Not pruned from the rotation path, which is hot — session-cap enforcement lists, so a login bounds it. nest-auth carries the identical fix.

The non-Latin fix changed the default screen from refusing every non-Latin password to admitting every one of them. Neither end is the whole story: the reduction now preserves letters in any script, but the shipped base words hold no entries in those scripts, so the equivalent of `password` in one of them passes. That is a limitation worth naming rather than leaving for a deployment to discover — and it has a remedy, since extra words are normalized through the same reduction, so a non-Latin entry matches a decorated form of itself exactly as an ASCII one does. Held in step with nest-auth's copy.

…ly encoding Reverts the reader added earlier in this branch, and finishes the cleanup 6da0382 started. Both libraries are new and have never backed a deployment, so there is no corpus in nest-auth's pre-PHC `scrypt:N:r:p:{saltHex}:{derivedHex}` shape — and a reader for it is an unused branch in the credential-verification core, which is exactly the reasoning 6da0382 gave when it removed the compatibility paths in the first place. What that earlier commit left behind, and this one clears: - `legacy.rs` and the `verify_phc` fallback that reached it. - Two `exclude_re` patterns in `.cargo/mutants.toml` describing `is_legacy` and a `decode_hex` built on `(hi << 4) | lo` — both part of the reader 6da0382 deleted, and neither matching a mutant since. A stale exclusion is worse than none: it silently suppresses a future function that happens to share the name, and nobody decided that. - The specification's claim that "a compatibility shim parses that legacy colon-delimited format", which described the deleted shim rather than the code. nest-auth writes PHC as of the paired change, so a hash from either backend verifies under the other and nothing in the credential path branches on which library wrote the record. The contract keeps two vectors, one per implementation, and its comment records why there is no third. Correcting my own earlier commit message: 098aaf6 said the documented shim "never existed". It did — it was removed deliberately on 2026-07-29, and only the documentation and the mutation-config exclusions outlived it. Verified: workspace lib tests, `clippy -D warnings --workspace --all-targets --all-features`, the adapter integration tier, and `cargo fmt --check` all clean.

… a suppression, a stale doc Five findings from the code and security reviews of this branch. Each was verified against the code before being accepted; three are corrections to things this branch itself introduced. **`log_safe` was applied at one tracing site out of ten.** The helper exists as the second lock — the DTO rejects control characters at the boundary, and this catches a value that reached a log line without passing one, which is exactly what a host-supplied `TenantIdResolver` returns. Its own doc comment says so. But only `record_failure_and_reject` used it; `login: account locked`, `login: MFA challenge issued`, `login: success`, `lockout cleared`, `register`, `verify email` and the three invitation events all interpolated `%tenant_id` raw. All ten go through it now. **The DEL clause was dead and its comment was wrong.** `log_safe` and `no_control_characters` both read `c.is_control() || c == '\u{7f}'`, with a comment claiming `is_control` covers C0 and C1 "but not DEL, which is named explicitly". It does cover DEL — Unicode category Cc includes U+007F, confirmed by running it. The clause was unreachable and the comment was a false statement about the standard library. Both are gone, and the rule is stated as what it is. **A `#[allow(clippy::type_complexity)]` this branch added.** A suppression is itself a finding under the workspace rules, and this one was avoidable: a `LimitSetter` type alias removes the complexity the lint was pointing at rather than hiding it. The workspace is back to zero `#[allow]`/`#[expect]`. **Two pieces of new security code had no test at all.** `no_control_characters` is wired onto all nine `tenant_id` fields and nothing exercised it; the grace-pointer pruning in `list_sessions_inner` was the one change motivated by an unbounded-growth finding and nothing exercised that either. Both have tests that fail when the code is reverted. The pruning test runs against real Redis and lets the short pointer expire rather than deleting it — expiry is how that absence actually arises — and asserts both halves: the dead member is dropped and the LIVE one survives, since that member is what lets a revoke-all also kill a token rotated away moments earlier. **The specification still described the legacy reader.** That paragraph was rewritten once before the decision to drop compatibility entirely, and not revisited after. It now says what the code does: PHC is the only encoding either implementation reads, a hash either parses as PHC or is refused, and there is no compatibility path because neither library has ever backed a deployment. Verified: `cargo fmt --check`, `clippy -D warnings --workspace --all-targets --all-features`, workspace lib tests, the adapter tier and the real-Redis `redis_stores` tier all clean.

…ot hold `OAuthInitiateQuery::tenant_id` still read "Not validated against the DB here (the `on_oauth_login` hook enforces tenant membership)" — the exact reasoning this branch's own `oauth_initiate` fix records as false. The hook is handed the same value through its `HookContext`, so a hook deciding on the profile alone admitted a caller into any tenant they named, on the one flow that decides which tenant an account is provisioned into. The field is a request for a tenant, not a decision about one, and the doc says so now: the resolver runs before anything is minted and the resolved value is what reaches the state record.

Copilot

🟡 Changes recommended

One modified tracing call contradicts its own “single-line for coverage” constraint, risking reintroducing the exact 100%-coverage false-negative it warns about.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

Closes multiple findings from the second cross-implementation security audit by aligning rust-auth’s tenant handling, rate-limit wiring, credential-format contract, logging safety, and Redis session index hygiene with the shared wire contract and the NestJS sibling.

Changes:

Resolve OAuth tenant via TenantIdResolver (and pass request context through adapters) so caller-provided tenantId cannot select a provisioning tenant.
Pin password-hash encoding to PHC via shared contract vectors; improve breach-base reduction to support non-ASCII scripts and correct character-count semantics.
Fix rate-limit wiring for specific routes; harden tenant-id validation and log safety; prune expired grace-pointer members during session listing.

File summaries

File	Description
docs/technical_specification.md	Updates credential-format specification text to reflect PHC-only contract.
crates/bymax-auth-redis/tests/redis_stores.rs	Adds regression test for pruning expired grace-pointer members on session listing.
crates/bymax-auth-redis/src/stores/session.rs	Prunes expired `rp:` members during `list_sessions` to prevent unbounded session index growth.
crates/bymax-auth-crypto/src/password/tests.rs	Adds contract-driven PHC verification vectors test (reads shared wire-contract JSON).
crates/bymax-auth-crypto/src/password/phc.rs	Documents PHC as the only supported encoding across implementations.
crates/bymax-auth-crypto/src/password/mod.rs	Removes stale mention of a legacy compatibility parser from module docs.
crates/bymax-auth-crypto/Cargo.toml	Adds `serde_json` dev-dependency for contract-vector tests.
crates/bymax-auth-core/src/traits/common_password.rs	Preserves alphanumerics from any script and counts base length by characters (not UTF-8 bytes); adds tests.
crates/bymax-auth-core/src/services/token_manager.rs	Improves refresh-token reuse detection logging with family/user attribution.
crates/bymax-auth-core/src/services/oauth.rs	Resolves tenant before minting OAuth state; updates OAuth tests and adds resolver-honoring test.
crates/bymax-auth-core/src/services/auth/register.rs	Sanitizes logged `tenant_id` via `log_safe`.
crates/bymax-auth-core/src/services/auth/mod.rs	Notes OAuth initiate tenant-resolver coverage location in tests.
crates/bymax-auth-core/src/services/auth/login.rs	Sanitizes logged `tenant_id` via `log_safe` and expands invalid-credentials logging context.
crates/bymax-auth-core/src/services/auth/invitation.rs	Sanitizes logged `tenant_id` via `log_safe`.
crates/bymax-auth-core/src/services/auth/email_verification.rs	Sanitizes logged `tenant_id` via `log_safe`.
crates/bymax-auth-core/src/normalize.rs	Introduces `log_safe` helper to prevent control-character log forging.
crates/bymax-auth-core/src/lib.rs	Re-exports `log_safe` from the core crate surface.
crates/bymax-auth-axum/tests/adapter.rs	Adds tests asserting each route is wired to the intended named rate limit.
crates/bymax-auth-axum/src/routes/platform.rs	Applies throttling to `POST /platform/logout` and `DELETE /platform/sessions` per intended limits.
crates/bymax-auth-axum/src/routes/platform_mfa.rs	Corrects recovery-codes throttling to `mfa_disable` for platform MFA route.
crates/bymax-auth-axum/src/routes/oauth.rs	Passes request context into engine OAuth initiate so tenant resolver can be consulted.
crates/bymax-auth-axum/src/routes/mfa.rs	Corrects recovery-codes throttling to `mfa_disable` for dashboard MFA route.
crates/bymax-auth-axum/src/dto.rs	Adds `tenant_id` control-character rejection across DTOs + validation tests; updates OAuth initiate query docs.
conformance/wire-contract.json	Adds `passwordHashFormat` section with PHC vectors and updates password-hash description.
Cargo.lock	Locks new `serde_json` dev-dependency.
.cargo/mutants.toml	Removes stale mutation excludes tied to deleted legacy credential reader.

Review details

Files reviewed: 25/26 changed files
Comments generated: 1
Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

msalvatti · 2026-08-06T12:53:43Z

{
  "consecutiveFailures": {
    "rustdoc:broken-intra-doc-links": 1,
    "core / coverage (llvm-cov):fail-under-lines": 1
  },
  "flakyReruns": {
    "core / clippy": 2,
    "core / msrv": 1,
    "analyze (javascript-typescript)": 1
  },
  "processedCommentIds": [3727983755],
  "processedReviewIds": [4873636396, 4875365715, 4875938659],
  "paused": false,
  "localGate": "cargo llvm-cov --workspace --all-features --locked --fail-under-lines 100 --fail-under-functions 100",
  "lastFixCommit": "ba588aa",
  "lastLocalGate": "pass — 26218/26218 lines, 2945/2945 functions; fmt, workspace clippy, core lib 531/531; pre-push ran the full workspace suite clean",
  "terminated": "2026-08-06T15:30:00Z",
  "terminationReason": "green on ba588aa — 25 checks pass, mutation skipped by design on PRs, mergeable, the single Copilot thread resolved, all three review bodies triaged",
  "flakyEvidence": "GitHub Actions outage 15:08-15:23Z: 'Failed to resolve action download info. Internal Server Error / Service Unavailable' during Set up job. No code ran. msrv and analyze(js-ts) recovered on rerun 1; core / clippy needed rerun 2 and then genuinely executed 'cargo clippy --workspace --all-targets --all-features --locked -- -D warnings' and passed.",
  "note": "Copilot posts findings as SUPPRESSED comments in the review body, with no inline thread. Phase 2 MUST read pulls/<N>/reviews bodies, not only reviewThreads — body-only findings were missed twice (on f93b95c and 2201573).",
  "copilotReviewStale": "IMPORTANT: the copilot-pull-request-reviewer run for ba588aa (31114374091) died in the same outage and reports 'cannot be retried'. The newest Copilot review covers 2201573, not the current head. Not a PR check, so it never blocked — but nothing has reviewed the register.rs change on the head. Re-request a Copilot review if you want head coverage.",
  "openForAuthor": "Copilot review 4875938659 says 'Human review recommended' for the security-critical surfaces this PR touches (OAuth tenancy, password hash contract, rate limiting, Redis session semantics). Not a bot's call to clear."
}

… broke Two red checks, both real, both introduced by this branch. **`rustdoc`.** `oauth_initiate`'s doc comment linked `crate::traits::TenantIdResolver`. The trait is in `crate::config`; `traits` has no such item, so `rustdoc::broken_intra_doc_links` failed the build under `-D warnings`. Linked where the trait actually lives, as `services::auth::resolve_tenant` already does. **`core / coverage`.** Eighteen lines uncovered against `--fail-under-lines 100`. Fourteen of them are `tracing` field expressions that carry a call. A field expression is not evaluated without an installed subscriber, so a call sitting on its own line reads as uncovered while the branch around it is perfectly exercised. The previous commit applied `log_safe` to nine `tenant_id` fields, which pushed every one of those macro calls past `max_width` and rustfmt split them — the exact hazard the comment above `login: account locked` warned about, which Copilot flagged as now stale. The comment was right and is kept, at the one site that explains it; the calls move into bindings ahead of the macro, which is width-independent, unlike keeping the macro on one line. A field with no call (`%family`, `%revoker_user_id`) never gets a region, so those stay inline. `log_safe`'s rejection branch had only a doctest, which llvm-cov does not measure. It has a unit test now, covering every category the rule names (C0, DEL, C1) and the two it deliberately excludes (U+2028/U+2029). The remaining three were shape, not substance: - The tenant-resolver OAuth test bound its engine through a multi-line `let ... else { return }`, so the never-taken arm became a LINE rather than a region. Bound on one line, as `rustfmt.toml` documents. - The same test's first assertion awaited inside a `matches!` scrutinee, which leaves the resumption region on a line of its own. Awaited into a binding first, matching the assertion ten lines below it. - `list_sessions`' grace-pointer prune had no case where the member was neither a live session nor a pointer. A legacy bare-hash member is exactly that, and it must be left alone — the prune keys on an expired `rp:` key and a bare hash has none. The pruning test now plants one and asserts it survives. The redaction itself was never asserted anywhere. `log_capture` exists so a log line that is a branch's only observable effect can be tested, and six sites now use it: the masked address and the sanitized tenant reach `login: success`, `login: MFA challenge issued`, `login: invalid credentials`, `login: account locked`, `lockout cleared`, `verify email`, `invitation: created`/`withdrawn`/`revoke refused`, and both reuse-detection warnings, which name the revoked account nowhere else. Verified: `cargo fmt --check`, `clippy -D warnings --workspace --all-targets --all-features`, `RUSTDOCFLAGS=-D warnings cargo doc --workspace --all-features`, and the CI coverage command itself — `cargo llvm-cov --workspace --all-features --locked --fail-under-lines 100 --fail-under-functions 100` — at 26217/26217 lines and 2945/2945 functions against a real Redis.

Copilot

🟡 Changes recommended

A test helper claims to widen every RateLimitConfig field but currently only covers a subset, reducing correctness and future-proofing of the new route↔limit wiring assertions.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (1)

crates/bymax-auth-axum/tests/adapter.rs:3461

ALL_LIMIT_SETTERS is documented as covering every field of RateLimitConfig, but it currently only sets a small subset (e.g., it omits forgot_password, oauth_initiate, ws_ticket, etc.). That makes router_with_one_narrow_limit less future-proof than the comment claims, and it can allow an unrelated default limit to trip if this probe is ever extended to hit additional routes.

Consider including every RateLimitConfig field in ALL_LIMIT_SETTERS so the helper actually widens all limits as intended.

const ALL_LIMIT_SETTERS: &[LimitSetter] = &[
    |c, v| c.login = v,
    |c, v| c.register = v,
    |c, v| c.refresh = v,
    |c, v| c.logout = v,

Files reviewed: 26/27 changed files
Comments generated: 0 new
Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

… to widen `ALL_LIMIT_SETTERS` is documented as "every field of `RateLimitConfig` … so a NEW limit added to the struct shows up here rather than being silently left at its default during a probe". It listed 8 of the struct's 27 fields, so the guarantee it states was not one it kept. Nothing is mis-asserted today: `router_with_one_narrow_limit` is used for routes wired to `logout`, `revoke_all_sessions`, `mfa_disable` and `mfa_setup`, and all four were in the list. The hazard is the next route added to the probe — an unrelated default limit trips, and the failure is attributed to the wiring under test. The negative assertion is the one that fails quietly: it reads "recovery-codes must NOT be served under `mfa_setup`", and a default limit tripping somewhere else would satisfy it for the wrong reason. All 27 fields are listed now, in declaration order so a field added between two of them is visibly missing rather than lost off the end of an unordered list. None are feature-gated, so the list needs no `cfg`. Verified: `cargo fmt --check`, `clippy -D warnings -p bymax-auth-axum --all-targets --all-features`, and the adapter tier — 75 passed, including `each_route_is_served_under_the_limit_it_declares`.

msalvatti · 2026-08-06T14:43:32Z

Re: Copilot review of 2026-08-06 13:55Z — ALL_LIMIT_SETTERS does not cover every RateLimitConfig field

Replying here rather than on a thread: that finding arrived as a suppressed comment in the review body, so it has no inline thread to answer on.

Applied in 2201573. The finding is correct — verified against the code rather than taken on faith:

RateLimitConfig has 27 fields; ALL_LIMIT_SETTERS listed 8.
Its doc says "Every field of RateLimitConfig … so a NEW limit added to the struct shows up here rather than being silently left at its default during a probe". That guarantee was not one it kept.

On impact, to be precise about what was and was not broken: nothing is mis-asserted today. router_with_one_narrow_limit is used only for routes wired to logout, revoke_all_sessions, mfa_disable and mfa_setup, and all four were already in the list. The hazard is the next route added to the probe — an unrelated default limit trips and the failure is attributed to the wiring under test. The negative assertion is the one that would fail quietly: recovery-codes must NOT be served under the mfa_setup limit is satisfied by any non-429, so a default limit tripping elsewhere would make it pass for the wrong reason.

All 27 fields are listed now, in declaration order, so a field added between two of them is visibly missing rather than lost off the end of an unordered list. None of the fields are feature-gated, so no cfg is needed.

Verified: cargo fmt --check, clippy -D warnings -p bymax-auth-axum --all-targets --all-features, and the adapter tier — 75 passed, each_route_is_served_under_the_limit_it_declares included. The file is under tests/, which cargo-llvm-cov excludes, so the 100% coverage gate is unaffected.

Copilot

🔵 Human review recommended

It changes multiple security-critical auth surfaces (OAuth tenancy, password hash contract, rate limiting, Redis session semantics) and should receive final human verification despite strong tests.

Review details

Suppressed comments (1)

crates/bymax-auth-core/src/services/auth/register.rs:84

log_safe(&tenant_id) is called inside the tracing::info! field expression here, which is the pattern that previously caused coverage-gate false negatives when rustfmt wraps fields onto their own lines (and also makes the sanitization harder to assert unless a subscriber is installed). To make this robust and consistent with the other logging sites, bind the sanitized tenant first and log the binding.

        let safe = SafeAuthUser::from(user);
        tracing::info!(user_id = %safe.id, tenant_id = %log_safe(&tenant_id), "register: user registered");
        let result = self

Files reviewed: 26/27 changed files
Comments generated: 0 new
Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

The one `log_safe` call this branch left inside a `tracing` field expression. It is covered today only because the line happens to fit; the previous commit moved nine sibling sites into bindings precisely because a field expression is not evaluated without an installed subscriber, so the call reads as an uncovered line the moment rustfmt wraps it. Leaving one site on the old shape keeps the hazard alive and makes the rule harder to see. Behaviour is unchanged: the value is the same, computed one statement earlier. Not touched: the `mask_email` fields in `traits/email.rs` and `services/platform.rs` are on the same shape but predate this branch and are outside its diff. Verified: `cargo fmt --check`, `clippy -D warnings -p bymax-auth-core --all-targets --all-features`, the core lib tier (531 passed), and the CI coverage command — `cargo llvm-cov --workspace --all-features --locked --fail-under-lines 100 --fail-under-functions 100` — at 26218/26218 lines and 2945/2945 functions.

msalvatti · 2026-08-06T15:08:06Z

Re: Copilot review of 2026-08-06 14:49Z — log_safe(&tenant_id) inline in the register tracing field

Another suppressed comment in the review body, so again no thread to answer on.

Applied in ba588aa. The finding is correct and it is the one site the previous pass missed:

let safe = SafeAuthUser::from(user);
let tenant = log_safe(&tenant_id);
tracing::info!(user_id = %safe.id, tenant_id = %tenant, "register: user registered");

It survived because it happened to fit on one line, which is exactly the fragile property — the nine sibling sites broke in this branch when log_safe pushed them past max_width and they were wrapped. Leaving one site on the old shape keeps the hazard and makes the rule harder to see. Behaviour is unchanged: same value, computed one statement earlier.

Deliberately not touched: traits/email.rs (7 sites) and services/platform.rs:120 interpolate mask_email(...) in the same shape, but they predate this branch and are outside its diff. Worth a follow-up, not a widening of this PR.

Verified: cargo fmt --check, clippy -D warnings -p bymax-auth-core --all-targets --all-features, the core lib tier (531 passed), and the CI coverage command — cargo llvm-cov --workspace --all-features --locked --fail-under-lines 100 --fail-under-functions 100 — at 26218/26218 lines, 2945/2945 functions. The pre-push hook also ran the full workspace suite clean.

On the review's headline (🔵 Human review recommended): agreed, and it is not something a bot loop should sign off. Flagged to the author.

Copilot

Pull request overview

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

Suppressed comments (3)

crates/bymax-auth-redis/tests/redis_stores.rs:355

This test is time-based and can be flaky because Redis TTL is second-granularity and expiry timing depends on scheduling; after 1500ms the key may still exist briefly (e.g., TTL returns 0) even though it’s about to expire. Prefer polling with a bounded timeout until the key is absent (TTL == -2) or using a longer wait with a retry loop, so CI variance doesn’t cause intermittent failures.

    tokio::time::sleep(std::time::Duration::from_millis(1500)).await;
    assert_eq!(
        redis.ttl("auth:rp:p1").await,
        -2,
        "the short pointer must have expired"
    );

docs/technical_specification.md:5129

This sentence is potentially misleading now that the PR states both implementations write PHC and PHC is the only accepted encoding. Consider rephrasing to clarify the parity is about the default KDF selection (e.g., 'nest-auth defaults to scrypt') rather than implying nest-auth writes a non-PHC 'scrypt' format.

**Recommended vs default.** **Argon2id is the *recommended* writer for new deployments** — it is OWASP's first-choice memory-hard KDF and the more conservative choice against GPU/ASIC attackers — while **scrypt is the *default*** purely for drop-in parity with nest-auth, which writes scrypt. A greenfield deployment SHOULD enable the `argon2` feature (§19.2) and configure Argon2id as the writer (§19.3); scrypt verification is retained so any legacy hashes still validate and lazily migrate via rehash-on-verify. At the type level this is enforced by making `Argon2id` a `#[cfg(feature = "argon2")]` enum variant (§5.1.9): the default active algorithm is `Scrypt`, and Argon2id becomes *selectable* only once the feature is compiled in, so a default build can never name an uncompiled hasher.

crates/bymax-auth-crypto/src/password/tests.rs:395

Using unwrap_or_default() / unwrap_or(Null) here discards the underlying I/O/parse error details, which can make contract-path failures harder to diagnose (e.g., wrong working directory in CI). Consider failing fast with an expect/error message that includes the path and the original read/parse error, so the failure mode is immediately actionable.

        let raw = std::fs::read_to_string(path).unwrap_or_default();
        let root: serde_json::Value = serde_json::from_str(&raw).unwrap_or(serde_json::Value::Null);
        root.get("passwordHashFormat")
            .and_then(|s| s.get("vectors"))
            .and_then(serde_json::Value::as_array)
            .cloned()
            .unwrap_or_default()

msalvatti added 7 commits August 4, 2026 13:54

Copilot AI lite review requested due to automatic review settings August 6, 2026 10:22

Copilot started reviewing on behalf of msalvatti August 6, 2026 10:23 View session

Copilot AI reviewed Aug 6, 2026

View reviewed changes

Comment thread crates/bymax-auth-core/src/services/auth/login.rs Outdated

Copilot AI review requested due to automatic review settings August 6, 2026 13:49

Copilot started reviewing on behalf of msalvatti August 6, 2026 13:50 View session

Copilot AI reviewed Aug 6, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings August 6, 2026 14:43

Copilot started reviewing on behalf of msalvatti August 6, 2026 14:43 View session

Copilot AI reviewed Aug 6, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings August 6, 2026 15:07

Copilot AI reviewed Aug 6, 2026

View reviewed changes

msalvatti merged commit 86ad4ff into main Aug 6, 2026
66 of 71 checks passed

msalvatti deleted the fix/audit2-hash-parity branch August 6, 2026 15:49

Navigation Menu

Uh oh!

fix(security): close the second audit's findings, and align the credential path with nest-auth - #131

fix(security): close the second audit's findings, and align the credential path with nest-auth#131
msalvatti merged 10 commits into
mainfrom
fix/audit2-hash-parity

msalvatti commented Aug 6, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

msalvatti commented Aug 6, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

msalvatti commented Aug 6, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

msalvatti commented Aug 6, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

msalvatti commented Aug 6, 2026

What was wrong

What changed

Review findings, closed

Verification

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

msalvatti commented Aug 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

msalvatti commented Aug 6, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Human review recommended

Suppressed comments (1)

Uh oh!

msalvatti commented Aug 6, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

msalvatti commented Aug 6, 2026 •

edited

Loading