fix(frontend): gate tool-step links through the href scheme allowlist (#5526)

* fix(frontend): gate tool-step links through the href scheme allowlist

The chain-of-thought renderer turned web_fetch args and web_search /
image_search result URLs straight into <a href>. Markdown links already
pass isSafeHref, but these tool-step links bypassed it, so a
prompt-injected tool call could put file:, ms-msdt:, vscode: or other
OS protocol-handler links into the chat. React 19 only rewrites
javascript: hrefs.

All three sites now reuse the markdown allowlist and render an unsafe
URL as plain text (the image thumbnail stays, unlinked). Tests render
MessageGroup for each tool with unsafe schemes plus a web-URL control.

* docs(changelog): note tool-step link scheme gating (#5526)

* fix(frontend): mark omitted tool-step links and guard web_fetch url type

Review follow-up. Tool steps dropped an unsafe URL to bare text, while
markdown and artifact links show a dotted "Unsafe link omitted" span, so
the two surfaces applying the same rule degraded differently. That span
was already duplicated between markdown-link.tsx and artifact-link.tsx;
it is now one UnsafeLink component used by all three renderers. It
passes extra props through so the image tile still works as a Radix
tooltip trigger.

web_fetch also read args.url with a cast only. A non-string url (models
occasionally emit one mid-stream) reached JSX as an object and threw,
taking down the message list. It is now typeof-guarded.

* fix(frontend): default missing tool-call args before rendering steps

Review follow-up. The web_fetch typeof guard dropped the optional
chaining of the cast it replaced, so a tool call without an args object
threw again. Other branches were already exposed the same way: seven
tool kinds (web_fetch, web_search, image_search, read_file, write_file,
str_replace, browser_*) threw on a missing or null args while building
their labels. convertToSteps now defaults args to {} once, so every
ToolCall branch receives an object.
This commit is contained in:
Hyeonsang Cho 2026-09-18 17:35:16 +09:00 committed by GitHub
parent 57d027f903
commit d540be7e21
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
9 changed files with 274 additions and 55 deletions

View File

@ -2770,6 +2770,14 @@ This release closes that milestone with **765 merged pull requests**.
### Security
- **frontend:** Tool steps no longer turn non-web URLs into links. The
`web_fetch` URL and `web_search` / `image_search` result links in the
chain-of-thought panel skipped the scheme allowlist that markdown links use,
so a prompt-injected tool call could put a `file:` or OS protocol-handler
link (`ms-msdt:`, `vscode:`, …) into the chat. They now pass `isSafeHref`
and show an unsafe URL with the same "Unsafe link omitted" marker as
markdown links. A tool call whose args are missing, or whose `web_fetch` URL
is not a string, no longer crashes the message list. ([#5526])
- **skills:** Close gaps that let files skip SkillScan in the public skill
review gate. The review analyzer passed SkillScan only files it had decoded
as text, so executable binaries and nested archives were never checked; it
@ -4281,3 +4289,4 @@ with **180 merged pull requests** since the first 2.0 milestone tag.
[#5504]: https://github.com/bytedance/deer-flow/pull/5504
[#5505]: https://github.com/bytedance/deer-flow/pull/5505
[#5524]: https://github.com/bytedance/deer-flow/pull/5524
[#5526]: https://github.com/bytedance/deer-flow/pull/5526

View File

@ -2085,6 +2085,12 @@
### 安全
- **前端:** 工具步骤不再把非 Web URL 渲染为链接。思维链面板中的 `web_fetch` URL 与
`web_search` / `image_search` 结果链接此前绕过了 Markdown 链接使用的协议白名单,
被提示注入的工具调用可在聊天中放入 `file:` 或系统协议处理程序链接(`ms-msdt:`
`vscode:` 等)。现在它们会经过 `isSafeHref`,不安全的 URL 与 Markdown 链接一样显示
“Unsafe link omitted” 标记;缺少 args 的工具调用或非字符串的 `web_fetch` URL 也不再导致
消息列表崩溃。([#5526])
- **技能:** 修复公共技能审查门禁中文件可绕过 SkillScan 的缺口。审查分析器此前只把解码为
文本的文件交给 SkillScan可执行二进制文件和嵌套压缩包从未被检查豁免了任意层级
`evals/fixtures/` 目录下的所有文件;重复的压缩包成员或仅大小写不同的文件名会在扫描前静默
@ -3492,3 +3498,4 @@ DeerFlow 2.0 是围绕"超级智能体"框架的彻底重写,核心包含子
[#5504]: https://github.com/bytedance/deer-flow/pull/5504
[#5505]: https://github.com/bytedance/deer-flow/pull/5505
[#5524]: https://github.com/bytedance/deer-flow/pull/5524
[#5526]: https://github.com/bytedance/deer-flow/pull/5526

View File

@ -144,6 +144,7 @@ Array previews coalesce consecutive generated markers only at the end into one o
- **SSE replay gaps** are handled in `core/api/api-client.ts`, which wraps both initial and joined run streams because the upstream SDK ignores unknown event names. An id-less backend `gap` control frame clears stale reconnect metadata, emits an internal `stream_replay_gap` custom event, reloads durable thread values, and resumes after the server-provided retained tail when one exists (or rejoins without a cursor if the buffer is empty), with up to five recovery rejoins after the original stream (six total stream calls on an all-gap exhaustion path). The wrapper remains a lazy async iterable because the SDK consumes it with `for await`. `core/threads/hooks.ts` clears optimistic/transient/subtask state, invalidates durable history caches, and shows the localized recovery warning; never let a gap fall through as a normal stream finish or cancel the still-running backend run.
- **Streaming Markdown rendering** is owned by `core/streamdown`: Streamdown's `animated` / `isAnimating` API handles incremental word animation, while the shared `streamdownRenderingPlugins` config registers the named code-highlighting and Mermaid plugins required by Streamdown 2.5. Keep wrappers and derived configs wired to that shared object; do not reintroduce a rehype plugin that wraps every word, because reparsing a growing block remounts old words and replays their animation.
- Citation links in message and artifact Markdown must derive their `citation:` label from the full `ReactNode` children tree, since Streamdown may provide element or array children during streaming rather than a plain string.
- Tool-step links in `message-group.tsx` (`web_fetch` args, `web_search` / `image_search` result URLs) are model- or provider-controlled, so they pass the markdown `isSafeHref` allowlist; every surface renders a rejected href through the shared `UnsafeLink` marker.
- **Environment validation** uses `@t3-oss/env-nextjs` with Zod schemas (`src/env.js`). Skip with `SKIP_ENV_VALIDATION=1`
- **Subtask step history and runtime metadata** (`core/tasks/`) — the subtask card shows a subagent's full step timeline (#3779): its assistant reasoning turns interleaved with the tools it ran. The task tool's model-visible `description` is an optional progress label; `MessageList` uses the required `prompt` (then the localized generic subtask label) when a provider omits it, so a valid task call never renders a blank card title. `Subtask.steps[]` is accumulated live from `task_running` events (appended via `mergeSteps`, not overwritten) and backfilled on expand for historical runs by `fetchSubtaskSteps`, which pages the events endpoint scoped to one task (GET `/runs/{runId}/events?event_types=subagent.step&task_id=…&after_seq=…`) until a short page, so the run-wide limit can't truncate the timeline. `task_started` carries the effective `model_name`; `task_running` carries a cumulative usage snapshot after each completed LLM call. `core/tasks/lifecycle.ts` normalizes these additive events, and `computeNextSubtask` keeps the largest cumulative total so replayed or late SSE frames cannot double-count or roll the folded card backward. Terminal ToolMessage metadata (`subagent_model_name` / `subagent_token_usage`) restores the same values from normal history after reload; no per-card event fetch is needed. `core/tasks/steps.ts` is the pure step model: `messageToStep` (live), `eventsToSteps` (reload), `mergeSteps` (dedup by `message_index`), and `stepsForDisplay` (what the card renders — keeps tool steps + AI steps with text, drops the trailing final-answer AI step when completed since it's shown as `result`). `core/tasks/context.tsx`'s `useUpdateSubtask` applies updates against a `tasksRef` mirroring the latest state (not a closure snapshot), so a late-resolving `fetchSubtaskSteps` backfill merges into current state instead of clobbering SSE steps or sibling subtasks that arrived meanwhile. The owning `run_id` is carried onto history content messages in `buildVisibleHistoryMessages` so the card can resolve the events endpoint.

View File

@ -2,7 +2,7 @@ import type { AnchorHTMLAttributes } from "react";
import { cn } from "@/lib/utils";
import { isSafeHref } from "../messages/markdown-link";
import { isSafeHref, UnsafeLink } from "../messages/markdown-link";
import { CitationLink, extractReactNodeText } from "./citation-link";
@ -21,16 +21,9 @@ export function ArtifactLink(props: AnchorHTMLAttributes<HTMLAnchorElement>) {
// 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}`}
>
<UnsafeLink href={props.href} className={className}>
{children}
</span>
</UnsafeLink>
);
}
const childrenText = extractReactNodeText(props.children);

View File

@ -1,4 +1,4 @@
import type { AnchorHTMLAttributes } from "react";
import type { AnchorHTMLAttributes, ComponentProps } from "react";
import { resolveMarkdownArtifactURL } from "@/core/artifacts/utils";
import { cn } from "@/lib/utils";
@ -49,6 +49,30 @@ export function isSafeHref(href: string | undefined): boolean {
}
}
/**
* Inert stand-in for a link whose href failed `isSafeHref`. It keeps the
* visible label and marks the omission on hover and for assistive tech, so
* every surface that applies the allowlist degrades the same way. Extra props
* pass through for wrappers such as Radix `asChild` triggers.
*/
export function UnsafeLink({
href,
className,
...props
}: ComponentProps<"span"> & { href: string }) {
return (
<span
{...props}
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}`}
/>
);
}
function isExternalUrl(href: string | undefined): boolean {
if (typeof href !== "string") {
return false;
@ -78,16 +102,9 @@ export function createMarkdownLinkComponent(threadId?: string) {
// <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}`}
>
<UnsafeLink href={href} className={className}>
{children}
</span>
</UnsafeLink>
);
}
// Safe-href check passed — citation links now route through CitationLink.

View File

@ -48,6 +48,7 @@ import { FlipDisplay } from "../flip-display";
import { Tooltip } from "../tooltip";
import { MarkdownContent } from "./markdown-content";
import { isSafeHref, UnsafeLink } from "./markdown-link";
import { ToolCallDetails } from "./tool-call-details";
interface MessageGroupProps {
@ -759,11 +760,18 @@ function ToolCall({
>
{Array.isArray(result) && (
<ChainOfThoughtSearchResults>
{/* Tool args and results are model- or provider-controlled, so
every tool link passes the same scheme allowlist as markdown
links and degrades to the same UnsafeLink marker. */}
{result.map((item) => (
<ChainOfThoughtSearchResult key={item.url}>
<a href={item.url} target="_blank" rel="noopener noreferrer">
{item.title}
</a>
{isSafeHref(item.url) ? (
<a href={item.url} target="_blank" rel="noopener noreferrer">
{item.title}
</a>
) : (
<UnsafeLink href={item.url}>{item.title}</UnsafeLink>
)}
</ChainOfThoughtSearchResult>
))}
</ChainOfThoughtSearchResults>
@ -794,32 +802,48 @@ function ToolCall({
{Array.isArray(results) && (
<ChainOfThoughtSearchResults>
{Array.isArray(results) &&
results.map((item) => (
<Tooltip key={item.image_url} content={item.title}>
<a
className="size-24 overflow-hidden rounded-lg object-cover"
href={item.source_url}
target="_blank"
rel="noopener noreferrer"
>
<div className="bg-accent size-24">
<img
className="size-full object-cover"
src={item.thumbnail_url}
alt={item.title}
width={100}
height={100}
/>
</div>
</a>
</Tooltip>
))}
results.map((item) => {
const thumbnail = (
<div className="bg-accent size-24">
<img
className="size-full object-cover"
src={item.thumbnail_url}
alt={item.title}
width={100}
height={100}
/>
</div>
);
return (
<Tooltip key={item.image_url} content={item.title}>
{isSafeHref(item.source_url) ? (
<a
className="size-24 overflow-hidden rounded-lg object-cover"
href={item.source_url}
target="_blank"
rel="noopener noreferrer"
>
{thumbnail}
</a>
) : (
<UnsafeLink
href={item.source_url}
className="size-24 overflow-hidden rounded-lg"
>
{thumbnail}
</UnsafeLink>
)}
</Tooltip>
);
})}
</ChainOfThoughtSearchResults>
)}
</ChainOfThoughtStep>
);
} else if (kind === "web_fetch") {
const url = (args as { url: string })?.url;
// Models occasionally emit non-string args mid-stream; an object here
// would reach the JSX below and throw.
const url = typeof args.url === "string" ? args.url : undefined;
let title = url;
if (typeof result === "string") {
const potentialTitle = extractTitleFromMarkdown(result);
@ -834,16 +858,19 @@ function ToolCall({
icon={GlobeIcon}
>
<ChainOfThoughtSearchResult>
{url && (
<a
href={url}
target="_blank"
rel="noopener noreferrer"
className="cursor-pointer"
>
{title}
</a>
)}
{url &&
(isSafeHref(url) ? (
<a
href={url}
target="_blank"
rel="noopener noreferrer"
className="cursor-pointer"
>
{title}
</a>
) : (
<UnsafeLink href={url}>{title}</UnsafeLink>
))}
</ChainOfThoughtSearchResult>
</ChainOfThoughtStep>
);
@ -1089,7 +1116,9 @@ function convertToSteps(messages: Message[]): CoTStep[] {
messageId: message.id,
type: "toolCall",
name: tool_call.name,
args: tool_call.args,
// Persisted or mid-stream tool calls can omit args (or send null);
// every ToolCall branch reads them, so normalize once here.
args: tool_call.args ?? {},
};
const toolCallId = tool_call.id;
if (toolCallId) {

View File

@ -16,6 +16,7 @@ describe("ArtifactLink rendering", () => {
expect(html).toContain("<span");
expect(html).toContain("click me");
expect(html).not.toContain("href=");
expect(html).toContain('aria-label="Unsafe link omitted"');
});
it("renders a safe https href as a hardened anchor", () => {

View File

@ -50,6 +50,7 @@ describe("MarkdownLink rendering", () => {
expect(html).toContain("<span");
expect(html).toContain("click me");
expect(html).not.toContain("href=");
expect(html).toContain('aria-label="Unsafe link omitted"');
});
it("blocks unsafe hrefs before the citation branch", () => {

View File

@ -516,6 +516,167 @@ describe("MessageGroup", () => {
});
});
// Tool args come from the model and results from search providers, so a
// prompt-injected URL must not become a navigable anchor. React only rewrites
// javascript: hrefs; local and OS-handler schemes would otherwise pass through.
// A blocked URL keeps the markdown path's "Unsafe link omitted" marker.
describe("MessageGroup tool links", () => {
const unsafeUrls = [
"javascript:alert(1)",
"file:///etc/passwd",
"ms-msdt:/id PCWDiagnostic",
"vscode://file/etc/passwd",
];
it.each(unsafeUrls)("marks a web_fetch URL of %s as omitted", (url) => {
const html = renderToolCall("web_fetch", { url });
expect(html).toContain(`>${url}</span>`);
expect(html).toContain(`title="Unsafe link scheme in ${url}"`);
expect(unsafeMarkerCount(html)).toBe(1);
expect(html).not.toContain("<a");
});
it.each(unsafeUrls)("marks a web_search result at %s as omitted", (url) => {
const html = renderToolCall(
"web_search",
{ query: "DeerFlow" },
JSON.stringify([
{ title: "Safe source", url: "https://safe.example" },
{ title: "Injected source", url },
]),
);
expect(html).toContain('href="https://safe.example"');
expect(html).toContain(">Injected source</span>");
expect(unsafeMarkerCount(html)).toBe(1);
expect(anchorCount(html)).toBe(1);
});
it.each(unsafeUrls)(
"marks an image_search source at %s as omitted",
(url) => {
const html = renderToolCall(
"image_search",
{ query: "DeerFlow" },
JSON.stringify({
results: [
{
title: "Injected image",
source_url: url,
thumbnail_url: "https://images.example/thumb.png",
image_url: "https://images.example/full.png",
},
],
}),
);
expect(html).toContain('src="https://images.example/thumb.png"');
expect(unsafeMarkerCount(html)).toBe(1);
expect(html).not.toContain("<a");
},
);
it("keeps linking web_fetch and image_search results with web URLs", () => {
const fetchHtml = renderToolCall("web_fetch", {
url: "https://example.com/page",
});
const imageHtml = renderToolCall(
"image_search",
{ query: "DeerFlow" },
JSON.stringify({
results: [
{
title: "Image",
source_url: "https://example.com/source",
thumbnail_url: "https://images.example/thumb.png",
image_url: "https://images.example/full.png",
},
],
}),
);
expect(fetchHtml).toContain('href="https://example.com/page"');
expect(imageHtml).toContain('href="https://example.com/source"');
expect(unsafeMarkerCount(fetchHtml + imageHtml)).toBe(0);
});
// Models occasionally emit non-string args, and the step renders mid-stream;
// an object reaching the JSX would throw and take down the message list.
it("renders a web_fetch step whose url arg is not a string", () => {
const html = renderToolCall("web_fetch", {
url: { href: "https://example.com/page" },
});
expect(html).toContain("View web page");
expect(html).not.toContain("<a");
});
// Persisted or mid-stream tool calls can arrive without an args object;
// every specialized branch reads args, so none may throw on its absence.
it.each([
"web_fetch",
"web_search",
"image_search",
"ls",
"read_file",
"write_file",
"str_replace",
"bash",
"ask_clarification",
"write_todos",
"browser_navigate",
"mcp_lookup",
])("renders a %s step whose tool call has no args", (name) => {
for (const args of [undefined, null]) {
const render = () =>
renderGroup([
{
id: "ai-1",
type: "ai",
content: "",
tool_calls: [{ id: "call-1", name, args }],
} as unknown as Message,
]);
expect(render).not.toThrow();
}
});
});
function renderToolCall(
name: string,
args: Record<string, unknown>,
content?: string,
) {
const messages: Message[] = [
{
id: "ai-1",
type: "ai",
content: "",
tool_calls: [{ id: "call-1", name, args }],
} as Message,
];
if (content !== undefined) {
messages.push({
id: "tool-1",
type: "tool",
name,
tool_call_id: "call-1",
content,
} as Message);
}
return renderGroup(messages);
}
function unsafeMarkerCount(html: string) {
return html.split('aria-label="Unsafe link omitted"').length - 1;
}
function anchorCount(html: string) {
return html.match(/<a\s/g)?.length ?? 0;
}
/** Asserts every needle is present and that they appear in the given order. */
function expectRenderedInOrder(html: string, needles: string[]) {
const indices = needles.map((needle) => html.indexOf(needle));