From 4c389743d7222111464ecabccd73138a04b3f4d4 Mon Sep 17 00:00:00 2001 From: Pyronewbic <36982731+Pyronewbic@users.noreply.github.com> Date: Tue, 21 Jul 2026 15:16:44 +0530 Subject: [PATCH] fix(learn): don't abort the review when the last blocked host isn't a child `sluice learn`'s wildcard-collapse offer built its subdomain list with `[ test ] && echo`, as the last stage of a pipeline inside a command substitution. When the final iteration's test failed - i.e. whenever the last blocked host was not a child of the parent being offered - the `&&` short-circuited, the while loop exited 1, pipefail carried that out of the substitution, and the plain assignment propagated it into errexit. `_learn_review` is called bare, so the whole command died there: the user saw the header, a blank line, and exit 1. No error, no prompt, no indication that anything had gone wrong. It fires whenever a collapse is offered and some other host sorts last, which is not an exotic arrangement - rows are ordered by bytes, so it depends on traffic volume rather than on anything the user controls. Uses `if` instead, so a non-matching line leaves the loop at 0. The test drives the real function sed-extracted from the built bin/sluice, with two mechanical rewrites that leave the loop untouched: the interactive read is pointed at stdin, and the "both streams non-tty" early return is forced off - without the latter, bats' piped stdout takes that return and the loop never runs. It also calls the function BARE, as production does: calling it under `|| echo` puts it in a tested context, which disarms errexit for the entire function body and makes every assertion pass against the unfixed code. Both traps were hit while writing this, hence the harness notes and the extraction guard. Verified as a real tripwire: 4 of the 5 cases fail against the unfixed slice, and the fifth - the mirror case where the last host IS a child - passes in both states, which is the control that proves the harness is not simply broken. --- Makefile | 1 + bin/sluice | 5 +- src/80-learn.sh | 5 +- test/verify-learn-collapse-unit.bats | 92 ++++++++++++++++++++++++++++ 4 files changed, 101 insertions(+), 2 deletions(-) create mode 100644 test/verify-learn-collapse-unit.bats diff --git a/Makefile b/Makefile index 794772f..bf1669f 100644 --- a/Makefile +++ b/Makefile @@ -25,6 +25,7 @@ UNIT_BATS := test/init-detection.bats test/verify-init-quoting.bats test/ver test/verify-bump-knobs-unit.bats test/verify-replay-unit.bats \ test/verify-podman-userns-unit.bats test/verify-build-hash-unit.bats \ test/verify-policy-ceilings-unit.bats test/verify-home-guard-unit.bats \ + test/verify-learn-collapse-unit.bats \ test/verify-drift-render-unit.bats test/verify-awk-data-unit.bats ACCEPT_BATS := test/acceptance.bats test/acceptance-bump.bats test/verify-run-default.bats SECURITY_BATS := $(wildcard test/verify-security-*.bats) diff --git a/bin/sluice b/bin/sluice index f2b9ff2..bb0e30f 100755 --- a/bin/sluice +++ b/bin/sluice @@ -4194,7 +4194,10 @@ _learn_review() { local parents p subs ya; parents="$(printf '%s\n' "$hosts" | while read -r h; do [ -n "$h" ] && parent_of "$h"; done | sort | uniq -c | awk '$1>=2{print $2}')" for p in $parents; do _collapsible "$p" || continue # never offer a wildcard equal to a public suffix (foo.github.io -> .github.io) - subs="$(printf '%s\n' "$hosts" | while read -r h; do [ "$(parent_of "$h")" = "$p" ] && echo "$h"; done)" + # `if`, not `&&`: a failed test on the LAST line leaves the while loop - and so the whole command + # substitution - at status 1, which this plain assignment propagates into errexit. That killed the + # review mid-screen, with no error text, whenever the last blocked host wasn't a child of $p. + subs="$(printf '%s\n' "$hosts" | while read -r h; do if [ "$(parent_of "$h")" = "$p" ]; then echo "$h"; fi; done)" # shellcheck disable=SC2086 printf ' %s subdomains of %s: %s\n' "$(printf '%s\n' "$subs" | awk 'END{print NR}')" "$p" "$(printf '%s ' $subs)" printf ' collapse to .%s (matches all its subdomains)? [y/N] ' "$p" diff --git a/src/80-learn.sh b/src/80-learn.sh index 74fc132..ff3d2f0 100644 --- a/src/80-learn.sh +++ b/src/80-learn.sh @@ -271,7 +271,10 @@ _learn_review() { local parents p subs ya; parents="$(printf '%s\n' "$hosts" | while read -r h; do [ -n "$h" ] && parent_of "$h"; done | sort | uniq -c | awk '$1>=2{print $2}')" for p in $parents; do _collapsible "$p" || continue # never offer a wildcard equal to a public suffix (foo.github.io -> .github.io) - subs="$(printf '%s\n' "$hosts" | while read -r h; do [ "$(parent_of "$h")" = "$p" ] && echo "$h"; done)" + # `if`, not `&&`: a failed test on the LAST line leaves the while loop - and so the whole command + # substitution - at status 1, which this plain assignment propagates into errexit. That killed the + # review mid-screen, with no error text, whenever the last blocked host wasn't a child of $p. + subs="$(printf '%s\n' "$hosts" | while read -r h; do if [ "$(parent_of "$h")" = "$p" ]; then echo "$h"; fi; done)" # shellcheck disable=SC2086 printf ' %s subdomains of %s: %s\n' "$(printf '%s\n' "$subs" | awk 'END{print NR}')" "$p" "$(printf '%s ' $subs)" printf ' collapse to .%s (matches all its subdomains)? [y/N] ' "$p" diff --git a/test/verify-learn-collapse-unit.bats b/test/verify-learn-collapse-unit.bats new file mode 100644 index 0000000..6fe167d --- /dev/null +++ b/test/verify-learn-collapse-unit.bats @@ -0,0 +1,92 @@ +#!/usr/bin/env bats +# _learn_review's wildcard-collapse offer (unit; no engine). The offer loop builds `subs` from a command +# substitution whose last pipeline stage is a `while` loop. When the final iteration's test fails, the +# `&&` short-circuits, the loop exits 1, pipefail carries it out of the substitution, and the plain +# assignment propagates it - so `set -e` kills `sluice learn` mid-screen with NO error text: the user +# sees the header and a blank line, exit 1. Fires whenever a collapse is offered and the last blocked +# host is not a child of that parent. +# +# Harness notes, all three load-bearing: +# 1. The function is sed-extracted from the BUILT bin/sluice (the verify-hostbudget-unit pattern), so +# the loop under test is the real shipped text, not a copy retyped here. +# 2. Two mechanical rewrites, neither touching the loop: the interactive `read` is pointed at stdin +# instead of /dev/tty, and the "both streams non-tty" early return is forced off. Without the +# second, bats' piped stdout takes that return and the loop never executes - a vacuous pass. +# 3. The function is called BARE, exactly as src/80-learn.sh:342 calls it. Calling it as +# `_learn_review ... || echo rc=$?` disarms errexit for the whole function body, and every +# assertion here would pass against the unfixed code. +load test_helper/common + +setup() { + SRC="$(cd "$BATS_TEST_DIRNAME/.." && pwd)/src" + SLUICE_BIN="${SRC%/src}/bin/sluice" + EXTRACT="$BATS_TEST_TMPDIR/learn_review.sh" + sed -n '/^_learn_review()/,/^}/p' "$SLUICE_BIN" \ + | sed -e 's# "$EXTRACT" + # Guard the extraction itself: a sed that silently matched nothing would make every test below + # pass for the wrong reason. + grep -q 'subs="\$(' "$EXTRACT" + [ "$(grep -c '/dev/tty' "$EXTRACT")" = 0 ] +} + +# $1 = rows ("\t\t"), $2 = answers fed to the prompts. +_review() { + local t="$BATS_TEST_TMPDIR/drive.sh" + { + echo 'set -euo pipefail' + echo ". '$SRC/00-prelude.sh'" + echo ". '$SRC/10-egress-helpers.sh'" + echo ". '$EXTRACT'" + echo 'learn_apply() { echo "APPLY-REACHED"; return 0; }' + echo 'reload_allowlist() { return 0; }' + printf '_learn_review "%s" "blocked"\n' "$1" # bare - see harness note 3 + echo 'echo "SURVIVED"' + } > "$t" + printf '%s\n' "$2" | bash "$t" 2>&1 +} + +# bytes-desc, the order cmd_learn feeds in; the trailing row is NOT a child of example.com. +_rows_trailing_stranger() { + local TAB; TAB="$(printf '\t')" + printf 'a.example.com%s1%s300\nb.example.com%s1%s200\nz.other.test%s1%s100' "$TAB" "$TAB" "$TAB" "$TAB" "$TAB" "$TAB" +} + +# POSITIVE CONTROL. Proves the harness reaches the collapse offer at all. Without this, the regression +# below could pass because the function early-returned rather than because the bug is fixed. +@test "learn-collapse: the harness reaches the collapse offer (positive control)" { + run _review "$(_rows_trailing_stranger)" "$(printf 'n\ns\ns\ns')" + assert_output --partial "collapse to .example.com" +} + +# THE REGRESSION. Unfixed, this prints the header and dies: exit 1, no prompt, no error text. +@test "learn-collapse: a trailing non-child host does not abort the review" { + run _review "$(_rows_trailing_stranger)" "$(printf 'n\ns\ns\ns')" + assert_success + assert_output --partial "collapse to .example.com" + assert_output --partial "SURVIVED" +} + +# The mirror case, which always worked - pinned so a fix cannot regress it the other way. +@test "learn-collapse: a trailing child host still offers the collapse" { + local TAB; TAB="$(printf '\t')" + run _review "$(printf 'z.other.test%s1%s300\na.example.com%s1%s200\nb.example.com%s1%s100' "$TAB" "$TAB" "$TAB" "$TAB" "$TAB" "$TAB")" "$(printf 's\nn\ns\ns')" + assert_success + assert_output --partial "collapse to .example.com" + assert_output --partial "SURVIVED" +} + +# subs feeds both the offer's count and the `handled` set - a truncated list would under-report which +# hosts a collapse covers, and silently leave one blocked after the user accepted. +@test "learn-collapse: the offer counts both subdomains, not one" { + run _review "$(_rows_trailing_stranger)" "$(printf 'n\ns\ns\ns')" + assert_success + assert_output --partial "2 subdomains of example.com" +} + +@test "learn-collapse: accepting the collapse consumes both subdomains" { + run _review "$(_rows_trailing_stranger)" "$(printf 'y\ns')" + assert_success + assert_output --partial "collapse to .example.com" + assert_output --partial "SURVIVED" +}