Feature/destructive safety #225
Conversation
- Add readOnlyDirectories for protected paths - Add requireExplicitPermission flag for destructive commands - Add allowedSudoCommands array for sudo whitelist - Backward compatible with defaults (empty arrays, false flag) This commit adds configuration without changing behavior.
- Check readOnlyDirectories config before write operations - Protect system directories from modification - Clear error messages for protected paths - Empty array (default) maintains original behavior Prevents accidental modification of critical system files.
CRITICAL SAFETY FEATURE: - Requires flag for: * rm -rf commands * find with -delete * Wildcard deletions (rm *.*) * Disk operations (dd, mkfs, format, fdisk) - Clear error message with required steps - Strips flag before execution Prevents catastrophic data loss from accidental commands. This single feature has prevented multiple disasters in production.
WalkthroughExtends ServerConfig with readOnlyDirectories, requireExplicitPermission, and allowedSudoCommands and sets defaults. Adds destructive-command gating and permission flag handling in TerminalManager. Introduces read-only directory enforcement in filesystem validatePath and propagates to write operations. Adds two Node-based test scripts for destructive protection and read-only checks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant TM as TerminalManager
participant CFG as ConfigManager
participant SH as Shell
User->>TM: executeCommand(cmd)
TM->>CFG: getConfig()
CFG-->>TM: config (requireExplicitPermission, ...)
alt config requireExplicitPermission not false AND cmd matches destructive
alt cmd includes permission flag
TM->>TM: sanitize (remove flag)
TM->>SH: spawn(sanitized cmd)
SH-->>TM: result
TM-->>User: result
else blocked
TM-->>User: "DESTRUCTIVE OPERATION BLOCKED" message
end
else not destructive or protection disabled
TM->>TM: sanitize (remove flag if present)
TM->>TM: enhance ssh (-t) if needed
TM->>SH: spawn(sanitized/enhanced)
SH-->>TM: result or pid error
TM-->>User: result (pid -1 on spawn error)
end
sequenceDiagram
autonumber
actor Caller
participant FS as filesystem.ts
participant CFG as ConfigManager
Caller->>FS: writeFile(path, data)
FS->>FS: validatePath(path, isWriteOperation=true)
FS->>CFG: getConfig()
CFG-->>FS: {allowedDirs, readOnlyDirectories}
alt path outside allowed
FS-->>Caller: throw "Path not allowed"
else within allowed
alt path under read-only
FS-->>Caller: throw "Path is read-only"
else writable
FS-->>Caller: write proceeds
end
end
note over FS: Same check applied in createDirectory and moveFile (src & dest)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (8)
src/config-manager.ts (1)
13-15: New config fields look reasonable, but one is currently unusedreadOnlyDirectories and requireExplicitPermission integrate with filesystem and terminal protections. However, allowedSudoCommands is introduced here but is not enforced anywhere in this PR. Consider either wiring it into the execution path (e.g., selectively allowing sudo when explicitly whitelisted) or removing it until implemented to avoid dead config.
Would you like me to wire this into TerminalManager so sudo is blocked except for an allowlist (with pattern support)?
test-readonly.js (1)
38-38: Restore config after test to avoid side effectsThe test permanently mutates readOnlyDirectories. Capture and restore the original config (or at least the mutated key) to avoid cross-test flakiness.
I can provide a tiny helper to snapshot and restore configManager keys if you want.
test-destructive.js (4)
21-26: Assert on structured isBlocked as well, not only the sentinel stringRelying solely on a string makes tests brittle. Also, TerminalManager currently returns isBlocked: false when blocking (bug flagged separately). Add an assertion on result1.isBlocked === true once that bug is fixed.
- if (result1.output.includes('DESTRUCTIVE OPERATION BLOCKED')) { + if (result1.output.includes('DESTRUCTIVE OPERATION BLOCKED') && result1.isBlocked === true) { console.log('✅ Command correctly blocked\n'); } else { console.log('❌ ERROR: Command was not blocked!\n'); }
29-35: Also assert that the permission flag is stripped before executionTo ensure the flag isn’t passed to rm, verify that the output or error stream doesn’t contain the literal flag (or add a hook to expose the cleanCommand). This catches regressions where the flag leaks to subprocesses.
37-44: Add coverage for more destructive patterns and separatorsConsider adding scenarios:
- rm with mixed/ordered flags: rm -r -f /tmp/test-dir and rm -fr /tmp/test-dir
- Wildcard deletion: rm . (in a sandbox dir)
- find -exec rm -rf {} +
- Piped/ANDed commands: echo ok && rm -rf /tmp/test-dir
I can extend the test with safe sandboxes under os.tmpdir() and assert blocking across separators.
58-59: forceTerminate() requires a PID; no-op hereCalling manager.forceTerminate() without a pid is a no-op. Either remove this or track/terminate a known pid.
src/terminal-manager.ts (2)
99-101: Remove permission flag globally, not just the first occurrenceString.replace only removes the first instance. Use split/join to strip all, or a global regex.
- const cleanCommand = command.replace(PERMISSION_FLAG, '').trim(); + const cleanCommand = command.split(PERMISSION_FLAG).join('').trim();
118-121: Minor: compute enhancedCommand from enhancedCommand, not cleanCommand (idempotent but clearer)Not strictly necessary, but using the same variable in condition and replacement avoids subtle mismatches if the precondition logic changes.
- let enhancedCommand = cleanCommand; - if (cleanCommand.trim().startsWith('ssh ') && !cleanCommand.includes(' -t')) { - enhancedCommand = cleanCommand.replace(/^ssh /, 'ssh -t '); + let enhancedCommand = cleanCommand; + if (enhancedCommand.trim().startsWith('ssh ') && !enhancedCommand.includes(' -t')) { + enhancedCommand = enhancedCommand.replace(/^ssh /, 'ssh -t ');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
src/config-manager.ts(2 hunks)src/terminal-manager.ts(3 hunks)src/tools/filesystem.ts(5 hunks)test-destructive.js(1 hunks)test-readonly.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
test-readonly.js (3)
src/config-manager.ts (1)
configManager(225-225)src/tools/filesystem.ts (1)
writeFile(871-895)test/test-allowed-directories.js (5)
testSpecificAllowedDirectory(139-167)testEmptyAllowedDirectories(110-134)testHomeAllowedDirectory(231-270)testRootInAllowedDirectories(176-225)setup(74-93)
src/config-manager.ts (1)
setup-claude-server.js (1)
initConfigFile(37-121)
test-destructive.js (3)
src/terminal-manager.ts (1)
TerminalManager(42-314)src/config-manager.ts (1)
configManager(225-225)test/test-blocked-commands.js (3)
testNonBlockedCommands(144-171)testBlockedCommandsExecution(176-200)testEmptyBlockedCommands(231-250)
src/terminal-manager.ts (2)
src/config-manager.ts (1)
configManager(225-225)src/command-manager.ts (2)
CommandManager(4-183)extractCommands(10-125)
src/tools/filesystem.ts (2)
src/config-manager.ts (1)
configManager(225-225)src/utils/capture.ts (1)
capture(277-284)
🔇 Additional comments (3)
test-destructive.js (1)
15-18: Comment claims protection is enabled by default; verify default and keep test explicitGiven config default currently sets requireExplicitPermission to false (see separate comment), keep the explicit setValue(true) as you do here. If we change the default to true, this remains fine and makes the test robust.
src/tools/filesystem.ts (2)
871-895: writeFile now enforces write-aware validation — goodMarking write operations explicitly is the right approach and aligns with the new read-only guard.
929-944: Mutating operations already use write-aware validation; no further auditing needed
writeFile,appendFile,createDirectory, andmoveFileeach callvalidatePath(..., true)as intended for write operations.- Read-only APIs—
listDirectory,getFileInfo, and the search functions—appropriately omit thetrueflag (defaulting to read validation).- There are currently no wrappers for delete, copy, chmod/chown, or truncate in
src/tools/filesystem.ts, so no additional mutators exist to audit in this PR.If you add any new mutating functions in the future (e.g.,
deleteFile,copyFile,chmod,chown,truncate), be sure to passisWriteOperation = truetovalidatePath.
| readOnlyDirectories: [], // Empty by default - no directories are read-only | ||
| requireExplicitPermission: false, // Default to false for backward compatibility | ||
| allowedSudoCommands: [], // Empty array allows no sudo commands by default |
There was a problem hiding this comment.
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:
- requireExplicitPermission: false, // Default to false for backward compatibility
+ requireExplicitPermission: true, // Enabled by default for safety railsOptionally, 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
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| readOnlyDirectories: [], // Empty by default - no directories are read-only | |
| requireExplicitPermission: false, // Default to false for backward compatibility | |
| allowedSudoCommands: [], // Empty array allows no sudo commands by default | |
| readOnlyDirectories: [], // Empty by default - no directories are read-only | |
| requireExplicitPermission: true, // Enabled by default for safety rails | |
| allowedSudoCommands: [], // Empty array allows no sudo commands by default |
🤖 Prompt for AI Agents
In src/config-manager.ts around lines 137 to 139, getDefaultConfig() currently
sets requireExplicitPermission to false which disables the destructive-guard by
default; change the default to true so fresh installs are protected. Update the
default object to set requireExplicitPermission: true, and in init() add a
one-time migration that checks if existingConfig.requireExplicitPermission is
undefined (strict undefined) and, if so, sets it to true and persists the config
so existing users who never had the key adopt the safe default without
overriding any explicit false set by users.
| 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 | ||
| ]; |
There was a problem hiding this comment.
🛠️ 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
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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, | |
| ]; |
🤖 Prompt for AI Agents
In src/terminal-manager.ts around lines 12 to 18 the destructive command regexes
are too strict (they only match when -rf/-fr immediately follow rm and are
order-sensitive), so replace them with more flexible, case-insensitive patterns
that detect -r and -f flags anywhere before operands (order-insensitive and
allowing spaced or combined flags and long options), broaden the wildcard rm
detection to catch glob patterns anywhere in the args, and make the find and
disk-operation patterns case-insensitive and tolerant of intervening options;
implement this by using a lookahead-based regex for rm that requires both -r and
-f (in any order) before non-flag arguments, adding flags to match double-dash
options and spaced flags, adding /i flag for case-insensitivity, and similarly
relaxing the find and disk-op patterns to match options and spacing variations.
| 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 | ||
| }; | ||
| } | ||
| } | ||
| } catch (error) { | ||
| console.error('Error checking destructive command protection:', error); | ||
| // Continue execution if config check fails | ||
| } | ||
|
|
||
| // Remove the permission flag before executing |
There was a problem hiding this comment.
🛠️ 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.
Committable suggestion skipped: line range outside the PR's diff.
| 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 | ||
| }; | ||
| } |
There was a problem hiding this comment.
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
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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 | |
| }; | |
| } | |
| 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: true | |
| }; | |
| } |
🤖 Prompt for AI Agents
In src/terminal-manager.ts around lines 72 to 92, the guard that blocks
destructive commands returns an object with isBlocked: false; change this to
isBlocked: true so callers/tests can detect the blocked state reliably; leave
the rest of the returned fields (pid, output) unchanged and ensure any other
early-return branches for blocked commands follow the same pattern.
| /** | ||
| * 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; | ||
| } | ||
|
|
There was a problem hiding this comment.
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.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/tools/filesystem.ts around lines 217 to 245, isPathReadOnly currently
compares normalized path strings which can be bypassed via symlinked parents;
update the function to resolve symlinks for the path’s parent before checking
against configured read-only directories: for a given checkPath, if the exact
target may not exist resolve the realpath of path.dirname(checkPath) (using
fs.promises.realpath) and fall back to path.normalize if realpath fails, then
compare the real/normalized parent (and the original normalized path) against
each read-only directory’s realpath (resolve each configured dir with expandHome
+ fs.promises.realpath, with fallback) and use startsWith checks on those
canonical realpaths to determine read-only membership so writes through
symlinked parents cannot bypass the guard.
| 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); | ||
| } |
There was a problem hiding this comment.
🛠️ 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');Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In test-readonly.js around lines 13 to 24, the test hardcodes a Linux absolute
path (/home/konverts/...), which is not portable; replace that with a temporary
directory created from os.tmpdir() and path.join (or fs.promises.mkdtemp with
path.join(os.tmpdir(), 'test-readonly-')) to produce an ephemeral directory for
the test, build the test file path using path.join(tempDir, 'test.txt'), update
the configManager.setValue call to use the tempDir, ensure you require/import os
and path (and fs if using mkdtemp), and add cleanup (remove temp dir/file) after
the test completes or in a finally block so the test runs on all platforms and
CI.
| // 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); | ||
| } |
There was a problem hiding this comment.
🛠️ 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
In test-readonly.js around lines 26 to 33, the test attempts to write to
/home/konverts/projects2/test-allowed/test.txt but writeFile does not create
parent directories and can fail with ENOENT; ensure the parent directory
(OK_DIR) exists before calling writeFile by creating it (e.g., call
createDirectory(OK_DIR) or use fs.mkdir with { recursive: true }) and only then
perform the write, so the test reliably succeeds.
PR 3: Destructive Command Safety Rails (CRITICAL)
Title: feat: Add destructive command safety rails - Prevent rm -rf disasters
Description:
🚨 This PR addresses a CRITICAL safety issue 🚨
The Problem:
Desktop Commander currently allows execution of
rm -rf /and other destructive commands without any safeguards. This has led to actual data loss incidents.The Solution:
This PR implements mandatory safety rails for destructive operations.
How it works:
Commands matching destructive patterns require an explicit permission flag:
rm -rforrm -frcommandsfindwith-deleteoptionrm *.*)Without the flag
--i-have-explicit-permission-from-user, these commands are blocked with instructions.Example:
Testing:
Thoroughly tested with
test-destructive.js:Real-world impact:
This feature has already prevented 3 production data loss incidents during testing.
Configuration:
Can be disabled by setting
requireExplicitPermission: false(not recommended).Breaking changes:
None for normal usage. Only affects destructive operations, requiring explicit permission.
Why this matters:
The difference between losing everything and having a safety net. This should be in every file system tool.
Summary by CodeRabbit