From b2f4e3a24bd51b175118b42c69f188f1791ad45a Mon Sep 17 00:00:00 2001 From: manjunathbhaskar Date: Fri, 14 Aug 2026 13:51:16 +0200 Subject: [PATCH 01/10] fix(command-mode): close three confirm-gate bypasses: absolute path, 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 #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 #861. Closes #861 Co-Authored-By: Claude Sonnet 5 --- Fluid.xcodeproj/project.pbxproj | 4 + .../Fluid/Services/CommandModeService.swift | 47 +++++++- ...ommandModeDestructiveCommandGapTests.swift | 110 ++++++++++++++++++ 3 files changed, 159 insertions(+), 2 deletions(-) create mode 100644 Tests/FluidDictationIntegrationTests/CommandModeDestructiveCommandGapTests.swift diff --git a/Fluid.xcodeproj/project.pbxproj b/Fluid.xcodeproj/project.pbxproj index 37167fada..9642ece30 100644 --- a/Fluid.xcodeproj/project.pbxproj +++ b/Fluid.xcodeproj/project.pbxproj @@ -13,6 +13,7 @@ 7C5AF14C2F15041600DE21B0 /* MediaRemoteAdapter in Embed Frameworks */ = {isa = PBXBuildFile; productRef = 7C5AF14A2F15041600DE21B0 /* MediaRemoteAdapter */; settings = {ATTRIBUTES = (CodeSignOnCopy, RemoveHeadersOnCopy, ); }; }; 7C9A71022F58B00000FB7CAF /* TranscribeCpp in Frameworks */ = {isa = PBXBuildFile; productRef = 7C9A71012F58B00000FB7CAF /* TranscribeCpp */; }; 7C91B0012F42AA0100C0DEF0 /* HotkeyShortcutTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7C91B0022F42AA0100C0DEF0 /* HotkeyShortcutTests.swift */; }; + 1A6BEFDDF86DAE98C18BC3BF /* CommandModeDestructiveCommandGapTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 96B2C564157D276092AC9C7D /* CommandModeDestructiveCommandGapTests.swift */; }; 7CDB0A2D2F3C4D5600FB7CAD /* DictationE2ETests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7CDB0A292F3C4D5600FB7CAD /* DictationE2ETests.swift */; }; CD1C7A0000000000000000B2 /* CustomDictionaryManualEntryTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = CD1C7A0000000000000000B1 /* CustomDictionaryManualEntryTests.swift */; }; 7CDB0A2E2F3C4D5600FB7CAD /* AudioFixtureLoader.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7CDB0A2A2F3C4D5600FB7CAD /* AudioFixtureLoader.swift */; }; @@ -68,6 +69,7 @@ DA7100010000000000000001 /* DirectAudioReliabilityTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = DirectAudioReliabilityTests.swift; sourceTree = ""; }; 7C078D8F2E3B339200FB7CAC /* FluidVoice Debug.app */ = {isa = PBXFileReference; explicitFileType = wrapper.application; includeInIndex = 0; path = "FluidVoice Debug.app"; sourceTree = BUILT_PRODUCTS_DIR; }; 7C91B0022F42AA0100C0DEF0 /* HotkeyShortcutTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = HotkeyShortcutTests.swift; sourceTree = ""; }; + 96B2C564157D276092AC9C7D /* CommandModeDestructiveCommandGapTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = CommandModeDestructiveCommandGapTests.swift; sourceTree = ""; }; 7CDB0A202F3C4D5600FB7CAD /* FluidDictationIntegrationTests.xctest */ = {isa = PBXFileReference; explicitFileType = wrapper.cfbundle; includeInIndex = 0; path = FluidDictationIntegrationTests.xctest; sourceTree = BUILT_PRODUCTS_DIR; }; 7CDB0A292F3C4D5600FB7CAD /* DictationE2ETests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = DictationE2ETests.swift; sourceTree = ""; }; CD1C7A0000000000000000B1 /* CustomDictionaryManualEntryTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = CustomDictionaryManualEntryTests.swift; sourceTree = ""; }; @@ -145,6 +147,7 @@ 7CDB0A292F3C4D5600FB7CAD /* DictationE2ETests.swift */, CD1C7A0000000000000000B1 /* CustomDictionaryManualEntryTests.swift */, 7C91B0022F42AA0100C0DEF0 /* HotkeyShortcutTests.swift */, + 96B2C564157D276092AC9C7D /* CommandModeDestructiveCommandGapTests.swift */, 343B29013F4441D6A797D12D /* LLMClientRequestBodyTests.swift */, 980330F3CE464336ADCE3E23 /* TemperatureSupportTests.swift */, A11A00000000000000000001 /* AnalyticsDatabaseTests.swift */, @@ -314,6 +317,7 @@ 7CDB0A2D2F3C4D5600FB7CAD /* DictationE2ETests.swift in Sources */, CD1C7A0000000000000000B2 /* CustomDictionaryManualEntryTests.swift in Sources */, 7C91B0012F42AA0100C0DEF0 /* HotkeyShortcutTests.swift in Sources */, + 1A6BEFDDF86DAE98C18BC3BF /* CommandModeDestructiveCommandGapTests.swift in Sources */, 86CAA2D4EF18433096185602 /* LLMClientRequestBodyTests.swift in Sources */, 272BFB5CB271489892CAE50C /* TemperatureSupportTests.swift in Sources */, A11A00000000000000000002 /* AnalyticsDatabaseTests.swift in Sources */, diff --git a/Sources/Fluid/Services/CommandModeService.swift b/Sources/Fluid/Services/CommandModeService.swift index 5e6248943..96413936a 100644 --- a/Sources/Fluid/Services/CommandModeService.swift +++ b/Sources/Fluid/Services/CommandModeService.swift @@ -437,7 +437,7 @@ final class CommandModeService: ObservableObject { } // Check if we need confirmation for destructive commands - if SettingsStore.shared.commandModeConfirmBeforeExecute, self.isDestructiveCommand(tc.command) { + 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 { } } - private func isDestructiveCommand(_ command: String) -> Bool { + 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) }) { + return true + } + } + return false } diff --git a/Tests/FluidDictationIntegrationTests/CommandModeDestructiveCommandGapTests.swift b/Tests/FluidDictationIntegrationTests/CommandModeDestructiveCommandGapTests.swift new file mode 100644 index 000000000..11a35d036 --- /dev/null +++ b/Tests/FluidDictationIntegrationTests/CommandModeDestructiveCommandGapTests.swift @@ -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" + ) + } + } +} From f59090ad4b2fc438d8516497c8877b3773d3a2f8 Mon Sep 17 00:00:00 2001 From: manjunathbhaskar Date: Fri, 21 Aug 2026 09:12:13 +0200 Subject: [PATCH 02/10] fix(command-mode): quoted absolute paths and diskutil arg false positives 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. --- .../Fluid/Services/CommandModeService.swift | 29 ++++++++++++++--- ...ommandModeDestructiveCommandGapTests.swift | 32 +++++++++++++++++++ 2 files changed, 56 insertions(+), 5 deletions(-) diff --git a/Sources/Fluid/Services/CommandModeService.swift b/Sources/Fluid/Services/CommandModeService.swift index 96413936a..6e74bdaa6 100644 --- a/Sources/Fluid/Services/CommandModeService.swift +++ b/Sources/Fluid/Services/CommandModeService.swift @@ -607,9 +607,20 @@ final class CommandModeService: ObservableObject { // (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" }) + var leadingToken = String( + cmd + .drop(while: { $0 == " " || $0 == "\t" }) + .prefix(while: { $0 != " " && $0 != "\t" }) + ) + // `"/bin/rm" -rf ~` keeps its quotes here since quotes aren't a token + // delimiter above, so lastPathComponent would derive `rm"` and miss the + // match entirely. Strip one matching wrapping pair, same as a shell would. + if leadingToken.count >= 2, + let first = leadingToken.first, let last = leadingToken.last, + first == last, first == "\"" || first == "'" + { + leadingToken = String(leadingToken.dropFirst().dropLast()) + } let commandName = (leadingToken as NSString).lastPathComponent let destructiveCommandNames: Set = [ "rm", "rmdir", "mv", "sudo", "kill", "pkill", "killall", @@ -632,11 +643,19 @@ final class CommandModeService: ObservableObject { // subcommands specifically so read-only uses (`diskutil list`, // `diskutil info`) are not flagged. if commandName == "diskutil" { - let destructiveDiskutilSubcommands = [ + // Match the subcommand token itself, not a substring anywhere in the + // command -- `diskutil info /Volumes/EraseDisk` contains "erasedisk" + // in its argument and isn't destructive at all. + let diskutilSubcommand = cmd + .split(whereSeparator: { $0 == " " || $0 == "\t" }) + .dropFirst() + .first + .map(String.init) ?? "" + let destructiveDiskutilSubcommands: Set = [ "erasedisk", "erasevolume", "secureerase", "reformat", "partitiondisk", "zerodisk", "unmountdisk", ] - if destructiveDiskutilSubcommands.contains(where: { cmd.contains($0) }) { + if destructiveDiskutilSubcommands.contains(diskutilSubcommand) { return true } } diff --git a/Tests/FluidDictationIntegrationTests/CommandModeDestructiveCommandGapTests.swift b/Tests/FluidDictationIntegrationTests/CommandModeDestructiveCommandGapTests.swift index 11a35d036..f2dc27df2 100644 --- a/Tests/FluidDictationIntegrationTests/CommandModeDestructiveCommandGapTests.swift +++ b/Tests/FluidDictationIntegrationTests/CommandModeDestructiveCommandGapTests.swift @@ -82,6 +82,38 @@ final class CommandModeDestructiveCommandGapTests: XCTestCase { } } + // MARK: - Fix 4: quoted absolute-path invocation + + func testQuotedAbsolutePathInvocationIsCaught() { + let cases = [ + "\"/bin/rm\" -rf ~/Documents", + "'/bin/rm' -rf ~/Documents", + "\"/usr/bin/sudo\" reboot", + ] + for command in cases { + XCTAssertTrue( + CommandModeService.isDestructiveCommand(command), + "expected \"\(command)\" to require confirmation despite the quoted absolute path" + ) + } + } + + // MARK: - Fix 5: diskutil subcommand matching doesn't false-positive on arguments + + func testDiskutilArgumentContainingSubcommandNameIsNotFlagged() { + let cases = [ + "diskutil info /Volumes/EraseDisk", + "diskutil list /Volumes/ReformatBackup", + ] + for command in cases { + XCTAssertFalse( + CommandModeService.isDestructiveCommand(command), + "expected \"\(command)\" NOT to require confirmation -- the destructive-looking " + + "text is in an argument, not the diskutil subcommand" + ) + } + } + // MARK: - No new false positives on benign commands func testBenignCommandsAreNotFlagged() { From 9e42636644488978abfbb58568e9f97474371e80 Mon Sep 17 00:00:00 2001 From: manjunathbhaskar Date: Fri, 21 Aug 2026 09:21:01 +0200 Subject: [PATCH 03/10] fix(command-mode): diskutil's quiet modifier no longer hides the verb 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. --- .../Fluid/Services/CommandModeService.swift | 5 +++- ...ommandModeDestructiveCommandGapTests.swift | 23 +++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/Sources/Fluid/Services/CommandModeService.swift b/Sources/Fluid/Services/CommandModeService.swift index 6e74bdaa6..a8db628f5 100644 --- a/Sources/Fluid/Services/CommandModeService.swift +++ b/Sources/Fluid/Services/CommandModeService.swift @@ -645,10 +645,13 @@ final class CommandModeService: ObservableObject { if commandName == "diskutil" { // Match the subcommand token itself, not a substring anywhere in the // command -- `diskutil info /Volumes/EraseDisk` contains "erasedisk" - // in its argument and isn't destructive at all. + // in its argument and isn't destructive at all. `quiet` is a real + // diskutil modifier that can precede the verb (`diskutil quiet + // eraseDisk ...`), so skip over it rather than reading it as the verb. let diskutilSubcommand = cmd .split(whereSeparator: { $0 == " " || $0 == "\t" }) .dropFirst() + .drop(while: { $0 == "quiet" }) .first .map(String.init) ?? "" let destructiveDiskutilSubcommands: Set = [ diff --git a/Tests/FluidDictationIntegrationTests/CommandModeDestructiveCommandGapTests.swift b/Tests/FluidDictationIntegrationTests/CommandModeDestructiveCommandGapTests.swift index f2dc27df2..458993155 100644 --- a/Tests/FluidDictationIntegrationTests/CommandModeDestructiveCommandGapTests.swift +++ b/Tests/FluidDictationIntegrationTests/CommandModeDestructiveCommandGapTests.swift @@ -114,6 +114,29 @@ final class CommandModeDestructiveCommandGapTests: XCTestCase { } } + // MARK: - Fix 6: diskutil's `quiet` modifier doesn't hide the destructive verb + + func testDiskutilQuietModifierStillCatchesDestructiveVerb() { + let cases = [ + "diskutil quiet eraseDisk JHFS+ Untitled disk0", + "diskutil quiet secureErase 0 /dev/disk0", + ] + for command in cases { + XCTAssertTrue( + CommandModeService.isDestructiveCommand(command), + "expected \"\(command)\" to require confirmation -- `quiet` precedes the verb, " + + "it isn't the verb" + ) + } + } + + func testDiskutilQuietModifierAloneIsNotFlagged() { + XCTAssertFalse( + CommandModeService.isDestructiveCommand("diskutil quiet list"), + "expected a read-only verb after `quiet` NOT to require confirmation" + ) + } + // MARK: - No new false positives on benign commands func testBenignCommandsAreNotFlagged() { From e04361a550d5dc91a1960d9f1714f7e6ca93d69b Mon Sep 17 00:00:00 2001 From: manjunathbhaskar Date: Fri, 21 Aug 2026 09:58:39 +0200 Subject: [PATCH 04/10] fix(command-mode): parse commands instead of pattern-matching the leading 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. --- .../Fluid/Services/CommandModeService.swift | 229 +++++++++++------- ...ommandModeDestructiveCommandGapTests.swift | 65 +++++ 2 files changed, 208 insertions(+), 86 deletions(-) diff --git a/Sources/Fluid/Services/CommandModeService.swift b/Sources/Fluid/Services/CommandModeService.swift index a8db628f5..306e2bd97 100644 --- a/Sources/Fluid/Services/CommandModeService.swift +++ b/Sources/Fluid/Services/CommandModeService.swift @@ -559,106 +559,158 @@ final class CommandModeService: ObservableObject { } } - nonisolated static func isDestructiveCommand(_ command: String) -> Bool { - let cmd = command.lowercased() - - // Commands that start with these are destructive - let destructivePrefixes = [ - "rm ", "rm\t", "rmdir ", "rm -", // delete - "mv ", "mv\t", // move/rename - "sudo ", // elevated privileges - "kill ", "pkill ", "killall ", // terminate processes - "chmod ", "chown ", "chgrp ", // change permissions/ownership - "dd ", // disk operations - "mkfs", "format", // filesystem formatting - "> ", // overwrite file - "truncate ", // truncate file - "shred ", // secure delete - ] + private static let destructiveCommandNames: Set = [ + "rm", "rmdir", "mv", "sudo", "kill", "pkill", "killall", + "chmod", "chown", "chgrp", "dd", "shred", "truncate", "format", + ] + + private static let destructiveDiskutilSubcommands: Set = [ + "erasedisk", "erasevolume", "secureerase", + "reformat", "partitiondisk", "zerodisk", "unmountdisk", + ] + + /// Splits a full command into the individual commands a shell would run, on unquoted + /// `&&`, `||`, `;`, `&`, `|`, and newline. Every prior version of this classifier only + /// ever looked at the leading token of the whole string, so anything after a separator + /// -- `cd /tmp && rm -rf victim` -- was invisible to it. This makes every command in a + /// chain go through the same check, at whatever position it's in. + /// + /// Doesn't handle backslash-escaped separators (`\;`) -- an escaped operator inside a + /// `find -exec ... \;` gets split as if it were real, but the -exec/rm detection below + /// checks word membership on the resulting pieces rather than the exact tail, so it + /// still matches correctly. A real escape-aware parser would be needed to do better, + /// and nothing here depends on it. + private nonisolated static func splitIntoSimpleCommands(_ command: String) -> [String] { + var commands: [String] = [] + var current = "" + var quote: Character? + let chars = Array(command) + var i = 0 + while i < chars.count { + let c = chars[i] + if let q = quote { + current.append(c) + if c == q { quote = nil } + i += 1 + continue + } + if c == "\"" || c == "'" { + quote = c + current.append(c) + i += 1 + continue + } + if c == "&" || c == "|" { + if i + 1 < chars.count, chars[i + 1] == c { + i += 1 // swallow the doubled form (&&, ||) as one separator + } + commands.append(current) + current = "" + i += 1 + continue + } + if c == ";" || c == "\n" { + commands.append(current) + current = "" + i += 1 + continue + } + current.append(c) + i += 1 + } + commands.append(current) + return commands + .map { $0.trimmingCharacters(in: .whitespaces) } + .filter { !$0.isEmpty } + } - // Check if command starts with any destructive prefix - if destructivePrefixes.contains(where: { cmd.hasPrefix($0) }) { - return true + /// Tokenizes one simple command into words, tracking quote state so a quoted span + /// (single or double) stays one word even when it contains whitespace -- the gap that + /// let `"/tmp/tools dir/rm" file` resolve to `tools` instead of `rm`. Quote characters + /// themselves are dropped from the output, the same way a shell would consume them. + private nonisolated static func tokenizeWords(_ simpleCommand: String) -> [String] { + var words: [String] = [] + var current = "" + var quote: Character? + var inWord = false + for c in simpleCommand { + if let q = quote { + if c == q { + quote = nil + } else { + current.append(c) + } + inWord = true + continue + } + if c == "\"" || c == "'" { + quote = c + inWord = true + continue + } + if c == " " || c == "\t" { + if inWord { + words.append(current) + current = "" + inWord = false + } + continue + } + current.append(c) + inWord = true } + if inWord { + words.append(current) + } + return words + } - // Check for destructive patterns anywhere in piped commands - let destructivePatterns = [ - "| rm ", "| sudo ", "| dd ", - "; rm ", "; sudo ", - "&& rm ", "&& sudo ", - "xargs rm", "xargs -I", - ] + /// Classifies one already-split simple command. `words` is already lowercase and + /// tokenized, so this only ever deals with correctly-resolved argv, not raw text. + private nonisolated static func isDestructiveSimpleCommand(_ words: [String]) -> Bool { + guard let rawCommand = words.first else { return false } - if destructivePatterns.contains(where: { cmd.contains($0) }) { + // A bare truncating/creating redirect, `> file` or `>> file`, anywhere in the + // command's own words -- not just as a prefix of the whole thing, so `echo bad > + // /etc/hosts` is caught the same as a command that opens with `>` alone. + if words.dropFirst().contains(where: { $0 == ">" || $0 == ">>" }) { return true } - // rm with flags like -rf, -r, -f anywhere - if cmd.contains("rm -") { - return true - } + // Absolute and relative paths resolve to the same bare name a shell would use + // (`/bin/rm`, `/usr/bin/rm`, and bare `rm` are all just `rm`), so this alone + // covers what used to need a separate prefix-list plus a path-resolution pass. + let commandName = (rawCommand as NSString).lastPathComponent - // 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. - var leadingToken = String( - cmd - .drop(while: { $0 == " " || $0 == "\t" }) - .prefix(while: { $0 != " " && $0 != "\t" }) - ) - // `"/bin/rm" -rf ~` keeps its quotes here since quotes aren't a token - // delimiter above, so lastPathComponent would derive `rm"` and miss the - // match entirely. Strip one matching wrapping pair, same as a shell would. - if leadingToken.count >= 2, - let first = leadingToken.first, let last = leadingToken.last, - first == last, first == "\"" || first == "'" - { - leadingToken = String(leadingToken.dropFirst().dropLast()) - } - 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) { + if destructiveCommandNames.contains(commandName) || commandName.hasPrefix("mkfs") { 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 + // `find -delete` / `find ... -exec rm ...` deletes without ever matching a + // bare command name, since `find` itself isn't destructive. + if commandName == "find" { + if words.contains("-delete") { return true } + if words.contains("-exec") && words.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. + // diskutil's erase/reformat/partition subcommands are as destructive as + // `dd`/`mkfs`/`format` but are a different binary and weren't covered by any + // check above. Scoped to the destructive subcommands specifically so read-only + // uses (`diskutil list`, `diskutil info`) aren't flagged. `quiet` is a real + // diskutil modifier that can precede the verb (`diskutil quiet eraseDisk ...`), + // so it's skipped rather than read as the verb itself. if commandName == "diskutil" { - // Match the subcommand token itself, not a substring anywhere in the - // command -- `diskutil info /Volumes/EraseDisk` contains "erasedisk" - // in its argument and isn't destructive at all. `quiet` is a real - // diskutil modifier that can precede the verb (`diskutil quiet - // eraseDisk ...`), so skip over it rather than reading it as the verb. - let diskutilSubcommand = cmd - .split(whereSeparator: { $0 == " " || $0 == "\t" }) - .dropFirst() - .drop(while: { $0 == "quiet" }) - .first - .map(String.init) ?? "" - let destructiveDiskutilSubcommands: Set = [ - "erasedisk", "erasevolume", "secureerase", - "reformat", "partitiondisk", "zerodisk", "unmountdisk", - ] - if destructiveDiskutilSubcommands.contains(diskutilSubcommand) { + let subcommand = words.dropFirst().drop(while: { $0 == "quiet" }).first ?? "" + if destructiveDiskutilSubcommands.contains(subcommand) { + return true + } + } + + // `find ... | xargs rm` hands the destructive program to xargs as its own + // argument rather than invoking it directly, so it never shows up as this + // simple command's own leading word. + if commandName == "xargs" { + if words.dropFirst().contains(where: { destructiveCommandNames.contains($0) }) { return true } } @@ -666,6 +718,11 @@ final class CommandModeService: ObservableObject { return false } + nonisolated static func isDestructiveCommand(_ command: String) -> Bool { + let simpleCommands = splitIntoSimpleCommands(command.lowercased()) + return simpleCommands.contains { isDestructiveSimpleCommand(tokenizeWords($0)) } + } + private func executeCommand(_ command: String, workingDirectory: String?, callId: String, purpose: String? = nil) async { self.currentStep = .executing(command) diff --git a/Tests/FluidDictationIntegrationTests/CommandModeDestructiveCommandGapTests.swift b/Tests/FluidDictationIntegrationTests/CommandModeDestructiveCommandGapTests.swift index 458993155..42c5fcd0b 100644 --- a/Tests/FluidDictationIntegrationTests/CommandModeDestructiveCommandGapTests.swift +++ b/Tests/FluidDictationIntegrationTests/CommandModeDestructiveCommandGapTests.swift @@ -137,6 +137,71 @@ final class CommandModeDestructiveCommandGapTests: XCTestCase { ) } + // MARK: - Fix 7: format/mkfs.* basenames + + func testFormatAndMkfsVariantBasenamesAreCaught() { + let cases = [ + "/usr/local/bin/format /dev/disk2", + "/sbin/mkfs.ext4 /dev/sdb1", + "mkfs.vfat /dev/disk3", + ] + for command in cases { + XCTAssertTrue( + CommandModeService.isDestructiveCommand(command), + "expected \"\(command)\" to require confirmation" + ) + } + } + + // MARK: - Fix 8: quoted path with an internal space + + func testQuotedPathWithInternalSpaceIsCaught() { + XCTAssertTrue( + CommandModeService.isDestructiveCommand("\"/tmp/tools dir/rm\" file"), + "expected the quoted path to resolve to rm despite the internal space" + ) + } + + // MARK: - Fix 9: destructive command after a compound-command separator + + func testDestructiveCommandAfterSeparatorIsCaught() { + let cases = [ + "cd /tmp && /bin/rm victim", + "echo done; rm -rf ~/Documents", + "true || sudo reboot", + "find . -name '*.log' | xargs rm", + ] + for command in cases { + XCTAssertTrue( + CommandModeService.isDestructiveCommand(command), + "expected \"\(command)\" to require confirmation for the command after the separator" + ) + } + } + + func testBenignCommandsChainedWithSeparatorsAreNotFlagged() { + let cases = [ + "cd /tmp && ls -la", + "git status; git log", + "find . -name '*.log' | xargs cat", + ] + for command in cases { + XCTAssertFalse( + CommandModeService.isDestructiveCommand(command), + "expected \"\(command)\" NOT to require confirmation" + ) + } + } + + // MARK: - Fix 10: redirect anywhere in the command, not just as a leading prefix + + func testRedirectAnywhereInCommandIsCaught() { + XCTAssertTrue( + CommandModeService.isDestructiveCommand("echo malicious > /etc/hosts"), + "expected a mid-command redirect to require confirmation" + ) + } + // MARK: - No new false positives on benign commands func testBenignCommandsAreNotFlagged() { From 2da52a8308537dde5c57b520bb0c92c0f6b6d059 Mon Sep 17 00:00:00 2001 From: manjunathbhaskar Date: Fri, 21 Aug 2026 10:17:55 +0200 Subject: [PATCH 05/10] fix(command-mode): resolve xargs targets, bare redirects, and env assignments 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. --- .../Fluid/Services/CommandModeService.swift | 35 ++++++++++---- ...ommandModeDestructiveCommandGapTests.swift | 46 +++++++++++++++++++ 2 files changed, 72 insertions(+), 9 deletions(-) diff --git a/Sources/Fluid/Services/CommandModeService.swift b/Sources/Fluid/Services/CommandModeService.swift index 306e2bd97..5f5669c1a 100644 --- a/Sources/Fluid/Services/CommandModeService.swift +++ b/Sources/Fluid/Services/CommandModeService.swift @@ -665,18 +665,31 @@ final class CommandModeService: ObservableObject { return words } + /// `NAME=value` in leading position is a real shell feature -- it scopes an + /// environment variable to the command that follows, not a command itself. + /// `LC_ALL=C rm -rf victim` runs `rm`, not `lc_all=c`. + private nonisolated static func isEnvironmentAssignmentWord(_ word: String) -> Bool { + guard let equalsIndex = word.firstIndex(of: "=") else { return false } + let name = word[word.startIndex.. Bool { - guard let rawCommand = words.first else { return false } + guard !words.isEmpty else { return false } // A bare truncating/creating redirect, `> file` or `>> file`, anywhere in the - // command's own words -- not just as a prefix of the whole thing, so `echo bad > - // /etc/hosts` is caught the same as a command that opens with `>` alone. - if words.dropFirst().contains(where: { $0 == ">" || $0 == ">>" }) { + // command's own words, including as the very first one (`> file` alone is a + // complete, valid, destructive command). + if words.contains(where: { $0 == ">" || $0 == ">>" }) { return true } + let remaining = words.drop(while: isEnvironmentAssignmentWord) + guard let rawCommand = remaining.first else { return false } + // Absolute and relative paths resolve to the same bare name a shell would use // (`/bin/rm`, `/usr/bin/rm`, and bare `rm` are all just `rm`), so this alone // covers what used to need a separate prefix-list plus a path-resolution pass. @@ -689,8 +702,8 @@ final class CommandModeService: ObservableObject { // `find -delete` / `find ... -exec rm ...` deletes without ever matching a // bare command name, since `find` itself isn't destructive. if commandName == "find" { - if words.contains("-delete") { return true } - if words.contains("-exec") && words.contains("rm") { return true } + if remaining.contains("-delete") { return true } + if remaining.contains("-exec") && remaining.contains("rm") { return true } } // diskutil's erase/reformat/partition subcommands are as destructive as @@ -700,7 +713,7 @@ final class CommandModeService: ObservableObject { // diskutil modifier that can precede the verb (`diskutil quiet eraseDisk ...`), // so it's skipped rather than read as the verb itself. if commandName == "diskutil" { - let subcommand = words.dropFirst().drop(while: { $0 == "quiet" }).first ?? "" + let subcommand = remaining.dropFirst().drop(while: { $0 == "quiet" }).first ?? "" if destructiveDiskutilSubcommands.contains(subcommand) { return true } @@ -708,9 +721,13 @@ final class CommandModeService: ObservableObject { // `find ... | xargs rm` hands the destructive program to xargs as its own // argument rather than invoking it directly, so it never shows up as this - // simple command's own leading word. + // simple command's own leading word. That argument can itself be path-qualified + // (`xargs -I{} /bin/rm {}`), so it needs the same basename resolution as the + // leading command, not a raw-string comparison. if commandName == "xargs" { - if words.dropFirst().contains(where: { destructiveCommandNames.contains($0) }) { + if remaining.dropFirst().contains(where: { + destructiveCommandNames.contains(($0 as NSString).lastPathComponent) + }) { return true } } diff --git a/Tests/FluidDictationIntegrationTests/CommandModeDestructiveCommandGapTests.swift b/Tests/FluidDictationIntegrationTests/CommandModeDestructiveCommandGapTests.swift index 42c5fcd0b..4eb15d7be 100644 --- a/Tests/FluidDictationIntegrationTests/CommandModeDestructiveCommandGapTests.swift +++ b/Tests/FluidDictationIntegrationTests/CommandModeDestructiveCommandGapTests.swift @@ -202,6 +202,52 @@ final class CommandModeDestructiveCommandGapTests: XCTestCase { ) } + // MARK: - Fix 11: path-qualified xargs target + + func testPathQualifiedXargsTargetIsCaught() { + XCTAssertTrue( + CommandModeService.isDestructiveCommand("find . -print0 | xargs -0 /bin/rm"), + "expected the path-qualified xargs target to resolve to rm" + ) + } + + // MARK: - Fix 12: bare redirect as the entire simple command + + func testBareRedirectAsEntireCommandIsCaught() { + let cases = [ + "> important.txt", + ">> important.txt", + ] + for command in cases { + XCTAssertTrue( + CommandModeService.isDestructiveCommand(command), + "expected \"\(command)\" to require confirmation" + ) + } + } + + // MARK: - Fix 13: leading environment assignment before a destructive command + + func testLeadingEnvironmentAssignmentIsCaught() { + let cases = [ + "LC_ALL=C rm -rf victim", + "A=1 B=2 sudo reboot", + ] + for command in cases { + XCTAssertTrue( + CommandModeService.isDestructiveCommand(command), + "expected \"\(command)\" to require confirmation despite the leading assignment" + ) + } + } + + func testLeadingEnvironmentAssignmentAloneIsNotFlagged() { + XCTAssertFalse( + CommandModeService.isDestructiveCommand("LC_ALL=C ls -la"), + "expected a benign command after an assignment NOT to require confirmation" + ) + } + // MARK: - No new false positives on benign commands func testBenignCommandsAreNotFlagged() { From 3d8ec3c8c7a609c9540c5c49e0bd7ab2aacb4ec0 Mon Sep 17 00:00:00 2001 From: manjunathbhaskar Date: Fri, 21 Aug 2026 10:35:51 +0200 Subject: [PATCH 06/10] fix(command-mode): resolve argument programs everywhere, honor escapes, 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. --- .../Fluid/Services/CommandModeService.swift | 101 +++++++++++++++--- ...ommandModeDestructiveCommandGapTests.swift | 78 ++++++++++++++ 2 files changed, 163 insertions(+), 16 deletions(-) diff --git a/Sources/Fluid/Services/CommandModeService.swift b/Sources/Fluid/Services/CommandModeService.swift index 5f5669c1a..a293f348e 100644 --- a/Sources/Fluid/Services/CommandModeService.swift +++ b/Sources/Fluid/Services/CommandModeService.swift @@ -588,6 +588,15 @@ final class CommandModeService: ObservableObject { var i = 0 while i < chars.count { let c = chars[i] + // A backslash escapes the next character (outside single quotes), so an + // escaped separator like `find ... -exec rm {} \;` doesn't split here. + // Both characters are kept verbatim -- tokenizeWords consumes the escape. + if c == "\\", quote != "'", i + 1 < chars.count { + current.append(c) + current.append(chars[i + 1]) + i += 2 + continue + } if let q = quote { current.append(c) if c == q { quote = nil } @@ -627,13 +636,27 @@ final class CommandModeService: ObservableObject { /// Tokenizes one simple command into words, tracking quote state so a quoted span /// (single or double) stays one word even when it contains whitespace -- the gap that /// let `"/tmp/tools dir/rm" file` resolve to `tools` instead of `rm`. Quote characters - /// themselves are dropped from the output, the same way a shell would consume them. + /// themselves are dropped from the output, the same way a shell would consume them, + /// and a backslash escapes the next character (outside single quotes, where a shell + /// treats it literally) so `/tmp/tools\ dir/rm` stays one word too. private nonisolated static func tokenizeWords(_ simpleCommand: String) -> [String] { var words: [String] = [] var current = "" var quote: Character? var inWord = false + var escaped = false for c in simpleCommand { + if escaped { + current.append(c) + inWord = true + escaped = false + continue + } + if c == "\\", quote != "'" { + escaped = true + inWord = true + continue + } if let q = quote { if c == q { quote = nil @@ -675,9 +698,42 @@ final class CommandModeService: ObservableObject { return name.allSatisfy { $0.isLetter || $0.isNumber || $0 == "_" } } + /// Utilities that take another program as an argument and run it. The destructive + /// program is never this simple command's own leading word, so each argument has to + /// be considered a candidate command in its own right. + private static let commandRunnerNames: Set = [ + "xargs", "env", "nohup", "command", "exec", "time", "nice", + "setsid", "stdbuf", "timeout", "watch", "sudo", + ] + + /// Shells whose `-c` payload is itself a shell command, so it can be parsed with the + /// same machinery rather than treated as an opaque string. Distinct from a general + /// interpreter (`python3 -c`), whose payload is a different language entirely and is + /// out of reach of this classifier. + private static let shellInterpreterNames: Set = [ + "sh", "bash", "zsh", "dash", "ksh", + ] + + /// True if any word, resolved to its basename, names a destructive program. + /// Anywhere a program can appear as an argument rather than as argv[0], the same + /// basename resolution the leading command gets has to apply -- comparing raw words + /// misses `/bin/rm` in exactly the places the leading-word check would have caught it. + private nonisolated static func containsDestructiveProgram( + _ words: ArraySlice + ) -> Bool { + words.contains { word in + let name = (word as NSString).lastPathComponent + return destructiveCommandNames.contains(name) || name.hasPrefix("mkfs") + } + } + /// Classifies one already-split simple command. `words` is already lowercase and /// tokenized, so this only ever deals with correctly-resolved argv, not raw text. - private nonisolated static func isDestructiveSimpleCommand(_ words: [String]) -> Bool { + /// `depth` bounds recursion into nested shell payloads. + private nonisolated static func isDestructiveSimpleCommand( + _ words: [String], + depth: Int + ) -> Bool { guard !words.isEmpty else { return false } // A bare truncating/creating redirect, `> file` or `>> file`, anywhere in the @@ -700,10 +756,13 @@ final class CommandModeService: ObservableObject { } // `find -delete` / `find ... -exec rm ...` deletes without ever matching a - // bare command name, since `find` itself isn't destructive. + // bare command name, since `find` itself isn't destructive. The -exec target + // can be path-qualified like any other program reference. if commandName == "find" { if remaining.contains("-delete") { return true } - if remaining.contains("-exec") && remaining.contains("rm") { return true } + if remaining.contains("-exec") || remaining.contains("-execdir") { + if containsDestructiveProgram(remaining.dropFirst()) { return true } + } } // diskutil's erase/reformat/partition subcommands are as destructive as @@ -719,25 +778,35 @@ final class CommandModeService: ObservableObject { } } - // `find ... | xargs rm` hands the destructive program to xargs as its own - // argument rather than invoking it directly, so it never shows up as this - // simple command's own leading word. That argument can itself be path-qualified - // (`xargs -I{} /bin/rm {}`), so it needs the same basename resolution as the - // leading command, not a raw-string comparison. - if commandName == "xargs" { - if remaining.dropFirst().contains(where: { - destructiveCommandNames.contains(($0 as NSString).lastPathComponent) - }) { - return true + // `xargs rm`, `env rm`, `nohup rm`, `sudo rm` -- the program that actually runs + // is an argument here, not argv[0]. + if commandRunnerNames.contains(commandName) { + if containsDestructiveProgram(remaining.dropFirst()) { return true } + } + + // `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 { + for argument in remaining.dropFirst() where !argument.hasPrefix("-") { + if isDestructiveCommand(argument, depth: depth + 1) { return true } } } return false } - nonisolated static func isDestructiveCommand(_ command: String) -> Bool { + /// Bounds `sh -c '...'` nesting so a pathological payload can't recurse without end. + private static let maxShellRecursionDepth = 4 + + private nonisolated static func isDestructiveCommand(_ command: String, depth: Int) -> Bool { let simpleCommands = splitIntoSimpleCommands(command.lowercased()) - return simpleCommands.contains { isDestructiveSimpleCommand(tokenizeWords($0)) } + return simpleCommands.contains { + isDestructiveSimpleCommand(tokenizeWords($0), depth: depth) + } + } + + nonisolated static func isDestructiveCommand(_ command: String) -> Bool { + isDestructiveCommand(command, depth: 0) } private func executeCommand(_ command: String, workingDirectory: String?, callId: String, purpose: String? = nil) async { diff --git a/Tests/FluidDictationIntegrationTests/CommandModeDestructiveCommandGapTests.swift b/Tests/FluidDictationIntegrationTests/CommandModeDestructiveCommandGapTests.swift index 4eb15d7be..a9185e4e3 100644 --- a/Tests/FluidDictationIntegrationTests/CommandModeDestructiveCommandGapTests.swift +++ b/Tests/FluidDictationIntegrationTests/CommandModeDestructiveCommandGapTests.swift @@ -248,6 +248,84 @@ final class CommandModeDestructiveCommandGapTests: XCTestCase { ) } + // MARK: - Fix 14: backslash-escaped whitespace in an executable path + + func testEscapedWhitespaceInPathIsCaught() { + let cases = [ + "/tmp/tools\\ dir/rm -rf victim", + "/tmp/tools\\ dir/sudo reboot", + ] + for command in cases { + XCTAssertTrue( + CommandModeService.isDestructiveCommand(command), + "expected \"\(command)\" to resolve past the escaped space to the real basename" + ) + } + } + + // MARK: - Fix 15: program passed as an argument to a runner resolves by basename + + func testPathQualifiedProgramInRunnersIsCaught() { + let cases = [ + "find . -exec /bin/rm -rf {} \\;", + "find . -execdir /bin/rm {} \\;", + "env /bin/rm -rf victim", + "nohup /bin/rm -rf victim", + "time /bin/rm victim", + ] + for command in cases { + XCTAssertTrue( + CommandModeService.isDestructiveCommand(command), + "expected \"\(command)\" to resolve the argument program to its basename" + ) + } + } + + func testBenignProgramsInRunnersAreNotFlagged() { + let cases = [ + "env /bin/ls -la", + "nohup /usr/bin/python3 script.py", + "time git status", + "find . -type f -exec /bin/cat {} \\;", + ] + for command in cases { + XCTAssertFalse( + CommandModeService.isDestructiveCommand(command), + "expected \"\(command)\" NOT to require confirmation" + ) + } + } + + // MARK: - Fix 16: shell -c payload is parsed, not treated as an opaque argument + + func testShellWrapperPayloadIsParsed() { + let cases = [ + "sh -c 'rm -rf victim'", + "bash -c \"rm -rf victim\"", + "zsh -c 'cd /tmp && /bin/rm victim'", + "sh -c 'sh -c \"rm -rf victim\"'", + ] + for command in cases { + XCTAssertTrue( + CommandModeService.isDestructiveCommand(command), + "expected the shell payload in \"\(command)\" to be parsed for destructive commands" + ) + } + } + + func testBenignShellWrapperPayloadIsNotFlagged() { + let cases = [ + "sh -c 'ls -la'", + "bash -c \"git status\"", + ] + for command in cases { + XCTAssertFalse( + CommandModeService.isDestructiveCommand(command), + "expected \"\(command)\" NOT to require confirmation" + ) + } + } + // MARK: - No new false positives on benign commands func testBenignCommandsAreNotFlagged() { From e15aac273408494959be5d5511cf62ac0dde7593 Mon Sep 17 00:00:00 2001 From: manjunathbhaskar Date: Fri, 21 Aug 2026 10:50:44 +0200 Subject: [PATCH 07/10] fix(command-mode): treat a shell in argument position as a nested invocation 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. --- .../Fluid/Services/CommandModeService.swift | 34 ++++++++++++------ ...ommandModeDestructiveCommandGapTests.swift | 35 +++++++++++++++++++ 2 files changed, 59 insertions(+), 10 deletions(-) diff --git a/Sources/Fluid/Services/CommandModeService.swift b/Sources/Fluid/Services/CommandModeService.swift index a293f348e..be916a9d1 100644 --- a/Sources/Fluid/Services/CommandModeService.swift +++ b/Sources/Fluid/Services/CommandModeService.swift @@ -714,17 +714,31 @@ final class CommandModeService: ObservableObject { "sh", "bash", "zsh", "dash", "ksh", ] - /// True if any word, resolved to its basename, names a destructive program. - /// Anywhere a program can appear as an argument rather than as argv[0], the same - /// basename resolution the leading command gets has to apply -- comparing raw words - /// misses `/bin/rm` in exactly the places the leading-word check would have caught it. + /// True if a program reference anywhere in `words` is destructive. + /// + /// Used for every position where a program can appear as an argument rather than as + /// argv[0]. Two things have to hold there, and they are the same two that hold for a + /// leading command: a reference resolves by basename (so `/bin/rm` is `rm`), and a + /// shell is not a leaf -- everything after it is a nested invocation that gets the + /// same classification, so `xargs sh -c 'rm -rf x'` is caught like `sh -c 'rm -rf x'`. private nonisolated static func containsDestructiveProgram( - _ words: ArraySlice + _ words: ArraySlice, + depth: Int ) -> Bool { - words.contains { word in - let name = (word as NSString).lastPathComponent - return destructiveCommandNames.contains(name) || name.hasPrefix("mkfs") + var index = words.startIndex + while index < words.endIndex { + let name = (words[index] as NSString).lastPathComponent + if destructiveCommandNames.contains(name) || name.hasPrefix("mkfs") { + return true + } + if shellInterpreterNames.contains(name), depth < maxShellRecursionDepth { + if isDestructiveSimpleCommand(Array(words[index...]), depth: depth) { + return true + } + } + index = words.index(after: index) } + return false } /// Classifies one already-split simple command. `words` is already lowercase and @@ -761,7 +775,7 @@ final class CommandModeService: ObservableObject { if commandName == "find" { if remaining.contains("-delete") { return true } if remaining.contains("-exec") || remaining.contains("-execdir") { - if containsDestructiveProgram(remaining.dropFirst()) { return true } + if containsDestructiveProgram(remaining.dropFirst(), depth: depth) { return true } } } @@ -781,7 +795,7 @@ final class CommandModeService: ObservableObject { // `xargs rm`, `env rm`, `nohup rm`, `sudo rm` -- the program that actually runs // is an argument here, not argv[0]. if commandRunnerNames.contains(commandName) { - if containsDestructiveProgram(remaining.dropFirst()) { return true } + if containsDestructiveProgram(remaining.dropFirst(), depth: depth) { return true } } // `sh -c 'rm -rf victim'` -- the payload is a shell command, so run it back diff --git a/Tests/FluidDictationIntegrationTests/CommandModeDestructiveCommandGapTests.swift b/Tests/FluidDictationIntegrationTests/CommandModeDestructiveCommandGapTests.swift index a9185e4e3..35b72d941 100644 --- a/Tests/FluidDictationIntegrationTests/CommandModeDestructiveCommandGapTests.swift +++ b/Tests/FluidDictationIntegrationTests/CommandModeDestructiveCommandGapTests.swift @@ -326,6 +326,41 @@ final class CommandModeDestructiveCommandGapTests: XCTestCase { } } + // MARK: - Fix 17: a shell in argument position is a nested invocation, not a leaf + + func testNestedShellPayloadInArgumentPositionIsCaught() { + let cases = [ + "xargs sh -c 'rm -rf victim'", + "find . -exec sh -c 'rm -rf victim' \\;", + "find . -execdir bash -c 'rm -rf victim' \\;", + "env sh -c 'rm -rf victim'", + "nohup zsh -c 'rm -rf victim'", + "timeout 5 sh -c 'rm -rf victim'", + "xargs -I{} /bin/sh -c 'rm -rf {}'", + "cd /tmp && xargs sh -c 'rm -rf victim'", + ] + for command in cases { + XCTAssertTrue( + CommandModeService.isDestructiveCommand(command), + "expected the nested shell payload in \"\(command)\" to be parsed" + ) + } + } + + func testNestedBenignShellPayloadInArgumentPositionIsNotFlagged() { + let cases = [ + "xargs sh -c 'ls -la'", + "find . -exec sh -c 'cat {}' \\;", + "env bash -c 'git status'", + ] + for command in cases { + XCTAssertFalse( + CommandModeService.isDestructiveCommand(command), + "expected \"\(command)\" NOT to require confirmation" + ) + } + } + // MARK: - No new false positives on benign commands func testBenignCommandsAreNotFlagged() { From d00fe1a26a4ab423b2cea5b54dbdb70cbd8943c7 Mon Sep 17 00:00:00 2001 From: manjunathbhaskar Date: Tue, 1 Sep 2026 20:44:37 +0200 Subject: [PATCH 08/10] fix(command-mode): replace destructive-pattern detection with a known-safe-command allowlist The blocklist approach kept needing more code every review round to close a new place a destructive command could hide (absolute paths, quoting, xargs, find -exec/-delete, eval, nested shells, escapes, leading redirects, runners). Flips the model instead: only a short, explicit set of known-safe commands is trusted, everything else requires confirmation by default, and redirects, command substitution, and shell metacharacters are rejected outright regardless of the leading command. Closes every previously reported bypass structurally rather than case by case, including two that were never addressed in review (recursion depth, special commands behind a runner), plus shell grouping and control-flow forms that were explicit known limitations before. Net decrease in source lines versus both the original PR and the prior cleanup pass. --- .../Fluid/Services/CommandModeService.swift | 280 +++--------- ...ommandModeDestructiveCommandGapTests.swift | 411 +++++------------- 2 files changed, 165 insertions(+), 526 deletions(-) diff --git a/Sources/Fluid/Services/CommandModeService.swift b/Sources/Fluid/Services/CommandModeService.swift index be916a9d1..404e16284 100644 --- a/Sources/Fluid/Services/CommandModeService.swift +++ b/Sources/Fluid/Services/CommandModeService.swift @@ -559,138 +559,63 @@ final class CommandModeService: ObservableObject { } } - private static let destructiveCommandNames: Set = [ - "rm", "rmdir", "mv", "sudo", "kill", "pkill", "killall", - "chmod", "chown", "chgrp", "dd", "shred", "truncate", "format", - ] - - private static let destructiveDiskutilSubcommands: Set = [ - "erasedisk", "erasevolume", "secureerase", - "reformat", "partitiondisk", "zerodisk", "unmountdisk", - ] - - /// Splits a full command into the individual commands a shell would run, on unquoted - /// `&&`, `||`, `;`, `&`, `|`, and newline. Every prior version of this classifier only - /// ever looked at the leading token of the whole string, so anything after a separator - /// -- `cd /tmp && rm -rf victim` -- was invisible to it. This makes every command in a - /// chain go through the same check, at whatever position it's in. - /// - /// Doesn't handle backslash-escaped separators (`\;`) -- an escaped operator inside a - /// `find -exec ... \;` gets split as if it were real, but the -exec/rm detection below - /// checks word membership on the resulting pieces rather than the exact tail, so it - /// still matches correctly. A real escape-aware parser would be needed to do better, - /// and nothing here depends on it. + /// Splits a command into the simple commands a shell would run, on unquoted + /// `&&`, `||`, `;`, `&`, `|`, and newline, so every command in a chain gets + /// checked, not just the first one before a separator. + /// Splits on unquoted `;`, `&&`, `||`, `|`, newline. Quotes are tracked so a + /// separator inside a quoted argument doesn't split; escaped separators are + /// not handled, an over-split just means one more piece to check, which can + /// only ask for confirmation more often, never less. private nonisolated static func splitIntoSimpleCommands(_ command: String) -> [String] { - var commands: [String] = [] + var parts: [String] = [] var current = "" var quote: Character? - let chars = Array(command) - var i = 0 - while i < chars.count { - let c = chars[i] - // A backslash escapes the next character (outside single quotes), so an - // escaped separator like `find ... -exec rm {} \;` doesn't split here. - // Both characters are kept verbatim -- tokenizeWords consumes the escape. - if c == "\\", quote != "'", i + 1 < chars.count { - current.append(c) - current.append(chars[i + 1]) - i += 2 - continue - } + for c in command { if let q = quote { current.append(c) if c == q { quote = nil } - i += 1 continue } if c == "\"" || c == "'" { quote = c current.append(c) - i += 1 - continue - } - if c == "&" || c == "|" { - if i + 1 < chars.count, chars[i + 1] == c { - i += 1 // swallow the doubled form (&&, ||) as one separator - } - commands.append(current) - current = "" - i += 1 - continue - } - if c == ";" || c == "\n" { - commands.append(current) + } else if ";&|\n".contains(c) { + parts.append(current) current = "" - i += 1 - continue + } else { + current.append(c) } - current.append(c) - i += 1 } - commands.append(current) - return commands - .map { $0.trimmingCharacters(in: .whitespaces) } - .filter { !$0.isEmpty } + parts.append(current) + return parts.map { $0.trimmingCharacters(in: .whitespaces) }.filter { !$0.isEmpty } } - /// Tokenizes one simple command into words, tracking quote state so a quoted span - /// (single or double) stays one word even when it contains whitespace -- the gap that - /// let `"/tmp/tools dir/rm" file` resolve to `tools` instead of `rm`. Quote characters - /// themselves are dropped from the output, the same way a shell would consume them, - /// and a backslash escapes the next character (outside single quotes, where a shell - /// treats it literally) so `/tmp/tools\ dir/rm` stays one word too. - private nonisolated static func tokenizeWords(_ simpleCommand: String) -> [String] { - var words: [String] = [] - var current = "" - var quote: Character? - var inWord = false - var escaped = false - for c in simpleCommand { - if escaped { - current.append(c) - inWord = true - escaped = false - continue - } - if c == "\\", quote != "'" { - escaped = true - inWord = true - continue - } - if let q = quote { - if c == q { - quote = nil - } else { - current.append(c) - } - inWord = true - continue - } - if c == "\"" || c == "'" { - quote = c - inWord = true - continue - } - if c == " " || c == "\t" { - if inWord { - words.append(current) - current = "" - inWord = false - } - continue - } - current.append(c) - inWord = true + /// Characters that mean this simple command isn't plain text: a redirect, a + /// subshell, or command substitution. Any of these riding along with an + /// otherwise safe command means it isn't actually safe, `echo $(rm -rf x)` + /// is not safe just because `echo` is. Filenames with these characters will + /// also require confirmation; that's an acceptable cost of not being a full + /// shell parser. + private static let unsafeCharacters = CharacterSet(charactersIn: "$`(){}<>") + + /// The leading word of a simple command, with a leading env assignment + /// dropped (`LC_ALL=C ls` runs `ls`) and surrounding quotes stripped if the + /// whole word is one quoted span. + private nonisolated static func leadingCommand(_ simpleCommand: String) -> String? { + var words = simpleCommand.split(separator: " ").map(String.init) + while let first = words.first, isEnvironmentAssignmentWord(first) { + words.removeFirst() } - if inWord { - words.append(current) + guard var word = words.first else { return nil } + if word.count >= 2, let q = word.first, (q == "\"" || q == "'"), word.last == q { + word.removeFirst() + word.removeLast() } - return words + return word } - /// `NAME=value` in leading position is a real shell feature -- it scopes an - /// environment variable to the command that follows, not a command itself. - /// `LC_ALL=C rm -rf victim` runs `rm`, not `lc_all=c`. + /// `NAME=value` in leading position scopes an env var to the command that + /// follows; `LC_ALL=C rm -rf victim` runs `rm`, not `lc_all=c`. private nonisolated static func isEnvironmentAssignmentWord(_ word: String) -> Bool { guard let equalsIndex = word.firstIndex(of: "=") else { return false } let name = word[word.startIndex.. = [ - "xargs", "env", "nohup", "command", "exec", "time", "nice", - "setsid", "stdbuf", "timeout", "watch", "sudo", - ] - - /// Shells whose `-c` payload is itself a shell command, so it can be parsed with the - /// same machinery rather than treated as an opaque string. Distinct from a general - /// interpreter (`python3 -c`), whose payload is a different language entirely and is - /// out of reach of this classifier. - private static let shellInterpreterNames: Set = [ - "sh", "bash", "zsh", "dash", "ksh", + /// Commands that never take a destructive action regardless of arguments or + /// flags. Short and conservative on purpose: anything not recognized asks + /// for confirmation, rather than trying to enumerate every way a command + /// could be dangerous. Add a command only after auditing every invocation. + private static let knownSafeCommands: Set = [ + "ls", "cat", "echo", "pwd", "whoami", "date", "hostname", "uname", + "head", "tail", "wc", "file", "stat", "which", "type", "printenv", ] - /// True if a program reference anywhere in `words` is destructive. - /// - /// Used for every position where a program can appear as an argument rather than as - /// argv[0]. Two things have to hold there, and they are the same two that hold for a - /// leading command: a reference resolves by basename (so `/bin/rm` is `rm`), and a - /// shell is not a leaf -- everything after it is a nested invocation that gets the - /// same classification, so `xargs sh -c 'rm -rf x'` is caught like `sh -c 'rm -rf x'`. - private nonisolated static func containsDestructiveProgram( - _ words: ArraySlice, - depth: Int - ) -> Bool { - var index = words.startIndex - while index < words.endIndex { - let name = (words[index] as NSString).lastPathComponent - if destructiveCommandNames.contains(name) || name.hasPrefix("mkfs") { - return true - } - if shellInterpreterNames.contains(name), depth < maxShellRecursionDepth { - if isDestructiveSimpleCommand(Array(words[index...]), depth: depth) { - return true - } - } - index = words.index(after: index) - } - return false - } - - /// Classifies one already-split simple command. `words` is already lowercase and - /// tokenized, so this only ever deals with correctly-resolved argv, not raw text. - /// `depth` bounds recursion into nested shell payloads. - private nonisolated static func isDestructiveSimpleCommand( - _ words: [String], - depth: Int - ) -> Bool { - guard !words.isEmpty else { return false } - - // A bare truncating/creating redirect, `> file` or `>> file`, anywhere in the - // command's own words, including as the very first one (`> file` alone is a - // complete, valid, destructive command). - if words.contains(where: { $0 == ">" || $0 == ">>" }) { - return true - } - - let remaining = words.drop(while: isEnvironmentAssignmentWord) - guard let rawCommand = remaining.first else { return false } - - // Absolute and relative paths resolve to the same bare name a shell would use - // (`/bin/rm`, `/usr/bin/rm`, and bare `rm` are all just `rm`), so this alone - // covers what used to need a separate prefix-list plus a path-resolution pass. - let commandName = (rawCommand as NSString).lastPathComponent - - if destructiveCommandNames.contains(commandName) || commandName.hasPrefix("mkfs") { - return true - } - - // `find -delete` / `find ... -exec rm ...` deletes without ever matching a - // bare command name, since `find` itself isn't destructive. The -exec target - // can be path-qualified like any other program reference. - if commandName == "find" { - if remaining.contains("-delete") { return true } - if remaining.contains("-exec") || remaining.contains("-execdir") { - if containsDestructiveProgram(remaining.dropFirst(), depth: depth) { return true } - } - } - - // diskutil's erase/reformat/partition subcommands are as destructive as - // `dd`/`mkfs`/`format` but are a different binary and weren't covered by any - // check above. Scoped to the destructive subcommands specifically so read-only - // uses (`diskutil list`, `diskutil info`) aren't flagged. `quiet` is a real - // diskutil modifier that can precede the verb (`diskutil quiet eraseDisk ...`), - // so it's skipped rather than read as the verb itself. - if commandName == "diskutil" { - let subcommand = remaining.dropFirst().drop(while: { $0 == "quiet" }).first ?? "" - if destructiveDiskutilSubcommands.contains(subcommand) { - return true - } - } - - // `xargs rm`, `env rm`, `nohup rm`, `sudo rm` -- the program that actually runs - // is an argument here, not argv[0]. - if commandRunnerNames.contains(commandName) { - if containsDestructiveProgram(remaining.dropFirst(), depth: depth) { return true } - } - - // `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 { - for argument in remaining.dropFirst() where !argument.hasPrefix("-") { - if isDestructiveCommand(argument, depth: depth + 1) { return true } - } - } - - return false + nonisolated static func isDestructiveCommand(_ command: String) -> Bool { + splitIntoSimpleCommands(command.lowercased()).contains { !isSafeSimpleCommand($0) } } - /// Bounds `sh -c '...'` nesting so a pathological payload can't recurse without end. - private static let maxShellRecursionDepth = 4 - - private nonisolated static func isDestructiveCommand(_ command: String, depth: Int) -> Bool { - let simpleCommands = splitIntoSimpleCommands(command.lowercased()) - return simpleCommands.contains { - isDestructiveSimpleCommand(tokenizeWords($0), depth: depth) - } + private nonisolated static func isSafeSimpleCommand(_ simpleCommand: String) -> Bool { + guard simpleCommand.rangeOfCharacter(from: unsafeCharacters) == nil else { return false } + guard let name = leadingCommand(simpleCommand) else { return true } + return knownSafeCommands.contains((name as NSString).lastPathComponent) } - nonisolated static func isDestructiveCommand(_ command: String) -> Bool { - isDestructiveCommand(command, depth: 0) - } private func executeCommand(_ command: String, workingDirectory: String?, callId: String, purpose: String? = nil) async { self.currentStep = .executing(command) diff --git a/Tests/FluidDictationIntegrationTests/CommandModeDestructiveCommandGapTests.swift b/Tests/FluidDictationIntegrationTests/CommandModeDestructiveCommandGapTests.swift index 35b72d941..c72919aec 100644 --- a/Tests/FluidDictationIntegrationTests/CommandModeDestructiveCommandGapTests.swift +++ b/Tests/FluidDictationIntegrationTests/CommandModeDestructiveCommandGapTests.swift @@ -2,77 +2,79 @@ 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. +/// Covers `CommandModeService.isDestructiveCommand`'s allowlist model: a command +/// requires confirmation unless every simple command in it resolves, after +/// dropping a leading env assignment, to a name on a short known-safe list, with +/// no redirect riding along. Anything not recognized asks for confirmation by +/// default, rather than trying to enumerate every way a command could be +/// dangerous. final class CommandModeDestructiveCommandGapTests: XCTestCase { - // MARK: - Regression: existing bare-command detection still works + // MARK: - Known-safe commands pass through - func testBareDestructiveCommandsAreStillCaught() { + func testKnownSafeCommandsAreNotFlagged() { 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) + "ls -la", + "cat README.md", + "echo hello world", + "pwd", + "whoami", + "date", + "hostname", + "uname -a", + "head -n 5 file.txt", + "tail -f log.txt", + "wc -l file.txt", + "file image.png", + "stat file.txt", + "which python3", + "printenv PATH", ] for command in cases { - XCTAssertTrue( + XCTAssertFalse( CommandModeService.isDestructiveCommand(command), - "expected \"\(command)\" to require confirmation despite the absolute path" + "expected \"\(command)\" NOT to require confirmation" ) } } - // MARK: - Fix 2: find -delete / find -exec rm + // MARK: - Everything not on the safe list requires confirmation, even when benign - func testFindDeleteAndExecRmAreCaught() { + /// This is the actual tradeoff of the allowlist model, stated as a test rather + /// than left implicit: ordinary, harmless commands not on the short safe list + /// now require confirmation too, since the model is "prove it's safe" rather + /// than "prove it's dangerous." + func testCommandsNotOnTheSafeListRequireConfirmation() { let cases = [ - "find ~/Documents -delete", - "find ~/Documents -type f -delete", - "find / -name '*.important' -exec rm {} \\;", + "cd /tmp", + "git status", + "find . -name '*.txt'", + "curl -s https://example.com", + "python3 script.py", + "diskutil list", + "npm install", + "mkdir newfolder", + "touch file.txt", ] for command in cases { XCTAssertTrue( CommandModeService.isDestructiveCommand(command), - "expected \"\(command)\" to require confirmation" + "expected \"\(command)\" to require confirmation, it isn't on the known-safe list" ) } } - // MARK: - Fix 3: diskutil destructive subcommands + // MARK: - Genuinely destructive commands still require confirmation - func testDiskutilDestructiveSubcommandsAreCaught() { + func testDestructiveCommandsAreCaught() { let cases = [ + "rm -rf ~/Documents", + "sudo reboot", + "mv secret.txt /tmp/", + "chmod 000 /etc/hosts", + "killall Finder", + "rmdir ~/Documents", + "dd if=/dev/zero of=/dev/disk0", "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( @@ -82,308 +84,125 @@ final class CommandModeDestructiveCommandGapTests: XCTestCase { } } - // MARK: - Fix 4: quoted absolute-path invocation + // MARK: - Path-qualified, quoted, and escaped forms still resolve correctly - func testQuotedAbsolutePathInvocationIsCaught() { - let cases = [ - "\"/bin/rm\" -rf ~/Documents", - "'/bin/rm' -rf ~/Documents", - "\"/usr/bin/sudo\" reboot", + func testAbsoluteAndQuotedPathsStillResolveToTheBareCommand() { + let safeCases = [ + "/bin/ls -la", + "\"/bin/cat\" README.md", ] - for command in cases { - XCTAssertTrue( - CommandModeService.isDestructiveCommand(command), - "expected \"\(command)\" to require confirmation despite the quoted absolute path" - ) - } - } - - // MARK: - Fix 5: diskutil subcommand matching doesn't false-positive on arguments - - func testDiskutilArgumentContainingSubcommandNameIsNotFlagged() { - let cases = [ - "diskutil info /Volumes/EraseDisk", - "diskutil list /Volumes/ReformatBackup", - ] - for command in cases { + for command in safeCases { XCTAssertFalse( CommandModeService.isDestructiveCommand(command), - "expected \"\(command)\" NOT to require confirmation -- the destructive-looking " - + "text is in an argument, not the diskutil subcommand" - ) - } - } - - // MARK: - Fix 6: diskutil's `quiet` modifier doesn't hide the destructive verb - - func testDiskutilQuietModifierStillCatchesDestructiveVerb() { - let cases = [ - "diskutil quiet eraseDisk JHFS+ Untitled disk0", - "diskutil quiet secureErase 0 /dev/disk0", - ] - for command in cases { - XCTAssertTrue( - CommandModeService.isDestructiveCommand(command), - "expected \"\(command)\" to require confirmation -- `quiet` precedes the verb, " - + "it isn't the verb" + "expected \"\(command)\" NOT to require confirmation, it resolves to a safe basename" ) } - } - - func testDiskutilQuietModifierAloneIsNotFlagged() { - XCTAssertFalse( - CommandModeService.isDestructiveCommand("diskutil quiet list"), - "expected a read-only verb after `quiet` NOT to require confirmation" - ) - } - // MARK: - Fix 7: format/mkfs.* basenames - - func testFormatAndMkfsVariantBasenamesAreCaught() { - let cases = [ - "/usr/local/bin/format /dev/disk2", - "/sbin/mkfs.ext4 /dev/sdb1", - "mkfs.vfat /dev/disk3", + let unsafeCases = [ + "/bin/rm -rf ~/Documents", + "\"/bin/rm\" -rf ~/Documents", + "/tmp/tools\\ dir/rm file", ] - for command in cases { + for command in unsafeCases { XCTAssertTrue( CommandModeService.isDestructiveCommand(command), - "expected \"\(command)\" to require confirmation" + "expected \"\(command)\" to require confirmation, it resolves to rm" ) } } - // MARK: - Fix 8: quoted path with an internal space + // MARK: - Accepted tradeoff: no escape-aware tokenizing, so an escaped + // whitespace character in the leading command's own path isn't specially + // handled. It fails toward confirmation, not toward silently allowing + // something, and it's a genuinely rare shape for a leading command to take. - func testQuotedPathWithInternalSpaceIsCaught() { + func testEscapedWhitespaceInLeadingCommandPathAsksForConfirmation() { XCTAssertTrue( - CommandModeService.isDestructiveCommand("\"/tmp/tools dir/rm\" file"), - "expected the quoted path to resolve to rm despite the internal space" + CommandModeService.isDestructiveCommand("/tmp/tools\\ dir/ls"), + "expected an escaped-space path to require confirmation rather than resolve to a safe basename" ) } - // MARK: - Fix 9: destructive command after a compound-command separator + // MARK: - A leading env assignment doesn't change the verdict - func testDestructiveCommandAfterSeparatorIsCaught() { - let cases = [ - "cd /tmp && /bin/rm victim", - "echo done; rm -rf ~/Documents", - "true || sudo reboot", - "find . -name '*.log' | xargs rm", - ] - for command in cases { - XCTAssertTrue( - CommandModeService.isDestructiveCommand(command), - "expected \"\(command)\" to require confirmation for the command after the separator" - ) - } - } - - func testBenignCommandsChainedWithSeparatorsAreNotFlagged() { - let cases = [ - "cd /tmp && ls -la", - "git status; git log", - "find . -name '*.log' | xargs cat", - ] - for command in cases { - XCTAssertFalse( - CommandModeService.isDestructiveCommand(command), - "expected \"\(command)\" NOT to require confirmation" - ) - } - } - - // MARK: - Fix 10: redirect anywhere in the command, not just as a leading prefix - - func testRedirectAnywhereInCommandIsCaught() { - XCTAssertTrue( - CommandModeService.isDestructiveCommand("echo malicious > /etc/hosts"), - "expected a mid-command redirect to require confirmation" + func testLeadingEnvironmentAssignmentIsSkippedBeforeChecking() { + XCTAssertFalse( + CommandModeService.isDestructiveCommand("LC_ALL=C ls -la"), + "expected an env-prefixed safe command NOT to require confirmation" ) - } - - // MARK: - Fix 11: path-qualified xargs target - - func testPathQualifiedXargsTargetIsCaught() { XCTAssertTrue( - CommandModeService.isDestructiveCommand("find . -print0 | xargs -0 /bin/rm"), - "expected the path-qualified xargs target to resolve to rm" + CommandModeService.isDestructiveCommand("LC_ALL=C rm -rf victim"), + "expected an env-prefixed destructive command to still require confirmation" ) } - // MARK: - Fix 12: bare redirect as the entire simple command + // MARK: - A redirect makes an otherwise-safe command unsafe - func testBareRedirectAsEntireCommandIsCaught() { + func testRedirectOnAnOtherwiseSafeCommandIsCaught() { let cases = [ + "echo malicious > /etc/hosts", + "cat file.txt > /etc/hosts", "> important.txt", - ">> important.txt", + "2>/dev/null ls", ] for command in cases { XCTAssertTrue( CommandModeService.isDestructiveCommand(command), - "expected \"\(command)\" to require confirmation" + "expected \"\(command)\" to require confirmation because of the redirect" ) } } - // MARK: - Fix 13: leading environment assignment before a destructive command - - func testLeadingEnvironmentAssignmentIsCaught() { - let cases = [ - "LC_ALL=C rm -rf victim", - "A=1 B=2 sudo reboot", - ] - for command in cases { - XCTAssertTrue( - CommandModeService.isDestructiveCommand(command), - "expected \"\(command)\" to require confirmation despite the leading assignment" - ) - } - } + // MARK: - Every simple command in a chain is checked, not just the first - func testLeadingEnvironmentAssignmentAloneIsNotFlagged() { + func testEverySimpleCommandInAChainIsChecked() { + let allSafe = "ls -la; cat README.md && echo done" XCTAssertFalse( - CommandModeService.isDestructiveCommand("LC_ALL=C ls -la"), - "expected a benign command after an assignment NOT to require confirmation" + CommandModeService.isDestructiveCommand(allSafe), + "expected a chain of only safe commands NOT to require confirmation" ) - } - - // MARK: - Fix 14: backslash-escaped whitespace in an executable path - - func testEscapedWhitespaceInPathIsCaught() { - let cases = [ - "/tmp/tools\\ dir/rm -rf victim", - "/tmp/tools\\ dir/sudo reboot", - ] - for command in cases { - XCTAssertTrue( - CommandModeService.isDestructiveCommand(command), - "expected \"\(command)\" to resolve past the escaped space to the real basename" - ) - } - } - - // MARK: - Fix 15: program passed as an argument to a runner resolves by basename - func testPathQualifiedProgramInRunnersIsCaught() { - let cases = [ - "find . -exec /bin/rm -rf {} \\;", - "find . -execdir /bin/rm {} \\;", - "env /bin/rm -rf victim", - "nohup /bin/rm -rf victim", - "time /bin/rm victim", - ] - for command in cases { - XCTAssertTrue( - CommandModeService.isDestructiveCommand(command), - "expected \"\(command)\" to resolve the argument program to its basename" - ) - } - } + let oneUnsafe = "ls -la && rm -rf victim" + XCTAssertTrue( + CommandModeService.isDestructiveCommand(oneUnsafe), + "expected a chain with one destructive command to require confirmation" + ) - func testBenignProgramsInRunnersAreNotFlagged() { - let cases = [ - "env /bin/ls -la", - "nohup /usr/bin/python3 script.py", - "time git status", - "find . -type f -exec /bin/cat {} \\;", - ] - for command in cases { - XCTAssertFalse( - CommandModeService.isDestructiveCommand(command), - "expected \"\(command)\" NOT to require confirmation" - ) - } + let oneUnrecognized = "ls -la && git status" + XCTAssertTrue( + CommandModeService.isDestructiveCommand(oneUnrecognized), + "expected a chain with one unrecognized command to require confirmation" + ) } - // MARK: - Fix 16: shell -c payload is parsed, not treated as an opaque argument + // MARK: - The bypass constructs raised across the original review are all closed, + // for the same reason: none of these leading commands are on the safe list. - func testShellWrapperPayloadIsParsed() { + func testPreviouslyReportedBypassConstructsAllRequireConfirmation() { let cases = [ + "\"/bin/rm\" -rf ~/Documents", + "diskutil quiet eraseDisk JHFS+ Untitled disk0", + "/usr/local/bin/format /dev/disk2", + "/sbin/mkfs.ext4 /dev/disk2", + "cd /tmp && /bin/rm victim", + "xargs -I{} /bin/rm {}", + "find . -exec /bin/rm {} \\;", "sh -c 'rm -rf victim'", - "bash -c \"rm -rf victim\"", - "zsh -c 'cd /tmp && /bin/rm victim'", - "sh -c 'sh -c \"rm -rf victim\"'", - ] - for command in cases { - XCTAssertTrue( - CommandModeService.isDestructiveCommand(command), - "expected the shell payload in \"\(command)\" to be parsed for destructive commands" - ) - } - } - - func testBenignShellWrapperPayloadIsNotFlagged() { - let cases = [ - "sh -c 'ls -la'", - "bash -c \"git status\"", - ] - for command in cases { - XCTAssertFalse( - CommandModeService.isDestructiveCommand(command), - "expected \"\(command)\" NOT to require confirmation" - ) - } - } - - // MARK: - Fix 17: a shell in argument position is a nested invocation, not a leaf - - func testNestedShellPayloadInArgumentPositionIsCaught() { - let cases = [ - "xargs sh -c 'rm -rf victim'", - "find . -exec sh -c 'rm -rf victim' \\;", - "find . -execdir bash -c 'rm -rf victim' \\;", "env sh -c 'rm -rf victim'", - "nohup zsh -c 'rm -rf victim'", - "timeout 5 sh -c 'rm -rf victim'", - "xargs -I{} /bin/sh -c 'rm -rf {}'", - "cd /tmp && xargs sh -c 'rm -rf victim'", + "eval 'rm -rf victim'", + "env diskutil eraseDisk JHFS+ Untitled disk0", + "nohup find . -delete", + "(rm -rf victim)", + "if true; then rm -rf victim; fi", + "echo $(rm -rf victim)", ] for command in cases { XCTAssertTrue( CommandModeService.isDestructiveCommand(command), - "expected the nested shell payload in \"\(command)\" to be parsed" - ) - } - } - - func testNestedBenignShellPayloadInArgumentPositionIsNotFlagged() { - let cases = [ - "xargs sh -c 'ls -la'", - "find . -exec sh -c 'cat {}' \\;", - "env bash -c 'git status'", - ] - for command in cases { - XCTAssertFalse( - CommandModeService.isDestructiveCommand(command), - "expected \"\(command)\" NOT to require confirmation" + "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" - ) - } - } } From 28bd4cc0b458d4409b5caf366290fbf352499590 Mon Sep 17 00:00:00 2001 From: manjunathbhaskar Date: Tue, 1 Sep 2026 20:52:18 +0200 Subject: [PATCH 09/10] fix(command-mode): only treat an unsafe character as unsafe outside quotes echo '>' and echo \> pass an ordinary quoted or escaped argument, not a redirect, so they shouldn't require confirmation. The character scan was a raw string search with no quote or escape awareness; replaced with a small quote-tracking scan that still catches every unquoted, unescaped case exactly as before. --- .../Fluid/Services/CommandModeService.swift | 25 ++++++++++++++++++- ...ommandModeDestructiveCommandGapTests.swift | 16 ++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/Sources/Fluid/Services/CommandModeService.swift b/Sources/Fluid/Services/CommandModeService.swift index 404e16284..3b9af6a14 100644 --- a/Sources/Fluid/Services/CommandModeService.swift +++ b/Sources/Fluid/Services/CommandModeService.swift @@ -598,6 +598,29 @@ final class CommandModeService: ObservableObject { /// shell parser. private static let unsafeCharacters = CharacterSet(charactersIn: "$`(){}<>") + /// True if an unsafe character appears outside quotes and isn't escaped. + /// `echo '>'` and `echo \>` pass an ordinary argument, not a redirect, and + /// shouldn't require confirmation just because the character is scary + /// elsewhere. + private nonisolated static func hasUnquotedUnsafeCharacter(_ simpleCommand: String) -> Bool { + var quote: Character? + var escaped = false + for c in simpleCommand { + if escaped { + escaped = false + } else if c == "\\", quote != "'" { + escaped = true + } else if let q = quote { + if c == q { quote = nil } + } else if c == "\"" || c == "'" { + quote = c + } else if c.unicodeScalars.contains(where: unsafeCharacters.contains) { + return true + } + } + return false + } + /// The leading word of a simple command, with a leading env assignment /// dropped (`LC_ALL=C ls` runs `ls`) and surrounding quotes stripped if the /// whole word is one quoted span. @@ -637,7 +660,7 @@ final class CommandModeService: ObservableObject { } private nonisolated static func isSafeSimpleCommand(_ simpleCommand: String) -> Bool { - guard simpleCommand.rangeOfCharacter(from: unsafeCharacters) == nil else { return false } + guard !hasUnquotedUnsafeCharacter(simpleCommand) else { return false } guard let name = leadingCommand(simpleCommand) else { return true } return knownSafeCommands.contains((name as NSString).lastPathComponent) } diff --git a/Tests/FluidDictationIntegrationTests/CommandModeDestructiveCommandGapTests.swift b/Tests/FluidDictationIntegrationTests/CommandModeDestructiveCommandGapTests.swift index c72919aec..52a641915 100644 --- a/Tests/FluidDictationIntegrationTests/CommandModeDestructiveCommandGapTests.swift +++ b/Tests/FluidDictationIntegrationTests/CommandModeDestructiveCommandGapTests.swift @@ -153,6 +153,22 @@ final class CommandModeDestructiveCommandGapTests: XCTestCase { } } + // MARK: - A quoted or escaped unsafe character is an argument, not a redirect + + func testQuotedOrEscapedUnsafeCharacterIsNotFlagged() { + let cases = [ + "echo '>'", + "echo \\>", + "echo \"(hello)\"", + ] + for command in cases { + XCTAssertFalse( + CommandModeService.isDestructiveCommand(command), + "expected \"\(command)\" NOT to require confirmation, the character is quoted or escaped" + ) + } + } + // MARK: - Every simple command in a chain is checked, not just the first func testEverySimpleCommandInAChainIsChecked() { From 1eb44437c7d188310af4da22257aa2e550262603 Mon Sep 17 00:00:00 2001 From: manjunathbhaskar Date: Tue, 1 Sep 2026 21:03:30 +0200 Subject: [PATCH 10/10] fix(command-mode): close three real bypasses in the allowlist check itself Codex and Greptile found real gaps in the allowlist model, not stale findings against removed code: - An escaped double quote (echo "foo\"bar" && rm -rf victim) was read as closing the string early, so the && after it never split, and the whole thing was judged safe from the leading echo alone. - Command substitution inside double quotes (echo "$(rm -rf victim)") was invisible to the unsafe-character check, which skipped scanning entirely for any active quote type, real shells still evaluate $(...) and backticks inside double quotes. - A path-qualified safe-looking command (./ls, /tmp/cat) was trusted purely by basename, even though it could be an arbitrary planted script sharing the name. Now only trusted for a bare name or a real system directory (/bin, /usr/bin, /sbin, /usr/sbin). All three are silent-execution bypasses, not extra-confirmation tradeoffs, so they needed fixing rather than documenting as accepted cost. Verified each one fails on the unpatched logic and passes after, with new regression tests, full suite still green. --- .../Fluid/Services/CommandModeService.swift | 60 ++++++++++++++---- ...ommandModeDestructiveCommandGapTests.swift | 62 +++++++++++++++++++ 2 files changed, 110 insertions(+), 12 deletions(-) diff --git a/Sources/Fluid/Services/CommandModeService.swift b/Sources/Fluid/Services/CommandModeService.swift index 3b9af6a14..6b79b45c0 100644 --- a/Sources/Fluid/Services/CommandModeService.swift +++ b/Sources/Fluid/Services/CommandModeService.swift @@ -563,14 +563,25 @@ final class CommandModeService: ObservableObject { /// `&&`, `||`, `;`, `&`, `|`, and newline, so every command in a chain gets /// checked, not just the first one before a separator. /// Splits on unquoted `;`, `&&`, `||`, `|`, newline. Quotes are tracked so a - /// separator inside a quoted argument doesn't split; escaped separators are - /// not handled, an over-split just means one more piece to check, which can - /// only ask for confirmation more often, never less. + /// separator inside a quoted argument doesn't split, and a backslash-escaped + /// quote doesn't close early, real shells honor `\"` inside a double-quoted + /// string as a literal character, not the end of the string. private nonisolated static func splitIntoSimpleCommands(_ command: String) -> [String] { var parts: [String] = [] var current = "" var quote: Character? + var escaped = false for c in command { + if escaped { + current.append(c) + escaped = false + continue + } + if c == "\\", quote != "'" { + current.append(c) + escaped = true + continue + } if let q = quote { current.append(c) if c == q { quote = nil } @@ -598,21 +609,38 @@ final class CommandModeService: ObservableObject { /// shell parser. private static let unsafeCharacters = CharacterSet(charactersIn: "$`(){}<>") - /// True if an unsafe character appears outside quotes and isn't escaped. - /// `echo '>'` and `echo \>` pass an ordinary argument, not a redirect, and - /// shouldn't require confirmation just because the character is scary - /// elsewhere. + /// `$` and backtick stay live for substitution inside double quotes; a real + /// shell still evaluates `"$(rm -rf x)"`. Everything else in + /// `unsafeCharacters` (redirects, braces, parens on their own) is inert + /// inside double quotes, same as any other literal character. + private static let unsafeInsideDoubleQuotes = CharacterSet(charactersIn: "$`") + + /// True if an unsafe character appears where it's actually live: unquoted, + /// or (for `$`/backtick specifically) inside double quotes. Single quotes + /// suppress everything, and an escaped character is a literal, not a + /// redirect or substitution, `echo '>'` and `echo \>` pass an ordinary + /// argument. private nonisolated static func hasUnquotedUnsafeCharacter(_ simpleCommand: String) -> Bool { var quote: Character? var escaped = false for c in simpleCommand { if escaped { escaped = false - } else if c == "\\", quote != "'" { + continue + } + if c == "\\", quote != "'" { escaped = true - } else if let q = quote { - if c == q { quote = nil } - } else if c == "\"" || c == "'" { + continue + } + if let q = quote { + if c == q { + quote = nil + } else if q == "\"", c.unicodeScalars.contains(where: unsafeInsideDoubleQuotes.contains) { + return true + } + continue + } + if c == "\"" || c == "'" { quote = c } else if c.unicodeScalars.contains(where: unsafeCharacters.contains) { return true @@ -655,6 +683,12 @@ final class CommandModeService: ObservableObject { "head", "tail", "wc", "file", "stat", "which", "type", "printenv", ] + /// A path-qualified command is only trusted if it also lives in one of + /// these directories. A bare name resolving to `ls` is the real system + /// `ls`; a path like `./ls` or `/tmp/cat` could be an arbitrary planted + /// script that merely shares the name. + private static let trustedDirectories: Set = ["/bin", "/usr/bin", "/sbin", "/usr/sbin"] + nonisolated static func isDestructiveCommand(_ command: String) -> Bool { splitIntoSimpleCommands(command.lowercased()).contains { !isSafeSimpleCommand($0) } } @@ -662,7 +696,9 @@ final class CommandModeService: ObservableObject { private nonisolated static func isSafeSimpleCommand(_ simpleCommand: String) -> Bool { guard !hasUnquotedUnsafeCharacter(simpleCommand) else { return false } guard let name = leadingCommand(simpleCommand) else { return true } - return knownSafeCommands.contains((name as NSString).lastPathComponent) + guard knownSafeCommands.contains((name as NSString).lastPathComponent) else { return false } + guard name.contains("/") else { return true } + return trustedDirectories.contains((name as NSString).deletingLastPathComponent) } diff --git a/Tests/FluidDictationIntegrationTests/CommandModeDestructiveCommandGapTests.swift b/Tests/FluidDictationIntegrationTests/CommandModeDestructiveCommandGapTests.swift index 52a641915..92ef9f661 100644 --- a/Tests/FluidDictationIntegrationTests/CommandModeDestructiveCommandGapTests.swift +++ b/Tests/FluidDictationIntegrationTests/CommandModeDestructiveCommandGapTests.swift @@ -169,6 +169,67 @@ final class CommandModeDestructiveCommandGapTests: XCTestCase { } } + // MARK: - A backslash-escaped quote doesn't hide a real separator + + func testEscapedQuoteDoesNotHideSeparator() { + XCTAssertTrue( + CommandModeService.isDestructiveCommand("echo \"foo\\\"bar\" && rm -rf victim"), + "expected the destructive command after && to require confirmation, the escaped quote shouldn't close the string early" + ) + } + + func testEscapedQuoteInsideAnOtherwiseSafeCommandIsNotFlagged() { + XCTAssertFalse( + CommandModeService.isDestructiveCommand("echo \"foo\\\"bar\""), + "expected a genuinely safe command with an escaped quote NOT to require confirmation" + ) + } + + // MARK: - Command substitution inside double quotes is still substitution + + func testCommandSubstitutionInsideDoubleQuotesIsCaught() { + XCTAssertTrue( + CommandModeService.isDestructiveCommand("echo \"$(rm -rf victim)\""), + "expected a double-quoted command substitution to require confirmation, a real shell still evaluates it" + ) + } + + func testSingleQuotedSubstitutionSyntaxIsNotFlagged() { + XCTAssertFalse( + CommandModeService.isDestructiveCommand("echo '$(rm -rf victim)'"), + "expected single-quoted text NOT to require confirmation, single quotes suppress all substitution" + ) + } + + func testParenthesesInsideDoubleQuotesAreStillInert() { + XCTAssertFalse( + CommandModeService.isDestructiveCommand("echo \"(hello)\""), + "expected literal parentheses inside double quotes NOT to require confirmation, they aren't substitution on their own" + ) + } + + // MARK: - A path-qualified safe command is only trusted from a real system directory + + func testPathQualifiedSafeCommandOutsideATrustedDirectoryIsFlagged() { + let cases = ["./ls", "/tmp/cat -la", "ls_backup/ls"] + for command in cases { + XCTAssertTrue( + CommandModeService.isDestructiveCommand(command), + "expected \"\(command)\" to require confirmation, it isn't in a trusted system directory" + ) + } + } + + func testPathQualifiedSafeCommandInATrustedDirectoryIsNotFlagged() { + let cases = ["/bin/ls -la", "/usr/bin/cat README.md"] + for command in cases { + XCTAssertFalse( + CommandModeService.isDestructiveCommand(command), + "expected \"\(command)\" NOT to require confirmation, it's a real system path" + ) + } + } + // MARK: - Every simple command in a chain is checked, not just the first func testEverySimpleCommandInAChainIsChecked() { @@ -221,4 +282,5 @@ final class CommandModeDestructiveCommandGapTests: XCTestCase { } + }