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

.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\(\(\)\)',
 ]

conformance/wire-contract.json

@@ -350,12 +350,53 @@
       "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.",
+      "",
+      "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. 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",
+        "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"
+      }
+    ]
+  },
+
   "rateLimits": {
     "$comment": [
       "The per-IP limit each auth route is served under, as `requests/windowSeconds`. Both",

crates/bymax-auth-axum/src/dto.rs

@@ -9,6 +9,30 @@
 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 {
+    // `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(())
+}
+
 /// `POST /auth/register` body.
 #[derive(Debug, Deserialize, Validate)]
 #[serde(rename_all = "camelCase", deny_unknown_fields)]
@@ -23,7 +47,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 +68,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 +80,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 +143,7 @@ pub struct ResetPasswordDto {
     #[garde(inner(length(min = 64, max = 64)))]
     pub verified_token: Option<String>,
     /// 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 +158,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 +170,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 +190,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 +202,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,
 }
 
@@ -360,10 +384,16 @@ 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).
-    #[garde(length(min = 1, max = 128))]
+    /// 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,
 }
 
@@ -599,4 +629,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:?}"
+            );
+        }
+    }
 }

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),
             ),
     )
 }

crates/bymax-auth-axum/src/routes/oauth.rs

@@ -70,11 +70,15 @@ async fn initiate(
     State(state): State<AuthState>,
     cookies: Cookies,
     Path(provider): Path<String>,
+    RequestMeta(ctx): RequestMeta,
     ValidatedQuery(query): ValidatedQuery<OAuthInitiateQuery>,
 ) -> 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) => {

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.

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),
         )
 }