From 118342f71a32c7d903382351d693fed98d953bc4 Mon Sep 17 00:00:00 2001 From: NNIIOVO Date: Wed, 4 Nov 2026 11:19:44 -0600 Subject: [PATCH 1/4] fix: close reviewed agent key, focus and Reader safety gaps --- companion/isolation/write-agent-key.mjs | 46 +++++++-- companion/test/write-agent-key.test.mjs | 62 +++++++++++- docs/guide.md | 10 +- scripts/build-reader.sh | 10 +- scripts/build-reader.test.mjs | 123 ++++++++++++++++++++++++ scripts/check.sh | 3 + src-tauri/src/commands.rs | 117 +++++++++++++++++++++- src-tauri/src/lib.rs | 4 +- 8 files changed, 352 insertions(+), 23 deletions(-) create mode 100644 scripts/build-reader.test.mjs diff --git a/companion/isolation/write-agent-key.mjs b/companion/isolation/write-agent-key.mjs index b505b64..0c288dd 100644 --- a/companion/isolation/write-agent-key.mjs +++ b/companion/isolation/write-agent-key.mjs @@ -7,11 +7,12 @@ // written, so a password or Seed Phrase left on the clipboard can't land in the file. Anthropic // (sk-ant-) and OpenRouter (sk-or-) keys are refused too, but an OpenAI key copied from the Vault // would still pass. No message ever includes the value. -import { open } from 'node:fs/promises'; +import { lstat, open, rename, unlink } from 'node:fs/promises'; import { constants, realpathSync } from 'node:fs'; +import { randomUUID } from 'node:crypto'; import path from 'node:path'; import { pathToFileURL } from 'node:url'; -import { run, parseConfirmedCredential, checkAcl } from './common.mjs'; +import { run, parseConfirmedCredential, checkAcl, assertProtectedPath } from './common.mjs'; const MAX_INPUT_BYTES = 4096; export const KEY_REFUSED = "That doesn't look like an OpenAI API key (one line starting with sk-, no spaces), " + @@ -48,25 +49,54 @@ export async function writeAgentKey(file, input, { exclude = excludeFromBackup, const contents = Buffer.from('OPENAI_AGENT_API_KEY=' + value + '\n', 'utf8'); try { parseConfirmedCredential(contents.toString('utf8')); // the same check Start makes - const handle = await open(file, constants.O_WRONLY | constants.O_CREAT | constants.O_NOFOLLOW, 0o600) + file = path.resolve(file); + const parent = path.dirname(file); + const protectParent = () => assertProtectedPath(parent, process.getuid()) .catch(() => { throw new Error(FILE_REFUSED); }); + const inspectDestination = async () => { + let info; + try { info = await lstat(file); } + catch (error) { if (error.code === 'ENOENT') return null; throw new Error(FILE_REFUSED); } + if (!info.isFile() || info.uid !== process.getuid() || info.nlink !== 1) throw new Error(FILE_REFUSED); + await aclCheck(file).catch(() => { throw new Error(FILE_REFUSED); }); + return info; + }; + await protectParent(); + const before = await inspectDestination(); + const temporary = path.join(parent, '.enote-agent-key-' + randomUUID()); + const handle = await open(temporary, constants.O_WRONLY | constants.O_CREAT | constants.O_EXCL | constants.O_NOFOLLOW, 0o600) + .catch(() => { throw new Error(FILE_REFUSED); }); + let published = false; try { const info = await handle.stat(); if (!info.isFile() || info.uid !== process.getuid() || info.nlink !== 1) throw new Error(FILE_REFUSED); // An allow entry, set on the file or inherited from its folder, lets another account read it // whatever its mode. - await aclCheck(file).catch(() => { throw new Error(FILE_REFUSED); }); + await aclCheck(temporary).catch(() => { throw new Error(FILE_REFUSED); }); await handle.chmod(0o600); - await handle.truncate(0); - await handle.write(contents, 0, contents.length, 0); + await handle.writeFile(contents); await handle.sync(); // Checked again before success is reported; if one appeared, the key is taken out again. - await aclCheck(file).catch(async () => { + await aclCheck(temporary).catch(async () => { await handle.truncate(0); await handle.sync(); throw new Error(ACL_APPEARED); }); - } finally { await handle.close(); } + await protectParent(); + const after = await inspectDestination(); + if (before?.dev !== after?.dev || before?.ino !== after?.ino || + before?.mtimeMs !== after?.mtimeMs || before?.ctimeMs !== after?.ctimeMs) throw new Error(FILE_REFUSED); + // A fresh inode keeps an earlier reader's open descriptor away from the new key. + await rename(temporary, file); + published = true; + } finally { + if (!published) { + await handle.truncate(0).catch(() => {}); + await handle.sync().catch(() => {}); + } + try { await handle.close(); } + finally { if (!published) await unlink(temporary).catch(() => {}); } + } } finally { contents.fill(0); } return { excluded: await exclude(file) }; } diff --git a/companion/test/write-agent-key.test.mjs b/companion/test/write-agent-key.test.mjs index 788aa90..9a1eb2a 100644 --- a/companion/test/write-agent-key.test.mjs +++ b/companion/test/write-agent-key.test.mjs @@ -59,12 +59,27 @@ test('writes an owner-only file that Start accepts, and asks Time Machine to ski assert.deepEqual(excluded, FAKE_KEYS.map(() => file)); // A failed exclusion still leaves the written file, and the owner is told. assert.deepEqual(await writeAgentKey(file, Buffer.from(FAKE_KEYS[0]), { exclude: async () => false }), { excluded: false }); - // An existing file with looser permissions is tightened before the key goes in. + // An existing file with looser permissions is replaced by a fresh owner-only file. await fs.chmod(file, 0o644); await writeAgentKey(file, Buffer.from(FAKE_KEYS[1]), { exclude: async () => true }); assert.equal((await fs.stat(file)).mode & 0o777, 0o600); }); +test('replacing a readable file never gives its existing readers the new key', async t => { + const directory = await scratch(t); + const file = path.join(directory, 'agent-key.env'); + const earlier = 'FAKE_EARLIER_CONTENT\n'; + await fs.writeFile(file, earlier, { mode: 0o644 }); + const reader = await fs.open(file, 'r'); + t.after(() => reader.close()); + const before = await reader.stat(); + await writeAgentKey(file, Buffer.from(FAKE_KEYS[0]), { exclude: async () => true }); + assert.equal(await reader.readFile('utf8'), earlier); + assert.notEqual((await fs.stat(file)).ino, before.ino); + assert.equal(await readConfirmedCredential(file, process.getuid()), FAKE_KEYS[0]); + assert.deepEqual(await fs.readdir(directory), ['agent-key.env']); +}); + test('refuses a value or a file before writing anything', async t => { const directory = await scratch(t); const file = path.join(directory, 'agent-key.env'); @@ -105,18 +120,55 @@ test('refuses a file an ACL opens to another account, set on it or inherited fro assert.equal(spawnSync('/bin/chmod', ['+a', 'user:nobody allow read,file_inherit', folder]).status, 0); const fresh = path.join(folder, 'agent-key.env'); await assert.rejects(writeAgentKey(fresh, Buffer.from(FAKE_KEYS[0]), { exclude }), { message: FILE_REFUSED }); - assert.equal((await fs.stat(fresh)).size, 0); + await assert.rejects(fs.stat(fresh), { code: 'ENOENT' }); + assert.deepEqual(await fs.readdir(folder), []); }); -test('an ACL found after writing takes the key out again, and nothing is reported as written', async t => { +test('an ACL found after writing removes the temporary key and keeps the earlier file', async t => { const directory = await scratch(t); const file = path.join(directory, 'agent-key.env'); + await fs.writeFile(file, 'FAKE_EARLIER_CONTENT\n', { mode: 0o600 }); let checks = 0; - const aclCheck = async () => { if (++checks === 2) throw new Error('FAKE ACL allow entry'); }; + const aclCheck = async checked => { + if (checked !== file && ++checks === 2) throw new Error('FAKE ACL allow entry'); + }; await assert.rejects(writeAgentKey(file, Buffer.from(FAKE_KEYS[0]), { exclude: async () => true, aclCheck }), { message: ACL_APPEARED }); assert.equal(checks, 2); - assert.equal((await fs.stat(file)).size, 0); + assert.equal(await fs.readFile(file, 'utf8'), 'FAKE_EARLIER_CONTENT\n'); + assert.deepEqual(await fs.readdir(directory), ['agent-key.env']); +}); + +test('a destination changed while preparing the key is never overwritten', async t => { + const directory = await scratch(t); + const file = path.join(directory, 'agent-key.env'); + await fs.writeFile(file, 'FAKE_EARLIER_CONTENT\n', { mode: 0o600 }); + let checks = 0; + const aclCheck = async checked => { + if (checked !== file && ++checks === 2) { + await fs.rename(file, path.join(directory, 'earlier.env')); + await fs.writeFile(file, 'FAKE_REPLACEMENT_CONTENT\n', { mode: 0o600 }); + } + }; + await assert.rejects(writeAgentKey(file, Buffer.from(FAKE_KEYS[0]), { exclude: async () => true, aclCheck }), + { message: FILE_REFUSED }); + assert.equal(await fs.readFile(file, 'utf8'), 'FAKE_REPLACEMENT_CONTENT\n'); + assert.equal(await fs.readFile(path.join(directory, 'earlier.env'), 'utf8'), 'FAKE_EARLIER_CONTENT\n'); + assert.deepEqual((await fs.readdir(directory)).sort(), ['agent-key.env', 'earlier.env']); +}); + +test('unprotected or linked parent folders are refused before a key file is created', async t => { + const directory = await scratch(t); + const writable = path.join(directory, 'writable'); + await fs.mkdir(writable); + await fs.chmod(writable, 0o777); + const linked = path.join(directory, 'linked'); + await fs.symlink(writable, linked); + for (const parent of [writable, linked]) { + await assert.rejects(writeAgentKey(path.join(parent, 'agent-key.env'), Buffer.from(FAKE_KEYS[0]), + { exclude: async () => true }), { message: FILE_REFUSED }); + } + assert.deepEqual(await fs.readdir(writable), []); }); test('input past the bound is refused as too long, even when the first read alone is key-shaped', async t => { diff --git a/docs/guide.md b/docs/guide.md index b4c57f5..58ee13b 100644 --- a/docs/guide.md +++ b/docs/guide.md @@ -79,9 +79,15 @@ Keys in your Vault are for you. The app gives agents no way to use them, and an 3. Keep it in a private file outside any repository, in the account where the agent runs. Never put it in a project's `.env` file, on a shared drive or an exFAT drive (exFAT has no owner-only permissions), or in a chat. On a Mac, copy the new key at the service, run this, then copy something else so the key leaves the clipboard. It leaves nothing in shell history: ```bash - ( umask 077; printf 'SERVICE_API_KEY=%s\n' "$(pbpaste)" > ~/agent-job-key.env ) + ( + umask 077 + key_file=$(mktemp "$HOME/agent-job-key.XXXXXX") || exit 1 + printf 'SERVICE_API_KEY=%s\n' "$(pbpaste)" > "$key_file" && printf 'Saved key file: %s\n' "$key_file" + ) ``` + This creates a fresh private file with a different name each time; it never overwrites an earlier key file or follows an existing link. Use the file name it prints when setting up your agent tool. + If the agent runs in a separate account, how to get the key into that account is still open ([Order and account use](agent-companion.md#order-and-account-use)). 4. Treat the key as already seen by the agent and by its AI company. @@ -166,7 +172,7 @@ git fetch origin scripts/build-reader.sh --out ``` -It needs Rust, like the app build. Choose a folder outside the project, for example next to your Backups; the script refuses one inside it, or one that already holds a Reader. It builds the Reader from a fresh copy of `origin/main` as of that `git fetch`, not from your working folder, and prints the commit and the SHA-256 of the `enote-reader` it saved. Write both on paper, and copy this guide next to the Reader. Before you use it, run `shasum -a 256 enote-reader` in that folder and check that the result matches your paper. Another build of the same commit may give a different SHA-256, so check the copy you saved. Make a new one after updating the app, because a newer Vault may need a newer Reader. `--commit` builds an older commit on `main`, and `--offline` builds only from what Cargo has already downloaded. The script hasn't been tried on Windows. +It needs Rust and Node.js 24 or newer, like the app build. Choose a folder outside the project, for example next to your Backups; the script refuses one inside it, or one that already holds a Reader. It builds the Reader from a fresh copy of `origin/main` as of that `git fetch`, not from your working folder, and prints the commit and the SHA-256 of the `enote-reader` it saved. Write both on paper, and copy this guide next to the Reader. Before you use it, run `shasum -a 256 enote-reader` in that folder and check that the result matches your paper. Another build of the same commit may give a different SHA-256, so check the copy you saved. Make a new one after updating the app, because a newer Vault may need a newer Reader. `--commit` builds an older commit on `main`, and `--offline` builds only from what Cargo has already downloaded. The script hasn't been tried on Windows. Why not just `cargo run`? It compiles whatever the project folder holds at the moment you type your Master Password, including unfinished or unreviewed changes an AI coding tool made there. A saved Reader built from merged `main` doesn't change afterwards. diff --git a/scripts/build-reader.sh b/scripts/build-reader.sh index 8694a47..dbe6fa8 100755 --- a/scripts/build-reader.sh +++ b/scripts/build-reader.sh @@ -34,6 +34,7 @@ done need() { command -v "$1" >/dev/null 2>&1 || { echo "$1 not found: $2" >&2; exit 1; }; } need git "install Git (on Mac, run 'xcode-select --install')" need cargo "install rustup, which reads the version from rust-toolchain.toml" +need node "install Node.js 24 or newer, as for the app build" sha256() { if command -v shasum >/dev/null 2>&1; then shasum -a 256 "$1" | cut -d' ' -f1; else sha256sum "$1" | cut -d' ' -f1; fi } @@ -63,7 +64,7 @@ outside "$EXISTING" mkdir -p "$OUT" OUT=$(cd "$OUT" && pwd -P) outside "$OUT" -[[ ! -e "$OUT/enote-reader" && ! -e "$OUT/enote-reader.exe" ]] || { echo "$OUT already has an enote-reader. Choose an empty folder, or move the old one yourself." >&2; exit 1; } +[[ ! -e "$OUT/enote-reader" && ! -L "$OUT/enote-reader" && ! -e "$OUT/enote-reader.exe" && ! -L "$OUT/enote-reader.exe" ]] || { echo "$OUT already has an enote-reader. Choose an empty folder, or move the old one yourself." >&2; exit 1; } WORK=$(mktemp -d "${TMPDIR:-/tmp}/enote-reader-build.XXXXXX") trap '/bin/rm -rf "$WORK"' EXIT @@ -89,8 +90,13 @@ done BIN="$WORK/target/release/enote-reader" [[ -f "$BIN" ]] || BIN="$BIN.exe" [[ -f "$BIN" ]] || { echo "No enote-reader binary was produced." >&2; exit 1; } -cp "$BIN" "$OUT/" SAVED="$OUT/$(basename "$BIN")" +# Exclusive creation also refuses special files another build put here while this compiled. +( umask 077; node -e ' + const fs = require("node:fs"); + try { fs.copyFileSync(process.argv[1], process.argv[2], fs.constants.COPYFILE_EXCL); } + catch { process.exit(1); } +' "$BIN" "$SAVED" ) || { echo "Could not save $SAVED without replacing an existing file. Choose an empty folder." >&2; exit 1; } chmod 755 "$SAVED" echo "Reader: $SAVED" diff --git a/scripts/build-reader.test.mjs b/scripts/build-reader.test.mjs new file mode 100644 index 0000000..cc7ba44 --- /dev/null +++ b/scripts/build-reader.test.mjs @@ -0,0 +1,123 @@ +import test from 'node:test'; +import assert from 'node:assert/strict'; +import { execFile } from 'node:child_process'; +import { promisify } from 'node:util'; +import { mkdtemp, mkdir, writeFile, readFile, lstat, readlink, symlink, open, rm } from 'node:fs/promises'; +import { constants } from 'node:fs'; +import { createHash } from 'node:crypto'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; + +const execute = promisify(execFile); +const script = fileURLToPath(new URL('./build-reader.sh', import.meta.url)); +const fictionalBinary = 'FAKE_READER_BYTES_FOR_PUBLICATION_TEST_ONLY\n'; + +async function fixture(run) { + const root = await mkdtemp(path.join(tmpdir(), 'enote-reader-publication-')); + const bin = path.join(root, 'bin'); + const out = path.join(root, 'out'); + const scratch = path.join(root, 'tmp'); + try { + await Promise.all([bin, out, scratch].map(folder => mkdir(folder))); + await writeFile(path.join(bin, 'git'), `#!${process.execPath} +import fs from 'node:fs'; +const args = process.argv.slice(2); +while (args[0] === '-C' || args[0] === '-c') args.splice(0, 2); +if (args[0] === 'init') fs.mkdirSync(args.at(-1), { recursive: true }); +else if (args[0] === 'rev-parse') process.stdout.write('FAKE_REVIEWED_COMMIT_ONLY\\n'); +else if (!['fetch', 'checkout', 'merge-base'].includes(args[0])) process.exit(2); +`, { mode: 0o755 }); + await writeFile(path.join(bin, 'cargo'), `#!${process.execPath} +import fs from 'node:fs'; +import path from 'node:path'; +const release = path.join(process.env.CARGO_TARGET_DIR, 'release'); +fs.mkdirSync(release, { recursive: true }); +fs.writeFileSync(path.join(release, 'enote-reader'), ${JSON.stringify(fictionalBinary)}); +const output = path.join(process.env.ENOTE_REVIEW_OUT, 'enote-reader'); +if (process.env.ENOTE_REVIEW_LATE === 'file') fs.writeFileSync(output, 'FAKE_EXISTING_READER_ONLY\\n'); +if (process.env.ENOTE_REVIEW_LATE === 'link') fs.symlinkSync(path.join(process.env.ENOTE_REVIEW_OUT, 'fictional-target'), output); +if (process.env.ENOTE_REVIEW_LATE === 'fifo') fs.renameSync(path.join(process.env.ENOTE_REVIEW_OUT, 'fictional-target'), output); +if (process.env.ENOTE_REVIEW_LATE === 'fifo-link') fs.symlinkSync(path.join(process.env.ENOTE_REVIEW_OUT, 'fictional-target'), output); +`, { mode: 0o755 }); + await writeFile(path.join(bin, 'shasum'), `#!${process.execPath} +import fs from 'node:fs'; +import { createHash } from 'node:crypto'; +const file = process.argv.at(-1); +// A broken publication must fail its assertion, rather than hanging while hashing a FIFO. +process.stdout.write(fs.lstatSync(file).isFile() ? createHash('sha256').update(fs.readFileSync(file)).digest('hex') : 'FAKE_NONREGULAR_HASH'); +`, { mode: 0o755 }); + const build = (late = '') => execute('bash', [script, '--out', out], { + env: { ...process.env, PATH: bin + path.delimiter + process.env.PATH, + TMPDIR: scratch, ENOTE_REVIEW_OUT: out, ENOTE_REVIEW_LATE: late }, + }); + await run({ out, build }); + } finally { + await rm(root, { recursive: true, force: true }); + } +} + +// The owner script is Bash-only; its Windows/Git Bash build remains unverified. +const options = { skip: process.platform === 'win32' }; + +test('Reader publication saves a fresh executable and reports its actual hash', options, async () => { + await fixture(async ({ out, build }) => { + const { stdout } = await build(); + const saved = path.join(out, 'enote-reader'); + assert.equal(await readFile(saved, 'utf8'), fictionalBinary); + assert.equal((await lstat(saved)).mode & 0o777, 0o755); + assert.ok(stdout.includes(createHash('sha256').update(fictionalBinary).digest('hex'))); + }); +}); + +test('Reader publication refuses an existing file or dangling link for either binary name', options, async () => { + for (const name of ['enote-reader', 'enote-reader.exe']) { + for (const kind of ['file', 'link']) { + await fixture(async ({ out, build }) => { + const saved = path.join(out, name); + const target = path.join(out, 'fictional-target'); + if (kind === 'file') await writeFile(saved, 'FAKE_EXISTING_READER_ONLY\n'); + else await symlink(target, saved); + await assert.rejects(build()); + if (kind === 'file') assert.equal(await readFile(saved, 'utf8'), 'FAKE_EXISTING_READER_ONLY\n'); + else { + assert.equal(await readlink(saved), target); + await assert.rejects(lstat(target), { code: 'ENOENT' }); + } + }); + } + } +}); + +test('Reader publication refuses a file or dangling link created while compiling', options, async () => { + for (const late of ['file', 'link']) { + await fixture(async ({ out, build }) => { + await assert.rejects(build(late)); + const saved = path.join(out, 'enote-reader'); + if (late === 'file') assert.equal(await readFile(saved, 'utf8'), 'FAKE_EXISTING_READER_ONLY\n'); + else { + assert.equal(await readlink(saved), path.join(out, 'fictional-target')); + await assert.rejects(lstat(path.join(out, 'fictional-target')), { code: 'ENOENT' }); + } + }); + } +}); + +test('Reader publication refuses a FIFO or link to one created while compiling', options, async () => { + for (const late of ['fifo', 'fifo-link']) { + await fixture(async ({ out, build }) => { + const target = path.join(out, 'fictional-target'); + await execute('mkfifo', ['-m', '600', target]); + // Keep a nonblocking reader open so a faulty writer is observable without hanging. + const reader = await open(target, constants.O_RDONLY | constants.O_NONBLOCK); + try { + await assert.rejects(build(late)); + const node = late === 'fifo' ? path.join(out, 'enote-reader') : target; + assert.equal((await lstat(node)).mode & 0o777, 0o600); + assert.equal((await reader.read(Buffer.alloc(128), 0, 128, null)).bytesRead, 0); + } finally { + await reader.close(); + } + }); + } +}); diff --git a/scripts/check.sh b/scripts/check.sh index 5d2f3e6..9825331 100755 --- a/scripts/check.sh +++ b/scripts/check.sh @@ -40,6 +40,9 @@ node -e ' } ' +echo "==> Reader publication safety" +node --test scripts/build-reader.test.mjs + # build.rs compiles only the client; service.swift is otherwise built by package-companion.sh alone. if [[ $(uname -s) == Darwin ]]; then echo "==> companion helper typecheck" diff --git a/src-tauri/src/commands.rs b/src-tauri/src/commands.rs index 56e86f2..7ebb780 100644 --- a/src-tauri/src/commands.rs +++ b/src-tauri/src/commands.rs @@ -33,6 +33,15 @@ pub const STATUS_EVENT: &str = "status-changed"; /// Managed state: the one `AppCore`. pub struct CoreState(pub Mutex); +impl CoreState { + fn lock(&self, focused: &AtomicBool) -> MutexGuard<'_, AppCore> { + let mut core = self.0.lock().unwrap_or_else(PoisonError::into_inner); + // A queued Show or Copy can run before the focus-event worker. + core.set_window_in_front(focused.load(Ordering::SeqCst)); + core + } +} + /// True while a Backup file dialog is open. The dialog takes focus from our window, which must /// not count as switching to another app (that could lock the Vault mid-backup). pub static DIALOG_OPEN: AtomicBool = AtomicBool::new(false); @@ -49,9 +58,7 @@ pub fn status(core: &AppCore) -> AppStatus { pub fn core(app: &AppHandle) -> MutexGuard<'_, AppCore> { app.state::() .inner() - .0 - .lock() - .unwrap_or_else(PoisonError::into_inner) + .lock(&crate::WINDOW_FOCUSED) } /// Run `f` on the core, then tell the screens the new status. @@ -744,9 +751,111 @@ fn open_chosen(path: &std::path::Path) -> Result { #[cfg(test)] mod tests { use super::{ - COMPANION_START, SettingsRequest, backup_file_name, companion_start_confirmed, + COMPANION_START, CoreState, SettingsRequest, backup_file_name, companion_start_confirmed, companion_start_text, }; + use crate::core::native::NativeAction; + use crate::core::{ApiError, AppCore, Clipboard, Config}; + use std::sync::atomic::{AtomicBool, AtomicU64, Ordering}; + use std::sync::{Arc, Mutex}; + use zeroize::Zeroizing; + + struct CountingClipboard(Arc); + + impl Clipboard for CountingClipboard { + fn write(&mut self, _text: &str) -> Result { + Ok(self.0.fetch_add(1, Ordering::SeqCst) + 1) + } + + fn clear_if(&mut self, _change_count: u64) -> bool { + true + } + } + + #[test] + fn show_and_copy_use_focus_before_the_event_worker_runs() { + let temp = tempfile::tempdir().unwrap(); + let copies = Arc::new(AtomicU64::new(0)); + let mut core = AppCore::new(Config { + data_dir: temp.path().join("fictional-vault"), + home_dir: temp.path().join("fictional-home"), + limits: vault_format::Limits::TESTING, + clock: Box::new(crate::SystemClock::start()), + clipboard: Box::new(CountingClipboard(copies.clone())), + activity_anywhere_supported: false, + }) + .unwrap(); + let recovery = core + .create_vault( + Zeroizing::new("fake-lantern-quartz-mosaic-velvet-orbit".to_owned()), + vault_format::KdfParams::TESTING, + ) + .unwrap() + .recovery_key + .unwrap(); + core.confirm_recovery_kit(&recovery[recovery.len() - 9..]) + .unwrap(); + let id = core + .create_note( + serde_json::from_value(serde_json::json!({ + "kind": "api_key", "title": "Fictional focus test", + "visible": {"service": "Fictional provider", "env_var": "FAKE_AGENT_KEY"}, + "hidden": {"key": "FAKE_KEY_FOR_FOCUS_TESTS_ONLY"} + })) + .unwrap(), + None, + ) + .unwrap(); + let state = CoreState(Mutex::new(core)); + let focused = AtomicBool::new(true); + { + let mut core = state.lock(&focused); + core.show_field(&id, "key").unwrap(); + core.hide_field(); + } + + // The OS recorded the loss, but its worker has not yet acquired CoreState. + focused.store(false, Ordering::SeqCst); + { + let mut core = state.lock(&focused); + let error = core.show_field(&id, "key").unwrap_err(); + assert_eq!(error.code, "invalid_input"); + assert!(error.message.contains("window is in front")); + for result in [ + core.copy_field(&id, "key", None), + core.copy_field_line(&id, "key", 0), + core.copy_field_env(&id), + core.copy_visible_field(&id, "service"), + ] { + assert_eq!(result.unwrap_err().code, "invalid_input"); + } + assert_eq!( + core.begin_native(NativeAction::CopyField { + id: id.clone(), + field: "key".to_owned(), + }) + .err() + .unwrap() + .code, + "invalid_input" + ); + assert_eq!(core.native_pending_id(), None); + assert_eq!(core.app_status().reveal, None); + } + assert_eq!(copies.load(Ordering::SeqCst), 0); + + // The gain can also be newer than the last focus event applied to the core. + state + .0 + .lock() + .unwrap() + .os_event(session::OsEvent::FocusLost); + focused.store(true, Ordering::SeqCst); + let mut core = state.lock(&focused); + core.show_field(&id, "key").unwrap(); + core.copy_field(&id, "key", None).unwrap(); + assert_eq!(copies.load(Ordering::SeqCst), 1); + } #[test] fn only_the_start_button_starts_the_companion() { diff --git a/src-tauri/src/lib.rs b/src-tauri/src/lib.rs index d9ab272..24f26c6 100644 --- a/src-tauri/src/lib.rs +++ b/src-tauri/src/lib.rs @@ -52,8 +52,8 @@ use crate::commands::{CoreState, DIALOG_OPEN, STATUS_EVENT}; use crate::core::{AppCore, AppStatus, Clock, Config}; /// Whether the window is in front, by the latest focus event. Set on the thread that reports -/// the event, before the core hears of it on a thread of its own. Those threads reach the core -/// in no set order, so each then gives the core this latest value (`commands::os_event`). +/// the event, before its worker reaches the core. Every core access refreshes from this value +/// under the mutex before a command can Show or Copy (`commands::core`). static WINDOW_FOCUSED: AtomicBool = AtomicBool::new(true); fn note_focus(event: OsEvent) { From bbd42b159438aa64cbf11ae8a3a6d3e179b3596d Mon Sep 17 00:00:00 2001 From: NNIIOVO Date: Wed, 4 Nov 2026 11:31:55 -0600 Subject: [PATCH 2/4] fix(companion): bind before publishing launch tokens --- companion/README.md | 6 +- companion/isolation/PLAN.md | 6 +- companion/isolation/integrated-broker.mjs | 53 ++----- companion/isolation/owner-launcher.mjs | 46 ++++-- companion/isolation/token-handoff.mjs | 66 ++++++++ companion/test/integrated.test.mjs | 86 ++++++++++- companion/test/owner-launcher.test.mjs | 177 ++++++++++++++++++++++ 7 files changed, 378 insertions(+), 62 deletions(-) create mode 100644 companion/isolation/token-handoff.mjs create mode 100644 companion/test/owner-launcher.test.mjs diff --git a/companion/README.md b/companion/README.md index 3e3f37f..e4f8341 100644 --- a/companion/README.md +++ b/companion/README.md @@ -10,6 +10,10 @@ The selected deployment is: agent/code → stdio bridge → local owner broker The bridge lists the tool even while the broker is off and reads the token again on each call, so a new Start needs no restart of the agent's MCP client. With no token or no listener, a call returns one fixed error telling the agent to ask the owner. +Both protected launchers bind the real loopback listener before publishing a fresh random 32-byte base64url token with the agent's read ACL. A failed bind leaves any existing launch's token untouched. The standalone launcher also rotates the token on every launch, without extending the reviewed plan's grant deadline. Cleanup uses the created file's inode/device lease, retains its open handle until cleanup, and removes only that publication; stale cleanup cannot remove a successor already published in the owner-controlled handoff directory. A publication failure closes the listener and cleans its own partial publication. + +This closes the demonstrated capture-before-bind and later-token-reuse path. The bearer transport does not authenticate the server to the client; it is not complete endpoint authentication or protection against malicious software running as the owner. + [The keyless code example](docs/keyless-code.md) shows the client call and its prerequisites. This is one restricted operation, not a drop-in OpenAI SDK endpoint. The Mac identity boundary, provider controls and live connection remain unverified. The planned credential file format is plaintext with owner-only permissions, not encrypted Vault storage; no real credential file has been provisioned. OpenAI receives the key over HTTPS for authentication. Owner Vault and wallet keys are never part of this route. ## Fixed operation @@ -104,7 +108,7 @@ The integrated entry point runs the same fixed broker with: - fixed data paths inside the helper's own container (`$HOME/Library/Application Support/encrypted-note-companion/plan.json` and `credential.env`), never chosen by a caller; - one new grant of at most one hour for each owner Start, enforced before the credential read, before listening and before every admission, including direct loopback calls; -- a fresh delegated agent token for each Start, written owner-only (0600, plus the single agent read ACL) into the owner-prepared handoff folder, and removed on every stop; +- a fresh delegated agent token for each Start, written only after binding loopback, owner-only (0600, plus the single agent read ACL) into the owner-prepared handoff folder; each stop removes only the publication owned by that launch; - a bounded owner-control protocol on stdin/stdout, separate from the agent MCP channel: one `{"v":1,"op":"stop"}` command in; `ready`, `stopped`, `error` and `check` events with fixed codes out. Control EOF, a malformed command, a signal or the grant deadline stops the broker, and the process then exits without waiting for the helper to close the pipe. Nothing from the plan, credential, token, provider or MCP traffic is printed. Its `--check` mode reports readiness booleans and never reads the credential's contents. Code and runtime ancestors may be admin-group-writable (an app in /Applications) only after the agent account is confirmed not to be an administrator. diff --git a/companion/isolation/PLAN.md b/companion/isolation/PLAN.md index 84942cc..456e2db 100644 --- a/companion/isolation/PLAN.md +++ b/companion/isolation/PLAN.md @@ -29,7 +29,7 @@ The local Vault requires no new server login, OpenAI account, Apple Developer lo 6. Complete process, clipboard, capture/control and connected-host checks below. Fix failures only through a separately reviewed change; do not grant privacy/debugging permissions to make a denial check pass. If ownership is ignored on a volume or broad ACL changes seem necessary, stop and select a different deployment location. 7. Independently apply provider permission, model/tool denial, rate limits, one-hour expiry, $1 HARD monthly project limit and owner revocation controls. Spend alerts are insufficient. Hard-limit enforcement can slightly overshoot during propagation. The known provider hosted-shell scope gap remains explicit; raw-key custody must hold. 8. Only after boundary review, use the secure Platform picker, destination confirmation and trusted helper for the NEW Agent Key. Read no existing owner key. The selected file is parsed without eval, sourcing or environment export. The one key-paste route is write-agent-key.mjs, which refuses any value that isn't shaped like an OpenAI API key. -9. In the owner session, explicitly launch owner-launcher.mjs with the reviewed plan path, confirmed credential-file path and delegated-token-file path. These arguments are paths only. The launcher binds loopback, holds the key in owner-process memory, and stops when the plan's grant ends (at most one hour). JavaScript does not guarantee memory erasure. Alternatively, encrypted-note's integrated helper runs isolation/integrated-broker.mjs: the owner installs the plan and confirmed credential through Settings → Agent Companion → Set up…, and each Start issues a one-hour grant and a fresh delegated token in the handoff folder (encrypted-note docs/agent-companion.md). +9. In the owner session, explicitly launch owner-launcher.mjs with the reviewed plan path, confirmed credential-file path and delegated-token-file path. These arguments are paths only. The launcher binds loopback before publishing a fresh random delegated token for that launch, holds the key in owner-process memory, and stops when the original plan's grant ends (at most one hour). It removes only its own token publication when the listener closes; restarting does not renew the grant or accept the prior launch's token. JavaScript does not guarantee memory erasure. Alternatively, encrypted-note's integrated helper runs isolation/integrated-broker.mjs: the owner installs the plan and confirmed credential through Settings → Agent Companion → Set up…, and each Start issues a one-hour grant and a fresh delegated token in the handoff folder (encrypted-note docs/agent-companion.md). 10. In the actual agent session, configure the MCP client to launch agent-bridge.mjs with only the nonsecret plan path. No key or token value belongs in MCP configuration. Make one owner-approved fictional text request. Verify expiry, revocation and provider denial before recommending real use. Stopping the broker is not provider revocation. No launchd plist, login item, automatic restart, sudoers rule, public listener, custom user-login service or account-creation script is installed. The broker is an explicitly started local process and can be stopped with Control-C in the owner terminal. @@ -41,11 +41,11 @@ Use a trusted absolute Node executable; replace paths locally without putting ac - fixtures.mjs prepare takes the reviewed plan path and an existing protected parent; it creates only a fresh enote-isolation-* directory with 0700 directories and 0600 fictional members. Its output is a nonsecret private fixture manifest, never a GitHub artifact. - agent-probe.mjs takes the reviewed plan, copied nonsecret fixture manifest and a new private agent workspace. It checks actual UID/name, privileged groups and noninteractive sudo before touching fixtures. It then checks its own workspace positively and requires permission denial for owner read/list/write/create/rename/unlink, fake code/policy/dependency writes and an agent-created symlink into the owner fixture. - fixtures.mjs verify-owner takes the original owner-record.json and requires unchanged fixture ownership, modes and hashes. Preserve evidence on failure. Remove only these explicitly created disposable members after owner verification; no broad recursive cleanup or permission command is provided. -- owner-launcher.mjs takes reviewed plan, confirmed credential path and delegated-token path. It requires the owner UID and the token path to match the plan. Application/SDK imports are delayed until protection checks finish, but an independent trusted preflight is still mandatory. +- owner-launcher.mjs takes reviewed plan, confirmed credential path and delegated-token path. It requires the owner UID and the token path to match the plan, checks the owner-prepared handoff directory, and creates a fresh token only after successfully binding the listener. A failed bind leaves an existing publication untouched. A failed publication closes the listener and cleans only the file created by that launch. Application/SDK imports are delayed until protection checks finish, but an independent trusted preflight is still mandatory. - write-agent-key.mjs takes one output path and reads the NEW Agent Key on stdin (`pbpaste | node write-agent-key.mjs ~/enote-agent-key.env`), never as an argument. It writes only a value shaped like an OpenAI API key (sk-, then 37 to 509 letters, digits, - or _; no spaces or line breaks). It refuses Admin keys (sk-admin-), Anthropic (sk-ant-) and OpenRouter (sk-or-) keys, and input over 4096 bytes however the reads split it; an OpenAI key copied from the Vault would still pass. It writes owner-only 0600 without following a symlink or writing through a hard link, refuses a file with an ACL allow entry (set on it or inherited from its folder, checked before and after writing), then asks Time Machine to leave the file out (tmutil addexclusion), which may fail without stopping it. No message includes the value. - agent-bridge.mjs requires the actual selected Standard UID. It reads only the plan and the separate delegated token and exposes stdio to the local MCP client. It forwards only the one fixed bounded operation. It lists the tool even while the broker is off and reads the token again on each call, so a new Start needs no restart; with no token or no listener, a call returns one fixed error. -The delegated token may need a fresh directory under /Users/Shared if the owner home is not traversable. This exception is only for the limited local token, never the provider key. Require root-owned sticky /Users/Shared, a new owner-owned 0700 child, exactly one agent-user search ACL on that child and exactly one agent-user read ACL on its 0600 token file. Other allow ACLs fail closed. Never weaken an existing owner parent to make token sharing work. An owner-reviewed targeted token-creation/ACL step remains pending; this package executes none. +The delegated token may need a fresh directory under /Users/Shared if the owner home is not traversable. This exception is only for the limited local token, never the provider key. Require root-owned sticky /Users/Shared, a new owner-owned 0700 child, exactly one agent-user search ACL on that child and exactly one agent-user read ACL on its 0600 token file. Other allow ACLs fail closed. Never weaken an existing owner parent to make token sharing work. An owner-reviewed targeted handoff-directory preparation step remains pending; this package executes none. Each protected broker launch creates its own 0600 token and adds the exact agent read ACL after binding. Cleanup holds an inode/device lease with an open file handle, so stale cleanup cannot delete a successor already published in this owner-controlled directory. These lifetime checks close capture-before-bind and token reuse; the bearer transport does not authenticate a server to its client or protect against malicious software running as the owner. ## Additional fictional checks and stop rules diff --git a/companion/isolation/integrated-broker.mjs b/companion/isolation/integrated-broker.mjs index 270f9f6..0a43740 100644 --- a/companion/isolation/integrated-broker.mjs +++ b/companion/isolation/integrated-broker.mjs @@ -8,10 +8,11 @@ // provider or MCP traffic is ever written to stdout or stderr. import path from 'node:path'; import { constants } from 'node:fs'; -import { open, lstat, unlink, realpath } from 'node:fs/promises'; +import { open, lstat, realpath } from 'node:fs/promises'; import { randomBytes } from 'node:crypto'; import { fileURLToPath, pathToFileURL } from 'node:url'; import { performance } from 'node:perf_hooks'; +import { checkHandoffDirectory, writeAgentToken, removeAgentToken, tokenIo } from './token-handoff.mjs'; import { loadPlan, requireRole, assertProtectedPath, verifyCodeTree, readConfirmedCredential, assertGrantActive, grantGuard, MAX_GRANT_MS, checkAcl, run, @@ -101,41 +102,9 @@ async function verifyCode(ownerUid) { } } -// The handoff directory is owner-prepared: owner-owned 0700 with exactly one agent search ACL. -export async function checkHandoffDirectory(plan, { stat = lstat, aclCheck = checkAcl } = {}) { - const dir = path.dirname(plan.bearerTokenFile); - const info = await stat(dir); - if (!info.isDirectory() || info.isSymbolicLink() || info.uid !== plan.ownerUid || (info.mode & 0o777) !== 0o700) { - fail('handoff_unavailable'); - } - await assertProtectedPath(dir, plan.ownerUid, stat, { - tokenAgent: plan.agentShortName, - aclCheck: (file, permitted) => aclCheck(file, file === dir ? 'user:' + plan.agentShortName + ' allow search' : permitted), - }); -} - -const defaultIo = { - stat: lstat, - aclCheck: checkAcl, - addReadAcl: (file, agent) => run('/bin/chmod', ['+a', 'user:' + agent + ' allow read', file], { maxBuffer: 1024, timeout: 5000 }), - verifyCode, - assertAgentNotAdmin, -}; - -export async function writeAgentToken(plan, token, io) { - const file = plan.bearerTokenFile; - // A token left by an abnormal exit is removed only if it is our own plain file. - try { - const stale = await io.stat(file); - if (!stale.isFile() || stale.isSymbolicLink() || stale.uid !== plan.ownerUid || stale.nlink !== 1) fail('handoff_unavailable'); - await unlink(file); - } catch (error) { if (error instanceof ControlError) throw error; if (error?.code !== 'ENOENT') fail('handoff_unavailable'); } - const handle = await open(file, constants.O_WRONLY | constants.O_CREAT | constants.O_EXCL | constants.O_NOFOLLOW, 0o600); - try { await handle.writeFile(token + '\n'); await handle.sync(); } finally { await handle.close(); } - await io.addReadAcl(file, plan.agentShortName); - await assertProtectedPath(file, plan.ownerUid, io.stat, { aclCheck: io.aclCheck, tokenAgent: plan.agentShortName }); -} -async function removeAgentToken(file) { try { await unlink(file); } catch { /* already gone */ } } +// Keep the existing token helpers importable from this entry point. +export { checkHandoffDirectory, writeAgentToken } from './token-handoff.mjs'; +const defaultIo = { ...tokenIo, verifyCode, assertAgentNotAdmin }; // Metadata-only readiness. The credential's contents are not read here. export async function checkReadiness({ home, uid = process.getuid(), io = defaultIo } = {}) { @@ -192,12 +161,14 @@ export async function startIntegratedBroker({ generate = guardGenerate(createGenerator({ apiKey, ...(transport ? { transport } : {}) }), isActive); } catch { apiKey = ''; fail('credential_invalid'); } apiKey = ''; // Best effort only; JavaScript strings cannot be guaranteed erased. - try { await writeAgentToken(plan, token, io); } - catch { await removeAgentToken(plan.bearerTokenFile); fail('handoff_unavailable'); } - if (!isActive()) { await removeAgentToken(plan.bearerTokenFile); fail('grant_inactive'); } + if (!isActive()) fail('grant_inactive'); let server; try { server = await startHttpServer({ generate, bearerToken: token, port: port ?? plan.brokerPort }); } - catch { await removeAgentToken(plan.bearerTokenFile); fail('port_unavailable'); } + catch { fail('port_unavailable'); } + let publication; + try { publication = await writeAgentToken(plan, token, io); } + catch { stopHttpServer(server); fail('handoff_unavailable'); } + if (!isActive()) { stopHttpServer(server); await removeAgentToken(publication); fail('grant_inactive'); } let stopped = false; let resolveDone; @@ -227,7 +198,7 @@ export async function startIntegratedBroker({ input.destroy?.(); for (const event of ['SIGTERM', 'SIGINT']) signals.removeListener?.(event, onSignal); stopHttpServer(server); - await removeAgentToken(plan.bearerTokenFile); + await removeAgentToken(publication); emit(encodeEvent({ event: 'stopped', reason })); resolveDone(reason); return done; diff --git a/companion/isolation/owner-launcher.mjs b/companion/isolation/owner-launcher.mjs index 1068c0e..c0dceb2 100644 --- a/companion/isolation/owner-launcher.mjs +++ b/companion/isolation/owner-launcher.mjs @@ -1,24 +1,34 @@ import path from 'node:path'; import { fileURLToPath, pathToFileURL } from 'node:url'; import { realpath } from 'node:fs/promises'; +import { randomBytes } from 'node:crypto'; +import { tokenIo, checkHandoffDirectory, writeAgentToken, removeAgentToken } from './token-handoff.mjs'; import { loadPlan, requireRole, assertProtectedPath, verifyCodeTree, - readConfirmedCredential, readAgentToken, assertGrantActive, grantGuard, + readConfirmedCredential, assertGrantActive, grantGuard, GRANT_INACTIVE, MAX_GRANT_MS } from './common.mjs'; -export async function launchOwnerBroker(plan, { credentialFile, tokenFile, output = process.stderr } = {}) { - requireRole(plan, 'owner'); - assertGrantActive(plan); - if (process.platform !== 'darwin') throw new Error('This launcher is for the reviewed Mac only.'); +async function verifyCode(ownerUid) { const root = path.dirname(path.dirname(fileURLToPath(import.meta.url))); - await assertProtectedPath(await realpath(process.execPath), plan.ownerUid); + await assertProtectedPath(await realpath(process.execPath), ownerUid); for (const item of ['src', 'isolation', 'node_modules']) { - await verifyCodeTree(path.join(root, item), plan.ownerUid); + await verifyCodeTree(path.join(root, item), ownerUid); } for (const item of ['policy.json', 'package.json', 'package-lock.json']) { - await assertProtectedPath(path.join(root, item), plan.ownerUid); + await assertProtectedPath(path.join(root, item), ownerUid); } +} +const defaultIo = { ...tokenIo, verifyCode }; + +export async function launchOwnerBroker(plan, { + credentialFile, tokenFile, output = process.stderr, io = defaultIo, transport, + port, // tests only; the production entry point always uses the reviewed plan's port +} = {}) { + requireRole(plan, 'owner'); + assertGrantActive(plan); + if (process.platform !== 'darwin') throw new Error('This launcher is for the reviewed Mac only.'); + await io.verifyCode(plan.ownerUid); if (tokenFile !== plan.bearerTokenFile) throw new Error('Delegated token path differs from the reviewed plan.'); - await assertProtectedPath(tokenFile, plan.ownerUid, undefined, { tokenAgent: plan.agentShortName }); + await checkHandoffDirectory(plan, io); // An independent owner preflight must already trust this launcher and its Node runtime. // Delay application/SDK imports until the remaining protected tree checks complete. const { createGenerator, guardGenerate } = await import('../src/generate.mjs'); @@ -26,12 +36,23 @@ export async function launchOwnerBroker(plan, { credentialFile, tokenFile, outpu // Grant again immediately before the credential read and before the broker opens. const expiresAt = assertGrantActive(plan); let apiKey = await readConfirmedCredential(credentialFile, plan.ownerUid); - const bearerToken = await readAgentToken(tokenFile, plan.ownerUid); + const bearerToken = randomBytes(32).toString('base64url'); if (apiKey === bearerToken) { apiKey = ''; throw new Error('Separate credentials are required.'); } - const generate = guardGenerate(createGenerator({ apiKey }), grantGuard(expiresAt)); + const isActive = grantGuard(expiresAt); + const generate = guardGenerate(createGenerator({ apiKey, ...(transport ? { transport } : {}) }), isActive); apiKey = ''; // Best effort only; JavaScript strings cannot be guaranteed erased. assertGrantActive(plan); - const server = await startHttpServer({ generate, bearerToken, port: plan.brokerPort }); + if (!isActive()) throw new Error(GRANT_INACTIVE); + const server = await startHttpServer({ generate, bearerToken, port: port ?? plan.brokerPort }); + let publication; + try { + publication = await writeAgentToken(plan, bearerToken, io); + if (!isActive()) throw new Error(GRANT_INACTIVE); + } catch (error) { + stopHttpServer(server); + await removeAgentToken(publication); + throw error; + } const stop = () => stopHttpServer(server); // Stop at the earlier of the absolute grant deadline and one hour from now. Supplemental // to, never a substitute for, provider-enforced key expiry. @@ -40,6 +61,7 @@ export async function launchOwnerBroker(plan, { credentialFile, tokenFile, outpu server.once('close', () => { clearTimeout(lifetime); for (const event of ['SIGINT', 'SIGTERM']) process.removeListener(event, stop); + void removeAgentToken(publication); }); output.write('Owner broker is listening on reviewed loopback port 8789 until its grant ends (at most one hour).\n'); return server; diff --git a/companion/isolation/token-handoff.mjs b/companion/isolation/token-handoff.mjs new file mode 100644 index 0000000..65c2344 --- /dev/null +++ b/companion/isolation/token-handoff.mjs @@ -0,0 +1,66 @@ +// Launch-lifetime authority in the owner-prepared, agent-search-only handoff directory. +// The owner-only directory is the boundary: it excludes agent renames, not other owner software. +import path from 'node:path'; +import { constants } from 'node:fs'; +import { open, lstat, unlink } from 'node:fs/promises'; +import { assertProtectedPath, checkAcl, run } from './common.mjs'; + +export const tokenIo = { + stat: lstat, + aclCheck: checkAcl, + addReadAcl: (file, agent) => run('/bin/chmod', ['+a', 'user:' + agent + ' allow read', file], { maxBuffer: 1024, timeout: 5000 }), +}; +const publications = new WeakMap(); +const unavailable = () => { throw new Error('Delegated token handoff is unavailable.'); }; +const matches = (info, lease) => info.isFile() && !info.isSymbolicLink() && + info.dev === lease.dev && info.ino === lease.ino; + +export async function checkHandoffDirectory(plan, { stat = lstat, aclCheck = checkAcl } = {}) { + const dir = path.dirname(plan.bearerTokenFile); + const info = await stat(dir); + if (!info.isDirectory() || info.isSymbolicLink() || info.uid !== plan.ownerUid || (info.mode & 0o777) !== 0o700) unavailable(); + await assertProtectedPath(dir, plan.ownerUid, stat, { + tokenAgent: plan.agentShortName, + aclCheck: (file, permitted) => aclCheck(file, file === dir ? 'user:' + plan.agentShortName + ' allow search' : permitted), + }); +} + +// Call only after successfully binding this launch's listener. Retain the open handle until +// cleanup, so an unlinked publication's inode cannot be recycled into a successor's lease. +export async function writeAgentToken(plan, token, io = tokenIo) { + const file = plan.bearerTokenFile; + try { + const stale = await io.stat(file); + if (!stale.isFile() || stale.isSymbolicLink() || stale.uid !== plan.ownerUid || stale.nlink !== 1) unavailable(); + await unlink(file); + } catch (error) { if (error?.code !== 'ENOENT') throw error; } + const handle = await open(file, constants.O_WRONLY | constants.O_CREAT | constants.O_EXCL | constants.O_NOFOLLOW, 0o600); + let lease; + try { + const info = await handle.stat(); + lease = Object.freeze({ file, dev: info.dev, ino: info.ino }); + publications.set(lease, handle); + await handle.writeFile(token + '\n'); + await handle.sync(); + await io.addReadAcl(file, plan.agentShortName); + await assertProtectedPath(file, plan.ownerUid, io.stat, { aclCheck: io.aclCheck, tokenAgent: plan.agentShortName }); + if (!matches(await lstat(file), lease)) unavailable(); + return lease; + } catch (error) { + if (lease) await removeAgentToken(lease); + else await handle.close(); + throw error; + } +} + +// A pathname is never cleanup authority. Only a lease issued by this module can remove the +// exact publication it owns. There is no same-owner malicious rename protection in Node's unlink. +export async function removeAgentToken(lease) { + const handle = lease && publications.get(lease); + if (!handle) return; + publications.delete(lease); + try { + if (matches(await lstat(lease.file), lease)) await unlink(lease.file); + } catch { /* gone, replaced, or no longer removable */ } + finally { await handle.close(); } +} diff --git a/companion/test/integrated.test.mjs b/companion/test/integrated.test.mjs index 51b5b4a..474cecd 100644 --- a/companion/test/integrated.test.mjs +++ b/companion/test/integrated.test.mjs @@ -73,12 +73,12 @@ function provider(calls, response) { output: [{ type: 'message', role: 'assistant', content: [{ type: 'output_text', text: 'Fictional output.' }] }] } }; }; } -async function start(fx, { calls = [], response, io, now, mono, setTimer } = {}) { +async function start(fx, { calls = [], response, io, now, mono, setTimer, port = 0 } = {}) { const lines = []; const input = new PassThrough(); const broker = await startIntegratedBroker({ home: fx.home, uid, transport: provider(calls, response), io: io ?? fakeIo().io, input, - emit: line => lines.push(line), createToken: () => TOKEN, port: 0, signals: { once() {}, removeListener() {} }, + emit: line => lines.push(line), createToken: () => TOKEN, port, signals: { once() {}, removeListener() {} }, ...(now ? { now } : {}), ...(mono ? { mono } : {}), ...(setTimer ? { setTimer, clearTimer: () => {} } : {}), }); return { broker, lines, input, calls, url: 'http://127.0.0.1:' + broker.server.address().port + '/mcp' }; @@ -140,6 +140,77 @@ test('start issues one fresh grant, hands off a private token and serves exactly } finally { await fx.cleanup(); } }); +test('integrated token read access is published only while the real listener owns the port', async () => { + const fx = await fixture(); + const reservation = net.createServer(); + await new Promise(resolve => reservation.listen(0, '127.0.0.1', resolve)); + const port = reservation.address().port; + await new Promise(resolve => reservation.close(resolve)); + let publicationSawBoundListener = false; + let running; + try { + const { io } = fakeIo({ addReadAcl: async () => { + const contender = net.createServer(); + publicationSawBoundListener = await new Promise(resolve => { + contender.once('error', error => resolve(error.code === 'EADDRINUSE')); + contender.listen(port, '127.0.0.1', () => contender.close(() => resolve(false))); + }); + } }); + running = await start(fx, { io, port }); + assert.equal(publicationSawBoundListener, true); + } finally { await running?.broker.stop('owner'); await fx.cleanup(); } +}); + +test('integrated stop cannot unlink a successor token publication', async () => { + const fx = await fixture(); + let running; + try { + running = await start(fx); + const successor = path.join(fx.handoff, 'successor'); + const successorToken = 'FAKE_SUCCESSOR_TOKEN_FOR_TESTS_ONLY_0123456789'; + await fs.writeFile(successor, successorToken + '\n', { mode: 0o600 }); + await fs.rename(successor, fx.plan.bearerTokenFile); + await running.broker.stop('owner'); + assert.equal(await fs.readFile(fx.plan.bearerTokenFile, 'utf8'), successorToken + '\n'); + } finally { await running?.broker.stop('owner'); await fx.cleanup(); } +}); + +test('failed integrated publication cleans only its own file and closes the listener', async () => { + const fx = await fixture(); + const reservation = net.createServer(); + await new Promise(resolve => reservation.listen(0, '127.0.0.1', resolve)); + const port = reservation.address().port; + await new Promise(resolve => reservation.close(resolve)); + const successorToken = 'FAKE_SUCCESSOR_TOKEN_FOR_TESTS_ONLY_0123456789'; + try { + const { io } = fakeIo({ addReadAcl: async file => { + const successor = path.join(fx.handoff, 'successor'); + await fs.writeFile(successor, successorToken + '\n', { mode: 0o600 }); + await fs.rename(successor, file); + throw new Error('FAKE_ACL_FAILURE'); + } }); + await assert.rejects(start(fx, { io, port }), { code: 'handoff_unavailable' }); + assert.equal(await fs.readFile(fx.plan.bearerTokenFile, 'utf8'), successorToken + '\n'); + await assert.rejects(fetch('http://127.0.0.1:' + port + '/mcp')); + } finally { await fx.cleanup(); } +}); + +test('integrated expiry during startup refuses before listener or token publication', async () => { + const fx = await fixture(); + let wall = Date.now(); + let publications = 0; + try { + await assert.rejects(startIntegratedBroker({ + home: fx.home, uid, now: () => wall, port: 0, transport: provider([]), + io: fakeIo({ addReadAcl: async () => { publications++; } }).io, + input: new PassThrough(), emit: () => {}, signals: { once() {}, removeListener() {} }, + createToken: () => { wall += MAX_GRANT_MS; return TOKEN; }, + }), { code: 'grant_inactive' }); + assert.equal(publications, 0); + await assert.rejects(fs.stat(fx.plan.bearerTokenFile), { code: 'ENOENT' }); + } finally { await fx.cleanup(); } +}); + test('control pipe EOF, malformed control and oversized control all stop the broker', async () => { for (const [send, reason] of [[null, 'parent'], ['{"v":1,"op":"restart"}\n', 'error'], ['x'.repeat(1024), 'error']]) { const fx = await fixture(); @@ -438,16 +509,21 @@ test('every refusal path returns one fixed code and leaves no token behind', asy assert.ok(ERROR_CODES.includes('port_unavailable')); }); -test('a busy reviewed port fails closed and removes the handed-off token', async () => { +test('a busy reviewed port never publishes or removes existing launch authority', async () => { const fx = await fixture(); const blocker = net.createServer(); await new Promise(resolve => blocker.listen(0, '127.0.0.1', resolve)); + const existingToken = 'FAKE_EXISTING_TOKEN_FOR_TESTS_ONLY_0123456789'; + await fs.writeFile(fx.plan.bearerTokenFile, existingToken + '\n', { mode: 0o600 }); + let publications = 0; try { const lines = []; - await assert.rejects(startIntegratedBroker({ home: fx.home, uid, transport: provider([]), io: fakeIo().io, + await assert.rejects(startIntegratedBroker({ home: fx.home, uid, transport: provider([]), io: fakeIo({ addReadAcl: async () => { publications++; } }).io, input: new PassThrough(), emit: line => lines.push(line), createToken: () => TOKEN, port: blocker.address().port, signals: { once() {}, removeListener() {} } }), error => error.code === 'port_unavailable'); - await assert.rejects(fs.stat(fx.plan.bearerTokenFile), { code: 'ENOENT' }); + assert.equal(publications, 0); + assert.equal(await fs.readFile(fx.plan.bearerTokenFile, 'utf8'), existingToken + '\n'); + assert.equal(lines.length, 0); } finally { blocker.close(); await fx.cleanup(); } }); diff --git a/companion/test/owner-launcher.test.mjs b/companion/test/owner-launcher.test.mjs new file mode 100644 index 0000000..259e470 --- /dev/null +++ b/companion/test/owner-launcher.test.mjs @@ -0,0 +1,177 @@ +import test from 'node:test'; +import assert from 'node:assert/strict'; +import fs from 'node:fs/promises'; +import os from 'node:os'; +import path from 'node:path'; +import net from 'node:net'; +import { once } from 'node:events'; +import { launchOwnerBroker } from '../isolation/owner-launcher.mjs'; +import { stopHttpServer } from '../src/http.mjs'; +import { writeAgentToken, removeAgentToken } from '../isolation/token-handoff.mjs'; + +const KEY = 'FAKE_OPENAI_AGENT_KEY_FOR_TESTS_ONLY'; +const OLD_TOKEN = 'FAKE_PREVIOUS_LAUNCH_TOKEN_FOR_TESTS_ONLY_0123456789'; +const uid = process.getuid(); +async function fixture() { + const root = await fs.mkdtemp(path.join(await fs.realpath(os.tmpdir()), 'enote-companion-owner-')); + const handoff = path.join(root, 'handoff'); + await fs.mkdir(handoff, { mode: 0o700 }); + const credentialFile = path.join(root, 'credential.env'); + const tokenFile = path.join(handoff, 'agent-token'); + await fs.writeFile(credentialFile, 'OPENAI_AGENT_API_KEY=' + KEY + '\n', { mode: 0o600 }); + await fs.writeFile(tokenFile, OLD_TOKEN + '\n', { mode: 0o600 }); + const plan = { + version: 2, ownerUid: uid, agentUid: uid + 1, agentShortName: 'enote-agent', + credentialExpirySeconds: 3600, projectHardSpendLimitCents: 100, projectHardSpendCurrency: 'USD', + projectHardSpendInterval: 'month', brokerHost: '127.0.0.1', brokerPort: 8789, + providerModel: 'gpt-6-luna', providerScope: 'api.responses.write', + bearerTokenFile: tokenFile, grantExpiresAt: new Date(Date.now() + 60_000).toISOString(), + }; + const output = []; + const io = { stat: fs.lstat, aclCheck: async () => {}, addReadAcl: async () => {}, verifyCode: async () => {} }; + return { root, handoff, plan, tokenFile, output, io, + options: { credentialFile, tokenFile, output: { write: text => output.push(text) }, io, port: 0, + transport: async () => { throw new Error('FAKE_UNEXPECTED_PROVIDER_CALL'); } }, + cleanup: () => fs.rm(root, { recursive: true, force: true }) }; +} +async function close(server) { + if (!server?.listening) return; + const closed = once(server, 'close'); + stopHttpServer(server); + await closed; +} +async function waitForRemoval(file) { + for (let i = 0; i < 500; i++) { + try { await fs.lstat(file); } + catch (error) { if (error.code === 'ENOENT') return; throw error; } + await new Promise(resolve => setTimeout(resolve, 10)); + } + assert.fail('Launch authority was not removed after the listener closed.'); +} +async function availablePort() { + const reservation = net.createServer(); + await new Promise(resolve => reservation.listen(0, '127.0.0.1', resolve)); + const port = reservation.address().port; + await new Promise(resolve => reservation.close(resolve)); + return port; +} +async function authStatus(server, token) { + return (await fetch('http://127.0.0.1:' + server.address().port + '/mcp', { + method: 'POST', headers: { authorization: 'Bearer ' + token, 'content-type': 'application/json', + accept: 'application/json, text/event-stream' }, + body: JSON.stringify({ jsonrpc: '2.0', id: 1, method: 'tools/list', params: {} }), + })).status; +} + +test('standalone restart rejects prior launch authority and publishes a fresh token', async () => { + const fx = await fixture(); + let server; + const grantExpiresAt = fx.plan.grantExpiresAt; + try { + server = await launchOwnerBroker(fx.plan, fx.options); + const first = (await fs.readFile(fx.tokenFile, 'utf8')).trim(); + await close(server); + await waitForRemoval(fx.tokenFile); + server = await launchOwnerBroker(fx.plan, fx.options); + const second = (await fs.readFile(fx.tokenFile, 'utf8')).trim(); + assert.notEqual(first, second); + assert.notEqual(first, OLD_TOKEN); + assert.match(first, /^[A-Za-z0-9_-]{43}$/); + assert.match(second, /^[A-Za-z0-9_-]{43}$/); + assert.equal(await authStatus(server, first), 401); + assert.equal(await authStatus(server, second), 200); + assert.equal(fx.plan.grantExpiresAt, grantExpiresAt); + assert.deepEqual(fx.output, Array(2).fill('Owner broker is listening on reviewed loopback port 8789 until its grant ends (at most one hour).\n')); + for (const marker of [KEY, first, second, 'Bearer ']) assert.equal(fx.output.join('').includes(marker), false); + } finally { await close(server); await waitForRemoval(fx.tokenFile); await fx.cleanup(); } +}); + +test('standalone publishes token read access only after binding the real listener', async () => { + const fx = await fixture(); + const port = await availablePort(); + let server; + let publicationSawBoundListener = false; + try { + fx.io.addReadAcl = async (file, agent) => { + assert.equal(file, fx.tokenFile); + assert.equal(agent, 'enote-agent'); + const contender = net.createServer(); + publicationSawBoundListener = await new Promise(resolve => { + contender.once('error', error => resolve(error.code === 'EADDRINUSE')); + contender.listen(port, '127.0.0.1', () => contender.close(() => resolve(false))); + }); + }; + server = await launchOwnerBroker(fx.plan, { ...fx.options, port }); + assert.equal(publicationSawBoundListener, true); + assert.equal((await fs.stat(fx.tokenFile)).mode & 0o777, 0o600); + } finally { await close(server); await waitForRemoval(fx.tokenFile); await fx.cleanup(); } +}); + +test('a failed standalone bind preserves an existing launch token without publishing authority', async () => { + const fx = await fixture(); + const incumbent = await launchOwnerBroker(fx.plan, fx.options); + const currentToken = await fs.readFile(fx.tokenFile, 'utf8'); + let publications = 0; + fx.io.addReadAcl = async () => { publications++; }; + try { + await assert.rejects(launchOwnerBroker(fx.plan, { ...fx.options, port: incumbent.address().port }), { code: 'EADDRINUSE' }); + assert.equal(publications, 0); + assert.equal(await fs.readFile(fx.tokenFile, 'utf8'), currentToken); + assert.equal(await authStatus(incumbent, currentToken.trim()), 200); + assert.equal(fx.output.length, 1); + } finally { await close(incumbent); await waitForRemoval(fx.tokenFile); await fx.cleanup(); } +}); + +test('a failed standalone publication closes its listener and removes its own partial token', async () => { + const fx = await fixture(); + const port = await availablePort(); + fx.io.addReadAcl = async () => { throw new Error('FAKE_ACL_FAILURE'); }; + try { + await assert.rejects(launchOwnerBroker(fx.plan, { ...fx.options, port }), /FAKE_ACL_FAILURE/); + await assert.rejects(fs.lstat(fx.tokenFile), { code: 'ENOENT' }); + await assert.rejects(fetch('http://127.0.0.1:' + port + '/mcp')); + const probe = net.createServer(); + await new Promise((resolve, reject) => { probe.once('error', reject); probe.listen(port, '127.0.0.1', resolve); }); + await new Promise(resolve => probe.close(resolve)); + assert.deepEqual(fx.output, []); + } finally { await fx.cleanup(); } +}); + +test('standalone creates first-launch authority and stops at the original reviewed grant deadline', { timeout: 10_000 }, async () => { + const fx = await fixture(); + await fs.unlink(fx.tokenFile); + const deadline = Date.now() + 2000; + fx.plan.grantExpiresAt = new Date(deadline).toISOString(); + let server; + try { + server = await launchOwnerBroker(fx.plan, fx.options); + assert.equal(server.listening, true); + assert.equal(await authStatus(server, (await fs.readFile(fx.tokenFile, 'utf8')).trim()), 200); + await once(server, 'close'); + await waitForRemoval(fx.tokenFile); + assert.ok(Date.now() >= deadline); + assert.equal(fx.plan.grantExpiresAt, new Date(deadline).toISOString()); + } finally { await close(server); await fx.cleanup(); } +}); + +test('token cleanup accepts only its inode/dev lease and cannot delete a successor publication', async () => { + const fx = await fixture(); + let first; let second; + try { + first = await writeAgentToken(fx.plan, OLD_TOKEN, fx.io); + const info = await fs.lstat(fx.tokenFile); + assert.equal(first.dev, info.dev); + assert.equal(first.ino, info.ino); + await removeAgentToken(fx.tokenFile); + await removeAgentToken({ ...first }); + assert.equal(await fs.readFile(fx.tokenFile, 'utf8'), OLD_TOKEN + '\n'); + const successorToken = 'FAKE_SUCCESSOR_TOKEN_FOR_TESTS_ONLY_0123456789'; + second = await writeAgentToken(fx.plan, successorToken, fx.io); + assert.notEqual(second.ino, first.ino); + await removeAgentToken(first); + await removeAgentToken(first); + assert.equal(await fs.readFile(fx.tokenFile, 'utf8'), successorToken + '\n'); + await removeAgentToken(second); + await assert.rejects(fs.lstat(fx.tokenFile), { code: 'ENOENT' }); + } finally { await removeAgentToken(first); await removeAgentToken(second); await fx.cleanup(); } +}); From 2ab992dc1bb3d7110b109567a9405e69470dbaf4 Mon Sep 17 00:00:00 2001 From: NNIIOVO Date: Wed, 4 Nov 2026 12:09:46 -0600 Subject: [PATCH 3/4] fix(companion): reserve listener through token cleanup --- companion/README.md | 2 +- companion/isolation/PLAN.md | 4 +- companion/isolation/integrated-broker.mjs | 30 +++-- companion/isolation/owner-launcher.mjs | 35 +++-- companion/isolation/token-handoff.mjs | 16 ++- companion/src/http.mjs | 35 ++++- companion/test/integrated.test.mjs | 146 +++++++++++++++++++++ companion/test/mcp.test.mjs | 85 ++++++++++++ companion/test/owner-launcher.test.mjs | 151 ++++++++++++++++++++++ 9 files changed, 470 insertions(+), 34 deletions(-) diff --git a/companion/README.md b/companion/README.md index e4f8341..42c322a 100644 --- a/companion/README.md +++ b/companion/README.md @@ -10,7 +10,7 @@ The selected deployment is: agent/code → stdio bridge → local owner broker The bridge lists the tool even while the broker is off and reads the token again on each call, so a new Start needs no restart of the agent's MCP client. With no token or no listener, a call returns one fixed error telling the agent to ask the owner. -Both protected launchers bind the real loopback listener before publishing a fresh random 32-byte base64url token with the agent's read ACL. A failed bind leaves any existing launch's token untouched. The standalone launcher also rotates the token on every launch, without extending the reviewed plan's grant deadline. Cleanup uses the created file's inode/device lease, retains its open handle until cleanup, and removes only that publication; stale cleanup cannot remove a successor already published in the owner-controlled handoff directory. A publication failure closes the listener and cleans its own partial publication. +Both protected launchers bind the real loopback listener before publishing a fresh random 32-byte base64url token with the agent's read ACL. A failed bind leaves any existing launch's token untouched. The standalone launcher also rotates the token on every launch, without extending the reviewed plan's grant deadline. Cleanup uses the created file's inode/device lease, retains its open handle until cleanup, and removes only that publication; stale cleanup cannot remove a successor already published in the owner-controlled handoff directory. During startup, provider admissions remain disabled until publication protection fully validates and the grant is rechecked. Shutdown disables admissions and cancels active calls first, then keeps the listener bound through owned token cleanup before releasing the port. The same ordering covers failed publication and startup expiry, so a cooperating restart cannot publish a successor during the old cleanup. Standard close callbacks and repeated close calls retain their behavior. This closes the demonstrated capture-before-bind and later-token-reuse path. The bearer transport does not authenticate the server to the client; it is not complete endpoint authentication or protection against malicious software running as the owner. diff --git a/companion/isolation/PLAN.md b/companion/isolation/PLAN.md index 456e2db..47ab574 100644 --- a/companion/isolation/PLAN.md +++ b/companion/isolation/PLAN.md @@ -41,11 +41,11 @@ Use a trusted absolute Node executable; replace paths locally without putting ac - fixtures.mjs prepare takes the reviewed plan path and an existing protected parent; it creates only a fresh enote-isolation-* directory with 0700 directories and 0600 fictional members. Its output is a nonsecret private fixture manifest, never a GitHub artifact. - agent-probe.mjs takes the reviewed plan, copied nonsecret fixture manifest and a new private agent workspace. It checks actual UID/name, privileged groups and noninteractive sudo before touching fixtures. It then checks its own workspace positively and requires permission denial for owner read/list/write/create/rename/unlink, fake code/policy/dependency writes and an agent-created symlink into the owner fixture. - fixtures.mjs verify-owner takes the original owner-record.json and requires unchanged fixture ownership, modes and hashes. Preserve evidence on failure. Remove only these explicitly created disposable members after owner verification; no broad recursive cleanup or permission command is provided. -- owner-launcher.mjs takes reviewed plan, confirmed credential path and delegated-token path. It requires the owner UID and the token path to match the plan, checks the owner-prepared handoff directory, and creates a fresh token only after successfully binding the listener. A failed bind leaves an existing publication untouched. A failed publication closes the listener and cleans only the file created by that launch. Application/SDK imports are delayed until protection checks finish, but an independent trusted preflight is still mandatory. +- owner-launcher.mjs takes reviewed plan, confirmed credential path and delegated-token path. It requires the owner UID and the token path to match the plan, checks the owner-prepared handoff directory, and creates a fresh token only after successfully binding the listener. A failed bind leaves an existing publication untouched. Provider admissions stay disabled until publication fully validates and the original grant is rechecked. Shutdown and failed publication disable admissions, cancel active calls, and clean only that launch's publication while keeping the listener bound; only then is the port released for a successor. Application/SDK imports are delayed until protection checks finish, but an independent trusted preflight is still mandatory. - write-agent-key.mjs takes one output path and reads the NEW Agent Key on stdin (`pbpaste | node write-agent-key.mjs ~/enote-agent-key.env`), never as an argument. It writes only a value shaped like an OpenAI API key (sk-, then 37 to 509 letters, digits, - or _; no spaces or line breaks). It refuses Admin keys (sk-admin-), Anthropic (sk-ant-) and OpenRouter (sk-or-) keys, and input over 4096 bytes however the reads split it; an OpenAI key copied from the Vault would still pass. It writes owner-only 0600 without following a symlink or writing through a hard link, refuses a file with an ACL allow entry (set on it or inherited from its folder, checked before and after writing), then asks Time Machine to leave the file out (tmutil addexclusion), which may fail without stopping it. No message includes the value. - agent-bridge.mjs requires the actual selected Standard UID. It reads only the plan and the separate delegated token and exposes stdio to the local MCP client. It forwards only the one fixed bounded operation. It lists the tool even while the broker is off and reads the token again on each call, so a new Start needs no restart; with no token or no listener, a call returns one fixed error. -The delegated token may need a fresh directory under /Users/Shared if the owner home is not traversable. This exception is only for the limited local token, never the provider key. Require root-owned sticky /Users/Shared, a new owner-owned 0700 child, exactly one agent-user search ACL on that child and exactly one agent-user read ACL on its 0600 token file. Other allow ACLs fail closed. Never weaken an existing owner parent to make token sharing work. An owner-reviewed targeted handoff-directory preparation step remains pending; this package executes none. Each protected broker launch creates its own 0600 token and adds the exact agent read ACL after binding. Cleanup holds an inode/device lease with an open file handle, so stale cleanup cannot delete a successor already published in this owner-controlled directory. These lifetime checks close capture-before-bind and token reuse; the bearer transport does not authenticate a server to its client or protect against malicious software running as the owner. +The delegated token may need a fresh directory under /Users/Shared if the owner home is not traversable. This exception is only for the limited local token, never the provider key. Require root-owned sticky /Users/Shared, a new owner-owned 0700 child, exactly one agent-user search ACL on that child and exactly one agent-user read ACL on its 0600 token file. Other allow ACLs fail closed. Never weaken an existing owner parent to make token sharing work. An owner-reviewed targeted handoff-directory preparation step remains pending; this package executes none. Each protected broker launch creates its own 0600 token and adds the exact agent read ACL after binding. Cleanup holds an inode/device lease with an open file handle, and the protected HTTP close lifecycle reserves the listener until that cleanup finishes, so cooperating launches cannot publish a successor between the ownership check and unlink. Stale cleanup also leaves an already-replaced publication untouched. These lifetime checks close capture-before-bind and token reuse; the bearer transport does not authenticate a server to its client or protect against malicious software running as the owner. ## Additional fictional checks and stop rules diff --git a/companion/isolation/integrated-broker.mjs b/companion/isolation/integrated-broker.mjs index 0a43740..a6e083f 100644 --- a/companion/isolation/integrated-broker.mjs +++ b/companion/isolation/integrated-broker.mjs @@ -147,8 +147,9 @@ export async function startIntegratedBroker({ try { await io.verifyCode(plan.ownerUid); } catch { fail('code_unprotected'); } try { await checkHandoffDirectory(plan, io); } catch { fail('handoff_unavailable'); } // Delay SDK imports until the protected-tree checks pass, as owner-launcher.mjs does. - const { createGenerator, guardGenerate } = await import('../src/generate.mjs'); + const { createGenerator, guardGenerate, CompanionError } = await import('../src/generate.mjs'); const { startHttpServer, stopHttpServer } = await import('../src/http.mjs'); + let ready = false; const isActive = grantGuard(expiresAt, { now, mono }); if (!isActive()) fail('grant_inactive'); let apiKey; @@ -158,17 +159,28 @@ export async function startIntegratedBroker({ if (!/^[A-Za-z0-9_-]{32,256}$/.test(token) || token === apiKey) { apiKey = ''; fail('internal'); } let generate; try { - generate = guardGenerate(createGenerator({ apiKey, ...(transport ? { transport } : {}) }), isActive); + const generator = createGenerator({ apiKey, ...(transport ? { transport } : {}) }); + generate = guardGenerate(async (args, options) => { + if (!ready) throw new CompanionError('helper_off'); + return generator(args, options); + }, isActive); } catch { apiKey = ''; fail('credential_invalid'); } apiKey = ''; // Best effort only; JavaScript strings cannot be guaranteed erased. if (!isActive()) fail('grant_inactive'); + let publication; let server; - try { server = await startHttpServer({ generate, bearerToken: token, port: port ?? plan.brokerPort }); } + try { server = await startHttpServer({ generate, bearerToken: token, port: port ?? plan.brokerPort, + beforeClose: () => { ready = false; return removeAgentToken(publication); } }); } catch { fail('port_unavailable'); } - let publication; - try { publication = await writeAgentToken(plan, token, io); } - catch { stopHttpServer(server); fail('handoff_unavailable'); } - if (!isActive()) { stopHttpServer(server); await removeAgentToken(publication); fail('grant_inactive'); } + const closed = new Promise(resolve => server.once('close', resolve)); + const closeServer = () => { stopHttpServer(server); return closed; }; + try { + publication = await writeAgentToken(plan, token, io, { + cleanupFailure: async lease => { publication = lease; await closeServer(); }, + }); + } + catch { await closeServer(); fail('handoff_unavailable'); } + if (!isActive()) { await closeServer(); fail('grant_inactive'); } let stopped = false; let resolveDone; @@ -197,8 +209,7 @@ export async function startIntegratedBroker({ // called from the data handler, the stream starts reading again once the handler returns. input.destroy?.(); for (const event of ['SIGTERM', 'SIGINT']) signals.removeListener?.(event, onSignal); - stopHttpServer(server); - await removeAgentToken(publication); + await closeServer(); emit(encodeEvent({ event: 'stopped', reason })); resolveDone(reason); return done; @@ -206,6 +217,7 @@ export async function startIntegratedBroker({ input.on('data', onData); input.once('end', onEnd); input.once('close', onEnd); for (const event of ['SIGTERM', 'SIGINT']) signals.once?.(event, onSignal); input.resume?.(); + ready = true; emit(encodeEvent({ event: 'ready', expiresAt: Math.floor(expiresAt / 1000) })); return { stop, done, server, expiresAt }; } diff --git a/companion/isolation/owner-launcher.mjs b/companion/isolation/owner-launcher.mjs index c0dceb2..0e9ad6a 100644 --- a/companion/isolation/owner-launcher.mjs +++ b/companion/isolation/owner-launcher.mjs @@ -20,49 +20,58 @@ async function verifyCode(ownerUid) { const defaultIo = { ...tokenIo, verifyCode }; export async function launchOwnerBroker(plan, { - credentialFile, tokenFile, output = process.stderr, io = defaultIo, transport, + credentialFile, tokenFile, output = process.stderr, io = defaultIo, transport, now = Date.now, port, // tests only; the production entry point always uses the reviewed plan's port } = {}) { requireRole(plan, 'owner'); - assertGrantActive(plan); + assertGrantActive(plan, now()); if (process.platform !== 'darwin') throw new Error('This launcher is for the reviewed Mac only.'); await io.verifyCode(plan.ownerUid); if (tokenFile !== plan.bearerTokenFile) throw new Error('Delegated token path differs from the reviewed plan.'); await checkHandoffDirectory(plan, io); // An independent owner preflight must already trust this launcher and its Node runtime. // Delay application/SDK imports until the remaining protected tree checks complete. - const { createGenerator, guardGenerate } = await import('../src/generate.mjs'); + const { createGenerator, guardGenerate, CompanionError } = await import('../src/generate.mjs'); const { startHttpServer, stopHttpServer } = await import('../src/http.mjs'); // Grant again immediately before the credential read and before the broker opens. - const expiresAt = assertGrantActive(plan); + const expiresAt = assertGrantActive(plan, now()); let apiKey = await readConfirmedCredential(credentialFile, plan.ownerUid); const bearerToken = randomBytes(32).toString('base64url'); if (apiKey === bearerToken) { apiKey = ''; throw new Error('Separate credentials are required.'); } - const isActive = grantGuard(expiresAt); - const generate = guardGenerate(createGenerator({ apiKey, ...(transport ? { transport } : {}) }), isActive); + let ready = false; + const isActive = grantGuard(expiresAt, { now }); + const generator = createGenerator({ apiKey, ...(transport ? { transport } : {}) }); + const generate = guardGenerate(async (args, options) => { + if (!ready) throw new CompanionError('helper_off'); + return generator(args, options); + }, isActive); apiKey = ''; // Best effort only; JavaScript strings cannot be guaranteed erased. - assertGrantActive(plan); + assertGrantActive(plan, now()); if (!isActive()) throw new Error(GRANT_INACTIVE); - const server = await startHttpServer({ generate, bearerToken, port: port ?? plan.brokerPort }); let publication; + const server = await startHttpServer({ generate, bearerToken, port: port ?? plan.brokerPort, + beforeClose: () => { ready = false; return removeAgentToken(publication); } }); + const closed = new Promise(resolve => server.once('close', resolve)); + const closeServer = () => { stopHttpServer(server); return closed; }; try { - publication = await writeAgentToken(plan, bearerToken, io); + publication = await writeAgentToken(plan, bearerToken, io, { + cleanupFailure: async lease => { publication = lease; await closeServer(); }, + }); if (!isActive()) throw new Error(GRANT_INACTIVE); } catch (error) { - stopHttpServer(server); - await removeAgentToken(publication); + await closeServer(); throw error; } const stop = () => stopHttpServer(server); // Stop at the earlier of the absolute grant deadline and one hour from now. Supplemental // to, never a substitute for, provider-enforced key expiry. - const lifetime = setTimeout(stop, Math.max(0, Math.min(expiresAt - Date.now(), MAX_GRANT_MS))); + const lifetime = setTimeout(stop, Math.max(0, Math.min(expiresAt - now(), MAX_GRANT_MS))); for (const event of ['SIGINT', 'SIGTERM']) process.once(event, stop); server.once('close', () => { clearTimeout(lifetime); for (const event of ['SIGINT', 'SIGTERM']) process.removeListener(event, stop); - void removeAgentToken(publication); }); + ready = true; output.write('Owner broker is listening on reviewed loopback port 8789 until its grant ends (at most one hour).\n'); return server; } diff --git a/companion/isolation/token-handoff.mjs b/companion/isolation/token-handoff.mjs index 65c2344..7adbd29 100644 --- a/companion/isolation/token-handoff.mjs +++ b/companion/isolation/token-handoff.mjs @@ -27,7 +27,7 @@ export async function checkHandoffDirectory(plan, { stat = lstat, aclCheck = che // Call only after successfully binding this launch's listener. Retain the open handle until // cleanup, so an unlinked publication's inode cannot be recycled into a successor's lease. -export async function writeAgentToken(plan, token, io = tokenIo) { +export async function writeAgentToken(plan, token, io = tokenIo, { cleanupFailure = removeAgentToken } = {}) { const file = plan.bearerTokenFile; try { const stale = await io.stat(file); @@ -39,7 +39,7 @@ export async function writeAgentToken(plan, token, io = tokenIo) { try { const info = await handle.stat(); lease = Object.freeze({ file, dev: info.dev, ino: info.ino }); - publications.set(lease, handle); + publications.set(lease, { handle, stat: io.stat }); await handle.writeFile(token + '\n'); await handle.sync(); await io.addReadAcl(file, plan.agentShortName); @@ -47,20 +47,22 @@ export async function writeAgentToken(plan, token, io = tokenIo) { if (!matches(await lstat(file), lease)) unavailable(); return lease; } catch (error) { - if (lease) await removeAgentToken(lease); + if (lease) await cleanupFailure(lease); else await handle.close(); throw error; } } // A pathname is never cleanup authority. Only a lease issued by this module can remove the -// exact publication it owns. There is no same-owner malicious rename protection in Node's unlink. +// exact publication it owns. Protected callers keep the listener reserved throughout this +// check and unlink, serializing legitimate launches. Malicious owner renames remain out of scope. export async function removeAgentToken(lease) { - const handle = lease && publications.get(lease); - if (!handle) return; + const publication = lease && publications.get(lease); + if (!publication) return; + const { handle, stat } = publication; publications.delete(lease); try { - if (matches(await lstat(lease.file), lease)) await unlink(lease.file); + if (matches(await stat(lease.file), lease)) await unlink(lease.file); } catch { /* gone, replaced, or no longer removable */ } finally { await handle.close(); } } diff --git a/companion/src/http.mjs b/companion/src/http.mjs index 150f35b..aa821ff 100644 --- a/companion/src/http.mjs +++ b/companion/src/http.mjs @@ -36,17 +36,20 @@ async function readBody(request) { } return JSON.parse(new TextDecoder('utf-8', { fatal: true }).decode(Buffer.concat(chunks))); } -export async function startHttpServer({ generate, bearerToken, port = 8789 }) { +export async function startHttpServer({ generate, bearerToken, port = 8789, beforeClose }) { if (typeof bearerToken !== 'string' || !/^[A-Za-z0-9_-]{32,256}$/.test(bearerToken) || - !Number.isInteger(port) || port < 0 || port > 65535) { + !Number.isInteger(port) || port < 0 || port > 65535 || + (beforeClose !== undefined && typeof beforeClose !== 'function')) { throw new Error('Invalid HTTP configuration.'); } // Stateless HTTP transports still share cancellation state for the one scoped client. const activeCalls = new Map(); + let shuttingDown = false; const server = http.createServer(async (request, response) => { let mcp; let transport; try { + if (shuttingDown) { reply(response, 503); return; } const actualPort = server.address().port; const hosts = ['127.0.0.1:' + actualPort, 'localhost:' + actualPort]; const origins = hosts.map(host => 'http://' + host); @@ -63,6 +66,7 @@ export async function startHttpServer({ generate, bearerToken, port = 8789 }) { reply(response, 415); return; } const body = await readBody(request); + if (shuttingDown) { reply(response, 503); return; } // A single bounded JSON-RPC message; batch calls are deliberately unsupported. if (!body || typeof body !== 'object' || Array.isArray(body)) { reply(response, 400); return; } if (body.method === 'notifications/cancelled') { @@ -72,6 +76,7 @@ export async function startHttpServer({ generate, bearerToken, port = 8789 }) { response.writeHead(202, { 'cache-control': 'no-store' }); response.end(); return; } const scopedGenerate = async (args, { signal } = {}) => { + if (shuttingDown) throw new CompanionError('grant_expired'); const id = body.id; if (activeCalls.has(id)) throw new CompanionError('busy'); const controller = new AbortController(); @@ -105,6 +110,32 @@ export async function startHttpServer({ generate, bearerToken, port = 8789 }) { server.on('clientError', (_error, socket) => { socket.end('HTTP/1.1 400 Bad Request\r\nConnection: close\r\n\r\n'); }); + if (beforeClose) { + // Only protected callers install this per-instance barrier. Keep the real listener bound + // until its publication cleanup finishes; a successor process must still fail its bind. + const close = server.close.bind(server); + const pendingCloses = []; + let released = false; + const release = () => { + released = true; + // Calling the native close once for each queued call preserves its callback behavior: + // the first completes normally, repeated calls report ERR_SERVER_NOT_RUNNING. + for (const args of pendingCloses.splice(0)) close(...args); + server.closeAllConnections(); + }; + server.close = (...args) => { + if (released) return close(...args); + pendingCloses.push(args); + if (!shuttingDown) { + shuttingDown = true; + for (const controller of activeCalls.values()) controller.abort(); + server.closeAllConnections(); + // Cleanup failures must still finish shutdown; their text never leaves this boundary. + void Promise.resolve().then(beforeClose).then(release, release); + } + return server; + }; + } await new Promise((resolve, reject) => { server.once('error', reject); server.listen(port, '127.0.0.1', resolve); diff --git a/companion/test/integrated.test.mjs b/companion/test/integrated.test.mjs index 474cecd..aa87c9d 100644 --- a/companion/test/integrated.test.mjs +++ b/companion/test/integrated.test.mjs @@ -211,6 +211,152 @@ test('integrated expiry during startup refuses before listener or token publicat } finally { await fx.cleanup(); } }); +test('integrated shutdown reserves the listener until checked token cleanup completes', { timeout: 10_000 }, async () => { + const fx = await fixture(); + let armed = false; + let announcePaused; let resume; + const paused = new Promise(resolve => { announcePaused = resolve; }); + const released = new Promise(resolve => { resume = resolve; }); + const { io } = fakeIo({ stat: async file => { + const info = await fs.lstat(file); + if (armed && file === fx.plan.bearerTokenFile) { + armed = false; + announcePaused(); + await released; + } + return info; + } }); + let running; let attempted; let successor; let stopping; + try { + running = await start(fx, { io }); + const port = running.broker.server.address().port; + armed = true; + stopping = running.broker.stop('owner'); + await paused; + let bindError; + try { attempted = await start(fx, { port }); } catch (error) { bindError = error; } + assert.equal(bindError?.code, 'port_unavailable'); + assert.equal(running.broker.server.listening, true); + // No new provider admission while shutdown is waiting on the filesystem. + await assert.rejects(callTool(running.url, TOKEN)); + assert.deepEqual(running.calls, []); + resume(); + await stopping; + successor = await start(fx, { port }); + assert.equal(await fs.readFile(fx.plan.bearerTokenFile, 'utf8'), TOKEN + '\n'); + assert.equal((await callTool(successor.url, TOKEN)).isError, undefined); + } finally { + resume(); + await stopping; + await attempted?.broker.stop('owner'); + await successor?.broker.stop('owner'); + await running?.broker.stop('owner'); + await fx.cleanup(); + } +}); + +test('integrated post-publication expiry keeps the listener reserved through token cleanup', { timeout: 10_000 }, async () => { + const fx = await fixture(); + const reservation = net.createServer(); + await new Promise(resolve => reservation.listen(0, '127.0.0.1', resolve)); + const port = reservation.address().port; + await new Promise(resolve => reservation.close(resolve)); + let wall = Date.now(); let armed = false; + let announcePaused; let resume; + const paused = new Promise(resolve => { announcePaused = resolve; }); + const released = new Promise(resolve => { resume = resolve; }); + const { io } = fakeIo({ + aclCheck: async file => { + if (file === fx.plan.bearerTokenFile) { wall += MAX_GRANT_MS; armed = true; } + }, + stat: async file => { + const info = await fs.lstat(file); + if (armed && file === fx.plan.bearerTokenFile) { + armed = false; announcePaused(); await released; + } + return info; + }, + }); + const failedStartup = start(fx, { io, port, now: () => wall }).then(() => undefined, error => error); + let attempted; let successor; + try { + await paused; + let bindError; + try { attempted = await start(fx, { port }); } catch (error) { bindError = error; } + assert.equal(bindError?.code, 'port_unavailable'); + resume(); + assert.equal((await failedStartup)?.code, 'grant_inactive'); + await assert.rejects(fs.lstat(fx.plan.bearerTokenFile), { code: 'ENOENT' }); + successor = await start(fx, { port }); + assert.equal(await fs.readFile(fx.plan.bearerTokenFile, 'utf8'), TOKEN + '\n'); + } finally { + resume(); await failedStartup; + await attempted?.broker.stop('owner'); await successor?.broker.stop('owner'); await fx.cleanup(); + } +}); + +for (const failure of [false, true]) { + test('integrated publication validation gates provider admissions' + (failure ? ' on failure' : ' before ready'), { timeout: 10_000 }, async () => { + const fx = await fixture(); + const reservation = net.createServer(); + await new Promise(resolve => reservation.listen(0, '127.0.0.1', resolve)); + const port = reservation.address().port; + await new Promise(resolve => reservation.close(resolve)); + let checking; let finishCheck; let cleaning; let finishCleanup; + const validation = new Promise(resolve => { checking = resolve; }); + const checked = new Promise(resolve => { finishCheck = resolve; }); + const cleanup = new Promise(resolve => { cleaning = resolve; }); + const cleaned = new Promise(resolve => { finishCleanup = resolve; }); + let armCleanup = false; + const { io } = fakeIo({ + aclCheck: async file => { + if (file === fx.plan.bearerTokenFile) { + checking(); await checked; + if (failure) { armCleanup = true; throw new Error('FAKE_FINAL_ACL_FAILURE'); } + } + }, + stat: async file => { + const info = await fs.lstat(file); + if (armCleanup && file === fx.plan.bearerTokenFile) { + armCleanup = false; cleaning(); await cleaned; + } + return info; + }, + }); + const calls = []; + const startup = start(fx, { io, port, calls }).then(running => ({ running }), error => ({ error })); + let running; let attempted; let successor; + try { + await validation; + const url = 'http://127.0.0.1:' + port + '/mcp'; + assert.equal((await fs.readFile(fx.plan.bearerTokenFile, 'utf8')).trim(), TOKEN); + assert.equal((await callTool(url, TOKEN)).isError, true); + assert.deepEqual(calls, []); + finishCheck(); + if (failure) { + await cleanup; + assert.equal((await fetch(url, { method: 'POST' })).status, 503); + let bindError; + try { attempted = await start(fx, { port }); } catch (error) { bindError = error; } + assert.equal(bindError?.code, 'port_unavailable'); + finishCleanup(); + assert.equal((await startup).error?.code, 'handoff_unavailable'); + successor = await start(fx, { port }); + assert.equal(await fs.readFile(fx.plan.bearerTokenFile, 'utf8'), TOKEN + '\n'); + } else { + running = (await startup).running; + assert.equal((await callTool(running.url, TOKEN)).isError, undefined); + assert.deepEqual(calls, [true]); + } + } finally { + finishCheck(); finishCleanup(); + running ??= (await startup).running; + await running?.broker.stop('owner'); await attempted?.broker.stop('owner'); + await successor?.broker.stop('owner'); await fx.cleanup(); + } + }); +} + test('control pipe EOF, malformed control and oversized control all stop the broker', async () => { for (const [send, reason] of [[null, 'parent'], ['{"v":1,"op":"restart"}\n', 'error'], ['x'.repeat(1024), 'error']]) { const fx = await fixture(); diff --git a/companion/test/mcp.test.mjs b/companion/test/mcp.test.mjs index 70b5c77..0f96ffa 100644 --- a/companion/test/mcp.test.mjs +++ b/companion/test/mcp.test.mjs @@ -316,3 +316,88 @@ test('HTTP shutdown cancels an admitted operation and closes active connections await pending; } }); + +test('protected HTTP close waits for one cleanup, rejects late admissions and preserves close callbacks', { timeout: 10_000 }, async () => { + let begin; let resume; let admitted; + const cleaning = new Promise(resolve => { begin = resolve; }); + const released = new Promise(resolve => { resume = resolve; }); + const started = new Promise(resolve => { admitted = resolve; }); + let hooks = 0; let calls = 0; let capturedSignal; + const generate = createGenerator({ apiKey: KEY, transport: async ({ signal }) => { + calls++; + capturedSignal = signal; + admitted(); + return new Promise(() => {}); + } }); + const server = await startHttpServer({ generate, bearerToken: TOKEN, port: 0, + beforeClose: async () => { hooks++; begin(); await released; } }); + const address = 'http://127.0.0.1:' + server.address().port + '/mcp'; + const headers = { authorization: 'Bearer ' + TOKEN, 'content-type': 'application/json', + accept: 'application/json, text/event-stream' }; + const body = JSON.stringify({ jsonrpc: '2.0', id: 'fictional-close', method: 'tools/call', + params: { name: 'generate_text', arguments: { text: 'Fiction.' } } }); + const pending = fetch(address, { method: 'POST', headers, body }) + .then(response => response.arrayBuffer(), error => error); + let slowRequest; + let slowOutcome; + const callbacks = []; + try { + await started; + // A real request is already parsing its body when close begins. + const parsing = new Promise(resolve => server.once('request', resolve)); + slowOutcome = new Promise(resolve => { + slowRequest = http.request(address, { method: 'POST', headers }, response => { + response.resume(); + response.once('end', () => resolve(response.statusCode)); + response.once('error', resolve); + }); + slowRequest.once('error', resolve); + slowRequest.write(body.slice(0, 20)); + }); + await parsing; + const first = new Promise(resolve => assert.equal(server.close(error => { + callbacks.push(error?.code); resolve(error); + }), server)); + const repeated = new Promise(resolve => assert.equal(server.close(error => { + callbacks.push(error?.code); resolve(error); + }), server)); + await cleaning; + assert.equal(capturedSignal.aborted, true); + assert.equal(server.listening, true); + assert.deepEqual(callbacks, []); + slowRequest.end(body.slice(20)); + const late = await fetch(address, { method: 'POST', headers, body }); + assert.equal(late.status, 503); + assert.deepEqual(await late.json(), { error: 'Request rejected.' }); + assert.equal(calls, 1); + resume(); + assert.equal(await first, undefined); + assert.equal((await repeated)?.code, 'ERR_SERVER_NOT_RUNNING'); + assert.deepEqual(callbacks, [undefined, 'ERR_SERVER_NOT_RUNNING']); + assert.equal(server.listening, false); + assert.equal((await new Promise(resolve => server.close(resolve)))?.code, 'ERR_SERVER_NOT_RUNNING'); + assert.equal(hooks, 1); + await pending; + await slowOutcome; + assert.equal(calls, 1); + assert.equal(await new Promise((resolve, reject) => server.getConnections((error, count) => + error ? reject(error) : resolve(count))), 0); + } finally { + resume(); + slowRequest?.destroy(); + await new Promise(resolve => server.close(resolve)); + server.closeAllConnections(); + await pending; + await slowOutcome; + } +}); + +test('protected HTTP cleanup rejection still releases the listener without repeating cleanup', async () => { + let hooks = 0; + const server = await startHttpServer({ generate: generator(), bearerToken: TOKEN, port: 0, + beforeClose: async () => { hooks++; throw new Error('FAKE_PRIVATE_CLEANUP_ERROR'); } }); + await new Promise(resolve => server.close(resolve)); + assert.equal(server.listening, false); + assert.equal((await new Promise(resolve => server.close(resolve)))?.code, 'ERR_SERVER_NOT_RUNNING'); + assert.equal(hooks, 1); +}); diff --git a/companion/test/owner-launcher.test.mjs b/companion/test/owner-launcher.test.mjs index 259e470..42f8187 100644 --- a/companion/test/owner-launcher.test.mjs +++ b/companion/test/owner-launcher.test.mjs @@ -175,3 +175,154 @@ test('token cleanup accepts only its inode/dev lease and cannot delete a success await assert.rejects(fs.lstat(fx.tokenFile), { code: 'ENOENT' }); } finally { await removeAgentToken(first); await removeAgentToken(second); await fx.cleanup(); } }); + +test('standalone close reserves the listener until checked token cleanup completes', { timeout: 10_000 }, async () => { + const fx = await fixture(); + let armed = false; + let announcePaused; let resume; + const paused = new Promise(resolve => { announcePaused = resolve; }); + const released = new Promise(resolve => { resume = resolve; }); + fx.io.stat = async file => { + const info = await fs.lstat(file); + if (armed && file === fx.tokenFile) { + armed = false; + announcePaused(); + await released; + } + return info; + }; + let server; let attempted; let successor; let closed; + try { + server = await launchOwnerBroker(fx.plan, fx.options); + const port = server.address().port; + armed = true; + closed = once(server, 'close'); + assert.equal(server.close(), server); + await paused; + let bindError; + try { attempted = await launchOwnerBroker(fx.plan, { ...fx.options, io: { ...fx.io, stat: fs.lstat }, port }); } + catch (error) { bindError = error; } + assert.equal(bindError?.code, 'EADDRINUSE'); + assert.equal(server.listening, true); + resume(); + await closed; + successor = await launchOwnerBroker(fx.plan, { ...fx.options, port }); + const token = (await fs.readFile(fx.tokenFile, 'utf8')).trim(); + assert.match(token, /^[A-Za-z0-9_-]{43}$/); + assert.equal(await authStatus(successor, token), 200); + } finally { + resume(); + await closed; + await close(attempted); + await close(successor); + await close(server); + await fx.cleanup(); + } +}); + +test('standalone post-publication expiry reserves the listener through token cleanup', { timeout: 10_000 }, async () => { + const fx = await fixture(); + const port = await availablePort(); + let wall = Date.now(); + const deadline = wall + 60_000; + fx.plan.grantExpiresAt = new Date(deadline).toISOString(); + let armed = false; + let announcePaused; let resume; + const paused = new Promise(resolve => { announcePaused = resolve; }); + const released = new Promise(resolve => { resume = resolve; }); + fx.io.aclCheck = async file => { + if (file === fx.tokenFile) { + wall = deadline; + armed = true; + } + }; + fx.io.stat = async file => { + const info = await fs.lstat(file); + if (armed && file === fx.tokenFile) { + armed = false; announcePaused(); await released; + } + return info; + }; + const failedStartup = launchOwnerBroker(fx.plan, { ...fx.options, port, now: () => wall }).then(() => undefined, error => error); + let attempted; let successor; + const freshIo = { ...fx.io, stat: fs.lstat, aclCheck: async () => {} }; + try { + await paused; + const successorPlan = { ...fx.plan, grantExpiresAt: new Date(Date.now() + 60_000).toISOString() }; + let bindError; + try { attempted = await launchOwnerBroker(successorPlan, { ...fx.options, io: freshIo, port }); } + catch (error) { bindError = error; } + assert.equal(bindError?.code, 'EADDRINUSE'); + resume(); + assert.match((await failedStartup)?.message, /approved agent grant/); + await assert.rejects(fs.lstat(fx.tokenFile), { code: 'ENOENT' }); + successor = await launchOwnerBroker(successorPlan, { ...fx.options, io: freshIo, port }); + const token = (await fs.readFile(fx.tokenFile, 'utf8')).trim(); + assert.equal(await authStatus(successor, token), 200); + } finally { + resume(); await failedStartup; + await close(attempted); await close(successor); await fx.cleanup(); + } +}); + +for (const failure of [false, true]) { + test('standalone publication validation gates provider admissions' + (failure ? ' on failure' : ' before ready'), { timeout: 10_000 }, async () => { + const fx = await fixture(); + const port = await availablePort(); + let checking; let finishCheck; let cleaning; let finishCleanup; + const validation = new Promise(resolve => { checking = resolve; }); + const checked = new Promise(resolve => { finishCheck = resolve; }); + const cleanup = new Promise(resolve => { cleaning = resolve; }); + const cleaned = new Promise(resolve => { finishCleanup = resolve; }); + let armCleanup = false; let calls = 0; + fx.io.aclCheck = async file => { + if (file === fx.tokenFile) { + checking(); await checked; + if (failure) { armCleanup = true; throw new Error('FAKE_FINAL_ACL_FAILURE'); } + } + }; + fx.io.stat = async file => { + const info = await fs.lstat(file); + if (armCleanup && file === fx.tokenFile) { armCleanup = false; cleaning(); await cleaned; } + return info; + }; + const startup = launchOwnerBroker(fx.plan, { ...fx.options, port, + transport: async () => { calls++; throw new Error('FAKE_UNEXPECTED_PROVIDER_CALL'); }, + }).then(server => ({ server }), error => ({ error })); + let server; let attempted; let successor; + try { + await validation; + const token = (await fs.readFile(fx.tokenFile, 'utf8')).trim(); + const url = 'http://127.0.0.1:' + port + '/mcp'; + const response = await fetch(url, { method: 'POST', headers: { authorization: 'Bearer ' + token, + 'content-type': 'application/json', accept: 'application/json, text/event-stream' }, + body: JSON.stringify({ jsonrpc: '2.0', id: 1, method: 'tools/call', + params: { name: 'generate_text', arguments: { text: 'Fiction.' } } }) }); + assert.equal((await response.json()).result.isError, true); + assert.equal(calls, 0); + assert.equal(fx.output.length, 0); + finishCheck(); + if (failure) { + await cleanup; + assert.equal((await fetch(url, { method: 'POST' })).status, 503); + let bindError; + const freshIo = { ...fx.io, stat: fs.lstat, aclCheck: async () => {} }; + try { attempted = await launchOwnerBroker(fx.plan, { ...fx.options, io: freshIo, port }); } + catch (error) { bindError = error; } + assert.equal(bindError?.code, 'EADDRINUSE'); + finishCleanup(); + assert.match((await startup).error?.message, /FAKE_FINAL_ACL_FAILURE/); + successor = await launchOwnerBroker(fx.plan, { ...fx.options, io: freshIo, port }); + assert.equal(await authStatus(successor, (await fs.readFile(fx.tokenFile, 'utf8')).trim()), 200); + } else { + server = (await startup).server; + assert.equal(fx.output.length, 1); + assert.equal(await authStatus(server, token), 200); + } + } finally { + finishCheck(); finishCleanup(); + server ??= (await startup).server; + await close(server); await close(attempted); await close(successor); await fx.cleanup(); + } + }); +} From 28a010cf47c7610f08bdbec4cea050603e6a249b Mon Sep 17 00:00:00 2001 From: NNIIOVO Date: Wed, 4 Nov 2026 12:34:42 -0600 Subject: [PATCH 4/4] docs: record product UI direction and accessibility requirements --- PRODUCT.md | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) create mode 100644 PRODUCT.md diff --git a/PRODUCT.md b/PRODUCT.md new file mode 100644 index 0000000..b82ad86 --- /dev/null +++ b/PRODUCT.md @@ -0,0 +1,50 @@ +# Product + +## Register + +product + +## Users + +An individual owner keeping personal secrets on their own Mac or Windows computer. The owner needs to find a Note, deliberately reveal a Hidden Field, save changes, make a Backup, or recover access without a server account. Use the established names in [CONTEXT.md](CONTEXT.md) and explain actions in plain language. + +The optional Agent Companion serves a separate workflow: an owner gives tools in a distinct standard Mac account one restricted OpenAI text operation using a separately created Agent Key. An agent has no app interface to the Vault. The first private Agent Key entry flow is Mac only. + +## Product Purpose + +Keep Notes encrypted locally and make access, locking, saving and recovery understandable. The Vault and its WebView remain offline. The optional companion's fixed network operation belongs to its separate sandbox and never receives Vault content or metadata. + +Success means the owner can tell what an action will do before choosing it, what actually happened afterward, and how to cancel or recover. A local readiness check, an encrypted save, a running grant, provider restrictions and a working ChatGPT connection are different facts; the interface must identify each accurately. + +## Brand Personality + +Calm, direct, careful. Preserve the existing desktop utility's familiar system controls, concise labels and restrained Settings hierarchy. Use functional explanations and specific limitations instead of promotional language or absolute security claims. The existing app is the visual reference; no external brand reference has been supplied. + +## Anti-references + +- Marketing heroes, decorative photographs, repeated feature-card grids and oversized display typography inside the app. +- Decorative glass, gradients, pulsing requests, animated urgency and celebratory effects around sensitive actions. +- Cryptic technical errors, unexplained disabled controls, vague success messages and claims of protection that have not been observed. +- Custom controls that make ordinary keyboard, focus or cancellation behavior unfamiliar. + +## Design Principles + +1. **Keep the owner in control.** A request may produce a fixed notice. Opening private entry, saving or replacing a key, and starting a companion grant each require their own appropriate local action. Do not steal focus or start work automatically after saving. +2. **Make state truthful.** Show waiting, progress, cancellation, failure and completion in words. Distinguish local storage from provider validity and connection readiness. Report unverified behavior as unverified. +3. **Keep sensitive entry native.** An Agent Key belongs in the dedicated native secure control, never in chat or a WebView form. Status, errors, help and accessibility descriptions must not disclose its value or value-derived metadata. Vault credentials and content remain outside the companion workflow. +4. **Preserve familiar interaction.** Reuse the existing system font, semantic colors, grouped Settings rhythm and native controls. Keep active status and Stop visible, preserve focus across updates and keep the action hierarchy compact. +5. **Make interruption safe and understandable.** Cancel and Escape are ordinary outcomes. Focus loss and lifecycle changes invalidate private work. A late callback cannot revive a cancelled action, replace another request or restart a grant. + +## Accessibility & Inclusion + +Support keyboard-only use, visible focus, descriptive control labels and meaningful screen-reader status. Keep private input's native secure-control semantics without creating a plaintext mirror. Announce state changes without repeating unchanged polling results or moving focus. + +Target at least 4.5:1 contrast for normal text, including placeholders and button labels, and 3:1 for focus and required non-text control indicators. Check actual supported light, dark, accent and Increase Contrast settings; system colors alone are not evidence of a passed check. Use words alongside semantic color. Respect reduced motion and keep actions reachable at the minimum supported window size and larger text settings. + +These are implementation and acceptance requirements, not a claim of completed accessibility certification. Native authentication, secure accessibility, capture behavior and focus transitions need fictional-data physical checks in addition to source and Web fixture checks. + +## Context and Current Scope + +This document records existing product direction from the README, CONTEXT, native credential design, committed styles and the owner's approved private Agent Key design. It does not change the security policy in AGENTS.md or the ADRs. + +The approved private entry flow is still being implemented. Existing plaintext companion setup must not be described as encrypted custody. New encrypted readiness and ChatGPT completion copy may ship only with the corresponding implementation and evidence. Source-based UI guidance and acceptance details are recorded locally in `.scratch/private-agent-key-entry-20261104/UI-AUDIT.md`.