Skip to content

fix(filesystem): prevent move_file from overwriting existing destination (#4628) - #4709

Closed
LuckTerence wants to merge 1 commit into
modelcontextprotocol:mainfrom
LuckTerence:fix/filesystem-move-file-overwrite-guard
Closed

fix(filesystem): prevent move_file from overwriting existing destination (#4628)#4709
LuckTerence wants to merge 1 commit into
modelcontextprotocol:mainfrom
LuckTerence:fix/filesystem-move-file-overwrite-guard

Conversation

@LuckTerence

Copy link
Copy Markdown

Closes #4628

Problem

The move_file tool handler in @modelcontextprotocol/server-filesystem previously called fs.rename(validSourcePath, validDestPath) directly without verifying whether the destination path already existed. Under POSIX and Node.js semantics, fs.rename silently overwrites existing target files, leading to unexpected data loss. This contradicted the tool specification and documentation ("If the destination exists, the operation will fail").

Fix

  • Added an existence check for validDestPath using await fs.stat(validDestPath) before invoking fs.rename.
  • If the destination already exists (file or directory), throws Error(Destination already exists: ${args.destination}).
  • If fs.stat throws ENOENT, execution proceeds to fs.rename as expected. Any unexpected I/O errors are rethrown.

Tests

  • Added unit & integration regression tests in src/filesystem/__tests__/move-file.test.ts:
    • Verified move_file succeeds when moving a file to a non-existent destination.
    • Verified move_file fails with an explicit error when destination file exists, leaving both source and destination file contents intact (data loss prevention).
    • Verified move_file fails when destination directory exists, leaving source and destination intact.
    • Verified move_file succeeds when moving an entire directory to a non-existent destination.
  • Ran test suite: all 9 test suites and 161 tests passed.

@olaservo

Copy link
Copy Markdown
Member

Closing in favour of #4630, which has been merged for #4628. Thanks for the PR.

@olaservo olaservo closed this Aug 28, 2026
@LuckTerence

Copy link
Copy Markdown
Author

Superseded by #4630, which was merged earlier today — closing this rather than leaving a conflicting duplicate in the queue.

Worth noting #4630 landed the guard inside moveFile() in src/filesystem/lib.ts and uses lstat rather than stat, so a symlink already sitting at the destination is detected instead of being followed. That's the better check, and it covers file, directory and symlink cases in one place.

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.

server-filesystem: move_file silently overwrites an existing destination (data loss)

2 participants