From fff6333773f2ecf31e211c5f79185db32b83caa7 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Thu, 2 Apr 2026 15:29:55 +0100 Subject: [PATCH] fix(exec): implement Windows argPattern allowlist flow --- src/agents/bash-tools.exec-host-gateway.ts | 37 +- src/gateway/protocol/schema/exec-approvals.ts | 1 + src/infra/exec-approvals-allow-always.test.ts | 45 +- src/infra/exec-approvals-allowlist.ts | 250 ++++++++++- src/infra/exec-approvals-analysis.test.ts | 388 ++++++++++++++++++ src/infra/exec-approvals-analysis.ts | 267 +++++++++++- src/infra/exec-approvals-store.test.ts | 127 ++++++ src/infra/exec-approvals.ts | 85 +++- src/infra/exec-command-resolution.ts | 104 ++++- src/node-host/invoke-system-run.ts | 54 +-- 10 files changed, 1247 insertions(+), 111 deletions(-) diff --git a/src/agents/bash-tools.exec-host-gateway.ts b/src/agents/bash-tools.exec-host-gateway.ts index 05420887b8d..c4b0122b526 100644 --- a/src/agents/bash-tools.exec-host-gateway.ts +++ b/src/agents/bash-tools.exec-host-gateway.ts @@ -1,17 +1,16 @@ import type { AgentToolResult } from "@mariozechner/pi-agent-core"; import { addDurableCommandApproval, - addAllowlistEntry, type ExecAsk, resolveExecApprovalAllowedDecisions, type ExecSecurity, buildEnforcedShellCommand, evaluateShellAllowlist, hasDurableExecApproval, - recordAllowlistUse, + persistAllowAlwaysPatterns, + recordAllowlistMatchesUse, resolveApprovalAuditCandidatePath, requiresExecApproval, - resolveAllowAlwaysPatterns, } from "../infra/exec-approvals.js"; import { describeInterpreterInlineEval, @@ -144,19 +143,14 @@ export async function processGatewayAllowlist( logInfo(`exec: obfuscation detected (gateway): ${obfuscation.reasons.join(", ")}`); params.warnings.push(`⚠️ Obfuscated command detected: ${obfuscation.reasons.join("; ")}`); } - const recordMatchedAllowlistUse = (resolvedPath?: string) => { - if (allowlistMatches.length === 0) { - return; - } - const seen = new Set(); - for (const match of allowlistMatches) { - if (seen.has(match.pattern)) { - continue; - } - seen.add(match.pattern); - recordAllowlistUse(approvals.file, params.agentId, match, params.command, resolvedPath); - } - }; + const recordMatchedAllowlistUse = (resolvedPath?: string) => + recordAllowlistMatchesUse({ + approvals: approvals.file, + agentId: params.agentId, + matches: allowlistMatches, + command: params.command, + resolvedPath, + }); const hasHeredocSegment = allowlistEval.segments.some((segment) => segment.argv.some((token) => token.startsWith("<<")), ); @@ -320,20 +314,15 @@ export async function processGatewayAllowlist( } else if (decision === "allow-always") { approvedByAsk = true; if (!requiresInlineEvalApproval) { - const patterns = resolveAllowAlwaysPatterns({ + const patterns = persistAllowAlwaysPatterns({ + approvals: approvals.file, + agentId: params.agentId, segments: allowlistEval.segments, cwd: params.workdir, env: params.env, platform: process.platform, strictInlineEval: params.strictInlineEval === true, }); - for (const pattern of patterns) { - if (pattern) { - addAllowlistEntry(approvals.file, params.agentId, pattern, { - source: "allow-always", - }); - } - } if (patterns.length === 0) { addDurableCommandApproval(approvals.file, params.agentId, params.command); } diff --git a/src/gateway/protocol/schema/exec-approvals.ts b/src/gateway/protocol/schema/exec-approvals.ts index 49953a12704..465a04d4f68 100644 --- a/src/gateway/protocol/schema/exec-approvals.ts +++ b/src/gateway/protocol/schema/exec-approvals.ts @@ -5,6 +5,7 @@ export const ExecApprovalsAllowlistEntrySchema = Type.Object( { id: Type.Optional(NonEmptyString), pattern: Type.String(), + argPattern: Type.Optional(Type.String()), lastUsedAt: Type.Optional(Type.Integer({ minimum: 0 })), lastUsedCommand: Type.Optional(Type.String()), lastResolvedPath: Type.Optional(Type.String()), diff --git a/src/infra/exec-approvals-allow-always.test.ts b/src/infra/exec-approvals-allow-always.test.ts index 12fe3c06a9c..7c399d9e5e0 100644 --- a/src/infra/exec-approvals-allow-always.test.ts +++ b/src/infra/exec-approvals-allow-always.test.ts @@ -146,31 +146,38 @@ describe("resolveAllowAlwaysPatterns", () => { expect(second.allowlistSatisfied).toBe(false); } - function expectPositionalArgvCarrierRejected(command: string) { + function expectPositionalArgvCarrierResult(params: { + command: string; + expectPersisted: boolean; + }) { const dir = makeTempDir(); const touch = makeExecutable(dir, "touch"); const env = { PATH: `${dir}${path.delimiter}${process.env.PATH ?? ""}` }; const safeBins = resolveSafeBins(undefined); const marker = path.join(dir, "marker"); - const expandedCommand = command.replaceAll("{marker}", marker); + const command = params.command.replaceAll("{marker}", marker); const { persisted } = resolvePersistedPatterns({ - command: expandedCommand, + command, dir, env, safeBins, }); - expect(persisted).toEqual([]); + if (params.expectPersisted) { + expect(persisted).toEqual([touch]); + } else { + expect(persisted).toEqual([]); + } const second = evaluateShellAllowlist({ - command: expandedCommand, + command, allowlist: [{ pattern: touch }], safeBins, cwd: dir, env, platform: process.platform, }); - expect(second.allowlistSatisfied).toBe(false); + expect(second.allowlistSatisfied).toBe(params.expectPersisted); } it("returns direct executable paths for non-shell segments", () => { @@ -387,29 +394,41 @@ describe("resolveAllowAlwaysPatterns", () => { if (process.platform === "win32") { return; } - expectPositionalArgvCarrierRejected(`sh -lc '$0 "$1"' touch {marker}`); + expectPositionalArgvCarrierResult({ + command: `sh -lc '$0 "$1"' touch {marker}`, + expectPersisted: true, + }); }); it("rejects exec positional argv carriers", () => { if (process.platform === "win32") { return; } - expectPositionalArgvCarrierRejected(`sh -lc 'exec -- "$0" "$1"' touch {marker}`); + expectPositionalArgvCarrierResult({ + command: `sh -lc 'exec -- "$0" "$1"' touch {marker}`, + expectPersisted: true, + }); }); it("rejects positional argv carriers when $0 is single-quoted", () => { if (process.platform === "win32") { return; } - expectPositionalArgvCarrierRejected(`sh -lc "'$0' "$1"" touch {marker}`); + expectPositionalArgvCarrierResult({ + command: `sh -lc "'$0' "$1"" touch {marker}`, + expectPersisted: false, + }); }); it("rejects positional argv carriers when exec is separated from $0 by a newline", () => { if (process.platform === "win32") { return; } - expectPositionalArgvCarrierRejected(`sh -lc "exec -$0 \\"$1\\"" touch {marker}`); + expectPositionalArgvCarrierResult({ + command: `sh -lc "exec +$0 \\"$1\\"" touch {marker}`, + expectPersisted: false, + }); }); it("rejects positional argv carriers when inline command contains extra shell operations", () => { @@ -915,7 +934,7 @@ $0 \\"$1\\"" touch {marker}`); expect(second.allowlistSatisfied).toBe(false); }); - it("rejects positional carrier when carried executable is an unknown dispatch carrier", () => { + it("allows positional carriers for unknown carried executables when explicitly allowlisted", () => { if (process.platform === "win32") { return; } @@ -940,6 +959,6 @@ $0 \\"$1\\"" touch {marker}`); env, platform: process.platform, }); - expect(second.allowlistSatisfied).toBe(false); + expect(second.allowlistSatisfied).toBe(true); }); }); diff --git a/src/infra/exec-approvals-allowlist.ts b/src/infra/exec-approvals-allowlist.ts index 716386137b7..2eeb8958235 100644 --- a/src/infra/exec-approvals-allowlist.ts +++ b/src/infra/exec-approvals-allowlist.ts @@ -1,4 +1,5 @@ import path from "node:path"; +import { isDispatchWrapperExecutable } from "./dispatch-wrapper-resolution.js"; import { analyzeShellCommand, isWindowsPlatform, @@ -30,9 +31,11 @@ import { extractShellWrapperInlineCommand, isShellWrapperExecutable, normalizeExecutableToken, + POWERSHELL_WRAPPERS, } from "./exec-wrapper-resolution.js"; import { resolveExecWrapperTrustPlan } from "./exec-wrapper-trust-plan.js"; import { expandHomePrefix } from "./home-dir.js"; +import { POSIX_INLINE_COMMAND_FLAGS, resolveInlineCommandMatch } from "./shell-inline-command.js"; function hasShellLineContinuation(command: string): boolean { return /\\(?:\r\n|\n|\r)/.test(command); @@ -396,8 +399,29 @@ function evaluateSegments( candidatePath && executableResolution ? { ...executableResolution, resolvedPath: candidatePath } : executableResolution; - const executableMatch = matchAllowlist(params.allowlist, candidateResolution); const inlineCommand = extractShellWrapperInlineCommand(allowlistSegment.argv); + const isPositionalCarrierInvocation = + inlineCommand !== null && isDirectShellPositionalCarrierInvocation(inlineCommand); + const executableMatch = isPositionalCarrierInvocation + ? null + : matchAllowlist(params.allowlist, candidateResolution, effectiveArgv, params.platform); + const shellPositionalArgvCandidatePath = resolveShellWrapperPositionalArgvCandidatePath({ + segment: allowlistSegment, + cwd: params.cwd, + env: params.env, + }); + const shellPositionalArgvMatch = shellPositionalArgvCandidatePath + ? matchAllowlist( + params.allowlist, + { + rawExecutable: shellPositionalArgvCandidatePath, + resolvedPath: shellPositionalArgvCandidatePath, + executableName: path.basename(shellPositionalArgvCandidatePath), + }, + undefined, + params.platform, + ) + : null; const shellScriptCandidatePath = inlineCommand === null ? resolveShellWrapperScriptCandidatePath({ @@ -405,14 +429,34 @@ function evaluateSegments( cwd: params.cwd, }) : undefined; - const shellScriptMatch = shellScriptCandidatePath - ? matchAllowlist(params.allowlist, { - rawExecutable: shellScriptCandidatePath, - resolvedPath: shellScriptCandidatePath, - executableName: path.basename(shellScriptCandidatePath), - }) + const shellScriptArgv = shellScriptCandidatePath + ? (() => { + const scriptBase = path.basename(shellScriptCandidatePath).toLowerCase(); + const cwdBase = params.cwd && params.cwd.trim() ? params.cwd.trim() : process.cwd(); + const resolveArgPath = (a: string): string => + path.isAbsolute(a) ? a : path.resolve(cwdBase, a); + let idx = effectiveArgv.findIndex((a) => resolveArgPath(a) === shellScriptCandidatePath); + if (idx === -1) { + idx = effectiveArgv.findIndex((a) => path.basename(a).toLowerCase() === scriptBase); + } + const scriptArgs = idx !== -1 ? effectiveArgv.slice(idx + 1) : []; + return [shellScriptCandidatePath, ...scriptArgs]; + })() : null; - const match = executableMatch ?? shellScriptMatch; + const shellScriptMatch = + shellScriptCandidatePath && shellScriptArgv + ? matchAllowlist( + params.allowlist, + { + rawExecutable: shellScriptCandidatePath, + resolvedPath: shellScriptCandidatePath, + executableName: path.basename(shellScriptCandidatePath), + }, + shellScriptArgv, + params.platform, + ) + : null; + const match = executableMatch ?? shellPositionalArgvMatch ?? shellScriptMatch; if (match) { matches.push(match); } @@ -545,6 +589,9 @@ function hasDisqualifyingShellWrapperScriptOption(token: string): boolean { ); } +const POWERSHELL_OPTIONS_WITH_VALUE_RE = + /^-(?:executionpolicy|ep|windowstyle|w|workingdirectory|wd|inputformat|outputformat|settingsfile|configurationfile|version|v|psconsolefile|pscf|encodedcommand|en|enc|encodedarguments|ea)$/i; + function resolveShellWrapperScriptCandidatePath(params: { segment: ExecCommandSegment; cwd?: string; @@ -558,6 +605,9 @@ function resolveShellWrapperScriptCandidatePath(params: { return undefined; } + const wrapperName = normalizeExecutableToken(argv[0] ?? ""); + const isPowerShell = POWERSHELL_WRAPPERS.has(wrapperName); + let idx = 1; while (idx < argv.length) { const token = argv[idx]?.trim() ?? ""; @@ -572,10 +622,10 @@ function resolveShellWrapperScriptCandidatePath(params: { if (token === "-c" || token === "--command") { return undefined; } - if (/^-[^-]*c[^-]*$/i.test(token)) { + if (!isPowerShell && /^-[^-]*c[^-]*$/i.test(token)) { return undefined; } - if (token === "-s" || /^-[^-]*s[^-]*$/i.test(token)) { + if (token === "-s" || (!isPowerShell && /^-[^-]*s[^-]*$/i.test(token))) { return undefined; } if (hasDisqualifyingShellWrapperScriptOption(token)) { @@ -585,6 +635,10 @@ function resolveShellWrapperScriptCandidatePath(params: { idx += 2; continue; } + if (isPowerShell && POWERSHELL_OPTIONS_WITH_VALUE_RE.test(token)) { + idx += 2; + continue; + } if (token.startsWith("-") || token.startsWith("+")) { idx += 1; continue; @@ -605,6 +659,127 @@ function resolveShellWrapperScriptCandidatePath(params: { return path.resolve(base, expanded); } +function resolveShellWrapperPositionalArgvCandidatePath(params: { + segment: ExecCommandSegment; + cwd?: string; + env?: NodeJS.ProcessEnv; +}): string | undefined { + if (!isShellWrapperSegment(params.segment)) { + return undefined; + } + + const argv = params.segment.argv; + if (!Array.isArray(argv) || argv.length < 4) { + return undefined; + } + + const wrapper = normalizeExecutableToken(argv[0] ?? ""); + if (!["ash", "bash", "dash", "fish", "ksh", "sh", "zsh"].includes(wrapper)) { + return undefined; + } + + const inlineMatch = resolveInlineCommandMatch(argv, POSIX_INLINE_COMMAND_FLAGS, { + allowCombinedC: true, + }); + if (inlineMatch.valueTokenIndex === null || !inlineMatch.command) { + return undefined; + } + if (!isDirectShellPositionalCarrierInvocation(inlineMatch.command)) { + return undefined; + } + + const carriedExecutable = argv + .slice(inlineMatch.valueTokenIndex + 1) + .map((token) => token.trim()) + .find((token) => token.length > 0); + if (!carriedExecutable) { + return undefined; + } + + const carriedName = normalizeExecutableToken(carriedExecutable); + if (isDispatchWrapperExecutable(carriedName) || isShellWrapperExecutable(carriedName)) { + return undefined; + } + + const resolution = resolveCommandResolutionFromArgv([carriedExecutable], params.cwd, params.env); + return resolveExecutionTargetCandidatePath(resolution, params.cwd); +} + +function isDirectShellPositionalCarrierInvocation(command: string): boolean { + const trimmed = command.trim(); + if (trimmed.length === 0) { + return false; + } + + const shellWhitespace = String.raw`[^\S\r\n]+`; + const positionalZero = String.raw`(?:\$(?:0|\{0\})|"\$(?:0|\{0\})")`; + const positionalArg = String.raw`(?:\$(?:[@*]|[1-9]|\{[@*1-9]\})|"\$(?:[@*]|[1-9]|\{[@*1-9]\})")`; + return new RegExp( + `^(?:exec${shellWhitespace}(?:--${shellWhitespace})?)?${positionalZero}(?:${shellWhitespace}${positionalArg})*$`, + "u", + ).test(trimmed); +} + +export type AllowAlwaysPattern = { + pattern: string; + argPattern?: string; +}; + +function escapeRegExpLiteral(input: string): string { + return input.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); +} + +function buildScriptArgPatternFromArgv( + argv: string[], + scriptPath: string, + cwd?: string, + platform?: string | null, +): string | undefined { + if (!isWindowsPlatform(platform ?? process.platform)) { + return undefined; + } + const scriptBase = path.basename(scriptPath).toLowerCase(); + const base = cwd && cwd.trim() ? cwd.trim() : process.cwd(); + const resolveArgPath = (arg: string): string => + path.isAbsolute(arg) ? arg : path.resolve(base, arg); + let scriptIdx = argv.findIndex((arg) => resolveArgPath(arg) === scriptPath); + if (scriptIdx === -1) { + scriptIdx = argv.findIndex((arg) => path.basename(arg).toLowerCase() === scriptBase); + } + const scriptArgs = scriptIdx !== -1 ? argv.slice(scriptIdx + 1) : []; + const normalized = scriptArgs.map((a) => a.replace(/\//g, "\\")); + if (normalized.length === 0) { + return "^\x00\x00$"; + } + return `^${normalized.map(escapeRegExpLiteral).join("\x00")}\x00$`; +} + +function buildArgPatternFromArgv(argv: string[], platform?: string | null): string | undefined { + if (!isWindowsPlatform(platform ?? process.platform)) { + return undefined; + } + const args = argv.slice(1); + const normalized = args.map((a) => a.replace(/\//g, "\\")); + if (normalized.length === 0) { + return "^\x00\x00$"; + } + const joined = normalized.join("\x00"); + return `^${escapeRegExpLiteral(joined)}\x00$`; +} + +function addAllowAlwaysPattern( + out: AllowAlwaysPattern[], + pattern: string, + argPattern?: string, +): void { + const exists = out.some( + (p) => p.pattern === pattern && (p.argPattern ?? undefined) === (argPattern ?? undefined), + ); + if (!exists) { + out.push({ pattern, argPattern }); + } +} + function collectAllowAlwaysPatterns(params: { segment: ExecCommandSegment; cwd?: string; @@ -612,7 +787,7 @@ function collectAllowAlwaysPatterns(params: { platform?: string | null; strictInlineEval?: boolean; depth: number; - out: Set; + out: AllowAlwaysPattern[]; }) { if (params.depth >= 3) { return; @@ -645,18 +820,45 @@ function collectAllowAlwaysPatterns(params: { } } if (!trustPlan.shellWrapperExecutable) { - params.out.add(candidatePath); + const argPattern = buildArgPatternFromArgv(segment.argv, params.platform); + addAllowAlwaysPattern(params.out, candidatePath, argPattern); return; } - const inlineCommand = - trustPlan.shellInlineCommand ?? extractShellWrapperInlineCommand(segment.argv); + const positionalArgvPath = resolveShellWrapperPositionalArgvCandidatePath({ + segment, + cwd: params.cwd, + env: params.env, + }); + if (positionalArgvPath) { + addAllowAlwaysPattern(params.out, positionalArgvPath); + return; + } + const isPowerShellFileInvocation = + POWERSHELL_WRAPPERS.has(normalizeExecutableToken(segment.argv[0] ?? "")) && + segment.argv.some((t) => { + const lower = t.trim().toLowerCase(); + return lower === "-file" || lower === "-f"; + }) && + !segment.argv.some((t) => { + const lower = t.trim().toLowerCase(); + return lower === "-command" || lower === "-c" || lower === "--command"; + }); + const inlineCommand = isPowerShellFileInvocation + ? null + : (trustPlan.shellInlineCommand ?? extractShellWrapperInlineCommand(segment.argv)); if (!inlineCommand) { const scriptPath = resolveShellWrapperScriptCandidatePath({ segment, cwd: params.cwd, }); if (scriptPath) { - params.out.add(scriptPath); + const argPattern = buildScriptArgPatternFromArgv( + params.segment.argv, + scriptPath, + params.cwd, + params.platform, + ); + addAllowAlwaysPattern(params.out, scriptPath, argPattern); } return; } @@ -687,14 +889,14 @@ function collectAllowAlwaysPatterns(params: { * When a command is wrapped in a shell (for example `zsh -lc ""`), * persist the inner executable(s) rather than the shell binary. */ -export function resolveAllowAlwaysPatterns(params: { +export function resolveAllowAlwaysPatternEntries(params: { segments: ExecCommandSegment[]; cwd?: string; env?: NodeJS.ProcessEnv; platform?: string | null; strictInlineEval?: boolean; -}): string[] { - const patterns = new Set(); +}): AllowAlwaysPattern[] { + const patterns: AllowAlwaysPattern[] = []; for (const segment of params.segments) { collectAllowAlwaysPatterns({ segment, @@ -706,7 +908,17 @@ export function resolveAllowAlwaysPatterns(params: { out: patterns, }); } - return Array.from(patterns); + return patterns; +} + +export function resolveAllowAlwaysPatterns(params: { + segments: ExecCommandSegment[]; + cwd?: string; + env?: NodeJS.ProcessEnv; + platform?: string | null; + strictInlineEval?: boolean; +}): string[] { + return resolveAllowAlwaysPatternEntries(params).map((pattern) => pattern.pattern); } /** diff --git a/src/infra/exec-approvals-analysis.test.ts b/src/infra/exec-approvals-analysis.test.ts index 40c4a439344..8d8b87b4eb6 100644 --- a/src/infra/exec-approvals-analysis.test.ts +++ b/src/infra/exec-approvals-analysis.test.ts @@ -8,9 +8,11 @@ import { buildEnforcedShellCommand, buildSafeBinsShellCommand, resolvePlannedSegmentArgv, + windowsEscapeArg, } from "./exec-approvals-analysis.js"; import { makePathEnv, makeTempDir } from "./exec-approvals-test-helpers.js"; import type { ExecAllowlistEntry } from "./exec-approvals.js"; +import { matchAllowlist } from "./exec-command-resolution.js"; function expectAnalyzedShellCommand( command: string, @@ -176,6 +178,142 @@ describe("exec approvals shell analysis", () => { expect(res.reason).toBe(reason); }); + it("accepts shell metacharacters inside double-quoted arguments on Windows", () => { + const cases = [ + // parentheses in a date/title argument + 'node add_lifelog.js "2026-03-28" "2026-03-28 (土) - LifeLog" --markdown', + // pipe, redirection, ampersand inside quotes + 'node tool.js "--filter=a|b" "--label=x>y" "--name=foo & bar"', + // caret inside quotes + 'node tool.js "--pattern=a^b"', + // exclamation inside quotes + 'node tool.js "--msg=Hello!"', + ]; + for (const command of cases) { + const res = analyzeShellCommand({ command, platform: "win32" }); + expect(res.ok).toBe(true); + expect(res.segments[0]?.argv[0]).toBe("node"); + } + }); + + it("still rejects unquoted metacharacters on Windows", () => { + const cases = [ + "ping 127.0.0.1 -n 1 & whoami", + "echo hello | clip", + "node tool.js > output.txt", + "for /f %i in (file.txt) do echo %i", + ]; + for (const command of cases) { + const res = analyzeShellCommand({ command, platform: "win32" }); + expect(res.ok).toBe(false); + } + }); + + it("still rejects % inside double quotes on Windows", () => { + const res = analyzeShellCommand({ + command: 'node tool.js "--user=%USERNAME%"', + platform: "win32", + }); + expect(res.ok).toBe(false); + }); + + it("rejects PowerShell $ expansions in Windows commands", () => { + // $ followed by identifier-start, { or ( is always unsafe — PowerShell + // expands these even inside double-quoted strings, matching windowsEscapeArg. + const cases = [ + 'node app.js "$env:USERPROFILE"', + "node app.js ${var}", + "node app.js $(whoami)", + ]; + for (const command of cases) { + const res = analyzeShellCommand({ command, platform: "win32" }); + expect(res.ok).toBe(false); + } + }); + + it("rejects $? and $$ (PowerShell automatic variables) in Windows commands", () => { + // $? (last exit status) and $$ (PID) are expanded by PowerShell inside + // double-quoted strings and must be blocked to prevent unexpected expansion. + const cases = ['node app.js "$?"', 'node app.js "$$"', "node app.js $?", "node app.js $$"]; + for (const command of cases) { + const res = analyzeShellCommand({ command, platform: "win32" }); + expect(res.ok).toBe(false); + } + }); + + it("allows bare $ not followed by identifier on Windows (e.g. UNC paths)", () => { + const res = analyzeShellCommand({ + command: 'net use "\\\\host\\C$"', + platform: "win32", + }); + expect(res.ok).toBe(true); + }); + + it("rejects metacharacters inside single-quoted arguments on Windows", () => { + // Single quotes are NOT quoting characters in cmd.exe (the Windows execution + // shell). Shell metacharacters inside single quotes remain active and unsafe. + const cases = [ + "node tool.js '--name=foo & bar'", + "node tool.js '--filter=a|b'", + "node tool.js '--msg=Hello!'", + "node tool.js '--pattern=(x)'", + ]; + for (const command of cases) { + const res = analyzeShellCommand({ command, platform: "win32" }); + expect(res.ok).toBe(false); + } + }); + + it("rejects % in single-quoted arguments on Windows", () => { + // Single quotes are literal in cmd.exe, so % is treated as unquoted and + // can be used for variable-expansion injection. + const res = analyzeShellCommand({ + command: "node tool.js '--label=%USERNAME%'", + platform: "win32", + }); + expect(res.ok).toBe(false); + }); + + it("tokenizer strips single quotes and treats content as one token on Windows", () => { + // tokenizeWindowsSegment recognises PowerShell single-quote quoting so that + // 'hello world' is correctly parsed as a single argument during enforcement. + const res = analyzeShellCommand({ + command: "node tool.js 'hello world'", + platform: "win32", + }); + expect(res.ok).toBe(true); + expect(res.segments[0]?.argv).toEqual(["node", "tool.js", "hello world"]); + }); + + it("parses '' as escaped apostrophe in Windows single-quoted args", () => { + const res = analyzeShellCommand({ + command: "node tool.js 'O''Brien'", + platform: "win32", + }); + expect(res.ok).toBe(true); + expect(res.segments[0]?.argv).toEqual(["node", "tool.js", "O'Brien"]); + }); + + it("preserves empty double-quoted args on Windows", () => { + // tokenizeWindowsSegment must not drop "" — empty quoted args are intentional + // (e.g. node tool.js "" passes an explicit empty string to the child process). + const res = analyzeShellCommand({ + command: 'node tool.js ""', + platform: "win32", + }); + expect(res.ok).toBe(true); + expect(res.segments[0]?.argv).toEqual(["node", "tool.js", ""]); + }); + + it("preserves empty single-quoted args on Windows", () => { + const res = analyzeShellCommand({ + command: "node tool.js ''", + platform: "win32", + }); + expect(res.ok).toBe(true); + expect(res.segments[0]?.argv).toEqual(["node", "tool.js", ""]); + }); + it.each(['echo "output: \\$(whoami)"', "echo 'output: $(whoami)'"])( "accepts inert substitution-like syntax for %s", (command) => { @@ -245,6 +383,87 @@ describe("exec approvals shell analysis", () => { expect(res.ok).toBe(true); expect(res.segments[0]?.argv).toEqual(["C:\\Program Files\\Tool\\tool.exe", "--version"]); }); + + it('unescapes "" inside powershell -Command double-quoted payload', () => { + // powershell -Command "node a.js ""hello world""" uses "" to encode a + // literal " inside the outer double-quoted shell argument. After stripping + // the wrapper the payload must be unescaped so the tokenizer sees the + // correct double-quote boundaries. + const res = analyzeShellCommand({ + command: 'powershell -Command "node a.js ""hello world"""', + platform: "win32", + }); + expect(res.ok).toBe(true); + expect(res.segments[0]?.argv).toEqual(["node", "a.js", "hello world"]); + }); + + it("unescapes '' inside powershell -Command single-quoted payload", () => { + // In a PowerShell single-quoted string '' encodes a literal apostrophe. + // 'node a.js ''hello world''' has outer ' delimiters and '' acts as + // the escape for the space-containing argument — after unescaping the + // payload becomes "node a.js 'hello world'" which the tokenizer parses + // as a single argv token. + const res = analyzeShellCommand({ + command: "powershell -Command 'node a.js ''hello world'''", + platform: "win32", + }); + expect(res.ok).toBe(true); + expect(res.segments[0]?.argv).toEqual(["node", "a.js", "hello world"]); + }); + + it("unwraps powershell -Command with value-taking flags", () => { + const cases = [ + 'powershell -NoProfile -ExecutionPolicy Bypass -Command "node a.js"', + 'powershell -NonInteractive -ExecutionPolicy RemoteSigned -Command "node a.js"', + 'pwsh -NoLogo -WindowStyle Hidden -Command "node a.js"', + // single-quoted payload + "powershell -NoProfile -Command 'node a.js'", + "pwsh -ExecutionPolicy Bypass -Command 'node a.js'", + ]; + for (const command of cases) { + const res = analyzeShellCommand({ command, platform: "win32" }); + expect(res.ok).toBe(true); + expect(res.segments[0]?.argv[0]).toBe("node"); + } + }); + + it("unwraps powershell -Command when a flag value contains spaces (quoted)", () => { + // psFlags previously used \S+ for flag values, which cannot match + // quoted values containing spaces such as "C:\Users\Jane Doe\proj". + // The wrapper was therefore not stripped, leaving powershell as the + // executable and breaking allow-always matching for the inner command. + const cases = [ + 'powershell -WorkingDirectory "C:\\Users\\Jane Doe\\proj" -Command "node a.js"', + "powershell -WorkingDirectory 'C:\\Users\\Jane Doe\\proj' -Command \"node a.js\"", + 'pwsh -ExecutionPolicy Bypass -WorkingDirectory "C:\\My Projects\\app" -Command "node a.js"', + ]; + for (const command of cases) { + const res = analyzeShellCommand({ command, platform: "win32" }); + expect(res.ok).toBe(true); + expect(res.segments[0]?.argv[0]).toBe("node"); + } + }); + + it("unwraps powershell -c alias and --command alias", () => { + // stripWindowsShellWrapperOnce previously only matched -Command, so + // `pwsh -c "inner"` was left as-is. The allow-always path persists the + // inner executable via extractShellWrapperInlineCommand (which treats -c + // as a command flag), but later evaluations would see `pwsh` as the + // executable, causing repeated approval prompts for the same command. + const cases = [ + ['pwsh -c "node a.js"', "node"], + ['pwsh -NoLogo -c "node a.js"', "node"], + ['powershell -c "node a.js"', "node"], + ['pwsh --command "node a.js"', "node"], + ["pwsh -c 'node a.js'", "node"], + ["pwsh -c node a.js", "node"], + ]; + for (const [command, expected] of cases) { + const res = analyzeShellCommand({ command, platform: "win32" }); + expect(res.ok).toBe(true); + expect(res.segments[0]?.argv[0]).toBe(expected); + } + }); }); describe("shell allowlist (chained commands)", () => { @@ -437,3 +656,172 @@ describe("exec approvals shell analysis", () => { }); }); }); + +describe("windowsEscapeArg", () => { + it("returns empty string quoted", () => { + expect(windowsEscapeArg("")).toEqual({ ok: true, escaped: '""' }); + }); + + it("returns safe values as-is", () => { + expect(windowsEscapeArg("foo.exe")).toEqual({ ok: true, escaped: "foo.exe" }); + expect(windowsEscapeArg("C:/Program/bin")).toEqual({ ok: true, escaped: "C:/Program/bin" }); + }); + + it("double-quotes values with spaces", () => { + expect(windowsEscapeArg("hello world")).toEqual({ ok: true, escaped: '"hello world"' }); + }); + + it("escapes embedded double quotes", () => { + expect(windowsEscapeArg('say "hi"')).toEqual({ ok: true, escaped: '"say ""hi"""' }); + }); + + it("rejects tokens with % meta character", () => { + expect(windowsEscapeArg("%PATH%")).toEqual({ ok: false }); + }); + + it("allows ! in double-quoted args (PowerShell does not treat ! as special)", () => { + expect(windowsEscapeArg("hello!")).toEqual({ ok: true, escaped: '"hello!"' }); + }); + + it("rejects $ followed by identifier (PowerShell variable expansion)", () => { + expect(windowsEscapeArg("$env:SECRET")).toEqual({ ok: false }); + expect(windowsEscapeArg("$var")).toEqual({ ok: false }); + expect(windowsEscapeArg("${var}")).toEqual({ ok: false }); + }); + + it("rejects $( subexpressions (PowerShell subexpression operator)", () => { + // PowerShell evaluates $(expression) inside double-quoted strings, so + // a token like "$(whoami)" would execute whoami even when double-quoted. + expect(windowsEscapeArg("$(whoami)")).toEqual({ ok: false }); + expect(windowsEscapeArg("$(Get-Date)")).toEqual({ ok: false }); + }); + + it("rejects $? and $$ (PowerShell automatic variables)", () => { + expect(windowsEscapeArg("$?")).toEqual({ ok: false }); + expect(windowsEscapeArg("$$")).toEqual({ ok: false }); + }); + + it("allows $ not followed by identifier (e.g. UNC admin share C$)", () => { + expect(windowsEscapeArg("\\\\host\\C$")).toEqual({ ok: true, escaped: '"\\\\host\\C$"' }); + expect(windowsEscapeArg("trailing$")).toEqual({ ok: true, escaped: '"trailing$"' }); + }); +}); + +describe("matchAllowlist with argPattern", () => { + // argPattern matching is Windows-only; skip this suite on other platforms. + if (process.platform !== "win32") { + it.skip("argPattern tests are Windows-only", () => {}); + return; + } + + const resolution = { + rawExecutable: "python3", + resolvedPath: "/usr/bin/python3", + executableName: "python3", + }; + + it("matches path-only entry regardless of argv", () => { + const entries: ExecAllowlistEntry[] = [{ pattern: "/usr/bin/python3" }]; + expect(matchAllowlist(entries, resolution, ["python3", "a.py"])).toBeTruthy(); + expect(matchAllowlist(entries, resolution, ["python3", "b.py"])).toBeTruthy(); + expect(matchAllowlist(entries, resolution, ["python3"])).toBeTruthy(); + }); + + it("matches argPattern with regex", () => { + const entries: ExecAllowlistEntry[] = [{ pattern: "/usr/bin/python3", argPattern: "^a\\.py$" }]; + expect(matchAllowlist(entries, resolution, ["python3", "a.py"])).toBeTruthy(); + expect(matchAllowlist(entries, resolution, ["python3", "b.py"])).toBeNull(); + expect(matchAllowlist(entries, resolution, ["python3", "a.py", "--verbose"])).toBeNull(); + }); + + it("prefers argPattern match over path-only match", () => { + const entries: ExecAllowlistEntry[] = [ + { pattern: "/usr/bin/python3" }, + { pattern: "/usr/bin/python3", argPattern: "^a\\.py$" }, + ]; + const match = matchAllowlist(entries, resolution, ["python3", "a.py"]); + expect(match).toBeTruthy(); + expect(match!.argPattern).toBe("^a\\.py$"); + }); + + it("falls back to path-only match when argPattern doesn't match", () => { + const entries: ExecAllowlistEntry[] = [ + { pattern: "/usr/bin/python3" }, + { pattern: "/usr/bin/python3", argPattern: "^a\\.py$" }, + ]; + const match = matchAllowlist(entries, resolution, ["python3", "b.py"]); + expect(match).toBeTruthy(); + expect(match!.argPattern).toBeUndefined(); + }); + + it("handles invalid regex gracefully", () => { + const entries: ExecAllowlistEntry[] = [{ pattern: "/usr/bin/python3", argPattern: "[invalid" }]; + expect(matchAllowlist(entries, resolution, ["python3", "a.py"])).toBeNull(); + }); + + it("rejects split-arg bypass against single-arg auto-generated argPattern", () => { + // buildArgPatternFromArgv always appends a trailing \x00 sentinel so that + // matchArgPattern can detect \x00-join style via .includes("\x00") even for + // single-arg patterns. "^hello world\x00$" is the auto-generated form for + // argv ["python3", "hello world"]. + const entries: ExecAllowlistEntry[] = [ + { pattern: "/usr/bin/python3", argPattern: "^hello world\x00$" }, + ]; + // Original approved single-arg must still match (argsString = "hello world\x00"). + expect(matchAllowlist(entries, resolution, ["python3", "hello world"])).toBeTruthy(); + // Split-arg bypass must be rejected (argsString = "hello\x00world\x00"). + expect(matchAllowlist(entries, resolution, ["python3", "hello", "world"])).toBeNull(); + }); + + it("supports regex alternation in argPattern", () => { + const entries: ExecAllowlistEntry[] = [ + { pattern: "/usr/bin/python3", argPattern: "^(a|b)\\.py$" }, + ]; + expect(matchAllowlist(entries, resolution, ["python3", "a.py"])).toBeTruthy(); + expect(matchAllowlist(entries, resolution, ["python3", "b.py"])).toBeTruthy(); + expect(matchAllowlist(entries, resolution, ["python3", "c.py"])).toBeNull(); + }); + + it("distinguishes zero-arg pattern from one-empty-string-arg pattern", () => { + // buildArgPatternFromArgv encodes [] as "^\x00\x00$" (double sentinel) and + // [""] as "^\x00$" (single sentinel) so the two cannot cross-match. + const zeroArgEntries: ExecAllowlistEntry[] = [ + { pattern: "/usr/bin/python3", argPattern: "^\x00\x00$" }, + ]; + const emptyArgEntries: ExecAllowlistEntry[] = [ + { pattern: "/usr/bin/python3", argPattern: "^\x00$" }, + ]; + // Zero-arg command must match zero-arg pattern but not empty-string-arg pattern. + expect(matchAllowlist(zeroArgEntries, resolution, ["python3"])).toBeTruthy(); + expect(matchAllowlist(emptyArgEntries, resolution, ["python3"])).toBeNull(); + // One-empty-string-arg command must match empty-string-arg pattern but not zero-arg pattern. + expect(matchAllowlist(emptyArgEntries, resolution, ["python3", ""])).toBeTruthy(); + expect(matchAllowlist(zeroArgEntries, resolution, ["python3", ""])).toBeNull(); + }); +}); + +describe("Windows rebuildShellCommandFromSource", () => { + it("builds enforced command for simple Windows command", () => { + const analysis = analyzeShellCommand({ + command: "python3 a.py", + platform: "win32", + }); + expect(analysis.ok).toBe(true); + const result = buildEnforcedShellCommand({ + command: "python3 a.py", + segments: analysis.segments, + platform: "win32", + }); + expect(result.ok).toBe(true); + expect(result.command).toBeDefined(); + }); + + it("rejects Windows commands with unsafe tokens", () => { + const result = buildEnforcedShellCommand({ + command: "echo ok & del file", + segments: [], + platform: "win32", + }); + expect(result.ok).toBe(false); + }); +}); diff --git a/src/infra/exec-approvals-analysis.ts b/src/infra/exec-approvals-analysis.ts index a64cac2209a..c92f1d0ef6e 100644 --- a/src/infra/exec-approvals-analysis.ts +++ b/src/infra/exec-approvals-analysis.ts @@ -54,6 +54,7 @@ const WINDOWS_UNSUPPORTED_TOKENS = new Set([ ")", "%", "!", + "`", "\n", "\r", ]); @@ -352,9 +353,50 @@ function splitShellPipeline(command: string): { ok: boolean; reason?: string; se return { ok: true, segments }; } +// Characters that remain unsafe even inside double-quoted strings. +// - \n / \r: newlines break command parsing regardless of quoting. +// - %: cmd.exe expands %VAR% inside double quotes, so % can still be used +// for injection even when quoted. +// - `: PowerShell escape character; forms escape sequences (`n, `0, `") even +// inside double-quoted strings, so it cannot be safely quoted. +const WINDOWS_ALWAYS_UNSAFE_TOKENS = new Set(["\n", "\r", "%", "`"]); + function findWindowsUnsupportedToken(command: string): string | null { - for (const ch of command) { + let inDouble = false; + // Single-quote tracking is intentionally omitted here. cmd.exe (used by the + // node-host exec path via buildNodeShellCommand) does not recognise single + // quotes as quoting, so metacharacters inside single-quoted strings remain + // active at runtime. Rejecting them at this layer keeps both execution paths + // (PowerShell gateway and cmd.exe node-host) safe. + // tokenizeWindowsSegment does track single quotes for accurate argv extraction + // during enforcement, which is a separate concern from the safety check here. + for (let i = 0; i < command.length; i++) { + const ch = command[i]; + if (ch === '"') { + inDouble = !inDouble; + continue; + } + // PowerShell expands $var, ${var}, and $(expr) inside double-quoted strings, + // so $ followed by an identifier-start character, {, or ( is always unsafe — + // regardless of quoting context. A bare $ not followed by those characters + // is safe (e.g. UNC admin share suffix \\host\C$). + if (ch === "$") { + const next = command[i + 1]; + // Block $var, ${var}, $(expr), $? (exit status), and $$ (PID) — all expanded + // by PowerShell inside double-quoted strings. A bare $ not followed by these + // characters is safe (e.g. the UNC admin share suffix \\host\C$). + if (next !== undefined && /[A-Za-z_{(?$]/.test(next)) { + return "$"; + } + continue; + } if (WINDOWS_UNSUPPORTED_TOKENS.has(ch)) { + // Inside double-quoted strings, most special characters are safe literal + // values (e.g. "2026-03-28 (土) - LifeLog" contains "()" which are fine). + // tokenizeWindowsSegment already handles all of these correctly inside quotes. + if (inDouble && !WINDOWS_ALWAYS_UNSAFE_TOKENS.has(ch)) { + continue; + } if (ch === "\n" || ch === "\r") { return "newline"; } @@ -368,40 +410,158 @@ function tokenizeWindowsSegment(segment: string): string[] | null { const tokens: string[] = []; let buf = ""; let inDouble = false; + let inSingle = false; + // Set to true when a quote-open is seen; ensures empty quoted args ("" or '') + // are preserved as empty-string tokens rather than being silently dropped. + let wasQuoted = false; const pushToken = () => { - if (buf.length > 0) { + if (buf.length > 0 || wasQuoted) { tokens.push(buf); buf = ""; } + wasQuoted = false; }; for (let i = 0; i < segment.length; i += 1) { const ch = segment[i]; - if (ch === '"') { + // Double-quote toggle (not inside single quotes). + if (ch === '"' && !inSingle) { + if (!inDouble) { + wasQuoted = true; + } inDouble = !inDouble; continue; } - if (!inDouble && /\s/.test(ch)) { + // Single-quote toggle (not inside double quotes) — PowerShell literal strings. + // '' inside a single-quoted string is the PowerShell escape for a literal apostrophe. + if (ch === "'" && !inDouble) { + if (inSingle && segment[i + 1] === "'") { + buf += "'"; + i += 1; + continue; + } + if (!inSingle) { + wasQuoted = true; + } + inSingle = !inSingle; + continue; + } + if (!inDouble && !inSingle && /\s/.test(ch)) { pushToken(); continue; } buf += ch; } - if (inDouble) { + if (inDouble || inSingle) { return null; } pushToken(); return tokens.length > 0 ? tokens : null; } +/** + * Recursively strip transparent Windows shell wrappers from a command string. + * + * LLMs generate commands with arbitrary nesting of shell wrappers: + * powershell -NoProfile -Command "& node 'C:\path' --count 3" + * cmd /c "node C:\path --count 3" + * & node C:\path --count 3 + * + * All of these should resolve to: node C:\path --count 3 + * + * Recognised wrappers (applied repeatedly until stable): + * - PowerShell call-operator: `& exe args` + * - cmd.exe pass-through: `cmd /c "..."` or `cmd /c ...` + * - PowerShell invocation: `powershell [-flags] -Command "..."` + */ +function stripWindowsShellWrapper(command: string): string { + const MAX_DEPTH = 5; + let result = command; + for (let i = 0; i < MAX_DEPTH; i++) { + const prev = result; + result = stripWindowsShellWrapperOnce(result.trim()); + if (result === prev) { + break; + } + } + return result; +} + +function stripWindowsShellWrapperOnce(command: string): string { + // PowerShell call-operator: & exe args → exe args + const psCallMatch = command.match(/^&\s+(.+)$/s); + if (psCallMatch) { + return psCallMatch[1]; + } + + // PowerShell invocation: powershell[.exe] [-flags] -Command|-c|--command "inner" + // Also handles pwsh[.exe] and the common -c / --command abbreviations of -Command. + // Flags before -Command may be bare (-NoProfile) or take a single value + // (-ExecutionPolicy Bypass, -WindowStyle Hidden). The lookahead (?!-) + // prevents a flag value from consuming the next flag name. + // psFlags matches zero or more PowerShell flags before the command-introducing flag. + // Each flag is either bare (-NoProfile) or takes a single value. + // Flag values may be unquoted (-ExecutionPolicy Bypass) or quoted with + // double-quotes (-WorkingDirectory "C:\Users\Jane Doe\proj") or single- + // quotes (-WorkingDirectory 'C:\Users\Jane Doe\proj'). \S+ alone cannot + // match quoted values that contain spaces, so we try double-quoted and + // single-quoted patterns first, then fall back to \S+ for unquoted values. + // + // The negative lookahead (?!c(?:ommand)?\b|-command\b) prevents psFlags from + // consuming -c or -command as an ordinary flag before the command-introducing + // flag is matched. Without it, -c "inner" would be swallowed as a value-taking + // flag and the outer pattern would never see -c to match against psCommandFlag. + const psFlags = + /(?:-(?!c(?:ommand)?\b|-command\b)\w+(?:\s+(?!-)(?:"[^"]*(?:""[^"]*)*"|'[^']*(?:''[^']*)*'|\S+))?\s+)*/i + .source; + // Matches -Command, its abbreviation -c, and the --command double-dash alias. + const psCommandFlag = `(?:-command|-c|--command)`; + const psInvokeMatch = command.match( + new RegExp(`^(?:powershell|pwsh)(?:\\.exe)?\\s+${psFlags}${psCommandFlag}\\s+"(.+)"$`, "is"), + ); + if (psInvokeMatch) { + // Within a double-quoted -Command argument, "" is the escape sequence for a + // literal ". Unescape before passing the payload to the tokenizer so that + // `powershell -Command "node a.js ""hello world"""` correctly yields the + // single argv token "hello world" rather than splitting on the space. + return psInvokeMatch[1].replace(/""/g, '"'); + } + // PowerShell -Command (or -c/--command) with single-quoted payload + const psInvokeSingleQuote = command.match( + new RegExp(`^(?:powershell|pwsh)(?:\\.exe)?\\s+${psFlags}${psCommandFlag}\\s+'(.+)'$`, "is"), + ); + if (psInvokeSingleQuote) { + // Inside a PowerShell single-quoted string '' encodes a literal apostrophe. + // Unescape before tokenizing so that 'node a.js ''hello world''' correctly + // yields the single argv token "hello world". + return psInvokeSingleQuote[1].replace(/''/g, "'"); + } + // PowerShell -Command (or -c/--command) without quotes (bare unquoted payload) + const psInvokeNoQuote = command.match( + new RegExp(`^(?:powershell|pwsh)(?:\\.exe)?\\s+${psFlags}${psCommandFlag}\\s+(.+)$`, "is"), + ); + if (psInvokeNoQuote) { + return psInvokeNoQuote[1]; + } + + // Note: cmd /c is intentionally NOT stripped here. If a command is wrapped + // with `cmd /c`, its inner payload would later be executed by PowerShell, which + // changes semantics for cmd.exe builtins (dir, copy, etc.). Callers that submit + // `cmd /c ` must have an explicit allowlist entry for `cmd` itself, or + // the command will require user approval. + + return command; +} + function analyzeWindowsShellCommand(params: { command: string; cwd?: string; env?: NodeJS.ProcessEnv; }): ExecCommandAnalysis { - const unsupported = findWindowsUnsupportedToken(params.command); + const effective = stripWindowsShellWrapper(params.command.trim()); + const unsupported = findWindowsUnsupportedToken(effective); if (unsupported) { return { ok: false, @@ -409,7 +569,7 @@ function analyzeWindowsShellCommand(params: { segments: [], }; } - const argv = tokenizeWindowsSegment(params.command); + const argv = tokenizeWindowsSegment(effective); if (!argv || argv.length === 0) { return { ok: false, reason: "unable to parse windows command", segments: [] }; } @@ -574,8 +734,56 @@ function shellEscapeSingleArg(value: string): string { return `'${value.replace(/'/g, singleQuoteEscape)}'`; } +// Characters that cannot be safely double-quoted in PowerShell enforced commands. +// % — cmd.exe immediate/delayed expansion; also blocked in analysis phase. +// $id — PowerShell variable expansion: "$env:SECRET", "${var}", "$x" ($ followed by identifier +// start or {). A bare $ not followed by [A-Za-z_{] is treated literally (e.g. "C$"). +// ` — PowerShell escape character; can form escape sequences like `n, `0 inside double quotes. +// Note: ! is intentionally omitted — PowerShell does not treat ! as special in double-quoted +// strings (unlike cmd.exe delayed expansion), so "Hello!" is safe to pass through. +const WINDOWS_UNSAFE_CMD_META = /[%`]|\$(?=[A-Za-z_{(?$])/; + +export function windowsEscapeArg(value: string): { ok: true; escaped: string } | { ok: false } { + if (value === "") { + return { ok: true, escaped: '""' }; + } + // Reject tokens containing cmd.exe / PowerShell meta characters that cannot be safely quoted. + if (WINDOWS_UNSAFE_CMD_META.test(value)) { + return { ok: false }; + } + // If the value contains only safe characters, return as-is. + if (/^[a-zA-Z0-9_./:~\\=-]+$/.test(value)) { + return { ok: true, escaped: value }; + } + // Double-quote the value, escaping embedded double-quotes. + const escaped = value.replace(/"/g, '""'); + return { ok: true, escaped: `"${escaped}"` }; +} + type ShellSegmentRenderResult = { ok: true; rendered: string } | { ok: false; reason: string }; +function rebuildWindowsShellCommandFromSource(params: { + command: string; + renderSegment: (rawSegment: string, segmentIndex: number) => ShellSegmentRenderResult; +}): { ok: boolean; command?: string; reason?: string; segmentCount?: number } { + const source = stripWindowsShellWrapper(params.command.trim()); + if (!source) { + return { ok: false, reason: "empty command" }; + } + const unsupported = findWindowsUnsupportedToken(source); + if (unsupported) { + return { ok: false, reason: `unsupported windows shell token: ${unsupported}` }; + } + const rendered = params.renderSegment(source, 0); + if (!rendered.ok) { + return { ok: false, reason: rendered.reason }; + } + // Prefix with PowerShell call operator (&) so that quoted executable paths + // (e.g. "C:\Program Files\nodejs\node.exe") are treated as commands, not + // string literals. The & operator is harmless for unquoted paths too. + return { ok: true, command: `& ${rendered.rendered}`, segmentCount: 1 }; +} + function rebuildShellCommandFromSource(params: { command: string; platform?: string | null; @@ -583,7 +791,7 @@ function rebuildShellCommandFromSource(params: { }): { ok: boolean; command?: string; reason?: string; segmentCount?: number } { const platform = params.platform ?? null; if (isWindowsPlatform(platform)) { - return { ok: false, reason: "unsupported platform" }; + return rebuildWindowsShellCommandFromSource(params); } const source = params.command.trim(); if (!source) { @@ -629,21 +837,43 @@ export function buildSafeShellCommand(params: { command: string; platform?: stri command?: string; reason?: string; } { + const isWindows = isWindowsPlatform(params.platform); const rebuilt = rebuildShellCommandFromSource({ command: params.command, platform: params.platform, renderSegment: (segmentRaw) => { - const argv = splitShellArgs(segmentRaw); - if (!argv || argv.length === 0) { + const argv = isWindows + ? (tokenizeWindowsSegment(segmentRaw) ?? []) + : (splitShellArgs(segmentRaw) ?? []); + if (argv.length === 0) { return { ok: false, reason: "unable to parse shell segment" }; } + if (isWindows) { + return renderWindowsQuotedArgv(argv); + } return { ok: true, rendered: argv.map((token) => shellEscapeSingleArg(token)).join(" ") }; }, }); return finalizeRebuiltShellCommand(rebuilt); } -function renderQuotedArgv(argv: string[]): string { +function renderWindowsQuotedArgv(argv: string[]): ShellSegmentRenderResult { + const parts: string[] = []; + for (const token of argv) { + const result = windowsEscapeArg(token); + if (!result.ok) { + return { ok: false, reason: `unsafe windows token: ${token}` }; + } + parts.push(result.escaped); + } + return { ok: true, rendered: parts.join(" ") }; +} + +function renderQuotedArgv(argv: string[], platform?: string | null): string | null { + if (isWindowsPlatform(platform)) { + const result = renderWindowsQuotedArgv(argv); + return result.ok ? result.rendered : null; + } return argv.map((token) => shellEscapeSingleArg(token)).join(" "); } @@ -681,12 +911,15 @@ export function resolvePlannedSegmentArgv(segment: ExecCommandSegment): string[] return argv; } -function renderSafeBinSegmentArgv(segment: ExecCommandSegment): string | null { +function renderSafeBinSegmentArgv( + segment: ExecCommandSegment, + platform?: string | null, +): string | null { const argv = resolvePlannedSegmentArgv(segment); if (!argv || argv.length === 0) { return null; } - return renderQuotedArgv(argv); + return renderQuotedArgv(argv, platform); } /** @@ -716,7 +949,7 @@ export function buildSafeBinsShellCommand(params: { if (!needsLiteral) { return { ok: true, rendered: raw.trim() }; } - const rendered = renderSafeBinSegmentArgv(seg); + const rendered = renderSafeBinSegmentArgv(seg, params.platform); if (!rendered) { return { ok: false, reason: "segment execution plan unavailable" }; } @@ -743,7 +976,11 @@ export function buildEnforcedShellCommand(params: { if (!argv) { return { ok: false, reason: "segment execution plan unavailable" }; } - return { ok: true, rendered: renderQuotedArgv(argv) }; + const rendered = renderQuotedArgv(argv, params.platform); + if (!rendered) { + return { ok: false, reason: "unsafe windows token in argv" }; + } + return { ok: true, rendered }; }, }); return finalizeRebuiltShellCommand(rebuilt, params.segments.length); diff --git a/src/infra/exec-approvals-store.test.ts b/src/infra/exec-approvals-store.test.ts index 067a6e8b19a..3e3703a7fdf 100644 --- a/src/infra/exec-approvals-store.test.ts +++ b/src/infra/exec-approvals-store.test.ts @@ -18,7 +18,9 @@ let addDurableCommandApproval: ExecApprovalsModule["addDurableCommandApproval"]; let ensureExecApprovals: ExecApprovalsModule["ensureExecApprovals"]; let mergeExecApprovalsSocketDefaults: ExecApprovalsModule["mergeExecApprovalsSocketDefaults"]; let normalizeExecApprovals: ExecApprovalsModule["normalizeExecApprovals"]; +let persistAllowAlwaysPatterns: ExecApprovalsModule["persistAllowAlwaysPatterns"]; let readExecApprovalsSnapshot: ExecApprovalsModule["readExecApprovalsSnapshot"]; +let recordAllowlistMatchesUse: ExecApprovalsModule["recordAllowlistMatchesUse"]; let recordAllowlistUse: ExecApprovalsModule["recordAllowlistUse"]; let requestExecApprovalViaSocket: ExecApprovalsModule["requestExecApprovalViaSocket"]; let resolveExecApprovalsPath: ExecApprovalsModule["resolveExecApprovalsPath"]; @@ -34,7 +36,9 @@ beforeAll(async () => { ensureExecApprovals, mergeExecApprovalsSocketDefaults, normalizeExecApprovals, + persistAllowAlwaysPatterns, readExecApprovalsSnapshot, + recordAllowlistMatchesUse, recordAllowlistUse, requestExecApprovalViaSocket, resolveExecApprovalsPath, @@ -229,6 +233,40 @@ describe("exec approvals store helpers", () => { ).not.toHaveProperty("commandText"); }); + it("preserves source and argPattern metadata for allow-always entries", () => { + const dir = createHomeDir(); + vi.spyOn(Date, "now").mockReturnValue(321_000); + + const approvals = ensureExecApprovals(); + addAllowlistEntry(approvals, "worker", "/usr/bin/python3", { + argPattern: "^script\\.py\x00$", + source: "allow-always", + }); + addAllowlistEntry(approvals, "worker", "/usr/bin/python3", { + argPattern: "^script\\.py\x00$", + source: "allow-always", + }); + addAllowlistEntry(approvals, "worker", "/usr/bin/python3", { + argPattern: "^other\\.py\x00$", + source: "allow-always", + }); + + expect(readApprovalsFile(dir).agents?.worker?.allowlist).toEqual([ + expect.objectContaining({ + pattern: "/usr/bin/python3", + argPattern: "^script\\.py\x00$", + source: "allow-always", + lastUsedAt: 321_000, + }), + expect.objectContaining({ + pattern: "/usr/bin/python3", + argPattern: "^other\\.py\x00$", + source: "allow-always", + lastUsedAt: 321_000, + }), + ]); + }); + it("records allowlist usage on the matching entry and backfills missing ids", () => { const dir = createHomeDir(); vi.spyOn(Date, "now").mockReturnValue(999_000); @@ -264,6 +302,95 @@ describe("exec approvals store helpers", () => { expect(readApprovalsFile(dir).agents?.main?.allowlist?.[0]?.id).toMatch(/^[0-9a-f-]{36}$/i); }); + it("dedupes allowlist usage by pattern and argPattern", () => { + const dir = createHomeDir(); + vi.spyOn(Date, "now").mockReturnValue(777_000); + + const approvals: ExecApprovalsFile = { + version: 1, + agents: { + main: { + allowlist: [ + { pattern: "/usr/bin/python3", argPattern: "^a\\.py\x00$" }, + { pattern: "/usr/bin/python3", argPattern: "^b\\.py\x00$" }, + ], + }, + }, + }; + fs.mkdirSync(path.dirname(approvalsFilePath(dir)), { recursive: true }); + fs.writeFileSync(approvalsFilePath(dir), JSON.stringify(approvals, null, 2), "utf8"); + + recordAllowlistMatchesUse({ + approvals, + agentId: undefined, + matches: [ + { pattern: "/usr/bin/python3", argPattern: "^a\\.py\x00$" }, + { pattern: "/usr/bin/python3", argPattern: "^a\\.py\x00$" }, + { pattern: "/usr/bin/python3", argPattern: "^b\\.py\x00$" }, + ], + command: "python3 a.py", + resolvedPath: "/usr/bin/python3", + }); + + expect(readApprovalsFile(dir).agents?.main?.allowlist).toEqual([ + expect.objectContaining({ + pattern: "/usr/bin/python3", + argPattern: "^a\\.py\x00$", + lastUsedAt: 777_000, + }), + expect.objectContaining({ + pattern: "/usr/bin/python3", + argPattern: "^b\\.py\x00$", + lastUsedAt: 777_000, + }), + ]); + }); + + it("persists allow-always patterns with shared helper", () => { + const dir = createHomeDir(); + vi.spyOn(Date, "now").mockReturnValue(654_321); + + const approvals = ensureExecApprovals(); + const patterns = persistAllowAlwaysPatterns({ + approvals, + agentId: "worker", + platform: "win32", + segments: [ + { + raw: "/usr/bin/custom-tool.exe a.py", + argv: ["/usr/bin/custom-tool.exe", "a.py"], + resolution: { + execution: { + rawExecutable: "/usr/bin/custom-tool.exe", + resolvedPath: "/usr/bin/custom-tool.exe", + executableName: "custom-tool", + }, + policy: { + rawExecutable: "/usr/bin/custom-tool.exe", + resolvedPath: "/usr/bin/custom-tool.exe", + executableName: "custom-tool", + }, + }, + }, + ], + }); + + expect(patterns).toEqual([ + { + pattern: "/usr/bin/custom-tool.exe", + argPattern: "^a\\.py\x00$", + }, + ]); + expect(readApprovalsFile(dir).agents?.worker?.allowlist).toEqual([ + expect.objectContaining({ + pattern: "/usr/bin/custom-tool.exe", + argPattern: "^a\\.py\x00$", + source: "allow-always", + lastUsedAt: 654_321, + }), + ]); + }); + it("returns null when approval socket credentials are missing", async () => { await expect( requestExecApprovalViaSocket({ diff --git a/src/infra/exec-approvals.ts b/src/infra/exec-approvals.ts index 4a34d9035ae..f303951c70a 100644 --- a/src/infra/exec-approvals.ts +++ b/src/infra/exec-approvals.ts @@ -2,6 +2,8 @@ import crypto from "node:crypto"; import fs from "node:fs"; import path from "node:path"; import { DEFAULT_AGENT_ID } from "../routing/session-key.js"; +import type { ExecCommandSegment } from "./exec-approvals-analysis.js"; +import { resolveAllowAlwaysPatternEntries } from "./exec-approvals-allowlist.js"; import { expandHomePrefix } from "./home-dir.js"; import { requestJsonlSocket } from "./jsonl-socket.js"; export * from "./exec-approvals-analysis.js"; @@ -123,6 +125,7 @@ export type ExecAllowlistEntry = { pattern: string; source?: "allow-always"; commandText?: string; + argPattern?: string; lastUsedAt?: number; lastUsedCommand?: string; lastResolvedPath?: string; @@ -202,8 +205,12 @@ function mergeLegacyAgent( const allowlist: ExecAllowlistEntry[] = []; const seen = new Set(); const pushEntry = (entry: ExecAllowlistEntry) => { - const key = normalizeAllowlistPattern(entry.pattern); - if (!key || seen.has(key)) { + const patternKey = normalizeAllowlistPattern(entry.pattern); + if (!patternKey) { + return; + } + const key = `${patternKey}\x00${entry.argPattern?.trim() ?? ""}`; + if (seen.has(key)) { return; } seen.add(key); @@ -757,7 +764,8 @@ export function recordAllowlistUse( const existing = agents[target] ?? {}; const allowlist = Array.isArray(existing.allowlist) ? existing.allowlist : []; const nextAllowlist = allowlist.map((item) => - item.pattern === entry.pattern + item.pattern === entry.pattern && + (item.argPattern ?? undefined) === (entry.argPattern ?? undefined) ? { ...item, id: item.id ?? crypto.randomUUID(), @@ -772,11 +780,46 @@ export function recordAllowlistUse( saveExecApprovals(approvals); } +function buildAllowlistEntryMatchKey(entry: Pick): string { + return `${entry.pattern}\x00${entry.argPattern?.trim() ?? ""}`; +} + +export function recordAllowlistMatchesUse(params: { + approvals: ExecApprovalsFile; + agentId: string | undefined; + matches: readonly ExecAllowlistEntry[]; + command: string; + resolvedPath?: string; +}): void { + if (params.matches.length === 0) { + return; + } + const seen = new Set(); + for (const match of params.matches) { + if (!match.pattern) { + continue; + } + const key = buildAllowlistEntryMatchKey(match); + if (seen.has(key)) { + continue; + } + seen.add(key); + recordAllowlistUse( + params.approvals, + params.agentId, + match, + params.command, + params.resolvedPath, + ); + } +} + export function addAllowlistEntry( approvals: ExecApprovalsFile, agentId: string | undefined, pattern: string, options?: { + argPattern?: string; source?: ExecAllowlistEntry["source"]; }, ) { @@ -788,7 +831,11 @@ export function addAllowlistEntry( if (!trimmed) { return; } - const existingEntry = allowlist.find((entry) => entry.pattern === trimmed); + const trimmedArgPattern = options?.argPattern?.trim() || undefined; + const existingEntry = allowlist.find( + (entry) => + entry.pattern === trimmed && (entry.argPattern ?? undefined) === trimmedArgPattern, + ); if (existingEntry && (!options?.source || existingEntry.source === options.source)) { return; } @@ -798,6 +845,7 @@ export function addAllowlistEntry( entry.pattern === trimmed ? { ...entry, + argPattern: trimmedArgPattern, source: options?.source ?? entry.source, lastUsedAt: now, } @@ -808,6 +856,7 @@ export function addAllowlistEntry( { id: crypto.randomUUID(), pattern: trimmed, + argPattern: trimmedArgPattern, source: options?.source, lastUsedAt: now, }, @@ -831,6 +880,34 @@ export function addDurableCommandApproval( }); } +export function persistAllowAlwaysPatterns(params: { + approvals: ExecApprovalsFile; + agentId: string | undefined; + segments: ExecCommandSegment[]; + cwd?: string; + env?: NodeJS.ProcessEnv; + platform?: string | null; + strictInlineEval?: boolean; +}): ReturnType { + const patterns = resolveAllowAlwaysPatternEntries({ + segments: params.segments, + cwd: params.cwd, + env: params.env, + platform: params.platform, + strictInlineEval: params.strictInlineEval, + }); + for (const pattern of patterns) { + if (!pattern.pattern) { + continue; + } + addAllowlistEntry(params.approvals, params.agentId, pattern.pattern, { + argPattern: pattern.argPattern, + source: "allow-always", + }); + } + return patterns; +} + export function minSecurity(a: ExecSecurity, b: ExecSecurity): ExecSecurity { const order: Record = { deny: 0, allowlist: 1, full: 2 }; return order[a] <= order[b] ? a : b; diff --git a/src/infra/exec-command-resolution.ts b/src/infra/exec-command-resolution.ts index 242e904a043..bfafd9b87f0 100644 --- a/src/infra/exec-command-resolution.ts +++ b/src/infra/exec-command-resolution.ts @@ -246,9 +246,85 @@ export function resolvePolicyAllowlistCandidatePath( return resolvePolicyTargetCandidatePath(resolution, cwd); } +// Strip trailing shell redirections (e.g. `2>&1`, `2>/dev/null`) so that +// allow-always argPatterns built without them still match commands that include +// them. LLMs commonly add or omit these between runs of the same cron job. +const TRAILING_SHELL_REDIRECTIONS_RE = /\s+(?:[12]>&[12]|[12]>\/dev\/null)\s*$/; + +function stripTrailingRedirections(value: string): string { + let prev = value; + // eslint-disable-next-line no-constant-condition + while (true) { + const next = prev.replace(TRAILING_SHELL_REDIRECTIONS_RE, ""); + if (next === prev) { + return next; + } + prev = next; + } +} + +function matchArgPattern(argPattern: string, argv: string[], platform?: string | null): boolean { + // Patterns built by buildArgPatternFromArgv use \x00 as the argument separator and + // always include a trailing \x00 sentinel so that every auto-generated pattern + // (including zero-arg "^\x00\x00$" and single-arg "^hello world\x00$") contains at + // least one \x00. This lets matchArgPattern detect the join style unambiguously + // via .includes("\x00") without misidentifying anchored hand-authored patterns. + // Legacy hand-authored patterns use a plain space and contain no \x00. + // When \x00 style is active, a trailing \x00 is appended to the joined args string + // to match the sentinel embedded in the pattern. + // + // Zero args use a double sentinel "\x00\x00" to distinguish [] from [""] — both + // join to "" but must match different patterns ("^\x00\x00$" vs "^\x00$"). + const sep = argPattern.includes("\x00") ? "\x00" : " "; + const argsSlice = argv.slice(1); + const argsString = + sep === "\x00" + ? argsSlice.length === 0 + ? "\x00\x00" // zero args: double sentinel matches "^\x00\x00$" pattern + : argsSlice.join(sep) + sep // trailing sentinel to match pattern format + : argsSlice.join(sep); + try { + const regex = new RegExp(argPattern); + if (regex.test(argsString)) { + return true; + } + // On Windows, LLMs may use forward slashes (`C:/path`) or backslashes + // (`C:\path`) interchangeably. Normalize to backslashes and retry so + // that an argPattern built from one style still matches the other. + // Use the caller-supplied target platform so Linux gateways evaluating + // Windows node commands also perform the normalization. + const effectivePlatform = String(platform ?? process.platform) + .trim() + .toLowerCase(); + if (effectivePlatform.startsWith("win")) { + const normalized = argsString.replace(/\//g, "\\"); + if (normalized !== argsString && regex.test(normalized)) { + return true; + } + } + // Retry after stripping trailing shell redirections (2>&1, etc.) so that + // patterns saved without them still match commands that include them. + // Only applies for space-joined (legacy hand-authored) patterns. For + // \x00-joined auto-generated patterns, redirections are already blocked + // upstream by findWindowsUnsupportedToken, so any surviving 2>&1 token + // is a literal data argument and must not be stripped. + if (sep === " ") { + const stripped = stripTrailingRedirections(argsString); + if (stripped !== argsString && regex.test(stripped)) { + return true; + } + } + return false; + } catch { + return false; + } +} + export function matchAllowlist( entries: ExecAllowlistEntry[], resolution: ExecutableResolution | null, + argv?: string[], + platform?: string | null, ): ExecAllowlistEntry | null { if (!entries.length) { return null; @@ -256,7 +332,7 @@ export function matchAllowlist( // A bare "*" wildcard allows any parsed executable command. // Check it before the resolvedPath guard so unresolved PATH lookups still // match (for example platform-specific executables without known extensions). - const bareWild = entries.find((e) => e.pattern?.trim() === "*"); + const bareWild = entries.find((e) => e.pattern?.trim() === "*" && !e.argPattern); if (bareWild && resolution) { return bareWild; } @@ -264,6 +340,14 @@ export function matchAllowlist( return null; } const resolvedPath = resolution.resolvedPath; + // argPattern matching is currently Windows-only. On other platforms every + // path-matched entry is treated as a match regardless of argPattern, which + // preserves the pre-existing behaviour. + // Use the caller-supplied target platform rather than process.platform so that + // a Linux gateway evaluating a Windows node command applies argPattern correctly. + const effectivePlatform = platform ?? process.platform; + const useArgPattern = String(effectivePlatform).trim().toLowerCase().startsWith("win"); + let pathOnlyMatch: ExecAllowlistEntry | null = null; for (const entry of entries) { const pattern = entry.pattern?.trim(); if (!pattern) { @@ -273,11 +357,25 @@ export function matchAllowlist( if (!hasPath) { continue; } - if (matchesExecAllowlistPattern(pattern, resolvedPath)) { + if (!matchesExecAllowlistPattern(pattern, resolvedPath)) { + continue; + } + if (!useArgPattern) { + // Non-Windows: first path match wins (legacy behaviour). + return entry; + } + if (!entry.argPattern) { + if (!pathOnlyMatch) { + pathOnlyMatch = entry; + } + continue; + } + // Entry has argPattern — check argv match. + if (argv && matchArgPattern(entry.argPattern, argv, platform)) { return entry; } } - return null; + return pathOnlyMatch; } export type ExecArgvToken = diff --git a/src/node-host/invoke-system-run.ts b/src/node-host/invoke-system-run.ts index 0c118b33b7d..c91a47eaac6 100644 --- a/src/node-host/invoke-system-run.ts +++ b/src/node-host/invoke-system-run.ts @@ -4,11 +4,10 @@ import { loadConfig } from "../config/config.js"; import type { GatewayClient } from "../gateway/client.js"; import { addDurableCommandApproval, - addAllowlistEntry, hasDurableExecApproval, - recordAllowlistUse, + persistAllowAlwaysPatterns, + recordAllowlistMatchesUse, resolveApprovalAuditCandidatePath, - resolveAllowAlwaysPatterns, resolveExecApprovals, type ExecAllowlistEntry, type ExecAsk, @@ -564,41 +563,30 @@ async function executeSystemRunPhase( } if (phase.policy.approvalDecision === "allow-always" && phase.inlineEvalHit === null) { - const patterns = resolveAllowAlwaysPatterns({ - segments: phase.segments, - cwd: phase.cwd, - env: phase.env, - platform: process.platform, - strictInlineEval: phase.strictInlineEval, - }); - for (const pattern of patterns) { - if (pattern) { - addAllowlistEntry(phase.approvals.file, phase.agentId, pattern, { - source: "allow-always", - }); - } - } + const patterns = + phase.policy.analysisOk + ? persistAllowAlwaysPatterns({ + approvals: phase.approvals.file, + agentId: phase.agentId, + segments: phase.segments, + cwd: phase.cwd, + env: phase.env, + platform: process.platform, + strictInlineEval: phase.strictInlineEval, + }) + : []; if (patterns.length === 0) { addDurableCommandApproval(phase.approvals.file, phase.agentId, phase.commandText); } } - if (phase.allowlistMatches.length > 0) { - const seen = new Set(); - for (const match of phase.allowlistMatches) { - if (!match?.pattern || seen.has(match.pattern)) { - continue; - } - seen.add(match.pattern); - recordAllowlistUse( - phase.approvals.file, - phase.agentId, - match, - phase.commandText, - resolveApprovalAuditCandidatePath(phase.segments[0]?.resolution ?? null, phase.cwd), - ); - } - } + recordAllowlistMatchesUse({ + approvals: phase.approvals.file, + agentId: phase.agentId, + matches: phase.allowlistMatches, + command: phase.commandText, + resolvedPath: resolveApprovalAuditCandidatePath(phase.segments[0]?.resolution ?? null, phase.cwd), + }); if (phase.needsScreenRecording) { await sendSystemRunDenied(opts, phase.execution, {