Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/config-manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ export interface ServerConfig {
blockedCommands?: string[];
defaultShell?: string;
allowedDirectories?: string[];
readOnlyDirectories?: string[]; // Directories that can be read but not modified
requireExplicitPermission?: boolean; // Require explicit flag for destructive commands
allowedSudoCommands?: string[]; // Whitelist of allowed sudo commands with pattern support
telemetryEnabled?: boolean; // New field for telemetry control
fileWriteLineLimit?: number; // Line limit for file write operations
fileReadLineLimit?: number; // Default line limit for file read operations (changed from character-based)
Expand Down Expand Up @@ -131,6 +134,9 @@ class ConfigManager {
],
defaultShell: os.platform() === 'win32' ? 'powershell.exe' : '/bin/sh',
allowedDirectories: [],
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
telemetryEnabled: true, // Default to opt-out approach (telemetry on by default)
fileWriteLineLimit: 50, // Default line limit for file write operations (changed from 100)
fileReadLineLimit: 1000 // Default line limit for file read operations (changed from character-based)
Expand Down
53 changes: 47 additions & 6 deletions src/tools/filesystem.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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;
    }
  }
}

/**
* 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);
Expand All @@ -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.`);
}

Comment on lines +277 to +286
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
// 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.

// Check if path exists
try {
const stats = await fs.stat(absolute);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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 });
}

Expand All @@ -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);
}

Expand Down