Skip to content

fix: strip env variable prefixes from permission pattern matching#31103

Closed
weiconghe wants to merge 1 commit into
anomalyco:devfrom
weiconghe:fix/14110-bash-permission-env-prefix
Closed

fix: strip env variable prefixes from permission pattern matching#31103
weiconghe wants to merge 1 commit into
anomalyco:devfrom
weiconghe:fix/14110-bash-permission-env-prefix

Conversation

@weiconghe

Copy link
Copy Markdown
Contributor

Issue for this PR

Closes #14110

Type of change

  • Bug fix
  • New feature
  • Refactor / code improvement
  • Documentation

What does this PR do?

The source() function in packages/opencode/src/tool/shell.ts returns the full command text for permission pattern matching. When a command has inline environment variable prefixes (e.g., GOFLAGS=-mod=vendor go test ./...), the tree-sitter AST includes variable_assignment nodes at the start of the command node. source() returned the full text including these nodes, so the permission pattern became GOFLAGS=-mod=vendor go test ./... instead of go test ./....

This meant permission rules like "go *": "allow" would not match, and users would get unnecessary permission prompts even though the command should be allowed.

Note: The parts() function (used for always auto-approval patterns) already correctly skips variable_assignment nodes. Only the patterns path via source() was affected.

Fix: Skip leading variable_assignment children in source() by finding the first non-variable_assignment child and slicing the text from its offset. This correctly handles:

  • Single env var: NODE_ENV=production echo helloecho hello
  • Multiple env vars: NODE_ENV=production DEBUG=1 go test ./...go test ./...
  • No env vars: go test ./...go test ./... (unchanged)
  • Redirected statements: NODE_ENV=production go test ./... > /dev/nullgo test ./... > /dev/null

How did you verify your code works?

Added two new test cases in packages/opencode/test/tool/shell.test.ts:

  1. Single env variable prefix: verifies NODE_ENV=production echo hello produces pattern echo hello
  2. Multiple env variable prefixes: verifies NODE_ENV=production DEBUG=1 go test ./... produces pattern go test ./...

Both tests verify that the always auto-approval pattern is also correct (e.g., echo *, go test *). Tests run for bash and cmd shells (PowerShell doesn't support this syntax).

Test results: 68 pass, 1 fail (pre-existing Windows path normalization test unrelated to this change).

Screenshots / recordings

N/A

Checklist

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

The `source()` function returned the full command text including leading
`variable_assignment` nodes (e.g., `GOFLAGS=-mod=vendor go test ./...`).
Permission patterns like `go *` could not match, causing unnecessary
permission prompts even when the rule should apply.

Skip leading `variable_assignment` children in `source()` so the pattern
becomes just the command portion (`go test ./...`), matching what `parts()`
already does for the `always` auto-approval patterns.

Closes anomalyco#14110

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

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

Potential Duplicate PRs Found:

  1. PR fix: strip inline env var prefixes from bash permission patterns #28475 - fix: strip inline env var prefixes from bash permission patterns

    • Appears to address the same issue with environment variable prefixes affecting shell permission pattern matching
  2. PR fix(shell): strip env variable assignments from permission patterns #30561 - fix(shell): strip env variable assignments from permission patterns

    • Directly related to stripping env variable assignments from permission patterns in the shell module

These PRs seem to be addressing the same or very similar issue. PR #31103 (the current PR) should be checked against these two to ensure it's not duplicating work already done or merged.

@weiconghe

Copy link
Copy Markdown
Contributor Author

Closing in favor of #30561 which has a more complete solution with security considerations for unsafe variable assignment values (command substitution, process substitution, etc.). Will contribute test cases to #30561 instead.

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.

Bash permission rules don't match commands with env variable prefixes

1 participant