fix(core): compare RPC bearer tokens in constant time#2572
Conversation
📝 WalkthroughWalkthroughThe pull request hardens bearer-token verification in ChangesBearer Token Timing-Attack Hardening
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/core/auth.rs (1)
238-260: ⚡ Quick winReuse the existing constant-time comparator instead of carrying a second copy.
src/openhuman/security/pairing.rs:255-276already implements the same max-length constant-time compare contract. Keeping two copies of a security-sensitive primitive makes future fixes easy to land in one path and miss in the other. Please routebearer_matchesthrough the shared helper or extract one common utility for both call sites.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/auth.rs` around lines 238 - 260, The bearer_matches implementation duplicates a constant-time comparator; replace its internal constant_time_eq with the single shared comparator used in the pairing/security module (i.e., call the existing constant-time compare function from that module instead of defining a new constant_time_eq), or extract a common utility function and have both bearer_matches and the pairing code call that shared helper; ensure you preserve the non-empty check in bearer_matches and import or expose the shared comparator symbol so no duplicate logic remains.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/core/auth.rs`:
- Around line 238-260: The bearer_matches implementation duplicates a
constant-time comparator; replace its internal constant_time_eq with the single
shared comparator used in the pairing/security module (i.e., call the existing
constant-time compare function from that module instead of defining a new
constant_time_eq), or extract a common utility function and have both
bearer_matches and the pairing code call that shared helper; ensure you preserve
the non-empty check in bearer_matches and import or expose the shared comparator
symbol so no duplicate logic remains.
2 participants
Summary
Problem
bearer_matcheswas still using direct equality, which can short-circuit on mismatch. The RPC bearer is checked for HTTP, SSE/query-token, Socket.IO, and future transports through this helper, so the comparison boundary should not leak partial-match timing.Solution
The helper now compares byte slices in constant time by walking the longer input and folding byte and length differences into the final result. The change is scoped to
src/core/auth.rs.Submission Checklist
core::authfocused unit tests.Closes #NNNin the## RelatedsectionImpact
Security hardening only. Valid bearer tokens continue to authenticate and invalid/empty/prefix tokens remain rejected; no migration or config changes.
Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
fix/constant-time-rpc-bearere7d2643e773a753eaa218dc4de4d2c8b937da295Validation Run
cargo fmt --checkgit diff --checkdocker run ... CARGO_TARGET_DIR=/target cargo test --lib core::auth(16 passed)cargo fmt --checkValidation Blocked
command:N/Aerror:N/Aimpact:N/ABehavior Changes
Parity Contract
bearer_matcheshelper.Duplicate / Superseded PR Handling
main.Summary by CodeRabbit
Bug Fixes
Tests