fix: merge site permissions and network endpoints structurally, and give the identity merge a collision floor (closes #304)
The whole-field carve-out for allowedSites/deniedSites was false: src/background/index.js pushes an approved/denied hostname onto them in place, and the Settings revoke button filters one out in place from a different page — the exact membership-vs-leaf pattern that made a whole-field wallets diff unsafe, on a security-relevant field. A stale page's save could resurrect a just-revoked permission or wipe one just granted elsewhere. Both are now merged by address key and then by hostname (mergeSiteMap()), the same way wallets merge by identity. networkEndpoints gets the same per-key treatment for its lesser, non-security version of the same race. tokenHolderCache stays whole-field, correctly this time: nothing in src/ ever writes an entry into it. mergeListByIdentity() also had no floor of its own: two wallets sharing walletIdentity()'s empty-fallback identity collapsed into one via a Map, and mergeWallet() discarded the losing side's encryptedSecret outright when there was no shared baseline to diff against. Not reachable from today's UI, but the merge should not rely solely on call-site discipline elsewhere. A same-identity collision within `ours`, or between an unmatched `theirs` and a colliding `ours`, is now detected and both records are kept rather than one silently dropped. New tests in tests/stateMerge.test.js, confirmed failing against the prior state.js (stashed the fix, reran full suite: 3 red, 832 green; restored, all 835 green): - a dApp approval survives a stale Settings page revoking an unrelated site - a revoked site permission stays revoked against a stale page's later save - two independently created wallets with a colliding identity both survive, encryptedSecret included make check: 835/835 tests, test-verify-build 39/39, check-censored clean, lint stage ran fresh in the pinned container (not CACHED), prettier clean. No containers left running.
This commit is contained in:
@@ -289,3 +289,155 @@ describe("background refresh racing a wallet deleted on another page", () => {
|
||||
expect(persisted.wallets[0].addresses[0].balance).toBe("1.2345");
|
||||
});
|
||||
});
|
||||
|
||||
// allowedSites/deniedSites: { [address]: [hostname, ...] }. Mutated in place
|
||||
// from two different contexts — src/background/index.js:592-599 pushes a
|
||||
// newly approved hostname onto state.allowedSites[activeAddress], and the
|
||||
// Settings "revoke" button (src/popup/views/settings.js:55-68) filters a
|
||||
// hostname out of state[key][addr] in place, deleting the address key
|
||||
// entirely once its list is empty — the exact membership-vs-whole-field
|
||||
// pattern that made the whole-field `wallets` diff unsafe, on a
|
||||
// security-relevant field: a stale whole-field save here can resurrect a
|
||||
// revoked permission or wipe a freshly granted one.
|
||||
const ADDR1 = "0x66133E8ea0f5D1d612D2502a968757D1048c214a";
|
||||
const ADDR2 = "0xdAC17F958D2ee523a2206206994597C13D831ec7";
|
||||
|
||||
function approveSite(pageState, address, hostname) {
|
||||
if (!pageState.allowedSites[address]) {
|
||||
pageState.allowedSites[address] = [];
|
||||
}
|
||||
if (!pageState.allowedSites[address].includes(hostname)) {
|
||||
pageState.allowedSites[address].push(hostname);
|
||||
}
|
||||
}
|
||||
|
||||
function revokeSite(pageState, hostname) {
|
||||
for (const addr of Object.keys(pageState.allowedSites)) {
|
||||
pageState.allowedSites[addr] = pageState.allowedSites[addr].filter(
|
||||
(h) => h !== hostname,
|
||||
);
|
||||
if (pageState.allowedSites[addr].length === 0) {
|
||||
delete pageState.allowedSites[addr];
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
describe("a dApp approval racing a stale Settings page's later save", () => {
|
||||
test("the fresh approval survives Settings revoking an unrelated site", async () => {
|
||||
const storage = makeStorage();
|
||||
await storage.set({
|
||||
autistmask: {
|
||||
wallets: [W1],
|
||||
allowedSites: { [ADDR2]: ["other.example"] },
|
||||
},
|
||||
});
|
||||
|
||||
// Settings loads first, and its save lands last — before either has
|
||||
// any idea a dApp approval happened elsewhere in between.
|
||||
const settings = loadPage(storage);
|
||||
await settings.state.loadState();
|
||||
|
||||
// A dApp approval window, opened later, approves a new site for a
|
||||
// different address and saves — the real sequence at
|
||||
// src/background/index.js:592-599.
|
||||
const approval = loadPage(storage);
|
||||
await approval.state.loadState();
|
||||
approveSite(approval.state.state, ADDR1, "dapp.example");
|
||||
await approval.state.saveState();
|
||||
expect(
|
||||
(await storage.get("autistmask")).autistmask.allowedSites[ADDR1],
|
||||
).toEqual(["dapp.example"]);
|
||||
|
||||
// Settings revokes its own, unrelated site — the real sequence at
|
||||
// src/popup/views/settings.js:55-68 — and saves from state loaded
|
||||
// before the dApp approval ever happened.
|
||||
revokeSite(settings.state.state, "other.example");
|
||||
await settings.state.saveState();
|
||||
|
||||
const persisted = (await storage.get("autistmask")).autistmask;
|
||||
expect(persisted.allowedSites[ADDR1]).toEqual(["dapp.example"]);
|
||||
expect(persisted.allowedSites[ADDR2]).toBeUndefined();
|
||||
});
|
||||
});
|
||||
|
||||
describe("a revoked site permission against a stale page's later save", () => {
|
||||
test("the revocation holds even when the stale page approves something else", async () => {
|
||||
const storage = makeStorage();
|
||||
await storage.set({
|
||||
autistmask: {
|
||||
wallets: [W1],
|
||||
allowedSites: { [ADDR1]: ["evil.example"] },
|
||||
},
|
||||
});
|
||||
|
||||
// A stale page loads while the permission still stands.
|
||||
const stale = loadPage(storage);
|
||||
await stale.state.loadState();
|
||||
|
||||
// Settings revokes it — src/popup/views/settings.js:55-68 — from a
|
||||
// second page.
|
||||
const settings = loadPage(storage);
|
||||
await settings.state.loadState();
|
||||
revokeSite(settings.state.state, "evil.example");
|
||||
await settings.state.saveState();
|
||||
expect(
|
||||
(await storage.get("autistmask")).autistmask.allowedSites[ADDR1],
|
||||
).toBeUndefined();
|
||||
|
||||
// The stale page, unaware of the revoke, approves an unrelated site
|
||||
// for a different address and saves — src/background/index.js:592-599.
|
||||
approveSite(stale.state.state, ADDR2, "good.example");
|
||||
await stale.state.saveState();
|
||||
|
||||
const persisted = (await storage.get("autistmask")).autistmask;
|
||||
expect(persisted.allowedSites[ADDR2]).toEqual(["good.example"]);
|
||||
expect(persisted.allowedSites[ADDR1]).toBeUndefined();
|
||||
});
|
||||
});
|
||||
|
||||
// mergeListByIdentity()'s identity function is not guaranteed collision-free
|
||||
// — walletIdentity() falls back to one shared "addr:" value for any wallet
|
||||
// with neither an xpub nor a populated first address (a legacy or corrupt
|
||||
// record). Two such records created independently on two different pages
|
||||
// must not silently collapse into one, dropping the loser's
|
||||
// encryptedSecret with no error and no log.
|
||||
function legacyWallet(name, secret) {
|
||||
return {
|
||||
type: "legacy",
|
||||
name,
|
||||
encryptedSecret: secret,
|
||||
nextIndex: 0,
|
||||
addresses: [],
|
||||
};
|
||||
}
|
||||
|
||||
describe("two wallets independently created with a colliding identity", () => {
|
||||
test("both survive, encryptedSecret included, instead of one silently replacing the other", async () => {
|
||||
const storage = makeStorage();
|
||||
await storage.set({ autistmask: { wallets: [W1] } });
|
||||
|
||||
// Both pages load before either has created their malformed wallet,
|
||||
// so neither has baseline knowledge of the other's.
|
||||
const pageA = loadPage(storage);
|
||||
await pageA.state.loadState();
|
||||
const pageB = loadPage(storage);
|
||||
await pageB.state.loadState();
|
||||
|
||||
pageA.state.state.wallets.push(legacyWallet("Legacy A", "secret-a"));
|
||||
pageA.state.state.hasWallet = true;
|
||||
await pageA.state.saveState();
|
||||
expect(
|
||||
(await storage.get("autistmask")).autistmask.wallets,
|
||||
).toHaveLength(2);
|
||||
|
||||
pageB.state.state.wallets.push(legacyWallet("Legacy B", "secret-b"));
|
||||
pageB.state.state.hasWallet = true;
|
||||
await pageB.state.saveState();
|
||||
|
||||
const persisted = (await storage.get("autistmask")).autistmask;
|
||||
const secrets = persisted.wallets.map((w) => w.encryptedSecret);
|
||||
expect(secrets).toContain("secret-one");
|
||||
expect(secrets).toContain("secret-a");
|
||||
expect(secrets).toContain("secret-b");
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user