From 098aaf6d32b8fa309cc4926420dfab38749c154b Mon Sep 17 00:00:00 2001 From: Maximiliano <40447063+msalvatti@users.noreply.github.com> Date: Tue, 4 Aug 2026 13:54:13 -0300 Subject: [PATCH 01/10] fix(crypto): read nest-auth's pre-PHC password hashes, and pin the format by vector MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- Cargo.lock | 1 + conformance/wire-contract.json | 46 +++++- crates/bymax-auth-crypto/Cargo.toml | 4 + .../bymax-auth-crypto/src/password/legacy.rs | 131 ++++++++++++++++++ crates/bymax-auth-crypto/src/password/mod.rs | 1 + crates/bymax-auth-crypto/src/password/phc.rs | 14 +- .../bymax-auth-crypto/src/password/tests.rs | 131 ++++++++++++++++++ 7 files changed, 325 insertions(+), 3 deletions(-) create mode 100644 crates/bymax-auth-crypto/src/password/legacy.rs diff --git a/Cargo.lock b/Cargo.lock index e8bf754..2306810 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -470,6 +470,7 @@ dependencies = [ "proptest", "rand 0.8.6", "scrypt", + "serde_json", "sha1", "sha2", "subtle", diff --git a/conformance/wire-contract.json b/conformance/wire-contract.json index 9af94d8..10045cf 100644 --- a/conformance/wire-contract.json +++ b/conformance/wire-contract.json @@ -350,12 +350,56 @@ "or a session started on one backend cannot continue on the other." ], "refreshToken": "64 lowercase hex characters (32 CSPRNG bytes)", - "passwordHash": "self-describing: the parameters the hash was written under travel with it, so a verify never assumes the currently configured cost", + "passwordHash": "PHC — see the passwordHashFormat section, which pins it by vector", "totpSecretAtRest": "aes-256-gcm over the BASE32 TEXT of the secret", "recoveryCodeDigest": "hex hmac-sha256 of the code under the derived identifier key", "wsTicket": "64 lowercase hex characters (32 CSPRNG bytes), single-use, 30 s lifetime" }, + "passwordHashFormat": { + "$comment": [ + "The stored password hash, pinned by known-answer vector rather than by description.", + "credentialFormats.passwordHash used to read 'self-describing: the parameters the hash was", + "written under travel with it'. BOTH sides satisfied that while writing mutually unreadable", + "strings — nest-auth wrote scrypt:N:r:p:{saltHex}:{derivedHex}, rust-auth wrote PHC. Neither", + "could verify the other's, and because verification is total the failure surfaced as", + "auth.invalid_credentials rather than as a parse error: five correct attempts by the owner", + "tripped the SHARED brute-force counter and locked the account out of both backends. Prose", + "each side can satisfy alone is not a contract. A vector each side must verify against the", + "other's real output is. Nothing below is hand-written — every string is emitted output." + ], + "encoding": "$scrypt$ln={log2(N)},r={r},p={p}${saltB64}${derivedB64}", + "b64": "PHC 'B64': the standard base64 alphabet with padding stripped. NOT base64url — a hash written with '-' or '_' is one the sibling parser rejects.", + "params": "read by name, never by position: ln, r and p may appear in any order, and a repeated key is refused rather than resolved", + "derivedKeyLengthBytes": "carried implicitly by the field's own length; 10..=64 accepted (the bounds of rust-auth's password_hash::Output). nest-auth writes 64, rust-auth writes 32, and each verifies the other under the length it reads.", + "needsRehash": "each vector's flag is evaluated with the deployment configured at exactly the cost that vector records (ln=14, r=8, p=1). Against a higher configured cost every vector is stale, which would make the flag say nothing about the encoding.", + "rehashTriggers": "a recorded cost below the configured one, or the legacy encoding. NOT the derived-key length: treating the sibling's length as stale would rehash every hash on every crossing of a shared user table and never converge.", + "legacyEncoding": "scrypt:{N}:{r}:{p}:{saltHex}:{derivedHex} — nest-auth's pre-PHC shape. READ-ONLY on both sides and always reported as needing a rehash, so a stored corpus migrates on each owner's next successful sign-in. Nothing mints it.", + "vectors": [ + { + "password": "correct horse battery staple", + "hash": "$scrypt$ln=14,r=8,p=1$1kyoaG59xNOp3cu0ikQZTg$yycIZlUDbS+4ho98Hh41gIiPcFp75kvtypfOmV6AcTJrfTfo3k2GgQmpzS2SpHJ/L32+OwUuwfOt7JMO+s9iRQ", + "writtenBy": "nest-auth", + "needsRehash": false, + "note": "64-byte derived key — the maximum password_hash::Output can represent" + }, + { + "password": "correct horse battery staple", + "hash": "$scrypt$ln=14,r=8,p=1$IoZhPkOiIMJkWmsBVfD5KA$9S6n0x6kJv5C+T+0eZDOMuXevCc+UGv0dqWCivQQcUY", + "writtenBy": "rust-auth", + "needsRehash": false, + "note": "32-byte derived key — the RustCrypto default" + }, + { + "password": "correct horse battery staple", + "hash": "scrypt:16384:8:1:d64ca8686e7dc4d3a9ddcbb48a44194e:cb27086655036d2fb8868f7c1e1e3580888f705a7be64bedca97ce995e8071326b7d37e8de4d868109a9cd2d92a4727f2f7dbe3b052ec1f3adec930efacf6245", + "writtenBy": "nest-auth (pre-PHC)", + "needsRehash": true, + "note": "must verify AND must report needsRehash on both sides" + } + ] + }, + "rateLimits": { "$comment": [ "The per-IP limit each auth route is served under, as `requests/windowSeconds`. Both", diff --git a/crates/bymax-auth-crypto/Cargo.toml b/crates/bymax-auth-crypto/Cargo.toml index 3418039..b8136d8 100644 --- a/crates/bymax-auth-crypto/Cargo.toml +++ b/crates/bymax-auth-crypto/Cargo.toml @@ -48,6 +48,10 @@ data-encoding = { version = "2", optional = true } [dev-dependencies] proptest = "1" hex = "0.4" +# Reads `conformance/wire-contract.json` so the password-hash vectors are asserted against +# the shared file rather than copied into the test — a copy drifts silently, which is the +# failure mode the contract exists to prevent. +serde_json = "1" # Benchmarks only (`cargo bench`). `default-features = false` drops the HTML/plotters # and rayon trees, keeping the dev dependency graph lean. criterion = { version = "0.8", default-features = false, features = ["cargo_bench_support"] } diff --git a/crates/bymax-auth-crypto/src/password/legacy.rs b/crates/bymax-auth-crypto/src/password/legacy.rs new file mode 100644 index 0000000..06004e5 --- /dev/null +++ b/crates/bymax-auth-crypto/src/password/legacy.rs @@ -0,0 +1,131 @@ +//! The pre-PHC nest-auth encoding: `scrypt:N:r:p:{salt_hex}:{derived_hex}`. +//! +//! Read-only, and always reported as needing a rehash — nothing here mints this shape. +//! +//! # Why this exists +//! +//! The two implementations share one user table and one brute-force counter. nest-auth wrote +//! this encoding before the pair agreed on PHC, and a hash this crate cannot read does not +//! surface as a parse failure: [`super::verify`] is total, so it collapses to `Ok(false)` and +//! the engine answers `auth.invalid_credentials` — indistinguishable from a wrong password. +//! Five of those trip the *shared* `lf:` lockout, so an account whose hash is in the legacy +//! shape is locked out of **both** backends by its owner's own correct attempts. +//! +//! The wire contract called the format "self-describing", which both encodings are. That is +//! precisely why the divergence survived a release: prose that each side satisfied separately +//! and neither could test against the other. `credentialFormats.passwordHash` now pins the +//! encoding with known-answer vectors, and `password::tests` verifies a vector nest-auth +//! actually produced. + +#[cfg(feature = "scrypt")] +use scrypt::{Params, scrypt}; +#[cfg(feature = "scrypt")] +use subtle::ConstantTimeEq; + +/// The derived-key length nest-auth wrote under this encoding, in bytes. +#[cfg(feature = "scrypt")] +const LEGACY_KEY_LEN: usize = 64; + +/// A parsed legacy hash: the cost it records, its salt and its derived key. +#[cfg(feature = "scrypt")] +struct LegacyHash { + log_n: u8, + r: u32, + p: u32, + salt: Vec, + derived: Vec, +} + +/// Decode an even-length lowercase-or-uppercase hex string. +/// +/// Written by hand rather than pulled in as a dependency: this is the only hex in the crate, +/// and `from_str_radix` on two-byte windows keeps it allocation-light and panic-free. +#[cfg(feature = "scrypt")] +fn decode_hex(text: &str) -> Option> { + if text.is_empty() || !text.len().is_multiple_of(2) { + return None; + } + let bytes = text.as_bytes(); + let mut out = Vec::with_capacity(text.len() / 2); + for pair in bytes.chunks_exact(2) { + // `chunks_exact(2)` yields two-byte windows, and both are ASCII by the `is_ascii` + // guard below, so `from_utf8` cannot fail — but it is handled rather than unwrapped, + // because the workspace denies `unwrap`. + let text = core::str::from_utf8(pair).ok()?; + if !text.bytes().all(|b| b.is_ascii_hexdigit()) { + return None; + } + out.push(u8::from_str_radix(text, 16).ok()?); + } + Some(out) +} + +/// Parse `scrypt:N:r:p:{salt_hex}:{derived_hex}`. +/// +/// Returns `None` for anything else, including a PHC string (which contains no `:` before its +/// first `$`, so the two shapes never collide). +#[cfg(feature = "scrypt")] +fn parse(stored: &str) -> Option { + let mut fields = stored.split(':'); + if fields.next()? != "scrypt" { + return None; + } + let n: u64 = fields.next()?.parse().ok()?; + let r: u32 = fields.next()?.parse().ok()?; + let p: u32 = fields.next()?.parse().ok()?; + let salt = decode_hex(fields.next()?)?; + let derived = decode_hex(fields.next()?)?; + // Exactly six fields: a trailing one means the value is not this encoding. + if fields.next().is_some() { + return None; + } + + // `N` must be a power of two for scrypt, and the `Params` constructor takes log2(N) as a + // `u8`. Rejecting a non-power-of-two here rather than rounding keeps a corrupt record from + // verifying under a cost it never used. + if !n.is_power_of_two() || n < 2 { + return None; + } + let log_n = u8::try_from(n.trailing_zeros()).ok()?; + if derived.len() != LEGACY_KEY_LEN || r == 0 || p == 0 { + return None; + } + + Some(LegacyHash { + log_n, + r, + p, + salt, + derived, + }) +} + +/// Verify `password` against a legacy-encoded hash, in constant time. +/// +/// Returns `false` when `stored` is not in this encoding, so the caller can try it after PHC +/// without branching on which shape it holds. +#[cfg(feature = "scrypt")] +pub(super) fn verify_legacy(password: &[u8], stored: &str) -> bool { + let Some(parsed) = parse(stored) else { + return false; + }; + // Derived under the parameters the hash RECORDS, never under whatever is configured today + // — the property that makes the cost factor raisable at all. + let Ok(params) = Params::new(parsed.log_n, parsed.r, parsed.p, parsed.derived.len()) else { + return false; + }; + let mut candidate = vec![0u8; parsed.derived.len()]; + if scrypt(password, &parsed.salt, ¶ms, &mut candidate).is_err() { + return false; + } + // Lengths are equal by construction (`candidate` is sized from `derived`), so `ct_eq` + // compares the full buffers with no early exit. + candidate.ct_eq(&parsed.derived).into() +} + +/// Without the `scrypt` feature there is no verifier for this encoding, so a legacy hash is +/// simply unreadable — the same answer the crate gives for any algorithm it cannot compute. +#[cfg(not(feature = "scrypt"))] +pub(super) fn verify_legacy(_password: &[u8], _stored: &str) -> bool { + false +} diff --git a/crates/bymax-auth-crypto/src/password/mod.rs b/crates/bymax-auth-crypto/src/password/mod.rs index d258aac..20741bd 100644 --- a/crates/bymax-auth-crypto/src/password/mod.rs +++ b/crates/bymax-auth-crypto/src/password/mod.rs @@ -18,6 +18,7 @@ #[cfg(feature = "argon2")] mod argon2; +mod legacy; mod phc; #[cfg(feature = "scrypt")] mod scrypt; diff --git a/crates/bymax-auth-crypto/src/password/phc.rs b/crates/bymax-auth-crypto/src/password/phc.rs index 8312b27..2e1f229 100644 --- a/crates/bymax-auth-crypto/src/password/phc.rs +++ b/crates/bymax-auth-crypto/src/password/phc.rs @@ -7,14 +7,20 @@ use argon2::Argon2; #[cfg(feature = "scrypt")] use scrypt::Scrypt; +use super::legacy; use super::{PasswordAlgorithm, PasswordParams}; -/// Verify `password` against a PHC string, auto-selecting the verifier from the PHC +/// Verify `password` against a stored hash, auto-selecting the verifier from the PHC /// algorithm prefix. Returns `false` for a wrong password, a malformed string, or an /// algorithm whose feature is not compiled in — never panics. +/// +/// A value `PasswordHash::new` rejects is tried against the pre-PHC nest-auth encoding before +/// being given up on. That fallback is not a courtesy: the two implementations share a user +/// table, and a hash this crate refuses to read surfaces as `invalid_credentials` and spends +/// an attempt on the *shared* lockout counter. See [`legacy`]. pub(super) fn verify_phc(password: &[u8], phc: &str) -> bool { let Ok(hash) = PasswordHash::new(phc) else { - return false; + return legacy::verify_legacy(password, phc); }; let verifiers: &[&dyn PasswordVerifier] = &[ #[cfg(feature = "scrypt")] @@ -30,6 +36,10 @@ pub(super) fn verify_phc(password: &[u8], phc: &str) -> bool { /// string. pub(super) fn needs_rehash_phc(phc: &str, current: &PasswordParams) -> bool { let Ok(hash) = PasswordHash::new(phc) else { + // Legacy and unparseable both answer `true`, but for different reasons worth keeping + // apart: an unparseable value is a corrupt record, while a legacy one is a readable + // hash in a shape the sibling implementation cannot use. Both need rewriting; only the + // second one will actually succeed, because only it just verified a password. return true; }; let ident = hash.algorithm.as_str(); diff --git a/crates/bymax-auth-crypto/src/password/tests.rs b/crates/bymax-auth-crypto/src/password/tests.rs index ccfa647..781d62c 100644 --- a/crates/bymax-auth-crypto/src/password/tests.rs +++ b/crates/bymax-auth-crypto/src/password/tests.rs @@ -371,4 +371,135 @@ mod cross { let argon_phc = hash(b"pw", &argon2_params()).unwrap_or_default(); assert!(needs_rehash(&argon_phc, &PasswordParams::default())); } + + // ----------------------------------------------------------------------- + // Cross-implementation conformance: `passwordHashFormat` in the wire contract + // ----------------------------------------------------------------------- + + /// Read `passwordHashFormat.vectors` from the shared cross-implementation wire contract. + /// + /// The file at `conformance/wire-contract.json` is held byte-identical by nest-auth, which + /// backs the same user table over the same Redis. Reading it here rather than copying the + /// strings in means a drift on either side turns that side red immediately. + fn contract_vectors() -> Vec { + let path = concat!( + env!("CARGO_MANIFEST_DIR"), + "/../../conformance/wire-contract.json" + ); + 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() + } + + #[test] + fn every_contract_password_hash_vector_verifies_here() { + // The vectors are real emitted output — one hash written by this crate, one written by + // nest-auth, and one in nest-auth's pre-PHC encoding. Each must verify, must refuse a + // wrong password, and must report the staleness the contract declares. + // + // This replaces an agreement that was prose: `credentialFormats.passwordHash` read + // "self-describing: the parameters travel with the hash", which BOTH sides satisfied + // while writing strings the other could not parse. Neither suite could fail, because + // neither was testing against the other's output. And the failure did not look like a + // parse error: `verify` is total, so an unreadable hash returns `Ok(false)` and the + // engine answers `invalid_credentials` — five of which trip the SHARED `lf:` counter + // and lock the account out of both backends using the owner's own correct password. + // Evaluated with the deployment configured at exactly the cost the PHC vectors record, + // which is what the contract's `needsRehash` field means. Against a HIGHER configured + // cost every vector is stale, and the assertion would say nothing about the encoding. + let at_vector_cost = PasswordParams { + scrypt: ScryptParams { + cost_factor: 1 << 14, + block_size: 8, + parallelization: 1, + }, + ..PasswordParams::default() + }; + let vectors = contract_vectors(); + assert_eq!( + vectors.len(), + 3, + "the contract must pin one vector per writer plus the legacy encoding — \ + it declared {} (did the file load?)", + vectors.len() + ); + + for vector in &vectors { + let password = vector + .get("password") + .and_then(serde_json::Value::as_str) + .unwrap_or_default(); + let stored = vector + .get("hash") + .and_then(serde_json::Value::as_str) + .unwrap_or_default(); + let written_by = vector + .get("writtenBy") + .and_then(serde_json::Value::as_str) + .unwrap_or_default(); + let wants_rehash = vector + .get("needsRehash") + .and_then(serde_json::Value::as_bool) + .unwrap_or_default(); + + assert!( + matches!(verify(password.as_bytes(), stored), Ok(true)), + "the vector written by {written_by} must verify here" + ); + assert!( + matches!(verify(b"definitely-not-the-password", stored), Ok(false)), + "the vector written by {written_by} must refuse a wrong password" + ); + assert_eq!( + needs_rehash(stored, &at_vector_cost), + wants_rehash, + "the vector written by {written_by} must report the staleness the contract declares" + ); + } + } + + #[test] + fn the_legacy_encoding_is_read_but_never_written() { + // It is a migration path. A migration that keeps producing the shape it is migrating + // away from never finishes, so nothing here mints it. + let phc = hash(b"pw", &PasswordParams::default()).unwrap_or_default(); + assert!(phc.starts_with("$scrypt$")); + assert!(!phc.starts_with("scrypt:")); + } + + #[test] + fn a_malformed_legacy_hash_is_refused_rather_than_verified() { + // Every rejection path in the legacy parser, so none of them can be widened into one + // that accepts a corrupt record under a cost it never used. + let salt = "d64ca8686e7dc4d3a9ddcbb48a44194e"; + let key = "cb".repeat(64); + for bad in [ + // Not the legacy prefix. + &format!("bcrypt:16384:8:1:{salt}:{key}"), + // A cost that is not a power of two: scrypt cannot have been run with it. + &format!("scrypt:16385:8:1:{salt}:{key}"), + // Zeroed cost parameters. + &format!("scrypt:16384:0:1:{salt}:{key}"), + &format!("scrypt:16384:8:0:{salt}:{key}"), + // Non-hex, odd-length and empty salt. + &format!("scrypt:16384:8:1:zzzz:{key}"), + &format!("scrypt:16384:8:1:abc:{key}"), + &format!("scrypt:16384:8:1::{key}"), + // A derived key that is not the 64 bytes this encoding always carried. + &format!("scrypt:16384:8:1:{salt}:cbcb"), + // A seventh field — not this encoding. + &format!("scrypt:16384:8:1:{salt}:{key}:extra"), + // Truncated. + &"scrypt:16384:8:1".to_owned(), + ] { + assert!( + matches!(verify(b"correct horse battery staple", bad), Ok(false)), + "must refuse {bad}" + ); + } + } } From 4ebde214378c664ebee78ca65e8f23113457b13e Mon Sep 17 00:00:00 2001 From: Maximiliano <40447063+msalvatti@users.noreply.github.com> Date: Tue, 4 Aug 2026 14:48:11 -0300 Subject: [PATCH 02/10] fix: resolve the tenant on the OAuth initiate, and admit non-Latin passwords MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- crates/bymax-auth-axum/src/routes/oauth.rs | 6 +- .../bymax-auth-core/src/services/auth/mod.rs | 6 + crates/bymax-auth-core/src/services/oauth.rs | 136 +++++++++++++++--- .../src/traits/common_password.rs | 92 +++++++++++- 4 files changed, 220 insertions(+), 20 deletions(-) diff --git a/crates/bymax-auth-axum/src/routes/oauth.rs b/crates/bymax-auth-axum/src/routes/oauth.rs index d349917..32533bd 100644 --- a/crates/bymax-auth-axum/src/routes/oauth.rs +++ b/crates/bymax-auth-axum/src/routes/oauth.rs @@ -70,11 +70,15 @@ async fn initiate( State(state): State, cookies: Cookies, Path(provider): Path, + RequestMeta(ctx): RequestMeta, ValidatedQuery(query): ValidatedQuery, ) -> Response { + // The context is what lets the engine consult the configured `TenantIdResolver`. Without + // it this route took `?tenantId=` verbatim — the only flow that did — and it is the flow + // that decides which tenant an account is provisioned into. match state .engine() - .oauth_initiate(&provider, &query.tenant_id) + .oauth_initiate(&provider, &query.tenant_id, &ctx) .await { Ok(redirect) => { diff --git a/crates/bymax-auth-core/src/services/auth/mod.rs b/crates/bymax-auth-core/src/services/auth/mod.rs index 5772da2..97ed262 100644 --- a/crates/bymax-auth-core/src/services/auth/mod.rs +++ b/crates/bymax-auth-core/src/services/auth/mod.rs @@ -699,6 +699,12 @@ mod tests { .await, Err(AuthError::Forbidden) )); + + // `oauth_initiate` is the eighth tenant-scoped flow and belongs on this list, but it + // needs a configured provider to reach the resolver at all (the provider is resolved + // first, so an unknown one fails before any consumer code runs). It is asserted in + // `services::oauth::tests::oauth_initiate_honours_the_tenant_resolver`, against a + // harness that wires Google. } #[tokio::test] diff --git a/crates/bymax-auth-core/src/services/oauth.rs b/crates/bymax-auth-core/src/services/oauth.rs index cfb7c66..ea03e7a 100644 --- a/crates/bymax-auth-core/src/services/oauth.rs +++ b/crates/bymax-auth-core/src/services/oauth.rs @@ -100,8 +100,17 @@ impl AuthEngine { /// provider authorization URL. The raw `state` is never stored — only its hash is a key — /// and only the `code_challenge` is exposed to the provider. /// - /// `tenant_id` is carried verbatim into the state and recovered on callback; it is **not** - /// validated here (the `on_oauth_login` hook enforces tenant membership). + /// The tenant is resolved through the configured [`TenantIdResolver`](crate::traits::TenantIdResolver) + /// before it is written into the state, exactly as login, register, the reset flows and + /// email verification resolve theirs (§24 invariant 8). This was the one door that still + /// took the caller's value verbatim, and it is the door that decides which tenant an + /// account gets PROVISIONED into — strictly more than the others were protecting. Its + /// previous rationale ("the `on_oauth_login` hook enforces tenant membership") did not + /// hold: the hook is handed the same `tenant_id` through its `HookContext`, so a hook + /// deciding on the profile alone admitted an attacker into any tenant they named. + /// + /// The RESOLVED value is what goes into the single-use state record, so the callback + /// cannot be talked into a different one either. /// /// # Errors /// @@ -112,14 +121,19 @@ impl AuthEngine { &self, provider: &str, tenant_id: &str, + ctx: &RequestContext, ) -> Result { // Resolve the provider first: an unknown provider fails without minting state. let provider_impl = self.resolve_oauth_provider(provider)?; + // A deployment that derives the tenant from the request has stated that the caller's + // value is not to be trusted. Resolved before any state is minted, so a request that + // the resolver refuses consumes nothing. + let tenant_id = self.resolve_tenant(tenant_id, ctx).await?; let state = generate_state(); let (code_verifier, code_challenge) = generate_pkce(); let payload = serde_json::to_string(&OAuthStatePayload { - tenant_id: tenant_id.to_owned(), + tenant_id, code_verifier, }) .map_err(oauth_state_serialize_failed)?; @@ -766,7 +780,7 @@ mod tests { /// Run a full initiate → callback, returning the callback outcome. The `code` is canned /// (the recording transport ignores it); the `state` is recovered from the authorize URL. async fn run_flow(h: &OAuthHarness) -> Result { - let url = h.engine.oauth_initiate("google", "t1").await; + let url = h.engine.oauth_initiate("google", "t1", &ctx()).await; let Ok(url) = url.map(|r| r.authorize_url) else { return Err(AuthError::OauthFailed) }; let state = extract_query_param(&url, "state").unwrap_or_default(); h.engine @@ -790,7 +804,7 @@ mod tests { // URL carrying the state and an S256 challenge. let hooks: Arc = Arc::new(DecisionHook(OAuthLoginResult::Create)); let Some(h) = harness(hooks, Arc::new(RoutingHttpClient::new()), false) else { return }; - let url = h.engine.oauth_initiate("google", "t1").await; + let url = h.engine.oauth_initiate("google", "t1", &ctx()).await; assert!( matches!(&url, Ok(r) if r.authorize_url.starts_with("https://accounts.google.com/")) ); @@ -818,11 +832,11 @@ mod tests { let hooks: Arc = Arc::new(DecisionHook(OAuthLoginResult::Create)); let Some(h) = harness(hooks, Arc::new(RoutingHttpClient::new()), false) else { return }; assert!(matches!( - h.engine.oauth_initiate("github", "t1").await, + h.engine.oauth_initiate("github", "t1", &ctx()).await, Err(AuthError::OauthFailed) )); assert!(matches!( - h.engine.oauth_initiate("BAD_NAME", "t1").await, + h.engine.oauth_initiate("BAD_NAME", "t1", &ctx()).await, Err(AuthError::OauthFailed) )); } @@ -840,7 +854,7 @@ mod tests { assert_eq!(result.user.oauth_provider.as_deref(), Some("google")); assert!(!result.access_token.is_empty()); // The PKCE verifier was forwarded on exchange and matches the issued challenge. - let body = h.engine.oauth_initiate("google", "t1").await; + let body = h.engine.oauth_initiate("google", "t1", &ctx()).await; assert!(body.is_ok()); let Some(exchange) = h.http.exchange_body() else { return }; assert!(exchange.contains("code_verifier=")); @@ -853,7 +867,7 @@ mod tests { // challenge that left in the authorize URL. let hooks: Arc = Arc::new(DecisionHook(OAuthLoginResult::Create)); let Some(h) = harness(hooks, Arc::new(RoutingHttpClient::new()), false) else { return }; - let url = h.engine.oauth_initiate("google", "t1").await; + let url = h.engine.oauth_initiate("google", "t1", &ctx()).await; let Ok(url) = url.map(|r| r.authorize_url) else { return }; let state = extract_query_param(&url, "state").unwrap_or_default(); let challenge = extract_query_param(&url, "code_challenge").unwrap_or_default(); @@ -972,7 +986,7 @@ mod tests { Err(AuthError::OauthFailed) )); // Issue a real state, consume it once, then replay it. - let url = h.engine.oauth_initiate("google", "t1").await; + let url = h.engine.oauth_initiate("google", "t1", &ctx()).await; let Ok(url) = url.map(|r| r.authorize_url) else { return }; let state = extract_query_param(&url, "state").unwrap_or_default(); assert!( @@ -999,7 +1013,7 @@ mod tests { // mismatched one are both fatal. let hooks: Arc = Arc::new(DecisionHook(OAuthLoginResult::Create)); let Some(h) = harness(hooks, Arc::new(RoutingHttpClient::new()), false) else { return }; - let url = h.engine.oauth_initiate("google", "t1").await; + let url = h.engine.oauth_initiate("google", "t1", &ctx()).await; let Ok(url) = url.map(|r| r.authorize_url) else { return }; let state = extract_query_param(&url, "state").unwrap_or_default(); @@ -1039,7 +1053,7 @@ mod tests { // callback unsatisfiable, and no other test would notice. let hooks: Arc = Arc::new(DecisionHook(OAuthLoginResult::Create)); let Some(h) = harness(hooks, Arc::new(RoutingHttpClient::new()), false) else { return }; - let Ok(redirect) = h.engine.oauth_initiate("google", "t1").await else { return }; + let Ok(redirect) = h.engine.oauth_initiate("google", "t1", &ctx()).await else { return }; assert_eq!( extract_query_param(&redirect.authorize_url, "state").as_deref(), Some(redirect.state.as_str()) @@ -1271,7 +1285,7 @@ mod tests { .build(); let Ok(engine) = engine else { return }; assert!(matches!( - engine.oauth_initiate("google", "t1").await, + engine.oauth_initiate("google", "t1", &ctx()).await, Err(AuthError::Internal(_)) )); } @@ -1386,7 +1400,7 @@ mod tests { .build(); let Ok(engine) = engine else { return }; - let url = engine.oauth_initiate("google", "t1").await; + let url = engine.oauth_initiate("google", "t1", &ctx()).await; let Ok(url) = url.map(|r| r.authorize_url) else { return }; let state = extract_query_param(&url, "state").unwrap_or_default(); let outcome = engine @@ -1426,7 +1440,7 @@ mod tests { .build(); let Ok(engine) = engine else { return }; - let url = engine.oauth_initiate("google", "t1").await; + let url = engine.oauth_initiate("google", "t1", &ctx()).await; let Ok(url) = url.map(|r| r.authorize_url) else { return }; let state = extract_query_param(&url, "state").unwrap_or_default(); let outcome = engine @@ -1461,7 +1475,7 @@ mod tests { let Ok(engine) = engine else { return }; // First sign-in creates the account and succeeds. - let url = engine.oauth_initiate("google", "t1").await; + let url = engine.oauth_initiate("google", "t1", &ctx()).await; let Ok(url) = url.map(|r| r.authorize_url) else { return }; let state = extract_query_param(&url, "state").unwrap_or_default(); assert!(matches!( @@ -1493,7 +1507,7 @@ mod tests { .build(); let Ok(linking) = linking else { return }; - let url = linking.oauth_initiate("google", "t1").await; + let url = linking.oauth_initiate("google", "t1", &ctx()).await; let Ok(url) = url.map(|r| r.authorize_url) else { return }; let state = extract_query_param(&url, "state").unwrap_or_default(); let banned = linking @@ -1535,4 +1549,92 @@ mod tests { assert!(matches!(cloned, OAuthOutcome::MfaChallenge(_))); assert!(format!("{outcome:?}").contains("MfaChallenge")); } + + /// A configured `TenantIdResolver` must win over `?tenantId=` on the OAuth initiate. + /// + /// This was the last flow reading the caller's value verbatim, and the worst one to leave: + /// it decides which tenant an account is PROVISIONED into. An attacker 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. + /// + /// The check is indirect but exact: the resolver refuses when no `host` header is present, + /// so a flow that consults it fails with `Forbidden` on an empty context and a flow that + /// ignores it mints a redirect. nest-auth has resolved here since its own fix; this side + /// had not, which is the drift the parity work exists to end. + #[tokio::test] + async fn oauth_initiate_honours_the_tenant_resolver() { + /// Resolves the tenant from the `host` header, refusing when it is absent. + struct HostTenantResolver; + + #[async_trait::async_trait] + impl crate::config::TenantIdResolver for HostTenantResolver { + async fn resolve( + &self, + parts: &crate::config::RequestParts, + ) -> Result { + match parts.host.as_deref() { + Some("") | None => Err(crate::config::TenantResolveError::Empty), + Some(host) => Ok(host.to_owned()), + } + } + } + + let http = Arc::new(RoutingHttpClient::new()); + let users = Arc::new(InMemoryUserRepository::new()); + let stores = Arc::new(InMemoryStores::new()); + let google = GoogleOAuthProvider::new(google_config(), http.clone()); + let mut cfg = base_config(); + cfg.controllers.oauth = true; + cfg.tenant_id_resolver = Some(Arc::new(HostTenantResolver)); + let Ok(engine) = AuthEngine::builder() + .config(cfg) + .environment(Environment::Test) + .user_repository(users) + .redis_stores(stores.clone()) + .oauth_provider(Arc::new(google)) + .oauth_state_store(stores.clone()) + .build() + else { + return; + }; + + // No `host`: the resolver refuses, and the spoofed body value must not stand in for it. + let empty = RequestContext::new("1.2.3.4", "ua", std::collections::BTreeMap::new()); + assert!( + matches!( + engine + .oauth_initiate("google", "victim-tenant", &empty) + .await, + Err(AuthError::Forbidden) + ), + "the initiate must consult the resolver, not the query string" + ); + + // With a `host`, the RESOLVED tenant is what lands in the single-use state record — so + // the callback cannot be talked into a different one either. + let mut headers = std::collections::BTreeMap::new(); + headers.insert("host".to_owned(), "resolved-tenant".to_owned()); + let resolved_ctx = RequestContext::new("1.2.3.4", "ua", headers); + let Ok(redirect) = engine + .oauth_initiate("google", "victim-tenant", &resolved_ctx) + .await + else { + panic!("a resolvable request must mint a redirect") + }; + let stored = OAuthStateStore::take_state(stores.as_ref(), &state_key(&redirect.state)) + .await + .ok() + .flatten() + .unwrap_or_default(); + assert!( + stored.contains("resolved-tenant"), + "the state must carry the resolved tenant, not the requested one" + ); + assert!( + !stored.contains("victim-tenant"), + "the requested tenant must not survive into the state record" + ); + } } diff --git a/crates/bymax-auth-core/src/traits/common_password.rs b/crates/bymax-auth-core/src/traits/common_password.rs index 763210a..0d0b423 100644 --- a/crates/bymax-auth-core/src/traits/common_password.rs +++ b/crates/bymax-auth-core/src/traits/common_password.rs @@ -273,7 +273,17 @@ pub fn reduce_to_base_word(password: &str) -> String { undecorated .chars() .map(undo_leet) - .filter(char::is_ascii_alphanumeric) + // Letters and numbers in ANY script, not just ASCII. `is_ascii_alphanumeric` discarded + // every non-Latin character, so a password written in Cyrillic, Han, Kana, Hangul, + // Greek, Arabic, Hebrew or Thai reduced to the empty string — and an empty base is + // below `MIN_BASE_LENGTH`, which `is_breached` reads as "breached". Users of those + // scripts were refused on register, reset and change, and told their strong password + // was commonly used, which pushes them toward the strictly smaller ASCII keyspace. + // + // Keeping the characters also makes a consumer's non-Latin blocklist entry reachable: + // extra entries are normalized through this same function, so under the ASCII filter + // every one of them collapsed to "" and could never match. + .filter(|c| c.is_alphanumeric()) .collect() } @@ -352,7 +362,10 @@ impl PasswordBreachChecker for CommonPasswordChecker { // Almost nothing survived the reduction, so the password was decoration wrapped around // a fragment: `!!!!!!!!` and `12345678` leave nothing at all, `a1234567` leaves `a`. - if base.len() < MIN_BASE_LENGTH { + // Counted in CHARACTERS, not bytes: `len()` is the UTF-8 byte count, so a two-character + // Han base would score 6 and clear a floor meant to be about how many characters the + // reduction actually kept. nest-auth counts code points for the same reason. + if base.chars().count() < MIN_BASE_LENGTH { return true; } @@ -499,4 +512,79 @@ mod tests { .await ); } + + // ----------------------------------------------------------------------- + // Non-ASCII scripts + // ----------------------------------------------------------------------- + + /// A strong passphrase in a script that is not Latin must be ADMITTED. + /// + /// This was a live defect in both implementations. `reduce_to_base_word` filtered to + /// `is_ascii_alphanumeric`, so a password written 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. Every such user was refused on register, on reset and + /// on change, and told their password was commonly used. The effect was to push a whole + /// class of users onto the strictly smaller ASCII keyspace, which inverts the purpose of a + /// breach screen. Neither suite caught it: both tested ASCII inputs only. + #[tokio::test] + async fn a_strong_non_latin_password_is_admitted() { + let checker = CommonPasswordChecker::new(); + for password in [ + "пароль-очень-длинный", + "日本語のパスワードです", + "κωδικόςπρόσβασης", + "비밀번호가아주깁니다", + "סיסמאארוכהמאוד", + "كلمةالمرورطويلةجدا", + "ЖЫрафЖираф77", + "Ünterwegs-2024", + ] { + assert!( + !checker.is_breached(password).await, + "{password} is strong and must be admitted" + ); + } + } + + /// The characters survive the reduction rather than merely being tolerated, which is what + /// makes a consumer's non-Latin extra word reachable at all: extras are normalized through + /// the same function, so under the ASCII filter every one of them became "". + #[test] + fn non_ascii_letters_survive_the_reduction() { + assert_eq!(reduce_to_base_word("Пароль"), "пароль"); + assert_eq!(reduce_to_base_word("日本語"), "日本語"); + assert_eq!(reduce_to_base_word("Ünterwegs-2024"), "ünterwegs"); + } + + #[tokio::test] + async fn a_non_latin_extra_word_matches() { + let checker = CommonPasswordChecker::with_extra_words(["пароль"]); + assert!(checker.is_breached("Пароль123").await); + // A different Cyrillic word is still admitted — the entry blocks itself, not the script. + assert!(!checker.is_breached("черепаха").await); + } + + /// Widening what reduces to a non-empty base must not widen what gets through. Each of + /// these is decoration around a fragment too short to be a word, which is what the length + /// floor exists for. + #[tokio::test] + async fn the_weak_ascii_shapes_are_still_refused() { + let checker = CommonPasswordChecker::new(); + for password in ["!!!!!!!!", "12345678", "a1234567", "abc12345"] { + assert!( + checker.is_breached(password).await, + "{password} must be refused" + ); + } + } + + /// A repeated single character in a non-Latin script is now caught by the repeated-unit + /// rule instead of by collapsing to "". Same answer, reached for the right reason — and the + /// reason is what keeps holding when the script changes again. + #[tokio::test] + async fn a_repeated_non_ascii_character_is_refused_by_the_repetition_rule() { + let checker = CommonPasswordChecker::new(); + assert!(checker.is_breached("аааааааа").await); + assert!(checker.is_breached("東東東東東東東東").await); + } } From cee56b88131e4bee076a9ba7bde732a16705fa0a Mon Sep 17 00:00:00 2001 From: Maximiliano <40447063+msalvatti@users.noreply.github.com> Date: Tue, 4 Aug 2026 16:13:45 -0300 Subject: [PATCH 03/10] fix: align four route rate limits with nest-auth, and name the account in the logs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **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. --- crates/bymax-auth-axum/src/dto.rs | 40 ++++-- crates/bymax-auth-axum/src/routes/mfa.rs | 8 +- crates/bymax-auth-axum/src/routes/platform.rs | 20 ++- .../src/routes/platform_mfa.rs | 3 +- crates/bymax-auth-axum/tests/adapter.rs | 123 ++++++++++++++++++ crates/bymax-auth-core/src/lib.rs | 2 +- crates/bymax-auth-core/src/normalize.rs | 39 ++++++ .../src/services/auth/login.rs | 15 ++- crates/bymax-auth-core/src/services/oauth.rs | 11 +- .../src/services/token_manager.rs | 24 ++++ crates/bymax-auth-redis/src/stores/session.rs | 29 +++++ 11 files changed, 293 insertions(+), 21 deletions(-) diff --git a/crates/bymax-auth-axum/src/dto.rs b/crates/bymax-auth-axum/src/dto.rs index 29778ba..b9f8cf0 100644 --- a/crates/bymax-auth-axum/src/dto.rs +++ b/crates/bymax-auth-axum/src/dto.rs @@ -9,6 +9,28 @@ use garde::Validate; use serde::Deserialize; +/// Refuse a value carrying a control character. +/// +/// `tenant_id` is the widest attacker-controlled field on this surface: it arrives in the body +/// of `/login`, `/register`, `/verify-email`, `/password/forgot-password` and +/// `/oauth/{provider}` — all public — and is the caller's own value whenever no +/// `TenantIdResolver` is configured, which is the default. It then reaches a `tracing` event, a +/// Redis key segment and an HMAC preimage. +/// +/// A length bound alone does not cover that. nest-auth has always rejected control characters +/// here, and this side accepted them, so the same request was a 400 on one backend and a 200 on +/// the other — the exact divergence `requestFieldBounds` exists to prevent, and one that also +/// let a caller forge a record in a plain-text log pipeline (ASVS 16.4.1). +/// +/// `log_safe` is the second lock at the log site, for values that reach one without passing a +/// DTO — a host's `TenantIdResolver` returns whatever it returns. +fn no_control_characters(value: &str, _: &()) -> garde::Result { + if value.chars().any(|c| c.is_control() || c == '\u{7f}') { + return Err(garde::Error::new("must not contain control characters")); + } + Ok(()) +} + /// `POST /auth/register` body. #[derive(Debug, Deserialize, Validate)] #[serde(rename_all = "camelCase", deny_unknown_fields)] @@ -23,7 +45,7 @@ pub struct RegisterDto { #[garde(length(min = 2, max = 128))] pub name: String, /// The tenant scope; ignored when a `TenantIdResolver` is configured. - #[garde(length(min = 1, max = 128))] + #[garde(length(min = 1, max = 128), custom(no_control_characters))] pub tenant_id: String, } @@ -44,7 +66,7 @@ pub struct LoginDto { #[garde(length(min = 1, max = 128))] pub password: String, /// The tenant scope; ignored when a `TenantIdResolver` is configured. - #[garde(length(min = 1, max = 128))] + #[garde(length(min = 1, max = 128), custom(no_control_characters))] pub tenant_id: String, } @@ -56,7 +78,7 @@ pub struct ForgotPasswordDto { #[garde(email, length(max = 255))] pub email: String, /// The tenant scope. - #[garde(length(min = 1, max = 128))] + #[garde(length(min = 1, max = 128), custom(no_control_characters))] pub tenant_id: String, } @@ -119,7 +141,7 @@ pub struct ResetPasswordDto { #[garde(inner(length(min = 64, max = 64)))] pub verified_token: Option, /// The tenant scope. - #[garde(length(min = 1, max = 128))] + #[garde(length(min = 1, max = 128), custom(no_control_characters))] pub tenant_id: String, } @@ -134,7 +156,7 @@ pub struct VerifyOtpDto { #[garde(length(min = 4, max = 8))] pub otp: String, /// The tenant scope. - #[garde(length(min = 1, max = 128))] + #[garde(length(min = 1, max = 128), custom(no_control_characters))] pub tenant_id: String, } @@ -146,7 +168,7 @@ pub struct ResendOtpDto { #[garde(email, length(max = 255))] pub email: String, /// The tenant scope. - #[garde(length(min = 1, max = 128))] + #[garde(length(min = 1, max = 128), custom(no_control_characters))] pub tenant_id: String, } @@ -166,7 +188,7 @@ pub struct VerifyEmailDto { #[garde(length(min = 6, max = 6))] pub otp: String, /// The tenant scope. - #[garde(length(min = 1, max = 128))] + #[garde(length(min = 1, max = 128), custom(no_control_characters))] pub tenant_id: String, } @@ -178,7 +200,7 @@ pub struct ResendVerificationDto { #[garde(email, length(max = 255))] pub email: String, /// The tenant scope. - #[garde(length(min = 1, max = 128))] + #[garde(length(min = 1, max = 128), custom(no_control_characters))] pub tenant_id: String, } @@ -363,7 +385,7 @@ pub struct OAuthInitiateQuery { /// The tenant the user will join on success; carried in the Redis state and recovered /// on callback. Not validated against the DB here (the `on_oauth_login` hook enforces /// tenant membership). - #[garde(length(min = 1, max = 128))] + #[garde(length(min = 1, max = 128), custom(no_control_characters))] pub tenant_id: String, } diff --git a/crates/bymax-auth-axum/src/routes/mfa.rs b/crates/bymax-auth-axum/src/routes/mfa.rs index 3b43056..2682864 100644 --- a/crates/bymax-auth-axum/src/routes/mfa.rs +++ b/crates/bymax-auth-axum/src/routes/mfa.rs @@ -51,7 +51,13 @@ pub(crate) fn routes(config: &AxumAuthConfig, ip_source: ClientIpSource) -> Rout ) .route( "/recovery-codes", - crate::router::throttled(post(recovery_codes), limits.mfa_setup, ip_source), + // `mfa_disable`, not `mfa_setup`. nest-auth serves regeneration under the + // disable throttle and says why: the security posture is identical — + // authenticated, TOTP-gated, and MFA-affecting state. `mfa_setup` is 5/60 + // against `mfa_disable`'s 3/300, so this route was 25x more permissive here + // than on the sibling backend: a wider TOTP-guessing surface, and a way to + // invalidate a victim's recovery codes repeatedly. + crate::router::throttled(post(recovery_codes), limits.mfa_disable, ip_source), ), ) } diff --git a/crates/bymax-auth-axum/src/routes/platform.rs b/crates/bymax-auth-axum/src/routes/platform.rs index 49de492..7bda80b 100644 --- a/crates/bymax-auth-axum/src/routes/platform.rs +++ b/crates/bymax-auth-axum/src/routes/platform.rs @@ -41,13 +41,29 @@ pub(crate) fn routes(config: &AxumAuthConfig, ip_source: ClientIpSource) -> Rout "/platform/mfa/challenge", crate::router::throttled(post(mfa_challenge), limits.mfa_challenge, ip_source), ) + // `/platform/me` is deliberately unthrottled, matching nest-auth: it is a cheap read + // behind a verified access token, and limiting it would cap a dashboard's own polling. .route("/platform/me", get(me)) - .route("/platform/logout", post(logout)) + // `/platform/logout` is PUBLIC by design (a caller with an expired access token must + // still be able to kill their refresh session), which is exactly why it needs a limit: + // unauthenticated and unthrottled, it 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 — for any 64-hex string a caller invents. nest-auth + // has always served it under `logout` (20/60). + .route( + "/platform/logout", + crate::router::throttled(post(logout), limits.logout, ip_source), + ) .route( "/platform/refresh", crate::router::throttled(post(refresh), limits.refresh, ip_source), ) - .route("/platform/sessions", delete(revoke_all)) + // Revoking every session is a state change over the whole account, and nest-auth + // serves it under `revoke_all_sessions` (5/60). It was unthrottled here. + .route( + "/platform/sessions", + crate::router::throttled(delete(revoke_all), limits.revoke_all_sessions, ip_source), + ) } /// `POST /auth/platform/login` (200). Public. Full platform session or an MFA challenge. diff --git a/crates/bymax-auth-axum/src/routes/platform_mfa.rs b/crates/bymax-auth-axum/src/routes/platform_mfa.rs index dae1290..c930e1f 100644 --- a/crates/bymax-auth-axum/src/routes/platform_mfa.rs +++ b/crates/bymax-auth-axum/src/routes/platform_mfa.rs @@ -38,7 +38,8 @@ pub(crate) fn routes(config: &AxumAuthConfig, ip_source: ClientIpSource) -> Rout ) .route( "/platform/mfa/recovery-codes", - crate::router::throttled(post(recovery_codes), limits.mfa_setup, ip_source), + // `mfa_disable`, not `mfa_setup` — see the dashboard twin in `routes/mfa.rs`. + crate::router::throttled(post(recovery_codes), limits.mfa_disable, ip_source), ) } diff --git a/crates/bymax-auth-axum/tests/adapter.rs b/crates/bymax-auth-axum/tests/adapter.rs index 1f1146c..97f974b 100644 --- a/crates/bymax-auth-axum/tests/adapter.rs +++ b/crates/bymax-auth-axum/tests/adapter.rs @@ -3418,3 +3418,126 @@ async fn the_address_change_routes_move_an_account_only_after_the_new_address_pr .await; assert_eq!(under_old.status, StatusCode::UNAUTHORIZED); } + +// ---------------------------------------------------------------------------------------- +// Which limit each route actually wears +// ---------------------------------------------------------------------------------------- + +/// Build a router whose limits are all generous except the one `narrow` names, which is +/// bottled down to a single request. Whatever 429s under it is what that route is wired to. +fn router_with_one_narrow_limit( + harness: &common::Harness, + narrow: fn(&mut bymax_auth_axum::RateLimitConfig, Option), +) -> axum::Router { + let mut config = + bymax_auth_axum::AxumAuthConfig::new(bymax_auth_axum::ClientIpSource::PeerAddr); + // Every limit generous enough that nothing else can trip during the probe. + let generous = Some(bymax_auth_axum::RateLimit::new(10_000, 60)); + let mut limits = bymax_auth_axum::RateLimitConfig::default(); + for setter in ALL_LIMIT_SETTERS { + setter(&mut limits, generous); + } + narrow(&mut limits, Some(bymax_auth_axum::RateLimit::new(1, 60))); + config.rate_limits = limits; + bymax_auth_axum::AuthRouter::from_engine(harness.engine.clone(), config).into_router() +} + +/// Every field of `RateLimitConfig`, as setters, so the helper above can widen them all +/// without naming each one at every call site — and so a NEW limit added to the struct shows +/// up here rather than being silently left at its default during a probe. +#[allow(clippy::type_complexity)] +const ALL_LIMIT_SETTERS: &[fn( + &mut bymax_auth_axum::RateLimitConfig, + Option, +)] = &[ + |c, v| c.login = v, + |c, v| c.register = v, + |c, v| c.refresh = v, + |c, v| c.logout = v, + |c, v| c.mfa_setup = v, + |c, v| c.mfa_disable = v, + |c, v| c.mfa_challenge = v, + |c, v| c.revoke_all_sessions = v, +]; + +/// Hit `request` twice against `app` and report whether the second call was throttled. +async fn trips_on_second_call(app: &axum::Router, build: impl Fn() -> Req) -> bool { + let first = build().send(app).await; + assert_ne!( + first.status, + StatusCode::TOO_MANY_REQUESTS, + "the FIRST call must pass — a burst of 1 means the limiter trips on the second" + ); + let second = build().send(app).await; + assert_ne!( + second.status, + StatusCode::NOT_FOUND, + "the route must be mounted — a 404 would make this assertion vacuous" + ); + second.status == StatusCode::TOO_MANY_REQUESTS +} + +/// Every value in `rateLimits` is pinned against the shared contract, and the contract is +/// checked for extra names — but nothing asserted which limit each ROUTE is wired to. Wiring +/// `login` to `register`'s 10/3600 would have passed the entire suite. +/// +/// That gap hid four real divergences from nest-auth, found by the second audit: +/// `/platform/logout` and `/platform/sessions` carried NO limit 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 TOTP-gated route that can also +/// invalidate a victim's recovery codes repeatedly. +/// +/// `/platform/logout` is the one that mattered most: it is PUBLIC by design, so unthrottled it +/// let an unauthenticated caller drive `find_session`, an HMAC verify, `revoke_session` and +/// `delete_grace_pointer` — two to four round trips each holding a pool connection — for any +/// 64-hex string they invent. +#[tokio::test] +async fn each_route_is_served_under_the_limit_it_declares() { + // Every optional group on: the four routes under test live in `platform` and `mfa`, and a + // group that is not mounted answers 404, which would make every assertion below vacuous. + let Some(h) = build(EngineSpec { + platform: true, + mfa: true, + sessions: true, + ..EngineSpec::default() + }) else { + return; + }; + + // Public platform logout, wired to `logout`. + let app = router_with_one_narrow_limit(&h, |c, v| c.logout = v); + assert!( + trips_on_second_call(&app, || Req::post("/auth/platform/logout") + .json(serde_json::json!({ "refreshToken": "a".repeat(64) }))) + .await, + "POST /auth/platform/logout must be served under the `logout` limit" + ); + + // Revoke-all, wired to `revoke_all_sessions`. + let app = router_with_one_narrow_limit(&h, |c, v| c.revoke_all_sessions = v); + assert!( + trips_on_second_call(&app, || Req::delete("/auth/platform/sessions")).await, + "DELETE /auth/platform/sessions must be served under the `revoke_all_sessions` limit" + ); + + // Both recovery-code routes, wired to `mfa_disable` rather than `mfa_setup`. + let app = router_with_one_narrow_limit(&h, |c, v| c.mfa_disable = v); + assert!( + trips_on_second_call(&app, || Req::post("/auth/mfa/recovery-codes")).await, + "POST /auth/mfa/recovery-codes must be served under the `mfa_disable` limit" + ); + + let app = router_with_one_narrow_limit(&h, |c, v| c.mfa_disable = v); + assert!( + trips_on_second_call(&app, || Req::post("/auth/platform/mfa/recovery-codes")).await, + "POST /auth/platform/mfa/recovery-codes must be served under the `mfa_disable` limit" + ); + + // The negative half: narrowing `mfa_setup` must NOT throttle a recovery-code request, or + // the assertions above would pass for a route wired to either name. + let app = router_with_one_narrow_limit(&h, |c, v| c.mfa_setup = v); + assert!( + !trips_on_second_call(&app, || Req::post("/auth/mfa/recovery-codes")).await, + "recovery-codes must NOT be served under the `mfa_setup` limit" + ); +} diff --git a/crates/bymax-auth-core/src/lib.rs b/crates/bymax-auth-core/src/lib.rs index 3bf9b22..7de0625 100644 --- a/crates/bymax-auth-core/src/lib.rs +++ b/crates/bymax-auth-core/src/lib.rs @@ -52,7 +52,7 @@ pub use engine::{AuthEngine, AuthEngineBuilder}; #[doc(inline)] pub use error::{ConfigError, RepositoryError}; #[doc(inline)] -pub use normalize::{mask_email, normalize_email}; +pub use normalize::{log_safe, mask_email, normalize_email}; #[cfg(feature = "oauth")] #[doc(inline)] pub use providers::GoogleOAuthProvider; diff --git a/crates/bymax-auth-core/src/normalize.rs b/crates/bymax-auth-core/src/normalize.rs index f04a37c..cf06fd4 100644 --- a/crates/bymax-auth-core/src/normalize.rs +++ b/crates/bymax-auth-core/src/normalize.rs @@ -56,6 +56,45 @@ pub fn mask_email(email: &str) -> String { } } +/// Sanitize a request-derived value before it is interpolated into a log line. +/// +/// A log line is a record, and a value carrying a newline writes a second one. `tracing`'s +/// `fmt` subscriber — the default a consumer reaches for — writes plain text, so an +/// unauthenticated caller who controls any field that reaches a log event can forge records in +/// it. `tenant_id` is the widest such field: it arrives in the body of `/login`, `/register`, +/// `/verify-email`, `/password/forgot-password` and `/oauth/{provider}`, all public, and is +/// attacker-chosen whenever no `TenantIdResolver` is configured — the default. A value like +/// `acme\nINFO login: success user_id=` puts a fabricated successful sign-in into the +/// operator's SIEM, or truncates the genuine records around it. ASVS v5 §16.4.1 requires log +/// data to be sanitized against exactly this. +/// +/// The value is replaced wholesale rather than escaped: an operator reading `` +/// learns the useful thing, which is that the field carried something no legitimate caller +/// sends. Anything printable passes through untouched, so a tenant naming scheme this library +/// cannot anticipate still reads normally. +/// +/// Byte-for-byte the same rule as nest-auth's `logSafe`, so one log pipeline fed by both +/// backends renders one value one way. The DTOs reject control characters at the boundary as +/// well; this is the second lock, because a `TenantIdResolver` is the host's code and returns +/// whatever it returns. +/// +/// # Examples +/// +/// ``` +/// # use bymax_auth_core::log_safe; +/// assert_eq!(log_safe("acme-corp"), "acme-corp"); +/// assert_eq!(log_safe("acme\nINFO forged"), ""); +/// ``` +#[must_use] +pub fn log_safe(value: &str) -> String { + // C0, DEL and C1 — every character that can forge a record boundary in a line-oriented + // pipeline. `is_control` covers C0 and C1 but not DEL, which is named explicitly. + if value.chars().any(|c| c.is_control() || c == '\u{7f}') { + return "".to_owned(); + } + value.to_owned() +} + #[cfg(test)] mod tests { use super::{mask_email, normalize_email}; diff --git a/crates/bymax-auth-core/src/services/auth/login.rs b/crates/bymax-auth-core/src/services/auth/login.rs index d6c3e21..c4d4e6e 100644 --- a/crates/bymax-auth-core/src/services/auth/login.rs +++ b/crates/bymax-auth-core/src/services/auth/login.rs @@ -12,7 +12,7 @@ use bymax_auth_types::{ use crate::context::RequestContext; use crate::engine::AuthEngine; -use crate::normalize::{mask_email, normalize_email}; +use crate::normalize::{log_safe, mask_email, normalize_email}; use crate::services::auth::detached::{ run_after_login, run_rehash_password, run_update_last_login, }; @@ -226,7 +226,18 @@ impl AuthEngine { user_id: Option<&str>, hook_ctx: &HookContext, ) -> Result { - tracing::warn!("login: invalid credentials"); + // Named, like the success line six lines up and like nest-auth's own refusal. Both + // values are parameters of this function and were simply unused: an operator reading a + // run of these could see that credentials were being refused and not for which account + // or tenant, which is the difference between a log and an audit trail (ASVS 16.2.1). + // The address is masked and the tenant sanitized — `tenant_id` is attacker-chosen from + // the request body whenever no `TenantIdResolver` is configured, which is the default, + // and a raw newline in it forges a record on a plain-text subscriber. + tracing::warn!( + email = %mask_email(email), + tenant_id = %log_safe(tenant_id), + "login: invalid credentials" + ); self.brute_force().record_failure(identifier).await?; self.fire_login_failed( email, diff --git a/crates/bymax-auth-core/src/services/oauth.rs b/crates/bymax-auth-core/src/services/oauth.rs index ea03e7a..1e2bca4 100644 --- a/crates/bymax-auth-core/src/services/oauth.rs +++ b/crates/bymax-auth-core/src/services/oauth.rs @@ -1617,12 +1617,13 @@ mod tests { let mut headers = std::collections::BTreeMap::new(); headers.insert("host".to_owned(), "resolved-tenant".to_owned()); let resolved_ctx = RequestContext::new("1.2.3.4", "ua", headers); - let Ok(redirect) = engine + // Asserted first, destructured second: the workspace denies `panic!` in tests too, so + // the failure has to come from the assertion rather than from an `else` arm. + let minted = engine .oauth_initiate("google", "victim-tenant", &resolved_ctx) - .await - else { - panic!("a resolvable request must mint a redirect") - }; + .await; + assert!(minted.is_ok(), "a resolvable request must mint a redirect"); + let Ok(redirect) = minted else { return }; let stored = OAuthStateStore::take_state(stores.as_ref(), &state_key(&redirect.state)) .await .ok() diff --git a/crates/bymax-auth-core/src/services/token_manager.rs b/crates/bymax-auth-core/src/services/token_manager.rs index 08054c2..3e2e484 100644 --- a/crates/bymax-auth-core/src/services/token_manager.rs +++ b/crates/bymax-auth-core/src/services/token_manager.rs @@ -578,7 +578,18 @@ impl TokenManagerService { // signature of a stolen token. Revoke the whole family (every live descendant // of that login) so the thief's chain dies too, then reject: every holder must // re-authenticate (§12.5.2, OWASP rotation with automatic reuse detection). + // Named, on both lines. This is the strongest compromise signal the library + // produces, and it used to be logged as bare prose: the account it concerns + // reached only a consumer who had wired `on_refresh_token_reuse_detected`, + // and the shipped hooks are no-ops. On a default deployment the one + // unambiguous theft signal was anonymous in the log and nowhere else, so an + // operator could tell that something happened and not to whom (ASVS 16.2.1). + // + // Two events rather than one: the detection is the finding and the revocation + // is the response to it, and a `revoke_family` that fails must not take the + // finding down with it. The owner is only knowable after the revocation. tracing::warn!( + family_id = %family, "refresh: reuse of a consumed refresh token detected — revoking the token family" ); // The owner comes back from the revocation, and can come from nowhere @@ -588,6 +599,11 @@ impl TokenManagerService { .session_store .revoke_family(SessionKind::Dashboard, &family) .await?; + tracing::warn!( + user_id = owner.as_deref().unwrap_or(""), + family_id = %family, + "refresh: token family revoked after reuse detection" + ); self.fire_reuse_detected(owner.as_deref(), &family).await; Err(AuthError::RefreshTokenInvalid) } @@ -782,7 +798,10 @@ impl TokenManagerService { RotateOutcome::Reused(family) => { // Post-grace replay of a consumed platform refresh token: revoke the whole // family and reject, the platform-keyspace analogue of the dashboard path. + // Named for the same reason as the dashboard plane, and more so: this is the + // highest-privilege identity in the system. tracing::warn!( + family_id = %family, "platform refresh: reuse of a consumed refresh token detected — revoking the token family" ); // The owner comes back from the revocation, and can come from nowhere @@ -792,6 +811,11 @@ impl TokenManagerService { .session_store .revoke_family(SessionKind::Platform, &family) .await?; + tracing::warn!( + user_id = owner.as_deref().unwrap_or(""), + family_id = %family, + "platform refresh: token family revoked after reuse detection" + ); self.fire_reuse_detected(owner.as_deref(), &family).await; Err(AuthError::RefreshTokenInvalid) } diff --git a/crates/bymax-auth-redis/src/stores/session.rs b/crates/bymax-auth-redis/src/stores/session.rs index 579aae8..30fe0f0 100644 --- a/crates/bymax-auth-redis/src/stores/session.rs +++ b/crates/bymax-auth-redis/src/stores/session.rs @@ -489,8 +489,37 @@ impl RedisStores { let mut conn = self.connection().await?; let members: Vec = conn.smembers(&sess_key).await?; let mut details = Vec::with_capacity(members.len()); + let grace_prefix = format!("{}:", prefixes.rp.as_str()); + let namespace = keys.namespace().to_owned(); for member in &members { let Some(hash) = live_member_hash(member, prefixes.rt) else { + // Not a live session. If it is a grace pointer whose key has already expired, + // drop the member here rather than leaving it: a rotation removes `rt:{old}` + // and adds TWO members — `rt:{new}` and `rp:{old}` — and until now only a full + // revoke-all ever removed the second. The `rp:` KEY dies with the grace window + // (30 s by default); the MEMBER did not, while `refresh_rotate` re-arms the + // set's own TTL on every rotation. The index therefore gained one permanent + // ~70-byte entry per refresh and never aged out while the account was in use. + // + // It is a growth defect with an amplifier attached, because every reader of + // this index is linear in its size: this method, `sweep_grace_pointers` (two + // sequential round trips per member), and `invalidate_user_sessions`, which + // iterates it inside a Lua script and so blocks the whole single-threaded + // store. One stolen refresh token rotated at the route limit adds ~14k members + // a day to its own index, with no ceiling. + // + // Pruned from the LIST path rather than from the rotation: the rotation is the + // hot path and an O(n) sweep there would trade a slow leak for a slow refresh. + // Session-cap enforcement lists too, so a login bounds it. + // + // A pointer still inside its window is left alone — that is what lets a + // revoke-all also kill a token rotated away moments earlier. + if member.starts_with(&grace_prefix) { + let live: bool = conn.exists(format!("{namespace}:{member}")).await?; + if !live { + let _: i64 = conn.srem(&sess_key, member).await?; + } + } continue; }; // The detail record is keyed by the BARE hash, so the member's prefix is stripped. From 4bbeaa038ba13e097d8d823f6a311652d805a472 Mon Sep 17 00:00:00 2001 From: Maximiliano <40447063+msalvatti@users.noreply.github.com> Date: Wed, 5 Aug 2026 14:36:18 -0300 Subject: [PATCH 04/10] docs(core): say that the shipped breach corpus is ASCII MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- crates/bymax-auth-core/src/traits/common_password.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/crates/bymax-auth-core/src/traits/common_password.rs b/crates/bymax-auth-core/src/traits/common_password.rs index 0d0b423..eae3802 100644 --- a/crates/bymax-auth-core/src/traits/common_password.rs +++ b/crates/bymax-auth-core/src/traits/common_password.rs @@ -314,6 +314,16 @@ fn is_padded_repeat(value: &str) -> bool { /// knows nothing about breach corpora. A deployment that wants that extends it with /// [`CommonPasswordChecker::with_extra_words`] (the context-specific words ASVS v5 §6.2.11 asks /// for) or supplies the HIBP checker, which searches a real corpus over the network. +/// +/// **The shipped base words are ASCII.** The reduction preserves letters and numbers in any +/// script — a strong Cyrillic, Han, Kana, Hangul, Greek, Arabic, Hebrew or Thai passphrase is +/// admitted, and used to be refused outright with the "commonly used" error, which pushed those +/// users onto the smaller ASCII keyspace. But the list itself holds no entries in those +/// scripts, so the equivalent of `password` in one of them passes this screen. A deployment +/// serving those users should add the common ones for its locale through +/// [`CommonPasswordChecker::with_extra_words`]; extras are normalized through the same +/// reduction, so a non-Latin entry matches a decorated form of itself the way an ASCII one +/// does. Held in step with nest-auth's `CommonPasswordChecker`. pub struct CommonPasswordChecker { blocked: HashSet, } From 2fcf29933b807788912344e47380d155aa9aefed Mon Sep 17 00:00:00 2001 From: Maximiliano <40447063+msalvatti@users.noreply.github.com> Date: Wed, 5 Aug 2026 18:06:20 -0300 Subject: [PATCH 05/10] refactor(crypto): drop the legacy password-hash reader; PHC is the only encoding MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .cargo/mutants.toml | 15 +- conformance/wire-contract.json | 17 +-- .../bymax-auth-crypto/src/password/legacy.rs | 131 ------------------ crates/bymax-auth-crypto/src/password/mod.rs | 4 +- crates/bymax-auth-crypto/src/password/phc.rs | 16 +-- .../bymax-auth-crypto/src/password/tests.rs | 53 +------ docs/technical_specification.md | 4 +- 7 files changed, 26 insertions(+), 214 deletions(-) delete mode 100644 crates/bymax-auth-crypto/src/password/legacy.rs diff --git a/.cargo/mutants.toml b/.cargo/mutants.toml index fd51672..d83edd4 100644 --- a/.cargo/mutants.toml +++ b/.cargo/mutants.toml @@ -93,14 +93,11 @@ additional_cargo_args = ["--all-features"] # were shifted into disjoint bit ranges (`<< 24`, `<< 16`, `<< 8`, none), so no two # operands share a set bit and XOR produces the identical word. The `| with &` mutants of # the same expression are NOT equivalent and the RFC 4226 vectors kill them. -# 11. `replace is_legacy -> bool with false` — its only caller is `needs_rehash`, where it is -# a fast path: a legacy `scrypt:hex:hex` string can never parse as a current PHC, so the -# fallback it short-circuits answers `true` for exactly the same inputs. The `with true` -# mutant of the same function is NOT equivalent (it would flag every current hash as -# stale) and is killed. -# 12. `replace | with ^ in decode_hex` — same shape as 1 and 10: `(hi << 4) | lo` combines a -# high nibble with a value `hex_nibble` bounds to 0..=15, so the two never share a set bit -# and XOR writes the identical byte. +# 11-12. REMOVED. These described `is_legacy` and a `decode_hex` built on `(hi << 4) | lo`, +# both part of the legacy credential reader that 6da0382 deleted, and neither matched a +# mutant any more. A stale exclusion is worse than none — it silently suppresses a future +# function that happens to share the name, and nobody decided that. The numbering below is +# left as it was so the remaining entries keep their identities. # 13. `EmailProvider::send_email_changed_notification` default body — the same shape as 8: it is # literally `let _ = (args); Ok(())`, where the binding exists only to tell the compiler the # arguments are deliberately unused. Replacing it with `Ok(())` is the same program. The @@ -119,7 +116,5 @@ exclude_re = [ 'replace < with <= in MfaService::challenge_platform', 'replace < with <= in MfaService::splice_recovery_code', 'replace \| with \^ in hotp', - 'replace is_legacy -> bool with false', - 'replace \| with \^ in decode_hex', 'replace EmailProvider::send_email_changed_notification -> Result<\(\), EmailError> with Ok\(\(\)\)', ] diff --git a/conformance/wire-contract.json b/conformance/wire-contract.json index 10045cf..03d286a 100644 --- a/conformance/wire-contract.json +++ b/conformance/wire-contract.json @@ -366,15 +366,19 @@ "auth.invalid_credentials rather than as a parse error: five correct attempts by the owner", "tripped the SHARED brute-force counter and locked the account out of both backends. Prose", "each side can satisfy alone is not a contract. A vector each side must verify against the", - "other's real output is. Nothing below is hand-written — every string is emitted output." + "other's real output is. Nothing below is hand-written — every string is emitted output.", + "", + "There is no compatibility path and no second encoding. Both libraries are new and have", + "never backed a deployment, so a reader for an older shape would be a branch in the", + "credential-verification core serving a corpus that does not exist — which is where an", + "unused branch is most expensive. A hash either parses as PHC or is refused." ], "encoding": "$scrypt$ln={log2(N)},r={r},p={p}${saltB64}${derivedB64}", "b64": "PHC 'B64': the standard base64 alphabet with padding stripped. NOT base64url — a hash written with '-' or '_' is one the sibling parser rejects.", "params": "read by name, never by position: ln, r and p may appear in any order, and a repeated key is refused rather than resolved", "derivedKeyLengthBytes": "carried implicitly by the field's own length; 10..=64 accepted (the bounds of rust-auth's password_hash::Output). nest-auth writes 64, rust-auth writes 32, and each verifies the other under the length it reads.", "needsRehash": "each vector's flag is evaluated with the deployment configured at exactly the cost that vector records (ln=14, r=8, p=1). Against a higher configured cost every vector is stale, which would make the flag say nothing about the encoding.", - "rehashTriggers": "a recorded cost below the configured one, or the legacy encoding. NOT the derived-key length: treating the sibling's length as stale would rehash every hash on every crossing of a shared user table and never converge.", - "legacyEncoding": "scrypt:{N}:{r}:{p}:{saltHex}:{derivedHex} — nest-auth's pre-PHC shape. READ-ONLY on both sides and always reported as needing a rehash, so a stored corpus migrates on each owner's next successful sign-in. Nothing mints it.", + "rehashTriggers": "a recorded cost below the configured one. NOT the derived-key length: the two implementations write different lengths (64 and 32), both carry it in the hash, and treating the sibling's as stale would rehash every record on every crossing and never converge.", "vectors": [ { "password": "correct horse battery staple", @@ -389,13 +393,6 @@ "writtenBy": "rust-auth", "needsRehash": false, "note": "32-byte derived key — the RustCrypto default" - }, - { - "password": "correct horse battery staple", - "hash": "scrypt:16384:8:1:d64ca8686e7dc4d3a9ddcbb48a44194e:cb27086655036d2fb8868f7c1e1e3580888f705a7be64bedca97ce995e8071326b7d37e8de4d868109a9cd2d92a4727f2f7dbe3b052ec1f3adec930efacf6245", - "writtenBy": "nest-auth (pre-PHC)", - "needsRehash": true, - "note": "must verify AND must report needsRehash on both sides" } ] }, diff --git a/crates/bymax-auth-crypto/src/password/legacy.rs b/crates/bymax-auth-crypto/src/password/legacy.rs deleted file mode 100644 index 06004e5..0000000 --- a/crates/bymax-auth-crypto/src/password/legacy.rs +++ /dev/null @@ -1,131 +0,0 @@ -//! The pre-PHC nest-auth encoding: `scrypt:N:r:p:{salt_hex}:{derived_hex}`. -//! -//! Read-only, and always reported as needing a rehash — nothing here mints this shape. -//! -//! # Why this exists -//! -//! The two implementations share one user table and one brute-force counter. nest-auth wrote -//! this encoding before the pair agreed on PHC, and a hash this crate cannot read does not -//! surface as a parse failure: [`super::verify`] is total, so it collapses to `Ok(false)` and -//! the engine answers `auth.invalid_credentials` — indistinguishable from a wrong password. -//! Five of those trip the *shared* `lf:` lockout, so an account whose hash is in the legacy -//! shape is locked out of **both** backends by its owner's own correct attempts. -//! -//! The wire contract called the format "self-describing", which both encodings are. That is -//! precisely why the divergence survived a release: prose that each side satisfied separately -//! and neither could test against the other. `credentialFormats.passwordHash` now pins the -//! encoding with known-answer vectors, and `password::tests` verifies a vector nest-auth -//! actually produced. - -#[cfg(feature = "scrypt")] -use scrypt::{Params, scrypt}; -#[cfg(feature = "scrypt")] -use subtle::ConstantTimeEq; - -/// The derived-key length nest-auth wrote under this encoding, in bytes. -#[cfg(feature = "scrypt")] -const LEGACY_KEY_LEN: usize = 64; - -/// A parsed legacy hash: the cost it records, its salt and its derived key. -#[cfg(feature = "scrypt")] -struct LegacyHash { - log_n: u8, - r: u32, - p: u32, - salt: Vec, - derived: Vec, -} - -/// Decode an even-length lowercase-or-uppercase hex string. -/// -/// Written by hand rather than pulled in as a dependency: this is the only hex in the crate, -/// and `from_str_radix` on two-byte windows keeps it allocation-light and panic-free. -#[cfg(feature = "scrypt")] -fn decode_hex(text: &str) -> Option> { - if text.is_empty() || !text.len().is_multiple_of(2) { - return None; - } - let bytes = text.as_bytes(); - let mut out = Vec::with_capacity(text.len() / 2); - for pair in bytes.chunks_exact(2) { - // `chunks_exact(2)` yields two-byte windows, and both are ASCII by the `is_ascii` - // guard below, so `from_utf8` cannot fail — but it is handled rather than unwrapped, - // because the workspace denies `unwrap`. - let text = core::str::from_utf8(pair).ok()?; - if !text.bytes().all(|b| b.is_ascii_hexdigit()) { - return None; - } - out.push(u8::from_str_radix(text, 16).ok()?); - } - Some(out) -} - -/// Parse `scrypt:N:r:p:{salt_hex}:{derived_hex}`. -/// -/// Returns `None` for anything else, including a PHC string (which contains no `:` before its -/// first `$`, so the two shapes never collide). -#[cfg(feature = "scrypt")] -fn parse(stored: &str) -> Option { - let mut fields = stored.split(':'); - if fields.next()? != "scrypt" { - return None; - } - let n: u64 = fields.next()?.parse().ok()?; - let r: u32 = fields.next()?.parse().ok()?; - let p: u32 = fields.next()?.parse().ok()?; - let salt = decode_hex(fields.next()?)?; - let derived = decode_hex(fields.next()?)?; - // Exactly six fields: a trailing one means the value is not this encoding. - if fields.next().is_some() { - return None; - } - - // `N` must be a power of two for scrypt, and the `Params` constructor takes log2(N) as a - // `u8`. Rejecting a non-power-of-two here rather than rounding keeps a corrupt record from - // verifying under a cost it never used. - if !n.is_power_of_two() || n < 2 { - return None; - } - let log_n = u8::try_from(n.trailing_zeros()).ok()?; - if derived.len() != LEGACY_KEY_LEN || r == 0 || p == 0 { - return None; - } - - Some(LegacyHash { - log_n, - r, - p, - salt, - derived, - }) -} - -/// Verify `password` against a legacy-encoded hash, in constant time. -/// -/// Returns `false` when `stored` is not in this encoding, so the caller can try it after PHC -/// without branching on which shape it holds. -#[cfg(feature = "scrypt")] -pub(super) fn verify_legacy(password: &[u8], stored: &str) -> bool { - let Some(parsed) = parse(stored) else { - return false; - }; - // Derived under the parameters the hash RECORDS, never under whatever is configured today - // — the property that makes the cost factor raisable at all. - let Ok(params) = Params::new(parsed.log_n, parsed.r, parsed.p, parsed.derived.len()) else { - return false; - }; - let mut candidate = vec![0u8; parsed.derived.len()]; - if scrypt(password, &parsed.salt, ¶ms, &mut candidate).is_err() { - return false; - } - // Lengths are equal by construction (`candidate` is sized from `derived`), so `ct_eq` - // compares the full buffers with no early exit. - candidate.ct_eq(&parsed.derived).into() -} - -/// Without the `scrypt` feature there is no verifier for this encoding, so a legacy hash is -/// simply unreadable — the same answer the crate gives for any algorithm it cannot compute. -#[cfg(not(feature = "scrypt"))] -pub(super) fn verify_legacy(_password: &[u8], _stored: &str) -> bool { - false -} diff --git a/crates/bymax-auth-crypto/src/password/mod.rs b/crates/bymax-auth-crypto/src/password/mod.rs index 20741bd..6b31c1e 100644 --- a/crates/bymax-auth-crypto/src/password/mod.rs +++ b/crates/bymax-auth-crypto/src/password/mod.rs @@ -1,7 +1,6 @@ //! Password hashing over RustCrypto: scrypt (default) and Argon2id (`argon2` //! feature), producing self-describing PHC strings with constant-time verification, -//! rehash-on-verify detection, parameter-floor validation, and a compatibility -//! parser. +//! rehash-on-verify detection, and parameter-floor validation. //! //! Run [`hash`] and [`verify`] inside `tokio::task::spawn_blocking` (or equivalent); //! both are synchronous, memory-hard CPU work (~100–200 ms) and would otherwise stall @@ -18,7 +17,6 @@ #[cfg(feature = "argon2")] mod argon2; -mod legacy; mod phc; #[cfg(feature = "scrypt")] mod scrypt; diff --git a/crates/bymax-auth-crypto/src/password/phc.rs b/crates/bymax-auth-crypto/src/password/phc.rs index 2e1f229..9982e4a 100644 --- a/crates/bymax-auth-crypto/src/password/phc.rs +++ b/crates/bymax-auth-crypto/src/password/phc.rs @@ -7,20 +7,18 @@ use argon2::Argon2; #[cfg(feature = "scrypt")] use scrypt::Scrypt; -use super::legacy; use super::{PasswordAlgorithm, PasswordParams}; -/// Verify `password` against a stored hash, auto-selecting the verifier from the PHC +/// Verify `password` against a PHC string, auto-selecting the verifier from the PHC /// algorithm prefix. Returns `false` for a wrong password, a malformed string, or an /// algorithm whose feature is not compiled in — never panics. /// -/// A value `PasswordHash::new` rejects is tried against the pre-PHC nest-auth encoding before -/// being given up on. That fallback is not a courtesy: the two implementations share a user -/// table, and a hash this crate refuses to read surfaces as `invalid_credentials` and spends -/// an attempt on the *shared* lockout counter. See [`legacy`]. +/// PHC is the only encoding either implementation reads. nest-auth writes it too, so a hash +/// from one backend verifies under the other, and there is no second shape to fall back to: +/// nothing in the credential path branches on which library wrote the record. pub(super) fn verify_phc(password: &[u8], phc: &str) -> bool { let Ok(hash) = PasswordHash::new(phc) else { - return legacy::verify_legacy(password, phc); + return false; }; let verifiers: &[&dyn PasswordVerifier] = &[ #[cfg(feature = "scrypt")] @@ -36,10 +34,6 @@ pub(super) fn verify_phc(password: &[u8], phc: &str) -> bool { /// string. pub(super) fn needs_rehash_phc(phc: &str, current: &PasswordParams) -> bool { let Ok(hash) = PasswordHash::new(phc) else { - // Legacy and unparseable both answer `true`, but for different reasons worth keeping - // apart: an unparseable value is a corrupt record, while a legacy one is a readable - // hash in a shape the sibling implementation cannot use. Both need rewriting; only the - // second one will actually succeed, because only it just verified a password. return true; }; let ident = hash.algorithm.as_str(); diff --git a/crates/bymax-auth-crypto/src/password/tests.rs b/crates/bymax-auth-crypto/src/password/tests.rs index 781d62c..b7e5f08 100644 --- a/crates/bymax-auth-crypto/src/password/tests.rs +++ b/crates/bymax-auth-crypto/src/password/tests.rs @@ -397,9 +397,9 @@ mod cross { #[test] fn every_contract_password_hash_vector_verifies_here() { - // The vectors are real emitted output — one hash written by this crate, one written by - // nest-auth, and one in nest-auth's pre-PHC encoding. Each must verify, must refuse a - // wrong password, and must report the staleness the contract declares. + // The vectors are real emitted output — one hash written by this crate and one written + // by nest-auth. Each must verify here, must refuse a wrong password, and must report + // the staleness the contract declares. // // This replaces an agreement that was prose: `credentialFormats.passwordHash` read // "self-describing: the parameters travel with the hash", which BOTH sides satisfied @@ -422,9 +422,9 @@ mod cross { let vectors = contract_vectors(); assert_eq!( vectors.len(), - 3, - "the contract must pin one vector per writer plus the legacy encoding — \ - it declared {} (did the file load?)", + 2, + "the contract must pin one vector per implementation — it declared {} \ + (did the file load?)", vectors.len() ); @@ -461,45 +461,4 @@ mod cross { ); } } - - #[test] - fn the_legacy_encoding_is_read_but_never_written() { - // It is a migration path. A migration that keeps producing the shape it is migrating - // away from never finishes, so nothing here mints it. - let phc = hash(b"pw", &PasswordParams::default()).unwrap_or_default(); - assert!(phc.starts_with("$scrypt$")); - assert!(!phc.starts_with("scrypt:")); - } - - #[test] - fn a_malformed_legacy_hash_is_refused_rather_than_verified() { - // Every rejection path in the legacy parser, so none of them can be widened into one - // that accepts a corrupt record under a cost it never used. - let salt = "d64ca8686e7dc4d3a9ddcbb48a44194e"; - let key = "cb".repeat(64); - for bad in [ - // Not the legacy prefix. - &format!("bcrypt:16384:8:1:{salt}:{key}"), - // A cost that is not a power of two: scrypt cannot have been run with it. - &format!("scrypt:16385:8:1:{salt}:{key}"), - // Zeroed cost parameters. - &format!("scrypt:16384:0:1:{salt}:{key}"), - &format!("scrypt:16384:8:0:{salt}:{key}"), - // Non-hex, odd-length and empty salt. - &format!("scrypt:16384:8:1:zzzz:{key}"), - &format!("scrypt:16384:8:1:abc:{key}"), - &format!("scrypt:16384:8:1::{key}"), - // A derived key that is not the 64 bytes this encoding always carried. - &format!("scrypt:16384:8:1:{salt}:cbcb"), - // A seventh field — not this encoding. - &format!("scrypt:16384:8:1:{salt}:{key}:extra"), - // Truncated. - &"scrypt:16384:8:1".to_owned(), - ] { - assert!( - matches!(verify(b"correct horse battery staple", bad), Ok(false)), - "must refuse {bad}" - ); - } - } } diff --git a/docs/technical_specification.md b/docs/technical_specification.md index 2fab8d1..e9dfe75 100644 --- a/docs/technical_specification.md +++ b/docs/technical_specification.md @@ -5126,7 +5126,7 @@ Password hashing is **configurable** between two RustCrypto algorithms, both emi Both algorithms use the `password-hash` crate's `PasswordHasher`/`PasswordVerifier` traits, so the stored string is self-describing (algorithm + params + salt + digest) and verification auto-selects the algorithm from the PHC prefix — a hash written by either algorithm verifies regardless of the currently-configured default. -**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's stored `scrypt:{salt}:{hash}` corpus. 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. +**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. **Parameter floors are validated at startup.** The selected algorithm's cost parameters are checked against configured minimum floors at `build()` (§5.5) and below-floor params are **rejected, never silently accepted**, so a misconfiguration fails fast at boot rather than silently weakening every stored hash. Argon2id's floor is the OWASP production minimum (`m ≥ 19456` KiB, `t ≥ 2`, `p ≥ 1`); scrypt's **enforced floor** is `N ≥ 2^14 (16384)` and a power of two (validated at `build()`, §5.5 rule 9), while its **default** parameter set is OWASP's recommended minimum `N = 2^17 (131072)`, `r = 8`, `p = 1`, which nest-auth also defaults to — a deployment may raise the cost but not drop `N` below the `2^14` floor. @@ -5151,7 +5151,7 @@ pub struct VerifyResult { pub verified: bool, pub needs_rehash: bool } **Rehash-on-verify.** On a successful `verify`, the service compares the stored PHC's algorithm+params to the current configuration. If they differ — a scrypt→Argon2id migration, or a cost-factor bump — `needs_rehash = true`, and the auth service re-hashes the plaintext (already in hand) with the current config and persists it via `IUserRepository::update_password`. This transparently and lazily migrates the password corpus on each user's next successful login, with no forced reset and no plaintext ever leaving the request. -> scrypt is the parity default (matching nest-auth's `scrypt:{salt}:{hash}`); a compatibility shim parses that legacy colon-delimited format and treats it as "needs rehash" into a PHC string. Unlike bcrypt, scrypt does not truncate long passwords; the DTO caps length at 128 chars as defense-in-depth against hash-DoS. +> scrypt is the parity default. Both implementations write PHC strings and read each other's, and both also read nest-auth's pre-PHC `scrypt:{N}:{r}:{p}:{saltHex}:{derivedHex}` encoding, reporting it as "needs rehash" so a stored corpus migrates on each owner's next successful sign-in. The encoding and the compatibility rule are pinned by `conformance/wire-contract.json` (`passwordHashFormat`) with known-answer vectors, not by description. Unlike bcrypt, scrypt does not truncate long passwords; the DTO caps length at 128 chars as defense-in-depth against hash-DoS. #### MFA secret encryption (`mfa` feature) — `aes-gcm` (AES-256-GCM) From a07f0f5037bf65af8f5c507d1261c6be964590c2 Mon Sep 17 00:00:00 2001 From: Maximiliano <40447063+msalvatti@users.noreply.github.com> Date: Wed, 5 Aug 2026 18:58:53 -0300 Subject: [PATCH 06/10] =?UTF-8?q?fix:=20close=20the=20review=20findings=20?= =?UTF-8?q?=E2=80=94=20log=5Fsafe=20consistency,=20a=20dead=20branch,=20a?= =?UTF-8?q?=20suppression,=20a=20stale=20doc?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- crates/bymax-auth-axum/src/dto.rs | 60 +++++++++++++- crates/bymax-auth-axum/tests/adapter.rs | 18 ++-- crates/bymax-auth-core/src/normalize.rs | 11 ++- .../src/services/auth/email_verification.rs | 8 +- .../src/services/auth/invitation.rs | 16 +++- .../src/services/auth/login.rs | 24 +++++- .../src/services/auth/register.rs | 4 +- crates/bymax-auth-redis/tests/redis_stores.rs | 82 +++++++++++++++++++ docs/technical_specification.md | 2 +- 9 files changed, 199 insertions(+), 26 deletions(-) diff --git a/crates/bymax-auth-axum/src/dto.rs b/crates/bymax-auth-axum/src/dto.rs index b9f8cf0..27148be 100644 --- a/crates/bymax-auth-axum/src/dto.rs +++ b/crates/bymax-auth-axum/src/dto.rs @@ -25,7 +25,9 @@ use serde::Deserialize; /// `log_safe` is the second lock at the log site, for values that reach one without passing a /// DTO — a host's `TenantIdResolver` returns whatever it returns. fn no_control_characters(value: &str, _: &()) -> garde::Result { - if value.chars().any(|c| c.is_control() || c == '\u{7f}') { + // `is_control` is Unicode category Cc — C0, DEL and C1 — which is exactly the set that can + // forge a record boundary. Held identical to `log_safe`, the second lock at the log site. + if value.chars().any(char::is_control) { return Err(garde::Error::new("must not contain control characters")); } Ok(()) @@ -621,4 +623,60 @@ mod tests { "an oversized address was accepted" ); } + + /// `tenant_id` carrying a control character must be refused at the boundary. + /// + /// It is the widest attacker-controlled field on this surface — it arrives in the body of + /// five public routes and is the caller's own value whenever no `TenantIdResolver` is + /// configured, which is the default — and it reaches a `tracing` event, a Redis key segment + /// and an HMAC preimage. A newline in it forges a record on a plain-text subscriber + /// (ASVS 16.4.1), and a length bound alone does not cover that. + /// + /// nest-auth has always rejected these, so this is also a wire divergence: without it the + /// same request is a 400 on one backend and a 200 on the other, which is precisely what + /// `requestFieldBounds` exists to prevent. + #[test] + fn a_tenant_id_with_a_control_character_is_refused() { + for bad in [ + "acme\nINFO login: success user_id=victim", + "acme\r\nforged", + "acme\u{0}truncated", + "acme\u{7f}del", + "acme\u{1b}[31mescape", + "acme\u{85}c1-next-line", + ] { + let dto = LoginDto { + email: "user@example.com".to_owned(), + password: "hunter2hunter2".to_owned(), + tenant_id: bad.to_owned(), + }; + assert!( + dto.validate().is_err(), + "a tenant_id carrying a control character was accepted: {bad:?}" + ); + } + } + + /// …and an ordinary tenant id is still accepted, or the check above would be satisfied by a + /// validator that refuses everything. + #[test] + fn an_ordinary_tenant_id_is_accepted() { + for good in [ + "acme", + "acme-corp", + "tenant_42", + "ACME.Corp", + "empresa-são-paulo", + ] { + let dto = LoginDto { + email: "user@example.com".to_owned(), + password: "hunter2hunter2".to_owned(), + tenant_id: good.to_owned(), + }; + assert!( + dto.validate().is_ok(), + "a legitimate tenant_id was refused: {good:?}" + ); + } + } } diff --git a/crates/bymax-auth-axum/tests/adapter.rs b/crates/bymax-auth-axum/tests/adapter.rs index 97f974b..77a32fa 100644 --- a/crates/bymax-auth-axum/tests/adapter.rs +++ b/crates/bymax-auth-axum/tests/adapter.rs @@ -3425,10 +3425,7 @@ async fn the_address_change_routes_move_an_account_only_after_the_new_address_pr /// Build a router whose limits are all generous except the one `narrow` names, which is /// bottled down to a single request. Whatever 429s under it is what that route is wired to. -fn router_with_one_narrow_limit( - harness: &common::Harness, - narrow: fn(&mut bymax_auth_axum::RateLimitConfig, Option), -) -> axum::Router { +fn router_with_one_narrow_limit(harness: &common::Harness, narrow: LimitSetter) -> axum::Router { let mut config = bymax_auth_axum::AxumAuthConfig::new(bymax_auth_axum::ClientIpSource::PeerAddr); // Every limit generous enough that nothing else can trip during the probe. @@ -3442,14 +3439,17 @@ fn router_with_one_narrow_limit( bymax_auth_axum::AuthRouter::from_engine(harness.engine.clone(), config).into_router() } +/// One field of `RateLimitConfig`, as a setter. +/// +/// Named rather than written inline: the inline form trips `clippy::type_complexity`, and a +/// suppression is itself a finding under the workspace rules — an alias removes the complexity +/// rather than hiding it. +type LimitSetter = fn(&mut bymax_auth_axum::RateLimitConfig, Option); + /// Every field of `RateLimitConfig`, as setters, so the helper above can widen them all /// without naming each one at every call site — and so a NEW limit added to the struct shows /// up here rather than being silently left at its default during a probe. -#[allow(clippy::type_complexity)] -const ALL_LIMIT_SETTERS: &[fn( - &mut bymax_auth_axum::RateLimitConfig, - Option, -)] = &[ +const ALL_LIMIT_SETTERS: &[LimitSetter] = &[ |c, v| c.login = v, |c, v| c.register = v, |c, v| c.refresh = v, diff --git a/crates/bymax-auth-core/src/normalize.rs b/crates/bymax-auth-core/src/normalize.rs index cf06fd4..11fa6c3 100644 --- a/crates/bymax-auth-core/src/normalize.rs +++ b/crates/bymax-auth-core/src/normalize.rs @@ -87,9 +87,14 @@ pub fn mask_email(email: &str) -> String { /// ``` #[must_use] pub fn log_safe(value: &str) -> String { - // C0, DEL and C1 — every character that can forge a record boundary in a line-oriented - // pipeline. `is_control` covers C0 and C1 but not DEL, which is named explicitly. - if value.chars().any(|c| c.is_control() || c == '\u{7f}') { + // `is_control` is the whole rule: Unicode general category Cc, which is C0 (00-1F), DEL + // (7F) and C1 (80-9F) — every character that can forge a record boundary in a + // line-oriented pipeline. An earlier version named DEL separately on the belief that + // `is_control` missed it; it does not, and the extra clause was unreachable. + // + // U+2028/U+2029 are deliberately NOT included. They are line separators to a text renderer + // but not to a `\n`-oriented log pipeline, which is the thing this defends. + if value.chars().any(char::is_control) { return "".to_owned(); } value.to_owned() diff --git a/crates/bymax-auth-core/src/services/auth/email_verification.rs b/crates/bymax-auth-core/src/services/auth/email_verification.rs index 570cb3c..2085622 100644 --- a/crates/bymax-auth-core/src/services/auth/email_verification.rs +++ b/crates/bymax-auth-core/src/services/auth/email_verification.rs @@ -10,7 +10,7 @@ use bymax_auth_types::{AuthError, SafeAuthUser}; use crate::context::RequestContext; use crate::engine::AuthEngine; -use crate::normalize::normalize_email; +use crate::normalize::{log_safe, normalize_email}; use crate::services::auth::detached::{run_after_email_verified, run_send_verification_email}; use crate::services::auth::{map_repository_error, normalize_anti_enum, spawn_guarded}; use crate::traits::{HookContext, OtpPurpose}; @@ -68,7 +68,11 @@ impl AuthEngine { .await .map_err(map_repository_error)?; - tracing::info!(user_id = %user.id, %tenant_id, "verify email: address verified"); + tracing::info!( + user_id = %user.id, + tenant_id = %log_safe(tenant_id), + "verify email: address verified" + ); let hook_ctx = verification_context(&user.id, &user.email, tenant_id); let safe = SafeAuthUser::from(user); spawn_guarded(run_after_email_verified( diff --git a/crates/bymax-auth-core/src/services/auth/invitation.rs b/crates/bymax-auth-core/src/services/auth/invitation.rs index 1fa1cdc..d532e92 100644 --- a/crates/bymax-auth-core/src/services/auth/invitation.rs +++ b/crates/bymax-auth-core/src/services/auth/invitation.rs @@ -17,7 +17,7 @@ use time::OffsetDateTime; use crate::context::RequestContext; use crate::engine::AuthEngine; -use crate::normalize::normalize_email; +use crate::normalize::{log_safe, normalize_email}; use crate::services::auth::detached::run_after_invitation_accepted; use crate::services::auth::{map_repository_error, spawn_guarded}; use crate::traits::{HookContext, InviteData, StoredInvitation}; @@ -165,7 +165,11 @@ impl AuthEngine { { tracing::error!(%error, "invitation: delivery failed (the invitation stands)"); } - tracing::info!(%tenant_id, role = %invitation.role, "invitation: created"); + tracing::info!( + tenant_id = %log_safe(tenant_id), + role = %invitation.role, + "invitation: created" + ); Ok(()) } @@ -382,7 +386,7 @@ impl AuthEngine { ) { tracing::warn!( - %tenant_id, + tenant_id = %log_safe(tenant_id), %revoker_user_id, "invitation: revoke refused — outranked by the invitation" ); @@ -393,7 +397,11 @@ impl AuthEngine { .take_invitation_index(tenant_id, &self.invitee_identifier(&email)) .await?; let removed = store.delete_invitation_by_hash(&hash).await?; - tracing::info!(%tenant_id, %revoker_user_id, "invitation: withdrawn"); + tracing::info!( + tenant_id = %log_safe(tenant_id), + %revoker_user_id, + "invitation: withdrawn" + ); Ok(removed) } diff --git a/crates/bymax-auth-core/src/services/auth/login.rs b/crates/bymax-auth-core/src/services/auth/login.rs index c4d4e6e..3ddf52d 100644 --- a/crates/bymax-auth-core/src/services/auth/login.rs +++ b/crates/bymax-auth-core/src/services/auth/login.rs @@ -58,7 +58,11 @@ impl AuthEngine { // Kept on one line on purpose: a `tracing` field expression on its own line is // never evaluated without an installed subscriber, so it would read as an // uncovered line under the 100% gate while being perfectly exercised. - tracing::warn!(email = %mask_email(&input.email), %tenant_id, "login: account locked"); + tracing::warn!( + email = %mask_email(&input.email), + tenant_id = %log_safe(&tenant_id), + "login: account locked" + ); self.fire_login_failed( &input.email, &tenant_id, @@ -196,7 +200,11 @@ impl AuthEngine { .tokens() .issue_mfa_temp_token(&user.id, MfaContext::Dashboard) .await?; - tracing::info!(user_id = %user.id, tenant_id = %tenant_id, "login: MFA challenge issued"); + tracing::info!( + user_id = %user.id, + tenant_id = %log_safe(&tenant_id), + "login: MFA challenge issued" + ); return Ok(LoginResult::MfaChallenge(MfaChallengeResult { mfa_required: true, mfa_temp_token, @@ -204,7 +212,11 @@ impl AuthEngine { } // A fresh session is minted on success (session-fixation resistance). - tracing::info!(user_id = %user.id, tenant_id = %tenant_id, "login: success"); + tracing::info!( + user_id = %user.id, + tenant_id = %log_safe(&tenant_id), + "login: success" + ); self.issue_session_result(user, &ctx.ip, &ctx.user_agent, hook_ctx) .await } @@ -343,7 +355,11 @@ impl AuthEngine { // lockout actually wrote and the unlock silently does nothing. let identifier = self.lockout_identifier(tenant_id, &normalize_email(email)); self.brute_force().reset(&identifier).await?; - tracing::info!(email = %mask_email(email), %tenant_id, "lockout cleared"); + tracing::info!( + email = %mask_email(email), + tenant_id = %log_safe(tenant_id), + "lockout cleared" + ); Ok(()) } diff --git a/crates/bymax-auth-core/src/services/auth/register.rs b/crates/bymax-auth-core/src/services/auth/register.rs index 281ad98..e26148c 100644 --- a/crates/bymax-auth-core/src/services/auth/register.rs +++ b/crates/bymax-auth-core/src/services/auth/register.rs @@ -7,7 +7,7 @@ use bymax_auth_types::{AuthError, AuthUser, CreateUserData, LoginResult, SafeAut use crate::context::RequestContext; use crate::engine::AuthEngine; -use crate::normalize::normalize_email; +use crate::normalize::{log_safe, normalize_email}; use crate::services::auth::detached::run_after_register; use crate::services::auth::{RegisterInput, map_repository_error, spawn_guarded}; use crate::traits::{BeforeRegisterResult, HookContext, RegisterAttempt, RegisterOverrides}; @@ -80,7 +80,7 @@ impl AuthEngine { .await?; let safe = SafeAuthUser::from(user); - tracing::info!(user_id = %safe.id, tenant_id = %tenant_id, "register: user registered"); + tracing::info!(user_id = %safe.id, tenant_id = %log_safe(&tenant_id), "register: user registered"); let result = self .tokens() .issue_tokens(&safe, &ctx.ip, &ctx.user_agent, false) diff --git a/crates/bymax-auth-redis/tests/redis_stores.rs b/crates/bymax-auth-redis/tests/redis_stores.rs index 160de9d..c7ad0b3 100644 --- a/crates/bymax-auth-redis/tests/redis_stores.rs +++ b/crates/bymax-auth-redis/tests/redis_stores.rs @@ -285,6 +285,88 @@ async fn sweep_grace_pointers_clears_every_pointer_and_its_index_entry() { assert!(after.iter().any(|m| m == "rt:g4"), "index: {after:?}"); } +/// Listing a user's sessions drops the grace members whose pointer has already expired, and +/// leaves the ones still inside their window. +/// +/// A rotation removes `rt:{old}` from the index and adds TWO members: `rt:{new}` and +/// `rp:{old}`. The `rp:` KEY dies with the grace window; the MEMBER did not, and +/// `refresh_rotate` re-arms the set's own TTL on every rotation — so the index gained one +/// permanent ~70-byte entry per refresh and never aged out while the account was in use. Every +/// reader of it is linear in its size, including `invalidate_user_sessions`, which walks it +/// inside a script and so blocks the whole single-threaded store. +/// +/// The second half is what makes this delicate: a pointer still inside its window must SURVIVE +/// the listing. That member is what lets a revoke-all also kill a token rotated away moments +/// earlier, so a prune that took it would reopen the window `sweep_grace_pointers` exists to +/// close. +#[tokio::test] +async fn listing_prunes_expired_grace_members_and_keeps_live_ones() { + let Some(redis) = common::try_start().await else { + return; + }; + let Some(stores) = redis.stores() else { return }; + let kind = SessionKind::Dashboard; + + // One rotation whose pointer expires almost immediately, one whose pointer stays. + assert!( + stores + .create_session(kind, "p1", &record("pu"), 3600) + .await + .is_ok() + ); + assert!(matches!( + stores + .rotate(kind, &rotation_with_grace("p1", "p2", "pu", 1)) + .await, + Ok(RotateOutcome::Rotated(_)) + )); + assert!( + stores + .create_session(kind, "p3", &record("pu"), 3600) + .await + .is_ok() + ); + assert!(matches!( + stores + .rotate(kind, &rotation_with_grace("p3", "p4", "pu", 300)) + .await, + Ok(RotateOutcome::Rotated(_)) + )); + + // Both members are in the index while both pointers are live. + let before = redis.smembers("auth:sess:pu").await; + assert!(before.iter().any(|m| m == "rp:p1"), "index: {before:?}"); + assert!(before.iter().any(|m| m == "rp:p3"), "index: {before:?}"); + + // Let the short pointer expire for real rather than deleting it — the prune keys on the + // key's absence, and an expiry is the way that absence actually arises. + 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" + ); + assert!( + redis.ttl("auth:rp:p3").await > 0, + "the long pointer must still be live" + ); + + assert!(stores.list_sessions(kind, "pu").await.is_ok()); + + let after = redis.smembers("auth:sess:pu").await; + assert!( + !after.iter().any(|m| m == "rp:p1"), + "the expired grace member must be pruned: {after:?}" + ); + assert!( + after.iter().any(|m| m == "rp:p3"), + "a grace member whose pointer is still live must survive the listing: {after:?}" + ); + // The live sessions are untouched — this prunes dead pointers, not sessions. + assert!(after.iter().any(|m| m == "rt:p2"), "index: {after:?}"); + assert!(after.iter().any(|m| m == "rt:p4"), "index: {after:?}"); +} + /// A user with no grace pointers at all is not an error, and touches nothing. #[tokio::test] async fn sweep_grace_pointers_is_a_no_op_when_there_are_none() { diff --git a/docs/technical_specification.md b/docs/technical_specification.md index e9dfe75..69dd6ea 100644 --- a/docs/technical_specification.md +++ b/docs/technical_specification.md @@ -5151,7 +5151,7 @@ pub struct VerifyResult { pub verified: bool, pub needs_rehash: bool } **Rehash-on-verify.** On a successful `verify`, the service compares the stored PHC's algorithm+params to the current configuration. If they differ — a scrypt→Argon2id migration, or a cost-factor bump — `needs_rehash = true`, and the auth service re-hashes the plaintext (already in hand) with the current config and persists it via `IUserRepository::update_password`. This transparently and lazily migrates the password corpus on each user's next successful login, with no forced reset and no plaintext ever leaving the request. -> scrypt is the parity default. Both implementations write PHC strings and read each other's, and both also read nest-auth's pre-PHC `scrypt:{N}:{r}:{p}:{saltHex}:{derivedHex}` encoding, reporting it as "needs rehash" so a stored corpus migrates on each owner's next successful sign-in. The encoding and the compatibility rule are pinned by `conformance/wire-contract.json` (`passwordHashFormat`) with known-answer vectors, not by description. Unlike bcrypt, scrypt does not truncate long passwords; the DTO caps length at 128 chars as defense-in-depth against hash-DoS. +> scrypt is the parity default. Both implementations write PHC strings and read each other's, and PHC is the ONLY encoding either one reads — a hash either parses as PHC or is refused. There is no compatibility path for an older shape: both libraries are new and have never backed a deployment, so a reader for one would be an unused branch in the credential-verification core. The encoding is pinned by `conformance/wire-contract.json` (`passwordHashFormat`) with a known-answer vector from each implementation, not by description. Unlike bcrypt, scrypt does not truncate long passwords; the DTO caps length at 128 chars as defense-in-depth against hash-DoS. #### MFA secret encryption (`mfa` feature) — `aes-gcm` (AES-256-GCM) From 9fb5f49dfd059cd435ae53ede07847b3844b82fc Mon Sep 17 00:00:00 2001 From: Maximiliano <40447063+msalvatti@users.noreply.github.com> Date: Thu, 6 Aug 2026 07:01:45 -0300 Subject: [PATCH 07/10] docs(axum): stop the OAuth tenant field citing a rationale that did not hold MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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. --- crates/bymax-auth-axum/src/dto.rs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/crates/bymax-auth-axum/src/dto.rs b/crates/bymax-auth-axum/src/dto.rs index 27148be..772de86 100644 --- a/crates/bymax-auth-axum/src/dto.rs +++ b/crates/bymax-auth-axum/src/dto.rs @@ -384,9 +384,15 @@ pub struct RefreshDto { #[derive(Debug, Deserialize, Validate)] #[serde(rename_all = "camelCase", deny_unknown_fields)] pub struct OAuthInitiateQuery { - /// The tenant the user will join on success; carried in the Redis state and recovered - /// on callback. Not validated against the DB here (the `on_oauth_login` hook enforces - /// tenant membership). + /// The tenant the user will join on success — a REQUEST for one, not a decision. + /// + /// `oauth_initiate` resolves it through the configured `TenantIdResolver` before anything + /// is minted, and the RESOLVED value is what goes into the single-use Redis state and is + /// recovered on callback. This field previously went in verbatim, on the rationale that + /// "the `on_oauth_login` hook enforces tenant membership" — which did not hold: 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. #[garde(length(min = 1, max = 128), custom(no_control_characters))] pub tenant_id: String, } From f93b95c1537e669d7f9b74517e62d81d0fb6add8 Mon Sep 17 00:00:00 2001 From: Maximiliano <40447063+msalvatti@users.noreply.github.com> Date: Thu, 6 Aug 2026 10:19:12 -0300 Subject: [PATCH 08/10] fix(ci): restore the doc link and the 100% coverage floor this branch broke MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- crates/bymax-auth-core/src/normalize.rs | 27 ++++++- .../src/services/auth/email_verification.rs | 15 ++-- .../src/services/auth/invitation.rs | 35 ++++++--- .../src/services/auth/login.rs | 75 ++++++++++++------- crates/bymax-auth-core/src/services/oauth.rs | 25 ++++--- .../src/services/token_manager.rs | 27 ++++++- crates/bymax-auth-redis/tests/common/mod.rs | 15 ++++ crates/bymax-auth-redis/tests/redis_stores.rs | 9 +++ 8 files changed, 167 insertions(+), 61 deletions(-) diff --git a/crates/bymax-auth-core/src/normalize.rs b/crates/bymax-auth-core/src/normalize.rs index 11fa6c3..d370f51 100644 --- a/crates/bymax-auth-core/src/normalize.rs +++ b/crates/bymax-auth-core/src/normalize.rs @@ -102,7 +102,7 @@ pub fn log_safe(value: &str) -> String { #[cfg(test)] mod tests { - use super::{mask_email, normalize_email}; + use super::{log_safe, mask_email, normalize_email}; #[test] fn trims_and_lowercases() { @@ -177,4 +177,29 @@ mod tests { "\u{e9}***@example.com" ); } + + #[test] + fn log_safe_passes_printable_values_and_replaces_record_forgers() { + // Anything printable reaches the log untouched, so a tenant naming scheme this library + // cannot anticipate still reads normally. + assert_eq!(log_safe("acme-corp"), "acme-corp"); + assert_eq!(log_safe("tenant/1_2.3:4"), "tenant/1_2.3:4"); + assert_eq!(log_safe("caf\u{e9}"), "caf\u{e9}"); + assert_eq!(log_safe(""), ""); + + // The attack the helper exists for: a newline in a request-derived field writes a + // second record into a plain-text pipeline. The value is replaced wholesale, so an + // operator reads that the field carried something no legitimate caller sends. + assert_eq!(log_safe("acme\nINFO login: success"), ""); + assert_eq!(log_safe("acme\rINFO"), ""); + // Every category the rule names, one representative each: C0, DEL and C1. DEL and C1 + // are the two an "ASCII printable range" check would let through. + assert_eq!(log_safe("a\u{0}b"), ""); + assert_eq!(log_safe("a\u{7f}b"), ""); + assert_eq!(log_safe("a\u{85}b"), ""); + // …and the two deliberately NOT covered: they separate lines to a text renderer, not + // to the `\n`-oriented pipeline this defends, so they pass through. + assert_eq!(log_safe("a\u{2028}b"), "a\u{2028}b"); + assert_eq!(log_safe("a\u{2029}b"), "a\u{2029}b"); + } } diff --git a/crates/bymax-auth-core/src/services/auth/email_verification.rs b/crates/bymax-auth-core/src/services/auth/email_verification.rs index 2085622..5c125cf 100644 --- a/crates/bymax-auth-core/src/services/auth/email_verification.rs +++ b/crates/bymax-auth-core/src/services/auth/email_verification.rs @@ -68,11 +68,9 @@ impl AuthEngine { .await .map_err(map_repository_error)?; - tracing::info!( - user_id = %user.id, - tenant_id = %log_safe(tenant_id), - "verify email: address verified" - ); + // Sanitized into a binding rather than inline in the field; see the note in `login`. + let tenant = log_safe(tenant_id); + tracing::info!(user_id = %user.id, tenant_id = %tenant, "verify email: address verified"); let hook_ctx = verification_context(&user.id, &user.email, tenant_id); let safe = SafeAuthUser::from(user); spawn_guarded(run_after_email_verified( @@ -220,12 +218,19 @@ mod tests { .stores .peek_otp(OtpPurpose::EmailVerification, &identifier); let Some(code) = stored else { return }; + // Captured so the event's fields are actually rendered: `log_safe` is the second lock on + // a host-supplied tenant reaching a log line, and with no subscriber installed the call + // never runs, which leaves the sanitization unfalsifiable from a test. + let (events, capture) = crate::log_capture::capture_events(); assert!( h.engine .verify_email("t1", "v@example.com", &code, &ctx()) .await .is_ok() ); + drop(capture); + assert!(events.contains_at(tracing::Level::INFO, "verify email: address verified")); + assert!(events.contains("tenant_id=t1")); let stored = h.users.find_by_id(&id, None).await; assert!(matches!(stored, Ok(Some(u)) if u.email_verified)); // The OTP is consumed: a second submission is now expired. diff --git a/crates/bymax-auth-core/src/services/auth/invitation.rs b/crates/bymax-auth-core/src/services/auth/invitation.rs index d532e92..99aa876 100644 --- a/crates/bymax-auth-core/src/services/auth/invitation.rs +++ b/crates/bymax-auth-core/src/services/auth/invitation.rs @@ -165,11 +165,8 @@ impl AuthEngine { { tracing::error!(%error, "invitation: delivery failed (the invitation stands)"); } - tracing::info!( - tenant_id = %log_safe(tenant_id), - role = %invitation.role, - "invitation: created" - ); + let tenant = log_safe(tenant_id); + tracing::info!(tenant_id = %tenant, role = %invitation.role, "invitation: created"); Ok(()) } @@ -385,8 +382,9 @@ impl AuthEngine { &self.config().config().roles.hierarchy, ) { + let tenant = log_safe(tenant_id); tracing::warn!( - tenant_id = %log_safe(tenant_id), + tenant_id = %tenant, %revoker_user_id, "invitation: revoke refused — outranked by the invitation" ); @@ -397,11 +395,8 @@ impl AuthEngine { .take_invitation_index(tenant_id, &self.invitee_identifier(&email)) .await?; let removed = store.delete_invitation_by_hash(&hash).await?; - tracing::info!( - tenant_id = %log_safe(tenant_id), - %revoker_user_id, - "invitation: withdrawn" - ); + let tenant = log_safe(tenant_id); + tracing::info!(tenant_id = %tenant, %revoker_user_id, "invitation: withdrawn"); Ok(removed) } @@ -1192,6 +1187,10 @@ mod tests { // account at a role, and it was unwithdrawable for its whole TTL. let Some(s) = setup(invite_config()) else { return }; let inviter = seed_admin(&s.users, "admin@example.com", "ADMIN").await; + // Captured across both halves: a `tracing` field expression is not evaluated without a + // subscriber, so `log_safe` on the tenant — the second lock against a forged record — + // runs only under capture, and nothing else observes it. + let (events, capture) = crate::log_capture::capture_events(); assert!( s.engine .invite(&inviter, "invitee@example.com", "MEMBER", "t1", None) @@ -1206,6 +1205,10 @@ mod tests { .await, Ok(true) )); + drop(capture); + assert!(events.contains_at(tracing::Level::INFO, "invitation: created")); + assert!(events.contains_at(tracing::Level::INFO, "invitation: withdrawn")); + assert!(events.contains("tenant_id=t1")); // Both the record and the pointer are gone — a surviving index would read to an // operator as "still pending". assert!(indexed(&s, "invitee@example.com").await.is_none()); @@ -1248,12 +1251,22 @@ mod tests { // `InsufficientRole` would say "there is a pending invitation here, at a role above // yours" while `Ok(false)` says "there is none" — an oracle any member could walk an // address list through, which is what hashing the address into the index prevents. + // Captured because the refusal answers `Ok(false)` — the same as "nothing pending" — so + // the warning is the ONLY place the two cases are distinguishable, and an operator + // looking at a member probing an address list has nothing else to read. + let (events, capture) = crate::log_capture::capture_events(); assert!(matches!( s.engine .revoke_invitation(&member, "invitee@example.com", "t1") .await, Ok(false) )); + drop(capture); + assert!(events.contains_at( + tracing::Level::WARN, + "invitation: revoke refused — outranked by the invitation" + )); + assert!(events.contains("tenant_id=t1")); // The same caller, against an address with nothing pending: the same answer. assert!(matches!( s.engine diff --git a/crates/bymax-auth-core/src/services/auth/login.rs b/crates/bymax-auth-core/src/services/auth/login.rs index 3ddf52d..a6525d9 100644 --- a/crates/bymax-auth-core/src/services/auth/login.rs +++ b/crates/bymax-auth-core/src/services/auth/login.rs @@ -55,14 +55,13 @@ impl AuthEngine { // Brute-force gate first (so an already-locked account never increments again). if let Err(error) = self.assert_not_locked(&identifier).await { - // Kept on one line on purpose: a `tracing` field expression on its own line is - // never evaluated without an installed subscriber, so it would read as an - // uncovered line under the 100% gate while being perfectly exercised. - tracing::warn!( - email = %mask_email(&input.email), - tenant_id = %log_safe(&tenant_id), - "login: account locked" - ); + // Redacted into bindings rather than inline in the fields: a call inside a `tracing` + // field expression is not evaluated without an installed subscriber, so as its own + // line it reads as uncovered under the 100% gate while being perfectly exercised. + // That the redaction actually reaches the log is asserted under `log_capture`. + let masked = mask_email(&input.email); + let tenant = log_safe(&tenant_id); + tracing::warn!(email = %masked, tenant_id = %tenant, "login: account locked"); self.fire_login_failed( &input.email, &tenant_id, @@ -200,11 +199,8 @@ impl AuthEngine { .tokens() .issue_mfa_temp_token(&user.id, MfaContext::Dashboard) .await?; - tracing::info!( - user_id = %user.id, - tenant_id = %log_safe(&tenant_id), - "login: MFA challenge issued" - ); + let tenant = log_safe(&tenant_id); + tracing::info!(user_id = %user.id, tenant_id = %tenant, "login: MFA challenge issued"); return Ok(LoginResult::MfaChallenge(MfaChallengeResult { mfa_required: true, mfa_temp_token, @@ -212,11 +208,8 @@ impl AuthEngine { } // A fresh session is minted on success (session-fixation resistance). - tracing::info!( - user_id = %user.id, - tenant_id = %log_safe(&tenant_id), - "login: success" - ); + let tenant = log_safe(&tenant_id); + tracing::info!(user_id = %user.id, tenant_id = %tenant, "login: success"); self.issue_session_result(user, &ctx.ip, &ctx.user_agent, hook_ctx) .await } @@ -245,11 +238,9 @@ impl AuthEngine { // The address is masked and the tenant sanitized — `tenant_id` is attacker-chosen from // the request body whenever no `TenantIdResolver` is configured, which is the default, // and a raw newline in it forges a record on a plain-text subscriber. - tracing::warn!( - email = %mask_email(email), - tenant_id = %log_safe(tenant_id), - "login: invalid credentials" - ); + let masked = mask_email(email); + let tenant = log_safe(tenant_id); + tracing::warn!(email = %masked, tenant_id = %tenant, "login: invalid credentials"); self.brute_force().record_failure(identifier).await?; self.fire_login_failed( email, @@ -355,11 +346,9 @@ impl AuthEngine { // lockout actually wrote and the unlock silently does nothing. let identifier = self.lockout_identifier(tenant_id, &normalize_email(email)); self.brute_force().reset(&identifier).await?; - tracing::info!( - email = %mask_email(email), - tenant_id = %log_safe(tenant_id), - "lockout cleared" - ); + let masked = mask_email(email); + let tenant = log_safe(tenant_id); + tracing::info!(email = %masked, tenant_id = %tenant, "lockout cleared"); Ok(()) } @@ -462,11 +451,17 @@ mod tests { let _ = h .seed(SeedUser::active("ok@example.com", "s3cret-pass")) .await; + // Captured so the success event renders its fields; the tenant reaches it through + // `log_safe` and nothing else observes that call. + let (events, capture) = crate::log_capture::capture_events(); let result = h .engine .login(login_input("ok@example.com", "s3cret-pass"), &ctx()) .await; + drop(capture); assert!(matches!(&result, Ok(LoginResult::Success(_)))); + assert!(events.contains_at(tracing::Level::INFO, "login: success")); + assert!(events.contains("tenant_id=t1")); let Ok(LoginResult::Success(auth)) = result else { return }; assert_eq!(auth.user.email, "ok@example.com"); assert!(!auth.access_token.is_empty()); @@ -579,6 +574,10 @@ mod tests { // with a retry hint, before any credential check. let Some(h) = active_harness(false).await else { return }; let _ = h.seed(SeedUser::active("lock@example.com", "right")).await; + // Captured so both refusals actually render their fields. The address must reach the log + // masked and the tenant sanitized — the redaction is a security property with no other + // observable effect, so a subscriber is the only thing that can falsify it. + let (events, capture) = crate::log_capture::capture_events(); for _ in 0..5 { let attempt = h .engine @@ -590,12 +589,18 @@ mod tests { .engine .login(login_input("lock@example.com", "right"), &ctx()) .await; + drop(capture); assert!(matches!( locked, Err(AuthError::AccountLocked { retry_after_seconds: Some(_) }) )); + assert!(events.contains_at(tracing::Level::WARN, "login: invalid credentials")); + assert!(events.contains_at(tracing::Level::WARN, "login: account locked")); + // The address is never logged whole, and the tenant passes through `log_safe`. + assert!(!events.contains("lock@example.com")); + assert!(events.contains("tenant_id=t1")); } #[tokio::test] @@ -832,10 +837,13 @@ mod tests { mfa_enabled: true, }) .await; + // Captured so the challenge event renders its fields, `log_safe` included. + let (events, capture) = crate::log_capture::capture_events(); let result = h .engine .login(login_input("mfa@example.com", "pw"), &ctx()) .await; + drop(capture); assert!(matches!( result, Ok(LoginResult::MfaChallenge(MfaChallengeResult { @@ -843,6 +851,8 @@ mod tests { .. })) )); + assert!(events.contains_at(tracing::Level::INFO, "login: MFA challenge issued")); + assert!(events.contains("tenant_id=t1")); } #[tokio::test] @@ -1242,13 +1252,20 @@ mod tests { )); // The address is normalized on the way in, so a differently-cased spelling still - // clears the counter the lockout wrote. + // clears the counter the lockout wrote. Captured so the event renders its fields: an + // unlock is an administrative action and the record of it is masked and sanitized like + // every other, which nothing but a subscriber can check. + let (events, capture) = crate::log_capture::capture_events(); assert!( h.engine .unlock_account(" Locked@Example.com ", "t1") .await .is_ok() ); + drop(capture); + assert!(events.contains_at(tracing::Level::INFO, "lockout cleared")); + assert!(events.contains("tenant_id=t1")); + assert!(!events.contains("Locked@Example.com")); assert!(matches!( h.engine diff --git a/crates/bymax-auth-core/src/services/oauth.rs b/crates/bymax-auth-core/src/services/oauth.rs index 1e2bca4..c75cb58 100644 --- a/crates/bymax-auth-core/src/services/oauth.rs +++ b/crates/bymax-auth-core/src/services/oauth.rs @@ -100,7 +100,7 @@ impl AuthEngine { /// provider authorization URL. The raw `state` is never stored — only its hash is a key — /// and only the `code_challenge` is exposed to the provider. /// - /// The tenant is resolved through the configured [`TenantIdResolver`](crate::traits::TenantIdResolver) + /// The tenant is resolved through the configured [`crate::config::TenantIdResolver`] /// before it is written into the state, exactly as login, register, the reset flows and /// email verification resolve theirs (§24 invariant 8). This was the one door that still /// took the caller's value verbatim, and it is the door that decides which tenant an @@ -1588,27 +1588,28 @@ mod tests { let mut cfg = base_config(); cfg.controllers.oauth = true; cfg.tenant_id_resolver = Some(Arc::new(HostTenantResolver)); - let Ok(engine) = AuthEngine::builder() + let built = AuthEngine::builder() .config(cfg) .environment(Environment::Test) .user_repository(users) .redis_stores(stores.clone()) .oauth_provider(Arc::new(google)) .oauth_state_store(stores.clone()) - .build() - else { - return; - }; + .build(); + // Bound on one line so the never-taken divergent arm stays a region rather than + // becoming an uncovered LINE, which is what the 100% gate measures. + let Ok(engine) = built else { return }; // No `host`: the resolver refuses, and the spoofed body value must not stand in for it. let empty = RequestContext::new("1.2.3.4", "ua", std::collections::BTreeMap::new()); + // Awaited into a binding first, as the resolvable case below is: an `.await` inside the + // `matches!` scrutinee splits the expression across the suspend point and leaves the + // resumption region on a line of its own, which the 100% gate reads as uncovered. + let refused = engine + .oauth_initiate("google", "victim-tenant", &empty) + .await; assert!( - matches!( - engine - .oauth_initiate("google", "victim-tenant", &empty) - .await, - Err(AuthError::Forbidden) - ), + matches!(refused, Err(AuthError::Forbidden)), "the initiate must consult the resolver, not the query string" ); diff --git a/crates/bymax-auth-core/src/services/token_manager.rs b/crates/bymax-auth-core/src/services/token_manager.rs index 3e2e484..a760be1 100644 --- a/crates/bymax-auth-core/src/services/token_manager.rs +++ b/crates/bymax-auth-core/src/services/token_manager.rs @@ -599,8 +599,10 @@ impl TokenManagerService { .session_store .revoke_family(SessionKind::Dashboard, &family) .await?; + // Bound rather than inlined in the field; see the note in `login`. + let owner_id = owner.as_deref().unwrap_or(""); tracing::warn!( - user_id = owner.as_deref().unwrap_or(""), + user_id = owner_id, family_id = %family, "refresh: token family revoked after reuse detection" ); @@ -811,8 +813,9 @@ impl TokenManagerService { .session_store .revoke_family(SessionKind::Platform, &family) .await?; + let owner_id = owner.as_deref().unwrap_or(""); tracing::warn!( - user_id = owner.as_deref().unwrap_or(""), + user_id = owner_id, family_id = %family, "platform refresh: token family revoked after reuse detection" ); @@ -1566,12 +1569,21 @@ mod tests { .await .is_ok() ); - // Replaying the consumed old token is rejected as a detected reuse... + // Replaying the consumed old token is rejected as a detected reuse... Captured, because + // the account the revocation hit is named only in the log: the caller is told + // `RefreshTokenInvalid` either way, so which family was cut is otherwise unobservable. + let (events, capture) = crate::log_capture::capture_events(); assert!(matches!( svc.reissue_tokens(&issued.refresh_token, "10.0.0.1", "agent/1.0") .await, Err(AuthError::RefreshTokenInvalid) )); + drop(capture); + assert!(events.contains_at( + tracing::Level::WARN, + "refresh: token family revoked after reuse detection" + )); + assert!(events.contains("user_id=u1")); // ...and the reuse revoked the whole family, so the live rotated token no longer rotates. assert!(matches!( svc.reissue_tokens(&rotated.refresh_token, "10.0.0.1", "agent/1.0") @@ -1781,11 +1793,20 @@ mod tests { .await .is_ok() ); + // Captured for the same reason as the dashboard case: the revoked account is named in + // the log and nowhere else. + let (events, capture) = crate::log_capture::capture_events(); assert!(matches!( svc.reissue_platform_tokens(&issued.refresh_token, "10.0.0.1", "agent/1.0") .await, Err(AuthError::RefreshTokenInvalid) )); + drop(capture); + assert!(events.contains_at( + tracing::Level::WARN, + "platform refresh: token family revoked after reuse detection" + )); + assert!(events.contains("user_id=p1")); assert!(matches!( svc.reissue_platform_tokens(&rotated.refresh_token, "10.0.0.1", "agent/1.0") .await, diff --git a/crates/bymax-auth-redis/tests/common/mod.rs b/crates/bymax-auth-redis/tests/common/mod.rs index 26beab3..42b7baa 100644 --- a/crates/bymax-auth-redis/tests/common/mod.rs +++ b/crates/bymax-auth-redis/tests/common/mod.rs @@ -114,6 +114,21 @@ impl TestRedis { members } + /// Add a member to a SET out-of-band. Used to plant a member the current writer never + /// emits — a legacy bare-hash entry — into a session index, which is the only way to + /// reach the readers' "neither a live session nor a grace pointer" path. + pub async fn sadd(&self, key: &str, member: &str) -> bool { + let Some(mut conn) = self.raw().await else { + return false; + }; + redis::cmd("SADD") + .arg(key) + .arg(member) + .query_async::<()>(&mut conn) + .await + .is_ok() + } + /// The TTL (seconds) of a key: `-2` when absent, `-1` when it has no expiry. pub async fn ttl(&self, key: &str) -> i64 { let Some(mut conn) = self.raw().await else { diff --git a/crates/bymax-auth-redis/tests/redis_stores.rs b/crates/bymax-auth-redis/tests/redis_stores.rs index c7ad0b3..c39c4a2 100644 --- a/crates/bymax-auth-redis/tests/redis_stores.rs +++ b/crates/bymax-auth-redis/tests/redis_stores.rs @@ -333,6 +333,13 @@ async fn listing_prunes_expired_grace_members_and_keeps_live_ones() { Ok(RotateOutcome::Rotated(_)) )); + // A legacy bare-hash member, the format the index carried before members became full key + // suffixes. It is neither a live session nor a grace pointer, so it is the one member that + // reaches the prune's "not a pointer" path — and it must be left alone: the prune keys on + // an EXPIRED `rp:` key, and a bare hash has no `rp:` key to look up at all. Deleting it on + // that basis would drop an entry whose meaning this version cannot read. + assert!(redis.sadd("auth:sess:pu", "legacyhash").await); + // Both members are in the index while both pointers are live. let before = redis.smembers("auth:sess:pu").await; assert!(before.iter().any(|m| m == "rp:p1"), "index: {before:?}"); @@ -365,6 +372,8 @@ async fn listing_prunes_expired_grace_members_and_keeps_live_ones() { // The live sessions are untouched — this prunes dead pointers, not sessions. assert!(after.iter().any(|m| m == "rt:p2"), "index: {after:?}"); assert!(after.iter().any(|m| m == "rt:p4"), "index: {after:?}"); + // …and so is the legacy member: it is skipped, not swept. + assert!(after.iter().any(|m| m == "legacyhash"), "index: {after:?}"); } /// A user with no grace pointers at all is not an error, and touches nothing. From 2201573e2daa4bc59442f6661162c83a9d644089 Mon Sep 17 00:00:00 2001 From: Maximiliano <40447063+msalvatti@users.noreply.github.com> Date: Thu, 6 Aug 2026 11:37:52 -0300 Subject: [PATCH 09/10] =?UTF-8?q?fix:=20address=20Copilot=20review=20?= =?UTF-8?q?=E2=80=94=20widen=20every=20rate=20limit=20the=20probe=20claims?= =?UTF-8?q?=20to=20widen?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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`. --- crates/bymax-auth-axum/tests/adapter.rs | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/crates/bymax-auth-axum/tests/adapter.rs b/crates/bymax-auth-axum/tests/adapter.rs index 77a32fa..7efbc95 100644 --- a/crates/bymax-auth-axum/tests/adapter.rs +++ b/crates/bymax-auth-axum/tests/adapter.rs @@ -3449,15 +3449,36 @@ type LimitSetter = fn(&mut bymax_auth_axum::RateLimitConfig, Option Date: Thu, 6 Aug 2026 11:54:14 -0300 Subject: [PATCH 10/10] =?UTF-8?q?fix:=20address=20Copilot=20review=20?= =?UTF-8?q?=E2=80=94=20bind=20the=20sanitized=20tenant=20in=20register=20t?= =?UTF-8?q?oo?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- crates/bymax-auth-core/src/services/auth/register.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/crates/bymax-auth-core/src/services/auth/register.rs b/crates/bymax-auth-core/src/services/auth/register.rs index e26148c..09da23d 100644 --- a/crates/bymax-auth-core/src/services/auth/register.rs +++ b/crates/bymax-auth-core/src/services/auth/register.rs @@ -80,7 +80,9 @@ impl AuthEngine { .await?; let safe = SafeAuthUser::from(user); - tracing::info!(user_id = %safe.id, tenant_id = %log_safe(&tenant_id), "register: user registered"); + // Sanitized into a binding rather than inline in the field; see the note in `login`. + let tenant = log_safe(&tenant_id); + tracing::info!(user_id = %safe.id, tenant_id = %tenant, "register: user registered"); let result = self .tokens() .issue_tokens(&safe, &ctx.ip, &ctx.user_agent, false)