Skip to content

fix(filesystem): add stable fail-closed path validation reason codes - #4789

Open
tiagovilasboas wants to merge 2 commits into
modelcontextprotocol:mainfrom
tiagovilasboas:cursor/filesystem-path-validation-reasons-cb04
Open

fix(filesystem): add stable fail-closed path validation reason codes#4789
tiagovilasboas wants to merge 2 commits into
modelcontextprotocol:mainfrom
tiagovilasboas:cursor/filesystem-path-validation-reasons-cb04

Conversation

@tiagovilasboas

Copy link
Copy Markdown

Description

Filesystem path validation now fails closed with stable, machine-readable reason codes. Callers can classify denials without parsing free-form strings.

This implements the intended contract from #3525 on current main (which already walks missing ancestors via resolveUnicodeEquivalentPath). Open PR #3526 introduced PathValidationError on an older validatePath, but a bare inner catch remapped PARENT_OUTSIDE_ALLOWED to PARENT_DIRECTORY_NOT_FOUND, making that reason code dead. This PR keeps the coded error type and rethrows PathValidationError in both catch paths so PARENT_OUTSIDE_ALLOWED is observable.

Reason codes:

  • path_outside_allowed — requested path is outside configured roots
  • symlink_target_outside_allowed — existing path resolves (via symlink) outside allowed directories
  • parent_outside_allowed — missing path whose nearest existing ancestor resolves outside allowed directories
  • parent_directory_not_found — no resolvable ancestor directory exists

Publishing Your Server

Not applicable — this changes the existing filesystem server only.

Server Details

  • Server: filesystem
  • Changes to: path validation (validatePath / resolveUnicodeEquivalentPath)

Motivation and Context

Filesystem roots are the primary safety boundary for the reference filesystem server. Denial paths need a deterministic, auditable contract so symlink escapes and parent-directory escapes cannot be confused with a missing parent.

Fixes #3525
Related: #3526 (still open; this rebase onto current main avoids the catch-swallow bug called out in review)

How Has This Been Tested?

  • Unit tests in src/filesystem/__tests__/lib.test.ts assert instanceof PathValidationError and the reason field for all four codes (including mocked symlink-target and parent-outside cases).
  • Real-filesystem regressions in src/filesystem/__tests__/nested-parents.test.ts prove PATH_OUTSIDE_ALLOWED, SYMLINK_TARGET_OUTSIDE_ALLOWED, and PARENT_OUTSIDE_ALLOWED are observable (the last would fail if the inner catch swallowed the coded error).
  • Full filesystem package test suite (vitest with coverage).

I have not re-tested this through an interactive LLM client; coverage is the existing filesystem validation suite plus the new structured reason-code assertions.

Breaking Changes

No client configuration changes. Thrown messages keep the existing Access denied - ... / Parent directory does not exist text. Errors are now PathValidationError instances (still Error subclasses), so message-based handling continues to work.

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

Narrow AppSec change: no promotional links, no README listing updates, no new server. #3526 remains the prior attempt on stale validatePath; this PR is a clean implementation of that contract on current main with the review-requested catch fix and missing reason-code tests.

Throw PathValidationError with machine-readable reasons for fail-closed
denials, and rethrow coded errors in inner catch blocks so
PARENT_OUTSIDE_ALLOWED is not remapped to PARENT_DIRECTORY_NOT_FOUND.

Co-authored-by: Tiago Vilas Boas <tcarvalhovb@gmail.com>
Cover PATH_OUTSIDE_ALLOWED, SYMLINK_TARGET_OUTSIDE_ALLOWED,
PARENT_OUTSIDE_ALLOWED, and PARENT_DIRECTORY_NOT_FOUND with structured
assertions, including a real-filesystem parent-escape regression.

Co-authored-by: Tiago Vilas Boas <tcarvalhovb@gmail.com>
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.

Filesystem: fail closed on symlink traversal outside configured roots

1 participant