diff --git a/packages/shared/src/issue-thread-interactions.test.ts b/packages/shared/src/issue-thread-interactions.test.ts index 241b7e5bd2f..630d0c9da95 100644 --- a/packages/shared/src/issue-thread-interactions.test.ts +++ b/packages/shared/src/issue-thread-interactions.test.ts @@ -70,29 +70,37 @@ describe("issue thread interaction schemas", () => { }); it("accepts custom targets for request_confirmation interactions", () => { - const parsed = createIssueThreadInteractionSchema.parse({ - kind: "request_confirmation", - payload: { - version: 1, - prompt: "Proceed with the external checklist?", - target: { - type: "custom", - key: "external-checklist", - revisionId: "checklist-v1", - revisionNumber: 1, - label: "Checklist v1", - href: "https://example.com/checklist", + for (const href of [ + "https://example.com/checklist", + "http://example.com/checklist", + "/PAP/issues/PAP-123#document-plan", + "#document-plan", + ]) { + const parsed = createIssueThreadInteractionSchema.parse({ + kind: "request_confirmation", + payload: { + version: 1, + prompt: "Proceed with the external checklist?", + target: { + type: "custom", + key: "external-checklist", + revisionId: "checklist-v1", + revisionNumber: 1, + label: "Checklist v1", + href, + }, }, - }, - }); + }); - expect(parsed.kind).toBe("request_confirmation"); - if (parsed.kind !== "request_confirmation") return; - expect(parsed.payload.target).toMatchObject({ - type: "custom", - key: "external-checklist", - label: "Checklist v1", - }); + expect(parsed.kind).toBe("request_confirmation"); + if (parsed.kind !== "request_confirmation") return; + expect(parsed.payload.target).toMatchObject({ + type: "custom", + key: "external-checklist", + label: "Checklist v1", + href, + }); + } }); it("rejects unsafe request_confirmation target hrefs", () => { @@ -110,7 +118,16 @@ describe("issue thread interaction schemas", () => { }, } as const; - for (const href of ["javascript:alert(1)", "data:text/html,hi", "//evil.example/path"]) { + for (const href of [ + "javascript:alert(1)", + "data:text/html,hi", + "//evil.example/path", + "file:///tmp/x", + "mailto:user@example.com", + "slack://channel?id=1", + "vscode://file/tmp/x", + "ftp://example.com/file", + ]) { expect(() => createIssueThreadInteractionSchema.parse({ ...base, payload: { @@ -120,7 +137,7 @@ describe("issue thread interaction schemas", () => { href, }, }, - })).toThrow("href must not use javascript:, data:, or protocol-relative URLs"); + })).toThrow("href must be a root-relative path, same-page fragment, or http(s) URL"); } }); @@ -202,6 +219,36 @@ describe("issue thread interaction schemas", () => { })).toThrow("maxSelected must be greater than or equal to minSelected"); }); + it("rejects unsafe request_checkbox_confirmation target hrefs", () => { + const base = { + kind: "request_checkbox_confirmation", + payload: { + version: 1, + prompt: "Which items should be archived?", + options: [{ id: "item-1", label: "Draft report" }], + target: { + type: "custom", + key: "external-checklist", + revisionId: "checklist-v1", + label: "Checklist v1", + }, + }, + } as const; + + for (const href of ["file:///tmp/x", "slack://channel?id=1", "vscode://file/tmp/x"]) { + expect(() => createIssueThreadInteractionSchema.parse({ + ...base, + payload: { + ...base.payload, + target: { + ...base.payload.target, + href, + }, + }, + })).toThrow("href must be a root-relative path, same-page fragment, or http(s) URL"); + } + }); + it("accepts empty checkbox selections and rejects duplicate selected option ids", () => { expect(acceptIssueThreadInteractionSchema.parse({ selectedOptionIds: [] })).toEqual({ selectedOptionIds: [], diff --git a/packages/shared/src/validators/issue.ts b/packages/shared/src/validators/issue.ts index dfbc0f041d3..fa8c8a362db 100644 --- a/packages/shared/src/validators/issue.ts +++ b/packages/shared/src/validators/issue.ts @@ -682,11 +682,10 @@ export const askUserQuestionsResultSchema = z.object({ }); const requestConfirmationHrefSchema = z.string().trim().min(1).max(2000).refine((value) => { - const lower = value.toLowerCase(); - return !lower.startsWith("javascript:") - && !lower.startsWith("data:") - && !value.startsWith("//"); -}, "href must not use javascript:, data:, or protocol-relative URLs"); + if (value.startsWith("#")) return true; + if (value.startsWith("/")) return !value.startsWith("//"); + return /^https?:\/\//i.test(value); +}, "href must be a root-relative path, same-page fragment, or http(s) URL"); const requestConfirmationTargetBaseSchema = z.object({ label: z.string().trim().min(1).max(120).nullable().optional(), diff --git a/ui/src/components/IssueThreadInteractionCard.tsx b/ui/src/components/IssueThreadInteractionCard.tsx index 5b683cbafd1..13c7303c08e 100644 --- a/ui/src/components/IssueThreadInteractionCard.tsx +++ b/ui/src/components/IssueThreadInteractionCard.tsx @@ -8,6 +8,7 @@ import { collectSuggestedTaskClientKeys, countSuggestedTaskNodes, getCheckboxConfirmationSelectedLabels, + getRequestConfirmationTargetHref, getQuestionAnswerLabels, type AskUserQuestionsAnswer, type AskUserQuestionsInteraction, @@ -983,21 +984,6 @@ function requestConfirmationTargetLabel(target: RequestConfirmationTarget) { return `${target.key}${revision}`; } -function requestConfirmationTargetHref({ - issueId, - target, -}: { - issueId: string; - target: RequestConfirmationTarget; -}) { - if (target.href) return target.href; - if (target.type === "issue_document") { - const targetIssueId = target.issueId ?? issueId; - return `/issues/${targetIssueId}#document-${encodeURIComponent(target.key)}`; - } - return null; -} - function RequestConfirmationTargetChip({ issueId, target, @@ -1009,7 +995,7 @@ function RequestConfirmationTargetChip({ }) { if (!target) return null; - const href = requestConfirmationTargetHref({ issueId, target }); + const href = getRequestConfirmationTargetHref({ issueId, target }); const className = cn( "inline-flex max-w-full items-center gap-1.5 rounded-sm border px-2 py-0.5 text-[10px] font-medium uppercase tracking-[0.16em]", tone === "default" diff --git a/ui/src/lib/issue-thread-interactions.test.ts b/ui/src/lib/issue-thread-interactions.test.ts index 78415ac6b6e..6e64993bc80 100644 --- a/ui/src/lib/issue-thread-interactions.test.ts +++ b/ui/src/lib/issue-thread-interactions.test.ts @@ -5,7 +5,9 @@ import { collectSuggestedTaskClientKeys, countSuggestedTaskNodes, getCheckboxConfirmationSelectedLabels, + getRequestConfirmationTargetHref, getQuestionAnswerLabels, + normalizeRequestConfirmationTargetHref, } from "./issue-thread-interactions"; describe("buildSuggestedTaskTree", () => { @@ -186,6 +188,51 @@ describe("issue thread interaction helpers", () => { expect(labels).toEqual(["Charlie", "Alpha"]); }); + it("allows only safe confirmation target hrefs for rendering", () => { + for (const href of [ + "https://example.com/checklist", + "http://example.com/checklist", + "/PAP/issues/PAP-123#document-plan", + "#document-plan", + ]) { + expect(normalizeRequestConfirmationTargetHref(href)).toBe(href); + } + + for (const href of [ + "file:///tmp/x", + "mailto:user@example.com", + "slack://channel?id=1", + "vscode://file/tmp/x", + "ftp://example.com/file", + "//evil.example/path", + ]) { + expect(normalizeRequestConfirmationTargetHref(href)).toBeNull(); + } + }); + + it("does not return unsafe custom target hrefs from accepted payloads", () => { + expect(getRequestConfirmationTargetHref({ + issueId: "issue-1", + target: { + type: "custom", + key: "unsafe-target", + label: "Unsafe target", + href: "file:///tmp/x", + }, + })).toBeNull(); + + expect(getRequestConfirmationTargetHref({ + issueId: "issue-1", + target: { + type: "issue_document", + issueId: "issue-2", + key: "plan", + revisionId: "11111111-1111-4111-8111-111111111111", + href: "slack://channel?id=1", + }, + })).toBe("/issues/issue-2#document-plan"); + }); + it("maps stored option ids back to labels for answered summaries", () => { const labels = getQuestionAnswerLabels({ question: { diff --git a/ui/src/lib/issue-thread-interactions.ts b/ui/src/lib/issue-thread-interactions.ts index 2999b27ba3e..5da5b05081b 100644 --- a/ui/src/lib/issue-thread-interactions.ts +++ b/ui/src/lib/issue-thread-interactions.ts @@ -33,6 +33,7 @@ import type { RequestCheckboxConfirmationPayload, RequestCheckboxConfirmationResult, RequestConfirmationInteraction, + RequestConfirmationTarget, SuggestedTaskDraft, SuggestTasksInteraction, SuggestTasksResultCreatedTask, @@ -73,6 +74,31 @@ export function getCheckboxConfirmationSelectedLabels(args: { .filter((label): label is string => typeof label === "string"); } +export function normalizeRequestConfirmationTargetHref(href: string) { + const value = href.trim(); + if (value.startsWith("#")) return value; + if (value.startsWith("/")) return value.startsWith("//") ? null : value; + return /^https?:\/\//i.test(value) ? value : null; +} + +export function getRequestConfirmationTargetHref({ + issueId, + target, +}: { + issueId: string; + target: RequestConfirmationTarget; +}) { + if (target.href) { + const safeHref = normalizeRequestConfirmationTargetHref(target.href); + if (safeHref) return safeHref; + } + if (target.type === "issue_document") { + const targetIssueId = target.issueId ?? issueId; + return `/issues/${targetIssueId}#document-${encodeURIComponent(target.key)}`; + } + return null; +} + export function buildIssueThreadInteractionSummary( interaction: IssueThreadInteraction, ) {