From 1e65ff3f8b935af454571fb278ac5fa63a2496b4 Mon Sep 17 00:00:00 2001 From: Jason Woltje Date: Thu, 12 Nov 2026 15:29:59 -0600 Subject: [PATCH] fix(org): applyPack requires the secret check on every write (starter packs SP3 security P2-1) A board, workflow or pack write without assertClean now refuses definition-secret-check-required, and the scan covers every key and string in the plan before the writer reads any of it. --- packages/organization/src/organization.mjs | 18 +++---- .../tests/starter-pack-keys.test.mjs | 48 +++++++++++-------- 2 files changed, 37 insertions(+), 29 deletions(-) diff --git a/packages/organization/src/organization.mjs b/packages/organization/src/organization.mjs index cb557e484..ba895cd13 100644 --- a/packages/organization/src/organization.mjs +++ b/packages/organization/src/organization.mjs @@ -457,7 +457,9 @@ export function organizationWriter({ dir, rolesDir, dataRoot = null, barrier = ( // (human-required), a name an instance uses refuses definition-in-use, the credential store's // check is required and checkDefinition runs it with checkComposition. An agent never applies a // saved template either. The rest is organization.change's, with no decision (Jason, 11-12). - // assertClean, when given, also runs on every string in the plan. Each workflow written must name + // assertClean, the credential store's check, is required on every call and runs on every key and + // string in the plan before anything else reads it: workflow text reaches agents' prompts and a + // pack's inline proposals are role entries. Each workflow written must name // roles that resolve now: an org definition (after this plan), a V2 template, or human. The load // checks only their shape, so a later removal never fails the file. const PLAN_TOP = ["definitions", "board", "workflows", "packs"]; @@ -466,7 +468,13 @@ export function organizationWriter({ dir, rolesDir, dataRoot = null, barrier = ( const PLAN_NAME = /^[a-z][a-z0-9-]{0,63}$/; const planObject = (v) => v !== null && typeof v === "object" && !Array.isArray(v) && [Object.prototype, null].includes(Object.getPrototypeOf(v)); function applyPack(id, plan, { revision, actor, assertClean, detail }) { + if (typeof assertClean !== "function") fail("definition-secret-check-required", "a pack, board or workflow write needs the credential store's secret check; write it through the bus"); if (!planObject(plan) || Object.keys(plan).length === 0) fail("invalid-request", "a pack plan is an object naming at least one item"); + const scan = (value) => { + if (typeof value === "string") assertClean(value); + else if (value !== null && typeof value === "object") for (const [k, v] of Object.entries(value)) { assertClean(k); scan(v); } + }; + scan(plan); const projectKeys = []; Object.keys(plan).forEach((key, i) => { const match = PLAN_PROJECT.exec(key); @@ -487,13 +495,6 @@ export function organizationWriter({ dir, rolesDir, dataRoot = null, barrier = ( } else if (Object.hasOwn(plan, "definitions")) { fail("invalid-request", "definitions arrive only with a packs entry; write one definition through setDefinition"); } - if (typeof assertClean === "function") { - const scan = (value) => { - if (typeof value === "string") assertClean(value); - else if (value !== null && typeof value === "object") for (const [k, v] of Object.entries(value)) { assertClean(k); scan(v); } - }; - scan(plan); - } const at = new Date().toISOString(); const logged = { ...detail, verb: pack ? "pack.apply" : "organization.layout", pack: pack ? pack.id : null, added: null }; const build = (old) => { @@ -505,7 +506,6 @@ export function organizationWriter({ dir, rolesDir, dataRoot = null, barrier = ( const written = []; if (Object.hasOwn(plan, "definitions")) { if (!planObject(plan.definitions) || Object.keys(plan.definitions).length === 0) fail("invalid-request", "plan definitions must name at least one definition"); - if (typeof assertClean !== "function") fail("definition-secret-check-required", "a definition write needs the credential store's secret check; write it through the bus"); const definitions = { ...(old.definitions ?? {}) }; for (const [name, entry] of Object.entries(plan.definitions)) { checkDefinitionName(name, "plan definition name"); diff --git a/packages/organization/tests/starter-pack-keys.test.mjs b/packages/organization/tests/starter-pack-keys.test.mjs index 4b1d7ec00..15bd724cd 100644 --- a/packages/organization/tests/starter-pack-keys.test.mjs +++ b/packages/organization/tests/starter-pack-keys.test.mjs @@ -107,7 +107,7 @@ test("SP3 a definition a workflow names can still be removed, and the file then const scouting = { scouting: { steps: [{ title: "Scout", state: "todo", roles: ["scout", "human"] }] } }; const packs = { ...packPlan().packs, proposedHires: [] }; f.writer.applyPack("acme", { definitions: { scout: PLAIN }, workflows: scouting, packs }, { revision: rev(f), actor: "jason", assertClean }); - f.writer.applyPack("acme", { "projects.stack.workflows": scouting }, { revision: rev(f), actor: "jason" }); + f.writer.applyPack("acme", { "projects.stack.workflows": scouting }, { revision: rev(f), actor: "jason", assertClean }); assert.deepEqual(doc(f).definitions.scout, { ...PLAIN, rev: 1 }); f.writer.setDefinition("acme", "scout", () => null, { revision: rev(f), actor: "jason", assertClean }); const after = doc(f); @@ -119,7 +119,7 @@ test("SP3 a definition a workflow names can still be removed, and the file then assert.deepEqual(loaded.projects.stack.workflows, scouting); // The writer reads it back too; only a new write of that workflow resolves its roles again. assert.equal(f.writer.read("acme").revision, rev(f)); - assert.throws(() => f.writer.applyPack("acme", { workflows: { again: scouting.scouting } }, { revision: rev(f), actor: "jason" }), { code: "invalid-request" }); + assert.throws(() => f.writer.applyPack("acme", { workflows: { again: scouting.scouting } }, { revision: rev(f), actor: "jason", assertClean }), { code: "invalid-request" }); }); test("SP3 each invalid shape refuses", (t) => { @@ -219,7 +219,7 @@ test("SP3 applyPack writes definitions, board, workflows and the packs entry in assert.ok(!JSON.stringify(log).includes("Backlog")); // A second pack appends; the first entry stays. const second = { packs: { ...packPlan().packs, id: "review", proposedHires: [] }, workflows: { review: WORKFLOWS.change } }; - f.writer.applyPack("acme", second, { revision: rev(f), actor: "jason" }); + f.writer.applyPack("acme", second, { revision: rev(f), actor: "jason", assertClean }); assert.deepEqual(doc(f).packs.map((p) => p.id), ["coding", "review"]); assert.deepEqual(doc(f).packs[0], entry); assert.deepEqual(doc(f).packs[1].added, ["workflows.review"]); @@ -255,13 +255,13 @@ test("SP3 an agent actor with a definition in the plan refuses human-required; b } // A saved template carries invented roles; an agent never applies one. const saved = { packs: { ...packPlan().packs, origin: "saved", proposedHires: [] }, board: BOARD }; - assert.throws(() => f.writer.applyPack("acme", saved, { revision: rev(f), actor: "chief-of-staff.chief" }), { code: "human-required" }); + assert.throws(() => f.writer.applyPack("acme", saved, { revision: rev(f), actor: "chief-of-staff.chief", assertClean }), { code: "human-required" }); assert.equal(bytes(f), original); // The non-definition half of a pack, and a board write, need no human (Jason's answer 2). const half = { packs: { ...packPlan().packs, proposedHires: [{ name: "coder-1", pin: { template: "coder", sha256: HASH } }] }, workflows: WORKFLOWS }; - f.writer.applyPack("acme", half, { revision: rev(f), actor: "chief-of-staff.chief" }); + f.writer.applyPack("acme", half, { revision: rev(f), actor: "chief-of-staff.chief", assertClean }); assert.equal(doc(f).packs[0].by, "chief-of-staff.chief"); - f.writer.applyPack("acme", { board: BOARD }, { revision: rev(f), actor: "chief-of-staff.chief" }); + f.writer.applyPack("acme", { board: BOARD }, { revision: rev(f), actor: "chief-of-staff.chief", assertClean }); assert.deepEqual(doc(f).board, BOARD); }); @@ -292,32 +292,32 @@ test("SP3 a plan holding any other key refuses", (t) => { test("SP3 a board-only plan changes only board, and a project board only that project's board", (t) => { const f = fixture(t); const before = doc(f); - f.writer.applyPack("acme", { board: BOARD }, { revision: rev(f), actor: "jason" }); + f.writer.applyPack("acme", { board: BOARD }, { revision: rev(f), actor: "jason", assertClean }); assert.deepEqual(without(doc(f), "board"), before); assert.deepEqual(doc(f).board, BOARD); // board.set replaces a board, and null removes it. - f.writer.applyPack("acme", { board: LIST_BOARD }, { revision: rev(f), actor: "jason" }); + f.writer.applyPack("acme", { board: LIST_BOARD }, { revision: rev(f), actor: "jason", assertClean }); assert.deepEqual(doc(f).board, LIST_BOARD); - f.writer.applyPack("acme", { board: null }, { revision: rev(f), actor: "jason" }); + f.writer.applyPack("acme", { board: null }, { revision: rev(f), actor: "jason", assertClean }); assert.deepEqual(doc(f), before); // A project's board. - f.writer.applyPack("acme", { "projects.stack.board": BOARD }, { revision: rev(f), actor: "jason" }); + f.writer.applyPack("acme", { "projects.stack.board": BOARD }, { revision: rev(f), actor: "jason", assertClean }); assert.deepEqual(doc(f).projects.stack, { ...before.projects.stack, board: BOARD }); assert.deepEqual(without(doc(f), "projects"), without(before, "projects")); assert.deepEqual(f.writer.read("acme").document.projects.stack.board, BOARD); // Workflows merge by name; null removes one; a project's workflows likewise. - f.writer.applyPack("acme", { workflows: WORKFLOWS }, { revision: rev(f), actor: "jason" }); - f.writer.applyPack("acme", { workflows: { review: WORKFLOWS.change } }, { revision: rev(f), actor: "jason" }); + f.writer.applyPack("acme", { workflows: WORKFLOWS }, { revision: rev(f), actor: "jason", assertClean }); + f.writer.applyPack("acme", { workflows: { review: WORKFLOWS.change } }, { revision: rev(f), actor: "jason", assertClean }); assert.deepEqual(Object.keys(doc(f).workflows), ["change", "review"]); - f.writer.applyPack("acme", { workflows: { change: null } }, { revision: rev(f), actor: "jason" }); + f.writer.applyPack("acme", { workflows: { change: null } }, { revision: rev(f), actor: "jason", assertClean }); assert.deepEqual(Object.keys(doc(f).workflows), ["review"]); - f.writer.applyPack("acme", { workflows: { review: null } }, { revision: rev(f), actor: "jason" }); + f.writer.applyPack("acme", { workflows: { review: null } }, { revision: rev(f), actor: "jason", assertClean }); assert.ok(!Object.hasOwn(doc(f), "workflows")); - f.writer.applyPack("acme", { "projects.stack.workflows": WORKFLOWS }, { revision: rev(f), actor: "jason" }); + f.writer.applyPack("acme", { "projects.stack.workflows": WORKFLOWS }, { revision: rev(f), actor: "jason", assertClean }); assert.deepEqual(doc(f).projects.stack.workflows, WORKFLOWS); // Removing what isn't there refuses. - assert.throws(() => f.writer.applyPack("acme", { board: null }, { revision: rev(f), actor: "jason" }), { code: "invalid-request" }); - assert.throws(() => f.writer.applyPack("acme", { workflows: { nope: null } }, { revision: rev(f), actor: "jason" }), { code: "invalid-request" }); + assert.throws(() => f.writer.applyPack("acme", { board: null }, { revision: rev(f), actor: "jason", assertClean }), { code: "invalid-request" }); + assert.throws(() => f.writer.applyPack("acme", { workflows: { nope: null } }, { revision: rev(f), actor: "jason", assertClean }), { code: "invalid-request" }); }); test("SP3 a workflow in the plan must name an org definition, a V2 template or human", (t) => { @@ -325,12 +325,12 @@ test("SP3 a workflow in the plan must name an org definition, a V2 template or h const original = bytes(f); const flow = (roles) => ({ workflows: { change: { steps: [{ title: "Step", state: "todo", roles }] } } }); for (const roles of [["ghost-role"], ["pm", "ghost-role"], ["conductor-policy"]]) { - assert.throws(() => f.writer.applyPack("acme", flow(roles), { revision: rev(f), actor: "jason" }), { code: "invalid-request" }, roles.join()); - assert.throws(() => f.writer.applyPack("acme", { "projects.stack.workflows": flow(roles).workflows }, { revision: rev(f), actor: "jason" }), { code: "invalid-request" }); + assert.throws(() => f.writer.applyPack("acme", flow(roles), { revision: rev(f), actor: "jason", assertClean }), { code: "invalid-request" }, roles.join()); + assert.throws(() => f.writer.applyPack("acme", { "projects.stack.workflows": flow(roles).workflows }, { revision: rev(f), actor: "jason", assertClean }), { code: "invalid-request" }); } assert.equal(bytes(f), original); // A template, human, and a definition the same plan adds. - f.writer.applyPack("acme", flow(["pm", "human"]), { revision: rev(f), actor: "jason" }); + f.writer.applyPack("acme", flow(["pm", "human"]), { revision: rev(f), actor: "jason", assertClean }); f.writer.applyPack("acme", packPlan({ board: undefined, workflows: { scouting: { steps: [{ title: "Scout", state: "todo", roles: ["scout"] }] } } }), { revision: rev(f), actor: "jason", assertClean }); assert.deepEqual(Object.keys(doc(f).workflows), ["change", "scouting"]); }); @@ -342,7 +342,15 @@ test("SP3 applyPack keeps setDefinition's definition rules", (t) => { assert.throws(() => f.writer.applyPack("acme", plan({ scout: PLAIN }), { revision: rev(f), actor: "jason", assertClean }), { code: "definition-in-use" }); assert.throws(() => f.writer.applyPack("acme", plan({ spare: PLAIN }), { revision: rev(f), actor: "jason", assertClean }), { code: "definition-exists" }); assert.throws(() => f.writer.applyPack("acme", plan({ fresh: { ...PLAIN, rev: 1 } }), { revision: rev(f), actor: "jason", assertClean }), { code: "definition-rev-conflict" }); + // The credential store's check is required on every apply, not only one that writes definitions. assert.throws(() => f.writer.applyPack("acme", plan({ fresh: PLAIN }), { revision: rev(f), actor: "jason" }), { code: "definition-secret-check-required" }); + for (const bare of [{ board: BOARD }, { workflows: WORKFLOWS }, { "projects.stack.board": BOARD }, { packs: { ...packPlan().packs, proposedHires: [] }, workflows: WORKFLOWS }]) { + assert.throws(() => f.writer.applyPack("acme", bare, { revision: rev(f), actor: "jason" }), { code: "definition-secret-check-required" }, Object.keys(bare).join()); + } + // A secret in workflow text, a plan key or an inline proposal's entry refuses before anything is written. + const leak = { ...WORKFLOWS.change, description: SECRET }; + assert.throws(() => f.writer.applyPack("acme", { workflows: { change: leak } }, { revision: rev(f), actor: "jason", assertClean }), /credential-leak/); + assert.throws(() => f.writer.applyPack("acme", { [SECRET]: BOARD }, { revision: rev(f), actor: "jason", assertClean }), /credential-leak/); assert.throws(() => f.writer.applyPack("acme", plan({ fresh: { ...PLAIN, instructions: `token ${SECRET}\n` } }), { revision: rev(f), actor: "jason", assertClean }), /credential-leak/); assert.throws(() => f.writer.applyPack("acme", plan({ fresh: { ...PLAIN, role: { ...PLAIN.role, tools: ["no-such-tool"] } } }), { revision: rev(f), actor: "jason", assertClean }), { name: "BusinessError" }); // checkComposition runs: a hired composition never holds role.hire.