From 153aeff1b6c9302516d7390174cc9fe7cc7fc643 Mon Sep 17 00:00:00 2001 From: fitz123 <10243861+fitz123@users.noreply.github.com> Date: Thu, 21 May 2026 18:31:52 +0400 Subject: [PATCH 1/7] fix(menubar): route bb-vpn:// via AppDelegate, not NSAppleEventManager MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Symptom: clicking a bb-vpn://enroll URI from Safari (paste in address bar, Allow on the 'Open in BBVPN?' dialog) brought BBVPN.app to the foreground but never invoked the enroll handler. End-to-end test on a clean .pkg install showed: - Launch Services successfully foregrounded BBVPN.app (SetFrontProcessWithWindowIDAndCPSOptions in syslog) - But the NSAppleEventManager handler registered in BBVPNApp.init() never fired — no bb-vpn enroll subprocess, no NSAlert, no inbox file dropped - Daemons stayed grey; manual 'bb-vpn enroll' from terminal worked, so the daemon-side ingest path was healthy Root cause: NSAppleEventManager + AEEventClass(kInternetEventClass) dispatch is unreliable for LSUIElement (background) SwiftUI menubar apps on macOS 13/14. The handler IS registered, but the AppleEvent never makes it through SwiftUI's MenuBarExtra lifecycle to the kAEGetURL selector. Fix: bind an NSApplicationDelegateAdaptor with application(_:open:) — the canonical AppKit URL entry point. AppKit dispatches it off the run loop regardless of scene/window state, so it works for LSUIElement apps. Same pattern other menubar apps use for custom URL schemes. Removed the URLEventHandler class from EnrollHandler.swift since the new path obsoletes it; EnrollHandler.handleEnrollURL is now called directly from AppDelegate.application(_:open:). Verified on macold: fresh .pkg install, paste URI in Safari, Allow click, 'Enrollment queued' NSAlert, daemons green within ~5s. Sync log confirms the WatchPaths-triggered sync after the click landed the inbox file: promoted=true kickstarted=true. --- client/menubar/BBVPN/BBVPNApp.swift | 35 ++++++++++++++++++------ client/menubar/BBVPN/EnrollHandler.swift | 30 ++++---------------- 2 files changed, 33 insertions(+), 32 deletions(-) diff --git a/client/menubar/BBVPN/BBVPNApp.swift b/client/menubar/BBVPN/BBVPNApp.swift index 59d7163..e12a684 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,17 @@ 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. +final class AppDelegate: NSObject, NSApplicationDelegate { + func application(_ application: NSApplication, open urls: [URL]) { + for url in urls { + EnrollHandler.handleEnrollURL(url) + } + } +} diff --git a/client/menubar/BBVPN/EnrollHandler.swift b/client/menubar/BBVPN/EnrollHandler.swift index d703c15..fb6747e 100644 --- a/client/menubar/BBVPN/EnrollHandler.swift +++ b/client/menubar/BBVPN/EnrollHandler.swift @@ -86,27 +86,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. From 38300a2df862254f617ee90cacaf7886aa8b733b Mon Sep 17 00:00:00 2001 From: fitz123 <10243861+fitz123@users.noreply.github.com> Date: Thu, 21 May 2026 18:44:54 +0400 Subject: [PATCH 2/7] fix: address multi-review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - BBVPNApp.swift: filter application(_:open:) urls to bb-vpn:// only. AppKit's application(_:open:) is the catch-all for both custom URL schemes AND file:// URLs from open -a BBVPN / Finder drag drops — scopes the previous NSAppleEventManager + kInternetEventClass handler did not receive. Without the filter, a stray file drop would fire N 'Not a bb-vpn enroll link' modals serially on the main thread (Gemini high finding, confidence 0.95). - EnrollHandler.swift: watchdog comment no longer references the removed NSAppleEventManager path; now points at AppDelegate.application(_:open:) (Opus low finding, 0.95). - Info.plist: CFBundleURLTypes comment likewise updated to mention AppDelegate.application(_:open:) instead of NSAppleEventManager (Codex + Opus low finding, 0.9). --- client/menubar/BBVPN/BBVPNApp.swift | 9 ++++++++- client/menubar/BBVPN/EnrollHandler.swift | 13 +++++++------ client/menubar/Info.plist | 3 ++- 3 files changed, 17 insertions(+), 8 deletions(-) diff --git a/client/menubar/BBVPN/BBVPNApp.swift b/client/menubar/BBVPN/BBVPNApp.swift index e12a684..47602f9 100644 --- a/client/menubar/BBVPN/BBVPNApp.swift +++ b/client/menubar/BBVPN/BBVPNApp.swift @@ -105,9 +105,16 @@ struct BBVPNApp: App { // 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 { + 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 fb6747e..571525a 100644 --- a/client/menubar/BBVPN/EnrollHandler.swift +++ b/client/menubar/BBVPN/EnrollHandler.swift @@ -51,12 +51,13 @@ enum EnrollHandler { proc.standardError = errPipe proc.standardOutput = Pipe() // discard stdout // 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 { 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 From fadf185e8761a49fca52aa394385c5cb0e075df0 Mon Sep 17 00:00:00 2001 From: fitz123 <10243861+fitz123@users.noreply.github.com> Date: Thu, 21 May 2026 18:50:12 +0400 Subject: [PATCH 3/7] fix(menubar): discard stdout via FileHandle.nullDevice, not Pipe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit proc.standardOutput = Pipe() with no reader buffers child writes in-kernel; once the pipe buffer fills (~16-64KB on macOS) the child blocks on write() and the parent blocks on exited.wait(), forcing the 10s watchdog to terminate the subprocess with a misleading 'timed out' error. bb-vpn enroll's actual output is tiny today (~60 bytes), so the bug is latent — but the pattern is unsafe for any future change that emits more, and FileHandle.nullDevice is the documented way to discard child output. (multi-review round 2, Gemini critical, 1.0 confidence.) Skipped: re-entrancy / main-thread blocking concerns (Gemini high, Opus low) — both require an async refactor of EnrollHandler and are out of scope under the single-user threat model + 'don't rush edge cases' standing instruction. The 10s watchdog continues to bound damage. --- client/menubar/BBVPN/EnrollHandler.swift | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/client/menubar/BBVPN/EnrollHandler.swift b/client/menubar/BBVPN/EnrollHandler.swift index 571525a..bbb4278 100644 --- a/client/menubar/BBVPN/EnrollHandler.swift +++ b/client/menubar/BBVPN/EnrollHandler.swift @@ -49,7 +49,15 @@ enum EnrollHandler { proc.arguments = args let errPipe = Pipe() proc.standardError = errPipe - proc.standardOutput = Pipe() // discard stdout + // Discard stdout to /dev/null — NOT to a Pipe(). 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 10s watchdog to terminate with a misleading + // "timeout" error. bb-vpn enroll's actual output is ~60 + // bytes today, but the pattern is unsafe for any future + // change that emits more. + proc.standardOutput = FileHandle.nullDevice // Watchdog: handleEnrollURL runs on the main thread (Launch Services // delivers the bb-vpn:// click via AppDelegate.application(_:open:) // in BBVPNApp.swift → here). A hung `bb-vpn enroll` (slow disk, From 656447f7bcd1010af7741f242e15f6425c463d7b Mon Sep 17 00:00:00 2001 From: fitz123 <10243861+fitz123@users.noreply.github.com> Date: Thu, 21 May 2026 18:55:46 +0400 Subject: [PATCH 4/7] fix(menubar): drain stderr concurrently to close the same deadlock class as stdout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 2 fixed the stdout pipe deadlock but left stderr exposed to the identical pattern: proc.standardError = Pipe() and we only call readToEnd() AFTER exited.wait() returns. Three round-3 reviewers (Codex, Opus, Gemini) all flagged it independently with the same mechanism: 1. Buffer-fill deadlock: if bb-vpn enroll writes >16-64KB to stderr (e.g., a Go panic stack trace, a future verbose flag, or a long error message echoing an attacker-supplied invalid UUID via %q formatting), the child blocks on write() before exit. The parent blocks on exited.wait(), the 10s watchdog fires, and the user sees a misleading 'timed out after 10s' modal instead of the real error. 2. Infinite main-thread hang: readToEnd() blocks until ALL writer fds close. If bb-vpn enroll ever forks a long-lived helper that inherits the stderr fd (e.g., a future detached launchctl kickstart from enroll), the child's own fd closes on exit but the grandchild's stays open. readToEnd() then blocks the menubar UI indefinitely — with NO watchdog above it (the watchdog covers only exited.wait()). Fix: drain stderr concurrently via readabilityHandler while the child runs, accumulating into a locked Data buffer. The handler self-clears on EOF (empty chunk = child closed its writer), and we defensively clear it again after exited.wait() in case the handler missed the final delivery due to scheduling. Stderr text remains available for the NSAlert on failure paths, so user error fidelity is unchanged. Deferred (still): NSAlert.runModal nested-run-loop re-entrancy if the user clicks a second bb-vpn:// link while the first alert is up — would need an async refactor, out of scope under the single-user threat model. Watchdog continues to bound damage. Also deferred (Opus low): proc.terminate() leaking pipe fds on the timeout path — pre-existing, low-impact (BBVPN.app is long-lived but timeouts should be rare). --- client/menubar/BBVPN/EnrollHandler.swift | 49 ++++++++++++++++++------ 1 file changed, 38 insertions(+), 11 deletions(-) diff --git a/client/menubar/BBVPN/EnrollHandler.swift b/client/menubar/BBVPN/EnrollHandler.swift index bbb4278..e97f433 100644 --- a/client/menubar/BBVPN/EnrollHandler.swift +++ b/client/menubar/BBVPN/EnrollHandler.swift @@ -47,17 +47,39 @@ 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. The handler also + // self-clears on EOF (empty chunk = child closed its writer), + // and we defensively clear it again after the wait in case + // the handler missed the final delivery due to scheduling. let errPipe = Pipe() proc.standardError = errPipe - // Discard stdout to /dev/null — NOT to a Pipe(). 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 10s watchdog to terminate with a misleading - // "timeout" error. bb-vpn enroll's actual output is ~60 - // bytes today, but the pattern is unsafe for any future - // change that emits more. - proc.standardOutput = FileHandle.nullDevice + var stderrData = Data() + let stderrLock = NSLock() + errPipe.fileHandleForReading.readabilityHandler = { handle in + let chunk = handle.availableData + if chunk.isEmpty { + handle.readabilityHandler = nil + return + } + stderrLock.lock() + stderrData.append(chunk) + stderrLock.unlock() + } + // Watchdog: handleEnrollURL runs on the main thread (Launch Services // delivers the bb-vpn:// click via AppDelegate.application(_:open:) // in BBVPNApp.swift → here). A hung `bb-vpn enroll` (slow disk, @@ -71,14 +93,19 @@ enum EnrollHandler { 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() + errPipe.fileHandleForReading.readabilityHandler = nil return CLIResult(status: -1, stderr: "bb-vpn enroll timed out after 10s") } - let data = (try? errPipe.fileHandleForReading.readToEnd()) ?? Data() - let stderrText = String(data: data, encoding: .utf8) ?? "" + errPipe.fileHandleForReading.readabilityHandler = nil + + stderrLock.lock() + let stderrText = String(data: stderrData, encoding: .utf8) ?? "" + stderrLock.unlock() return CLIResult(status: proc.terminationStatus, stderr: stderrText) } From 9e904c71e37f5fe0eed85ab5aa13ca02682a4c95 Mon Sep 17 00:00:00 2001 From: fitz123 <10243861+fitz123@users.noreply.github.com> Date: Thu, 21 May 2026 18:59:54 +0400 Subject: [PATCH 5/7] fix(menubar): synchronize stderr drain with process exit, preserve stderr on timeout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 4 found a residual race in the round-3 stderr drain: NOTE_EXIT (Process.terminationHandler) and EVFILT_READ (Pipe readabilityHandler) are independent kevents on separate GCD queues. The terminationHandler can win the race and signal `exited` BEFORE the readabilityHandler has been invoked for the final bytes still buffered in the pipe. Clearing the handler immediately after exited.wait() then drops those bytes — the user sees the generic 'bb-vpn enroll failed (exit N)' alert instead of the actual error string the child emitted. All three round-4 reviewers (Codex 0.86, Opus 0.7, Gemini 1.0) flagged this independently with the same fix: have the handler signal a second semaphore on its empty-chunk delivery (EOF) and wait briefly for it before snapshotting. Bounded at 100ms — short enough to be invisible on the happy path, long enough to absorb any kevent scheduling latency, and capped so a future grandchild inheriting the writer fd cannot pin the menubar indefinitely. Also addresses Opus's secondary observability nit: on the timeout path we now snapshot whatever stderr the child managed to emit before hanging and include it in the modal message. The watchdog exists to make hung-child failures visible, and the previous code overwrote the most useful diagnostic with a generic timeout string. Still deferred per single-user threat model: - NSAlert.runModal nested run loop re-entrancy - main-thread blocking in handleEnrollURL - terminate-path pipe-fd leak (BBVPN.app is long-lived, timeouts should be rare) --- client/menubar/BBVPN/EnrollHandler.swift | 32 +++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/client/menubar/BBVPN/EnrollHandler.swift b/client/menubar/BBVPN/EnrollHandler.swift index e97f433..98cff06 100644 --- a/client/menubar/BBVPN/EnrollHandler.swift +++ b/client/menubar/BBVPN/EnrollHandler.swift @@ -69,10 +69,21 @@ enum EnrollHandler { proc.standardError = errPipe 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) errPipe.fileHandleForReading.readabilityHandler = { handle in let chunk = handle.availableData if chunk.isEmpty { handle.readabilityHandler = nil + stderrEOF.signal() return } stderrLock.lock() @@ -98,9 +109,28 @@ enum EnrollHandler { } if exited.wait(timeout: .now() + 10) == .timedOut { proc.terminate() + // Still wait briefly for EOF — proc.terminate() closes + // the child's stderr fd, which should unblock the + // readabilityHandler with a final empty chunk. Bounded so + // a leaked-fd grandchild can't pin us. + _ = stderrEOF.wait(timeout: .now() + 0.1) errPipe.fileHandleForReading.readabilityHandler = nil - return CLIResult(status: -1, stderr: "bb-vpn enroll timed out after 10s") + stderrLock.lock() + let captured = String(data: stderrData, encoding: .utf8) ?? "" + 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) } + // 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() From bdf6c51dfcb5a39261369f4d5cef7109c6e30cb8 Mon Sep 17 00:00:00 2001 From: fitz123 <10243861+fitz123@users.noreply.github.com> Date: Thu, 21 May 2026 19:15:54 +0400 Subject: [PATCH 6/7] fix(menubar): idempotent EOF signal + lossy UTF-8 decode for stderr MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 5 surfaced two real issues missed by round-4: 1. (Gemini critical, defensive fix) The closure self-cleared via `handle.readabilityHandler = nil` while the main thread also cleared it after the EOF semaphore wait. Foundation's setter thread-safety vs the IO queue running the closure is undocumented — concurrent clear could in theory deadlock the main thread waiting for the in-flight closure. Replace the self-clear with an idempotent `hasEOF` flag guarded by stderrLock: the handler can safely fire multiple empty-chunk deliveries until the main thread clears it, but only signals the semaphore once (and never over-signals on deinit). Main thread retains sole ownership of the handler-clear, so no concurrent-write surface at all. 2. (Gemini medium) `String(data:encoding:.utf8) ?? ""` returns nil if the captured bytes happen to end mid-multibyte-sequence, dropping the ENTIRE stderr text into an empty string. Replace with `String(decoding: stderrData, as: UTF8.self)` which replaces invalid sequences with U+FFFD and preserves the surrounding diagnostic. Also fixed the misleading comment on the timeout path (Opus low): proc.terminate() doesn't directly close the child's stderr fd — it sends SIGTERM, and the kernel closes fds when the child exits in response. The 100ms bound absorbs that SIGTERM-to-exit latency. Codex's medium ('use synchronous drain instead of 100ms') skipped: Opus's analysis showed the bounded 100ms wait is intentional for the grandchild-leaked-fd fallback, and kqueue empirically delivers the final empty-read well under that window. Still deferred per single-user threat model: - NSAlert.runModal nested run loop re-entrancy - main-thread blocking in handleEnrollURL - terminate-path pipe-fd leak --- client/menubar/BBVPN/EnrollHandler.swift | 36 ++++++++++++++++++------ 1 file changed, 27 insertions(+), 9 deletions(-) diff --git a/client/menubar/BBVPN/EnrollHandler.swift b/client/menubar/BBVPN/EnrollHandler.swift index 98cff06..c9ca9bb 100644 --- a/client/menubar/BBVPN/EnrollHandler.swift +++ b/client/menubar/BBVPN/EnrollHandler.swift @@ -79,14 +79,27 @@ enum EnrollHandler { // 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 { - handle.readabilityHandler = nil - stderrEOF.signal() + if !hasEOF { + hasEOF = true + stderrLock.unlock() + stderrEOF.signal() + return + } + stderrLock.unlock() return } - stderrLock.lock() stderrData.append(chunk) stderrLock.unlock() } @@ -109,14 +122,15 @@ enum EnrollHandler { } if exited.wait(timeout: .now() + 10) == .timedOut { proc.terminate() - // Still wait briefly for EOF — proc.terminate() closes - // the child's stderr fd, which should unblock the - // readabilityHandler with a final empty chunk. Bounded so - // a leaked-fd grandchild can't pin us. + // 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(data: stderrData, encoding: .utf8) ?? "" + 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 @@ -134,7 +148,11 @@ enum EnrollHandler { errPipe.fileHandleForReading.readabilityHandler = nil stderrLock.lock() - let stderrText = String(data: stderrData, encoding: .utf8) ?? "" + // 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) } From 8beabcd83e37d98af6174c623a46d26cf5599f70 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 21 May 2026 15:26:31 +0000 Subject: [PATCH 7/7] docs(menubar): fix EnrollHandler EOF-handler comment wording Agent-Logs-Url: https://github.com/fitz123/bb-dpi/sessions/17e810c1-41d8-4a95-8b3e-4bef763cecf9 Co-authored-by: fitz123 <10243861+fitz123@users.noreply.github.com> --- client/menubar/BBVPN/EnrollHandler.swift | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/client/menubar/BBVPN/EnrollHandler.swift b/client/menubar/BBVPN/EnrollHandler.swift index c9ca9bb..c2394ef 100644 --- a/client/menubar/BBVPN/EnrollHandler.swift +++ b/client/menubar/BBVPN/EnrollHandler.swift @@ -61,10 +61,9 @@ enum EnrollHandler { // 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. The handler also - // self-clears on EOF (empty chunk = child closed its writer), - // and we defensively clear it again after the wait in case - // the handler missed the final delivery due to scheduling. + // 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 var stderrData = Data()