fix: secure built-in filesystem MCP root handling by DeJeune · Pull Request #13294 · CherryHQ/cherry-studio

DeJeune · 2026-03-07T18:32:09Z

What this PR does

Before this PR:

The built-in filesystem MCP server accepted absolute paths outside the configured workspace root.
The in-memory filesystem server only read WORKSPACE_ROOT, so roots configured via args were ignored.
The server registered tool handlers asynchronously, which could cause first-load mcp:list-tools failures.
Sensitive filesystem tools could be auto-approved by default.

After this PR:

Filesystem MCP paths are constrained to the configured workspace root after realpath resolution.
The built-in filesystem server resolves its root from WORKSPACE_ROOT or the configured args.
Tool handlers are registered before async directory initialization starts.
Built-in filesystem write, edit, and delete require 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:

Existing filesystem MCP configurations now default to safer approval behavior for destructive tools.
Path validation resolves symlinks and nearest existing ancestors to block directory escapes for both existing and newly created paths.

The following alternatives were considered:

Trusting UI configuration alone without enforcing the root in the filesystem tool layer.
Keeping auto-approve defaults unchanged and relying on users to disable risky tools manually.
Waiting for async initialization to finish before serving requests instead of registering handlers immediately.

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, and delete by default. Users who relied on automatic approval for these tools need to re-enable auto-approve explicitly.

Special notes for your reviewer

This PR does not change Redux state shape or IndexedDB schema.
Added regression coverage for root resolution, path escapes, symlink escapes, and filesystem approval defaults.

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.

PR: The PR description is expressive enough and will help future contributors
Code: Write code that humans can understand and Keep it simple
Refactor: You have left the code cleaner than you found it (Boy Scout Rule)
Upgrade: Impact of this change on upgrade flows was considered and addressed if required
Documentation: A user-guide update was considered and is present (link) or not required. Check this only when the PR introduces or changes a user-facing feature or behavior.
Self-review: I have reviewed my own code (e.g., via /gh-pr-review, gh pr diff, or GitHub UI) before requesting review from others

Release note

action required: The built-in filesystem MCP server now enforces the configured workspace root and requires manual approval for write, edit, and delete operations by default.

Signed-off-by: suyao <[email protected]>

EurFelux

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 ✅

resolveRealOrNearestExistingPath is a solid approach that handles both existing files (via realpath) and new files (by walking up to the nearest existing ancestor and resolving from there). This correctly blocks symlink-based directory escapes.
Splitting initialize() into registerHandlers() (sync) + ensureBaseDir() (async) fixes a real race condition where tools could be called before handlers were registered.
resolveFilesystemBaseDir elegantly supports both WORKSPACE_ROOT env and args fallback, 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 resolveRealOrNearestExistingPath exhausts 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.

src/main/mcpServers/filesystem/types.ts

src/main/mcpServers/filesystem/__tests__/filesystem.test.ts

src/renderer/src/store/mcp.ts

EurFelux · 2026-03-07T19:22:42Z

Note

This issue/comment/review was translated by Claude.

I recall that this issue didn't exist earlier. It may have been introduced by #11937 without strict review.

Original Content

印象里早些时候是没有这个问题的。可能是 #11937 未经严格审查引入了该问题。

Signed-off-by: suyao <[email protected]>

vaayne · 2026-03-08T00:38:06Z

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 测试的时候说过这个问题，我是故意没有这个限制的，因为有人配置了目录但是问题更多，不过加上了也好

vaayne · 2026-03-08T00:45:59Z

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

但是我仍然觉得使用绝对路径会更好，看你们吧，和不和我都可以

EurFelux

LGTM! 🛡️ Solid security hardening for the built-in filesystem MCP server.

What's well done:

resolveRealOrNearestExistingPath correctly handles both existing files (via realpath) and new files (nearest ancestor resolution), blocking symlink-based directory escapes
Splitting initialize() → sync registerHandlers() + async ensureBaseDir() fixes the tool registration race condition
isPathWithinRoot with path.relative is clean and handles edge cases (Windows case-insensitivity, cross-drive paths)
Good test coverage: root containment, symlinks, relative traversal, ~ expansion, cwd() fallback
Backfill approach for disabledAutoApproveTools is pragmatic given v2 constraints

All previous review suggestions have been addressed. Approving.

EurFelux

LGTM! Solid security hardening for the built-in filesystem MCP server.

What's well done:

resolveRealOrNearestExistingPath correctly handles both existing files (via realpath) and new files (nearest ancestor resolution), blocking symlink-based directory escapes
Splitting initialize() → sync registerHandlers() + async ensureBaseDir() fixes the tool registration race condition
isPathWithinRoot with path.relative is clean and handles edge cases (Windows case-insensitivity, cross-drive paths)
Good test coverage: root containment, symlinks, relative traversal, ~ expansion, cwd() fallback
Backfill approach for disabledAutoApproveTools is pragmatic given v2 constraints

All previous review suggestions have been addressed. Approving.

src/renderer/src/store/mcp.ts

The backfill in initializeMCPServers was dead code — the function is exported but never called at runtime. Existing filesystem server entries in persisted Redux state would never receive disabledAutoApproveTools. Move the backfill to redux-persist migration version 200, which actually runs against persisted state on app startup. Remove the dead code from initializeMCPServers and update tests to verify the migration path. Co-Authored-By: Claude Opus 4.6 <[email protected]> Signed-off-by: suyao <[email protected]>

zhibisora · 2026-03-11T05:49:46Z

src/main/mcpServers/filesystem/config.ts

+    return envWorkspaceRoot
+  }
+
+  return args.find((arg) => typeof arg === 'string' && arg.trim().length > 0)?.trim()


Note: This change makes the built-in filesystem server effectively single-root by taking only the first non-empty arg, while the settings UI and copy still imply that multiple filesystem directories can be configured. I do not think this needs to block the security fix, but it would be good to either align the UI/copy with a single-root model or track multi-root support explicitly in a follow-up issue.

zhibisora · 2026-03-11T05:50:56Z

src/main/mcpServers/filesystem/tools/glob.ts

 - Pattern syntax: * (any chars), ** (any path), {a,b} (alternatives), ? (single char)
 - Results are limited to 100 files
- The path parameter must be an absolute path if specified
+- The path parameter must resolve within the configured workspace root if specified


Security: This updated description now promises that the path stays within the configured workspace root, but handleGlobTool still runs ripgrep with --follow and never re-validates each returned match against the root. A symlinked directory inside the workspace can therefore still make glob enumerate files outside the configured root. Please either drop --follow or validate each ripgrep result with the same realpath and root check before returning it.

zhibisora · 2026-03-11T06:05:04Z

Follow-up: to clarify my earlier COMMENT review, I consider the remaining glob symlink-traversal gap blocking and would like that fixed before merge. The built-in filesystem MCP can still enumerate files outside the configured root by following symlinked directories without re-validating each returned match against the root.

The single-root vs multi-directory configuration note is non-blocking and can be addressed either in this PR or in a follow-up issue.

The glob tool uses ripgrep --follow which traverses symlinks, and the returned file paths were not re-validated against the workspace root. Similarly, the grep tool (both ripgrep and fallback paths) and the ls tool's recursive mode could enumerate content outside the root via symlinked directories. Re-validate each result path against the workspace root in glob, grep, and ls to filter out entries that resolve outside the configured root after symlink resolution. Co-Authored-By: Claude Opus 4.6 <[email protected]> Signed-off-by: suyao <[email protected]>

DeJeune · 2026-03-12T07:40:38Z

Fixed in 61d68c6. All three enumeration tools now re-validate each result path against the workspace root after resolution:

glob: Each file path returned by ripgrep --follow is passed through validatePath() before inclusion. Symlink escapes are silently dropped.
grep: Both the ripgrep code path and the fallback searchDirectory now validate paths. For ripgrep results, each match's file path is checked; for the fallback, each traversed entry is validated before descending or reading.
ls: In recursive mode, symlinked directories are validated via validatePath() before descending into them.

Added two regression tests:

glob excludes files reached via symlinked directories outside root
ls excludes symlinked directories outside root in recursive mode

Signed-off-by: suyao <[email protected]> # Conflicts: # src/renderer/src/store/migrate.ts

The glob symlink test called handleGlobTool which invokes runRipgrep, but runRipgrep depends on @anthropic-ai/claude-agent-sdk/package.json which is not exported in CI. Mock runRipgrep to return both the legit and escaped file paths, so the test validates the validatePath filtering without requiring the ripgrep binary. Co-Authored-By: Claude Opus 4.6 <[email protected]> Signed-off-by: suyao <[email protected]>

zhibisora

Thanks for addressing the symlink-traversal gap and tightening the filesystem MCP sandboxing. The remaining blocking issue I called out in glob is fixed on the latest PR head, the related coverage is in place, and CI is green. I still see the single-root versus multi-directory configuration mismatch as a non-blocking follow-up, but this looks good to merge from my side.

Signed-off-by: suyao <[email protected]> # Conflicts: # src/renderer/src/store/migrate.ts

### What this PR does Before this PR: - Version is 1.7.24 After this PR: - Version is 1.7.25 - Release notes updated in `electron-builder.yml` ### Why we need it and why it was done in this way Standard release workflow: collect commits since v1.7.24, generate bilingual release notes, bump version. The following tradeoffs were made: N/A The following alternatives were considered: N/A ### Breaking changes **Action Required**: The built-in filesystem MCP server now requires manual approval for write/edit/delete operations by default. ### Special notes for your reviewer **Included commits since v1.7.24:** ✨ New Features: - feat(websearch): add Querit search provider (#13050) - feat(minapp,provider): add MiniMax Agent, IMA mini apps and MiniMax Global, Z.ai providers (#13099) - feat(provider): add agent support filter for provider list (#11932) - feat(agent): add custom environment variables to agent configuration (#13357) - feat: add GPT-5.4 support (#13293) - feat(gemini): add thought signature persistence (#13100) 🐛 Bug Fixes: - fix: secure built-in filesystem MCP root handling (#13294) - fix: check underlying tool permissions for hub invoke/exec (#13282) - fix: remove approval countdown timers and add system notifications (#13281) - fix: agent tool status not stopping on abort (#13111) - fix: resolve spawn ENOENT on Windows for Code Tools (#13405) - fix: Use default assistant for topic auto-renaming (#13387) - fix(backup): defer auto backup during streaming response (#13307) - fix(ui): fix Move To submenu overflow (#13399) - fix: render xml fenced svg blocks as svg previews (#13431) - fix: correct Gemini reasoning params (#13388) - fix: improve Qwen 3.5 reasoning model detection (#13235) - fix: upgrade xAI web search to Responses API (#12812) - fix(api-server): relax chat completion validation (#13279) - fix: install or uninstall button state issues (#13114) - fix: improve update dialog behavior (#13363) - fix: auto-convert reasoning_effort to reasoningEffort (#12831) ### Checklist - [x] PR: The PR description is expressive enough and will help future contributors - [x] Code: Write code that humans can understand and Keep it simple - [x] Refactor: You have left the code cleaner than you found it (Boy Scout Rule) - [x] Upgrade: Impact of this change on upgrade flows was considered and addressed if required - [x] Documentation: A user-guide update was considered and is present (link) or not required. - [x] Self-review: I have reviewed my own code before requesting review from others ### Release note ```release-note See release notes in electron-builder.yml ``` --------- Signed-off-by: kangfenmao <[email protected]>

fix: secure builtin filesystem mcp

4630efe

Signed-off-by: suyao <[email protected]>

DeJeune requested a review from 0xfullex as a code owner March 7, 2026 18:32

DeJeune requested review from EurFelux and vaayne March 7, 2026 18:45

EurFelux reviewed Mar 7, 2026

View reviewed changes

test: harden filesystem mcp path coverage

1f7d356

Signed-off-by: suyao <[email protected]>

DeJeune requested review from EurFelux and zhibisora March 9, 2026 10:07

EurFelux approved these changes Mar 9, 2026

View reviewed changes

zhibisora reviewed Mar 9, 2026

View reviewed changes

src/renderer/src/store/mcp.ts Outdated Show resolved Hide resolved

DeJeune requested a review from zhibisora March 10, 2026 18:21

zhibisora reviewed Mar 11, 2026

View reviewed changes

DeJeune added the ready-to-merge This PR has sufficient reviewer approvals but is awaiting code owner approval. label Mar 12, 2026

DeJeune requested a review from zhibisora March 12, 2026 07:13

DeJeune and others added 2 commits March 12, 2026 15:46

Merge remote-tracking branch 'origin/main' into DeJeune/issue-12447-fix

7e5f701

Signed-off-by: suyao <[email protected]> # Conflicts: # src/renderer/src/store/migrate.ts

zhibisora approved these changes Mar 12, 2026

View reviewed changes

Merge remote-tracking branch 'origin/main' into DeJeune/issue-12447-fix

8b98a36

Signed-off-by: suyao <[email protected]> # Conflicts: # src/renderer/src/store/migrate.ts

kangfenmao merged commit da10cca into main Mar 13, 2026
7 checks passed

kangfenmao deleted the DeJeune/issue-12447-fix branch March 13, 2026 02:02

kangfenmao mentioned this pull request Mar 13, 2026

chore: release v1.7.25 #13438

Merged

6 tasks

Conversation

DeJeune commented Mar 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does

Why we need it and why it was done in this way

Breaking changes

Special notes for your reviewer

Checklist

Release note

Uh oh!

EurFelux left a comment

Choose a reason for hiding this comment

Review Summary

What's Good ✅

Suggestions (Non-blocking) 💡

Regarding v2 Blocked File

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

EurFelux commented Mar 7, 2026 • edited by kangfenmao Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vaayne commented Mar 8, 2026 • edited by kangfenmao Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vaayne commented Mar 8, 2026 • edited by kangfenmao Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EurFelux left a comment

Choose a reason for hiding this comment

Uh oh!

EurFelux left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zhibisora Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

zhibisora Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

zhibisora commented Mar 11, 2026 • edited by DeJeune Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DeJeune commented Mar 12, 2026

Uh oh!

zhibisora left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

DeJeune commented Mar 7, 2026 •

edited

Loading

EurFelux commented Mar 7, 2026 •

edited by kangfenmao

Loading

vaayne commented Mar 8, 2026 •

edited by kangfenmao

Loading

vaayne commented Mar 8, 2026 •

edited by kangfenmao

Loading

zhibisora commented Mar 11, 2026 •

edited by DeJeune

Loading