From f3f33a6171ab10f62039914871c2fddcf805a61c Mon Sep 17 00:00:00 2001 From: Jason Woltje Date: Wed, 11 Nov 2026 01:48:59 -0600 Subject: [PATCH 1/8] fix(organization,cli,discord,seat,control-board): refusals name the field or file by label, never a host path (#1873) redact6. A refusal names the field, the file or the instance. It never quotes a config value, a host path, or a raw e.message or stderr that carries one. - organization: checkCredentialRef names the field and each forbidden root by label; OrgError keeps .path only for a document field ($...); the registry, fence and lock refusals use the quiet helpers (call and code). - cli: loadSystem never relays mosaic-config stderr: instance refusals keep their code and names, everything else reads as a field and its constraint against a labelled config. Project-key, access warning, halt gate, sessions file, launch socket, harness relays and the workspace rename name labels. Launch-log reasons read a Node system error as its call and code. - scripts/mosaic-config: configuration not found names where the config came from. - discord, seat, control-board: config, token, context, tool-root, journal, binding, registration and seen-marks refusals name labels. Exit codes, refusal codes and check order are unchanged. Each new or changed test fails on BASE be698e602. --- packages/cli/src/cli.mjs | 3 +- packages/cli/src/config.mjs | 43 ++++- packages/cli/src/launcher.mjs | 41 +++-- packages/cli/tests/access-boot.test.mjs | 4 +- packages/cli/tests/json-redaction.test.mjs | 4 +- packages/cli/tests/last-values.test.mjs | 7 +- packages/cli/tests/lifecycle.test.mjs | 4 + .../cli/tests/path-redaction-labels.test.mjs | 156 ++++++++++++++++++ packages/cli/tests/stack-instances.test.mjs | 4 +- packages/control-board/src/scan.mjs | 15 +- .../tests/path-redaction-labels.test.mjs | 61 +++++++ .../tests/refusal-redaction.test.mjs | 10 +- packages/discord/src/approvals.mjs | 2 +- packages/discord/src/binding.mjs | 58 ++++--- packages/discord/src/cli.mjs | 6 +- packages/discord/src/git.mjs | 10 +- packages/discord/src/journal.mjs | 25 ++- packages/discord/src/setspark.mjs | 10 +- .../tests/path-redaction-labels.test.mjs | 88 ++++++++++ .../discord/tests/refusal-redaction.test.mjs | 20 ++- packages/organization/src/config-command.mjs | 54 +++--- packages/organization/src/credentials.mjs | 19 ++- packages/organization/src/org-registry.mjs | 24 ++- packages/organization/src/org-service.mjs | 7 +- .../tests/path-redaction-labels.test.mjs | 90 ++++++++++ packages/seat/src/seat.mjs | 20 ++- packages/seat/src/session.mjs | 5 +- .../seat/tests/path-redaction-labels.test.mjs | 70 ++++++++ .../seat/tests/refusal-redaction.test.mjs | 10 +- scripts/mosaic-config.mjs | 4 +- 30 files changed, 729 insertions(+), 145 deletions(-) create mode 100644 packages/cli/tests/path-redaction-labels.test.mjs create mode 100644 packages/control-board/tests/path-redaction-labels.test.mjs create mode 100644 packages/discord/tests/path-redaction-labels.test.mjs create mode 100644 packages/organization/tests/path-redaction-labels.test.mjs create mode 100644 packages/seat/tests/path-redaction-labels.test.mjs diff --git a/packages/cli/src/cli.mjs b/packages/cli/src/cli.mjs index 314ff713f..d8fbeda92 100644 --- a/packages/cli/src/cli.mjs +++ b/packages/cli/src/cli.mjs @@ -519,7 +519,8 @@ async function processingCommand(verb, rest, io, deps) { if (!result || typeof result !== 'object' || (typeof result.halted !== 'boolean' && !result.invalid)) throw new CliError('processing outcome unknown; use mosaic processing status before trying again', 1); if (verb === 'halt' && result.quiescent !== true) - throw new CliError(`halt gate is on for instance ${system.dataRoot}; ${result.remaining ?? 'unknown'} stops unconfirmed. Use mosaic processing status`, 1); + // #1873: the refusal names the instance, never its data root; a count that is not a count is unknown. + throw new CliError(`halt gate is on for this instance; ${Number.isSafeInteger(result.remaining) && result.remaining >= 0 ? result.remaining : 'unknown'} stops unconfirmed. Use mosaic processing status`, 1); if (parsed.flags.has('--json')) { io.stdout.write(JSON.stringify(result, null, 2) + '\n'); return; } const status = verb === 'resume' ? 'resumed' : result.halted ? 'halted' : result.invalid ? 'uninitialized' : 'running'; io.stdout.write(`processing ${status} for instance ${system.dataRoot}\n`); diff --git a/packages/cli/src/config.mjs b/packages/cli/src/config.mjs index 36fee7d83..16d211900 100644 --- a/packages/cli/src/config.mjs +++ b/packages/cli/src/config.mjs @@ -13,6 +13,7 @@ import { BusinessError, accessFilePath, configDir, loadAccess, loadBusiness, loa import { loadRootAgents } from "../../organization/src/agent-levels.mjs"; import { materializeCopies, withCopies } from "../../organization/src/agent-copies.mjs"; import { organizationWriter } from "../../organization/src/organization.mjs"; +import { systemConfigProblem } from "../../organization/src/config-command.mjs"; import { businessFilePath } from "../../organization/src/organization-config.mjs"; import { busOrganization } from "../../bus/src/organization-context.mjs"; import { BusError } from "../../bus/src/broker.mjs"; @@ -41,13 +42,40 @@ export function orgPaths(root) { threads: join(root, "threads"), hostDir }; } +// #1873: mosaic-config's stderr can quote the config's path, the data root or a config value, so +// it is never relayed. An instance refusal keeps its code and the instance names, which passed +// the instance pattern; a missing config keeps its bootstrap hint; anything else reads as +// `mosaic org config` reads it: a known field and its constraint, the config named by label. +const INSTANCE = "[a-z][a-z0-9]{1,15}"; +function instanceProblem(problem, label) { + const body = problem.replace(/^mosaic-config: /, ""); + let m = new RegExp(`^instance-root-mismatch: .+ is claimed by instance (${INSTANCE})(?:, not (${INSTANCE})|; select it with --instance \\1)$`).exec(body); + if (m) return `instance-root-mismatch: the data root is claimed by instance ${m[1]}${m[2] ? `, not ${m[2]}` : `; select it with --instance ${m[1]}`}`; + m = new RegExp(`^instance-root-unclaimed: .+ holds no instance\\.json naming (${INSTANCE}) \\(scripts/bootstrap\\.sh --instance \\1 claims a new root\\)$`).exec(body); + if (m) return `instance-root-unclaimed: the data root holds no instance.json naming ${m[1]} (scripts/bootstrap.sh --instance ${m[1]} claims a new root)`; + if (body.startsWith("instance-root-unclaimed: ")) return "instance-root-unclaimed: the data root's instance.json is not a readable, valid marker"; + m = new RegExp(`^instance-config-mismatch: --instance (${INSTANCE}) names `).exec(body); + if (m) return `instance-config-mismatch: --instance ${m[1]} names its own config, but MOSAIC_CONFIG names another`; + if (body.startsWith("instance-config-mismatch: ")) return "instance-config-mismatch: --instance names its own config, but MOSAIC_CONFIG names another"; + if (body.startsWith("instance-name-invalid: ")) return `instance-name-invalid: an instance name must match /^${INSTANCE}$/`; + if (body.startsWith("configuration not found: ")) { + m = new RegExp(`\\(run (scripts/bootstrap\\.sh(?: --instance ${INSTANCE})?) to create it\\)$`).exec(body); + return `configuration not found: ${label}${m ? ` (run ${m[1]} to create it)` : ""}`; + } + return null; +} + export function loadSystem({ env = process.env, instance = null } = {}) { const proc = spawnSync(process.execPath, [join(REPO, "scripts", "mosaic-config.mjs"), "validate", ...(instance === null ? [] : ["--instance", instance])], { encoding: "utf8", maxBuffer: 1024 * 1024, env, }); - if (proc.status !== 0) throw new CliError(`system config problem (mosaic-config exit ${proc.status}): ${proc.stderr.trim()}`, 3); + if (proc.status !== 0) { + const label = instance !== null && instance !== "dev" ? `the config of instance ${instance}` : env.MOSAIC_CONFIG ? "the system config MOSAIC_CONFIG names" : "the default system config"; + const known = instanceProblem(String(proc.stderr ?? "").trim(), label); + throw new CliError(known ? `system config problem (mosaic-config exit ${proc.status}): ${known}` : systemConfigProblem(proc, env, label), 3); + } try { return JSON.parse(proc.stdout); } catch { @@ -90,7 +118,7 @@ function trackerFor(system, b, warn, dataRoot) { if (dataRoot && managedState({ dataRoot, business: b.id, project: id, entry }) === 'reserved') continue; if (!existsSync(projectFilePath(entry.root))) continue; const project = loadProject(entry.root); - if (project.id !== id) throw new CliError(`project file ${project.file} has id that does not match the declared project key`, 3); + if (project.id !== id) throw new CliError(`project file of project ${id} has id that does not match the declared project key`, 3); const vars = resolveInstance({ system, business: b, project, instance: first }).vars; if (vars["tracker.project"] !== undefined) named.push({ id, vars }); } @@ -127,17 +155,20 @@ function accessFor(system, env, warn) { try { context = accessContextFor(system, env); } catch (e) { - warn(`access.json is present but its context can't be read, so no granted user binds: ${e.message}`); + // #1873: the error's message can quote a path; only a plain code is relayed. + const code = typeof e?.code === "string" && /^[A-Za-z][A-Za-z0-9_-]{0,63}$/.test(e.code) ? ` (${e.code})` : ""; + warn(`access.json is present but its context can't be read, so no granted user binds${code}`); return null; } // sage-3153: one line at boot when the loader refuses the file, so the owner sees why no user - // binds. The loader's messages can quote the file, so the line names only the path and a code - // from its exit code: 4 is a file it could not open as a regular file, anything else invalid. + // binds. The loader's messages can quote the file, so the line names only the file, by label, and + // a code from its exit code: 4 is a file it could not open as a regular file, anything else + // invalid. #1873: no host path. try { loadAccess(file, context); } catch (e) { const code = e instanceof BusinessError && e.exitCode !== 4 ? "access-invalid" : "access-unreadable"; - warn(`access file refused at boot (${code}), no granted user binds until it is fixed: ${file}`); + warn(`access file refused at boot (${code}), no granted user binds until it is fixed: access.json in the config directory`); } return { file, context }; } diff --git a/packages/cli/src/launcher.mjs b/packages/cli/src/launcher.mjs index 49299f2c5..24f43e0b5 100644 --- a/packages/cli/src/launcher.mjs +++ b/packages/cli/src/launcher.mjs @@ -145,6 +145,7 @@ import { chmodSync, existsSync, lstatSync, mkdirSync, readFileSync, renameSync, import { dirname, join, relative } from "node:path"; import { configDir, resolveInstance, systemVars } from "../../organization/src/index.mjs"; import { organizationWriter } from "../../organization/src/organization.mjs"; +import { quietMessage } from "../../organization/src/org-registry.mjs"; import { businessFilePath } from "../../organization/src/organization-config.mjs"; import { copyOf, copyRefusal, loadWithCopies, projectRefusal, withCopies } from "../../organization/src/agent-copies.mjs"; import { selectModelProvider } from "../../plugins/src/model-provider-selection.mjs"; @@ -201,13 +202,17 @@ class Refusal extends Error { } } +// #1873: a refusal's reason never quotes a host path. A Node system error's message names one, +// so it reads as the call and its code (org-registry quietMessage); any other message stays. +const reason = (e) => quietMessage(e, "a host file") ?? String(e?.message ?? e); + // A harness module runs with the host's authority, and anything it throws refuses the launch // and names the module rather than escaping the host. function call(name, what, fn) { try { return fn(); } catch (e) { - throw new Refusal("harness-failed", `harness ${name} ${what}: ${e.message}`); + throw new Refusal("harness-failed", `harness ${name} ${what}: ${reason(e)}`); } } @@ -272,7 +277,8 @@ export function createLauncher({ try { leftover = readSessions(dataRoot).filter((s) => s.business === b.id); } catch (e) { - throw new CliError(`${e.message}; check which sessions still run, then move the file aside`, 3); + // readSessions names the file by its place under the data root (#1873); anything else is unreadable. + throw new CliError(`${e?.name === "SeatError" ? e.message : "sessions file is unreadable"}; check which sessions still run, then move the file aside`, 3); } const registry = new Registry(dataRoot, { readOnly: !launches }); // Row 269: earlier-host runs still live after recover(). check() sees each while it lives. @@ -437,7 +443,9 @@ export function createLauncher({ const harnessOf = async (m) => { const loader = makeLoader({ dataRoot:system.instanceRoot ?? dataRoot, log: (entry) => record({ ...entry, instance }) }); const { refused } = await loader.load({ harness: [m.harness] }); - if (refused.length) throw new Refusal("harness-refused", `harness ${m.harness}: ${refused.map((r) => `${r.code}: ${r.message}`).join("; ")}`); + // #1873: a loader message can quote the module's path or its load error; only a + // missing-export one, which names the export, is relayed. Any other refusal names its code. + if (refused.length) throw new Refusal("harness-refused", `harness ${m.harness}: ${refused.map((r) => r.code === "missing-export" ? `${r.code}: ${r.message}` : String(r.code)).join("; ")}`); const module = loader.modules("harness")[0]; const levels = call(m.harness, "levels", () => module.levels()); if (!levels.includes(m.thinking)) throw new Refusal("thinking-unsupported", `harness ${m.harness} takes thinking ${levels.join(", ")}, not ${m.thinking}`); @@ -861,7 +869,7 @@ export function createLauncher({ unregisterRuntime(dataRoot, managed.id); if (e instanceof Refusal) throw e; if (e.code?.startsWith('proxy-')) throw new Refusal(e.code, e.code); - throw new Refusal("launch-failed", e.message); + throw new Refusal("launch-failed", reason(e)); } children.set(run, exited); const outputWatch = admittedProxy ? watchModelOutput({ runDir, armAtFirstTurn: true, keyId: modelKeyId, seconds: system.proxy.firstOutputSeconds ?? 120, @@ -1041,7 +1049,7 @@ export function createLauncher({ if (refusedThreads.get(key) === code) return; refusedThreads.set(key, code); log(`thread ${t.thread} session ${t.session} (${t.instance}): start refused: ${code}`); - record({ event: "refused", by: b.human, instance: t.instance, thread: t.thread, session: t.session, code, why: e.message, ...(e.limit?{limit:e.limit}:{}) }); + record({ event: "refused", by: b.human, instance: t.instance, thread: t.thread, session: t.session, code, why: reason(e), ...(e.limit?{limit:e.limit}:{}) }); } }); } @@ -1143,7 +1151,7 @@ export function createLauncher({ if (clearLogged.get(c.op) === code) return; clearLogged.set(c.op, code); log(`clear ${c.op}: successor ${c.successor} (${c.instance}): start ${CLEAR_HELD.has(code) ? "held" : "refused"}: ${code}`); - record({ event: "refused", by: b.human, instance: c.instance, thread: c.thread, session: c.successor, code, why: e.message, ...(e.limit ? { limit: e.limit } : {}) }); + record({ event: "refused", by: b.human, instance: c.instance, thread: c.thread, session: c.successor, code, why: reason(e), ...(e.limit ? { limit: e.limit } : {}) }); await host.op({ op: "sessionClearStart", record: { ...where, start: CLEAR_HELD.has(code) ? "held" : "refused", refusal: code } }); return; } @@ -1203,7 +1211,7 @@ export function createLauncher({ if (refusedLand.get(h.decision) === code) return; refusedLand.set(h.decision, code); log(`decision ${h.decision} (${h.instance}): landing start refused: ${code}`); - record({ event: "refused", by: b.human, instance: h.instance, decision: h.decision, code, why: e.message, ...(e.limit ? { limit: e.limit } : {}) }); + record({ event: "refused", by: b.human, instance: h.instance, decision: h.decision, code, why: reason(e), ...(e.limit ? { limit: e.limit } : {}) }); } }); } @@ -1248,7 +1256,7 @@ export function createLauncher({ try { done = await act(take); } catch (e) { - done = { refusal: e instanceof Refusal ? e.code : "launch-failed", why: e.message, limit: e.limit }; + done = { refusal: e instanceof Refusal ? e.code : "launch-failed", why: reason(e), limit: e.limit }; } const { why, limit, ...closing } = done; if (closing.refusal) record({ event: "refused", by: r.by, instance: r.instance, request: r.request, code: closing.refusal, ...(why ? { why } : {}), ...(limit ? { limit } : {}) }); @@ -1356,18 +1364,19 @@ export function createLauncher({ const bot = out.bot ? { bot: out.bot } : {}; // The old instance workspace, then (row 269) the instance's session workspaces, each moved // whole. A retry moves whichever is still in place. - for (const from of [workspaceDir(dataRoot, b.id, r.instance), join(dataRoot, "session-workspaces", b.id, r.instance)]) { + for (const [from, label] of [[workspaceDir(dataRoot, b.id, r.instance), "the instance workspace"], [join(dataRoot, "session-workspaces", b.id, r.instance), "the session workspaces"]]) { if (!existsSync(from)) continue; const to = join(dirname(from), ".retired", `${r.instance}-${transaction}`); try { mkdirSync(dirname(to), { recursive: true, mode: 0o700 }); chmodSync(dirname(to), 0o700); - if (existsSync(to)) throw new Error(`${to} exists`); + if (existsSync(to)) throw new Error("its .retired name already exists"); renameSync(from, to); chmodSync(to, 0o700); } catch (e) { log(`request ${r.request} (${r.instance}): workspace rename failed: ${e.message}; left at ${from}`); - return { transaction, ...bot, refusal: "workspace-rename-failed", why: `${e.message}; left at ${from}` }; + // #1873: the reason names the workspace by label and the error by code, never a path. + return { transaction, ...bot, refusal: "workspace-rename-failed", why: `${quietMessage(e, label) ?? `${label}: ${e.message}`}; left in place` }; } record({ event: "workspace-retired", instance: r.instance, request: r.request, transaction, path: to }); } @@ -1438,7 +1447,7 @@ export function createLauncher({ } catch (e) { if (!(e instanceof Refusal)) throw e; await host.op({ op: "refuse", cap, code: e.code }).catch(() => {}); - record({ event: "refused", by: who.role, instance, code: e.code, why: e.message, ...(e.limit?{limit:e.limit}:{}) }); + record({ event: "refused", by: who.role, instance, code: e.code, why: reason(e), ...(e.limit?{limit:e.limit}:{}) }); return reply({ ok: false, error: e.code }); } try { @@ -1453,7 +1462,7 @@ export function createLauncher({ const code = e instanceof Refusal ? e.code : "launch-failed"; log(`launch of ${instance} for ${who.role} failed: ${e.message}`); await host.op({ op: "refuse", cap, code }).catch(() => {}); - record({ event: "refused", by: who.role, instance, code, why: e.message, ...(e.limit?{limit:e.limit}:{}) }); + record({ event: "refused", by: who.role, instance, code, why: reason(e), ...(e.limit?{limit:e.limit}:{}) }); return reply({ ok: false, error: code }); } }); @@ -1467,8 +1476,8 @@ export function createLauncher({ return await start({ instance: b.launch.by, launchedBy: b.human, ...(await check(b.launch.by, { pm: true })) }); } catch (e) { if (!(e instanceof Refusal)) throw e; - record({ event: "refused", by: b.human, instance: b.launch.by, code: e.code, why: e.message, ...(e.limit?{limit:e.limit}:{}) }); - const error = new CliError(`PM launch refused: ${e.code}${e.message !== e.code ? ` (${e.message})` : ""}`, 3); + record({ event: "refused", by: b.human, instance: b.launch.by, code: e.code, why: reason(e), ...(e.limit?{limit:e.limit}:{}) }); + const error = new CliError(`PM launch refused: ${e.code}${reason(e) !== e.code ? ` (${reason(e)})` : ""}`, 3); error.code = e.code; throw error; } @@ -1519,7 +1528,7 @@ export function createLauncher({ await recover(); const path = launchSocketPath(dataRoot); if (existsSync(path)) { - if (!lstatSync(path).isSocket()) throw new CliError(`${path} exists and isn't a socket`, 3); + if (!lstatSync(path).isSocket()) throw new CliError("launch socket path exists and isn't a socket", 3); rmSync(path); } server = createServer((socket) => { diff --git a/packages/cli/tests/access-boot.test.mjs b/packages/cli/tests/access-boot.test.mjs index c9337d6a4..d573d0931 100644 --- a/packages/cli/tests/access-boot.test.mjs +++ b/packages/cli/tests/access-boot.test.mjs @@ -91,7 +91,9 @@ test("R6 B3 a boot with a file R1's loader refuses still boots, binds no user an const s = await boot(t, file); assert.equal(s.config.access.file, s.path, why); assert.equal(s.bind("alice"), "access-revoked", why); - assert.deepEqual(s.warnings, [TRACKER, `access file refused at boot (${code}), no granted user binds until it is fixed: ${s.path}`], why); + // #1873: the warning names access.json by its place, never by host path. + assert.deepEqual(s.warnings, [TRACKER, `access file refused at boot (${code}), no granted user binds until it is fixed: access.json in the config directory`], why); + assert.ok(!s.warnings.join("\n").includes(s.path), why); assert.doesNotMatch(s.warnings.join("\n"), /secret-marker/, why); } }); diff --git a/packages/cli/tests/json-redaction.test.mjs b/packages/cli/tests/json-redaction.test.mjs index bbcffd626..7fbc4a09b 100644 --- a/packages/cli/tests/json-redaction.test.mjs +++ b/packages/cli/tests/json-redaction.test.mjs @@ -40,7 +40,9 @@ test('system config parse refusal and CLI relay omit parser bytes', t => { redacted(f.run(configScript, 'validate', '--instance', 'redacttest'), f.file, 2); assert.throws(() => loadSystem({ env: f.env, instance: 'redacttest' }), error => { assert.equal(error.exitCode, 3); - assert.ok(error.message.includes(f.file)); + // #1873: the cli relay names the instance's config by label, never its path. + assert.ok(!error.message.includes(f.file), error.message); + assert.ok(error.message.includes('configuration is not valid JSON; expected readable JSON: the config of instance redacttest'), error.message); assert.match(error.message, /not valid JSON/); assert.doesNotMatch(error.message, /sk-test|Unexpected token/); return true; diff --git a/packages/cli/tests/last-values.test.mjs b/packages/cli/tests/last-values.test.mjs index f7e823f3c..e529b80c2 100644 --- a/packages/cli/tests/last-values.test.mjs +++ b/packages/cli/tests/last-values.test.mjs @@ -89,7 +89,12 @@ test('CLI boot project id mismatch omits the configured value and keeps exit 3', const root = scratch(t), f = fixture(root), file = join(f.doc.projects.stack.root, '.mosaic', 'project.json'); mkdirSync(join(f.doc.projects.stack.root, '.mosaic')); 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())); + assert.throws(() => bootConfig({ system: loadSystem({ env: f.env }), businessId: 'acme', env: f.env }), error => { + // #1873: the refusal names the project by its key, never the project file's path. + safe('project file of project stack has id that does not match the declared project key', CliError, { exitCode: 3 }, secret.toLowerCase())(error); + assert.ok(!error.message.includes(file) && !error.message.includes(f.doc.projects.stack.root), error.message); + return true; + }); }); for (const mission of [false, true]) test(`duplicate ${mission ? 'mission' : 'task'} tool rejects an unknown value before naming it`, t => { diff --git a/packages/cli/tests/lifecycle.test.mjs b/packages/cli/tests/lifecycle.test.mjs index 7548107d6..bf417882f 100644 --- a/packages/cli/tests/lifecycle.test.mjs +++ b/packages/cli/tests/lifecycle.test.mjs @@ -404,6 +404,10 @@ test("row 194 V5: delete retires the name, renames the workspace to .retired/I-< const del = s.call("instances.set", { revision: revision(), instance: "sec-1", op: "delete" }).request; const failed = await s.carry(del); assert.equal(failed.done.refusal, "workspace-rename-failed"); + // #1873: the refused event's reason names the workspace by label and the error by code, never a path. + const refused = launchLog(p.f).filter((e) => e.event === "refused" && e.code === "workspace-rename-failed"); + assert.deepEqual(refused.map((e) => e.why), ["cannot mkdir the instance workspace (EEXIST); left in place"]); + assert.ok(!JSON.stringify(refused).includes(p.f.dataRoot)); const transaction = failed.done.transaction; assert.ok(transaction); const file = JSON.parse(readFileSync(join(p.root, "config", "businesses", "acme.json"), "utf8")); diff --git a/packages/cli/tests/path-redaction-labels.test.mjs b/packages/cli/tests/path-redaction-labels.test.mjs new file mode 100644 index 000000000..f30ecb3e7 --- /dev/null +++ b/packages/cli/tests/path-redaction-labels.test.mjs @@ -0,0 +1,156 @@ +// #1873 (redact6): a cli refusal names the file, the instance or the field, never a host path, +// and never relays mosaic-config's stderr or a Node system error's message. The sentinel is a +// component of every scratch path, so a refusal that quotes a path quotes the sentinel too; each +// test also checks the scratch root and the label. The launcher cases need no bus: a bus socket +// under a sentinel path would pass the Unix socket length limit, so they use a stub host. +import test from 'node:test'; +import assert from 'node:assert/strict'; +import { mkdirSync, readFileSync, writeFileSync } from 'node:fs'; +import { join } from 'node:path'; +import { createLauncher } from '../src/launcher.mjs'; +import { CliError } from '../src/errors.mjs'; +import { loadSystem } from '../src/config.mjs'; +import { main } from '../src/cli.mjs'; +import { initializeControl } from '../../processing-control/src/state.mjs'; +import { launchLogFile } from '../../seat/src/session.mjs'; +import { fakeHarnesses, fixture, io, tmp, withoutHostCredentials } from './helpers.mjs'; + +const secret = 'sk-test-NOT-A-SECRET-1234'; +function scratch(t) { + const root = tmp(t, 'r6-'); + const dir = join(root, secret); + mkdirSync(dir, { mode: 0o700 }); + return { root, dir }; +} +function clean(text, root) { + assert.ok(!text.includes(root), text); + assert.doesNotMatch(text, /sk-test|NOT-A-SECRET/); +} +const PI = JSON.stringify(new URL('../../plugins/harness/pi/index.mjs', import.meta.url).href); + +function stubLauncher(t, { op = async () => ({}) } = {}) { + const { root, dir } = scratch(t); + const f = fixture(dir, 'acme'); + initializeControl(f.dataRoot); + withoutHostCredentials(f.env, { piKey: true }); + const system = JSON.parse(readFileSync(f.config, 'utf8')); + const host = { business: 'acme', beforeClose: () => {}, op }; + return { root, f, make: () => createLauncher({ host, system, env: f.env, bwrap: '/usr/bin/true', log: () => {} }) }; +} + +test('an unreadable sessions file refuses exit 3 naming bus-host/sessions.json under the data root, never its path', t => { + const s = stubLauncher(t); + mkdirSync(join(s.f.dataRoot, 'bus-host'), { recursive: true, mode: 0o700 }); + writeFileSync(join(s.f.dataRoot, 'bus-host', 'sessions.json'), '{not json'); + assert.throws(() => s.make(), e => { + assert.ok(e instanceof CliError, e.stack); + assert.equal(e.exitCode, 3); + assert.equal(e.message, 'sessions file is unreadable: bus-host/sessions.json under the data root; check which sessions still run, then move the file aside'); + clean(e.message, s.root); + return true; + }); + writeFileSync(join(s.f.dataRoot, 'bus-host', 'sessions.json'), JSON.stringify({ sessionsVersion: 999, sessions: [] })); + assert.throws(() => s.make(), e => e.exitCode === 3 && e.message.startsWith('sessions file is malformed: bus-host/sessions.json under the data root;') && !e.message.includes(s.root)); +}); + +test('a launch socket path that is not a socket refuses exit 3 by label, never its path', async t => { + const s = stubLauncher(t); + mkdirSync(join(s.f.dataRoot, 'bus-host'), { recursive: true, mode: 0o700 }); + writeFileSync(join(s.f.dataRoot, 'bus-host', 'launch.sock'), 'in the way\n'); + const launcher = s.make(); + t.after(() => launcher.close?.()); + await assert.rejects(launcher.listen(), e => { + assert.ok(e instanceof CliError, e.stack); + assert.equal(e.exitCode, 3); + assert.equal(e.message, "launch socket path exists and isn't a socket"); + clean(e.message, s.root); + return true; + }); +}); + +// The launch-routes pattern: a stub host stops each launch at authorizeLaunch, and the launch +// log's refused event carries the reason. +async function routed(t, harness) { + const { root, dir } = scratch(t); + const f = fixture(dir, 'acme', doc => { + doc.roles.coder.vars = { harness, model: 'claude-sonnet-5-5', thinking: 'high' }; + doc.plugins = { ...doc.plugins, harness: ['pi', 'claude-code', harness] }; + return doc; + }); + initializeControl(f.dataRoot); + withoutHostCredentials(f.env, { piKey: true }); + const host = { business: 'acme', beforeClose: () => {}, op: async x => { + if (x.op === 'identity') return { business: 'acme', role: 'pm' }; + if (x.op === 'mayLaunch') return { allow: true }; + if (x.op === 'authorizeLaunch') throw Object.assign(Error('fixture authorization stop'), { code: 'fixture-policy' }); + return {}; + } }; + const absent = join(dir, 'absent.json'); + const makeLoader = await fakeHarnesses(t, { pi: null, 'claude-code': null }, write => { + // fsthrows: its preflight reads a missing file, so the error's message quotes the path. + write('harness', 'fsthrows', `export * from ${PI};\nimport { readFileSync } from "node:fs";\nexport function preflight() { readFileSync(${JSON.stringify(absent)}); }\n`); + }); + const system = JSON.parse(readFileSync(f.config, 'utf8')); + const launcher = createLauncher({ host, system, env: f.env, makeLoader, bwrap: '/usr/bin/true', log: () => {} }); + const last = () => readFileSync(launchLogFile(f.dataRoot, 'acme'), 'utf8').trim().split('\n').map(l => JSON.parse(l)).at(-1); + return { root, launch: () => launcher.fromSession('fixture-cap', 'coder'), last }; +} + +test('a harness whose preflight throws a system error refuses harness-failed naming the call and code, never the path', async t => { + const s = await routed(t, 'fsthrows'); + assert.deepEqual(await s.launch(), { ok: false, error: 'harness-failed' }); + assert.equal(s.last().why, 'harness fsthrows preflight: cannot open a host file (ENOENT)'); + clean(JSON.stringify(s.last()), s.root); +}); + +test('a harness the loader refuses names its code only, never the loader message or a path', async t => { + const s = await routed(t, 'absent'); + assert.deepEqual(await s.launch(), { ok: false, error: 'harness-refused' }); + assert.match(s.last().why, /^harness absent: [a-z][a-z0-9-]*$/); + clean(JSON.stringify(s.last()), s.root); +}); + +test('the halt gate refusal names the instance, never its data root, and keeps exit 1 and the count', async t => { + const { root, dir } = scratch(t); + const result = { version: 1, generation: '11111111-1111-4111-8111-111111111111', operation: '11111111-1111-4111-8111-111111111111', business: 'demo', human: 'jason', at: '2026-10-20T00:00:00.000Z', recovered: false, halted: true, quiescent: false, remaining: 2, stopped: 0 }; + const x = io(); + await assert.rejects(main(['halt', '--business', 'demo'], x, { system: { dataRoot: join(dir, 'data') }, transport: () => async () => result }), e => { + assert.equal(e.exitCode, 1); + assert.equal(e.message, 'halt gate is on for this instance; 2 stops unconfirmed. Use mosaic processing status'); + clean(e.message, root); + return true; + }); + const odd = io(); + await assert.rejects(main(['halt', '--business', 'demo'], odd, { system: { dataRoot: join(dir, 'data') }, transport: () => async () => ({ ...result, remaining: join(dir, 'x') }) }), e => { + assert.equal(e.message, 'halt gate is on for this instance; unknown stops unconfirmed. Use mosaic processing status'); + clean(e.message, root); + return true; + }); +}); + +test('a system config problem names the config by label and never relays mosaic-config stderr', t => { + const { root, dir } = scratch(t); + // Missing: the label for where it came from, with the bootstrap hint. + assert.throws(() => loadSystem({ env: { PATH: process.env.PATH, HOME: dir, MOSAIC_CONFIG: join(dir, 'absent.json') } }), e => { + assert.ok(e instanceof CliError, e.stack); + assert.equal(e.exitCode, 3); + assert.equal(e.message, 'system config problem (mosaic-config exit 3): configuration not found: the system config MOSAIC_CONFIG names (run scripts/bootstrap.sh to create it)'); + clean(e.message, root); + return true; + }); + assert.throws(() => loadSystem({ env: { PATH: process.env.PATH, HOME: dir } }), e => { + assert.match(e.message, /^system config problem \(mosaic-config exit 3\): configuration not found: the default system config \(run scripts\/bootstrap\.sh to create it\)$/); + clean(e.message, root); + return true; + }); + // A field whose value quotes the sentinel: the field and its constraint, never the value. + const f = fixture(dir, 'acme'); + const doc = JSON.parse(readFileSync(f.config, 'utf8')); + writeFileSync(f.config, JSON.stringify({ ...doc, dataRoot: `relative/${secret}` })); + assert.throws(() => loadSystem({ env: f.env }), e => { + assert.equal(e.exitCode, 3); + assert.match(e.message, /^system config problem \(mosaic-config exit \d+\): dataRoot: expected a canonical absolute instance data path: the system config MOSAIC_CONFIG names$/); + clean(e.message, root); + return true; + }); +}); diff --git a/packages/cli/tests/stack-instances.test.mjs b/packages/cli/tests/stack-instances.test.mjs index ff028b5cd..daa3f8972 100644 --- a/packages/cli/tests/stack-instances.test.mjs +++ b/packages/cli/tests/stack-instances.test.mjs @@ -270,7 +270,9 @@ test("T-I2 selection: the four rows of D1 through the instance op and through an await t.test("a named instance with no config exits 3 with its own bootstrap hint", () => { const r = config(home, {}, "instance", "--instance", "gamma"); assert.equal(r.code, 3); - assert.match(r.err, /configuration not found: .*mosaic-gamma\/config\.json \(run scripts\/bootstrap\.sh --instance gamma to create it\)/); + // #1873: the refusal names the config by its instance, never by host path. + assert.match(r.err, /configuration not found: the config of instance gamma \(run scripts\/bootstrap\.sh --instance gamma to create it\)/); + assert.ok(!r.err.includes(cfgOf(home, "gamma")) && !r.err.includes(home), r.err); }); await t.test("an entry point takes --instance in any position, once, and refuses a mismatch before it prints", () => { diff --git a/packages/control-board/src/scan.mjs b/packages/control-board/src/scan.mjs index 9854812b2..d32bea84b 100644 --- a/packages/control-board/src/scan.mjs +++ b/packages/control-board/src/scan.mjs @@ -32,17 +32,18 @@ export function defaultConfigPath() { } // Fail closed: the config must exist, parse, and name an absolute dataRoot. +// Refusals name the config, never its path (#1873). export function loadConfig(path = defaultConfigPath()) { - if (!existsSync(path)) throw new ConfigError(`config not found: ${path}`); + if (!existsSync(path)) throw new ConfigError("config not found"); let raw; try { raw = parseJsonFileText(readFileSync(path, "utf8"), path); } catch (error) { - throw new ConfigError(`config is not valid JSON: ${path}${error.code ? ` (${error.code})` : ""}`); + throw new ConfigError(`config is not valid JSON${error.code ? ` (${error.code})` : ""}`); } - if (!raw || typeof raw !== "object" || Array.isArray(raw)) throw new ConfigError(`config is not an object: ${path}`); + if (!raw || typeof raw !== "object" || Array.isArray(raw)) throw new ConfigError("config is not an object"); if (typeof raw.dataRoot !== "string" || !isAbsolute(raw.dataRoot)) { - throw new ConfigError(`config.dataRoot must be an absolute path: ${path}`); + throw new ConfigError("config.dataRoot must be an absolute path"); } return { dataRoot: raw.dataRoot }; } @@ -230,11 +231,11 @@ export function loadSeen(boardDir) { try { parsed = parseJsonFileText(readFileSync(path, "utf8"), path); } catch (error) { - throw new ConfigError(`seen marks file is not valid JSON: ${path}${error.code ? ` (${error.code})` : ""}`); + throw new ConfigError(`seen marks file seen.json is not valid JSON${error.code ? ` (${error.code})` : ""}`); } - if (!parsed || typeof parsed !== "object" || Array.isArray(parsed)) throw new ConfigError(`seen marks file must be a JSON object: ${path}`); + if (!parsed || typeof parsed !== "object" || Array.isArray(parsed)) throw new ConfigError("seen marks file seen.json must be a JSON object"); for (const [k, v] of Object.entries(parsed)) { - if (typeof v !== "string") throw new ConfigError(`seen marks file has a non-string value for ${JSON.stringify(k)}: ${path}`); + if (typeof v !== "string") throw new ConfigError(`seen marks file seen.json has a non-string value for ${JSON.stringify(k)}`); } return parsed; } diff --git a/packages/control-board/tests/path-redaction-labels.test.mjs b/packages/control-board/tests/path-redaction-labels.test.mjs new file mode 100644 index 000000000..0953a6a96 --- /dev/null +++ b/packages/control-board/tests/path-redaction-labels.test.mjs @@ -0,0 +1,61 @@ +// #1873 (redact6): a control-board refusal names the config, the seen marks file or the seat +// by label, never a host path. The sentinel is a component of every scratch path, so a refusal +// that quotes a path quotes the sentinel too; each test also checks the scratch root and the label. +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { dirname, join } from 'node:path'; +import { ConfigError, loadConfig, loadRegistrations, loadSeen } from '../src/scan.mjs'; +import { registrationPath } from '../../seat/src/seat.mjs'; + +const secret = 'sk-test-NOT-A-SECRET-1234'; +function scratch(t) { + const root = mkdtempSync(join(tmpdir(), 'r6-')); + t.after(() => rmSync(root, { recursive: true, force: true })); + const dir = join(root, secret); + mkdirSync(dir); + return { root, dir }; +} +function clean(text, root) { + assert.ok(!text.includes(root), text); + assert.doesNotMatch(text, /sk-test|NOT-A-SECRET/); +} +function labelled(label, root) { + return error => { + assert.ok(error instanceof ConfigError, error.stack); + assert.equal(error.message, label); + clean(error.message, root); + return true; + }; +} +function write(file, text) { + mkdirSync(dirname(file), { recursive: true }); + writeFileSync(file, text); + return file; +} + +test('config refusals name the config, not its path', t => { + const { root, dir } = scratch(t); + assert.throws(() => loadConfig(join(dir, 'config.json')), labelled('config not found', root)); + assert.throws(() => loadConfig(write(join(dir, 'array.json'), '[]')), labelled('config is not an object', root)); + assert.throws(() => loadConfig(write(join(dir, 'relative.json'), '{"dataRoot":"data"}')), labelled('config.dataRoot must be an absolute path', root)); +}); + +test('seen marks refusals name seen.json, not its path', t => { + const { root, dir } = scratch(t); + write(join(dir, 'seen.json'), '[]'); + assert.throws(() => loadSeen(dir), labelled('seen marks file seen.json must be a JSON object', root)); + write(join(dir, 'seen.json'), '{"a":1}'); + assert.throws(() => loadSeen(dir), labelled('seen marks file seen.json has a non-string value for "a"', root)); +}); + +test('a bad registration is reported by layout and seat, never by its path', t => { + const { root, dir } = scratch(t); + write(registrationPath(dir, 'builder', 'repo'), '{}'); + const { registrations, errors } = loadRegistrations(dir); + assert.deepEqual(registrations, []); + assert.equal(errors.length, 1); + assert.match(errors[0], /^seat repo\/builder: .*: registration repo\/builder$/); + clean(errors[0], root); +}); diff --git a/packages/control-board/tests/refusal-redaction.test.mjs b/packages/control-board/tests/refusal-redaction.test.mjs index 171af624b..dbbcc5f59 100644 --- a/packages/control-board/tests/refusal-redaction.test.mjs +++ b/packages/control-board/tests/refusal-redaction.test.mjs @@ -6,10 +6,13 @@ import { dirname, join } from 'node:path'; const secret = 'sk-test-NOT-A-SECRET-1234'; // JSON.parse quotes a prefix near this fault on Node 26; check partial leaks too. const malformed = secret; +// #1873: the sentinel is a path component, so a refusal that quotes a path quotes it too. function scratch(t) { const root = mkdtempSync(join(tmpdir(), 'r3-')); t.after(() => rmSync(root, { recursive: true, force: true })); - return root; + const dir = join(root, secret); + mkdirSync(dir); + return dir; } function write(file, text = malformed) { mkdirSync(dirname(file), { recursive: true }); @@ -21,6 +24,7 @@ function safe(label, ErrorType, properties = {}) { assert.ok(error instanceof ErrorType); for (const [key, value] of Object.entries(properties)) assert.equal(error[key], value); assert.ok(error.message.includes(label), error.message); + assert.ok(!error.message.includes(tmpdir()), error.message); assert.doesNotMatch(error.message, /sk-test|Unexpected token/); assert.doesNotMatch(error.stack, /sk-test/); assert.equal(error.cause, undefined); @@ -30,13 +34,13 @@ function safe(label, ErrorType, properties = {}) { import { ConfigError, loadConfig, loadSeen } from '../src/scan.mjs'; for (const kind of ['config', 'seen']) test(`${kind} JSON refusal names only the file`, t => { const root = scratch(t), file = write(join(root, kind === 'config' ? 'config.json' : 'seen.json')); - assert.throws(() => kind === 'config' ? loadConfig(file) : loadSeen(root), safe(file, ConfigError)); + assert.throws(() => kind === 'config' ? loadConfig(file) : loadSeen(root), safe(kind === 'config' ? 'config is not valid JSON' : 'seen marks file seen.json is not valid JSON', ConfigError)); }); for (const kind of ['config', 'seen']) test(`${kind} read refusal retains the filesystem code`, t => { const root = scratch(t), file = join(root, kind === 'config' ? 'config.json' : 'seen.json'); mkdirSync(file); assert.throws(() => kind === 'config' ? loadConfig(file) : loadSeen(root), error => { - safe(file, ConfigError)(error); + safe(kind === 'config' ? 'config is not valid JSON' : 'seen marks file seen.json is not valid JSON', ConfigError)(error); assert.match(error.message, /\(EISDIR\)$/); assert.doesNotMatch(error.message, /illegal operation/); return true; diff --git a/packages/discord/src/approvals.mjs b/packages/discord/src/approvals.mjs index 516af3d65..f0027bbff 100644 --- a/packages/discord/src/approvals.mjs +++ b/packages/discord/src/approvals.mjs @@ -107,7 +107,7 @@ export function readApprovals(dir) { try { out.push(parseJsonFileText(line, `${path}:${i + 1}`)); } catch { - throw new DiscordError(`${path}:${i + 1}: not valid JSON`); + throw new DiscordError(`approvals.jsonl:${i + 1}: not valid JSON`); } } return out; diff --git a/packages/discord/src/binding.mjs b/packages/discord/src/binding.mjs index 6076f52ee..527b97b31 100644 --- a/packages/discord/src/binding.mjs +++ b/packages/discord/src/binding.mjs @@ -22,7 +22,7 @@ import { parseJsonFileText } from "../../organization/src/util.mjs"; import { existsSync, lstatSync, readFileSync, realpathSync, statSync } from "node:fs"; import { loadWebConfig } from "./web.mjs"; import { loadSetsparkConfig } from "./setspark.mjs"; -import { isAbsolute, join, resolve, sep } from "node:path"; +import { basename, isAbsolute, join, resolve, sep } from "node:path"; import { homedir } from "node:os"; import { DiscordError } from "./errors.mjs"; import { TOOL_DEFAULTS } from "./tools.mjs"; @@ -56,16 +56,17 @@ export function defaultConfigPath(env = process.env) { } // Only dataRoot is read here; scripts/mosaic-config.mjs owns full validation. +// Refusals name the config, never its path (#1873). export function loadDataRoot(path = defaultConfigPath()) { - if (!existsSync(path)) throw new DiscordError(`config not found: ${path}`); + if (!existsSync(path)) throw new DiscordError("config not found"); let raw; try { raw = parseJsonFileText(readFileSync(path, "utf8"), path); } catch (error) { - throw new DiscordError(`config is not valid JSON: ${path}${error.code ? ` (${error.code})` : ""}`); + throw new DiscordError(`config is not valid JSON${error.code ? ` (${error.code})` : ""}`); } - if (!raw || typeof raw !== "object" || Array.isArray(raw)) throw new DiscordError(`config is not an object: ${path}`); - if (typeof raw.dataRoot !== "string" || !isAbsolute(raw.dataRoot)) throw new DiscordError(`config.dataRoot must be an absolute path: ${path}`); + if (!raw || typeof raw !== "object" || Array.isArray(raw)) throw new DiscordError("config is not an object"); + if (typeof raw.dataRoot !== "string" || !isAbsolute(raw.dataRoot)) throw new DiscordError("config.dataRoot must be an absolute path"); return raw.dataRoot; } @@ -274,30 +275,35 @@ export function reloadDiff(current, next) { } // A private file: regular, not a symlink, owner-only (0600), non-empty. +// `what` is the label a refusal names; the path never appears (#1873). export function checkPrivateFile(path, what) { let st; try { st = lstatSync(path); } catch { - throw new DiscordError(`${what} not found: ${path}`); + throw new DiscordError(`${what} not found`); } - if (st.isSymbolicLink()) throw new DiscordError(`${what} must not be a symlink: ${path}`); - if (!st.isFile()) throw new DiscordError(`${what} is not a regular file: ${path}`); + if (st.isSymbolicLink()) throw new DiscordError(`${what} must not be a symlink`); + if (!st.isFile()) throw new DiscordError(`${what} is not a regular file`); const mode = st.mode & 0o777; - if (mode !== 0o600) throw new DiscordError(`${what} must be mode 0600, is ${mode.toString(8).padStart(4, "0")}: ${path}`); - if (st.size === 0) throw new DiscordError(`${what} is empty: ${path}`); + if (mode !== 0o600) throw new DiscordError(`${what} must be mode 0600, is ${mode.toString(8).padStart(4, "0")}`); + if (st.size === 0) throw new DiscordError(`${what} is empty`); return st; } +// The binding is named by its file's name when that is a valid binding name +// (bindingPath() makes it so), never by its path. export function loadBinding(path) { - checkPrivateFile(path, "binding"); + const name = basename(String(path), ".json"); + const label = BINDING_NAME.test(name) ? `binding ${name}` : "binding"; + checkPrivateFile(path, label); let raw; try { raw = parseJsonFileText(readFileSync(path, "utf8"), path); } catch (error) { - throw new DiscordError(`binding is not valid JSON: ${path}${error.code ? ` (${error.code})` : ""}`); + throw new DiscordError(`${label} is not valid JSON${error.code ? ` (${error.code})` : ""}`); } - return validateBinding(raw, `binding ${path}`); + return validateBinding(raw, label); } // The token is read once into memory and handed to the REST and gateway @@ -305,7 +311,7 @@ export function loadBinding(path) { export function readToken(binding) { checkPrivateFile(binding.tokenFile, "token file"); const token = readFileSync(binding.tokenFile, "utf8").trim(); - if (!/^[A-Za-z0-9._-]{20,}$/.test(token)) throw new DiscordError(`token file does not hold a bot token: ${binding.tokenFile}`); + if (!/^[A-Za-z0-9._-]{20,}$/.test(token)) throw new DiscordError("token file does not hold a bot token"); return token; } @@ -316,21 +322,23 @@ export function readToken(binding) { // Discord Sage (Q14, Q16). Every file must be a regular non-empty file. export function resolveContextFiles(binding, repo) { const root = realpathSync(repo); - return binding.context.files.map((f) => { + // Refusals name the entry by its index in context.files, never its path (#1873). + return binding.context.files.map((f, i) => { + const at = `context.files[${i}]`; if (typeof f !== "string" || f.length === 0) throw new DiscordError("context file must be a non-empty string"); - if (isAbsolute(f)) throw new DiscordError(`context file must be repository-relative: ${f}`); - if (f.split(/[\\/]/).includes("..")) throw new DiscordError(`context file must not escape the repository: ${f}`); + if (isAbsolute(f)) throw new DiscordError(`context file must be repository-relative: ${at}`); + if (f.split(/[\\/]/).includes("..")) throw new DiscordError(`context file must not escape the repository: ${at}`); const path = resolve(root, f); let st; try { st = lstatSync(path); } catch { - throw new DiscordError(`missing context file: ${path}`); + throw new DiscordError(`missing context file: ${at}`); } - if (st.isSymbolicLink()) throw new DiscordError(`context file must not be a symlink: ${path}`); - if (!st.isFile() || st.size === 0) throw new DiscordError(`context file is not a regular non-empty file: ${path}`); + if (st.isSymbolicLink()) throw new DiscordError(`context file must not be a symlink: ${at}`); + if (!st.isFile() || st.size === 0) throw new DiscordError(`context file is not a regular non-empty file: ${at}`); const real = realpathSync(path); - if (real !== path || !real.startsWith(root + sep)) throw new DiscordError(`context file resolves outside the repository: ${f}`); + if (real !== path || !real.startsWith(root + sep)) throw new DiscordError(`context file resolves outside the repository: ${at}`); return path; }); } @@ -369,12 +377,12 @@ export function resolveToolRoots(binding, { dataRoot }) { try { st = lstatSync(r.path); } catch { - throw new DiscordError(`tool root ${r.name} does not exist: ${r.path}`); + throw new DiscordError(`tool root ${r.name} does not exist`); } - if (st.isSymbolicLink()) throw new DiscordError(`tool root ${r.name} must not be a symlink: ${r.path}`); - if (!st.isDirectory()) throw new DiscordError(`tool root ${r.name} is not a directory: ${r.path}`); + if (st.isSymbolicLink()) throw new DiscordError(`tool root ${r.name} must not be a symlink`); + if (!st.isDirectory()) throw new DiscordError(`tool root ${r.name} is not a directory`); const real = realpathSync(r.path); - if (real === data || real.startsWith(data + sep) || data.startsWith(real + sep)) throw new DiscordError(`tool root ${r.name} overlaps the data root: ${r.path}`); + if (real === data || real.startsWith(data + sep) || data.startsWith(real + sep)) throw new DiscordError(`tool root ${r.name} overlaps the data root`); return { name: r.name, path: real, write: r.write, ...(r.git ? { git: { branch: r.git.branch, identity: r.git.identity, tokenFile: r.git.tokenFile, author: `${r.git.author.name} <${r.git.author.email}>`, ...(r.git.protocol ? { protocol: r.git.protocol } : {}) } } : {}) }; }); // setspark carries only the keys loadSetsparkConfig accepts; the extension diff --git a/packages/discord/src/cli.mjs b/packages/discord/src/cli.mjs index e291d8c03..fda919ba0 100755 --- a/packages/discord/src/cli.mjs +++ b/packages/discord/src/cli.mjs @@ -61,7 +61,7 @@ import { TOOLS_ENV, enabledToolNames } from "./tools.mjs"; import { createSetsparkApi } from "./setspark.mjs"; import { assembleContext } from "./context.mjs"; import { createConnector } from "./connector.mjs"; -import { ensureJournal, requestStop, requestProcessingStop, stopRequested, readPid, stopTarget, writePid, clearPid, unlock, recover, appendReload, BRAKE_EXIT } from "./journal.mjs"; +import { ensureJournal, requestStop, requestProcessingStop, stopRequested, readPid, stopTarget, writePid, clearPid, unlock, recover, appendReload, journalLabel, BRAKE_EXIT } from "./journal.mjs"; const USAGE = [ "usage: mosaic-discord check [--config PATH] [--repo PATH] [--offline]", @@ -135,7 +135,7 @@ function prepare(opts) { const contextFiles = resolveContextFiles(binding, opts.repo); const toolRoots = resolveToolRoots(binding, { dataRoot }); const pi = join(opts.repo, "node_modules", ".bin", "pi"); - if (!existsSync(pi)) throw new DiscordError(`pi not found at ${pi}; run npm ci in the repository`); + if (!existsSync(pi)) throw new DiscordError("pi not found at node_modules/.bin/pi in the repository; run npm ci in the repository"); const journalDir = bindingDataDir(dataRoot, binding.name); const sessionDir = join(dataRoot, "sessions", `discord-${binding.name}`); return { dataRoot, bindingFile, binding, contextFiles, toolRoots, pi, journalDir, sessionDir }; @@ -205,7 +205,7 @@ async function run(opts) { const outcome = recover(journalDir); if (outcome === "cleared") warn("supervised start: run.lock left by a process that is gone was removed"); } - if (stopRequested(journalDir)) throw new DiscordError(`STOP is present in ${journalDir}; remove it to run`, BRAKE_EXIT); + if (stopRequested(journalDir)) throw new DiscordError(`STOP is present in ${journalLabel(journalDir)}; remove it to run`, BRAKE_EXIT); writePid(journalDir, process.pid); let managed = null; const cleanupPid = () => { diff --git a/packages/discord/src/git.mjs b/packages/discord/src/git.mjs index 2351b4270..56105f7fa 100644 --- a/packages/discord/src/git.mjs +++ b/packages/discord/src/git.mjs @@ -139,12 +139,12 @@ function checkPrivateFile(path, what) { try { st = lstatSync(path); } catch { - throw new Error(`${what}: not found: ${path}`); + throw new Error(`${what}: not found`); } - if (st.isSymbolicLink()) throw new Error(`${what}: must not be a symlink: ${path}`); - if (!st.isFile()) throw new Error(`${what}: not a regular file: ${path}`); - if ((st.mode & 0o777) !== 0o600) throw new Error(`${what}: must be mode 0600: ${path}`); - if (st.size === 0) throw new Error(`${what}: is empty: ${path}`); + if (st.isSymbolicLink()) throw new Error(`${what}: must not be a symlink`); + if (!st.isFile()) throw new Error(`${what}: not a regular file`); + if ((st.mode & 0o777) !== 0o600) throw new Error(`${what}: must be mode 0600`); + if (st.size === 0) throw new Error(`${what}: is empty`); } // The environment every child sees. No global or system git configuration, diff --git a/packages/discord/src/journal.mjs b/packages/discord/src/journal.mjs index a8a31dde7..fa5e25cf7 100644 --- a/packages/discord/src/journal.mjs +++ b/packages/discord/src/journal.mjs @@ -25,8 +25,17 @@ import { appendFileSync, closeSync, constants, existsSync, fstatSync, fsyncSync, mkdirSync, openSync, readdirSync, readFileSync, readSync, renameSync, rmdirSync, rmSync, unlinkSync, writeFileSync, writeSync, } from "node:fs"; -import { join } from "node:path"; +import { basename, join } from "node:path"; import { DiscordError } from "./errors.mjs"; +import { BINDING_NAME } from "./binding.mjs"; + +// Where a refusal says a binding's journal is: its place under the data root +// when the directory carries a valid binding name (bindingDataDir() makes it +// so), never the host path (#1873). +export function journalLabel(dir) { + const name = basename(String(dir)); + return BINDING_NAME.test(name) ? `discord/${name} under the data root` : "the binding's journal directory"; +} export const OUTBOX_STATUS = Object.freeze(["intent", "confirmed", "refused", "unknown"]); @@ -50,7 +59,7 @@ function readLines(path) { try { out.push(parseJsonFileText(line, `${path}:${i + 1}`)); } catch { - throw new DiscordError(`${path}:${i + 1}: not valid JSON`); + throw new DiscordError(`${basename(path)}:${i + 1}: not valid JSON`); } } return out; @@ -119,7 +128,7 @@ export function writeTurn(dir, id, record) { try { fd = openSync(path, "wx", 0o600); } catch (err) { - if (err.code === "EEXIST") throw new DiscordError(`turn record already exists: ${path}`, 1); + if (err.code === "EEXIST") throw new DiscordError(`turn record already exists: ${id}`, 1); throw err; } try { @@ -396,7 +405,7 @@ function lockRefusal(dir, opts) { const existing = readPid(dir); const state = ownerState(existing, opts); if (state === "absent") return new DiscordError(`run.lock exists without an owner record: a start is in progress or was interrupted; ${UNLOCK_HINT}`, BRAKE_EXIT); - if (state === "invalid") return new DiscordError(`run.lock has an owner record that cannot be read; refusing. Inspect ${ownerPath(dir)} by hand`, BRAKE_EXIT); + if (state === "invalid") return new DiscordError(`run.lock has an owner record that cannot be read; refusing. Inspect run.lock/owner.json in ${journalLabel(dir)} by hand`, BRAKE_EXIT); if (state === "live") return new DiscordError(`another connector is running for this binding (pid ${existing.pid})`, BRAKE_EXIT); if (state === "unknown") return new DiscordError(`run.lock belongs to pid ${existing.pid}, which is alive but whose identity cannot be verified; refusing`, BRAKE_EXIT); return new DiscordError(`run.lock belongs to pid ${existing.pid}, which is gone or is a different process now; ${UNLOCK_HINT}`, BRAKE_EXIT); @@ -419,7 +428,7 @@ export function writePid(dir, pid, { now = Date.now(), identity = identityOf } = // this claim must not stand, however it interleaved with an unlock. if (stopRequested(dir)) { clearPid(dir, pid); - throw new DiscordError(`STOP is present in ${dir}; remove it to run`, BRAKE_EXIT); + throw new DiscordError(`STOP is present in ${journalLabel(dir)}; remove it to run`, BRAKE_EXIT); } } @@ -439,7 +448,7 @@ export function unlock(dir, { beforeRemove = null, identity = identityOf, reason const state = ownerState(rec, { identity }); if (state === "live") throw new DiscordError(`refusing to unlock: the connector is running (pid ${rec.pid}); use stop, and unlock only a lock whose owner is gone`, 1); if (state === "unknown") throw new DiscordError(`refusing to unlock: pid ${rec.pid} is alive and its identity cannot be verified; nothing removed. Stop that process first`, 1); - if (state === "invalid") throw new DiscordError(`refusing to unlock: the owner record cannot be read; nothing removed. Inspect ${ownerPath(dir)} by hand`, 1); + if (state === "invalid") throw new DiscordError(`refusing to unlock: the owner record cannot be read; nothing removed. Inspect run.lock/owner.json in ${journalLabel(dir)} by hand`, 1); if (beforeRemove) beforeRemove(); rmSync(lock, { recursive: true, force: true, maxRetries: 10, retryDelay: 20 }); return rec; @@ -468,14 +477,14 @@ export function unlock(dir, { beforeRemove = null, identity = identityOf, reason export function recover(dir, { identity = identityOf, beforeRemove = null } = {}) { const ours = (lines) => lines.length === 1 && lines[0].reason === RECOVER_REASON; if (stopRequested(dir)) { - if (!ours(readStop(dir))) throw new DiscordError(`STOP is present in ${dir}; the brake is on. Remove it to run`, BRAKE_EXIT); + if (!ours(readStop(dir))) throw new DiscordError(`STOP is present in ${journalLabel(dir)}; the brake is on. Remove it to run`, BRAKE_EXIT); clearStop(dir); } if (!existsSync(lockPath(dir))) return "clean"; const state = ownerState(readPid(dir), { identity }); if (state === "live" || state === "unknown" || state === "invalid") throw lockRefusal(dir, { identity }); unlock(dir, { identity, reason: RECOVER_REASON, beforeRemove }); - if (!ours(readStop(dir))) throw new DiscordError(`STOP was written by an operator while recovering ${dir}; the brake stays. Remove it to run`, BRAKE_EXIT); + if (!ours(readStop(dir))) throw new DiscordError(`STOP was written by an operator while recovering ${journalLabel(dir)}; the brake stays. Remove it to run`, BRAKE_EXIT); clearStop(dir); return "cleared"; } diff --git a/packages/discord/src/setspark.mjs b/packages/discord/src/setspark.mjs index 41b0c95de..729a58a0b 100644 --- a/packages/discord/src/setspark.mjs +++ b/packages/discord/src/setspark.mjs @@ -108,12 +108,12 @@ function checkPrivateFile(path, what) { try { st = lstatSync(path); } catch { - throw new Error(`${what}: not found: ${path}`); + throw new Error(`${what}: not found`); } - if (st.isSymbolicLink()) throw new Error(`${what}: must not be a symlink: ${path}`); - if (!st.isFile()) throw new Error(`${what}: not a regular file: ${path}`); - if ((st.mode & 0o777) !== 0o600) throw new Error(`${what}: must be mode 0600: ${path}`); - if (st.size === 0) throw new Error(`${what}: is empty: ${path}`); + if (st.isSymbolicLink()) throw new Error(`${what}: must not be a symlink`); + if (!st.isFile()) throw new Error(`${what}: not a regular file`); + if ((st.mode & 0o777) !== 0o600) throw new Error(`${what}: must be mode 0600`); + if (st.size === 0) throw new Error(`${what}: is empty`); } // Read the key for one call. The file is re-checked every time, so a key diff --git a/packages/discord/tests/path-redaction-labels.test.mjs b/packages/discord/tests/path-redaction-labels.test.mjs new file mode 100644 index 000000000..931f91b8e --- /dev/null +++ b/packages/discord/tests/path-redaction-labels.test.mjs @@ -0,0 +1,88 @@ +// #1873 (redact6): a discord refusal names the file, field or binding, never a host path. +// The sentinel is a component of every scratch path, so a refusal that quotes a path +// quotes the sentinel too; each test also checks the scratch root and the label. +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { mkdtempSync, mkdirSync, writeFileSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { DiscordError } from '../src/errors.mjs'; +import { loadDataRoot, readToken, resolveContextFiles, resolveToolRoots } from '../src/binding.mjs'; +import { ensureJournal, lockPath, ownerPath, recover, requestStop, unlock, writeTurn } from '../src/journal.mjs'; +import { loadGitConfig } from '../src/git.mjs'; +import { loadSetsparkConfig } from '../src/setspark.mjs'; + +const secret = 'sk-test-NOT-A-SECRET-1234'; +function scratch(t) { + const root = mkdtempSync(join(tmpdir(), 'r6-')); + t.after(() => rmSync(root, { recursive: true, force: true })); + const dir = join(root, secret); + mkdirSync(dir); + return { root, dir }; +} +function labelled(label, root, Type = DiscordError) { + return error => { + assert.ok(error instanceof Type, error.stack); + assert.ok(error.message.includes(label), error.message); + assert.ok(!error.message.includes(root), error.message); + assert.doesNotMatch(error.message, /sk-test|NOT-A-SECRET/); + return true; + }; +} + +test('config not found names the config, not its path', t => { + const { root, dir } = scratch(t); + assert.throws(() => loadDataRoot(join(dir, 'config.json')), labelled('config not found', root)); +}); + +test('token file refusals name the token file, not its path', t => { + const { root, dir } = scratch(t); + assert.throws(() => readToken({ tokenFile: join(dir, 'token') }), labelled('token file not found', root)); + const short = join(dir, 'short'); + writeFileSync(short, 'x', { mode: 0o600 }); + assert.throws(() => readToken({ tokenFile: short }), labelled('token file does not hold a bot token', root)); + const open = join(dir, 'open'); + writeFileSync(open, 'x'.repeat(30), { mode: 0o644 }); + assert.throws(() => readToken({ tokenFile: open }), labelled('token file must be mode 0600, is 0644', root)); +}); + +test('a context file refusal names its index in context.files', t => { + const { root, dir } = scratch(t); + const repo = join(dir, 'repo'); + mkdirSync(repo); + assert.throws(() => resolveContextFiles({ context: { files: [join(dir, 'abs.md')] } }, repo), labelled('context file must be repository-relative: context.files[0]', root)); + assert.throws(() => resolveContextFiles({ context: { files: ['ok.md', `${secret}.md`] } }, repo), labelled('missing context file: context.files[0]', root)); +}); + +test('a tool root refusal names the root, not its path', t => { + const { root, dir } = scratch(t); + const binding = { tools: { roots: [{ name: 'docs', path: join(dir, 'docs'), write: false }] } }; + assert.throws(() => resolveToolRoots(binding, { dataRoot: join(dir, 'data') }), labelled('tool root docs does not exist', root)); + mkdirSync(join(dir, 'data', 'docs'), { recursive: true }); + binding.tools.roots[0].path = join(dir, 'data', 'docs'); + assert.throws(() => resolveToolRoots(binding, { dataRoot: join(dir, 'data') }), labelled('tool root docs overlaps the data root', root)); +}); + +test('git and setspark key file refusals name the field', t => { + const { root, dir } = scratch(t); + const git = { branch: 'main', identity: 'sage', tokenFile: join(dir, 'gitea'), author: 'Sage ' }; + assert.throws(() => loadGitConfig(git, 'tools.roots[0].git'), labelled('tools.roots[0].git.tokenFile: not found', root, Error)); + const setspark = { baseUrl: 'https://setspark.example', keyFile: join(dir, 'key'), principal: 'sage' }; + assert.throws(() => loadSetsparkConfig(setspark), labelled('setspark.keyFile: not found', root, Error)); +}); + +for (const [name, label] of [['bot', 'discord/bot under the data root'], [secret, "the binding's journal directory"]]) { + test(`journal refusals name ${label}`, t => { + const { root, dir } = scratch(t); + const journal = join(dir, name); + ensureJournal(journal); + requestStop(journal, 'operator'); + assert.throws(() => recover(journal), labelled(`STOP is present in ${label}`, root)); + rmSync(join(journal, 'STOP')); + mkdirSync(lockPath(journal)); + writeFileSync(ownerPath(journal), secret, { mode: 0o600 }); + assert.throws(() => unlock(journal), labelled(`Inspect run.lock/owner.json in ${label} by hand`, root)); + writeTurn(journal, 't1', {}); + assert.throws(() => writeTurn(journal, 't1', {}), labelled('turn record already exists: t1', root)); + }); +} diff --git a/packages/discord/tests/refusal-redaction.test.mjs b/packages/discord/tests/refusal-redaction.test.mjs index 12ad349be..415628cf6 100644 --- a/packages/discord/tests/refusal-redaction.test.mjs +++ b/packages/discord/tests/refusal-redaction.test.mjs @@ -6,10 +6,13 @@ import { dirname, join } from 'node:path'; const secret = 'sk-test-NOT-A-SECRET-1234'; // JSON.parse quotes a prefix near this fault on Node 26; check partial leaks too. const malformed = secret; +// #1873: the sentinel is a path component, so a refusal that quotes a path quotes it too. function scratch(t) { const root = mkdtempSync(join(tmpdir(), 'r3-')); t.after(() => rmSync(root, { recursive: true, force: true })); - return root; + const dir = join(root, secret); + mkdirSync(dir); + return dir; } function write(file, text = malformed) { mkdirSync(dirname(file), { recursive: true }); @@ -21,6 +24,7 @@ function safe(label, ErrorType, properties = {}) { assert.ok(error instanceof ErrorType); for (const [key, value] of Object.entries(properties)) assert.equal(error[key], value); assert.ok(error.message.includes(label), error.message); + assert.ok(!error.message.includes(tmpdir()), error.message); assert.doesNotMatch(error.message, /sk-test|Unexpected token/); assert.doesNotMatch(error.stack, /sk-test/); assert.equal(error.cause, undefined); @@ -33,15 +37,15 @@ import { readApprovals } from '../src/approvals.mjs'; import { readInboxIds } from '../src/journal.mjs'; import extension from '../extension/tools.mjs'; import { TOOLS_ENV } from '../src/tools.mjs'; -for (const [name, read, relative] of [ - ['config', (file) => loadDataRoot(file), 'config.json'], - ['binding', (file) => loadBinding(file), 'binding.json'], - ['approvals', (_file, root) => readApprovals(root), 'approvals.jsonl'], - ['journal', (_file, root) => readInboxIds(root), 'inbox.jsonl'], +for (const [name, read, relative, label] of [ + ['config', (file) => loadDataRoot(file), 'config.json', 'config is not valid JSON'], + ['binding', (file) => loadBinding(file), 'binding.json', 'binding binding is not valid JSON'], + ['approvals', (_file, root) => readApprovals(root), 'approvals.jsonl', 'approvals.jsonl:2: not valid JSON'], + ['journal', (_file, root) => readInboxIds(root), 'inbox.jsonl', 'inbox.jsonl:2: not valid JSON'], ]) test(`${name} JSON refusal omits parser bytes`, t => { const root = scratch(t), ledger = name === 'approvals' || name === 'journal'; const file = write(join(root, relative), ledger ? '{}\n' + malformed : malformed); - assert.throws(() => read(file, root), safe(ledger ? `${file}:2` : file, DiscordError, { exitCode: 2 })); + assert.throws(() => read(file, root), safe(label, DiscordError, { exitCode: 2 })); }); test('tools environment JSON refusal names the config key without its bytes', () => { const previous = process.env[TOOLS_ENV]; @@ -52,7 +56,7 @@ test('tools environment JSON refusal names the config key without its bytes', () test('config read refusal retains the filesystem code', t => { const file = scratch(t); assert.throws(() => loadDataRoot(file), error => { - safe(file, DiscordError, { exitCode: 2 })(error); + safe('config is not valid JSON', DiscordError, { exitCode: 2 })(error); assert.match(error.message, /\(EISDIR\)$/); return true; }); diff --git a/packages/organization/src/config-command.mjs b/packages/organization/src/config-command.mjs index 2395f5aec..d75218f5b 100644 --- a/packages/organization/src/config-command.mjs +++ b/packages/organization/src/config-command.mjs @@ -40,6 +40,24 @@ export const ORG_CONFIG = Object.freeze({ usage: "usage: mosaic org config validate ID | resolve ID INSTANCE [--project ID]", }); +// A failed `mosaic-config.mjs validate` as one refusal line. Render known key paths with fixed +// constraints; never relay the child's stderr, which can quote a host path or a config value. +// The config is named by where it came from, never by host path (PR #1916): `label` when the +// caller knows it (the cli's --instance load, #1873), else from MOSAIC_CONFIG. +export function systemConfigProblem(proc, env, label = null) { + const problem = String(proc.stderr ?? "").trim(); + const field = problem.startsWith('mosaic-config: execution "dataRoot"') ? "dataRoot" + : /^mosaic-config: (?:unsupported )?"?((?:configVersion|environment|dataRoot|execution|console|federation|compactionReadBytes|compactionStreams)(?:\.[A-Za-z][A-Za-z0-9]*|\[\d+\])*)"?(?=[: ])/u.exec(problem)?.[1]; + const name = "a 1-200 character name starting with a letter or digit and containing only letters, digits, . _ : / -"; + const expected = { configVersion: "1", environment: "development or production", dataRoot: "a canonical absolute instance data path", + "execution.backend": "docker", "execution.adapter": "pi or mock", "execution.provider": name, "execution.model": name }; + const detail = problem.startsWith("mosaic-config: configuration is not valid JSON (") + ? "configuration is not valid JSON; expected readable JSON: " + : field ? `${field}: expected ${expected[field] ?? "a value satisfying the system configuration schema"}: ` + : "expected valid system configuration: "; + return `system config problem (mosaic-config exit ${proc.status}): ${detail}${label ?? (env.MOSAIC_CONFIG ? "the system config MOSAIC_CONFIG names" : "the default system config")}${proc.error?.code ? ` (${proc.error.code})` : ""}`; +} + // Thrown after the message is written; runConfigCommand turns it into the exit code. class Exit { constructor(code) { @@ -67,21 +85,7 @@ export function runConfigCommand(argv, { stdout = process.stdout, stderr = proce maxBuffer: 1024 * 1024, env, }); - // Render known key paths with fixed constraints; never relay child stderr. - if (proc.status !== 0) { - const problem = proc.stderr.trim(); - const field = problem.startsWith('mosaic-config: execution "dataRoot"') ? "dataRoot" - : /^mosaic-config: (?:unsupported )?"?((?:configVersion|environment|dataRoot|execution|console|federation|compactionReadBytes|compactionStreams)(?:\.[A-Za-z][A-Za-z0-9]*|\[\d+\])*)"?(?=[: ])/u.exec(problem)?.[1]; - const name = "a 1-200 character name starting with a letter or digit and containing only letters, digits, . _ : / -"; - const expected = { configVersion: "1", environment: "development or production", dataRoot: "a canonical absolute instance data path", - "execution.backend": "docker", "execution.adapter": "pi or mock", "execution.provider": name, "execution.model": name }; - const detail = problem.startsWith("mosaic-config: configuration is not valid JSON (") - ? "configuration is not valid JSON; expected readable JSON: " - : field ? `${field}: expected ${expected[field] ?? "a value satisfying the system configuration schema"}: ` - : "expected valid system configuration: "; - // Name the config by where it came from, never by host path (PR #1916). - fail(3, `system config problem (mosaic-config exit ${proc.status}): ${detail}${env.MOSAIC_CONFIG ? "the system config MOSAIC_CONFIG names" : "the default system config"}${proc.error?.code ? ` (${proc.error.code})` : ""}`); - } + if (proc.status !== 0) fail(3, systemConfigProblem(proc, env)); try { return JSON.parse(proc.stdout); } catch { @@ -90,19 +94,23 @@ export function runConfigCommand(argv, { stdout = process.stdout, stderr = proce }; const load = (id) => loadOrganization(id, { dir: configDir(env), rolesDir: resolve(env.MOSAIC_ROLES_DIR || join(REPO, "roles")) }); - // Check credential references; return the problems and warnings. + // Check credential references, each a [field, ref] pair; return the problems + // and warnings. They name the field and the roots by label, never a path (#1873). const checkRefs = (refs, forbiddenRoots) => { const problems = []; const warnings = []; - for (const ref of refs) { - const result = checkCredentialRef(ref, { forbiddenRoots, env }); + for (const [where, ref] of refs) { + const result = checkCredentialRef(ref, { forbiddenRoots, env, where }); problems.push(...result.problems); warnings.push(...result.warnings); } return { problems, warnings }; }; - const forbiddenRoots = (config, organization) => - [REPO, config.dataRoot, ...Object.values(organization.projects).filter((p) => p.kind !== "library").map((p) => p.root)]; + const forbiddenRoots = (config, organization) => [ + { path: REPO, label: "the repository" }, + { path: config.dataRoot, label: "the data root" }, + ...Object.entries(organization.projects).filter(([, p]) => p.kind !== "library").map(([id, p]) => ({ path: p.root, label: `the root of project ${id}` })), + ]; const validate = (args) => { if (args.length !== 1) usage(); @@ -125,8 +133,8 @@ export function runConfigCommand(argv, { stdout = process.stdout, stderr = proce projects[id] = "valid"; } const refs = [ - ...Object.values(organization.tracker?.sync?.credentials ?? {}), - ...Object.values(organization.roles).flatMap((r) => Object.values(r.credentials)), + ...Object.entries(organization.tracker?.sync?.credentials ?? {}).map(([service, ref]) => [`tracker.sync.credentials.${service}`, ref]), + ...Object.entries(organization.roles).flatMap(([instance, r]) => Object.entries(r.credentials).map(([service, ref]) => [`roles.${instance}.credentials.${service}`, ref])), ]; const checked = checkRefs(refs, forbiddenRoots(config, organization)); warnings.push(...checked.warnings); @@ -163,7 +171,7 @@ export function runConfigCommand(argv, { stdout = process.stdout, stderr = proce if (entry.kind !== "library") loaded = loadProject(entry.root); } const resolved = resolveInstance({ system: systemVars(config), business: organization, project: loaded, instance }); - const checked = checkRefs(Object.values(resolved.credentials), forbiddenRoots(config, organization)); + const checked = checkRefs(Object.entries(resolved.credentials).map(([service, ref]) => [`${instance} ${service}`, ref]), forbiddenRoots(config, organization)); for (const w of checked.warnings) say(`warning: ${w}`); if (checked.problems.length > 0) { for (const p of checked.problems) say(p); diff --git a/packages/organization/src/credentials.mjs b/packages/organization/src/credentials.mjs index 02e53e219..9958bcc80 100644 --- a/packages/organization/src/credentials.mjs +++ b/packages/organization/src/credentials.mjs @@ -44,7 +44,7 @@ export function parseCredentialRef(ref, service, where, { store = null } = {}) { } out.file = ref.file; if (store && inside(join(store.configDir, "secrets", "agents"), ref.file) && ref.file !== storePath(store, service)) { - refuse(`${where}.file credential-not-yours: ${ref.file} is in the agents' store but not this agent's path`); + refuse(`${where}.file credential-not-yours: the file is in the agents' store but not this agent's path`); } } else { if (typeof ref.env !== "string" || !ENV_NAME.test(ref.env)) refuse(`${where}.env must match ${ENV_NAME}`); @@ -68,11 +68,16 @@ function inside(root, path) { // Check a parsed reference against the filesystem and the calendar. // Returns { problems: [...], warnings: [...] }; a caller that finds any // problem refuses. `forbiddenRoots` are directories a token file must not -// sit in (the repository, dataRoot). `now` is a Date. -export function checkCredentialRef(ref, { forbiddenRoots = [], now = new Date(), env = process.env, uid = process.getuid() } = {}) { +// sit in (the repository, dataRoot): each a path, or { path, label } so a +// problem can name it. `where` is the reference's field (for example +// roles.coder.credentials.gitea). `now` is a Date. +// A problem or warning names the reference by its field, its service and its +// kind, and an env reference by its checked variable name. It never quotes the +// token file's path or a root's path (#1873). +export function checkCredentialRef(ref, { forbiddenRoots = [], now = new Date(), env = process.env, uid = process.getuid(), where = null } = {}) { const problems = []; const warnings = []; - const label = `${ref.service} ${ref.file ? `file ${ref.file}` : `env ${ref.env}`}`; + const label = `${where ?? ref.service} ${ref.store ? "store file" : ref.file ? "file" : `env ${ref.env}`}`; let notIssued = false; if (ref.file) { let stat = null; @@ -96,14 +101,16 @@ export function checkCredentialRef(ref, { forbiddenRoots = [], now = new Date(), } catch { problems.push(`${label}: path can't be resolved`); } - for (const root of forbiddenRoots) { + for (const entry of forbiddenRoots) { + const root = typeof entry === "string" ? entry : entry.path; + const name = typeof entry === "string" ? "a forbidden root" : entry.label; let realRoot = root; try { realRoot = realpathSync(root); } catch { // A root that doesn't exist yet can't contain the file. } - if (inside(realRoot, real)) problems.push(`${label}: inside ${root}; token files live outside the repository and dataRoot`); + if (inside(realRoot, real)) problems.push(`${label}: inside ${name}; token files live outside the repository and dataRoot`); } } } diff --git a/packages/organization/src/org-registry.mjs b/packages/organization/src/org-registry.mjs index 6bd757909..61188717e 100644 --- a/packages/organization/src/org-registry.mjs +++ b/packages/organization/src/org-registry.mjs @@ -6,8 +6,26 @@ import { processState } from '../../seat/src/process-state.mjs'; import { ID_PATTERN } from './vocabulary.mjs'; import { parseJsonFileText } from './util.mjs'; +// `path` is kept only when it is a document field ($.projects.alpha.root): a +// host path never rides on the error, where a caller could print or serialise +// it (#1873). export class OrgError extends Error { - constructor(code, message, path) { super(message); this.name = 'OrgError'; this.code = code; if (path) this.path = path; } + constructor(code, message, path) { super(message); this.name = 'OrgError'; this.code = code; if (typeof path === 'string' && path.startsWith('$')) this.path = path; } +} +// A Node system error names the host path in its message, and a spawn's syscall names the +// program. quietMessage is the call's first word, the file's label and the error code, or null +// for an error that is not a system error; quiet puts it in the message. The code and syscall +// fields stay (#1873, from PR #1916's org-service quiet). +const quieted = new WeakSet(); +export function quietMessage(e, what) { + if (!e || typeof e !== 'object') return null; + if (quieted.has(e)) return e.message; + return typeof e.syscall === 'string' && typeof e.code === 'string' ? `cannot ${e.syscall.split(' ')[0]} ${what} (${e.code})` : null; +} +export function quiet(e, what) { + const message = quietMessage(e, what); + if (message !== null && !quieted.has(e)) { e.message = message; quieted.add(e); } + return e; } const fail = (code, message, path) => { throw new OrgError(code, message, path); }; function idCheck(id) { if (typeof id !== 'string' || !ID_PATTERN.test(id)) fail('org-invalid', 'invalid organization id'); } @@ -33,7 +51,7 @@ function validateRegistry(doc, file) { } export async function readRegistry({ configDir }) { await safeDir(configDir); await privateDir(configDir); const file = join(configDir, 'org-runtime.json'); let doc; - try { doc = await readPrivate(file); } catch (e) { if (e.code === 'ENOENT') return null; if (e instanceof OrgError) throw e; fail('org-mismatch', `invalid registry: ${e.message}`, file); } + try { doc = await readPrivate(file); } catch (e) { if (e.code === 'ENOENT') return null; if (e instanceof OrgError) throw e; fail('org-mismatch', `invalid registry: ${e instanceof SyntaxError ? e.message : quiet(e, 'registry file').message}`); } return validateRegistry(doc, file); } export async function listOrgs(ctx) { const doc = await readRegistry(ctx); return doc ? Object.values(doc.orgs) : []; } @@ -185,7 +203,7 @@ export async function orgLock(ctx, id, { clear = false } = {}) { if (e.code !== 'EEXIST') throw e; const holder = inspectRecoveryFence(guard); if (!holder) continue; - if (holder.unreadable) fail('org-locked', `unreadable recovery fence ${guard}; remove it by hand`, guard); + if (holder.unreadable) fail('org-locked', `unreadable recovery fence org-lifecycle-${id}.lock.clear in the config directory; remove it by hand`); if (holder.alive !== false) fail('org-locked', `lock recovery is already in progress by pid ${holder.pid}`, guard); let current; try { current = lstatSync(guard); } catch (err) { if (err.code === 'ENOENT') continue; throw err; } if (current.dev !== holder.dev || current.ino !== holder.ino) fail('org-locked', 'recovery fence changed during recovery', guard); diff --git a/packages/organization/src/org-service.mjs b/packages/organization/src/org-service.mjs index 4b384d9b6..161ad969b 100644 --- a/packages/organization/src/org-service.mjs +++ b/packages/organization/src/org-service.mjs @@ -6,7 +6,7 @@ import { dirname, join, resolve } from 'node:path'; import { fileURLToPath } from 'node:url'; import { validateBusinessDocument, httpsUrl } from './organization-config.mjs'; import { ID_PATTERN } from './vocabulary.mjs'; -import { OrgError, orgRoot, readRegistry, reserveOrg, resolveOrg, withRegistry, withOrgLifecycle } from './org-registry.mjs'; +import { OrgError, orgRoot, quiet, readRegistry, reserveOrg, resolveOrg, withRegistry, withOrgLifecycle } from './org-registry.mjs'; import { loadInstalled } from '../../plugins/src/loader.mjs'; import { BINDING_NAME } from '../../discord/src/binding.mjs'; import { parseJsonFileText } from './util.mjs'; @@ -21,11 +21,6 @@ function authorize(ctx) { if (!ctx || typeof ctx.instance !== 'string' || !/^[a- function checkId(id) { if (typeof id !== 'string' || !ID_PATTERN.test(id)) fail('org-invalid', 'invalid id'); } // A filesystem error names the operation, the file by label and the error code, never the // host path Node puts in its message (PR #1916). code, syscall and the other fields stay. -const quieted = new WeakSet(); -function quiet(e, what) { - if (e && typeof e === 'object' && typeof e.syscall === 'string' && typeof e.code === 'string' && !quieted.has(e)) { e.message = `cannot ${e.syscall} ${what} (${e.code})`; quieted.add(e); } - return e; -} async function privateRead(file, what = 'organization file') { let fd; try { fd = await fs.open(file, constants.O_RDONLY | constants.O_NOFOLLOW | constants.O_NONBLOCK); } catch (e) { throw quiet(e, what); } try { const st = await fd.stat(); if (!st.isFile() || st.uid !== process.getuid() || (st.mode & 0o077)) fail('org-mismatch', 'unsafe file', file); return await fd.readFile('utf8'); } catch (e) { throw quiet(e, what); } finally { await fd.close(); } } async function atomic(file, content, exclusive = false) { const tmp = join(dirname(file), `.org-write-${randomBytes(12).toString('hex')}`); diff --git a/packages/organization/tests/path-redaction-labels.test.mjs b/packages/organization/tests/path-redaction-labels.test.mjs new file mode 100644 index 000000000..047b5f199 --- /dev/null +++ b/packages/organization/tests/path-redaction-labels.test.mjs @@ -0,0 +1,90 @@ +// #1873 (redact6): an organization refusal names the field, the file or the root by label, +// never a host path. The sentinel is a component of every scratch path, so a refusal that +// quotes a path quotes the sentinel too; each test also checks the scratch root and the label. +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { chmodSync, mkdirSync, mkdtempSync, rmSync, symlinkSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { checkCredentialRef, parseCredentialRef, storePath } from '../src/credentials.mjs'; +import { OrgError, orgLock, quietMessage, readRegistry } from '../src/org-registry.mjs'; + +const secret = 'sk-test-NOT-A-SECRET-1234'; +function scratch(t) { + const root = mkdtempSync(join(tmpdir(), 'r6-')); + t.after(() => rmSync(root, { recursive: true, force: true })); + const dir = join(root, secret); + mkdirSync(dir, { mode: 0o700 }); + return { root, dir }; +} +function clean(text, root) { + assert.ok(!text.includes(root), text); + assert.doesNotMatch(text, /sk-test|NOT-A-SECRET/); +} + +test('a credential problem names the field and the root by label, never the token file or the root', t => { + const { root, dir } = scratch(t); + const file = join(dir, 'gitea'); + writeFileSync(file, 'token', { mode: 0o644 }); + const ref = { service: 'gitea', file, rotateBy: '2999-01-01' }; + const { problems } = checkCredentialRef(ref, { forbiddenRoots: [{ path: dir, label: 'the data root' }, root], where: 'roles.coder.credentials.gitea' }); + assert.deepEqual(problems, [ + 'roles.coder.credentials.gitea file: mode 644 gives group or other access; use 600', + 'roles.coder.credentials.gitea file: inside the data root; token files live outside the repository and dataRoot', + 'roles.coder.credentials.gitea file: inside a forbidden root; token files live outside the repository and dataRoot', + ]); + for (const p of problems) clean(p, root); + const missing = checkCredentialRef({ service: 'gitea', file: join(dir, 'absent'), rotateBy: '2999-01-01' }); + assert.deepEqual(missing.problems, ['gitea file: not found']); + const store = checkCredentialRef({ service: 'gitea', store: true, file: join(dir, 'absent'), rotateBy: '2999-01-01' }, { where: 'roles.coder.credentials.gitea' }); + assert.deepEqual(store.warnings, ['roles.coder.credentials.gitea store file: not issued']); +}); + +test('credential-not-yours names the field, not the file', t => { + const { root, dir } = scratch(t); + const store = { configDir: dir, business: 'acme', agent: 'coder' }; + const other = storePath({ ...store, agent: 'pm' }, 'gitea'); + assert.throws(() => parseCredentialRef({ file: other, rotateBy: '2999-01-01' }, 'gitea', 'roles.coder.credentials.gitea', { store }), error => { + assert.match(error.message, /^roles\.coder\.credentials\.gitea\.file credential-not-yours: the file is in the agents' store/); + clean(error.message, root); + return true; + }); +}); + +test('an OrgError keeps a document field but drops a host path', t => { + const { dir } = scratch(t); + assert.equal(new OrgError('org-mismatch', 'unsafe file', join(dir, 'org.json')).path, undefined); + assert.equal(new OrgError('org-invalid', 'bad root', '$.projects.alpha.root').path, '$.projects.alpha.root'); + const error = Object.assign(new Error(`ENOENT: no such file or directory, open '${join(dir, 'x')}'`), { code: 'ENOENT', syscall: 'open' }); + assert.equal(quietMessage(error, 'the registry file'), 'cannot open the registry file (ENOENT)'); + assert.equal(quietMessage(Object.assign(new Error('x'), { code: 'ENOENT', syscall: `spawn ${join(dir, 'git')}` }), 'git'), 'cannot spawn git (ENOENT)'); +}); + +test('an unreadable registry names the registry file and the error code, never its path', async t => { + const { root, dir } = scratch(t); + writeFileSync(join(dir, 'real.json'), '{}', { mode: 0o600 }); + symlinkSync(join(dir, 'real.json'), join(dir, 'org-runtime.json')); + await assert.rejects(readRegistry({ configDir: dir }), error => { + assert.ok(error instanceof OrgError, error.stack); + assert.equal(error.code, 'org-mismatch'); + assert.equal(error.message, 'invalid registry: cannot open registry file (ELOOP)'); + assert.equal(error.path, undefined); + clean(error.message, root); + clean(JSON.stringify(error), root); + return true; + }); +}); + +test('an unreadable recovery fence names the fence in the config directory, never its path', async t => { + const { root, dir } = scratch(t); + writeFileSync(join(dir, 'org-lifecycle-acme.lock.clear'), '{}', { mode: 0o600 }); + chmodSync(join(dir, 'org-lifecycle-acme.lock.clear'), 0o644); + await assert.rejects(orgLock({ configDir: dir }, 'acme', { clear: true }), error => { + assert.ok(error instanceof OrgError, error.stack); + assert.equal(error.code, 'org-locked'); + assert.equal(error.message, 'unreadable recovery fence org-lifecycle-acme.lock.clear in the config directory; remove it by hand'); + assert.equal(error.path, undefined); + clean(error.message, root); + return true; + }); +}); diff --git a/packages/seat/src/seat.mjs b/packages/seat/src/seat.mjs index 6549daeaf..dd0a827f3 100644 --- a/packages/seat/src/seat.mjs +++ b/packages/seat/src/seat.mjs @@ -50,16 +50,17 @@ export function defaultConfigPath(env = process.env) { // Fail closed: the config must exist, parse, and name an absolute dataRoot. // Only dataRoot is read here; scripts/mosaic-config.mjs owns full validation. +// Refusals name the config, never its path (#1873). export function loadDataRoot(path = defaultConfigPath()) { - if (!existsSync(path)) throw new SeatError(`config not found: ${path}`); + if (!existsSync(path)) throw new SeatError("config not found"); let raw; try { raw = parseJsonFileText(readFileSync(path, "utf8"), path); } catch (error) { - throw new SeatError(`config is not valid JSON: ${path}${error.code ? ` (${error.code})` : ""}`); + throw new SeatError(`config is not valid JSON${error.code ? ` (${error.code})` : ""}`); } - if (!raw || typeof raw !== "object" || Array.isArray(raw)) throw new SeatError(`config is not an object: ${path}`); - if (typeof raw.dataRoot !== "string" || !isAbsolute(raw.dataRoot)) throw new SeatError(`config.dataRoot must be an absolute path: ${path}`); + if (!raw || typeof raw !== "object" || Array.isArray(raw)) throw new SeatError("config is not an object"); + if (typeof raw.dataRoot !== "string" || !isAbsolute(raw.dataRoot)) throw new SeatError("config.dataRoot must be an absolute path"); return raw.dataRoot; } @@ -87,10 +88,11 @@ export function resolveSeat(arg, { repo = process.cwd() } = {}) { else throw new SeatError(`invalid seat name: ${JSON.stringify(arg)}`, 4); const seat = basename(seatDir); if (!SEAT_NAME.test(seat)) throw new SeatError(`invalid seat name: ${JSON.stringify(seat)}`, 4); - if (!existsSync(seatDir) || !statSync(seatDir).isDirectory()) throw new SeatError(`no such seat directory: ${seatDir}`); + // Named by the checked seat name, never the directory's path (#1873). + if (!existsSync(seatDir) || !statSync(seatDir).isDirectory()) throw new SeatError(`no such seat directory: seat ${seat}`); const launchScript = join(seatDir, "launch.sh"); - if (!existsSync(launchScript) || !statSync(launchScript).isFile()) throw new SeatError(`seat has no launch.sh: ${seatDir}`); - if ((statSync(launchScript).mode & 0o111) === 0) throw new SeatError(`launch script is not executable: ${launchScript}`); + if (!existsSync(launchScript) || !statSync(launchScript).isFile()) throw new SeatError(`seat has no launch.sh: seat ${seat}`); + if ((statSync(launchScript).mode & 0o111) === 0) throw new SeatError(`launch script is not executable: launch.sh of seat ${seat}`); const parent = dirname(seatDir); const root = basename(parent) === "agents" ? dirname(parent) : null; @@ -205,14 +207,14 @@ export function readRegistration(seats, seat, layout) { try { parsed = parseJsonFileText(readFileSync(path, "utf8"), path); } catch (error) { - throw new SeatError(`registration is not valid JSON: ${path}${error.code ? ` (${error.code})` : ""}`); + throw new SeatError(`registration ${layout}/${seat} is not valid JSON${error.code ? ` (${error.code})` : ""}`); } try { const record = validateRegistration(parsed); if (record.seat !== seat || record.layout !== layout) throw new SeatError("registration does not match its path"); return record; } catch (err) { - throw new SeatError(`${err.message}: ${path}`); + throw new SeatError(`${err.message}: registration ${layout}/${seat}`); } } diff --git a/packages/seat/src/session.mjs b/packages/seat/src/session.mjs index 060542a3e..7d1c49d8d 100644 --- a/packages/seat/src/session.mjs +++ b/packages/seat/src/session.mjs @@ -399,9 +399,10 @@ export function readSessions(dataRoot) { try { doc = JSON.parse(readFileSync(file, "utf8")); } catch { - throw new SeatError(`sessions file is unreadable: ${file}`, 2); + throw new SeatError("sessions file is unreadable: bus-host/sessions.json under the data root", 2); } - if (!SESSIONS_READ.includes(doc?.sessionsVersion) || !Array.isArray(doc.sessions)) throw new SeatError(`sessions file is malformed: ${file}`, 2); + // #1873: named by its fixed place under the data root, never by host path. + if (!SESSIONS_READ.includes(doc?.sessionsVersion) || !Array.isArray(doc.sessions)) throw new SeatError("sessions file is malformed: bus-host/sessions.json under the data root", 2); return doc.sessions.map((s) => ({ ...s, live: startTimeOf(s.runnerPid) === s.runnerStartTime })); } diff --git a/packages/seat/tests/path-redaction-labels.test.mjs b/packages/seat/tests/path-redaction-labels.test.mjs new file mode 100644 index 000000000..99976a8bf --- /dev/null +++ b/packages/seat/tests/path-redaction-labels.test.mjs @@ -0,0 +1,70 @@ +// #1873 (redact6): a seat refusal names the config, the seat or the registration by label, +// never a host path. The sentinel is a component of every scratch path, so a refusal that +// quotes a path quotes the sentinel too; each test also checks the scratch root and the label. +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { chmodSync, mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { dirname, join } from 'node:path'; +import { SeatError, loadDataRoot, readRegistration, registrationPath, resolveSeat } from '../src/seat.mjs'; +import { readSessions, sessionsFile } from '../src/session.mjs'; + +const secret = 'sk-test-NOT-A-SECRET-1234'; +function scratch(t) { + const root = mkdtempSync(join(tmpdir(), 'r6-')); + t.after(() => rmSync(root, { recursive: true, force: true })); + const dir = join(root, secret); + mkdirSync(dir); + return { root, dir }; +} +function labelled(label, root) { + return error => { + assert.ok(error instanceof SeatError, error.stack); + assert.equal(error.message, label); + assert.ok(!error.message.includes(root), error.message); + assert.doesNotMatch(error.message, /sk-test|NOT-A-SECRET/); + return true; + }; +} +function write(file, text) { + mkdirSync(dirname(file), { recursive: true }); + writeFileSync(file, text); + return file; +} + +test('config refusals name the config, not its path', t => { + const { root, dir } = scratch(t); + assert.throws(() => loadDataRoot(join(dir, 'config.json')), labelled('config not found', root)); + assert.throws(() => loadDataRoot(write(join(dir, 'array.json'), '[]')), labelled('config is not an object', root)); + assert.throws(() => loadDataRoot(write(join(dir, 'relative.json'), '{"dataRoot":"data"}')), labelled('config.dataRoot must be an absolute path', root)); +}); + +test('seat directory refusals name the seat, not its directory', t => { + const { root, dir } = scratch(t); + assert.throws(() => resolveSeat(join(dir, 'agents', 'builder')), labelled('no such seat directory: seat builder', root)); + mkdirSync(join(dir, 'agents', 'builder'), { recursive: true }); + assert.throws(() => resolveSeat('builder', { repo: dir }), labelled('seat has no launch.sh: seat builder', root)); + write(join(dir, 'agents', 'builder', 'launch.sh'), '#!/bin/sh\n'); + chmodSync(join(dir, 'agents', 'builder', 'launch.sh'), 0o644); + assert.throws(() => resolveSeat('builder', { repo: dir }), labelled('launch script is not executable: launch.sh of seat builder', root)); +}); + +test('a registration that does not validate is named by its layout and seat', t => { + const { root, dir } = scratch(t); + write(registrationPath(dir, 'builder', 'repo'), '{}'); + assert.throws(() => readRegistration(dir, 'builder', 'repo'), error => { + assert.ok(error instanceof SeatError, error.stack); + assert.match(error.message, /: registration repo\/builder$/); + assert.ok(!error.message.includes(root), error.message); + assert.doesNotMatch(error.message, /sk-test|NOT-A-SECRET/); + return true; + }); +}); + +test('an unreadable or malformed sessions file is named by its place under the data root', t => { + const { root, dir } = scratch(t); + write(sessionsFile(dir), 'not json'); + assert.throws(() => readSessions(dir), labelled('sessions file is unreadable: bus-host/sessions.json under the data root', root)); + write(sessionsFile(dir), '{"sessionsVersion":0}'); + assert.throws(() => readSessions(dir), labelled('sessions file is malformed: bus-host/sessions.json under the data root', root)); +}); diff --git a/packages/seat/tests/refusal-redaction.test.mjs b/packages/seat/tests/refusal-redaction.test.mjs index 74f9d96f7..9d340b7c9 100644 --- a/packages/seat/tests/refusal-redaction.test.mjs +++ b/packages/seat/tests/refusal-redaction.test.mjs @@ -6,10 +6,13 @@ import { dirname, join } from 'node:path'; const secret = 'sk-test-NOT-A-SECRET-1234'; // JSON.parse quotes a prefix near this fault on Node 26; check partial leaks too. const malformed = secret; +// #1873: the sentinel is a path component, so a refusal that quotes a path quotes it too. function scratch(t) { const root = mkdtempSync(join(tmpdir(), 'r3-')); t.after(() => rmSync(root, { recursive: true, force: true })); - return root; + const dir = join(root, secret); + mkdirSync(dir); + return dir; } function write(file, text = malformed) { mkdirSync(dirname(file), { recursive: true }); @@ -21,6 +24,7 @@ function safe(label, ErrorType, properties = {}) { assert.ok(error instanceof ErrorType); for (const [key, value] of Object.entries(properties)) assert.equal(error[key], value); assert.ok(error.message.includes(label), error.message); + assert.ok(!error.message.includes(tmpdir()), error.message); assert.doesNotMatch(error.message, /sk-test|Unexpected token/); assert.doesNotMatch(error.stack, /sk-test/); assert.equal(error.cause, undefined); @@ -30,13 +34,13 @@ function safe(label, ErrorType, properties = {}) { import { SeatError, loadDataRoot, readRegistration, registrationPath } from '../src/seat.mjs'; for (const kind of ['config', 'registration']) test(`${kind} JSON refusal names only the file`, t => { const root = scratch(t), file = write(kind === 'config' ? join(root, 'config.json') : registrationPath(root, 'builder', 'repo')); - assert.throws(() => kind === 'config' ? loadDataRoot(file) : readRegistration(root, 'builder', 'repo'), safe(file, SeatError, { exitCode: 2 })); + assert.throws(() => kind === 'config' ? loadDataRoot(file) : readRegistration(root, 'builder', 'repo'), safe(kind === 'config' ? 'config is not valid JSON' : 'registration repo/builder is not valid JSON', SeatError, { exitCode: 2 })); }); for (const kind of ['config', 'registration']) test(`${kind} read refusal retains the filesystem code`, t => { const root = scratch(t), file = kind === 'config' ? join(root, 'config.json') : registrationPath(root, 'builder', 'repo'); mkdirSync(file, { recursive: true }); assert.throws(() => kind === 'config' ? loadDataRoot(file) : readRegistration(root, 'builder', 'repo'), error => { - safe(file, SeatError, { exitCode: 2 })(error); + safe(kind === 'config' ? 'config is not valid JSON' : 'registration repo/builder is not valid JSON', SeatError, { exitCode: 2 })(error); assert.match(error.message, /\(EISDIR\)$/); return true; }); diff --git a/scripts/mosaic-config.mjs b/scripts/mosaic-config.mjs index a676242a5..2567b13ea 100755 --- a/scripts/mosaic-config.mjs +++ b/scripts/mosaic-config.mjs @@ -377,7 +377,9 @@ async function load(file, instance = null) { const raw = readRaw(file); if (raw === null) { const bootstrap = isNamed(instance) ? `scripts/bootstrap.sh --instance ${instance}` : "scripts/bootstrap.sh"; - fail(3, `configuration not found: ${file} (run ${bootstrap} to create it)`); + // #1873: named by where it came from, never by host path. + const which = isNamed(instance) ? `the config of instance ${instance}` : process.env.MOSAIC_CONFIG ? "the system config MOSAIC_CONFIG names" : "the default system config"; + fail(3, `configuration not found: ${which} (run ${bootstrap} to create it)`); } let document; try { -- 2.54.0 From 87db40e0cda8b37e115d8313a328ace97d62392c Mon Sep 17 00:00:00 2001 From: Jason Woltje Date: Wed, 11 Nov 2026 02:20:11 -0600 Subject: [PATCH 2/8] fix(cli,seat,discord,organization,control-board,harness,plugins,webui): review round: launcher reasons, session, discord reads, mosaic-config and keys by label (#1873) Review "changes 8" fix round for org-redact6. - launcher: a harness module's throw names its system code or class, never its message; preflight, loader, bundle, checkout, sandbox and route text is relayed with host paths replaced (relay()). - claude-code and pi preflights name MOSAIC_CLAUDE_BIN and the repository's node_modules, never a path; persona and skill bundle errors and sandboxProblem name their file or the sandbox by label. - seat session: thread store, legacy store, overlap, busy lock and newRun refusals name the place under the data root. - discord: unknown keys, binding names and turn ids are described when not plain; approvals, journal and repository read errors name the file and code; git drops stderr lines that carry a host path and a spawn failure names its code only. - seat, control-board: registration fields and seen keys are described when not plain; invalid seat name and layout state the constraint. - organization: the registry names the config directory and registry file by label; --project is quoted only when it is a plain name; systemConfigProblem moves to its own module. - scripts/mosaic-config.mjs: the config by where it came from, the data root and its marker by role, keys and arguments only when plain, and error codes in place of raw messages. Exit codes unchanged. - webui sso-config: an unknown console key is named only when plain. Sentinel tests in each package's path-redaction-labels file fail on BASE; pins that quoted a path now assert the label and its absence. --- packages/cli/src/config.mjs | 2 +- packages/cli/src/launcher.mjs | 34 ++++--- packages/cli/tests/json-redaction.test.mjs | 6 +- packages/cli/tests/launch-routes.test.mjs | 2 +- packages/cli/tests/launcher.test.mjs | 11 ++- packages/cli/tests/org-recovery.test.mjs | 3 +- .../cli/tests/path-redaction-labels.test.mjs | 92 ++++++++++++++++++- .../cli/tests/proxy-launch-order.test.mjs | 3 +- packages/cli/tests/rename-b-commands.test.mjs | 2 +- packages/cli/tests/stack-instances.test.mjs | 16 +++- packages/control-board/src/scan.mjs | 4 +- .../tests/path-redaction-labels.test.mjs | 25 ++++- packages/discord/src/approvals.mjs | 10 +- packages/discord/src/binding.mjs | 18 +++- packages/discord/src/errors.mjs | 12 +++ packages/discord/src/git.mjs | 13 ++- packages/discord/src/journal.mjs | 11 ++- packages/discord/src/setspark.mjs | 3 +- packages/discord/tests/binding.test.mjs | 3 +- packages/discord/tests/notify.test.mjs | 2 +- .../tests/path-redaction-labels.test.mjs | 54 ++++++++++- packages/harness/src/bundle.mjs | 24 ++--- packages/organization/src/config-command.mjs | 22 +---- packages/organization/src/org-registry.mjs | 70 +++++++------- .../src/system-config-problem.mjs | 21 +++++ packages/organization/tests/cli.test.mjs | 15 ++- .../tests/instance-settings.test.mjs | 2 +- .../tests/path-redaction-labels.test.mjs | 25 ++++- .../plugins/harness/claude-code/index.mjs | 3 +- packages/plugins/harness/pi/index.mjs | 2 +- packages/seat/src/sandbox.mjs | 5 +- packages/seat/src/seat.mjs | 10 +- packages/seat/src/session.mjs | 43 +++++---- .../seat/tests/path-redaction-labels.test.mjs | 52 ++++++++++- packages/seat/tests/sandbox.test.mjs | 5 +- packages/webui/src/sso-config.mjs | 3 +- scripts/mosaic-config.mjs | 64 +++++++------ 37 files changed, 513 insertions(+), 179 deletions(-) create mode 100644 packages/organization/src/system-config-problem.mjs diff --git a/packages/cli/src/config.mjs b/packages/cli/src/config.mjs index 16d211900..e34d81a8c 100644 --- a/packages/cli/src/config.mjs +++ b/packages/cli/src/config.mjs @@ -13,7 +13,7 @@ import { BusinessError, accessFilePath, configDir, loadAccess, loadBusiness, loa import { loadRootAgents } from "../../organization/src/agent-levels.mjs"; import { materializeCopies, withCopies } from "../../organization/src/agent-copies.mjs"; import { organizationWriter } from "../../organization/src/organization.mjs"; -import { systemConfigProblem } from "../../organization/src/config-command.mjs"; +import { systemConfigProblem } from "../../organization/src/system-config-problem.mjs"; import { businessFilePath } from "../../organization/src/organization-config.mjs"; import { busOrganization } from "../../bus/src/organization-context.mjs"; import { BusError } from "../../bus/src/broker.mjs"; diff --git a/packages/cli/src/launcher.mjs b/packages/cli/src/launcher.mjs index 24f43e0b5..8ba047a9f 100644 --- a/packages/cli/src/launcher.mjs +++ b/packages/cli/src/launcher.mjs @@ -203,16 +203,23 @@ class Refusal extends Error { } // #1873: a refusal's reason never quotes a host path. A Node system error's message names one, -// so it reads as the call and its code (org-registry quietMessage); any other message stays. -const reason = (e) => quietMessage(e, "a host file") ?? String(e?.message ?? e); +// so it reads as the call and its code (org-registry quietMessage). Any other message is the +// host's own text or text relayed from a module (a preflight, the loader, a bundle or checkout +// error); relay() replaces every absolute or home path and file: URL in it with "a host path". +const HOST_PATH = /(^|[\s'"(=:,[<])(?:~?\/(?!\/)|file:)[^\s'"),\]>;]*/gi; +export const relay = (text) => String(text).replace(HOST_PATH, (_, before) => `${before}a host path`); +const reason = (e) => quietMessage(e, "a host file") ?? relay(e?.message ?? e); +const relayed = (failed) => failed && (typeof failed.message === "string" ? { ...failed, message: relay(failed.message) } : failed); // A harness module runs with the host's authority, and anything it throws refuses the launch -// and names the module rather than escaping the host. +// and names the module rather than escaping the host. What it threw is named by its system code +// or its class, never its message, which the host didn't write (#1873). function call(name, what, fn) { try { return fn(); } catch (e) { - throw new Refusal("harness-failed", `harness ${name} ${what}: ${reason(e)}`); + const thrown = quietMessage(e, "a host file") ?? `threw ${/^[A-Za-z][A-Za-z0-9]{0,39}$/.test(String(e?.name)) ? e.name : "a value"} without a system code`; + throw new Refusal("harness-failed", `harness ${name} ${what}: ${thrown}`); } } @@ -392,7 +399,7 @@ export function createLauncher({ // org-defscan (#1873): the entry the contract came from, taken before any await lets another check replace b. definitionEntry = String(resolved.definition).startsWith("org:") ? b.definitions?.[resolved.definition] : undefined; } catch (e) { - throw new Refusal(e.code === "plugin-disabled" ? "plugin-disabled" : "unsupported-harness", e.code === "plugin-disabled" ? e.plugin : e.message); + throw new Refusal(e.code === "plugin-disabled" ? "plugin-disabled" : "unsupported-harness", e.code === "plugin-disabled" ? e.plugin : relay(e.message)); } // Resolve the business's current provider modules with a fresh loader; a refused // load never inherits a previous business's authority. @@ -415,7 +422,7 @@ export function createLauncher({ return sessionModel(resolved, b.launch.models, which, { proxy: Boolean(system.proxy || proxyContext), resolver }); } catch (e) { const code = e instanceof BundleError && (e.code === "no-route" || e.code.startsWith("proxy-") || e.code.startsWith("model-")) ? e.code : "unsupported-harness"; - throw new Refusal(code, e.message); + throw new Refusal(code, relay(e.message)); } }; const primary = routeOf("primary"); @@ -431,7 +438,7 @@ export function createLauncher({ try { readSkills(resolved.skills, skillsDir); } catch (e) { - if (e instanceof BundleError) throw new Refusal(e.code === "skill-not-enabled" ? "skill-not-enabled" : "skill-refused", e.message); + if (e instanceof BundleError) throw new Refusal(e.code === "skill-not-enabled" ? "skill-not-enabled" : "skill-refused", relay(e.message)); throw e; } // Both of a launch's harnesses must be switched on, whichever it takes. @@ -445,7 +452,7 @@ export function createLauncher({ const { refused } = await loader.load({ harness: [m.harness] }); // #1873: a loader message can quote the module's path or its load error; only a // missing-export one, which names the export, is relayed. Any other refusal names its code. - if (refused.length) throw new Refusal("harness-refused", `harness ${m.harness}: ${refused.map((r) => r.code === "missing-export" ? `${r.code}: ${r.message}` : String(r.code)).join("; ")}`); + if (refused.length) throw new Refusal("harness-refused", `harness ${m.harness}: ${refused.map((r) => r.code === "missing-export" ? `${r.code}: ${relay(r.message)}` : String(r.code)).join("; ")}`); const module = loader.modules("harness")[0]; const levels = call(m.harness, "levels", () => module.levels()); if (!levels.includes(m.thinking)) throw new Refusal("thinking-unsupported", `harness ${m.harness} takes thinking ${levels.join(", ")}, not ${m.thinking}`); @@ -492,7 +499,7 @@ export function createLauncher({ } } const unavailable = sandboxProblem({ bwrap }); - if (unavailable) throw new Refusal("sandbox-unavailable", unavailable); + if (unavailable) throw new Refusal("sandbox-unavailable", relay(unavailable)); const hit=limitHit(registry.sessions,b.id,b.launch.limits,model); if(hit){const error=new Refusal("capacity-full",JSON.stringify(hit));error.limit=hit;throw error;} // Row 125 (#1619, REQ-LIMIT-4): the limit checks. An answer that isn't an allow refuses, @@ -540,7 +547,8 @@ export function createLauncher({ // and the helper returns the last one they saw. const pickRoute = async (proxyOf) => { let sessionProxy = await proxyOf(primary); - const failed = call(primary.harness, "preflight", () => harness.preflight({ env: sessionProxy ? sessionEnv(env, "preflight", sessionProxy) : env, proxy: sessionProxy })); + // #1873: a preflight's message is the module's own text, relayed with host paths replaced. + const failed = relayed(call(primary.harness, "preflight", () => harness.preflight({ env: sessionProxy ? sessionEnv(env, "preflight", sessionProxy) : env, proxy: sessionProxy }))); if (failed) { if (!fallback) throw new Refusal(failed.code, failed.message); // The fallback only on a failed primary preflight, and only at launch (row 65). Any refusal of @@ -550,7 +558,7 @@ export function createLauncher({ throw e; }); sessionProxy = await proxyOf(fallback); - const also = call(fallback.harness, "preflight", () => second.preflight({ env: sessionProxy ? sessionEnv(env, "preflight", sessionProxy) : env, proxy: sessionProxy })); + const also = relayed(call(fallback.harness, "preflight", () => second.preflight({ env: sessionProxy ? sessionEnv(env, "preflight", sessionProxy) : env, proxy: sessionProxy }))); if (also) throw new Refusal(also.code, `fallback ${fallback.harness}: ${also.message} (primary ${primary.harness}: ${failed.message})`); [model, harness, route, routeReason] = [fallback, second, "fallback", failed.code]; } @@ -672,7 +680,7 @@ export function createLauncher({ return ensureCheckouts({ workspace, projects: codeProjects, env }); }); } catch (e) { - if (e instanceof CheckoutError) throw new Refusal("checkout-failed", `${e.project}: ${e.message}`); + if (e instanceof CheckoutError) throw new Refusal("checkout-failed", `${e.project}: ${relay(e.message)}`); throw e; } for (const c of checkouts) { @@ -1376,7 +1384,7 @@ export function createLauncher({ } catch (e) { log(`request ${r.request} (${r.instance}): workspace rename failed: ${e.message}; left at ${from}`); // #1873: the reason names the workspace by label and the error by code, never a path. - return { transaction, ...bot, refusal: "workspace-rename-failed", why: `${quietMessage(e, label) ?? `${label}: ${e.message}`}; left in place` }; + return { transaction, ...bot, refusal: "workspace-rename-failed", why: `${quietMessage(e, label) ?? `${label}: ${relay(e.message)}`}; left in place` }; } record({ event: "workspace-retired", instance: r.instance, request: r.request, transaction, path: to }); } diff --git a/packages/cli/tests/json-redaction.test.mjs b/packages/cli/tests/json-redaction.test.mjs index 7fbc4a09b..8c57cc046 100644 --- a/packages/cli/tests/json-redaction.test.mjs +++ b/packages/cli/tests/json-redaction.test.mjs @@ -22,7 +22,7 @@ function fixture(t) { return { root, file, env, run, boot }; } // label: the CLI refusal names the file by this label and never by host path (PR #1916). -// Without it, the refusal comes from scripts/mosaic-config.mjs, which still names the file. +// scripts/mosaic-config.mjs names it by label too (#1873 redact6). function redacted(result, file, status, label) { assert.equal(result.status, status, result.stderr); assert.equal(result.stdout, ''); @@ -37,7 +37,7 @@ function redacted(result, file, status, label) { test('system config parse refusal and CLI relay omit parser bytes', t => { const f = fixture(t); mkdirSync(dirname(f.file), { recursive: true }); writeFileSync(f.file, secret); - redacted(f.run(configScript, 'validate', '--instance', 'redacttest'), f.file, 2); + redacted(f.run(configScript, 'validate', '--instance', 'redacttest'), f.file, 2, 'configuration is not valid JSON (the config of instance redacttest)'); assert.throws(() => loadSystem({ env: f.env, instance: 'redacttest' }), error => { assert.equal(error.exitCode, 3); // #1873: the cli relay names the instance's config by label, never its path. @@ -54,7 +54,7 @@ test('named instance marker parse refusal omits parser bytes', t => { const f = fixture(t); f.boot(); const { dataRoot } = JSON.parse(readFileSync(f.file, 'utf8')); const marker = join(dataRoot, 'instance.json'); writeFileSync(marker, secret); - redacted(f.run(configScript, 'validate', '--instance', 'redacttest'), marker, 3); + redacted(f.run(configScript, 'validate', '--instance', 'redacttest'), marker, 3, "instance-root-unclaimed: the data root's instance marker is not a valid marker (not valid JSON)"); }); for (const operation of ['validate', 'create', 'update']) test(`org ${operation} document refusal omits parser bytes`, t => { diff --git a/packages/cli/tests/launch-routes.test.mjs b/packages/cli/tests/launch-routes.test.mjs index a945cd9a2..de2cdb2fc 100644 --- a/packages/cli/tests/launch-routes.test.mjs +++ b/packages/cli/tests/launch-routes.test.mjs @@ -120,7 +120,7 @@ test('row 65 the loader refusing a module refuses harness-refused and names it; } const s=await setup(t,route('throws','claude-sonnet-5-5','high')); assert.deepEqual(await s.launch(),{ok:false,error:'harness-failed'}); - assert.equal(s.last().why,'harness throws preflight: fixture preflight threw'); + assert.equal(s.last().why,'harness throws preflight: threw Error without a system code'); }); test('row 65 a thinking level the route harness does not apply refuses thinking-unsupported, on the primary or a fallback taken',async t=>{ diff --git a/packages/cli/tests/launcher.test.mjs b/packages/cli/tests/launcher.test.mjs index 5ec0b06e9..2dc8277fd 100644 --- a/packages/cli/tests/launcher.test.mjs +++ b/packages/cli/tests/launcher.test.mjs @@ -768,9 +768,10 @@ test("a missing or failing bwrap refuses sandbox-unavailable before anything sta const bad = join(given.root, "bwrap"); writeFileSync(bad, "#!/bin/sh\necho 'bwrap: setting up uid map: Permission denied' >&2\nexit 1\n", { mode: 0o755 }); const { f, launcher, cto } = await setup(t, undefined, given, { bwrap: bad }); - await assert.rejects(launcher.launchPm(), (e) => e.message === `PM launch refused: sandbox-unavailable (${bad} failed: bwrap: setting up uid map: Permission denied)`); + // #1873: the sandbox by name, never the binary's host path. + await assert.rejects(launcher.launchPm(), (e) => e.message === "PM launch refused: sandbox-unavailable (the sandbox (bwrap) failed: bwrap: setting up uid map: Permission denied)"); rmSync(bad); - await assert.rejects(launcher.launchPm(), (e) => e.message === `PM launch refused: sandbox-unavailable (${bad} is missing)`); + await assert.rejects(launcher.launchPm(), (e) => e.message === "PM launch refused: sandbox-unavailable (the sandbox (bwrap) is missing)"); // A session's launch request gets the same code, through refuse(). assert.deepEqual(await launcher.fromSession(cto.cap, "coder"), { ok: false, error: "sandbox-unavailable" }); assert.deepEqual(log(f).map((e) => e.code), ["sandbox-unavailable", "sandbox-unavailable", "sandbox-unavailable"]); @@ -791,7 +792,7 @@ test("row 64 a role skill that isn't enabled refuses skill-not-enabled, and one assert.deepEqual(await launcher.fromSession(cto.cap, "coder"), { ok: false, error: "skill-not-enabled" }); // A symlink in skills-enabled is refused, not followed. symlinkSync(join(REPO, "skills", "ms-unslop"), join(enabled, "ms-unslop")); - await assert.rejects(launcher.launchPm(), (e) => e.exitCode === 3 && e.message === `PM launch refused: skill-refused (skill ms-unslop isn't a plain directory under ${enabled})`); + await assert.rejects(launcher.launchPm(), (e) => e.exitCode === 3 && e.message === "PM launch refused: skill-refused (skill ms-unslop isn't a plain directory under the enabled skills directory)"); assert.deepEqual(await launcher.fromSession(cto.cap, "coder"), { ok: false, error: "skill-refused" }); assert.deepEqual(log(f).map((e) => [e.event, e.code]), [["refused", "skill-not-enabled"], ["refused", "skill-not-enabled"], ["refused", "skill-refused"], ["refused", "skill-refused"]]); assert.deepEqual(readSessions(f.dataRoot), []); @@ -832,7 +833,9 @@ test("row 213 a creative instance's persona is read from /personas: o const e = entries[1]; assert.deepEqual([e.event, e.code], ["refused", "launch-failed"]); assert.equal(e.thread, thread); - assert.equal(e.why, `instance bean: persona file ${join(dir, "bean.md")} isn't the file persona.sha256 names`); + // #1873: the file by its place under the persona directory, never its host path. + assert.equal(e.why, "instance bean: persona file acme/bean.md in the persona directory isn't the file persona.sha256 names"); + assert.ok(!e.why.includes(dir)); assert.ok(!JSON.stringify(log(f)).includes("PERSONA-MARKER-d2")); assert.deepEqual(readSessions(f.dataRoot), []); // The run directory exists by then (BUILD.md residual); the persona is read before any bundle file. diff --git a/packages/cli/tests/org-recovery.test.mjs b/packages/cli/tests/org-recovery.test.mjs index e2309f03e..bdab8aa28 100644 --- a/packages/cli/tests/org-recovery.test.mjs +++ b/packages/cli/tests/org-recovery.test.mjs @@ -84,7 +84,8 @@ test('lock recovery reports fences and recovers a dead clearer while refusing li assert.equal(shown.fence.pid, process.pid); await assert.rejects(f.call('org', 'lock', 'clear', 'alpha'), new RegExp(`recovery is already in progress.*${process.pid}`)); writeFileSync(fence, ''); - await assert.rejects(f.call('org', 'lock', 'clear', 'alpha'), e => e.message.includes(fence) && /remove.*hand/i.test(e.message)); + // #1873: the fence is named by label in the config directory, never by its path. + await assert.rejects(f.call('org', 'lock', 'clear', 'alpha'), e => !e.message.includes(f.ctx.configDir) && e.message.includes('recovery fence org-lifecycle-alpha.lock.clear in the config directory') && /remove.*hand/i.test(e.message)); unlinkSync(fence); const registry = new URL('../../organization/src/org-registry.mjs', import.meta.url).href; const code = `import fs from 'node:fs'; import { syncBuiltinESMExports } from 'node:module'; diff --git a/packages/cli/tests/path-redaction-labels.test.mjs b/packages/cli/tests/path-redaction-labels.test.mjs index f30ecb3e7..16a528381 100644 --- a/packages/cli/tests/path-redaction-labels.test.mjs +++ b/packages/cli/tests/path-redaction-labels.test.mjs @@ -5,9 +5,10 @@ // under a sentinel path would pass the Unix socket length limit, so they use a stub host. import test from 'node:test'; import assert from 'node:assert/strict'; -import { mkdirSync, readFileSync, writeFileSync } from 'node:fs'; +import { mkdirSync, readFileSync, symlinkSync, writeFileSync } from 'node:fs'; +import { spawnSync } from 'node:child_process'; import { join } from 'node:path'; -import { createLauncher } from '../src/launcher.mjs'; +import { createLauncher, relay } from '../src/launcher.mjs'; import { CliError } from '../src/errors.mjs'; import { loadSystem } from '../src/config.mjs'; import { main } from '../src/cli.mjs'; @@ -89,13 +90,55 @@ async function routed(t, harness) { const makeLoader = await fakeHarnesses(t, { pi: null, 'claude-code': null }, write => { // fsthrows: its preflight reads a missing file, so the error's message quotes the path. write('harness', 'fsthrows', `export * from ${PI};\nimport { readFileSync } from "node:fs";\nexport function preflight() { readFileSync(${JSON.stringify(absent)}); }\n`); + // plainthrows: a plain Error whose message quotes a host path, as a module's own text. + write('harness', 'plainthrows', `export * from ${PI};\nexport function preflight() { throw new Error(${JSON.stringify(`cannot read ${absent}`)}); }\n`); + // pathfails: a preflight that returns a failure whose message quotes a host path. + write('harness', 'pathfails', `export * from ${PI};\nexport function preflight() { return { code: "harness-binary-missing", message: ${JSON.stringify(`no binary at ${absent}; see file://${absent}`)} }; }\n`); }); const system = JSON.parse(readFileSync(f.config, 'utf8')); const launcher = createLauncher({ host, system, env: f.env, makeLoader, bwrap: '/usr/bin/true', log: () => {} }); const last = () => readFileSync(launchLogFile(f.dataRoot, 'acme'), 'utf8').trim().split('\n').map(l => JSON.parse(l)).at(-1); - return { root, launch: () => launcher.fromSession('fixture-cap', 'coder'), last }; + return { root, f, launch: () => launcher.fromSession('fixture-cap', 'coder'), last }; } +test('a harness whose preflight throws a plain Error naming a path refuses harness-failed by its class, never its message', async t => { + const s = await routed(t, 'plainthrows'); + assert.deepEqual(await s.launch(), { ok: false, error: 'harness-failed' }); + assert.equal(s.last().why, 'harness plainthrows preflight: threw Error without a system code'); + clean(JSON.stringify(s.last()), s.root); +}); + +test('a preflight failure whose message quotes a path keeps its code and relays the text with the path replaced', async t => { + const s = await routed(t, 'pathfails'); + assert.deepEqual(await s.launch(), { ok: false, error: 'harness-binary-missing' }); + assert.equal(s.last().why, 'no binary at a host path; see a host path'); + clean(JSON.stringify(s.last()), s.root); +}); + +test('the claude-code preflight names MOSAIC_CLAUDE_BIN, never its value', async t => { + const { root, dir } = scratch(t); + const f = fixture(dir, 'acme', doc => { + doc.roles.coder.vars = { harness: 'claude-code', model: 'claude-sonnet-5-5', thinking: 'high' }; + return doc; + }); + initializeControl(f.dataRoot); + withoutHostCredentials(f.env); + f.env.CLAUDE_CODE_OAUTH_TOKEN = 'fixture-only'; + f.env.MOSAIC_CLAUDE_BIN = join(dir, 'absent-claude'); + const host = { business: 'acme', beforeClose: () => {}, op: async x => x.op === 'identity' ? { business: 'acme', role: 'pm' } : x.op === 'mayLaunch' ? { allow: true } : {} }; + const makeLoader = await fakeHarnesses(t, { pi: null, 'claude-code': null }); + const launcher = createLauncher({ host, system: JSON.parse(readFileSync(f.config, 'utf8')), env: f.env, makeLoader, bwrap: '/usr/bin/true', log: () => {} }); + assert.deepEqual(await launcher.fromSession('fixture-cap', 'coder'), { ok: false, error: 'harness-binary-missing' }); + const last = JSON.parse(readFileSync(launchLogFile(f.dataRoot, 'acme'), 'utf8').trim().split('\n').at(-1)); + assert.equal(last.why, "MOSAIC_CLAUDE_BIN isn't an executable file at an absolute path"); + clean(JSON.stringify(last), root); +}); + +test('relay replaces absolute, home and file: paths and keeps the rest of the text', () => { + assert.equal(relay(`bad ${'/srv/x/' + secret} and ~/y, (file:///z) here`), 'bad a host path and a host path, (a host path) here'); + assert.equal(relay('fatal: not a git repository: .git; see https://example.test/a'), 'fatal: not a git repository: .git; see https://example.test/a'); +}); + test('a harness whose preflight throws a system error refuses harness-failed naming the call and code, never the path', async t => { const s = await routed(t, 'fsthrows'); assert.deepEqual(await s.launch(), { ok: false, error: 'harness-failed' }); @@ -154,3 +197,46 @@ test('a system config problem names the config by label and never relays mosaic- return true; }); }); + +// scripts/mosaic-config.mjs run standalone: every refusal names the config by where it came from, +// the data root and its marker by their roles, and a key only when it is a plain name. Exit codes +// are unchanged: 2 invalid, 3 missing or unclaimed, 4 usage. +const CONFIG_SCRIPT = new URL('../../../scripts/mosaic-config.mjs', import.meta.url).pathname; +function mosaicConfig(home, env, ...args) { + return spawnSync(process.execPath, [CONFIG_SCRIPT, ...args], { encoding: 'utf8', env: { PATH: process.env.PATH, HOME: home, ...env } }); +} +function refusedBy(r, status, text, root) { + assert.equal(r.status, status, r.stderr); + assert.equal(r.stdout, ''); + assert.ok(r.stderr.includes(text), r.stderr); + clean(r.stderr, root); +} + +test('mosaic-config refusals name the config, data root and keys by label, never a host path or value', t => { + const { root, dir } = scratch(t); + const file = join(dir, 'config.json'); + const env = { MOSAIC_CONFIG: file }; + writeFileSync(file, 'not json'); + refusedBy(mosaicConfig(dir, env, 'validate'), 2, 'mosaic-config: configuration is not valid JSON (the system config MOSAIC_CONFIG names)', root); + const good = { configVersion: 1, environment: 'development', dataRoot: join(dir, 'data'), execution: { backend: 'docker', provider: 'zai', model: 'glm-5.3-flash' } }; + writeFileSync(file, JSON.stringify({ ...good, [join(dir, 'k')]: 1 })); + refusedBy(mosaicConfig(dir, env, 'validate'), 2, 'unsupported configuration key: that is not a plain name', root); + writeFileSync(file, JSON.stringify({ ...good, console: { [join(dir, 'k')]: 1 } })); + refusedBy(mosaicConfig(dir, env, 'validate'), 2, 'console has a key that is not a plain name', root); + writeFileSync(file, JSON.stringify({ ...good, dataRoot: dir })); + refusedBy(mosaicConfig(dir, env, 'validate'), 2, 'execution "dataRoot" must not be or contain the home directory', root); + writeFileSync(join(dir, 'real.json'), JSON.stringify(good)); + const link = join(dir, 'link.json'); + symlinkSync(join(dir, 'real.json'), link); + refusedBy(mosaicConfig(dir, { MOSAIC_CONFIG: link }, 'validate'), 2, 'configuration path must be a regular, non-symbolic-link file: the system config MOSAIC_CONFIG names', root); + // A named instance: MOSAIC_CONFIG naming another file is usage (4); an unmarked root is 3. + refusedBy(mosaicConfig(dir, env, 'validate', '--instance', 'alpha'), 4, 'instance-config-mismatch: --instance alpha names its own config, but MOSAIC_CONFIG names another file', root); + refusedBy(mosaicConfig(dir, {}, 'validate', '--instance', join(dir, 'x')), 4, 'instance-name-invalid: that name must match', root); + const named = join(dir, '.config', 'mosaic-alpha'); + mkdirSync(named, { recursive: true, mode: 0o700 }); + mkdirSync(join(dir, '.mosaic-alpha'), { mode: 0o700 }); + writeFileSync(join(named, 'config.json'), JSON.stringify({ ...good, dataRoot: join(dir, '.mosaic-alpha') })); + refusedBy(mosaicConfig(dir, {}, 'validate', '--instance', 'alpha'), 3, 'instance-root-unclaimed: the data root holds no instance.json naming alpha', root); + mkdirSync(join(dir, '.mosaic-alpha', 'instance.json')); + refusedBy(mosaicConfig(dir, {}, 'validate', '--instance', 'alpha'), 3, "instance-root-unclaimed: the data root's instance marker must be a regular, non-symbolic-link file", root); +}); diff --git a/packages/cli/tests/proxy-launch-order.test.mjs b/packages/cli/tests/proxy-launch-order.test.mjs index 3461fd3e3..4e00713f9 100644 --- a/packages/cli/tests/proxy-launch-order.test.mjs +++ b/packages/cli/tests/proxy-launch-order.test.mjs @@ -130,7 +130,8 @@ test('#774 F1 proxy launch of an agent with no key yet passes the first check un // claude-code module on claude-opus-5-5 (provider anthropic); MOSAIC_CLAUDE_BIN decides whether its // preflight passes. The stated fallback is pi on glm-5.3-flash (provider zai). const NO_CLAUDE = '/nonexistent-774/claude'; -const NO_CLAUDE_WHY = `MOSAIC_CLAUDE_BIN ${NO_CLAUDE} isn't an executable file at an absolute path`; +// #1873: names the variable, never its value (was `MOSAIC_CLAUDE_BIN ${NO_CLAUDE} isn't ...`). +const NO_CLAUDE_WHY = "MOSAIC_CLAUDE_BIN isn't an executable file at an absolute path"; const routed = (t, bin) => managedLaunchFixture(t, { harness: 'claude-code', extra: ['pi'], real: ['claude-code'], hostEnv: { MOSAIC_CLAUDE_BIN: bin } }); const withFallback = (doc, harness = 'pi', model = 'glm-5.3-flash') => { claudeCoder(doc); diff --git a/packages/cli/tests/rename-b-commands.test.mjs b/packages/cli/tests/rename-b-commands.test.mjs index a72605092..a41e2a754 100644 --- a/packages/cli/tests/rename-b-commands.test.mjs +++ b/packages/cli/tests/rename-b-commands.test.mjs @@ -112,7 +112,7 @@ test("rename r2 B2: a credential problem, an expiry warning and a system config for (const args of [["validate", "acme"], ["resolve", "acme", "coder"]]) { const { old, now } = await sameAcrossSpellings(args, bad.env, bad.root, 2); assert.equal(now.stderr, old.stderr, "a refusal is byte for byte the same, unnormalized"); - assert.match(old.stderr, /^mosaic business: .*coder-gitea\.token.*mode 644/m); + assert.match(old.stderr, /^mosaic business: (?:roles\.coder\.credentials\.gitea|coder gitea) file: mode 644/m); assert.match(old.stderr, /credential reference problem\(s\)\n$/); } const soon = new Date(Date.now() + 3 * 86400000).toISOString().slice(0, 10); diff --git a/packages/cli/tests/stack-instances.test.mjs b/packages/cli/tests/stack-instances.test.mjs index daa3f8972..8476b2c84 100644 --- a/packages/cli/tests/stack-instances.test.mjs +++ b/packages/cli/tests/stack-instances.test.mjs @@ -133,7 +133,9 @@ test("T-I1 bootstrap --instance creates each instance's config, root and marker for (const name of ["gamma", "worktrees", "delta"]) { const r = bootstrap(home, "--instance", name); assert.equal(r.code, 3, r.err); - assert.match(r.err, new RegExp(`instance-root-unclaimed: ${join(home, `.mosaic-${name}`)} exists, is not empty and holds no instance.json naming ${name}; nothing was written`)); + // #1873: the data root by its role, never its path. + assert.match(r.err, new RegExp(`instance-root-unclaimed: the instance's data root exists, is not empty and holds no instance.json naming ${name}; nothing was written`)); + assert.ok(!r.err.includes(home), r.err); assert.equal(existsSync(join(home, ".config", `mosaic-${name}`)), false); } assert.deepEqual(snapshot(home), before); @@ -154,7 +156,8 @@ test("T-I1 bootstrap --instance creates each instance's config, root and marker const before = snapshot(home); const r = bootstrap(home, "--instance", "alpha"); assert.equal(r.code, 3); - assert.match(r.err, /instance-root-unclaimed: .*\.mosaic-alpha holds no instance\.json naming alpha/); + assert.match(r.err, /instance-root-unclaimed: the data root holds no instance\.json naming alpha/); + assert.ok(!r.err.includes(home), r.err); assert.deepEqual(snapshot(home), before); }); @@ -262,7 +265,8 @@ test("T-I2 selection: the four rows of D1 through the instance op and through an const r = config(home, { MOSAIC_CONFIG: given }, op, "--instance", "alpha"); assert.equal(r.code, 4, `${op} ${given}: ${r.err}`); assert.equal(r.out, ""); - assert.match(r.err, new RegExp(`instance-config-mismatch: --instance alpha names ${cfgOf(home, "alpha")}, but MOSAIC_CONFIG names ${given}`)); + assert.match(r.err, /instance-config-mismatch: --instance alpha names its own config, but MOSAIC_CONFIG names another file/); + assert.ok(!r.err.includes(home) && !r.err.includes(given), r.err); } } }); @@ -328,7 +332,8 @@ test("T-I7 claims: the instance op refuses a root another instance claims, and a const r = config(home, {}, op, "--instance", "alpha"); assert.equal(r.code, 3, `${op}: ${r.err}`); assert.equal(r.out, ""); - assert.match(r.err, new RegExp(`instance-root-mismatch: ${join(home, ".mosaic-beta")} is claimed by instance beta, not alpha`)); + assert.match(r.err, /instance-root-mismatch: the data root is claimed by instance beta, not alpha/); + assert.ok(!r.err.includes(home), r.err); } edit("alpha", join(home, ".mosaic-alpha")); assert.equal(config(home, {}, "instance", "--instance", "alpha").code, 0); @@ -338,7 +343,8 @@ test("T-I7 claims: the instance op refuses a root another instance claims, and a for (const op of ["instance"]) { const r = config(home, { MOSAIC_CONFIG: cfgOf(home, "alpha") }, op); assert.equal(r.code, 3, r.err); - assert.match(r.err, new RegExp(`instance-root-mismatch: ${join(home, ".mosaic-alpha")} is claimed by instance alpha; select it with --instance alpha`)); + assert.match(r.err, /instance-root-mismatch: the data root is claimed by instance alpha; select it with --instance alpha/); + assert.ok(!r.err.includes(home), r.err); } }); diff --git a/packages/control-board/src/scan.mjs b/packages/control-board/src/scan.mjs index d32bea84b..a8307880e 100644 --- a/packages/control-board/src/scan.mjs +++ b/packages/control-board/src/scan.mjs @@ -18,7 +18,7 @@ import { existsSync, readFileSync, readdirSync, statSync, mkdirSync, writeFileSy import { join, basename, isAbsolute, resolve } from "node:path"; import { homedir } from "node:os"; import { spawnSync } from "node:child_process"; -import { readRegistration, samePath, SeatError, LAYOUTS } from "../../seat/src/seat.mjs"; +import { readRegistration, samePath, SeatError, LAYOUTS, plainKey } from "../../seat/src/seat.mjs"; import { discoverDiscordAgents, inspectDiscord, newestDiscordSession } from './discord.mjs'; @@ -235,7 +235,7 @@ export function loadSeen(boardDir) { } if (!parsed || typeof parsed !== "object" || Array.isArray(parsed)) throw new ConfigError("seen marks file seen.json must be a JSON object"); for (const [k, v] of Object.entries(parsed)) { - if (typeof v !== "string") throw new ConfigError(`seen marks file seen.json has a non-string value for ${JSON.stringify(k)}`); + if (typeof v !== "string") throw new ConfigError(`seen marks file seen.json has a non-string value for ${plainKey(k)}`); } return parsed; } diff --git a/packages/control-board/tests/path-redaction-labels.test.mjs b/packages/control-board/tests/path-redaction-labels.test.mjs index 0953a6a96..194492605 100644 --- a/packages/control-board/tests/path-redaction-labels.test.mjs +++ b/packages/control-board/tests/path-redaction-labels.test.mjs @@ -7,7 +7,7 @@ import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { dirname, join } from 'node:path'; import { ConfigError, loadConfig, loadRegistrations, loadSeen } from '../src/scan.mjs'; -import { registrationPath } from '../../seat/src/seat.mjs'; +import { readRegistration, registrationPath, SeatError } from '../../seat/src/seat.mjs'; const secret = 'sk-test-NOT-A-SECRET-1234'; function scratch(t) { @@ -59,3 +59,26 @@ test('a bad registration is reported by layout and seat, never by its path', t = assert.match(errors[0], /^seat repo\/builder: .*: registration repo\/builder$/); clean(errors[0], root); }); + +// Review round (#1873): a key or name that could be a host path is described, never echoed. +test('a seen marks key or a registration field that is not a plain name is described, never echoed', t => { + const { root, dir } = scratch(t); + writeFileSync(join(dir, 'seen.json'), JSON.stringify({ [join(dir, 'host')]: 1 })); + assert.throws(() => loadSeen(dir), labelled('seen marks file seen.json has a non-string value for that is not a plain name', root)); + const f = registrationPath(dir, 'builder', 'repo'); + mkdirSync(dirname(f), { recursive: true }); + writeFileSync(f, JSON.stringify({ [join(dir, 'host')]: 1 })); + assert.throws(() => readRegistration(dir, 'builder', 'repo'), e => { + assert.ok(e instanceof SeatError, e.stack); + assert.equal(e.exitCode, 2); + assert.equal(e.message, 'registration has an unknown field: that is not a plain name: registration repo/builder'); + clean(e.message, root); + return true; + }); +}); + +test('an invalid seat name or layout states the constraint, never the value, and keeps exit 4', t => { + const { root, dir } = scratch(t); + assert.throws(() => registrationPath(dir, join(dir, 'x'), 'repo'), e => e instanceof SeatError && e.exitCode === 4 && e.message === 'invalid seat name (lowercase letters, digits, ., _ or -, at most 64)' && (clean(e.message, root), true)); + assert.throws(() => registrationPath(dir, 'builder', join(dir, 'x')), e => e instanceof SeatError && e.exitCode === 4 && e.message === 'invalid layout (one of repo, fleet, unknown)' && (clean(e.message, root), true)); +}); diff --git a/packages/discord/src/approvals.mjs b/packages/discord/src/approvals.mjs index f0027bbff..4814f5fa7 100644 --- a/packages/discord/src/approvals.mjs +++ b/packages/discord/src/approvals.mjs @@ -22,7 +22,7 @@ import { parseJsonFileText } from "../../organization/src/util.mjs"; import { appendFileSync, existsSync, readFileSync } from "node:fs"; import { join } from "node:path"; -import { DiscordError } from "./errors.mjs"; +import { DiscordError, quietRead } from "./errors.mjs"; export const APPROVE_WORD = "approve"; export const CUSTOM_ID_PREFIX = "approve:"; @@ -102,7 +102,13 @@ export function readApprovals(dir) { const path = approvalsPath(dir); if (!existsSync(path)) return []; const out = []; - for (const [i, line] of readFileSync(path, "utf8").split("\n").entries()) { + let text; + try { + text = readFileSync(path, "utf8"); + } catch (e) { + throw quietRead(e, "approvals.jsonl"); + } + for (const [i, line] of text.split("\n").entries()) { if (line.length === 0) continue; try { out.push(parseJsonFileText(line, `${path}:${i + 1}`)); diff --git a/packages/discord/src/binding.mjs b/packages/discord/src/binding.mjs index 527b97b31..68595f0a4 100644 --- a/packages/discord/src/binding.mjs +++ b/packages/discord/src/binding.mjs @@ -24,7 +24,7 @@ import { loadWebConfig } from "./web.mjs"; import { loadSetsparkConfig } from "./setspark.mjs"; import { basename, isAbsolute, join, resolve, sep } from "node:path"; import { homedir } from "node:os"; -import { DiscordError } from "./errors.mjs"; +import { DiscordError, plainKey, quietRead } from "./errors.mjs"; import { TOOL_DEFAULTS } from "./tools.mjs"; import { loadGitConfig } from "./git.mjs"; @@ -74,13 +74,16 @@ export function discordDir(dataRoot) { return join(dataRoot, "discord"); } +// The name is checked, never echoed: a bad one could be a host path (#1873). +const INVALID_BINDING_NAME = "invalid binding name (lowercase letters, digits, ., _ or -, at most 64)"; + export function bindingPath(dataRoot, name) { - if (!BINDING_NAME.test(String(name))) throw new DiscordError(`invalid binding name: ${JSON.stringify(name)}`, 4); + if (!BINDING_NAME.test(String(name))) throw new DiscordError(INVALID_BINDING_NAME, 4); return join(discordDir(dataRoot), `${name}.json`); } export function bindingDataDir(dataRoot, name) { - if (!BINDING_NAME.test(String(name))) throw new DiscordError(`invalid binding name: ${JSON.stringify(name)}`, 4); + if (!BINDING_NAME.test(String(name))) throw new DiscordError(INVALID_BINDING_NAME, 4); return join(discordDir(dataRoot), name); } @@ -90,7 +93,7 @@ function isObject(v) { function onlyKeys(obj, allowed, where) { for (const key of Object.keys(obj)) { - if (!allowed.includes(key)) throw new DiscordError(`${where}: unknown key ${JSON.stringify(key)}`); + if (!allowed.includes(key)) throw new DiscordError(`${where}: unknown key ${plainKey(key)}`); } } @@ -321,7 +324,12 @@ export function readToken(binding) { // the model's prompt, so this is the boundary that keeps host files out of // Discord Sage (Q14, Q16). Every file must be a regular non-empty file. export function resolveContextFiles(binding, repo) { - const root = realpathSync(repo); + let root; + try { + root = realpathSync(repo); + } catch (e) { + throw quietRead(e, "the repository"); + } // Refusals name the entry by its index in context.files, never its path (#1873). return binding.context.files.map((f, i) => { const at = `context.files[${i}]`; diff --git a/packages/discord/src/errors.mjs b/packages/discord/src/errors.mjs index f15f12477..1cb8eae62 100644 --- a/packages/discord/src/errors.mjs +++ b/packages/discord/src/errors.mjs @@ -1,6 +1,18 @@ // One error class for the package. exitCode follows docs/TOOLS.md: 1 operation // failed, 2 invalid data or configuration, 3 refused by a brake (STOP or a // held binding; a supervisor must not retry), 4 usage. +// A refusal that names an unknown key quotes it only when it is a plain name; +// a key that could carry a host path or a secret is described, never echoed (#1873). +export const plainKey = (k) => (/^[A-Za-z_][A-Za-z0-9_-]{0,63}$/.test(String(k)) ? JSON.stringify(k) : "that is not a plain name"); + +// A read that fails names the file by label and the error code, never the +// path Node puts in the message. The same error object is rethrown, so its +// type, code and the exit it maps to are unchanged (#1873). +export function quietRead(e, label) { + if (e && typeof e === "object" && typeof e.code === "string") e.message = `cannot read ${label} (${e.code})`; + return e; +} + export class DiscordError extends Error { constructor(message, exitCode = 2, details = undefined) { super(message); diff --git a/packages/discord/src/git.mjs b/packages/discord/src/git.mjs index 56105f7fa..44c8dda13 100644 --- a/packages/discord/src/git.mjs +++ b/packages/discord/src/git.mjs @@ -46,6 +46,7 @@ import { spawnSync } from "node:child_process"; import { existsSync, lstatSync } from "node:fs"; import { join } from "node:path"; +import { plainKey } from "./errors.mjs"; import { fileURLToPath } from "node:url"; export const GIT_TOOL_NAMES = Object.freeze(["git_status", "git_commit", "git_pull", "git_push"]); @@ -109,7 +110,7 @@ const AUTHOR = /^([^<>\r\n]{1,64}?) <([^<>\s@]+@[^<>\s@]+)>$/; export function loadGitConfig(raw, where, real) { if (!isObject(raw)) throw new Error(`${where}: not an object`); for (const k of Object.keys(raw)) { - if (!["branch", "identity", "tokenFile", "author", "protocol"].includes(k)) throw new Error(`${where}: unknown key ${JSON.stringify(k)}`); + if (!["branch", "identity", "tokenFile", "author", "protocol"].includes(k)) throw new Error(`${where}: unknown key ${plainKey(k)}`); } if (typeof raw.branch !== "string" || !BRANCH_NAME.test(raw.branch) || raw.branch.includes("..")) throw new Error(`${where}.branch: must be a branch name`); if (typeof raw.identity !== "string" || !IDENTITY.test(raw.identity)) throw new Error(`${where}.identity: must match ${IDENTITY}`); @@ -125,7 +126,7 @@ export function loadGitConfig(raw, where, real) { } catch { // handled below } - if (!st || !(st.isDirectory() || st.isFile())) throw new Error(`${where}: ${real} is not a git work tree`); + if (!st || !(st.isDirectory() || st.isFile())) throw new Error(`${where}: the root is not a git work tree`); } if (process.execPath.includes("'") || CREDENTIAL_HELPER.includes("'")) throw new Error(`${where}: the node or helper path holds a quote and cannot be a credential helper`); return Object.freeze({ @@ -201,7 +202,7 @@ function cap(text) { function run(cmd, args, cwd, env, spawn) { const r = spawn(cmd, args, { cwd, env, encoding: "utf8", timeout: GIT_TIMEOUT_MS, maxBuffer: 4 * 1024 * 1024, stdio: ["ignore", "pipe", "pipe"] }); if (r.error && r.error.code === "ETIMEDOUT") throw new GitRefusal(GIT_REFUSAL.TIMEOUT, `${cmd} ${args[0] || ""}`); - if (r.error) throw new GitRefusal(GIT_REFUSAL.NO_GIT, `${cmd} could not start (${r.error.code || r.error.message})`); + if (r.error) throw new GitRefusal(GIT_REFUSAL.NO_GIT, `${cmd} could not start (${r.error.code || "error"})`); return { status: r.status, stdout: cap(r.stdout || ""), stderr: cap(r.stderr || "") }; } @@ -209,8 +210,12 @@ function git(root, gitCfg, args, { remote = false, spawn = spawnSync } = {}) { return run("git", args, root.real, gitEnv(gitCfg, { remote }), spawn); } +// git output reaches a refusal detail, the journal and Discord. A line that +// carries a host path (an absolute or home path, or a file: URL) is dropped, +// never relayed; repo-relative lines such as a lock owner or a conflict stay (#1873). +const HOST_PATH = /(^|[\s'"(=:,[<])~?\/(?!\/)|file:/i; function firstLines(text, n = 5) { - return text.split("\n").filter((l) => l.trim().length > 0).slice(0, n).join("\n"); + return text.split("\n").filter((l) => l.trim().length > 0 && !HOST_PATH.test(l)).slice(0, n).join("\n"); } // The guard every verb runs first: on the named branch, nothing in diff --git a/packages/discord/src/journal.mjs b/packages/discord/src/journal.mjs index fa5e25cf7..f0bce3e60 100644 --- a/packages/discord/src/journal.mjs +++ b/packages/discord/src/journal.mjs @@ -26,7 +26,7 @@ import { unlinkSync, writeFileSync, writeSync, } from "node:fs"; import { basename, join } from "node:path"; -import { DiscordError } from "./errors.mjs"; +import { DiscordError, quietRead } from "./errors.mjs"; import { BINDING_NAME } from "./binding.mjs"; // Where a refusal says a binding's journal is: its place under the data root @@ -53,7 +53,12 @@ function appendLine(path, record) { function readLines(path) { if (!existsSync(path)) return []; const out = []; - const text = readFileSync(path, "utf8"); + let text; + try { + text = readFileSync(path, "utf8"); + } catch (e) { + throw quietRead(e, basename(path)); + } for (const [i, line] of text.split("\n").entries()) { if (line.length === 0) continue; try { @@ -118,7 +123,7 @@ export function readDrops(dir) { const TURN_ID = /^[A-Za-z0-9][A-Za-z0-9._-]{0,127}$/; export function turnPath(dir, id) { - if (!TURN_ID.test(String(id))) throw new DiscordError(`invalid turn id: ${JSON.stringify(id)}`, 1); + if (!TURN_ID.test(String(id))) throw new DiscordError("invalid turn id (letters, digits, ., _ or -, at most 128)", 1); return join(dir, "turns", `${id}.json`); } diff --git a/packages/discord/src/setspark.mjs b/packages/discord/src/setspark.mjs index 729a58a0b..7978003aa 100644 --- a/packages/discord/src/setspark.mjs +++ b/packages/discord/src/setspark.mjs @@ -28,6 +28,7 @@ import { lstatSync, readFileSync } from "node:fs"; import { request as httpsRequest } from "node:https"; import { request as httpRequest } from "node:http"; +import { plainKey } from "./errors.mjs"; export const SETSPARK_DEFAULTS = Object.freeze({ timeoutMs: 15000, maxResponseBytes: 262144 }); export const USER_AGENT = "mosaic-discord-sage/1 (Mosaic Stack Discord connector; setspark client)"; @@ -69,7 +70,7 @@ const isObject = (v) => v !== null && typeof v === "object" && !Array.isArray(v) export function loadSetsparkConfig(raw, where = "setspark") { if (!isObject(raw)) throw new Error(`${where}: not an object`); for (const k of Object.keys(raw)) { - if (!["baseUrl", "keyFile", "principal", "timeoutMs", "approvers"].includes(k)) throw new Error(`${where}: unknown key ${JSON.stringify(k)}`); + if (!["baseUrl", "keyFile", "principal", "timeoutMs", "approvers"].includes(k)) throw new Error(`${where}: unknown key ${plainKey(k)}`); } if (typeof raw.baseUrl !== "string") throw new Error(`${where}.baseUrl: must be a url string`); let u; diff --git a/packages/discord/tests/binding.test.mjs b/packages/discord/tests/binding.test.mjs index 0e6b07df8..4d5816c9d 100644 --- a/packages/discord/tests/binding.test.mjs +++ b/packages/discord/tests/binding.test.mjs @@ -177,7 +177,8 @@ test("cli: check refuses a missing context file and a missing binding with exit assert.match(r.stderr, /missing context file/); r = runCli(["check", "other-seat", "--config", dep.config, "--repo", repo]); assert.equal(r.status, 2); - assert.match(r.stderr, /binding not found/); + assert.match(r.stderr, /binding other-seat not found/); + assert.ok(!r.stderr.includes(root), r.stderr); r = runCli(["check"]); assert.equal(r.status, 4); r = runCli(["frobnicate", "x"]); diff --git a/packages/discord/tests/notify.test.mjs b/packages/discord/tests/notify.test.mjs index f793eeaa0..024aaa88e 100644 --- a/packages/discord/tests/notify.test.mjs +++ b/packages/discord/tests/notify.test.mjs @@ -75,6 +75,6 @@ test("notify: no dmRecipient, a non-0600 binding or token, or a missing binding assert.throws(() => openDirect({ dataRoot: none.dataRoot, name: "test-seat", fetch }), (e) => e instanceof DiscordError && e.exitCode === 2 && /no dmRecipient/.test(e.message)); const loose = makeDeployment(makeRoot(), { dmRecipient: IDS.owner }, { tokenMode: 0o644 }); assert.throws(() => openDirect({ dataRoot: loose.dataRoot, name: "test-seat", fetch }), (e) => e instanceof DiscordError && /mode 0600/.test(e.message) && !e.message.includes(TOKEN)); - assert.throws(() => openDirect({ dataRoot: none.dataRoot, name: "absent", fetch }), (e) => e instanceof DiscordError && /binding not found/.test(e.message)); + assert.throws(() => openDirect({ dataRoot: none.dataRoot, name: "absent", fetch }), (e) => e instanceof DiscordError && /binding absent not found/.test(e.message) && !e.message.includes(none.dataRoot)); assert.equal(calls, 0); }); diff --git a/packages/discord/tests/path-redaction-labels.test.mjs b/packages/discord/tests/path-redaction-labels.test.mjs index 931f91b8e..5c5615791 100644 --- a/packages/discord/tests/path-redaction-labels.test.mjs +++ b/packages/discord/tests/path-redaction-labels.test.mjs @@ -4,12 +4,13 @@ import { test } from 'node:test'; import assert from 'node:assert/strict'; import { mkdtempSync, mkdirSync, writeFileSync, rmSync } from 'node:fs'; +import { readApprovals } from '../src/approvals.mjs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { DiscordError } from '../src/errors.mjs'; -import { loadDataRoot, readToken, resolveContextFiles, resolveToolRoots } from '../src/binding.mjs'; -import { ensureJournal, lockPath, ownerPath, recover, requestStop, unlock, writeTurn } from '../src/journal.mjs'; -import { loadGitConfig } from '../src/git.mjs'; +import { bindingDataDir, bindingPath, loadDataRoot, readToken, resolveContextFiles, resolveToolRoots, validateBinding } from '../src/binding.mjs'; +import { ensureJournal, lockPath, ownerPath, readInboxIds, recover, requestStop, turnPath, unlock, writeTurn } from '../src/journal.mjs'; +import { gitPull, loadGitConfig } from '../src/git.mjs'; import { loadSetsparkConfig } from '../src/setspark.mjs'; const secret = 'sk-test-NOT-A-SECRET-1234'; @@ -86,3 +87,50 @@ for (const [name, label] of [['bot', 'discord/bot under the data root'], [secret assert.throws(() => writeTurn(journal, 't1', {}), labelled('turn record already exists: t1', root)); }); } + +// Review round (#1873): keys, names, read errors and git output that could carry a host path. +test('an unknown key that is not a plain name is described, never echoed; a plain one is still quoted', t => { + const { root, dir } = scratch(t); + const doc = { [join(dir, 'host')]: 1 }; + assert.throws(() => validateBinding(doc), labelled('unknown key that is not a plain name', root)); + assert.throws(() => loadGitConfig(doc, 'tools.roots[0].git'), labelled('tools.roots[0].git: unknown key that is not a plain name', root, Error)); + assert.throws(() => loadSetsparkConfig(doc, 'setspark'), labelled('setspark: unknown key that is not a plain name', root, Error)); + assert.throws(() => validateBinding({ colour: 1 }), labelled('unknown key "colour"', root)); +}); + +test('an invalid binding name or turn id states the constraint, never the value, and keeps its exit', t => { + const { root, dir } = scratch(t); + for (const f of [bindingPath, bindingDataDir]) { + assert.throws(() => f(dir, join(dir, 'bad')), e => labelled('invalid binding name (lowercase letters, digits, ., _ or -, at most 64)', root)(e) && e.exitCode === 4); + } + assert.throws(() => turnPath(dir, join(dir, 'bad')), e => labelled('invalid turn id (letters, digits, ., _ or -, at most 128)', root)(e) && e.exitCode === 1); +}); + +test('an unreadable journal or approvals file, or a missing repository, names the file and code, never the path', t => { + const { root, dir } = scratch(t); + writeFileSync(join(dir, 'approvals.jsonl'), '{}\n', { mode: 0o000 }); + writeFileSync(join(dir, 'inbox.jsonl'), '{}\n', { mode: 0o000 }); + assert.throws(() => readApprovals(dir), e => labelled('cannot read approvals.jsonl (EACCES)', root, Error)(e) && e.code === 'EACCES' && !(e instanceof DiscordError)); + assert.throws(() => readInboxIds(dir), e => labelled('cannot read inbox.jsonl (EACCES)', root, Error)(e) && e.code === 'EACCES'); + assert.throws(() => resolveContextFiles({ context: { files: ['x'] } }, join(dir, 'missing')), e => labelled('cannot read the repository (ENOENT)', root, Error)(e) && e.code === 'ENOENT'); +}); + +test('a git refusal drops stderr lines that carry a host path and a spawn failure names only its code', t => { + const { root, dir } = scratch(t); + const rootCfg = { real: dir, name: 'docs', git: { branch: 'main', identity: 'sage', tokenFile: join(dir, 'never-read'), author: { name: 'Sage', email: 'sage@example.test' } } }; + let calls = 0; + const spawn = () => ++calls === 1 ? { status: 0, stdout: 'main\n', stderr: '' } : { status: 128, stdout: '', stderr: `fatal: cannot access ${join(dir, 'repo')}\n` }; + assert.throws(() => gitPull(rootCfg, { spawn }), e => { + assert.equal(e.reason, 'that root has no git'); + assert.equal(e.message, e.reason); + assert.ok(!e.detail, String(e.detail)); + return labelled(e.reason, root, Error)(e); + }); + assert.equal(calls, 2); + // A repo-relative line survives: the detail still says what git said. + calls = 0; + const kept = () => ++calls === 1 ? { status: 0, stdout: 'main\n', stderr: '' } : { status: 128, stdout: '', stderr: `fatal: not a git repository: .git\n` }; + assert.throws(() => gitPull(rootCfg, { spawn: kept }), e => e.detail === 'fatal: not a git repository: .git'); + const failed = () => ({ error: Object.assign(new Error(`spawn ${join(dir, 'git')} ENOENT`), { code: 'ENOENT' }) }); + assert.throws(() => gitPull(rootCfg, { spawn: failed }), e => labelled('git could not start (ENOENT)', root, Error)(e)); +}); diff --git a/packages/harness/src/bundle.mjs b/packages/harness/src/bundle.mjs index 93bdb2257..d0cd080e2 100644 --- a/packages/harness/src/bundle.mjs +++ b/packages/harness/src/bundle.mjs @@ -159,7 +159,7 @@ function readSkill(name, root, size) { try { return lstatSync(path); } catch (e) { if (e.code === "ENOENT") throw skillNotEnabled(name); throw e; } }; // scripts/skill.sh activate moves a real directory in, so a symlink here is refused, not followed. - if (!found(join(root, name)).isDirectory()) throw new BundleError(`skill ${name} isn't a plain directory under ${root}`); + if (!found(join(root, name)).isDirectory()) throw new BundleError(`skill ${name} isn't a plain directory under the enabled skills directory`); if (!found(join(root, name, "SKILL.md")).isFile()) throw new BundleError(`skill ${name}: SKILL.md isn't a plain file`); walk(join(root, name), ""); return files; @@ -222,34 +222,36 @@ function readPersona(resolved, personaDir) { throw new BundleError(`instance ${resolved.instance} has a persona, but the bundle has no persona directory`, "persona-invalid"); } const path = join(personaDir, resolved.business, `${resolved.instance}.md`); + // #1873: a refusal names the file under the persona directory, never its host path. + const shown = `${resolved.business}/${resolved.instance}.md in the persona directory`; let fd; try { fd = openSync(path, constants.O_RDONLY | constants.O_NOFOLLOW | constants.O_NONBLOCK); } catch (e) { if (e.code === "ENOENT" && digest === undefined) return null; - if (e.code === "ENOENT") throw new BundleError(`instance ${resolved.instance}: persona file ${path} doesn't exist`, "persona-invalid"); - if (e.code === "ELOOP") throw new BundleError(`instance ${resolved.instance}: persona file ${path} is a symlink`, "persona-invalid"); - throw new BundleError(`instance ${resolved.instance}: persona file ${path} can't be opened (${e.code})`, "persona-invalid"); + if (e.code === "ENOENT") throw new BundleError(`instance ${resolved.instance}: persona file ${shown} doesn't exist`, "persona-invalid"); + if (e.code === "ELOOP") throw new BundleError(`instance ${resolved.instance}: persona file ${shown} is a symlink`, "persona-invalid"); + throw new BundleError(`instance ${resolved.instance}: persona file ${shown} can't be opened (${e.code})`, "persona-invalid"); } let data; try { const stat = fstatSync(fd); - if (!stat.isFile()) throw new BundleError(`instance ${resolved.instance}: persona file ${path} isn't a regular file`, "persona-invalid"); - if (stat.uid !== process.getuid()) throw new BundleError(`instance ${resolved.instance}: persona file ${path} isn't owned by uid ${process.getuid()}`, "persona-invalid"); - if (stat.mode & 0o077) throw new BundleError(`instance ${resolved.instance}: persona file ${path} has group or other mode bits`, "persona-invalid"); + if (!stat.isFile()) throw new BundleError(`instance ${resolved.instance}: persona file ${shown} isn't a regular file`, "persona-invalid"); + if (stat.uid !== process.getuid()) throw new BundleError(`instance ${resolved.instance}: persona file ${shown} isn't owned by uid ${process.getuid()}`, "persona-invalid"); + if (stat.mode & 0o077) throw new BundleError(`instance ${resolved.instance}: persona file ${shown} has group or other mode bits`, "persona-invalid"); // Four bytes is the most one character takes in UTF-8. - if (stat.size > PERSONA_CHARS * 4) throw new BundleError(`instance ${resolved.instance}: persona file ${path} is over ${PERSONA_CHARS} characters`, "persona-invalid"); + if (stat.size > PERSONA_CHARS * 4) throw new BundleError(`instance ${resolved.instance}: persona file ${shown} is over ${PERSONA_CHARS} characters`, "persona-invalid"); data = readFileSync(fd); } finally { closeSync(fd); } if (sha256(data) !== digest) { - throw new BundleError(`instance ${resolved.instance}: persona file ${path} isn't the file persona.sha256 names`, "persona-mismatch"); + throw new BundleError(`instance ${resolved.instance}: persona file ${shown} isn't the file persona.sha256 names`, "persona-mismatch"); } let text; - try { text = utf8.decode(data); } catch { throw new BundleError(`instance ${resolved.instance}: persona file ${path} isn't UTF-8 text`, "persona-invalid"); } + try { text = utf8.decode(data); } catch { throw new BundleError(`instance ${resolved.instance}: persona file ${shown} isn't UTF-8 text`, "persona-invalid"); } const fault = personaTextFault(text); - if (fault) throw new BundleError(`instance ${resolved.instance}: persona file ${path} ${fault}`, "persona-invalid"); + if (fault) throw new BundleError(`instance ${resolved.instance}: persona file ${shown} ${fault}`, "persona-invalid"); return { name: resolved.vars["persona.name"], sha256: digest, text }; } // Between the contract and "## This session". The contract wins on a conflict, and the broker's diff --git a/packages/organization/src/config-command.mjs b/packages/organization/src/config-command.mjs index d75218f5b..45da727cd 100644 --- a/packages/organization/src/config-command.mjs +++ b/packages/organization/src/config-command.mjs @@ -27,6 +27,7 @@ import { configDir, loadOrganization } from "./organization-config.mjs"; import { loadProject, projectFilePath } from "./project.mjs"; import { checkCredentialRef } from "./credentials.mjs"; import { systemVars, resolveInstance } from "./resolve.mjs"; +import { systemConfigProblem } from "./system-config-problem.mjs"; const REPO = resolve(dirname(fileURLToPath(import.meta.url)), "..", "..", ".."); @@ -40,24 +41,6 @@ export const ORG_CONFIG = Object.freeze({ usage: "usage: mosaic org config validate ID | resolve ID INSTANCE [--project ID]", }); -// A failed `mosaic-config.mjs validate` as one refusal line. Render known key paths with fixed -// constraints; never relay the child's stderr, which can quote a host path or a config value. -// The config is named by where it came from, never by host path (PR #1916): `label` when the -// caller knows it (the cli's --instance load, #1873), else from MOSAIC_CONFIG. -export function systemConfigProblem(proc, env, label = null) { - const problem = String(proc.stderr ?? "").trim(); - const field = problem.startsWith('mosaic-config: execution "dataRoot"') ? "dataRoot" - : /^mosaic-config: (?:unsupported )?"?((?:configVersion|environment|dataRoot|execution|console|federation|compactionReadBytes|compactionStreams)(?:\.[A-Za-z][A-Za-z0-9]*|\[\d+\])*)"?(?=[: ])/u.exec(problem)?.[1]; - const name = "a 1-200 character name starting with a letter or digit and containing only letters, digits, . _ : / -"; - const expected = { configVersion: "1", environment: "development or production", dataRoot: "a canonical absolute instance data path", - "execution.backend": "docker", "execution.adapter": "pi or mock", "execution.provider": name, "execution.model": name }; - const detail = problem.startsWith("mosaic-config: configuration is not valid JSON (") - ? "configuration is not valid JSON; expected readable JSON: " - : field ? `${field}: expected ${expected[field] ?? "a value satisfying the system configuration schema"}: ` - : "expected valid system configuration: "; - return `system config problem (mosaic-config exit ${proc.status}): ${detail}${label ?? (env.MOSAIC_CONFIG ? "the system config MOSAIC_CONFIG names" : "the default system config")}${proc.error?.code ? ` (${proc.error.code})` : ""}`; -} - // Thrown after the message is written; runConfigCommand turns it into the exit code. class Exit { constructor(code) { @@ -167,7 +150,8 @@ export function runConfigCommand(argv, { stdout = process.stdout, stderr = proce let loaded = null; if (project !== null) { const entry = Object.hasOwn(organization.projects, project) ? organization.projects[project] : null; - if (!entry) fail(2, `business ${organization.id} declares no project ${JSON.stringify(project)}`); + // The flag is quoted only when it is a plain name, never a path (#1873). + if (!entry) fail(2, `business ${organization.id} declares no project ${/^[a-z0-9][a-z0-9._-]{0,63}$/.test(project) ? JSON.stringify(project) : "by that name"}`); if (entry.kind !== "library") loaded = loadProject(entry.root); } const resolved = resolveInstance({ system: systemVars(config), business: organization, project: loaded, instance }); diff --git a/packages/organization/src/org-registry.mjs b/packages/organization/src/org-registry.mjs index 61188717e..6ef935bec 100644 --- a/packages/organization/src/org-registry.mjs +++ b/packages/organization/src/org-registry.mjs @@ -27,30 +27,34 @@ export function quiet(e, what) { if (message !== null && !quieted.has(e)) { e.message = message; quieted.add(e); } return e; } +// A filesystem call's error keeps its code and loses its path: rethrow(what) for a promise, +// quiet(e, what) in a catch. +const rethrow = what => e => { throw quiet(e, what); }; +const CONFIG = 'the config directory'; const fail = (code, message, path) => { throw new OrgError(code, message, path); }; function idCheck(id) { if (typeof id !== 'string' || !ID_PATTERN.test(id)) fail('org-invalid', 'invalid organization id'); } export function orgRoot(dataRoot, id) { idCheck(id); if (!isAbsolute(dataRoot) || normalize(dataRoot) !== dataRoot) fail('org-invalid', 'data root must be a normalized absolute path'); return join(dataRoot, 'org-runtime', id); } -async function safeDir(path, create = false) { +async function safeDir(path, create = false, what = 'a directory') { if (!isAbsolute(path) || resolve(path) !== path) fail('org-mismatch', 'directory must be a normalized absolute path', path); let current = parse(path).root; for (const part of path.slice(current.length).split('/').filter(Boolean)) { current = join(current, part); try { const st = await fs.lstat(current); if (!st.isDirectory() || st.isSymbolicLink()) fail('org-mismatch', 'directory path contains a symlink or non-directory', current); } - catch (e) { if (e.code !== 'ENOENT' || !create) throw e; await fs.mkdir(current, { mode: 0o700 }).catch(err => { if (err.code !== 'EEXIST') throw err; }); const st = await fs.lstat(current); if (!st.isDirectory() || st.isSymbolicLink()) fail('org-mismatch', 'unsafe directory', current); } + catch (e) { if (e.code !== 'ENOENT' || !create) throw quiet(e, what); await fs.mkdir(current, { mode: 0o700 }).catch(err => { if (err.code !== 'EEXIST') throw quiet(err, what); }); const st = await fs.lstat(current).catch(rethrow(what)); if (!st.isDirectory() || st.isSymbolicLink()) fail('org-mismatch', 'unsafe directory', current); } } } async function readPrivate(file) { const fd = await fs.open(file, constants.O_RDONLY | constants.O_NOFOLLOW | constants.O_NONBLOCK); try { const st = await fd.stat(); if (!st.isFile() || st.uid !== process.getuid() || (st.mode & 0o777) !== 0o600) fail('org-mismatch', 'unsafe metadata file', file); return parseJsonFileText(await fd.readFile('utf8'), file, 'metadata file'); } finally { await fd.close(); } } -async function privateDir(path) { const st = await fs.lstat(path); if (!st.isDirectory() || st.isSymbolicLink() || st.uid !== process.getuid() || (st.mode & 0o077)) fail('org-mismatch', 'unsafe private directory', path); } +async function privateDir(path, what = 'a directory') { const st = await fs.lstat(path).catch(rethrow(what)); if (!st.isDirectory() || st.isSymbolicLink() || st.uid !== process.getuid() || (st.mode & 0o077)) fail('org-mismatch', 'unsafe private directory', path); } function validateRegistry(doc, file) { if (!doc || doc.version !== 1 || typeof doc.instance !== 'string' || !doc.orgs || typeof doc.orgs !== 'object' || Array.isArray(doc.orgs)) fail('org-mismatch', 'invalid registry', file); for (const [id, entry] of Object.entries(doc.orgs)) { idCheck(id); if (!entry || entry.id !== id || typeof entry.root !== 'string' || !['reserved', 'active', 'deleting', 'archived'].includes(entry.state) || !Number.isSafeInteger(entry.revision) || entry.revision < 1 || typeof entry.created !== 'string') fail('org-mismatch', 'invalid registry entry', file); } return doc; } export async function readRegistry({ configDir }) { - await safeDir(configDir); await privateDir(configDir); const file = join(configDir, 'org-runtime.json'); let doc; + await safeDir(configDir, false, CONFIG); await privateDir(configDir, CONFIG); const file = join(configDir, 'org-runtime.json'); let doc; try { doc = await readPrivate(file); } catch (e) { if (e.code === 'ENOENT') return null; if (e instanceof OrgError) throw e; fail('org-mismatch', `invalid registry: ${e instanceof SyntaxError ? e.message : quiet(e, 'registry file').message}`); } return validateRegistry(doc, file); } @@ -67,8 +71,8 @@ export async function readRegistryLocked(registry, ctx) { return validateRegistry(registry, join(held.ctx.configDir, 'org-runtime.json')); } export async function withRegistry(ctx, fn, { write = true } = {}) { - await safeDir(ctx.configDir); await privateDir(ctx.configDir); const lock = join(ctx.configDir, 'org-runtime.lock'); let fd; - for (let n = 0; n < 200; n++) { try { fd = await fs.open(lock, 'wx', 0o600); break; } catch (e) { if (e.code !== 'EEXIST') throw e; await new Promise(r => setTimeout(r, 10)); } } + await safeDir(ctx.configDir, false, CONFIG); await privateDir(ctx.configDir, CONFIG); const lock = join(ctx.configDir, 'org-runtime.lock'); let fd; + for (let n = 0; n < 200; n++) { try { fd = await fs.open(lock, 'wx', 0o600); break; } catch (e) { if (e.code !== 'EEXIST') throw quiet(e, 'the registry lock'); await new Promise(r => setTimeout(r, 10)); } } if (!fd) fail('org-locked', 'registry lock is held', lock); try { const persisted = await readRegistry(ctx); @@ -80,10 +84,10 @@ export async function withRegistry(ctx, fn, { write = true } = {}) { finally { heldRegistries.delete(registry); } if (write) { const file = join(ctx.configDir, 'org-runtime.json'); const temp = join(ctx.configDir, `.org-runtime-${randomBytes(12).toString('hex')}`); - try { await fs.writeFile(temp, JSON.stringify(registry, null, 2) + '\n', { mode: 0o600, flag: 'wx' }); await fs.rename(temp, file); } finally { await fs.rm(temp, { force: true }); } + try { await fs.writeFile(temp, JSON.stringify(registry, null, 2) + '\n', { mode: 0o600, flag: 'wx' }); await fs.rename(temp, file); } catch (e) { throw quiet(e, 'the registry file'); } finally { await fs.rm(temp, { force: true }).catch(rethrow('the registry file')); } } return result; - } finally { await fd.close(); await fs.unlink(lock); } + } finally { await fd.close(); await fs.unlink(lock).catch(rethrow('the registry lock')); } } // The broker's existing organization writer is synchronous. It shares the // registry lock with the async service and refuses contention rather than @@ -92,16 +96,16 @@ export function withRegistryWriter(configDir, fn) { const file = join(configDir, 'org-runtime.json'); const lock = join(configDir, 'org-runtime.lock'); let lockFd; try { lockFd = openSync(lock, constants.O_WRONLY | constants.O_CREAT | constants.O_EXCL | constants.O_NOFOLLOW, 0o600); } - catch (e) { if (e.code === 'EEXIST') fail('org-locked', 'registry lock is held', lock); throw e; } - const release = () => { closeSync(lockFd); unlinkSync(lock); }; + catch (e) { if (e.code === 'EEXIST') fail('org-locked', 'registry lock is held', lock); throw quiet(e, 'the registry lock'); } + const release = () => { closeSync(lockFd); try { unlinkSync(lock); } catch (e) { throw quiet(e, 'the registry lock'); } }; let pending = false; try { let fd, registry = null; try { fd = openSync(file, constants.O_RDONLY | constants.O_NOFOLLOW | constants.O_NONBLOCK); } - catch (e) { if (e.code !== 'ENOENT') throw e; } + catch (e) { if (e.code !== 'ENOENT') throw quiet(e, 'the registry file'); } if (fd !== undefined) { try { - const dir = lstatSync(configDir); + let dir; try { dir = lstatSync(configDir); } catch (e) { throw quiet(e, CONFIG); } if (!dir.isDirectory() || dir.isSymbolicLink() || dir.uid !== process.getuid() || (dir.mode & 0o077)) fail('org-mismatch', 'unsafe private directory', configDir); const st = fstatSync(fd); if (!st.isFile() || st.uid !== process.getuid() || (st.mode & 0o777) !== 0o600) fail('org-mismatch', 'unsafe metadata file', file); @@ -111,7 +115,8 @@ export function withRegistryWriter(configDir, fn) { const publish = () => { const temp = join(configDir, `.org-runtime-${randomBytes(12).toString('hex')}`); try { writeFileSync(temp, JSON.stringify(registry, null, 2) + '\n', { mode: 0o600, flag: 'wx' }); renameSync(temp, file); } - finally { try { unlinkSync(temp); } catch (e) { if (e.code !== 'ENOENT') throw e; } } + catch (e) { throw quiet(e, 'the registry file'); } + finally { try { unlinkSync(temp); } catch (e) { if (e.code !== 'ENOENT') throw quiet(e, 'the registry file'); } } }; const result = fn(registry, publish); if (result !== null && typeof result === 'object' && typeof result.then === 'function') { @@ -126,10 +131,10 @@ export async function reserveOrg(ctx, id) { return withRegistry(ctx, async registry => { if (Object.hasOwn(registry.orgs, id)) fail('org-exists', 'organization already reserved', root); for (const entry of Object.values(registry.orgs)) if (entry.root === root || root.startsWith(entry.root + '/') || entry.root.startsWith(root + '/')) fail('org-overlap', 'organization roots overlap', root); - await safeDir(ctx.dataRoot); await privateDir(ctx.dataRoot); - await safeDir(dirname(root), true); await privateDir(dirname(root)); - try { await fs.mkdir(root, { mode: 0o700 }); } catch (e) { if (e.code === 'EEXIST') fail('org-exists', 'organization root already exists', root); throw e; } - await fs.writeFile(join(root, '.mosaic-org.json'), JSON.stringify({ instance: ctx.instance, id }) + '\n', { mode: 0o600, flag: 'wx' }); + await safeDir(ctx.dataRoot, false, 'the data root'); await privateDir(ctx.dataRoot, 'the data root'); + await safeDir(dirname(root), true, 'org-runtime under the data root'); await privateDir(dirname(root), 'org-runtime under the data root'); + try { await fs.mkdir(root, { mode: 0o700 }); } catch (e) { if (e.code === 'EEXIST') fail('org-exists', 'organization root already exists', root); throw quiet(e, 'the organization root'); } + await fs.writeFile(join(root, '.mosaic-org.json'), JSON.stringify({ instance: ctx.instance, id }) + '\n', { mode: 0o600, flag: 'wx' }).catch(rethrow('the organization marker')); const entry = { id, root, state: 'reserved', created: new Date().toISOString(), revision: 1 }; registry.orgs[id] = entry; return { ...entry }; }); } @@ -150,14 +155,14 @@ export async function activateOrg(ctx, id) { // no document yet; preserve the registry's existing activation contract. const dir = join(ctx.configDir, 'businesses'), file = join(dir, `${id}.json`); try { - await safeDir(dir); + await safeDir(dir, false, 'businesses in the config directory'); const fd = await fs.open(file, constants.O_RDONLY | constants.O_NOFOLLOW | constants.O_NONBLOCK); try { const st = await fd.stat(); if (!st.isFile() || st.uid !== process.getuid() || (st.mode & 0o022)) fail('org-mismatch', 'unsafe organization document', file); registry.orgs[id].digest = createHash('sha256').update(await fd.readFile()).digest('hex'); } finally { await fd.close(); } - } catch (e) { if (e.code !== 'ENOENT') throw e; } + } catch (e) { if (e.code !== 'ENOENT') throw quiet(e, 'the organization document'); } registry.orgs[id].state = 'active'; return { ...registry.orgs[id] }; }); @@ -166,12 +171,12 @@ export async function activateOrg(ctx, id) { // Serialize bus launch with organization lifecycle operations. // A leftover lock is never guessed stale; the operator must inspect it. export async function withOrgLifecycle(ctx, id, fn) { - idCheck(id); await safeDir(ctx.configDir); await privateDir(ctx.configDir); + idCheck(id); await safeDir(ctx.configDir, false, CONFIG); await privateDir(ctx.configDir, CONFIG); const path = join(ctx.configDir, `org-lifecycle-${id}.lock`); let fd; try { fd = await fs.open(path, 'wx', 0o600); } - catch (e) { if (e.code === 'EEXIST') fail('org-locked', 'organization lifecycle lock is held; inspect the lock and resume'); throw e; } + catch (e) { if (e.code === 'EEXIST') fail('org-locked', 'organization lifecycle lock is held; inspect the lock and resume'); throw quiet(e, 'the lifecycle lock'); } try { await fd.writeFile(JSON.stringify({ pid: process.pid, at: new Date().toISOString() }) + '\n'); return await fn(); } - finally { await fd.close(); await fs.unlink(path); } + finally { await fd.close(); await fs.unlink(path).catch(rethrow('the lifecycle lock')); } } // Lifecycle locks carry {pid, at}; registry locks are empty and cannot be @@ -191,7 +196,7 @@ function inspectRecoveryFence(path) { } finally { closeSync(fd); } } export async function orgLock(ctx, id, { clear = false } = {}) { - idCheck(id); await safeDir(ctx.configDir); await privateDir(ctx.configDir); + idCheck(id); await safeDir(ctx.configDir, false, CONFIG); await privateDir(ctx.configDir, CONFIG); const path = join(ctx.configDir, `org-lifecycle-${id}.lock`); const guard = `${path}.clear`; let guardFd, self; if (clear) { @@ -200,16 +205,16 @@ export async function orgLock(ctx, id, { clear = false } = {}) { for (;;) { try { guardFd = openSync(guard, constants.O_WRONLY | constants.O_CREAT | constants.O_EXCL | constants.O_NOFOLLOW, 0o600); break; } catch (e) { - if (e.code !== 'EEXIST') throw e; + if (e.code !== 'EEXIST') throw quiet(e, 'the recovery fence'); const holder = inspectRecoveryFence(guard); if (!holder) continue; if (holder.unreadable) fail('org-locked', `unreadable recovery fence org-lifecycle-${id}.lock.clear in the config directory; remove it by hand`); if (holder.alive !== false) fail('org-locked', `lock recovery is already in progress by pid ${holder.pid}`, guard); - let current; try { current = lstatSync(guard); } catch (err) { if (err.code === 'ENOENT') continue; throw err; } + let current; try { current = lstatSync(guard); } catch (err) { if (err.code === 'ENOENT') continue; throw quiet(err, 'the recovery fence'); } if (current.dev !== holder.dev || current.ino !== holder.ino) fail('org-locked', 'recovery fence changed during recovery', guard); const stamp = new Date().toISOString().replace(/[:.]/g, '-'); try { renameSync(guard, `${guard}.stale-${stamp}-${randomBytes(4).toString('hex')}`); } - catch (err) { if (err.code === 'ENOENT') continue; throw err; } + catch (err) { if (err.code === 'ENOENT') continue; throw quiet(err, 'the recovery fence'); } } } } @@ -219,7 +224,7 @@ export async function orgLock(ctx, id, { clear = false } = {}) { const reportFence = result => { if (fence) { const { dev, ino, ...visible } = fence; result.fence = visible; } return result; }; let fd; try { fd = openSync(path, constants.O_RDONLY | constants.O_NOFOLLOW | constants.O_NONBLOCK); } - catch (e) { if (e.code === 'ENOENT') return reportFence(clear ? { held: false, cleared: false } : { held: false }); throw e; } + catch (e) { if (e.code === 'ENOENT') return reportFence(clear ? { held: false, cleared: false } : { held: false }); throw quiet(e, 'the lifecycle lock'); } try { const st = fstatSync(fd); if (!st.isFile() || st.uid !== process.getuid() || (st.mode & 0o777) !== 0o600 || st.nlink !== 1) fail('org-mismatch', 'unsafe lifecycle lock', path); @@ -228,10 +233,11 @@ export async function orgLock(ctx, id, { clear = false } = {}) { let status; try { status = processState(holder.pid); } catch { fail('org-mismatch', 'lifecycle lock holder is uncertain', path); } const result = { held: true, pid: holder.pid, start: holder.at, alive: status.alive }; if (clear) { - if (status.alive) fail('org-locked', `lifecycle lock held by pid ${holder.pid} since ${holder.at} (alive)`, path); - const current = lstatSync(path); + if (status.alive) fail('org-locked', `lifecycle lock held by pid ${holder.pid} since ${new Date(holder.at).toISOString()} (alive)`, path); + let current; try { current = lstatSync(path); } catch (e) { throw quiet(e, 'the lifecycle lock'); } if (current.dev !== st.dev || current.ino !== st.ino) fail('org-locked', 'lifecycle lock changed during recovery', path); - unlinkSync(path); result.cleared = true; + try { unlinkSync(path); } catch (e) { throw quiet(e, 'the lifecycle lock'); } + result.cleared = true; } return reportFence(result); } finally { closeSync(fd); } @@ -239,9 +245,9 @@ export async function orgLock(ctx, id, { clear = false } = {}) { if (guardFd !== undefined) { const owned = fstatSync(guardFd); try { - const current = lstatSync(guard); + let current; try { current = lstatSync(guard); } catch (e) { throw quiet(e, 'the recovery fence'); } if (owned.dev !== current.dev || owned.ino !== current.ino) fail('org-locked', 'recovery fence changed during recovery', guard); - unlinkSync(guard); + try { unlinkSync(guard); } catch (e) { throw quiet(e, 'the recovery fence'); } } finally { closeSync(guardFd); } } } diff --git a/packages/organization/src/system-config-problem.mjs b/packages/organization/src/system-config-problem.mjs new file mode 100644 index 000000000..6864cfcc8 --- /dev/null +++ b/packages/organization/src/system-config-problem.mjs @@ -0,0 +1,21 @@ +// One refusal line for a failed `mosaic-config.mjs validate`, shared by `mosaic business` / +// `mosaic org config` and the cli's system config load (#1873). Its own module, so the +// config-command module's export list stays what importers pin. + +// A failed `mosaic-config.mjs validate` as one refusal line. Render known key paths with fixed +// constraints; never relay the child's stderr, which can quote a host path or a config value. +// The config is named by where it came from, never by host path (PR #1916): `label` when the +// caller knows it (the cli's --instance load, #1873), else from MOSAIC_CONFIG. +export function systemConfigProblem(proc, env, label = null) { + const problem = String(proc.stderr ?? "").trim(); + const field = problem.startsWith('mosaic-config: execution "dataRoot"') ? "dataRoot" + : /^mosaic-config: (?:unsupported )?"?((?:configVersion|environment|dataRoot|execution|console|federation|compactionReadBytes|compactionStreams)(?:\.[A-Za-z][A-Za-z0-9]*|\[\d+\])*)"?(?=[: ])/u.exec(problem)?.[1]; + const name = "a 1-200 character name starting with a letter or digit and containing only letters, digits, . _ : / -"; + const expected = { configVersion: "1", environment: "development or production", dataRoot: "a canonical absolute instance data path", + "execution.backend": "docker", "execution.adapter": "pi or mock", "execution.provider": name, "execution.model": name }; + const detail = problem.startsWith("mosaic-config: configuration is not valid JSON (") + ? "configuration is not valid JSON; expected readable JSON: " + : field ? `${field}: expected ${expected[field] ?? "a value satisfying the system configuration schema"}: ` + : "expected valid system configuration: "; + return `system config problem (mosaic-config exit ${proc.status}): ${detail}${label ?? (env.MOSAIC_CONFIG ? "the system config MOSAIC_CONFIG names" : "the default system config")}${proc.error?.code ? ` (${proc.error.code})` : ""}`; +} diff --git a/packages/organization/tests/cli.test.mjs b/packages/organization/tests/cli.test.mjs index 111632347..d921c8b0a 100644 --- a/packages/organization/tests/cli.test.mjs +++ b/packages/organization/tests/cli.test.mjs @@ -124,8 +124,8 @@ test("validate: credential reference problems exit 2 and name each one", () => { const r = run(s, "validate", "acme"); assert.equal(r.code, 2); assert.equal(r.out, ""); - assert.match(r.err, /cto-gitea\.token: mode 644/); - assert.match(r.err, /coder\.token: inside .*data/); + assert.match(r.err, /roles\.cto\.credentials\.gitea file: mode 644/); + assert.match(r.err, /roles\.coder\.credentials\.vikunja file: inside the data root/); assert.match(r.err, /expired on 2026-01-01/); assert.match(r.err, /3 credential reference problem/); }); @@ -140,8 +140,8 @@ test("validate: a not-issued store agent dated today passes, and an issued one w }); const r = run(s, "validate", "acme"); assert.equal(r.code, 0, r.err); - assert.match(r.err, /gitea file .*secrets\/agents\/acme\/coder\/gitea: not issued/); - assert.match(r.err, /vikunja file .*secrets\/agents\/acme\/coder\/vikunja: not issued/); + assert.match(r.err, /roles\.coder\.credentials\.gitea store file: not issued/); + assert.match(r.err, /roles\.coder\.credentials\.vikunja store file: not issued/); assert.doesNotMatch(r.err, /expire|rotation/); const issued = setup((doc) => { @@ -150,7 +150,7 @@ test("validate: a not-issued store agent dated today passes, and an issued one w tokenFile(join(issued.root, "config", "secrets", "agents", "acme", "coder"), "vikunja"); const e = run(issued, "validate", "acme"); assert.equal(e.code, 2); - assert.match(e.err, /vikunja file .*coder\/vikunja: expired on 2026-01-01/); + assert.match(e.err, /roles\.coder\.credentials\.vikunja store file: expired on 2026-01-01/); }); test("validate: a token file inside the repository is refused", () => { @@ -211,5 +211,10 @@ test("resolve: refusals", () => { assert.equal(run(s, "resolve", "acme", "ghost").code, 2); assert.equal(run(s, "resolve", "acme", "constructor").code, 2); assert.match(run(s, "resolve", "acme", "pm", "--project", "ghost").err, /declares no project "ghost"/); + // #1873: a --project value that is not a plain name is never echoed. + const odd = run(s, "resolve", "acme", "pm", "--project", "/srv/sk-test-NOT-A-SECRET-1234/x"); + assert.equal(odd.code, 2); + assert.match(odd.err, /declares no project by that name/); + assert.doesNotMatch(odd.err, /sk-test|NOT-A-SECRET|\/srv\//); assert.equal(run(s, "resolve", "acme", "pm", "--project", "stack").code, 4); }); diff --git a/packages/organization/tests/instance-settings.test.mjs b/packages/organization/tests/instance-settings.test.mjs index 993f2a7fa..617d054d9 100644 --- a/packages/organization/tests/instance-settings.test.mjs +++ b/packages/organization/tests/instance-settings.test.mjs @@ -177,7 +177,7 @@ test("T19 add of a credentialed definition writes store references dated today, }); const validated = f.validate(); assert.equal(validated.status, 0, validated.stderr); - assert.match(validated.stderr, /reviewer-2\/vikunja: not issued/); + assert.match(validated.stderr, /roles\.reviewer-2\.credentials\.vikunja store file: not issued/); // The Console's create form keeps listing it as not accepted. assert.equal(f.settings.read("acme").definitions.reviewer.accepted, false); }); diff --git a/packages/organization/tests/path-redaction-labels.test.mjs b/packages/organization/tests/path-redaction-labels.test.mjs index 047b5f199..749ffa3b6 100644 --- a/packages/organization/tests/path-redaction-labels.test.mjs +++ b/packages/organization/tests/path-redaction-labels.test.mjs @@ -7,7 +7,7 @@ import { chmodSync, mkdirSync, mkdtempSync, rmSync, symlinkSync, writeFileSync } import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { checkCredentialRef, parseCredentialRef, storePath } from '../src/credentials.mjs'; -import { OrgError, orgLock, quietMessage, readRegistry } from '../src/org-registry.mjs'; +import { OrgError, orgLock, quietMessage, readRegistry, withRegistryWriter } from '../src/org-registry.mjs'; const secret = 'sk-test-NOT-A-SECRET-1234'; function scratch(t) { @@ -88,3 +88,26 @@ test('an unreadable recovery fence names the fence in the config directory, neve return true; }); }); + +// Review round (#1873): the registry's own filesystem calls name the directory or file by label. +test('a missing config directory names it by label and code, never its path, and keeps the system code', async t => { + const { root, dir } = scratch(t); + await assert.rejects(readRegistry({ configDir: join(dir, 'absent') }), error => { + assert.equal(error.code, 'ENOENT'); + assert.equal(error.message, 'cannot lstat the config directory (ENOENT)'); + clean(error.message, root); + return true; + }); +}); + +test('the registry writer names the registry file by label when it cannot open it, never its path', t => { + const { root, dir } = scratch(t); + writeFileSync(join(dir, 'real'), '{}', { mode: 0o600 }); + symlinkSync(join(dir, 'real'), join(dir, 'org-runtime.json')); + assert.throws(() => withRegistryWriter(dir, () => {}), error => { + assert.equal(error.code, 'ELOOP'); + assert.equal(error.message, 'cannot open the registry file (ELOOP)'); + clean(error.message, root); + return true; + }); +}); diff --git a/packages/plugins/harness/claude-code/index.mjs b/packages/plugins/harness/claude-code/index.mjs index e65f38c57..e72a4b2e6 100644 --- a/packages/plugins/harness/claude-code/index.mjs +++ b/packages/plugins/harness/claude-code/index.mjs @@ -63,7 +63,8 @@ export function preflight({ env, proxy = null }) { if (!binary(env)) { return { code: "harness-binary-missing", - message: env.MOSAIC_CLAUDE_BIN ? `MOSAIC_CLAUDE_BIN ${env.MOSAIC_CLAUDE_BIN} isn't an executable file at an absolute path` : "no claude on the bus host's PATH", + // Names the variable, never its value, which is a host path (#1873). + message: env.MOSAIC_CLAUDE_BIN ? "MOSAIC_CLAUDE_BIN isn't an executable file at an absolute path" : "no claude on the bus host's PATH", }; } return null; diff --git a/packages/plugins/harness/pi/index.mjs b/packages/plugins/harness/pi/index.mjs index 28ae7cc1b..4e07d0c33 100644 --- a/packages/plugins/harness/pi/index.mjs +++ b/packages/plugins/harness/pi/index.mjs @@ -37,7 +37,7 @@ export function preflight({ env = {}, proxy = null } = {}) { && !["ANTHROPIC_API_KEY", "OPENAI_API_KEY", "OPENROUTER_API_KEY", "ZAI_API_KEY"].some(name => Boolean(env[name]))) { return { code: "no-session-credential", message: "pi needs its own provider key or a session proxy key" }; } - if (!existsSync(BIN)) return { code: "harness-binary-missing", message: `pi isn't installed at ${BIN}; run npm ci in the repository` }; + if (!existsSync(BIN)) return { code: "harness-binary-missing", message: "pi isn't installed in the repository's node_modules; run npm ci in the repository" }; return null; } diff --git a/packages/seat/src/sandbox.mjs b/packages/seat/src/sandbox.mjs index 2232afa76..f401bc99e 100644 --- a/packages/seat/src/sandbox.mjs +++ b/packages/seat/src/sandbox.mjs @@ -194,9 +194,10 @@ export function sandboxArgv(list, runDir, command, { business, role, bwrap = BWR // Whether bwrap runs here at all: the namespaces and the system binds, then // setpriv and `true`. Returns null, or why not. export function sandboxProblem({ bwrap = BWRAP } = {}) { - if (!existsSync(bwrap)) return `${bwrap} is missing`; + // #1873: names the sandbox, never the binary's host path; a launch refusal carries this text. + if (!existsSync(bwrap)) return "the sandbox (bwrap) is missing"; const r = spawnSync(bwrap, [...NAMESPACES, ...argsOf(systemBinds()), "--remount-ro", "/", "--", ...KILL_WITH_PARENT, "/usr/bin/true"], { encoding: "utf8", timeout: 10000 }); if (r.status === 0) return null; const why = (r.stderr ?? "").trim().split("\n")[0] || (r.error ? r.error.code : `exit ${r.status ?? r.signal}`); - return `${bwrap} failed: ${why}`; + return `the sandbox (bwrap) failed: ${why}`; } diff --git a/packages/seat/src/seat.mjs b/packages/seat/src/seat.mjs index dd0a827f3..e95ab6611 100644 --- a/packages/seat/src/seat.mjs +++ b/packages/seat/src/seat.mjs @@ -70,11 +70,15 @@ export function seatsDir(dataRoot) { export const LAYOUTS = Object.freeze(["repo", "fleet", "unknown"]); +// A refusal quotes a key or name only when it is a plain name; anything that +// could carry a host path or a secret is described, never echoed (#1873). +export const plainKey = (k) => (/^[A-Za-z_][A-Za-z0-9_.-]{0,63}$/.test(String(k)) ? JSON.stringify(k) : "that is not a plain name"); + // Keyed by layout and seat, because a seat name is not unique across // layouts (this repository and the fleet both have a "darkwing"). export function registrationPath(seats, seat, layout) { - if (!SEAT_NAME.test(String(seat))) throw new SeatError(`invalid seat name: ${JSON.stringify(seat)}`, 4); - if (!LAYOUTS.includes(layout)) throw new SeatError(`invalid layout: ${JSON.stringify(layout)}`, 4); + if (!SEAT_NAME.test(String(seat))) throw new SeatError("invalid seat name (lowercase letters, digits, ., _ or -, at most 64)", 4); + if (!LAYOUTS.includes(layout)) throw new SeatError(`invalid layout (one of ${LAYOUTS.join(", ")})`, 4); return join(seats, layout, seat, "registration.json"); } @@ -143,7 +147,7 @@ const isTimestamp = (v) => typeof v === "string" && Number.isFinite(Date.parse(v // SeatError with the failing field; never echoes the offending value. export function validateRegistration(record) { if (!record || typeof record !== "object" || Array.isArray(record)) throw new SeatError("registration is not an object"); - for (const key of Object.keys(record)) if (!FIELDS.includes(key)) throw new SeatError(`registration has an unknown field: ${key}`); + for (const key of Object.keys(record)) if (!FIELDS.includes(key)) throw new SeatError(`registration has an unknown field: ${plainKey(key)}`); if (record.version !== REGISTRATION_VERSION) throw new SeatError("registration has an unsupported version"); if (typeof record.seat !== "string" || !SEAT_NAME.test(record.seat)) throw new SeatError("registration.seat is invalid"); if (typeof record.task !== "string" || record.task.length > TASK_LIMIT) throw new SeatError("registration.task must be a string"); diff --git a/packages/seat/src/session.mjs b/packages/seat/src/session.mjs index 7d1c49d8d..60964627f 100644 --- a/packages/seat/src/session.mjs +++ b/packages/seat/src/session.mjs @@ -35,7 +35,7 @@ import { spawn } from "node:child_process"; import { randomBytes } from "node:crypto"; import { appendFileSync, closeSync, existsSync, linkSync, lstatSync, mkdirSync, openSync, readdirSync, readFileSync, realpathSync, renameSync, unlinkSync, writeFileSync } from "node:fs"; import { constants } from "node:os"; -import { dirname, join, sep } from "node:path"; +import { basename, dirname, join, relative, sep } from "node:path"; import { childrenOf, cmdlineOf, startTimeOf } from "./proc.mjs"; import { binds, prepareRun, REPO, sandboxArgv } from "./sandbox.mjs"; import { SeatError } from "./seat.mjs"; @@ -71,23 +71,33 @@ function segments(what, pairs, code) { if (!segment(x)) throw seatError(`${what}: ${name} ${JSON.stringify(x)} isn't an id`, code); } } +// A directory under the data root is named by its path relative to the data root, never +// its host path (#1873). Every segment below the root is a checked id or a fixed name. +const under = (dataRoot, p) => `under the data root, ${relative(dataRoot, p) || "the root itself"}`; // Makes each directory in order, then checks it is ours, a real directory and private. -function privateDirs(what, dirs, code) { +function privateDirs(what, dataRoot, dirs, code) { for (const d of dirs) { + // A system error keeps its code and type; its message names the directory by label. + const quiet = (e) => Object.assign(e, { message: `${what}: cannot ${e.syscall ?? "use"} ${under(dataRoot, d)} (${e.code})` }); try { mkdirSync(d, { mode: 0o700 }); } catch (e) { - if (e.code !== "EEXIST") throw e; + if (e.code !== "EEXIST") throw quiet(e); } - const st = lstatSync(d); - if (st.isSymbolicLink() || !st.isDirectory() || st.uid !== process.getuid() || st.mode & 0o077) throw seatError(`${what}: ${d} is unsafe`, code); + let st; + try { + st = lstatSync(d); + } catch (e) { + throw quiet(e); + } + if (st.isSymbolicLink() || !st.isDirectory() || st.uid !== process.getuid() || st.mode & 0o077) throw seatError(`${what}: ${under(dataRoot, d)} is unsafe`, code); } } export function threadStore(dataRoot, business, thread) { segments("thread store", [["business", business], ["thread", thread]], "thread-store-refused"); const dir = join(dataRoot, "threads", business, thread); const out = { dir, session: join(dir, "session"), projects: join(dir, "claude-projects") }; - privateDirs("thread store", [join(dataRoot, "threads"), join(dataRoot, "threads", business), dir, out.session, out.projects], "thread-store-refused"); + privateDirs("thread store", dataRoot, [join(dataRoot, "threads"), join(dataRoot, "threads", business), dir, out.session, out.projects], "thread-store-refused"); return out; } @@ -110,11 +120,11 @@ export function sessionStore(dataRoot, business, thread, session, { legacy = fal if (e.code !== "ENOENT") throw e; old = false; } - if (old) throw seatError(`thread store: ${join(threadDir, "session")} holds history no legacy session maps`, "legacy-store-unmapped"); + if (old) throw seatError(`thread store: ${under(dataRoot, join(threadDir, "session"))} holds history no legacy session maps`, "legacy-store-unmapped"); } const dir = join(threadDir, "sessions", session); const out = { dir, session: join(dir, "session"), projects: join(dir, "claude-projects") }; - privateDirs("thread store", [join(dataRoot, "threads"), join(dataRoot, "threads", business), threadDir, join(threadDir, "sessions"), dir, out.session, out.projects], "thread-store-refused"); + privateDirs("thread store", dataRoot, [join(dataRoot, "threads"), join(dataRoot, "threads", business), threadDir, join(threadDir, "sessions"), dir, out.session, out.projects], "thread-store-refused"); return out; } @@ -138,7 +148,7 @@ export function sessionWorkspace(dataRoot, business, instance, session, { live = segments("session workspace", [["business", business], ["instance", instance], ["session", session]], "session-workspace-unsafe"); const root = join(dataRoot, "session-workspaces"); const path = join(root, business, instance, session); - privateDirs("session workspace", [root, join(root, business), join(root, business, instance), path], "session-workspace-unsafe"); + privateDirs("session workspace", dataRoot, [root, join(root, business), join(root, business, instance), path], "session-workspace-unsafe"); const real = realpathSync(path); const legacy = join(dataRoot, "workspaces"); const named = [legacy, join(legacy, business), join(legacy, business, instance)]; @@ -152,11 +162,11 @@ export function sessionWorkspace(dataRoot, business, instance, session, { live = }; for (const p of [...named, ...entries(legacy), ...entries(join(legacy, business))]) { const other = realOrNull(p); - if (other && (inside(real, other) || inside(other, real))) throw seatError(`session workspace: ${path} overlaps the legacy workspace ${p}`, "session-workspace-overlap"); + if (other && (inside(real, other) || inside(other, real))) throw seatError(`session workspace: ${under(dataRoot, path)} overlaps the legacy workspace ${relative(dataRoot, p)}`, "session-workspace-overlap"); } for (const p of live) { const other = realOrNull(p); - if (other && other !== real && (inside(real, other) || inside(other, real))) throw seatError(`session workspace: ${path} overlaps the live workspace ${p}`, "session-workspace-overlap"); + if (other && other !== real && (inside(real, other) || inside(other, real))) throw seatError(`session workspace: ${under(dataRoot, path)} overlaps a live workspace`, "session-workspace-overlap"); } return path; } @@ -171,7 +181,8 @@ export function lockSessionWorkspace(workspace, owner) { const file = `${workspace}.lock`; let held = { ...owner }; const write = (flag) => writeFileSync(file, `${JSON.stringify(held)}\n`, { mode: 0o600, flag }); - const busy = (why) => seatError(`session workspace ${workspace} is busy: ${why}`, "session-workspace-busy"); + // Named by its session id (the workspace's last segment, a checked id), never its path (#1873). + const busy = (why) => seatError(`session workspace ${basename(workspace)} is busy: ${why}`, "session-workspace-busy"); for (let attempt = 0; attempt < 2; attempt++) { try { write("wx"); @@ -209,7 +220,7 @@ export function lockSessionWorkspace(workspace, owner) { other = JSON.parse(raw); } catch {} // An unreadable lock may be one being written, so it counts as held. - if (!other || typeof other !== "object") throw busy(`${file} is unreadable`); + if (!other || typeof other !== "object") throw busy("its lock is unreadable"); if (lockAlive(other.hostPid, other.hostStartTime)) throw busy(`host pid ${other.hostPid} holds it`); if (lockAlive(other.runPid, other.runStartTime)) throw busy(`run pid ${other.runPid} holds it`); // Stale. Move it aside; if what moved isn't what was judged stale, another @@ -226,11 +237,11 @@ export function lockSessionWorkspace(workspace, owner) { linkSync(aside, file); } catch {} unlinkSync(aside); - throw busy(`another host took ${file} over`); + throw busy("another host took its lock over"); } unlinkSync(aside); } - throw busy(`${file} could not be taken`); + throw busy("its lock could not be taken"); } // The session's environment is an allowlist: nothing else from the host's @@ -279,7 +290,7 @@ export function newRun(dataRoot, business) { if (e.code === "EEXIST") continue; throw e; } - if (Buffer.byteLength(join(runDir, "tools.sock")) > 107) throw new SeatError(`data root path too long for a socket: ${runDir}`, 2); + if (Buffer.byteLength(join(runDir, "tools.sock")) > 107) throw new SeatError(`data root path too long for a socket: run ${run}`, 2); return { run, runDir }; } throw new SeatError("could not pick a free run id", 1); diff --git a/packages/seat/tests/path-redaction-labels.test.mjs b/packages/seat/tests/path-redaction-labels.test.mjs index 99976a8bf..21183a78b 100644 --- a/packages/seat/tests/path-redaction-labels.test.mjs +++ b/packages/seat/tests/path-redaction-labels.test.mjs @@ -7,7 +7,7 @@ import { chmodSync, mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:f import { tmpdir } from 'node:os'; import { dirname, join } from 'node:path'; import { SeatError, loadDataRoot, readRegistration, registrationPath, resolveSeat } from '../src/seat.mjs'; -import { readSessions, sessionsFile } from '../src/session.mjs'; +import { lockSessionWorkspace, newRun, readSessions, sessionStore, sessionWorkspace, sessionsFile, threadStore } from '../src/session.mjs'; const secret = 'sk-test-NOT-A-SECRET-1234'; function scratch(t) { @@ -68,3 +68,53 @@ test('an unreadable or malformed sessions file is named by its place under the d write(sessionsFile(dir), '{"sessionsVersion":0}'); assert.throws(() => readSessions(dir), labelled('sessions file is malformed: bus-host/sessions.json under the data root', root)); }); + +// Review round (#1873): session stores, workspaces, locks and run directories are named under +// the data root by their checked ids, never by host path; codes and exits are unchanged. +function coded(label, root, code) { + return error => labelled(label, root)(error) && error.code === code && error.exitCode === 2; +} + +test('an unsafe or unmakeable thread store directory is named under the data root, never by path', t => { + const { root, dir } = scratch(t); + mkdirSync(join(dir, 'threads'), { mode: 0o755 }); + chmodSync(join(dir, 'threads'), 0o755); + assert.throws(() => threadStore(dir, 'acme', 't1'), coded('thread store: under the data root, threads is unsafe', root, 'thread-store-refused')); + // A system error keeps its code; its message names the directory by label. + assert.throws(() => threadStore(join(dir, 'absent'), 'acme', 't1'), error => { + assert.equal(error.code, 'ENOENT'); + assert.equal(error.message, 'thread store: cannot mkdir under the data root, threads (ENOENT)'); + assert.ok(!error.message.includes(root), error.message); + return true; + }); +}); + +test('a legacy session store left behind is named under the data root, never by path', t => { + const { root, dir } = scratch(t); + mkdirSync(join(dir, 'threads', 'acme', 't1', 'session'), { recursive: true, mode: 0o700 }); + assert.throws(() => sessionStore(dir, 'acme', 't1', 's1'), coded('thread store: under the data root, threads/acme/t1/session holds history no legacy session maps', root, 'legacy-store-unmapped')); +}); + +test('a session workspace that overlaps a live one is named under the data root, never by path', t => { + const { root, dir } = scratch(t); + sessionWorkspace(dir, 'acme', 'i1', 's1'); + assert.throws(() => sessionWorkspace(dir, 'acme', 'i1', 's2', { live: [join(dir, 'session-workspaces', 'acme')] }), coded('session workspace: under the data root, session-workspaces/acme/i1/s2 overlaps a live workspace', root, 'session-workspace-overlap')); +}); + +test('a busy session workspace lock names the session, never the lock path', t => { + const { root, dir } = scratch(t); + const ws = join(dir, 's1'); + writeFileSync(`${ws}.lock`, 'not json'); + assert.throws(() => lockSessionWorkspace(ws, {}), coded('session workspace s1 is busy: its lock is unreadable', root, 'session-workspace-busy')); +}); + +test('a data root too long for a socket names the run, never its directory', t => { + const { root, dir } = scratch(t); + assert.throws(() => newRun(join(dir, 'a'.repeat(70)), 'acme'), error => { + assert.ok(error instanceof SeatError, error.stack); + assert.equal(error.exitCode, 2); + assert.match(error.message, /^data root path too long for a socket: run r[0-9a-f]{12}$/); + assert.ok(!error.message.includes(root), error.message); + return true; + }); +}); diff --git a/packages/seat/tests/sandbox.test.mjs b/packages/seat/tests/sandbox.test.mjs index 08da8d152..c7377a1c8 100644 --- a/packages/seat/tests/sandbox.test.mjs +++ b/packages/seat/tests/sandbox.test.mjs @@ -66,11 +66,12 @@ test("row 331 mounts only the parsing validator and fixed marker code, never pro test("sandboxProblem names a missing bwrap and a bwrap that fails", (t) => { const root = scratch(t); - assert.equal(sandboxProblem({ bwrap: join(root, "nope") }), `${join(root, "nope")} is missing`); + // #1873: the sandbox by name, never the binary's host path. + assert.equal(sandboxProblem({ bwrap: join(root, "nope") }), "the sandbox (bwrap) is missing"); const bad = join(root, "bwrap"); writeFileSync(bad, "#!/bin/sh\necho 'bwrap: No permissions to create new namespace' >&2\nexit 1\n"); chmodSync(bad, 0o755); - assert.equal(sandboxProblem({ bwrap: bad }), `${bad} failed: bwrap: No permissions to create new namespace`); + assert.equal(sandboxProblem({ bwrap: bad }), "the sandbox (bwrap) failed: bwrap: No permissions to create new namespace"); if (!skip) assert.equal(sandboxProblem(), null); }); diff --git a/packages/webui/src/sso-config.mjs b/packages/webui/src/sso-config.mjs index 1238d2842..04f7e7143 100644 --- a/packages/webui/src/sso-config.mjs +++ b/packages/webui/src/sso-config.mjs @@ -22,7 +22,8 @@ function refuse(where, why) { } function keys(value, where, allowed, required = allowed) { if (!object(value)) refuse(where, 'must be a JSON object'); - for (const key of Object.keys(value)) if (!allowed.includes(key)) refuse(`${where}.${key}`, 'is not a supported key'); + // #1873: an unknown key is named only when it is a plain name; a key can be anything, a host path too. + for (const key of Object.keys(value)) if (!allowed.includes(key)) refuse(/^[A-Za-z_][A-Za-z0-9_-]{0,63}$/.test(key) ? `${where}.${key}` : where, /^[A-Za-z_][A-Za-z0-9_-]{0,63}$/.test(key) ? 'is not a supported key' : 'has a key that is not a plain name'); for (const key of required) if (!Object.hasOwn(value, key)) refuse(`${where}.${key}`, 'is required'); } function httpsURL(value, where) { diff --git a/scripts/mosaic-config.mjs b/scripts/mosaic-config.mjs index 2567b13ea..6c8220c41 100755 --- a/scripts/mosaic-config.mjs +++ b/scripts/mosaic-config.mjs @@ -67,6 +67,16 @@ function fail(exitCode, message) { throw new Failure(exitCode, message); } +// #1873: a refusal names the config by where it came from, the data root and its marker by +// their roles, and a key or argument only when it is a plain name; never a host path or a raw +// error message. CONFIG is set when the selection is made. +let CONFIG = "the default system config"; +function configLabel(instance) { + return isNamed(instance) ? `the config of instance ${instance}` : process.env.MOSAIC_CONFIG ? "the system config MOSAIC_CONFIG names" : "the default system config"; +} +const plain = (value, otherwise) => /^[A-Za-z_][A-Za-z0-9_.-]{0,63}$/.test(String(value)) ? JSON.stringify(String(value)) : otherwise; +const MARKER_LABEL = "the data root's instance marker"; + function instancePath(name) { return path.join(os.homedir(), ".config", `mosaic-${name}`, "config.json"); } @@ -82,6 +92,7 @@ function realOrNull(file) { // The config file for a selection. Without --instance, as before. With it, // NAME's file; a $MOSAIC_CONFIG beside it must be the same file. function configPath(instance) { + CONFIG = configLabel(instance); if (instance === null) { return process.env.MOSAIC_CONFIG ? path.resolve(process.env.MOSAIC_CONFIG) @@ -92,7 +103,7 @@ function configPath(instance) { const given = path.resolve(process.env.MOSAIC_CONFIG); const same = given === named || (realOrNull(given) !== null && realOrNull(given) === realOrNull(named)); if (!same) { - fail(4, `instance-config-mismatch: --instance ${instance} names ${named}, but MOSAIC_CONFIG names ${given}`); + fail(4, `instance-config-mismatch: --instance ${instance} names its own config, but MOSAIC_CONFIG names another file`); } } return named; @@ -106,7 +117,7 @@ function parseArgs(args) { } const name = args[1]; if (!INSTANCE_PATTERN.test(name)) { - fail(4, `instance-name-invalid: ${JSON.stringify(name)} must match ${INSTANCE_PATTERN}`); + fail(4, `instance-name-invalid: ${plain(name, "that name")} must match ${INSTANCE_PATTERN}`); } return name; } @@ -124,20 +135,20 @@ function readMarker(dataRoot) { stat = fs.lstatSync(file); } catch (error) { if (error.code === "ENOENT") return null; - fail(3, `instance-root-unclaimed: cannot read ${file} (${error.code})`); + fail(3, `instance-root-unclaimed: cannot read ${MARKER_LABEL} (${error.code})`); } if (!stat.isFile() || stat.isSymbolicLink()) { - fail(3, `instance-root-unclaimed: ${file} must be a regular, non-symbolic-link file`); + fail(3, `instance-root-unclaimed: ${MARKER_LABEL} must be a regular, non-symbolic-link file`); } let marker; try { marker = JSON.parse(fs.readFileSync(file, "utf8")); } catch (error) { - fail(3, `instance-root-unclaimed: ${file} is not a valid marker (${error instanceof SyntaxError ? 'not valid JSON' : error.message})`); + fail(3, `instance-root-unclaimed: ${MARKER_LABEL} is not a valid marker (${error instanceof SyntaxError ? 'not valid JSON' : `cannot read it, ${error.code ?? "error"}`})`); } if (!isPlainObject(marker) || Object.keys(marker).sort().join(",") !== "instanceVersion,name" || marker.instanceVersion !== 1 || typeof marker.name !== "string" || !INSTANCE_PATTERN.test(marker.name)) { - fail(3, `instance-root-unclaimed: ${file} is not a valid marker ({"instanceVersion":1,"name":NAME})`); + fail(3, `instance-root-unclaimed: ${MARKER_LABEL} is not a valid marker ({"instanceVersion":1,"name":NAME})`); } return marker.name; } @@ -148,13 +159,13 @@ function checkClaim(dataRoot, instance) { const owner = readMarker(dataRoot); if (isNamed(instance)) { if (owner === null) { - fail(3, `instance-root-unclaimed: ${dataRoot} holds no ${MARKER} naming ${instance} (scripts/bootstrap.sh --instance ${instance} claims a new root)`); + fail(3, `instance-root-unclaimed: the data root holds no ${MARKER} naming ${instance} (scripts/bootstrap.sh --instance ${instance} claims a new root)`); } if (owner !== instance) { - fail(3, `instance-root-mismatch: ${dataRoot} is claimed by instance ${owner}, not ${instance}`); + fail(3, `instance-root-mismatch: the data root is claimed by instance ${owner}, not ${instance}`); } } else if (owner !== null && owner !== DEFAULT_INSTANCE) { - fail(3, `instance-root-mismatch: ${dataRoot} is claimed by instance ${owner}; select it with --instance ${owner}`); + fail(3, `instance-root-mismatch: the data root is claimed by instance ${owner}; select it with --instance ${owner}`); } } @@ -166,19 +177,19 @@ function readRaw(file) { return null; // missing } if (!stat.isFile() || stat.isSymbolicLink()) { - fail(2, `configuration path must be a regular, non-symbolic-link file: ${file}`); + fail(2, `configuration path must be a regular, non-symbolic-link file: ${CONFIG}`); } try { return fs.readFileSync(file, "utf8"); } catch { - fail(2, `configuration file is not readable: ${file}`); + fail(2, `configuration file is not readable: ${CONFIG}`); } } function rejectUnknownKeys(object, allowed, where) { for (const key of Object.keys(object)) { if (!allowed.includes(key)) { - fail(2, `unsupported ${where} key: "${key}"`); + fail(2, `unsupported ${where} key: ${plain(key, "that is not a plain name")}`); } } } @@ -206,11 +217,11 @@ function validateDataRoot(value, file) { } const home = path.resolve(os.homedir()); if (resolved === home || isAncestorOf(resolved, home)) { - fail(2, `execution "dataRoot" must not be or contain the home directory (${home})`); + fail(2, 'execution "dataRoot" must not be or contain the home directory'); } const configDir = path.dirname(path.resolve(file)); if (resolved === configDir || isAncestorOf(resolved, configDir)) { - fail(2, `execution "dataRoot" must not be or contain the configuration directory (${configDir})`); + fail(2, 'execution "dataRoot" must not be or contain the configuration directory'); } return resolved; } @@ -378,14 +389,13 @@ async function load(file, instance = null) { if (raw === null) { const bootstrap = isNamed(instance) ? `scripts/bootstrap.sh --instance ${instance}` : "scripts/bootstrap.sh"; // #1873: named by where it came from, never by host path. - const which = isNamed(instance) ? `the config of instance ${instance}` : process.env.MOSAIC_CONFIG ? "the system config MOSAIC_CONFIG names" : "the default system config"; - fail(3, `configuration not found: ${which} (run ${bootstrap} to create it)`); + fail(3, `configuration not found: ${configLabel(instance)} (run ${bootstrap} to create it)`); } let document; try { document = JSON.parse(raw); } catch { - fail(2, `configuration is not valid JSON (${file})`); + fail(2, `configuration is not valid JSON (${CONFIG})`); } return await validate(document, file); } @@ -411,7 +421,7 @@ const DEFAULT_CONFIG = defaultConfig(path.join(os.homedir(), ".mosaic-dev")); // 'wx': creation is exclusive; an existing file is never overwritten, and a // symbolic link at the path is never followed. -function createExclusive(file, text, label = file) { +function createExclusive(file, text, label) { let fd; try { fd = fs.openSync(file, "wx", 0o644); @@ -419,7 +429,7 @@ function createExclusive(file, text, label = file) { return true; } catch (error) { if (error.code === "EEXIST") return false; - fail(1, `unable to create ${label}: ${error.message}`); + fail(1, `unable to create ${label} (${error.code ?? "error"})`); } finally { if (fd !== undefined) fs.closeSync(fd); } @@ -454,23 +464,23 @@ async function bootstrapNamed(file, instance) { let entries = null; try { const stat = fs.lstatSync(dataRoot); - if (!stat.isDirectory()) fail(3, `instance-root-unclaimed: ${dataRoot} exists and is not a directory; nothing was written`); + if (!stat.isDirectory()) fail(3, `instance-root-unclaimed: the instance's data root exists and is not a directory; nothing was written`); entries = fs.readdirSync(dataRoot); } catch (error) { if (error instanceof Failure) throw error; - if (error.code !== "ENOENT") fail(3, `instance-root-unclaimed: cannot read ${dataRoot} (${error.code}); nothing was written`); + if (error.code !== "ENOENT") fail(3, `instance-root-unclaimed: cannot read the instance's data root (${error.code}); nothing was written`); } if (entries !== null && entries.length > 0 && readMarker(dataRoot) !== instance) { - fail(3, `instance-root-unclaimed: ${dataRoot} exists, is not empty and holds no ${MARKER} naming ${instance}; nothing was written`); + fail(3, `instance-root-unclaimed: the instance's data root exists, is not empty and holds no ${MARKER} naming ${instance}; nothing was written`); } const configDir = path.dirname(file); fs.mkdirSync(path.dirname(configDir), { recursive: true }); try { fs.mkdirSync(configDir, { mode: 0o700 }); } catch (error) { - if (error.code !== "EEXIST") fail(1, `unable to create ${configDir}: ${error.message}`); + if (error.code !== "EEXIST") fail(1, `unable to create the config directory of instance ${instance} (${error.code ?? "error"})`); } - if (!createExclusive(file, `${JSON.stringify(defaultConfig(dataRoot), null, 2)}\n`)) { + if (!createExclusive(file, `${JSON.stringify(defaultConfig(dataRoot), null, 2)}\n`, CONFIG)) { const resolved = await load(file, instance); checkClaim(resolved.dataRoot, instance); return; @@ -479,9 +489,9 @@ async function bootstrapNamed(file, instance) { try { fs.mkdirSync(resolved.dataRoot, { mode: 0o700 }); } catch (error) { - if (error.code !== "EEXIST") fail(1, `unable to create ${resolved.dataRoot}: ${error.message}`); + if (error.code !== "EEXIST") fail(1, `unable to create the data root (${error.code ?? "error"})`); } - createExclusive(path.join(resolved.dataRoot, MARKER), `${JSON.stringify({ instanceVersion: 1, name: instance })}\n`); + createExclusive(path.join(resolved.dataRoot, MARKER), `${JSON.stringify({ instanceVersion: 1, name: instance })}\n`, MARKER_LABEL); checkClaim(resolved.dataRoot, instance); process.stderr.write(`mosaic-config: created configuration for instance ${instance}: ${file}, data root ${resolved.dataRoot}\n`); } @@ -533,7 +543,7 @@ const operation = process.argv[2]; try { if (!["bootstrap", "validate", "env", "instance"].includes(operation)) { - fail(1, `unknown operation: ${JSON.stringify(operation ?? "")} (expected bootstrap | validate | env | instance)`); + fail(1, `unknown operation: ${plain(operation ?? "", "that is not one")} (expected bootstrap | validate | env | instance)`); } const instance = parseArgs(process.argv.slice(3)); switch (operation) { -- 2.54.0 From 4a9d9d4b894a7e8b997d05a212aafba7541c5c1c Mon Sep 17 00:00:00 2001 From: Jason Woltje Date: Wed, 11 Nov 2026 02:23:08 -0600 Subject: [PATCH 3/8] fix(business): old-path re-export for system-config-problem.mjs (#1873) rename-b-identity pins one packages/business/src module per moved organization module; the review round's new module needs its shim. --- packages/business/src/system-config-problem.mjs | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 packages/business/src/system-config-problem.mjs diff --git a/packages/business/src/system-config-problem.mjs b/packages/business/src/system-config-problem.mjs new file mode 100644 index 000000000..7f1324563 --- /dev/null +++ b/packages/business/src/system-config-problem.mjs @@ -0,0 +1,2 @@ +// Old path of packages/organization/src/system-config-problem.mjs (rename design r2, stage B). +export * from "../../organization/src/system-config-problem.mjs"; -- 2.54.0 From 809b1b01000e6ee61e846d17d83cfaa0e10a2ee0 Mon Sep 17 00:00:00 2001 From: Jason Woltje Date: Wed, 11 Nov 2026 02:27:33 -0600 Subject: [PATCH 4/8] test(control-board): board API body carries the seen marks label, not the path (#1873) The board API returns a scan refusal in its JSON error body. The test starts a board on port 0 over a seen.json that is not an object and checks the body names seen.json by label, with no sentinel or scratch path. It fails on BASE, where the body quotes the host path. --- .../tests/path-redaction-labels.test.mjs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/packages/control-board/tests/path-redaction-labels.test.mjs b/packages/control-board/tests/path-redaction-labels.test.mjs index 194492605..29167ba50 100644 --- a/packages/control-board/tests/path-redaction-labels.test.mjs +++ b/packages/control-board/tests/path-redaction-labels.test.mjs @@ -8,6 +8,7 @@ import { tmpdir } from 'node:os'; import { dirname, join } from 'node:path'; import { ConfigError, loadConfig, loadRegistrations, loadSeen } from '../src/scan.mjs'; import { readRegistration, registrationPath, SeatError } from '../../seat/src/seat.mjs'; +import { startServer } from '../src/serve.mjs'; const secret = 'sk-test-NOT-A-SECRET-1234'; function scratch(t) { @@ -82,3 +83,16 @@ test('an invalid seat name or layout states the constraint, never the value, and assert.throws(() => registrationPath(dir, join(dir, 'x'), 'repo'), e => e instanceof SeatError && e.exitCode === 4 && e.message === 'invalid seat name (lowercase letters, digits, ., _ or -, at most 64)' && (clean(e.message, root), true)); assert.throws(() => registrationPath(dir, 'builder', join(dir, 'x')), e => e instanceof SeatError && e.exitCode === 4 && e.message === 'invalid layout (one of repo, fleet, unknown)' && (clean(e.message, root), true)); }); + +// The board API returns a scan refusal in its JSON body, so the HTTP body carries the label too. +test('a seen marks refusal reaches the board API body by label, never by path', async t => { + const { root, dir } = scratch(t); + write(join(dir, 'seen.json'), '[]'); + const server = await startServer({ host: '127.0.0.1', port: 0, specs: [], boardDir: dir, isAlive: () => true, page: '' }); + t.after(() => new Promise(done => server.close(done))); + const res = await fetch(`http://127.0.0.1:${server.address().port}/api/board`); + const body = await res.text(); + assert.equal(res.status, 500); + assert.deepEqual(JSON.parse(body), { error: 'seen marks file seen.json must be a JSON object' }); + clean(body, root); +}); -- 2.54.0 From c3fe6b77ad3aac250d7b762cb204ac766faef6f2 Mon Sep 17 00:00:00 2001 From: Jason Woltje Date: Wed, 11 Nov 2026 02:50:45 -0600 Subject: [PATCH 5/8] fix(discord): journal.mjs takes BINDING_NAME from errors.mjs, not binding.mjs (#1873) The journal label import pulled binding.mjs and its config readers into every journal.mjs importer. The queue test repo copies only journal.mjs and errors.mjs, so its genesis failed with ERR_MODULE_NOT_FOUND (webui reads.test.mjs red, 4 tests). BINDING_NAME now lives in errors.mjs and binding.mjs re-exports it, so its export is unchanged. --- packages/discord/src/binding.mjs | 4 ++-- packages/discord/src/errors.mjs | 4 ++++ packages/discord/src/journal.mjs | 3 +-- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/packages/discord/src/binding.mjs b/packages/discord/src/binding.mjs index 68595f0a4..c2e492f72 100644 --- a/packages/discord/src/binding.mjs +++ b/packages/discord/src/binding.mjs @@ -24,12 +24,12 @@ import { loadWebConfig } from "./web.mjs"; import { loadSetsparkConfig } from "./setspark.mjs"; import { basename, isAbsolute, join, resolve, sep } from "node:path"; import { homedir } from "node:os"; -import { DiscordError, plainKey, quietRead } from "./errors.mjs"; +import { BINDING_NAME, DiscordError, plainKey, quietRead } from "./errors.mjs"; import { TOOL_DEFAULTS } from "./tools.mjs"; import { loadGitConfig } from "./git.mjs"; export const BINDING_VERSION = 1; -export const BINDING_NAME = /^[a-z0-9][a-z0-9._-]{0,63}$/; +export { BINDING_NAME }; export const SNOWFLAKE = /^[0-9]{17,20}$/; export const CHANNEL_MODES = Object.freeze(["open", "mention"]); export const THINKING_LEVELS = Object.freeze(["off", "minimal", "low", "medium", "high", "xhigh", "max"]); diff --git a/packages/discord/src/errors.mjs b/packages/discord/src/errors.mjs index 1cb8eae62..7b5c968d3 100644 --- a/packages/discord/src/errors.mjs +++ b/packages/discord/src/errors.mjs @@ -4,6 +4,10 @@ // A refusal that names an unknown key quotes it only when it is a plain name; // a key that could carry a host path or a secret is described, never echoed (#1873). export const plainKey = (k) => (/^[A-Za-z_][A-Za-z0-9_-]{0,63}$/.test(String(k)) ? JSON.stringify(k) : "that is not a plain name"); +// A binding (and seat) name. Defined here, not in binding.mjs, so journal.mjs can label a +// journal directory without importing binding.mjs and its config readers; copies of +// journal.mjs + errors.mjs (the queue test repo) stay self-contained. binding.mjs re-exports it. +export const BINDING_NAME = /^[a-z0-9][a-z0-9._-]{0,63}$/; // A read that fails names the file by label and the error code, never the // path Node puts in the message. The same error object is rethrown, so its diff --git a/packages/discord/src/journal.mjs b/packages/discord/src/journal.mjs index f0bce3e60..e0f5d97fa 100644 --- a/packages/discord/src/journal.mjs +++ b/packages/discord/src/journal.mjs @@ -26,8 +26,7 @@ import { unlinkSync, writeFileSync, writeSync, } from "node:fs"; import { basename, join } from "node:path"; -import { DiscordError, quietRead } from "./errors.mjs"; -import { BINDING_NAME } from "./binding.mjs"; +import { BINDING_NAME, DiscordError, quietRead } from "./errors.mjs"; // Where a refusal says a binding's journal is: its place under the data root // when the directory carries a valid binding name (bindingDataDir() makes it -- 2.54.0 From d9023e28db5317f3f7998a28f41135cd2b1b1d9b Mon Sep 17 00:00:00 2001 From: Jason Woltje Date: Wed, 11 Nov 2026 09:50:14 -0600 Subject: [PATCH 6/8] fix(discord): check-then-read gaps name the file by label, never the path (#1873) A token file, context file, tool root, data root or setspark key file that changes between its check and its read failed with a native error whose message carried the host path. Each read now names the file by label and code (quietRead): same error object, type and code, so exit codes, refusal codes and check order are unchanged. --- packages/discord/src/binding.mjs | 29 ++++++-- packages/discord/src/setspark.mjs | 11 ++- .../tests/path-redaction-labels.test.mjs | 72 ++++++++++++++++++- 3 files changed, 104 insertions(+), 8 deletions(-) diff --git a/packages/discord/src/binding.mjs b/packages/discord/src/binding.mjs index c2e492f72..359aa2534 100644 --- a/packages/discord/src/binding.mjs +++ b/packages/discord/src/binding.mjs @@ -313,7 +313,13 @@ export function loadBinding(path) { // clients. It is never printed, journaled, or put on a command line. export function readToken(binding) { checkPrivateFile(binding.tokenFile, "token file"); - const token = readFileSync(binding.tokenFile, "utf8").trim(); + // A file that changes after its check fails with a native error that carries the path (#1873). + let token; + try { + token = readFileSync(binding.tokenFile, "utf8").trim(); + } catch (e) { + throw quietRead(e, "token file"); + } if (!/^[A-Za-z0-9._-]{20,}$/.test(token)) throw new DiscordError("token file does not hold a bot token"); return token; } @@ -345,7 +351,12 @@ export function resolveContextFiles(binding, repo) { } if (st.isSymbolicLink()) throw new DiscordError(`context file must not be a symlink: ${at}`); if (!st.isFile() || st.size === 0) throw new DiscordError(`context file is not a regular non-empty file: ${at}`); - const real = realpathSync(path); + let real; + try { + real = realpathSync(path); + } catch (e) { + throw quietRead(e, `context file ${at}`); + } if (real !== path || !real.startsWith(root + sep)) throw new DiscordError(`context file resolves outside the repository: ${at}`); return path; }); @@ -379,7 +390,12 @@ function webConfig(raw, where) { export function resolveToolRoots(binding, { dataRoot }) { if (!binding.tools) return null; - const data = existsSync(dataRoot) ? realpathSync(dataRoot) : resolve(dataRoot); + let data; + try { + data = existsSync(dataRoot) ? realpathSync(dataRoot) : resolve(dataRoot); + } catch (e) { + throw quietRead(e, "the data root"); + } const roots = binding.tools.roots.map((r) => { let st; try { @@ -389,7 +405,12 @@ export function resolveToolRoots(binding, { dataRoot }) { } if (st.isSymbolicLink()) throw new DiscordError(`tool root ${r.name} must not be a symlink`); if (!st.isDirectory()) throw new DiscordError(`tool root ${r.name} is not a directory`); - const real = realpathSync(r.path); + let real; + try { + real = realpathSync(r.path); + } catch (e) { + throw quietRead(e, `tool root ${r.name}`); + } if (real === data || real.startsWith(data + sep) || data.startsWith(real + sep)) throw new DiscordError(`tool root ${r.name} overlaps the data root`); return { name: r.name, path: real, write: r.write, ...(r.git ? { git: { branch: r.git.branch, identity: r.git.identity, tokenFile: r.git.tokenFile, author: `${r.git.author.name} <${r.git.author.email}>`, ...(r.git.protocol ? { protocol: r.git.protocol } : {}) } } : {}) }; }); diff --git a/packages/discord/src/setspark.mjs b/packages/discord/src/setspark.mjs index 7978003aa..aceabe88c 100644 --- a/packages/discord/src/setspark.mjs +++ b/packages/discord/src/setspark.mjs @@ -28,7 +28,7 @@ import { lstatSync, readFileSync } from "node:fs"; import { request as httpsRequest } from "node:https"; import { request as httpRequest } from "node:http"; -import { plainKey } from "./errors.mjs"; +import { plainKey, quietRead } from "./errors.mjs"; export const SETSPARK_DEFAULTS = Object.freeze({ timeoutMs: 15000, maxResponseBytes: 262144 }); export const USER_AGENT = "mosaic-discord-sage/1 (Mosaic Stack Discord connector; setspark client)"; @@ -133,7 +133,14 @@ export function readKey(config) { throw new SetsparkRefusal(SETSPARK_REFUSAL.KEY_FILE); } if (st.size > KEY_MAX_BYTES) throw new SetsparkRefusal(SETSPARK_REFUSAL.KEY_SHAPE); - const text = readFileSync(config.keyFile, "utf8"); + // A file that changes after its checks fails with a native error that carries the path: it + // keeps its type and code and names the field (#1873). + let text; + try { + text = readFileSync(config.keyFile, "utf8"); + } catch (e) { + throw quietRead(e, "keyFile"); + } // Either one bare key line, or the mint's own JSON output // ({"key_id", "key", "note"}) stored as is; only "key" is used. let key = text; diff --git a/packages/discord/tests/path-redaction-labels.test.mjs b/packages/discord/tests/path-redaction-labels.test.mjs index 5c5615791..46a8a74e6 100644 --- a/packages/discord/tests/path-redaction-labels.test.mjs +++ b/packages/discord/tests/path-redaction-labels.test.mjs @@ -3,7 +3,8 @@ // quotes the sentinel too; each test also checks the scratch root and the label. import { test } from 'node:test'; import assert from 'node:assert/strict'; -import { mkdtempSync, mkdirSync, writeFileSync, rmSync } from 'node:fs'; +import fs, { mkdtempSync, mkdirSync, writeFileSync, rmSync } from 'node:fs'; +import { syncBuiltinESMExports } from 'node:module'; import { readApprovals } from '../src/approvals.mjs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; @@ -11,7 +12,7 @@ import { DiscordError } from '../src/errors.mjs'; import { bindingDataDir, bindingPath, loadDataRoot, readToken, resolveContextFiles, resolveToolRoots, validateBinding } from '../src/binding.mjs'; import { ensureJournal, lockPath, ownerPath, readInboxIds, recover, requestStop, turnPath, unlock, writeTurn } from '../src/journal.mjs'; import { gitPull, loadGitConfig } from '../src/git.mjs'; -import { loadSetsparkConfig } from '../src/setspark.mjs'; +import { loadSetsparkConfig, readKey } from '../src/setspark.mjs'; const secret = 'sk-test-NOT-A-SECRET-1234'; function scratch(t) { @@ -134,3 +135,70 @@ test('a git refusal drops stderr lines that carry a host path and a spawn failur const failed = () => ({ error: Object.assign(new Error(`spawn ${join(dir, 'git')} ENOENT`), { code: 'ENOENT' }) }); assert.throws(() => gitPull(rootCfg, { spawn: failed }), e => labelled('git could not start (ENOENT)', root, Error)(e)); }); + +// Re-check (#1873): a file removed between its check and its read surfaces a native +// error. Each site names the file by label and code; the error's path never reaches the +// message. `race` swaps one node:fs function so the target goes away right before the +// real call, which then fails with a genuine ENOENT that names the path. +function race(t, fn, target, remove) { + const original = fs[fn]; + const swapped = function (p, ...rest) { + if (String(p) === target) remove(); + return original.call(this, p, ...rest); + }; + if (original.native) swapped.native = original.native; + fs[fn] = swapped; + syncBuiltinESMExports(); + t.after(() => { + fs[fn] = original; + syncBuiltinESMExports(); + }); +} + +test('a token file removed between its check and its read names the token file', t => { + const { root, dir } = scratch(t); + const tokenFile = join(dir, 'token'); + writeFileSync(tokenFile, 'x'.repeat(30), { mode: 0o600 }); + race(t, 'readFileSync', tokenFile, () => rmSync(tokenFile)); + assert.throws(() => readToken({ tokenFile }), labelled('cannot read token file (ENOENT)', root, Error)); +}); + +test('a context file removed between its check and its realpath names its index', t => { + const { root, dir } = scratch(t); + const repo = join(dir, 'repo'); + mkdirSync(repo); + writeFileSync(join(repo, 'ok.md'), 'ok'); + writeFileSync(join(repo, `${secret}.md`), 'x'); + const target = join(repo, `${secret}.md`); + race(t, 'realpathSync', target, () => rmSync(target)); + assert.throws(() => resolveContextFiles({ context: { files: ['ok.md', `${secret}.md`] } }, repo), labelled('cannot read context file context.files[1] (ENOENT)', root, Error)); +}); + +test('a tool root removed between its check and its realpath names the root', t => { + const { root, dir } = scratch(t); + const docs = join(dir, 'docs'); + mkdirSync(docs); + const binding = { tools: { roots: [{ name: 'docs', path: docs, write: false }] } }; + race(t, 'realpathSync', docs, () => rmSync(docs, { recursive: true })); + assert.throws(() => resolveToolRoots(binding, { dataRoot: join(dir, 'data') }), labelled('cannot read tool root docs (ENOENT)', root, Error)); +}); + +test('a data root removed between its check and its realpath names the data root', t => { + const { root, dir } = scratch(t); + const docs = join(dir, 'docs'); + mkdirSync(docs); + const binding = { tools: { roots: [{ name: 'docs', path: docs, write: false }] } }; + const data = join(dir, 'data'); + mkdirSync(data); + race(t, 'realpathSync', data, () => rmSync(data, { recursive: true })); + assert.throws(() => resolveToolRoots(binding, { dataRoot: data }), labelled('cannot read the data root (ENOENT)', root, Error)); +}); + +test('a setspark key file removed between its check and its read names keyFile', t => { + const { root, dir } = scratch(t); + const keyFile = join(dir, 'key'); + writeFileSync(keyFile, 'ssk_' + 'a'.repeat(40), { mode: 0o600 }); + const config = loadSetsparkConfig({ baseUrl: 'https://setspark.example', keyFile, principal: 'sage' }); + race(t, 'readFileSync', keyFile, () => rmSync(keyFile)); + assert.throws(() => readKey(config), labelled('cannot read keyFile (ENOENT)', root, Error)); +}); -- 2.54.0 From 63e2a574affe3af8998b71ec62f13b3897a10936 Mon Sep 17 00:00:00 2001 From: Jason Woltje Date: Wed, 11 Nov 2026 10:11:11 -0600 Subject: [PATCH 7/8] fix(discord): a native fs error never prints its path: CLI choke point and journal labels (#1873) The discord CLI's top-level handler prints a native Node error that is not already a labelled refusal as 'cannot