harden: key remembered site permissions by full origin (closes #402)
allowedSites and deniedSites held the bare hostname, so a grant to https://dapp.example also authorised http://dapp.example and every port on that host, and the connection, transaction and signature prompts named only the hostname. Both lists now store and match the full origin (scheme://host[:port]), the key the connections approved without Remember already used. The prompts, the Settings site lists and AUTISTMASK_REMOVE_SITE use the origin too. Entries saved by hostname are not migrated (pre-1.0): they match no site. Model: opus-5-5
This commit was merged in pull request #431.
This commit is contained in:
+33
-31
@@ -270,31 +270,32 @@ describe("background refresh racing a wallet deleted on another page", () => {
|
||||
});
|
||||
});
|
||||
|
||||
// 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.
|
||||
// allowedSites/deniedSites: { [address]: [origin, ...] }. Mutated in place
|
||||
// from two different contexts — rememberSiteChoice() in
|
||||
// src/background/index.js pushes a newly approved origin onto
|
||||
// state.allowedSites[activeAddress], and the Settings "revoke" button
|
||||
// (forgetOrigin() in src/popup/views/settings.js) filters an origin 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) {
|
||||
function approveSite(pageState, address, origin) {
|
||||
if (!pageState.allowedSites[address]) {
|
||||
pageState.allowedSites[address] = [];
|
||||
}
|
||||
if (!pageState.allowedSites[address].includes(hostname)) {
|
||||
pageState.allowedSites[address].push(hostname);
|
||||
if (!pageState.allowedSites[address].includes(origin)) {
|
||||
pageState.allowedSites[address].push(origin);
|
||||
}
|
||||
}
|
||||
|
||||
function revokeSite(pageState, hostname) {
|
||||
function revokeSite(pageState, origin) {
|
||||
for (const addr of Object.keys(pageState.allowedSites)) {
|
||||
pageState.allowedSites[addr] = pageState.allowedSites[addr].filter(
|
||||
(h) => h !== hostname,
|
||||
(o) => o !== origin,
|
||||
);
|
||||
if (pageState.allowedSites[addr].length === 0) {
|
||||
delete pageState.allowedSites[addr];
|
||||
@@ -308,7 +309,7 @@ describe("a dApp approval racing a stale Settings page's later save", () => {
|
||||
await storage.set({
|
||||
autistmask: {
|
||||
wallets: [W1],
|
||||
allowedSites: { [ADDR2]: ["other.example"] },
|
||||
allowedSites: { [ADDR2]: ["https://other.example"] },
|
||||
},
|
||||
});
|
||||
|
||||
@@ -318,24 +319,24 @@ describe("a dApp approval racing a stale Settings page's later save", () => {
|
||||
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.
|
||||
// different address and saves — the real sequence in
|
||||
// rememberSiteChoice(), src/background/index.js.
|
||||
const approval = loadPage(storage);
|
||||
await approval.state.loadState();
|
||||
approveSite(approval.state.state, ADDR1, "dapp.example");
|
||||
approveSite(approval.state.state, ADDR1, "https://dapp.example");
|
||||
await approval.state.saveState();
|
||||
expect(
|
||||
(await storage.get("autistmask")).autistmask.allowedSites[ADDR1],
|
||||
).toEqual(["dapp.example"]);
|
||||
).toEqual(["https://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");
|
||||
// Settings revokes its own, unrelated site — the real sequence in
|
||||
// forgetOrigin(), src/popup/views/settings.js — and saves from state
|
||||
// loaded before the dApp approval ever happened.
|
||||
revokeSite(settings.state.state, "https://other.example");
|
||||
await settings.state.saveState();
|
||||
|
||||
const persisted = (await storage.get("autistmask")).autistmask;
|
||||
expect(persisted.allowedSites[ADDR1]).toEqual(["dapp.example"]);
|
||||
expect(persisted.allowedSites[ADDR1]).toEqual(["https://dapp.example"]);
|
||||
expect(persisted.allowedSites[ADDR2]).toBeUndefined();
|
||||
});
|
||||
});
|
||||
@@ -346,7 +347,7 @@ describe("a revoked site permission against a stale page's later save", () => {
|
||||
await storage.set({
|
||||
autistmask: {
|
||||
wallets: [W1],
|
||||
allowedSites: { [ADDR1]: ["evil.example"] },
|
||||
allowedSites: { [ADDR1]: ["https://evil.example"] },
|
||||
},
|
||||
});
|
||||
|
||||
@@ -354,23 +355,24 @@ describe("a revoked site permission against a stale page's later save", () => {
|
||||
const stale = loadPage(storage);
|
||||
await stale.state.loadState();
|
||||
|
||||
// Settings revokes it — src/popup/views/settings.js:55-68 — from a
|
||||
// second page.
|
||||
// Settings revokes it — forgetOrigin(), src/popup/views/settings.js —
|
||||
// from a second page.
|
||||
const settings = loadPage(storage);
|
||||
await settings.state.loadState();
|
||||
revokeSite(settings.state.state, "evil.example");
|
||||
revokeSite(settings.state.state, "https://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");
|
||||
// for a different address and saves — rememberSiteChoice(),
|
||||
// src/background/index.js.
|
||||
approveSite(stale.state.state, ADDR2, "https://good.example");
|
||||
await stale.state.saveState();
|
||||
|
||||
const persisted = (await storage.get("autistmask")).autistmask;
|
||||
expect(persisted.allowedSites[ADDR2]).toEqual(["good.example"]);
|
||||
expect(persisted.allowedSites[ADDR2]).toEqual(["https://good.example"]);
|
||||
expect(persisted.allowedSites[ADDR1]).toBeUndefined();
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user