fix(auth): harden API token comparison against length timing leakage - #830
fix(auth): harden API token comparison against length timing leakage#830seonghobae wants to merge 4 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: trueThanks 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 |
seonghobae
changed the title
1 participant
Current boundary
develop@e06b1f3fb10903569124af011da213951e6e24738d08a83a3a77b9f114843d76f37c400c7ab6138bThe security premise is narrower than the previous title implied. Python's
hmac.compare_digest()is designed to avoid content-based short-circuiting, but the Python documentation explicitly notes that differing lengths can theoretically reveal information about operand types and lengths. The current source therefore compares credential length first and, on mismatch, executes a same-length dummycompare_digest(expected_token, expected_token)before returning 401; equal-length credentials still usecompare_digest(credentials, expected_token).This is a defensible length-side-channel hardening. It is not yet evidence of a remotely exploitable token-recovery vulnerability, and it must not be promoted as one without a realistic timing experiment that separates network/runtime noise from the signal and shows why token length is itself sensitive in the deployment model.
Review findings
The effective PR is wider than the causal authentication change. In addition to
src/newsdom_api/main.py, it contains generated.jules/sentinel.mddoctrine and formatter-only churn across schema/auth tests. The Sentinel entry is especially invalid as repository authority because it is dated2026-10-27, a future date relative to the current repository state, and states a stronger constant-time guarantee than has been demonstrated. These unrelated/generated deltas must be removed or reconciled non-destructively before Ready; do not treat them as security evidence.RED/GREEN acceptance
RED, if the side-channel claim is retained, should measure the protected implementation and candidate under repeated local process-level requests with equal-content-different-length and equal-length-wrong-content credentials, with randomized trial order, warm-up excluded only by a predeclared rule, and median/p95/distribution evidence. Network benchmarks alone are insufficient for causal attribution.
GREEN requires:
compare_digest(credentials, expected_token);If realistic measurement cannot establish a material length signal or token length is not sensitive in the deployment threat model, keep the code only as low-cost defense in depth and withdraw vulnerability/exploit language.
No force push, destructive rebase, self-approval, source-neutral retrigger, synthetic status, predecessor-GREEN transfer, scanner suppression or gate weakening.