diff --git a/CHANGELOG.md b/CHANGELOG.md index 3944d16251..f8c7a30c3c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,6 +42,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Autocomplete icon colors ignoring the theme. - Theme color wells and the theme preview unreadable to VoiceOver. +### Security + +- Prompts from MCP clients, pairing and query links taking focus from other apps, and Return approving them. + ## [0.80.0] - 2026-10-11 Google Cloud IAM sign-in for MySQL and PostgreSQL on Cloud SQL, with no Cloud SQL Auth Proxy. diff --git a/TablePro/Core/MCP/MCPApprovalPresenter.swift b/TablePro/Core/MCP/MCPApprovalPresenter.swift index 744e352dc6..f3f4bc6529 100644 --- a/TablePro/Core/MCP/MCPApprovalPresenter.swift +++ b/TablePro/Core/MCP/MCPApprovalPresenter.swift @@ -30,7 +30,7 @@ protocol MCPApprovalPresenting: Sendable { /// so that stepping away costs at most the one call. struct MCPApprovalAlertPresenter: MCPApprovalPresenting { func requestApproval(_ request: MCPApprovalRequest) async -> Bool { - await AlertHelper.runApprovalModal( + await AlertHelper.runCrossProcessApproval( title: String(localized: "MCP Access Request"), message: Self.message(for: request), confirm: String(localized: "Allow"), @@ -38,8 +38,12 @@ struct MCPApprovalAlertPresenter: MCPApprovalPresenting { ) } + static let maximumClientLabelLength = 80 + + /// A paired client names itself, at any length, and the connection it wants comes after that + /// name, so the name is cut short enough for the connection to stay on screen. static func message(for request: MCPApprovalRequest) -> String { - let client = request.clientLabel ?? String(localized: "An MCP client") + let client = request.clientLabel.map(Self.bounded) ?? String(localized: "An MCP client") let opening = String( format: String(localized: "%@ wants to access '%@' (%@)."), client, @@ -50,6 +54,11 @@ struct MCPApprovalAlertPresenter: MCPApprovalPresenting { return opening + "\n\n" + Self.consequence(of: request.memory) + " " + pointer } + private static func bounded(_ label: String) -> String { + guard label.count > maximumClientLabelLength else { return label } + return String(label.prefix(maximumClientLabelLength)) + "…" + } + private static func consequence(of memory: MCPApprovalMemory) -> String { switch memory { case .thisRequestOnly: diff --git a/TablePro/Core/Services/Execution/Providers/OperationConfirming.swift b/TablePro/Core/Services/Execution/Providers/OperationConfirming.swift index bb129aaa48..d9f819748d 100644 --- a/TablePro/Core/Services/Execution/Providers/OperationConfirming.swift +++ b/TablePro/Core/Services/Execution/Providers/OperationConfirming.swift @@ -97,7 +97,6 @@ internal enum OperationConfirmationPrompt { internal struct AlertOperationConfirming: OperationConfirming { @MainActor func confirm(_ request: OperationConfirmationRequest) async -> Bool { - AppActivationPolicyController.shared.activate(ignoringOtherApps: true) let window = WindowLifecycleMonitor.shared.activeWindow( for: request.connectionId, preferring: NSApp.keyWindow diff --git a/TablePro/Core/Services/Infrastructure/AppActivationPolicyController.swift b/TablePro/Core/Services/Infrastructure/AppActivationPolicyController.swift index 7f356f2481..43892930cc 100644 --- a/TablePro/Core/Services/Infrastructure/AppActivationPolicyController.swift +++ b/TablePro/Core/Services/Infrastructure/AppActivationPolicyController.swift @@ -72,6 +72,20 @@ internal final class AppActivationPolicyController { } } + /// For a prompt another process caused. Taking focus would hand it the keystrokes the person + /// was typing elsewhere, so the app gets a Dock icon to bounce and waits to be chosen. + @discardableResult + internal func requestAttention() -> Int? { + enterForeground() + guard !NSApp.isActive else { return nil } + return NSApp.requestUserAttention(.criticalRequest) + } + + internal func endAttentionRequest(_ request: Int?) { + guard let request else { return } + NSApp.cancelUserAttentionRequest(request) + } + /// For a path that puts a window on screen without asking to come forward. The window does not /// exist yet at this point, so this promotes outright rather than counting windows. @discardableResult diff --git a/TablePro/Core/Services/Infrastructure/TabRouter.swift b/TablePro/Core/Services/Infrastructure/TabRouter.swift index d3bceaf760..762831a7a4 100644 --- a/TablePro/Core/Services/Infrastructure/TabRouter.swift +++ b/TablePro/Core/Services/Infrastructure/TabRouter.swift @@ -302,7 +302,7 @@ internal final class TabRouter { } let preview = previewForSQL(sql) - let confirmed = await AlertHelper.runApprovalModal( + let confirmed = await AlertHelper.runCrossProcessApproval( title: String(localized: "Open Query from Link"), message: String( format: String(localized: "An external link wants to open a query on \"%@\":\n\n%@"), diff --git a/TablePro/Core/Utilities/UI/AlertHelper.swift b/TablePro/Core/Utilities/UI/AlertHelper.swift index 1f194c203d..a0dc3210b0 100644 --- a/TablePro/Core/Utilities/UI/AlertHelper.swift +++ b/TablePro/Core/Utilities/UI/AlertHelper.swift @@ -91,18 +91,18 @@ final class AlertHelper { in window: NSWindow?, completion: @escaping @MainActor (NSApplication.ModalResponse) -> Void = { _ in } ) { + let attention = AppActivationPolicyController.shared.requestAttention() guard let parent = resolveWindow(window) else { - /// The two sibling detached-modal paths already come forward before they block, and this - /// one did not. An alert nobody can see still holds a nested modal run loop, and a - /// process serving MCP in the background has no window to click and no Dock icon to - /// reach for, so the caller waits on an answer that can never be given. - AppActivationPolicyController.shared.activate(ignoringOtherApps: true) let response = alert.runModal() + AppActivationPolicyController.shared.endAttentionRequest(attention) AppActivationPolicyController.shared.reevaluate() completion(response) return } - alert.beginSheetModal(for: parent, completionHandler: completion) + alert.beginSheetModal(for: parent) { response in + AppActivationPolicyController.shared.endAttentionRequest(attention) + completion(response) + } } static func response(to alert: NSAlert, in window: NSWindow?) async -> NSApplication.ModalResponse { @@ -167,22 +167,29 @@ final class AlertHelper { // MARK: - Cross-Process Approval - static func runApprovalModal( + /// Return and Escape both refuse. `NSAlert` gives a button one key equivalent, so no alert can + /// bind both to Deny; the decision window takes Escape and the view gives Deny Return. + static func runCrossProcessApproval( title: String, message: String, confirm: String, cancel: String ) async -> Bool { - AppActivationPolicyController.shared.activate(ignoringOtherApps: true) - let alert = NSAlert() - alert.messageText = title - alert.informativeText = message - alert.alertStyle = .warning - alert.addButton(withTitle: confirm) - alert.addButton(withTitle: cancel) - let response = alert.runModal() - AppActivationPolicyController.shared.reevaluate() - return response == .alertFirstButtonReturn + let gate = ModalDecisionGate.confirmation() + let allowed = try? await runHostedDecision( + title: title, + fittingWidth: CrossProcessApprovalView.width, + gate: gate, + window: nil, + rootView: CrossProcessApprovalView( + title: title, + message: message, + allowTitle: confirm, + denyTitle: cancel, + onDecide: { gate.deliver(.success($0)) } + ) + ) + return allowed ?? false } /// Pairing is a security decision, so the attached case uses a critical sheet: it must not @@ -290,31 +297,26 @@ final class AlertHelper { size: ModalDecisionWindowSizing.contentSize(fitting: fitted, within: budget) ) - host.title = title - let sheetWindow = ModalDecisionWindow(contentViewController: host) - sheetWindow.styleMask = styleMask - sheetWindow.isReleasedWhenClosed = false - /// The HIG gives Escape to Cancel on every alert and sheet, and a confirmation that came - /// forward over the user's own work is exactly the one that has to stay dismissable. - sheetWindow.onCancel = { [weak gate] in gate?.cancel() } - - let delegate = ModalDecisionWindowDelegate { [weak gate] in gate?.cancel() } - gate.windowDelegate = delegate - sheetWindow.delegate = delegate + let sheetWindow = decisionWindow(hosting: host, title: title, styleMask: styleMask, gate: gate) + let attention = AppActivationPolicyController.shared.requestAttention() guard let parent else { gate.onResolve = { [weak sheetWindow] in + AppActivationPolicyController.shared.endAttentionRequest(attention) guard let sheetWindow else { return } sheetWindow.close() AppActivationPolicyController.shared.reevaluate(excluding: sheetWindow) } - AppActivationPolicyController.shared.activate(ignoringOtherApps: true) + /// The level an app-modal alert runs at, so the decision shows over other apps while + /// the keyboard stays with the one the person is using. + sheetWindow.level = .modalPanel sheetWindow.center() sheetWindow.makeKeyAndOrderFront(nil) return try await gate.value() } gate.onResolve = { [weak sheetWindow] in + AppActivationPolicyController.shared.endAttentionRequest(attention) guard let sheetWindow else { return } parent.endSheet(sheetWindow) sheetWindow.close() @@ -323,6 +325,26 @@ final class AlertHelper { return try await gate.value() } + static func decisionWindow( + hosting host: NSViewController, + title: String, + styleMask: NSWindow.StyleMask, + gate: ModalDecisionGate + ) -> ModalDecisionWindow { + host.title = title + let window = ModalDecisionWindow(contentViewController: host) + window.styleMask = styleMask + window.isReleasedWhenClosed = false + /// The HIG gives Escape to Cancel on every alert and sheet, and a confirmation that came + /// forward over the user's own work is exactly the one that has to stay dismissable. + window.onCancel = { [weak gate] in gate?.cancel() } + + let delegate = ModalDecisionWindowDelegate { [weak gate] in gate?.cancel() } + gate.windowDelegate = delegate + window.delegate = delegate + return window + } + // MARK: - Save Changes Confirmation enum SaveConfirmationResult { diff --git a/TablePro/Views/Components/CrossProcessApprovalView.swift b/TablePro/Views/Components/CrossProcessApprovalView.swift new file mode 100644 index 0000000000..e391e658bd --- /dev/null +++ b/TablePro/Views/Components/CrossProcessApprovalView.swift @@ -0,0 +1,42 @@ +import AppKit +import SwiftUI + +internal struct CrossProcessApprovalView: View { + internal static let width: CGFloat = 440 + + let title: String + let message: String + let allowTitle: String + let denyTitle: String + let onDecide: (Bool) -> Void + + var body: some View { + VStack(alignment: .leading, spacing: 16) { + HStack(alignment: .top, spacing: 16) { + Image(nsImage: NSApp.applicationIconImage) + .resizable() + .frame(width: 64, height: 64) + .accessibilityHidden(true) + + VStack(alignment: .leading, spacing: 8) { + Text(title) + .font(.headline) + Text(message) + .lineLimit(20) + .fixedSize(horizontal: false, vertical: true) + .textSelection(.enabled) + } + .frame(maxWidth: .infinity, alignment: .leading) + } + + DialogFooter { + Button(allowTitle) { onDecide(true) } + + Button(denyTitle, role: .cancel) { onDecide(false) } + .keyboardShortcut(.defaultAction) + } + } + .padding(20) + .frame(width: Self.width) + } +} diff --git a/TablePro/Views/Settings/Sections/PairingApprovalSheet.swift b/TablePro/Views/Settings/Sections/PairingApprovalSheet.swift index 53c5d12cf4..4ef738b8b1 100644 --- a/TablePro/Views/Settings/Sections/PairingApprovalSheet.swift +++ b/TablePro/Views/Settings/Sections/PairingApprovalSheet.swift @@ -268,11 +268,6 @@ struct PairingApprovalSheet: View { private var actionBar: some View { DialogFooter { - Button(String(localized: "Deny"), role: .cancel) { - onComplete(.failure(DatabaseAccessError.userCancelled)) - } - .keyboardShortcut(.cancelAction) - Button(String(localized: "Approve")) { let approval = PairingApproval( grantedPermissions: permissions, @@ -282,9 +277,12 @@ struct PairingApprovalSheet: View { ) onComplete(.success(approval)) } - .buttonStyle(.borderedProminent) - .keyboardShortcut(.defaultAction) .disabled(approveDisabled) + + Button(String(localized: "Deny"), role: .cancel) { + onComplete(.failure(DatabaseAccessError.userCancelled)) + } + .keyboardShortcut(.defaultAction) } } diff --git a/TableProTests/Core/Services/Infrastructure/AppActivationPolicySourceTests.swift b/TableProTests/Core/Services/Infrastructure/AppActivationPolicySourceTests.swift index 61c7520f7d..9b3581cdcb 100644 --- a/TableProTests/Core/Services/Infrastructure/AppActivationPolicySourceTests.swift +++ b/TableProTests/Core/Services/Infrastructure/AppActivationPolicySourceTests.swift @@ -49,6 +49,19 @@ struct AppActivationPolicySourceTests { ) } + @Test("Prompts and confirmations ask for attention instead of taking focus") + func promptsDoNotTakeFocus() throws { + let prompts = ["Core/Utilities/UI/AlertHelper.swift", "Core/Services/Execution/Providers/OperationConfirming.swift"] + let found = try prompts.filter { path in + let url = Self.repositoryRoot.appendingPathComponent("TablePro").appendingPathComponent(path) + return try String(contentsOf: url, encoding: .utf8).contains(".activate(") + } + #expect( + found.isEmpty, + "A prompt that takes focus can be answered by a keystroke meant for another app; use requestAttention(): \(found)" + ) + } + @Test("Only the policy controller sets the activation policy") func policyChangesGoThroughTheController() throws { let found = try offenders(matching: "setActivationPolicy(") diff --git a/TableProTests/Core/Utilities/CrossProcessApprovalTests.swift b/TableProTests/Core/Utilities/CrossProcessApprovalTests.swift new file mode 100644 index 0000000000..cd242dc982 --- /dev/null +++ b/TableProTests/Core/Utilities/CrossProcessApprovalTests.swift @@ -0,0 +1,115 @@ +import AppKit +import SwiftUI +import Testing + +@testable import TablePro + +@MainActor +struct CrossProcessApprovalTests { + private func window( + hosting rootView: some View, + gate: ModalDecisionGate + ) -> ModalDecisionWindow { + let host = NSHostingController(rootView: rootView) + let window = AlertHelper.decisionWindow( + hosting: host, + title: "Approval", + styleMask: [.titled, .closable], + gate: gate + ) + host.view.layoutSubtreeIfNeeded() + return window + } + + private func press(_ characters: String, keyCode: UInt16, in window: NSWindow) throws -> Bool { + let event = try #require(NSEvent.keyEvent( + with: .keyDown, + location: .zero, + modifierFlags: [], + timestamp: 0, + windowNumber: window.windowNumber, + context: nil, + characters: characters, + charactersIgnoringModifiers: characters, + isARepeat: false, + keyCode: keyCode + )) + return window.performKeyEquivalent(with: event) + } + + private func approvalView(gate: ModalDecisionGate) -> CrossProcessApprovalView { + CrossProcessApprovalView( + title: "MCP Access Request", + message: "A client wants to access 'prod'.", + allowTitle: "Allow", + denyTitle: "Deny", + onDecide: { gate.deliver(.success($0)) } + ) + } + + @Test("Return denies a cross-process approval") + func returnDenies() async throws { + let gate = ModalDecisionGate.confirmation() + let window = window(hosting: approvalView(gate: gate), gate: gate) + + #expect(try press("\r", keyCode: 36, in: window)) + #expect(try await gate.value() == false) + } + + @Test("Escape denies a cross-process approval") + func escapeDenies() async throws { + let gate = ModalDecisionGate.confirmation() + let window = window(hosting: approvalView(gate: gate), gate: gate) + + window.cancelOperation(nil) + + #expect(try await gate.value() == false) + } + + @Test("Return denies a pairing request") + func returnDeniesPairing() async throws { + let gate = ModalDecisionGate.pairing() + let redirect = try #require(URL(string: "http://127.0.0.1:51888/callback")) + let sheet = PairingApprovalSheet( + request: PairingRequest( + clientName: "Launcher", + challenge: "YWJjZGVmZ2hpamtsbW5vcHFyc3R1dnd4eXoxMjM0NTY", + redirectURL: redirect, + requestedScopes: nil, + requestedConnectionIds: nil + ), + codeExpiresAt: Date(timeIntervalSinceNow: 300), + allowsHiddenConnectionListing: false, + onComplete: { gate.deliver($0) } + ) + let window = window(hosting: sheet, gate: gate) + + #expect(try press("\r", keyCode: 36, in: window)) + await #expect(throws: DatabaseAccessError.self) { + _ = try await gate.value() + } + } + + @Test("The approval prompt is not greedy") + func approvalViewHasABoundedHeight() { + let gate = ModalDecisionGate.confirmation() + let host = NSHostingController(rootView: approvalView(gate: gate)) + host.sizingOptions = [] + let size = host.sizeThatFits(in: NSSize(width: CrossProcessApprovalView.width, height: .greatestFiniteMagnitude)) + #expect(size.height.isFinite) + #expect(size.height < 600) + } + + @Test("A client name of any length leaves the connection it asks for on screen") + func longClientNameKeepsTheConnectionVisible() { + let message = MCPApprovalAlertPresenter.message(for: MCPApprovalRequest( + connectionName: "prod", + databaseType: "PostgreSQL", + clientLabel: String(repeating: "x", count: 5_000), + memory: .untilRevoked + )) + + #expect(message.contains("'prod' (PostgreSQL)")) + #expect(message.count < MCPApprovalAlertPresenter.maximumClientLabelLength + 300) + } +}