From d040c2f5afc9b2d4cd0a9e154fdb189da3be7ea2 Mon Sep 17 00:00:00 2001 From: thePR0M3TH3AN <53631862+PR0M3TH3AN@users.noreply.github.com> Date: Fri, 31 Jul 2026 09:40:49 -0400 Subject: [PATCH] Tier 1 hardening: no more silent hangs, no writes before consent Reliability, which is where most of these reach users: - The worker RPC had no message validation, no error/messageerror listener, no timeout, and terminate() left every pending promise unsettled. A malformed frame threw inside the listener (posting no response at all) and a dead worker stranded its callers -- the documented innerHTML-remount hazard manifested as a permanent freeze rather than an error. Both sides now validate the envelope, calls time out at 60s, and worker death rejects everything outstanding. - requestNwcConnection fired no pending event, so a host using the RECOMMENDED hidden-mount pattern navigated a hidden element to a consent screen nobody could see and waited forever. Now symmetric with bitlogin-offer-pending. Consent integrity: - vaultOfferCheck published an origin rebinding to relays -- a durable mutation of the user's account -- from a plain host API call before any consent screen existed. It is read-only now, and a wallet stored under a DIFFERENT origin no longer reports as already-saved, so the binding can only change through the consented path. - offerNwcConnection's guard sat above three awaits; two concurrent calls both passed it and the second dropped the first's promise. The slot is claimed synchronously. Vault reads fail closed: - listConnections threw rather than returning a relay-shaped subset when quorum is not met -- a short listing is indistinguishable from "you have no connections", and one relay should not get to say that. - The 500-record page limit is reported (truncated) instead of silently deciding which connections exist. Supply chain: - All eight crypto dependencies pinned to exact versions (they held users' keys behind caret ranges), and Bitcoin Connect with them. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01CMr9Yt8SYmZcfgrg3CV2G1 --- packages/core/package.json | 16 +++--- packages/core/src/vault/session.ts | 15 ++++++ packages/core/src/vault/sync.ts | 12 ++++- packages/widget/package.json | 2 +- packages/widget/src/element.ts | 24 ++++++++- packages/widget/src/worker/cryptoWorker.ts | 57 ++++++++++++++-------- packages/widget/src/worker/workerClient.ts | 51 ++++++++++++++++++- 7 files changed, 141 insertions(+), 36 deletions(-) diff --git a/packages/core/package.json b/packages/core/package.json index 2896265..216f938 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -21,14 +21,14 @@ "typecheck": "tsc -b --noEmit" }, "dependencies": { - "@noble/ciphers": "^2.2.0", - "@noble/curves": "^1.9.1", - "@noble/hashes": "^1.7.1", - "@scure/base": "^1.2.6", - "@scure/bip39": "^1.5.4", - "canonicalize": "^2.0.0", - "diceware-wordlist-en-eff": "^1.0.1", - "hash-wasm": "^4.12.0" + "@noble/ciphers": "2.2.0", + "@noble/curves": "1.9.7", + "@noble/hashes": "1.8.0", + "@scure/base": "1.2.6", + "@scure/bip39": "1.6.0", + "canonicalize": "2.1.0", + "diceware-wordlist-en-eff": "1.0.1", + "hash-wasm": "4.12.0" }, "devDependencies": { "@types/node": "^22.10.0", diff --git a/packages/core/src/vault/session.ts b/packages/core/src/vault/session.ts index 46133c6..7234d07 100644 --- a/packages/core/src/vault/session.ts +++ b/packages/core/src/vault/session.ts @@ -226,14 +226,28 @@ export class VaultSession { relayUrls: string[]; store: KeyValueStore; timeoutMs?: number; + /** Accept a below-quorum answer anyway. Off by default: a partial view + * of a credential store reads as authoritative and isn't. */ + acknowledgeIncompleteQuorum?: boolean; }): Promise<{ connections: DecryptedConnectionRecord[]; rollbackWarnings: string[]; /** Event ids that failed to decrypt or validate. Surfaced, never silent. */ unreadable: string[]; + /** True when the relay page filled, so records may be missing. */ + truncated: boolean; quorumMet: boolean; }> { const fetched = await this.fetchEvents(params); + if (!fetched.quorumMet && !params.acknowledgeIncompleteQuorum) { + // Failing closed matters more here than for a read of public data: a + // short listing looks identical to "you have no connections", and a + // single relay deciding that is how a revoked-looking vault gets + // re-populated or a live credential gets hidden. + throw new Error( + "Not enough vault relays answered to list your connections reliably. Check your connection and retry." + ); + } const connections: DecryptedConnectionRecord[] = []; const unreadable: string[] = []; for (const event of fetched.events.values()) { @@ -254,6 +268,7 @@ export class VaultSession { connections, rollbackWarnings: fetched.rollbackWarnings, unreadable, + truncated: fetched.truncated, quorumMet: fetched.quorumMet }; } diff --git a/packages/core/src/vault/sync.ts b/packages/core/src/vault/sync.ts index 5d1f4fa..d12c2c6 100644 --- a/packages/core/src/vault/sync.ts +++ b/packages/core/src/vault/sync.ts @@ -99,8 +99,14 @@ export interface FetchedConnectionEvents { rollbackWarnings: string[]; quorumMet: boolean; respondedCount: number; + /** True when a relay returned the full page, so records may be missing. */ + truncated: boolean; } +/** Ceiling on records fetched in one query. Hit means the relay, not the + * user, decided which connections exist -- so it is REPORTED, not silent. */ +const FETCH_LIMIT = 500; + /** Fetches every connection record visible for this vault identity (§CV10). */ export async function fetchConnectionRecordEvents(params: { vaultPrk: Uint8Array; @@ -114,13 +120,14 @@ export async function fetchConnectionRecordEvents(params: { let quorum; try { quorum = await pool.queryQuorum( - { kinds: [KIND_APP_DATA], authors: [vaultPubkey], limit: 500 }, + { kinds: [KIND_APP_DATA], authors: [vaultPubkey], limit: FETCH_LIMIT }, params.timeoutMs ); } finally { pool.closeAll(); } + const truncated = quorum.outcomes.some((outcome) => outcome.events.length >= FETCH_LIMIT); const newestById = new Map(); for (const event of quorum.outcomes.flatMap((o) => o.events)) { if (!verifyNostrEvent(event) || event.pubkey !== vaultPubkey) continue; @@ -157,6 +164,7 @@ export async function fetchConnectionRecordEvents(params: { events, rollbackWarnings, quorumMet: quorum.quorumMet, - respondedCount: quorum.respondedCount + respondedCount: quorum.respondedCount, + truncated }; } diff --git a/packages/widget/package.json b/packages/widget/package.json index 7d52e43..1317756 100644 --- a/packages/widget/package.json +++ b/packages/widget/package.json @@ -13,7 +13,7 @@ }, "dependencies": { "@bitlogin/core": "*", - "@getalby/bitcoin-connect": "^3.12.3" + "@getalby/bitcoin-connect": "3.12.3" }, "devDependencies": { "typescript": "^5.7.3", diff --git a/packages/widget/src/element.ts b/packages/widget/src/element.ts index a00122b..e5a3c5a 100644 --- a/packages/widget/src/element.ts +++ b/packages/widget/src/element.ts @@ -123,6 +123,8 @@ export class BitLoginAuthElement extends HTMLElement { private vaultConnections: VaultConnectionSummary[] | null = null; /** offerNwcConnection state: the app already holds this URI; the only * question on screen is whether a copy enters the user's vault. */ + /** Claimed synchronously by offerNwcConnection before any await (see there). */ + private offerInFlight = false; private vaultOffer: { uri: string; appName: string; @@ -239,6 +241,10 @@ export class BitLoginAuthElement extends HTMLElement { const appName = options.appName?.trim() || window.location.hostname || "This app"; return new Promise((resolve) => { this.vaultRequest = { appName, reason: options.reason, origin, stage: "auth", resolve }; + // Symmetric with bitlogin-offer-pending. Without it, a host following + // the recommended hidden-mount pattern navigated a HIDDEN element to a + // consent screen nobody could see, and the promise never settled. + this.dispatchEvent(new CustomEvent("bitlogin-request-pending")); void (async () => { try { const status = await this.worker.getSessionStatus(); @@ -272,12 +278,26 @@ export class BitLoginAuthElement extends HTMLElement { uri: string, options: { appName?: string; label?: string } = {} ): Promise<"saved" | "declined" | "already-saved" | "unavailable"> { - if (this.vaultOffer || this.vaultRequest) return "unavailable"; + // The slot is claimed SYNCHRONOUSLY. The guard used to sit above three + // awaits, so two concurrent calls both passed it and the second + // overwrote the first's resolve -- dropping a promise that never settled. + if (this.vaultOffer || this.vaultRequest || this.offerInFlight) return "unavailable"; + this.offerInFlight = true; + try { + return await this.runOfferNwcConnection(uri, options); + } finally { + this.offerInFlight = false; + } + } + + private async runOfferNwcConnection( + uri: string, + options: { appName?: string; label?: string } + ): Promise<"saved" | "declined" | "already-saved" | "unavailable"> { const status = await this.worker.getSessionStatus().catch(() => ({ unlocked: false })); if (!status.unlocked) return "unavailable"; const vaultStatus = await this.worker.vaultStatus(); if (!vaultStatus.enabled) return "unavailable"; - const origin = window.location.origin; const check = await this.worker.vaultOfferCheck({ uri }); if (check.duplicate) return "already-saved"; diff --git a/packages/widget/src/worker/cryptoWorker.ts b/packages/widget/src/worker/cryptoWorker.ts index 8eace5c..5e819c5 100644 --- a/packages/widget/src/worker/cryptoWorker.ts +++ b/packages/widget/src/worker/cryptoWorker.ts @@ -601,36 +601,26 @@ async function handle(action: string, payload: unknown): Promise { } case "vaultOfferCheck": { - // The offer-to-save flow's dedupe: the same wallet + secret already - // stored means nothing NEW would enter the vault, so no consent screen - // is warranted. The origin binding is refreshed silently — the offering - // origin already HOLDS the credential (it supplied it), so binding is - // bookkeeping for later one-tap approvals, not a new grant. + // READ-ONLY on purpose. This used to publish an origin rebinding to + // relays -- a real, durable mutation of the user's account -- from a + // plain host API call, before any consent screen existed. Deduping is + // a question, not a write. If the caller wants the binding refreshed, + // that happens through the consented save path. const p = payload as { uri: string }; const origin = selfOrigin(); const credential = parseNwcUri(p.uri); - const { vault, connections } = await listLiveConnections(); + const { connections } = await listLiveConnections(); const existing = connections.find( (c) => c.record.connection_type === "nwc" && sameNwcCredential(c.record.credential as unknown as NwcCredential, credential) ); if (!existing) return { duplicate: false }; - if (existing.record.application_binding.origin !== origin) { - const { record, event } = await vault.updateConnection(existing, { - application_binding: { origin, app_pubkey: null } - }); - const publish = await vault.publish({ - event, - connectionId: record.connection_id, - relayUrls: vaultRelayUrls, - store - }); - if (publish.success) { - const decrypted = await vault.decryptEvent(event); - return { duplicate: true, connection: summarize(decrypted!) }; - } - } + // Only report it as already-saved when it is already THIS origin's. + // A wallet stored under another origin is not this caller's to claim + // silently; treating it as new sends the user through consent, which + // is the only place a cross-origin binding may legitimately change. + if (existing.record.application_binding.origin !== origin) return { duplicate: false }; return { duplicate: true, connection: summarize(existing) }; } @@ -639,7 +629,32 @@ async function handle(action: string, payload: unknown): Promise { } } +const KNOWN_ACTIONS = new Set([ + "configure", "register", "previewImportKey", "login", "recover", "completeRecovery", + "changePassword", "publishProfileAndRelayLists", "getPublicKey", "signEvent", + "nip44Encrypt", "nip44Decrypt", "nip04Encrypt", "nip04Decrypt", "exportIdentity", + "buildRecoveryExport", "repairReplicas", "getSessionStatus", "restoreSession", "logout", + "vaultStatus", "vaultList", "vaultSaveNwc", "vaultFindForOrigin", "vaultRevealNwc", + "vaultSetBinding", "vaultDelete", "vaultOfferCheck" +]); + self.addEventListener("message", (event: MessageEvent) => { + // Validate the envelope BEFORE destructuring. A malformed frame (or a + // stray postMessage from anything else on the page) used to throw straight + // out of this listener, which posts no response at all and leaves the + // caller's promise pending forever. + const frame = event.data as Partial | null; + if (!frame || typeof frame !== "object" || typeof frame.id !== "string") return; + if (typeof frame.action !== "string" || !KNOWN_ACTIONS.has(frame.action)) { + const response: WorkerResponse = { + id: frame.id, + ok: false, + error: `Unknown worker action: ${String(frame.action)}`, + errorName: "Error" + }; + (self as unknown as Worker).postMessage(response); + return; + } const { id, action, payload } = event.data; handle(action, payload).then( (result) => { diff --git a/packages/widget/src/worker/workerClient.ts b/packages/widget/src/worker/workerClient.ts index 5162b92..0cf671d 100644 --- a/packages/widget/src/worker/workerClient.ts +++ b/packages/widget/src/worker/workerClient.ts @@ -25,10 +25,21 @@ import type { VaultOfferCheckPayload } from "./protocol.js"; +/** + * How long a single worker call may take before the caller is told, rather + * than left hanging. Argon2id at 64 MiB plus a relay quorum round trip is the + * slow case; a minute is far past it and still finite. + */ +const CALL_TIMEOUT_MS = 60_000; + export class WorkerClient { private readonly worker: Worker; - private readonly pending = new Map void; reject: (error: Error) => void }>(); + private readonly pending = new Map< + string, + { resolve: (value: unknown) => void; reject: (error: Error) => void; timer: ReturnType } + >(); private counter = 0; + private dead = false; constructor() { // Deliberately NOT the literal `new Worker(new URL("./x.js", import.meta.url))` shape: Vite @@ -42,9 +53,13 @@ export class WorkerClient { this.worker = new Worker(workerUrl, { type: "module" }); this.worker.addEventListener("message", (event: MessageEvent) => { const msg = event.data; + // Shape-check before trusting: a malformed frame used to throw inside + // this listener, which posts no response and hangs the caller forever. + if (!msg || typeof msg !== "object" || typeof (msg as WorkerResponse).id !== "string") return; const entry = this.pending.get(msg.id); if (!entry) return; this.pending.delete(msg.id); + clearTimeout(entry.timer); if (msg.ok) { entry.resolve(msg.result); } else { @@ -53,12 +68,40 @@ export class WorkerClient { entry.reject(error); } }); + + // A worker that dies -- failed script load, an uncaught throw, the + // documented innerHTML-remount hazard -- must REJECT its callers, not + // strand them. Every hang reported against this widget traced back to a + // promise with nobody left to settle it. + const fail = (reason: string) => () => this.failAll(reason); + this.worker.addEventListener("error", fail("The BitLogin crypto worker stopped unexpectedly.")); + this.worker.addEventListener( + "messageerror", + fail("The BitLogin crypto worker sent an unreadable message.") + ); + } + + /** Settles every outstanding call with an error. Idempotent. */ + private failAll(reason: string): void { + const entries = [...this.pending.values()]; + this.pending.clear(); + for (const entry of entries) { + clearTimeout(entry.timer); + entry.reject(new Error(reason)); + } } private call(action: A, payload: WorkerActionMap[A][0]): Promise { + if (this.dead) { + return Promise.reject(new Error("This BitLogin session was torn down; reload the page to sign in again.")); + } const id = `${Date.now().toString(36)}-${(this.counter++).toString(36)}`; return new Promise((resolve, reject) => { - this.pending.set(id, { resolve: resolve as (value: unknown) => void, reject }); + const timer = setTimeout(() => { + this.pending.delete(id); + reject(new Error(`BitLogin's ${action} call timed out.`)); + }, CALL_TIMEOUT_MS); + this.pending.set(id, { resolve: resolve as (value: unknown) => void, reject, timer }); const request: WorkerRequest = { id, action, payload }; this.worker.postMessage(request); }); @@ -150,6 +193,10 @@ export class WorkerClient { } terminate(): void { + this.dead = true; this.worker.terminate(); + // terminate() silently kills in-flight work; without this every caller + // waits forever on a worker that no longer exists. + this.failAll("The BitLogin crypto worker was terminated."); } }