mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-10-17 20:39:13 +00:00
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S3bPMESe2TfrGvbs1ef9c5
This commit is contained in:
Claude
@@ -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..."
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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"
|
||||
}
|
||||
@@ -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")
|
||||
|
||||
|
||||
Reference in New Issue
Block a user