fix: Safari magic link redirect on MacOS - #4464
Conversation
|
|
Hi @deepshekhardas, thanks for your interest in contributing! This project requires that pull request authors are vouched, and you are not in the list of vouched users. This PR will be closed automatically. See https://github.com/triggerdotdev/trigger.dev/blob/main/CONTRIBUTING.md for more details. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughThe session cookie ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Server-change note appears to be missing for this webapp-only change
CONTRIBUTING.md requires a .server-changes/ markdown file when a PR only touches server components under apps/webapp/. This PR only modifies apps/webapp/app/services/sessionStorage.server.ts and does not appear to add one, so the fix would not show up in release notes.
Was this helpful? React with 👍 or 👎 to provide feedback.
| cookie: { | ||
| name: "__session", | ||
| sameSite: "lax", | ||
| sameSite: env.NODE_ENV === "production" ? "none" : "lax", |
There was a problem hiding this comment.
🔴 Login sessions become unreliable on older Safari and lose cross-site protection
The login cookie is now sent on every cross-site request in production (sameSite: "none" at apps/webapp/app/services/sessionStorage.server.ts:14) instead of only same-site navigations, so users on older Safari versions can be silently signed out and every logged-in user's browser will attach their session to requests started by other websites.
Impact: Some users cannot stay logged in, and all users' dashboard actions can be triggered from other sites while they are signed in.
Why SameSite=None does not fix the magic-link case but does break other behaviour
A magic-link click is a top-level GET navigation, which is already allowed by SameSite=Lax; therefore None is not required for that flow. Meanwhile:
- Safari 12 (macOS 10.14) / iOS 12 implement the older spec and treat the literal value
NoneasStrict, so the__sessioncookie stops being sent on cross-site navigations at all — the exact browser family the PR aims to fix. - The webapp has no CSRF token layer (only
sameSite: "lax"comments inapps/webapp/app/services/onboardingSession.server.ts:7andapps/webapp/app/services/impersonation.server.ts:12), soLaxon__sessionis the sole protection for all Remix form actions. SettingNoneremoves it. - The change is also inconsistent with every other cookie in the app (
redirectTo,impersonation,lastAuthMethod,uiPreferences, github/google auth state), which all remainlax; if a cross-site cookie really were needed for the login flow,redirectTowould still fail.
Prompt for agents
The PR changes the main __session cookie to SameSite=None in production to work around a Safari magic-link redirect issue (#1384). This is unlikely to be the correct root-cause fix: magic-link clicks are top-level GET navigations, which SameSite=Lax already permits. Meanwhile SameSite=None (a) is interpreted as Strict by Safari 12/iOS 12, potentially worsening the exact scenario being fixed, and (b) removes the only CSRF mitigation the webapp has, since there is no CSRF token middleware and all other cookies in apps/webapp/app/services/*.server.ts stay on lax. Investigate the actual Safari failure: check whether the magic-link handler performs a cross-origin redirect (LOGIN_ORIGIN vs APP_ORIGIN in app/env.server.ts) that causes the Set-Cookie to be dropped, or whether Safari's link prefetch/preview consumes the one-time magic link before the user clicks. Prefer keeping sameSite lax and fixing the redirect/origin handling, or, if a cross-site cookie is genuinely required, apply it consistently to redirectTo and add CSRF token verification.
Was this helpful? React with 👍 or 👎 to provide feedback.
| cookie: { | ||
| name: "__session", | ||
| sameSite: "lax", | ||
| sameSite: env.NODE_ENV === "production" ? "none" : "lax", |
There was a problem hiding this comment.
🔍 Self-hosted HTTP deployments and SameSite=None
SameSite=None cookies are rejected outright by modern browsers unless they also carry Secure. Here secure is env.NODE_ENV === "production" (apps/webapp/app/services/sessionStorage.server.ts:18), so the two flags stay in sync today. However, self-hosters who terminate TLS incorrectly or run the container over plain HTTP with NODE_ENV=production would now have the login cookie silently discarded rather than merely being insecure — worth confirming against the self-hosting docs before merging.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Fixes #1384 - Safari magic link redirect on MacOS.