fix(auth): offer the API-key login on server-mode admin 403s - #1569
fix(auth): offer the API-key login on server-mode admin 403s#1569paoloantinori wants to merge 2 commits into
Conversation
…sh#1568) Since debpalash#1525 the /system router sits behind require_admin, so an unauthenticated non-loopback browser gets 403 where it used to get 401. The SPA routed only 401s into the ov:auth-required flow, so a remote browser spammed 403s on /sysinfo, /model/status, /system/notifications forever (polling hooks retry infinitely) and was never offered the master-for-session exchange. Frontend: route a 403 whose detail names the admin API key into the existing apikey gate; other 403s (CSRF, desktop-only) stay plain errors because no key can satisfy them. Backend: _admin_gate_403() emits a mode-distinct detail — server mode keeps "loopback origin or admin API key required" (a key genuinely satisfies it), desktop mode answers "loopback origin required" (truthful: the credential checks only run under server mode, so routing desktop guests to a key form would loop forever). tests/test_auth_gate_detail_lockstep.py locks the literal contract across layers so a backend reword fails CI unless client.ts moves with it. Verified end-to-end on a live LAN deployment: gate renders after 3 initial 403s, key exchange produces a session, all polls 200.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughChangesThe backend now returns mode-specific admin-gate 403 details. Server-mode API-key errors enter the frontend authentication flow. Desktop, CSRF, and unrelated 403 responses remain ordinary errors. Tests and documentation cover the behavior. Admin 403 authentication
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change routes server-mode admin 403 responses to the API-key login while preserving non-fixable 403 errors; no actionable merge-blocking risk remains after normal checks and review. Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 8 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (8 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
| Filename | Overview |
|---|---|
| backend/api/dependencies.py | The new detail helper accurately names the API key only when server mode has a configured key that can satisfy the administrator gate. |
| frontend/src/api/client.ts | Administrator-gate 403 routing and request-token-aware session invalidation address both previously reported authentication failures. |
| frontend/src/api/client.test.ts | Tests cover administrator 403 routing, unrelated 403 exclusion, and preservation of a session created while a stale request is in flight. |
| tests/test_loopback_server_mode.py | Tests pin desktop, API-key server, and PIN-only server detail behavior for both administrator dependencies. |
| tests/test_auth_gate_detail_lockstep.py | The cross-layer test protects the literal discriminator between backend details and frontend authentication routing. |
Reviews (2): Last reviewed commit: "fix(auth): review follow-ups for #1569 —..." | Re-trigger Greptile
…e-403 race Greptile P1 #1 (unsatisfiable PIN-only gate): in server mode with a share PIN but no OMNIVOICE_API_KEY, the PIN closes read-only bootstrap while no key exists to present — the key-naming detail sent the browser to a login form whose exchange can never succeed, hiding the whole app behind an unrecoverable gate. _admin_gate_403() now names the key only when server mode AND an API key is configured; PIN-only and bare servers answer the plain loopback detail, which is truthful (only loopback can use admin when no key exists) and keeps the SPA out of the gate. Greptile P1 debpalash#2 (stale 403 clears new session): a late 403/401 landing after a key exchange wiped the fresh session via the unconditional clearAdminSession(), reloading a successful login straight back into the gate. A failed response may now only invalidate the credentials it actually carried (send-time session capture + token compare before clearing). Also adds the docstrings CodeRabbit's coverage check asked for on the new test functions.
|
The red Tests (backend + frontend) check is pre-existing on |
Fixes #1568
What
client.ts): a 403 whose detail names the admin API key now routes into the existingov:auth-requiredflow (mode: 'apikey'), exactly like the 401 paths —RemoteAuthGateshows the API-key form, the app tree unmounts, and the infinite-retry polling stops instead of spamming the console. Other 403s (CSRF "browser origin rejected", desktop-only routes) deliberately stay plain errors, because presenting a key cannot satisfy them.dependencies.py): new_admin_gate_403()emits a mode-distinct detail. Server mode keeps"loopback origin or admin API key required"(a key genuinely satisfies the gate there); desktop mode now answers"loopback origin required"— truthful, since the admin-credential checks only run under_server_mode(), so no presented key can ever satisfy the desktop gate. Without this split, routing on the message alone would trap a desktop PIN-share guest in a login form that can never succeed.tests/test_auth_gate_detail_lockstep.py): pins the literal contract across layers — the frontend matcher ("admin api key") must keep matching the backend's server-mode detail and must NOT match the details a key cannot fix. A backend reword without updatingclient.tsfails CI instead of silently regressing to 403 spam.Tests
client.test.ts: admin-gate 403 dispatches{mode:"apikey"}; CSRF 403 does not dispatch (fail-before/pass-after).test_loopback_server_mode.py: per-mode detail forrequire_admin+require_admin_action(desktop → plain loopback, server+key → key-naming).test_engines_route_shape.pyto the new truthful detail.main: backend 208 passed, frontend api 95 passed;oxfmt --checkclean.Verification
On a live LAN deployment (server mode +
OMNIVOICE_API_KEY): before, a remote browser logged/sysinfo,/model/status,/system/notifications403s every few seconds indefinitely. After: 3 initial 403s, the API-key gate renders, the exchange produces a short-lived session, and all polls return 200.An adversarial review pass caught the first version of the frontend-only fix breaking desktop LAN-share guests (unsatisfiable key form); the mode-distinct backend detail is the root-cause fix for that class.
Server-mode admin 403 responses now trigger API-key authentication, while unrelated 403 responses remain errors and stale responses cannot clear newer sessions. Backend details now reflect mode and API-key availability, with cross-layer tests covering routing and recovery. Review the frontend/backend error-detail matching because mismatches can break authentication routing.
Review follow-ups (commit e2bfeaa)
Greptile's review on 014e18c flagged two P1s, both fixed with fail-before/pass-after tests:
dependencies.py): server mode + share PIN + noOMNIVOICE_API_KEYclosed read-only bootstrap while no key existed to present, so the key-naming detail gated the whole UI behind a form whose exchange can never succeed._admin_gate_403()now names the key only when server mode and an API key is configured; PIN-only and bare servers answer the plain loopback detail (truthful: only loopback can use admin when no key exists), which the SPA leaves a plain error. New test:test_require_admin_pin_only_server_mode_detail_is_plain_loopback.client.ts): a late 403/401 landing after a key exchange wiped the fresh session (clearAdminSession()was unconditional), reloading a successful login straight back into the gate. A failed response may now only invalidate the credentials it actually carried (send-time session capture + token compare). New test:a stale 403 does not clear a session stored during its flight.CodeRabbit's docstring-coverage warning on the new test functions is also addressed.