diff --git a/apps/desktop/src/main/index.ts b/apps/desktop/src/main/index.ts index aa555a8e47..5b94b72b1a 100644 --- a/apps/desktop/src/main/index.ts +++ b/apps/desktop/src/main/index.ts @@ -34,6 +34,16 @@ import { createDesktopRuntime } from "./runtime.js"; // pinning them is worth the small extra surface. export { isAllowedChildWindowUrl, isHttpUrl } from "./runtime.js"; +// Re-export the path-allowlist helpers for the same reason (#974). +// shell.openPath is privileged main-process behaviour; pinning the +// validation gate via tests is worth the extra surface. +export { + validateExistingDirectory, + createProjectRootGate, + type PathValidationResult, + type ProjectRootGate, +} from "./runtime.js"; + const TOOLS_DEV_PARENT_PID_ENV = SIDECAR_ENV.TOOLS_DEV_PARENT_PID; export type DesktopMainOptions = { diff --git a/apps/desktop/src/main/preload.cts b/apps/desktop/src/main/preload.cts index 2cfe5e4229..f598e10e50 100644 --- a/apps/desktop/src/main/preload.cts +++ b/apps/desktop/src/main/preload.cts @@ -7,4 +7,6 @@ contextBridge.exposeInMainWorld('electronAPI', { ipcRenderer.invoke('dialog:pick-folder'), openPath: (path: string): Promise => ipcRenderer.invoke('shell:open-path', path), + registerProjectRoot: (path: string): Promise => + ipcRenderer.invoke('shell:register-project-root', path), }); diff --git a/apps/desktop/src/main/runtime.ts b/apps/desktop/src/main/runtime.ts index 887c0f8914..5193144c20 100644 --- a/apps/desktop/src/main/runtime.ts +++ b/apps/desktop/src/main/runtime.ts @@ -1,9 +1,106 @@ -import { mkdir, writeFile } from "node:fs/promises"; +import { mkdir, writeFile, realpath, stat } from "node:fs/promises"; import { dirname, isAbsolute, join, resolve } from "node:path"; import { fileURLToPath } from "node:url"; import { BrowserWindow, dialog, ipcMain, shell } from "electron"; +/** + * Result of validating a candidate path before exposing it to a + * privileged shell operation. + */ +export type PathValidationResult = + | { ok: true; resolved: string } + | { ok: false; reason: string }; + +/** + * Validates that a renderer-supplied string points at an existing + * absolute directory. Used as the floor check before consulting the + * project-root allowlist below — anything that fails this check + * (relative paths, files, apps, non-existent paths) is rejected + * outright. Returns the realpath-resolved canonical path on success + * so symlink games can't be used to register one path and reach + * another. + */ +export async function validateExistingDirectory(p: string): Promise { + if (typeof p !== "string" || p.length === 0) { + return { ok: false, reason: "path must be a non-empty string" }; + } + if (!isAbsolute(p)) { + return { ok: false, reason: "path must be absolute" }; + } + let resolvedReal: string; + try { + resolvedReal = await realpath(p); + } catch { + return { ok: false, reason: "path does not exist" }; + } + let st; + try { + st = await stat(resolvedReal); + } catch { + return { ok: false, reason: "path could not be stat'd" }; + } + if (!st.isDirectory()) { + return { ok: false, reason: "path is not a directory" }; + } + return { ok: true, resolved: resolvedReal }; +} + +/** + * Allowlist gate around `shell.openPath`. The renderer calls + * `register(absDir)` once per project mount, passing the daemon- + * validated `resolvedDir` for the active project. Subsequent + * `tryOpen(p)` calls only succeed when `p` resolves to a path that + * was previously registered. Validating against the allowlist + * (instead of only checking the path's existence) shrinks the + * renderer→main trust boundary that the openPath bridge introduces: + * without this, any XSS or compromised renderer dependency could + * forward arbitrary local paths into `shell.openPath`. + * + * Threat-model caveat: an attacker that fully controls the renderer + * can also call `register` directly with arbitrary paths. Closing + * that gap fully requires a daemon-side round-trip to derive the + * canonical `resolvedDir` from the daemon's project registry, which + * is deferred here to keep the PR focused. Today's allowlist still + * defends against accidental misuse, bugs, and common XSS payloads + * that don't know to call `register` first. + */ +export interface ProjectRootGate { + register(p: string): Promise; + isApproved(p: string): boolean; + size(): number; + /** Test-only: empty the in-memory allowlist between tests. */ + reset(): void; +} + +export function createProjectRootGate(): ProjectRootGate { + const approved = new Set(); + return { + async register(p: string): Promise { + const validated = await validateExistingDirectory(p); + if (!validated.ok) return false; + approved.add(validated.resolved); + return true; + }, + isApproved(p: string): boolean { + return approved.has(p); + }, + size(): number { + return approved.size; + }, + reset(): void { + approved.clear(); + }, + }; +} + +/** + * Singleton used by the live IPC handlers in `createDesktopRuntime`. + * Tests use `createProjectRootGate()` directly to avoid sharing state + * between cases. + */ +const projectRootGate = createProjectRootGate(); + const PENDING_POLL_MS = 120; const RUNNING_POLL_MS = 2000; const MAX_CONSOLE_ENTRIES = 200; @@ -278,6 +375,7 @@ export async function createDesktopRuntime(options: DesktopRuntimeOptions): Prom ipcMain.removeHandler("dialog:pick-folder"); ipcMain.removeHandler("shell:open-external"); ipcMain.removeHandler("shell:open-path"); + ipcMain.removeHandler("shell:register-project-root"); ipcMain.handle("dialog:pick-folder", async () => { const result = await dialog.showOpenDialog({ properties: ["openDirectory"] }); return result.canceled || result.filePaths.length === 0 ? null : result.filePaths[0]; @@ -291,17 +389,34 @@ export async function createDesktopRuntime(options: DesktopRuntimeOptions): Prom return false; } }); + // Renderer registers a project working directory that it has + // received from the daemon's GET /api/projects/:id (resolvedDir). + // Only paths that pass through this registration are eligible for + // shell.openPath below. + ipcMain.handle("shell:register-project-root", async (_event, p: string) => { + return projectRootGate.register(p); + }); // shell.openPath opens an absolute filesystem path in the OS file // manager (Finder / Explorer / Files). It resolves to '' on success // and to a non-empty error string on failure (per Electron's // contract). The web caller uses that empty/non-empty distinction // to decide between the success toast and the manual fallback toast. + // + // Path is gated by: + // 1. Shape: non-empty string, absolute path, realpath-resolves, + // points at an existing directory. + // 2. Allowlist: must be in the projectRootGate (registered by + // the renderer with a daemon-validated resolvedDir). + // Any other input returns a non-empty error string so the renderer + // shows the fallback toast naming the path manually. ipcMain.handle("shell:open-path", async (_event, p: string) => { - if (typeof p !== "string" || p.length === 0) { - return "open-path: empty path"; + const validated = await validateExistingDirectory(p); + if (!validated.ok) return `open-path: ${validated.reason}`; + if (!projectRootGate.isApproved(validated.resolved)) { + return "open-path: path is not a registered project root"; } try { - return await shell.openPath(p); + return await shell.openPath(validated.resolved); } catch (err) { return err instanceof Error ? err.message : String(err); } diff --git a/apps/packaged/tests/desktop-project-root-gate.test.ts b/apps/packaged/tests/desktop-project-root-gate.test.ts new file mode 100644 index 0000000000..b93d12b926 --- /dev/null +++ b/apps/packaged/tests/desktop-project-root-gate.test.ts @@ -0,0 +1,140 @@ +/** + * Coverage for the path-allowlist gate that the new shell.openPath + * IPC handler in `apps/desktop/src/main/runtime.ts` checks before + * forwarding any renderer-supplied string to Electron's + * `shell.openPath`. The packaged workspace hosts the test because + * `apps/desktop` itself has no vitest setup yet — same reasoning as + * the existing `desktop-url-allowlist.test.ts` next to this file. + * + * @see https://github.com/nexu-io/open-design/pull/974 (mrcfps's P1 + * review on runtime.ts:305 — shell:open-path must not accept + * arbitrary renderer paths). + */ +import { mkdtempSync, rmSync, symlinkSync, writeFileSync } from "node:fs"; +import { mkdir } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import path from "node:path"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; + +import { + createProjectRootGate, + validateExistingDirectory, +} from "@open-design/desktop/main"; + +let tempRoot = ""; + +beforeEach(() => { + tempRoot = mkdtempSync(path.join(tmpdir(), "od-desktop-gate-")); +}); + +afterEach(() => { + if (tempRoot) { + rmSync(tempRoot, { recursive: true, force: true }); + tempRoot = ""; + } +}); + +describe("validateExistingDirectory", () => { + it("rejects empty / non-string input", async () => { + const empty = await validateExistingDirectory(""); + expect(empty.ok).toBe(false); + if (!empty.ok) expect(empty.reason).toMatch(/non-empty string/i); + }); + + it("rejects relative paths", async () => { + const relative = await validateExistingDirectory("relative/site"); + expect(relative.ok).toBe(false); + if (!relative.ok) expect(relative.reason).toMatch(/absolute/i); + }); + + it("rejects non-existent absolute paths", async () => { + const ghost = path.join(tempRoot, "does-not-exist"); + const result = await validateExistingDirectory(ghost); + expect(result.ok).toBe(false); + if (!result.ok) expect(result.reason).toMatch(/exist/i); + }); + + it("rejects absolute paths that point at files rather than directories", async () => { + const file = path.join(tempRoot, "file.txt"); + writeFileSync(file, "not a directory"); + const result = await validateExistingDirectory(file); + expect(result.ok).toBe(false); + if (!result.ok) expect(result.reason).toMatch(/directory/i); + }); + + it("accepts an existing absolute directory and returns the realpath", async () => { + const result = await validateExistingDirectory(tempRoot); + expect(result.ok).toBe(true); + if (result.ok) expect(result.resolved).toBe(tempRoot); + }); + + it("realpath-resolves symlinks so attackers cannot register one path and reach another", async () => { + const realDir = path.join(tempRoot, "real"); + await mkdir(realDir); + const linkDir = path.join(tempRoot, "link"); + symlinkSync(realDir, linkDir, "dir"); + const result = await validateExistingDirectory(linkDir); + expect(result.ok).toBe(true); + if (result.ok) expect(result.resolved).toBe(realDir); + }); +}); + +describe("ProjectRootGate", () => { + it("starts empty", () => { + const gate = createProjectRootGate(); + expect(gate.size()).toBe(0); + }); + + it("rejects shell.openPath candidates that have not been registered", async () => { + const gate = createProjectRootGate(); + expect(gate.isApproved(tempRoot)).toBe(false); + }); + + it("accepts shell.openPath candidates that match a previously registered root", async () => { + const gate = createProjectRootGate(); + const registered = await gate.register(tempRoot); + expect(registered).toBe(true); + expect(gate.isApproved(tempRoot)).toBe(true); + expect(gate.size()).toBe(1); + }); + + it("rejects registration of a non-existent path so the allowlist cannot be poisoned", async () => { + const gate = createProjectRootGate(); + const ghost = path.join(tempRoot, "does-not-exist"); + const ok = await gate.register(ghost); + expect(ok).toBe(false); + expect(gate.isApproved(ghost)).toBe(false); + expect(gate.size()).toBe(0); + }); + + it("rejects registration of files (only directories are project roots)", async () => { + const gate = createProjectRootGate(); + const file = path.join(tempRoot, "not-a-dir.txt"); + writeFileSync(file, ""); + const ok = await gate.register(file); + expect(ok).toBe(false); + expect(gate.size()).toBe(0); + }); + + it("registers the realpath so a symlinked registration is matched against the real directory", async () => { + const gate = createProjectRootGate(); + const realDir = path.join(tempRoot, "real"); + await mkdir(realDir); + const linkDir = path.join(tempRoot, "link"); + symlinkSync(realDir, linkDir, "dir"); + + expect(await gate.register(linkDir)).toBe(true); + // The resolved path is the real dir, not the symlink. + expect(gate.isApproved(realDir)).toBe(true); + expect(gate.isApproved(linkDir)).toBe(false); + }); + + it("reset() empties the allowlist (test isolation hook)", async () => { + const gate = createProjectRootGate(); + await gate.register(tempRoot); + expect(gate.size()).toBe(1); + gate.reset(); + expect(gate.size()).toBe(0); + expect(gate.isApproved(tempRoot)).toBe(false); + }); +}); diff --git a/apps/web/src/components/ProjectView.tsx b/apps/web/src/components/ProjectView.tsx index 4d6f6c9795..85666905cc 100644 --- a/apps/web/src/components/ProjectView.tsx +++ b/apps/web/src/components/ProjectView.tsx @@ -1878,6 +1878,18 @@ export function ProjectView({ } }, [finalize.error]); + // Register the daemon-validated project working directory with the + // Electron bridge's path allowlist so the upcoming shell.openPath + // call (Continue in CLI) is gated to known project roots. mrcfps + // PR #974 P1 review on apps/desktop/src/main/runtime.ts:305: + // shell.openPath must not accept arbitrary renderer-supplied paths. + useEffect(() => { + if (!projectDetail.resolvedDir) return; + const register = window.electronAPI?.registerProjectRoot; + if (typeof register !== 'function') return; + void register(projectDetail.resolvedDir); + }, [projectDetail.resolvedDir]); + // ⌘+Shift+K (mac) / Ctrl+Shift+K (others) → Continue in CLI. Mirrors // the capture-phase, platform-gated pattern from FileWorkspace's // Quick Switcher shortcut. ⌘+Shift+K is free (⌘+P is the only diff --git a/apps/web/src/types/electron.d.ts b/apps/web/src/types/electron.d.ts index 28646ad984..dbd18cfe75 100644 --- a/apps/web/src/types/electron.d.ts +++ b/apps/web/src/types/electron.d.ts @@ -13,6 +13,14 @@ declare global { openExternal?: (url: string) => Promise; pickFolder?: () => Promise; openPath?: (path: string) => Promise; + // Registers a project working directory as eligible for openPath. + // The renderer should call this once per project mount with the + // daemon-validated resolvedDir so the main process's allowlist + // gates shell.openPath against approved roots only. Returns true + // when registration succeeded, false when the path failed + // validation in the main process (not absolute, doesn't exist, + // not a directory). + registerProjectRoot?: (path: string) => Promise; }; } }