security(windows): enhance command argument validation#38846
security(windows): enhance command argument validation#38846JnyRoad wants to merge 2 commits intoopenclaw:mainfrom
Conversation
- Expand WINDOWS_UNSAFE_CMD_CHARS_RE with comprehensive character list - Add POWERSHELL_UNSAFE_PATTERNS_RE for PowerShell cmdlet blocking - Add validateWindowsArgument() with detailed error messages - Block null bytes and Unicode control characters - Export validateWindowsCommandArgument() for external use - Add security documentation and JSDoc comments Security: Defense-in-depth for Windows command execution. This prevents command injection attacks via crafted arguments. Reference: CVE-2024-27980, Microsoft cmd.exe parsing documentation Fixes: Bug openclaw#3 from security audit 2026-03-07
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73a73bbf30
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * * - Wildcard | ||
| * ? - Wildcard | ||
| */ | ||
| const WINDOWS_UNSAFE_CMD_CHARS_RE = /[&|<>\^%\r\n;`$(){}[\]=+'\\/*?!~]/; |
There was a problem hiding this comment.
The expanded WINDOWS_UNSAFE_CMD_CHARS_RE now rejects / and \\, so any Windows .cmd wrapper invocation fails for normal arguments that contain path separators, including scoped npm package specs (for example the packageName passed by buildNodeInstallCommand in src/agents/skills-install.ts, such as @openclaw/matrix). In the Windows pnpm/yarn path this throws inside escapeForCmdExe before spawning, which turns legitimate installs/commands into hard errors rather than just blocking injection payloads.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR expands Windows command-injection defences in Key changes:
Critical issues found:
Confidence Score: 1/5
Last reviewed commit: 73a73bb |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| * * - Wildcard | ||
| * ? - Wildcard | ||
| */ | ||
| const WINDOWS_UNSAFE_CMD_CHARS_RE = /[&|<>\^%\r\n;`$(){}[\]=+'\\/*?!~]/; |
There was a problem hiding this comment.
Backslash in blocked set breaks all .cmd/.bat execution on Windows
\\ (backslash) is now in WINDOWS_UNSAFE_CMD_CHARS_RE. On Windows, every absolute path uses backslashes — e.g. C:\Users\alice\project\build.bat. The call chain is:
buildCmdExeCommandLine(resolvedCommand, execArgs) // line 130
→ escapeForCmdExe(resolvedCommand) // line 120
→ validateWindowsArgument("C:\...\build.bat") // line 69
→ WINDOWS_UNSAFE_CMD_CHARS_RE.test(...) // line 70 → matches '\' → throws
This means every .cmd/.bat invocation will throw before execution. Backslash should be removed from the blocked set; the surrounding buildCmdExeCommandLine approach (passing the whole command line as a single /c argument with windowsVerbatimArguments: true) already prevents UNC injection at the structural level.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/process/exec.ts
Line: 45
Comment:
**Backslash in blocked set breaks all `.cmd`/`.bat` execution on Windows**
`\\` (backslash) is now in `WINDOWS_UNSAFE_CMD_CHARS_RE`. On Windows, every absolute path uses backslashes — e.g. `C:\Users\alice\project\build.bat`. The call chain is:
```
buildCmdExeCommandLine(resolvedCommand, execArgs) // line 130
→ escapeForCmdExe(resolvedCommand) // line 120
→ validateWindowsArgument("C:\...\build.bat") // line 69
→ WINDOWS_UNSAFE_CMD_CHARS_RE.test(...) // line 70 → matches '\' → throws
```
This means **every** `.cmd`/`.bat` invocation will throw before execution. Backslash should be removed from the blocked set; the surrounding `buildCmdExeCommandLine` approach (passing the whole command line as a single `/c` argument with `windowsVerbatimArguments: true`) already prevents UNC injection at the structural level.
How can I resolve this? If you propose a fix, please make it concise.| * * - Wildcard | ||
| * ? - Wildcard | ||
| */ | ||
| const WINDOWS_UNSAFE_CMD_CHARS_RE = /[&|<>\^%\r\n;`$(){}[\]=+'\\/*?!~]/; |
There was a problem hiding this comment.
/ and = blocking rejects valid command arguments
Both / and = are added to the blocked character set, but they appear in completely legitimate argument patterns:
/is the standard Windows switch prefix — arguments like/E,/I,/H(used byXCOPY) and/v(used by many utilities) would all be rejected.=is used in long-option style arguments:--output=file.txt, MSBuild-style/p:Configuration=Release, etc.
For cmd.exe, neither character is an injection vector on its own — injection requires &, |, <, >, ^, %, and newlines (the original character set). Removing / and = from the blocked set would restore legitimate use without weakening the actual injection defences.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/process/exec.ts
Line: 45
Comment:
**`/` and `=` blocking rejects valid command arguments**
Both `/` and `=` are added to the blocked character set, but they appear in completely legitimate argument patterns:
- `/` is the standard Windows switch prefix — arguments like `/E`, `/I`, `/H` (used by `XCOPY`) and `/v` (used by many utilities) would all be rejected.
- `=` is used in long-option style arguments: `--output=file.txt`, MSBuild-style `/p:Configuration=Release`, etc.
For cmd.exe, neither character is an injection vector on its own — injection requires `&`, `|`, `<`, `>`, `^`, `%`, and newlines (the original character set). Removing `/` and `=` from the blocked set would restore legitimate use without weakening the actual injection defences.
How can I resolve this? If you propose a fix, please make it concise.| throw new Error( | ||
| `Unsafe Windows cmd.exe argument detected: ${JSON.stringify(arg)}. ` + | ||
| "Pass an explicit shell-wrapper argv at the call site instead.", | ||
| `Dangerous character '${dangerousChar}' found. ` + | ||
| (context ? `Context: ${context}. ` : "") + | ||
| "Pass an explicit shell-wrapper argv at the call site instead." | ||
| ); |
There was a problem hiding this comment.
Sensitive argument values are echoed verbatim in error messages
JSON.stringify(arg) embeds the full argument string in every thrown Error (lines 74, 84, 92). If a caller passes an argument that contains credentials, tokens, or API keys, the secret will be visible in:
- Exception
.message(surfaced in catch blocks and stack traces) - Any error-logging pipeline that records the caught error
Consider truncating or redacting the value to avoid inadvertent secret exposure in logs. For example:
| throw new Error( | |
| `Unsafe Windows cmd.exe argument detected: ${JSON.stringify(arg)}. ` + | |
| "Pass an explicit shell-wrapper argv at the call site instead.", | |
| `Dangerous character '${dangerousChar}' found. ` + | |
| (context ? `Context: ${context}. ` : "") + | |
| "Pass an explicit shell-wrapper argv at the call site instead." | |
| ); | |
| if (WINDOWS_UNSAFE_CMD_CHARS_RE.test(arg)) { | |
| const charMatch = arg.match(WINDOWS_UNSAFE_CMD_CHARS_RE); | |
| const dangerousChar = charMatch ? charMatch[0] : "unknown"; | |
| throw new Error( | |
| `Unsafe Windows cmd.exe argument detected. ` + | |
| `Dangerous character '${dangerousChar}' found (length: ${arg.length}). ` + | |
| (context ? `Context: ${context}. ` : "") + | |
| "Pass an explicit shell-wrapper argv at the call site instead." | |
| ); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/process/exec.ts
Line: 73-78
Comment:
**Sensitive argument values are echoed verbatim in error messages**
`JSON.stringify(arg)` embeds the full argument string in every thrown `Error` (lines 74, 84, 92). If a caller passes an argument that contains credentials, tokens, or API keys, the secret will be visible in:
- Exception `.message` (surfaced in catch blocks and stack traces)
- Any error-logging pipeline that records the caught error
Consider truncating or redacting the value to avoid inadvertent secret exposure in logs. For example:
```suggestion
if (WINDOWS_UNSAFE_CMD_CHARS_RE.test(arg)) {
const charMatch = arg.match(WINDOWS_UNSAFE_CMD_CHARS_RE);
const dangerousChar = charMatch ? charMatch[0] : "unknown";
throw new Error(
`Unsafe Windows cmd.exe argument detected. ` +
`Dangerous character '${dangerousChar}' found (length: ${arg.length}). ` +
(context ? `Context: ${context}. ` : "") +
"Pass an explicit shell-wrapper argv at the call site instead."
);
}
```
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc1c169fad
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * * - Wildcard | ||
| * ? - Wildcard | ||
| */ | ||
| const WINDOWS_UNSAFE_CMD_CHARS_RE = /[&|<>\^%\r\n;`$(){}[\]=+'\\/*?!~]/; |
There was a problem hiding this comment.
The expanded blacklist now rejects = in every argument passed through the Windows .cmd wrapper, which breaks legitimate npm flags that this repo always emits (for example --loglevel=error in src/infra/update-global.ts). In Bun-based Windows runs, resolveNpmArgvForWindows explicitly falls back to npm.cmd, so these update/install commands now throw inside escapeForCmdExe before the process is even spawned, turning normal openclaw update/global-install flows into hard failures.
Useful? React with 👍 / 👎.
mdlmarkham
left a comment
mdlmarkham
left a comment
There was a problem hiding this comment.
Security Review: Windows Command Argument Validation
Verdict: Strong defense-in-depth improvement. A few potential bypass vectors to consider.
What's Covered ✅
The expanded blocklist is comprehensive:
- Command separators:
&,|,; - Redirection:
<,>,[,] - Variable expansion:
%,$,! - Shell metacharacters:
`,^,(,),{,} - PowerShell cmdlets:
Invoke-Expression,IEX,Invoke-Command, etc. - Null bytes and control characters
Good export of validateWindowsCommandArgument for pre-validation.
Potential Bypass Vectors to Consider
1. Base64 Encoded Commands (PowerShell)
powershell -EncodedCommand <base64>The POWERSHELL_UNSAFE_PATTERNS_RE blocks Invoke-Expression in the argument, but -EncodedCommand allows arbitrary base64-encoded commands to execute.
Mitigation: Consider blocking -EncodedCommand or -e, -enc variants.
2. Environment Variable Expansion via CMD
%COMSPEC% /c whoami%COMSPEC% expands to cmd.exe path. The % is blocked, but %PATH% and other env vars might slip through context-dependent expansion.
Current protection: % is in blocklist ✅
3. forfiles / Other Native Tools
forfiles /P C:\ /M notepad.exe /C "cmd /c whoami"Native Windows tools that execute commands. Not blocked by current patterns.
Assessment: These require the binary to exist and might be out of scope for argument validation. Document scope if intentional.
4. Path Traversal via \
The backslash \ is blocked, which prevents:
..\..\..\..\..\..\etc\passwd
But on Windows, \ is a path separator. Consider if this blocks legitimate paths like C:\Users\....
Recommendation: Allow \ when path is validated as absolute or within allowed directories. Currently blocks all \, which might break legitimate use cases.
Suggested Additions
-
Block
-EncodedCommandvariants:const ENCODED_COMMAND_RE = /\b-[Ee](?:ncodedCommand|nc|ncodedCommand)\b/;
-
Block environment variable expansion:
// Already covered by % in blocklist, but explicit: const ENV_VAR_RE = /\%[^%]+\%/;
-
Document scope: Clarify that this validates individual arguments, not command composition. Attackers could still chain multiple validated arguments if the call site builds a command line incorrectly.
Testing Recommendations
- Test base64-encoded PowerShell commands bypass
- Test legitimate paths with
\(should they pass?) - Test
forfilesand other native execution tools - Test Unicode normalization attacks (e.g.,
\u0026for&) - Test with real-world command injection payloads from OWASP
Overall: Good security hardening. Consider the bypass vectors above, especially -EncodedCommand. Approve with minor suggestions.
2 participants
Security: Defense-in-depth for Windows command execution. This prevents command injection attacks via crafted arguments.
Reference: CVE-2024-27980, Microsoft cmd.exe parsing documentation
Fixes: Bug #3 from security audit 2026-03-07
Summary
Describe the problem and fix in 2–5 bullets:
Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
User-visible / Behavior Changes
List user-visible changes (including defaults/config).
If none, write
None.Security Impact (required)
Yes/No)Yes/No)Yes/No)Yes/No)Yes/No)Yes, explain risk + mitigation:Repro + Verification
Environment
Steps
Expected
Actual
Evidence
Attach at least one:
Human Verification (required)
What you personally verified (not just CI), and how:
Compatibility / Migration
Yes/No)Yes/No)Yes/No)Failure Recovery (if this breaks)
Risks and Mitigations
List only real risks for this PR. Add/remove entries as needed. If none, write
None.