diff --git a/assets/sports/ncaa_logos/COR.png b/assets/sports/ncaa_logos/COR.png new file mode 100644 index 00000000..d39027fa Binary files /dev/null and b/assets/sports/ncaa_logos/COR.png differ diff --git a/first_time_install.sh b/first_time_install.sh old mode 100644 new mode 100755 diff --git a/scripts/install/install_service.sh b/scripts/install/install_service.sh old mode 100644 new mode 100755 diff --git a/scripts/install/install_web_service.sh b/scripts/install/install_web_service.sh old mode 100644 new mode 100755 diff --git a/src/web_interface/secret_helpers.py b/src/web_interface/secret_helpers.py index 310d9848..f4cae945 100644 --- a/src/web_interface/secret_helpers.py +++ b/src/web_interface/secret_helpers.py @@ -143,6 +143,12 @@ def mask_secret_fields(config: Dict[str, Any], schema_properties: Dict[str, Any] return result +#: What a masked secret looks like on the wire. Named because the write path +#: has to recognise it coming back: a client that renders the mask and posts +#: it unchanged must not store the mask as if it were the secret. +SECRET_MASK = '\u2022' * 8 + + def mask_all_secret_values(config: Dict[str, Any]) -> Dict[str, Any]: """Blanket-mask every non-empty value in a secrets config dict. @@ -161,7 +167,7 @@ def mask_all_secret_values(config: Dict[str, Any]) -> Dict[str, Any]: if isinstance(v, dict): masked[k] = mask_all_secret_values(v) elif v not in (None, '') and not (isinstance(v, str) and v.startswith('YOUR_')): - masked[k] = '••••••••' + masked[k] = SECRET_MASK else: masked[k] = v return masked @@ -189,3 +195,30 @@ def remove_empty_secrets(secrets: Dict[str, Any]) -> Dict[str, Any]: elif v is not None and not (isinstance(v, str) and v.strip() == ''): result[k] = v return result + + +def strip_masked_values(secrets: Dict[str, Any]) -> Dict[str, Any]: + """Remove values a client echoed back rather than changed. + + The counterpart to :func:`mask_all_secret_values`. A client that GETs the + masked secrets, edits one field and POSTs the whole object back is sending + ``SECRET_MASK`` for every field it did not touch. Storing those would + replace each untouched credential with eight bullet characters. + + Drops the mask and, like :func:`remove_empty_secrets`, blank values -- so + the caller can merge the result onto what is already stored and have + "unchanged" mean unchanged. Empty nested dicts are pruned. + """ + result: Dict[str, Any] = {} + for k, v in secrets.items(): + if isinstance(v, dict): + nested = strip_masked_values(v) + if nested: + result[k] = nested + elif v is None: + continue + elif isinstance(v, str) and (v.strip() == '' or v == SECRET_MASK): + continue + else: + result[k] = v + return result diff --git a/start_display.sh b/start_display.sh old mode 100644 new mode 100755 diff --git a/stop_display.sh b/stop_display.sh old mode 100644 new mode 100755 diff --git a/test/test_config_main_redacts_secrets.py b/test/test_config_main_redacts_secrets.py new file mode 100644 index 00000000..15172df2 --- /dev/null +++ b/test/test_config_main_redacts_secrets.py @@ -0,0 +1,143 @@ +"""GET /config/main must not hand out credentials. + +The endpoint returned the raw config to anyone who could reach the port, and +this web interface has no authentication of any kind. Measured against a live +rig, an unauthenticated request 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. + +The x-secret masking the plugin config endpoints use does not apply here: this +endpoint never consults a schema, and core keys such as github.api_token have +no schema to carry the marker. Several of those fields *are* tagged x-secret in +their plugin's schema and were still returned in full, which is what makes the +schema route the wrong one to rely on for this endpoint. + +Matching on field name is blunt. For a whole-config dump it is the right +default: anything named like a credential should not leave the process, and a +new plugin that adds a differently-shaped secret is covered without anyone +remembering to tag it. +""" +import pytest + +from web_interface.blueprints.api_v3 import ( + _looks_like_a_credential, + _redact_credentials, +) + + +@pytest.mark.parametrize("name", [ + "password", "mqtt_password", "opensky_password", "passwd", + "api_key", "apikey", "API_KEY", "flightaware_api_key", + "token", "ha_token", "api_token", "access_token", + "secret", "client_secret", "spotify_client_secret", + "access_key", "private_key", +]) +def test_credential_names_are_recognised(name): + assert _looks_like_a_credential(name) + + +@pytest.mark.parametrize("name", [ + "timezone", "city", "brightness", "enabled", "update_interval", + "favorite_teams", "display_duration", "keyword", +]) +def test_ordinary_names_are_left_alone(name): + assert not _looks_like_a_credential(name) + + +def test_the_measured_leak_is_closed(): + """The exact shape taken off the rig.""" + config = { + "github": {"api_token": "ghp_" + "x" * 36}, + "incoming-packages": {"ha_token": "y" * 183, "enabled": True}, + "jellyfin-now-playing": {"api_key": "z" * 32}, + "on-air": {"mqtt_password": "hunter22"}, + "youtube": {"api_key": "k" * 20}, + "timezone": "America/New_York", + } + out = _redact_credentials(config) + assert out["github"]["api_token"] == "" + assert out["incoming-packages"]["ha_token"] == "" + assert out["jellyfin-now-playing"]["api_key"] == "" + assert out["on-air"]["mqtt_password"] == "" + assert out["youtube"]["api_key"] == "" + # Everything else survives, or the config editor breaks. + assert out["timezone"] == "America/New_York" + assert out["incoming-packages"]["enabled"] is True + + +def test_nested_and_listed_credentials_are_reached(): + config = {"a": {"b": {"c": {"password": "p"}}}, + "feeds": [{"name": "x", "api_key": "k"}, {"name": "y"}]} + out = _redact_credentials(config) + assert out["a"]["b"]["c"]["password"] == "" + assert out["feeds"][0]["api_key"] == "" + assert out["feeds"][0]["name"] == "x" + + +def test_the_original_is_not_mutated(): + """The caller holds the live config; redaction must not edit it in place.""" + config = {"github": {"api_token": "keepme"}} + _redact_credentials(config) + assert config["github"]["api_token"] == "keepme" + + +def test_a_credential_shaped_container_is_still_walked(): + """`secrets: {...}` is a section name, not a value to blank.""" + config = {"secrets": {"api_key": "k", "note": "keep"}} + out = _redact_credentials(config) + assert out["secrets"]["api_key"] == "" + assert out["secrets"]["note"] == "keep" + + +def test_non_dict_input_passes_through(): + assert _redact_credentials("plain") == "plain" + assert _redact_credentials(7) == 7 + assert _redact_credentials(None) is None + + +def test_the_endpoint_itself_redacts(): + """Through the view function, not the helper. + + The helper tests above all passed with the route still returning + `config` -- reverting the one line that calls the redactor changed + nothing, because nothing exercised the route. 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. + """ + import json as _json + from unittest.mock import MagicMock + + import flask + + from web_interface.blueprints import api_v3 as mod + + raw = {"github": {"api_token": "ghp_secret_value"}, + "timezone": "America/New_York"} + + manager = MagicMock() + manager.load_config.return_value = raw + previous = getattr(mod.api_v3, "config_manager", None) + mod.api_v3.config_manager = manager + + app = flask.Flask(__name__) + try: + with app.test_request_context("/config/main"): + response = mod.get_main_config() + payload = response.get_json() if hasattr(response, "get_json") else _json.loads(response[0].data) + finally: + mod.api_v3.config_manager = previous + + data = payload["data"] + assert data["github"]["api_token"] == "", ( + "the endpoint returned the token; the redactor is not wired in") + assert data["timezone"] == "America/New_York" + # And the config the manager handed over is untouched. + assert raw["github"]["api_token"] == "ghp_secret_value" diff --git a/test/test_git_pull_resolution.py b/test/test_git_pull_resolution.py index d5668142..4d235df5 100644 --- a/test/test_git_pull_resolution.py +++ b/test/test_git_pull_resolution.py @@ -58,7 +58,7 @@ def repos(tmp_path): def test_branch_with_upstream_uses_a_plain_pull(repos): args, note, error = resolve_pull_command(str(repos)) assert error is None - assert args == ['git', 'pull', '--rebase'] + assert args == ['git', 'pull', '--rebase', '--autostash'] assert note == '' @@ -73,7 +73,7 @@ def test_branch_without_upstream_falls_back_to_origin_branch(repos): args, note, error = resolve_pull_command(str(repos)) assert error is None - assert args == ['git', 'pull', '--rebase', 'origin', 'audit'] + assert args == ['git', 'pull', '--rebase', '--autostash', 'origin', 'audit'] assert 'audit' in note @@ -155,7 +155,7 @@ def test_switching_attaches_tracking_so_pull_needs_no_fallback(repos): args, note, error = resolve_pull_command(str(repos)) assert error is None - assert args == ['git', 'pull', '--rebase'] + assert args == ['git', 'pull', '--rebase', '--autostash'] assert note == '' @@ -200,3 +200,37 @@ def test_stash_option_lets_the_switch_through_and_keeps_the_work(repos): assert _git('branch', '--show-current', cwd=repos).stdout.strip() == 'other' # The edit is not lost — it is on the stash. assert 'switch to other' in _git('stash', 'list', cwd=repos).stdout + + +class TestInstallerDoesNotBlockTheUpdateButton: + """first_time_install.sh chmods scripts that git tracked as 644. + + With core.fileMode true -- the default on Linux -- that leaves five + permanently modified tracked files on every machine that ran the + installer, and `git pull --rebase` refuses to start: + + error: cannot pull with rebase: You have unstaged changes. + + Tracking them as executable makes the installer's chmod a no-op. + """ + + CHMODDED = [ + 'first_time_install.sh', + 'start_display.sh', + 'stop_display.sh', + 'scripts/install/install_service.sh', + 'scripts/install/install_web_service.sh', + ] + + def test_scripts_the_installer_chmods_are_tracked_executable(self): + import subprocess + from pathlib import Path + root = Path(__file__).resolve().parent.parent + out = subprocess.run(['git', 'ls-files', '-s', *self.CHMODDED], + capture_output=True, text=True, cwd=str(root)).stdout + modes = {line.split()[3]: line.split()[0] for line in out.strip().split('\n') if line} + non_exec = sorted(f for f, m in modes.items() if m != '100755') + assert not non_exec, ( + f"{non_exec} are chmodded by the installer but tracked non-executable, " + "so every install leaves the working tree dirty and the update " + "button cannot pull") diff --git a/test/test_update_check_reports_failure.py b/test/test_update_check_reports_failure.py new file mode 100644 index 00000000..f638bb9d --- /dev/null +++ b/test/test_update_check_reports_failure.py @@ -0,0 +1,85 @@ +"""A check that could not run must not be reported as "up to date". + +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, and with nothing for the +user to act on. The usual cause is an install performed as root, after which +every git command fails with "detected dubious ownership". +""" +import subprocess +import sys +from pathlib import Path +from unittest.mock import patch + +import pytest +from flask import Flask + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +from web_interface.blueprints import api_v3 as mod # noqa: E402 +from web_interface.blueprints.api_v3 import api_v3 # noqa: E402 + +DUBIOUS = ("fatal: detected dubious ownership in repository at " + "'/home/pi/LEDMatrix'\nTo add an exception for this directory, call:\n" + "\tgit config --global --add safe.directory /home/pi/LEDMatrix\n") + + +@pytest.fixture +def client(): + app = Flask(__name__) + app.config['TESTING'] = True + app.register_blueprint(api_v3, url_prefix='/api/v3') + mod._update_check_cache['result'] = None + mod._update_check_cache['ts'] = 0 + return app.test_client() + + +def _fetch_fails(stderr: bytes): + def fake_run(args, **kwargs): + if args[:2] == ['git', 'fetch']: + return subprocess.CompletedProcess(args, 1, stdout=b'', stderr=stderr) + return subprocess.CompletedProcess(args, 0, stdout='', stderr='') + return fake_run + + +class TestFailedCheckIsNotSilence: + def test_dubious_ownership_is_reported_not_swallowed(self, client): + with patch.object(mod.subprocess, 'run', _fetch_fails(DUBIOUS.encode())): + data = client.get('/api/v3/system/check-update').get_json() + assert data['check_failed'] is True, ( + "a git failure was reported as a successful 'no update' check") + assert data['update_available'] is False + + def test_the_message_tells_the_user_what_to_do(self, client): + with patch.object(mod.subprocess, 'run', _fetch_fails(DUBIOUS.encode())): + data = client.get('/api/v3/system/check-update').get_json() + assert 'chown' in data['error'], ( + "dubious ownership is unactionable without the fix command") + assert 'root' in data['error'] + + def test_an_ordinary_git_failure_still_surfaces(self, client): + with patch.object(mod.subprocess, 'run', + _fetch_fails(b'fatal: some other git problem\n')): + data = client.get('/api/v3/system/check-update').get_json() + assert data['check_failed'] is True + assert 'some other git problem' in data['error'] + + def test_offline_reads_as_offline(self, client): + with patch.object(mod.subprocess, 'run', + _fetch_fails(b'fatal: could not resolve host: github.com\n')): + data = client.get('/api/v3/system/check-update').get_json() + assert 'Could not reach GitHub' in data['error'] + + +class TestSuccessPathUnchanged: + def test_up_to_date_carries_no_failure_flag(self, client): + def fake_run(args, **kwargs): + if args[:2] == ['git', 'fetch']: + return subprocess.CompletedProcess(args, 0, stdout=b'', stderr=b'') + if args[:2] == ['git', 'rev-parse']: + return subprocess.CompletedProcess(args, 0, stdout='abc123\n', stderr='') + return subprocess.CompletedProcess(args, 0, stdout='0\n', stderr='') + with patch.object(mod.subprocess, 'run', fake_run): + data = client.get('/api/v3/system/check-update').get_json() + assert data['update_available'] is False + assert not data.get('check_failed'), "a healthy check must not look like a failure" diff --git a/test/test_update_prompts_restart.py b/test/test_update_prompts_restart.py new file mode 100644 index 00000000..d7871e85 --- /dev/null +++ b/test/test_update_prompts_restart.py @@ -0,0 +1,84 @@ +"""A pull that changed nothing on the running system is not an applied update. + +git_pull replaces files on disk and restarts nothing -- there is no systemctl +call anywhere in the handler. The display and web services keep running the +code they loaded at boot, so the user is told "Code updated successfully" and +sees no change until they happen to reboot. The response now says whether a +restart is owed, and the UI raises the existing restart-pending banner. +""" +import subprocess +import sys +from pathlib import Path +from unittest.mock import patch + +import pytest +from flask import Flask + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +from web_interface.blueprints import api_v3 as mod # noqa: E402 +from web_interface.blueprints.api_v3 import api_v3 # noqa: E402 + + +@pytest.fixture +def client(): + app = Flask(__name__) + app.config['TESTING'] = True + app.register_blueprint(api_v3, url_prefix='/api/v3') + # The handler consults these after a successful pull; None is the + # "not wired up" case it already guards for. + api_v3.plugin_store_manager = None + api_v3.config_manager = None + return app.test_client() + + +def _git(heads, pull_rc=0, pull_out='Updating a1b2c3..d4e5f6\n'): + """Fake git. `heads` are the successive answers to rev-parse HEAD.""" + seq = list(heads) + + def run(args, **kwargs): + def ok(stdout='', rc=0, b=False): + return subprocess.CompletedProcess( + args, rc, stdout=(stdout.encode() if b else stdout), + stderr=(b'' if b else '')) + if args[:2] == ['git', 'rev-parse'] and args[-1] == 'HEAD': + return ok(seq.pop(0) + '\n' if seq else 'deadbeef\n') + if 'symbolic-full-name' in args or '@{u}' in args: + return ok('origin/main\n') + if args[:2] == ['git', 'status']: + return ok('') + if args[:2] == ['git', 'diff']: + return ok('') + if args[:2] == ['git', 'pull']: + return ok(pull_out, pull_rc) + return ok('') + return run + + +def _pull(client): + return client.post('/api/v3/system/action', + json={'action': 'git_pull'}).get_json() + + +class TestRestartIsRequestedWhenCodeChanged: + def test_a_pull_that_moved_head_asks_for_a_restart(self, client): + with patch.object(mod.subprocess, 'run', _git(['aaa111', 'bbb222'])): + data = _pull(client) + assert data['status'] == 'success' + assert data['restart_required'] is True, ( + "new code on disk, services still running the old code, and " + "nothing told the user to restart") + + def test_already_up_to_date_does_not(self, client): + with patch.object(mod.subprocess, 'run', + _git(['aaa111', 'aaa111'], pull_out='Already up to date.\n')): + data = _pull(client) + assert data['status'] == 'success' + assert data['restart_required'] is False, ( + "prompting after a no-op update trains users to ignore the prompt") + + def test_a_failed_pull_does_not(self, client): + with patch.object(mod.subprocess, 'run', _git(['aaa111'], pull_rc=1)): + data = _pull(client) + assert data['status'] == 'error' + assert data['restart_required'] is False diff --git a/test/web_interface/test_api_v3_secret_roundtrip.py b/test/web_interface/test_api_v3_secret_roundtrip.py index a4648824..1d70f009 100644 --- a/test/web_interface/test_api_v3_secret_roundtrip.py +++ b/test/web_interface/test_api_v3_secret_roundtrip.py @@ -194,15 +194,46 @@ def test_round_trip_with_array_secrets(self, env): def test_secret_count_message_counts_top_level_keys(self, env): # Pinned: the "(N secret field(s))" message counts TOP-LEVEL keys of - # the separated secrets dict. Here that is 2: the posted accounts - # array (all its item tokens count as ONE key) plus the schema's - # api_key default ("") that merge_with_defaults adds before - # separation. + # the separated secrets dict. Here that is 1: the posted accounts + # array, whose item tokens all count as ONE key. + # + # It was 2 before blank secrets were dropped, the second being the + # schema's api_key default (""), which merge_with_defaults adds to + # every save. Counting it was the visible edge of a real bug: that + # injected blank was merged over the stored api_key, so saving any + # unrelated field destroyed the credential. See + # test_an_unrelated_edit_does_not_erase_a_stored_secret. resp = self._save(env, { "accounts": [{"name": "a", "token": "t"}], }) message = resp.get_json()["message"] - assert "(2 secret field(s) saved to config_secrets.json)" in message + assert "(1 secret field(s) saved to config_secrets.json)" in message + + def test_an_unrelated_edit_does_not_erase_a_stored_secret(self, env): + """Editing one field must not wipe the plugin's API key. + + The config form renders secrets masked, so the browser posts them + back blank; merge_with_defaults injects a blank api_key even when + the client omits it entirely. Either way a "" reached the secrets + file and deep_merge wrote it over the stored credential. + """ + assert self._save(env, {"api_key": "REAL-KEY-0123456789", + "city": "Austin"}).status_code == 200 + assert _on_disk(env.secrets_file)[PLUGIN_ID]["api_key"] == \ + "REAL-KEY-0123456789" + + # the user changes the city; the masked api_key rides along blank + assert self._save(env, {"api_key": "", "city": "Dallas"}).status_code == 200 + + assert _on_disk(env.secrets_file)[PLUGIN_ID]["api_key"] == \ + "REAL-KEY-0123456789", "an unrelated edit destroyed the API key" + assert env.fresh_load()[PLUGIN_ID]["city"] == "Dallas" + + def test_a_secret_can_still_be_changed(self, env): + """Dropping blanks must not stop a real new value from being saved.""" + self._save(env, {"api_key": "first-key"}) + self._save(env, {"api_key": "second-key"}) + assert _on_disk(env.secrets_file)[PLUGIN_ID]["api_key"] == "second-key" def test_resave_replaces_stored_secrets_list_wholesale(self, env): # Characterized: api_v3's deep_merge intentionally replaces lists, diff --git a/test/web_interface/test_config_secrets_masking.py b/test/web_interface/test_config_secrets_masking.py new file mode 100644 index 00000000..dcae66c8 --- /dev/null +++ b/test/web_interface/test_config_secrets_masking.py @@ -0,0 +1,82 @@ +"""GET /config/secrets must not hand out credentials, and the client's +read-modify-write cycle must not destroy them. + +This interface has no authentication. The endpoint returned the whole +config_secrets.json to anyone who could reach the port; on one rig that was a +40-character GitHub token, a 183-character Home Assistant token and three API +keys. Masking it alone is not enough: the only client fetches every secret, +edits one field and posts all of them back, so the write path has to treat an +echoed mask as "unchanged". +""" +import json +import sys +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).parent)) + +from test_api_v3_secret_roundtrip import env, _on_disk # noqa: F401,E402 +from src.web_interface.secret_helpers import SECRET_MASK # noqa: E402 + +STORED = { + "github": {"api_token": "ghp_" + "x" * 36}, + "ledmatrix-weather": {"api_key": "w" * 32}, + "incoming-packages": {"ha_token": "h" * 183}, + "unset-plugin": {"api_key": ""}, + "placeholder-plugin": {"api_key": "YOUR_API_KEY_HERE"}, +} + + +def _seed(env): + env.secrets_file.write_text(json.dumps(STORED)) + + +def _get(env): + r = env.client.get("/api/v3/config/secrets") + assert r.status_code == 200, r.get_data(as_text=True)[:200] + return r.get_json()["data"] + + +def test_no_credential_leaves_the_process(env): + _seed(env) + body = json.dumps(_get(env)) + for secret in ("ghp_" + "x" * 36, "w" * 32, "h" * 183): + assert secret not in body, "endpoint returned a stored credential" + + +def test_set_and_unset_remain_distinguishable(env): + _seed(env) + data = _get(env) + assert data["github"]["api_token"] == SECRET_MASK + assert data["unset-plugin"]["api_key"] == "" + assert data["placeholder-plugin"]["api_key"] == "YOUR_API_KEY_HERE" + + +def test_the_clients_read_modify_write_preserves_every_other_secret(env): + """What the GitHub-token save button actually does.""" + _seed(env) + secrets = _get(env) # everything arrives masked + secrets["github"]["api_token"] = "ghp_" + "n" * 36 # user changes one + r = env.client.post("/api/v3/config/raw/secrets", json=secrets) + assert r.status_code == 200, r.get_data(as_text=True)[:200] + + on_disk = _on_disk(env.secrets_file) + assert on_disk["github"]["api_token"] == "ghp_" + "n" * 36, "new token not saved" + assert on_disk["ledmatrix-weather"]["api_key"] == "w" * 32 + assert on_disk["incoming-packages"]["ha_token"] == "h" * 183 + + +def test_a_mask_echoed_back_is_never_stored(env): + _seed(env) + env.client.post("/api/v3/config/raw/secrets", json=_get(env)) + on_disk = _on_disk(env.secrets_file) + assert SECRET_MASK not in json.dumps(on_disk), "the mask was stored as a secret" + assert on_disk["github"]["api_token"] == "ghp_" + "x" * 36 + + +def test_a_brand_new_secret_can_still_be_added(env): + _seed(env) + env.client.post("/api/v3/config/raw/secrets", + json={"new-plugin": {"api_key": "brand-new"}}) + on_disk = _on_disk(env.secrets_file) + assert on_disk["new-plugin"]["api_key"] == "brand-new" + assert on_disk["github"]["api_token"] == "ghp_" + "x" * 36 diff --git a/web_interface/blueprints/api_v3.py b/web_interface/blueprints/api_v3.py index 984637e8..b5d994fe 100644 --- a/web_interface/blueprints/api_v3.py +++ b/web_interface/blueprints/api_v3.py @@ -21,7 +21,9 @@ # Import new infrastructure from src.web_interface.api_helpers import success_response, error_response, validate_request_json from src.web_interface.errors import ErrorCode -from src.web_interface.secret_helpers import find_secret_fields, separate_secrets +from src.web_interface.secret_helpers import (find_secret_fields, mask_all_secret_values, + remove_empty_secrets, separate_secrets, + strip_masked_values) from src.web_interface.error_handler import describe_exception, redact_text from src.plugin_system.operation_types import OperationType from src.web_interface.validators import ( @@ -262,15 +264,54 @@ def _stop_display_service(): result['status'] = status return result +#: Field names whose value is a credential. Matched by name because this +#: endpoint returns the whole config, core keys included, and core config has +#: no schema to carry x-secret markers. +_CREDENTIAL_NAME_PARTS = ("password", "passwd", "secret", "token", "api_key", + "apikey", "access_key", "private_key", "client_secret") + + +def _looks_like_a_credential(name: str) -> bool: + lowered = name.lower() + return any(part in lowered for part in _CREDENTIAL_NAME_PARTS) + + +def _redact_credentials(value): + """A copy of `value` with credential-named fields blanked. + + /config/main returned the raw config to anyone who could reach the port, + and this interface has no authentication. On one rig that meant a 40-char + GitHub token, a 183-char Home Assistant token and five API keys were + readable by anything on the LAN. + + The x-secret masking used by the plugin config endpoints does not help + here: this endpoint never consults a schema, and core keys such as + github.api_token have no schema to mark. Matching on the field name is + blunt, but for a whole-config dump the right default is that anything + named like a credential does not leave the process. + + Blanked rather than removed, and safe to blank: POST /config/main merges + into the loaded config and only writes the keys it was given, so a client + that round-trips this response cannot erase a secret it never saw. + """ + if isinstance(value, dict): + return {k: ("" if _looks_like_a_credential(k) and not isinstance(v, (dict, list)) + else _redact_credentials(v)) + for k, v in value.items()} + if isinstance(value, list): + return [_redact_credentials(item) for item in value] + return value + + @api_v3.route('/config/main', methods=['GET']) def get_main_config(): - """Get main configuration""" + """Get main configuration, with credentials redacted.""" try: if not api_v3.config_manager: return jsonify({'status': 'error', 'message': 'Config manager not initialized'}), 500 config = api_v3.config_manager.load_config() - return jsonify({'status': 'success', 'data': config}) + return jsonify({'status': 'success', 'data': _redact_credentials(config)}) except Exception as e: logger.error('Unhandled exception', exc_info=True) return jsonify({'status': 'error', 'message': 'An error occurred; see logs for details', 'details': describe_exception(e)}), 500 @@ -715,10 +756,12 @@ def save_main_config(): if not data: return jsonify({'status': 'error', 'message': 'No data provided'}), 400 - import logging - logging.error(f"DEBUG: save_main_config received data: {data}") - logging.error(f"DEBUG: Content-Type header: {request.content_type}") - logging.error(f"DEBUG: Headers: {dict(request.headers)}") + # What arrives here is the config itself, and the headers carry the + # session cookie -- neither belongs in the journal, least of all at + # ERROR on every save. The shape of the request is the part with + # diagnostic value, so log that, at the level it deserves. + logger.debug("save_main_config: %s, %d top-level key(s)", + request.content_type or 'no content-type', len(data)) # Merge with existing config (similar to original implementation) current_config = api_v3.config_manager.load_config() @@ -1216,6 +1259,11 @@ def _copies_fits_hardware(copies: int) -> Optional[str]: # Separate secrets from regular config (same logic as save_plugin_config) regular_config, secrets_config = separate_secrets(plugin_config, secret_fields) + # The config form renders secrets masked, so every save posts + # them back blank. Without this the blank is merged over the + # stored value and the credential is destroyed by the act of + # changing an unrelated setting. A blank means "unchanged". + secrets_config = remove_empty_secrets(secrets_config) # PRE-PROCESSING: Preserve 'enabled' state if not in regular_config # This prevents overwriting the enabled state when saving config from a form that doesn't include the toggle @@ -1333,7 +1381,12 @@ def get_secrets_config(): return jsonify({'status': 'error', 'message': 'Config manager not initialized'}), 500 config = api_v3.config_manager.get_raw_file_content('secrets') - return jsonify({'status': 'success', 'data': config}) + # This interface has no authentication, and this file is nothing but + # credentials. It was handing all of them to anyone who could reach + # the port. Values are masked; empty and YOUR_* placeholders are left + # alone so a client can still tell "set" from "not set". + return jsonify({'status': 'success', + 'data': mask_all_secret_values(config)}) except Exception as e: logger.error('Unhandled exception', exc_info=True) return jsonify({'status': 'error', 'message': 'An error occurred; see logs for details', 'details': describe_exception(e)}), 500 @@ -1395,8 +1448,19 @@ def save_raw_secrets_config(): if not data: return jsonify({'status': 'error', 'message': 'No data provided'}), 400 - # Save the secrets config - api_v3.config_manager.save_raw_file_content('secrets', data) + # The GET above masks what it returns, and this endpoint's only client + # reads the whole file, edits one field and posts all of it back. So + # most of what arrives here is the mask, echoed rather than changed -- + # storing it verbatim would replace every untouched credential with + # eight bullets. Strip those, then merge onto what is already stored, + # which makes "unchanged" mean unchanged. + # + # The cost is that a secret can no longer be cleared by blanking it. + # That needs its own affordance; a control that erases credentials as + # a side effect of saving an unrelated one is not it. + current = api_v3.config_manager.get_raw_file_content('secrets') or {} + merged = deep_merge(current, strip_masked_values(data)) + api_v3.config_manager.save_raw_file_content('secrets', merged) # Reload GitHub token in plugin store manager if it exists if api_v3.plugin_store_manager: @@ -1657,13 +1721,22 @@ def resolve_pull_command(project_dir): backup, or following an install guide that names one. The update button then reports a failure the user cannot act on. + ``--autostash`` is passed for the same reason. Rebase refuses to start + when any tracked file is modified, and on these installs something always + is: first_time_install.sh chmods five scripts that git tracked as 644, so + every machine that ran the installer carries five permanent mode changes + and the update button reports "cannot pull with rebase: You have unstaged + changes". Those modes are corrected in this commit, but a user cannot pull + the correction while the pull is what is blocked, and any other local edit + would reproduce it anyway. Autostash reapplies the changes afterwards. + Returns ``(args, note, error)``. When ``origin/`` exists the pull is made explicit against it, so the update proceeds and the branch is given tracking information afterwards. """ upstream = _git_upstream(project_dir) if upstream: - return ['git', 'pull', '--rebase'], '', None + return ['git', 'pull', '--rebase', '--autostash'], '', None branch = _git_current_branch(project_dir) if not branch: @@ -1673,7 +1746,7 @@ def resolve_pull_command(project_dir): ) if _git_remote_branch_exists(project_dir, branch): return ( - ['git', 'pull', '--rebase', 'origin', branch], + ['git', 'pull', '--rebase', '--autostash', 'origin', branch], f"Branch '{branch}' had no upstream; pulled from origin/{branch} and set it as the upstream.", None, ) @@ -1821,6 +1894,33 @@ def get_system_version(): _update_check_cache: Dict[str, Any] = {'result': None, 'ts': 0.0} _UPDATE_CHECK_TTL = 300 # 5 minutes — avoids a git fetch on every page load +def _update_check_failed(detail: str) -> Dict[str, Any]: + """A check that could not run is not the same as being up to date. + + Reporting update_available=False on a git failure hides the banner, and + the banner is the only route to the update button -- so a checkout git + refuses to touch looks exactly like a current one, permanently. The most + common cause is an install performed as root: git then reports "dubious + ownership" and every command fails, including the fetch here. + """ + return {'update_available': False, 'remote_sha': 'unknown', + 'commits_behind': 0, 'check_failed': True, 'error': detail} + + +def _describe_git_failure(stderr: str) -> str: + """Turn git's stderr into something the user can act on.""" + text = (stderr or '').strip() + if 'dubious ownership' in text or 'detected dubious ownership' in text: + return ("This checkout is owned by a different user than the one " + "running the web interface, so git refuses to use it. It is " + "usually the result of installing as root. Fix the ownership " + "and the update will work: sudo chown -R $USER:$USER " + + str(PROJECT_ROOT)) + if 'could not resolve host' in text.lower() or 'network is unreachable' in text.lower(): + return "Could not reach GitHub to check for updates." + return "Could not check for updates: " + (text.splitlines()[0] if text else "git failed") + + @api_v3.route('/system/check-update', methods=['GET']) def check_for_update(): """Check whether a newer LEDMatrix commit is available on origin/main.""" @@ -1836,12 +1936,13 @@ def check_for_update(): capture_output=True, timeout=10, cwd=cwd, ) if fetch_result.returncode != 0: + stderr = fetch_result.stderr.decode(errors='replace').strip() logger.warning("check-update: git fetch failed (rc=%d): %s", - fetch_result.returncode, - fetch_result.stderr.decode(errors='replace').strip()) - _update_check_cache['result'] = _safe + fetch_result.returncode, stderr) + failed = _update_check_failed(_describe_git_failure(stderr)) + _update_check_cache['result'] = failed _update_check_cache['ts'] = now - return jsonify(_safe) + return jsonify(failed) local = subprocess.run( ['git', 'rev-parse', 'HEAD'], capture_output=True, text=True, timeout=5, cwd=cwd, @@ -1869,7 +1970,8 @@ def check_for_update(): return jsonify(result) except Exception as e: logger.warning("check-update failed: %s", e) - return jsonify(_safe) + return jsonify(_update_check_failed( + "Could not check for updates; see logs for details.")) @api_v3.route('/system/action', methods=['POST']) def execute_system_action(): @@ -1996,6 +2098,11 @@ def execute_system_action(): except subprocess.TimeoutExpired: logger.warning("git rev-parse timed out before pull") + # Whether the pull actually brought new code in. "Already up to + # date" is a success too, and prompting for a restart then would + # train users to ignore the prompt. + code_changed = False + # Perform the git pull. Branches without an upstream were given # an explicit "origin " above so the update still works. result = subprocess.run( @@ -2039,6 +2146,7 @@ def execute_system_action(): capture_output=True, text=True, timeout=10, cwd=project_dir) new_head = _post.stdout.strip() if _post.returncode == 0 else None if old_head and new_head and old_head != new_head: + code_changed = True diff = subprocess.run( ['git', 'diff', '--name-only', f'{old_head}..{new_head}'], capture_output=True, text=True, timeout=15, cwd=project_dir) @@ -2098,9 +2206,14 @@ def execute_system_action(): if ln.strip()), '') pull_message = f"Update failed: {detail}" if detail else "Update failed; check logs for details" + # Nothing here restarts anything: the pull replaces files on + # disk while the display and web services keep running the code + # they loaded at boot. Without this the user is told the update + # succeeded and sees no change until they happen to reboot. return jsonify({ 'status': 'success' if result.returncode == 0 else 'error', 'message': pull_message, + 'restart_required': bool(result.returncode == 0 and code_changed), }) elif action == 'checkout_branch': # Switch branches from the Tools tab. Needed because a checkout @@ -5599,6 +5712,11 @@ def normalize_config_values(config, schema_props, prefix=''): # Separate secrets from regular config (handles nested configs and # array-item secrets — see src/web_interface/secret_helpers.py) regular_config, secrets_config = separate_secrets(plugin_config, secret_fields) + # The config form renders secrets masked, so every save posts + # them back blank. Without this the blank is merged over the + # stored value and the credential is destroyed by the act of + # changing an unrelated setting. A blank means "unchanged". + secrets_config = remove_empty_secrets(secrets_config) # Get current configs current_config = api_v3.config_manager.load_config() diff --git a/web_interface/static/v3/app.js b/web_interface/static/v3/app.js index 82b2a9e5..581c66f2 100644 --- a/web_interface/static/v3/app.js +++ b/web_interface/static/v3/app.js @@ -116,14 +116,25 @@ document.body.addEventListener('htmx:afterRequest', function(event) { // ===== Restart-pending banner ===== // Shown after restart-requiring saves; persists across tab switches (and // reloads, via sessionStorage) until the display restarts or it's dismissed. -window.showRestartPending = function() { - try { sessionStorage.setItem('ledmatrix-restart-pending', '1'); } catch { /* private browsing */ } +window.showRestartPending = function(message) { + try { + sessionStorage.setItem('ledmatrix-restart-pending', '1'); + // Persisted alongside the flag: a code update and a config save want + // different wording, and the banner outlives the page that raised it. + if (message) sessionStorage.setItem('ledmatrix-restart-pending-text', message); + else sessionStorage.removeItem('ledmatrix-restart-pending-text'); + } catch { /* private browsing */ } const banner = document.getElementById('restart-pending-banner'); + const text = document.getElementById('restart-pending-text'); + if (text && message) text.textContent = message; if (banner) banner.style.display = 'block'; }; window.dismissRestartPending = function() { - try { sessionStorage.removeItem('ledmatrix-restart-pending'); } catch { /* no-op */ } + try { + sessionStorage.removeItem('ledmatrix-restart-pending'); + sessionStorage.removeItem('ledmatrix-restart-pending-text'); + } catch { /* no-op */ } const banner = document.getElementById('restart-pending-banner'); if (banner) banner.style.display = 'none'; }; @@ -151,6 +162,9 @@ document.addEventListener('DOMContentLoaded', function() { try { if (sessionStorage.getItem('ledmatrix-restart-pending') === '1') { const banner = document.getElementById('restart-pending-banner'); + const saved = sessionStorage.getItem('ledmatrix-restart-pending-text'); + const text = document.getElementById('restart-pending-text'); + if (text && saved) text.textContent = saved; if (banner) banner.style.display = 'block'; } } catch { /* no-op */ } diff --git a/web_interface/static/v3/plugins_manager.js b/web_interface/static/v3/plugins_manager.js index 8bca1821..ea020998 100644 --- a/web_interface/static/v3/plugins_manager.js +++ b/web_interface/static/v3/plugins_manager.js @@ -4622,15 +4622,17 @@ window.loadGithubToken = function() { // Handle empty data (secrets file doesn't exist) - API returns {} in this case const secrets = data.data || {}; const token = secrets.github?.api_token || ''; + const configured = token && token !== 'YOUR_GITHUB_PERSONAL_ACCESS_TOKEN'; if (input) { - if (token && token !== 'YOUR_GITHUB_PERSONAL_ACCESS_TOKEN') { - // Token exists and is valid - input.value = token; - showNotification('GitHub token loaded successfully', 'success'); + // The endpoint masks what it returns, so this never holds + // the real token -- and the field is deliberately left + // empty rather than filled with the mask, which would be + // saved verbatim the next time the user pressed Save. + input.value = ''; + if (configured) { + showNotification('A GitHub token is saved. Enter a new one to replace it.', 'success'); } else { - // No token configured or placeholder value - input.value = ''; showNotification('No GitHub token configured. Enter a new token to save.', 'info'); } } diff --git a/web_interface/templates/v3/base.html b/web_interface/templates/v3/base.html index fb4cfa6a..e739dd8c 100644 --- a/web_interface/templates/v3/base.html +++ b/web_interface/templates/v3/base.html @@ -413,7 +413,8 @@

- + Configuration saved — restart the display to apply the changes
@@ -1107,15 +1108,29 @@

Run Plugin On-Deman fetch('/api/v3/system/check-update') .then(function(r) { return r.json(); }) .then(function(data) { + var banner = document.getElementById('update-banner'); + var btn = document.getElementById('update-banner-btn'); + if (data.check_failed) { + // A check that could not run is not the same as being up + // to date. Hiding the banner here made a checkout git + // refuses to touch look permanently current, with no + // route to the update button and nothing to act on. + document.getElementById('update-banner-text').textContent = + data.error || 'Could not check for updates.'; + if (btn) btn.style.display = 'none'; + banner.style.display = ''; + return; + } + if (btn) btn.style.display = ''; if (data.update_available && getDismissedSha() !== data.remote_sha) { var n = data.commits_behind || 0; var msg = 'A new LEDMatrix update is available'; if (n > 0) msg += ' (' + n + ' commit' + (n > 1 ? 's' : '') + ')'; document.getElementById('update-banner-text').textContent = msg; - document.getElementById('update-banner').style.display = ''; + banner.style.display = ''; try { sessionStorage.setItem('update-sha', data.remote_sha); } catch(e) {} } else { - document.getElementById('update-banner').style.display = 'none'; + banner.style.display = 'none'; } }) .catch(function() {}); @@ -1146,6 +1161,13 @@

Run Plugin On-Deman if (data.status === 'success') { document.getElementById('update-banner').style.display = 'none'; try { sessionStorage.removeItem('update-sha-dismissed'); } catch(e) {} + // The pull replaced files on disk; the running services still + // hold the code they loaded at boot. Ask for the restart that + // makes the update actually take effect. + if (data.restart_required && typeof window.showRestartPending === 'function') { + window.showRestartPending( + 'Update installed \u2014 restart the display to run the new code'); + } } if (typeof showNotification === 'function') { showNotification(data.message || 'Update complete', data.status || 'success');