From 082d03036ff9258920de177cb03e5d283c956e46 Mon Sep 17 00:00:00 2001 From: Brian Krabach Date: Thu, 2 Apr 2026 20:20:32 -0700 Subject: [PATCH] fix: preserve federation keys when PATCH sends redacted empty values MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The GET /api/settings endpoint redacts remote_instances key fields to "" for security. When the frontend PATCHes any setting (e.g. device_name) it sends the full remote_instances array back with those empty strings, which previously overwrote the real keys on disk. Fix: in patch_settings(), snapshot existing remote keys by URL before applying the patch. After merging, restore any key that was emptied by redaction — identified by url match + empty/missing key in patch. Only a non-empty key in the patch is treated as an intentional key rotation. Also adds 5 regression tests covering: - Empty key in patch preserves existing key (core redaction bug) - Non-empty key in patch overwrites old key (intentional rotation) - Missing key field in patch preserves existing key - Multi-remote list: all keys preserved when only one field changes - New remote with a key is saved correctly --- muxplex/settings.py | 25 ++++++ muxplex/tests/test_settings.py | 157 +++++++++++++++++++++++++++++++++ 2 files changed, 182 insertions(+) diff --git a/muxplex/settings.py b/muxplex/settings.py index 378d7d2..aa22a31 100644 --- a/muxplex/settings.py +++ b/muxplex/settings.py @@ -70,11 +70,36 @@ def patch_settings(patch: dict) -> dict: """Merge known keys from *patch* into the current settings, save, and return result. Unknown keys in *patch* are silently ignored. + + Key-preservation rule: when ``remote_instances`` is included in the patch, + any remote whose ``key`` field is empty or absent in the patch retains its + existing key from the on-disk settings. This prevents the redaction-wipe + bug where ``GET /api/settings`` returns ``key=""`` for security reasons and + a subsequent PATCH would silently overwrite real keys with empty strings. + Only a patch that supplies a *non-empty* key value is treated as an + intentional key rotation and actually written to disk. """ current = load_settings() + + # Snapshot existing remote keys by URL *before* applying the patch so we + # can restore them if the patch contains redacted (empty) key values. + existing_remote_keys: dict[str, str] = { + r["url"]: r.get("key", "") + for r in current.get("remote_instances", []) + if r.get("url") + } + for key in DEFAULT_SETTINGS: if key in patch: current[key] = patch[key] + + # Restore keys that were stripped by redaction. + if "remote_instances" in patch: + for remote in current["remote_instances"]: + url = remote.get("url", "") + if not remote.get("key") and url in existing_remote_keys: + remote["key"] = existing_remote_keys[url] + save_settings(current) return current diff --git a/muxplex/tests/test_settings.py b/muxplex/tests/test_settings.py index 2fef229..1a4004f 100644 --- a/muxplex/tests/test_settings.py +++ b/muxplex/tests/test_settings.py @@ -488,3 +488,160 @@ def test_remote_instances_with_key_round_trip(tmp_path, monkeypatch): save_settings({"remote_instances": instances}) result = load_settings() assert result["remote_instances"] == instances + + +# ============================================================ +# Federation key preservation during PATCH (redaction bug fix) +# ============================================================ + + +def test_patch_preserves_existing_key_when_patch_sends_empty_key(): + """patch_settings() must NOT wipe a remote instance key when the patch sends empty string. + + This is the core redaction bug: GET /api/settings returns key="" for remote + instances (security redaction). When the frontend PATCHes any other field it + sends the redacted remote_instances array back, which should NOT overwrite the + real keys stored on disk. + """ + # Step 1: save settings with a real key present on disk + save_settings( + { + "remote_instances": [ + {"url": "http://spark-2:8088", "name": "spark-2", "key": "h0NtMnGt-real-key"}, + ] + } + ) + + # Step 2: PATCH with the same remote but an empty key (simulating redacted GET response) + patch_settings( + { + "device_name": "new-name", + "remote_instances": [ + {"url": "http://spark-2:8088", "name": "spark-2", "key": ""}, + ], + } + ) + + # Step 3: verify the real key was preserved + loaded = load_settings() + assert loaded["remote_instances"][0]["key"] == "h0NtMnGt-real-key", ( + "patch_settings() must preserve existing key when patch sends empty string; " + f"got: {loaded['remote_instances'][0]['key']!r}" + ) + # Other field changes should still be applied + assert loaded["device_name"] == "new-name" + + +def test_patch_overwrites_key_when_patch_sends_non_empty_key(): + """patch_settings() must update a remote instance key when a new non-empty key is provided. + + Intentional key rotation must still work — sending a real new key should + overwrite the old one. + """ + # Save initial settings with an old key + save_settings( + { + "remote_instances": [ + {"url": "http://spark-1:8088", "name": "spark-1", "key": "old-key-abc"}, + ] + } + ) + + # PATCH with a brand-new non-empty key (intentional rotation) + patch_settings( + { + "remote_instances": [ + {"url": "http://spark-1:8088", "name": "spark-1", "key": "new-key-xyz"}, + ] + } + ) + + loaded = load_settings() + assert loaded["remote_instances"][0]["key"] == "new-key-xyz", ( + "patch_settings() must accept new non-empty key; " + f"got: {loaded['remote_instances'][0]['key']!r}" + ) + + +def test_patch_preserves_key_when_key_field_absent_from_patch(): + """patch_settings() must preserve a remote instance key when the patch omits the key field entirely.""" + save_settings( + { + "remote_instances": [ + {"url": "http://tower:8088", "name": "tower", "key": "tower-secret-key"}, + ] + } + ) + + # PATCH with the remote missing the 'key' field entirely + patch_settings( + { + "remote_instances": [ + {"url": "http://tower:8088", "name": "tower"}, + ] + } + ) + + loaded = load_settings() + assert loaded["remote_instances"][0].get("key") == "tower-secret-key", ( + "patch_settings() must preserve existing key when patch omits key field; " + f"got: {loaded['remote_instances'][0].get('key')!r}" + ) + + +def test_patch_preserves_keys_for_unchanged_remotes_in_multi_remote_list(): + """Keys for all existing remotes are preserved when one unrelated remote is updated.""" + save_settings( + { + "remote_instances": [ + {"url": "http://spark-1:8088", "name": "spark-1", "key": "key-spark-1"}, + {"url": "http://spark-2:8088", "name": "spark-2", "key": "key-spark-2"}, + {"url": "http://cortex:8088", "name": "cortex", "key": "key-cortex"}, + ] + } + ) + + # Frontend sends back all three remotes with redacted empty keys, just changing a name + patch_settings( + { + "remote_instances": [ + {"url": "http://spark-1:8088", "name": "spark-1-renamed", "key": ""}, + {"url": "http://spark-2:8088", "name": "spark-2", "key": ""}, + {"url": "http://cortex:8088", "name": "cortex", "key": ""}, + ] + } + ) + + loaded = load_settings() + remotes_by_url = {r["url"]: r for r in loaded["remote_instances"]} + + assert remotes_by_url["http://spark-1:8088"]["key"] == "key-spark-1", ( + f"spark-1 key must be preserved, got: {remotes_by_url['http://spark-1:8088']['key']!r}" + ) + assert remotes_by_url["http://spark-2:8088"]["key"] == "key-spark-2", ( + f"spark-2 key must be preserved, got: {remotes_by_url['http://spark-2:8088']['key']!r}" + ) + assert remotes_by_url["http://cortex:8088"]["key"] == "key-cortex", ( + f"cortex key must be preserved, got: {remotes_by_url['http://cortex:8088']['key']!r}" + ) + # Non-key field change should still be applied + assert remotes_by_url["http://spark-1:8088"]["name"] == "spark-1-renamed" + + +def test_patch_new_remote_with_key_is_saved(): + """A newly added remote instance with a key is saved correctly.""" + save_settings({"remote_instances": []}) + + patch_settings( + { + "remote_instances": [ + {"url": "http://new-host:8088", "name": "new-host", "key": "brand-new-key"}, + ] + } + ) + + loaded = load_settings() + assert len(loaded["remote_instances"]) == 1 + assert loaded["remote_instances"][0]["key"] == "brand-new-key", ( + f"New remote with key must be saved; got: {loaded['remote_instances'][0]['key']!r}" + )