fix(command-mode): close three confirm-gate bypasses: absolute path, find -delete/-exec rm, diskutil #862
+159
−2
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -437,7 +437,7 @@ final class CommandModeService: ObservableObject { | |
| } | ||
|
|
||
| // Check if we need confirmation for destructive commands | ||
| if SettingsStore.shared.commandModeConfirmBeforeExecute, Self.isDestructiveCommand(tc.command) { | ||
| self.pendingCommand = PendingCommand( | ||
| id: tc.id, | ||
| command: tc.command, | ||
|
|
@@ -559,7 +559,7 @@ final class CommandModeService: ObservableObject { | |
| } | ||
| } | ||
|
|
||
| nonisolated static func isDestructiveCommand(_ command: String) -> Bool { | ||
| let cmd = command.lowercased() | ||
|
|
||
| // Commands that start with these are destructive | ||
|
|
@@ -598,6 +598,49 @@ final class CommandModeService: ObservableObject { | |
| return true | ||
| } | ||
|
|
||
| // The prefix list above only matches a bare command name. A model | ||
| // that reaches for `/bin/rm`, `/usr/bin/sudo`, etc. (not unusual — | ||
| // absolute paths are a normal way to disambiguate a binary) skips | ||
| // every check above except the `rm -` fallback, which only happens | ||
| // to catch `rm` and only when it carries a `-` flag. Resolve the | ||
| // leading token to its bare command name the same way a shell would | ||
| // (last path component) so `/bin/rm`, `/usr/bin/rm`, and bare `rm` | ||
| // are all recognized as the same command regardless of how the | ||
| // model referenced it. | ||
| let leadingToken = cmd | ||
| .drop(while: { $0 == " " || $0 == "\t" }) | ||
| .prefix(while: { $0 != " " && $0 != "\t" }) | ||
| let commandName = (leadingToken as NSString).lastPathComponent | ||
| let destructiveCommandNames: Set = [ | ||
| "rm", "rmdir", "mv", "sudo", "kill", "pkill", "killall", | ||
| "chmod", "chown", "chgrp", "dd", "mkfs", "shred", "truncate", | ||
| ] | ||
| if destructiveCommandNames.contains(commandName) { | ||
| return true | ||
| } | ||
|
|
||
| // `find -delete` / `find ... -exec rm ...` deletes without ever | ||
| // matching "rm -" or any `|`/`;`/`&&` pattern above, since `rm` | ||
| // inside `-exec` never sits next to a matched separator. | ||
| if commandName == "find", cmd.contains(" -delete") || (cmd.contains("-exec") && cmd.contains("rm ")) { | ||
| return true | ||
| } | ||
|
|
||
| // diskutil's erase/reformat/partition subcommands are as destructive | ||
| // as `dd`/`mkfs`/`format` but are a different binary entirely and | ||
| // weren't covered by any check above. Scoped to the destructive | ||
| // subcommands specifically so read-only uses (`diskutil list`, | ||
| // `diskutil info`) are not flagged. | ||
| if commandName == "diskutil" { | ||
| let destructiveDiskutilSubcommands = [ | ||
| "erasedisk", "erasevolume", "secureerase", | ||
| "reformat", "partitiondisk", "zerodisk", "unmountdisk", | ||
| ] | ||
| if destructiveDiskutilSubcommands.contains(where: { cmd.contains($0) }) { | ||
|
There was a problem hiding this comment.
Searching the entire command for each subcommand name also matches ordinary arguments, so a benign command such as Knowledge Base Used: AI Enhancement Pipeline Prompt To Fix With AIThis is a comment left during a code review.
Path: Sources/Fluid/Services/CommandModeService.swift
Line: 639
Comment:
**Diskutil arguments trigger false positives**
Searching the entire command for each subcommand name also matches ordinary arguments, so a benign command such as `diskutil info /Volumes/EraseDisk` is suspended behind manual confirmation. Parse and compare the actual diskutil subcommand token instead.
**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. |
||
| return true | ||
| } | ||
| } | ||
|
|
||
| return false | ||
| } | ||
|
|
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,110 @@ | ||
| @testable import FluidVoice_Debug | ||
| import Foundation | ||
| import XCTest | ||
|
|
||
| /// Covers three confirm-gate bypass classes in `CommandModeService.isDestructiveCommand`: | ||
| /// the primary command invoked via an absolute path, `find -delete`/`-exec rm`, and | ||
| /// `diskutil`'s destructive subcommands. None of the three require anything adversarial- | ||
| /// looking from the model -- an absolute path, `find`, and `diskutil` are all ordinary, | ||
| /// unremarkable tool choices. | ||
| final class CommandModeDestructiveCommandGapTests: XCTestCase { | ||
| // MARK: - Regression: existing bare-command detection still works | ||
|
|
||
| func testBareDestructiveCommandsAreStillCaught() { | ||
| let cases = [ | ||
| "rm -rf ~/Documents", | ||
| "sudo reboot", | ||
| "mv secret.txt /tmp/", | ||
| "chmod 000 /etc/hosts", | ||
| "killall Finder", | ||
| "rmdir ~/Documents", | ||
| ] | ||
| for command in cases { | ||
| XCTAssertTrue( | ||
| CommandModeService.isDestructiveCommand(command), | ||
| "expected \"\(command)\" to require confirmation" | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| // MARK: - Fix 1: absolute-path invocation | ||
|
|
||
| func testAbsolutePathInvocationIsCaught() { | ||
| let cases = [ | ||
| "/usr/bin/sudo reboot", | ||
| "/bin/mv secret.txt /tmp/", | ||
| "/bin/chmod 000 /etc/hosts", | ||
| "/usr/bin/killall Finder", | ||
| "/bin/rmdir ~/Documents", | ||
| "/bin/rm somefile.txt", // rm with no dash flag -- the "rm -" fallback doesn't apply here | ||
| "/bin/rm -rf ~/Documents", // still caught (now redundantly, by both the old fallback and the new check) | ||
| ] | ||
| for command in cases { | ||
| XCTAssertTrue( | ||
| CommandModeService.isDestructiveCommand(command), | ||
| "expected \"\(command)\" to require confirmation despite the absolute path" | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| // MARK: - Fix 2: find -delete / find -exec rm | ||
|
|
||
| func testFindDeleteAndExecRmAreCaught() { | ||
| let cases = [ | ||
| "find ~/Documents -delete", | ||
| "find ~/Documents -type f -delete", | ||
| "find / -name '*.important' -exec rm {} \\;", | ||
| ] | ||
| for command in cases { | ||
| XCTAssertTrue( | ||
| CommandModeService.isDestructiveCommand(command), | ||
| "expected \"\(command)\" to require confirmation" | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| // MARK: - Fix 3: diskutil destructive subcommands | ||
|
|
||
| func testDiskutilDestructiveSubcommandsAreCaught() { | ||
| let cases = [ | ||
| "diskutil eraseDisk JHFS+ Untitled disk0", | ||
| "diskutil secureErase 0 /dev/disk0", | ||
| "diskutil eraseVolume APFS Wiped /Volumes/Backup", | ||
| "diskutil reformat /dev/disk2s1", | ||
| "diskutil partitionDisk disk0 1 JHFS+ Untitled 100%", | ||
| "diskutil zeroDisk /dev/disk0", | ||
| ] | ||
| for command in cases { | ||
| XCTAssertTrue( | ||
| CommandModeService.isDestructiveCommand(command), | ||
| "expected \"\(command)\" to require confirmation" | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| // MARK: - No new false positives on benign commands | ||
|
|
||
| func testBenignCommandsAreNotFlagged() { | ||
| let cases = [ | ||
| "ls -la", | ||
| "git status", | ||
| "git commit -m \"fix bug\"", | ||
| "find . -name '*.txt'", // find WITHOUT -delete or -exec rm | ||
| "find . -type f -name '*.log' -exec cat {} \\;", // -exec, but not rm | ||
| "diskutil list", // read-only | ||
| "diskutil info disk0", // read-only | ||
| "diskutil activity", // read-only | ||
| "echo hello world", | ||
| "cat README.md", | ||
| "curl -s https://example.com", | ||
| "python3 script.py", | ||
| "/usr/bin/python3 --version", // absolute path but not a destructive command name | ||
| ] | ||
| for command in cases { | ||
| XCTAssertFalse( | ||
| CommandModeService.isDestructiveCommand(command), | ||
| "expected \"\(command)\" NOT to require confirmation" | ||
| ) | ||
| } | ||
| } | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When a destructive executable is quoted, such as
"/bin/rm" -rf ~/Documents, this parser retains the closing quote and derivesrm"instead ofrm. The classifier therefore returns false, while/bin/zsh -cresolves and executes/bin/rmwithout confirmation.Knowledge Base Used: AI Enhancement Pipeline
Prompt To Fix With AI