Skip to content

fix(filesystem): preserve file permissions during write and edit operations#4115

Open
drudiger wants to merge 1 commit intomodelcontextprotocol:mainfrom
drudiger:fix/preserve-file-permissions
Open

fix(filesystem): preserve file permissions during write and edit operations#4115
drudiger wants to merge 1 commit intomodelcontextprotocol:mainfrom
drudiger:fix/preserve-file-permissions

Conversation

@drudiger
Copy link
Copy Markdown

@drudiger drudiger commented May 6, 2026

Description

The filesystem server's atomic write pattern (write to temp file + fs.rename()) replaces the original file's inode, causing the new file to inherit default 0644 permissions from the temp file. This silently strips execute bits and any other non-default permissions from the original file.

This fix captures stat.mode before the write and restores it with fs.chmod() after the rename, in both writeFileContent() and applyFileEdits().

Server Details

  • Server: filesystem
  • Changes to: tools (write_file, edit_file)

Motivation and Context

When using edit_file or write_file on an executable script (e.g., a shell script with chmod +x), the file loses its execute permission after the edit. This causes downstream failures when the script is invoked -- for example, a CI build calling ./pip_install.sh fails with "Permission denied" after the MCP server edits the file.

The root cause is the atomic write security pattern: fs.writeFile(tempPath) creates a new file with the process's default umask (typically 0644), then fs.rename(tempPath, filePath) atomically replaces the original inode. Since the rename replaces the inode entirely, the original file's permission bits are lost.

How Has This Been Tested?

  1. Unit tests (4 new tests added, all 149 pass):

    • writeFileContent preserves 755 permissions when overwriting existing file
    • applyFileEdits preserves 755 permissions after applying edits
    • applyFileEdits does NOT call chmod in dry run mode
    • Existing applyFileEdits tests updated with stat/chmod mocks in beforeEach
  2. Manual testing with MCP client:

    • Created a file with chmod 755, edited it via the edit_file MCP tool, confirmed permissions remained 755
    • Same test with write_file MCP tool, confirmed permissions preserved
    • Tested with a real Roo Code / VS Code MCP client session

Breaking Changes

None. This is a bug fix that restores expected behavior. No configuration changes needed.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • I have read the MCP Protocol Documentation
  • My changes follows MCP security best practices
  • I have updated the server's README accordingly
  • I have tested this with an LLM client
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have documented all environment variables and configuration options

Note: No README update is needed because this is a bug fix, not a new feature. No new environment variables or configuration options were introduced.

Additional context

The fix adds fs.stat() before the atomic write and fs.chmod() after fs.rename() in two locations:

  • writeFileContent() (line ~170 in lib.ts) -- used by the write_file tool
  • applyFileEdits() (line ~272 in lib.ts) -- used by the edit_file tool

There is an inherent TOCTOU window between stat and chmod where another process could change the file's permissions, but this is the same class of race that exists for the content read in applyFileEdits and is not practically exploitable in this context.

…ations

The atomic write pattern (write temp file + rename) replaces the original
inode, causing the new file to have default 0644 permissions regardless of
what the original file had. This breaks executable scripts and other files
with non-default permissions.

Fix: capture stat.mode before writing and restore it with chmod after rename.

Fixes both writeFileContent() and applyFileEdits().
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant