diff --git a/DEVLOG.md b/DEVLOG.md index 613f0969..ed18620c 100644 --- a/DEVLOG.md +++ b/DEVLOG.md @@ -2,6 +2,19 @@ > Append-only session log. Read at session start. Update at session end. +## 2026-06-18 — Safe URL scheme handling for rich content links + +Fixed AXI-85 by centralizing renderable/external URL validation and applying it to rich markdown links, link attachments, and existing link-attachment open/preview surfaces. + +Fixes: +- Added `src/lib/url-safety.ts` to allow only `http:`/`https:` external URLs and intentional internal app paths beginning with `/` for rendered markdown navigation. +- Updated `MarkdownWithAttachments` so unsafe markdown link schemes such as `javascript:` and `data:` render as inert text, while `https://…`, internal `/w/...` links, and existing http(s)-only forge-link chips continue to work. +- Enforced http(s)-only link attachment URLs in the tRPC attachment router, MCP `attachments.attachLink`, and storage helper; also guarded existing link attachment chip/lightbox/canvas open and iframe-preview paths against legacy unsafe values. +- Added regression tests for shared URL safety, markdown rendering behavior, tRPC attachLink rejection, and MCP attachLink rejection. + +Verify: +`env -u OPENAI_API_KEY pnpm exec vitest run tests/unit/url-safety.test.ts tests/unit/markdown-url-safety.test.ts src/server/services/__tests__/mcp.test.ts src/server/routers/__tests__/attachment.test.ts --reporter=verbose --testNamePattern "url safety|MarkdownWithAttachments URL safety|attachments.attachLink accepts only http\\(s\\) external URLs|rejects non-http"`, `pnpm typecheck`, `pnpm lint`, `env -u OPENAI_API_KEY pnpm test`. + ## 2026-06-18 — Quick-access Mission Control and bounded Command Center Refined the floating activity dock after the live Command Center screenshots diff --git a/src/app/(app)/w/[slug]/canvas/[canvasId]/page.tsx b/src/app/(app)/w/[slug]/canvas/[canvasId]/page.tsx index 8d32b0b1..144acc5d 100644 --- a/src/app/(app)/w/[slug]/canvas/[canvasId]/page.tsx +++ b/src/app/(app)/w/[slug]/canvas/[canvasId]/page.tsx @@ -64,6 +64,7 @@ import { type InspectorSelection, } from "@/components/canvas/canvas-selection-inspector"; import { computeSnap } from "@/lib/canvas-snap-guides"; +import { isSafeExternalUrl } from "@/lib/url-safety"; import { useCanvasUndoStack } from "@/lib/canvas-undo"; import { CanvasEntityCreator, @@ -6014,7 +6015,9 @@ function AttachmentPreviewCardBody({ node }: { node: HydratedNode }) { }, ); const previewKind = canvasKindForAttachment(meta); - const url = isLink ? meta.externalUrl ?? null : dl.data?.url ?? null; + const safeExternalUrl = + meta.externalUrl && isSafeExternalUrl(meta.externalUrl) ? meta.externalUrl : null; + const url = isLink ? safeExternalUrl : dl.data?.url ?? null; // Header — filename + mime chip. Padding mirrors the legacy card body. return (
@@ -6039,8 +6042,8 @@ function AttachmentPreviewCardBody({ node }: { node: HydratedNode }) { height={Math.max(120, node.height - 60)} onExpand={() => { if (isLink) { - if (meta.externalUrl) { - window.open(meta.externalUrl, "_blank", "noopener,noreferrer"); + if (safeExternalUrl) { + window.open(safeExternalUrl, "_blank", "noopener,noreferrer"); } return; } diff --git a/src/components/attachments/attachment-chip.tsx b/src/components/attachments/attachment-chip.tsx index 84478573..7321f812 100644 --- a/src/components/attachments/attachment-chip.tsx +++ b/src/components/attachments/attachment-chip.tsx @@ -11,6 +11,7 @@ import { } from "lucide-react"; import { trpc } from "@/lib/trpc"; import { cn } from "@/lib/utils"; +import { isSafeExternalUrl } from "@/lib/url-safety"; import { useAttachmentLightbox } from "@/components/attachments/attachment-lightbox"; /** @@ -32,7 +33,7 @@ export function LinkFavicon({ let origin = ""; let host = ""; try { - if (url) { + if (url && isSafeExternalUrl(url)) { const parsed = new URL(url); origin = parsed.origin; host = parsed.hostname.replace(/^www\./i, "").toLowerCase(); @@ -110,7 +111,7 @@ function isLinkAttachment(a: AttachmentChipData): boolean { /** Best-effort hostname extraction for LINK chips' trailing slot. */ function safeHostname(url: string | null | undefined): string { - if (!url) return ""; + if (!url || !isSafeExternalUrl(url)) return ""; try { return new URL(url).hostname.replace(/^www\./i, ""); } catch { @@ -142,7 +143,7 @@ export function AttachmentChip({ // is "open the page in a new tab". Lightbox would render an // iframe-via-presigned-url that doesn't apply here. const href = attachment.externalUrl ?? ""; - if (href) { + if (href && isSafeExternalUrl(href)) { window.open(href, "_blank", "noopener,noreferrer"); } return; diff --git a/src/components/attachments/attachment-lightbox.tsx b/src/components/attachments/attachment-lightbox.tsx index e7534b0d..9ac68929 100644 --- a/src/components/attachments/attachment-lightbox.tsx +++ b/src/components/attachments/attachment-lightbox.tsx @@ -23,6 +23,7 @@ import { useModalBehavior } from "@/components/ui/modal/use-modal-behavior"; import { trpc } from "@/lib/trpc"; import { cn } from "@/lib/utils"; import { MOTION } from "@/lib/motion"; +import { isSafeExternalUrl } from "@/lib/url-safety"; import { LinkFavicon } from "@/components/attachments/attachment-chip"; /** @@ -192,7 +193,11 @@ function Lightbox({ retry: false, }, ); - const url = isLink ? (current?.externalUrl ?? "") : data?.url; + const url = isLink + ? current?.externalUrl && isSafeExternalUrl(current.externalUrl) + ? current.externalUrl + : "" + : data?.url; // Delete plumbing — confirm modal stacks on top of the lightbox. const [confirmOpen, setConfirmOpen] = React.useState(false); @@ -499,7 +504,10 @@ function Preview({ } function LinkPreview({ attachment }: { attachment: AttachmentLite }) { - const href = attachment.externalUrl ?? ""; + const href = + attachment.externalUrl && isSafeExternalUrl(attachment.externalUrl) + ? attachment.externalUrl + : ""; let host = ""; try { if (href) host = new URL(href).hostname.replace(/^www\./i, ""); diff --git a/src/components/attachments/issue-attachments-panel.tsx b/src/components/attachments/issue-attachments-panel.tsx index 6f4ba268..8fe6fb5e 100644 --- a/src/components/attachments/issue-attachments-panel.tsx +++ b/src/components/attachments/issue-attachments-panel.tsx @@ -16,6 +16,7 @@ import { Button } from "@/components/ui/button"; import { Confirm } from "@/components/ui/modal"; import { trpc } from "@/lib/trpc"; import { cn } from "@/lib/utils"; +import { isSafeExternalUrl } from "@/lib/url-safety"; import { useWorkspace } from "@/hooks/use-workspace"; import { useAttachmentLightbox } from "@/components/attachments/attachment-lightbox"; import { LinkFavicon } from "@/components/attachments/attachment-chip"; @@ -494,7 +495,7 @@ function AttachmentTile({ const openTile = () => { if (isLink) { const href = attachment.externalUrl ?? ""; - if (href) window.open(href, "_blank", "noopener,noreferrer"); + if (href && isSafeExternalUrl(href)) window.open(href, "_blank", "noopener,noreferrer"); return; } lightbox.open({ @@ -633,7 +634,7 @@ function isPdf(mime: string): boolean { } function linkAttachmentMeta(url: string | null): { host: string; path: string } { - if (!url) return { host: "", path: "" }; + if (!url || !isSafeExternalUrl(url)) return { host: "", path: "" }; try { const parsed = new URL(url); const host = parsed.hostname.replace(/^www\./i, ""); diff --git a/src/components/markdown/attachment-renderer.tsx b/src/components/markdown/attachment-renderer.tsx index 13d7c8bd..b5b860de 100644 --- a/src/components/markdown/attachment-renderer.tsx +++ b/src/components/markdown/attachment-renderer.tsx @@ -31,6 +31,7 @@ import { YouTubeEmbed } from "@/components/embeds/youtube-embed"; import { GithubEmbed } from "@/components/embeds/github-embed"; import { LoomEmbed } from "@/components/embeds/loom-embed"; import { FigmaEmbed } from "@/components/embeds/figma-embed"; +import { isSafeExternalUrl, toRenderableHref } from "@/lib/url-safety"; /** * Rich body renderer for descriptions, comments, artifacts, and notes. @@ -663,49 +664,47 @@ function renderInlineSegs( inlineList={inlineList} /> ); - if (s.type === "linkChip") + if (s.type === "linkChip") { + if (!isSafeExternalUrl(s.url)) { + return {s.label || s.url}; + } return ; + } if (s.type === "issueRef") return ; if (s.type === "mention") return ; if (s.type === "mdLink") { - // External markdown link — open in a new tab. Internal absolute - // paths (e.g. `/w/foo/issues/abc`) get an in-app Link. - if (/^https?:\/\//i.test(s.url)) { + // Explicit allowlist: http(s) links open in a new tab, and internal + // app paths (e.g. `/w/foo/issues/abc`) get an in-app Link. Everything + // else renders as inert text so `javascript:` / `data:` never become + // clickable browser-controlled hrefs. + const renderable = toRenderableHref(s.url); + if (renderable?.kind === "external") { return ( - {s.label || s.url} + {s.label || renderable.href} ); } - if (s.url.startsWith("/")) { + if (renderable?.kind === "internal") { return ( - {s.label || s.url} + {s.label || renderable.href} ); } - // Other schemes (mailto:, etc.) render as plain anchors. - return ( - - {s.label || s.url} - - ); + return {s.label || s.url}; } if (s.type === "url") return ( diff --git a/src/lib/url-safety.ts b/src/lib/url-safety.ts new file mode 100644 index 00000000..f0660f4c --- /dev/null +++ b/src/lib/url-safety.ts @@ -0,0 +1,49 @@ +export type RenderableHrefKind = "external" | "internal"; + +export interface RenderableHref { + href: string; + kind: RenderableHrefKind; +} + +const SAFE_EXTERNAL_PROTOCOLS = new Set(["http:", "https:"]); + +export function isSafeExternalUrl(value: string): boolean { + const trimmed = value.trim(); + if (!trimmed) return false; + try { + const parsed = new URL(trimmed); + return SAFE_EXTERNAL_PROTOCOLS.has(parsed.protocol); + } catch { + return false; + } +} + +export function assertSafeExternalUrl(value: string): string { + const trimmed = value.trim(); + if (!isSafeExternalUrl(trimmed)) { + throw new Error("External links must use http or https URLs."); + } + return trimmed; +} + +export function safeExternalUrlMessage(): string { + return "External links must use http or https URLs."; +} + +export function isSafeInternalAppPath(value: string): boolean { + const trimmed = value.trim(); + if (!trimmed.startsWith("/")) return false; + // Avoid protocol-relative URLs (`//example.com`) and backslash variants + // that browsers can treat as host-relative navigations. + if (trimmed.startsWith("//") || trimmed.startsWith("/\\")) return false; + if (/^\/%5c/i.test(trimmed)) return false; + return true; +} + +export function toRenderableHref(value: string): RenderableHref | null { + const trimmed = value.trim(); + if (!trimmed) return null; + if (isSafeInternalAppPath(trimmed)) return { href: trimmed, kind: "internal" }; + if (isSafeExternalUrl(trimmed)) return { href: trimmed, kind: "external" }; + return null; +} diff --git a/src/server/routers/__tests__/attachment.test.ts b/src/server/routers/__tests__/attachment.test.ts index fc0eaed2..25c027b0 100644 --- a/src/server/routers/__tests__/attachment.test.ts +++ b/src/server/routers/__tests__/attachment.test.ts @@ -61,6 +61,28 @@ async function putBytes(url: string, body: Buffer, mimeType: string) { } describe("attachmentRouter", () => { + it("rejects non-http(s) link attachment URLs", async () => { + const { caller, fixture } = await setup(); + const issue = await createIssue(fixture); + + await expect( + caller.attachLink({ + targetType: "issue", + targetId: issue.id, + url: "javascript:alert(1)", + title: "Bad", + }), + ).rejects.toThrow(/http or https/i); + await expect( + caller.attachLink({ + targetType: "issue", + targetId: issue.id, + url: "data:text/html,

x

", + title: "Bad", + }), + ).rejects.toThrow(/http or https/i); + }); + it("allows chat-message uploads for the owning human thread", async () => { const { caller, fixture } = await setup(); const prisma = getPrisma(); diff --git a/src/server/routers/attachment.ts b/src/server/routers/attachment.ts index d8a7aeaf..8374cda4 100644 --- a/src/server/routers/attachment.ts +++ b/src/server/routers/attachment.ts @@ -2,6 +2,11 @@ import { z } from "zod"; import { TRPCError } from "@trpc/server"; import { EventKind } from "@prisma/client"; import type { PrismaClient } from "@prisma/client"; +import { + assertSafeExternalUrl, + isSafeExternalUrl, + safeExternalUrlMessage, +} from "@/lib/url-safety"; import { router, workspaceProcedure, adminProcedure } from "@/server/trpc"; import { recordChange } from "@/server/audit"; import { @@ -74,7 +79,9 @@ const finalizeInput = z.object({ const attachLinkInput = z.object({ targetType: targetTypeSchema, targetId: z.string().cuid(), - url: z.string().url().max(2048), + url: z.string().max(2048).refine(isSafeExternalUrl, { + message: safeExternalUrlMessage(), + }), title: z.string().max(255).optional(), }); @@ -175,13 +182,14 @@ export const attachmentRouter = router({ .input(attachLinkInput) .mutation(async ({ ctx, input }) => { await assertTargetInWorkspace(ctx, input.targetType, input.targetId); + const safeUrl = assertSafeExternalUrl(input.url); // Caller didn't provide a label → try to scrape off the // target page so the chip is meaningful. Fail soft: any error // leaves resolvedTitle null and createLinkAttachment falls back // to hostname. let resolvedTitle: string | null = input.title ?? null; if (resolvedTitle === null) { - const meta = await fetchLinkMetadata(input.url); + const meta = await fetchLinkMetadata(safeUrl); if (meta.title) resolvedTitle = meta.title; } let row: Awaited<ReturnType<typeof createLinkAttachment>>; @@ -190,7 +198,7 @@ export const attachmentRouter = router({ workspaceId: ctx.workspaceId, targetType: input.targetType, targetId: input.targetId, - url: input.url, + url: safeUrl, title: resolvedTitle, }); } catch (err) { diff --git a/src/server/services/__tests__/mcp.test.ts b/src/server/services/__tests__/mcp.test.ts index b188068f..61ba69fd 100644 --- a/src/server/services/__tests__/mcp.test.ts +++ b/src/server/services/__tests__/mcp.test.ts @@ -103,6 +103,53 @@ function envelopeOf<T>(value: unknown): { data: T[]; nextCursor?: string } { } describe("mcp tool registry", () => { + it("attachments.attachLink accepts only http(s) external URLs", async () => { + const f = await createWorkspaceFixture({ keyPrefix: "LNK" }); + fixtures.push(f); + const issue = await createIssue(f, { title: "link target" }); + const { ctx } = buildMcpCtx(f); + + const link = (await call( + "attachments.attachLink", + { + targetType: "issue", + targetId: issue.id, + url: "https://example.com/runbook", + title: "Runbook", + }, + ctx, + )) as { externalUrl: string; kind: string }; + expect(link).toMatchObject({ + externalUrl: "https://example.com/runbook", + kind: "LINK", + }); + + await expect( + call( + "attachments.attachLink", + { + targetType: "issue", + targetId: issue.id, + url: "javascript:alert(1)", + title: "Bad", + }, + ctx, + ), + ).rejects.toThrow(/http or https/i); + await expect( + call( + "attachments.attachLink", + { + targetType: "issue", + targetId: issue.id, + url: "data:text/html,<h1>x</h1>", + title: "Bad", + }, + ctx, + ), + ).rejects.toThrow(/http or https/i); + }); + it("registers >= 30 tools spanning the new primitives", () => { const names = Object.keys(mcpTools); expect(names.length).toBeGreaterThanOrEqual(30); diff --git a/src/server/services/mcp.ts b/src/server/services/mcp.ts index 27ed07dd..6299739a 100644 --- a/src/server/services/mcp.ts +++ b/src/server/services/mcp.ts @@ -70,6 +70,11 @@ import { presignUploadUrl, } from "@/server/services/storage"; import { forgeEntityTypeSchema, type ForgeEntityType } from "@/lib/entity-ref"; +import { + assertSafeExternalUrl, + isSafeExternalUrl, + safeExternalUrlMessage, +} from "@/lib/url-safety"; import { hydrateEntityRefs } from "@/server/services/entity-hydration"; import { computeAlignment, type AlignItem } from "@/server/services/canvas-alignment"; import { validateRuntimeConfig } from "@/server/services/runtime-config"; @@ -4628,9 +4633,9 @@ export const mcpTools = { targetId: z.string().describe("Cuid of the target row in this workspace."), url: z .string() - .url() .max(2048) - .describe("Absolute URL to attach. Must be parseable by URL(); typically https://… ."), + .refine(isSafeExternalUrl, { message: safeExternalUrlMessage() }) + .describe("Absolute http(s) URL to attach."), title: z .string() .max(255) @@ -4669,15 +4674,16 @@ export const mcpTools = { // soft — createLinkAttachment falls back to hostname when title is // null/undefined. let resolvedTitle = input.title; + const safeUrl = assertSafeExternalUrl(input.url); if (resolvedTitle === undefined) { - const meta = await fetchLinkMetadata(input.url); + const meta = await fetchLinkMetadata(safeUrl); resolvedTitle = meta.title; } return createLinkAttachment({ workspaceId: ctx.workspaceId, targetType: input.targetType, targetId: input.targetId, - url: input.url, + url: safeUrl, title: resolvedTitle, }); }, diff --git a/src/server/services/storage.ts b/src/server/services/storage.ts index dfed5b71..bd1b8e3d 100644 --- a/src/server/services/storage.ts +++ b/src/server/services/storage.ts @@ -16,6 +16,7 @@ import { } from "@aws-sdk/client-s3"; import { getSignedUrl } from "@aws-sdk/s3-request-presigner"; import { randomUUID } from "node:crypto"; +import { assertSafeExternalUrl } from "@/lib/url-safety"; import { db } from "@/server/db"; /** @@ -758,9 +759,10 @@ export async function createLinkAttachment( input.targetType, input.targetId, ); - // URL parsing is the only validation we do — caller supplies the - // workspace scope check. - const host = hostnameOf(input.url); + // Enforce browser-safe external links at storage time as a defense in depth + // for every caller (tRPC, MCP, future imports/backfills). + const url = assertSafeExternalUrl(input.url); + const host = hostnameOf(url); const linkTitle = (input.title ?? "").trim() || host; if (linkTitle.length > 255) { throw new Error("Link title must be 255 characters or less."); @@ -775,8 +777,8 @@ export async function createLinkAttachment( filename: linkTitle, mimeType: LINK_ATTACHMENT_MIME, size: 0, - url: input.url, - externalUrl: input.url, + url, + externalUrl: url, linkTitle, }, }); @@ -789,7 +791,7 @@ export async function createLinkAttachment( mimeType: row.mimeType, size: row.size, url: row.url, - externalUrl: row.externalUrl ?? input.url, + externalUrl: row.externalUrl ?? url, linkTitle: row.linkTitle ?? linkTitle, kind: "LINK", createdAt: row.createdAt, diff --git a/tests/unit/markdown-url-safety.test.ts b/tests/unit/markdown-url-safety.test.ts new file mode 100644 index 00000000..b629dde3 --- /dev/null +++ b/tests/unit/markdown-url-safety.test.ts @@ -0,0 +1,51 @@ +import React from "react"; +import { renderToStaticMarkup } from "react-dom/server"; +import { describe, expect, it } from "vitest"; +import { MarkdownWithAttachments } from "@/components/markdown/attachment-renderer"; + +(globalThis as unknown as { React: typeof React }).React = React; + +function renderMarkdown(body: string): string { + return renderToStaticMarkup(React.createElement(MarkdownWithAttachments, { body })); +} + +describe("MarkdownWithAttachments URL safety", () => { + it("renders safe external markdown links as new-tab anchors", () => { + const html = renderMarkdown("[Example](https://example.com/docs)"); + + expect(html).toContain('href="https://example.com/docs"'); + expect(html).toContain('target="_blank"'); + expect(html).toContain("Example"); + }); + + it("renders internal app paths as internal links", () => { + const html = renderMarkdown("[Issue](/w/axiom-labs/issues/AXI-85)"); + + expect(html).toContain('href="/w/axiom-labs/issues/AXI-85"'); + expect(html).toContain("Issue"); + expect(html).not.toContain('target="_blank"'); + }); + + it("does not render javascript or data markdown links as clickable hrefs", () => { + const jsHtml = renderMarkdown("[bad](javascript:alert(1))"); + const dataHtml = renderMarkdown("[bad](data:text/html,<h1>x</h1>)"); + + expect(jsHtml).toContain("bad"); + expect(jsHtml).not.toContain("javascript:alert"); + expect(jsHtml).not.toContain("<a "); + expect(dataHtml).toContain("bad"); + expect(dataHtml).not.toContain("data:text/html"); + expect(dataHtml).not.toContain("<a "); + }); + + it("keeps forge-link chips limited to http(s)", () => { + const html = renderMarkdown( + "[good](forge-link:https://example.com) [bad](forge-link:javascript:alert(1))", + ); + + expect(html).toContain('href="https://example.com"'); + expect(html).toContain("good"); + expect(html).toContain("forge-link:javascript:alert(1)"); + expect(html).not.toContain('href="javascript:alert'); + }); +}); diff --git a/tests/unit/url-safety.test.ts b/tests/unit/url-safety.test.ts new file mode 100644 index 00000000..38d9245f --- /dev/null +++ b/tests/unit/url-safety.test.ts @@ -0,0 +1,43 @@ +import { describe, expect, it } from "vitest"; +import { + assertSafeExternalUrl, + isSafeExternalUrl, + isSafeInternalAppPath, + toRenderableHref, +} from "@/lib/url-safety"; + +describe("url safety helpers", () => { + it("allows only http(s) external URLs", () => { + expect(isSafeExternalUrl("https://example.com/path")).toBe(true); + expect(isSafeExternalUrl(" http://example.com ")).toBe(true); + expect(isSafeExternalUrl("javascript:alert(1)")).toBe(false); + expect(isSafeExternalUrl("data:text/html,<h1>x</h1>")).toBe(false); + expect(isSafeExternalUrl("mailto:ops@example.com")).toBe(false); + expect(isSafeExternalUrl("/w/axiom-labs/issues/AXI-85")).toBe(false); + }); + + it("allows internal app paths without accepting protocol-relative URLs", () => { + expect(isSafeInternalAppPath("/w/axiom-labs/issues/AXI-85")).toBe(true); + expect(isSafeInternalAppPath("//evil.example/path")).toBe(false); + expect(isSafeInternalAppPath("/\\evil.example/path")).toBe(false); + expect(isSafeInternalAppPath("/%5cevil.example/path")).toBe(false); + }); + + it("normalizes renderable hrefs to external, internal, or inert", () => { + expect(toRenderableHref("https://example.com")).toEqual({ + href: "https://example.com", + kind: "external", + }); + expect(toRenderableHref("/w/axiom-labs")).toEqual({ + href: "/w/axiom-labs", + kind: "internal", + }); + expect(toRenderableHref("javascript:alert(1)")).toBeNull(); + expect(toRenderableHref("data:text/html,<h1>x</h1>")).toBeNull(); + }); + + it("throws a stable validation message for unsafe external URLs", () => { + expect(assertSafeExternalUrl(" https://example.com ")).toBe("https://example.com"); + expect(() => assertSafeExternalUrl("javascript:alert(1)")).toThrow(/http or https/i); + }); +});