Drives the real popup in a real Firefox with dist/firefox/ installed as an
unpacked MV2 temporary add-on, via geckodriver. Covers popup load, wallet
creation through the UI, and the Add Token screen. Outside make check, like
the Chrome suite.
Zero npm dependencies: tests/e2e/firefox/driver.js is a WebDriver client
over global fetch and child_process against geckodriver's HTTP API. The
Dockerfile pins the node base image, the Firefox 153.0.3 tarball and
geckodriver 0.36.0 by digest.
Errors are read from the privileged nsIConsoleService in Marionette's chrome
context, filtered to non-warning entries whose sourceName is the extension
origin. BiDi log.entryAdded delivers nothing at all for extension pages, so
a Playwright-BiDi or Puppeteer-BiDi harness would see nothing and report
success; the code says so where someone would be tempted to simplify it.
Errors logged during add-on install and background startup are drained and
folded into step 1, never discarded: a throw at the top of
src/background/index.js kills the background page and fails the run.
Content-script capture is left as unverified, because --network none leaves
no http:// page for a content script to be injected into.
Each drain reads the console and clears it in ONE chrome script. Splitting
the read from Services.console.reset() left a window between the two round
trips in which an error was logged into a buffer about to be discarded, and
destroyed unread rather than deferred to the next drain; a probe of 100
sequenced throws at 20ms spacing lost one. With the drain atomic the same
probe accounts for every throw that falls inside the observed window, on two
consecutive runs.
No driver layer is shared with the Chrome suite and the three UI steps are
written twice deliberately: the two backends have no common substrate, and
three steps do not pay for a shim.
Two limits are documented rather than papered over, with measurements rather
than absolutes. Error capture is poll-based, so an error is attributed to a
step and not to a moment within it; the drained window ends ~1.5s after the
last step returns (a 500ms settle, a 1000ms sleep and two drain round trips),
and that cut-off jitters run to run — three runs of throws at fixed offsets
reported everything up to +1.5s and one of the three also reported +1.6s.
Inside the window the atomic drain leaves no race, but nsIConsoleService
keeps a ring buffer of only 250 messages, so more than 250 console messages
between two drains evicts unread errors: 400 throws inside one step report
as exactly the newest 250, on three runs, while occupancy in a clean run
peaks at 4 of 250 at the install drain and 0 at every later drain. Nothing is
stubbed; the container runs with --network none instead, which proves no
request escaped, cannot report which were attempted, and runs only the
failure branches of network-dependent code.
The guard that reports unrecognised POST bodies used batch.every(), which is
vacuously true on an empty array, so a POST with body [] was answered 200 []
and escaped the one mechanism whose job is to make unrecognised outbound
traffic fail the suite rather than pass silently. Unreachable in practice
today, which is exactly the qualifier that stops being true later.
The comment explaining the guard also described a mechanism that does not
exist: playwright-core decodes a binary body lossily rather than returning
null, so such a body reaches the JSON parse as mojibake and is reported by the
catch, while only an absent or empty body decodes to null and is reported by
the type guard. Both are reported; the comment now describes the two real
routes.
ConfirmTx -- the screen that decides what gets signed -- had no automated
coverage of its own behaviour. The arithmetic underneath was well tested; the
wiring was not, so a mutant making the spend gate read the displayed fee
estimate instead of the reserve would have reintroduced the #154 overspend with
the suite still green.
Nine end-to-end tests now drive it for both the native and ERC-20 paths,
covering the pending, funded, over-balance and estimate-failed states, and
asserting that the gate reads the reserve rather than the estimate. Swapping the
two makes the suite fail. The view height is asserted constant across every
state transition rather than merely printed.
Reaching the screen needs a funded balance and a gas estimate, so the route
interception gains fixtures for both. Testing the estimate-failed state means
provoking the console error the code is supposed to emit, which the harness
otherwise fails a run on; an expectation mechanism consumes exactly one matching
record, is scoped to the declaring test, and fails that test if nothing matched,
so it cannot mask an unrelated error.
The dust-threshold field was the only validated input in Settings that rejected
without saying anything: the value silently changed back to the stored one with
no explanation. It now flashes "Please enter a whole number of gwei, zero or
greater." alongside the existing resync, matching the idiom the RPC URL field
already uses.
The parse moves to its own module and accepts plain decimal digits only, zero
or greater. Hex and exponent notation are refused rather than accepted: Number()
reads "0x10" as 16 and "1e3" as 1000, neither of which the previous parseInt
produced, and storing a number the user did not type is the same silent
substitution this change exists to remove.
The message must fit one line of the reserved flash area -- a wrapped message
pushes the settings view down, which the No Layout Shift policy forbids. That is
pinned by an end-to-end test measuring the rendered line height and the position
of the elements below it, in a single round trip because the flash clears after
two seconds.
approvalVerify now compares every field of the signed artifact against the
approval, not a subset. Transaction types are allowlisted to 0/1/2 and any
field the module does not check is refused outright, so a future transaction
type cannot smuggle consequential fields past verification -- an EIP-7702
type-4 artifact that delegates the signer's own EOA while matching every
displayed field was accepted before this change. The serialized bytes handed
to broadcastTransaction are compared against the parsed artifact, so the
guarantee covers the bytes that actually go to the node.
Signing failures in the popup are retryable again. To make that safe, an
approval is claimed synchronously before the first await and every path that
resolves or removes one goes through a single chokepoint that refuses a claimed
approval. Without it, closing the approval window, switching the active address
or a late reject would report "User rejected the request." to the dApp while
the broadcast completed -- the user then redoes the transfer at a fresh nonce
and it sends twice.
Failure copy distinguishes the stage reached, so a user is never told to start
again from the site when the first attempt may already have reached the network.
Address rows on Home gain an [x] control, on wallets that derive addresses from
an extended key and hold more than one, opening a DeleteAddress confirmation
screen.
Removal cannot destroy anything: the key material stays. Derivation indices are
not renumbered, so the next "+" derives the next unused index rather than
resurrecting the removed one. The confirmation states the real route back --
delete the whole wallet in Settings, which asks for the password and destroys
the stored recovery phrase, then import it again -- and notes that the scan
which follows only finds addresses with on-chain activity. The copy varies by
wallet type, since an xprv wallet has no recovery phrase.
Removing an address that holds a balance is allowed, with a warning naming no
figure; the funds are at the address on-chain and stay there either way.
Selection and active address move only when the removed address was the one
selected, and site permissions are dropped for it alone.
The state transition shares its address comparison, permission cleanup and
active-changed broadcast with the wallet-level removal.
build.js records which emitted bundles contain src/shared/constants.js, and
constants.js carries a marker constant-folded from DEBUG itself. script/verify-build
cross-checks the two and fails on every way of not knowing, so deleting the
__BUILD_DEBUG__ define now breaks the build instead of shipping a live debug branch.
The password no longer crosses the extension messaging boundary: the popup
decrypts and signs, and sends only the raw signed transaction or the signature.
The background re-derives the signer from the artifact and checks it against
the approval it holds before broadcasting, so it is not a blind relay.
47 tests over src/shared/transactions.js: both real address-poisoning attacks
as fixtures, each of the four filters on and off, threshold boundaries, no
false positives, and the per-address merge/dedup path. No source file changed.
Runs the real popup in a pinned containerized Chrome and fails on any uncaught
page error or console.error. Also fixes the two defects it caught: the missing
showView import in addToken.js and the missing addressDotHtml import in
transactionDetail.js.
closes#150closes#151
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>
