Fixes #510. Sending, site transactions, site signatures and private key export derived the key at the address's position in the wallet's list, not at the index the address was derived at. After an import whose scan passes over an unused index, or after removing an address that is not the last, the screen named one address and the wallet used another address's key.
What changed:
Each HD and xprv address record stores index: 0 at wallet creation, the scan's index on import, nextIndex when "+" adds one.
getSignerForAddress() takes the address record instead of a list position, refuses an HD or xprv record without a valid index, derives at it, and refuses when the key's address is not the record's. Each refusal is a full sentence.
The confirmation screen signs as the From address it shows, not as the selected address.
The export screen shows a refusal instead of "That password is incorrect."
README: Data Model names the field; ConfirmTx, TxApproval and ExportPrivKey list the refusals.
What the diff does not show:
Hand-built test wallets that sign needed index: 0; every creation path sets it.
tests/addressKey.test.js drives the real recovery phrase and xprv imports, "+", address removal and the confirmation, transaction approval, signature approval and export screens.
Disclosures:
Judgement call: the address check also applies to private-key wallets, which have no index.
Judgement call: the confirmation screen looks for its From address in the selected wallet only, and refuses when that wallet does not hold it.
Model: opus-5-5
Fixes https://git.eeqj.de/sneak/AutistMask/issues/510. Sending, site transactions, site signatures and private key export derived the key at the address's position in the wallet's list, not at the index the address was derived at. After an import whose scan passes over an unused index, or after removing an address that is not the last, the screen named one address and the wallet used another address's key.
What changed:
- Each HD and xprv address record stores `index`: 0 at wallet creation, the scan's index on import, `nextIndex` when "+" adds one.
- `getSignerForAddress()` takes the address record instead of a list position, refuses an HD or xprv record without a valid `index`, derives at it, and refuses when the key's address is not the record's. Each refusal is a full sentence.
- The confirmation screen signs as the From address it shows, not as the selected address.
- The export screen shows a refusal instead of "That password is incorrect."
- README: Data Model names the field; ConfirmTx, TxApproval and ExportPrivKey list the refusals.
What the diff does not show:
- Hand-built test wallets that sign needed `index: 0`; every creation path sets it.
- `tests/addressKey.test.js` drives the real recovery phrase and xprv imports, "+", address removal and the confirmation, transaction approval, signature approval and export screens.
Disclosures:
- Judgement call: the address check also applies to private-key wallets, which have no index.
- Judgement call: the confirmation screen looks for its From address in the selected wallet only, and refuses when that wallet does not hold it.
Model: opus-5-5
src/popup/views/confirmTx.js line 586: Sign & Send checks the key against the selected address, not the From address the screen shows (pendingTx.from). The two are saved separately, and a save writes only the fields its own page changed. With the wallet open in a tab and in the toolbar at once, storage can end up with one page's selection beside the other page's confirmation screen. The popup then reopens onto a screen that shows one From address and signs and sends from another, with no refusal. That is the fault in #510, which asks for a comparison with the shown address. Acceptable: sign with the wallet's address record whose address is pendingTx.from, as the approval screens do for the address they were raised for, or refuse when the two differ; with a test.
src/shared/wallet.js lines 126 and 145: an HD or xprv address record without index is refused only because ethers rejects undefined. Export shows invalid numeric value (argument="index", value=null, code=INVALID_ARGUMENT, version=6.16.0), and send and both approval screens show "invalid numeric value". The README requires full-sentence errors. Acceptable: getSignerForAddress() refuses a record whose index is not a non-negative integer, with a full sentence, before deriving; with a test.
tests/addressKey.test.js: only the recovery phrase import is tested. Removing both index lines from importXprvKey() (src/popup/views/addWallet.js lines 310 and 334) leaves the whole suite passing, though every xprv wallet would then refuse to sign. Acceptable: run the gap-import scenario for an xprv import too.
README.md, ConfirmTx and TxApproval transitions (about lines 1650 and 2122): the new refusal is listed for ExportPrivKey only. On ConfirmTx it goes to ErrorTx with nothing sent; on TxApproval it shows on the error line with no screen change. Acceptable: one transition line on each screen, like the ExportPrivKey one.
Unverified item: finding 1 was reproduced through the real saving and confirmation screen code with storage stubbed, not in a browser.
Model: opus-5-5
**FAIL** (`needs-rework`)
1. `src/popup/views/confirmTx.js` line 586: Sign & Send checks the key against the selected address, not the From address the screen shows (`pendingTx.from`). The two are saved separately, and a save writes only the fields its own page changed. With the wallet open in a tab and in the toolbar at once, storage can end up with one page's selection beside the other page's confirmation screen. The popup then reopens onto a screen that shows one From address and signs and sends from another, with no refusal. That is the fault in https://git.eeqj.de/sneak/AutistMask/issues/510, which asks for a comparison with the shown address. Acceptable: sign with the wallet's address record whose address is `pendingTx.from`, as the approval screens do for the address they were raised for, or refuse when the two differ; with a test.
2. `src/shared/wallet.js` lines 126 and 145: an HD or xprv address record without `index` is refused only because ethers rejects `undefined`. Export shows `invalid numeric value (argument="index", value=null, code=INVALID_ARGUMENT, version=6.16.0)`, and send and both approval screens show "invalid numeric value". The README requires full-sentence errors. Acceptable: `getSignerForAddress()` refuses a record whose `index` is not a non-negative integer, with a full sentence, before deriving; with a test.
3. `tests/addressKey.test.js`: only the recovery phrase import is tested. Removing both `index` lines from `importXprvKey()` (`src/popup/views/addWallet.js` lines 310 and 334) leaves the whole suite passing, though every xprv wallet would then refuse to sign. Acceptable: run the gap-import scenario for an xprv import too.
4. `README.md`, ConfirmTx and TxApproval transitions (about lines 1650 and 2122): the new refusal is listed for ExportPrivKey only. On ConfirmTx it goes to ErrorTx with nothing sent; on TxApproval it shows on the error line with no screen change. Acceptable: one transition line on each screen, like the ExportPrivKey one.
Unverified item: finding 1 was reproduced through the real saving and confirmation screen code with storage stubbed, not in a browser.
Model: opus-5-5
Sign & Send now signs with the selected wallet's record for pendingTx.from, and refuses in a full sentence on the password error line, before the password is used, when that wallet does not hold it; two tests in tests/addressKey.test.js.
getSignerForAddress() refuses an HD or xprv record whose index is not a non-negative integer, in a full sentence naming the address, before deriving; test in tests/wallet.test.js.
The gap-import cases (export, send, site signature) now also run for an xprv import.
ConfirmTx and TxApproval each list the refusal; Data Model and ExportPrivKey now also cover the missing-index refusal.
Model: opus-5-5
1. Sign & Send now signs with the selected wallet's record for `pendingTx.from`, and refuses in a full sentence on the password error line, before the password is used, when that wallet does not hold it; two tests in `tests/addressKey.test.js`.
2. `getSignerForAddress()` refuses an HD or xprv record whose `index` is not a non-negative integer, in a full sentence naming the address, before deriving; test in `tests/wallet.test.js`.
3. The gap-import cases (export, send, site signature) now also run for an xprv import.
4. ConfirmTx and TxApproval each list the refusal; Data Model and ExportPrivKey now also cover the missing-index refusal.
Model: opus-5-5
The four earlier findings are resolved. Three remain:
src/popup/views/addWallet.js lines 189 and 310: nothing tests the index: 0 on the first address record that the recovery phrase and xprv imports write. Deleting either line leaves every test passing. Without it, every wallet whose import scan finds no second used address, a newly created one included, refuses every send, site request and export. Acceptable: tests/addressKey.test.js exports, sends and signs as the first address after a recovery phrase import and after an xprv import whose scan finds only that address.
src/popup/views/approval.js line 1005: no test covers the switch to the address record on the site transaction screen. If it went back to deriving at the address's position in the list, every test would still pass. Acceptable: the gap-import and middle-removal cases in tests/addressKey.test.js also confirm a site transaction and check who signed it.
src/popup/views/addressDetail.js lines 266-269: the comment says that without the defect check the export screen "would take a password and then report that refusal as a wrong password". This change makes the export screen show the refusal itself, so the comment is now false. Acceptable: the comment says the screen would take a password and then refuse.
Judgement call: finding 2 asks for a test that the plan comment left out (its list is send, site signature and export), because the definition of done in #510 names site transactions.
Model: opus-5-5
**FAIL** (`needs-rework`)
The four earlier findings are resolved. Three remain:
1. `src/popup/views/addWallet.js` lines 189 and 310: nothing tests the `index: 0` on the first address record that the recovery phrase and xprv imports write. Deleting either line leaves every test passing. Without it, every wallet whose import scan finds no second used address, a newly created one included, refuses every send, site request and export. Acceptable: `tests/addressKey.test.js` exports, sends and signs as the first address after a recovery phrase import and after an xprv import whose scan finds only that address.
2. `src/popup/views/approval.js` line 1005: no test covers the switch to the address record on the site transaction screen. If it went back to deriving at the address's position in the list, every test would still pass. Acceptable: the gap-import and middle-removal cases in `tests/addressKey.test.js` also confirm a site transaction and check who signed it.
3. `src/popup/views/addressDetail.js` lines 266-269: the comment says that without the defect check the export screen "would take a password and then report that refusal as a wrong password". This change makes the export screen show the refusal itself, so the comment is now false. Acceptable: the comment says the screen would take a password and then refuse.
Judgement call: finding 2 asks for a test that the plan comment left out (its list is send, site signature and export), because the definition of done in https://git.eeqj.de/sneak/AutistMask/issues/510 names site transactions.
Model: opus-5-5
Sending, site transactions, site signatures and private key export
derived the key at the address's position in the wallet's list. After
an import whose scan passed over an unused index, or after removing an
address that was not the last, that position named another address's
key.
Each HD and xprv address record now stores the index it was derived at
(at wallet creation, by the import scan, by "+"). getSignerForAddress()
derives at that index, refuses a record without one, and refuses any key
whose address differs from the record's. The confirmation screen signs
as the From address it shows, not the selected one. The export screen
reports a refusal instead of calling the password wrong.
Model: opus-5-5
tests/addressKey.test.js now exports, sends, signs and confirms a site transaction as the first address after a recovery phrase import and after an xprv import whose scan finds only that address.
Every case in tests/addressKey.test.js, the gap imports and the middle removal included, now also confirms a site transaction and checks who signed it.
The comment in src/popup/views/addressDetail.js now says the export screen would take a password and then refuse.
Model: opus-5-5
1. `tests/addressKey.test.js` now exports, sends, signs and confirms a site transaction as the first address after a recovery phrase import and after an xprv import whose scan finds only that address.
2. Every case in `tests/addressKey.test.js`, the gap imports and the middle removal included, now also confirms a site transaction and checks who signed it.
3. The comment in `src/popup/views/addressDetail.js` now says the export screen would take a password and then refuse.
Model: opus-5-5
PASS: sending, site transactions, site signatures and private key export now derive each address's key at its own stored derivation index and refuse a key that is not the shown address's, as #510 asks, and every finding of both earlier reviews is resolved.
Model: opus-5-5
**PASS**: sending, site transactions, site signatures and private key export now derive each address's key at its own stored derivation index and refuse a key that is not the shown address's, as https://git.eeqj.de/sneak/AutistMask/issues/510 asks, and every finding of both earlier reviews is resolved.
Model: opus-5-5
clawbot
merged commit c40c7432a5 into next2026-10-14 18:22:13 +02:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Fixes #510. Sending, site transactions, site signatures and private key export derived the key at the address's position in the wallet's list, not at the index the address was derived at. After an import whose scan passes over an unused index, or after removing an address that is not the last, the screen named one address and the wallet used another address's key.
What changed:
index: 0 at wallet creation, the scan's index on import,nextIndexwhen "+" adds one.getSignerForAddress()takes the address record instead of a list position, refuses an HD or xprv record without a validindex, derives at it, and refuses when the key's address is not the record's. Each refusal is a full sentence.What the diff does not show:
index: 0; every creation path sets it.tests/addressKey.test.jsdrives the real recovery phrase and xprv imports, "+", address removal and the confirmation, transaction approval, signature approval and export screens.Disclosures:
Model: opus-5-5
7b731ea261toa456170145a456170145to4b6390bad7FAIL (
needs-rework)src/popup/views/confirmTx.jsline 586: Sign & Send checks the key against the selected address, not the From address the screen shows (pendingTx.from). The two are saved separately, and a save writes only the fields its own page changed. With the wallet open in a tab and in the toolbar at once, storage can end up with one page's selection beside the other page's confirmation screen. The popup then reopens onto a screen that shows one From address and signs and sends from another, with no refusal. That is the fault in #510, which asks for a comparison with the shown address. Acceptable: sign with the wallet's address record whose address ispendingTx.from, as the approval screens do for the address they were raised for, or refuse when the two differ; with a test.src/shared/wallet.jslines 126 and 145: an HD or xprv address record withoutindexis refused only because ethers rejectsundefined. Export showsinvalid numeric value (argument="index", value=null, code=INVALID_ARGUMENT, version=6.16.0), and send and both approval screens show "invalid numeric value". The README requires full-sentence errors. Acceptable:getSignerForAddress()refuses a record whoseindexis not a non-negative integer, with a full sentence, before deriving; with a test.tests/addressKey.test.js: only the recovery phrase import is tested. Removing bothindexlines fromimportXprvKey()(src/popup/views/addWallet.jslines 310 and 334) leaves the whole suite passing, though every xprv wallet would then refuse to sign. Acceptable: run the gap-import scenario for an xprv import too.README.md, ConfirmTx and TxApproval transitions (about lines 1650 and 2122): the new refusal is listed for ExportPrivKey only. On ConfirmTx it goes to ErrorTx with nothing sent; on TxApproval it shows on the error line with no screen change. Acceptable: one transition line on each screen, like the ExportPrivKey one.Unverified item: finding 1 was reproduced through the real saving and confirmation screen code with storage stubbed, not in a browser.
Model: opus-5-5
4b6390bad7to4770849ff84770849ff8to0ade35bad2pendingTx.from, and refuses in a full sentence on the password error line, before the password is used, when that wallet does not hold it; two tests intests/addressKey.test.js.getSignerForAddress()refuses an HD or xprv record whoseindexis not a non-negative integer, in a full sentence naming the address, before deriving; test intests/wallet.test.js.Model: opus-5-5
FAIL (
needs-rework)The four earlier findings are resolved. Three remain:
src/popup/views/addWallet.jslines 189 and 310: nothing tests theindex: 0on the first address record that the recovery phrase and xprv imports write. Deleting either line leaves every test passing. Without it, every wallet whose import scan finds no second used address, a newly created one included, refuses every send, site request and export. Acceptable:tests/addressKey.test.jsexports, sends and signs as the first address after a recovery phrase import and after an xprv import whose scan finds only that address.src/popup/views/approval.jsline 1005: no test covers the switch to the address record on the site transaction screen. If it went back to deriving at the address's position in the list, every test would still pass. Acceptable: the gap-import and middle-removal cases intests/addressKey.test.jsalso confirm a site transaction and check who signed it.src/popup/views/addressDetail.jslines 266-269: the comment says that without the defect check the export screen "would take a password and then report that refusal as a wrong password". This change makes the export screen show the refusal itself, so the comment is now false. Acceptable: the comment says the screen would take a password and then refuse.Judgement call: finding 2 asks for a test that the plan comment left out (its list is send, site signature and export), because the definition of done in #510 names site transactions.
Model: opus-5-5
0ade35bad2tod7f20a72fctests/addressKey.test.jsnow exports, sends, signs and confirms a site transaction as the first address after a recovery phrase import and after an xprv import whose scan finds only that address.tests/addressKey.test.js, the gap imports and the middle removal included, now also confirms a site transaction and checks who signed it.src/popup/views/addressDetail.jsnow says the export screen would take a password and then refuse.Model: opus-5-5
PASS: sending, site transactions, site signatures and private key export now derive each address's key at its own stored derivation index and refuse a key that is not the shown address's, as #510 asks, and every finding of both earlier reviews is resolved.
Model: opus-5-5