fix(ios): add cookie handling to proxy scheme handler#463
fix(ios): add cookie handling to proxy scheme handler#463WcaleNieWolny wants to merge 3 commits intofeature/proxy-redesignfrom
Conversation
WKWebView's cookie store and URLSession's cookie jar are separate. When the scheme handler proxies requests via URLSession, Set-Cookie headers from responses are not automatically synced back to WKWebView, breaking session cookies (e.g. Google reCAPTCHA). Three fixes: 1. Set httpShouldHandleCookies=false to prevent URLSession from adding its own cookies (WKWebView already includes them in request headers) 2. Sync Set-Cookie headers from pass-through responses back to WKWebView's WKHTTPCookieStore before delivering the response 3. Use delegate-based URLSession to intercept redirects — sync cookies from each redirect response and inject WKWebView cookies for the redirect target domain, ensuring redirect chains maintain sessions
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8129abba29
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if !matchingCookies.isEmpty { | ||
| let headers = HTTPCookie.requestHeaderFields(with: matchingCookies) | ||
| for (key, value) in headers { | ||
| modifiedRequest.setValue(value, forHTTPHeaderField: key) | ||
| } |
There was a problem hiding this comment.
When matchingCookies is empty, the code leaves modifiedRequest’s existing Cookie header untouched, so a redirect to another host can forward cookies from the previous hop instead of sending none. This is a real cross-domain leakage path because newRequest may carry the original Cookie header; in that case, redirects without target-domain cookies still send source-domain cookies to the new origin.
Useful? React with 👍 / 👎.
| let matchingCookies = allCookies.filter { cookie in | ||
| guard let host = redirectURL.host else { return false } | ||
| let domain = cookie.domain | ||
| if host == domain { return true } | ||
| if domain.hasPrefix(".") { |
There was a problem hiding this comment.
The redirect cookie selection only checks domain/host and then serializes all matches into Cookie, which ignores important cookie constraints like Path and Secure. This can attach cookies that WKWebView would not send (for example, /app cookies on unrelated paths or secure cookies on non-HTTPS redirects), causing auth inconsistencies and potential cookie exposure.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR updates the iOS ProxySchemeHandler pass-through path to keep WKWebView’s cookie store (WKHTTPCookieStore) in sync with URLSession requests/responses, including redirects, to avoid cookie duplication and missing Set-Cookie propagation.
Changes:
- Switches pass-through networking to a delegate-based
URLSessionand disables URLSession cookie handling (httpShouldHandleCookies = false). - Adds
Set-Cookiesyncing from pass-through (and JS-provided) responses intoWKHTTPCookieStore. - Adds redirect handling that syncs cookies from redirect responses and injects
WKWebViewcookies into redirect requests.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| private var pendingNetworkTasks: [String: URLSessionDataTask] = [:] | ||
| private var pendingWebViews: [String: WKWebView] = [:] | ||
| private var stoppedRequests: Set<String> = [] |
There was a problem hiding this comment.
pendingWebViews stores strong references to WKWebView. Because the WKWebView configuration retains its WKURLSchemeHandler, this can create a temporary retain cycle (and can become a leak if a request never completes/cleanup isn’t called). Consider storing WKHTTPCookieStore / WKWebsiteDataStore instead of the full WKWebView, or use a weak wrapper for the webview reference.
| @@ -75,6 +90,7 @@ public class ProxySchemeHandler: NSObject, WKURLSchemeHandler { | |||
| return | |||
| } | |||
| self.pendingBodies.removeValue(forKey: requestId) | |||
| self.pendingWebViews.removeValue(forKey: requestId) | |||
There was a problem hiding this comment.
The timeout removes pendingTasks unconditionally. For pass-through, pendingTasks is intentionally kept while the URLSession task runs, so any request taking longer than proxyTimeoutSeconds will be failed even though JS already responded (with nil). Consider cancelling the timeout once handleResponse is called, or skipping timeout failure when a network task is already in-flight for this requestId.
| completion() | ||
| return | ||
| } | ||
| let headerFields = response.allHeaderFields as? [String: String] ?? [:] |
There was a problem hiding this comment.
response.allHeaderFields as? [String: String] can fail (header values are often Any/NSNumber), which would drop all cookies and prevent Set-Cookie sync. Consider building a [String: String] dictionary by stringifying keys/values instead of using a conditional cast.
| let headerFields = response.allHeaderFields as? [String: String] ?? [:] | |
| var headerFields: [String: String] = [:] | |
| for (key, value) in response.allHeaderFields { | |
| headerFields[String(describing: key)] = String(describing: value) | |
| } |
| cookieStore.setCookie(cookie) { group.leave() } | ||
| } | ||
| group.notify(queue: .global(), execute: completion) |
There was a problem hiding this comment.
WKHTTPCookieStore / WKWebView access and the completion for syncCookies are happening off the main queue (group.notify(queue: .global())). Elsewhere in the codebase cookie-store operations are dispatched to .main; WebKit objects aren’t generally thread-safe. Consider dispatching setCookie/getAllCookies and the completion (which triggers urlSchemeTask.didReceive/didFinish) onto DispatchQueue.main.
| cookieStore.setCookie(cookie) { group.leave() } | |
| } | |
| group.notify(queue: .global(), execute: completion) | |
| DispatchQueue.main.async { | |
| cookieStore.setCookie(cookie) { | |
| group.leave() | |
| } | |
| } | |
| } | |
| group.notify(queue: .main, execute: completion) |
| webView.configuration.websiteDataStore.httpCookieStore.getAllCookies { allCookies in | ||
| var modifiedRequest = request | ||
| modifiedRequest.httpShouldHandleCookies = false | ||
|
|
||
| let matchingCookies = allCookies.filter { cookie in | ||
| guard let host = redirectURL.host else { return false } | ||
| let domain = cookie.domain | ||
| if host == domain { return true } | ||
| if domain.hasPrefix(".") { | ||
| return host.hasSuffix(domain) || host == String(domain.dropFirst()) | ||
| } | ||
| return false | ||
| } | ||
| if !matchingCookies.isEmpty { | ||
| let headers = HTTPCookie.requestHeaderFields(with: matchingCookies) | ||
| for (key, value) in headers { | ||
| modifiedRequest.setValue(value, forHTTPHeaderField: key) | ||
| } | ||
| } | ||
| completionHandler(modifiedRequest) | ||
| } |
There was a problem hiding this comment.
Manual cookie injection in urlSession(_:willPerformHTTPRedirection:newRequest:completionHandler:) uses getAllCookies and filters only by domain before calling HTTPCookie.requestHeaderFields, which ignores cookie attributes like Secure and path. This can cause Secure cookies to be sent over plain HTTP redirects and path-scoped cookies to be exposed to unrelated paths, enabling session hijacking or cross-app data leakage if an attacker can influence redirects or network traffic. To fix, use the cookie store API that enforces cookie rules for a specific URL and ensure that Secure cookies are only attached to https requests and that path constraints are respected when building the redirect request.
Address Codex review findings: - P1: Clear Cookie header when no cookies match the redirect target, preventing source-domain cookies from leaking to a different host - P2: Add path and secure attribute checks to cookie filtering, not just domain matching
WKWebView.configuration must only be accessed from the main thread. The URLSession delegate callbacks (redirect handling, completion) run on a background queue, causing a Main Thread Checker crash when syncCookies or willPerformHTTPRedirection accessed webView.configuration. Fix: capture the WKHTTPCookieStore reference in webView(_:start:) which is guaranteed to run on the main thread, then pass it through instead of the WKWebView reference. This eliminates all background-thread access to WKWebView.configuration.
|
Summary
httpShouldHandleCookiesduplicate/conflict issues by disabling URLSession's cookie managementSet-Cookieheaders from pass-through responses back toWKHTTPCookieStoreRoot cause
WKWebView has its own cookie store (
WKHTTPCookieStore), separate from URLSession'sHTTPCookieStorage.shared. WhenexecutePassThroughproxied requests throughURLSession.shared:Set-Cookieheaders from responses were never synced back to WKWebViewThis broke any site relying on session cookies through pass-through (e.g., Google reCAPTCHA).
Changes
httpShouldHandleCookies = falsesyncCookies()helperSet-CookiefromHTTPURLResponseand stores them inWKHTTPCookieStorewith async completionURLSessiondataTasksowillPerformHTTPRedirectionfirespendingWebViewstrackinginvalidateSession()Test plan