Skip to content

fix: Safari magic link redirect on MacOS #4464

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
deepshekhardas wants to merge 1 commit into from
Closed

fix: Safari magic link redirect on MacOS #4464

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion apps/webapp/app/services/sessionStorage.server.ts
View file
Open in desktop

@devin-ai-integration devin-ai-integration Bot Aug 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 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.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ export const DEFAULT_SESSION_DURATION_SECONDS = 31_556_952;
export const sessionStorage = createCookieSessionStorage({
cookie: {
name: "__session",
sameSite: "lax",
sameSite: env.NODE_ENV === "production" ? "none" : "lax",

@devin-ai-integration devin-ai-integration Bot Aug 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 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 None as Strict, so the __session cookie 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 in apps/webapp/app/services/onboardingSession.server.ts:7 and apps/webapp/app/services/impersonation.server.ts:12), so Lax on __session is the sole protection for all Remix form actions. Setting None removes it.
  • The change is also inconsistent with every other cookie in the app (redirectTo, impersonation, lastAuthMethod, uiPreferences, github/google auth state), which all remain lax; if a cross-site cookie really were needed for the login flow, redirectTo would 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.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

@devin-ai-integration devin-ai-integration Bot Aug 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 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.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

path: "/",
httpOnly: true,
secrets: [env.SESSION_SECRET],
Expand Down