fix: production-readiness hardening — security, perf, a11y, observability
Lint & Test / test (push) Successful in 20s
Lint & Test / test (push) Successful in 20s
Security - Default scripts_management, callbacks_management, links_management, and media_folders_management to False so a leaked token cannot escalate to RCE through admin CRUD endpoints. - TokenSpec + scope hierarchy (read | control | admin); legacy bare-string api_tokens entries promote to admin for back-compat. Management endpoints now require admin scope. - WebSocket subprotocol auth (Sec-WebSocket-Protocol: media-server.token.<T>) preferred over ?token= query so the token no longer lands in URL/history/ Referer; query fallback retained for HA integration back-compat. - Origin allow-list check on the WS endpoint (CSWSH defence). - In-process token-bucket rate limiter: 5/min for failed auths, 10/min for /api/scripts/execute and /api/callbacks/execute. - shell=False subprocess path (shlex.split) + per-parameter regex `pattern` in ScriptParameterConfig to harden shell=true scripts against parameter injection (Windows cmd.exe env-var expansion). - CSP gains form-action, worker-src, manifest-src directives. - Refuse cors_origins=["*"] at startup; strip token=... from uvicorn access logs; validate Gitea release tag against strict SemVer regex. - noopener noreferrer + no-referrer referrerpolicy on every outbound link. - icacls hardening of config.yaml on Windows (current user + SYSTEM + Administrators only); 0600 still enforced on POSIX. - WS volume handler clamps input and never drops the socket on bad messages. Performance - Album-art read in windows_media gated by track key — was decoding the WinRT thumbnail twice per second regardless of track changes. - /api/media/artwork returns content-derived ETag + Cache-Control so the browser sends If-None-Match and gets 304s on track repeats. - Foreground-service ctypes argtypes hoisted to one-time module init (was re-declaring ~14 prototypes per probe). - display_service _static_cache keyed by (edid_hash, ...) tuple with eviction of disappeared monitors — fixes stale capabilities on hot-plug swaps where the new topology has the same monitor count. - Visualizer rAF loop paused on document.hidden, resumed on visible. Reliability / bug fixes - Lifespan rewritten as try/yield/finally so a partial-startup failure cannot orphan background tasks or executors. - _run_callback in routes/media.py keeps a strong task ref (GC-safe) and uses the dedicated callback executor instead of the default pool. - macos_media.set_volume() no longer always returns True. - TrayManager._restart_requested initialised in __init__; set before signalling exit so the main thread observes it correctly. - Missing static_dir now logs a WARNING instead of silent UI disable. UX / accessibility / PWA - manifest.json theme_color and background_color match the Studio Reference base (#0E0D0B); added id and scope for PWA installability. - ARIA on mini-player icon buttons; inner SVGs marked aria-hidden. - OS mediaSession API wired so headset / lockscreen / Bluetooth buttons drive play/pause/next/prev/seek and show track metadata + artwork. Observability - X-Request-ID middleware (accept upstream id if it matches a safe regex, otherwise UUID4); request_id_var added to ContextVars and included in every log line alongside the token label. - Audit log (append-only JSONL) for every script + callback execution, including the on_play/on_pause/etc. event callbacks. Background-thread writer; queue capped; flushed in lifespan teardown. Deployment - proxy_headers + forwarded_allow_ips plumbed through Settings → uvicorn.Config for reverse-proxy installs. - HTTPS support via ssl_certfile + ssl_keyfile (+ optional password); startup refuses to launch with only one of the pair set. - Thumbnail cache moved from project-root .cache to %LOCALAPPDATA%/media-server/cache (Windows) and $XDG_CACHE_HOME/media-server/thumbnails (POSIX). Tests - 35 new tests across auth scopes, rate limiter, browser path traversal (../ NUL UNC absolute), script-param validation incl. regex, Gitea tag whitelist, config atomic write + POSIX perms. 47 passed / 4 skipped.
This commit is contained in:
@@ -0,0 +1,58 @@
|
||||
"""Tests for token scope hierarchy + back-compat with legacy bare-string tokens."""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import pytest
|
||||
|
||||
from media_server.config import Settings, TokenSpec
|
||||
|
||||
|
||||
def test_bare_string_token_promotes_to_admin_scope():
|
||||
"""Legacy `label: <token>` form must still work and grant admin."""
|
||||
s = Settings(api_tokens={"legacy": "deadbeef-deadbeef-deadbeef-deadbeef"})
|
||||
spec = s.api_tokens["legacy"]
|
||||
assert isinstance(spec, TokenSpec)
|
||||
assert spec.token == "deadbeef-deadbeef-deadbeef-deadbeef"
|
||||
assert spec.scopes == ["admin"]
|
||||
assert spec.grants("admin")
|
||||
assert spec.grants("control")
|
||||
assert spec.grants("read")
|
||||
|
||||
|
||||
def test_dict_token_with_explicit_scopes():
|
||||
s = Settings(api_tokens={
|
||||
"ha": {"token": "aaaaaaaaaaaaaaaa", "scopes": ["read", "control"]},
|
||||
})
|
||||
spec = s.api_tokens["ha"]
|
||||
assert spec.grants("control")
|
||||
assert spec.grants("read")
|
||||
assert not spec.grants("admin")
|
||||
|
||||
|
||||
def test_read_only_scope_grants_only_read():
|
||||
spec = TokenSpec(token="xxxxxxxxxxxxxxxx", scopes=["read"])
|
||||
assert spec.grants("read")
|
||||
assert not spec.grants("control")
|
||||
assert not spec.grants("admin")
|
||||
|
||||
|
||||
def test_admin_scope_implies_control_and_read():
|
||||
spec = TokenSpec(token="xxxxxxxxxxxxxxxx", scopes=["admin"])
|
||||
assert spec.grants("read")
|
||||
assert spec.grants("control")
|
||||
assert spec.grants("admin")
|
||||
|
||||
|
||||
def test_unknown_scope_rejected():
|
||||
with pytest.raises(ValueError, match="unknown scopes"):
|
||||
TokenSpec(token="xxxxxxxxxxxxxxxx", scopes=["root"])
|
||||
|
||||
|
||||
def test_empty_scopes_rejected():
|
||||
with pytest.raises(ValueError, match="at least one"):
|
||||
TokenSpec(token="xxxxxxxxxxxxxxxx", scopes=[])
|
||||
|
||||
|
||||
def test_short_token_rejected():
|
||||
with pytest.raises(ValueError):
|
||||
TokenSpec(token="short", scopes=["read"])
|
||||
@@ -0,0 +1,84 @@
|
||||
"""Path traversal defence for BrowserService.validate_path.
|
||||
|
||||
The browser endpoint is the single most security-critical filesystem entry
|
||||
point in the app: it serves file contents and folder listings to the WebUI.
|
||||
A bypass here = arbitrary read of any file the server process can see.
|
||||
|
||||
The current implementation signals rejection by *raising* (ValueError for
|
||||
traversal/NUL/unknown folder, FileNotFoundError for non-existent absolute
|
||||
paths). Either rejection mode is acceptable — these tests assert that the
|
||||
adversarial input never returns a path *inside* the configured base.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import tempfile
|
||||
from pathlib import Path
|
||||
from unittest.mock import patch
|
||||
|
||||
import pytest
|
||||
|
||||
from media_server.services.browser_service import BrowserService
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def tmp_media_folder():
|
||||
"""A real temp dir registered as a media folder for the test duration."""
|
||||
with tempfile.TemporaryDirectory() as tmp:
|
||||
base = Path(tmp).resolve()
|
||||
(base / "ok.mp3").write_bytes(b"id3")
|
||||
(base / "sub").mkdir()
|
||||
(base / "sub" / "nested.mp3").write_bytes(b"id3")
|
||||
|
||||
from media_server.config import MediaFolderConfig
|
||||
folders = {"test": MediaFolderConfig(path=str(base), label="Test", enabled=True)}
|
||||
with patch("media_server.services.browser_service.settings.media_folders", folders):
|
||||
yield base
|
||||
|
||||
|
||||
def _is_rejected(folder_id: str, rel: str) -> bool:
|
||||
"""Helper: True iff validate_path either raises or returns None."""
|
||||
try:
|
||||
result = BrowserService.validate_path(folder_id, rel)
|
||||
except (ValueError, FileNotFoundError, OSError):
|
||||
return True
|
||||
return result is None
|
||||
|
||||
|
||||
def test_validate_path_accepts_a_real_file(tmp_media_folder: Path):
|
||||
p = BrowserService.validate_path("test", "ok.mp3")
|
||||
assert p is not None
|
||||
assert p.is_file()
|
||||
# Defence-in-depth: resolved path must live inside the base.
|
||||
assert tmp_media_folder in p.resolve().parents or p.resolve().parent == tmp_media_folder
|
||||
|
||||
|
||||
def test_validate_path_accepts_nested(tmp_media_folder: Path):
|
||||
p = BrowserService.validate_path("test", "sub/nested.mp3")
|
||||
assert p is not None
|
||||
|
||||
|
||||
def test_unknown_folder_rejected(tmp_media_folder: Path):
|
||||
assert _is_rejected("ghost", "ok.mp3")
|
||||
|
||||
|
||||
def test_dotdot_traversal_rejected(tmp_media_folder: Path):
|
||||
assert _is_rejected("test", "../etc/passwd")
|
||||
assert _is_rejected("test", "..\\..\\Windows\\System32")
|
||||
assert _is_rejected("test", "sub/../../etc/passwd")
|
||||
|
||||
|
||||
def test_absolute_path_rejected(tmp_media_folder: Path):
|
||||
assert _is_rejected("test", "/etc/passwd")
|
||||
assert _is_rejected("test", "C:\\Windows\\System32")
|
||||
assert _is_rejected("test", "C:/Windows")
|
||||
|
||||
|
||||
def test_unc_path_rejected(tmp_media_folder: Path):
|
||||
assert _is_rejected("test", "\\\\server\\share")
|
||||
assert _is_rejected("test", "//server/share")
|
||||
|
||||
|
||||
def test_null_byte_rejected(tmp_media_folder: Path):
|
||||
assert _is_rejected("test", "ok.mp3\x00.png")
|
||||
assert _is_rejected("test", "sub\x00/nested.mp3")
|
||||
@@ -0,0 +1,46 @@
|
||||
"""Atomic config writes + POSIX permission hardening."""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import os
|
||||
import stat
|
||||
import sys
|
||||
import tempfile
|
||||
from pathlib import Path
|
||||
|
||||
import pytest
|
||||
|
||||
from media_server.config import _restrict_config_perms, _write_yaml_atomic
|
||||
|
||||
|
||||
def test_atomic_write_round_trip():
|
||||
with tempfile.TemporaryDirectory() as tmp:
|
||||
path = Path(tmp) / "config.yaml"
|
||||
_write_yaml_atomic(path, {"port": 8765, "host": "127.0.0.1"})
|
||||
assert path.exists()
|
||||
# Tmp file from the rename should be gone.
|
||||
assert not path.with_suffix(path.suffix + ".tmp").exists()
|
||||
# Contents are valid YAML and round-trip.
|
||||
import yaml
|
||||
data = yaml.safe_load(path.read_text())
|
||||
assert data == {"port": 8765, "host": "127.0.0.1"}
|
||||
|
||||
|
||||
def test_atomic_write_replaces_existing():
|
||||
with tempfile.TemporaryDirectory() as tmp:
|
||||
path = Path(tmp) / "config.yaml"
|
||||
path.write_text("old: 1\n")
|
||||
_write_yaml_atomic(path, {"new": 2})
|
||||
import yaml
|
||||
assert yaml.safe_load(path.read_text()) == {"new": 2}
|
||||
|
||||
|
||||
@pytest.mark.skipif(sys.platform == "win32", reason="POSIX-only permission check")
|
||||
def test_restrict_config_perms_posix():
|
||||
with tempfile.TemporaryDirectory() as tmp:
|
||||
path = Path(tmp) / "config.yaml"
|
||||
path.write_text("token: secret\n")
|
||||
_restrict_config_perms(path)
|
||||
mode = stat.S_IMODE(os.stat(path).st_mode)
|
||||
# Owner read+write only.
|
||||
assert mode == 0o600, f"got {oct(mode)}"
|
||||
@@ -8,10 +8,6 @@ Windows/Linux/macOS probes themselves are exercised through manual runs.
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import time
|
||||
|
||||
import pytest
|
||||
|
||||
from media_server.services import foreground_service as fg
|
||||
|
||||
|
||||
|
||||
@@ -0,0 +1,45 @@
|
||||
"""Tag-name validation in the Gitea release provider.
|
||||
|
||||
Whitelist regex protects the URL we broadcast to clients from any path
|
||||
traversal or character-set abuse in a hostile (or MITM'd) upstream response.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from media_server.services.gitea_release_provider import _TAG_RE
|
||||
|
||||
|
||||
def test_accepts_plain_semver():
|
||||
assert _TAG_RE.match("1.0.0")
|
||||
assert _TAG_RE.match("v1.0.0")
|
||||
assert _TAG_RE.match("0.3.7")
|
||||
|
||||
|
||||
def test_accepts_pre_release_suffix():
|
||||
assert _TAG_RE.match("v1.0.0-alpha.1")
|
||||
assert _TAG_RE.match("v2.3.4-rc.10")
|
||||
assert _TAG_RE.match("v0.2.7+build.42")
|
||||
|
||||
|
||||
def test_rejects_path_traversal():
|
||||
assert not _TAG_RE.match("../etc/passwd")
|
||||
assert not _TAG_RE.match("v1.0.0/../../evil")
|
||||
assert not _TAG_RE.match("v1.0.0/secret")
|
||||
|
||||
|
||||
def test_rejects_url_injection():
|
||||
assert not _TAG_RE.match("v1.0.0?evil=1")
|
||||
assert not _TAG_RE.match("v1.0.0#frag")
|
||||
assert not _TAG_RE.match("v1.0.0 OR 1=1")
|
||||
assert not _TAG_RE.match("https://evil.example/")
|
||||
|
||||
|
||||
def test_rejects_empty_and_garbage():
|
||||
assert not _TAG_RE.match("")
|
||||
assert not _TAG_RE.match("not-a-version")
|
||||
assert not _TAG_RE.match("v")
|
||||
|
||||
|
||||
def test_rejects_excessively_long_suffix():
|
||||
long_suffix = "x" * 40
|
||||
assert not _TAG_RE.match(f"v1.0.0-{long_suffix}")
|
||||
@@ -0,0 +1,68 @@
|
||||
"""Token-bucket rate limiter behaviour."""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import time
|
||||
|
||||
import pytest
|
||||
|
||||
from media_server.services import rate_limit
|
||||
|
||||
|
||||
@pytest.fixture(autouse=True)
|
||||
def _reset_state():
|
||||
rate_limit._state.clear()
|
||||
rate_limit._LAST_CLEANUP = 0.0
|
||||
yield
|
||||
rate_limit._state.clear()
|
||||
|
||||
|
||||
def test_allows_up_to_capacity_then_blocks(monkeypatch):
|
||||
"""Default execute bucket = 10/min."""
|
||||
peer = "10.0.0.1"
|
||||
for i in range(10):
|
||||
ok, retry = rate_limit.check("execute", peer)
|
||||
assert ok, f"expected allow on attempt {i + 1}, got block (retry={retry})"
|
||||
ok, retry = rate_limit.check("execute", peer)
|
||||
assert ok is False
|
||||
assert retry is not None and retry > 0
|
||||
|
||||
|
||||
def test_different_peers_independent():
|
||||
for _ in range(10):
|
||||
assert rate_limit.check("execute", "10.0.0.1")[0]
|
||||
# Different peer should still be allowed.
|
||||
assert rate_limit.check("execute", "10.0.0.2")[0]
|
||||
|
||||
|
||||
def test_unknown_bucket_uses_default():
|
||||
peer = "10.0.0.3"
|
||||
# default = 60/min — first call always allowed.
|
||||
allowed, _ = rate_limit.check("nonexistent-bucket", peer)
|
||||
assert allowed
|
||||
|
||||
|
||||
def test_auth_bucket_is_strict():
|
||||
"""auth bucket = 5/min."""
|
||||
peer = "10.0.0.4"
|
||||
for _ in range(5):
|
||||
assert rate_limit.check("auth", peer)[0]
|
||||
blocked, retry = rate_limit.check("auth", peer)
|
||||
assert not blocked
|
||||
assert retry is not None
|
||||
|
||||
|
||||
def test_refill_eventually_unblocks(monkeypatch):
|
||||
"""Verify the bucket refills — exhaust then wait one refill period."""
|
||||
peer = "10.0.0.5"
|
||||
# Replace BUCKETS with a fast-refilling one for the test only.
|
||||
monkeypatch.setitem(
|
||||
rate_limit.BUCKETS,
|
||||
"fast-test",
|
||||
rate_limit.BucketConfig(capacity=2, refill_per_sec=10.0),
|
||||
)
|
||||
assert rate_limit.check("fast-test", peer)[0]
|
||||
assert rate_limit.check("fast-test", peer)[0]
|
||||
assert not rate_limit.check("fast-test", peer)[0]
|
||||
time.sleep(0.15) # 0.15 * 10 = 1.5 tokens
|
||||
assert rate_limit.check("fast-test", peer)[0]
|
||||
@@ -0,0 +1,77 @@
|
||||
"""Validation rules for script parameters (type coercion, regex pattern)."""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import pytest
|
||||
from fastapi import HTTPException
|
||||
|
||||
from media_server.config import ScriptParameterConfig
|
||||
from media_server.routes.scripts import _validate_params
|
||||
|
||||
|
||||
def _defs(**kwargs) -> dict[str, ScriptParameterConfig]:
|
||||
return {name: ScriptParameterConfig(**spec) for name, spec in kwargs.items()}
|
||||
|
||||
|
||||
def test_unknown_param_rejected():
|
||||
with pytest.raises(HTTPException) as ei:
|
||||
_validate_params({"x": "1"}, _defs())
|
||||
assert ei.value.status_code == 400
|
||||
assert "Unknown" in ei.value.detail
|
||||
|
||||
|
||||
def test_missing_required_rejected():
|
||||
defs = _defs(name={"type": "string", "required": True})
|
||||
with pytest.raises(HTTPException, match="missing"):
|
||||
_validate_params({}, defs)
|
||||
|
||||
|
||||
def test_integer_coercion_and_bounds():
|
||||
defs = _defs(volume={"type": "integer", "min": 0, "max": 100})
|
||||
out = _validate_params({"volume": "42"}, defs)
|
||||
assert out == {"SCRIPT_PARAM_VOLUME": "42"}
|
||||
|
||||
with pytest.raises(HTTPException, match="<="):
|
||||
_validate_params({"volume": 200}, defs)
|
||||
with pytest.raises(HTTPException, match=">="):
|
||||
_validate_params({"volume": -1}, defs)
|
||||
with pytest.raises(HTTPException, match="integer"):
|
||||
_validate_params({"volume": "not-a-number"}, defs)
|
||||
|
||||
|
||||
def test_boolean_coercion():
|
||||
defs = _defs(flag={"type": "boolean"})
|
||||
assert _validate_params({"flag": "true"}, defs) == {"SCRIPT_PARAM_FLAG": "True"}
|
||||
assert _validate_params({"flag": "no"}, defs) == {"SCRIPT_PARAM_FLAG": "False"}
|
||||
with pytest.raises(HTTPException, match="boolean"):
|
||||
_validate_params({"flag": "maybe"}, defs)
|
||||
|
||||
|
||||
def test_select_rejects_non_option():
|
||||
defs = _defs(mode={"type": "select", "options": ["a", "b", "c"]})
|
||||
assert _validate_params({"mode": "a"}, defs) == {"SCRIPT_PARAM_MODE": "a"}
|
||||
with pytest.raises(HTTPException, match="must be one of"):
|
||||
_validate_params({"mode": "z"}, defs)
|
||||
|
||||
|
||||
def test_pattern_enforced_on_string():
|
||||
"""Regex pattern is the defence against shell metachars in shell=true scripts."""
|
||||
defs = _defs(host={"type": "string", "pattern": r"^[a-z0-9.\-]+$"})
|
||||
assert _validate_params({"host": "example.com"}, defs) == {"SCRIPT_PARAM_HOST": "example.com"}
|
||||
with pytest.raises(HTTPException, match="pattern"):
|
||||
_validate_params({"host": "evil & calc.exe"}, defs)
|
||||
with pytest.raises(HTTPException, match="pattern"):
|
||||
_validate_params({"host": "$(rm -rf /)"}, defs)
|
||||
|
||||
|
||||
def test_pattern_can_disallow_empty():
|
||||
defs = _defs(host={"type": "string", "pattern": r"^[a-z]+$"})
|
||||
with pytest.raises(HTTPException, match="pattern"):
|
||||
_validate_params({"host": ""}, defs)
|
||||
|
||||
|
||||
def test_invalid_pattern_in_config_fails_closed():
|
||||
defs = _defs(host={"type": "string", "pattern": r"["}) # unmatched bracket
|
||||
with pytest.raises(HTTPException) as ei:
|
||||
_validate_params({"host": "x"}, defs)
|
||||
assert ei.value.status_code == 500
|
||||
Reference in New Issue
Block a user