3 Commits

Include renames

Author	SHA1	Message	Date
ChuckandClaude Opus 5.5	3967a6cffc	fix(security): re-harden root sudo helpers; installer fixes; ARCHITECTURE and PERMISSIONS docs (#640) ... * docs: add ARCHITECTURE and PERMISSIONS guides ARCHITECTURE.md maps the processes, the state the display and web services share through the cache, the display loop, the plugin system, the web UI and the update path, with links into the code and a where-to-start table. PERMISSIONS.md lists who owns what after install, both sudoers files (and why iptables is not granted), the polkit rule, and which scripts/fix_perms script to run as which user. Both are linked from the docs index, along with the MQTT bridge README and src/common/README.md. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs: correct stale setup, service and troubleshooting claims - README: quick actions run systemctl on ledmatrix.service (run.py), not display_controller.py; use_short_date_format has no effect; the installer uses system pip with --break-system-packages, not a venv. - CONFIG_DEBUGGING: LEDMATRIX_DEBUG must be "true"; logs are in journald. - GETTING_STARTED, WEB_INTERFACE_GUIDE, TROUBLESHOOTING: enabling a plugin, plugin settings, brightness and Vegas settings apply without a restart; matrix hardware settings still need one. - TROUBLESHOOTING: install dependencies with sudo so the root service sees them; point permission problems at PERMISSIONS.md instead of a project-wide chown. - ADVANCED_FEATURES: real BackgroundDataService stats keys; Vegas hooks return VegasDisplayMode and None falls back to capture; cache files are 0660; fix_web_permissions.sh runs as the web user and does not touch sudoers. - STARLARK_APPS_GUIDE: only the linux-arm64 pixlet binary is downloaded. - HOW_TO_RUN_TESTS: test class examples that exist. - CLAUDE.md: PluginStoreManager, plugin_dirs.py, monorepo installs via the Trees API with ZIP fallback, requirements.txt is optional. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs: mark deprecated plugin APIs and state manifest fields once Methods @deprecated("3.7.0") (the set pinned in test_deprecation.py) were shown as current API in the quick reference, API reference, advanced guide, development guide and FONT_MANAGER. Each is now marked deprecated with its replacement. FONT_MANAGER is rewritten around the current API; the override editor is gone and override methods are deprecated. Required manifest fields were stated three different ways. The API reference now has one section: the 7 schema-required fields, the 4 the store refuses without, class_name for the loader, and the 8 to set. The other guides link to it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs: document every src/common module and every widget - src/common/README.md covered 7 of 17 modules. It now has a table of all of them (purpose, whether plugins import it, release to floor on), a short entry each, and logging advice that matches the code. - SPORTS_UNIFICATION listed two shared modules and called sports_helpers the first; it now lists all six. - The widgets README lists all 28 registered widgets plus the support files, and absorbs the parts that only docs/widget-guide.md had (x-options.labels, x-advanced, x-display hidden, plugin-file-manager). docs/widget-guide.md is now a pointer to it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(security): fix_web_permissions.sh re-hardens the root sudo helpers The script chowns the whole project to the web user. That included scripts/fix_perms/safe_plugin_rm.sh and safe_pip_install.sh -- the two helpers /etc/sudoers.d/ledmatrix_web lets the web user run as root -- so running it turned both into a root shell for whoever can edit them. It also re-grouped config_secrets.json away from ledmatrix. After the chown it now does what first_time_install.sh's Steps 11 and 11.1 do: helpers back to root:root 755, and config_secrets.json back to the web unit's User=:ledmatrix 640. Each step is non-fatal and prints the manual command if it fails. Also fixes what the script and its docs claimed: it never configured sudoers, its closing hint pointed at ./configure_web_sudo.sh (wrong path), and the README and ADVANCED_FEATURES.md said to run it with sudo, which it refuses. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(security): validate and harden every sudoers drop-in the scripts write configure_wifi_permissions.sh copied its rules into /etc/sudoers.d/ledmatrix_wifi without `visudo -c`. A malformed drop-in makes sudo refuse every command for every user, which on a headless Pi leaves no way back in. It now checks first and leaves the installed file alone when the rules do not parse, as the other two writers do. (It already used mktemp, so that part of the review did not apply.) It also grants the two literal commands wifi_manager.py runs for NetworkManager's shared-mode dnsmasq drop-in -- `cp /tmp/ledmatrix-nm-dnsmasq.conf .../dnsmasq-shared.d/ledmatrix-captive.conf` and `rm -f` of that file. The directory's mkdir was granted, the file was not. Both are pinned in test_sudo_allowlist_covers_calls.py. configure_web_sudo.sh wrote its rules to /tmp/ledmatrix_web_sudoers_$$, a predictable name in a world-writable directory; it now uses mktemp with an EXIT trap, as first_time_install.sh does. It sets mode 440 on the installed file instead of leaving the temp file's mode, and finds visudo in /usr/sbin when that is not on the user's PATH, which skipped the check silently. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(install): escape the project path in the DNS-fix and MQTT unit renderers install_dns_fix.sh and install_mqtt_bridge.sh substituted __PROJECT_ROOT_DIR__ with the raw path, while the other three renderers go through sed_escape_replacement from lib_systemd_render.sh. A checkout under a path containing `&`, `\` or `|` rendered a corrupted unit from these two only. Both now source the helper and use it, and a test checks that every placeholder substitution in scripts/install uses an escaped value. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(install): stop the installer scripts reporting things that are not true - first_time_install.sh printed "Password: ledmatrix123" for the setup access point. wifi_manager creates it as an open network ("No password" on the panel), so it now says so. - Step 10.1 printed "✓ WiFi management permissions configured" straight after its own failure message; install_wifi_monitor.sh printed "✓ Package installation completed" after a failed apt install. The tick now only follows success. - Step 7 printed "Web dependencies already installed ... in Step 5" in the one branch that runs because Step 5 did not install them, then created .web_deps_installed on that basis. It now warns and leaves the marker off so the next run retries, as the comment below it intends. - check_system_compatibility.sh called Debian 12 Bookworm "full compatibility confirmed" while first_time_install.sh refuses anything but Debian 13. Bookworm, older Debian and non-Debian systems are now errors. Its counters used ((X++)), which under `set -e` exits the script at the first warning or error (the expression is 0), so the check never reached its summary on any system with one. - configure_web_sudo.sh and configure_wifi_permissions.sh finished by testing `sudo -n test -f ...` and `sudo -n nmcli device status`, neither of which is granted, so they always reported a failure. They now ask `sudo -n -l` about commands the new rules do grant, which checks the rule without running anything. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(install): print the completion summary before rebooting With -y -- and so for every one-shot `curl | bash` install, which always passes -y -- first_time_install.sh ran `reboot` about 180 lines before its "Installation Complete / Web UI Access" summary. reboot returns at once, so the summary printed while the Pi was going down and the SSH session usually dropped before the web UI address could be read. The reboot block moves, unchanged, to the very end of the script. The interactive prompt now also follows the summary. Because the summary now runs before the -y reboot, its one command that could fail under `set -Eeuo pipefail` (the SSID lookup, when nmcli reports a connected device but no active network line) gets `|| true`; a missing SSID was already handled as "SSID unknown". one-shot-install.sh prints its "Next steps" after the installer returns, by which time the reboot is under way, so it now says so, and README's Quick Install mentions the automatic reboot. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * chore(scripts): correct wrong comments and messages, drop dead code No behaviour change except the output text noted below. - 2775 is setgid, not the sticky bit (first_time_install.sh Step 3.1, fix_plugin_permissions.sh), and root needs no "PWM hardware access" to plugin files. - The 777 comments in first_time_install.sh Step 3's fallback and fix_assets_permissions.sh said root needs it to write. Root ignores mode bits; the comments now say what 777 actually opens. The 777 itself is unchanged. - apt_remove ends in `|| true`, so Step 12's "Some packages could not be removed" branch could never run; it is gone and the helper stays non-fatal. - detect_web_service_user's comment named Step 8 for the web unit (install_service.sh installs it in Step 7.5) and now says which branch actually runs. - Step 5 described an "already installed" check that does not exist; the ACTUAL_USER comment described the re-exec backwards. - on_error printed a literal "\n" before "Common fixes:". - Dead code: one-shot-install.sh's uncalled fix_tmp_permissions, LEDMATRIX_ELEVATED=1 (never read) on the sudo re-exec, and configure_web_sudo.sh's unused PYTHON_PATH, which also made a missing python3 fatal for rules that never mention it. - start_display.sh / stop_display.sh said "for user: <you>"; the service runs as root. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * refactor(fix_perms): fix_cache_permissions.sh uses setup_cache.sh's model There were two models for /var/cache/ledmatrix. setup_cache.sh (the installer's Step 2) and install_web_service.sh share it through the ledmatrix group: root:ledmatrix, 2775, files 660, which is also what DiskCache relies on to give files the directory's group. fix_cache_permissions.sh instead made it 777 and re-grouped it to the invoking user's group, undoing that. It now runs setup_cache.sh for /var/cache/ledmatrix and keeps its own handling of ~/.ledmatrix_cache. Dropped: /var/cache/ledmatrix/ placeholder_logos (nothing reads it) and the checks against the `daemon` user (no service runs as daemon). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * ci: pin actions/checkout in the Claude workflows, drop template comments claude.yml and claude-code-review.yml used actions/checkout@v4 while test.yml and release-version-check.yml pin the v4.2.2 commit SHA; they now pin the same SHA. The commented-out starter-template settings (prompt, claude_args, paths, author filter) are removed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs(scripts): index every script and list removal candidates New scripts/README.md gives one line per top-level script and scripts directory, marked keep, dev-only or diagnostic, and lists the eight scripts nothing in the repo refers to as candidates for removal (kept for now). The install, utils and dev READMEs now list the files they were missing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test: tighten two checks that mutation testing showed were too loose - The wifi sudoers check matched `visudo -c -f "$TEMP_SUDOERS"` in the error report too, so replacing the check with `if false` still passed. It now requires the command as the condition. - The summary test never had the setup access point up, so reinstating the bogus "Password: ledmatrix123" line went unnoticed. A case with hostapd active now checks the AP is described as open. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs(permissions): describe the repaired fix_perms scripts and new WiFi grants Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs(changelog): docs-scripts Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>	2026-09-24 17:31:41 -04:00
ChuckandClaude Opus 5	9c0c0dc851	consolidate(web): credential exposure, secret loss, and the update path (#485) ... * fix(web): stop /config/main handing out every credential it holds The endpoint returned the raw config to anyone who could reach the port, and this web interface has no authentication of any kind. An unauthenticated request against a live rig returned: github.api_token 40 chars incoming-packages.ha_token 183 chars jellyfin-now-playing.api_key 32 chars ledmatrix-weather.api_key 32 chars on-air.mqtt_password 8 chars youtube.api_key 20 chars youtube-stats.api_key 39 chars A GitHub token and a Home Assistant long-lived token among them. Anything on that LAN could read them. The x-secret masking the plugin config endpoints use does not reach here: this route never consults a schema, and core keys such as github.api_token have no schema to carry the marker. Several of the fields above *are* tagged x-secret in their plugin's schema and were still returned in full, which is what rules out the schema route as the fix for this endpoint. Credential-named fields are now blanked. Matching on the name is blunt, and for a whole-config dump that is the right default: anything named like a credential should not leave the process, and a new plugin adding a differently-shaped secret is covered without anyone remembering to tag it. Blanked rather than removed, and safe to blank: POST /config/main merges into the freshly loaded config and writes only the keys it was given, so a client that round-trips this response cannot erase a secret it never saw. The web API suites confirm it -- 81 passing, unchanged. On the test that matters: the first version of this suite exercised the two helpers and nothing else, and reverting the single line that wires the redactor into the route passed all thirty of them. A property asserted on a helper is not a property asserted on the endpoint, and it is the endpoint that is exposed to the network. The added test goes through the view function, and it does fail on that revert. This also corrects an earlier claim of mine. I reported that GET /api/v3/config did not expose these values; that path 404s, so the check proved nothing. The real route is /config/main and it exposed all of them. * fix(web): stop an unrelated config edit from erasing a plugin's secret Saving any field on a plugin's config form destroyed that plugin's stored credential. On a rig with a weather API key, changing the city silently emptied the key, and the plugin stopped working at the next fetch with no indication why. The path had no guard at any step. The config partial masks secrets before rendering (pages_v3.py:740), so the browser posts them back blank; _parse_value deliberately preserves "" for optional string fields; separate_secrets routes that "" into secrets_config, which is a truthy dict; deep_merge writes it over the stored value; save_raw_file_content persists it. The blank does not even need the round-trip. merge_with_defaults injects the schema's api_key default ("") into every save, so a client that never sends the field at all still erases it. test_secret_count_message_counts_top_level_keys was counting exactly that injected blank as a saved secret field -- the visible edge of the bug, pinned as expected behaviour. remove_empty_secrets() already existed for this, with seven unit tests and a docstring describing this precise scenario ("clients will send those empty strings back ... so that existing stored secrets are not overwritten with blanks"). It was never wired into a call site. This wires it into both save paths that merge into the secrets file. A blank now means "unchanged" rather than "delete", which is the same contract the helper's tests already describe. The cost is that a secret can no longer be cleared by emptying the field; clearing needs its own affordance, since a control that erases credentials as a side effect of ordinary edits is not one. Verified by reverting the guard: the new round-trip test then fails with the stored key read back as ''. 262 web tests pass with it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW * fix(web): stop dumping the config and request headers to the journal save_main_config logged its entire POST body and the full request headers at ERROR on every save. The body is the configuration itself, and the headers carry the session cookie, so a routine settings change wrote both to the journal -- at a level that guarantees they survive any sane log filter. The lines are leftover debug output: they say "DEBUG:" in the message while calling logging.error, and they went through the root logger rather than the module logger, bypassing the level configured for this blueprint. Replaced with a debug-level line recording the shape of the request, which is the part with diagnostic value. The local `import logging` went with them; it shadowed a module-level import that was already there. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW * fix(web): stop /config/secrets handing out every credential it holds GET /api/v3/config/secrets returned config_secrets.json in full to anyone who could reach the port, and this interface has no authentication. Probed against a real rig it produced six populated credential fields: a 40-character GitHub token, a 183-character Home Assistant token, and Jellyfin and weather API keys. This is the second door onto the same credentials; #477 closes the first. Masking the response alone would have been worse than the leak. The only client fetches every secret, edits one field and posts all of them back, and save_raw_file_content replaces the file wholesale -- so a masked GET followed by the client's own save would write the mask over every credential the user had not touched. That is why this was left open when the leak was found; it needs both halves. Read side: mask_all_secret_values(), which already existed for exactly this endpoint -- its docstring names it -- and had never been wired to a call site. It leaves empty values and YOUR_* placeholders alone, so a client can still tell "set" from "not set" without being told the secret. Write side: strip the echoed mask and blanks from the submission, then merge onto what is stored, so "unchanged" means unchanged. The cost is that a secret can no longer be cleared by blanking it; that wants its own affordance, since a control that erases credentials as a side effect of saving an unrelated one is not one. Browser side: the token field is now left empty rather than filled from the response. Filling it with the mask would have stored eight bullet characters as the token the next time the user pressed Save, and filling it with the real value is the thing being fixed. It reports whether a token is saved instead. Verified end to end through the Flask endpoints, not the helpers. Reverting the masking fails the leak tests; reverting the merge fails the preservation tests; both halves are independently guarded. 278 web tests pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW * fix(web): stop reporting "no update" when the update check could not run check-update returned update_available=False whenever git failed. The banner is the only route to the update button, so a checkout git refuses to touch looked exactly like a current one -- permanently, with nothing on screen to act on and only a log line recording why. The common cause is an install performed as root. scripts/install/one-shot-install.sh clones into ${HOME}/LEDMatrix, never consults SUDO_USER, and contains no chown at all, while its own error text suggests running the whole thing under sudo. The result is a root-owned checkout, and on a rig this is what every git command in it does: fatal: detected dubious ownership in repository at '...' including the fetch this endpoint runs. Verified on real hardware rather than assumed. A failed check now reports check_failed with a message the user can act on -- for dubious ownership, the chown that fixes it. The banner shows that message instead of hiding itself, with the update button suppressed since updating cannot work until the cause is fixed. The success path is untouched. This does not fix the installer, which is the real cause; it stops the symptom being invisible. The installer needs SUDO_USER handling and a chown, and its suggestion to run as root should go. Reverting the endpoint change fails four of the five new tests; the fifth guards the success path and correctly does not move. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW * fix(web): stop the installer chmod stripping exec bits on every update git tracks five scripts as mode 644 that first_time_install.sh then chmods to 755 (start_display.sh, stop_display.sh, the two install_*_service.sh, and one-shot-install.sh does the same to first_time_install.sh). With core.fileMode true, the default on Linux, git reports all five as modified from then on, in files the user never touched. The update button stashes local changes before pulling, so it is not blocked by this. But it never pops that stash -- stash pop and stash apply appear nowhere in the update flow -- so the mode change is stashed away and left there, and the files revert: === file modes after the update button's stash === 664 first_time_install.sh <- installer had made these 755 664 start_display.sh 664 stop_display.sh 664 scripts/install/install_service.sh So every web-UI update silently strips the executable bit from the installer's own scripts, and leaves a stash entry holding the difference. start_display.sh and stop_display.sh stop working from the shell afterwards. A manual `git pull --rebase` over SSH fails outright, since nothing stashes for it: "cannot pull with rebase: You have unstaged changes". That is the likely source of the reports, since plenty of people update that way. Tracking the five as 755 -- what they should always have been, as the installer chmodding them attests -- removes the spurious mode change entirely: nothing to stash, nothing stripped, no stash entry, and manual pulls work. The pull also passes --autostash, for the case the code explicitly tolerates: when the stash fails it logs a warning and pulls anyway, and that pull is what then fails. Autostash also pops what it stashes, which the manual stash does not. Note that `git add -A` after `git update-index --chmod=+x` silently reverts the index to the on-disk mode, so the modes here were set by chmodding the files themselves. Regression test asserts the five stay tracked executable; reverting any one of them fails it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW * fix(web): ask for the restart that makes an update take effect The update button pulls new code and restarts nothing. There is no systemctl, restart, reload or reboot anywhere in the 172-line git_pull handler -- it stashes, pulls, installs changed requirements, re-removes plugins the user had uninstalled, and returns "Code updated successfully." Meanwhile both services go on running the code they loaded at boot. So the display keeps rendering the old build, the web interface keeps serving the old build, and the user is told the update worked. Nothing on screen suggests otherwise, and the next reboot is what actually applies it -- whenever that is. The affordance for this already exists: the restart-pending banner, raised after main-config saves, with a Restart Now button wired to the display service. A code update is a stronger reason to show it than a config save is. The response now reports restart_required, and applyUpdate raises the banner with wording for a code update rather than a config save. The banner's message became a parameter and is persisted next to the flag, since it outlives the page that raised it. restart_required is only true when the pull actually moved HEAD. "Already up to date" is a success too, and prompting after a no-op would train users to dismiss the prompt unread. This covers the display service, which is what the Restart Now button drives and what users notice. The web interface still picks up its own new code on its next restart; restarting it from inside a request it is serving is a larger change than this one. Reverting the flag fails the test that a pull which moved HEAD asks for a restart. 290 tests pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW * Mask list-shaped secrets element-wise, close two vacuous tests mask_all_secret_values treated any non-empty list as a scalar, so a secrets file holding "accounts": [{"name": "a", "token": "tok-a"}, {...}] came back as a single "••••••••". The caller could not see how many entries existed, and the raw editor was handed a string where the file holds an array. Recurse into lists in both _mask_value and _contains_mask. Lists merge by replacement, not key-wise, so strip_masked_values now drops a list outright if any element still carries the mask -- storing a half-masked list would discard the untouched entries. Two tests could pass without exercising what they claim to check: - test_git_pull_resolution asserted modes only for paths git ls-files returned. A renamed or deleted installer target is simply absent from that output, so its mode was never checked. Assert every CHMODDED path is tracked first. - test_config_secrets_masking never checked the POST status. A 500 leaves the old file in place, which satisfies every assertion that follows. Assert 200 before reading the file back. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-21 14:30:29 -04:00
ChuckBuilds	a7b99c5c53	Add systemd service and control scripts for LED Matrix Display	2025-04-19 12:40:21 -05:00