Repository navigation
fix(VSDK-909): use platform-default TLS validation for signaling WebSocket - #153
mobile-sdk-afk[bot] wants to merge 3 commits into
Conversation
…ocket Remove the react-native-websocket-self-signed transport, which unconditionally accepted any certificate for any host on the signaling connection (empty checkServerTrusted plus an always-true hostnameVerifier on Android; an unevaluated useCredential trust answer on iOS). Signaling now goes through the React Native built-in WebSocket via a new lib/signaling-socket.ts adapter, so TLS certificate and hostname validation follow platform defaults for both the production and development gateways. - Drop the dependency from package/package.json (version 1.1.0 -> 1.1.1) and from the demo app manifest; regenerate the root lockfile accordingly - Add a package/CHANGELOG.md Security entry under Unreleased - Re-point the two connection test mocks at the new adapter and add a signaling-socket test suite pinning the URL-only constructor contract
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa968134cf
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const WebSocketImpl = (globalThis as unknown as Record<string, unknown>) | ||
| .WebSocket; |
There was a problem hiding this comment.
Avoid requiring
globalThis on supported React Native versions
On older JavaScriptCore or Hermes runtimes covered by the package's react-native >=0.60.0 peer range, globalThis may not exist. Evaluating this identifier then throws a synchronous ReferenceError before connect() can return its promise, so Connection cannot open signaling at all. Read the WebSocket constructor from React Native's universally available global object or the ambient WebSocket, or raise the minimum supported runtime.
Useful? React with 👍 / 👎.
Address the P1 review thread on PR #153 (oPr0VwHkEu73mA6NW1kwDQ): the WebSocket constructor lookup evaluated a bare `globalThis` identifier, which throws a synchronous ReferenceError on older JavaScriptCore/Hermes runtimes within the react-native >=0.60.0 peer range - before connect() could return a rejected promise, so Connection could never open signaling. Resolve the global object through a typeof-guarded helper with React Native's universal `global` fallback, and pin the contract with two new tests (resolver identity + source-shape guard).
…ls-bypass # Conflicts: # package-lock.json
Summary
Removes the
react-native-websocket-self-signedtransport from the signaling connection and restores platform-default TLS certificate and hostname validation (VSDK-909).The previous transport unconditionally accepted any certificate for any host — an empty
checkServerTrustedplus an always-true hostname verifier on Android, and an unevaluated.useCredentialtrust answer on iOS — allowing a network-positioned attacker to intercept the signaling WebSocket and capture login credentials. Both documented gateways (wss://rtc.telnyx.com,wss://rtcdev.telnyx.com) serve publicly-signed certificates, so no deployment needs the bypass.What changed
package/lib/signaling-socket.ts(new) —SignalingSocketinterface +createSignalingSocket(url)adapter over the React Native built-inWebSocket. Constructed with the URL only — no custom trust options — so TLS validation follows platform defaults. Same listener/dedup/fresh-instance-per-reconnect semantics theConnectionclass expects.package/lib/connection.ts— swapsWebSocketSelfSigned.getInstance(wsUrl)forcreateSignalingSocket(wsUrl); connect/close/reconnect/teardown logic untouched. A handshake failure (untrusted certificate) now rejectsconnect()instead of ever treating the connection as established.package/package.json— dependencyreact-native-websocket-self-signedremoved; version bumped 1.1.0 → 1.1.1 per the acceptance criteria.package/CHANGELOG.md—### Securityentry underUnreleased(final release-note wording should stay coordinated with the SEC-1045 disclosure timing).react-native-websocket-self-signeddeclaration removed, lockfile regenerated (direct declaration purge; see risk notes for the transitive remainder).__tests__/signaling-socket.test.tspins the security contract (URL-only constructor — any second constructor argument fails the test — and handshake failures rejectingconnect()) plus lifecycle/reconnect behavior; version-pinned fixtures updated 1.1.0 → 1.1.1.Validation (run from this branch)
package/:Baseline on
mainat branch creation: 24 suites / 270 tests → the +1 suite / +10 tests is the newsignaling-socket.test.ts; all 270 pre-existing tests still pass.react-voice-commons-sdk/:Repo root:
Pre-existing on
main(same count) — unchanged by this PR;npm audit --audit-level=highstill exits non-zero due to the known demo-app dependency tree.Pre-existing on
main— both files untouched by this PR. All files included in this diff pass prettier (npx prettier --check "package/**/*.ts"→All matched files use Prettier code style!).Risk notes
@telnyx/react-native-voice-sdk^0.3.0from the npm registry, soreact-native-websocket-self-signedremains in its lockfile as a transitive dependency of the published 0.3.x tarball. It disappears from the demo once the demo app bumps to a patched release (1.1.1 of this package) — release-time follow-up, coordinated with the version bump above.Connectionsocket API (onOpen/onError/onClose/onMessage/connect/send/close/removeOn*Listener) is unchanged forclient.ts; connection-level integration paths (login →telnyx.socket.open→ attach) are covered by the existing suites, but a device smoke test (login + call on both gateways) is still recommended at review since the transport swap sits beneath runtime behavior.Linear
VSDK-909 — link posted on the ticket in the same handoff.
Review-fix cycle (2026-10-21)
Addresses P1 review thread
oPr0VwHkEu73mA6NW1kwDQ("Avoid requiring globalThis on supported React Native versions").e070e01):connect()no longer evaluates a bareglobalThisidentifier. A typeof-guardedgetGlobalObject()helper resolves the runtime's global object, falling back to React Native's universally availableglobalon older JavaScriptCore/Hermes runtimes (within thereact-native >=0.60.0peer range) whereglobalThisdoes not exist. Previously the unguarded evaluation threw a synchronousReferenceErrorbeforeconnect()could return its promise, soConnectioncould never open signaling at all. Security contract unchanged: URL-only WebSocket construction, platform-default TLS certificate + hostname validation, untrusted-cert handshake failures still rejectconnect().globalThisevaluation cannot return and thetypeof globalThis/typeof globalguarded fallbacks stay in place.e070e01:package/npx jest: 25 suites / 282 tests passed (baseline 280);react-voice-commons-sdk/npm run buildOK,npm test16 suites / 158 tests passed,npm run lint0 errors / 246 pre-existing warnings; rootnpm run quality:checkfails only on the 2 pre-existing unformatted READMEs (unchanged from main); rootnpm run security:checkreports 83 vulnerabilities (same total count as main, unchanged). Changed files prettier-clean.