feat(mac): menu bar connector overhaul + inline sign-in card#774
Merged
Conversation
Reshape the menu bar popover so connectors and sign-in feel native. Connectors UX - Replace +Add buttons with switch toggles on Photos / Health / Screen Time / WhatsApp. - Local folder + Google Chrome rows now open a native NSMenu cascading from the row's right edge (was a SwiftUI popover balloon that drifted off-screen near the menu bar edge). Click anywhere on the row to open. - Obsidian connector: auto-discovers vaults from ~/Library/Application Support/ obsidian/obsidian.json, lists them as checkmark items, reuses the existing local-folder bookmark pipeline. iCloud-backed vaults that fail a directory read are flagged "(needs Full Disk Access)" inline so toggling them on doesn't silently no-sync. - Hide WhatsApp + Obsidian rows entirely when the app isn't installed, matching the existing browser-row behavior. - Rename the section "Integrations" -> "Device connectors" to match the app.lobu.ai web UI label. Disclosure consistency - Inbox and Recent activity become collapsible disclosures with a count next to the header (e.g. "INBOX 3", "RECENT ACTIVITY 8"). Default collapsed. - Device connectors header gains the same count treatment. Sign-in flow - Kill the dedicated sign-in screen. Always show the main popover; render a compact connection card (URL field + Connect button) when signed-out. - Drop the Cloud / Self-hosted / Local picker; one editable URL field instead. Localhost URLs auto-start the embedded server before OAuth, anything else just OAuths. - Default URL is http://localhost:8787. Existing users keep their stored preference via a migration in the @published initializer. - "Run on this Mac" remains the only path that auto-spawns lobu run. Misc - Fix LobuUpdater wedging on "Checking..." forever when Sparkle finds no update. updaterDidNotFindUpdate now sets latestVersion to the current bundle version so the footer flips to "Up to date". - Add docs/plans/personal-mode-auth.md design doc covering the next step (auth bypass for menu-bar-spawned localhost server).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR modernizes the macOS Lobu menu bar app to support personal-mode authentication with an embedded local server. It introduces Obsidian vault enumeration, consolidates server modes to a URL-driven connection flow, replaces SwiftUI menus with native NSMenu infrastructure, and redesigns the menu bar UI with collapsible sections and unified integration rows. ChangesMenu bar app modernization and personal-mode auth foundation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 782986850b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+182
to
+183
| @Published var customServerDraft: String = UserDefaults.standard.string(forKey: "lobuCustomServerURL") | ||
| ?? "http://localhost:8787" { |
There was a problem hiding this comment.
Existing users who had lobuServerMode == "cloud" typically do not have lobuCustomServerURL set, so after this migration they are classified as .remote but the sign-in field still falls back to http://localhost:8787. If they sign out or need to reconnect, the next Connect will start/sign into a local server instead of Lobu Cloud, which is exactly the case the migration comment says it is preserving; seed this draft from cloudURL when migrating old cloud installs or keep a separate persisted URL per mode.
Useful? React with 👍 / 👎.
Comment on lines
+744
to
+745
| } else if vault.isReadable { | ||
| state.addFolderBookmark(url: vault.url) |
There was a problem hiding this comment.
Toggling an Obsidian vault just adds the vault root as a regular local folder, but LocalDirectorySyncService only does a shallow contentsOfDirectory pass and documents that it does not recurse. Any vault with notes under subfolders (a common Obsidian layout) will appear enabled while most of its notes never sync, so the Obsidian path needs a recursive mode or a dedicated sync path instead of reusing the shallow local-folder feed unchanged.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
apps/mac/Lobu/BrowserProfilesView.swift (1)
215-226: 💤 Low valueRemove unused
profileBinding(for:)helper.This binding was used with the prior SwiftUI
Menubut is now dead code—the nativeNSMenuitems callmirror/deletedirectly via closures inbuildChromeMenu().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/mac/Lobu/BrowserProfilesView.swift` around lines 215 - 226, Remove the dead helper profileBinding(for:) and any unused supporting symbols only used by it; specifically delete the private function profileBinding(for:) and ensure no callers remain (it was previously used by the old SwiftUI Menu). Verify references to mirroredProfile(for:), mirror(_:), and delete(_:) remain intact for buildChromeMenu() which now drives NSMenu actions directly, and remove any imports or comments that only existed for the removed binding.apps/mac/Lobu/MenuBarContent.swift (1)
465-471: 💤 Low valueInconsistent localhost detection with
AppState.connect().
connectButtonTitleuses substring matching (url.contains("localhost")) whileAppState.connect()properly parses the URL host. A malformed URL like "http://notlocalhost.com" would show "Start & sign in" but not actually start the local server.Consider aligning with the same URL-parsing approach used in
connect()for consistency:♻️ Suggested alignment
private var connectButtonTitle: String { if state.isLoggingIn { return "Waiting for approval…" } let url = state.customServerDraft.trimmingCharacters(in: .whitespacesAndNewlines) - let isLocal = url.contains("localhost") || url.contains("127.0.0.1") + let parsed = URL(string: url) + let isLocal = parsed?.host.map { $0 == "localhost" || $0 == "127.0.0.1" } ?? false if isLocal && !state.localLobuStatus.isRunning { return "Start & sign in" } return "Sign in" }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/mac/Lobu/MenuBarContent.swift` around lines 465 - 471, connectButtonTitle currently detects "localhost" via substring matching on state.customServerDraft which is inconsistent with AppState.connect() that parses the URL host; change connectButtonTitle to parse the draft into a URL and inspect url.host (or treat invalid URL as non-local) the same way AppState.connect() does, and keep the existing check against state.localLobuStatus.isRunning; reference the connectButtonTitle computed property, state.customServerDraft, AppState.connect(), and state.localLobuStatus.isRunning when making the change.docs/plans/personal-mode-auth.md (4)
110-113: 💤 Low valueClarify localhost resolution validation scope.
Line 112 mentions rejecting "
localhostresolutions that aren't loopback" but line 111'sserver.address()check only verifies what address was actually bound, not DNS resolution behavior. If the server accepts--bind localhostas input, it should resolve that to an IP before binding and verify it's loopback. The post-listenserver.address()assertion catches misconfigurations but won't detect if someone configured DNS to point localhost elsewhere (though the bind would have already happened by then).📝 Suggested clarification
- Server reads `LOBU_PERSONAL_MODE=1` at boot. If set: - - Refuse to start unless `bind` is `127.0.0.1` or `::1`. + - Refuse to start unless `bind` is `127.0.0.1` or `::1` (if the user passes `localhost`, resolve it and verify it's loopback before binding). - After `server.listen()`, call `server.address()` and assert the result is loopback. Crash with a clear message if not. - - Reject `0.0.0.0`, `::`, external interface IPs, and `localhost` resolutions that aren't loopback (some custom `/etc/hosts` setups). + - Reject `0.0.0.0`, `::`, and external interface IPs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/plans/personal-mode-auth.md` around lines 110 - 113, The post-listen server.address() check in start-local.ts (around start-local.ts:82) is insufficient to validate "--bind localhost" input because DNS can resolve "localhost" to a non-loopback IP; resolve the provided bind host to an IP before calling server.listen(), and assert that the resolved IP is a loopback address (127.0.0.0/8 or ::1) rejecting inputs like "localhost" when DNS returns a non-loopback result; retain the existing post-listen server.address() assertion as a safety check but add a pre-bind resolution + loopback check for the bind string handling code that parses the CLI --bind value.
93-93: ⚡ Quick winClarify Map.delete() atomicity assumptions.
The design relies on
Map.delete()return value to prevent double-submit races. This works correctly in Node.js's single-threaded event loop, but the document should explicitly note this dependency. If the server were to adopt worker threads or clustering in the future, the in-memory Map would need to be replaced with a shared atomic store (Redis, etc.).📝 Suggested clarification
Add a note after line 93:
Server hashes the supplied token, atomically deletes the entry from the in-memory map (the `Map.delete()` return value is the only authoritative "is this the first call" signal — prevents double-submit races), verifies TTL, then calls Better Auth to mint a session for the local user. +(Note: This relies on Node.js's single-threaded event loop. If clustering or worker threads are introduced, the token store must migrate to a shared atomic backend like Redis.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/plans/personal-mode-auth.md` at line 93, Add a short clarifying note after the sentence that references Map.delete(): state that the atomicity of using the Map.delete() return value to detect first-use relies on Node.js's single-threaded event loop and is only safe for a single-process in-memory Map; also instruct that if the server is moved to worker threads, clustering, or multiple processes the in-memory Map must be replaced with a shared atomic store (e.g., Redis, other external datastore or coordinator) that provides atomic delete/check semantics to prevent double-submit races.
226-230: ⚡ Quick winClarify stdout protocol format.
Line 229 says "parse port + warning" but doesn't specify the format. Based on lines 67 and 146, the protocol is separate lines (
LOBU_LISTEN_PORT=<port>\nthen optionallyLOBU_WARNING=<msg>\n). The implementation section should explicitly document this line-oriented protocol format to avoid ambiguity.📝 Suggested clarification
- Attach stdin pipe, write the handshake JSON on spawn, close write half. - - Read first stdout line, parse port + warning. + - Read stdout lines: first line is `LOBU_LISTEN_PORT=<port>`, optionally followed by `LOBU_WARNING=<message>` (see lines 67, 146 for protocol details). - Persist port to `~/.lobu-menubar/<NSUserName>/port` for crash recovery.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/plans/personal-mode-auth.md` around lines 226 - 230, Update the "parse port + warning" sentence to explicitly document the line-oriented stdout protocol: the process writes a separate line "LOBU_LISTEN_PORT=<port>\n" and, if present, a separate line "LOBU_WARNING=<message>\n" (each terminated by newline); the reader should read stdout line-by-line, parse the LOBU_LISTEN_PORT value first, then handle an optional LOBU_WARNING line and persist the port as described. Reference the literal tokens LOBU_LISTEN_PORT and LOBU_WARNING and the phrase "parse port + warning" when making the change so it's clear where to update.
128-136: 💤 Low valueClarify CORS preflight denial mechanism.
Lines 128 and 135 both describe cross-origin preflight handling but use different terminology ("does not include" vs "deny silently"). For implementation clarity, specify that "denying a preflight" means either (a) responding with 403 Forbidden, or (b) omitting the required
Access-Control-Allow-*headers, causing the browser to fail the preflight.📝 Suggested clarification
-- **CORS:** allowed origins are exactly `http://127.0.0.1:<port>` and `http://localhost:<port>`. No wildcards. No reflection of `Origin`. `Access-Control-Allow-Headers` does **not** include `X-Lobu-Client` or `Authorization` for cross-origin preflights — a foreign tab's preflight fails before the actual request runs. +- **CORS:** allowed origins are exactly `http://127.0.0.1:<port>` and `http://localhost:<port>`. No wildcards. No reflection of `Origin`. For cross-origin preflights, either respond with 403 or omit/restrict `Access-Control-Allow-Headers` and `Access-Control-Allow-Origin` so that `X-Lobu-Client` and `Authorization` are not grantable — a foreign tab's preflight fails before the actual request runs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/plans/personal-mode-auth.md` around lines 128 - 136, The CORS preflight denial language is ambiguous; update the CORS/OPTIONS docs (the bullet describing Access-Control-Allow-Headers and the "OPTIONS: preflight" bullet) to explicitly define "deny preflight" as either (a) respond to the preflight with 403 Forbidden, or (b) for silent denial, omit the required Access-Control-Allow-* headers so the browser fails the preflight; also state which approach is used by default for cross-origin preflights (recommendation: omit headers for silent denial, 403 for explicit denial of actual mutating requests) and reference the exact bullets "CORS:" and "OPTIONS:" and the Missing-`Origin` behavior so implementers know when to apply each denial mode.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/mac/Lobu/LobuUpdater.swift`:
- Around line 71-80: The abort handler updater(_:didAbortWithError:) sets
latestVersion but forgets to clear the update flag; updateAvailable should be
set to false just like in updaterDidNotFindUpdate so the UI shows "Up to date".
Edit the Task { `@MainActor` in ... } block in updater(_:didAbortWithError:) to
also assign self.updateAvailable = false (after setting self.latestVersion) to
mirror the existing no-update path and ensure consistent UI state.
In `@apps/mac/Lobu/ObsidianVaultManager.swift`:
- Around line 18-19: The exists property currently uses
FileManager.default.fileExists(atPath: path) which returns true for files too;
change it to use FileManager.default.fileExists(atPath:isDirectory:) (or
FileManager.attributesOfItem and check type) and ensure isDirectory == true so
only directories pass the .filter(\.exists) gate; update the exists computed
property in ObsidianVaultManager (and the similar check referenced at the second
occurrence) to return true only for directory paths.
In `@docs/plans/personal-mode-auth.md`:
- Around line 129-132: The CSRF policy only covers mutating routes but misses
sensitive GET endpoints; update the doc and policy to require the same Origin
checks for any GET routes that return sensitive user data by treating them like
mutating routes: require Sec-Fetch-Site: same-origin or none OR an allowed
Origin, or accept X-Lobu-Client: menubar + valid Bearer for native clients;
alternatively add a global requirement that all authenticated API routes
(referenced in the doc as Sec-Fetch-Site / Origin checks) must include a custom
header (e.g., X-Lobu-Client or a web-app-specific header) to be accepted, and
enumerate that change where Sec-Fetch-Site, Origin, and X-Lobu-Client are
described.
---
Nitpick comments:
In `@apps/mac/Lobu/BrowserProfilesView.swift`:
- Around line 215-226: Remove the dead helper profileBinding(for:) and any
unused supporting symbols only used by it; specifically delete the private
function profileBinding(for:) and ensure no callers remain (it was previously
used by the old SwiftUI Menu). Verify references to mirroredProfile(for:),
mirror(_:), and delete(_:) remain intact for buildChromeMenu() which now drives
NSMenu actions directly, and remove any imports or comments that only existed
for the removed binding.
In `@apps/mac/Lobu/MenuBarContent.swift`:
- Around line 465-471: connectButtonTitle currently detects "localhost" via
substring matching on state.customServerDraft which is inconsistent with
AppState.connect() that parses the URL host; change connectButtonTitle to parse
the draft into a URL and inspect url.host (or treat invalid URL as non-local)
the same way AppState.connect() does, and keep the existing check against
state.localLobuStatus.isRunning; reference the connectButtonTitle computed
property, state.customServerDraft, AppState.connect(), and
state.localLobuStatus.isRunning when making the change.
In `@docs/plans/personal-mode-auth.md`:
- Around line 110-113: The post-listen server.address() check in start-local.ts
(around start-local.ts:82) is insufficient to validate "--bind localhost" input
because DNS can resolve "localhost" to a non-loopback IP; resolve the provided
bind host to an IP before calling server.listen(), and assert that the resolved
IP is a loopback address (127.0.0.0/8 or ::1) rejecting inputs like "localhost"
when DNS returns a non-loopback result; retain the existing post-listen
server.address() assertion as a safety check but add a pre-bind resolution +
loopback check for the bind string handling code that parses the CLI --bind
value.
- Line 93: Add a short clarifying note after the sentence that references
Map.delete(): state that the atomicity of using the Map.delete() return value to
detect first-use relies on Node.js's single-threaded event loop and is only safe
for a single-process in-memory Map; also instruct that if the server is moved to
worker threads, clustering, or multiple processes the in-memory Map must be
replaced with a shared atomic store (e.g., Redis, other external datastore or
coordinator) that provides atomic delete/check semantics to prevent
double-submit races.
- Around line 226-230: Update the "parse port + warning" sentence to explicitly
document the line-oriented stdout protocol: the process writes a separate line
"LOBU_LISTEN_PORT=<port>\n" and, if present, a separate line
"LOBU_WARNING=<message>\n" (each terminated by newline); the reader should read
stdout line-by-line, parse the LOBU_LISTEN_PORT value first, then handle an
optional LOBU_WARNING line and persist the port as described. Reference the
literal tokens LOBU_LISTEN_PORT and LOBU_WARNING and the phrase "parse port +
warning" when making the change so it's clear where to update.
- Around line 128-136: The CORS preflight denial language is ambiguous; update
the CORS/OPTIONS docs (the bullet describing Access-Control-Allow-Headers and
the "OPTIONS: preflight" bullet) to explicitly define "deny preflight" as either
(a) respond to the preflight with 403 Forbidden, or (b) for silent denial, omit
the required Access-Control-Allow-* headers so the browser fails the preflight;
also state which approach is used by default for cross-origin preflights
(recommendation: omit headers for silent denial, 403 for explicit denial of
actual mutating requests) and reference the exact bullets "CORS:" and "OPTIONS:"
and the Missing-`Origin` behavior so implementers know when to apply each denial
mode.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 810658ef-d9ac-4de7-927e-b63eb5ee5d90
📒 Files selected for processing (7)
apps/mac/Lobu.xcodeproj/project.pbxprojapps/mac/Lobu/AppState.swiftapps/mac/Lobu/BrowserProfilesView.swiftapps/mac/Lobu/LobuUpdater.swiftapps/mac/Lobu/MenuBarContent.swiftapps/mac/Lobu/ObsidianVaultManager.swiftdocs/plans/personal-mode-auth.md
Comment on lines
+71
to
80
| nonisolated func updater(_ updater: SPUUpdater, didAbortWithError error: Error) { | ||
| // Same idea on network errors / appcast parse failures: surface "Up to | ||
| // date" rather than wedging the UI. Sparkle has already logged the | ||
| // detail; the menu-bar footer doesn't need to spell it out. | ||
| let current = Bundle.main | ||
| .infoDictionary?["CFBundleShortVersionString"] as? String | ||
| Task { @MainActor in | ||
| self.latestVersion = current | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing updateAvailable = false assignment.
When an update check aborts with an error, latestVersion is set to the current version but updateAvailable is not cleared. If a previous check had found an update (updateAvailable = true), the UI will remain in an inconsistent state—showing "Update to v1.0.0" when the user is already on v1.0.0.
The comment confirms the intent is to show "Up to date" rather than wedging the UI, which requires clearing the update flag. This mirrors the logic in updaterDidNotFindUpdate above.
🔧 Proposed fix
let current = Bundle.main
.infoDictionary?["CFBundleShortVersionString"] as? String
Task { `@MainActor` in
+ self.updateAvailable = false
self.latestVersion = current
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/mac/Lobu/LobuUpdater.swift` around lines 71 - 80, The abort handler
updater(_:didAbortWithError:) sets latestVersion but forgets to clear the update
flag; updateAvailable should be set to false just like in
updaterDidNotFindUpdate so the UI shows "Up to date". Edit the Task { `@MainActor`
in ... } block in updater(_:didAbortWithError:) to also assign
self.updateAvailable = false (after setting self.latestVersion) to mirror the
existing no-update path and ensure consistent UI state.
Comment on lines
+18
to
+19
| var exists: Bool { FileManager.default.fileExists(atPath: path) } | ||
|
|
There was a problem hiding this comment.
Filter vault entries to directories, not any filesystem node.
fileExists(atPath:) is true for regular files too, so invalid path values can pass the .filter(\.exists) gate and show up as vaults.
Suggested fix
- var exists: Bool { FileManager.default.fileExists(atPath: path) }
+ var exists: Bool {
+ var isDirectory: ObjCBool = false
+ guard FileManager.default.fileExists(atPath: path, isDirectory: &isDirectory) else {
+ return false
+ }
+ return isDirectory.boolValue
+ }Also applies to: 59-59
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/mac/Lobu/ObsidianVaultManager.swift` around lines 18 - 19, The exists
property currently uses FileManager.default.fileExists(atPath: path) which
returns true for files too; change it to use
FileManager.default.fileExists(atPath:isDirectory:) (or
FileManager.attributesOfItem and check type) and ensure isDirectory == true so
only directories pass the .filter(\.exists) gate; update the exists computed
property in ObsidianVaultManager (and the similar check referenced at the second
occurrence) to return true only for directory paths.
Comment on lines
+129
to
+132
| - **Origin / Sec-Fetch-Site:** all mutating routes (POST/PUT/PATCH/DELETE) require either: | ||
| - `Sec-Fetch-Site: same-origin` or `Sec-Fetch-Site: none` (no-CORS, navigation), OR | ||
| - `Origin` present and in the allowed list above. | ||
| - **Missing-`Origin` behavior:** native clients (the Mac app) often omit `Origin`. For mutating routes, require either (a) `Origin` present-and-allowed, OR (b) `X-Lobu-Client: menubar` + valid Bearer (the menu bar path). Pure missing-Origin without the Lobu client header → 403. |
There was a problem hiding this comment.
Missing protection for sensitive GET requests.
The CSRF protection focuses on "mutating routes (POST/PUT/PATCH/DELETE)" but does not address sensitive GET endpoints. An attacker can initiate cross-origin GET requests without triggering a preflight (simple request), potentially exfiltrating data if the API returns sensitive information on GET routes that rely solely on cookie-based authentication.
🔒 Recommended additional protection
Add to the CSRF section:
- **Missing-`Origin` behavior:** native clients (the Mac app) often omit `Origin`. For mutating routes, require either (a) `Origin` present-and-allowed, OR (b) `X-Lobu-Client: menubar` + valid Bearer (the menu bar path). Pure missing-Origin without the Lobu client header → 403.
+- **Sensitive GET routes:** Apply the same Origin/Sec-Fetch-Site checks to GET routes that return sensitive data. The Better Auth session cookie alone is not sufficient CSRF protection for read operations in a localhost context where an attacker tab can make same-origin requests.
- **`Host` allowlist:** request `Host` header must be `127.0.0.1[:port]` or `localhost[:port]` — reject DNS rebinding attacks.Alternatively, consider requiring X-Lobu-Client: menubar (or a custom header from the web app) on all API routes, not just mutations, since the web app can easily set it.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - **Origin / Sec-Fetch-Site:** all mutating routes (POST/PUT/PATCH/DELETE) require either: | |
| - `Sec-Fetch-Site: same-origin` or `Sec-Fetch-Site: none` (no-CORS, navigation), OR | |
| - `Origin` present and in the allowed list above. | |
| - **Missing-`Origin` behavior:** native clients (the Mac app) often omit `Origin`. For mutating routes, require either (a) `Origin` present-and-allowed, OR (b) `X-Lobu-Client: menubar` + valid Bearer (the menu bar path). Pure missing-Origin without the Lobu client header → 403. | |
| - **Origin / Sec-Fetch-Site:** all mutating routes (POST/PUT/PATCH/DELETE) require either: | |
| - `Sec-Fetch-Site: same-origin` or `Sec-Fetch-Site: none` (no-CORS, navigation), OR | |
| - `Origin` present and in the allowed list above. | |
| - **Missing-`Origin` behavior:** native clients (the Mac app) often omit `Origin`. For mutating routes, require either (a) `Origin` present-and-allowed, OR (b) `X-Lobu-Client: menubar` + valid Bearer (the menu bar path). Pure missing-Origin without the Lobu client header → 403. | |
| - **Sensitive GET routes:** Apply the same Origin/Sec-Fetch-Site checks to GET routes that return sensitive data. The Better Auth session cookie alone is not sufficient CSRF protection for read operations in a localhost context where an attacker tab can make same-origin requests. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/plans/personal-mode-auth.md` around lines 129 - 132, The CSRF policy
only covers mutating routes but misses sensitive GET endpoints; update the doc
and policy to require the same Origin checks for any GET routes that return
sensitive user data by treating them like mutating routes: require
Sec-Fetch-Site: same-origin or none OR an allowed Origin, or accept
X-Lobu-Client: menubar + valid Bearer for native clients; alternatively add a
global requirement that all authenticated API routes (referenced in the doc as
Sec-Fetch-Site / Origin checks) must include a custom header (e.g.,
X-Lobu-Client or a web-app-specific header) to be accepted, and enumerate that
change where Sec-Fetch-Site, Origin, and X-Lobu-Client are described.
- connect() now only auto-starts the embedded lobu runner when the URL
matches the runner's exact host:port (LocalLobuRunner.baseURL). A user
typing http://localhost:9999 used to silently start :8787 and then OAuth
against :9999, which would fail. Now we only auto-start when it's our
managed instance.
- Reject non-http(s) URL schemes up front. file:/ftp:/custom schemes used
to be persisted as baseURL and then fail somewhere deeper in the stack.
- Add AppState.isLoopback(_:URL) — URL.host-parsing loopback check shared
between connect() and MenuBarContent's button-title logic so they can't
disagree on the same URL. Previously the button title did substring
contains("localhost") which classified https://localhost.evil.com as
local while connect() correctly classified it as remote.
- Migrate ex-cloud users to keep the Lobu Cloud URL when their stored
customServerDraft is empty. Without this, the previous PR's "remove
Cloud picker" change would silently point cloud users at localhost on
next sign-out because customServerDraft defaulted to localhost.
- Obsidian NSMenu now shows the full vault path (collapsed to ~) next to
the display name so users can verify what they'd actually sync. A stale
or malicious obsidian.json could point "Notes" at any folder; the user
needs to see the path before they click toggle.
…hardening Tightens the loopback / auto-start logic after pi's second pass. - AppState.matchesManagedRunner(_:URL) replaces the loose isLoopback check. Returns true only when scheme + canonical-loopback host + effective port all match LocalLobuRunner.baseURL exactly. localhost / 127.0.0.1 / ::1 / [::1] all normalize to one canonical form so they compare equal. - connect() now sets serverMode = .local only when the URL matches the managed runner — not for any loopback URL. Previously, signing in against localhost:9999 would still set .local and cause the next app launch's auto-start path to spawn :8787 alongside. - Reject http(s) URLs with no host (e.g. bare "https://") up front instead of letting them fail at OAuth time with a worse error. - Button title now mirrors matchesManagedRunner exactly so it never says "Start & sign in" for a URL the runner won't actually start. - customServerDraft default cascade now also reads the legacy lobuBaseURL UserDefault, so users whose URL lived there (pre-merge custom mode) get their URL back instead of falling through to the cloud default. - Replace plain substring `replacingOccurrences(of: NSHomeDirectory(), with: "~")` with a prefix-aware abbreviatedHomePath() helper. Plain substring mangles /Users/burakemre.backup/foo into ~.backup/foo. Used in both the Obsidian vault menu and the Local folder menu.
This was referenced May 16, 2026
buremba
added a commit
that referenced
this pull request
May 28, 2026
Pi flagged that dropping the build-worker dependency also dropped the transitive connector-parity-smoke gate, which is the runtime self-check that catches worker images that crash on boot (the failure mode #774 was added to prevent). Restore connector-parity-smoke as a direct build-app dependency. Still parallel with build-worker and build-embeddings-service for the ~7-10min savings — just guards against the higher-severity 'broken worker image gets shipped silently' class instead of the lower-severity 'app tag appears 30s before worker tag' class.
buremba
added a commit
that referenced
this pull request
May 28, 2026
Pi flagged that dropping the build-worker dependency also dropped the transitive connector-parity-smoke gate, which is the runtime self-check that catches worker images that crash on boot (the failure mode #774 was added to prevent). Restore connector-parity-smoke as a direct build-app dependency. Still parallel with build-worker and build-embeddings-service for the ~7-10min savings — just guards against the higher-severity 'broken worker image gets shipped silently' class instead of the lower-severity 'app tag appears 30s before worker tag' class.
buremba
added a commit
that referenced
this pull request
May 28, 2026
…ster deploys (#1128) * ci: drop arm64 from main pushes + parallelize build-app for ~30min faster deploys Today main → prod takes ~40min and has a recurring disk-full failure mode (PR #1116's build-app failed mid-rollout earlier today on 'no space left on device' during the arm64 image export). Two scoped changes that compound: ## Drop arm64 on main pushes Prod is single-node Hetzner cpx41 — x86_64 only. Building linux/arm64 via QEMU on every main push is pure waste: - ~15-20min of emulated build time per main push - Doubles Playwright/Chrome binary downloads (one set per arch) which is what fills the ~14GB GHA runner disk - No prod node ever pulls the arm64 manifest The arm64 leg is still available on demand: workflow_dispatch now takes a 'platforms' input defaulting to linux/amd64 — a dev who needs the multi-arch image for an arm64 machine can re-run with 'linux/amd64,linux/arm64'. ## Parallelize build-app build-app previously had needs: [build-worker, build-embeddings-service] to keep all three image tags appearing on ghcr in sync (Flux watches only the app policy but rolls the shared tag to all three Deployments). The serial gate cost ~7-10min critical path. In practice build-app is already the longest job and finishes 2-3min after build-worker even when launched in parallel — so the new app tag appears AFTER the worker/embeddings tags in nearly every case. kubelet's image-pull back-off handles the rare outlier (pod retries until the tag exists, no permanent failure). ## Expected outcome - Per-job: build-app ~25min → ~8min (no QEMU arm64) - Critical path: ~40min → ~10-12min - Disk pressure on the runner halved, eliminating the failure mode that nearly stranded today's CORS rollout Validation will be empirical on the first main push after this lands. * ci: keep connector-parity-smoke gate on build-app (address pi blocker) Pi flagged that dropping the build-worker dependency also dropped the transitive connector-parity-smoke gate, which is the runtime self-check that catches worker images that crash on boot (the failure mode #774 was added to prevent). Restore connector-parity-smoke as a direct build-app dependency. Still parallel with build-worker and build-embeddings-service for the ~7-10min savings — just guards against the higher-severity 'broken worker image gets shipped silently' class instead of the lower-severity 'app tag appears 30s before worker tag' class. * ci: restore sibling-build dependency gate on build-app (address pi v2 blocker) Pi v2 flagged that the parallelization win opens a real failure window: even with the connector-parity-smoke gate in place, a failed build-worker or build-embeddings-service (disk pressure, registry hiccup, unrelated Dockerfile regression) lets build-app still publish the Flux-watched shared tag — Flux then rolls a release to a tag whose sibling images don't exist. The ~7min critical-path saving isn't worth that risk window. Reverted to the safe gate. The arm64-drop alone still cuts ~15-20min off the critical path (the bigger lever anyway), and addresses today's disk-full incident root cause directly. Net: PR is now the simpler 'drop arm64 from main pushes' change. Same ~25min → ~10-12min critical-path improvement was overoptimistic; realistic outcome is ~25min → ~17-20min, no reliability regression. * chore: bump owletto pointer to current main (clear check-drift) * chore: point owletto at current main (b05d2fa) to clear check-drift
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
2 participants
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
Test plan
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
Documentation