security: make DEBUG a build-time flag defaulting to off (closes #149) #169

clawbot · 2026-08-09T03:56:10+02:00

clawbot commented

2026-08-09 03:56:10 +02:00

Fixes the highest-severity item in the repo: src/shared/constants.js had
const DEBUG = true;, so generateMnemonic() returned the publicly committed
DEBUG_MNEMONIC for every wallet created from a build of main, and the real
entropy path was dead code in every artifact we could produce.

What changed

build.js — AUTISTMASK_DEBUG is read from the environment and injected
as a __BUILD_DEBUG__ entry in the existing esbuild define map, next to the
other __BUILD_*__ defines. Only the exact value 1 enables it; unset, empty,
true, or a typo all yield a release build, so the insecure direction requires
a deliberate opt-in and any mistake fails safe. The build prints
Build mode: release (DEBUG off) or
Build mode: DEBUG (INSECURE - hardcoded test mnemonic, do not ship).

src/shared/constants.js — DEBUG now uses the same typeof guard that
src/shared/buildInfo.js already uses for the other build-time defines, and
defaults to false when the define is absent (jest, plain require).
DEBUG_MNEMONIC stays in the tree and stays exported.

Makefile — new build-debug target (AUTISTMASK_DEBUG=1 + the same
build) so a debug build stays a one-liner for development.

README.md — new "Debug Builds" subsection under Getting Started, and the
DEBUG Mode Policy section now states that DEBUG is build-time-only and spells
out the boundary against the runtime toggle.

tests/wallet.test.js — new, covering both build modes.

TODO.md — refreshed in the same commit (details at the bottom).

No new if (DEBUG) branch was added and nothing about what DEBUG does changed:
still exactly the red banner plus the hardcoded test phrase, per the README
DEBUG Mode Policy and RULES.md:76-80.

The interaction with the #145 settings toggle

This is the subtle part, so spelling out the reasoning.

There are two distinct debug flags in the tree after #145:

the compile-time DEBUG constant from constants.js, and
the runtime debugMode state flag, which the settings easter egg toggles
and which settings.js:379 pushes into log.js via setRuntimeDebug().

log.js merges them: isDebug() is DEBUG || _runtimeDebug. That merged
value feeds exactly two things — the log level threshold (log.js:24) and the
red banner (views/helpers.js:71). Making the banner user-toggleable is the
intended behavior of #145, and this PR leaves it alone.

generateMnemonic() does not consult isDebug(). It reads the
compile-time DEBUG binding directly. That distinction is what makes a release
build coherent: with __BUILD_DEBUG__ false, DEBUG is false in the bundle,
so no amount of clicking the version ten times and flipping the toggle can
reach return DEBUG_MNEMONIC. The user can turn the banner and verbose logging
on in a release build; they cannot turn the hardcoded phrase on.

The failure mode to guard against is someone later "tidying up" the two flags by
routing wallet.js through isDebug(), which would silently reintroduce this
exact vulnerability with the runtime toggle as the trigger. Three things now
guard that: a comment at the wallet.js call site saying it must stay the
compile-time constant and why, the same statement in the README DEBUG Mode
Policy, and a regression test that calls setRuntimeDebug(true), asserts
isDebug() is genuinely true, and then asserts generateMnemonic() still
returns fresh entropy.

I considered instead making the runtime toggle unavailable in release builds,
but rejected it: that removes a feature #145 deliberately added, and it defends
the wrong boundary. The banner is not the dangerous part; the mnemonic path is,
and that one is already unreachable.

Verification

make check — green, 5 suites, 55 tests, plus lint and fmt-check. It also ran
via the pre-commit hook on the commit itself.

The new tests, per the verification standard in the manager comment on the
issue (not just a !== b) — with the flag off: two successive
generateMnemonic() calls differ, both pass isValidMnemonic, both are 12
words, neither equals DEBUG_MNEMONIC, and the result derives a usable HD
wallet (xpub + a well-formed first address), so a broken implementation
returning a counter or a truncated phrase would fail. Same assertions again
with the runtime toggle forced on. With the flag on (__BUILD_DEBUG__ defined
before a jest.resetModules() re-require): DEBUG is true and
generateMnemonic() returns DEBUG_MNEMONIC, so the debug path is proven
working rather than silently deleted.

Build artifacts — make build and make build-debug both produce dist/chrome
and dist/firefox successfully. Grepping the minified bundles for the emitted
DEBUG export value across all four bundles (chrome popup, chrome background,
firefox popup, firefox background):

# after make build
$ grep -roh 'DEBUG:![01]' dist/chrome dist/firefox | sort | uniq -c
      4 DEBUG:!1

# after make build-debug
$ grep -roh 'DEBUG:![01]' dist/chrome dist/firefox | sort | uniq -c
      4 DEBUG:!0

!1 is minified false, !0 is true. Also checked the fail-safe path:
AUTISTMASK_DEBUG=true make build prints Build mode: release (DEBUG off) and
likewise yields 4 DEBUG:!1.

One thing a reviewer should know about the grep: the DEBUG_MNEMONIC string
literal is still present in the release bundle. That is not a leak of anything
(the phrase is in this public repo already) and it does not mean the branch is
live — esbuild cannot tree-shake a CommonJS module.exports object, so the
constant survives while DEBUG folds to false. The compiled function is
function PL(){return ML?UL:f_.fromEntropy(globalThis.crypto.getRandomValues(new Uint8Array(16))).phrase}
where ML is the DEBUG:!1 export. So "the phrase string is absent" is not
the right test for a release build; "the exported DEBUG is !1" is, which is
what I checked.

`TODO.md` refresh

Per the manager comment: Status rewritten (no branch in flight —
feat/issue-144-settings-about landed as #145, scripts-to-rule-them-all landed
as #148, so the scripts/ question is resolved; make check recorded as
verified green on main at 23aeae4); the completed "Verify main passes make
check" Future Step removed; Future Steps rewritten against the #149-#168
backlog in rough priority order, keeping branch pruning (now #167) and the
pre-1.0 security review (noting #149 and #157 are parts of it but it is
broader).

One deliberate deviation to flag rather than bury: the manager asked that Next
Step become this issue. Taken literally against the Workflow section, this
commit completes #149, which would normally move it into Completed Steps. I
followed the repo's existing convention for in-flight work instead — the
previous Next Step was phrased as "Land feat/issue-144-settings-about", so Next
Step is now "Land #149 ... PR open, awaiting review", which is accurate until
this merges. Whoever merges should move it to Completed Steps and promote the
first Future Step. Happy to change it if the reviewer prefers the strict
reading.

Out of scope

script/lint being prettier --check only and unable to catch undefined
identifiers (#152) — noted in the TODO but not fixed here; I greped for DEBUG
consumers by hand rather than relying on lint, as advised. Nothing else in the
DEBUG consumer set (log.js, helpers.js, state.js, settings.js) changed
behavior.

Fixes the highest-severity item in the repo: `src/shared/constants.js` had `const DEBUG = true;`, so `generateMnemonic()` returned the publicly committed `DEBUG_MNEMONIC` for every wallet created from a build of `main`, and the real entropy path was dead code in every artifact we could produce. ## What changed **`build.js`** — `AUTISTMASK_DEBUG` is read from the environment and injected as a `__BUILD_DEBUG__` entry in the existing esbuild `define` map, next to the other `__BUILD_*__` defines. Only the exact value `1` enables it; unset, empty, `true`, or a typo all yield a release build, so the insecure direction requires a deliberate opt-in and any mistake fails safe. The build prints `Build mode: release (DEBUG off)` or `Build mode: DEBUG (INSECURE - hardcoded test mnemonic, do not ship)`. **`src/shared/constants.js`** — `DEBUG` now uses the same `typeof` guard that `src/shared/buildInfo.js` already uses for the other build-time defines, and defaults to `false` when the define is absent (jest, plain `require`). `DEBUG_MNEMONIC` stays in the tree and stays exported. **`Makefile`** — new `build-debug` target (`AUTISTMASK_DEBUG=1` + the same build) so a debug build stays a one-liner for development. **`README.md`** — new "Debug Builds" subsection under Getting Started, and the DEBUG Mode Policy section now states that `DEBUG` is build-time-only and spells out the boundary against the runtime toggle. **`tests/wallet.test.js`** — new, covering both build modes. **`TODO.md`** — refreshed in the same commit (details at the bottom). No new `if (DEBUG)` branch was added and nothing about what DEBUG *does* changed: still exactly the red banner plus the hardcoded test phrase, per the README DEBUG Mode Policy and `RULES.md:76-80`. ## The interaction with the #145 settings toggle This is the subtle part, so spelling out the reasoning. There are two distinct debug flags in the tree after #145: 1. the compile-time `DEBUG` constant from `constants.js`, and 2. the runtime `debugMode` state flag, which the settings easter egg toggles and which `settings.js:379` pushes into `log.js` via `setRuntimeDebug()`. `log.js` merges them: `isDebug()` is `DEBUG || _runtimeDebug`. That merged value feeds exactly two things — the log level threshold (`log.js:24`) and the red banner (`views/helpers.js:71`). Making the banner user-toggleable is the intended behavior of #145, and this PR leaves it alone. `generateMnemonic()` does **not** consult `isDebug()`. It reads the compile-time `DEBUG` binding directly. That distinction is what makes a release build coherent: with `__BUILD_DEBUG__` false, `DEBUG` is false in the bundle, so no amount of clicking the version ten times and flipping the toggle can reach `return DEBUG_MNEMONIC`. The user can turn the banner and verbose logging on in a release build; they cannot turn the hardcoded phrase on. The failure mode to guard against is someone later "tidying up" the two flags by routing `wallet.js` through `isDebug()`, which would silently reintroduce this exact vulnerability with the runtime toggle as the trigger. Three things now guard that: a comment at the `wallet.js` call site saying it must stay the compile-time constant and why, the same statement in the README DEBUG Mode Policy, and a regression test that calls `setRuntimeDebug(true)`, asserts `isDebug()` is genuinely true, and then asserts `generateMnemonic()` still returns fresh entropy. I considered instead making the runtime toggle unavailable in release builds, but rejected it: that removes a feature #145 deliberately added, and it defends the wrong boundary. The banner is not the dangerous part; the mnemonic path is, and that one is already unreachable. ## Verification `make check` — green, 5 suites, 55 tests, plus lint and fmt-check. It also ran via the pre-commit hook on the commit itself. The new tests, per the verification standard in the manager comment on the issue (not just `a !== b`) — with the flag off: two successive `generateMnemonic()` calls differ, both pass `isValidMnemonic`, both are 12 words, neither equals `DEBUG_MNEMONIC`, and the result derives a usable HD wallet (`xpub` + a well-formed first address), so a broken implementation returning a counter or a truncated phrase would fail. Same assertions again with the runtime toggle forced on. With the flag on (`__BUILD_DEBUG__` defined before a `jest.resetModules()` re-require): `DEBUG` is `true` and `generateMnemonic()` returns `DEBUG_MNEMONIC`, so the debug path is proven working rather than silently deleted. Build artifacts — `make build` and `make build-debug` both produce `dist/chrome` and `dist/firefox` successfully. Grepping the minified bundles for the emitted `DEBUG` export value across all four bundles (chrome popup, chrome background, firefox popup, firefox background): # after make build $ grep -roh 'DEBUG:![01]' dist/chrome dist/firefox | sort | uniq -c 4 DEBUG:!1 # after make build-debug $ grep -roh 'DEBUG:![01]' dist/chrome dist/firefox | sort | uniq -c 4 DEBUG:!0 `!1` is minified `false`, `!0` is `true`. Also checked the fail-safe path: `AUTISTMASK_DEBUG=true make build` prints `Build mode: release (DEBUG off)` and likewise yields `4 DEBUG:!1`. One thing a reviewer should know about the grep: the `DEBUG_MNEMONIC` string literal is still present in the release bundle. That is not a leak of anything (the phrase is in this public repo already) and it does not mean the branch is live — esbuild cannot tree-shake a CommonJS `module.exports` object, so the constant survives while `DEBUG` folds to `false`. The compiled function is `function PL(){return ML?UL:f_.fromEntropy(globalThis.crypto.getRandomValues(new Uint8Array(16))).phrase}` where `ML` is the `DEBUG:!1` export. So "the phrase string is absent" is *not* the right test for a release build; "the exported `DEBUG` is `!1`" is, which is what I checked. ## `TODO.md` refresh Per the manager comment: Status rewritten (no branch in flight — `feat/issue-144-settings-about` landed as #145, scripts-to-rule-them-all landed as #148, so the `scripts/` question is resolved; `make check` recorded as verified green on `main` at `23aeae4`); the completed "Verify main passes make check" Future Step removed; Future Steps rewritten against the #149-#168 backlog in rough priority order, keeping branch pruning (now #167) and the pre-1.0 security review (noting #149 and #157 are parts of it but it is broader). One deliberate deviation to flag rather than bury: the manager asked that Next Step become this issue. Taken literally against the Workflow section, this commit *completes* #149, which would normally move it into Completed Steps. I followed the repo's existing convention for in-flight work instead — the previous Next Step was phrased as "Land feat/issue-144-settings-about", so Next Step is now "Land #149 ... PR open, awaiting review", which is accurate until this merges. Whoever merges should move it to Completed Steps and promote the first Future Step. Happy to change it if the reviewer prefers the strict reading. ## Out of scope `script/lint` being `prettier --check` only and unable to catch undefined identifiers (#152) — noted in the TODO but not fixed here; I greped for `DEBUG` consumers by hand rather than relying on lint, as advised. Nothing else in the DEBUG consumer set (`log.js`, `helpers.js`, `state.js`, `settings.js`) changed behavior.

clawbot added 1 commit 2026-08-09 03:56:10 +02:00

security: make DEBUG a build-time flag defaulting to off (closes #149 )

check / check (push) Successful in 29s

Details

acb58856c4

DEBUG was hardcoded to true in src/shared/constants.js, so every wallet
created from a build of main received the publicly committed test recovery
phrase and was instantly drainable. There was no way to produce a non-debug
build at all: the real entropy path in generateMnemonic() was dead code in
every artifact.

DEBUG is now a build-time constant injected by esbuild's define in build.js,
alongside the existing __BUILD_* defines, and read by constants.js with the
same typeof guard buildInfo.js uses. It is false unless the build was run
with AUTISTMASK_DEBUG=1 — an exact match, so an unset, empty or mistyped
value fails safe towards a release build. The build prints which mode it
used, and make build-debug is a shim for the debug case.

What DEBUG does when enabled is unchanged: the red banner plus the hardcoded
test phrase, no new conditionals. Mnemonic generation deliberately keeps
reading the compile-time constant rather than isDebug() from log.js, which
also ORs in the runtime debugMode flag the settings toggle drives; routing it
through isDebug() would let a user of a release build re-enable the known
test phrase for real wallets. That is now recorded at the call site, in the
README DEBUG Mode Policy, and covered by a regression test.

New tests/wallet.test.js covers both build modes: with the flag off, two
successive generateMnemonic() calls differ, both validate as BIP-39 phrases,
both are 12 words, neither is DEBUG_MNEMONIC, and the result still derives a
usable HD wallet — including with the runtime toggle forced on. With the flag
on, DEBUG is true and the test phrase is returned, so the debug path stays
proven rather than silently removed.

Verified with make check (55 tests, lint, fmt-check all green), and with
make build and make build-debug: all four bundles across dist/chrome and
dist/firefox export DEBUG:!1 in a release build and DEBUG:!0 in a debug
build, and AUTISTMASK_DEBUG=true likewise yields DEBUG:!1.

clawbot added the needs-review label 2026-08-09 03:56:14 +02:00

clawbot self-assigned this 2026-08-09 03:56:19 +02:00

clawbot added this to the 1.0.0 milestone 2026-08-09 03:56:20 +02:00

clawbot commented

2026-08-09 03:56:37 +02:00

Ready for review. Summary of what landed on fix/issue-149-debug-build-flag
(single commit acb5885, 7 files, +207/-21):

Built

build.js: AUTISTMASK_DEBUG env var, injected as a __BUILD_DEBUG__
esbuild define beside the existing __BUILD_*__ defines. Exact 1 opts in;
every other value produces a release build. Build mode is printed.
src/shared/constants.js: DEBUG reads the define via the same typeof
guard as src/shared/buildInfo.js, defaulting to false. DEBUG_MNEMONIC
unchanged and still exported.
src/shared/wallet.js: comment only — records that generateMnemonic() must
keep reading the compile-time constant and must never be switched to
isDebug().
Makefile: build-debug target.
README.md: "Debug Builds" section, and DEBUG Mode Policy now documents the
build-time-vs-runtime split.
tests/wallet.test.js: new, 6 tests over both build modes.
TODO.md: refreshed per the manager comment (same commit as the code).

Verified

make check: 5 suites, 55 tests, lint and fmt-check all green. Also ran
through the pre-commit hook on the commit.
make build: dist/chrome and dist/firefox both produced;
grep -roh 'DEBUG:![01]' dist/chrome dist/firefox | sort | uniq -c gives
4 DEBUG:!1 (four bundles: popup and background for each browser).
make build-debug: same four bundles, 4 DEBUG:!0.
AUTISTMASK_DEBUG=true make build: prints Build mode: release (DEBUG off)
and yields 4 DEBUG:!1 — the fail-safe path.

The bit worth reviewing carefully is the runtime toggle from #145. The
settings easter egg drives _runtimeDebug in log.js, and isDebug() is
DEBUG || _runtimeDebug, which feeds the log threshold and the banner only.
generateMnemonic() reads the compile-time DEBUG binding, not isDebug(), so
a release build cannot have the hardcoded phrase re-enabled by any runtime
action. Full reasoning, and why I did not instead disable the toggle in release
builds, is in the PR body. A regression test asserts isDebug() is true and
generateMnemonic() still returns fresh entropy at the same time.

Two things called out in the PR body that a reviewer should not have to
rediscover: (1) the DEBUG_MNEMONIC string literal legitimately survives in the
release bundle because esbuild cannot tree-shake a CommonJS exports object, so
grepping for the phrase is the wrong test — grep the exported DEBUG value;
(2) I set TODO.md Next Step to "land #149, PR open, awaiting review" rather
than moving it straight to Completed Steps, matching how the previous in-flight
Next Step was written; say the word if the strict Workflow reading is preferred.

Ready for review. Summary of what landed on `fix/issue-149-debug-build-flag` (single commit `acb5885`, 7 files, +207/-21): **Built** - `build.js`: `AUTISTMASK_DEBUG` env var, injected as a `__BUILD_DEBUG__` esbuild define beside the existing `__BUILD_*__` defines. Exact `1` opts in; every other value produces a release build. Build mode is printed. - `src/shared/constants.js`: `DEBUG` reads the define via the same `typeof` guard as `src/shared/buildInfo.js`, defaulting to `false`. `DEBUG_MNEMONIC` unchanged and still exported. - `src/shared/wallet.js`: comment only — records that `generateMnemonic()` must keep reading the compile-time constant and must never be switched to `isDebug()`. - `Makefile`: `build-debug` target. - `README.md`: "Debug Builds" section, and DEBUG Mode Policy now documents the build-time-vs-runtime split. - `tests/wallet.test.js`: new, 6 tests over both build modes. - `TODO.md`: refreshed per the manager comment (same commit as the code). **Verified** - `make check`: 5 suites, 55 tests, lint and fmt-check all green. Also ran through the pre-commit hook on the commit. - `make build`: `dist/chrome` and `dist/firefox` both produced; `grep -roh 'DEBUG:![01]' dist/chrome dist/firefox | sort | uniq -c` gives `4 DEBUG:!1` (four bundles: popup and background for each browser). - `make build-debug`: same four bundles, `4 DEBUG:!0`. - `AUTISTMASK_DEBUG=true make build`: prints `Build mode: release (DEBUG off)` and yields `4 DEBUG:!1` — the fail-safe path. **The bit worth reviewing carefully** is the runtime toggle from #145. The settings easter egg drives `_runtimeDebug` in `log.js`, and `isDebug()` is `DEBUG || _runtimeDebug`, which feeds the log threshold and the banner only. `generateMnemonic()` reads the compile-time `DEBUG` binding, not `isDebug()`, so a release build cannot have the hardcoded phrase re-enabled by any runtime action. Full reasoning, and why I did not instead disable the toggle in release builds, is in the PR body. A regression test asserts `isDebug()` is true and `generateMnemonic()` still returns fresh entropy at the same time. Two things called out in the PR body that a reviewer should not have to rediscover: (1) the `DEBUG_MNEMONIC` string literal legitimately survives in the release bundle because esbuild cannot tree-shake a CommonJS exports object, so grepping for the phrase is the wrong test — grep the exported `DEBUG` value; (2) I set `TODO.md` Next Step to "land #149, PR open, awaiting review" rather than moving it straight to Completed Steps, matching how the previous in-flight Next Step was written; say the word if the strict Workflow reading is preferred.

clawbot commented

2026-08-09 04:06:58 +02:00

Review: PR #169 — PASS

Independent adversarial review. Nothing below is taken from the author's
claims; every assertion was re-derived locally against head acb5885 in a
throwaway worktree. No files were modified.

Verdict

PASS. Label merge-ready, assign to sneak. The vulnerability is
genuinely fixed, the runtime-toggle boundary holds, and every DoD item in #149
plus both extra requirements from the manager comment on the issue are met.
Three non-blocking findings are recorded at the bottom; none of them justify
holding the highest-severity fix in the repo.

What I verified myself

1. Vulnerability actually fixed. Confirmed, and confirmed at the level of
the emitted artifact, not the source.

make build, then grep -ro 'DEBUG:![01]' dist/chrome dist/firefox:
DEBUG:!1 in all four bundles that contain constants.js
({chrome,firefox}/src/popup/index.js, {chrome,firefox}/src/background/index.js).
The other four emitted JS files (content/index.js, content/inpage.js)
do not bundle constants.js at all — confirmed by grepping each file
individually for both DEBUG:! and the phrase literal. So "4 of 4", not
"4 of 8 unexplained".
make build-debug: DEBUG:!0 in the same four. The debug path is intact,
not deleted.
Zero occurrences of __BUILD_DEBUG__ remain in any emitted file, so the
esbuild define is fully applied and nothing falls through to the
typeof-guard default in shipped code.

2. The DEBUG_MNEMONIC literal surviving the release bundle — assessed, and
the author's characterisation is correct. I did not take it on trust:

The constants module compiles to oB.exports={DEBUG:!1,DEBUG_MNEMONIC:ok,...}
— a literal false, not a computed value.
wallet.js compiles to
var{Mnemonic:f_,...}=kn(),{DEBUG:ML,DEBUG_MNEMONIC:UL,BIP44_ETH_PATH:l_}=qn();function PL(){return ML?UL:f_.fromEntropy(globalThis.crypto.getRandomValues(new Uint8Array(16))).phrase}
ML is bound once by that destructure and never assigned anywhere in any
bundle — I grepped every bundle for [^A-Za-z_$]ML *=[^=] and for
[A-Za-z_$.]{1,20}\.DEBUG *=; both return nothing. Same result for the
background bundle's corresponding identifier ZD. Because the value is
captured by destructure at module-init time, even mutating the exports
object afterwards could not change it.

So the ternary is genuinely dead at runtime and the surviving string is inert.
Grepping for the phrase is indeed the wrong test; grepping the exported
DEBUG value is the right one. Agreed.

3. The #145 runtime-toggle interaction — the crux. Claim verified, hard. I
traced every consumer rather than reading the argument:

grep -rn 'DEBUG\|isDebug\|setRuntimeDebug\|debugMode' src/ yields the
complete consumer set. isDebug() (src/shared/log.js:19-21, DEBUG || _runtimeDebug)
has exactly two call sites: src/shared/log.js:24 (log threshold) and
src/popup/views/helpers.js:71 (banner). _runtimeDebug is written only by
setRuntimeDebug(), called from src/popup/index.js:209 and
src/popup/views/settings.js:379,385.

DEBUG_MNEMONIC appears in exactly two files: src/shared/constants.js:11 and
src/shared/wallet.js:12. generateMnemonic() is called from exactly one place,
src/popup/views/addWallet.js:286 (the die button). src/shared/wallet.js:5
imports DEBUG from ./constants directly and never imports log.js.

There is no path from the easter egg to the hardcoded phrase. In a release
build a user can turn the banner and verbose logging on and cannot reach
return DEBUG_MNEMONIC. Confirmed in the compiled artifact too, since ML is
the literal-false export and is never reassigned.

The author's rejected alternative (disabling the toggle in release builds) was
the right thing to reject: it would have removed a feature #145 deliberately
added while defending a boundary that is not the dangerous one.

Also confirmed no regression in #145 itself — settings.js, state.js,
helpers.js and log.js are untouched by this diff, and state.debugMode
still defaults to false (src/shared/state.js:32).

4. DoD item "the red banner does not appear" in a release build. Holds by
construction: helpers.js:73 is debug || net.isTestnet, debug is
false || false in a release build with the default state, and the default
network is mainnet (src/shared/networks.js:15, isTestnet: false).

5. Env var fail-safe. Verified empirically, not from the source. Each of
these prints Build mode: release (DEBUG off):
AUTISTMASK_DEBUG=true, AUTISTMASK_DEBUG= (empty), AUTISTMASK_DEBUG=01,
AUTISTMASK_DEBUG=" 1" (leading space), AUTISTMASK_DEBUG=yes. I confirmed
the true case also produces 4 DEBUG:!1 in the artifact, not just the log
line. Only the exact 1 opts in. make build-debug works, and the README's
documented alternative AUTISTMASK_DEBUG=1 make build also works (env
propagates through make to yarn) — verified, both produce DEBUG:!0.
Dockerfile:17 runs a bare make build with no AUTISTMASK_DEBUG ARG or ENV,
so the container build is a release build.

6. Test quality — meets the manager's standard, non-vacuous.
tests/wallet.test.js asserts validity via isValidMnemonic, 12-word count,
inequality of successive calls, inequality against DEBUG_MNEMONIC, HD
derivability of the result, and the debug path still returning
DEBUG_MNEMONIC. The regression test at tests/wallet.test.js:56-66 is the
important one and it is not vacuous: it asserts log.isDebug() is genuinely
true before asserting the phrase is still fresh, so it would not pass
silently if the toggle stopped working.

Against the old broken code (const DEBUG = true) three of the six tests fail:
DEBUG defaults to false, returns fresh, valid 12-word phrases
(at expect(first).not.toBe(second)), and the runtime-toggle regression test.
The suite is a real gate on this bug.

I tried to construct broken implementations that still pass. A fixed-but-valid
phrase fails on not.toBe(second); a counter or truncated phrase fails
isValidMnemonic and the word count; deleting the debug branch fails the
debug-build describe; routing through isDebug() fails the regression test.
The only survivors are low-entropy sources (Math.random instead of
getRandomValues), which no reasonable unit test catches, and the gap noted in
finding 2 below.

7. make check — green, run by me. 5 suites, 55 tests, plus
prettier --check lint and fmt-check, all pass. Run via make check only; no
direct yarn/jest/prettier invocation. CI on head acb5885 is
success ("check / check (push)", 29s). Mergeable against current main:
API reports mergeable: true and git merge-base --is-ancestor origin/main HEAD
confirms the head already contains current main — no rebase needed.

8. Policy. No occurrence of the disallowed vendor names anywhere in the
diff, the commit message, or the PR body; the only repo-wide hits are
pre-existing entries in src/shared/phishingBlocklist.json and a
.prettierignore line, neither touched here. No attribution trailers in
acb5885. Commit subject ends in (closes #149). RULES.md is not in the
diff. No stray files: git diff --name-status shows exactly the 7 intended
paths and git status is clean, so no git add -A sweep. TODO.md is in the
same commit. make fmt-check passes on the markdown. No new if (DEBUG)
branch — wallet.js:12 is the pre-existing one and the only addition there is
a comment, so RULES.md:76-80 and the README DEBUG Mode Policy are respected.
constants.js:1-8 matches buildInfo.js:4-22 exactly in idiom, including the
/* global */ directive. No stutter in isDebugBuild() / __BUILD_DEBUG__ /
build-debug. No non-inclusive terminology introduced. No scope creep: the
README.md and TODO.md edits are both explicitly mandated, by the issue and
by the manager comment respectively.

build-debug shelling straight to yarn run build rather than a script/
entrypoint is consistent with the pre-existing build target and is not a new
divergence; the scripts-to-rule-them-all canonical set in REPO_POLICIES.md is
unaffected. Not a finding.

Non-blocking findings

F1. build.js:18-20 — a set-but-unrecognised AUTISTMASK_DEBUG is silently
ignored.
return process.env.AUTISTMASK_DEBUG === "1"; means AUTISTMASK_DEBUG=true
produces a release build with no diagnostic about the value having been
discarded. Silent defaulting on a set-but-unparseable config value is normally
a rejected pattern. I am not blocking on it, for two specific reasons: the
silent direction is the safe one, and the build prints its resulting mode
unconditionally on every invocation, so no operator can end up unaware of which
artifact they built. Acceptable hardening if the owner wants it: treat unset,
empty, 0 and 1 as valid and process.exit(1) with
AUTISTMASK_DEBUG must be 0 or 1, got "<value>" on anything else — that is
both loud and still fail-safe, since an aborted build ships nothing.

F2. Nothing automatically guards the esbuild wiring itself.
The release-mode tests exercise the typeof __BUILD_DEBUG__ !== "undefined"
fallback in src/shared/constants.js:8, which is the jest path, not the
bundler path. If a future edit dropped __BUILD_DEBUG__ from the define map
at build.js:66, all six tests in tests/wallet.test.js would still pass and
the bundle would still be safe by accident (the fallback is false) — but the
inverse mistake, a define that resolves truthy, is equally untested. For the
single highest-severity failure mode in the repo, an artifact-level assertion
would be worth having: a script/ step after make build that fails unless
every emitted bundle containing DEBUG: shows DEBUG:!1, or a unit test on
build.js's define map. Not required by the DoD and not a defect in this
change; recording it as a follow-up worth filing.

F3. tests/wallet.test.js:65 — log.setRuntimeDebug(false) runs in the test
body rather than an afterEach.
If an earlier assertion in that test throws, the cleanup is skipped. Harmless
today because the beforeEach jest.resetModules() hands the next test a
fresh log module with _runtimeDebug back at false, so there is no real
leak — but the cleanup line is then also load-bearing for nothing and is
misleading. Cosmetic.

Verdict on the flagged TODO.md deviation

The author's phrasing stands; no change required.

The manager comment said Next Step should become this issue, and
TODO.md:20-26 does exactly that. The tension the author identified with the
Workflow section (TODO.md:4-6, "do the work in Next Step / move Next Step to
the top of Completed Steps") is real, but the literal instruction from the
manager is the more specific and more recent one, and the in-flight phrasing
matches the convention the file already used for feat/issue-144-settings-about.
Flagging it rather than burying it was the correct call.

One consequence to be aware of at merge time, since the repo's default merge
style is squash and the merge will not touch TODO.md: the moment this lands,
main will carry a Next Step reading "PR open, awaiting review" for a merged
PR. Whoever merges should move it to Completed Steps and promote #150/#151 into
Next Step, as the author anticipated. That is a merge-time action, not a
rework item.

## Review: PR #169 — PASS Independent adversarial review. Nothing below is taken from the author's claims; every assertion was re-derived locally against head `acb5885` in a throwaway worktree. No files were modified. ### Verdict **PASS.** Label `merge-ready`, assign to `sneak`. The vulnerability is genuinely fixed, the runtime-toggle boundary holds, and every DoD item in #149 plus both extra requirements from the manager comment on the issue are met. Three non-blocking findings are recorded at the bottom; none of them justify holding the highest-severity fix in the repo. ### What I verified myself **1. Vulnerability actually fixed.** Confirmed, and confirmed at the level of the emitted artifact, not the source. - `make build`, then `grep -ro 'DEBUG:![01]' dist/chrome dist/firefox`: `DEBUG:!1` in all four bundles that contain `constants.js` (`{chrome,firefox}/src/popup/index.js`, `{chrome,firefox}/src/background/index.js`). The other four emitted JS files (`content/index.js`, `content/inpage.js`) do not bundle `constants.js` at all — confirmed by grepping each file individually for both `DEBUG:!` and the phrase literal. So "4 of 4", not "4 of 8 unexplained". - `make build-debug`: `DEBUG:!0` in the same four. The debug path is intact, not deleted. - Zero occurrences of `__BUILD_DEBUG__` remain in any emitted file, so the esbuild define is fully applied and nothing falls through to the `typeof`-guard default in shipped code. **2. The `DEBUG_MNEMONIC` literal surviving the release bundle — assessed, and the author's characterisation is correct.** I did not take it on trust: - The constants module compiles to `oB.exports={DEBUG:!1,DEBUG_MNEMONIC:ok,...}` — a literal `false`, not a computed value. - `wallet.js` compiles to `var{Mnemonic:f_,...}=kn(),{DEBUG:ML,DEBUG_MNEMONIC:UL,BIP44_ETH_PATH:l_}=qn();function PL(){return ML?UL:f_.fromEntropy(globalThis.crypto.getRandomValues(new Uint8Array(16))).phrase}` - `ML` is bound once by that destructure and **never assigned anywhere in any bundle** — I grepped every bundle for `[^A-Za-z_$]ML *=[^=]` and for `[A-Za-z_$.]{1,20}\.DEBUG *=`; both return nothing. Same result for the background bundle's corresponding identifier `ZD`. Because the value is captured by destructure at module-init time, even mutating the exports object afterwards could not change it. So the ternary is genuinely dead at runtime and the surviving string is inert. Grepping for the phrase is indeed the wrong test; grepping the exported `DEBUG` value is the right one. Agreed. **3. The #145 runtime-toggle interaction — the crux. Claim verified, hard.** I traced every consumer rather than reading the argument: `grep -rn 'DEBUG\|isDebug\|setRuntimeDebug\|debugMode' src/` yields the complete consumer set. `isDebug()` (`src/shared/log.js:19-21`, `DEBUG || _runtimeDebug`) has exactly two call sites: `src/shared/log.js:24` (log threshold) and `src/popup/views/helpers.js:71` (banner). `_runtimeDebug` is written only by `setRuntimeDebug()`, called from `src/popup/index.js:209` and `src/popup/views/settings.js:379,385`. `DEBUG_MNEMONIC` appears in exactly two files: `src/shared/constants.js:11` and `src/shared/wallet.js:12`. `generateMnemonic()` is called from exactly one place, `src/popup/views/addWallet.js:286` (the die button). `src/shared/wallet.js:5` imports `DEBUG` from `./constants` directly and never imports `log.js`. There is no path from the easter egg to the hardcoded phrase. In a release build a user can turn the banner and verbose logging on and cannot reach `return DEBUG_MNEMONIC`. Confirmed in the compiled artifact too, since `ML` is the literal-`false` export and is never reassigned. The author's rejected alternative (disabling the toggle in release builds) was the right thing to reject: it would have removed a feature #145 deliberately added while defending a boundary that is not the dangerous one. Also confirmed no regression in #145 itself — `settings.js`, `state.js`, `helpers.js` and `log.js` are untouched by this diff, and `state.debugMode` still defaults to `false` (`src/shared/state.js:32`). **4. DoD item "the red banner does not appear" in a release build.** Holds by construction: `helpers.js:73` is `debug || net.isTestnet`, `debug` is `false || false` in a release build with the default state, and the default network is mainnet (`src/shared/networks.js:15`, `isTestnet: false`). **5. Env var fail-safe.** Verified empirically, not from the source. Each of these prints `Build mode: release (DEBUG off)`: `AUTISTMASK_DEBUG=true`, `AUTISTMASK_DEBUG=` (empty), `AUTISTMASK_DEBUG=01`, `AUTISTMASK_DEBUG=" 1"` (leading space), `AUTISTMASK_DEBUG=yes`. I confirmed the `true` case also produces `4 DEBUG:!1` in the artifact, not just the log line. Only the exact `1` opts in. `make build-debug` works, and the README's documented alternative `AUTISTMASK_DEBUG=1 make build` also works (env propagates through make to yarn) — verified, both produce `DEBUG:!0`. `Dockerfile:17` runs a bare `make build` with no `AUTISTMASK_DEBUG` ARG or ENV, so the container build is a release build. **6. Test quality — meets the manager's standard, non-vacuous.** `tests/wallet.test.js` asserts validity via `isValidMnemonic`, 12-word count, inequality of successive calls, inequality against `DEBUG_MNEMONIC`, HD derivability of the result, and the debug path still returning `DEBUG_MNEMONIC`. The regression test at `tests/wallet.test.js:56-66` is the important one and it is not vacuous: it asserts `log.isDebug()` is genuinely `true` before asserting the phrase is still fresh, so it would not pass silently if the toggle stopped working. Against the old broken code (`const DEBUG = true`) three of the six tests fail: `DEBUG defaults to false`, `returns fresh, valid 12-word phrases` (at `expect(first).not.toBe(second)`), and the runtime-toggle regression test. The suite is a real gate on this bug. I tried to construct broken implementations that still pass. A fixed-but-valid phrase fails on `not.toBe(second)`; a counter or truncated phrase fails `isValidMnemonic` and the word count; deleting the debug branch fails the debug-build describe; routing through `isDebug()` fails the regression test. The only survivors are low-entropy sources (`Math.random` instead of `getRandomValues`), which no reasonable unit test catches, and the gap noted in finding 2 below. **7. `make check` — green, run by me.** 5 suites, 55 tests, plus `prettier --check` lint and fmt-check, all pass. Run via `make check` only; no direct yarn/jest/prettier invocation. CI on head `acb5885` is `success` ("check / check (push)", 29s). Mergeable against current `main`: API reports `mergeable: true` and `git merge-base --is-ancestor origin/main HEAD` confirms the head already contains current `main` — no rebase needed. **8. Policy.** No occurrence of the disallowed vendor names anywhere in the diff, the commit message, or the PR body; the only repo-wide hits are pre-existing entries in `src/shared/phishingBlocklist.json` and a `.prettierignore` line, neither touched here. No attribution trailers in `acb5885`. Commit subject ends in ` (closes #149)`. `RULES.md` is not in the diff. No stray files: `git diff --name-status` shows exactly the 7 intended paths and `git status` is clean, so no `git add -A` sweep. `TODO.md` is in the same commit. `make fmt-check` passes on the markdown. No new `if (DEBUG)` branch — `wallet.js:12` is the pre-existing one and the only addition there is a comment, so `RULES.md:76-80` and the README DEBUG Mode Policy are respected. `constants.js:1-8` matches `buildInfo.js:4-22` exactly in idiom, including the `/* global */` directive. No stutter in `isDebugBuild()` / `__BUILD_DEBUG__` / `build-debug`. No non-inclusive terminology introduced. No scope creep: the `README.md` and `TODO.md` edits are both explicitly mandated, by the issue and by the manager comment respectively. `build-debug` shelling straight to `yarn run build` rather than a `script/` entrypoint is consistent with the pre-existing `build` target and is not a new divergence; the scripts-to-rule-them-all canonical set in `REPO_POLICIES.md` is unaffected. Not a finding. ### Non-blocking findings **F1. `build.js:18-20` — a set-but-unrecognised `AUTISTMASK_DEBUG` is silently ignored.** `return process.env.AUTISTMASK_DEBUG === "1";` means `AUTISTMASK_DEBUG=true` produces a release build with no diagnostic about the value having been discarded. Silent defaulting on a set-but-unparseable config value is normally a rejected pattern. I am not blocking on it, for two specific reasons: the silent direction is the *safe* one, and the build prints its resulting mode unconditionally on every invocation, so no operator can end up unaware of which artifact they built. Acceptable hardening if the owner wants it: treat unset, empty, `0` and `1` as valid and `process.exit(1)` with `AUTISTMASK_DEBUG must be 0 or 1, got "<value>"` on anything else — that is both loud and still fail-safe, since an aborted build ships nothing. **F2. Nothing automatically guards the esbuild wiring itself.** The release-mode tests exercise the `typeof __BUILD_DEBUG__ !== "undefined"` fallback in `src/shared/constants.js:8`, which is the jest path, not the bundler path. If a future edit dropped `__BUILD_DEBUG__` from the `define` map at `build.js:66`, all six tests in `tests/wallet.test.js` would still pass and the bundle would still be safe *by accident* (the fallback is `false`) — but the inverse mistake, a define that resolves truthy, is equally untested. For the single highest-severity failure mode in the repo, an artifact-level assertion would be worth having: a `script/` step after `make build` that fails unless every emitted bundle containing `DEBUG:` shows `DEBUG:!1`, or a unit test on `build.js`'s define map. Not required by the DoD and not a defect in this change; recording it as a follow-up worth filing. **F3. `tests/wallet.test.js:65` — `log.setRuntimeDebug(false)` runs in the test body rather than an `afterEach`.** If an earlier assertion in that test throws, the cleanup is skipped. Harmless today because the `beforeEach` `jest.resetModules()` hands the next test a fresh `log` module with `_runtimeDebug` back at `false`, so there is no real leak — but the cleanup line is then also load-bearing for nothing and is misleading. Cosmetic. ### Verdict on the flagged TODO.md deviation The author's phrasing stands; no change required. The manager comment said Next Step should become this issue, and `TODO.md:20-26` does exactly that. The tension the author identified with the Workflow section (`TODO.md:4-6`, "do the work in Next Step / move Next Step to the top of Completed Steps") is real, but the literal instruction from the manager is the more specific and more recent one, and the in-flight phrasing matches the convention the file already used for `feat/issue-144-settings-about`. Flagging it rather than burying it was the correct call. One consequence to be aware of at merge time, since the repo's default merge style is squash and the merge will not touch `TODO.md`: the moment this lands, `main` will carry a Next Step reading "PR open, awaiting review" for a merged PR. Whoever merges should move it to Completed Steps and promote #150/#151 into Next Step, as the author anticipated. That is a merge-time action, not a rework item.

clawbot commented

2026-08-09 04:07:43 +02:00

Manager note (the review verdict is in its own comment above).

Independent adversarial review passed with no blocking findings. The reviewer
did not author this change, and re-derived the critical claims from the built
artifacts rather than accepting the author's word: all four bundles that
actually contain constants.js compile to DEBUG:!1, the surviving
DEBUG_MNEMONIC literal is provably dead (ML is bound once at destructure
and never reassigned), and there is no path from the #145 easter-egg toggle to
the hardcoded phrase — isDebug() has exactly two call sites, neither in
wallet.js. The reviewer also tried to construct broken-but-passing
implementations against the new tests and only low-entropy sources survived.

Marking merge-ready and assigning to @sneak for merge, since main is
protected.

One thing to do at merge time. This PR is a squash merge, so TODO.md will
land carrying "Land #149 … PR open, awaiting review" for a PR that is by then
merged. Whoever merges should move that line to Completed Steps and promote the
next item. I will pick this up in the following work unit if it is easier to
let it ride for one commit — flagging it so it is a choice rather than a
surprise.

Three non-blocking findings were recorded by the reviewer. My dispositions:

Env var AUTISTMASK_DEBUG=true is silently discarded (build.js:18-20).
Not acting. The silent direction is the safe one and the build prints its
resulting mode unconditionally, so it is not actually silent to the operator.
No automated guard on the esbuild wiring — deleting __BUILD_DEBUG__
from build.js would leave all six tests green, because the tests exercise
the jest fallback path, not the bundler path. This is the only durable
defence against this exact vulnerability returning, so I have filed it as
#170 against the 1.0.0 milestone.
setRuntimeDebug(false) in the test body rather than afterEach
(tests/wallet.test.js:65). Cosmetic, harmless given the beforeEach
jest.resetModules(). Not acting.

Manager note (the review verdict is in its own comment above). Independent adversarial review passed with no blocking findings. The reviewer did not author this change, and re-derived the critical claims from the built artifacts rather than accepting the author's word: all four bundles that actually contain `constants.js` compile to `DEBUG:!1`, the surviving `DEBUG_MNEMONIC` literal is provably dead (`ML` is bound once at destructure and never reassigned), and there is no path from the #145 easter-egg toggle to the hardcoded phrase — `isDebug()` has exactly two call sites, neither in `wallet.js`. The reviewer also tried to construct broken-but-passing implementations against the new tests and only low-entropy sources survived. Marking `merge-ready` and assigning to @sneak for merge, since `main` is protected. **One thing to do at merge time.** This PR is a squash merge, so `TODO.md` will land carrying "Land #149 … PR open, awaiting review" for a PR that is by then merged. Whoever merges should move that line to Completed Steps and promote the next item. I will pick this up in the following work unit if it is easier to let it ride for one commit — flagging it so it is a choice rather than a surprise. Three non-blocking findings were recorded by the reviewer. My dispositions: - **Env var `AUTISTMASK_DEBUG=true` is silently discarded** (`build.js:18-20`). Not acting. The silent direction is the safe one and the build prints its resulting mode unconditionally, so it is not actually silent to the operator. - **No automated guard on the esbuild wiring** — deleting `__BUILD_DEBUG__` from `build.js` would leave all six tests green, because the tests exercise the jest fallback path, not the bundler path. This is the only durable defence against this exact vulnerability returning, so I have filed it as #170 against the 1.0.0 milestone. - **`setRuntimeDebug(false)` in the test body rather than `afterEach`** (`tests/wallet.test.js:65`). Cosmetic, harmless given the `beforeEach` `jest.resetModules()`. Not acting.

security: make DEBUG a build-time flag defaulting to off (closes #149) #169

What changed

The interaction with the #145 settings toggle

Verification

TODO.md refresh

Out of scope

Review: PR #169 — PASS

Verdict

What I verified myself

Non-blocking findings

Verdict on the flagged TODO.md deviation

`TODO.md` refresh