fix(core): evaluate permissions for commands the scanner cannot decompose - #49954
Hotragn wants to merge 1 commit into
Conversation
Sorry, something went wrong.
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
Sorry, something went wrong.
|
The following comment was made by an LLM, it may be inaccurate: |
Sorry, something went wrong.
|
Note on the needs:issue label — I think it's a false positive on v2 PRs rather than a missing link. The body does contain Closes #49948, and running pr-standards.yml's own fallback against this exact body gives: section matched: true issueContent: "Closes #49948" hasBodyIssueRef: true so hasLinkedIssue should be true and removeLabel('needs:issue') should run. check-standards does report success, and the label still lands ~6s after the run starts. Same on #48968 and #48969, which are also v2-based — consistent with the closingIssuesReferences caveat already noted in the workflow, where the GraphQL count is 0 for non-default-branch PRs and only the body fallback can clear it. Happy to open a separate issue if that's useful. |
Sorry, something went wrong.
Issue for this PR
Closes #49948
Type of change
What does this PR do?
A command the shell scanner finds no commands in currently skips the permission check and runs unchecked:
A bare redirect is the clearest way in. > file is valid POSIX, truncates or creates the file, and parses to zero commands — so with permission.shell "*": "deny" it still empties the file, with no prompt and no denial.
Checked against ShellParse.scan directly, which is what prepare calls:
Command substitutions are already handled — FOO=$(whoami) yields 1 — so this is specific to a redirect with no command word.
The destructive part is easy to confirm:
This PR drops the guard and falls back to the raw invocation when the scan yields nothing, so a deny rule applies and an explicit allow can still permit it. Commands the scanner does decompose are unaffected — same resources, same save patterns as before.
Worth flagging why the guard is load-bearing: with an empty resources list, evaluateInput in packages/core/src/permission.ts returns allow, because neither effects.includes("deny") nor effects.includes("ask") is true for []. So the empty case doesn't fall through to "ask" — it falls through to "allow". I left that code alone here since nothing reaches it once resources is always populated, but it may be worth hardening separately.
How did you verify your code works?
Added four cases to packages/core/test/permission.test.ts (both redirect forms × both scanners) asserting that the scan really does yield zero commands, that the raw-invocation fallback evaluates to deny under a deny-all ruleset, and that an empty resource list evaluates to allow — the fail-open this guards against. That last assertion passes against the current implementation, so the behaviour is documented rather than assumed.
108/108 pass in that file, bun run lint is clean, and packages/core typechecks.
Checklist