mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-10-04 22:35:08 +00:00
* fix(config): load_config hands each caller a private copy ConfigManager.load_config() returned its cached self.config itself (the mtime fast path from #410 kept the full path's aliasing). Web handlers edit what they load and then validate: the plugin form save applies the posted fields to the loaded section (a shallow .copy(), so nested dicts were the cache's own), and save_main_config sets its checkboxes before it checks auto_update_channel. When the save was refused, the edit stayed in the cache the fast path serves, and the next save of any other setting wrote it to config.json: the refused value, and a nested secret typed into the same form (mqtt.password, league.espn_s2, flightaware.api_key) in plain text, since it never reached config_secrets.json to be stripped. The form also reloaded showing the refused values. load_config() now returns a private copy on both paths, and save_config/save_config_atomic keep a copy of what they were given, so nothing a caller edits reaches the cache unless it is saved. Fixing it here rather than in each handler covers every route that edits before it validates. No caller relies on editing the cache without saving: every src/ and web_interface/ caller either reads, or saves the dict it edited. get_config() still returns the live dict for the display process's readers. The copy is a pickle round trip: on a Pi 4 with its real 64 KiB config, 2.0 ms against 6.9 ms for copy.deepcopy (json round trip 3.4 ms). Two tests asserted the aliasing itself and now assert a copy. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(web): GET /plugins/config masks secrets and refuses core sections The route returned the plugin's section as load_config() has it, with config_secrets.json merged in: API keys and tokens went out in plain text. #276 masked them here; #330's rewrite of the route dropped it, while the settings page and GET /config/secrets kept masking. It also took any plugin_id, so ?plugin_id=web_auth returned the login's cookie-signing key and password hash, and ?plugin_id=github the Plugin Store token, which GET /config/main strips and redacts. The route now refuses what _non_plugin_id_error refuses for reset and uninstall (core sections, malformed ids) with a 400, and blanks x-secret fields with mask_secret_fields after the defaults merge, as the page does. A plugin with no schema has its credential-named fields blanked by _redact_credentials, as GET /config/main does. Blank rather than the bullets of GET /config/secrets: the save drops a blank secret as "unchanged" (remove_empty_secrets) but would store the bullets, so the response must post back as it came. Tested: GET, then POST the response unchanged, keeps every stored secret. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(web): parse a table row's cells against the list's item schema An array of objects drawn as a table posts each cell as "cities.0.timezone". _get_schema_property stopped at "cities" (an array, not an object with properties), so _parse_form_value_with_schema got no schema for the cell and guessed: a blank optional text cell became None and a text cell holding digits became an int. Validation refused both, so every save of the page failed for as long as such a row existed -- geochron's city without a timezone, a countdown named "2027". A secret cell is always drawn blank, so a plugin with secrets in its rows could not be saved from the form at all. The lookup now steps from an index segment into the array's items: to the item schema itself for "color.2", into its properties for a row cell. Number, boolean and required cells convert as before. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(web): a blank secret field saves as "unchanged", required or not The settings page draws a stored secret blank (mask_secret_fields) and posts the blank back. _parse_form_value_with_schema turned a blank optional string into "" -- which the save drops as unchanged (remove_empty_secrets) -- but a blank required one into None. For a secret that is required with no default (youtube-stats' api_key) that None failed validation, so every save of the page was refused until the key was typed in again. A blank text secret (x-secret, type string) now parses to "", whatever its required list says; a list or object secret keeps getting [] or {}, which the save drops the same way. Not _SKIP_FIELD: skipping keeps the value load_config() merged in, and the save would then write it back to config_secrets.json -- after a secret change the cached section can still hold the old one, so that write reverted it. A test covers that sequence. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(web): POST /plugins/config refuses core sections and malformed ids Reset and uninstall check the plugin id with _non_plugin_id_error; the save did not. {"plugin_id": "display", "config": {...}} found no schema, so nothing was validated or filtered, and the body was merged into the core display section along with "enabled": true -- rows: "banana" included. A plugin_id that was not a string (a list, an object, a number) reached config.get() or the schema lookup, raised TypeError, and came back as a 500. Both the JSON and the form path now call _non_plugin_id_error first and answer its 400. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(web): a text field keeps "true", "[1, 2]" and "{}" as typed _parse_form_value_with_schema guessed before it consulted the schema: "true"/"false" became booleans, and a value starting with "[" or "{" that parsed as JSON became a list or object, whatever the field's type. A text setting holding "true", "False", "[1, 2]" or "{}" was then refused by validation ("Expected type string, got bool"), and the save with it. A field whose schema type is string, or string-or-null, now returns the posted text as it came. Every other type goes through the conversions as before; numbers in text fields were already left alone. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(config): copy the cached config without pickle _private_copy was a pickle round trip. It only ever unpickled bytes it had just made from our own dict, so nothing untrusted reached it, but it put pickle in the config path and Codacy failed the PR for it (B301/B403). The config is JSON data, so copying its dicts and lists is a full copy; every other value is immutable. Measured on ledpi (Pi 4) with its real 60 KiB config: 2.11 ms, against 1.92 ms for pickle and 6.75 ms for copy.deepcopy. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs(changelog): describe the config copy without pickle Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
363 lines
16 KiB
Python
363 lines
16 KiB
Python
"""
|
|
Tests for the ConfigManager secrets round-trip and the load_config fast path.
|
|
|
|
The contract under test: config_secrets.json values are deep-merged INTO the
|
|
in-memory config at load time, and stripped back OUT before anything is
|
|
written to config.json — so secrets live in exactly one file on disk. This
|
|
suite pins that round-trip plus its sharp edges, including the guard that a
|
|
save REFUSES (ConfigError) when the secrets file exists but can't be loaded,
|
|
rather than leaking merged secrets into config.json in plaintext.
|
|
|
|
Complements test_config_manager.py, which covers loading/migration/validation.
|
|
"""
|
|
|
|
import json
|
|
import os
|
|
|
|
import pytest
|
|
|
|
from src.config_manager import ConfigManager
|
|
from src.exceptions import ConfigError
|
|
|
|
|
|
def make_manager(tmp_path, config=None, secrets=None):
|
|
"""A ConfigManager over tmp_path files, template migration neutralized."""
|
|
config_file = tmp_path / "config.json"
|
|
secrets_file = tmp_path / "config_secrets.json"
|
|
config_file.write_text(json.dumps(config if config is not None else {}))
|
|
if secrets is not None:
|
|
secrets_file.write_text(json.dumps(secrets))
|
|
manager = ConfigManager(config_path=str(config_file),
|
|
secrets_path=str(secrets_file))
|
|
# Point the (CWD-relative) template at nothing so migration never runs —
|
|
# these tests assert exact on-disk contents.
|
|
manager.template_path = str(tmp_path / "no-template.json")
|
|
return manager
|
|
|
|
|
|
class TestLoadMergesSecrets:
|
|
def test_secrets_deep_merged_into_config(self, tmp_path):
|
|
manager = make_manager(
|
|
tmp_path,
|
|
config={"weather": {"city": "Austin"}, "timezone": "UTC"},
|
|
secrets={"weather": {"api_key": "s3cret"}},
|
|
)
|
|
loaded = manager.load_config()
|
|
assert loaded["weather"] == {"city": "Austin", "api_key": "s3cret"}
|
|
assert loaded["timezone"] == "UTC"
|
|
|
|
def test_secret_scalar_overrides_config_value(self, tmp_path):
|
|
manager = make_manager(
|
|
tmp_path,
|
|
config={"weather": {"api_key": "YOUR_API_KEY"}},
|
|
secrets={"weather": {"api_key": "real-key"}},
|
|
)
|
|
assert manager.load_config()["weather"]["api_key"] == "real-key"
|
|
|
|
def test_missing_secrets_file_loads_config_fine(self, tmp_path):
|
|
manager = make_manager(tmp_path, config={"timezone": "UTC"})
|
|
assert manager.load_config() == {"timezone": "UTC"}
|
|
|
|
def test_corrupt_secrets_file_loads_config_without_secrets(self, tmp_path):
|
|
manager = make_manager(tmp_path, config={"timezone": "UTC"})
|
|
(tmp_path / "config_secrets.json").write_text("{not json")
|
|
loaded = manager.load_config()
|
|
assert loaded["timezone"] == "UTC"
|
|
|
|
|
|
class TestSaveStripsSecrets:
|
|
def test_round_trip_keeps_secrets_out_of_config_json(self, tmp_path):
|
|
manager = make_manager(
|
|
tmp_path,
|
|
config={"weather": {"city": "Austin"}},
|
|
secrets={"weather": {"api_key": "s3cret"}},
|
|
)
|
|
loaded = manager.load_config()
|
|
assert loaded["weather"]["api_key"] == "s3cret" # merged in memory
|
|
|
|
manager.save_config(loaded)
|
|
|
|
on_disk = json.loads((tmp_path / "config.json").read_text())
|
|
assert "api_key" not in on_disk.get("weather", {})
|
|
assert on_disk["weather"]["city"] == "Austin"
|
|
# In-memory config still carries the secret for runtime use.
|
|
assert manager.config["weather"]["api_key"] == "s3cret"
|
|
|
|
def test_group_dropped_when_only_secrets_remain(self, tmp_path):
|
|
# _strip_secrets_recursive drops a group entirely when nothing
|
|
# non-secret is left in it.
|
|
manager = make_manager(
|
|
tmp_path,
|
|
config={},
|
|
secrets={"weather": {"api_key": "s3cret"}},
|
|
)
|
|
manager.save_config({"weather": {"api_key": "s3cret"}, "timezone": "UTC"})
|
|
on_disk = json.loads((tmp_path / "config.json").read_text())
|
|
assert on_disk == {"timezone": "UTC"}
|
|
|
|
def test_scalar_secret_key_stripped_at_top_level(self, tmp_path):
|
|
manager = make_manager(tmp_path, config={}, secrets={"token": "t"})
|
|
manager.save_config({"token": "t", "timezone": "UTC"})
|
|
on_disk = json.loads((tmp_path / "config.json").read_text())
|
|
assert on_disk == {"timezone": "UTC"}
|
|
|
|
def test_corrupt_secrets_file_refuses_save_no_plaintext_leak(self, tmp_path):
|
|
# Regression guard: when the secrets file exists but is corrupt at
|
|
# save time, stripping is impossible — the save must raise instead of
|
|
# writing the merged secrets into config.json in plaintext (the
|
|
# historical behavior).
|
|
manager = make_manager(
|
|
tmp_path,
|
|
config={"weather": {"city": "Austin"}},
|
|
secrets={"weather": {"api_key": "s3cret"}},
|
|
)
|
|
loaded = manager.load_config()
|
|
(tmp_path / "config_secrets.json").write_text("{corrupt")
|
|
|
|
with pytest.raises(ConfigError):
|
|
manager.save_config(loaded)
|
|
|
|
# On-disk config untouched: no secret leaked.
|
|
on_disk = json.loads((tmp_path / "config.json").read_text())
|
|
assert "api_key" not in on_disk.get("weather", {})
|
|
|
|
def test_corrupt_secrets_file_refuses_atomic_save_too(self, tmp_path):
|
|
# Same refusal on the atomic save path, which shared the leak.
|
|
manager = make_manager(
|
|
tmp_path,
|
|
config={"weather": {"city": "Austin"}},
|
|
secrets={"weather": {"api_key": "s3cret"}},
|
|
)
|
|
loaded = manager.load_config()
|
|
(tmp_path / "config_secrets.json").write_text("{corrupt")
|
|
|
|
with pytest.raises(ConfigError):
|
|
manager.save_config_atomic(loaded)
|
|
|
|
on_disk = json.loads((tmp_path / "config.json").read_text())
|
|
assert "api_key" not in on_disk.get("weather", {})
|
|
|
|
|
|
class TestLoadFastPath:
|
|
def test_unchanged_files_return_cached_dict(self, tmp_path):
|
|
manager = make_manager(tmp_path, config={"timezone": "UTC"})
|
|
first = manager.load_config()
|
|
second = manager.load_config()
|
|
# A copy of the cached dict, never the dict itself; that it is not
|
|
# re-read is test_config_load_cache's test_unchanged_files_are_not_reread
|
|
assert second == first
|
|
assert second is not first
|
|
|
|
def test_touching_secrets_file_invalidates_cache(self, tmp_path):
|
|
manager = make_manager(
|
|
tmp_path,
|
|
config={"weather": {}},
|
|
secrets={"weather": {"api_key": "old"}},
|
|
)
|
|
assert manager.load_config()["weather"]["api_key"] == "old"
|
|
|
|
secrets_file = tmp_path / "config_secrets.json"
|
|
secrets_file.write_text(json.dumps({"weather": {"api_key": "new"}}))
|
|
# Force a different mtime_ns in case the write landed within the
|
|
# filesystem's timestamp granularity.
|
|
os.utime(secrets_file, ns=(1, 1))
|
|
|
|
assert manager.load_config()["weather"]["api_key"] == "new"
|
|
|
|
def test_same_mtime_same_size_change_served_stale(self, tmp_path):
|
|
# Characterized fast-path blind spot: the signature is (mtime_ns,
|
|
# size) only, so a same-length content swap with a forged identical
|
|
# mtime is not detected. Real writes bump mtime_ns, so this is
|
|
# acceptable — but it is a contract worth pinning.
|
|
manager = make_manager(tmp_path, config={"timezone": "AAA"})
|
|
config_file = tmp_path / "config.json"
|
|
os.utime(config_file, ns=(1_000_000_000, 1_000_000_000))
|
|
manager._loaded_sig = None
|
|
first = manager.load_config()
|
|
assert first["timezone"] == "AAA"
|
|
|
|
config_file.write_text(json.dumps({"timezone": "BBB"})) # same length
|
|
os.utime(config_file, ns=(1_000_000_000, 1_000_000_000))
|
|
|
|
assert manager.load_config()["timezone"] == "AAA" # stale, by design
|
|
|
|
|
|
class TestArraySecretStripAndMerge:
|
|
"""Array-item secrets round-trip (parallel-placeholder lists).
|
|
|
|
secret_helpers.separate_secrets emits array secrets as a list parallel
|
|
to the regular list, with {} for items that carry no secrets. Strip
|
|
must remove the secret fields from config.json while preserving item
|
|
indices; load must merge them back into the right items. The regular
|
|
list's length is authoritative in both directions.
|
|
"""
|
|
|
|
def test_strip_removes_array_item_secrets_keeps_indices(self, tmp_path):
|
|
manager = make_manager(tmp_path)
|
|
data = {"plugin": {"accounts": [
|
|
{"name": "a", "token": "ta"},
|
|
{"name": "b"},
|
|
]}}
|
|
secrets = {"plugin": {"accounts": [{"token": "ta"}, {}]}}
|
|
stripped = manager._strip_secrets_recursive(data, secrets)
|
|
assert stripped == {"plugin": {"accounts": [{"name": "a"}, {"name": "b"}]}}
|
|
|
|
def test_strip_keeps_all_placeholder_items(self, tmp_path):
|
|
# Even when every item strips to nothing extra, the list survives
|
|
# with its indices — required for merge-on-load alignment.
|
|
manager = make_manager(tmp_path)
|
|
data = {"accounts": [{"token": "t1"}, {"token": "t2"}]}
|
|
secrets = {"accounts": [{"token": "t1"}, {"token": "t2"}]}
|
|
stripped = manager._strip_secrets_recursive(data, secrets)
|
|
assert stripped == {"accounts": [{}, {}]}
|
|
|
|
def test_strip_whole_scalar_array_secret_drops_key(self, tmp_path):
|
|
# A list of secret scalars is a whole-key secret, not the parallel
|
|
# shape — the key must vanish from config.json entirely.
|
|
manager = make_manager(tmp_path)
|
|
data = {"recovery_codes": ["a", "b"], "city": "Austin"}
|
|
secrets = {"recovery_codes": ["a", "b"]}
|
|
stripped = manager._strip_secrets_recursive(data, secrets)
|
|
assert stripped == {"city": "Austin"}
|
|
|
|
def test_strip_shape_mismatch_drops_key(self, tmp_path):
|
|
# Conservative contract: if the shapes disagree, never leak.
|
|
manager = make_manager(tmp_path)
|
|
data = {"accounts": {"name": "not-a-list"}}
|
|
secrets = {"accounts": [{"token": "t"}]}
|
|
stripped = manager._strip_secrets_recursive(data, secrets)
|
|
assert stripped == {}
|
|
|
|
def test_strip_ignores_extra_secrets_entries(self, tmp_path):
|
|
# Regular list length is authoritative: a user deleted an item.
|
|
manager = make_manager(tmp_path)
|
|
data = {"accounts": [{"name": "a", "token": "ta"}]}
|
|
secrets = {"accounts": [{"token": "ta"}, {"token": "tb"}]}
|
|
stripped = manager._strip_secrets_recursive(data, secrets)
|
|
assert stripped == {"accounts": [{"name": "a"}]}
|
|
|
|
def test_merge_restores_array_item_secrets(self, tmp_path):
|
|
manager = make_manager(tmp_path)
|
|
target = {"accounts": [{"name": "a"}, {"name": "b"}]}
|
|
manager._deep_merge(target, {"accounts": [{"token": "ta"}, {}]})
|
|
assert target == {"accounts": [
|
|
{"name": "a", "token": "ta"},
|
|
{"name": "b"},
|
|
]}
|
|
|
|
def test_merge_ignores_extra_secrets_entries_with_warning(self, tmp_path, caplog):
|
|
manager = make_manager(tmp_path)
|
|
target = {"accounts": [{"name": "a"}]}
|
|
with caplog.at_level("WARNING"):
|
|
manager._deep_merge(
|
|
target, {"accounts": [{"token": "ta"}, {"token": "ghost"}]})
|
|
assert target == {"accounts": [{"name": "a", "token": "ta"}]}
|
|
assert any("longer than the config list" in r.message for r in caplog.records)
|
|
|
|
def test_merge_non_dict_item_replaced_by_secret(self, tmp_path):
|
|
# Shape drift inside the list: the secret wins for that index.
|
|
manager = make_manager(tmp_path)
|
|
target = {"accounts": ["oddball", {"name": "b"}]}
|
|
manager._deep_merge(target, {"accounts": [{"token": "ta"}, {}]})
|
|
assert target == {"accounts": [{"token": "ta"}, {"name": "b"}]}
|
|
|
|
def test_merge_whole_scalar_array_still_replaces(self, tmp_path):
|
|
# Legacy behavior preserved: a non-parallel list replaces wholesale.
|
|
manager = make_manager(tmp_path)
|
|
target = {"recovery_codes": ["old"]}
|
|
manager._deep_merge(target, {"recovery_codes": ["new1", "new2"]})
|
|
assert target == {"recovery_codes": ["new1", "new2"]}
|
|
|
|
def test_full_save_load_round_trip(self, tmp_path):
|
|
# End to end on real files: save strips array secrets out of
|
|
# config.json; load merges them back into the right items.
|
|
manager = make_manager(
|
|
tmp_path,
|
|
config={"plugin": {"accounts": [
|
|
{"name": "a", "token": "s3cret-a"},
|
|
{"name": "b", "token": "s3cret-b"},
|
|
]}},
|
|
secrets={"plugin": {"accounts": [
|
|
{"token": "s3cret-a"}, {"token": "s3cret-b"},
|
|
]}},
|
|
)
|
|
loaded = manager.load_config()
|
|
assert loaded["plugin"]["accounts"][0]["token"] == "s3cret-a"
|
|
|
|
manager.save_config(loaded)
|
|
|
|
raw = (tmp_path / "config.json").read_text()
|
|
assert "s3cret" not in raw
|
|
on_disk = json.loads(raw)
|
|
assert on_disk["plugin"]["accounts"] == [{"name": "a"}, {"name": "b"}]
|
|
|
|
# A fresh manager (constructed directly — make_manager would
|
|
# overwrite the just-saved config.json) re-merges from the secrets
|
|
# file on load.
|
|
fresh = ConfigManager(config_path=str(tmp_path / "config.json"),
|
|
secrets_path=str(tmp_path / "config_secrets.json"))
|
|
fresh.template_path = str(tmp_path / "no-template.json")
|
|
reloaded = fresh.load_config()
|
|
assert reloaded["plugin"]["accounts"] == [
|
|
{"name": "a", "token": "s3cret-a"},
|
|
{"name": "b", "token": "s3cret-b"},
|
|
]
|
|
|
|
def test_whole_item_secret_list_never_leaks_values(self, tmp_path):
|
|
# When the ENTIRE array item is secret (schema marks both key[]
|
|
# and key[].field), separate_secrets stores the full item dicts in
|
|
# the secrets file. That shape also matches the parallel-list
|
|
# discriminator — which is safe: strip drops every leaf key that
|
|
# appears in the secret item, so only empty {} skeletons (item
|
|
# count, no values) can reach config.json, and merge-on-load
|
|
# restores the full items from those skeletons.
|
|
from src.web_interface.secret_helpers import (
|
|
find_secret_fields, separate_secrets)
|
|
schema_props = {"accounts": {
|
|
"type": "array",
|
|
"items": {"type": "object", "x-secret": True, "properties": {
|
|
"id": {"type": "string"},
|
|
"token": {"type": "string", "x-secret": True},
|
|
}},
|
|
}}
|
|
paths = find_secret_fields(schema_props)
|
|
assert paths == {"accounts[]", "accounts[].token"}
|
|
full = {"accounts": [{"id": "i1", "token": "s3cret-a"},
|
|
{"id": "i2", "token": "s3cret-b"}]}
|
|
_, secrets = separate_secrets(full, paths)
|
|
assert secrets == full # whole items are secret
|
|
|
|
manager = make_manager(tmp_path)
|
|
stripped = manager._strip_secrets_recursive(full, secrets)
|
|
assert stripped == {"accounts": [{}, {}]}
|
|
|
|
raw = json.dumps(stripped)
|
|
assert "s3cret" not in raw and "i1" not in raw
|
|
|
|
manager._deep_merge(stripped, secrets)
|
|
assert stripped == full # round trip restores the items
|
|
|
|
|
|
class TestLoadWithoutPosixOwnershipApis:
|
|
"""A secrets file must load on platforms that have no uid/gid at all.
|
|
|
|
load_config() chgrp's the secrets file to the shared group before reading
|
|
it, to self-heal a root-written file the non-root web user can't read.
|
|
That helper is documented as best-effort, but it looked up os.geteuid
|
|
unguarded -- absent on Windows -- and the resulting AttributeError is not
|
|
an OSError, so it escaped every except clause on the way out. The symptom
|
|
was a ConfigError from load_config on any Windows checkout carrying a
|
|
config/config_secrets.json, which took `import web_interface.app` with it.
|
|
"""
|
|
|
|
def test_secrets_still_merge_without_geteuid(self, tmp_path, monkeypatch):
|
|
monkeypatch.delattr(os, "geteuid", raising=False)
|
|
monkeypatch.delattr(os, "chown", raising=False)
|
|
manager = make_manager(
|
|
tmp_path,
|
|
config={"weather": {"city": "Austin"}},
|
|
secrets={"weather": {"api_key": "s3cret"}},
|
|
)
|
|
|
|
assert manager.load_config()["weather"] == {"city": "Austin", "api_key": "s3cret"}
|