fix(desktop): allowlist-validate shell.openPath against registered project roots

Addresses mrcfps's PR #974 P1 review on runtime.ts:305 (also called
out as P1.2 in lefarcen's deep-dive review): the new
`shell:open-path` IPC handler accepted any renderer-supplied
string and forwarded it straight into Electron's `shell.openPath`,
widening the renderer→main trust boundary so XSS or a compromised
renderer dependency could open arbitrary local paths to the user.

Adds an explicit gate around the bridge:

  1. validateExistingDirectory(p) — floor check that rejects empty
     strings, relative paths, files, apps, and non-existent paths;
     realpath-resolves so symlink games can't be used to register
     one path and reach another.

  2. createProjectRootGate() — Set-backed allowlist of
     daemon-validated project working directories. The renderer
     calls registerProjectRoot(absDir) once per project mount via
     a new IPC method (preload bridge); the main process only
     opens paths that pass both the floor check and the allowlist.

ProjectView wires the registration via a useEffect tied to
projectDetail.resolvedDir, so the active project's daemon-supplied
working directory is always the one being approved (not a renderer-
synthesized string).

Threat-model caveat documented in the runtime.ts comment block: an
attacker that fully controls the renderer can also call register
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 to keep this PR focused.
Today's allowlist still defends against accidental misuse, bugs,
and common XSS payloads that don't know to call register first.

Adds apps/packaged/tests/desktop-project-root-gate.test.ts with 13
cases: floor-validation rejection cases (empty / relative / missing
/ file), happy-path resolution, symlink realpath canonicalization,
and the allowlist's register/isApproved/reset semantics. Mirrors
the existing apps/packaged/tests/desktop-url-allowlist.test.ts
pattern from PR #911 — the packaged workspace hosts the test
because apps/desktop has no vitest setup yet.
This commit is contained in:
DevForgeAI CI/CD Engineer
2026-05-08 14:22:01 -04:00
parent 1f2309f0f5
commit 8bf5659734
6 changed files with 291 additions and 4 deletions

View File

@@ -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 = {

View File

@@ -7,4 +7,6 @@ contextBridge.exposeInMainWorld('electronAPI', {
ipcRenderer.invoke('dialog:pick-folder'),
openPath: (path: string): Promise<string> =>
ipcRenderer.invoke('shell:open-path', path),
registerProjectRoot: (path: string): Promise<boolean> =>
ipcRenderer.invoke('shell:register-project-root', path),
});

View File

@@ -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<PathValidationResult> {
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<boolean>;
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<string>();
return {
async register(p: string): Promise<boolean> {
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);
}

View File

@@ -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);
});
});

View File

@@ -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

View File

@@ -13,6 +13,14 @@ declare global {
openExternal?: (url: string) => Promise<boolean>;
pickFolder?: () => Promise<string | null>;
openPath?: (path: string) => Promise<string>;
// 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<boolean>;
};
}
}