hardening(auth): equalize bearer compare operands on length mismatch - #814
hardening(auth): equalize bearer compare operands on length mismatch#814seonghobae wants to merge 11 commits into
Conversation
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueNo actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughBearer 자격 증명의 길이가 설정된 토큰과 달라도 더미 비교를 수행한 후 401 응답을 반환합니다. 길이가 같으면 기존처럼 Changes인증 실패 처리
Estimated code review effort: 1 (간단) | ~5분 Merge Risk: ⚪ Minimal · up to Bearer 토큰 길이가 다른 인증 요청도 비교 후 401로 거부하도록 변경되며, 기존 실패 동작과 인증 경계가 유지됩니다. 현재 병합을 막을 구체적인 위험은 확인되지 않았습니다. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Current authority
develop@e06b1f3fb10903569124af011da213951e6e2473e40475966b38aa75dae6caa81ba8408ef319dba5src/newsdom_api/main.pyplustests/test_auth_compare_digest.pyFinding classification
This is defense-in-depth hardening, not a demonstrated MEDIUM vulnerability or a claim of wall-clock constant time. Python documents
hmac.compare_digest()as content-short-circuit resistant and notes only that different operand lengths can theoretically reveal type/length information, not values. The repository has not established that the configured bearer-token length is a protected secret, nor has it supplied a remote timing experiment that demonstrates exploitable token recovery.The previous PR wording therefore overstated both severity and verification. It is removed from the acceptance authority.
Candidate invariant
For a syntactically valid Bearer credential:
compare_digest()is invoked with equal-length operands (credentials,credentials) rather than unequal operands;The focused tests do not claim to measure timing. They deterministically spy on the comparison boundary and require a wrong-length credential to reach
compare_digest()with equal-length operands, while an equal-length wrong credential must still compare against the configured token. The current descendant also types the pytest monkeypatch fixture so the regression does not introduce an avoidable untyped-test lint boundary.Scope repair
A concurrent/generated
uv.lockchange upgradedpypdf6.15.0→6.17.0 and changed unrelated dependency markers. That delta has no causal relationship to bearer comparison and was restored to the exact protected-base blob through normal descendant347c1fd6464663c771e08a720167e10076870cd2. No force push or destructive rebase was used.Acceptance
Do not merge this as a security vulnerability fix merely because the structural tests pass. Exact-head correctness/security/static-analysis gates must terminate successfully and review must confirm the extra branch/compare is preferable to the simpler protected implementation. Any claim that it makes request handling 'constant time', hides all timing information, or materially changes exploitability requires an explicit threat model and representative timing evidence; otherwise keep the scope as defense-in-depth only.
Fresh exact-head workflows for
e40475966...are non-terminal: tests34000560041, Security Scan34000560033, SAST Semgrep34000560061, CodeQL PR34000560056, codeql34000560053, clusterfuzzlite34000560019, container-image34000560044, and scorecards34000560038. Predecessor GREEN does not transfer.