You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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
legacy
portable
> victim.txt
0 commands
0 commands
>> victim.txt
0 commands
0 commands
echo hi > out.txt
1
1
rm -rf x
1
1
Command substitutions are already handled — FOO=$(whoami) yields 1 — so this is specific to a redirect with no command word.
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.
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.
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
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.
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.
> fileis valid POSIX, truncates or creates the file, and parses to zero commands — so withpermission.shell "*": "deny"it still empties the file, with no prompt and no denial.Checked against
ShellParse.scandirectly, which is whatpreparecalls:> victim.txt>> victim.txtecho hi > out.txtrm -rf xCommand 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
resourceslist,evaluateInputinpackages/core/src/permission.tsreturnsallow, because neithereffects.includes("deny")noreffects.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 onceresourcesis 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 todenyunder a deny-all ruleset, and that an empty resource list evaluates toallow— 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 lintis clean, andpackages/coretypechecks.Checklist