From 02d6af29edc35a8826e88906970028b6af222cb8 Mon Sep 17 00:00:00 2001
From: BoulderBadgeDad <batmanesignal@gmail.com>
Date: Mon, 15 Jun 2026 20:09:53 -0700
Subject: [PATCH] #879: a failed settings load must never overwrite the saved
 config

Reported by @Lysticity: opening Settings reset the whole config to defaults. The
chain: GET /api/settings 500s (their env: ConfigManager missing redacted_config)
-> loadSettingsData() called response.json() WITHOUT checking response.ok, so the
error body {"error": ...} was treated as settings -> every field populated as
`settings.x?.y || ''` blanked to defaults -> autosave then wrote those defaults
over the real config.

Fix (settings.js): bail BEFORE touching any field when the response isn't ok / is
an error body, set window._settingsLoadFailed, and guard BOTH save paths
(debouncedAutoSaveSettings + saveSettings) on it. The flag clears on the next
successful load. So any load failure (500, lock, network) now leaves the saved
config untouched instead of wiping it.

The redacted_config method exists in all 2.7.x source + on dev (their 500 looks
like a stale/mismatched build), but the UI must not destroy config on ANY failed
load. Regression test pins redacted_config stays a callable method on the class
(its removal is exactly what 500s the endpoint).
---
 tests/test_settings_redaction.py | 17 +++++++++++++++++
 webui/static/settings.js         | 30 +++++++++++++++++++++++++++++-
 2 files changed, 46 insertions(+), 1 deletion(-)

diff --git a/tests/test_settings_redaction.py b/tests/test_settings_redaction.py
index 3aea7a2a..a27e4d25 100644
--- a/tests/test_settings_redaction.py
+++ b/tests/test_settings_redaction.py
@@ -24,6 +24,23 @@ def _cm(config_data):
     return cm
 
 
+# ── #879: the GET /api/settings handler calls config_manager.redacted_config().
+# If that method is ever renamed/removed the endpoint 500s, the web UI treats the
+# error body as settings, blanks the form to defaults, and autosaves over the
+# user's real config. Pin the method's presence on the class so that can't ship.
+
+def test_redacted_config_is_a_method_on_the_class():
+    assert callable(getattr(ConfigManager, 'redacted_config', None)), (
+        "GET /api/settings depends on ConfigManager.redacted_config(); removing or "
+        "renaming it 500s the settings endpoint and the UI then wipes the config (#879)")
+
+
+def test_redacted_config_callable_on_instance_returns_dict():
+    cm = _cm({'spotify': {'client_secret': 'REAL'}})
+    out = cm.redacted_config()
+    assert isinstance(out, dict) and out.get('spotify', {}).get('client_secret') == S
+
+
 # ── redacted_config: secrets out, everything else intact ────────────────────
 
 def test_configured_secrets_are_masked():
diff --git a/webui/static/settings.js b/webui/static/settings.js
index 1921839b..d00c79d9 100644
--- a/webui/static/settings.js
+++ b/webui/static/settings.js
@@ -45,6 +45,9 @@ function debouncedAutoSaveSettings() {
     // fields on load — those aren't user edits and must not trigger a full
     // save (which re-initializes every backend service client).
     if (window._suppressSettingsAutoSave) return;
+    // #879: never auto-save while the last settings load failed — the form is
+    // showing defaults, not the real config, so saving would wipe it.
+    if (window._settingsLoadFailed) return;
     // #827: the Logs tab has no savable settings — its live-viewer controls
     // (source picker, filters, auto-scroll) were tripping the auto-save and
     // flooding app.log with "Settings saved" lines, drowning out the logs the
@@ -976,6 +979,18 @@ async function loadSettingsData() {
         const response = await fetch(API.settings);
         const settings = await response.json();
 
+        // #879: a failed GET /api/settings returns an error body (e.g. {"error":
+        // "..."} on a 500), NOT real settings. Populating from it blanks every
+        // field to its default ('settings.spotify?.x || ""'), and the next
+        // (auto)save then overwrites the user's real config. Abort BEFORE
+        // touching any field and flag it so saves stay blocked until a good load.
+        if (!response.ok || !settings || typeof settings !== 'object' || settings.error) {
+            window._settingsLoadFailed = true;
+            throw new Error('settings load failed (HTTP ' + response.status + '): ' +
+                ((settings && settings.error) || 'unexpected response'));
+        }
+        window._settingsLoadFailed = false;  // good load → saving is safe again
+
         // Populate Spotify settings
         document.getElementById('spotify-client-id').value = settings.spotify?.client_id || '';
         document.getElementById('spotify-client-secret').value = settings.spotify?.client_secret || '';
@@ -1461,7 +1476,10 @@ async function loadSettingsData() {
 
     } catch (error) {
         console.error('Error loading settings:', error);
-        showToast('Failed to load settings', 'error');
+        // #879: any load failure → block saves so a blank/partial form can't be
+        // written over the real config. Cleared on the next successful load.
+        window._settingsLoadFailed = true;
+        showToast('Failed to load settings — reload the page before saving (your saved config is untouched)', 'error');
     }
 }
 
@@ -2861,6 +2879,16 @@ function _getTagConfig(path) {
 }
 
 async function saveSettings(quiet = false) {
+    // #879: refuse to save if the settings never loaded successfully — the form
+    // is showing defaults, not the user's real config, so saving would wipe it.
+    // Cleared automatically on the next successful load (reload the page).
+    if (window._settingsLoadFailed) {
+        if (!quiet && typeof showToast === 'function') {
+            showToast("Settings didn't load — reload the page before saving (your config is untouched)", 'error');
+        }
+        return;
+    }
+
     // Validate file organization templates before saving
     const validationErrors = validateFileOrganizationTemplates();
     if (validationErrors.length > 0) {