ken/muxplex
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix: preserve federation keys when PATCH sends redacted empty values

Browse Source 
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
This commit is contained in:
Brian Krabach
2026-04-02 20:20:32 -07:00
parent b0c1d0ab8d
commit 082d03036f
2 changed files with 182 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files
muxplex/settings.py
+25
View File
@@ -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
muxplex/tests/test_settings.py
+157
View File
@@ -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}"
)