fix(worker): windowed per-tenant quota + restore #230 (end webhook 429 lockout)

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=<hex>` 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<boolean> {
+  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));
 }

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,

worker/test/signature.test.ts

@@ -1,64 +1,76 @@
 /**
- * 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";
 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,
+  );
 });

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 });