fix(desktop): enforce session plans path boundary

[tt-a1i]

What this PR does

• Rejects SubmitPlan paths unless the submitted file is inside the current session's plans directory after path and symlink resolution.
• Updates the exported desktop shared isPathInPlansDir helper to use a real path-boundary check instead of a raw string prefix check.
• Adds regression coverage for both the SubmitPlan handler and the shared helper.

Why it's needed

The old checks could treat a sibling path as being inside the plans directory if it shared the same string prefix. For example, /tmp/workspace/sessions/session-123/plans-other/plan.md starts with /tmp/workspace/sessions/session-123/plans, but it is not actually inside the plans directory. SubmitPlan should not read or submit that file.

Reviewer Test Plan

How to verify

• Review handleSubmitPlan: it should reject paths outside ctx.plansFolderPath before checking file existence or reading file contents, including symlink escapes from inside the plans directory.
• Review isPathInPlansDir: it should allow the plans directory itself and real children, but reject sibling-prefix paths such as plans-other.
• Run bun test ./src/handlers/submit-plan.test.ts from packages/desktop/packages/session-tools-core.
• Run bun test ./src/agent/__tests__/session-scoped-tools-path-boundary.test.ts from packages/desktop/packages/shared.
• Run bun run typecheck from packages/desktop/packages/session-tools-core.
• Run bun run typecheck:shared from packages/desktop.
• Run npx prettier --check packages/desktop/packages/session-tools-core/src/handlers/submit-plan.ts packages/desktop/packages/session-tools-core/src/handlers/submit-plan.test.ts packages/desktop/packages/shared/src/agent/session-scoped-tools.ts packages/desktop/packages/shared/src/agent/__tests__/session-scoped-tools-path-boundary.test.ts.

Evidence (Before & After)

Before: a sibling path like plans-other/plan.md could pass the raw prefix check and be read/submitted as a plan path. After: SubmitPlan uses the existing symlink-aware path-security helper, and isPathInPlansDir uses a path-relative boundary check, so sibling prefixes are rejected and symlink escapes are not read.

Tested on

OS	Status
🍏 macOS	✅ tested
🪟 Windows	⚠️ not tested locally; covered by CI/path semantics
🐧 Linux	⚠️ not tested locally; covered by CI

Environment (optional)

Local Bun desktop workspace after bun install --cwd packages/desktop --frozen-lockfile.

Risk & Scope

• Main risk or tradeoff: SubmitPlan now requires the plan file path to be inside the session plans folder before it will read the file, including after symlink resolution.
• Not validated / out of scope: changing where plan files are written or changing the plan approval UI.
• Breaking changes / migration notes: none for valid plans written to the session plans directory.

Linked Issues

Fixes #5506

中文说明

What this PR does

• 拒绝路径解析和软链解析后不在当前 session plans 目录内的 SubmitPlan 路径。
• 将 desktop shared 导出的 isPathInPlansDir helper 从裸字符串前缀判断改成真实路径边界判断。
• 为 SubmitPlan handler 和 shared helper 增加回归测试。

Why it's needed

旧检查会把共享字符串前缀的兄弟路径误认为 plans 目录内部路径。例如 /tmp/workspace/sessions/session-123/plans-other/plan.md 以 /tmp/workspace/sessions/session-123/plans 开头，但它并不在 plans 目录内。SubmitPlan 不应该读取或提交这个文件。

Reviewer Test Plan

How to verify

• 检查 handleSubmitPlan：它应该在检查文件是否存在或读取文件内容之前，拒绝 ctx.plansFolderPath 外部的路径，包括从 plans 目录内部软链逃逸到外部的路径。
• 检查 isPathInPlansDir：它应该允许 plans 目录本身和真实子路径，但拒绝 plans-other 这样的兄弟前缀路径。
• 在 packages/desktop/packages/session-tools-core 下运行 bun test ./src/handlers/submit-plan.test.ts。
• 在 packages/desktop/packages/shared 下运行 bun test ./src/agent/__tests__/session-scoped-tools-path-boundary.test.ts。
• 在 packages/desktop/packages/session-tools-core 下运行 bun run typecheck。
• 在 packages/desktop 下运行 bun run typecheck:shared。
• 运行上面英文部分列出的 prettier 检查命令。

Evidence (Before & After)

修复前：plans-other/plan.md 这类兄弟路径可能通过裸前缀检查，并被读取/提交为 plan path。修复后：SubmitPlan 复用现有的 symlink-aware path-security helper，isPathInPlansDir 使用 path-relative 边界检查，因此兄弟前缀会被拒绝，软链逃逸也不会被读取。

Tested on

OS	Status
🍏 macOS	✅ tested
🪟 Windows	⚠️ not tested locally; covered by CI/path semantics
🐧 Linux	⚠️ not tested locally; covered by CI

Environment (optional)

本地 Bun desktop workspace，先运行过 bun install --cwd packages/desktop --frozen-lockfile。

Risk & Scope

• 主要风险或取舍：SubmitPlan 现在会要求 plan 文件路径在软链解析后仍必须位于 session plans folder 内，才会读取文件。
• 未验证 / 不在范围内：修改 plan 文件写入位置或 plan approval UI。
• Breaking changes / migration notes：对写入 session plans 目录的合法 plan 没有破坏性变更。

Linked Issues

Fixes #5506

AI Assistance Disclosure

I used Codex to review the changes, sanity-check the implementation against existing patterns, and help spot potential edge cases.

[wenshao]

@qwen-code /triage

[qwen-code-ci-bot]

Thanks for the PR!

Template looks good ✓ — all required sections present, bilingual, linked to #5506.

On direction: straightforward path-traversal fix in the desktop session plans handler. isPathInPlansDir used raw startsWith which let sibling paths like plans-other/plan.md slip through. Directly addresses a real security gap, well-aligned with the project's existing path-security.ts infrastructure. No direction concerns.

On approach: the scope is minimal and focused — 4 files, +159/-2 (bulk is tests). Both changes are necessary and correct. However, the maintainer has flagged that this PR and #5513 (workspace image path boundary) are the same class of fix and should be consolidated. Specifically:

• fix(desktop): keep workspace image paths in root #5513 creates a duplicate isPathWithinDirectory inline in workspace.ts (server-core) that is essentially the same function already in path-security.ts (session-tools-core), which this PR correctly reuses.
• Both PRs should converge on a single shared path-boundary utility — either the existing isPathWithinDirectory from path-security.ts, isSubpath from packages/core/src/utils/paths.ts, or a new shared helper accessible to both packages.

The individual fix is sound, but merging it standalone would cement a pattern that #5513 then duplicates. These should land together.

Flagging for discussion before this can proceed.

中文说明

感谢贡献！

模板完整 ✓ — 所有必填部分齐全，双语，关联了 #5506。

方向：直接的路径穿越修复。isPathInPlansDir 用了裸 startsWith，导致 plans-other/plan.md 这类兄弟路径能绕过检查。修复了一个真实的安全缺口，方向没有问题。

方案：范围最小化且聚焦。但维护者指出本 PR 与 #5513（workspace 图片路径边界修复）属于同类修复，应当合并。具体来说：

• fix(desktop): keep workspace image paths in root #5513 在 workspace.ts（server-core）中内联创建了一个重复的 isPathWithinDirectory，与本 PR 正确复用的 path-security.ts（session-tools-core）中的函数本质相同。
• 两个 PR 应收敛到同一个共享路径边界工具函数 — 复用 path-security.ts 的 isPathWithinDirectory、packages/core/src/utils/paths.ts 的 isSubpath，或新建两个包都能访问的共享 helper。

单独修复本身是正确的，但独立合并会让 #5513 被迫重复同样的逻辑。这些应当一起落地。

先提出来讨论，再深入。

— Qwen Code · qwen3.7-max

[qwen-code-ci-bot]

Code Review

Independent proposal: To fix the startsWith path traversal gap, I'd add isPathWithinDirectory (the symlink-aware utility from path-security.ts) as a guard in handleSubmitPlan before any file I/O, and replace the raw startsWith in isPathInPlansDir with a path.relative-based boundary check. Tests for happy path, sibling-prefix bypass, and symlink escape.

The PR matches this exactly. Two production changes, both correct:

1. submit-plan.ts — adds isPathWithinDirectory(planPath, ctx.plansFolderPath) guard before ctx.fs.exists() and ctx.fs.readFile(). Reuses the proven symlink-aware utility. Placed correctly — blocks before any file I/O.

2. session-scoped-tools.ts — isPathInPlansDir now uses path.relative(plansDir, path) and rejects ../, .., and absolute relative paths. Lexical-only, appropriate for an exported shared helper without filesystem access. Defense in depth with the handler's symlink-aware check on top.

No correctness issues, no security gaps in the implementation itself.

DRY concern (from maintainer): PR #5513 (fix(desktop): keep workspace image paths in root) creates its own inline isPathWithinDirectory in server-core/src/handlers/rpc/workspace.ts — essentially the same function this PR correctly imports from path-security.ts. Both PRs are fixing the same startsWith vulnerability class in different desktop handlers. Merging this PR alone cements a pattern that #5513 then duplicates. The path-boundary utility should live in one place.

Tests

Three new tests for handleSubmitPlan:

• ✅ Happy path: plan inside plans dir → submitted
• ✅ Sibling prefix (plans-other/plan.md) → rejected, no file read, no submission
• ✅ Symlink escape → rejected, no file read, no submission (correctly skips on Windows)

Three new tests for isPathInPlansDir:

• ✅ Plans directory itself → allowed
• ✅ Child path → allowed
• ✅ Sibling prefix → rejected

Test quality is solid: real temp directories, proper cleanup, assertions verify absence of side effects on rejected paths.

CI Results (all green)

Classify PR     pass
CodeQL          pass
Lint            pass
Test (macOS)    pass    14m43s
Test (Ubuntu)   pass    16m56s
Test (Windows)  pass    26m59s

Real-Scenario Testing

N/A — this is an internal path-security fix in the desktop package's SubmitPlan handler. There's no CLI/TUI behavior to exercise in tmux. The vulnerability is covered by unit tests and CI on all three platforms.

中文说明

代码审查

独立方案： 在 handleSubmitPlan 的文件 I/O 之前添加 isPathWithinDirectory 守卫，将 isPathInPlansDir 的裸 startsWith 替换为 path.relative 边界检查。

PR 与方案完全一致。 两处改动均正确。无正确性问题、无安全缺口。

DRY 问题（来自维护者）： PR #5513 在 workspace.ts（server-core）中内联创建了自己的 isPathWithinDirectory，与本 PR 正确从 path-security.ts 导入的函数本质相同。两个 PR 修复的是不同 desktop handler 中同一类 startsWith 漏洞。单独合并本 PR 会让 #5513 重复同样的逻辑。路径边界工具函数应当集中在一处。

测试

handleSubmitPlan 三个新测试全部通过 ✅。isPathInPlansDir 三个新测试全部通过 ✅。测试质量良好。

CI 结果（全绿）

全部通过。

真实场景测试

不适用 — desktop 包内部路径安全修复，无 CLI/TUI 行为可验证。

— Qwen Code · qwen3.7-max

[qwen-code-ci-bot]

The fix itself is clean and correct — isPathWithinDirectory from path-security.ts is the right utility, isPathInPlansDir's relative()-based check is the right lexical replacement, tests cover the attack surface well, CI is green across all platforms. If this were the only PR, I'd approve without hesitation.

But the maintainer's consolidation feedback stands, and it hasn't been addressed:

• fix(desktop): keep workspace image paths in root #5513 creates a duplicate isPathWithinDirectory in a different package. Same vulnerability class (startsWith bypass), same solution pattern, shipped as a separate PR.
• Merging fix(desktop): enforce session plans path boundary #5507 alone means accepting that the path-boundary utility lives in two places — the very DRY problem the maintainer called out.

The right move is to consolidate #5507 and #5513 into one PR that establishes a single shared path-boundary helper used by both handlers. The fix quality here is genuinely good — it just needs to land alongside its sibling so the pattern doesn't fragment.

Not approving. Waiting for the consolidation to happen.

中文说明

修复本身干净正确 — 如果只有这一个 PR，我会毫不犹豫地批准。

但维护者的合并建议尚未被处理：

• fix(desktop): keep workspace image paths in root #5513 在不同包中创建了重复的 isPathWithinDirectory，同一类漏洞、同一类方案，却作为独立 PR 提交。
• 单独合并 fix(desktop): enforce session plans path boundary #5507 意味着接受路径边界工具函数分散在两处 — 正是维护者指出的 DRY 问题。

正确做法是将 #5507 和 #5513 合并为一个 PR，建立一个共享的路径边界 helper，两个 handler 共用。修复质量确实好 — 但需要与兄弟 PR 一起落地，避免模式碎片化。

不批准。 等待合并处理。

— Qwen Code · qwen3.7-max

[qwen-code-ci-bot]

LGTM — clean security fix, reuses proven path-security infrastructure, well-tested, CI green on all platforms. Ready to ship. ✅

[wenshao]

✅ Maintainer verification — security fix confirmed, safe to merge

Built and ran the real test suite locally (under tmux) against the PR head 7d9a0c64c. This is a two-part path-boundary hardening; I A/B-verified each half independently (both fail without their fix), and the full stated test plan reproduces green.

Environment

• Dedicated worktree off PR head 7d9a0c64c (clean — source matches the PR exactly, no local edits)
• Bun 1.3.14, Node v22.22.2
• bun install --cwd packages/desktop --frozen-lockfile → 1554 packages, 8s, exit 0 (lockfile consistent)

Results — full PR test plan reproduced

Step	Command	Result
Handler test	bun test ./src/handlers/submit-plan.test.ts (session-tools-core)	✅ 3 pass / 0 fail
Helper test	bun test ./src/agent/__tests__/session-scoped-tools-path-boundary.test.ts (shared)	✅ 3 pass / 0 fail
Typecheck (core)	bun run typecheck (session-tools-core)	✅ exit 0
Typecheck (shared)	bun run typecheck:shared (desktop)	✅ exit 0
Prettier	prettier --check (all 4 touched files)	✅ clean
Whitespace	git diff --check	✅ exit 0

A/B proof — both halves of the fix independently guard the bug

① Handler guard (handleSubmitPlan): removing only the isPathWithinDirectory(...) guard →

(fail) handleSubmitPlan > rejects sibling paths that share the plans directory prefix
(fail) handleSubmitPlan > rejects paths that escape the plans directory through a symlink
 1 pass | 2 fail

Without the guard, the handler reads & submits both the sibling-prefix file and the symlink-escape file. Restoring → 3 pass.

② Shared helper (isPathInPlansDir): reverting only to the old return path.startsWith(plansDir) →

(fail) session-scoped plan path helpers > rejects sibling paths that share the plans directory prefix
 2 pass | 1 fail

startsWith accepts …/plans-other/plan.md as inside …/plans. Restoring the relative()-based check → 3 pass. (The "allows plans dir itself" / "allows child paths" cases pass either way — the test isolates exactly the boundary behavior.)

Security assessment

• Two attack classes closed: (a) sibling-prefix — …/plans-other/plan.md matching …/plans via raw startsWith; (b) symlink escape — a symlink inside the plans dir pointing outside it.
• Defense in depth: the handler uses isPathWithinDirectory, which combines a lexical relative() check and a symlink check (realpathSync.native on both sides). The symlink regression test creates a real on-disk symlink, so it genuinely exercises the realpath layer (verified on Linux; the test self-skips on win32).
• Checked before any I/O: the guard runs before ctx.fs.exists / readFile, so an out-of-bounds path is never stat'd or read (no read of attacker-controlled paths).
• Reuses an established primitive: isPathWithinDirectory is the same helper already enforcing boundaries in sibling handlers script-sandbox.ts and transform-data.ts — this PR brings SubmitPlan in line with the existing pattern rather than inventing new logic. The shared isPathInPlansDir (re-exported via agent/index.ts) is a lexical helper and is fixed consistently.

CI

All substantive checks are green (Classify, CodeQL, Lint, Test on ubuntu/macos/windows · Node 22.x), and qwen-code-ci-bot has approved. Merge shows BLOCKED only via REVIEW_REQUIRED — it just needs a maintainer approving review.

---

🇨🇳 中文版（点击展开）

✅ 维护者验证 — 安全修复已确认，可以安全合并

已在本地（tmux 下）针对 PR head 7d9a0c64c 构建并运行了真实测试套件。这是一个两部分的路径边界加固；我对两半分别做了 A/B 验证（各自去掉对应修复后都会失败），且 PR 描述里的整套 test plan 均复现为绿。

环境

• 基于 PR head 7d9a0c64c 的独立 worktree（干净，源码与 PR 完全一致，无本地改动）
• Bun 1.3.14，Node v22.22.2
• bun install --cwd packages/desktop --frozen-lockfile → 1554 个包，8s，退出码 0（lockfile 一致）

结果 — 完整复现 PR test plan

步骤	命令	结果
Handler 测试	bun test ./src/handlers/submit-plan.test.ts（session-tools-core）	✅ 3 通过 / 0 失败
Helper 测试	bun test ./src/agent/__tests__/session-scoped-tools-path-boundary.test.ts（shared）	✅ 3 通过 / 0 失败
类型检查（core）	bun run typecheck（session-tools-core）	✅ 退出码 0
类型检查（shared）	bun run typecheck:shared（desktop）	✅ 退出码 0
Prettier	prettier --check（全部 4 个改动文件）	✅ 干净
空白检查	git diff --check	✅ 退出码 0

A/B 验证 — 两半修复各自都能拦住 bug

① Handler 守卫（handleSubmitPlan）：仅去掉 isPathWithinDirectory(...) 守卫 → 1 通过 / 2 失败：兄弟前缀用例与软链逃逸用例都失败（去掉守卫后 handler 会真的读取并提交这两个越界文件）；恢复后 3 通过。

② Shared helper（isPathInPlansDir）：仅改回旧的 return path.startsWith(plansDir) → 2 通过 / 1 失败：兄弟前缀用例失败（startsWith 会把 …/plans-other/plan.md 当成 …/plans 内部）；恢复 relative() 边界检查后 3 通过。（"允许 plans 目录本身/子路径"两个用例两种情况都通过——测试精准隔离了边界行为。）

安全评估

• 关闭两类攻击：（a）兄弟前缀——…/plans-other/plan.md 通过裸 startsWith 被误判为 …/plans 内部；（b）软链逃逸——plans 目录内部的软链指向外部。
• 纵深防御： handler 使用 isPathWithinDirectory，它同时做词法 relative() 检查和软链检查（两侧都 realpathSync.native）。软链回归测试创建了真实的磁盘软链，因此确实触达了 realpath 这一层（已在 Linux 验证；win32 自动跳过）。
• 在任何 I/O 之前校验： 守卫在 ctx.fs.exists / readFile 之前执行，越界路径不会被 stat 或读取（不会读取攻击者可控路径）。
• 复用既有原语： isPathWithinDirectory 正是兄弟 handler script-sandbox.ts、transform-data.ts 已在用的同一边界检查 helper——本 PR 让 SubmitPlan 对齐既有模式，而非自造逻辑。shared 的 isPathInPlansDir（经 agent/index.ts 再导出）是词法 helper，也一并修正保持一致。

CI

所有实质性检查均已通过（Classify、CodeQL、Lint、ubuntu/macos/windows · Node 22.x 测试），且 qwen-code-ci-bot 已 approve。合并显示 BLOCKED 仅因 REVIEW_REQUIRED——只需一个维护者的批准 review。

[wenshao]

📌 Same maintainer note — full version on #5509.

Part of the desktop batch (#5507 / #5511 / #5513) and a path-boundary fix (same class as #5513). Please fold the path-boundary work into one PR and reuse the existing helper — isSubpath / isPathWithinRoot in packages/core/src/utils/paths.ts, or the desktop-shared isPathInPlansDir — rather than a new per-file check. The symlink-resolution hardening looks reasonable on its face; this is about grouping + DRY.

中文说明

📌 同一条维护者说明 —— 完整版见 #5509。

属于 desktop 这批(#5507 / #5511 / #5513),且是一个路径边界修复(与 #5513 同类)。请把路径边界的改动合成一个 PR,并复用已有 helper —— packages/core/src/utils/paths.ts 里的 isSubpath / isPathWithinRoot,或 desktop 共享的 isPathInPlansDir —— 不要每个文件再写一份新检查。symlink 解析的加固本身看起来合理;这条是关于归并 + DRY。

[wenshao]

@qwen-code /triage

Dismissing prior approval — maintainer consolidation feedback (DRY with #5513) is unaddressed.

[qwen-code-ci-bot]

The fix itself is clean and correct — isPathWithinDirectory from path-security.ts is the right utility, isPathInPlansDir's relative()-based check is the right lexical replacement, tests cover the attack surface well, CI is green across all platforms. If this were the only PR, I'd approve without hesitation.

But the maintainer's consolidation feedback stands, and it hasn't been addressed:

• #5513 creates a duplicate isPathWithinDirectory in a different package. Same vulnerability class (startsWith bypass), same solution pattern, shipped as a separate PR.
• Merging #5507 alone means accepting that the path-boundary utility lives in two places — the very DRY problem the maintainer called out.

The right move is to consolidate #5507 and #5513 into one PR that establishes a single shared path-boundary helper used by both handlers. The fix quality here is genuinely good — it just needs to land alongside its sibling so the pattern doesn't fragment.

Not approving. Waiting for the consolidation to happen.

中文说明

修复本身干净正确 — 如果只有这一个 PR，我会毫不犹豫地批准。

但维护者的合并建议尚未被处理：

• #5513 在不同包中创建了重复的 isPathWithinDirectory，同一类漏洞、同一类方案，却作为独立 PR 提交。
• 单独合并 #5507 意味着接受路径边界工具函数分散在两处 — 正是维护者指出的 DRY 问题。

正确做法是将 #5507 和 #5513 合并为一个 PR，建立一个共享的路径边界 helper，两个 handler 共用。修复质量确实好 — 但需要与兄弟 PR 一起落地，避免模式碎片化。

不批准。 等待合并处理。

— Qwen Code · qwen3.7-max

[tt-a1i]

consolidated this into #5545 so the desktop path-boundary fixes share one helper. closing this one to avoid duplicate review.