diff --git a/Fluid.xcodeproj/project.pbxproj b/Fluid.xcodeproj/project.pbxproj index 3cbb2afc8..f5890542b 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 */; }; C0DE89000000000000000002 /* SettingsNavigationStateTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = C0DE89000000000000000001 /* SettingsNavigationStateTests.swift */; }; 7CDB0A2D2F3C4D5600FB7CAD /* DictationE2ETests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7CDB0A292F3C4D5600FB7CAD /* DictationE2ETests.swift */; }; B52000000000000000000002 /* PrivateAIDictationTokenBudgetTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = B52000000000000000000001 /* PrivateAIDictationTokenBudgetTests.swift */; }; @@ -78,6 +79,7 @@ C0DE63700000000000000001 /* WhisperLanguageSelectionTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = WhisperLanguageSelectionTests.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 = ""; }; C0DE89000000000000000001 /* SettingsNavigationStateTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SettingsNavigationStateTests.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 = ""; }; @@ -157,6 +159,7 @@ 7CDB0A292F3C4D5600FB7CAD /* DictationE2ETests.swift */, CD1C7A0000000000000000B1 /* CustomDictionaryManualEntryTests.swift */, 7C91B0022F42AA0100C0DEF0 /* HotkeyShortcutTests.swift */, + 96B2C564157D276092AC9C7D /* CommandModeDestructiveCommandGapTests.swift */, C0DE89000000000000000001 /* SettingsNavigationStateTests.swift */, 343B29013F4441D6A797D12D /* LLMClientRequestBodyTests.swift */, 980330F3CE464336ADCE3E23 /* TemperatureSupportTests.swift */, @@ -333,6 +336,7 @@ B52000000000000000000002 /* PrivateAIDictationTokenBudgetTests.swift in Sources */, CD1C7A0000000000000000B2 /* CustomDictionaryManualEntryTests.swift in Sources */, 7C91B0012F42AA0100C0DEF0 /* HotkeyShortcutTests.swift in Sources */, + 1A6BEFDDF86DAE98C18BC3BF /* CommandModeDestructiveCommandGapTests.swift in Sources */, C0DE89000000000000000002 /* SettingsNavigationStateTests.swift in Sources */, 86CAA2D4EF18433096185602 /* LLMClientRequestBodyTests.swift in Sources */, 272BFB5CB271489892CAE50C /* TemperatureSupportTests.swift in Sources */, diff --git a/Sources/Fluid/Services/CommandModeService.swift b/Sources/Fluid/Services/CommandModeService.swift index 5e6248943..6b79b45c0 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,48 +559,149 @@ final class CommandModeService: ObservableObject { } } - private 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 - ] - - // Check if command starts with any destructive prefix - if destructivePrefixes.contains(where: { cmd.hasPrefix($0) }) { - return true + /// 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, 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 } + continue + } + if c == "\"" || c == "'" { + quote = c + current.append(c) + } else if ";&|\n".contains(c) { + parts.append(current) + current = "" + } else { + current.append(c) + } } + parts.append(current) + return parts.map { $0.trimmingCharacters(in: .whitespaces) }.filter { !$0.isEmpty } + } - // Check for destructive patterns anywhere in piped commands - let destructivePatterns = [ - "| rm ", "| sudo ", "| dd ", - "; rm ", "; sudo ", - "&& rm ", "&& sudo ", - "xargs rm", "xargs -I", - ] - - if destructivePatterns.contains(where: { cmd.contains($0) }) { - return 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: "$`(){}<>") + + /// `$` 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 + continue + } + if c == "\\", quote != "'" { + escaped = true + 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 + } } + return false + } - // rm with flags like -rf, -r, -f anywhere - if cmd.contains("rm -") { - return true + /// 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() + } + 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 word + } - return false + /// `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.. = [ + "ls", "cat", "echo", "pwd", "whoami", "date", "hostname", "uname", + "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) } } + private nonisolated static func isSafeSimpleCommand(_ simpleCommand: String) -> Bool { + guard !hasUnquotedUnsafeCharacter(simpleCommand) else { return false } + guard let name = leadingCommand(simpleCommand) else { return true } + guard knownSafeCommands.contains((name as NSString).lastPathComponent) else { return false } + guard name.contains("/") else { return true } + return trustedDirectories.contains((name as NSString).deletingLastPathComponent) + } + + 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 new file mode 100644 index 000000000..92ef9f661 --- /dev/null +++ b/Tests/FluidDictationIntegrationTests/CommandModeDestructiveCommandGapTests.swift @@ -0,0 +1,286 @@ +@testable import FluidVoice_Debug +import Foundation +import XCTest + +/// 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: - Known-safe commands pass through + + func testKnownSafeCommandsAreNotFlagged() { + let cases = [ + "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 { + XCTAssertFalse( + CommandModeService.isDestructiveCommand(command), + "expected \"\(command)\" NOT to require confirmation" + ) + } + } + + // MARK: - Everything not on the safe list requires confirmation, even when benign + + /// 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 = [ + "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, it isn't on the known-safe list" + ) + } + } + + // MARK: - Genuinely destructive commands still require confirmation + + 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", + ] + for command in cases { + XCTAssertTrue( + CommandModeService.isDestructiveCommand(command), + "expected \"\(command)\" to require confirmation" + ) + } + } + + // MARK: - Path-qualified, quoted, and escaped forms still resolve correctly + + func testAbsoluteAndQuotedPathsStillResolveToTheBareCommand() { + let safeCases = [ + "/bin/ls -la", + "\"/bin/cat\" README.md", + ] + for command in safeCases { + XCTAssertFalse( + CommandModeService.isDestructiveCommand(command), + "expected \"\(command)\" NOT to require confirmation, it resolves to a safe basename" + ) + } + + let unsafeCases = [ + "/bin/rm -rf ~/Documents", + "\"/bin/rm\" -rf ~/Documents", + "/tmp/tools\\ dir/rm file", + ] + for command in unsafeCases { + XCTAssertTrue( + CommandModeService.isDestructiveCommand(command), + "expected \"\(command)\" to require confirmation, it resolves to rm" + ) + } + } + + // 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 testEscapedWhitespaceInLeadingCommandPathAsksForConfirmation() { + XCTAssertTrue( + CommandModeService.isDestructiveCommand("/tmp/tools\\ dir/ls"), + "expected an escaped-space path to require confirmation rather than resolve to a safe basename" + ) + } + + // MARK: - A leading env assignment doesn't change the verdict + + func testLeadingEnvironmentAssignmentIsSkippedBeforeChecking() { + XCTAssertFalse( + CommandModeService.isDestructiveCommand("LC_ALL=C ls -la"), + "expected an env-prefixed safe command NOT to require confirmation" + ) + XCTAssertTrue( + CommandModeService.isDestructiveCommand("LC_ALL=C rm -rf victim"), + "expected an env-prefixed destructive command to still require confirmation" + ) + } + + // MARK: - A redirect makes an otherwise-safe command unsafe + + func testRedirectOnAnOtherwiseSafeCommandIsCaught() { + let cases = [ + "echo malicious > /etc/hosts", + "cat file.txt > /etc/hosts", + "> important.txt", + "2>/dev/null ls", + ] + for command in cases { + XCTAssertTrue( + CommandModeService.isDestructiveCommand(command), + "expected \"\(command)\" to require confirmation because of the redirect" + ) + } + } + + // 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: - 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() { + let allSafe = "ls -la; cat README.md && echo done" + XCTAssertFalse( + CommandModeService.isDestructiveCommand(allSafe), + "expected a chain of only safe commands NOT to require confirmation" + ) + + let oneUnsafe = "ls -la && rm -rf victim" + XCTAssertTrue( + CommandModeService.isDestructiveCommand(oneUnsafe), + "expected a chain with one destructive command to require confirmation" + ) + + let oneUnrecognized = "ls -la && git status" + XCTAssertTrue( + CommandModeService.isDestructiveCommand(oneUnrecognized), + "expected a chain with one unrecognized command to require confirmation" + ) + } + + // 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 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'", + "env 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 \"\(command)\" to require confirmation" + ) + } + } + + + +}