diff --git a/CHANGELOG.md b/CHANGELOG.md index 3ab58bc8..9e8b4353 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,8 @@ the frozen-backend fallback mirror it for their toolchains. **Highlights** +- Docker/server mode now requires an API key for remote changes and side-effectful admin checks across workers, engines, media tools, MCP, pronunciation, diagnostics, and LLM providers. (#1525) +- The unified Support page no longer throws while opening a section in browsers or test environments without `scrollIntoView`. (#1525) - A faster, cleaner Dub workspace for multilingual production (#1489) - VoiceStudio now gives the app, desktop chrome, documentation, and package metadata one clear identity - A local-first creative studio: voice cloning, design, dubbing, dictation, stories, audiobooks, and transcription without a subscription meter diff --git a/backend/api/dependencies.py b/backend/api/dependencies.py index d3217d6e..25e6edaf 100644 --- a/backend/api/dependencies.py +++ b/backend/api/dependencies.py @@ -6,7 +6,10 @@ composed at the route or router level without surprises. Currently exposed: - `require_loopback`: 403 unless the request came from a loopback origin - (bypassed in explicit server mode — see `_server_mode`). + (read-only bootstrap is allowed in explicit server mode; mutations still + require the admin API key — see `_server_mode`). +- `require_admin`: method-aware admin gate for privileged routers. +- `require_admin_action`: strict admin gate for side-effectful GET actions. - `require_native_access`: true-loopback-only access to the host filesystem; unlike `require_loopback`, it is never bypassed by server mode. - `ws_remote_authorized`: whether a WebSocket handshake from a non-loopback @@ -50,7 +53,7 @@ def _trusted_networks(): def is_loopback(host): """True loopback address only (127.0.0.1, ::1, localhost) — NOT a trusted - network. Admin gates (``require_loopback`` → ``/system/set-env``, + network. Admin gates (``require_admin`` → ``/system/set-env``, ``/api/settings/*``) use this so a trusted-network CIDR exempts consumption (TTS / dictation) but never the RCE-class admin surface.""" return host in _LOOPBACK_HOSTS @@ -74,6 +77,7 @@ def is_local_host(host): return any(ip in net for net in _trusted_networks()) _TRUTHY = frozenset({"1", "true", "yes", "on"}) +_READ_ONLY_METHODS = frozenset({"GET", "HEAD", "OPTIONS"}) def _server_mode() -> bool: @@ -96,6 +100,34 @@ def _server_mode() -> bool: return os.environ.get("OMNIVOICE_SERVER_MODE", "").strip().lower() in _TRUTHY +def remote_api_key() -> str | None: + """The normalized remote-backend bearer key, or None when remote mode is + off. Surrounding whitespace is configuration noise, never a valid secret. + Read at call time so tests can monkeypatch the environment.""" + return os.environ.get("OMNIVOICE_API_KEY", "").strip() or None + + +def presented_api_key(connection) -> str: + """Return the first non-empty normalized API key on an HTTP/WS connection. + + Authorization wins over query, which wins over cookie. Each channel is + stripped before fallback so whitespace in a higher-priority channel cannot + shadow a valid lower-priority credential. + """ + headers = getattr(connection, "headers", None) or {} + query = getattr(connection, "query_params", None) or {} + cookies = getattr(connection, "cookies", None) or {} + + auth = headers.get("authorization", "") + supplied = auth[7:].strip() if auth.lower().startswith("bearer ") else "" + if supplied: + return supplied + supplied = (query.get("api_key") or "").strip() + if supplied: + return supplied + return (cookies.get("ov_key") or "").strip() + + def _configured_pin(request) -> str | None: """The active share PIN (``app.state.network_share.pin``) or None. Read via getattr so a bare Request stub (or a request that hit before lifespan set @@ -107,10 +139,13 @@ def _configured_pin(request) -> str | None: def _admin_credential_configured(request) -> bool: - """Whether the operator has set ANY credential gate — the remote API key or - a share PIN. When neither is set, server mode leaves admin open (the Docker - issue #261 flow the image depends on).""" - if os.environ.get("OMNIVOICE_API_KEY"): + """Whether an API key or share PIN is configured. + + The PIN cannot authorize admin access, but its presence means the operator + opted out of bare-server discovery. Remote admin then remains closed until + they configure and present the long API key. + """ + if remote_api_key(): return True return bool(_configured_pin(request)) @@ -129,17 +164,10 @@ def _request_presents_admin_credential(request) -> bool: admin. Net: remote admin in server mode requires the API key; a PIN-only deployment keeps admin loopback-only. getattr-defensive so a minimal Request stub never raises.""" - api_key = os.environ.get("OMNIVOICE_API_KEY") or "" + api_key = remote_api_key() or "" if not api_key: return False - headers = getattr(request, "headers", None) or {} - query = getattr(request, "query_params", None) or {} - cookies = getattr(request, "cookies", None) or {} - - auth = headers.get("authorization", "") - supplied = auth[7:].strip() if auth.lower().startswith("bearer ") else "" - if not supplied: - supplied = query.get("api_key") or cookies.get("ov_key") or "" + supplied = presented_api_key(request) return bool(supplied and secrets.compare_digest(supplied, api_key)) @@ -163,9 +191,9 @@ def require_loopback(request: Request) -> None: unenforceable, so the gate can't require true loopback. It then applies the admin-credential rule instead: - - No credential configured (no API key, no PIN) → open, matching the #261 - Docker flow where the operator reaches ``/system/*`` off the bridge - gateway with nothing set. + - No credential configured (no API key, no PIN) → read-only requests are + open, matching the #261 Docker bootstrap flow. State-changing requests + fail closed even if a route accidentally kept this legacy dependency. - A credential IS configured → the request must present the **API key**. This keeps the two-tier privilege model intact under server mode: ``OMNIVOICE_TRUSTED_NETWORKS`` is a *consumption* exemption @@ -173,14 +201,20 @@ def require_loopback(request: Request) -> None: NEVER by itself unlock the admin surface (``/system/set-env`` — RCE-class — and ``/api/settings/*``). The 6-digit share PIN is a consumption credential too and does not gate admin, so a PIN-only deployment keeps admin - loopback-only; remote admin requires the (long) API key. A LAN client in a - trusted CIDR — or one holding only the PIN — gets 403 here even though it - sails through the consumption gates. See docs/api-auth.md (#1213). + loopback-only; remote admin requires the long API key. See + docs/api-auth.md (#1213). """ host = request.client.host if request.client else None if is_loopback(host): return if _server_mode(): + method = str(getattr(request, "method", "GET")).upper() + if method not in _READ_ONLY_METHODS: + # Defense in depth. Privileged routers should declare + # ``require_admin`` directly, but a missed migration must not turn + # into an unauthenticated Docker write primitive. + require_admin(request) + return if not _admin_credential_configured(request): return if _request_presents_admin_credential(request): @@ -206,14 +240,29 @@ def require_admin(request: Request) -> None: return if _server_mode(): method = str(getattr(request, "method", "GET")).upper() - read_only = method in {"GET", "HEAD", "OPTIONS"} - if read_only and not os.environ.get("OMNIVOICE_API_KEY", "").strip(): + read_only = method in _READ_ONLY_METHODS + if read_only and not _admin_credential_configured(request): return if _request_presents_admin_credential(request): return raise HTTPException(status_code=403, detail="loopback origin or admin API key required") +def require_admin_action(request: Request) -> None: + """Gate an administrative action even when its HTTP method is read-only. + + A small number of legacy GET endpoints have real side effects. For example, + an engine health check may spawn a sidecar process. Such routes cannot use + :func:`require_admin`'s bare-server discovery exception. + """ + host = request.client.host if request.client else None + if is_loopback(host): + return + if _server_mode() and _request_presents_admin_credential(request): + return + raise HTTPException(status_code=403, detail="loopback origin or admin API key required") + + def require_desktop(request: Request) -> None: """Gate capabilities that may select or execute host filesystem paths. @@ -232,9 +281,10 @@ def require_local(request: Request) -> None: trusted network. The consumption-tier companion to :func:`require_loopback`: use on routes a trusted-network client (LAN/proxy) should reach without a PIN or API key — e.g. the dictation model/prefs endpoints that pair with the - dictation WebSocket. Admin routes stay on :func:`require_loopback`. + dictation WebSocket. Admin routes stay on :func:`require_admin`. - In server mode the gate is a no-op (same as :func:`require_loopback`).""" + In server mode this consumption gate is a no-op. Admin dependencies remain + method-aware and independent from this exemption.""" host = request.client.host if request.client else None if is_local_host(host): return @@ -256,12 +306,6 @@ def require_native_access(request: Request) -> None: raise HTTPException(status_code=403, detail="native filesystem access requires loopback origin") -def remote_api_key() -> str | None: - """The remote-backend bearer key (Wave 2.3), or None when remote mode is - off. Read at call time so tests can monkeypatch the env.""" - return os.environ.get("OMNIVOICE_API_KEY") or None - - def ws_remote_authorized(websocket) -> bool: """Whether a WebSocket handshake presents the remote API key. @@ -273,12 +317,4 @@ def ws_remote_authorized(websocket) -> bool: key = remote_api_key() if not key: return False - auth = websocket.headers.get("authorization", "") - supplied = auth[7:].strip() if auth.lower().startswith("bearer ") else "" - if not supplied: - supplied = ( - websocket.query_params.get("api_key") - or websocket.cookies.get("ov_key") - or "" - ) - return secrets.compare_digest(supplied, key) + return secrets.compare_digest(presented_api_key(websocket), key) diff --git a/backend/api/routers/capture_ws.py b/backend/api/routers/capture_ws.py index 2bc1be24..c021087e 100644 --- a/backend/api/routers/capture_ws.py +++ b/backend/api/routers/capture_ws.py @@ -153,8 +153,8 @@ def _select_sherpa_spec(websocket: WebSocket): async def ws_transcribe(websocket: WebSocket): """Stream audio in, get partial + final transcription out.""" # Loopback origin guard — refuse anything not from 127.0.0.1, ::1, or - # localhost. HTTP routers use Depends(require_loopback) at router level; - # WebSocket dependency injection differs across FastAPI versions, so we + # localhost. Privileged HTTP routers use Depends(require_admin) at router + # level; WebSocket dependency injection differs across FastAPI versions, so we # inline the check before accept(). Without it, any local process could # stream the user's microphone over this endpoint. # Wave 2.3 (remote backend): a non-loopback client that presents the diff --git a/backend/api/routers/engines.py b/backend/api/routers/engines.py index 1570c6d6..0b0cdd6f 100644 --- a/backend/api/routers/engines.py +++ b/backend/api/routers/engines.py @@ -25,7 +25,7 @@ from huggingface_hub import utils as hf_utils from huggingface_hub.errors import HFValidationError from pydantic import BaseModel -from api.dependencies import require_loopback +from api.dependencies import require_admin, require_admin_action, require_desktop from core import prefs from services import tts_backend, asr_backend, llm_backend, translation_engines from services.audio_dsp import list_effect_presets @@ -113,7 +113,10 @@ def list_translation_engines(): } -@router.post("/engines/translation/{engine_id}/install") +@router.post( + "/engines/translation/{engine_id}/install", + dependencies=[Depends(require_admin)], +) async def install_translation_engine(engine_id: str): entry = translation_engines.get_engine(engine_id) if not entry: @@ -149,7 +152,10 @@ async def install_translation_engine(engine_id: str): } -@router.delete("/engines/translation/{engine_id}") +@router.delete( + "/engines/translation/{engine_id}", + dependencies=[Depends(require_admin)], +) async def uninstall_translation_engine(engine_id: str): entry = translation_engines.get_engine(engine_id) if not entry: @@ -188,15 +194,16 @@ async def uninstall_translation_engine(engine_id: str): # POST /engines/sonitranslate/install). Mirrors the # /engines/translation/{engine_id}/install namespace pattern. # -# Loopback-gated: installing spawns subprocesses (git/uv) and writes to the -# data directory — only the local desktop frontend may trigger it. The job -# runs fine in packaged builds: the venv lives under the user data dir, not -# inside the signed app bundle, and uv resolves via OMNIVOICE_BUNDLED_UV/PATH. +# Desktop-only: installing spawns git/uv against mutable source and writes an +# editable environment. An API key does not make that supply-chain path safe to +# trigger remotely. The job runs fine in packaged builds: the venv lives under +# the user data dir, not inside the signed app bundle, and uv resolves via +# OMNIVOICE_BUNDLED_UV/PATH. @router.post( "/engines/sidecar/{engine_id}/install", - dependencies=[Depends(require_loopback)], + dependencies=[Depends(require_admin), Depends(require_desktop)], ) def install_sidecar_engine(engine_id: str): """Start (or report) the one-click install for a sidecar engine. @@ -222,7 +229,7 @@ def install_sidecar_engine(engine_id: str): @router.get( "/engines/sidecar/{engine_id}/install/status", - dependencies=[Depends(require_loopback)], + dependencies=[Depends(require_admin)], ) def sidecar_install_status(engine_id: str): """Step-by-step status of the sidecar install job (poll while running). @@ -243,7 +250,7 @@ def sidecar_install_status(engine_id: str): @router.delete( "/engines/sidecar/{engine_id}/install", - dependencies=[Depends(require_loopback)], + dependencies=[Depends(require_admin)], ) def uninstall_sidecar_engine(engine_id: str): """Remove an app-managed sidecar install (checkout + venv + weights) and @@ -274,8 +281,8 @@ def uninstall_sidecar_engine(engine_id: str): # frame. Result includes wall-clock latency so the UI can render # "1234 ms — pong" inline next to the button. # -# Loopback-gated (T-02-13): only the local desktop frontend may trigger -# a sidecar spawn through this endpoint. +# Admin-gated (T-02-13): only the local desktop frontend or an authenticated +# server-mode administrator may trigger a sidecar spawn through this endpoint. # Engine instances cached for the lifetime of the FastAPI process so that # repeated health checks don't spawn a new SubprocessBackend (each spawn @@ -311,7 +318,7 @@ def _resolve_engine_class(engine_id: str): @router.get( "/engines/{engine_id}/health", - dependencies=[Depends(require_loopback)], + dependencies=[Depends(require_admin_action)], ) def engine_health(engine_id: str): """Spawn-and-ping a SubprocessBackend; ``is_available()`` for the rest. @@ -385,7 +392,7 @@ def engine_health(engine_id: str): # hanging the Settings panel. The orphaned worker is best-effort daemon. # * A process-wide lock serialises self-tests so a click-storm can't stack # concurrent model loads. -# * Only ever on user click (POST) — never on Settings load. Loopback-gated. +# * Only ever on user click (POST) — never on Settings load. Admin-gated. # Deliberately short + ASCII so the synth stays CPU-cheap and the phrase never # trips the no-hardcoded-CJK guard. @@ -452,7 +459,7 @@ class SelfTestResponse(BaseModel): @router.post( "/engines/{engine_id}/selftest", response_model=SelfTestResponse, - dependencies=[Depends(require_loopback)], + dependencies=[Depends(require_admin)], ) def engine_selftest(engine_id: str): """Run a bounded, real synthesis on an available in-process TTS engine. @@ -551,7 +558,11 @@ class SelectEngineResponse(BaseModel): routing_reason: str | None = None -@router.post("/engines/select", response_model=SelectEngineResponse) +@router.post( + "/engines/select", + response_model=SelectEngineResponse, + dependencies=[Depends(require_admin)], +) def select_engine(req: SelectEngineRequest): """Persist a family's engine pick to prefs.json. Refuses unknown backends, backends whose deps aren't installed, AND backends that cannot run on THIS diff --git a/backend/api/routers/mcp_bindings.py b/backend/api/routers/mcp_bindings.py index c1cf7aae..602cd3b5 100644 --- a/backend/api/routers/mcp_bindings.py +++ b/backend/api/routers/mcp_bindings.py @@ -9,13 +9,13 @@ from __future__ import annotations from fastapi import APIRouter, Depends, HTTPException from pydantic import BaseModel, Field -from api.dependencies import require_loopback +from api.dependencies import require_admin from services import mcp_bindings router = APIRouter( prefix="/api/mcp", tags=["mcp"], - dependencies=[Depends(require_loopback)], + dependencies=[Depends(require_admin)], ) diff --git a/backend/api/routers/media_tools.py b/backend/api/routers/media_tools.py index 770cd865..50e5a181 100644 --- a/backend/api/routers/media_tools.py +++ b/backend/api/routers/media_tools.py @@ -12,10 +12,10 @@ import logging from fastapi import APIRouter, Depends, HTTPException from pydantic import BaseModel -from api.dependencies import require_loopback +from api.dependencies import require_admin logger = logging.getLogger("omnivoice.api") -router = APIRouter(dependencies=[Depends(require_loopback)]) +router = APIRouter(dependencies=[Depends(require_admin)]) class CustomPathRequest(BaseModel): diff --git a/backend/api/routers/pronunciation.py b/backend/api/routers/pronunciation.py index 7db9759f..193f9c44 100644 --- a/backend/api/routers/pronunciation.py +++ b/backend/api/routers/pronunciation.py @@ -7,7 +7,7 @@ CRUD for the DB-backed, per-language pronunciation dictionary the before synthesis (see ``services/pronunciation.apply_pronunciation`` and the generate path), so a saved entry actually changes the audio on every engine. -Endpoints (loopback-only, like the dictation router): +Endpoints (admin-gated; loopback or authenticated server mode): GET /pronunciation → list every entry POST /pronunciation → create one entry PUT /pronunciation/{entry_id} → update an entry (partial) @@ -30,12 +30,12 @@ from typing import List, Optional from fastapi import APIRouter, Depends, HTTPException from pydantic import BaseModel -from api.dependencies import require_loopback +from api.dependencies import require_admin from core.db import db_conn from services.pronunciation import apply_pronunciation, entries_for_language logger = logging.getLogger("omnivoice.pronunciation") -router = APIRouter() +router = APIRouter(dependencies=[Depends(require_admin)]) _VALID_TYPES = ("respelling", "ipa", "cmu") _ALL_LANG = "*" @@ -133,7 +133,7 @@ class PronImportRequest(BaseModel): # ── CRUD ───────────────────────────────────────────────────────────────────── -@router.get("/pronunciation", dependencies=[Depends(require_loopback)]) +@router.get("/pronunciation") def list_entries(): with db_conn() as conn: rows = conn.execute( @@ -143,7 +143,7 @@ def list_entries(): return [_row_to_dict(r) for r in rows] -@router.post("/pronunciation", dependencies=[Depends(require_loopback)]) +@router.post("/pronunciation") def create_entry(entry: PronEntry): term = entry.term.strip() if not term: @@ -171,7 +171,7 @@ def create_entry(entry: PronEntry): return _row_to_dict(row) -@router.put("/pronunciation/{entry_id}", dependencies=[Depends(require_loopback)]) +@router.put("/pronunciation/{entry_id}") def update_entry(entry_id: str, patch: PronEntryUpdate): with db_conn() as conn: existing = conn.execute( @@ -226,7 +226,7 @@ def update_entry(entry_id: str, patch: PronEntryUpdate): return _row_to_dict(row) -@router.delete("/pronunciation/{entry_id}", dependencies=[Depends(require_loopback)]) +@router.delete("/pronunciation/{entry_id}") def delete_entry(entry_id: str): with db_conn() as conn: cur = conn.execute("DELETE FROM pronunciation_entries WHERE id = ?", (entry_id,)) @@ -236,7 +236,7 @@ def delete_entry(entry_id: str): # ── Dry-run + import/export ─────────────────────────────────────────────────── -@router.post("/pronunciation/test", dependencies=[Depends(require_loopback)]) +@router.post("/pronunciation/test") def test_substitution(req: PronTestRequest): """Show the post-substitution text for ``req.text`` — no model call. @@ -258,7 +258,7 @@ def test_substitution(req: PronTestRequest): } -@router.get("/pronunciation/export", dependencies=[Depends(require_loopback)]) +@router.get("/pronunciation/export") def export_entries(): """Every entry as a JSON-serializable list (round-trips ``/import``).""" with db_conn() as conn: @@ -273,7 +273,7 @@ def export_entries(): ]} -@router.post("/pronunciation/import", dependencies=[Depends(require_loopback)]) +@router.post("/pronunciation/import") def import_entries(req: PronImportRequest): """Bulk-add entries. ``replace=true`` clears the table first. diff --git a/backend/api/routers/settings.py b/backend/api/routers/settings.py index e2591801..70d33bf5 100644 --- a/backend/api/routers/settings.py +++ b/backend/api/routers/settings.py @@ -20,7 +20,7 @@ from fastapi import APIRouter, Depends, HTTPException, Query from pydantic import BaseModel, Field from core.logging_utils import log_safe -from api.dependencies import require_admin +from api.dependencies import require_admin, require_admin_action logger = logging.getLogger("omnivoice.api.settings") @@ -92,8 +92,8 @@ def get_hf_token_state(fresh: bool = Query(False)): # ── Performance settings (INST-12) ──────────────────────────────────────── -# Threat T-02-04: same loopback guard as the hf-token endpoints via the -# router-level `require_loopback` dep. +# Threat T-02-04: same admin guard as the hf-token endpoints via the +# router-level `require_admin` dep. _TORCH_COMPILE_KEY = "perf.torch_compile_disabled" @@ -481,7 +481,10 @@ def _local_models(base_url: str, api_key: str): return None -@router.get("/llm-providers/{provider_id}/models") +@router.get( + "/llm-providers/{provider_id}/models", + dependencies=[Depends(require_admin_action)], +) def list_llm_provider_models(provider_id: str): """List model ids the provider's key can access (OpenAI-compat /models). diff --git a/backend/api/routers/system.py b/backend/api/routers/system.py index 48e9547a..40bea2e8 100644 --- a/backend/api/routers/system.py +++ b/backend/api/routers/system.py @@ -11,7 +11,7 @@ from core.prefs import set_ as prefs_set, delete as prefs_delete from services import network_share from services import tailscale as _tailscale from api.schemas import SysinfoResponse, SystemInfoResponse, ModelStatusResponse -from api.dependencies import is_loopback, require_admin +from api.dependencies import is_loopback, require_admin, require_admin_action from fastapi.responses import FileResponse, StreamingResponse import torch import shutil @@ -1089,7 +1089,10 @@ async def diagnostic_bundle(network: bool = Query(False, description="Include th # ── Self-check diagnostics ──────────────────────────────────────────────── -@router.get("/system/diagnose") +@router.get( + "/system/diagnose", + dependencies=[Depends(require_admin_action)], +) async def system_diagnose( network: bool = Query(True, description="Include the HuggingFace hub reachability probe"), deep: bool = Query(False, description="Also load the active engine and synthesize a short utterance (may cold-load the model — minutes on first run)"), diff --git a/backend/api/routers/workers.py b/backend/api/routers/workers.py index 554e6ebb..3136520c 100644 --- a/backend/api/routers/workers.py +++ b/backend/api/routers/workers.py @@ -28,7 +28,7 @@ import logging from fastapi import APIRouter, Depends, HTTPException, Request from pydantic import BaseModel, Field -from api.dependencies import require_loopback +from api.dependencies import require_admin from worker import registry, routing, service logger = logging.getLogger("omnivoice.worker") @@ -39,9 +39,9 @@ logger = logging.getLogger("omnivoice.worker") # the task's own deadline does. _DISCONNECT_POLL_SECONDS = 1.0 -# Management is loopback-only: these endpoints mint join tokens and revoke -# machines, so they follow the same rule as the app's other privileged routes. -router = APIRouter(prefix="/workers", tags=["workers"], dependencies=[Depends(require_loopback)]) +# Management is admin-gated: these endpoints mint join tokens and revoke +# machines, so Docker writes require the API key while desktop stays loopback. +router = APIRouter(prefix="/workers", tags=["workers"], dependencies=[Depends(require_admin)]) class EnableRequest(BaseModel): diff --git a/backend/main.py b/backend/main.py index 64a14c4f..d88a45ed 100644 --- a/backend/main.py +++ b/backend/main.py @@ -375,7 +375,11 @@ from services.model_manager import ( ) from services import network_share -from api.dependencies import is_local_host # loopback + OMNIVOICE_TRUSTED_NETWORKS +from api.dependencies import ( # loopback + OMNIVOICE_TRUSTED_NETWORKS + is_local_host, + presented_api_key, + remote_api_key, +) from api.routers import ( system, @@ -1197,7 +1201,7 @@ class BearerKeyMiddleware: async def __call__(self, scope, receive, send): if scope["type"] not in ("http", "websocket"): return await self.app(scope, receive, send) - key = os.environ.get("OMNIVOICE_API_KEY") or "" + key = remote_api_key() or "" if not key: return await self.app(scope, receive, send) client = scope["client"][0] if scope.get("client") else None @@ -1212,10 +1216,7 @@ class BearerKeyMiddleware: from starlette.requests import HTTPConnection conn = HTTPConnection(scope) - auth = conn.headers.get("authorization", "") - supplied = auth[7:].strip() if auth.lower().startswith("bearer ") else "" - if not supplied: - supplied = conn.query_params.get("api_key") or conn.cookies.get("ov_key") or "" + supplied = presented_api_key(conn) if not secrets.compare_digest(supplied, key): if scope["type"] == "websocket": diff --git a/docs/api-auth.md b/docs/api-auth.md index cc68ae84..5b1bcb26 100644 --- a/docs/api-auth.md +++ b/docs/api-auth.md @@ -196,10 +196,11 @@ time, so in production **restart the backend** to apply a change. Default empty ## Admin routes and server mode Admin routes — `/system/*` (including `set-env`, **RCE-class**), -`/api/settings/*`, engine install/uninstall, media tools, MCP bindings — sit on -a stricter gate (`require_admin`, `backend/api/dependencies.py`) than -consumption. On the desktop build they are **true-loopback-only**: no PIN, key, -or trusted network reaches them from another machine. +`/api/settings/*`, engine selection/install/uninstall, media tools, MCP +bindings, pronunciation settings, and remote-worker management — sit on a +stricter gate (`require_admin`, `backend/api/dependencies.py`) than consumption. +On the desktop build they are **true-loopback-only**: no PIN, key, or trusted +network reaches them from another machine. In **server mode** (`OMNIVOICE_SERVER_MODE=1`, the Docker image) the loopback origin is unenforceable — NAT rewrites the source and even a @@ -207,16 +208,22 @@ origin is unenforceable — NAT rewrites the source and even a requirement is dropped (issue #261, else the operator is 403'd out of their own `/system/*`). It is replaced by a **credential rule**, not removed: -- **No API key configured** → read-only admin discovery remains available for - the bare Docker bootstrap flow, but `POST`/`PUT`/`PATCH`/`DELETE` requests are - denied. Set `OMNIVOICE_API_KEY` before changing settings remotely. -- **A credential is configured** → admin requires the **API key** (`Authorization: +- **No credential configured** (neither API key nor share PIN) → read-only + admin discovery remains available for the bare Docker bootstrap flow, but + `POST`/`PUT`/`PATCH`/`DELETE` requests are denied. Side-effectful GET actions + are denied too: engine health may start a sidecar, deep diagnostics may load + a model, and LLM provider discovery makes a request with the saved provider + credential. Set `OMNIVOICE_API_KEY` before changing settings or triggering + those actions remotely. +- **An API key is configured** → admin requires that **API key** (`Authorization: Bearer` / `?api_key` / `ov_key` cookie), or genuine loopback. The **6-digit share PIN does not gate admin** (it is brute-forceable), and trusted-network membership never does either. A **PIN-only** server-mode deployment therefore - allows remote read-only discovery but blocks remote mutations; remote writes - require the long API key. Discovery never returns the share PIN itself; only - loopback or a caller already authenticated with the API key can read it. + keeps admin routes loopback-only; remote admin requires the long API key. + +Managed sidecar installation remains true-loopback-only even with an API key. +Its installer fetches mutable source and creates an editable environment, so it +must be run directly on that machine until the source supply chain is pinned. Host paths are never selected through HTTP. The native Tauri process validates model-cache and export destinations plus custom FFmpeg/FFprobe binaries, writes diff --git a/docs/remote-workers.md b/docs/remote-workers.md index 5a2b1e4c..d4aeefaa 100644 --- a/docs/remote-workers.md +++ b/docs/remote-workers.md @@ -7,6 +7,12 @@ This is **opt-in and off by default**. Until you turn it on and approve a worker, nothing leaves your computer, no port is opened, and the app behaves exactly as it did before. +Worker management is an admin surface. In Docker/server mode, viewing status +works during bare bootstrap, but joining, enabling, approving, issuing keys, +disconnecting, or removing machines remotely requires `OMNIVOICE_API_KEY`. +The share PIN and trusted-network exemptions authorize playback, not worker +administration. + > **Not the same as [Remote backend](remote-gpu.md).** That points this app at > a backend running somewhere else, so the whole app — your projects, your > voices, your history — lives on that machine. This keeps everything here and @@ -157,10 +163,11 @@ The Dictation surface states that it always uses this machine without showing the generic "not ported yet" notice. For protocol development, a task can also be placed by hand with -`POST /workers/tasks` — a **development-only** endpoint. It is loopback-only, +`POST /workers/tasks` — a **development-only** endpoint. It is admin-gated, sits behind the same opt-in as everything else here, takes a mandatory -deadline, submits one task and waits for it. It is not a stable API and goes -away once generation routes itself. +deadline, submits one task and waits for it. On desktop that means loopback; +in server mode a remote caller needs `OMNIVOICE_API_KEY`. It is not a stable +API and goes away once generation routes itself. ## How work is placed diff --git a/frontend/src/pages/SupportPage.jsx b/frontend/src/pages/SupportPage.jsx index 04ce8285..99f7f9e1 100644 --- a/frontend/src/pages/SupportPage.jsx +++ b/frontend/src/pages/SupportPage.jsx @@ -523,9 +523,10 @@ export default function SupportPage({ onBack, initialView = 'support' }) { const id = SECTION_IDS[initialView] || SECTION_IDS.support; // rAF: the panel has to be laid out before an offset means anything. const frame = requestAnimationFrame(() => { - sectionRef.current - ?.querySelector(`#${id}`) - ?.scrollIntoView({ block: 'start', behavior: 'auto' }); + const section = sectionRef.current?.querySelector(`#${id}`); + if (typeof section?.scrollIntoView === 'function') { + section.scrollIntoView({ block: 'start', behavior: 'auto' }); + } }); return () => cancelAnimationFrame(frame); }, [initialView]); diff --git a/tests/backend/api/test_engines_route_shape.py b/tests/backend/api/test_engines_route_shape.py index f7131222..90834274 100644 --- a/tests/backend/api/test_engines_route_shape.py +++ b/tests/backend/api/test_engines_route_shape.py @@ -66,7 +66,7 @@ def fresh_app(monkeypatch, tmp_path): def _client(app, host="127.0.0.1"): """TestClient anchored to a loopback (or non-loopback) client tuple. - `require_loopback` reads `request.client.host`; the default + The admin dependency reads `request.client.host`; the default TestClient tuple is `('testclient', 50000)` which the dep rejects. """ from fastapi.testclient import TestClient @@ -614,12 +614,56 @@ def test_engine_health_unknown_id(fresh_app): assert "unknown engine id" in r.json()["detail"] -def test_engine_health_loopback_only(fresh_app): - """Non-loopback client tuple is rejected by require_loopback.""" +def test_engine_health_is_admin_gated(fresh_app): + """Non-loopback desktop traffic is rejected by require_admin.""" 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 required" + assert r.json()["detail"] == "loopback origin or admin API key required" + + +def test_server_mode_engine_mutations_require_api_key(fresh_app, monkeypatch): + monkeypatch.setenv("OMNIVOICE_SERVER_MODE", "1") + monkeypatch.delenv("OMNIVOICE_API_KEY", raising=False) + client = _client(fresh_app, host="172.17.0.1") + + requests = [ + ("post", "/engines/translation/deep-translator/install", None), + ("delete", "/engines/translation/deep-translator", None), + ("post", "/engines/sidecar/indextts2/install", None), + ("delete", "/engines/sidecar/indextts2/install", None), + ("post", "/engines/omnivoice/selftest", None), + ("post", "/engines/select", {}), + ] + for method, path, body in requests: + response = client.request(method.upper(), path, json=body) + assert response.status_code == 403, path + + +def test_server_mode_engine_health_requires_api_key(fresh_app, monkeypatch): + """Health is a GET but can spawn a sidecar, so it is not discovery.""" + monkeypatch.setenv("OMNIVOICE_SERVER_MODE", "1") + monkeypatch.delenv("OMNIVOICE_API_KEY", raising=False) + client = _client(fresh_app, host="172.17.0.1") + + response = client.get("/engines/omnivoice/health") + + assert response.status_code == 403 + + +def test_server_mode_sidecar_install_stays_desktop_only(fresh_app, monkeypatch): + """An API key cannot remotely trigger the mutable-source installer.""" + monkeypatch.setenv("OMNIVOICE_SERVER_MODE", "1") + monkeypatch.setenv("OMNIVOICE_API_KEY", "s3cret") + client = _client(fresh_app, host="172.17.0.1") + + response = client.post( + "/engines/sidecar/indextts2/install", + headers={"authorization": "Bearer s3cret"}, + ) + + assert response.status_code == 403 + assert response.json()["detail"] == "desktop origin required" def test_engine_health_caches_instance_across_calls(fresh_app, monkeypatch): @@ -747,10 +791,10 @@ def test_selftest_unknown_id_is_404(fresh_app): assert "unknown TTS engine id" in r.json()["detail"] -def test_selftest_loopback_only(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 required" + assert r.json()["detail"] == "loopback origin or admin API key required" def test_selftest_captures_synth_exception_without_500(fresh_app): diff --git a/tests/backend/test_engine_spawn_token.py b/tests/backend/test_engine_spawn_token.py index 054389ed..28bdc790 100644 --- a/tests/backend/test_engine_spawn_token.py +++ b/tests/backend/test_engine_spawn_token.py @@ -43,7 +43,7 @@ def fresh_app(monkeypatch, tmp_path): def _client(app): - """TestClient anchored to a loopback client tuple so require_loopback + """TestClient anchored to a loopback client tuple so require_admin treats requests as local. The default TestClient client tuple is ('testclient', 50000), which the dep rejects.""" from fastapi.testclient import TestClient @@ -78,7 +78,7 @@ def test_post_hf_token_loopback_succeeds(fresh_app, monkeypatch): def test_post_hf_token_non_loopback_returns_403(fresh_app): """A non-loopback origin (simulated via TestClient client tuple) is - rejected with 403 per the require_loopback dep.""" + rejected with 403 per the require_admin dep.""" from fastapi.testclient import TestClient with TestClient(fresh_app, client=("10.0.0.5", 12345)) as c: r = c.post("/api/settings/hf-token", json={"token": SAMPLE_TOKEN}) @@ -175,7 +175,7 @@ def test_get_hf_token_state_fresh_busts_whoami_cache(fresh_app, monkeypatch): def test_get_hf_token_state_loopback_only(fresh_app): - """GET state is on the same loopback-only router; non-loopback → 403.""" + """GET state is on the same admin router; non-loopback desktop → 403.""" from fastapi.testclient import TestClient with TestClient(fresh_app, client=("10.0.0.5", 12345)) as c: r = c.get("/api/settings/hf-token/state") diff --git a/tests/probe/_boot_runner.py b/tests/probe/_boot_runner.py index 7ac1720b..fd5e1ee3 100644 --- a/tests/probe/_boot_runner.py +++ b/tests/probe/_boot_runner.py @@ -77,7 +77,7 @@ def main() -> int: # Loopback security: a non-loopback origin must be rejected on system # routes. Use a bare client (no `with`) so we don't re-enter the app # lifespan — re-entry rebinds the module-level task queue to a new event - # loop and crashes. The require_loopback dependency only inspects + # loop and crashes. The require_admin dependency only inspects # request.client.host, which doesn't need lifespan state. nl = TestClient(app) # default client host 'testclient' = non-loopback ctx["loopback_reject_status"] = nl.get("/system/info").status_code diff --git a/tests/test_admin_route_policy.py b/tests/test_admin_route_policy.py new file mode 100644 index 00000000..b41b98ed --- /dev/null +++ b/tests/test_admin_route_policy.py @@ -0,0 +1,135 @@ +"""Static policy guard for server-mode administrative routes. + +The behavioural tests prove the dependencies themselves. This file proves +the dangerous routers are actually wired to the strict dependency; testing a +perfect guard is worthless when a route imports the legacy one instead. +""" + +from __future__ import annotations + +import ast +from pathlib import Path + +import pytest + + +ROOT = Path(__file__).resolve().parents[1] +ROUTERS = ROOT / "backend" / "api" / "routers" + + +def _tree(filename: str) -> ast.Module: + return ast.parse((ROUTERS / filename).read_text(encoding="utf-8")) + + +def _router_assignment(tree: ast.Module) -> ast.expr: + for node in tree.body: + if not isinstance(node, ast.Assign): + continue + if not any( + isinstance(target, ast.Name) and target.id == "router" + for target in node.targets + ): + continue + return node.value + raise AssertionError("router assignment not found") + + +def _route_decorators(tree: ast.Module, function_name: str) -> list[ast.expr]: + for node in tree.body: + if ( + isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)) + and node.name == function_name + ): + return node.decorator_list + raise AssertionError(f"route function not found: {function_name}") + + +def _dependency_names(nodes: ast.AST | list[ast.AST]) -> set[str]: + roots = nodes if isinstance(nodes, list) else [nodes] + names: set[str] = set() + for root in roots: + for node in ast.walk(root): + if not isinstance(node, ast.Call) or not node.args: + continue + if not isinstance(node.func, ast.Name) or node.func.id != "Depends": + continue + dependency = node.args[0] + if isinstance(dependency, ast.Name): + names.add(dependency.id) + return names + + +def _mutating_route_functions( + tree: ast.Module, +) -> list[ast.FunctionDef | ast.AsyncFunctionDef]: + mutating_methods = {"post", "put", "patch", "delete"} + functions = [] + for node in tree.body: + if not isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)): + continue + for decorator in node.decorator_list: + if not isinstance(decorator, ast.Call): + continue + route = decorator.func + if ( + isinstance(route, ast.Attribute) + and isinstance(route.value, ast.Name) + and route.value.id == "router" + and route.attr in mutating_methods + ): + functions.append(node) + break + return functions + + +@pytest.mark.parametrize( + "filename", + [ + "mcp_bindings.py", + "media_tools.py", + "pronunciation.py", + "settings.py", + "system.py", + "workers.py", + ], +) +def test_privileged_router_uses_method_aware_admin_guard(filename): + dependencies = _dependency_names(_router_assignment(_tree(filename))) + assert "require_admin" in dependencies + assert "require_loopback" not in dependencies + + +def test_every_mutating_engine_route_uses_method_aware_admin_guard(): + functions = _mutating_route_functions(_tree("engines.py")) + assert functions + for function in functions: + dependencies = _dependency_names(function.decorator_list) + assert "require_admin" in dependencies, function.name + assert "require_loopback" not in dependencies, function.name + + +def test_sidecar_install_status_uses_method_aware_admin_guard(): + dependencies = _dependency_names( + _route_decorators(_tree("engines.py"), "sidecar_install_status") + ) + assert "require_admin" in dependencies + + +def test_managed_sidecar_install_stays_desktop_only(): + dependencies = _dependency_names( + _route_decorators(_tree("engines.py"), "install_sidecar_engine") + ) + assert {"require_admin", "require_desktop"} <= dependencies + + +@pytest.mark.parametrize( + ("filename", "function_name"), + [ + ("engines.py", "engine_health"), + ("settings.py", "list_llm_provider_models"), + ("system.py", "system_diagnose"), + ], +) +def test_side_effectful_get_requires_strict_admin_action(filename, function_name): + dependencies = _dependency_names(_route_decorators(_tree(filename), function_name)) + assert "require_admin_action" in dependencies diff --git a/tests/test_api.py b/tests/test_api.py index fe12b1e0..2f591017 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -74,7 +74,7 @@ def client(): `client=("127.0.0.1", 50000)` makes `request.client.host` resolve to a loopback address — required because `backend/api/routers/system.py` is - now gated by a router-level `require_loopback` dependency. Tests that + now gated by a router-level `require_admin` dependency. Tests that deliberately exercise the non-loopback rejection path build their own plain `TestClient(app)` (which defaults to host='testclient'). """ @@ -551,7 +551,7 @@ class TestStreamingTTS: def test_set_env_rejects_non_loopback(): """A TestClient that does NOT override `client=` sets `request.client.host = 'testclient'` (non-loopback). The router-level - `require_loopback` dependency must return 403 and must NOT mutate + `require_admin` dependency must return 403 and must NOT mutate os.environ. NOTE: the project-wide `client` fixture is now built with a loopback override so most tests see protected routes — this test instantiates its own plain client to exercise the rejection path.""" diff --git a/tests/test_bearer_middleware.py b/tests/test_bearer_middleware.py index 699df0dd..42054512 100644 --- a/tests/test_bearer_middleware.py +++ b/tests/test_bearer_middleware.py @@ -30,6 +30,24 @@ def test_inert_without_env(monkeypatch): assert c.get("/health").status_code == 200 +def test_whitespace_only_env_is_not_an_api_key(monkeypatch): + monkeypatch.setenv("OMNIVOICE_API_KEY", " ") + c = _client() + response = c.get("/v1/audio/voices") + assert response.status_code == 200 + assert isinstance(response.json().get("voices"), list) + + +def test_whitespace_query_does_not_shadow_valid_cookie(key_env): + c = _client() + c.cookies.set("ov_key", key_env) + + response = c.get("/v1/audio/voices?api_key=%20%20%20") + + assert response.status_code == 200 + assert isinstance(response.json().get("voices"), list) + + def test_loopback_bypasses_key(key_env): c = _client(("127.0.0.1", 1)) assert c.get("/system/info").status_code == 200 diff --git a/tests/test_generate_engine.py b/tests/test_generate_engine.py index cb36ef8f..ae3ab57c 100644 --- a/tests/test_generate_engine.py +++ b/tests/test_generate_engine.py @@ -84,7 +84,7 @@ def client(): # module's event loop and broke teardown when the full suite mixes it # with the non-lifespan TestClients every other test file uses # (test_api.py pattern). Loopback client addr: required by the - # router-level require_loopback dependency. + # router-level require_admin dependency. from fastapi.testclient import TestClient from main import app diff --git a/tests/test_loopback_server_mode.py b/tests/test_loopback_server_mode.py index 0738a31f..775fafb2 100644 --- a/tests/test_loopback_server_mode.py +++ b/tests/test_loopback_server_mode.py @@ -1,9 +1,9 @@ -"""`require_loopback` gate contract (issue #261). +"""Server-mode origin and admin gate contracts (issue #261). The gate must stay strict on the desktop build (non-loopback → 403, which is the -PR #81 trust boundary), but become a no-op in the headless Docker server mode, -where Docker's NAT makes the loopback origin unenforceable and exposure is -governed by the port mapping + the share PIN instead. +PR #81 trust boundary). Docker NAT makes the host operator appear non-loopback, +so bare server mode keeps read-only discovery open while every mutation still +requires the long admin API key. """ from types import SimpleNamespace @@ -30,6 +30,10 @@ def require_admin(*args, **kwargs): return _dependency("require_admin")(*args, **kwargs) +def require_admin_action(*args, **kwargs): + return _dependency("require_admin_action")(*args, **kwargs) + + def require_desktop(*args, **kwargs): return _dependency("require_desktop")(*args, **kwargs) @@ -88,7 +92,7 @@ def test_falsey_server_mode_keeps_gate_strict(monkeypatch, val): # Trusted local networks (OMNIVOICE_TRUSTED_NETWORKS) — issue #1170. # A self-hoster can name CIDRs treated as trusted by the CONSUMPTION gates # (PIN/API-key/WS), so a LAN or reverse proxy is exempted. Admin gates -# (require_loopback) stay true-loopback-only — two-tier privilege model. +# (require_admin) stay true-loopback-only — two-tier privilege model. @pytest.mark.parametrize("host", ["127.0.0.1", "::1", "localhost"]) @@ -178,12 +182,101 @@ def _req_full(host, *, headers=None, query=None, cookies=None, pin=None, method= ) +@pytest.mark.parametrize("method", ["POST", "PUT", "PATCH", "DELETE"]) +def test_legacy_loopback_guard_fails_closed_for_server_mode_mutations( + monkeypatch, method +): + """A stale route guard must not reopen writes in a bare Docker server. + + ``require_admin`` is the explicit dependency for privileged routers, but + this fallback closes the whole bug class: a future mutation that + accidentally keeps ``require_loopback`` still requires the long API key. + """ + monkeypatch.setenv("OMNIVOICE_SERVER_MODE", "1") + monkeypatch.delenv("OMNIVOICE_API_KEY", raising=False) + + with pytest.raises(HTTPException) as exc: + require_loopback(_req_full("172.17.0.1", method=method)) + + assert exc.value.status_code == 403 + + +def test_legacy_loopback_guard_allows_authenticated_server_mode_mutation(monkeypatch): + monkeypatch.setenv("OMNIVOICE_SERVER_MODE", "1") + monkeypatch.setenv("OMNIVOICE_API_KEY", "s3cret") + + require_loopback( + _req_full( + "172.17.0.1", + method="POST", + headers={"authorization": "Bearer s3cret"}, + ) + ) + + +def test_server_mode_side_effectful_get_requires_api_key(monkeypatch): + monkeypatch.setenv("OMNIVOICE_SERVER_MODE", "1") + monkeypatch.delenv("OMNIVOICE_API_KEY", raising=False) + + with pytest.raises(HTTPException) as exc: + require_admin_action(_req_full("172.17.0.1", method="GET")) + + assert exc.value.status_code == 403 + + +def test_server_mode_side_effectful_get_accepts_api_key(monkeypatch): + monkeypatch.setenv("OMNIVOICE_SERVER_MODE", "1") + monkeypatch.setenv("OMNIVOICE_API_KEY", "s3cret") + + require_admin_action( + _req_full( + "172.17.0.1", + method="GET", + headers={"authorization": "Bearer s3cret"}, + ) + ) + + +def test_side_effectful_get_rejects_remote_api_key_outside_server_mode(monkeypatch): + 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 + + +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") + monkeypatch.delenv("OMNIVOICE_API_KEY", raising=False) + + with pytest.raises(HTTPException) as exc: + require_admin_action( + _req_full( + "10.1.2.3", + method="GET", + pin="123456", + headers={"x-omnivoice-pin": "123456"}, + ) + ) + + assert exc.value.status_code == 403 + + # Server mode + trusted network + credential — issue #1213. # Regression for the two-tier collapse: with OMNIVOICE_SERVER_MODE=1 the # loopback origin is unenforceable, so admin can't require true loopback. But a -# configured credential (API key / PIN) must still gate admin — a trusted-network -# client that presents NO credential must NOT reach /system/* or /api/settings/* -# just because is_local_host exempts it from the consumption middleware. +# configured API key must still gate admin — a trusted-network client that +# presents NO key must NOT reach /system/* or /api/settings/* just because +# is_local_host exempts it from the consumption middleware. def test_server_mode_trusted_network_no_credential_reaches_admin(monkeypatch): @@ -232,21 +325,36 @@ def test_server_mode_admin_rejects_wrong_api_key(monkeypatch): assert exc.value.status_code == 403 -def test_server_mode_pin_does_not_unlock_admin(monkeypatch): +def test_server_mode_pin_only_keeps_admin_loopback_only(monkeypatch): # CodeRabbit #1213: the 6-digit share PIN is a CONSUMPTION credential and is # brute-forceable (10^6, no lockout), so it must NEVER gate the RCE-class - # admin surface. With a PIN set but no API key, admin is still *gated* (not - # left open) — but only loopback or the long API key can reach it. Presenting - # even the correct PIN over the network does NOT unlock admin. + # admin surface. Once a PIN is configured, bare read-only discovery closes; + # presenting that PIN still cannot authorize either a read or a write. monkeypatch.setenv("OMNIVOICE_SERVER_MODE", "1") monkeypatch.setenv("OMNIVOICE_TRUSTED_NETWORKS", "10.0.0.0/8") monkeypatch.delenv("OMNIVOICE_API_KEY", raising=False) - # No PIN presented → 403. + # No PIN presented → discovery denied. with pytest.raises(HTTPException): require_loopback(_req_full("10.1.2.3", pin="1234")) - # Correct PIN presented → STILL 403 (the PIN never gates admin). + # Correct PIN presented → STILL denied (the PIN never gates admin). with pytest.raises(HTTPException): - require_loopback(_req_full("10.1.2.3", pin="1234", headers={"x-omnivoice-pin": "1234"})) + require_loopback( + _req_full( + "10.1.2.3", + pin="1234", + headers={"x-omnivoice-pin": "1234"}, + ) + ) + # Mutations are denied for the same reason. + with pytest.raises(HTTPException): + require_loopback( + _req_full( + "10.1.2.3", + pin="1234", + method="POST", + headers={"x-omnivoice-pin": "1234"}, + ) + ) # Loopback admin still needs no credential (the local operator path)… require_loopback(_req_full("127.0.0.1", pin="1234")) # …and the trusted client keeps its consumption exemption. @@ -261,9 +369,9 @@ def test_server_mode_loopback_admin_never_needs_credential(monkeypatch): require_loopback(_req_full("127.0.0.1")) # must not raise -# GHAS #506/#440/#441: require_loopback permits an unconfigured bare Docker -# server for compatibility. RCE/filesystem-capable routers use the stricter, -# method-aware admin gate instead. +# GHAS #506/#440/#441: bare Docker retains read-only discovery for bootstrap. +# RCE/filesystem-capable routers use the method-aware admin gate, while the +# legacy loopback guard independently fails closed on accidental mutations. @pytest.mark.parametrize("method", ["POST", "PUT", "PATCH", "DELETE"]) @@ -281,10 +389,12 @@ def test_server_mode_admin_read_keeps_bare_docker_bootstrap(monkeypatch): require_admin(_req_full("172.17.0.1", method="GET")) -def test_server_mode_admin_read_keeps_pin_only_discovery(monkeypatch): +def test_server_mode_admin_read_rejects_pin_only_deployment(monkeypatch): monkeypatch.setenv("OMNIVOICE_SERVER_MODE", "1") monkeypatch.delenv("OMNIVOICE_API_KEY", raising=False) - require_admin(_req_full("172.17.0.1", method="GET", pin="123456")) + with pytest.raises(HTTPException) as exc: + require_admin(_req_full("172.17.0.1", method="GET", pin="123456")) + assert exc.value.status_code == 403 def test_server_mode_admin_mutation_allows_api_key(monkeypatch): @@ -297,6 +407,40 @@ def test_server_mode_admin_mutation_allows_api_key(monkeypatch): )) +def test_whitespace_only_api_key_cannot_authorize_admin_mutation(monkeypatch): + monkeypatch.setenv("OMNIVOICE_SERVER_MODE", "1") + monkeypatch.setenv("OMNIVOICE_API_KEY", " ") + + for credential in ( + {"query": {"api_key": " "}}, + {"cookies": {"ov_key": " "}}, + ): + with pytest.raises(HTTPException) as exc: + require_admin( + _req_full("172.17.0.1", method="POST", **credential) + ) + assert exc.value.status_code == 403 + + monkeypatch.setenv("OMNIVOICE_API_KEY", " s3cret ") + require_admin( + _req_full("172.17.0.1", method="POST", query={"api_key": " s3cret "}) + ) + + +def test_whitespace_query_does_not_shadow_admin_key_cookie(monkeypatch): + monkeypatch.setenv("OMNIVOICE_SERVER_MODE", "1") + monkeypatch.setenv("OMNIVOICE_API_KEY", "s3cret") + + require_admin( + _req_full( + "172.17.0.1", + method="POST", + query={"api_key": " "}, + cookies={"ov_key": "s3cret"}, + ) + ) + + def test_server_mode_desktop_capability_rejects_remote_api_key(monkeypatch): monkeypatch.setenv("OMNIVOICE_SERVER_MODE", "1") monkeypatch.setenv("OMNIVOICE_API_KEY", "s3cret") diff --git a/tests/test_mcp_bindings.py b/tests/test_mcp_bindings.py index 93935963..f28e408a 100644 --- a/tests/test_mcp_bindings.py +++ b/tests/test_mcp_bindings.py @@ -206,6 +206,20 @@ def test_rest_crud_roundtrip(client): assert client.delete("/api/mcp/bindings/claude-code").status_code == 404 +def test_server_mode_binding_mutations_require_api_key(client, monkeypatch): + from fastapi.testclient import TestClient + + monkeypatch.setenv("OMNIVOICE_SERVER_MODE", "1") + monkeypatch.delenv("OMNIVOICE_API_KEY", raising=False) + remote = TestClient(client.app, client=("172.17.0.1", 50000)) + + assert remote.get("/api/mcp/bindings").status_code == 200 + assert remote.put( + "/api/mcp/bindings", json={"client_id": "attacker"} + ).status_code == 403 + assert remote.delete("/api/mcp/bindings/attacker").status_code == 403 + + def test_rest_rejects_empty_client_id(client): r = client.put("/api/mcp/bindings", json={"client_id": ""}) assert r.status_code == 422 # pydantic min_length diff --git a/tests/test_media_tools.py b/tests/test_media_tools.py index 94cd8f1e..0483b27f 100644 --- a/tests/test_media_tools.py +++ b/tests/test_media_tools.py @@ -432,7 +432,7 @@ def test_router_ytdlp_routes_not_shadowed_by_tool_param(mt, monkeypatch): assert r.json()["state"] == "running" -def test_router_is_loopback_gated(mt): +def test_router_is_admin_gated(mt): from main import app c = TestClient(app) # client.host = 'testclient' → non-loopback for method, path in [ @@ -442,4 +442,21 @@ def test_router_is_loopback_gated(mt): ("post", "/media-tools/ytdlp/update"), ]: r = getattr(c, method)(path) - assert r.status_code == 403, f"{path} must be loopback-only" + assert r.status_code == 403, f"{path} must be admin-only" + + +def test_server_mode_media_tool_mutations_require_api_key(mt, monkeypatch): + from main import app + + monkeypatch.setenv("OMNIVOICE_SERVER_MODE", "1") + monkeypatch.delenv("OMNIVOICE_API_KEY", raising=False) + remote = TestClient(app, client=("172.17.0.1", 50000)) + + assert remote.get("/media-tools/status").status_code == 200 + for path in ( + "/media-tools/acquire", + "/media-tools/ytdlp/update", + "/media-tools/ytdlp/restore", + "/media-tools/ffmpeg/use-system", + ): + assert remote.post(path).status_code == 403, path diff --git a/tests/test_pronunciation_api.py b/tests/test_pronunciation_api.py index 416fa558..f7da1873 100644 --- a/tests/test_pronunciation_api.py +++ b/tests/test_pronunciation_api.py @@ -217,6 +217,22 @@ def test_crud_roundtrip(client): assert client.get("/pronunciation").json() == [] +def test_server_mode_pronunciation_mutations_require_api_key(client, monkeypatch): + from fastapi.testclient import TestClient + + monkeypatch.setenv("OMNIVOICE_SERVER_MODE", "1") + monkeypatch.delenv("OMNIVOICE_API_KEY", raising=False) + remote = TestClient(client.app, client=("172.17.0.1", 50000)) + + assert remote.get("/pronunciation").status_code == 200 + assert remote.post( + "/pronunciation", json={"term": "GIF", "replacement": "jiff"} + ).status_code == 403 + assert remote.post( + "/pronunciation/import", json={"entries": [], "replace": True} + ).status_code == 403 + + def test_create_rejects_blank_term(client): assert client.post("/pronunciation", json={"term": " "}).status_code == 400 diff --git a/tests/test_router_smoke.py b/tests/test_router_smoke.py index 84f2e238..083bf3f6 100644 --- a/tests/test_router_smoke.py +++ b/tests/test_router_smoke.py @@ -40,7 +40,7 @@ def client(): # `client=("127.0.0.1", 50000)` so `request.client.host` resolves to a # loopback address — the system router is gated by a router-level - # `require_loopback` dependency. Smoke tests are happy-path tests and + # `require_admin` dependency. Smoke tests are happy-path tests and # should pass the gate. return TestClient(app, client=("127.0.0.1", 50000)) diff --git a/tests/test_worker_join_api.py b/tests/test_worker_join_api.py index 0024ac45..732aa488 100644 --- a/tests/test_worker_join_api.py +++ b/tests/test_worker_join_api.py @@ -24,14 +24,14 @@ import pytest from fastapi import FastAPI from fastapi.testclient import TestClient -from api.dependencies import require_loopback +from api.dependencies import require_admin from api.routers import workers as workers_router from worker import agent as worker_agent @pytest.fixture def client(monkeypatch, tmp_path): - """The workers router with the loopback guard stubbed out.""" + """The workers router with the admin guard stubbed out.""" settings: dict[str, str] = {} class _Store: @@ -59,7 +59,7 @@ def client(monkeypatch, tmp_path): app = FastAPI() app.include_router(workers_router.router) - app.dependency_overrides[require_loopback] = lambda: None + app.dependency_overrides[require_admin] = lambda: None with TestClient(app) as c: yield c, settings diff --git a/tests/test_worker_service_api.py b/tests/test_worker_service_api.py index cd5ab744..fedd2c8a 100644 --- a/tests/test_worker_service_api.py +++ b/tests/test_worker_service_api.py @@ -214,17 +214,17 @@ def _app(): @pytest.fixture def client(db): - """A client that satisfies the loopback gate. + """A client that satisfies the admin gate. The gate is real and is exercised separately below; overriding it here keeps every other test about the endpoint's own behaviour. """ from fastapi.testclient import TestClient - from api.dependencies import require_loopback + from api.dependencies import require_admin app = _app() - app.dependency_overrides[require_loopback] = lambda: None + app.dependency_overrides[require_admin] = lambda: None return TestClient(app) @@ -239,6 +239,20 @@ def test_management_endpoints_are_loopback_only(db): assert unguarded.delete("/workers/anything").status_code == 403 +def test_server_mode_worker_mutations_require_api_key(db, monkeypatch): + """Bare Docker discovery stays usable; its worker controls stay closed.""" + from fastapi.testclient import TestClient + + monkeypatch.setenv("OMNIVOICE_SERVER_MODE", "1") + monkeypatch.delenv("OMNIVOICE_API_KEY", raising=False) + remote = TestClient(_app(), client=("172.17.0.1", 50000)) + + assert remote.get("/workers").status_code == 200 + assert remote.post("/workers/enabled", json={"enabled": True}).status_code == 403 + assert remote.post("/workers/agent/join", json={"token": "hostile"}).status_code == 403 + assert remote.delete("/workers/anything").status_code == 403 + + def test_listing_workers_is_safe_when_the_feature_is_off(client): response = client.get("/workers") assert response.status_code == 200