diff --git a/CHANGELOG.md b/CHANGELOG.md index 2ba9fa43..30eb6246 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,7 @@ the frozen-backend fallback mirror it for their toolchains. - The OmniVoice guide now covers combining style attributes with a reference clip (consistent instruct stabilizes cloning; the reference wins conflicts), inline pronunciation control (pinyin / CMU phonemes), and corrects the claim that the default engine can't do voice design — it can, from attributes (#1565) ### Fixed +- A remote browser hitting the admin-gated 403 ("loopback origin or admin API key required") now gets the API-key login form instead of endless console 403s, and the desktop build's admin 403 says plain "loopback origin required" so PIN-share guests are never offered a login form no key can satisfy (#1568) - The crash-isolated ASR sidecar and its download preflight now agree on which model to load — setting the shared faster-whisper model variable applies to both variants instead of the sidecar quietly using a different one (#1556) - "Ready" now requires the deep health probe (a working database-backed route), not just the identity probe — a backend whose install broke underneath can no longer be announced up while every real request fails (#1548) - Supervisor restarts after repeat crashes now back off (immediate, then 5s, then 15s) instead of respawning back-to-back, so a tight crash loop can't burn the whole restart budget in seconds (#1548) diff --git a/backend/api/dependencies.py b/backend/api/dependencies.py index d825e870..c16633fe 100644 --- a/backend/api/dependencies.py +++ b/backend/api/dependencies.py @@ -157,6 +157,31 @@ def require_loopback(request: Request) -> None: raise HTTPException(status_code=403, detail="loopback origin required") +def _admin_gate_403() -> None: + """Raise the admin-gate 403 with a detail that states what would ACTUALLY + satisfy the gate. The bundled UI routes any 403 whose detail mentions + "admin api key" to the API-key login form (frontend ``client.ts``; the + literal contract is locked by ``tests/test_auth_gate_detail_lockstep.py``), + so the wording must not name a key where presenting one cannot help. + + The detail names the key only when the gate would accept one: server mode + WITH an API key configured. Every other rejection — desktop mode (the + credential checks in the callers only run under server mode) and a + server-mode deployment with only a share PIN or nothing configured — keeps + the plain loopback detail, because only loopback can use admin there. + Naming the key in those cases would trap a LAN-share guest in a login + form that can never succeed (#1213, #1525; PR #1569 review). + """ + raise HTTPException( + status_code=403, + detail=( + "loopback origin or admin API key required" + if _server_mode() and remote_api_key() + else "loopback origin required" + ), + ) + + def require_admin(request: Request) -> None: """Gate RCE/filesystem-capable admin routers. @@ -180,7 +205,7 @@ def require_admin(request: Request) -> None: return if _request_presents_admin_credential(request): return - raise HTTPException(status_code=403, detail="loopback origin or admin API key required") + _admin_gate_403() def require_admin_action(request: Request) -> None: @@ -198,7 +223,7 @@ def require_admin_action(request: Request) -> None: side_effectful_get=True, ): return - raise HTTPException(status_code=403, detail="loopback origin or admin API key required") + _admin_gate_403() def require_desktop(request: Request) -> None: diff --git a/docs/api-auth.md b/docs/api-auth.md index 89c24c5d..8de72ccd 100644 --- a/docs/api-auth.md +++ b/docs/api-auth.md @@ -342,7 +342,7 @@ arbitrary path merely because it ends in `/ws/events` or `/ws/transcribe`. | Code | Meaning | What to do | |---|---|---| | **401** | Consumption auth failed — `{"detail": "PIN required"}` or `{"detail": "API key required"}`. | Supply the PIN / key (header, cookie, or query param above). A WebSocket surfaces this as close code **1008**. | -| **403** | Authorization failed: loopback/native access was required, cookie Origin/CSRF validation failed, a server-mode mutation lacked an admin credential, or a native path capability was invalid/expired. | A PIN cannot grant admin or filesystem access. Re-authenticate the UI; scripts should use the API-key header; run native operations from the desktop app. | +| **403** | Authorization failed: loopback/native access was required, cookie Origin/CSRF validation failed, a server-mode mutation lacked an admin credential, or a native path capability was invalid/expired. | A PIN cannot grant admin or filesystem access. Re-authenticate the UI; scripts should use the API-key header; run native operations from the desktop app. The admin gates are mode-distinct: server mode answers `{"detail": "loopback origin or admin API key required"}` (the bundled UI routes it to the API-key login form), the desktop build answers `{"detail": "loopback origin required"}` (a presented key cannot satisfy it — only loopback can). | | **429** | A failed administrator-session exchange exceeded its per-client limit, the GPU pool is saturated, or a model download is rate-limited. Ships with `Retry-After`; workload throttles also carry `X-VoiceStudio-Retryable: true`. | Back off for `Retry-After` seconds. For authentication, verify the master before retrying; a correct master is never locked out. | --- diff --git a/frontend/src/api/client.test.ts b/frontend/src/api/client.test.ts index 89c42e36..e5b4afdf 100644 --- a/frontend/src/api/client.test.ts +++ b/frontend/src/api/client.test.ts @@ -182,15 +182,16 @@ describe('apiFetch 401 routing', () => { dispatch.mockRestore(); }); - const stub401 = (detail: string) => + const stubStatus = (status: number, statusText: string, detail: string) => vi.fn(() => Promise.resolve({ ok: false, - status: 401, - statusText: 'Unauthorized', + status, + statusText, text: async () => JSON.stringify({ detail }), }), ) as any; + const stub401 = (detail: string) => stubStatus(401, 'Unauthorized', detail); const authEvent = () => dispatch.mock.calls.map((c) => c[0]).find((e) => (e as Event).type === 'ov:auth-required'); @@ -227,6 +228,62 @@ describe('apiFetch 401 routing', () => { expect(authEvent()).toBeTruthy(); expect((authEvent() as any).detail.mode).toBe('pin'); }); + + const stub403 = (detail: string) => stubStatus(403, 'Forbidden', detail); + + it('dispatches ov:auth-required {mode:"apikey"} on an admin-gate 403 (#1525)', async () => { + globalThis.fetch = stub403('loopback origin or admin API key required'); + const { apiFetch } = await import('./client'); + try { + await apiFetch('/system/info'); + } catch { + /* ApiError expected */ + } + expect(authEvent()).toBeTruthy(); + expect((authEvent() as any).detail.mode).toBe('apikey'); + }); + + it('does not dispatch ov:auth-required on other 403s (CSRF / desktop-only)', async () => { + globalThis.fetch = stub403('browser origin rejected'); + const { apiFetch } = await import('./client'); + try { + await apiFetch('/system/info'); + } catch { + /* ApiError expected */ + } + expect(authEvent()).toBeFalsy(); + }); + + it('a stale 403 does not clear a session stored during its flight (PR #1569 race)', async () => { + // The request goes out with NO credential; while it is in flight the + // user completes the key exchange. A late 403 may only invalidate the + // credentials the failed request actually carried — wiping the fresh + // session would reload a successful login straight back into the gate. + globalThis.fetch = vi.fn(() => { + sessionStorage.setItem( + ADMIN_SESSION_STORAGE_KEY, + JSON.stringify({ + token: `ovs_admin_session_${'N'.repeat(43)}`, + expiresAt: Date.now() / 1000 + 3600, + apiBase: API, + }), + ); + return Promise.resolve({ + ok: false, + status: 403, + statusText: 'Forbidden', + text: async () => JSON.stringify({ detail: 'loopback origin or admin API key required' }), + }); + }) as any; + const { apiFetch } = await import('./client'); + try { + await apiFetch('/system/info'); + } catch { + /* ApiError expected */ + } + expect(authEvent()).toBeTruthy(); + expect(sessionStorage.getItem(ADMIN_SESSION_STORAGE_KEY)).not.toBeNull(); + }); }); describe('apiFetch 404 from a non-VoiceStudio server (#1385)', () => { diff --git a/frontend/src/api/client.ts b/frontend/src/api/client.ts index 37cfd659..f66cac7c 100644 --- a/frontend/src/api/client.ts +++ b/frontend/src/api/client.ts @@ -528,14 +528,32 @@ export async function apiFetch(path: string, opts: ApiFetchOptions = {}): Promis // "API key required" (BearerKeyMiddleware, OMNIVOICE_API_KEY) vs anything // else, i.e. "PIN required" (NetworkAccessMiddleware). Both are 401; the // detail is the only discriminator (only two 401 sites exist backend-side). - if (backendTarget && res.status === 401 && typeof window !== 'undefined') { + // The router-level admin gates answer 403 "loopback origin or admin API + // key required" (require_admin/require_admin_action) — same situation, the + // client just isn't admin-authenticated — so it routes to the API-key form + // too. Other 403s (CSRF "browser origin rejected", loopback-only routes) + // are NOT credential gaps; presenting a key won't help, so they stay plain + // errors. + const adminGate403 = + res.status === 403 && + typeof detail === 'string' && + detail.toLowerCase().includes('admin api key'); + if (backendTarget && (res.status === 401 || adminGate403) && typeof window !== 'undefined') { // readError's declared `string` return isn't guaranteed at runtime — // `j.detail` can be a structured object/array on a future 401. Match only // real strings (avoids both a `.toLowerCase()` crash and `String()` itself // throwing on a malformed object); anything else falls back to PIN. + // (No adminGate403 arm here: "admin api key" ⊇ "api key", so the sniff + // below already yields 'apikey' for every admin-gate 403.) const mode = typeof detail === 'string' && detail.toLowerCase().includes('api key') ? 'apikey' : 'pin'; - if (mode === 'apikey') clearAdminSession(); + // A failed response may only invalidate the credentials it actually + // carried (`session` is captured at send time). Clearing blindly let + // a stale 403 that landed after a key exchange wipe the fresh + // session, reloading a successful login straight back into the gate. + if (mode === 'apikey' && session && getAdminSession(API)?.token === session.token) { + clearAdminSession(); + } window.dispatchEvent(new CustomEvent('ov:auth-required', { detail: { mode } })); } // Structured details (e.g. the typed asr_model_missing 409) carry a diff --git a/tests/backend/api/test_engines_route_shape.py b/tests/backend/api/test_engines_route_shape.py index a30d28d2..120ef70c 100644 --- a/tests/backend/api/test_engines_route_shape.py +++ b/tests/backend/api/test_engines_route_shape.py @@ -627,7 +627,7 @@ def test_engine_health_is_admin_gated(fresh_app): client = _client(fresh_app, host="10.0.0.5") r = client.get("/engines/omnivoice/health") assert r.status_code == 403 - assert r.json()["detail"] == "loopback origin or admin API key required" + assert r.json()["detail"] == "loopback origin required" def test_server_mode_engine_mutations_require_api_key(fresh_app, monkeypatch): @@ -802,7 +802,7 @@ def test_selftest_unknown_id_is_404(fresh_app): def test_selftest_is_admin_gated(fresh_app): r = _client(fresh_app, host="10.0.0.9").post("/engines/omnivoice/selftest") assert r.status_code == 403 - assert r.json()["detail"] == "loopback origin or admin API key required" + assert r.json()["detail"] == "loopback origin required" def test_selftest_captures_synth_exception_without_500(fresh_app): diff --git a/tests/test_auth_gate_detail_lockstep.py b/tests/test_auth_gate_detail_lockstep.py new file mode 100644 index 00000000..68f58c4d --- /dev/null +++ b/tests/test_auth_gate_detail_lockstep.py @@ -0,0 +1,76 @@ +"""Cross-layer contract lock for the admin-gate 403 detail string. + +The backend's ``require_admin``/``require_admin_action`` answer 403 with a +mode-distinct ``detail`` (``_admin_gate_403`` in backend/api/dependencies.py): +"loopback origin or admin API key required" in server mode, plain +"loopback origin required" on the desktop build. The SPA's ``apiFetch`` routes +a 403 to the API-key login gate exactly when the detail contains the substring +"admin api key" (frontend/src/api/client.ts) — i.e. when presenting the key +could actually satisfy the gate. The per-mode behaviour is pinned by +tests/test_loopback_server_mode.py; this file pins the LITERAL contract across +layers: a backend reword keeps backend tests green while the frontend matcher +silently stops firing, and a LAN user is back to raw 403 spam instead of the +login form. +""" + +from __future__ import annotations + +import re +from pathlib import Path + +ROOT = Path(__file__).resolve().parents[1] +DEPS = ROOT / "backend" / "api" / "dependencies.py" +CLIENT = ROOT / "frontend" / "src" / "api" / "client.ts" + + +def _frontend_sniff() -> str: + """The substring apiFetch matches on a 403 to admit it to the auth gate.""" + text = CLIENT.read_text(encoding="utf-8") + # adminGate403 = ... detail.toLowerCase().includes('') + m = re.search(r"adminGate403 =.*?includes\('([^']+)'\)", text, re.DOTALL) + assert m, "adminGate403 matcher not found in frontend/src/api/client.ts" + return m.group(1) + + +def _key_named_details() -> set[str]: + """Every quoted string in dependencies.py that names the admin API key.""" + return set(re.findall(r'"([^"]*admin API key[^"]*)"', DEPS.read_text(encoding="utf-8"))) + + +def test_key_named_details_match_frontend_sniff(): + """Every backend literal naming the admin key must contain the SPA matcher.""" + details = _key_named_details() + assert details, ( + "no 'admin API key' detail literal left in dependencies.py — moved or " + "reworded? Update frontend/src/api/client.ts in the same change." + ) + sniff = _frontend_sniff() + for detail in details: + # Case-insensitive substring, mirroring apiFetch's toLowerCase match. + assert sniff in detail.lower(), ( + f"backend detail {detail!r} no longer contains the frontend matcher " + f"{sniff!r} — the SPA would stop routing it to the API-key gate. " + "Update frontend/src/api/client.ts in the same change." + ) + + +def test_frontend_sniff_rejects_details_a_key_cannot_fix(): + """The sniff must not swallow 403s an API key cannot satisfy. + + The desktop admin-gate arm (loopback-only regardless of credentials), the + legacy require_loopback desktop 403, the CSRF rejection, and the + desktop-only filesystem gate: routing any of these to the login form would + trap the user in a form that can never succeed. + """ + sniff = _frontend_sniff() + unfixable = ( + "loopback origin required", # desktop admin arm + require_loopback + "browser origin rejected", # BearerKeyMiddleware CSRF (main.py) + "desktop origin required", # require_desktop — loopback-only forever + "native filesystem access requires loopback origin", # require_native + ) + for detail in unfixable: + assert sniff not in detail.lower(), ( + f"frontend matcher {sniff!r} now also matches {detail!r}, which " + "an API key cannot satisfy — the login gate would loop." + ) diff --git a/tests/test_loopback_server_mode.py b/tests/test_loopback_server_mode.py index 138291bf..8888c1ba 100644 --- a/tests/test_loopback_server_mode.py +++ b/tests/test_loopback_server_mode.py @@ -253,6 +253,89 @@ def test_side_effectful_get_rejects_remote_api_key_outside_server_mode(monkeypat assert exc.value.status_code == 403 +# Mode-distinct admin-gate detail: the 403 message must state what would +# ACTUALLY satisfy the gate. The bundled UI routes any 403 whose detail +# mentions "admin api key" to the API-key login form (frontend client.ts; +# the literal contract is locked by tests/test_auth_gate_detail_lockstep.py). +# Server mode accepts the key, so naming it is right. Desktop mode rejects +# every non-loopback client regardless of credentials — the checks above only +# run under server mode — so it must keep the plain loopback detail: naming +# the key there invites a login form that can never succeed (a desktop +# LAN-share guest would lose the whole consumption UI to it, #1213). + + +def test_require_admin_desktop_detail_is_plain_loopback(monkeypatch): + """Desktop build: no presented key can satisfy the gate.""" + monkeypatch.delenv("OMNIVOICE_SERVER_MODE", raising=False) + monkeypatch.setenv("OMNIVOICE_API_KEY", "s3cret") # a valid key can't help here + + with pytest.raises(HTTPException) as exc: + require_admin( + _req_full("10.0.0.5", headers={"authorization": "Bearer s3cret"}) + ) + + assert exc.value.status_code == 403 + assert exc.value.detail == "loopback origin required" + + +def test_require_admin_server_mode_detail_names_the_key(monkeypatch): + """Server mode with an API key configured: the 403 names the key.""" + monkeypatch.setenv("OMNIVOICE_SERVER_MODE", "1") + monkeypatch.setenv("OMNIVOICE_API_KEY", "s3cret") + + with pytest.raises(HTTPException) as exc: + require_admin(_req_full("172.17.0.1")) # credential configured, none presented + + assert exc.value.status_code == 403 + assert exc.value.detail == "loopback origin or admin API key required" + + +def test_require_admin_pin_only_server_mode_detail_is_plain_loopback(monkeypatch): + """Server mode with ONLY a share PIN (Greptile P1, PR #1569): the PIN + closes read-only bootstrap but no API key exists to present, so naming + the key would send the browser to a login form that can never succeed. + Only loopback can use admin here — the plain detail says so, and the + SPA leaves it a plain error instead of gating the whole UI.""" + monkeypatch.setenv("OMNIVOICE_SERVER_MODE", "1") + monkeypatch.delenv("OMNIVOICE_API_KEY", raising=False) + + with pytest.raises(HTTPException) as exc: + require_admin(_req_full("172.17.0.1", pin="424242")) # PIN ≠ admin credential + + assert exc.value.status_code == 403 + assert exc.value.detail == "loopback origin required" + + +def test_require_admin_action_desktop_detail_is_plain_loopback(monkeypatch): + """Desktop build, side-effectful GET: plain loopback detail.""" + monkeypatch.delenv("OMNIVOICE_SERVER_MODE", raising=False) + monkeypatch.setenv("OMNIVOICE_API_KEY", "s3cret") + + with pytest.raises(HTTPException) as exc: + require_admin_action( + _req_full( + "10.0.0.5", + method="GET", + headers={"authorization": "Bearer s3cret"}, + ) + ) + + assert exc.value.status_code == 403 + assert exc.value.detail == "loopback origin required" + + +def test_require_admin_action_server_mode_detail_names_the_key(monkeypatch): + """Server mode + key configured, side-effectful GET: names the key.""" + monkeypatch.setenv("OMNIVOICE_SERVER_MODE", "1") + monkeypatch.setenv("OMNIVOICE_API_KEY", "s3cret") + + with pytest.raises(HTTPException) as exc: + require_admin_action(_req_full("172.17.0.1", method="GET")) + + assert exc.value.status_code == 403 + assert exc.value.detail == "loopback origin or admin API key required" + + def test_side_effectful_get_rejects_pin_and_trusted_network(monkeypatch): monkeypatch.setenv("OMNIVOICE_SERVER_MODE", "1") monkeypatch.setenv("OMNIVOICE_TRUSTED_NETWORKS", "10.0.0.0/8")