fix(filesystem): create_directory doesn't create parent directories - #4631
Open
rileybuilds wants to merge 1 commit into
Open
fix(filesystem): create_directory doesn't create parent directories#4631rileybuilds wants to merge 1 commit into
rileybuilds wants to merge 1 commit into
Conversation
validatePath()'s ENOENT fallback checked only the immediate parent directory before allowing a new path through. Creating a path with more than one missing level (e.g. a/b/c when none of a, b, c exist) made the immediate parent check fail with "Parent directory does not exist", even though fs.mkdir was already called with recursive: true one level up in the create_directory handler and an existing allowed ancestor was available further up the tree. Walk up through missing ancestors until an existing one is found (or the filesystem root is reached), verifying that ancestor still resolves inside an allowed directory via realpath. The top-level lexical containment check already ran against the full target path, so this only extends how far the existence/symlink check climbs. Fixes modelcontextprotocol#4629
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #4629.
Bug
create_directoryalready callsfs.mkdir(validPath, { recursive: true }), butvalidatePath()'s security check for not-yet-existing paths only walked up one level before deciding a parent "does not exist":So creating
<allowed_dir>/_diag/nested/deepwhen neither_diagnor_diag/nestedexist yet fails withParent directory does not exist: <allowed_dir>/_diag/nested, even though<allowed_dir>itself is a real, allowed directory one level further up — exactly the repro in #4629. Callingcreate_directoryonce per level works, confirming only the recursive case was broken, matching the issue.Fix
Walk up through missing ancestors (not just the immediate parent) until an existing one is found, verifying via
fs.realpaththat it still resolves inside an allowed directory, or until the filesystem root is reached (in which case the original error is thrown). The top-level lexical containment check (isPathWithinAllowedDirectoriesagainst the full target path) already ran before this fallback, so this only extends how far the existence/symlink check climbs — it does not change what paths are considered in-bounds.Testing
__tests__/lib.test.ts: the existing'rejects when parent directory does not exist'test was actually asserting the buggy single-level behavior (2 missing levels under an otherwise-real allowed directory) — replaced with a test that a real allowed ancestor further up is found and the path is accepted, plus a new test that a target with no existing ancestor anywhere (all the way to the filesystem root) still correctly rejects.lib.tsand confirmed the new "walks past multiple missing ancestors" test fails with the exact error from the issue (Parent directory does not exist: .../nonexistent/nested), then restored the fix and confirmed it passes.npm testinsrc/filesystem).dist/index.jswith the MCP SDK client, calledcreate_directorywith the issue's exact repro path (_diag/nested/deep, 3 missing levels), got a success response, and confirmed with a realfs.statthat all three levels exist as real directories on disk.Scope note
The issue also flags that
write_filehas the same "parent must already exist" behavior, but is undocumented either way, and suggests deciding explicitly whether it should also become recursive. I leftwrite_fileunchanged — that's a separate documented-contract decision, whereascreate_directory's README/tool-description explicitly promise recursive creation and the code just didn't do it.