From 821fe75f5d80eba0a6b98b1a4133bf25b582e27b Mon Sep 17 00:00:00 2001 From: Diogo Soares Rodrigues Date: Mon, 27 Jul 2026 14:50:38 -0300 Subject: [PATCH] fix(cursor): close download hardlink escape, MCP mime and read range Hard link escape: a hardlink inside the workspace is a regular file that passes containment AND `O_NOFOLLOW` while sharing its inode with a file anywhere else, so truncating it clobbers that file. Proven before the fix. The open now drops `O_TRUNC`, checks `nlink`/regular on the OPEN handle, and truncates only after - the pattern `autolearn/managed-skills.ts` already uses. `O_NOFOLLOW` covers the final component only; the parent-swap window is documented, not claimed shut. MCP resource discovery: `getServerResources` is async and awaits `ensureServerResources`, so a frame arriving while a server's catalog still loads no longer reads the empty cache and reports "advertises nothing" - a lie the model cannot distinguish from the truth. Mixed-content reads: the mime type came from `contents[0]` while the payload came from whichever item supplied it, so an image blob followed by a text note sent the text as `image/png`. Ranged `pi_read`: a plain `:N+K` selector pads one leading and three trailing context lines, so offset 5/limit 20 handed Cursor lines 4-27. Ranged reads compose `:raw:N+K`, verified against a real `ReadTool`. The wire result is an opaque string, so the gutter `raw` drops is not part of the contract. (cherry picked from commit 679785aa6b3243ea39b26abf4dda9435960019b1) --- packages/ai/CHANGELOG.md | 1 + packages/ai/src/providers/cursor-pi-args.ts | 14 +- packages/ai/test/cursor-exec-modern.test.ts | 6 +- packages/coding-agent/CHANGELOG.md | 4 +- packages/coding-agent/src/cursor.ts | 85 +++++++- packages/coding-agent/src/sdk.ts | 8 +- packages/coding-agent/src/tools/path-utils.ts | 24 ++- .../coding-agent/test/cursor-exec.test.ts | 185 ++++++++++++++++-- 8 files changed, 284 insertions(+), 43 deletions(-) diff --git a/packages/ai/CHANGELOG.md b/packages/ai/CHANGELOG.md index 12292282d..ddbd82ecd 100644 --- a/packages/ai/CHANGELOG.md +++ b/packages/ai/CHANGELOG.md @@ -62,6 +62,7 @@ ### Fixed +- Fixed the `pi_read` range translation padding the slice it asks for. `piReadPath` composed a plain `:N+K` selector, which the local `read` tool expands by one leading and three trailing context line — so a frame naming offset 5/limit 20 received lines 4-27. Ranged Pi reads now compose `:raw:N+K`; the wire result is an opaque output string, so the line-number gutter `raw` also drops carries nothing the contract needs. - Fixed four Cursor exec frames answering with a result whose oneof was never set. In proto3 that is not an empty result — the server reads it as "the tool ran and produced nothing", indistinguishable from real success. `listMcpResourcesExecResult`, `readMcpResourceExecResult`, `recordScreenResult` and `computerUseResult` now send `ListMcpResourcesSuccess{resources: []}`, `ReadMcpResourceNotFound{uri}`, `RecordScreenFailure` and `ComputerUseError` respectively. - The MCP resource frames now answer from the host instead of a fixed verdict. `CursorExecHandlers` gained `listMcpResources`/`readMcpResource`, so a host holding live MCP connections advertises them; the empty catalog and `not_found` above remain the answer when no handler is supplied. A handler that throws surfaces as `ListMcpResourcesError`/`ReadMcpResourceError` rather than collapsing into "none exist", which the model cannot retry. A read carrying `download_path` forwards it and answers with `ReadMcpResourceSuccess.download_path` and no content, which is what that mode means. - Fixed Cursor `connect_scm` calls losing their repository and settling on a fabricated verdict. The target rides in the `ConnectScmArgs.target` oneof, so reading a flat `github` property always saw `undefined`; and the authoritative `success`/`error`/`rejected` result only arrives on the completion frame, so answering at the announcement persisted a fixed failure for every call — including the ones the server went on to accept. The block now opens on the start frame and settles from the completion's decoded result. diff --git a/packages/ai/src/providers/cursor-pi-args.ts b/packages/ai/src/providers/cursor-pi-args.ts index ec9e43177..b546691b5 100644 --- a/packages/ai/src/providers/cursor-pi-args.ts +++ b/packages/ai/src/providers/cursor-pi-args.ts @@ -22,21 +22,29 @@ import * as path from "node:path"; /** - * A `pi_read` range composed onto the path as `read`'s inline `:N+K` selector. + * A `pi_read` range composed onto the path as `read`'s inline `:raw:N+K` + * selector. * * `read` exposes no range kwargs, so an uncomposed range reads the whole file. * `offset` is a 1-indexed start clamped like the reference's * `Math.max(0, offset - 1)` over 0-indexed lines; `limit` is a line count. * `null` marks a present `limit: 0` — zero lines, which no selector expresses * and which must not degrade into a whole-file read. + * + * The range is `raw` because a plain `:N+K` deliberately pads with one leading + * and three trailing context lines: helpful for a human reading a snippet, + * wrong for a caller that asked for exactly `limit` lines from `offset`. The + * wire result is an opaque `output` string, so the hashline and line-number + * gutter that `raw` also drops carry nothing the frame's contract needs. + * A range-free read keeps the ordinary form — whole-file reads want them. */ export function piReadPath(readPath: string, offset?: number, limit?: number): string | null { if (limit !== undefined && Math.floor(limit) <= 0) return null; const start = offset !== undefined ? Math.max(1, Math.floor(offset)) : undefined; const count = limit !== undefined ? Math.floor(limit) : undefined; if (start === undefined && count === undefined) return readPath; - if (start === undefined) return `${readPath}:1+${count}`; - return count === undefined ? `${readPath}:${start}-` : `${readPath}:${start}+${count}`; + if (start === undefined) return `${readPath}:raw:1+${count}`; + return count === undefined ? `${readPath}:raw:${start}-` : `${readPath}:raw:${start}+${count}`; } /** diff --git a/packages/ai/test/cursor-exec-modern.test.ts b/packages/ai/test/cursor-exec-modern.test.ts index 8ff570343..ff11d93ce 100644 --- a/packages/ai/test/cursor-exec-modern.test.ts +++ b/packages/ai/test/cursor-exec-modern.test.ts @@ -896,9 +896,9 @@ describe("Cursor modern exec frames: Pi tools", () => { expect(blocks[0].name).toBe("read"); expect(results.map(r => r.toolCallId)).toEqual([blocks[0].id]); // The displayed args must be the operation that actually runs. The bridge - // composes offset/limit into `read`'s `:N+K` selector, so a block showing - // the bare path claims a whole-file read that never happened. - expect(blocks[0].arguments).toEqual({ path: "/repo/a.ts:5+20" }); + // composes offset/limit into `read`'s `:raw:N+K` selector, so a block + // showing the bare path claims a whole-file read that never happened. + expect(blocks[0].arguments).toEqual({ path: "/repo/a.ts:raw:5+20" }); }); it("maps a failing Pi handler onto the frame's own error variant", async () => { diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index b39c9bf61..e547133d9 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -143,9 +143,11 @@ - 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 `..`. 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 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` — awaiting a server's background resource discovery rather than reading the not-yet-populated cache and reporting "advertises nothing" — and 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. That path arrives from the server while the general-purpose resolver deliberately honors absolute paths and `..`, so downloads are confined to the workspace: the resolved target and its deepest existing ancestor must stay inside it, and a target that is itself a symlink is refused. The write then opens `O_NOFOLLOW` and refuses a non-regular or hard-linked file before truncating, so the final component cannot be swapped for a link or an inode shared outside after the check. A parent directory replaced by a symlink mid-write is still followed; closing that needs `openat`/dirfd walking, which this does not attempt. - 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. +- Fixed a mixed-content MCP resource read reaching Cursor mislabelled. The mime type was taken from the first content item while the payload came from whichever item supplied it, so an image blob followed by a text note sent the text as `image/png`. Each branch now reports the type of the part it actually sends. +- Fixed `pi_read`'s `offset`/`limit` returning more lines than the frame asked for. The range is composed onto the local `read` tool's inline selector, and a plain `:N+K` deliberately pads with one leading and three trailing context lines — helpful when a human reads a snippet, wrong for a caller that named an exact range: offset 5/limit 20 handed Cursor lines 4-27. Ranged Pi reads now compose `:raw:N+K`, which slices exactly the requested lines. ## [17.1.5] - 2026-07-27 diff --git a/packages/coding-agent/src/cursor.ts b/packages/coding-agent/src/cursor.ts index 2f4267a32..eb3cfecab 100644 --- a/packages/coding-agent/src/cursor.ts +++ b/packages/coding-agent/src/cursor.ts @@ -1,5 +1,6 @@ import { randomUUID } from "node:crypto"; import * as fs from "node:fs"; +import * as path from "node:path"; import type { AgentEvent, AgentTool, @@ -90,16 +91,70 @@ interface CursorExecBridgeOptions { * `list_mcp_resources` / `read_mcp_resource` ask what this client's servers * advertise. Without this the bridge answers an empty catalog and * `not_found`, hiding resources the session is in fact connected to. + * + * `getServerResources` is async because a server's catalog loads in the + * background after its tools register: a frame arriving in that window would + * otherwise read the not-yet-populated cache and report an empty catalog, + * which is indistinguishable from a server that advertises nothing. */ mcpResources?: { serverNames(): string[]; getServerResources( name: string, - ): { resources: { uri: string; name?: string; description?: string; mimeType?: string }[] } | undefined; + ): Promise<{ resources: { uri: string; name?: string; description?: string; mimeType?: string }[] } | undefined>; readServerResource(name: string, uri: string): Promise; }; } +/** + * Write a downloaded resource without following a link at the target. + * + * The containment check and the write are separate syscalls, so a link planted + * at the target in between would redirect the bytes — the check cannot close + * that window on its own. `O_NOFOLLOW` decides it atomically for symlinks. + * + * A hard link needs a second check: it is a regular file that passes both the + * containment check and `O_NOFOLLOW` while sharing its inode with a file + * anywhere else on the volume, so truncating it overwrites that file too. + * `nlink > 1` on the OPEN handle is the test — statting the path first would + * reintroduce the race the open just closed. Same reasoning, and the same + * refusal, as `autolearn/managed-skills.ts`. + * + * Truncation therefore happens after that check rather than through `O_TRUNC`, + * which would have already destroyed the contents by the time it ran. + * + * Scope: `O_NOFOLLOW` applies to the FINAL component only. A parent directory + * swapped for an outward symlink between the check and this open is still + * followed; refusing that needs an `openat`/dirfd walk of every segment, which + * this does not attempt — an attacker who can rewrite the workspace tree + * mid-download is already inside the boundary this guard defends. + * + * Parent directories are created first, since the frame may name a path whose + * directories do not exist yet. + */ +async function writeWithoutFollowingLinks(absolutePath: string, payload: string | Buffer): Promise { + await fs.promises.mkdir(path.dirname(absolutePath), { recursive: true }); + const handle = await fs.promises.open( + absolutePath, + fs.constants.O_WRONLY | fs.constants.O_CREAT | fs.constants.O_NOFOLLOW, + ); + try { + const stat = await handle.stat(); + if (!stat.isFile()) { + throw new Error(`Refusing to download onto a non-regular file: ${absolutePath}`); + } + if (stat.nlink > 1) { + throw new Error( + `Refusing to download onto a file with ${stat.nlink} hard links, which would overwrite its other names: ${absolutePath}`, + ); + } + await handle.truncate(0); + await handle.writeFile(payload); + } finally { + await handle.close(); + } +} + function createToolResultMessage( toolCallId: string, toolName: string, @@ -598,9 +653,12 @@ export class CursorExecHandlers implements ICursorExecHandlers { const mcp = this.options.mcpResources; if (!mcp) return []; const names = server ? [server] : mcp.serverNames(); + // Concurrently: each name may block on that server's first catalog load, + // and a slow server should not delay the rest of the listing. + const catalogs = await Promise.all(names.map(async name => [name, await mcp.getServerResources(name)] as const)); const listed: CursorMcpResource[] = []; - for (const name of names) { - for (const resource of mcp.getServerResources(name)?.resources ?? []) { + for (const [name, catalog] of catalogs) { + for (const resource of catalog?.resources ?? []) { listed.push({ uri: resource.uri, name: resource.name, @@ -638,9 +696,16 @@ export class CursorExecHandlers implements ICursorExecHandlers { if (!mcp) return null; const read = await mcp.readServerResource(server, uri); if (!read) return null; - const mimeType = read.contents[0]?.mimeType; - const texts = read.contents.filter(item => item.text !== undefined).map(item => item.text as string); - const blob = read.contents.find(item => item.blob !== undefined)?.blob; + // The mime type must describe the bytes actually sent, not whatever item + // happened to be first: an image blob followed by a text note would + // otherwise label the text `image/png` and mislead the model about what + // it is holding. Each branch below takes the type from its own producer. + const textItems = read.contents.filter(item => item.text !== undefined); + const texts = textItems.map(item => item.text as string); + const blobItem = read.contents.find(item => item.blob !== undefined); + const blob = blobItem?.blob; + const textMimeType = textItems[0]?.mimeType; + const blobMimeType = blobItem?.mimeType; if (downloadPath) { // Text resources download as their own bytes; a blob decodes first. @@ -655,15 +720,15 @@ export class CursorExecHandlers implements ICursorExecHandlers { 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); + await writeWithoutFollowingLinks(absolutePath, payload); // The path echoed back is the one the frame asked for; the model // addresses it the same relative way. - return { uri, mimeType, downloadPath }; + return { uri, mimeType: texts.length > 0 ? textMimeType : blobMimeType, downloadPath }; } - if (texts.length > 0) return { uri, mimeType, text: texts.join("\n") }; + if (texts.length > 0) return { uri, mimeType: textMimeType, text: texts.join("\n") }; if (blob === undefined) return null; - return { uri, mimeType, blob: Buffer.from(blob, "base64") }; + return { uri, mimeType: blobMimeType, blob: Buffer.from(blob, "base64") }; } /** diff --git a/packages/coding-agent/src/sdk.ts b/packages/coding-agent/src/sdk.ts index 43c5dbcab..bbe65eee6 100644 --- a/packages/coding-agent/src/sdk.ts +++ b/packages/coding-agent/src/sdk.ts @@ -2660,7 +2660,13 @@ export async function createAgentSession(options: CreateAgentSessionOptions = {} // only live connections have any. mcpResources: mcpManager && { serverNames: () => mcpManager.getConnectedServers(), - getServerResources: name => mcpManager.getServerResources(name), + getServerResources: async name => { + // The manager registers a server's tools before its background + // resource load finishes, so a frame arriving in that window + // would read an empty cache and report "advertises nothing". + await mcpManager.ensureServerResources(name); + return mcpManager.getServerResources(name); + }, readServerResource: (name, uri) => mcpManager.readServerResource(name, uri), }, emitEvent: event => cursorEventEmitter?.(event), diff --git a/packages/coding-agent/src/tools/path-utils.ts b/packages/coding-agent/src/tools/path-utils.ts index 834a6778e..c7c9d2583 100644 --- a/packages/coding-agent/src/tools/path-utils.ts +++ b/packages/coding-agent/src/tools/path-utils.ts @@ -558,15 +558,13 @@ export function confineToWorkspace(filePath: string, cwd: string): string | null 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; - } + // to wherever it points. Chasing the chain to decide would mean + // reimplementing symlink resolution (multi-hop, relative hops, loops, and + // a TOCTOU window against a link that can be re-pointed between the check + // and the write). A download names a file to create, so a path that is + // already an unresolvable link is refused outright — the one shape where + // "cannot tell where this lands" is the whole answer. + if (isSymlink(resolved)) return 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 @@ -601,12 +599,12 @@ function tryRealpath(target: string): string | null { } } -/** The immediate link target, or `null` when the path is not a symlink. */ -function tryReadlink(target: string): string | null { +/** Whether the path itself is a symlink, without following it. */ +function isSymlink(target: string): boolean { try { - return fs.readlinkSync(target); + return fs.lstatSync(target).isSymbolicLink(); } catch { - return null; + return false; } } diff --git a/packages/coding-agent/test/cursor-exec.test.ts b/packages/coding-agent/test/cursor-exec.test.ts index cc718c12e..4b6571f1c 100644 --- a/packages/coding-agent/test/cursor-exec.test.ts +++ b/packages/coding-agent/test/cursor-exec.test.ts @@ -656,7 +656,7 @@ describe("CursorExecHandlers mounted tool bridge", () => { tools: new Map(), mcpResources: { serverNames: () => ["docs", "issues"], - getServerResources: name => + getServerResources: async name => name === "docs" ? { resources: [{ uri: "docs://readme", name: "README", mimeType: "text/markdown" }] } : { resources: [{ uri: "issues://open" }] }, @@ -672,6 +672,61 @@ describe("CursorExecHandlers mounted tool bridge", () => { expect((await handlers.listMcpResources({ server: "issues" })).map(r => r.uri)).toEqual(["issues://open"]); }); + it("waits for a server's catalog instead of reporting it empty", async () => { + // A server registers its tools before its resource catalog finishes + // loading. A frame landing in that window must not read the empty cache + // and answer "this server advertises nothing" - that is a lie the model + // cannot distinguish from the truth, and it will not ask again. + // The load is gated on a promise this test resolves, so "did the listing + // wait" is answered by the gate rather than by a duration. + const discovery = Promise.withResolvers(); + let loaded = false; + const handlers = new CursorExecHandlers({ + cwd: ".", + tools: new Map(), + mcpResources: { + serverNames: () => ["slow"], + getServerResources: async () => { + await discovery.promise; + loaded = true; + return { resources: [{ uri: "slow://ready" }] }; + }, + readServerResource: async () => undefined, + }, + }); + + const listing = handlers.listMcpResources({}); + // Nothing can have been reported yet: the catalog has not arrived. + expect(loaded).toBe(false); + discovery.resolve(); + expect((await listing).map(r => r.uri)).toEqual(["slow://ready"]); + expect(loaded).toBe(true); + }); + + it("labels a mixed-content read with the mime type of the part it sends", async () => { + // An image blob followed by a text note: the wire carries one payload, + // and its mime type has to describe that payload rather than whichever + // item happened to come first, or the model is told plain text is a PNG. + const handlers = new CursorExecHandlers({ + cwd: ".", + tools: new Map(), + mcpResources: { + serverNames: () => ["files"], + getServerResources: async () => undefined, + readServerResource: async (_name, uri) => ({ + contents: [ + { uri, mimeType: "image/png", blob: Buffer.from("PNG-BYTES").toString("base64") }, + { uri, mimeType: "text/plain", text: "a note about the image" }, + ], + }), + }, + }); + + const read = await handlers.readMcpResource({ server: "files", uri: "files://mixed" }); + expect(read?.text).toBe("a note about the image"); + expect(read?.mimeType).toBe("text/plain"); + }); + it("reads a resource and decodes a blob payload into wire bytes", async () => { // MCP hands back a list of content items with base64 blobs; the wire // carries one text or one byte payload. @@ -680,7 +735,7 @@ describe("CursorExecHandlers mounted tool bridge", () => { tools: new Map(), mcpResources: { serverNames: () => ["files"], - getServerResources: () => undefined, + getServerResources: async () => undefined, readServerResource: async (name, uri) => name === "files" && uri === "files://logo" ? { contents: [{ uri, mimeType: "image/png", blob: Buffer.from("PNG").toString("base64") }] } @@ -706,7 +761,7 @@ describe("CursorExecHandlers mounted tool bridge", () => { tools: new Map(), mcpResources: { serverNames: () => ["files"], - getServerResources: () => undefined, + getServerResources: async () => undefined, readServerResource: async (_name, uri) => ({ contents: [{ uri, mimeType: "image/png", blob: Buffer.from("PNG-BYTES").toString("base64") }], }), @@ -745,7 +800,7 @@ describe("CursorExecHandlers mounted tool bridge", () => { tools: new Map(), mcpResources: { serverNames: () => ["files"], - getServerResources: () => undefined, + getServerResources: async () => undefined, readServerResource: async (_name, uri) => ({ contents: [{ uri, text: "payload" }] }), }, }); @@ -765,7 +820,8 @@ 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. + // lands wherever the link points — as does a write to a dangling link, + // including one that only reaches outside through a second hop. const workspace = await fs.mkdtemp(path.join(os.tmpdir(), "cursor-mcp-symlink-")); try { const inner = path.join(workspace, "ws"); @@ -778,17 +834,28 @@ describe("CursorExecHandlers mounted tool bridge", () => { // 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")); + // Two hops: the first link stays inside, so resolving one level reads + // as contained while the write still follows the chain out. + await fs.symlink(path.join(inner, "second.txt"), path.join(inner, "chain.txt")); + await fs.symlink(path.join(outside, "chained.txt"), path.join(inner, "second.txt")); const handlers = new CursorExecHandlers({ cwd: inner, tools: new Map(), mcpResources: { serverNames: () => ["files"], - getServerResources: () => undefined, + getServerResources: async () => undefined, readServerResource: async (_name, uri) => ({ contents: [{ uri, text: "payload" }] }), }, }); - for (const escape of ["out/config", "out/deep/nested.txt", "link.txt", "existing.txt"]) { + for (const escape of [ + "out/config", + "out/deep/nested.txt", + "link.txt", + "existing.txt", + "chain.txt", + "second.txt", + ]) { await expect( handlers.readMcpResource({ server: "files", uri: "files://x", downloadPath: escape }), ).rejects.toThrow(/outside the workspace/); @@ -805,6 +872,67 @@ describe("CursorExecHandlers mounted tool bridge", () => { } }); + it("overwrites an existing download target in place", async () => { + // The download open is `O_NOFOLLOW`, which also drops the plain + // `Bun.write` path — so the ordinary case has to keep working: a + // re-download truncates the previous file rather than failing or + // appending to it. + const workspace = await fs.mkdtemp(path.join(os.tmpdir(), "cursor-mcp-rewrite-")); + try { + let payload = "a much longer first payload"; + const handlers = new CursorExecHandlers({ + cwd: workspace, + tools: new Map(), + mcpResources: { + serverNames: () => ["files"], + getServerResources: async () => undefined, + readServerResource: async (_name, uri) => ({ contents: [{ uri, text: payload }] }), + }, + }); + + await handlers.readMcpResource({ server: "files", uri: "files://x", downloadPath: "out/doc.txt" }); + payload = "second"; + await handlers.readMcpResource({ server: "files", uri: "files://x", downloadPath: "out/doc.txt" }); + + expect(await Bun.file(path.join(workspace, "out/doc.txt")).text()).toBe("second"); + } finally { + await removeWithRetries(workspace); + } + }); + + it("refuses a download onto a hard link that shares its inode outside", async () => { + // A hard link is a regular file that lives inside the workspace and + // passes both containment and `O_NOFOLLOW`, yet writing through it + // overwrites every other name for the same inode — including one out of + // reach. Only the link count on the opened file reveals it. + const workspace = await fs.mkdtemp(path.join(os.tmpdir(), "cursor-mcp-hardlink-")); + try { + const inner = path.join(workspace, "ws"); + const outside = path.join(workspace, "outside"); + await fs.mkdir(inner); + await fs.mkdir(outside); + const victim = path.join(outside, "secret.txt"); + await Bun.write(victim, "SECRET"); + await fs.link(victim, path.join(inner, "innocent.txt")); + const handlers = new CursorExecHandlers({ + cwd: inner, + tools: new Map(), + mcpResources: { + serverNames: () => ["files"], + getServerResources: async () => undefined, + readServerResource: async (_name, uri) => ({ contents: [{ uri, text: "payload" }] }), + }, + }); + + await expect( + handlers.readMcpResource({ server: "files", uri: "files://x", downloadPath: "innocent.txt" }), + ).rejects.toThrow(/hard links/); + expect(await Bun.file(victim).text()).toBe("SECRET"); + } 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. @@ -1147,7 +1275,8 @@ describe("CursorExecHandlers Pi frame translation", () => { it("composes pi_read's offset/limit onto the path as the read tool's range selector", async () => { // `read` takes no range kwargs, so a dropped offset/limit silently returns // the whole file. `offset` is a 1-indexed start and `limit` a line count, - // which is exactly the `:N+K` selector. + // which is exactly the `:N+K` selector — `raw`, since a plain range pads + // with context lines the frame never asked for. const { handlers, calls } = recordingHandlers("read"); await handlers.piRead({ toolCallId: "c1", args: { path: "a.ts", offset: 5, limit: 20 } } as never); @@ -1159,11 +1288,11 @@ describe("CursorExecHandlers Pi frame translation", () => { await handlers.piRead({ toolCallId: "c5", args: { path: "a.ts", offset: 0, limit: 20 } } as never); expect(calls).toEqual([ - { path: "a.ts:5+20" }, - { path: "a.ts:5-" }, - { path: "a.ts:1+20" }, + { path: "a.ts:raw:5+20" }, + { path: "a.ts:raw:5-" }, + { path: "a.ts:raw:1+20" }, { path: "a.ts" }, - { path: "a.ts:1+20" }, + { path: "a.ts:raw:1+20" }, ]); }); @@ -1180,6 +1309,38 @@ describe("CursorExecHandlers Pi frame translation", () => { expect(result.content).toEqual([{ type: "text", text: "" }]); }); + it("returns exactly the lines a pi_read range asked for", async () => { + // Producer/consumer contract against the real `ReadTool`: a plain `:N+K` + // selector deliberately pads with one leading and three trailing context + // lines, so offset 5/limit 20 would hand Cursor lines 4-27 for a request + // that named 5-24. The frame has no way to tell the padding apart from + // content it asked for. + const cwd = await fs.mkdtemp(path.join(os.tmpdir(), "cursor-piread-range-")); + try { + await Bun.write( + path.join(cwd, "n.txt"), + `${Array.from({ length: 40 }, (_, i) => `line${i + 1}`).join("\n")}\n`, + ); + const handlers = new CursorExecHandlers({ + cwd, + tools: new Map([["read", new ReadTool(createTestSession(cwd))]]), + }); + + const result = await handlers.piRead({ + toolCallId: "c1", + args: { path: "n.txt", offset: 5, limit: 20 }, + } as never); + const text = result.content + .filter(part => part.type === "text") + .map(part => (part as { text: string }).text) + .join(""); + + expect(text.trimEnd().split("\n")).toEqual(Array.from({ length: 20 }, (_, i) => `line${i + 5}`)); + } finally { + await removeWithRetries(cwd); + } + }); + it("escapes pi_grep's pattern when the frame asks for a literal search", async () => { // The local tool is regex-only, so an unescaped literal turns regex // metacharacters into operators and matches the wrong lines.