fix: version the stored profile, and give a record that cannot be read a way out (closes #311)
The stored profile carried no version, so nothing could tell a record this build wrote from one a later build did, and loadState() coerced scalars while trusting the structure. A wallets that was a string, an array of nulls, or a later schema's wallet records reached the popup and threw on the first dereference: no view, no message, no control, and every dApp call answering a generic -32603 because getActiveAddress() dereferenced the same record. There was no reset or wipe control anywhere in the product, so the only escape was clearing extension storage through browser internals. saveState() and updateState() now both stamp STATE_SCHEMA_VERSION, and every read goes through assertStateUsable() on the raw bytes before normalization can paper over them. Version 1 is the shape that shipped unversioned, so the profile every existing install holds loads normally and is migrated in place by being stamped on the first write; an upgrade shows nobody a wipe prompt for a wallet that is fine. A record this build cannot vouch for is refused instead, and refused all the way: not normalized, not written back, not half-loaded, and not overwritten by a save either. The gate covers what nothing downstream can floor. Everything else is normalizePersisted()'s job, and two of those floors were written on truthiness rather than on type, so a truthy value of the wrong type walked straight through and threw on the first dereference — the same blank popup, by a longer route. trackedTokens: "nope" rendered nothing with "Cannot read properties of undefined (reading 'toLowerCase')", and activeAddress: 42 rendered nothing with "address.slice is not a function", for profiles whose wallets were perfectly readable. Both are type checks now, matching what networkEndpoints and viewStack already did in the same file, and an empty list or an empty string still survives. The popup shows a new StateRecovery screen. It names the problem in a sentence, exports the stored record into a text box on the page with no normalization or repair on it (and downloads it where the browser allows), and offers an erase behind a typed ERASE MY WALLET. Both controls are required: an export with no reset leaves the user stuck, and a reset with no export destroys the only copy of possibly recoverable key material. The Settings gear is hidden while it is up, and showView() is not used to raise it, because both read the state singleton that by then refuses to be read. The export is JSON.stringify of the deserialized record, so a value JSON cannot represent — a cycle, or a BigInt, which Firefox's storage can hold — fails the export entirely and leaves erase as the only control; that is now stated where the export is written. The background refuses the same record and answers dApps -32007 with a message saying the saved data cannot be read and that nothing was signed or sent, rather than the -32603 it also answers when a signing attempt breaks. EIP-1474 sets aside -32000..-32099 for implementation-defined server errors but assigns meanings to -32000 through -32006, including -32001 "Resource not found" and the -32002 "Resource unavailable" this wallet already uses for a pending approval; -32007..-32099 are the unassigned ones, and a test pins the code against that table. networkById() now throws on an id it does not know instead of quietly answering mainnet, which also stops NETWORKS["constructor"] resolving off the prototype chain. Every key test in the gate is an own-property test, because networkId is an object key into networkEndpoints and an unvalidated "__proto__" set that map's prototype instead of an own key, dropping the user's endpoint silently; normalizePersisted() copies endpoint entries with defineProperty for the same reason. That own-property discipline is the gate's alone — normalizePersisted() reads the same fields plainly, and the two agree only because a record from storage has been through structuredClone and carries Object.prototype. The three corrupt blobs from the issue drive the real popup entry point and the real worker in tests; each rendered nothing at all and answered -32603 before this, and the unversioned-but-valid case is tested too. Three test files used fixture wallets the product cannot produce (a bare address string where an address record belongs, a wallet with no address list) and now use whole records. src/popup/restorableViews.js moved to src/shared/restorableViews.js, since persistedState.js requires it and that module is in the background bundle.
This commit is contained in:
203
tests/stateUnusableRpc.test.js
Normal file
203
tests/stateUnusableRpc.test.js
Normal file
@@ -0,0 +1,203 @@
|
||||
// What a dApp is told when the wallet's stored profile cannot be read
|
||||
// (https://git.eeqj.de/sneak/AutistMask/issues/311).
|
||||
//
|
||||
// The popup is not the only casualty of a bad blob. getActiveAddress()
|
||||
// dereferences the stored wallet list on nearly every method, so against the
|
||||
// build this file was added to, EVERY request from EVERY page came back as
|
||||
// -32603 "AutistMask could not complete this request because of an internal
|
||||
// error" — the code the wallet also answers when a signing attempt blows up,
|
||||
// with nothing in it to tell the page or the user what is actually wrong or
|
||||
// what to do about it.
|
||||
//
|
||||
// So what is pinned here is that the answer is SPECIFIC: its own code, and a
|
||||
// message that says the saved data cannot be read, that nothing was signed or
|
||||
// sent, and where to go to fix it.
|
||||
//
|
||||
// Same cold-worker shape as tests/coldWorkerChainId.test.js: the real state
|
||||
// modules, over a storage stub, with no loadState() of the test's own — the
|
||||
// handler has to reach storage by itself, as a worker revived by the page's
|
||||
// own message does.
|
||||
|
||||
const CONNECTED_ORIGIN = "https://dapp.example";
|
||||
const ADDRESS = "0x66133E8ea0f5D1d612D2502a968757D1048c214a";
|
||||
|
||||
// The generic answer, quoted rather than imported: this file's whole point is
|
||||
// that the state-unusable path stopped using it.
|
||||
const GENERIC_INTERNAL_ERROR_CODE = -32603;
|
||||
|
||||
// EIP-1474's assigned non-standard codes, verbatim. The spec sets aside
|
||||
// -32000..-32099 for implementation-defined server errors but hands out
|
||||
// meanings for the first seven, so those are exactly the codes this condition
|
||||
// may NOT take: a page reading -32001 is entitled to read "Resource not
|
||||
// found". -32002 is in this table AND in use here, for a pending approval.
|
||||
const EIP_1474_ASSIGNED = {
|
||||
"-32000": "Invalid input",
|
||||
"-32001": "Resource not found",
|
||||
"-32002": "Resource unavailable",
|
||||
"-32003": "Transaction rejected",
|
||||
"-32004": "Method not supported",
|
||||
"-32005": "Limit exceeded",
|
||||
"-32006": "JSON-RPC version not supported",
|
||||
};
|
||||
|
||||
// The three blobs from the issue.
|
||||
const CORRUPT_BLOBS = [
|
||||
{
|
||||
name: "wallets is a string",
|
||||
blob: { hasWallet: true, wallets: ADDRESS },
|
||||
},
|
||||
{
|
||||
name: "wallets is an array of garbage",
|
||||
blob: { hasWallet: true, wallets: [null, 42, "wallet"] },
|
||||
},
|
||||
{
|
||||
name: "future-schema blob (unknown fields, no version)",
|
||||
blob: {
|
||||
hasWallet: true,
|
||||
wallets: [{ id: "wallet-1", accounts: [{ addr: ADDRESS }] }],
|
||||
profileFormat: "am-2",
|
||||
},
|
||||
},
|
||||
];
|
||||
|
||||
async function settle() {
|
||||
for (let i = 0; i < 50; i++) await Promise.resolve();
|
||||
}
|
||||
|
||||
afterEach(() => {
|
||||
delete global.chrome;
|
||||
});
|
||||
|
||||
function loadColdWorker(stored) {
|
||||
jest.resetModules();
|
||||
|
||||
jest.doMock("../src/shared/balances", () => ({
|
||||
getProvider: () => ({}),
|
||||
refreshBalances: jest.fn(async () => {}),
|
||||
}));
|
||||
jest.doMock("../src/shared/phishingDomains", () => ({
|
||||
isPhishingDomain: () => false,
|
||||
}));
|
||||
jest.doMock("../src/shared/alarms", () => ({
|
||||
BALANCE_REFRESH_ALARM: "balance",
|
||||
BALANCE_REFRESH_PERIOD_MINUTES: 1,
|
||||
ensureRecurringAlarms: jest.fn(async () => {}),
|
||||
registerAlarmHandlers: jest.fn(),
|
||||
}));
|
||||
|
||||
const store = { autistmask: structuredClone(stored) };
|
||||
let messageListener = null;
|
||||
const set = jest.fn(async (items) => {
|
||||
store.autistmask = structuredClone(items.autistmask);
|
||||
});
|
||||
|
||||
global.chrome = {
|
||||
storage: {
|
||||
local: {
|
||||
get: jest.fn(async () => structuredClone(store)),
|
||||
set,
|
||||
},
|
||||
},
|
||||
runtime: {
|
||||
getURL: (p) => "chrome-extension://autistmask/" + p,
|
||||
onMessage: {
|
||||
addListener: (fn) => {
|
||||
messageListener = fn;
|
||||
},
|
||||
},
|
||||
onConnect: { addListener: () => {} },
|
||||
lastError: null,
|
||||
},
|
||||
windows: {
|
||||
getLastFocused: (cb) => cb(null),
|
||||
create: (options, cb) => cb({ id: 1 }),
|
||||
remove: (id, cb) => {
|
||||
if (cb) cb();
|
||||
},
|
||||
onRemoved: { addListener: () => {} },
|
||||
},
|
||||
tabs: {
|
||||
query: (queryInfo, cb) => cb([{ id: 1 }]),
|
||||
sendMessage: (tabId, message, cb) => {
|
||||
if (cb) cb();
|
||||
},
|
||||
},
|
||||
action: { setPopup: () => {} },
|
||||
};
|
||||
|
||||
require("../src/background/index");
|
||||
|
||||
async function rpc(method, params) {
|
||||
let result = null;
|
||||
messageListener(
|
||||
{ type: "AUTISTMASK_RPC", method, params: params || [] },
|
||||
{ origin: CONNECTED_ORIGIN },
|
||||
(r) => {
|
||||
result = r;
|
||||
},
|
||||
);
|
||||
await settle();
|
||||
return result;
|
||||
}
|
||||
|
||||
return { rpc, persisted: () => store.autistmask, storageSet: set };
|
||||
}
|
||||
|
||||
// Every method a page can reach that has to consult the profile.
|
||||
const METHODS = [
|
||||
"eth_accounts",
|
||||
"eth_requestAccounts",
|
||||
"eth_chainId",
|
||||
"personal_sign",
|
||||
"eth_sendTransaction",
|
||||
];
|
||||
|
||||
describe("a dApp call against a profile the wallet cannot read", () => {
|
||||
for (const { name, blob } of CORRUPT_BLOBS) {
|
||||
test(`${name}: a specific error, not the generic internal one`, async () => {
|
||||
const bg = loadColdWorker(blob);
|
||||
|
||||
const answer = await bg.rpc("eth_accounts");
|
||||
|
||||
expect(answer.error).toBeDefined();
|
||||
expect(answer.error.code).not.toBe(GENERIC_INTERNAL_ERROR_CODE);
|
||||
// The message has to say what is wrong, that nothing was sent,
|
||||
// and where to go. "Internal error" says none of the three.
|
||||
expect(answer.error.message).toMatch(/saved data/i);
|
||||
expect(answer.error.message).toMatch(/nothing was/i);
|
||||
expect(answer.error.message).toMatch(/AutistMask/);
|
||||
});
|
||||
}
|
||||
|
||||
test("every method that consults the profile answers the same way", async () => {
|
||||
const bg = loadColdWorker(CORRUPT_BLOBS[0].blob);
|
||||
|
||||
const codes = new Set();
|
||||
for (const method of METHODS) {
|
||||
const answer = await bg.rpc(method, ["0x00", ADDRESS]);
|
||||
expect(answer.error).toBeDefined();
|
||||
codes.add(answer.error.code);
|
||||
}
|
||||
|
||||
// One code for the condition, whatever the method was.
|
||||
expect(codes.size).toBe(1);
|
||||
expect(codes.has(GENERIC_INTERNAL_ERROR_CODE)).toBe(false);
|
||||
|
||||
// And it is a code EIP-1474 has not already given a meaning to, so a
|
||||
// page reading it by the spec's table is not told something false.
|
||||
const code = [...codes][0];
|
||||
expect(EIP_1474_ASSIGNED[String(code)]).toBeUndefined();
|
||||
expect(code).toBeLessThanOrEqual(-32007);
|
||||
expect(code).toBeGreaterThanOrEqual(-32099);
|
||||
});
|
||||
|
||||
test("it does not write over the record it could not read", async () => {
|
||||
const bg = loadColdWorker(CORRUPT_BLOBS[1].blob);
|
||||
|
||||
await bg.rpc("eth_accounts");
|
||||
await bg.rpc("eth_chainId");
|
||||
|
||||
expect(bg.storageSet).not.toHaveBeenCalled();
|
||||
expect(bg.persisted()).toEqual(CORRUPT_BLOBS[1].blob);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user