test(security): add webhook auth and token management tests

marcoscaceres · marcoscaceres · commit 97d90470c8d3 · 2026-04-25T22:12:32.000+10:00
Add comprehensive test suites for two security-critical modules:

- utils/auth-github-webhook: 8 tests covering valid HMAC verification,
  invalid/missing/empty signatures, body tampering, and wrong secrets.
- routes/github/lib/utils/tokens: 9 tests covering token cycling, rate
  limit tracking, and token masking in getLimits() output.

Fix two security issues discovered during testing:

1. Webhook signature comparison used === (timing-attack vulnerable).
   Now uses crypto.timingSafeEqual() with explicit length check and
   early return for missing headers.

2. Token masking in getLimits() replaced only the last 30 chars with 2
   asterisks, leaking up to 10 prefix characters of a 40-char GitHub
   token. Now masks all but the last 4 characters with per-character
   asterisks, preserving output length.
diff --git a/routes/github/lib/utils/tokens.ts b/routes/github/lib/utils/tokens.ts
@@ -26,7 +26,8 @@ export function updateRateLimit(token: string, rateLimit: RateLimit) {
 }
 
 export function getLimits() {
-  const secureToken = (token: string) => token.replace(/.{30}$/, "*".repeat(2));
+  const secureToken = (token: string) =>
+    "*".repeat(Math.max(0, token.length - 4)) + token.slice(-4);
   const result: Record<string, RateLimit | null> = {};
   for (const [token, limits] of LIMITS) {
     result[secureToken(token)] = limits;
diff --git a/tests/routes/github/lib/utils/tokens.test.js b/tests/routes/github/lib/utils/tokens.test.js
@@ -0,0 +1,133 @@
+// GH_TOKEN must be set before importing the module, since tokens.ts
+// calls env("GH_TOKEN") at the top level during module initialization.
+process.env.GH_TOKEN = "ghp_TestToken1234567890abcdefghijklmnopqrst";
+
+const {
+  getToken,
+  updateRateLimit,
+  getLimits,
+} = await import(
+  "../../../../../build/routes/github/lib/utils/tokens.js"
+);
+
+describe("routes/github/lib/utils/tokens", () => {
+  describe("getToken()", () => {
+    it("returns a token string", () => {
+      const token = getToken();
+      expect(typeof token).toBe("string");
+      expect(token.length).toBeGreaterThan(0);
+    });
+
+    it("returns the configured GH_TOKEN", () => {
+      const token = getToken();
+      expect(token).toBe(process.env.GH_TOKEN);
+    });
+
+    it("cycles back to the same token (single-token rotation)", () => {
+      // With a single token, every call returns the same value
+      const first = getToken();
+      const second = getToken();
+      const third = getToken();
+      expect(first).toBe(second);
+      expect(second).toBe(third);
+    });
+  });
+
+  describe("updateRateLimit()", () => {
+    it("stores rate limit data for a token", () => {
+      const token = getToken();
+      const rateLimit = {
+        remaining: 4500,
+        resetAt: new Date("2026-01-01T00:00:00Z"),
+        limit: 5000,
+      };
+      updateRateLimit(token, rateLimit);
+
+      const limits = getLimits();
+      const entries = Object.values(limits);
+      // The stored rate limit should match what we set
+      const stored = entries.find(v => v !== null);
+      expect(stored).toBeDefined();
+      expect(stored.remaining).toBe(4500);
+      expect(stored.limit).toBe(5000);
+    });
+
+    it("updates existing rate limit data", () => {
+      const token = getToken();
+      const first = {
+        remaining: 4500,
+        resetAt: new Date("2026-01-01T00:00:00Z"),
+        limit: 5000,
+      };
+      updateRateLimit(token, first);
+
+      const second = {
+        remaining: 100,
+        resetAt: new Date("2026-01-01T01:00:00Z"),
+        limit: 5000,
+      };
+      updateRateLimit(token, second);
+
+      const limits = getLimits();
+      const entries = Object.values(limits);
+      const stored = entries.find(v => v !== null);
+      expect(stored.remaining).toBe(100);
+    });
+  });
+
+  describe("getLimits()", () => {
+    it("returns an object keyed by masked tokens", () => {
+      const limits = getLimits();
+      expect(typeof limits).toBe("object");
+      expect(Object.keys(limits).length).toBeGreaterThan(0);
+    });
+
+    it("never exposes the full token in keys", () => {
+      const fullToken = process.env.GH_TOKEN;
+      const limits = getLimits();
+      for (const key of Object.keys(limits)) {
+        expect(key).not.toBe(fullToken);
+      }
+    });
+
+    it("masks all but the last 4 characters of the token", () => {
+      const fullToken = process.env.GH_TOKEN;
+      const limits = getLimits();
+      const keys = Object.keys(limits);
+      expect(keys).toHaveSize(1);
+
+      const maskedKey = keys[0];
+      // Should show only the last 4 characters, rest are asterisks
+      const expectedSuffix = fullToken.slice(-4);
+      const expectedStars = "*".repeat(fullToken.length - 4);
+      expect(maskedKey).toBe(expectedStars + expectedSuffix);
+    });
+
+    it("masked key has the same length as the full token", () => {
+      const fullToken = process.env.GH_TOKEN;
+      const limits = getLimits();
+      const maskedKey = Object.keys(limits)[0];
+      // Length is preserved (stars replace chars 1:1)
+      expect(maskedKey.length).toBe(fullToken.length);
+    });
+
+    it("does not leak the token prefix (ghp_ or similar)", () => {
+      const fullToken = process.env.GH_TOKEN;
+      const limits = getLimits();
+      const maskedKey = Object.keys(limits)[0];
+      // The ghp_ prefix must NOT appear in the masked key
+      const prefix = fullToken.slice(0, 4);
+      expect(maskedKey.startsWith(prefix)).toBeFalse();
+      expect(maskedKey.startsWith("*")).toBeTrue();
+    });
+
+    it("exposes at most 4 characters of the actual token", () => {
+      const fullToken = process.env.GH_TOKEN;
+      const limits = getLimits();
+      const maskedKey = Object.keys(limits)[0];
+      // Count non-asterisk characters
+      const visibleChars = [...maskedKey].filter(c => c !== "*").length;
+      expect(visibleChars).toBeLessThanOrEqual(4);
+    });
+  });
+});
diff --git a/tests/utils/auth-github-webhook.test.js b/tests/utils/auth-github-webhook.test.js
@@ -0,0 +1,237 @@
+import { createHmac } from "crypto";
+import githubWebhookAuthenticator from "../../build/utils/auth-github-webhook.js";
+
+/**
+ * Compute the SHA-1 HMAC signature GitHub sends in X-Hub-Signature.
+ * @param {Buffer} body Raw request body
+ * @param {string} secret Shared webhook secret
+ * @returns {string} e.g. "sha1=abc123..."
+ */
+function sign(body, secret) {
+  const hash = createHmac("sha1", secret).update(body).digest("hex");
+  return `sha1=${hash}`;
+}
+
+/**
+ * Create a minimal Express-like request object.
+ * @param {object} options
+ * @param {Buffer} options.body Raw body buffer
+ * @param {Record<string, string>} [options.headers] Request headers
+ * @returns {object} Mock request
+ */
+function mockReq({ body, headers = {} }) {
+  const lowerHeaders = {};
+  for (const [k, v] of Object.entries(headers)) {
+    lowerHeaders[k.toLowerCase()] = v;
+  }
+  return {
+    body,
+    headers: lowerHeaders,
+    get(name) {
+      return this.headers[name.toLowerCase()];
+    },
+  };
+}
+
+/**
+ * Create a minimal Express-like response object.
+ * @returns {{ statusCode: number|null, body: string|null, status: Function, send: Function }}
+ */
+function mockRes() {
+  const res = {
+    statusCode: null,
+    body: null,
+    status(code) {
+      res.statusCode = code;
+      return res;
+    },
+    send(data) {
+      res.body = data;
+      return res;
+    },
+  };
+  return res;
+}
+
+describe("utils/auth-github-webhook", () => {
+  const SECRET = "test-webhook-secret-1234";
+
+  it("returns an array of [rawBodyParser, verifier]", () => {
+    const middleware = githubWebhookAuthenticator(SECRET);
+    expect(Array.isArray(middleware)).toBeTrue();
+    expect(middleware).toHaveSize(2);
+    // First element is express.raw() middleware, second is the verifier fn
+    expect(typeof middleware[0]).toBe("function");
+    expect(typeof middleware[1]).toBe("function");
+  });
+
+  describe("signature verification (verifier middleware)", () => {
+    // We test the second element of the array (the verifier function)
+    // directly, since express.raw() is Express's own middleware.
+    let verifier;
+
+    beforeEach(() => {
+      const middleware = githubWebhookAuthenticator(SECRET);
+      verifier = middleware[1];
+    });
+
+    it("calls next() when the HMAC signature is valid", () => {
+      const payload = { action: "opened", number: 42 };
+      const rawBody = Buffer.from(JSON.stringify(payload));
+      const signature = sign(rawBody, SECRET);
+
+      const req = mockReq({
+        body: rawBody,
+        headers: { "X-Hub-Signature": signature },
+      });
+      const res = mockRes();
+      let nextCalled = false;
+      const next = () => {
+        nextCalled = true;
+      };
+
+      verifier(req, res, next);
+
+      expect(nextCalled).toBeTrue();
+      // Body should be parsed as JSON after verification
+      expect(req.body).toEqual(payload);
+    });
+
+    it("parses the raw body as JSON after successful verification", () => {
+      const payload = { ref: "refs/heads/main", commits: [{ id: "abc" }] };
+      const rawBody = Buffer.from(JSON.stringify(payload));
+      const signature = sign(rawBody, SECRET);
+
+      const req = mockReq({
+        body: rawBody,
+        headers: { "X-Hub-Signature": signature },
+      });
+      const res = mockRes();
+      verifier(req, res, () => {});
+
+      expect(req.body).toEqual(payload);
+      expect(typeof req.body).toBe("object");
+    });
+
+    it("responds with 'pong' for GitHub ping events (zen field present)", () => {
+      const payload = { zen: "Keep it logically awesome.", hook_id: 1 };
+      const rawBody = Buffer.from(JSON.stringify(payload));
+      const signature = sign(rawBody, SECRET);
+
+      const req = mockReq({
+        body: rawBody,
+        headers: { "X-Hub-Signature": signature },
+      });
+      const res = mockRes();
+      let nextCalled = false;
+
+      verifier(req, res, () => {
+        nextCalled = true;
+      });
+
+      expect(nextCalled).toBeFalse();
+      expect(res.body).toBe("pong");
+    });
+
+    it("returns 401 when the signature is invalid", () => {
+      const payload = { action: "opened" };
+      const rawBody = Buffer.from(JSON.stringify(payload));
+      const badSignature = "sha1=0000000000000000000000000000000000000000";
+
+      const req = mockReq({
+        body: rawBody,
+        headers: { "X-Hub-Signature": badSignature },
+      });
+      const res = mockRes();
+      let nextCalled = false;
+
+      verifier(req, res, () => {
+        nextCalled = true;
+      });
+
+      expect(nextCalled).toBeFalse();
+      expect(res.statusCode).toBe(401);
+      expect(res.body).toContain("Failed to authenticate");
+    });
+
+    it("returns 401 when X-Hub-Signature header is missing", () => {
+      const payload = { action: "opened" };
+      const rawBody = Buffer.from(JSON.stringify(payload));
+
+      const req = mockReq({ body: rawBody, headers: {} });
+      const res = mockRes();
+      let nextCalled = false;
+
+      verifier(req, res, () => {
+        nextCalled = true;
+      });
+
+      expect(nextCalled).toBeFalse();
+      expect(res.statusCode).toBe(401);
+    });
+
+    it("returns 401 when the body has been tampered with", () => {
+      const original = { action: "opened", number: 42 };
+      const rawOriginal = Buffer.from(JSON.stringify(original));
+      const signature = sign(rawOriginal, SECRET);
+
+      // Tamper with the body after signing
+      const tampered = { action: "opened", number: 99 };
+      const rawTampered = Buffer.from(JSON.stringify(tampered));
+
+      const req = mockReq({
+        body: rawTampered,
+        headers: { "X-Hub-Signature": signature },
+      });
+      const res = mockRes();
+      let nextCalled = false;
+
+      verifier(req, res, () => {
+        nextCalled = true;
+      });
+
+      expect(nextCalled).toBeFalse();
+      expect(res.statusCode).toBe(401);
+    });
+
+    it("returns 401 when the secret is wrong", () => {
+      const payload = { action: "opened" };
+      const rawBody = Buffer.from(JSON.stringify(payload));
+      // Sign with a different secret
+      const signature = sign(rawBody, "wrong-secret");
+
+      const req = mockReq({
+        body: rawBody,
+        headers: { "X-Hub-Signature": signature },
+      });
+      const res = mockRes();
+      let nextCalled = false;
+
+      verifier(req, res, () => {
+        nextCalled = true;
+      });
+
+      expect(nextCalled).toBeFalse();
+      expect(res.statusCode).toBe(401);
+    });
+
+    it("rejects an empty signature header value", () => {
+      const payload = { action: "opened" };
+      const rawBody = Buffer.from(JSON.stringify(payload));
+
+      const req = mockReq({
+        body: rawBody,
+        headers: { "X-Hub-Signature": "" },
+      });
+      const res = mockRes();
+      let nextCalled = false;
+
+      verifier(req, res, () => {
+        nextCalled = true;
+      });
+
+      expect(nextCalled).toBeFalse();
+      expect(res.statusCode).toBe(401);
+    });
+  });
+});
diff --git a/utils/auth-github-webhook.ts b/utils/auth-github-webhook.ts
@@ -1,4 +1,4 @@
-import { createHmac } from "crypto";
+import { createHmac, timingSafeEqual } from "crypto";
 
 import express from "express";
 import { NextFunction, Request, Response } from "express";
@@ -27,6 +27,9 @@ export default function githubWebhookAuthenticator(secret: string) {
  * See: https://developer.github.com/webhooks/securing/
  */
 function isValidGithubSignature(req: RawRequest, secret: string) {
-  const hash = createHmac("sha1", secret).update(req.body).digest("hex");
-  return req.get("X-Hub-Signature") === `sha1=${hash}`;
+  const signature = req.get("X-Hub-Signature");
+  if (!signature) return false;
+  const expected = `sha1=${createHmac("sha1", secret).update(req.body).digest("hex")}`;
+  if (signature.length !== expected.length) return false;
+  return timingSafeEqual(Buffer.from(signature), Buffer.from(expected));
 }

Original file line number	Diff line number	Diff line change
`@@ -26,7 +26,8 @@ export function updateRateLimit(token: string, rateLimit: RateLimit) {`
`26`	`26`	`}`
`27`	`27`
`28`	`28`	`export function getLimits() {`
`29`		`- const secureToken = (token: string) => token.replace(/.{30}$/, "*".repeat(2));`
	`29`	`+ const secureToken = (token: string) =>`
	`30`	`+ "*".repeat(Math.max(0, token.length - 4)) + token.slice(-4);`
`30`	`31`	`const result: Record<string, RateLimit \| null> = {};`
`31`	`32`	`for (const [token, limits] of LIMITS) {`
`32`	`33`	`result[secureToken(token)] = limits;`