From 3e82bc16c37c35a8422155b9bf43df0e3b2fac9f Mon Sep 17 00:00:00 2001 From: Jason Woltje Date: Tue, 10 Nov 2026 21:35:14 -0600 Subject: [PATCH 1/2] fix(refusals): restore validated identifiers already shown by the system (#1873) --- packages/cli/tests/last-values.test.mjs | 19 +++- packages/organization/src/access.mjs | 10 +-- packages/organization/src/role.mjs | 16 ++-- packages/organization/tests/access.test.mjs | 4 +- .../organization/tests/last-values.test.mjs | 17 ++-- packages/organization/tests/role.test.mjs | 14 +-- .../organization/tests/safe-names.test.mjs | 86 +++++++++++++++++++ packages/queue/src/queue.mjs | 42 ++++----- packages/queue/tests/data.test.mjs | 12 +-- packages/queue/tests/last-values.test.mjs | 64 +++++++++++++- packages/queue/tests/review.test.mjs | 30 +++---- packages/queue/tests/store.test.mjs | 6 +- scripts/mosaic-config.mjs | 2 +- scripts/mosaic-task.mjs | 4 +- scripts/test-config.sh | 2 +- 15 files changed, 247 insertions(+), 81 deletions(-) create mode 100644 packages/organization/tests/safe-names.test.mjs diff --git a/packages/cli/tests/last-values.test.mjs b/packages/cli/tests/last-values.test.mjs index 84a294c98..563d57602 100644 --- a/packages/cli/tests/last-values.test.mjs +++ b/packages/cli/tests/last-values.test.mjs @@ -58,18 +58,20 @@ test('project id mismatch omits the file value in config-command', t => { for (const [field, edit] of [ ['dataRoot', d => { d.dataRoot = `/x/${secret}/../y`; }], ['federation.peers[0].id', d => { d.federation = { self: secret.toLowerCase(), peers: [{ id: secret.toLowerCase(), origin: 'https://peer.example', label: 'Peer' }] }; }], -]) test(`config ${field} repeated/canonical value is omitted`, t => { +]) test(`config ${field} repeated/canonical value ${field.endsWith(".id") ? "names the checked id" : "is omitted"}`, t => { const root = scratch(t), f = fixture(root), d = JSON.parse(readFileSync(f.config, 'utf8')); edit(d); writeFileSync(f.config, JSON.stringify(d)); const r = spawnSync(process.execPath, [join(repo, 'scripts/mosaic-config.mjs'), 'validate'], { encoding: 'utf8', env: { ...f.env, PATH: process.env.PATH, HOME: process.env.HOME, TMPDIR: process.env.TMPDIR } }); - assert.equal(r.status, 2); assert.ok(r.stderr.includes(field), r.stderr); assert.doesNotMatch(r.stderr, /sk-test|NOT-A-SECRET/); + assert.equal(r.status, 2); assert.ok(r.stderr.includes(field), r.stderr); + if (field.endsWith(".id")) assert.ok(r.stderr.includes(secret.toLowerCase()), r.stderr); + else assert.doesNotMatch(r.stderr, /sk-test|NOT-A-SECRET/); }); -for (const mission of [false, true]) test(`duplicate ${mission ? 'mission' : 'task'} tool omits the validated value`, t => { +for (const mission of [false, true]) test(`duplicate ${mission ? 'mission' : 'task'} tool names the validated value`, t => { const root = scratch(t), d = task(); if (mission) { d.mission = 'mission.json'; writeFileSync(join(root, d.mission), JSON.stringify({ missionVersion: 1, id: 'valid', objective: 'hello', capabilities: { tools: ['read', 'read'] } })); } else d.capabilities = { tools: ['read', 'read'] }; const file = join(root, 'task.json'); writeFileSync(file, JSON.stringify(d)); const r = spawnSync(process.execPath, [join(repo, 'scripts/mosaic-task.mjs'), 'validate', file], { encoding: 'utf8', env: { PATH: process.env.PATH, HOME: process.env.HOME, TMPDIR: process.env.TMPDIR } }); - assert.equal(r.status, 2); assert.match(r.stderr, /duplicate tool in/); assert.doesNotMatch(r.stderr, /read/); + assert.equal(r.status, 2); assert.match(r.stderr, /duplicate tool in/); assert.match(r.stderr, /read/); }); for (const childStderr of [secret, `mosaic-config: execution.model must match ${secret}`]) test(`config-command does not relay arbitrary child stderr${childStderr.startsWith('mosaic-config:') ? ' after a known field' : ''}`, t => { const root = scratch(t), file = join(root, 'config.json'); @@ -89,3 +91,12 @@ test('CLI boot project id mismatch omits the configured value and keeps exit 3', writeFileSync(file, JSON.stringify({ projectVersion: 1, id: secret.toLowerCase() })); assert.throws(() => bootConfig({ system: loadSystem({ env: f.env }), businessId: 'acme', env: f.env }), safe(file, CliError, { exitCode: 3 }, secret.toLowerCase())); }); + +for (const mission of [false, true]) test(`duplicate ${mission ? 'mission' : 'task'} tool rejects an unknown value before naming it`, t => { + const root = scratch(t), d = task(); + if (mission) { d.mission = 'mission.json'; writeFileSync(join(root, d.mission), JSON.stringify({ missionVersion: 1, id: 'valid', objective: 'hello', capabilities: { tools: [secret, secret] } })); } + else d.capabilities = { tools: [secret, secret] }; + const file = join(root, 'task.json'); writeFileSync(file, JSON.stringify(d)); + const r = spawnSync(process.execPath, [join(repo, 'scripts/mosaic-task.mjs'), 'validate', file], { encoding: 'utf8', env: { PATH: process.env.PATH, HOME: process.env.HOME, TMPDIR: process.env.TMPDIR } }); + assert.equal(r.status, 2); assert.match(r.stderr, /unsupported.*tool/); assert.doesNotMatch(r.stderr, /sk-test|NOT-A-SECRET/); +}); diff --git a/packages/organization/src/access.mjs b/packages/organization/src/access.mjs index 6fa840c46..76c18a3c3 100644 --- a/packages/organization/src/access.mjs +++ b/packages/organization/src/access.mjs @@ -72,8 +72,8 @@ function checkUser(user, at, ctx, humans, seenSubjects) { requireObject(user, at); rejectUnknownKeys(user, ["id", "home", "subjects"], at); requireId(user.id, `${at}.id`); - if (RESERVED_USERS.includes(user.id)) refuse(`${at}.id must not be a bus principal`); - if (humans.has(user.id)) refuse(`${at}.id must not be a business's human`); + if (RESERVED_USERS.includes(user.id)) refuse(`${at}.id must not be ${JSON.stringify(user.id)}, a bus principal`); + if (humans.has(user.id)) refuse(`${at}.id must not be a business's human (${JSON.stringify(user.id)})`); if (user.home !== LOCAL_HOME && !ctx.peers.has(user.home)) { refuse(`${at}.home must be a configured peer or ${JSON.stringify(LOCAL_HOME)}`); } @@ -105,7 +105,7 @@ function checkScope(value, where, check) { const seen = new Set(); list.forEach((entry, i) => { check(entry, `${where}[${i}]`); - if (seen.has(entry)) refuse(`duplicate entry in ${where}`); + if (seen.has(entry)) refuse(`duplicate entry in ${where}: ${JSON.stringify(entry)}`); seen.add(entry); }); } @@ -122,7 +122,7 @@ function checkGrant(grant, at, ctx, users, seenGrants) { requireObject(grant, at); rejectUnknownKeys(grant, ["id", "user", "business", "projects", "instances", "level", "expires"], at); requireId(grant.id, `${at}.id`); - if (seenGrants.has(grant.id)) refuse(`${at}.id is a duplicate grant id`); + if (seenGrants.has(grant.id)) refuse(`${at}.id is a duplicate grant id: ${grant.id}`); seenGrants.add(grant.id); if (typeof grant.user !== "string" || !users.has(grant.user)) refuse(`${at}.user must name a user in this file`); if (typeof grant.business !== "string" || !ctx.businesses.has(grant.business)) { @@ -153,7 +153,7 @@ export function checkAccess(document, context) { const seenSubjects = new Set(); requireArray(document.users, "users").forEach((user, i) => { checkUser(user, `users[${i}]`, ctx, humans, seenSubjects); - if (users.has(user.id)) refuse(`users[${i}].id is a duplicate user id`); + if (users.has(user.id)) refuse(`users[${i}].id is a duplicate user id: ${user.id}`); users.add(user.id); }); const seenGrants = new Set(); diff --git a/packages/organization/src/role.mjs b/packages/organization/src/role.mjs index 837812ce0..488ef6c69 100644 --- a/packages/organization/src/role.mjs +++ b/packages/organization/src/role.mjs @@ -40,11 +40,11 @@ function checkAuthority(authority) { for (const key of ["withinRole", "crossRole"]) { lists[key] = requireDistinctList(authority[key], `role authority.${key}`, (action) => { if (!ACTIONS.includes(action)) refuse(`unknown action in authority.${key}; expected an action from the role vocabulary`); - if (GATED_ONLY.includes(action)) refuse(`authority.${key} lists an action which is always gated`); + if (GATED_ONLY.includes(action)) refuse(`authority.${key} lists ${action}, which is always gated`); }); } const both = lists.withinRole.filter((a) => lists.crossRole.includes(a)); - if (both.length > 0) refuse(`action listed as both withinRole and crossRole`); + if (both.length > 0) refuse(`action listed as both withinRole and crossRole: ${both.join(", ")}`); return lists; } @@ -87,13 +87,13 @@ function checkHireable(hireable, file) { const dir = dirname(file); return requireDistinctList(hireable, "role hireable", (name) => { requireId(name, "role hireable entry"); - if (CREATIVE_DEFINITIONS.includes(name)) refuse(`hireable names a creative definition only the human adds`); + if (CREATIVE_DEFINITIONS.includes(name)) refuse(`hireable names ${name}, a creative definition only the human adds`); const path = join(dir, `${name}.json`); if (!existsSync(path)) refuse(`hireable names a definition which has no role file`); const named = validateRoleDocument(readJsonFile(path, "role file"), path, { hireable: false }); if (named.roleVersion !== 2) refuse(`hireable names a definition which is role version 1`); const outside = unhireableAction(named); - if (outside !== null) refuse(`hireable names a definition which holds an action outside the hireable ceiling`); + if (outside !== null) refuse(`hireable names ${name}, which holds ${outside} outside the hireable ceiling`); }); } @@ -106,7 +106,7 @@ function checkGiteaScopes(scopes) { }, { nonEmpty: true }); const categories = list.map((s) => s.split(":")[1]); const twice = categories.filter((c, i) => categories.indexOf(c) !== i); - if (twice.length > 0) refuse(`gitea scopes name a category twice; a token holds one level per category`); + if (twice.length > 0) refuse(`gitea scopes name ${twice[0]} twice; a token holds one level per category`); return list; } @@ -135,7 +135,7 @@ function checkCredentials(credentials) { if (!host && !SERVICES.includes(entry.service)) { refuse(`role credentials[${i}].service must be one of: ${[...SERVICES, HOST_SERVICE].join(", ")}`); } - if (seen.has(entry.service)) refuse(`role credentials name a service twice`); + if (seen.has(entry.service)) refuse(`role credentials name ${entry.service} twice`); seen.add(entry.service); if (host) return { service: HOST_SERVICE }; const scopes = entry.service === "gitea" ? checkGiteaScopes(entry.scopes) : checkVikunjaScopes(entry.scopes); @@ -167,12 +167,12 @@ function checkContract(contract, file) { // docs: its authority is withinRole CREATIVE_ACTIONS at most, and its one credential is gitea. function checkCreative(role) { const wider = role.tools.filter((tool) => !CREATIVE_TOOLS.includes(tool)); - if (wider.length > 0) refuse(`creative role ${role.name} may not name tools outside the allowed set (allowed: ${CREATIVE_TOOLS.join(", ")})`); + if (wider.length > 0) refuse(`creative role ${role.name} may not name tools ${wider.join(", ")} (allowed: ${CREATIVE_TOOLS.join(", ")})`); if (role.authority.crossRole.length > 0 || role.authority.withinRole.some((a) => !CREATIVE_ACTIONS.includes(a))) { refuse(`creative role ${role.name} may hold only withinRole ${CREATIVE_ACTIONS.join(" and ")}`); } const services = role.credentials.map((c) => c.service).filter((service) => service !== CREATIVE_SERVICE); - if (services.length > 0) refuse(`creative role ${role.name} may hold only the ${CREATIVE_SERVICE} credential`); + if (services.length > 0) refuse(`creative role ${role.name} may hold only the ${CREATIVE_SERVICE} credential (got ${services.join(", ")})`); if (role.messaging === null) refuse(`creative role ${role.name} requires "messaging"`); for (const key of ["sendTo", "receiveFrom"]) { if (role.messaging[key].some((entry) => entry !== "human")) { diff --git a/packages/organization/tests/access.test.mjs b/packages/organization/tests/access.test.mjs index 28ade0aa0..4633786f9 100644 --- a/packages/organization/tests/access.test.mjs +++ b/packages/organization/tests/access.test.mjs @@ -108,8 +108,8 @@ test("users: unknown keys and the id rule", () => { }); test("users: human, tester and any business's human are refused", () => { - refuses(() => checkAccess(mutated((d) => { d.users[0].id = "human"; d.grants = []; }), context()), /users\[0\]\.id must not be a bus principal/); - refuses(() => checkAccess(mutated((d) => { d.users[1].id = "tester"; d.grants = []; }), context()), /users\[1\]\.id must not be a bus principal/); + refuses(() => checkAccess(mutated((d) => { d.users[0].id = "human"; d.grants = []; }), context()), /users\[0\]\.id must not be "human", a bus principal/); + refuses(() => checkAccess(mutated((d) => { d.users[1].id = "tester"; d.grants = []; }), context()), /users\[1\]\.id must not be "tester", a bus principal/); refuses(() => checkAccess(mutated((d) => { d.users[0].id = "melanie"; d.grants = []; }), context()), /users\[0\]\.id must not be a business's human/); // The human of a business the grant doesn't name is refused too. refuses(() => checkAccess(mutated((d) => { d.users[1].id = "jason"; d.grants = []; }), context()), /users\[1\]\.id must not be a business's human/); diff --git a/packages/organization/tests/last-values.test.mjs b/packages/organization/tests/last-values.test.mjs index abef521aa..9d91007d6 100644 --- a/packages/organization/tests/last-values.test.mjs +++ b/packages/organization/tests/last-values.test.mjs @@ -53,22 +53,29 @@ for (const [field, edit] of [ assert.throws(() => validateRoleDocument(d, join(root, 'r.json')), safe(field, BusinessError, { exitCode: 2 }, secret.toLowerCase())); }); // This branch is reachable only with already-validated service names; a sentinel service -// is refused earlier. Pin the same no-value rule with an admissible service. -test('creative service refusal omits the validated service list', t => { +// is refused earlier. A known service is already displayed in the role's rails. +test('creative service refusal names the validated service list', t => { const root = scratch(t); writeFileSync(join(root, 'companion.md'), '# R\n\n## Authority\n'); const d = { ...role(), name: 'companion', contract: 'companion.md', messaging: { sendTo: ['human'], receiveFrom: ['human'] }, credentials: [{ service: 'vikunja', scopes: { tasks: ['read_one'] } }] }; - assert.throws(() => validateRoleDocument(d, join(root, 'companion.json')), safe('credential', BusinessError, { exitCode: 2 }, 'vikunja')); + assert.throws(() => validateRoleDocument(d, join(root, 'companion.json')), named('credential', 'vikunja')); }); test('role duplicate skill omits the accepted list value', t => { const root = scratch(t); writeFileSync(join(root, 'r.md'), '# R\n\n## Authority\n'); const d = { ...role(), skills: [secret, secret] }; assert.throws(() => validateRoleDocument(d, join(root, 'r.json')), safe('role skills', BusinessError, { exitCode: 2 })); }); -for (const at of ['user', 'grant', 'scope', 'human']) test(`access ${at} duplicate/reserved refusal omits accepted ids`, () => { +for (const at of ['user', 'grant', 'scope', 'human']) test(`access ${at} duplicate/reserved refusal names accepted ids`, () => { const value = secret.toLowerCase(), d = access(), ctx = structuredClone(context); if (at === 'user') { d.users[0].id = value; d.users.push({ ...structuredClone(d.users[0]), subjects: [{ provider: 'test', sub: 'second' }] }); } if (at === 'grant') { d.grants[0].id = value; d.grants.push(structuredClone(d.grants[0])); } if (at === 'scope') { ctx.businesses.acme.instances = [value]; d.grants[0].instances = [value, value]; } if (at === 'human') { ctx.businesses.acme.human = value; d.users[0].id = value; } - assert.throws(() => checkAccess(d, ctx), safe(at === 'scope' ? 'instances' : '.id', BusinessError, { exitCode: 2 }, value)); + assert.throws(() => checkAccess(d, ctx), named(at === 'scope' ? 'instances' : '.id', value)); }); + +function named(label, value) { return error => { + assert.ok(error instanceof BusinessError); assert.equal(error.exitCode, 2); + assert.ok(error.message.includes(label), error.message); + assert.ok(error.message.includes(value), error.message); + return true; +}; } diff --git a/packages/organization/tests/role.test.mjs b/packages/organization/tests/role.test.mjs index ffc6d330d..18c3a8d55 100644 --- a/packages/organization/tests/role.test.mjs +++ b/packages/organization/tests/role.test.mjs @@ -189,7 +189,7 @@ test("credentials: Gitea scopes", () => { refuses(() => gitea(["write:admin"]), /unsupported gitea scope/); refuses(() => gitea(["all"]), /unsupported gitea scope/); refuses(() => gitea(["sudo:repository"]), /unsupported gitea scope/); - refuses(() => gitea(["read:issue", "write:issue"]), /name a category twice/); + refuses(() => gitea(["read:issue", "write:issue"]), /name issue twice/); refuses(() => gitea([]), /must not be empty/); }); @@ -255,7 +255,7 @@ test("M1: a creative role file wider than read and answer is refused", () => { }; assert.doesNotThrow(creative("ideas", { tools: ["read", "grep", "find", "ls", "web"] })); for (const tool of ["write", "edit", "bash"]) { - refuses(creative("ideas", { tools: ["read", tool] }), /^creative role ideas may not name tools outside the allowed set /); + refuses(creative("ideas", { tools: ["read", tool] }), /^creative role ideas may not name tools (?:write|edit|bash) \(allowed: /); } const gitea = [{ service: "gitea", scopes: ["read:user", "write:repository"] }]; assert.doesNotThrow(creative("ideas", { authority: { withinRole: ["message.send", "library.write"], crossRole: [] }, credentials: gitea })); @@ -268,9 +268,9 @@ test("M1: a creative role file wider than read and answer is refused", () => { refuses(creative("proofreader", { authority: { withinRole: ["message.send"], crossRole: ["library.write"] } }), only); // Row 285: library.write with any credential but gitea refuses. refuses(creative("ideas", { authority: { withinRole: ["message.send", "library.write"], crossRole: [] }, credentials: [...gitea, { service: "vikunja", scopes: { tasks: ["read_one"] } }] }), - /^creative role ideas may hold only the gitea credential$/); + /^creative role ideas may hold only the gitea credential \(got vikunja\)$/); refuses(creative("ideas", { authority: { withinRole: ["message.send", "library.write"], crossRole: [] }, credentials: [{ service: "host" }] }), - /^creative role ideas may hold only the gitea credential$/); + /^creative role ideas may hold only the gitea credential \(got host\)$/); refuses(creative("continuity", { messaging: undefined }), /^creative role continuity requires "messaging"$/); refuses(creative("research", { messaging: { sendTo: ["human", "pm"], receiveFrom: ["human"] } }), /^creative role research messaging.sendTo may name only human$/); refuses(creative("research", { messaging: { sendTo: ["human"], receiveFrom: ["assistant"] } }), /^creative role research messaging.receiveFrom may name only human$/); @@ -307,14 +307,14 @@ test("hireable: a list naming a definition that holds any refused action refuses for (const action of REFUSED_FOR_HIRE) { for (const key of ["withinRole", "crossRole"]) { const dir = hireDir({ chief: { hireable: ["worker"] }, worker: { [key]: [action], ...(key === "crossRole" ? { withinRole: [] } : {}) } }); - refuses(() => loadRole(dir, "chief"), /hireable names a definition which holds an action outside the hireable ceiling$/); + refuses(() => loadRole(dir, "chief"), /hireable names worker, which holds [a-z.]+ outside the hireable ceiling$/); } } }); test("hireable: a mixed definition refuses, a clean list loads, and the named file's own hireable isn't read", () => { const mixed = hireDir({ chief: { hireable: ["worker"] }, worker: { withinRole: ["message.send", "role.launch"] } }); - refuses(() => loadRole(mixed, "chief"), /hireable names a definition which holds an action outside the hireable ceiling/); + refuses(() => loadRole(mixed, "chief"), /hireable names worker, which holds [a-z.]+ outside the hireable ceiling/); const clean = hireDir({ chief: { hireable: ["worker", "helper"] }, worker: { withinRole: [...HIRE_ALLOWED] }, @@ -331,7 +331,7 @@ test("hireable: a missing, version 1, duplicate or badly named entry refuses, an refuses(() => loadRole(hireDir({ chief: { hireable: ["old"] }, old: 1 }), "chief"), /hireable names a definition which is role version 1/); // Row 213 (M10): a creative definition refuses by name, before its file is read. for (const name of CREATIVE_DEFINITIONS) { - refuses(() => loadRole(hireDir({ chief: { hireable: [name] } }), "chief"), /hireable names a creative definition only the human adds$/); + refuses(() => loadRole(hireDir({ chief: { hireable: [name] } }), "chief"), /hireable names [a-z-]+, a creative definition only the human adds$/); } refuses(() => loadRole(hireDir({ chief: { hireable: ["w", "w"] }, w: {} }), "chief"), /duplicate entry in role hireable/); refuses(() => loadRole(hireDir({ chief: { hireable: ["../w"] } }), "chief"), /role hireable entry/); diff --git a/packages/organization/tests/safe-names.test.mjs b/packages/organization/tests/safe-names.test.mjs new file mode 100644 index 000000000..2e7d40908 --- /dev/null +++ b/packages/organization/tests/safe-names.test.mjs @@ -0,0 +1,86 @@ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { mkdtempSync, writeFileSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { validateRoleDocument } from '../src/role.mjs'; +import { checkAccess } from '../src/access.mjs'; +import { BusinessError } from '../src/errors.mjs'; + +const bad = 'sk-test-NOT-A-SECRET-1234'; +const badId = `${bad.toLowerCase()} "`; +function refusal(fn, names = [], omitted = bad) { + assert.throws(fn, error => { + assert.ok(error instanceof BusinessError); + assert.equal(error.exitCode, 2); + for (const name of names) assert.ok(error.message.includes(name), error.message); + assert.ok(!error.message.includes(omitted), error.message); + return true; + }); +} +function role(t, edit = () => {}) { + const root = mkdtempSync(join(tmpdir(), 'names-')); + t.after(() => rmSync(root, { recursive: true, force: true })); + writeFileSync(join(root, 'r.md'), '# R\n\n## Authority\n'); + writeFileSync(join(root, 'companion.md'), '# R\n\n## Authority\n'); + const doc = { roleVersion: 2, name: 'r', title: 'R', contract: 'r.md', tools: ['read'], network: 'none', + authority: { withinRole: [], crossRole: [] }, credentials: [] }; + edit(doc, root); + return () => validateRoleDocument(doc, join(root, `${doc.name}.json`)); +} + +for (const key of ['withinRole', 'crossRole']) test(`gated ${key} names the known action; unknown action stays unnamed`, t => { + refusal(role(t, d => { d.authority[key] = ['deploy']; }), ['authority.' + key, 'deploy']); + refusal(role(t, d => { d.authority[key] = [bad]; })); +}); +test('overlapping authority lists name every known action; invalid entries stop first', t => { + refusal(role(t, d => { d.authority = { withinRole: ['message.send', 'record.write'], crossRole: ['message.send', 'record.write'] }; }), ['message.send', 'record.write']); + refusal(role(t, d => { d.authority = { withinRole: [bad], crossRole: [bad] }; })); +}); +for (const kind of ['creative', 'outside']) test(`hireable ${kind} names a known definition; invalid name stops first`, t => { + const edit = (d, root) => { + const name = kind === 'creative' ? 'companion' : 'helper'; + d.hireable = [name]; + if (kind === 'outside') writeFileSync(join(root, 'helper.json'), JSON.stringify({ ...d, name, hireable: [], authority: { withinRole: ['role.hire'], crossRole: [] } })); + }; + refusal(role(t, edit), kind === 'creative' ? ['companion'] : ['helper', 'role.hire']); + refusal(role(t, d => { d.hireable = [badId]; }), [], badId); +}); +test('hireable wider action rejects unknown action before naming it', t => { + refusal(role(t, (d, root) => { + d.hireable = ['helper']; + writeFileSync(join(root, 'helper.json'), JSON.stringify({ ...d, name: 'helper', hireable: [], authority: { withinRole: [bad], crossRole: [] } })); + })); +}); +test('gitea category conflict names the checked category; invalid scope stops first', t => { + refusal(role(t, d => { d.credentials = [{ service: 'gitea', scopes: ['read:issue', 'write:issue'] }]; }), ['issue']); + refusal(role(t, d => { d.credentials = [{ service: 'gitea', scopes: [bad] }]; })); +}); +test('duplicate credential names the checked service; invalid service stops first', t => { + refusal(role(t, d => { d.credentials = [{ service: 'gitea', scopes: ['read:issue'] }, { service: 'gitea', scopes: ['read:issue'] }]; }), ['gitea']); + refusal(role(t, d => { d.credentials = [{ service: bad }, { service: bad }]; })); +}); +for (const field of ['tools', 'credentials']) test(`creative ${field} names the checked wider grant; invalid grant stops first`, t => { + const edit = d => { d.name = 'companion'; d.contract = 'companion.md'; d.messaging = { sendTo: ['human'], receiveFrom: ['human'] }; }; + refusal(role(t, d => { edit(d); if (field === 'tools') d.tools.push('bash'); else d.credentials = [{ service: 'vikunja', scopes: { tasks: ['read_one'] } }]; }), [field === 'tools' ? 'bash' : 'vikunja']); + refusal(role(t, d => { edit(d); if (field === 'tools') d.tools = [bad]; else d.credentials = [{ service: bad }]; })); +}); + +const context = () => ({ providers: ['test'], peers: [], businesses: { acme: { human: 'jason', instances: ['coder'] } }, ssoSubjects: [] }); +const access = () => ({ accessVersion: 1, revision: 1, + users: [{ id: 'bean', home: 'local', subjects: [{ provider: 'test', sub: 'one' }] }], + grants: [{ id: 'grant', user: 'bean', business: 'acme', projects: ['*'], instances: ['coder'], level: 'viewer' }] }); +for (const kind of ['reserved', 'human', 'user', 'grant', 'project', 'instance']) test(`access ${kind} conflict names checked id; malformed replacement stops first`, () => { + function run(value) { + const d = access(), ctx = context(); + if (kind === 'reserved' || kind === 'human') d.users[0].id = value; + if (kind === 'user') d.users.push({ ...structuredClone(d.users[0]), id: value, subjects: [{ provider: 'test', sub: 'two' }] }); + if (kind === 'grant') { d.grants[0].id = value; d.grants.push(structuredClone(d.grants[0])); } + if (kind === 'project') d.grants[0].projects = [value, value]; + if (kind === 'instance') d.grants[0].instances = [value, value]; + return () => checkAccess(d, ctx); + } + const value = { reserved: 'human', human: 'jason', user: 'bean', grant: 'g-known-1', project: '11111111-2222-4333-8444-555555555555', instance: 'coder' }[kind]; + refusal(run(value), [value]); + refusal(run(badId), [], badId); +}); diff --git a/packages/queue/src/queue.mjs b/packages/queue/src/queue.mjs index 36ad2fe00..7d2b8293f 100644 --- a/packages/queue/src/queue.mjs +++ b/packages/queue/src/queue.mjs @@ -346,7 +346,7 @@ export function validateRow(row) { } if (row.claim !== null) { keysExactly(row.claim, ["seat", "op"], `${w} claim`); - if (row.claim.seat !== row.owner) throw refuse(`${w} claim seat is not the owner`); + if (row.claim.seat !== row.owner) throw refuse(`${w} claim seat is not the owner ${row.owner}`); checkCallerOpId(row.claim.op, `${w} claim op`); const s = row.state === "blocked" ? row.previousState : row.state; if (!CLAIM_KEPT.has(s)) throw refuse(`${w} is claimed but ${row.state}`); @@ -517,8 +517,8 @@ const isPriv = (by) => PRIVILEGED.has(by); function ownerOrPriv(row, by, doing) { if (by === row.owner || isPriv(by)) return; - const claimed = row.claim !== null ? `row ${row.id} is claimed; ` : ""; - throw refuse(`${claimed}only the owner or a privileged actor may ${doing} row ${row.id}`); + const claimed = row.claim !== null ? `row ${row.id} is claimed by ${row.claim.seat}; ` : ""; + throw refuse(`${claimed}only the owner (${row.owner}) or a privileged actor may ${doing} row ${row.id}`); } function requirePriv(by, doing) { @@ -580,7 +580,7 @@ function unresolvedAttempts(row) { function refuseUnresolved(row, doing) { const open = unresolvedAttempts(row); - if (open.length) throw refuse(`row ${row.id} has an unresolved review request; resolve or abandon it before ${doing}`); + if (open.length) throw refuse(`row ${row.id} has an unresolved review request: ${open.join(", ")}; resolve or abandon it before ${doing}`); } // A comment round leaves in-review, for done or for Jason's gate, only on @@ -589,7 +589,7 @@ function requireApprovals(row, cur, to) { if (row.reviewers.length === 0) throw refuse(`row ${row.id} lists no reviewers now; a privileged actor sets them before it ${to === "done" ? "closes" : `moves to ${to}`}`); const approved = new Set(cur.receipts.filter((r) => r.verdict === "approve").map((r) => r.reviewer)); const missing = row.reviewers.filter((s) => !approved.has(s)); - if (missing.length) throw refuse(`row ${row.id} round ${cur.n} has no approval recorded by every listed reviewer`); + if (missing.length) throw refuse(`row ${row.id} round ${cur.n} has no approval recorded by ${missing.join(", ")}`); } // The request round an attempt belongs to, and the attempt. @@ -645,7 +645,7 @@ function applyReview(rows, row, entry) { const cur = currentCommentRound(row, "review request"); const live = cur.attempts.filter((x) => x.state !== "failed" && x.state !== "abandoned"); refuseUnresolved(row, "a new request"); - if (live.length) throw refuse(`row ${row.id} round ${cur.n} already has a posted request`); + if (live.length) throw refuse(`row ${row.id} round ${cur.n} already has a posted request (${live[0].op}, comment ${live[0].comment})`); const rounds = row.review.rounds.map((r) => (r.n === cur.n ? { ...r, attempts: [...r.attempts, newAttempt(entry)] } : r)); return { row: { ...row, review: { rounds } }, @@ -657,8 +657,8 @@ function applyReview(rows, row, entry) { if (entry.op !== `${a.attempt}.outcome`) throw refuse(`an outcome's op is its attempt's op plus .outcome`); const found = findAttempt(row, a.attempt); const att = found.attempt; - if (by !== att.by) throw refuse(`only the actor who made the request records its outcome`); - if (att.transport !== null) throw refuse(`request already has its outcome`); + if (by !== att.by) throw refuse(`only ${att.by}, who made request ${att.op}, records its outcome`); + if (att.transport !== null) throw refuse(`request ${att.op} already has its outcome`); const t = { outcome: a.outcome, status: a.status, comment: a.comment, detail: a.detail }; const state = outcomeState(att, t); const comment = state === "posted" ? (att.comment ?? t.comment) : null; @@ -678,7 +678,7 @@ function applyReview(rows, row, entry) { else requirePriv(by, "abandon a review request"); const found = findAttempt(row, a.attempt); const att = found.attempt; - if (!UNRESOLVED.has(att.state)) throw refuse(`request is ${att.state}; only a requesting, uncertain or conflict request can be ${resolve ? "resolved" : "abandoned"}`); + if (!UNRESOLVED.has(att.state)) throw refuse(`request ${att.op} is ${att.state}; only a requesting, uncertain or conflict request can be ${resolve ? "resolved" : "abandoned"}`); const res = { verb: resolve ? "resolve" : "abandon", op: entry.op, by, at: entry.at, comment: resolve ? a.comment : null, reason: resolve ? null : a.reason }; const state = resolve ? "posted" : "abandoned"; const next = withAttempt(row, found, { ...att, state, comment: resolve ? a.comment : null, resolutions: [...att.resolutions, res] }, resolve ? {} : { duplicateRisk: true }); @@ -689,11 +689,11 @@ function applyReview(rows, row, entry) { }; } case "review-record": { - if (!row.reviewers.includes(by) || by === row.owner) throw refuse(`only a listed reviewer other than the owner may record a verdict on row ${row.id}`); + if (!row.reviewers.includes(by) || by === row.owner) throw refuse(`only a listed reviewer other than the owner may record a verdict on row ${row.id} (reviewers: ${fmt(row.reviewers)})`); if (!row.review) throw refuse(`row ${row.id} has no review round`); const cur = currentCommentRound(row, "review record"); if (a.candidate !== cur.candidate.digest) throw refuse(`candidate is not round ${cur.n}'s candidate`); - if (cur.receipts.some((r) => r.reviewer === by)) throw refuse(`reviewer already recorded a verdict for row ${row.id} round ${cur.n}`); + if (cur.receipts.some((r) => r.reviewer === by)) throw refuse(`${by} already recorded a verdict for row ${row.id} round ${cur.n}`); const rec = { reviewer: by, op: entry.op, at: entry.at, verdict: a.verdict, comment: a.comment, candidate: a.candidate }; const rounds = row.review.rounds.map((r) => (r.n === cur.n ? { ...r, receipts: [...r.receipts, rec] } : r)); return { @@ -759,14 +759,14 @@ function applyMove(rows, row, entry, resolved) { } else if (from === "queued" && to === "briefed") { requirePriv(by, "accept a brief (queued→briefed)"); } else if (from === "briefed" && to === "in-progress") { - if (by !== row.owner) throw refuse(`only the owner may start row ${row.id}`); + if (by !== row.owner) throw refuse(`only the owner (${row.owner}) may start row ${row.id}`); const missing = afterSatisfied(rows, row); if (missing.length) throw refuse(`row ${row.id} waits on ${missing.join(", ")}`); next.claim = { seat: row.owner, op: entry.op }; } else if (from === "in-progress" && to === "briefed") { throw refuse(`use \`queue release ${row.id}\` to give up a claim`); } else if (from === "in-progress" && to === "in-review") { - if (row.claim === null || by !== row.claim.seat) throw refuse(`only the claimant may request review of row ${row.id}`); + if (row.claim === null || by !== row.claim.seat) throw refuse(`only the claimant (${row.claim?.seat ?? "nobody"}) may request review of row ${row.id}`); if (candidate === null) throw refuse("in-progress→in-review needs --candidate "); revIssue = reviewIssue(row, issue); cand = checkCandidate(resolved.candidate); @@ -797,7 +797,7 @@ function applyMove(rows, row, entry, resolved) { next.claim = null; } else if (from === "in-review" && to === "done") { if (row.gateOwner === "jason") throw refuse(`row ${row.id}'s gate is Jason's; it goes through waiting-on-jason`); - if (by !== row.gateOwner && !isPriv(by)) throw refuse(`only the gate owner or a privileged actor may close row ${row.id}`); + if (by !== row.gateOwner && !isPriv(by)) throw refuse(`only the gate owner (${row.gateOwner}) or a privileged actor may close row ${row.id}`); refuseUnresolved(row, "closing it"); const cur = row.review?.rounds.at(-1); if (cur?.request === "comment") { @@ -834,7 +834,7 @@ function applySet(rows, row, entry, resolved) { switch (field) { case "piece": case "gate": case "reviewers": case "issues": requirePriv(by, `change ${field}`); - if (field === "reviewers" && value.includes(row.owner)) throw refuse(`row ${row.id} owner can't be a listed reviewer`); + if (field === "reviewers" && value.includes(row.owner)) throw refuse(`row ${row.id} owner ${row.owner} can't be a listed reviewer`); next[field] = value; // N10: closes follows the issues only if nobody narrowed it. A logged // narrowing keeps its intersection with the new issues. @@ -908,7 +908,7 @@ export function applyEntry(state, entry, resolved) { const id = highWater + 1; const owner = a.owner ?? by; const reviewers = a.reviewers ?? []; - if (reviewers.includes(owner)) throw refuse(`row ${id} owner can't be a listed reviewer`); + if (reviewers.includes(owner)) throw refuse(`row ${id} owner ${owner} can't be a listed reviewer`); highWater = id; const required = a.required ?? false; const row = { @@ -947,7 +947,7 @@ export function applyEntry(state, entry, resolved) { case "release": { const row = getRow(rows, a.id); if (row.state !== "in-progress") throw refuse(`row ${row.id} is ${row.state}; release applies to in-progress rows`); - if (!(row.claim && by === row.claim.seat) && !isPriv(by)) throw refuse(`only the claimant or a privileged actor may release row ${row.id}`); + if (!(row.claim && by === row.claim.seat) && !isPriv(by)) throw refuse(`only the claimant (${row.claim?.seat ?? "nobody"}) or a privileged actor may release row ${row.id}`); rows.set(row.id, touch({ ...row, state: "briefed", claim: null }, entry)); result = { row: row.id, from: "in-progress", to: "briefed", receipt: receipt(entry, rev, `row ${row.id} in-progress→briefed`) }; break; @@ -956,8 +956,8 @@ export function applyEntry(state, entry, resolved) { const row = getRow(rows, a.id); requirePriv(by, "assign a row"); if (row.state === "done") throw refuse(`row ${row.id} is done; done rows never change`); - if (row.owner === a.seat) throw refuse(`row ${row.id} is already owned by that seat`); - if (row.reviewers.includes(a.seat)) throw refuse(`can't assign row ${row.id}: seat is one of its reviewers`); + if (row.owner === a.seat) throw refuse(`row ${row.id} is already owned by ${row.owner}`); + if (row.reviewers.includes(a.seat)) throw refuse(`can't assign row ${row.id}: ${a.seat} is one of its reviewers`); const claim = row.claim ? { seat: a.seat, op: entry.op } : null; rows.set(row.id, touch({ ...row, owner: a.seat, claim }, entry)); result = { row: row.id, field: "owner", from: row.owner, to: a.seat, receipt: receipt(entry, rev, `row ${row.id} owner: ${row.owner}→${a.seat}`) }; @@ -966,7 +966,7 @@ export function applyEntry(state, entry, resolved) { case "note": { const row = getRow(rows, a.id); if (row.state === "done" || row.state === "parked") throw refuse(`row ${row.id} is ${row.state}; notes are closed`); - if (by !== row.owner && !row.reviewers.includes(by) && !isPriv(by)) throw refuse(`only the owner, a listed reviewer or a privileged actor may note row ${row.id}`); + if (by !== row.owner && !row.reviewers.includes(by) && !isPriv(by)) throw refuse(`only the owner (${row.owner}), a listed reviewer or a privileged actor may note row ${row.id}`); // On a blocked row the note is the reason (8.7: "update the reason // with note"); elsewhere it is the row's note, and "" clears it. const field = row.state === "blocked" ? "blockedReason" : "note"; @@ -1135,7 +1135,7 @@ export function loadDoc(bytes) { doc.log.forEach(checkEntryShape); const ops = new Set(); for (const e of doc.log) { - if (ops.has(e.op)) throw refuse(`op id appears twice in the log`); + if (ops.has(e.op)) throw refuse(`op id ${e.op} appears twice in the log`); ops.add(e.op); } if (doc.revision !== doc.log.length - 1) throw refuse(`revision is not the last rev ${doc.log.length - 1}`); diff --git a/packages/queue/tests/data.test.mjs b/packages/queue/tests/data.test.mjs index 5669d4330..bb49160a6 100644 --- a/packages/queue/tests/data.test.mjs +++ b/packages/queue/tests/data.test.mjs @@ -119,8 +119,8 @@ test("add: defaults for an ordinary seat, privileged extras, refusals", () => { refused(() => step(d, "add", { ...add, owner: "rocko" }, "dewey", { brief: brief() }), /only a privileged actor may set owner/); refused(() => step(d, "add", { ...add, reviewers: ["rocko"] }, "dewey", { brief: brief() }), /reviewers/); refused(() => step(d, "add", { ...add, required: true }, "dewey", { brief: brief() }), /required/); - refused(() => step(d, "add", { ...add, owner: "rocko", reviewers: ["rocko"] }, "sage", { brief: brief() }), /row 12 owner can't be a listed reviewer/); - refused(() => step(d, "add", { ...add, reviewers: ["sage"] }, "sage", { brief: brief() }), /row 12 owner can't be a listed reviewer/); + refused(() => step(d, "add", { ...add, owner: "rocko", reviewers: ["rocko"] }, "sage", { brief: brief() }), /row 12 owner rocko can't be a listed reviewer/); + refused(() => step(d, "add", { ...add, reviewers: ["sage"] }, "sage", { brief: brief() }), /row 12 owner sage can't be a listed reviewer/); const d2 = step(d, "add", { ...add, owner: "rocko", gateOwner: "filbert", after: [{ id: 9, when: "done" }], reviewers: ["dewey"], required: true }, "sage", { brief: brief() }); const r2 = row(d2, 12); assert.deepEqual([r2.owner, r2.gateOwner, r2.required, r2.after], ["rocko", "filbert", true, [{ id: 9, when: "done" }]]); @@ -134,7 +134,7 @@ test("matrix: queued→briefed privileged; briefed→in-progress owner with afte let d = step(genesisDoc(), "add", { piece: "N", gate: "g", brief: "docs/plans/brief-b.md#Queue" }, "dewey", { brief: brief() }); refused(() => step(d, "move", mv(12, "briefed"), "dewey"), /privileged/); d = step(d, "move", mv(12, "briefed"), "sage"); - refused(() => step(d, "move", mv(12, "in-progress"), "sage"), /only the owner may start/); + refused(() => step(d, "move", mv(12, "in-progress"), "sage"), /only the owner \(dewey\) may start/); d = step(d, "move", mv(12, "in-progress"), "dewey"); assert.deepEqual(row(d, 12).claim.seat, "dewey"); refused(() => step(d, "move", mv(12, "in-progress"), "dewey"), /not a transition/); @@ -391,8 +391,8 @@ test("field edits: who may change what", () => { const set = (id, field, value, reason = null) => ({ id, field, value, reason }); refused(() => step(d, "set", set(11, "piece", "x"), "dewey"), /privileged/); assert.match(step(d, "set", set(11, "piece", "Renamed"), "sage").log.at(-1).result.receipt, /row 11 piece: Brief template→Renamed$/); - refused(() => step(d, "set", set(9, "reviewers", ["darkwing"]), "sage"), /row 9 owner can't be a listed reviewer/); - refused(() => step(d, "set", set(9, "reviewers", ["filbert", "darkwing"]), "sage"), /row 9 owner can't be a listed reviewer/); + refused(() => step(d, "set", set(9, "reviewers", ["darkwing"]), "sage"), /row 9 owner darkwing can't be a listed reviewer/); + refused(() => step(d, "set", set(9, "reviewers", ["filbert", "darkwing"]), "sage"), /row 9 owner darkwing can't be a listed reviewer/); assert.deepEqual(row(step(d, "set", set(9, "reviewers", ["dewey"]), "sage"), 9).reviewers, ["dewey"]); refused(() => step(d, "set", set(9, "after", []), "sage"), /only jason may change after on a required row/); step(d, "set", set(9, "after", []), "jason"); @@ -586,7 +586,7 @@ test("note: owner, listed reviewer or privileged; empty clears", () => { test("assign moves the claim with the owner; done clears it", () => { let d = row9Started(); refused(() => step(d, "assign", { id: 9, seat: "dewey" }, "darkwing"), /privileged/); - refused(() => step(d, "assign", { id: 9, seat: "filbert" }, "sage"), /can't assign row 9: seat is one of its reviewers/); + refused(() => step(d, "assign", { id: 9, seat: "filbert" }, "sage"), /can't assign row 9: filbert is one of its reviewers/); d = step(d, "assign", { id: 9, seat: "dewey" }, "sage", {}, "assign-row-9-dewey"); assert.deepEqual(row(d, 9).claim, { seat: "dewey", op: "assign-row-9-dewey" }); assert.match(d.log.at(-1).result.receipt, /owner: darkwing→dewey$/); diff --git a/packages/queue/tests/last-values.test.mjs b/packages/queue/tests/last-values.test.mjs index 0a4c0f378..14a01ad34 100644 --- a/packages/queue/tests/last-values.test.mjs +++ b/packages/queue/tests/last-values.test.mjs @@ -72,6 +72,7 @@ function operation(verb, args, edit = () => {}, options = {}) { return applyEntry(state, entry, options.resolved ?? {}); } const ownerValue = row => { row.owner = seatValue; if (row.claim) row.claim.seat = seatValue; }; +const restored = new Set(['claimed owner authority', 'start owner', 'review claimant', 'close gate owner', 'set owner reviewer', 'release claimant', 'assign existing owner', 'assign reviewer', 'note owner', 'add owner reviewer', 'unresolved request', 'missing approval', 'posted request', 'outcome actor', 'duplicate outcome', 'settled resolution', 'reviewer list', 'duplicate verdict']); for (const [name, field, fn, omitted = seatValue] of [ ['canonical duplicate reviewer', 'repeated value', () => canonArgs('set', { id: 6, field: 'reviewers', value: [seatValue, seatValue] })], ['canonical state', 'state', () => canonArgs('move', { id: 6, to: secret }), secret], @@ -112,7 +113,7 @@ for (const [name, field, fn, omitted = seatValue] of [ // outcomeState also defends direct callers with an inconsistent transport record. ['inconsistent outcome record', 'outcome', () => operation('review-outcome', { attempt: seatValue, outcome: 'posted', status: 201, comment: 1, detail: 'fixture' }, r => reviewRow(r, 'failed'), { by: 'darkwing', op: seatValue + '.outcome', unchecked: true })], -]) test(`queue replay refusal ${name} omits stored values`, () => assert.throws(fn, safe(field, QueueError, { code: 2 }, omitted))); +]) test(`queue replay refusal ${name} ${restored.has(name) ? 'names validated identifiers' : 'omits stored values'}`, () => assert.throws(fn, restored.has(name) ? named(field, omitted, name === 'posted request' ? 'comment 1' : null) : safe(field, QueueError, { code: 2 }, omitted))); test('migration map shape omits an unchecked row id', () => assert.throws(() => parseMigrationMap(mapText([{ id: secret }])), safe('migration map row', QueueError, { code: 2 }))); test('migration map brief shape omits an unchecked row id', () => { @@ -127,3 +128,64 @@ test('BASE control: genesis expected claim already omits the stored op', () => { const d = doc(); d.log[0].op = seatValue; d.log[0].result.receipt = genesisReceipt(seatValue, d.rows.length); assert.throws(() => loadDoc(Buffer.from(serialize(d))), safe('claim', QueueError, { code: 2 }, seatValue)); }); + +function named(label, value, additional = null) { return error => { + assert.ok(error instanceof QueueError); assert.equal(error.code, 2); + assert.ok(error.message.includes(label), error.message); + assert.ok(error.message.includes(value), error.message); + if (additional !== null) assert.ok(error.message.includes(additional), error.message); + return true; +}; } + +// Each restored replay refusal is behind row validation or canonical arguments +// on the store's write path. Replace the relevant identifier at that boundary. +for (const [site, edit] of [ + ['claimed owner authority', r => { r.owner = secret; }], + ['start owner', r => { r.owner = secret; }], + ['review claimant', r => { r.claim.seat = secret; }], + ['close gate owner', r => { r.gateOwner = secret; }], + ['set owner reviewer', r => { r.owner = secret; }], + ['release claimant', r => { r.claim.seat = secret; }], + ['assign existing owner', r => { r.owner = secret; }], + ['assign reviewer', r => { r.reviewers = [secret]; }], + ['note owner', r => { r.owner = secret; }], + ['unresolved request', r => { reviewRow(r); r.review.rounds[0].attempts[0].op = secret + '/'; }], + ['missing approval', r => { reviewRow(r, 'posted'); r.reviewers = [secret]; }], + ['posted request', r => { reviewRow(r, 'posted'); r.review.rounds[0].attempts[0].op = secret + '/'; }], + ['posted request comment', r => { reviewRow(r, 'posted'); r.review.rounds[0].attempts[0].comment = secret; }], + ['outcome actor', r => { reviewRow(r); r.review.rounds[0].attempts[0].by = secret; }], + ['duplicate outcome', r => { reviewRow(r, 'posted'); r.review.rounds[0].attempts[0].op = secret + '/'; }], + ['settled resolution', r => { reviewRow(r, 'posted'); r.review.rounds[0].attempts[0].op = secret + '/'; }], + ['reviewer list', r => { r.reviewers = [secret]; }], + ['duplicate verdict', r => { reviewRow(r, 'posted'); r.review.rounds[0].receipts = [{ reviewer: secret, op: 'receipt-op-0001', at, verdict: 'approve', comment: 1, candidate: digestValue }]; }], +]) test(`queue ${site} rejects malformed identifier before replay refusal`, () => { + const row = doc().rows.find(r => r.id === 6); edit(row); + assert.throws(() => validateRow(row), safe('', QueueError, { code: 2 })); +}); +test('add owner reviewer rejects invalid owner before the conflict refusal', () => { + assert.throws(() => canonArgs('add', { piece: 'piece', gate: 'gate', brief: 'docs/brief.md#Brief', owner: secret, reviewers: [secret] }), safe('owner', QueueError, { code: 2 })); +}); +test('claim mismatch names only the validated owner', () => { + const row = doc().rows.find(r => r.id === 6); row.claim.seat = secret; + assert.throws(() => validateRow(row), error => { + assert.ok(safe('claim seat', QueueError, { code: 2 })(error)); + assert.ok(error.message.includes('darkwing'), error.message); return true; + }); +}); +test('duplicate logged op names the validated op; malformed op stops first', () => { + for (const op of ['genesis-op-0001', secret + '/']) { + const d = doc(); d.log[0].op = op; d.log.push({ ...structuredClone(d.log[0]), rev: 1 }); d.revision = 1; + assert.throws(() => loadDoc(Buffer.from(serialize(d))), op === 'genesis-op-0001' ? named('op id', op) : safe('op', QueueError, { code: 2 })); + } +}); +test('unresolved request refusal names every open request', () => { + assert.throws(() => operation('review-request', {}, row => { + reviewRow(row); + row.review.rounds[0].attempts.push({ ...structuredClone(row.review.rounds[0].attempts[0]), op: 'second-request-01' }); + }), named('unresolved review request', seatValue, 'second-request-01')); +}); +test('approval refusal names every missing approver', () => { + assert.throws(() => operation('move', { to: 'done' }, row => { + reviewRow(row, 'posted'); row.gateOwner = 'sage'; row.reviewers = ['filbert', 'rocko']; + }), named('approval', 'filbert', 'rocko')); +}); diff --git a/packages/queue/tests/review.test.mjs b/packages/queue/tests/review.test.mjs index dfc742946..d713b6e69 100644 --- a/packages/queue/tests/review.test.mjs +++ b/packages/queue/tests/review.test.mjs @@ -240,8 +240,8 @@ test("an unresolved request blocks a new request, a new round, waiting-on-jason const head = s.head(); no(s.run(request("request-9-0002"), "darkwing"), 2, /unresolved review request/); ok(s.run(["review", "record", "9", "--verdict", "approve", "--comment", "77", "--candidate", head, "--op", "record-9-filbert1"], "filbert")); - no(s.run(["move", "9", "done", "--op", "done-9-000001"], "filbert"), 2, /unresolved review request; resolve or abandon it before closing it/); - no(s.run(["move", "9", "waiting-on-jason", "--op", "wait-9-000001"], "darkwing"), 2, /unresolved review request; resolve or abandon it before moving it to waiting-on-jason/); + no(s.run(["move", "9", "done", "--op", "done-9-000001"], "filbert"), 2, /unresolved review request: .*; resolve or abandon it before closing it/); + no(s.run(["move", "9", "waiting-on-jason", "--op", "wait-9-000001"], "darkwing"), 2, /unresolved review request: .*; resolve or abandon it before moving it to waiting-on-jason/); assert.equal(s.posts().length, 1); // A POST that lands after the row reached waiting-on-jason: abandoned, @@ -256,7 +256,7 @@ test("an unresolved request blocks a new request, a new round, waiting-on-jason }; assert.equal(s3.mut({ verb: "move", op: "review-9-00001", args: { id: 9, to: "in-review", candidate: "HEAD" }, by: "darkwing" }, { hook }).code, 3); assert.deepEqual([s3.row(9).state, s3.round(9, 1).attempts[0].state], ["waiting-on-jason", "conflict"]); - no(s3.run(["move", "9", "done", "--op", "done-9-000002"], "jason"), 2, /unresolved review request; resolve or abandon it before closing it/); + no(s3.run(["move", "9", "done", "--op", "done-9-000002"], "jason"), 2, /unresolved review request: .*; resolve or abandon it before closing it/); // Back to in-progress is allowed; the next round is not. const s2 = await ready(t); @@ -264,7 +264,7 @@ test("an unresolved request blocks a new request, a new round, waiting-on-jason no(s2.run(move9(), "darkwing"), 3); s2.rules([]); ok(s2.run(["move", "9", "in-progress", "--op", "changes-9-0001"], "darkwing")); - no(s2.run(["move", "9", "in-review", "--candidate", "HEAD", "--op", "review-9-00002"], "darkwing"), 2, /unresolved review request; resolve or abandon it before a new round/); + no(s2.run(["move", "9", "in-review", "--candidate", "HEAD", "--op", "review-9-00002"], "darkwing"), 2, /unresolved review request: .*; resolve or abandon it before a new round/); assert.equal(s2.posts().length, 1); }); @@ -407,7 +407,7 @@ test("resolve checks the comment: issue, markers, round, candidate and author", no(s.run(["review", "resolve", "9", "review-9-00001", "--comment", "7001", "--op", "resolve-9-00001"], "darkwing"), 1, /failed or had no answer/); no(s.run(["review", "resolve", "9", "review-9-00001", "--comment", "7001", "--op", "resolve-9-00001"], "darkwing", { MOSAIC_GITEA_CREDENTIAL_FILE: s.tokens.dewey }), 2, /cannot check comment 7001: .*not darkwing's token file/); const before = s.calls().length; - no(s.run(["review", "resolve", "9", "review-9-00001", "--comment", "7001", "--op", "resolve-9-00001"], "rocko"), 2, /^queue: row 9 is claimed; only the owner or a privileged actor may resolve a request on row 9$/m); + no(s.run(["review", "resolve", "9", "review-9-00001", "--comment", "7001", "--op", "resolve-9-00001"], "rocko"), 2, /^queue: row 9 is claimed by darkwing; only the owner \(darkwing\) or a privileged actor may resolve a request on row 9$/m); assert.equal(s.calls().length, before, "a refused actor makes no request"); assert.equal(s.doc().revision, 4); s.rules([{ method: "GET", path: "comments/7001$", http: 200, body: good }]); @@ -463,9 +463,9 @@ test("request, changes, a new candidate, approval: every round pinned; no review no(s.run(["review", "record", "9", "--verdict", "maybe", "--comment", "88", "--candidate", c1, "--op", "record-9-filb01"], "filbert"), 2, /approve or changes/); no(s.run(["review", "record", "9", "--verdict", "changes", "--candidate", c1, "--op", "record-9-filb01"], "filbert"), 4, /--comment N is required/); ok(s.run(["review", "record", "9", "--verdict", "changes", "--comment", "88", "--candidate", c1, "--op", "record-9-filb01"], "filbert"), /row 9 round 1 changes by filbert \(comment 88\)$/m); - no(s.run(["review", "record", "9", "--verdict", "approve", "--comment", "89", "--candidate", c1, "--op", "record-9-filb02"], "filbert"), 2, /reviewer already recorded a verdict for row 9 round 1/); + no(s.run(["review", "record", "9", "--verdict", "approve", "--comment", "89", "--candidate", c1, "--op", "record-9-filb02"], "filbert"), 2, /filbert already recorded a verdict for row 9 round 1/); ok(cli(s.repo, ["next", "filbert"]), /^nothing$/m); - no(s.run(["move", "9", "done", "--op", "done-9-000001"], "filbert"), 2, /round 1 has no approval recorded by every listed reviewer/); + no(s.run(["move", "9", "done", "--op", "done-9-000001"], "filbert"), 2, /round 1 has no approval recorded by (?:filbert|rocko)(?:, rocko)?/); ok(s.run(["move", "9", "in-progress", "--op", "changes-9-0001"], "darkwing")); no(s.run(["review", "record", "9", "--verdict", "approve", "--comment", "89", "--candidate", c1, "--op", "record-9-filb02"], "filbert"), 2, /is in-progress; review record applies to in-review rows/); writeFileSync(join(s.repo.root, "packages/fix.mjs"), "export {};\n"); @@ -477,7 +477,7 @@ test("request, changes, a new candidate, approval: every round pinned; no review no(s.run(["review", "record", "9", "--verdict", "approve", "--comment", "89", "--candidate", c1, "--op", "record-9-filb02"], "filbert"), 2, /candidate is not round 2's candidate/); ok(s.run(["review", "record", "9", "--verdict", "approve", "--comment", "89", "--candidate", c2, "--op", "record-9-filb02"], "filbert")); no(s.run(["move", "9", "done", "--evidence", `comment=89,round=2,candidate=${c2}`, "--op", "done-9-000001"], "filbert"), 2, /closes on its recorded verdicts; drop --evidence/); - no(s.run(["move", "9", "done", "--op", "done-9-000001"], "rocko"), 2, /only the gate owner or a privileged actor/); + no(s.run(["move", "9", "done", "--op", "done-9-000001"], "rocko"), 2, /only the gate owner \(filbert\) or a privileged actor/); ok(s.run(["move", "9", "done", "--op", "done-9-000001"], "filbert"), /in-review→done round 2$/m); const rounds = s.row(9).review.rounds; assert.deepEqual(rounds.map((r) => [r.n, r.candidate.digest, r.attempts.map((a) => [a.op, a.state, a.comment]), r.receipts.map((x) => [x.reviewer, x.verdict, x.comment, x.candidate])]), [ @@ -569,7 +569,7 @@ test("semantics: v1 entries replay as before; review entries need v2", async (t) // An attempt takes one outcome, even one that agrees with it. const full = queue.replay(d.log); const again = { ...outcome, rev: full.revision + 1 }; - assert.throws(() => queue.applyEntry(full, again, {}), /request already has its outcome/); + assert.throws(() => queue.applyEntry(full, again, {}), /request review-9-00001 already has its outcome/); // review-outcome is the one entry a done row takes. const at = "2026-09-27T00:00:00.000Z"; // The request was abandoned and the row closed before the POST's answer came. @@ -585,7 +585,7 @@ test("semantics: v1 entries replay as before; review entries need v2", async (t) assert.deepEqual([late.state.rows.get(9).state, a.state, a.transport.comment], ["done", "conflict", 1000]); const lateOk = (verb, op, args, by) => queue.applyEntry(st, entry(st, verb, op, args, by), {}); assert.throws(() => lateOk("review-resolve", "resolve-9-00001", { id: 9, attempt: "review-9-00001", comment: 5 }), /done rows never change/); - assert.throws(() => queue.applyEntry(mid, entry(mid, "review-outcome", "review-9-00001.outcome", { id: 9, attempt: "review-9-00001", outcome: "failed", status: 403, comment: null, detail: "x" }, "sage"), {}), /only the actor who made the request records its outcome/); + assert.throws(() => queue.applyEntry(mid, entry(mid, "review-outcome", "review-9-00001.outcome", { id: 9, attempt: "review-9-00001", outcome: "failed", status: 403, comment: null, detail: "x" }, "sage"), {}), /only darkwing, who made request review-9-00001, records its outcome/); assert.throws(() => queue.canonArgs("review-outcome", { id: 9, attempt: "review-9-00001", outcome: "failed", status: 500, comment: null, detail: "x" }), /a failed outcome's status is one of 400, 401, 403, 404, 422/); assert.throws(() => queue.canonArgs("review-outcome", { id: 9, attempt: "review-9-00001", outcome: "posted", status: 201, comment: null, detail: "x" }), /posted exactly when HTTP 201 returned a comment id/); assert.throws(() => queue.canonArgs("review-outcome", { id: 9, attempt: "review-9-00001", outcome: "posted", status: 200, comment: 4, detail: "x" }), /posted exactly when HTTP 201/); @@ -593,7 +593,7 @@ test("semantics: v1 entries replay as before; review entries need v2", async (t) test("set reviewers refuses the row's owner (2026-09-28)", async (t) => { const s = await ready(t); - no(s.run(["set", "9", "reviewers", "filbert,darkwing", "--op", "reviewers-9-self"], "sage"), 2, /row 9 owner can't be a listed reviewer/); + no(s.run(["set", "9", "reviewers", "filbert,darkwing", "--op", "reviewers-9-self"], "sage"), 2, /row 9 owner darkwing can't be a listed reviewer/); assert.deepEqual(s.row(9).reviewers, ["filbert"]); }); @@ -665,17 +665,17 @@ test("a Jason-gated row reaches waiting-on-jason only on every reviewer's approv ok(s.run(move9(), "darkwing"), /posted comment 1000$/m); assert.deepEqual(s.round(9, 1).receipts, []); no(s.run(["move", "9", "done", "--op", "done-9-000001"], "darkwing"), 2, /gate is Jason's; it goes through waiting-on-jason/); - no(s.run(["move", "9", "waiting-on-jason", "--op", "wait-9-000001"], "darkwing"), 2, /row 9 round 1 has no approval recorded by every listed reviewer$/m); - no(s.run(["move", "9", "waiting-on-jason", "--op", "wait-9-000001"], "sage"), 2, /no approval recorded by every listed reviewer$/m); + no(s.run(["move", "9", "waiting-on-jason", "--op", "wait-9-000001"], "darkwing"), 2, /row 9 round 1 has no approval recorded by (?:filbert|rocko)(?:, rocko)?$/m); + no(s.run(["move", "9", "waiting-on-jason", "--op", "wait-9-000001"], "sage"), 2, /no approval recorded by (?:filbert|rocko)(?:, rocko)?$/m); ok(s.run(["review", "record", "9", "--verdict", "approve", "--comment", "88", "--candidate", head, "--op", "record-9-filb01"], "filbert")); ok(s.run(["review", "record", "9", "--verdict", "changes", "--comment", "89", "--candidate", head, "--op", "record-9-rock01"], "rocko")); - no(s.run(["move", "9", "waiting-on-jason", "--op", "wait-9-000001"], "darkwing"), 2, /no approval recorded by every listed reviewer$/m); + no(s.run(["move", "9", "waiting-on-jason", "--op", "wait-9-000001"], "darkwing"), 2, /no approval recorded by (?:filbert|rocko)(?:, rocko)?$/m); // Changes go back to in-progress, as before. ok(s.run(["move", "9", "in-progress", "--op", "changes-9-0001"], "darkwing")); ok(s.run(move9("review-9-00002"), "darkwing"), /posted comment 1001$/m); ok(s.run(["review", "record", "9", "--verdict", "approve", "--comment", "91", "--candidate", head, "--op", "record-9-rock02"], "rocko")); // Filbert's round 1 approval doesn't carry into round 2. - no(s.run(["move", "9", "waiting-on-jason", "--op", "wait-9-000001"], "darkwing"), 2, /row 9 round 2 has no approval recorded by every listed reviewer$/m); + no(s.run(["move", "9", "waiting-on-jason", "--op", "wait-9-000001"], "darkwing"), 2, /row 9 round 2 has no approval recorded by (?:filbert|rocko)(?:, rocko)?$/m); ok(s.run(["review", "record", "9", "--verdict", "approve", "--comment", "90", "--candidate", head, "--op", "record-9-filb02"], "filbert")); ok(s.run(["move", "9", "waiting-on-jason", "--op", "wait-9-000001"], "darkwing"), /in-review→waiting-on-jason$/m); ok(s.run(["move", "9", "done", "--op", "done-9-000001"], "jason"), /waiting-on-jason→done$/m); diff --git a/packages/queue/tests/store.test.mjs b/packages/queue/tests/store.test.mjs index c66283970..88b12dec6 100644 --- a/packages/queue/tests/store.test.mjs +++ b/packages/queue/tests/store.test.mjs @@ -245,11 +245,11 @@ test("claims and add defaults through the CLI; candidates are manifests or reach test("add, set reviewers and assign refuse the row's owner as a reviewer", (t) => { const repo = ready(t); const before = doc(repo); - no(cli(repo, ["add", "--op", "add-owner-reviewer", "--piece", "x", "--gate", "g", "--brief", "docs/plans/brief-b.md#Template", "--owner", "rocko", "--gate-owner", "filbert", "--reviewer", "rocko"], { by: "sage" }), 2, /row 12 owner can't be a listed reviewer/); + no(cli(repo, ["add", "--op", "add-owner-reviewer", "--piece", "x", "--gate", "g", "--brief", "docs/plans/brief-b.md#Template", "--owner", "rocko", "--gate-owner", "filbert", "--reviewer", "rocko"], { by: "sage" }), 2, /row 12 owner rocko can't be a listed reviewer/); assert.deepEqual(doc(repo), before); - no(cli(repo, ["set", "9", "reviewers", "filbert,darkwing", "--op", "set-9-owner-reviewer"], { by: "sage" }), 2, /row 9 owner can't be a listed reviewer/); + no(cli(repo, ["set", "9", "reviewers", "filbert,darkwing", "--op", "set-9-owner-reviewer"], { by: "sage" }), 2, /row 9 owner darkwing can't be a listed reviewer/); assert.deepEqual(doc(repo), before); - no(cli(repo, ["assign", "9", "filbert", "--op", "assign-9-filbert"], { by: "sage" }), 2, /can't assign row 9: seat is one of its reviewers/); + no(cli(repo, ["assign", "9", "filbert", "--op", "assign-9-filbert"], { by: "sage" }), 2, /can't assign row 9: filbert is one of its reviewers/); assert.deepEqual(doc(repo), before); // The unchanged accept paths: a reviewer list or a new owner that does not collide with the other. ok(cli(repo, ["set", "9", "reviewers", "filbert,dewey", "--op", "set-9-ok"], { by: "sage" })); diff --git a/scripts/mosaic-config.mjs b/scripts/mosaic-config.mjs index 369bf5341..a676242a5 100755 --- a/scripts/mosaic-config.mjs +++ b/scripts/mosaic-config.mjs @@ -278,7 +278,7 @@ function validateFederation(value) { fail(2, `${where}.id must match ${FEDERATION_ID_PATTERN}`); } if (ids.has(peer.id)) { - fail(2, `${where}.id is already named (by federation.self or an earlier peer)`); + fail(2, `${where}.id ${JSON.stringify(peer.id)} is already named (by federation.self or an earlier peer)`); } ids.add(peer.id); validateFederationOrigin(peer.origin, `${where}.origin`); diff --git a/scripts/mosaic-task.mjs b/scripts/mosaic-task.mjs index d931d8f8c..624221c0c 100755 --- a/scripts/mosaic-task.mjs +++ b/scripts/mosaic-task.mjs @@ -125,7 +125,7 @@ function validateMission(document, file) { if (!SUPPORTED_TOOLS.includes(tool)) { fail(2, `unsupported mission tool: (supported: ${SUPPORTED_TOOLS.join(", ")})`); } - if (seen.has(tool)) fail(2, `duplicate tool in mission capabilities.tools`); + if (seen.has(tool)) fail(2, `duplicate tool in mission capabilities.tools: ${tool}`); seen.add(tool); } capabilities = { tools: [...seen] }; @@ -230,7 +230,7 @@ function validateTask(document, file) { if (!SUPPORTED_TOOLS.includes(tool)) { fail(2, `unsupported tool: (supported: ${SUPPORTED_TOOLS.join(", ")})`); } - if (seen.has(tool)) fail(2, `duplicate tool in capabilities.tools`); + if (seen.has(tool)) fail(2, `duplicate tool in capabilities.tools: ${tool}`); seen.add(tool); } tools = [...seen]; diff --git a/scripts/test-config.sh b/scripts/test-config.sh index aae644e9d..5daed4294 100755 --- a/scripts/test-config.sh +++ b/scripts/test-config.sh @@ -238,7 +238,7 @@ expect_fed_refusal "federation peer id pattern exits 2 naming the key" "federati FED_LONG_LABEL="$(printf 'x%.0s' $(seq 81))" expect_fed_refusal "federation peer label over 80 characters exits 2 naming the key" "federation.peers[0].label" \ '{"self":"hub","peers":[{"id":"factory","origin":"https://mosaic.woltje.com","label":"'"$FED_LONG_LABEL"'"}]}' -expect_fed_refusal "federation duplicate peer id exits 2 naming the key" 'federation.peers[1].id is already named' \ +expect_fed_refusal "federation duplicate peer id exits 2 naming the key" 'federation.peers[1].id "factory" is already named' \ '{"self":"hub","peers":[{"id":"factory","origin":"https://mosaic.woltje.com","label":"F"},{"id":"factory","origin":"https://other.woltje.com","label":"G"}]}' expect_fed_refusal "federation peer origin with a trailing slash exits 2 naming the key" "federation.peers[0].origin" \ '{"self":"hub","peers":[{"id":"factory","origin":"https://mosaic.woltje.com/","label":"F"}]}' -- 2.54.0 From f7813339b5daeb257aea16481dcb2150abe1f8af Mon Sep 17 00:00:00 2001 From: Jason Woltje Date: Tue, 10 Nov 2026 22:28:00 -0600 Subject: [PATCH 2/2] test(refusals): detect escaped malformed identifiers (#1873) --- packages/organization/tests/safe-names.test.mjs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/packages/organization/tests/safe-names.test.mjs b/packages/organization/tests/safe-names.test.mjs index 2e7d40908..59d747b83 100644 --- a/packages/organization/tests/safe-names.test.mjs +++ b/packages/organization/tests/safe-names.test.mjs @@ -9,12 +9,14 @@ import { BusinessError } from '../src/errors.mjs'; const bad = 'sk-test-NOT-A-SECRET-1234'; const badId = `${bad.toLowerCase()} "`; -function refusal(fn, names = [], omitted = bad) { +function refusal(fn, names = [], omitted = bad, validation = null) { assert.throws(fn, error => { assert.ok(error instanceof BusinessError); assert.equal(error.exitCode, 2); for (const name of names) assert.ok(error.message.includes(name), error.message); + assert.doesNotMatch(error.message + error.stack, /sk-test|NOT-A-SECRET/i); assert.ok(!error.message.includes(omitted), error.message); + if (validation !== null) assert.match(error.message, validation); return true; }); } @@ -44,7 +46,7 @@ for (const kind of ['creative', 'outside']) test(`hireable ${kind} names a known if (kind === 'outside') writeFileSync(join(root, 'helper.json'), JSON.stringify({ ...d, name, hireable: [], authority: { withinRole: ['role.hire'], crossRole: [] } })); }; refusal(role(t, edit), kind === 'creative' ? ['companion'] : ['helper', 'role.hire']); - refusal(role(t, d => { d.hireable = [badId]; }), [], badId); + refusal(role(t, d => { d.hireable = [badId]; }), [], badId, /role hireable entry must match/); }); test('hireable wider action rejects unknown action before naming it', t => { refusal(role(t, (d, root) => { @@ -82,5 +84,6 @@ for (const kind of ['reserved', 'human', 'user', 'grant', 'project', 'instance'] } const value = { reserved: 'human', human: 'jason', user: 'bean', grant: 'g-known-1', project: '11111111-2222-4333-8444-555555555555', instance: 'coder' }[kind]; refusal(run(value), [value]); - refusal(run(badId), [], badId); + const validation = kind === 'project' ? /lowercase UUID/ : kind === 'instance' ? /declared instance/ : /\.id must match/; + refusal(run(badId), [], badId, validation); }); -- 2.54.0