Prevent XSS via unsafe href protocols in WebFetchToolCall#3403
Conversation
The web_fetch tool call component rendered args.url directly into an <a href=...> without protocol validation. Since Zod's z.string().url() accepts javascript: URLs, a prompt-injected tool call could produce a clickable link that executes arbitrary JS in the renderer. Add isSafeHref() that allowlists http:/https: protocols. Non-safe URLs render as plain text instead of clickable links. https://claude.ai/code/session_015vY8c55jcKPoPNKm2264uC
|
@codex review |
|
/coder-agents-review |
|
Chat: Review posted | View chat Review historydeep-review v0.5.0 | Round 2 | Last posted: Round 2, 4 findings (1 P2, 2 P3, 1 Nit), COMMENT. Review Finding inventoryFindings
Contested and acknowledged(none) Round logRound 1Panel (15 reviewers). 1 P2, 2 P3, 1 Nit. Reviewed against 5d79d9b..bc5e16e. Round 2BLOCKED. CRF-4 addressed (62ae660). CRF-1, CRF-2, CRF-3 silent. No review. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
|
Codex Review: Didn't find any major issues. 👍 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Correct fix. The allowlist approach, URL constructor normalization, and fail-closed error handling are all the right calls. Graceful degradation to plain text preserves readability without the click target. The commit message is excellent: names the vulnerability, vector, and component in one subject line.
Seven reviewers independently verified that isSafeHref correctly blocks javascript:, data:, vbscript:, blob:, file:, mixed-case, and malformed URLs. Kurapika confirmed that javascript: href execution is inline (not a navigation event), so the renderer-level fix is the correct primary defense even with Electron's setWindowOpenHandler in place.
The main concern is scope. Six reviewers converged on the same structural issue: isSafeHref duplicates the protocol allowlist already in browserUrl.ts, and its file-local placement leaves a verified sibling vulnerability at WorkspaceStatusIndicator.tsx:77 unprotected. Extracting the function resolves the duplication, the sibling, and the missing test coverage in one move.
1 P2 (extract to shared utility + sibling XSS), 2 P3 (test coverage, schema validation gap), 1 Nit (comment).
"A security utility that only one caller can reach is a security utility that rots." Hisoka
🤖 This review was automatically generated with Coder Agents.
| /** | ||
| * Only allow http/https URLs as clickable hrefs to prevent javascript: XSS. | ||
| */ | ||
| function isSafeHref(url: string): boolean { |
There was a problem hiding this comment.
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)
🤖
| /** | ||
| * Only allow http/https URLs as clickable hrefs to prevent javascript: XSS. | ||
| */ | ||
| function isSafeHref(url: string): boolean { |
There was a problem hiding this comment.
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)
🤖
| /** | ||
| * Only allow http/https URLs as clickable hrefs to prevent javascript: XSS. | ||
| */ | ||
| function isSafeHref(url: string): boolean { |
There was a problem hiding this comment.
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)
🤖
Address coder-agents-review nit: the comment was too narrow (only mentioned javascript:), but the function blocks all non-http(s) schemes. https://claude.ai/code/session_015vY8c55jcKPoPNKm2264uC
|
/coder-agents-review |
There was a problem hiding this comment.
CRF-4 (Nit, comment wording) was addressed in 62ae660. The updated comment now lists the full scope of blocked schemes.
Three findings remain silent with no author response or code change:
- CRF-2 (P2): Extract
isSafeHrefto sharedbrowserUrl.tsutility and apply atWorkspaceStatusIndicator.tsx:77, which has the same unprotectedhref={agentStatus.url}from model-controlled data. - CRF-1 (P3): Add test coverage for the security boundary function (resolved automatically if CRF-2 extraction is adopted).
- CRF-3 (P3): Add
.refine()to theweb_fetchtool schema to enforce http/https at the validation layer.
Further review is blocked until the author responds to these findings or pushes fixes. A reply acknowledging, contesting, or deferring each item (with a ticket for deferrals) unblocks the next round.
🤖 This review was automatically generated with Coder Agents.
2 participants
Summary
Add URL validation to prevent javascript: and other unsafe protocol XSS attacks when rendering fetched URLs as clickable links in the WebFetchToolCall component.
Changes
isSafeHref()function: Validates that URLs use only http: or https: protocols before rendering as clickable<a>tags. UsesURLconstructor for robust parsing and safely catches malformed URLs.<span>elements instead of clickable links, preventing potential XSS while still displaying the URL text.Implementation Details
The validation happens at render time in the URL display section. Safe URLs render as before with full link functionality; unsafe URLs (including malformed ones) degrade gracefully to plain text, maintaining readability without the security risk.
https://claude.ai/code/session_015vY8c55jcKPoPNKm2264uC