-
-
Notifications
You must be signed in to change notification settings - Fork 556
Feature/destructive safety #225
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -5,6 +5,32 @@ import { configManager } from './config-manager.js'; | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import {capture} from "./utils/capture.js"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { analyzeProcessState } from './utils/process-detection.js'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // The permission flag for destructive commands | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const PERMISSION_FLAG = '--i-have-explicit-permission-from-user'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Patterns for destructive commands | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const DESTRUCTIVE_PATTERNS = [ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /\brm\s+(-rf?|-fr?)\s+/, // rm -rf or rm -fr | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /\brm\s+.*\*/, // rm with wildcards | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /\bfind\s+.*-delete/, // find with -delete | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /\bfind\s+.*-exec\s+rm/, // find with -exec rm | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /\b(dd|mkfs|format|fdisk)\b/, // Disk operations | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ]; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+12
to
+18
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Destructive regexes are under-matching and can be bypassed Current rm pattern only matches when -rf/-fr immediately follow rm. Commands like rm --flag -rf path or rm -r -f path won’t match; mixed/combined flags later in the command can bypass the guard. Strengthen patterns to detect -r and -f anywhere before operands (order-insensitive), and cover spaced flags. Apply this diff to broaden coverage (case-insensitive and more flexible): -const DESTRUCTIVE_PATTERNS = [
- /\brm\s+(-rf?|-fr?)\s+/, // rm -rf or rm -fr
- /\brm\s+.*\*/, // rm with wildcards
- /\bfind\s+.*-delete/, // find with -delete
- /\bfind\s+.*-exec\s+rm/, // find with -exec rm
- /\b(dd|mkfs|format|fdisk)\b/, // Disk operations
-];
+const DESTRUCTIVE_PATTERNS = [
+ // rm where both -r and -f are present anywhere before operands (order-insensitive)
+ /\brm\b[^\n]*\s-(?:[^\s]*r[^\s]*f[^\s]*|[^\s]*f[^\s]*r[^\s]*)/i,
+ // rm with separated flags (e.g., -r ... -f or -f ... -r)
+ /\brm\b[^\n]*\s-r\b[^\n]*\s-f\b/i,
+ /\brm\b[^\n]*\s-f\b[^\n]*\s-r\b/i,
+ // rm with wildcards
+ /\brm\b[^\n]*\*/,
+ // find with -delete or -exec rm
+ /\bfind\b[^\n]*-delete\b/i,
+ /\bfind\b[^\n]*-exec\b[^\n]*\brm\b/i,
+ // Disk operations
+ /\b(dd|mkfs|format|fdisk)\b/i,
+];📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Check if a command is destructive and needs explicit permission | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| function isDestructiveCommand(command: string): boolean { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return DESTRUCTIVE_PATTERNS.some(pattern => pattern.test(command)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Check if command has permission flag | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| function hasPermissionFlag(command: string): boolean { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return command.includes(PERMISSION_FLAG); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| interface CompletedSession { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pid: number; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| output: string; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -44,6 +70,35 @@ export class TerminalManager { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| async executeCommand(command: string, timeoutMs: number = DEFAULT_COMMAND_TIMEOUT, shell?: string): Promise<CommandExecutionResult> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Check for destructive commands if protection is enabled | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const config = await configManager.getConfig(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (config.requireExplicitPermission !== false) { // Default to true if not set | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (isDestructiveCommand(command) && !hasPermissionFlag(command)) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pid: -1, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| output: `🚨 DESTRUCTIVE OPERATION BLOCKED! 🚨 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| This command requires explicit permission. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| To execute, you MUST: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 1. Ask the user what specifically they want deleted | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 2. Show them what will be affected | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 3. Get explicit confirmation | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 4. Add flag: ${PERMISSION_FLAG} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Example: rm ${PERMISSION_FLAG} -rf /path/to/delete`, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| isBlocked: false | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
72
to
+92
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Blocked-case sets isBlocked to false; flip to true When the guard blocks execution, isBlocked should be true so callers/tests can rely on structured state. - return {
+ return {
pid: -1,
output: `🚨 DESTRUCTIVE OPERATION BLOCKED! 🚨
@@
-Example: rm ${PERMISSION_FLAG} -rf /path/to/delete`,
- isBlocked: false
+Example: rm ${PERMISSION_FLAG} -rf /path/to/delete`,
+ isBlocked: true
};📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } catch (error) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| console.error('Error checking destructive command protection:', error); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Continue execution if config check fails | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Remove the permission flag before executing | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
72
to
+99
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Also respect blockedCommands via CommandManager before spawning executeCommand currently doesn’t consult blockedCommands, which can bypass existing policy (see test/test-blocked-commands.js). Validate with CommandManager first. async executeCommand(command: string, timeoutMs: number = DEFAULT_COMMAND_TIMEOUT, shell?: string): Promise<CommandExecutionResult> {
+ // Enforce static blockedCommands first
+ try {
+ const { default: CommandManagerMod } = await import('./command-manager.js');
+ const commandManager = new CommandManagerMod.default?.constructor === Function
+ ? new CommandManagerMod.default()
+ : new CommandManagerMod.CommandManager?.constructor === Function
+ ? new CommandManagerMod.CommandManager()
+ : null;
+ if (commandManager && !(await commandManager.validateCommand(command))) {
+ return {
+ pid: -1,
+ output: 'Command blocked by policy (blockedCommands).',
+ isBlocked: true
+ };
+ }
+ } catch (e) {
+ // Non-fatal: if validation fails, continue to the destructive guard
+ }If the module shape is stable (named export), prefer a direct import: -import { spawn } from 'child_process';
+import { spawn } from 'child_process';
+import { CommandManager } from './command-manager.js';And then instantiate/use it directly.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const cleanCommand = command.replace(PERMISSION_FLAG, '').trim(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Get the shell from config if not specified | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let shellToUse: string | boolean | undefined = shell; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!shellToUse) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -60,9 +115,9 @@ export class TerminalManager { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Note: No special stdio options needed here, Node.js handles pipes by default | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Enhance SSH commands automatically | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let enhancedCommand = command; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (command.trim().startsWith('ssh ') && !command.includes(' -t')) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| enhancedCommand = command.replace(/^ssh /, 'ssh -t '); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let enhancedCommand = cleanCommand; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (cleanCommand.trim().startsWith('ssh ') && !cleanCommand.includes(' -t')) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| enhancedCommand = cleanCommand.replace(/^ssh /, 'ssh -t '); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| console.log(`Enhanced SSH command: ${enhancedCommand}`); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -214,16 +214,47 @@ async function isPathAllowed(pathToCheck: string): Promise<boolean> { | |
| return isAllowed; | ||
| } | ||
|
|
||
| /** | ||
| * Check if a path is within a read-only directory | ||
| * @param checkPath The path to check | ||
| * @returns Promise<boolean> True if the path is read-only | ||
| */ | ||
| async function isPathReadOnly(checkPath: string): Promise<boolean> { | ||
| const config = await configManager.getConfig(); | ||
| const readOnlyDirs = config.readOnlyDirectories || []; | ||
|
|
||
| if (readOnlyDirs.length === 0) { | ||
| return false; // No read-only directories configured | ||
| } | ||
|
|
||
| const normalizedCheckPath = path.normalize(checkPath).toLowerCase(); | ||
|
|
||
| for (const dir of readOnlyDirs) { | ||
| const expandedDir = expandHome(dir); | ||
| const normalizedDir = path.normalize(expandedDir).toLowerCase(); | ||
|
|
||
| // Check if the path is within the read-only directory | ||
| if (normalizedCheckPath === normalizedDir || | ||
| normalizedCheckPath.startsWith(normalizedDir + path.sep)) { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
|
Comment on lines
+217
to
+245
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Read-only check is symlink-bypassable for write operations isPathReadOnly compares string-normalized paths. For writes to non-existent targets via a symlinked parent (e.g., /tmp/link → /protected; writing /tmp/link/file.txt), fs.writeFile will traverse the symlink and write inside a protected directory. Since validatePath checks read-only before realpath resolution, this bypasses the guard. Use the parent directory’s realpath for write checks. Apply this refactor: async function isPathReadOnly(checkPath: string): Promise<boolean> {
- const config = await configManager.getConfig();
- const readOnlyDirs = config.readOnlyDirectories || [];
+ const config = await configManager.getConfig();
+ const readOnlyDirs = config.readOnlyDirectories || [];
@@
- const normalizedCheckPath = path.normalize(checkPath).toLowerCase();
+ const normalizedCheckPath = path.normalize(checkPath).toLowerCase();
@@
for (const dir of readOnlyDirs) {
const expandedDir = expandHome(dir);
const normalizedDir = path.normalize(expandedDir).toLowerCase();
@@
}
return false;
}And in validatePath(), derive a canonical target for write checks by resolving the parent directory: - // Check if path is read-only for write operations
- if (isWriteOperation && await isPathReadOnly(absolute)) {
+ // Check if path is read-only for write operations (resolve parent symlinks)
+ if (isWriteOperation) {
+ const parent = path.dirname(absolute);
+ let canonicalParent = parent;
+ try {
+ canonicalParent = await fs.realpath(parent);
+ } catch { /* parent may not exist; keep as-is */ }
+ const canonicalTarget = path.join(canonicalParent, path.basename(absolute));
+ if (await isPathReadOnly(canonicalTarget)) {
capture('server_path_validation_error', {
error: 'Path is read-only',
operation: 'write'
});
-
- throw new Error(`Path is read-only: ${requestedPath}. This directory is protected from modifications.`);
+ throw new Error(`Path is read-only: ${requestedPath}. This directory is protected from modifications.`);
+ }
}This closes the symlink hole for create/append/rename into protected trees.
🤖 Prompt for AI Agents |
||
| /** | ||
| * Validates a path to ensure it can be accessed or created. | ||
| * For existing paths, returns the real path (resolving symlinks). | ||
| * For non-existent paths, validates parent directories to ensure they exist. | ||
| * For write operations, also checks if the path is read-only. | ||
| * | ||
| * @param requestedPath The path to validate | ||
| * @param isWriteOperation Whether this is a write operation (default: false) | ||
| * @returns Promise<string> The validated path | ||
| * @throws Error if the path or its parent directories don't exist or if the path is not allowed | ||
| * @throws Error if the path or its parent directories don't exist or if the path is not allowed or read-only | ||
| */ | ||
| export async function validatePath(requestedPath: string): Promise<string> { | ||
| export async function validatePath(requestedPath: string, isWriteOperation: boolean = false): Promise<string> { | ||
| const validationOperation = async (): Promise<string> => { | ||
| // Expand home directory if present | ||
| const expandedPath = expandHome(requestedPath); | ||
|
|
@@ -243,6 +274,16 @@ export async function validatePath(requestedPath: string): Promise<string> { | |
| throw new Error(`Path not allowed: ${requestedPath}. Must be within one of these directories: ${(await getAllowedDirs()).join(', ')}`); | ||
| } | ||
|
|
||
| // Check if path is read-only for write operations | ||
| if (isWriteOperation && await isPathReadOnly(absolute)) { | ||
| capture('server_path_validation_error', { | ||
| error: 'Path is read-only', | ||
| operation: 'write' | ||
| }); | ||
|
|
||
| throw new Error(`Path is read-only: ${requestedPath}. This directory is protected from modifications.`); | ||
| } | ||
|
|
||
| // Check if path exists | ||
| try { | ||
| const stats = await fs.stat(absolute); | ||
|
|
@@ -828,7 +869,7 @@ function splitLinesPreservingEndings(content: string): string[] { | |
| } | ||
|
|
||
| export async function writeFile(filePath: string, content: string, mode: 'rewrite' | 'append' = 'rewrite'): Promise<void> { | ||
| const validPath = await validatePath(filePath); | ||
| const validPath = await validatePath(filePath, true); // Mark as write operation | ||
|
|
||
| // Get file extension for telemetry | ||
| const fileExtension = getFileExtension(validPath); | ||
|
|
@@ -886,7 +927,7 @@ export async function readMultipleFiles(paths: string[]): Promise<MultiFileResul | |
| } | ||
|
|
||
| export async function createDirectory(dirPath: string): Promise<void> { | ||
| const validPath = await validatePath(dirPath); | ||
| const validPath = await validatePath(dirPath, true); // Creating directory is a write operation | ||
| await fs.mkdir(validPath, { recursive: true }); | ||
| } | ||
|
|
||
|
|
@@ -897,8 +938,8 @@ export async function listDirectory(dirPath: string): Promise<string[]> { | |
| } | ||
|
|
||
| export async function moveFile(sourcePath: string, destinationPath: string): Promise<void> { | ||
| const validSourcePath = await validatePath(sourcePath); | ||
| const validDestPath = await validatePath(destinationPath); | ||
| const validSourcePath = await validatePath(sourcePath, true); // Source needs write permission (to delete) | ||
| const validDestPath = await validatePath(destinationPath, true); // Destination needs write permission | ||
| await fs.rename(validSourcePath, validDestPath); | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| #!/usr/bin/env node | ||
|
|
||
| // Test script for destructive command safety rails | ||
|
|
||
| import { TerminalManager } from './dist/terminal-manager.js'; | ||
| import { configManager } from './dist/config-manager.js'; | ||
|
|
||
| async function testDestructiveCommandProtection() { | ||
| console.log('Testing destructive command safety rails...\n'); | ||
|
|
||
| const manager = new TerminalManager(); | ||
|
|
||
| // Enable protection (it's enabled by default) | ||
| await configManager.init(); | ||
| await configManager.setValue('requireExplicitPermission', true); | ||
|
|
||
| console.log('✅ Protection enabled\n'); | ||
|
|
||
| // Test 1: Try rm -rf without permission flag | ||
| console.log('Test 1: rm -rf without permission flag'); | ||
| const result1 = await manager.executeCommand('rm -rf /tmp/test-dir', 1000); | ||
| if (result1.output.includes('DESTRUCTIVE OPERATION BLOCKED')) { | ||
| console.log('✅ Command correctly blocked\n'); | ||
| } else { | ||
| console.log('❌ ERROR: Command was not blocked!\n'); | ||
| } | ||
|
|
||
| // Test 2: Try rm -rf WITH permission flag | ||
| console.log('Test 2: rm -rf WITH permission flag'); | ||
| const result2 = await manager.executeCommand('rm --i-have-explicit-permission-from-user -rf /tmp/test-dir', 1000); | ||
| if (!result2.output.includes('DESTRUCTIVE OPERATION BLOCKED')) { | ||
| console.log('✅ Command allowed with permission flag\n'); | ||
| } else { | ||
| console.log('❌ ERROR: Command was blocked even with flag!\n'); | ||
| } | ||
|
|
||
| // Test 3: Try find with -delete | ||
| console.log('Test 3: find with -delete'); | ||
| const result3 = await manager.executeCommand('find /tmp -name "*.log" -delete', 1000); | ||
| if (result3.output.includes('DESTRUCTIVE OPERATION BLOCKED')) { | ||
| console.log('✅ find -delete correctly blocked\n'); | ||
| } else { | ||
| console.log('❌ ERROR: find -delete was not blocked!\n'); | ||
| } | ||
|
|
||
| // Test 4: Normal commands should work | ||
| console.log('Test 4: Normal command (ls)'); | ||
| const result4 = await manager.executeCommand('ls /tmp', 1000); | ||
| if (!result4.output.includes('DESTRUCTIVE OPERATION BLOCKED')) { | ||
| console.log('✅ Normal command allowed\n'); | ||
| } else { | ||
| console.log('❌ ERROR: Normal command was blocked!\n'); | ||
| } | ||
|
|
||
| console.log('✅ Destructive command protection test complete!'); | ||
|
|
||
| // Clean up | ||
| manager.forceTerminate(); | ||
| } | ||
|
|
||
| testDestructiveCommandProtection().catch(console.error); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| #!/usr/bin/env node | ||
|
|
||
| // Test script for read-only directory protection | ||
|
|
||
| import { configManager } from './dist/config-manager.js'; | ||
| import { writeFile } from './dist/tools/filesystem.js'; | ||
|
|
||
| async function testReadOnlyProtection() { | ||
| console.log('Testing read-only directory protection...\n'); | ||
|
|
||
| // Set up a read-only directory within allowed paths | ||
| await configManager.init(); | ||
| await configManager.setValue('readOnlyDirectories', ['/home/konverts/projects2/test-readonly']); | ||
|
|
||
| console.log('✅ Configuration set: /home/konverts/projects2/test-readonly is now read-only\n'); | ||
|
|
||
| // Try to write to a read-only directory | ||
| try { | ||
| console.log('Attempting to write to /home/konverts/projects2/test-readonly/test.txt...'); | ||
| await writeFile('/home/konverts/projects2/test-readonly/test.txt', 'This should fail'); | ||
| console.log('❌ ERROR: Write succeeded when it should have failed!'); | ||
| } catch (error) { | ||
| console.log('✅ Write correctly blocked:', error.message); | ||
| } | ||
|
Comment on lines
+13
to
+24
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Make the test portable; avoid hardcoded absolute paths The test uses absolute Linux-specific paths (/home/konverts/...). This will fail on other machines/CI and on Windows. Use os.tmpdir() + path.join() to create ephemeral directories under a temp root. -import { configManager } from './dist/config-manager.js';
-import { writeFile } from './dist/tools/filesystem.js';
+import { configManager } from './dist/config-manager.js';
+import { writeFile, createDirectory } from './dist/tools/filesystem.js';
+import os from 'os';
+import path from 'path';
@@
- await configManager.setValue('readOnlyDirectories', ['/home/konverts/projects2/test-readonly']);
+ const TMP = os.tmpdir();
+ const RO_DIR = path.join(TMP, 'dcmd-readonly');
+ const OK_DIR = path.join(TMP, 'dcmd-allowed');
+ await createDirectory(RO_DIR);
+ await createDirectory(OK_DIR);
+ await configManager.setValue('readOnlyDirectories', [RO_DIR]);
@@
- console.log('Attempting to write to /home/konverts/projects2/test-readonly/test.txt...');
- await writeFile('/home/konverts/projects2/test-readonly/test.txt', 'This should fail');
+ const roFile = path.join(RO_DIR, 'test.txt');
+ console.log(`Attempting to write to ${roFile}...`);
+ await writeFile(roFile, 'This should fail');
@@
- console.log('\nAttempting to write to /home/konverts/projects2/test-allowed/test.txt...');
- await writeFile('/home/konverts/projects2/test-allowed/test.txt', 'This should work');
+ const okFile = path.join(OK_DIR, 'test.txt');
+ console.log(`\nAttempting to write to ${okFile}...`);
+ await writeFile(okFile, 'This should work');
🤖 Prompt for AI Agents |
||
|
|
||
| // Try to write to a non-protected directory | ||
| try { | ||
| console.log('\nAttempting to write to /home/konverts/projects2/test-allowed/test.txt...'); | ||
| await writeFile('/home/konverts/projects2/test-allowed/test.txt', 'This should work'); | ||
| console.log('✅ Write succeeded to non-protected directory'); | ||
| } catch (error) { | ||
| console.log('❌ ERROR: Write failed:', error.message); | ||
| } | ||
|
Comment on lines
+26
to
+33
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Create parent directories for the “allowed” write writeFile doesn’t create parent directories. The current test can fail with ENOENT. Ensure the OK_DIR exists before writing (see diff above adding createDirectory(OK_DIR)). 🤖 Prompt for AI Agents |
||
|
|
||
| console.log('\n✅ Read-only protection test complete!'); | ||
| } | ||
|
|
||
| testReadOnlyProtection().catch(console.error); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Default disables the safety rails for new installs; contradicts PR intent
getDefaultConfig() sets requireExplicitPermission to false. Combined with TerminalManager’s check (config.requireExplicitPermission !== false), fresh installs will have destructive-guarding OFF (because the default explicitly writes false). The PR description says the rails are “mandatory” and tests assume protection “enabled by default.” Set the default to true to make new configs safe by default.
Apply this diff:
Optionally, add a one-time migration in init(): if requireExplicitPermission is undefined in an existing config, set it to true to adopt the safe default without overriding explicit user choice.
📝 Committable suggestion
🤖 Prompt for AI Agents