From 7a944f1baabdcb59e430502d22eda9e3a0745d89 Mon Sep 17 00:00:00 2001 From: Diogo Soares Rodrigues Date: Mon, 27 Jul 2026 14:01:25 -0300 Subject: [PATCH] fix(cursor): confine MCP resource downloads to the workspace `download_path` is workspace-relative by contract, but it arrives from the server and `resolveToCwd` deliberately honors absolute paths, `~`, and `..` - correct for a path a user typed, a write-anywhere primitive for one a remote peer supplied. `/etc/cron.d/x` or `../../escape` would have been written wherever the process can reach. `confineToWorkspace` accepts only a non-empty relative path resolving under the live cwd, and the download refuses anything else. The refusal throws inside the dispatch's existing try, so it reaches the model as a `ReadMcpResourceError` rather than a silent success or a crash. (cherry picked from commit 963cfee21a56576033ec115db745bb18ab0a8d06) --- packages/coding-agent/CHANGELOG.md | 2 +- packages/coding-agent/src/cursor.ts | 15 ++++++--- packages/coding-agent/src/tools/path-utils.ts | 23 +++++++++++++ .../coding-agent/test/cursor-exec.test.ts | 32 +++++++++++++++++++ 4 files changed, 67 insertions(+), 5 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 60a873dbb..40ed2c1a1 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -143,7 +143,7 @@ - Fixed `pi_bash` killing commands that explicitly asked for no deadline. `timeout` is `optional int32` and `bash` documents `0` as "disables the command deadline", but a truthiness check folded a supplied `0` into unset, applying the 300s default instead. A present `0` now passes through; negatives, which have no local meaning and would otherwise clamp to the 1s floor, still fall back to the default. - Fixed the Cursor exec bridge granting `edit` and `grep` to sessions that withheld them. Both bridge-only tools are constructed rather than looked up, and `executeTool` prefers a constructed override over the registry, so a restricted tool set (`toolNames` without them, or `restrictToolNames`) still got a working `pi_edit`/`pi_grep` — native frames arrive regardless of the advertised catalog. Both are now gated on the session having actually granted the tool, matching the `delete` frame's existing check (issue #5680). - Fixed Cursor advisor bridge tools bypassing approval settings. The advisor's `pi_edit`/`pi_grep` instances are approval-wrapped, but the wrapper reads `tools.approvalMode`, per-tool `tools.approval.` policies and `autoApprove` only from the execute-time tool context — which the advisor bridge never supplied, so every native advisor frame resolved as `yolo` with empty policies and ran past a configured `ask` or `deny`. Advisors now receive the same context store as the primary bridge. -- Fixed Cursor's `list_mcp_resources`/`read_mcp_resource` frames answering as though the client hosted no MCP servers. The bridge hardcoded an empty catalog and `not_found`, so resources from servers the session held live connections to were invisible to the model even while the same session read them through `mcp://`. Both frames now answer from the session's `MCPManager`; a lookup failure surfaces as an error rather than an empty catalog, which would read as "asked, none exist". A read carrying `download_path` writes the resource to that workspace-relative path and answers with the path alone, per the wire contract, instead of putting the payload back in the model's context. +- Fixed Cursor's `list_mcp_resources`/`read_mcp_resource` frames answering as though the client hosted no MCP servers. The bridge hardcoded an empty catalog and `not_found`, so resources from servers the session held live connections to were invisible to the model even while the same session read them through `mcp://`. Both frames now answer from the session's `MCPManager`; a lookup failure surfaces as an error rather than an empty catalog, which would read as "asked, none exist". A read carrying `download_path` writes the resource to that path and answers with the path alone, per the wire contract, instead of putting the payload back in the model's context — confined to the workspace, since that path arrives from the server and the general-purpose resolver deliberately honors absolute paths and `..`. - Fixed the Cursor native `delete` frame bypassing approval settings. Unlike every other frame it removes the file directly instead of running a registry tool, so no approval wrapper sat in front of it — `allowNativeDelete` answers whether a mutating tool was granted, which is a different question from whether the user's policy allows the call. A configured `tools.approval.delete: deny`, or an `always-ask` session that this channel cannot prompt in, now refuses the frame and keeps the file. - Fixed `pi_ls` never reporting that a listing was clipped. The bridge read the entry cap from a flat `details.resultLimitReached`, which `glob` sets but `read` — the tool serving `pi_ls` — does not: it records the cap through `OutputMeta` at `details.meta.limits.resultLimit.reached`. Every capped listing therefore reached Cursor with `entry_limit_reached` unset, reading as complete. Both shapes are now checked, the same way the truncation translation already handles its two producers. diff --git a/packages/coding-agent/src/cursor.ts b/packages/coding-agent/src/cursor.ts index 5962e08a5..2f4267a32 100644 --- a/packages/coding-agent/src/cursor.ts +++ b/packages/coding-agent/src/cursor.ts @@ -28,7 +28,7 @@ import { sanitizeText } from "@oh-my-pi/pi-utils"; import type { MCPResourceReadResult } from "./mcp/types"; import type { ApprovalMode } from "./tools/approval"; import { resolveApproval } from "./tools/approval"; -import { resolveToCwd } from "./tools/path-utils"; +import { confineToWorkspace, resolveToCwd } from "./tools/path-utils"; import type { TodoPhase, TodoStatus } from "./tools/todo"; /** Phase used for Cursor-owned tasks with no local phase grouping. */ @@ -647,10 +647,17 @@ export class CursorExecHandlers implements ICursorExecHandlers { const payload = texts.length > 0 ? texts.join("\n") : blob !== undefined ? Buffer.from(blob, "base64") : undefined; if (payload === undefined) return null; - const absolutePath = resolveToCwd(downloadPath, this.options.getCwd?.() ?? this.options.cwd); + // The path is workspace-relative BY CONTRACT, but it arrives from the + // server, and `resolveToCwd` deliberately honors absolute paths and + // `..` for user-authored tool input. Taking it at its word would let a + // frame write anywhere this process can reach, so confine it here + // rather than trusting the declaration. + const cwd = this.options.getCwd?.() ?? this.options.cwd; + const absolutePath = confineToWorkspace(downloadPath, cwd); + if (!absolutePath) throw new Error(`Refusing to download outside the workspace: ${downloadPath}`); await Bun.write(absolutePath, payload); - // The path echoed back is the one the frame asked for: it is - // workspace-relative by contract, and the model addresses it that way. + // The path echoed back is the one the frame asked for; the model + // addresses it the same relative way. return { uri, mimeType, downloadPath }; } diff --git a/packages/coding-agent/src/tools/path-utils.ts b/packages/coding-agent/src/tools/path-utils.ts index 747f0492d..da9a4e41d 100644 --- a/packages/coding-agent/src/tools/path-utils.ts +++ b/packages/coding-agent/src/tools/path-utils.ts @@ -520,6 +520,29 @@ export function resolveToCwd(filePath: string, cwd: string): string { return path.resolve(cwd, expanded); } +/** + * Resolve a path that MUST stay inside `cwd`, or `null` when it would escape. + * + * {@link resolveToCwd} deliberately honors absolute paths, `~`, and `..` — + * correct for a path a user typed, wrong for one a remote peer supplied. + * Callers handling untrusted input (Cursor's `download_path`) use this instead: + * only a non-empty relative path resolving under the live cwd is accepted, so + * neither `/etc/passwd` nor `../../escape` can be written through. + * + * The cwd itself is rejected: a download names a file, never the directory. + */ +export function confineToWorkspace(filePath: string, cwd: string): string | null { + if (!filePath || path.isAbsolute(filePath)) return null; + // `~` expands to an absolute path, and an internal URL is not a filesystem + // target at all; neither is a relative workspace path. + if (filePath.startsWith("~") || isInternalUrlPath(filePath)) return null; + const root = path.resolve(cwd); + const resolved = path.resolve(root, filePath); + const relative = path.relative(root, resolved); + if (!relative || relative.startsWith("..") || path.isAbsolute(relative)) return null; + return resolved; +} + export function formatPathRelativeToCwd( filePath: string, cwd: string, diff --git a/packages/coding-agent/test/cursor-exec.test.ts b/packages/coding-agent/test/cursor-exec.test.ts index de0866a5c..ac272a246 100644 --- a/packages/coding-agent/test/cursor-exec.test.ts +++ b/packages/coding-agent/test/cursor-exec.test.ts @@ -730,6 +730,38 @@ describe("CursorExecHandlers mounted tool bridge", () => { } }); + it("refuses a download path that escapes the workspace", async () => { + // `download_path` is workspace-relative by contract, but it comes from + // the server and the generic resolver honors absolute paths and `..` — + // correct for a path a user typed, a write-anywhere primitive for one a + // remote peer supplied. + const workspace = await fs.mkdtemp(path.join(os.tmpdir(), "cursor-mcp-escape-")); + const outside = path.join(workspace, "outside.txt"); + try { + const inner = path.join(workspace, "ws"); + await fs.mkdir(inner); + const handlers = new CursorExecHandlers({ + cwd: inner, + tools: new Map(), + mcpResources: { + serverNames: () => ["files"], + getServerResources: () => undefined, + readServerResource: async (_name, uri) => ({ contents: [{ uri, text: "payload" }] }), + }, + }); + + for (const escape of ["../outside.txt", outside, "nested/../../outside.txt"]) { + await expect( + handlers.readMcpResource({ server: "files", uri: "files://x", downloadPath: escape }), + ).rejects.toThrow(/outside the workspace/); + } + // Nothing was written on any of those attempts. + expect(await Bun.file(outside).exists()).toBe(false); + } finally { + await removeWithRetries(workspace); + } + }); + it("answers nothing when the session has no MCP manager", async () => { // A host without MCP must still answer truthfully rather than throwing: // an empty catalog and `not_found` are the honest responses.