From 742eeaad3ca49ddb7a70f2f5ef19928a01dbec99 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Thu, 10 Sep 2026 01:25:01 +0200 Subject: [PATCH 1/4] fix(ops): refuse a synthetic-journey target that is not ours S1. The journey signs in with a real account and POSTs that account's password to whatever BASE_URL names, and the workflow took BASE_URL as free text. Anyone with repo write, or a leaked token, could dispatch the run at a collector of their own and keep a working credential; secret masking guards the log, never the egress. The host now has to be on a short const allowlist in the script, checked before anything is sent, with exit 2 and a named refusal otherwise. The workflow input is a choice between the two hosted instances, so the form cannot offer a third; the script's own check still stands behind it for a dispatch that goes round the form. --self-test covers the refusal, the prefix lookalike a startsWith check would have waved through included. --- .github/workflows/synthetic-journey.yml | 16 ++++- scripts/synthetic-journey.mjs | 81 ++++++++++++++++++++++++- 2 files changed, 91 insertions(+), 6 deletions(-) diff --git a/.github/workflows/synthetic-journey.yml b/.github/workflows/synthetic-journey.yml index 6354bdcc4..2d096f9b0 100644 --- a/.github/workflows/synthetic-journey.yml +++ b/.github/workflows/synthetic-journey.yml @@ -21,10 +21,19 @@ name: Synthetic journey on: workflow_dispatch: inputs: + # A closed list, not free text. The dispatcher chooses where the probe + # account's password is POSTed, so the choice is between the hosted + # instances and nothing else. The script re-checks the host against its + # own const allowlist and exits 2 on anything else, so a dispatch that + # goes round this form still cannot reach a stranger's collector. base_url: - description: Instance to walk, e.g. https://review.healthlog.dev + description: Instance to walk required: true - type: string + type: choice + default: https://review.healthlog.dev + options: + - https://review.healthlog.dev + - https://healthlog.bombeck.io expected_version: description: Version the instance must serve, e.g. v1.38.13 required: true @@ -53,7 +62,8 @@ jobs: run: node scripts/synthetic-journey.mjs --self-test # Every input and secret rides an env var — never interpolated into - # the shell command. + # the shell command. The script refuses a `BASE_URL` whose host is off + # its allowlist before it sends the password anywhere. - name: Walk the journey env: BASE_URL: ${{ inputs.base_url }} diff --git a/scripts/synthetic-journey.mjs b/scripts/synthetic-journey.mjs index e59d7fa21..eae3442d4 100644 --- a/scripts/synthetic-journey.mjs +++ b/scripts/synthetic-journey.mjs @@ -26,12 +26,16 @@ * the row's id, value, unit and source. * 5. delete — `DELETE /api/measurements/{id}` so the account stays clean. * + * `BASE_URL` decides where the probe account's password is POSTed, so it is + * checked against a closed in-repo host allowlist and refused with exit 2 + * otherwise. + * * The first failing leg ends the run with a non-zero exit, the response * status and a short body excerpt. Anything token-shaped in that excerpt * is redacted before it reaches the log. * * Usage: - * BASE_URL=https://review.example.test \ + * BASE_URL=https://review.healthlog.dev \ * SYNTHETIC_USERNAME=… SYNTHETIC_PASSWORD=… EXPECTED_VERSION=v1.38.13 \ * node scripts/synthetic-journey.mjs * @@ -60,6 +64,36 @@ const BODY_EXCERPT_LIMIT = 400; /** The reading the journey writes. Distinctive, plausible, and short-lived. */ const PROBE = { type: "WEIGHT", value: 77.7, unit: "kg", source: "MANUAL" }; +/** + * The journey signs in with a real account and POSTs that account's password + * to whatever `BASE_URL` names, so `BASE_URL` alone decides where the + * password goes. Anyone able to name the host walks off with a working + * credential; masking guards the log, not the egress. The allowlist is the + * defence, and it is deliberately a const: it changes by a commit and a + * review, never by a form field. + */ +const ALLOWED_HOSTS = [ + "review.healthlog.dev", + "healthlog.bombeck.io", + "localhost", + "127.0.0.1", +]; + +/** + * `null` when the host may receive the probe credentials, otherwise the line + * the caller prints before exiting 2. + */ +function hostRefusal(baseUrl) { + let host; + try { + host = new URL(baseUrl).hostname; + } catch { + return `BASE_URL is not a URL: ${JSON.stringify(baseUrl)}`; + } + if (ALLOWED_HOSTS.includes(host)) return null; + return `BASE_URL host ${JSON.stringify(host)} is not on the allowlist — the probe password is only ever sent to ${ALLOWED_HOSTS.join(", ")}`; +} + /** * Patterns that must never reach a log line. The first three are this * project's own token prefixes plus the OpenAI-style key shape the @@ -488,6 +522,40 @@ function startMockInstance(breaks) { }); } +/** + * The allowlist is the only thing standing between a dispatcher and the + * probe account's password, so prove it refuses — including the prefix + * lookalike, which a `startsWith` check would have waved through. + */ +function selfTestHostAllowlist() { + const cases = [ + { url: "https://review.healthlog.dev", refused: false }, + { url: "https://healthlog.bombeck.io/", refused: false }, + { url: "http://localhost:3000", refused: false }, + { url: "http://127.0.0.1:3000", refused: false }, + { url: "https://healthlog.bombeck.io.example.test", refused: true }, + { url: "https://collector.example.test", refused: true }, + { url: "not-a-url", refused: true }, + ]; + + let failures = 0; + for (const testCase of cases) { + const refusal = hostRefusal(testCase.url); + const label = `host ${testCase.url}`; + if (Boolean(refusal) === testCase.refused) { + console.log( + `self-test ok ${label.padEnd(44)} → ${refusal ? "refused" : "allowed"}`, + ); + } else { + failures += 1; + console.log( + `self-test FAILED ${label.padEnd(44)} → ${refusal ? "refused" : "allowed"}, expected ${testCase.refused ? "refused" : "allowed"}`, + ); + } + } + return failures; +} + async function selfTest() { const cases = [ { breaks: null, expect: "pass" }, @@ -501,7 +569,7 @@ async function selfTest() { { breaks: "delete", expect: "fail", leg: "delete" }, ]; - let failures = 0; + let failures = selfTestHostAllowlist(); for (const testCase of cases) { const label = testCase.breaks ?? "nothing broken"; const instance = await startMockInstance(testCase.breaks); @@ -537,7 +605,9 @@ async function selfTest() { console.error(`\nself-test: ${failures} case(s) behaved wrongly`); return 1; } - console.log(`\nself-test: ${cases.length} cases, every leg proven breakable`); + console.log( + `\nself-test: ${cases.length} journey cases, every leg proven breakable; host allowlist proven`, + ); return 0; } @@ -563,6 +633,11 @@ function readConfig() { ); process.exit(2); } + const refusal = hostRefusal(config.baseUrl); + if (refusal) { + console.error(refusal); + process.exit(2); + } return config; } From 25cb79ecfc4b743c6fb1a9c4101ccf5b81815bd2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Thu, 10 Sep 2026 01:26:04 +0200 Subject: [PATCH 2/4] fix(ops): let no failed journey leg leave a reading behind MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit S2. The legs ran in one loop that returned on the first red one, so a failed read back never reached the delete and the probe account kept the row. Worse, the id was assigned only after the write's await returned: a POST that timed out once the server had already committed left a row the script could not even name. Legs 3 to 5 now sit in a try/finally. The finally sweeps whatever the delete leg did not, and when there is no id it asks the instance for the row by a marker the write puts in `notes`, unique per run and known before the request goes out. The sweep never throws — the red leg is the verdict, and it says out loud what it could not remove. --self-test asserts the mock holds no rows after every case that got as far as the write, so a red read that skipped the delete is caught. --- scripts/synthetic-journey.mjs | 187 +++++++++++++++++++++++++++------- 1 file changed, 152 insertions(+), 35 deletions(-) diff --git a/scripts/synthetic-journey.mjs b/scripts/synthetic-journey.mjs index eae3442d4..dd28e5e65 100644 --- a/scripts/synthetic-journey.mjs +++ b/scripts/synthetic-journey.mjs @@ -26,6 +26,13 @@ * the row's id, value, unit and source. * 5. delete — `DELETE /api/measurements/{id}` so the account stays clean. * + * Legs 3 to 5 run inside a `try`/`finally`: from the write on, the run owns + * a row on a real account, and a red read leg must not leave it behind. The + * `finally` sweeps whatever the delete leg did not — including a row whose + * id the write leg never learned, because the response was lost after the + * server had committed. That row is found again by a marker the write puts + * in `notes`, unique per run. + * * `BASE_URL` decides where the probe account's password is POSTed, so it is * checked against a closed in-repo host allowlist and refused with exit 2 * otherwise. @@ -279,17 +286,19 @@ async function signIn(baseUrl, jar, username, password) { return { detail: "session cookie held" }; } -async function writeReading(baseUrl, jar) { +async function writeReading(baseUrl, jar, run) { const measuredAt = new Date().toISOString(); const result = await call(baseUrl, "/api/measurements", { method: "POST", jar, - headers: { "idempotency-key": randomUUID() }, + headers: { "idempotency-key": run.idempotencyKey }, body: { type: PROBE.type, value: PROBE.value, measuredAt, source: PROBE.source, + // The one handle on this row that exists before the response does. + notes: run.marker, }, }); expectStatus(result, 201, "write"); @@ -303,7 +312,7 @@ async function writeReading(baseUrl, jar) { return { id: created.id, measuredAt, detail: `wrote ${created.id}` }; } -async function readBack(baseUrl, jar, written) { +async function readBack(baseUrl, jar, writtenId) { const result = await call( baseUrl, "/api/measurements?type=WEIGHT&limit=1&sortBy=measuredAt&sortDir=desc", @@ -318,15 +327,15 @@ async function readBack(baseUrl, jar, written) { }); } const row = rows[0]; - expectValue(row.id, written.id, "read back: id", result); + expectValue(row.id, writtenId, "read back: id", result); expectValue(row.value, PROBE.value, "read back: value", result); expectValue(row.unit, PROBE.unit, "read back: unit", result); expectValue(row.source, PROBE.source, "read back: source", result); return { detail: `${row.value} ${row.unit} (${row.source})` }; } -async function removeReading(baseUrl, jar, written) { - const result = await call(baseUrl, `/api/measurements/${written.id}`, { +async function removeReading(baseUrl, jar, writtenId) { + const result = await call(baseUrl, `/api/measurements/${writtenId}`, { method: "DELETE", jar, }); @@ -334,6 +343,46 @@ async function removeReading(baseUrl, jar, written) { return { detail: "account left clean" }; } +/** + * The id the write leg never learned. A POST that times out after the server + * committed leaves a row this run owns and cannot name, so ask the instance + * for it: the marker rides in `notes`, is unique per run, and comes back + * decrypted on the list. + */ +async function findLeftover(baseUrl, jar, marker) { + const result = await call( + baseUrl, + "/api/measurements?type=WEIGHT&limit=20&sortBy=measuredAt&sortDir=desc", + { jar }, + ); + if (result.status !== 200) return null; + const rows = result.json?.data?.measurements; + if (!Array.isArray(rows)) return null; + return rows.find((row) => row?.notes === marker)?.id ?? null; +} + +/** + * Best effort, never throws: a red leg is already the run's verdict, and a + * sweep that threw would replace it. It says out loud what it could not + * remove so the maintainer can. + */ +async function sweep(baseUrl, jar, writtenId, marker, log) { + let id = writtenId; + if (!id) { + id = await findLeftover(baseUrl, jar, marker).catch(() => null); + } + if (!id) return; + const result = await call(baseUrl, `/api/measurements/${id}`, { + method: "DELETE", + jar, + }).catch(() => null); + log( + result?.status === 200 + ? `sweep removed ${id} after a failed leg` + : `sweep could not remove ${id} — delete it by hand`, + ); +} + /** * Run the journey. Returns `{ ok: true }` or `{ ok: false, leg, error }` — * the caller decides what an exit code is, which is what lets the self-test @@ -342,6 +391,13 @@ async function removeReading(baseUrl, jar, written) { async function runJourney(config, log = console.log) { const { baseUrl, username, password, expectedVersion } = config; const jar = createCookieJar(); + // Both known BEFORE the write, so a write whose response never arrives is + // still recoverable: the key folds into the server's idempotency cell, the + // marker into the row's own `notes`. + const run = { + idempotencyKey: randomUUID(), + marker: `synthetic-journey ${randomUUID()}`, + }; const warmStarted = performance.now(); await warm(baseUrl, jar); @@ -349,33 +405,19 @@ async function runJourney(config, log = console.log) { `warm ${Math.round(performance.now() - warmStarted)} ms (discarded)`, ); - let written = null; - const legs = [ - { - name: "version", - run: () => assertVersion(baseUrl, jar, expectedVersion), - }, - { name: "sign in", run: () => signIn(baseUrl, jar, username, password) }, - { - name: "write", - run: async () => (written = await writeReading(baseUrl, jar)), - }, - { name: "read back", run: () => readBack(baseUrl, jar, written) }, - { name: "delete", run: () => removeReading(baseUrl, jar, written) }, - ]; - - for (const leg of legs) { + const runLeg = async (name, execute) => { const started = performance.now(); try { - const outcome = await leg.run(); + const outcome = await execute(); const elapsed = Math.round(performance.now() - started); log( - `ok ${leg.name.padEnd(10)} ${String(elapsed).padStart(6)} ms ${outcome?.detail ?? ""}`.trimEnd(), + `ok ${name.padEnd(10)} ${String(elapsed).padStart(6)} ms ${outcome?.detail ?? ""}`.trimEnd(), ); + return { ok: true }; } catch (error) { const elapsed = Math.round(performance.now() - started); log( - `FAILED ${leg.name.padEnd(10)} ${String(elapsed).padStart(6)} ms ${redact(error.message)}`, + `FAILED ${name.padEnd(10)} ${String(elapsed).padStart(6)} ms ${redact(error.message)}`, ); if (error instanceof LegFailure) { if (error.status !== null) log(` status ${error.status}`); @@ -383,8 +425,48 @@ async function runJourney(config, log = console.log) { } else { log(` ${redact(error.stack ?? "")}`); } - return { ok: false, leg: leg.name, error }; + return { ok: false, leg: name, error }; } + }; + + for (const [name, execute] of [ + ["version", () => assertVersion(baseUrl, jar, expectedVersion)], + ["sign in", () => signIn(baseUrl, jar, username, password)], + ]) { + const result = await runLeg(name, execute); + if (!result.ok) return result; + } + + // From the write on, the run owns a row on somebody's real account. A red + // read leg is not a reason to leave it there, so the cleanup sits in a + // `finally` and not in a leg that a `return` can skip. + let writtenId = null; + let removed = false; + try { + for (const [name, execute] of [ + [ + "write", + async () => { + const written = await writeReading(baseUrl, jar, run); + writtenId = written.id; + return written; + }, + ], + ["read back", () => readBack(baseUrl, jar, writtenId)], + [ + "delete", + async () => { + const outcome = await removeReading(baseUrl, jar, writtenId); + removed = true; + return outcome; + }, + ], + ]) { + const result = await runLeg(name, execute); + if (!result.ok) return result; + } + } finally { + if (!removed) await sweep(baseUrl, jar, writtenId, run.marker, log); } return { ok: true }; } @@ -470,8 +552,14 @@ function startMockInstance(breaks) { unit: PROBE.unit, source: parsed.source, measuredAt: parsed.measuredAt, + notes: parsed.notes ?? null, }; rows.set(row.id, row); + // The case the sweep exists for: the server committed and the + // caller never learned the id. Only the marker leads back here. + if (breaks === "write: committed") { + return send(500, { data: null, error: "gateway timed out" }); + } send(201, { data: row, error: null }); }); return; @@ -516,6 +604,7 @@ function startMockInstance(breaks) { const { port } = server.address(); resolve({ baseUrl: `http://127.0.0.1:${port}`, + rows, close: () => new Promise((done) => server.close(done)), }); }); @@ -558,14 +647,40 @@ function selfTestHostAllowlist() { async function selfTest() { const cases = [ - { breaks: null, expect: "pass" }, + { breaks: null, expect: "pass", clean: true }, { breaks: "version", expect: "fail", leg: "version" }, { breaks: "sign in", expect: "fail", leg: "sign in" }, - { breaks: "write", expect: "fail", leg: "write" }, - { breaks: "read back: missing", expect: "fail", leg: "read back" }, - { breaks: "read back: value", expect: "fail", leg: "read back" }, - { breaks: "read back: unit", expect: "fail", leg: "read back" }, - { breaks: "read back: source", expect: "fail", leg: "read back" }, + { breaks: "write", expect: "fail", leg: "write", clean: true }, + // The write committed and the response was lost: no id, and the sweep + // has to find the row again by its marker. + { breaks: "write: committed", expect: "fail", leg: "write", clean: true }, + // The four read cases are also the proof that a red read still deletes: + // `clean` is asserted after the run, and only the `finally` can satisfy + // it once the delete leg has been skipped. + { + breaks: "read back: missing", + expect: "fail", + leg: "read back", + clean: true, + }, + { + breaks: "read back: value", + expect: "fail", + leg: "read back", + clean: true, + }, + { + breaks: "read back: unit", + expect: "fail", + leg: "read back", + clean: true, + }, + { + breaks: "read back: source", + expect: "fail", + leg: "read back", + clean: true, + }, { breaks: "delete", expect: "fail", leg: "delete" }, ]; @@ -583,19 +698,21 @@ async function selfTest() { }, (line) => lines.push(line), ); + const leftBehind = instance.rows.size; await instance.close(); const wanted = testCase.expect === "pass"; const legMatches = testCase.expect === "pass" || outcome.leg === testCase.leg; - if (outcome.ok === wanted && legMatches) { + const cleanEnough = !testCase.clean || leftBehind === 0; + if (outcome.ok === wanted && legMatches && cleanEnough) { console.log( - `self-test ok ${label.padEnd(20)} → ${outcome.ok ? "green" : `red on "${outcome.leg}"`}`, + `self-test ok ${label.padEnd(44)} → ${outcome.ok ? "green" : `red on "${outcome.leg}"`}${testCase.clean ? ", account clean" : ""}`, ); } else { failures += 1; console.log( - `self-test FAILED ${label.padEnd(20)} → ${outcome.ok ? "green" : `red on "${outcome.leg}"`}, expected ${testCase.expect}${testCase.leg ? ` on "${testCase.leg}"` : ""}`, + `self-test FAILED ${label.padEnd(44)} → ${outcome.ok ? "green" : `red on "${outcome.leg}"`}${cleanEnough ? "" : `, ${leftBehind} row(s) left behind`}, expected ${testCase.expect}${testCase.leg ? ` on "${testCase.leg}"` : ""}`, ); for (const line of lines) console.log(` | ${line}`); } From 3ba2fe3b8278e14969af3d677ea88e126fceb3e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Thu, 10 Sep 2026 01:26:24 +0200 Subject: [PATCH 3/4] fix(ops): prove the journey's reading is gone, not just accepted Low, review 2. The delete leg asserted HTTP 200 and printed "account left clean". An instance that answers 200 and keeps the row would have printed the same line, which is the failure mode the journey exists to catch. The leg now reads the id back and requires a 404. The self-test carries a mock that answers 200 without deleting; with the read-back removed that case goes green, which is the proof the check can fail. --- scripts/synthetic-journey.mjs | 31 ++++++++++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-) diff --git a/scripts/synthetic-journey.mjs b/scripts/synthetic-journey.mjs index dd28e5e65..d5ddfb732 100644 --- a/scripts/synthetic-journey.mjs +++ b/scripts/synthetic-journey.mjs @@ -24,7 +24,9 @@ * 3. write — `POST /api/measurements` with an `Idempotency-Key`. * 4. read — `GET /api/measurements?type=WEIGHT&limit=1` and assert * the row's id, value, unit and source. - * 5. delete — `DELETE /api/measurements/{id}` so the account stays clean. + * 5. delete — `DELETE /api/measurements/{id}`, then a direct `GET` on + * the same id that must 404, so the account stays clean and + * the leg proves it rather than trusting a 200. * * Legs 3 to 5 run inside a `try`/`finally`: from the write on, the run owns * a row on a real account, and a red read leg must not leave it behind. The @@ -340,6 +342,16 @@ async function removeReading(baseUrl, jar, writtenId) { jar, }); expectStatus(result, 200, "delete"); + // A 200 is the instance's claim, not the outcome. An instance that answers + // 200 and keeps the row would have printed "account left clean" — so read + // the id back and require the 404. + const gone = await call(baseUrl, `/api/measurements/${writtenId}`, { jar }); + if (gone.status !== 404) { + throw new LegFailure( + `delete: ${writtenId} still reads back after a 200 delete`, + { status: gone.status, body: gone.text }, + ); + } return { detail: "account left clean" }; } @@ -592,10 +604,26 @@ function startMockInstance(breaks) { if (breaks === "delete") { return send(404, { data: null, error: "Measurement not found" }); } + // 200 and the row still there — the status alone cannot catch this, + // only the read-back the delete leg does afterwards. + if (breaks === "delete: kept") { + return send(200, { data: { success: true }, error: null }); + } rows.delete(url.pathname.split("/").pop()); return send(200, { data: { success: true }, error: null }); } + if ( + url.pathname.startsWith("/api/measurements/") && + request.method === "GET" + ) { + const row = rows.get(url.pathname.split("/").pop()); + if (!row) { + return send(404, { data: null, error: "Measurement not found" }); + } + return send(200, { data: row, error: null }); + } + send(404, { data: null, error: "Not found" }); }); @@ -682,6 +710,7 @@ async function selfTest() { clean: true, }, { breaks: "delete", expect: "fail", leg: "delete" }, + { breaks: "delete: kept", expect: "fail", leg: "delete" }, ]; let failures = selfTestHostAllowlist(); From 79cb08efc7e61372a0e703498bd6b544941e912f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Thu, 10 Sep 2026 01:26:40 +0200 Subject: [PATCH 4/4] test(ops): prove the journey's redactor on a key-shaped body Low, review 2. The self-test walked every leg but never once asked whether the redactor actually holds. The excerpt path is the only thing between a token in a failure body and the run log, so it gets its own case: a body carrying this project's own key prefix must come back with the token replaced. --- scripts/synthetic-journey.mjs | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/scripts/synthetic-journey.mjs b/scripts/synthetic-journey.mjs index d5ddfb732..e70a5c4db 100644 --- a/scripts/synthetic-journey.mjs +++ b/scripts/synthetic-journey.mjs @@ -673,6 +673,22 @@ function selfTestHostAllowlist() { return failures; } +/** + * The excerpt path is what keeps a token out of a failure log. Prove it + * once, on a body shaped like this project's own key prefix. + */ +function selfTestRedaction() { + const shaped = `hlk_${"a".repeat(24)}`; + const redacted = excerpt(`{"error":"bad token","token":"${shaped}"}`); + const label = "redaction"; + if (redacted.includes("hlk_") || !redacted.includes("[redacted]")) { + console.log(`self-test FAILED ${label.padEnd(44)} → ${redacted}`); + return 1; + } + console.log(`self-test ok ${label.padEnd(44)} → ${redacted}`); + return 0; +} + async function selfTest() { const cases = [ { breaks: null, expect: "pass", clean: true }, @@ -713,7 +729,7 @@ async function selfTest() { { breaks: "delete: kept", expect: "fail", leg: "delete" }, ]; - let failures = selfTestHostAllowlist(); + let failures = selfTestHostAllowlist() + selfTestRedaction(); for (const testCase of cases) { const label = testCase.breaks ?? "nothing broken"; const instance = await startMockInstance(testCase.breaks); @@ -752,7 +768,7 @@ async function selfTest() { return 1; } console.log( - `\nself-test: ${cases.length} journey cases, every leg proven breakable; host allowlist proven`, + `\nself-test: ${cases.length} journey cases, every leg proven breakable; host allowlist and redaction proven`, ); return 0; }