diff --git a/migrations/1788025225.sh b/migrations/1788025225.sh index ce95b9bc..b131d046 100644 --- a/migrations/1788025225.sh +++ b/migrations/1788025225.sh @@ -67,6 +67,12 @@ active_lines() { emit_logical "$logical$line" logical="" done + + # A file whose last line ends in a backslash still carries a live directive for + # systemd: `systemd-analyze verify` resolves an ExecStop= written that way. + # udev ignores the dangling line and sudo rejects the file outright, so emitting + # it costs those two nothing. + emit_logical "$logical" } # One logical line, whitespace collapsed so a reformatted copy still compares @@ -195,6 +201,7 @@ plymouth_unit_runs_from_home() { local home_pattern="^(/home/[^/]+|/root)/\\.local/share/omarchy/bin/$binary\$" local line word local -a words + local matched=1 while IFS= read -r line; do if [[ ! $line =~ $exec_stop_pattern ]]; then @@ -203,6 +210,11 @@ plymouth_unit_runs_from_home() { read -ra words <<<"${BASH_REMATCH[1]}" if (( ! ${#words[@]} )); then + # An empty assignment resets the list, so nothing named before it still + # runs. `systemd-analyze verify` reports the missing command for a unit + # with one ExecStop=, and reports nothing once a bare ExecStop= follows it. + # An administrator who neutralised the unit this way is left alone. + matched=1 continue fi @@ -214,11 +226,13 @@ plymouth_unit_runs_from_home() { done if [[ $word =~ $home_pattern || $word == "$HOME/.local/share/omarchy/bin/$binary" ]]; then - return 0 + matched=0 + else + matched=1 fi done < <(active_lines systemd) - return 1 + return $matched } # /etc/sudoers.d is 0750 root:root as shipped, and omarchy-migrate runs as the diff --git a/test/shell.d/fixtures/privileged-heredoc/hop-twice-home-path.sh b/test/shell.d/fixtures/privileged-heredoc/hop-twice-home-path.sh old mode 100755 new mode 100644 diff --git a/test/shell.d/fixtures/privileged-heredoc/hop-variable-home-path.sh b/test/shell.d/fixtures/privileged-heredoc/hop-variable-home-path.sh old mode 100755 new mode 100644 diff --git a/test/shell.d/fixtures/privileged-heredoc/shadowed-assignment-home-path.sh b/test/shell.d/fixtures/privileged-heredoc/shadowed-assignment-home-path.sh old mode 100755 new mode 100644 diff --git a/test/shell.d/legacy-power-udev-rules-migration-test.sh b/test/shell.d/legacy-power-udev-rules-migration-test.sh index 8a092abe..6751edda 100755 --- a/test/shell.d/legacy-power-udev-rules-migration-test.sh +++ b/test/shell.d/legacy-power-udev-rules-migration-test.sh @@ -213,14 +213,16 @@ run_migration fail "migration matches the binary the filename promises, not any home path" pass "migration matches the binary the filename promises, not any home path" -# udev resumes a continuation across a comment: `udevadm verify` on -# 'SUBSYSTEM=="power_supply" \' + "# c" + ', RUN+="..."' reports its style warning -# on line 1, so those three lines are one rule and the rule is live. +# udev resumes a continuation across a comment: `udevadm verify` reports its +# complaint on line 1 for a rule split this way, so the three lines are one live +# rule. The split falls inside the RUN+= value on purpose -- with the whole +# RUN+= below the comment the assertion passes even against an implementation +# that throws the pending half away, which is the shape this guards against. reset_machine cat >"$power_rule" <<'RULE' -SUBSYSTEM=="power_supply", ATTR{type}=="Mains" \ +SUBSYSTEM=="power_supply", ATTR{type}=="Mains", RUN+="/usr/bin/systemd-run --no-block --unit=omarchy-power-profile \ # split for readability -, RUN+="/home/someuser/.local/share/omarchy/bin/omarchy-powerprofiles-set" +/home/someuser/.local/share/omarchy/bin/omarchy-powerprofiles-set" RULE run_migration diff --git a/test/shell.d/privileged-heredoc-test.sh b/test/shell.d/privileged-heredoc-test.sh index 26a894e3..c62d3f45 100755 --- a/test/shell.d/privileged-heredoc-test.sh +++ b/test/shell.d/privileged-heredoc-test.sh @@ -176,16 +176,17 @@ declare -A VARS_TAINTED=() # isolation would miss in whichever direction it picked. collect_vars() { local -n source_lines="$1" - local line name value + local line name value append VARS=() VARS_TAINTED=() for line in "${source_lines[@]}"; do [[ $line =~ ^[[:space:]]*# ]] && continue - [[ $line =~ ^[[:space:]]*(local|declare|export|readonly|typeset)?[[:space:]]*([A-Za-z_][A-Za-z0-9_]*)=(.*)$ ]] || continue + [[ $line =~ ^[[:space:]]*(local|declare|export|readonly|typeset)?[[:space:]]*([A-Za-z_][A-Za-z0-9_]*)(\+?)=(.*)$ ]] || continue name=${BASH_REMATCH[2]} - value=${BASH_REMATCH[3]} + append=${BASH_REMATCH[3]} + value=${BASH_REMATCH[4]} value=${value%%[[:space:]]#*} value=${value%[[:space:]]} if [[ $value == \"*\" || $value == \'*\' ]]; then @@ -193,6 +194,11 @@ collect_vars() { fi mentions_user_writable_root "$value" && VARS_TAINTED["$name"]=1 + + # An append never wins the value -- an array grown across a file resolves to + # nothing useful -- but it does carry the taint, or a name could reach a user + # root through += and never be judged on it. + [[ -n $append ]] && continue [[ -v VARS[$name] ]] || VARS["$name"]=$value done } @@ -457,9 +463,8 @@ privileged_destination() { fi done < <(command_destinations "$line") - # An elevated write whose destination cannot be resolved counts as - # privileged: sudo tee is not aimed at a user's own dotfile, and assuming - # otherwise is how this bug class survived six reviews. + # An elevated write whose destination cannot be resolved counts as privileged: + # sudo tee is not aimed at a user's own dotfile. if ((elevated == 0)) && ((${#unresolved[@]} > 0)); then printf '%s' "${unresolved[0]} (unresolved destination of an elevated write)" return 0 diff --git a/test/shell.d/retired-installer-artifacts-migration-test.sh b/test/shell.d/retired-installer-artifacts-migration-test.sh index d626fc33..d89ab09c 100755 --- a/test/shell.d/retired-installer-artifacts-migration-test.sh +++ b/test/shell.d/retired-installer-artifacts-migration-test.sh @@ -197,21 +197,25 @@ run_migration fail "migration keeps a same-named file that never cleaned itself up" pass "migration keeps a same-named file that never cleaned itself up" -# A rule continued onto the next line is one logical line, and a comment that is -# continued stays a comment for the whole of it. +# A spec continued onto the next line is one logical line, and a comment's own +# trailing backslash swallows nothing: `visudo -cf` on "# note \" plus a bogus +# token reports the error on line 2, so the spec below a commented line is live. +# The hand-written spec sits under the comment on purpose -- above it, the file +# is kept under either reading and the assertion cannot fail. reset_machine cat >"$first_run" <<'EOF' # Retired, keeping the old grant here for reference: \ -installer ALL=(ALL) NOPASSWD: /usr/bin/systemctl +installer ALL=(ALL) NOPASSWD: /usr/local/bin/our-own-deploy-script Cmnd_Alias FIRST_RUN_CLEANUP = /bin/rm -f /etc/sudoers.d/first-run installer ALL=(ALL) NOPASSWD: \ - /usr/local/bin/our-own-deploy-script + /usr/bin/systemctl +installer ALL=(ALL) NOPASSWD: FIRST_RUN_CLEANUP EOF run_migration [[ -e $first_run ]] || - fail "migration reads a continued line as one rule and a continued comment as comment" -pass "migration reads a continued line as one rule and a continued comment as comment" + fail "migration keeps a spec left live under a commented continuation" +pass "migration keeps a spec left live under a commented continuation" reset_machine printf 'installer ALL=(ALL) NOPASSWD: %s/.local/bin/tsui\n' "$home_dir" >"$tsui" @@ -454,3 +458,49 @@ run_migration [[ ! -e $plymouth_unit ]] || fail "migration removes a unit whose ExecStop continues across a comment" "$(cat "$plymouth_unit")" pass "migration removes a unit whose ExecStop continues across a comment" + +# An empty ExecStop= resets the list: `systemd-analyze verify` reports the missing +# command for a unit with one ExecStop=, and reports nothing once a bare +# ExecStop= follows it. An administrator who neutralised the unit that way runs +# nothing at shutdown and keeps their file. +reset_machine +cat >"$plymouth_unit" <<'EOF' +[Service] +Type=oneshot +ExecStart=/usr/bin/true +ExecStop=/home/installer/.local/share/omarchy/bin/omarchy-plymouth-shutdown-sync +ExecStop= +EOF +before=$(cat "$plymouth_unit") +run_migration + +[[ -e $plymouth_unit ]] || + fail "migration keeps a unit whose ExecStop list was reset to empty" +[[ $(cat "$plymouth_unit") == "$before" ]] || + fail "migration leaves that unit byte for byte" +pass "migration keeps a unit whose ExecStop list was reset to empty" + +# A reset followed by a fresh home ExecStop= is live again. +reset_machine +cat >"$plymouth_unit" <<'EOF' +[Service] +Type=oneshot +ExecStart=/usr/bin/true +ExecStop= +ExecStop=/home/installer/.local/share/omarchy/bin/omarchy-plymouth-shutdown-sync +EOF +run_migration + +[[ ! -e $plymouth_unit ]] || + fail "migration removes a unit whose ExecStop is set again after a reset" +pass "migration removes a unit whose ExecStop is set again after a reset" + +# systemd honours a directive whose line ends the file mid-continuation: +# `systemd-analyze verify` resolves an ExecStop= written that way. +reset_machine +printf '[Service]\nType=oneshot\nExecStart=/usr/bin/true\nExecStop=/home/installer/.local/share/omarchy/bin/omarchy-plymouth-shutdown-sync \\\n' >"$plymouth_unit" +run_migration + +[[ ! -e $plymouth_unit ]] || + fail "migration removes a unit whose last line ends mid-continuation" +pass "migration removes a unit whose last line ends mid-continuation"