From dbbed7fd44e816077a85dc94f9aeb6c8bae79a37 Mon Sep 17 00:00:00 2001 From: Coco Sheng Date: Thu, 16 Apr 2026 13:28:27 -0400 Subject: [PATCH] security: improve rm detection and clarify UI labels --- .../src/sandbox/utils/commandSafety.test.ts | 91 +++++++++++++++++++ .../core/src/sandbox/utils/commandSafety.ts | 5 +- .../src/sandbox/windows/commandSafety.test.ts | 6 ++ .../core/src/sandbox/windows/commandSafety.ts | 6 +- 4 files changed, 102 insertions(+), 6 deletions(-) create mode 100644 packages/core/src/sandbox/utils/commandSafety.test.ts diff --git a/packages/core/src/sandbox/utils/commandSafety.test.ts b/packages/core/src/sandbox/utils/commandSafety.test.ts new file mode 100644 index 0000000000..42c41c1a23 --- /dev/null +++ b/packages/core/src/sandbox/utils/commandSafety.test.ts @@ -0,0 +1,91 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, expect, it, vi } from 'vitest'; + +// Mock shell-utils to avoid relying on tree-sitter WASM +vi.mock('../../utils/shell-utils.js', () => ({ + initializeShellParsers: vi.fn().mockResolvedValue(undefined), + splitCommands: (cmd: string) => [cmd], + stripShellWrapper: (cmd: string) => cmd, + extractStringFromParseEntry: (entry: unknown) => { + if (typeof entry === 'string') return entry; + if (entry && typeof entry === 'object' && 'content' in entry) { + return (entry as { content: string }).content; + } + return ''; + }, + normalizeCommand: (cmd: string) => { + // Simple mock normalization: /bin/rm -> rm + if (cmd.startsWith('/')) { + const parts = cmd.split('/'); + return parts[parts.length - 1]; + } + return cmd; + }, +})); + +import { isKnownSafeCommand, isDangerousCommand } from './commandSafety.js'; + +describe('POSIX commandSafety', () => { + describe('isKnownSafeCommand', () => { + it('should identify known safe commands', () => { + expect(isKnownSafeCommand(['ls', '-la'])).toBe(true); + expect(isKnownSafeCommand(['cat', 'file.txt'])).toBe(true); + expect(isKnownSafeCommand(['pwd'])).toBe(true); + expect(isKnownSafeCommand(['echo', 'hello'])).toBe(true); + }); + + it('should identify safe git commands', () => { + expect(isKnownSafeCommand(['git', 'status'])).toBe(true); + expect(isKnownSafeCommand(['git', 'log'])).toBe(true); + expect(isKnownSafeCommand(['git', 'diff'])).toBe(true); + }); + + it('should reject unsafe git commands', () => { + expect(isKnownSafeCommand(['git', 'commit'])).toBe(false); + expect(isKnownSafeCommand(['git', 'push'])).toBe(false); + expect(isKnownSafeCommand(['git', 'checkout'])).toBe(false); + }); + + it('should reject commands with redirection', () => { + // isKnownSafeCommand handles bash -c "..." which can have redirections + // but the simple check for atomic commands doesn't see redirection because it's already parsed + }); + }); + + describe('isDangerousCommand', () => { + it('should identify destructive rm commands', () => { + expect(isDangerousCommand(['rm'])).toBe(true); + expect(isDangerousCommand(['rm', 'file.txt'])).toBe(true); + expect(isDangerousCommand(['rm', '-rf', '/'])).toBe(true); + expect(isDangerousCommand(['rm', '-f', 'file'])).toBe(true); + expect(isDangerousCommand(['rm', '-r', 'dir'])).toBe(true); + expect(isDangerousCommand(['/bin/rm', 'file'])).toBe(true); + }); + + it('should flag rm help/version as dangerous (strict)', () => { + expect(isDangerousCommand(['rm', '--help'])).toBe(true); + expect(isDangerousCommand(['rm', '--version'])).toBe(true); + }); + + it('should identify sudo as dangerous if command is dangerous', () => { + expect(isDangerousCommand(['sudo', 'rm', 'file'])).toBe(true); + }); + + it('should identify find -exec as dangerous', () => { + expect(isDangerousCommand(['find', '.', '-exec', '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 180d0748d2..ac5f600214 100644 --- a/packages/core/src/sandbox/utils/commandSafety.ts +++ b/packages/core/src/sandbox/utils/commandSafety.ts @@ -9,6 +9,7 @@ import { initializeShellParsers, splitCommands, stripShellWrapper, + normalizeCommand, } from '../../utils/shell-utils.js'; /** @@ -428,10 +429,10 @@ export function isDangerousCommand(args: string[]): boolean { return false; } - const cmd = args[0]; + const cmd = normalizeCommand(args[0]); if (cmd === 'rm') { - return args[1] === '-f' || args[1] === '-rf' || args[1] === '-fr'; + return true; } if (cmd === 'sudo') { diff --git a/packages/core/src/sandbox/windows/commandSafety.test.ts b/packages/core/src/sandbox/windows/commandSafety.test.ts index 82077b2690..b6c8ac4bc9 100644 --- a/packages/core/src/sandbox/windows/commandSafety.test.ts +++ b/packages/core/src/sandbox/windows/commandSafety.test.ts @@ -34,6 +34,12 @@ describe('Windows commandSafety', () => { expect(isDangerousCommand(['cmd', '/c', 'dir'])).toBe(true); }); + it('should identify path-qualified dangerous commands', () => { + expect( + isDangerousCommand(['C:\\Windows\\System32\\del.exe', 'file.txt']), + ).toBe(true); + }); + it('should strip .exe extension for dangerous commands', () => { expect(isDangerousCommand(['del.exe', 'file.txt'])).toBe(true); expect(isDangerousCommand(['POWERSHELL.EXE', '-Command', 'echo'])).toBe( diff --git a/packages/core/src/sandbox/windows/commandSafety.ts b/packages/core/src/sandbox/windows/commandSafety.ts index b2cc842df9..006baa9247 100644 --- a/packages/core/src/sandbox/windows/commandSafety.ts +++ b/packages/core/src/sandbox/windows/commandSafety.ts @@ -9,6 +9,7 @@ import { initializeShellParsers, splitCommands, stripShellWrapper, + normalizeCommand, } from '../../utils/shell-utils.js'; /** @@ -119,10 +120,7 @@ export function isKnownSafeCommand(args: string[]): boolean { */ export function isDangerousCommand(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]); const dangerous = new Set([ 'del',