fix(frontend): harden artifact and markdown rendering (#4117)

* fix(frontend): harden artifact and markdown rendering

* fix(#4117): restore allow-scripts for scroll-restore, fix citation XSS bypass

willem-bd identified these issues:

1. [REGRESSION] sandbox removed allow-scripts which broke HTML artifact
   scroll-restoration — the injected postMessage script could not run.
   The prior config (allow-scripts allow-forms without allow-same-origin,
   i.e. opaque origin) was already safe. Restored with corrected comment.

2. [XSS bypass] CitationLink rendered <a href={href}> directly before
   isSafeHref ran, so prompt-injected [citation:x](javascript:...)
   bypassed the safety check. Moved citation block after the guard.

Also:
- Added mailto: and tel: to SAFE_HREF_PROTOCOLS
- Kept anchor-only attributes off the <span> fallback (React DOM warning)

Note: the loadMessages re-throw originally in this commit was dropped
during the rebase — upstream #4065 rewrote useThreadHistory as a
TanStack useInfiniteQuery that already surfaces fetch failures.

* test(e2e): update sandbox assertion to match allow-scripts allow-forms

The artifact preview iframe's sandbox was restored to 'allow-scripts allow-forms'
for scroll-restoration. Update the E2E test to expect the corrected value.

* fix(#4117): address review — ArtifactLink XSS guard, urlOfArtifact sandbox e2e

- Apply the isSafeHref guard to ArtifactLink (artifact-link.tsx) so a
  prompt-injected javascript:/data: href in a .md artifact preview cannot
  reach a real anchor in the main document, matching the guard in
  createMarkdownLinkComponent.
- Add an e2e asserting the urlOfArtifact iframe (non-code
  browser-previewable files like PDF) keeps its empty sandbox.

Note: the loadMessages error-surfacing changes originally in this commit
were dropped during the rebase — upstream #4065 rewrote useThreadHistory
as a TanStack useInfiniteQuery whose queryFn already throws on
!response.ok and surfaces the failure via a toast.

* fix(frontend): let write_file non-code previewable artifacts render in sandboxed iframe

When a write_file artifact such as PDF is clicked in chat, the component receives a path that forced isCodeFile=true, hiding the sandboxed iframe behind the code editor. Now non-code browser-previewable files are detected early so the sandboxed iframe renders correctly.

Fixes the E2E test: renders sandboxed iframe for a browser-previewable non-code file.

* fix(#4117): align PDF artifact route pattern with convention (drop /mock/ prefix)

The test used /mock/api/threads/... for the PDF artifact content route,
but the urlOfArtifact helper generates /api/threads/... (without /mock/)
when isMock=false. The other tests (e.g. presented artifacts) already use
the correct /api/threads/... pattern.

* test(frontend): add render-level coverage for unsafe markdown/artifact links

- Render MarkdownLink and ArtifactLink via renderToStaticMarkup and
  assert an unsafe javascript: href produces a disabled <span> (never an
  <a>), including through the citation-labelled branch, and that safe
  https hrefs render hardened anchors (target=_blank, rel=noopener).
- The new ArtifactLink render test caught the unsafe href leaking onto
  the fallback <span> through the {...rest} spread; drop the spread in
  both span fallbacks so anchor-only attributes (href/target/rel) and
  react-markdown's node prop never reach the span.
- Cover mailto:/tel: in the isSafeHref unit cases and fix the stale
  SAFE_HREF_PROTOCOLS docstring that still claimed http/https-only.

* fix(frontend): route the agent save-hint through the safe localStorage facade

Export safeLocalStorage from core/settings/local and use it for the
agent-create save-hint read/write so blocked browser storage (Safari
private mode, strict containers, embedded WebViews) cannot throw from
the effect. core/agents/feature-cache.ts already guards its
localStorage access with try/catch, so settings + agent pages are now
consistently best-effort.

* fix(frontend): allow scheme-less relative markdown links in isSafeHref
This commit is contained in:
黄云龙 2026-07-21 11:35:44 +08:00 committed by GitHub
parent b565e6c0f0
commit 97ca7f88cf
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
9 changed files with 360 additions and 11 deletions

View File

@ -45,6 +45,7 @@ import {
type HumanInputResponse,
} from "@/core/messages/human-input";
import { isHiddenFromUIMessage } from "@/core/messages/utils";
import { safeLocalStorage } from "@/core/settings/local";
import { useThreadStream } from "@/core/threads/hooks";
import { uuid } from "@/core/utils/uuid";
import { isIMEComposing } from "@/lib/ime";
@ -130,11 +131,11 @@ export default function NewAgentPage() {
if (typeof window === "undefined" || step !== "chat") {
return;
}
if (window.localStorage.getItem(SAVE_HINT_STORAGE_KEY) === "1") {
if (safeLocalStorage.getItem(SAVE_HINT_STORAGE_KEY) === "1") {
return;
}
setShowSaveHint(true);
window.localStorage.setItem(SAVE_HINT_STORAGE_KEY, "1");
safeLocalStorage.setItem(SAVE_HINT_STORAGE_KEY, "1");
}, [step]);
const handleConfirmName = useCallback(async () => {

View File

@ -129,7 +129,13 @@ export function ArtifactFileDetail({
}, [filepath]);
const { isCodeFile, language } = useMemo(() => {
if (isWriteFile) {
let language = checkCodeFile(filepath).language;
const codeResult = checkCodeFile(filepath);
// Non-code browser-previewable files (PDF, images, audio, video)
// should render in the sandboxed iframe, not the code editor.
if (!codeResult.isCodeFile && canBrowserPreviewFile(filepath)) {
return codeResult;
}
let language = codeResult.language;
language ??= "text";
return { isCodeFile: true, language };
}
@ -360,6 +366,7 @@ export function ArtifactFileDetail({
{!isCodeFile && canPreviewInBrowser && (
<iframe
className="size-full"
sandbox=""
src={urlOfArtifact({ filepath, threadId, isMock })}
/>
)}
@ -528,6 +535,11 @@ export function ArtifactFilePreview({
ref={iframeRef}
className="size-full"
title="Artifact preview"
// allow-scripts is needed for the scroll-restoration injected
// script (appendHtmlPreviewScrollRestoration) which communicates
// via postMessage. allow-same-origin is deliberately omitted: the
// opaque origin prevents access to parent.document and cookies,
// and postMessage(..., "*") works fine from it.
sandbox="allow-scripts allow-forms"
src={htmlPreviewUrl}
/>

View File

@ -2,6 +2,8 @@ import type { AnchorHTMLAttributes } from "react";
import { cn } from "@/lib/utils";
import { isSafeHref } from "../messages/markdown-link";
import { CitationLink } from "./citation-link";
function isExternalUrl(href: string | undefined): boolean {
@ -10,6 +12,27 @@ function isExternalUrl(href: string | undefined): boolean {
/** Link renderer for artifact markdown: citation: prefix → CitationLink, otherwise underlined text. */
export function ArtifactLink(props: AnchorHTMLAttributes<HTMLAnchorElement>) {
// Reject unsafe schemes so prompt-injected [label](javascript:...) in a .md
// artifact preview cannot execute in the main document, matching the guard in
// createMarkdownLinkComponent (markdown-link.tsx).
if (props.href !== undefined && !isSafeHref(props.href)) {
// Intentionally no {...props} spread: anchor-only attributes (href,
// target, rel) are not valid on a <span> and would leak the unsafe URL
// into the DOM / trigger React DOM warnings.
const { className, children } = props;
return (
<span
className={cn(
"text-muted-foreground cursor-not-allowed underline decoration-dotted underline-offset-2",
className,
)}
aria-label="Unsafe link omitted"
title={`Unsafe link scheme in ${props.href}`}
>
{children}
</span>
);
}
if (typeof props.children === "string") {
const match = /^citation:(.+)$/.exec(props.children);
if (match) {

View File

@ -5,8 +5,55 @@ import { cn } from "@/lib/utils";
import { CitationLink } from "../citations/citation-link";
/**
* Schemes we are willing to render as a navigable ``<a href=...>``.
*
* Anything else (``javascript:``, ``data:text/html``, ``vbscript:``,
* ``file:``, ) is blocked because once it lands in a real anchor the
* browser happily executes the payload in the chat surface where
* sessionStorage / CSRF cookies are reachable. We accept ``http(s)``
* plus the non-executing ``mailto:`` / ``tel:`` schemes; same-origin
* absolute paths (``/``), scheme-less relative references (``report.md``,
* ``./``, ``../``), and in-document anchors (``#``) are also allowed
* they all resolve under the current HTTP(S) origin.
*/
const SAFE_HREF_PROTOCOLS = ["http:", "https:", "mailto:", "tel:"] as const;
export function isSafeHref(href: string | undefined): boolean {
if (typeof href !== "string" || href.length === 0) {
return false;
}
// Same-document anchors (e.g. "#section").
if (href.startsWith("#")) {
return true;
}
// Protocol-relative URLs (//evil.com/path) would inherit the current
// page's scheme and navigate to an unknown host. Also reject
// backslash-normalised variants (\\evil.com → //evil.com) because
// browsers treat backslash as a path separator in URLs.
if (/^(\/\/|\\\\)/.test(href)) {
return false;
}
// Parse the href so we can inspect its scheme. Items without an explicit
// protocol (report.md, ./report.md, ../assets/chart.png, /workspace/foo)
// resolve relative to the dummy HTTPS base, producing an https: URL.
// Explicitly-schemed URLs (https://…, mailto:, javascript:, data:, …)
// keep their native scheme, which we then check against the allowlist.
try {
const parsed = new URL(href, "https://dummy.example/");
return (SAFE_HREF_PROTOCOLS as ReadonlyArray<string>).includes(
parsed.protocol,
);
} catch {
return false;
}
}
function isExternalUrl(href: string | undefined): boolean {
return !!href && /^https?:\/\//.test(href);
if (typeof href !== "string") {
return false;
}
return /^https?:\/\//.test(href);
}
/**
@ -19,6 +66,31 @@ export function createMarkdownLinkComponent(threadId?: string) {
href,
...props
}: AnchorHTMLAttributes<HTMLAnchorElement>) {
// Reject unsafe schemes up front so a prompt-injected / pasted href can
// never reach the rendered anchor — including through the citation
// branch (which renders <a href={href}> directly). Check before the
// citation block so prompt-injected [citation:x](javascript:...) is
// blocked. Keep the visible label so the user can still see what the
// link claimed to point at.
if (href !== undefined && !isSafeHref(href)) {
// Intentionally no {...props} spread: react-markdown props like `node`
// (and anchor-only attributes such as target/rel) are not valid on a
// <span> and would trigger React DOM warnings.
const { className, children } = props;
return (
<span
className={cn(
"text-muted-foreground cursor-not-allowed underline decoration-dotted underline-offset-2",
className,
)}
aria-label="Unsafe link omitted"
title={`Unsafe link scheme in ${href}`}
>
{children}
</span>
);
}
// Safe-href check passed — citation links now route through CitationLink.
if (typeof props.children === "string") {
const match = /^citation:(.+)$/.exec(props.children);
if (match) {

View File

@ -23,6 +23,45 @@ function isBrowser(): boolean {
return typeof window !== "undefined";
}
/**
* Best-effort localStorage facade.
*
* Safari private mode, Firefox strict containers, some embedded WebViews, and
* quotas already filled by sibling tabs throw ``SecurityError`` or
* ``QuotaExceededError`` from ``getItem``/``setItem``. Without a guard those
* exceptions bubble into React render handlers and break the composer /
* settings panel. This wrapper traps every storage exception so callers can
* always fall back to a sane default.
*/
export const safeLocalStorage = {
getItem(key: string): string | null {
if (!isBrowser()) return null;
try {
return window.localStorage.getItem(key);
} catch {
return null;
}
},
setItem(key: string, value: string): boolean {
if (!isBrowser()) return false;
try {
window.localStorage.setItem(key, value);
return true;
} catch {
return false;
}
},
removeItem(key: string): boolean {
if (!isBrowser()) return false;
try {
window.localStorage.removeItem(key);
return true;
} catch {
return false;
}
},
};
export interface LocalSettings {
notification: {
enabled: boolean;
@ -72,7 +111,9 @@ export function getThreadModelName(threadId: string): string | undefined {
if (!isBrowser()) {
return undefined;
}
return localStorage.getItem(getThreadModelStorageKey(threadId)) ?? undefined;
return (
safeLocalStorage.getItem(getThreadModelStorageKey(threadId)) ?? undefined
);
}
export function saveThreadModelName(
@ -84,10 +125,10 @@ export function saveThreadModelName(
}
const key = getThreadModelStorageKey(threadId);
if (!modelName) {
localStorage.removeItem(key);
safeLocalStorage.removeItem(key);
return;
}
localStorage.setItem(key, modelName);
safeLocalStorage.setItem(key, modelName);
}
export function applyThreadModelOverride(
@ -110,7 +151,7 @@ export function getLocalSettings(): LocalSettings {
if (!isBrowser()) {
return DEFAULT_LOCAL_SETTINGS;
}
const json = localStorage.getItem(LOCAL_SETTINGS_KEY);
const json = safeLocalStorage.getItem(LOCAL_SETTINGS_KEY);
try {
if (json) {
const settings = JSON.parse(json) as Partial<LocalSettings>;
@ -124,5 +165,5 @@ export function saveLocalSettings(settings: LocalSettings) {
if (!isBrowser()) {
return;
}
localStorage.setItem(LOCAL_SETTINGS_KEY, JSON.stringify(settings));
safeLocalStorage.setItem(LOCAL_SETTINGS_KEY, JSON.stringify(settings));
}

View File

@ -6,12 +6,14 @@ const ARTIFACT_PATH = "/artifact-fixtures/report.html";
const MARKDOWN_ARTIFACT_PATH = "/artifact-fixtures/report.md";
const JSON_ARTIFACT_PATH = "/artifact-fixtures/report.json";
const PRESENTED_ARTIFACT_PATH = "/mnt/user-data/outputs/presented-report.md";
const PDF_ARTIFACT_PATH = "/artifact-fixtures/report.pdf";
const IN_PROGRESS_THREAD_ID = "00000000-0000-0000-0000-000000003119";
const COMPLETE_THREAD_ID = "00000000-0000-0000-0000-000000003120";
const MARKDOWN_THREAD_ID = "00000000-0000-0000-0000-000000003121";
const MARKDOWN_ANCHOR_THREAD_ID = "00000000-0000-0000-0000-000000003123";
const JSON_THREAD_ID = "00000000-0000-0000-0000-000000003122";
const PRESENTED_THREAD_ID = "00000000-0000-0000-0000-000000003123";
const PDF_THREAD_ID = "00000000-0000-0000-0000-000000003124";
function writeFileMessages({
path = ARTIFACT_PATH,
@ -109,6 +111,9 @@ test.describe("Artifact preview stability", () => {
await expect(
artifactsPanel.locator('iframe[title="Artifact preview"]'),
).toBeVisible();
await expect(
artifactsPanel.locator('iframe[title="Artifact preview"]'),
).toHaveAttribute("sandbox", "allow-scripts allow-forms");
});
test("renders preview iframe after the write artifact succeeds", async ({
@ -320,4 +325,44 @@ test.describe("Artifact preview stability", () => {
await presentedOption.click();
await expect(artifactsPanel.getByText("Presented Report")).toBeVisible();
});
test("renders sandboxed iframe for a browser-previewable non-code file (urlOfArtifact path)", async ({
page,
}) => {
mockLangGraphAPI(page, {
threads: [
{
thread_id: PDF_THREAD_ID,
title: "PDF artifact preview",
messages: writeFileMessages({
path: PDF_ARTIFACT_PATH,
content: "%PDF-fake-content",
}),
},
],
});
await page.route(
`**/api/threads/${PDF_THREAD_ID}/artifacts${PDF_ARTIFACT_PATH}`,
(route) =>
route.fulfill({
status: 200,
contentType: "application/pdf",
body: "%PDF-1.4 fake pdf",
}),
);
await page.goto(`/workspace/chats/${PDF_THREAD_ID}`);
await expect(page.getByText(PDF_ARTIFACT_PATH)).toBeVisible({
timeout: 15_000,
});
await page.getByText(PDF_ARTIFACT_PATH).click();
const artifactsPanel = page.locator("#artifacts");
await expect(artifactsPanel.getByText("report.pdf")).toBeVisible();
const urlOfArtifactIframe = artifactsPanel.locator("iframe:not([title])");
await expect(urlOfArtifactIframe).toBeVisible();
await expect(urlOfArtifactIframe).toHaveAttribute("sandbox", "");
});
});

View File

@ -0,0 +1,41 @@
import { describe, expect, it } from "@rstest/core";
import { createElement } from "react";
import { renderToStaticMarkup } from "react-dom/server";
import { ArtifactLink } from "@/components/workspace/citations/artifact-link";
// Render-level coverage for the .md artifact preview link renderer: a
// prompt-injected javascript:/data: href must never reach a real anchor in
// the main document (mirrors the MarkdownLink guard).
describe("ArtifactLink rendering", () => {
it("renders an unsafe href as a disabled span, never an anchor", () => {
const html = renderToStaticMarkup(
createElement(ArtifactLink, { href: "javascript:alert(1)" }, "click me"),
);
expect(html).not.toContain("<a");
expect(html).toContain("<span");
expect(html).toContain("click me");
expect(html).not.toContain("href=");
});
it("renders a safe https href as a hardened anchor", () => {
const html = renderToStaticMarkup(
createElement(ArtifactLink, { href: "https://example.com/x" }, "site"),
);
expect(html).toContain("<a");
expect(html).toContain('href="https://example.com/x"');
expect(html).toContain('target="_blank"');
expect(html).toContain('rel="noopener noreferrer"');
});
it("renders scheme-less relative hrefs as navigable anchors", () => {
const tests = ["report.md", "./report.md", "../assets/chart.png"];
for (const href of tests) {
const html = renderToStaticMarkup(
createElement(ArtifactLink, { href }, "doc"),
);
expect(html).toContain("<a");
expect(html).toContain(`href="${href}"`);
}
});
});

View File

@ -0,0 +1,91 @@
import { describe, expect, it } from "@rstest/core";
import { createElement } from "react";
import { renderToStaticMarkup } from "react-dom/server";
import {
createMarkdownLinkComponent,
isSafeHref,
} from "@/components/workspace/messages/markdown-link";
describe("isSafeHref", () => {
it("allows web URLs and same-origin paths", () => {
expect(isSafeHref("https://example.com/path")).toBe(true);
expect(isSafeHref("http://example.com/path")).toBe(true);
expect(isSafeHref("/workspace/chats/1")).toBe(true);
expect(isSafeHref("#section")).toBe(true);
});
it("allows scheme-less relative references", () => {
expect(isSafeHref("report.md")).toBe(true);
expect(isSafeHref("./report.md")).toBe(true);
expect(isSafeHref("../assets/chart.png")).toBe(true);
});
it("allows non-executing contact schemes", () => {
expect(isSafeHref("mailto:someone@example.com")).toBe(true);
expect(isSafeHref("tel:+15551234567")).toBe(true);
});
it("rejects executable, local, and protocol-relative URLs", () => {
expect(isSafeHref("javascript:alert(1)")).toBe(false);
expect(isSafeHref("data:text/html,<script>alert(1)</script>")).toBe(false);
expect(isSafeHref("file:///etc/passwd")).toBe(false);
expect(isSafeHref("//example.com/path")).toBe(false);
expect(isSafeHref("\\\\evil.com")).toBe(false);
expect(isSafeHref(undefined)).toBe(false);
});
});
// Render-level coverage: these tests exercise the component itself, so
// removing (or inverting) the isSafeHref guard inside MarkdownLink fails
// them even though isSafeHref stays untouched.
describe("MarkdownLink rendering", () => {
const MarkdownLink = createMarkdownLinkComponent();
it("renders an unsafe href as a disabled span, never an anchor", () => {
const html = renderToStaticMarkup(
createElement(MarkdownLink, { href: "javascript:alert(1)" }, "click me"),
);
expect(html).not.toContain("<a");
expect(html).toContain("<span");
expect(html).toContain("click me");
expect(html).not.toContain("href=");
});
it("blocks unsafe hrefs before the citation branch", () => {
const html = renderToStaticMarkup(
createElement(
MarkdownLink,
{ href: "javascript:alert(1)" },
"citation:evil",
),
);
expect(html).not.toContain("<a");
expect(html).not.toContain("href=");
});
it("renders a safe https href as a hardened anchor", () => {
const html = renderToStaticMarkup(
createElement(
MarkdownLink,
{ href: "https://example.com/report" },
"report",
),
);
expect(html).toContain("<a");
expect(html).toContain('href="https://example.com/report"');
expect(html).toContain('target="_blank"');
expect(html).toContain('rel="noopener noreferrer"');
});
it("renders a scheme-less relative href as a navigable anchor", () => {
const tests = ["report.md", "./report.md", "../assets/chart.png"];
for (const href of tests) {
const html = renderToStaticMarkup(
createElement(MarkdownLink, { href }, "doc"),
);
expect(html).toContain("<a");
expect(html).toContain(`href="${href}"`);
}
});
});

View File

@ -1,6 +1,16 @@
import { expect, test } from "@rstest/core";
import { afterEach, expect, rs, test } from "@rstest/core";
import { DEFAULT_LOCAL_SETTINGS } from "@/core/settings/local";
import {
DEFAULT_LOCAL_SETTINGS,
getLocalSettings,
getThreadModelName,
saveLocalSettings,
saveThreadModelName,
} from "@/core/settings/local";
afterEach(() => {
rs.unstubAllGlobals();
});
test("defaults token usage to header total plus per-turn breakdown", () => {
expect(DEFAULT_LOCAL_SETTINGS.tokenUsage).toEqual({
@ -8,3 +18,16 @@ test("defaults token usage to header total plus per-turn breakdown", () => {
inlineMode: "per_turn",
});
});
test("falls back when localStorage access is blocked", () => {
rs.stubGlobal("window", {
get localStorage() {
throw new DOMException("Blocked", "SecurityError");
},
});
expect(getLocalSettings()).toEqual(DEFAULT_LOCAL_SETTINGS);
expect(getThreadModelName("thread-1")).toBeUndefined();
expect(() => saveLocalSettings(DEFAULT_LOCAL_SETTINGS)).not.toThrow();
expect(() => saveThreadModelName("thread-1", "model-1")).not.toThrow();
});