fix(core): reject Windows-style workspace artifact paths

[VectorPeak]

What this PR does

Tightens record_artifact workspacePath validation so Windows-style absolute paths and backslash traversal are rejected consistently, including when Qwen Code is running on POSIX/Linux.

Why it's needed

What Problem This Solves

record_artifact accepts workspacePath as a workspace-relative metadata locator. The tool already rejected native absolute paths and POSIX-style parent-directory traversal, but its validation relied partly on the host platform's path semantics.

On POSIX/Linux, backslash is not a path separator. That meant Windows-style path strings such as ..\secret.txt, reports\..\..\secret.txt, C:\tmp\report.html, or UNC-like paths could be treated as ordinary relative strings at the record_artifact boundary, even though they do not match the intended workspace-relative contract.

This change makes the validation portable: path strings carrying Windows absolute, drive-relative, UNC-like, or backslash traversal semantics are rejected consistently before record_artifact reports success.

Changes

• Adds Windows-aware absolute path checks with path.win32.isAbsolute(...).
• Rejects drive-prefixed paths such as C:\tmp\report.html, C:/tmp/report.html, and C:tmp\report.html.
• Normalizes backslashes to POSIX separators before checking whether the path escapes the workspace.
• Keeps ordinary workspace-relative artifact paths valid, including reports/summary.html and reports\summary.html.
• Leaves record_artifact scoped to metadata only; it still does not read, write, upload, publish, or verify the referenced resource.

Evidence

The PR changes only packages/core/src/tools/record-artifact.ts and packages/core/src/tools/record-artifact.test.ts.

The implementation now checks native absolute paths, Windows absolute paths, drive-prefixed paths, and portable parent-directory traversal before accepting workspacePath.

Regression coverage was expanded to reject:

• ../secret.txt
• ..\secret.txt
• ..\..\secret.txt
• reports\..\..\secret.txt
• reports/..\..\secret.txt
• C:\tmp\report.html
• C:/tmp/report.html
• C:tmp\report.html
• \\server\share\report.html
• \tmp\report.html

The tests also confirm that normal workspace-relative paths still resolve successfully for both slash and backslash examples.

Possible call chain / impact

A caller provides workspacePath to record_artifact, RecordArtifactTool.build(...) validates the locator, and execute(...) returns artifact metadata only after validation succeeds.

Before this change, some Windows-style escape or absolute-path strings could pass validation when the tool was running on POSIX/Linux because backslashes were treated as plain characters. After this change, those strings are rejected at build-time, so clients receive a validation error instead of a successful artifact metadata result.

The expected impact is limited to invalid workspacePath inputs. Valid workspace-relative artifact paths should continue to work unchanged.

Reviewer Test Plan

How to verify

Confirm record_artifact still accepts normal workspace-relative paths such as reports/summary.html and reports\summary.html, and rejects ../secret.txt, ..\secret.txt, ..\..\secret.txt, reports\..\..\secret.txt, reports/..\..\secret.txt, C:\tmp\report.html, C:/tmp/report.html, C:tmp\report.html, \\server\share\report.html, and \tmp\report.html before reporting success.

Evidence (Before & After)

N/A - non-UI validation change. Regression coverage is in packages/core/src/tools/record-artifact.test.ts.

Focused validation run on Windows:

> npm run test --workspace=packages/core -- src/tools/record-artifact.test.ts
✓ src/tools/record-artifact.test.ts (15 tests)
Test Files  1 passed (1)
Tests       15 passed (15)

Additional local validation:

> npm run typecheck --workspace=packages/core
exit 0

> npm run lint --workspace=packages/core
exit 0

> git diff --check
exit 0

Linux/WSL validation:

> npm run test --workspace=packages/core -- src/tools/record-artifact.test.ts
✓ src/tools/record-artifact.test.ts (15 tests)
Test Files  1 passed (1)
Tests       15 passed (15)

Note: this checkout shares node_modules between Windows and WSL. Running the focused test on WSL required installing Rollup's Linux optional package; restoring the Windows optional package afterward kept the Windows focused test/typecheck/lint path green. No package files were changed.

Tested on

OS	Status
🍎 macOS	not tested
🪟 Windows	✅ tested
🐧 Linux	✅ tested via WSL Ubuntu-22.04

Environment (optional)

Windows: Node.js v24.15.0, npm 11.12.1. Linux: WSL Ubuntu-22.04 with Node.js v24.15.0.

Risk & Scope

• Main risk or tradeoff: Low; this rejects cross-platform path strings that violate the existing workspace-relative artifact locator contract, while preserving ordinary relative paths.
• Not validated / out of scope: This is not a broad daemon artifact-store rewrite and does not claim a high-severity path traversal exploit. The downstream artifact store keeps its own containment checks.
• Breaking changes / migration notes: None expected for valid workspace-relative artifact paths.

Linked Issues

N/A

中文说明

本 PR 做了什么

收紧 record_artifact 的 workspacePath 校验，让 Windows 风格的绝对路径和反斜杠父目录跳转在 POSIX/Linux 环境下也会被一致拒绝。

为什么需要

workspacePath 是一个指向当前 workspace 内产物文件的 metadata locator。之前的校验能拒绝本机绝对路径和 POSIX 风格的 ../...，但依赖当前运行平台的 node:path 分隔符语义。

在 POSIX/Linux 上，反斜杠不会被当作路径分隔符，因此 ..\secret.txt、reports\..\..\secret.txt、C:\tmp\report.html 这类 Windows 风格路径可能在工具入口处看起来像普通相对字符串。这会让 record_artifact 的工具层校验弱于它声明的 workspace-relative 语义。

本 PR 保持范围很窄：record_artifact 仍然只记录 artifact metadata，不读取、不写入、不上传、也不验证引用资源；这里只是在返回成功 artifact 结果前拒绝带有 Windows 绝对路径、盘符相对路径、UNC 风格路径或反斜杠 traversal 语义的字符串。

Reviewer Test Plan

验证 record_artifact 仍接受 reports/summary.html 和 reports\summary.html 这类正常 workspace 相对路径，同时拒绝 ../secret.txt、..\secret.txt、..\..\secret.txt、reports\..\..\secret.txt、reports/..\..\secret.txt、C:\tmp\report.html、C:/tmp/report.html、C:tmp\report.html、\\server\share\report.html 和 \tmp\report.html。

本地已在 Windows 和 WSL Ubuntu-22.04 上跑过 focused test。Windows 上还跑过 core typecheck、core lint 和 git diff --check。

风险与范围

风险较低；该修改只拒绝违反现有 workspace-relative contract 的跨平台路径字符串，不改 artifact 读取、写入、上传或 daemon store 行为。它不应被表述为高危 path traversal 漏洞。

[qwen-code-ci-bot]

Thanks for the PR!

Template looks good ✓

Problem: The gap is real — I verified on this Linux box that the old validateWorkspacePath passes ..\\secret.txt through without rejection. On POSIX, path.normalize('..\\secret.txt') returns '..\\secret.txt' verbatim (backslash isn't a separator), and path.isAbsolute returns false, so neither existing check catches it. The PR's own framing is honest though: no linked issue, no reproduction of this actually firing in the wild. The concern is a cross-platform validation gap, not an observed bug.

Direction: Aligned. workspacePath declares workspace-relative semantics in its schema description; tightening validation to be platform-independent is consistent with that contract. The tool only records metadata (doesn't read/write), so the practical blast radius of a Windows-style path slipping through on Linux is limited to stale artifact metadata — but the validation contract should still be honored.

Size: 14 production lines (+9/-5 in record-artifact.ts), 57 test lines (+50/-7 in record-artifact.test.ts). Core path (packages/core/src/tools/), well under any threshold. Not applicable for escalation.

Approach: The scope is right — one function, one concern, corresponding tests. The fix uses path.posix.normalize(trimmed.replace(/\\/g, '/')) which is a clean, portable approach. One observation: the old code uses platform-native path.isAbsolute() for the "must be relative" check while the new code adds path.win32.isAbsolute() and a drive-letter regex alongside it. This means the absolute-path check now uses three conditions instead of one. It's correct, but I'd want to verify there's no overlap where one condition subsumes another (e.g., path.posix.isAbsolute(portableNormalized) at the end already catches UNC paths after normalization — is path.win32.isAbsolute still needed for those?). Not a blocker, just a question for the author to think about.

Moving on to code review. 🔍

中文说明

感谢贡献！

模板完整 ✓

问题： 差距确实存在——我在 Linux 上验证了旧的 validateWorkspacePath 不会拒绝 ..\\secret.txt。在 POSIX 上，path.normalize('..\\secret.txt') 原样返回（反斜杠不是分隔符），path.isAbsolute 返回 false，所以现有检查都抓不到。PR 自己的描述也很诚实：没有关联 issue，没有实际触发的复现。这是一个跨平台校验缺口，而非已观测到的 bug。

方向： 对齐。workspacePath 的 schema 描述声明了 workspace-relative 语义；收紧校验使其与平台无关，与该契约一致。该工具只记录 metadata（不读写），所以 Windows 风格路径在 Linux 上通过的实际影响仅限于过期的 artifact metadata——但校验契约仍然应该被遵守。

规模： 14 行生产代码（record-artifact.ts +9/-5），57 行测试代码（record-artifact.test.ts +50/-7）。核心路径（packages/core/src/tools/），远低于任何阈值。无需上报。

方案： 范围合理——一个函数、一个关注点、对应测试。修复使用 path.posix.normalize(trimmed.replace(/\\/g, '/'))，简洁且跨平台。一个观察：旧代码对"必须相对"检查使用平台原生 path.isAbsolute()，新代码在其基础上增加了 path.win32.isAbsolute() 和盘符正则。绝对路径检查现在有三个条件。逻辑正确，但想确认条件之间是否有重叠（例如末尾的 path.posix.isAbsolute(portableNormalized) 已经能在规范化后捕获 UNC 路径——path.win32.isAbsolute 对这些情况是否仍然必要？）。不是 blocker，只是供作者思考的问题。

进入代码审查 🔍

— Qwen Code · qwen3.7-max

[qwen-code-ci-bot]

Code Review

The diff is focused — one function (validateWorkspacePath), 14 production lines, and the tests to match. I walked through the logic for every rejected path on Linux and confirmed the fix catches all of them:

Input	Caught by
../secret.txt	path.isAbsolute → no; traversal check (portableNormalized.startsWith('../')) → yes
..\\secret.txt	drive regex → no; path.win32.isAbsolute → no; traversal check (../secret.txt) → yes
reports\\..\\..\\secret.txt	traversal check (../secret.txt after normalize) → yes
C:\\tmp\\report.html	drive regex → yes
C:/tmp/report.html	drive regex → yes
C:tmp\\report.html	drive regex → yes
\\\\server\\share\\report.html	path.win32.isAbsolute → yes; also path.posix.isAbsolute after normalize → yes
\\tmp\\report.html	path.win32.isAbsolute → yes; also path.posix.isAbsolute after normalize → yes

Safe paths (reports/summary.html, reports\\summary.html) pass correctly.

One minor observation: path.win32.isAbsolute() is fully redundant given the other two conditions. Drive paths (C:\..., C:/..., C:tmp\...) are all caught by the /^[A-Za-z]:/.test() regex (which is actually broader — it also catches drive-relative paths like C:report.html that path.win32.isAbsolute misses). UNC paths (\\server\...) and backslash-absolute paths (\tmp\...) are caught by path.posix.isAbsolute(portableNormalized) after \ → / normalization. Not a blocker — the redundancy is harmless and arguably makes intent clearer — but removing it would simplify the condition to just two checks.

No critical blockers. No AGENTS.md violations. The code follows project conventions (ESM imports, node:path, collocated tests, no unnecessary abstractions).

Test Results

This is a non-UI validation change — tmux scenario testing is N/A. Ran unit tests directly:

Before (main branch, without PR)

 RUN  v3.2.4 packages/core

 ✓ records a link artifact without touching the resource 27ms
 ✓ records workspace and managed artifacts with inferred storage 4ms
 ✓ rejects published storage 5ms
 ✓ requires exactly one locator 3ms
 ✓ rejects workspace traversal and unsafe urls before reporting success 3ms
 ✓ rejects path-like managed ids before reporting success 3ms
 ✓ rejects storage values that do not match the locator 4ms
 ✓ rejects artifact metadata that the daemon store would drop 3ms
 ✓ rejects invalid artifact sizes before reporting success 3ms
 ✓ rejects unsafe display markup before reporting success 3ms
 ✓ allows benign words ending with on before equals signs 3ms
 ✓ rejects Unicode control characters before reporting success 3ms
 ✓ accepts line whitespace in descriptions but not titles 3ms

 Test Files  1 passed (1)
      Tests  13 passed (13)

13 tests pass. The old test only covers ../secret.txt (POSIX traversal) — no Windows-style paths tested.

After (this PR)

 RUN  v3.2.4 packages/core

 ✓ records a link artifact without touching the resource 28ms
 ✓ records workspace and managed artifacts with inferred storage 5ms
 ✓ rejects published storage 5ms
 ✓ requires exactly one locator 3ms
 ✓ rejects workspace paths that escape the workspace 4ms
 ✓ accepts safe workspace-relative artifact paths 3ms
 ✓ rejects unsafe urls before reporting success 3ms
 ✓ rejects path-like managed ids before reporting success 4ms
 ✓ rejects storage values that do not match the locator 3ms
 ✓ rejects artifact metadata that the daemon store would drop 3ms
 ✓ rejects invalid artifact sizes before reporting success 3ms
 ✓ rejects unsafe display markup before reporting success 4ms
 ✓ allows benign words ending with on before equals signs 3ms
 ✓ rejects Unicode control characters before reporting success 4ms
 ✓ accepts line whitespace in descriptions but not titles 3ms

 Test Files  1 passed (1)
      Tests  15 passed (15)

All 15 tests pass. The PR splits the old combined test into focused rejects workspace paths that escape the workspace (10 Windows/POSIX traversal variants) and accepts safe workspace-relative artifact paths (positive cases for both / and \ relative paths), plus keeps the URL-only test separate. Good test organization.

中文说明

代码审查

Diff 聚焦于一个函数（validateWorkspacePath），14 行生产代码加对应测试。我在 Linux 上逐一验证了每个被拒绝路径的逻辑：

输入	被哪个条件捕获
../secret.txt	traversal check（portableNormalized.startsWith('../')）
..\\secret.txt	traversal check（规范化后为 ../secret.txt）
reports\\..\\..\\secret.txt	traversal check（规范化后为 ../secret.txt）
C:\\tmp\\report.html	盘符正则
C:/tmp/report.html	盘符正则
C:tmp\\report.html	盘符正则
\\\\server\\share\\report.html	path.win32.isAbsolute + path.posix.isAbsolute（规范化后）
\\tmp\\report.html	path.win32.isAbsolute + path.posix.isAbsolute（规范化后）

安全路径（reports/summary.html、reports\\summary.html）正确通过。

一个小观察： path.win32.isAbsolute() 完全冗余。盘符路径已被 /^[A-Za-z]:/.test() 覆盖（后者甚至更广——还能捕获 path.win32.isAbsolute 不认为是绝对路径的 C:report.html 等盘符相对路径）。UNC 和反斜杠绝对路径被 path.posix.isAbsolute(portableNormalized) 捕获。非 blocker——冗余无害，甚至可能使意图更清晰——但删除它可将条件简化为两个检查。

无关键 blocker。无 AGENTS.md 违规。代码遵循项目规范。

测试结果

非 UI 校验变更——tmux 场景测试不适用。直接运行单元测试：

Before（main 分支，无 PR）： 13 个测试全部通过。旧测试仅覆盖 ../secret.txt（POSIX traversal）。

After（本 PR）： 15 个测试全部通过。PR 将旧的组合测试拆分为聚焦的 rejects workspace paths that escape the workspace（10 种 Windows/POSIX traversal 变体）和 accepts safe workspace-relative artifact paths（/ 和 \ 相对路径正例）。测试组织良好。

— Qwen Code · qwen3.7-max

[qwen-code-ci-bot]

Stepping back: this is a clean, narrow validation fix that does exactly what it says. The cross-platform gap in validateWorkspacePath is real (I confirmed ..\\secret.txt passes through on Linux), and the fix is the minimal correct response — use portable normalization plus cross-platform absolute-path checks. 14 production lines, 15 passing tests, no drive-by refactors, no scope creep.

The one observation from code review — path.win32.isAbsolute() is technically redundant given the drive-letter regex and the later path.posix.isAbsolute() check — is a cleanup opportunity, not a correctness concern. The redundancy is harmless and arguably makes the intent more explicit for future readers.

The problem isn't an observed-in-the-wild bug (no linked issue, no reproduction from actual usage), but it's not purely theoretical either — the validation semantics are demonstrably wrong on POSIX for Windows-style inputs. For a 14-line fix with no risk of regression, closing the gap is worth doing.

Ships it. ✅

中文说明

总结：这是一个干净、窄范围的校验修复，完全如其描述。validateWorkspacePath 的跨平台缺口是真实的（我确认了 ..\\secret.txt 在 Linux 上能通过），修复是最小正确响应——使用可移植规范化加跨平台绝对路径检查。14 行生产代码，15 个通过测试，无顺手重构，无范围蔓延。

代码审查中的一个观察——path.win32.isAbsolute() 在技术上冗余（盘符正则和后续的 path.posix.isAbsolute() 已覆盖）——是清理机会，非正确性问题。冗余无害，甚至可能让意图对未来读者更明确。

问题不是已在线上观测到的 bug（无关联 issue，无实际使用中的复现），但也不是纯理论性的——POSIX 上对 Windows 风格输入的校验语义可证明是错误的。对于一个 14 行的修复且无回归风险，补上这个缺口值得做。

通过 ✅

— Qwen Code · qwen3.7-max

[qwen-code-ci-bot]

LGTM, looks ready to ship. ✅

[qwen-code-ci-bot]

[Suggestion] The three-layer absolute-path check (path.isAbsolute + path.win32.isAbsolute + /^[A-Za-z]:/ regex) is non-obvious. Each layer catches what the others miss:

1. path.isAbsolute — native platform absolute (POSIX: /foo; Win32: C:\foo, \foo)
2. path.win32.isAbsolute — Windows-style absolute even on Linux (\tmp, \\UNC, C:\foo)
3. /^[A-Za-z]:/ — drive-relative paths like C:tmp\file (win32.isAbsolute returns false for these)

Without a comment, a future maintainer may remove the seemingly redundant win32.isAbsolute call or regex, silently reopening the path-traversal gap on some platform.

Suggested change

	}
	// Three layers, each catching what the others miss:
	// 1. path.isAbsolute — native platform absolute
	// 2. path.win32.isAbsolute — Windows-style absolute even on POSIX (\tmp, \\UNC, C:\foo)
	// 3. /^[A-Za-z]:/ — drive-relative paths like C:tmp\foo (win32.isAbsolute returns false)
	path.isAbsolute(trimmed) ||
	path.win32.isAbsolute(trimmed) ||
	/^[A-Za-z]:/.test(trimmed)

— qwen3.7-max via Qwen Code /review

[qwen-code-ci-bot]

[Suggestion] Backslash paths like reports\\summary.html pass validation and are stored verbatim in the artifact metadata. On POSIX, path.resolve() treats \ as a literal filename character — the artifact resolves to a single file named reports\summary.html in the workspace root, not summary.html inside a reports/ directory. Downstream code (e.g., sessionArtifacts.ts calling path.resolve(workspaceCwd, workspacePath)) will silently resolve to a nonexistent path.

Consider normalizing backslashes to forward slashes in the stored value so the artifact path resolves correctly cross-platform:

// In execute(), after validation:
workspacePath: trimmed.replace(/\\/g, '/'),

— qwen3.7-max via Qwen Code /review

[wenshao]

Reviewed — no blockers. Suggestion-level recommendations are in the Suggestion summary comment below.

[wenshao]

Suggestions — commit 1019aa62886a6c59d1b62553910b75e48824204d

File	Issue	Suggested fix
packages/core/src/tools/record-artifact.test.ts:98	Loop-based test (for (const workspacePath of [...]) collapses 10 inputs into one test entry. If one input fails, the error doesn't identify which workspacePath caused it. The codebase convention is it.each (35 uses across packages/core/src/tools/*.test.ts).	Convert to it.each([...])('rejects workspace path %j', (workspacePath) => { ... }) for per-case isolation and named output.
packages/core/src/tools/record-artifact.test.ts:95	Bare .. input (no trailing filename) is not tested. path.posix.normalize('..') returns '..', hitting the === '..' branch — distinct from startsWith('../'). That branch has zero direct test coverage.	Add '..' to the traversal rejection list.
packages/core/src/tools/record-artifact.ts:370	FULLWIDTH Unicode homoglyph bypass: ．．／secret.txt (U+FF0E U+FF0E U+FF0F) passes validation because replace(/\\/g, '/') only matches U+005C and path.posix.normalize doesn't do Unicode normalization. After NFKC, it becomes ../secret.txt. Pre-existing, but the PR's new backslash handling has the same gap for U+FF3C (FULLWIDTH REVERSE SOLIDUS).	Apply NFKC normalization before all other checks: const trimmed = value.normalize('NFKC').trim();

— glm-5.2 via Qwen Code /review