diff --git a/apps/web/src/chat/Markdown.tsx b/apps/web/src/chat/Markdown.tsx index 5fe1a89a4..0e1934e38 100644 --- a/apps/web/src/chat/Markdown.tsx +++ b/apps/web/src/chat/Markdown.tsx @@ -14,12 +14,14 @@ export function Markdown({ className = CHAT_PROSE, remarkPlugins, rehypePlugins, + urlTransform, components, }: { text: string; className?: string; remarkPlugins?: ComponentProps["remarkPlugins"]; rehypePlugins?: ComponentProps["rehypePlugins"]; + urlTransform?: ComponentProps["urlTransform"]; components?: ComponentProps["components"]; }) { return ( @@ -27,6 +29,7 @@ export function Markdown({ {text} diff --git a/apps/web/src/chat/SPEC.md b/apps/web/src/chat/SPEC.md index fe00634d5..1cf787ea5 100644 --- a/apps/web/src/chat/SPEC.md +++ b/apps/web/src/chat/SPEC.md @@ -79,7 +79,21 @@ blocks in order into rows; `ChatTurnView` dispatches on row kind: summarized disappear immediately rather than only after reload. Its `summary` opens on click (`data-testid="chat-compaction"`). Hydration/reopen starts directly from that same durable form. Both forms share the **"Context compacted"** title; only facts unavailable after reload disappear. -- `markdown` — a non-empty assistant text block (react-markdown + remark-gfm + shiki). A fenced +- `markdown` — a non-empty assistant text block (react-markdown + remark-gfm + shiki). Safe + worktree-relative links in assistant prose open the target in ThinkRail through `ChatTurnView`'s existing + workspace-file callback; absolute paths are accepted only when they normalize inside the active worktree, + while URL schemes, protocol-relative URLs, fragments, and unsafe/outside paths retain ordinary safe + new-tab anchor behavior. Percent-encoded file paths are decoded once before validation, so encoded + separators and traversal cannot bypass containment. A narrow assistant-only URL transform preserves + recognized Windows drive-letter anchor paths, including Markdown's percent-encoded backslash form, until + validation; drive-rooted containment compares case-insensitively while preserving the linked path's casing. + Every other value delegates to react-markdown's default sanitizer, and a rejected Windows path + is re-sanitized before fallback anchor rendering. The generic `Markdown` primitive remains props-driven and + receives this behavior only as an `a` component override at the assistant-turn integration edge; + accepted workspace targets render as button controls without a raw browser `href`, so alternate native + anchor activation cannot escape into the SPA fallback. That override keeps a stable component identity + while its workspace inputs are unchanged: workbench focus can rerender a chat row between pointer-down and + click, and replacing the control in that interval cancels activation. A fenced ```mermaid block renders as a themed diagram via `tools/visualize`'s `MermaidView` (fullscreen pan-zoom, error → source fallback) — uniform across every `Markdown` surface (chat, file/specs preview); until mounted it renders as highlighted source, so static contexts (`RenderedDiff`'s @@ -879,14 +893,15 @@ from their `toolCall` args and reply through **`ChatActions`** (see below). Work ## Boundary -- **Public surface:** the registry API (`toolRegistry`), the props-driven slash-completion primitive, and - the renderers (incl. the presentational `Markdown` — GFM + shiki, no store/transport; the rendering is fixed but the **prose skin** is the +- **Public surface:** the registry API (`toolRegistry`), the shared workspace-file target canonicalizer + (`fileTargets`), the props-driven slash-completion primitive, and the renderers (incl. the presentational + `Markdown` — GFM + shiki, no store/transport; the rendering is fixed but the **prose skin** is the caller's via an optional `className` — chat uses the compact bubble skin (`tr-prose-chat`), `panels/MarkdownPreview` the document skin (`tr-prose-doc`). A skin names exactly one generated `tr-prose-*` system and then carries only spacing/measure/chrome — no size, weight, leading or tracking (see `styles/TYPOGRAPHY.md`); a caller may - also **extend** the render with extra `remarkPlugins` + `components`, e.g. the file view's GitHub - alert callouts), the view types + also **extend** the render with an optional `urlTransform`, extra `remarkPlugins`, and `components`, e.g. + the file view's GitHub alert callouts), the view types (`types.ts`, incl. `ToolResultState` + `ExtUiDialogRequest`), and `ChatView` (lazy-mounted by the shell workbench resource renderer; diff --git a/apps/web/src/chat/assistantLinks.test.tsx b/apps/web/src/chat/assistantLinks.test.tsx new file mode 100644 index 000000000..6b825eea7 --- /dev/null +++ b/apps/web/src/chat/assistantLinks.test.tsx @@ -0,0 +1,91 @@ +import { describe, expect, test } from "bun:test"; +import { renderToStaticMarkup } from "react-dom/server"; +import { AssistantMarkdown, assistantFileTarget, assistantUrlTransform } from "./assistantLinks"; + +describe("assistantFileTarget", () => { + test("accepts normalized relative paths and absolute paths inside the workspace", () => { + expect(assistantFileTarget(".thinkrail/context/report.md", "/repo")).toBe( + ".thinkrail/context/report.md", + ); + expect(assistantFileTarget("docs/../README.md#usage", "/repo")).toBe("README.md"); + expect(assistantFileTarget("reports/manual%20report.md", "/repo")).toBe( + "reports/manual report.md", + ); + expect(assistantFileTarget("/repo/docs/report.md?raw=1", "/repo")).toBe("docs/report.md"); + expect(assistantFileTarget("c:/repo/docs/Report.md", "C:/Repo")).toBe("docs/Report.md"); + }); + + test("rejects external, fragment, outside-workspace, and context-free targets", () => { + for (const href of [ + "https://example.com/report", + "mailto:owner@example.com", + "//example.com/report", + "#report", + "../report.md", + "%2e%2e/report.md", + "/tmp/report.md", + "report%E0%A4%A.md", + ]) { + expect(assistantFileTarget(href, "/repo")).toBeNull(); + } + expect(assistantFileTarget("README.md", undefined)).toBeNull(); + }); +}); + +test("assistant URL transforms preserve only anchor-shaped Windows paths", () => { + const anchor = { tagName: "a" }; + expect(assistantUrlTransform("C:/repo/report.md", "href", anchor)).toBe("C:/repo/report.md"); + expect(assistantUrlTransform("C:%5Crepo%5Creport.md", "href", anchor)).toBe( + "C:%5Crepo%5Creport.md", + ); + expect(assistantUrlTransform("C:/repo/image.png", "src", { tagName: "img" })).toBe(""); + expect(assistantUrlTransform("C:/repo/style.css", "href", { tagName: "link" })).toBe(""); + expect(assistantUrlTransform("javascript:alert(1)", "href", anchor)).toBe(""); + expect(assistantUrlTransform("https://example.com/report", "href", anchor)).toBe( + "https://example.com/report", + ); +}); + +test("assistant Markdown distinguishes workspace files from ordinary anchors", () => { + const html = renderToStaticMarkup( + )", + "[site](https://example.com/report)", + "[outside](../report.md)", + ].join("\n\n")} + workspaceRoot="/repo" + onOpenFile={() => {}} + />, + ); + + expect(html).toContain(' + ); +} + +export function AssistantMarkdown({ + text, + workspaceRoot, + onOpenFile, +}: { + text: string; + workspaceRoot?: string | undefined; + onOpenFile?: ((path: string) => void) | undefined; +}) { + const components = useMemo( + () => ({ + a: ({ href, children }) => ( + + {children} + + ), + }), + [workspaceRoot, onOpenFile], + ); + return ; +} diff --git a/apps/web/src/chat/fileTargets.test.ts b/apps/web/src/chat/fileTargets.test.ts new file mode 100644 index 000000000..9e2211c4b --- /dev/null +++ b/apps/web/src/chat/fileTargets.test.ts @@ -0,0 +1,19 @@ +import { describe, expect, it } from "bun:test"; +import { workspaceFileTarget } from "./fileTargets"; + +describe("workspaceFileTarget", () => { + it("canonicalizes only paths contained by the active worktree", () => { + for (const [path, root, expected] of [ + ["module-a/../README.md", "/repo", "README.md"], + ["/repo/module-a/SPEC.md", "/repo", "module-a/SPEC.md"], + ["C:\\repo\\module-a\\SPEC.md", "C:\\repo", "module-a/SPEC.md"], + ["/repo-other/SPEC.md", "/repo", null], + ["../outside.md", "/repo", null], + ["https://example.com/a.md", "/repo", null], + ["file:///repo/a.md", "/repo", null], + ["", "/repo", null], + ] as const) { + expect(workspaceFileTarget(path, root)).toBe(expected); + } + }); +}); diff --git a/apps/web/src/chat/fileTargets.ts b/apps/web/src/chat/fileTargets.ts new file mode 100644 index 000000000..96bbf7ad6 --- /dev/null +++ b/apps/web/src/chat/fileTargets.ts @@ -0,0 +1,26 @@ +import { isAbsolutePath, projectRelativePath } from "@/lib"; + +const URI_SCHEME = /^[A-Za-z][A-Za-z0-9+.-]*:/; +const WINDOWS_ABSOLUTE_PATH = /^[A-Za-z]:[\\/]/; + +export function hasUriScheme(value: string): boolean { + return URI_SCHEME.test(value); +} + +export function isWindowsAbsolutePath(value: string): boolean { + return WINDOWS_ABSOLUTE_PATH.test(value); +} + +export function workspaceFileTarget( + path: string, + workspaceRoot?: string | undefined, +): string | null { + const candidate = path.trim(); + if (!candidate) return null; + if (!isAbsolutePath(candidate) && hasUriScheme(candidate)) return null; + const relative = projectRelativePath(candidate, workspaceRoot); + if (!relative || isAbsolutePath(relative) || relative === ".." || relative.startsWith("../")) { + return null; + } + return relative; +} diff --git a/apps/web/src/chat/tools/SPEC.md b/apps/web/src/chat/tools/SPEC.md index 291e3fd9d..f0b65a0ed 100644 --- a/apps/web/src/chat/tools/SPEC.md +++ b/apps/web/src/chat/tools/SPEC.md @@ -200,8 +200,9 @@ registration runs once when the chat module mounts. Unregistered tools fall back also registered for `get_subagent_result`, routine), the `SubagentCompletionCard` turn card, and the pure `runDetails` readers/formatters; own child spec ([subagent/SPEC.md](subagent/SPEC.md)). - **Shared pieces** — `CodeBlock` (shiki), `Collapsible` ("Show all N lines" fold for long output), - `ToolFileLink` + exact-reference linked text, pure `toolHelpers` (arg readers, `resultText`, - `languageFromPath`) + `lib`'s `projectRelativePath`. `resultText` delegates canonical result parsing to + `ToolFileLink` + exact-reference linked text backed by the parent chat module's shared + `workspaceFileTarget`, pure `toolHelpers` (arg readers, `resultText`, `languageFromPath`) + `lib`'s + `projectRelativePath`. `resultText` delegates canonical result parsing to the parent chat primitive, so text extraction and the common image layer cannot disagree about what constitutes a valid content block. @@ -209,7 +210,8 @@ registration runs once when the chat module mounts. Unregistered tools fall back - **Public surface:** the side-effect `register` import + the shared `CodeBlock`/`Collapsible`/ `toolHelpers` for sibling renderers + `visualize/MermaidView` for the parent `Markdown` primitive. No barrel (chat pulls shiki — per-file imports, as in the parent). -- **Allowed deps:** parent chat primitives (`toolRegistry`, `Markdown`, `ChatActions`, `askState`); +- **Allowed deps:** parent chat primitives (`toolRegistry`, `Markdown`, `ChatActions`, `askState`, + `fileTargets`); `contracts` (type-only + the `ASK_USER_ANSWERS_CUSTOM_TYPE` constant); `components/ui`; `lib`; `@remixicon/react`; `mermaid` (**lazy, `visualize/` only**). - **Forbidden:** value-importing any `pi` package; `store`/`transport` (renderers stay presentational — diff --git a/apps/web/src/chat/tools/ToolFileLink.tsx b/apps/web/src/chat/tools/ToolFileLink.tsx index 2bfba67e0..7fe08b873 100644 --- a/apps/web/src/chat/tools/ToolFileLink.tsx +++ b/apps/web/src/chat/tools/ToolFileLink.tsx @@ -1,17 +1,5 @@ import { cn, isAbsolutePath, projectRelativePath } from "@/lib"; - -const URI_SCHEME = /^[A-Za-z][A-Za-z0-9+.-]*:/; - -export function toolFileTarget(path: string, workspaceRoot?: string | undefined): string | null { - const candidate = path.trim(); - if (!candidate) return null; - if (!isAbsolutePath(candidate) && URI_SCHEME.test(candidate)) return null; - const relative = projectRelativePath(candidate, workspaceRoot); - if (!relative || isAbsolutePath(relative) || relative === ".." || relative.startsWith("../")) { - return null; - } - return relative; -} +import { hasUriScheme, workspaceFileTarget } from "../fileTargets"; export function ToolFileLink({ path, @@ -31,10 +19,10 @@ export function ToolFileLink({ const candidate = path.trim(); const displayPath = label ?? - (candidate && !isAbsolutePath(candidate) && URI_SCHEME.test(candidate) + (candidate && !isAbsolutePath(candidate) && hasUriScheme(candidate) ? candidate : projectRelativePath(candidate, workspaceRoot)); - const target = disabled ? null : toolFileTarget(candidate, workspaceRoot); + const target = disabled ? null : workspaceFileTarget(candidate, workspaceRoot); if (!target || !onOpenFile) { return ( { - it("canonicalizes only paths contained by the active worktree", () => { - for (const [path, root, expected] of [ - ["module-a/../README.md", "/repo", "README.md"], - ["/repo/module-a/SPEC.md", "/repo", "module-a/SPEC.md"], - ["C:\\repo\\module-a\\SPEC.md", "C:\\repo", "module-a/SPEC.md"], - ["/repo-other/SPEC.md", "/repo", null], - ["../outside.md", "/repo", null], - ["https://example.com/a.md", "/repo", null], - ["file:///repo/a.md", "/repo", null], - ["", "/repo", null], - ] as const) { - expect(toolFileTarget(path, root)).toBe(expected); - } - }); - it("opens relative and in-worktree absolute read paths under one canonical relative label", () => { const relative = markup(ReadCard, "read", "module-a/../README.md"); const absolute = markup(ReadCard, "read", "/repo/module-a/SPEC.md"); diff --git a/apps/web/src/chat/turns.tsx b/apps/web/src/chat/turns.tsx index 3d79e7ad8..e74a4cdba 100644 --- a/apps/web/src/chat/turns.tsx +++ b/apps/web/src/chat/turns.tsx @@ -22,6 +22,7 @@ import { userText, } from "@/lib"; import { ActivityGroup } from "./ActivityGroup"; +import { AssistantMarkdown } from "./assistantLinks"; import { FileChip } from "./FileChip"; import { useFold, useSelection } from "./foldState"; import { Markdown } from "./Markdown"; @@ -91,7 +92,11 @@ export function ChatTurnView({ data-role="assistant" className="tr-text-reading text-text-default" > - + ); case "subagentCompletion": diff --git a/apps/web/src/lib/SPEC.md b/apps/web/src/lib/SPEC.md index 0ceda0112..8182a20f2 100644 --- a/apps/web/src/lib/SPEC.md +++ b/apps/web/src/lib/SPEC.md @@ -54,7 +54,8 @@ Tiny UI helpers shared across components. semantic resource key, so delimiters and stable noncanonical placement ids cannot split or alias identities. - **Public surface (barrel):** `cn`, `isMarkdownPath`, `stripFrontmatter`, `cssColorToHex`, `normalizePath`, `isAbsolutePath`, `projectRelativePath` (canonical worktree-relative POSIX identity; - collapses in-root `.`/`..` aliases but preserves an attempted leading escape for host rejection), + collapses in-root `.`/`..` aliases but preserves an attempted leading escape for host rejection; Windows + drive-rooted containment compares path/root case-insensitively while preserving the candidate's casing), `shallowEqualArrays`, `userText`, `parseSkillInvocation`, `matchesSkillInvocationCommand`, `relativeTime`, `platformShortcutLabel`, `hasPlatformModifier`, `copyText`, `randomId`, `DOUBLE_CLICK_SETTLE_MS`, `tupleKey`, `parseTupleKey`, `layoutResourceIdentity`, diff --git a/apps/web/src/lib/utils.test.ts b/apps/web/src/lib/utils.test.ts index c27ce6ad7..fdd323033 100644 --- a/apps/web/src/lib/utils.test.ts +++ b/apps/web/src/lib/utils.test.ts @@ -121,6 +121,10 @@ test("projectRelativePath yields the worktree-relative tab identity from every r expect(projectRelativePath("src/./nested/../foo.ts", root)).toBe("src/foo.ts"); expect(projectRelativePath("../outside.ts", root)).toBe("../outside.ts"); expect(projectRelativePath("C:\\wt\\ws\\src\\..\\foo.ts", "C:\\wt\\ws")).toBe("foo.ts"); + expect(projectRelativePath("c:/WT/ws/Src/Foo.ts", "C:/wt/WS")).toBe("Src/Foo.ts"); + expect(projectRelativePath("C:/Repository/foo.ts", "c:/repo")).toBe("C:/Repository/foo.ts"); + expect(projectRelativePath("D:/Repo/foo.ts", "c:/repo")).toBe("D:/Repo/foo.ts"); + expect(projectRelativePath("/WT/ws/src/foo.ts", root)).toBe("/WT/ws/src/foo.ts"); expect(projectRelativePath("/src/foo.ts", "/")).toBe("src/foo.ts"); expect(projectRelativePath("C:/src/foo.ts", "C:/")).toBe("src/foo.ts"); expect(projectRelativePath("/elsewhere/foo.ts", root)).toBe("/elsewhere/foo.ts"); diff --git a/apps/web/src/lib/utils.ts b/apps/web/src/lib/utils.ts index 45aa93b0f..b8ea21f10 100644 --- a/apps/web/src/lib/utils.ts +++ b/apps/web/src/lib/utils.ts @@ -94,9 +94,13 @@ export function normalizePath(path: string): string { return path.replaceAll("\\", "/").replace(/^\.\/+/, ""); } +function hasWindowsDriveRoot(path: string): boolean { + return /^[A-Za-z]:\//.test(normalizePath(path)); +} + export function isAbsolutePath(path: string): boolean { const normalized = normalizePath(path); - return normalized.startsWith("/") || /^[A-Za-z]:\//.test(normalized); + return normalized.startsWith("/") || hasWindowsDriveRoot(normalized); } export function shallowEqualArrays( @@ -142,8 +146,14 @@ export function projectRelativePath(path: string, workspaceRoot?: string | undef if (!canonical || !isAbsolutePath(canonical)) return canonical; const root = workspaceRoot ? trimTrailingSlashes(canonicalPosixPath(workspaceRoot)) : ""; - const rootPrefix = root.endsWith("/") ? root : `${root}/`; - if (root && (canonical === root || canonical.startsWith(rootPrefix))) { + const ignoreCase = hasWindowsDriveRoot(canonical) && hasWindowsDriveRoot(root); + const comparableCanonical = ignoreCase ? canonical.toLowerCase() : canonical; + const comparableRoot = ignoreCase ? root.toLowerCase() : root; + const rootPrefix = comparableRoot.endsWith("/") ? comparableRoot : `${comparableRoot}/`; + if ( + comparableRoot && + (comparableCanonical === comparableRoot || comparableCanonical.startsWith(rootPrefix)) + ) { return canonical.slice(root.length).replace(/^\/+/, "") || fileName(canonical); } diff --git a/e2e/tool-file-links.spec.ts b/e2e/tool-file-links.spec.ts index 5febd01d4..3c18f0d59 100644 --- a/e2e/tool-file-links.spec.ts +++ b/e2e/tool-file-links.spec.ts @@ -170,6 +170,52 @@ async function returnToChat(page: Page): Promise { await expect(page.getByTestId("chat-view")).toBeVisible(); } +test("assistant Markdown opens safe relative files without navigating the browser", async ({ + page, +}) => { + await openFixtureProject(page); + seedWorkspaceSession(repoCwd(), { + name: "assistant file links", + messages: [ + { role: "user", text: "show the evidence", timestamp: BASE_TS }, + { + role: "assistant", + text: [ + "[Open README](README.md)", + "[Open docs](https://example.com/docs)", + "[Outside workspace](../outside.md)", + ].join("\n\n"), + timestamp: BASE_TS + 1_000, + }, + ], + }); + + await expect(defaultWorkspaceRow(page)).toBeVisible(); + await enterDefaultWorkspace(page); + await expect(page.locator('[data-testid="editor-tab"][data-kind="chat"]')).toHaveCount(1); + + const message = page.locator('[data-testid="chat-message"][data-role="assistant"]').last(); + const local = message.getByRole("button", { name: "Open README" }); + await expect(local).toHaveAttribute("data-testid", "chat-file-link"); + await expect(local).toHaveAttribute("data-path", "README.md"); + await expect(local).not.toHaveAttribute("href", "README.md"); + await expect(local).not.toHaveAttribute("target", "_blank"); + await expect(message.getByRole("link", { name: "Open docs" })).toHaveAttribute( + "target", + "_blank", + ); + await expect(message.getByRole("link", { name: "Outside workspace" })).toHaveAttribute( + "target", + "_blank", + ); + + await local.click(); + const fileTab = page.locator('[data-testid="editor-tab"][data-kind="file"]'); + await expect(fileTab).toHaveCount(1); + await expect(fileTab).toContainText("README.md"); + await expect(fileTab).toHaveAttribute("data-preview", "true"); +}); + test("structured tool paths reuse the preview tab while rich tool results stay intentional", async ({ page, }) => {