fix: secure built-in filesystem MCP root handling#13294
fix: secure built-in filesystem MCP root handling#13294
Conversation
Signed-off-by: suyao <sy20010504@gmail.com>
EurFelux
left a comment
EurFelux
left a comment
There was a problem hiding this comment.
Review Summary
This is a well-structured security fix that addresses a real vulnerability — the built-in filesystem MCP server previously had no path containment enforcement, allowing absolute paths outside the configured workspace root to be accessed freely.
What's Good ✅
resolveRealOrNearestExistingPathis a solid approach that handles both existing files (viarealpath) and new files (by walking up to the nearest existing ancestor and resolving from there). This correctly blocks symlink-based directory escapes.- Splitting
initialize()intoregisterHandlers()(sync) +ensureBaseDir()(async) fixes a real race condition where tools could be called before handlers were registered. resolveFilesystemBaseDirelegantly supports bothWORKSPACE_ROOTenv andargsfallback, making the configuration more flexible.- Backfill for existing configs is a pragmatic approach to ensure users upgrading get safer defaults without breaking their setup.
- Tests cover the key attack vectors: root containment, symlink escapes.
- Tool descriptions updated to reflect the new constraints — good attention to detail.
Suggestions (Non-blocking) 💡
- Additional test coverage: relative path traversal (
../../),~expansion, and no-baseDir fallback - Temp dir location: Consider using
os.tmpdir()instead of project-local.context/vitest-temp - Log warning when
resolveRealOrNearestExistingPathexhausts all ancestors (edge case) - TOCTOU awareness: Inherent limitation noted for future hardening consideration
Regarding v2 Blocked File
This PR modifies src/renderer/src/store/mcp.ts which has a v2 deprecation header. However, this is a security fix that qualifies as a critical bug fix under the contribution guidelines. The changes are minimal and don't alter the Redux state shape.
Overall: solid security improvement. The inline comments are suggestions for hardening, not blockers.
| while (true) { | ||
| try { | ||
| const realCurrentPath = await fs.realpath(currentPath) | ||
| const relativeSuffix = path.relative(currentPath, targetPath) | ||
| return normalizePath(path.join(realCurrentPath, relativeSuffix)) | ||
| } catch { | ||
| const parentPath = path.dirname(currentPath) | ||
| if (parentPath === currentPath) { | ||
| return normalizePath(targetPath) | ||
| } | ||
| currentPath = parentPath | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential edge case: root with no existing ancestor
When the workspace root path itself doesn't exist and neither does any of its ancestors (very extreme edge case, e.g. an external drive path before mounting), this loop reaches the filesystem root and falls back to normalizePath(targetPath) without resolving symlinks.
In practice this is unlikely to be exploitable since both resolvedRoot and resolvedPath would both get this fallback treatment, but worth considering adding a log warning when the loop exhausts all ancestors.
if (parentPath === currentPath) {
logger.warn('Could not resolve any existing ancestor for path', { targetPath })
return normalizePath(targetPath)
}nit: Not a blocker.
There was a problem hiding this comment.
Added a warning when ancestor resolution exhausts the path tree before finding any existing entry. This keeps the existing fallback behavior but makes the edge case visible in logs.
| export async function validatePath(requestedPath: string, baseDir?: string): Promise<string> { | ||
| const expandedPath = expandHome(requestedPath) | ||
| const root = baseDir ?? process.cwd() | ||
| const root = expandHome(baseDir ?? process.cwd()) | ||
| const absolute = path.isAbsolute(expandedPath) ? path.resolve(expandedPath) : path.resolve(root, expandedPath) | ||
|
|
||
| // Handle symlinks by checking their real path | ||
| try { | ||
| const realPath = await fs.realpath(absolute) | ||
| return normalizePath(realPath) | ||
| } catch (error) { | ||
| // For new files that don't exist yet, verify parent directory | ||
| const parentDir = path.dirname(absolute) | ||
| try { | ||
| const realParentPath = await fs.realpath(parentDir) | ||
| normalizePath(realParentPath) | ||
| return normalizePath(absolute) | ||
| } catch { | ||
| return normalizePath(absolute) | ||
| } | ||
| const resolvedRoot = await resolveRealOrNearestExistingPath(path.resolve(root)) | ||
| const resolvedPath = await resolveRealOrNearestExistingPath(absolute) | ||
|
|
||
| if (!isPathWithinRoot(resolvedPath, resolvedRoot)) { | ||
| throw new Error(`Access denied: Path is outside the configured workspace root: ${requestedPath}`) | ||
| } | ||
|
|
There was a problem hiding this comment.
TOCTOU consideration (informational)
There's an inherent TOCTOU (Time-of-check-time-of-use) window between validatePath resolving the real path and the actual file operation. For example, a symlink could be created between the validation and the file read/write.
This is a known limitation of path-based security validation in general and not specific to this PR. Just flagging for awareness — mitigating this fully would require something like O_NOFOLLOW or openat() which is beyond scope here.
Not a blocker — the current approach significantly raises the bar compared to no validation at all.
There was a problem hiding this comment.
Added regression coverage for relative traversal (../outside.txt), ~ expansion outside the root, and the process.cwd() fallback path when baseDir is omitted.
| expect(resolveFilesystemBaseDir(['C:/args-root'], {})).toBe('C:/args-root') | ||
| expect(resolveFilesystemBaseDir(['C:/args-root'], { WORKSPACE_ROOT: 'C:/env-root' })).toBe('C:/env-root') | ||
| expect(resolveFilesystemBaseDir([], {})).toBeUndefined() | ||
| }) |
There was a problem hiding this comment.
Suggestion: Add test cases for relative paths and ~ expansion
The current tests cover the key security scenarios well (root containment, symlink escapes). Consider adding coverage for:
- Relative path resolution:
validatePath('../../etc/passwd', workspaceRoot)should be rejected ~expansion:validatePath('~/sensitive-file', workspaceRoot)should be rejected if home dir is outside rootvalidatePathwith nobaseDir: verify fallback toprocess.cwd()still enforces containment
These would strengthen the regression suite for path traversal attacks.
| const existingFilesystemServer = existingServers.find((server) => server.name === BuiltinMCPServerNames.filesystem) | ||
| if (existingFilesystemServer && existingFilesystemServer.disabledAutoApproveTools === undefined) { | ||
| dispatch( | ||
| updateMCPServer({ | ||
| ...existingFilesystemServer, | ||
| disabledAutoApproveTools: [...filesystemManualApprovalTools] | ||
| }) | ||
| ) |
There was a problem hiding this comment.
Question: Backfill runs on every app initialization
This backfill code runs on every initializeMCPServers call. After the first run, disabledAutoApproveTools will be set, so the === undefined check will prevent re-applying.
However, what if a user intentionally clears disabledAutoApproveTools (sets it to undefined or deletes the property)? The next initialization would re-apply the defaults, overriding their choice.
A more robust approach might be to use a separate one-time migration flag, e.g. _filesystemApprovalMigrated: true, to track whether the backfill has been applied. But this is a minor concern — the current approach is pragmatic and covers the common case well.
nit: Not a blocker.
There was a problem hiding this comment.
I kept the current backfill approach for now. Adding a dedicated migration flag would mean introducing more state into src/renderer/src/store/mcp.ts, which is currently v2-blocked. In this case I think treating undefined as “apply the safer default” is the better tradeoff until a broader migration path is in place.
Signed-off-by: suyao <sy20010504@gmail.com>
|
Note This issue/comment/review was translated by Claude. @kangfenmao mentioned this issue during testing before. I intentionally didn't have this restriction because some people configured directories but had more issues, but adding it is also good. Original Content之前 @kangfenmao 测试的时候说过这个问题，我是故意没有这个限制的，因为有人配置了目录但是问题更多，不过加上了也好 |
|
Note This issue/comment/review was translated by Claude. I still think using absolute paths would be better, it's up to you guys, I'm fine either way Original Content但是我仍然觉得使用绝对路径会更好，看你们吧，和不和我都可以 |
3 participants
What this PR does
Before this PR:
WORKSPACE_ROOT, so roots configured viaargswere ignored.mcp:list-toolsfailures.After this PR:
WORKSPACE_ROOTor the configuredargs.write,edit, anddeleterequire manual approval by default, including a backfill for existing configs.Fixes #12447
Why we need it and why it was done in this way
The following tradeoffs were made:
The following alternatives were considered:
Links to places where the discussion took place: #12447, #13280
Breaking changes
The built-in filesystem MCP server now requires manual approval for
write,edit, anddeleteby default. Users who relied on automatic approval for these tools need to re-enable auto-approve explicitly.Special notes for your reviewer
Checklist
This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.
/gh-pr-review,gh pr diff, or GitHub UI) before requesting review from othersRelease note