From f8eeb5a96381e22dc5c9b937f6870ac1ef1efe8c Mon Sep 17 00:00:00 2001 From: debpalash <4178343+debpalash@users.noreply.github.com> Date: Sun, 9 Aug 2026 21:01:14 +0000 Subject: [PATCH 01/22] fix(security): require API key for remote admin writes --- backend/api/dependencies.py | 26 ++++++++++++++++ backend/api/routers/settings.py | 13 ++++---- backend/api/routers/system.py | 15 +++++----- docs/api-auth.md | 5 ++-- tests/test_api.py | 25 +++++++++++++++- tests/test_loopback_server_mode.py | 48 ++++++++++++++++++++++++++++-- tests/test_models_dir_setting.py | 17 +++++++++++ 7 files changed, 129 insertions(+), 20 deletions(-) diff --git a/backend/api/dependencies.py b/backend/api/dependencies.py index 83bf17a5..3fcb2f7f 100644 --- a/backend/api/dependencies.py +++ b/backend/api/dependencies.py @@ -186,6 +186,32 @@ def require_loopback(request: Request) -> None: raise HTTPException(status_code=403, detail="loopback origin required") +def require_admin(request: Request) -> None: + """Gate RCE/filesystem-capable admin routers. + + Desktop callers keep the loopback-only contract. Docker cannot reliably + observe the host operator as loopback, so authenticated remote admin stays + available there, but every state-changing request must present the long API + key. An unconfigured server must never expose executable-path or filesystem + settings to every client that can reach its published port. + + Read-only requests retain the bare-Docker bootstrap behaviour until an API + key is configured. Share PINs and trusted CIDRs are consumption credentials; + neither authorizes this gate. + """ + host = request.client.host if request.client else None + if is_loopback(host): + return + if _server_mode(): + method = str(getattr(request, "method", "GET")).upper() + read_only = method in {"GET", "HEAD", "OPTIONS"} + 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_local(request: Request) -> None: """Reject any request whose client.host is not loopback OR on a configured trusted network. The consumption-tier companion to :func:`require_loopback`: diff --git a/backend/api/routers/settings.py b/backend/api/routers/settings.py index b4394f50..f47d00ff 100644 --- a/backend/api/routers/settings.py +++ b/backend/api/routers/settings.py @@ -1,11 +1,10 @@ """Settings API — HF token save/clear/state endpoints (Phase 1 AUTH-03 backend half). These endpoints are the backend half of the Wave 2 Settings → API Keys -panel. Threat T-01-03 mitigation: every write endpoint is gated by the -router-level `require_loopback` dep, so non-loopback origins get 403 -before the handler runs. Reads are loopback-gated too — the masked -token preview is useful telemetry that we still don't want exposed on -the LAN. +panel. Threat T-01-03 mitigation: the router-level `require_admin` dependency +keeps desktop callers loopback-only and requires the long API key for every +remote server-mode mutation. Read-only bare-Docker discovery remains available +until an API key is configured; once configured, reads require it too. The state endpoint duplicates `/system/hf-token/state` (which lives on `system.py` for legacy-router compatibility); both return the same shape. @@ -20,14 +19,14 @@ from dataclasses import asdict from fastapi import APIRouter, Depends, HTTPException, Query from pydantic import BaseModel, Field -from api.dependencies import require_loopback +from api.dependencies import require_admin logger = logging.getLogger("omnivoice.api.settings") router = APIRouter( prefix="/api/settings", tags=["settings"], - dependencies=[Depends(require_loopback)], + dependencies=[Depends(require_admin)], ) diff --git a/backend/api/routers/system.py b/backend/api/routers/system.py index 89fc1856..be10f50e 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 require_loopback +from api.dependencies import require_admin from fastapi.responses import FileResponse, StreamingResponse import torch import shutil @@ -21,17 +21,16 @@ from core.version import APP_VERSION from services.model_manager import get_model_status, get_best_device, resolve_omnivoice_checkpoint from services.ffmpeg_utils import find_ffmpeg, run_ffmpeg -# Router-level loopback gate. Every route mounted on `router` (GET + POST, -# present and future) is gated by `require_loopback`, which 403s any request -# whose `client.host` is not a loopback address. This closes the same trust +# Router-level admin gate. Every route mounted on `router` (GET + POST, +# present and future) is gated by `require_admin`: desktop requests must be +# loopback; server-mode mutations require the long API key. This closes the trust # boundary that PR #81 only patched on `/system/set-env` and that the # 260518-ivy deferred-items file enumerated for follow-up: /model/unload/*, # /system/logs/clear, /system/logs/tauri/clear, /system/flush-memory, # /clean-audio (POSTs) plus the read-side info-disclosure routes # /system/info, /system/logs, /system/logs/tauri, /system/logs/stream. -# This router only ever serves the local Tauri shell and the dev frontend -# at http://127.0.0.1:3901 — both are loopback origins. -router = APIRouter(dependencies=[Depends(require_loopback)]) +# Native Tauri/dev callers remain loopback and need no credential. +router = APIRouter(dependencies=[Depends(require_admin)]) logger = logging.getLogger("omnivoice.api") # Cache device checks at module load — they don't change at runtime @@ -843,7 +842,7 @@ async def set_env_var(body: dict): are set on ``os.environ`` for the running process. The loopback-origin gate that previously lived inline here is now applied - at the router level via `dependencies=[Depends(require_loopback)]` on + at the router level via `dependencies=[Depends(require_admin)]` on `router` — see the top of this file. Every route on this router is gated, including this one. The 403 body and behavior are unchanged. """ diff --git a/docs/api-auth.md b/docs/api-auth.md index f707e3f1..4de1ea6e 100644 --- a/docs/api-auth.md +++ b/docs/api-auth.md @@ -206,8 +206,9 @@ 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 credential configured** (no API key, no PIN) → admin is open. The bare - Docker flow; exposure rests entirely on your port mapping / firewall. +- **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: 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 diff --git a/tests/test_api.py b/tests/test_api.py index 182b971b..b629140f 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -600,6 +600,30 @@ def test_set_env_allows_loopback(): os.environ["HF_TOKEN"] = original +def test_server_mode_remote_without_api_key_cannot_set_executable_path(monkeypatch, tmp_path): + """GHAS #506: bare Docker exposure must not become an executable setter.""" + from fastapi.testclient import TestClient + from main import app + + executable = tmp_path / "ffmpeg" + executable.write_bytes(b"not actually executable") + original = os.environ.get("FFMPEG_PATH") + monkeypatch.setenv("OMNIVOICE_SERVER_MODE", "1") + monkeypatch.delenv("OMNIVOICE_API_KEY", raising=False) + try: + response = TestClient(app, client=("172.17.0.1", 50000)).post( + "/system/set-env", + json={"key": "FFMPEG_PATH", "value": str(executable)}, + ) + assert response.status_code == 403 + assert os.environ.get("FFMPEG_PATH") == original + finally: + if original is None: + os.environ.pop("FFMPEG_PATH", None) + else: + os.environ["FFMPEG_PATH"] = original + + def test_set_env_loopback_still_validates_allowlist(): """Even on the loopback path, keys outside the allow-list must return 400 — the new guard must NOT bypass the existing allow-list enforcement.""" @@ -686,4 +710,3 @@ def test_static_audio_served_with_canonical_mime(): ) finally: tmp_wav.unlink(missing_ok=True) - diff --git a/tests/test_loopback_server_mode.py b/tests/test_loopback_server_mode.py index 8145e248..a4b70877 100644 --- a/tests/test_loopback_server_mode.py +++ b/tests/test_loopback_server_mode.py @@ -10,7 +10,13 @@ from types import SimpleNamespace import pytest from fastapi import HTTPException -from api.dependencies import is_loopback, is_local_host, require_local, require_loopback +from api.dependencies import ( + is_loopback, + is_local_host, + require_admin, + require_local, + require_loopback, +) def _req(host): @@ -134,7 +140,7 @@ def test_require_local_rejects_untrusted_non_loopback(monkeypatch): assert exc.value.status_code == 403 -def _req_full(host, *, headers=None, query=None, cookies=None, pin=None): +def _req_full(host, *, headers=None, query=None, cookies=None, pin=None, method="GET"): """Richer stub carrying the channels the admin-credential check reads: headers, query params, cookies, and app.state.network_share.pin.""" ns = SimpleNamespace(pin=pin) if pin is not None else None @@ -145,6 +151,7 @@ def _req_full(host, *, headers=None, query=None, cookies=None, pin=None): query_params=query or {}, cookies=cookies or {}, app=app, + method=method, ) @@ -231,6 +238,43 @@ 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. + + +@pytest.mark.parametrize("method", ["POST", "PUT", "PATCH", "DELETE"]) +def test_server_mode_admin_mutation_requires_api_key_when_unconfigured(monkeypatch, method): + 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", method=method)) + assert exc.value.status_code == 403 + + +def test_server_mode_admin_read_keeps_bare_docker_bootstrap(monkeypatch): + monkeypatch.setenv("OMNIVOICE_SERVER_MODE", "1") + monkeypatch.delenv("OMNIVOICE_API_KEY", raising=False) + require_admin(_req_full("172.17.0.1", method="GET")) + + +def test_server_mode_admin_mutation_allows_api_key(monkeypatch): + monkeypatch.setenv("OMNIVOICE_SERVER_MODE", "1") + monkeypatch.setenv("OMNIVOICE_API_KEY", "s3cret") + require_admin(_req_full( + "172.17.0.1", + method="POST", + headers={"authorization": "Bearer s3cret"}, + )) + + +@pytest.mark.parametrize("method", ["GET", "POST", "PUT", "DELETE"]) +def test_loopback_admin_never_needs_api_key(monkeypatch, method): + monkeypatch.setenv("OMNIVOICE_SERVER_MODE", "1") + monkeypatch.delenv("OMNIVOICE_API_KEY", raising=False) + require_admin(_req_full("127.0.0.1", method=method)) + + def test_is_local_host_unwraps_ipv4_mapped_ipv6(monkeypatch): # Dual-stack proxies (Caddy, Node.js) pass ::ffff:192.168.1.5 — should # match an IPv4 CIDR after unwrapping the mapped address. diff --git a/tests/test_models_dir_setting.py b/tests/test_models_dir_setting.py index 6ac7532c..e825bfd1 100644 --- a/tests/test_models_dir_setting.py +++ b/tests/test_models_dir_setting.py @@ -59,6 +59,23 @@ def test_rejects_path_with_null_byte(env): assert ei.value.status_code == 400 +def test_server_mode_remote_without_api_key_cannot_create_models_dir(env, monkeypatch, tmp_path): + """GHAS #440/#441: a published bare Docker port is not filesystem auth.""" + from fastapi.testclient import TestClient + + monkeypatch.setenv("OMNIVOICE_SERVER_MODE", "1") + monkeypatch.delenv("OMNIVOICE_API_KEY", raising=False) + target = tmp_path / "must-not-exist" + app = fastapi.FastAPI() + app.include_router(s.router) + response = TestClient(app, client=("172.17.0.1", 50000)).put( + "/api/settings/storage/models-dir", + json={"path": str(target)}, + ) + assert response.status_code == 403 + assert not target.exists() + + def test_clear_reverts_to_default(env): user_env.set_user_env("OMNIVOICE_CACHE_DIR", "/old") res = s.set_models_dir(s._ModelsDirBody(path="")) From afc51b2920138a4306028bfc4d5cb3bab55b1286 Mon Sep 17 00:00:00 2001 From: debpalash <4178343+debpalash@users.noreply.github.com> Date: Sun, 9 Aug 2026 21:01:40 +0000 Subject: [PATCH 02/22] docs: note remote admin mutation hardening --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 50771571..a0efaf59 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,6 +41,7 @@ The bundled TTS model package (`pyproject.toml`) is versioned independently. ### Fixed +- Remote Docker clients can no longer change executable paths or filesystem settings unless they present the admin API key; local desktop behavior is unchanged. (#1448) - Sidecar engines no longer break when a library they load prints to the console. Those bytes landed in the middle of the engine's data stream, failing the generation and leaving the connection scrambled for every request after it. (#1428) — thanks @1335-Group! - A generation abandoned while stuck on an internal lock now says so, instead of blaming your hardware and suggesting shorter text. Nothing had been computed, so none of that advice applied. (#1416, #1419) - A machine with a GPU that ends up on CPU now says why — a missing device node, a permissions problem, a card newer than the installed ROCm, an `HSA_OVERRIDE_GFX_VERSION` that is doing more harm than good, or an NVIDIA driver the container can't reach each read differently. Before, all of them looked identical to having no GPU at all. (#1274, #1228) From 7abc07e7f37b28d216a34d3e499371813b7c08b3 Mon Sep 17 00:00:00 2001 From: debpalash <4178343+debpalash@users.noreply.github.com> Date: Sun, 9 Aug 2026 21:10:51 +0000 Subject: [PATCH 03/22] fix(security): keep host paths desktop-only --- backend/api/dependencies.py | 15 ++++++++++++++- backend/api/routers/settings.py | 4 ++-- backend/api/routers/system.py | 4 ++-- docs/api-auth.md | 8 +++++++- tests/test_api.py | 25 +++++++++++++++++++++++++ tests/test_loopback_server_mode.py | 20 ++++++++++++++++++++ tests/test_models_dir_setting.py | 18 ++++++++++++++++++ 7 files changed, 88 insertions(+), 6 deletions(-) diff --git a/backend/api/dependencies.py b/backend/api/dependencies.py index 3fcb2f7f..2c8bc394 100644 --- a/backend/api/dependencies.py +++ b/backend/api/dependencies.py @@ -205,13 +205,26 @@ def require_admin(request: Request) -> None: if _server_mode(): method = str(getattr(request, "method", "GET")).upper() read_only = method in {"GET", "HEAD", "OPTIONS"} - if read_only and not _admin_credential_configured(request): + if read_only and not os.environ.get("OMNIVOICE_API_KEY", "").strip(): return if _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. + + An API key authorizes remote administration, not access to the desktop + shell's native file-picker boundary. These capabilities therefore remain + strictly loopback-only even when server mode is enabled. + """ + host = request.client.host if request.client else None + if is_loopback(host): + return + raise HTTPException(status_code=403, detail="desktop origin required") + + def require_local(request: Request) -> None: """Reject any request whose client.host is not loopback OR on a configured trusted network. The consumption-tier companion to :func:`require_loopback`: diff --git a/backend/api/routers/settings.py b/backend/api/routers/settings.py index f47d00ff..d15f1a94 100644 --- a/backend/api/routers/settings.py +++ b/backend/api/routers/settings.py @@ -19,7 +19,7 @@ from dataclasses import asdict from fastapi import APIRouter, Depends, HTTPException, Query from pydantic import BaseModel, Field -from api.dependencies import require_admin +from api.dependencies import require_admin, require_desktop logger = logging.getLogger("omnivoice.api.settings") @@ -703,7 +703,7 @@ def get_models_dir(): } -@router.put("/storage/models-dir") +@router.put("/storage/models-dir", dependencies=[Depends(require_desktop)]) def set_models_dir(body: _ModelsDirBody): """Set (or clear, with an empty path) the models download directory. diff --git a/backend/api/routers/system.py b/backend/api/routers/system.py index be10f50e..d9637090 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 require_admin +from api.dependencies import require_admin, require_desktop from fastapi.responses import FileResponse, StreamingResponse import torch import shutil @@ -831,7 +831,7 @@ except Exception: # pragma: no cover — defensive: env panel > installer wirin _PORT_KEYS = {"OMNIVOICE_PORT", "OMNIVOICE_SHARE_PORT", "OMNIVOICE_UI_PORT"} -@router.post("/system/set-env") +@router.post("/system/set-env", dependencies=[Depends(require_desktop)]) async def set_env_var(body: dict): """Set an environment variable at runtime, persisted across restarts. diff --git a/docs/api-auth.md b/docs/api-auth.md index 4de1ea6e..2f402111 100644 --- a/docs/api-auth.md +++ b/docs/api-auth.md @@ -21,7 +21,8 @@ tools keep working unchanged whichever gate is set. > VoiceStudio separates **consumption** (TTS, dictation, voices) from > **administration** (`/system/*`, `/api/settings/*` — RCE-class). The PIN and > trusted networks are *consumption* credentials; the **admin surface is only -> ever reached from loopback or with the API key** (see [Admin routes](#admin-routes-and-server-mode)). +> ever reached from loopback or with the API key**. Host-path capabilities stay +> desktop-only even with a key (see [Admin routes](#admin-routes-and-server-mode)). > Both gates can be active at once. The PIN and the API key are independent; when > both are set, each is checked on the paths it covers. @@ -215,6 +216,11 @@ requirement is dropped (issue #261, else the operator is 403'd out of their own membership never does either. So a **PIN-only** server-mode deployment keeps admin loopback-only; remote admin requires the long API key. +Two host-path capabilities are never remote: `/system/set-env` and +`PUT /api/settings/storage/models-dir`. They can select executable or writable +filesystem paths, so only a genuine loopback desktop caller may use them; +server mode and an API key do not weaken that boundary. + This is the fix for a real escalation (#1213): before it, server mode made the admin gate a no-op, so with an API key set *and* a trusted CIDR configured, a LAN client in that CIDR could `POST /system/set-env` — RCE-class — with **no diff --git a/tests/test_api.py b/tests/test_api.py index b629140f..6cd9fa95 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -624,6 +624,31 @@ def test_server_mode_remote_without_api_key_cannot_set_executable_path(monkeypat os.environ["FFMPEG_PATH"] = original +def test_server_mode_remote_api_key_cannot_set_executable_path(monkeypatch, tmp_path): + """An admin key does not grant the desktop file-picker capability.""" + from fastapi.testclient import TestClient + from main import app + + executable = tmp_path / "ffmpeg" + executable.write_bytes(b"not actually executable") + original = os.environ.get("FFMPEG_PATH") + monkeypatch.setenv("OMNIVOICE_SERVER_MODE", "1") + monkeypatch.setenv("OMNIVOICE_API_KEY", "s3cret") + try: + response = TestClient(app, client=("172.17.0.1", 50000)).post( + "/system/set-env", + headers={"authorization": "Bearer s3cret"}, + json={"key": "FFMPEG_PATH", "value": str(executable)}, + ) + assert response.status_code == 403 + assert os.environ.get("FFMPEG_PATH") == original + finally: + if original is None: + os.environ.pop("FFMPEG_PATH", None) + else: + os.environ["FFMPEG_PATH"] = original + + def test_set_env_loopback_still_validates_allowlist(): """Even on the loopback path, keys outside the allow-list must return 400 — the new guard must NOT bypass the existing allow-list enforcement.""" diff --git a/tests/test_loopback_server_mode.py b/tests/test_loopback_server_mode.py index a4b70877..7ad733c8 100644 --- a/tests/test_loopback_server_mode.py +++ b/tests/test_loopback_server_mode.py @@ -14,6 +14,7 @@ from api.dependencies import ( is_loopback, is_local_host, require_admin, + require_desktop, require_local, require_loopback, ) @@ -258,6 +259,13 @@ 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): + monkeypatch.setenv("OMNIVOICE_SERVER_MODE", "1") + monkeypatch.delenv("OMNIVOICE_API_KEY", raising=False) + monkeypatch.setenv("OMNIVOICE_SHARE_PIN", "123456") + require_admin(_req_full("172.17.0.1", method="GET")) + + def test_server_mode_admin_mutation_allows_api_key(monkeypatch): monkeypatch.setenv("OMNIVOICE_SERVER_MODE", "1") monkeypatch.setenv("OMNIVOICE_API_KEY", "s3cret") @@ -268,6 +276,18 @@ def test_server_mode_admin_mutation_allows_api_key(monkeypatch): )) +def test_server_mode_desktop_capability_rejects_remote_api_key(monkeypatch): + monkeypatch.setenv("OMNIVOICE_SERVER_MODE", "1") + monkeypatch.setenv("OMNIVOICE_API_KEY", "s3cret") + with pytest.raises(HTTPException) as exc: + require_desktop(_req_full( + "172.17.0.1", + method="POST", + headers={"authorization": "Bearer s3cret"}, + )) + assert exc.value.status_code == 403 + + @pytest.mark.parametrize("method", ["GET", "POST", "PUT", "DELETE"]) def test_loopback_admin_never_needs_api_key(monkeypatch, method): monkeypatch.setenv("OMNIVOICE_SERVER_MODE", "1") diff --git a/tests/test_models_dir_setting.py b/tests/test_models_dir_setting.py index e825bfd1..60b94328 100644 --- a/tests/test_models_dir_setting.py +++ b/tests/test_models_dir_setting.py @@ -76,6 +76,24 @@ def test_server_mode_remote_without_api_key_cannot_create_models_dir(env, monkey assert not target.exists() +def test_server_mode_remote_api_key_cannot_create_models_dir(env, monkeypatch, tmp_path): + """An admin key does not grant the desktop file-picker capability.""" + from fastapi.testclient import TestClient + + monkeypatch.setenv("OMNIVOICE_SERVER_MODE", "1") + monkeypatch.setenv("OMNIVOICE_API_KEY", "s3cret") + target = tmp_path / "must-not-exist" + app = fastapi.FastAPI() + app.include_router(s.router) + response = TestClient(app, client=("172.17.0.1", 50000)).put( + "/api/settings/storage/models-dir", + headers={"authorization": "Bearer s3cret"}, + json={"path": str(target)}, + ) + assert response.status_code == 403 + assert not target.exists() + + def test_clear_reverts_to_default(env): user_env.set_user_env("OMNIVOICE_CACHE_DIR", "/old") res = s.set_models_dir(s._ModelsDirBody(path="")) From 3e6679c03e7c370b3c7b8cb626b4ba17e22707d4 Mon Sep 17 00:00:00 2001 From: debpalash <4178343+debpalash@users.noreply.github.com> Date: Sun, 9 Aug 2026 21:16:55 +0000 Subject: [PATCH 04/22] fix(security): enforce filesystem trust boundaries --- backend/api/dependencies.py | 15 +++ backend/api/routers/dub_export.py | 195 ++++++++++++++++++--------- backend/api/routers/marketplace.py | 65 ++++++--- backend/api/routers/profiles.py | 18 ++- backend/api/routers/sonitranslate.py | 5 +- backend/api/routers/tools.py | 26 +++- backend/core/path_security.py | 63 +++++++++ backend/services/sonitranslate.py | 14 +- tests/test_filesystem_boundaries.py | 150 +++++++++++++++++++++ 9 files changed, 455 insertions(+), 96 deletions(-) create mode 100644 backend/core/path_security.py create mode 100644 tests/test_filesystem_boundaries.py diff --git a/backend/api/dependencies.py b/backend/api/dependencies.py index 83bf17a5..52287885 100644 --- a/backend/api/dependencies.py +++ b/backend/api/dependencies.py @@ -7,6 +7,8 @@ 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`). +- `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 client carries the remote API key (Wave 2.3) — used by WS endpoints that keep their own inline loopback guards. @@ -202,6 +204,19 @@ def require_local(request: Request) -> None: raise HTTPException(status_code=403, detail="loopback origin required") +def require_native_access(request: Request) -> None: + """Protect capabilities that read or write operator-chosen host paths. + + Docker server mode deliberately relaxes the ordinary admin gate because a + bridge makes even local traffic appear remote. That exception is unsafe for + native file pickers: a remote API caller must never probe or overwrite an + arbitrary path on the backend host, even with the server API key. + """ + host = request.client.host if request.client else None + if not is_loopback(host): + 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.""" diff --git a/backend/api/routers/dub_export.py b/backend/api/routers/dub_export.py index 6f5f253e..c0bb1fbc 100644 --- a/backend/api/routers/dub_export.py +++ b/backend/api/routers/dub_export.py @@ -7,13 +7,15 @@ import uuid import asyncio import logging from typing import Optional -from fastapi import APIRouter, HTTPException, Query, Response +from fastapi import APIRouter, HTTPException, Query, Request, Response from fastapi.responses import FileResponse, StreamingResponse from core.config import DUB_DIR, dub_seg_path from core.tasks import task_manager from core.http_headers import content_disposition from api.routers.dub_core import _get_job +from api.dependencies import require_native_access +from core.path_security import UnsafePath, resolve_within from services.ffmpeg_utils import ( bed_mix_filter, explain_ffmpeg_failure, @@ -40,6 +42,47 @@ def _unique_stamp() -> str: _SAFE_LANG = re.compile(r"^[A-Za-z0-9_-]{1,32}$") +def _job_dir_or_400(job_id: str) -> str: + if not re.fullmatch(r"[A-Za-z0-9_-]{1,64}", job_id or ""): + raise HTTPException(status_code=400, detail="Invalid job id") + try: + return str(resolve_within(DUB_DIR, job_id)) + except UnsafePath as exc: + raise HTTPException(status_code=400, detail="Invalid job id") from exc + + +def _dub_artifact(value: object, *, missing_detail: str = "File not found") -> str: + """Resolve a persisted job artifact inside the global dub-data boundary.""" + try: + path = resolve_within(DUB_DIR, str(value or "")) + except UnsafePath as exc: + raise HTTPException(status_code=400, detail="Invalid job artifact path") from exc + if not path.is_file(): + raise HTTPException(status_code=404, detail=missing_detail) + return str(path) + + +def _optional_dub_artifact(value: object) -> str | None: + if not value: + return None + try: + path = resolve_within(DUB_DIR, str(value)) + except UnsafePath as exc: + raise HTTPException(status_code=400, detail="Invalid job artifact path") from exc + return str(path) if path.is_file() else None + + +def _safe_lang_or_400(lang: str | None) -> str | None: + if lang is not None and not _SAFE_LANG.fullmatch(lang): + raise HTTPException(status_code=400, detail="Invalid language code") + return lang + + +def _guard_native_save(request: Request, save_path: str) -> None: + if save_path: + require_native_access(request) + + def _native_save(source: str, destination: str, display_name: str, media_type: str): """Copy a generated export file to a user-chosen destination and return JSON.""" import shutil @@ -429,6 +472,7 @@ def _build_audio_export_cmd( @router.get("/dub/download/{job_id}/{filename}") async def dub_download( job_id: str, + request: Request, preserve_bg: bool = Query(True, description="Mix background noise into dubbed tracks"), default_track: str = Query("original"), include_tracks: str = Query("", description="Comma-separated list of tracks to include (e.g. 'original,de,es'). Empty = include all."), @@ -440,8 +484,8 @@ async def dub_download( # Strict allowlist on the path param BEFORE it reaches any filesystem # path or ffmpeg argv (export dir, retime work path, slice paths). Real # job ids are short uuid slices — alnum/hyphen/underscore only. - if not re.fullmatch(r"[A-Za-z0-9_-]{1,64}", job_id): - raise HTTPException(status_code=400, detail="Invalid job id") + job_dir = _job_dir_or_400(job_id) + _guard_native_save(request, save_path) job = _get_job(job_id) if not job: raise HTTPException(status_code=404, detail="Job not found") @@ -458,12 +502,20 @@ async def dub_download( else: filtered_tracks = dict(tracks) + filtered_tracks = { + key: { + **value, + "path": _dub_artifact(value.get("path"), missing_detail="Dubbed track not found"), + } + for key, value in filtered_tracks.items() + } + if not filtered_tracks and not include_original: raise HTTPException(status_code=400, detail="No tracks selected for export") - video_path = job["video_path"] + video_path = _dub_artifact(job["video_path"], missing_detail="Source video not found") stamp = _unique_stamp() - exports_dir = os.path.join(DUB_DIR, job_id, "exports") + exports_dir = os.path.join(job_dir, "exports") os.makedirs(exports_dir, exist_ok=True) output_path = os.path.join(exports_dir, f"dubbed_video_{stamp}.mp4") ffmpeg = find_ffmpeg() @@ -488,8 +540,7 @@ async def dub_download( # safe_name below). safe_lang = "".join(c for c in lang_code if c.isalnum() or c in "-_") or "track" out_path = os.path.join(exports_dir, f"dubbed_audio_{safe_lang}_{stamp}.{fmt}") - bg = job.get("no_vocals_path") if preserve_bg else None - bg = bg if (bg and os.path.exists(bg)) else None + bg = _optional_dub_artifact(job.get("no_vocals_path")) if preserve_bg else None cmd = _build_audio_export_cmd(ffmpeg, track_info["path"], bg, out_path, fmt) try: rc, _, stderr = await run_ffmpeg(cmd, timeout=1800.0) @@ -612,9 +663,9 @@ async def dub_download( retimed_idx = input_idx input_idx += 1 - bg_audio = job.get("no_vocals_path") if preserve_bg else None + bg_audio = _optional_dub_artifact(job.get("no_vocals_path")) if preserve_bg else None bg_idx = None - if bg_audio and os.path.exists(bg_audio) and filtered_tracks: + if bg_audio and filtered_tracks: cmd += ["-i", bg_audio] bg_idx = input_idx input_idx += 1 @@ -819,12 +870,11 @@ _MEDIA_TYPES = { @router.get("/dub/media/{job_id}") async def dub_get_media(job_id: str): + _job_dir_or_400(job_id) job = _get_job(job_id) if not job: raise HTTPException(status_code=404, detail="Job not found") - video_path = job["video_path"] - if not os.path.exists(video_path): - raise HTTPException(status_code=404, detail="Media file not found") + video_path = _dub_artifact(job["video_path"], missing_detail="Media file not found") # Pass an explicit media_type. Without this Starlette falls back to # mimetypes.guess_type, which on some platforms returns the wrong # MIME (e.g. "application/octet-stream" for .mkv), and the Tauri @@ -863,8 +913,8 @@ async def dub_preview_video( # Strict allowlist on the path param BEFORE it reaches any filesystem # path or ffmpeg argv (exports dir, preview/retime work paths) — same # boundary check as dub_download. - if not re.fullmatch(r"[A-Za-z0-9_-]{1,64}", job_id): - raise HTTPException(status_code=400, detail="Invalid job id") + job_dir = _job_dir_or_400(job_id) + lang = _safe_lang_or_400(lang) job = _get_job(job_id) if not job: raise HTTPException(status_code=404, detail="Job not found") @@ -874,25 +924,19 @@ async def dub_preview_video( if not track_info: raise HTTPException(status_code=404, detail=f"No dubbed track for lang={lang}") - track_path = track_info.get("path") - if not track_path or not os.path.exists(track_path): - raise HTTPException(status_code=404, detail="Dubbed track file missing") + track_path = _dub_artifact(track_info.get("path"), missing_detail="Dubbed track file missing") - video_path = job.get("video_path") - if not video_path or not os.path.exists(video_path): - raise HTTPException(status_code=404, detail="Source video missing") + video_path = _dub_artifact(job.get("video_path"), missing_detail="Source video missing") - bg_audio = job.get("no_vocals_path") if preserve_bg else None - has_bg = bool(bg_audio and os.path.exists(bg_audio)) + bg_audio = _optional_dub_artifact(job.get("no_vocals_path")) if preserve_bg else None + has_bg = bool(bg_audio) - if not _SAFE_LANG.match(lang): - raise HTTPException(status_code=400, detail="Invalid lang") # realpath-normalised + containment-checked inline BEFORE any filesystem # access so the guard dominates every sink (the file's established # pattern — see dub_preview_segment; CodeQL does not track the guard # through a helper's return value). _base = os.path.realpath(DUB_DIR) - exports_dir = os.path.realpath(os.path.join(_base, job_id, "exports")) + exports_dir = os.path.realpath(os.path.join(job_dir, "exports")) if not exports_dir.startswith(_base + os.sep): raise HTTPException(status_code=400, detail="Invalid job id") os.makedirs(exports_dir, exist_ok=True) @@ -1113,15 +1157,16 @@ async def dub_get_onsets(job_id: str): newer than the cache (e.g. re-ingest into the same job dir). """ import json + job_dir = _job_dir_or_400(job_id) job = _get_job(job_id) if not job: raise HTTPException(status_code=404, detail="Job not found") - vocals = job.get("vocals_path") - mix = job.get("audio_path") - if vocals and os.path.exists(vocals): + vocals = _optional_dub_artifact(job.get("vocals_path")) + mix = _optional_dub_artifact(job.get("audio_path")) + if vocals: src_path, source = vocals, "vocals" - elif mix and os.path.exists(mix): + elif mix: src_path, source = mix, "mix" else: raise HTTPException(status_code=404, detail="No audio track available for onset analysis") @@ -1129,7 +1174,7 @@ async def dub_get_onsets(job_id: str): # Containment inlined (not via _safe_job_path): CodeQL can't track the # sanitizer through a helper's return — the file's established idiom. base = os.path.realpath(DUB_DIR) - cache_path = os.path.realpath(os.path.join(base, job_id, "onsets.json")) + cache_path = os.path.realpath(os.path.join(job_dir, "onsets.json")) if not cache_path.startswith(base + os.sep): raise HTTPException(status_code=400, detail="Invalid job id") try: @@ -1167,23 +1212,23 @@ async def dub_get_onsets(job_id: str): @router.get("/dub/thumb/{job_id}") async def dub_get_thumb(job_id: str): """Serve the extracted dub video thumbnail (jpg). 404 if not generated.""" + job_dir = _job_dir_or_400(job_id) job = _get_job(job_id) if not job: raise HTTPException(status_code=404, detail="Job not found") # Resolve under DUB_DIR to prevent traversal. - thumb = os.path.join(DUB_DIR, job_id, "thumb.jpg") + thumb = os.path.join(job_dir, "thumb.jpg") if not os.path.exists(thumb): raise HTTPException(status_code=404, detail="Thumbnail not available") return FileResponse(thumb, media_type="image/jpeg", headers={"Cache-Control": "public, max-age=3600"}) @router.get("/dub/audio/{job_id}") async def dub_get_audio(job_id: str): + _job_dir_or_400(job_id) job = _get_job(job_id) if not job: raise HTTPException(status_code=404, detail="Job not found") - audio = job.get("audio_path") - if not audio or not os.path.exists(audio): - raise HTTPException(status_code=404, detail="Audio file not found") + audio = _dub_artifact(job.get("audio_path"), missing_detail="Audio file not found") return FileResponse(audio, media_type="audio/wav") def _seg_wav_candidates(job: dict, lang: "str | None", seg_keys: tuple) -> list: @@ -1207,6 +1252,8 @@ def _seg_wav_candidates(job: dict, lang: "str | None", seg_keys: tuple) -> list: @router.get("/dub/preview/{job_id}/{segment_index}") async def dub_preview_segment(job_id: str, segment_index: int, lang: str = Query(None)): + _job_dir_or_400(job_id) + lang = _safe_lang_or_400(lang) job = _get_job(job_id) if not job: raise HTTPException(status_code=404, detail="Job not found") @@ -1243,19 +1290,18 @@ async def dub_qc_pass(job_id: str, lang: str = Query(None), drift_threshold: flo from services import dub_qc from services.dub_pipeline import put_job, save_job + _job_dir_or_400(job_id) + lang = _safe_lang_or_400(lang) job = _get_job(job_id) if not job: raise HTTPException(status_code=404, detail="Job not found") tracks = job.get("dubbed_tracks", {}) if lang and lang in tracks: - wav_path = tracks[lang]["path"] + wav_path = _dub_artifact(tracks[lang].get("path"), missing_detail="Dubbed audio file not found") elif tracks: - wav_path = list(tracks.values())[0]["path"] + wav_path = _dub_artifact(list(tracks.values())[0].get("path"), missing_detail="Dubbed audio file not found") else: raise HTTPException(status_code=400, detail="No dubbed audio track generated yet") - if not os.path.exists(wav_path): - raise HTTPException(status_code=404, detail="Dubbed audio file not found") - segments = job.get("segments") or [] if not segments: raise HTTPException(status_code=400, detail="Job has no segments") @@ -1335,29 +1381,36 @@ async def dub_qc_pass(job_id: str, lang: str = Query(None), drift_threshold: flo @router.get("/dub/download-audio/{job_id}") @router.get("/dub/download-audio/{job_id}/{filename}") -async def dub_download_audio(job_id: str, lang: str = Query(None), preserve_bg: bool = Query(True), save_path: str = Query("")): +async def dub_download_audio( + job_id: str, + request: Request, + lang: str = Query(None), + preserve_bg: bool = Query(True), + save_path: str = Query(""), +): + job_dir = _job_dir_or_400(job_id) + _guard_native_save(request, save_path) + lang = _safe_lang_or_400(lang) job = _get_job(job_id) if not job: raise HTTPException(status_code=404, detail="Job not found") tracks = job.get("dubbed_tracks", {}) if lang and lang in tracks: - wav_path = tracks[lang]["path"] + wav_path = _dub_artifact(tracks[lang].get("path"), missing_detail="Audio file not found") elif tracks: - wav_path = list(tracks.values())[0]["path"] + wav_path = _dub_artifact(list(tracks.values())[0].get("path"), missing_detail="Audio file not found") else: raise HTTPException(status_code=400, detail="No dubbed audio track generated yet") - if not os.path.exists(wav_path): - raise HTTPException(status_code=404, detail="Audio file not found") - lang_label = lang or list(tracks.keys())[0] + _safe_lang_or_400(lang_label) stamp = _unique_stamp() - exports_dir = os.path.join(DUB_DIR, job_id, "exports") + exports_dir = os.path.join(job_dir, "exports") os.makedirs(exports_dir, exist_ok=True) - bg_audio = job.get("no_vocals_path") if preserve_bg else None - if bg_audio and os.path.exists(bg_audio): + bg_audio = _optional_dub_artifact(job.get("no_vocals_path")) if preserve_bg else None + if bg_audio: ffmpeg = find_ffmpeg() final_audio_path = os.path.join(exports_dir, f"mixed_dub_{lang_label}_{stamp}.wav") cmd = [ @@ -1435,6 +1488,8 @@ async def dub_export_srt( dual: bool = False, lang: str = Query(None, description="Track language code. Emits that track's text (segments_i18n) when the job carries it; when that track was generated under Smart Fit or stretch_video, cue times come from the fitted timeline."), ): + _job_dir_or_400(job_id) + lang = _safe_lang_or_400(lang) job = _get_job(job_id) if not job: raise HTTPException(status_code=404, detail="Job not found") @@ -1487,6 +1542,8 @@ async def dub_export_vtt( dual: bool = False, lang: str = Query(None, description="Track language code. Emits that track's text (segments_i18n) when the job carries it; when that track was generated under Smart Fit or stretch_video, cue times come from the fitted timeline."), ): + _job_dir_or_400(job_id) + lang = _safe_lang_or_400(lang) job = _get_job(job_id) if not job: raise HTTPException(status_code=404, detail="Job not found") @@ -1524,6 +1581,8 @@ async def dub_export_vtt( @router.get("/dub/export-segments/{job_id}") async def dub_export_segments_zip(job_id: str, lang: str = Query(None)): import zipfile + _job_dir_or_400(job_id) + lang = _safe_lang_or_400(lang) job = _get_job(job_id) if not job: raise HTTPException(status_code=404, detail="Job not found") @@ -1563,31 +1622,39 @@ async def dub_export_segments_zip(job_id: str, lang: str = Query(None)): @router.get("/dub/download-mp3/{job_id}") @router.get("/dub/download-mp3/{job_id}/{filename}") -async def dub_download_mp3(job_id: str, lang: str = Query(None), preserve_bg: bool = Query(True), save_path: str = Query(""), bitrate: str = Query("192k")): +async def dub_download_mp3( + job_id: str, + request: Request, + lang: str = Query(None), + preserve_bg: bool = Query(True), + save_path: str = Query(""), + bitrate: str = Query("192k"), +): + job_dir = _job_dir_or_400(job_id) + _guard_native_save(request, save_path) + lang = _safe_lang_or_400(lang) job = _get_job(job_id) if not job: raise HTTPException(status_code=404, detail="Job not found") tracks = job.get("dubbed_tracks", {}) if lang and lang in tracks: - wav_path = tracks[lang]["path"] + wav_path = _dub_artifact(tracks[lang].get("path"), missing_detail="Audio file not found") elif tracks: - wav_path = list(tracks.values())[0]["path"] + wav_path = _dub_artifact(list(tracks.values())[0].get("path"), missing_detail="Audio file not found") else: raise HTTPException(status_code=400, detail="No dubbed audio track generated yet") - if not os.path.exists(wav_path): - raise HTTPException(status_code=404, detail="Audio file not found") - lang_label = lang or list(tracks.keys())[0] + _safe_lang_or_400(lang_label) ffmpeg = find_ffmpeg() stamp = _unique_stamp() - exports_dir = os.path.join(DUB_DIR, job_id, "exports") + exports_dir = os.path.join(job_dir, "exports") os.makedirs(exports_dir, exist_ok=True) source_path = wav_path - bg_audio = job.get("no_vocals_path") if preserve_bg else None - if bg_audio and os.path.exists(bg_audio): + bg_audio = _optional_dub_artifact(job.get("no_vocals_path")) if preserve_bg else None + if bg_audio: mixed_path = os.path.join(exports_dir, f"mixed_mp3_{lang_label}_{stamp}.wav") cmd_mix = [ ffmpeg, "-i", bg_audio, "-i", wav_path, @@ -1644,6 +1711,8 @@ async def dub_download_mp3(job_id: str, lang: str = Query(None), preserve_bg: bo @router.get("/dub/export-stems/{job_id}") async def dub_export_stems(job_id: str, lang: str = Query(None)): import zipfile + _job_dir_or_400(job_id) + lang = _safe_lang_or_400(lang) job = _get_job(job_id) if not job: raise HTTPException(status_code=404, detail="Job not found") @@ -1653,22 +1722,22 @@ async def dub_export_stems(job_id: str, lang: str = Query(None)): raise HTTPException(status_code=400, detail="No dubbed tracks generated yet") if lang and lang in tracks: - vocals_path = tracks[lang]["path"] + vocals_path = _dub_artifact(tracks[lang].get("path"), missing_detail="Dubbed audio file not found") lang_label = lang elif tracks: first_key = list(tracks.keys())[0] - vocals_path = tracks[first_key]["path"] + _safe_lang_or_400(first_key) + vocals_path = _dub_artifact(tracks[first_key].get("path"), missing_detail="Dubbed audio file not found") lang_label = first_key else: raise HTTPException(status_code=400, detail="No dubbed audio track") - bg_path = job.get("no_vocals_path") + bg_path = _optional_dub_artifact(job.get("no_vocals_path")) zip_buffer = io.BytesIO() with zipfile.ZipFile(zip_buffer, "w", zipfile.ZIP_DEFLATED) as zf: - if os.path.exists(vocals_path): - zf.write(vocals_path, f"vocals_dubbed_{lang_label}.wav") - if bg_path and os.path.exists(bg_path): + zf.write(vocals_path, f"vocals_dubbed_{lang_label}.wav") + if bg_path: zf.write(bg_path, "background_original.wav") zip_buffer.seek(0) diff --git a/backend/api/routers/marketplace.py b/backend/api/routers/marketplace.py index b99131da..f6846f14 100644 --- a/backend/api/routers/marketplace.py +++ b/backend/api/routers/marketplace.py @@ -40,6 +40,7 @@ from core.db import db_conn from core import event_bus from core.version import APP_VERSION from core.http_headers import content_disposition +from core.path_security import UnsafePath, resolve_within, safe_filename logger = logging.getLogger("omnivoice.marketplace") @@ -56,6 +57,24 @@ BUNDLE_VERSION = 1 MAX_BUNDLE_BYTES = 100 * 1024 * 1024 +def _contained_path(root, value, *, detail="Invalid file path") -> Path: + try: + return resolve_within(root, value) + except UnsafePath as exc: + raise HTTPException(status_code=400, detail=detail) from exc + + +def _voice_asset(value) -> Path | None: + """Resolve a DB-stored voice asset without trusting the database value.""" + if not value: + return None + try: + return resolve_within(VOICES_DIR, value) + except UnsafePath: + logger.warning("Ignoring voice asset outside the voices directory") + return None + + # ── Export ────────────────────────────────────────────────────────────────── @@ -109,18 +128,18 @@ def export_profile(profile_id: str): # Reference audio ref_path = profile.get("ref_audio_path") if ref_path: - full_ref = os.path.join(VOICES_DIR, ref_path) - if os.path.isfile(full_ref): + full_ref = _voice_asset(ref_path) + if full_ref and full_ref.is_file(): ext = os.path.splitext(ref_path)[1] or ".wav" - zf.write(full_ref, f"ref_audio{ext}") + zf.write(str(full_ref), f"ref_audio{ext}") # Locked audio (if profile is locked) locked_path = profile.get("locked_audio_path") if locked_path: - full_locked = os.path.join(VOICES_DIR, locked_path) - if os.path.isfile(full_locked): + full_locked = _voice_asset(locked_path) + if full_locked and full_locked.is_file(): ext = os.path.splitext(locked_path)[1] or ".wav" - zf.write(full_locked, f"locked_audio{ext}") + zf.write(str(full_locked), f"locked_audio{ext}") buf.seek(0) safe_name = "".join( @@ -270,7 +289,11 @@ def publish_to_marketplace( safe_name = "".join( c if c.isalnum() or c in "-_ " else "" for c in profile.get("name", "voice") ).strip().replace(" ", "_")[:40] - bundle_path = MARKETPLACE_DIR / f"{safe_name}_{profile_id}.omnivoice" + bundle_path = _contained_path( + MARKETPLACE_DIR, + f"{safe_name}_{profile_id}.omnivoice", + detail="Invalid profile id", + ) # Build the bundle with zipfile.ZipFile(str(bundle_path), "w", zipfile.ZIP_DEFLATED) as zf: @@ -283,17 +306,17 @@ def publish_to_marketplace( ref_path = profile.get("ref_audio_path") if ref_path: - full_ref = os.path.join(VOICES_DIR, ref_path) - if os.path.isfile(full_ref): + full_ref = _voice_asset(ref_path) + if full_ref and full_ref.is_file(): ext = os.path.splitext(ref_path)[1] or ".wav" - zf.write(full_ref, f"ref_audio{ext}") + zf.write(str(full_ref), f"ref_audio{ext}") locked_path = profile.get("locked_audio_path") if locked_path: - full_locked = os.path.join(VOICES_DIR, locked_path) - if os.path.isfile(full_locked): + full_locked = _voice_asset(locked_path) + if full_locked and full_locked.is_file(): ext = os.path.splitext(locked_path)[1] or ".wav" - zf.write(full_locked, f"locked_audio{ext}") + zf.write(str(full_locked), f"locked_audio{ext}") logger.info("Published voice %r to marketplace: %s", profile.get("name"), bundle_path) return { @@ -353,7 +376,13 @@ def browse_marketplace( @router.post("/install/{filename}") async def install_from_marketplace(filename: str): """Import a voice profile from a bundle in the local marketplace directory.""" - bundle_path = MARKETPLACE_DIR / filename + try: + filename = safe_filename(filename) + except UnsafePath as exc: + raise HTTPException(status_code=400, detail="Invalid bundle filename") from exc + if not filename.endswith(".omnivoice"): + raise HTTPException(status_code=400, detail="Invalid bundle filename") + bundle_path = _contained_path(MARKETPLACE_DIR, filename, detail="Invalid bundle filename") if not bundle_path.is_file(): raise HTTPException(status_code=404, detail=f"Bundle not found: {filename}") @@ -426,7 +455,13 @@ async def install_from_marketplace(filename: str): @router.delete("/{filename}") def remove_from_marketplace(filename: str): """Remove a bundle from the local marketplace directory.""" - bundle_path = MARKETPLACE_DIR / filename + try: + filename = safe_filename(filename) + except UnsafePath as exc: + raise HTTPException(status_code=400, detail="Invalid bundle filename") from exc + if not filename.endswith(".omnivoice"): + raise HTTPException(status_code=400, detail="Invalid bundle filename") + bundle_path = _contained_path(MARKETPLACE_DIR, filename, detail="Invalid bundle filename") if not bundle_path.is_file(): raise HTTPException(status_code=404, detail=f"Bundle not found: {filename}") try: diff --git a/backend/api/routers/profiles.py b/backend/api/routers/profiles.py index b27b8f57..af563a45 100644 --- a/backend/api/routers/profiles.py +++ b/backend/api/routers/profiles.py @@ -13,6 +13,7 @@ from core.config import VOICES_DIR, OUTPUTS_DIR from core import event_bus from core.personalities import get_personalities from omnivoice.utils.voice_design import heal_design_instruct, sanitize_instruct +from core.path_security import UnsafePath, resolve_within router = APIRouter() @@ -377,13 +378,18 @@ async def lock_profile( if not history or not history["audio_path"]: raise HTTPException(status_code=404, detail="History item not found or has no audio") - src_path = os.path.join(OUTPUTS_DIR, history["audio_path"]) - if not os.path.exists(src_path): + try: + src_path = resolve_within(OUTPUTS_DIR, history["audio_path"]) + except UnsafePath as exc: + raise HTTPException(status_code=400, detail="Invalid history audio path") from exc + if not src_path.is_file(): raise HTTPException(status_code=404, detail="Audio file not found on disk") locked_filename = f"{profile_id}_locked.wav" - locked_path = os.path.join(VOICES_DIR, locked_filename) - shutil.copy2(src_path, locked_path) + locked_path = _voices_path(locked_filename) + if locked_path is None: + raise HTTPException(status_code=400, detail="Invalid profile id") + shutil.copy2(str(src_path), locked_path) ref_text = history["text"][:100] if history["text"] else "" @@ -405,8 +411,8 @@ async def unlock_profile(profile_id: str): ) if profile["locked_audio_path"]: - locked_path = os.path.join(VOICES_DIR, profile["locked_audio_path"]) - if os.path.exists(locked_path): + locked_path = _voices_path(profile["locked_audio_path"]) + if locked_path and os.path.exists(locked_path): os.remove(locked_path) conn.execute( diff --git a/backend/api/routers/sonitranslate.py b/backend/api/routers/sonitranslate.py index 658490a8..c48a8fd3 100644 --- a/backend/api/routers/sonitranslate.py +++ b/backend/api/routers/sonitranslate.py @@ -5,11 +5,12 @@ SoniTranslate sidecar integration. """ import logging -from fastapi import APIRouter, HTTPException +from fastapi import APIRouter, Depends, HTTPException from pydantic import BaseModel from typing import Optional from services import sonitranslate as soni +from api.dependencies import require_native_access router = APIRouter(prefix="/engines/sonitranslate", tags=["SoniTranslate"]) logger = logging.getLogger("omnivoice.api") @@ -71,7 +72,7 @@ class DubRequest(BaseModel): output_dir: Optional[str] = None -@router.post("/dub") +@router.post("/dub", dependencies=[Depends(require_native_access)]) async def sonitranslate_dub(body: DubRequest): """Run full dubbing pipeline via SoniTranslate. diff --git a/backend/api/routers/tools.py b/backend/api/routers/tools.py index 70001957..b410ac81 100644 --- a/backend/api/routers/tools.py +++ b/backend/api/routers/tools.py @@ -21,13 +21,16 @@ import asyncio import json import logging import os +import re from typing import Optional -from fastapi import APIRouter, HTTPException +from fastapi import APIRouter, Depends, HTTPException from pydantic import BaseModel, Field from services import director, speech_rate, incremental from services.ffmpeg_utils import find_ffprobe, spawn_subprocess +from api.dependencies import require_native_access +from core.path_security import UnsafePath, resolve_within logger = logging.getLogger("omnivoice.tools") router = APIRouter() @@ -40,7 +43,7 @@ class ProbeReq(BaseModel): path: str -@router.post("/tools/probe") +@router.post("/tools/probe", dependencies=[Depends(require_native_access)]) async def probe(req: ProbeReq): target = os.path.realpath(os.path.expanduser(req.path)) if not os.path.exists(target): @@ -174,18 +177,27 @@ async def analyse_video_context(job_id: str): from core.config import DUB_DIR from services.video_context import analyse_video + if not re.fullmatch(r"[A-Za-z0-9_-]{1,64}", job_id or ""): + raise HTTPException(status_code=400, detail="Invalid job id") + try: + job_dir = resolve_within(DUB_DIR, job_id) + except UnsafePath as exc: + raise HTTPException(status_code=400, detail="Invalid job id") from exc job = _get_job(job_id) if not job: from fastapi import HTTPException raise HTTPException(status_code=404, detail="Job not found") - video_path = os.path.join(DUB_DIR, job_id, "source.mp4") - if not os.path.exists(video_path): - video_path = job.get("video_path", "") + video_path = resolve_within(DUB_DIR, job_dir / "source.mp4") + if not video_path.is_file(): + try: + video_path = resolve_within(DUB_DIR, job.get("video_path", "")) + except UnsafePath: + return {"error": "Source video not found", "segments": {}} - if not video_path or not os.path.exists(video_path): + if not video_path.is_file(): return {"error": "Source video not found", "segments": {}} segments = job.get("segments") or [] - ctx = await analyse_video(video_path, segments) + ctx = await analyse_video(str(video_path), segments) return ctx.to_dict() diff --git a/backend/core/path_security.py b/backend/core/path_security.py new file mode 100644 index 00000000..8e349d01 --- /dev/null +++ b/backend/core/path_security.py @@ -0,0 +1,63 @@ +"""Filesystem trust-boundary helpers. + +Paths persisted in SQLite are still untrusted: older clients and imported job +records can contain absolute paths, traversal components, or symlink escapes. +Keep containment checks at the filesystem boundary instead of relying on the +route or database layer to have sanitised a value earlier. +""" + +from __future__ import annotations + +import ntpath +import os +from pathlib import Path + + +class UnsafePath(ValueError): + """Raised when a path crosses its allowed filesystem boundary.""" + + +def safe_filename(value: object) -> str: + """Return a portable bare filename, rejecting traversal and drive paths.""" + name = str(value or "") + if ( + not name + or name in {".", ".."} + or "/" in name + or "\\" in name + or os.path.isabs(name) + or ntpath.isabs(name) + or ntpath.basename(name) != name + ): + raise UnsafePath("expected a bare filename") + return name + + +def resolve_within(root: os.PathLike[str] | str, value: os.PathLike[str] | str) -> Path: + """Resolve *value* beneath *root*, rejecting traversal and symlink escapes. + + Absolute values are accepted only when they already resolve inside the + root. This preserves existing database rows, which historically stored a + mixture of relative filenames and absolute job-artifact paths. + """ + raw = os.fspath(value) if value is not None else "" + if not raw: + raise UnsafePath("path is empty") + # Treat both separator families as structural on every host. Otherwise a + # Windows traversal string is an innocent-looking filename when validated + # on Linux (and can become dangerous after persisted data is moved). + if os.sep != "\\" and ("\\" in raw or bool(ntpath.splitdrive(raw)[0])): + raise UnsafePath("path uses a foreign separator or drive") + root_path = Path(root).expanduser().resolve(strict=False) + candidate = Path(raw).expanduser() + if not candidate.is_absolute(): + candidate = root_path / candidate + resolved = candidate.resolve(strict=False) + try: + if os.path.commonpath((str(root_path), str(resolved))) != str(root_path): + raise UnsafePath("path escapes its allowed root") + except ValueError as exc: # Windows paths on different drives + raise UnsafePath("path escapes its allowed root") from exc + if resolved == root_path: + raise UnsafePath("path must name an item below its allowed root") + return resolved diff --git a/backend/services/sonitranslate.py b/backend/services/sonitranslate.py index 2c668d2d..04da9387 100644 --- a/backend/services/sonitranslate.py +++ b/backend/services/sonitranslate.py @@ -15,6 +15,7 @@ from pathlib import Path from typing import Optional from services.ffmpeg_utils import spawn_subprocess +from core.path_security import UnsafePath, resolve_within, safe_filename logger = logging.getLogger("omnivoice.sonitranslate") @@ -308,9 +309,16 @@ async def dub_video( output_file = result if output_file and output_dir: - dest = os.path.join(output_dir, os.path.basename(output_file)) - shutil.copy2(output_file, dest) - output_file = dest + output_root = Path(output_dir).expanduser() + if not output_root.is_absolute() or not output_root.is_dir(): + raise ValueError("output_dir must be an existing absolute directory") + try: + output_name = safe_filename(os.path.basename(output_file)) + dest = resolve_within(output_root, output_name) + except UnsafePath as exc: + raise ValueError("SoniTranslate returned an invalid output filename") from exc + shutil.copy2(output_file, str(dest)) + output_file = str(dest) logger.info("SoniTranslate dub complete: %s", output_file) return { diff --git a/tests/test_filesystem_boundaries.py b/tests/test_filesystem_boundaries.py new file mode 100644 index 00000000..1684927c --- /dev/null +++ b/tests/test_filesystem_boundaries.py @@ -0,0 +1,150 @@ +"""Regression tests for host-filesystem and persisted-path trust boundaries.""" + +from __future__ import annotations + +import asyncio +from contextlib import contextmanager +from types import SimpleNamespace + +import pytest +from fastapi import HTTPException + +from api.dependencies import require_native_access +from core.path_security import UnsafePath, resolve_within, safe_filename + + +def _request(host: str | None): + return SimpleNamespace(client=SimpleNamespace(host=host) if host else None) + + +@pytest.mark.parametrize("host", ["127.0.0.1", "::1", "localhost"]) +def test_native_filesystem_capabilities_allow_true_loopback(host): + require_native_access(_request(host)) + + +@pytest.mark.parametrize("host", ["172.17.0.1", "192.168.1.4", None]) +def test_native_filesystem_capabilities_reject_remote_even_in_server_mode(monkeypatch, host): + monkeypatch.setenv("OMNIVOICE_SERVER_MODE", "1") + monkeypatch.setenv("OMNIVOICE_API_KEY", "operator-secret") + with pytest.raises(HTTPException) as exc: + require_native_access(_request(host)) + assert exc.value.status_code == 403 + + +@pytest.mark.parametrize( + "name", + ["../secret.wav", "folder/voice.wav", r"folder\voice.wav", r"C:\secret.wav", ".", "..", ""], +) +def test_safe_filename_rejects_posix_and_windows_escapes(name): + with pytest.raises(UnsafePath): + safe_filename(name) + + +def test_resolve_within_accepts_relative_and_existing_absolute_paths(tmp_path): + root = tmp_path / "root" + root.mkdir() + item = root / "voice.wav" + assert resolve_within(root, "voice.wav") == item + assert resolve_within(root, item) == item + + +def test_resolve_within_rejects_parent_and_absolute_escape(tmp_path): + root = tmp_path / "root" + root.mkdir() + with pytest.raises(UnsafePath): + resolve_within(root, "../secret.wav") + with pytest.raises(UnsafePath): + resolve_within(root, tmp_path / "secret.wav") + with pytest.raises(UnsafePath): + resolve_within(root, r"..\secret.wav") + with pytest.raises(UnsafePath): + resolve_within(root, r"C:\secret.wav") + + +def test_resolve_within_rejects_symlink_escape(tmp_path): + root = tmp_path / "root" + outside = tmp_path / "outside" + root.mkdir() + outside.mkdir() + (root / "link").symlink_to(outside, target_is_directory=True) + with pytest.raises(UnsafePath): + resolve_within(root, "link/secret.wav") + + +def test_marketplace_filename_cannot_escape_store(tmp_path, monkeypatch): + from api.routers import marketplace + + monkeypatch.setattr(marketplace, "MARKETPLACE_DIR", tmp_path / "store") + (tmp_path / "store").mkdir() + with pytest.raises(HTTPException) as exc: + asyncio.run(marketplace.install_from_marketplace("../secret.omnivoice")) + assert exc.value.status_code == 400 + + +def test_marketplace_db_asset_cannot_escape_voices(tmp_path, monkeypatch): + from api.routers import marketplace + + voices = tmp_path / "voices" + voices.mkdir() + secret = tmp_path / "secret.wav" + secret.write_bytes(b"secret") + monkeypatch.setattr(marketplace, "VOICES_DIR", str(voices)) + assert marketplace._voice_asset(secret) is None + + +def test_profile_lock_rejects_history_path_outside_outputs(tmp_path, monkeypatch): + from api.routers import profiles + + outputs = tmp_path / "outputs" + voices = tmp_path / "voices" + outputs.mkdir() + voices.mkdir() + secret = tmp_path / "secret.wav" + secret.write_bytes(b"secret") + + class FakeConnection: + def execute(self, query, _params): + if "voice_profiles" in query: + return SimpleNamespace(fetchone=lambda: {"id": "profile"}) + return SimpleNamespace( + fetchone=lambda: {"audio_path": str(secret), "text": "private"} + ) + + @contextmanager + def fake_db_conn(): + yield FakeConnection() + + monkeypatch.setattr(profiles, "OUTPUTS_DIR", str(outputs)) + monkeypatch.setattr(profiles, "VOICES_DIR", str(voices)) + monkeypatch.setattr(profiles, "db_conn", fake_db_conn) + with pytest.raises(HTTPException) as exc: + asyncio.run(profiles.lock_profile("profile", history_id="history")) + assert exc.value.status_code == 400 + assert not any(voices.iterdir()) + + +def test_dub_artifact_rejects_db_path_and_symlink_escapes(tmp_path, monkeypatch): + from api.routers import dub_export + + root = tmp_path / "dub" + outside = tmp_path / "outside" + root.mkdir() + outside.mkdir() + (outside / "secret.wav").write_bytes(b"secret") + (root / "link").symlink_to(outside, target_is_directory=True) + monkeypatch.setattr(dub_export, "DUB_DIR", str(root)) + + for value in (outside / "secret.wav", root / "link" / "secret.wav"): + with pytest.raises(HTTPException) as exc: + dub_export._dub_artifact(value) + assert exc.value.status_code == 400 + + +def test_native_save_guard_runs_only_for_host_path_mode(monkeypatch): + from api.routers import dub_export + + remote = _request("10.0.0.8") + dub_export._guard_native_save(remote, "") + with pytest.raises(HTTPException) as exc: + dub_export._guard_native_save(remote, "/tmp/export.wav") + assert exc.value.status_code == 403 From c9c60d182f3edca43d9b88dead6938018de041f2 Mon Sep 17 00:00:00 2001 From: debpalash <4178343+debpalash@users.noreply.github.com> Date: Sun, 9 Aug 2026 21:17:20 +0000 Subject: [PATCH 05/22] docs(changelog): note filesystem boundary hardening --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 50771571..cb189e61 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,6 +41,7 @@ The bundled TTS model package (`pyproject.toml`) is versioned independently. ### Fixed +- Remote backends can no longer probe or overwrite arbitrary host files through native-only tools, and imported or persisted paths cannot escape their VoiceStudio data folders. (#1455) - Sidecar engines no longer break when a library they load prints to the console. Those bytes landed in the middle of the engine's data stream, failing the generation and leaving the connection scrambled for every request after it. (#1428) — thanks @1335-Group! - A generation abandoned while stuck on an internal lock now says so, instead of blaming your hardware and suggesting shorter text. Nothing had been computed, so none of that advice applied. (#1416, #1419) - A machine with a GPU that ends up on CPU now says why — a missing device node, a permissions problem, a card newer than the installed ROCm, an `HSA_OVERRIDE_GFX_VERSION` that is doing more harm than good, or an NVIDIA driver the container can't reach each read differently. Before, all of them looked identical to having no GPU at all. (#1274, #1228) From fa3ba2f366bb884e4bd1a76ece913703d23165c1 Mon Sep 17 00:00:00 2001 From: debpalash <4178343+debpalash@users.noreply.github.com> Date: Sun, 9 Aug 2026 21:23:00 +0000 Subject: [PATCH 06/22] fix(security): authorize host paths through native IPC --- CHANGELOG.md | 2 +- backend/api/routers/media_tools.py | 9 +- backend/api/routers/settings.py | 19 +-- backend/api/routers/system.py | 22 +--- backend/core/path_authorization.py | 67 +++++++++++ docs/api-auth.md | 14 ++- frontend/src-tauri/Cargo.lock | 1 + frontend/src-tauri/Cargo.toml | 1 + frontend/src-tauri/src/backend.rs | 4 + frontend/src-tauri/src/commands.rs | 108 +++++++++++++++++- frontend/src-tauri/src/lib.rs | 1 + .../components/settings/AudioToolsPanel.jsx | 22 +++- .../settings/AudioToolsPanel.test.jsx | 23 ++++ .../src/components/settings/StoragePanel.jsx | 6 +- .../components/settings/StoragePanel.test.jsx | 43 +++++++ tests/test_api.py | 6 +- tests/test_loopback_server_mode.py | 38 ++++-- tests/test_media_tools.py | 26 ++++- tests/test_models_dir_setting.py | 49 ++++++-- 19 files changed, 397 insertions(+), 64 deletions(-) create mode 100644 backend/core/path_authorization.py create mode 100644 frontend/src/components/settings/StoragePanel.test.jsx diff --git a/CHANGELOG.md b/CHANGELOG.md index a0efaf59..6e8118b3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,7 +41,7 @@ The bundled TTS model package (`pyproject.toml`) is versioned independently. ### Fixed -- Remote Docker clients can no longer change executable paths or filesystem settings unless they present the admin API key; local desktop behavior is unchanged. (#1448) +- Server-mode settings mutations require the admin API key, while host destinations and executable paths can only be selected through the native desktop app. (#1448) - Sidecar engines no longer break when a library they load prints to the console. Those bytes landed in the middle of the engine's data stream, failing the generation and leaving the connection scrambled for every request after it. (#1428) — thanks @1335-Group! - A generation abandoned while stuck on an internal lock now says so, instead of blaming your hardware and suggesting shorter text. Nothing had been computed, so none of that advice applied. (#1416, #1419) - A machine with a GPU that ends up on CPU now says why — a missing device node, a permissions problem, a card newer than the installed ROCm, an `HSA_OVERRIDE_GFX_VERSION` that is doing more harm than good, or an NVIDIA driver the container can't reach each read differently. Before, all of them looked identical to having no GPU at all. (#1274, #1228) diff --git a/backend/api/routers/media_tools.py b/backend/api/routers/media_tools.py index 95a93e38..770cd865 100644 --- a/backend/api/routers/media_tools.py +++ b/backend/api/routers/media_tools.py @@ -19,7 +19,7 @@ router = APIRouter(dependencies=[Depends(require_loopback)]) class CustomPathRequest(BaseModel): - path: str + authorization: str def _svc(): @@ -61,8 +61,13 @@ def media_tools_ytdlp_restore(): @router.post("/media-tools/{tool}/custom-path") def media_tools_custom_path(tool: str, body: CustomPathRequest): + from core.path_authorization import PathAuthorizationError, consume + try: - return _svc().set_custom_path(tool, body.path) + path = consume(body.authorization, tool) + return _svc().set_custom_path(tool, path) + except PathAuthorizationError as e: + raise HTTPException(status_code=403, detail=str(e)) from e except ValueError as e: raise HTTPException(status_code=400, detail=str(e)) diff --git a/backend/api/routers/settings.py b/backend/api/routers/settings.py index d15f1a94..be09628a 100644 --- a/backend/api/routers/settings.py +++ b/backend/api/routers/settings.py @@ -19,7 +19,7 @@ from dataclasses import asdict from fastapi import APIRouter, Depends, HTTPException, Query from pydantic import BaseModel, Field -from api.dependencies import require_admin, require_desktop +from api.dependencies import require_admin logger = logging.getLogger("omnivoice.api.settings") @@ -684,7 +684,7 @@ def _effective_models_dir() -> str: class _ModelsDirBody(BaseModel): - path: str = Field(default="", description="Absolute directory; empty clears → default cache") + authorization: str = Field(description="One-shot native desktop authorization") @router.get("/storage/models-dir") @@ -703,7 +703,7 @@ def get_models_dir(): } -@router.put("/storage/models-dir", dependencies=[Depends(require_desktop)]) +@router.put("/storage/models-dir") def set_models_dir(body: _ModelsDirBody): """Set (or clear, with an empty path) the models download directory. @@ -713,17 +713,18 @@ def set_models_dir(body: _ModelsDirBody): saved. Returns restart_required=True. """ from core import user_env + from core.path_authorization import PathAuthorizationError, consume - raw = (body.path or "").strip() + try: + raw = consume(body.authorization, "models_dir").strip() + except PathAuthorizationError as exc: + raise HTTPException(status_code=403, detail=str(exc)) from exc if not raw: user_env.unset_user_env(_MODELS_DIR_ENV) return {"configured": None, "default": _default_models_dir(), "restart_required": True} - # Reject control characters / NUL before touching the filesystem: an - # embedded NUL makes os.makedirs raise ValueError (→ 500). This is also - # the input-validation barrier for the path before it reaches any fs call - # (the dir is user-chosen by design — this is a loopback-gated, same-user - # local file picker, not a cross-privilege boundary). + # Tauri already validates this before issuing the capability. Keep the + # backend checks as defense in depth against a corrupt capability file. if any(ord(ch) < 0x20 or ord(ch) == 0x7F for ch in raw): raise HTTPException(status_code=400, detail="Path contains invalid control characters") diff --git a/backend/api/routers/system.py b/backend/api/routers/system.py index d9637090..3a957f91 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 require_admin, require_desktop +from api.dependencies import require_admin from fastapi.responses import FileResponse, StreamingResponse import torch import shutil @@ -805,7 +805,6 @@ async def ack_crash(): PERSISTENT_KEYS = { "HTTP_PROXY", "HTTPS_PROXY", "ALL_PROXY", "http_proxy", "https_proxy", "all_proxy", - "FFMPEG_PATH", "FFPROBE_PATH", "TRANSLATE_BASE_URL", "TRANSLATE_API_KEY", "TRANSLATE_MODEL", "DEEPL_API_KEY", "DEEPL_BASE_URL", "MICROSOFT_API_KEY", "MICROSOFT_BASE_URL", @@ -831,7 +830,7 @@ except Exception: # pragma: no cover — defensive: env panel > installer wirin _PORT_KEYS = {"OMNIVOICE_PORT", "OMNIVOICE_SHARE_PORT", "OMNIVOICE_UI_PORT"} -@router.post("/system/set-env", dependencies=[Depends(require_desktop)]) +@router.post("/system/set-env") async def set_env_var(body: dict): """Set an environment variable at runtime, persisted across restarts. @@ -857,23 +856,6 @@ async def set_env_var(body: dict): ) if value: - # Validate executable paths if the user is setting them manually. - # Reject control characters / null bytes (defense-in-depth against - # path-injection), then require an existing regular file. NOTE: this - # endpoint is loopback-only and MUST remain so — a remote caller able - # to set FFMPEG_PATH/FFPROBE_PATH could point it at an arbitrary - # binary (RCE). Network sharing must never expose /system/set-env. - if key in ("FFMPEG_PATH", "FFPROBE_PATH"): - if any(ord(c) < 0x20 or ord(c) == 0x7F for c in value): - raise HTTPException( - status_code=400, - detail="Invalid path: control characters are not allowed", - ) - if not os.path.isfile(value): - raise HTTPException( - status_code=400, - detail=f"File not found: {value}", - ) # Port keys must be a numeric string in the unprivileged range so a # typo can't drop the backend onto a privileged port (<1024) or an # out-of-range value uvicorn would reject at bind time. diff --git a/backend/core/path_authorization.py b/backend/core/path_authorization.py new file mode 100644 index 00000000..95f1ac96 --- /dev/null +++ b/backend/core/path_authorization.py @@ -0,0 +1,67 @@ +"""Consume one-shot host paths authorized by the native Tauri process. + +The web API never accepts a filesystem destination or executable path. Tauri +validates the user's native IPC request, writes a private capability file, and +only the unguessable capability token crosses loopback HTTP. +""" +from __future__ import annotations + +import json +import os +import re +import stat + +_TOKEN_RE = re.compile(r"[0-9a-f]{64}\Z") +_KINDS = {"models_dir", "ffmpeg", "ffprobe"} + + +class PathAuthorizationError(ValueError): + pass + + +def consume(token: str, expected_kind: str) -> str: + """Consume and return a single Tauri-authorized path. + + Capability files are one-shot and opened without following symlinks. The + containing directory is supplied only to the desktop-spawned backend; a + source/Docker backend has no path-authority channel by design. + """ + if expected_kind not in _KINDS or not _TOKEN_RE.fullmatch(token or ""): + raise PathAuthorizationError("Invalid or expired desktop authorization") + root = os.environ.get("OMNIVOICE_PATH_AUTH_DIR", "") + if not root: + raise PathAuthorizationError("This path can only be selected in the desktop app") + candidate = os.path.join(root, f"{token}.json") + claimed = os.path.join(root, f".{token}.consuming") + try: + os.replace(candidate, claimed) + except OSError as exc: + raise PathAuthorizationError("Invalid or expired desktop authorization") from exc + flags = os.O_RDONLY + if hasattr(os, "O_NOFOLLOW"): + flags |= os.O_NOFOLLOW + try: + fd = os.open(claimed, flags) + except OSError as exc: + raise PathAuthorizationError("Invalid or expired desktop authorization") from exc + try: + info = os.fstat(fd) + if not stat.S_ISREG(info.st_mode) or info.st_size > 16_384: + raise PathAuthorizationError("Invalid desktop authorization") + with os.fdopen(fd, "r", encoding="utf-8") as handle: + fd = -1 + payload = json.load(handle) + except (OSError, UnicodeError, json.JSONDecodeError, TypeError) as exc: + raise PathAuthorizationError("Invalid desktop authorization") from exc + finally: + if fd >= 0: + os.close(fd) + try: + os.unlink(claimed) + except OSError: + pass + if not isinstance(payload, dict): + raise PathAuthorizationError("Invalid desktop authorization") + if payload.get("kind") != expected_kind or not isinstance(payload.get("path"), str): + raise PathAuthorizationError("Desktop authorization does not match this setting") + return payload["path"] diff --git a/docs/api-auth.md b/docs/api-auth.md index 2f402111..6c0cccf6 100644 --- a/docs/api-auth.md +++ b/docs/api-auth.md @@ -213,13 +213,15 @@ requirement is dropped (issue #261, else the operator is 403'd out of their own - **A credential is configured** → admin requires the **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. So a **PIN-only** server-mode deployment keeps - admin loopback-only; remote admin requires the long API key. + 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. -Two host-path capabilities are never remote: `/system/set-env` and -`PUT /api/settings/storage/models-dir`. They can select executable or writable -filesystem paths, so only a genuine loopback desktop caller may use them; -server mode and an API key do not weaken that boundary. +Host paths are never selected through HTTP. The native Tauri process validates +model-cache destinations and custom FFmpeg/FFprobe binaries, writes a private +one-shot capability, and only that opaque authorization reaches the backend. +`/system/set-env` does not accept executable-path keys at all. Server mode and +an API key do not weaken that native boundary. This is the fix for a real escalation (#1213): before it, server mode made the admin gate a no-op, so with an API key set *and* a trusted CIDR configured, a LAN diff --git a/frontend/src-tauri/Cargo.lock b/frontend/src-tauri/Cargo.lock index f177b98e..24471215 100644 --- a/frontend/src-tauri/Cargo.lock +++ b/frontend/src-tauri/Cargo.lock @@ -2947,6 +2947,7 @@ dependencies = [ "dirs-next", "enigo", "fs4", + "getrandom 0.3.4", "libc", "log", "reqwest", diff --git a/frontend/src-tauri/Cargo.toml b/frontend/src-tauri/Cargo.toml index 221bed66..68fb498f 100644 --- a/frontend/src-tauri/Cargo.toml +++ b/frontend/src-tauri/Cargo.toml @@ -22,6 +22,7 @@ tauri-build = { version = "2.6.0", features = [] } [dependencies] serde_json = "1.0" serde = { version = "1.0", features = ["derive"] } +getrandom = "0.3" log = "0.4" tauri = { version = "2.11.0", features = ["macos-private-api", "protocol-asset", "tray-icon", "image-png"] } tauri-plugin-log = "2" diff --git a/frontend/src-tauri/src/backend.rs b/frontend/src-tauri/src/backend.rs index a207552d..2189ecf5 100644 --- a/frontend/src-tauri/src/backend.rs +++ b/frontend/src-tauri/src/backend.rs @@ -387,6 +387,10 @@ pub fn spawn_backend(app: &tauri::AppHandle, progress: Opt // pass below — otherwise a user-set OMNIVOICE_PORT would change the // LAN-share/Tailscale target while the listener stayed on the Rust port. env.push(("OMNIVOICE_PORT".into(), backend_port().to_string())); + env.push(( + "OMNIVOICE_PATH_AUTH_DIR".into(), + crate::commands::path_authorization_dir(app).to_string_lossy().into(), + )); if cfg!(target_os = "windows") { env.push(("TORCHDYNAMO_DISABLE".into(), "1".into())); env.push(("HF_HUB_DISABLE_SYMLINKS_WARNING".into(), "1".into())); diff --git a/frontend/src-tauri/src/commands.rs b/frontend/src-tauri/src/commands.rs index b619c81d..1c966a89 100644 --- a/frontend/src-tauri/src/commands.rs +++ b/frontend/src-tauri/src/commands.rs @@ -5,13 +5,119 @@ use std::path::{Path, PathBuf}; use std::sync::atomic::Ordering; use std::time::Duration; -use serde::Serialize; +use serde::{Deserialize, Serialize}; use tauri::image::Image; +use tauri::Manager; use crate::{AppFlags, TrayHandle, DictationShortcutState}; use crate::{TRAY_ICON_DEFAULT, TRAY_ICON_RECORDING}; use crate::config::{load_config, save_config}; +// ── Native host-path authorization ─────────────────────────────────────── + +#[derive(Serialize, Deserialize)] +struct AuthorizedHostPath { + kind: String, + path: String, +} + +pub fn path_authorization_dir(app: &tauri::AppHandle) -> PathBuf { + app.path() + .app_local_data_dir() + .unwrap_or_default() + .join("path-authorizations") +} + +fn validate_host_path(kind: &str, raw: &str) -> Result { + if !matches!(kind, "models_dir" | "ffmpeg" | "ffprobe") { + return Err("Unsupported host-path capability".into()); + } + if raw.chars().any(|c| c.is_control()) { + return Err("Path contains invalid control characters".into()); + } + if kind == "models_dir" && raw.is_empty() { + return Ok(PathBuf::new()); // explicit reset to the platform default + } + let path = PathBuf::from(raw); + if !path.is_absolute() { + return Err("Path must be absolute".into()); + } + if kind == "models_dir" { + fs::create_dir_all(&path).map_err(|e| format!("Directory is not writable: {e}"))?; + let probe = path.join(".voicestudio-write-test"); + fs::write(&probe, b"ok").map_err(|e| format!("Directory is not writable: {e}"))?; + let _ = fs::remove_file(probe); + } else { + if !path.is_file() { + return Err("Selected media tool is not a file".into()); + } + let status = crate::tools::no_window( + std::process::Command::new(&path) + .arg("-version") + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()), + ) + .status() + .map_err(|e| format!("Selected media tool could not run: {e}"))?; + if !status.success() { + return Err("Selected media tool failed its version check".into()); + } + } + Ok(path) +} + +#[tauri::command] +pub fn authorize_host_path( + app: tauri::AppHandle, + kind: String, + path: String, +) -> Result { + let validated = validate_host_path(&kind, path.trim())?; + let mut random = [0_u8; 32]; + getrandom::fill(&mut random).map_err(|e| format!("Secure randomness unavailable: {e}"))?; + let token: String = random.iter().map(|b| format!("{b:02x}")).collect(); + let dir = path_authorization_dir(&app); + fs::create_dir_all(&dir).map_err(|e| format!("Could not create authorization store: {e}"))?; + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + fs::set_permissions(&dir, fs::Permissions::from_mode(0o700)) + .map_err(|e| format!("Could not protect authorization store: {e}"))?; + } + let target = dir.join(format!("{token}.json")); + let payload = AuthorizedHostPath { + kind, + path: validated.to_string_lossy().into_owned(), + }; + fs::write(&target, serde_json::to_vec(&payload).map_err(|e| e.to_string())?) + .map_err(|e| format!("Could not authorize path: {e}"))?; + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + fs::set_permissions(&target, fs::Permissions::from_mode(0o600)) + .map_err(|e| format!("Could not protect authorization: {e}"))?; + } + Ok(token) +} + +#[cfg(test)] +mod host_path_authorization_tests { + use super::validate_host_path; + use std::path::PathBuf; + + #[test] + fn rejects_unknown_relative_and_control_character_paths() { + assert!(validate_host_path("shell", "/tmp/tool").is_err()); + assert!(validate_host_path("models_dir", "relative/models").is_err()); + assert!(validate_host_path("models_dir", "/tmp/bad\npath").is_err()); + } + + #[test] + fn empty_models_path_is_the_authorized_default_reset() { + assert_eq!(validate_host_path("models_dir", "").unwrap(), PathBuf::new()); + } +} + // ── System metrics ──────────────────────────────────────────────────────── #[derive(Serialize, Clone)] diff --git a/frontend/src-tauri/src/lib.rs b/frontend/src-tauri/src/lib.rs index 4313d44f..71c7897e 100644 --- a/frontend/src-tauri/src/lib.rs +++ b/frontend/src-tauri/src/lib.rs @@ -422,6 +422,7 @@ pub fn run() { updater_channel::install_update, updater_channel::list_releases, commands::get_sysinfo, + commands::authorize_host_path, commands::read_log_tail, commands::hf_cache_scan, commands::simulate_paste, diff --git a/frontend/src/components/settings/AudioToolsPanel.jsx b/frontend/src/components/settings/AudioToolsPanel.jsx index d7808327..4d1bc522 100644 --- a/frontend/src/components/settings/AudioToolsPanel.jsx +++ b/frontend/src/components/settings/AudioToolsPanel.jsx @@ -267,7 +267,27 @@ export default function AudioToolsPanel() { const onToolAction = useCallback( async (path, body) => { - const ok = await post(path, body); + let requestBody = body; + if (path.endsWith('/custom-path')) { + try { + const { invoke } = await import('@tauri-apps/api/core'); + const tool = path.includes('/ffprobe/') ? 'ffprobe' : 'ffmpeg'; + const authorization = await invoke('authorize_host_path', { + kind: tool, + path: body?.path || '', + }); + requestBody = { authorization }; + } catch (e) { + toast.error( + t('settings.audio_tools_path_failed', { + message: e.message || String(e), + defaultValue: "Couldn't set path: {{message}}", + }), + ); + return; + } + } + const ok = await post(path, requestBody); if (ok && (path.endsWith('/custom-path') || path.endsWith('/use-system'))) { toast.success( t('settings.audio_tools_path_set', { diff --git a/frontend/src/components/settings/AudioToolsPanel.test.jsx b/frontend/src/components/settings/AudioToolsPanel.test.jsx index 34fea7e9..a596fcec 100644 --- a/frontend/src/components/settings/AudioToolsPanel.test.jsx +++ b/frontend/src/components/settings/AudioToolsPanel.test.jsx @@ -11,6 +11,8 @@ vi.mock('../../api/client', () => ({ apiJson: vi.fn(), apiFetch: vi.fn(), })); +const invoke = vi.fn(); +vi.mock('@tauri-apps/api/core', () => ({ invoke: (...args) => invoke(...args) })); import { toast } from 'react-hot-toast'; import { apiJson, apiFetch } from '../../api/client'; @@ -57,6 +59,7 @@ describe('AudioToolsPanel — power-user surface for the media tools', () => { vi.clearAllMocks(); apiJson.mockResolvedValue(JSON.parse(JSON.stringify(STATUS))); apiFetch.mockResolvedValue(okResponse); + invoke.mockResolvedValue('c'.repeat(64)); }); it('renders one row per tool with version, path, and origin badge', async () => { @@ -85,6 +88,26 @@ describe('AudioToolsPanel — power-user surface for the media tools', () => { await waitFor(() => expect(toast.success).toHaveBeenCalled()); }); + it('sends only a native one-shot authorization for a custom executable', async () => { + render(); + fireEvent.click(await screen.findByLabelText('FFmpeg: Choose file…')); + const input = await screen.findByLabelText('FFmpeg binary path'); + fireEvent.change(input, { target: { value: '/opt/tools/ffmpeg' } }); + fireEvent.click(screen.getByRole('button', { name: 'Save' })); + + await waitFor(() => + expect(invoke).toHaveBeenCalledWith('authorize_host_path', { + kind: 'ffmpeg', + path: '/opt/tools/ffmpeg', + }), + ); + expect(apiFetch).toHaveBeenCalledWith( + '/media-tools/ffmpeg/custom-path', + expect.objectContaining({ body: JSON.stringify({ authorization: 'c'.repeat(64) }) }), + ); + expect(apiFetch.mock.calls.flat().join(' ')).not.toContain('/opt/tools/ffmpeg'); + }); + it('Restore bundled is per-tool and always available (safe revert)', async () => { render(); fireEvent.click(await screen.findByLabelText('FFprobe: Restore bundled')); diff --git a/frontend/src/components/settings/StoragePanel.jsx b/frontend/src/components/settings/StoragePanel.jsx index 64d08e20..7cf29dec 100644 --- a/frontend/src/components/settings/StoragePanel.jsx +++ b/frontend/src/components/settings/StoragePanel.jsx @@ -9,7 +9,7 @@ * Endpoints: * GET /api/settings/storage/models-dir * → {configured, effective, default, restart_required} - * PUT /api/settings/storage/models-dir body {path} (empty path clears) + * Native IPC authorizes the path, then PUT sends only the one-shot token. */ import React, { useCallback, useEffect, useState } from 'react'; import { HardDrive } from 'lucide-react'; @@ -52,10 +52,12 @@ export default function StoragePanel() { setSaving(true); setError(null); try { + const { invoke } = await import('@tauri-apps/api/core'); + const authorization = await invoke('authorize_host_path', { kind: 'models_dir', path }); const res = await apiFetch('/api/settings/storage/models-dir', { method: 'PUT', headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ path }), + body: JSON.stringify({ authorization }), }); if (!res.ok) { const b = await res.json().catch(() => ({})); diff --git a/frontend/src/components/settings/StoragePanel.test.jsx b/frontend/src/components/settings/StoragePanel.test.jsx new file mode 100644 index 00000000..262b9ce1 --- /dev/null +++ b/frontend/src/components/settings/StoragePanel.test.jsx @@ -0,0 +1,43 @@ +import React from 'react'; +import { describe, it, expect, vi } from 'vitest'; +import { fireEvent, render, screen, waitFor } from '@testing-library/react'; + +vi.mock('react-hot-toast', () => ({ default: { success: vi.fn() } })); +const apiJson = vi.fn(); +const apiFetch = vi.fn(); +vi.mock('../../api/client', () => ({ + apiJson: (...args) => apiJson(...args), + apiFetch: (...args) => apiFetch(...args), +})); +const invoke = vi.fn(); +vi.mock('@tauri-apps/api/core', () => ({ invoke: (...args) => invoke(...args) })); + +import StoragePanel from './StoragePanel'; + +describe('StoragePanel native path boundary', () => { + it('never sends the selected models directory through HTTP', async () => { + apiJson.mockResolvedValue({ configured: '', effective: '/cache', default: '/default' }); + apiFetch.mockResolvedValue({ + ok: true, + json: async () => ({ configured: '/private/models', restart_required: true }), + }); + invoke.mockResolvedValue('d'.repeat(64)); + render(); + + const input = await screen.findByTestId('models-dir-input'); + fireEvent.change(input, { target: { value: '/private/models' } }); + fireEvent.click(screen.getByTestId('models-dir-save')); + + await waitFor(() => + expect(invoke).toHaveBeenCalledWith('authorize_host_path', { + kind: 'models_dir', + path: '/private/models', + }), + ); + expect(apiFetch).toHaveBeenCalledWith( + '/api/settings/storage/models-dir', + expect.objectContaining({ body: JSON.stringify({ authorization: 'd'.repeat(64) }) }), + ); + expect(apiFetch.mock.calls.flat().join(' ')).not.toContain('/private/models'); + }); +}); diff --git a/tests/test_api.py b/tests/test_api.py index 6cd9fa95..fe12b1e0 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -624,8 +624,8 @@ def test_server_mode_remote_without_api_key_cannot_set_executable_path(monkeypat os.environ["FFMPEG_PATH"] = original -def test_server_mode_remote_api_key_cannot_set_executable_path(monkeypatch, tmp_path): - """An admin key does not grant the desktop file-picker capability.""" +def test_set_env_never_accepts_executable_path_even_with_admin_key(monkeypatch, tmp_path): + """Executable selection exists only behind native IPC, never this API.""" from fastapi.testclient import TestClient from main import app @@ -640,7 +640,7 @@ def test_server_mode_remote_api_key_cannot_set_executable_path(monkeypatch, tmp_ headers={"authorization": "Bearer s3cret"}, json={"key": "FFMPEG_PATH", "value": str(executable)}, ) - assert response.status_code == 403 + assert response.status_code == 400 assert os.environ.get("FFMPEG_PATH") == original finally: if original is None: diff --git a/tests/test_loopback_server_mode.py b/tests/test_loopback_server_mode.py index 7ad733c8..3369f0e1 100644 --- a/tests/test_loopback_server_mode.py +++ b/tests/test_loopback_server_mode.py @@ -10,14 +10,36 @@ from types import SimpleNamespace import pytest from fastapi import HTTPException -from api.dependencies import ( - is_loopback, - is_local_host, - require_admin, - require_desktop, - require_local, - require_loopback, -) +def _dependency(name): + # Resolve at test execution time: other suites intentionally replace + # ``api.*`` modules in sys.modules while probing cold-start behavior. + from api import dependencies + + return getattr(dependencies, name) + + +def is_loopback(*args, **kwargs): + return _dependency("is_loopback")(*args, **kwargs) + + +def is_local_host(*args, **kwargs): + return _dependency("is_local_host")(*args, **kwargs) + + +def require_admin(*args, **kwargs): + return _dependency("require_admin")(*args, **kwargs) + + +def require_desktop(*args, **kwargs): + return _dependency("require_desktop")(*args, **kwargs) + + +def require_local(*args, **kwargs): + return _dependency("require_local")(*args, **kwargs) + + +def require_loopback(*args, **kwargs): + return _dependency("require_loopback")(*args, **kwargs) def _req(host): diff --git a/tests/test_media_tools.py b/tests/test_media_tools.py index bee4309a..691157d2 100644 --- a/tests/test_media_tools.py +++ b/tests/test_media_tools.py @@ -10,6 +10,7 @@ from __future__ import annotations import hashlib import io +import json import os import zipfile from unittest.mock import patch @@ -27,6 +28,9 @@ def mt(monkeypatch, tmp_path): monkeypatch.setattr(prefs, "_PREFS_PATH", str(tmp_path / "prefs.json")) monkeypatch.setattr(mt_mod, "media_tools_dir", lambda: str(tmp_path / "media_tools")) + auth_dir = tmp_path / "path-authorizations" + auth_dir.mkdir() + monkeypatch.setenv("OMNIVOICE_PATH_AUTH_DIR", str(auth_dir)) for op in mt_mod._ops.values(): op.update(state="idle", progress=0.0, error=None) mt_mod._version_cache.clear() @@ -382,11 +386,27 @@ def test_router_status_and_acquire_endpoints(mt, monkeypatch): assert r.json()["state"] == "running" -def test_router_custom_path_maps_validation_to_400(mt): +def test_router_custom_path_rejects_raw_http_path(mt): c = _client() r = c.post("/media-tools/ffmpeg/custom-path", json={"path": "/no/such/binary"}) - assert r.status_code == 400 - assert "not found" in r.json()["detail"].lower() + assert r.status_code == 422 + + +def test_router_custom_path_consumes_native_authorization(mt, monkeypatch, tmp_path): + binary = tmp_path / "ffmpeg" + binary.write_bytes(b"native-authorized") + monkeypatch.setattr(mt, "_binary_runs", lambda _path: True) + token = "b" * 64 + auth_file = os.path.join(os.environ["OMNIVOICE_PATH_AUTH_DIR"], f"{token}.json") + with open(auth_file, "w", encoding="utf-8") as handle: + json.dump({"kind": "ffmpeg", "path": str(binary)}, handle) + c = _client() + response = c.post( + "/media-tools/ffmpeg/custom-path", json={"authorization": token} + ) + assert response.status_code == 200 + assert os.environ["FFMPEG_PATH"] == str(binary) + assert not os.path.exists(auth_file) def test_router_use_system_maps_lookup_to_404(mt, monkeypatch): diff --git a/tests/test_models_dir_setting.py b/tests/test_models_dir_setting.py index 60b94328..af715cc3 100644 --- a/tests/test_models_dir_setting.py +++ b/tests/test_models_dir_setting.py @@ -8,6 +8,7 @@ second store to diverge from. from __future__ import annotations import os +import json import fastapi import pytest @@ -24,12 +25,23 @@ def env(tmp_path, monkeypatch): # module object — a setattr monkeypatch wouldn't reach the endpoint's copy. envfile = str(tmp_path / "env") monkeypatch.setenv("OMNIVOICE_ENV_FILE", envfile) + auth_dir = tmp_path / "authorizations" + auth_dir.mkdir() + monkeypatch.setenv("OMNIVOICE_PATH_AUTH_DIR", str(auth_dir)) return envfile +def _body(path, kind="models_dir"): + auth_dir = os.environ["OMNIVOICE_PATH_AUTH_DIR"] + token = "a" * 64 + with open(os.path.join(auth_dir, f"{token}.json"), "w", encoding="utf-8") as f: + json.dump({"kind": kind, "path": path}, f) + return s._ModelsDirBody(authorization=token) + + def test_set_persists_and_writes_durable_env(env, tmp_path): target = str(tmp_path / "models") - res = s.set_models_dir(s._ModelsDirBody(path=target)) + res = s.set_models_dir(_body(target)) abs_target = os.path.abspath(target) assert res["configured"] == abs_target assert res["restart_required"] is True @@ -47,7 +59,7 @@ def test_rejects_unwritable_dir(env, monkeypatch, tmp_path): monkeypatch.setattr(os, "makedirs", boom) with pytest.raises(fastapi.HTTPException) as ei: - s.set_models_dir(s._ModelsDirBody(path=str(tmp_path / "ro"))) + s.set_models_dir(_body(str(tmp_path / "ro"))) assert ei.value.status_code == 400 @@ -55,7 +67,7 @@ def test_rejects_path_with_null_byte(env): # An embedded NUL would otherwise blow up os.makedirs with a ValueError # (→ 500). Validate up front and return a clean 400 instead. with pytest.raises(fastapi.HTTPException) as ei: - s.set_models_dir(s._ModelsDirBody(path="/tmp/mo\x00dels")) + s.set_models_dir(_body("/tmp/mo\x00dels")) assert ei.value.status_code == 400 @@ -76,8 +88,8 @@ def test_server_mode_remote_without_api_key_cannot_create_models_dir(env, monkey assert not target.exists() -def test_server_mode_remote_api_key_cannot_create_models_dir(env, monkeypatch, tmp_path): - """An admin key does not grant the desktop file-picker capability.""" +def test_server_mode_admin_key_cannot_supply_raw_models_path(env, monkeypatch, tmp_path): + """An admin key is not a native path authorization.""" from fastapi.testclient import TestClient monkeypatch.setenv("OMNIVOICE_SERVER_MODE", "1") @@ -90,13 +102,34 @@ def test_server_mode_remote_api_key_cannot_create_models_dir(env, monkeypatch, t headers={"authorization": "Bearer s3cret"}, json={"path": str(target)}, ) - assert response.status_code == 403 + assert response.status_code == 422 assert not target.exists() +def test_loopback_raw_path_is_not_a_models_directory_authorization(env, tmp_path): + from fastapi.testclient import TestClient + + target = tmp_path / "must-not-exist" + app = fastapi.FastAPI() + app.include_router(s.router) + response = TestClient(app, client=("127.0.0.1", 50000)).put( + "/api/settings/storage/models-dir", json={"path": str(target)} + ) + assert response.status_code == 422 + assert not target.exists() + + +def test_models_directory_authorization_is_one_shot(env, tmp_path): + body = _body(str(tmp_path / "models")) + assert s.set_models_dir(body)["configured"] + with pytest.raises(fastapi.HTTPException) as exc: + s.set_models_dir(body) + assert exc.value.status_code == 403 + + def test_clear_reverts_to_default(env): user_env.set_user_env("OMNIVOICE_CACHE_DIR", "/old") - res = s.set_models_dir(s._ModelsDirBody(path="")) + res = s.set_models_dir(_body("")) assert res["configured"] is None assert res["restart_required"] is True assert user_env.get_user_env("OMNIVOICE_CACHE_DIR") is None @@ -119,7 +152,7 @@ def test_path_with_spaces_survives_the_full_persistence_chain(env, tmp_path, mon still shows the chosen folder. Pin the whole chain byte-for-byte: endpoint → env file → load_into_environ → os.environ → GET.""" target = str(tmp_path / "Program Data" / "OmniVoice" / "Model Cache") - res = s.set_models_dir(s._ModelsDirBody(path=target)) + res = s.set_models_dir(_body(target)) abs_target = os.path.abspath(target) assert res["configured"] == abs_target assert user_env.get_user_env("OMNIVOICE_CACHE_DIR") == abs_target From c4a42d7e5550e046c8450d6a86ec6d02147d94fe Mon Sep 17 00:00:00 2001 From: debpalash <4178343+debpalash@users.noreply.github.com> Date: Sun, 9 Aug 2026 21:27:51 +0000 Subject: [PATCH 07/22] fix(security): keep capability lookup data-independent --- backend/core/path_authorization.py | 37 +++++++++++++++++++++++------- frontend/src-tauri/src/backend.rs | 4 ---- frontend/src-tauri/src/commands.rs | 10 ++++---- tests/test_media_tools.py | 8 ++++--- tests/test_models_dir_setting.py | 8 ++++--- 5 files changed, 44 insertions(+), 23 deletions(-) diff --git a/backend/core/path_authorization.py b/backend/core/path_authorization.py index 95f1ac96..165b0433 100644 --- a/backend/core/path_authorization.py +++ b/backend/core/path_authorization.py @@ -9,10 +9,14 @@ from __future__ import annotations import json import os import re +import secrets import stat +from core.config import DATA_DIR + _TOKEN_RE = re.compile(r"[0-9a-f]{64}\Z") _KINDS = {"models_dir", "ffmpeg", "ffprobe"} +_AUTH_DIR = os.path.join(DATA_DIR, ".path-authorizations") class PathAuthorizationError(ValueError): @@ -22,18 +26,33 @@ class PathAuthorizationError(ValueError): def consume(token: str, expected_kind: str) -> str: """Consume and return a single Tauri-authorized path. - Capability files are one-shot and opened without following symlinks. The - containing directory is supplied only to the desktop-spawned backend; a - source/Docker backend has no path-authority channel by design. + Capability files are one-shot and opened without following symlinks. Tauri + writes them into the app's private data directory; source/Docker callers + cannot mint a valid token through HTTP. """ if expected_kind not in _KINDS or not _TOKEN_RE.fullmatch(token or ""): raise PathAuthorizationError("Invalid or expired desktop authorization") - root = os.environ.get("OMNIVOICE_PATH_AUTH_DIR", "") - if not root: - raise PathAuthorizationError("This path can only be selected in the desktop app") - candidate = os.path.join(root, f"{token}.json") - claimed = os.path.join(root, f".{token}.consuming") + root = _AUTH_DIR + candidate = None try: + for entry in os.scandir(root): + if not _TOKEN_RE.fullmatch(entry.name.removesuffix(".json")): + continue + if not entry.is_file(follow_symlinks=False): + continue + try: + with open(entry.path, "r", encoding="utf-8") as handle: + probe = json.load(handle) + except (OSError, UnicodeError, json.JSONDecodeError): + continue # Ignore corrupt/stale capabilities; they authorize nothing. + if isinstance(probe, dict) and secrets.compare_digest( + str(probe.get("token", "")), token + ): + candidate = entry.path + break + if candidate is None: + raise OSError("capability not found") + claimed = os.path.join(root, f".consuming-{os.getpid()}-{secrets.token_hex(16)}") os.replace(candidate, claimed) except OSError as exc: raise PathAuthorizationError("Invalid or expired desktop authorization") from exc @@ -62,6 +81,8 @@ def consume(token: str, expected_kind: str) -> str: pass if not isinstance(payload, dict): raise PathAuthorizationError("Invalid desktop authorization") + if not secrets.compare_digest(str(payload.get("token", "")), token): + raise PathAuthorizationError("Invalid desktop authorization") if payload.get("kind") != expected_kind or not isinstance(payload.get("path"), str): raise PathAuthorizationError("Desktop authorization does not match this setting") return payload["path"] diff --git a/frontend/src-tauri/src/backend.rs b/frontend/src-tauri/src/backend.rs index 2189ecf5..a207552d 100644 --- a/frontend/src-tauri/src/backend.rs +++ b/frontend/src-tauri/src/backend.rs @@ -387,10 +387,6 @@ pub fn spawn_backend(app: &tauri::AppHandle, progress: Opt // pass below — otherwise a user-set OMNIVOICE_PORT would change the // LAN-share/Tailscale target while the listener stayed on the Rust port. env.push(("OMNIVOICE_PORT".into(), backend_port().to_string())); - env.push(( - "OMNIVOICE_PATH_AUTH_DIR".into(), - crate::commands::path_authorization_dir(app).to_string_lossy().into(), - )); if cfg!(target_os = "windows") { env.push(("TORCHDYNAMO_DISABLE".into(), "1".into())); env.push(("HF_HUB_DISABLE_SYMLINKS_WARNING".into(), "1".into())); diff --git a/frontend/src-tauri/src/commands.rs b/frontend/src-tauri/src/commands.rs index 1c966a89..4be42a13 100644 --- a/frontend/src-tauri/src/commands.rs +++ b/frontend/src-tauri/src/commands.rs @@ -7,7 +7,6 @@ use std::time::Duration; use serde::{Deserialize, Serialize}; use tauri::image::Image; -use tauri::Manager; use crate::{AppFlags, TrayHandle, DictationShortcutState}; use crate::{TRAY_ICON_DEFAULT, TRAY_ICON_RECORDING}; @@ -17,15 +16,15 @@ use crate::config::{load_config, save_config}; #[derive(Serialize, Deserialize)] struct AuthorizedHostPath { + token: String, kind: String, path: String, } pub fn path_authorization_dir(app: &tauri::AppHandle) -> PathBuf { - app.path() - .app_local_data_dir() - .unwrap_or_default() - .join("path-authorizations") + crate::setup::resolved_data_dir(app) + .unwrap_or_else(crate::setup::default_data_dir) + .join(".path-authorizations") } fn validate_host_path(kind: &str, raw: &str) -> Result { @@ -86,6 +85,7 @@ pub fn authorize_host_path( } let target = dir.join(format!("{token}.json")); let payload = AuthorizedHostPath { + token: token.clone(), kind, path: validated.to_string_lossy().into_owned(), }; diff --git a/tests/test_media_tools.py b/tests/test_media_tools.py index 691157d2..94cd8f1e 100644 --- a/tests/test_media_tools.py +++ b/tests/test_media_tools.py @@ -30,7 +30,8 @@ def mt(monkeypatch, tmp_path): monkeypatch.setattr(mt_mod, "media_tools_dir", lambda: str(tmp_path / "media_tools")) auth_dir = tmp_path / "path-authorizations" auth_dir.mkdir() - monkeypatch.setenv("OMNIVOICE_PATH_AUTH_DIR", str(auth_dir)) + from core import path_authorization + monkeypatch.setattr(path_authorization, "_AUTH_DIR", str(auth_dir)) for op in mt_mod._ops.values(): op.update(state="idle", progress=0.0, error=None) mt_mod._version_cache.clear() @@ -397,9 +398,10 @@ def test_router_custom_path_consumes_native_authorization(mt, monkeypatch, tmp_p binary.write_bytes(b"native-authorized") monkeypatch.setattr(mt, "_binary_runs", lambda _path: True) token = "b" * 64 - auth_file = os.path.join(os.environ["OMNIVOICE_PATH_AUTH_DIR"], f"{token}.json") + from core import path_authorization + auth_file = os.path.join(path_authorization._AUTH_DIR, f"{token}.json") with open(auth_file, "w", encoding="utf-8") as handle: - json.dump({"kind": "ffmpeg", "path": str(binary)}, handle) + json.dump({"token": token, "kind": "ffmpeg", "path": str(binary)}, handle) c = _client() response = c.post( "/media-tools/ffmpeg/custom-path", json={"authorization": token} diff --git a/tests/test_models_dir_setting.py b/tests/test_models_dir_setting.py index af715cc3..d0983482 100644 --- a/tests/test_models_dir_setting.py +++ b/tests/test_models_dir_setting.py @@ -27,15 +27,17 @@ def env(tmp_path, monkeypatch): monkeypatch.setenv("OMNIVOICE_ENV_FILE", envfile) auth_dir = tmp_path / "authorizations" auth_dir.mkdir() - monkeypatch.setenv("OMNIVOICE_PATH_AUTH_DIR", str(auth_dir)) + from core import path_authorization + monkeypatch.setattr(path_authorization, "_AUTH_DIR", str(auth_dir)) return envfile def _body(path, kind="models_dir"): - auth_dir = os.environ["OMNIVOICE_PATH_AUTH_DIR"] + from core import path_authorization + auth_dir = path_authorization._AUTH_DIR token = "a" * 64 with open(os.path.join(auth_dir, f"{token}.json"), "w", encoding="utf-8") as f: - json.dump({"kind": kind, "path": path}, f) + json.dump({"token": token, "kind": kind, "path": path}, f) return s._ModelsDirBody(authorization=token) From 74511db595dd36ded6e97e6a35b690db65d071b4 Mon Sep 17 00:00:00 2001 From: debpalash <4178343+debpalash@users.noreply.github.com> Date: Sun, 9 Aug 2026 21:30:02 +0000 Subject: [PATCH 08/22] fix(security): redact share PIN from remote discovery --- backend/api/routers/system.py | 15 ++++++++++++--- backend/core/path_authorization.py | 2 +- docs/api-auth.md | 3 ++- tests/test_network_share.py | 22 ++++++++++++++++++++++ 4 files changed, 37 insertions(+), 5 deletions(-) diff --git a/backend/api/routers/system.py b/backend/api/routers/system.py index 3a957f91..12a6b995 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 require_admin +from api.dependencies import is_loopback, require_admin from fastapi.responses import FileResponse, StreamingResponse import torch import shutil @@ -1082,12 +1082,21 @@ def quarantine_status(): # ── Network sharing (loopback-only control surface) ────────────────────────── @router.get("/system/network/state") -async def network_state(): +async def network_state(request: Request): st = network_share.get_state() + # PIN-only server mode permits unauthenticated read-only discovery, but the + # PIN is itself a consumption credential. Reveal it only to the native + # loopback UI or to a remote caller that already passed the configured + # long API-key gate. The boolean lets headless dashboards remain useful. + host = request.client.host if request.client else None + may_reveal_pin = is_loopback(host) or bool( + os.environ.get("OMNIVOICE_API_KEY", "").strip() + ) return { "enabled": st.enabled, "share_port": st.share_port, - "pin": st.pin, + "pin": st.pin if may_reveal_pin else None, + "pin_required": bool(st.pin), "lan_addresses": st.lan_addresses, } diff --git a/backend/core/path_authorization.py b/backend/core/path_authorization.py index 165b0433..86d88620 100644 --- a/backend/core/path_authorization.py +++ b/backend/core/path_authorization.py @@ -78,7 +78,7 @@ def consume(token: str, expected_kind: str) -> str: try: os.unlink(claimed) except OSError: - pass + pass # Best-effort cleanup; the random claimed name cannot be reused. if not isinstance(payload, dict): raise PathAuthorizationError("Invalid desktop authorization") if not secrets.compare_digest(str(payload.get("token", "")), token): diff --git a/docs/api-auth.md b/docs/api-auth.md index 6c0cccf6..8d5a9e18 100644 --- a/docs/api-auth.md +++ b/docs/api-auth.md @@ -215,7 +215,8 @@ requirement is dropped (issue #261, else the operator is 403'd out of their own 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. + 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. Host paths are never selected through HTTP. The native Tauri process validates model-cache destinations and custom FFmpeg/FFprobe binaries, writes a private diff --git a/tests/test_network_share.py b/tests/test_network_share.py index 9cd32090..619c1f81 100644 --- a/tests/test_network_share.py +++ b/tests/test_network_share.py @@ -75,6 +75,28 @@ def test_network_state_endpoint_defaults_disabled(): assert r.json()["enabled"] is False +def test_pin_only_remote_discovery_never_returns_share_pin(monkeypatch): + from main import app + + monkeypatch.setenv("OMNIVOICE_SERVER_MODE", "1") + monkeypatch.delenv("OMNIVOICE_API_KEY", raising=False) + monkeypatch.setattr( + ns, + "_state", + ns.ShareState(True, 3901, "123456", ["192.168.1.10"]), + ) + # Keep the consumption middleware inert: this endpoint is testing the + # intentional admin read-only exception itself, before a PIN is supplied. + monkeypatch.setattr(app.state, "network_share", None, raising=False) + response = TestClient(app, client=("172.17.0.1", 50000)).get( + "/system/network/state" + ) + assert response.status_code == 200 + assert response.json()["pin"] is None + assert response.json()["pin_required"] is True + assert "123456" not in response.text + + def test_network_control_rejects_non_loopback(): from main import app c = TestClient(app, client=("10.0.0.5", 9999)) From 77be03b1305f775010a860db4a718846d58b1ed8 Mon Sep 17 00:00:00 2001 From: debpalash <4178343+debpalash@users.noreply.github.com> Date: Sun, 9 Aug 2026 21:33:45 +0000 Subject: [PATCH 09/22] fix(security): fail closed on cleanup and redaction errors --- backend/api/routers/batch.py | 15 ++-- backend/api/routers/gallery.py | 26 ++++--- backend/api/routers/system.py | 7 +- backend/core/file_cleanup.py | 24 +++++++ backend/core/scrub.py | 18 ++--- tests/test_destructive_cleanup.py | 110 ++++++++++++++++++++++++++++++ tests/test_scrub.py | 45 +++++++++++- 7 files changed, 217 insertions(+), 28 deletions(-) create mode 100644 backend/core/file_cleanup.py create mode 100644 tests/test_destructive_cleanup.py diff --git a/backend/api/routers/batch.py b/backend/api/routers/batch.py index a65ed2c8..b8885727 100644 --- a/backend/api/routers/batch.py +++ b/backend/api/routers/batch.py @@ -20,6 +20,7 @@ from pydantic import BaseModel from core.config import DATA_DIR from core import failure +from core.file_cleanup import FileCleanupError, unlink_if_present router = APIRouter() logger = logging.getLogger("omnivoice.batch") @@ -573,14 +574,18 @@ def cancel_batch_job(job_id: str): @router.delete("/batch/jobs/{job_id}") def delete_batch_job(job_id: str): """Delete a batch job record and its video file.""" - job = _jobs.pop(job_id, None) + job = _jobs.get(job_id) if not job: raise HTTPException(404, "Job not found") - if job.get("video_path") and os.path.exists(job["video_path"]): + if job.get("video_path"): try: - os.remove(job["video_path"]) - except Exception: - pass + unlink_if_present(job["video_path"]) + except FileCleanupError as exc: + raise HTTPException( + status_code=500, + detail="Could not delete the batch video file. Close any app using it and retry.", + ) from exc + _jobs.pop(job_id, None) return {"deleted": True} diff --git a/backend/api/routers/gallery.py b/backend/api/routers/gallery.py index dcab06f9..f5990f0f 100644 --- a/backend/api/routers/gallery.py +++ b/backend/api/routers/gallery.py @@ -13,6 +13,7 @@ from pydantic import BaseModel from core.db import db_conn from core.config import VOICES_DIR, OUTPUTS_DIR from core import event_bus +from core.file_cleanup import FileCleanupError, unlink_if_present from services.ffmpeg_utils import spawn_subprocess logger = logging.getLogger("omnivoice.gallery") @@ -139,11 +140,14 @@ def delete_voice(voice_id: str): raise HTTPException(status_code=404, detail="Voice not found") audio_path = row["audio_path"] - if audio_path and os.path.exists(audio_path): + if audio_path: try: - os.remove(audio_path) - except Exception: - pass + unlink_if_present(audio_path) + except FileCleanupError as exc: + raise HTTPException( + status_code=500, + detail="Could not delete the voice audio file. Close any app using it and retry.", + ) from exc conn.execute("DELETE FROM voice_gallery WHERE id = ?", (voice_id,)) return {"success": True} @@ -478,19 +482,22 @@ def batch_delete_voices(body: dict): return {"deleted": 0} deleted = 0 + failed = 0 with db_conn() as conn: for vid in ids: row = conn.execute("SELECT audio_path FROM voice_gallery WHERE id = ?", (vid,)).fetchone() if row: audio_path = row["audio_path"] - if audio_path and os.path.exists(audio_path): + if audio_path: try: - os.remove(audio_path) - except Exception: - pass + unlink_if_present(audio_path) + except FileCleanupError as exc: + logger.warning("Voice audio cleanup failed for gallery item %s: %s", vid, exc) + failed += 1 + continue conn.execute("DELETE FROM voice_gallery WHERE id = ?", (vid,)) deleted += 1 - return {"deleted": deleted} + return {"deleted": deleted, "failed": failed} @router.post("/gallery/voices/{voice_id}/to-profile") @@ -526,4 +533,3 @@ def voice_to_profile(voice_id: str): event_bus.emit("profiles", {"action": "created", "id": profile_id}) return {"success": True, "profile_id": profile_id, "name": voice["name"]} - diff --git a/backend/api/routers/system.py b/backend/api/routers/system.py index 89fc1856..0666e1f1 100644 --- a/backend/api/routers/system.py +++ b/backend/api/routers/system.py @@ -468,14 +468,15 @@ def _truncate_file(path: str): async def clear_tauri_logs(): """Truncate whichever Tauri-side log files we know about. OS-level rotation may recreate them.""" cleared = [] + failed = 0 for p in _tauri_log_candidates(): if os.path.exists(p): try: await asyncio.to_thread(_truncate_file, p) cleared.append(p) - except Exception: - pass - return {"cleared": cleared} + except OSError: + failed += 1 + return {"cleared": cleared, "failed": failed} @router.get("/sysinfo", response_model=SysinfoResponse) def get_sys_info(): diff --git a/backend/core/file_cleanup.py b/backend/core/file_cleanup.py new file mode 100644 index 00000000..37efab1e --- /dev/null +++ b/backend/core/file_cleanup.py @@ -0,0 +1,24 @@ +"""Reliable file deletion for user-visible destructive operations.""" +from __future__ import annotations + +import os + + +class FileCleanupError(OSError): + """A requested file exists but could not be removed.""" + + +def unlink_if_present(path: str | os.PathLike[str]) -> bool: + """Delete *path*, returning whether it existed. + + Missing files make delete operations idempotent. Other failures must reach + the caller so it cannot discard the only record from which cleanup can be + retried. + """ + try: + os.unlink(path) + except FileNotFoundError: + return False + except OSError as exc: + raise FileCleanupError("file cleanup failed") from exc + return True diff --git a/backend/core/scrub.py b/backend/core/scrub.py index 7b3b3d68..152198b2 100644 --- a/backend/core/scrub.py +++ b/backend/core/scrub.py @@ -91,9 +91,9 @@ def _env_secret_values() -> list[str]: def scrub_text(text: str | None) -> str: """Return ``text`` with secrets and home paths redacted. - Never raises — scrubbing failure must not block a bug report, and a - partially-scrubbed string is still better than an unscrubbed one, so - each pass is independent. + Never raises. If a redaction pass itself fails, return only the redaction + marker: diagnostic detail is less important than keeping credentials and + private paths out of a report. """ if not text: return "" if text is None else str(text) @@ -104,18 +104,18 @@ def scrub_text(text: str | None) -> str: for val in _env_secret_values(): s = s.replace(val, REDACTED) except Exception: - pass + return REDACTED # 2. Credential-shaped substrings + URL query secrets. for pat in _TOKEN_PATTERNS: try: s = pat.sub(REDACTED, s) except Exception: - pass + return REDACTED try: s = _URL_SECRET_RE.sub(lambda m: m.group(1) + REDACTED, s) except Exception: - pass + return REDACTED # 3. This process's real home dir (covers symlinked/nonstandard homes # the generic patterns miss), then the per-OS shapes. Boundary-aware so @@ -126,12 +126,12 @@ def scrub_text(text: str | None) -> str: if home and home not in ("/", "~"): s = re.sub(re.escape(home) + r"(?=[/\\\s\"']|$)", "~", s) except Exception: - pass + return REDACTED for pat in _HOME_PATTERNS: try: s = pat.sub("~", s) except Exception: - pass + return REDACTED return s @@ -153,5 +153,5 @@ def scrub_provider_error(detail: object, api_key: str | None = None) -> str: if api_key and api_key != "local" and len(api_key) >= _MIN_SECRET_LEN: s = s.replace(api_key, REDACTED) except Exception: - pass + return REDACTED return scrub_text(s) diff --git a/tests/test_destructive_cleanup.py b/tests/test_destructive_cleanup.py new file mode 100644 index 00000000..36a2f012 --- /dev/null +++ b/tests/test_destructive_cleanup.py @@ -0,0 +1,110 @@ +"""Destructive endpoints must not report success when file cleanup fails.""" +from contextlib import contextmanager + +import pytest +from fastapi import HTTPException + +from api.routers import batch, gallery, system +from core.file_cleanup import FileCleanupError, unlink_if_present + + +class _Result: + def __init__(self, row=None): + self._row = row + + def fetchone(self): + return self._row + + +class _Connection: + def __init__(self, audio_path): + self.audio_path = audio_path + self.deleted = False + + def execute(self, query, _params=()): + if query.startswith("SELECT"): + return _Result({"audio_path": self.audio_path}) + if query.startswith("DELETE"): + self.deleted = True + return _Result() + + +def test_unlink_missing_file_is_idempotent(tmp_path): + assert unlink_if_present(tmp_path / "already-gone.wav") is False + + +def test_gallery_delete_keeps_record_when_audio_cannot_be_removed(monkeypatch): + conn = _Connection("locked.wav") + + @contextmanager + def fake_db(): + yield conn + + monkeypatch.setattr(gallery, "db_conn", fake_db) + monkeypatch.setattr( + gallery, + "unlink_if_present", + lambda _path: (_ for _ in ()).throw(FileCleanupError("locked")), + ) + + with pytest.raises(HTTPException) as caught: + gallery.delete_voice("voice-1") + + assert caught.value.status_code == 500 + assert conn.deleted is False + assert "locked.wav" not in caught.value.detail + + +def test_batch_delete_keeps_job_when_video_cannot_be_removed(monkeypatch): + job = {"video_path": "locked.mp4"} + monkeypatch.setitem(batch._jobs, "job-1", job) + monkeypatch.setattr( + batch, + "unlink_if_present", + lambda _path: (_ for _ in ()).throw(FileCleanupError("locked")), + ) + + with pytest.raises(HTTPException) as caught: + batch.delete_batch_job("job-1") + + assert caught.value.status_code == 500 + assert batch._jobs["job-1"] is job + assert "locked.mp4" not in caught.value.detail + + +def test_gallery_batch_delete_reports_failure_and_keeps_failed_record(monkeypatch): + conn = _Connection("locked.wav") + + @contextmanager + def fake_db(): + yield conn + + monkeypatch.setattr(gallery, "db_conn", fake_db) + monkeypatch.setattr( + gallery, + "unlink_if_present", + lambda _path: (_ for _ in ()).throw(FileCleanupError("locked")), + ) + + assert gallery.batch_delete_voices({"ids": ["voice-1"]}) == { + "deleted": 0, + "failed": 1, + } + assert conn.deleted is False + + +@pytest.mark.asyncio +async def test_tauri_log_clear_reports_truncate_failure(monkeypatch, tmp_path): + log = tmp_path / "webview.log" + log.write_text("data", encoding="utf-8") + monkeypatch.setattr(system, "_tauri_log_candidates", lambda: [str(log)]) + monkeypatch.setattr( + system, + "_truncate_file", + lambda _path: (_ for _ in ()).throw(PermissionError("locked")), + ) + + result = await system.clear_tauri_logs() + + assert result == {"cleared": [], "failed": 1} + assert str(log) not in str(result) diff --git a/tests/test_scrub.py b/tests/test_scrub.py index d85882d2..8a8db5e3 100644 --- a/tests/test_scrub.py +++ b/tests/test_scrub.py @@ -8,7 +8,9 @@ import os import pytest -from core.scrub import scrub_text, REDACTED +import core.scrub as scrub_module + +from core.scrub import REDACTED, scrub_provider_error, scrub_text # ── Home directory redaction ────────────────────────────────────────────── @@ -146,3 +148,44 @@ def test_home_superstring_not_corrupted(monkeypatch): assert "~ny" not in out assert "johnny" not in out # still redacted by the generic macOS shape assert out == "~/secret.wav" + + +class _BrokenPattern: + def sub(self, *_args, **_kwargs): + raise RuntimeError("redaction engine failed") + + +def test_env_secret_collection_failure_is_fail_closed(monkeypatch): + secret = "private-provider-key-value" + monkeypatch.setattr( + scrub_module, + "_env_secret_values", + lambda: (_ for _ in ()).throw(RuntimeError("environment unavailable")), + ) + assert scrub_text(f"provider rejected {secret}") == REDACTED + + +@pytest.mark.parametrize("target", ["_TOKEN_PATTERNS", "_URL_SECRET_RE", "_HOME_PATTERNS"]) +def test_redaction_pattern_failure_is_fail_closed(monkeypatch, target): + value = (_BrokenPattern(),) if target.endswith("PATTERNS") else _BrokenPattern() + monkeypatch.setattr(scrub_module, target, value) + assert scrub_text("token=private-provider-key-value /home/alice/file") == REDACTED + + +def test_actual_home_redaction_failure_is_fail_closed(monkeypatch): + monkeypatch.setattr(scrub_module.os.path, "expanduser", lambda _path: (_ for _ in ()).throw(RuntimeError())) + assert scrub_text("token=private-provider-key-value /private/alice/file") == REDACTED + + +def test_provider_exact_key_redaction_failure_is_fail_closed(): + class _BrokenKey: + def __bool__(self): + return True + + def __eq__(self, _other): + return False + + def __len__(self): + raise RuntimeError("key inspection failed") + + assert scrub_provider_error("provider echoed a credential", _BrokenKey()) == REDACTED From aa5de23bd1cb468b5ff28248574db89911e06eea Mon Sep 17 00:00:00 2001 From: debpalash <4178343+debpalash@users.noreply.github.com> Date: Sun, 9 Aug 2026 21:34:07 +0000 Subject: [PATCH 10/22] docs: note reliable cleanup and redaction --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1b5afdd4..715d42bd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,6 +41,7 @@ The bundled TTS model package (`pyproject.toml`) is versioned independently. ### Fixed +- Failed gallery, batch-video, and desktop-log cleanup is now reported instead of silently claiming success, and diagnostic redaction fails closed if a scrubber breaks. (#1458) - Automatic model-mirror checks now reject untrusted URLs before opening a network connection. (#1447) - Sidecar engines no longer break when a library they load prints to the console. Those bytes landed in the middle of the engine's data stream, failing the generation and leaving the connection scrambled for every request after it. (#1428) — thanks @1335-Group! - A generation abandoned while stuck on an internal lock now says so, instead of blaming your hardware and suggesting shorter text. Nothing had been computed, so none of that advice applied. (#1416, #1419) From 8d4a9d11c8acbbb694f56f46ff5fcfbdc1db8f80 Mon Sep 17 00:00:00 2001 From: debpalash <4178343+debpalash@users.noreply.github.com> Date: Sun, 9 Aug 2026 21:39:37 +0000 Subject: [PATCH 11/22] fix(security): authorize native dub destinations --- backend/api/routers/dub_export.py | 88 +++++++++++++++++------- backend/core/path_authorization.py | 2 +- frontend/src-tauri/src/commands.rs | 25 ++++++- frontend/src/App.jsx | 2 +- frontend/src/utils/mediaDownload.js | 22 +++--- frontend/src/utils/mediaDownload.test.js | 15 ++-- tests/test_filesystem_boundaries.py | 75 ++++++++++++++++++-- 7 files changed, 184 insertions(+), 45 deletions(-) diff --git a/backend/api/routers/dub_export.py b/backend/api/routers/dub_export.py index c0bb1fbc..50716588 100644 --- a/backend/api/routers/dub_export.py +++ b/backend/api/routers/dub_export.py @@ -1,21 +1,21 @@ -import os +import asyncio import io -import re import json +import logging +import ntpath +import os +import re import time import uuid -import asyncio -import logging +from pathlib import Path, PureWindowsPath from typing import Optional -from fastapi import APIRouter, HTTPException, Query, Request, Response -from fastapi.responses import FileResponse, StreamingResponse from core.config import DUB_DIR, dub_seg_path -from core.tasks import task_manager from core.http_headers import content_disposition -from api.routers.dub_core import _get_job -from api.dependencies import require_native_access from core.path_security import UnsafePath, resolve_within +from core.tasks import task_manager +from fastapi import APIRouter, Header, HTTPException, Query, Response +from fastapi.responses import FileResponse, StreamingResponse from services.ffmpeg_utils import ( bed_mix_filter, explain_ffmpeg_failure, @@ -30,6 +30,8 @@ from services.video_retime import ( prepare_smart_fit_video, ) +from api.routers.dub_core import _get_job + router = APIRouter() logger = logging.getLogger("omnivoice.api") @@ -51,10 +53,45 @@ def _job_dir_or_400(job_id: str) -> str: raise HTTPException(status_code=400, detail="Invalid job id") from exc +def _resolve_dub_artifact(value: object) -> Path: + """Resolve current or safely rebased pre-relocation dub artifact paths.""" + raw = str(value or "") + try: + return resolve_within(DUB_DIR, raw) + except UnsafePath: + # Older job rows store absolute paths. After the user relocates the + # data directory, preserve only the suffix rooted at the exact + # ``dub_jobs`` boundary; never touch the old host path itself. + if ntpath.isabs(raw): + parts = PureWindowsPath(raw).parts + elif os.path.isabs(raw): + parts = Path(raw).parts + else: + raise + anchor = Path(DUB_DIR).name + positions = [index for index, part in enumerate(parts) if part == anchor] + if not positions: + raise + relative_parts = parts[positions[-1] + 1:] + if ( + not relative_parts + or not re.fullmatch(r"[A-Za-z0-9_-]{1,64}", relative_parts[0]) + or any( + part in {"", ".", ".."} + or "/" in part + or "\\" in part + or ":" in part + for part in relative_parts + ) + ): + raise + return resolve_within(DUB_DIR, Path(*relative_parts)) + + def _dub_artifact(value: object, *, missing_detail: str = "File not found") -> str: """Resolve a persisted job artifact inside the global dub-data boundary.""" try: - path = resolve_within(DUB_DIR, str(value or "")) + path = _resolve_dub_artifact(value) except UnsafePath as exc: raise HTTPException(status_code=400, detail="Invalid job artifact path") from exc if not path.is_file(): @@ -66,7 +103,7 @@ def _optional_dub_artifact(value: object) -> str | None: if not value: return None try: - path = resolve_within(DUB_DIR, str(value)) + path = _resolve_dub_artifact(value) except UnsafePath as exc: raise HTTPException(status_code=400, detail="Invalid job artifact path") from exc return str(path) if path.is_file() else None @@ -78,9 +115,15 @@ def _safe_lang_or_400(lang: str | None) -> str | None: return lang -def _guard_native_save(request: Request, save_path: str) -> None: - if save_path: - require_native_access(request) +def _consume_native_save(authorization: str) -> str | None: + if not authorization: + return None + from core.path_authorization import PathAuthorizationError, consume + + try: + return consume(authorization, "dub_export") + except PathAuthorizationError as exc: + raise HTTPException(status_code=403, detail=str(exc)) from exc def _native_save(source: str, destination: str, display_name: str, media_type: str): @@ -472,11 +515,10 @@ def _build_audio_export_cmd( @router.get("/dub/download/{job_id}/{filename}") async def dub_download( job_id: str, - request: Request, preserve_bg: bool = Query(True, description="Mix background noise into dubbed tracks"), default_track: str = Query("original"), include_tracks: str = Query("", description="Comma-separated list of tracks to include (e.g. 'original,de,es'). Empty = include all."), - save_path: str = Query("", description="Absolute destination path. If set, mux output is copied there and JSON returned instead of FileResponse."), + save_authorization: str = Header("", alias="X-VoiceStudio-Path-Authorization"), burn_subs: bool = Query(False, description="Burn subtitles into the video stream (forces re-encode). Uses dual-subtitle layout when dual=1."), dual: bool = Query(False, description="When burn_subs=1, render translated on top of italicised original."), out_format: str = Query("m4a", description="Audio-only jobs (#119): output container — wav, m4a, mp3, or flac. Ignored for video jobs."), @@ -485,7 +527,7 @@ async def dub_download( # path or ffmpeg argv (export dir, retime work path, slice paths). Real # job ids are short uuid slices — alnum/hyphen/underscore only. job_dir = _job_dir_or_400(job_id) - _guard_native_save(request, save_path) + save_path = _consume_native_save(save_authorization) job = _get_job(job_id) if not job: raise HTTPException(status_code=404, detail="Job not found") @@ -1323,8 +1365,8 @@ async def dub_qc_pass(job_id: str, lang: str = Query(None), drift_threshold: flo return result.get("segments", []), backend.id try: - from services.model_manager import _get_gpu_pool from services.asr_backend import ASRTimeoutError, run_transcribe_guarded + from services.model_manager import _get_gpu_pool recognized, engine_id = await run_transcribe_guarded( _get_gpu_pool(), _recognize, what="QC", ) @@ -1383,13 +1425,12 @@ async def dub_qc_pass(job_id: str, lang: str = Query(None), drift_threshold: flo @router.get("/dub/download-audio/{job_id}/{filename}") async def dub_download_audio( job_id: str, - request: Request, lang: str = Query(None), preserve_bg: bool = Query(True), - save_path: str = Query(""), + save_authorization: str = Header("", alias="X-VoiceStudio-Path-Authorization"), ): job_dir = _job_dir_or_400(job_id) - _guard_native_save(request, save_path) + save_path = _consume_native_save(save_authorization) lang = _safe_lang_or_400(lang) job = _get_job(job_id) if not job: @@ -1624,14 +1665,13 @@ async def dub_export_segments_zip(job_id: str, lang: str = Query(None)): @router.get("/dub/download-mp3/{job_id}/{filename}") async def dub_download_mp3( job_id: str, - request: Request, lang: str = Query(None), preserve_bg: bool = Query(True), - save_path: str = Query(""), + save_authorization: str = Header("", alias="X-VoiceStudio-Path-Authorization"), bitrate: str = Query("192k"), ): job_dir = _job_dir_or_400(job_id) - _guard_native_save(request, save_path) + save_path = _consume_native_save(save_authorization) lang = _safe_lang_or_400(lang) job = _get_job(job_id) if not job: diff --git a/backend/core/path_authorization.py b/backend/core/path_authorization.py index 86d88620..6ac3f37f 100644 --- a/backend/core/path_authorization.py +++ b/backend/core/path_authorization.py @@ -15,7 +15,7 @@ import stat from core.config import DATA_DIR _TOKEN_RE = re.compile(r"[0-9a-f]{64}\Z") -_KINDS = {"models_dir", "ffmpeg", "ffprobe"} +_KINDS = {"models_dir", "ffmpeg", "ffprobe", "dub_export"} _AUTH_DIR = os.path.join(DATA_DIR, ".path-authorizations") diff --git a/frontend/src-tauri/src/commands.rs b/frontend/src-tauri/src/commands.rs index 4be42a13..e84261d3 100644 --- a/frontend/src-tauri/src/commands.rs +++ b/frontend/src-tauri/src/commands.rs @@ -28,7 +28,7 @@ pub fn path_authorization_dir(app: &tauri::AppHandle) -> P } fn validate_host_path(kind: &str, raw: &str) -> Result { - if !matches!(kind, "models_dir" | "ffmpeg" | "ffprobe") { + if !matches!(kind, "models_dir" | "ffmpeg" | "ffprobe" | "dub_export") { return Err("Unsupported host-path capability".into()); } if raw.chars().any(|c| c.is_control()) { @@ -46,6 +46,13 @@ fn validate_host_path(kind: &str, raw: &str) -> Result { let probe = path.join(".voicestudio-write-test"); fs::write(&probe, b"ok").map_err(|e| format!("Directory is not writable: {e}"))?; let _ = fs::remove_file(probe); + } else if kind == "dub_export" { + let parent = path + .parent() + .ok_or_else(|| "Save destination must have a parent directory".to_string())?; + if !parent.is_dir() { + return Err("Save destination directory does not exist".into()); + } } else { if !path.is_file() { return Err("Selected media tool is not a file".into()); @@ -116,6 +123,22 @@ mod host_path_authorization_tests { fn empty_models_path_is_the_authorized_default_reset() { assert_eq!(validate_host_path("models_dir", "").unwrap(), PathBuf::new()); } + + #[test] + fn dub_export_accepts_only_absolute_paths_in_existing_directories() { + let parent = std::env::temp_dir(); + let destination = parent.join("voicestudio-authorized-export.wav"); + assert_eq!( + validate_host_path("dub_export", destination.to_str().unwrap()).unwrap(), + destination, + ); + assert!(validate_host_path("dub_export", "relative/export.wav").is_err()); + assert!(validate_host_path( + "dub_export", + parent.join("missing-directory/export.wav").to_str().unwrap(), + ) + .is_err()); + } } // ── System metrics ──────────────────────────────────────────────────────── diff --git a/frontend/src/App.jsx b/frontend/src/App.jsx index 2b6b58fe..5a3c5afc 100644 --- a/frontend/src/App.jsx +++ b/frontend/src/App.jsx @@ -839,7 +839,7 @@ function App() { toast.error(i18n.t('app.toast_open_folder_failed', { message: err.message })); } }; - // Save a dynamic (save_path-aware) export — dub video/audio/subtitles — to + // Save a dynamic export — dub video/audio/subtitles — to // disk. The parity-safe dialog + server-side copy vs browser-blob branch now // lives in the shared `downloadMedia` util (#1218) so audiobook/story exports // reuse the exact same path and never fall back to a webview-hijacking diff --git a/frontend/src/utils/mediaDownload.js b/frontend/src/utils/mediaDownload.js index 09d6cca8..93be69be 100644 --- a/frontend/src/utils/mediaDownload.js +++ b/frontend/src/utils/mediaDownload.js @@ -22,8 +22,8 @@ // to the destination. The /audio mount is a StaticFiles mount with no // ?save_path= support, so this is the only server-side copy that works // for those files. -// (b) no `sourceFilename` → a dynamic, save_path-aware endpoint (dub exports): -// append ?save_path= and let that endpoint render + copy, returning JSON. +// (b) no `sourceFilename` → a dynamic dub endpoint: Tauri authorizes the +// chosen path once and only the capability token crosses loopback HTTP. // Subtitles (srt/vtt) are small text bodies fetched raw and written via the // trusted `save_text_file` command — the backend never handles their dest // path (#309). @@ -50,7 +50,7 @@ function guessMode(ext) { * as the App's own export flow. Never creates an ``. * * @param {string} url HTTP URL of the file (also the source for the - * browser blob download + dynamic save_path copy). + * browser blob download + authorized dynamic copy). * @param {string} fallbackName Suggested filename in the save dialog / for the * download. * @param {object} [opts] @@ -70,7 +70,7 @@ export async function downloadMedia(url, fallbackName, opts = {}) { const modeGuess = guessMode(extGuess); // exportRecord writes the history row for paths the backend didn't already - // record (browser download, save_path, subtitle). Non-fatal: a failed record + // record (browser download, authorized dub save, subtitle). Non-fatal: a failed record // must not turn a successful save into an error toast. const recordHistory = async (filename, destinationPath) => { try { @@ -120,11 +120,17 @@ export async function downloadMedia(url, fallbackName, opts = {}) { return; } - // (c) Dynamic save_path-aware endpoint (dub exports): the endpoint copies - // to destPath and returns a JSON envelope. Guard the content-type so a + // (c) Dynamic dub endpoint: bind the native picker result to a one-shot + // capability. The host path never enters an HTTP query or body. Guard the content-type so a // raw-body response surfaces a clear error, not a JSON.parse crash (#309). - const sep = url.includes('?') ? '&' : '?'; - const res = await apiFetch(`${url}${sep}save_path=${encodeURIComponent(destPath)}`); + const { invoke } = await import('@tauri-apps/api/core'); + const authorization = await invoke('authorize_host_path', { + kind: 'dub_export', + path: destPath, + }); + const res = await apiFetch(url, { + headers: { 'X-VoiceStudio-Path-Authorization': authorization }, + }); if (!res.ok) throw new Error(`HTTP ${res.status}`); // a 4xx/5xx isn't a successful save const ctype = res.headers.get('content-type') || ''; if (!ctype.includes('application/json')) { diff --git a/frontend/src/utils/mediaDownload.test.js b/frontend/src/utils/mediaDownload.test.js index b493a3fa..cb27a8e7 100644 --- a/frontend/src/utils/mediaDownload.test.js +++ b/frontend/src/utils/mediaDownload.test.js @@ -97,7 +97,7 @@ describe('downloadMedia — Tauri branch (isTauri=true)', () => { expect(toast.error).not.toHaveBeenCalled(); }); - it('dynamic endpoint (no sourceFilename): appends ?save_path= and records history', async () => { + it('dynamic endpoint uses a one-shot native path authorization and records history', async () => { const downloadMedia = await loadDownloadMedia({ tauri: true }); save.mockResolvedValueOnce('/Users/me/Movies/dubbed_video.mp4'); apiFetch.mockResolvedValueOnce({ @@ -108,15 +108,22 @@ describe('downloadMedia — Tauri branch (isTauri=true)', () => { display_name: 'dubbed_video.mp4', }), }); + invoke.mockResolvedValueOnce('a'.repeat(64)); await downloadMedia( 'http://x/dub/download/job/dubbed_video.mp4?preserve_bg=1', 'dubbed_video.mp4', ); - const fetchedUrl = apiFetch.mock.calls[0][0]; - expect(fetchedUrl).toContain('save_path='); - expect(fetchedUrl).toContain(encodeURIComponent('/Users/me/Movies/dubbed_video.mp4')); + expect(invoke).toHaveBeenCalledWith('authorize_host_path', { + kind: 'dub_export', + path: '/Users/me/Movies/dubbed_video.mp4', + }); + expect(apiFetch).toHaveBeenCalledWith( + 'http://x/dub/download/job/dubbed_video.mp4?preserve_bg=1', + { headers: { 'X-VoiceStudio-Path-Authorization': 'a'.repeat(64) } }, + ); + expect(apiFetch.mock.calls[0][0]).not.toContain('save_path='); expect(exportAction).not.toHaveBeenCalled(); // dynamic endpoint copies itself expect(exportRecord).toHaveBeenCalledWith( expect.objectContaining({ filename: 'dubbed_video.mp4', mode: 'video' }), diff --git a/tests/test_filesystem_boundaries.py b/tests/test_filesystem_boundaries.py index 1684927c..14ab3a37 100644 --- a/tests/test_filesystem_boundaries.py +++ b/tests/test_filesystem_boundaries.py @@ -3,14 +3,15 @@ from __future__ import annotations import asyncio +import inspect +import json from contextlib import contextmanager from types import SimpleNamespace import pytest -from fastapi import HTTPException - from api.dependencies import require_native_access from core.path_security import UnsafePath, resolve_within, safe_filename +from fastapi import HTTPException def _request(host: str | None): @@ -140,11 +141,73 @@ def test_dub_artifact_rejects_db_path_and_symlink_escapes(tmp_path, monkeypatch) assert exc.value.status_code == 400 -def test_native_save_guard_runs_only_for_host_path_mode(monkeypatch): +def test_dub_artifact_rebases_trusted_path_after_data_relocation(tmp_path, monkeypatch): from api.routers import dub_export - remote = _request("10.0.0.8") - dub_export._guard_native_save(remote, "") + current = tmp_path / "new-data" / "dub_jobs" + artifact = current / "job_123" / "tracks" / "voice.wav" + artifact.parent.mkdir(parents=True) + artifact.write_bytes(b"voice") + monkeypatch.setattr(dub_export, "DUB_DIR", str(current)) + + old_posix = tmp_path / "old-data" / "dub_jobs" / "job_123" / "tracks" / "voice.wav" + old_windows = r"D:\Old VoiceStudio\dub_jobs\job_123\tracks\voice.wav" + assert dub_export._dub_artifact(old_posix) == str(artifact.resolve()) + assert dub_export._dub_artifact(old_windows) == str(artifact.resolve()) + + +def test_dub_artifact_rebase_rejects_unanchored_traversal_and_symlink(tmp_path, monkeypatch): + from api.routers import dub_export + + current = tmp_path / "new-data" / "dub_jobs" + outside = tmp_path / "outside" + current.mkdir(parents=True) + outside.mkdir() + (outside / "secret.wav").write_bytes(b"secret") + (current / "job_123").symlink_to(outside, target_is_directory=True) + monkeypatch.setattr(dub_export, "DUB_DIR", str(current)) + + rejected = [ + tmp_path / "old-data" / "other" / "job_123" / "secret.wav", + str(tmp_path / "old-data" / "dub_jobs" / ".." / "secret.wav"), + tmp_path / "old-data" / "dub_jobs" / "job_123" / "secret.wav", + r"D:\old\dub_jobs\..\secret.wav", + ] + for value in rejected: + with pytest.raises(HTTPException) as exc: + dub_export._dub_artifact(value) + assert exc.value.status_code == 400 + + +def test_dub_native_save_requires_one_shot_tauri_authorization(tmp_path, monkeypatch): + from api.routers import dub_export + from core import path_authorization + + auth_dir = tmp_path / "authorizations" + auth_dir.mkdir() + monkeypatch.setattr(path_authorization, "_AUTH_DIR", str(auth_dir)) + token = "a" * 64 + destination = str(tmp_path / "export.wav") + (auth_dir / f"{token}.json").write_text( + json.dumps({"token": token, "kind": "dub_export", "path": destination}), + encoding="utf-8", + ) + + assert dub_export._consume_native_save("") is None + assert dub_export._consume_native_save(token) == destination with pytest.raises(HTTPException) as exc: - dub_export._guard_native_save(remote, "/tmp/export.wav") + dub_export._consume_native_save(token) assert exc.value.status_code == 403 + + +def test_dub_routes_never_accept_an_http_destination_path(): + from api.routers import dub_export + + for route in ( + dub_export.dub_download, + dub_export.dub_download_audio, + dub_export.dub_download_mp3, + ): + parameters = inspect.signature(route).parameters + assert "save_path" not in parameters + assert "save_authorization" in parameters From 511d21a8b9a264992f9814c1ba1c6b536cb1d3ca Mon Sep 17 00:00:00 2001 From: debpalash <4178343+debpalash@users.noreply.github.com> Date: Sun, 9 Aug 2026 21:42:46 +0000 Subject: [PATCH 12/22] fix: report desktop log cleanup failure --- backend/api/routers/system.py | 7 ++- .../src/test/LogsFooterClearFailure.test.jsx | 56 +++++++++++++++++++ tests/test_destructive_cleanup.py | 7 ++- 3 files changed, 66 insertions(+), 4 deletions(-) create mode 100644 frontend/src/test/LogsFooterClearFailure.test.jsx diff --git a/backend/api/routers/system.py b/backend/api/routers/system.py index 0666e1f1..fcaec4ab 100644 --- a/backend/api/routers/system.py +++ b/backend/api/routers/system.py @@ -476,7 +476,12 @@ async def clear_tauri_logs(): cleared.append(p) except OSError: failed += 1 - return {"cleared": cleared, "failed": failed} + if failed: + raise HTTPException( + status_code=500, + detail="One or more desktop log files could not be cleared. Close any app using them and retry.", + ) + return {"cleared": cleared, "failed": 0} @router.get("/sysinfo", response_model=SysinfoResponse) def get_sys_info(): diff --git a/frontend/src/test/LogsFooterClearFailure.test.jsx b/frontend/src/test/LogsFooterClearFailure.test.jsx new file mode 100644 index 00000000..68f09a2d --- /dev/null +++ b/frontend/src/test/LogsFooterClearFailure.test.jsx @@ -0,0 +1,56 @@ +import React from 'react'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { fireEvent, render, screen, waitFor } from '@testing-library/react'; +import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; + +const { clearTauriLogs, toastError, toastSuccess } = vi.hoisted(() => ({ + clearTauriLogs: vi.fn(), + toastError: vi.fn(), + toastSuccess: vi.fn(), +})); + +vi.mock('../api/system', () => ({ + clearSystemLogs: vi.fn(), + clearTauriLogs, +})); +vi.mock('../api/hooks', () => ({ + useSystemLogs: () => ({ data: null, refetch: vi.fn() }), + useTauriLogs: () => ({ data: null, refetch: vi.fn() }), + useVisibleNotifications: () => ({ notifications: [] }), + isDismissibleNotification: () => false, +})); +vi.mock('../components/NetworkToggle', () => ({ default: () => null })); +vi.mock('react-hot-toast', () => ({ + default: Object.assign(vi.fn(), { error: toastError, success: toastSuccess }), +})); + +import LogsFooter from '../components/LogsFooter'; + +function renderFooter() { + return render( + + + , + ); +} + +describe('LogsFooter Tauri cleanup failure', () => { + beforeEach(() => { + localStorage.clear(); + localStorage.setItem('omnivoice.logs.active', 'tauri'); + clearTauriLogs.mockReset(); + toastError.mockReset(); + toastSuccess.mockReset(); + }); + + it('shows failure and never claims the log was cleared', async () => { + clearTauriLogs.mockRejectedValueOnce(new Error('desktop log is locked')); + renderFooter(); + + fireEvent.click(screen.getByRole('button', { name: /expand logs panel/i })); + fireEvent.click(screen.getByRole('button', { name: /clear log/i })); + + await waitFor(() => expect(toastError).toHaveBeenCalled()); + expect(toastSuccess).not.toHaveBeenCalled(); + }); +}); diff --git a/tests/test_destructive_cleanup.py b/tests/test_destructive_cleanup.py index 36a2f012..c54cd6ae 100644 --- a/tests/test_destructive_cleanup.py +++ b/tests/test_destructive_cleanup.py @@ -104,7 +104,8 @@ async def test_tauri_log_clear_reports_truncate_failure(monkeypatch, tmp_path): lambda _path: (_ for _ in ()).throw(PermissionError("locked")), ) - result = await system.clear_tauri_logs() + with pytest.raises(HTTPException) as caught: + await system.clear_tauri_logs() - assert result == {"cleared": [], "failed": 1} - assert str(log) not in str(result) + assert caught.value.status_code == 500 + assert str(log) not in caught.value.detail From 7a87e5b5cd45b414a9a2642d926b3df9a2e45841 Mon Sep 17 00:00:00 2001 From: debpalash <4178343+debpalash@users.noreply.github.com> Date: Mon, 10 Aug 2026 04:34:52 +0000 Subject: [PATCH 13/22] fix: keep cleanup logs injection-safe --- backend/api/routers/gallery.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/backend/api/routers/gallery.py b/backend/api/routers/gallery.py index f5990f0f..217914a6 100644 --- a/backend/api/routers/gallery.py +++ b/backend/api/routers/gallery.py @@ -491,8 +491,8 @@ def batch_delete_voices(body: dict): if audio_path: try: unlink_if_present(audio_path) - except FileCleanupError as exc: - logger.warning("Voice audio cleanup failed for gallery item %s: %s", vid, exc) + except FileCleanupError: + logger.warning("Voice audio cleanup failed for a gallery item") failed += 1 continue conn.execute("DELETE FROM voice_gallery WHERE id = ?", (vid,)) From c4f9e787df0188e767b93b366b48ca3724a93042 Mon Sep 17 00:00:00 2001 From: debpalash <4178343+debpalash@users.noreply.github.com> Date: Mon, 10 Aug 2026 04:37:45 +0000 Subject: [PATCH 14/22] fix(security): bind native paths to picker capabilities --- frontend/src/components/MediaEngineCard.jsx | 38 +++---------- .../src/components/MediaEngineCard.test.jsx | 16 ++++-- .../components/settings/AudioToolsPanel.jsx | 55 +------------------ .../settings/AudioToolsPanel.test.jsx | 3 - frontend/src/utils/mediaDownload.js | 1 - tests/test_smart_fit_export.py | 4 +- 6 files changed, 25 insertions(+), 92 deletions(-) diff --git a/frontend/src/components/MediaEngineCard.jsx b/frontend/src/components/MediaEngineCard.jsx index c64444b4..b1452e65 100644 --- a/frontend/src/components/MediaEngineCard.jsx +++ b/frontend/src/components/MediaEngineCard.jsx @@ -19,8 +19,6 @@ export default function MediaEngineCard() { const { t } = useTranslation(); const [status, setStatus] = useState(null); const [detectError, setDetectError] = useState(null); - const [customPath, setCustomPath] = useState(''); - const [showPathInput, setShowPathInput] = useState(false); const [busy, setBusy] = useState(false); const refresh = useCallback(async () => { @@ -81,17 +79,18 @@ export default function MediaEngineCard() { const chooseFile = async () => { try { if ('__TAURI_INTERNALS__' in window) { - const { open } = await import('@tauri-apps/plugin-dialog'); - const picked = await open({ multiple: false, directory: false, title: 'FFmpeg' }); - if (typeof picked === 'string') { - await post('/media-tools/ffmpeg/custom-path', { path: picked }); + const { invoke } = await import('@tauri-apps/api/core'); + const selection = await invoke('authorize_host_path', { kind: 'ffmpeg' }); + if (selection) { + await post('/media-tools/ffmpeg/custom-path', { + authorization: selection.authorization, + }); return; } } - } catch { - /* picker unavailable — fall through to the inline input */ + } catch (e) { + setDetectError(e?.message || String(e)); } - setShowPathInput(true); }; if (!status || status.ready) return null; // the ideal outcome: nothing. @@ -154,27 +153,6 @@ export default function MediaEngineCard() { - {showPathInput && ( - <> - setCustomPath(e.target.value)} - placeholder="/usr/bin/ffmpeg" - className="min-w-[220px] flex-1 rounded border border-border bg-transparent px-2 py-1 font-mono text-xs text-fg" - aria-label={t('settings.ffmpeg_input_aria', { defaultValue: 'FFmpeg path' })} - data-testid="media-engine-path" - /> - - - )} ); diff --git a/frontend/src/components/MediaEngineCard.test.jsx b/frontend/src/components/MediaEngineCard.test.jsx index 998b07ef..5a584cf1 100644 --- a/frontend/src/components/MediaEngineCard.test.jsx +++ b/frontend/src/components/MediaEngineCard.test.jsx @@ -6,6 +6,8 @@ vi.mock('../api/client', () => ({ apiJson: vi.fn(), apiFetch: vi.fn(), })); +const invoke = vi.fn(); +vi.mock('@tauri-apps/api/core', () => ({ invoke: (...args) => invoke(...args) })); import { apiJson, apiFetch } from '../api/client'; import MediaEngineCard from './MediaEngineCard'; @@ -19,6 +21,7 @@ const statusWith = (ready, acquire) => ({ describe('MediaEngineCard — invisible-by-default media engine', () => { beforeEach(() => { vi.clearAllMocks(); + delete window.__TAURI_INTERNALS__; apiFetch.mockResolvedValue({ ok: true, json: async () => ({}) }); }); @@ -83,18 +86,21 @@ describe('MediaEngineCard — invisible-by-default media engine', () => { ); }); - it('Choose file… falls back to an inline path input outside Tauri and saves it', async () => { + it('Choose file uses only a native picker authorization', async () => { apiJson.mockResolvedValue(statusWith(false, { state: 'error', error: 'boom' })); + window.__TAURI_INTERNALS__ = {}; + invoke.mockResolvedValue({ authorization: 'e'.repeat(64), path: '/usr/local/bin/ffmpeg' }); render(); fireEvent.click(await screen.findByText('Choose file…')); - const input = await screen.findByTestId('media-engine-path'); - fireEvent.change(input, { target: { value: '/usr/local/bin/ffmpeg' } }); - fireEvent.click(screen.getByText('Save')); + await waitFor(() => + expect(invoke).toHaveBeenCalledWith('authorize_host_path', { kind: 'ffmpeg' }), + ); await waitFor(() => expect(apiFetch).toHaveBeenCalledWith( '/media-tools/ffmpeg/custom-path', - expect.objectContaining({ body: JSON.stringify({ path: '/usr/local/bin/ffmpeg' }) }), + expect.objectContaining({ body: JSON.stringify({ authorization: 'e'.repeat(64) }) }), ), ); + expect(apiFetch.mock.calls.flat().join(' ')).not.toContain('/usr/local/bin/ffmpeg'); }); }); diff --git a/frontend/src/components/settings/AudioToolsPanel.jsx b/frontend/src/components/settings/AudioToolsPanel.jsx index e49ebf24..e58694ce 100644 --- a/frontend/src/components/settings/AudioToolsPanel.jsx +++ b/frontend/src/components/settings/AudioToolsPanel.jsx @@ -5,7 +5,7 @@ * One row per tool: * • FFmpeg / FFprobe — version + origin badge (Bundled / System / Custom / * App package) + path; actions: Use system copy (auto-detect), - * Choose file… (picker in Tauri, inline path input everywhere), + * Choose file… (native picker in Tauri), * Restore bundled (always-safe revert). The section header carries * "Update bundled build" (one download covers both binaries). * • yt-dlp — module version + Update (fetches the newest wheel into an @@ -19,9 +19,8 @@ import { toast } from 'react-hot-toast'; import { useTranslation } from 'react-i18next'; import { AudioLines, Film, ScanSearch, DownloadCloud } from 'lucide-react'; import { Button, Badge } from '../../ui'; -import { SettingsSection, SettingRow, SettingsInput } from './primitives'; +import { SettingsSection, SettingRow } from './primitives'; import RestartBadge from './RestartBadge'; -import { isTauri } from './native'; const ORIGIN_TONE = { bundled: 'success', @@ -46,33 +45,11 @@ function OriginBadge({ origin }) { ); } -/** Open the OS file picker in Tauri; return the chosen path or null. */ -async function pickBinary(title) { - if (!isTauri()) return null; - try { - const { open } = await import('@tauri-apps/plugin-dialog'); - const picked = await open({ multiple: false, directory: false, title }); - return typeof picked === 'string' ? picked : null; - } catch { - return null; - } -} - function BinaryRow({ tool, info, onAction, busy }) { const { t } = useTranslation(); - const [path, setPath] = useState(''); - const [showInput, setShowInput] = useState(false); const label = tool === 'ffmpeg' ? 'FFmpeg' : 'FFprobe'; - const chooseFile = async () => { - const picked = await pickBinary(label); - if (picked) { - onAction(`/media-tools/${tool}/custom-path`, { path: picked }); - } else { - // Web preview / picker unavailable — fall back to the inline input. - setShowInput(true); - } - }; + const chooseFile = () => onAction(`/media-tools/${tool}/custom-path`); return ( {t('settings.audio_tools_restore', { defaultValue: 'Restore bundled' })} - {showInput && ( - <> - setPath(e.target.value)} - onKeyDown={(e) => - e.key === 'Enter' && - path.trim() && - onAction(`/media-tools/${tool}/custom-path`, { path: path.trim() }) - } - aria-label={t('settings.audio_tools_path_input_aria', { - tool: label, - defaultValue: '{{tool}} binary path', - })} - /> - - - )} } /> diff --git a/frontend/src/components/settings/AudioToolsPanel.test.jsx b/frontend/src/components/settings/AudioToolsPanel.test.jsx index b447dc93..e72d3393 100644 --- a/frontend/src/components/settings/AudioToolsPanel.test.jsx +++ b/frontend/src/components/settings/AudioToolsPanel.test.jsx @@ -91,9 +91,6 @@ describe('AudioToolsPanel — power-user surface for the media tools', () => { it('sends only a native one-shot authorization for a custom executable', async () => { render(); fireEvent.click(await screen.findByLabelText('FFmpeg: Choose file…')); - const input = await screen.findByLabelText('FFmpeg binary path'); - fireEvent.change(input, { target: { value: '/opt/tools/ffmpeg' } }); - fireEvent.click(screen.getByRole('button', { name: 'Save' })); await waitFor(() => expect(invoke).toHaveBeenCalledWith('authorize_host_path', { diff --git a/frontend/src/utils/mediaDownload.js b/frontend/src/utils/mediaDownload.js index fb7ec6c6..da2de479 100644 --- a/frontend/src/utils/mediaDownload.js +++ b/frontend/src/utils/mediaDownload.js @@ -146,7 +146,6 @@ export async function downloadMedia(url, fallbackName, opts = {}) { await recordHistory(fallbackName, destPath); return; } - } catch (err) { console.error(err); toast.error(i18n.t('app.toast_save_error', { message: err.message }), { id: fallbackName }); diff --git a/tests/test_smart_fit_export.py b/tests/test_smart_fit_export.py index b7eb2a80..a1d87568 100644 --- a/tests/test_smart_fit_export.py +++ b/tests/test_smart_fit_export.py @@ -515,8 +515,10 @@ def _seed_retime_job(dc, tmp_path: Path, video: Path, track_dur: float) -> str: job_dir.mkdir(parents=True, exist_ok=True) track_wav = job_dir / "dubbed_de.wav" _make_sine_wav(track_wav, track_dur) + local_video = job_dir / "source.mp4" + local_video.write_bytes(video.read_bytes()) job = _smart_fit_job(track_dur=track_dur) - job["video_path"] = str(video) + job["video_path"] = str(local_video) job["dubbed_tracks"]["de"]["path"] = str(track_wav) dc._dub_jobs[job_id] = job return job_id From f70199db3262c479431f37a08d8dce3d65b04ea6 Mon Sep 17 00:00:00 2001 From: debpalash <4178343+debpalash@users.noreply.github.com> Date: Mon, 10 Aug 2026 04:56:42 +0000 Subject: [PATCH 15/22] fix(security): make path containment explicit to analysis --- backend/api/routers/tools.py | 1 - backend/core/path_security.py | 24 ++++++++++++++++++++---- 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/backend/api/routers/tools.py b/backend/api/routers/tools.py index b410ac81..6f706b42 100644 --- a/backend/api/routers/tools.py +++ b/backend/api/routers/tools.py @@ -185,7 +185,6 @@ async def analyse_video_context(job_id: str): raise HTTPException(status_code=400, detail="Invalid job id") from exc job = _get_job(job_id) if not job: - from fastapi import HTTPException raise HTTPException(status_code=404, detail="Job not found") video_path = resolve_within(DUB_DIR, job_dir / "source.mp4") diff --git a/backend/core/path_security.py b/backend/core/path_security.py index 8e349d01..b821cbe1 100644 --- a/backend/core/path_security.py +++ b/backend/core/path_security.py @@ -41,7 +41,7 @@ def resolve_within(root: os.PathLike[str] | str, value: os.PathLike[str] | str) mixture of relative filenames and absolute job-artifact paths. """ raw = os.fspath(value) if value is not None else "" - if not raw: + if not isinstance(raw, str) or not raw: raise UnsafePath("path is empty") # Treat both separator families as structural on every host. Otherwise a # Windows traversal string is an innocent-looking filename when validated @@ -49,9 +49,25 @@ def resolve_within(root: os.PathLike[str] | str, value: os.PathLike[str] | str) if os.sep != "\\" and ("\\" in raw or bool(ntpath.splitdrive(raw)[0])): raise UnsafePath("path uses a foreign separator or drive") root_path = Path(root).expanduser().resolve(strict=False) - candidate = Path(raw).expanduser() - if not candidate.is_absolute(): - candidate = root_path / candidate + root_text = str(root_path) + if os.path.isabs(raw): + prefix = root_text.rstrip(os.sep) + os.sep + if not os.path.normcase(raw).startswith(os.path.normcase(prefix)): + raise UnsafePath("path escapes its allowed root") + raw = raw[len(prefix):] + + # Rebuild from individually sanitized basenames. Besides making the + # containment proof explicit to static analysis, this rejects empty, + # dot, parent, drive, and separator-bearing components before Path sees + # any persisted/request-derived string. + parts = raw.split(os.sep) + clean_parts: list[str] = [] + for part in parts: + clean = os.path.basename(part) + if not clean or clean in {".", ".."} or clean != part: + raise UnsafePath("path contains an unsafe component") + clean_parts.append(clean) + candidate = root_path.joinpath(*clean_parts) resolved = candidate.resolve(strict=False) try: if os.path.commonpath((str(root_path), str(resolved))) != str(root_path): From e8eff3c6a043fb47fe331ea9b0b47ee7e06e95db Mon Sep 17 00:00:00 2001 From: debpalash <4178343+debpalash@users.noreply.github.com> Date: Mon, 10 Aug 2026 05:01:26 +0000 Subject: [PATCH 16/22] fix(security): bound native tool authorization --- frontend/src-tauri/src/commands.rs | 28 +++++++++++++++++-- frontend/src/components/MediaEngineCard.jsx | 10 +++++-- .../src/components/MediaEngineCard.test.jsx | 7 +++++ .../components/settings/AudioToolsPanel.jsx | 25 ++++++++++------- .../settings/AudioToolsPanel.test.jsx | 9 ++++++ 5 files changed, 63 insertions(+), 16 deletions(-) diff --git a/frontend/src-tauri/src/commands.rs b/frontend/src-tauri/src/commands.rs index 779dc154..06610015 100644 --- a/frontend/src-tauri/src/commands.rs +++ b/frontend/src-tauri/src/commands.rs @@ -3,7 +3,7 @@ use std::fs; use std::path::{Path, PathBuf}; use std::sync::atomic::Ordering; -use std::time::Duration; +use std::time::{Duration, Instant}; use serde::{Deserialize, Serialize}; use tauri::image::Image; @@ -75,14 +75,36 @@ fn validate_host_path(kind: &str, path: PathBuf) -> Result { if !path.is_file() { return Err("Selected media tool is not a file".into()); } - let output = crate::tools::no_window( + let mut child = crate::tools::no_window( std::process::Command::new(&path) .arg("-version") .stdout(std::process::Stdio::piped()) .stderr(std::process::Stdio::piped()), ) - .output() + .spawn() .map_err(|e| format!("Selected media tool could not run: {e}"))?; + let deadline = Instant::now() + Duration::from_secs(5); + loop { + match child.try_wait() { + Ok(Some(_)) => break, + Ok(None) if Instant::now() < deadline => { + std::thread::sleep(Duration::from_millis(25)); + } + Ok(None) => { + let _ = child.kill(); + let _ = child.wait(); + return Err("Selected media tool did not respond within 5 seconds".into()); + } + Err(e) => { + let _ = child.kill(); + let _ = child.wait(); + return Err(format!("Selected media tool could not be checked: {e}")); + } + } + } + let output = child + .wait_with_output() + .map_err(|e| format!("Selected media tool output could not be read: {e}"))?; let version_text = format!( "{}{}", String::from_utf8_lossy(&output.stdout), diff --git a/frontend/src/components/MediaEngineCard.jsx b/frontend/src/components/MediaEngineCard.jsx index b1452e65..eaf23a68 100644 --- a/frontend/src/components/MediaEngineCard.jsx +++ b/frontend/src/components/MediaEngineCard.jsx @@ -93,6 +93,8 @@ export default function MediaEngineCard() { } }; + const isDesktop = '__TAURI_INTERNALS__' in window; + if (!status || status.ready) return null; // the ideal outcome: nothing. const op = status.ops?.acquire || {}; @@ -150,9 +152,11 @@ export default function MediaEngineCard() { > {t('setup.media_engine_use_system', { defaultValue: 'Use a system copy' })} - + {isDesktop && ( + + )} ); diff --git a/frontend/src/components/MediaEngineCard.test.jsx b/frontend/src/components/MediaEngineCard.test.jsx index 5a584cf1..1946a868 100644 --- a/frontend/src/components/MediaEngineCard.test.jsx +++ b/frontend/src/components/MediaEngineCard.test.jsx @@ -103,4 +103,11 @@ describe('MediaEngineCard — invisible-by-default media engine', () => { ); expect(apiFetch.mock.calls.flat().join(' ')).not.toContain('/usr/local/bin/ffmpeg'); }); + + it('does not offer native file selection in a browser', async () => { + apiJson.mockResolvedValue(statusWith(false, { state: 'error', error: 'boom' })); + render(); + await screen.findByTestId('media-engine-card'); + expect(screen.queryByText('Choose file…')).not.toBeInTheDocument(); + }); }); diff --git a/frontend/src/components/settings/AudioToolsPanel.jsx b/frontend/src/components/settings/AudioToolsPanel.jsx index e58694ce..4369b438 100644 --- a/frontend/src/components/settings/AudioToolsPanel.jsx +++ b/frontend/src/components/settings/AudioToolsPanel.jsx @@ -90,15 +90,17 @@ function BinaryRow({ tool, info, onAction, busy }) { > {t('settings.audio_tools_use_system', { defaultValue: 'Use system copy' })} - + {'__TAURI_INTERNALS__' in window && ( + + )}