Skip to content

fix(filesystem): make move_file fail instead of silently overwriting the destination - #4630

Open
eeshsaxena wants to merge 1 commit into
modelcontextprotocol:mainfrom
eeshsaxena:fix/filesystem-move-file-no-overwrite
Open

fix(filesystem): make move_file fail instead of silently overwriting the destination#4630
eeshsaxena wants to merge 1 commit into
modelcontextprotocol:mainfrom
eeshsaxena:fix/filesystem-move-file-no-overwrite

Conversation

@eeshsaxena

Copy link
Copy Markdown

Fixes #4628.

move_file's description and the README both say the move fails if the destination already exists:

If the destination exists, the operation will fail.

But the handler calls fs.rename directly, which silently overwrites the destination and still reports success. Because this server ships no delete tool, an agent that reads the contract reasonably treats move_file as non-clobbering, so this quietly turns into unadvertised, irreversible file deletion: move any file onto an existing target and the target's contents are gone.

Fix

Added a moveFile helper in lib.ts that checks the destination first and rejects if anything already occupies it, then renames only when the target is free:

try {
  await fs.lstat(destinationPath);
} catch (error) {
  if ((error as NodeJS.ErrnoException).code === 'ENOENT') {
    await fs.rename(sourcePath, destinationPath);
    return;
  }
  throw error;
}
throw new Error(`Destination already exists: ${destinationPath}`);

lstat (not stat) is used so an existing symlink at the destination is detected rather than followed. The move_file handler now calls this helper. Behaviour now matches the documented contract, and the successful-move path is unchanged.

Tests

Added two cases to __tests__/lib.test.ts:

  • moves when the destination doesn't exist (rename is called)
  • rejects with "Destination already exists" and never calls rename when the destination is present

The second test fails against the old fs.rename-only behaviour and passes with the fix. Full file suite green:

cd src/filesystem && npm run build && npx vitest run __tests__/lib.test.ts
# 48 passed

…nation

The move_file tool description and README both state the operation fails if
the destination already exists, but the handler called fs.rename directly,
which silently overwrites the destination. Since this server ships no delete
tool, move_file effectively provided unadvertised, irreversible file
deletion: moving any file onto an existing target destroyed the target.

Add a moveFile helper in lib.ts that rejects when the destination already
exists (checked with lstat, so an existing symlink is detected rather than
followed) and only renames when the target is free, and use it from the
move_file handler. Behaviour now matches the documented contract.

Fixes modelcontextprotocol#4628
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)

1 participant