From f7f141a757ac564aaf7c894d747f51738d102dc7 Mon Sep 17 00:00:00 2001 From: clawbot Date: Sun, 9 Aug 2026 16:19:09 +0200 Subject: [PATCH] security: make DEBUG a build-time flag defaulting to off (closes #149) (#169) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-on: https://git.eeqj.de/sneak/AutistMask/pulls/169 Co-authored-by: clawbot Co-committed-by: clawbot --- Makefile | 9 +++- README.md | 30 +++++++++++++ TODO.md | 65 +++++++++++++++++++--------- build.js | 16 +++++++ src/shared/constants.js | 10 ++++- src/shared/wallet.js | 4 ++ tests/wallet.test.js | 94 +++++++++++++++++++++++++++++++++++++++++ 7 files changed, 207 insertions(+), 21 deletions(-) create mode 100644 tests/wallet.test.js diff --git a/Makefile b/Makefile index 7d29268..33ab6d0 100644 --- a/Makefile +++ b/Makefile @@ -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/ diff --git a/README.md b/README.md index 618e757..a4023da 100644 --- a/README.md +++ b/README.md @@ -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 diff --git a/TODO.md b/TODO.md index 5677a1b..b94902f 100644 --- a/TODO.md +++ b/TODO.md @@ -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. diff --git a/build.js b/build.js index 54c1179..d738586 100644 --- a/build.js +++ b/build.js @@ -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), diff --git a/src/shared/constants.js b/src/shared/constants.js index 1280296..7daaaf7 100644 --- a/src/shared/constants.js +++ b/src/shared/constants.js @@ -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"; diff --git a/src/shared/wallet.js b/src/shared/wallet.js index ff29648..8b2dadc 100644 --- a/src/shared/wallet.js +++ b/src/shared/wallet.js @@ -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)), diff --git a/tests/wallet.test.js b/tests/wallet.test.js new file mode 100644 index 0000000..620bd19 --- /dev/null +++ b/tests/wallet.test.js @@ -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, + ); + }); +});