🛡️ Sentinel: [CRITICAL] Fix string timing attack risk#50
Conversation
Convert string parameters to Uint8Array using TextEncoder before using timingSafeEqual to avoid JS engine string implementation timing differences. Co-authored-by: aloewright <3641844+aloewright@users.noreply.github.com>
|
👋 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. |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
postpilot | c9f7d90 | May 29 2026, 05:29 AM |
WalkthroughThis PR hardens security-critical string comparisons by applying constant-time byte-encoded equality checks. It documents the timing-safe pattern requirement and applies it to webhook signature verification and admin API key validation. ChangesTiming-Safe Comparison Pattern
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Warning Review ran into problems🔥 ProblemsThese MCP integrations need to be re-authenticated in the Integrations settings: Sentry 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses a timing attack vulnerability by converting raw strings to Uint8Array buffers using TextEncoder before comparing them with Hono's timingSafeEqual in both polar.ts and admin.ts. The reviewer correctly identified that passing Uint8Array directly to timingSafeEqual will trigger TypeScript compilation errors because the function expects string, ArrayBuffer, or DataView. The reviewer provided actionable suggestions to resolve this by accessing the underlying .buffer property of the Uint8Array.
| const encoder = new TextEncoder(); | ||
| const expectedBuf = encoder.encode(expected); | ||
| for (const c of candidates) { | ||
| const [version, sig] = c.split(","); | ||
| if (version === "v1" && sig && (await timingSafeEqual(sig, expected))) { | ||
| if ( | ||
| version === "v1" && | ||
| sig && | ||
| (await timingSafeEqual(encoder.encode(sig), expectedBuf)) | ||
| ) { |
There was a problem hiding this comment.
Passing a Uint8Array (returned by TextEncoder.prototype.encode()) directly to Hono's timingSafeEqual will cause a TypeScript compilation error because timingSafeEqual expects string | ArrayBuffer | DataView, and Uint8Array is not assignable to ArrayBuffer | DataView in strict mode. Accessing the underlying .buffer property of the Uint8Array resolves this type mismatch cleanly and safely.
| const encoder = new TextEncoder(); | |
| const expectedBuf = encoder.encode(expected); | |
| for (const c of candidates) { | |
| const [version, sig] = c.split(","); | |
| if (version === "v1" && sig && (await timingSafeEqual(sig, expected))) { | |
| if ( | |
| version === "v1" && | |
| sig && | |
| (await timingSafeEqual(encoder.encode(sig), expectedBuf)) | |
| ) { | |
| const encoder = new TextEncoder(); | |
| const expectedBuf = encoder.encode(expected).buffer; | |
| for (const c of candidates) { | |
| const [version, sig] = c.split(","); | |
| if ( | |
| version === "v1" && | |
| sig && | |
| (await timingSafeEqual(encoder.encode(sig).buffer, expectedBuf)) | |
| ) { |
| const encoder = new TextEncoder(); | ||
| if ( | ||
| !provided || | ||
| !(await timingSafeEqual(encoder.encode(expected), encoder.encode(provided))) | ||
| ) { |
There was a problem hiding this comment.
Passing a Uint8Array directly to Hono's timingSafeEqual will cause a TypeScript compilation error because timingSafeEqual expects string | ArrayBuffer | DataView. Accessing the underlying .buffer property of the Uint8Array resolves this type mismatch cleanly and safely.
| const encoder = new TextEncoder(); | |
| if ( | |
| !provided || | |
| !(await timingSafeEqual(encoder.encode(expected), encoder.encode(provided))) | |
| ) { | |
| const encoder = new TextEncoder(); | |
| if ( | |
| !provided || | |
| !(await timingSafeEqual(encoder.encode(expected).buffer, encoder.encode(provided).buffer)) | |
| ) { |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In @.jules/sentinel.md:
- Around line 9-12: Update the sentinel entry so the date and guidance are
consistent: change the future date on the current entry to the PR date
(2026-05-29) and modify the earlier entry (the one that currently implies raw
strings are acceptable) to require converting strings to Uint8Array before using
timingSafeEqual; explicitly reference the constant-time function timingSafeEqual
and the recommended conversion via new TextEncoder().encode() (or equivalent) so
both entries uniformly instruct encoding strings to bytes prior to comparison.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2cd79bf5-c6fe-44cd-8f13-56940b671118
📒 Files selected for processing (3)
.jules/sentinel.mdapps/quill/src/lib/polar.tsapps/quill/src/routes/admin.ts
| ## 2026-12-29 - [Fix String Timing Attack Risk] | ||
| **Vulnerability:** Constant-time comparison function `timingSafeEqual` from `hono/utils/buffer` was used with raw strings instead of Uint8Array buffers. This undermines the timing attack protection because string representations in JavaScript engines can cause varying execution times, negating the constant-time guarantee. | ||
| **Learning:** Custom constant-time string comparisons in JavaScript/TypeScript are unreliable against timing attacks due to JIT optimizations and engine string storage mechanisms. | ||
| **Prevention:** Always convert strings to `Uint8Array` buffers using `new TextEncoder().encode()` before comparing them with `timingSafeEqual` to prevent timing leaks. |
There was a problem hiding this comment.
Align this sentinel entry with the earlier one and fix the date.
Line 9 is future-dated (2026-12-29) even though this PR was opened on May 29, 2026, and this new guidance now conflicts with Lines 3-4, which still imply raw strings are fine. Please update the earlier entry to require byte encoding too, otherwise the security note stays internally inconsistent.
🤖 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 @.jules/sentinel.md around lines 9 - 12, Update the sentinel entry so the
date and guidance are consistent: change the future date on the current entry to
the PR date (2026-05-29) and modify the earlier entry (the one that currently
implies raw strings are acceptable) to require converting strings to Uint8Array
before using timingSafeEqual; explicitly reference the constant-time function
timingSafeEqual and the recommended conversion via new TextEncoder().encode()
(or equivalent) so both entries uniformly instruct encoding strings to bytes
prior to comparison.
timingSafeEqualfromhono/utils/bufferwas used with raw strings instead of Uint8Array buffers. This undermines the timing attack protection because string representations in JavaScript engines can cause varying execution times.TextEncoder().encode()to encode strings toUint8Arraybefore passing totimingSafeEqual.pnpm build,pnpm typecheck,pnpm check.PR created automatically by Jules for task 5930281496800758417 started by @aloewright
Summary by CodeRabbit
Bug Fixes
Documentation