fix(workflows): validate runId before recursive prune delete (path-traversal dir wipe) - #5740
Merged
Merged
Conversation
Collaborator
|
(Re-run of prior triage — updating in place.) Thanks for the PR! The PR body doesn't follow the template headings — uses "Summary / The bug / The fix / Tests" instead of "What this PR does / Why it's needed / Reviewer Test Plan / Risk & Scope / Linked Issues". Normally that would be a template gate failure, but the content quality here is genuinely high: clear vulnerability description, concrete attack scenario, minimal fix, and test evidence. Not going to block a security fix on heading names — flagging it for future reference only. On direction: this is a clean-cut security fix. On approach: the fix is exactly the right shape — one regex guard ( Moving on to code review. 🔍 中文说明（先前审查的重跑——就地更新。） 感谢贡献！ PR 正文没有使用模板的标准标题（用了 "Summary / The bug / The fix / Tests" 而不是 "What this PR does / Why it's needed / Reviewer Test Plan / Risk & Scope / Linked Issues"）。按规则这本应是模板门槛失败，但内容质量确实很高：清晰的漏洞描述、具体的攻击场景、最小化修复和测试证据。不会因为标题名称阻断一个安全修复——仅作提醒。 方向：这是一个明确的安全修复。 方案：修复的方式完全正确——在递归删除前加一个正则守卫（ 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Collaborator
|
(Re-run of prior triage — updating in place.) Code ReviewThe fix is surgical and correct. One regex guard ( The conditional spread ( The comment block above the guard is longer than usual but earns its place: it documents a security-critical invariant (why this regex, what attack it prevents) that would otherwise be non-obvious to the next reader. That's the one case where comments are genuinely load-bearing. No issues found. Typecheck clean. CI all green across Linux, macOS, Windows. Test VerificationRED→GREEN confirmed (from prior triage run). Without the fix, the crafted Before (without fix — RED)After (with fix — GREEN)All 8 existing tests continue to pass. The new path-traversal regression test catches the real vulnerability (parent directory wiped via 中文说明代码审查修复精准且正确。在递归
守卫上方的注释块比一般代码长，但有存在的理由：它记录了一个安全关键的不变量（为什么用这个正则、防御什么攻击），这些信息对下一个阅读代码的人来说不是显而易见的。这正是注释真正不可或缺的场景。 未发现问题。类型检查通过。CI 在 Linux、macOS、Windows 全部绿色。 测试验证RED→GREEN 已确认（来自先前审查）。没有修复时，构造的
现有的 8 个测试继续通过。新的路径遍历回归测试捕获了真实的漏洞（通过 — Qwen Code · qwen3.7-max |
Collaborator
|
(Re-run of prior triage — updating in place.) This is a clean security fix that does exactly what it says. The vulnerability is real — verified by reverting the fix and watching the canary file get deleted by the CI is all green across Linux, macOS, and Windows. The code is mergeable, and the approach matches what I would have written independently: gate the recursive delete on the same One note on the PR description's threat model (not a code blocker): the actual blast radius is The PR body doesn't use the template headings (minor), but the substance is excellent. Nothing to change here. Re-approving. ✅ 中文说明（先前审查的重跑——就地更新。） 这是一个干净的安全修复，完全如其所述。漏洞是真实的——通过回退修复并观察 canary 文件被 CI 在 Linux、macOS、Windows 全部绿色。代码可合并，方案与我独立会写的方案一致：在递归删除上加 关于 PR 描述中的威胁模型说明（不影响代码）：实际影响范围是 PR 正文没有使用模板标题（小问题），但内容质量很高。无需更改。 重新批准 ✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
previously approved these changes
Jun 23, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
pruneSnapshots derived runId from a snapshot filename and passed it straight to fs.rm(..., { recursive: true, force: true }). The listing is a plain *.json glob, so a file named ...json yields runId '..' and the rm deletes the runs dir's PARENT (the project root, .git and all); notarun.json deletes a sibling dir. A malicious repo could commit such a file under workflows/; once a victim runs more than the retention cap, the prune wipes their project.
Gate the recursive delete on the generated wf_<hex> run-id shape (the same pattern workflow.ts already uses to validate resumeFromRunId). The .json unlink stays unconditional — it removes exactly that one file, never a directory. Carries a RED-before-fix test that plants ...json / notarun.json as the oldest snapshots and asserts the parent canary and a sibling dir survive.
Follow-up to PR #5600 (merged); addresses review thread r3451484367.
LaZzyMan
force-pushed
the
lazzy/fix-workflow-prune-path-traversal
branch
from
June 23, 2026 03:55
1d8d44c to
35ef8dc
Compare
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
wenshao
reviewed
Jun 23, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: CI still running.
Reviewed the pruneSnapshots path-traversal guard and the regression test. Local verification passed: npx vitest run src/agents/workflow-snapshot.test.ts, qwen review deterministic, npm run build, and npm run typecheck after build.
— GPT-5 Codex via Qwen Code /review
Collaborator
✅ Local verification report — real build + runtime A/BVerdict: the fix is correct and verified — recommend merge. One note on the threat‑model wording in the description (does not block the code). I rebuilt a clean minimal A/B on top of current Layer 1 — Real test suite on the fixed code ✅
Layer 2 — Revert‑proof: the new test on the unpatched source ❌→ (RED as required)I put the PR's new test on top of the vulnerable This proves the test genuinely catches the bug (RED without the fix, GREEN with it). Layer 3 — Standalone real‑filesystem exploit A/B ✅A vitest‑independent driver runs the real
The fix blocks every out‑of‑bounds delete while keeping both desired behaviors: stray Layer 4 — Typecheck & format ✅
Code assessment ✅The guard Note on severity / threat model — for the PR description, not a code blockerThe fix is right; I'd only suggest tightening the framing of the description:
Suggestion: soften the description from "wiping the project, 🇨🇳 中文版（点击展开）✅ 本地验证报告 —— 真实构建 + 运行时 A/B 对照结论：修复正确且已验证通过，建议合并。 仅对描述中的“威胁模型”措辞有一点建议（不影响代码本身）。 我在当前 第 1 层 —— 修复后代码上的真实测试 ✅ 第 2 层 —— 反向验证：把新测试放到“未修复”源码上 ❌（按预期变红） 这证明该测试确实能捕获此 bug（无修复时红、加修复后绿）。 第 3 层 —— 独立的真实文件系统漏洞利用 A/B ✅
修复阻止了所有越界删除，同时保留两个应有行为：杂散 第 4 层 —— 类型检查与格式 ✅
代码评估 ✅ 关于严重性 / 威胁模型的说明 —— 针对 PR 描述，不阻塞代码
建议： 把描述里的 “wiping the project, Verified locally via tmux: minimal 2‑file A/B vs current |
| await expect(fs.access(canary)).resolves.toBeUndefined(); | ||
| await expect( | ||
| fs.access(path.join(dir, 'notarun', 'keep.txt')), | ||
| ).resolves.toBeUndefined(); |
Collaborator
There was a problem hiding this comment.
[Suggestion] The test asserts that the canary file and sibling directory survived, but doesn't verify that the malicious .json files themselves were actually unlinked. Since the fix intentionally keeps fs.unlink unconditional (only gating the recursive fs.rm), asserting the cleanup actually happened would strengthen the regression test — a bug that skipped pruning entirely would also pass this test because the canaries would trivially survive when nothing is pruned.
Suggested change
| ).resolves.toBeUndefined(); | |
| // The guard spared both the parent canary and the non-run sibling dir. | |
| await expect(fs.access(canary)).resolves.toBeUndefined(); | |
| await expect( | |
| fs.access(path.join(dir, 'notarun', 'keep.txt')), | |
| ).resolves.toBeUndefined(); | |
| // The malicious .json files themselves were still unlinked (only the fs.rm was gated). | |
| await expect( | |
| fs.access(path.join(dir, '...json')), | |
| ).rejects.toThrow(); | |
| await expect( | |
| fs.access(path.join(dir, 'notarun.json')), | |
| ).rejects.toThrow(); |
— qwen3.7-max via Qwen Code /review
wenshao
approved these changes
Jun 23, 2026
Collaborator
|
@qwen-code /triage |
qwen-code-ci-bot
approved these changes
Jun 23, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A path-traversal in
pruneSnapshotslets a crafted snapshot filename drive an unboundedfs.rm(..., { recursive: true, force: true }), deleting directories outside the workflow runs dir — up to and including the project root (.gitand all).Fast-follow on #5600 (Dynamic Workflows port, merged), addressing the review thread #5600 (comment) that surfaced just before merge.
The bug
pruneSnapshotslists*.jsonsnapshots, derivesrunId = file.replace(/\.json$/, ''), and removes each pruned run's sibling journal dir:The listing is a plain
.jsonglob, so the filename controlsrunId:runIdfs.rmtarget...json..notarun.jsonnotarunnotarun/dirAttack: a malicious repo commits
workflows/...json. When a victim clones, enables workflows, and exceedsMAX_RETAINED_SNAPSHOTSruns, pruning selects that file and recursively deletes the parent of the runs dir — wiping the project,.gitincluded.The fix
Gate the recursive delete on the generated
wf_<hex>run-id shape — the same patternworkflow.tsalready uses to validateresumeFromRunId:The
.jsonunlinkstays unconditional (it removes exactly one file, never a directory), so stray/malicious snapshot files are still cleaned up — they just can't drive a recursive delete. Legitimatewf_<hex>journal dirs are still pruned as before.Tests
A RED-before-fix regression test plants
...jsonandnotarun.jsonas the oldest snapshots, fills past the retention cap to trigger pruning, and asserts a canary in the parent dir and a sibling dir both survive. Verified RED on the pre-fix code (canary deleted,ENOENT) → GREEN after. The existing "prunes journal dirs too" test still passes.workflow-snapshot.test.ts9/9 passcoretypecheck + lint clean