fix(reset): clear the asset-icon cache on wallet reset

[dmnyc]

Fixes #106 (L-8).

apogee:asseticon:<assetId> keys persist which non-LBTC assets the wallet displayed, and wallet/reset didn't remove them — a holdings fingerprint survived the wipe. clearAssetIconCache() now drops every cached icon (and the in-memory negative cache) as part of reset, alongside the keystore, scan-state, and idempotency cleanup. setActiveWallet is unwired today; wallet-switch cleanup can ride along when it's exposed.

---

^{Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.}

[dmnyc]

@claude review

[claude]

Claude finished @dmnyc's task in 6m 29s —— View job

---

Code review

• Gather context (diff, manifests, related files)
• Review asset-icon cache clearing on reset
• Review the other 5 commits bundled in this branch
• Check CI status
• Post review

---

The icon-cache fix is the right shape and the reasoning in the comments is good throughout. One finding below is significantly more serious than the stated scope of this PR, and it's in the offscreen commit that's riding along.

Note up front: this branch carries 6 commits across 5 unrelated concerns (offscreen sender gate, keystore throttle, WAR dynamic URL, two approval-display changes, icon cache), but the title and body only describe the icon cache. Flagging for reviewability — reviewers filtering on the description will skim past the security-relevant one.

CI: Provider conformance passed on this branch, which meaningfully de-risks the use_dynamic_url flip (that job builds the extension and drives window.apogee through a real Chromium load).

---

🔴 The offscreen sender gate doesn't close the hole it describes

src/offscreen/offscreen.ts:18

if (sender.id !== browser.runtime.id) return false;

sender.id equals our own extension id for our content script too — which runs in every page at document_start. So this gate admits page-influenced traffic. The route:

1. src/content/content.ts:82 strips only type and source from page-supplied params:

   if (k !== "type" && k !== "source") safeParams[k] = v;

   target and req pass through untouched.
2. content.ts:87 sends { ...safeParams, type: "provider/rpc" } — so a page calling window.apogee.rpc({ target: "offscreen", req: {...} }) produces a message carrying target: "offscreen".
3. src/background/index.ts:2936 deliberately declines it (if (msg?.target === "offscreen") return false;) before the origin gate at line 2944 — the background is not the gate here.
4. The offscreen listener matches target === "offscreen", passes the new sender.id check, and calls handle(msg.req). It returns true, so the response even relays back to the page.

The exposed surface is EngineRequest — broadcastPset with an arbitrary PSET, sync/checkEsplora/broadcastPset with an attacker-chosen esploraUrl (arbitrary fetch from the extension's privileged origin, which holds <all_urls>-adjacent host_permissions), and getBalance/getTransactions/getUtxos/walletIdentity for any descriptor. It is not key theft — signPset/signBroadcast carry their own mnemonic — but it is unauthenticated access to the signing-and-broadcasting engine from any web page, gated only on the offscreen document being alive.

This hole predates the PR; the commit just doesn't close it. Two changes:

// offscreen.ts — mirror the background gate fully, not just the id half
const EXT_ORIGIN = new URL(browser.runtime.getURL("/")).origin;
if (sender.origin !== EXT_ORIGIN || sender.id !== browser.runtime.id) return false;

// content.ts:82 — routing keys are never page-supplied
if (k !== "type" && k !== "source" && k !== "target") safeParams[k] = v;

One link I could not verify statically here (no deps installed in this checkout): that a content-script runtime.sendMessage is actually delivered to the offscreen document's onMessage. Everything else in the chain is confirmed in the source, and step 3 — the background explicitly stepping aside for target: "offscreen" — only makes sense if cross-context delivery happens. Worth a 5-minute manual confirmation before deciding severity.

Fix this →

---

🟠 Icon-cache cleanup can block the wipe

src/background/index.ts:479

await clearAssetIconCache();
return keystore.reset();

The icon cache is the least important thing in this handler, but it's now a hard await in front of the one step that actually destroys the vault. If storage.local.get(null) or remove() rejects, the whole wallet/reset throws and the keystore survives — a user trying to wipe a device ends up with a wallet still on it. Contrast the IndexedDB delete just above, which explicitly resolves on onerror/onblocked rather than propagating.

Either move it after keystore.reset() or wrap it: await clearAssetIconCache().catch((e) => console.warn("[apogee] icon-cache clear failed", e));

Fix this →

---

🟠 failed.clear() runs in the wrong context

src/lib/asset-icons.ts:66

assetIconSrc has exactly one caller — src/sidepanel/components/AssetIcon.tsx:31, in the panel. clearAssetIconCache runs in the background service worker, a separate JS context with its own module instance. The background never populates failed, so failed.clear() there clears an always-empty set; the panel's copy — the only populated one — is untouched.

Impact is small (session-only, memory-only, and the panel usually navigates to onboarding after a reset), but the PR body's "(and the in-memory negative cache)" and the comment on line 61–62 both currently claim something that doesn't happen. Either drop the claim, or make it true with a browser.storage.onChanged listener in asset-icons.ts that clears failed when the cache keys disappear.

Fix this →

---

🟡 An in-flight icon fetch can re-seed the cache after the wipe

src/sidepanel/components/AssetIcon.tsx:31 guards with alive, but that only suppresses setSrc — assetIconSrc still runs await browser.storage.local.set({ [key]: dataUri }) at asset-icons.ts:53 when the fetch resolves, unmounted or not. A fetch started shortly before the user confirms reset can land after clearAssetIconCache() and re-persist apogee:asseticon:<id> for the wallet that was just wiped. The window is one network round-trip, so it's narrow but not theoretical on a slow connection.

The same cross-context split as above makes this awkward to fix cleanly — a generation counter in asset-icons.ts wouldn't see the background's clear. A storage.onChanged listener that both clears failed and sets a "don't write" flag would close both this and the previous finding in one move.

---

🟡 changePassword throttle: correct, but note what it implies once wired up

src/keystore/keystore.ts:347-356. Routing the old-password check through the same counter is right — it is the same oracle, and leaving it unthrottled would have been a clean bypass of the unlock lockout. Two notes:

• changePassword has no caller outside keystore.ts today (same situation as the setActiveWallet you mention in the description), so this is pre-wiring.
• When it is wired: MAX_UNLOCK_FAILS = 21 hard-locks the vault into recovery-only. That now means a user who is already unlocked and fat-fingers their current password enough times in a Change Password form bricks the wallet into "re-import your recovery phrase". Defensible — but worth being a deliberate decision rather than a side effect. Whichever screen wires this must decode UNLOCK_THROTTLED:<ts> and UNLOCK_BLOCKED the way the Unlock screen does, or the user gets a raw error code.

---

🟡 Vite plugin nits

vite.config.ts:36-52

• Fails open silently. If a crxjs upgrade changes the emitted key or shape, if (!Array.isArray(wars)) return; ships the fingerprintable manifest with no signal. The whole point is a security property, so it should be loud — the playground case is already distinguished by the missing manifest, so the WAR-shape branch can safely throw (or at minimum console.warn).
• outDir resolution. path.resolve(outDir, "manifest.json") resolves against process.cwd(), while Vite resolves build.outDir against config.root. Identical here, so it works — path.resolve(config.root, config.build.outDir) in configResolved is the form that stays correct.
• The !wars.some((war) => war.use_dynamic_url !== true) early-return logic is right for all three cases (empty, all-already-true, some-false).
• Firefox is unaffected — scripts/build-firefox.ts runs with configFile: false, and firefoxManifest() declares no web_accessible_resources at all. Good.

---

🟢 Approval display

src/sidepanel/screens/Approval.tsx

• Both changes are well-motivated and the comments explain the why, which is the right call for security-relevant UI.
• shortenHex(request.origin, 18, 14) — a hex-specific helper on a URL reads oddly, and its head/tail budget was tuned for hex. A shortenMiddle alias in src/lib/utils.ts would cost nothing and say what it means.
• Middle-truncation does keep the registrable domain visible, which is the real win. It doesn't fully solve the lookalike case though: https://apogee.io.evil-domain.com renders as https://apogee.io.…il-domain.com, still leading with a trusted-looking prefix. If you want the strongest anti-spoof read, render the eTLD+1 in --text-strong and subdue the rest.
• Dropping truncate from the <p> is safe given shortenHex caps the string at 33 chars.
• Row ... wrap renders min-w-0 break-all text-right inside a flex items-center row. A ~100-char confidential Liquid address wraps to several lines with the "To" label floating vertically centered against them — items-start would read better. And since the point is user verification, 4-char grouping beats break-all's arbitrary break points for catching a swapped character.

---

Testing

src/lib/ has a solid test convention (asset-registry, portfolio, qr-secret, …) but asset-icons.ts has none, and nothing in this PR adds one. Reset hygiene is exactly the kind of property that regresses silently — a test with a fake browser.storage.local asserting that every apogee:asseticon:* key goes, that unrelated keys (the encrypted vault, the moon-intro flag) survive, and that remove() isn't called when nothing is cached, is cheap insurance for an issue you're closing on security grounds.

Add the test →

Minor: browser.storage.local.get(null) pulls all of local storage into memory, including every cached data-URI (≤200 KB each) and the encrypted vault. Fine at current scale, and storage.local.getKeys() needs Chrome 130+ against a minimum_chrome_version: "116" — so get(null) is the right call today, just worth a comment noting why.

---

I did not run the build or test suite — dependencies aren't installed in this checkout and pnpm install isn't permitted for this job. The findings above are from source reading plus the passing Provider conformance run.
· fix/106-asseticon-reset

[dmnyc]

Review feedback addressed in 632ff00:

• Icon-cache cleanup can block the wipe: the reset handler now fires clearAssetIconCache() without awaiting it, with a .catch(console.warn) — matching the failure-tolerant convention of the IndexedDB delete above it. A storage error can no longer leave the vault intact on a device the user asked to wipe.
• In-flight fetch re-seeding after the wipe: a module-level generation counter — bumped on clear, captured at fetch start — makes an in-flight fetch drop its result instead of writing the cache entry back.
• failed.clear() context: kept, but the doc comment now states the scope honestly — the SW's instance of the set is empty (the panel is the fetcher); its in-memory state dies with its document, storage is the durable part this clear owns.