Repository navigation
test(e2e): a negative assertion passes when its own matcher fails, and every security check in the suites relies on one #3700
Description
Two bats suites carry a hand-copied assert_file_lacks_pattern, and every safety claim these suites make about their artifacts goes through it. Five of its sixteen call sites pass a pattern carrying regex metacharacters, and all five assert that something must not appear: tls.crt / tls.key / data: in a rendered manifest, secret: and secretName:, the verbs delete / exec / apply in a command log, certificate and talosconfig in output that must not carry them. Those are the assertions with the least tolerance for passing by accident, and they are exactly the ones this defect makes vacuous.
The helper reads:
if awk -v pattern="${pattern}" '$0 ~ pattern { found = 1 } END { exit found ? 0 : 1 }' "${file}"; then
printf 'expected %s not to match: %s\n' "${file}" "${pattern}" >&2
return 1
fiThe assertion holds when awk exits non-zero, and awk exits non-zero for three different reasons: no line matched, the regex could not be parsed, and the file could not be read. Only the first is the fact being asserted. So "this artifact contains no secret" is satisfied by a regex awk choked on -- and by the artifact not existing at all. A missing bundle reads as a clean one.
The direction matters. A positive assertion whose matcher breaks fails loudly and gets fixed on the next run. A negative one goes green and stays green, and nothing distinguishes it from a passing check.
That it can sit unnoticed is not hypothetical. A call site in an open PR passes '\-k 5 0 ', which makes awk print warning: escape sequence \- treated as plain - on every run of that test. The match still worked, so nothing failed, and the warning goes to a test's stderr that nobody reads while the suite is green; it was found by a reviewer reading the code, not by any run. The five metacharacter-carrying patterns here are one edit away from the same class.
The duplication is why this is one issue and not two. hack/run-kubernetes-serial-console_test.bats defines the helper and calls it twelve times; hack/run-kubernetes-talos-diagnostics_test.bats defines its own copy and calls it four times. Neither references the other, so a fix in one reaches neither. And the count is growing rather than settled: a third copy with seventeen more call sites is in an open pull request right now, written by copying one of these two, which is the same way these two came to exist. Whatever is done here should land before that copy does, or it will be three.
Two separable halves to a fix. Branching on awk's exact status rather than on truthiness -- 1 is no match, 2 is an error -- closes the class without touching a single call site. Collapsing the copies into one shared definition stops the next hardening from landing in whichever file its author happened to open.
Activity
[-]test(e2e): a negative assertion passes when its own matcher fails, in two hand-copied helpers[/-][+]test(e2e): a negative assertion passes when its own matcher fails, and every security check in the suites relies on one[/+]lexfrei commentedon Aug 9, 2026
The two copies are not identical, and the difference is a second way for the assertion to go vacuous.
hack/run-kubernetes-serial-console_test.batsguards the file first: its helper is 16 lines and one of them is a[ ! -f ]check that fails the assertion when the artifact is missing.hack/run-kubernetes-talos-diagnostics_test.batsis 9 lines with no such guard. There, a missing artifact reaches awk directly, awk exits non-zero because it cannot open the file, and the assertion holds — so all four of that file's negative assertions, including'^[[:space:]]*(tls[.]crt|tls[.]key|data):'and'secret:|secretName:', pass when the artifact does not exist at all.That splits the problem in two rather than one:
1(no match) and2(bad regex) both satisfy the assertion. 16 call sites are exposed.talos-diagnosticsare satisfied by an absent artifact, and those are the ones checking that rendered manifests carry no key material.The second is worse in a specific way. A bad regex takes an edit to introduce; a missing artifact is a normal outcome of a failed run, which is exactly when these assertions are supposed to speak. A collection step that dies leaves no file, and the suite then reports that the file it never got contains no secrets.
Both halves are closed by the same fix from opposite ends: branch on awk's exact status so
2fails the assertion, and guard the file explicitly so a missing artifact fails it too. Neither requires touching a call site. The divergence between the copies is itself the argument for collapsing them: they have already drifted, so "keep them in sync by hand" is not the status quo being defended, it is a thing that stopped happening some time ago.lexfrei commentedon Aug 10, 2026
The two copies are not equally exposed, and the difference decides which to fix first.
hack/run-kubernetes-talos-diagnostics_test.batscarries neither guard: it does not check that the file exists, and it does not separate awk's exit 2 (could not evaluate the pattern) from exit 1 (evaluated, no match). Both failure modes land in the same branch as a clean pass.hack/run-kubernetes-serial-console_test.batshas the existence check and not the exit-2 split, so only the unparseable-pattern half is open there.That ordering matters because the missing existence check is the half that is reachable without any pattern being unusual. An assertion of the form "the log of calls contains no
delete" is satisfied by the log of calls never having been created, which is exactly the state a run produces when the code under test takes an early return and makes no calls at all. The assertion then reads as "it did not do the destructive thing" when what happened is "it did nothing, and nothing was recorded".The guard that closes both halves is small. Invoke awk with
|| _rc=$?rather than inside anif, then handle the three outcomes separately: 0 is a match and fails the assertion, 1 is a clean pass, anything else is reported as "could not evaluate" and fails. The|| _rc=$?matters on its own: a bareawkinside a function running underset -eaborts the function on exit 1, which is the ordinary no-match case.lexfrei commentedon Oct 2, 2026
Another instance of this class, found while fixing #4580: the read-only upload check in the
verify-s3-accessstep ofhack/e2e-chainsaw/bucket/chainsaw-test.yamltreats any non-zeromcexit other than 124 as the expected denial. A broken port-forward or a 5xx from the gateway therefore passes as "read-only user was denied", and the accessPolicy=readonly regression this check guards would go unnoticed behind any transport failure. Fix shape: require the specific S3AccessDeniederror text before counting the attempt as denied, and fail with a named message on anything else.