Skip to content

fix(gateway): preserve AsyncEventQueue error state & use crypto random for client IDs#152

Open
kevinchennewbee wants to merge 1 commit into
OpenBMB:mainfrom
kevinchennewbee:fix/gateway-event-queue-and-crypto-random
Open

fix(gateway): preserve AsyncEventQueue error state & use crypto random for client IDs#152
kevinchennewbee wants to merge 1 commit into
OpenBMB:mainfrom
kevinchennewbee:fix/gateway-event-queue-and-crypto-random

Conversation

@kevinchennewbee
Copy link
Copy Markdown

Summary

This PR fixes two bugs in src/web/client/GatewayBrowserClient.ts:

1. AsyncEventQueue silently swallows errors after first read (Closes #128)

Root cause: In next(), the error field was cleared (this.error = undefined) immediately after the first consumer read it. Any subsequent retry or reconnection attempt would receive { done: true } instead of the original error, making the failure invisible.

Fix: The error is no longer cleared on read — it is preserved so that all consumers (including retries) see the rejection. The queue's close() and throw() methods still function as before.

2. Non-cryptographic Math.random() used for security-sensitive client IDs (Closes #136)

Root cause: The fallback ID generation used Math.random(), which is predictable and unsuitable for tokens that may be used in authentication or session identification contexts.

Fix: Replaced with crypto.getRandomValues() (available in all modern browsers and Node.js 19+). A 16-byte hex string is generated from cryptographically secure random bytes.

Changes

  • GatewayBrowserClient.tsAsyncEventQueue.next(): removed this.error = undefined
  • GatewayBrowserClient.tsgenerateClientId(): replaced Math.random() with crypto.getRandomValues()

Test plan

  • Verify AsyncEventQueue error is visible to multiple consumers (read error twice, both should reject)
  • Verify generateClientId() produces hex-suffixed IDs in browser and Node.js environments
  • Confirm no TypeScript errors: npx tsc --noEmit -p tsconfig.json

🤖 Generated with [Qoder][https://qoder.com]

…m for IDs

Two fixes in GatewayBrowserClient.ts:

1. **AsyncEventQueue error state preserved (issue OpenBMB#128)**
   The `next()` method was clearing `this.error` after the first read,
   causing subsequent consumers to get `{done: true}` instead of the
   error. This broke WebSocket reconnection retry logic — after the
   first consumer saw the error, all retries silently got "done" and
   stopped trying.

   Fix: keep `this.error` persistent. Every call to `next()` after a
   failure now correctly rejects with the same error.

2. **Non-crypto random fallback replaced (issue OpenBMB#136)**
   When `crypto.randomUUID()` is unavailable, the fallback used
   `Math.random()` for token/ID generation — producing predictable
   tokens exploitable in WebSocket reconnection scenarios.

   Fix: use `crypto.getRandomValues(new Uint8Array(16))` converted
   to hex for the fallback path. Only falls through to `Math.random()`
   when no crypto API exists at all (extremely rare in modern runtimes).

Verified: `npx tsc --noEmit -p tsconfig.json` passes clean.

Closes: OpenBMB#128, OpenBMB#136

🤖 Generated with [Qoder][https://qoder.com]
@kevinchennewbee
Copy link
Copy Markdown
Author

Hi! Two small but impactful fixes in GatewayBrowserClient.ts:

AsyncEventQueue error swallowing (#128): The next() method was clearing this.error after the first read, so any retry or reconnection logic would see { done: true } instead of the actual error. This made WebSocket stream failures silently disappear. The fix preserves the error so all consumers (including retries) see the rejection.

Non-crypto random for client IDs (#136): generateClientId() fell back to Math.random() which is predictable and unsuitable for session tokens. Replaced with crypto.getRandomValues() — available in all modern runtimes — producing a 16-byte hex string from secure random bytes.

Both changes are localized and don't alter the public API. Let me know if anything needs tweaking!

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.

[SECURITY] Non-crypto random used for token generation fallback [BUG] AsyncEventQueue error state lost after first read

1 participant