fix(weixin): allow Windows image paths inside workspace#4465
Merged
Conversation
Contributor
📋 Review SummaryThis PR fixes Windows path handling in the Weixin channel's image path validation, ensuring that Windows-style paths (e.g., 🔍 General Feedback
🎯 Specific Feedback🟡 High
🟢 Medium
🔵 Low
✅ Highlights
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates Weixin’s [IMAGE: ...] path allowlist enforcement so that Windows-style paths are evaluated using Windows path semantics (rather than raw string-prefix checks), preventing valid workspace images from being incorrectly rejected on Windows.
Changes:
- Replace
startsWith()-based allowlist checks with apath.relative()-based containment check that switches betweenpath.win32andpath.posix. - Add unit tests covering Windows workspace acceptance, sibling-directory prefix rejection, and ensuring POSIX backslashes aren’t treated as separators.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/channels/weixin/src/send.ts | Adds Windows/POSIX-aware containment helper and uses it for image allowlist validation. |
| packages/channels/weixin/src/send.test.ts | Adds regression tests for Windows-style image/workspace paths and separator semantics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| return ( | ||
| relative === '' || | ||
| (!relative.startsWith('..') && !pathImpl.isAbsolute(relative)) |
Comment on lines
147
to
151
| ...workspaceDirs.map((d) => realpathSync(resolve(d)) + '/'), | ||
| ]; | ||
|
|
||
| if (!ALLOWED_DIRS.some((dir) => real.startsWith(dir))) { | ||
| if (!ALLOWED_DIRS.some((dir) => isInsideAllowedDir(real, dir))) { | ||
| throw new Error(`Image path outside allowed directories: ${real}`); |
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
requested changes
May 23, 2026
|
|
||
| return ( | ||
| relative === '' || | ||
| (!relative.startsWith('..') && !pathImpl.isAbsolute(relative)) |
Collaborator
There was a problem hiding this comment.
[Critical] The !pathImpl.isAbsolute(relative) guard is the sole defense against cross-drive escapes (e.g., workspace on C:, image on D:), but no test covers this scenario.
win32.relative('C:\\workspace', 'D:\\evil\\file.png') returns 'D:\\evil\\file.png' — an absolute path that does NOT start with ... If a future simplifier removes the isAbsolute check as "redundant" (it's unreachable on POSIX), cross-drive traversal silently becomes allowed with no test to catch the regression.
Suggest adding a test:
it('rejects Windows paths on a different drive letter', () => {
const imagePath = 'D:\\Photos\\img.png';
const workspaceDir = 'C:\\Project';
mockRealpathSync.mockImplementation((p: string) => {
if (p.includes('img.png')) return imagePath;
if (p.includes('Project')) return workspaceDir;
return p;
});
expect(() => validateImagePath(imagePath, [workspaceDir])).toThrow(
'Image path outside allowed directories',
);
});— qwen3.7-max via Qwen Code /review
| ); | ||
| }); | ||
|
|
||
| it('allows Windows paths inside the workspace directory', () => { |
Collaborator
There was a problem hiding this comment.
[Suggestion] The UNC branch in looksLikeWindowsPath (pathValue.startsWith('\\\\')) has no test coverage — all three Windows tests use drive-letter paths only (D:\\...).
Consider adding a test with a UNC path (e.g., \\\\server\\share\\file.png) as either the image path or workspace directory to verify this branch works correctly.
— qwen3.7-max via Qwen Code /review
| }); | ||
|
|
||
| it('rejects Windows paths in a sibling directory with the same prefix', () => { | ||
| const imagePath = 'D:\\WorkGroup\\QwenCode\\0022\\hello.png'; |
Collaborator
There was a problem hiding this comment.
[Suggestion] normalizeWindowsPath exists to convert / to \\, but all Windows test paths use only backslashes. Mixed-separator paths like D:\\WorkGroup/QwenCode\\002/hello.png are common in practice (Git Bash, MSYS2, WSL interop) and would actually exercise the normalization logic.
Consider adding a test variant with mixed separators:
it('allows Windows paths with mixed separators inside the workspace', () => {
const imagePath = 'D:\\WorkGroup/QwenCode\\002/hello.png';
const workspaceDir = 'D:\\WorkGroup\\QwenCode\\002';
// ...
});— qwen3.7-max via Qwen Code /review
wenshao
approved these changes
May 24, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
No issues found. LGTM! ✅ — qwen3.7-max via Qwen Code /review
Collaborator
|
Fix Windows image path validation in WeChat channel — properly handles win32/posix path separators for workspace containment checks. |
doudouOUC
added a commit
that referenced
this pull request
May 25, 2026
…4500) Pulls 5 main commits since #4469 (2026-05-24): - #4464 fix(weixin): send decryptable image payloads - #4465 fix(weixin): allow Windows image paths inside workspace - #4470 fix(cli): resolve stale closure race in text buffer submit handler - #4468 feat(skills): add memory-leak-debug skill for heap snapshot diagnosis - #4288 feat(cli): do not append trailing space for directory completions (#4092) 11 manual conflicts resolved + 2 add/add conflicts taken from main wholesale: Manual UU (12, all daemon-side preferred except text-buffer.ts): - packages/acp-bridge/package.json — kept HEAD's fuller description (F1 lift expanded the package surface; main has stale pre-F1 wording). - packages/cli/src/acp-integration/acpAgent.ts — kept HEAD's WorkspaceMcpBudget import (F2 needs it). - packages/cli/src/acp-integration/acpAgent.worktree.test.ts (AA): kept HEAD's superset of mocks (MCP_BUDGET_WARN_FRACTION, getMCPDiscoveryState, MCPServerStatus, McpTransportPool, WorkspaceMcpBudget, workspace/debug/mcp config mocks). HEAD already includes main-side SessionStartSource + SessionEndReason mocks. - packages/cli/src/ui/commands/directoryCommand.tsx — pure formatting (HEAD wrapped vs main inline). Kept HEAD. - packages/cli/src/ui/commands/directoryCommand.test.tsx — pure formatting. Kept HEAD. - packages/cli/src/ui/commands/skillsCommand.ts — pure formatting. Kept HEAD. - packages/cli/src/ui/hooks/useCommandCompletion.tsx — pure formatting. Kept HEAD. - packages/cli/src/ui/hooks/useCommandCompletion.test.ts — pure formatting. Kept HEAD. - packages/cli/src/ui/hooks/useSlashCompletion.test.ts — pure formatting. Kept HEAD. - packages/core/src/config/config.test.ts — kept HEAD's TrustGateError import (daemon-added). text-buffer.ts (4 zones — took MAIN wholesale for #4470's stale-closure fix): - Import: useRef instead of useReducer (daemon side had useReducer as a dead import — file uses dispatch via useCallback, not useReducer; verified via grep). useRef is needed for stateRef + #4470's currentText capture. - writeFileSync zone: use stateRef.current.lines.join('\n') instead of stale closure-captured `text`. Fixes #4470's bug. - text comparison: `newText !== currentText` not `newText !== text`. - dep array: `[dispatch, ...]` not `[text, ...]` (callback reads from ref now, doesn't need to re-bind on text change). AA (2, main wholesale via git checkout --theirs): - packages/core/src/permissions/dangerousRules.ts + .test.ts Original #4151 Auto-mode added these on main, came into daemon via #4469 squash. Main then landed #4371 ("strip additional dangerous interpreter rules") as a follow-up that daemon side never saw. Take main's evolved version wholesale. Verification: - packages/core tsc: 50 errors PRE-merge, 50 errors POST-merge (pre-existing baseline — none introduced by this sync). - packages/acp-bridge tsc: clean. - 5 spot-test runs on conflict-resolved files: 132 + 17 + 24 + 30 + 1 = 204 tests pass (text-buffer / directoryCommand / useCommandCompletion / useSlashCompletion / skillsCommand). Mirrors #4469's pattern (squash merge daemon_mode_b_main-side). Unblocks #4490 daemon_mode_b_main → main reverse integration merge (currently CONFLICTING precisely because of these 5 main commits).
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
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
[IMAGE: ...]files.D:\WorkGroup\QwenCode\002\hello.pngcould be rejected as outside the allowed workspace directory even when the file is inside that workspace.Validation
Commands run:
Prompts / inputs used:
D:\WorkGroup\QwenCode\002\hello.pnginsideD:\WorkGroup\QwenCode\002.Expected result:
Observed result:
Quickest reviewer verification path:
cd packages/channels/weixin npx vitest run src/send.test.ts src/media.test.tsEvidence:
src/send.test.tsnow includes regression coverage for Windows-style workspace image paths and boundary rejection.Scope / Risk
Main risk or tradeoff:
[IMAGE: ...]markers. The change usespath.win32.relative()/path.posix.relative()so path containment is checked with path semantics instead of raw string prefixes.Not covered / not validated:
Breaking changes / migration notes:
Testing Matrix
Testing matrix notes:
npx vitestandnpx tsc.Linked Issues / Bugs
Refs #4441