#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).
This commit is contained in:
parent
2905fe0853
commit
02d6af29ed
2 changed files with 46 additions and 1 deletions
|
|
@ -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():
|
||||
|
|
|
|||
|
|
@ -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) {
|
||||
|
|
|
|||
Loading…
Reference in a new issue