fix(exec): decode Windows command output with codepage-aware streaming#73751
fix(exec): decode Windows command output with codepage-aware streaming#73751vincentkoc wants to merge 2 commits intomainfrom
Conversation
🔒 Aisle Security AnalysisWe found 2 potential security issue(s) in this PR:
1. 🔵 Possible execution of attacker-controlled cmd.exe via UNC SystemRoot in Windows codepage detection
Description
However, If an attacker can influence the process environment (e.g., launching OpenClaw from a tainted environment), they can set Vulnerable code: const result = spawnSync(resolveTrustedWindowsCmdPath(), ["/d", "/s", "/c", "chcp"], ...);
...
if (!trimmed || trimmed.includes("\0") || !pathWin32.isAbsolute(trimmed)) {
return null;
}
...
// UNC like \\server\share\Windows passes these checks
pathWin32.basename(normalized).toLowerCase() === "windows" &&
pathWin32.dirname(normalized).toLowerCase() === parsed.root.toLowerCase();RecommendationHarden For example: function normalizeWindowsSystemRoot(value: unknown): string | null {
if (typeof value !== "string") return null;
const trimmed = value.trim();
if (!trimmed || trimmed.includes("\0")) return null;
const normalized = pathWin32.normalize(trimmed);
const parsed = pathWin32.parse(normalized);
// Require local drive root like C:\
if (!/^[A-Za-z]:\\$/.test(parsed.root)) return null;
if (
pathWin32.basename(normalized).toLowerCase() !== "windows" ||
pathWin32.dirname(normalized).toLowerCase() !== parsed.root.toLowerCase()
) {
return null;
}
return normalized;
}Alternatively, avoid shelling out to 2. 🔵 Possible execution of attacker-controlled cmd.exe via UNC SystemRoot in memory-host SDK Windows output decoder
DescriptionThe memory-host SDK’s Windows output decoder spawns
Vulnerable code: const result = spawnSync(resolveTrustedWindowsCmdPath(), ["/d", "/s", "/c", "chcp"], ...);The intent is to avoid PATH hijacking, but UNC acceptance can reintroduce binary planting risk. RecommendationReject UNC/device/extended-length roots when deriving an executable path from Example hardening: const parsed = pathWin32.parse(normalized);
if (!/^[A-Za-z]:\\$/.test(parsed.root)) return null; // reject UNC like \\server\share\Or remove the need to spawn Analyzed PR: #73751 at commit Last updated on: 2026-04-29T01:36:59Z |
|
Codex review: keeping this open for maintainer follow-up; there is still a little grit to resolve. Keep this PR open. Automated cleanup is blocked because the PR is member-authored and carries the protected Best possible solution: Keep this PR open for maintainer review. The best path is to either land a hardened version that covers local TUI and QMD decoding with portable tests, or have a maintainer explicitly narrow the scope to the already-merged #72393 core exec fix and close this PR manually. What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 7e5c3753f6f7. |
Greptile SummaryThis PR adds stateful, codepage-aware stream decoders to
Confidence Score: 3/5Production code is sound but the qmd-process test will fail on non-Windows CI, blocking clean merge. One P1 finding: the runCliCommand test in qmd-process.test.ts is broken on all non-Windows runners because resolveWindowsConsoleEncoding bypasses the spawnSyncMock via an early platform guard, making the GBK assertion impossible to pass. The runtime fix (stateful decoders) is correct; only the test infrastructure needs a decoder-factory injection seam like tui-local-shell already has. packages/memory-host-sdk/src/host/qmd-process.test.ts and packages/memory-host-sdk/src/host/qmd-process.ts (missing decoder-factory injection) Prompt To Fix All With AIThis is a comment left during a code review.
Path: packages/memory-host-sdk/src/host/qmd-process.test.ts
Line: 139-163
Comment:
**Test will always fail on non-Windows CI because `resolveWindowsConsoleEncoding` returns early**
`resolveWindowsConsoleEncoding()` in `windows-output-decoder.ts` guards behind `process.platform !== "win32"` and returns `null` before ever calling `spawnSync`. This means `spawnSyncMock.mockReturnValue({ stdout: "Active code page: 936", stderr: "" })` is never reached on Linux/macOS, `legacyDecoder` is set to `null`, and `decode()` falls through to `buffer.toString("utf8")`. The GBK bytes `[0xb2, 0xe2, 0xca, 0xd4]` produce replacement characters instead of `"测试"`, so the assertion fails on every non-Windows runner.
The sibling test in `tui-local-shell.test.ts` avoids this correctly by injecting `createOutputDecoder: () => createWindowsOutputDecoder({ platform: "win32", windowsEncoding: "gbk" })`. `runCliCommand` needs the same injection seam — add an optional decoder factory to its parameter type in `qmd-process.ts` and pass it into the decoders — or the test must pass explicit platform/encoding overrides before creating the decoders.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(exec): decode Windows command output..." | Re-trigger Greptile |
| it("decodes Windows codepage output across split qmd stdout and stderr chunks", async () => { | ||
| spawnSyncMock.mockReturnValue({ stdout: "Active code page: 936", stderr: "" }); | ||
| const child = createMockChild(); | ||
| spawnMock.mockImplementationOnce(() => { | ||
| queueMicrotask(() => { | ||
| child.stdout.emit("data", Buffer.from([0xb2])); | ||
| child.stdout.emit("data", Buffer.from([0xe2, 0xca])); | ||
| child.stdout.emit("data", Buffer.from([0xd4])); | ||
| child.stderr.emit("data", Buffer.from([0xa3])); | ||
| child.stderr.emit("data", Buffer.from([0xbb])); | ||
| child.emit("close", 0); | ||
| }); | ||
| return child; | ||
| }); | ||
|
|
||
| await expect( | ||
| runCliCommand({ | ||
| commandSummary: "qmd query", | ||
| spawnInvocation: { command: "qmd", argv: ["query"] }, | ||
| env: process.env, | ||
| cwd: tempDir, | ||
| maxOutputChars: 1_000, | ||
| }), | ||
| ).resolves.toEqual({ stdout: "测试", stderr: "；" }); | ||
| }); |
There was a problem hiding this comment.
resolveWindowsConsoleEncoding returns early
resolveWindowsConsoleEncoding() in windows-output-decoder.ts guards behind process.platform !== "win32" and returns null before ever calling spawnSync. This means spawnSyncMock.mockReturnValue({ stdout: "Active code page: 936", stderr: "" }) is never reached on Linux/macOS, legacyDecoder is set to null, and decode() falls through to buffer.toString("utf8"). The GBK bytes [0xb2, 0xe2, 0xca, 0xd4] produce replacement characters instead of "测试", so the assertion fails on every non-Windows runner.
The sibling test in tui-local-shell.test.ts avoids this correctly by injecting createOutputDecoder: () => createWindowsOutputDecoder({ platform: "win32", windowsEncoding: "gbk" }). runCliCommand needs the same injection seam — add an optional decoder factory to its parameter type in qmd-process.ts and pass it into the decoders — or the test must pass explicit platform/encoding overrides before creating the decoders.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/memory-host-sdk/src/host/qmd-process.test.ts
Line: 139-163
Comment:
**Test will always fail on non-Windows CI because `resolveWindowsConsoleEncoding` returns early**
`resolveWindowsConsoleEncoding()` in `windows-output-decoder.ts` guards behind `process.platform !== "win32"` and returns `null` before ever calling `spawnSync`. This means `spawnSyncMock.mockReturnValue({ stdout: "Active code page: 936", stderr: "" })` is never reached on Linux/macOS, `legacyDecoder` is set to `null`, and `decode()` falls through to `buffer.toString("utf8")`. The GBK bytes `[0xb2, 0xe2, 0xca, 0xd4]` produce replacement characters instead of `"测试"`, so the assertion fails on every non-Windows runner.
The sibling test in `tui-local-shell.test.ts` avoids this correctly by injecting `createOutputDecoder: () => createWindowsOutputDecoder({ platform: "win32", windowsEncoding: "gbk" })`. `runCliCommand` needs the same injection seam — add an optional decoder factory to its parameter type in `qmd-process.ts` and pass it into the decoders — or the test must pass explicit platform/encoding overrides before creating the decoders.
How can I resolve this? If you propose a fix, please make it concise.d7c85e0 to
5bcb170
Compare
Summary
Credit
This carries forward the useful approach from @slepybear's source PR #64661, which could not be safely updated directly because the branch is closed, maintainer_can_modify=false, checks failed, and bot review identified unresolved decoder-state issues.
Linked Issues
Fixes/addresses the Windows CJK exec mojibake family reported in #40613, #50519, and #56462.
Validation
Preflight Note
Hydrate the mentioned #72393 before opening a replacement PR. If #72393 is already a live maintainer-owned replacement with equivalent coverage and clean review gates, route this cluster to that PR instead of creating a duplicate.
ProjectClownfish replacement details: