fix: Safari magic link redirect on MacOS#3799
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 Run ID: 📒 Files selected for processing (1)
WalkthroughThe session cookie configuration in Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes ✨ 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}
| cookie: { | ||
| name: "__session", | ||
| sameSite: "lax", | ||
| sameSite: env.NODE_ENV === "production" ? "none" : "lax", |
There was a problem hiding this comment.
🔴 Changing sameSite to "none" in production removes CSRF protection from the session cookie
Setting sameSite: "none" means the __session cookie will be sent on all cross-site requests in production. The application has no alternative CSRF protection mechanism — no CSRF tokens, no double-submit cookies — as confirmed by searching the codebase for "csrf"/"CSRF". The only references to CSRF in the entire webapp are comments noting that sameSite: "lax" itself is the CSRF defense (e.g. apps/webapp/app/services/impersonation.server.ts:12). Every other cookie in the app (impersonation, onboardingSession, referralSource, redirectTo, gitHubSession, lastAuthMethod, uiPreferences, OAuth state cookies) all remain sameSite: "lax". An attacker could host a page that issues POST requests (or other state-changing methods) to the Trigger.dev webapp, and the victim's session cookie would be included automatically — allowing task triggers, settings changes, or other authenticated actions without consent.
Prompt for agents
The change from sameSite: 'lax' to sameSite: 'none' removes the application's only CSRF protection for the session cookie. The app has no CSRF token mechanism to compensate.
If cross-site cookie sending is genuinely needed (e.g. for an iframe embed scenario), you must add an alternative CSRF protection layer — such as per-request CSRF tokens verified on state-changing endpoints, or Origin/Referer header validation. Without such a mechanism, any authenticated POST/PUT/DELETE endpoint is vulnerable to cross-site request forgery.
If the cross-site requirement is narrow (e.g. only needed for specific API endpoints), consider keeping sameSite: 'lax' on the session cookie and using a separate authentication mechanism (like Bearer tokens) for the cross-site use case.
Relevant files: apps/webapp/app/services/sessionStorage.server.ts (the changed file), apps/webapp/app/services/impersonation.server.ts:12 (comment confirming lax = CSRF defense).
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.
🚩 All other cookies in the webapp remain sameSite: "lax" — inconsistent security posture
Every other cookie-based session storage in the webapp hardcodes sameSite: "lax": OAuth state cookies (auth.google.ts:35, auth.github.ts:35), impersonation (impersonation.server.ts:12), onboarding (onboardingSession.server.ts:7), redirect (redirectTo.server.ts:12), referral (referralSource.server.ts:15), GitHub session (gitHubSession.server.ts:12), UI preferences (uiPreferences.server.ts:7), last auth method (lastAuthMethod.server.ts:10), and messages (message.server.ts:42). The main session cookie is the only one being changed to "none". If the intent is to support cross-origin embedding, the other cookies (especially OAuth state cookies) would also need updating, otherwise cross-origin login flows would still fail since the OAuth cookies wouldn't be sent.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Fixes #1384
Safari's Intelligent Tracking Prevention (ITP) blocks session cookies set during OAuth/magic link redirect flows when the navigation originates from a cross-site context (e.g., clicking a magic link in an email).
The root cause was the session cookie's SameSite=Lax attribute. While Lax allows cookies on top-level GET navigations, Safari ITP can still block the setting of new cookies on the response during cross-site navigations where the user has no prior interaction with the domain.
Changed sameSite to "none" in production (where secure: true and HTTPS is used), which tells the browser to allow the cookie in cross-site contexts. Development on localhost retains "lax" since localhost does not have HTTPS (required for SameSite=None).
Changed file: apps/webapp/app/services/sessionStorage.server.ts