security: make DEBUG a build-time flag defaulting to off (closes #149) (#169)
Some checks failed
check / check (push) Has been cancelled

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.

Co-authored-by: sneak <sneak@sneak.berlin>
Reviewed-on: #169
Co-authored-by: clawbot <clawbot@noreply.example.org>
Co-committed-by: clawbot <clawbot@noreply.example.org>
This commit was merged in pull request #169.
This commit is contained in:
2026-08-09 16:19:09 +02:00
committed by Jeffrey Paul
parent 23aeae4841
commit f7f141a757
7 changed files with 207 additions and 21 deletions

View File

@@ -1,4 +1,4 @@
.PHONY: bootstrap setup install test lint fmt fmt-check check docker hooks build clean dev
.PHONY: bootstrap setup install test lint fmt fmt-check check docker hooks build build-debug clean dev
# Standard targets are thin shims; the implementations live in script/
# per the scripts-to-rule-them-all pattern (see the Entrypoints section
@@ -38,6 +38,13 @@ build:
@echo "Building extension..."
@yarn run build 2>&1
# Development-only build: enables the red DEBUG / INSECURE banner and makes
# the hardcoded test recovery phrase the output of wallet creation. Never
# distribute the artifacts this produces.
build-debug:
@echo "Building extension (DEBUG)..."
@AUTISTMASK_DEBUG=1 yarn run build 2>&1
clean:
@rm -rf dist/

View File

@@ -42,6 +42,23 @@ Load the extension:
- **Firefox**: Navigate to `about:debugging#/runtime/this-firefox`, click "Load
Temporary Add-on", and select `dist/firefox/manifest.json`.
### Debug Builds
`make build` always produces a release build: the build-time `DEBUG` constant is
`false`, so wallet creation uses real entropy and the red banner is off. To
produce a debug build instead, set `AUTISTMASK_DEBUG=1` in the environment:
```bash
make build-debug # or: AUTISTMASK_DEBUG=1 make build
```
Only the exact value `1` enables it; any other value (including unset, empty, or
`true`) yields a release build, so a typo cannot accidentally ship the debug
behavior. The build prints which mode it used. See the
[DEBUG Mode Policy](#debug-mode-policy) for what the flag changes. **Never
distribute a debug build** — every wallet it creates gets the same publicly
known test recovery phrase.
## Entrypoints
This repository adheres to the
@@ -668,6 +685,19 @@ flows, or alter program behavior beyond the banner and the hardcoded mnemonic.
Adding new DEBUG-conditional branches requires explicit approval from the
project owner.
`DEBUG` is a build-time constant, not a runtime setting. `build.js` injects it
into the bundle as the `__BUILD_DEBUG__` define — `false` unless the build was
run with `AUTISTMASK_DEBUG=1` (see [Debug Builds](#debug-builds)) — and
`src/shared/constants.js` reads it. It cannot be changed after the bundle is
produced.
The debug-mode toggle in settings is a separate, runtime-only flag. It raises
the log level and turns the banner on, and that is all it may ever do: it feeds
`isDebug()` in `src/shared/log.js`, which is deliberately not what
`generateMnemonic()` consults. Mnemonic generation reads the build-time `DEBUG`
constant directly, so no runtime toggle in a release build can reach the
hardcoded test phrase.
### Key Decisions
- **No framework**: The popup UI is vanilla JS and HTML. The extension is small

65
TODO.md
View File

@@ -10,24 +10,31 @@
# Status
pre-1.0. Tagged v0.1.0 on 2026-02-27. Active development on branch
feat/issue-144-settings-about (another agent working as of 2026-07-06). Full
policy file set present; make check on main not verified.
pre-1.0, working towards the 1.0.0 milestone. Tagged v0.1.0 on 2026-02-27. No
other branch is in flight: the settings About well landed as #145 on 2026-07-26
and scripts-to-rule-them-all landed as #148, so the `scripts/` directory
question is resolved. Full policy file set present. `make check` verified
passing on `main` at `23aeae4` on 2026-08-09. The 1.0.0 backlog is filed as
#149-#168.
# Next Step
Land feat/issue-144-settings-about: finish the settings About well (build info,
app name and repo link, release date, version click easter egg, git info derived
inside Docker), resolve the untracked scripts/ directory (commit or gitignore),
get review, merge to main.
Land #149: make `DEBUG` a build-time constant that defaults to off, injected as
the `__BUILD_DEBUG__` esbuild define from `AUTISTMASK_DEBUG=1`, so a plain
`make build` stops handing every newly created wallet the publicly committed
test recovery phrase. Branch `fix/issue-149-debug-build-flag`; PR open, awaiting
review.
# Completed Steps
- 2026-08-09: Reviewed the repo end to end and filed the 1.0.0 backlog
(#149-#168).
- 2026-07-26: About well in settings with build info, repo link and the version
click easter egg (#145); proper view navigation stack (#146).
- 2026-07-07 Adopted scripts-to-rule-them-all: `script/` entrypoints, Makefile
shims, README Entrypoints section
- 2026-03-01: About well in settings with build info and easter egg (in flight
on feature branch); USD display suppressed on testnets (#142); estimated USD
for ETH in approve-tx view (#141).
shims, README Entrypoints section (#148)
- 2026-03-01: USD display suppressed on testnets (#142); estimated USD for ETH
in approve-tx view (#141).
- Sepolia testnet support (#137); etherscan links go to token-specific URLs
(#136).
- Transaction detail improvements: Type field and on-chain details (#130),
@@ -45,12 +52,32 @@ get review, merge to main.
# Future Steps
- Verify main passes make check after the feature branch merges (not verified
2026-07-06 because an agent was active in the tree); fix anything red. main
must always be green.
- Prune stale branches: dozens of merged local and remote feature branches
remain (fix/_, feature/_, tx-\*); delete merged ones locally and on origin.
- Continue the issue backlog toward a feature-complete wallet, then cut further
tags as milestones land.
- Fix the two `ReferenceError` crashes that make whole screens unreachable:
AddToken (#150) and TransactionDetail for every ERC-20 transfer (#151).
- Add ESLint to `script/lint` (#152). `make check` is `prettier --check` only
and cannot catch undefined identifiers, which is how #150 and #151 shipped.
- Make the Firefox target functional: Chrome callback APIs are used against the
promise-only `browser` namespace (#153).
- Send and transaction-flow correctness: gas fee excluded from the
insufficient-balance check (#154), WaitTx 60s timeout overwriting a rendered
success screen (#155), last-wallet deletion leaving inconsistent state (#156).
- Security: plaintext password crossing the extension messaging boundary during
dApp approvals (#157); MV3 service worker termination killing the background
refresh and the 24h phishing list update (#158).
- Test the crypto core — `wallet.js` derivation and `vault.js` encryption (#159)
— and the address-poisoning defense in `transactions.js` (#160).
- Wallet features for 1.0: show a wallet's recovery phrase behind the password
(#161), delete an address from an HD wallet (#162).
- Docs: `docs/README.md` contradicts the code on external services and names
competitors (#163); README Screen Map omits three shipped screens (#164).
- Owner decisions: Sepolia support versus "Non-Goals for 1.0", and `isMetaMask`
naming a competitor in shipped code (#165).
- Repo policy compliance sweep: test rerun pattern, `yarn`/`npx`, frozen
lockfile, undocumented Makefile targets (#166).
- Prune the 24 stale remote feature branches (#167).
- Remove dead exports and de-duplicate copy-pasted view helpers (#168).
- Pre-1.0 security review of the extension (key handling, DEBUG mode policy, RPC
input validation) before any 1.0rc tag.
input validation) before any 1.0rc tag; #149 and #157 are parts of it, but the
review is broader than either.
- Cut 1.0.0 once the milestone is empty, then continue tagging as milestones
land.

View File

@@ -11,6 +11,14 @@ function ensureDir(dir) {
fs.mkdirSync(dir, { recursive: true });
}
// DEBUG is a build-time flag, off unless explicitly requested. It is the only
// thing that makes the hardcoded test mnemonic reachable, so the opt-in must be
// exact: anything other than the literal "1" (unset, empty, "true", a typo)
// produces a release build. Failing towards the safe mode is deliberate.
function isDebugBuild() {
return process.env.AUTISTMASK_DEBUG === "1";
}
function getBuildInfo() {
const pkg = JSON.parse(
fs.readFileSync(path.join(__dirname, "package.json"), "utf8"),
@@ -47,7 +55,15 @@ async function build() {
const buildInfo = getBuildInfo();
console.log("Build info:", buildInfo);
const debugBuild = isDebugBuild();
console.log(
debugBuild
? "Build mode: DEBUG (INSECURE - hardcoded test mnemonic, do not ship)"
: "Build mode: release (DEBUG off)",
);
const define = {
__BUILD_DEBUG__: JSON.stringify(debugBuild),
__BUILD_VERSION__: JSON.stringify(buildInfo.version),
__BUILD_LICENSE__: JSON.stringify(buildInfo.license),
__BUILD_AUTHOR__: JSON.stringify(buildInfo.author),

View File

@@ -1,4 +1,12 @@
const DEBUG = true;
// DEBUG is a build-time constant injected by esbuild's define in build.js
// (see src/shared/buildInfo.js for the same pattern). It is false unless the
// bundle was produced with AUTISTMASK_DEBUG=1, and it is false whenever the
// module is loaded outside a bundle (tests, plain require). It must never be
// derived from anything the user can change at runtime: it is what gates the
// hardcoded test mnemonic below.
/* global __BUILD_DEBUG__ */
const DEBUG = typeof __BUILD_DEBUG__ !== "undefined" ? __BUILD_DEBUG__ : false;
const DEBUG_MNEMONIC =
"cube evolve unfold result inch risk jealous skill hotel bulb night wreck";

View File

@@ -5,6 +5,10 @@ const { Mnemonic, HDNodeWallet, Wallet } = require("ethers");
const { DEBUG, DEBUG_MNEMONIC, BIP44_ETH_PATH } = require("./constants");
function generateMnemonic() {
// This must stay the compile-time DEBUG constant. Do NOT switch it to
// isDebug() from log.js: that also ORs in the runtime debugMode flag the
// settings toggle drives, which would let a user of a release build turn
// the hardcoded, publicly known test phrase back on for real wallets.
if (DEBUG) return DEBUG_MNEMONIC;
const m = Mnemonic.fromEntropy(
globalThis.crypto.getRandomValues(new Uint8Array(16)),

94
tests/wallet.test.js Normal file
View File

@@ -0,0 +1,94 @@
// Tests for the DEBUG build flag as it gates mnemonic generation.
//
// The modules read the __BUILD_DEBUG__ global that esbuild replaces at bundle
// time. Under jest the global is absent, which is exactly the release-build
// case; the debug-build case is exercised by defining the global and
// re-requiring the modules with a fresh registry.
const WORDS_IN_12_WORD_PHRASE = 12;
function loadWallet() {
const constants = require("../src/shared/constants");
const wallet = require("../src/shared/wallet");
const log = require("../src/shared/log");
return { constants, wallet, log };
}
describe("generateMnemonic in a release build", () => {
beforeEach(() => {
jest.resetModules();
delete globalThis.__BUILD_DEBUG__;
});
test("DEBUG defaults to false when the build define is absent", () => {
const { constants } = loadWallet();
expect(constants.DEBUG).toBe(false);
});
test("returns fresh, valid 12-word phrases that are not the test phrase", () => {
const { constants, wallet } = loadWallet();
const first = wallet.generateMnemonic();
const second = wallet.generateMnemonic();
expect(first).not.toBe(second);
for (const phrase of [first, second]) {
expect(wallet.isValidMnemonic(phrase)).toBe(true);
expect(phrase.split(" ")).toHaveLength(WORDS_IN_12_WORD_PHRASE);
expect(phrase).not.toBe(constants.DEBUG_MNEMONIC);
}
});
test("derives a usable HD wallet from the generated phrase", () => {
const { wallet } = loadWallet();
const { xpub, firstAddress } = wallet.hdWalletFromMnemonic(
wallet.generateMnemonic(),
);
expect(xpub.startsWith("xpub")).toBe(true);
expect(firstAddress).toMatch(/^0x[0-9a-fA-F]{40}$/);
});
test("the runtime debug toggle cannot re-enable the test phrase", () => {
const { constants, wallet, log } = loadWallet();
// What the settings easter-egg toggle does at runtime.
log.setRuntimeDebug(true);
expect(log.isDebug()).toBe(true);
const phrase = wallet.generateMnemonic();
expect(phrase).not.toBe(constants.DEBUG_MNEMONIC);
expect(wallet.isValidMnemonic(phrase)).toBe(true);
expect(phrase).not.toBe(wallet.generateMnemonic());
log.setRuntimeDebug(false);
});
});
describe("generateMnemonic in a debug build", () => {
beforeEach(() => {
jest.resetModules();
globalThis.__BUILD_DEBUG__ = true;
});
afterEach(() => {
delete globalThis.__BUILD_DEBUG__;
});
test("DEBUG is true and the test phrase is returned", () => {
const { constants, wallet } = loadWallet();
expect(constants.DEBUG).toBe(true);
expect(wallet.generateMnemonic()).toBe(constants.DEBUG_MNEMONIC);
});
test("the test phrase is itself a valid 12-word BIP-39 phrase", () => {
const { constants, wallet } = loadWallet();
expect(wallet.isValidMnemonic(constants.DEBUG_MNEMONIC)).toBe(true);
expect(constants.DEBUG_MNEMONIC.split(" ")).toHaveLength(
WORDS_IN_12_WORD_PHRASE,
);
});
});