Skip to content

Commit

Permalink
fix(clerk-js): Set localhost SameSite=None cookies as Secure (#3604)
Browse files Browse the repository at this point in the history
This change removes the specific fix we did a while ago for Cypress.
The real reason our cookies where getting deleted in Cypress Chrome is because Chrome requires Secure when setting SameSite=None cookies.

Safari needs a bit of a different handling because it does not consider localhost to be a secure context, so in Safari localhost cookies cannot have the Secure attribute.

So the new logic about the Secure attribute on cookies goes as follows:
- If you are in a https location then add Secure on cookies.
- If you are not in https:
  1. If the browser is Safari, then do not add Secure (safari considers secure only https origins).
  2. If the window.isSecureContext property exists, then respect it (safari does not work correctly with this, that's why we added a condition before this)
  3. Lastly, if the window location is localhost and SameSite is set to None, add the Secure attribute (that's required by Chrome)
  • Loading branch information
anagstef authored Jul 10, 2024
1 parent 1f79d3d commit 6a98c08
Show file tree
Hide file tree
Showing 7 changed files with 136 additions and 9 deletions.
5 changes: 5 additions & 0 deletions .changeset/plenty-pandas-leave.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@clerk/clerk-js": patch
---

Set the localhost cookies with the Secure attribute
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
import { getSecureAttribute } from '../getSecureAttribute';

describe('getSecureAttribute', () => {
let windowSpy: any;

beforeEach(() => {
windowSpy = jest.spyOn(window, 'window', 'get');
});

afterEach(() => {
windowSpy.mockRestore();
});

it('returns true if the protocol is https', () => {
windowSpy.mockImplementation(() => ({
location: new URL('https://www.clerk.com'),
safari: undefined,
isSecureContext: undefined,
}));
expect(getSecureAttribute('None')).toBe(true);
});

it('returns false if the protocol is not https and the SameSite is not None', () => {
windowSpy.mockImplementation(() => ({
location: new URL('http://www.clerk.com'),
safari: undefined,
isSecureContext: undefined,
}));
expect(getSecureAttribute('Lax')).toBe(false);
});

it('returns false if the protocol is not https and the browser is Safari', () => {
windowSpy.mockImplementation(() => ({
location: new URL('http://www.clerk.com'),
safari: { dummyValue: true },
isSecureContext: undefined,
}));
expect(getSecureAttribute('None')).toBe(false);
});

it('returns true if isSecureContext is true', () => {
windowSpy.mockImplementation(() => ({
location: new URL('http://www.clerk.com'),
safari: undefined,
isSecureContext: true,
}));
expect(getSecureAttribute('None')).toBe(true);
});

it('returns false if isSecureContext is false', () => {
windowSpy.mockImplementation(() => ({
location: new URL('http://www.clerk.com'),
safari: undefined,
isSecureContext: false,
}));
expect(getSecureAttribute('None')).toBe(false);
});

it('returns true if the protocol is http and the hostname is localhost and sameSite is None', () => {
windowSpy.mockImplementation(() => ({
location: new URL('http://localhost'),
safari: undefined,
isSecureContext: undefined,
}));
expect(getSecureAttribute('None')).toBe(true);
});

it('returns false if the protocol is http and the hostname is localhost and sameSite is not None', () => {
windowSpy.mockImplementation(() => ({
location: new URL('http://localhost'),
safari: undefined,
isSecureContext: undefined,
}));
expect(getSecureAttribute('Lax')).toBe(false);
});

it('returns false if the protocol is http and the hostname is not localhost', () => {
windowSpy.mockImplementation(() => ({
location: new URL('http://www.clerk.com'),
safari: undefined,
isSecureContext: undefined,
}));
expect(getSecureAttribute('None')).toBe(false);
});

it('returns false in case window.safari is undefined and isSecureContext is true on localhost for Lax cookie', () => {
windowSpy.mockImplementation(() => ({
location: new URL('http://localhost'),
safari: undefined,
isSecureContext: true,
}));
expect(getSecureAttribute('Lax')).toBe(false);
});
});
3 changes: 2 additions & 1 deletion packages/clerk-js/src/core/auth/cookies/clientUat.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import type { ClientResource } from '@clerk/types';

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

const CLIENT_UAT_COOKIE_NAME = '__client_uat';

Expand All @@ -28,7 +29,7 @@ export const createClientUatCookie = (): ClientUatCookieHandler => {
const set = (client: ClientResource | undefined) => {
const expires = addYears(Date.now(), 1);
const sameSite = inCrossOriginIframe() ? 'None' : 'Strict';
const secure = window.location.protocol === 'https:';
const secure = getSecureAttribute(sameSite);
const domain = getCookieDomain();

// '0' indicates the user is signed out
Expand Down
3 changes: 2 additions & 1 deletion packages/clerk-js/src/core/auth/cookies/devBrowser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { addYears } from '@clerk/shared/date';
import { DEV_BROWSER_JWT_KEY } from '@clerk/shared/devBrowser';

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

export type DevBrowserCookieHandler = {
set: (jwt: string) => void;
Expand All @@ -24,7 +25,7 @@ export const createDevBrowserCookie = (): DevBrowserCookieHandler => {
const set = (jwt: string) => {
const expires = addYears(Date.now(), 1);
const sameSite = inCrossOriginIframe() ? 'None' : 'Lax';
const secure = window.location.protocol === 'https:';
const secure = getSecureAttribute(sameSite);

return devBrowserCookie.set(jwt, {
expires,
Expand Down
3 changes: 2 additions & 1 deletion packages/clerk-js/src/core/auth/cookies/session.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { createCookieHandler } from '@clerk/shared/cookie';
import { addYears } from '@clerk/shared/date';

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

const SESSION_COOKIE_NAME = '__session';

Expand All @@ -23,7 +24,7 @@ export const createSessionCookie = (): SessionCookieHandler => {
const set = (token: string) => {
const expires = addYears(Date.now(), 1);
const sameSite = inCrossOriginIframe() ? 'None' : 'Lax';
const secure = window.location.protocol === 'https:';
const secure = getSecureAttribute(sameSite);

return sessionCookie.set(token, {
expires,
Expand Down
30 changes: 30 additions & 0 deletions packages/clerk-js/src/core/auth/getSecureAttribute.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
type SameSiteAttributeType = 'None' | 'Lax' | 'Strict';

// The Secure attribute is required for SameSite=None on Chrome and Firefox
// Also, localhost is considered secure for testing purposes by Chrome and Firefox
// Safari does not support the Secure attribute on localhost, although it returns true for isSecureContext
// ref: https://bugs.webkit.org/show_bug.cgi?id=232088#c8
export const getSecureAttribute = (sameSite: SameSiteAttributeType): boolean => {
if (window.location.protocol === 'https:') {
return true;
}

// If the cookie is not SameSite=None, then the Secure attribute is not required
if (sameSite !== 'None') {
return false;
}

// This is because Safari does not support the Secure attribute on localhost
if (typeof (window as any).safari !== 'undefined') {
return false;
}

// This is a useful way to check if the current context is secure
// This is needed for example in environments like Firefox Remote Control
// where the `localhost` is not considered secure
if (typeof window.isSecureContext !== 'undefined') {
return window.isSecureContext;
}

return window.location.hostname === 'localhost';
};
7 changes: 1 addition & 6 deletions packages/clerk-js/src/utils/runtime.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,9 @@ export function usesHttps() {
return inBrowser() && window.location.protocol === 'https:';
}

function isCypress() {
return typeof window !== 'undefined' && typeof (window as any).Cypress !== 'undefined';
}

export function inIframe() {
// checks if the current window is an iframe
// excludes the case where the current window runs in a Cypress test
return inBrowser() && window.self !== window.top && !isCypress();
return inBrowser() && window.self !== window.top;
}

export function inCrossOriginIframe() {
Expand Down

0 comments on commit 6a98c08

Please sign in to comment.