From 87db40e0cda8b37e115d8313a328ace97d62392c Mon Sep 17 00:00:00 2001 From: Jason Woltje Date: Wed, 11 Nov 2026 02:20:11 -0600 Subject: [PATCH] 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) {