Close privileged cleanup review gaps

migrations/1787946619.sh

@@ -1,6 +1,8 @@
 echo "Remove Omarchy 3 power udev rules that run a command out of a user home"
 
 rules_dir="${OMARCHY_UDEV_RULES_DIR:-/etc/udev/rules.d}"
+reload_needed_marker="${OMARCHY_UDEV_RELOAD_NEEDED_MARKER:-/var/lib/omarchy/migrations/1787946619-udev-reload-needed}"
+udev_control="${OMARCHY_UDEV_CONTROL:-/run/udev/control}"
 
 as_root() {
   if (( EUID == 0 )); then
@@ -78,19 +80,41 @@ rule_runs_from_home() {
   return 1
 }
 
+finish_pending_reload() {
+  # With no control socket there is no running udevd holding the deleted rule;
+  # the next daemon start reads the directory from disk. If a daemon is running,
+  # a failed reload must keep this migration pending so the in-memory root rule
+  # cannot outlive the per-user completion marker.
+  if [[ -e $udev_control ]] && ! as_root udevadm control --reload 2>/dev/null; then
+    echo "Could not reload udev after removing a vulnerable legacy rule. Ask an administrator to run omarchy-migrate." >&2
+    exit 1
+  fi
+
+  if ! as_root rm -f "$reload_needed_marker"; then
+    echo "Could not finish the legacy udev-rule repair. Ask an administrator to run omarchy-migrate." >&2
+    exit 1
+  fi
+}
+
+# Deleting the file and reloading the daemon are one repair. A prior run may
+# have removed the file and then failed before udevd accepted the new ruleset.
+if [[ -e $reload_needed_marker ]]; then
+  finish_pending_reload
+fi
+
 for legacy_rule in "99-power-profile.rules:omarchy-powerprofiles-set" "99-wifi-powersave.rules:omarchy-wifi-powersave"; do
   rule_file="$rules_dir/${legacy_rule%%:*}"
 
   if [[ -f $rule_file ]] && rule_runs_from_home "$rule_file" "${legacy_rule##*:}"; then
+    if ! as_root install -Dm644 /dev/null "$reload_needed_marker"; then
+      echo "Administrator privileges are required to remove the vulnerable legacy udev rule. Ask an administrator to run omarchy-migrate." >&2
+      exit 1
+    fi
     if ! as_root rm -f "$rule_file"; then
       echo "Administrator privileges are required to remove the vulnerable legacy udev rule. Ask an administrator to run omarchy-migrate." >&2
       exit 1
     fi
 
-    # Reload after each removal, not after the whole loop. If removing a later
-    # rule fails, udevd must not keep running one this migration already deleted.
-    # Best effort the way install/post-install/udev.sh is: a machine with no
-    # udevd to talk to has had the file removed, and the next boot reads fresh.
-    as_root udevadm control --reload 2>/dev/null || true
+    finish_pending_reload
   fi
 done

migrations/1788025225.sh

@@ -116,8 +116,8 @@ sudoers_hash_is_active() {
 # hand-written line anywhere in the file and it is not ours to delete.
 first_run_sudoers_is_generated() {
   local spec_pattern='^([^[:space:]]+) ALL=\(ALL\) NOPASSWD: (.+)$'
-  local marker_pattern='^/bin/rm -f /home/[^/]+/\.local/state/omarchy/first-run\.mode$'
-  local line user command generated_user=""
+  local marker_pattern='^/bin/rm -f /home/([^/]+)/\.local/state/omarchy/first-run\.mode$'
+  local line user command marker_user generated_user=""
   local seen_any=0 seen_marker=0 seen_spec=0
 
   while IFS= read -r line; do
@@ -161,6 +161,8 @@ first_run_sudoers_is_generated() {
     esac
 
     if [[ $command =~ $marker_pattern ]]; then
+      marker_user=${BASH_REMATCH[1]}
+      [[ $marker_user == "$generated_user" ]] || return 1
       seen_marker=1
       continue
     fi

test/shell.d/fixtures/privileged-heredoc/nested-parameter-default.sh

@@ -0,0 +1,6 @@
+#!/bin/bash
+
+# omarchy:heredoc-expands paths=none -- review regression fixture
+sudo tee /etc/omarchy/review.conf >/dev/null <<EOF
+ExecStart=${target:-$HOME/.local/bin/payload}
+EOF

test/shell.d/fixtures/privileged-heredoc/route-install-hop-literal.sh

@@ -0,0 +1,7 @@
+#!/bin/bash
+
+cat <<EOF >/tmp/omarchy-review-unit
+[Service]
+ExecStart=$HOME/.local/bin/payload
+EOF
+sudo install -m 644 /tmp/omarchy-review-unit /etc/systemd/system/review.service

test/shell.d/fixtures/privileged-heredoc/route-prebody-escaped-pipeline.sh

@@ -0,0 +1,6 @@
+#!/bin/bash
+
+cat <<EOF | \
+  sudo tee /etc/omarchy/review.conf
+ExecStart=$HOME/.local/bin/payload
+EOF

test/shell.d/legacy-power-udev-rules-migration-test.sh

@@ -25,6 +25,10 @@ cat >"$test_dir/bin/udevadm" <<'STUB'
 #!/bin/bash
 
 printf 'udevadm %s\n' "$*" >>"$CALLS"
+if [[ -n ${FAIL_UDEV_RELOAD_ONCE_MARKER:-} && ! -e $FAIL_UDEV_RELOAD_ONCE_MARKER ]]; then
+  touch "$FAIL_UDEV_RELOAD_ONCE_MARKER"
+  exit 1
+fi
 STUB
 
 chmod +x "$test_dir/bin/"*
@@ -44,10 +48,13 @@ rules_dir="$test_dir/rules.d"
 home_dir="$test_dir/home"
 power_rule="$rules_dir/99-power-profile.rules"
 wifi_rule="$rules_dir/99-wifi-powersave.rules"
+reload_needed_marker="$test_dir/reload-needed"
+udev_control="$test_dir/udev-control"
 
 reset_machine() {
-  rm -rf "$rules_dir" "$home_dir"
+  rm -rf "$rules_dir" "$home_dir" "$reload_needed_marker" "$udev_control"
   mkdir -p "$rules_dir" "$home_dir"
+  touch "$udev_control"
 }
 
 run_migration() {
@@ -55,6 +62,8 @@ run_migration() {
 
   HOME="$home_dir" \
     OMARCHY_UDEV_RULES_DIR="$rules_dir" \
+    OMARCHY_UDEV_RELOAD_NEEDED_MARKER="$reload_needed_marker" \
+    OMARCHY_UDEV_CONTROL="$udev_control" \
     PATH="$test_dir/bin:$PATH" \
     bash -euo pipefail "$migration" >/dev/null
 }
@@ -118,6 +127,45 @@ run_migration
   fail "migration reloads udev after each removal" "$(cat "$CALLS")"
 pass "migration removes both legacy rules and reloads after each one"
 
+# Removing the file and reloading the running daemon are one repair. If reload
+# fails, the durable marker must keep the migration pending even though the rule
+# has already disappeared from disk; a retry finishes that half before exiting.
+reset_machine
+write_vulnerable_wifi_rule
+reload_failure_seen="$test_dir/reload-failure-seen"
+rm -f "$reload_failure_seen"
+
+set +e
+FAIL_UDEV_RELOAD_ONCE_MARKER="$reload_failure_seen" run_migration
+reload_status=$?
+set -e
+
+(( reload_status != 0 )) || fail "migration fails when a running udevd cannot reload"
+[[ ! -e $wifi_rule && -e $reload_needed_marker ]] ||
+  fail "migration records a deleted rule whose daemon reload is still pending"
+pass "migration keeps a failed udev reload pending"
+
+run_migration
+
+[[ ! -e $reload_needed_marker ]] || fail "migration clears the reload marker after a successful retry"
+(( $(reload_count) == 1 )) ||
+  fail "migration retries the pending udev reload" "$(cat "$CALLS")"
+pass "migration retries and completes a previously failed udev reload"
+
+# A chroot or stopped daemon has no in-memory ruleset to update. An absent udev
+# control socket is therefore a completed removal, not a permanent migration
+# failure waiting for a daemon that is not running.
+reset_machine
+rm -f "$udev_control"
+write_vulnerable_wifi_rule
+run_migration
+
+[[ ! -e $wifi_rule && ! -e $reload_needed_marker ]] ||
+  fail "migration completes the disk-only repair when udevd is not running"
+(( $(reload_count) == 0 )) ||
+  fail "migration does not contact an absent udevd" "$(cat "$CALLS")"
+pass "migration permits environments with no running udev daemon"
+
 # The second run is what every other account on the machine does, and what a
 # user gets from running omarchy-migrate again.
 run_migration
@@ -219,6 +267,8 @@ write_vulnerable_wifi_rule
 set +e
 HOME="$home_dir" \
   OMARCHY_UDEV_RULES_DIR="$rules_dir" \
+  OMARCHY_UDEV_RELOAD_NEEDED_MARKER="$reload_needed_marker" \
+  OMARCHY_UDEV_CONTROL="$udev_control" \
   PATH="$test_dir/failing-bin:$PATH" \
   bash -euo pipefail "$migration" >"$test_dir/elevation-failure.out" 2>&1
 failure_status=$?
@@ -250,6 +300,8 @@ chmod +x "$test_dir/failing-bin/sudo"
 set +e
 HOME="$home_dir" \
   OMARCHY_UDEV_RULES_DIR="$rules_dir" \
+  OMARCHY_UDEV_RELOAD_NEEDED_MARKER="$reload_needed_marker" \
+  OMARCHY_UDEV_CONTROL="$udev_control" \
   PATH="$test_dir/failing-bin:$test_dir/bin:$PATH" \
   bash -euo pipefail "$migration" >"$test_dir/partial-failure.out" 2>&1
 partial_status=$?
@@ -257,6 +309,7 @@ set -e
 
 (( partial_status != 0 )) || fail "migration fails when the second rule cannot be removed"
 [[ ! -e $power_rule && -e $wifi_rule ]] || fail "migration preserves the expected partial-removal state"
+[[ -e $reload_needed_marker ]] || fail "migration records the second rule removal as still pending"
 (( $(reload_count) == 1 )) ||
   fail "migration reloads udev before a later removal failure" "$(cat "$CALLS")"
 pass "a later removal failure cannot leave an already-deleted rule loaded"

test/shell.d/privileged-heredoc-test.sh

@@ -121,8 +121,8 @@ strip_escapes() {
 # path. Because both halves come from one pass over one pattern, the Nth \001
 # is the Nth name, so a token can be judged against the right variable.
 mask_and_names() {
-  local text="$1" masked="" body name guard=0
-  local -a names=()
+  local text="$1" masked="" body inner tail name guard=0 nested_masked
+  local -a names=() nested_scan=() nested_names=()
 
   # Normalize backtick substitution into $( ) so one pattern covers both.
   while ((guard++ < 64)) && [[ $text =~ ^([^\`]*)\`([^\`]*)\`(.*)$ ]]; do
@@ -135,20 +135,35 @@ mask_and_names() {
     masked+="${BASH_REMATCH[1]}"$'\001'
     body=${BASH_REMATCH[2]}
     text=${BASH_REMATCH[4]}
+    nested_masked=""
+    nested_names=()
 
     if [[ $body == \(* ]]; then
       name=$COMMAND_SUBSTITUTION
     elif [[ $body == \{* ]]; then
-      body=${body:1:${#body}-2}
+      inner=${body:1:${#body}-2}
       # ${name}, ${name:-default}, ${name//a/b}, ${#name}, ${!name} all start
       # with the name once the decorations are stripped.
-      body=${body#[\#!]}
-      if [[ $body =~ ^([A-Za-z_][A-Za-z0-9_]*) ]]; then
+      inner=${inner#[\#!]}
+      if [[ $inner =~ ^([A-Za-z_][A-Za-z0-9_]*) ]]; then
         name=${BASH_REMATCH[1]}
-      elif [[ $body =~ ^[0-9@*#?$!-]$ ]]; then
+        tail=${inner#"$name"}
+      elif [[ $inner =~ ^[0-9@*#?$!-] ]]; then
         name="shell-parameter"
+        tail=${inner:1}
       else
         name=$COMMAND_SUBSTITUTION
+        tail=$inner
+      fi
+
+      # The shell expands the operator payload too. Keep it as a synthetic
+      # adjacent token so its placeholders stay aligned with their names while
+      # the outer expansion remains independently classifiable. Without this,
+      # ${target:-$HOME/path} is consumed as only `target` and hides HOME.
+      if [[ $tail =~ $EXPANSION_RE || $tail == *'`'* ]]; then
+        mapfile -t nested_scan < <(mask_and_names "$tail")
+        nested_masked=${nested_scan[0]}
+        nested_names=("${nested_scan[@]:1}")
       fi
     else
       name=${body%%\[*}
@@ -156,6 +171,10 @@ mask_and_names() {
     fi
 
     names+=("$name")
+    if ((${#nested_names[@]} > 0)); then
+      masked+=" $nested_masked"
+      names+=("${nested_names[@]}")
+    fi
   done
 
   printf '%s\n' "$masked$text"
@@ -469,25 +488,24 @@ privileged_destination() {
       unresolved+=("$dest")
     fi
 
-    # One hop: a later copy of this same expression into a root-owned path.
-    if [[ $dest == *'$'* ]]; then
-      follow=$start_index
-      while ((follow < ${#scan_lines[@]})); do
-        hop=${scan_lines[follow]}
-        follow=$((follow + 1))
-        [[ $hop =~ (^|[[:space:]])(install|cp|mv)([[:space:]]|$) ]] || continue
-        line_carries_destination "$hop" "$dest" || continue
-        while IFS= read -r hop_dest; do
-          [[ $hop_dest == $'\002elevated' ]] && continue
-          [[ $hop_dest == "$dest" ]] && continue
-          hop_dest=$(resolve_value "$hop_dest")
-          if starts_with_privileged_prefix "$hop_dest"; then
-            printf '%s' "$hop_dest"
-            return 0
-          fi
-        done < <(command_destinations "$hop")
-      done
-    fi
+    # One hop: a later copy of this same destination into a root-owned path.
+    # Literal scratch files need tracing just as much as variable destinations.
+    follow=$start_index
+    while ((follow < ${#scan_lines[@]})); do
+      hop=${scan_lines[follow]}
+      follow=$((follow + 1))
+      [[ $hop =~ (^|[[:space:]])(install|cp|mv)([[:space:]]|$) ]] || continue
+      line_carries_destination "$hop" "$dest" || continue
+      while IFS= read -r hop_dest; do
+        [[ $hop_dest == $'\002elevated' ]] && continue
+        [[ $hop_dest == "$dest" ]] && continue
+        hop_dest=$(resolve_value "$hop_dest")
+        if starts_with_privileged_prefix "$hop_dest"; then
+          printf '%s' "$hop_dest"
+          return 0
+        fi
+      done < <(command_destinations "$hop")
+    done
   done < <(command_destinations "$line")
 
   # An elevated write whose destination cannot be resolved counts as privileged:
@@ -572,7 +590,7 @@ inside_same_line_arithmetic() {
 scan_file() {
   local file="$1" display="${2:-$1}"
   local -a lines=()
-  local index lineno line scan rest raw operator match prefix guard slot delim candidate candidate_delim body_start
+  local index lineno line command scan rest raw operator match prefix guard slot delim candidate candidate_delim body_start
   local body_text unescaped destination destination_command body_line masked_line token name
   local declared_paths annotation look shown_paths shown_plain count next slots terminated
   local hd_re='(<<-?)[[:space:]]*("[A-Za-z_][A-Za-z0-9_]*"|'"'"'[A-Za-z_][A-Za-z0-9_]*'"'"'|[A-Za-z_][A-Za-z0-9_]*)'
@@ -588,9 +606,21 @@ scan_file() {
 
     [[ $line =~ ^[[:space:]]*# ]] && continue
 
+    # A backslash-escaped newline is removed before Bash parses the command, so
+    # a pipeline consumer can appear on the next physical line before heredoc
+    # body collection begins: `cat <<EOF | \\` then `sudo tee /etc/file`.
+    # Join those physical lines first. A bare newline after `|` instead starts
+    # the heredoc body and is handled after the terminator below.
+    command=$line
+    while [[ $command == *\\ ]] && ((index < ${#lines[@]})); do
+      command=${command%\\}
+      command+=" ${lines[index]}"
+      index=$((index + 1))
+    done
+
     # Herestrings are not heredocs. Blanking them keeps <<<"$x" from reading as
     # a heredoc while preserving every other offset on the line.
-    scan=${line//<<</   }
+    scan=${command//<<</   }
     [[ $scan == *"<<"* ]] || continue
 
     # Collect this line's heredoc delimiters in order. Quoted ones are safe by
@@ -661,7 +691,7 @@ scan_file() {
       unescaped=$(strip_escapes "$body_text")
       [[ $unescaped =~ $EXPANSION_RE || $unescaped == *'`'* ]] || continue
 
-      destination_command=$(continued_heredoc_command "$line" "$index" lines)
+      destination_command=$(continued_heredoc_command "$command" "$index" lines)
       destination=$(privileged_destination "$destination_command" "$index" lines) || continue
 
       # Sort the expansions into the ones that bake a path into the file and
@@ -880,12 +910,16 @@ fixture_flags route-variable-path.sh \
   "flags an elevated write whose destination is a variable resolving under /etc"
 fixture_flags route-install-hop.sh \
   "flags a scratch file that install(1) later copies into /usr"
+fixture_flags route-install-hop-literal.sh \
+  "flags a literal scratch file that install(1) later copies into /etc"
 fixture_flags route-install-hop-braced.sh \
   "flags a scratch-file hop whose variable uses braces at the privileged copy"
 fixture_flags route-install-hop-alias.sh \
   "flags a scratch-file hop carried through an alias variable"
 fixture_flags route-continued-pipeline.sh \
   "flags a privileged pipeline command continued after the heredoc terminator"
+fixture_flags route-prebody-escaped-pipeline.sh \
+  "flags an escaped-line pipeline consumer before the heredoc body"
 fixture_flags route-dash-delimiter.sh "flags an indented <<- heredoc"
 fixture_flags route-append-redirect.sh "flags an append redirect into /etc"
 fixture_flags arithmetic-left-shift-before-heredoc.sh \
@@ -894,6 +928,9 @@ fixture_flags arithmetic-left-shift-before-heredoc.sh \
 fixture_flags plain-heredoc-indented-pseudo-delimiter.sh \
   "an indented delimiter does not terminate a plain heredoc" \
   "path-shaped expansions: HOME"
+fixture_flags nested-parameter-default.sh \
+  "a nested parameter default cannot hide a baked home path" \
+  "path-shaped expansions are HOME"
 
 mapfile -t dd_destinations < <(command_destinations \
   'sudo dd if=/tmp/input bs=4M status=none of=/etc/omarchy/image')

test/shell.d/retired-installer-artifacts-migration-test.sh

@@ -220,6 +220,21 @@ run_migration
   fail "migration leaves a generated file extended for another user byte for byte"
 pass "migration does not delete an administrator grant that uses a generated command"
 
+# The oldest installer variant expanded the same $USER into the sudoers account
+# and the /home/<user>/ cleanup path. A different account in the path proves the
+# line was edited or hand-written and makes the whole file administrator-owned.
+reset_machine
+cat >"$first_run" <<'EOF'
+alice ALL=(ALL) NOPASSWD: /bin/rm -f /home/bob/.local/state/omarchy/first-run.mode
+EOF
+before=$(cat "$first_run")
+run_migration
+
+[[ -e $first_run ]] || fail "migration keeps a first-run cleanup path for another account"
+[[ $(cat "$first_run") == "$before" ]] ||
+  fail "migration leaves the cross-account first-run file byte for byte"
+pass "migration requires the cleanup path account to match the granted account"
+
 # Nothing in this file ties it to Omarchy's first run: no self-cleanup line.
 reset_machine
 cat >"$first_run" <<'EOF'