From 63b0c50d9f9d341cfb7f4f3e8d1e6e6a2773351c Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Tue, 25 Aug 2026 13:34:08 -0700 Subject: [PATCH 1/2] Fail Closed on Swallowed Host-Setup Precondition-Check Failures Five host-setup/ sites converted a precondition-check failure into an empty or apparently-safe result with || true or 2>/dev/null, then proceeded into a download, package install, sudoers-file write, or release upgrade as if the check had passed. Recurring CodeRabbit finding across PR #951 and PR #952, both times correctly declined there as pre-existing and out of scope; this fixes the class. Four sites now distinguish a check that ran and found nothing from a check that failed to run, and fail closed before the mutation they guard: - install-tools.sh apt_install_displacing(): a failed apt-get -s install simulation now aborts instead of reading as no removals. - install-tools.sh sudoers scan: grep exit 1 (no matches) still proceeds, any higher exit now aborts before writing or deleting sudoers files. - upgrade-host.sh release_preconditions(): a dpkg --audit that fails to run now aborts instead of reading as no half-configured packages. - upgrade-host.sh upgradable_count(): only backs a status report, so a failed apt list now reports "unknown" rather than a misleading 0; nothing downstream mutates on this count. bootstrap.sh resolve_ref() stays lenient, now with an inline comment explaining why: it gates no mutation. download_tree falls back to fetching $REF by name when resolution fails, exactly as it would if resolve_ref did not exist, and it has its own die on a real download failure. Fixes #954 --- host-setup/bootstrap.sh | 2 ++ host-setup/linux/install-tools.sh | 15 ++++++++++++--- host-setup/linux/upgrade-host.sh | 20 ++++++++++++++++---- 3 files changed, 30 insertions(+), 7 deletions(-) diff --git a/host-setup/bootstrap.sh b/host-setup/bootstrap.sh index 84526cda..bf8a1ac9 100755 --- a/host-setup/bootstrap.sh +++ b/host-setup/bootstrap.sh @@ -92,6 +92,8 @@ resolve_ref() { # An unauthenticated request is rate limited per address, so a busy network can lose the lookup while the download itself is fine. # The run continues and says it cannot name its own revision, which is worth a warning rather than a refusal. + # Deliberately lenient (issue #954): unlike the other host-setup precondition checks, this one gates no mutation. + # The download step falls back to fetching $REF by name when RESOLVED is empty, exactly as it would if resolve_ref did not exist, and it has its own die on a real download failure, so a lost lookup here cannot make a download look like it succeeded when it did not. warn "Could not resolve $REF to a commit, so this run cannot be attributed to one" RESOLVED="" return 0 diff --git a/host-setup/linux/install-tools.sh b/host-setup/linux/install-tools.sh index 54fc22c3..bd64bb46 100755 --- a/host-setup/linux/install-tools.sh +++ b/host-setup/linux/install-tools.sh @@ -241,8 +241,13 @@ apt_install_displacing() { return 0 fi + # The simulation is what removals are previewed from, so a simulation that fails to run at all must not read the same as a simulation that ran and found nothing to remove (issue #954). + local sim + sim=$(apt-get -s install "$package" 2>&1) || + die "apt-get -s install $package failed, so removals cannot be previewed before the real install runs: $sim" + local -a removals=() - readarray -t removals < <(apt-get -s install "$package" 2>/dev/null | awk '/^Remv / { print $2 }') + readarray -t removals < <(awk '/^Remv / { print $2 }' <<<"$sim") if [[ ${#removals[@]} -gt 0 ]]; then log " Installing $package removes ${#removals[@]} package(s): ${removals[*]}" log " Their dependencies are left installed, for a later apt autoremove to clean up" @@ -1410,11 +1415,15 @@ configure_sudo_timestamp() { "${SUDO[@]}" cmp -s "$staged" "$SUDOERS_FILE" 2>/dev/null && own_current=true # Another file setting either option is named rather than merged into, since which one wins is the order sudo reads them in and not something this can decide. - local elsewhere + local elsewhere status=0 # A name holding a dot or ending in a tilde is one sudo skips, this run's own staged file included, so a setting in it is an override sudo never reads. elsewhere=$("${SUDO[@]}" grep -rnsE '^[[:space:]]*Defaults.*timestamp_(type|timeout)' \ --exclude='*.*' --exclude='*~' --exclude="${SUDOERS_FILE##*/}" \ - /etc/sudoers /etc/sudoers.d 2>/dev/null) || elsewhere="" + /etc/sudoers /etc/sudoers.d 2>&1) || status=$? + # Grep exits 1 for "no matches", the ordinary and expected case; anything higher means the scan itself did not complete, and this must not write or delete a sudoers file on the strength of a scan that never actually ran (issue #954). + if [[ $status -gt 1 ]]; then + die "Scanning /etc/sudoers and /etc/sudoers.d for other timestamp_type/timestamp_timeout entries failed (grep exit $status): $elsewhere" + fi # Only this user's own entry is ever a delete candidate; a different user's entry, or one with no user named at all, changes something beyond what this run was asked to change, so it is reported and left alone. local -a delete_files=() unsafe_files=() diff --git a/host-setup/linux/upgrade-host.sh b/host-setup/linux/upgrade-host.sh index 95abeb64..4677fdf5 100755 --- a/host-setup/linux/upgrade-host.sh +++ b/host-setup/linux/upgrade-host.sh @@ -174,7 +174,15 @@ refresh_snaps() { } upgradable_count() { - apt list --upgradable 2>/dev/null | grep -c '/' || true + # A failed listing and a listing that genuinely found nothing upgradable both read as "no matches" through grep alone, so the two are told apart here rather than both printing 0 (issue #954). + # This only ever backs a status report, so the answer here is "unknown" rather than a die: nothing downstream mutates on the strength of this count. + local out status=0 + out=$(apt list --upgradable 2>/dev/null) || status=$? + if [[ $status -ne 0 ]]; then + printf 'unknown, apt list --upgradable failed (exit %s)' "$status" + return 0 + fi + printf '%s package(s), against the lists as they stand' "$(grep -c '/' <<<"$out" || true)" } # --- Reboot --- @@ -241,8 +249,12 @@ release_preconditions() { die "Held packages block a release upgrade, unhold them first: $held" fi - local audit - audit=$("${SUDO[@]}" dpkg --audit 2>/dev/null || true) + # A dpkg --audit that fails to run is not the same as one that runs and finds nothing, and only the second one clears the way into a release upgrade (issue #954). + local audit status=0 + audit=$("${SUDO[@]}" dpkg --audit 2>&1) || status=$? + if [[ $status -ne 0 ]]; then + die "dpkg --audit failed to run (exit $status), so half-configured packages cannot be ruled out before a release upgrade: $audit" + fi if [[ -n $audit ]]; then die "dpkg reports half-configured packages, fix them first: $audit" fi @@ -547,7 +559,7 @@ release_summary() { status() { log "Host : $(host_description)" - log "Upgradable: $(upgradable_count) package(s), against the lists as they stand" + log "Upgradable: $(upgradable_count)" log "Release : $(release_summary)" report_reboot From db901a8c2b35e302565cd4ad8a63f0527858cd4c Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Tue, 25 Aug 2026 13:46:29 -0700 Subject: [PATCH 2/2] Separate sudo Failure from grep No-Match in the Sudoers Scan Addresses PR #1007 review findings: - CodeRabbit (real, fixed): the sudoers scan captured the exit status of the outer "${SUDO[@]} grep ..." call and treated any status <= 1 as safe to proceed on. sudo itself can also exit 1 on an authentication or policy failure, before grep ever runs, which the previous fix could not tell apart from grep's own "no matches" exit 1. The no-match remap now happens inside the privileged sh -c script, so the status sudo hands back to the caller only ever means "could not run this at all" or "the real error grep hit", never a genuine no-match collapsing into a false failure signal. - qodo (fixed, 5 findings): dropped the "(issue #954)" citations from the four other new comments, matching this as task history rather than durable code context; the bootstrap.sh resolve_ref() comment block is condensed back to one added line instead of restating the leniency rationale twice. - qodo (fixed, 1 finding): removed the semicolon from the sudoers scan comment, folded into the same edit as the sudo/grep exit-code fix. Verified live: the reworked scan still reads status=0 on a genuine no-match and on a real match, and now reads a real nonzero status when grep itself hits a scan error or when sudo fails before grep ever runs. --- host-setup/bootstrap.sh | 3 +-- host-setup/linux/install-tools.sh | 19 ++++++++++++------- host-setup/linux/upgrade-host.sh | 4 ++-- 3 files changed, 15 insertions(+), 11 deletions(-) diff --git a/host-setup/bootstrap.sh b/host-setup/bootstrap.sh index bf8a1ac9..9c18cdcc 100755 --- a/host-setup/bootstrap.sh +++ b/host-setup/bootstrap.sh @@ -92,8 +92,7 @@ resolve_ref() { # An unauthenticated request is rate limited per address, so a busy network can lose the lookup while the download itself is fine. # The run continues and says it cannot name its own revision, which is worth a warning rather than a refusal. - # Deliberately lenient (issue #954): unlike the other host-setup precondition checks, this one gates no mutation. - # The download step falls back to fetching $REF by name when RESOLVED is empty, exactly as it would if resolve_ref did not exist, and it has its own die on a real download failure, so a lost lookup here cannot make a download look like it succeeded when it did not. + # This fallback is deliberate and gates no mutation: download_tree falls back to fetching $REF by name when RESOLVED is empty, exactly as it would if resolve_ref did not exist, and it has its own die on a real download failure. warn "Could not resolve $REF to a commit, so this run cannot be attributed to one" RESOLVED="" return 0 diff --git a/host-setup/linux/install-tools.sh b/host-setup/linux/install-tools.sh index bd64bb46..0a4ecb0b 100755 --- a/host-setup/linux/install-tools.sh +++ b/host-setup/linux/install-tools.sh @@ -241,7 +241,7 @@ apt_install_displacing() { return 0 fi - # The simulation is what removals are previewed from, so a simulation that fails to run at all must not read the same as a simulation that ran and found nothing to remove (issue #954). + # The simulation is what removals are previewed from, so a simulation that fails to run at all must not read the same as a simulation that ran and found nothing to remove. local sim sim=$(apt-get -s install "$package" 2>&1) || die "apt-get -s install $package failed, so removals cannot be previewed before the real install runs: $sim" @@ -1417,12 +1417,17 @@ configure_sudo_timestamp() { # Another file setting either option is named rather than merged into, since which one wins is the order sudo reads them in and not something this can decide. local elsewhere status=0 # A name holding a dot or ending in a tilde is one sudo skips, this run's own staged file included, so a setting in it is an override sudo never reads. - elsewhere=$("${SUDO[@]}" grep -rnsE '^[[:space:]]*Defaults.*timestamp_(type|timeout)' \ - --exclude='*.*' --exclude='*~' --exclude="${SUDOERS_FILE##*/}" \ - /etc/sudoers /etc/sudoers.d 2>&1) || status=$? - # Grep exits 1 for "no matches", the ordinary and expected case; anything higher means the scan itself did not complete, and this must not write or delete a sudoers file on the strength of a scan that never actually ran (issue #954). - if [[ $status -gt 1 ]]; then - die "Scanning /etc/sudoers and /etc/sudoers.d for other timestamp_type/timestamp_timeout entries failed (grep exit $status): $elsewhere" + # Grep's own "no match" exit (1) is folded to 0 inside the privileged shell, so the status sudo hands back distinguishes only "sudo could not even run this" from "the scan ran", never grep's ordinary no-match case from a sudo failure that also happens to exit 1. + # shellcheck disable=SC2016 # $1/$2 are meant for the inner sh -c script, not this outer shell. + elsewhere=$("${SUDO[@]}" sh -c ' + out=$(grep -rnsE "$1" --exclude="*.*" --exclude="*~" --exclude="$2" /etc/sudoers /etc/sudoers.d 2>&1) + rc=$? + printf %s "$out" + [ "$rc" -eq 1 ] && exit 0 + exit "$rc" + ' _ '^[[:space:]]*Defaults.*timestamp_(type|timeout)' "${SUDOERS_FILE##*/}") || status=$? + if [[ $status -ne 0 ]]; then + die "Scanning /etc/sudoers and /etc/sudoers.d for other timestamp_type/timestamp_timeout entries failed: $elsewhere" fi # Only this user's own entry is ever a delete candidate; a different user's entry, or one with no user named at all, changes something beyond what this run was asked to change, so it is reported and left alone. diff --git a/host-setup/linux/upgrade-host.sh b/host-setup/linux/upgrade-host.sh index 4677fdf5..9792da8f 100755 --- a/host-setup/linux/upgrade-host.sh +++ b/host-setup/linux/upgrade-host.sh @@ -174,7 +174,7 @@ refresh_snaps() { } upgradable_count() { - # A failed listing and a listing that genuinely found nothing upgradable both read as "no matches" through grep alone, so the two are told apart here rather than both printing 0 (issue #954). + # A failed listing and a listing that genuinely found nothing upgradable both read as "no matches" through grep alone, so the two are told apart here rather than both printing 0. # This only ever backs a status report, so the answer here is "unknown" rather than a die: nothing downstream mutates on the strength of this count. local out status=0 out=$(apt list --upgradable 2>/dev/null) || status=$? @@ -249,7 +249,7 @@ release_preconditions() { die "Held packages block a release upgrade, unhold them first: $held" fi - # A dpkg --audit that fails to run is not the same as one that runs and finds nothing, and only the second one clears the way into a release upgrade (issue #954). + # A dpkg --audit that fails to run is not the same as one that runs and finds nothing, and only the second one clears the way into a release upgrade. local audit status=0 audit=$("${SUDO[@]}" dpkg --audit 2>&1) || status=$? if [[ $status -ne 0 ]]; then