fix(core): accept contained symlinks in the older-Git archive fallback - #9969
Conversation
The public GitHub archive fallback rejected every tar link entry, so a repository carrying an ordinary in-repo symlink could not install on Git older than 2.37. The reported repro repository, obra/superpowers, ships a root `AGENTS.md -> CLAUDE.md`, and GitHub codeload archives preserve it. Accept symbolic-link entries whose targets provably resolve inside the archive root, and keep refusing everything else: absolute targets, Windows drive-qualified targets, `..` escapes, and hard links. A hard link names another archive entry rather than a path on disk, so it needs a different containment argument than the one made here and stays unsupported. Containment is decided from the archive's own entry paths before extraction begins, so no link is followed and nothing is written while deciding. The carve-out is opt-in and enabled only for the public archive fallback, matching how the resource limits were gated; local, npm, and release archives keep their previous fail-closed behavior. Renames `assertTarArchiveHasNoLinks` to `assertTarArchiveLinksAreSafe`, which no longer describes a blanket ban. Fixes QwenLM#9724 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Re-run on the current head (triggered by Thanks for the PR! Template looks good ✓ Problem: observed, with evidence. Linked issue #9724 reproduces the failure installing Direction: aligned. This completes what the older-Git fallback (merged via #9690) set out to do; the issue thread on that fallback prescribed the safe-symlink carve-out as the missing piece, and #9724 was filed to track exactly this. The area is security-sensitive (archive extraction), which is precisely why the change stays opt-in and fail-closed everywhere else — the review focused there. Size: touches Approach: the scope still feels right — containment decided from entry paths before anything is written, opt-in only on the public-archive fallback, hard links rejected everywhere. The five follow-up rounds added defense in depth rather than scope: strict tar extraction, a post-flatten layout re-check, a relocation re-validation guard in the extension manager, an 8 MiB path-metadata budget, and duplicate-entry rejection. Every layer fails closed. No unrelated changes spotted. Risk: no elevated risk signals — no high-risk paths matched. The multi-entry tar-slip question from the first pass is now covered by the PR's own tests rather than only by the pinned tar 7.5.22 backstop; details in Stage 2. Moving on to code review. 🔍 中文说明应 感谢贡献！ 模板完整 ✓ 问题：已观测到，且有证据。关联 issue #9724 复现了在 Ubuntu 22.04（Git 2.34.1）上安装 方向：对齐。这补全了旧版 Git 回退（#9690 已合并）的目标；那个回退的 issue 讨论中就已提出"安全符号链接豁免"是缺失的一环，#9724 正是为此而建。该领域对安全敏感（归档解压），因此改动保持默认关闭、其余路径全部失败关闭——代码审查重点看这里。 规模：触及 方案：范围依然合理——在写入任何文件之前仅凭条目路径判定 containment、仅在公共归档回退路径默认开启、硬链接一律拒绝。后续五轮评审增加的是纵深防御而非范围扩张：严格模式解压、解压后扁平化布局复检、扩展管理器中的迁移后重校验守卫、8 MiB 路径元数据上限、重复条目拒绝。每一层都失败关闭。未发现无关改动。 风险：无升级风险信号——未命中高风险路径。第一轮提出的多条目 tar-slip 问题现在由 PR 自带测试覆盖，而不再只依赖锁定的 tar 7.5.22 兜底；详见 Stage 2。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code review (cumulative, on the current head)The question this PR has to answer is unchanged — does accepting contained symlinks keep extraction safe — and the answer now rests on four layers instead of one. I re-read the whole cumulative diff. Layer 1 — pre-extraction scan. Layer 2 — strict extraction. Layer 3 — post-flatten layout check. This is the round-2 concern resolved: flatten moves entries, and a link contained pre-flatten can escape after the wrapper directory is stripped. Layer 4 — relocation re-validation. Blast radius is unchanged and mechanically verifiable: Non-blocking, recorded: the round-5/6 review deferrals remain open as documented — the CI evidenceAll checks on the reviewed commit are completed — nothing pending.
Sandboxed verification would settle the one claim CI cannot: that a live Real-scenario testingN/A — unattended run, and there is no user-visible TUI surface here: the change is extension-install internals, exercised end-to-end by the CI tests above. 中文说明代码审查（累计，基于当前 head）这个 PR 必须回答的问题没有变——接受归档内符号链接后解压是否仍然安全——而现在的答案建立在四层防御上，不再只有一层。我重新通读了完整的累计 diff。 第一层——解压前扫描。 第二层——严格解压。 该路径上 第三层——扁平化后布局复检。 这正是第 2 轮顾虑的解决方案：扁平化会移动条目，解压前被包含的链接在剥掉包装目录后可能逃逸。 第四层——迁移后重校验。 影响面不变且可机械验证： 非阻塞，记录在案： 第 5/6 轮评审的延后项仍按记录保留——扁平化后检查的 CI 证据被审提交上的所有检查均已完成——无 pending。 （CI 表格见上方标记区域，由 finalize 工作流维护。） 沙盒验证可以补齐 CI 无法覆盖的唯一论断：线上 真实场景测试N/A——无人值守运行，且本改动没有用户可见的 TUI 界面：属于扩展安装内部逻辑，已由上述 CI 测试端到端覆盖。 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — a real, reproduced problem fixed with layered, fail-closed defenses that I verified end to end; what remains are test-pinning gaps and documented known limitations, none of which open an escape path. Going back to the proposal I wrote before first reading this diff — an opt-in contained-symlink carve-out decided from entry paths, fail-closed everywhere else, hard links always rejected — the PR is still essentially that, and I found no simpler path it missed. (Materializing links as file copies remains rejected from the issue thread: it would diverge from what the Git ≥ 2.37 clone path installs.) The five review rounds since then added depth, not scope: strict extraction, a post-flatten re-check, a relocation re-validation guard, a path-metadata budget, duplicate-entry rejection. Each one fails closed, and the review loop has converged — the last pass found nothing at the critical floor. What convinced me on this re-run: I checked every layer myself rather than trusting the round summaries. The post-flatten escape hazard (a link contained before the wrapper is stripped but escaping after) is now pinned by a dedicated test through the real fallback chain, including the wrapped error message. The round-5 gaps are genuinely fixed — the mock passthrough survives Reservations, non-blocking and named so they aren't lost: the deferred round-5/6 probes (unpinned Approving, pinned to the reviewed commit. 中文说明置信度：4/5 —— 一个真实且有复现的问题，用分层的、失败关闭的防御修复，我已逐层验证；剩余的是测试钉扎缺口与已记录的已知局限，均不构成逃逸路径。 回到我第一次读 diff 之前写下的方案——默认关闭的归档内符号链接豁免、仅凭条目路径判定、其余路径全部失败关闭、硬链接一律拒绝——这个 PR 依然是那个方案，我也没找到它遗漏的更简路径。（"把链接实体化为文件拷贝"已在 issue 讨论中被否决：它会与 Git ≥ 2.37 克隆路径的安装结果产生分歧。）此后的五轮评审增加的是纵深而非范围：严格解压、扁平化后复检、迁移后重校验守卫、路径元数据上限、重复条目拒绝。每一层都失败关闭，且评审循环已经收敛——最后一轮在 critical 门槛上没有新发现。 这次重跑说服我的点：每一层都是我自己核过的，不是转述各轮结论。扁平化后逃逸隐患（链接在剥掉包装目录前被包含、之后逃逸）现在由专门测试经由真实回退链路钉住，包括包装后的报错信息。第 5 轮的缺口是真正修复了——mock 透传在 保留意见（非阻塞，写出来以免丢失）：第 5/6 轮延后的探针项（未钉扎的 批准，钉在被审提交上。 — 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.
LGTM, looks ready to ship — CI landed green after the review. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Test Plan (not a blocker): 168 passed — this review observed 21257, 1689, 23739, 1658, 601, 4227, 626 passed; 23736 passed — this review observed 21257, 1689, 23739, 1658, 601, 4227, 626 passed.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
Test Plan（非阻断）：168 passed — this review observed 21257, 1689, 23739, 1658, 601, 4227, 626 passed; 23736 passed — this review observed 21257, 1689, 23739, 1658, 601, 4227, 626 passed。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const containingDirectory = path.posix.dirname(entryPath); | ||
| const resolved = path.posix.normalize( |
There was a problem hiding this comment.
[Critical] R1-1: Containment is judged against entry paths that include the codeload top-level repo-<sha>/ wrapper directory, but flattenSingleExtensionDirectory (github.ts) removes that directory after extraction without rewriting symlink targets. A link with a single ../ target is therefore accepted as contained pre-flatten and points OUTSIDE the extraction destination post-flatten — the exact case issue #9724 says must stay rejected ("relative link targets that resolve inside the extraction destination"). A hostile public repo shipping evil-link -> ../pwn passes the scan; after flatten, the installed tree holds a link resolving outside the destination, and since copyExtension copies with dereference: true, a planted neighbor file in the world-writable temp directory is ingested into the installed extension. The new escape tests use ../../etc/hosts, which escapes both roots, so this single-../ band is not covered. Re-validate link containment against the flattened tree before the extension is consumed (the scan-side strip-one-component variant over-rejects legitimate wrapper-less nested links), and add a fallback-path regression test for a repo-root symlink with a single ../ target.
Probe through the real extractArchiveFile at ad75c928c:
extractArchiveFile error: null
symlink: {"isSymbolicLink":true,"target":"../pwn","resolvesInsideDest":false}
dereferenced copy content: "PLANTED-CONTENT"
The probe flips to Error: Tar archive contains unsupported link entry: evil-link when a post-flatten re-check is added, with all legitimate symlink tests still passing.
中文说明
containment 判断所依据的条目路径包含了 codeload 归档顶层的 repo-<sha>/ 包装目录，但 flattenSingleExtensionDirectory（github.ts）在解压后会移除该目录，且不会重写符号链接的目标。因此，只带一层 ../ 的目标在 flatten 之前会被判定为“在归档内”而被接受，而在 flatten 之后就指向了解压目标目录之外——这正是 issue #9724 明确要求继续拒绝的情形（“相对链接目标必须解析到解压目标目录内部”）。攻击者只需发布一个带有 evil-link -> ../pwn 根符号链接的公开仓库即可通过扫描；flatten 之后，安装目录中会出现一个解析到目标目录之外的链接，而 copyExtension 以 dereference: true 复制，位于可写临时目录中的、攻击者可预先布置的邻居文件内容会被读进已安装的扩展。新增的逃逸测试使用的是 ../../etc/hosts（同时逃出两层根目录），因此覆盖不到这层只带一个 ../ 的区间。建议在 flatten 之后、扩展被使用之前，对扁平化后的目录树重新校验链接的 containment（在扫描端“剥掉一层路径分量”的变体会过度拒绝不带包装目录的合法嵌套链接），并为仓库根目录带单个 ../ 目标的符号链接补充回退路径回归测试。
探针（针对真实的 extractArchiveFile，提交 ad75c92）：
extractArchiveFile error: null
symlink: {"isSymbolicLink":true,"target":"../pwn","resolvesInsideDest":false}
dereferenced copy content: "PLANTED-CONTENT"
加入 flatten 后的重新校验后，探针翻转为 Error: Tar archive contains unsupported link entry: evil-link，且所有合法符号链接测试仍然通过。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // A drive-qualified or backslash-rooted target is absolute on Windows even | ||
| // though `path.posix.isAbsolute` reads it as relative. Tar paths are always | ||
| // posix-separated, so this is checked explicitly rather than via `path`. | ||
| const WINDOWS_ABSOLUTE_LINK_TARGET = /^(?:[a-zA-Z]:|\\)/; |
There was a problem hiding this comment.
[Critical] R1-2: isContainedSymlinkTarget models containment purely with path.posix, where \ is an ordinary filename character, and this regex only catches drive-qualified or backslash-ROOTED targets — so backslash-separated traversal targets (..\..\x) are classified "contained" even though they escape on Windows. Today the escape is stopped only by node-tar's own STRIPABSOLUTEPATH check, which silently SKIPS the entry instead of rejecting the archive: the extension installs incomplete rather than failing closed, and the documented "refused before extraction" guarantee does not hold for this input class. Any future change that bypasses that library check (a different extractor, preservePaths, a tar behavior change) turns the accepted entry into a real path traversal. All new symlink tests skip win32 and none covers backslash targets, so CI cannot see this divergence. Normalize separators before judging containment — this only ever adds rejections, so it is fail-closed on both platforms:
const normalizedLinkPath = linkPath.replaceAll('\\', '/');
// run the existing absolute/containment checks against normalizedLinkPathand add a regression test asserting rejection of a ..\..\-style target.
Probe at ad75c928c:
gate error (null = gate ACCEPTED backslash target): null
extractFile error (null = install reported success): null
dest entries after extract: []
tar warnings: ["TAR_ENTRY_ERROR: linkpath escapes extraction directory"]
中文说明
isContainedSymlinkTarget 完全基于 path.posix 建模 containment，而在 posix 语义下 \ 只是普通文件名字符；上面的正则只能匹配带盘符或以反斜杠开头的目标——因此以反斜杠分隔的穿越目标（..\..\x）会被判定为“在归档内”，尽管它在 Windows 上会逃出去。目前这类逃逸仅由 node-tar 自身的 STRIPABSOLUTEPATH 检查兜住，而该检查只是静默跳过该条目、并不会拒绝整个归档：扩展会以“缺文件”的状态安装成功而不是失败关闭，文档承诺的“在解压前拒绝”对这一输入类别并不成立。未来任何绕过该库检查的变化（更换解压器、preservePaths、tar 行为变化）都会把这个被接受的条目变成真实的路径穿越。所有新增符号链接测试都在 win32 上跳过，且没有任何用例覆盖反斜杠目标，因此 CI 看不到这一分歧。建议在判断 containment 之前先归一化分隔符——这只会增加拒绝，在两个平台上都是失败关闭的：
const normalizedLinkPath = linkPath.replaceAll('\\', '/');
// 对 normalizedLinkPath 运行现有的绝对路径/containment 检查并补充一个断言拒绝 ..\..\ 风格目标的回归测试。
探针（提交 ad75c92）：
gate error (null = gate ACCEPTED backslash target): null
extractFile error (null = install reported success): null
dest entries after extract: []
tar warnings: ["TAR_ENTRY_ERROR: linkpath escapes extraction directory"]
— qwen3.8-max via Qwen Code /review (v0.22.0)
| await assertTarArchiveLinksAreSafe(file, signal, options); | ||
| signal?.throwIfAborted(); |
There was a problem hiding this comment.
[Critical] R1-3: Once the scan accepts contained symlinks, actual link creation is delegated to non-strict tar.x with no warn listener, which swallows per-entry failures as unread warn events. On hosts where symlink creation fails — Windows without Developer Mode / elevation, the PR's own repro (obra/superpowers) on such a host — fs.symlink fails with EPERM, node-tar emits warn('TAR_ENTRY_ERROR') and skips the entry, and the pipeline completes: the install reports success with the linked file silently missing, where this path used to reject the archive with a clear unsupported link entry error. Treat extraction warnings as fatal in this flow (collect TAR_ENTRY_ERROR via an onwarn listener and throw after the pipeline, or extract strictly), so a failed symlink creation fails the install loudly instead of silently producing a partial extension.
Probe at ad75c928c (fs.symlink failure modeled per tar's [ONERROR] semantics):
extractFile error (null = install reported success): null
dest entries: ["CLAUDE.md"]
AGENTS.md present?: false
With warn-collection added the probe flips to a loud Error: Tar archive could not be extracted safely: EPERM: operation not permitted, symlink, and all legitimate symlink tests stay green.
中文说明
一旦扫描接受了归档内的符号链接，真正创建链接的工作就交给了非 strict 的 tar.x，而这里没有挂任何 warn 监听器，逐条目的失败会被当作无人读取的 warn 事件吞掉。在无法创建符号链接的主机上——未开启开发者模式/未提权的 Windows（本 PR 自带的复现仓库 obra/superpowers 在这类主机上就会命中）——fs.symlink 以 EPERM 失败，node-tar 发出 warn('TAR_ENTRY_ERROR') 并跳过该条目，流水线照常完成：安装“成功”，但链接文件被静默丢弃；而在此前，这条路径会以清晰的 unsupported link entry 错误直接拒绝整个归档。建议在本流程中把解压告警视为致命错误（通过 onwarn 监听器收集 TAR_ENTRY_ERROR 并在流水线结束后抛出，或以 strict 方式解压），让符号链接创建失败时安装高声失败，而不是静默装出一个不完整的扩展。
探针（提交 ad75c92，按 tar 的 [ONERROR] 语义模拟 fs.symlink 失败）：
extractFile error (null = install reported success): null
dest entries: ["CLAUDE.md"]
AGENTS.md present?: false
加入告警收集后，探针翻转为显式报错 Error: Tar archive could not be extracted safely: EPERM: operation not permitted, symlink，且所有合法符号链接测试保持通过。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const resolved = path.posix.normalize( | ||
| path.posix.join(containingDirectory, linkPath), | ||
| ); | ||
| return resolved !== '..' && !resolved.startsWith('../'); |
There was a problem hiding this comment.
[Critical] R1-8: The containment verdict is cycle-blind. A symlink whose target resolves to the link's own directory or an ancestor (loop -> ., sub/loop -> ..) normalizes to a contained path and is accepted, producing a directory cycle in the installed tree. copyExtension (fs.promises.cp with recursive + dereference and a stat-following filter) then re-copies the whole tree at each nesting level up to the kernel symlink depth limit — measured 41× amplification (16 KB payload → 673 KB staged, 40-deep nesting, 41 copies of every file). Against the 1 GiB expanded cap (symlink entries count size 0), a hostile public repo can drive ~40 GiB of staging writes → ENOSPC mid-install, or leave a corrupted 40-deep installed extension on machines that survive it. This was unreachable before this diff, when every link entry was rejected. Reject ancestor/self-resolving targets — a cycle exists exactly when the resolved target equals the link's own path or is an ancestor of it:
| const resolved = path.posix.normalize( | |
| path.posix.join(containingDirectory, linkPath), | |
| ); | |
| return resolved !== '..' && !resolved.startsWith('../'); | |
| path.posix.join(containingDirectory, linkPath), | |
| ); | |
| return ( | |
| resolved !== '..' && | |
| !resolved.startsWith('../') && | |
| resolved !== '.' && | |
| entryPath !== resolved && | |
| !entryPath.startsWith(`${resolved}/`) | |
| ); |
plus rejection tests for self -> . and sub/loop -> ...
Probe at ad75c928c through the real scan/extract/flatten/copyExtension chain:
self -> . accepted by scan: true
copyExtension outcome: resolved <- no error; install proceeds
base payload bytes: 16384
staging total bytes: 673302 <- 41.1x amplification
duplicated self-nesting depth: 40
The suggested fix flips the probe to unsupported link entry: repo-sha/self with all 15 archive-safety tests still green.
中文说明
containment 判定对“环”是盲的：目标解析到链接自身所在目录或其祖先目录的符号链接（loop -> .、sub/loop -> ..）经归一化后仍是“归档内”路径，会被接受，从而在安装出的目录树中形成目录环。随后 copyExtension（fs.promises.cp，recursive + dereference，filter 会跟随 stat）会在每一层嵌套重复复制整棵树，直到内核符号链接深度上限——实测放大 41 倍（16 KB 载荷 → 暂存 673 KB、40 层嵌套、每个文件 41 份副本）。对照 1 GiB 的解压上限（符号链接条目按 0 字节计），恶意公开仓库可以制造约 40 GiB 的暂存写入，导致安装中途 ENOSPC；在撑过去的机器上则会留下一个 40 层深、内容损坏的已安装扩展。在本次 diff 之前，所有链接条目都会被拒绝，因此这条路径不可达。建议拒绝解析到自身/祖先的目标——当且仅当解析结果等于链接自身路径或为其祖先时存在环：
（上方 ```suggestion 代码块即为修复：在返回条件中排除 resolved === '.'、`entryPath === resolved` 以及 `entryPath` 位于 `resolved` 之下的情形）
并补充 self -> . 与 sub/loop -> .. 的拒绝用例。
探针（提交 ad75c92，走真实的 扫描/解压/flatten/copyExtension 链路）：
self -> . accepted by scan: true
copyExtension outcome: resolved <- no error; install proceeds
base payload bytes: 16384
staging total bytes: 673302 <- 41.1x amplification
duplicated self-nesting depth: 40
应用上述修复后，探针翻转为 unsupported link entry: repo-sha/self，且 archive-safety 的 15 个测试全部保持通过。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| Git 2.37 or newer is required for credentialed, non-GitHub, nested marketplace, submodule, and Git LFS sources because Qwen Code uses `http.curloptResolve` to pin Git connections to validated DNS results. On older Git versions, Qwen Code supports only anonymous public `https://github.com/{owner}/{repo}[.git]` root repositories by resolving the requested ref to a commit and downloading GitHub's source archive with the same public-network and archive-safety checks. | ||
|
|
||
| Because the older-Git fallback installs from a source archive rather than a clone, it cannot install repositories that rely on symlinks, submodules, or Git LFS, and it caps downloads at 100 MiB compressed and archives at 100,000 entries / 1 GiB expanded. Release-based installs are still preferred when a repository publishes releases. | ||
| Because the older-Git fallback installs from a source archive rather than a clone, it cannot install repositories that rely on submodules or Git LFS, and it caps downloads at 100 MiB compressed and archives at 100,000 entries / 1 GiB expanded. Symlinks are supported when their targets stay inside the repository: a link pointing outside it, an absolute target, or a hard link is rejected and still requires Git 2.37 or newer. Release-based installs are still preferred when a repository publishes releases. |
There was a problem hiding this comment.
[Critical] R1-9: The caps documented on this line are defeated for archives carrying accepted symlinks. Contained symlinks have no fan-in bound and their tar size counts as 0 toward expandedBytes, so copyExtension's dereference copy re-materializes the target file once per link. Measured at reduced scale on the real pipeline: one 4 MiB file + 300 symlinks to it passed the scan counting only 4,194,304 bytes (symlinks: 300 × 0 bytes), then staged 301 files totaling 1,262,485,504 bytes — 301× amplification, already over the 1 GiB cap, and exactly linear in link count. At the cap (one ~1 GiB file of zeros — compresses to almost nothing against the 100 MiB download cap — plus ~99,999 links, staying under the 100,000-entry ceiling) this extrapolates to ~100 TB of staging writes and ENOSPC mid-install. This is acyclic many-links-to-one-file, distinct from the cycle case, and not addressed by rejecting ancestor/self targets. Account for materialization in the scan: record entry sizes by path, count accepted symlinks toward the link cap (it currently counts only unsupported links), and after the pass sum targetSize × in-degree over accepted links' resolved targets, failing validation against MAX_ARCHIVE_EXPANDED_BYTES.
Probe at ad75c928c (reduced scale):
{"scanOutcome":"ACCEPTED","scannedBytes":4194304,
"byType":{"File":{"count":1,"bytes":4194304},"SymbolicLink":{"count":300,"bytes":0}},
"stagedFiles":301,"stagedBytes":1262485504,
"expandedByteCap":1073741824,"amplificationVsTarget":"301.00"}
With byte accounting added the probe flips to REJECTED: Tar archive expands beyond 1073741824 bytes. while all 15 archive-safety tests stay green.
中文说明
本行文档所承诺的上限，对携带被接受符号链接的归档实际上已不成立。归档内的符号链接没有“扇入”数量限制，且其 tar 尺寸按 0 字节计入 expandedBytes，因此 copyExtension 的 dereference 复制会按每个链接把目标文件完整重新实体化一份。在真实流水线上以缩小规模实测：1 个 4 MiB 文件 + 300 个指向它的符号链接通过了扫描，扫描只计了 4,194,304 字节（符号链接：300 × 0 字节），随后暂存出 301 个文件、共 1,262,485,504 字节——放大 301 倍，已超过 1 GiB 上限，且放大系数与链接数量严格线性相关。按上限构造（1 个约 1 GiB 的全零文件——相对 100 MiB 下载上限几乎可完全压缩——外加约 99,999 个链接，仍不超 100,000 条目上限），外推可得约 100 TB 的暂存写入，安装中途即 ENOSPC。这是无环的“多链接指向同一文件”形态，与目录环问题相互独立，也不能通过拒绝自身/祖先目标来修复。建议在扫描阶段把实体化计入成本：按路径记录条目尺寸，把被接受的符号链接计入链接上限（目前该上限只统计不被支持的链接），并在扫描结束后对被接受链接的解析目标求和 目标尺寸 × 入度，超过 MAX_ARCHIVE_EXPANDED_BYTES 即校验失败。
探针（提交 ad75c92，缩小规模）：
{"scanOutcome":"ACCEPTED","scannedBytes":4194304,
"byType":{"File":{"count":1,"bytes":4194304},"SymbolicLink":{"count":300,"bytes":0}},
"stagedFiles":301,"stagedBytes":1262485504,
"expandedByteCap":1073741824,"amplificationVsTarget":"301.00"}
加入字节核算后，探针翻转为 REJECTED: Tar archive expands beyond 1073741824 bytes.，且 archive-safety 的 15 个测试全部保持通过。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| linkPath: string | undefined, | ||
| ): boolean { | ||
| if (!linkPath) return false; | ||
| if (path.posix.isAbsolute(linkPath)) return false; |
There was a problem hiding this comment.
[Suggestion] R1-4: The guard returns true for entries whose own entry PATH is absolute: dirname('/a/b/link') is /a/b, and normalize(join('/a/b', '../../..')) yields /, which is neither '..' nor '../'-prefixed — accepted. The verdict is documented as self-contained ("decided from the archive's own entry paths") but is correct for this class only by coincidence with node-tar's STRIPABSOLUTEPATH root-stripping at extraction; any future extraction change that preserves absolute paths (preservePaths, a different extractor, a second consumer of this check) silently turns these accepted entries into symlinks written outside the destination. GitHub codeload never emits absolute entry paths, so failing closed here is a pure tightening:
| if (path.posix.isAbsolute(linkPath)) return false; | |
| if (path.posix.isAbsolute(linkPath)) return false; | |
| if (path.posix.isAbsolute(entryPath)) return false; |
Probe at ad75c928c:
scan error (null = ACCEPTED absolute-path entry): null
extractFile error: null ; dest entries: []
tar warnings: ["TAR_ENTRY_INFO: stripping / from absolute path",
"TAR_ENTRY_ERROR: linkpath escapes extraction directory"]
中文说明
对于条目路径本身是绝对路径的符号链接条目，该守卫会返回 true：dirname('/a/b/link') 为 /a/b，normalize(join('/a/b', '../../..')) 得到 /，既不是 '..' 也不以 '../' 开头——于是被接受。文档声称该判定是完备自足的（“仅依据归档自身的条目路径”），但对这一类别而言，其正确性只是恰好与 node-tar 在解压时的 STRIPABSOLUTEPATH 去根处理相吻合；未来任何保留绝对路径的解压变化（preservePaths、更换解压器、该检查的第二个使用方）都会把这些已被接受的条目静默变成写到目标目录之外的符号链接。GitHub codeload 从不生成绝对路径条目，因此在这里失败关闭是纯粹的收紧：
（上方 ```suggestion 代码块：在 linkPath 的绝对路径检查旁，补充对 entryPath 的同样检查）
探针（提交 ad75c92）：
scan error (null = ACCEPTED absolute-path entry): null
extractFile error: null ; dest entries: []
tar warnings: ["TAR_ENTRY_INFO: stripping / from absolute path",
"TAR_ENTRY_ERROR: linkpath escapes extraction directory"]
— qwen3.8-max via Qwen Code /review (v0.22.0)
| if ( | ||
| allowContainedSymlinks && | ||
| entry.type === 'SymbolicLink' && | ||
| isContainedSymlinkTarget(entry.path, entry.linkpath) | ||
| ) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
[Suggestion] R1-5: Containment is decided per entry in isolation, but the extractor cannot create a symlink whose resolved target passes through an already-extracted symlink (tar's ENSURE_NO_SYMLINK) — such entries are silently skipped with an unlistened warn. The guard therefore accepts shapes the install then loses without error: legitimate link-to-link chains (stable -> v1.2 plus current -> stable: stable extracts first, current is refused-through-symlink and dropped, so the installed extension silently diverges from a Git ≥ 2.37 clone), and entries placed under an intermediate in-archive symlink, whose escape potential is backstopped only by that same untested skip. Track accepted symlink entry paths during the scan and reject any entry whose normalized resolution passes through one of them, so such archives fail closed with the existing message; and/or pin the extractor-layer behavior with a regression test.
Probe at ad75c928c through the real extractFile:
extractFile error: null
dest entries after extract: ["a","z"]
tar warnings: ["TAR_ENTRY_ERROR: TAR_SYMLINK_ERROR: Cannot extract through symbolic link"]
(`b` silently lost from {z, a -> z, b -> a}.)
中文说明
containment 是逐条目孤立判定的，但解压器无法创建“解析目标需要穿过已解压符号链接”的符号链接（tar 的 ENSURE_NO_SYMLINK）——这类条目会被静默跳过，只留下无人监听的 warn。于是守卫接受了这些形状，而安装在无任何报错的情况下把它们弄丢：合法的链式链接（stable -> v1.2 加 current -> stable：stable 先被解压，current 因需穿过符号链接而被拒绝创建并丢弃，安装结果与 Git ≥ 2.37 克隆静默不一致）；以及放置在中间符号链接之下的条目，其潜在逃逸目前仅由同一个未经测试的跳过行为兜底。建议在扫描期间记录已接受的符号链接条目路径，凡解析路径需要穿过其中之一的条目一律拒绝，使此类归档以现有错误信息失败关闭；和/或为解压器层的这一行为补充回归测试加以固化。
探针（提交 ad75c92，走真实的 extractFile）：
extractFile error: null
dest entries after extract: ["a","z"]
tar warnings: ["TAR_ENTRY_ERROR: TAR_SYMLINK_ERROR: Cannot extract through symbolic link"]
（{z, a -> z, b -> a} 中的 `b` 被静默丢弃。）
— qwen3.8-max via Qwen Code /review (v0.22.0)
| ); | ||
|
|
||
| it.runIf(process.platform !== 'win32')( | ||
| 'rejects a symlink whose target escapes the archive root', |
There was a problem hiding this comment.
[Suggestion] R1-6: The resolved !== '..' clause of isContainedSymlinkTarget has no test: no added case uses a symlink whose normalized target is exactly '..', and since '..'.startsWith('../') is false, deleting the clause survives the entire suite while the gate flips from rejected to accepted. A public archive carrying a root-level escape -> .. (or nested docs/link -> ../..) would then be accepted with allowContainedSymlinks: true and extracted pointing at the parent of the install destination — precisely the escape the clause blocks. Add rejection cases beside this one, e.g.:
it.runIf(process.platform !== 'win32')(
'rejects a symlink whose normalized target is the archive root parent',
async () => {
await fs.symlink('..', path.join(root, 'escape-dotdot'));
const archive = path.join(root, 'escape-dotdot.tar');
await tar.c({ cwd: root, file: archive }, ['escape-dotdot']);
await expect(
assertTarArchiveLinksAreSafe(archive, undefined, allowLinks),
).rejects.toThrow('unsupported link entry');
},
);Mutation probe at ad75c928c:
UNMUTATED gate verdict for root target '..': "REJECTED (unsupported link entry: escape-dotdot)"
MUTATED gate verdict for root target '..': "ACCEPTED" (all suites stay green)
中文说明
isContainedSymlinkTarget 中的 resolved !== '..' 子句没有任何测试覆盖：新增用例中没有出现归一化结果恰为 '..' 的符号链接目标；又因为 '..'.startsWith('../') 为 false，删除该子句后整个测试套件依旧全绿，而守卫的行为会从“拒绝”翻转为“接受”。届时，一个带有根级 escape -> ..（或嵌套 docs/link -> ../..）链接的公开归档在 allowContainedSymlinks: true 下会被接受，解压出的链接将指向安装目标目录的父目录——正是该子句要拦截的逃逸。建议在旁边补充拒绝用例，例如：
it.runIf(process.platform !== 'win32')(
'rejects a symlink whose normalized target is the archive root parent',
async () => {
await fs.symlink('..', path.join(root, 'escape-dotdot'));
const archive = path.join(root, 'escape-dotdot.tar');
await tar.c({ cwd: root, file: archive }, ['escape-dotdot']);
await expect(
assertTarArchiveLinksAreSafe(archive, undefined, allowLinks),
).rejects.toThrow('unsupported link entry');
},
);变异探针（提交 ad75c92）：
未变异时，根级目标 '..' 的守卫判定: "REJECTED (unsupported link entry: escape-dotdot)"
变异（删除该子句）后: "ACCEPTED"（所有套件仍全绿）
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // Public repositories legitimately carry in-repo symlinks (the reported | ||
| // case is a root `AGENTS.md -> CLAUDE.md`), and this fallback is the only | ||
| // way to install them without Git 2.37+. Targets that escape the archive | ||
| // root are still refused before extraction. | ||
| allowContainedSymlinks: true, |
There was a problem hiding this comment.
[Suggestion] R1-10: Containment does not require the link TARGET to exist as an archive entry. A dangling contained symlink (legal in git: links to generated files, or a target deleted while the link stayed — e.g. latest.md -> sub/generated-docs.md) passes this guard, gets created by tar.x, and is then silently dropped by copyExtension's stat-following filter during staging (stat ENOENT → filter returns false): the install "succeeds" with the entry missing and no warning, silently diverging from what a Git ≥ 2.37 clone gives. Before this diff the archive was rejected outright. Make the "provably resolves inside the archive" claim include existence: collect entry paths during the scan and reject (or explicitly warn on) an accepted symlink whose normalized resolved target is not a known entry path.
Probe at ad75c928c:
{"scanOutcome":"ACCEPTED","extractedLinkIsSymlink":true,"copyError":null,
"stagedEntries":["CLAUDE.md","sub"],"latestPresentInStaging":false}
The entry-existence fix flips this to REJECTED: Tar archive contains symlink latest.md whose target sub/generated-docs.md is not an archive entry. with the PR's archive-safety tests still green.
中文说明
containment 判定并不要求链接目标真实存在于归档条目中。一个悬空但“在归档内”的符号链接（这在 git 中是合法的：指向生成文件的链接，或目标被删除而链接保留——例如 latest.md -> sub/generated-docs.md）会通过该守卫、被 tar.x 创建出来，随后在暂存阶段被 copyExtension 跟随 stat 的 filter 静默丢弃（stat 得到 ENOENT → filter 返回 false）：安装“成功”，但该条目缺失且无任何告警，与 Git ≥ 2.37 克隆的结果静默不一致。在本次 diff 之前，此类归档会被直接拒绝。建议把“可证明解析到归档内部”的承诺扩展为包含“存在性”：在扫描期间收集条目路径集合，对被接受的符号链接，若其归一化解析目标不属于已知条目路径，则拒绝（或至少显式告警）。
探针（提交 ad75c92）：
{"scanOutcome":"ACCEPTED","extractedLinkIsSymlink":true,"copyError":null,
"stagedEntries":["CLAUDE.md","sub"],"latestPresentInStaging":false}
加入“目标必须存在”的检查后，翻转为 REJECTED: Tar archive contains symlink latest.md whose target sub/generated-docs.md is not an archive entry.，且 PR 的 archive-safety 测试保持全绿。
— qwen3.8-max via Qwen Code /review (v0.22.0)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Test Plan (not a blocker): 168 passed — this review observed 21266, 1689, 23739, 1658, 601, 4227, 626 passed; 23736 passed — this review observed 21266, 1689, 23739, 1658, 601, 4227, 626 passed.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
Test Plan（非阻断）：168 passed — this review observed 21266, 1689, 23739, 1658, 601, 4227, 626 passed; 23736 passed — this review observed 21266, 1689, 23739, 1658, 601, 4227, 626 passed。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| function isContainedPath(root: string, candidate: string): boolean { | ||
| const relative = path.relative(root, candidate); | ||
| return ( |
There was a problem hiding this comment.
[Suggestion] The new isContainedPath is a near-verbatim copy of the codebase's canonical exported containment helper isSubpath (packages/core/src/utils/paths.ts:400, ~40 call sites) — the third near-identical containment predicate next to isWithinRoot (packages/core/src/utils/fileUtils.ts:496). Both compute path.relative(root, candidate) and test the same three escape conditions; the only difference is the extra relative !== '' here, which excludes the root itself. Because this security-load-bearing post-extraction check relies on the private copy, any future hardening of isSubpath will not propagate here, and the two implementations can drift silently with no test to surface it — the same drift that already produced isWithinRoot's local duplicate in zip-extraction.ts:26. Delegate to the existing helper and keep the strict-inside semantics by composition:
import { isSubpath } from '../utils/paths.js';
function isContainedPath(root: string, candidate: string): boolean {
return isSubpath(root, candidate) && path.relative(root, candidate) !== '';
}中文说明
新增的 isContainedPath 与代码库中标准的导出辅助函数 isSubpath（packages/core/src/utils/paths.ts:400，约 40 处调用）几乎逐字相同，是继 isWithinRoot（packages/core/src/utils/fileUtils.ts:496）之后第三个几乎相同的 containment 谓词。两者都计算 path.relative(root, candidate) 并检查同样的三种逃逸条件，唯一区别是这里多了排除根目录自身的 relative !== ''。由于这个承载安全职责的解压后检查依赖私有副本，未来对 isSubpath 的任何加固都不会传播到这里，两个实现可能在没有任何测试暴露的情况下悄悄漂移——zip-extraction.ts:26 中 isWithinRoot 的本地重复正是这种漂移的先例。建议委托给现有辅助函数，并通过组合保留“严格在内部”的语义（见上方代码）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| if (options.allowContainedSymlinks === true) { | ||
| await assertDirectorySymlinksAreSafe(destination); | ||
| } |
There was a problem hiding this comment.
[Suggestion] Containment is judged against the archive root in the pre-extraction scan, but the installed root is the directory flattenSingleExtensionDirectory promotes — the mismatch is compensated by this second, differently-formalised containment walk after flattening instead of judging acceptance against the root that actually gets installed. The diff's own escape test demonstrates the class: repo-archive/escape -> ../pwn with a root-level pwn entry passes the scan (because pwn is contained in the archive root), full extraction runs, flattening re-roots the tree, and only then does assertDirectorySymlinksAreSafe reject. The concrete costs: the single policy "a symlink must resolve to a regular file inside the installed root" is implemented twice in two formalisms (posix-string resolution over tar paths vs. realpath walk over the extracted tree) that must stay in sync, so any future policy change must be made in both; archives that will ultimately be refused are fully extracted (up to the 1 GiB expanded cap) before failing; and the walk throws "Tar archive contains unsupported link entry" while inspecting a flattened directory, pointing diagnosis at the archive when the cause is the flatten re-rooting. Consider deciding containment in the scan against the top-level component the link lives under (the root that survives flattening — for codeload archives that always wraps everything in a single directory this matches the installed root in both the flattened and non-flattened cases), so the acceptance decision describes the installed tree by construction; if this walk stays as defense-in-depth, give it an error message that names the extracted tree rather than the tar archive.
中文说明
containment 在解压前的扫描阶段是相对“归档根目录”判定的，但真正安装的根目录是 flattenSingleExtensionDirectory 提升出来的那个目录——这里用 flatten 之后的第二次、形式化方式不同的 containment 遍历来补偿这一错位，而不是直接对最终安装的根目录做接受判定。本 diff 自带的逃逸测试正好演示了这一类问题：repo-archive/escape -> ../pwn 加上根层级的 pwn 条目能通过扫描（因为 pwn 在归档根内），完整解压照常进行，flatten 重新定根后才由 assertDirectorySymlinksAreSafe 拒绝。具体代价：同一条策略“符号链接必须解析到安装根目录内的常规文件”被用两种形式化方式实现了两遍（对 tar 路径做 posix 字符串解析 vs. 对解压后的树做 realpath 遍历），两者必须保持同步，未来任何策略变更都得在两处同时修改；最终会被拒绝的归档会先被完整解压（直至 1 GiB 解压上限）才失败；而且该遍历在检查一个扁平化后的目录时抛出 “Tar archive contains unsupported link entry”，把问题归因指向归档，而真正原因是 flatten 重新定根。建议扫描阶段直接相对链接所在的顶层分量（即 flatten 后保留的根——codeload 归档总是把所有内容包在单个目录里，无论是否 flatten 该根都与安装根一致）来判定 containment，使接受判定按构造描述安装后的树；如果保留本遍历作为纵深防御，请让它的报错信息指明是“解压出的目录树”而不是 tar 归档。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const hasArchiveDescendant = (entryPath: string) => { | ||
| for (const candidatePath of archiveEntries.keys()) { | ||
| if (candidatePath.startsWith(`${entryPath}/`)) return true; |
There was a problem hiding this comment.
[Suggestion] hasArchiveDescendant linearly scans the entire entry map once per accepted symlink — O(accepted-links × entries) with a fresh ${entryPath}/ prefix string allocated on every comparison — and the condition order at the post-pass runs this expensive scan before the O(1) checks that would reject the link anyway. The production caller enables both allowContainedSymlinks and enforceResourceLimits, so the bounds are MAX_LINK_ENTRIES = 100 links × MAX_ARCHIVE_ENTRIES = 100,000 entries — up to 10⁷ startsWith comparisons plus 10⁷ allocations in one synchronous burst on the install path; a hostile archive of contained-but-dangling symlinks forces the full scan per link before the cheap archiveEntries.get() check could short-circuit it. Measured at production bounds on the PR code: median 1165 ms for the scan+reject; after reordering the condition to the cheap checks first, the same rejection takes median 623 ms — ~540 ms of the worst case is the condition order alone. Reorder to !target || !REGULAR_FILE_TYPES.has(target.type) || hasArchiveDescendant(link.entryPath), and either hoist the prefix string out of the inner loop or precompute a Set of every entry's ancestor-directory prefixes once (O(total path length)) and answer each descendant query with a single Set.has.
中文说明
hasArchiveDescendant 对每个被接受的符号链接都会线性扫描整个条目表——复杂度为 O(被接受链接数 × 条目数)，且每次比较都会新分配一个 ${entryPath}/ 前缀字符串——而 post-pass 中的条件顺序让这一昂贵扫描先于那些本可直接拒绝链接的 O(1) 检查执行。生产调用方同时启用了 allowContainedSymlinks 与 enforceResourceLimits，因此上界是 MAX_LINK_ENTRIES = 100 个链接 × MAX_ARCHIVE_ENTRIES = 100,000 个条目——安装路径上单次同步突发最多 10⁷ 次 startsWith 比较加 10⁷ 次字符串分配；携带“归档内但悬空”符号链接的恶意归档会迫使每个链接都走完整扫描，而便宜的 archiveEntries.get() 检查本可以先短路。在生产上界下实测：PR 代码扫描+拒绝中位 1165 ms；把条件顺序改为先做便宜检查后，同样的拒绝只需中位 623 ms——最坏情况下约 540 ms 完全来自条件顺序。建议把条件重排为 !target || !REGULAR_FILE_TYPES.has(target.type) || hasArchiveDescendant(link.entryPath)，并把前缀字符串提出内层循环，或一次性预计算所有条目祖先目录前缀的 Set（O(总路径长度)），之后每个后代查询只需一次 Set.has。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| if (options.allowContainedSymlinks === true) { | ||
| await assertDirectorySymlinksAreSafe(destination); | ||
| } |
There was a problem hiding this comment.
[Suggestion] The same unsafe-symlink violation reaches the user in two different shapes. A scan-level rejection (e.g. an absolute target like AGENTS.md -> /etc/hostname) happens inside extractFile, which sits inside extractArchiveFile's try/catch, so the user sees "Extension archive could not be extracted. Make sure it is a valid .zip or .tar.gz file. Tar archive contains unsupported link entry: …" — implying the archive is malformed, though it is valid and was refused by policy. The sibling case — a link that escapes only after wrapper-directory flattening, which this diff's own escape test exercises — propagates unwrapped from this check as the clean "Tar archive contains unsupported link entry: …" that the test pins. Identical policy violation, two user-facing formats, one of which misdescribes the failure. Let link-safety validation errors bypass the extraction-failure wrapper so both checks report identically — e.g. rethrow the Tar archive contains validation errors unwrapped in extractArchiveFile's catch, or perform the scan outside the wrapped extraction region.
中文说明
同一类不安全符号链接违规会以两种不同形态到达用户。扫描阶段的拒绝（例如 AGENTS.md -> /etc/hostname 这类绝对路径目标）发生在 extractFile 内部，而 extractFile 位于 extractArchiveFile 的 try/catch 之中，因此用户看到的是 “Extension archive could not be extracted. Make sure it is a valid .zip or .tar.gz file. Tar archive contains unsupported link entry: …”——暗示归档本身损坏，实际上归档是合法的、只是被策略拒绝。姊妹情形——只在包装目录 flatten 之后才逃逸的链接（本 diff 自带的逃逸测试覆盖的情形）——则从这个检查直接抛出未被包装的干净报错 “Tar archive contains unsupported link entry: …”（测试固定的文案）。同一种策略违规，两种面向用户的格式，其中一种误导了失败原因。建议让链接安全校验错误绕开解压失败的包装，使两处检查的报告一致——例如在 extractArchiveFile 的 catch 中对 Tar archive contains 校验错误原样重新抛出，或把扫描放到被包装的解压区域之外。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| fs.createReadStream(file), | ||
| tar.x({ | ||
| cwd: dest, | ||
| strict: options.allowContainedSymlinks === true, | ||
| }), |
There was a problem hiding this comment.
[Suggestion] Extraction strict is silently piggybacked on allowContainedSymlinks with nothing explaining that the coupling is load-bearing: without strict, node-tar turns every per-entry failure (a symlink failing to materialize, an entry its own containment check skips) into a warning and continues extraction, and the post-extraction assertDirectorySymlinksAreSafe only detects bad symlinks, never a missing one. A future maintainer reading strict as unrelated to symlink policy could hoist it into its own option or drop it, after which a failed fs.symlink (EEXIST/EPERM) completes extraction with the entry silently absent — the installed extension quietly diverges from the repository instead of failing the install. The test 'fails when a contained symlink cannot be extracted' guards this only indirectly. Add a comment at the strict: line:
| fs.createReadStream(file), | |
| tar.x({ | |
| cwd: dest, | |
| strict: options.allowContainedSymlinks === true, | |
| }), | |
| fs.createReadStream(file), | |
| tar.x({ | |
| cwd: dest, | |
| // strict makes per-entry extraction failures (such as an accepted | |
| // symlink failing to materialize) errors instead of silently | |
| // skipped entries. | |
| strict: options.allowContainedSymlinks === true, | |
| }), |
中文说明
解压的 strict 被悄悄挂靠在 allowContainedSymlinks 上，没有任何说明指出这一耦合是承重的：没有 strict 时，node-tar 会把每个逐条目的失败（符号链接创建失败、被其自身 containment 检查跳过的条目）降级为警告并继续解压，而解压后的 assertDirectorySymlinksAreSafe 只能发现“坏”的符号链接，发现“缺失”的。未来维护者若把 strict 理解为与符号链接策略无关，可能把它拆成独立选项或直接删掉，此后一次 fs.symlink 失败（EEXIST/EPERM）就会让解压在条目静默缺失的情况下“成功”——安装的扩展与仓库悄悄产生偏差，而不是让安装失败。测试 'fails when a contained symlink cannot be extracted' 只是间接地守护了这一点。建议在 strict: 行加上注释（见上方 suggestion）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| if (archiveEntries.has(entryPath)) { | ||
| failValidation( | ||
| new Error( | ||
| `Tar archive contains duplicate entry path: ${formatEntryPath(entry.path)}`, |
There was a problem hiding this comment.
[Suggestion] The new duplicate-entry rejection branch has no test anywhere in the suite — deleting this guard is a mutation the whole suite survives (verified by mutation probe: with the branch deleted, a crafted tar carrying two target entries plus a symlink to target flipped from rejects 'duplicate entry path' to silent acceptance, while all 23 + 154 existing tests stayed green). That matters because archiveEntries overwrites on duplicate: with the guard gone, a symlink's acceptance would track whatever the last duplicate entry records, so the pre-extraction scan's acceptance decision could rest on different metadata than intended, and rejection would be left entirely to the post-flatten realpath walk, which exists only in the fallback pipeline. Add a unit test, e.g. craft a tar with createTarFileHeader('target', 0) twice plus symlinkHeader('link', 'target') and expect assertTarArchiveLinksAreSafe(archive, undefined, { allowContainedSymlinks: true }) to reject with 'duplicate entry path'.
中文说明
新增的“重复条目路径拒绝”分支在整个测试套件中没有任何测试覆盖——删除这个守卫是一种整个套件都能“存活”的突变（已用突变探针验证：删除该分支后，一个携带两个 target 条目外加指向 target 的符号链接的构造 tar 从 拒绝 'duplicate entry path' 翻转为静默接受，而现有 23 + 154 个测试全部保持通过）。这一点很要紧，因为 archiveEntries 在重复条目上是覆盖写入：守卫一旦缺失，符号链接的接受决定将跟随最后一个重复条目记录的元数据，解压前扫描的接受判定可能基于与预期不同的元数据做出，而拒绝就完全依赖 flatten 后的 realpath 遍历——该遍历只存在于回退安装流水线中。建议补一个单元测试：用 createTarFileHeader('target', 0) 两次加 symlinkHeader('link', 'target') 构造 tar，断言 assertTarArchiveLinksAreSafe(archive, undefined, { allowContainedSymlinks: true }) 以 'duplicate entry path' 拒绝。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| it('rejects a symlink resolving to its own directory', async () => { | ||
| const archive = path.join(root, 'self-cycle.tar'); | ||
| await writeCraftedTar(archive, [symlinkHeader('self', '.')]); |
There was a problem hiding this comment.
[Suggestion] The two cycle-rejection tests never reach the conditions their names claim to pin: symlinkHeader('self', '.') resolves against dirname('self') to '.', and symlinkHeader('sub/loop', '..') also resolves to '.' — both are rejected by the resolved === '.' clause, leaving the self/ancestor clauses in archive-safety.ts:86-87 with zero discriminating tests. Verified by mutation probe: deleting those two lines leaves the entire suite green, while a crafted archive (regular file a + symlink a/b/link -> ..) flips from rejection to silent acceptance — on the production path such a link would then pass the pre-extraction scan and rejection falls to layers that exist for other purposes (tar strict-mode entry errors, the post-flatten walk). The entry-path WINDOWS_ABSOLUTE_PATH.test(entryPath) clause has the same gap — only the link-target clause is exercised today. Add crafted-tar cases isolating each condition: symlinkHeader('link', 'link') (self-reference), symlinkHeader('a/b/link', '..') alongside createTarFileHeader('a', 0) (own ancestor that is a regular-file entry), and a symlink whose entry path is Windows-absolute.
中文说明
两个“环拒绝”测试实际上触达不到其名称声称要固定的条件：symlinkHeader('self', '.') 相对 dirname('self') 解析得到 '.'，symlinkHeader('sub/loop', '..') 同样解析为 '.'——两者都被 resolved === '.' 子句拒绝，导致 archive-safety.ts:86-87 的自身/祖先子句没有任何判别性测试。突变探针验证：删除这两行后整套测试依旧全绿，而一个构造归档（常规文件 a + 符号链接 a/b/link -> ..）会从“拒绝”翻转为“静默接受”——在生产路径上这类链接将通过解压前扫描，拒绝只能落在为其他目的存在的层（tar strict 模式的条目错误、flatten 后的遍历）。入口路径侧的 WINDOWS_ABSOLUTE_PATH.test(entryPath) 子句存在同样缺口——目前只有链接目标侧的子句被测试覆盖。建议补充隔离每个条件的构造 tar 用例：symlinkHeader('link', 'link')（自引用）、symlinkHeader('a/b/link', '..') 加 createTarFileHeader('a', 0)（自身祖先是常规文件条目）、以及入口路径为 Windows 绝对路径的符号链接。
— qwen3.8-max via Qwen Code /review (v0.22.0)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Not explored to full depth (tool budget reached): "agent 6a": none — no check was cut short..
Test Plan (not a blocker): 168 passed — this review observed 1689, 23736, 1658, 601, 4227, 626 passed.
Convergence: round 3 posted 7 inline comment(s), 3 of them reported for the first time; the previous round posted 7 (7 new). Findings keep coming back to the same files: packages/core/src/extension/archive-safety.ts (findings in round 2; 2 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. No Critical finding is open on this round, so merging and moving the remaining Suggestion threads to a follow-up issue is available as an ending — a merged pull request cannot diverge further. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
已审查。 建议见行内评论。
未探索到全部深度（达到工具调用预算）："agent 6a"：none — no check was cut short.。
Test Plan（非阻断）：168 passed — this review observed 1689, 23736, 1658, 601, 4227, 626 passed。
收敛情况：第 3 轮发布了 7 条行内评论，其中 3 条是首次提出；上一轮发布了 7 条（其中 7 条首次提出）。发现反复回到同一批文件：packages/core/src/extension/archive-safety.ts（第 2 轮已出过发现，本轮又有 2 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。本轮没有未决的 Critical，因此"合入后把剩余 Suggestion 线程转到后续 issue"是一个可选的结束方式——已合入的 PR 不会继续发散。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.0)
| } | ||
|
|
||
| export async function assertTarArchiveHasNoLinks( | ||
| function isContainedPath(root: string, candidate: string): boolean { |
There was a problem hiding this comment.
[Suggestion] R2-1: isContainedPath is still a near-verbatim copy of the codebase's canonical exported containment helper isSubpath (packages/core/src/utils/paths.ts:400, ~40 call sites) — one more near-identical lexical-containment predicate next to isWithinRoot (packages/core/src/utils/fileUtils.ts:496, a path.resolve+startsWith variant) and the private isWithinRoot in packages/core/src/extension/zip-extraction.ts. All compute a relative path and reject the .. / ..-prefixed / absolute results; the only delta here is relative !== '', excluding the root itself. Because this security-load-bearing post-extraction check relies on the private copy, any future hardening of isSubpath will not propagate here, and the implementations can drift silently with no test to surface it — the same drift that already produced the zip-extraction.ts:26 duplicate. Delegate to the existing helper and keep the strict-inside semantics by composition:
import { isSubpath } from '../utils/paths.js';
function isContainedPath(root: string, candidate: string): boolean {
return isSubpath(root, candidate) && path.relative(root, candidate) !== '';
}A future containment fix (Windows case-insensitivity, trailing-separator handling) applied to one copy but not the others would make the zip and tar extraction paths silently disagree about whether a path is inside the root.
中文说明
isContainedPath 仍然是代码库中标准导出辅助函数 isSubpath（packages/core/src/utils/paths.ts:400，约 40 处调用）的近乎逐字副本——这是继 isWithinRoot（packages/core/src/utils/fileUtils.ts:496，path.resolve+startsWith 变体）和 packages/core/src/extension/zip-extraction.ts 中的私有 isWithinRoot 之后又一个几乎相同的 lexical-containment 谓词。它们都计算相对路径并拒绝 .. / .. 前缀 / 绝对路径结果；此处唯一区别是多了排除根目录自身的 relative !== ''。由于这个承载安全职责的解压后检查依赖私有副本，未来对 isSubpath 的任何加固都不会传播到这里，各实现可能在没有任何测试暴露的情况下悄悄漂移——zip-extraction.ts:26 的本地重复正是这种漂移的先例。建议委托给现有辅助函数，并通过组合保留"严格在内部"的语义（见上方代码）。未来某次 containment 修复（Windows 大小写不敏感、尾分隔符处理）若只应用于其中一个副本，zip 与 tar 解压路径就会在"路径是否在根目录内"这一问题上悄悄产生分歧。
— qwen3.8-max via Qwen Code /review (v0.22.0)
There was a problem hiding this comment.
Fixed — delegates to the shared isSubpath helper now (isContainedPath(root, candidate) = isSubpath(root, candidate) && path.relative(root, candidate) !== ''), so the containment logic lives in one place.
| await flattenSingleExtensionDirectory(destination, archivePath); | ||
| signal?.throwIfAborted(); | ||
| if (options.allowContainedSymlinks === true) { | ||
| await assertDirectorySymlinksAreSafe(destination, signal, { |
There was a problem hiding this comment.
[Suggestion] R2-2: Containment is judged against the archive root in the pre-extraction scan, but the installed root is the directory flattenSingleExtensionDirectory promotes — the mismatch is compensated by this second, differently-formalised containment walk after flattening instead of judging acceptance against the root that actually gets installed. The diff's own escape test demonstrates the class: repo-archive/escape -> ../pwn with a root-level pwn entry passes the scan, full extraction runs, flattening re-roots the tree, and only then does this walk reject. The concrete costs persist: the single policy "a symlink must resolve to a regular file inside the installed root" is implemented twice in two formalisms (posix-string resolution over tar paths vs. realpath walk over the extracted tree) that must stay in sync; archives that will ultimately be refused are fully extracted (up to the 1 GiB expanded cap) before failing; and the walk throws "Tar archive contains unsupported link entry" while inspecting a flattened directory, pointing diagnosis at the archive when the cause is the flatten re-rooting. Consider deciding containment in the scan against the top-level component the link lives under (the root that survives flattening), so the acceptance decision describes the installed tree by construction; if this walk stays as defense-in-depth, give it an error message that names the extracted tree rather than the tar archive.
中文说明
containment 在解压前的扫描阶段是相对"归档根目录"判定的，但真正安装的根目录是 flattenSingleExtensionDirectory 提升出来的那个目录——这里用 flatten 之后的第二次、形式化方式不同的 containment 遍历来补偿这一错位，而不是直接对最终安装的根目录做接受判定。本 diff 自带的逃逸测试正好演示了这一类问题：repo-archive/escape -> ../pwn 加上根层级的 pwn 条目能通过扫描，完整解压照常进行，flatten 重新定根后才由该遍历拒绝。具体代价依然存在：同一条策略"符号链接必须解析到安装根目录内的常规文件"被用两种形式化方式实现了两遍（对 tar 路径做 posix 字符串解析 vs. 对解压后的树做 realpath 遍历），两者必须保持同步；最终会被拒绝的归档会先被完整解压（直至 1 GiB 解压上限）才失败；而且该遍历在检查一个扁平化后的目录时抛出 "Tar archive contains unsupported link entry"，把问题归因指向归档，而真正原因是 flatten 重新定根。建议扫描阶段直接相对链接所在的顶层分量（即 flatten 后保留的根）来判定 containment，使接受判定按构造描述安装后的树；如果保留本遍历作为纵深防御，请让它的报错信息指明是"解压出的目录树"而不是 tar 归档。
— qwen3.8-max via Qwen Code /review (v0.22.0)
There was a problem hiding this comment.
This is real and I don't have a clean single-pass fix without predicting flatten's outcome before extraction even runs, which is its own design change. I improved the fail-closed diagnosis instead: the post-flatten walk's error messages now say "Extracted directory tree contains..." instead of "Tar archive contains...", since an oncall reading it was being pointed at the wrong artifact. The two-formalism duplication itself (pre-flatten archive-root scan vs. post-flatten realpath walk) is still there as defense-in-depth — leaving it as a documented known limitation rather than redesigning the extraction flow here. Happy to file a follow-up issue if you'd rather track it separately.
| if (validationError) return; | ||
| const entryPath = normalizeArchiveEntryPath(entry.path); | ||
| if (allowContainedSymlinks) { | ||
| if (archiveEntries.has(entryPath)) { |
There was a problem hiding this comment.
[Suggestion] R2-6: The duplicate-entry rejection branch still has no test anywhere in the suite — deleting this guard is a mutation the whole suite survives (re-verified by probe this round: crafted tars with literal duplicates foo+foo, dot-relative foo+./foo, and trailing-slash foo/+foo all hit the branch and reject with Tar archive contains duplicate entry path; with the branch deleted all three resolve silently while the full suite stays green). That matters because archiveEntries overwrites on duplicate: with the guard gone, a symlink's acceptance would track whatever the last duplicate entry records, so the pre-extraction scan's acceptance decision could rest on different metadata than intended, and rejection would be left entirely to the post-flatten realpath walk, which exists only in the fallback pipeline. Add a unit test, e.g.:
await writeCraftedTar(archive, [
createTarFileHeader('foo', 0),
createTarFileHeader('./foo', 0),
]);
await expect(
assertTarArchiveLinksAreSafe(archive, undefined, { allowContainedSymlinks: true }),
).rejects.toThrow('duplicate entry path');中文说明
"重复条目路径拒绝"分支在整个测试套件中仍然没有任何测试覆盖——删除这个守卫是一种整个套件都能"存活"的突变（本轮已用探针重新验证：构造的字面重复 foo+foo、点相对 foo+./foo、尾斜杠 foo/+foo 三种 tar 都会命中该分支并以 Tar archive contains duplicate entry path 拒绝；删除该分支后三者全部静默通过，而整套测试依旧全绿）。这一点要紧，因为 archiveEntries 在重复条目上是覆盖写入：守卫一旦缺失，符号链接的接受决定将跟随最后一个重复条目记录的元数据，解压前扫描的接受判定可能基于与预期不同的元数据做出，而拒绝就完全依赖 flatten 后的 realpath 遍历——该遍历只存在于回退安装流水线中。建议补一个单元测试（见上方代码）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
There was a problem hiding this comment.
Added a regression test for this branch (dot-relative duplicate foo/./foo).
|
|
||
| it('rejects a symlink resolving to an ancestor directory', async () => { | ||
| const archive = path.join(root, 'ancestor-cycle.tar'); | ||
| await writeCraftedTar(archive, [symlinkHeader('sub/loop', '..')]); |
There was a problem hiding this comment.
[Suggestion] R2-7: The two cycle-rejection tests still never reach the conditions their names claim to pin: symlinkHeader('self', '.') resolves against dirname('self') to '.', and symlinkHeader('sub/loop', '..') also resolves to '.' — both are rejected by the resolved === '.' clause, leaving the self/ancestor clauses in archive-safety.ts (normalizedEntry === resolved and normalizedEntry.startsWith(${resolved}/)) with zero discriminating tests. Round 2's mutation probe showed deleting those two clauses leaves the suite green while a crafted archive (regular file a + symlink a/b/link -> ..) flips from rejection to silent acceptance. The entry-path WINDOWS_ABSOLUTE_PATH.test(entryPath) clause has the same gap — only the link-target clause is exercised today. Add crafted-tar cases isolating each condition: symlinkHeader('link', 'link') (self-reference), symlinkHeader('a/b/link', '..') alongside createTarFileHeader('a', 0) (own ancestor that is a regular-file entry), and a symlink whose entry path is Windows-absolute.
中文说明
两个"环拒绝"测试仍然触达不到其名称声称要固定的条件：symlinkHeader('self', '.') 相对 dirname('self') 解析得到 '.'，symlinkHeader('sub/loop', '..') 同样解析为 '.'——两者都被 resolved === '.' 子句拒绝，导致 archive-safety.ts 中的自身/祖先子句（normalizedEntry === resolved 与 normalizedEntry.startsWith(${resolved}/)）没有任何判别性测试。第 2 轮的突变探针已表明：删除这两个子句后整套测试依旧全绿，而一个构造归档（常规文件 a + 符号链接 a/b/link -> ..）会从拒绝翻转为静默接受。入口路径侧的 WINDOWS_ABSOLUTE_PATH.test(entryPath) 子句存在同样缺口——目前只有链接目标侧的子句被覆盖。建议补充隔离每个条件的构造 tar 用例：symlinkHeader('link', 'link')（自引用）、symlinkHeader('a/b/link', '..') 加 createTarFileHeader('a', 0)（自身祖先是常规文件条目）、以及入口路径为 Windows 绝对路径的符号链接。
— qwen3.8-max via Qwen Code /review (v0.22.0)
There was a problem hiding this comment.
Retargeted both tests at inputs that actually reach their named clause — verified by mutation (deleting the ancestor clause now fails the ancestor test). The self-reference clause turned out to be structurally unreachable as a deciding factor: onReadEntry always records the entry itself in archiveEntries (typed SymbolicLink) before this check runs, so a link naming itself is always caught by the post-loop "target must be a distinct regular-file entry" scan regardless of this clause. Left a comment on that test explaining why it can't be made discriminating rather than claim it is.
| entry.linkpath, | ||
| ); | ||
| if (targetPath) { | ||
| retainedPathBytes += Buffer.byteLength(targetPath); |
There was a problem hiding this comment.
[Suggestion] R3-1: The identical retained-path-bytes accumulate-and-fail block is pasted twice inside assertTarArchiveLinksAreSafe — once for entry paths (~line 163) and once here for accepted symlink targets — character-for-character identical except the operand. Any change to the ceiling, the accounting, or the error message must now be made in two places in the same function; editing one and missing the other silently leaves entry paths and resolved link targets under different budgets — exactly the drift this limit exists to prevent, since the limit guards untrusted-archive memory retention. Extract one closure next to recordUnsupportedLink and call it at both sites:
// at each of the two sites (entry path ~line 163, accepted target ~line 216):
if (exceedsRetainedPathBudget(entryPath /* or targetPath */)) {
return;
}with the shared closure:
const exceedsRetainedPathBudget = (value: string): boolean => {
retainedPathBytes += Buffer.byteLength(value);
if (retainedPathBytes > MAX_ARCHIVE_PATH_BYTES) {
failValidation(
new Error(
`Tar archive path metadata exceeds ${MAX_ARCHIVE_PATH_BYTES} bytes.`,
),
);
return true;
}
return false;
};中文说明
完全相同的"保留路径字节数累加并失败"代码块在 assertTarArchiveLinksAreSafe 内部被粘贴了两次——一次针对条目路径（约第 163 行），一次在此处针对被接受的符号链接目标——除操作数外逐字符相同。今后对上限、计数方式或错误信息的任何修改都必须同时在同一函数的两处进行；只改一处而漏掉另一处，就会让条目路径与解析后的链接目标悄悄处于不同的预算之下——这正是该限额本要防止的漂移，因为该限额守护的是不可信归档的内存保留。建议在 recordUnsupportedLink 旁提取一个闭合并在这两处调用（见上方代码）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
There was a problem hiding this comment.
Extracted the shared exceedsRetainedPathBudget closure, used at both sites.
| } catch { | ||
| signal?.throwIfAborted(); | ||
| targetSize = undefined; | ||
| } |
There was a problem hiding this comment.
[Suggestion] R3-2: This bare catch discards the underlying filesystem error: lstat(rawTargetPath) and realpath(entryPath) above can fail for non-containment reasons (EMFILE under fd exhaustion, EACCES from a restrictive tmpdir mount, EIO on a flaky disk), and every such failure then surfaces as Tar archive contains unsupported link entry: <name>. Fail-closed is the right direction, but the diagnosis it produces is wrong for this class of failures — an oncall engineer sees what looks like a malicious/unsupported repo and audits the archive and the security policy, while the actual cause is a local resource problem; nothing in the error chain points at it. Probe at this commit: a valid contained link whose realpath was mocked to throw EMFILE was rejected as unsupported link entry: link with no cause/code anywhere in the error chain; attaching the error as cause flips the probe to carry Error: EMFILE: too many open files.
} catch (error) {
signal?.throwIfAborted();
targetSize = undefined;
// then include getErrorMessage(error) as `cause` on the
// "unsupported link entry" error thrown below
}(or rethrow non-ENOENT errors directly).
中文说明
这个裸 catch 丢弃了底层的文件系统错误：上方的 lstat(rawTargetPath) 与 realpath(entryPath) 可能因非 containment 原因失败（fd 耗尽时的 EMFILE、受限 tmpdir 挂载导致的 EACCES、磁盘不稳定导致的 EIO），而每一次这类失败最终都会以 Tar archive contains unsupported link entry: <name> 的形式出现。失败关闭的方向是对的，但对这一类失败而言它给出的诊断是错的——值班工程师看到的是"疑似恶意/不支持的仓库"，会去审查归档与安全策略，而真正原因是本地资源问题；错误链中没有任何信息指向真实原因。在本提交上的探针：一个合法的归档内链接，其 realpath 被 mock 为抛出 EMFILE，结果以 unsupported link entry: link 被拒绝，错误链中没有任何 cause/code；把错误作为 cause 附上后，探针翻转为携带 Error: EMFILE: too many open files。（或直接重新抛出非 ENOENT 错误，并在抛出的 "unsupported link entry" 错误上附带 getErrorMessage(error) 作为 cause。）
— qwen3.8-max via Qwen Code /review (v0.22.0)
There was a problem hiding this comment.
Now attaches the underlying stat/realpath error as cause on the thrown error, so an EMFILE/EACCES/EIO shows up in the error chain instead of reading like a hostile archive.
| if (installMetadata.type !== 'link') { | ||
| await copyExtension(localSourcePath, stagingPath, { | ||
| skipSymlinks: isAgentPlugin, | ||
| skipSymlinks: isAgentPlugin && !archiveSymlinksValidated, |
There was a problem hiding this comment.
[Suggestion] R3-3: archiveSymlinksValidated switches copyExtension into symlink-following mode based on validation of the pre-conversion extracted tree, but this copy runs on the post-conversion tree (localSourcePath may be a converter's new directory), and nothing in this diff documents or enforces that conversion preserves the validated layout — even though the allowContainedSymlinks JSDoc in archive-safety.ts tells callers that move or flatten the extracted tree to re-run assertDirectorySymlinksAreSafe on the final layout, which this flow does not do. Today this is safe only because every converter reachable from the fallback either keeps the directory unchanged (AgentPlugins) or materializes symlinks under its own confinement (Gemini/Claude/Qoder copyDirectory); precedent for a link-preserving restructure already exists in this family (claude-converter.ts's fs.promises.cp(sourcePath, pluginDir, { recursive: true }) copies links without dereferencing). A future converter that preserves symlinks while restructuring would silently invalidate the validation: copyExtension with dereference: true and a stat-following filter follows whatever links are present and would materialize an escaping link's target into the installed extension. Either re-run assertDirectorySymlinksAreSafe(localSourcePath, signal) immediately before copyExtension when the flag is set and the source moved, or at minimum state the load-bearing invariant in a comment at the flag declaration: conversion must either leave the validated tree in place or materialize all symlinks.
中文说明
archiveSymlinksValidated 基于对"转换前"解压目录树的校验，把 copyExtension 切换为跟随符号链接的模式，但这里的复制作用于"转换后"的目录树（localSourcePath 可能是某个转换器新建的目录），而本 diff 中没有任何内容记录或强制"转换必须保持已校验布局"这一前提——尽管 archive-safety.ts 中 allowContainedSymlinks 的 JSDoc 明确要求移动或扁平化解压树的调用方对最终布局重新运行 assertDirectorySymlinksAreSafe，而本流程并没有这样做。目前之所以安全，仅因为从回退路径可达的每个转换器要么保持目录不变（AgentPlugins），要么在自己的逃逸防护下把符号链接实体化（Gemini/Claude/Qoder 的 copyDirectory）；而保留链接的目录重构在本文件家族中已有先例（claude-converter.ts 的 fs.promises.cp(sourcePath, pluginDir, { recursive: true }) 复制链接时不做解引用）。未来某个在重构时保留符号链接的转换器会悄悄使校验失效：copyExtension 以 dereference: true 加跟随 stat 的 filter 复制时，会跟随现存的任何链接，把逃逸链接的目标内容实体化进已安装的扩展。建议在标志为真且源目录已移动时，于 copyExtension 之前重新运行 assertDirectorySymlinksAreSafe(localSourcePath, signal)；至少应在标志声明处加注释说明这一承重不变量：转换要么保持已校验目录树不变，要么把所有符号链接实体化。
— qwen3.8-max via Qwen Code /review (v0.22.0)
There was a problem hiding this comment.
Added a re-validation call: when archiveSymlinksValidated is set, the install is an AgentPlugins install, and conversion swapped localSourcePath away from the validated directory, assertDirectorySymlinksAreSafe now re-runs against the actual post-conversion tree before copyExtension trusts it. No converter reachable today triggers this, but it no longer relies on that staying true.
- isContainedPath now delegates to the shared isSubpath helper instead of duplicating its logic (a third near-identical copy already existed in zip-extraction.ts). - The retained-path-byte budget accounting for entry paths and accepted symlink targets was pasted twice; extract one exceedsRetainedPathBudget closure so a future change to the ceiling can't drift between the two. - assertDirectorySymlinksAreSafe's bare catch discarded the real filesystem error (EMFILE/EACCES/EIO) behind a generic "unsupported link entry" message; attach it as `cause` so a resource failure isn't misdiagnosed as a hostile archive. Reworded that function's two error messages from "Tar archive contains" to "Extracted directory tree contains" since they fire from a directory walk, not a tar read (updates the one existing test asserting the old wording). - Added a regression test for the duplicate-entry-path rejection branch, which had none. - The two existing "self/ancestor cycle" tests never actually reached the clauses their names claimed (both fell through the earlier `resolved === '.'` check). Retargeted them at inputs that isolate each clause, added a case for the entry-path Windows-absolute check, and left a comment on the self-reference test explaining why it can't be made discriminating (the entry always shadows itself in archiveEntries before the check runs, so the post-loop "target must be a distinct regular-file entry" scan rejects it either way). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…idated source archiveSymlinksValidated is only ever proven true for the pre-conversion tree (set right after downloadPublicGitHubArchiveFallback's post-flatten walk). copyExtension trusted that flag for whatever localSourcePath happened to be by the time it ran, even when convertCompatibleExtension had since pointed it at a different, never-validated directory. Today every reachable converter either leaves the tree unchanged (AgentPlugins) or materializes symlinks while copying (Gemini/Claude/ Qoder), so nothing currently exploits this — but that's not an invariant the type system or a comment enforces. Re-run assertDirectorySymlinksAreSafe on the post-conversion tree in the one case where it would matter: the flag is set, the extension is an AgentPlugins install (the only case skipSymlinks depends on the flag), and conversion actually swapped the directory. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Addressed all 7 round-3 suggestions (replies inline on each). R2-2's deeper two-formalism duplication is left as a documented known limitation rather than redesigned — see inline reply. New/changed tests pass, plus the full |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed. No new issues found — all 7 round-3 findings have been addressed by the author's self-review commits.
Verified changes since round 3 (commit 3e2f352):
- R2-1 (isContainedPath copy): now delegates to the shared utility ✓
- R2-2 (containment vs archive root): acknowledged; post-flatten mitigates ✓
- R2-6 (no test for duplicate entry): test added for duplicate ✓
- R2-7 (cycle tests never reach conditions): retargeted to inputs that actually reach their named clause ✓
- R3-1 (retained-path-bytes block duplicated): extracted to shared closure ✓
- R3-2 (bare catch discards error): underlying stat/realpath error attached as ✓
- R3-3 (archiveSymlinksValidated trust): re-validation added when conversion moves the source ✓
New observations (no new findings):
- now uses binary search (O(log n)) instead of linear scan — the R2-3 concern is resolved.
- The containment logic is correct: uses + empty-relative check, and operates on POSIX-normalized paths with defense-in-depth checks for backslash, absolute, and Windows drive-qualified paths.
- The post-extraction validates the final layout after flattening, which is the correct place to catch symlink re-targeting.
- Test coverage is comprehensive: contained symlinks (accepted), escaping, absolute, Windows-absolute, UNC, backslash, hard links, duplicates, cycles, dangling, directory targets, descendant entries, and the option-off backstop are all tested.
Not verified (infrastructure): build-and-test could not run locally — git fetch is blocked on this Windows machine (known infrastructure issue). CI has landed green.
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed. No new issues found — all 7 round-3 findings have been addressed by the authors self-review commits.
Verified changes since round 3 (commit 3e2f352):
- R2-1 (isContainedPath copy): now delegates to the shared isSubpath utility
- R2-2 (containment vs archive root): acknowledged; post-flatten assertDirectorySymlinksAreSafe mitigates
- R2-6 (no test for duplicate entry): test added for ./foo duplicate
- R2-7 (cycle tests never reach conditions): retargeted to inputs that actually reach their named clause
- R3-1 (retained-path-bytes block duplicated): extracted to shared exceedsRetainedPathBudget closure
- R3-2 (bare catch discards error): underlying stat/realpath error attached as cause
- R3-3 (archiveSymlinksValidated trust): re-validation added when conversion moves the source
New observations (no new findings):
- hasArchiveDescendant now uses binary search (O(log n)) instead of linear scan — the R2-3 concern is resolved.
- The containment logic is correct: isContainedPath uses isSubpath + empty-relative check, and resolveContainedSymlinkTarget operates on POSIX-normalized paths with defense-in-depth checks for backslash, absolute, and Windows drive-qualified paths.
- The post-extraction assertDirectorySymlinksAreSafe validates the final layout after flattening, which is the correct place to catch symlink re-targeting.
- Test coverage is comprehensive: contained symlinks (accepted), escaping, absolute, Windows-absolute, UNC, backslash, hard links, duplicates, cycles, dangling, directory targets, descendant entries, and the option-off backstop are all tested.
Not verified (infrastructure): build-and-test could not run locally — git fetch is blocked on this Windows machine (known infrastructure issue). CI has landed green.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:
packages/core/src/extension/github.ts:1393 — [probe] Windows fallback without symlink privilege fails late with a cryptic EPERM extraction error instead of the pre-PR early named rejection
Convergence: round 4 posted 1 inline comment(s), 1 of them reported for the first time; the previous round posted 7 (3 new). Findings keep coming back to the same files: packages/core/src/extension/extensionManager.ts (findings in round 3; 1 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. No Critical finding is open on this round, so merging and moving the remaining Suggestion threads to a follow-up issue is available as an ending — a merged pull request cannot diverge further. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
已审查。 建议见行内评论。
收敛姿态下延后（第 4 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 4 轮发布了 1 条行内评论，其中 1 条是首次提出；上一轮发布了 7 条（其中 3 条首次提出）。发现反复回到同一批文件：packages/core/src/extension/extensionManager.ts（第 3 轮已出过发现，本轮又有 1 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。本轮没有未决的 Critical，因此"合入后把剩余 Suggestion 线程转到后续 issue"是一个可选的结束方式——已合入的 PR 不会继续发散。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.0)
| if ( | ||
| archiveSymlinksValidated && | ||
| isAgentPlugin && | ||
| localSourcePath !== sourceBeforeConversion | ||
| ) { |
There was a problem hiding this comment.
[Suggestion] The re-validation guard added by this round's fix commit is unreachable: isAgentPlugin is only true when convertCompatibleExtension returns the input directory unchanged (extension-converter.ts assigns 'AgentPlugins' only in the branch that never reassigns newExtensionDir), so whenever isAgentPlugin holds, localSourcePath !== sourceBeforeConversion is false — and every converter branch that actually moves the tree returns a different originSource, so for those the guard short-circuits on isAgentPlugin. The protection the comment promises (a converter that restructures the tree while preserving symlinks would otherwise carry stale trust onto a directory that was never actually checked) therefore never fires, and converted trees are exactly the ones installed with skipSymlinks: false, where copyExtension runs with dereference: true and would ingest any symlink such a converter leaves behind without re-validation. There is no live exploit today — every current tree-moving converter materializes links via copyFileSync with its own confinement — the cost is a dead guard and a comment that promises a protection the code cannot provide, which will mislead the next maintainer who adds a link-preserving converter. Dropping the isAgentPlugin conjunct makes the guard fire exactly in the case its comment describes.
| if ( | |
| archiveSymlinksValidated && | |
| isAgentPlugin && | |
| localSourcePath !== sourceBeforeConversion | |
| ) { | |
| if ( | |
| archiveSymlinksValidated && | |
| localSourcePath !== sourceBeforeConversion | |
| ) { |
中文说明
本轮修复提交新增的重新校验守卫实际不可达：只有当 convertCompatibleExtension 原样返回目录时 isAgentPlugin 才为真（extension-converter.ts 只在从不改动 newExtensionDir 的分支里赋值 'AgentPlugins'），因此 isAgentPlugin 成立时 localSourcePath !== sourceBeforeConversion 必为假；而任何真正移动目录树的转换器分支都返回不同的 originSource，对这些分支守卫会先在 isAgentPlugin 上短路。注释承诺的保护（重构目录树但保留符号链接的转换器会带着过期信任进入未经校验的目录）因此永远不会触发；而被转换的目录恰恰以 skipSymlinks: false 安装，copyExtension 以 dereference: true 运行，会解引用此类转换器留下的任何符号链接且不再重新校验。当前没有可利用的现实路径——现有移动目录树的转换器都通过自带范围限制的 copyFileSync 把链接实体化——代价是一个失效的守卫和一段承诺了代码无法提供之保护的注释，会误导下一个新增保留链接的转换器的维护者。去掉 isAgentPlugin 条件即可让守卫恰好在注释描述的场景触发。
— qwen3.8-max via Qwen Code /review (v0.22.0)
…able isAgentPlugin is true only when convertCompatibleExtension left the source directory unchanged (extension-converter.ts's AgentPlugins branch never reassigns its output dir), so gating the previous commit's re-validation guard on isAgentPlugin as well made it unreachable: every conversion branch that actually moves the tree (Gemini, Claude, Qoder) sets a different originSource, so isAgentPlugin is always false exactly when localSourcePath !== sourceBeforeConversion is true. Drop that conjunct so the guard fires in the case its own comment describes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and its suite did not run locally.
Deferred under the convergence posture (round 5, not a blocker) — recorded, not requested in this round:
packages/core/src/extension/github.ts:966 — [probe] unpinned maxExpandedBytes wiring on the post-flatten layout check — deleting the argument passes 339/339 tests; the 1 GiB materialization ceiling rides on it alonepackages/core/src/extension/github.ts:970 — [probe] unpinned excludePath wiring on the post-flatten layout check — deleting the argument passes 339/339 tests; near-cap archives would false-rejectpackages/core/src/extension/archive-safety.ts:217 — [probe] link-target side of the 8 MiB path-metadata budget is untested — deleting the line passes 194/194 tests
Convergence: round 5 posted 3 inline comment(s), 3 of them reported for the first time; the previous round posted 1 (1 new). Findings keep coming back to the same files: packages/core/src/extension/extensionManager.ts (findings in round 4; 1 more now). The rate of new findings is not falling. A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. Batching the remaining fixes and verifying them before the next push, or dropping this PR's reviews to --severity-floor critical, keeps the loop from re-deriving the same set. No Critical finding is open on this round, so merging and moving the remaining Suggestion threads to a follow-up issue is available as an ending — a merged pull request cannot diverge further. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally。
未审查：build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and its suite did not run locally。
收敛姿态下延后（第 5 轮，非阻断）——已记录，本轮不要求修改：共 3 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 5 轮发布了 3 条行内评论，其中 3 条是首次提出；上一轮发布了 1 条（其中 1 条首次提出）。发现反复回到同一批文件：packages/core/src/extension/extensionManager.ts（第 4 轮已出过发现，本轮又有 1 条）。新发现的产出速度没有下降。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送，或将本 PR 的评审降到 --severity-floor critical，可以避免循环反复推导同一组发现。本轮没有未决的 Critical，因此"合入后把剩余 Suggestion 线程转到后续 issue"是一个可选的结束方式——已合入的 PR 不会继续发散。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // Clear call history only — mockImplementation (the real passthrough | ||
| // wired up in the vi.mock factory above) is set once at module load. | ||
| mockAssertDirectorySymlinksAreSafe.mockClear(); |
There was a problem hiding this comment.
[Suggestion] The real-passthrough implementation of mockAssertDirectorySymlinksAreSafe is attached once at module load — mockImplementation(actual.assertDirectorySymlinksAreSafe) on a bare vi.fn() inside the vi.mock('./archive-safety.js') factory — but afterEach's vi.restoreAllMocks() resets a bare vi.fn() back to a no-op, so from the second test in this file on, the mock silently resolves without ever calling the real assertDirectorySymlinksAreSafe, and the comment above mockClear() ("set once at module load") asserts a property that stops holding after the first test. The suite is green today (146/146), but every later test runs against a resolving no-op, so any test presuming the real validation ran passes for the wrong reason. Probe against the installed @vitest/spy 3.2.4:
PR wiring, second test after one vi.restoreAllMocks(): expected undefined to be 'real:ok'
creation-time-impl control arm: both tests pass
One fix shape — keep the real function in a hoisted holder and re-attach it per test (the factory and this beforeEach both change):
const realAssertDirectorySymlinksAreSafe = vi.hoisted(() => ({
current: undefined as
| undefined
| typeof import('./archive-safety.js')['assertDirectorySymlinksAreSafe'],
}));
// in the vi.mock factory, next to the existing mockImplementation:
realAssertDirectorySymlinksAreSafe.current = actual.assertDirectorySymlinksAreSafe;
// in beforeEach, replacing the comment + bare mockClear():
mockAssertDirectorySymlinksAreSafe.mockImplementation(
realAssertDirectorySymlinksAreSafe.current!,
);
mockAssertDirectorySymlinksAreSafe.mockClear();中文说明
mockAssertDirectorySymlinksAreSafe 的真实透传实现只在模块加载时挂载一次（在 vi.mock('./archive-safety.js') 工厂里对裸 vi.fn() 调用 mockImplementation(actual.assertDirectorySymlinksAreSafe)），但 afterEach 中的 vi.restoreAllMocks() 会把裸 vi.fn() 重置为空实现，因此从本文件的第二个测试开始，该 mock 会静默直接 resolve、不再调用真实的 assertDirectorySymlinksAreSafe；mockClear() 上方注释（“在模块加载时设置一次”）所断言的性质在第一个测试之后即不再成立。当前套件是绿的（146/146），但其后每个测试实际面对的都是一个只会 resolve 的空实现，任何假定真实校验已执行的测试都会因错误的原因通过。已用本机 @vitest/spy 3.2.4 探针验证：
PR 接线，经过一次 vi.restoreAllMocks() 后的第二个测试：expected undefined to be 'real:ok'
创建时传入实现的对照组：两个测试均通过
一种修复方式：用 hoisted 持有者保存真实函数，并在每个测试前重新挂载（工厂与此处 beforeEach 都需要改动，见上方英文部分的 TypeScript 代码块）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| expect(mockAssertDirectorySymlinksAreSafe).toHaveBeenCalledWith( | ||
| expect.not.stringMatching(new RegExp(`^${fallbackDestination}$`)), | ||
| undefined, | ||
| ); |
There was a problem hiding this comment.
[Suggestion] This wiring assertion builds a RegExp from an unescaped filesystem path. On Windows, fallbackDestination is shaped like C:\Users\RUNNER~1\AppData\Local\Temp\..., and fed raw into new RegExp('^...$') the backslashes become regex escapes (\T a tab escape, \U an identity escape), so the pattern can never match the literal path and expect.not.stringMatching(...) passes for ANY first argument — including sourceBeforeConversion itself. The repo runs this suite on Windows (ci.yml test_windows), so if the guard regressed to re-validating the pre-conversion directory — the exact stale-trust bug this test exists to catch — the suite would still be green there; the test discriminates the regression only on POSIX. Probe against Windows-shaped paths:
re.test() against the very path it was built from → false
not.stringMatching(re) accepted the regression argument on the Windows shape; rejected it on the POSIX shape
| expect(mockAssertDirectorySymlinksAreSafe).toHaveBeenCalledWith( | |
| expect.not.stringMatching(new RegExp(`^${fallbackDestination}$`)), | |
| undefined, | |
| ); | |
| expect(mockAssertDirectorySymlinksAreSafe).toHaveBeenCalledWith( | |
| expect.not.stringMatching( | |
| `^${fallbackDestination.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}$`, | |
| ), | |
| undefined, | |
| ); |
中文说明
该接线断言用未转义的文件系统路径构造 RegExp：在 Windows 上 fallbackDestination 形如 C:\Users\RUNNER~1\AppData\Local\Temp\...，直接传入 new RegExp('^...$') 时反斜杠会变成正则转义（\T 成为制表符转义、\U 成为恒等转义），该模式永远无法匹配字面路径，于是 expect.not.stringMatching(...) 对任意第一个参数都通过——包括 sourceBeforeConversion 本身。本仓库在 Windows 上运行该套件（ci.yml 的 test_windows），因此如果守卫回归为“对转换前的目录重新校验”——正是本测试要防的过期信任缺陷——在 Windows 上套件仍会是绿的；该测试只在 POSIX 上能区分这一回归。针对 Windows 形状路径的探针：
re.test() 对其构造来源的路径本身 → false
not.stringMatching(re) 在 Windows 形状路径上接受了回归参数；在 POSIX 形状上拒绝
上方 ```suggestion 代码块即为修复：构造正则前先转义路径中的正则元字符。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| await assertDirectorySymlinksAreSafe(localSourcePath, signal); | ||
| } |
There was a problem hiding this comment.
[Suggestion] This round's fix commit made the re-validation guard reachable — good. But its rejection branch is exercised by no test: the existing 're-validates symlinks on the post-conversion tree…' test pins only that the guard CALLS the validator on a relocated directory; nothing makes the validator fail and asserts the install aborts. A swallowing mutant at this call site passes the whole suite:
mutant: try { await assertDirectorySymlinksAreSafe(localSourcePath, signal); } catch {} → Tests 146 passed (146)
The install would proceed carrying exactly the stale trust this guard exists to prevent. (The more literal .catch(() => {}) mutant fails today only accidentally — a TypeError on undefined caused by the separate mock-passthrough issue flagged in extensionManager.test.ts — and this gap fully reopens the moment that is fixed and the mock returns a real promise.) Add a fallback→converter scenario where the validator rejects (e.g. mockRejectedValueOnce(new Error('unsupported link entry')) after the validation-phase passthrough) and assert installExtension fails rather than producing an installed extension.
中文说明
本轮的修复提交让重新校验守卫变得可达——这很好。但其拒绝分支仍然没有任何测试覆盖：现有的 're-validates symlinks on the post-conversion tree…' 测试只固定了“守卫会在发生迁移的目录上调用校验器”这一点；没有任何用例让校验器失败并断言安装被中止。在该调用点应用一个吞掉异常的突变体后，整个套件依然通过：
突变体：try { await assertDirectorySymlinksAreSafe(localSourcePath, signal); } catch {} → Tests 146 passed (146)
也就是说，安装会带着该守卫本要防止的“过期信任”继续完成。（更字面的 .catch(() => {}) 突变体目前只是意外失败——因 extensionManager.test.ts 中另一处 mock 透传问题导致对 undefined 调用 .catch 而抛出 TypeError——一旦那个问题被修复、mock 返回真正的 Promise，该覆盖缺口就会完全重新暴露。）建议补充一个 回退→转换器 场景：让校验器拒绝（例如在校验阶段的透传之后使用 mockRejectedValueOnce(new Error('unsupported link entry'))），并断言 installExtension 失败而不是产出已安装的扩展。
— qwen3.8-max via Qwen Code /review (v0.22.0)
- mockAssertDirectorySymlinksAreSafe's real-passthrough implementation was only attached once at module load; afterEach's vi.restoreAllMocks() resets a bare vi.fn() back to a no-op, so from the second test in this file on, the mock silently resolved without running real validation. Hold the real implementation in a hoisted reference and re-attach it in beforeEach on every test. - The Windows-path regex wiring assertion built a RegExp from an unescaped filesystem path; on Windows the backslashes in fallbackDestination read as regex escapes, so the pattern could never match and the assertion passed for any argument, silently losing its ability to catch the regression it exists to catch on that platform. Escape regex metacharacters before constructing the pattern. - The re-validation guard's rejection path had no test: only "the validator is called" was pinned, not "a rejecting validator aborts the install." Add a case where the validator rejects and assert installExtension fails rather than completing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- unpinned maxExpandedBytes wiring on the post-flatten layout check (github.ts:964-972) — already reported as a round-5 deferral (review 5029163621)
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and its suite did not run locally.
Test Plan (not a blocker): 168 passed — this review observed 21642, 1702, 24736, 1659, 601, 4235, 630 passed; 23736 passed — this review observed 21642, 1702, 24736, 1659, 601, 4235, 630 passed.
Deferred under the convergence posture (round 6, not a blocker) — recorded, not requested in this round:
packages/core/src/extension/archive-safety.ts:319 — [probe] rawTargetPath raw-concatenation is load-bearing but unexplained
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally。
未审查：build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and its suite did not run locally。
Test Plan（非阻断）：168 passed — this review observed 21642, 1702, 24736, 1659, 601, 4235, 630 passed; 23736 passed — this review observed 21642, 1702, 24736, 1659, 601, 4235, 630 passed。
收敛姿态下延后（第 6 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
@qwen-code /triage |
|
Sandboxed verification: Skipped because the pre-execution risk screen refused this sponsored run: the model risk screen failed or returned an unparseable answer. A maintainer who has reviewed the diff can run the verification manually in a disposable environment. 中文 — 判定：
|
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. ✅
Maintainer verification — built and ran this locally against a real stackI rebuilt both sides of this PR and drove real Test bed
1. The bug reproduces on base and is fixed on head
Base fails with exactly the error from #9724 ( 2. Archive-safety matrix — 22 archives × 2 builds, one real install each
5 benign archives flip from reject → install. 13 hostile archives stay rejected on head, canaries intact, no write ever lands outside the extraction root. Two limit cases behave as documented (100-link cap, 1 GiB materialized-bytes cap). Fixtures are hand-written ustar streams so entry type/order/duplicates are byte-controlled; 3. The post-flatten filesystem scan is load-bearing — not redundantThis is the finding I most want on the record, because it justifies the extra layer the review rounds added.
I built
So moving Two more mutants map the rest of the layering (all still rejected, so the layers genuinely compose):
4. The fallback now installs what a Git ≥ 2.37 clone installs
Same repository, same commit, served both as a git remote and as a codeload tarball. Installed trees are identical (paths, types, per-file sha256) apart from 5.
|
| 被测代码路径 | qwen serve 的 POST /workspace/extensions/install —— 只有它会设置 networkPolicy: 'public'，因此也只有它能触达 downloadPublicGitHubArchiveFallback（CLI 的 qwen extensions install 从不设置该值） |
| 旧 Git 宿主 | node:22-bullseye → Git 2.30.2（< 2.37，shouldUsePublicGitHubArchiveFallback 为真；与 #9724 中 Ubuntu 22.04 / Git 2.34.1 同类） |
| 新 Git 宿主 | node:22-bookworm → Git 2.39.5（clone 路径，用于下面的对比） |
| GitHub | api.github.com + codeload.github.com + github.com 由本地源站提供，绑在公网段（不在黑名单内）IP 上并配真实 TLS 证书，因此 resolveNetworkTarget 的 DNS/黑名单校验是被真正执行而非绕过的；github.com 跑真实 git http-backend，clone 路径是货真价实的 HTTPS git clone --depth 1 |
| 对比构建 | base = 0756be0ce7（merge-base，未打本 PR） vs head = e0e653061e（PR #9969）。git diff base..head 恰好就是本 PR 的 9 个文件 |
| 金丝雀 | 在 /、/tmp、/root、/home、/var/tmp 预置文件，每轮结束校验其摘要，并全盘搜 PWNED.txt |
1. base 上能复现，head 上已修复
base 报出的正是 #9724 的原始错误（Tar archive contains 2 unsupported link entries）。head 安装成功，且 AGENTS.md 与 CLAUDE.md 的 sha256 一致。
2. 归档安全矩阵 —— 22 个归档 × 2 个构建，每格一次真实安装
5 个良性归档从「拒绝」翻转为「安装」；13 个恶意归档在 head 上依然全部拒绝，金丝雀完好，任何一次运行都没有写到解压根之外。两个限额用例行为与文档一致（100 链接上限、1 GiB 物化字节上限）。case-abs-entry 是唯一一行 head 比 base 更严的：base 会静默接受带绝对路径条目的归档（node-tar 把 /tmp/ABS_PWNED.txt 相对化后放进扩展目录），head 在新的 strict: true 下直接拒绝。
夹具是手写的 ustar 字节流，条目类型／顺序／重复项都可精确控制；另有 case-real-safe（GNU tar czf 打包真实带符号链接的目录树）和 case-git-archive（git archive 输出，含 codeload 会带的 pax_global_header）来证明手写夹具具有代表性。
3. flatten 之后的文件系统复检是承重墙，不是冗余
这是我最想留档的一条结论，因为它正当化了评审轮次中新增的那一层。
我构造了 case-flatten-escape：tar 根部放一个与包装目录平级的 SECRET.txt，再加 case-flatten-escape-<sha>/AGENTS.md -> ../SECRET.txt。条目路径预扫描会接受它 —— 目标在 tar 内解析得到，且是归档中的普通文件条目。但随后 flattenSingleExtensionDirectory() 剥掉包装目录，同一条链接就指向了解压根的上一级（/tmp/SECRET.txt）。
- head 原样：安装失败关闭 ——
Extracted directory tree contains unsupported link entry: AGENTS.md，无泄漏。 - head 只改一行（让
assertDirectorySymlinksAreSafe()直接 return）：安装成功，且安装后的扩展里装着从解压根之外读到的宿主文件内容。
所以把 flattenSingleExtensionDirectory 挪进受保护块并复检最终布局，不是双保险，而是这一类攻击唯一的闸门。
另外两个变异体刻画了其余分层（全部仍被拒绝，说明各层确实互补）：
| 变异 | 效果 |
|---|---|
| 去掉「目标必须是归档内普通文件 / 不得有后代条目」的后置规则 | escape-parent、dir-symlink、symlink-chain、flatten-escape 下沉到 flatten 后复检，仍被拒绝 |
去掉词法层的 ../ / 自引用拒绝 |
escape-deep 等下沉到归档成员规则，仍被拒绝 |
4. 回退路径现在装出来的东西和 Git ≥ 2.37 clone 一致
同一仓库、同一 commit，一边作为 git 远端、一边作为 codeload 归档提供。除 .qwen-extension-install.json 外，安装后的目录树完全一致（路径、类型、逐文件 sha256）。两条路径都把符号链接物化成普通文件副本，因为 copyExtension 在两边都是 fs.cp(..., dereference: true)。这点值得写明：设计文档里把「把链接物化成文件副本」列为被否决的方案，但最终落盘状态本来就是物化的；本 PR 修的是回退路径不再在到达那一步之前就拒收归档。
5. strict: true 的回归探针
strict: true 会把 node-tar 的任何告警升级为硬失败，所以我针对真实 codeload 归档的形态做了探针：
pax_global_header（git archive／codeload 都会带）→ 安装正常- 206 字节的条目路径（超出 ustar 100 字节 name 字段）→ 安装正常
- 非 ASCII 文件名（
中文-文件名-ünïcödé.md）→ 安装正常 - 向上三级但仍在范围内的链接（
.../up-to-root.md -> ../../../CLAUDE.md）→ 被接受
6. 测试套件
- PR head 上跑
packages/core/src/extension/{archive-safety,github,extensionManager}.test.ts：340 通过 - 把 PR 与当前
origin/main合并后再跑同样三个套件：340 通过；packages/core的tsc --noEmit干净
给作者的几点说明 —— 都不构成阻塞
strict: true的影响范围超出了符号链接本身。 它现在作用于该回退路径上的每一个归档，包括完全没有链接的。上面的探针覆盖了我能想到的形态，但需要意识到：此前可安装、却会触发任何 tar 告警的归档，现在会安装失败。矩阵里的case-abs-entry就是这种变化的一个实例。- Windows。
cloneFromGit在 win32 上刻意设置core.symlinks=false，让 git 把链接物化成文件；归档路径没有对应处理，且在strict: true下fs.symlink的EPERM会变成硬失败而不是跳过该条目。文档改动里已经写了「可能需要开发者模式」，所以两者是自洽的 —— 但这意味着在原生 Windows 主机上obra/superpowers可能仍装不上，也就是这个修复在那里可能不生效。这不是回归（base 本来就拒绝该归档），提出来只是想确认这是有意识的取舍。 - 100 链接上限现在把被接受的链接也计入。 已验证：99 条范围内符号链接可安装，150 条被拒。
introduction.md的改动已正确记录；只是提醒一句，这对良性仓库同样是实打实的天花板，而不只是针对恶意归档。
本轮未覆盖： Windows 与 macOS 宿主（macOS 文件系统大小写不敏感，而归档成员查找是大小写敏感的）、zip 归档（本 PR 未触及），以及任何需要真实 github.com 的场景。
结论
无阻塞发现。所报故障在 base 上可复现、在 head 上消失；我能构造出的所有逃逸形态都被拒绝且没有任何写入落到解压根之外；分层防御是用变异测试而非阅读代码验证的；回退路径的安装结果现在与 clone 路径逐字节一致。我这边 LGTM，建议合入。
4 participants
What this PR does
The public GitHub extension archive fallback (used to install extensions on Git older than 2.37) now accepts symbolic-link entries whose targets provably resolve inside the archive, instead of rejecting every tar link entry outright. Absolute targets, Windows drive-qualified targets,
..escapes, and hard links are still rejected everywhere, including on this path.Why it's needed
A repository that ships an ordinary in-repo symlink could not be installed through the older-Git archive fallback at all. The reported repro,
obra/superpowers, ships a rootAGENTS.md -> CLAUDE.md, and GitHub's codeload archives preserve that link —assertTarArchiveHasNoLinksrejected every link entry unconditionally, so the install failed with no way around it short of upgrading Git.Reviewer Test Plan
How to verify
Containment is decided from the archive's own entry paths before extraction begins (
isContainedSymlinkTargetinarchive-safety.ts), so no link is ever followed and nothing is written while deciding. The newallowContainedSymlinksoption is opt-in and wired only intodownloadPublicGitHubArchiveFallback; local extension installs, npm registry installs, and release archives keep the pre-existing fail-closed behavior of rejecting every link. Hard links stay unsupported on every path, opt-in or not, since a hard-link entry names another archive entry rather than a filesystem path and needs a different containment argument than the one made here.Evidence (Before & After)
N/A — non-UI change (extension install internals + docs).
npx vitest run packages/core/src/extension/archive-safety.test.ts packages/core/src/extension/github.test.ts→ 168 passed, 0 failed, including new cases for a contained symlink (accepted), a symlink escaping the archive root via../(rejected), an absolute-path target (rejected), a Windows drive-qualified target (rejected), and a hard link pointing inside the archive root (still rejected).Full-repo preflight run separately against a clean
npm cicheckout of this branch: 23736 passed, 13 failed. All 13 are pre-existing CLI flakes unrelated to this change (12AuthDialog+ 1 ACP transport test), reproduced identically against unmodifiedorigin/main.Tested on
Not tested on Windows/Linux locally. The change is Node-only path logic (no OS-specific extraction calls) and includes an explicit test case for Windows-style drive-qualified targets.
Environment (optional)
npx vitest run, local Node/npm. No live GitHub archive download — archive-safety and extraction are tested against synthetic in-memory tar streams, matching the existing test style in these files.Risk & Scope
Linked Issues
Fixes #9724
中文说明
本 PR 的作用
面向低于 2.37 版本 Git 的 GitHub 扩展归档回退安装路径，现在会接受目标确实解析到归档内部的符号链接条目，而不再无条件拒绝所有 tar 链接条目。绝对路径目标、Windows 盘符路径目标、
..逃逸路径，以及硬链接，在这条路径上依然会被拒绝。为什么需要
带有普通仓库内符号链接的仓库此前完全无法通过旧版 Git 的归档回退安装。复现案例
obra/superpowers在根目录有AGENTS.md -> CLAUDE.md链接，GitHub 的 codeload 归档会保留该链接；而assertTarArchiveHasNoLinks会无条件拒绝所有链接条目，导致安装失败，且除升级 Git 外别无他法。验证
containment 判断在解压开始前，仅依据归档自身的条目路径完成（
archive-safety.ts中的isContainedSymlinkTarget），因此判断过程中不会跟随任何链接，也不会写入任何文件。新增的allowContainedSymlinks选项默认关闭，只在downloadPublicGitHubArchiveFallback中启用；本地扩展安装、npm 仓库安装、以及 release 归档安装保持原有的拒绝所有链接的失败关闭行为不变。硬链接在所有路径上都仍不受支持：硬链接条目指向的是归档内的另一个条目而非文件系统路径，需要与本次改动不同的 containment 论证方式。运行
npx vitest run packages/core/src/extension/archive-safety.test.ts packages/core/src/extension/github.test.ts：168 个测试全部通过，新增用例覆盖归档内符号链接（接受）、通过../逃逸归档根目录的符号链接（拒绝）、绝对路径目标（拒绝）、Windows 盘符路径目标（拒绝），以及指向归档内部的硬链接（仍拒绝）。另在该分支的干净
npm cicheckout 上单独运行了全量 preflight：23736 个测试通过，13 个失败；这 13 个失败均为与本次改动无关的既有 CLI flaky 用例（12 个AuthDialog+ 1 个 ACP transport），在未修改的origin/main上同样能复现。风险与范围