`> /dev/null` and `2>&1` should be special-cased to not require permission
Describe the bug
It seems cd is a special case in that you have to approve it every time. This is probably fine, but the issue is that copilot CLI frequently wants to cd into the current directory before running other commands. So even if those commands are already approved, the fact that the cd is there means we still have to approve it.
Here is an example:
╭──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮
│ Check desktop apps files: │
│ │
│ ╭──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮ │
│ │ cd /current/directory && for file in sub/dir/fileA sub/dir/fileB │ │
│ │ sub/dir/fileC; do echo "=== $file ==="; grep -n "NEEDLE" "$file" 2>/dev/null | head -3; │ │
│ │ done │ │
│ ╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯ │
│ │
│ Do you want to run this command? │
│ │
│ ❯ 1. Yes │
│ 2. No, and tell Copilot what to do differently (Esc) │
│ │
│ Confirm with number keys or ↑↓ keys and Enter, Cancel with Esc │
╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯
And especially note that the approval is one-time.
This happens constantly with many commands so I don't think it has anything to do with the for loop.
Affected version
0.0.334
Steps to reproduce the behavior
No response
Expected behavior
No response
Additional context
No response
A better example of something even more innocuous
╭──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮
│ Check if this file exists: │
│ │
│ ╭──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮ │
│ │ cd /current/directory && ls -la path/to/file 2>&1 │ │
│ ╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯ │
│ │
│ Do you want to run this command? │
│ │
│ ❯ 1. Yes │
│ 2. No, and tell Copilot what to do differently (Esc) │
│ │
│ Confirm with number keys or ↑↓ keys and Enter, Cancel with Esc │
╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯
I suspect this isn't to do with cd at all, but really it's the redirection operators that are triggering the prompt. We prompt for confirmation if it looks like the script is trying to write a file using > destination syntax because that could otherwise bypass rules on which directories may be written to.
However we should special-case > /dev/null and 2>&1, as they aren't going to write to a file on disk.
Can confirm a simple cd $CWD && do_something_else does not trigger it, it's the output redirection:
Going to rename this issue to better reflect the change needed!
We have hit this. We have an invocation of Copilot CLI
copilot --add-dir /tmp/gh-aw/ --log-level all --log-dir /tmp/gh-aw/.copilot/logs/ --allow-tool 'github(download_workflow_run_artifact)' --allow-tool 'github(search_users)' --allow-tool safe_outputs --allow-tool shell --allow-tool web-fetch --allow-tool write --prompt "$COPILOT_CLI_INSTRUCTION" 2>&1 | tee /tmp/gh-aw/agent-stdio.log
During the execution we see this shell command
dotnet tool install --global dotnet-reportgenerator-globaltool 2>&1 || echo "ReportGenerator already installed"
resulting in
Permission denied and could not request permission from user
but then this very similar shell command
export PATH="$PATH:$HOME/.dotnet/tools" && dotnet tool list --global | grep reportgenerator || dotnet tool install --global dotnet-reportgenerator-globaltool
is accepted through the permissions system.
I have access to Copilot CLI source code at GitHub and I set VSCode Agent Mode analyzing this and it said this:
Looking through the onShell handler, I don't see hasWriteApproval being used directly in the shell permission logic! It's only used in the onWrite handler
This might be a bug in the logic. The onShell handler should probably check if write redirections are approved when hasWriteApproval is true.
The issue is that line 428 doesn't consider whether write operations are approved:
This would explain why:
- First command with
2>&1fails: hasWriteFileRedirection is true, but the logic doesn't check hasWriteApproval- Second command without redirections succeeds: hasWriteFileRedirection is false
The fix would be to modify the condition to also allow approval when write operations are explicitly approved via the --allow-tool write flag.
We've improved our permissions checking for redirection scenarios in 0.0.351!
I believe we're seeing a variant of this too - our code is running Copilot in the background with a -p prompt to accomplish some work.
I'm trying to lock down permissions as responsibly as possible, so we're being quite explicit. The relevant set of flags for this are:
copilot -- additional-mcp-config @<file> --deny-tool write --allow-tool UDB_Server --allow-tool "shell(grep)" --allow-tool "shell(find)" --allow-tool "shell(cat)" --allow-tool "shell(xargs)" --allow-tool "shell(ls)" --allow-tool "shell(head)" -p <prompt>"
What I'm seeing is that Copilot often attempts to run commands such as:
$ find DIRNAME -type f -name "*.c" -path "*/PATTERN/*" 2>/dev/null | head -20
And these seem to consistently fail with permissions issues. I've replicated this by directly asking Copilot to run the command and seen the permissions failure - then removed the 2>/dev/null and seen success.
This use of redirects seems to be common to all the permissions issues we're seen. I note that it's different to the > /dev/null and 2>&1 mentioned in the issue title.
@RyanHecht from the change logs on 0.0.351:
Shell redirections like > some_file.txt in paths you've already granted write permissions, > /dev/null, and 2>&1 (Fixes https://github.com/github/copilot-cli/issues/211)
I think the 2>/dev/null pattern should be treated in the same way here.