Feature/read only protection #224
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.
WalkthroughIntroduces three optional config fields in ServerConfig and initializes them in getDefaultConfig. Updates filesystem path validation to accept an isWriteOperation flag and enforce read-only directory restrictions for write operations. Write-related methods now pass the flag. Read operations and existing allowed-directory checks remain unchanged. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant FS as Filesystem
participant Validator as validatePath()
Caller->>FS: writeFile/createDirectory/moveFile(path, ...)
activate FS
FS->>Validator: validatePath(path, isWriteOperation=true)
activate Validator
Validator->>Validator: Resolve path, check allowed dirs
alt Path inside read-only directory
Validator-->>FS: Throw read-only violation
FS-->>Caller: Error (blocked write)
else Path allowed for write
Validator-->>FS: OK
FS-->>Caller: Perform write and return
end
deactivate Validator
deactivate FS
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: 2
🧹 Nitpick comments (3)
src/config-manager.ts (2)
13-15: Scope creep: two new config fields are unrelated to read-only protection and are unused
requireExplicitPermissionandallowedSudoCommandsexpand the public API but are not enforced anywhere in this PR. This increases surface area and expectations without hard guarantees. Suggest deferring these to a focused follow-up PR, or at minimum, add a brief doc comment noting they’re placeholders and not yet enforced.If you intend to keep them, please confirm the planned enforcement points and whether any UI/docs need updating in this PR.
137-139: Defaults are sensible; consider normalizing/validating readOnlyDirectories at config boundaryInitializing
readOnlyDirectoriesto[]preserves behavior. As a small hardening step, normalize entries (expand~, resolve absolute, trim trailing separators, dedupe) when setting/updating this field so all downstream checks operate on canonical values.Example normalization at set-time (outside this range):
function normalizeDirs(dirs: string[]): string[] { const seen = new Set<string>(); return dirs .map(d => d.trim()) .filter(Boolean) .map(d => d.replace(/[/\\]+$/, '')) // strip trailing sep .map(d => d.startsWith('~') ? path.join(os.homedir(), d.slice(1)) : d) .map(d => path.isAbsolute(d) ? path.normalize(d) : path.normalize(path.resolve(process.cwd(), d))) .filter(d => (seen.has(d) ? false : (seen.add(d), true))); }src/tools/filesystem.ts (1)
250-256: Docstring is accurate; add a note about canonicalizationGiven the security implications, mention that the check uses a canonical (symlink-resolved) path to avoid bypasses, once the fix is applied.
📜 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 (2)
src/config-manager.ts(2 hunks)src/tools/filesystem.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/config-manager.ts (1)
src/utils/system-info.ts (1)
SystemInfo(31-46)
src/tools/filesystem.ts (2)
src/config-manager.ts (1)
configManager(225-225)src/utils/capture.ts (1)
capture(277-284)
🔇 Additional comments (4)
src/tools/filesystem.ts (4)
872-872: Good: writeFile now explicitly marks validation as a write operationThis ensures the new read-only policy is enforced on file writes.
930-931: Good: createDirectory enforces write policyDirectory creation correctly passes
truetovalidatePath, preventing creation inside protected paths.
941-943: Good: moveFile validates both source and destination as write operationsMoving out of or into a protected directory will now be blocked. This matches the intended protection semantics.
257-257: validatePath write flag usage verifiedAll write-related wrappers now correctly pass
isWriteOperation = truetovalidatePath. No missing flags were detected in the codebase, so the signature change is safe.
| /** | ||
| * 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 enforcement can be bypassed via symlinks; also unify normalization and trailing-separator handling
Current check uses path.normalize(...).toLowerCase() on raw strings and compares before any symlink resolution. An attacker can write into a protected directory via a symlinked parent (e.g., write to /tmp/link_to_/var/log/file where /tmp/link_to_/var/log → /var/log). Because the check occurs pre-realpath, it won’t match /var/log and the write is allowed.
Also, unlike isPathAllowed, this function doesn’t trim trailing separators, so config entries like /var/log/ may fail to match.
Fix: compare against a canonical, symlink-resolved path for both the target and each protected directory; reuse normalizePath and trim trailing separators.
Apply this diff within this function:
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 || [];
if (readOnlyDirs.length === 0) {
return false; // No read-only directories configured
}
- const normalizedCheckPath = path.normalize(checkPath).toLowerCase();
+ // Resolve symlinks for the deepest existing ancestor, then normalize and lower-case
+ let normalizedCheckPath = normalizePath(await resolveToRealPathOrAncestor(checkPath));
+ if (normalizedCheckPath.endsWith(path.sep)) {
+ normalizedCheckPath = normalizedCheckPath.slice(0, -1);
+ }
for (const dir of readOnlyDirs) {
- const expandedDir = expandHome(dir);
- const normalizedDir = path.normalize(expandedDir).toLowerCase();
+ const expandedDir = expandHome(dir);
+ let normalizedDir = normalizePath(await resolveToRealPathOrAncestor(expandedDir));
+ if (normalizedDir.endsWith(path.sep)) {
+ normalizedDir = normalizedDir.slice(0, -1);
+ }
// Check if the path is within the read-only directory
if (normalizedCheckPath === normalizedDir ||
normalizedCheckPath.startsWith(normalizedDir + path.sep)) {
return true;
}
}
return false;
}Place this helper (outside this range) to resolve the deepest existing ancestor to a real path while preserving the remainder:
// Helper: resolve symlinks for existing ancestors, then re-attach remaining segments
async function resolveToRealPathOrAncestor(p: string): Promise<string> {
const expanded = expandHome(p);
const absolute = path.isAbsolute(expanded) ? path.resolve(expanded) : path.resolve(process.cwd(), expanded);
let current = absolute;
const tail: string[] = [];
// Walk up until we find an existing path
while (true) {
try {
const real = await fs.realpath(current);
return path.join(real, ...tail.reverse());
} catch {
const parent = path.dirname(current);
if (parent === current) {
// Nothing exists; return normalized absolute best-effort
return absolute;
}
tail.push(path.basename(current));
current = parent;
}
}
}| // 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.`); | ||
| } | ||
|
|
There was a problem hiding this comment.
Read-only check happens before symlink resolution in validatePath
This is the same bypass vector described above. Perform the policy checks (allowed/read-only) on a symlink-resolved, canonical path (or existing-ancestor real path for non-existent targets) to prevent writes via symlinked parents.
Apply this diff in validatePath to compute a canonical path once and use it consistently:
// Convert to absolute path
const absolute = path.isAbsolute(expandedPath)
? path.resolve(expandedPath)
: path.resolve(process.cwd(), expandedPath);
+ // Resolve symlinks for existing ancestors before applying policy checks
+ const policyAbsolute = await resolveToRealPathOrAncestor(absolute);
+
// Check if path is allowed
- if (!(await isPathAllowed(absolute))) {
+ if (!(await isPathAllowed(policyAbsolute))) {
capture('server_path_validation_error', {
error: 'Path not allowed',
allowedDirsCount: (await getAllowedDirs()).length
});
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)) {
+ if (isWriteOperation && await isPathReadOnly(policyAbsolute)) {
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.`);
}📝 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.
| // 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.`); | |
| } | |
| // Convert to absolute path | |
| const absolute = path.isAbsolute(expandedPath) | |
| ? path.resolve(expandedPath) | |
| : path.resolve(process.cwd(), expandedPath); | |
| // Resolve symlinks for existing ancestors before applying policy checks | |
| const policyAbsolute = await resolveToRealPathOrAncestor(absolute); | |
| // Check if path is allowed | |
| if (!(await isPathAllowed(policyAbsolute))) { | |
| capture('server_path_validation_error', { | |
| error: 'Path not allowed', | |
| allowedDirsCount: (await getAllowedDirs()).length | |
| }); | |
| 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(policyAbsolute)) { | |
| 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.`); | |
| } |
🤖 Prompt for AI Agents
In src/tools/filesystem.ts around lines 277 to 286, the read-only and policy
checks are being performed on the original path before resolving symlinks, which
allows bypass via symlinked parents; update validatePath to compute a single
canonical/resolved path at the start (use fs.realpath on the target or, for
non-existent targets, resolve to the nearest existing ancestor's realpath plus
the remaining segments) and then perform the allowed/read-only checks (and
capture logging) against that resolved canonical path instead of the original
requested path so all policy checks apply to the true filesystem location.
1 participant
PR 2: Read-Only Directory Protection
Title: feat: Add read-only directory protection
Description:
Implements protection for critical system directories from accidental modification.
Problem:
Currently, Desktop Commander allows writing to any accessible directory, including critical system paths like logs, configurations, and system files.
Solution:
This PR adds read-only directory protection through the
readOnlyDirectoriesconfiguration option.How it works:
Testing:
Tested with
test-readonly.js- verification successful.Use cases:
Summary by CodeRabbit
New Features
Chores