mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-10-07 07:36:37 +00:00
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>
This commit is contained in:
@@ -618,6 +618,57 @@ policies are unchanged.
|
||||
the plugin leaves rotation until the cooldown ends, the same as a raising
|
||||
`update()`. The display still moves straight on to the next mode. A hung
|
||||
`display()` is still recorded once, as a hang.
|
||||
- A plugin settings save that failed validation no longer leaks into the next
|
||||
save. `ConfigManager.load_config()` returned its cached config itself (the
|
||||
fast path from #410), so the form save's edits went into the cache before
|
||||
validation ran, and a refused save left them there. The next save of any
|
||||
other setting (another plugin's, a plugin toggle, the schedule) wrote them
|
||||
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, because it had never reached config_secrets.json to be stripped.
|
||||
The form also reloaded showing the refused values. `load_config()` now
|
||||
returns a private copy, and the saves keep one, so nothing a caller edits
|
||||
reaches the cache unless it is saved. The copy duplicates only the dicts
|
||||
and lists (every other JSON value is immutable): 2.1 ms for a real 60 KiB
|
||||
config on a Pi 4, against 6.8 ms for `copy.deepcopy`.
|
||||
- `GET /api/v3/plugins/config` no longer returns secrets. It sent back the
|
||||
plugin's section with config_secrets.json merged in, API keys and tokens
|
||||
in plain text: the masking #276 added was dropped in #330. It also took
|
||||
any id, so `?plugin_id=web_auth` returned the login's cookie-signing key
|
||||
and password hash and `?plugin_id=github` the Plugin Store token. Secret
|
||||
fields now come back blank, as the settings page renders them, and a
|
||||
plugin with no schema has its credential-named fields blanked, as
|
||||
`GET /config/main` does. Blank rather than the `••••••••` of
|
||||
`GET /config/secrets`, because the save reads a blank secret as
|
||||
"unchanged", so a client can post the response back without erasing
|
||||
one. Core sections and malformed ids get a 400, as they already did from
|
||||
reset and uninstall.
|
||||
- Plugin settings with a table (a list of rows, such as geochron's cities
|
||||
or the countdowns) save again when a text cell is blank or holds only
|
||||
digits. A row posts its cells as `cities.0.timezone`, and the schema
|
||||
lookup stopped at the list, so each cell was parsed with no schema: a
|
||||
blank optional text cell became null, and a name like "2027" became a
|
||||
number. Either failed validation, and every save of the page failed for
|
||||
as long as the row existed. A plugin with a secret in its rows could not
|
||||
be saved from the page at all, since the secret cell is drawn blank. The
|
||||
lookup now steps from the index into the list's item schema.
|
||||
- A plugin whose API key is required and has no default (youtube-stats)
|
||||
can be saved from its settings page without typing the key in again. The
|
||||
page draws a stored secret blank and posts the blank back; for a required
|
||||
secret the save read that blank as null, failed validation, and refused
|
||||
every save of the page. A blank secret field now means "unchanged", as it
|
||||
already did for an optional one.
|
||||
- `POST /api/v3/plugins/config` refuses a core section or a malformed
|
||||
plugin id with a 400, as reset and uninstall already did.
|
||||
`{"plugin_id": "display", ...}` merged unvalidated values into the core
|
||||
display section (and added `"enabled": true` to it), and an id that was
|
||||
not a string answered with a 500.
|
||||
- A plugin text setting saves what was typed when that looks like a
|
||||
boolean or JSON. The form save tried `true`/`false` and `[...]`/`{...}`
|
||||
before it looked at the schema, so a text field holding "true", "False",
|
||||
"[1, 2]" or "{}" was stored as a boolean, list or object, and the save
|
||||
failed validation. Text fields, nullable ones included, are now taken as
|
||||
typed; other types convert as before.
|
||||
- A WiFi notice (such as "Connected to HomeNet" or "AP mode on") now shows
|
||||
within about a second of being posted. It was only checked between
|
||||
screens, so a 5 s notice posted during a 20 s screen expired before that
|
||||
|
||||
Reference in New Issue
Block a user