From a561a7500a56e7a2812dc7646ebddacc56a933d0 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 11 Sep 2026 02:22:37 +0000 Subject: [PATCH] fix(install): escape sed replacements, use mktemp, and make render failures fatal Address CodeRabbit findings on install_service.sh, install_web_service.sh and install_wifi_monitor.sh: - Values interpolated into each script's sed expression (project root path, username) were not escaped, so a value containing &, \ or the | delimiter would corrupt the rendered systemd unit. Add a shared sed_escape_replacement() helper in the new scripts/install/lib_systemd_render.sh (sourced by all three scripts) and apply it to every sed replacement. - install_service.sh rendered the main and web units to the predictable path /tmp/ledmatrix.service.tmp before installing them -- a symlink/TOCTOU race (CWE-377). Use mktemp for both, with a trap to clean up on exit. - install_service.sh treated a missing template as a mere warning and then checked only whether a unit already existed at the destination before enabling/starting it, so a render failure could silently fall back to enabling a stale, previously-installed unit. Both unit blocks now exit non-zero on a missing template or a failed render. Also rename the ambiguous loop variable `l` to `line` in test/test_systemd_unit_drift.py (Ruff E741); ruff isn't wired into any CI workflow in this repo today, so this isn't currently CI-blocking, but the rename is trivial and correct regardless. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01S3bPMESe2TfrGvbs1ef9c5 --- scripts/install/install_service.sh | 48 ++++++++++++++++++++----- scripts/install/install_web_service.sh | 7 +++- scripts/install/install_wifi_monitor.sh | 6 +++- scripts/install/lib_systemd_render.sh | 27 ++++++++++++++ test/test_systemd_unit_drift.py | 2 +- 5 files changed, 78 insertions(+), 12 deletions(-) create mode 100644 scripts/install/lib_systemd_render.sh diff --git a/scripts/install/install_service.sh b/scripts/install/install_service.sh index 36ea994d..0a7955d8 100755 --- a/scripts/install/install_service.sh +++ b/scripts/install/install_service.sh @@ -16,6 +16,9 @@ USER_HOME=$(eval echo ~$ACTUAL_USER) # Determine the Project Root Directory (parent of scripts/install/) PROJECT_ROOT_DIR=$(cd "$(dirname "$0")/../.." && pwd) +# shellcheck source=scripts/install/lib_systemd_render.sh +source "$PROJECT_ROOT_DIR/scripts/install/lib_systemd_render.sh" + echo "Installing LED Matrix Display Service for user: $ACTUAL_USER" echo "Using home directory: $USER_HOME" echo "Project root directory: $PROJECT_ROOT_DIR" @@ -23,15 +26,29 @@ echo "Project root directory: $PROJECT_ROOT_DIR" # Render the main display unit from its template. The display service runs as # root (it needs GPIO), so __USER__ is always root here -- unlike the web unit # below, which runs as whoever installed it. +# +# A missing template or a failed render is fatal: falling through would leave +# whatever unit already sits at /etc/systemd/system/ledmatrix.service (from a +# previous install) untouched, and the enable/start step below would then +# silently reuse that stale unit instead of the one this run was asked to +# install. if [ -f "$PROJECT_ROOT_DIR/systemd/ledmatrix.service" ]; then - sed "s|__PROJECT_ROOT_DIR__|$PROJECT_ROOT_DIR|g; s|__USER__|root|g" \ - "$PROJECT_ROOT_DIR/systemd/ledmatrix.service" > /tmp/ledmatrix.service.tmp + ESCAPED_PROJECT_ROOT_DIR=$(sed_escape_replacement "$PROJECT_ROOT_DIR") + MAIN_UNIT_TMP=$(mktemp) + trap 'rm -f "$MAIN_UNIT_TMP"' EXIT + if ! sed "s|__PROJECT_ROOT_DIR__|$ESCAPED_PROJECT_ROOT_DIR|g; s|__USER__|root|g" \ + "$PROJECT_ROOT_DIR/systemd/ledmatrix.service" > "$MAIN_UNIT_TMP"; then + echo "ERROR: failed to render ledmatrix.service from its template." >&2 + exit 1 + fi # Copy the service file to the systemd directory - sudo cp /tmp/ledmatrix.service.tmp /etc/systemd/system/ledmatrix.service + sudo cp "$MAIN_UNIT_TMP" /etc/systemd/system/ledmatrix.service # Clean up - rm /tmp/ledmatrix.service.tmp + rm -f "$MAIN_UNIT_TMP" + trap - EXIT else - echo "WARNING: ledmatrix.service template not found at $PROJECT_ROOT_DIR/systemd/ledmatrix.service. Main display service not configured." + echo "ERROR: ledmatrix.service template not found at $PROJECT_ROOT_DIR/systemd/ledmatrix.service." >&2 + exit 1 fi @@ -57,12 +74,25 @@ echo "Installing LEDMatrix Web Interface service (ledmatrix-web.service)..." # src/startup_validator.py compares the installed unit against the template, # every boot warned "re-run install_service.sh" -- and doing so reinstalled the # same stale copy, so the warning could never clear. +# +# As with the main unit above, a missing template or a failed render is +# fatal -- otherwise the enable/start check below would fall back to +# whatever unit (possibly stale) already exists at the destination path. if [ -f "$PROJECT_ROOT_DIR/systemd/ledmatrix-web.service" ]; then - sed "s|__PROJECT_ROOT_DIR__|$PROJECT_ROOT_DIR|g; s|__USER__|$ACTUAL_USER|g" \ - "$PROJECT_ROOT_DIR/systemd/ledmatrix-web.service" \ - | sudo tee /etc/systemd/system/ledmatrix-web.service > /dev/null + ESCAPED_ACTUAL_USER=$(sed_escape_replacement "$ACTUAL_USER") + WEB_UNIT_TMP=$(mktemp) + trap 'rm -f "$WEB_UNIT_TMP"' EXIT + if ! sed "s|__PROJECT_ROOT_DIR__|$ESCAPED_PROJECT_ROOT_DIR|g; s|__USER__|$ESCAPED_ACTUAL_USER|g" \ + "$PROJECT_ROOT_DIR/systemd/ledmatrix-web.service" > "$WEB_UNIT_TMP"; then + echo "ERROR: failed to render ledmatrix-web.service from its template." >&2 + exit 1 + fi + sudo cp "$WEB_UNIT_TMP" /etc/systemd/system/ledmatrix-web.service + rm -f "$WEB_UNIT_TMP" + trap - EXIT else - echo "WARNING: ledmatrix-web.service template not found at $PROJECT_ROOT_DIR/systemd/ledmatrix-web.service. Web interface service not configured." + echo "ERROR: ledmatrix-web.service template not found at $PROJECT_ROOT_DIR/systemd/ledmatrix-web.service." >&2 + exit 1 fi echo "Reloading systemd daemon for web service..." diff --git a/scripts/install/install_web_service.sh b/scripts/install/install_web_service.sh index de8d9af2..1e7a9476 100755 --- a/scripts/install/install_web_service.sh +++ b/scripts/install/install_web_service.sh @@ -17,6 +17,9 @@ fi # Determine the Project Root Directory (parent of scripts/install/) PROJECT_ROOT_DIR=$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd) +# shellcheck source=scripts/install/lib_systemd_render.sh +source "$PROJECT_ROOT_DIR/scripts/install/lib_systemd_render.sh" + echo "Installing for user: $ACTUAL_USER" echo "Project root directory: $PROJECT_ROOT_DIR" @@ -38,7 +41,9 @@ if [ ! -f "$TEMPLATE" ]; then fi echo "Writing service file to /etc/systemd/system/ledmatrix-web.service" -sed "s|__PROJECT_ROOT_DIR__|$PROJECT_ROOT_DIR|g; s|__USER__|$ACTUAL_USER|g" \ +ESCAPED_PROJECT_ROOT_DIR=$(sed_escape_replacement "$PROJECT_ROOT_DIR") +ESCAPED_ACTUAL_USER=$(sed_escape_replacement "$ACTUAL_USER") +sed "s|__PROJECT_ROOT_DIR__|$ESCAPED_PROJECT_ROOT_DIR|g; s|__USER__|$ESCAPED_ACTUAL_USER|g" \ "$TEMPLATE" > /etc/systemd/system/ledmatrix-web.service # Ensure cache directory exists with proper permissions diff --git a/scripts/install/install_wifi_monitor.sh b/scripts/install/install_wifi_monitor.sh index f4e8093e..0c24b0b8 100755 --- a/scripts/install/install_wifi_monitor.sh +++ b/scripts/install/install_wifi_monitor.sh @@ -18,6 +18,9 @@ USER_HOME=$(eval echo ~$ACTUAL_USER) # Determine the Project Root Directory (parent of scripts/install/) PROJECT_ROOT_DIR=$(cd "$(dirname "$0")/../.." && pwd) +# shellcheck source=scripts/install/lib_systemd_render.sh +source "$PROJECT_ROOT_DIR/scripts/install/lib_systemd_render.sh" + echo "Installing LED Matrix WiFi Monitor Service for user: $ACTUAL_USER" echo "Using home directory: $USER_HOME" echo "Project root directory: $PROJECT_ROOT_DIR" @@ -75,7 +78,8 @@ if [ ! -f "$TEMPLATE" ]; then exit 1 fi -SERVICE_FILE_CONTENT=$(sed "s|__PROJECT_ROOT_DIR__|$PROJECT_ROOT_DIR|g; s|__USER__|root|g" "$TEMPLATE") +ESCAPED_PROJECT_ROOT_DIR=$(sed_escape_replacement "$PROJECT_ROOT_DIR") +SERVICE_FILE_CONTENT=$(sed "s|__PROJECT_ROOT_DIR__|$ESCAPED_PROJECT_ROOT_DIR|g; s|__USER__|root|g" "$TEMPLATE") if [ "$EUID" -eq 0 ]; then echo "$SERVICE_FILE_CONTENT" | tee /etc/systemd/system/ledmatrix-wifi-monitor.service > /dev/null diff --git a/scripts/install/lib_systemd_render.sh b/scripts/install/lib_systemd_render.sh new file mode 100644 index 00000000..d07dcec1 --- /dev/null +++ b/scripts/install/lib_systemd_render.sh @@ -0,0 +1,27 @@ +#!/bin/bash +# +# Shared helper for rendering systemd unit templates via sed. +# +# Sourced by install_service.sh, install_web_service.sh and +# install_wifi_monitor.sh so all three escape sed replacement text the same +# way instead of carrying three copies of the same fix. + +# sed_escape_replacement VALUE +# +# Print VALUE escaped for safe use as the replacement side of `sed +# s|pattern|replacement|`. Every one of these scripts builds its sed +# expression by interpolating a shell variable (a path, a username, ...) +# straight into the replacement text. sed gives three characters special +# meaning there: backslash (escape character), & (whole match) and the +# delimiter itself (here `|`). A value containing any of them -- e.g. a +# username or path with an `&`, a literal backslash, or a `|` -- would +# otherwise corrupt the rendered unit file instead of being substituted +# literally. Escape the backslash first so the later escapes aren't +# double-escaped. +sed_escape_replacement() { + local value="$1" + value="${value//\\/\\\\}" + value="${value//&/\\&}" + value="${value//|/\\|}" + printf '%s' "$value" +} diff --git a/test/test_systemd_unit_drift.py b/test/test_systemd_unit_drift.py index 4b7f5562..562e550e 100644 --- a/test/test_systemd_unit_drift.py +++ b/test/test_systemd_unit_drift.py @@ -204,7 +204,7 @@ def test_the_web_unit_still_reports_a_real_changed_directive(validator, tmp_path rendered = _render(template.read_text(encoding="utf-8"), project_root, "hdpi") # Drop RestartSec -- one of the directives the stale heredoc was missing. - stale = "\n".join(l for l in rendered.splitlines() if not l.startswith("RestartSec=")) + stale = "\n".join(line for line in rendered.splitlines() if not line.startswith("RestartSec=")) installed = tmp_path / "ledmatrix-web.service" installed.write_text(stale + "\n", encoding="utf-8")