From ab9785d8533cd5ef20f93e0d960342afc6c37684 Mon Sep 17 00:00:00 2001 From: Will Gordon Date: Fri, 31 Jul 2026 22:21:28 -0400 Subject: [PATCH] fix(cursor): addresses quality-gate Layer 1.5 findings Restores a comment that misrepresented execFile's actual argv shape after an earlier bracket-removal fix, this time avoiding literal closing-bracket characters entirely so the openapi checker's naive array parser can't be broken by either version. Bounds the sweep- and manual-route-triggered tryIdeAuth() busy-timeout to 250ms (down from the interactive auto-import path's 2000ms), since both share the main event loop with all other in-flight requests and should fail fast on a WAL-lock collision rather than block the whole instance for up to ~4s. Has the manual refresh route bypass the sweep's IDE-auth dedup cache so a click always sees a fresh read, consistent with this plan's existing "manual actions never see stale cached data" convention. Documents the previously-missing agent-availability route in ROUTE_GUARD_TIERS.md's spawn-capable table. --- docs/security/ROUTE_GUARD_TIERS.md | 35 ++++++++++--------- .../providers/[id]/refresh-cursor/route.ts | 30 +++++++++++++--- src/lib/cursor/renewal.ts | 17 ++++++++- src/lib/cursor/tokenExtractor.ts | 21 +++++++++-- src/server/authz/routeGuard.ts | 2 +- 5 files changed, 79 insertions(+), 26 deletions(-) diff --git a/docs/security/ROUTE_GUARD_TIERS.md b/docs/security/ROUTE_GUARD_TIERS.md index 3fd1f7e16a..5a533b88e5 100644 --- a/docs/security/ROUTE_GUARD_TIERS.md +++ b/docs/security/ROUTE_GUARD_TIERS.md @@ -39,23 +39,24 @@ spawn-capable route: a leaked token over a tunnel still can't reach the spawn. `check-route-guard-membership` gate enumerates every `route.ts` under the spawn-capable prefixes and fails CI if any is not classified local-only. -| Prefix / pattern | Why it's local-only | Manage-scope bypassable? | -| -------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ----------------------------- | -| `/api/mcp/` | MCP server — spawns stdio bridges + SSE handlers | **Yes** (only one) | -| `/api/cli-tools/runtime/` | CLI tool runtime — executes arbitrary plugin code | No — spawn-capable | -| `/api/services/` | Embedded services (9router/CLIProxy) — `npm install` + spawn | No — spawn-capable | -| `/dashboard/providers/services/` | Reverse proxy to embedded-service UIs | No | -| `/api/copilot/` | Unauthenticated LLM driver — CLI-only by default | Operator opt-in: manage/admin | -| `/api/tools/agent-bridge/` | AgentBridge — spawns MITM server + DNS edits | No — spawn-capable | -| `/api/tools/traffic-inspector/` | Traffic Inspector — http-proxy listener + system proxy | No — spawn-capable | -| `/api/plugins/`, `/api/plugins` | Plugins — load/execute via `worker_threads` + `child_process` | No — spawn-capable | -| `/api/system/version` | Auto-update (POST only; GET/HEAD/OPTIONS exempt) — spawns `git checkout` + `npm install` | No | -| `/api/db-backups/exportAll` | Spawns `tar` for the export archive | No | -| `/api/local/` | 1-click local launchers (Redis today) — spawns podman/docker | No — spawn-capable | -| `/api/headroom/start`, `/stop` | Headroom proxy lifecycle — spawns python CLI / signals PID | No — spawn-capable | -| `/api/oauth/cursor/auto-import` | `execFile("which", ["cursor"])` before importing creds | No | -| `/api/providers/{id}/login` (regex) | Launches a headful Playwright Chromium for web-cookie login | No | -| `/api/providers/{id}/refresh-cursor` (regex) | Manual Cursor session renewal — nudges `cursor-agent` (`--list-models`/`status` via `src/lib/cursor/renewal.ts`); the rest of `/api/providers/`, including the generic `/refresh`, intentionally stays remote-reachable | No — spawn-capable | +| Prefix / pattern | Why it's local-only | Manage-scope bypassable? | +| -------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ----------------------------- | +| `/api/mcp/` | MCP server — spawns stdio bridges + SSE handlers | **Yes** (only one) | +| `/api/cli-tools/runtime/` | CLI tool runtime — executes arbitrary plugin code | No — spawn-capable | +| `/api/services/` | Embedded services (9router/CLIProxy) — `npm install` + spawn | No — spawn-capable | +| `/dashboard/providers/services/` | Reverse proxy to embedded-service UIs | No | +| `/api/copilot/` | Unauthenticated LLM driver — CLI-only by default | Operator opt-in: manage/admin | +| `/api/tools/agent-bridge/` | AgentBridge — spawns MITM server + DNS edits | No — spawn-capable | +| `/api/tools/traffic-inspector/` | Traffic Inspector — http-proxy listener + system proxy | No — spawn-capable | +| `/api/plugins/`, `/api/plugins` | Plugins — load/execute via `worker_threads` + `child_process` | No — spawn-capable | +| `/api/system/version` | Auto-update (POST only; GET/HEAD/OPTIONS exempt) — spawns `git checkout` + `npm install` | No | +| `/api/db-backups/exportAll` | Spawns `tar` for the export archive | No | +| `/api/local/` | 1-click local launchers (Redis today) — spawns podman/docker | No — spawn-capable | +| `/api/headroom/start`, `/stop` | Headroom proxy lifecycle — spawns python CLI / signals PID | No — spawn-capable | +| `/api/oauth/cursor/auto-import` | `execFile("which", ["cursor"])` before importing creds | No | +| `/api/providers/{id}/login` (regex) | Launches a headful Playwright Chromium for web-cookie login | No | +| `/api/providers/{id}/refresh-cursor` (regex) | Manual Cursor session renewal — nudges `cursor-agent` (`--list-models`/`status` via `src/lib/cursor/renewal.ts`); the rest of `/api/providers/`, including the generic `/refresh`, intentionally stays remote-reachable | No — spawn-capable | +| `/api/providers/cursor/agent-availability` | Dashboard install-nudge check — spawns `cursor-agent status --format json` via `checkCursorAgentAvailability()`/`getCachedCursorAgentAvailability()` (`src/lib/cursor/renewal.ts`); credential-free response (`{cursorAgentAvailable: boolean}` only) | No — spawn-capable | **Response on violation:** `403 LOCAL_ONLY` diff --git a/src/app/api/providers/[id]/refresh-cursor/route.ts b/src/app/api/providers/[id]/refresh-cursor/route.ts index 8f6c2895cc..a1c0b098d9 100644 --- a/src/app/api/providers/[id]/refresh-cursor/route.ts +++ b/src/app/api/providers/[id]/refresh-cursor/route.ts @@ -2,12 +2,31 @@ import { NextResponse } from "next/server"; import { getCachedProviderConnectionById } from "@/lib/localDb"; import { updateProviderConnection } from "@/lib/db/providers"; import { sanitizeErrorMessage } from "@omniroute/open-sse/utils/error"; +import { tryIdeAuth } from "@/lib/cursor/tokenExtractor"; import { renewCursorConnection, buildCursorRenewedUpdate, runCursorRenewalExclusive, + BACKGROUND_IDE_AUTH_TIMEOUT_MS, } from "@/lib/cursor/renewal"; +/** + * A manual "Refresh" click must always see a fresh IDE-credential read, never + * a stale answer memoized by renewCursorConnection()'s default sweep-facing + * dedup cache (renewal.ts's dedupedTryIdeAuth, keyed host-wide with a 5s TTL + * — correct for coalescing multiple Cursor connections due in the SAME sweep + * tick, but wrong for a click that could otherwise reuse a read taken before + * THIS request's own nudge attempt completed). Matches the same + * always-uncached convention this plan already established for + * checkCursorAgentAvailability() vs. getCachedCursorAgentAvailability(). + * Keeps the shared short busy-timeout (not the interactive auto-import + * route's longer 2000ms default) since this request shares the same event + * loop as every other in-flight request on this instance. + */ +function uncachedTryIdeAuth(): ReturnType { + return tryIdeAuth({ timeoutMs: BACKGROUND_IDE_AUTH_TIMEOUT_MS }); +} + interface CursorConnectionLike { id: string; provider?: string; @@ -91,10 +110,13 @@ export async function POST(_request: Request, { params }: { params: Promise<{ id lastManualRefreshAttemptAt.set(connection.id, now); return await runCursorRenewalExclusive(connection.id, async () => { - const result = await renewCursorConnection({ - accessToken: connection.accessToken ?? "", - machineId: connection.providerSpecificData?.machineId as string | null | undefined, - }); + const result = await renewCursorConnection( + { + accessToken: connection.accessToken ?? "", + machineId: connection.providerSpecificData?.machineId as string | null | undefined, + }, + { tryIdeAuth: uncachedTryIdeAuth } + ); switch (result.status) { case "renewed": { diff --git a/src/lib/cursor/renewal.ts b/src/lib/cursor/renewal.ts index 8d4aa2dab9..1ab5071965 100644 --- a/src/lib/cursor/renewal.ts +++ b/src/lib/cursor/renewal.ts @@ -139,6 +139,21 @@ export async function getCachedCursorAgentAvailability(): Promise<{ const IDE_AUTH_DEDUP_TTL_MS = 5_000; +/** + * Busy-timeout for a background-triggered tryIdeAuth() open — deliberately + * much shorter than tryIdeAuth()'s own 2000ms default (used by the one-shot, + * user-initiated auto-import modal, a code path outside this orchestrator). + * Both the sweep AND the manual-refresh route share the same Node event loop + * as the HTTP server, so either one blocking on a WAL-lock collision stalls + * every other in-flight request on the instance — not just their own. An + * automated/backend-triggered call should fail fast and let the existing + * exponential circuit-breaker (sweep) or the user's next click (manual route) + * retry, rather than risk up to ~2s per driver in the fallback cascade + * (worst case ~4s total). Exported so the manual-refresh route can reuse the + * same value when it bypasses the dedup cache below for freshness. + */ +export const BACKGROUND_IDE_AUTH_TIMEOUT_MS = 250; + let cachedIdeAuthCall: { home: string; promise: ReturnType; @@ -161,7 +176,7 @@ function dedupedTryIdeAuth(): ReturnType { if (cachedIdeAuthCall && cachedIdeAuthCall.home === home && cachedIdeAuthCall.expiresAt > now) { return cachedIdeAuthCall.promise; } - const promise = tryIdeAuth(); + const promise = tryIdeAuth({ timeoutMs: BACKGROUND_IDE_AUTH_TIMEOUT_MS }); cachedIdeAuthCall = { home, promise, expiresAt: now + IDE_AUTH_DEDUP_TTL_MS }; return promise; } diff --git a/src/lib/cursor/tokenExtractor.ts b/src/lib/cursor/tokenExtractor.ts index c21c77d533..fda379004b 100644 --- a/src/lib/cursor/tokenExtractor.ts +++ b/src/lib/cursor/tokenExtractor.ts @@ -229,14 +229,27 @@ export async function tryAgentAuth(): Promise<{ * Cursor renames a key in a future release. * * Linux and Windows code paths are unchanged. + * + * `options.timeoutMs` bounds the SQLite busy-timeout on the open (default + * 2000ms, byte-identical for existing callers). The unattended sweep path + * (`src/lib/cursor/renewal.ts`) passes a much shorter override — an + * automated background job that fails to acquire the lock quickly should + * fail fast and let the existing exponential circuit-breaker retry on a + * later tick, rather than blocking the shared Node event loop for up to + * ~2s per driver in the fallback cascade (worst case ~4s: better-sqlite3's + * busy-timeout elapsing, then node:sqlite's). The one-shot, user-initiated + * `/api/oauth/cursor/auto-import` modal action keeps the longer default, + * since a single explicit click reasonably can wait longer for a better + * one-time success rate. */ -export async function tryIdeAuth(): Promise<{ +export async function tryIdeAuth(options?: { timeoutMs?: number }): Promise<{ found: boolean; accessToken?: string; machineId?: string; source?: string; error?: string; }> { + const timeoutMs = options?.timeoutMs ?? 2000; const platform = process.platform; const candidates = cursorDbCandidatePaths(platform, { home: homedir(), @@ -291,8 +304,10 @@ export async function tryIdeAuth(): Promise<{ // sweep tick (src/lib/cursor/renewal.ts::renewCursorConnection()) on every // near-expiry cycle, not just the explicit auto-import modal action, so a // WAL-lock collision with a running Cursor IDE needs a retry window on - // every driver path (see driverFactory.ts::toNodeSqliteOptions()). - db = tryOpenSync(dbPath, { readonly: true, fileMustExist: true, timeout: 2000 }); + // every driver path (see driverFactory.ts::toNodeSqliteOptions()). The + // sweep path overrides `timeoutMs` to a much shorter value (see the + // options.timeoutMs doc comment above). + db = tryOpenSync(dbPath, { readonly: true, fileMustExist: true, timeout: timeoutMs }); if (!db) { if (platform === "darwin") { return { diff --git a/src/server/authz/routeGuard.ts b/src/server/authz/routeGuard.ts index f1e04efa64..ca7fa1ff56 100644 --- a/src/server/authz/routeGuard.ts +++ b/src/server/authz/routeGuard.ts @@ -53,7 +53,7 @@ export const LOCAL_ONLY_API_PREFIXES: ReadonlyArray = [ "/api/local/", // T-12: 1-click local service launchers (Redis today; spawns podman/docker) — loopback-enforced by isLocalRequestAllowed() in src/lib/security/localEndpoints.ts (Hard Rules #15 + #17) "/api/headroom/start", // Headroom token-saver proxy lifecycle: spawns headroom-ai python CLI (Hard Rules #15 + #17) "/api/headroom/stop", // Headroom token-saver proxy lifecycle: sends SIGTERM/SIGKILL to managed PID (Hard Rules #15 + #17) - "/api/oauth/cursor/auto-import", // spawns `execFile("which", "cursor")` to verify a local Cursor install before importing creds — RCE-via-tunnel surface (Hard Rules #15 + #17, found by 6A.8 route-guard gate). Specific path only: the rest of /api/oauth/ (browser redirect/callback flows) must stay remote-reachable. + "/api/oauth/cursor/auto-import", // spawns execFile("which", argv-array-of-one-arg "cursor") to verify a local Cursor install before importing creds — RCE-via-tunnel surface (Hard Rules #15 + #17, found by 6A.8 route-guard gate). Specific path only: the rest of /api/oauth/ (browser redirect/callback flows) must stay remote-reachable. Note: this comment intentionally avoids a literal closing square bracket character — check-openapi-security-tiers.mjs's naive regex parser for this array stops at the first one it finds, silently truncating its view of every entry after this one. "/api/skills/collect/", // Skill Collector CLI detection: GET .../detect probes getCliRuntimeStatus() per CLI_TOOL_IDS entry, which spawns a child process to check each tool — RCE-via-tunnel surface (Hard Rules #15 + #17, PR #6294 review). "/api/discovery/", // Discovery tool (opt-in provider scanner): the scan route makes outbound probes to provider endpoints (SSRF-adjacent) and the whole surface is an admin research tool — strict-loopback only, no manage-scope bypass (NOT in LOCAL_ONLY_MANAGE_SCOPE_BYPASS_PREFIXES). See _tasks/features-v3.8.42/gaps/DISCOVERY_TOOL_DESIGN.md. VNC_ROUTE_PREFIX, // #7892: /api/vnc-session/* spawns Docker containers via child_process.spawn (src/lib/vncSession/service.ts) — RCE-via-tunnel surface (Hard Rules #15 + #17), same CVE class (GHSA-fhh6-4qxv-rpqj).