fix(auth): contain untrusted transport failures - #413
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
sirdeggen
left a comment
There was a problem hiding this comment.
Review: fix(auth): contain untrusted transport failures
Overall this is a solid, well-scoped hardening of BRC-103 transport boundaries. The containment model is consistent across AuthSocket server/client, SDK Peer / SimplifiedFetchTransport, and Express transport; tests (unit + fast-check properties + real Socket.IO isolation + mutation targets) match the threat model; migration notes and version bumps are coordinated.
Verdict
Approve once CI is green — no critical logic defects found. A few behavior changes and CI gates need attention before merge.
What works well
- Total event decode: non-envelope JSON and arbitrary bytes map to
_unknowninstead of throwing. - Sync + async containment at transport
processMessage, app callbacks, connection callbacks, andonErroritself (nested failures cannot escape). - Peer isolation:
failedlatch +disconnect(true)only on the offending socket; subsequent frames ignored; capacity released infinally. - Concurrency bound with fail-fast validation (
positive safe integer, default 32). - Safe diagnostics:
onErrorcontext is phase / socketId / eventName only — no remote payload or wallet material. - Side fix: AuthSocket server options strip wallet/session/hooks before constructing Socket.IO
Server(was previously leaking BRC-103 options into IO config). - Express:
Promise.resolve().then(async () => await messageCallback(...))correctly catches synchronous throws that bare.catch()missed. - Peer: null/array/non-object messages rejected before field access; listeners awaited so rejections reach the owning transport.
- Evidence quality: property suites registered in governance, mutation targets ratcheted (89% / 87%), real Socket.IO process-survival test.
Merge blockers (CI)
-
Repository health contractis failing onpr-evidence:Dependency changes require the ## Dependency evidence sectionCurrent PR body does contain a complete
## Dependency evidencesection with all required fields. This may be a stale run /PR_BODYinjection glitch — re-run the health job and confirm it passes before merge. If it fails again, verify the workflow is receiving the full body. -
codecov/patchfailing (~87%, 25 uncovered lines, mostlyAuthSocketServer.ts/ client /Peer.ts). Not a logic blocker if package coverage gates already pass, but it is red on the PR.
Behavioral changes worth explicit sign-off
These look intentional and documented, but they change production semantics:
-
Application callback throw/reject → disconnect
App bugs become connection terminations. Good for containment; operators relying on "log and continue" must adoptonError+ more defensive handlers. Migration notes cover this — good. -
Peerlistener dispatch is now sequentialawait
First rejecting general/certificate listener short-circuits remaining listeners and fails the transport path. Previously fire-and-forget. Multi-listener consumers should be rare but this is a real semantic change. -
Connection callbacks are now sequenced with
await(AuthSocketServer)
PreviouslyforEach(cb => cb(authSocket))(no await). A slow/hung async connection handler now blocks later handlers for that socket with no timeout. Consider documenting, or isolating each callback with its own try/catch so one hang/failure does not stall the whole list (failures already disconnect; hangs do not).
Non-blocking notes
| Area | Note |
|---|---|
| Unknown-event data | Server uses data: null, client data: undefined — pre-existing asymmetry, preserved; fine if intentional. |
| Payload size | Concurrency is bounded; single huge authMessage still pays UTF-8 + JSON.parse cost. Size limits would be a nice follow-up for untrusted ingress. |
SimplifiedFetchTransport |
Sync throws are contained, but still silent (no onError). Asymmetry vs AuthSocket is OK for this patch if deliberate. |
Empty eventName: "" |
Accepted as a named event (typeof === 'string'). Harmless. |
pack:check export list |
Still lists only primary classes; new public types/decodeAuthSocketEventPayload ride on export * — OK, optional to extend the pack smoke list. |
| SDK CHANGELOG | Security note correctly under [Unreleased]. |
Test / security checklist (reviewed)
- Sync throw + rejected promise contained at socket transport
- Concurrent double-fail reports/disconnects once
- Concurrency limit disconnects offender only
- Capacity released after success
- Invalid limit rejected at construct time
- Property: arbitrary bytes never throw decode; non-envelopes →
_unknown - Real Socket.IO: bad first client isolated, second client served, HTTP still listening
- Express sync callback failure → controlled 500
- Peer rejects null/[]/string messages
- Observer/
onErrorrejection contained
Recommendation
Ship after:
- Green Repository health contract (re-run / confirm
PR_BODY), and - Team acknowledgment of the three intentional behavior changes above (especially disconnect-on-app-error and sequential Peer listeners).
No code changes strictly required from this review unless you want a timeout/isolation hardening for connection callbacks or patch-coverage fill-ins for the Codecov red X.
|



Summary
@bsv/authsocket,@bsv/authsocket-client,@bsv/sdk, and@bsv/auth-express-middlewareAudit and hardening
The audit covered the AuthSocket server and client transports, AuthSocket application callback dispatch, SDK
Peerlistener dispatch,SimplifiedFetchTransport, andExpressTransport. Each callback boundary now handles both synchronous and asynchronous failures. Socket transports disconnect only the offending peer after a terminal authentication/application failure, ignore subsequent frames, contain failures from observer hooks, and reject concurrent-message overloads. SDK listener promises are awaited so transport policy can contain their failures deterministically.The new error hooks expose only phase, socket identifier, and event name context; they do not include untrusted payload bytes. Event envelopes accept only a non-array object with a string
eventName; all other byte sequences and JSON values map to the existing_unknownevent instead of throwing.Dependency-aware CI and fail-fast behavior
For this PR the committed selector resolves 4 directly changed packages, 33 dependency-affected projects, 34 build prerequisites, 28 dependent regression packages, exactly 2 AuthSocket mutation targets, documentation validation, no conformance vectors, and zero infrastructure/runtime images.
QA
Compatibility
The new limits and error hooks are optional. The default per-socket pending authentication-message limit is 32. Existing synchronous callbacks remain supported; application callbacks may now also return promises. A terminal authentication or application failure intentionally closes only the affected Socket.IO connection.
Dependency evidence
fast-checkandsocket.io-clientadditions plus the four coordinated first-party package snapshots;pnpm install --frozen-lockfile --ignore-scriptspasses.fast-checkfor both AuthSocket packages andsocket.io-clientfor the real server integration test; no runtime dependency is added. Dependency review and Socket security pass, and exact-head CodeQL/Sonar are required before merge.@bsv/authsocket2.1.5,@bsv/authsocket-client2.1.4,@bsv/sdk2.2.16, and@bsv/auth-express-middleware2.1.6.Related: #400