Skip to content

feat(clerk-js): Use SameSite=none by default for development instances #6374

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.

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

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
5 changes: 5 additions & 0 deletions .changeset/great-rats-argue.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@clerk/clerk-js': minor
---

Adjusts cookie setting in development so Clerk can function in embedded iframe scenarios.
4 changes: 2 additions & 2 deletions packages/clerk-js/src/core/auth/AuthCookieService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,8 @@ export class AuthCookieService {
this.refreshTokenOnFocus();
this.startPollingForToken();

this.clientUat = createClientUatCookie(cookieSuffix);
this.sessionCookie = createSessionCookie(cookieSuffix);
this.clientUat = createClientUatCookie({ cookieSuffix, isProduction: this.instanceType === 'production' });
this.sessionCookie = createSessionCookie({ cookieSuffix, isProduction: this.instanceType === 'production' });
this.activeCookie = createCookieHandler('clerk_active_context');
this.devBrowser = createDevBrowser({
frontendApi: clerk.frontendApi,
Expand Down
51 changes: 25 additions & 26 deletions packages/clerk-js/src/core/auth/cookies/__tests__/clientUat.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,39 +35,21 @@ describe('createClientUatCookie', () => {
});

it('should create both suffixed and non-suffixed cookie handlers', () => {
createClientUatCookie(mockCookieSuffix);
createClientUatCookie({ cookieSuffix: mockCookieSuffix, isProduction: false });
expect(createCookieHandler).toHaveBeenCalledTimes(2);
expect(createCookieHandler).toHaveBeenCalledWith('__client_uat');
expect(createCookieHandler).toHaveBeenCalledWith('__client_uat_test-suffix');
});

it('should set cookies with correct parameters in non-cross-origin context', () => {
const cookieHandler = createClientUatCookie(mockCookieSuffix);
const cookieHandler = createClientUatCookie({ cookieSuffix: mockCookieSuffix, isProduction: false });
cookieHandler.set({
id: 'test-client',
updatedAt: new Date('2024-01-01'),
signedInSessions: ['session1'],
});

expect(mockSet).toHaveBeenCalledTimes(2);
expect(mockSet).toHaveBeenCalledWith('1704067200', {
domain: mockDomain,
expires: mockExpires,
sameSite: 'Strict',
secure: true,
partitioned: false,
});
});

it('should set cookies with None sameSite in cross-origin context', () => {
(inCrossOriginIframe as jest.Mock).mockReturnValue(true);
const cookieHandler = createClientUatCookie(mockCookieSuffix);
cookieHandler.set({
id: 'test-client',
updatedAt: new Date('2024-01-01'),
signedInSessions: ['session1'],
});

expect(mockSet).toHaveBeenCalledWith('1704067200', {
domain: mockDomain,
expires: mockExpires,
Expand All @@ -78,20 +60,20 @@ describe('createClientUatCookie', () => {
});

it('should set value to 0 when client is undefined', () => {
const cookieHandler = createClientUatCookie(mockCookieSuffix);
const cookieHandler = createClientUatCookie({ cookieSuffix: mockCookieSuffix, isProduction: false });
cookieHandler.set(undefined);

expect(mockSet).toHaveBeenCalledWith('0', {
domain: mockDomain,
expires: mockExpires,
sameSite: 'Strict',
sameSite: 'None',
secure: true,
partitioned: false,
});
});

it('should set value to 0 when client has no signed in sessions', () => {
const cookieHandler = createClientUatCookie(mockCookieSuffix);
const cookieHandler = createClientUatCookie({ cookieSuffix: mockCookieSuffix, isProduction: false });
cookieHandler.set({
id: 'test-client',
updatedAt: new Date('2024-01-01'),
Expand All @@ -101,7 +83,7 @@ describe('createClientUatCookie', () => {
expect(mockSet).toHaveBeenCalledWith('0', {
domain: mockDomain,
expires: mockExpires,
sameSite: 'Strict',
sameSite: 'None',
secure: true,
partitioned: false,
});
Expand All @@ -110,7 +92,7 @@ describe('createClientUatCookie', () => {
it('should get cookie value from suffixed cookie first, then fallback to non-suffixed', () => {
mockGet.mockImplementationOnce(() => '1234567890').mockImplementationOnce(() => '9876543210');

const cookieHandler = createClientUatCookie(mockCookieSuffix);
const cookieHandler = createClientUatCookie({ cookieSuffix: mockCookieSuffix, isProduction: false });
const result = cookieHandler.get();

expect(result).toBe(1234567890);
Expand All @@ -119,9 +101,26 @@ describe('createClientUatCookie', () => {
it('should return 0 when no cookie value is present', () => {
mockGet.mockImplementationOnce(() => undefined).mockImplementationOnce(() => undefined);

const cookieHandler = createClientUatCookie(mockCookieSuffix);
const cookieHandler = createClientUatCookie({ cookieSuffix: mockCookieSuffix, isProduction: false });
const result = cookieHandler.get();

expect(result).toBe(0);
});

it('should set SameSite=Strict in production', () => {
const cookieHandler = createClientUatCookie({ cookieSuffix: mockCookieSuffix, isProduction: true });
cookieHandler.set({
id: 'test-client',
updatedAt: new Date('2024-01-01'),
signedInSessions: ['session1'],
});

expect(mockSet).toHaveBeenCalledWith('1704067200', {
domain: mockDomain,
expires: mockExpires,
sameSite: 'Strict',
secure: true,
partitioned: false,
});
});
});
56 changes: 38 additions & 18 deletions packages/clerk-js/src/core/auth/cookies/__tests__/session.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,59 +32,67 @@ describe('createSessionCookie', () => {
});

it('should create both suffixed and non-suffixed cookie handlers', () => {
createSessionCookie(mockCookieSuffix);
createSessionCookie({ cookieSuffix: mockCookieSuffix, isProduction: false });
expect(createCookieHandler).toHaveBeenCalledTimes(2);
expect(createCookieHandler).toHaveBeenCalledWith('__session');
expect(createCookieHandler).toHaveBeenCalledWith('__session_test-suffix');
});

it('should set cookies with correct parameters in non-cross-origin context', () => {
const cookieHandler = createSessionCookie(mockCookieSuffix);
it('should set cookies with correct parameters', () => {
const cookieHandler = createSessionCookie({ cookieSuffix: mockCookieSuffix, isProduction: false });
cookieHandler.set(mockToken);

expect(mockSet).toHaveBeenCalledTimes(2);
expect(mockSet).toHaveBeenCalledWith(mockToken, {
expires: mockExpires,
sameSite: 'Lax',
sameSite: 'None',
secure: true,
partitioned: false,
});
});

it('should set cookies with None sameSite in cross-origin context', () => {
(inCrossOriginIframe as jest.Mock).mockReturnValue(true);
const cookieHandler = createSessionCookie(mockCookieSuffix);
it('should remove both cookies when remove is called', () => {
const cookieHandler = createSessionCookie({ cookieSuffix: mockCookieSuffix, isProduction: false });
cookieHandler.remove();

expect(mockRemove).toHaveBeenCalledTimes(2);
});

it('should remove cookies with the same attributes as set', () => {
const cookieHandler = createSessionCookie({ cookieSuffix: mockCookieSuffix, isProduction: false });
cookieHandler.set(mockToken);
cookieHandler.remove();

const expectedAttributes = {
sameSite: 'None',
secure: true,
partitioned: false,
};

expect(mockSet).toHaveBeenCalledWith(mockToken, {
expires: mockExpires,
sameSite: 'None',
secure: true,
partitioned: false,
});
});

it('should remove both cookies when remove is called', () => {
const cookieHandler = createSessionCookie(mockCookieSuffix);
cookieHandler.remove();

expect(mockRemove).toHaveBeenCalledTimes(2);
expect(mockRemove).toHaveBeenCalledWith(expectedAttributes);
});

it('should remove cookies with the same attributes as set', () => {
const cookieHandler = createSessionCookie(mockCookieSuffix);
const cookieHandler = createSessionCookie({ cookieSuffix: mockCookieSuffix, isProduction: false });
cookieHandler.set(mockToken);
cookieHandler.remove();

const expectedAttributes = {
sameSite: 'Lax',
sameSite: 'None',
secure: true,
partitioned: false,
};

expect(mockSet).toHaveBeenCalledWith(mockToken, {
expires: mockExpires,
sameSite: 'Lax',
sameSite: 'None',
secure: true,
partitioned: false,
});
Expand All @@ -98,7 +106,7 @@ describe('createSessionCookie', () => {
it('should get cookie value from suffixed cookie first, then fallback to non-suffixed', () => {
mockGet.mockImplementationOnce(() => 'suffixed-value').mockImplementationOnce(() => 'non-suffixed-value');

const cookieHandler = createSessionCookie(mockCookieSuffix);
const cookieHandler = createSessionCookie({ cookieSuffix: mockCookieSuffix, isProduction: false });
const result = cookieHandler.get();

expect(result).toBe('suffixed-value');
Expand All @@ -107,9 +115,21 @@ describe('createSessionCookie', () => {
it('should fallback to non-suffixed cookie when suffixed cookie is not present', () => {
mockGet.mockImplementationOnce(() => undefined).mockImplementationOnce(() => 'non-suffixed-value');

const cookieHandler = createSessionCookie(mockCookieSuffix);
const cookieHandler = createSessionCookie({ cookieSuffix: mockCookieSuffix, isProduction: false });
const result = cookieHandler.get();

expect(result).toBe('non-suffixed-value');
});

it('should not set SameSite=None in production', () => {
const cookieHandler = createSessionCookie({ cookieSuffix: mockCookieSuffix, isProduction: true });
cookieHandler.set(mockToken);

expect(mockSet).toHaveBeenCalledWith(mockToken, {
expires: mockExpires,
sameSite: 'Lax',
secure: true,
partitioned: false,
});
});
});
16 changes: 14 additions & 2 deletions packages/clerk-js/src/core/auth/cookies/clientUat.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,13 @@ export type ClientUatCookieHandler = {
* The cookie is used as hint from the Clerk Backend packages to identify
* if the user is authenticated or not.
*/
export const createClientUatCookie = (cookieSuffix: string): ClientUatCookieHandler => {
export const createClientUatCookie = ({
cookieSuffix,
isProduction,
}: {
cookieSuffix: string;
isProduction: boolean;
}): ClientUatCookieHandler => {
const clientUatCookie = createCookieHandler(CLIENT_UAT_COOKIE_NAME);
const suffixedClientUatCookie = createCookieHandler(getSuffixedCookieName(CLIENT_UAT_COOKIE_NAME, cookieSuffix));

Expand All @@ -37,7 +43,13 @@ export const createClientUatCookie = (cookieSuffix: string): ClientUatCookieHand
* Generally, this is handled by redirectWithAuth() being called and relying on the dev browser ID in the URL,
* but if that isn't used we rely on this. In production, nothing is cross-domain and Lax is used when client_uat is set from FAPI.
*/
const sameSite = __BUILD_VARIANT_CHIPS__ ? 'None' : inCrossOriginIframe() ? 'None' : 'Strict';
const sameSite = __BUILD_VARIANT_CHIPS__
? 'None'
: inCrossOriginIframe()
Copy link
Member

Choose a reason for hiding this comment

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

What was making inCrossOriginIframe to not detect the iframe usage inside the GenAI tools ?

Copy link
Member Author

Choose a reason for hiding this comment

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

This was working, but one issue we ran into with this approach is that the app would set SameSite=lax cookies when the preview was opened in a new tab, and so cookies would not persist between the iframe'd preview and the preview opened in its own tab. In this case, it seemed preferable to always set SameSite=None.

? 'None'
: isProduction
? 'Strict'
: 'None';
const secure = getSecureAttribute(sameSite);
const partitioned = __BUILD_VARIANT_CHIPS__ && secure;
const domain = getCookieDomain();
Expand Down
5 changes: 2 additions & 3 deletions packages/clerk-js/src/core/auth/cookies/devBrowser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import { addYears } from '@clerk/shared/date';
import { DEV_BROWSER_JWT_KEY } from '@clerk/shared/devBrowser';
import { getSuffixedCookieName } from '@clerk/shared/keys';

import { inCrossOriginIframe } from '../../../utils';
import { getSecureAttribute } from '../getSecureAttribute';

export type DevBrowserCookieHandler = {
Expand All @@ -13,7 +12,7 @@ export type DevBrowserCookieHandler = {
};

const getCookieAttributes = (): { sameSite: string; secure: boolean } => {
const sameSite = inCrossOriginIframe() ? 'None' : 'Lax';
const sameSite = 'None';
const secure = getSecureAttribute(sameSite);
return { sameSite, secure };
};
Expand All @@ -24,7 +23,7 @@ const getCookieAttributes = (): { sameSite: string; secure: boolean } => {
* The cookie is used to authenticate FAPI requests and pass
* authentication from AP to the app.
*/
export const createDevBrowserCookie = (cookieSuffix: string): DevBrowserCookieHandler => {
export const createDevBrowserCookie = ({ cookieSuffix }: { cookieSuffix: string }): DevBrowserCookieHandler => {
const devBrowserCookie = createCookieHandler(DEV_BROWSER_JWT_KEY);
const suffixedDevBrowserCookie = createCookieHandler(getSuffixedCookieName(DEV_BROWSER_JWT_KEY, cookieSuffix));

Expand Down
20 changes: 15 additions & 5 deletions packages/clerk-js/src/core/auth/cookies/session.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,12 @@ export type SessionCookieHandler = {
get: () => string | undefined;
};

const getCookieAttributes = (): { sameSite: string; secure: boolean; partitioned: boolean } => {
const sameSite = __BUILD_VARIANT_CHIPS__ ? 'None' : inCrossOriginIframe() ? 'None' : 'Lax';
const getCookieAttributes = ({
isProduction,
}: {
isProduction: boolean;
}): { sameSite: string; secure: boolean; partitioned: boolean } => {
const sameSite = __BUILD_VARIANT_CHIPS__ ? 'None' : inCrossOriginIframe() ? 'None' : isProduction ? 'Lax' : 'None';
const secure = getSecureAttribute(sameSite);
const partitioned = __BUILD_VARIANT_CHIPS__ && secure;
return { sameSite, secure, partitioned };
Expand All @@ -25,19 +29,25 @@ const getCookieAttributes = (): { sameSite: string; secure: boolean; partitioned
* The cookie is used by the Clerk backend SDKs to identify
* the authenticated user.
*/
export const createSessionCookie = (cookieSuffix: string): SessionCookieHandler => {
export const createSessionCookie = ({
cookieSuffix,
isProduction,
}: {
cookieSuffix: string;
isProduction: boolean;
}): SessionCookieHandler => {
const sessionCookie = createCookieHandler(SESSION_COOKIE_NAME);
const suffixedSessionCookie = createCookieHandler(getSuffixedCookieName(SESSION_COOKIE_NAME, cookieSuffix));

const remove = () => {
const attributes = getCookieAttributes();
const attributes = getCookieAttributes({ isProduction });
sessionCookie.remove(attributes);
suffixedSessionCookie.remove(attributes);
};

const set = (token: string) => {
const expires = addYears(Date.now(), 1);
const { sameSite, secure, partitioned } = getCookieAttributes();
const { sameSite, secure, partitioned } = getCookieAttributes({ isProduction });

// If setting Partitioned to true, remove the existing session cookies.
// This is to avoid conflicts with the same cookie name without Partitioned attribute.
Expand Down
2 changes: 1 addition & 1 deletion packages/clerk-js/src/core/auth/devBrowser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ export type CreateDevBrowserOptions = {
};

export function createDevBrowser({ cookieSuffix, frontendApi, fapiClient }: CreateDevBrowserOptions): DevBrowser {
const devBrowserCookie = createDevBrowserCookie(cookieSuffix);
const devBrowserCookie = createDevBrowserCookie({ cookieSuffix });

function getDevBrowserJWT() {
return devBrowserCookie.get();
Expand Down
Loading