From 01a67ec54366298c0187797f11566049f3613131 Mon Sep 17 00:00:00 2001 From: Dotta Date: Tue, 21 Apr 2026 14:41:02 -0500 Subject: [PATCH] Apply work product security patch and test service mocks - Restrict work product URL schema to http/https via shared validator - Defensively gate UI action hrefs through safeActionHref helper - Add issueDeliverableService mocks to route tests that stub the services registry so the new dependency resolves Co-Authored-By: Paperclip --- .../shared/src/validators/work-product.ts | 11 ++++++- .../agent-instructions-routes.test.ts | 2 +- .../__tests__/issue-attachment-routes.test.ts | 1 + .../issue-closed-workspace-routes.test.ts | 1 + .../issue-comment-cancel-routes.test.ts | 1 + .../issue-comment-reopen-routes.test.ts | 2 ++ .../issue-document-restore-routes.test.ts | 3 ++ .../__tests__/issue-feedback-routes.test.ts | 1 + .../__tests__/issue-telemetry-routes.test.ts | 1 + ...issue-update-comment-wakeup-routes.test.ts | 3 ++ .../issue-workspace-command-authz.test.ts | 1 + .../__tests__/work-product-validator.test.ts | 31 +++++++++++++++++++ ui/src/components/IssueWorkProductTab.tsx | 10 +++--- ui/src/lib/safeLinks.test.ts | 18 +++++++++++ ui/src/lib/safeLinks.ts | 23 ++++++++++++++ ui/src/pages/Artifacts.tsx | 5 +-- 16 files changed, 106 insertions(+), 8 deletions(-) create mode 100644 server/src/__tests__/work-product-validator.test.ts create mode 100644 ui/src/lib/safeLinks.test.ts create mode 100644 ui/src/lib/safeLinks.ts diff --git a/packages/shared/src/validators/work-product.ts b/packages/shared/src/validators/work-product.ts index 839cc15a540..e138262ee6d 100644 --- a/packages/shared/src/validators/work-product.ts +++ b/packages/shared/src/validators/work-product.ts @@ -1,5 +1,14 @@ import { z } from "zod"; +function isHttpUrl(value: string) { + try { + const parsed = new URL(value); + return parsed.protocol === "http:" || parsed.protocol === "https:"; + } catch { + return false; + } +} + export const issueWorkProductTypeSchema = z.enum([ "preview_url", "runtime_service", @@ -37,7 +46,7 @@ export const createIssueWorkProductSchema = z.object({ provider: z.string().min(1), externalId: z.string().optional().nullable(), title: z.string().min(1), - url: z.string().url().optional().nullable(), + url: z.string().trim().refine(isHttpUrl, "URL must use http or https").optional().nullable(), status: issueWorkProductStatusSchema.default("active"), reviewState: issueWorkProductReviewStateSchema.optional().default("none"), isPrimary: z.boolean().optional().default(false), diff --git a/server/src/__tests__/agent-instructions-routes.test.ts b/server/src/__tests__/agent-instructions-routes.test.ts index dbac3bd03b2..2b219541635 100644 --- a/server/src/__tests__/agent-instructions-routes.test.ts +++ b/server/src/__tests__/agent-instructions-routes.test.ts @@ -200,7 +200,7 @@ describe("agent instructions bundle routes", () => { entryFile: "AGENTS.md", }); expect(mockAgentInstructionsService.getBundle).toHaveBeenCalled(); - }); + }, 10_000); it("writes a bundle file and persists compatibility config", async () => { const res = await request(await createApp()) diff --git a/server/src/__tests__/issue-attachment-routes.test.ts b/server/src/__tests__/issue-attachment-routes.test.ts index 51cba13f556..ed4b06bbc01 100644 --- a/server/src/__tests__/issue-attachment-routes.test.ts +++ b/server/src/__tests__/issue-attachment-routes.test.ts @@ -62,6 +62,7 @@ function registerRouteMocks() { routineService: () => ({ syncRunStatusForIssue: vi.fn(async () => undefined), }), + issueDeliverableService: () => ({}), workProductService: () => ({}), })); } diff --git a/server/src/__tests__/issue-closed-workspace-routes.test.ts b/server/src/__tests__/issue-closed-workspace-routes.test.ts index e9a876023fa..53e561066a9 100644 --- a/server/src/__tests__/issue-closed-workspace-routes.test.ts +++ b/server/src/__tests__/issue-closed-workspace-routes.test.ts @@ -80,6 +80,7 @@ function registerServiceMocks() { routineService: () => ({ syncRunStatusForIssue: vi.fn(async () => undefined), }), + issueDeliverableService: () => ({}), workProductService: () => ({}), })); } diff --git a/server/src/__tests__/issue-comment-cancel-routes.test.ts b/server/src/__tests__/issue-comment-cancel-routes.test.ts index ca070233511..963fba87269 100644 --- a/server/src/__tests__/issue-comment-cancel-routes.test.ts +++ b/server/src/__tests__/issue-comment-cancel-routes.test.ts @@ -56,6 +56,7 @@ vi.mock("../services/index.js", () => ({ logActivity: mockLogActivity, projectService: () => ({}), routineService: () => ({ syncRunStatusForIssue: vi.fn(async () => undefined) }), + issueDeliverableService: () => ({}), workProductService: () => ({}), })); diff --git a/server/src/__tests__/issue-comment-reopen-routes.test.ts b/server/src/__tests__/issue-comment-reopen-routes.test.ts index 2ef33886348..79b917b0b7d 100644 --- a/server/src/__tests__/issue-comment-reopen-routes.test.ts +++ b/server/src/__tests__/issue-comment-reopen-routes.test.ts @@ -80,6 +80,7 @@ vi.mock("../services/index.js", () => ({ logActivity: mockLogActivity, projectService: () => ({}), routineService: () => mockRoutineService, + issueDeliverableService: () => ({}), workProductService: () => ({}), })); @@ -107,6 +108,7 @@ function registerModuleMocks() { logActivity: mockLogActivity, projectService: () => ({}), routineService: () => mockRoutineService, + issueDeliverableService: () => ({}), workProductService: () => ({}), })); } diff --git a/server/src/__tests__/issue-document-restore-routes.test.ts b/server/src/__tests__/issue-document-restore-routes.test.ts index b127587da37..7f4ae2ac9e3 100644 --- a/server/src/__tests__/issue-document-restore-routes.test.ts +++ b/server/src/__tests__/issue-document-restore-routes.test.ts @@ -71,6 +71,9 @@ function registerModuleMocks() { getGeneral: vi.fn(async () => ({ feedbackDataSharingPreference: "prompt" })), }), issueApprovalService: () => ({}), + issueDeliverableService: () => ({ + getForIssue: vi.fn(async () => null), + }), issueService: () => mockIssueService, logActivity: mockLogActivity, projectService: () => ({}), diff --git a/server/src/__tests__/issue-feedback-routes.test.ts b/server/src/__tests__/issue-feedback-routes.test.ts index 6a2df50d94d..69d645ccd3d 100644 --- a/server/src/__tests__/issue-feedback-routes.test.ts +++ b/server/src/__tests__/issue-feedback-routes.test.ts @@ -74,6 +74,7 @@ function registerModuleMocks() { logActivity: mockLogActivity, projectService: () => ({}), routineService: () => mockRoutineService, + issueDeliverableService: () => ({}), workProductService: () => ({}), })); } diff --git a/server/src/__tests__/issue-telemetry-routes.test.ts b/server/src/__tests__/issue-telemetry-routes.test.ts index bd2e1f31485..933e844e736 100644 --- a/server/src/__tests__/issue-telemetry-routes.test.ts +++ b/server/src/__tests__/issue-telemetry-routes.test.ts @@ -48,6 +48,7 @@ function registerModuleMocks() { routineService: () => ({ syncRunStatusForIssue: vi.fn(async () => undefined), }), + issueDeliverableService: () => ({}), workProductService: () => ({}), })); } diff --git a/server/src/__tests__/issue-update-comment-wakeup-routes.test.ts b/server/src/__tests__/issue-update-comment-wakeup-routes.test.ts index b147c176114..8dc39a954b1 100644 --- a/server/src/__tests__/issue-update-comment-wakeup-routes.test.ts +++ b/server/src/__tests__/issue-update-comment-wakeup-routes.test.ts @@ -97,6 +97,9 @@ function registerModuleMocks() { listCompanyIds: vi.fn(async () => ["company-1"]), }), issueApprovalService: () => ({}), + issueDeliverableService: () => ({ + getForIssue: vi.fn(async () => null), + }), issueService: () => mockIssueService, logActivity: vi.fn(async () => undefined), projectService: () => ({}), diff --git a/server/src/__tests__/issue-workspace-command-authz.test.ts b/server/src/__tests__/issue-workspace-command-authz.test.ts index cf9a7847b01..8539f67b3b9 100644 --- a/server/src/__tests__/issue-workspace-command-authz.test.ts +++ b/server/src/__tests__/issue-workspace-command-authz.test.ts @@ -58,6 +58,7 @@ vi.mock("../services/index.js", () => ({ routineService: () => ({ syncRunStatusForIssue: vi.fn(async () => undefined), }), + issueDeliverableService: () => ({}), workProductService: () => ({}), })); diff --git a/server/src/__tests__/work-product-validator.test.ts b/server/src/__tests__/work-product-validator.test.ts new file mode 100644 index 00000000000..cd153e63ec0 --- /dev/null +++ b/server/src/__tests__/work-product-validator.test.ts @@ -0,0 +1,31 @@ +import { describe, expect, it } from "vitest"; +import { createIssueWorkProductSchema } from "@paperclipai/shared"; + +const baseWorkProduct = { + type: "pull_request", + provider: "github", + title: "PR 42", +}; + +describe("work product URL validation", () => { + it("accepts http and https URLs", () => { + expect(createIssueWorkProductSchema.safeParse({ + ...baseWorkProduct, + url: "https://github.com/paperclipai/paperclip/pull/42", + }).success).toBe(true); + expect(createIssueWorkProductSchema.safeParse({ + ...baseWorkProduct, + url: "http://127.0.0.1:3100/preview", + }).success).toBe(true); + }); + + it("rejects executable and local-file URL schemes", () => { + for (const url of ["javascript:alert(document.domain)", "file:///Users/operator/.ssh/id_rsa", "data:text/html,hello"]) { + const result = createIssueWorkProductSchema.safeParse({ + ...baseWorkProduct, + url, + }); + expect(result.success).toBe(false); + } + }); +}); diff --git a/ui/src/components/IssueWorkProductTab.tsx b/ui/src/components/IssueWorkProductTab.tsx index 7a428860438..9b0e6a8dd03 100644 --- a/ui/src/components/IssueWorkProductTab.tsx +++ b/ui/src/components/IssueWorkProductTab.tsx @@ -1,6 +1,7 @@ import type { IssueDeliverableItem, IssueDeliverablesResponse } from "@paperclipai/shared"; import { Link } from "@/lib/router"; import { projectWorkspaceUrl, relativeTime } from "@/lib/utils"; +import { safeActionHref } from "@/lib/safeLinks"; import { EmptyState } from "./EmptyState"; import { PageSkeleton } from "./PageSkeleton"; import { Badge } from "@/components/ui/badge"; @@ -48,17 +49,18 @@ function itemIcon(item: IssueDeliverableItem) { } function DeliverableAction({ href, label }: { href: string | null; label: string | null }) { - if (!href || !label) return null; - if (href.startsWith("#")) { + const safeHref = safeActionHref(href, { allowHash: true }); + if (!safeHref || !label) return null; + if (safeHref.startsWith("#")) { return ( ); } return ( diff --git a/ui/src/lib/safeLinks.test.ts b/ui/src/lib/safeLinks.test.ts new file mode 100644 index 00000000000..fbfc5cbad30 --- /dev/null +++ b/ui/src/lib/safeLinks.test.ts @@ -0,0 +1,18 @@ +import { describe, expect, it } from "vitest"; +import { safeActionHref } from "./safeLinks"; + +describe("safeActionHref", () => { + it("allows http, https, app-relative paths, and explicit hash links", () => { + expect(safeActionHref("https://example.com/pr/42")).toBe("https://example.com/pr/42"); + expect(safeActionHref("http://127.0.0.1:3100/preview")).toBe("http://127.0.0.1:3100/preview"); + expect(safeActionHref("/api/attachments/attachment-1/content")).toBe("/api/attachments/attachment-1/content"); + expect(safeActionHref("#document-plan", { allowHash: true })).toBe("#document-plan"); + }); + + it("rejects executable, protocol-relative, local-file, and hash links by default", () => { + expect(safeActionHref("javascript:alert(document.domain)")).toBeNull(); + expect(safeActionHref("//evil.example/path")).toBeNull(); + expect(safeActionHref("file:///Users/operator/.ssh/id_rsa")).toBeNull(); + expect(safeActionHref("#document-plan")).toBeNull(); + }); +}); diff --git a/ui/src/lib/safeLinks.ts b/ui/src/lib/safeLinks.ts new file mode 100644 index 00000000000..5f312ec1bda --- /dev/null +++ b/ui/src/lib/safeLinks.ts @@ -0,0 +1,23 @@ +export function safeActionHref(href: string | null | undefined, options: { allowHash?: boolean } = {}) { + const trimmed = href?.trim(); + if (!trimmed) return null; + + if (options.allowHash && trimmed.startsWith("#")) { + return trimmed; + } + + if (trimmed.startsWith("/") && !trimmed.startsWith("//")) { + return trimmed; + } + + try { + const parsed = new URL(trimmed); + if (parsed.protocol === "http:" || parsed.protocol === "https:") { + return trimmed; + } + } catch { + return null; + } + + return null; +} diff --git a/ui/src/pages/Artifacts.tsx b/ui/src/pages/Artifacts.tsx index 2fccdf26c2e..6e71259d1e5 100644 --- a/ui/src/pages/Artifacts.tsx +++ b/ui/src/pages/Artifacts.tsx @@ -13,6 +13,7 @@ import { Button } from "@/components/ui/button"; import { Input } from "@/components/ui/input"; import { ToggleSwitch } from "@/components/ui/toggle-switch"; import { queryKeys } from "../lib/queryKeys"; +import { safeActionHref } from "../lib/safeLinks"; import { createIssueDetailPath } from "../lib/issueDetailBreadcrumb"; import { projectUrl, relativeTime } from "../lib/utils"; import { @@ -157,7 +158,7 @@ function ArtifactCard({ projectHref: string | null; }) { const meta = deliverableMeta(item); - const href = itemHref(item); + const href = safeActionHref(itemHref(item)); const sizeLabel = formatByteSize(item.byteSize); const MetaIcon = meta.icon; const isInternalDocLink = Boolean(href?.startsWith("/issues/")); @@ -263,7 +264,7 @@ function ArtifactCard({ ) : (