fix(core): allow dot-prefixed plans directories - #5460
Conversation
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
— DeepSeek/deepseek-v4-pro via Qwen Code /review
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Hi @tt-a1i, thanks for the PR — the fix itself looks targeted and reasonable.
However, the PR body doesn't follow our pull request template. Several required sections are missing:
## What this PR does/## Why it's needed(you have## Summaryinstead)## Reviewer Test PlanwithHow to verify,Evidence (Before & After), andTested ontable## Risk & Scope## Linked Issues<details>中文说明</details>translation block
Could you update the PR description to match the template? It helps reviewers (and future-you) understand the motivation, verify the fix, and assess risk at a glance. Happy to re-review once that's done. 🙏
中文说明
你好 @tt-a1i，感谢提交 PR — 修复本身看起来是合理且有针对性的。
不过 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<details>中文说明</details>翻译区块
麻烦按照模板补充一下 PR 描述，方便 reviewer 理解动机、验证修复和评估风险。补完之后我会重新审查。🙏
— Qwen Code · qwen3.7-max
✅ Local runtime verification — PR #5460 (
|
| # | input | expected | main (before) | PR (after) |
|---|---|---|---|---|
| 01 | ./..plans |
allow | ❌ THREW | ✅ RESOLVED — fixed |
| 02 | ..plans |
allow | ❌ THREW | ✅ RESOLVED — fixed |
| 03 | ./...triple |
allow | ❌ THREW | ✅ RESOLVED — fixed |
| 07 | <project>/..plans (absolute, inside) |
allow | ❌ THREW | ✅ RESOLVED — fixed |
| 04 | ./.hidden |
allow | ✅ RESOLVED | ✅ RESOLVED |
| 05 | ./plans |
allow | ✅ RESOLVED | ✅ RESOLVED |
| 06 | ./sub/..keep (dot-prefixed, nested) |
allow | ✅ RESOLVED | ✅ RESOLVED |
| 08 | ./inside-link (real symlink, inside) |
allow | ✅ RESOLVED | ✅ RESOLVED |
| 09 | ../plans |
reject | 🛡️ THREW | 🛡️ THREW |
| 10 | .. |
reject | 🛡️ THREW | 🛡️ THREW |
| 11 | ../ |
reject | 🛡️ THREW | 🛡️ THREW |
| 12 | ../..plans (dot-prefixed in parent) |
reject | 🛡️ THREW | 🛡️ THREW |
| 13 | <outside> (absolute, outside) |
reject | 🛡️ THREW | 🛡️ THREW |
| 14 | ./escape-link (real symlink → outside) |
reject | 🛡️ THREW | 🛡️ THREW |
- ALLOW cases correct:
4/8 → 8/8— the 4 dot-prefixed names that wrongly threw on main now resolve. - REJECT (security) cases correct:
6/6 → 6/6— unchanged on both builds. Notably Related to #5: Fixed bounds checking in RadioButtonSelect component to ensure active… #12 (../..plans) and the real symlink escape 为什么不能跟cc或者geminicli一样，把密钥放在终端的环境变量里面？ #14 are still rejected, so narrowingstartsWith('..')tostartsWith('..' + sep) || === '..'does not open a traversal hole.
Committed tests + teeth
- PR test suite:
vitest run storage.test.ts→ 63/63 passed (was 62 on main; +1 regression test). - Counterfactual (proves the test has teeth): the PR's new test
allows project subdirectories whose names start with two dots, run against main'sstorage.ts, FAILS — so the test genuinely pins the fixed behavior, not a tautology. eslint packages/core/src/config/storage.ts→ clean (exit 0).
Why the fix is sound (reasoning)
path.relative(parent, child) only emits .. segments at the start, each followed by a separator or end-of-string. So a result can be exactly .., or start with .. + sep (genuine traversal), or be a plain name like ..plans (a real child). The old startsWith('..') conflated the third with the first two; the PR rejects only === '..', '..' + sep prefix, and absolute paths — precise and complete.
🇨🇳 中文版（点击展开）
✅ 本地运行时验证 — PR #5460（fix(core): allow dot-prefixed plans directories）
结论：通过。 该修复解决了 #5459，并且完整保留了所有“路径穿越 / 符号链接逃逸”的拦截。验证方式：用 tsx 直接运行真实的 Storage.getPlansDir() 源码（无任何 mock），针对一个真实的磁盘目录夹具——包含真实的以 .. 开头的目录和真实的符号链接——在 tmux 中对 origin/main 与 PR 头（7382437）做 A/B 对比。
测试方法
- 驱动脚本导入真实的
packages/core/src/config/storage.ts，对 14 个输入调用getPlansDir(projectRoot, value)，记录是RESOLVED <路径>还是THREW（抛错）。 - 夹具是真实文件系统：项目根下真实创建了
plans/、..plans/、...triple/、.hidden/、sub/..keep/，外加两个真实符号链接——escape-link → <项目外>和inside-link → plans/。这会真正走到单元测试里被 mock 掉的fs.realpathSync代码路径。 - A/B = 同一驱动、同一夹具，分别在
origin/main（修复前）和 PR 的storage.ts（修复后）各跑一次。仅覆盖替换了storage.ts，结束后工作区已还原干净。
A/B 差异对比（真实文件系统）
| # | 输入 | 期望 | main（修复前） | PR（修复后） |
|---|---|---|---|---|
| 01 | ./..plans |
允许 | ❌ 抛错 | ✅ 解析 —已修复 |
| 02 | ..plans |
允许 | ❌ 抛错 | ✅ 解析 —已修复 |
| 03 | ./...triple |
允许 | ❌ 抛错 | ✅ 解析 —已修复 |
| 07 | <项目>/..plans（绝对路径，在项目内） |
允许 | ❌ 抛错 | ✅ 解析 —已修复 |
| 04 | ./.hidden |
允许 | ✅ 解析 | ✅ 解析 |
| 05 | ./plans |
允许 | ✅ 解析 | ✅ 解析 |
| 06 | ./sub/..keep（更深一层的 .. 前缀目录） |
允许 | ✅ 解析 | ✅ 解析 |
| 08 | ./inside-link（真实符号链接，指向项目内） |
允许 | ✅ 解析 | ✅ 解析 |
| 09 | ../plans |
拒绝 | 🛡️ 抛错 | 🛡️ 抛错 |
| 10 | .. |
拒绝 | 🛡️ 抛错 | 🛡️ 抛错 |
| 11 | ../ |
拒绝 | 🛡️ 抛错 | 🛡️ 抛错 |
| 12 | ../..plans（父目录里的 .. 前缀目录） |
拒绝 | 🛡️ 抛错 | 🛡️ 抛错 |
| 13 | <项目外>（绝对路径，在项目外） |
拒绝 | 🛡️ 抛错 | 🛡️ 抛错 |
| 14 | ./escape-link（真实符号链接 → 项目外） |
拒绝 | 🛡️ 抛错 | 🛡️ 抛错 |
- 允许类正确数：
4/8 → 8/8——原本在 main 上被错误拦截的 4 个..前缀目录名现在能正常解析。 - 拒绝（安全）类正确数：
6/6 → 6/6——两个版本都保持不变。 特别是 Related to #5: Fixed bounds checking in RadioButtonSelect component to ensure active… #12（../..plans）和真实符号链接逃逸 为什么不能跟cc或者geminicli一样，把密钥放在终端的环境变量里面？ #14 仍被拒绝，说明把startsWith('..')收窄为startsWith('..' + sep) || === '..'没有打开任何穿越漏洞。
已提交测试 + 有效性（teeth）
- PR 测试套件：
vitest run storage.test.ts→ 63/63 通过（main 上为 62；新增 1 条回归测试）。 - 反事实验证（证明测试确实有效）： 把 PR 新增的测试
allows project subdirectories whose names start with two dots放到 main 的storage.ts上跑 → 失败——说明该测试真正锁定了修复后的行为，而非永真断言。 eslint packages/core/src/config/storage.ts→ 干净（退出码 0）。
修复为什么是正确的（推理）
path.relative(parent, child) 只会在开头产生 .. 片段，且每个 .. 后面要么跟分隔符、要么就是字符串结尾。因此结果只可能是：正好等于 ..、以 .. + 分隔符开头（真正的穿越）、或一个普通名字如 ..plans（真实子目录）。旧的 startsWith('..') 把第三种和前两种混为一谈；PR 只拒绝 === '..'、'..' + 分隔符 前缀、以及绝对路径——精确且完备。
Reproducible: tsx driver over a real-fs fixture (real ..-dirs + real symlinks), A/B origin/main vs PR 7382437, run in tmux. Working tree restored clean after the run.
|
updated the PR body to match the template. |
|
@qwen-code /triage |
|
Hi @tt-a1i, thanks for the fix! Template looks good ✓ — all required sections present, including bilingual description and test plan. On direction: this fixes a genuine bug where On approach: the scope is minimal and focused — 2 files, +17/-5. The fix narrows the check from Moving on to code review. 🔍 中文说明你好 @tt-a1i，感谢修复！ 模板完整 ✓ — 所有必填部分齐全，包含双语描述和测试计划。 方向：这个修复解决了一个真实的 bug — 方案：范围最小且聚焦 — 2 个文件，+17/-5。修复将检查从 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
2a. Code ReviewWrote my own proposal before reading the diff: narrow The diff is clean — one extracted function No correctness bugs, no security regressions, no AGENTS.md violations. 63/63 unit tests pass, eslint clean, typecheck clean. 2b. Real-Scenario Testing (tmux, tsx driver)Ran Before (installed build / main)After (this PR)Summary: Before — 2/4 allow cases incorrectly rejected (the bug). After — all 4 allow cases pass, all 3 reject cases still blocked. Security posture unchanged. 中文说明2a. 代码审查在阅读 diff 之前先写了自己的方案：将 Diff 干净 — 一个提取的函数 无正确性 bug、无安全回归、无 AGENTS.md 违规。63/63 单元测试通过，eslint 干净，typecheck 干净。 2b. 真实场景测试（tmux, tsx 驱动）通过 tsx 直接调用 修复前 — 4 个应允许的用例中有 2 个被错误拒绝（即 bug）。修复后 — 所有 4 个允许用例通过，所有 3 个拒绝用例仍被拦截。安全态势未改变。 — Qwen Code · qwen3.7-max |
|
This is a clean, well-scoped bugfix. The PR does exactly one thing — fix an over-broad My independent proposal before reading the diff was identical: narrow the check to actual traversal segments, extract a testable helper. The PR matches this, which is a good sign the approach is the obvious right one. The before/after tmux output tells the story: two cases that wrongly threw now resolve, while every security rejection stays intact. The regression test has teeth (fails on main's code, passes on the PR's). 63/63 tests pass, lint and typecheck clean. The extracted Approving. ✅ 中文说明这是一个干净、范围恰当的 bugfix。PR 只做一件事 — 修复过于宽泛的 我在阅读 diff 之前独立提出的方案与此完全一致：将检查收窄到真正的路径穿越片段，提取一个可测试的辅助函数。PR 与此吻合，说明这是显而易见的正确方案。 tmux 前后对比清楚地展示了：两个原本错误抛错的用例现在能正常解析，同时所有安全拒绝保持不变。回归测试有效（在 main 代码上失败，在 PR 代码上通过）。63/63 测试通过，lint 和 typecheck 干净。 提取的 批准。✅ — Qwen Code · qwen3.7-max |
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. ✅
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
— DeepSeek/deepseek-v4-pro via Qwen Code /review
✅ Local end-to-end verification (real filesystem, tmux)Verified by calling the real built
A/B result (real
|
plansDirectory input |
BASE 5924ab3a |
PR 7382437e |
kind |
|---|---|---|---|
./..plans, ..plans, ...triple, ..plans/nested |
❌ REJECT (the bug) | ✅ ALLOW → <proj>/… |
bug-fix |
normalplans, . |
✅ ALLOW | ✅ ALLOW | control |
.., ../plans, ../../etc, /etc/qwen-plans |
✅ REJECT | ✅ REJECT | traversal |
..plans/../../etc (adversarial) |
✅ REJECT | ✅ REJECT | no-hole |
evil → symlink outside project |
✅ REJECT | ✅ REJECT | symlink escape |
========== BASE (5924ab3a) ==========
❌→ ./..plans REJECT (FatalConfigError) dot-prefixed child (the bug)
..plans/../../etc REJECT (FatalConfigError) ADVERSARIAL dot-prefix then traversal
evil REJECT (FatalConfigError) symlink escaping project (realpath)
RESULT: all 12 probes matched expectation for BASE
========== PR (7382437e) ==========
✅→ ./..plans ALLOW -> <proj>/..plans dot-prefixed child (the bug)
..plans/../../etc REJECT (FatalConfigError) ADVERSARIAL dot-prefix then traversal
evil REJECT (FatalConfigError) symlink escaping project (realpath)
RESULT: all 12 probes matched expectation for PR
So BASE wrongly rejects a project subdirectory literally named ..plans/...triple (the issue), while PR resolves it inside the project. Crucially, every real escape stays rejected on both builds — .., ../…, absolutes, the adversarial ..plans/../../etc (which resolves to ../etc), and a symlink pointing outside the project. The fix narrows a false-positive without opening a false-negative.
Unit test + revert-proof + static
vitest run storage.test.ts -t "getPlansDir"on the PR source → 15/15 pass.- Revert-proof: running the PR's new test against the base source fails only
allows project subdirectories whose names start with two dots❌ — the pre-existing security tests (rejects relative … escape,rejects absolute … outside,rejects symlink pointing outside,allows legitimate symlink that stays within,rejects missing nested path under symlink that escapes) all still pass on both, confirming the boundary's defense is preserved. - ESLint clean (exit 0); Prettier clean;
git diff --checkclean.
Note
The bug is the classic startsWith('..') vs startsWith('..' + path.sep) distinction: '..plans'.startsWith('..') is true, so the old check treated the legitimate child as an escape. The new helper rejects only an exact .., a ../… prefix, or an absolute relative result — which is what an actual parent-directory traversal produces. Because the check runs on the realpath-resolved child, any traversal through a dot-prefixed name (..plans/../../etc) and any symlink escape still resolve to a ../… relative and are rejected.
Verdict: LGTM — fixes the false-positive rejection of dot-prefixed plans directories while fully preserving traversal and symlink-escape protection.
🇨🇳 中文版本（点击展开）
✅ 本地端到端验证（真实文件系统，tmux）
通过对真实编译产物中的 Storage.getPlansDir 进行验证：在一个真实临时项目里创建真实的 ..plans / ...triple / ..plans/nested 子目录,以及一个真实指向项目外部的符号链接,从而让 resolvePathThroughExistingAncestor 里的 fs.realpathSync 规范化真正执行（无 mock）。两个版本都编译为真实的 packages/core/dist：
- BASE = merge-base
5924ab3a（!relativePath.startsWith('..')） - PR = head
7382437e（!startsWith('..'+sep) && !== '..' && !isAbsolute）
A/B 结果（真实 Storage.getPlansDir(projectRoot, …)）
plansDirectory 输入 |
BASE 5924ab3a |
PR 7382437e |
类别 |
|---|---|---|---|
./..plans、..plans、...triple、..plans/nested |
❌ 拒绝（即 bug） | ✅ 允许 → <proj>/… |
修复点 |
normalplans、. |
✅ 允许 | ✅ 允许 | 对照 |
..、../plans、../../etc、/etc/qwen-plans |
✅ 拒绝 | ✅ 拒绝 | 穿越 |
..plans/../../etc（对抗） |
✅ 拒绝 | ✅ 拒绝 | 无漏洞 |
evil → 指向项目外的软链 |
✅ 拒绝 | ✅ 拒绝 | 软链逃逸 |
也就是说：BASE 会错误拒绝一个名为 ..plans/...triple 的项目内真实子目录（即该 issue）,而 PR 能正确解析到项目内部。关键在于：所有真正的逃逸在两个版本上都仍被拒绝——..、../…、绝对路径、对抗用例 ..plans/../../etc（它会解析成 ../etc）以及指向项目外的符号链接。本修复只收窄了误报,没有引入漏报。
单元测试 + 反向验证 + 静态检查
- 在 PR 源码上
vitest run storage.test.ts -t "getPlansDir"→ 15/15 通过。 - 反向验证：把 PR 新增测试放到 base 源码上跑,只有
allows project subdirectories whose names start with two dots❌ 失败——既有的安全测试（拒绝相对路径逃逸、拒绝绝对路径越界、拒绝指向外部的符号链接、允许停留在项目内的合法符号链接、拒绝经由逃逸符号链接的缺失嵌套路径）在两边都仍然通过,说明边界防御得到了保留。 - ESLint 通过（exit 0）；Prettier 通过；
git diff --check通过。
补充说明
该 bug 是经典的 startsWith('..') 与 startsWith('..' + path.sep) 的区别：'..plans'.startsWith('..') 为 true,所以旧检查把合法子目录当成了逃逸。新 helper 只拒绝精确的 ..、../… 前缀,或绝对的相对结果——这正是真正的父目录穿越会产生的形态。由于检查作用在经 realpath 解析后的 child 上,任何经由点开头目录名的穿越（..plans/../../etc）以及任何符号链接逃逸仍会解析成 ../… 相对路径并被拒绝。
结论：LGTM —— 修复了对点开头 plans 目录的误拒,同时完整保留了路径穿越与符号链接逃逸的防护。
3 participants
What this PR does
Allows custom plans directories whose directory names start with dots, such as
..plansor...triple, while keeping real parent-directory traversal rejected.Why it's needed
The old boundary check treated any
path.relative(...)result starting with..as an escape. That accidentally rejected legitimate project subdirectories whose names merely begin with two dots. Users could not place plans in those directories even though they are still inside the project.Reviewer Test Plan
How to verify
Review the new
Storage.getPlansDir()regression coverage. The allowed case is a real project child named with a..prefix; traversal cases such as..,../plans, and paths that resolve outside the project should still throw.Run:
npm --workspace packages/core test -- storage.test.ts --coverage.enabled=false npx eslint packages/core/src/config/storage.ts packages/core/src/config/storage.test.ts npm --workspace packages/core run typecheckEvidence (Before & After)
Before:
getPlansDir(projectRoot, "..plans")threw because the relative path started with...After: dot-prefixed child directory names resolve, while actual traversal still fails.
Tested on
Environment (optional)
Local Node/npm workspace tests.
Risk & Scope
..prefix check to actual parent segments.Linked Issues
Fixes #5459
AI Assistance Disclosure
I used Codex to review the changes, sanity-check the implementation against existing patterns, and help spot potential edge cases.
中文说明
这个 PR 做了什么
允许使用目录名以点开头的自定义 plans 目录，例如
..plans或...triple，同时继续拒绝真正的父目录穿越。为什么需要
旧的边界检查把所有
path.relative(...)结果以..开头的路径都当作逃逸路径，导致项目内真实存在的..前缀子目录也被误拒绝。用户即使目录仍在项目内，也无法把 plans 放在那里。Reviewer Test Plan
如何验证
查看新增的
Storage.getPlansDir()回归测试。允许的用例是项目内名为..前缀的真实子目录；..、../plans以及解析到项目外的路径仍应抛错。运行：
npm --workspace packages/core test -- storage.test.ts --coverage.enabled=false npx eslint packages/core/src/config/storage.ts packages/core/src/config/storage.test.ts npm --workspace packages/core run typecheck改动前后证据
改动前：
getPlansDir(projectRoot, "..plans")会因为相对路径以..开头而抛错。改动后：以点开头的子目录名可以正常解析，真正的路径穿越仍会失败。
本地测试平台
环境
本地 Node/npm workspace 测试。
风险与范围
..前缀检查收窄到真实父目录片段。关联 Issue
Fixes #5459
AI Assistance Disclosure
I used Codex to review the changes, sanity-check the implementation against existing patterns, and help spot potential edge cases.