diff --git a/worker/src/signature.ts b/worker/src/signature.ts index 6823d50..0a49f1c 100644 --- a/worker/src/signature.ts +++ b/worker/src/signature.ts @@ -1,10 +1,14 @@ /** - * GitHub webhook signature verification (#227). + * GitHub webhook signature verification (#227, hardened in #229). * - * Verifies GitHub's `X-Hub-Signature-256` header — an HMAC-SHA256 digest of the - * raw request body keyed with the shared webhook secret — using Web Crypto - * (`crypto.subtle`). Returns true only when the recomputed `sha256=` digest - * exactly matches the supplied signature string; any mismatch (tampered body, + * Validates GitHub's `X-Hub-Signature-256` header — an HMAC-SHA256 digest of the + * raw request body keyed with the shared webhook secret — using Web Crypto's + * `crypto.subtle.verify`, which recomputes the HMAC and compares it in CONSTANT + * TIME. The previous implementation compared the recomputed digest with the + * supplied signature via `===`, which early-exits on the first mismatched + * character and leaks the expected signature through response timing — a timing + * side-channel at the webhook trust boundary. The functional contract is + * unchanged: a correct signature returns true; any mismatch (tampered body, * wrong secret, malformed signature) returns false. This is the webhook trust * boundary: it proves both authentic GitHub origin and body integrity. */ @@ -13,17 +17,26 @@ export async function verifyGitHubSignature( body: string, signature: string, ): Promise { + const prefix = "sha256="; + if (!signature.startsWith(prefix)) return false; + const hex = signature.slice(prefix.length); + // An HMAC-SHA256 digest is 32 bytes = exactly 64 hex chars. Reject any other + // shape before touching crypto so malformed input fails fast and uniformly. + if (!/^[0-9a-fA-F]{64}$/.test(hex)) return false; + + const sigBytes = new Uint8Array(32); + for (let i = 0; i < 32; i++) { + sigBytes[i] = parseInt(hex.slice(i * 2, i * 2 + 2), 16); + } + const encoder = new TextEncoder(); const key = await crypto.subtle.importKey( "raw", encoder.encode(secret), { name: "HMAC", hash: "SHA-256" }, false, - ["sign"], + ["verify"], ); - const sig = await crypto.subtle.sign("HMAC", key, encoder.encode(body)); - const expected = "sha256=" + Array.from(new Uint8Array(sig)) - .map((b) => b.toString(16).padStart(2, "0")) - .join(""); - return expected === signature; + // verify() recomputes the HMAC and compares it in constant time. + return crypto.subtle.verify("HMAC", key, sigBytes, encoder.encode(body)); } diff --git a/worker/src/tenant.ts b/worker/src/tenant.ts index b7e59e3..c393da0 100644 --- a/worker/src/tenant.ts +++ b/worker/src/tenant.ts @@ -21,6 +21,15 @@ export interface TenantQuota { events_limit: number; } +/** + * Per-tenant quota window. events_stored is a throughput counter that resets at + * the start of each window, so the quota is "events per window" — it can never + * become a permanent lifetime lockout. (#231 regression: the counter only ever + * incremented and, once it reached events_limit, returned 429 forever because + * nothing reset it across the DO's persistent storage.) + */ +const QUOTA_WINDOW_MS = 60 * 60 * 1000; // 1 hour (matches the 429 Retry-After: 3600) + export class TenantRegistry extends DurableObject { private initialized = false; @@ -46,10 +55,23 @@ export class TenantRegistry extends DurableObject { CREATE TABLE IF NOT EXISTS quotas ( account_id INTEGER PRIMARY KEY, events_stored INTEGER NOT NULL DEFAULT 0, - events_limit INTEGER NOT NULL DEFAULT 10000 + events_limit INTEGER NOT NULL DEFAULT 10000, + window_started_at INTEGER NOT NULL DEFAULT 0 ) `); + // Migration for quotas tables created before the windowed-quota fix (#231): + // add window_started_at if absent. Existing rows keep DEFAULT 0, which + // /quota-check treats as an elapsed window and resets on the next check — + // clearing the permanent lockout left by the old monotonic counter. + try { + this.ctx.storage.sql.exec( + `ALTER TABLE quotas ADD COLUMN window_started_at INTEGER NOT NULL DEFAULT 0`, + ); + } catch { + // Column already exists (table created post-fix) — nothing to migrate. + } + this.initialized = true; } @@ -173,14 +195,14 @@ export class TenantRegistry extends DurableObject { return Response.json(quota); } - // ── Check quota and atomically increment if within limit ── + // ── Check quota and atomically increment if within the current window ── // Returns { allowed: true/false, events_stored, events_limit } // Used by the Worker to gate webhook ingestion before forwarding to WebhookStore DO. if (url.pathname === "/quota-check" && request.method === "POST") { const body = await request.json() as { account_id: number }; const rows = this.ctx.storage.sql.exec( - `SELECT events_stored, events_limit FROM quotas WHERE account_id = ?`, + `SELECT events_stored, events_limit, window_started_at FROM quotas WHERE account_id = ?`, body.account_id, ).toArray(); @@ -191,6 +213,25 @@ export class TenantRegistry extends DurableObject { const stored = rows[0].events_stored as number; const limit = rows[0].events_limit as number; + const windowStartedAt = rows[0].window_started_at as number; + const now = Date.now(); + + // Start a fresh window when the previous one elapsed (or was never set — + // e.g. rows migrated from the pre-fix monotonic counter). This counts the + // current event as #1 of the new window and clears any stale lockout, so + // the quota can never become a permanent 429 (#231). + if (windowStartedAt === 0 || now - windowStartedAt >= QUOTA_WINDOW_MS) { + this.ctx.storage.sql.exec( + `UPDATE quotas SET events_stored = 1, window_started_at = ? WHERE account_id = ?`, + now, + body.account_id, + ); + return Response.json({ + allowed: true, + events_stored: 1, + events_limit: limit, + }); + } if (stored >= limit) { return Response.json({ @@ -201,7 +242,7 @@ export class TenantRegistry extends DurableObject { }, { status: 429 }); } - // Atomically increment + // Within the current window — atomically increment. this.ctx.storage.sql.exec( `UPDATE quotas SET events_stored = events_stored + 1 WHERE account_id = ?`, body.account_id, diff --git a/worker/test/signature.test.ts b/worker/test/signature.test.ts index f45ab57..412dc9f 100644 --- a/worker/test/signature.test.ts +++ b/worker/test/signature.test.ts @@ -1,12 +1,20 @@ /** - * Unit tests for worker/src/signature.ts (#227). + * Unit tests for worker/src/signature.ts (#227, constant-time switch in #229). * * verifyGitHubSignature is the webhook trust boundary: it must accept a body - * signed with the matching secret and reject every other case. The expected - * valid signature is generated INDEPENDENTLY via Node's node:crypto - * (createHmac → "sha256=" + hex digest), giving an oracle that does not call - * the function under test. crypto.subtle exists in Node 20+, so this runs as a - * pure tsx --test unit test (no Miniflare). + * signed with the matching secret and reject every other case. The primary + * oracle is GitHub's OWN published example vector from "Validating webhook + * deliveries" (secret / body / signature below), which cross-checks the impl + * against GitHub's real contract independently of any code under test. An + * independent node:crypto createHmac oracle is also kept for additional + * generated cases. + * + * #229 switched the internal compare from a non-constant-time `===` (which + * early-exits and leaks the expected signature through timing) to + * crypto.subtle.verify (constant-time internal compare). The timing property + * itself is not unit-observable; these tests pin the FUNCTIONAL contract that + * the constant-time implementation preserves. crypto.subtle exists in Node 20+, + * so this runs as a pure tsx --test unit test (no Miniflare). */ import { test } from "node:test"; import assert from "node:assert/strict"; @@ -14,51 +22,55 @@ import { createHmac } from "node:crypto"; import { verifyGitHubSignature } from "../src/signature.js"; -const SECRET = "s3cr3t-webhook-key"; -const BODY = JSON.stringify({ action: "opened", number: 42 }); +// GitHub's published example vector ("Validating webhook deliveries" docs). +const SECRET = "It's a Secret to Everybody"; +const BODY = "Hello, World!"; +const VALID = + "sha256=757107ea0eb2509fc211221cce984b8a37570b6d7586c22c46f4379c8b043e17"; /** Independent oracle: GitHub's X-Hub-Signature-256 = "sha256=" + HMAC-SHA256 hex. */ function sign(secret: string, body: string): string { return "sha256=" + createHmac("sha256", secret).update(body).digest("hex"); } -// ── Valid case ─────────────────────────────────────────────────────── +// ── Valid case (GitHub's published example vector) ─────────────────── -test("accepts a signature generated with the same secret and body", async () => { - const signature = sign(SECRET, BODY); - assert.equal(await verifyGitHubSignature(SECRET, BODY, signature), true); +test("accepts GitHub's published example vector", async () => { + assert.equal(await verifyGitHubSignature(SECRET, BODY, VALID), true); }); // ── Tampered body ──────────────────────────────────────────────────── test("rejects when the body was tampered after signing", async () => { - // Sign body A, verify against body B. - const signature = sign(SECRET, BODY); - const tamperedBody = JSON.stringify({ action: "opened", number: 9999 }); - assert.equal(await verifyGitHubSignature(SECRET, tamperedBody, signature), false); + // Verify a different body against the signature for the original body. + assert.equal(await verifyGitHubSignature(SECRET, "Hello, World?", VALID), false); }); // ── Wrong secret ───────────────────────────────────────────────────── test("rejects when verified with a different secret", async () => { - const signature = sign(SECRET, BODY); - assert.equal(await verifyGitHubSignature("wrong-secret", BODY, signature), false); + assert.equal(await verifyGitHubSignature("wrong-secret", BODY, VALID), false); }); // ── Malformed signature formats ────────────────────────────────────── +test("rejects a signature without the sha256= prefix", async () => { + // Bare hex, no "sha256=" prefix. + const bareHex = createHmac("sha256", SECRET).update(BODY).digest("hex"); + assert.equal(await verifyGitHubSignature(SECRET, BODY, bareHex), false); +}); + test("rejects an empty signature string", async () => { assert.equal(await verifyGitHubSignature(SECRET, BODY, ""), false); }); -test("rejects a signature without the sha256= prefix", async () => { - // Same hex digest but missing the "sha256=" prefix. - const hexOnly = createHmac("sha256", SECRET).update(BODY).digest("hex"); - assert.equal(await verifyGitHubSignature(SECRET, BODY, hexOnly), false); +test("rejects a sha256=-prefixed hex of the wrong length", async () => { + assert.equal(await verifyGitHubSignature(SECRET, BODY, "sha256=deadbeef"), false); }); -test("rejects a sha256=-prefixed hex of the wrong length", async () => { - // Truncated digest (valid prefix, too few hex chars). - const shortHex = createHmac("sha256", SECRET).update(BODY).digest("hex").slice(0, 32); - assert.equal(await verifyGitHubSignature(SECRET, BODY, "sha256=" + shortHex), false); +test("rejects a sha256=-prefixed non-hex string of the right length", async () => { + assert.equal( + await verifyGitHubSignature(SECRET, BODY, "sha256=" + "z".repeat(64)), + false, + ); }); diff --git a/worker/test/workers/tenant.test.ts b/worker/test/workers/tenant.test.ts index 5914d5f..1e768e4 100644 --- a/worker/test/workers/tenant.test.ts +++ b/worker/test/workers/tenant.test.ts @@ -107,17 +107,20 @@ describe("TenantRegistry: quota-check (gate + atomic increment)", () => { expect(quota.events_stored).toBe(1); }); - it("rejects with 429 when stored is at/over the limit, without incrementing", async () => { + it("rejects with 429 when stored is at/over the limit within an active window, without incrementing", async () => { const stub = registryFor("quota-check-at-limit"); await createInstallation(stub, { installation_id: 31, account_id: 71 }); - // push events_stored up to the limit (10000) via a single delta increment, - // avoiding 10000 individual calls. - const inc = await post(stub, "/quota-increment", { account_id: 71, delta: 10000 }); + // Open the window with one real check (events_stored=1, window_started_at=now), + // then push the rest of the way to the limit (10000) within that same window + // via a single delta increment, avoiding 10000 individual calls. + const first = await post(stub, "/quota-check", { account_id: 71 }); + expect(await first.json()).toEqual({ allowed: true, events_stored: 1, events_limit: 10000 }); + const inc = await post(stub, "/quota-increment", { account_id: 71, delta: 9999 }); expect(inc.status).toBe(200); expect(await inc.json()).toEqual({ events_stored: 10000, events_limit: 10000, over_limit: false }); - // stored (10000) >= limit (10000) -> 429 + // stored (10000) >= limit (10000), window still active -> 429 const res = await post(stub, "/quota-check", { account_id: 71 }); expect(res.status).toBe(429); expect(await res.json()).toEqual({ @@ -132,6 +135,24 @@ describe("TenantRegistry: quota-check (gate + atomic increment)", () => { expect(quota.events_stored).toBe(10000); }); + it("resets a stale window (events_stored at limit, window_started_at=0) instead of 429ing forever (#231)", async () => { + const stub = registryFor("quota-check-stale-window-reset"); + await createInstallation(stub, { installation_id: 32, account_id: 72 }); + + // Reproduce the pre-fix lockout: events_stored pushed to the limit while + // window_started_at stays 0 (the migrated monotonic-counter state). Before + // the fix this 429'd permanently; the zero/elapsed window now resets it. + await post(stub, "/quota-increment", { account_id: 72, delta: 10000 }); + + const res = await post(stub, "/quota-check", { account_id: 72 }); + expect(res.status).toBe(200); + expect(await res.json()).toEqual({ allowed: true, events_stored: 1, events_limit: 10000 }); + + // the counter was reset to 1 (this event), not left at the locked 10000 + const quota = (await (await stub.fetch(new Request(`${BASE}/quota?account_id=72`))).json()) as TenantQuota; + expect(quota.events_stored).toBe(1); + }); + it("returns 404 for an unknown tenant", async () => { const stub = registryFor("quota-check-unknown"); const res = await post(stub, "/quota-check", { account_id: 9999 });