From 7f436b9e346e00f6332c933ba5e45a6af327522a Mon Sep 17 00:00:00 2001 From: boris Date: Thu, 10 Sep 2026 15:55:11 +0300 Subject: [PATCH 1/2] Fix #143: ask for the Maintenance admin password once Each admin task spawned a new osascript process, so macOS could not cache credentials. Run do shell script in-process via NSAppleScript. --- .../Maintenance/MaintenanceModule.swift | 68 ++++---- .../Maintenance/PrivilegedShellRunner.swift | 60 +++++++ .../Views/Performance/MaintenanceView.swift | 6 +- Sources/MacCleanKit/MaintenanceShell.swift | 31 +++- .../MaintenanceShellTests.swift | 40 +++++ .../MaintenanceExecutorTests.swift | 160 ++++++++++++++++++ 6 files changed, 328 insertions(+), 37 deletions(-) create mode 100644 Sources/MacClean/Modules/Maintenance/PrivilegedShellRunner.swift create mode 100644 Tests/MacCleanTests/MaintenanceExecutorTests.swift diff --git a/Sources/MacClean/Modules/Maintenance/MaintenanceModule.swift b/Sources/MacClean/Modules/Maintenance/MaintenanceModule.swift index 5bf6c97..44327a5 100644 --- a/Sources/MacClean/Modules/Maintenance/MaintenanceModule.swift +++ b/Sources/MacClean/Modules/Maintenance/MaintenanceModule.swift @@ -14,7 +14,8 @@ public struct MaintenanceModule: ScanModule { // MARK: - Maintenance Executor // // `MaintenanceTask` (the enum + descriptions + system commands) lives in -// MacCleanKit. This actor wraps `Process` to actually run the commands. +// MacCleanKit. This actor runs those commands: admin ones through in-process +// AppleScript (password cached per process, issue #143), the rest via Process. public actor MaintenanceExecutor { public struct TaskResult: Sendable { @@ -24,7 +25,23 @@ public actor MaintenanceExecutor { public let error: String? } - public init() {} + private let privilegedRunner: any PrivilegedShellRunning + private let commandExists: @Sendable (String) -> Bool + + public init() { + self.init( + privilegedRunner: AppleScriptPrivilegedRunner(), + commandExists: { FileManager.default.isExecutableFile(atPath: $0) } + ) + } + + init( + privilegedRunner: any PrivilegedShellRunning, + commandExists: @escaping @Sendable (String) -> Bool + ) { + self.privilegedRunner = privilegedRunner + self.commandExists = commandExists + } public func execute(_ task: MaintenanceTask) async -> TaskResult { if case .speedUpMail = task { return await reindexMail() } @@ -44,9 +61,7 @@ public actor MaintenanceExecutor { // `systemCommand`, the same way `pruneDocker` already gates on the // Docker CLI. Checked unprivileged, so an admin task fails before the // password prompt rather than after it. - guard task.systemCommandIsAvailable(existing: { - FileManager.default.isExecutableFile(atPath: $0) - }) else { + guard task.systemCommandIsAvailable(existing: commandExists) else { return TaskResult( task: task, success: false, output: "", error: L10n.tr("\(command) 在当前 macOS 版本中不可用，无法执行该任务。", @@ -61,35 +76,28 @@ public actor MaintenanceExecutor { } /// Run a root-requiring command via the standard macOS admin-auth prompt - /// (`do shell script … with administrator privileges`). macOS shows its - /// native password dialog and runs the command as root — no persistent - /// privileged helper needed. The command strings come from the fixed - /// `MaintenanceTask` enum (never user input); we still escape the - /// AppleScript string literal defensively. + /// (`do shell script … with administrator privileges`). Executed + /// in-process so successive tasks reuse the cached credentials (issue + /// #143) — spawning `/usr/bin/osascript` each time could not. The + /// command strings come from the fixed `MaintenanceTask` enum (never + /// user input). private func runAdminProcess(task: MaintenanceTask, command: String, args: [String]) async -> TaskResult { - // Each argv element is single-quoted (MaintenanceShell.quote) so sh - // can't re-split or interpret it; the assembled command is then - // escaped as an AppleScript string literal for `do shell script`. let shell = MaintenanceShell.commandLine(command, args) - let escaped = shell - .replacingOccurrences(of: "\\", with: "\\\\") - .replacingOccurrences(of: "\"", with: "\\\"") - let script = "do shell script \"\(escaped)\" with administrator privileges" - let result = await runProcess(task: task, command: "/usr/bin/osascript", args: ["-e", script]) - - // osascript returns -128 / "User canceled." when the user dismisses - // the auth dialog — surface that as a friendly note, not an error. - if !result.success, let err = result.error { - if err.contains("User canceled") || err.contains("-128") { - return TaskResult(task: task, success: false, output: "", - error: L10n.tr("已取消——未授予管理员权限。", "Cancelled — administrator access was not granted.", "Отменено: не предоставлены права администратора.")) - } - // Otherwise strip osascript's "1:92: execution error: … (1)" wrapper - // so the user sees the real underlying message (issue #82). + let result = await privilegedRunner.run(commandLine: shell) + + if result.success { + return TaskResult(task: task, success: true, output: result.output, error: nil) + } + + let err = result.error ?? "" + if MaintenanceShell.isAuthorizationCancelled(err, errorNumber: result.errorNumber) { return TaskResult(task: task, success: false, output: "", - error: MaintenanceShell.humanReadableError(err)) + error: L10n.tr("已取消——未授予管理员权限。", "Cancelled — administrator access was not granted.", "Отменено: не предоставлены права администратора.")) } - return result + // Strip AppleScript's "1:92: execution error: … (1)" wrapper so the + // user sees the real underlying message (issue #82). + return TaskResult(task: task, success: false, output: "", + error: MaintenanceShell.humanReadableError(err)) } private func runProcess(task: MaintenanceTask, command: String, args: [String]) async -> TaskResult { diff --git a/Sources/MacClean/Modules/Maintenance/PrivilegedShellRunner.swift b/Sources/MacClean/Modules/Maintenance/PrivilegedShellRunner.swift new file mode 100644 index 0000000..4975f62 --- /dev/null +++ b/Sources/MacClean/Modules/Maintenance/PrivilegedShellRunner.swift @@ -0,0 +1,60 @@ +import AppKit +import Foundation +import MacCleanKit + +/// Result of one privileged `do shell script` invocation. +struct PrivilegedShellResult: Sendable, Equatable { + let success: Bool + let output: String + let error: String? + let errorNumber: Int? + + static func ok(_ output: String) -> PrivilegedShellResult { + PrivilegedShellResult(success: true, output: output, error: nil, errorNumber: nil) + } + + static func failed(_ error: String, errorNumber: Int? = nil) -> PrivilegedShellResult { + PrivilegedShellResult(success: false, output: "", error: error, errorNumber: errorNumber) + } +} + +/// Runs a pre-quoted shell command line with administrator privileges. +protocol PrivilegedShellRunning: Sendable { + func run(commandLine: String) async -> PrivilegedShellResult +} + +/// In-process AppleScript runner. macOS caches the admin password for about +/// five minutes **per process**, so sequential Maintenance tasks share one +/// prompt instead of asking again for every task (issue #143). +/// +/// Calls are serialized on a dedicated queue: `NSAppleScript` is not thread +/// safe, and `executeAndReturnError` can block for minutes (`periodic`) so +/// it must not sit on the Swift concurrency thread pool. +struct AppleScriptPrivilegedRunner: PrivilegedShellRunning { + private static let queue = DispatchQueue(label: "sai.maintenance.privileged-applescript") + + func run(commandLine: String) async -> PrivilegedShellResult { + let source = MaintenanceShell.appleScriptSource(commandLine: commandLine) + return await withCheckedContinuation { continuation in + Self.queue.async { + continuation.resume(returning: Self.execute(source)) + } + } + } + + private static func execute(_ source: String) -> PrivilegedShellResult { + guard let script = NSAppleScript(source: source) else { + return .failed("Failed to create AppleScript") + } + var error: NSDictionary? + let descriptor = script.executeAndReturnError(&error) + if let error { + let message = (error[NSAppleScript.errorMessage] as? String) + ?? "AppleScript failed" + let number = (error[NSAppleScript.errorNumber] as? NSNumber)?.intValue + ?? error[NSAppleScript.errorNumber] as? Int + return .failed(message, errorNumber: number) + } + return .ok(descriptor.stringValue ?? "") + } +} diff --git a/Sources/MacClean/Views/Performance/MaintenanceView.swift b/Sources/MacClean/Views/Performance/MaintenanceView.swift index bc682d7..cbb0cd0 100644 --- a/Sources/MacClean/Views/Performance/MaintenanceView.swift +++ b/Sources/MacClean/Views/Performance/MaintenanceView.swift @@ -182,9 +182,9 @@ struct MaintenanceView: View { } /// Bulk button runs ONLY safe tasks, and runs them SEQUENTIALLY. Several - /// safe tasks need admin (purge, periodic); firing them in parallel popped - /// multiple macOS password dialogs at once (issue #82). Awaiting each in - /// turn means at most one auth prompt is on screen at a time. + /// safe tasks need admin (purge, periodic). Sequential order keeps the + /// UI status per-task; the in-process AppleScript runner then reuses the + /// cached admin password so the user types it once (issues #82 / #143). private func runSafeTasks() { let safeTasks = MaintenanceTask.allCases.filter { $0.severity == .safe } Task { diff --git a/Sources/MacCleanKit/MaintenanceShell.swift b/Sources/MacCleanKit/MaintenanceShell.swift index 67da3e8..9a93094 100644 --- a/Sources/MacCleanKit/MaintenanceShell.swift +++ b/Sources/MacCleanKit/MaintenanceShell.swift @@ -1,10 +1,11 @@ import Foundation /// POSIX shell quoting for assembling the maintenance admin command that -/// `osascript`'s `do shell script` hands to `/bin/sh`. Wrapping each -/// argument in single quotes neutralises every shell metacharacter; the -/// only character that can't appear literally inside single quotes is the -/// single quote itself, handled with the standard close-escape-reopen idiom. +/// in-process `NSAppleScript` (`do shell script`) hands to `/bin/sh`. +/// Wrapping each argument in single quotes neutralises every shell +/// metacharacter; the only character that can't appear literally inside +/// single quotes is the single quote itself, handled with the standard +/// close-escape-reopen idiom. public enum MaintenanceShell { public static func quote(_ argument: String) -> String { "'" + argument.replacingOccurrences(of: "'", with: "'\\''") + "'" @@ -15,6 +16,28 @@ public enum MaintenanceShell { ([executable] + arguments).map(quote).joined(separator: " ") } + /// AppleScript source that runs `commandLine` as root via the standard + /// macOS admin-auth dialog. Built as a pure string so quoting is + /// unit-testable without executing anything. + /// + /// The runner executes this **in-process** (`NSAppleScript`) rather than + /// spawning `/usr/bin/osascript`. A new osascript process cannot reuse + /// macOS's ~5-minute authorization cache, so every maintenance task + /// prompted for a password (issue #143). + public static func appleScriptSource(commandLine: String) -> String { + let escaped = commandLine + .replacingOccurrences(of: "\\", with: "\\\\") + .replacingOccurrences(of: "\"", with: "\\\"") + return "do shell script \"\(escaped)\" with administrator privileges" + } + + /// True when the user dismissed the admin-auth dialog. AppleScript uses + /// error number `-128` / the text `User canceled.` for that path. + public static func isAuthorizationCancelled(_ message: String, errorNumber: Int? = nil) -> Bool { + if errorNumber == -128 { return true } + return message.contains("User canceled") || message.contains("-128") + } + /// Turn an osascript `do shell script` failure into the underlying message. /// /// osascript surfaces failures as `":: execution error: diff --git a/Tests/MacCleanKitTests/MaintenanceShellTests.swift b/Tests/MacCleanKitTests/MaintenanceShellTests.swift index 7ee0fbf..de543c4 100644 --- a/Tests/MacCleanKitTests/MaintenanceShellTests.swift +++ b/Tests/MacCleanKitTests/MaintenanceShellTests.swift @@ -25,4 +25,44 @@ final class MaintenanceShellTests: XCTestCase { let line = MaintenanceShell.commandLine("/usr/sbin/periodic", ["daily", "weekly"]) XCTAssertEqual(line, "'/usr/sbin/periodic' 'daily' 'weekly'") } + + // MARK: - AppleScript source (issue #143) + + /// Admin tasks run `do shell script` in-process so macOS can cache the + /// password (~5 min, per process). The source string is the contract + /// between MaintenanceShell (quoting) and the NSAppleScript runner. + func testAppleScriptSourceWrapsQuotedCommandLine() { + let line = MaintenanceShell.commandLine("/usr/sbin/purge", []) + XCTAssertEqual( + MaintenanceShell.appleScriptSource(commandLine: line), + "do shell script \"'/usr/sbin/purge'\" with administrator privileges" + ) + } + + func testAppleScriptSourceEscapesQuotesAndBackslashes() { + // POSIX quoting keeps the double-quote literal; AppleScript then + // needs it escaped so the string literal stays valid. + let line = MaintenanceShell.commandLine(#"/tmp/foo"bar"#, []) + XCTAssertEqual( + MaintenanceShell.appleScriptSource(commandLine: line), + #"do shell script "'/tmp/foo\"bar'" with administrator privileges"# + ) + + let withSlash = MaintenanceShell.commandLine(#"/tmp/foo\bar"#, []) + XCTAssertEqual( + MaintenanceShell.appleScriptSource(commandLine: withSlash), + #"do shell script "'/tmp/foo\\bar'" with administrator privileges"# + ) + } + + func testAuthorizationCancelDetectedByErrorNumber() { + XCTAssertTrue(MaintenanceShell.isAuthorizationCancelled("", errorNumber: -128)) + XCTAssertFalse(MaintenanceShell.isAuthorizationCancelled("disk full", errorNumber: 1)) + } + + func testAuthorizationCancelDetectedByMessage() { + XCTAssertTrue(MaintenanceShell.isAuthorizationCancelled("User canceled.")) + XCTAssertTrue(MaintenanceShell.isAuthorizationCancelled("1:92: execution error: User canceled. (-128)")) + XCTAssertFalse(MaintenanceShell.isAuthorizationCancelled("Operation not permitted")) + } } diff --git a/Tests/MacCleanTests/MaintenanceExecutorTests.swift b/Tests/MacCleanTests/MaintenanceExecutorTests.swift new file mode 100644 index 0000000..d2ab919 --- /dev/null +++ b/Tests/MacCleanTests/MaintenanceExecutorTests.swift @@ -0,0 +1,160 @@ +import AppKit +import XCTest +@testable import MacClean +@testable import MacCleanKit + +/// Issue #143: each admin task used to spawn a fresh `/usr/bin/osascript` +/// process, so macOS could not cache the password. The executor must send +/// every privileged command through one injected in-process runner. +final class MaintenanceExecutorTests: EnglishAppLanguageTestCase { + + func testAdminTaskGoesThroughPrivilegedRunner() async { + let runner = RecordingPrivilegedRunner( + result: .ok("purged") + ) + let executor = MaintenanceExecutor( + privilegedRunner: runner, + commandExists: { _ in true } + ) + + let result = await executor.execute(.freeUpRAM) + + XCTAssertTrue(result.success) + XCTAssertEqual(result.output, "purged") + XCTAssertEqual(runner.commandLines, [ + MaintenanceShell.commandLine("/usr/sbin/purge", []) + ]) + } + + func testSequentialAdminTasksReuseTheSameRunner() async { + let runner = RecordingPrivilegedRunner(result: .ok("")) + let executor = MaintenanceExecutor( + privilegedRunner: runner, + commandExists: { _ in true } + ) + + _ = await executor.execute(.freeUpRAM) + _ = await executor.execute(.freeUpPurgeableSpace) + _ = await executor.execute(.runMaintenanceScripts) + + XCTAssertEqual(runner.commandLines, [ + MaintenanceShell.commandLine("/usr/sbin/purge", []), + MaintenanceShell.commandLine("/usr/bin/tmutil", ["thinlocalsnapshots", "/", "999999999999", "1"]), + MaintenanceShell.commandLine("/usr/sbin/periodic", ["daily", "weekly", "monthly"]), + ]) + } + + func testUnprivilegedTaskDoesNotTouchPrivilegedRunner() async { + let runner = RecordingPrivilegedRunner(result: .ok("unused")) + let executor = MaintenanceExecutor( + privilegedRunner: runner, + commandExists: { _ in true } + ) + + _ = await executor.execute(.flushDNSCache) + + XCTAssertTrue( + runner.commandLines.isEmpty, + "non-admin tasks must not trigger the password prompt" + ) + } + + func testMissingBinaryDoesNotPromptForAdmin() async { + let runner = RecordingPrivilegedRunner(result: .ok("should not run")) + let executor = MaintenanceExecutor( + privilegedRunner: runner, + commandExists: { _ in false } + ) + + let result = await executor.execute(.runMaintenanceScripts) + + XCTAssertFalse(result.success) + XCTAssertTrue(runner.commandLines.isEmpty, "password prompt must not appear for a missing tool") + XCTAssertEqual( + result.error, + "/usr/sbin/periodic isn't available on this version of macOS, so this task can't run." + ) + } + + func testUserCancelIsMappedToFriendlyMessage() async { + let runner = RecordingPrivilegedRunner( + result: .failed("User canceled.", errorNumber: -128) + ) + let executor = MaintenanceExecutor( + privilegedRunner: runner, + commandExists: { _ in true } + ) + + let result = await executor.execute(.freeUpRAM) + + XCTAssertFalse(result.success) + XCTAssertEqual(result.error, "Cancelled — administrator access was not granted.") + } + + func testAdminFailureStripsAppleScriptWrapper() async { + let runner = RecordingPrivilegedRunner( + result: .failed("1:92: execution error: Operation not permitted (1)") + ) + let executor = MaintenanceExecutor( + privilegedRunner: runner, + commandExists: { _ in true } + ) + + let result = await executor.execute(.freeUpRAM) + + XCTAssertFalse(result.success) + XCTAssertEqual(result.error, "Operation not permitted") + } + + /// `NSAppleScript` is documented as main-thread-only. The production + /// runner uses a dedicated serial queue so long `periodic` jobs don't + /// freeze the UI. This proves `do shell script` (no admin) still works + /// on that kind of queue — same API the runner uses. + func testDoShellScriptWorksOffMainThread() async { + let result: (String?, NSDictionary?) = await withCheckedContinuation { continuation in + DispatchQueue(label: "sai.test.applescript").async { + var error: NSDictionary? + let script = NSAppleScript(source: "do shell script \"echo ok\"") + let descriptor = script?.executeAndReturnError(&error) + continuation.resume(returning: (descriptor?.stringValue, error)) + } + } + XCTAssertNil(result.1, "off-main do shell script failed: \(result.1 ?? [:])") + XCTAssertEqual(result.0, "ok") + } + + func testGeneratedAdminScriptsCompile() { + let commands = MaintenanceTask.allCases.compactMap { task -> String? in + guard task.requiresAdmin, let command = task.systemCommand else { return nil } + return MaintenanceShell.commandLine(command.executable, command.arguments) + } + XCTAssertEqual(commands.count, 5, "every admin task with a systemCommand should compile") + for command in commands { + let source = MaintenanceShell.appleScriptSource(commandLine: command) + var error: NSDictionary? + let script = NSAppleScript(source: source) + XCTAssertNotNil(script, source) + XCTAssertTrue( + script?.compileAndReturnError(&error) == true, + "compile failed for \(command): \(error ?? [:])" + ) + } + } +} + +/// Records every privileged command line. `@unchecked Sendable` because the +/// executor is an actor and tests await it sequentially — no concurrent +/// mutation. +final class RecordingPrivilegedRunner: PrivilegedShellRunning, @unchecked Sendable { + private(set) var commandLines: [String] = [] + var result: PrivilegedShellResult + + init(result: PrivilegedShellResult) { + self.result = result + } + + func run(commandLine: String) async -> PrivilegedShellResult { + commandLines.append(commandLine) + return result + } +} From 90d6856538d43a67a7f03e53196971f7db546a56 Mon Sep 17 00:00:00 2001 From: Iliya Date: Wed, 16 Sep 2026 13:47:40 -0400 Subject: [PATCH 2/2] Bump to 1.19.2 for the single-password Maintenance fix --- Sources/MacCleanKit/Constants.swift | 2 +- VERSION | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Sources/MacCleanKit/Constants.swift b/Sources/MacCleanKit/Constants.swift index 5a06cc7..4ede2c3 100644 --- a/Sources/MacCleanKit/Constants.swift +++ b/Sources/MacCleanKit/Constants.swift @@ -223,5 +223,5 @@ public enum MCConstants { // plugin was tried (commit history) but doesn't work under multi-arch // `swift build --arch arm64 --arch x86_64` because xcbuild doesn't // execute plugins. - public static let appVersion = "1.19.1" + public static let appVersion = "1.19.2" } diff --git a/VERSION b/VERSION index 66e2ae6..836ae4e 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.19.1 +1.19.2