refactor: update file write operations to use copy instead of rename …

[mahdirajaee]

…for improved safety

Description

Publishing Your Server

Note: We are no longer accepting PRs to add servers to the README. Instead, please publish your server to the MCP Server Registry to make it discoverable to the MCP ecosystem.

To publish your server, follow the quickstart guide. You can browse published servers at https://registry.modelcontextprotocol.io/.

Server Details

• Server:
• Changes to:

Motivation and Context

How Has This Been Tested?

Breaking Changes

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation update

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

Additional context

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 2m 10s —— View job

---

PR Review

• Read changed files and understand the approach
• Evaluate correctness and safety of the copy-vs-rename refactor
• Check test coverage
• Post full review

---

Summary

This PR replaces fs.rename(tempPath, filePath) with fs.copyFile(tempPath, filePath) + fs.unlink(tempPath) in writeFileContent() and applyFileEdits(), intending to fix Windows EPERM errors (issue #3199). However, this change introduces a critical security regression and the underlying PR description template is also completely unfilled.

---

Critical Issue: Security Regression (TOCTOU / Symlink Attack)

The original code was deliberately using rename() for a documented security reason — it was not a casual implementation choice. The now-removed comment explained:

// Security: Use atomic rename to prevent race conditions where symlinks
// could be created between validation and write. Rename operations
// replace the target file atomically and don't follow symlinks.

Here's why this matters:

• fs.rename(tempPath, filePath) on POSIX replaces the directory entry for filePath. If filePath is a symlink, the symlink itself is replaced — the symlink target is never written to. This is the key protection.
• fs.copyFile(tempPath, filePath) opens filePath for writing, which follows symlinks. If an attacker created a symlink at filePath pointing to /etc/passwd (or any sensitive file outside the allowed directory), copyFile would overwrite the symlink target instead of the symlink itself.

The filesystem server's core security model is path-based access control (validatePath). The temp+rename pattern was a second layer of defense ensuring the final write also can't escape that boundary. Removing it degrades the security posture of the server. Fix this →

---

Moderate Issue: The Fix May Not Actually Resolve the Windows EPERM

The Windows EPERM on rename() usually occurs when the destination file is locked (e.g., held open by an antivirus scanner, Windows Search, or another process). In that scenario, fs.copyFile(tempPath, filePath) also needs to open the destination file for writing — it will fail with the same EPERM. The change may not fix the underlying issue in the most common case.

A more targeted fix would be:

if (process.platform === 'win32') {
  // On Windows, rename can fail with EPERM when destination is locked.
  // copyFile+unlink is less atomic but handles this case.
  await fs.copyFile(tempPath, filePath);
  await fs.unlink(tempPath);
} else {
  // On POSIX: atomic, non-symlink-following replacement.
  await fs.rename(tempPath, filePath);
}

---

Minor: Error Handling

The shift from catch (err) { ...; throw err } to finally { ... } is functionally equivalent for error propagation in JavaScript — errors from copyFile still propagate after finally runs. No bug here, but the change is unnecessary if reverting to rename.

---

Tests

The test updates in src/filesystem/__tests__/lib.test.ts are mechanically correct — they properly mock copyFile and unlink instead of rename. However, they test the new (insecure) behavior rather than validating the security invariant.

---

PR Template

The PR description template is completely unfilled — no server specified, no motivation, no testing details, no checklist items checked. This makes it hard to understand the full scope and test coverage.

---

Verdict

Request changes. The security regression introduced by switching from rename to copyFile outweighs the benefit of addressing the Windows EPERM issue, especially since the fix may not work in the most common case anyway. The correct approach is a platform-conditional — use copyFile+unlink on Windows and keep rename on POSIX.