fix(command-mode): close three confirm-gate bypasses: absolute path, find -delete/-exec rm, diskutil - #862
Conversation
…find -delete/-exec rm, diskutil isDestructiveCommand only matched bare command names, so an absolute-path invocation (/bin/mv, /usr/bin/sudo, /bin/chmod, etc.) skipped the confirm gate a bare-name equivalent would trigger. find -delete / find -exec rm and diskutil's erase/reformat subcommands were never covered at all. None of these three classes need anything adversarial-looking from the model -- an absolute path, find, or diskutil are all ordinary tool choices, so a plausible model output can land in any of them silently. Resolves the leading command to its bare name the same way a shell would (last path component of the first token) so any path prefix is recognized uniformly, plus dedicated checks for find and diskutil. Scoped narrowly: diskutil list/info (read-only) stay unflagged. Verified the gap survives PR altic-dev#434's independent hardening pass on the same function (fetched its branch, reran the same adversarial inputs against its actual patched code) before writing this, so this is a distinct gap, not overlapping work -- see altic-dev#861. Closes altic-dev#861 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Greptile SummaryThis PR replaces raw destructive-command substring checks with quote-aware shell tokenization and recursive classification to close previously reported confirmation bypasses.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
please fix the issues and I can check if I Can merge it. THanks! |
…ives
Two gaps in the confirm-gate classifier this PR already touches:
- A quoted executable ("/bin/rm" -rf ~) kept its closing quote through
lastPathComponent, so it resolved to rm" instead of rm and skipped
confirmation entirely. Strip a matching wrapping quote pair before
resolving the bare command name.
- diskutil's destructive-subcommand check searched the whole command
string, so an argument like /Volumes/EraseDisk tripped it on a plain
diskutil info call. Match the actual subcommand token instead.
Addresses the two open review comments on this PR.
|
fixed both review comments -- quoted absolute paths ("/bin/rm") now resolve correctly instead of keeping the trailing quote, and diskutil's subcommand check matches the actual subcommand token instead of searching the whole command string, so an argument like /Volumes/EraseDisk on a plain diskutil info call doesn't false-positive anymore. both covered by new tests, ran the full suite locally, everything green. |
![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: f59090ad4b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Only the first token after diskutil was read as the subcommand, so diskutil quiet eraseDisk JHFS+ Untitled disk0 classified quiet as the verb and let the actual erase through unconfirmed. quiet is a real diskutil modifier that can precede the verb -- skip it before reading the subcommand.
|
fixed the quiet modifier gap too -- was only reading the first token after diskutil as the subcommand, so quiet eraseDisk classified quiet as the verb and let the erase through. now skips quiet before reading the subcommand. new tests cover both the quiet+destructive-verb case and quiet+read-only-verb staying unflagged. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e42636644
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…ding token Every finding this PR has gotten -- quoted paths, quoted paths with internal spaces, the diskutil quiet modifier, format/mkfs.* basenames, and anything after &&/;/| -- was a different instance of the same root cause: the classifier only ever looked at the first space-delimited token of the whole string, with quoting and separators bolted on as special cases as they were found. Replaces that with two small passes: split the full command into its constituent simple commands on unquoted &&, ||, ;, &, |, and newline, then tokenize each one with real quote tracking so a quoted span stays one word regardless of internal whitespace. Every existing check (bare names, absolute paths, find -delete/-exec, diskutil + quiet, xargs) now runs against every simple command in a chain instead of only the leading token of the first one. Also generalizes the redirect check (was a whole-string prefix match on "> ") to any position in a simple command's own words, so a mid-command redirect like echo bad > /etc/hosts is caught the same as a command that opens with a bare redirect. Known limits, unchanged by this: this still can't see through an interpreter (python3 -c, osascript -e), shell variable/command substitution, or symlink indirection, and the destructive-name lists are still hand-maintained. Those are different problems from the ones this PR has been asked to fix.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e04361a550
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…ignments
Three gaps left in the tokenizer rewrite, all in how words are
interpreted after tokenization, not in the tokenizer itself:
- xargs's own destructive target could be path-qualified (xargs -I{}
/bin/rm {}), and the check compared it against the raw word instead
of its resolved basename. Now resolves each candidate the same way
the leading command already was.
- A bare redirect as the entire command (> important.txt) was missed
because the redirect scan started after the first word, assuming
the first word was always a command name. It can be the redirect
itself.
- A leading environment assignment (LC_ALL=C rm -rf victim) resolved
to the assignment as the command name and never looked past it.
Skips any number of leading NAME=value words before resolving the
actual program, the same way diskutil's quiet modifier is skipped.
Re-fuzzed the tokenizer standalone after these changes (degenerate
assignment shapes, huge repeated inputs, unicode) -- no crashes, no
slow paths. Full app build succeeds in Release configuration.
|
fixed all three from the latest round -- xargs target now resolves basename before comparing (was matching /bin/rm against bare rm and missing it), a bare redirect as the whole command (> file) is caught (redirect scan wasn't including the first word), and a leading env assignment (LC_ALL=C rm -rf x) now gets skipped before resolving the actual program instead of stopping at the assignment itself. also: the two older codex comments (format/mkfs prefix, compound command after &&) are already covered by the tokenizer rewrite a few commits back -- format/mkfs.* is in the basename check now and every simple command in a chain gets checked, not just the first. tests for both exist (testFormatAndMkfsVariantBasenamesAreCaught, testDestructiveCommandAfterSeparatorIsCaught) and pass. greptile auto-resolved its own version of these, codex's threads just didn't follow. re-fuzzed the tokenizer standalone after this round too -- degenerate assignment shapes, 40k+ char inputs, unicode, no crashes. full app build succeeds. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2da52a8308
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…s, parse shell payloads
Three gaps, and the first one is the interesting one:
- find -exec compared its target against the raw word, so
find . -exec /bin/rm {} \; missed. This is the same bug the previous
commit fixed for xargs three lines away -- fixed there, not here.
Rather than patch the second instance, every place a program can
appear as an argument instead of argv[0] now goes through one shared
basename-resolving helper: find -exec/-execdir, xargs, env, nohup,
command, exec, time, nice, setsid, stdbuf, timeout, watch.
- The tokenizer split on whitespace without processing backslash
escapes, so /tmp/tools\ dir/rm resolved to "tools\" instead of "rm".
Both the splitter and the tokenizer now honor escapes, which also
removes the find -exec ... \; caveat noted in the splitter's docs.
- sh -c 'rm -rf victim' resolved only the outer sh and treated the
payload as opaque. The old contains("rm -") catch-all caught this by
accident, so the rewrite regressed it. A shell payload is shell, so
it now goes back through the same parse, depth-capped at 4. This does
not extend to python3 -c and friends, whose payload is another
language entirely -- that stays out of scope.
|
@altic-dev fixed the latest three. worth calling out what the find -exec one actually was: it's the same bug as the xargs one from the previous round, three lines away, and i'd fixed one and not the other. so instead of patching the second instance i pulled it into one helper that every argument-position program reference now goes through -- find -exec/-execdir, xargs, env, nohup, command, exec, time, nice, setsid, stdbuf, timeout, watch. including ones nobody has flagged yet. also fixed backslash-escaped paths in the tokenizer (/tmp/tools\ dir/rm), and sh -c payloads now get parsed recursively instead of treated as an opaque argument -- that one was a real regression i introduced, the old contains("rm -") caught it by accident and my rewrite didn't. on the review loop generally: i've rewritten the PR description with an explicit "known limits" section, because i don't think these bots will ever return zero findings on a function like this and i'd rather the scope be stated than implied. non-shell interpreters (python3 -c, osascript -e), variable/command substitution (\ -rf), symlink indirection, and destructive commands not in the name list are all out of reach of string analysis and are documented as such. closing those properly means inverting the model to an allowlist, which is a product call rather than something to bolt onto this function. within that boundary this is strictly better than main -- everything it catches main missed, nothing previously caught was lost, 23 tests each verified fail-before/pass-after, tokenizer fuzzed separately, full release build green. happy to keep going if you want any of the out-of-scope items pulled in, but i'd rather you make that call than keep expanding scope on my own. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d8ec3c8c7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…ocation xargs sh -c 'rm -rf victim' passed the gate: the runner and find-exec branches only compared argument basenames, and the shell-payload recursion was wired only into leading-command position. This is the third round of the same mistake -- the previous commit fixed find -exec after the one before it fixed xargs, and this one would have fixed the runner branch and left find -exec again. So the recursion moved into containsDestructiveProgram, which is the single helper every argument-position program reference already goes through. A shell encountered at any argument index now classifies everything after it as a nested invocation, so xargs, find -exec, -execdir, env, nohup, timeout, and every other runner get it at once, in whatever combination, rather than one call site per review round. Depth is threaded through so nested payloads stay capped at the existing limit and cannot recurse without end.
|
|
||
| // `sh -c 'rm -rf victim'` -- the payload is a shell command, so run it back | ||
| // through the same parse instead of treating it as an opaque argument. | ||
| if shellInterpreterNames.contains(commandName), depth < maxShellRecursionDepth { |
There was a problem hiding this comment.
When a command contains five nested sh -c invocations with rm -rf victim in the innermost payload, depth < maxShellRecursionDepth stops inspecting the fifth shell, causing the unchanged destructive command to execute without confirmation.
How this was verified: At depth four the recursion guard stops inspecting shell payloads, while a false classification sends the unchanged command to terminal execution.
Knowledge Base Used: AI Enhancement Pipeline
Prompt To Fix With AI
This is a comment left during a code review.
Path: Sources/Fluid/Services/CommandModeService.swift
Line: 803
Comment:
**Recursion limit leaves payload unchecked**
When a command contains five nested `sh -c` invocations with `rm -rf victim` in the innermost payload, `depth < maxShellRecursionDepth` stops inspecting the fifth shell, causing the unchanged destructive command to execute without confirmation.
**How this was verified:** At depth four the recursion guard stops inspecting shell payloads, while a false classification sends the unchanged command to terminal execution.
**Knowledge Base Used:** [AI Enhancement Pipeline](https://app.greptile.com/altic/-/custom-context/knowledge-base/altic-dev/fluidvoice/-/docs/ai-enhancement.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e15aac2734
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
2 participants
Description
isDestructiveCommanddecides whether an AI-issued shell command in Command Mode needs user confirmation before running. It was doing ad hoc string matching on the raw command — a prefix list of bare names, a hardcoded list of separator substrings like"&& rm ", and acmd.contains("rm -")catch-all. That technique can't be made complete: every bypass found during review was a different instance of the same two root causes (only the first space-delimited token was ever examined, and quoting/escaping wasn't parsed at all).This replaces it with an actual parse. Same file, same function, same signature, one call site — nothing else in the app changes.
How it works now
&&,||,;,&,|, and newline, honoring quotes and backslash escapes.argv[0]to its basename and classify every simple command in the chain, not just the leading token of the first.Where a program can appear as an argument rather than as
argv[0], the same basename resolution applies — one shared helper used byfind -exec/-execdir,xargs,env,nohup,command,exec,time,nice,setsid,stdbuf,timeout, andwatch. Ash/bash/zsh -cpayload is parsed recursively (depth-capped) rather than treated as an opaque string.What this closes
Bare names; absolute and relative path invocation; quoted paths, including with internal spaces; backslash-escaped paths; destructive commands anywhere in a compound chain, not just first; leading environment assignments (
LC_ALL=C rm -rf x); redirects anywhere including as the entire command (> file); path-qualified programs handed to a runner (xargs -0 /bin/rm,find . -exec /bin/rm {} \;); nested shell payloads (sh -c 'rm -rf x');find -delete/-exec; anddiskutil's destructive subcommands including behind thequietmodifier, without false-positiving ondiskutil info /Volumes/EraseDisk.Known limits — deliberately out of scope
This is a blocklist over shell syntax, so it is bounded by what string analysis can see. It does not catch:
python3 -c "os.remove(...)",osascript -e,perl -e,node -e. The payload is a different language; reading it means statically analyzing arbitrary code. (Shell wrappers are handled because the payload is shell and this parser can read it.)X=rm; $X -rf ~,$(echo rm) -rf ~. Catching these requires evaluating shell semantics, not parsing.ln -s /bin/rm ~/t && ~/t -rf ~. Needs runtime filesystem resolution.defaults delete,launchctl unload,tccutil reset,csrutil,nvram. The name lists are hand-maintained and will always lag.Closing those requires inverting the model — confirm everything except a short allowlist of known-safe commands — which is a product decision rather than a fix to this function, and out of scope here. This PR is a strict improvement over
mainwithin the stated boundary: everything it catches,mainmissed, and nothing previously caught was lost.Testing
23 tests in
Tests/FluidDictationIntegrationTests/CommandModeDestructiveCommandGapTests.swift, covering every case above plus negative cases so benign commands don't start requiring confirmation. Each fix was verified fail-before/pass-after against the preceding commit.The tokenizer was additionally fuzzed standalone outside XCTest — empty input, unterminated quotes, lone trailing backslashes, 100k-character strings, 10k repeated operators, null bytes, unicode and emoji, tab-only whitespace — no crashes and no slow paths (worst case 21 ms).
swiftlintclean. Full app build succeeds in Release configuration. The widerFluidDictationIntegrationTeststarget passes (138 tests) once past a pre-existing launch crash inHotkeyShortcutTeststhat reproduces onmainand is unrelated to this change.Closes #861