Security: enforce the launch PIN server-side, not just a client overlay (#832)
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.
This commit is contained in:
BoulderBadgeDad
parent
111af5150e
commit
66724186b1
4 changed files with 245 additions and 2 deletions
0
core/security/__init__.py
Normal file
0
core/security/__init__.py
Normal file
89
core/security/launch_lock.py
Normal file
89
core/security/launch_lock.py
Normal file
|
|
@ -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']
|
||||
115
tests/test_launch_lock_gate.py
Normal file
115
tests/test_launch_lock_gate.py
Normal file
|
|
@ -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
|
||||
|
|
@ -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,
|
||||
|
|
|
|||
Loading…
Reference in a new issue