fix(exec): implement Windows exec allowlist with argPattern and quote-aware analysis#56285
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb075e919a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // ` — PowerShell escape character; can form escape sequences like `n, `0 inside double quotes. | ||
| // Note: ! is intentionally omitted — PowerShell does not treat ! as special in double-quoted | ||
| // strings (unlike cmd.exe delayed expansion), so "Hello!" is safe to pass through. | ||
| const WINDOWS_UNSAFE_CMD_META = /[%`]|\$(?=[A-Za-z_{])/; |
There was a problem hiding this comment.
Reject
$( subexpressions in Windows arg escaping
windowsEscapeArg currently blocks %, backticks, and $ before identifiers/braces, but it still allows $(. In the new Windows allowlist path, arguments parsed from single-quoted input are rebuilt with double quotes, so a command like node tool.js 'safe$(Start-Process calc)' can pass analysis/matching and then execute the $() subexpression when the enforced command runs. This allows non-allowlisted commands to execute via argument expansion on Windows and undermines the allowlist boundary for any approved command that carries attacker-controlled arguments containing $(.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in commit 21309bdcf7: WINDOWS_UNSAFE_CMD_META lookahead extended from [A-Za-z_{] to [A-Za-z_{(] to also block $(expression) subexpressions. A test case covering $(whoami) and $(Get-Date) has been added.
Greptile SummaryThis PR implements Windows exec allowlist support by: (1) making Key issues found:
Confidence Score: 4/5Mostly safe to merge; one P1 security gap should be addressed before shipping to Windows users. The feature is well-designed and thoroughly tested. The argPattern mechanism correctly prevents injection in constrained allowlist entries. However, the quote-aware change to src/infra/exec-approvals-analysis.ts — specifically the WINDOWS_UNSAFE_CMD_META regex.
|
| Filename | Overview |
|---|---|
| src/infra/exec-approvals-analysis.ts | Largest changed file — adds quote-aware token scanning, single-quote tokenizer support, Windows shell wrapper stripping, windowsEscapeArg, and Windows buildSafeShellCommand. One P1 security gap: $(expression) is not caught by WINDOWS_UNSAFE_CMD_META but is now reachable inside double-quoted args. |
| src/infra/exec-approvals-allowlist.ts | Adds AllowAlwaysPattern type and argPattern field; changes resolveAllowAlwaysPatterns return type from string[] to AllowAlwaysPattern[]; adds buildArgPatternFromArgv / buildScriptArgPatternFromArgv helpers. Deduplication change from Set<string> to array with custom addAllowAlwaysPattern is correct. |
| src/infra/exec-command-resolution.ts | Adds matchArgPattern, stripTrailingRedirections, and updates matchAllowlist to accept argv and prefer argPattern-matched entries on Windows. The \x00-separator stripping in stripTrailingRedirections is a no-op for Windows (redirections are already blocked upstream), but is logically inconsistent. |
| src/infra/exec-approvals.ts | Adds argPattern to ExecAllowlistEntry type, threads it through addAllowlistEntry and recordAllowlistUse, and extends ExecApprovalDecision with "expired". Changes are clean and consistent. |
| src/agents/bash-tools.exec.ts | Adds buildExecToolDescription to surface pre-approved Windows commands as an LLM hint. The deriveExecShortName and argPatternToReadable helpers are display-only. Clean implementation with try/catch to avoid blocking tool creation. |
| src/agents/bash-tools.exec-host-gateway.ts | Correctly updated to destructure AllowAlwaysPattern objects and pass argPattern to addAllowlistEntry. |
| src/node-host/invoke-system-run.ts | Same mechanical update as bash-tools.exec-host-gateway.ts — threads argPattern through to addAllowlistEntry. Correct. |
Comments Outside Diff (1)
-
src/infra/exec-approvals-analysis.ts, line 53-66 (link)Duplicate JSDoc comment block on
deriveExecShortNameThere are two consecutive JSDoc blocks before
deriveExecShortName. The first block (lines 53–58) appears to be an earlier draft that was superseded by the second block (lines 60–66) but not removed. Only the second block should remain.Prompt To Fix With AI
This is a comment left during a code review. Path: src/infra/exec-approvals-analysis.ts Line: 53-66 Comment: **Duplicate JSDoc comment block on `deriveExecShortName`** There are two consecutive JSDoc blocks before `deriveExecShortName`. The first block (lines 53–58) appears to be an earlier draft that was superseded by the second block (lines 60–66) but not removed. Only the second block should remain. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: src/infra/exec-approvals-analysis.ts
Line: 709
Comment:
**PowerShell `$()` subexpression not blocked by `WINDOWS_UNSAFE_CMD_META`**
`WINDOWS_UNSAFE_CMD_META` correctly blocks `$var`, `${var}`, and `$env:SECRET` (via `\$(?=[A-Za-z_{])`), and backtick escape sequences. However, it does not block `$(expression)` — the PowerShell subexpression operator — because `$` followed by `(` is not matched by `[A-Za-z_{]`.
This is newly exploitable after the quote-aware changes in this PR. Before, `(` was in `WINDOWS_UNSUPPORTED_TOKENS` and blocked everywhere. Now `(` inside double-quoted arguments is permitted (the motivating fix for `"2026-03-28 (土) - LifeLog"`). As a result, a command like `node tool.js "$(Get-Item env:SECRET)"` now passes both `findWindowsUnsupportedToken` (the `(` is inside a double-quoted string and `$` is not in `WINDOWS_UNSUPPORTED_TOKENS`) and `windowsEscapeArg` (the lookahead `(?=[A-Za-z_{])` does not match `(`). PowerShell **does** expand `$(expression)` inside double-quoted strings, so the rebuilt command would evaluate the expression before passing the result to `node`.
This affects commands with path-only allowlist entries (no `argPattern`) and represents a new injection surface that didn't exist before this PR.
Suggestion: extend `WINDOWS_UNSAFE_CMD_META` to also catch `$(`:
```suggestion
const WINDOWS_UNSAFE_CMD_META = /[%`]|\$(?=[A-Za-z_{(])/;
```
The `(` in the lookahead catches `$(expression)` while still permitting bare `$` not followed by an identifier or `(` (e.g. UNC paths like `\\host\C$`), consistent with the existing test for that case.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/infra/exec-approvals-analysis.ts
Line: 53-66
Comment:
**Duplicate JSDoc comment block on `deriveExecShortName`**
There are two consecutive JSDoc blocks before `deriveExecShortName`. The first block (lines 53–58) appears to be an earlier draft that was superseded by the second block (lines 60–66) but not removed. Only the second block should remain.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/infra/exec-command-resolution.ts
Line: 266-295
Comment:
**`stripTrailingRedirections` is a no-op for `\x00`-joined arg strings**
`matchArgPattern` joins `argv` with either `\x00` (for patterns built by `buildArgPatternFromArgv`) or ` ` (for legacy space-separated patterns). When `\x00` is the separator, `TRAILING_SHELL_REDIRECTIONS_RE` uses `\s+` before the redirection operator, but `\x00` is not whitespace — so the regex never matches and the retry is silently a no-op for all auto-generated argPatterns.
In practice this is harmless: on Windows, `>` and `&` are in `WINDOWS_UNSUPPORTED_TOKENS` and are rejected before reaching `matchArgPattern`, so a trailing `2>&1` token would never appear in a valid Windows command. But the retry logic is misleading as written. Consider documenting that this branch only fires for legacy space-joined patterns, or updating the regex to also accept `\x00` as a separator before the redirection.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(exec): address remaining codex P2 is..." | Re-trigger Greptile
|
All findings addressed in commits Codex P1 — single-quote Windows quoting mismatch: Codex P2 — Greptile P1 — Greptile P2 — Greptile P2 Style — duplicate JSDoc on |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21309bdcf7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 888efd68b1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc70267216
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb30eb0c3c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af98b40d5d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c5827553fa
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 36f2d79f00
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
https://github.com/openclaw/openclaw/blob/4e5708d1a5fd7b826dbfe7e2f9c88cc850a04ed1/src/infra/exec-approvals-allowlist.ts#L425-L429
Skip value-taking PowerShell flags before script path extraction
resolveShellWrapperScriptCandidatePath is used for shell wrappers on Windows, but this loop only skips values for a POSIX-oriented subset of flags. For common commands like powershell -ExecutionPolicy Bypass -File script.ps1, -ExecutionPolicy is consumed but Bypass is then treated as the script token, so allow-always can persist/match /cwd/Bypass instead of script.ps1. That breaks allowlist stability for PowerShell script invocations and can bind approvals to the wrong path.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
Fixed in 94fa8e3 (review #4025536905). Added |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 94fa8e32d3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7acdd2e477
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 398671e6b1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed52e1cf5f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let idx = effectiveArgv.findIndex((a) => a === shellScriptCandidatePath); | ||
| if (idx === -1) { | ||
| idx = effectiveArgv.findIndex( | ||
| (a) => path.basename(a).toLowerCase() === scriptBase, | ||
| ); |
There was a problem hiding this comment.
Normalize relative script paths before basename fallback
Fresh evidence beyond the prior basename fix: shellScriptCandidatePath is resolved to an absolute path, so effectiveArgv.findIndex((a) => a === shellScriptCandidatePath) fails whenever -File was passed as a relative path. In that case the basename fallback can latch onto an earlier non-script argument with the same filename (for example -SettingsFile C:\tmp\deploy.ps1 -File .\scripts\deploy.ps1), which slices scriptArgs from the wrong point and makes allow-always argPattern matching depend on unrelated pre-script flags. This causes false allowlist misses/re-prompts for otherwise identical script invocations.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 15f2760. Added optional cwd to buildScriptArgPatternFromArgv and a resolveArgPath helper that normalizes relative argv tokens to absolute before comparing against the already-absolute scriptPath. The same normalization is applied in evaluateSegments using params.cwd. The basename fallback remains as a last resort.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 15f27602da
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
bacddf4 to
95b734b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 95b734b4ac
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 80189aabeb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e948952b8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e30a380b9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
ed70f9f to
13d6b66
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 13d6b664ff
ℹ️ 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".
| argPattern: Type.Optional(Type.String()), | ||
| lastUsedAt: Type.Optional(Type.Integer({ minimum: 0 })), | ||
| lastUsedCommand: Type.Optional(Type.String()), | ||
| lastResolvedPath: Type.Optional(Type.String()), |
There was a problem hiding this comment.
Accept allowlist source metadata in exec approvals schema
addAllowlistEntry now persists allow-always entries with source: "allow-always" (see src/infra/exec-approvals.ts), but this schema still rejects any extra allowlist keys via additionalProperties: false and does not permit source. That breaks exec.approvals.set / exec.approvals.node.set round-trips after an allow-always decision: exec.approvals.get can return entries containing source, then the same payload is rejected as invalid params when sent back. The protocol schema needs to include the persisted metadata (or redact it before transport) to keep approvals editing/sync working.
Useful? React with 👍 / 👎.
fadb65d to
30e2953
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 30e29535bf
ℹ️ 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".
| // Note: cmd /c is intentionally NOT stripped here. If a command is wrapped | ||
| // with `cmd /c`, its inner payload would later be executed by PowerShell, which | ||
| // changes semantics for cmd.exe builtins (dir, copy, etc.). Callers that submit | ||
| // `cmd /c <thing>` must have an explicit allowlist entry for `cmd` itself, or | ||
| // the command will require user approval. |
There was a problem hiding this comment.
Keep
cmd /c wrapper handling consistent with allow-always
On Windows, this branch intentionally leaves cmd /c wrappers unstripped during analysis/enforcement, but allow-always persistence still derives and stores the inner executable via shell-wrapper inline-command extraction. That mismatch means approving cmd /c "node app.js" as allow-always persists a node allowlist entry, while later evaluations still resolve the executable as cmd, so the exact same command misses the allowlist and prompts again (or is denied under allowlist-only). Please align these paths by either persisting cmd for this wrapper style or unwrapping cmd /c consistently in matching/enforcement.
Useful? React with 👍 / 👎.
Summary
buildSafeShellCommandvia PowerShell&call operator (fixes [Bug]: Regression of #25376 - Exec allowlist returns 'unsupported platform' on Windows (v2026.3.12) #50403)findWindowsUnsupportedTokenquote-aware: metacharacters inside double- or single-quoted args (e.g.(土)in"2026-03-28 (土) - LifeLog") no longer block allowlist checking entirelyargPatternfield toExecAllowlistEntryfor regex-based argument matching (Windows-only); uses\x00separator to preserve argv boundaries2>&1etc.) before argPattern matchingpowershell -ExecutionPolicy Bypass -Command "..."and'...'single-quoted payloads in wrapper strippingplugin-sdk-entrypoints.json: add missingsignal-coreentry (oversight ind5841f64—runtime-api.tsimports it but entrypoint was not registered, breakingvitestalias resolution)Root cause (fixes #50403)
buildSafeShellCommandreturned{ ok: false, reason: "unsupported platform" }on Windows, causing every exec to request approval even for allowlisted commands. Additionally,findWindowsUnsupportedTokenwas not quote-aware, so a command likenode add_lifelog.js "2026-03-28 (土) - LifeLog"was rejected before the allowlist was even checked — the(in the quoted title string triggered an early exit.Codex review
Ran
codex review --base origin/mainlocally and addressed all findings:!fromWINDOWS_UNSAFE_CMD_META(PowerShell doesn't treat!as special in double-quoted strings)buildArgPatternFromArgvreturnsundefinedon non-Windows)tokenizeWindowsSegmentto handle''as escaped apostrophe in single-quoted strings\x00arg-boundary separator inbuildArgPatternFromArgvto prevent argv-flattening bypass$not followed by identifier (\host\C$etc.) inwindowsEscapeArgKnown pre-existing failures in
main(not introduced by this PR)pnpm build: Telegram TypeScript errors (extensions/telegram/src/bot.ts)pnpm test:src/image-generation/runtime.test.ts(2 failures, unrelated to exec)Testing
pnpm testpasses (93/93 on changed files; 2 pre-existing failures inimage-generationare unrelated)(土)in title now runs without approval promptsAI-assisted PR checklist
codex review --base origin/mainrun locally; all findings addressed