test(install): stop assuming pytest's tmp_path is on disk

test_returns_nothing_when_tmpdir_is_already_disk_backed asserted that lm_disk_backed_tmpdir prints nothing when TMPDIR is already disk-backed, and used pytest's tmp_path as the "disk-backed" directory: # tmp_path is on the regular filesystem, so the default must be kept. assert call("lm_disk_backed_tmpdir", env={"TMPDIR": str(tmp_path)}) == "" That premise is false on the platform the helper was written for. Debian 13 mounts /tmp as tmpfs -- which is the entire reason lm_disk_backed_tmpdir exists -- and pytest puts tmp_path under /tmp. So on the target platform TMPDIR is memory-backed, the helper correctly answers /var/tmp, and the test fails: E AssertionError: assert '/var/tmp' == '' The helper is right; the test was wrong. Reproduced on a box where /tmp is tmpfs and / is ext4. The test now looks for a directory whose backing store is actually disk -- tmp_path, else a scratch dir under /var/tmp, else beside the library -- using the same findmnt lookup the helper itself uses, and skips only if no disk-backed directory exists anywhere. An earlier version of this fix skipped whenever tmp_path was tmpfs, which made it skip on every machine with a tmpfs /tmp; that is barely better than asserting the wrong thing, so it now searches instead of giving up. Verified: 31 passed, 0 skipped. Mutation-checked -- deleting the "is the current TMPDIR memory-backed?" guard from lm_disk_backed_tmpdir fails this test, so it still catches the regression it is there for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW
2026-08-23 11:28:14 +00:00 · 2026-08-21 14:04:08 -04:00
28 changed files with 96 additions and 1614 deletions
@@ -100,15 +100,10 @@ TEMP_SUDOERS="/tmp/ledmatrix_web_sudoers_$$"
    echo "$WEB_USER ALL=(ALL) NOPASSWD: $SYSTEMCTL_PATH restart ledmatrix-web.service"
    # Optional: journalctl (non-critical — skip if not found)
    #
    # NOEXEC, matching first_time_install.sh. These rules end in a wildcard and
    # journalctl starts a pager, so without it the caller can reach a shell:
    # less runs "!command" as the user the pager belongs to, which here is
    # root. NOEXEC stops the granted command executing anything of its own.
    if [ -n "$JOURNALCTL_PATH" ]; then
-        echo "$WEB_USER ALL=(ALL) NOPASSWD:NOEXEC: $JOURNALCTL_PATH -u ledmatrix.service *"
+        echo "$WEB_USER ALL=(ALL) NOPASSWD: $JOURNALCTL_PATH -u ledmatrix.service *"
-        echo "$WEB_USER ALL=(ALL) NOPASSWD:NOEXEC: $JOURNALCTL_PATH -u ledmatrix *"
+        echo "$WEB_USER ALL=(ALL) NOPASSWD: $JOURNALCTL_PATH -u ledmatrix *"
-        echo "$WEB_USER ALL=(ALL) NOPASSWD:NOEXEC: $JOURNALCTL_PATH -t ledmatrix *"
+        echo "$WEB_USER ALL=(ALL) NOPASSWD: $JOURNALCTL_PATH -t ledmatrix *"
    fi
    # Required: python3, bash
@@ -25,44 +25,9 @@ if [ "$EUID" -eq 0 ]; then
    exit 1
 fi
 # Resolve command paths against a fixed PATH, and check what we resolved.
 #
 # Every path found here is written into a sudoers file as a NOPASSWD grant, so
 # whoever controls the binary at that path controls root. first_time_install.sh
 # re-execs itself with `sudo -E`, which preserves the invoking user's
 # environment -- PATH included -- so without pinning it, `which nmcli` can
 # resolve to anything on that PATH: a writable directory early in it turns a
 # compromise of the low-privilege web user into permanent root.
 PATH=/usr/sbin:/usr/bin:/sbin:/bin
 export PATH
 # A binary named in a sudoers rule must be root-owned and writable by nobody
 # else, or the grant hands root to whoever can rewrite it.
 require_trusted_binary() {
    local label="$1" path="$2"
    if [ ! -x "$path" ]; then
        echo "✗ $label: $path is not an executable file"
        exit 1
    fi
    local owner perms
    owner=$(stat -c '%u' "$path") || exit 1
    perms=$(stat -c '%a' "$path") || exit 1
    if [ "$owner" != "0" ]; then
        echo "✗ $label: $path is not owned by root (uid $owner); refusing to"
        echo "  grant it NOPASSWD sudo."
        exit 1
    fi
    # Group- or world-writable means someone other than root can replace it.
    case "$perms" in
        *[2367]) echo "✗ $label: $path is writable by group or other ($perms);"
                 echo "  refusing to grant it NOPASSWD sudo."
                 exit 1 ;;
    esac
 }
 # Get the full paths to commands
-NMCLI_PATH=$(command -v nmcli || echo "/usr/bin/nmcli")
+NMCLI_PATH=$(which nmcli || echo "/usr/bin/nmcli")
-SYSTEMCTL_PATH=$(command -v systemctl)
+SYSTEMCTL_PATH=$(which systemctl)
 echo "Command paths:"
 echo "  nmcli: $NMCLI_PATH"
@@ -72,18 +37,6 @@ echo "  systemctl: $SYSTEMCTL_PATH"
 echo ""
 echo "Step 1: Configuring sudo permissions for nmcli..."
 SUDOERS_FILE="/etc/sudoers.d/ledmatrix_wifi"
 SYSCTL_PATH=$(command -v sysctl || echo /usr/sbin/sysctl)
 NFT_PATH=$(command -v nft || echo /usr/sbin/nft)
 RFKILL_PATH=$(command -v rfkill || echo /usr/sbin/rfkill)
 MKDIR_PATH=$(command -v mkdir || echo /usr/bin/mkdir)
 # Checked before any of them reaches the sudoers file.
 require_trusted_binary "nmcli"     "$NMCLI_PATH"
 require_trusted_binary "systemctl" "$SYSTEMCTL_PATH"
 require_trusted_binary "sysctl"    "$SYSCTL_PATH"
 require_trusted_binary "nft"       "$NFT_PATH"
 require_trusted_binary "rfkill"    "$RFKILL_PATH"
 require_trusted_binary "mkdir"     "$MKDIR_PATH"
 # Create a temporary sudoers file using mktemp (handles permissions better)
 TEMP_SUDOERS=$(mktemp) || {
@@ -109,36 +62,6 @@ $WEB_USER ALL=(ALL) NOPASSWD: $SYSTEMCTL_PATH start dnsmasq
 $WEB_USER ALL=(ALL) NOPASSWD: $SYSTEMCTL_PATH stop dnsmasq
 $WEB_USER ALL=(ALL) NOPASSWD: $SYSTEMCTL_PATH restart dnsmasq
 $WEB_USER ALL=(ALL) NOPASSWD: $SYSTEMCTL_PATH restart NetworkManager
 # The captive portal turns IP forwarding on while the access point is up and
 # restores the previous value when it comes down (wifi_manager._setup_iptables_
 # redirect / _teardown_iptables_redirect). Without this rule that sudo call
 # needs a password, so forwarding stays off and clients associate to the AP but
 # cannot route. It goes unnoticed on a stock Raspberry Pi image, where
 # /etc/sudoers.d/010_pi-nopasswd grants the default user blanket NOPASSWD and
 # masks every gap in this file -- it only bites once that blanket rule is
 # removed.
 $WEB_USER ALL=(ALL) NOPASSWD: $SYSCTL_PATH -w net.ipv4.ip_forward=0
 $WEB_USER ALL=(ALL) NOPASSWD: $SYSCTL_PATH -w net.ipv4.ip_forward=1
 # The portal's redirect lives in its own nftables table, created when the AP
 # comes up and deleted when it goes down, and the radio has to be unblocked
 # before the AP can start at all. Same story as the sysctl rules above: called
 # with sudo, never granted here, and invisible on a stock Pi image.
 $WEB_USER ALL=(ALL) NOPASSWD: $NFT_PATH add table ip ledmatrix
 $WEB_USER ALL=(ALL) NOPASSWD: $NFT_PATH delete table ip ledmatrix
 $WEB_USER ALL=(ALL) NOPASSWD: $RFKILL_PATH unblock wifi
 # NetworkManager's dnsmasq drop-in directory, exact path.
 $WEB_USER ALL=(ALL) NOPASSWD: $MKDIR_PATH -p /etc/NetworkManager/dnsmasq-shared.d
 #
 # iptables is deliberately NOT granted here. Its rules are built from the live
 # interface name and port, so a rule covering them needs a trailing wildcard --
 # and `iptables --modprobe=/path/to/anything` runs that path as root, so
 # `NOPASSWD: iptables *` is a root shell for the web user by another name. That
 # is a worse outcome than the gap it would close, which today is masked anyway
 # by the blanket NOPASSWD rule on stock Pi images.
 #
 # Closing it safely means a wrapper script that builds the rules itself and
 # takes only an interface and a port, granted the way safe_plugin_rm.sh already
 # is. That belongs in its own change rather than being smuggled into this one.
 # Allow copying hostapd and dnsmasq config files into place
 $WEB_USER ALL=(ALL) NOPASSWD: /usr/bin/cp /tmp/hostapd.conf /etc/hostapd/hostapd.conf
@@ -63,9 +63,6 @@ class StartupValidator:
        if self.plugin_manager:
            self._validate_plugins()
        # Warn when the running systemd unit no longer matches the repo's
        self._validate_systemd_units()
        is_valid = len(self.errors) == 0
        if is_valid:
@@ -77,80 +74,6 @@ class StartupValidator:
        return (is_valid, self.errors.copy(), self.warnings.copy())
    #: Units this project installs, and where each is installed to.
    _UNITS = (
        ("systemd/ledmatrix.service", "/etc/systemd/system/ledmatrix.service"),
        ("systemd/ledmatrix-web.service", "/etc/systemd/system/ledmatrix-web.service"),
    )
    def _validate_systemd_units(self) -> None:
        """Warn when an installed unit has drifted from the repo's template.
        Nothing re-applies these after the first install. `git pull` -- which is
        what the web UI's update button runs -- brings a new template into the
        checkout, but nothing copies it to /etc/systemd/system and nothing runs
        `systemctl daemon-reload`, so the unit that actually runs is whatever
        first_time_install.sh wrote on day one.
        That makes every hardening added to a unit inert on existing installs.
        Measured on one rig: the installed unit was thirteen days older than the
        repo's and differed in content, so a MemoryMax the repo had specified
        was not being enforced at all -- `systemctl show` reported
        MemoryMax=infinity.
        A warning rather than an error, and certainly not a silent rewrite:
        editing files under /etc and restarting services is the installer's job,
        not something a display process should do to a machine while it boots.
        The remedy is to re-run scripts/install/install_service.sh.
        """
        try:
            project_root = Path(__file__).resolve().parent.parent
            for template_rel, installed_path in self._UNITS:
                template = project_root / template_rel
                installed = Path(installed_path)
                if not template.is_file() or not installed.is_file():
                    continue
                # The template carries placeholders the installer substitutes,
                # so compare the substituted form rather than the raw file.
                expected = template.read_text(encoding="utf-8")
                expected = expected.replace("__PROJECT_ROOT_DIR__", str(project_root))
                expected = expected.replace("__USER__", "root")
                try:
                    actual = installed.read_text(encoding="utf-8")
                except PermissionError:
                    continue
                if self._unit_body(expected) != self._unit_body(actual):
                    self.warnings.append(
                        f"{installed.name} differs from {template_rel}; the "
                        "installed unit is not refreshed by an update, so "
                        "settings added to the template are not in effect. "
                        "Re-run scripts/install/install_service.sh to apply them."
                    )
        except OSError as e:
            self.logger.debug("Could not compare systemd units: %s", e)
    @staticmethod
    def _unit_body(text: str) -> str:
        """A unit's meaningful lines, in order: no comments, no blanks.
        Order is preserved deliberately. This used to sort, which made the
        comparison insensitive to two changes that matter in a systemd unit:
        repeated directives such as ExecStartPre= and ExecStartPost= run in
        the order they appear, and a directive that moves between [Unit],
        [Service] and [Install] means something different -- or nothing --
        where it lands. A drift check that normalises those away reports no
        drift for a unit that has genuinely changed.
        """
        lines = []
        for line in text.splitlines():
            line = line.strip()
            if line and not line.startswith("#"):
                lines.append(line)
        return "\n".join(lines)
    def _validate_config(self) -> None:
        """Validate configuration files."""
        try:
@@ -5,7 +5,7 @@ Provides functions for identifying, masking, separating, and filtering
 secret fields in plugin configurations based on JSON Schema x-secret markers.
 """
-from typing import Any, Dict, Optional, Set, Tuple
+from typing import Any, Dict, Set, Tuple
 def find_secret_fields(properties: Dict[str, Any], prefix: str = '') -> Set[str]:
@@ -143,12 +143,6 @@ 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.
@@ -162,25 +156,15 @@ def mask_all_secret_values(config: Dict[str, Any]) -> Dict[str, Any]:
    Returns:
        A copy with all real values replaced by ``'••••••••'``.
    """
-    return {k: _mask_value(v) for k, v in config.items()}
+    masked: Dict[str, Any] = {}
-
+    for k, v in config.items():
-
+        if isinstance(v, dict):
-def _mask_value(value: Any) -> Any:
+            masked[k] = mask_all_secret_values(v)
-    """Mask one value, recursing through dicts and lists.
+        elif v not in (None, '') and not (isinstance(v, str) and v.startswith('YOUR_')):
-
+            masked[k] = '••••••••'
-    A list used to be masked as though it were a scalar, so
+        else:
-    ``accounts: [{"name": "a", "token": "..."}]`` came back as a single
+            masked[k] = v
-    ``'••••••••'``. Nothing leaked, but the caller could no longer see how
+    return masked
    many entries there were or any of their non-secret fields, and the raw
    editor was shown a string where the file holds an array.
    """
    if isinstance(value, dict):
        return {k: _mask_value(v) for k, v in value.items()}
    if isinstance(value, list):
        return [_mask_value(item) for item in value]
    if value in (None, '') or (isinstance(value, str) and value.startswith('YOUR_')):
        return value
    return SECRET_MASK
 def remove_empty_secrets(secrets: Dict[str, Any]) -> Dict[str, Any]:
@@ -202,133 +186,6 @@ def remove_empty_secrets(secrets: Dict[str, Any]) -> Dict[str, Any]:
            nested = remove_empty_secrets(v)
            if nested:
                result[k] = nested
        elif isinstance(v, list):
            # Lists used to fall through to the scalar branch below and be
            # kept verbatim, blanks and all. Because lists merge by
            # *replacement*, saving any unrelated setting then wrote
            # [{"token": ""}, ...] straight over the stored list and
            # destroyed every credential in it.
            pruned = _prune_secret_list(v)
            if pruned is not None:
                result[k] = pruned
        elif v is not None and not (isinstance(v, str) and v.strip() == ''):
            result[k] = v
    return result
 def _prune_secret_list(items: list) -> Optional[list]:
    """Strip blanks from inside a list of secrets, preserving every index.
    The rest of the system treats a secrets list as *parallel* to the regular
    one -- ``sec[i]`` holds the secret fields of item ``i``, and ``{}`` means
    "item i has none" (see ConfigManager._strip_secrets_recursive). So an
    emptied dict item stays ``{}``: putting ``None`` there makes that list stop
    looking parallel, and the stripper then drops the whole key from the main
    config, taking the non-secret fields with it.
    A blank *scalar* becomes ``None``, meaning "no update at this index" --
    :func:`merge_secrets` substitutes whatever is stored there. Returns
    ``None`` when nothing in the list carries a real value, so the caller drops
    the key and leaves the stored list untouched.
    """
    pruned: list = []
    has_real_value = False
    for item in items:
        if isinstance(item, dict):
            kept = remove_empty_secrets(item)
            pruned.append(kept)
            has_real_value = has_real_value or bool(kept)
        elif isinstance(item, list):
            sub = _prune_secret_list(item)
            pruned.append(sub if sub is not None else [])
            has_real_value = has_real_value or sub is not None
        elif item is not None and not (isinstance(item, str) and item.strip() == ''):
            pruned.append(item)
            has_real_value = True
        else:
            pruned.append(None)
    return pruned if has_real_value else None
 def merge_secrets(stored: Any, incoming: Any) -> Any:
    """Merge submitted secrets over stored ones, element-wise inside lists.
    ``deep_merge`` replaces a list wholesale. For secrets that is destructive:
    an incoming list that carries a real value for one entry and ``None`` for
    the rest would drop the stored credentials of every other entry. Here a
    list merges by index, and ``None`` means "keep what is stored".
    Entries are matched by *position*, which is what the config form gives us
    -- there is no schema-declared identity to key on, and it is the same
    contract ConfigManager._strip_secrets_recursive already relies on. The
    incoming list's length wins, so deleting an item deletes its secrets;
    an item the client left blank keeps whatever is stored at that index.
    """
    if isinstance(stored, dict) and isinstance(incoming, dict):
        merged = dict(stored)
        for key, value in incoming.items():
            merged[key] = (merge_secrets(stored[key], value)
                           if key in stored else value)
        return merged
    if isinstance(stored, list) and isinstance(incoming, list):
        # The incoming list sets the length -- the regular config's list is
        # authoritative about how many items exist, and this one runs parallel
        # to it. Removing an entry must therefore remove its secrets too.
        merged_list = []
        for index, item in enumerate(incoming):
            stored_item = stored[index] if index < len(stored) else None
            merged_list.append(stored_item if item is None
                               else merge_secrets(stored_item, item))
        return merged_list
    if incoming is None:
        return stored
    return incoming
 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 isinstance(v, list):
            # A list is merged by replacement, not element by element -- there
            # is no identity to match entries on -- so a list that still holds
            # a mask cannot be merged safely: keeping it would store bullets,
            # and keeping the submitted entries alone would drop whichever the
            # client did not send back. Dropping the key leaves the stored
            # list untouched, which is what an untouched list should do.
            #
            # The consequence, deliberately: editing one secret inside a list
            # through this endpoint requires sending real values for all of
            # them. Sending some masks leaves the whole list as it was.
            if not _contains_mask(v):
                result[k] = v
        elif v is None:
            continue
        elif isinstance(v, str) and (v.strip() == '' or v == SECRET_MASK):
            continue
        else:
            result[k] = v
    return result
 def _contains_mask(value: Any) -> bool:
    """True when a mask sentinel survives anywhere inside ``value``."""
    if isinstance(value, dict):
        return any(_contains_mask(v) for v in value.values())
    if isinstance(value, list):
        return any(_contains_mask(item) for item in value)
    return value == SECRET_MASK
@@ -8,18 +8,6 @@ Type=simple
 User=root
 WorkingDirectory=__PROJECT_ROOT_DIR__
 Environment=PYTHONDONTWRITEBYTECODE=1
 # glibc gives each allocating thread its own malloc arena, up to 8 x CPU count,
 # and an arena that has grown is never handed back to the OS. This process runs
 # 9 threads on a 3-core Pi, so the ceiling is 24 arenas -- and a rig measured at
 # 1030 MB resident held 23 large anonymous mappings on 64 MB-aligned addresses,
 # 920 MB of them, while the live data it was actually holding (widest scroll
 # strip seen: 35,746 x 64) accounts for roughly 15 MB. That gap is arena bloat,
 # not leaked objects: RSS was flat across repeated sampling, not climbing.
 #
 # Capping the arenas trades a little allocator concurrency for a large amount of
 # resident memory on a device that has neither to spare. 2 is the usual value;
 # raise it if frame times regress.
 Environment=MALLOC_ARENA_MAX=2
 ExecStart=/usr/bin/python3 __PROJECT_ROOT_DIR__/run.py
 # Restart=always, not on-failure: run.py exiting 0 (a clean shutdown path taken
 # for a reason that no longer applies, e.g. a config reload) would otherwise leave
@@ -1,143 +0,0 @@
 """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"
@@ -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', '--autostash']
+    assert args == ['git', 'pull', '--rebase']
    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', '--autostash', 'origin', 'audit']
+    assert args == ['git', 'pull', '--rebase', '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', '--autostash']
+    assert args == ['git', 'pull', '--rebase']
    assert note == ''
@@ -200,44 +200,3 @@ 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}
        # git ls-files says nothing about a path it does not track, so a
        # renamed or deleted script would simply be absent here and the mode
        # check below would pass over it silently.
        untracked = sorted(set(self.CHMODDED) - set(modes))
        assert not untracked, (
            f"{untracked} are chmodded by the installer but not tracked by "
            "git, so their mode cannot be asserted at all")
        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")
@@ -13,6 +13,7 @@ need root and mutate the system, so they are exercised manually instead.
 """
 import subprocess
 import tempfile
 from pathlib import Path
 import pytest
@@ -31,6 +32,16 @@ def run_lib(snippet: str, env: dict | None = None) -> subprocess.CompletedProces
    )
 def _fstype_of(path: object) -> str:
    """Filesystem type backing ``path``, via the same tool the helper uses."""
    result = subprocess.run(
        ["findmnt", "-no", "FSTYPE", "--target", str(path)],
        capture_output=True, text=True,
        env={"PATH": "/usr/bin:/bin:/usr/sbin:/sbin"},
    )
    return result.stdout.strip()
 def call(fn: str, *args: object, env: dict | None = None) -> str:
    joined = " ".join(str(a) for a in args)
    result = run_lib(f"{fn} {joined}", env=env)
@@ -195,8 +206,29 @@ class TestOomDetection:
 class TestDiskBackedTmpdir:
    def test_returns_nothing_when_tmpdir_is_already_disk_backed(self, tmp_path):
-        # tmp_path is on the regular filesystem, so the default must be kept.
+        # Do not assume tmp_path is disk-backed. Debian 13 -- the platform this
-        assert call("lm_disk_backed_tmpdir", env={"TMPDIR": str(tmp_path)}) == ""
+        # helper exists for -- mounts /tmp as tmpfs, and pytest puts tmp_path
        # under /tmp, so this asserted against a *memory*-backed directory and
        # failed on the target platform while the helper behaved exactly as
        # designed. Search for a directory whose backing store is really disk.
        scratch = None
        disk_backed = None
        for candidate in (tmp_path, Path("/var/tmp"), LIB.parent):
            if _fstype_of(candidate) not in ("tmpfs", "ramfs", ""):
                if candidate is tmp_path:
                    disk_backed = candidate
                else:
                    scratch = Path(tempfile.mkdtemp(dir=str(candidate)))
                    disk_backed = scratch
                break
        if disk_backed is None:
            pytest.skip("no disk-backed directory available to test against")
        try:
            assert call("lm_disk_backed_tmpdir",
                        env={"TMPDIR": str(disk_backed)}) == ""
        finally:
            if scratch is not None:
                scratch.rmdir()
    def test_redirects_away_from_a_memory_backed_tmpdir(self):
        # Debian 13 mounts /tmp as tmpfs, which would otherwise hold the whole
@@ -1,147 +0,0 @@
 """The captive portal's fixed-argument sudo calls must be granted.
 The installers write two allow-lists, /etc/sudoers.d/ledmatrix_web and
 ledmatrix_wifi. A sudo call absent from both needs a password, which a service
 cannot supply, so it fails.
 Four such calls were ungranted, all of them captive-portal teardown/setup:
    sysctl -w net.ipv4.ip_forward=0|1     wifi_manager.py:788, 883
    nft add|delete table ip ledmatrix     wifi_manager.py:835, 895
    rfkill unblock wifi                   wifi_manager.py:1811
    mkdir -p .../dnsmasq-shared.d         wifi_manager.py:922
 It goes unnoticed because a stock Raspberry Pi image ships
 /etc/sudoers.d/010_pi-nopasswd granting the default user
 `ALL=(ALL) NOPASSWD: ALL`, which satisfies every gap in both files. It only
 bites once that blanket rule is removed or the service runs as another user.
 Scope, deliberately narrow: this pins the four commands above, each of which
 can be written out literally. The portal makes further sudo calls whose
 arguments are built at runtime -- iptables and nft rules carrying an interface
 name and a port, `ip addr`, `ip link` -- and those cannot be granted safely
 here. A rule covering them needs a trailing wildcard, and
 `iptables --modprobe=/path/to/anything` runs that path as root, so
 `NOPASSWD: iptables *` is a root shell for the web user by another name.
 Closing that half needs a privileged helper that builds the rules itself and
 takes only an interface and a port, granted the way safe_plugin_rm.sh already
 is. That is a design decision, not a one-line grant, and belongs in its own
 change.
 """
 import re
 from pathlib import Path
 import pytest
 ROOT = Path(__file__).resolve().parent.parent
 INSTALLERS = (
    ROOT / "first_time_install.sh",
    ROOT / "scripts" / "install" / "configure_wifi_permissions.sh",
 )
 #: Commands this change grants, each fully literal in the source.
 REQUIRED = (
    ("sysctl", "-w", "net.ipv4.ip_forward=0"),
    ("sysctl", "-w", "net.ipv4.ip_forward=1"),
    ("nft", "add", "table", "ip", "ledmatrix"),
    ("nft", "delete", "table", "ip", "ledmatrix"),
    ("rfkill", "unblock", "wifi"),
    ("mkdir", "-p", "/etc/NetworkManager/dnsmasq-shared.d"),
 )
 #: Tools with an option that executes a program of the caller's choosing.
 #: A trailing wildcard on any of these is a privilege escalation.
 EXEC_CAPABLE = ("iptables", "ip6tables", "nft", "tcpdump", "find", "awk",
                "sed", "perl", "python", "python3", "env")
 def _grant_lines():
    lines = []
    for installer in INSTALLERS:
        if not installer.is_file():
            continue
        for line in installer.read_text(encoding="utf-8", errors="replace").splitlines():
            if "NOPASSWD:" in line:
                lines.append(line.split("NOPASSWD:", 1)[1])
    return lines
 def _normalise(rule):
    """One rule with binary-path variables reduced to bare tool names.
    Rules are written as `$SYSCTL_PATH -w ...` or `${NFT_PATH} ...`, so
    matching the literal "sysctl" finds nothing and every rule looks absent --
    which is exactly how an earlier version of this test reported six gaps
    that did not exist. Both spellings are handled: shell expands them
    identically, and a check that understood only one silently skipped the
    other.
    """
    rule = re.sub(r"\$\{?([A-Z][A-Z0-9_]*)_PATH\}?",
                  lambda m: m.group(1).lower(), rule)
    return re.sub(r"/usr/(?:s?bin)/", "", rule)
 def _granted_commands():
    """The command each NOPASSWD rule actually grants, normalised.
    _grant_lines() already returns everything after "NOPASSWD:", so what
    arrives here is the command, possibly preceded by the NOEXEC tag and
    possibly still carrying the closing quote of an `echo "..."` that wrote
    it. Both are stripped so the result is comparable to a plain command.
    """
    commands = []
    for rule in _grant_lines():
        command = _normalise(rule).strip()
        command = re.sub(r"^NOEXEC:\s*", "", command)
        command = command.rstrip('"').rstrip("'").strip()
        if command:
            commands.append(" ".join(command.split()))
    return commands
 def test_the_installers_are_present():
    missing = [str(p.relative_to(ROOT)) for p in INSTALLERS if not p.is_file()]
    assert not missing, f"installer(s) missing: {missing}"
@pytest.mark.parametrize("command", REQUIRED, ids=lambda c: " ".join(c))
 def test_the_command_is_granted(command):
    """Whole command, not just the binary.
    Checking only the binary made this far weaker than it looked: with
    `sysctl` present anywhere, deleting the ip_forward=0 grant still passed,
    and the portal would then be unable to restore forwarding on teardown.
    """
    wanted = " ".join(command)
    granted = _granted_commands()
    # Exact match, not a prefix. A substring search was satisfied by
    # `sysctl -w net.ipv4.ip_forward=0 *`, and that trailing wildcard lets the
    # caller append whatever they like to a command running as root -- a far
    # wider grant than the one this test is meant to be confirming.
    assert wanted in granted, (
        f"no installer grants exactly `{wanted}`; closest matches: "
        + str([g for g in granted if g.startswith(command[0])])[:200])
 def test_no_wildcard_on_a_tool_that_can_exec():
    """`NOPASSWD: iptables *` hands the web user root.
    iptables --modprobe=/path runs that path as root. This caught a grant added
    in this very change, which is why it is here.
    """
    offenders = []
    for rule in _grant_lines():
        rule = rule.strip()
        if not rule.endswith("*"):
            continue
        # Normalised the same way as everything else: `${NFT_PATH} *` left a
        # brace before the tool name, and the word-boundary check below does
        # not treat "{" as a boundary, so that spelling slipped through.
        haystack = _normalise(rule).lower()
        for tool in EXEC_CAPABLE:
            if re.search(rf"(^|/|\s|\$){tool}(\s|$)", haystack):
                offenders.append(rule)
                break
    assert not offenders, (
        "wildcard grant on a tool that can execute another program:\n  "
        + "\n  ".join(offenders))
@@ -30,10 +30,6 @@ ROOT = Path(__file__).resolve().parent.parent
 INSTALLERS = (
    ROOT / "first_time_install.sh",
    ROOT / "scripts" / "install" / "configure_wifi_permissions.sh",
    # Writes the same journalctl grants as first_time_install.sh. It was
    # missing here, and because of that this suite passed while three
    # ungranted wildcard rules sat in it.
    ROOT / "scripts" / "install" / "configure_web_sudo.sh",
 )
 #: Commands that will start another program of their own accord -- a pager, an
@@ -48,14 +44,8 @@ def _grant_lines():
            continue
        for line in installer.read_text(encoding="utf-8", errors="replace").splitlines():
            stripped = line.strip()
-            if "NOPASSWD" not in stripped or stripped.startswith("#"):
+            if "NOPASSWD" in stripped and not stripped.startswith("#"):
-                continue
+                lines.append(stripped)
            # Installers emit rules two ways: written literally into a heredoc,
            # or echoed into a file. An echoed rule ends in a quote, so the
            # trailing-wildcard check below would skip it and the rule would
            # never be examined at all.
            echoed = re.fullmatch(r"""echo\s+(['"])(.*)\1""", stripped)
            lines.append(echoed.group(2) if echoed else stripped)
    return lines
@@ -88,12 +78,10 @@ def test_journalctl_is_granted_at_all():
        "no journalctl grant remains; the web interface reads logs through it")
-@pytest.mark.parametrize("selector", ["-u ledmatrix.service", "-u ledmatrix",
+@pytest.mark.parametrize("unit", ["ledmatrix.service", "ledmatrix"])
-                                      "-t ledmatrix"])
+def test_each_journalctl_rule_is_tagged(unit):
 def test_each_journalctl_rule_is_tagged(selector):
    """Every selector, so removing one cannot pass by the others' presence."""
    matching = [r for r in _grant_lines()
-                if "JOURNALCTL_PATH" in r and f"{selector} " in r]
+                if "JOURNALCTL_PATH" in r and f"-u {unit} " in r]
-    assert matching, f"no journalctl rule for {selector}"
+    assert matching, f"no journalctl rule for -u {unit}"
    untagged = [r for r in matching if "NOEXEC" not in r]
    assert not untagged, f"untagged journalctl rule(s): {untagged}"
@@ -1,95 +0,0 @@
 """The display unit must cap glibc's malloc arenas.
 glibc hands each allocating thread its own malloc arena, up to 8 x CPU count,
 and an arena that has grown is never returned to the OS. This process runs
 threads for the render loop, the update workers and the background fetchers, so
 on a 3-core Pi the ceiling is 24 arenas.
 Measured on a live rig, 2.5 hours in:
    RSS                 1030 MB
    Private_Dirty        988 MB
    anonymous mappings > 10 MB     23     (ceiling is 8 x 3 = 24)
    largest few          104, 79, 66, 63, 63 MB, on 64 MB-aligned addresses
 against live data that accounts for perhaps 15 MB -- the widest scroll strip
 observed was 35,746 x 64, about 7 MB as RGB and the same again for its numpy
 mirror. Repeated sampling showed RSS flat between 990 and 1030 MB rather than
 climbing, so this is arena bloat rather than a leak: memory Python has freed
 but glibc is holding per-arena.
 The device had 59 MB free at the time.
 Capping the arena count trades a little allocator concurrency for that resident
 memory. The render loop is latency-sensitive, so if p99 frame time regresses the
 right response is to raise this rather than remove it.
 """
 import re
 from pathlib import Path
 import pytest
 UNIT = (Path(__file__).resolve().parent.parent / "systemd" / "ledmatrix.service")
 #: The value the unit is expected to carry. 2 is the usual choice for a
 #: threaded Python process; 1-4 all keep some of the saving, but only one of
 #: them is what this project ships.
 EXPECTED_ARENA_MAX = 2
 def _environment(unit_text):
    return dict(
        line.split("=", 2)[1:3] if line.count("=") >= 2 else (line.split("=", 1)[1], "")
        for line in unit_text.splitlines()
        if line.startswith("Environment=")
    )
 def test_the_unit_exists():
    assert UNIT.is_file(), f"{UNIT} is missing"
 def test_malloc_arena_max_is_capped():
    env = _environment(UNIT.read_text(encoding="utf-8"))
    assert "MALLOC_ARENA_MAX" in env, (
        "the display unit does not cap glibc arenas; on a 3-core Pi the default "
        "ceiling is 24 and a measured rig held 23 of them, 920 MB"
    )
    value = int(env["MALLOC_ARENA_MAX"])
    # Pinned, not a range. A range let a change to 4 -- which hands most of the
    # saving back -- pass unnoticed, which was the point of the finding that
    # prompted this. Raising it is a legitimate response to a frame-time
    # regression, but it should be a visible edit here rather than a silent
    # drift, so the number lives in one place and changing it shows up in
    # review.
    assert value == EXPECTED_ARENA_MAX, (
        f"MALLOC_ARENA_MAX={value}, expected {EXPECTED_ARENA_MAX}. If this was "
        "raised deliberately because frame times regressed, update "
        "EXPECTED_ARENA_MAX here and say so in the commit."
    )
 def test_the_reason_is_recorded_next_to_it():
    """A bare tuning knob invites removal by whoever meets it next."""
    text = UNIT.read_text(encoding="utf-8")
    index = text.index("Environment=MALLOC_ARENA_MAX")
    preamble = text[:index].splitlines()[-12:]
    comment = "\n".join(line for line in preamble if line.startswith("#"))
    assert "arena" in comment.lower(), "no explanation precedes the setting"
    assert re.search(r"\d", comment), (
        "the explanation cites no measurement, so a reader cannot tell whether "
        "it still applies to their hardware"
    )
@pytest.mark.parametrize("unit", ["ledmatrix.service"])
 def test_the_unit_still_parses_as_ini(unit):
    """systemd will refuse a malformed unit, and the panel stays dark."""
    import configparser
    path = UNIT.parent / unit
    parser = configparser.ConfigParser(strict=False)
    # systemd allows repeated keys; ConfigParser needs them merged, not rejected.
    parser.read_string(path.read_text(encoding="utf-8"))
    assert parser.has_section("Service")
    assert parser.has_option("Service", "ExecStart")
@@ -1,152 +0,0 @@
 """An installed unit that no longer matches the repo's must be reported.
 Nothing re-applies systemd units after the first install. `git pull` -- what
 the web UI's update button runs -- brings a new template into the checkout, but
 no code in web_interface/ or src/ copies it to /etc/systemd/system or runs
 `systemctl daemon-reload`. The unit that actually runs is whatever
 first_time_install.sh wrote on day one.
 So every hardening added to a unit is inert on existing installs. Measured on a
 live rig: the installed unit was dated 2026-08-06 and the repo's 2026-08-19,
 and they differed -- with the result that a MemoryMax=85% present in the repo's
 template was not being enforced at all. `systemctl show` reported
 MemoryMax=infinity.
 This is a warning, not an error, and deliberately not a silent rewrite:
 editing files under /etc and restarting services is the installer's job, not
 something a display process should do to a machine while it boots.
 """
 import logging
 from pathlib import Path
 from unittest.mock import MagicMock
 import pytest
 from src.startup_validator import StartupValidator
@pytest.fixture
 def validator():
    v = StartupValidator(config_manager=MagicMock())
    v.logger = logging.getLogger("test")
    v.warnings = []
    v.errors = []
    return v
 def test_a_matching_unit_produces_no_warning(validator, tmp_path):
    """The installed unit, substituted exactly as the installer would."""
    project_root = Path("src/startup_validator.py").resolve().parent.parent
    template_rel = "systemd/ledmatrix.service"
    template = project_root / template_rel
    if not template.is_file():
        pytest.skip("repo unit template not present")
    installed = tmp_path / "ledmatrix.service"
    installed.write_text(
        template.read_text(encoding="utf-8")
        .replace("__PROJECT_ROOT_DIR__", str(project_root))
        .replace("__USER__", "root"),
        encoding="utf-8")
    validator._UNITS = ((template_rel, str(installed)),)
    validator._validate_systemd_units()
    assert not validator.warnings, f"a matching unit warned: {validator.warnings}"
    assert not validator.errors
 def test_comments_and_blank_lines_are_not_drift():
    """Otherwise every comment the repo adds would look like a changed unit."""
    a = "[Service]\n# explains a setting\nExecStart=/x\nRestart=always\n"
    b = "[Service]\nExecStart=/x\n\nRestart=always\n"
    assert StartupValidator._unit_body(a) == StartupValidator._unit_body(b)
 def test_a_changed_directive_is_drift():
    a = "[Service]\nExecStart=/x\nMemoryMax=85%\n"
    b = "[Service]\nExecStart=/x\n"
    assert StartupValidator._unit_body(a) != StartupValidator._unit_body(b)
 def test_reordered_directives_are_drift():
    """Order is not noise in a systemd unit.
    Repeated directives -- ExecStartPre=, ExecStartPost= -- run in the order
    they appear, and a directive that moves between [Unit], [Service] and
    [Install] means something different, or nothing, where it lands. This
    check used to sort the lines before comparing, which reported no drift for
    a unit that had genuinely changed.
    """
    a = "[Service]\nExecStartPre=/first\nExecStartPre=/second\n"
    b = "[Service]\nExecStartPre=/second\nExecStartPre=/first\n"
    assert StartupValidator._unit_body(a) != StartupValidator._unit_body(b), (
        "swapping two ExecStartPre= lines changes what runs first, and was "
        "being normalised away")
 def test_a_directive_moved_between_sections_is_drift():
    a = "[Unit]\nDescription=x\n[Service]\nExecStart=/x\n"
    b = "[Unit]\nDescription=x\nExecStart=/x\n[Service]\n"
    assert StartupValidator._unit_body(a) != StartupValidator._unit_body(b), (
        "ExecStart= in [Unit] is not the same unit, and sorting hid it")
 def test_cosmetic_differences_do_not_warn(validator, tmp_path):
    """Through the real comparison, not the helper.
    The repo's template carries explanatory comments the installed copy may not
    have, and the installer does not preserve ordering or blank lines. If those
    counted as drift, every boot would warn and the warning would be ignored.
    Asserting this on _unit_body alone would not catch a comparison that stopped
    calling it -- which is exactly what a careless edit does.
    """
    project_root = Path("src/startup_validator.py").resolve().parent.parent
    template_rel = "systemd/ledmatrix.service"
    template = project_root / template_rel
    if not template.is_file():
        pytest.skip("repo unit template not present")
    substituted = (template.read_text(encoding="utf-8")
                   .replace("__PROJECT_ROOT_DIR__", str(project_root))
                   .replace("__USER__", "root"))
    # Cosmetic means comments, blank lines and stray indentation -- the things
    # the installer really does drop. Not reordering: that changes the unit,
    # and is asserted as drift above.
    directives = [line.strip() for line in substituted.splitlines()
                  if line.strip() and not line.strip().startswith("#")]
    installed = tmp_path / "ledmatrix.service"
    installed.write_text(
        "\n\n".join("   " + d for d in directives) + "\n", encoding="utf-8")
    validator._UNITS = ((template_rel, str(installed)),)
    validator._validate_systemd_units()
    assert not validator.warnings, (
        f"cosmetic-only difference reported as drift: {validator.warnings}")
 def test_drift_is_reported_as_a_warning(validator, tmp_path):
    """The whole point: a real difference must surface, and only as a warning."""
    installed = tmp_path / "ledmatrix.service"
    installed.write_text("[Service]\nExecStart=/usr/bin/python3 /x/run.py\n")
    project_root = Path("src/startup_validator.py").resolve().parent.parent
    template_rel = "systemd/ledmatrix.service"
    template = project_root / template_rel
    if not template.is_file():
        pytest.skip("repo unit template not present")
    validator._UNITS = ((template_rel, str(installed)),)
    validator._validate_systemd_units()
    assert validator.warnings, "a differing unit produced no warning"
    assert "install_service.sh" in validator.warnings[0], (
        "the warning does not tell the user how to fix it")
    assert not validator.errors, "drift must not be fatal at startup"
 def test_a_missing_installed_unit_is_silent(validator, tmp_path):
    """Development checkouts have no /etc/systemd unit; that is not drift."""
    validator._UNITS = (("systemd/ledmatrix.service", str(tmp_path / "absent.service")),)
    validator._validate_systemd_units()
    assert not validator.warnings
    assert not validator.errors
@@ -1,85 +0,0 @@
 """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"
@@ -1,84 +0,0 @@
 """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
@@ -194,84 +194,15 @@ class TestSavePluginConfig:
    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 1: the posted accounts
+        # the separated secrets dict. Here that is 2: the posted accounts
-        # array, whose item tokens all count as ONE key.
+        # array (all its item tokens count as ONE key) plus the schema's
-        #
+        # api_key default ("") that merge_with_defaults adds before
-        # It was 2 before blank secrets were dropped, the second being the
+        # separation.
        # 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 "(1 secret field(s) saved to config_secrets.json)" in message
+        assert "(2 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_an_unrelated_edit_does_not_erase_array_item_secrets(self, env):
        """The scalar api_key case above, but for a list of credentials.
        remove_empty_secrets recursed into dicts only, so a list went into
        deep_merge untouched -- and lists merge by *replacement*. Saving any
        unrelated field posted [{"token": ""}, ...] straight over the stored
        array and destroyed every token in it at once.
        """
        assert self._save(env, {"accounts": [
            {"name": "a", "token": "REAL-A"},
            {"name": "b", "token": "REAL-B"},
        ], "city": "Austin"}).status_code == 200
        # the user changes the city; both masked tokens ride along blank
        assert self._save(env, {"accounts": [
            {"name": "a", "token": ""},
            {"name": "b", "token": ""},
        ], "city": "Dallas"}).status_code == 200
        merged = env.fresh_load()[PLUGIN_ID]
        assert [a.get("token") for a in merged["accounts"]] == \
            ["REAL-A", "REAL-B"], "an unrelated edit destroyed the array secrets"
        assert [a["name"] for a in merged["accounts"]] == ["a", "b"]
        assert merged["city"] == "Dallas"
    def test_one_array_secret_can_be_changed_without_losing_the_rest(self, env):
        assert self._save(env, {"accounts": [
            {"name": "a", "token": "REAL-A"},
            {"name": "b", "token": "REAL-B"},
        ]}).status_code == 200
        assert self._save(env, {"accounts": [
            {"name": "a", "token": ""},
            {"name": "b", "token": "NEW-B"},
        ]}).status_code == 200
        merged = env.fresh_load()[PLUGIN_ID]
        assert [a.get("token") for a in merged["accounts"]] == ["REAL-A", "NEW-B"]
    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,
@@ -1,45 +0,0 @@
 """The validation logging ran before separate_secrets, so it logged credentials.
 api_v3's plugin-config save logged `Full config: {plugin_config}` at INFO and
 `Config that failed: {plugin_config}` at ERROR. Both run *before*
 separate_secrets(), so plugin_config still held the values the user just typed
 into the form -- API keys and tokens went to the journal in clear text.
 """
 import re
 from pathlib import Path
 import pytest
 SOURCE = (Path(__file__).resolve().parents[2]
          / "web_interface" / "blueprints" / "api_v3.py")
 #: Objects that still hold submitted secret values at the point these log
 #: calls run. Interpolating one whole into a log message leaks credentials.
 UNREDACTED = ("plugin_config", "secrets_config", "current_secrets")
 def _logging_lines():
    for number, line in enumerate(SOURCE.read_text(encoding="utf-8").splitlines(), 1):
        stripped = line.strip()
        if stripped.startswith("#"):
            continue
        if re.match(r"logger\.(debug|info|warning|error|critical|exception)\(", stripped):
            yield number, stripped
@pytest.mark.parametrize("name", UNREDACTED)
 def test_no_log_call_interpolates_a_whole_secret_bearing_object(name):
    # {name} or {name['k']} leaks; {list(name.keys())} and {len(name)} do not.
    bare = re.compile(r"\{" + re.escape(name) + r"(\[[^\]]*\])*\}")
    offenders = [f"{n}: {text}" for n, text in _logging_lines() if bare.search(text)]
    assert not offenders, (
        f"{name} still holds submitted secrets where these log calls run:\n  "
        + "\n  ".join(offenders))
 def test_the_guard_would_notice_a_reintroduced_leak():
    """Pin the detector itself, so a rewrite cannot silently stop matching."""
    bare = re.compile(r"\{" + re.escape("plugin_config") + r"(\[[^\]]*\])*\}")
    assert bare.search('logger.info(f"Full config: {plugin_config}")')
    assert bare.search("logger.error(f\"{plugin_config['api_key']}\")")
    assert not bare.search('logger.info(f"{list(plugin_config.keys())}")')
@@ -1,123 +0,0 @@
 """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)
    # Assert the write succeeded. A 500 leaves the old file in place, so the
    # assertions below would hold without the write path running at all.
    resp = env.client.post("/api/v3/config/raw/secrets", json=_get(env))
    assert resp.status_code == 200, resp.get_data(as_text=True)[:200]
    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
 def test_a_list_of_secrets_keeps_its_shape(env):
    """A list must not be masked as though it were one scalar.
    accounts: [{...}, {...}] came back as a single '••••••••', so a caller
    could not see how many entries existed, and the raw editor was shown a
    string where the file holds an array.
    """
    env.secrets_file.write_text(json.dumps({
        "myplugin": {"accounts": [{"name": "a", "token": "tok-a"},
                                  {"name": "b", "token": "tok-b"}]}}))
    accounts = _get(env)["myplugin"]["accounts"]
    assert isinstance(accounts, list), "the list was flattened to a scalar"
    assert len(accounts) == 2, "entries were lost"
    assert all(isinstance(a, dict) for a in accounts), "entry shape was lost"
    assert "tok-a" not in json.dumps(accounts), "a token survived masking"
 def test_a_list_posted_back_unchanged_is_left_alone(env):
    """Lists merge by replacement, so a half-masked list must not be stored."""
    original = {"myplugin": {"accounts": [{"name": "a", "token": "tok-a"},
                                          {"name": "b", "token": "tok-b"}]}}
    env.secrets_file.write_text(json.dumps(original))
    resp = env.client.post("/api/v3/config/raw/secrets", json=_get(env))
    assert resp.status_code == 200, resp.get_data(as_text=True)[:200]
    assert _on_disk(env.secrets_file)["myplugin"]["accounts"] == \
        original["myplugin"]["accounts"], "round-tripping the mask damaged the list"
 def test_a_fully_supplied_list_still_saves(env):
    env.secrets_file.write_text(json.dumps(
        {"myplugin": {"accounts": [{"name": "a", "token": "old"}]}}))
    body = _get(env)
    body["myplugin"]["accounts"] = [{"name": "a", "token": "new"}]
    resp = env.client.post("/api/v3/config/raw/secrets", json=body)
    assert resp.status_code == 200, resp.get_data(as_text=True)[:200]
    assert _on_disk(env.secrets_file)["myplugin"]["accounts"][0]["token"] == "new"
@@ -17,7 +17,6 @@ from src.web_interface.secret_helpers import (
    separate_secrets,
    mask_secret_fields,
    mask_all_secret_values,
    merge_secrets,
    remove_empty_secrets,
 )
@@ -240,67 +239,3 @@ class TestRemoveEmptySecrets:
    def test_keeps_falsey_non_string_values(self):
        # 0 and False are neither None nor blank strings — they are kept.
        assert remove_empty_secrets({"a": 0, "b": False}) == {"a": 0, "b": False}
 class TestArrayItemSecrets:
    """Lists merge by replacement, so a blanked array wipes stored credentials.
    remove_empty_secrets recursed into dicts but let a list through untouched,
    so [{"token": ""}] went straight into deep_merge and overwrote the stored
    list. Saving any unrelated setting destroyed every token in the array.
    """
    STORED = {"accounts": [{"name": "a", "token": "REAL-A"},
                           {"name": "b", "token": "REAL-B"}]}
    def test_an_unrelated_save_keeps_every_stored_token(self):
        posted = {"accounts": [{"name": "a", "token": ""},
                               {"name": "b", "token": ""}]}
        merged = merge_secrets(self.STORED, remove_empty_secrets(posted))
        assert [a["token"] for a in merged["accounts"]] == ["REAL-A", "REAL-B"]
    def test_editing_one_entry_leaves_the_others_alone(self):
        posted = {"accounts": [{"name": "a", "token": ""},
                               {"name": "b", "token": "NEW-B"}]}
        merged = merge_secrets(self.STORED, remove_empty_secrets(posted))
        assert [a["token"] for a in merged["accounts"]] == ["REAL-A", "NEW-B"]
    def test_a_new_entry_is_appended(self):
        posted = {"accounts": [{"name": "a", "token": ""},
                               {"name": "b", "token": ""},
                               {"name": "c", "token": "NEW-C"}]}
        merged = merge_secrets(self.STORED, remove_empty_secrets(posted))
        assert [a["token"] for a in merged["accounts"]] == \
            ["REAL-A", "REAL-B", "NEW-C"]
    def test_a_list_of_bare_strings_merges_by_index(self):
        merged = merge_secrets({"keys": ["K1", "K2", "K3"]},
                               remove_empty_secrets({"keys": ["", "K2-NEW", ""]}))
        assert merged["keys"] == ["K1", "K2-NEW", "K3"]
    def test_an_all_blank_list_is_dropped_entirely(self):
        posted = {"accounts": [{"token": ""}, {"token": ""}]}
        assert "accounts" not in remove_empty_secrets(posted)
    def test_plain_dict_secrets_are_unaffected(self):
        merged = merge_secrets({"api_key": "OLD", "other": "keep"},
                               remove_empty_secrets({"api_key": "", "other": "changed"}))
        assert merged == {"api_key": "OLD", "other": "changed"}
    def test_a_removed_entry_takes_its_secret_with_it(self):
        """The regular config's list is authoritative about how many items
        exist, and the secrets list runs parallel to it -- see
        ConfigManager._strip_secrets_recursive. So a shorter incoming list
        must shorten the stored secrets too, or the two fall out of step."""
        posted = {"accounts": [{"name": "a", "token": "NEW-A"}]}
        merged = merge_secrets(self.STORED, remove_empty_secrets(posted))
        assert [a["token"] for a in merged["accounts"]] == ["NEW-A"]
    def test_an_emptied_item_stays_a_dict_not_none(self):
        """None there stops the list looking parallel, and
        _strip_secrets_recursive then drops the whole key from the main
        config -- deleting the item's non-secret fields as well."""
        pruned = remove_empty_secrets(
            {"accounts": [{"token": "real"}, {"token": ""}]})
        assert pruned["accounts"] == [{"token": "real"}, {}]
        assert None not in pruned["accounts"]
@@ -21,10 +21,7 @@ logger = logging.getLogger(__name__)
 # 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, mask_all_secret_values,
+from src.web_interface.secret_helpers import find_secret_fields, separate_secrets
                                              merge_secrets, 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 (
@@ -265,54 +262,15 @@ 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, with credentials redacted."""
+    """Get main configuration"""
    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': _redact_credentials(config)})
+        return jsonify({'status': 'success', 'data': 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
@@ -757,12 +715,10 @@ def save_main_config():
        if not data:
            return jsonify({'status': 'error', 'message': 'No data provided'}), 400
-        # What arrives here is the config itself, and the headers carry the
+        import logging
-        # session cookie -- neither belongs in the journal, least of all at
+        logging.error(f"DEBUG: save_main_config received data: {data}")
-        # ERROR on every save. The shape of the request is the part with
+        logging.error(f"DEBUG: Content-Type header: {request.content_type}")
-        # diagnostic value, so log that, at the level it deserves.
+        logging.error(f"DEBUG: Headers: {dict(request.headers)}")
        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()
@@ -1260,11 +1216,6 @@ def save_main_config():
                # 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
@@ -1297,10 +1248,7 @@ def save_main_config():
                if secrets_config:
                    if plugin_id not in current_secrets:
                        current_secrets[plugin_id] = {}
-                    # Lists merge by replacement, so deep_merge here wrote a
+                    current_secrets[plugin_id] = deep_merge(current_secrets[plugin_id], secrets_config)
                    # blanked array straight over the stored credentials.
                    current_secrets[plugin_id] = merge_secrets(
                        current_secrets[plugin_id], secrets_config)
                    # Save secrets file
                    api_v3.config_manager.save_raw_file_content('secrets', current_secrets)
@@ -1385,12 +1333,7 @@ def get_secrets_config():
            return jsonify({'status': 'error', 'message': 'Config manager not initialized'}), 500
        config = api_v3.config_manager.get_raw_file_content('secrets')
-        # This interface has no authentication, and this file is nothing but
+        return jsonify({'status': 'success', 'data': config})
        # 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
@@ -1458,19 +1401,8 @@ def save_raw_secrets_config():
        if not data:
            return jsonify({'status': 'error', 'message': 'No data provided'}), 400
-        # The GET above masks what it returns, and this endpoint's only client
+        # Save the secrets config
-        # reads the whole file, edits one field and posts all of it back. So
+        api_v3.config_manager.save_raw_file_content('secrets', data)
        # 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:
@@ -1728,22 +1660,13 @@ 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/<branch>`` 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', '--autostash'], '', None
+        return ['git', 'pull', '--rebase'], '', None
    branch = _git_current_branch(project_dir)
    if not branch:
@@ -1753,7 +1676,7 @@ def resolve_pull_command(project_dir):
        )
    if _git_remote_branch_exists(project_dir, branch):
        return (
-            ['git', 'pull', '--rebase', '--autostash', 'origin', branch],
+            ['git', 'pull', '--rebase', 'origin', branch],
            f"Branch '{branch}' had no upstream; pulled from origin/{branch} and set it as the upstream.",
            None,
        )
@@ -1901,33 +1824,6 @@ 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."""
@@ -1943,13 +1839,12 @@ 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, stderr)
+                           fetch_result.returncode,
-            failed = _update_check_failed(_describe_git_failure(stderr))
+                           fetch_result.stderr.decode(errors='replace').strip())
-            _update_check_cache['result'] = failed
+            _update_check_cache['result'] = _safe
            _update_check_cache['ts'] = now
-            return jsonify(failed)
+            return jsonify(_safe)
        local = subprocess.run(
            ['git', 'rev-parse', 'HEAD'],
            capture_output=True, text=True, timeout=5, cwd=cwd,
@@ -1977,8 +1872,7 @@ def check_for_update():
        return jsonify(result)
    except Exception as e:
        logger.warning("check-update failed: %s", e)
-        return jsonify(_update_check_failed(
+        return jsonify(_safe)
            "Could not check for updates; see logs for details."))
@api_v3.route('/system/action', methods=['POST'])
 def execute_system_action():
@@ -2105,11 +1999,6 @@ 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 <branch>" above so the update still works.
            result = subprocess.run(
@@ -2153,7 +2042,6 @@ 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)
@@ -2213,14 +2101,9 @@ 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
@@ -5679,10 +5562,8 @@ def save_plugin_config():
        if schema:
            # Log what we're validating for debugging
            logger.info(f"Validating config for {plugin_id}")
            # Only the shape. plugin_config still holds the submitted secret
            # values at this point -- separate_secrets does not run until
            # below -- so logging it wrote live credentials to the journal.
            logger.info(f"Config keys being validated: {list(plugin_config.keys())}")
            logger.info(f"Full config: {plugin_config}")
            # Get enhanced schema keys (including injected core properties)
            # We need to create an enhanced schema to get the actual allowed keys
@@ -5705,8 +5586,7 @@ def save_plugin_config():
                # Log validation errors for debugging
                logger.error(f"Config validation failed for {plugin_id}")
                logger.error(f"Validation errors: {validation_errors}")
-                # Keys only, for the same reason as above.
+                logger.error(f"Config that failed: {plugin_config}")
                logger.error(f"Config keys that failed: {list(plugin_config.keys())}")
                logger.error(f"Schema properties: {list(enhanced_schema.get('properties', {}).keys())}")
                # Also print to console for immediate visibility
@@ -5737,11 +5617,6 @@ def save_plugin_config():
        # 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()
@@ -5757,9 +5632,7 @@ def save_plugin_config():
        if secrets_config:
            if plugin_id not in current_secrets:
                current_secrets[plugin_id] = {}
-            # See above -- secrets lists must merge element-wise.
+            current_secrets[plugin_id] = deep_merge(current_secrets[plugin_id], secrets_config)
            current_secrets[plugin_id] = merge_secrets(
                current_secrets[plugin_id], secrets_config)
            # Save secrets file
            try:
                api_v3.config_manager.save_raw_file_content('secrets', current_secrets)
@@ -116,35 +116,14 @@ 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(message) {
+window.showRestartPending = function() {
-    try {
+    try { sessionStorage.setItem('ledmatrix-restart-pending', '1'); } catch { /* private browsing */ }
        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) {
        // Without the else-branch a config save inherited whatever wording the
        // previous update left in the DOM: showRestartPending() clears the
        // stored text but used to leave the element itself alone. The default
        // is read back from the server-rendered copy rather than duplicated
        // here, so the template stays the one place that owns the string.
        if (text.dataset.defaultText === undefined) {
            text.dataset.defaultText = text.textContent.trim();
        }
        text.textContent = message || text.dataset.defaultText;
    }
    if (banner) banner.style.display = 'block';
 };
 window.dismissRestartPending = function() {
-    try {
+    try { sessionStorage.removeItem('ledmatrix-restart-pending'); } catch { /* no-op */ }
        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';
 };
@@ -172,9 +151,6 @@ 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 */ }
@@ -4622,17 +4622,15 @@ 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) {
-                    // The endpoint masks what it returns, so this never holds
+                    if (token && token !== 'YOUR_GITHUB_PERSONAL_ACCESS_TOKEN') {
-                    // the real token -- and the field is deliberately left
+                        // Token exists and is valid
-                    // empty rather than filled with the mask, which would be
+                        input.value = token;
-                    // saved verbatim the next time the user pressed Save.
+                        showNotification('GitHub token loaded successfully', 'success');
                    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');
                    }
                }
@@ -413,8 +413,7 @@
            <div class="flex items-center justify-between">
                <div class="flex items-center space-x-3">
                    <i class="fas fa-rotate text-lg"></i>
-                    <span class="text-sm font-medium" aria-live="polite"
+                    <span class="text-sm font-medium" aria-live="polite">
                          id="restart-pending-text">
                        Configuration saved &mdash; restart the display to apply the changes
                    </span>
                </div>
@@ -1108,29 +1107,15 @@
        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;
-                    banner.style.display = '';
+                    document.getElementById('update-banner').style.display = '';
                    try { sessionStorage.setItem('update-sha', data.remote_sha); } catch(e) {}
                } else {
-                    banner.style.display = 'none';
+                    document.getElementById('update-banner').style.display = 'none';
                }
            })
            .catch(function() {});
@@ -1161,13 +1146,6 @@
            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');