fix(auth): treat NIP-46 auth challenges as prompts, not failures - #531
fix(auth): treat NIP-46 auth challenges as prompts, not failures#531rabble wants to merge 12 commits into
Conversation
Deploying divine-web with
|
| Latest commit: |
0566cf4
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d112af8a.divine-web.pages.dev |
| Branch Preview URL: | https://fix-profile-grid-and-bunker.divine-web.pages.dev |
🚀 Preview DeploymentLast updated:
|
realmeylisdev
left a comment
realmeylisdev
left a comment
There was a problem hiding this comment.
Fixes 1 and 2 look right. I checked the dataLength guard in react-infinite-scroll-component@6.1.0 (dist/index.js:374-377) and the offset math in fetchUserVideos — the diagnosis holds, and the scroll tests reproduce the clamp honestly.
Fix 3 has the right diagnosis too (NConnectSigner.cmd() does throw on any non-empty error, and send() does return out of the for await). But moving to nostr-tools' BunkerSigner drops three things @nostrify was providing, and two of them undo the half of #485 this is meant to fix. Separately, one commit carries files that don't belong to it.
Four things, all inline:
- A
SimplePool+ open socket peruseCurrentUser()consumer, never closed - Mid-session auth challenges silently dropped — no tab, no link, nothing
- The 60s request timeout is gone; the dialog can sit on "Connecting…" forever
4740c7eships an unrelated failing Playwright spec plus a gitignored run artifact
Nice catch in passing, worth calling out: bunkerSignerFromLogin targets data.bunkerPubkey, where NUser.fromBunkerLogin was p-tagging login.pubkey. NIP-46 requires those to be distinct, so this quietly fixes a real bug for signers whose remote-signer key isn't the user key.
Ran locally: vitest 1749/1749, tsc -p tsconfig.app.json --noEmit clean, eslint src/ 0 errors / 17 warnings — matches your numbers.
| // Not NUser.fromBunkerLogin: that builds a signer which treats a NIP-46 | ||
| // auth challenge as a fatal error, so a signer that asks for approval | ||
| // mid-session breaks every subsequent request (divine-web#485). | ||
| return new NUser('bunker', login.pubkey, bunkerSignerFromLogin(login.data)); |
There was a problem hiding this comment.
Every consumer builds its own pool, and nothing closes it.
createUserFromLogin runs inside a useMemo in both useCurrentUser (65 call sites) and useLoggedInAccounts — in the latter it's called purely as a validity probe and the result is discarded. Each call now reaches BunkerSigner.fromBunker, which does new SimplePool() and calls setupSubscription() eagerly, so a socket to every bunker relay opens at construction.
Measured: three components calling useCurrentUser() produce three signers and zero close() calls, including after unmount; five constructions open five distinct connections against a local ws server. close() is on BunkerNostrSigner but nothing in src/ ever calls it.
NUser.fromBunkerLogin(login, nostr) shared the app's NPool via pool.group(relays), and NConnectSigner only opened a REQ per request and tore it down on response — so this is new. Either thread the app's pool into BunkerSigner via params.pool, or memoize one signer per login id outside the render path.
| { pubkey: bunkerPubkey, relays, secret }, | ||
| { | ||
| onauth: (url: string) => { | ||
| onAuthChallenge?.(presentAuthChallenge(url)); |
There was a problem hiding this comment.
The ongoing-session path never presents the challenge.
createUserFromLogin calls bunkerSignerFromLogin(login.data) with no handler, so onAuthChallenge is undefined here. An optional call doesn't evaluate its arguments when the callee is nullish, so presentAuthChallenge(url) never runs — a mid-session challenge opens no tab and surfaces no link.
The subscription staying alive is the real half of the fix and that part works. But "a mid-session challenge would break every later request" reads as solved in the PR body, and from the user's side it isn't: they're simply never told to approve.
Calling presentAuthChallenge(url) unconditionally and routing the result to a toast would cover the session path without touching the login flow.
| export function createBunkerSigner(options: CreateBunkerSignerOptions): BunkerNostrSigner { | ||
| const { clientSecretKey, bunkerPubkey, relays, secret = null, onAuthChallenge } = options; | ||
|
|
||
| const bunker = BunkerSigner.fromBunker( |
There was a problem hiding this comment.
The 60s request timeout is gone.
NLogin.fromBunker and NUser.fromBunkerLogin both passed timeout: 60_000 into NConnectSigner. BunkerSigner.sendRequest has none — the promise settles only if a matching response arrives.
So a signer that's unreachable but whose relay accepts the publish leaves loginWithBunker pending forever: LoginDialog's finally never runs, isLoginLoading stays true, and errorBunkerConnectFailed never renders. The dialog just sits on "Connecting…". Same for signEvent in an established session — and combined with the swallowed challenge above, a mid-session auth_url is an unbounded hang with no UI at all.
divine-connect hits the same fork and keeps a 60s bound (src/services/nip46_client.rs:283) with copy that names the approval step. A timeout that restarts on auth_url would cover both the waiting-on-approval case and the dead-signer case.
| @@ -0,0 +1,140 @@ | |||
| // ABOUTME: axe coverage for /notifications, which is login-gated and so cannot be reached by the public route sweep | |||
There was a problem hiding this comment.
This is unrelated to the PR, and it fails.
Nothing here touches NIP-46, pagination, or scroll restoration, and the PR body doesn't mention it — it looks like it rode along on 4740c7e.
npx playwright test tests/visual/notifications-a11y.spec.ts fails at line 125: getByText('A looping video') never appears, so the rows don't render. Playwright isn't wired into .github/workflows/ci.yml, which is why all four checks are green.
Either drop it from this PR or land it separately once it passes.
| { | ||
| "status": "passed", | ||
| "failedTests": [] | ||
| "status": "failed", |
There was a problem hiding this comment.
Local run artifact, committed in a failed state.
test-results/ is gitignored (.gitignore:56) but this file is tracked, so every local Playwright run rewrites it. This commit records a failure, and the id here is byte-identical to what my local run produced for notifications-a11y.spec.ts above.
git rm --cached test-results/.last-run.json would stop it riding along on unrelated commits.
|
Review in progress on my side, taking a closer look at the NIP-46 half specifically. No need to wait on me before working through realmeylisdev's points; I'll dedupe against their review rather than repeat it. |
There was a problem hiding this comment.
Deduped against realmeylisdev's review. Their five points are untouched and still yours; I confirmed each but have nothing to add to them.
I pushed seven commits, one per finding, all clear-remediation fixes. Everything needing your judgment is listed at the bottom rather than implemented. Revert anything you disagree with.
Pushed
fcc0d631 The auth-challenge popup result was unobservable. window.open returns null whenever the feature string sets noopener, and noreferrer implies noopener. Both were passed, so opened was false on every call. I checked all four variants in Chromium: noopener,noreferrer, noreferrer alone and noopener alone each return null while still opening the tab; only an empty feature string returns a Window.
So if (url && !opened) was if (url), the fallback always rendered, and loginDialog.bunkerAuthPrompt states the browser blocked the popup. All 20 locales assert blocking, so every user hitting a challenge was told their tab was blocked when it had opened. Two tests encoded the unreachable branch, including a mock returning {}, which no browser returns under those arguments.
Now opens with no features and sets popup.opener = null, which I verified succeeds cross-origin. The only cost is the Referer header, and the app sets strict-origin-when-cross-origin at index.html:11, so the signer receives https://divine.video/ with no path or query. I confirmed that on the wire: document.referrer on the opened page is origin-only. The signer already knows that origin. Existing copy becomes true, so no locale changes.
42d6ef6d A dismissed bunker login still committed. LoginArea.tsx:60 renders LoginDialog unconditionally and only toggles isOpen, so closing it neither unmounts the component nor settles the pending handshake. beforeCommit re-checked only keyHandoverRestrictedRef, so an approval arriving after the user gave up ran addLogin() and wrote the cross-subdomain cookie. With no request timeout that window is unbounded. This is the stale-predicate-across-an-await case the guard was added for in #182; dismissal was not among its predicates. Now re-checks that the dialog is open and the attempt has not been superseded.
28758351 The handshake signer and its sockets were never released. BunkerSigner.fromBunker subscribes during construction. loginWithBunker built one, used it for connect() and get_public_key, then dropped it without closing, on success as well as failure. The session signer is rebuilt from persisted data, so that connection had no further purpose, and retrying a bad URI leaked one per attempt. Note close() at nostr-tools/nip46.js:1215 closes only the subscription and leaves the pool connected, so this passes a pool the function owns and destroys it. Distinct from realmeylisdev's point 1, which covers the per-consumer signers.
24413fea Three guards had no test holding them. Removing the bunker: scheme check, the 64-hex remote-signer pubkey check, or the nsec type check on stored login data each left all 7 tests green. For contrast, removing the empty-relay check, the onauth wiring, the protocol allowlist or the #182 commit guard turns tests red, so surrounding coverage is real. These decide who NIP-46 traffic is encrypted to and what bytes become the client key. Three cases added; each fails when its guard is removed.
3ccfff46 A scrollbar drag did not cancel restoration. Hand-over detection listened for wheel, touchstart and keydown. Dragging the scrollbar fires none of them, so on a page whose saved offset is unreachable the loop re-pinned the viewport every frame for the full 3s while the user dragged. Added mousedown.
4fb79c7a An interrupted restore overwrote the saved offset. Cleanup saved window.scrollY unconditionally, including the clamped value a restore was still chasing, so navigating away mid-restore persisted the partial value and each interrupted back-navigation moved the saved position toward the top. This predates the branch. I ran the same probe against merge base c6bcdc40 and it fails identically. Raising and fixing it because it is local to the code this branch rewrites and it defeats the feature the branch adds. Revert this one if you would rather keep the branch to new work.
68e26aa8 The challenge link hid its destination. The URL comes from the remote signer and the anchor text is loginDialog.bunkerAuthLink, so the destination was not visible before clicking. Now shows the host alongside. A hostname is data rather than copy, so no locale changes.
Validation
npm run test, the CI command: 243 files, 1761 tests passing, 0 eslint errors and 17 warnings, matching your warning count. Every fix is mutation checked, meaning removing the fix turns its test red. All four PR checks pass on 68e26aa8.
Scroll restoration verified end to end in Chromium before and after: scrolled to 5678, forward navigation landed at top, back restored to 5678 exactly. The pagination fix verifies clean in both directions: reverting dataLength to videos.length, or counting filtered pages instead of raw, each turns its tests red.
Yours to decide
1. Split this PR? Findings distribute unevenly. 209dc950 drew none. 9ee01cab drew one, plus the pre-existing defect above. Everything else, mine and realmeylisdev's, is against 4740c7e8. The pagination and scroll fixes verify clean and could land now rather than wait on another auth round. Splitting also disposes of realmeylisdev's points 4 and 5, since both rode along on the auth commit. Land the first two and move the NIP-46 work to its own PR, or keep them together?
2. Logout no longer ends the signer session, and that is new on this branch. This is the one unfixed defect rather than a preference, so it is worth separating from the design question attached to it.
NConnectSigner.send opened a REQ per request and returned out of its for await as soon as the response id matched, which tore the subscription down, and it ran on the app's shared pool via pool.group(relays). Nothing persistent existed to outlive a logout. BunkerSigner.fromBunker instead calls setupSubscription during construction, on a private SimplePool, and nothing closes it: close() is never called anywhere in src/, and it is not reachable through the NostrSigner type that NUser stores. So removeLogin drops the login from state while the remote-signer subscription stays live.
The defect is unambiguous. The remedy is not, which is why I did not push one: a signer registry keyed by login id, a provider, or re-plumbing the shared pool would each work, and the choice carries the same root as realmeylisdev's point 1. Which shape do you want?
3. Reuse parseBunkerInput? nostr-tools/nip46 exports it, and it also resolves NIP-05 bunker addresses via queryBunkerProfile. parseBunkerUri adds validation the library lacks and throws typed messages. Reuse would change behavior by accepting user@domain inputs. Worth adopting, or keep the local parser?
4. 4740c7e8 also carries pagination test work. Beyond the two files realmeylisdev flagged, it adds 24 lines to src/pages/ProfilePage.infiniteScroll.test.tsx: component mocks, an assertion that the grid renders 2 deduplicated videos, and a 20s timeout. None of it relates to NIP-46 and the commit message does not mention it, though the PR body says one commit each. Not load bearing: I checked out 209dc950 alone and that test passes in 1.6s. Moving it needs a history rewrite on your branch, so it is yours.
5. es and it render two colons in one paragraph. es/common.json:427 and it/common.json:427 both translate the en dash as :, and the string already ends in : before the link. The other 18 kept a dash. Reporting only; I am not proposing wording in languages I cannot verify. Moot if you would rather reword the prompt instead of taking the Referer trade in fcc0d631.
Requesting changes because item 2 is an unfixed regression introduced on this branch, realmeylisdev's five are still open, and the rest need your decisions. Review the pushed commits, revert anything you disagree with, and merge when ready.
realmeylisdev
left a comment
realmeylisdev
left a comment
There was a problem hiding this comment.
Second pass. My first review was against ecd735d8, so mbradley's seven commits are the new material. I re-measured everything I had open rather than assuming it still stood, and went looking for defects in the pushed fixes.
Still open
My five inline comments are all live against the current head and none have moved. Two are worth restating because I now have numbers instead of an argument:
The per-consumer pool leak. Ten mounted components sharing one bunker login open 20 sockets to the bunker's relays — one per component per relay, each with its own SimplePool and its own live NIP-46 subscription, none closed. The same probe against origin/main opens zero, because NConnectSigner's constructor only assigns fields and it ran on the app's shared NPool. useLoggedInAccounts.ts:35 still builds one purely as a try/catch validity probe and discards the result. This and your item 2 are the same root; I agree the shape is yours to choose.
The unrelated spec. tests/visual/notifications-a11y.spec.ts still fails locally at line 125, and I can now say why rather than just that it does. Its fixtures are built on fields that exist nowhere in src/ — root_addressable_id, source_profile, referenced_video, root_d_tag, referenced_event_title — while RawApiNotification requires an id the fixtures omit and Notification carries no title, thumbnail or actor profile at all. The "grouped row with a stacked actor list" the file documents is not a shape this app renders. That is why I have not touched it: whether that contract is coming or the fixture is simply wrong is yours to say. Playwright is not in ci.yml, so the green checks say nothing either way.
The other three — no timeout on sendRequest, bunkerSignerFromLogin(login.data) passing no onAuthChallenge, and the run artifact — are unchanged from those threads.
Pushed
Three commits, one finding each, all mutation checked: reverting the fix turns its own test red and leaves the rest green.
53bc7388 The restore was animating, which defeated the retry loop and the handover listeners. html { scroll-behavior: smooth } applies app-wide (src/index.css:233), and the positional window.scrollTo(x, y) form scrolls with behavior "auto", which resolves to the root element's computed value. So every attempt in the loop started or retargeted an animated scroll.
Measured in the repo's own Chromium, against a page already tall enough to honour the offset in one write: the loop needed 154 frames over 1271ms to satisfy window.scrollY >= target, versus 1 frame and 0ms with the animation out of the way — so it was chasing its own animation rather than the page's height, and spending nearly half of RESTORE_TIMEOUT_MS doing it. Worse for 3ccfff46: cancelling the loop does not cancel the animation. Stopping on a real mousedown recorded stop() at y=292 while the page carried on to y=6000 anyway, so the listeners you added could not actually hand the page back.
Now restores with the options form and an explicit behavior: 'instant', verified to land synchronously despite the CSS. Forward navigation stays on the positional form so its behaviour is unchanged. Both test files mocked scrollTo positionally; they now read the offset from either shape, and an assertion pins the call form so the animation cannot come back unnoticed.
71b01cf5 mousedown cancellation defeated the interrupted-restore guard. 4fb79c7a decides whether an offset is worth saving by asking isPending(), and 3ccfff46 had already made mousedown one of the events that clears stopped. A click is not a scroll — it hands control back without moving the viewport. So the loop stopped while the page still sat at the clamped offset, cleanup read that as "settled", and the real saved offset was overwritten with the clamped one. That is the defect 4fb79c7a set out to fix, reached through the listeners instead of through an interrupted navigation.
Back into a feed saved at 1800 with the grid still short (clamps to 150), one mousedown, then forward and back again restored 150. Drop the mousedown from that same sequence and it restored 1800, so the check works and this event is the hole. Your two tests pass either way because fireEvent.click dispatches no mousedown, and the scrollbar-drag case assigns a new scrollY afterwards, which is the branch where the viewer really did move.
Now compares the live offset against what the loop last wrote, rather than asking whether the loop stopped. Reaching the target, a wheel, a touch, and a drag that actually moved all still save; a click, a keystroke, and a timeout on a page that never grew all leave the saved offset alone.
One thing to flag: this revises a decision you stated in 4fb79c7a. A restore that times out no longer persists its clamped position, because under this rule the viewer never chose that offset. Revert this commit if you would rather keep the old behaviour — it stands alone.
3f4f5793 Reverted the Playwright run artifact. Content back to origin/main's. git rm --cached is still the durable fix and I left that to you.
New
42d6ef6d closes the window only until the dialog is reopened. beforeCommit gates on isOpenRef.current && isCurrentAttempt(), but nothing bumps bunkerAttemptRef when the dialog closes, and the reset effect at LoginDialog.tsx:110 runs on open and re-arms isOpenRef to true. So: a user starts a handshake, gives up, closes the dialog, then later taps a like button — openLoginDialog() reopens the same mounted instance, the abandoned approval lands against a guard that now passes, and addLogin() plus the cross-subdomain cookie go through for the account they walked away from. The reset also clears isLoginLoading, so the dialog looks idle while that attempt is still live, and with no request timeout the window has no upper bound. LoginDialog.test.tsx's dismissal case only asserts beforeCommit() === false while isOpen is false, so it passes against this.
To be fair to the commit: pre-PR this committed unconditionally, so this is a residual gap in a new mitigation rather than a regression you introduced. bunkerAttemptRef.current++ in the if (!isOpen) branch of the reset effect closes it, since a superseded attempt can never become current again — and it stops a stale onAuthChallenge writing into the fresh dialog's state too. I have not pushed it: it is in the auth path, and it sits next to the signer-lifecycle question you are already deciding.
28758351's socket release has nothing holding it. Removing the pool.destroy() leaves bunkerSigner.test.ts green. Worth an assertion given the leak it exists to prevent is the one still open above.
Checked and dropped
Recording these so they do not come back as findings later. I chased each and they do not hold:
get_relaysis gone from the new signer. True mechanically —NConnectSignerimplements it, the new wrapper does not, anddm.ts:630/:656feature-detect it. Butget_relaysis not in the current NIP-46 method table, so a compliant signer errors and the pre-PR path landed on the same empty list via.catch(() => ({})). For the legacy signers that do answer it, the pre-PR call went tologin.pubkeyrather than the remote-signer pubkey and awaited with no signal behind a 60s timeout, so dropping it is neutral to positive.- No link shown when the challenge tab does open. Not a defect — the tab is the primary channel, the URL is in session history, and the pending request lives in the signer's own UI. The "spins forever" part of it is the missing timeout, which is already open.
- No live region on the fallback link. The dialog has never had one for
generalError,keyError,bunkerErroror theConnecting…relabel. Pre-existing convention, and the link is a real focusable<a>in normal DOM order — not something this PR introduced. onAuthChallengeforwarding untested. Both ends are covered (bunkerSigner.test.tsandLoginDialog.test.tsx); the middle hop is thin but not a gap worth a finding.- The
dataLengthshape inVideoFeed. That is #529, deliberately deferred, and I agree with deferring it.
Validation
npm run test, the CI command: 243 files, 1763 tests passing, tsc -p tsconfig.app.json --noEmit clean, eslint 0 errors and 17 warnings (matching your count), build succeeds. Chromium measurements above were taken with the repo's own pinned browser.
Requesting changes: the auth items from my first pass are still open, and 42d6ef6d's residual gap plus your own items 1 and 2 need your decisions rather than mine. The three pushed commits are independent — revert any of them on its own.
A remote signer may answer a request with `{result: "auth_url", error: <url>}`,
meaning the user has to approve out of band while the client keeps listening on
the same request id. The bunker client treated any non-empty `error` field as
terminal, so it both failed the call and tore down the subscription the real
answer arrives on. Signers that ask for approval — nsecbunkerd admin approval,
nsec.app manual confirmation — could not sign in at all, and the surfaced error
message was the challenge URL itself.
Move bunker login and the ongoing bunker signer onto the nostr-tools NIP-46
client, which keeps waiting after a challenge and exposes it through `onauth`.
The challenge opens in a new tab; because that fires after an await, popup
blockers commonly eat it, so the login dialog offers the link as a fallback.
Only http(s) challenge URLs are opened, since the URL comes from the signer.
The client key is still generated per login and persisted as before, so
existing bunker sessions keep their NIP-46 identity and need no re-approval.
Closes #485
`tsc -p tsconfig.app.json` type-checks test files, and casting an untyped `vi.fn()`'s `mock.calls[0]` to a tuple fails TS2352. Declare the mock's parameters so the call tuple is inferred and the casts are unnecessary.
`window.open` returns null whenever the feature string sets `noopener`, and `noreferrer` implies `noopener`. Both were passed, so `opened` was false on every call even when the tab opened fine. Verified in Chromium: every variant opens the tab, but only an empty feature string returns a Window. The dialog therefore rendered the fallback block unconditionally, telling every user in all 20 locales that their browser had blocked a popup that in fact opened. Open with no features so the return value is meaningful, and sever the opener reference by hand instead. `opener` is settable cross-origin, so a hostile signer's page still cannot reach back into this one. Dropping `noreferrer` means the signer sees this page's URL in the Referer header. That is a party the user is deliberately authenticating against, which is preferable to reporting the wrong outcome. The existing copy becomes true again, so no locale needs re-translating.
LoginArea renders LoginDialog unconditionally and only toggles `isOpen`, so closing the dialog never unmounts it and never settles a pending NIP-46 handshake. `beforeCommit` re-checked only the protected-minor policy, so an approval arriving after the user gave up still ran addLogin() and wrote the cross-subdomain login cookie. With no request timeout on the signer, that window is unbounded rather than bounded by the old 60s. Re-check that the dialog is still open and that this attempt has not been superseded before committing, and scope the challenge-link and spinner updates to the current attempt so a stale attempt cannot write over a newer one's UI. This is the same stale-predicate-across-an-await class the commit guard was added for in #182. Dismissal was the predicate it did not cover.
`BunkerSigner.fromBunker` opens a subscription to every bunker relay during construction. `loginWithBunker` built one, used it for connect() and get_public_key, then dropped it without closing, on the success path as well as the failure path. The session signer is rebuilt from the persisted data, so that connection had no remaining purpose and stayed up for the life of the page. Retrying a bad URI leaked one per attempt. `close()` only drops the subscription and leaves the pool's sockets connected, so pass a pool this function owns and destroy it in a finally. This is the handshake half of the wider signer-lifecycle problem. Who owns the long-lived session signers is a design question left to the author.
Mutation testing found three validations on the bunker path with no test holding them. Removing the `bunker:` scheme check, the 64-hex remote-signer pubkey check, or the `nsec` type check on stored login data each left the suite green. Every one of these decides who NIP-46 traffic is encrypted to, or what bytes are used as the client key, so an unheld guard there is worth closing. All three now fail when the guard is removed.
The challenge URL comes from the remote signer, and the dialog rendered it
behind our own localized label ("Approve in your signer"), so the user had no
way to see the destination before clicking a link the app appeared to vouch for.
Show the host alongside the link. A hostname is data rather than copy, so this
adds no new string and no locale needs updating.
3f4f579 to
e07260d
Compare
rabble
changed the title
|
@mbradley @realmeylisdev — split done, per your item 1. Force-pushed, so a heads-up on what moved and where. This PR is now auth-only. Seven commits, all NIP-46. Your five auth commits are here with original authorship: #536 has the grid work — the pagination and scroll fixes, plus your four scroll commits, also with authorship intact: The pre-split head is at Two of realmeylisdev's findings are resolved by the rewrite rather than by a fix:
Everything else you raised is still open and still yours — I haven't touched the signer-lifecycle question, the missing request timeout, the dropped mid-session challenges, the Also: realmeylisdev, your |
BunkerSigner.sendRequest registers a listener and settles only when a matching response arrives. A signer that is unreachable behind a relay that still accepts the publish therefore leaves the promise pending forever: loginWithBunker never returns, LoginDialog's finally never runs, isLoginLoading stays true, and the dialog sits on "Connecting…" with no error. NConnectSigner, which this replaced, bounded every request at 60s. Restore that bound in the wrapper, since BunkerSignerParams has no timeout option to pass through. An auth_url challenge restarts the clock rather than counting against it: the signer answering proves it is reachable, and the rest of the wait is a human approving out of band. The restarted clock still expires, so an abandoned approval cannot hang either. Covers connect, get_public_key, sign_event and both nip04/nip44 pairs, so a mid-session request cannot hang either.
createUserFromLogin runs in a useMemo in useCurrentUser, which is mounted across the app, and again in useLoggedInAccounts purely as a validity probe whose result is discarded. Each call reached BunkerSigner.fromBunker, which constructs its own SimplePool and calls setupSubscription eagerly, so every consumer of a login opened a socket to every one of its relays and nothing ever called close(). Ten mounted components sharing one login held twenty sockets. NUser.fromBunkerLogin, which this replaced, ran on the app's shared NPool and opened a REQ per request, so idle logins cost nothing there. Keep one signer per login id in a registry so consumers share it, and open the connection on the first request rather than at construction so a login that never signs anything costs nothing. The client key is still decoded up front: callers treat a throw as 'skip this login', and deferring that check would let an unusable login read as valid and fail every later request instead. useLoggedInAccounts now answers its probe with canCreateUserFromLogin, which validates without registering anything. useCurrentUser reconciles the registry against the surviving login ids, which covers logout, account switching and a login dropped as invalid without each call site having to remember.
The guard added earlier gated on `isOpenRef.current && isCurrentAttempt()`, but nothing bumped the attempt counter on close and the reset effect re-arms `isOpenRef` to true whenever the dialog is reopened. So the window it closed reopened along with the dialog: a user starts a handshake, gives up, closes, then anything calling openLoginDialog() later reopens the same mounted instance and the abandoned approval commits against a guard that passes again, taking addLogin() and the cross-subdomain cookie with it. Nothing bounds how long the signer takes, so that attempt can still be in flight. Bump the counter on close instead. A superseded attempt can then never become current again whatever happens to `isOpen`, and a stale challenge cannot write into the fresh dialog's state either. The existing dismissal test only asserted the guard while the dialog was closed, which passed against this. It now continues through a reopen. Reported by realmeylisdev.
Removing `pool.destroy()` left the suite green, so the socket release the commit exists to perform had nothing holding it. The pool was also unobservable because the test used a real `SimplePool`. Mock it and assert both that the owned pool reaches `BunkerSigner.fromBunker` and that it is destroyed, on the success and failure paths. Dropping either the destroy call or the pool argument now turns these red. Reported by realmeylisdev.
`releaseBunkerSigner` closes the signer on logout, but `BunkerSigner.close()` only drops the NIP-46 subscription. When no pool is passed, nostr-tools builds one internally and keeps it private, so nothing could ever release it: the sockets to every bunker relay stayed connected for the life of the tab, which is the leak the registry was added to close. Create the pool here instead, so closing the signer can destroy it. A pool the caller supplied is left alone, since the caller may still be using it and is responsible for it. `loginWithBunker` already works that way and is unchanged. Reverting either half turns the new tests red.
mbradley
left a comment
mbradley
left a comment
There was a problem hiding this comment.
Re-reviewed at bbc5873d against my previous pass. The split landed cleanly: range-diff shows all five of my auth commits carried through the force-push byte-identical, with nothing dropped.
89b50fe0 and 90df9e76 resolve the two items my last review was blocking on, so I am clearing that block. One fix pushed, one question left.
Pushed
0566cf43 Releasing a signer left its sockets connected. releaseBunkerSigner closes the signer on logout, but BunkerSigner.close() only drops the NIP-46 subscription (nostr-tools/lib/esm/nip46.js:1215). With no pool passed, nostr-tools builds one internally at nip46.js:1116 and keeps it private, so nothing could ever release it: a socket per bunker relay stayed connected for the life of the tab, which is the leak the registry exists to close.
The pool is now created in createBunkerSigner, so close() can destroy it. A pool the caller supplied is left alone, since the caller may still be using it. loginWithBunker already owned its pool and is unchanged. Reverting either half turns the new tests red.
I could not observe this at runtime: nip46.js bundles its own SimplePool at line 974, so mocking the nostr-tools/pool module does not intercept the internal one. The finding rests on the source instead, which is unambiguous on all three points: internal construction, private visibility, and a close() that never touches it.
Open question
Is 60s enough after an auth challenge? 90df9e76 restarts the deadline on auth_url, which is the right shape. But onauth fires once per request: nip46.js:1188 deletes waitingForAuth[id] before calling it, so a request gets exactly one 60s extension.
That covers a signer the user approves promptly. It does not obviously cover nsecbunkerd admin approval, which this PR's original description names as a target case and which waits on a second human. Past 60s, connect() rejects and the dialog shows errorBunkerConnectFailed while the approval tab is still open, which is close to the failure #485 set out to fix.
Options, none free: a longer bound once a challenge has been seen; restart on any signer traffic rather than only auth_url; or accept 60s and treat slow approval as a retry. Which do you want? Happy to implement whichever you pick.
Checked and dropped
Recording these so they do not resurface:
- Registry sharing untested. False. Defeating the shared-signer branch turns 4 tests red, including "hands every consumer of one login the same signer". My first run said otherwise and was wrong: a parallel process had mutated the same file, so my baseline was contaminated.
- The validity probe changed semantics. It did not.
canCreateUserFromLoginspecial-cases bunker to avoid opening a connection and falls through to the original try/catch for every other type. - Release wiring incomplete. It is not.
useCurrentUserreconciles against survivingloginIds, which covers logout, account switching, and a login dropped as invalid. - The deadline breaks DM decryption. It does not. Each
nip44call through the signer was previously unbounded, so bounding it is an improvement rather than a regression.
Validation
npm run test, the CI command: 242 files, 1799 tests passing, eslint 0 errors and 17 warnings, build succeeds. All checks green on 0566cf43.
Approving the state I pushed. realmeylisdev's review is still open on its own items, and the question above is yours rather than mine. Review the pushed commit, revert it if you disagree, and merge when you are ready.
|
@realmeylisdev I saw that you requested a review from Daniel and me on this PR but since you're not the author of the PR, I wasn't sure if that was intentional or just an accident. But just in case it was intended, is there anything specific you'd like us to take a second look at? |
|
Sorry, that was a mistake on my part — no action needed. |
realmeylisdev
left a comment
realmeylisdev
left a comment
There was a problem hiding this comment.
Third pass, at 0566cf43. My last review was against 3f4f5793, so the split and the five commits after it are the new material. I re-ran every finding I had open rather than reading the commit messages, and mutation checked each fix: reverting it turns its own test red and leaves the rest green.
Clearing my block. Four of my five are closed, one is still open and I am leaving it to you rather than holding the PR for it.
Closed
The per-consumer pool leak. 89b50fe0 keys one signer per login id and defers construction to the first request, so an idle login costs nothing and ten mounted components share one connection. useCurrentUser reconciles against the surviving login ids, which covers logout, account switching, and a login dropped as invalid. 0566cf43 closes the other half: BunkerSigner keeps its pool private and close() only drops the subscription (nip46.js:1215), so a pool it creates internally could never be released. Defeating the sharing branch turns 4 tests red, dropping the release effect turns 2 red, dropping ownedPool.destroy() turns 1 red.
The missing request timeout. 90df9e76 bounds every method at 60s and restarts every in-flight clock on auth_url, which is the shape I asked for — the dead-signer case fails instead of hanging, and the waiting-on-approval case is not punished for the wait. Defaulting the bound to 0 turns 5 red; dropping the restart turns "restarts the clock when the signer asks for approval" red.
42d6ef6d's residual gap. 67b35168 bumps bunkerAttemptRef in the if (!isOpen) branch, so a superseded attempt can never become current again however isOpen moves afterwards. The test now reopens the dialog after dismissing it and asserts the guard stays closed; remove the bump and it goes red, where the old assertion passed against the gap.
The handshake pool release had nothing holding it. bbc5873d covers both exits. Removing pool.destroy() from loginWithBunker turns the success and the failure case red together.
The unrelated spec and the run artifact. Both gone with the split — neither file appears in main...HEAD. git rm --cached test-results/.last-run.json plus a gitignore entry is still the durable fix for that file, and still not this PR's job.
Still open — yours
The session path never presents a challenge. bunkerSignerRegistry.ts:37 calls bunkerSignerFromLogin(data) with no handler, and LoginDialog.tsx:261 is the only onAuthChallenge in src/. So mid-session, onauth restarts the deadlines and returns: the optional call never evaluates its argument, so presentAuthChallenge does not run, there is no tab attempt and no link, and the request expires at the bound with nothing shown.
The mechanism half is genuinely fixed — the subscription survives the challenge and the request keeps waiting, which is what NConnectSigner got wrong — so this is better than main, where the challenge was fatal and poisoned every later request. What is missing is that the user is never told to approve.
I have not pushed a fix because the remedy has to reach the UI from a module-level singleton with no React context: a callback registered by a provider, a toast imported into a lib module, or moving the registry behind context. That is an architectural choice rather than a clear remediation. Reusing loginDialog.bunkerAuthPrompt and bunkerAuthLink in a toast would cover it without new copy in 20 locales, if you want the smallest version. Happy to implement whichever shape you pick, or to open a follow-up issue and let this land as it is.
Approving with it open because it regresses nothing, and blocking on it keeps the login path broken too.
On the 60s question
@mbradley's reading of nip46.js holds: waitingForAuth[id] is deleted before params.onauth runs, so a request gets exactly one extension however many challenges the signer sends.
One thing that follows from the same code and is worth weighing before choosing an option: a second auth_url on the same request id falls past the waitingForAuth branch into listeners[id], where if (error) handler.reject(error) rejects the request with the URL as its message. That is the @nostrify failure mode this PR exists to remove, reachable again on a repeated challenge. I have not seen a signer do it, so I am reporting the mechanism rather than a defect — but it does mean "restart on any signer traffic" has little to work with on this path, since a request awaiting approval is otherwise silent. A longer bound once a challenge has been seen is the option that does not depend on signer behaviour.
Either way this is tuning on a path that had no bound at all before, so it does not need to hold the PR.
Validation
npm run test, the CI command: 242 files, 1799 tests passing, tsc -p tsconfig.app.json --noEmit clean, build succeeds. All checks green on 0566cf43. The mutation checks above ran against the seven auth suites (96 tests), each applied and reverted on an otherwise clean tree.
Approving. The open item and the timeout question are both yours — merge when you are ready.
4 participants
Split: the pagination and scroll-restoration fixes moved to #536, which also carries the four scroll commits @mbradley and @realmeylisdev pushed here. This PR is now NIP-46 only.
Closes #485.
Problem
Per NIP-46 a signer may answer
{result: "auth_url", error: <url>}, meaning "the user must approve out of band, keep listening on this request id".@nostrify'sNConnectSignerhas a two-part defect:cmd()throws on any non-emptyerror, so the challenge surfaces as an error whose message is literally a URLsend()resolves on the first response matching the request id andreturns out of thefor await, tearing down the subscription the real answer arrives onBoth the login handshake (
NLogin.fromBunker) and the ongoing session signer (NUser.fromBunkerLogin) went through it, so signers that ask for approval — nsecbunkerd admin approval, nsec.app manual confirmation — could not sign in, and a mid-session challenge would break every later request.Fix
Move both paths onto the
nostr-toolsNIP-46 client already in the dependency tree, which keeps waiting after a challenge and exposes it viaonauth. The challenge opens in a new tab, with the link shown as a fallback. Onlyhttp(s)challenge URLs are opened, since the URL comes from the remote signer.The client key is still generated per login and persisted as
clientNsecin the same shape, so existing bunker sessions keep their NIP-46 identity and need no re-approval.Incidental fix worth noting (@realmeylisdev spotted it):
bunkerSignerFromLogintargetsdata.bunkerPubkeywhereNUser.fromBunkerLoginwas p-tagginglogin.pubkey. NIP-46 requires those to be distinct, so this corrects a real bug for signers whose remote-signer key isn't the user key.Reviewer commits, preserved
ab8cda08ca4ec381eb1494f5bfd496dce07260d6— all five of @mbradley's auth fixes, original authorship intact. The pre-split head is preserved atbackup/pre-split-531.Still open — see the review threads
BunkerSigner.fromBunkersubscribes during construction on a privateSimplePool; nothing callsclose(), and it isn't reachable through theNostrSignertypeNUserstores. New on this branch.SimplePooland open sockets peruseCurrentUser()consumer. @realmeylisdev measured 10 mounted components on one login opening 20 sockets, versus zero onmain. Same root as the above.NConnectSignerhad 60s; the dialog can now sit on "Connecting…" indefinitely.bunkerSignerFromLoginreceives noonAuthChallenge, so a challenge outside login produces no tab and no link — the exact failure this PR exists to fix, relocated.ca4ec381has a residual gap. Nothing bumpsbunkerAttemptRefwhen the dialog closes, and the reset effect re-armsisOpenRefon open, so an abandoned approval can still commit once the dialog is reopened.eb1494f5'spool.destroy()has no test holding it.The first four share one design decision — how the bunker signer's lifetime is owned. That's unresolved and is what this PR is waiting on.
Resolved by the split
Both of @realmeylisdev's commit-hygiene findings are gone with the rewrite:
tests/visual/notifications-a11y.spec.ts(unrelated, failing, swept in by a carelessgit add -A) is dropped entirely, and thetest-results/.last-run.jsonchurn never appears — so3f4f5793, which reverted it, is no longer needed. TheProfilePage.infiniteScroll.test.tsxwork that rode on the auth commit moved to #536 as its own commit.git rm --cached test-results/.last-run.jsonplus a gitignore entry is still the durable fix for that file and is not done here.Validation
tsc -p tsconfig.app.json --noEmitclean, eslint 0 errors / 17 warnings, and the four auth suites pass 50/50. Full-suite numbers from CI rather than local — this machine is currently flaky under load, withorigin/mainalone failing 8 tests across 5 files.