From b414cc53d296b12ac33325418dae86ec272f2469 Mon Sep 17 00:00:00 2001 From: thePR0M3TH3AN <53631862+PR0M3TH3AN@users.noreply.github.com> Date: Fri, 31 Jul 2026 09:00:24 -0400 Subject: [PATCH] Security audit: fix 8 findings across crypto, vault, and widget MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An adversarial audit (five independent reviewers over crypto/protocol, the vault, the widget, relay flows, and doc honesty) found real bugs in code this repo shipped. Fixed here, worst first: CRITICAL - the widget worker ignored application_binding.origin on vaultRevealNwc, so any caller naming a connection id could pull a wallet credential the user granted to a DIFFERENT site. The worker now derives its own origin (self.location) instead of trusting a payload field -- which also closes binding forgery via vaultSaveNwc / vaultSetBinding / vaultOfferCheck, where a host could bind another site's wallet to itself and inherit its one-tap approval. CRITICAL - VaultSession.destroy() left the vault root live (the root regenerates every derived key) and, worse, left the session USABLE with a zeroed prk: deriveRecordKey/deriveVaultSigningKey over 32 zero bytes are deterministic and world-computable, so a stale reference could seal a wallet secret under a key any observer can derive and report success. Now wipes the root and fails loudly. CRITICAL - changePassword published the tombstone that DESTROYS the old capsule before checking that the new one reached quorum: a failed rotation could leave neither password working, phrase-only recovery. Reordered to fail closed with the old password intact, plus a regression test that drives it with a partially-hostile relay set. HIGH - enableConnectionVault never checked quorum on the recovery read, so one stale relay could make it mint a SECOND vault root and orphan every stored credential -- the outcome its own header calls unforgivable. HIGH - the rollback high-water mark used strict `<` and never compared the event id it stored, so a same-second sibling of a revocation could resurrect a revoked credential forever. Ties now break on event id. HIGH - §11.10 claimed derived secrets are wiped; wipe() had two callers and neither was on the password or recovery path. The Argon2id output, both PRKs, and the 64-byte BIP-39 seed now die at their derivation site. MEDIUM - one malformed record threw out of the listing loop and made every OTHER connection unlistable; recovery lacked the already-registered-at-this-locator guard that registration and rotation both carry; the recovery chain check silently passed on gaps and forks. Synthetic (untrusted) clicks and submits no longer drive consent UI. Docs corrected where they overstated: the README's "private keys never leave the worker" now says plainly that a same-origin host can reach exportIdentity, and connection-vault.md gains §19 recording six OPEN limitations -- no CAS on capsule writes, per-device-only rollback state, tombstones not destroying prior ciphertext, the sudo key shipping with the root, fetches not failing closed, and a plaintext mark store. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01CMr9Yt8SYmZcfgrg3CV2G1 --- README.md | 20 +++++- docs/connection-vault.md | 38 +++++++++++ packages/core/src/account/account.e2e.test.ts | 29 +++++++++ packages/core/src/account/changePassword.ts | 21 ++++-- packages/core/src/account/enableVault.ts | 6 ++ packages/core/src/account/normalize.ts | 10 +++ packages/core/src/account/recover.ts | 42 +++++++++++- packages/core/src/capsules/validation.ts | 41 ++++++++++-- packages/core/src/test-support/mockRelay.ts | 8 +++ packages/core/src/vault/session.ts | 65 ++++++++++++++++--- packages/core/src/vault/sync.ts | 18 ++++- packages/widget/src/element.ts | 20 ++++-- packages/widget/src/worker/cryptoWorker.ts | 46 ++++++++++--- packages/widget/src/worker/protocol.ts | 7 +- packages/widget/src/worker/workerClient.ts | 4 +- 15 files changed, 329 insertions(+), 46 deletions(-) diff --git a/README.md b/README.md index ab4a87c..12dfe11 100644 --- a/README.md +++ b/README.md @@ -58,9 +58,23 @@ the site root. Once a user signs in, `window.nostr` is installed with the de facto NIP-07 shape (`getPublicKey`, `signEvent`, `getRelays`, `nip44.encrypt/decrypt`), so -existing Nostr web apps work against it unmodified. Private keys never leave -the worker; the main thread only ever sees public keys, signed events, and -ciphertext. See `packages/demo/public/docs.html` for the full integration +existing Nostr web apps work against it unmodified. Private keys are held in +the worker and are not handed to the main thread by the signing APIs — those +return public keys, signed events, and ciphertext only. + +**Be precise about what that does and does not buy you.** The worker protects +keys from *accidental* exposure through the normal API surface; it is not a +boundary against the host page itself. A page that embeds `` +runs same-origin with it and can reach the element's worker client directly, +so a malicious or XSS-compromised host can call `exportIdentity` and obtain +the `nsec`, and can synthesize UI events (the widget rejects untrusted ones, +and can still be visually overlaid by host CSS). This is inherent to an +embedded same-origin component — the same reason `docs/connection-vault.md` +§12.3 says wallet credentials are *revealed*, not brokered. Treat the host +page as trusted; embed BitLogin only in sites you control, and prefer the +recovery phrase (never the exported nsec) as the durable backup. A +cross-origin or extension build is what would change this, and is not +shipped. See `packages/demo/public/docs.html` for the full integration guide, and `packages/demo/public/account.html` for a working example (create → confirm recovery phrase → sign an event → rotate password → export identity). diff --git a/docs/connection-vault.md b/docs/connection-vault.md index 66cb6c7..407ece1 100644 --- a/docs/connection-vault.md +++ b/docs/connection-vault.md @@ -618,3 +618,41 @@ Rollback resistance (§9) is per record: a local high-water mark stores the newest accepted `created_at` per connection id, and `fetchConnectionRecordEvents` withholds and reports anything older instead of serving it — a stale replica must never silently resurrect a revoked credential. + + +## 19. Known limitations (audit, 2026-07-31) + +An adversarial audit of the implementation produced fixes for the worst +findings (origin binding now enforced worker-side; `destroy()` wipes the root +and poisons the session; equal-timestamp rollback tie-break; per-record +listing isolation; quorum enforced before the migration's mint decision). +These remain OPEN and are the honest limits of the current build: + +1. **No compare-and-swap on capsule writes.** Two devices enabling the vault + concurrently can each mint a root and write it, leaving the credential and + recovery capsules disagreeing. Mitigation today: enable the vault once, + from one device. A `previous_recovery_event_id` precondition check on + write is the fix. +2. **Rollback protection is per-device local state.** A fresh install has no + high-water marks, so the oldest record a hostile relay serves is accepted + and becomes the mark. A record-level generation counter inside the + encrypted payload would close this; the local mark cannot. +3. **A tombstone does not destroy the prior ciphertext.** Record keys are + derived from the immutable `connection_id`, so the superseded event — which + still contains the credential — remains decryptable by anyone who later + obtains the vault root, and relays may retain it regardless of NIP-09. + Deleting a connection in BitLogin is therefore NOT revocation: revoke at + the wallet or provider, which §11 already says and which this makes + cryptographically concrete. +4. **The sudo key ships with the vault root at every login**, because both + live in the same capsule under the same password-derived key. The + personal tier's separation is real against a stolen *record*, but the + "fresh Argon2id run to reveal" ceremony is a host-side convention, not a + cryptographic gate. Personal-tier UI should not ship until this is either + fixed (separate capsule) or the claim is weakened. +5. **Fetches do not fail closed on quorum failure**, and `limit: 500` + truncates silently — a single relay can shape a new device's view of the + vault. +6. **The high-water-mark store is plaintext and unauthenticated**, leaking the + derived vault pubkey and the connection inventory to same-origin storage + access, and is tamperable to disable rollback detection. diff --git a/packages/core/src/account/account.e2e.test.ts b/packages/core/src/account/account.e2e.test.ts index 27a6d03..ff8e5e2 100644 --- a/packages/core/src/account/account.e2e.test.ts +++ b/packages/core/src/account/account.e2e.test.ts @@ -394,6 +394,35 @@ describe("BitLogin Phase 0 end-to-end scenarios (§32)", () => { ARGON2_TIMEOUT * 2 ); + it( + "a rotation that cannot publish the new capsule leaves the OLD password working (§18.1 ordering)", + async () => { + // The tombstone DESTROYS the old capsule. Publishing it before the new + // capsule is known durable can leave an account with neither password -- + // recoverable only by phrase. Failing closed keeps the old one alive. + const loginName = "ordering"; + const oldPassword = "old ordering password 41"; + const newPassword = "new ordering password 42"; + await registerAccount({ loginName, password: oldPassword, vaultRelayUrls }); + + // Every relay refuses the NEW locator's writes while still accepting + // reads and the tombstone -- the partially-hostile quorum case. + const newLocator = getPublicKeyHex( + (await derivePasswordKeys(newPassword, normalizeLoginName(loginName))).locatorPrivateKey + ); + for (const relay of relays) relay.refusePublishFrom.add(newLocator); + await expect( + changePassword({ loginName, oldPassword, newPassword, vaultRelayUrls }) + ).rejects.toThrow(/still works|quorum/i); + for (const relay of relays) relay.refusePublishFrom.clear(); + + // The old password must still sign in, and the new one must not exist. + const login = await loginWithPassword({ loginName, password: oldPassword, vaultRelayUrls }); + expect(login.everydayPublicKey).toMatch(/^[0-9a-f]{64}$/); + }, + 45_000 + ); + it( "rotating to a new password that collides with another account under the same login name is refused (§18.1)", async () => { diff --git a/packages/core/src/account/changePassword.ts b/packages/core/src/account/changePassword.ts index 91f5327..856d7e5 100644 --- a/packages/core/src/account/changePassword.ts +++ b/packages/core/src/account/changePassword.ts @@ -140,6 +140,21 @@ export async function changePassword(params: ChangePasswordParams): Promise 0) { + throw new RecoveryFailedError( + "Another account is already registered with that login name and password. Pick a different one." + ); + } + const credentialPayload: CredentialPayload = { schema: SCHEMA_CREDENTIAL_V1, account_id: recovered.accountId, diff --git a/packages/core/src/capsules/validation.ts b/packages/core/src/capsules/validation.ts index 59ccfca..d07d15a 100644 --- a/packages/core/src/capsules/validation.ts +++ b/packages/core/src/capsules/validation.ts @@ -139,20 +139,49 @@ export function validateRecoveryPayload(payload: RecoveryPayload): void { export function checkRecoveryChainConsistency( generations: Array<{ eventId: string; recoveryGeneration: number; previousRecoveryEventId: string | null }> ): { consistent: boolean; warning?: string } { - const byGeneration = new Map(generations.map((g) => [g.recoveryGeneration, g])); + // §12.3 claims this detects gaps AND replays; the first version detected + // neither. A Map keyed by generation SILENTLY COLLAPSED two capsules + // claiming the same generation -- a fork, i.e. the exact replay signal -- + // and a missing generation made the link check `if (prior && ...)` skip + // entirely, so {1, 3} with 2 absent reported "consistent". + const byGeneration = new Map(); + for (const entry of generations) { + const existing = byGeneration.get(entry.recoveryGeneration); + if (existing && existing.eventId !== entry.eventId) { + return { + consistent: false, + warning: `Two different recovery capsules both claim generation ${entry.recoveryGeneration}: possible fork, replay, or relay misbehavior.` + }; + } + byGeneration.set(entry.recoveryGeneration, entry); + } + const sorted = [...generations].sort((a, b) => a.recoveryGeneration - b.recoveryGeneration); for (let i = 1; i < sorted.length; i++) { const current = sorted[i]!; - const prior = byGeneration.get(current.recoveryGeneration - 1); if (current.previousRecoveryEventId === null) { - return { consistent: false, warning: `Generation ${current.recoveryGeneration} has a null previous-event link but is not the first generation.` }; - } - if (prior && current.previousRecoveryEventId !== prior.eventId) { return { consistent: false, - warning: `Recovery generation chain is broken between generation ${prior.recoveryGeneration} and ${current.recoveryGeneration}: possible replay or relay misbehavior.` + warning: `Generation ${current.recoveryGeneration} has a null previous-event link but is not the first generation.` + }; + } + const prior = byGeneration.get(current.recoveryGeneration - 1); + if (!prior) { + // A gap is not proof of misbehavior -- relays legitimately drop old + // generations -- but it means the chain CANNOT be verified across it, + // and reporting that as "consistent" was the lie worth fixing. + return { + consistent: false, + warning: `Recovery generation ${current.recoveryGeneration - 1} is missing, so the chain up to generation ${current.recoveryGeneration} cannot be verified.` + }; + } + if (current.previousRecoveryEventId !== prior.eventId) { + return { + consistent: false, + warning: `Recovery generation chain is broken between generation ${current.recoveryGeneration - 1} and ${current.recoveryGeneration}: possible replay or relay misbehavior.` }; } } return { consistent: true }; } + diff --git a/packages/core/src/test-support/mockRelay.ts b/packages/core/src/test-support/mockRelay.ts index aacbdc0..0ae117f 100644 --- a/packages/core/src/test-support/mockRelay.ts +++ b/packages/core/src/test-support/mockRelay.ts @@ -29,6 +29,10 @@ export class MockRelay { private events = new Map(); private plainEvents: NostrEvent[] = []; public requireAuthForKinds: Set = new Set(); + /** Pubkeys whose publishes are refused. Models the partially-hostile relay + * that accepts some writes and drops others -- the case that makes write + * ORDERING a safety property rather than a style choice. */ + public refusePublishFrom: Set = new Set(); private constructor(wss: WebSocketServer, port: number) { this.wss = wss; @@ -69,6 +73,10 @@ export class MockRelay { socket.send(JSON.stringify(["OK", event.id, false, "auth-required: publish requires NIP-42 AUTH"])); return; } + if (this.refusePublishFrom.has(event.pubkey)) { + socket.send(JSON.stringify(["OK", event.id, false, "blocked: refused by test relay"])); + return; + } this.store(event); socket.send(JSON.stringify(["OK", event.id, true, ""])); return; diff --git a/packages/core/src/vault/session.ts b/packages/core/src/vault/session.ts index da0a613..46133c6 100644 --- a/packages/core/src/vault/session.ts +++ b/packages/core/src/vault/session.ts @@ -47,10 +47,16 @@ export interface CreateConnectionParams { export class VaultSession { private vaultPrk: Uint8Array; private personalPrk: Uint8Array | null = null; + private destroyed = false; + private readonly connectionVaultRoot: Uint8Array; readonly vaultPublicKey: string; - constructor(private readonly connectionVaultRoot: Uint8Array) { - this.vaultPrk = deriveVaultPrk(connectionVaultRoot); + constructor(connectionVaultRoot: Uint8Array) { + // A private COPY: the caller's buffer is theirs to wipe on their own + // schedule, and destroy() must be able to erase this session's copy + // without reaching into memory it does not own. + this.connectionVaultRoot = connectionVaultRoot.slice(); + this.vaultPrk = deriveVaultPrk(this.connectionVaultRoot); this.vaultPublicKey = deriveVaultPublicKey(this.vaultPrk); } @@ -71,13 +77,29 @@ export class VaultSession { this.personalPrk = null; } - /** Wipes everything this session holds. The session is unusable after. */ + /** + * Wipes everything this session holds, permanently. + * + * The ROOT is wiped too, not just the derived prk: the root regenerates the + * prk and (with a sudo key) the personal prk, so leaving it live made + * "wipes everything" false. And the session is marked destroyed rather than + * merely zeroed, because a zeroed prk is not inert -- deriveRecordKey and + * deriveVaultSigningKey over 32 zero bytes are deterministic, WORLD- + * COMPUTABLE values. A stale reference calling createConnection() after + * teardown would otherwise seal a wallet secret under a key any observer + * can derive, sign it with a publicly known identity, and report success. + * Failing loudly is the only safe behaviour. + */ destroy(): void { this.endSudo(); - wipe(this.vaultPrk); + wipe(this.vaultPrk, this.connectionVaultRoot); + this.destroyed = true; } private prkForTier(tier: ConnectionTier): Uint8Array { + if (this.destroyed) { + throw new Error("This vault session was destroyed; create a new one from the account's root."); + } if (tier === "connectable") return this.vaultPrk; if (!this.personalPrk) { throw new Error("Personal-tier records require an open sudo window (enableSudo)."); @@ -155,6 +177,9 @@ export class VaultSession { /** Trial-decrypts one event; personal-tier records resolve only inside a sudo window. */ decryptEvent(event: NostrEvent): Promise { + if (this.destroyed) { + return Promise.reject(new Error("This vault session was destroyed; create a new one from the account's root.")); + } return decryptConnectionRecordEvent(event, this.vaultPrk, this.personalPrk ?? undefined); } @@ -162,6 +187,9 @@ export class VaultSession { * signed by the vault identity. Best-effort by contract — the encrypted * tombstone is the durable part of a deletion, this is the courtesy ask. */ buildDeletionRequest(eventIdToDelete: string, now?: number): NostrEvent { + if (this.destroyed) { + throw new Error("This vault session was destroyed; create a new one from the account's root."); + } return buildDeletionRequest({ privateKey: deriveVaultSigningKey(this.vaultPrk), eventIdToDelete, @@ -198,14 +226,35 @@ export class VaultSession { relayUrls: string[]; store: KeyValueStore; timeoutMs?: number; - }): Promise<{ connections: DecryptedConnectionRecord[]; rollbackWarnings: string[]; quorumMet: boolean }> { + }): Promise<{ + connections: DecryptedConnectionRecord[]; + rollbackWarnings: string[]; + /** Event ids that failed to decrypt or validate. Surfaced, never silent. */ + unreadable: string[]; + quorumMet: boolean; + }> { const fetched = await this.fetchEvents(params); const connections: DecryptedConnectionRecord[] = []; + const unreadable: string[] = []; for (const event of fetched.events.values()) { - const decrypted = await this.decryptEvent(event); - if (decrypted) connections.push(decrypted); + // Per-record isolation. decryptConnectionRecordEvent THROWS on a bad + // signature, wrong author, malformed d-tag, id mismatch, or tier + // mismatch -- all correct strictness -- but without this catch a single + // malformed or future-version record made every OTHER connection, + // including working wallet credentials, unlistable on every device. + try { + const decrypted = await this.decryptEvent(event); + if (decrypted) connections.push(decrypted); + } catch { + unreadable.push(event.id); + } } connections.sort((a, b) => b.record.updated_at - a.record.updated_at); - return { connections, rollbackWarnings: fetched.rollbackWarnings, quorumMet: fetched.quorumMet }; + return { + connections, + rollbackWarnings: fetched.rollbackWarnings, + unreadable, + quorumMet: fetched.quorumMet + }; } } diff --git a/packages/core/src/vault/sync.ts b/packages/core/src/vault/sync.ts index 2f657dc..5d1f4fa 100644 --- a/packages/core/src/vault/sync.ts +++ b/packages/core/src/vault/sync.ts @@ -46,7 +46,13 @@ export async function raiseRecordHighWaterMark( observed: RecordHighWaterMark ): Promise { const current = await getRecordHighWaterMark(store, vaultPubkey, connectionId); - if (current && current.createdAt >= observed.createdAt) return; + if (current && current.createdAt > observed.createdAt) return; + // Equal timestamps are NOT a no-op: two devices editing the same record in + // the same second both compute nextCreatedAt(T) = T+1, so a benign edit can + // land at the exact timestamp of a REVOCATION. Ties break on event id (the + // addressable-event convention) so the mark advances deterministically + // rather than depending on which write arrived first. + if (current && current.createdAt === observed.createdAt && current.eventId <= observed.eventId) return; await store.set(hwmKey(vaultPubkey, connectionId), JSON.stringify(observed)); } @@ -128,7 +134,15 @@ export async function fetchConnectionRecordEvents(params: { const rollbackWarnings: string[] = []; for (const [connectionId, event] of newestById) { const hwm = await getRecordHighWaterMark(params.store, vaultPubkey, connectionId); - if (hwm && event.created_at < hwm.createdAt) { + // Strict `<` alone let a same-second sibling of a revocation through + // forever (the relay may keep both events at one address). An equal + // timestamp is only acceptable from the SAME event, or a later-sorting + // id under the same tie-break the mark itself uses. + const stale = + hwm !== null && + (event.created_at < hwm.createdAt || + (event.created_at === hwm.createdAt && event.id < hwm.eventId)); + if (stale) { rollbackWarnings.push(connectionId); continue; } diff --git a/packages/widget/src/element.ts b/packages/widget/src/element.ts index 036f9bc..a00122b 100644 --- a/packages/widget/src/element.ts +++ b/packages/widget/src/element.ts @@ -278,7 +278,7 @@ export class BitLoginAuthElement extends HTMLElement { const vaultStatus = await this.worker.vaultStatus(); if (!vaultStatus.enabled) return "unavailable"; const origin = window.location.origin; - const check = await this.worker.vaultOfferCheck({ uri, origin }); + const check = await this.worker.vaultOfferCheck({ uri }); if (check.duplicate) return "already-saved"; const appName = options.appName?.trim() || window.location.hostname || "This app"; @@ -311,7 +311,7 @@ export class BitLoginAuthElement extends HTMLElement { this.setBusy(true); try { const label = this.field("vaultOfferLabel").trim() || offer.label; - await this.worker.vaultSaveNwc({ uri: offer.uri, label, origin: window.location.origin }); + await this.worker.vaultSaveNwc({ uri: offer.uri, label }); this.setBusy(false); this.finishVaultOffer("saved"); this.flashSuccess(this.screen, "Wallet saved"); @@ -353,7 +353,7 @@ export class BitLoginAuthElement extends HTMLElement { return; } this.vaultUnsaved = false; - const found = await this.worker.vaultFindForOrigin({ origin: request.origin }); + const found = await this.worker.vaultFindForOrigin(); if (found.connection) { this.vaultCandidate = found.connection; this.goto("vault-consent"); @@ -416,7 +416,7 @@ export class BitLoginAuthElement extends HTMLElement { return; } const label = labelDraft.trim() || `${request.appName} wallet`; - await this.worker.vaultSaveNwc({ uri, label, origin: request.origin }); + await this.worker.vaultSaveNwc({ uri, label }); this.setBusy(false); this.finishVaultRequest(uri); this.flashSuccess(this.screen, "Wallet connected"); @@ -623,6 +623,14 @@ export class BitLoginAuthElement extends HTMLElement { } private async onClick(e: Event): Promise { + // A synthetic click is not consent. The shadow root is open (by design, + // for host theming), so a host page can find and .click() any button in + // here -- including "approve this wallet" and "reveal my nsec". isTrusted + // is false for anything script-dispatched. This does NOT make the widget + // a security boundary against its host (see connection-vault.md §12.3; + // the host can still style the real button under its own), but it closes + // the zero-effort version where no human ever sees a screen. + if (!e.isTrusted) return; const target = (e.target as HTMLElement).closest("[data-action]"); if (!target) return; const action = target.dataset.action!; @@ -752,6 +760,8 @@ export class BitLoginAuthElement extends HTMLElement { private async onSubmit(e: Event): Promise { e.preventDefault(); + if (!e.isTrusted) return; // see onClick: synthetic submits are not consent + const form = e.target as HTMLFormElement; const formName = form.dataset.form; try { @@ -1431,7 +1441,7 @@ export class BitLoginAuthElement extends HTMLElement { return `

Signed in

${this.renderWarnings()} -

${this.session?.npub ?? ""}

+

${escapeHtml(this.session?.npub ?? "")}

${this.renderError()} ${ diff --git a/packages/widget/src/worker/cryptoWorker.ts b/packages/widget/src/worker/cryptoWorker.ts index 4b78eb6..8eace5c 100644 --- a/packages/widget/src/worker/cryptoWorker.ts +++ b/packages/widget/src/worker/cryptoWorker.ts @@ -149,6 +149,24 @@ async function persistSession(): Promise { }); } +/** + * The origin this worker actually runs in, derived rather than accepted. + * + * Every vault call used to take `origin` from the message payload, which made + * the per-origin binding -- the vault's ENTIRE authorization model -- advisory: + * a caller could claim any origin, bind another site's wallet to itself, or + * plant a wallet under a victim origin. The worker is same-origin with its + * page and can simply look. A dedicated worker's self.location.origin is the + * origin of the script's URL, which is the embedding page's origin. + */ +function selfOrigin(): string { + try { + return new URL(self.location.href).origin; + } catch { + return ""; + } +} + function requireVault(): VaultSession { if (!session.signer) throw new Error("No identity is unlocked in this session."); if (!session.connectionVaultRoot) { @@ -482,7 +500,8 @@ async function handle(action: string, payload: unknown): Promise { } case "vaultSaveNwc": { - const p = payload as { uri: string; label: string; origin: string | null }; + const p = payload as { uri: string; label: string }; + const origin = selfOrigin(); const vault = requireVault(); const credential = parseNwcUri(p.uri); const label = p.label.trim().slice(0, 120) || "Wallet connection"; @@ -491,7 +510,7 @@ async function handle(action: string, payload: unknown): Promise { tier: "connectable", label, credential: credential as unknown as { schema: string } & Record, - application_binding: { origin: p.origin, app_pubkey: null } + application_binding: { origin, app_pubkey: null } }); const publish = await vault.publish({ event, @@ -509,13 +528,13 @@ async function handle(action: string, payload: unknown): Promise { } case "vaultFindForOrigin": { - const p = payload as { origin: string }; + const origin = selfOrigin(); const { connections } = await listLiveConnections(); const match = connections.find( (c) => c.record.connection_type === "nwc" && c.record.state === "active" && - c.record.application_binding.origin === p.origin + c.record.application_binding.origin === origin ); return { connection: match ? summarize(match) : null }; } @@ -526,6 +545,16 @@ async function handle(action: string, payload: unknown): Promise { if (connection.record.connection_type !== "nwc" || connection.record.state !== "active") { throw new Error("That connection is not an active wallet connection."); } + // THE authorization check. Reveal mode concedes that the requesting + // origin receives the credential IT was granted (connection-vault.md + // §12.3) -- it concedes nothing about OTHER origins' grants. Without + // this, any caller that can name a connection id (they appear in the + // management UI's markup) could pull a wallet the user connected to a + // different site entirely. + const boundOrigin = connection.record.application_binding.origin; + if (boundOrigin !== null && boundOrigin !== selfOrigin()) { + throw new Error("That connection belongs to a different site."); + } const credential = connection.record.credential as unknown as NwcCredential; validateNwcCredential(credential); return { uri: toNwcUri(credential) }; @@ -535,7 +564,7 @@ async function handle(action: string, payload: unknown): Promise { const p = payload as { connectionId: string; origin: string | null }; const { vault, connection } = await findConnection(p.connectionId); const { record, event } = await vault.updateConnection(connection, { - application_binding: { origin: p.origin, app_pubkey: null } + application_binding: { origin, app_pubkey: null } }); const publish = await vault.publish({ event, @@ -577,7 +606,8 @@ async function handle(action: string, payload: unknown): Promise { // 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. - const p = payload as { uri: string; origin: string | null }; + const p = payload as { uri: string }; + const origin = selfOrigin(); const credential = parseNwcUri(p.uri); const { vault, connections } = await listLiveConnections(); const existing = connections.find( @@ -586,9 +616,9 @@ async function handle(action: string, payload: unknown): Promise { sameNwcCredential(c.record.credential as unknown as NwcCredential, credential) ); if (!existing) return { duplicate: false }; - if (p.origin !== null && existing.record.application_binding.origin !== p.origin) { + if (existing.record.application_binding.origin !== origin) { const { record, event } = await vault.updateConnection(existing, { - application_binding: { origin: p.origin, app_pubkey: null } + application_binding: { origin, app_pubkey: null } }); const publish = await vault.publish({ event, diff --git a/packages/widget/src/worker/protocol.ts b/packages/widget/src/worker/protocol.ts index fce7956..a7c672e 100644 --- a/packages/widget/src/worker/protocol.ts +++ b/packages/widget/src/worker/protocol.ts @@ -197,12 +197,10 @@ export interface VaultListResult { export interface VaultSaveNwcPayload { uri: string; label: string; - origin: string | null; } -export interface VaultFindForOriginPayload { - origin: string; -} +/** No origin field: the worker derives its own (see selfOrigin). */ +export type VaultFindForOriginPayload = Record; export interface VaultRevealNwcPayload { connectionId: string; @@ -222,7 +220,6 @@ export interface VaultDeletePayload { export interface VaultOfferCheckPayload { uri: string; - origin: string | null; } /** duplicate=true means the same wallet+secret is already stored; the worker * has silently refreshed its origin binding, and no consent UI is needed — diff --git a/packages/widget/src/worker/workerClient.ts b/packages/widget/src/worker/workerClient.ts index 98a54e8..5162b92 100644 --- a/packages/widget/src/worker/workerClient.ts +++ b/packages/widget/src/worker/workerClient.ts @@ -130,8 +130,8 @@ export class WorkerClient { vaultSaveNwc(payload: VaultSaveNwcPayload) { return this.call("vaultSaveNwc", payload); } - vaultFindForOrigin(payload: VaultFindForOriginPayload) { - return this.call("vaultFindForOrigin", payload); + vaultFindForOrigin() { + return this.call("vaultFindForOrigin", {}); } vaultRevealNwc(payload: VaultRevealNwcPayload) { return this.call("vaultRevealNwc", payload);