Skip to content

fix(auth): contain untrusted transport failures - #413

Merged
ty-everett merged 9 commits into
mainfrom
codex/auth-transport-payload-hardening
Jul 31, 2026
Merged

fix(auth): contain untrusted transport failures#413
ty-everett merged 9 commits into
mainfrom
codex/auth-transport-payload-hardening

Conversation

@ty-everett

@ty-everett ty-everett commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • contain synchronous throws and rejected promises at every audited first-party BRC-103 transport and listener boundary
  • isolate invalid or overloaded Socket.IO peers without terminating the host process or affecting healthy peers
  • make AuthSocket event decoding total for arbitrary bytes and JSON values, and avoid including remote payloads in error reports
  • add bounded per-socket authentication-message concurrency with a conservative default and documented override
  • publish patch versions and migration notes for @bsv/authsocket, @bsv/authsocket-client, @bsv/sdk, and @bsv/auth-express-middleware
  • replace global CI fan-out with dependency-, trust-boundary-, and build-context-aware validation

Audit and hardening

The audit covered the AuthSocket server and client transports, AuthSocket application callback dispatch, SDK Peer listener dispatch, SimplifiedFetchTransport, and ExpressTransport. 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 _unknown event instead of throwing.

Dependency-aware CI and fail-fast behavior

  • a zero-install job resolves direct changes, reverse dependents, forward build prerequisites, changed lockfile importers, documentation/conformance inputs, mutation target inputs, and infrastructure build contexts before dependency installation
  • directly changed packages generate coverage; reverse dependents run non-instrumented regressions plus type and packed-browser compatibility; builds include the exact prerequisite closure
  • mutation selection follows each registered implementation, property, regression, configuration, policy, and changed lockfile importer rather than treating SDK, CI, or governance edits as global invalidations
  • infrastructure and runtime image matrices follow changed Docker build contexts; CI-workflow-only and application-package changes schedule no unrelated images
  • empty infrastructure/runtime matrices allocate no build runner, and the standalone conformance workflow is limited to its exact vectors/specifications/generator inputs because SDK-dependent conformance already follows the package graph
  • repository health must pass before the expensive prepare job, all expensive matrices cancel unfinished siblings on the first failure, and every CI job has a reviewed timeout instead of GitHub’s six-hour default
  • both pull-request and main-branch push runs use the actual base/head graph, with full-workspace fallback only when the base revision is unavailable

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

  • full affected-package coverage suites: authsocket 41 tests (100% statements and branches); authsocket-client 39 tests (100% statements, 97.5% branches); auth-express-middleware 104 tests (92.99% statements); SDK 5,817 tests (94.76% statements, 87.01% branches)
  • focused final rerun: 168 tests across SDK Peer/fetch, AuthSocket server/client, real Socket.IO isolation, and Express hardening
  • fast-check properties for arbitrary remote values, arbitrary byte parsing, non-envelope rejection, and canonical envelope round trips on both AuthSocket packages
  • real Socket.IO regression proving a malformed first client is isolated while the HTTP/socket server remains listening and accepts a second client
  • governed mutation targets added for both AuthSocket transports: server 89.06%, client 87.69%; ratchets set to 89% and 87%
  • 125 zero-install repository/governance tests, selector contract tests, YAML parsing, dependency-scoped build/typecheck, all documentation examples, lint, and formatting pass locally
  • exact-head CI run 30661033466 completed with 25 successful jobs, 7 intentional skips, and no failures; all 43 PR checks are non-failing, Codecov reports 97.44% patch coverage, CodeQL/Sonar/Socket are green, and the runtime contract selected zero images

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

  • Release notes and necessity: Patch release notes document required security containment and migration behavior for all four affected public packages.
  • Runtime, build, and peer compatibility: No third-party ranges change; exact workspace references advance only for coordinated first-party patch versions and pass the Node 24.14 / pnpm 10.33.2 matrix.
  • Deduplicated lockfile: The frozen pnpm lockfile changes only for test-only fast-check and socket.io-client additions plus the four coordinated first-party package snapshots; pnpm install --frozen-lockfile --ignore-scripts passes.
  • Audit and CodeQL: The only external additions are test-only fast-check for both AuthSocket packages and socket.io-client for 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.
  • Package and consumer tests: Full changed-package coverage, affected-dependent regressions, pack checks, browser consumers, 21 exact documentation tarballs, integration, property, mutation, dependency-scoped build, and reverse-closure typecheck pass.
  • Bundle and performance impact: No dependency or browser adapter is added; browser package checks pass. Per-socket pending authentication work is now bounded at 32 by default.
  • Affected public package versions: @bsv/authsocket 2.1.5, @bsv/authsocket-client 2.1.4, @bsv/sdk 2.2.16, and @bsv/auth-express-middleware 2.1.6.

Related: #400

@codecov

codecov Bot commented Jul 31, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.43590% with 5 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
packages/sdk/src/auth/Peer.ts 71.42% 2 Missing and 2 partials ⚠️
...essaging/authsocket-client/src/AuthSocketClient.ts 97.61% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@sirdeggen sirdeggen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 _unknown instead of throwing.
  • Sync + async containment at transport processMessage, app callbacks, connection callbacks, and onError itself (nested failures cannot escape).
  • Peer isolation: failed latch + disconnect(true) only on the offending socket; subsequent frames ignored; capacity released in finally.
  • Concurrency bound with fail-fast validation (positive safe integer, default 32).
  • Safe diagnostics: onError context 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)

  1. Repository health contract is failing on pr-evidence:

    Dependency changes require the ## Dependency evidence section
    

    Current PR body does contain a complete ## Dependency evidence section with all required fields. This may be a stale run / PR_BODY injection glitch — re-run the health job and confirm it passes before merge. If it fails again, verify the workflow is receiving the full body.

  2. codecov/patch failing (~87%, 25 uncovered lines, mostly AuthSocketServer.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:

  1. Application callback throw/reject → disconnect
    App bugs become connection terminations. Good for containment; operators relying on "log and continue" must adopt onError + more defensive handlers. Migration notes cover this — good.

  2. Peer listener dispatch is now sequential await
    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.

  3. Connection callbacks are now sequenced with await (AuthSocketServer)
    Previously forEach(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/onError rejection contained

Recommendation

Ship after:

  1. Green Repository health contract (re-run / confirm PR_BODY), and
  2. 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.

@sonarqubecloud

Copy link
Copy Markdown

@ty-everett
ty-everett merged commit f4f735d into main Jul 31, 2026
43 checks passed
@ty-everett
ty-everett deleted the codex/auth-transport-payload-hardening branch July 31, 2026 20:15
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.

2 participants