fix(android): sync Cronet and token-refresh requests with CookieManager#73
Conversation
- NitroFetchClient: attach Cookie from CookieManager when request has no Cookie header; persist Set-Cookie from responses (including redirects). - AutoPrefetcher: same for HttpURLConnection token refresh; persist Set-Cookie from refresh response. Helps SAML/session flows where the session cookie lives in the WebView cookie jar. Made-with: Cursor
|
Hey @oferRounds , Thanks for the PR . |
|
Sure, let me know what :) |
Can you please look at my review comments 😅 |
| } | ||
| } | ||
| } | ||
| cookieManager.flush() |
There was a problem hiding this comment.
This will run everytime even though we dont have cookies . Maybe we can add check here to run only if actually set-cookie exist
There was a problem hiding this comment.
Also maybe we flush once on onSucceeded . Idk if thats good . Ideally we should not flush on every redirect
| for (header in setCookieHeaders) { | ||
| cookieManager.setCookie(responseUrl, header.value) | ||
| } | ||
| cookieManager.flush() |
| conn.doInput = true | ||
| if (body != null) conn.doOutput = true | ||
|
|
||
| var hasCookieHeader = false |
There was a problem hiding this comment.
Lets create this in a function like NitroFetchClient.kt . Lets avoid 2 code doing the exact thing but in different ways
|
good catches! Let me fix them |
- NitroCookieSync: shared attach + Set-Cookie helpers; flush only when cookies applied - Cronet: apply Set-Cookie on redirects without flush; flush once on success when redirects or final response stored cookies - HttpURLConnection token refresh: reuse attach helper; Set-Cookie + flush only if needed Made-with: Cursor
|
Addressed the review feedback in a follow-up commit:
Let me know if you want anything tweaked. |
HttpURLConnection follows redirects by default; conn.url returns the final URL, which is the correct domain to associate Set-Cookie with. Made-with: Cursor
|
Thanks. Looks much better now. Will see to merge tomorrow :) |
|
So just one minor thing after discussing this internally . Can we make this opt-in and configurable. Because requests that were previously anonymous may now become authenticated or behave differently . So lets avoid this breaking change |
…eSync() Cookie sync is now disabled by default — requests that were previously anonymous stay that way unless the consumer explicitly calls NitroCookieSync.enableCookieSync() from Application.onCreate. All public methods (attach, store, flush) short-circuit with a no-op when disabled. Addresses: margelo#73 (comment) Made-with: Cursor
|
Done! Pushed a follow-up commit (25f5968): Cookie sync is now opt-in — // e.g. in MainApplication.onCreate, before AutoPrefetcher.prefetchOnStart
NitroCookieSync.enableCookieSync()This way existing consumers see zero behavior change, and apps that rely on WebView cookie bridging can explicitly opt in. |
|
@riteshshukla04 is attempting to deploy a commit to the Margelo Team on Vercel. A member of the Team first needs to authorize it. |
2 participants
Summary
Aligns Android Cronet (
NitroFetchClient) and the HttpURLConnection cold-start token-refresh path (AutoPrefetcher) withCookieManager: attachCookiewhen the request has noCookieheader, and persistSet-Cookiefrom responses (including redirects).Helps SAML/session flows where the session cookie lives in the WebView cookie jar.
Split from #71
This is PR 1 of 2 from the discussion on #71 (atomic PRs per review).
PR 2 (upstream): #74 — cold-start token-refresh outcome → JS (draft until this PR lands; rebase after merge).
Outcome-only diff vs this branch (fork stack): oferRounds#1
How to verify
Exercise cold-start prefetch + token refresh on Android with a session that relies on cookies set via WebView; confirm requests include the expected cookie and refreshed cookies are stored.