fix(desktop): allow double dots in bundle filenames - #5515
Conversation
|
📌 Same maintainer note — full version on #5509. This is the |
|
@qwen-code /triage |
|
Thanks for the PR! Template: the body uses "## Summary" / "## Tests" / "## AI Assistance Disclosure" instead of the template headings ("What this PR does", "Why it's needed", "Reviewer Test Plan" with OS matrix, "Risk & Scope", "Linked Issues"). Not blocking on this — the substance is clear and it's a small fix — but future PRs should follow the template for faster review. Direction: this is a clear-cut bug fix. The Approach: minimal and focused — one helper function, one line changed, two regression tests. No scope creep, no drive-by refactors. Exactly the right size for the problem. Moving on to code review. 🔍 中文说明感谢贡献! 模板: PR 正文使用了 "## Summary" / "## Tests" / "## AI Assistance Disclosure" 而非模板要求的标题。不阻断——内容清楚且改动很小——但后续 PR 请参照模板以加速审查。 方向: 明确的 bug 修复。 方案: 最小化且聚焦——一个辅助函数、一行改动、两个回归测试。无范围蔓延、无顺手重构。问题规模的恰当修复。 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code ReviewIndependent proposal: I'd replace the The PR matches this exactly. The No concerns to raise. The implementation is straightforward and correct. TestingUnit tests (25/25 pass): Both new tests pass:
Tmux real-scenario testing: not applicable. Typecheck/lint: pre-existing 中文说明代码审查独立方案: 将 PR 完全匹配此方案。 测试单元测试(25/25 通过):两个新增测试均通过—— Tmux 真实场景测试: 不适用。 类型检查/lint: — Qwen Code · qwen3.7-max |
|
This is the kind of PR that's easy to approve. Clear bug, clear fix, clean code, good tests. The original The only note for future PRs is the template — the "Reviewer Test Plan" section with OS coverage helps maintainers prioritize review. But for a fix this small and well-tested, it didn't slow things down. LGTM. Approving. ✅ 中文说明这是一个容易批准的 PR。清晰的 bug、清晰的修复、干净的代码、良好的测试。 原来的 唯一建议是后续 PR 请使用模板——"Reviewer Test Plan" 部分及操作系统覆盖有助于维护者优先审查。但对于如此小且测试良好的修复,这并不影响审查效率。 LGTM. 批准。✅ — 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 real-build verification on macOS (head
|
| Input | OLD includes('..') |
NEW segment-wise | |
|---|---|---|---|
release..notes.md |
❌ "Path traversal" (false reject) | ✅ allowed | the bug, fixed |
docs/release..notes.md |
❌ "Path traversal" | ✅ allowed | fixed |
a..b/c.txt, file...txt, foo.., ..foo |
❌ "Path traversal" | ✅ allowed | fixed |
../etc/passwd · docs/../escape.txt · .. · a/../b · sub/.. · ../.. |
rejected | rejected | traversal still blocked ✅ |
docs\..\escape.txt (backslash) |
rejected | rejected (by the backslash check, not the .. check) |
no regression ✅ |
test.txt, subdir/file.txt |
allowed | allowed | control ✅ |
3 · The new test is non-vacuous (mutation)
Reverting bundle-files.ts to base (7e66b1710) and re-running:
accepts double dots inside a path segment→ FAILS on base (old code returnsPath traversal detected), passes on the PR → it genuinely guards the fix.rejects parent traversal segments below the bundle root→ passes on both → confirms the fix didn't open a hole to add the feature.
4 · End-to-end restoreFiles (the real consumer: resource & session bundle restore)
- legit
docs/release..notes.md→ written to<target>/docs/release..notes.md, content verified ✅ docs/../escape.txt→ throwsInvalid bundle file: Path traversal detected✅- Defense-in-depth intact:
restoreFilesstill re-checksfullPath.startsWith(targetDir + sep)afterjoin(line 192), independent ofvalidateBundleFile.
5 · Security note — one Windows-only edge case (non-blocking)
The segment-equality check newly allows a segment that is .. plus a trailing space or dot (.. , and ...), which the old substring check happened to reject. On POSIX this is safe and I verified containment — .. /escape.txt restores to a literal .. subdirectory inside the target (escaped-target = false). On Windows, the filesystem strips trailing spaces/periods from path components, so .. can canonicalize back to ..; and the in-code startsWith guard does not catch it because path.win32.join(target, '.. /escape.txt') keeps the literal .. segment (= target\.. \escape.txt, which does start with target\). I could not reproduce the final FS step on macOS, so I'm flagging it as documented-but-unverified rather than a confirmed escape.
This is exactly the argument for the maintainer's DRY suggestion: routing restore through a single realpath-based boundary helper (e.g. isSubpath in packages/core/src/utils/paths.ts, or a realpathSync + containment check in restoreFiles) would close this without a hand-rolled per-file rule. Minimal alternative: trim trailing dots/spaces per segment before the .. comparison.
Verdict
The fix is correct, minimal, and the new test is meaningful — safe to merge (or fold into #5509 per the DRY note). The Windows .. -segment hardening is an optional, separate follow-up.
🇨🇳 中文版(完整对应)
🔬 macOS 本地真实构建验证(head 5b3dacb46)
针对 bundle-files.ts 的真实源码做了端到端验证 —— desktop shared 包用 bun:test、且不在 npm workspaces 里,所以我用 tsx 直接 import 真实 .ts,并把完整的现有测试文件通过 vitest(API 兼容)跑了一遍。整个过程在 tmux 里运行并 tee 到日志。(与上方/ #5509 的维护者 DRY 说明互补 —— 这条是证据侧。)
结论:✅ 这个修复是正确的、且有测试覆盖。 它把子串检查(relativePath.includes('..'),会误拒 release..notes.md 这类合法文件名)换成了按路径段检查(split('/').includes('..')),这才是检测父级穿越的正确方式。所有真正的穿越向量依旧被拦。末尾有一条可选的、仅限 Windows 的加固说明(不阻塞合并)。
1 · 完整现有测试
真正的 runner bun:test 本地没装,所以我把原封不动的测试文件通过 vitest(同样的 describe/it/expect)对真实源码跑了一遍:25 / 25 通过,含新增的 2 个用例。
2 · 行为 A/B —— NEW(修复) vs OLD(base 的 includes('..')),用回退源码重跑真实代码
| 输入 | OLD includes('..') |
NEW 按段检查 | |
|---|---|---|---|
release..notes.md |
❌ "Path traversal"(误拒) | ✅ 放行 | 这就是 bug,已修 |
docs/release..notes.md |
❌ "Path traversal" | ✅ 放行 | 已修 |
a..b/c.txt、file...txt、foo..、..foo |
❌ "Path traversal" | ✅ 放行 | 已修 |
../etc/passwd · docs/../escape.txt · .. · a/../b · sub/.. · ../.. |
拒绝 | 拒绝 | 穿越依旧被拦 ✅ |
docs\..\escape.txt(反斜杠) |
拒绝 | 拒绝(走的是反斜杠检查,不是 .. 检查) |
无回归 ✅ |
test.txt、subdir/file.txt |
放行 | 放行 | 对照 ✅ |
3 · 新增测试非空过场(变异测试)
把 bundle-files.ts 回退到 base(7e66b1710)重跑:
accepts double dots inside a path segment→ 在 base 上失败(旧代码返回Path traversal detected),在 PR 上通过 → 它确实守护了这个修复。rejects parent traversal segments below the bundle root→ 两边都通过 → 证明为加这个特性并没有开出新的口子。
4 · 端到端 restoreFiles(真实调用方:resource / session bundle 还原)
- 合法
docs/release..notes.md→ 正确写到<target>/docs/release..notes.md,内容校验通过 ✅ docs/../escape.txt→ 抛出Invalid bundle file: Path traversal detected✅- 纵深防御仍在:
restoreFiles在join之后依旧独立地复查fullPath.startsWith(targetDir + sep)(第 192 行),不依赖validateBundleFile。
5 · 安全说明 —— 一个仅限 Windows 的边界情况(不阻塞)
按段相等检查现在会放行「.. 后面再跟一个空格或点」的段(.. ,以及 ...)—— 这些旧的子串检查恰好会拒。在 POSIX 上是安全的,我验证了它被包含在目标内 —— .. /escape.txt 会还原到目标目录内部一个字面叫 .. 的子目录(escaped-target = false)。在 Windows 上,文件系统会去掉路径段尾部的空格/点,于是 .. 可能被规范化回 ..;而代码里的 startsWith 防护也拦不住它,因为 path.win32.join(target, '.. /escape.txt') 会保留字面的 .. 段(= target\.. \escape.txt,确实以 target\ 开头)。最后这一步 FS 规范化我在 macOS 上无法复现,所以把它标为「有据可查但未实测」,而不是已确认的逃逸。
这恰好印证了维护者的 DRY 建议:让还原走一个统一的、基于 realpath 的边界 helper(比如 packages/core/src/utils/paths.ts 里的 isSubpath,或在 restoreFiles 里加一道 realpathSync + 包含性检查),就能不靠逐文件手写规则把这个口子也堵上。最小改法:在做 .. 比较前,先去掉每段尾部的点和空格。
结论
修复正确、改动精简、新增测试有意义 —— 可以合并(或按 DRY 说明并入 #5509)。Windows 的 .. 段加固是一个可选的、独立的后续项。
✅ Local runtime verification — PR #5515 (
|
| relativePath | kind | main (before) | PR (after) |
|---|---|---|---|
../escape.txt |
traversal | 🛡️ REJECT | 🛡️ REJECT |
docs/../escape.txt |
traversal | 🛡️ REJECT | 🛡️ REJECT |
.. |
traversal | 🛡️ REJECT | 🛡️ REJECT |
a/../../b.txt |
traversal | 🛡️ REJECT | 🛡️ REJECT |
docs/.. |
traversal | 🛡️ REJECT | 🛡️ REJECT |
release..notes.md |
legit .. |
❌ REJECT 🐛 | ✅ OK |
docs/release..notes.md |
legit .. |
❌ REJECT 🐛 | ✅ OK |
assets/v1..2/icon.svg |
legit .. |
❌ REJECT 🐛 | ✅ OK |
..hidden.txt |
legit .. |
❌ REJECT 🐛 | ✅ OK |
foo...bar.txt |
legit .. |
❌ REJECT 🐛 | ✅ OK |
a..b/c..d.txt |
legit .. |
❌ REJECT 🐛 | ✅ OK |
docs/normal.txt |
control | OK | OK |
/etc/passwd |
control (absolute) | REJECT (Absolute path) | REJECT (same) |
docs\escape.txt |
control (backslash) | REJECT (Backslash) | REJECT (same) |
docs//x.txt |
control (double slash) | REJECT (double slash) | REJECT (same) |
- 6 legitimate double-dot filenames flip
REJECT → OK. - All 5 real traversal cases stay
REJECTon both arms (security preserved). - The independent guards — absolute path, backslash separator, double slash — are unchanged and still reject with their own distinct messages. Narrowing the
..check opens no new traversal surface (a backslash variant likedocs\..\escapeis still caught by the backslash guard).
A/B differential — real restoreFiles() to a temp directory
This is the user-facing effect: does a bundle containing such a file actually restore?
| relativePath | main (before) | PR (after) |
|---|---|---|
docs/release..notes.md |
❌ THREW Path traversal detected 🐛 |
✅ WROTE to disk (content verified) |
v1..2/icon.txt |
❌ THREW Path traversal detected 🐛 |
✅ WROTE to disk (content verified) |
docs/../escape.txt |
🛡️ THREW Path traversal detected |
🛡️ THREW Path traversal detected |
On main, a bundle holding a legitimate release..notes.md fails to restore entirely (the whole restoreFiles call throws). On the PR it restores correctly, and real traversal still throws on both.
Committed tests + teeth (counterfactual)
| check | result |
|---|---|
committed bundle-files.test.ts (bun test) on PR |
✅ 25 pass / 0 fail (incl. the 2 new tests) |
counterfactual — new test accepts double dots inside a path segment vs main bundle-files.ts |
❌ FAILS — Received: "Path traversal detected: docs/release..notes.md" |
The counterfactual failing proves the new test genuinely pins the fixed behavior.
Why the fix is sound
The traversal check changes from "rejects any .. substring" to split('/').includes('..') — rejecting only path segments that are exactly ... A .. inside a segment (release..notes.md) is no longer a false positive, while ../, docs/.., and a/../../b still contain a literal .. segment and are rejected. Defense-in-depth is intact: the separate absolute/backslash/double-slash guards run unchanged, and restoreFiles keeps its second fullPath.startsWith(targetDir + sep) containment check.
🇨🇳 中文版(点击展开)
✅ 本地运行时验证 — PR #5515(fix(desktop): allow double dots in bundle filenames)
结论:通过。 该修复解决了 #5514:validateBundleFile 不再因为文件名里仅仅包含 ..(如 release..notes.md、assets/v1..2/icon.svg)就拒绝,而真正的路径穿越(../escape.txt、docs/../escape.txt)仍然会被拒绝。验证方式:驱动真实的 validateBundleFile() 跑一个路径矩阵,以及真实的 restoreFiles() 文件系统路径(真实写入临时目录),在 tmux 中用 bun 对 PR 与 origin/main 做 A/B 对比。
测试方法
- 验证测试运行:(1)
validateBundleFile()覆盖 15 个relativePath变体(穿越 / 合法双点 / 对照守卫);(2)restoreFiles()针对真实临时目标目录,观察每个文件是真的写入磁盘还是抛错。 - A/B = 同一个测试,分别在 PR 的
bundle-files.ts(分段检查split('/').includes('..'))上跑一次,再把该文件精确还原回origin/main(子串检查includes('..'))跑一次。两组之间只有这一处修复的差别。
A/B 差异对比 — validateBundleFile() 判定
| relativePath | 类型 | main(修复前) | PR(修复后) |
|---|---|---|---|
../escape.txt |
穿越 | 🛡️ 拒绝 | 🛡️ 拒绝 |
docs/../escape.txt |
穿越 | 🛡️ 拒绝 | 🛡️ 拒绝 |
.. |
穿越 | 🛡️ 拒绝 | 🛡️ 拒绝 |
a/../../b.txt |
穿越 | 🛡️ 拒绝 | 🛡️ 拒绝 |
docs/.. |
穿越 | 🛡️ 拒绝 | 🛡️ 拒绝 |
release..notes.md |
合法双点 | ❌ 拒绝 🐛 | ✅ 通过 |
docs/release..notes.md |
合法双点 | ❌ 拒绝 🐛 | ✅ 通过 |
assets/v1..2/icon.svg |
合法双点 | ❌ 拒绝 🐛 | ✅ 通过 |
..hidden.txt |
合法双点 | ❌ 拒绝 🐛 | ✅ 通过 |
foo...bar.txt |
合法双点 | ❌ 拒绝 🐛 | ✅ 通过 |
a..b/c..d.txt |
合法双点 | ❌ 拒绝 🐛 | ✅ 通过 |
docs/normal.txt |
对照 | 通过 | 通过 |
/etc/passwd |
对照(绝对路径) | 拒绝 (Absolute path) | 拒绝 (相同) |
docs\escape.txt |
对照(反斜杠) | 拒绝 (Backslash) | 拒绝 (相同) |
docs//x.txt |
对照(双斜杠) | 拒绝 (double slash) | 拒绝 (相同) |
- 6 个合法的双点文件名从
拒绝 → 通过。 - 全部 5 个真实穿越用例在两个版本上都保持
拒绝(安全性保留)。 - 独立的守卫——绝对路径、反斜杠分隔符、双斜杠——都没变,仍以各自独立的消息拒绝。收窄
..检查没有打开新的穿越面(像docs\..\escape这种反斜杠变体仍会被反斜杠守卫拦下)。
A/B 差异对比 — 真实 restoreFiles() 写入临时目录
这是用户可见的效果:包含这种文件的 bundle 能不能真的恢复?
| relativePath | main(修复前) | PR(修复后) |
|---|---|---|
docs/release..notes.md |
❌ 抛错 Path traversal detected 🐛 |
✅ 写入磁盘(内容已校验) |
v1..2/icon.txt |
❌ 抛错 Path traversal detected 🐛 |
✅ 写入磁盘(内容已校验) |
docs/../escape.txt |
🛡️ 抛错 Path traversal detected |
🛡️ 抛错 Path traversal detected |
在 main 上,含有合法 release..notes.md 的 bundle 会整体恢复失败(整个 restoreFiles 调用抛错)。在 PR 上它能正确恢复,而真实穿越在两个版本上仍然抛错。
已提交测试 + 有效性(反事实)
| 检查项 | 结果 |
|---|---|
已提交 bundle-files.test.ts(bun test)在 PR 上 |
✅ 25 通过 / 0 失败(含 2 个新增测试) |
反事实 —— 新测试 accepts double dots inside a path segment 对 main 的 bundle-files.ts |
❌ 失败 —— Received: "Path traversal detected: docs/release..notes.md" |
反事实失败证明新增测试真正锁定了修复后的行为。
修复为什么是正确的
穿越检查从「拒绝任何 .. 子串」改为 split('/').includes('..')——只拒绝恰好等于 .. 的路径分段。段内的 ..(release..notes.md)不再误报,而 ../、docs/..、a/../../b 仍含有字面量 .. 分段、会被拒绝。纵深防御保持完整:独立的绝对路径 / 反斜杠 / 双斜杠守卫照常运行,restoreFiles 也保留了第二道 fullPath.startsWith(targetDir + sep) 包含性检查。
Reproducible: real validateBundleFile() + real restoreFiles() (actual temp-dir writes) driven by a bun verify test (15 path variants + 3 restore cases), A/B origin/main vs PR 5b3dacb, plus the committed suite and a reverted-prod counterfactual — all in tmux. Working tree restored clean.
Summary
..path segment..filenamesFixes #5514
Tests
bun test packages/desktop/packages/shared/src/utils/__tests__/bundle-files.test.tsbun run typecheck:sharedinpackages/desktopnpx eslint src/utils/bundle-files.ts src/utils/__tests__/bundle-files.test.tsinpackages/desktop/packages/sharednpx prettier --check packages/desktop/packages/shared/src/utils/bundle-files.ts packages/desktop/packages/shared/src/utils/__tests__/bundle-files.test.tsgit diff --checkAI Assistance Disclosure
I used Codex to review the changes, sanity-check the implementation against existing patterns, and help spot potential edge cases.