From 64c78d9a2ca0a2977720be681de59b8dec3ee64d Mon Sep 17 00:00:00 2001 From: Sourabh Choraria Date: Sat, 1 Aug 2026 00:06:31 +0100 Subject: [PATCH] test(dev): pin that the engine SHIPS KMS_MODE blank, and drop the IAM name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I merged #861 while the review still said BLOCK, having assumed its verdict was the previous round's. It was not: it had re-run and raised a different, correct finding, and I did not read it before merging. Both fixes are here. 1) The shipped DEFAULT was not pinned, only the enforcement. My tests covered that the four AWS fields are `parityRequired` and that only `KMS_MODE=local` relaxes them. None of that notices the manifest going back to `{ scope: "local", value: "local" }` — `pnpm dev:secrets` would then WRITE `KMS_MODE=local` into every .dev.vars, preflight would relax all four, and the engine would use the hermetic KEK again with all three tests still green. The regression lives in what we SHIP, not in what we enforce, and I had only guarded the second. Now pinned on both: the spec's scope and value, and the rendered example line (`KMS_MODE=`), because the rendered artifact is what actually reaches a developer's machine. ⚠️ My first mutation check "passed" and I nearly took that as proof the test was weak. It was the CHECK that was wrong: `s.index('name: "KMS_MODE",')` matched a `relaxedBy: { name: "KMS_MODE", ... }` reference seven lines earlier, so it mutated a different field's scope. Verifying the mutation actually mutated — printing the resulting spec — is what caught it. A mutation that does not mutate proves nothing in either direction. 2) The IAM principal name was in a public doc. `no-secrets` covers account identifiers, not only secrets, and this repo is public. The doc now says what the principal is scoped to without naming it; the name lives in the credential store. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01BRmGUnxeYsQoG9c8BCZcae --- docs/local-parity.md | 5 +++-- scripts/dev-preflight.test.mjs | 35 +++++++++++++++++++++++++++++++++- 2 files changed, 37 insertions(+), 3 deletions(-) diff --git a/docs/local-parity.md b/docs/local-parity.md index bdc4d352..cd923dfd 100644 --- a/docs/local-parity.md +++ b/docs/local-parity.md @@ -216,8 +216,9 @@ compliance-critical path running only in production. The page used to say this had "no alternative today: nobody should hold production AWS credentials to develop". The premise was wrong in the way this page warns about: **the credential already existed**. IAM -user `webhook-co-claude-code` is already least-privilege — `kms:GenerateDataKey` + `kms:Decrypt`, on one -key, and nothing else. AGENTS.md says to look for the real credential before inventing a substitute, and it +user is already least-privilege — `kms:GenerateDataKey` + `kms:Decrypt`, on one key, and nothing else +(the principal is named in the team's credential store, not here: this repo is public and the `no-secrets` +rule covers account identifiers, not just secrets). AGENTS.md says to look for the real credential before inventing a substitute, and it was there the whole time. Verified with a real round-trip: a DEK wrapped by the KEK, unwrapped, and the two proven identical by diff --git a/scripts/dev-preflight.test.mjs b/scripts/dev-preflight.test.mjs index d4d4db9a..1f4e4fd7 100644 --- a/scripts/dev-preflight.test.mjs +++ b/scripts/dev-preflight.test.mjs @@ -9,7 +9,8 @@ import { parseDevVars, requiredSpecs, } from "./dev-preflight.mjs"; -import { APP_NAMES } from "./dev-secrets-manifest.mjs"; +import { APP_NAMES, specsFor } from "./dev-secrets-manifest.mjs"; +import { renderExample } from "./dev-secrets.mjs"; // What this prevents: a clone with no `.dev.vars` boots, serves a login page that renders perfectly, // and simply offers fewer ways in — because the page derives its buttons from which OAuth secrets are @@ -242,3 +243,35 @@ test("KMS_MODE=local is the ONLY thing that relaxes them", () => { "an unrecognised value must not relax either", ); }); + +// The SHIPPED DEFAULT, which the tests above do not cover. +// +// They pin that the four AWS fields are required and that only `KMS_MODE=local` relaxes them. None of +// that notices if the manifest goes back to `{ scope: "local", value: "local" }`: `pnpm dev:secrets` would +// then WRITE `KMS_MODE=local` into every .dev.vars, preflight would relax all four, the engine would use +// the hermetic KEK again — and all three tests above would still pass. The regression is in what we ship, +// not in what we enforce. +test("the engine ships KMS_MODE BLANK — local is an opt-out, never the default", () => { + const spec = specsFor("engine").find((s) => s.name === "KMS_MODE"); + assert.ok(spec, "the engine no longer declares KMS_MODE at all"); + assert.equal( + spec.scope, + "external", + "a `local` scope makes the generator write a value, not a blank", + ); + assert.notEqual( + spec.value, + "local", + "shipping `local` puts every machine back on the throwaway KEK", + ); + + // The rendered artifact, not just the spec: this is what actually lands in a developer's .dev.vars. + const line = renderExample("engine") + .split("\n") + .find((l) => l.startsWith("KMS_MODE=")); + assert.equal( + line, + "KMS_MODE=", + `the generated example ships "${line}" — the substitute, by default`, + ); +});