Skip to content

fix: validate paths in delete_files with validatePathWithinRepo#1006

Open
qinlongli2024-ai wants to merge 1 commit intoanthropics:mainfrom
qinlongli2024-ai:fix/delete-files-path-validation
Open

fix: validate paths in delete_files with validatePathWithinRepo#1006
qinlongli2024-ai wants to merge 1 commit intoanthropics:mainfrom
qinlongli2024-ai:fix/delete-files-path-validation

Conversation

@qinlongli2024-ai
Copy link

Summary

The delete_files MCP tool in github-file-ops-server.ts uses a weaker path validation than commit_files. While commit_files correctly calls validatePathWithinRepo() (which resolves symlinks and blocks .. traversal), delete_files only checks if absolute paths start with cwd and passes relative paths through without any validation.

The inconsistency

// commit_files — ✅ validated
const fullPath = await validatePathWithinRepo(filePath, REPO_DIR);

// delete_files — ❌ ad-hoc check only
if (filePath.startsWith("/")) {
  if (filePath.startsWith(cwd)) {
    return filePath.slice(cwd.length + 1);  // no symlink check
  }
}
return filePath;  // relative paths not checked at all

Fix

Replace the ad-hoc path processing in delete_files with the same validatePathWithinRepo() + relative path normalisation pattern used by commit_files:

const resolvedRepoDir = resolve(REPO_DIR);
const processedPaths = await Promise.all(
  paths.map(async (filePath) => {
    await validatePathWithinRepo(filePath, REPO_DIR);
    const normalizedPath = resolve(resolvedRepoDir, filePath);
    return normalizedPath.slice(resolvedRepoDir.length + 1);
  }),
);

This ensures both tools enforce identical path safety guarantees:

  • .. traversal is caught by realpath resolution
  • Symlinks escaping the repository root are blocked
  • Absolute paths outside the repo are rejected
  • Path normalisation is consistent

Checklist

  • Uses existing validated validatePathWithinRepo() function
  • Consistent with commit_files implementation
  • No breaking changes for legitimate paths
  • No new dependencies

The delete_files MCP tool used a simple startsWith(cwd) check for
absolute paths and accepted relative paths without any validation.
This is inconsistent with commit_files, which correctly calls
validatePathWithinRepo() to prevent path-traversal via '..' segments
and symlink escapes.

Replace the ad-hoc check with the same validatePathWithinRepo() call
used by commit_files, ensuring both tools enforce identical path
safety guarantees.

- Absolute paths that resolve outside the repo are now rejected
- Relative paths with '..' traversal are caught by realpath resolution
- Symlinks escaping the repository root are blocked
- Relative path normalisation is consistent with commit_files
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