From 66724186b1efaf2cea6048120e2ab9e3e9b801ec Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Tue, 9 Jun 2026 21:58:44 -0700 Subject: [PATCH] Security: enforce the launch PIN server-side, not just a client overlay (#832) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Beckid: the admin launch PIN was a CLIENT-SIDE overlay only. `launch_pin_required` just told the frontend to draw a fixed div over the app — removing it (Safari "Hide Distracting Items", devtools, or any non-browser client like curl) gave full unauthenticated access to every /api/* endpoint, because the server never checked it. Anyone who reverse-proxies SoulSync publicly was wide open. Fix: a before_request gate (_enforce_launch_pin) that rejects every request from an unverified session while security.require_pin_on_launch is on. The decision is a pure, unit-tested helper (core/security/launch_lock.request_is_locked) so the allow/deny matrix can't silently regress. Allowed while locked: the page shell + static assets, the unlock flow (current/list/select/verify/reset/logout), and the public REST API /api/v1/ (its own @require_api_key governs it) — EXCEPT /api/v1/api-keys-internal*, the "no auth required" key-management endpoints, which stay locked so an attacker can't mint an API key and walk in the side door. Everything else (data, settings, profile create/edit/delete/set-pin, socket.io) is blocked. A blocked top-level browser navigation (deep link / refresh on a sub-page like /dashboard) is redirected to the root lock screen instead of dumping raw JSON — detected via Sec-Fetch-Mode: navigate / Accept: text/html (is_html_navigation). Programmatic fetch/XHR still get the JSON 401 so the frontend can react. Also fixed the verified flag: get_current_profile POPPED launch_pin_verified (one page load), but an enforced gate needs it to persist — now READ, so verification lasts the session (until logout/expiry). No-ops entirely when require_pin_on_launch is off (default). Tests: full allow/deny matrix + navigation detection. 20 gate tests + 232 profile/security tests pass. --- core/security/__init__.py | 0 core/security/launch_lock.py | 89 +++++++++++++++++++++++++ tests/test_launch_lock_gate.py | 115 +++++++++++++++++++++++++++++++++ web_server.py | 43 +++++++++++- 4 files changed, 245 insertions(+), 2 deletions(-) create mode 100644 core/security/__init__.py create mode 100644 core/security/launch_lock.py create mode 100644 tests/test_launch_lock_gate.py diff --git a/core/security/__init__.py b/core/security/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/core/security/launch_lock.py b/core/security/launch_lock.py new file mode 100644 index 00000000..8b8ce52a --- /dev/null +++ b/core/security/launch_lock.py @@ -0,0 +1,89 @@ +"""Server-side enforcement of the launch PIN (#832). + +Beckid: the admin "launch PIN" was a client-side overlay only — the +``launch_pin_required`` flag just told the frontend to draw a fixed-position +div over the app. Removing that div (Safari "Hide Distracting Items", devtools, +or any non-browser client like curl) gave full, unauthenticated access to every +``/api/*`` endpoint, because nothing on the server ever checked it. + +``request_is_locked`` is the pure decision the ``before_request`` gate uses: +given the request path/method and the session's verified state, should this +request be blocked? Kept pure (no Flask) so the allow/deny matrix is unit- +testable without standing up the whole app. + +Allow-list while locked (everything else → 401): + * ``/`` and ``/static/`` and ``/favicon*`` — the page shell + lock-screen + assets must load so the user can enter the PIN. + * The unlock flow itself — current-profile probe, profile list/select for the + picker, verify-launch-pin, reset-pin-via-credential, logout. + * The public REST API ``/api/v1/`` — those routes carry their OWN + ``@require_api_key`` auth and are built for headless automation, so a + launch-locked UI shouldn't break a legitimate key holder. EXCEPT + ``/api/v1/api-keys-internal*``, which are session-UI key management + ("no auth required") and MUST stay locked — otherwise an attacker could + mint a key and walk in through the public API. +""" + +from __future__ import annotations + +# GET endpoints the lock/picker screens need before a PIN is entered. +_ALLOWED_GET = frozenset({ + '/api/profiles', # profile picker list (multi-profile launch) + '/api/profiles/current', # how the frontend detects the lock state +}) + +# POST endpoints that drive selection + unlock. Selecting a profile only sets +# session['profile_id'] (+ any per-profile PIN check); it does NOT set +# launch_pin_verified, so it can't bypass the launch lock. +_ALLOWED_POST = frozenset({ + '/api/profiles/select', + '/api/profiles/verify-launch-pin', + '/api/profiles/reset-pin-via-credential', + '/api/profiles/logout', +}) + + +def is_html_navigation(method: str, accept: str, sec_fetch_mode: str) -> bool: + """True when a BLOCKED request is a top-level browser navigation (address + bar, link, refresh) rather than a programmatic fetch/XHR. + + Such a request should be bounced to the root lock screen, not handed a raw + JSON 401 — otherwise deep-linking/refreshing on a sub-page (e.g. /dashboard) + while locked dumps JSON in the user's face (#832 follow-up). Programmatic + fetches (Accept: */* or application/json) still get the JSON so the frontend + can react to the lock. + """ + if (method or 'GET').upper() != 'GET': + return False + if (sec_fetch_mode or '').strip().lower() == 'navigate': + return True + return 'text/html' in (accept or '').lower() + + +def request_is_locked(path: str, method: str, *, + require_pin: bool, pin_verified: bool) -> bool: + """True when the launch-PIN gate must reject this request with 401.""" + if not require_pin or pin_verified: + return False + + path = path or '' + method = (method or 'GET').upper() + + # Page shell + assets needed to render the lock screen. + if path == '/' or path.startswith('/static/') or path.startswith('/favicon'): + return False + + # Key-authed public API — its own auth governs it. The session-UI key + # management under it is the one exception that stays locked. + if path.startswith('/api/v1/') and not path.startswith('/api/v1/api-keys-internal'): + return False + + if method == 'GET' and path in _ALLOWED_GET: + return False + if method == 'POST' and path in _ALLOWED_POST: + return False + + return True + + +__all__ = ['request_is_locked', 'is_html_navigation'] diff --git a/tests/test_launch_lock_gate.py b/tests/test_launch_lock_gate.py new file mode 100644 index 00000000..1e897815 --- /dev/null +++ b/tests/test_launch_lock_gate.py @@ -0,0 +1,115 @@ +"""Server-side launch-PIN gate (#832, Beckid). + +The admin PIN was a client-side overlay only; removing the div gave full API +access. request_is_locked() is the pure allow/deny decision the before_request +gate uses. These pin the exact matrix so the lock can't silently regress to +'advisory'. +""" + +from __future__ import annotations + +import pytest + +from core.security.launch_lock import request_is_locked + + +def L(path, method='GET', require_pin=True, pin_verified=False): + return request_is_locked(path, method, require_pin=require_pin, pin_verified=pin_verified) + + +# ── gate disabled / already verified → never locks ────────────────────────── + +def test_no_lock_when_require_pin_off(): + # The default config: nothing is gated. + assert L('/api/downloads/start', 'POST', require_pin=False) is False + assert L('/api/settings', 'POST', require_pin=False) is False + + +def test_no_lock_when_session_verified(): + assert L('/api/downloads/start', 'POST', pin_verified=True) is False + assert L('/api/v1/api-keys-internal/generate', 'POST', pin_verified=True) is False + + +# ── locked session blocks the app ─────────────────────────────────────────── + +@pytest.mark.parametrize('path,method', [ + ('/api/watchlist/artists', 'GET'), + ('/api/downloads/start', 'POST'), + ('/api/settings', 'POST'), + ('/api/wishlist/tracks', 'GET'), + ('/api/library/artists', 'GET'), + ('/socket.io/', 'GET'), +]) +def test_locked_blocks_data_and_action_endpoints(path, method): + assert L(path, method) is True + + +def test_locked_blocks_profile_mutation(): + # Creating/editing/deleting profiles or setting PINs must NOT be reachable + # pre-auth — else an attacker mints an admin or rewrites the PIN. + assert L('/api/profiles', 'POST') is True # create + assert L('/api/profiles/2', 'PUT') is True # edit + assert L('/api/profiles/2', 'DELETE') is True # delete + assert L('/api/profiles/1/set-pin', 'POST') is True + + +def test_locked_blocks_internal_key_minting(): + # The documented "no auth required" key-management endpoints are the + # secondary bypass: mint a key, walk in via the public API. Must be locked. + assert L('/api/v1/api-keys-internal', 'GET') is True + assert L('/api/v1/api-keys-internal/generate', 'POST') is True + assert L('/api/v1/api-keys-internal/revoke/abc', 'DELETE') is True + + +# ── locked session still allows the unlock flow + shell ───────────────────── + +@pytest.mark.parametrize('path', ['/', '/static/init.js', '/static/dist/app.js', '/favicon.ico']) +def test_locked_allows_page_shell_and_assets(path): + assert L(path) is False + + +def test_locked_allows_unlock_flow(): + assert L('/api/profiles/current', 'GET') is False + assert L('/api/profiles', 'GET') is False # picker list + assert L('/api/profiles/select', 'POST') is False + assert L('/api/profiles/verify-launch-pin', 'POST') is False + assert L('/api/profiles/reset-pin-via-credential', 'POST') is False + assert L('/api/profiles/logout', 'POST') is False + + +def test_locked_allows_keyauthed_public_api(): + # The public REST API carries its own @require_api_key, so a launch-locked + # UI must not break a legitimate headless key holder. + assert L('/api/v1/search', 'GET') is False + assert L('/api/v1/playlists', 'GET') is False + + +def test_method_matters_for_shared_paths(): + # GET /api/profiles is the picker (allowed); POST /api/profiles is create + # (blocked). Same path, opposite verdicts. + assert L('/api/profiles', 'GET') is False + assert L('/api/profiles', 'POST') is True + + +# ── blocked navigations bounce to the lock screen, not JSON (#832 follow-up) ── + +from core.security.launch_lock import is_html_navigation # noqa: E402 + + +def test_browser_navigation_is_detected(): + # Address-bar / link / refresh — gets redirected to the root lock screen. + assert is_html_navigation('GET', 'text/html,application/xhtml+xml,*/*', '') is True + assert is_html_navigation('GET', '*/*', 'navigate') is True + + +def test_programmatic_fetch_is_not_navigation(): + # fetch()/XHR want JSON so the frontend can react to the 401. + assert is_html_navigation('GET', '*/*', 'cors') is False + assert is_html_navigation('GET', 'application/json', 'same-origin') is False + assert is_html_navigation('GET', '', '') is False + + +def test_non_get_is_never_navigation(): + # A programmatic POST/DELETE always gets JSON, never a redirect. + assert is_html_navigation('POST', 'text/html', 'navigate') is False + assert is_html_navigation('DELETE', 'text/html', '') is False diff --git a/web_server.py b/web_server.py index 922bb85a..8f17ced1 100644 --- a/web_server.py +++ b/web_server.py @@ -390,6 +390,41 @@ def inject_webui_assets(): 'vite_assets': build_webui_vite_assets, } +# --- Launch PIN gate (before_request hook) --- +@app.before_request +def _enforce_launch_pin(): + """Server-side enforcement of the launch PIN (#832). + + The PIN was previously a client-only overlay — removing the div (Safari + "Hide Distracting Items", devtools, curl) gave full API access. This rejects + every request from an unverified session with 401 while the launch lock is + on, except the page shell + the unlock flow + the key-authed public API. + No-ops entirely when ``security.require_pin_on_launch`` is off (the default). + """ + try: + require_pin = bool(config_manager.get('security.require_pin_on_launch', False)) if config_manager else False + except Exception: + require_pin = False + if not require_pin: + return + from core.security.launch_lock import request_is_locked, is_html_navigation + if request_is_locked( + request.path, request.method, + require_pin=require_pin, + pin_verified=bool(session.get('launch_pin_verified', False)), + ): + # A browser navigating to a sub-page (deep link / refresh) should land + # on the lock screen, not raw JSON — bounce it to the root, which serves + # the lock UI. Programmatic fetch/XHR get the JSON so the frontend reacts. + if is_html_navigation( + request.method, + request.headers.get('Accept', ''), + request.headers.get('Sec-Fetch-Mode', ''), + ): + return redirect('/') + return jsonify({"error": "locked", "launch_pin_required": True}), 401 + + # --- Profile Context (before_request hook) --- @app.before_request def _set_profile_context(): @@ -25059,8 +25094,12 @@ def get_current_profile(): # Check if launch PIN is required require_pin = config_manager.get('security.require_pin_on_launch', False) if config_manager else False - # Check if PIN was verified this page load, then consume the flag - pin_verified = session.pop('launch_pin_verified', False) + # #832: READ (don't pop) the verified flag — the server-side gate + # (_enforce_launch_pin) relies on it persisting for the whole session, + # so verified requests keep passing. Verification now lasts the session + # (until logout / cookie expiry) instead of one page load — which is + # both what an enforced gate requires and the correct security model. + pin_verified = session.get('launch_pin_verified', False) return jsonify({ 'success': True,