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

[Mainfurr]

Package: @modelcontextprotocol/server-filesystem
Version: 2026.7.10
Node: v25.2.1
OS: Windows 11 (server process on Windows; reached over HTTP via supergateway)

Summary

move_file overwrites an existing destination file without warning and reports success. The README states the opposite:

• move_file
  • Move or rename files and directories
  • Inputs: source (string), destination (string)
  • Fails if destination exists

The tool description returned over the wire says the same thing:

If the destination exists, the operation will fail.

Because no existence check is performed, the destination file's contents are destroyed and unrecoverable. This is a data-loss bug, not just a documentation error: an agent that reads "fails if destination exists" will reasonably treat move_file as a safe, non-clobbering operation and use it without first checking whether the target is occupied.

The impact is amplified by the fact that this server ships no delete tool. Users (and agents) reasonably infer that the toolset cannot destroy existing files, only create and modify them. move_file quietly provides arbitrary file deletion: move anything onto a target and the target is gone.

That inference is actively encouraged by the README, which is itself inconsistent here. The Features section advertises:

Read/write files · Create/list/delete directories · Move files/directories

No delete tool exists in the API section or in the tool list returned at runtime. So a reader is told deletion is available, cannot find it, concludes the toolset is non-destructive by design — and then loses data to move_file.

Reproduction

Two files with different names, different contents, and different sizes, so the result is unambiguous.

1. write_file -> <allowed_dir>/_movetest/alpha_source.txt

   FILE ALPHA
   role: SOURCE
   unique marker: AAAA-1111-SOURCE
   length: this file is deliberately short.
   

2. write_file -> <allowed_dir>/_movetest/beta_destination.txt

   FILE BETA
   role: DESTINATION
   unique marker: BBBB-2222-DESTINATION
   ...(padding so the sizes differ)...
   

3. list_directory_with_sizes on _movetest:

   [FILE] alpha_source.txt                     97 B
   [FILE] beta_destination.txt                307 B
   Total: 2 files, 0 directories
   

4. move_file with source = alpha_source.txt, destination = beta_destination.txt

Expected: error — destination exists.

Actual: success, no warning:

Successfully moved <...>/_movetest/alpha_source.txt to <...>/_movetest/beta_destination.txt

5. list_directory_with_sizes on _movetest:

   [FILE] beta_destination.txt                 97 B
   Total: 1 files, 0 directories
   

6. read_text_file on beta_destination.txt returns ALPHA's contents. The marker BBBB-2222-DESTINATION is gone; 307 bytes were destroyed by an operation documented to refuse.

Cause

Presumably a bare fs.rename with no prior existence check — which clobbers on every platform Node supports.

POSIX rename(2) replaces an existing destination by definition. On Windows, libuv's fs__rename calls MoveFileEx with MOVEFILE_REPLACE_EXISTING (added in joyent/libuv#283 precisely to match Linux behaviour), and per the Win32 docs that flag replaces the contents of the destination file. The libuv docs accordingly describe uv_fs_rename as equivalent to rename(2).

So the overwrite is the platform default in both cases, and the README documents an intent that was never implemented in code.

Note on tool annotations

move_file currently carries destructiveHint: true, but the README's own rationale for it is "Deletes source file" — i.e. the annotation is justified by the loss of the source, and the destination is still assumed safe. See #2988, which proposed destructiveHint: false on the explicit grounds that "a repeat typically errors because source no longer exists and README says 'fails if destination exists'".

So the reasoning around this tool's safety has been anchored on a README claim the code does not honour. Worth revisiting the note and rationale even though the flag itself happens to be correct.

Suggested fix

Either:

(a) Implement the documented behaviour — stat the destination first and reject if it exists:

try {
  await fs.stat(validDestPath);
  throw new Error(`Destination already exists: ${destination}`);
} catch (e) {
  if (e.code !== "ENOENT") throw e;
}
await fs.rename(validSourcePath, validDestPath);

Note this is TOCTOU-racy but adequate for this server's threat model; a stricter option on Linux is renameat2 with RENAME_NOREPLACE.

(b) Keep the overwrite behaviour but fix the README and the tool description to say so plainly, mirroring write_file's existing wording ("exercise caution with this"), and add an explicit overwrite: boolean input defaulting to false.

(a) is preferable: it matches every published description of the tool, and clients that gate on destructiveHint today do so believing the destination is protected.