fix(ios): plant Django session cookie natively so it survives cold reopen#32410
fix(ios): plant Django session cookie natively so it survives cold reopen#32410clopen-set wants to merge 3 commits into
Conversation
…open The JS path was setting the cookie via `document.cookie` with no Max-Age, making it a session cookie that WKWebView drops when the app process is killed. Every cold reopen lost the cookie and fell through to the Keychain biometric path, prompting Face/Touch ID even when the user hadn't actually signed out. Move cookie planting to a shared `SessionCookieInstaller` invoked from `NativeAuthPlugin` (after the OAuth exchange) and `NativeBiometricPlugin` (after Keychain retrieval). The native path can set `Max-Age`, `HttpOnly`, and a host-only `Domain`, and writes directly to WKWebView's `WKHTTPCookieStore` so there's no async flush to wait on. Also fixes three secondary issues that came along with the JS plant: the cookie was scoped to `.vellum.ai` (leaking the session to every subdomain), it was not HttpOnly (readable by any XSS), and both `sessionid` and `__Secure-sessionid` were planted on every login when only one is read by the deployed backend. Removes the `waitForNativeSessionCookie` polling workaround — no longer needed because the native write completes synchronously before the plugin promise resolves.
![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: f39a0d6d39
ℹ️ 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".
…ession
Codex caught a regression: once the cookie is HttpOnly, the settings
toggle's `getSessionTokenFromCookies()` call returns null, so flipping
biometrics on mid-session silently fails to store a token in the
Keychain while still flipping the preference — the next cookie expiry
then can't be recovered with Face/Touch ID.
Add `NativeBiometric.readSessionCookie({ serverURL })` which reads
the cookie out of WKWebView's jar (where the native side can see
HttpOnly cookies), and only flip the preference if storing the token
succeeded. Deletes the now-dead `getSessionTokenFromCookies` helper.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0dff3881e6
ℹ️ 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".
…cookie Codex caught that `readSessionCookie`'s matcher iterated the cookie store via `first(where:)` with a `Set`, so on HTTPS builds with both cookies present, store order rather than name preference decided which one was returned. Upgraded users (whose WKWebView jars may still carry the legacy `.vellum.ai`-scoped `sessionid` from the pre-PR dual-cookie planting) could see the stale token end up in the Keychain when toggling biometrics on. Switch to a two-pass lookup: look for the preferred name first, only fall back to bare `sessionid` if no `__Secure-` cookie is present.
|
Codex Review: Didn't find any major issues. You're on a roll. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Closing — this PR scoped in too much. The user-reported bug (Touch ID on every cold reopen) has a one-line fix: add This PR also fixed three latent issues that came along "for free" once I moved cookie planting to native Swift: HttpOnly (defense in depth, not critical for our single-origin shell), host-only Domain (no leak to other vellum.ai subdomains), and dropping the duplicate cookie name. None of those were reported and bundling them silently was a scope-creep mistake. They each deserve their own change with their own discussion. Will reopen a minimal max-age-only PR after local QA. |
|
Closed in favor of #32529 It's a simpler approach that preserves existing JS cookie setting flow. We should still fix the overly broad domain + cookie naming issues |
1 participant
Closing & reopening the iOS app prompted for Touch ID every time because the Django session cookie was being planted from JS via
document.cookiewith noMax-Age— so WKWebView treated it as a session cookie and dropped it every time the app process was killed. Every cold reopen then fell through to the Keychain biometric-recovery path.Move cookie planting to the native side (
SessionCookieInstaller, called fromNativeAuthPluginafter OAuth andNativeBiometricPluginafter Keychain recovery). The native write can do whatdocument.cookiecan't:Max-Age(fixes the Touch ID bug)HttpOnly(XSS can no longer read the session token)Domaininstead of.vellum.ai(no leakage to other subdomains)__Secure-sessionidon HTTPS,sessionidon LAN-IP HTTP dev)Also deletes the 50ms
waitForNativeSessionCookiepolling — the native write is synchronous so the race it papered over is gone.The settings card had to switch to a new
NativeBiometric.readSessionCookieto capture the now-HttpOnly cookie when toggling biometrics on mid-session.Test plan
On a real iPhone: log in, kill the app, reopen — should land in the authed app with no Touch ID prompt. Web Inspector should show
__Secure-sessionid's Expires column as a real date ~2 weeks out (not "Session").