fix(menubar): wire bb-vpn:// via AppDelegate + harden EnrollHandler subprocess plumbing#29
Merged
Merged
Conversation
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.
- 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 <files> / 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).
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.
…ass as stdout 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).
…derr on timeout
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)
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
There was a problem hiding this comment.
Pull request overview
This PR fixes macOS custom URL handling for the BBVPN menubar app so bb-vpn://enroll?uuid=… links reliably trigger enrollment by routing Launch Services URL opens through an AppKit NSApplicationDelegate entry point, and it hardens the bb-vpn enroll subprocess stderr/stdout handling to avoid pipe-buffer deadlocks and improve diagnostics.
Changes:
- Switch URL dispatch from
NSAppleEventManagerregistration toNSApplicationDelegateAdaptor+application(_:open:)(with scheme filtering). - Harden subprocess execution by discarding stdout to
/dev/nulland draining stderr concurrently viareadabilityHandler, preserving stderr on timeout. - Update Info.plist documentation comments to reflect the new URL routing path.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| client/menubar/Info.plist | Updates URL-handler documentation comment to match the new AppDelegate-based dispatch path. |
| client/menubar/BBVPN/EnrollHandler.swift | Prevents subprocess pipe deadlocks (stdout null device, concurrent stderr drain) and improves timeout/error diagnostics. |
| client/menubar/BBVPN/BBVPNApp.swift | Adds NSApplicationDelegateAdaptor AppDelegate to reliably receive bb-vpn:// URL opens and forward to EnrollHandler. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Owner
Author
|
@copilot please re-review — fixes pushed since the last review |
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>
Contributor
Re-review complete. I fixed the remaining review nit in |
Owner
Author
|
@copilot please re-review — fixes pushed since the last review |
Contributor
Re-review complete on the latest branch state. I don’t see additional issues to fix right now; the prior EnrollHandler comment alignment remains in |
Owner
Author
|
@copilot please re-review — fixes pushed since the last review |
Contributor
Re-review complete on the latest branch state; I don’t see additional fixes needed beyond |
Copilot stopped work on behalf of
fitz123 due to an error
May 21, 2026 15:41
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the BBVPN.app URL handler so clicking a
bb-vpn://enroll?uuid=…link from Safari/Slack/Mail actually invokesbb-vpn enroll. End-to-end verified on macold: fresh.pkginstall → paste URI in Safari → Allow dialog → "Enrollment queued" NSAlert → daemons green.Root cause
NSAppleEventManager+kInternetEventClass/kAEGetURLregistration inBBVPNApp.init()doesn't reliably fire for LSUIElement (background) SwiftUI menubar apps on macOS 13/14. Launch Services foregrounds the app correctly (visible asSetFrontProcessWithWindowID…in syslog) but the AppleEvent never reaches the registered selector. Until this branch, clicking an enroll link did nothing.Fix
Bind an
NSApplicationDelegateAdaptorwithapplication(_:open:)— the canonical AppKit URL entry point. AppKit dispatches it directly off the run loop regardless of scene/window state. This is the documented pattern other menubar apps use for custom URL schemes.Surrounding hardening
5 rounds of
/multi-review(Codex + Opus + Gemini) caught a chain of latent issues in the surrounding subprocess plumbing that became more reachable once the URL handler started firing:153aeff38300a2application(_:open:)urls by scheme (drops file:// URLs that AppKit would otherwise route here); refresh stale comments in EnrollHandler/Info.plist; cross-refdocs/release.mdfrom README/AGENTSfadf185proc.standardOutput = FileHandle.nullDevice(unreadPipe()would buffer up to 64KB then deadlock the child)656447freadabilityHandler(same deadlock class; plus latentreadToEndhang on grandchild-inherited fd)9e904c7bdf6c51hasEOFflag (defensive, sidesteps any reentrancy in FileHandle.readabilityHandler property setter); lossy UTF-8 decode (String(decoding:as:)preserves partial diagnostics through multi-byte boundary truncation)Deliberately deferred
Under the single-user-Mac threat model + "concentrate only on must-fix issues":
NSAlert.runModalnested run loop re-entrancy if a second URL arrives while the first modal is upProcess+runModalTest plan
make build-menubar+make build-pkgproduce a signed.pkgwith ad-hocSignature=adhocon BBVPN.applast_error: no_identitybb-vpn://enroll?uuid=…in Safari → Allow → NSAlert "Enrollment queued" appearsgitleaksscan against fitz123/gitleaks-config (clean)