fix(serve): allow dotdot-prefixed cwd subdirectories - #9848
Conversation
Use the shared path containment helper so workspace-local directories such as ..build are not treated as escapes while real parent traversal remains rejected. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 12 scenario(s). — Qwen Code · serve A/B |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Thanks for the fix, @AaronZ345. The direction looks reasonable — the current inline check treats any relative path starting with .. as parent traversal, which wrongly rejects contained directories like ..build, and the shared containment helper already handles that distinction correctly. Before we can take this further, though, the PR description needs to follow the repository's pull request template.
Right now the body only has ## Summary and ## Test plan; the sections the template requires are missing:
- What this PR does — partly covered by your Summary, needs to move into the template section.
- Why it's needed — missing, and this is the important one. This is a
fix:PR, but there is no linked issue or before/after reproduction showing that a contained..-prefixed directory is actually rejected in real usage of the serve routes. Please describe the scenario that hits this, or link an issue. - Reviewer Test Plan (
How to verify/Evidence (Before & After)/Tested on) — missing; your three checked commands are a good start for "How to verify". - Risk & Scope — missing.
- Linked Issues — missing.
Please edit the PR description to fill in the template — above all Why it's needed with a reproduction — then re-run triage with @qwen-code /triage.
中文说明
感谢提交,@AaronZ345。修复方向是合理的——目前的内联检查会把所有以 .. 开头的相对路径都当作上级目录穿越,从而错误地拒绝像 ..build 这样位于工作区内的目录,而仓库里共享的路径包含判断辅助函数已经能正确区分这两种情况。不过在继续审查之前,PR 描述需要先符合仓库的 PR 模板。
目前 PR 正文只有 ## Summary 和 ## Test plan,模板要求的部分均缺失:
- What this PR does —— Summary 已部分覆盖,需要迁移到模板对应章节。
- Why it's needed —— 缺失,也是最重要的一项。这是一个
fix:PR,但没有关联 issue,也没有 before/after 复现来证明 serve 路由的真实使用中确实会出现「工作区内以..开头的目录被拒绝」的情况。请补充触发此问题的场景,或关联相应 issue。 - Reviewer Test Plan(
How to verify/Evidence (Before & After)/Tested on)—— 缺失;你已勾选的三条命令很适合作为 "How to verify" 的内容。 - Risk & Scope —— 缺失。
- Linked Issues —— 缺失。
请编辑 PR 描述补全模板——尤其是 Why it's needed 和复现——然后使用 @qwen-code /triage 重新触发审查。
— Qwen Code · qwen3.8-max
|
@qwen-code /triage |
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.
Not reviewed: reverse audit — stopped before round 1 by the review time budget.
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
@qwen-code /triage |
|
@qwen-code /review --severity-floor critical |
|
@qwen-code /triage |
1 similar comment
|
@qwen-code /triage |
|
All review threads are resolved and CI is green. The |
|
@qwen-code /triage |
1 similar comment
|
@qwen-code /triage |
|
@qwen-code /review\n\nThe latest review reports no findings. Please refresh the review decision for this PR. |
|
@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. 中文 — 判定:
|
|
Thanks for the PR, and for filling in the template — the earlier block was only the missing template sections, and that's resolved now. ✓ Problem: Real but rare. The old Direction: Aligned. This brings the serve cwd check in line with the repo's existing segment-aware containment helper instead of carrying a divergent inline check. The sibling instances of the same bug class named in #8835 (acpAgent.ts, session.ts, run-qwen-serve.ts, fs/policy.ts) remain report-only there awaiting maintainer judgment — correctly out of this PR's scope. Claude Code's CHANGELOG has no direct counterpart (the serve daemon is qwen-code-specific), but it carries several recent path-containment correctness fixes, so the area is squarely in scope. Size: Core-module gate not applicable — Approach: Minimal and the right shape — both Risk: No elevated risk signals — none of the revert-correlated high-risk paths are touched. Moving on to code review. 🔍 中文说明感谢提交,也感谢补全模板——此前的阻塞仅因描述缺少模板章节,现已解决。✓ 问题: 真实但罕见。旧的 方向: 对齐。此改动让 serve 的 cwd 检查与仓库已有的分段感知包含判断辅助函数保持一致,而不是各自维护一套内联检查。#8835 中点名的同类缺陷的其余位置(acpAgent.ts、session.ts、run-qwen-serve.ts、fs/policy.ts)在该 issue 中仍为 report-only、等待 maintainer 判断——不在本 PR 范围内是正确的。Claude Code 的 CHANGELOG 没有直接对应项(serve 守护进程是 qwen-code 特有的),但其中有多条近期的路径包含正确性修复,说明该领域在核心方向之内。 规模: 核心模块门槛不适用—— 方案: 最小且形态正确—— 风险: 无升级风险信号——未触及任何与 revert 相关的高风险路径。 进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewIndependent proposal first, before reading the diff: for this defect I'd swap the inline check for the repo's existing segment-aware Semantics check: both call sites pass already-realpath'd paths, where the helper rejects exactly No blocking findings. Test evidence — the PR's own CI on the reviewed commitNothing was built or run locally (review is static; fork code is never executed here). The table below is the PR's own CI on
Reading the signal: no red checks anywhere. The ubuntu unit suite ran the two new regression tests green, and they genuinely pin the change — statically, the old Sandboxed verification would settle that last gap: 中文说明代码审查先说独立方案:看到这个缺陷,我会把内联检查替换为仓库已有的分段感知 语义核对:两个调用点传入的都是已经过 realpath 的路径,此时该辅助函数恰好拒绝 无阻塞性发现。 测试证据 —— PR 自身的 CI(被审查提交)本地未构建、未运行任何代码(审查为静态;fork 代码不在此执行)。上表为 沙盒验证可以补上最后这个缺口: — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — clean, minimal correctness fix pinned by green regression tests; the only loose ends are pre-existing (fork CI gating skips the macOS/Windows legs) and tracked elsewhere (#8835). Reflection: this one earns its merge. The problem is real if unfashionable — nobody names a directory Approving, pinned to the reviewed commit. ✅ 中文说明置信度:4/5 —— 干净、最小的正确性修复,回归测试全绿且确实钉住改动;遗留的小尾巴都是既有问题(fork 的 CI 限流跳过了 macOS/Windows 腿),且已在别处跟踪(#8835)。 复盘:这个 PR 值得合入。问题真实存在,只是不常见——没人会故意把目录命名为 批准合入,审批固定于被审查的提交。✅ — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
Maintainer verification — real local environment, base vs headI built this PR into a full bundle and drove it against two live
That gap is now closed. The PR does exactly what its description claims, changes nothing else, and I found no blocking issue. Setup — how the A/B was made airtight
1. Live route behaviour
So on 2. Nothing new gets in16 cwd shapes against the live daemon, both builds: The only rows that flip base→head are 3. Do the new tests actually pin it?
4. Windows, since CI skipped it here
12/12 as intended: 5. Repo health on the merged tree
6. Two #8835 sibling claims, re-tested (out of scope — informational)
VerdictNo blocking findings. The relaxation is provably narrow, the escape surface is unchanged in a live daemon, and the behaviour the PR describes is reproduced end-to-end on the real HTTP routes rather than only at the resolver. Good to merge from my side; the only thing standing in the way is the review decision, and the earlier 中文说明维护者验证 —— 本地真实环境,base 对 head 双跑我把这个 PR 打成完整 bundle,挂到两个真实运行的
这个口子现在补上了。PR 的行为与描述完全一致,没有任何额外影响,未发现阻塞问题。 环境搭建 —— 如何让 A/B 无懈可击
1. 真实路由行为
也就是说, 2. 没有放进任何新东西对活着的守护进程跑了 16 种 cwd 形态,两侧都跑: base→head 唯一翻转的行是 3. 新测试是否真的钉住了行为?
4. Windows:因为本 PR 的 CI 跳过了它本 PR 上 12/12 符合预期: 5. 合并树的仓库健康度
6. 顺带复测 #8835 的两条兄弟结论(不在本 PR 范围,仅供参考)
结论无阻塞问题。放宽范围可证明是窄的,活体守护进程上的逃逸面没有变化,PR 所描述的行为在真实 HTTP 路由上端到端复现,而不只是停留在 resolver 层。从我这边看可以合入;目前唯一挡着的是评审决策,而此前的 |





What this PR does
Updates serve workspace cwd containment checks so an in-workspace directory whose basename starts with
..(for example<workspace>/..build) is treated as contained, while real parent traversal such as<workspace>/../outsideremains rejected.Why it's needed
The serve git routes accept a
cwdunder the workspace. The previous inline check usedrel.startsWith('..'), sopath.relative(workspace, path.join(workspace, '..build'))produced..buildand was incorrectly classified as escaping the workspace. Read routes could silently fall back to the workspace root and mutation routes could reject the cwd withinvalid_cwd, even though the path is inside the workspace. This aligns the check with the repository's segment-aware containment rule: reject only..,../..., and absolute relatives.Reviewer Test Plan
How to verify
Run the targeted serve runtime tests and confirm that dotdot-prefixed in-workspace directories are accepted while real parent traversal is still rejected.
Evidence (Before & After)
Before: the added regression test for
..buildfails because the oldstartsWith('..')check treats it as parent traversal. After: the targeted test suite passes, and existing parent traversal rejection remains covered.Tested on
Environment (optional)
Local Node/npm workspace on macOS.
Risk & Scope
Linked Issues
Refs #8835 (
contained-cwd-dotdot-prefix).中文说明
What this PR does
更新 serve workspace 的 cwd 包含性判断:如果工作区内目录名以
..开头(例如<workspace>/..build),仍然视为工作区内路径;真正的父目录逃逸(例如<workspace>/../outside)仍会被拒绝。Why it's needed
serve 的 git 路由允许传入工作区内的
cwd。旧逻辑使用rel.startsWith('..'),所以path.relative(workspace, path.join(workspace, '..build'))得到..build后会被误判为逃逸工作区。读取类路由可能静默回退到工作区根目录,变更类路由可能返回invalid_cwd,但该路径实际仍在工作区内。本 PR 将判断改为仓库已有的分段感知规则:只拒绝..、../...和绝对相对路径。Reviewer Test Plan
How to verify
运行目标 serve runtime 测试,确认工作区内
..build这类目录可被接受,同时真正的父目录逃逸仍被拒绝。Evidence (Before & After)
Before:新增的
..build回归测试会失败,因为旧的startsWith('..')检查会把它当作父目录穿越。After:目标测试套件通过,已有父目录逃逸拒绝逻辑仍被覆盖。Tested on
Environment (optional)
macOS 本地 Node/npm 工作区。
Risk & Scope
Linked Issues
Refs #8835 (
contained-cwd-dotdot-prefix).