fix(exec): resolve Windows PowerShell cmdlet allowlist miss#70341
fix(exec): resolve Windows PowerShell cmdlet allowlist miss#70341adamdaum wants to merge 4 commits intoopenclaw:mainfrom
Conversation
…#52952) PowerShell cmdlets (Get-ChildItem, etc.) are shell builtins without filesystem paths. The allowlist system assumed every command maps to a file, causing bare cmdlets to fail with 'exec denied: allowlist miss' on Windows. Add a cmdlet fallback in the allowlist evaluation layer: when a bare command has no resolved path on Windows, try matching against the PowerShell executable path with a synthetic argv that includes the cmdlet name for argPattern verification. This keeps the resolution layer honest (no fake resolvedPath) while letting cmdlets match allowlist entries scoped to the PowerShell executable. Also extract resolvePowerShellPath/resolveShellFromPath to the infra layer (executable-path.ts) so the allowlist code can resolve the PS path without a circular agents->infra dependency. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Greptile SummaryThis PR fixes Windows PowerShell cmdlet exec-allowlist misses by adding a targeted fallback that, when a bare command has no resolved filesystem path on Windows, matches it against the PowerShell executable with a synthetic argv carrying the cmdlet name for Confidence Score: 5/5Safe to merge; all findings are minor P2 style/performance suggestions with no correctness impact. The logic is sound, the priority chain is correctly ordered, guards cover all edge cases, and the test plan is comprehensive. Remaining observations (uncached resolvePowerShellPath and hand-constructed argPattern strings in tests) are non-blocking. No files require special attention. Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/infra/exec-approvals-allowlist.ts
Line: 431
Comment:
**`resolvePowerShellPath` called without caching per segment**
`resolvePowerShellPath()` does multiple `fs.existsSync` probes on every call. In `resolveWindowsCmdletAllowlistFallback` it runs once per unresolved bare-name segment, and again in `collectAllowAlwaysPatterns`. For a multi-cmdlet pipeline on Windows this results in redundant filesystem I/O each time the allowlist is evaluated.
Consider memoising the result (a module-level `let cachedPsPath: string | undefined` is enough since the PowerShell install location won't change mid-process) or passing the resolved path in from the call site.
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-windows-cmdlet.test.ts
Line: 47-49
Comment:
**Hardcoded `buildArgPatternFromArgv` internals**
The test constructs the expected `argPattern` string manually (null-byte separator, trailing null, regex escaping) rather than calling `buildArgPatternFromArgv` on the same synthetic argv. If the pattern format ever changes this test will produce a confusing failure — it will appear to reject a valid allowlist entry rather than flagging a format change.
Consider building the expected pattern the same way production code does:
```ts
const syntheticArgv = [psPath, "Get-ChildItem", "-LiteralPath", "C:\\Users\\foo"];
const argPattern = buildArgPatternFromArgv(syntheticArgv, WINDOWS_PLATFORM);
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(exec): resolve Windows PowerShell cm..." | Re-trigger Greptile |
| if (!rawExecutable || !isBareName(rawExecutable) || params.resolution?.resolvedPath) { | ||
| return null; | ||
| } | ||
| const psPath = resolvePowerShellPath(); |
There was a problem hiding this comment.
resolvePowerShellPath called without caching per segment
resolvePowerShellPath() does multiple fs.existsSync probes on every call. In resolveWindowsCmdletAllowlistFallback it runs once per unresolved bare-name segment, and again in collectAllowAlwaysPatterns. For a multi-cmdlet pipeline on Windows this results in redundant filesystem I/O each time the allowlist is evaluated.
Consider memoising the result (a module-level let cachedPsPath: string | undefined is enough since the PowerShell install location won't change mid-process) or passing the resolved path in from the call site.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/infra/exec-approvals-allowlist.ts
Line: 431
Comment:
**`resolvePowerShellPath` called without caching per segment**
`resolvePowerShellPath()` does multiple `fs.existsSync` probes on every call. In `resolveWindowsCmdletAllowlistFallback` it runs once per unresolved bare-name segment, and again in `collectAllowAlwaysPatterns`. For a multi-cmdlet pipeline on Windows this results in redundant filesystem I/O each time the allowlist is evaluated.
Consider memoising the result (a module-level `let cachedPsPath: string | undefined` is enough since the PowerShell install location won't change mid-process) or passing the resolved path in from the call site.
How can I resolve this? If you propose a fix, please make it concise.| { pattern: psPath, argPattern }, | ||
| ]); | ||
|
|
There was a problem hiding this comment.
buildArgPatternFromArgv internals
The test constructs the expected argPattern string manually (null-byte separator, trailing null, regex escaping) rather than calling buildArgPatternFromArgv on the same synthetic argv. If the pattern format ever changes this test will produce a confusing failure — it will appear to reject a valid allowlist entry rather than flagging a format change.
Consider building the expected pattern the same way production code does:
const syntheticArgv = [psPath, "Get-ChildItem", "-LiteralPath", "C:\\Users\\foo"];
const argPattern = buildArgPatternFromArgv(syntheticArgv, WINDOWS_PLATFORM);Prompt To Fix With AI
This is a comment left during a code review.
Path: src/infra/exec-approvals-windows-cmdlet.test.ts
Line: 47-49
Comment:
**Hardcoded `buildArgPatternFromArgv` internals**
The test constructs the expected `argPattern` string manually (null-byte separator, trailing null, regex escaping) rather than calling `buildArgPatternFromArgv` on the same synthetic argv. If the pattern format ever changes this test will produce a confusing failure — it will appear to reject a valid allowlist entry rather than flagging a format change.
Consider building the expected pattern the same way production code does:
```ts
const syntheticArgv = [psPath, "Get-ChildItem", "-LiteralPath", "C:\\Users\\foo"];
const argPattern = buildArgPatternFromArgv(syntheticArgv, WINDOWS_PLATFORM);
```
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: b0e879c994
ℹ️ 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".
| effectiveArgv, | ||
| inlineCommand, | ||
| match: executableMatch ?? shellPositionalArgvMatch ?? shellScriptMatch, | ||
| match: executableMatch ?? windowsCmdletMatch ?? shellPositionalArgvMatch ?? shellScriptMatch, |
There was a problem hiding this comment.
Preserve wrapper for allowlisted Windows cmdlets
This new fallback can mark bare PowerShell cmdlets as allowlisted, but downstream Windows node execution assumes an allowlisted shell payload is directly executable and swaps to segments[0].argv (src/node-host/invoke-system-run-allowlist.ts:124-131). In the cmdlet case (Get-ChildItem, Set-Location, etc.), that drops the shell wrapper and attempts to spawn the cmdlet token as a process, which fails (ENOENT) on reruns after allow-always/manual allowlist entries.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1dffcd4d93
ℹ️ 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".
| effectiveArgv, | ||
| inlineCommand, | ||
| match: executableMatch ?? shellPositionalArgvMatch ?? shellScriptMatch, | ||
| match: executableMatch ?? windowsCmdletMatch ?? shellPositionalArgvMatch ?? shellScriptMatch, |
There was a problem hiding this comment.
Keep shell wrapper for cmdlet allowlist matches
By adding windowsCmdletMatch into segment matching, bare PowerShell cmdlets can now satisfy allowlist on Windows, but execution still rewrites allowlisted shell payloads to segments[0].argv in src/node-host/invoke-system-run-allowlist.ts:121-131. In the cmdlet case that drops the PowerShell host and attempts to spawn Get-ChildItem (or similar) as a process, which fails with ENOENT on reruns after allow-always/manual allowlist entries. Fresh evidence from this commit is src/infra/exec-approvals-windows-cmdlet.test.ts, which now asserts bare cmdlets are allowlist-satisfied and therefore makes this execution path reachable.
Useful? React with 👍 / 👎.
|
Codex review: found issues before merge. What this changes: The PR adds a Windows-only PowerShell cmdlet fallback for exec allowlist matching and allow-always persistence, moves PowerShell path resolution into infra, and adds cmdlet allowlist tests. Maintainer follow-up before merge: This needs maintainer security/product review rather than an automated repair because it changes exec allowlist semantics and the right direction may be exact-command durable approvals instead of reusable PowerShell cmdlet fallback semantics. Security review: Security review needs attention: The diff changes exec allowlist trust semantics and currently lets the approved PowerShell target diverge from the argv that the node host executes. Review findings:
Review detailsBest possible solution: Revise the design so the allowlist decision and execution plan bind the same command: either preserve a PowerShell-hosted argv for cmdlet matches all the way through node-host execution, or avoid reusable cmdlet fallback semantics and rely on exact-command durable approvals. The final PR should include Windows node-host execution coverage and the required changelog entry. Do we have a high-confidence way to reproduce the issue? Yes. The source-level reproduction path is high confidence: the PR makes a bare cmdlet allowlist-satisfied via a synthetic PowerShell match, while current node-host execution rewrites allowlisted Windows shell payloads to the bare segment argv before spawning. Is this the best way to solve the issue? No. The PR updates allowlist evaluation and persistence, but not the execution plan, so the approved target can diverge from what the node host attempts to execute; carrying cmdlet-match metadata or an exact PowerShell argv into execution is the safer fix. Full review comments:
Overall correctness: patch is incorrect Security concerns:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 068b33de8717. |
Summary
Fixes #52952.
Get-ChildItem,Set-Location, etc.) are shell builtins without filesystem paths. The exec allowlist system assumed every command maps to a file, causing bare cmdlets on Windows to fail withexec denied: allowlist miss.resolvePowerShellPath/resolveShellFromPathfromsrc/agents/shell-utils.tstosrc/infra/executable-path.tsso the allowlist code can resolve the PS path without a circular agents→infra dependency.Design decisions
ExecutableResolution.resolvedPathremains undefined for cmdlets — no fake path. The fallback is confined to the allowlist evaluation layer.Test plan
powershell -Command "cmdlet"matches after wrapper stripping🤖 Generated with Claude Code