Repository navigation
Conversation
|
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. |
|
The following comment was made by an LLM, it may be inaccurate: |
|
Note on the The body does contain so |
e889963 to
608df9c
Compare
|
Independent verification on current dev (2fa3363): confirmed at the scanner level. One linkage worth keeping in scope during review: for the compound case ( |
608df9c to
41500d2
Compare
Parker-Fawcett
left a comment
There was a problem hiding this comment.
Independently verified the core claim before finding this PR (I had opened #55883 with the same shell.ts change, now closed as a duplicate): on current dev, the legacy shell scan returns zero commands for inputs like a comment-only command, and the old length guard skipped permission.assert entirely, so a deny rule never fired. Your redirect cases are a sharper repro than mine since they are destructive.
Two observations from that exercise, offered as suggestions:
-
The tests here construct the AssertInput by hand, so they pin the service behavior but not the actual bug site: if someone later removes or re-guards the assert call in shell.ts, these tests stay green. A tool-level test through executeTool with the mocked permission layer (the assertions pattern already used in test/tool-shell.test.ts, e.g. the compound-commands test) that feeds an input scanning to zero commands and expects one assertion with the raw command would cover the wiring. I prototyped exactly that (comment-only command, legacy scanner, denyAction set to shell) and it fails on dev and passes with this fix.
-
Agree with save set to the raw command over omitting it: persisting an exact-string allow keeps "always" meaningful without minting a broad rule. That is strictly better than the empty-save variant I had, where "always" silently behaved as "once".
No objection to the approach; the empty-resources-evaluates-to-allow note is worth its own follow-up issue if one does not exist.
|
I hit the same fail-open from the PowerShell side (#54393: a bare test(`PowerShell ${portable ? "native" : "legacy"}: a bare -- command cannot skip the shell permission check`, () =>
withScanner(
portable,
(registry, directory) =>
Effect.gen(function* () {
const command = "git rev-parse HEAD --"
const marker = path.join(directory.active, "marker")
const asked = yield* runPermissionCommand(registry, command, marker, ["reject"])
expect(asked.requests).toMatchObject([
{
action: "shell",
resources: [command],
save: portable ? ["git rev-parse *"] : [command],
},
])
expect(Exit.isFailure(asked.exit)).toBe(true)
if (Exit.isFailure(asked.exit))
expect(
asked.exit.cause.reasons.some(
(reason) => Cause.isDieReason(reason) && reason.defect instanceof Permission.DeclinedError,
),
).toBe(true)
const agents = yield* Agent.Service
yield* agents.transform((editor) =>
editor.update(toolIdentity.agent, (agent) => {
agent.permissions = [{ action: "shell", resource: "*", effect: "deny" }]
}),
)
const denied = yield* runPermissionCommand(registry, command, marker, [])
expect(denied.requests).toEqual([])
expect(denied.exit).toMatchObject({
_tag: "Success",
value: { status: "error", error: { message: expect.stringContaining("Permission denied: shell") } },
})
expect(yield* Effect.promise(() => Bun.file(marker).exists())).toBe(false)
}),
pwsh ?? "pwsh",
))It slots into the PowerShell |
|
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 #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