fix(exec): support bare command patterns in approvals allowlist#61208
fix(exec): support bare command patterns in approvals allowlist#61208mvanhorn wants to merge 4 commits intoopenclaw:mainfrom
Conversation
Greptile SummaryThis PR fixes the silent-skip of bare executable name patterns (e.g. Confidence Score: 4/5Safe to merge for Linux/macOS users, but Windows users will still experience silent allowlist failures for bare patterns. One P1 remains: the bare-pattern matching against executableName doesn't account for the .exe extension Windows adds, so the bug is not fixed on that platform. Scoring 4 rather than 5 because this is a present defect on the changed code path for a security-adjacent feature (exec approvals). src/infra/exec-command-resolution.ts lines 355-371 (bare-pattern branch); src/infra/exec-allowlist-matching.test.ts (missing Windows/.exe coverage) Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/infra/exec-command-resolution.ts
Line: 358-371
Comment:
**Bare patterns still silently fail on Windows due to `.exe` extension mismatch**
The new branch matches `pattern` against `resolution.executableName`, which on Windows is `path.basename(resolvedPath)` — e.g. `python3.exe` — because Windows resolves `python3` → `C:\Python39\python3.exe`. Inside `matchesExecAllowlistPattern`, a non-wildcard pattern on win32 has `tryRealpath("python3")` return null (not an absolute path), so `normalizedPattern` stays `python3` while `normalizedTarget` stays `python3.exe`. The compiled regex `^python3$` does not match `python3.exe`, so the entry is silently skipped — the same class of bug this PR intended to fix. The PR description doesn't acknowledge this Windows caveat.
A targeted fix is to strip the `.exe` suffix from `executableName` before the comparison when on Windows:
```typescript
// On Windows, executableName includes the .exe extension that bare patterns lack
const effectiveExecName =
useArgPattern && resolution.executableName.toLowerCase().endsWith(".exe")
? resolution.executableName.slice(0, -4)
: resolution.executableName;
if (matchesExecAllowlistPattern(pattern, effectiveExecName)) {
```
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-allowlist-matching.test.ts
Line: 22-39
Comment:
**No test coverage for Windows `.exe` bare-pattern scenario**
The new test block only exercises Unix-style `executableName` values (e.g. `python3`, `rg`). There is no case where `executableName` carries a `.exe` suffix (the Windows norm), so the regression described above — bare pattern `python3` not matching `executableName = "python3.exe"` — would not be caught by CI on Linux runners.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(exec): support bare command patterns..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (matchesExecAllowlistPattern(pattern, resolution.executableName)) { | ||
| if (!useArgPattern) { | ||
| return entry; | ||
| } | ||
| if (!entry.argPattern) { | ||
| if (!pathOnlyMatch) { | ||
| pathOnlyMatch = entry; | ||
| } | ||
| continue; | ||
| } | ||
| if (argv && matchArgPattern(entry.argPattern, argv, platform)) { | ||
| return entry; | ||
| } | ||
| } |
There was a problem hiding this comment.
.exe extension mismatch
The new branch matches pattern against resolution.executableName, which on Windows is path.basename(resolvedPath) — e.g. python3.exe — because Windows resolves python3 → C:\Python39\python3.exe. Inside matchesExecAllowlistPattern, a non-wildcard pattern on win32 has tryRealpath("python3") return null (not an absolute path), so normalizedPattern stays python3 while normalizedTarget stays python3.exe. The compiled regex ^python3$ does not match python3.exe, so the entry is silently skipped — the same class of bug this PR intended to fix. The PR description doesn't acknowledge this Windows caveat.
A targeted fix is to strip the .exe suffix from executableName before the comparison when on Windows:
// On Windows, executableName includes the .exe extension that bare patterns lack
const effectiveExecName =
useArgPattern && resolution.executableName.toLowerCase().endsWith(".exe")
? resolution.executableName.slice(0, -4)
: resolution.executableName;
if (matchesExecAllowlistPattern(pattern, effectiveExecName)) {Prompt To Fix With AI
This is a comment left during a code review.
Path: src/infra/exec-command-resolution.ts
Line: 358-371
Comment:
**Bare patterns still silently fail on Windows due to `.exe` extension mismatch**
The new branch matches `pattern` against `resolution.executableName`, which on Windows is `path.basename(resolvedPath)` — e.g. `python3.exe` — because Windows resolves `python3` → `C:\Python39\python3.exe`. Inside `matchesExecAllowlistPattern`, a non-wildcard pattern on win32 has `tryRealpath("python3")` return null (not an absolute path), so `normalizedPattern` stays `python3` while `normalizedTarget` stays `python3.exe`. The compiled regex `^python3$` does not match `python3.exe`, so the entry is silently skipped — the same class of bug this PR intended to fix. The PR description doesn't acknowledge this Windows caveat.
A targeted fix is to strip the `.exe` suffix from `executableName` before the comparison when on Windows:
```typescript
// On Windows, executableName includes the .exe extension that bare patterns lack
const effectiveExecName =
useArgPattern && resolution.executableName.toLowerCase().endsWith(".exe")
? resolution.executableName.slice(0, -4)
: resolution.executableName;
if (matchesExecAllowlistPattern(pattern, effectiveExecName)) {
```
How can I resolve this? If you propose a fix, please make it concise.| it("matches bare executable name patterns without path separators", () => { | ||
| // Exact bare name match | ||
| expect(matchAllowlist([{ pattern: "rg" }], baseResolution)?.pattern).toBe("rg"); | ||
|
|
||
| // Bare name should be case-sensitive (no match for wrong case) | ||
| expect(matchAllowlist([{ pattern: "RG" }], baseResolution)?.pattern ?? null).toBe(null); | ||
|
|
||
| // Bare name matches regardless of resolved path | ||
| const pythonResolution = { | ||
| rawExecutable: "python3", | ||
| resolvedPath: "/usr/bin/python3", | ||
| executableName: "python3", | ||
| }; | ||
| expect(matchAllowlist([{ pattern: "python3" }], pythonResolution)?.pattern).toBe("python3"); | ||
|
|
||
| // Bare name does not match a different executable | ||
| expect(matchAllowlist([{ pattern: "node" }], pythonResolution)?.pattern ?? null).toBe(null); | ||
| }); |
There was a problem hiding this comment.
.exe bare-pattern scenario
The new test block only exercises Unix-style executableName values (e.g. python3, rg). There is no case where executableName carries a .exe suffix (the Windows norm), so the regression described above — bare pattern python3 not matching executableName = "python3.exe" — would not be caught by CI on Linux runners.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/infra/exec-allowlist-matching.test.ts
Line: 22-39
Comment:
**No test coverage for Windows `.exe` bare-pattern scenario**
The new test block only exercises Unix-style `executableName` values (e.g. `python3`, `rg`). There is no case where `executableName` carries a `.exe` suffix (the Windows norm), so the regression described above — bare pattern `python3` not matching `executableName = "python3.exe"` — would not be caught by CI on Linux runners.
How can I resolve this? If you propose a fix, please make it concise.![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: 9425a2f200
ℹ️ 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".
| if (!useArgPattern) { | ||
| return entry; |
There was a problem hiding this comment.
The new bare-pattern branch now returns early on non-Windows whenever matchesExecAllowlistPattern() succeeds, so an entry like { pattern: "*", argPattern: "..." } becomes an unconditional allow for every executable on Linux/macOS. Previously, the code intentionally only treated bare * as a wildcard when argPattern was absent (bareWild check), and * with argPattern did not match at all. This is a policy-widening regression for any approvals JSON that contains wildcard entries with argPattern (manual edits, imported files, or future tooling), and it can silently bypass expected exec gating.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c41f835898
ℹ️ 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".
| if (argv && matchArgPattern(entry.argPattern, argv, platform)) { | ||
| return entry; |
There was a problem hiding this comment.
In normal allowlist evaluation, resolveSegmentAllowlistMatch() calls matchAllowlist(..., effectiveArgv, ...), so argv is present for real command checks; with that input, a rule like { pattern: "*", argPattern: "..." } now matches any executable on Linux/macOS whenever the arg regex matches because * passes matchesExecAllowlistPattern() for every bareTarget and the branch then returns on argPattern alone. Fresh evidence compared with the prior comment is that this widened path is exercised in production via src/infra/exec-approvals-allowlist.ts:427-431.
Useful? React with 👍 / 👎.
| if (useArgPattern && bareTarget.toLowerCase().endsWith(".exe")) { | ||
| bareTarget = bareTarget.slice(0, -4); |
There was a problem hiding this comment.
This always strips .exe from the executable name before bare-pattern matching when Windows logic is enabled, which means explicit bare rules such as python3.exe or *.exe can never match (they are compared against python3). That silently breaks valid user-entered bare patterns on Windows and makes part of the new bare-pattern support unreliable.
Useful? React with 👍 / 👎.
mvanhorn
force-pushed
the
fix/61193-exec-approvals-bare-patterns
branch
from
c41f835 to
c9fe485
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c9fe485de4
ℹ️ 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".
| expect(matchAllowlist([{ pattern: "rg" }], baseResolution)?.pattern).toBe("rg"); | ||
|
|
||
| // Bare name should be case-sensitive (no match for wrong case) | ||
| expect(matchAllowlist([{ pattern: "RG" }], baseResolution)?.pattern ?? null).toBe(null); |
There was a problem hiding this comment.
The new assertion that "RG" should not match "rg" is only true on non-Windows platforms, but matchAllowlist() uses Windows-style case-insensitive matching when process.platform === "win32" (via matchesExecAllowlistPattern), so this test now fails in Windows CI (checks-windows-node-test) even though the runtime behavior is expected there. Guard this expectation by platform (or split per-platform expectations) so the suite remains deterministic across runners.
Useful? React with 👍 / 👎.
mvanhorn
force-pushed
the
fix/61193-exec-approvals-bare-patterns
branch
from
8e56b07 to
3fe336e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3fe336ec24
ℹ️ 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".
| if (useArgPattern && bareTarget.toLowerCase().endsWith(".exe")) { | ||
| bareTarget = bareTarget.slice(0, -4); | ||
| } | ||
| if (matchesExecAllowlistPattern(pattern, bareTarget)) { |
There was a problem hiding this comment.
This new bare-pattern branch uses the caller-supplied platform to enable Windows behavior (including .exe stripping), but the actual pattern check still goes through matchesExecAllowlistPattern, which derives case/path semantics from process.platform. In mixed-host setups (for example, a Linux gateway evaluating a Windows node command with platform: "win32"), bare rules can fail unexpectedly ("RG" won’t match "rg.exe") because matching stays host-case-sensitive. That makes bare-name approvals dependent on gateway OS rather than the target command platform.
Useful? React with 👍 / 👎.
mvanhorn
force-pushed
the
fix/61193-exec-approvals-bare-patterns
branch
from
3fe336e to
4198be4
Compare
…rrectness matchesExecAllowlistPattern now accepts an optional platform parameter so that a Linux host evaluating Windows-target commands uses case-insensitive matching as the target platform expects, rather than the host's process.platform.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7266a572f
ℹ️ 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".
| if (useArgPattern && bareTarget.toLowerCase().endsWith(".exe")) { | ||
| bareTarget = bareTarget.slice(0, -4); |
There was a problem hiding this comment.
The new bare-name logic only strips .exe before matching, so on Windows a rule like pattern: "npm" will not match common shim executables such as npm.cmd/npm.bat even though resolveExecutablePath() resolves commands through PATHEXT entries. This means the bare-pattern fix still misses many real Windows commands and users will continue getting unexpected approval prompts for those binaries.
Useful? React with 👍 / 👎.
…hing Use path.parse to strip extensions instead of hardcoded .exe check. Try matching both original and stripped executable names so explicit extension patterns (python3.exe) still work alongside bare patterns (python3). Gate extension handling on win32 platform, not useArgPattern.
🔒 Aisle Security AnalysisWe found 2 potential security issue(s) in this PR:
1. 🟠 Allowlist bypass via bare executable-name matching (PATH hijacking)
Description
This weakens the allowlist from "approved binary at an approved path" to "any binary with an approved name", enabling allowlist bypass in environments where an attacker can influence resolution (e.g., by controlling
Vulnerable code: const hasPath = pattern.includes("/") || pattern.includes("\\") || pattern.includes("~");
if (!hasPath) {
const bareTargets = [resolution.executableName];
// ...
if (bareTargets.some((target) =>
matchesExecAllowlistPattern(pattern, target, effectivePlatform))) {
return entry; // (non-Windows)
}
}Impact examples:
RecommendationRe-pin allowlist matching to a path identity rather than only a basename. Options (choose based on intended security model):
Example safer approach (require full path match): // Only allow path patterns; bare patterns do not authorize execution
if (!hasPath) continue;
if (!matchesExecAllowlistPattern(pattern, resolution.resolvedRealPath ?? resolvedPath, effectivePlatform)) {
continue;
}Also consider:
2. 🟠 Allowlist argPattern bypass via spoofed/invalid platform parameter in matchAllowlist()
Description
This becomes a security issue when Relevant behavior:
Vulnerable code: const effectivePlatform = (platform ?? process.platform) as NodeJS.Platform;
const normalizedPlatform = String(effectivePlatform).trim().toLowerCase();
const useArgPattern = normalizedPlatform.startsWith("win");
...
if (!useArgPattern) {
// Non-Windows: first path match wins (legacy behaviour).
return entry;
}And one concrete call site where platform: nodeInfo?.platform,(from Impact depends on deployment, but if allowlist entries rely on RecommendationFail closed and/or remove trust in caller-supplied platform for security decisions. Recommended options:
const raw = (platform ?? process.platform);
const normalized = String(raw).trim().toLowerCase();
const known = new Set(["win32","linux","darwin","aix","freebsd","openbsd","sunos","android","cygwin","netbsd"]);
const safePlatform = known.has(normalized) ? (normalized as NodeJS.Platform) : process.platform;
const useArgPattern = safePlatform === "win32";
if (entry.argPattern) {
if (!argv) continue; // or return null / fail closed
if (!matchArgPattern(entry.argPattern, argv, safePlatform)) continue;
return entry;
}
Analyzed PR: #61208 at commit Last updated on: 2026-04-09T14:06:11Z |
|
Addressed the bot feedback on bare-pattern matching:
Pushed in 74e8cb7. |
|
Closing - 17 days with no maintainer signal and base has drifted. The allowlist still silently skips patterns without path separators like |
1 participant
Summary
/,\,~). Patterns likepython3ornodeare accepted byopenclaw approvals allowlist addbut never matched during command evaluation.python3to match/usr/bin/python3, but the pattern is silently ignored. Commands that should be auto-approved still require manual approval.matchAllowlist()now matches it againstexecutableName(the basename) instead of skipping it entirely.resolvedPathas before. The bare*wildcard shortcut is unchanged.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Root Cause
matchAllowlist()insrc/infra/exec-command-resolution.ts:353-355has a guard that checkshasPathand skips any pattern without/,\, or~. Bare executable names likepython3fail this check and are silently dropped from evaluation. The fix routes bare patterns throughmatchesExecAllowlistPattern()withexecutableNameas the target instead ofresolvedPath, preserving argPattern semantics on Windows.This contribution was developed with AI assistance (Claude Code).