From e181a9691c64caf1e82aa3ca205e96b5734b6948 Mon Sep 17 00:00:00 2001 From: le king fu Date: Sun, 16 Aug 2026 14:24:43 -0400 Subject: [PATCH] fix(review): close the three gaps found in stack review Follow-up to the /pr-review pass on PRs #18-#22. Three findings, none of which changed behaviour, all of which weakened a guarantee the stack was supposed to provide. 1. auth.test.js carried "any new route must be added here" but /hosts was never added when #13 introduced it, so no test asserted GET /hosts -> 401 on a missing header. The drift class the file exists to catch slipped on its first outing. ROUTES now covers /hosts, and a dedicated block pins both directions of the token separation: a read token cannot write, an ingest token cannot read. 2. ingestOversized() folded any transport error into 413, so the four oversized-body tests would have stayed green if the server had stopped writing the status and merely killed the socket - on the one path where "a status, not a dead socket" is the whole client contract. Transport errors are now surfaced instead of swallowed. 3. HOST_AGENT_TIMEOUT_MS was read by push-metrics.js but never exported by run-push.sh, so setting it in the documented env file did nothing. Now exported and documented. Also drops a claim from agent/README.md that the review proved false: the ingest/read token split buys no containment on the ThinkPad, which already stores HEALTH_TOKEN in cleartext for defenseur-auto. Losing that laptop compromises both, so they rotate together. --- __tests__/auth.test.js | 51 +++++++++++++++++++++++++++++++++++++++-- __tests__/hosts.test.js | 9 ++++++-- agent/README.md | 15 ++++++++---- agent/run-push.sh | 5 +++- 4 files changed, 71 insertions(+), 9 deletions(-) diff --git a/__tests__/auth.test.js b/__tests__/auth.test.js index ec9cc46..a7e42ed 100644 --- a/__tests__/auth.test.js +++ b/__tests__/auth.test.js @@ -16,8 +16,14 @@ const path = require("node:path"); const TOKEN = "test-token"; -// Every route reachable by the handler. Any new route must be added here. -const ROUTES = ["/health", "/defenseurs", "/defenseurs/findings", "/reports/scans"]; +// Every GET route reachable by the handler. Any new route must be added here. +// POST /hosts/ 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", "/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; @@ -67,6 +73,11 @@ beforeEach(async () => { 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.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"; @@ -152,3 +163,39 @@ describe("routing", () => { 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/ 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/ 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); + }); +}); diff --git a/__tests__/hosts.test.js b/__tests__/hosts.test.js index 542c076..17292b9 100644 --- a/__tests__/hosts.test.js +++ b/__tests__/hosts.test.js @@ -164,8 +164,13 @@ async function ingestOversized(route, body) { try { const { status } = await ingest(route, { body }); return status; - } catch { - return 413; + } catch (err) { + // A dead socket is NOT a 413. The whole client contract on this path is + // "you get a status code, not a transport error" — so surface the failure + // instead of folding it into the expected value. Returning 413 here would + // keep these tests green even if the server stopped writing the status and + // merely destroyed the connection. + return `transport error instead of a status: ${err.cause?.code || err.code || err.message}`; } } diff --git a/agent/README.md b/agent/README.md index 3a20d1c..6d997ef 100644 --- a/agent/README.md +++ b/agent/README.md @@ -67,16 +67,23 @@ value is the most common install failure. HOSTS_API_URL=https://health.lacompagniemaximus.com HOSTS_INGEST_TOKEN= HOST_ID=thinkpad -# NODE_BIN=/usr/bin/node # only if cron cannot find node (see below) +# NODE_BIN=/usr/bin/node # only if cron cannot find node (see below) +# HOST_AGENT_TIMEOUT_MS=10000 # push timeout, wall-clock; default 10000 ``` ``` chmod 600 ~/.config/maximus-host-agent.env ``` -`HOSTS_INGEST_TOKEN` is the write-only token: it can push snapshots and nothing -else. It is deliberately *not* `HEALTH_TOKEN` — a stolen workstation must not -grant read access to the Defenseurs reports. +`HOSTS_INGEST_TOKEN` is the write-only token: it can push snapshots for an +allowlisted id and nothing else. It is deliberately *not* `HEALTH_TOKEN`, so +that the two can be rotated independently and so a workstation that only pushes +never needs the read token. + +Do not read more into that separation than it gives you. On the ThinkPad it +buys no containment at all: that machine already stores `HEALTH_TOKEN` in +cleartext for the `defenseur-auto` cron, so losing the laptop compromises both +tokens. **Rotate them together** — see `secret-rotation-ops.md`. `HOST_ID` must match `^[a-z0-9][a-z0-9-]{0,31}$` and be listed in the server's `HOSTS_ALLOWED_IDS`, otherwise the push comes back `403`. diff --git a/agent/run-push.sh b/agent/run-push.sh index b5ec568..9864033 100755 --- a/agent/run-push.sh +++ b/agent/run-push.sh @@ -62,6 +62,9 @@ if ! command -v "$NODE_BIN" >/dev/null 2>&1; then exit 1 fi -export HOSTS_API_URL HOSTS_INGEST_TOKEN HOST_ID +# HOST_AGENT_TIMEOUT_MS is optional and read by push-metrics.js. It must be +# exported too, otherwise setting it in the env file silently does nothing and +# the agent always runs on its 10s default. +export HOSTS_API_URL HOSTS_INGEST_TOKEN HOST_ID HOST_AGENT_TIMEOUT_MS exec "$NODE_BIN" "$AGENT" "$@" -- 2.45.2