diff --git a/config/settings.py b/config/settings.py index 8c2ab6db..c32c5b75 100644 --- a/config/settings.py +++ b/config/settings.py @@ -66,6 +66,12 @@ class ConfigManager: self._load_config() + # Placeholder shipped to the browser in place of a configured secret + # (#832 follow-up). The settings UI shows it as masked dots; if it's + # round-tripped back on save, ``set()`` treats it as "keep existing" so the + # real value is never overwritten by the mask. + REDACTED_SENTINEL = '__redacted_unchanged__' + # Dot-notation paths to sensitive config values that must be encrypted at rest. # Paths pointing to dicts encrypt the entire dict as a JSON blob. _SENSITIVE_PATHS = frozenset({ @@ -792,7 +798,40 @@ class ConfigManager: return value + def redacted_config(self) -> Dict[str, Any]: + """Deep copy of the live config with every sensitive value masked. + + Used for ``GET /api/settings`` so decrypted secrets never reach the + browser (#832 follow-up). A *set* secret becomes ``REDACTED_SENTINEL`` + (the UI renders it as masked dots); an unset one stays empty so the UI + can show "not configured". Dict-valued secrets (OAuth sessions) collapse + to the sentinel too — the UI has no field for them anyway. The matching + guard in ``set()`` turns a round-tripped sentinel back into a no-op. + """ + import copy + data = copy.deepcopy(self.config_data) + for path in self._SENSITIVE_PATHS: + keys = path.split('.') + parent = data + for k in keys[:-1]: + if isinstance(parent, dict) and k in parent: + parent = parent[k] + else: + parent = None + break + if not isinstance(parent, dict): + continue + leaf = keys[-1] + if leaf in parent and parent[leaf] not in (None, '', {}, [], 0, False): + parent[leaf] = self.REDACTED_SENTINEL + return data + def set(self, key: str, value: Any): + # The UI round-trips REDACTED_SENTINEL for any secret the user didn't + # touch — never let the mask overwrite the real value (#832 follow-up). + if value == self.REDACTED_SENTINEL and key in self._SENSITIVE_PATHS: + return + keys = key.split('.') config = self.config_data diff --git a/tests/test_settings_redaction.py b/tests/test_settings_redaction.py new file mode 100644 index 00000000..3aea7a2a --- /dev/null +++ b/tests/test_settings_redaction.py @@ -0,0 +1,111 @@ +"""Settings-secret redaction for GET /api/settings (#832 follow-up). + +GET /api/settings used to ship the DECRYPTED config to the browser — every API +key, token, and password in cleartext. redacted_config() masks configured +secrets with a sentinel; set() refuses to let that round-tripped sentinel +overwrite the real value. Together: secrets never reach the client, and saving +an unchanged form never clobbers them. + +ConfigManager.__init__ touches the DB, so these build instances via __new__ and +set config_data directly — the methods under test only read/write that dict. +""" + +from __future__ import annotations + +from config.settings import ConfigManager + +S = ConfigManager.REDACTED_SENTINEL + + +def _cm(config_data): + cm = ConfigManager.__new__(ConfigManager) + cm.config_data = config_data + cm._save_config = lambda: None + return cm + + +# ── redacted_config: secrets out, everything else intact ──────────────────── + +def test_configured_secrets_are_masked(): + cm = _cm({'spotify': {'client_secret': 'REAL', 'redirect_uri': 'http://x'}, + 'plex': {'token': 'PLEXTOK', 'url': 'http://plex'}}) + r = cm.redacted_config() + assert r['spotify']['client_secret'] == S + assert r['plex']['token'] == S + # Non-secret siblings pass through untouched. + assert r['spotify']['redirect_uri'] == 'http://x' + assert r['plex']['url'] == 'http://plex' + + +def test_unset_secret_stays_empty_not_masked(): + # An empty secret must NOT become the sentinel — the UI shows "not set". + cm = _cm({'jellyfin': {'api_key': ''}, 'navidrome': {'password': None}}) + r = cm.redacted_config() + assert r['jellyfin']['api_key'] == '' + assert r['navidrome']['password'] is None + + +def test_dict_valued_secret_is_masked(): + # OAuth session blobs (tidal/qobuz) collapse to the sentinel. + cm = _cm({'tidal_download': {'session': {'access': 'A', 'refresh': 'R'}}}) + assert cm.redacted_config()['tidal_download']['session'] == S + + +def test_redaction_does_not_mutate_live_config(): + cm = _cm({'spotify': {'client_secret': 'REAL'}}) + cm.redacted_config() + assert cm.config_data['spotify']['client_secret'] == 'REAL' + + +def _put(cfg, path, value): + parent = cfg + keys = path.split('.') + for k in keys[:-1]: + parent = parent.setdefault(k, {}) + parent[keys[-1]] = value + + +def _at(cfg, path): + cur = cfg + for k in path.split('.'): + cur = cur[k] + return cur + + +def test_every_sensitive_path_is_masked(): + # Put a value at every sensitive path (any depth) — none may survive in clear. + cfg = {} + for path in ConfigManager._SENSITIVE_PATHS: + _put(cfg, path, 'VALUE') + r = _cm(cfg).redacted_config() + leaked = [p for p in ConfigManager._SENSITIVE_PATHS if _at(r, p) != S] + assert leaked == [], f"secrets shipped in cleartext: {leaked}" + + +# ── set() guard: the sentinel can never overwrite a real secret ───────────── + +def test_sentinel_roundtrip_keeps_existing_secret(): + cm = _cm({'spotify': {'client_secret': 'REAL'}}) + cm.set('spotify.client_secret', S) # untouched masked field saved + assert cm.config_data['spotify']['client_secret'] == 'REAL' + + +def test_real_value_overwrites(): + cm = _cm({'spotify': {'client_secret': 'REAL'}}) + cm.set('spotify.client_secret', 'NEW') + assert cm.config_data['spotify']['client_secret'] == 'NEW' + + +def test_empty_value_clears_secret(): + # Deliberately clearing a secret must still work (empty != sentinel). + cm = _cm({'spotify': {'client_secret': 'REAL'}}) + cm.set('spotify.client_secret', '') + assert cm.config_data['spotify']['client_secret'] == '' + + +def test_sentinel_on_non_secret_path_writes_normally(): + # The guard is scoped to sensitive paths — a literal sentinel elsewhere is + # a normal write (absurd in practice, but proves the guard isn't global). + cm = _cm({'ui_appearance': {'theme': 'dark'}}) + cm.set('ui_appearance.theme', S) + assert cm.config_data['ui_appearance']['theme'] == S diff --git a/web_server.py b/web_server.py index 3f97b988..c2cc2017 100644 --- a/web_server.py +++ b/web_server.py @@ -3045,11 +3045,12 @@ def handle_settings(): return jsonify({"success": False, "error": str(e)}), 500 else: # GET request try: - data = dict(config_manager.config_data) - # Never ship the OAuth token payload to the browser — the settings - # UI has no field for it and it doesn't belong in devtools/HAR - # captures. NOTE: dict() above is a SHALLOW copy of live config - # state, so rebuild the section instead of popping in place. + # Masks every configured secret (API keys, tokens, passwords) as a + # sentinel so decrypted credentials never reach the browser/devtools/ + # HAR captures (#832 follow-up). Deep copy — safe to mutate below. + data = config_manager.redacted_config() + # Also drop the Spotify OAuth token payload — not a settings field + # and not in the sensitive-paths list. if isinstance(data.get('spotify'), dict) and 'token_info' in data['spotify']: data['spotify'] = {k: v for k, v in data['spotify'].items() if k != 'token_info'} # Include which download sources are configured so the UI can auto-disable unconfigured ones diff --git a/webui/static/settings.js b/webui/static/settings.js index f1212f2e..533d66b8 100644 --- a/webui/static/settings.js +++ b/webui/static/settings.js @@ -1400,12 +1400,49 @@ async function loadSettingsData() { console.error('Error checking dev mode:', error); } + // Secret fields now arrive masked as REDACTED_SECRET_SENTINEL (#832 + // follow-up) — wire them so editing replaces the mask instead of typing + // on top of it, and an untouched field re-masks on blur (round-trips the + // sentinel, which the server treats as "keep existing"). + _wireRedactedSecrets(); + } catch (error) { console.error('Error loading settings:', error); showToast('Failed to load settings', 'error'); } } +// Mirrors ConfigManager.REDACTED_SENTINEL — secrets are never sent to the +// browser; configured ones come back as this placeholder (rendered as dots in +// the password inputs). +const REDACTED_SECRET_SENTINEL = '__redacted_unchanged__'; + +function _wireRedactedSecrets() { + document.querySelectorAll('input[type="password"]').forEach(el => { + if (el.dataset.redactWired === '1') return; + el.dataset.redactWired = '1'; + // Clear the mask on focus so the user types a fresh value, not on top + // of the sentinel. + el.addEventListener('focus', () => { + if (el.value === REDACTED_SECRET_SENTINEL) { + el.value = ''; + el.dataset.wasRedacted = '1'; + } + }); + // Untouched (focused but not edited, left empty) → restore the mask so + // save round-trips the sentinel and the real secret is kept. + el.addEventListener('blur', () => { + if (el.dataset.wasRedacted === '1' && el.value === '') { + el.value = REDACTED_SECRET_SENTINEL; + el.dataset.wasRedacted = ''; + } + }); + // Real typing means a genuine change/clear — drop the redacted mark so + // blur won't re-mask (an emptied field then saves as a real clear). + el.addEventListener('input', () => { el.dataset.wasRedacted = ''; }); + }); +} + async function changeLogLevel() { const selector = document.getElementById('log-level-select'); const level = selector.value;