Skip to content

fix(mcp): sandbox tool handlers to allowed filesystem roots#112

Merged
runonthespot merged 2 commits into
mainfrom
fix/mcp-sandbox-escape
May 23, 2026
Merged

fix(mcp): sandbox tool handlers to allowed filesystem roots#112
runonthespot merged 2 commits into
mainfrom
fix/mcp-sandbox-escape

Conversation

@runonthespot
Copy link
Copy Markdown
Contributor

Problem (security: HIGH)

Every MCP tool handler accepted `request.path` verbatim:

```rust
let path_buf = PathBuf::from(request.path);
```

The only validation was an existence check. An agent connected over MCP could ask ck to index or semantically search any path readable by the host — `/etc`, another user's home, anything. `McpContext` stored a `cwd` field but nothing in the request pipeline enforced it.

Found via code review (task #9).

Fix

  1. `McpContext::allowed_roots: Vec` — canonical roots the MCP server is permitted to touch. Defaults to `[canonical(cwd)]`. Operators can extend via `CK_MCP_ALLOWED_ROOTS` env var (colon-separated, like `PATH`).
  2. `McpContext::resolve_request_path(raw) -> Result<PathBuf, ErrorData>` — parses the raw string, joins relative against cwd, rejects empty / non-existent paths with `invalid_params`, canonicalizes the result (so symlinks resolve), then verifies containment against `allowed_roots`.
  3. All 6 tool handlers (`semantic`, `lexical`, `regex`, `hybrid`, `index_status`, `reindex`) replace `PathBuf::from(path)` + ad-hoc existence check with one `resolve_request_path` call.

Tests (all 7 pass under `--no-default-features`)

  • `accepts_existing_path_inside_root`
  • `accepts_relative_path_inside_root`
  • `rejects_absolute_path_outside_root` ← the primary escape vector
  • `rejects_dot_dot_escape` ← `../..` traversal
  • `rejects_symlink_pointing_outside_root` (unix-gated) ← symlink trickery
  • `rejects_empty_path`
  • `rejects_nonexistent_path`

Notes

  • Independent of PR fix(ck-engine): apply path scope before top_k in semantic search #111 — touches `ck-cli/src/mcp/*` only.
  • Future work: extend allowed_roots config to a config file or per-tool overrides if operators need finer control. Env var is the minimum-viable knob.
  • Operator-visible behavior change: requests for paths outside the sandbox now fail with a clear `invalid_params` error mentioning `CK_MCP_ALLOWED_ROOTS`, instead of silently scanning them.

Test plan

  • `cargo test -p ck-search --no-default-features --lib sandbox_tests` — 7/7 pass
  • `cargo clippy -p ck-search --all-targets -- -D warnings` — clean
  • `cargo fmt --all --check` — clean
  • CI green

🤖 Generated with Claude Code

runonthespot and others added 2 commits May 23, 2026 22:43
Before this commit, every MCP tool handler accepted \`request.path\`
verbatim — \`let path_buf = PathBuf::from(request.path)\` — and the
only validation was an existence check. An agent connected over MCP
could ask ck to index or semantically search \`/etc\`, \`/Users/<other>\`,
or any readable path on the host. McpContext stored a \`cwd\` but
nothing enforced it.

Changes:

- McpContext gains a canonical \`allowed_roots\` vector. cwd is
  canonicalized at construction so symlink-based escape doesn't slip
  past containment. Operators can extend the sandbox with the
  \`CK_MCP_ALLOWED_ROOTS\` env var (colon-separated, like \`PATH\`).

- New \`McpContext::resolve_request_path(raw) -> Result<PathBuf, ErrorData>\`
  parses the request string, joins relative paths against cwd,
  rejects empty/non-existent paths with \`invalid_params\`, canonicalizes
  the result, and verifies it lives under at least one allowed root.

- All six tool handlers (semantic, lexical, regex, hybrid, index_status,
  reindex) replace their \`PathBuf::from(path)\` + ad-hoc existence
  check with one call to \`resolve_request_path\`.

- Seven sandbox tests cover: accepted absolute + relative inside-root
  paths, rejected absolute-outside, rejected \`..\` traversal, rejected
  symlink-pointing-outside (unix), rejected empty path, rejected
  nonexistent path. All pass under \`--no-default-features\` so they're
  exercised on every CI matrix slot.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The sandbox refactor changed the not-found error from
"Path does not exist" to "path does not exist" (lowercase, since
all other invalid_params messages are lowercase). Update the test
assertion to match on the case-stable substring "does not exist"
so it stays robust to either form.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@runonthespot runonthespot merged commit f2e327c into main May 23, 2026
14 checks passed
@runonthespot runonthespot deleted the fix/mcp-sandbox-escape branch May 23, 2026 19:21
runonthespot added a commit that referenced this pull request May 23, 2026
Ships the bug fixes and security work merged today:

- #111 fix: scoped semantic search returned [] when global top_k
  was consumed by chunks outside the requested path scope
- #112 security: MCP tool handlers were sandbox-escapable via
  any readable host path; added allowed_roots + canonicalize check
- #106 fix: MCP tool schemas now Gemini-API compatible
  (no more union types in JSON Schema)
- #100 fix: oneshot 0.1.13 patches a use-after-free race
- #99 chore: docs-site + ck-vscode dev-dep bumps

See CHANGELOG.md.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.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.

1 participant