Skip to content

Prevent XSS via unsafe href protocols in WebFetchToolCall #3403

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ThomasK33 merged 2 commits into from
May 28, 2026
+24 −8
Merged

Prevent XSS via unsafe href protocols in WebFetchToolCall #3403

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bc5e16e
fix: prevent XSS via javascript: URLs in WebFetchToolCall href
claude May 27, 2026
62ae660
fix: broaden isSafeHref comment to reflect full scope of blocked schemes
claude May 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 24 additions & 8 deletions src/browser/features/Tools/WebFetchToolCall.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,18 @@ interface NormalizedResult {
error?: string;
}

/**
* Allowlist http/https for clickable hrefs. Blocks javascript:, data:, vbscript:, etc.
*/
Comment thread
ThomasK33 marked this conversation as resolved.
function isSafeHref(url: string): boolean {

@coder-agents-review coder-agents-review Bot May 27, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 [CRF-2] This function duplicates the protocol allowlist already defined in browserUrl.ts (ALLOWED_BROWSER_URL_PROTOCOLS: new Set(["http:", "https:"])) and the same new URL().protocol check in normalizeBrowserUrl. The existing utility has full test coverage in browserUrl.test.ts covering javascript:, data:, file:, and vbscript:.

More importantly, the file-local placement means WorkspaceStatusIndicator.tsx:77 renders agentStatus.url as a clickable <a href> with no protocol validation. That URL flows from AgentStatusSchema (z.string().optional(), no protocol check) and carries the same threat model. In browser mode (no Electron guards), clicking a javascript: href there executes in the page context.

Extract a boolean predicate to browserUrl.ts that reuses ALLOWED_BROWSER_URL_PROTOCOLS:

export function isSafeHref(url: string): boolean {
  try {
    return ALLOWED_BROWSER_URL_PROTOCOLS.has(new URL(url).protocol);
  } catch {
    return false;
  }
}

Then apply it here and at WorkspaceStatusIndicator.tsx:77. The thin wrapper inherits existing test coverage; only the boolean interface needs a new test. GovernorSection.tsx:103 hand-rolls the same check and could also consume this. (Hisoka P2, Meruem P3, Robin P3, Nami, Gon, Leorio)

🤖

@coder-agents-review coder-agents-review Bot May 27, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3 [CRF-1] No test coverage for a security boundary function. The function is correct (verified by 7 reviewers against javascript:, data:, vbscript:, mixed-case, malformed URLs), but visual inspection is not regression protection. If CRF-2 is adopted and this moves to browserUrl.ts, the existing test suite covers the core logic and only the boolean wrapper needs a test. If it stays here, it is pure logic (no DOM, no React state) and testable with a plain vitest file. (Netero, Mafu-san)

🤖

@coder-agents-review coder-agents-review Bot May 27, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3 [CRF-3] The web_fetch tool schema at toolDefinitions.ts uses z.string().url().describe("The URL to fetch (http or https)"). The description promises http/https but z.string().url() accepts javascript:alert(1), data:text/html,..., and any valid URL scheme. A .refine() or .regex() enforcing http/https at the schema level would protect every consumer, with isSafeHref remaining as defense-in-depth at the render layer. (Pariston)

🤖

try {
const protocol = new URL(url).protocol;
return protocol === "http:" || protocol === "https:";
} catch {
return false;
}
}

/**
* Extract domain from URL for compact display
*/
Expand Down Expand Up @@ -144,14 +156,18 @@ export const WebFetchToolCall: React.FC<WebFetchToolCallProps> = ({
<div className="bg-code-bg flex flex-wrap gap-4 rounded px-2 py-1.5 text-[11px] leading-[1.4]">
<div className="flex min-w-0 gap-1.5">
<span className="text-secondary font-medium">URL:</span>
<a
href={args.url}
target="_blank"
rel="noopener noreferrer"
className="text-link font-monospace truncate hover:underline"
>
{args.url}
</a>
{isSafeHref(args.url) ? (
<a
href={args.url}
target="_blank"
rel="noopener noreferrer"
className="text-link font-monospace truncate hover:underline"
>
{args.url}
</a>
) : (
<span className="font-monospace truncate">{args.url}</span>
)}
</div>
{normalized?.success && normalized.title && (
<div className="flex min-w-0 gap-1.5">
Expand Down
Loading