From 1947a056fbce10b451b191c0809ff8a0ffe16475 Mon Sep 17 00:00:00 2001 From: Jason Woltje Date: Wed, 11 Nov 2026 13:05:27 -0600 Subject: [PATCH] fix(console): D1 review fixes for the detached dev-owner supervisor (#1873) Build review P2: the supervisor ignores a lost reply pipe and keeps supervising. Security P2: a stop signal during start is held until the start settles, and the CLI's start timeout signals the supervisor's process group. Security P1 (part): console start --dev-local-owner refuses inside an agent run. Tests cover review, confirm and reverse on the org autonomy route; TOOLS.md and the webui README are corrected. --- docs/TOOLS.md | 2 +- packages/cli/src/cli.mjs | 2 + packages/cli/src/console-supervisor.mjs | 11 ++- packages/cli/src/org-console.mjs | 15 +++- packages/cli/tests/org-console.test.mjs | 87 ++++++++++++++++++- packages/webui/README.md | 2 +- .../webui/tests/org-autonomy-write.test.mjs | 8 ++ 7 files changed, 121 insertions(+), 6 deletions(-) diff --git a/docs/TOOLS.md b/docs/TOOLS.md index 0da5cff29..930931dde 100644 --- a/docs/TOOLS.md +++ b/docs/TOOLS.md @@ -333,7 +333,7 @@ For local development on a named instance only, `mosaic bus start BUSINESS --dev `mosaic console start --dev-local-owner --dev-insecure-bind ADDR --dev-allow-host NAME[,NAME...]` opens that dev owner Console to other machines, for UI testing from another device on a trusted LAN. It has no sign-in: anyone who can reach ADDR:PORT acts as the owner. Start, and `mosaic console status` while that Console runs, print `WARNING: dev owner Console listens on ADDR:PORT with no sign-in; anyone who can reach it acts as the owner.` on stderr. ADDR is `0.0.0.0` or an IPv4 address, and a specific address must also be one of the names. NAME is a host name or IPv4 address without a port; give several as one comma-separated value. The Console answers only `NAME:` and the loopback names, and takes a write only when Origin is `http://` plus that same Host. A browser on such an http origin sends no Fetch Metadata, so for an allowed NAME a write with that exact Origin and no `Sec-Fetch-Site` counts as same-origin; a `Sec-Fetch-Site` the browser did send is still checked. Both flags are refused without `--dev-local-owner`, and `--dev-insecure-bind` is refused without a `--dev-allow-host`. SSO, preview and tester Consoles stay on loopback. The Console state file records `bind` and `allowHosts`. `mosaic console status` prints the state as JSON (`bind` is `127.0.0.1` for a loopback Console) and never moves a stale state file. -`mosaic console restart` and `mosaic org reload ORG` are the Console's Settings controls (F3) from a terminal (U1, #1873). Each sends one request to the running Console named in the instance's console state, as the page sends it (the Console's own Host and Origin, `Sec-Fetch-Site: same-origin`, a JSON `{}` body), so the Console applies the same owner, `--send` and origin checks and writes the same action trail, `/console/actions.jsonl`. Both refuse inside an agent run (exit 3) before reading anything, and refuse with exit 3 when no Console runs. `console restart` works only for a Console started in the foreground by `mosaic console start`, or by `mosaic console start --dev-local-owner --detach`, which leaves a detached supervisor (`packages/cli/src/console-supervisor.mjs`, its own session) running the Console as the foreground start does; its receipt adds the supervisor's `supervisor` pid, and its later warnings go to `console/supervisor-*.log` under the data root. Any other Console answers `not-supervised`; it waits up to 30 seconds for the relaunched Console to answer and prints `{"outcome":"restarted","from":OLD,"boot":NEW}`. `org reload ORG` restarts a running bus or starts a stopped one and prints the Console's done receipt. A request that left but got no answer is reported as outcome unknown (exit 1) and is never retried; check `mosaic console status`, `mosaic bus status ORG` and the trail. `mosaic bus restart ORG` remains the trail-less host verb. +`mosaic console restart` and `mosaic org reload ORG` are the Console's Settings controls (F3) from a terminal (U1, #1873). Each sends one request to the running Console named in the instance's console state, as the page sends it (the Console's own Host and Origin, `Sec-Fetch-Site: same-origin`, a JSON `{}` body), so the Console applies the same owner, `--send` and origin checks and writes the same action trail, `/console/actions.jsonl`. Both refuse inside an agent run (exit 3) before reading anything, and refuse with exit 3 when no Console runs. `console restart` works only for a Console started in the foreground by `mosaic console start`, or by `mosaic console start --dev-local-owner --detach`, which leaves a detached supervisor (`packages/cli/src/console-supervisor.mjs`, its own session) running the Console as the foreground start does; its receipt adds the supervisor's `supervisor` pid, and its later warnings go to `console/supervisor-*.log` under the data root. `mosaic console stop` stops that Console, and its supervisor then exits. `mosaic console start --dev-local-owner` refuses inside an agent run (exit 3), with or without `--detach`; a start without `--dev-local-owner` is unchanged. Any other Console answers `not-supervised`. `console restart` waits up to 30 seconds for the relaunched Console to answer and prints `{"outcome":"restarted","from":OLD,"boot":NEW}`. `org reload ORG` restarts a running bus or starts a stopped one and prints the Console's done receipt. A request that left but got no answer is reported as outcome unknown (exit 1) and is never retried; check `mosaic console status`, `mosaic bus status ORG` and the trail. `mosaic bus restart ORG` remains the trail-less host verb. The instance processing switch is separate from per-business launches off/on. `mosaic halt` durably stops managed processing across every business in the diff --git a/packages/cli/src/cli.mjs b/packages/cli/src/cli.mjs index 56d454236..b167888bb 100644 --- a/packages/cli/src/cli.mjs +++ b/packages/cli/src/cli.mjs @@ -595,6 +595,8 @@ async function consoleCommand(rest, io, deps) { // org-lan-bind: refused here, before any config is read, unless both flags come with // --dev-local-owner. ADDR is 0.0.0.0 or an IPv4 address; NAME[,NAME...] has no ports. const devOwner = parsed.flags.has("--dev-local-owner"), bind = parsed.values["--dev-insecure-bind"] ?? null; + // D1: an agent run never starts a dev-local-owner Console (org-console.mjs consoleStart checks again). + if (devOwner) refuseInsideAgent(io.env); const names = Object.hasOwn(parsed.values, "--dev-allow-host") ? [parsed.values["--dev-allow-host"]] : []; let lan; try { lan = devLanOptions({ devOwner, bind, names }); } catch (err) { throw usage(err.message); } diff --git a/packages/cli/src/console-supervisor.mjs b/packages/cli/src/console-supervisor.mjs index b21935fbb..c94323b08 100644 --- a/packages/cli/src/console-supervisor.mjs +++ b/packages/cli/src/console-supervisor.mjs @@ -10,7 +10,15 @@ const { configDir, dataRoot, instance, port, bind, allowHosts } = JSON.parse(pro const warnings = []; let started = false; const warn = s => { if (started) process.stderr.write(s); else warnings.push(s); }; +// The starting CLI can be gone before the reply (Ctrl-C, a wrapper's timeout). The Console is +// started and its state names it, so a reply nobody reads is dropped and supervision goes on. +process.stdout.on('error', () => {}); const reply = value => new Promise(resolve => process.stdout.write(`${JSON.stringify(value)}\n`, () => resolve())); +// A stop signal during the start (the CLI's start timeout signals this process group) is kept and +// acted on once the start settles, so a failed start still releases the start lock. +let stopAsked = false; +const early = () => { stopAsked = true; }; +for (const signal of ['SIGINT', 'SIGTERM']) process.on(signal, early); let receipt; try { receipt = await consoleStart({ configDir, dataRoot, instance, env: process.env }, { devLocalOwner: true, port, bind, allowHosts, warn }); @@ -23,7 +31,8 @@ try { started = true; // A stop signal ends supervision cleanly, as in the foreground (cli.mjs consoleCommand). const stop = () => receipt.stop(); -for (const signal of ['SIGINT', 'SIGTERM']) process.on(signal, stop); +for (const signal of ['SIGINT', 'SIGTERM']) { process.off(signal, early); process.on(signal, stop); } +if (stopAsked) stop(); await reply({ receipt, warnings }); const code = await receipt.done; if (code !== 0) process.stderr.write(`mosaic: console stopped unexpectedly (${code})\n`); diff --git a/packages/cli/src/org-console.mjs b/packages/cli/src/org-console.mjs index 6c0c2b53c..a9d56afa5 100644 --- a/packages/cli/src/org-console.mjs +++ b/packages/cli/src/org-console.mjs @@ -10,6 +10,7 @@ import { listOrgs, resolveOrg } from '../../organization/src/org-registry.mjs'; import { cmdlineOf, startTimeOf } from './host.mjs'; import { CliError } from './errors.mjs'; import { devLanOptions } from './dev-local-owner.mjs'; +import { refuseInsideAgent } from './transport.mjs'; const WEBUI = fileURLToPath(new URL('../../webui/src/cli.mjs', import.meta.url)); const forbidden = new Set([7332, 7340, 7341, 33223, 17450, 17443, 44307]); @@ -182,7 +183,16 @@ async function detachedSupervisor(ctx, { port, bind, allowHosts, warn }) { ]); clearTimeout(timer); child.stdout.destroy(); - if (line === undefined) { child.kill('SIGTERM'); await done; fail(unclear); } + // The supervisor leads its own process group, with the Console it may have started: a timed-out + // start signals that group, then kills it if it is still there after 10 seconds. + if (line === undefined) { + const group = signal => { try { process.kill(-child.pid, signal); } catch { /* already gone */ } }; + group('SIGTERM'); + let grace; const exited = await Promise.race([done.then(() => true), new Promise(r => { grace = setTimeout(() => r(false), 10000); })]); + clearTimeout(grace); + if (!exited) { group('SIGKILL'); await done; } + fail(unclear); + } let reply = null; try { reply = JSON.parse(line); } catch { /* no reply */ } for (const w of Array.isArray(reply?.warnings) ? reply.warnings : []) if (typeof w === 'string') warn(w); @@ -206,6 +216,9 @@ export async function consoleStart(ctx, { devLocalOwner = false, detach = false, if (!(port === 'auto' || (typeof port === 'string' && /^\d+$/.test(port) && value >= 1 && value <= 65535 && !forbidden.has(value)))) fail('invalid or forbidden console port'); let lan; try { lan = devLanOptions({ devOwner: devLocalOwner, bind, names: allowHosts }); } catch (e) { throw new CliError(e.message, 4); } + // D1 fix round (security review P1): a dev-local-owner Console acts as the owner with no sign-in, + // so an agent run never starts one, detached or not. Other starts are unchanged. + if (devLocalOwner) refuseInsideAgent(ctx.env ?? process.env); if (detach && devLocalOwner) return detachedSupervisor(ctx, { port, bind, allowHosts, warn }); const release = takeLock(ctx); let run, args; diff --git a/packages/cli/tests/org-console.test.mjs b/packages/cli/tests/org-console.test.mjs index f2d5af1e1..089226c90 100644 --- a/packages/cli/tests/org-console.test.mjs +++ b/packages/cli/tests/org-console.test.mjs @@ -1,6 +1,6 @@ import { test } from 'node:test'; import assert from 'node:assert/strict'; -import { spawnSync } from 'node:child_process'; +import { spawn, spawnSync } from 'node:child_process'; import { mkdtempSync, mkdirSync, writeFileSync, readdirSync, readFileSync, rmSync, existsSync, statSync } from 'node:fs'; import { get, request } from 'node:http'; import { join } from 'node:path'; @@ -237,7 +237,8 @@ test('a detached dev-owner console runs under a detached supervisor, so the owne // The page's request, as `mosaic console restart` sends it: 202, then a new boot on the same address. const restarted = await consoleRestart(ctx); assert.equal(restarted.outcome, 'restarted'); assert.equal(restarted.from, before.boot); assert.notEqual(restarted.boot, before.boot); - const saved = JSON.parse(readFileSync(stateFile, 'utf8')); + // The relaunched Console answers before the supervisor writes its state (relaunch), so wait for it. + const saved = await relaunchedState(ctx, result.pid); assert.notEqual(saved.pid, result.pid); assert.equal(saved.url, result.url); assert.equal(Number(statOf(saved.pid)[1]), result.supervisor); @@ -272,6 +273,88 @@ test('a supervisor stopped by a signal stops its detached dev-owner console', { assert.equal(consoleStatus(ctx).running, false); }); +// relaunch() writes console.json after the new Console answers; read it once it names a new pid. +async function relaunchedState(ctx, oldPid) { + const file = join(ctx.dataRoot, 'console/console.json'); + for (const deadline = Date.now() + 15000; ; await new Promise(r => setTimeout(r, 50))) { + const saved = JSON.parse(readFileSync(file, 'utf8')); + if (saved.pid !== oldPid || Date.now() > deadline) return saved; + } +} + +// D1 fix round (build review P2): the starting CLI can die before it reads the supervisor's reply +// (Ctrl-C, a wrapper's timeout). The reply is dropped, and the supervisor keeps supervising. +test('a supervisor whose starting CLI is gone keeps supervising, so Restart Console still relaunches', { timeout: 60000 }, async t => { + const { ctx } = await instance(t); + const child = spawn(process.execPath, [SUPERVISOR, JSON.stringify({ configDir: ctx.configDir, dataRoot: ctx.dataRoot, instance: 's1test', port: 'auto', bind: null, allowHosts: [] })], + { env: ctx.env, detached: true, stdio: ['ignore', 'pipe', 'pipe'] }); + t.after(() => { try { process.kill(child.pid, 'SIGTERM'); } catch {} }); + const exit = new Promise(resolve => child.once('exit', code => resolve(code))); + let stderr = ''; child.stderr.setEncoding('utf8').on('data', c => { stderr += c; }); + child.stdout.destroy(); + let status = consoleStatus(ctx); + for (const deadline = Date.now() + 30000; !status.running; status = consoleStatus(ctx)) { + if (Date.now() > deadline) assert.fail(`the console did not start: ${stderr}`); + await new Promise(r => setTimeout(r, 100)); + } + // The reply follows the state write; give it time to meet the closed pipe. + await new Promise(r => setTimeout(r, 1000)); + assert.equal(child.exitCode, null, stderr); + assert.equal(Number(statOf(status.pid)[1]), child.pid); + const restarted = await consoleRestart(ctx); + assert.equal(restarted.outcome, 'restarted'); + const after = await relaunchedState(ctx, status.pid); + assert.notEqual(after.pid, status.pid); assert.equal(after.url, status.url); + assert.equal(Number(statOf(after.pid)[1]), child.pid); + assert.deepEqual(await consoleStop(ctx), { stopped: true, pid: after.pid }); + assert.equal(await exit, 0); + assert.doesNotMatch(stderr, /EPIPE|Unhandled/); +}); + +// D1 fix round (security review P2): a stop signal while the supervisor holds the start lock is kept +// until the start settles; then the Console stops, the lock is released and the supervisor exits. +test('a supervisor signalled during its start stops the console it started and releases the start lock', { timeout: 60000 }, async t => { + const { ctx } = await instance(t); + const child = spawn(process.execPath, [SUPERVISOR, JSON.stringify({ configDir: ctx.configDir, dataRoot: ctx.dataRoot, instance: 's1test', port: 'auto', bind: null, allowHosts: [] })], + { env: ctx.env, detached: true, stdio: ['ignore', 'pipe', 'pipe'] }); + t.after(() => { try { process.kill(-child.pid, 'SIGKILL'); } catch {} }); + const exit = new Promise(resolve => child.once('exit', (code, signal) => resolve({ code, signal }))); + let stdout = '', stderr = ''; + child.stdout.setEncoding('utf8').on('data', c => { stdout += c; }); + child.stderr.setEncoding('utf8').on('data', c => { stderr += c; }); + const lock = join(ctx.dataRoot, 'console/start.lock'); + for (const deadline = Date.now() + 20000; !existsSync(lock); await new Promise(r => setTimeout(r, 2))) { + if (Date.now() > deadline) assert.fail(`the supervisor never took the start lock: ${stderr}`); + } + process.kill(child.pid, 'SIGTERM'); + assert.deepEqual(await exit, { code: 0, signal: null }, stderr); + assert.equal(existsSync(lock), false); + // As after a foreground stop by signal, the state file stays and reads stale, not running. + assert.equal(consoleStatus(ctx).running, false); + // The started Console was this supervisor's child, and it is gone. + const reply = JSON.parse(stdout.trim().split('\n')[0]); + assert.equal(existsSync(`/proc/${reply.receipt.pid}`), false); +}); + +// D1 fix round (security review P1): an agent run never starts a dev-local-owner Console. +test('an agent run cannot start a dev-local-owner console; a start without it is unchanged', { timeout: 60000 }, async t => { + const { ctx, mosaic } = await instance(t); + const agent = { ...ctx, env: { ...ctx.env, MOSAIC_RUN_ID: 'run-1' } }; + for (const detach of [true, false]) { + await assert.rejects(consoleStart(agent, { devLocalOwner: true, detach, port: 'auto', warn: () => {} }), e => e.exitCode === 3 && /only from a human shell/.test(e.message)); + } + const agentCli = args => spawnSync(join(REPO, 'scripts/mosaic'), args, { env: { ...ctx.env, MOSAIC_AGENT_RUN: '1' }, encoding: 'utf8', timeout: 30000 }); + const refused = agentCli(['console', 'start', '--dev-local-owner', '--detach', '--port', 'auto']); + assert.equal(refused.status, 3); assert.equal(refused.stdout, ''); assert.match(refused.stderr, /only from a human shell/); + const consoleDir = join(ctx.dataRoot, 'console'); + assert.deepEqual(existsSync(consoleDir) ? readdirSync(consoleDir) : [], []); + assert.deepEqual(consoleStatus(ctx), { running: false }); + const plain = agentCli(['console', 'start', '--detach', '--port', 'auto']); + assert.equal(plain.status, 0, plain.stderr); + assert.deepEqual(Object.keys(JSON.parse(plain.stdout)), ['url', 'pid']); + assert.equal((await consoleStop(ctx)).stopped, true); +}); + // Without --dev-local-owner a detached start is what it was before D1: no supervisor, restart refused. test('a detached console without the dev owner has no supervisor, so it refuses a restart and stays up', { timeout: 60000 }, async t => { const { ctx, mosaic } = await instance(t); diff --git a/packages/webui/README.md b/packages/webui/README.md index 086d0fcf0..4517aa3a9 100644 --- a/packages/webui/README.md +++ b/packages/webui/README.md @@ -22,7 +22,7 @@ baseline CLI. | Start, stop or restart a bus | `mosaic bus start ` / `mosaic bus stop ` / `mosaic bus restart ` | S6 integration hook | | Restart (or first start) an organization's bus | `mosaic bus restart `, or `mosaic bus start ` for a bus never started | Enabled (F3): the owner's **Restart bus** control, offered where a page says the bus is down or a change needs a restart; trailed | | Restart the Console | Stop and run `mosaic console start` again | Enabled (F3) when `mosaic console start` runs it in the foreground, or with `--dev-local-owner --detach` under a detached supervisor (D1): Settings > Console; the owner only; trailed. Other Consoles answer `not-supervised` | -| Save, review, confirm or reverse an organization's autonomy | None; console verbs `autonomy.set.console`, `autonomy.review.console`, `decision.confirm.console`, `decision.reverse.console` | Enabled (D1) on a `--dev-local-owner` Console: the owner only, on the selected organization's bus, behind a fence that passes those four verbs and nothing else | +| Save, review, confirm or reverse an organization's autonomy | Save only: edit the autonomy block and run `mosaic org update ORG --from FILE --if-revision N`; no autonomy verb yet (readiness P1-3) | Enabled (D1) on a `--dev-local-owner` Console: the owner only, on the selected organization's bus, behind a fence that passes only the four console verbs `autonomy.set.console`, `autonomy.review.console`, `decision.confirm.console` and `decision.reverse.console` | | Create chat project linked to a configured repository | `mosaic project create --repo --org ` | Enabled | | Create a thread | `mosaic thread create --org ` | Planned enabled | | Open a thread | `mosaic thread open --org ` | Planned enabled | diff --git a/packages/webui/tests/org-autonomy-write.test.mjs b/packages/webui/tests/org-autonomy-write.test.mjs index 8bc74932a..2374e0248 100644 --- a/packages/webui/tests/org-autonomy-write.test.mjs +++ b/packages/webui/tests/org-autonomy-write.test.mjs @@ -42,6 +42,7 @@ export const consoleCall = o => async (verb, args, { subject }) => { record('console', o, verb, args, { subject }); if (verb === 'autonomy.get.console') return { revision: 3, autonomy: { default: 'in' } }; if (verb === 'autonomy.set.console') return { revision: 4, autonomy: args.autonomy, receipt: { action: 'autonomy.set', revision: 4 } }; + if (['autonomy.review.console', 'decision.confirm.console', 'decision.reverse.console'].includes(verb)) return { answered: verb }; throw new BusReadError('unavailable', 'fixture: ' + verb); }; `); @@ -109,6 +110,13 @@ test('the dev-local owner reads an organization autonomy page as writable and a { business: 'alpha', socket, verb: 'autonomy.get.console', args: {}, subject }, { business: 'alpha', socket, verb: 'autonomy.set.console', args: AUTONOMY, subject }, ]); + // Review, confirm and reverse go to the same organization bus as the owner (D1 fix round). + for (const [step, body, verb] of [['review', { decisionClass: 'merge' }, 'autonomy.review.console'], ['confirm', { id: 'd-1', confirm: true, reason: 'checked' }, 'decision.confirm.console'], ['reverse', { id: 'd-1' }, 'decision.reverse.console']]) { + const r = await call(port, `/api/orgs/alpha/autonomy/${step}`, { method: 'POST', body: JSON.stringify(body) }); + assert.equal(r.status, 200, `${step}: ${JSON.stringify(r.body)}`); + assert.deepEqual(r.body, { answered: verb, writable: true }); + assert.deepEqual(calls().filter(c => c.kind === 'console').at(-1), { kind: 'console', business: 'alpha', socket, verb, args: body, subject }); + } // The write keeps the route's own checks: another site saves nothing. const before = calls().length; for (const headers of [{ origin: 'http://evil.example' }, { 'sec-fetch-site': 'cross-site' }]) {