fix: one wording for an empty password field on every screen (closes #265) #296
Reference in New Issue
Block a user
Delete Branch "issue-265-empty-password-copy"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #265.
Wording
"Please enter your password."— the majority wording, unchanged on five ofthe six screens. Only
src/popup/views/exportPrivkey.jsmoves, from"Password is required.".Call sites
The issue's list was re-derived from the current
nextrather than trusted;the line numbers had drifted in
approval.jsbut the set is the same six, andnothing was missed:
next)src/popup/views/showPhrase.jsPlease enter your password.src/popup/views/confirmTx.jsPlease enter your password.src/popup/views/deleteWallet.jsPlease enter your password.src/popup/views/approval.jsPlease enter your password.src/popup/views/approval.jsPlease enter your password.src/popup/views/exportPrivkey.jsPassword is required.<- changedsrc/popup/views/addWallet.jsalso rejects an empty password field(
"Please choose a password.") and is deliberately NOT in the set: there thefield is a password being CHOSEN at wallet creation, not the existing one being
entered, which is a different condition. The test anchors on the
decryptWithPasswordcall sites, so that screen is excluded structurallyrather than by an exception list.
Strings only. Every screen still refuses to proceed on an empty field exactly
as before; no control flow, no guard, no adjacent copy touched. The
decryption-failure message from
#172 is untouched.
Test
tests/passwordMessages.test.jsalready pinned the rejected-password sentenceper
decryptWithPasswordcall site. It now pins the empty-field sentence thesame way: each decrypt's password argument is read, walked out to the innermost
enclosing block that declares it (the submit handler), and the single
if (!password)guard in that block must show the canonical sentence andnothing else.
"Password is required."was also added to the whole-filesuperseded-wording sweep.
Six new per-call-site assertions, one per screen.
Failing-first evidence
Two probes, both reverted afterwards; the tree as pushed is green.
exportPrivkey.jsput back to"Password is required.":approval.jssite #2 given a novel wording that is on no superseded list(
"You must type a password."), while site #1 in the same file stayedcanonical:
Exactly one failure, on the diverging call site, in a file whose other call
site is correct — so the check is per call site and not per file, and it fires
on a wording nobody has seen before rather than only on a known-obsolete one.
Layout at 360px — measured
Measured, not reasoned. The built popup (
dist/chrome/src/popup/index.html)loaded in the pinned Playwright container
(
mcr.microsoft.com/playwright@sha256:35246d87..., the digestscript/test-e2epins) at a 360x600 viewport, one view visible at a time asthe popup shows them. For each error container: the container's own height, and
the
topof the element directly below it, in three states — empty andreserved, old string, new string.
The export screen's container is its own element and was measured as such:
zero shift — 20px tall and the password section at the same 241px offset in
all three states. Text lays out at 194.44px inside a 368px content box, so
there is ~173px of headroom before it could wrap. (
bodyiswidth: 396pxinthe stylesheet with
overflow-x: hidden; 368px is that minus the app'sp-2 pr-5. The 360px viewport is the popup window.)Disclosure, from the same run: the two dApp approval containers grow 20px ->
26px the moment they hold ANY text, including the string they already show on
nexttoday.Identical for the old and the new string, so this PR neither causes nor worsens
it — it is the status quo. Cause: those two containers are
border border-dashed p-1 min-h-[1.25rem], and withbox-sizing: border-boxthe reserved 20px is consumed by 2px of border plus 8px of padding, leaving
10px for a 16px line. The four plain containers have no border or padding and
fit exactly. Not fixed here — out of scope for a strings-only change; filed
separately as #297.
Verification
make fmtthenmake check: green. 29 suites, 709 tests,test-verify-build18 cases, prettier clean.
make checkre-run green after rebasing ontonextat51e84ae(TODO.mdconflicted with #285 and
was resolved keeping both entries).
script/cibuildwith the cache invalidated, so thelint and the suite provably executed rather than reporting
CACHED:The measurement probe was a throwaway script and is not part of this diff.
FAIL — one finding.
9931484commit identity.git log -1 --format='%an <%ae> | %cn <%ce>'on the head commit givessneak <sneak@sneak.berlin> | sneak <sneak@sneak.berlin>. Every one of the twelve commits before it onnextisclawbot <clawbot@noreply.example.org>. This is the recurrence #186 records for the third time today, same mechanical cause (identity inherited from the clone), and it is handled as rework there: the head commits of PR 291 and PR 286 are being corrected rather than waived. Acceptable: recommit withuser.name/user.emailset toclawbotbefore push. (The squash rewrites the landed author, so nothing misattributed would reachnext— the defect is on the branch commit only.)Everything else verified and clean, including the two claims worth attacking:
approval.jssite 2 say"You must type a password."fails exactly one test while site 1 in the same file stays green; renaming thedecryptWithPasswordcall out ofexportPrivkey.jsfailsthe call sites are found where they are expected, which pins the membership map, so a vanished screen cannot silently shrink the set; rewritingshowPhrase.js's guard toif (password === "")throwsexpected one empty-password guard, found 0rather than passing on zero guards. The password identifier is read off each call's second argument (pwindeleteWallet.js,passwordelsewhere), so the walk is structural, andaddWallet.jsis excluded because it never decrypts.dist/chrome:#export-privkey-flashisheight=20with#export-privkey-password-sectionattop=241for the empty reserved state, the old string and the new string alike — zero shift.#approve-tx-errorreproduces at 20 -> 26 / 405 -> 411, identical for old and new; markup confirms the stated cause (border border-dashed p-1overmin-h-[1.25rem]underborder-box). Pre-existing, untouched by this PR, correctly deferred to #297 — fixing a container's padding in a strings-only unit would have been scope creep.make checkgreen in a fresh clone (29 suites, 709 tests,test-verify-build18 cases, prettier clean); CI green on9931484; merges cleanly intonext; single commit titled(closes #265);TODO.mdupdated in the same commit with the #285 entry intact alongside the new one; six call sites re-derived and matching, onlyexportPrivkey.js:115changed behaviourally, #172's decryption-failure message untouched; no attribution trailers.Disclosures: one single-suite run went through
npx jestbefore I switched to theyarn/makeentrypoints for everything reported above. The superseded-wording sweep, including the newly added"Password is required.", only walks files that contain a decrypt call, so a straggler in a non-decrypting view would not be caught — pre-existing, not introduced here.9931484bccto514d025c56Fixed the single finding: the head commit's author/committer were
sneak <sneak@sneak.berlin>(identity inherited from the clone) instead ofclawbot. Recommitted withuser.name/user.emailset toclawbot <clawbot@noreply.example.org>via--amend --reset-author; content, message and diff are unchanged. New head:514d025. Rebased onto currentnext(no-op, already current),make checkre-run green (29 suites, 709 tests,test-verify-build18 cases, prettier clean), force-pushed.FAIL — needs-rebase. One blocker, mechanical; the change itself is clean.
TODO.md:47— conflicts with currentnext(c06765e). The branch is based ond9d50f0;e07efb7(#261) andc06765e(#271) have landed since, and #261 inserted its Completed Steps entry at the same position as this one.git mergegivesCONFLICT (content): Merge conflict in TODO.md; the tracker reportsmergeable: false. Matters because it cannot be merged as-is, and a careless resolution drops one of the two entries. Acceptable: rebase onto currentnext, keeping both the #261 and #265 entries (as was already done for #285), and force-push. No other file conflicts.I resolved the conflict locally, keeping both entries, and
make checkon the merged tree is green (30 suites, 743 tests,test-verify-build18 cases, prettier clean) — so the rebase is expected to be purely mechanical.Disclosures:
src/on currentnext, not from the PR body: tentype="password"inputs inindex.html;import-private-keyandimport-xprv-keyare key material,add-wallet-password/add-wallet-password-confirmare a password being chosen at creation (addWallet.jshas its own three messages and never decrypts). The remaining six —export-privkey-password,confirm-tx-password,delete-wallet-password,show-phrase-password,approve-tx-password,approve-sign-password— are exactly the sixdecryptWithPasswordcall sites and are all covered. There is no unlock/lock screen and no change-password screen insrc/popup/views/; every sensitive operation re-prompts. Nothing missed.exportPrivkey.js:115to"Password is required."failspopup/views/exportPrivkey.js #1 answers an empty password field with the canonical sentenceandpopup/views/exportPrivkey.js carries no superseded wording. Separately, givingdeleteWallet.jsa wording on no superseded list ("You must type a password to continue.") fails exactly one test,popup/views/deleteWallet.js #1 ...— so the check is per call site, fires on novel wording, and the identifier walk is structural (that screen's variable ispw, notpassword).make checkon514d025green (29 suites, 709 tests, 18 verify-build cases, prettier clean);script/cibuildexit 0 with#11 [7/8] RUN make checkexecuting UNCACHED (noCACHEDmarker, 18s of layer time). CI green on514d025.RULES.mdline 120, and the test now asserts it.src/diff is one string on one line; no other error message changed.#export-privkey-flash; the pre-existing approval-container growth is correctly deferred to #297.SUPERSEDEDwhole-file sweep only walks files that contain a decrypt call, so the newly added"Password is required."would not be caught in a non-decrypting view.514d025c56to974bbae770Rebased onto current
next(c06765e). New head:974bbae.The rebase pulled in #261 (
e07efb7) and #271 (c06765e).One conflict,
TODO.mdCompleted Steps, where #261 inserted at the same position. Resolved keeping both entries — #265's first, then #261's intact; the #285 and #271 entries below are untouched. No other file conflicted. Substance unchanged:git showof thesrc/andtests/hunks is byte-identical to514d025.Gates on the rebased tree, fresh clone:
make fmtno-op (nothing to reformat), thenmake checkexit 0 — 30 suites, 743 tests,test-verify-build18 cases, prettier clean. Matches the numbers from the review's local resolution.Browser e2e skipped: not load-bearing for a strings-only change, and the host is contended (#287, #290).
Still one commit ending
(closes #265)withTODO.mdin it. Force-pushed with--force-with-lease; the tracker now reportsmergeable: true.974bbae770to3f3bfcb010Correction to the previous comment:
nextmoved to743b196(#259) while this was in flight, so the branch was rebased again. Pushed head is now3f3bfcbon743b196;974bbaeis superseded.That second rebase was conflict-free — #259 inserted its Completed Steps entry at the 2026-08-14 position, not at the top — so the resolution described above is unchanged and both the #261 and #265 entries are intact, with #259's below them.
Gates re-run on
3f3bfcb:make fmtno-op,make checkexit 0 — 30 suites, 743 tests,test-verify-build18 cases, prettier clean.src/andtests/hunks still byte-identical to514d025.mergeable: true.PASS.
git range-diff d9d50f0..514d025 743b196..3f3bfcbdiffers only inTODO.mdcontext, so the reviewed source is byte-identical and the earlier review carries. Squash-merging.