fix(cli): allow double dots in update archives - #5521
Conversation
|
📌 Same maintainer note — full version on #5509. This is the |
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Hi @tt-a1i — thanks for the fix! The change itself looks correct and focused, but the PR body doesn't follow our PR template. Could you update the description to include the required sections?
Mainly missing:
- What this PR does / Why it's needed — a prose description of the change and motivation
- Reviewer Test Plan — How to verify, Evidence (Before & After), and the Tested-on table so reviewers can confirm the fix
- Risk & Scope — what's in/out of scope, any tradeoffs
- Linked Issues —
Fixes #5520should go in this section (it's in the body but not under a dedicated heading) - 中文说明 — a Chinese translation in a
<details>block
This helps reviewers move faster and is required for merge. The actual code change looks fine — just the description needs updating. 🙏
中文说明
@tt-a1i 你好,感谢修复!代码改动本身没问题,但 PR 描述没有按照我们的 PR 模板 填写。麻烦补充以下必填章节:
- What this PR does / Why it's needed — 用文字描述改动内容和动机
- Reviewer Test Plan — 验证步骤、Before/After 证据、测试过的操作系统表格
- Risk & Scope — 范围、取舍、风险
- Linked Issues —
Fixes #5520放在这个章节下 - 中文说明 —
<details>块里的中文翻译
这些帮助 reviewer 更快审查,也是合并的必要条件。代码本身没问题,只需要更新描述。🙏
— Qwen Code · qwen3.7-max
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Hey @tt-a1i, thanks for the PR! The body doesn't follow our PR template — a few required sections are missing or renamed:
## What this PR doesand## Why it's needed— currently combined as## Summary. Please split into the two template headings.## Reviewer Test Plan— missing entirely. This should include### How to verify,### Evidence (Before & After), and### Tested on(OS table).## Risk & Scope— missing.## Linked Issues— missing (theFixes #5520reference should live under this heading).<details>中文说明</details>— missing bilingual section.
Please update the body to match the template and re-request review. The code change itself looks reasonable — this is purely a process gate. 🙏
中文说明
嗨 @tt-a1i,感谢提交!PR 正文没有按照 PR 模板 填写——部分必填章节缺失或被改名了:
## What this PR does和## Why it's needed— 目前合并成了## Summary,请拆成模板中的两个标题。## Reviewer Test Plan— 完全缺失,应包含### How to verify、### Evidence (Before & After)和### Tested on(操作系统表格)。## Risk & Scope— 缺失。## Linked Issues— 缺失(Fixes #5520应放在此章节下)。<details>中文说明</details>— 缺失双语说明。
请按模板更新正文后重新请求 review。代码改动本身没问题——这纯粹是流程关卡。🙏
— Qwen Code · qwen3.7-max
|
@qwen-code /triage |
|
Thanks for the PR, @tt-a1i! Template looks good ✓ On direction: This is a straightforward bug fix — the old On approach: The scope is tight — +9 lines of logic, +20 lines of tests, one file changed. The One thing to flag: @wenshao has already noted that this is part of a family of path-boundary fixes (#5515, #5458/#5460) and would prefer consolidating them + reusing the shared Moving on to code review and testing. 🔍 中文说明感谢贡献,@tt-a1i! 模板完整 ✓ 方向: 这是一个直接的 bug 修复——旧的 方案: 范围紧凑——+9 行逻辑、+20 行测试,仅修改一个文件。 需要指出的是:@wenshao 已经指出这是一系列路径边界修复的一部分(#5515、#5458/#5460),建议合并它们并复用 进入代码审查和测试 🔍 — Qwen Code · qwen3.7-max |
Code ReviewThe implementation is clean and correct. No blockers found.
The test suite covers the right cases: double dots inside filename segments (allowed), parent-directory segments (blocked), absolute paths on both platforms (blocked), empty paths (blocked), and backslash separators (blocked). One thing worth noting: the new filter is actually more secure than the old one. The old No AGENTS.md violations. Scope is minimal — only what's needed for the fix. Unit TestsAll 18 tests pass: Tmux VerificationRan a comparison of old vs new filter against representative tar entry paths: The
中文说明代码审查实现干净且正确,未发现阻塞问题。
测试套件覆盖了正确的场景:文件名段内的双点(允许)、上级目录段(阻止)、两个平台的绝对路径(阻止)、空路径(阻止)、反斜杠分隔符(阻止)。 值得注意的是:新过滤器实际上比旧版更安全。旧的 无 AGENTS.md 违规。范围最小——仅修复所需的内容。 单元测试全部 18 个测试通过。 Tmux 验证运行了旧过滤器与新过滤器在代表性 tar 条目路径上的对比,确认:
— Qwen Code · qwen3.7-max |
|
Stepping back: this is a clean, minimal bug fix that does exactly what it says. The old filter was too blunt — The code is 7 lines of logic plus 19 lines of tests. Every change in the diff is necessary. The implementation matches what I'd have written independently. All 18 tests pass, and the tmux verification confirms no regressions. The DRY consolidation point from @wenshao is worth considering — if the path-boundary checks across #5515, #5458/#5460, and this PR can share a common utility, that's a good follow-up. But the existing Verdict: Ship it. ✅ 中文说明总结:这是一个干净、最小化的 bug 修复,完全做到了它所声称的事情。旧过滤器太粗暴—— 代码是 7 行逻辑加 19 行测试。diff 中的每个改动都是必要的。实现与我会独立编写的一致。全部 18 个测试通过,tmux 验证确认无回归。 @wenshao 提出的 DRY 合并建议值得考虑——如果 #5515、#5458/#5460 和本 PR 中的路径边界检查可以共享一个通用工具函数,那是一个好的后续工作。但现有的 结论: 可以合并 ✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
✅ Local verification report — real build + adversarial tar-extraction E2EI verified this PR locally with real tooling (no mocks for the extraction path). Driven in Environment
1. The PR's own test plan — all green
¹ 2. Real end-to-end tar extraction (the part the PR marked "out of scope")The PR body lists "manual end-to-end standalone update against a real release archive" as not validated. I covered exactly that: I built real Filter decisions (ALLOW = extract / REJECT = skip):
On-disk outcome of the real
What this proves:
3. Defense-in-depth is intact (unchanged by the PR)The diff only swaps the one filter line + adds the exported helper. The two deeper guards are untouched and still active: node-tar's own Non-blocking note
VerdictApprove. Correct, narrowly scoped, fully covered by the new unit tests, and confirmed against real archives end-to-end. Traversal protection is preserved (and slightly improved). 🇨🇳 中文版验证报告(点击展开)✅ 本地验证报告 —— 真实构建 + 对抗性 tar 解压端到端测试我在本地使用真实工具链(解压路径无任何 mock)验证了本 PR,全程在 环境
1. PR 自带的测试计划 —— 全绿
¹ 2. 真实端到端 tar 解压(PR 中标注为"范围之外"的部分)PR 描述将"针对真实发布归档的手动端到端更新"列为未验证。我恰好补上了这一块:构造了包含对抗性条目名的真实 过滤决策(ALLOW = 解压 / REJECT = 跳过):
真实
由此证明:
3. 纵深防御保持完好(本 PR 未改动)diff 仅替换了那一行过滤判断 + 新增导出的辅助函数。两层更深的防护均未触及且仍然生效:node-tar 自身的 非阻塞提示
结论赞成合并。 实现正确、范围聚焦、由新增单元测试完整覆盖,并已针对真实归档做端到端确认。路径穿越防护得以保留(并略有增强)。 Verification method: isolated merge worktree, real |
What this PR does
Refines the standalone updater's tar-extraction path filter so it no longer rejects safe filenames that merely contain consecutive dots. A new
isSafeTarEntryPathhelper replaces the oldp.startsWith('/') || p.includes('..')check insideextractArchive. The helper:path.posix.isAbsolute/path.win32.isAbsolute)./or\separators and rejects it only when a whole segment is exactly..(a real parent-directory traversal)...that appears inside a single filename segment (e.g.qwen-code/release..notes.md,qwen-code/node/lib/foo..bar,qwen-code/.../file.txt).The function is exported so it can be unit-tested directly.
Why it's needed
The previous filter rejected any archive entry whose path contained
..anywhere. That blocked directory traversal, but it also dropped legitimate files whose names happen to contain consecutive dots (such asqwen-code/release..notes.mdorqwen-code/node/lib/foo..bar), because the dots are part of a single path segment rather than a parent-directory segment. The desired behavior is to keep rejecting absolute paths and real..path segments while allowing safe filenames that merely contain consecutive dots.Reviewer Test Plan
How to verify
npm run test --workspace=packages/cli -- standalone-update.test.tsnpm run typecheck --workspace=packages/clinpm run buildnpx eslint packages/cli/src/utils/standalone-update.ts packages/cli/src/utils/standalone-update.test.tsnpx prettier --check packages/cli/src/utils/standalone-update.ts packages/cli/src/utils/standalone-update.test.tsgit diff --checkEvidence (Before & After)
N/A — internal logic change covered by unit tests. New
isSafeTarEntryPathtests assert that double dots inside a filename segment are allowed while parent-directory segments and absolute paths (including backslash separators andC:\...drive paths) are rejected.Tested on
Environment (optional)
Local macOS workspace; unit tests via npm/vitest.
Risk & Scope
packages/cli/src/utils/standalone-update.ts. Traversal protection is preserved (absolute paths and real..segments are still rejected).Linked Issues
Fixes #5520
AI Assistance Disclosure
I used Codex to review the changes, sanity-check the implementation against existing patterns, and help spot potential edge cases.
中文说明
这个 PR 做了什么
优化独立更新器(standalone updater)的 tar 解压路径过滤逻辑,使其不再误拒仅仅包含连续点号的安全文件名。在
extractArchive内部,用新的isSafeTarEntryPath辅助函数替换了原先的p.startsWith('/') || p.includes('..')判断。该辅助函数:path.posix.isAbsolute/path.win32.isAbsolute判断)。/或\分隔符切分条目路径,仅当某个完整片段恰好为..(真正的上级目录穿越)时才拒绝。..(例如qwen-code/release..notes.md、qwen-code/node/lib/foo..bar、qwen-code/.../file.txt)。该函数被导出,以便可以直接进行单元测试。
为什么需要
之前的过滤器会拒绝任何路径中任意位置包含
..的归档条目。这虽然阻止了目录穿越,但也丢弃了名字恰好包含连续点号的合法文件(例如qwen-code/release..notes.md或qwen-code/node/lib/foo..bar),因为这些点号属于单个路径片段,而不是上级目录片段。期望的行为是:继续拒绝绝对路径和真正的..路径片段,同时允许仅仅包含连续点号的安全文件名。审阅者测试计划
如何验证
npm run test --workspace=packages/cli -- standalone-update.test.tsnpm run typecheck --workspace=packages/clinpm run buildnpx eslint packages/cli/src/utils/standalone-update.ts packages/cli/src/utils/standalone-update.test.tsnpx prettier --check packages/cli/src/utils/standalone-update.ts packages/cli/src/utils/standalone-update.test.tsgit diff --check证据(前后对比)
N/A — 属于内部逻辑改动,已由单元测试覆盖。新增的
isSafeTarEntryPath测试断言:文件名片段内部的连续点号被允许,而上级目录片段和绝对路径(包括反斜杠分隔符以及C:\...盘符路径)被拒绝。测试平台
环境(可选)
本地 macOS 工作区;通过 npm/vitest 运行单元测试。
风险与范围
packages/cli/src/utils/standalone-update.ts中的 tar 条目路径过滤逻辑。穿越防护得以保留(绝对路径和真正的..片段仍被拒绝)。关联 Issue
Fixes #5520
AI 协助声明
我使用 Codex 审阅了这些改动,对照现有模式核对实现,并帮助发现潜在的边界情况。