Repository navigation
fix(core): scan PowerShell commands the grammar fails to parse - #54448
saurav-shakya wants to merge 2 commits into
Conversation
tree-sitter-powershell returns an ERROR tree with no command nodes for commands containing a bare `--` (and for tab or CR separators), so the legacy scanner reported zero commands and the shell tool skipped its permission check entirely. When the parse has errors and yields no commands, fall back to the portable scanner, which either finds the commands or fails closed. Fixes anomalyco#54393
|
The following comment was made by an LLM, it may be inaccurate: |
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for tracking this down. This fixes the cases in #54393, but the bypass is still open whenever the command also includes a part the grammar parses.
I ran the issue's reproduction from source on the base and on this branch: a stand-in pwsh, the legacy scanner, and rules shell * deny plus a git status * allow. echo BYPASS-RAN -- is now denied, as it should be. But git status --short; echo MIXED-RAN -- still runs with no prompt or denial. Tree-sitter finds the git status command node, so commands.length > 0, the fallback is skipped, and only git status is checked against the rules. Calling the scanner directly gives the same result:
git status; Remove-Item victim --→ onlygit statusRemove-Item x --; Get-ChildItem→ onlyGet-ChildItem
Anyone who has a single allow rule can still run any command by adding it after an allowed one with a trailing --. See the inline comment.
The tests in packages/core (the shell-parse* files and tool-shell.test.ts) pass, and so does typecheck. A regression case covering a mixed command like the ones above would catch this.
The public API is unchanged.
| (tree) => Effect.sync(() => tree.delete()), | ||
| ) | ||
| // Grammar errors (e.g. a bare `--` under PowerShell) can hide every command node and skip permission checks. | ||
| if (error && result.commands.length === 0) return yield* scanPortable(command, shell, cwd) |
There was a problem hiding this comment.
This only falls back when the broken parse finds no commands. If an ERROR node sits next to a parsed command, the hidden part is never checked:
git status; Remove-Item victim --scans to justgit statusRemove-Item x --; Get-ChildItemscans to justGet-ChildItem
With a git status * allow under * deny, git status --short; echo MIXED-RAN -- ran end to end on this branch. Falling back to scanPortable whenever tree.rootNode.hasError is true would close this. scanPortable either finds every command or fails closed. You could limit it to PowerShell if the effect on bash is a concern.
A bare `--` could still hide a command when another command in the same line parsed (e.g. `git status; Remove-Item victim --`): legacy found one command, so the fallback was skipped and only `git status` was checked. For PowerShell parses with errors, compare against the portable scanner and use its result unless both agree on the commands and directories, which keeps saved prefixes stable when nothing is hidden. A tab-separated `git\tstatus; Write-Output done` was also dropping `git\tstatus`; the permission impact fixtures now expect legacy to check it like the portable scanner does.
|
Thanks, good catch. Fixed in fb3a88a: for PowerShell parses with errors, the legacy result is now compared with the portable scanner's, and the portable result is used unless both agree on the commands and directories, so a command hidden beside a parsed one is always checked. Added your three mixed cases as regression tests. This also fixed |
|
Automated PR Cleanup Thank you for contributing to opencode. Due to the high volume of PRs from users and AI agents, we periodically close older PRs using automated criteria so maintainers can focus review time on the most active and community-supported contributions. This PR was closed because it matched the following cleanup criteria:
PRs created within the last month are not affected by this cleanup. If you believe this PR was closed incorrectly, or if you are still actively working on it, please leave a comment explaining why it should be reopened. A maintainer can review and reopen it if appropriate. Thanks again for taking the time to contribute. |
Issue for this PR
Closes #54393
Type of change
What does this PR do?
tree-sitter-powershell can't parse a bare
--, so it returns anERRORroot with nocommandnodes. The legacy scanner then reports zero commands, and the shell tool skipspermission.assert, sogit rev-parse HEAD --runs undershell * deny. The same thing happens for tab and CR separators.In
scanLegacy, when the tree has errors and no commands were found, this PR falls back toscanPortable. That scanner either finds the commands (git grep -n x -- README.md→git grep *, so existing allow rules still match) or fails closed. Parses that still yield commands keep the legacy behavior.This is complementary to #49954, which makes
shell.tsassert on the raw command when the scanner returns nothing. This PR fixes the scanner so the real command prefix is matched.How did you verify your code works?
shell-parse.test.tsfor the four commands in the issue. They fail ondevand pass with this change.shell-parse-compatibility.test.tsthat recorded this as a known legacy gap (Get-ChildItem\rRemove-Item victim,git\tstatus,Set-Location a,b) now pass the parity check, so I moved them to the parity list.bun typecheckandbun run testinpackages/core: everything passes except twoflocktests that fail identically ondevwhen run as root.Screenshots / recordings
N/A, not a UI change.
Checklist
Added by the opencode bot (AI-generated).
Related: #49948, #52615