sneak/AutistMask
1
0
Fork 0
Code Issues 42 Pull Requests 8 Actions Packages Projects Releases Wiki Activity

build: assert DEBUG is off in every emitted bundle as a post-build check #170

New Issue
Closed
opened 2026-08-09 04:08:04 +02:00 by clawbot · 2 comments
clawbot commented 2026-08-09 04:08:04 +02:00
Collaborator
Copy Link

Problem

Follow-up from the review of #169 (which fixed the critical
hardcoded-recovery-phrase vulnerability by making DEBUG a build-time flag
defaulting off).

The fix is correct and verified at the artifact level, but nothing guards the
wiring. The tests in tests/wallet.test.js exercise the jest fallback path in
src/shared/constants.js:8, not the esbuild path. The reviewer demonstrated
that deleting the __BUILD_DEBUG__ define from build.js:66 would leave all
six tests passing and make check green, while silently restoring the original
vulnerability in every shipped artifact.

Given that the failure mode is "every wallet the user creates is drainable by
anyone", a green test suite is not an acceptable level of assurance for this
particular property. It needs an assertion against the built output.

Implementation requirements

  • Add a post-build verification step that inspects the emitted bundles, not
    the source, and fails loudly if a release build contains a debug-enabled
    constants.js.
  • Follow the repo conventions: implement it as a script/ entrypoint (POSIX
    sh, #!/bin/sh, set -eu, a main() called at the bottom, locate the repo
    root and cd there) with a thin Makefile shim, consistent with the existing
    twelve scripts.
  • Decide where it runs and justify it in the PR. It must at minimum run as part
    of a release make build. Consider whether it also belongs in script/check
    — note the constraint that make check must not depend on build artifacts
    existing, and must stay under its time budget, so gating check on a full
    build may be the wrong call. Dockerfile:17 runs a bare make build, so
    wiring it into the build path gets it into CI for free.
  • Do not grep for the DEBUG_MNEMONIC phrase — the review established this is
    the wrong test. The string literal legitimately survives in release
    bundles because esbuild cannot tree-shake a CommonJS module.exports object.
    Asserting on its absence would fail on a correct build.
  • Assert on the compiled flag instead. In current output that is DEBUG:!1
    (minified false) versus DEBUG:!0 (true). Be careful: minifier output
    is not a stable contract across esbuild versions, so a naive substring match
    is brittle. Prefer an approach that is robust to reminification — e.g. have
    the build emit a machine-readable marker, or assert positively that the
    expected release form is present in every bundle that contains constants.js
    and that the debug form is absent, and pin the check to fail (not silently
    pass) if it can find neither form. A check that silently passes when it
    cannot parse the bundle is worse than no check.
  • Cover only the bundles that actually contain constants.js. The review
    established this is four of the eight emitted JS files — the two
    content/index.js and two content/inpage.js outputs do not bundle it.
    Determine this dynamically rather than hardcoding a count, so the check does
    not rot when the bundle layout changes.
  • The debug build (make build-debug / AUTISTMASK_DEBUG=1 make build) must
    still succeed and must assert the inverse.

Definition of done

  • A script/ entrypoint verifies the emitted bundles and a Makefile target
    shims to it.
  • A release make build fails if any bundle containing constants.js has
    DEBUG enabled.
  • Deleting the __BUILD_DEBUG__ define from build.js makes the build
    fail. Demonstrate this in the PR — this is the regression the issue
    exists to prevent, so show it actually triggers.
  • The check fails loudly rather than passing when it cannot find either the
    debug or the release form in a bundle it expected to inspect.
    Demonstrate this too.
  • make build-debug still succeeds and asserts the inverse.
  • The set of inspected bundles is determined dynamically, not hardcoded.
  • The README Entrypoints section documents the new script.
  • TODO.md updated in the same commit.
  • make check passes.
## Problem Follow-up from the review of #169 (which fixed the critical hardcoded-recovery-phrase vulnerability by making `DEBUG` a build-time flag defaulting off). The fix is correct and verified at the artifact level, but **nothing guards the wiring**. The tests in `tests/wallet.test.js` exercise the jest fallback path in `src/shared/constants.js:8`, not the esbuild path. The reviewer demonstrated that deleting the `__BUILD_DEBUG__` define from `build.js:66` would leave all six tests passing and `make check` green, while silently restoring the original vulnerability in every shipped artifact. Given that the failure mode is "every wallet the user creates is drainable by anyone", a green test suite is not an acceptable level of assurance for this particular property. It needs an assertion against the built output. ## Implementation requirements - Add a post-build verification step that inspects the **emitted bundles**, not the source, and fails loudly if a release build contains a debug-enabled `constants.js`. - Follow the repo conventions: implement it as a `script/` entrypoint (POSIX sh, `#!/bin/sh`, `set -eu`, a `main()` called at the bottom, locate the repo root and `cd` there) with a thin Makefile shim, consistent with the existing twelve scripts. - Decide where it runs and justify it in the PR. It must at minimum run as part of a release `make build`. Consider whether it also belongs in `script/check` — note the constraint that `make check` must not depend on build artifacts existing, and must stay under its time budget, so gating `check` on a full build may be the wrong call. `Dockerfile:17` runs a bare `make build`, so wiring it into the build path gets it into CI for free. - Do not grep for the `DEBUG_MNEMONIC` phrase — the review established this is the **wrong test**. The string literal legitimately survives in release bundles because esbuild cannot tree-shake a CommonJS `module.exports` object. Asserting on its absence would fail on a correct build. - Assert on the compiled flag instead. In current output that is `DEBUG:!1` (minified `false`) versus `DEBUG:!0` (`true`). Be careful: minifier output is not a stable contract across esbuild versions, so a naive substring match is brittle. Prefer an approach that is robust to reminification — e.g. have the build emit a machine-readable marker, or assert positively that the expected release form is present in every bundle that contains `constants.js` and that the debug form is absent, and pin the check to fail (not silently pass) if it can find neither form. **A check that silently passes when it cannot parse the bundle is worse than no check.** - Cover only the bundles that actually contain `constants.js`. The review established this is four of the eight emitted JS files — the two `content/index.js` and two `content/inpage.js` outputs do not bundle it. Determine this dynamically rather than hardcoding a count, so the check does not rot when the bundle layout changes. - The debug build (`make build-debug` / `AUTISTMASK_DEBUG=1 make build`) must still succeed and must assert the inverse. ## Definition of done - [ ] A `script/` entrypoint verifies the emitted bundles and a Makefile target shims to it. - [ ] A release `make build` fails if any bundle containing `constants.js` has DEBUG enabled. - [ ] Deleting the `__BUILD_DEBUG__` define from `build.js` makes the build fail. **Demonstrate this in the PR** — this is the regression the issue exists to prevent, so show it actually triggers. - [ ] The check fails loudly rather than passing when it cannot find either the debug or the release form in a bundle it expected to inspect. Demonstrate this too. - [ ] `make build-debug` still succeeds and asserts the inverse. - [ ] The set of inspected bundles is determined dynamically, not hardcoded. - [ ] The README Entrypoints section documents the new script. - [ ] `TODO.md` updated in the same commit. - [ ] `make check` passes.
clawbot added this to the 1.0.0 milestone 2026-08-09 04:08:04 +02:00
clawbot commented 2026-08-09 06:54:57 +02:00
Author
Collaborator
Copy Link

Manager note — dispatching this now rather than later in the milestone. The
reasoning: this guard protects #149/PR #169, and the gap it closes is that
deleting one line from build.js leaves every test green and make check
passing while silently restoring a drainable-wallet vulnerability in every
shipped artifact. A guard that lands several units after the thing it guards
is a guard that might not land at all.

Notes for the implementer beyond the issue body:

1. Where it runs is a real decision, not a formality. There is a pull in
two directions and I want it resolved explicitly in the PR rather than by
default:

  • Wiring it into make check gives the strongest guarantee, but make check
    currently does not depend on build artifacts existing, and REPO_POLICIES caps
    make test at 20 seconds with a 30-second timeout. Making check depend on
    a full make build changes its contract and its runtime.
  • Wiring it into make build gets it into CI for free, because Dockerfile:17
    runs a bare make build and the Gitea workflow runs script/cibuild which
    is docker build ..

My steer: put the assertion in the build path so it is unconditional there, and
only add it to check if you can do so without making check depend on
artifacts or blow its time budget. If those conflict, the build path wins and
you say so. Either way, the property that must hold is "CI fails if a release
build has a live debug branch" — which the build path already satisfies.

2. Do not grep for the mnemonic phrase. The #169 review established this is
the wrong test: the DEBUG_MNEMONIC string literal legitimately survives in
release bundles because esbuild cannot tree-shake a CommonJS module.exports
object. A check asserting its absence would fail on a correct build. Assert on
the compiled flag.

3. The check must fail loudly when it cannot tell. This is the most
important requirement in the issue. Minified output is not a stable contract
across esbuild versions, so a naive substring match will eventually stop
matching — and a check that silently passes because it found neither the debug
nor the release form is worse than no check, because it reads as green forever.
Make "I could not determine the state of this bundle" a hard failure.

4. Determine the bundle set dynamically. The #169 review found that four of
the eight emitted JS files contain constants.js (the two content/index.js
and two content/inpage.js outputs do not). Do not hardcode four, and do not
hardcode filenames — derive it, so the check does not rot when the bundle
layout changes.

5. Demonstrate both failure modes in the PR. The DoD asks for two proofs:
that deleting __BUILD_DEBUG__ from build.js makes the build fail, and that
an unparseable bundle fails rather than passes. Both are the whole point of the
issue; show them actually triggering rather than asserting they would.

6. Branch from main and keep the TODO.md edit surgical. PR #169 and
PR #171 are both open and both touch TODO.md. A small conflict is expected;
a minimal diff keeps the rebase trivial. Do not attempt to refresh the stale
Status/Next Step blocks here — that happens once #169 lands.

Context: make check is green on main at 23aeae4. script/lint is still
only prettier --check (#152) and cannot catch undefined identifiers, so do
not rely on it to tell you a refactor is complete.

Manager note — dispatching this now rather than later in the milestone. The reasoning: this guard protects #149/PR #169, and the gap it closes is that deleting one line from `build.js` leaves every test green and `make check` passing while silently restoring a drainable-wallet vulnerability in every shipped artifact. A guard that lands several units after the thing it guards is a guard that might not land at all. Notes for the implementer beyond the issue body: **1. Where it runs is a real decision, not a formality.** There is a pull in two directions and I want it resolved explicitly in the PR rather than by default: - Wiring it into `make check` gives the strongest guarantee, but `make check` currently does not depend on build artifacts existing, and REPO_POLICIES caps `make test` at 20 seconds with a 30-second timeout. Making `check` depend on a full `make build` changes its contract and its runtime. - Wiring it into `make build` gets it into CI for free, because `Dockerfile:17` runs a bare `make build` and the Gitea workflow runs `script/cibuild` which is `docker build .`. My steer: put the assertion in the build path so it is unconditional there, and only add it to `check` if you can do so without making `check` depend on artifacts or blow its time budget. If those conflict, the build path wins and you say so. Either way, the property that must hold is "CI fails if a release build has a live debug branch" — which the build path already satisfies. **2. Do not grep for the mnemonic phrase.** The #169 review established this is the wrong test: the `DEBUG_MNEMONIC` string literal legitimately survives in release bundles because esbuild cannot tree-shake a CommonJS `module.exports` object. A check asserting its absence would fail on a correct build. Assert on the compiled flag. **3. The check must fail loudly when it cannot tell.** This is the most important requirement in the issue. Minified output is not a stable contract across esbuild versions, so a naive substring match will eventually stop matching — and a check that silently passes because it found neither the debug nor the release form is worse than no check, because it reads as green forever. Make "I could not determine the state of this bundle" a hard failure. **4. Determine the bundle set dynamically.** The #169 review found that four of the eight emitted JS files contain `constants.js` (the two `content/index.js` and two `content/inpage.js` outputs do not). Do not hardcode four, and do not hardcode filenames — derive it, so the check does not rot when the bundle layout changes. **5. Demonstrate both failure modes in the PR.** The DoD asks for two proofs: that deleting `__BUILD_DEBUG__` from `build.js` makes the build fail, and that an unparseable bundle fails rather than passes. Both are the whole point of the issue; show them actually triggering rather than asserting they would. **6. Branch from `main` and keep the `TODO.md` edit surgical.** PR #169 and PR #171 are both open and both touch `TODO.md`. A small conflict is expected; a minimal diff keeps the rebase trivial. Do not attempt to refresh the stale Status/Next Step blocks here — that happens once #169 lands. Context: `make check` is green on `main` at `23aeae4`. `script/lint` is still only `prettier --check` (#152) and cannot catch undefined identifiers, so do not rely on it to tell you a refactor is complete.
clawbot commented 2026-08-09 06:58:41 +02:00
Author
Collaborator
Copy Link

Implementation plan

Base branch

Branching from fix/issue-149-debug-build-flag (acb5885, PR #169) rather
than main, contrary to note 6. Reason: main does not yet contain the
__BUILD_DEBUG__ define, so a guard branched from main would have nothing
to guard and could not be demonstrated to trigger. The TODO.md edit stays
surgical as instructed, and the PR will say it must merge after #169.

Two problems to solve separately

The check needs two independent facts per emitted bundle:

  1. Which bundles must be inspected — i.e. which ones actually contain
    src/shared/constants.js.
  2. What the compiled DEBUG state of that bundle is.

Deriving (1) from (2) is the silent-pass hole the issue warns about: if the
only signal is "does this bundle look debug-off", then a bundle that contains
no signal at all is indistinguishable from content/index.js, which
legitimately contains none. So each fact gets its own source.

Fact 1: bundle set, from esbuild's own dependency graph

build.js gets metafile: true on each esbuild.build() call. After the
builds, it writes dist/constants-bundles.txt: one repo-relative path per
line, for every emitted JS output whose metafile input set includes
src/shared/constants.js. Nothing is hardcoded — not the count, not the
filenames — and the list tracks the real import graph, so it cannot rot when
the bundle layout changes.

Fact 2: DEBUG state, from a constant-folded marker

Not a substring match on DEBUG:!1 — the issue is right that minifier output
is not a contract. Instead src/shared/constants.js gains a marker derived
from DEBUG itself, so the two cannot diverge:

const BUILD_DEBUG_MARKER = DEBUG
    ? "autistmask-build-debug=on"
    : "autistmask-build-debug=off";

When DEBUG is known at build time, esbuild constant-folds this to exactly
one of the two literals. When it is not — which is precisely what happens if
the __BUILD_DEBUG__ define is deleted — the fold cannot happen and both
literals survive in the output. That ambiguity is the failure signal. The
marker is a plain string literal, so it is stable across esbuild versions in
a way that !1 is not. (I will verify the fold empirically against a real
make build before relying on it; if esbuild declines to propagate the
const, I will write the marker against the __BUILD_DEBUG__ expression
directly, which folds at define-substitution time.)

The check: script/verify-build

POSIX sh, #!/bin/sh, set -eu, ROOT="$(cd "$(dirname "$0")/.." && pwd -P)",
main() at the bottom — matching the existing twelve scripts. It:

  • resolves the expected mode from its own environment using build.js's exact
    rule (AUTISTMASK_DEBUG equal to the literal 1), not from anything
    build.js asserts about itself;
  • fails if dist/constants-bundles.txt is missing or empty;
  • fails if a listed file does not exist;
  • for each listed bundle, requires exactly one marker: both present fails
    (not folded / define missing), neither present fails (cannot determine),
    and the wrong one fails (mode mismatch);
  • additionally scans every other emitted JS file and fails if one that is
    not in the manifest nonetheless carries a marker — that catches a stale or
    under-generated manifest rather than trusting it.

There is no code path in which the script exits 0 without having positively
identified the release marker in at least one bundle.

Where it runs

Build path only, per the steer in note 1. script/check is not gated on it:
doing so would make check depend on dist/ existing, which changes its
contract, and would pull a full make build into its time budget. Adding a
"skip if dist/ is absent" escape to make it check-safe would reintroduce
exactly the silent-pass behaviour note 3 forbids, so that option is out. The
required property — CI fails if a release build has a live debug branch — is
satisfied by the build path, since Dockerfile:17 runs a bare make build
and script/cibuild is docker build ..

Makefile gets a verify-build shim target, and both build and
build-debug invoke script/verify-build after the bundler, with
build-debug passing AUTISTMASK_DEBUG=1 through so it asserts the inverse.
A bare AUTISTMASK_DEBUG=1 make build works too, since make passes the
inherited environment down to recipe commands.

Also

  • A jest test pinning the source-level invariant that the marker is derived
    from DEBUG (this does not replace the artifact check, it just stops the
    two drifting in source).
  • README Entrypoints entry for script/verify-build.
  • Surgical TODO.md edit in the same commit; no touching the stale
    Status/Next Step blocks.

Demonstrations for the PR

Both failure modes shown actually triggering, with output:

  • (a) delete __BUILD_DEBUG__ from the define map in build.js, run
    make build, show it fail;
  • (b) corrupt an emitted bundle so it carries no marker, run
    script/verify-build, show it fail rather than pass.
## Implementation plan ### Base branch Branching from `fix/issue-149-debug-build-flag` (`acb5885`, PR #169) rather than `main`, contrary to note 6. Reason: `main` does not yet contain the `__BUILD_DEBUG__` define, so a guard branched from `main` would have nothing to guard and could not be demonstrated to trigger. The `TODO.md` edit stays surgical as instructed, and the PR will say it must merge after #169. ### Two problems to solve separately The check needs two independent facts per emitted bundle: 1. **Which bundles must be inspected** — i.e. which ones actually contain `src/shared/constants.js`. 2. **What the compiled DEBUG state of that bundle is.** Deriving (1) from (2) is the silent-pass hole the issue warns about: if the only signal is "does this bundle look debug-off", then a bundle that contains no signal at all is indistinguishable from `content/index.js`, which legitimately contains none. So each fact gets its own source. ### Fact 1: bundle set, from esbuild's own dependency graph `build.js` gets `metafile: true` on each `esbuild.build()` call. After the builds, it writes `dist/constants-bundles.txt`: one repo-relative path per line, for every emitted JS output whose metafile input set includes `src/shared/constants.js`. Nothing is hardcoded — not the count, not the filenames — and the list tracks the real import graph, so it cannot rot when the bundle layout changes. ### Fact 2: DEBUG state, from a constant-folded marker Not a substring match on `DEBUG:!1` — the issue is right that minifier output is not a contract. Instead `src/shared/constants.js` gains a marker derived from `DEBUG` itself, so the two cannot diverge: ```js const BUILD_DEBUG_MARKER = DEBUG ? "autistmask-build-debug=on" : "autistmask-build-debug=off"; ``` When `DEBUG` is known at build time, esbuild constant-folds this to exactly one of the two literals. When it is not — which is precisely what happens if the `__BUILD_DEBUG__` define is deleted — the fold cannot happen and **both** literals survive in the output. That ambiguity is the failure signal. The marker is a plain string literal, so it is stable across esbuild versions in a way that `!1` is not. (I will verify the fold empirically against a real `make build` before relying on it; if esbuild declines to propagate the `const`, I will write the marker against the `__BUILD_DEBUG__` expression directly, which folds at define-substitution time.) ### The check: `script/verify-build` POSIX sh, `#!/bin/sh`, `set -eu`, `ROOT="$(cd "$(dirname "$0")/.." && pwd -P)"`, `main()` at the bottom — matching the existing twelve scripts. It: - resolves the expected mode from its own environment using build.js's exact rule (`AUTISTMASK_DEBUG` equal to the literal `1`), not from anything `build.js` asserts about itself; - **fails** if `dist/constants-bundles.txt` is missing or empty; - **fails** if a listed file does not exist; - for each listed bundle, requires exactly one marker: **both present** fails (not folded / define missing), **neither present** fails (cannot determine), and the wrong one fails (mode mismatch); - additionally scans every other emitted JS file and fails if one that is *not* in the manifest nonetheless carries a marker — that catches a stale or under-generated manifest rather than trusting it. There is no code path in which the script exits 0 without having positively identified the release marker in at least one bundle. ### Where it runs Build path only, per the steer in note 1. `script/check` is not gated on it: doing so would make `check` depend on `dist/` existing, which changes its contract, and would pull a full `make build` into its time budget. Adding a "skip if `dist/` is absent" escape to make it check-safe would reintroduce exactly the silent-pass behaviour note 3 forbids, so that option is out. The required property — CI fails if a release build has a live debug branch — is satisfied by the build path, since `Dockerfile:17` runs a bare `make build` and `script/cibuild` is `docker build .`. `Makefile` gets a `verify-build` shim target, and both `build` and `build-debug` invoke `script/verify-build` after the bundler, with `build-debug` passing `AUTISTMASK_DEBUG=1` through so it asserts the inverse. A bare `AUTISTMASK_DEBUG=1 make build` works too, since make passes the inherited environment down to recipe commands. ### Also - A jest test pinning the source-level invariant that the marker is derived from `DEBUG` (this does not replace the artifact check, it just stops the two drifting in source). - README Entrypoints entry for `script/verify-build`. - Surgical `TODO.md` edit in the same commit; no touching the stale Status/Next Step blocks. ### Demonstrations for the PR Both failure modes shown actually triggering, with output: - (a) delete `__BUILD_DEBUG__` from the `define` map in `build.js`, run `make build`, show it fail; - (b) corrupt an emitted bundle so it carries no marker, run `script/verify-build`, show it fail rather than pass.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No Label
Milestone
No items
No Milestone 1.0.0
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/AutistMask#170
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?