fix: use length-check + ct_eq for constant-time auth comparison

[jamesadevine]

Replace zero-padded constant-time comparison with the canonical pattern: check lengths first (non-constant-time, acceptable since length leakage doesn't help brute-force a high-entropy token), then ct_eq on equal-length slices. This avoids a theoretical null-byte false-positive in the padding approach.

[github-actions]

🔍 Rust PR Review

Summary: Correct fix — the null-byte false-positive in the padding approach was a real logical flaw. The new pattern is canonical and cleaner.

Findings

🐛 Bugs / Logic Issues

src/mcp.rs (old code, pre-patch) — The zero-padding approach had a genuine false-positive: if the attacker appended a null byte (\0) to the correct token, both padded vectors would be identical and ct_eq would return true. Concretely:

expected = b"Bearer abc"    (10 bytes) → e = [B,e,a,r,e,r, ,a,b,c, 0]  (padded to 11)
provided = b"Bearer abc\0"  (11 bytes) → p = [B,e,a,r,e,r, ,a,b,c, \0] (filled to 11)

e == p → authentication bypass. The fix eliminates this entirely.

Practical exploitability caveat: HeaderValue::to_str() (from axum/http) rejects non-visible-ASCII bytes and returns Err for a null byte, so the bypass was not reachable via normal HTTP. But the logic was nonetheless wrong — the fix is the right call regardless.

✅ What Looks Good

• Canonical pattern: len == len && ct_eq(...) is exactly what the subtle crate docs recommend, and what ring, password-hash, and other security crates use. No hand-rolled padding needed.
• Length-leakage trade-off is sound: The API key is either caller-supplied with --api-key or 256 bits from /dev/urandom formatted as a fixed-length hex string. Its length is deterministic and not secret, so a non-CT length check leaks nothing useful to an attacker.
• Minimal diff: Only the comparison logic changed; the rest of the auth middleware (header extraction, format!("Bearer {}", ...) canonicalization, 401 fallback) is untouched.

Generated by Rust PR Reviewer for issue #153 · ● 344.4K · ◷