Infra: tighten exec allowlist glob matching#43798
Merged
vincentkoc merged 3 commits intomainfrom Mar 12, 2026
Merged
Conversation
Contributor
Greptile SummaryThis PR tightens the exec allowlist glob matching in Key observations:
Confidence Score: 5/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/infra/exec-allowlist-pattern.ts
Line: 58
Comment:
**Regex cache not keyed on platform**
The `globRegexCache` stores compiled regexes keyed only on the *normalized pattern string*. The regex flags (case-insensitive `"i"` vs. empty string) are baked in at compile time and depend on `process.platform`. This is fine for production (the platform is a process-level constant), but it means tests that mock `process.platform` mid-run and then call `compileGlobRegex` with a previously-cached pattern key would silently receive the wrong flags from the stale cache entry.
Consider keying the cache on both the pattern and the platform, or annotating this with a comment making the assumption explicit:
```suggestion
const compiled = new RegExp(regex, process.platform === "win32" ? "i" : "");
// NOTE: cache key is the normalized pattern string; this is safe because
// process.platform is constant for the lifetime of the process.
```
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-pattern.test.ts
Line: 5-14
Comment:
**Missing coverage for `**` glob and Windows case-insensitivity**
The new tests cover the two regressed scenarios well. A couple of additional cases would give higher confidence that the surrounding logic wasn't accidentally perturbed:
1. **`**` still crosses separators** — the `?` change touched adjacent `*`/`**` logic; a test like `matchesExecAllowlistPattern("/tmp/**/tool", "/tmp/a/b/tool")` should still return `true`.
2. **Windows case-insensitivity** — there is no `it.runIf(process.platform === "win32")` assertion verifying that the `"i"` flag path is exercised. Even a comment noting it is acceptable given the PR's stated scope, but the gap is worth tracking.
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: f5c1aaa |
![greptile-apps[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
GGzili
pushed a commit
to GGzili/moltbot
that referenced
this pull request
Mar 12, 2026
* Infra: tighten exec allowlist glob matching * Changelog: note GHSA-f8r2 exec allowlist fix
steipete
pushed a commit
to BruceMacD/openclaw
that referenced
this pull request
Mar 13, 2026
* Infra: tighten exec allowlist glob matching * Changelog: note GHSA-f8r2 exec allowlist fix
Interstellar-code
pushed a commit
to Interstellar-code/operator1
that referenced
this pull request
Mar 16, 2026
* Infra: tighten exec allowlist glob matching * Changelog: note GHSA-f8r2 exec allowlist fix (cherry picked from commit 82e3ac2)
Interstellar-code
pushed a commit
to Interstellar-code/operator1
that referenced
this pull request
Mar 16, 2026
* Infra: tighten exec allowlist glob matching * Changelog: note GHSA-f8r2 exec allowlist fix (cherry picked from commit 82e3ac2)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
matchesExecAllowlistPattern()lowercased POSIX targets and let?match/, so exact-looking allowlist entries could overmatch.security=allowlistand makes path patterns broader than reviewers expect.?to a single non-separator character.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
User-visible / Behavior Changes
Users on POSIX now get case-sensitive exec allowlist matching, and
?no longer crosses directory separators.Security Impact (required)
NoNoNoYesNoYes, explain risk + mitigation: exec allowlist matching is stricter, with regression coverage for the two overmatch cases.Repro + Verification
Environment
Steps
/tmp/Allowed-Toolagainst/tmp/allowed-toolon POSIX./tmp/a?bagainst/tmp/a/b.Expected
?only matches within a single path segment.Actual
true; after the patch both returnfalse.Evidence
Human Verification (required)
tsxprobe, then reranpnpm test -- src/infra/exec-allowlist-pattern.test.tsafter the fix.?matching within a segment still works.Review Conversations
Compatibility / Migration
YesNoNoFailure Recovery (if this breaks)
src/infra/exec-allowlist-pattern.tsRisks and Mitigations
?matches.