From 6c757f208161dbc3f92a79115ddae2c414f19f10 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=9B=B7=E7=94=B5=E8=8A=BD=E8=A1=A3?= Date: Thu, 23 Apr 2026 20:54:39 -0400 Subject: [PATCH] fix: enforce work product external url policy --- src/main/api-server/routes-files.ts | 16 +- src/main/desktop-shell-window.ts | 33 +-- src/main/external-url-policy.ts | 54 +++++ src/main/ipc-core-read-handlers.ts | 28 ++- .../work-product-url-policy.spec.ts | 211 ++++++++++++++++++ tests/unit/external-url-policy.test.ts | 22 ++ .../ipc-open-work-product-url-policy.test.ts | 122 ++++++++++ 7 files changed, 456 insertions(+), 30 deletions(-) create mode 100644 src/main/external-url-policy.ts create mode 100644 tests/integration/work-product-url-policy.spec.ts create mode 100644 tests/unit/external-url-policy.test.ts create mode 100644 tests/unit/ipc-open-work-product-url-policy.test.ts diff --git a/src/main/api-server/routes-files.ts b/src/main/api-server/routes-files.ts index eb3e5738..6efe2e12 100644 --- a/src/main/api-server/routes-files.ts +++ b/src/main/api-server/routes-files.ts @@ -1,6 +1,18 @@ import log from "electron-log/main.js"; import { z } from "zod"; import { requireParam, type AgentApiServer } from "./server-core"; +import { normalizeExternalHttpUrl } from "../external-url-policy"; + +const workProductUrlSchema = z.string().trim().superRefine((value, ctx) => { + try { + normalizeExternalHttpUrl(value); + } catch (error) { + ctx.addIssue({ + code: "custom", + message: error instanceof Error ? error.message : String(error), + }); + } +}).transform((value) => normalizeExternalHttpUrl(value)); const createDocumentSchema = z.object({ type: z.string().optional(), @@ -30,7 +42,7 @@ const createWorkProductSchema = z.object({ title: z.string(), projectId: z.string().optional(), externalId: z.string().optional(), - url: z.string().optional(), + url: workProductUrlSchema.optional(), status: z.string().optional(), reviewState: z.string().optional(), isPrimary: z.boolean().optional(), @@ -43,7 +55,7 @@ const updateWorkProductSchema = z.object({ provider: z.string().optional(), externalId: z.string().optional(), title: z.string().optional(), - url: z.string().optional(), + url: workProductUrlSchema.optional(), status: z.string().optional(), reviewState: z.string().optional(), isPrimary: z.boolean().optional(), diff --git a/src/main/desktop-shell-window.ts b/src/main/desktop-shell-window.ts index 01e17c89..666720be 100644 --- a/src/main/desktop-shell-window.ts +++ b/src/main/desktop-shell-window.ts @@ -16,6 +16,7 @@ import { import log from "electron-log/main.js"; import { attachWindowStatePersistence, loadWindowState } from "./window-state"; import type { NotificationOptions } from "./notification-service"; +import { classifyExternalUrl } from "./external-url-policy"; export interface DesktopShellWindowDependencies { preloadPath: string; @@ -43,27 +44,6 @@ function isWithinDirectory(rootPath: string, targetPath: string) { return normalizedTarget === normalizedRoot || normalizedTarget.startsWith(`${normalizedRoot}${sep}`); } -const TRUSTED_EXTERNAL_HOSTS = new Set([ - "github.com", - "www.github.com", - "agentcompany.pages.dev", - "claude.ai", - "www.electronjs.org", - "www.electron.build", -]); - -function shouldOpenTrustedExternalUrl(rawUrl: string) { - try { - const url = new URL(rawUrl); - if (url.protocol !== "https:" && url.protocol !== "http:") { - return false; - } - return TRUSTED_EXTERNAL_HOSTS.has(url.hostname.toLowerCase()); - } catch { - return false; - } -} - export function createDesktopShellWindow(deps: DesktopShellWindowDependencies) { const { preloadPath, @@ -172,11 +152,12 @@ export function createDesktopShellWindow(deps: DesktopShellWindowDependencies) { }); app.on("web-contents-created", (_event, contents) => { contents.setWindowOpenHandler(({ url }) => { - if (shouldOpenTrustedExternalUrl(url)) { - void shell.openExternal(url); + const decision = classifyExternalUrl(url); + if (decision.kind === "trusted") { + void shell.openExternal(decision.url); return { action: "deny" }; } - if (url.startsWith("https://") || url.startsWith("http://")) { + if (decision.kind === "confirm") { // Dialog is dispatched asynchronously so the main-process event loop // is not blocked while the user decides. setWindowOpenHandler must // return synchronously, so the window is always denied here; if the @@ -190,13 +171,13 @@ export function createDesktopShellWindow(deps: DesktopShellWindowDependencies) { cancelId: 1, title: "Open External Link?", message: "AgentCompany blocked an untrusted external link.", - detail: `${url}\n\nOnly trusted product and documentation domains open automatically. Review this destination before continuing.`, + detail: `${decision.url}\n\nOnly trusted product and documentation domains open automatically. Review this destination before continuing.`, }; const result = parentWindow ? await dialog.showMessageBox(parentWindow, options) : await dialog.showMessageBox(options); if (result.response === 0) { - void shell.openExternal(url); + void shell.openExternal(decision.url); } })(); } diff --git a/src/main/external-url-policy.ts b/src/main/external-url-policy.ts new file mode 100644 index 00000000..18c3dc41 --- /dev/null +++ b/src/main/external-url-policy.ts @@ -0,0 +1,54 @@ +const TRUSTED_EXTERNAL_HOSTS = new Set([ + "github.com", + "www.github.com", + "agentcompany.pages.dev", + "claude.ai", + "www.electronjs.org", + "www.electron.build", +]); + +export type ExternalUrlDecision = + | { kind: "trusted"; url: string; host: string } + | { kind: "confirm"; url: string; host: string } + | { kind: "blocked"; reason: string }; + +export function normalizeExternalHttpUrl(rawUrl: string): string { + const trimmed = rawUrl.trim(); + if (!trimmed) { + throw new Error("External URL is required."); + } + let parsed: URL; + try { + parsed = new URL(trimmed); + } catch { + throw new Error("External URL must be absolute."); + } + if (parsed.protocol !== "https:" && parsed.protocol !== "http:") { + throw new Error("External URL protocol must be http or https."); + } + if (!parsed.hostname) { + throw new Error("External URL host is required."); + } + if (parsed.username || parsed.password) { + throw new Error("External URL credentials are not allowed."); + } + return parsed.toString(); +} + +export function classifyExternalUrl(rawUrl: string): ExternalUrlDecision { + let normalized: string; + try { + normalized = normalizeExternalHttpUrl(rawUrl); + } catch (error) { + return { + kind: "blocked", + reason: error instanceof Error ? error.message : String(error), + }; + } + const parsed = new URL(normalized); + const host = parsed.hostname.toLowerCase(); + if (TRUSTED_EXTERNAL_HOSTS.has(host)) { + return { kind: "trusted", url: normalized, host }; + } + return { kind: "confirm", url: normalized, host }; +} diff --git a/src/main/ipc-core-read-handlers.ts b/src/main/ipc-core-read-handlers.ts index 1cf583f5..adbfdd81 100644 --- a/src/main/ipc-core-read-handlers.ts +++ b/src/main/ipc-core-read-handlers.ts @@ -11,6 +11,31 @@ import { } from "@shared/contracts"; import { fail, ok, type RegisterCoreHandlersDependencies } from "./ipc-core-common"; import { logAppError } from "./errors/index.js"; +import { classifyExternalUrl } from "./external-url-policy"; + +async function openExternalUrlWithTrustPolicy(rawUrl: string) { + const decision = classifyExternalUrl(rawUrl); + if (decision.kind === "blocked") { + return fail("OPEN_EXTERNAL_URL_FORBIDDEN", decision.reason); + } + if (decision.kind === "confirm") { + const result = await dialog.showMessageBox({ + type: "question", + buttons: ["Open Link", "Cancel"], + defaultId: 1, + cancelId: 1, + title: "Open External Link?", + message: "AgentCompany blocked an untrusted external link.", + detail: `${decision.url}\n\nOnly trusted product and documentation domains open automatically. Review this destination before continuing.`, + }); + if (result.response !== 0) { + return ok(false); + } + } + const url = decision.url; + const error = await shell.openExternal(url).then(() => "").catch((err: unknown) => err instanceof Error ? err.message : String(err)); + return error ? fail("OPEN_EXTERNAL_URL_FAILED", error) : ok(true); +} export function registerCoreReadHandlers({ registerHandle, @@ -85,8 +110,7 @@ export function registerCoreReadHandlers({ return fail(codeByReason[target.reason], target.detail); } if (target.kind === "url") { - const error = await shell.openExternal(target.value).then(() => "").catch((err: unknown) => err instanceof Error ? err.message : String(err)); - return error ? fail("OPEN_PATH_FAILED", error) : ok(true); + return openExternalUrlWithTrustPolicy(target.value); } if (!db.core.settings.canOpenPath(target.value)) { return fail("OPEN_PATH_FORBIDDEN", "The requested path is outside the managed desktop workspace."); diff --git a/tests/integration/work-product-url-policy.spec.ts b/tests/integration/work-product-url-policy.spec.ts new file mode 100644 index 00000000..282bf130 --- /dev/null +++ b/tests/integration/work-product-url-policy.spec.ts @@ -0,0 +1,211 @@ +import { mkdtemp, rm } from "node:fs/promises"; +import { join } from "node:path"; +import { tmpdir } from "node:os"; +import { afterEach, describe, expect, it } from "vitest"; +import { createAgentToken, initializeAgentTokenSigningKey } from "../../src/main/agent-auth"; +import { createApiServer } from "../../src/main/api-server"; +import { AppDatabase } from "../../src/main/database"; +import { createTestApiServerCallbacks } from "../support/api-server-callbacks"; + +const tempDirs: string[] = []; + +async function createTempDir(prefix: string) { + const dir = await mkdtemp(join(tmpdir(), prefix)); + tempDirs.push(dir); + return dir; +} + +afterEach(async () => { + await Promise.all(tempDirs.splice(0).map((dir) => rm(dir, { recursive: true, force: true }))); +}); + +async function createHarness() { + const profileDir = await createTempDir("agentcompany-work-product-url-"); + initializeAgentTokenSigningKey(profileDir); + const db = new AppDatabase(profileDir); + db.init(); + db.core.connectors.updateConnectorHealth({ + id: "openai-codex", + status: "ready", + authState: "authenticated", + version: "test", + lastError: null, + }); + + const companyId = db.core.company.saveCompany({ + name: "URL Policy Co", + description: "Tests work product URL trust policy", + status: "active", + }); + const workspaceId = db.core.workspaces.saveWorkspace({ + companyId, + name: "Primary", + localPath: profileDir, + repoUrl: "", + repoRef: "", + isPrimary: true, + }); + const agentId = db.core.agents.saveAgent({ + companyId, + name: "Reporter", + role: "Researcher", + title: "Reporter", + status: "active", + reportsTo: null, + connectorId: "openai-codex", + workspaceId, + model: null, + capabilities: "Writes reports", + budgetMonthlyUsd: 100, + }); + const taskId = db.core.tasks.saveTask({ + companyId, + title: "Publish external report", + description: "Attach a report link.", + assigneeAgentId: agentId, + workspaceId, + priority: "medium", + status: "in_progress", + }); + const run = db.core.runs.createRun({ + companyId, + taskId, + agentId, + workspaceId, + connectorId: "openai-codex", + }); + const token = createAgentToken(agentId, companyId, run.id); + const server = createApiServer(db, createTestApiServerCallbacks()); + const port = await server.start(); + + return { + db, + server, + companyId, + taskId, + baseUrl: `http://127.0.0.1:${port}`, + headers: { + Authorization: `Bearer ${token}`, + "X-Agent-Company-Run-Id": run.id, + "Content-Type": "application/json; charset=utf-8", + }, + }; +} + +describe("work product URL API policy", () => { + it("rejects blank URLs during work product creation", async () => { + const harness = await createHarness(); + try { + const response = await fetch(`${harness.baseUrl}/api/issues/${harness.taskId}/work-products`, { + method: "POST", + headers: harness.headers, + body: JSON.stringify({ + type: "link", + provider: "external", + title: "Blank URL", + url: " ", + }), + }); + + expect(response.status).toBe(400); + expect(harness.db.queryAll("select id from work_products")).toHaveLength(0); + } finally { + harness.server.stop(); + harness.db.close(); + } + }); + + it("rejects custom schemes during work product creation", async () => { + const harness = await createHarness(); + try { + const response = await fetch(`${harness.baseUrl}/api/issues/${harness.taskId}/work-products`, { + method: "POST", + headers: harness.headers, + body: JSON.stringify({ + type: "link", + provider: "external", + title: "Local scheme", + url: "vscode://file/etc/passwd", + }), + }); + + expect(response.status).toBe(400); + expect(harness.db.queryAll("select id from work_products")).toHaveLength(0); + } finally { + harness.server.stop(); + harness.db.close(); + } + }); + + it("rejects credential-bearing URLs during work product creation", async () => { + const harness = await createHarness(); + try { + const response = await fetch(`${harness.baseUrl}/api/issues/${harness.taskId}/work-products`, { + method: "POST", + headers: harness.headers, + body: JSON.stringify({ + type: "link", + provider: "external", + title: "Credential URL", + url: "https://user:pass@example.com/reports/q1", + }), + }); + + expect(response.status).toBe(400); + expect(harness.db.queryAll("select id from work_products")).toHaveLength(0); + } finally { + harness.server.stop(); + harness.db.close(); + } + }); + + it("stores http links for later confirmation during open", async () => { + const harness = await createHarness(); + try { + const response = await fetch(`${harness.baseUrl}/api/issues/${harness.taskId}/work-products`, { + method: "POST", + headers: harness.headers, + body: JSON.stringify({ + type: "link", + provider: "external", + title: "External report", + url: " https://example.com/reports/q1 ", + }), + }); + + expect(response.status).toBe(201); + const row = harness.db.queryAll<{ url: string }>("select url from work_products limit 1")[0]; + expect(row?.url).toBe("https://example.com/reports/q1"); + } finally { + harness.server.stop(); + harness.db.close(); + } + }); + + it("rejects custom schemes during work product updates", async () => { + const harness = await createHarness(); + try { + const id = harness.db.core.documents.createWorkProduct({ + companyId: harness.companyId, + taskId: harness.taskId, + type: "link", + provider: "external", + title: "External report", + url: "https://example.com/reports/q1", + }); + + const response = await fetch(`${harness.baseUrl}/api/work-products/${id}`, { + method: "PATCH", + headers: harness.headers, + body: JSON.stringify({ url: "file:///etc/passwd" }), + }); + + expect(response.status).toBe(400); + const row = harness.db.queryAll<{ url: string }>("select url from work_products where id = ? limit 1", id)[0]; + expect(row?.url).toBe("https://example.com/reports/q1"); + } finally { + harness.server.stop(); + harness.db.close(); + } + }); +}); diff --git a/tests/unit/external-url-policy.test.ts b/tests/unit/external-url-policy.test.ts new file mode 100644 index 00000000..6e4db9d8 --- /dev/null +++ b/tests/unit/external-url-policy.test.ts @@ -0,0 +1,22 @@ +import { describe, expect, it } from "vitest"; +import { + classifyExternalUrl, + normalizeExternalHttpUrl, +} from "../../src/main/external-url-policy"; + +describe("external URL policy", () => { + it("normalizes http and https URLs", () => { + expect(normalizeExternalHttpUrl(" https://github.com/agentcompany ")).toBe("https://github.com/agentcompany"); + expect(normalizeExternalHttpUrl("http://example.com/path?q=1")).toBe("http://example.com/path?q=1"); + }); + + it("blocks custom schemes and credential-bearing URLs", () => { + expect(classifyExternalUrl("vscode://file/etc/passwd")).toMatchObject({ kind: "blocked" }); + expect(classifyExternalUrl("https://user:pass@example.com/report")).toMatchObject({ kind: "blocked" }); + }); + + it("separates trusted hosts from confirmation hosts", () => { + expect(classifyExternalUrl("https://github.com/acme/repo")).toMatchObject({ kind: "trusted", host: "github.com" }); + expect(classifyExternalUrl("https://example.com/report")).toMatchObject({ kind: "confirm", host: "example.com" }); + }); +}); diff --git a/tests/unit/ipc-open-work-product-url-policy.test.ts b/tests/unit/ipc-open-work-product-url-policy.test.ts new file mode 100644 index 00000000..974ac112 --- /dev/null +++ b/tests/unit/ipc-open-work-product-url-policy.test.ts @@ -0,0 +1,122 @@ +import { describe, expect, it, vi } from "vitest"; +import { IPC_CHANNELS } from "../../src/shared/contracts"; + +const electron = vi.hoisted(() => ({ + openExternal: vi.fn<() => Promise>(async () => {}), + openPath: vi.fn<() => Promise>(async () => ""), + showMessageBox: vi.fn<() => Promise<{ response: number }>>(async () => ({ response: 0 })), +})); + +vi.mock("electron", () => ({ + dialog: { showMessageBox: electron.showMessageBox }, + nativeTheme: { themeSource: "system" }, + shell: { + openExternal: electron.openExternal, + openPath: electron.openPath, + }, +})); + +import { registerCoreReadHandlers } from "../../src/main/ipc-core-read-handlers"; +import type { AppDatabase } from "../../src/main/database"; +import type { RegisterCoreHandlersDependencies } from "../../src/main/ipc-core-common"; + +const companyId = "11111111-1111-4111-8111-111111111111"; +const workProductId = "22222222-2222-4222-8222-222222222222"; + +function registerOpenWorkProductHandler(url: string) { + const handlers = new Map Promise | unknown>(); + const db = { + core: { + documents: { + resolveWorkProductTarget: vi.fn(() => ({ kind: "url", value: url })), + updateWorkProduct: vi.fn(), + }, + settings: { + canOpenPath: vi.fn(() => true), + }, + }, + projection: { + snapshot: { + listRendererSnapshot: vi.fn(() => ({})), + }, + }, + } as unknown as AppDatabase; + const deps = { + registerHandle: (channel, handler) => handlers.set(channel, handler as (...args: unknown[]) => Promise | unknown), + db, + ensureCompanyExists: () => null, + publishDomainChanged: vi.fn(), + checkForUpdates: vi.fn(async () => true), + resetAfterRestore: vi.fn(), + listConnectorModels: vi.fn(async () => []), + logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn() }, + } satisfies Pick< + RegisterCoreHandlersDependencies, + | "registerHandle" + | "db" + | "ensureCompanyExists" + | "publishDomainChanged" + | "checkForUpdates" + | "resetAfterRestore" + | "listConnectorModels" + | "logger" + >; + registerCoreReadHandlers(deps); + const handler = handlers.get(IPC_CHANNELS.openWorkProduct); + if (!handler) { + throw new Error("openWorkProduct handler was not registered."); + } + return handler; +} + +async function openUrl(url: string) { + const handler = registerOpenWorkProductHandler(url); + return handler({}, { companyId, id: workProductId }); +} + +describe("openWorkProduct external URL policy", () => { + it("opens trusted hosts without confirmation", async () => { + electron.openExternal.mockClear(); + electron.showMessageBox.mockClear(); + + const result = await openUrl("https://github.com/acme/repo"); + + expect(result).toEqual({ ok: true, data: true }); + expect(electron.showMessageBox).not.toHaveBeenCalled(); + expect(electron.openExternal).toHaveBeenCalledWith("https://github.com/acme/repo"); + }); + + it("asks before opening untrusted http hosts", async () => { + electron.openExternal.mockClear(); + electron.showMessageBox.mockClear(); + electron.showMessageBox.mockResolvedValueOnce({ response: 0 }); + + const result = await openUrl("https://example.com/report"); + + expect(result).toEqual({ ok: true, data: true }); + expect(electron.showMessageBox).toHaveBeenCalledTimes(1); + expect(electron.openExternal).toHaveBeenCalledWith("https://example.com/report"); + }); + + it("blocks custom schemes before shell handoff", async () => { + electron.openExternal.mockClear(); + electron.showMessageBox.mockClear(); + + const result = await openUrl("vscode://file/etc/passwd"); + + expect(result).toMatchObject({ ok: false, error: { code: "OPEN_EXTERNAL_URL_FORBIDDEN" } }); + expect(electron.showMessageBox).not.toHaveBeenCalled(); + expect(electron.openExternal).not.toHaveBeenCalled(); + }); + + it("blocks credential-bearing URLs before shell handoff", async () => { + electron.openExternal.mockClear(); + electron.showMessageBox.mockClear(); + + const result = await openUrl("https://user:pass@example.com/report"); + + expect(result).toMatchObject({ ok: false, error: { code: "OPEN_EXTERNAL_URL_FORBIDDEN" } }); + expect(electron.showMessageBox).not.toHaveBeenCalled(); + expect(electron.openExternal).not.toHaveBeenCalled(); + }); +});