From 91b703b6afef8140965da50a6dceaba8c37fbe15 Mon Sep 17 00:00:00 2001 From: Diogo Soares Rodrigues Date: Mon, 27 Jul 2026 14:18:54 -0300 Subject: [PATCH] fix(cursor): resolve symlinks when confining resource downloads A lexical containment check is not containment. `out/config` is relative and `..`-free, so it passed - while a `ws/out -> /elsewhere` link inside the workspace sent the write straight out. Proven before the fix: the download landed in the link's target. Containment now realpaths three things: the target when it exists, the immediate link destination when it is a dangling symlink (a write still follows it), and otherwise the deepest existing ancestor with the not-yet-created segments re-applied. Each branch has a regression, and all three fail the suite when individually reverted. Also moves this branch's ai/catalog changelog entries back under [Unreleased]; two commits had re-landed them inside the released [17.1.5] section, which left `packages/ai/CHANGELOG.md` with two. All three released sections are now byte-identical to upstream/main. (cherry picked from commit e3ed4035ab8a1781c49a68c1fbe92c88bec3aa25) --- packages/ai/CHANGELOG.md | 15 ---- packages/catalog/CHANGELOG.md | 4 +- packages/coding-agent/CHANGELOG.md | 2 +- packages/coding-agent/src/tools/path-utils.ts | 75 ++++++++++++++++++- .../coding-agent/test/cursor-exec.test.ts | 43 +++++++++++ 5 files changed, 116 insertions(+), 23 deletions(-) diff --git a/packages/ai/CHANGELOG.md b/packages/ai/CHANGELOG.md index a30ff0ce8..12292282d 100644 --- a/packages/ai/CHANGELOG.md +++ b/packages/ai/CHANGELOG.md @@ -53,21 +53,6 @@ - Added `getProxyForUrl()` for transports that need provider-specific and standard proxy environment resolution with `NO_PROXY` support ([#6770](https://github.com/can1357/oh-my-pi/issues/6770)). - Added SiliconFlow and SiliconFlow (China) to the built-in API-key login provider catalog so `omp login siliconflow` / `omp login siliconflow-cn` stores a reusable credential validated against each region's `/v1/models` endpoint. -### Changed - -- The Cursor Pi arg translation (`piReadPath`, `piJoinPath`, `piLsPath`, `piEscapeRegexLiteral`, `piLimit`) moved to `providers/cursor-pi-args`, re-exported from `providers/cursor/exec-modern` so existing imports are unaffected. The legacy pi shim shares these helpers and is compiled into the bundled virtual module registry, where a nested `providers//` specifier is unresolvable under bunfs — and importing them from the exec module would drag the whole protobuf graph in for two string functions. - -## [17.1.5] - 2026-07-27 - -### Fixed - -- Fixed OpenAI Responses replay treating a tool output as paired with a matching call that appeared later in the input, or a tool call as paired with an earlier output. Pair repair now respects wire order before preserving or synthesizing each side. -- Fixed adaptive-thinking Anthropic models omitting the interleaved-thinking beta on signature-enforcing proxies, which caused persisted interleaved assistant turns to fail on replay ([#6717](https://github.com/can1357/oh-my-pi/issues/6717)). -- Kimi Code now sends its session-stable prompt cache key on both supported transports: `prompt_cache_key` for OpenAI-compatible requests and `metadata.user_id` for Anthropic-compatible requests. Explicit keys survive side-channel session IDs, while `cacheRetention: "none"` still disables automatic affinity ([#6049](https://github.com/can1357/oh-my-pi/issues/6049)). -- Fresh encrypted auth-broker snapshot caches are revalidated within a short startup budget, so one-shot clients see newly imported or revoked credentials immediately when the broker is reachable while retaining cache fallback for transport and server failures. -- Fixed custom `anthropic-messages` endpoints dropping native web-search call/result blocks in the leaked-thinking wrapper, preserving signed continuation history in source order without carrying a preceding text signature onto later unsigned blocks ([#6703](https://github.com/can1357/oh-my-pi/issues/6703)). -### Added - - Cursor's modern exec wire protocol is now handled end to end. `agent.proto` models the frames current Cursor CLI builds emit — the seven Pi tools (`ExecServerMessage` 45-51), hooks, subagents, allowlist prechecks, MCP state, smart-mode classification, canvas diagnostics, conversation search, agent-store conflicts and git diff — and every one of them gets a typed answer. The Pi frames run their local equivalents (`read`/`bash`/`edit`/`write`/`grep`/`glob`); the rest answer with the error, not-found or empty-but-valid variant that is actually true of this client. Frames this build cannot name at all now raise `ExecClientControlMessage.throw` with `unknown_exec_variant`, and recognised frames with no truthful answer (`git_diff_request`, whose `GetDiffResponse` has no error variant) raise `exec_variant_unsupported`, instead of a silent ack that leaves the server waiting. - `lsp` is advertised in the MCP tool catalog again. It was filtered out as a Cursor-native tool, but the native `diagnostics` frame covers one of roughly ten LSP actions, so the other nine were unreachable. diff --git a/packages/catalog/CHANGELOG.md b/packages/catalog/CHANGELOG.md index df136f5b9..77d0bc81f 100644 --- a/packages/catalog/CHANGELOG.md +++ b/packages/catalog/CHANGELOG.md @@ -32,6 +32,7 @@ ### Added - Added SiliconFlow providers (`siliconflow`, `siliconflow-cn`) with dynamic-only OpenAI-compatible model discovery: no bundled catalog — the model list is fetched live from each region's `/v1/models` endpoint, with non-chat entries (embedding, reranker, image, audio, video) filtered out. Discovery hydrates pricing, context/output limits, and reasoning metadata from the provider's models.dev catalog at runtime (with bundled upstream references as a reasoning-only fallback for ids models.dev has not indexed), so reasoning models keep thinking enabled and sessions compact against real context windows. `SILICONFLOW_API_KEY` / `SILICONFLOW_CN_API_KEY` environment variables are wired into `getEnvApiKey`. +- Regenerated the Cursor agent protobufs (`discovery/cursor-gen/agent_pb.ts`) against the modern `agent.proto`, adding the message and enum families current Cursor CLI builds emit: Pi tool exec frames, hook queries and responses, subagents, allowlist prechecks, MCP state, smart-mode classification, canvas diagnostics, conversation search, agent-store conflicts and git diff. Purely additive — no existing exported symbol changed shape. ## [17.1.5] - 2026-07-27 @@ -39,9 +40,6 @@ - Fixed Kimi Code (`kimi-code`) reporting `maxTokens: 32000` for every model — its `/coding/v1/models` discovery mapper and the bundled catalog applied a blanket constant, truncating `k3`/`k3-256k` output at ~4x below their real 131072 ceiling and `kimi-for-coding`/`kimi-for-coding-highspeed` below their 32768 ceiling. Output caps are now derived per family, and the model cache is invalidated so upgrades drop the stale `maxTokens: 32000` rows (including the discovery-only `k3-256k`) instead of serving them until the next network refresh ([#6711](https://github.com/can1357/oh-my-pi/issues/6711)). - Fixed Anthropic model discovery 404ing when the registry derived the provider base URL from a bundled model without the `/v1` suffix (`https://api.anthropic.com/models` instead of `/v1/models`), which let a stale text-only cache row shadow fresh models.dev vision metadata — surfacing as snapcompact refusing to run on `claude-opus-5`. Discovery now always targets `/v1/models` while model rows keep the provider base URL ([#6563](https://github.com/can1357/oh-my-pi/issues/6563)). -### Added - -- Regenerated the Cursor agent protobufs (`discovery/cursor-gen/agent_pb.ts`) against the modern `agent.proto`, adding the message and enum families current Cursor CLI builds emit: Pi tool exec frames, hook queries and responses, subagents, allowlist prechecks, MCP state, smart-mode classification, canvas diagnostics, conversation search, agent-store conflicts and git diff. Purely additive — no existing exported symbol changed shape. ## [17.1.4] - 2026-07-26 diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 40ed2c1a1..b39c9bf61 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 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 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 `..`. Containment resolves symlinks (target, dangling link target, and deepest existing ancestor), because a relative `..`-free path through a link that points outward still writes outward. - 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/tools/path-utils.ts b/packages/coding-agent/src/tools/path-utils.ts index da9a4e41d..834a6778e 100644 --- a/packages/coding-agent/src/tools/path-utils.ts +++ b/packages/coding-agent/src/tools/path-utils.ts @@ -526,9 +526,16 @@ export function resolveToCwd(filePath: string, cwd: string): string { * {@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 + * only a non-empty relative path landing under the live cwd is accepted, so * neither `/etc/passwd` nor `../../escape` can be written through. * + * The lexical check alone is not containment: a symlink inside the workspace + * can point anywhere, so `out/config` under a `ws/out -> /elsewhere` link is + * relative, `..`-free, and still writes outside. Both the target and its + * deepest existing ancestor are therefore realpath-resolved — the ancestor + * because a download names a file that does not exist yet, so the link in its + * path is the only thing that can be resolved before the write. + * * The cwd itself is rejected: a download names a file, never the directory. */ export function confineToWorkspace(filePath: string, cwd: string): string | null { @@ -538,9 +545,69 @@ export function confineToWorkspace(filePath: string, cwd: string): string | null 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; + if (!isUnderRootLexical(resolved, root)) return null; + + // A workspace reached through a link of its own is legitimate (/tmp on + // macOS), so the real root is the comparison basis. An unresolvable root is + // not a workspace to contain anything in. + const realRoot = tryRealpath(root); + if (!realRoot) return null; + + // An existing target is authoritative: resolve it outright. + const realTarget = tryRealpath(resolved); + if (realTarget) return isUnderRootLexical(realTarget, realRoot) ? resolved : null; + + // `realpath` also fails on a *dangling* link, and a write follows that link + // to wherever it points. Resolving one level answers where the bytes would + // actually land; anything unresolvable stays refused. + const linkTarget = tryReadlink(resolved); + if (linkTarget !== null) { + const dest = path.resolve(path.dirname(resolved), linkTarget); + const realDest = tryRealpath(path.dirname(dest)); + if (!realDest) return null; + return isUnderRootLexical(path.join(realDest, path.basename(dest)), realRoot) ? resolved : null; + } + + // Otherwise walk up to the deepest ancestor that does exist and check that, + // then re-apply the segments below it. Those segments are `..`-free by the + // lexical check above, so they cannot climb back out. + let ancestor = path.dirname(resolved); + const tail: string[] = [path.basename(resolved)]; + for (;;) { + const real = tryRealpath(ancestor); + if (real) { + return isUnderRootLexical(path.join(real, ...tail.reverse()), realRoot) ? resolved : null; + } + const parent = path.dirname(ancestor); + // Ran past the root without finding anything real: the workspace itself + // resolved above, so this cannot happen unless it vanished mid-check. + if (parent === ancestor || !isUnderRootLexical(ancestor, root)) return null; + tail.push(path.basename(ancestor)); + ancestor = parent; + } +} + +/** Whether `target` is a strict descendant of `root`, ignoring symlinks. */ +function isUnderRootLexical(target: string, root: string): boolean { + const relative = path.relative(root, target); + return !!relative && !relative.startsWith("..") && !path.isAbsolute(relative); +} + +function tryRealpath(target: string): string | null { + try { + return fs.realpathSync.native(target); + } catch { + return null; + } +} + +/** The immediate link target, or `null` when the path is not a symlink. */ +function tryReadlink(target: string): string | null { + try { + return fs.readlinkSync(target); + } catch { + return null; + } } export function formatPathRelativeToCwd( diff --git a/packages/coding-agent/test/cursor-exec.test.ts b/packages/coding-agent/test/cursor-exec.test.ts index ac272a246..cc718c12e 100644 --- a/packages/coding-agent/test/cursor-exec.test.ts +++ b/packages/coding-agent/test/cursor-exec.test.ts @@ -762,6 +762,49 @@ describe("CursorExecHandlers mounted tool bridge", () => { } }); + it("refuses a download path that escapes through a symlink", async () => { + // A lexical check is not containment. `out/config` is relative and + // `..`-free, but with `ws/out` linked outside the workspace the write + // lands wherever the link points — as does a write to a dangling link. + const workspace = await fs.mkdtemp(path.join(os.tmpdir(), "cursor-mcp-symlink-")); + try { + const inner = path.join(workspace, "ws"); + const outside = path.join(workspace, "outside"); + await fs.mkdir(inner); + await fs.mkdir(outside); + await fs.symlink(outside, path.join(inner, "out")); + await fs.symlink(path.join(outside, "dangling.txt"), path.join(inner, "link.txt")); + // A link whose target already exists: the write would overwrite the + // real file out there rather than create anything new. + await Bun.write(path.join(outside, "existing.txt"), "original"); + await fs.symlink(path.join(outside, "existing.txt"), path.join(inner, "existing.txt")); + 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 ["out/config", "out/deep/nested.txt", "link.txt", "existing.txt"]) { + await expect( + handlers.readMcpResource({ server: "files", uri: "files://x", downloadPath: escape }), + ).rejects.toThrow(/outside the workspace/); + } + expect(await Array.fromAsync(new Bun.Glob("**/*").scan({ cwd: outside }))).toEqual(["existing.txt"]); + expect(await Bun.file(path.join(outside, "existing.txt")).text()).toBe("original"); + + // A real workspace path still downloads: the guard resolves links, it + // does not refuse every path whose parents do not exist yet. + await handlers.readMcpResource({ server: "files", uri: "files://x", downloadPath: "deep/new/file.txt" }); + expect(await Bun.file(path.join(inner, "deep/new/file.txt")).text()).toBe("payload"); + } 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.