fix: enforce external directory checks for shell paths - #46900
Conversation
github-actions
Bot
added
the
needs:compliance
|
The following comment was made by an LLM, it may be inaccurate: I found one related PR: PR #42986: fix(shell): scan redirection targets for external_directory This PR is related as it addresses shell redirection scanning for the external_directory security boundary. However, it appears to be an earlier fix that your current PR (46900) may be building upon or superseding with more comprehensive checks (including non-option arguments, embedded absolute paths, environment expansions, and symlink resolution). The current PR appears to be a more complete enforcement of external directory checks that goes beyond just redirection scanning. |
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 path expansion and symlink-boundary directory reporting contain correctness/security issues that can mis-scope external_directory permission patterns, plus a Windows-specific symlink test reliability problem.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR tightens the external_directory security boundary for the shell tool by removing reliance on a command-name allowlist and instead extracting filesystem path candidates from broader shell syntax, with regression tests covering known bypasses (e.g. redirections, env expansions, embedded literals, and symlink traversal).
Changes:
- Expand path discovery beyond allowlisted commands to include non-option args, redirection targets, and embedded absolute-path literals.
- Resolve candidate paths through symlinks when determining whether a path crosses the workspace boundary.
- Add adversarial regression tests for previously reported bypass patterns (issues #44728, #40159, #42979).
File summaries
| File | Description |
|---|---|
| packages/opencode/src/tool/shell.ts | Broadens path extraction for external_directory enforcement and adds symlink-resolved boundary checks. |
| packages/opencode/test/tool/shell.test.ts | Adds regression coverage for unlisted-command bypasses, quoted-redirection false positives, and symlink boundary behavior. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| const boundary = resolved && resolveThroughSymlinks(resolved) | ||
| if (!resolved || !boundary || containsPath(boundary, instance)) continue | ||
| const dir = (yield* fs.isDir(resolved)) ? resolved : path.dirname(resolved) | ||
| scan.dirs.add(dir) |
There was a problem hiding this comment.
Fixed in bfd5fe3. Boundary checks still use the canonical path, but an in-workspace symlink escape now reports the canonical external directory in the permission pattern. Ordinary external paths keep their lexical display path for stable prompts. Added assertions for the external directory and metadata.
| 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(/\$\{([A-Za-z_][A-Za-z0-9_]*)\}/g, (_, key: string) => envValue(key) || "") | ||
| .replace(/\$([A-Za-z_][A-Za-z0-9_]*)(?=$|[\\/])/g, (_, key: string) => envValue(key) || "") | ||
| .replace(/\$(HOME|PWD|PSHOME)(?=$|[\\/])/gi, (_, key: string) => auto(key, cwd, shell) || "") | ||
| return home(out) |
There was a problem hiding this comment.
Fixed in bfd5fe3. Generic and expansion now uses the shell-independent auto() values for HOME, PWD, and PSHOME before falling back to the environment. Explicit : forms still read the environment.
There was a problem hiding this comment.
Clarification: generic $HOME, $PWD,
| const project = yield* tmpdirScoped() | ||
| const outside = yield* tmpdirScoped() | ||
| const linked = path.join(project, "linked") | ||
| yield* Effect.promise(() => symlink(outside, linked)) | ||
|
|
There was a problem hiding this comment.
Fixed in bfd5fe3. The regression test now creates a directory symlink on Unix and a junction on Windows.
|
Superseded by clean replacement PR #46904. |
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 requestingexternal_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?
bun test --timeout 30000 test/tool/shell.test.ts test/util/filesystem.test.ts— 87 passed, 0 failedbun run typecheck— passedbunx prettier --check packages/opencode/src/tool/shell.ts packages/opencode/test/tool/shell.test.ts— passedbun turbo typecheck— 30 successful, 0 failedScreenshots / recordings
Not applicable; this is a shell permission and test-only change.
Checklist
The earlier full package test run had one unrelated PTY timing failure; the affected shell and neighboring filesystem suites pass cleanly.