fix(command-mode): flag pipe-to-shell and output-redirect commands in the destructive-command confirm gate#428
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 55dc96d047
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // The optional path segment catches absolute or relative interpreter | ||
| // paths; the trailing word boundary avoids false positives like | ||
| // `... | shasum` or `... | shuf`. | ||
| if cmd.range(of: #"\|\s*(\S*/)?(sh|bash|zsh|dash|fish)\b"#, options: .regularExpression) != nil { |
There was a problem hiding this comment.
Handle zsh
|& pipe-to-shell forms
When the pipeline uses zsh's |& operator, this check does not match because it only accepts whitespace/path/shell immediately after |. TerminalService.execute runs commands with /bin/zsh -c, and the zsh docs define |& as a shorthand for piping stderr and stdout into the next command, so cat install.sh |& sh still executes the script while isDestructiveCommand returns false and skips the confirmation gate.
Useful? React with 👍 / 👎.
| // or `cat y >> /etc/hosts`). Excluding a leading `-`/`=` skips arrows | ||
| // like `->`/`=>`, and requiring a non-`&` target skips file-descriptor | ||
| // duplications like `2>&1`. | ||
| if cmd.range(of: #"(?<![-=])>>?\s*[^&\s]"#, options: .regularExpression) != nil { |
There was a problem hiding this comment.
Catch zsh
>& file output redirects
This regex treats every > followed by & as a harmless file-descriptor duplication, but in zsh >& word and >>& word redirect stdout and stderr to a file unless word is a descriptor number. Because Command Mode executes via zsh, commands such as echo x >& ~/.zshrc or build >>& build.log still write files without confirmation even though the new redirect gate is meant to catch output writes.
Useful? React with 👍 / 👎.
… the destructive-command confirm gate
55dc96d to
9ba3df4
Compare
What
CommandModeService.isDestructiveCommandpowers the "confirm before running" safety gate in Command Mode (commandModeConfirmBeforeExecute). It missed several destructive command shapes that would auto-run without a confirmation prompt:curl https://example.com/install.sh | sh(and| bash/| zsh/| dash/| fish, including absolute or relative interpreter paths like| /bin/bash). Previously only| rm/| sudo/| ddwere caught.tee:echo ... | tee ~/.zshrc/tee -awrites (or appends) to files.| sudo teewas already caught via thesudopattern; the non-sudo case was not.echo x > ~/.zshrc,cat y >> /etc/hosts. The previous"> "check only matched commands that started with>, so real redirects slipped through.How
Three pattern checks added to
isDestructiveCommand, and the function is nownonisolated staticso it can be unit-tested directly (its single call site is updated accordingly). The redirect regex is tuned to skip file-descriptor cases:2>&1and>&2(fd duplication) and->/=>arrows do not trip it.Tests
New
CommandModeDestructiveCommandTests(6 methods, ~50 assertions): the newly-caught classes, equivalent bypass forms (path-prefixed shells, spaceless /&>/2>>redirects), case-insensitivity, a regression guard over the existing detections, and safe-command non-triggers (2>&1,| ssh,| shuf,| /usr/bin/shasum, quoted arrows, input redirects). Fullxcodebuild testis green andswiftlint --strictis clean.Accepted limitations
The redirect heuristic is intentionally token-blind, so it over-flags two harmless shapes (an extra confirmation prompt, never a missed destructive command): a quoted
>comparison likeawk '$1 > 10' data.csv, and a redirect to a discard target likecmd > /dev/null. That is the right bias for a confirm-before-execute gate, and shell-quote parsing was deliberately avoided to keep the heuristic simple and avoid introducing false negatives.