diff --git a/src/app/api/auth/oidc/callback/route.ts b/src/app/api/auth/oidc/callback/route.ts index 28e88fe054..fbd5a8df71 100644 --- a/src/app/api/auth/oidc/callback/route.ts +++ b/src/app/api/auth/oidc/callback/route.ts @@ -2,6 +2,7 @@ import { NextResponse } from "next/server"; import { getCachedSettings, updateSettings } from "@/lib/localDb"; import { SignJWT, jwtVerify, createRemoteJWKSet } from "jose"; import { cookies } from "next/headers"; +import { timingSafeCompare } from "@/shared/utils/timingSafeCompare"; // Test seam (static) — allows tests to inject a cookie store and capture the minted auth_token. // Mirrors the pattern in src/app/api/auth/login/route.ts export const oidcCallbackInternals = { @@ -54,7 +55,10 @@ export async function GET(request: Request) { // Validate state from cookie (via seam so tests can capture) const cookieStore = await oidcCallbackInternals.getCookieStore(); const storedState = cookieStore.get("oidc_state")?.value; - if (!storedState || storedState !== returnedState) { + // Constant-time: `!==` short-circuits on the first differing byte, so + // rejection time correlates with matching-prefix length (GHSA-7434-6q4c-33fh). + // The sibling OAuth callback already compares `state` this way. + if (!storedState || !timingSafeCompare(storedState, returnedState)) { return NextResponse.redirect(new URL("/login?oidc_error=invalid_state", originEarly)); } diff --git a/src/shared/middleware/chatAdmissionIdentity.ts b/src/shared/middleware/chatAdmissionIdentity.ts index 0be286af58..f786eaebd4 100644 --- a/src/shared/middleware/chatAdmissionIdentity.ts +++ b/src/shared/middleware/chatAdmissionIdentity.ts @@ -1,4 +1,5 @@ import { createHmac } from "crypto"; +import { timingSafeCompare } from "@/shared/utils/timingSafeCompare"; const ADMISSION_BYPASS_VALUE = "internal"; const SELF_LOOP_KEY = "sk_omniroute"; @@ -31,7 +32,10 @@ export function isInternalAdmissionBypass(request: Request): boolean { const auth = request.headers.get("authorization") || ""; const match = /^bearer\s+(\S+)$/i.exec(auth.trim()); - return Boolean(match && match[1].trim().toLowerCase() === resolveSelfLoopBearer().toLowerCase()); + if (!match) return false; + // This gates an admission-lane bypass on a shared secret, so the compare is + // constant-time — `===` leaks matching-prefix length (GHSA-7434 class). + return timingSafeCompare(match[1].trim().toLowerCase(), resolveSelfLoopBearer().toLowerCase()); } function fingerprint(value: string): string { diff --git a/src/shared/utils/timingSafeCompare.ts b/src/shared/utils/timingSafeCompare.ts new file mode 100644 index 0000000000..a94b488b22 --- /dev/null +++ b/src/shared/utils/timingSafeCompare.ts @@ -0,0 +1,27 @@ +import { timingSafeEqual } from "crypto"; + +/** + * Constant-time string comparison for secrets, tokens and single-use nonces. + * + * `===` short-circuits on the first differing byte, so rejection time + * correlates with how much of the value the caller already guessed (CWE-208). + * That is the comparison this repo already avoids in every OAuth callback, the + * A2A token check, the Telegram initData HMAC and the CLI token check — each of + * which grew its own private copy of these five lines. This is the shared one: + * reach for it instead of writing a ninth copy, and instead of `===`. + * + * Length is not secret here (it leaks through the early return, as it does in + * every other copy) — the value being protected is the content, not its size. + * `null`/`undefined` compare by identity so a missing secret never matches a + * present one. + */ +export function timingSafeCompare( + a: string | null | undefined, + b: string | null | undefined +): boolean { + if (a == null || b == null) return a === b; + const bufA = Buffer.from(String(a), "utf8"); + const bufB = Buffer.from(String(b), "utf8"); + if (bufA.length !== bufB.length) return false; + return timingSafeEqual(bufA, bufB); +} diff --git a/tests/unit/timing-safe-compare.test.ts b/tests/unit/timing-safe-compare.test.ts new file mode 100644 index 0000000000..cecca952d5 --- /dev/null +++ b/tests/unit/timing-safe-compare.test.ts @@ -0,0 +1,61 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import { readFileSync } from "node:fs"; +import { fileURLToPath } from "node:url"; + +import { timingSafeCompare } from "../../src/shared/utils/timingSafeCompare.ts"; + +// GHSA-7434-6q4c-33fh — the OIDC callback compared the CSRF `state` cookie with +// `!==` while every sibling callback already used a constant-time compare. Low +// severity on its own (single-use nonce), but the pattern gets copied, so the +// guard below pins the two callsites to the shared helper. + +test("timingSafeCompare accepts identical values", () => { + assert.equal(timingSafeCompare("abc123", "abc123"), true); + assert.equal(timingSafeCompare("", ""), true); +}); + +test("timingSafeCompare rejects different values, including same-length ones", () => { + assert.equal(timingSafeCompare("abc123", "abc124"), false); + assert.equal(timingSafeCompare("abc123", "xbc123"), false); + assert.equal(timingSafeCompare("abc", "abcdef"), false); + assert.equal(timingSafeCompare("abcdef", "abc"), false); +}); + +test("timingSafeCompare compares null/undefined by identity, never as a match", () => { + assert.equal(timingSafeCompare(null, null), true); + assert.equal(timingSafeCompare(undefined, undefined), true); + assert.equal(timingSafeCompare(null, undefined), false); + assert.equal(timingSafeCompare(null, "abc"), false); + assert.equal(timingSafeCompare("abc", undefined), false); + assert.equal(timingSafeCompare(undefined, ""), false); +}); + +test("timingSafeCompare is byte-exact, not unicode-normalizing", () => { + // "é" precomposed vs decomposed — different bytes, must not match. + assert.equal(timingSafeCompare("é", "é"), false); +}); + +function sourceOf(relPath: string): string { + return readFileSync(fileURLToPath(new URL(`../../${relPath}`, import.meta.url)), "utf8"); +} + +test("the OIDC callback validates `state` with the constant-time helper", () => { + const source = sourceOf("src/app/api/auth/oidc/callback/route.ts"); + assert.ok( + source.includes("timingSafeCompare"), + "oidc/callback must compare the state cookie in constant time (GHSA-7434-6q4c-33fh)" + ); + assert.ok( + !/storedState\s*!==\s*returnedState/.test(source), + "the short-circuiting `!==` state comparison is back" + ); +}); + +test("the internal admission bypass compares its bearer in constant time", () => { + const source = sourceOf("src/shared/middleware/chatAdmissionIdentity.ts"); + assert.ok( + source.includes("timingSafeCompare"), + "isInternalAdmissionBypass gates a bypass on a shared secret — compare it in constant time" + ); +});