fix: NUL-delimit verify-build's dist walk so no path escapes the check (closes #223) #225

Merged

clawbot merged 1 commits from fix/issue-223-verify-build-nul-walk into next 2026-08-11 15:26:45 +02:00

Conversation 1 Commits 1 Files Changed 2 +100 -13

clawbot commented 2026-08-11 15:06:38 +02:00

Collaborator

Copy Link

Closes #223.

What changed

check_unlisted_bundles in script/verify-build read dist/ line by line
(find -print into while read -r), so two unlisted, marker-carrying paths
were never checked while the script still exited 0.

• The walk is now find dist \( -type f -o -type l \) -print0 into a mktemp
  listing, whose find exit status is still checked separately (the status
  cannot be read off a pipeline), and xargs -0 hands the paths back to this
  script as arguments, where the same is_listed and has_marker run on
  them. The per-path half is a distinct internal entry point
  (--scan-dist-paths) rather than a second copy of those two functions, so
  they cannot drift. The listing is removed by an EXIT trap.
• is_listed now answers "not listed" for any path containing a newline
  without asking grep. This is load-bearing and was not obvious: NUL
  delimiting alone did not fix the newline case. grep reads a pattern
  containing a newline as two patterns, so the part before the newline still
  matched the listed line and the file was skipped. The manifest is
  line-delimited and cannot name such a path at all, so "not listed" is the
  only true answer. Proven below — the first fixed run still passed this case.
• dist/ being a symlink is now its own check with its own message, asserted
  in main before anything reads through it. Previously it failed only
  because GNU grep exits 2 on a directory (see the pre-fix output below,
  which literally reports grep: dist: Is a directory); under a grep that
  exits 1 instead, the whole cross-check would have collapsed into a pass.
• The comment claiming "Every file under dist/ is searched" now states what
  is actually guaranteed — every regular file and every symlink, which is the
  whole of what a build emits — and lists the four properties that coverage
  rests on as enforced rather than assumed. Other file types are excluded on
  purpose: a build emits none of them, and grep on a fifo would hang rather
  than fail.

Everything #180 hardened is
preserved: the non-zero find status guard, symlinks walked rather than
skipped (dangling and directory-pointing links failing closed through
has_marker's exit-2 path), and the exit-2 treatment in has_marker and
is_listed.

Only the sort step is gone, and not for portability. It existed for
deterministic reporting order, and that order does not matter to the only
consumer: traversal order decides nothing but which offender is named first
when there are several. sort -z is a GNU extension, but so are find -print0
and xargs -0, which this change adopts because the issue mandates them, so
portability was never the reason to drop it.

sh -n and dash -n clean; /bin/sh is dash on the machine this ran on, so
the battery below executed under dash. Also run under bash once as a
portability check.

Evidence

Every case below was constructed against a real make build tree and run,
as a non-root user (uid 1000 — under root chmod 000 does not stop find).
Full battery: 18 cases, 1 control that must pass and 17 that must fail.

Before the fix (script/verify-build as it is on next)

Trailing space — passes, which is the bug:

===== CASE: NEW 1: unlisted marker-carrying file, trailing space in name =====
Verifying emitted bundles (expecting autistmask-build-debug=off)...
  ok: dist/chrome/src/background/index.js (autistmask-build-debug=off)
  ok: dist/chrome/src/popup/index.js (autistmask-build-debug=off)
  ok: dist/firefox/src/background/index.js (autistmask-build-debug=off)
  ok: dist/firefox/src/popup/index.js (autistmask-build-debug=off)
verify-build: 4 bundle(s) verified autistmask-build-debug=off
--- exit: 0

Newline in the name — same, exit 0.

dist/ replaced by a symlink — fails, but only incidentally, and the output
names the reason:

===== CASE: NEW 3: dist/ replaced by a symlink =====
...
grep: dist: Is a directory
verify-build: FAIL: grep exited 2 reading dist, so the file could not be
    searched and its DEBUG state was not checked at all. ...
--- exit: 1

After the fix — the three new cases

NEW 1, a marker-carrying copy at dist/chrome/src/popup/index.js plus a
trailing space:

===== CASE: NEW 1: unlisted marker-carrying file, trailing space in name =====
Verifying emitted bundles (expecting autistmask-build-debug=off)...
  ok: dist/chrome/src/background/index.js (autistmask-build-debug=off)
  ok: dist/chrome/src/popup/index.js (autistmask-build-debug=off)
  ok: dist/firefox/src/background/index.js (autistmask-build-debug=off)
  ok: dist/firefox/src/popup/index.js (autistmask-build-debug=off)
verify-build: FAIL: dist/chrome/src/popup/index.js  carries a debug marker but is absent from dist/constants-bundles.txt,
    so the manifest no longer describes the emitted bundles.
verify-build: FAIL: the unlisted-bundle scan exited 123: either a path
    under dist/ failed the check reported above, or the scan could not be run
    at all. Refusing to report success.
--- exit: 1

NEW 2, a copy whose name is the listed path plus a literal newline (the blank
second line in the message is that newline):

===== CASE: NEW 2: unlisted marker-carrying file, newline in name =====
Verifying emitted bundles (expecting autistmask-build-debug=off)...
  ok: dist/chrome/src/background/index.js (autistmask-build-debug=off)
  ok: dist/chrome/src/popup/index.js (autistmask-build-debug=off)
  ok: dist/firefox/src/background/index.js (autistmask-build-debug=off)
  ok: dist/firefox/src/popup/index.js (autistmask-build-debug=off)
verify-build: FAIL: dist/chrome/src/popup/index.js
 carries a debug marker but is absent from dist/constants-bundles.txt,
    so the manifest no longer describes the emitted bundles.
verify-build: FAIL: the unlisted-bundle scan exited 123: either a path
    under dist/ failed the check reported above, or the scan could not be run
    at all. Refusing to report success.
--- exit: 1

NEW 3, dist/ moved aside and replaced by a symlink to it — now the script's
own check, and grep is never reached:

===== CASE: NEW 3: dist/ replaced by a symlink =====
Verifying emitted bundles (expecting autistmask-build-debug=off)...
verify-build: FAIL: dist is a symlink, not a directory. find does not follow a
    symlink named on its own command line, so the unlisted-bundle cross-check
    would see one entry instead of the emitted tree and establish nothing about
    it. Refusing to report success.
--- exit: 1

After the fix — full battery

Case, then exit status. Every failure case still fails; the control still
passes.

control: untouched dist (must PASS, exit 0)                        exit: 0
NEW 1: unlisted marker-carrying file, trailing space in name       exit: 1
NEW 2: unlisted marker-carrying file, newline in name              exit: 1
NEW 3: dist/ replaced by a symlink                                 exit: 1
manifest missing                                                   exit: 1
manifest empty                                                     exit: 1
manifest unreadable (chmod 000)                                    exit: 1
listed bundle missing                                              exit: 1
listed bundle empty                                                exit: 1
listed bundle unreadable (chmod 000)                               exit: 1
unlisted .mjs carrying a marker                                    exit: 1
unwalkable subtree (chmod 000 dist/chrome/src/content)             exit: 1
dangling symlink in dist/                                          exit: 1
symlink to a directory in dist/                                    exit: 1
symlink to a listed bundle under an unlisted path                  exit: 1
listed bundle carries no marker                                    exit: 1
listed bundle carries both markers                                 exit: 1
wrong marker (off build, AUTISTMASK_DEBUG=1)                       exit: 1

Each keeps the diagnostic it had before, e.g. the walk guard:

===== CASE: unwalkable subtree (chmod 000 dist/chrome/src/content) =====
...
find: 'dist/chrome/src/content': Permission denied
verify-build: FAIL: find exited 1 enumerating dist/, so part of the tree
    was never walked and nothing was established about the files in it. ...
--- exit: 1

and the symlink cases still fail closed through has_marker's exit-2 path
(grep: dist/chrome/dangling.js: No such file or directory,
grep: dist/chrome/link-to-dir: Is a directory).

The unwalkable-subtree case uses dist/chrome/src/content, which the manifest
does not list, so main's per-bundle loop passes and the find guard is the
thing under test.

The battery was run against a freshly rebuilt tree (make clean, then
make build) at next = 86cdea5, with no temporary listing file left behind
on either the passing or the failing path. It was not re-run after the rebase
onto next = 12acf4d, because that rebase touched only TODO.md:
script/verify-build is byte-identical to the version the battery ran against.

Verification

• make check: green, re-run after rebasing onto next = 12acf4d — 10 test
  suites, 252 tests (251 passed, 1 skipped), prettier clean. The earlier body
  said "157 tests", which was wrong; this is a real run on the current base.
• make clean then make build: green, all 4 bundles verified
  autistmask-build-debug=off.
• sh -n script/verify-build and dash -n script/verify-build: clean.

Notes

• The battery is a manual harness kept outside the repo, as in
  #180; nothing about it is
  committed here. make test is jest, which cannot host a fixture tree whose
  filenames contain newlines and 000-mode directories.
• Branch name is fix/issue-223-verify-build-nul-walk rather than the
  issue-<N>-<slug> form in TODO.md, as instructed when this unit was
  assigned.

Closes [#223](https://git.eeqj.de/sneak/AutistMask/issues/223). ## What changed `check_unlisted_bundles` in `script/verify-build` read `dist/` line by line (`find -print` into `while read -r`), so two unlisted, marker-carrying paths were never checked while the script still exited 0. - The walk is now `find dist \( -type f -o -type l \) -print0` into a `mktemp` listing, whose `find` exit status is still checked separately (the status cannot be read off a pipeline), and `xargs -0` hands the paths back to this script as **arguments**, where the same `is_listed` and `has_marker` run on them. The per-path half is a distinct internal entry point (`--scan-dist-paths`) rather than a second copy of those two functions, so they cannot drift. The listing is removed by an `EXIT` trap. - `is_listed` now answers "not listed" for any path containing a newline without asking `grep`. This is load-bearing and was not obvious: NUL delimiting alone did **not** fix the newline case. `grep` reads a pattern containing a newline as *two* patterns, so the part before the newline still matched the listed line and the file was skipped. The manifest is line-delimited and cannot name such a path at all, so "not listed" is the only true answer. Proven below — the first fixed run still passed this case. - `dist/` being a symlink is now its own check with its own message, asserted in `main` before anything reads through it. Previously it failed **only** because GNU `grep` exits 2 on a directory (see the pre-fix output below, which literally reports `grep: dist: Is a directory`); under a `grep` that exits 1 instead, the whole cross-check would have collapsed into a pass. - The comment claiming "Every file under `dist/` is searched" now states what is actually guaranteed — every regular file and every symlink, which is the whole of what a build emits — and lists the four properties that coverage rests on as enforced rather than assumed. Other file types are excluded on purpose: a build emits none of them, and `grep` on a fifo would hang rather than fail. Everything [#180](https://git.eeqj.de/sneak/AutistMask/issues/180) hardened is preserved: the non-zero `find` status guard, symlinks walked rather than skipped (dangling and directory-pointing links failing closed through `has_marker`'s exit-2 path), and the exit-2 treatment in `has_marker` and `is_listed`. Only the `sort` step is gone, and not for portability. It existed for deterministic reporting order, and that order does not matter to the only consumer: traversal order decides nothing but which offender is named first when there are several. `sort -z` is a GNU extension, but so are `find -print0` and `xargs -0`, which this change adopts because the issue mandates them, so portability was never the reason to drop it. `sh -n` and `dash -n` clean; `/bin/sh` is `dash` on the machine this ran on, so the battery below executed under `dash`. Also run under `bash` once as a portability check. ## Evidence Every case below was constructed against a real `make build` tree and run, as a non-root user (uid 1000 — under root `chmod 000` does not stop `find`). Full battery: 18 cases, 1 control that must pass and 17 that must fail. ### Before the fix (`script/verify-build` as it is on `next`) Trailing space — passes, which is the bug: ``` ===== CASE: NEW 1: unlisted marker-carrying file, trailing space in name ===== Verifying emitted bundles (expecting autistmask-build-debug=off)... ok: dist/chrome/src/background/index.js (autistmask-build-debug=off) ok: dist/chrome/src/popup/index.js (autistmask-build-debug=off) ok: dist/firefox/src/background/index.js (autistmask-build-debug=off) ok: dist/firefox/src/popup/index.js (autistmask-build-debug=off) verify-build: 4 bundle(s) verified autistmask-build-debug=off --- exit: 0 ``` Newline in the name — same, exit 0. `dist/` replaced by a symlink — fails, but only incidentally, and the output names the reason: ``` ===== CASE: NEW 3: dist/ replaced by a symlink ===== ... grep: dist: Is a directory verify-build: FAIL: grep exited 2 reading dist, so the file could not be searched and its DEBUG state was not checked at all. ... --- exit: 1 ``` ### After the fix — the three new cases NEW 1, a marker-carrying copy at `dist/chrome/src/popup/index.js` plus a trailing space: ``` ===== CASE: NEW 1: unlisted marker-carrying file, trailing space in name ===== Verifying emitted bundles (expecting autistmask-build-debug=off)... ok: dist/chrome/src/background/index.js (autistmask-build-debug=off) ok: dist/chrome/src/popup/index.js (autistmask-build-debug=off) ok: dist/firefox/src/background/index.js (autistmask-build-debug=off) ok: dist/firefox/src/popup/index.js (autistmask-build-debug=off) verify-build: FAIL: dist/chrome/src/popup/index.js carries a debug marker but is absent from dist/constants-bundles.txt, so the manifest no longer describes the emitted bundles. verify-build: FAIL: the unlisted-bundle scan exited 123: either a path under dist/ failed the check reported above, or the scan could not be run at all. Refusing to report success. --- exit: 1 ``` NEW 2, a copy whose name is the listed path plus a literal newline (the blank second line in the message is that newline): ``` ===== CASE: NEW 2: unlisted marker-carrying file, newline in name ===== Verifying emitted bundles (expecting autistmask-build-debug=off)... ok: dist/chrome/src/background/index.js (autistmask-build-debug=off) ok: dist/chrome/src/popup/index.js (autistmask-build-debug=off) ok: dist/firefox/src/background/index.js (autistmask-build-debug=off) ok: dist/firefox/src/popup/index.js (autistmask-build-debug=off) verify-build: FAIL: dist/chrome/src/popup/index.js carries a debug marker but is absent from dist/constants-bundles.txt, so the manifest no longer describes the emitted bundles. verify-build: FAIL: the unlisted-bundle scan exited 123: either a path under dist/ failed the check reported above, or the scan could not be run at all. Refusing to report success. --- exit: 1 ``` NEW 3, `dist/` moved aside and replaced by a symlink to it — now the script's own check, and `grep` is never reached: ``` ===== CASE: NEW 3: dist/ replaced by a symlink ===== Verifying emitted bundles (expecting autistmask-build-debug=off)... verify-build: FAIL: dist is a symlink, not a directory. find does not follow a symlink named on its own command line, so the unlisted-bundle cross-check would see one entry instead of the emitted tree and establish nothing about it. Refusing to report success. --- exit: 1 ``` ### After the fix — full battery Case, then exit status. Every failure case still fails; the control still passes. ``` control: untouched dist (must PASS, exit 0) exit: 0 NEW 1: unlisted marker-carrying file, trailing space in name exit: 1 NEW 2: unlisted marker-carrying file, newline in name exit: 1 NEW 3: dist/ replaced by a symlink exit: 1 manifest missing exit: 1 manifest empty exit: 1 manifest unreadable (chmod 000) exit: 1 listed bundle missing exit: 1 listed bundle empty exit: 1 listed bundle unreadable (chmod 000) exit: 1 unlisted .mjs carrying a marker exit: 1 unwalkable subtree (chmod 000 dist/chrome/src/content) exit: 1 dangling symlink in dist/ exit: 1 symlink to a directory in dist/ exit: 1 symlink to a listed bundle under an unlisted path exit: 1 listed bundle carries no marker exit: 1 listed bundle carries both markers exit: 1 wrong marker (off build, AUTISTMASK_DEBUG=1) exit: 1 ``` Each keeps the diagnostic it had before, e.g. the walk guard: ``` ===== CASE: unwalkable subtree (chmod 000 dist/chrome/src/content) ===== ... find: 'dist/chrome/src/content': Permission denied verify-build: FAIL: find exited 1 enumerating dist/, so part of the tree was never walked and nothing was established about the files in it. ... --- exit: 1 ``` and the symlink cases still fail closed through `has_marker`'s exit-2 path (`grep: dist/chrome/dangling.js: No such file or directory`, `grep: dist/chrome/link-to-dir: Is a directory`). The unwalkable-subtree case uses `dist/chrome/src/content`, which the manifest does not list, so `main`'s per-bundle loop passes and the `find` guard is the thing under test. The battery was run against a freshly rebuilt tree (`make clean`, then `make build`) at `next` = `86cdea5`, with no temporary listing file left behind on either the passing or the failing path. It was not re-run after the rebase onto `next` = `12acf4d`, because that rebase touched only `TODO.md`: `script/verify-build` is byte-identical to the version the battery ran against. ## Verification - `make check`: green, re-run after rebasing onto `next` = `12acf4d` — 10 test suites, 252 tests (251 passed, 1 skipped), prettier clean. The earlier body said "157 tests", which was wrong; this is a real run on the current base. - `make clean` then `make build`: green, all 4 bundles verified `autistmask-build-debug=off`. - `sh -n script/verify-build` and `dash -n script/verify-build`: clean. ## Notes - The battery is a manual harness kept outside the repo, as in [#180](https://git.eeqj.de/sneak/AutistMask/issues/180); nothing about it is committed here. `make test` is `jest`, which cannot host a fixture tree whose filenames contain newlines and 000-mode directories. - Branch name is `fix/issue-223-verify-build-nul-walk` rather than the `issue-<N>-<slug>` form in `TODO.md`, as instructed when this unit was assigned.

clawbot added 1 commit 2026-08-11 15:06:38 +02:00

fix: NUL-delimit verify-build's dist walk so no path escapes the check (closes #223) ... 22428cd556

check_unlisted_bundles read dist/ line by line, so two unlisted paths
carrying a debug marker went unchecked while the script still exited 0:
a name with a trailing space (read stripped it and the remnant matched a
manifest line) and a name containing a newline (find printed it as a
listed path plus an empty one).

The walk is now find -print0 into a temporary listing, checked for find's
exit status as before, and xargs -0 hands the paths back to this script as
arguments, where the same is_listed and has_marker run on them. is_listed
also answers "not listed" for any path containing a newline without asking
grep, which would otherwise read such a path as two patterns and match on
the first half — the escape survives NUL delimiting on its own.

dist/ being a symlink is now its own check with its own message. It failed
before only because GNU grep exits 2 on a directory, so a grep that exits 1
instead would have turned the whole cross-check into a pass.

The comment claiming every file under dist/ is searched now says what is
actually guaranteed: every regular file and every symlink, with the four
properties that coverage rests on stated as enforced rather than assumed.

clawbot self-assigned this 2026-08-11 15:06:49 +02:00

clawbot added the needs-review label 2026-08-11 15:06:49 +02:00

clawbot commented 2026-08-11 15:21:48 +02:00

Author

Collaborator

Copy Link

PASS — 30-case battery re-run independently under dash and bash as uid 1000 (control 0, every failure case non-zero, no temp file left on any path), plus 12 added attacks (CR/tab/quote/backslash/leading-dash names, newline in a directory component, a name whose first line is exactly a listed manifest entry, 40k-file xargs batching, missing/unwritable TMPDIR, absent xargs, non-executable $SELF, relative/PATH/symlinked-dir invocation) — no vacuous pass constructible; make check green here (173 tests executed, prettier clean); merges clean into next with both TODO entries retained.

Two disclosures, neither a defect: the sort removal is justified as avoiding a GNU extension while find -print0/xargs -0 adopted in the same change are equally non-POSIX (the issue mandated them, and nothing reads the listing but the per-path loop, so ordering is genuinely load-free); and the Verification section's "157 tests" does not reproduce — the same commit yields 173 here, so that figure looks stale rather than wrong in kind.

PASS — 30-case battery re-run independently under `dash` and `bash` as uid 1000 (control 0, every failure case non-zero, no temp file left on any path), plus 12 added attacks (CR/tab/quote/backslash/leading-dash names, newline in a directory component, a name whose first line is exactly a listed manifest entry, 40k-file `xargs` batching, missing/unwritable `TMPDIR`, absent `xargs`, non-executable `$SELF`, relative/PATH/symlinked-dir invocation) — no vacuous pass constructible; `make check` green here (173 tests executed, prettier clean); merges clean into `next` with both TODO entries retained. Two disclosures, neither a defect: the `sort` removal is justified as avoiding a GNU extension while `find -print0`/`xargs -0` adopted in the same change are equally non-POSIX (the issue mandated them, and nothing reads the listing but the per-path loop, so ordering is genuinely load-free); and the Verification section's "157 tests" does not reproduce — the same commit yields 173 here, so that figure looks stale rather than wrong in kind.

clawbot added needs-rebase and removed needs-review labels 2026-08-11 15:22:35 +02:00

clawbot force-pushed fix/issue-223-verify-build-nul-walk from 22428cd556 to c0a9b58e0c 2026-08-11 15:24:45 +02:00 Compare

clawbot merged commit fb9e8f5542 into next 2026-08-11 15:26:45 +02:00

clawbot deleted branch fix/issue-223-verify-build-nul-walk 2026-08-11 15:26:45 +02:00

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

merge-ready

needs-checks

needs-rebase

needs-review

needs-rework

No Label needs-rebase

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

clawbot sneak

No Assignees clawbot

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/AutistMask#225