fix: enforce external directory checks for shell paths - #46904
Conversation
github-actions
Bot
added
the
needs:compliance
|
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: Related PR FoundPR #42986:
Note: The current PR (46904) explicitly mentions it replaces PR #46900 as a duplicate. The search tool is returning the current PR itself rather than the older #46900, which may indicate #46900 is already closed/merged or not indexed. |
github-actions
Bot
removed
the
needs:compliance
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
There was a problem hiding this comment.
🟡 Changes recommended
The new embedded-path scanner only detects POSIX /... literals, leaving a Windows bypass path for nested interpreter snippets (and related quoting semantics need a deliberate decision).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR hardens ShellTool permission scanning so external_directory protections apply consistently across more shell commands and syntax forms (e.g., unlisted commands, redirections, embedded interpreter snippets), closing a bypass where some commands could touch external paths without requesting external_directory.
Changes:
- Expand the set of path candidates to include non-option arguments, redirection targets, and absolute-path literals embedded inside command strings.
- Resolve paths through symlinks for the containment decision, while keeping user-facing permission patterns readable.
- Add regression tests for previously bypassing commands/syntax (e.g.,
head,ls, redirections, nested snippets) and symlink boundary behavior.
File summaries
| File | Description |
|---|---|
| packages/opencode/src/tool/shell.ts | Broadens path/redirect detection and adds symlink-aware boundary checks for external_directory enforcement. |
| packages/opencode/test/tool/shell.test.ts | Adds regression tests covering bypass cases and symlink-to-external boundary behavior. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| function embeddedPathArgs(text: string) { | ||
| const out: string[] = [] | ||
| // A shell command can hand an entire script to another interpreter (for | ||
| // example `python -c 'open("/etc/hosts")'`). Such paths are not separate | ||
| // shell arguments, so also inspect path literals embedded in arguments. | ||
| const absolute = /(?<![\w:>/])\/(?:[A-Za-z0-9._~@+%-]+\/)*[A-Za-z0-9._~@+%-]+/g | ||
| for (const match of text.matchAll(absolute)) { | ||
| // The AST-based redirect scan handles this case; avoid rediscovering its | ||
| // destination when the redirect operator is separated by whitespace. | ||
| if (text.slice(0, match.index).trimEnd().endsWith(">")) continue | ||
| out.push(match[0]) | ||
| } | ||
| return out | ||
| } |
| @@ -151,11 +150,16 @@ | |||
| if (name === "PSHOME") return path.dirname(shell) | |||
| } | |||
|
|
|||
| function variableValue(key: string, cwd: string, shell: string) { | |||
| return auto(key, cwd, shell) ?? envValue(key) ?? "" | |||
| } | |||
|
|
|||
| function expand(text: string, cwd: string, shell: string) { | |||
| const out = unquote(text) | |||
| .replace(/\$\{env:([^}]+)\}/gi, (_, key: string) => envValue(key) || "") | |||
| .replace(/\$env:([A-Za-z_][A-Za-z0-9_]*)/gi, (_, key: string) => envValue(key) || "") | |||
| .replace(/\$(HOME|PWD|PSHOME)(?=$|[\\/])/gi, (_, key: string) => auto(key, cwd, shell) || "") | |||
| .replace(/\$\{([A-Za-z_][A-Za-z0-9_]*)\}/g, (_, key: string) => variableValue(key, cwd, shell)) | |||
| .replace(/\$([A-Za-z_][A-Za-z0-9_]*)(?=$|[\\/])/g, (_, key: string) => variableValue(key, cwd, shell)) | |||
| return home(out) | |||
Issue for this PR
Closes #44728
Type of change
What does this PR do?
Shell permission checks only recognized a short list of commands, so commands such as head, ls, and shell redirections could access paths outside the project without requesting external_directory.
This change checks path arguments and redirection targets more generally, including paths inside nested shell/interpreter snippets and paths reached through symlinks. It keeps permission patterns readable while using canonical paths for the containment decision. Regression tests cover the bypass cases from this issue and related reports.
How did you verify your code works?
Screenshots / recordings
Not applicable; this is a shell permission and test-only change.
Checklist
This replaces PR #46900, which was closed as a duplicate.