fix(desktop): keep workspace image paths in root

[tt-a1i]

What this PR does

This PR hardens the workspace image RPC handlers (READ_IMAGE / WRITE_IMAGE) so that resolved image paths cannot escape the workspace root via symlinks. The previous boundary check relied only on a string startsWith(workspace.rootPath) comparison on the joined path, which can be bypassed when an entry inside the workspace is a symlink (or has a symlinked parent directory) that points outside the root.

The new logic resolves the real path of both the workspace root and the target via realpathSync.native, and verifies containment with a proper path.relative based check (isPathWithinDirectory) instead of a raw string prefix match. For writes to a not-yet-existing file, it walks up to the nearest existing ancestor and validates that ancestor's real path is still inside the workspace, so a symlinked parent directory or a broken final symlink is rejected before any file is written. Reads of a missing optional image continue to return null rather than throwing, preserving existing behavior. Legitimate cases — including a workspace whose root itself is a symlink — are still allowed.

A new focused test file adds coverage for these read/write image path boundaries.

Why it's needed

Without resolving symlinks, the startsWith containment check could be tricked into reading or writing files outside the workspace directory (a path-traversal / symlink-escape issue). This fixes that boundary while keeping the existing "missing optional image returns null" behavior intact. See the linked issue for the reported problem.

Reviewer Test Plan

How to verify

• bun test packages/desktop/packages/server-core/src/handlers/rpc/workspace.image-path.test.ts
• bun run typecheck in packages/desktop/packages/server-core
• npx prettier --check packages/desktop/packages/server-core/src/handlers/rpc/workspace.ts packages/desktop/packages/server-core/src/handlers/rpc/workspace.image-path.test.ts
• git diff --check

Evidence (Before & After)

N/A — internal logic change covered by unit tests. The new workspace.image-path.test.ts asserts: missing optional images return null; reads/writes that escape via a symlink or symlinked parent throw outside workspace directory and create no outside file; and overwriting an image works when the workspace root is itself a symlink.

Tested on

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

Environment (optional)

Local macOS workspace; unit tests via npm/vitest (desktop server-core uses bun:test, so the test command above is run via bun).

Risk & Scope

• Main risk or tradeoff: low; scope-limited to the workspace image READ_IMAGE / WRITE_IMAGE path-containment check. The symlink-aware checks add realpathSync/lstatSync calls, but only on the image read/write paths.
• Not validated / out of scope: manual end-to-end run in the packaged desktop app.
• Breaking changes / migration notes: none.

Linked Issues

Fixes #5512

AI Assistance Disclosure

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

中文说明

这个 PR 做了什么

本 PR 加固了工作区图片的 RPC 处理器（READ_IMAGE / WRITE_IMAGE），使得解析后的图片路径无法通过符号链接（symlink）逃逸出工作区根目录。此前的边界检查只是对拼接后的路径做字符串 startsWith(workspace.rootPath) 比较，当工作区内的某个条目是指向根目录之外的符号链接（或其父目录是符号链接）时，这种检查可以被绕过。

新逻辑通过 realpathSync.native 解析工作区根目录和目标路径的真实路径，并使用基于 path.relative 的正确包含性检查（isPathWithinDirectory）来替代原始的字符串前缀匹配。对于写入尚不存在的文件，它会向上查找最近的已存在祖先目录，并校验该祖先目录的真实路径仍位于工作区内，因此在写入任何文件之前，符号链接的父目录或损坏的末端符号链接都会被拒绝。读取缺失的可选图片仍然返回 null 而非抛错，保持了既有行为。合法场景——包括工作区根目录本身就是符号链接的情况——依然被允许。

新增了一个专门的测试文件来覆盖这些读/写图片路径的边界。

为什么需要

如果不解析符号链接，startsWith 包含性检查可能被欺骗，从而读取或写入工作区目录之外的文件（一种路径穿越 / 符号链接逃逸问题）。本 PR 修复了该边界，同时保持了"缺失的可选图片返回 null"这一既有行为。具体问题见关联 Issue。

Reviewer Test Plan（评审测试计划）

如何验证

• bun test packages/desktop/packages/server-core/src/handlers/rpc/workspace.image-path.test.ts
• 在 packages/desktop/packages/server-core 中执行 bun run typecheck
• npx prettier --check packages/desktop/packages/server-core/src/handlers/rpc/workspace.ts packages/desktop/packages/server-core/src/handlers/rpc/workspace.image-path.test.ts
• git diff --check

证据（前后对比）

不适用 —— 这是内部逻辑改动，已由单元测试覆盖。新增的 workspace.image-path.test.ts 断言：缺失的可选图片返回 null；通过符号链接或符号链接父目录逃逸的读/写会抛出 outside workspace directory 且不会在外部创建任何文件；当工作区根目录本身是符号链接时，覆盖写入图片可以正常工作。

测试平台

操作系统	状态
🍏 macOS	⚠️ 本地未测试；由 CI 覆盖
🪟 Windows	⚠️ 本地未测试；由 CI 覆盖
🐧 Linux	⚠️ 本地未测试；由 CI 覆盖

环境（可选）

本地 macOS 工作区；单元测试通过 npm/vitest 运行（desktop server-core 使用 bun:test，因此上述测试命令通过 bun 运行）。

风险与范围

• 主要风险或权衡：低；范围仅限于工作区图片 READ_IMAGE / WRITE_IMAGE 的路径包含性检查。符号链接感知的检查增加了 realpathSync/lstatSync 调用，但仅作用于图片读/写路径。
• 未验证 / 范围之外：在打包后的桌面应用中进行手动端到端运行。
• 破坏性变更 / 迁移说明：无。

关联 Issue

Fixes #5512

AI 协助声明

我使用了 Codex 来评审改动、对照既有模式核查实现，并帮助发现潜在的边界情况。

[wenshao]

📌 Same maintainer note applies here — full version on #5509.

This is the desktop fix batch (#5507, #5509, #5511, #5513) and the second path-boundary one alongside #5507. Please fold the path-boundary work into a single PR and reuse the existing boundary helper — isSubpath / isPathWithinRoot in packages/core/src/utils/paths.ts, or the desktop-shared isPathInPlansDir — rather than a new per-file check. The symlink-escape hardening + null-on-missing-read look reasonable on their face; this is purely about grouping + DRY, not correctness.

Thanks! 🙏

中文说明

📌 同一条维护者说明也适用于此 —— 完整版见 #5509。

这是 desktop 这批修复(#5507、#5509、#5511、#5513)里第二个路径边界的(与 #5507 同类)。请把路径边界的改动合成一个 PR,并复用已有的边界 helper —— packages/core/src/utils/paths.ts 里的 isSubpath / isPathWithinRoot,或 desktop 共享的 isPathInPlansDir —— 不要每个文件再写一份新检查。防 symlink 逃逸 + 读不到时返回 null 本身看起来是合理的;这条纯粹是关于归并 + DRY,不涉及正确性。

谢谢!🙏

[wenshao]

@qwen-code /triage

[qwen-code-ci-bot]

Hey @tt-a1i, thanks for the security fix — symlink escape hardening on workspace image paths is a real issue worth addressing.

Two things blocking this from moving forward:

1. PR template: the body is missing several required sections from the PR template: "What this PR does", "Why it's needed", "Reviewer Test Plan" (with "How to verify", "Evidence (Before & After)", "Tested on" table), "Risk & Scope", "Linked Issues", and the Chinese translation in <details>. Please reformat the body to follow the template.

2. Consolidation needed: as @wenshao noted, this is one of four desktop fix PRs (#5507, #5509, #5511, #5513) and the second path-boundary one alongside #5507. Please fold the path-boundary work into a single PR. Also, the PR introduces a new isPathWithinDirectory helper, but the project already has isSubpath in packages/core/src/utils/paths.ts that does the same relative-path-prefix check. Reuse the existing helper rather than adding a per-file duplicate — the new isExistingWorkspacePath / isWorkspacePathAllowingMissingTarget functions can build on top of isSubpath for the structural check, and the realpath-based symlink hardening layer is the genuinely new contribution here.

Once the template is filled in and the path-boundary PRs are consolidated, happy to re-run the review.

中文说明

@tt-a1i 你好，感谢这个安全修复——workspace 图片路径的 symlink 逃逸加固确实是值得处理的问题。

目前有两点阻塞：

1. PR 模板：正文缺少 PR 模板 中多个必填章节："What this PR does"、"Why it's needed"、"Reviewer Test Plan"（含 "How to verify"、"Evidence (Before & After)"、"Tested on" 表格）、"Risk & Scope"、"Linked Issues" 以及 <details> 中的中文翻译。请按模板重新整理正文。

2. 需要合并：正如 @wenshao 指出，这是四篇 desktop 修复 PR（#5507、#5509、#5511、#5513）之一，也是与 #5507 同类的第二个路径边界修复。请把路径边界改动合并到一个 PR。另外，PR 新增了 isPathWithinDirectory helper，但项目已有 packages/core/src/utils/paths.ts 中的 isSubpath 做了同样的相对路径前缀检查。请复用已有 helper——新的 isExistingWorkspacePath / isWorkspacePathAllowingMissingTarget 可以在 isSubpath 之上构建结构检查，而基于 realpath 的 symlink 加固层才是这里真正的新贡献。

模板补全、路径边界 PR 合并后，随时可以重新审查。

— Qwen Code · qwen3.7-max

[wenshao]

@qwen-code /triage

[qwen-code-ci-bot]

Hey @tt-a1i, thanks for the security fix — symlink escape hardening on workspace image READ_IMAGE / WRITE_IMAGE is a real vulnerability worth addressing.

Template looks good — all required sections present ✓ (note: the previous review incorrectly flagged missing template sections; the PR body is complete).

On direction: this is squarely aligned with the project's security posture. Workspace-scoped image handlers must not follow symlinks outside the workspace. Fixes #5512 directly.

On approach: the core strategy — resolve real paths via realpathSync.native, walk up to the nearest existing ancestor for not-yet-created files — is correct and handles the key edge cases (symlinked parent dirs, broken final symlinks, workspace root that is itself a symlink). However, the new isPathWithinDirectory helper has a subtle bug: it checks !relativePath.startsWith('..') instead of !relativePath.startsWith('..' + sep) + relativePath !== '..'. This incorrectly rejects valid subdirectories named like ..foo (e.g., relative('workspace', 'workspace/..foo') returns '..foo' which starts with '..'). The existing isSubpath in packages/core/src/utils/paths.ts does this correctly. I realize isSubpath lives in the CLI core (@qwen-code/qwen-code-core) and isn't directly importable from @craft-agent/server-core, but the structural check logic should be copied verbatim rather than reimplemented with a weaker guard.

On consolidation: @wenshao asked to fold the path-boundary PRs (#5507, #5509, #5511, #5513) into a single PR. That's a maintainer process call — flagging it here but deferring to the maintainer on whether individual PRs are acceptable.

Moving on to code review and testing. 🔍

中文说明

@tt-a1i 你好，感谢这个安全修复——workspace 图片 READ_IMAGE / WRITE_IMAGE 的 symlink 逃逸加固确实是值得处理的安全漏洞。

模板完整 ✓（注意：此前的 review 错误地标记了模板缺失章节；PR 正文实际上是完整的）。

方向：与项目的安全立场完全一致。workspace 范围的图片处理器不应跟踪 symlink 逃逸出 workspace。直接修复 #5512。

方案：核心策略——通过 realpathSync.native 解析真实路径、对尚未创建的文件向上查找最近的已存在祖先目录——是正确的，处理了关键边界情况（symlink 父目录、损坏的末端 symlink、workspace 根目录本身是 symlink）。然而新的 isPathWithinDirectory helper 有一个细微 bug：它检查 !relativePath.startsWith('..') 而不是 !relativePath.startsWith('..' + sep) + relativePath !== '..'。这会错误地拒绝名为 ..foo 的合法子目录（例如 relative('workspace', 'workspace/..foo') 返回 '..foo'，以 '..' 开头）。packages/core/src/utils/paths.ts 中已有的 isSubpath 正确处理了这一点。我理解 isSubpath 在 CLI core（@qwen-code/qwen-code-core）中，不能直接从 @craft-agent/server-core 导入，但结构性检查逻辑应逐字复制而不是用更弱的守卫重新实现。

关于合并：@wenshao 要求将路径边界 PR（#5507、#5509、#5511、#5513）合并为一个 PR。这是维护者的流程决定——在此标记但交由维护者决定单独 PR 是否可接受。

进入代码审查和测试阶段 🔍

— Qwen Code · qwen3.7-max

[qwen-code-ci-bot]

Code Review

Independent proposal (before reading the diff): the fix should resolve symlinks via realpath on both the workspace root and the target path, then do a proper containment check. For not-yet-existing files, walk up to the nearest existing ancestor and resolve that. Ideally reuse the existing isPathWithinRoot from workspaceContext.ts or isSubpath from the CLI core for the structural check, since both handle the .. edge case correctly.

The PR's approach matches this conceptually — the realpath resolution layer, the ancestor-walking for missing files, and the "workspace root is a symlink" handling are all correct. Two code-level concerns:

1. isPathWithinDirectory has a subtle bug (minor): The check !relativePath.startsWith('..') is too aggressive — it rejects valid subdirectories named ..foo (e.g., relative('/ws', '/ws/..foo') returns '..foo' which starts with '..'). The existing isSubpath in packages/core/src/utils/paths.ts handles this correctly with !relative.startsWith('..' + path.sep) + relative !== '..'. Since isSubpath isn't directly importable from @craft-agent/server-core (it's in @qwen-code/qwen-code-core, not @craft-agent/core), the structural check logic should be copied verbatim. In practice the ..foo edge case is unlikely for image paths, but for a security-hardening function it's worth getting right.

2. isWorkspacePathAllowingMissingTarget calls isExistingWorkspacePath with different argument types: The function resolves paths at the top level but then passes the unresolved workspaceRoot to isExistingWorkspacePath, which re-resolves internally. This double-resolution is harmless (resolve is idempotent) but slightly confusing to read.

Neither is a merge blocker — the security hardening works correctly for all realistic image path scenarios. The ..foo issue is a correctness nit worth fixing in a follow-up or during consolidation.

Test Results

All 5 new tests pass:

bun test v1.3.14 (0d9b296a)

::group::src/handlers/rpc/workspace.image-path.test.ts:
✓ workspace image path boundaries > returns null for missing optional images inside the workspace [1.80ms]
✓ workspace image path boundaries > rejects image reads that escape through a symlink [2.07ms]
✓ workspace image path boundaries > rejects image writes that escape through a symlinked parent directory [0.97ms]
✓ workspace image path boundaries > rejects image writes through a broken final symlink [0.56ms]
✓ workspace image path boundaries > allows overwriting an image when the workspace root is a symlink [0.72ms]

::endgroup::

 5 pass
 0 fail
 7 expect() calls
Ran 5 tests across 1 file. [133.00ms]

Typecheck clean, prettier clean, no regressions in existing validate-file-path tests (13/13 pass).

中文说明

代码审查

独立方案（在读 diff 之前）：修复应该通过 realpath 解析 workspace 根目录和目标路径的 symlink，然后做正确的包含性检查。对于尚不存在的文件，向上查找最近的已存在祖先目录并解析它。理想情况下应复用 workspaceContext.ts 中已有的 isPathWithinRoot 或 CLI core 中的 isSubpath 来做结构性检查，因为两者都正确处理了 .. 边界情况。

PR 的方案在概念上与此一致——realpath 解析层、缺失文件的祖先查找、以及"workspace 根目录本身是 symlink"的处理都是正确的。两个代码层面的问题：

1. isPathWithinDirectory 有一个细微 bug（轻微）： !relativePath.startsWith('..') 的检查过于激进——它会拒绝名为 ..foo 的合法子目录（例如 relative('/ws', '/ws/..foo') 返回 '..foo'，以 '..' 开头）。packages/core/src/utils/paths.ts 中已有的 isSubpath 用 !relative.startsWith('..' + path.sep) + relative !== '..' 正确处理了这一点。由于 isSubpath 不能直接从 @craft-agent/server-core 导入（它在 @qwen-code/qwen-code-core 中，不是 @craft-agent/core），结构性检查逻辑应逐字复制。实践中 ..foo 边界情况对于图片路径不太可能发生，但对于安全加固函数值得做对。

2. isWorkspacePathAllowingMissingTarget 以不同参数类型调用 isExistingWorkspacePath： 函数在顶层解析了路径，但随后将未解析的 workspaceRoot 传给 isExistingWorkspacePath，后者内部会重新解析。这种双重解析无害（resolve 是幂等的）但阅读时略显困惑。

两者都不是合并阻塞项——安全加固对所有现实的图片路径场景都正确工作。..foo 问题是一个值得在后续或合并时修复的正确性细节。

测试结果

5 个新测试全部通过：

bun test v1.3.14 (0d9b296a)

::group::src/handlers/rpc/workspace.image-path.test.ts:
✓ workspace image path boundaries > returns null for missing optional images inside the workspace [1.80ms]
✓ workspace image path boundaries > rejects image reads that escape through a symlink [2.07ms]
✓ workspace image path boundaries > rejects image writes that escape through a symlinked parent directory [0.97ms]
✓ workspace image path boundaries > rejects image writes through a broken final symlink [0.56ms]
✓ workspace image path boundaries > allows overwriting an image when the workspace root is a symlink [0.72ms]

::endgroup::

 5 pass
 0 fail
 7 expect() calls
Ran 5 tests across 1 file. [133.00ms]

Typecheck 通过，prettier 通过，已有 validate-file-path 测试无回归（13/13 通过）。

— Qwen Code · qwen3.7-max

[qwen-code-ci-bot]

Stepping back: this PR does one thing well — it closes a symlink-escape hole in the workspace image read/write path. The approach (realpath resolution + ancestor walking for missing files + symlink-root workspace handling) is the right approach for this class of vulnerability. Five focused tests cover the key scenarios, typecheck and formatting are clean, no regressions in existing path validation tests.

The isPathWithinDirectory ..foo bug is a real correctness issue but extremely unlikely in practice for image paths — nobody names icon directories ..icons. Worth fixing, not worth blocking.

The consolidation question (@wenshao's request to fold #5507/#5509/#5511/#5513 into one PR and reuse isSubpath) is the bigger open item. From a code perspective, the duplicated containment-check logic across these PRs is exactly the kind of DRY concern that become maintenance burden. But blocking a security fix on consolidation with three other PRs also has a cost — the vulnerability stays open longer.

My honest take: the security fix is correct and ready to ship. The ..foo nit and the consolidation are both valid but neither should hold up a vulnerability fix. I'd suggest merging this as-is, then addressing DRY in the consolidated PR that the maintainer is requesting for the remaining path-boundary work.

Not approving or requesting changes — deferring to @wenshao on the consolidation decision.

中文说明

退一步看：这个 PR 做好了一件事——修补 workspace 图片读/写路径中的 symlink 逃逸漏洞。方案（realpath 解析 + 缺失文件的祖先查找 + symlink 根 workspace 处理）对于此类漏洞是正确的。5 个专注的测试覆盖了关键场景，typecheck 和格式化都干净，已有路径验证测试无回归。

isPathWithinDirectory 的 ..foo bug 是真实的正确性问题但在图片路径实践中极不可能出现——没有人会把图标目录命名为 ..icons。值得修复，不值得阻塞。

合并问题（@wenshao 要求将 #5507/#5509/#5511/#5513 合并为一个 PR 并复用 isSubpath）是更大的未决项。从代码角度看，这些 PR 之间重复的包含性检查逻辑正是会变成维护负担的 DRY 问题。但将安全修复合并到其他三个 PR 中也有代价——漏洞保持开放更久。

我的真实看法： 安全修复是正确的，可以合并。..foo 细节和合并都是合理的但都不应阻塞漏洞修复。建议按现状合并，然后在维护者要求的剩余路径边界工作的合并 PR 中处理 DRY。

不做 approve 或 request changes——将合并决定交给 @wenshao。

— 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.