[ATL-763] [LUM-1951] fix(ios): set max-age on session cookie so it survives cold reopen#32529
Conversation
![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: 7171c28f56
ℹ️ 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".
| // `max-age` makes the cookie persistent. Without it WKWebView drops | ||
| // it on app kill and the user gets bounced back to login every cold | ||
| // reopen. | ||
| const cookieAttrs = "path=/; domain=.vellum.ai; secure; samesite=lax; max-age=1209600"; |
There was a problem hiding this comment.
This makes the JS-installed native session cookie survive cold restarts, but the web logout path only calls allauthLogout() best-effort and then clears local state in finally (apps/web/src/stores/auth-store.ts:243-251), even if the DELETE fails/returns non-ok and therefore no server Set-Cookie expiry is applied. In that offline/server-error scenario an iOS user appears signed out and is redirected to login, but after reopening the app the now-persistent valid cookie logs them back in; either explicitly expire these cookie names during logout cleanup or avoid persisting them without a matching client-side clear path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
minor edge case that I'm not gonna address here
Without max-age the cookie is session-scoped and WKWebView drops it when the app process is killed, falling through to biometric recovery on every cold reopen. 1209600 = 2 weeks, matches Django's SESSION_COOKIE_AGE default.
clopen-set
force-pushed
the
fix/ios-cookie-max-age
branch
from
7171c28 to
9d4ec51
Compare
| document.cookie = `sessionid=${sessionToken}; ${cookieAttrs}`; | ||
| document.cookie = `__Secure-sessionid=${sessionToken}; ${cookieAttrs}`; |
There was a problem hiding this comment.
We should only set one of these cookies, not both. Public SPA is served over HTTPS and should use the __Secure prefix
| // `max-age` makes the cookie persistent. If unspecified, the cookie | ||
| // expires at the end of the session, and users will be required to | ||
| // login again. | ||
| const cookieAttrs = "path=/; domain=.vellum.ai; secure; samesite=lax; max-age=1209600"; |
There was a problem hiding this comment.
I don't like that we're manually reinstalling cookies. Why not just use X-Session-Token for auth, like with macOS?
As is, this requires us to keep all of these attributes in sync with the actual cookie, but we're not doing properly right now:
domainis wrong. This should be same-host and not be made available on all subdomainsmax-agealso needs to be kept in sync with backend in case we tweak default expiry
There was a problem hiding this comment.
cc @noanflaherty in case this has implications for electron
clopen-set
changed the title
clopen-set
changed the title
1 participant
After completing auth exchange, the Capacitor shell installs a session cookie into
WKWebView's cookie jar viadocument.cookie. We're manually setting cookie attributes in an attempt to mirror the actual cookie generated by the server.vellum-assistant/apps/web/src/runtime/native-auth.ts
Lines 182 to 186 in 5567c70
This cookie doesn't specify
max-ageorexpires, so its lifetime is scoped to the client session. The cookie gets wiped if you quit the app, so users would have to log in again. Biometric recovery flow was a sort of bandaid over this.As another bandaid, let's give the cookie a 2 week expiry so that it actually persists. I don't love the cookie dance we're doing here.
Testing
Verified this on iphone with biometric recovery disabled: