diff --git a/packages/core/src/sandbox/utils/commandSafety.test.ts b/packages/core/src/sandbox/utils/commandSafety.test.ts index 42c41c1a23..8796ba17f9 100644 --- a/packages/core/src/sandbox/utils/commandSafety.test.ts +++ b/packages/core/src/sandbox/utils/commandSafety.test.ts @@ -19,12 +19,10 @@ vi.mock('../../utils/shell-utils.js', () => ({ return ''; }, normalizeCommand: (cmd: string) => { - // Simple mock normalization: /bin/rm -> rm - if (cmd.startsWith('/')) { - const parts = cmd.split('/'); - return parts[parts.length - 1]; - } - return cmd; + if (!cmd) return ''; + const parts = cmd.split(/[\\/]/).filter(Boolean); + const base = parts.length > 0 ? parts[parts.length - 1] : ''; + return base.toLowerCase().replace(/\.exe$/, ''); }, })); @@ -74,6 +72,41 @@ describe('POSIX commandSafety', () => { it('should identify sudo as dangerous if command is dangerous', () => { expect(isDangerousCommand(['sudo', 'rm', 'file'])).toBe(true); + expect(isDangerousCommand(['sudo', '-u', 'root', 'rm', 'file'])).toBe( + true, + ); + expect(isDangerousCommand(['sudo', '--user', 'root', 'rm', 'file'])).toBe( + true, + ); + expect(isDangerousCommand(['sudo', '-t', 'type', 'rm', 'file'])).toBe( + true, + ); + expect(isDangerousCommand(['sudo', '-o', 'opt=val', 'rm', 'file'])).toBe( + true, + ); + expect( + isDangerousCommand(['sudo', '--option', 'opt=val', 'rm', 'file']), + ).toBe(true); + expect(isDangerousCommand(['sudo', '--askpass', 'rm', 'file'])).toBe( + true, + ); + expect(isDangerousCommand(['sudo', '-D', '/tmp', 'rm', '-rf', '/'])).toBe( + true, + ); + expect( + isDangerousCommand(['sudo', '--chroot', '/tmp', 'rm', '-rf', '/']), + ).toBe(true); + expect(isDangerousCommand(['sudo', '-r', 'role', 'rm', 'file'])).toBe( + true, + ); + expect(isDangerousCommand(['sudo', '-t', 'type', 'rm', 'file'])).toBe( + true, + ); + expect(isDangerousCommand(['sudo', '--askpass', 'rm', 'file'])).toBe( + true, + ); + expect(isDangerousCommand(['sudo', './rm=dangerous'])).toBe(false); + expect(isDangerousCommand(['sudo', './rm'])).toBe(true); }); it('should identify find -exec as dangerous', () => { @@ -82,6 +115,26 @@ describe('POSIX commandSafety', () => { ); }); + it('should identify dangerous commands wrapped in env', () => { + expect(isDangerousCommand(['env', 'rm', '-rf', '/'])).toBe(true); + expect(isDangerousCommand(['env', '-i', 'rm', '-rf', '/'])).toBe(true); + expect(isDangerousCommand(['env', '-u', 'USER', 'rm', '-rf', '/'])).toBe( + true, + ); + expect(isDangerousCommand(['env', 'VAR=val', 'rm', '-rf', '/'])).toBe( + true, + ); + expect(isDangerousCommand(['env', '--', 'rm', '-rf', '/'])).toBe(true); + }); + + it('should identify dangerous commands wrapped in xargs', () => { + expect(isDangerousCommand(['xargs', 'rm', '-rf', '/'])).toBe(true); + expect(isDangerousCommand(['xargs', '-I', '{}', 'rm', '{}'])).toBe(true); + expect(isDangerousCommand(['xargs', '-n', '1', 'rm'])).toBe(true); + expect(isDangerousCommand(['xargs', '-0', 'rm'])).toBe(true); + expect(isDangerousCommand(['xargs', '--', 'rm'])).toBe(true); + }); + it('should identify dangerous rg flags', () => { expect(isDangerousCommand(['rg', '--hostname-bin', 'something'])).toBe( true, diff --git a/packages/core/src/sandbox/utils/commandSafety.ts b/packages/core/src/sandbox/utils/commandSafety.ts index ac5f600214..7cf8b7769f 100644 --- a/packages/core/src/sandbox/utils/commandSafety.ts +++ b/packages/core/src/sandbox/utils/commandSafety.ts @@ -256,7 +256,7 @@ function isSafeToCallWithExec(args: string[]): boolean { * @param subcommands - A list of subcommands to look for. * @returns An object containing the index of the subcommand and its name. */ -function findGitSubcommand( +export function findGitSubcommand( args: string[], subcommands: string[], ): { idx: number; subcommand: string | null } { @@ -318,7 +318,7 @@ function findGitSubcommand( * @param args - The git command arguments. * @returns true if config overrides are present. */ -function gitHasConfigOverrideGlobalOption(args: string[]): boolean { +export function gitHasConfigOverrideGlobalOption(args: string[]): boolean { return args.some( (arg) => arg === '-c' || @@ -336,7 +336,7 @@ function gitHasConfigOverrideGlobalOption(args: string[]): boolean { * @param args - Arguments passed to the git subcommand. * @returns true if the arguments only represent read-only operations. */ -function gitSubcommandArgsAreReadOnly(args: string[]): boolean { +export function gitSubcommandArgsAreReadOnly(args: string[]): boolean { const unsafeFlags = new Set([ '--output', '--ext-diff', @@ -360,7 +360,7 @@ function gitSubcommandArgsAreReadOnly(args: string[]): boolean { * @param args - Arguments passed to `git branch`. * @returns true if it's purely a listing/read-only branch command. */ -function gitBranchIsReadOnly(args: string[]): boolean { +export function gitBranchIsReadOnly(args: string[]): boolean { if (args.length === 0) return true; let sawReadOnlyFlag = false; @@ -436,7 +436,98 @@ export function isDangerousCommand(args: string[]): boolean { } if (cmd === 'sudo') { - return isDangerousCommand(args.slice(1)); + let nextCmdIdx = 1; + while (nextCmdIdx < args.length) { + const arg = args[nextCmdIdx]; + if (arg === '--') { + nextCmdIdx++; + break; + } + if (!arg.startsWith('-')) { + // It could be variable assignment like VAR=value before the command + if (/^[a-zA-Z_][a-zA-Z0-9_]*=/.test(arg)) { + nextCmdIdx++; + continue; + } + break; + } + + // Check for options that consume the next argument + if ( + /^-[acCDghopRrTtUu]$/.test(arg) || + [ + '--auth-type', + '--login-class', + '--close-from', + '--chdir', + '--group', + '--host', + '--prompt', + '--chroot', + '--role', + '--type', + '--command-timeout', + '--timeout', + '--user', + '--other-user', + '--option', + ].includes(arg) + ) { + nextCmdIdx += 2; + } else { + nextCmdIdx += 1; + } + } + return isDangerousCommand(args.slice(nextCmdIdx)); + } + + if (['env', 'xargs'].includes(cmd)) { + let nextCmdIdx = 1; + while (nextCmdIdx < args.length) { + const arg = args[nextCmdIdx]; + if (arg === '--') { + nextCmdIdx++; + break; + } + if (!arg.startsWith('-')) { + // It could be variable assignment like VAR=value before the command (for env) + if (cmd === 'env' && /^[a-zA-Z_][a-zA-Z0-9_]*=/.test(arg)) { + nextCmdIdx++; + continue; + } + break; + } + + // Check for options that consume the next argument + if (cmd === 'env') { + if ( + /^-[uS]$/.test(arg) || + ['--unset', '--split-string'].includes(arg) + ) { + nextCmdIdx += 2; + } else { + nextCmdIdx += 1; + } + } else if (cmd === 'xargs') { + if ( + /^-[aEeIiLlnPs]$/.test(arg) || + [ + '--arg-file', + '--max-args', + '--max-procs', + '--max-chars', + '--process-slot-var', + ].includes(arg) + ) { + nextCmdIdx += 2; + } else { + nextCmdIdx += 1; + } + } else { + nextCmdIdx += 1; + } + } + return isDangerousCommand(args.slice(nextCmdIdx)); } if (cmd === 'find') { diff --git a/packages/core/src/sandbox/windows/commandSafety.test.ts b/packages/core/src/sandbox/windows/commandSafety.test.ts index b6c8ac4bc9..a9450c1911 100644 --- a/packages/core/src/sandbox/windows/commandSafety.test.ts +++ b/packages/core/src/sandbox/windows/commandSafety.test.ts @@ -25,6 +25,15 @@ describe('Windows commandSafety', () => { expect(isKnownSafeCommand(['unknown'])).toBe(false); expect(isKnownSafeCommand(['npm', 'install'])).toBe(false); }); + + it('should reject unsafe git commands', () => { + expect(isKnownSafeCommand(['git', 'commit'])).toBe(false); + expect(isKnownSafeCommand(['git', 'push'])).toBe(false); + expect(isKnownSafeCommand(['git', 'checkout'])).toBe(false); + expect(isKnownSafeCommand(['git', 'log', '--output=file.txt'])).toBe( + false, + ); + }); }); describe('isDangerousCommand', () => { @@ -34,6 +43,15 @@ describe('Windows commandSafety', () => { expect(isDangerousCommand(['cmd', '/c', 'dir'])).toBe(true); }); + it('should reject unsafe git commands as dangerous', () => { + expect(isDangerousCommand(['git', 'log', '--output=file.txt'])).toBe( + true, + ); + expect(isDangerousCommand(['git', '-c', 'alias.foo=!sh', 'status'])).toBe( + true, + ); + }); + it('should identify path-qualified dangerous commands', () => { expect( isDangerousCommand(['C:\\Windows\\System32\\del.exe', 'file.txt']), @@ -48,6 +66,41 @@ describe('Windows commandSafety', () => { expect(isDangerousCommand(['cmd.exe', '/c', 'dir'])).toBe(true); }); + it('should strip multiple trailing dots and extensions for dangerous commands', () => { + expect(isDangerousCommand(['powershell.', '-Command', 'echo'])).toBe( + true, + ); + expect(isDangerousCommand(['powershell.exe.', '-Command', 'echo'])).toBe( + true, + ); + expect(isDangerousCommand(['cmd.bat..', '/c', 'dir'])).toBe(true); + }); + + it('should flag rm as dangerous', () => { + expect(isDangerousCommand(['rm', 'file'])).toBe(true); + expect(isDangerousCommand(['rm.exe', 'file'])).toBe(true); + }); + + it('should identify dangerous commands wrapped in env', () => { + expect(isDangerousCommand(['env', 'rm', '-rf', '/'])).toBe(true); + expect(isDangerousCommand(['env', '-i', 'rm', '-rf', '/'])).toBe(true); + expect(isDangerousCommand(['env', '-u', 'USER', 'rm', '-rf', '/'])).toBe( + true, + ); + expect(isDangerousCommand(['env', 'VAR=val', 'rm', '-rf', '/'])).toBe( + true, + ); + expect(isDangerousCommand(['env', '--', 'rm', '-rf', '/'])).toBe(true); + }); + + it('should identify dangerous commands wrapped in xargs', () => { + expect(isDangerousCommand(['xargs', 'rm', '-rf', '/'])).toBe(true); + expect(isDangerousCommand(['xargs', '-I', '{}', 'rm', '{}'])).toBe(true); + expect(isDangerousCommand(['xargs', '-n', '1', 'rm'])).toBe(true); + expect(isDangerousCommand(['xargs', '-0', 'rm'])).toBe(true); + expect(isDangerousCommand(['xargs', '--', 'rm'])).toBe(true); + }); + it('should not flag safe commands as dangerous', () => { expect(isDangerousCommand(['dir'])).toBe(false); expect(isDangerousCommand(['echo', 'hello'])).toBe(false); diff --git a/packages/core/src/sandbox/windows/commandSafety.ts b/packages/core/src/sandbox/windows/commandSafety.ts index 006baa9247..003162bc20 100644 --- a/packages/core/src/sandbox/windows/commandSafety.ts +++ b/packages/core/src/sandbox/windows/commandSafety.ts @@ -10,7 +10,14 @@ import { splitCommands, stripShellWrapper, normalizeCommand, + hasRedirection, } from '../../utils/shell-utils.js'; +import { + findGitSubcommand, + gitHasConfigOverrideGlobalOption, + gitSubcommandArgsAreReadOnly, + gitBranchIsReadOnly, +} from '../utils/commandSafety.js'; /** * Determines if a command is strictly approved for execution on Windows. @@ -34,6 +41,10 @@ export async function isStrictlyApproved( const fullCmd = [command, ...args].join(' '); const stripped = stripShellWrapper(fullCmd); + if (hasRedirection(stripped)) { + return false; + } + const pipelineCommands = splitCommands(stripped); // Fallback for simple commands or parsing failures @@ -49,10 +60,7 @@ export async function isStrictlyApproved( const parsedArgs = shellParse(trimmed).map(extractStringFromParseEntry); if (parsedArgs.length === 0) return true; - let root = parsedArgs[0].toLowerCase(); - if (root.endsWith('.exe')) { - root = root.slice(0, -4); - } + const root = normalizeCommand(parsedArgs[0]); // The segment is approved if the root tool is in the allowlist OR if the whole segment is safe. return ( tools.some((t) => t.toLowerCase() === root) || @@ -66,10 +74,7 @@ export async function isStrictlyApproved( */ export function isKnownSafeCommand(args: string[]): boolean { if (!args || args.length === 0) return false; - let cmd = args[0].toLowerCase(); - if (cmd.endsWith('.exe')) { - cmd = cmd.slice(0, -4); - } + const cmd = normalizeCommand(args[0]); // Native Windows/PowerShell safe commands const safeCommands = new Set([ @@ -106,10 +111,35 @@ export function isKnownSafeCommand(args: string[]): boolean { // We allow git on Windows if it's read-only, using the same logic as POSIX if (cmd === 'git') { - // For simplicity in this branch, we'll allow standard git read operations - // In a full implementation, we'd port the sub-command validation too. - const sub = args[1]?.toLowerCase(); - return ['status', 'log', 'diff', 'show', 'branch'].includes(sub); + if (gitHasConfigOverrideGlobalOption(args)) { + return false; + } + + const { idx, subcommand } = findGitSubcommand(args, [ + 'status', + 'log', + 'diff', + 'show', + 'branch', + ]); + if (!subcommand) { + return false; + } + + const subcommandArgs = args.slice(idx + 1); + + if (['status', 'log', 'diff', 'show'].includes(subcommand)) { + return gitSubcommandArgsAreReadOnly(subcommandArgs); + } + + if (subcommand === 'branch') { + return ( + gitSubcommandArgsAreReadOnly(subcommandArgs) && + gitBranchIsReadOnly(subcommandArgs) + ); + } + + return false; } return false; @@ -123,6 +153,7 @@ export function isDangerousCommand(args: string[]): boolean { const cmd = normalizeCommand(args[0]); const dangerous = new Set([ + 'rm', 'del', 'erase', 'rd', @@ -144,5 +175,89 @@ export function isDangerousCommand(args: string[]): boolean { 'new-item', ]); - return dangerous.has(cmd); + if (dangerous.has(cmd)) { + return true; + } + + if (cmd === 'git') { + if (gitHasConfigOverrideGlobalOption(args)) { + return true; + } + + const { idx, subcommand } = findGitSubcommand(args, [ + 'status', + 'log', + 'diff', + 'show', + 'branch', + ]); + if (!subcommand) { + // It's a git command we don't recognize as explicitly safe. + return false; + } + + const subcommandArgs = args.slice(idx + 1); + + if (['status', 'log', 'diff', 'show'].includes(subcommand)) { + return !gitSubcommandArgsAreReadOnly(subcommandArgs); + } + + if (subcommand === 'branch') { + return !( + gitSubcommandArgsAreReadOnly(subcommandArgs) && + gitBranchIsReadOnly(subcommandArgs) + ); + } + + return false; + } + + if (['env', 'xargs'].includes(cmd)) { + let nextCmdIdx = 1; + while (nextCmdIdx < args.length) { + const arg = args[nextCmdIdx]; + if (arg === '--') { + nextCmdIdx++; + break; + } + if (!arg.startsWith('-')) { + if (cmd === 'env' && /^[a-zA-Z_][a-zA-Z0-9_]*=/.test(arg)) { + nextCmdIdx++; + continue; + } + break; + } + + if (cmd === 'env') { + if ( + /^-[uS]$/.test(arg) || + ['--unset', '--split-string'].includes(arg) + ) { + nextCmdIdx += 2; + } else { + nextCmdIdx += 1; + } + } else if (cmd === 'xargs') { + if ( + /^-[aEeIiLlnPs]$/.test(arg) || + [ + '--arg-file', + '--max-args', + '--max-procs', + '--max-chars', + '--process-slot-var', + ].includes(arg) + ) { + nextCmdIdx += 2; + } else { + nextCmdIdx += 1; + } + } else { + nextCmdIdx += 1; + } + } + return isDangerousCommand(args.slice(nextCmdIdx)); + } + + return false; } diff --git a/packages/core/src/utils/shell-utils.ts b/packages/core/src/utils/shell-utils.ts index 46cffa1d35..f1c2137b52 100644 --- a/packages/core/src/utils/shell-utils.ts +++ b/packages/core/src/utils/shell-utils.ts @@ -321,7 +321,26 @@ export function normalizeCommand(commandName: string): string { // Split by both separators and get the last non-empty part const parts = commandName.split(/[\\/]/).filter(Boolean); const base = parts.length > 0 ? parts[parts.length - 1] : ''; - return base.toLowerCase().replace(/\.exe$/, ''); + let result = base.toLowerCase(); + const extensions = ['.exe', '.cmd', '.bat', '.com', '.ps1']; + + while (true) { + const prev = result; + + while (result.endsWith('.')) { + result = result.slice(0, -1); + } + + for (const ext of extensions) { + if (result.endsWith(ext)) { + result = result.slice(0, -ext.length); + break; + } + } + + if (result === prev) break; + } + return result; } function extractNameFromNode(node: Node): string | null {