fix(android): add User-Agent interceptor to shared OkHttpClient#7427
fix(android): add User-Agent interceptor to shared OkHttpClient#7427mateussiqueira wants to merge 1 commit into
Conversation
Native HTTP calls on Android were leaking the default Dalvik User-Agent on call sites that didn't explicitly set one. This caused WAF rules and server logs to see 'okhttp/...' instead of 'RC Mobile'. Add a centralized userAgentInterceptor() that adds 'RC Mobile; android ...' to every outgoing request that doesn't already carry an explicit User-Agent. Applied to both getSharedOkHttpClient() and getOkHttpClient(). Affected sites now covered automatically: - SSLPinningTurboModule (ExpoImageClient, etc.) - MediaCallsAnswerRequest (VoIP accept/decline) - Any future call site using the shared client Fixes RocketChat#7342
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📜 Recent review details🔇 Additional comments (1)
WalkthroughA ChangesUser-Agent Interceptor for Android OkHttp
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Hi @mateussiqueira, Thanks for addressing this and adding the missing User-Agent to the native OkHttp client on Android! This ensures that all native background network operations properly identify as the official mobile client, which is crucial for server logging and firewall/WAF rule evaluation. Here is a technical recommendation to optimize memory usage and avoid object allocation churn: Reuse the Interceptor InstanceCurrently, private static Interceptor userAgentInterceptor() {
return chain -> { ... };
}Since the interceptor behavior is completely stateless and relies entirely on static utility methods ( private static final Interceptor USER_AGENT_INTERCEPTOR = chain -> {
Request original = chain.request();
if (original.header("User-Agent") != null) {
return chain.proceed(original);
}
Request request = original.newBuilder()
.header("User-Agent", NotificationHelper.getUserAgent())
.build();
return chain.proceed(request);
};And then simply reuse it in both OkHttpClient.Builder builder = new OkHttpClient.Builder()
.addInterceptor(USER_AGENT_INTERCEPTOR)
...This prevents allocation slop during native networking module setups. Other than that, the integration is very clean and the use of the centralized |
Shevilll
left a comment
There was a problem hiding this comment.
User-Agent Interceptor Feedback: Android Network Call Sites
Hi @mateussiqueira, thank you for addressing the User-Agent leak on Android native network calls! Standardizing on the RC Mobile User-Agent is an excellent step for both logging clarity and WAF rule consistency.
I've analyzed the changes and found a subtle but critical edge case where the default Dalvik/... / okhttp/... user-agent will still be leaked.
⚠️ The Bug: User-Agent Leak/Bypass when SSL Pinning is Unconfigured
The PR adds userAgentInterceptor() to both getSharedOkHttpClient() and getOkHttpClient().
However, let's look at how getSharedOkHttpClient() is designed:
public static OkHttpClient getSharedOkHttpClient() {
if (sharedClient != null) {
return sharedClient;
}
if (alias != null) {
OkHttpClient.Builder builder = new OkHttpClient.Builder()
.addInterceptor(userAgentInterceptor())
...
sharedClient = builder.build();
return sharedClient;
}
return null;
}If alias is null (which occurs when SSL Pinning is not configured or disabled on the current workspace), getSharedOkHttpClient() returns null.
Now, look at the call sites that consume this method on Android:
MediaCallsAnswerRequest.kt(line 46):val base = SSLPinningTurboModule.getSharedOkHttpClient() ?: OkHttpClient()
DDPClient.kt(line 23):SSLPinningTurboModule.getSharedOkHttpClient() ?: OkHttpClient.Builder()
The Concern:
When SSL Pinning is disabled (i.e. alias is null), getSharedOkHttpClient() returns null. Both of these call sites fall back to calling OkHttpClient() or OkHttpClient.Builder() directly.
Because these fallback clients are instantiated outside of SSLPinningTurboModule, they completely bypass the userAgentInterceptor(). Consequently, they will still leak the default okhttp or Dalvik User-Agent to the server, defeating the purpose of this PR on non-pinned workspaces.
Recommendation:
Expose a static helper method in SSLPinningTurboModule that returns a pre-configured OkHttpClient.Builder containing the userAgentInterceptor(), or ensure getSharedOkHttpClient() returns a default shared non-pinned client with the interceptor instead of null when alias is null.
For example, we could define:
public static OkHttpClient.Builder getOkHttpClientBuilder() {
return new OkHttpClient.Builder()
.addInterceptor(userAgentInterceptor())
.connectTimeout(0, TimeUnit.MILLISECONDS)
.readTimeout(0, TimeUnit.MILLISECONDS)
.writeTimeout(0, TimeUnit.MILLISECONDS)
.cookieJar(new ReactCookieJarContainer());
}And then in MediaCallsAnswerRequest.kt / DDPClient.kt, fall back to that builder so that the User-Agent header (and timeouts/cookie configuration) is consistently set for all native network requests:
val base = SSLPinningTurboModule.getSharedOkHttpClient() ?: SSLPinningTurboModule.getOkHttpClientBuilder().build()Thank you again for this contribution! This addition is highly valuable, and addressing this edge case will make it completely bulletproof across all server/client environments.
Problem
Native HTTP calls on Android were leaking the default
Dalvik/...User-Agent on call sites that didn't explicitly set one. This caused WAF rules and server logs to seeokhttp/...instead ofRC Mobile.Confirmed sites missing the
RC MobileUA:SSLPinningTurboModule.getSharedOkHttpClient()(used by ExpoImageClient, etc.)MediaCallsAnswerRequest.kt(VoIP accept/decline) — inherited via shared clientSolution
Add a centralized
userAgentInterceptor()to bothgetSharedOkHttpClient()andgetOkHttpClient(). The interceptor:User-Agentheader → passes through unchangedRC Mobile; android {version}; v{appVersion} ({build})Uses the existing
NotificationHelper.getUserAgent()so the format stays consistent with the manual headers already set inLoadNotification.java,ReplyBroadcast.java, andNotificationHelper.Fixes #7342
Summary by CodeRabbit