Fix Android WebSocket cookie lookup stripping the URL path - #58365
Fix Android WebSocket cookie lookup stripping the URL path#58365lazerg wants to merge 2 commits into
Conversation
| requestURI.query, | ||
| requestURI.fragment, |
There was a problem hiding this comment.
Why do we need to include query and fragment?
Can we simplify this method to only re-create the URI if the scheme is ws(s)?
There was a problem hiding this comment.
You're right on both counts. Query and fragment don't affect cookie matching under RFC 6265 (cookies are scoped by domain + path only), so there's no reason to preserve them here, and since getCookie's input is always the WebSocket's own ws/wss URI, the method never actually needs to fall back to reusing an original http(s) URI unmodified.
Simplified in 476697c: getCookieLookupUri now passes null for query and fragment instead of forwarding them through, so the lookup URI is just scheme://host[:port]/path. Kept the existing URI(...) reconstruction (via httpSchemeFor) rather than branching on "only rebuild if scheme is ws(s)", since we still need to explicitly drop userInfo on every call for the credential-leak reason noted above the method, and that requires rebuilding the URI regardless of scheme.
|
@javache has imported this pull request. If you are a Meta employee, you can view this in D119093059. |
Summary:
WebSocketModule.getCookielooks up cookies throughgetDefaultOrigin(uri), which strips the URL down toscheme://host[:port]before handing it toForwardingCookieHandler. Android'sCookieManagermatches cookies against the full URL (domain and path), so any cookie set with aPathother than/gets silently dropped from the WebSocket handshake, breaking auth/session cookies scoped to a sub-path (e.g./signal-r/hubs/messages).getDefaultOriginis also used to build theoriginheader for the handshake, where stripping the path is correct per the WebSocket protocol, so it can't just be changed in place without affecting that header too. This adds a separategetCookieLookupUrithat does the samews(s)://tohttp(s)://scheme mapping but keeps the path, query, and fragment, and uses it only for the cookie lookup. It also drops the URI's userinfo from that lookup, since it plays no role in cookie matching and shouldn't be forwarded into the CookieManager call.Changelog:
[ANDROID] [FIXED] - Fix WebSocket cookie lookup dropping path-scoped cookies
Test Plan:
Added
WebSocketModuleTest, exercisinggetCookieLookupUrithrough reflection and asserting the path, port, and query survive the ws/wss -> http/https conversion. Couldn't run it through the repo's own Gradle/Robolectric setup in this environment (react-native-gradle-pluginisn't resolvable without the full monorepo build), so I compiled the real companion object withkotlincstandalone and ran the same reflection lookup against it directly, confirming both cases pass and that the private companion method resolves without aNoSuchMethodException.Fixes #58358