🛡️ Sentinel: [CRITICAL] Fix timing attack vulnerability in secret comparisons#53
🛡️ Sentinel: [CRITICAL] Fix timing attack vulnerability in secret comparisons#53aloewright wants to merge 1 commit into
Conversation
…parisons 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR hardens timing-safe equality comparisons by encoding string credentials to byte arrays before calling ChangesTiming-safe equality comparisons
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 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 |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
postpilot | b5fa4df | May 30 2026, 05:34 AM |
![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 updates webhook verification and admin middleware to encode string values into Uint8Array using TextEncoder before passing them to timingSafeEqual. The review feedback recommends optimizing performance and reducing garbage collection overhead by reusing a single TextEncoder instance instead of instantiating it multiple times on every iteration or request.
| if ( | ||
| version === "v1" && | ||
| sig && | ||
| (await timingSafeEqual( | ||
| new TextEncoder().encode(sig), | ||
| new TextEncoder().encode(expected) | ||
| )) | ||
| ) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Instantiating new TextEncoder() twice on every iteration of the loop creates unnecessary garbage collection overhead and CPU cycles. Since timingSafeEqual from hono/utils/buffer internally instantiates new TextEncoder() if passed raw strings, passing pre-encoded Uint8Arrays is a good way to avoid this. However, we should reuse a single TextEncoder instance to prevent redundant allocations.
Additionally, consider defining a single TextEncoder instance at the module level of this file to reuse it across all helper functions (such as on lines 168 and 178).
if (version === "v1" && sig) {
const encoder = new TextEncoder();
if (
await timingSafeEqual(
encoder.encode(sig),
encoder.encode(expected)
)
) {
return true;
}
}| if ( | ||
| !provided || | ||
| !(await timingSafeEqual( | ||
| new TextEncoder().encode(expected), | ||
| new TextEncoder().encode(provided) | ||
| )) | ||
| ) { |
There was a problem hiding this comment.
Instantiating new TextEncoder() twice on every request to the admin middleware adds unnecessary allocation overhead. Creating a single TextEncoder instance and reusing it to encode both strings avoids this overhead and improves performance.
| if ( | |
| !provided || | |
| !(await timingSafeEqual( | |
| new TextEncoder().encode(expected), | |
| new TextEncoder().encode(provided) | |
| )) | |
| ) { | |
| const encoder = new TextEncoder(); | |
| if ( | |
| !provided || | |
| !(await timingSafeEqual( | |
| encoder.encode(expected), | |
| encoder.encode(provided) | |
| )) | |
| ) { |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b5fa4df8aa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| !(await timingSafeEqual( | ||
| new TextEncoder().encode(expected), | ||
| new TextEncoder().encode(provided) | ||
| )) |
There was a problem hiding this comment.
When x-admin-key is correct, this still rejects every request because Hono's timingSafeEqual hashes the two inputs and then also requires a === b; two freshly encoded Uint8Array objects are never the same reference. In the admin middleware this means all admin endpoints return 401 for any configured key, so the comparison needs to pass strings or use a byte comparator that actually compares buffer contents.
Useful? React with 👍 / 👎.
| (await timingSafeEqual( | ||
| new TextEncoder().encode(sig), | ||
| new TextEncoder().encode(expected) | ||
| )) |
There was a problem hiding this comment.
For valid Polar webhooks, this comparison now always evaluates false because each TextEncoder().encode(...) call creates a distinct Uint8Array, while Hono's timingSafeEqual additionally checks input identity with a === b after hashing. As a result no v1 signature can match and all webhook deliveries are rejected, so this should pass comparable values or switch to an actual byte-safe equality helper.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
🚨 Severity: CRITICAL
💡 Vulnerability: The
timingSafeEqualfunction fromhono/utils/bufferwas called with raw JS string arguments. Due to JS engine optimizations (like flat vs rope strings), comparing strings byte-by-byte can still leak timing information, making it vulnerable to side-channel attacks.🎯 Impact: Attackers could potentially exploit timing differences to guess webhook secrets or the
x-admin-key, leading to unauthorized access or forged webhooks.🔧 Fix: Updated the
timingSafeEqualcalls inapps/quill/src/routes/admin.tsandapps/quill/src/lib/polar.tsto explicitly encode the string arguments intoUint8Arraybyte buffers usingnew TextEncoder().encode()before comparing them. This ensures a true constant-time comparison.✅ Verification: Ran
pnpm checkandpnpm typecheckto verify no type or formatting errors were introduced. Reviewer approved.PR created automatically by Jules for task 6604547924413852452 started by @aloewright
Summary by CodeRabbit