Skip to content

fix(android): WebSocketModule stripping caller-supplied Cookie header#56579

Closed
psjostrom wants to merge 1 commit intofacebook:mainfrom
psjostrom:fix/android-websocket-cookie-header
Closed

fix(android): WebSocketModule stripping caller-supplied Cookie header#56579
psjostrom wants to merge 1 commit intofacebook:mainfrom
psjostrom:fix/android-websocket-cookie-header

Conversation

@psjostrom
Copy link
Copy Markdown
Contributor

Summary:

#55885 changed WebSocketModule to derive its OkHttpClient from OkHttpClientProvider.getOkHttpClient().newBuilder() in order to share the connection pool and dispatcher. The shared client carries ReactCookieJarContainer, so OkHttp's BridgeInterceptor now calls Request.Builder.header("Cookie", ...) on every outgoing request — case-insensitively replacing any Cookie / cookie header the caller passed via the WebSocket constructor's headers option.

This silently drops caller-supplied Cookie auth on the upgrade request. Apps that rely on new WebSocket(url, null, { headers: { cookie: ... } }) for authentication (a documented public API on Android/iOS) lose their session header on Android 0.83+, while iOS continues to work because the iOS WebSocket transport doesn't go through this interceptor pipeline.

Fix: explicitly set CookieJar.NO_COOKIES on the WebSocket-derived client. ForwardingCookieHandler cookies are still added manually via getCookie(), so cookies set via WebView's CookieManager keep flowing through. The connection pool and dispatcher sharing introduced by #55885 is preserved.

Changelog:

[ANDROID] [FIXED] - WebSocketModule no longer strips a Cookie header passed via the WebSocket constructor's headers option

Tested with:

Server logs the Cookie header it receives on the upgrade request:

const ws = new WebSocket('wss://example.com/ws', null, {
  headers: { cookie: 'session=abc' },
});
Build Server sees
0.81.6 Android Cookie: session=abc
0.83.6 Android (before this PR) Cookie: <whatever the cookie jar has, NOT session=abc>
0.83.6 Android (with this PR) Cookie: session=abc
iOS, all versions Cookie: session=abc

Verified locally on RN 0.83.6 + a production app whose WebSocket auth broke on the 0.83.6 upgrade — a real-time streaming feed silently downgraded logged-in users to anonymous access. Applying the same change via WebSocketModule.setCustomClientBuilder from the host app's MainApplication.onCreate (functionally identical to this patch) restored authenticated streaming. Verified fetch() and other HTTP requests still get the cookie jar's cookies correctly — only the WebSocket OkHttpClient is affected by this change.

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 23, 2026
@github-actions
Copy link
Copy Markdown

Warning

Missing Test Plan

Please add a "## Test Plan" section to your PR description. A Test Plan lets us know how these changes were tested.

@facebook-github-tools facebook-github-tools Bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Apr 23, 2026
Copy link
Copy Markdown
Member

@javache javache left a comment

Choose a reason for hiding this comment

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

Looks good to me. Is there any way to combine both, and get cookies from both BridgeInterceptor and the manually passed-in cookie header?

@meta-codesync
Copy link
Copy Markdown

meta-codesync Bot commented Apr 23, 2026

@javache has imported this pull request. If you are a Meta employee, you can view this in D102166959.

Copy link
Copy Markdown
Contributor

@cortinico cortinico left a comment

Choose a reason for hiding this comment

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

Review automatically exported from Phabricator review in Meta.

@meta-codesync meta-codesync Bot closed this in 24b51db Apr 23, 2026
@facebook-github-tools facebook-github-tools Bot added the Merged This PR has been merged. label Apr 23, 2026
@meta-codesync
Copy link
Copy Markdown

meta-codesync Bot commented Apr 23, 2026

@javache merged this pull request in 24b51db.

@react-native-bot
Copy link
Copy Markdown
Collaborator

This pull request was successfully merged by @psjostrom in 24b51db

When will my fix make it into a release? | How to file a pick request?

@psjostrom
Copy link
Copy Markdown
Contributor Author

Looks good to me. Is there any way to combine both, and get cookies from both BridgeInterceptor and the manually passed-in cookie header?

Sorry for not providing an answer in time. I was sketching on if it would be possible, and didn't have time to answer before it was merged :)

Technically yes — sketch below for reference. But honestly I don't think it's worth it, and since this PR is already merged it would have to be a follow-up.

The cookie-jar inheritance that #55885 introduced was a side effect of that PR — the stated goal was sharing the connection pool and dispatcher, with no mention of cookie semantics in the description, release notes, or any announcement. At least that is my understanding of it. Apps that genuinely need custom cookie-jar behavior on WebSocket can already use WebSocketModule.setCustomClientBuilder. The merge variant adds ~30 lines of cookie-parsing/precedence code to a hot path, plus questions like "what if the jar and the caller both supply a cookie with the same name" — solving a hypothetical at the cost of real complexity and ambiguous merge semantics.

The current PR restores known-good pre-#55885 behavior. If a real custom-jar use case surfaces, a follow-up can add it then.

For reference, the merge variant would look roughly like this — in addition to the existing import for okhttp3.CookieJar, add import okhttp3.HttpUrl.Companion.toHttpUrlOrNull. The jar stays disabled to prevent BridgeInterceptor from overriding what we just set; the merge happens manually inside connect(). Precedence is caller > jar > ForwardingCookieHandler (last-write-wins on same name, case-insensitive).

val sharedClient = OkHttpClientProvider.getOkHttpClient()
val okHttpBuilder =
    sharedClient.newBuilder()
        .cookieJar(CookieJar.NO_COOKIES)
        .connectTimeout(10, TimeUnit.SECONDS)
        .writeTimeout(10, TimeUnit.SECONDS)
        .readTimeout(0, TimeUnit.MINUTES)

applyCustomBuilder(okHttpBuilder)

val client = okHttpBuilder.build()
val builder = Request.Builder().tag(id).url(url)

// Merge cookies from ForwardingCookieHandler, the inherited CookieJar, and
// the caller's `Cookie` header into a single Cookie header (RFC 6265 §5.4).
// Last-write-wins on duplicate names (case-insensitive).
val cookieByName = LinkedHashMap<String, String>() // lowercased name -> "name=value"

fun parseCookieHeaderInto(headerValue: String) {
  for (pair in headerValue.split(';')) {
    val trimmed = pair.trim()
    if (trimmed.isEmpty()) continue
    val eq = trimmed.indexOf('=')
    if (eq <= 0) continue
    cookieByName[trimmed.substring(0, eq).trim().lowercase()] = trimmed
  }
}

// 1. ForwardingCookieHandler — legacy WebSocketModule path; reads WebView CookieManager.
this.getCookie(url)?.let(::parseCookieHeaderInto)

// 2. App-configured CookieJar on the shared client (e.g. via OkHttpClientFactory).
//    Convert ws/wss to http/https so HttpUrl.parse accepts the URL — same trick
//    Request.Builder.url(String) uses internally.
val cookieUrl = when {
  url.startsWith("wss:", ignoreCase = true) -> "https:" + url.substring(4)
  url.startsWith("ws:", ignoreCase = true) -> "http:" + url.substring(3)
  else -> url
}
cookieUrl.toHttpUrlOrNull()?.let { httpUrl ->
  for (cookie in sharedClient.cookieJar.loadForRequest(httpUrl)) {
    cookieByName[cookie.name.lowercase()] = "${cookie.name}=${cookie.value}"
  }
}

// 3. Caller-supplied Cookie header — captured during the headers loop below.
var hasOriginHeader = false
if (options?.hasKey("headers") == true && options.getType("headers") == ReadableType.Map) {
  val headers = checkNotNull(options.getMap("headers"))
  val iterator = headers.keySetIterator()
  while (iterator.hasNextKey()) {
    val key = iterator.nextKey()
    if (ReadableType.String == headers.getType(key)) {
      val value = checkNotNull(headers.getString(key)) { "value for name $key == null" }
      when {
        key.equals("origin", ignoreCase = true) -> {
          hasOriginHeader = true
          builder.addHeader(key, value)
        }
        key.equals("cookie", ignoreCase = true) -> parseCookieHeaderInto(value)
        else -> builder.addHeader(key, value)
      }
    } else {
      FLog.w(ReactConstants.TAG, "Ignoring: requested $key, value not a string")
    }
  }
}

if (cookieByName.isNotEmpty()) {
  builder.addHeader("Cookie", cookieByName.values.joinToString("; "))
}

Happy to send a follow-up PR with the merge variant if you'd rather ship it — but if I'm being honest, I'd leave it. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants