fix(auth): reply on the session the request arrived on - #368
Conversation
The Express auth middleware signed general-message responses with `peer.toPeer(payload, identityKey)`. `toPeer` resolves the session via `SessionManager.getSession(identityKey)`, which returns the peer's most recently updated session — not necessarily the session the request arrived on. A peer holding concurrent sessions therefore receives responses stamped with another session's `yourNonce`, which BRC-103 defines as an echo of the peer's nonce from the previous message (specs/auth/brc103-mutual-auth.yaml:78,174). The requesting session cannot resolve that nonce, so the response is unusable. Add `Peer.toSession(message, sessionNonce)` for responders: it resolves one exact session and throws when that session is unknown or unauthenticated. It never initiates a handshake — BRC-103 defines only `initialRequest` from the initiating party and `initialResponse` from the receiving party (specs/auth/brc103-mutual-auth.yaml:30,144), so a responder has no way to open a session and must fail instead. The middleware now calls `toSession` with the session nonce the client echoed in `x-bsv-auth-your-nonce`.
`SimplifiedFetchTransport.onData` discarded every failure raised while the peer processed an incoming message. The HTTP exchange has already completed by that point, so nothing else can settle the request: an `AuthFetch.fetch` promise resolves only from the general-message listener, and that listener never runs when processing fails. The caller waits forever, holding whatever it was holding — for a wallet storage client, its reader lock. A response signed against a session the client does not hold reaches exactly this path via `Session not found for nonce`. Give the transport an `onDataError` listener and route those failures to it. AuthFetch registers one, matches the failure to a pending request by the 32-byte request nonce the message opens with, retires its listener and rejects it. Failures that cannot be attributed to a request are left alone, and with no listener registered the previous swallowing behavior is unchanged. Failed responses are rejected rather than retried: the request has already been processed by the server, so replaying it is not safe.
shruggr
changed the title
Both were beyond the two defects being fixed. `Peer.toSession` re-implemented what `toPeer` already does: `getSession` resolves a session nonce directly, so passing the request's nonce is sufficient. The middleware now calls `toPeer(responsePayload, sessionNonce)` and no SDK change is required, which also drops the `@bsv/sdk` peer-range bump. The `stopListening` entry on AuthFetch's pending-request map addressed a listener leak, not the hang.
Sonar typescript:S6551 — String(error) renders a thrown object as '[object Object]'. Keep a thrown string as the message and attach any other thrown value as the error's cause, matching createNetworkError in the same file.
Out of scope for this PR. The swallowed client-side failure is a separate defect — a request that can never settle — and it argues on different grounds than a response bound to the wrong session. It should not gate this fix. Also restores the session nonce to a named constant in the middleware test rather than repeating the literal.
shruggr
changed the title
…main + PR bsv-blockchain#368) - Rename scope for interim publish so @1sat/wallet-server can take the response-session binding fix before upstream merges - Not for upstream: main keeps the @bsv scope
Point @bsv/auth-express-middleware at @bopen-io/auth-express-middleware 2.1.3 via an npm alias, so the wallet server replies on the session a request arrived on rather than the peer's most recently updated one. Interim until bsv-blockchain/ts-stack#368 merges and upstream releases; reverting is a one-line change back to the @bsv range. Also notes the AsyncSessionManager rewrite as a TODO on the Redis session store.
|
ty-everett
left a comment
ty-everett
left a comment
There was a problem hiding this comment.
Approved at exact head d6d8d927e6bd1b850ecfbf9826d704c3e760b492 after an adversarial, deployment-focused review.
The change is warranted. On exact current main, a deterministic real-Express integration reproduction with two independent AuthFetch clients sharing one identity establishes two sessions, holds session A's route response, updates session B, then proves A hangs because identity-key lookup selects B. On this head, A receives the correct authenticated 200 response. The request's canonical, already-verified x-bsv-auth-your-nonce identifies the exact server session that carried the request, so passing it to the existing Peer.toPeer lookup removes the race.
The runtime diff is deliberately minimal: no public API, wire format, header contract, signature scheme, CORS/CSP policy, package version, or dependency changes. Single-session behavior is unchanged. The request session is authenticated before Express dispatch and refreshed by message processing; an unavailable session still follows the existing response-signing failure path. Deployed consumers (Wallet Storage, Overlay Express auth surfaces, Message Box, UHRP, and payment middleware) do not change until a future package release and rollout.
Evidence: exact-main regression fails by deterministic timeout; exact-head regression passes. Auth middleware coverage passed 102 tests; Overlay Express passed 285; payment middleware passed 39; Wallet Storage remoting passed 9. Package typecheck, lint, format, pack/publint/strict consumers, root audit, and repository health all passed. Local LCOV executes both changed production lines 29 times. Hosted CI run 30297586037 completed successfully across all executed build/test/coverage/package/mutation/dependency gates; Conformance run 30297586062 passed. Sonar's exact-PR gate is OK with A ratings, 0 open/confirmed issues, 0 hotspots, and 0 duplication. Because GitHub skipped the fork PR's advanced-CodeQL matrix, the identical SHA was analyzed through trusted run 30297866753: Actions and JavaScript/TypeScript both passed with no open CodeQL findings. Socket and Dependency Review passed. There are no review threads.
Squash merge is appropriate so only the reviewed four-file, 81-addition/6-deletion final diff lands, not the branch's exploratory/revert history.
2 participants
The server responds with the wrong session nonce when a client holds more than one session. The client has no record of that session and cannot match the response to its request, so the
AuthFetchpromise neither resolves nor rejects, and no timeout releases it. Any caller holding a lock across that call holds it for the life of the process — in wallet-toolbox this deadlocks storage, blocking every subsequent wallet data operation until the client is restarted.Spec
yourNonceis "Echo of the peer's nonce from the previous message" (specs/auth/brc103-mutual-auth.yaml:174). A Phase 2 response carriesx-bsv-auth-your-nonce: <base64ClientNonce>(:78).Where it breaks
ExpressTransportsigns responses withpeer.toPeer(responsePayload, senderPublicKey)— an identity key.Peer.toPeerresolves that throughSessionManager.getSession(identityKey), which returns the peer's session with the newestlastUpdate, not the session the request arrived on:The response is otherwise valid and correctly signed — it answers on the wrong session, so only the client can detect it.
Fix
Pass the session nonce the client echoed in
x-bsv-auth-your-nonce, already validated as canonical base64 byvalidateGeneralAuthRequest.getSessionresolves a session nonce directly, so no SDK change is required.Existing assertions in
ExpressTransportHardening.test.tsfollow the call to that nonce. Middleware suite 100 pass / 6 suites, includingintegration.test.tsagainst a real Express server andPeer.tsc --noEmit,lintandformat:checkclean. No wire or API change.