Skip to content

fix: accept NIP-46 connect responses from signers that echo the URI secret#2947

Open
DocNR wants to merge 2 commits into
stackernews:masterfrom
DocNR:fix/nip46-spec-compliant-connect-response
Open

fix: accept NIP-46 connect responses from signers that echo the URI secret#2947
DocNR wants to merge 2 commits into
stackernews:masterfrom
DocNR:fix/nip46-spec-compliant-connect-response

Conversation

@DocNR
Copy link
Copy Markdown

@DocNR DocNR commented May 9, 2026

Summary

Fixes the NIP-46 bunker login hanging forever on the "Waiting for authorization" spinner when paired with spec-compliant signers like Clave. The root cause is two NIP-46 spec violations in NDK's NDKNip46Signer.blockUntilReady — present in every released NDK from 2.12.2 (this repo's pinned version) through 3.0.3.

Per the NIP-46 spec:

connect: [<remote-signer-pubkey>, <optional_secret>, <optional_requested_perms>]
result: "ack" OR <required-secret-value>

NDK violates both:

  1. Sends connect([userPubkey ?? "", secret]). For bunker://... URIs without ?pubkey= (the typical single-user signer case — Clave, etc.) userPubkey is null, so an empty string lands as the first param.
  2. Only accepts response.result === "ack", rejecting the spec-allowed alternative (the URI secret echoed back) with response.error — which is undefined when the bunker returned a successful-shaped response. Our setError(e) in components/nostr-auth.js then throws on e.message || e.toString(), leaving the spinner stuck.

This PR extends the existing NDKNip46SignerURLPatch (from #1636) with an overridden blockUntilReady that:

  • Sends bunkerPubkey as the first connect param when userPubkey is null (per spec)
  • Accepts both "ack" and the URI secret echoed back (per spec)
  • Defers to super.blockUntilReady() for the NIP-05 path (unchanged)
  • Wraps unexpected-result rejections in a real Error so the catch handler doesn't TypeError on undefined

The override can be deleted once we bump to an NDK that fixes both bugs upstream — companion PR filed at nostr-dev-kit/ndk#390.

Test plan

Verified end-to-end against @nostr-dev-kit/ndk@2.12.2 (the version pinned here) using a Clave-emulating mock NIP-46 bunker on a local nak serve relay, plus a real Clave bunker URI from a TestFlight device:

Mode Bunker response Outcome
Unpatched NDK 2.12.2 secret echo (Clave-style) ❌ rejects in 39ms with undefined (the bug)
Patched override secret echo (Clave-style) ✅ resolves in 112ms
Patched override "ack" (nsec.app-style) ✅ resolves in 110ms
Patched override real Clave URI on relay.powr.build ✅ resolves in 1990ms
  • Reproduces the original hang against unpatched NDK
  • Patched override completes the connect handshake against a Clave-style secret-echo response
  • Patched override still works with "ack" responses (regression — no breakage for nsec.app etc.)
  • Live test against a real Clave TestFlight build's bunker URI on relay.powr.build
  • NIP-05 login path is unchanged — defers to super.blockUntilReady() explicitly

🤖 Generated with Claude Code

…ecret

NDK's NDKNip46Signer.blockUntilReady has two NIP-46 spec violations
present in every released NDK from 2.12.2 through 3.0.3:

1. It sends connect([userPubkey ?? "", secret]). For bunker URIs without
   ?pubkey= (the typical single-user signer case), this passes an empty
   first param. Per NIP-46 the first param is the remote-signer-pubkey,
   not the user-pubkey.

2. It only accepts response.result === "ack" and rejects everything
   else with response.error -- which is undefined when a signer returns
   the spec-allowed alternative (the URI secret echoed back). Per
   NIP-46, valid `connect` responses are "ack" OR the URI secret
   echoed back. Spec-compliant signers like Clave
   (https://github.com/DocNR/clave) and any signer matching nostr-tools'
   BunkerSigner.fromURI return the secret-echo form, which made
   blockUntilReady() reject with undefined and our setError(e) later
   throw on e.message -- leaving the login spinner stuck on "Waiting
   for authorization" forever.

Extends the existing NDKNip46SignerURLPatch (originally for the
non-special-scheme URL parsing issue, PR stackernews#1636) with a corrected
blockUntilReady that:

- Sends bunkerPubkey as the first connect param when userPubkey is
  null (matches the spec)
- Accepts response.result === "ack" OR === the URI secret
- Defers to super.blockUntilReady() for the NIP-05 path (unchanged)

Verified with a Clave-emulating mock bunker against NDK 2.12.2
(the version pinned in package.json):

- Unpatched: rejects in 39ms with `undefined` (TypeError on setError(e))
- Patched (Clave-style secret-echo response): resolves in 112ms
- Patched (nsec.app-style "ack" response): resolves in 110ms

The override can be removed once we bump to an NDK that fixes both
bugs upstream. Tracking issue/PR to be filed on nostr-dev-kit/ndk
in parallel.
@Kampouse
Copy link
Copy Markdown

so i was not tripping

@DocNR
Copy link
Copy Markdown
Author

DocNR commented May 12, 2026

lol nah... there are a few other clients that use NDK that I've noticed have this issue.

@Kampouse
Copy link
Copy Markdown

The only one seems good now for login is
https://app.nostrmail.org/inbox But its in dart 😂

@Kampouse
Copy link
Copy Markdown

I copied the dart code and got my app to work again

@Kampouse
Copy link
Copy Markdown

https://github.com/Kampouse/nip46-connect-demo
working example ported from the flutter example
https://nip46-demo.pages.dev

Copy link
Copy Markdown

@Vinayak1337 Vinayak1337 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this still leaves one spec-compliant bunker token shape broken.

The new override says it fixes the first connect parameter by sending the remote-signer pubkey, but the implementation still does this:

const connectParams = [this.userPubkey || this.bunkerPubkey]

That means any bunker URI that has both a bunker pubkey in the host and ?pubkey=<user pubkey> will still send the user pubkey as params[0]. In NDK's parser, userPubkey comes from the query string, while bunkerPubkey comes from the token host. Current NIP-46 defines connect as [<remote-signer-pubkey>, <optional_secret>, ...], and nostr-tools' BunkerSigner.connect() also sends [this.bp.pubkey, this.bp.secret || ""].

So for a multi-user signer, or any signer that includes the expected user key in ?pubkey=... but validates connect.params[0] against its own signer pubkey, this PR will still reject or hang even though the token is otherwise valid. The narrow fix is to always send this.bunkerPubkey as the first connect param, then keep the existing get_public_key call after a successful ack/secret echo to resolve and store this.userPubkey.

A regression test with distinct bunkerPubkey and userPubkey would lock this down and also document why this override differs from NDK's current this.userPubkey ?? "" behavior.

Review feedback (stackernews#2947): the previous
`[this.userPubkey || this.bunkerPubkey]` still sent the user pubkey
as connect.params[0] whenever the bunker URI carried `?pubkey=`.

Per NIP-46, connect.params[0] is the remote-signer-pubkey — always
bunkerPubkey (the URI host). The `?pubkey=` value is the user
identity, resolved separately via the get_public_key call after the
handshake. Sending userPubkey breaks multi-user signers (and any
signer that validates params[0] against its own signer pubkey).
nostr-tools' BunkerSigner sends `this.bp.pubkey` likewise.
@DocNR
Copy link
Copy Markdown
Author

DocNR commented May 14, 2026

Confirmed. Verified against the NIP-46 spec (connect: [<remote-signer-pubkey>, <optional_secret>, ...]) and nostr-tools BunkerSigner.connect() (nip46.js in 2.23.3 sends [this.bp.pubkey, this.bp.secret || ""] — the bunker pubkey — then calls get_public_key afterward).

this.userPubkey || this.bunkerPubkey was a half-fix: it only fell through to bunkerPubkey when userPubkey was null, so a bunker://<host>?pubkey=<user> URI still sent the user pubkey as params[0].

Fixed in a007b45connectParams is now unconditionally [this.bunkerPubkey]. bunkerPubkey is guaranteed set by the guard at the top of the override, and the get_public_key call after the handshake still resolves and stores userPubkey (in the ?pubkey= case NDK's getPublicKey() returns the already-set value without a round trip, so it's a no-op cost).

On the regression test: this repo has no existing tests that touch lib/nostr.js or mock NDK internals, so I put the distinct-bunkerPubkey/userPubkey regression test in the upstream NDK PR — nostr-dev-kit/ndk#390 — where core/src/signers/nip46/index.test.ts already has the mock-RPC harness and where the canonical blockUntilReady logic lives (NDK has the identical this.userPubkey ?? "" bug on its bunker path). That test asserts params[0] === bunkerPubkey when the two are distinct. Once #390 lands and this repo bumps NDK, this whole NDKNip46SignerURLPatch override can be deleted. Happy to add a stackernews-side test too if you'd prefer the coverage local — it'd be the first NDK-mocking test in the jest suite, so I left it out by default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants