Skip to content

fix(core): evaluate permissions for commands the scanner cannot decompose - #49954

Closed
Hotragn wants to merge 2 commits into
anomalyco:devfrom
Hotragn:shell-redirect-permission
Closed

Hotragn wants to merge 2 commits into
anomalyco:devfrom
Hotragn:shell-redirect-permission

Conversation

@Hotragn

@Hotragn Hotragn commented Sep 19, 2026

Copy link
Copy Markdown

Issue for this PR

Closes #49948

Type of change

  • Bug fix

What does this PR do?

A command the shell scanner finds no commands in currently skips the permission check and runs unchecked:

if (parsed.commands.length > 0)
  yield* permission.assert({ … })

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.

The destructive part is easy to confirm:

printf 'important data\n' > victim.txt   # 15 bytes
/bin/sh -c '> victim.txt'
wc -c < victim.txt                       # 0

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

  • I have tested my changes locally
  • I have not included unrelated changes in this PR

@github-actions

Copy link
Copy Markdown
Contributor

Thanks for your contribution!

This PR doesn't have a linked issue. All PRs must reference an existing issue.

Please:

  1. Open an issue describing the bug/feature (if one doesn't exist)
  2. Add Fixes #<number> or Closes #<number> to this PR description

See CONTRIBUTING.md for details.

@github-actions

Copy link
Copy Markdown
Contributor

The following comment was made by an LLM, it may be inaccurate:

@Hotragn

Hotragn commented Sep 19, 2026

Copy link
Copy Markdown
Author

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.

@gorredinesh21

Copy link
Copy Markdown

Independent verification on current dev (2fa3363): confirmed at the scanner level. ShellTool.collect walks commands(root) = tree.rootNode.descendantsOfType("command"), and a redirect-only statement produces no command node, so scan.patterns stays empty and ask() returns before any permission assertion (the scan.patterns.size === 0 early-return in packages/opencode/src/tool/shell.ts). Same counts as the table above on both the bash and PowerShell parsers.

One linkage worth keeping in scope during review: for the compound case (echo foo; > out.txt) patterns are non-empty via the echo command, so the residual gap there is redirect-text attachment on decomposed commands — that's the #52083 / #52180 side of the family. This PR plus that work would close the class end to end.

@Parker-Fawcett Parker-Fawcett left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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.

  2. 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.

@MateoEstradaOSU

Copy link
Copy Markdown

I hit the same fail-open from the PowerShell side (#54393: a bare -- in a pwsh command parses to zero command nodes) and have a tool-level PowerShell regression test for it that works against this fix unchanged — dropping it here in case it's useful. It exercises both scanners end-to-end through the shell tool: the ask path (request must carry the raw command) and a deny-all rule (must fail with "Permission denied: shell"; pre-fix the command runs with no request at all):

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 for (const portable of [false, true]) loop in packages/core/test/tool-shell.test.ts. Verified it fails on dev (no request fires) and passes with this PR's fallback; it needs pwsh on the runner for shell resolution, same guard the existing PowerShell tests there already use.

@github-actions

Copy link
Copy Markdown
Contributor

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:

  • The PR was created more than 1 month ago
  • The PR had fewer than 2 positive reactions
  • Positive reactions are counted as thumbs-up, heart, celebration, or rocket reactions on the PR

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

shell: bare redirect bypasses the permission check (scanner returns no commands)

4 participants