RocketChat / RocketChat/Rocket.Chat.ReactNative

Android: pinned OkHttpClient lazy fallback can permanently bind to non-pinned client

Open
#7,248 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

needs-security-review type: bug
Dominant language
TypeScript
Stars
2.4k
Forks
1.5k
Avg merge
1d 18h
Merged PRs (30d)
90

Description

Problem

MediaCallsAnswerRequest.kt:45-53 and DDPClient.kt:29 both initialize the OkHttp client via a companion by lazy block:

```kotlin
private val httpClient: OkHttpClient by lazy {
SSLPinningTurboModule.getSharedOkHttpClient() ?: OkHttpClient()
}
```

If a VoIP notification (or DDP message) arrives during cold start before the user has selected a certificate, the lazy initializer triggers when the shared pinned client is still null, permanently binding to a plain OkHttpClient() for the process lifetime. Subsequent calls — including manual certificate selection later in the session — reuse the cached non-pinned fallback and silently bypass SSL pinning.

Where this surfaces

  • VoIP accept REST (MediaCallsAnswerRequest.fetch) when invoked from a PushKit/FCM cold-start handler
  • DDPClient connections under similar cold-start conditions

Suggested fix

Resolve the shared client at each call site rather than at lazy initialization, OR fail-fast with onResult(false) when getSharedOkHttpClient() returns null.

Background

Spotted by CodeRabbit on https://github.com/RocketChat/Rocket.Chat.ReactNative/pull/7245 — this is a pre-existing bug, not introduced by that PR. Filing separately so it doesn't block the release-readiness queue.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with MediaCallsAnswerRequest.kt:45-53 and DDPClient.kt:29, then trace how their companion lazy clients are used by MediaCallsAnswerRequest.fetch and DDPClient connections. Reproduce the cold-start path with no selected certificate, then select one and verify subsequent calls no longer reuse a plain client or silently bypass SSL pinning.

Written by the indexing model from the issue text.

Assessment

Tech stack
android, kotlin
Domain
mobile, security
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
68/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.