Skip to content

Commit c64005d

Browse files
democlaude
andcommitted
feat: v0.2.2 — whitespace-safe config fields + persistent file logging
Config fields (username, passwordPrefix, totpSecret, gateway, serverCertPin, paths) are now stripped of surrounding whitespace and invisible characters (zero-width, NBSP, full-width space, newlines) on both save AND load, so legacy configs written before this change are self-healing. Fixes silent auth/cert failures from pasting values out of chat apps / PDFs. Added a persistent file log at ~/Library/Logs/VPNMenuBar/vpnmenubar.log covering launch banner, full connect flow, dependency checks, network and interface changes, unexpected watchdog disconnects, and dependency- install results. Sensitive fields (password prefix, TOTP secret, generated code) are never written. Added "Show Logs…" menu item and a Settings "Reveal in Finder" button for quick access. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 9f0c424 commit c64005d

18 files changed

Lines changed: 288 additions & 54 deletions

CLAUDE.md

Lines changed: 27 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -19,8 +19,8 @@ This repo went through a `git filter-repo` desensitization pass before going pub
1919

2020
| Field | Placeholder in source | Where |
2121
|---|---|---|
22-
| `gateway` | `vpn.example.com` | `VPNConfig.swift` |
23-
| `serverCertPin` | `pin-sha256:AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=` | `VPNConfig.swift` (44-char base64 placeholder — syntactically valid but all zeros, end users must replace with their real cert pin) |
22+
| `gateway` | `""` (empty) | `VPNConfig.swift` — required field, user must fill in onboarding or Settings |
23+
| `serverCertPin` | `""` (empty) | `VPNConfig.swift` — required field, user must fill in onboarding or Settings |
2424
| `passwordPrefix` examples | `ExamplePass2021` | `vpn.py`, design docs |
2525
| `username` examples | `first.last` | docs only |
2626
| `totpSecret` examples | `EXAMPLEBASE32SECRETPLACEHOLDER34` | `vpn.py`, design docs (32-char Base32, syntactically valid but obviously not a real secret) |
@@ -54,13 +54,14 @@ xcodebuild -project VPNMenuBar.xcodeproj -scheme VPNMenuBar -destination 'platfo
5454
APP_SRC="$(xcodebuild -project VPNMenuBar.xcodeproj -scheme VPNMenuBar -showBuildSettings 2>/dev/null \
5555
| awk -F' = ' '/CODESIGNING_FOLDER_PATH/ {print $2}' | head -1)"
5656
cp -R "$APP_SRC" ./VPNMenuBar.app
57-
ditto -c -k --keepParent ./VPNMenuBar.app ./VPNMenuBar-0.1.0.zip
57+
ditto -c -k --keepParent ./VPNMenuBar.app ./VPNMenuBar-X.Y.Z.zip
5858
```
5959

6060
**Signing for Sparkle** (after creating the zip):
6161
```sh
62-
# sign_update is from Sparkle's tools (find it in DerivedData or download from Sparkle releases)
63-
/path/to/sign_update ./VPNMenuBar-X.Y.Z.zip
62+
# sign_update lives in the Sparkle SPM artifact:
63+
SIGN_UPDATE="$(find ~/Library/Developer/Xcode/DerivedData/VPNMenuBar-*/SourcePackages/artifacts/sparkle/Sparkle/bin -name sign_update -maxdepth 1)"
64+
"$SIGN_UPDATE" ./VPNMenuBar-X.Y.Z.zip
6465
# Copy the edSignature and length into appcast.xml's new <item>
6566
```
6667

@@ -93,10 +94,10 @@ Single-target SwiftUI app. Strict one-way dependency graph:
9394
- **Dependencies/**
9495
- `ArchDetector.swift` — maps `uname.machine` to `appleSilicon`/`intel` and exposes `defaultPaths` (brew, openconnect, vpnc-script). Used by both `VPNConfig` defaults and `DependencyChecker` so Intel users get `/usr/local/...` automatically without editing Settings
9596
- `DependencyChecker.swift` — verifies homebrew is installed, openconnect is installed, sudoers NOPASSWD works for both `sudo -n openconnect --version` and `sudo -n /sbin/route -n get default`, and vpnc-script exists. Returns `[DependencyStatus]` with copyable fix hints AND optional `InAppFix` enum cases that drive the Fix buttons
96-
- `DependencyInstaller.swift` — stateless namespace exposing the four in-app fix actions: `openTerminalForHomebrew`, `installOpenconnect(brewPath:progress:)`, `installSudoersRule(username:openconnectPath:)`, `resetVpncScriptPath(to:store:)`. See Quirk #12 for why each one uses the privilege model it uses
97-
- **UI/** — SwiftUI views: `MenuContentView`, `SettingsView`, `OnboardingView`, `DependencyAlertView`, `DependencyRowView`, `StatusBarIconFactory`, `RevealableSecureField` (eye-toggle SecureField for password/TOTP fields), `ImportSecretFromImageButton` (file-picker → `QRCodeSecretExtractor`)
97+
- `DependencyInstaller.swift` — stateless namespace exposing the four in-app fix actions: `openTerminalForHomebrew`, `installOpenconnect(brewPath:progress:)`, `installSudoersRule(username:openconnectPath:)`, `resetVpncScriptPath(to:store:)`. See Quirk #13 for why each one uses the privilege model it uses
98+
- **UI/** — SwiftUI views: `MenuContentView`, `SettingsView`, `OnboardingView`, `DependencyAlertView`, `DependencyRowView`, `AboutView`, `StatusBarIconFactory` (white rounded-rect badge with cutout shield + colored status dot), `RevealableSecureField` (eye-toggle SecureField for password/TOTP fields), `ImportSecretFromImageButton` (file-picker → `QRCodeSecretExtractor`)
9899
- **Util/LoginItemManager.swift** — thin `SMAppService.mainApp` wrapper. Preference is stored in **UserDefaults** (key `launchAtLoginEnabled`), not in `VPNConfig`, to avoid Codable schema migration
99-
- **App/VPNMenuBarApp.swift**`@main` + `AppCoordinator` (owns the singleton controller, hosts Onboarding/Settings/DependencyAlert as plain `NSWindow`s) + `AppDelegate` (intercepts `applicationShouldTerminate` to disconnect VPN on every quit path including Cmd-Q)
100+
- **App/VPNMenuBarApp.swift**`@main` + `AppCoordinator` (owns the singleton controller, hosts Onboarding/Settings/DependencyAlert/About as plain `NSWindow`s, initializes `SPUStandardUpdaterController` for Sparkle auto-updates) + `AppDelegate` (intercepts `applicationShouldTerminate` to disconnect VPN on every quit path including Cmd-Q)
100101

101102
### VPNController state machine — the "how does VPN actually work" knob
102103

@@ -116,7 +117,7 @@ Users must add to `/etc/sudoers.d/vpnmenubar-<user>`:
116117
<user> ALL=(root) NOPASSWD: /opt/homebrew/bin/openconnect, /usr/bin/pkill -x openconnect, /sbin/route
117118
```
118119

119-
The third entry (`/sbin/route`, no argv constraint) is used by `OpenConnectProcess.cleanupStaleHostRoute(forGateway:)` to delete a stale host route to the VPN gateway before each connect attempt — see Quirk #11. We don't constrain the argv to `route delete <ip>` because (a) the IP is not known until DNS resolves at runtime, and (b) sudoers wildcard matching for IP arguments is fragile. `route` is not a privileged tool beyond what openconnect itself already does to the routing table, so widening the entry is acceptable.
120+
The third entry (`/sbin/route`, no argv constraint) is used by `OpenConnectProcess.cleanupStaleHostRoute(forGateway:)` to delete a stale host route to the VPN gateway before each connect attempt — see Quirk #12. We don't constrain the argv to `route delete <ip>` because (a) the IP is not known until DNS resolves at runtime, and (b) sudoers wildcard matching for IP arguments is fragile. `route` is not a privileged tool beyond what openconnect itself already does to the routing table, so widening the entry is acceptable.
120121

121122
`DependencyChecker` only verifies the `openconnect` entry (via `sudo -n <path> --version`). It **does not** independently verify the `pkill` or `route` entries because sudoers matches argv exactly — any probe other than the exact `pkill -x openconnect` invocation wouldn't match the rule, and we can't probe with the real argv without actually killing any running openconnect. The `route` entry has no such constraint, but we still don't verify it: if it's missing, `cleanupStaleHostRoute` silently no-ops via `try?` and openconnect surfaces its native "Network is unreachable" error. The fix is to re-run `install-deps.sh` (the script's idempotency check now probes `sudo -n /sbin/route -n get default` and rewrites the sudoers file if missing, so old installs auto-upgrade).
122123

@@ -140,13 +141,15 @@ Things that require reading multiple files to understand — documenting once he
140141

141142
8. **App Sandbox is off** (required to spawn `sudo` subprocesses). This means the app can't ship via App Store and is ad-hoc signed only. See `project.yml`'s `ENABLE_APP_SANDBOX: NO` + `CODE_SIGN_IDENTITY: "-"`.
142143

143-
9. **Setup-incomplete menu lock-down**. When Onboarding is dismissed without completing, `VPNController.markSetupIncomplete()` sets `state = .failed(reason: "Setup incomplete...")`. `MenuContentView` detects this via `VPNState.isSetupIncomplete` (prefix match on the reason string) and hides Connect/Disconnect/Reconnect/Open Settings — only Status + Check Dependencies + Quit remain. This is fragile-by-design: the prefix match is the contract, don't reword the reason string without updating `isSetupIncomplete`.
144+
9. **`VPNConfig.isConfigured` checks all five required fields**: `username`, `passwordPrefix`, `totpSecret`, `gateway`, and `serverCertPin` must all be non-empty. Gateway and cert pin default to empty strings and are collected in onboarding step 3 alongside credentials. `SettingsView` tracks an `originalConfig` snapshot — the Save button is disabled until the user actually changes something. On save, the Settings window closes and the app auto-triggers a reconnect (disconnect first if already connected).
144145

145-
10. **`vpnc-script--no-dns` is a bundled resource**, not loaded from disk. It lives at `VPNMenuBar/Resources/vpnc-script--no-dns` and `project.yml` lists it as a single-file `buildPhase: resources` entry (a `type: folder` reference created a nested `Contents/Resources/Resources/` path that `Bundle.main.path(forResource:)` couldn't find). When `config.skipDNSModification == true`, `OpenConnectProcess.start` swaps the `--script` path from `config.vpncScriptPath` to the bundled copy via `Bundle.main.path(forResource:ofType:)`.
146+
10. **Setup-incomplete menu lock-down**. (Note: Quirk #9's `isConfigured` guard runs first — if required fields are empty, you can't even get past Onboarding.) When Onboarding is dismissed without completing, `VPNController.markSetupIncomplete()` sets `state = .failed(reason: "Setup incomplete...")`. `MenuContentView` detects this via `VPNState.isSetupIncomplete` (prefix match on the reason string) and hides Connect/Disconnect/Reconnect/Open Settings — only Status + Check Dependencies + Quit remain. This is fragile-by-design: the prefix match is the contract, don't reword the reason string without updating `isSetupIncomplete`.
146147

147-
11. **Stale host route cleanup before connect.** `vpnc-script` adds a host route to the VPN gateway (so encrypted tunnel traffic doesn't loop into itself) and removes it on disconnect. If openconnect dies *without* running the disconnect phase — SIGKILL, network drop, app crash, sleep/wake — the host route survives, pinned to the previous network's gateway. After switching WiFi the previous gateway is unreachable, so packets to the VPN host hit "Network is unreachable" before they ever leave the box. `OpenConnectProcess.cleanupStaleHostRoute(forGateway:)` runs before every connect: it parses `route -n get default` and `route -n get <gateway>`, and if the host route's nexthop differs from the current default gateway, runs `sudo -n /sbin/route -n delete <ip>`. The "differs from default" guard is important — when the route is fresh/correct it falls through to the default gateway and we must NOT touch it (deleting the live route during reconnect breaks healthy connections). Cleanup is best-effort: failures are `NSLog`'d only and openconnect proceeds.
148+
11. **`vpnc-script--no-dns` is a bundled resource**, not loaded from disk. It lives at `VPNMenuBar/Resources/vpnc-script--no-dns` and `project.yml` lists it as a single-file `buildPhase: resources` entry (a `type: folder` reference created a nested `Contents/Resources/Resources/` path that `Bundle.main.path(forResource:)` couldn't find). When `config.skipDNSModification == true`, `OpenConnectProcess.start` swaps the `--script` path from `config.vpncScriptPath` to the bundled copy via `Bundle.main.path(forResource:ofType:)`.
148149

149-
12. **In-app dependency installer.** `Dependencies/DependencyInstaller.swift` exposes four fix actions surfaced via `InAppFix` enum cases on `DependencyStatus`. Three of them are non-trivial:
150+
12. **Stale host route cleanup before connect.** `vpnc-script` adds a host route to the VPN gateway (so encrypted tunnel traffic doesn't loop into itself) and removes it on disconnect. If openconnect dies *without* running the disconnect phase — SIGKILL, network drop, app crash, sleep/wake — the host route survives, pinned to the previous network's gateway. After switching WiFi the previous gateway is unreachable, so packets to the VPN host hit "Network is unreachable" before they ever leave the box. `OpenConnectProcess.cleanupStaleHostRoute(forGateway:)` runs before every connect: it parses `route -n get default` and `route -n get <gateway>`, and if the host route's nexthop differs from the current default gateway, runs `sudo -n /sbin/route -n delete <ip>`. The "differs from default" guard is important — when the route is fresh/correct it falls through to the default gateway and we must NOT touch it (deleting the live route during reconnect breaks healthy connections). Cleanup is best-effort: failures are `NSLog`'d only and openconnect proceeds.
151+
152+
13. **In-app dependency installer.** `Dependencies/DependencyInstaller.swift` exposes four fix actions surfaced via `InAppFix` enum cases on `DependencyStatus`. Three of them are non-trivial:
150153
- **`installOpenconnect(brewPath:progress:)`** spawns `brew install openconnect` as the current user (brew refuses root, so this MUST not be wrapped in osascript admin). PATH is set to `<brewDir>:/usr/bin:/bin:/usr/sbin:/sbin` so brew can find auxiliary tools; `HOMEBREW_NO_AUTO_UPDATE=1` keeps it from doing a 3-minute self-update on every install. stdout+stderr are streamed line-by-line via the `progress` callback to drive the per-row spinner.
151154
- **`installSudoersRule(username:openconnectPath:)`** writes the sudoers file via `do shell script "..." with administrator privileges` — this is the ONLY supported privilege escalation under our ad-hoc signing constraint (no Developer ID → no SMJobBless). The shell command is `visudo -c -f tmp && install -m 440 -o root -g wheel tmp /etc/sudoers.d/vpnmenubar-<user>`. The temp file is written by the app under `NSTemporaryDirectory()` so it's user-owned; only the `install` step needs root. AppleScript cancel returns `errOSACancel = -128` in the error dictionary — we map that specific code to `DependencyInstallError.userCancelled` and stay silent (the row stays red, the user can click Fix again). All other failures throw `osascriptFailed`.
152155
- **`openTerminalForHomebrew()`** is the one fix that does NOT install anything itself: it launches Terminal.app via AppleScript with the official Homebrew installer command pre-typed. Homebrew's installer cannot run from inside the app — it needs an interactive `sudo` prompt and explicitly refuses to start as root. The user runs the installer in Terminal, comes back, clicks Recheck.
@@ -171,7 +174,17 @@ The repo IS the distribution. Three artifacts work together:
171174

172175
Gatekeeper bypass is `xattr -dr com.apple.quarantine /Applications/VPNMenuBar.app` or right-click → Open. Document this in any user-facing instructions.
173176

174-
**Version bumps:** if you change `CFBundleShortVersionString` in `project.yml`, no separate zip rename is needed anymore (we don't ship a versioned zip — the .app in tree IS the distribution).
177+
**Release workflow** (full checklist for cutting a new version):
178+
1. Bump `CFBundleShortVersionString` and `CFBundleVersion` in `project.yml`
179+
2. `xcodegen generate` → Release clean build → `cp -R "$APP_SRC" ./VPNMenuBar.app`
180+
3. `ditto -c -k --keepParent ./VPNMenuBar.app ./VPNMenuBar-X.Y.Z.zip`
181+
4. Sign the zip with `sign_update` (see above) → get `edSignature` and `length`
182+
5. Add a new `<item>` to `appcast.xml` with version, signature, length, and GitHub Release download URL
183+
6. Commit all changes (source + in-tree `.app` + zip + appcast.xml)
184+
7. `git push origin main`
185+
8. `gh release create vX.Y.Z ./VPNMenuBar-X.Y.Z.zip --title "vX.Y.Z" --notes "..."` to create the GitHub Release and upload the zip
186+
187+
**Version bumps:** if you change `CFBundleShortVersionString` in `project.yml`, no separate zip rename is needed for the in-tree `.app` — but the release zip should be named `VPNMenuBar-X.Y.Z.zip` to match the `appcast.xml` download URL.
175188

176189
`README.md` is the English entry point for the public repo and links to `INSTALL.md` for the detailed Chinese walkthrough. Both should be kept in sync with feature changes.
177190

VPNMenuBar-0.2.2.zip

1.69 MB
Binary file not shown.

VPNMenuBar.app/Contents/Info.plist

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,13 +23,13 @@
2323
<key>CFBundlePackageType</key>
2424
<string>APPL</string>
2525
<key>CFBundleShortVersionString</key>
26-
<string>0.2.1</string>
26+
<string>0.2.2</string>
2727
<key>CFBundleSupportedPlatforms</key>
2828
<array>
2929
<string>MacOSX</string>
3030
</array>
3131
<key>CFBundleVersion</key>
32-
<string>3</string>
32+
<string>4</string>
3333
<key>DTCompiler</key>
3434
<string>com.apple.compilers.llvm.clang.1_0</string>
3535
<key>DTPlatformBuild</key>
79.7 KB
Binary file not shown.

VPNMenuBar/App/VPNMenuBarApp.swift

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,8 @@ final class AppCoordinator: ObservableObject {
8080

8181
Self.shared = self
8282

83+
AppCoordinator.logStartupBanner()
84+
8385
// Republish controller's state changes so SwiftUI re-renders the MenuBarExtra label
8486
// (which reads coordinator.controller.state but is only observing coordinator).
8587
controller.objectWillChange
@@ -103,6 +105,15 @@ final class AppCoordinator: ObservableObject {
103105
}
104106
}
105107

108+
private static func logStartupBanner() {
109+
let bundle = Bundle.main
110+
let version = bundle.object(forInfoDictionaryKey: "CFBundleShortVersionString") as? String ?? "?"
111+
let build = bundle.object(forInfoDictionaryKey: "CFBundleVersion") as? String ?? "?"
112+
let arch = ArchDetector.current == .appleSilicon ? "arm64" : "x86_64"
113+
let os = ProcessInfo.processInfo.operatingSystemVersionString
114+
AppLogger.shared.info("===== VPNMenuBar launch v\(version) (\(build)) arch=\(arch) os=\(os) bundle=\(bundle.bundlePath) =====")
115+
}
116+
106117
private func requestNotificationPermission() {
107118
UNUserNotificationCenter.current().requestAuthorization(options: [.alert, .sound]) { _, _ in }
108119
}

VPNMenuBar/Config/ConfigStore.swift

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,8 +27,12 @@ final class ConfigStore {
2727
guard fm.fileExists(atPath: configURL.path) else { return nil }
2828
let data = try Data(contentsOf: configURL)
2929
do {
30-
return try JSONDecoder().decode(VPNConfig.self, from: data)
30+
// Sanitize on the way out so legacy configs written before the
31+
// whitespace-stripping was added still surface clean values to
32+
// connect() without waiting for a user-initiated save.
33+
return try JSONDecoder().decode(VPNConfig.self, from: data).sanitized()
3134
} catch {
35+
AppLogger.shared.error("config decode failed (\(error)) — backing up broken file")
3236
try backupBrokenFile()
3337
return nil
3438
}
@@ -40,7 +44,7 @@ final class ConfigStore {
4044

4145
let encoder = JSONEncoder()
4246
encoder.outputFormatting = [.prettyPrinted, .sortedKeys]
43-
let data = try encoder.encode(config)
47+
let data = try encoder.encode(config.sanitized())
4448

4549
// Atomic write: temp file + rename.
4650
let tmpURL = configURL.appendingPathExtension("tmp")

VPNMenuBar/Config/VPNConfig.swift

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,4 +20,34 @@ struct VPNConfig: Codable, Equatable {
2020
!username.isEmpty && !passwordPrefix.isEmpty && !totpSecret.isEmpty
2121
&& !gateway.isEmpty && !serverCertPin.isEmpty
2222
}
23+
24+
/// Strip surrounding whitespace and invisible characters (zero-width, BOM,
25+
/// newlines, NBSP, ideographic space) that commonly get pasted in from
26+
/// chat apps / PDFs and silently break `openconnect` arguments.
27+
func sanitized() -> VPNConfig {
28+
var copy = self
29+
copy.username = VPNConfig.cleanField(username)
30+
copy.passwordPrefix = VPNConfig.cleanField(passwordPrefix)
31+
copy.totpSecret = VPNConfig.cleanField(totpSecret)
32+
copy.gateway = VPNConfig.cleanField(gateway)
33+
copy.serverCertPin = VPNConfig.cleanField(serverCertPin)
34+
copy.openconnectPath = VPNConfig.cleanField(openconnectPath)
35+
copy.vpncScriptPath = VPNConfig.cleanField(vpncScriptPath)
36+
return copy
37+
}
38+
39+
private static let invisibleScalars: Set<Unicode.Scalar> = [
40+
"\u{200B}", "\u{200C}", "\u{200D}", "\u{FEFF}",
41+
"\u{2028}", "\u{2029}", "\u{00A0}", "\u{3000}",
42+
]
43+
44+
private static func cleanField(_ value: String) -> String {
45+
let scalars = value.unicodeScalars.filter { scalar in
46+
if invisibleScalars.contains(scalar) { return false }
47+
if scalar.value < 0x20 || scalar.value == 0x7F { return false }
48+
return true
49+
}
50+
return String(String.UnicodeScalarView(scalars))
51+
.trimmingCharacters(in: .whitespacesAndNewlines)
52+
}
2353
}

0 commit comments

Comments
 (0)