Files
LEDMatrix/test/test_config_manager_secrets.py
ChuckandClaude Opus 5.5 0b039c875f fix(web): plugin settings endpoints - a refused save no longer leaks into config.json; GET masks secrets (#742)
* 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>
2026-10-03 22:30:16 -04:00

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"}