Repository navigation
fix(core): enforce external_directory for shell file paths and redirects - #56390
pulkit777exe wants to merge 2 commits into
Conversation
The external_directory permission was enforced for file tools (write, edit, read) but not for the shell tool when a command explicitly named an external path as an argument or redirect target. Root cause: ShellParse.scan only populated for CWD-style commands (cd, Set-Location, etc.). All other commands — cp, touch, rm, Set-Content — and redirect targets (echo x > /tmp/file) left empty, so access.authorizeExternal was only called on the workdir. Fix: - scan.ts: track redirect target words on Command as . Both the bash and PowerShell portable scanners accumulate redirect destination words as the token is finalized; the bash scanner also attaches them to the last command when flushing a compound statement. - parse.ts: add to Result. Both scanLegacy and scanPortable extract candidate external paths (absolute, home-relative, or traversal-using) from redirect targets and non-CWD command arguments via the existing directoryArgument/isCandidatePath helpers. A new helper commandParts() de-duplicates the portability mapping so the CWD and non-CWD branches share the same word-decoding logic in scanPortable. - shell.ts: resolve each path via FileAccess.resolve (no kind hint, so FileAccess maps files and not-yet-created files to their parent dir) and pass the resulting targets to access.authorizeExternal alongside the existing workdir and CWD-change directory targets.
|
The following comment was made by an LLM, it may be inaccurate: |
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix works for the reported case, but as written it will prompt too often to merge.
I ran the issue's reproduction from source with an external_directory ask rule. On the base, echo > /tmp/…, cp and touch ran with no permission request. With this PR, one external_directory request for /tmp/oc-56288-outside/* and /etc/* is raised before anything runs. shell-parse.test.ts passes (25 tests) and typecheck for packages/core is clean. I couldn't run the PowerShell case (no Windows), so it is covered only by the unit tests.
Public API: unchanged. Result.paths and Command.redirectTargets are new optional fields on internal types.
Concerns:
- False positives (blocking). Any argument or redirect target starting with
/is now treated as an external path.ls 2>/dev/nullrun inside the project raised anexternal_directoryrequest for/dev/*. Agents redirect to/dev/nullconstantly, so with anaskrule this would prompt on many commands, and under adenyrule it would block them. Quoted regex and pattern arguments also count:grep -rn '/api/v1' srcgives/api/v1, andawk '/foo/ {print}' a.txtgives/foo/ {print}, in both the legacy and portable scanners. See the inline comment. - Design direction. When #38822 was closed, a maintainer said #39567 deliberately kept the shell's
external_directorycheck limited to the working directory, even though the docs mention scanner-inferred directories. Checking argument paths changes that boundary, so a maintainer should confirm it is wanted before this is polished further. - Tests. The new tests only check that paths are found. Please add the cases that must not prompt (
2>/dev/null, a quoted regex argument,2>&1), so the false-positive fix is pinned down.
| }) | ||
| } | ||
|
|
||
| function isCandidatePath(text: string) { |
There was a problem hiding this comment.
This treats every word starting with / as a path, for every command and every redirect. In practice:
ls 2>/dev/nullraises anexternal_directoryrequest for/dev/*. I confirmed this end to end with anaskrule; underdenythe command would be blocked.grep -rn '/api/v1' srcgives/api/v1, andawk '/foo/ {print}' a.txtgives/foo/ {print}. The quoted pattern is decoded and then treated as a path, which leads to prompts for directories that don't exist.
At a minimum, ignore well-known device paths (/dev/null, /dev/stdout, /dev/stderr, /dev/fd/*, and $null/NUL on PowerShell). Also consider not treating quoted arguments of arbitrary commands as paths, or limiting argument scanning to commands known to take paths. Redirect targets are much more reliable than free-form arguments.
Issue for this PR
Closes #56288
Type of change
What does this PR do?
The
external_directorypermission was enforced for file tools (write,edit,read) but bypassed in theshelltool when a command explicitly referenced an external path via arguments or redirect targets.Root cause:
ShellParse.scanonly populateddirectoriesfor directory-changing commands (cd,pushd,Set-Location, etc.). Other file commands (cp,touch,Set-Content) and redirection targets (>,>>) did not populate candidate paths, soFileAccess.authorizeExternalonly received the working directory.Changes:
scan.ts: AddedredirectTargets?: string[]toCommandin both the bash and PowerShell portable scanners so redirection destinations are tracked.parse.ts: Addedpaths?: string[]toResult. Extracted candidate external paths (matching/,\,~, drive letters, or..) from redirect targets and non-CWD command arguments across both legacy (tree-sitter) and portable parsers. Shared argument decoding inscanPortableusingcommandParts().shell.ts: Resolved extractedpathsviaFileAccess.resolveand included them inFileAccess.authorizeExternalalongside the working directory and CWD targets.How did you verify your code works?
packages/core/test/shell-parse.test.tscovering:>,>>)cp,touch)Set-Content) and redirectsbun test test/shell-parse.test.ts(all 25 tests pass).bun run check(all typechecks and lint passed cleanly).Screenshots / recordings
N/A (backend / permission parsing logic)
Checklist
Added by the opencode bot (AI-generated).
Related: #46398, #49948, #52083, #49671