Merge pull request 'fix(auth): compare HEALTH_TOKEN in constant time via tokenMatches' (#30) from issue-17-timing-safe-health-token into main

CLAUDE.md

@@ -24,7 +24,7 @@ Ordre **non negociable**, fige par deux tests `404 (not 401)` dans `__tests__/au
 Consequence voulue : un appelant non authentifie sur une route inconnue voit 404, pas 401. Ne pas « corriger » cet ordre en mettant l'auth d'abord.
 
 - Fail-closed : `HEALTH_TOKEN` absent -> 401 sur les lectures ; `HOSTS_INGEST_TOKEN` absent -> 503 sur l'ingestion
-- `tokenMatches()` (`timingSafeEqual` sur empreintes SHA-256) protege **uniquement** le chemin d'ingestion. Le realignement du `HEALTH_TOKEN` existant est suivi separement (issue #17) : il toucherait tous les chemins de lecture en production
+- `tokenMatches()` (`timingSafeEqual` sur empreintes SHA-256, longueurs egalisees par le hachage) compare **les deux tokens** depuis l'issue #17 : `checkAuth()` n'a qu'une expression de comparaison, seul le token attendu varie. Ne jamais revenir a une egalite de chaine (`` header === `Bearer ${x}` ``) : elle s'arrete au premier octet different et fuit longueur et prefixe par le temps de reponse. Un test de timing serait instable ; la garde est double dans `__tests__/auth.test.js` : un espion sur `crypto.timingSafeEqual` (pose avant un `require` frais, `index.js` destructurant l'import au chargement) exige que chaque requete compare deux empreintes de 32 octets, meme quand le token recu n'a pas la bonne longueur ; et des epingles sur le source de `checkAuth()` et de `tokenMatches()`
 - 401 passe avant 403 : un appelant sans token ne peut pas sonder l'allowlist
 - Chaque rejet 401/403 est journalise (`[auth] <code> <method> <url> ip=<X-Real-IP> id=<host> reason=<motif>`), valeurs filtrees en ASCII imprimable pour empecher la forge de lignes de log
 - **Coolify** : `HEALTH_TOKEN` et `HOSTS_INGEST_TOKEN` doivent etre `is_runtime=true, is_buildtime=false`. Buildtime fait fuiter le secret en clair dans `application_deployment_queues.logs`. Voir `la-compagnie-maximus/docs/coolify-ops.md` section "Secrets en buildtime".
@@ -59,11 +59,11 @@ curl -H "Authorization: Bearer $(cat ~/.coolify-token)" \
 
 ## Tests
 
-- `npm test` (vitest) — 263 cas
+- `npm test` (vitest) — 301 cas
   - `__tests__/findings.test.js` — `/defenseurs/findings` (14 cas : auth, validation, filtres severity/category, asymetrie INFO, scan clean vs no_data, JSON corrompu)
   - `__tests__/state.test.js` — `/defenseurs/state` + non-regression `/defenseurs` apres extraction de `serveJsonFile()` (9 cas : corps identique octet pour octet sur un JSON non canonique, no_data, isolation des deux chemins, 401 sans token / mauvais token / token d'ingestion, 404 sur POST)
   - `__tests__/health.test.js` — module `metrics.js` + `/health` (6 cas : exports, payload champ pour champ, garde de latence)
-  - `__tests__/auth.test.js` — filet de securite auth sur les routes de lecture (29 cas : matrice route x mode d'echec, en-tetes malformes, fail-closed, et **2 cas `404 (not 401)` qui figent l'ordre routage -> auth**)
+  - `__tests__/auth.test.js` — filet de securite auth sur les routes de lecture (67 cas : matrice route x mode d'echec, en-tetes malformes (dont le separateur `Bearer` autre qu'un espace), quasi-rates du token sur les 6 routes de lecture, fail-closed, **2 cas `404 (not 401)` qui figent l'ordre routage -> auth**, **un espion sur `timingSafeEqual`** qui exige deux empreintes de 32 octets a chaque requete, et **des epingles sur le source** : aucune egalite contre une chaine `Bearer`, un seul `tokenMatches()` et un seul `return null` dans `checkAuth()`, egalites limitees a une liste blanche, `tokenMatches()` qui hache les deux cotes sans test de longueur)
   - `__tests__/hosts.test.js` — matrice exhaustive des endpoints postes (155 cas : regex de route comme controle anti-traversee, 401 avant 403, cloisonnement des deux tokens, 503 fail-closed, reconstruction par liste blanche, 413, `neverSeen`, fraicheur serveur sous horloge figee, degradation de lecture)
   - `__tests__/agent.test.js` — agent poste (50 cas : contrat de payload verifie contre `sanitizeSnapshot()`, une seule tentative sans rejeu, timeout, codes de sortie, wrapper shell, et **le token absent de toute sortie d'echec, fragments de 5 caracteres compris**)
 - Runtime reste 0-dep ; vitest en devDep uniquement

__tests__/auth.test.js

@@ -117,13 +117,18 @@ describe("auth gate — invalid token", () => {
 });
 
 describe("auth gate — malformed Authorization header", () => {
-  // The header is compared byte for byte against `Bearer <token>`, so these
+  // The scheme must be exactly `Bearer `, then the token is compared in
-  // near-misses are all rejected. /defenseurs stands in for the read routes:
+  // constant time via tokenMatches(), so these near-misses are all rejected.
-  // it serves the parc-wide Defenseurs report and is the costliest to leak.
+  // /defenseurs stands in for the read routes: it serves the parc-wide
+  // Defenseurs report and is the costliest to leak.
   const MALFORMED = [
     ["raw token without the Bearer scheme", TOKEN],
     ["lowercase scheme", `bearer ${TOKEN}`],
     ["scheme with no token", "Bearer"],
+    // The separator is exactly one space: `startsWith("Bearer")` followed by
+    // slice(7) would accept any byte in its place.
+    ["tab instead of the space", `Bearer\t${TOKEN}`],
+    ["any other byte instead of the space", `BearerX${TOKEN}`],
   ];
 
   test.each(MALFORMED)("401 on GET /defenseurs — %s", async (_label, header) => {
@@ -133,6 +138,140 @@ describe("auth gate — malformed Authorization header", () => {
   });
 });
 
+describe("auth gate — near-miss tokens on the read routes", () => {
+  // A comparison that stops at the first differing byte leaks the token's
+  // length and prefix through timing (issue #17). These cases pin the
+  // observable contract — exact match only — on every read route. They would
+  // also pass against a naive `===`: the timing property itself is guarded by
+  // the timingSafeEqual spy and the source pins below, not by these.
+  const NEAR_MISSES = [
+    ["token prefix", `Bearer ${TOKEN.slice(0, -1)}`],
+    ["token with a suffix", `Bearer ${TOKEN}x`],
+    ["double space after the scheme", `Bearer  ${TOKEN}`],
+  ];
+  const CASES = ROUTES.flatMap((route) => NEAR_MISSES.map(([label, header]) => [route, label, header]));
+
+  test.each(CASES)("401 on GET %s — %s", async (route, _label, header) => {
+    const { status, body } = await request(route, { auth: header });
+    expect(status).toBe(401);
+    expect(body.error).toBe("Unauthorized");
+  });
+
+  // `Bearer ` with an empty token cannot be sent over the wire: the HTTP
+  // parser strips trailing whitespace and the server sees `Bearer`, already
+  // covered above. The empty-token branch is pinned on the helper directly.
+  test("tokenMatches rejects an empty token", () => {
+    const { tokenMatches } = require("../index.js");
+    expect(tokenMatches("", TOKEN)).toBe(false);
+    expect(tokenMatches(TOKEN, TOKEN)).toBe(true);
+  });
+});
+
+describe("auth gate — every comparison goes through timingSafeEqual on digests", () => {
+  // Behavioural guard for issue #17, independent of how the code is written.
+  // index.js destructures timingSafeEqual at load time, so the spy is set
+  // BEFORE a fresh require. Each request must then reach timingSafeEqual with
+  // two 32-byte buffers (SHA-256 digests). That fails for a short-circuiting
+  // `===` (never called), for a per-kind early return (never called on the
+  // read path), and for raw strings behind a length check (the GHSA-376v-xgjx-7mfr
+  // shape: never called when lengths differ, called with non-digests otherwise).
+  const crypto = require("node:crypto");
+  let spy;
+
+  beforeEach(async () => {
+    spy = vi.spyOn(crypto, "timingSafeEqual");
+    await restartServer();
+  });
+
+  afterEach(() => {
+    spy.mockRestore();
+  });
+
+  function expectDigestComparison() {
+    expect(spy).toHaveBeenCalledTimes(1);
+    const [a, b] = spy.mock.calls[0];
+    expect(Buffer.isBuffer(a) && Buffer.isBuffer(b)).toBe(true);
+    expect([a.length, b.length]).toEqual([32, 32]);
+  }
+
+  test.each(ROUTES)("GET %s with a shorter wrong token compares two digests", async (route) => {
+    const { status } = await request(route, { auth: "Bearer x" });
+    expect(status).toBe(401);
+    expectDigestComparison();
+  });
+
+  test.each(ROUTES)("GET %s with a same-length wrong token compares two digests", async (route) => {
+    const sameLength = "x".repeat(TOKEN.length);
+    const { status } = await request(route, { auth: `Bearer ${sameLength}` });
+    expect(status).toBe(401);
+    expectDigestComparison();
+  });
+
+  test("GET /defenseurs with the right token compares two digests", async () => {
+    const { status } = await request("/defenseurs");
+    expect(status).toBe(200);
+    expectDigestComparison();
+  });
+
+  test("POST /hosts/<id> with a shorter wrong token compares two digests", async () => {
+    const { status } = await request("/hosts/thinkpad", { method: "POST", auth: "Bearer x" });
+    expect(status).toBe(401);
+    expectDigestComparison();
+  });
+});
+
+describe("auth gate — source pins: one constant-time comparison", () => {
+  // Structural complement to the spy above: read index.js, comments stripped,
+  // and check how checkAuth() and tokenMatches() are written.
+  const source = fs
+    .readFileSync(path.join(__dirname, "..", "index.js"), "utf-8")
+    .replace(/\/\*[\s\S]*?\*\//g, "")
+    .replace(/^\s*\/\/.*$/gm, "");
+
+  function functionBody(name) {
+    const start = source.indexOf(`function ${name}(`);
+    expect(start).toBeGreaterThanOrEqual(0);
+    const rest = source.slice(start + 1);
+    const next = rest.search(/^(async\s+)?function\s/m);
+    return next === -1 ? rest : rest.slice(0, next);
+  }
+
+  // Every equality operator in a body, with its two operands.
+  function comparisons(body) {
+    return [...body.matchAll(/([\w."'$]+)\s*(===|!==|==|!=)\s*([\w."'$]+)/g)].map(
+      ([, left, op, right]) => `${left} ${op} ${right}`,
+    );
+  }
+
+  test("no direct equality against a Bearer string anywhere in index.js", () => {
+    // Template literal (`Bearer ${x}`) or concatenation ("Bearer " + x), on
+    // either side of == / ===.
+    const bearerExpr = String.raw`(\`Bearer [^\`]*\`|["']Bearer ["']\s*\+\s*[\w.]+)`;
+    const direct = new RegExp(String.raw`[!=]==?\s*${bearerExpr}|${bearerExpr}\s*[!=]==?`);
+    expect(source).not.toMatch(direct);
+  });
+
+  test("checkAuth() has one tokenMatches call, one success exit, no token equality", () => {
+    const body = functionBody("checkAuth");
+    expect(body.match(/tokenMatches\(/g)).toHaveLength(1);
+    // A single `return null`: no early success exit for one token kind.
+    expect(body.match(/return\s+null\b/g)).toHaveLength(1);
+    // Allowlist, both operand orders included: anything else (`want ===
+    // header`, `expected === token`) is a new comparison on a secret.
+    expect(comparisons(body).sort()).toEqual(['header === "string"', 'route.tokenKind === "ingest"']);
+    expect(body).not.toMatch(/isIngest\s*\?[^;]*tokenMatches/);
+  });
+
+  test("tokenMatches() hashes both sides before timingSafeEqual", () => {
+    const body = functionBody("tokenMatches");
+    expect(body.match(/createHash\("sha256"\)/g)).toHaveLength(2);
+    expect(body.match(/timingSafeEqual\(/g)).toHaveLength(1);
+    // No length check and no equality other than the type guards.
+    expect(body).not.toMatch(/\.length\b/);
+    expect(comparisons(body).sort()).toEqual(['expected !== "string"', 'provided !== "string"']);
+  });
+});
+
 describe("auth gate — fail-closed when HEALTH_TOKEN is unset", () => {
   test("401 on GET /defenseurs even with a well-formed bearer token", async () => {
     delete process.env.HEALTH_TOKEN;

index.js

@@ -257,13 +257,11 @@ function logRejection(req, status, reason, id) {
   );
 }
 
-// Constant-time bearer comparison for the ingestion path. Both sides are hashed
+// Constant-time bearer comparison, used for BOTH tokens (read and ingest). Both
-// first, so the buffers are always 32 bytes: timingSafeEqual never throws on
+// sides are hashed first, so the buffers are always 32 bytes: timingSafeEqual
-// mismatched lengths, and the length of the real token cannot leak.
+// never throws on mismatched lengths, and the length of the real token cannot
-//
+// leak. Passing the raw strings to timingSafeEqual would leak it
-// Deliberately scoped to the new write path. Realigning the existing
+// (GHSA-376v-xgjx-7mfr).
-// HEALTH_TOKEN comparison touches every read route in production and is tracked
-// separately (issue #17).
 function tokenMatches(provided, expected) {
   if (typeof provided !== "string" || typeof expected !== "string") return false;
   const a = createHash("sha256").update(provided).digest();
@@ -690,11 +688,10 @@ function checkAuth(req, route) {
   }
 
   const header = req.headers["authorization"];
-  const ok = isIngest
+  const ok =
-    ? typeof header === "string" &&
+    typeof header === "string" &&
-      header.startsWith("Bearer ") &&
+    header.startsWith("Bearer ") &&
-      tokenMatches(header.slice("Bearer ".length), expected)
+    tokenMatches(header.slice("Bearer ".length), expected);
-    : header === `Bearer ${expected}`;
 
   if (!ok) {
     return { status: 401, body: { error: "Unauthorized" }, reason: "missing or invalid bearer token" };