Skip to content

🛡️ Sentinel: [CRITICAL] Fix timing attack vulnerability in secret comparisons #43

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
aloewright wants to merge 1 commit into
base: main
Choose a base branch
from
Open

🛡️ Sentinel: [CRITICAL] Fix timing attack vulnerability in secret comparisons #43

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion apps/quill/src/lib/polar.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,9 +182,15 @@ export async function verifyWebhook(
// Header carries one or more `v1,<sig>` pairs (space-separated). Match
// any v1 entry — constant-time compare for each.
const candidates = sigHeader.split(" ");
const enc = new TextEncoder();
const expectedBuf = enc.encode(expected);
Comment on lines +185 to +186
for (const c of candidates) {
const [version, sig] = c.split(",");
if (version === "v1" && sig && (await timingSafeEqual(sig, expected))) {
if (
version === "v1" &&
sig &&
(await timingSafeEqual(enc.encode(sig), expectedBuf))
) {
return true;
}
}
Comment on lines +185 to 196

@gemini-code-assist gemini-code-assist Bot May 26, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Hono's timingSafeEqual utility natively supports raw strings of different lengths. Internally, it automatically converts strings to Uint8Array using TextEncoder and hashes both inputs using SHA-256 before performing a constant-time comparison.

Because of this, manually instantiating TextEncoder and encoding the strings beforehand is redundant and adds unnecessary overhead. We can safely revert to passing the raw strings directly.

  for (const c of candidates) {
    const [version, sig] = c.split(",");
    if (version === "v1" && sig && (await timingSafeEqual(sig, expected))) {
      return true;
    }
  }

Expand Down
6 changes: 5 additions & 1 deletion apps/quill/src/routes/admin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,11 @@ adminRouter.use("*", async (c, next) => {
throw new HTTPException(503, { message: "Admin API not configured." });
}
const provided = c.req.header("x-admin-key");
if (!provided || !(await timingSafeEqual(expected, provided))) {
if (!provided) {
throw new HTTPException(401, { message: "Invalid admin key." });
}
const enc = new TextEncoder();
if (!(await timingSafeEqual(enc.encode(expected), enc.encode(provided)))) {
Comment on lines +25 to +26
throw new HTTPException(401, { message: "Invalid admin key." });
}
Comment on lines +25 to 28

@gemini-code-assist gemini-code-assist Bot May 26, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

As with the webhook verification, Hono's timingSafeEqual natively handles raw strings by encoding and hashing them internally. Manually encoding expected and provided with TextEncoder is redundant. We can simplify this check by passing the strings directly.

  if (!(await timingSafeEqual(expected, provided))) {
    throw new HTTPException(401, { message: "Invalid admin key." });
  }

await next();
Expand Down
Loading