fix(command-mode): flag pipe-to-shell, output-redirect, and whitespace-prefixed destructive commands in the confirm gate#434
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88480e3fab
ℹ️ 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".
| // 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.
For commands that use the zsh/bash >& file form to redirect output to a file, this heuristic skips the redirect because it rejects any > whose target starts with &. That avoids >&2, but it also lets real file overwrites such as echo x >& ~/.zshrc or cmd >&log bypass the confirm gate and run without prompting, which is the same destructive class this change is trying to catch.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88480e3fab
ℹ️ 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".
| // 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.
>&file redirects in the confirm gate
This check treats any > followed by & as a file-descriptor duplication, but TerminalService runs commands through /bin/zsh (Sources/Fluid/Services/TerminalService.swift:79), and the zsh redirection docs document >& word/&> word as redirecting stdout and stderr to a file when it is not one of the duplication syntaxes. As a result, a command like echo hi >&/tmp/target can create or truncate a file without the new confirmation gate, even though the equivalent &> /tmp/target is caught; distinguish >&1/>&- from >&path instead of excluding every ampersand target.
Useful? React with 👍 / 👎.
… the destructive-command confirm gate
… gate The redirect check skipped any > whose target starts with &, which correctly ignores fd duplications like 2>&1 but also let >&file and >>&file (redirect both streams to a file, creating or truncating it) bypass the gate. TerminalService runs commands through /bin/zsh, so echo x >&~/.zshrc truncates the file with no confirmation prompt. Add a check that flags a >&/>>& target that is not a digit or -, keeping >&1/>&2/>&- fd duplication and close safe. Covered by CommandModeDestructiveCommandTests.testFlagsRedirectBothStreamsToFile.
postoso
force-pushed
the
fix/command-mode-destructive-detection
branch
from
88480e3 to
7555724
Compare
|
Rebased onto current main (now mergeable) and addressed the Codex P1. The redirect check excluded any |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7555724350
ℹ️ 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.
|& pipes to shell
Because TerminalService executes commands with /bin/zsh, |& has to be treated like a pipe: the zsh manual defines |& as shorthand for 2>&1 | (https://zsh.sourceforge.io/Doc/Release/Shell-Grammar.html#Simple-Commands-_0026-Pipelines). This regex only accepts whitespace after |, so curl -fsSL https://example.com/install.sh |& sh still pipes into sh but bypasses the new confirmation gate; include optional & in the pipe-to-shell check.
Useful? React with 👍 / 👎.
| // with `&`, so this catches the file form while keeping `>&1`/`>&2` | ||
| // (file-descriptor duplication) and `>&-` (fd close) safe by requiring a | ||
| // target that is not a digit or `-`. | ||
| if cmd.range(of: #"(?<![-=])>>?&\s*[^0-9\s&-]"#, options: .regularExpression) != nil { |
There was a problem hiding this comment.
The zsh docs list >& word as a file redirect except when it matches the specific >& number/>& - forms (https://zsh.sourceforge.io/Doc/Release/Redirection.html), but this check skips every target whose first character is a digit or -. As a result, echo hi >& 2file or cmd >& -log creates/truncates a real file without prompting; only the all-digit fd target and exact close form should be excluded.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb2661af0b
ℹ️ 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".
| // `... | shasum` or `... | shuf`. The pipe operator check accepts zsh's | ||
| // `|&` but avoids matching the second pipe in logical OR (`||`). | ||
| if cmd.range( | ||
| of: #"(?<!\|)\|&?(?!\|)\s*(\S*/)?(sh|bash|zsh|dash|fish)\b"#, |
There was a problem hiding this comment.
For installer-style commands that use env to locate the interpreter, such as curl https://example.com/install.sh | /usr/bin/env bash, zsh still pipes the downloaded script into bash, but this new regex only recognizes a shell name as the first word after the pipe. That leaves this common pipe-to-shell form outside the confirmation gate and it will auto-execute; account for an optional env wrapper before applying the shell-name check.
Useful? React with 👍 / 👎.
postoso
force-pushed
the
fix/command-mode-destructive-detection
branch
from
fb2661a to
4e60032
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e60032891
ℹ️ 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".
| } else if character == ";" || character == "&" || character == "|" { | ||
| return String(command[startIndex..<index]) |
There was a problem hiding this comment.
When the command after a pipe starts with an fd redirection, e.g. printf 'echo hi\n' | 2>&1 bash, zsh still treats the redirection as part of the same simple command and runs bash with the piped stdin, but this scanner treats the & in 2>&1 as a command separator and truncates the target before bash. Because the later redirect checks intentionally exempt fd duplications, this pipe-to-shell form bypasses the confirmation gate and auto-executes the downloaded/scripted input.
Useful? React with 👍 / 👎.
postoso
force-pushed
the
fix/command-mode-destructive-detection
branch
from
4e60032 to
f77d5eb
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f77d5ebd28
ℹ️ 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".
| // or `cat y >> /etc/hosts`). Excluding a leading `-`/`=` skips arrows | ||
| // like `->`/`=>`, and requiring a non-`&` target skips file-descriptor | ||
| // duplications like `2>&1`. | ||
| if unquotedSyntax.range(of: #"(?<![-=])>>?\s*[^&\s]"#, options: .regularExpression) != nil { |
There was a problem hiding this comment.
When the redirect target is quoted, the masking step blanks out the entire filename before this regex runs, so simple destructive redirects such as echo x > "$HOME/.zshrc" or printf x >"/tmp/target" are not classified as destructive even though zsh still expands/applies the quoted target and truncates the file. Preserve a non-whitespace placeholder for quoted redirect operands (or parse redirects before masking them) so quoted paths still trip the confirm gate.
Useful? React with 👍 / 👎.
postoso
force-pushed
the
fix/command-mode-destructive-detection
branch
from
f77d5eb to
22009f8
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22009f821c
ℹ️ 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".
| guard commandName == "env" else { | ||
| return false |
There was a problem hiding this comment.
Because this resolver only continues when the first resolved word is env, a pipe target that wraps the shell in sudo or a zsh precommand modifier is treated as safe. TerminalService executes through /bin/zsh (Sources/Fluid/Services/TerminalService.swift:79), and zsh pipelines connect the left command to the next command's stdin while exec runs the following command in place (zsh Shell Grammar §6.1/§6.2), so forms like curl -fsSL ... | /usr/bin/sudo bash when sudo is non-interactive/cached, or printf 'echo hi\n' | exec bash, still feed a shell but bypass confirmation; peel these wrappers before returning false.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed this is a real gap, but wrapper resolution beyond env (sudo, doas, zsh precommand modifiers, and friends) is an open-ended family, and each one added here makes this PR harder to review. The change set already extends the confirm gate well past main: pipes to shells, tee, redirects, env wrappers, and quoted targets. I'd like to land this as-is and handle wrapper resolution properly in a follow-up. I've opened a discussion with a concrete design proposal for that: #541
1 participant
Description
CommandModeService.isDestructiveCommandbacks the Command Mode confirm-before-execute gate. This PR tightens that classifier so commands that can write files or run downloaded content through a shell require confirmation before execution.It now flags:
/bin/bash,zsh,fish,|&, and shell forms reached throughenvor/usr/bin/env, includingenv -Steewrites and appends>,>>,&>, zsh/bash>& file/>>& file, and quoted targets such as> "$HOME/.zshrc"It also masks quoted and escaped text before matching
teeand redirect syntax, and skips escaped pipe characters, so examples likeecho "| tee ~/.zshrc",echo \| sh,echo "x > y", and fd redirects such as2>&1do not trigger the gate.Type of Change
Related Issue or Discussion
Reopens #428. Related to #432, but chained-command splitting remains out of scope here.
Testing
swiftlint lint --strict --config .swiftlint.yml -- Sources/Fluid/Services/CommandModeService.swift Tests/FluidDictationIntegrationTests/CommandModeDestructiveCommandTests.swiftswiftformat --config .swiftformat Sourcesxcodebuild test -project Fluid.xcodeproj -scheme Fluid -destination 'platform=macOS' -only-testing:FluidDictationIntegrationTests/CommandModeDestructiveCommandTests CODE_SIGN_IDENTITY="" CODE_SIGNING_ALLOWED=NO CODE_SIGNING_REQUIRED=NOpassed: 11 tests, 0 failures.xcodebuild test -project Fluid.xcodeproj -scheme Fluid -destination 'platform=macOS' CODE_SIGN_IDENTITY="" CODE_SIGNING_ALLOWED=NO CODE_SIGNING_REQUIRED=NOpassed: 65 tests, 0 failures.Additional verification:
git diff --checkpassed.SelectedSpeechModel, restored it withdefaults write com.FluidApp.app SelectedSpeechModel parakeet-tdt-v2and verifieddefaults read com.FluidApp.app SelectedSpeechModelreturnedparakeet-tdt-v2.Screenshots / Video
Notes
This is intentionally a bounded command-shape classifier, not a full shell parser. It covers the command-mode confirm gate cases above while leaving broader shell expansion, aliases,
$SHELL, and command substitution out of scope.