fix(security): prevent path traversal in getFileSkeleton (CWE-22)#35
Open
sebastiondev wants to merge 1 commit intoforloopcodes:mainfrom
Open
fix(security): prevent path traversal in getFileSkeleton (CWE-22)#35sebastiondev wants to merge 1 commit intoforloopcodes:mainfrom
sebastiondev wants to merge 1 commit intoforloopcodes:mainfrom
Conversation
…ry traversal (CWE-22) Validate that resolved file paths stay within rootDir before any file read. Also resolves symlinks to prevent symlink-based escapes. Without this check, an agent (or prompt-injected agent) could call get_file_skeleton with paths like "../../../../etc/passwd" to read arbitrary files outside the project root.
|
@sebastiondev is attempting to deploy a commit to the ForLoopCodes' projects Team on Vercel. A member of the Team first needs to authorize it. |
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.
Summary
This PR adds a path-containment check to the
getFileSkeletontool to prevent path traversal (CWE-22) when reading files via the MCPget_file_skeletontool.Vulnerability
src/tools/file-skeleton.tsgetFileSkeletonData flow
The MCP tool exposes
file_pathdirectly to callers. InsidegetFileSkeleton:path.resolvehappily collapses..segments and accepts absolute paths, so afile_pathlike../../../etc/passwdor/etc/passwdresolves outsiderootDirand is then read. There was no containment check before thereadFilecall. A symlink inside the project that points outsiderootDirwould also have been followed.This is the same class of issue already fixed in sibling tools in this repo:
proposeCommit— 43872ecshadow restore— 7c30ebawalker— 5383efbgetFileSkeletonwas the remaining unprotected file-read sink of this shape.Fix
Added an
assertInsideRoot(fullPath, rootDir)helper that:rootDirand the requested path, then verifies the requested path is the root or starts withroot + "/"(the trailing separator avoids the classic/srv/app-evilvs/srv/appprefix bug).fs.realpathon both and re-checks containment, so symlinks inside the project that point outside it are also rejected.ENOENTfromrealpath), falls back to the lexical check, preserving the existing error semantics for missing files.The check runs before
isSupportedFile/readFile, so it gates every code path through the function. Behaviour for legitimate in-tree paths is unchanged.Tests
Added
test/main/cwe22-file-skeleton.test.mjscovering:../traversal is rejectedrootDirare rejectedrootDirare rejectedFull suite passes locally: 212/212.
Security analysis
Exploitation requires the attacker to control the
file_pathargument toget_file_skeleton. In an MCP context this is reachable in two realistic ways:In either case, before this fix the server would return the contents (or a 20-line preview, for unsupported extensions) of any file readable by the server process —
.env, SSH keys, shadow files on misconfigured deployments, etc. The fix constrains reads torootDirand its descendants, matching the trust boundary the rest of the codebase already enforces.Adversarial review
Before submitting, we tried to talk ourselves out of this one. The tool name suggests it's only for project files, but nothing in the call path enforced that —
path.resolveis not a sandbox, and there's no allowlist, no chroot, and no framework-level filter between the MCP transport andreadFile. We also checked whether the project intentionally exposes arbitrary reads elsewhere; it doesn't, and the three prior CWE-22 fixes show the maintainers treatrootDiras the boundary. So the exposure is real and the fix is consistent with existing precedent.cc @lewiswigmore