diff --git a/client/menubar/BBVPN/BBVPNApp.swift b/client/menubar/BBVPN/BBVPNApp.swift index 59d7163..47602f9 100644 --- a/client/menubar/BBVPN/BBVPNApp.swift +++ b/client/menubar/BBVPN/BBVPNApp.swift @@ -17,20 +17,25 @@ // receipt and which drops one `inbox/enroll-*.json` for the root // daemon to ingest (drop-box is mode 1733 by the .pkg postinstall). +import AppKit import SwiftUI @main struct BBVPNApp: App { + // Launch Services delivers `bb-vpn://enroll?…` via NSApplication's + // `application(_:open:)` delegate hook — the canonical AppKit URL + // entry point. We bind it via NSApplicationDelegateAdaptor because + // SwiftUI's `.onOpenURL` on `MenuBarExtra` does not reliably fire + // for LSUIElement (background) apps on macOS 13/14: end-to-end + // testing on macold showed Launch Services brought BBVPN.app to + // the foreground (visible in syslog as SetFrontProcess) but the + // SwiftUI scene-modifier never received the URL. The previous + // NSAppleEventManager-in-init approach had the same problem. + // application(_:open:) works because AppKit dispatches it + // directly off the run loop regardless of scene/window state. + @NSApplicationDelegateAdaptor(AppDelegate.self) private var appDelegate @StateObject private var status = StatusModel() - init() { - // Wire Launch Services → EnrollHandler. Done once at process - // start so the very first bb-vpn:// click after install lands - // even though the user hasn't opened the app yet (MenuBarExtra - // apps launch implicitly on first URL receipt). - URLEventHandler.shared.register() - } - var body: some Scene { MenuBarExtra { menuContent @@ -93,3 +98,24 @@ struct BBVPNApp: App { .keyboardShortcut("q") } } + +// AppDelegate exists only to provide application(_:open:) — the +// AppKit hook Launch Services uses for `bb-vpn://` clicks. URLs +// arrive here as an array because macOS can deliver multiple at +// once (e.g., the user clicks several enroll links in quick +// succession before the app is fully booted); each is dispatched +// to EnrollHandler independently. +// +// `application(_:open:)` is also AppKit's catch-all for file:// URLs +// from `open -a BBVPN ` and Finder drag-and-drop — scopes the +// previous NSAppleEventManager + kInternetEventClass handler did +// NOT receive. Filter to bb-vpn:// only so a stray file drop doesn't +// fire N "Not a bb-vpn enroll link" modals serially on the main +// thread. +final class AppDelegate: NSObject, NSApplicationDelegate { + func application(_ application: NSApplication, open urls: [URL]) { + for url in urls where url.scheme?.lowercased() == "bb-vpn" { + EnrollHandler.handleEnrollURL(url) + } + } +} diff --git a/client/menubar/BBVPN/EnrollHandler.swift b/client/menubar/BBVPN/EnrollHandler.swift index d703c15..c2394ef 100644 --- a/client/menubar/BBVPN/EnrollHandler.swift +++ b/client/menubar/BBVPN/EnrollHandler.swift @@ -47,29 +47,112 @@ enum EnrollHandler { let proc = Process() proc.executableURL = URL(fileURLWithPath: bbvpnBin) proc.arguments = args + // Discard stdout to /dev/null. An unread Pipe buffers the + // child's writes and blocks on write() once the kernel buffer + // fills (~16-64KB on macOS), which would hang the child while + // the parent blocks on `exited.wait()` and force the watchdog + // to terminate with a misleading "timeout" error. + proc.standardOutput = FileHandle.nullDevice + + // Stderr feeds the user-visible NSAlert on failure, so we + // can't discard it — but we also can't leave the Pipe unread + // (same deadlock as stdout) or read synchronously *after* + // `exited.wait()` returns (readToEnd would block forever if + // a future grandchild inherits the writer fd, escaping the + // watchdog). Drain concurrently via readabilityHandler: + // bytes accumulate into stderrData while the child runs, and + // we snapshot it after the child exits. On EOF (empty chunk = + // child closed its writer), the handler signals stderrEOF; the + // main thread clears readabilityHandler after the bounded wait. let errPipe = Pipe() proc.standardError = errPipe - proc.standardOutput = Pipe() // discard stdout + var stderrData = Data() + let stderrLock = NSLock() + // EOF is signaled by the readabilityHandler on its empty-chunk + // delivery (writer-side close = child exit). NOTE_EXIT (which + // signals `exited` below) and EVFILT_READ (which fires the + // readabilityHandler) are independent kevents on separate GCD + // queues — the termination semaphore CAN fire before the + // handler has drained the bytes still buffered in the pipe. + // Without this extra synchronization, runBBVPN can return an + // empty stderr and the user sees "bb-vpn enroll failed (exit + // N)" instead of the actual error string. + let stderrEOF = DispatchSemaphore(value: 0) + // `hasEOF` is read+written under stderrLock so the handler can + // safely fire multiple empty-chunk deliveries (which is what + // happens until the main thread clears the handler) without + // over-signaling the semaphore. Doing the clear ONLY from the + // main thread also sidesteps the question of whether + // FileHandle's readabilityHandler setter is reentrant against + // the IO queue that runs the closure. + var hasEOF = false + errPipe.fileHandleForReading.readabilityHandler = { handle in + let chunk = handle.availableData + stderrLock.lock() + if chunk.isEmpty { + if !hasEOF { + hasEOF = true + stderrLock.unlock() + stderrEOF.signal() + return + } + stderrLock.unlock() + return + } + stderrData.append(chunk) + stderrLock.unlock() + } + // Watchdog: handleEnrollURL runs on the main thread (Launch Services - // delivers the bb-vpn:// click via NSAppleEventManager → AppleEvent - // handler → here). A hung `bb-vpn enroll` (slow disk, future enroll - // flow with a network call, etc.) would freeze the menubar UI - // indefinitely. Cap at 10s — enroll is local-only (validate UUID, - // write inbox/.json), so 10s is well over the realistic - // worst case and short enough to keep the UI responsive. + // delivers the bb-vpn:// click via AppDelegate.application(_:open:) + // in BBVPNApp.swift → here). A hung `bb-vpn enroll` (slow disk, + // future enroll flow with a network call, etc.) would freeze the + // menubar UI indefinitely. Cap at 10s — enroll is local-only + // (validate UUID, write inbox/.json), so 10s is well over + // the realistic worst case and short enough to keep the UI + // responsive. let exited = DispatchSemaphore(value: 0) proc.terminationHandler = { _ in exited.signal() } do { try proc.run() } catch { + errPipe.fileHandleForReading.readabilityHandler = nil return CLIResult(status: -1, stderr: "spawn \(bbvpnBin): \(error.localizedDescription)") } if exited.wait(timeout: .now() + 10) == .timedOut { proc.terminate() - return CLIResult(status: -1, stderr: "bb-vpn enroll timed out after 10s") + // proc.terminate() sends SIGTERM. When the child exits in + // response, the kernel closes its stderr writer fd, which + // unblocks the readabilityHandler with a final empty + // chunk. Bounded so a SIGTERM-ignoring child or a + // leaked-fd grandchild cannot pin us. + _ = stderrEOF.wait(timeout: .now() + 0.1) + errPipe.fileHandleForReading.readabilityHandler = nil + stderrLock.lock() + let captured = String(decoding: stderrData, as: UTF8.self) + stderrLock.unlock() + // Preserve whatever stderr the child managed to emit + // before hanging — that's the diagnostic the watchdog + // exists to make visible. + let msg = captured.isEmpty + ? "bb-vpn enroll timed out after 10s" + : "bb-vpn enroll timed out after 10s. Last stderr: \(captured)" + return CLIResult(status: -1, stderr: msg) } - let data = (try? errPipe.fileHandleForReading.readToEnd()) ?? Data() - let stderrText = String(data: data, encoding: .utf8) ?? "" + // Normal exit: wait up to 100ms for the readabilityHandler to + // drain final bytes. Bounded short — if EOF doesn't arrive in + // this window, return what's been captured (a future grandchild + // inheriting the fd would otherwise pin us indefinitely). + _ = stderrEOF.wait(timeout: .now() + 0.1) + errPipe.fileHandleForReading.readabilityHandler = nil + + stderrLock.lock() + // Lossy UTF-8 decode (invalid sequences → U+FFFD). The strict + // form `String(data:encoding:.utf8) ?? ""` drops the entire + // string if the drain happens to split a multi-byte sequence + // mid-character; lossy decode preserves the diagnostic. + let stderrText = String(decoding: stderrData, as: UTF8.self) + stderrLock.unlock() return CLIResult(status: proc.terminationStatus, stderr: stderrText) } @@ -86,27 +169,9 @@ enum EnrollHandler { } } -// MARK: - URL hook -// -// MenuBarExtra-based apps don't get a normal AppDelegate. We wire URL -// handling via NSAppleEventManager so Launch Services delivers the -// bb-vpn:// click directly to the running app. - -final class URLEventHandler: NSObject { - static let shared = URLEventHandler() - - func register() { - NSAppleEventManager.shared().setEventHandler( - self, - andSelector: #selector(handle(_:withReplyEvent:)), - forEventClass: AEEventClass(kInternetEventClass), - andEventID: AEEventID(kAEGetURL) - ) - } - - @objc func handle(_ event: NSAppleEventDescriptor, withReplyEvent: NSAppleEventDescriptor) { - guard let str = event.paramDescriptor(forKeyword: AEKeyword(keyDirectObject))?.stringValue, - let url = URL(string: str) else { return } - EnrollHandler.handleEnrollURL(url) - } -} +// URL dispatch (Launch Services → bb-vpn://) is wired via +// NSApplicationDelegateAdaptor + AppDelegate.application(_:open:) +// in BBVPNApp.swift. The previous NSAppleEventManager registration +// in BBVPNApp.init() was removed — it failed to fire for LSUIElement +// (background) menubar apps on macOS 13/14 even though Launch +// Services successfully foregrounded BBVPN.app. diff --git a/client/menubar/Info.plist b/client/menubar/Info.plist index b511dba..ef8e259 100644 --- a/client/menubar/Info.plist +++ b/client/menubar/Info.plist @@ -28,7 +28,8 @@ + URL to BBVPN.app via AppDelegate.application(_:open:) (see + BBVPNApp.swift) → EnrollHandler.handleEnrollURL. --> CFBundleURLTypes