fix(core): resolve tilde paths before search permission checks#5378
Conversation
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 |
|
Thanks for the PR, @tt-a1i! Template looks good ✓ — all required sections present including the bilingual summary. On direction: this is a straightforward security bug fix. The linked issue #5376 (P1, On approach: the fix is minimal and correct — replace Moving on to code review and testing. 🔍 中文说明感谢贡献! 模板完整 ✓ — 所有必填章节齐全,包括双语摘要。 方向:这是一个明确的安全 bug 修复。关联 issue #5376(P1, 方案:修复最小化且正确——在三个搜索工具的 进入代码审查和测试阶段 🔍 — Qwen Code · qwen3.7-max |
Code ReviewIndependent proposal (before seeing the diff): The fix is to replace Diff comparison: The PR does exactly this. Three one-line changes ( No critical blockers or convention violations found. The change is minimal, correct, and reuses existing infrastructure. Unit TestsAll three test suites pass on the PR branch: Each suite includes new Real-Scenario TestingTilde path (
|
ReflectionThis PR is what a security fix should look like: identify the mismatch between two code paths (permission check uses Three things I looked at before deciding:
The tmux test shows that in practice the LLM tends to expand No reservations. This is ready to ship. 中文说明总结这个 PR 是安全修复的典范:发现两条代码路径的不一致(权限检查用 Bug 真实存在(代码级演示已确认)。修复最小化且完整(三个文件各一行,无新抽象,无搭车重构)。测试有意义(直接测试 虽然实践中 LLM 通常会自行展开 没有顾虑,可以合并。 — 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 — PR #5378
|
| Check | Result |
|---|---|
Merges cleanly into main |
✅ MERGEABLE / CLEAN |
PR getDefaultPermission tests (glob+grep+ripgrep) |
✅ 9/9 pass |
| Genuine-regression — PR tests on BASE source | ✅ the 3 "ask for tilde" tests fail with expected 'allow' to be 'ask'; the 6 within-workspace tests pass on both → proves base grants the bypass |
Edge-case probe (real workspace + real resolvePath + real containment) |
✅ 7/7 (see below) |
tsc --noEmit core |
✅ 0 new type errors (clean, identical to base) |
prettier --check + eslint (6 files) + git diff --check |
✅ all exit 0 |
The tests are a real behavioral proof, not mocked-away: a real temp workspace under /tmp, a real containment check, and the real resolvePath — so ~/outside-workspace → /root/outside-workspace (outside /tmp/...) → ask on the PR, while base's path.resolve yields <workspace>/~/outside-workspace → allow.
Edge-case probe (real glob tool, PR head)
| input | result |
|---|---|
no path / sub / ./sub |
allow ✅ (normal workspace paths unaffected) |
bare ~ (home dir) |
ask ✅ |
~/x (tilde outside) |
ask ✅ |
absolute /tmp |
ask ✅ |
../../.. (parent traversal) |
ask ✅ |
Notes (non-blocking)
- Consistency with execution: the permission check now uses
resolvePath, which is the same~-expansion thatresolveAndValidatePath(used at execution time) relies on — so the check and the actual search now agree on where the path points. ✔ pathimport retained:glob.ts/grep.ts/ripGrep.tsstill usepathelsewhere, so no unused-import fallout.- Cosmetic: in
ripGrep.test.tsthe tool variable is namedgrepToolbut is actuallynew RipGrepTool(...)— a pre-existing naming quirk in that file, not introduced here; the new test does exercise the ripgrep tool.
Reproduce
git worktree add --detach /tmp/wt-after $(gh pr view 5378 --json headRefOid -q .headRefOid)
git worktree add --detach /tmp/wt-before origin/main
for w in wt-after wt-before; do ln -s "$PWD/node_modules" /tmp/$w/node_modules; \
ln -s "$PWD/packages/core/node_modules" /tmp/$w/packages/core/node_modules; done
(cd /tmp/wt-after/packages/core && ../../node_modules/.bin/vitest run --coverage.enabled=false \
src/tools/glob.test.ts src/tools/grep.test.ts src/tools/ripGrep.test.ts -t getDefaultPermission) # 9/9
# Catch the bug: copy PR tests onto BASE source -> the 3 tilde tests fail ('allow' instead of 'ask')🇨🇳 中文版(点击展开)
✅ 本地验证报告 — PR #5378 fix(core): resolve tilde paths before search permission checks
结论:强烈建议合并。 这是一个正确、最小、对安全有正面意义的修复。搜索工具的默认权限检查现在会像执行阶段一样展开 ~,因此对 ~/outside-workspace 的搜索会正确地要求确认(ask),而不再默默地默认为 allow。新测试确实能抓到该 bug,glob/grep/ripgrep 行为一致,且无类型/lint 回归。
Bug(一个确认绕过)
getDefaultPermission 此前用 path.resolve(targetDir, params.path)。对 ~/secret,Node 把 ~ 当成字面片段 → <workspace>/~/secret → 看起来在 workspace 内 → 返回 allow(无需确认)。但执行阶段用的是支持 ~ 的解析,工具实际搜索的是真实 home 目录(在 workspace 外)。结果:对外部 ~ 路径的搜索没有触发本应有的确认。本 PR 在三个工具里把 path.resolve 换成 resolvePath(与校验阶段相同的 ~ 展开解析器)。
验证方式
在 tmux 中、隔离 worktree 里运行真实 vitest:AFTER = 4052e823(PR head,MERGEABLE/CLEAN),BEFORE = origin/main(ca1ab06b),软链接复用 node_modules。Node v22.22.2。可干净合并;3 个源文件在 merge-base 与 main 之间一致。
| 检查项 | 结果 |
|---|---|
干净合入 main |
✅ MERGEABLE / CLEAN |
PR getDefaultPermission 测试(glob+grep+ripgrep) |
✅ 9/9 通过 |
| 回归测试有效性 —— PR 测试跑 BASE 源码 | ✅ 3 个 “ask for tilde” 测试失败,报 expected 'allow' to be 'ask';6 个 workspace 内测试两边都过 → 证明 base 存在该绕过 |
边界用例探测(真实 workspace + 真实 resolvePath + 真实 containment) |
✅ 7/7(见下) |
tsc --noEmit core |
✅ 0 新增类型错误(干净,与 base 一致) |
prettier --check + eslint(6 文件)+ git diff --check |
✅ 均 exit 0 |
这些测试是真实行为证明、并非被 mock 掉:真实的 /tmp 下临时 workspace、真实 containment 检查、真实的 resolvePath——所以 ~/outside-workspace → /root/outside-workspace(不在 /tmp/... 内)→ PR 上返回 ask,而 base 的 path.resolve 得到 <workspace>/~/outside-workspace → allow。
边界用例探测(真实 glob 工具,PR head)
| 输入 | 结果 |
|---|---|
无 path / sub / ./sub |
allow ✅(正常 workspace 路径不受影响) |
裸 ~(home 目录) |
ask ✅ |
~/x(workspace 外 tilde) |
ask ✅ |
绝对路径 /tmp |
ask ✅ |
../../..(向上穿越) |
ask ✅ |
说明(非阻塞)
- 与执行一致: 权限检查现在用
resolvePath,而执行时用的resolveAndValidatePath也依赖同样的~展开——所以检查与实际搜索现在对“路径指向哪里”达成一致。✔ path导入仍需要:glob.ts/grep.ts/ripGrep.ts其他地方仍用到path,不会产生未使用导入问题。- 无伤大雅:
ripGrep.test.ts里工具变量名叫grepTool,但实际是new RipGrepTool(...)——这是该文件里既有的命名习惯,并非本 PR 引入;新测试确实测的是 ripgrep 工具。
Verified locally under tmux on worktrees at the PR head and origin/main: PR permission tests, a genuine-regression run of the PR's tests against base source (3 tilde tests flip allow→ask), and an edge-case probe driving the real glob tool through real resolvePath + real workspace-containment for bare ~, tilde, absolute, and parent-traversal paths.
✅ Maintainer verification — real local + tmux testing (backs my approval)Built the PR head ( What it fixesAll three search tools ( Real-TUI A/B (tmux, DEFAULT approval mode)Real binary, isolated
Pre-fix read an out-of-workspace home file with zero confirmation — exactly the gap this closes. Mutation (deterministic,
|
修复前(path.resolve) |
修复后(resolvePath) |
|
|---|---|---|
~/outside-workspace 的权限 |
allow |
ask |
| 确认对话框 | 无 —— 自动执行 | 弹出 "Do you want to proceed?",等待用户 |
| workspace 外的文件 | 静默搜索并读取了 ~/outside-workspace/secret.txt → Found 1 match … NEEDLE_PR5378_SECRET |
被显式确认拦住 |
修复前: ✓ Grep 'NEEDLE_PR5378_SECRET' in path '~/outside-workspace' (无弹框, 读取了 /tmp/pr5378-home/outside-workspace/secret.txt)
修复后: ? Grep 'NEEDLE_PR5378_SECRET' in path '~/outside-workspace' → Do you want to proceed? 1. Yes … 4. No (等待用户确认…)
修复前在零确认下读取了 workspace 外的 home 文件 —— 正是本 PR 关掉的缺口。
变异测试(确定性,getDefaultPermission 层)
把 3 个源文件还原到 base(保留 PR 的测试)→ 三个新测试 should return ask for tilde paths outside workspace 失败,报 expected 'allow' to be 'ask'(直接显示 修复前=allow / 修复后=ask)。而非 tilde 的 should return ask for paths outside workspace(ripGrep)在 mutant 上仍通过 → 漏洞专属于 tilde 路径(绝对的 workspace 外路径本就正确)。allow for paths within workspace 两端都过 → 无误杀。
无回归
FIXED 构建上 glob.test.ts(46) · grep.test.ts(37) · ripGrep.test.ts(60) 143 / 143 全过。
反向审计补充
- 三个工具的
getDefaultPermission模式完全一致 —— 修复统一。 ~user路径(如~bob/x)不会被resolvePath展开,仍留在 workspace 内(<cwd>/~bob/x),不会逃逸 —— 无残留漏洞;真正的逃逸向量是~和~/,都已修复。- 超出范围的小瑕疵(非阻塞、既有、不在本 diff): 确认框里 "Always allow … in
<path>" 的作用域标签仍显示字面 tilde 路径<cwd>/~/outside-workspace/(由 tool-confirmation 框架生成,不是getDefaultPermission),所以对 tilde 路径选"always allow"可能 allowlist 到错误路径。值得单独跟进;不影响本 PR 的 ask/allow 判定。
结论
正确、精简、测试充分的安全修复。实时 tmux A/B 证明它在 DEFAULT 审批模式下关闭了一个真实的*"静默搜索并读取 workspace 外内容"*缺口(YOLO 模式下无影响,那里一切自动批准)。建议合并。(我已 approve;剩余的 BLOCKED 是必需的 checks,不是 review 异议。)
Method: isolated worktree build of 4052e8232 · source-mutation on the 3 getDefaultPermission methods (revert → the 3 tilde tests flip allow→ask, non-tilde stays correct) · real-TUI tmux A/B in DEFAULT approval mode (pre-fix silently reads ~/outside-workspace, fixed prompts) · 143/143 glob+grep+ripGrep tests on the rebuilt FIXED binary.
What this PR does
This PR makes the search tools use the existing tilde-aware path resolver before deciding the default permission for an explicit search path.
glob,grep, andripgrepnow resolve~/...the same way they resolve paths during validation instead of treating~as a literal child of the workspace.Why it's needed
The permission check previously used
path.resolve(targetDir, params.path). For a path like~/outside-workspace, Node treats~as a normal path segment, so the resolved path appears to be inside the workspace even though the user meant a home-directory path. That can make search tools default toallowfor a path that should require confirmation.Reviewer Test Plan
How to verify
Run the focused search tool tests and confirm each tool still allows a normal relative workspace path while returning
askfor a tilde path outside the workspace.Evidence (Before & After)
Before:
~/outside-workspacecould be resolved as a literal workspace child path during the default permission check. After:resolvePath()expands~first, so the workspace containment check sees the real home-directory path and returnsaskwhen it is outside the workspace.Tested on
Environment (optional)
Node 22, local repository checkout.
Risk & Scope
~paths may now require confirmation when they resolve outside the workspace.Linked Issues
Fixes #5376
AI Assistance Disclosure
I used Codex to review the changes, sanity-check the implementation against existing patterns, and help spot potential edge cases.
中文说明
What this PR does
这个 PR 让 search tools 在判断显式搜索路径的默认权限前,使用已有的支持
~展开的路径解析逻辑。glob、grep和ripgrep现在会像路径校验阶段一样解析~/...,而不是把~当成 workspace 下的普通字面目录。Why it's needed
之前 permission check 使用
path.resolve(targetDir, params.path)。对~/outside-workspace这样的路径,Node 会把~当成普通路径片段,所以解析结果看起来像在 workspace 内,尽管用户实际表达的是 home 目录路径。这会让 search tools 对本应确认的路径默认返回allow。Reviewer Test Plan
How to verify
运行聚焦的 search tool 测试,并确认每个工具仍然允许普通的 workspace 相对路径,同时对 workspace 外的 tilde 路径返回
ask。Evidence (Before & After)
Before:
~/outside-workspace在默认权限检查里可能被解析成 workspace 下的字面子路径。After:resolvePath()会先展开~,所以 workspace containment check 会看到真实的 home 目录路径,并在它位于 workspace 外时返回ask。Tested on
Environment (optional)
Node 22,本地仓库 checkout。
Risk & Scope
~目录的显式 tilde 路径,现在如果解析到 workspace 外,会要求确认。Linked Issues
Fixes #5376
AI Assistance Disclosure
I used Codex to review the changes, sanity-check the implementation against existing patterns, and help spot potential edge cases.