Review of #30 found four mutations that left the suite green: an early success return for the read kind, a reversed `want === header` comparison, and tokenMatches() reduced to `===` or to raw strings behind a length check (the GHSA-376v-xgjx-7mfr shape). Behavioural guard: a spy on crypto.timingSafeEqual, set before a fresh require of index.js, requires every request (6 read routes, short and same-length wrong tokens, the right token, the ingest route) to compare two 32-byte digests. Source pins now also allowlist every equality operator in checkAuth() (both operand orders), require a single `return null`, and check that tokenMatches() hashes both sides without a length test. All seven mutations (the reviewer's four, the original three) now turn 13-15 tests red. CLAUDE.md no longer claims the pin covers the helper. Refs #17 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
344 lines
14 KiB
JavaScript
344 lines
14 KiB
JavaScript
const http = require("node:http");
|
|
const fs = require("node:fs");
|
|
const os = require("node:os");
|
|
const path = require("node:path");
|
|
|
|
// Authentication safety net for the read routes.
|
|
//
|
|
// findings.test.js and health.test.js cover what each route *returns*; this
|
|
// file covers the gate in front of all of them. The small overlap on
|
|
// /defenseurs/findings and /health is deliberate: the value of this file is the
|
|
// complete route x failure-mode matrix in one place, so a routing/auth refactor
|
|
// that only rewires part of the table still fails loudly here.
|
|
//
|
|
// These tests describe the CURRENT behaviour of index.js. If one of them turns
|
|
// red after a refactor, the refactor changed the security contract.
|
|
|
|
const TOKEN = "test-token";
|
|
|
|
// Every GET route reachable by the handler. Any new route must be added here.
|
|
// POST /hosts/<id> is not a GET, so it gets its own describe block below —
|
|
// but it is covered, and any future route must be too.
|
|
const ROUTES = [
|
|
"/health",
|
|
"/defenseurs",
|
|
"/defenseurs/findings",
|
|
"/defenseurs/state",
|
|
"/reports/scans",
|
|
"/hosts",
|
|
];
|
|
|
|
// Ingest token, distinct from the read token on purpose: a read token must not
|
|
// open the write path, and a write token must not open the read routes.
|
|
const INGEST_TOKEN = "test-ingest-token";
|
|
|
|
let tmpDir;
|
|
let server;
|
|
let baseUrl;
|
|
let handler;
|
|
|
|
function startServer() {
|
|
// Fresh import so module-level constants pick up the stubbed env.
|
|
delete require.cache[require.resolve("../index.js")];
|
|
({ handler } = require("../index.js"));
|
|
server = http.createServer(handler);
|
|
return new Promise((resolve) => {
|
|
server.listen(0, () => {
|
|
const { port } = server.address();
|
|
baseUrl = `http://127.0.0.1:${port}`;
|
|
resolve();
|
|
});
|
|
});
|
|
}
|
|
|
|
function stopServer() {
|
|
return new Promise((resolve) => {
|
|
if (!server) return resolve();
|
|
server.close(() => resolve());
|
|
});
|
|
}
|
|
|
|
async function restartServer() {
|
|
await stopServer();
|
|
await startServer();
|
|
}
|
|
|
|
async function request(path, { method = "GET", auth = `Bearer ${TOKEN}` } = {}) {
|
|
const headers = {};
|
|
if (auth) headers.Authorization = auth;
|
|
const res = await fetch(`${baseUrl}${path}`, { method, headers });
|
|
const body = await res.json().catch(() => ({}));
|
|
return { status: res.status, body };
|
|
}
|
|
|
|
beforeEach(async () => {
|
|
tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "vps-health-auth-test-"));
|
|
|
|
process.env.HEALTH_TOKEN = TOKEN;
|
|
// Point every filesystem read at the temp dir so a rejected request can never
|
|
// fall back to the host's real /data/defenseurs.
|
|
process.env.REPORTS_DIR = path.join(tmpDir, "reports");
|
|
process.env.DEFENSEURS_AGENTS_MAP_PATH = path.join(tmpDir, "agents-map.json");
|
|
process.env.DEFENSEURS_STATUS_PATH = path.join(tmpDir, "status.json");
|
|
process.env.DEFENSEURS_FINDINGS_STATE_PATH = path.join(tmpDir, "findings-state.json");
|
|
process.env.HOSTS_DIR = path.join(tmpDir, "hosts");
|
|
process.env.HOSTS_ALLOWED_IDS = "thinkpad";
|
|
// Configured on purpose: an unset ingest token answers 503 before the header
|
|
// is ever read, which would hide the 401 these tests are here to pin.
|
|
process.env.HOSTS_INGEST_TOKEN = INGEST_TOKEN;
|
|
// Closed port: /health must never reach the real IdP. If the auth gate ever
|
|
// fails open, the request errors out fast instead of hitting production.
|
|
process.env.LOGTO_HEALTH_URL = "http://127.0.0.1:1/oidc/.well-known/openid-configuration";
|
|
|
|
await startServer();
|
|
});
|
|
|
|
afterEach(async () => {
|
|
await stopServer();
|
|
fs.rmSync(tmpDir, { recursive: true, force: true });
|
|
});
|
|
|
|
describe("auth gate — missing Authorization header", () => {
|
|
test.each(ROUTES)("401 on GET %s with no Authorization header", async (route) => {
|
|
const { status, body } = await request(route, { auth: null });
|
|
expect(status).toBe(401);
|
|
// Pin the gate itself: auth runs before any query-param validation, so
|
|
// /defenseurs/findings and /reports/scans must answer 401, never 400.
|
|
expect(body.error).toBe("Unauthorized");
|
|
});
|
|
});
|
|
|
|
describe("auth gate — invalid token", () => {
|
|
test.each(ROUTES)("401 on GET %s with a wrong bearer token", async (route) => {
|
|
const { status, body } = await request(route, { auth: "Bearer wrong-token" });
|
|
expect(status).toBe(401);
|
|
expect(body.error).toBe("Unauthorized");
|
|
});
|
|
});
|
|
|
|
describe("auth gate — malformed Authorization header", () => {
|
|
// The scheme must be exactly `Bearer `, then the token is compared in
|
|
// constant time via tokenMatches(), so these near-misses are all rejected.
|
|
// /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"],
|
|
];
|
|
|
|
test.each(MALFORMED)("401 on GET /defenseurs — %s", async (_label, header) => {
|
|
const { status, body } = await request("/defenseurs", { auth: header });
|
|
expect(status).toBe(401);
|
|
expect(body.error).toBe("Unauthorized");
|
|
});
|
|
});
|
|
|
|
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;
|
|
await restartServer();
|
|
const { status, body } = await request("/defenseurs", { auth: `Bearer ${TOKEN}` });
|
|
expect(status).toBe(401);
|
|
expect(body.error).toBe("HEALTH_TOKEN not configured");
|
|
});
|
|
});
|
|
|
|
describe("routing", () => {
|
|
test("404 on an unknown route", async () => {
|
|
const { status, body } = await request("/nope");
|
|
expect(status).toBe(404);
|
|
expect(body.error).toBe("Not found");
|
|
});
|
|
|
|
test.each(ROUTES)("404 on POST %s (method not allowed)", async (route) => {
|
|
const { status, body } = await request(route, { method: "POST" });
|
|
expect(status).toBe(404);
|
|
expect(body.error).toBe("Not found");
|
|
});
|
|
|
|
// Current ordering: the route/method check runs BEFORE authentication, so an
|
|
// unauthenticated caller gets 404 rather than 401 on these. Documented as-is;
|
|
// any refactor that flips the order will turn these red on purpose.
|
|
test("404 (not 401) on an unknown route without Authorization", async () => {
|
|
const { status, body } = await request("/nope", { auth: null });
|
|
expect(status).toBe(404);
|
|
expect(body.error).toBe("Not found");
|
|
});
|
|
|
|
test("404 (not 401) on POST /health without Authorization", async () => {
|
|
const { status, body } = await request("/health", { method: "POST", auth: null });
|
|
expect(status).toBe(404);
|
|
expect(body.error).toBe("Not found");
|
|
});
|
|
});
|
|
|
|
// The ingest path is the only write surface on this service, and the only one
|
|
// reachable with a token that is NOT the read token. Both directions of that
|
|
// separation are pinned here: a read token must not write, a write token must
|
|
// not read. The gate answers before the request body is ever buffered, so an
|
|
// unauthenticated caller cannot make the server hold 4 KiB on its behalf.
|
|
describe("auth gate — ingest route", () => {
|
|
test("401 on POST /hosts/<id> with no Authorization header", async () => {
|
|
const { status, body } = await request("/hosts/thinkpad", { method: "POST", auth: null });
|
|
expect(status).toBe(401);
|
|
expect(body.error).toBe("Unauthorized");
|
|
});
|
|
|
|
test("401 on POST /hosts/<id> with the read token", async () => {
|
|
const { status, body } = await request("/hosts/thinkpad", {
|
|
method: "POST",
|
|
auth: `Bearer ${TOKEN}`,
|
|
});
|
|
expect(status).toBe(401);
|
|
expect(body.error).toBe("Unauthorized");
|
|
});
|
|
|
|
test("401 on GET /hosts with the ingest token", async () => {
|
|
const { status, body } = await request("/hosts", { auth: `Bearer ${INGEST_TOKEN}` });
|
|
expect(status).toBe(401);
|
|
expect(body.error).toBe("Unauthorized");
|
|
});
|
|
|
|
// An unknown host id never reaches the handler: the route regex is the
|
|
// path-traversal control, so a malformed id is a 404, not a 403 — and that
|
|
// 404 arrives before authentication, like every other routing decision.
|
|
test("404 (not 401, not 403) on POST /hosts/../../etc/passwd", async () => {
|
|
const { status } = await request("/hosts/../../etc/passwd", { method: "POST", auth: null });
|
|
expect(status).toBe(404);
|
|
});
|
|
});
|