diff --git a/packages/business/src/system-config-problem.mjs b/packages/business/src/system-config-problem.mjs new file mode 100644 index 000000000..7f1324563 --- /dev/null +++ b/packages/business/src/system-config-problem.mjs @@ -0,0 +1,2 @@ +// Old path of packages/organization/src/system-config-problem.mjs (rename design r2, stage B). +export * from "../../organization/src/system-config-problem.mjs"; diff --git a/packages/cli/src/cli.mjs b/packages/cli/src/cli.mjs index 314ff713f..d8fbeda92 100644 --- a/packages/cli/src/cli.mjs +++ b/packages/cli/src/cli.mjs @@ -519,7 +519,8 @@ async function processingCommand(verb, rest, io, deps) { if (!result || typeof result !== 'object' || (typeof result.halted !== 'boolean' && !result.invalid)) throw new CliError('processing outcome unknown; use mosaic processing status before trying again', 1); if (verb === 'halt' && result.quiescent !== true) - throw new CliError(`halt gate is on for instance ${system.dataRoot}; ${result.remaining ?? 'unknown'} stops unconfirmed. Use mosaic processing status`, 1); + // #1873: the refusal names the instance, never its data root; a count that is not a count is unknown. + throw new CliError(`halt gate is on for this instance; ${Number.isSafeInteger(result.remaining) && result.remaining >= 0 ? result.remaining : 'unknown'} stops unconfirmed. Use mosaic processing status`, 1); if (parsed.flags.has('--json')) { io.stdout.write(JSON.stringify(result, null, 2) + '\n'); return; } const status = verb === 'resume' ? 'resumed' : result.halted ? 'halted' : result.invalid ? 'uninitialized' : 'running'; io.stdout.write(`processing ${status} for instance ${system.dataRoot}\n`); diff --git a/packages/cli/src/config.mjs b/packages/cli/src/config.mjs index 36fee7d83..e34d81a8c 100644 --- a/packages/cli/src/config.mjs +++ b/packages/cli/src/config.mjs @@ -13,6 +13,7 @@ import { BusinessError, accessFilePath, configDir, loadAccess, loadBusiness, loa import { loadRootAgents } from "../../organization/src/agent-levels.mjs"; import { materializeCopies, withCopies } from "../../organization/src/agent-copies.mjs"; import { organizationWriter } from "../../organization/src/organization.mjs"; +import { systemConfigProblem } from "../../organization/src/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"; @@ -41,13 +42,40 @@ export function orgPaths(root) { threads: join(root, "threads"), hostDir }; } +// #1873: mosaic-config's stderr can quote the config's path, the data root or a config value, so +// it is never relayed. An instance refusal keeps its code and the instance names, which passed +// the instance pattern; a missing config keeps its bootstrap hint; anything else reads as +// `mosaic org config` reads it: a known field and its constraint, the config named by label. +const INSTANCE = "[a-z][a-z0-9]{1,15}"; +function instanceProblem(problem, label) { + const body = problem.replace(/^mosaic-config: /, ""); + let m = new RegExp(`^instance-root-mismatch: .+ is claimed by instance (${INSTANCE})(?:, not (${INSTANCE})|; select it with --instance \\1)$`).exec(body); + if (m) return `instance-root-mismatch: the data root is claimed by instance ${m[1]}${m[2] ? `, not ${m[2]}` : `; select it with --instance ${m[1]}`}`; + m = new RegExp(`^instance-root-unclaimed: .+ holds no instance\\.json naming (${INSTANCE}) \\(scripts/bootstrap\\.sh --instance \\1 claims a new root\\)$`).exec(body); + if (m) return `instance-root-unclaimed: the data root holds no instance.json naming ${m[1]} (scripts/bootstrap.sh --instance ${m[1]} claims a new root)`; + if (body.startsWith("instance-root-unclaimed: ")) return "instance-root-unclaimed: the data root's instance.json is not a readable, valid marker"; + m = new RegExp(`^instance-config-mismatch: --instance (${INSTANCE}) names `).exec(body); + if (m) return `instance-config-mismatch: --instance ${m[1]} names its own config, but MOSAIC_CONFIG names another`; + if (body.startsWith("instance-config-mismatch: ")) return "instance-config-mismatch: --instance names its own config, but MOSAIC_CONFIG names another"; + if (body.startsWith("instance-name-invalid: ")) return `instance-name-invalid: an instance name must match /^${INSTANCE}$/`; + if (body.startsWith("configuration not found: ")) { + m = new RegExp(`\\(run (scripts/bootstrap\\.sh(?: --instance ${INSTANCE})?) to create it\\)$`).exec(body); + return `configuration not found: ${label}${m ? ` (run ${m[1]} to create it)` : ""}`; + } + return null; +} + export function loadSystem({ env = process.env, instance = null } = {}) { const proc = spawnSync(process.execPath, [join(REPO, "scripts", "mosaic-config.mjs"), "validate", ...(instance === null ? [] : ["--instance", instance])], { encoding: "utf8", maxBuffer: 1024 * 1024, env, }); - if (proc.status !== 0) throw new CliError(`system config problem (mosaic-config exit ${proc.status}): ${proc.stderr.trim()}`, 3); + if (proc.status !== 0) { + const label = instance !== null && instance !== "dev" ? `the config of instance ${instance}` : env.MOSAIC_CONFIG ? "the system config MOSAIC_CONFIG names" : "the default system config"; + const known = instanceProblem(String(proc.stderr ?? "").trim(), label); + throw new CliError(known ? `system config problem (mosaic-config exit ${proc.status}): ${known}` : systemConfigProblem(proc, env, label), 3); + } try { return JSON.parse(proc.stdout); } catch { @@ -90,7 +118,7 @@ function trackerFor(system, b, warn, dataRoot) { if (dataRoot && managedState({ dataRoot, business: b.id, project: id, entry }) === 'reserved') continue; if (!existsSync(projectFilePath(entry.root))) continue; const project = loadProject(entry.root); - if (project.id !== id) throw new CliError(`project file ${project.file} has id that does not match the declared project key`, 3); + if (project.id !== id) throw new CliError(`project file of project ${id} has id that does not match the declared project key`, 3); const vars = resolveInstance({ system, business: b, project, instance: first }).vars; if (vars["tracker.project"] !== undefined) named.push({ id, vars }); } @@ -127,17 +155,20 @@ function accessFor(system, env, warn) { try { context = accessContextFor(system, env); } catch (e) { - warn(`access.json is present but its context can't be read, so no granted user binds: ${e.message}`); + // #1873: the error's message can quote a path; only a plain code is relayed. + const code = typeof e?.code === "string" && /^[A-Za-z][A-Za-z0-9_-]{0,63}$/.test(e.code) ? ` (${e.code})` : ""; + warn(`access.json is present but its context can't be read, so no granted user binds${code}`); return null; } // sage-3153: one line at boot when the loader refuses the file, so the owner sees why no user - // binds. The loader's messages can quote the file, so the line names only the path and a code - // from its exit code: 4 is a file it could not open as a regular file, anything else invalid. + // binds. The loader's messages can quote the file, so the line names only the file, by label, and + // a code from its exit code: 4 is a file it could not open as a regular file, anything else + // invalid. #1873: no host path. try { loadAccess(file, context); } catch (e) { const code = e instanceof BusinessError && e.exitCode !== 4 ? "access-invalid" : "access-unreadable"; - warn(`access file refused at boot (${code}), no granted user binds until it is fixed: ${file}`); + warn(`access file refused at boot (${code}), no granted user binds until it is fixed: access.json in the config directory`); } return { file, context }; } diff --git a/packages/cli/src/launcher.mjs b/packages/cli/src/launcher.mjs index 49299f2c5..8ba047a9f 100644 --- a/packages/cli/src/launcher.mjs +++ b/packages/cli/src/launcher.mjs @@ -145,6 +145,7 @@ import { chmodSync, existsSync, lstatSync, mkdirSync, readFileSync, renameSync, import { dirname, join, relative } from "node:path"; import { configDir, resolveInstance, systemVars } from "../../organization/src/index.mjs"; import { organizationWriter } from "../../organization/src/organization.mjs"; +import { quietMessage } from "../../organization/src/org-registry.mjs"; import { businessFilePath } from "../../organization/src/organization-config.mjs"; import { copyOf, copyRefusal, loadWithCopies, projectRefusal, withCopies } from "../../organization/src/agent-copies.mjs"; import { selectModelProvider } from "../../plugins/src/model-provider-selection.mjs"; @@ -201,13 +202,24 @@ 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 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}: ${e.message}`); + 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}`); } } @@ -272,7 +284,8 @@ export function createLauncher({ try { leftover = readSessions(dataRoot).filter((s) => s.business === b.id); } catch (e) { - throw new CliError(`${e.message}; check which sessions still run, then move the file aside`, 3); + // readSessions names the file by its place under the data root (#1873); anything else is unreadable. + throw new CliError(`${e?.name === "SeatError" ? e.message : "sessions file is unreadable"}; check which sessions still run, then move the file aside`, 3); } const registry = new Registry(dataRoot, { readOnly: !launches }); // Row 269: earlier-host runs still live after recover(). check() sees each while it lives. @@ -386,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. @@ -409,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"); @@ -425,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. @@ -437,7 +450,9 @@ export function createLauncher({ const harnessOf = async (m) => { const loader = makeLoader({ dataRoot:system.instanceRoot ?? dataRoot, log: (entry) => record({ ...entry, instance }) }); const { refused } = await loader.load({ harness: [m.harness] }); - if (refused.length) throw new Refusal("harness-refused", `harness ${m.harness}: ${refused.map((r) => `${r.code}: ${r.message}`).join("; ")}`); + // #1873: a loader message can quote the module's path or its load error; only a + // missing-export one, which names the export, is relayed. Any other refusal names its code. + if (refused.length) throw new Refusal("harness-refused", `harness ${m.harness}: ${refused.map((r) => r.code === "missing-export" ? `${r.code}: ${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}`); @@ -484,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, @@ -532,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 @@ -542,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]; } @@ -664,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) { @@ -861,7 +877,7 @@ export function createLauncher({ unregisterRuntime(dataRoot, managed.id); if (e instanceof Refusal) throw e; if (e.code?.startsWith('proxy-')) throw new Refusal(e.code, e.code); - throw new Refusal("launch-failed", e.message); + throw new Refusal("launch-failed", reason(e)); } children.set(run, exited); const outputWatch = admittedProxy ? watchModelOutput({ runDir, armAtFirstTurn: true, keyId: modelKeyId, seconds: system.proxy.firstOutputSeconds ?? 120, @@ -1041,7 +1057,7 @@ export function createLauncher({ if (refusedThreads.get(key) === code) return; refusedThreads.set(key, code); log(`thread ${t.thread} session ${t.session} (${t.instance}): start refused: ${code}`); - record({ event: "refused", by: b.human, instance: t.instance, thread: t.thread, session: t.session, code, why: e.message, ...(e.limit?{limit:e.limit}:{}) }); + record({ event: "refused", by: b.human, instance: t.instance, thread: t.thread, session: t.session, code, why: reason(e), ...(e.limit?{limit:e.limit}:{}) }); } }); } @@ -1143,7 +1159,7 @@ export function createLauncher({ if (clearLogged.get(c.op) === code) return; clearLogged.set(c.op, code); log(`clear ${c.op}: successor ${c.successor} (${c.instance}): start ${CLEAR_HELD.has(code) ? "held" : "refused"}: ${code}`); - record({ event: "refused", by: b.human, instance: c.instance, thread: c.thread, session: c.successor, code, why: e.message, ...(e.limit ? { limit: e.limit } : {}) }); + record({ event: "refused", by: b.human, instance: c.instance, thread: c.thread, session: c.successor, code, why: reason(e), ...(e.limit ? { limit: e.limit } : {}) }); await host.op({ op: "sessionClearStart", record: { ...where, start: CLEAR_HELD.has(code) ? "held" : "refused", refusal: code } }); return; } @@ -1203,7 +1219,7 @@ export function createLauncher({ if (refusedLand.get(h.decision) === code) return; refusedLand.set(h.decision, code); log(`decision ${h.decision} (${h.instance}): landing start refused: ${code}`); - record({ event: "refused", by: b.human, instance: h.instance, decision: h.decision, code, why: e.message, ...(e.limit ? { limit: e.limit } : {}) }); + record({ event: "refused", by: b.human, instance: h.instance, decision: h.decision, code, why: reason(e), ...(e.limit ? { limit: e.limit } : {}) }); } }); } @@ -1248,7 +1264,7 @@ export function createLauncher({ try { done = await act(take); } catch (e) { - done = { refusal: e instanceof Refusal ? e.code : "launch-failed", why: e.message, limit: e.limit }; + done = { refusal: e instanceof Refusal ? e.code : "launch-failed", why: reason(e), limit: e.limit }; } const { why, limit, ...closing } = done; if (closing.refusal) record({ event: "refused", by: r.by, instance: r.instance, request: r.request, code: closing.refusal, ...(why ? { why } : {}), ...(limit ? { limit } : {}) }); @@ -1356,18 +1372,19 @@ export function createLauncher({ const bot = out.bot ? { bot: out.bot } : {}; // The old instance workspace, then (row 269) the instance's session workspaces, each moved // whole. A retry moves whichever is still in place. - for (const from of [workspaceDir(dataRoot, b.id, r.instance), join(dataRoot, "session-workspaces", b.id, r.instance)]) { + for (const [from, label] of [[workspaceDir(dataRoot, b.id, r.instance), "the instance workspace"], [join(dataRoot, "session-workspaces", b.id, r.instance), "the session workspaces"]]) { if (!existsSync(from)) continue; const to = join(dirname(from), ".retired", `${r.instance}-${transaction}`); try { mkdirSync(dirname(to), { recursive: true, mode: 0o700 }); chmodSync(dirname(to), 0o700); - if (existsSync(to)) throw new Error(`${to} exists`); + if (existsSync(to)) throw new Error("its .retired name already exists"); renameSync(from, to); chmodSync(to, 0o700); } catch (e) { log(`request ${r.request} (${r.instance}): workspace rename failed: ${e.message}; left at ${from}`); - return { transaction, ...bot, refusal: "workspace-rename-failed", why: `${e.message}; left at ${from}` }; + // #1873: the reason names the workspace by label and the error by code, never a path. + return { transaction, ...bot, refusal: "workspace-rename-failed", why: `${quietMessage(e, label) ?? `${label}: ${relay(e.message)}`}; left in place` }; } record({ event: "workspace-retired", instance: r.instance, request: r.request, transaction, path: to }); } @@ -1438,7 +1455,7 @@ export function createLauncher({ } catch (e) { if (!(e instanceof Refusal)) throw e; await host.op({ op: "refuse", cap, code: e.code }).catch(() => {}); - record({ event: "refused", by: who.role, instance, code: e.code, why: e.message, ...(e.limit?{limit:e.limit}:{}) }); + record({ event: "refused", by: who.role, instance, code: e.code, why: reason(e), ...(e.limit?{limit:e.limit}:{}) }); return reply({ ok: false, error: e.code }); } try { @@ -1453,7 +1470,7 @@ export function createLauncher({ const code = e instanceof Refusal ? e.code : "launch-failed"; log(`launch of ${instance} for ${who.role} failed: ${e.message}`); await host.op({ op: "refuse", cap, code }).catch(() => {}); - record({ event: "refused", by: who.role, instance, code, why: e.message, ...(e.limit?{limit:e.limit}:{}) }); + record({ event: "refused", by: who.role, instance, code, why: reason(e), ...(e.limit?{limit:e.limit}:{}) }); return reply({ ok: false, error: code }); } }); @@ -1467,8 +1484,8 @@ export function createLauncher({ return await start({ instance: b.launch.by, launchedBy: b.human, ...(await check(b.launch.by, { pm: true })) }); } catch (e) { if (!(e instanceof Refusal)) throw e; - record({ event: "refused", by: b.human, instance: b.launch.by, code: e.code, why: e.message, ...(e.limit?{limit:e.limit}:{}) }); - const error = new CliError(`PM launch refused: ${e.code}${e.message !== e.code ? ` (${e.message})` : ""}`, 3); + record({ event: "refused", by: b.human, instance: b.launch.by, code: e.code, why: reason(e), ...(e.limit?{limit:e.limit}:{}) }); + const error = new CliError(`PM launch refused: ${e.code}${reason(e) !== e.code ? ` (${reason(e)})` : ""}`, 3); error.code = e.code; throw error; } @@ -1519,7 +1536,7 @@ export function createLauncher({ await recover(); const path = launchSocketPath(dataRoot); if (existsSync(path)) { - if (!lstatSync(path).isSocket()) throw new CliError(`${path} exists and isn't a socket`, 3); + if (!lstatSync(path).isSocket()) throw new CliError("launch socket path exists and isn't a socket", 3); rmSync(path); } server = createServer((socket) => { diff --git a/packages/cli/tests/access-boot.test.mjs b/packages/cli/tests/access-boot.test.mjs index c9337d6a4..d573d0931 100644 --- a/packages/cli/tests/access-boot.test.mjs +++ b/packages/cli/tests/access-boot.test.mjs @@ -91,7 +91,9 @@ test("R6 B3 a boot with a file R1's loader refuses still boots, binds no user an const s = await boot(t, file); assert.equal(s.config.access.file, s.path, why); assert.equal(s.bind("alice"), "access-revoked", why); - assert.deepEqual(s.warnings, [TRACKER, `access file refused at boot (${code}), no granted user binds until it is fixed: ${s.path}`], why); + // #1873: the warning names access.json by its place, never by host path. + assert.deepEqual(s.warnings, [TRACKER, `access file refused at boot (${code}), no granted user binds until it is fixed: access.json in the config directory`], why); + assert.ok(!s.warnings.join("\n").includes(s.path), why); assert.doesNotMatch(s.warnings.join("\n"), /secret-marker/, why); } }); diff --git a/packages/cli/tests/json-redaction.test.mjs b/packages/cli/tests/json-redaction.test.mjs index bbcffd626..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,10 +37,12 @@ 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); - assert.ok(error.message.includes(f.file)); + // #1873: the cli relay names the instance's config by label, never its path. + assert.ok(!error.message.includes(f.file), error.message); + assert.ok(error.message.includes('configuration is not valid JSON; expected readable JSON: the config of instance redacttest'), error.message); assert.match(error.message, /not valid JSON/); assert.doesNotMatch(error.message, /sk-test|Unexpected token/); return true; @@ -52,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/last-values.test.mjs b/packages/cli/tests/last-values.test.mjs index f7e823f3c..e529b80c2 100644 --- a/packages/cli/tests/last-values.test.mjs +++ b/packages/cli/tests/last-values.test.mjs @@ -89,7 +89,12 @@ test('CLI boot project id mismatch omits the configured value and keeps exit 3', const root = scratch(t), f = fixture(root), file = join(f.doc.projects.stack.root, '.mosaic', 'project.json'); mkdirSync(join(f.doc.projects.stack.root, '.mosaic')); writeFileSync(file, JSON.stringify({ projectVersion: 1, id: secret.toLowerCase() })); - assert.throws(() => bootConfig({ system: loadSystem({ env: f.env }), businessId: 'acme', env: f.env }), safe(file, CliError, { exitCode: 3 }, secret.toLowerCase())); + assert.throws(() => bootConfig({ system: loadSystem({ env: f.env }), businessId: 'acme', env: f.env }), error => { + // #1873: the refusal names the project by its key, never the project file's path. + safe('project file of project stack has id that does not match the declared project key', CliError, { exitCode: 3 }, secret.toLowerCase())(error); + assert.ok(!error.message.includes(file) && !error.message.includes(f.doc.projects.stack.root), error.message); + return true; + }); }); for (const mission of [false, true]) test(`duplicate ${mission ? 'mission' : 'task'} tool rejects an unknown value before naming it`, t => { diff --git a/packages/cli/tests/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/lifecycle.test.mjs b/packages/cli/tests/lifecycle.test.mjs index 7548107d6..bf417882f 100644 --- a/packages/cli/tests/lifecycle.test.mjs +++ b/packages/cli/tests/lifecycle.test.mjs @@ -404,6 +404,10 @@ test("row 194 V5: delete retires the name, renames the workspace to .retired/I-< const del = s.call("instances.set", { revision: revision(), instance: "sec-1", op: "delete" }).request; const failed = await s.carry(del); assert.equal(failed.done.refusal, "workspace-rename-failed"); + // #1873: the refused event's reason names the workspace by label and the error by code, never a path. + const refused = launchLog(p.f).filter((e) => e.event === "refused" && e.code === "workspace-rename-failed"); + assert.deepEqual(refused.map((e) => e.why), ["cannot mkdir the instance workspace (EEXIST); left in place"]); + assert.ok(!JSON.stringify(refused).includes(p.f.dataRoot)); const transaction = failed.done.transaction; assert.ok(transaction); const file = JSON.parse(readFileSync(join(p.root, "config", "businesses", "acme.json"), "utf8")); diff --git a/packages/cli/tests/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 new file mode 100644 index 000000000..16a528381 --- /dev/null +++ b/packages/cli/tests/path-redaction-labels.test.mjs @@ -0,0 +1,242 @@ +// #1873 (redact6): a cli refusal names the file, the instance or the field, never a host path, +// and never relays mosaic-config's stderr or a Node system error's message. The sentinel is a +// component of every scratch path, so a refusal that quotes a path quotes the sentinel too; each +// test also checks the scratch root and the label. The launcher cases need no bus: a bus socket +// under a sentinel path would pass the Unix socket length limit, so they use a stub host. +import test from 'node:test'; +import assert from 'node:assert/strict'; +import { mkdirSync, readFileSync, symlinkSync, writeFileSync } from 'node:fs'; +import { spawnSync } from 'node:child_process'; +import { join } from 'node:path'; +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'; +import { initializeControl } from '../../processing-control/src/state.mjs'; +import { launchLogFile } from '../../seat/src/session.mjs'; +import { fakeHarnesses, fixture, io, tmp, withoutHostCredentials } from './helpers.mjs'; + +const secret = 'sk-test-NOT-A-SECRET-1234'; +function scratch(t) { + const root = tmp(t, 'r6-'); + const dir = join(root, secret); + mkdirSync(dir, { mode: 0o700 }); + return { root, dir }; +} +function clean(text, root) { + assert.ok(!text.includes(root), text); + assert.doesNotMatch(text, /sk-test|NOT-A-SECRET/); +} +const PI = JSON.stringify(new URL('../../plugins/harness/pi/index.mjs', import.meta.url).href); + +function stubLauncher(t, { op = async () => ({}) } = {}) { + const { root, dir } = scratch(t); + const f = fixture(dir, 'acme'); + initializeControl(f.dataRoot); + withoutHostCredentials(f.env, { piKey: true }); + const system = JSON.parse(readFileSync(f.config, 'utf8')); + const host = { business: 'acme', beforeClose: () => {}, op }; + return { root, f, make: () => createLauncher({ host, system, env: f.env, bwrap: '/usr/bin/true', log: () => {} }) }; +} + +test('an unreadable sessions file refuses exit 3 naming bus-host/sessions.json under the data root, never its path', t => { + const s = stubLauncher(t); + mkdirSync(join(s.f.dataRoot, 'bus-host'), { recursive: true, mode: 0o700 }); + writeFileSync(join(s.f.dataRoot, 'bus-host', 'sessions.json'), '{not json'); + assert.throws(() => s.make(), e => { + assert.ok(e instanceof CliError, e.stack); + assert.equal(e.exitCode, 3); + assert.equal(e.message, 'sessions file is unreadable: bus-host/sessions.json under the data root; check which sessions still run, then move the file aside'); + clean(e.message, s.root); + return true; + }); + writeFileSync(join(s.f.dataRoot, 'bus-host', 'sessions.json'), JSON.stringify({ sessionsVersion: 999, sessions: [] })); + assert.throws(() => s.make(), e => e.exitCode === 3 && e.message.startsWith('sessions file is malformed: bus-host/sessions.json under the data root;') && !e.message.includes(s.root)); +}); + +test('a launch socket path that is not a socket refuses exit 3 by label, never its path', async t => { + const s = stubLauncher(t); + mkdirSync(join(s.f.dataRoot, 'bus-host'), { recursive: true, mode: 0o700 }); + writeFileSync(join(s.f.dataRoot, 'bus-host', 'launch.sock'), 'in the way\n'); + const launcher = s.make(); + t.after(() => launcher.close?.()); + await assert.rejects(launcher.listen(), e => { + assert.ok(e instanceof CliError, e.stack); + assert.equal(e.exitCode, 3); + assert.equal(e.message, "launch socket path exists and isn't a socket"); + clean(e.message, s.root); + return true; + }); +}); + +// The launch-routes pattern: a stub host stops each launch at authorizeLaunch, and the launch +// log's refused event carries the reason. +async function routed(t, harness) { + const { root, dir } = scratch(t); + const f = fixture(dir, 'acme', doc => { + doc.roles.coder.vars = { harness, model: 'claude-sonnet-5-5', thinking: 'high' }; + doc.plugins = { ...doc.plugins, harness: ['pi', 'claude-code', harness] }; + return doc; + }); + initializeControl(f.dataRoot); + withoutHostCredentials(f.env, { piKey: true }); + const host = { business: 'acme', beforeClose: () => {}, op: async x => { + if (x.op === 'identity') return { business: 'acme', role: 'pm' }; + if (x.op === 'mayLaunch') return { allow: true }; + if (x.op === 'authorizeLaunch') throw Object.assign(Error('fixture authorization stop'), { code: 'fixture-policy' }); + return {}; + } }; + const absent = join(dir, 'absent.json'); + const makeLoader = await fakeHarnesses(t, { pi: null, 'claude-code': null }, write => { + // fsthrows: its preflight reads a missing file, so the error's message quotes the path. + write('harness', 'fsthrows', `export * from ${PI};\nimport { readFileSync } from "node:fs";\nexport function preflight() { readFileSync(${JSON.stringify(absent)}); }\n`); + // 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, 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' }); + assert.equal(s.last().why, 'harness fsthrows preflight: cannot open a host file (ENOENT)'); + clean(JSON.stringify(s.last()), s.root); +}); + +test('a harness the loader refuses names its code only, never the loader message or a path', async t => { + const s = await routed(t, 'absent'); + assert.deepEqual(await s.launch(), { ok: false, error: 'harness-refused' }); + assert.match(s.last().why, /^harness absent: [a-z][a-z0-9-]*$/); + clean(JSON.stringify(s.last()), s.root); +}); + +test('the halt gate refusal names the instance, never its data root, and keeps exit 1 and the count', async t => { + const { root, dir } = scratch(t); + const result = { version: 1, generation: '11111111-1111-4111-8111-111111111111', operation: '11111111-1111-4111-8111-111111111111', business: 'demo', human: 'jason', at: '2026-10-20T00:00:00.000Z', recovered: false, halted: true, quiescent: false, remaining: 2, stopped: 0 }; + const x = io(); + await assert.rejects(main(['halt', '--business', 'demo'], x, { system: { dataRoot: join(dir, 'data') }, transport: () => async () => result }), e => { + assert.equal(e.exitCode, 1); + assert.equal(e.message, 'halt gate is on for this instance; 2 stops unconfirmed. Use mosaic processing status'); + clean(e.message, root); + return true; + }); + const odd = io(); + await assert.rejects(main(['halt', '--business', 'demo'], odd, { system: { dataRoot: join(dir, 'data') }, transport: () => async () => ({ ...result, remaining: join(dir, 'x') }) }), e => { + assert.equal(e.message, 'halt gate is on for this instance; unknown stops unconfirmed. Use mosaic processing status'); + clean(e.message, root); + return true; + }); +}); + +test('a system config problem names the config by label and never relays mosaic-config stderr', t => { + const { root, dir } = scratch(t); + // Missing: the label for where it came from, with the bootstrap hint. + assert.throws(() => loadSystem({ env: { PATH: process.env.PATH, HOME: dir, MOSAIC_CONFIG: join(dir, 'absent.json') } }), e => { + assert.ok(e instanceof CliError, e.stack); + assert.equal(e.exitCode, 3); + assert.equal(e.message, 'system config problem (mosaic-config exit 3): configuration not found: the system config MOSAIC_CONFIG names (run scripts/bootstrap.sh to create it)'); + clean(e.message, root); + return true; + }); + assert.throws(() => loadSystem({ env: { PATH: process.env.PATH, HOME: dir } }), e => { + assert.match(e.message, /^system config problem \(mosaic-config exit 3\): configuration not found: the default system config \(run scripts\/bootstrap\.sh to create it\)$/); + clean(e.message, root); + return true; + }); + // A field whose value quotes the sentinel: the field and its constraint, never the value. + const f = fixture(dir, 'acme'); + const doc = JSON.parse(readFileSync(f.config, 'utf8')); + writeFileSync(f.config, JSON.stringify({ ...doc, dataRoot: `relative/${secret}` })); + assert.throws(() => loadSystem({ env: f.env }), e => { + assert.equal(e.exitCode, 3); + assert.match(e.message, /^system config problem \(mosaic-config exit \d+\): dataRoot: expected a canonical absolute instance data path: the system config MOSAIC_CONFIG names$/); + clean(e.message, root); + return true; + }); +}); + +// 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 ff028b5cd..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); } } }); @@ -270,7 +274,9 @@ test("T-I2 selection: the four rows of D1 through the instance op and through an await t.test("a named instance with no config exits 3 with its own bootstrap hint", () => { const r = config(home, {}, "instance", "--instance", "gamma"); assert.equal(r.code, 3); - assert.match(r.err, /configuration not found: .*mosaic-gamma\/config\.json \(run scripts\/bootstrap\.sh --instance gamma to create it\)/); + // #1873: the refusal names the config by its instance, never by host path. + assert.match(r.err, /configuration not found: the config of instance gamma \(run scripts\/bootstrap\.sh --instance gamma to create it\)/); + assert.ok(!r.err.includes(cfgOf(home, "gamma")) && !r.err.includes(home), r.err); }); await t.test("an entry point takes --instance in any position, once, and refuses a mismatch before it prints", () => { @@ -326,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); @@ -336,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 9854812b2..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'; @@ -32,17 +32,18 @@ export function defaultConfigPath() { } // Fail closed: the config must exist, parse, and name an absolute dataRoot. +// Refusals name the config, never its path (#1873). export function loadConfig(path = defaultConfigPath()) { - if (!existsSync(path)) throw new ConfigError(`config not found: ${path}`); + if (!existsSync(path)) throw new ConfigError("config not found"); let raw; try { raw = parseJsonFileText(readFileSync(path, "utf8"), path); } catch (error) { - throw new ConfigError(`config is not valid JSON: ${path}${error.code ? ` (${error.code})` : ""}`); + throw new ConfigError(`config is not valid JSON${error.code ? ` (${error.code})` : ""}`); } - if (!raw || typeof raw !== "object" || Array.isArray(raw)) throw new ConfigError(`config is not an object: ${path}`); + if (!raw || typeof raw !== "object" || Array.isArray(raw)) throw new ConfigError("config is not an object"); if (typeof raw.dataRoot !== "string" || !isAbsolute(raw.dataRoot)) { - throw new ConfigError(`config.dataRoot must be an absolute path: ${path}`); + throw new ConfigError("config.dataRoot must be an absolute path"); } return { dataRoot: raw.dataRoot }; } @@ -230,11 +231,11 @@ export function loadSeen(boardDir) { try { parsed = parseJsonFileText(readFileSync(path, "utf8"), path); } catch (error) { - throw new ConfigError(`seen marks file is not valid JSON: ${path}${error.code ? ` (${error.code})` : ""}`); + throw new ConfigError(`seen marks file seen.json is not valid JSON${error.code ? ` (${error.code})` : ""}`); } - if (!parsed || typeof parsed !== "object" || Array.isArray(parsed)) throw new ConfigError(`seen marks file must be a JSON object: ${path}`); + if (!parsed || typeof parsed !== "object" || Array.isArray(parsed)) throw new ConfigError("seen marks file seen.json must be a JSON object"); for (const [k, v] of Object.entries(parsed)) { - if (typeof v !== "string") throw new ConfigError(`seen marks file has a non-string value for ${JSON.stringify(k)}: ${path}`); + if (typeof v !== "string") throw new ConfigError(`seen marks file seen.json has a non-string value for ${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 new file mode 100644 index 000000000..29167ba50 --- /dev/null +++ b/packages/control-board/tests/path-redaction-labels.test.mjs @@ -0,0 +1,98 @@ +// #1873 (redact6): a control-board refusal names the config, the seen marks file or the seat +// by label, never a host path. The sentinel is a component of every scratch path, so a refusal +// that quotes a path quotes the sentinel too; each test also checks the scratch root and the label. +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { dirname, join } from 'node:path'; +import { ConfigError, loadConfig, loadRegistrations, loadSeen } from '../src/scan.mjs'; +import { readRegistration, registrationPath, SeatError } from '../../seat/src/seat.mjs'; +import { startServer } from '../src/serve.mjs'; + +const secret = 'sk-test-NOT-A-SECRET-1234'; +function scratch(t) { + const root = mkdtempSync(join(tmpdir(), 'r6-')); + t.after(() => rmSync(root, { recursive: true, force: true })); + const dir = join(root, secret); + mkdirSync(dir); + return { root, dir }; +} +function clean(text, root) { + assert.ok(!text.includes(root), text); + assert.doesNotMatch(text, /sk-test|NOT-A-SECRET/); +} +function labelled(label, root) { + return error => { + assert.ok(error instanceof ConfigError, error.stack); + assert.equal(error.message, label); + clean(error.message, root); + return true; + }; +} +function write(file, text) { + mkdirSync(dirname(file), { recursive: true }); + writeFileSync(file, text); + return file; +} + +test('config refusals name the config, not its path', t => { + const { root, dir } = scratch(t); + assert.throws(() => loadConfig(join(dir, 'config.json')), labelled('config not found', root)); + assert.throws(() => loadConfig(write(join(dir, 'array.json'), '[]')), labelled('config is not an object', root)); + assert.throws(() => loadConfig(write(join(dir, 'relative.json'), '{"dataRoot":"data"}')), labelled('config.dataRoot must be an absolute path', root)); +}); + +test('seen marks refusals name seen.json, not its path', t => { + const { root, dir } = scratch(t); + write(join(dir, 'seen.json'), '[]'); + assert.throws(() => loadSeen(dir), labelled('seen marks file seen.json must be a JSON object', root)); + write(join(dir, 'seen.json'), '{"a":1}'); + assert.throws(() => loadSeen(dir), labelled('seen marks file seen.json has a non-string value for "a"', root)); +}); + +test('a bad registration is reported by layout and seat, never by its path', t => { + const { root, dir } = scratch(t); + write(registrationPath(dir, 'builder', 'repo'), '{}'); + const { registrations, errors } = loadRegistrations(dir); + assert.deepEqual(registrations, []); + assert.equal(errors.length, 1); + assert.match(errors[0], /^seat repo\/builder: .*: registration repo\/builder$/); + clean(errors[0], root); +}); + +// 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)); +}); + +// The board API returns a scan refusal in its JSON body, so the HTTP body carries the label too. +test('a seen marks refusal reaches the board API body by label, never by path', async t => { + const { root, dir } = scratch(t); + write(join(dir, 'seen.json'), '[]'); + const server = await startServer({ host: '127.0.0.1', port: 0, specs: [], boardDir: dir, isAlive: () => true, page: '' }); + t.after(() => new Promise(done => server.close(done))); + const res = await fetch(`http://127.0.0.1:${server.address().port}/api/board`); + const body = await res.text(); + assert.equal(res.status, 500); + assert.deepEqual(JSON.parse(body), { error: 'seen marks file seen.json must be a JSON object' }); + clean(body, root); +}); diff --git a/packages/control-board/tests/refusal-redaction.test.mjs b/packages/control-board/tests/refusal-redaction.test.mjs index 171af624b..dbbcc5f59 100644 --- a/packages/control-board/tests/refusal-redaction.test.mjs +++ b/packages/control-board/tests/refusal-redaction.test.mjs @@ -6,10 +6,13 @@ import { dirname, join } from 'node:path'; const secret = 'sk-test-NOT-A-SECRET-1234'; // JSON.parse quotes a prefix near this fault on Node 26; check partial leaks too. const malformed = secret; +// #1873: the sentinel is a path component, so a refusal that quotes a path quotes it too. function scratch(t) { const root = mkdtempSync(join(tmpdir(), 'r3-')); t.after(() => rmSync(root, { recursive: true, force: true })); - return root; + const dir = join(root, secret); + mkdirSync(dir); + return dir; } function write(file, text = malformed) { mkdirSync(dirname(file), { recursive: true }); @@ -21,6 +24,7 @@ function safe(label, ErrorType, properties = {}) { assert.ok(error instanceof ErrorType); for (const [key, value] of Object.entries(properties)) assert.equal(error[key], value); assert.ok(error.message.includes(label), error.message); + assert.ok(!error.message.includes(tmpdir()), error.message); assert.doesNotMatch(error.message, /sk-test|Unexpected token/); assert.doesNotMatch(error.stack, /sk-test/); assert.equal(error.cause, undefined); @@ -30,13 +34,13 @@ function safe(label, ErrorType, properties = {}) { import { ConfigError, loadConfig, loadSeen } from '../src/scan.mjs'; for (const kind of ['config', 'seen']) test(`${kind} JSON refusal names only the file`, t => { const root = scratch(t), file = write(join(root, kind === 'config' ? 'config.json' : 'seen.json')); - assert.throws(() => kind === 'config' ? loadConfig(file) : loadSeen(root), safe(file, ConfigError)); + assert.throws(() => kind === 'config' ? loadConfig(file) : loadSeen(root), safe(kind === 'config' ? 'config is not valid JSON' : 'seen marks file seen.json is not valid JSON', ConfigError)); }); for (const kind of ['config', 'seen']) test(`${kind} read refusal retains the filesystem code`, t => { const root = scratch(t), file = join(root, kind === 'config' ? 'config.json' : 'seen.json'); mkdirSync(file); assert.throws(() => kind === 'config' ? loadConfig(file) : loadSeen(root), error => { - safe(file, ConfigError)(error); + safe(kind === 'config' ? 'config is not valid JSON' : 'seen marks file seen.json is not valid JSON', ConfigError)(error); assert.match(error.message, /\(EISDIR\)$/); assert.doesNotMatch(error.message, /illegal operation/); return true; diff --git a/packages/discord/src/approvals.mjs b/packages/discord/src/approvals.mjs index 516af3d65..d714d9482 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, quietFs, quietRead } from "./errors.mjs"; export const APPROVE_WORD = "approve"; export const CUSTOM_ID_PREFIX = "approve:"; @@ -95,19 +95,29 @@ export function appendApproval(dir, entry) { if (!isObject(entry) || typeof entry.kind !== "string") throw new DiscordError("approvals: entry needs a kind", 1); const line = JSON.stringify(entry); if (line.includes("\n")) throw new DiscordError("approvals: line must not contain a newline", 1); - appendFileSync(approvalsPath(dir), line + "\n", { mode: 0o600 }); + try { + appendFileSync(approvalsPath(dir), line + "\n", { mode: 0o600 }); + } catch (e) { + throw quietFs(e, "approvals.jsonl", "append to"); + } } 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}`)); } catch { - throw new DiscordError(`${path}:${i + 1}: not valid JSON`); + throw new DiscordError(`approvals.jsonl:${i + 1}: not valid JSON`); } } return out; diff --git a/packages/discord/src/binding.mjs b/packages/discord/src/binding.mjs index 6076f52ee..359aa2534 100644 --- a/packages/discord/src/binding.mjs +++ b/packages/discord/src/binding.mjs @@ -22,14 +22,14 @@ import { parseJsonFileText } from "../../organization/src/util.mjs"; import { existsSync, lstatSync, readFileSync, realpathSync, statSync } from "node:fs"; import { loadWebConfig } from "./web.mjs"; import { loadSetsparkConfig } from "./setspark.mjs"; -import { isAbsolute, join, resolve, sep } from "node:path"; +import { basename, isAbsolute, join, resolve, sep } from "node:path"; import { homedir } from "node:os"; -import { DiscordError } from "./errors.mjs"; +import { BINDING_NAME, DiscordError, plainKey, quietRead } from "./errors.mjs"; import { TOOL_DEFAULTS } from "./tools.mjs"; import { loadGitConfig } from "./git.mjs"; export const BINDING_VERSION = 1; -export const BINDING_NAME = /^[a-z0-9][a-z0-9._-]{0,63}$/; +export { BINDING_NAME }; export const SNOWFLAKE = /^[0-9]{17,20}$/; export const CHANNEL_MODES = Object.freeze(["open", "mention"]); export const THINKING_LEVELS = Object.freeze(["off", "minimal", "low", "medium", "high", "xhigh", "max"]); @@ -56,16 +56,17 @@ export function defaultConfigPath(env = process.env) { } // Only dataRoot is read here; scripts/mosaic-config.mjs owns full validation. +// Refusals name the config, never its path (#1873). export function loadDataRoot(path = defaultConfigPath()) { - if (!existsSync(path)) throw new DiscordError(`config not found: ${path}`); + if (!existsSync(path)) throw new DiscordError("config not found"); let raw; try { raw = parseJsonFileText(readFileSync(path, "utf8"), path); } catch (error) { - throw new DiscordError(`config is not valid JSON: ${path}${error.code ? ` (${error.code})` : ""}`); + throw new DiscordError(`config is not valid JSON${error.code ? ` (${error.code})` : ""}`); } - if (!raw || typeof raw !== "object" || Array.isArray(raw)) throw new DiscordError(`config is not an object: ${path}`); - if (typeof raw.dataRoot !== "string" || !isAbsolute(raw.dataRoot)) throw new DiscordError(`config.dataRoot must be an absolute path: ${path}`); + if (!raw || typeof raw !== "object" || Array.isArray(raw)) throw new DiscordError("config is not an object"); + if (typeof raw.dataRoot !== "string" || !isAbsolute(raw.dataRoot)) throw new DiscordError("config.dataRoot must be an absolute path"); return raw.dataRoot; } @@ -73,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); } @@ -89,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)}`); } } @@ -274,38 +278,49 @@ export function reloadDiff(current, next) { } // A private file: regular, not a symlink, owner-only (0600), non-empty. +// `what` is the label a refusal names; the path never appears (#1873). export function checkPrivateFile(path, what) { let st; try { st = lstatSync(path); } catch { - throw new DiscordError(`${what} not found: ${path}`); + throw new DiscordError(`${what} not found`); } - if (st.isSymbolicLink()) throw new DiscordError(`${what} must not be a symlink: ${path}`); - if (!st.isFile()) throw new DiscordError(`${what} is not a regular file: ${path}`); + if (st.isSymbolicLink()) throw new DiscordError(`${what} must not be a symlink`); + if (!st.isFile()) throw new DiscordError(`${what} is not a regular file`); const mode = st.mode & 0o777; - if (mode !== 0o600) throw new DiscordError(`${what} must be mode 0600, is ${mode.toString(8).padStart(4, "0")}: ${path}`); - if (st.size === 0) throw new DiscordError(`${what} is empty: ${path}`); + if (mode !== 0o600) throw new DiscordError(`${what} must be mode 0600, is ${mode.toString(8).padStart(4, "0")}`); + if (st.size === 0) throw new DiscordError(`${what} is empty`); return st; } +// The binding is named by its file's name when that is a valid binding name +// (bindingPath() makes it so), never by its path. export function loadBinding(path) { - checkPrivateFile(path, "binding"); + const name = basename(String(path), ".json"); + const label = BINDING_NAME.test(name) ? `binding ${name}` : "binding"; + checkPrivateFile(path, label); let raw; try { raw = parseJsonFileText(readFileSync(path, "utf8"), path); } catch (error) { - throw new DiscordError(`binding is not valid JSON: ${path}${error.code ? ` (${error.code})` : ""}`); + throw new DiscordError(`${label} is not valid JSON${error.code ? ` (${error.code})` : ""}`); } - return validateBinding(raw, `binding ${path}`); + return validateBinding(raw, label); } // The token is read once into memory and handed to the REST and gateway // clients. It is never printed, journaled, or put on a command line. export function readToken(binding) { checkPrivateFile(binding.tokenFile, "token file"); - const token = readFileSync(binding.tokenFile, "utf8").trim(); - if (!/^[A-Za-z0-9._-]{20,}$/.test(token)) throw new DiscordError(`token file does not hold a bot token: ${binding.tokenFile}`); + // A file that changes after its check fails with a native error that carries the path (#1873). + let token; + try { + token = readFileSync(binding.tokenFile, "utf8").trim(); + } catch (e) { + throw quietRead(e, "token file"); + } + if (!/^[A-Za-z0-9._-]{20,}$/.test(token)) throw new DiscordError("token file does not hold a bot token"); return token; } @@ -315,22 +330,34 @@ 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); - return binding.context.files.map((f) => { + 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}]`; if (typeof f !== "string" || f.length === 0) throw new DiscordError("context file must be a non-empty string"); - if (isAbsolute(f)) throw new DiscordError(`context file must be repository-relative: ${f}`); - if (f.split(/[\\/]/).includes("..")) throw new DiscordError(`context file must not escape the repository: ${f}`); + if (isAbsolute(f)) throw new DiscordError(`context file must be repository-relative: ${at}`); + if (f.split(/[\\/]/).includes("..")) throw new DiscordError(`context file must not escape the repository: ${at}`); const path = resolve(root, f); let st; try { st = lstatSync(path); } catch { - throw new DiscordError(`missing context file: ${path}`); + throw new DiscordError(`missing context file: ${at}`); } - if (st.isSymbolicLink()) throw new DiscordError(`context file must not be a symlink: ${path}`); - if (!st.isFile() || st.size === 0) throw new DiscordError(`context file is not a regular non-empty file: ${path}`); - const real = realpathSync(path); - if (real !== path || !real.startsWith(root + sep)) throw new DiscordError(`context file resolves outside the repository: ${f}`); + if (st.isSymbolicLink()) throw new DiscordError(`context file must not be a symlink: ${at}`); + if (!st.isFile() || st.size === 0) throw new DiscordError(`context file is not a regular non-empty file: ${at}`); + let real; + try { + real = realpathSync(path); + } catch (e) { + throw quietRead(e, `context file ${at}`); + } + if (real !== path || !real.startsWith(root + sep)) throw new DiscordError(`context file resolves outside the repository: ${at}`); return path; }); } @@ -363,18 +390,28 @@ function webConfig(raw, where) { export function resolveToolRoots(binding, { dataRoot }) { if (!binding.tools) return null; - const data = existsSync(dataRoot) ? realpathSync(dataRoot) : resolve(dataRoot); + let data; + try { + data = existsSync(dataRoot) ? realpathSync(dataRoot) : resolve(dataRoot); + } catch (e) { + throw quietRead(e, "the data root"); + } const roots = binding.tools.roots.map((r) => { let st; try { st = lstatSync(r.path); } catch { - throw new DiscordError(`tool root ${r.name} does not exist: ${r.path}`); + throw new DiscordError(`tool root ${r.name} does not exist`); } - if (st.isSymbolicLink()) throw new DiscordError(`tool root ${r.name} must not be a symlink: ${r.path}`); - if (!st.isDirectory()) throw new DiscordError(`tool root ${r.name} is not a directory: ${r.path}`); - const real = realpathSync(r.path); - if (real === data || real.startsWith(data + sep) || data.startsWith(real + sep)) throw new DiscordError(`tool root ${r.name} overlaps the data root: ${r.path}`); + if (st.isSymbolicLink()) throw new DiscordError(`tool root ${r.name} must not be a symlink`); + if (!st.isDirectory()) throw new DiscordError(`tool root ${r.name} is not a directory`); + let real; + try { + real = realpathSync(r.path); + } catch (e) { + throw quietRead(e, `tool root ${r.name}`); + } + if (real === data || real.startsWith(data + sep) || data.startsWith(real + sep)) throw new DiscordError(`tool root ${r.name} overlaps the data root`); return { name: r.name, path: real, write: r.write, ...(r.git ? { git: { branch: r.git.branch, identity: r.git.identity, tokenFile: r.git.tokenFile, author: `${r.git.author.name} <${r.git.author.email}>`, ...(r.git.protocol ? { protocol: r.git.protocol } : {}) } } : {}) }; }); // setspark carries only the keys loadSetsparkConfig accepts; the extension diff --git a/packages/discord/src/cli.mjs b/packages/discord/src/cli.mjs index e291d8c03..11726a514 100755 --- a/packages/discord/src/cli.mjs +++ b/packages/discord/src/cli.mjs @@ -46,22 +46,22 @@ // 3 refused by a brake (STOP present or the binding held; a supervisor must // not retry); 4 usage. -import { existsSync, mkdirSync, mkdtempSync, writeFileSync, statSync, readdirSync } from "node:fs"; +import { existsSync, mkdirSync, mkdtempSync, writeFileSync } from "node:fs"; import { dirname, join, resolve } from "node:path"; import { createHash } from "node:crypto"; -import { DiscordError } from "./errors.mjs"; +import { DiscordError, printable } from "./errors.mjs"; import { assertProcessing, processingStatus, ProcessingError } from "../../processing-control/src/state.mjs"; import { registerRuntime, updateRuntime, unregisterRuntime, processIdentity } from "../../processing-control/src/runtimes.mjs"; import { loadSeatSets } from "../../organization/src/library-seats.mjs"; import { defaultConfigPath, loadDataRoot, bindingPath, bindingDataDir, loadBinding, readToken, resolveContextFiles, resolveToolRoots, reloadDiff } from "./binding.mjs"; import { createRest } from "./rest.mjs"; import { createGateway, CONNECTOR_INTENTS } from "./gateway.mjs"; -import { createEngine, buildPiArgs } from "./engine-pi.mjs"; +import { createEngine, buildPiArgs, sessionContinues } from "./engine-pi.mjs"; import { TOOLS_ENV, enabledToolNames } from "./tools.mjs"; import { createSetsparkApi } from "./setspark.mjs"; import { assembleContext } from "./context.mjs"; import { createConnector } from "./connector.mjs"; -import { ensureJournal, requestStop, requestProcessingStop, stopRequested, readPid, stopTarget, writePid, clearPid, unlock, recover, appendReload, BRAKE_EXIT } from "./journal.mjs"; +import { ensureJournal, requestStop, requestProcessingStop, stopRequested, readPid, stopTarget, writePid, clearPid, unlock, recover, appendReload, journalLabel, BRAKE_EXIT } from "./journal.mjs"; const USAGE = [ "usage: mosaic-discord check [--config PATH] [--repo PATH] [--offline]", @@ -114,7 +114,7 @@ function checkLibraryEngine(configDirectory, binding) { try { sets = loadSeatSets({ dir: configDirectory }); } catch (err) { - throw new DiscordError(`discord-business-unreadable: ${err.message}`, 2); + throw new DiscordError(`discord-business-unreadable: ${printable(err, "a business file")}`, 2); } if (!sets.librarySeats.has(binding.seat)) return; if (binding.engine.provider !== "mosaic-bus") { @@ -135,7 +135,7 @@ function prepare(opts) { const contextFiles = resolveContextFiles(binding, opts.repo); const toolRoots = resolveToolRoots(binding, { dataRoot }); const pi = join(opts.repo, "node_modules", ".bin", "pi"); - if (!existsSync(pi)) throw new DiscordError(`pi not found at ${pi}; run npm ci in the repository`); + if (!existsSync(pi)) throw new DiscordError("pi not found at node_modules/.bin/pi in the repository; run npm ci in the repository"); const journalDir = bindingDataDir(dataRoot, binding.name); const sessionDir = join(dataRoot, "sessions", `discord-${binding.name}`); return { dataRoot, bindingFile, binding, contextFiles, toolRoots, pi, journalDir, sessionDir }; @@ -205,7 +205,7 @@ async function run(opts) { const outcome = recover(journalDir); if (outcome === "cleared") warn("supervised start: run.lock left by a process that is gone was removed"); } - if (stopRequested(journalDir)) throw new DiscordError(`STOP is present in ${journalDir}; remove it to run`, BRAKE_EXIT); + if (stopRequested(journalDir)) throw new DiscordError(`STOP is present in ${journalLabel(journalDir)}; remove it to run`, BRAKE_EXIT); writePid(journalDir, process.pid); let managed = null; const cleanupPid = () => { @@ -234,7 +234,7 @@ async function run(opts) { const promptFile = join(launch, "context.md"); writeFileSync(promptFile, snapshot.text, { mode: 0o600 }); writeFileSync(join(launch, "context.sha256"), `${snapshot.sha256} context.md\n`, { mode: 0o600 }); - const continueSession = existsSync(sessionDir) && statSync(sessionDir).isDirectory() && hasJsonl(sessionDir); + const continueSession = sessionContinues(sessionDir); const rest = createRest({ token, log: warn }); const gw = await rest.getGatewayBot(); @@ -272,7 +272,7 @@ async function run(opts) { clearInterval(processingWatch); exitCode = code; warn(immediate ? 'stopping: processing halted' : "stopping: finishing in-flight turns"); - connector.stop({ immediate }).catch((err) => warn(`stop failed: ${err.message}`)).finally(() => { + connector.stop({ immediate }).catch((err) => warn(`stop failed: ${printable(err)}`)).finally(() => { cleanupPid(); process.exit(exitCode); }); @@ -317,8 +317,8 @@ async function run(opts) { const n = (d) => `+${d.added.length} -${d.removed.length} ~${d.changed.length}`; warn(`reload applied: channels ${n(diff.channels)}, users ${n(diff.users)}, limits ${diff.limits.length ? diff.limits.join(",") : "unchanged"}${diff.guildName ? ", guildName" : ""}`); } catch (err) { - appendReload(journalDir, { at, outcome: "refused", error: err.message }); - warn(`reload refused, binding unchanged: ${err.message}`); + appendReload(journalDir, { at, outcome: "refused", error: printable(err) }); + warn(`reload refused, binding unchanged: ${printable(err)}`); } }; let reloadChain = Promise.resolve(); @@ -332,13 +332,6 @@ async function run(opts) { } } -function hasJsonl(dir) { - try { - return readdirSync(dir).some((f) => f.endsWith(".jsonl")); - } catch { - return false; - } -} function stop(opts) { const dataRoot = loadDataRoot(opts.config); @@ -413,7 +406,8 @@ main().then((code) => { if (code !== null) process.exit(code); }).catch((err) => { const code = err instanceof ProcessingError ? BRAKE_EXIT : err instanceof DiscordError ? err.exitCode : 1; - warn(err.message); + // Never a native error's message: it carries the path (#1873). + warn(printable(err)); if (err.details && err.details.nonces) warn(`nonces: ${err.details.nonces.join(", ")}`); process.exit(code); }); diff --git a/packages/discord/src/connector.mjs b/packages/discord/src/connector.mjs index a99a96794..74833e4c3 100644 --- a/packages/discord/src/connector.mjs +++ b/packages/discord/src/connector.mjs @@ -35,7 +35,7 @@ import { reloadDiff } from "./binding.mjs"; import { envelope, splitReply } from "./context.mjs"; import { ensureJournal, appendInbox, readInboxIds, appendOutbox, unresolvedOutbox, appendDrop, - writeTurn, appendAdmission, countAdmissionsOn, appendNotice, noticeOn, utcDate, stopRequested, + writeTurn, appendAdmission, countAdmissionsOn, appendNotice, noticeOn, utcDate, stopRequested, journalLabel, } from "./journal.mjs"; import { RestOutcome } from "./rest.mjs"; import { DiscordError } from "./errors.mjs"; @@ -596,7 +596,7 @@ ${status === "not-posted" ? APPROVAL_LINES.pressAgain : APPROVAL_LINES.failed}`; async start() { processing(); if (state.started) throw new DiscordError("connector already started", 1); - if (stopRequested(journalDir)) throw new DiscordError(`STOP is present in ${journalDir}; remove it to run`, 1); + if (stopRequested(journalDir)) throw new DiscordError(`STOP is present in ${journalLabel(journalDir)}; remove it to run`, 1); const reconciled = await reconcile(); const approvals = await reconcileApprovals(); processing(); diff --git a/packages/discord/src/engine-pi.mjs b/packages/discord/src/engine-pi.mjs index 96b4aa118..087e4355e 100644 --- a/packages/discord/src/engine-pi.mjs +++ b/packages/discord/src/engine-pi.mjs @@ -32,8 +32,9 @@ // without the connector noticing: the contract is start(), prompt(), stop(). import { spawn as nodeSpawn } from "node:child_process"; +import { existsSync, readdirSync, statSync } from "node:fs"; import { fileURLToPath } from "node:url"; -import { DiscordError } from "./errors.mjs"; +import { DiscordError, printable, quietFs } from "./errors.mjs"; import { enabledToolNames } from "./tools.mjs"; import { assertProcessing, processingStatus } from "../../processing-control/src/state.mjs"; import { registerRuntime, updateRuntime, unregisterRuntime, processIdentity, runtime, runtimeGroupState, createRuntimeSignaller } from "../../processing-control/src/runtimes.mjs"; @@ -51,6 +52,25 @@ export const TOOLS_EXTENSION = fileURLToPath(new URL("../extension/tools.mjs", i // Older name, kept for the tests and docs that still use it. export const READONLY_TOOLS_EXTENSION = TOOLS_EXTENSION; +// Whether pi continues the session in sessionDir: it is a directory holding a +// .jsonl session file. A stat that fails names the binding's session directory, +// never its path (#1873); a listing that fails means a new session, as before. +export function sessionContinues(sessionDir) { + if (!existsSync(sessionDir)) return false; + let st; + try { + st = statSync(sessionDir); + } catch (e) { + throw quietFs(e, "the binding's session directory"); + } + if (!st.isDirectory()) return false; + try { + return readdirSync(sessionDir).some((f) => f.endsWith(".jsonl")); + } catch { + return false; + } +} + export function buildPiArgs({ provider, model, thinking, sessionDir, appendSystemPromptFile, continueSession, tools = null }) { const args = [...PI_FIXED_ARGS]; if (tools) args.push("--no-builtin-tools", "--extension", TOOLS_EXTENSION, "--tools", enabledToolNames(tools).join(",")); @@ -393,9 +413,11 @@ export function createEngine({ child.stderr.on("data", (chunk) => log(`pi: ${chunk.trimEnd()}`)); child.on("error", (err) => { releaseRuntime(); - log(`engine spawn error: ${err.message}`); - state.exited = { code: null, signal: null, error: err.message }; - failAll("engine-down", `engine failed: ${err.message}`); + // A spawn error's message carries the command's path (#1873). + const why = printable(err, "pi"); + log(`engine spawn error: ${why}`); + state.exited = { code: null, signal: null, error: why }; + failAll("engine-down", `engine failed: ${why}`); onExit(state.exited); }); child.on("exit", (code, signal) => { diff --git a/packages/discord/src/errors.mjs b/packages/discord/src/errors.mjs index f15f12477..dbeb46fe7 100644 --- a/packages/discord/src/errors.mjs +++ b/packages/discord/src/errors.mjs @@ -1,6 +1,48 @@ // 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 binding (and seat) name. Defined here, not in binding.mjs, so journal.mjs can label a +// journal directory without importing binding.mjs and its config readers; copies of +// journal.mjs + errors.mjs (the queue test repo) stay self-contained. binding.mjs re-exports it. +export const BINDING_NAME = /^[a-z0-9][a-z0-9._-]{0,63}$/; + +// A filesystem call that fails names the file by label, the call 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, and +// it is marked so the CLI's top-level handler prints it as it is (#1873). +const QUIET = Symbol("discord.quiet"); +const SYSCALL = /^[a-z][a-z0-9_]{0,31}$/; +const CODE = /^[A-Z][A-Z0-9_]{0,63}$/; +const callOf = (e) => { + const word = typeof e.syscall === "string" ? e.syscall.split(" ")[0] : ""; + return SYSCALL.test(word) ? word : "access"; +}; +const codeOf = (e) => (typeof e.code === "string" && CODE.test(e.code) ? e.code : "UNKNOWN"); +export function quietFs(e, label, verb = null) { + if (e && typeof e === "object" && typeof e.code === "string") { + e.message = `cannot ${verb ?? callOf(e)} ${label} (${codeOf(e)})`; + e[QUIET] = true; + } + return e; +} +export const quietRead = (e, label) => quietFs(e, label, "read"); + +// What a top-level handler prints for an error (#1873). Our refusals and an +// error already labelled at its source print their own message. A native +// Node error (it carries a syscall, or a code and a path) that slipped past +// every source label prints only its call and code, never its message, +// which carries the path; `label` names the file when the error has one. +export function printable(err, label = "a discord file") { + if (err instanceof DiscordError || (err && err[QUIET] === true)) return String(err.message); + if (err && typeof err === "object" && (typeof err.syscall === "string" || (typeof err.code === "string" && typeof err.path === "string"))) { + return typeof err.path === "string" ? `cannot ${callOf(err)} ${label} (${codeOf(err)})` : `cannot ${callOf(err)} (${codeOf(err)})`; + } + return err && typeof err.message === "string" ? err.message : String(err); +} + 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 2351b4270..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({ @@ -139,12 +140,12 @@ function checkPrivateFile(path, what) { try { st = lstatSync(path); } catch { - throw new Error(`${what}: not found: ${path}`); + throw new Error(`${what}: not found`); } - if (st.isSymbolicLink()) throw new Error(`${what}: must not be a symlink: ${path}`); - if (!st.isFile()) throw new Error(`${what}: not a regular file: ${path}`); - if ((st.mode & 0o777) !== 0o600) throw new Error(`${what}: must be mode 0600: ${path}`); - if (st.size === 0) throw new Error(`${what}: is empty: ${path}`); + if (st.isSymbolicLink()) throw new Error(`${what}: must not be a symlink`); + if (!st.isFile()) throw new Error(`${what}: not a regular file`); + if ((st.mode & 0o777) !== 0o600) throw new Error(`${what}: must be mode 0600`); + if (st.size === 0) throw new Error(`${what}: is empty`); } // The environment every child sees. No global or system git configuration, @@ -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 a8a31dde7..ff1fb376b 100644 --- a/packages/discord/src/journal.mjs +++ b/packages/discord/src/journal.mjs @@ -25,8 +25,16 @@ import { appendFileSync, closeSync, constants, existsSync, fstatSync, fsyncSync, mkdirSync, openSync, readdirSync, readFileSync, readSync, renameSync, rmdirSync, rmSync, unlinkSync, writeFileSync, writeSync, } from "node:fs"; -import { join } from "node:path"; -import { DiscordError } from "./errors.mjs"; +import { basename, join } from "node:path"; +import { BINDING_NAME, DiscordError, quietFs, quietRead } from "./errors.mjs"; + +// Where a refusal says a binding's journal is: its place under the data root +// when the directory carries a valid binding name (bindingDataDir() makes it +// so), never the host path (#1873). +export function journalLabel(dir) { + const name = basename(String(dir)); + return BINDING_NAME.test(name) ? `discord/${name} under the data root` : "the binding's journal directory"; +} export const OUTBOX_STATUS = Object.freeze(["intent", "confirmed", "refused", "unknown"]); @@ -38,19 +46,28 @@ export function ensureJournal(dir) { function appendLine(path, record) { const line = JSON.stringify(record); if (line.includes("\n")) throw new DiscordError("journal line must not contain a newline", 1); - appendFileSync(path, line + "\n", { mode: 0o600 }); + try { + appendFileSync(path, line + "\n", { mode: 0o600 }); + } catch (e) { + throw quietFs(e, basename(path), "append to"); + } } 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 { out.push(parseJsonFileText(line, `${path}:${i + 1}`)); } catch { - throw new DiscordError(`${path}:${i + 1}: not valid JSON`); + throw new DiscordError(`${basename(path)}:${i + 1}: not valid JSON`); } } return out; @@ -109,42 +126,61 @@ 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`); } +// A parse refusal or a failed read or write names the turn by its id, never by +// host path (PR #1916, #1873); a listed file's id is named only when it passes TURN_ID. +const turnLabel = (id) => (TURN_ID.test(id) ? `turn record ${id}` : "turn record"); + export function writeTurn(dir, id, record) { const path = turnPath(dir, id); let fd; try { fd = openSync(path, "wx", 0o600); } catch (err) { - if (err.code === "EEXIST") throw new DiscordError(`turn record already exists: ${path}`, 1); - throw err; + if (err.code === "EEXIST") throw new DiscordError(`turn record already exists: ${id}`, 1); + throw quietFs(err, turnLabel(String(id)), "write"); } try { - writeSync(fd, JSON.stringify({ ...record, id }, null, 2) + "\n"); - } finally { - closeSync(fd); + try { + writeSync(fd, JSON.stringify({ ...record, id }, null, 2) + "\n"); + } finally { + closeSync(fd); + } + } catch (err) { + throw quietFs(err, turnLabel(String(id)), "write"); } return path; } -// A parse refusal names the turn by its id, never by host path (PR #1916); a -// listed file's id is named only when it passes TURN_ID. -const turnLabel = (id) => (TURN_ID.test(id) ? `turn record ${id}` : "turn record"); +function readTurnFile(path, id) { + let text; + try { + text = readFileSync(path, "utf8"); + } catch (e) { + throw quietRead(e, turnLabel(id)); + } + return parseJsonFileText(text, path, turnLabel(id)); +} export function readTurn(dir, id) { - const path = turnPath(dir, id); - return parseJsonFileText(readFileSync(path, "utf8"), path, turnLabel(String(id))); + return readTurnFile(turnPath(dir, id), String(id)); } export function listTurns(dir) { const turns = join(dir, "turns"); if (!existsSync(turns)) return []; - return readdirSync(turns) + let names; + try { + names = readdirSync(turns); + } catch (e) { + throw quietRead(e, `the turns directory in ${journalLabel(dir)}`); + } + return names .filter((f) => f.endsWith(".json")) - .map((f) => parseJsonFileText(readFileSync(join(turns, f), "utf8"), join(turns, f), turnLabel(f.slice(0, -".json".length)))); + .map((f) => readTurnFile(join(turns, f), f.slice(0, -".json".length))); } // The daily ceiling counts admissions on the current UTC date. An admission @@ -189,21 +225,38 @@ function stopLock(dir, action) { const lock = join(dir, '.stop.lock'); try { mkdirSync(lock, { mode: 0o700 }); } catch { throw new DiscordError('STOP update is locked; refusing', 1); } - try { return action(); } finally { rmdirSync(lock); } + try { return action(); } finally { + try { rmdirSync(lock); } catch (e) { throw quietFs(e, `.stop.lock in ${journalLabel(dir)}`, "remove"); } + } } +// A STOP write or removal that fails names STOP in the journal, never its path (#1873). +const stopLabel = (dir) => `STOP in ${journalLabel(dir)}`; + function appendStop(dir, reason) { const path = stopPath(dir); - const fd = openSync(path, constants.O_WRONLY | constants.O_APPEND | constants.O_CREAT | constants.O_NOFOLLOW, 0o600); try { - writeSync(fd, JSON.stringify({ at: new Date().toISOString(), reason }) + "\n"); - fsyncSync(fd); - } finally { - closeSync(fd); + const fd = openSync(path, constants.O_WRONLY | constants.O_APPEND | constants.O_CREAT | constants.O_NOFOLLOW, 0o600); + try { + writeSync(fd, JSON.stringify({ at: new Date().toISOString(), reason }) + "\n"); + fsyncSync(fd); + } finally { + closeSync(fd); + } + } catch (e) { + throw quietFs(e, stopLabel(dir), "write"); } return path; } +function removeStop(dir) { + try { + unlinkSync(stopPath(dir)); + } catch (e) { + throw quietFs(e, stopLabel(dir), "remove"); + } +} + const PROCESSING_GENERATION = /^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/; function processingStops(dir) { let fd; @@ -235,14 +288,13 @@ export function clearProcessingStop(dir, generation) { if (!rows?.length || rows.some(r => !r || Object.keys(r).sort().join(',') !== 'at,reason' || r.reason !== `processing:${generation}` || !Number.isFinite(Date.parse(r.at)) || new Date(r.at).toISOString() !== r.at)) return false; - unlinkSync(stopPath(dir)); + removeStop(dir); return true; }); } export function clearStop(dir) { - const path = stopPath(dir); - if (existsSync(path)) unlinkSync(path); + if (existsSync(stopPath(dir))) removeStop(dir); } // The STOP lines as written, oldest first. A line that does not parse is @@ -250,7 +302,13 @@ export function clearStop(dir) { // treated as ours. export function readStop(dir) { if (!stopRequested(dir)) return []; - return readFileSync(stopPath(dir), "utf8").split("\n").filter((l) => l.trim() !== "").map((l) => { + let text; + try { + text = readFileSync(stopPath(dir), "utf8"); + } catch (e) { + throw quietRead(e, stopLabel(dir)); + } + return text.split("\n").filter((l) => l.trim() !== "").map((l) => { try { const v = JSON.parse(l); return v && typeof v === "object" && typeof v.reason === "string" ? v : { reason: null }; @@ -396,7 +454,7 @@ function lockRefusal(dir, opts) { const existing = readPid(dir); const state = ownerState(existing, opts); if (state === "absent") return new DiscordError(`run.lock exists without an owner record: a start is in progress or was interrupted; ${UNLOCK_HINT}`, BRAKE_EXIT); - if (state === "invalid") return new DiscordError(`run.lock has an owner record that cannot be read; refusing. Inspect ${ownerPath(dir)} by hand`, BRAKE_EXIT); + if (state === "invalid") return new DiscordError(`run.lock has an owner record that cannot be read; refusing. Inspect run.lock/owner.json in ${journalLabel(dir)} by hand`, BRAKE_EXIT); if (state === "live") return new DiscordError(`another connector is running for this binding (pid ${existing.pid})`, BRAKE_EXIT); if (state === "unknown") return new DiscordError(`run.lock belongs to pid ${existing.pid}, which is alive but whose identity cannot be verified; refusing`, BRAKE_EXIT); return new DiscordError(`run.lock belongs to pid ${existing.pid}, which is gone or is a different process now; ${UNLOCK_HINT}`, BRAKE_EXIT); @@ -409,17 +467,21 @@ export function writePid(dir, pid, { now = Date.now(), identity = identityOf } = try { mkdirSync(lock, { mode: 0o700 }); } catch (err) { - if (err.code !== "EEXIST") throw err; + if (err.code !== "EEXIST") throw quietFs(err, `run.lock in ${journalLabel(dir)}`, "create"); throw lockRefusal(dir, { identity }); } const tmp = join(lock, "owner.json.tmp"); - writeFileSync(tmp, JSON.stringify({ pid, start, boot, at: new Date(now).toISOString() }) + "\n", { mode: 0o600 }); - renameSync(tmp, ownerPath(dir)); + try { + writeFileSync(tmp, JSON.stringify({ pid, start, boot, at: new Date(now).toISOString() }) + "\n", { mode: 0o600 }); + renameSync(tmp, ownerPath(dir)); + } catch (e) { + throw quietFs(e, `run.lock/owner.json in ${journalLabel(dir)}`, "write"); + } // The gate: STOP written before this point (by `stop` or `unlock`) means // this claim must not stand, however it interleaved with an unlock. if (stopRequested(dir)) { clearPid(dir, pid); - throw new DiscordError(`STOP is present in ${dir}; remove it to run`, BRAKE_EXIT); + throw new DiscordError(`STOP is present in ${journalLabel(dir)}; remove it to run`, BRAKE_EXIT); } } @@ -439,9 +501,9 @@ export function unlock(dir, { beforeRemove = null, identity = identityOf, reason const state = ownerState(rec, { identity }); if (state === "live") throw new DiscordError(`refusing to unlock: the connector is running (pid ${rec.pid}); use stop, and unlock only a lock whose owner is gone`, 1); if (state === "unknown") throw new DiscordError(`refusing to unlock: pid ${rec.pid} is alive and its identity cannot be verified; nothing removed. Stop that process first`, 1); - if (state === "invalid") throw new DiscordError(`refusing to unlock: the owner record cannot be read; nothing removed. Inspect ${ownerPath(dir)} by hand`, 1); + if (state === "invalid") throw new DiscordError(`refusing to unlock: the owner record cannot be read; nothing removed. Inspect run.lock/owner.json in ${journalLabel(dir)} by hand`, 1); if (beforeRemove) beforeRemove(); - rmSync(lock, { recursive: true, force: true, maxRetries: 10, retryDelay: 20 }); + removeLock(dir, { maxRetries: 10, retryDelay: 20 }); return rec; } @@ -468,14 +530,14 @@ export function unlock(dir, { beforeRemove = null, identity = identityOf, reason export function recover(dir, { identity = identityOf, beforeRemove = null } = {}) { const ours = (lines) => lines.length === 1 && lines[0].reason === RECOVER_REASON; if (stopRequested(dir)) { - if (!ours(readStop(dir))) throw new DiscordError(`STOP is present in ${dir}; the brake is on. Remove it to run`, BRAKE_EXIT); + if (!ours(readStop(dir))) throw new DiscordError(`STOP is present in ${journalLabel(dir)}; the brake is on. Remove it to run`, BRAKE_EXIT); clearStop(dir); } if (!existsSync(lockPath(dir))) return "clean"; const state = ownerState(readPid(dir), { identity }); if (state === "live" || state === "unknown" || state === "invalid") throw lockRefusal(dir, { identity }); unlock(dir, { identity, reason: RECOVER_REASON, beforeRemove }); - if (!ours(readStop(dir))) throw new DiscordError(`STOP was written by an operator while recovering ${dir}; the brake stays. Remove it to run`, BRAKE_EXIT); + if (!ours(readStop(dir))) throw new DiscordError(`STOP was written by an operator while recovering ${journalLabel(dir)}; the brake stays. Remove it to run`, BRAKE_EXIT); clearStop(dir); return "cleared"; } @@ -489,7 +551,15 @@ export function stopTarget(dir, opts) { export function clearPid(dir, pid) { const rec = readPid(dir); - if (rec !== null && !rec.invalid && rec.pid === pid) rmSync(lockPath(dir), { recursive: true, force: true }); + if (rec !== null && !rec.invalid && rec.pid === pid) removeLock(dir); +} + +function removeLock(dir, retry = {}) { + try { + rmSync(lockPath(dir), { recursive: true, force: true, ...retry }); + } catch (e) { + throw quietFs(e, `run.lock in ${journalLabel(dir)}`, "remove"); + } } // --- reloads --- diff --git a/packages/discord/src/setspark.mjs b/packages/discord/src/setspark.mjs index 41b0c95de..aceabe88c 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, quietRead } from "./errors.mjs"; export const SETSPARK_DEFAULTS = Object.freeze({ timeoutMs: 15000, maxResponseBytes: 262144 }); export const USER_AGENT = "mosaic-discord-sage/1 (Mosaic Stack Discord connector; setspark client)"; @@ -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; @@ -108,12 +109,12 @@ function checkPrivateFile(path, what) { try { st = lstatSync(path); } catch { - throw new Error(`${what}: not found: ${path}`); + throw new Error(`${what}: not found`); } - if (st.isSymbolicLink()) throw new Error(`${what}: must not be a symlink: ${path}`); - if (!st.isFile()) throw new Error(`${what}: not a regular file: ${path}`); - if ((st.mode & 0o777) !== 0o600) throw new Error(`${what}: must be mode 0600: ${path}`); - if (st.size === 0) throw new Error(`${what}: is empty: ${path}`); + if (st.isSymbolicLink()) throw new Error(`${what}: must not be a symlink`); + if (!st.isFile()) throw new Error(`${what}: not a regular file`); + if ((st.mode & 0o777) !== 0o600) throw new Error(`${what}: must be mode 0600`); + if (st.size === 0) throw new Error(`${what}: is empty`); } // Read the key for one call. The file is re-checked every time, so a key @@ -132,7 +133,14 @@ export function readKey(config) { throw new SetsparkRefusal(SETSPARK_REFUSAL.KEY_FILE); } if (st.size > KEY_MAX_BYTES) throw new SetsparkRefusal(SETSPARK_REFUSAL.KEY_SHAPE); - const text = readFileSync(config.keyFile, "utf8"); + // A file that changes after its checks fails with a native error that carries the path: it + // keeps its type and code and names the field (#1873). + let text; + try { + text = readFileSync(config.keyFile, "utf8"); + } catch (e) { + throw quietRead(e, "keyFile"); + } // Either one bare key line, or the mint's own JSON output // ({"key_id", "key", "note"}) stored as is; only "key" is used. let key = text; diff --git a/packages/discord/tests/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/connector.test.mjs b/packages/discord/tests/connector.test.mjs index a12018c33..f8aab191d 100644 --- a/packages/discord/tests/connector.test.mjs +++ b/packages/discord/tests/connector.test.mjs @@ -230,7 +230,7 @@ test("restart: an inbox with three ids and a replay of the same three produces z test("stop: STOP present refuses start; STOP written while running refuses new turns and the current one finishes", async () => { const { journalDir, rest, engine, connector } = setup({ replies: [{ text: "done", delayMs: 30 }] }); requestStop(journalDir, "test"); - await assert.rejects(connector.start(), /STOP is present/); + await assert.rejects(connector.start(), (err) => /STOP is present/.test(err.message) && !err.message.includes(journalDir)); assert.equal(engine.started, false); writeFileSync(join(journalDir, "STOP"), ""); // still present; clear it const { unlinkSync } = await import("node:fs"); diff --git a/packages/discord/tests/native-fs-labels.test.mjs b/packages/discord/tests/native-fs-labels.test.mjs new file mode 100644 index 000000000..93deda873 --- /dev/null +++ b/packages/discord/tests/native-fs-labels.test.mjs @@ -0,0 +1,225 @@ +// #1873 (redact6, re-check 2): a native fs error never prints its path. The journal and +// approvals readers and writers name the file by label at the source, and the CLI's +// top-level handler prints any native error that slipped through as its call and code +// only. The sentinel is a component of every scratch path, so an error message that +// quotes a path quotes the sentinel too. +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import fs, { existsSync, mkdtempSync, mkdirSync, writeFileSync, rmSync } from 'node:fs'; +import { syncBuiltinESMExports } from 'node:module'; +import { spawnSync } from 'node:child_process'; +import { fileURLToPath } from 'node:url'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { appendApproval } from '../src/approvals.mjs'; +import { DiscordError } from '../src/errors.mjs'; +import { + appendInbox, clearStop, ensureJournal, listTurns, lockPath, readTurn, recover, requestStop, stopPath, turnPath, writePid, writeTurn, +} from '../src/journal.mjs'; +import { makeDeployment, makeRepo } from './helpers.mjs'; +import { initializeControl } from '../../processing-control/src/state.mjs'; + +const secret = 'sk-test-NOT-A-SECRET-1234'; +const cli = fileURLToPath(new URL('../src/cli.mjs', import.meta.url)); + +// A journal directory named like a binding, under a sentinel path component, so its +// label is "discord/seat under the data root". +function scratch(t) { + const root = mkdtempSync(join(tmpdir(), 'r6n-')); + t.after(() => rmSync(root, { recursive: true, force: true })); + const dir = join(root, secret, 'seat'); + mkdirSync(dir, { recursive: true }); + return { root, dir }; +} +const where = 'discord/seat under the data root'; +function quiet(text, root) { + assert.ok(text.includes(root) === false, text); + assert.doesNotMatch(text, /sk-test|NOT-A-SECRET/); +} +function labelled(label, root) { + return error => { + assert.ok(error instanceof Error, String(error)); + assert.equal(error instanceof DiscordError, false, 'a native fs error keeps its type'); + assert.ok(error.message.includes(label), error.message); + quiet(error.message, root); + return true; + }; +} + +// Swap one node:fs function so the target goes away right before the real call, which +// then fails with a genuine error that names the path. One swap per test. +function race(t, fn, target, remove) { + const original = fs[fn]; + const swapped = function (p, ...rest) { + if (String(p) === target) remove(); + return original.call(this, p, ...rest); + }; + if (original.native) swapped.native = original.native; + fs[fn] = swapped; + syncBuiltinESMExports(); + t.after(() => { + fs[fn] = original; + syncBuiltinESMExports(); + }); +} + +// --- the four named sites --- + +test('recover: a STOP removed between its check and its read names STOP in the journal', t => { + const { root, dir } = scratch(t); + ensureJournal(dir); + writeFileSync(stopPath(dir), JSON.stringify({ at: new Date().toISOString(), reason: 'cli stop' }) + '\n', { mode: 0o600 }); + race(t, 'readFileSync', stopPath(dir), () => rmSync(stopPath(dir))); + assert.throws(() => recover(dir), labelled(`cannot read STOP in ${where} (ENOENT)`, root)); +}); + +test('readTurn: a turn record removed before its read names the turn by id', t => { + const { root, dir } = scratch(t); + ensureJournal(dir); + writeTurn(dir, 't1', { startedAt: new Date().toISOString() }); + race(t, 'readFileSync', turnPath(dir, 't1'), () => rmSync(turnPath(dir, 't1'))); + assert.throws(() => readTurn(dir, 't1'), labelled('cannot read turn record t1 (ENOENT)', root)); +}); + +test('listTurns: a turns directory removed between its check and its listing names the turns directory', t => { + const { root, dir } = scratch(t); + ensureJournal(dir); + const turns = join(dir, 'turns'); + race(t, 'readdirSync', turns, () => rmSync(turns, { recursive: true })); + assert.throws(() => listTurns(dir), labelled(`cannot read the turns directory in ${where} (ENOENT)`, root)); +}); + +test('listTurns: a turn record removed between the listing and its read names the turn by id', t => { + const { root, dir } = scratch(t); + ensureJournal(dir); + writeTurn(dir, 't1', { startedAt: new Date().toISOString() }); + race(t, 'readFileSync', turnPath(dir, 't1'), () => rmSync(turnPath(dir, 't1'))); + assert.throws(() => listTurns(dir), labelled('cannot read turn record t1 (ENOENT)', root)); +}); + +test("run: a session directory removed between its check and its stat names the binding's session directory", async t => { + const { root, dir } = scratch(t); + const { sessionContinues } = await import('../src/engine-pi.mjs'); + assert.equal(typeof sessionContinues, 'function', 'engine-pi.mjs exports sessionContinues'); + const session = join(dir, 'session'); + assert.equal(sessionContinues(session), false, 'no directory: a new session'); + mkdirSync(session); + assert.equal(sessionContinues(session), false, 'an empty directory: a new session'); + writeFileSync(join(session, 'a.jsonl'), ''); + assert.equal(sessionContinues(session), true, 'a .jsonl file: the session continues'); + race(t, 'statSync', session, () => rmSync(session, { recursive: true })); + assert.throws(() => sessionContinues(session), labelled("cannot stat the binding's session directory (ENOENT)", root)); +}); + +// --- journal and approvals writers (the connector logs err.message at runtime) --- + +test('a journal line that cannot be appended names the journal file, never its path', t => { + const { root, dir } = scratch(t); + ensureJournal(dir); + mkdirSync(join(dir, 'inbox.jsonl')); + assert.throws(() => appendInbox(dir, { id: '1' }), labelled('cannot append to inbox.jsonl (EISDIR)', root)); +}); + +test('an approvals line that cannot be appended names approvals.jsonl, never its path', t => { + const { root, dir } = scratch(t); + ensureJournal(dir); + mkdirSync(join(dir, 'approvals.jsonl')); + assert.throws(() => appendApproval(dir, { kind: 'opened' }), labelled('cannot append to approvals.jsonl (EISDIR)', root)); +}); + +test('a turn record that cannot be created names the turn by id; an existing one keeps its refusal', t => { + const { root, dir } = scratch(t); + assert.throws(() => writeTurn(dir, 't1', {}), labelled('cannot write turn record t1 (ENOENT)', root)); + ensureJournal(dir); + writeTurn(dir, 't1', {}); + assert.throws(() => writeTurn(dir, 't1', {}), e => e instanceof DiscordError && e.exitCode === 1 && /turn record already exists: t1/.test(e.message)); +}); + +test('a STOP that cannot be written names STOP in the journal; the lock is released', t => { + const { root, dir } = scratch(t); + mkdirSync(stopPath(dir)); + assert.throws(() => requestStop(dir, 'cli stop'), labelled(`cannot write STOP in ${where} (EISDIR)`, root)); + assert.equal(existsSync(join(dir, '.stop.lock')), false); +}); + +test('a STOP removed between its check and its removal names STOP in the journal', t => { + const { root, dir } = scratch(t); + writeFileSync(stopPath(dir), '{}\n', { mode: 0o600 }); + race(t, 'unlinkSync', stopPath(dir), () => rmSync(stopPath(dir))); + assert.throws(() => clearStop(dir), labelled(`cannot remove STOP in ${where} (ENOENT)`, root)); +}); + +test('a run.lock that cannot be created names run.lock in the journal', t => { + const { root, dir } = scratch(t); + const gone = join(dir, 'gone'); + assert.throws(() => writePid(gone, process.pid), e => { + assert.ok(e.message.includes('cannot create run.lock in discord/gone under the data root (ENOENT)'), e.message); + quiet(e.message, root); + return true; + }); + assert.equal(existsSync(lockPath(gone)), false); +}); + +test('engine: a pi that cannot be spawned is logged and recorded as "cannot spawn pi", never its path', async t => { + const { root, dir } = scratch(t); + const { createEngine } = await import('../src/engine-pi.mjs'); + const logs = []; + let exited = null; + const engine = createEngine({ command: join(dir, 'pi'), args: [], cwd: dir, env: {}, log: (m) => logs.push(m), onExit: (e) => (exited = e) }); + engine.start(); + for (let i = 0; i < 200 && exited === null; i += 1) await new Promise((res) => setTimeout(res, 10)); + assert.ok(exited, 'the spawn error reached onExit'); + assert.equal(exited.error, 'cannot spawn pi (ENOENT)'); + assert.ok(logs.includes('engine spawn error: cannot spawn pi (ENOENT)'), logs.join('\n')); + quiet(JSON.stringify({ logs, exited }), root); +}); + +// --- the choke point --- + +test('printable: a native error prints its call and code, a refusal or a labelled error its own message', async () => { + const { printable, quietRead } = await import('../src/errors.mjs'); + assert.equal(typeof printable, 'function', 'errors.mjs exports printable'); + const path = `/x/${secret}/discord/seat/STOP`; + const native = Object.assign(new Error(`ENOENT: no such file or directory, open '${path}'`), { code: 'ENOENT', syscall: 'open', path, errno: -2 }); + assert.equal(printable(native), 'cannot open a discord file (ENOENT)'); + const pathOnly = Object.assign(new Error(`EACCES ${path}`), { code: 'EACCES', path }); + assert.equal(printable(pathOnly), 'cannot access a discord file (EACCES)'); + const spawn = Object.assign(new Error(`spawn ${path} ENOENT`), { code: 'ENOENT', syscall: `spawn ${path}`, path }); + assert.equal(printable(spawn), 'cannot spawn a discord file (ENOENT)'); + const net = Object.assign(new Error('connect ECONNREFUSED 127.0.0.1:1'), { code: 'ECONNREFUSED', syscall: 'connect' }); + assert.equal(printable(net), 'cannot connect (ECONNREFUSED)'); + const odd = Object.assign(new Error(path), { code: `X ${path}`, syscall: path, path }); + assert.equal(printable(odd), 'cannot access a discord file (UNKNOWN)'); + const labelledRead = quietRead(Object.assign(new Error(path), { code: 'ENOENT', syscall: 'open', path }), 'token file'); + assert.equal(printable(labelledRead), 'cannot read token file (ENOENT)'); + assert.equal(printable(new DiscordError('STOP is present in discord/seat under the data root; remove it to run', 3)), 'STOP is present in discord/seat under the data root; remove it to run'); + assert.equal(printable(new Error('plain refusal')), 'plain refusal'); + for (const v of [native, pathOnly, spawn, odd]) assert.doesNotMatch(printable(v), /sk-test|NOT-A-SECRET/); +}); + +test('cli: a native fs error reaching the top-level handler prints its call and code, never the path, and exits 1', t => { + const { root } = scratch(t); + const base = join(root, secret, 'dep'); + mkdirSync(base, { recursive: true }); + const repo = makeRepo(base); + const dep = makeDeployment(base); + initializeControl(dep.dataRoot); + const run = (args) => spawnSync(process.execPath, [cli, ...args, '--config', dep.config, '--repo', repo], { encoding: 'utf8' }); + + // stop: the journal directory is a regular file, so ensureJournal's mkdir fails. + writeFileSync(dep.journalDir, 'not a directory'); + let r = run(['stop', 'test-seat']); + assert.equal(r.status, 1, r.stderr); + assert.equal(r.stderr, 'discord: cannot mkdir a discord file (ENOTDIR)\n'); + quiet(r.stderr + r.stdout, root); + rmSync(dep.journalDir); + + // run: the sessions directory is a regular file, so the claimed run's mkdir fails + // after the claim; the claim is released and the exit stays 1. + writeFileSync(join(dep.dataRoot, 'sessions'), 'not a directory'); + r = run(['run', 'test-seat']); + assert.equal(r.status, 1, r.stderr); + assert.equal(r.stderr, 'discord: cannot mkdir a discord file (ENOTDIR)\n'); + quiet(r.stderr + r.stdout, root); + assert.equal(existsSync(lockPath(dep.journalDir)), false, 'the claim is released'); +}); 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 new file mode 100644 index 000000000..46a8a74e6 --- /dev/null +++ b/packages/discord/tests/path-redaction-labels.test.mjs @@ -0,0 +1,204 @@ +// #1873 (redact6): a discord refusal names the file, field or binding, never a host path. +// The sentinel is a component of every scratch path, so a refusal that quotes a path +// quotes the sentinel too; each test also checks the scratch root and the label. +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import fs, { mkdtempSync, mkdirSync, writeFileSync, rmSync } from 'node:fs'; +import { syncBuiltinESMExports } from 'node:module'; +import { readApprovals } from '../src/approvals.mjs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { DiscordError } from '../src/errors.mjs'; +import { bindingDataDir, bindingPath, loadDataRoot, readToken, resolveContextFiles, resolveToolRoots, validateBinding } from '../src/binding.mjs'; +import { ensureJournal, lockPath, ownerPath, readInboxIds, recover, requestStop, turnPath, unlock, writeTurn } from '../src/journal.mjs'; +import { gitPull, loadGitConfig } from '../src/git.mjs'; +import { loadSetsparkConfig, readKey } from '../src/setspark.mjs'; + +const secret = 'sk-test-NOT-A-SECRET-1234'; +function scratch(t) { + const root = mkdtempSync(join(tmpdir(), 'r6-')); + t.after(() => rmSync(root, { recursive: true, force: true })); + const dir = join(root, secret); + mkdirSync(dir); + return { root, dir }; +} +function labelled(label, root, Type = DiscordError) { + return error => { + assert.ok(error instanceof Type, error.stack); + assert.ok(error.message.includes(label), error.message); + assert.ok(!error.message.includes(root), error.message); + assert.doesNotMatch(error.message, /sk-test|NOT-A-SECRET/); + return true; + }; +} + +test('config not found names the config, not its path', t => { + const { root, dir } = scratch(t); + assert.throws(() => loadDataRoot(join(dir, 'config.json')), labelled('config not found', root)); +}); + +test('token file refusals name the token file, not its path', t => { + const { root, dir } = scratch(t); + assert.throws(() => readToken({ tokenFile: join(dir, 'token') }), labelled('token file not found', root)); + const short = join(dir, 'short'); + writeFileSync(short, 'x', { mode: 0o600 }); + assert.throws(() => readToken({ tokenFile: short }), labelled('token file does not hold a bot token', root)); + const open = join(dir, 'open'); + writeFileSync(open, 'x'.repeat(30), { mode: 0o644 }); + assert.throws(() => readToken({ tokenFile: open }), labelled('token file must be mode 0600, is 0644', root)); +}); + +test('a context file refusal names its index in context.files', t => { + const { root, dir } = scratch(t); + const repo = join(dir, 'repo'); + mkdirSync(repo); + assert.throws(() => resolveContextFiles({ context: { files: [join(dir, 'abs.md')] } }, repo), labelled('context file must be repository-relative: context.files[0]', root)); + assert.throws(() => resolveContextFiles({ context: { files: ['ok.md', `${secret}.md`] } }, repo), labelled('missing context file: context.files[0]', root)); +}); + +test('a tool root refusal names the root, not its path', t => { + const { root, dir } = scratch(t); + const binding = { tools: { roots: [{ name: 'docs', path: join(dir, 'docs'), write: false }] } }; + assert.throws(() => resolveToolRoots(binding, { dataRoot: join(dir, 'data') }), labelled('tool root docs does not exist', root)); + mkdirSync(join(dir, 'data', 'docs'), { recursive: true }); + binding.tools.roots[0].path = join(dir, 'data', 'docs'); + assert.throws(() => resolveToolRoots(binding, { dataRoot: join(dir, 'data') }), labelled('tool root docs overlaps the data root', root)); +}); + +test('git and setspark key file refusals name the field', t => { + const { root, dir } = scratch(t); + const git = { branch: 'main', identity: 'sage', tokenFile: join(dir, 'gitea'), author: 'Sage ' }; + assert.throws(() => loadGitConfig(git, 'tools.roots[0].git'), labelled('tools.roots[0].git.tokenFile: not found', root, Error)); + const setspark = { baseUrl: 'https://setspark.example', keyFile: join(dir, 'key'), principal: 'sage' }; + assert.throws(() => loadSetsparkConfig(setspark), labelled('setspark.keyFile: not found', root, Error)); +}); + +for (const [name, label] of [['bot', 'discord/bot under the data root'], [secret, "the binding's journal directory"]]) { + test(`journal refusals name ${label}`, t => { + const { root, dir } = scratch(t); + const journal = join(dir, name); + ensureJournal(journal); + requestStop(journal, 'operator'); + assert.throws(() => recover(journal), labelled(`STOP is present in ${label}`, root)); + rmSync(join(journal, 'STOP')); + mkdirSync(lockPath(journal)); + writeFileSync(ownerPath(journal), secret, { mode: 0o600 }); + assert.throws(() => unlock(journal), labelled(`Inspect run.lock/owner.json in ${label} by hand`, root)); + writeTurn(journal, 't1', {}); + assert.throws(() => writeTurn(journal, 't1', {}), labelled('turn record already exists: t1', root)); + }); +} + +// 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)); +}); + +// Re-check (#1873): a file removed between its check and its read surfaces a native +// error. Each site names the file by label and code; the error's path never reaches the +// message. `race` swaps one node:fs function so the target goes away right before the +// real call, which then fails with a genuine ENOENT that names the path. +function race(t, fn, target, remove) { + const original = fs[fn]; + const swapped = function (p, ...rest) { + if (String(p) === target) remove(); + return original.call(this, p, ...rest); + }; + if (original.native) swapped.native = original.native; + fs[fn] = swapped; + syncBuiltinESMExports(); + t.after(() => { + fs[fn] = original; + syncBuiltinESMExports(); + }); +} + +test('a token file removed between its check and its read names the token file', t => { + const { root, dir } = scratch(t); + const tokenFile = join(dir, 'token'); + writeFileSync(tokenFile, 'x'.repeat(30), { mode: 0o600 }); + race(t, 'readFileSync', tokenFile, () => rmSync(tokenFile)); + assert.throws(() => readToken({ tokenFile }), labelled('cannot read token file (ENOENT)', root, Error)); +}); + +test('a context file removed between its check and its realpath names its index', t => { + const { root, dir } = scratch(t); + const repo = join(dir, 'repo'); + mkdirSync(repo); + writeFileSync(join(repo, 'ok.md'), 'ok'); + writeFileSync(join(repo, `${secret}.md`), 'x'); + const target = join(repo, `${secret}.md`); + race(t, 'realpathSync', target, () => rmSync(target)); + assert.throws(() => resolveContextFiles({ context: { files: ['ok.md', `${secret}.md`] } }, repo), labelled('cannot read context file context.files[1] (ENOENT)', root, Error)); +}); + +test('a tool root removed between its check and its realpath names the root', t => { + const { root, dir } = scratch(t); + const docs = join(dir, 'docs'); + mkdirSync(docs); + const binding = { tools: { roots: [{ name: 'docs', path: docs, write: false }] } }; + race(t, 'realpathSync', docs, () => rmSync(docs, { recursive: true })); + assert.throws(() => resolveToolRoots(binding, { dataRoot: join(dir, 'data') }), labelled('cannot read tool root docs (ENOENT)', root, Error)); +}); + +test('a data root removed between its check and its realpath names the data root', t => { + const { root, dir } = scratch(t); + const docs = join(dir, 'docs'); + mkdirSync(docs); + const binding = { tools: { roots: [{ name: 'docs', path: docs, write: false }] } }; + const data = join(dir, 'data'); + mkdirSync(data); + race(t, 'realpathSync', data, () => rmSync(data, { recursive: true })); + assert.throws(() => resolveToolRoots(binding, { dataRoot: data }), labelled('cannot read the data root (ENOENT)', root, Error)); +}); + +test('a setspark key file removed between its check and its read names keyFile', t => { + const { root, dir } = scratch(t); + const keyFile = join(dir, 'key'); + writeFileSync(keyFile, 'ssk_' + 'a'.repeat(40), { mode: 0o600 }); + const config = loadSetsparkConfig({ baseUrl: 'https://setspark.example', keyFile, principal: 'sage' }); + race(t, 'readFileSync', keyFile, () => rmSync(keyFile)); + assert.throws(() => readKey(config), labelled('cannot read keyFile (ENOENT)', root, Error)); +}); diff --git a/packages/discord/tests/refusal-redaction.test.mjs b/packages/discord/tests/refusal-redaction.test.mjs index 12ad349be..415628cf6 100644 --- a/packages/discord/tests/refusal-redaction.test.mjs +++ b/packages/discord/tests/refusal-redaction.test.mjs @@ -6,10 +6,13 @@ import { dirname, join } from 'node:path'; const secret = 'sk-test-NOT-A-SECRET-1234'; // JSON.parse quotes a prefix near this fault on Node 26; check partial leaks too. const malformed = secret; +// #1873: the sentinel is a path component, so a refusal that quotes a path quotes it too. function scratch(t) { const root = mkdtempSync(join(tmpdir(), 'r3-')); t.after(() => rmSync(root, { recursive: true, force: true })); - return root; + const dir = join(root, secret); + mkdirSync(dir); + return dir; } function write(file, text = malformed) { mkdirSync(dirname(file), { recursive: true }); @@ -21,6 +24,7 @@ function safe(label, ErrorType, properties = {}) { assert.ok(error instanceof ErrorType); for (const [key, value] of Object.entries(properties)) assert.equal(error[key], value); assert.ok(error.message.includes(label), error.message); + assert.ok(!error.message.includes(tmpdir()), error.message); assert.doesNotMatch(error.message, /sk-test|Unexpected token/); assert.doesNotMatch(error.stack, /sk-test/); assert.equal(error.cause, undefined); @@ -33,15 +37,15 @@ import { readApprovals } from '../src/approvals.mjs'; import { readInboxIds } from '../src/journal.mjs'; import extension from '../extension/tools.mjs'; import { TOOLS_ENV } from '../src/tools.mjs'; -for (const [name, read, relative] of [ - ['config', (file) => loadDataRoot(file), 'config.json'], - ['binding', (file) => loadBinding(file), 'binding.json'], - ['approvals', (_file, root) => readApprovals(root), 'approvals.jsonl'], - ['journal', (_file, root) => readInboxIds(root), 'inbox.jsonl'], +for (const [name, read, relative, label] of [ + ['config', (file) => loadDataRoot(file), 'config.json', 'config is not valid JSON'], + ['binding', (file) => loadBinding(file), 'binding.json', 'binding binding is not valid JSON'], + ['approvals', (_file, root) => readApprovals(root), 'approvals.jsonl', 'approvals.jsonl:2: not valid JSON'], + ['journal', (_file, root) => readInboxIds(root), 'inbox.jsonl', 'inbox.jsonl:2: not valid JSON'], ]) test(`${name} JSON refusal omits parser bytes`, t => { const root = scratch(t), ledger = name === 'approvals' || name === 'journal'; const file = write(join(root, relative), ledger ? '{}\n' + malformed : malformed); - assert.throws(() => read(file, root), safe(ledger ? `${file}:2` : file, DiscordError, { exitCode: 2 })); + assert.throws(() => read(file, root), safe(label, DiscordError, { exitCode: 2 })); }); test('tools environment JSON refusal names the config key without its bytes', () => { const previous = process.env[TOOLS_ENV]; @@ -52,7 +56,7 @@ test('tools environment JSON refusal names the config key without its bytes', () test('config read refusal retains the filesystem code', t => { const file = scratch(t); assert.throws(() => loadDataRoot(file), error => { - safe(file, DiscordError, { exitCode: 2 })(error); + safe('config is not valid JSON', DiscordError, { exitCode: 2 })(error); assert.match(error.message, /\(EISDIR\)$/); return true; }); diff --git a/packages/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 2395f5aec..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)), "..", "..", ".."); @@ -67,21 +68,7 @@ export function runConfigCommand(argv, { stdout = process.stdout, stderr = proce maxBuffer: 1024 * 1024, env, }); - // Render known key paths with fixed constraints; never relay child stderr. - if (proc.status !== 0) { - const problem = proc.stderr.trim(); - const field = problem.startsWith('mosaic-config: execution "dataRoot"') ? "dataRoot" - : /^mosaic-config: (?:unsupported )?"?((?:configVersion|environment|dataRoot|execution|console|federation|compactionReadBytes|compactionStreams)(?:\.[A-Za-z][A-Za-z0-9]*|\[\d+\])*)"?(?=[: ])/u.exec(problem)?.[1]; - const name = "a 1-200 character name starting with a letter or digit and containing only letters, digits, . _ : / -"; - const expected = { configVersion: "1", environment: "development or production", dataRoot: "a canonical absolute instance data path", - "execution.backend": "docker", "execution.adapter": "pi or mock", "execution.provider": name, "execution.model": name }; - const detail = problem.startsWith("mosaic-config: configuration is not valid JSON (") - ? "configuration is not valid JSON; expected readable JSON: " - : field ? `${field}: expected ${expected[field] ?? "a value satisfying the system configuration schema"}: ` - : "expected valid system configuration: "; - // Name the config by where it came from, never by host path (PR #1916). - fail(3, `system config problem (mosaic-config exit ${proc.status}): ${detail}${env.MOSAIC_CONFIG ? "the system config MOSAIC_CONFIG names" : "the default system config"}${proc.error?.code ? ` (${proc.error.code})` : ""}`); - } + if (proc.status !== 0) fail(3, systemConfigProblem(proc, env)); try { return JSON.parse(proc.stdout); } catch { @@ -90,19 +77,23 @@ export function runConfigCommand(argv, { stdout = process.stdout, stderr = proce }; const load = (id) => loadOrganization(id, { dir: configDir(env), rolesDir: resolve(env.MOSAIC_ROLES_DIR || join(REPO, "roles")) }); - // Check credential references; return the problems and warnings. + // Check credential references, each a [field, ref] pair; return the problems + // and warnings. They name the field and the roots by label, never a path (#1873). const checkRefs = (refs, forbiddenRoots) => { const problems = []; const warnings = []; - for (const ref of refs) { - const result = checkCredentialRef(ref, { forbiddenRoots, env }); + for (const [where, ref] of refs) { + const result = checkCredentialRef(ref, { forbiddenRoots, env, where }); problems.push(...result.problems); warnings.push(...result.warnings); } return { problems, warnings }; }; - const forbiddenRoots = (config, organization) => - [REPO, config.dataRoot, ...Object.values(organization.projects).filter((p) => p.kind !== "library").map((p) => p.root)]; + const forbiddenRoots = (config, organization) => [ + { path: REPO, label: "the repository" }, + { path: config.dataRoot, label: "the data root" }, + ...Object.entries(organization.projects).filter(([, p]) => p.kind !== "library").map(([id, p]) => ({ path: p.root, label: `the root of project ${id}` })), + ]; const validate = (args) => { if (args.length !== 1) usage(); @@ -125,8 +116,8 @@ export function runConfigCommand(argv, { stdout = process.stdout, stderr = proce projects[id] = "valid"; } const refs = [ - ...Object.values(organization.tracker?.sync?.credentials ?? {}), - ...Object.values(organization.roles).flatMap((r) => Object.values(r.credentials)), + ...Object.entries(organization.tracker?.sync?.credentials ?? {}).map(([service, ref]) => [`tracker.sync.credentials.${service}`, ref]), + ...Object.entries(organization.roles).flatMap(([instance, r]) => Object.entries(r.credentials).map(([service, ref]) => [`roles.${instance}.credentials.${service}`, ref])), ]; const checked = checkRefs(refs, forbiddenRoots(config, organization)); warnings.push(...checked.warnings); @@ -159,11 +150,12 @@ 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 }); - const checked = checkRefs(Object.values(resolved.credentials), forbiddenRoots(config, organization)); + const checked = checkRefs(Object.entries(resolved.credentials).map(([service, ref]) => [`${instance} ${service}`, ref]), forbiddenRoots(config, organization)); for (const w of checked.warnings) say(`warning: ${w}`); if (checked.problems.length > 0) { for (const p of checked.problems) say(p); diff --git a/packages/organization/src/credentials.mjs b/packages/organization/src/credentials.mjs index 02e53e219..9958bcc80 100644 --- a/packages/organization/src/credentials.mjs +++ b/packages/organization/src/credentials.mjs @@ -44,7 +44,7 @@ export function parseCredentialRef(ref, service, where, { store = null } = {}) { } out.file = ref.file; if (store && inside(join(store.configDir, "secrets", "agents"), ref.file) && ref.file !== storePath(store, service)) { - refuse(`${where}.file credential-not-yours: ${ref.file} is in the agents' store but not this agent's path`); + refuse(`${where}.file credential-not-yours: the file is in the agents' store but not this agent's path`); } } else { if (typeof ref.env !== "string" || !ENV_NAME.test(ref.env)) refuse(`${where}.env must match ${ENV_NAME}`); @@ -68,11 +68,16 @@ function inside(root, path) { // Check a parsed reference against the filesystem and the calendar. // Returns { problems: [...], warnings: [...] }; a caller that finds any // problem refuses. `forbiddenRoots` are directories a token file must not -// sit in (the repository, dataRoot). `now` is a Date. -export function checkCredentialRef(ref, { forbiddenRoots = [], now = new Date(), env = process.env, uid = process.getuid() } = {}) { +// sit in (the repository, dataRoot): each a path, or { path, label } so a +// problem can name it. `where` is the reference's field (for example +// roles.coder.credentials.gitea). `now` is a Date. +// A problem or warning names the reference by its field, its service and its +// kind, and an env reference by its checked variable name. It never quotes the +// token file's path or a root's path (#1873). +export function checkCredentialRef(ref, { forbiddenRoots = [], now = new Date(), env = process.env, uid = process.getuid(), where = null } = {}) { const problems = []; const warnings = []; - const label = `${ref.service} ${ref.file ? `file ${ref.file}` : `env ${ref.env}`}`; + const label = `${where ?? ref.service} ${ref.store ? "store file" : ref.file ? "file" : `env ${ref.env}`}`; let notIssued = false; if (ref.file) { let stat = null; @@ -96,14 +101,16 @@ export function checkCredentialRef(ref, { forbiddenRoots = [], now = new Date(), } catch { problems.push(`${label}: path can't be resolved`); } - for (const root of forbiddenRoots) { + for (const entry of forbiddenRoots) { + const root = typeof entry === "string" ? entry : entry.path; + const name = typeof entry === "string" ? "a forbidden root" : entry.label; let realRoot = root; try { realRoot = realpathSync(root); } catch { // A root that doesn't exist yet can't contain the file. } - if (inside(realRoot, real)) problems.push(`${label}: inside ${root}; token files live outside the repository and dataRoot`); + if (inside(realRoot, real)) problems.push(`${label}: inside ${name}; token files live outside the repository and dataRoot`); } } } diff --git a/packages/organization/src/org-registry.mjs b/packages/organization/src/org-registry.mjs index 6bd757909..6ef935bec 100644 --- a/packages/organization/src/org-registry.mjs +++ b/packages/organization/src/org-registry.mjs @@ -6,34 +6,56 @@ import { processState } from '../../seat/src/process-state.mjs'; import { ID_PATTERN } from './vocabulary.mjs'; import { parseJsonFileText } from './util.mjs'; +// `path` is kept only when it is a document field ($.projects.alpha.root): a +// host path never rides on the error, where a caller could print or serialise +// it (#1873). export class OrgError extends Error { - constructor(code, message, path) { super(message); this.name = 'OrgError'; this.code = code; if (path) this.path = path; } + constructor(code, message, path) { super(message); this.name = 'OrgError'; this.code = code; if (typeof path === 'string' && path.startsWith('$')) this.path = path; } } +// A Node system error names the host path in its message, and a spawn's syscall names the +// program. quietMessage is the call's first word, the file's label and the error code, or null +// for an error that is not a system error; quiet puts it in the message. The code and syscall +// fields stay (#1873, from PR #1916's org-service quiet). +const quieted = new WeakSet(); +export function quietMessage(e, what) { + if (!e || typeof e !== 'object') return null; + if (quieted.has(e)) return e.message; + return typeof e.syscall === 'string' && typeof e.code === 'string' ? `cannot ${e.syscall.split(' ')[0]} ${what} (${e.code})` : null; +} +export function quiet(e, what) { + const message = quietMessage(e, what); + if (message !== null && !quieted.has(e)) { e.message = message; quieted.add(e); } + return e; +} +// 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; - try { doc = await readPrivate(file); } catch (e) { if (e.code === 'ENOENT') return null; if (e instanceof OrgError) throw e; fail('org-mismatch', `invalid registry: ${e.message}`, file); } + 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); } export async function listOrgs(ctx) { const doc = await readRegistry(ctx); return doc ? Object.values(doc.orgs) : []; } @@ -49,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); @@ -62,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 @@ -74,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); @@ -93,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') { @@ -108,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 }; }); } @@ -132,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] }; }); @@ -148,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 @@ -173,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) { @@ -182,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 ${guard}; remove it by hand`, guard); + if (holder.unreadable) fail('org-locked', `unreadable recovery fence org-lifecycle-${id}.lock.clear in the config directory; remove it by hand`); if (holder.alive !== false) fail('org-locked', `lock recovery is already in progress by pid ${holder.pid}`, guard); - let current; try { current = lstatSync(guard); } catch (err) { if (err.code === 'ENOENT') continue; throw err; } + 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'); } } } } @@ -201,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); @@ -210,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); } @@ -221,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/org-service.mjs b/packages/organization/src/org-service.mjs index 4b384d9b6..161ad969b 100644 --- a/packages/organization/src/org-service.mjs +++ b/packages/organization/src/org-service.mjs @@ -6,7 +6,7 @@ import { dirname, join, resolve } from 'node:path'; import { fileURLToPath } from 'node:url'; import { validateBusinessDocument, httpsUrl } from './organization-config.mjs'; import { ID_PATTERN } from './vocabulary.mjs'; -import { OrgError, orgRoot, readRegistry, reserveOrg, resolveOrg, withRegistry, withOrgLifecycle } from './org-registry.mjs'; +import { OrgError, orgRoot, quiet, readRegistry, reserveOrg, resolveOrg, withRegistry, withOrgLifecycle } from './org-registry.mjs'; import { loadInstalled } from '../../plugins/src/loader.mjs'; import { BINDING_NAME } from '../../discord/src/binding.mjs'; import { parseJsonFileText } from './util.mjs'; @@ -21,11 +21,6 @@ function authorize(ctx) { if (!ctx || typeof ctx.instance !== 'string' || !/^[a- function checkId(id) { if (typeof id !== 'string' || !ID_PATTERN.test(id)) fail('org-invalid', 'invalid id'); } // A filesystem error names the operation, the file by label and the error code, never the // host path Node puts in its message (PR #1916). code, syscall and the other fields stay. -const quieted = new WeakSet(); -function quiet(e, what) { - if (e && typeof e === 'object' && typeof e.syscall === 'string' && typeof e.code === 'string' && !quieted.has(e)) { e.message = `cannot ${e.syscall} ${what} (${e.code})`; quieted.add(e); } - return e; -} async function privateRead(file, what = 'organization file') { let fd; try { fd = await fs.open(file, constants.O_RDONLY | constants.O_NOFOLLOW | constants.O_NONBLOCK); } catch (e) { throw quiet(e, what); } try { const st = await fd.stat(); if (!st.isFile() || st.uid !== process.getuid() || (st.mode & 0o077)) fail('org-mismatch', 'unsafe file', file); return await fd.readFile('utf8'); } catch (e) { throw quiet(e, what); } finally { await fd.close(); } } async function atomic(file, content, exclusive = false) { const tmp = join(dirname(file), `.org-write-${randomBytes(12).toString('hex')}`); diff --git a/packages/organization/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 new file mode 100644 index 000000000..749ffa3b6 --- /dev/null +++ b/packages/organization/tests/path-redaction-labels.test.mjs @@ -0,0 +1,113 @@ +// #1873 (redact6): an organization refusal names the field, the file or the root by label, +// never a host path. The sentinel is a component of every scratch path, so a refusal that +// quotes a path quotes the sentinel too; each test also checks the scratch root and the label. +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { chmodSync, mkdirSync, mkdtempSync, rmSync, symlinkSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { checkCredentialRef, parseCredentialRef, storePath } from '../src/credentials.mjs'; +import { OrgError, orgLock, quietMessage, readRegistry, withRegistryWriter } from '../src/org-registry.mjs'; + +const secret = 'sk-test-NOT-A-SECRET-1234'; +function scratch(t) { + const root = mkdtempSync(join(tmpdir(), 'r6-')); + t.after(() => rmSync(root, { recursive: true, force: true })); + const dir = join(root, secret); + mkdirSync(dir, { mode: 0o700 }); + return { root, dir }; +} +function clean(text, root) { + assert.ok(!text.includes(root), text); + assert.doesNotMatch(text, /sk-test|NOT-A-SECRET/); +} + +test('a credential problem names the field and the root by label, never the token file or the root', t => { + const { root, dir } = scratch(t); + const file = join(dir, 'gitea'); + writeFileSync(file, 'token', { mode: 0o644 }); + const ref = { service: 'gitea', file, rotateBy: '2999-01-01' }; + const { problems } = checkCredentialRef(ref, { forbiddenRoots: [{ path: dir, label: 'the data root' }, root], where: 'roles.coder.credentials.gitea' }); + assert.deepEqual(problems, [ + 'roles.coder.credentials.gitea file: mode 644 gives group or other access; use 600', + 'roles.coder.credentials.gitea file: inside the data root; token files live outside the repository and dataRoot', + 'roles.coder.credentials.gitea file: inside a forbidden root; token files live outside the repository and dataRoot', + ]); + for (const p of problems) clean(p, root); + const missing = checkCredentialRef({ service: 'gitea', file: join(dir, 'absent'), rotateBy: '2999-01-01' }); + assert.deepEqual(missing.problems, ['gitea file: not found']); + const store = checkCredentialRef({ service: 'gitea', store: true, file: join(dir, 'absent'), rotateBy: '2999-01-01' }, { where: 'roles.coder.credentials.gitea' }); + assert.deepEqual(store.warnings, ['roles.coder.credentials.gitea store file: not issued']); +}); + +test('credential-not-yours names the field, not the file', t => { + const { root, dir } = scratch(t); + const store = { configDir: dir, business: 'acme', agent: 'coder' }; + const other = storePath({ ...store, agent: 'pm' }, 'gitea'); + assert.throws(() => parseCredentialRef({ file: other, rotateBy: '2999-01-01' }, 'gitea', 'roles.coder.credentials.gitea', { store }), error => { + assert.match(error.message, /^roles\.coder\.credentials\.gitea\.file credential-not-yours: the file is in the agents' store/); + clean(error.message, root); + return true; + }); +}); + +test('an OrgError keeps a document field but drops a host path', t => { + const { dir } = scratch(t); + assert.equal(new OrgError('org-mismatch', 'unsafe file', join(dir, 'org.json')).path, undefined); + assert.equal(new OrgError('org-invalid', 'bad root', '$.projects.alpha.root').path, '$.projects.alpha.root'); + const error = Object.assign(new Error(`ENOENT: no such file or directory, open '${join(dir, 'x')}'`), { code: 'ENOENT', syscall: 'open' }); + assert.equal(quietMessage(error, 'the registry file'), 'cannot open the registry file (ENOENT)'); + assert.equal(quietMessage(Object.assign(new Error('x'), { code: 'ENOENT', syscall: `spawn ${join(dir, 'git')}` }), 'git'), 'cannot spawn git (ENOENT)'); +}); + +test('an unreadable registry names the registry file and the error code, never its path', async t => { + const { root, dir } = scratch(t); + writeFileSync(join(dir, 'real.json'), '{}', { mode: 0o600 }); + symlinkSync(join(dir, 'real.json'), join(dir, 'org-runtime.json')); + await assert.rejects(readRegistry({ configDir: dir }), error => { + assert.ok(error instanceof OrgError, error.stack); + assert.equal(error.code, 'org-mismatch'); + assert.equal(error.message, 'invalid registry: cannot open registry file (ELOOP)'); + assert.equal(error.path, undefined); + clean(error.message, root); + clean(JSON.stringify(error), root); + return true; + }); +}); + +test('an unreadable recovery fence names the fence in the config directory, never its path', async t => { + const { root, dir } = scratch(t); + writeFileSync(join(dir, 'org-lifecycle-acme.lock.clear'), '{}', { mode: 0o600 }); + chmodSync(join(dir, 'org-lifecycle-acme.lock.clear'), 0o644); + await assert.rejects(orgLock({ configDir: dir }, 'acme', { clear: true }), error => { + assert.ok(error instanceof OrgError, error.stack); + assert.equal(error.code, 'org-locked'); + assert.equal(error.message, 'unreadable recovery fence org-lifecycle-acme.lock.clear in the config directory; remove it by hand'); + assert.equal(error.path, undefined); + clean(error.message, root); + return true; + }); +}); + +// 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 6549daeaf..e95ab6611 100644 --- a/packages/seat/src/seat.mjs +++ b/packages/seat/src/seat.mjs @@ -50,16 +50,17 @@ export function defaultConfigPath(env = process.env) { // Fail closed: the config must exist, parse, and name an absolute dataRoot. // Only dataRoot is read here; scripts/mosaic-config.mjs owns full validation. +// Refusals name the config, never its path (#1873). export function loadDataRoot(path = defaultConfigPath()) { - if (!existsSync(path)) throw new SeatError(`config not found: ${path}`); + if (!existsSync(path)) throw new SeatError("config not found"); let raw; try { raw = parseJsonFileText(readFileSync(path, "utf8"), path); } catch (error) { - throw new SeatError(`config is not valid JSON: ${path}${error.code ? ` (${error.code})` : ""}`); + throw new SeatError(`config is not valid JSON${error.code ? ` (${error.code})` : ""}`); } - if (!raw || typeof raw !== "object" || Array.isArray(raw)) throw new SeatError(`config is not an object: ${path}`); - if (typeof raw.dataRoot !== "string" || !isAbsolute(raw.dataRoot)) throw new SeatError(`config.dataRoot must be an absolute path: ${path}`); + if (!raw || typeof raw !== "object" || Array.isArray(raw)) throw new SeatError("config is not an object"); + if (typeof raw.dataRoot !== "string" || !isAbsolute(raw.dataRoot)) throw new SeatError("config.dataRoot must be an absolute path"); return raw.dataRoot; } @@ -69,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"); } @@ -87,10 +92,11 @@ export function resolveSeat(arg, { repo = process.cwd() } = {}) { else throw new SeatError(`invalid seat name: ${JSON.stringify(arg)}`, 4); const seat = basename(seatDir); if (!SEAT_NAME.test(seat)) throw new SeatError(`invalid seat name: ${JSON.stringify(seat)}`, 4); - if (!existsSync(seatDir) || !statSync(seatDir).isDirectory()) throw new SeatError(`no such seat directory: ${seatDir}`); + // Named by the checked seat name, never the directory's path (#1873). + if (!existsSync(seatDir) || !statSync(seatDir).isDirectory()) throw new SeatError(`no such seat directory: seat ${seat}`); const launchScript = join(seatDir, "launch.sh"); - if (!existsSync(launchScript) || !statSync(launchScript).isFile()) throw new SeatError(`seat has no launch.sh: ${seatDir}`); - if ((statSync(launchScript).mode & 0o111) === 0) throw new SeatError(`launch script is not executable: ${launchScript}`); + if (!existsSync(launchScript) || !statSync(launchScript).isFile()) throw new SeatError(`seat has no launch.sh: seat ${seat}`); + if ((statSync(launchScript).mode & 0o111) === 0) throw new SeatError(`launch script is not executable: launch.sh of seat ${seat}`); const parent = dirname(seatDir); const root = basename(parent) === "agents" ? dirname(parent) : null; @@ -141,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"); @@ -205,14 +211,14 @@ export function readRegistration(seats, seat, layout) { try { parsed = parseJsonFileText(readFileSync(path, "utf8"), path); } catch (error) { - throw new SeatError(`registration is not valid JSON: ${path}${error.code ? ` (${error.code})` : ""}`); + throw new SeatError(`registration ${layout}/${seat} is not valid JSON${error.code ? ` (${error.code})` : ""}`); } try { const record = validateRegistration(parsed); if (record.seat !== seat || record.layout !== layout) throw new SeatError("registration does not match its path"); return record; } catch (err) { - throw new SeatError(`${err.message}: ${path}`); + throw new SeatError(`${err.message}: registration ${layout}/${seat}`); } } diff --git a/packages/seat/src/session.mjs b/packages/seat/src/session.mjs index 060542a3e..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); @@ -399,9 +410,10 @@ export function readSessions(dataRoot) { try { doc = JSON.parse(readFileSync(file, "utf8")); } catch { - throw new SeatError(`sessions file is unreadable: ${file}`, 2); + throw new SeatError("sessions file is unreadable: bus-host/sessions.json under the data root", 2); } - if (!SESSIONS_READ.includes(doc?.sessionsVersion) || !Array.isArray(doc.sessions)) throw new SeatError(`sessions file is malformed: ${file}`, 2); + // #1873: named by its fixed place under the data root, never by host path. + if (!SESSIONS_READ.includes(doc?.sessionsVersion) || !Array.isArray(doc.sessions)) throw new SeatError("sessions file is malformed: bus-host/sessions.json under the data root", 2); return doc.sessions.map((s) => ({ ...s, live: startTimeOf(s.runnerPid) === s.runnerStartTime })); } diff --git a/packages/seat/tests/path-redaction-labels.test.mjs b/packages/seat/tests/path-redaction-labels.test.mjs new file mode 100644 index 000000000..21183a78b --- /dev/null +++ b/packages/seat/tests/path-redaction-labels.test.mjs @@ -0,0 +1,120 @@ +// #1873 (redact6): a seat refusal names the config, the seat or the registration by label, +// never a host path. The sentinel is a component of every scratch path, so a refusal that +// quotes a path quotes the sentinel too; each test also checks the scratch root and the label. +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { chmodSync, mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { dirname, join } from 'node:path'; +import { SeatError, loadDataRoot, readRegistration, registrationPath, resolveSeat } from '../src/seat.mjs'; +import { lockSessionWorkspace, newRun, readSessions, sessionStore, sessionWorkspace, sessionsFile, threadStore } from '../src/session.mjs'; + +const secret = 'sk-test-NOT-A-SECRET-1234'; +function scratch(t) { + const root = mkdtempSync(join(tmpdir(), 'r6-')); + t.after(() => rmSync(root, { recursive: true, force: true })); + const dir = join(root, secret); + mkdirSync(dir); + return { root, dir }; +} +function labelled(label, root) { + return error => { + assert.ok(error instanceof SeatError, error.stack); + assert.equal(error.message, label); + assert.ok(!error.message.includes(root), error.message); + assert.doesNotMatch(error.message, /sk-test|NOT-A-SECRET/); + return true; + }; +} +function write(file, text) { + mkdirSync(dirname(file), { recursive: true }); + writeFileSync(file, text); + return file; +} + +test('config refusals name the config, not its path', t => { + const { root, dir } = scratch(t); + assert.throws(() => loadDataRoot(join(dir, 'config.json')), labelled('config not found', root)); + assert.throws(() => loadDataRoot(write(join(dir, 'array.json'), '[]')), labelled('config is not an object', root)); + assert.throws(() => loadDataRoot(write(join(dir, 'relative.json'), '{"dataRoot":"data"}')), labelled('config.dataRoot must be an absolute path', root)); +}); + +test('seat directory refusals name the seat, not its directory', t => { + const { root, dir } = scratch(t); + assert.throws(() => resolveSeat(join(dir, 'agents', 'builder')), labelled('no such seat directory: seat builder', root)); + mkdirSync(join(dir, 'agents', 'builder'), { recursive: true }); + assert.throws(() => resolveSeat('builder', { repo: dir }), labelled('seat has no launch.sh: seat builder', root)); + write(join(dir, 'agents', 'builder', 'launch.sh'), '#!/bin/sh\n'); + chmodSync(join(dir, 'agents', 'builder', 'launch.sh'), 0o644); + assert.throws(() => resolveSeat('builder', { repo: dir }), labelled('launch script is not executable: launch.sh of seat builder', root)); +}); + +test('a registration that does not validate is named by its layout and seat', t => { + const { root, dir } = scratch(t); + write(registrationPath(dir, 'builder', 'repo'), '{}'); + assert.throws(() => readRegistration(dir, 'builder', 'repo'), error => { + assert.ok(error instanceof SeatError, error.stack); + assert.match(error.message, /: registration repo\/builder$/); + assert.ok(!error.message.includes(root), error.message); + assert.doesNotMatch(error.message, /sk-test|NOT-A-SECRET/); + return true; + }); +}); + +test('an unreadable or malformed sessions file is named by its place under the data root', t => { + const { root, dir } = scratch(t); + write(sessionsFile(dir), 'not json'); + assert.throws(() => readSessions(dir), labelled('sessions file is unreadable: bus-host/sessions.json under the data root', root)); + write(sessionsFile(dir), '{"sessionsVersion":0}'); + assert.throws(() => readSessions(dir), labelled('sessions file is malformed: bus-host/sessions.json under the data root', root)); +}); + +// 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/refusal-redaction.test.mjs b/packages/seat/tests/refusal-redaction.test.mjs index 74f9d96f7..9d340b7c9 100644 --- a/packages/seat/tests/refusal-redaction.test.mjs +++ b/packages/seat/tests/refusal-redaction.test.mjs @@ -6,10 +6,13 @@ import { dirname, join } from 'node:path'; const secret = 'sk-test-NOT-A-SECRET-1234'; // JSON.parse quotes a prefix near this fault on Node 26; check partial leaks too. const malformed = secret; +// #1873: the sentinel is a path component, so a refusal that quotes a path quotes it too. function scratch(t) { const root = mkdtempSync(join(tmpdir(), 'r3-')); t.after(() => rmSync(root, { recursive: true, force: true })); - return root; + const dir = join(root, secret); + mkdirSync(dir); + return dir; } function write(file, text = malformed) { mkdirSync(dirname(file), { recursive: true }); @@ -21,6 +24,7 @@ function safe(label, ErrorType, properties = {}) { assert.ok(error instanceof ErrorType); for (const [key, value] of Object.entries(properties)) assert.equal(error[key], value); assert.ok(error.message.includes(label), error.message); + assert.ok(!error.message.includes(tmpdir()), error.message); assert.doesNotMatch(error.message, /sk-test|Unexpected token/); assert.doesNotMatch(error.stack, /sk-test/); assert.equal(error.cause, undefined); @@ -30,13 +34,13 @@ function safe(label, ErrorType, properties = {}) { import { SeatError, loadDataRoot, readRegistration, registrationPath } from '../src/seat.mjs'; for (const kind of ['config', 'registration']) test(`${kind} JSON refusal names only the file`, t => { const root = scratch(t), file = write(kind === 'config' ? join(root, 'config.json') : registrationPath(root, 'builder', 'repo')); - assert.throws(() => kind === 'config' ? loadDataRoot(file) : readRegistration(root, 'builder', 'repo'), safe(file, SeatError, { exitCode: 2 })); + assert.throws(() => kind === 'config' ? loadDataRoot(file) : readRegistration(root, 'builder', 'repo'), safe(kind === 'config' ? 'config is not valid JSON' : 'registration repo/builder is not valid JSON', SeatError, { exitCode: 2 })); }); for (const kind of ['config', 'registration']) test(`${kind} read refusal retains the filesystem code`, t => { const root = scratch(t), file = kind === 'config' ? join(root, 'config.json') : registrationPath(root, 'builder', 'repo'); mkdirSync(file, { recursive: true }); assert.throws(() => kind === 'config' ? loadDataRoot(file) : readRegistration(root, 'builder', 'repo'), error => { - safe(file, SeatError, { exitCode: 2 })(error); + safe(kind === 'config' ? 'config is not valid JSON' : 'registration repo/builder is not valid JSON', SeatError, { exitCode: 2 })(error); assert.match(error.message, /\(EISDIR\)$/); return true; }); diff --git a/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 a676242a5..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; } @@ -377,13 +388,14 @@ async function load(file, instance = null) { const raw = readRaw(file); if (raw === null) { const bootstrap = isNamed(instance) ? `scripts/bootstrap.sh --instance ${instance}` : "scripts/bootstrap.sh"; - fail(3, `configuration not found: ${file} (run ${bootstrap} to create it)`); + // #1873: named by where it came from, never by host path. + 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); } @@ -409,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); @@ -417,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); } @@ -452,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; @@ -477,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`); } @@ -531,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) {