mosaicstack/stack
25
0 Fork 1
Code Issues 404 Pull Requests 13 Packages Projects Releases 15 Wiki Activity

fix(org): applyPack requires the secret check on every write (starter packs SP3 security P2-1)

Browse Source 
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.
This commit was merged in pull request #1950.
This commit is contained in:
jason.woltje committed 2026-11-12 15:30:07 -06:00
1 parent 9cfb5f64fe
commit 1e65ff3f8b
2 files changed
+37 -29
Download Patch File Download Diff File Expand all files Collapse all files

No files matched your search

packages/organization/src/organization.mjs
+9 -9
View File
@@ -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");
packages/organization/tests/starter-pack-keys.test.mjs
+28 -20
View File
@@ -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.