Skip to content

fix(clerk-js,backend): Do not depend on _clerk_synced to terminate satellite sync flow #4516

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

Merged
merged 3 commits into from
Nov 13, 2024
Merged

fix(clerk-js,backend): Do not depend on _clerk_synced to terminate satellite sync flow #4516

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
349dd3b
fix(clerk-js,backend): Do not depend on _clerk_synced to terminate sa…
nikosdouvlis Nov 7, 2024
5d063f9
Merge branch 'main' into nikos/fix-satellite-issues-with-redirects
nikosdouvlis Nov 12, 2024
92a8a1f
Fix type
nikosdouvlis Nov 12, 2024
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
6 changes: 6 additions & 0 deletions .changeset/wild-schools-vanish.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
'@clerk/clerk-js': patch
'@clerk/backend': patch
---

Fixes satellite syncing when both the satellite and the primary apps use server-side enabled frameworks like NextJS
1 change: 1 addition & 0 deletions packages/backend/src/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ const Cookies = {

const QueryParameters = {
ClerkSynced: '__clerk_synced',
SuffixedCookies: 'suffixed_cookies',
ClerkRedirectUrl: '__clerk_redirect_url',
// use the reference to Cookies to indicate that it's the same value
DevBrowser: Cookies.DevBrowser,
Expand Down
18 changes: 9 additions & 9 deletions packages/backend/src/tokens/__tests__/authenticateContext.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ describe('AuthenticateContext', () => {
publishableKey: pkLive,
});

expect(context.suffixedCookies).toBe(false);
expect(context.usesSuffixedCookies()).toBe(false);
expect(context.sessionTokenInCookie).toBe(session);
expect(context.clientUat.toString()).toBe(clientUat);
});
Expand All @@ -60,7 +60,7 @@ describe('AuthenticateContext', () => {
publishableKey: pkLive,
});

expect(context.suffixedCookies).toBe(false);
expect(context.usesSuffixedCookies()).toBe(false);
expect(context.sessionTokenInCookie).toBe(newSession);
expect(context.clientUat.toString()).toBe(clientUat);
expect(context.devBrowserToken).toBe('__clerk_db_jwt');
Expand All @@ -80,7 +80,7 @@ describe('AuthenticateContext', () => {
publishableKey: pkTest,
});

expect(context.suffixedCookies).toBe(false);
expect(context.usesSuffixedCookies()).toBe(false);
expect(context.sessionTokenInCookie).toBe(session);
expect(context.clientUat.toString()).toBe(clientUat);
});
Expand All @@ -99,7 +99,7 @@ describe('AuthenticateContext', () => {
publishableKey: pkLive,
});

expect(context.suffixedCookies).toBe(true);
expect(context.usesSuffixedCookies()).toBe(true);
expect(context.sessionTokenInCookie).toBe(suffixedSession);
expect(context.clientUat.toString()).toBe('0');
});
Expand All @@ -119,7 +119,7 @@ describe('AuthenticateContext', () => {
const context = await createAuthenticateContext(clerkRequest, {
publishableKey: pkLive,
});
expect(context.suffixedCookies).toBe(true);
expect(context.usesSuffixedCookies()).toBe(true);
expect(context.sessionTokenInCookie).toBe(suffixedSession);
expect(context.clientUat.toString()).toBe(suffixedClientUat);
});
Expand All @@ -137,7 +137,7 @@ describe('AuthenticateContext', () => {
const context = await createAuthenticateContext(clerkRequest, {
publishableKey: pkLive,
});
expect(context.suffixedCookies).toBe(true);
expect(context.usesSuffixedCookies()).toBe(true);
expect(context.sessionTokenInCookie).toBe(suffixedSession);
expect(context.clientUat.toString()).toBe(suffixedClientUat);
});
Expand Down Expand Up @@ -166,7 +166,7 @@ describe('AuthenticateContext', () => {
publishableKey: pkTest,
});

expect(context.suffixedCookies).toBe(true);
expect(context.usesSuffixedCookies()).toBe(true);
expect(context.sessionTokenInCookie).toBe(suffixedSession);
expect(context.clientUat.toString()).toBe('0');
expect(context.devBrowserToken).toBe('__clerk_db_jwt-suffixed');
Expand All @@ -188,7 +188,7 @@ describe('AuthenticateContext', () => {
publishableKey: pkTest,
});

expect(context.suffixedCookies).toBe(true);
expect(context.usesSuffixedCookies()).toBe(true);
expect(context.sessionTokenInCookie).toBe(suffixedSession);
expect(context.clientUat.toString()).toBe('0');
expect(context.devBrowserToken).toBe('__clerk_db_jwt-suffixed');
Expand All @@ -207,7 +207,7 @@ describe('AuthenticateContext', () => {
publishableKey: pkLive,
});

expect(context.suffixedCookies).toBe(true);
expect(context.usesSuffixedCookies()).toBe(true);
expect(context.sessionTokenInCookie).toBeUndefined();
expect(context.clientUat.toString()).toBe('0');
});
Expand Down
146 changes: 72 additions & 74 deletions packages/backend/src/tokens/authenticateContext.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ interface AuthenticateContextInterface extends AuthenticateRequestOptions {
sessionTokenInCookie: string | undefined;
refreshTokenInCookie: string | undefined;
clientUat: number;
suffixedCookies: boolean;
// handshake-related values
devBrowserToken: string | undefined;
handshakeToken: string | undefined;
Expand Down Expand Up @@ -68,78 +67,7 @@ class AuthenticateContext {
this.clerkUrl = this.clerkRequest.clerkUrl;
}

private initPublishableKeyValues(options: AuthenticateRequestOptions) {
assertValidPublishableKey(options.publishableKey);
this.publishableKey = options.publishableKey;

const pk = parsePublishableKey(this.publishableKey, {
fatal: true,
proxyUrl: options.proxyUrl,
domain: options.domain,
});
this.instanceType = pk.instanceType;
this.frontendApi = pk.frontendApi;
}

private initHeaderValues() {
this.sessionTokenInHeader = this.stripAuthorizationHeader(this.getHeader(constants.Headers.Authorization));
this.origin = this.getHeader(constants.Headers.Origin);
this.host = this.getHeader(constants.Headers.Host);
this.forwardedHost = this.getHeader(constants.Headers.ForwardedHost);
this.forwardedProto =
this.getHeader(constants.Headers.CloudFrontForwardedProto) || this.getHeader(constants.Headers.ForwardedProto);
this.referrer = this.getHeader(constants.Headers.Referrer);
this.userAgent = this.getHeader(constants.Headers.UserAgent);
this.secFetchDest = this.getHeader(constants.Headers.SecFetchDest);
this.accept = this.getHeader(constants.Headers.Accept);
}

private initCookieValues() {
// suffixedCookies needs to be set first because it's used in getMultipleAppsCookie
this.suffixedCookies = this.shouldUseSuffixed();
this.sessionTokenInCookie = this.getSuffixedOrUnSuffixedCookie(constants.Cookies.Session);
this.refreshTokenInCookie = this.getSuffixedCookie(constants.Cookies.Refresh);
this.clientUat = Number.parseInt(this.getSuffixedOrUnSuffixedCookie(constants.Cookies.ClientUat) || '') || 0;
}

private initHandshakeValues() {
this.devBrowserToken =
this.getQueryParam(constants.QueryParameters.DevBrowser) ||
this.getSuffixedOrUnSuffixedCookie(constants.Cookies.DevBrowser);
// Using getCookie since we don't suffix the handshake token cookie
this.handshakeToken =
this.getQueryParam(constants.QueryParameters.Handshake) || this.getCookie(constants.Cookies.Handshake);
this.handshakeRedirectLoopCounter = Number(this.getCookie(constants.Cookies.RedirectCount)) || 0;
}

private stripAuthorizationHeader(authValue: string | undefined | null): string | undefined {
return authValue?.replace('Bearer ', '');
}

private getQueryParam(name: string) {
return this.clerkRequest.clerkUrl.searchParams.get(name);
}

private getHeader(name: string) {
return this.clerkRequest.headers.get(name) || undefined;
}

private getCookie(name: string) {
return this.clerkRequest.cookies.get(name) || undefined;
}

private getSuffixedCookie(name: string) {
return this.getCookie(getSuffixedCookieName(name, this.cookieSuffix)) || undefined;
}

private getSuffixedOrUnSuffixedCookie(cookieName: string) {
if (this.suffixedCookies) {
return this.getSuffixedCookie(cookieName);
}
return this.getCookie(cookieName);
}

private shouldUseSuffixed(): boolean {
public usesSuffixedCookies(): boolean {
Copy link
Member Author

@nikosdouvlis nikosdouvlis Nov 7, 2024

Choose a reason for hiding this comment

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

It's the same code - git diff messes with formatting here.
I dropped the suffixedCookies prop that was getting initialized in the ctor in favor of a simple function to simplify things a little.

const suffixedClientUat = this.getSuffixedCookie(constants.Cookies.ClientUat);
const clientUat = this.getCookie(constants.Cookies.ClientUat);
const suffixedSession = this.getSuffixedCookie(constants.Cookies.Session) || '';
Expand All @@ -158,7 +86,7 @@ class AuthenticateContext {
return true;
}

// If there is no suffixed cookies use un-suffixed
// If there are no suffixed cookies use un-suffixed
if (!suffixedClientUat && !suffixedSession) {
return false;
}
Expand Down Expand Up @@ -228,6 +156,76 @@ class AuthenticateContext {
return true;
}

private initPublishableKeyValues(options: AuthenticateRequestOptions) {
assertValidPublishableKey(options.publishableKey);
this.publishableKey = options.publishableKey;

const pk = parsePublishableKey(this.publishableKey, {
fatal: true,
proxyUrl: options.proxyUrl,
domain: options.domain,
});
this.instanceType = pk.instanceType;
this.frontendApi = pk.frontendApi;
}

private initHeaderValues() {
this.sessionTokenInHeader = this.stripAuthorizationHeader(this.getHeader(constants.Headers.Authorization));
this.origin = this.getHeader(constants.Headers.Origin);
this.host = this.getHeader(constants.Headers.Host);
this.forwardedHost = this.getHeader(constants.Headers.ForwardedHost);
this.forwardedProto =
this.getHeader(constants.Headers.CloudFrontForwardedProto) || this.getHeader(constants.Headers.ForwardedProto);
this.referrer = this.getHeader(constants.Headers.Referrer);
this.userAgent = this.getHeader(constants.Headers.UserAgent);
this.secFetchDest = this.getHeader(constants.Headers.SecFetchDest);
this.accept = this.getHeader(constants.Headers.Accept);
}

private initCookieValues() {
// suffixedCookies needs to be set first because it's used in getMultipleAppsCookie
this.sessionTokenInCookie = this.getSuffixedOrUnSuffixedCookie(constants.Cookies.Session);
this.refreshTokenInCookie = this.getSuffixedCookie(constants.Cookies.Refresh);
this.clientUat = Number.parseInt(this.getSuffixedOrUnSuffixedCookie(constants.Cookies.ClientUat) || '') || 0;
}

private initHandshakeValues() {
this.devBrowserToken =
this.getQueryParam(constants.QueryParameters.DevBrowser) ||
this.getSuffixedOrUnSuffixedCookie(constants.Cookies.DevBrowser);
// Using getCookie since we don't suffix the handshake token cookie
this.handshakeToken =
this.getQueryParam(constants.QueryParameters.Handshake) || this.getCookie(constants.Cookies.Handshake);
this.handshakeRedirectLoopCounter = Number(this.getCookie(constants.Cookies.RedirectCount)) || 0;
}

private stripAuthorizationHeader(authValue: string | undefined | null): string | undefined {
return authValue?.replace('Bearer ', '');
}

private getQueryParam(name: string) {
return this.clerkRequest.clerkUrl.searchParams.get(name);
}

private getHeader(name: string) {
return this.clerkRequest.headers.get(name) || undefined;
}

private getCookie(name: string) {
return this.clerkRequest.cookies.get(name) || undefined;
}

private getSuffixedCookie(name: string) {
return this.getCookie(getSuffixedCookieName(name, this.cookieSuffix)) || undefined;
}

private getSuffixedOrUnSuffixedCookie(cookieName: string) {
if (this.usesSuffixedCookies()) {
return this.getSuffixedCookie(cookieName);
}
return this.getCookie(cookieName);
}

private tokenHasIssuer(token: string): boolean {
const { data, errors } = decodeJwt(token);
if (errors) {
Expand Down
23 changes: 13 additions & 10 deletions packages/backend/src/tokens/request.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,10 @@ export async function authenticateRequest(

const url = new URL(`https://${frontendApiNoProtocol}/v1/client/handshake`);
url.searchParams.append('redirect_url', redirectUrl?.href || '');
url.searchParams.append('suffixed_cookies', authenticateContext.suffixedCookies.toString());
url.searchParams.append(
constants.QueryParameters.SuffixedCookies,
authenticateContext.usesSuffixedCookies().toString(),
);
url.searchParams.append(constants.QueryParameters.HandshakeReason, handshakeReason);

if (authenticateContext.instanceType === 'development' && authenticateContext.devBrowserToken) {
Expand Down Expand Up @@ -472,11 +475,6 @@ ${error.getFullMessage()}`,
const hasSessionToken = !!authenticateContext.sessionTokenInCookie;
const hasDevBrowserToken = !!authenticateContext.devBrowserToken;

const isRequestEligibleForMultiDomainSync =
authenticateContext.isSatellite &&
authenticateContext.secFetchDest === 'document' &&
!authenticateContext.clerkUrl.searchParams.has(constants.QueryParameters.ClerkSynced);

/**
* If we have a handshakeToken, resolve the handshake and attempt to return a definitive signed in or signed out state.
*/
Expand Down Expand Up @@ -512,6 +510,9 @@ ${error.getFullMessage()}`,
return handleMaybeHandshakeStatus(authenticateContext, AuthErrorReason.DevBrowserSync, '');
}

const isRequestEligibleForMultiDomainSync =
authenticateContext.isSatellite && authenticateContext.secFetchDest === 'document';

Comment on lines +513 to +515
Copy link
Member

@panteliselef panteliselef Nov 11, 2024

Choose a reason for hiding this comment

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

This is the actual change, correct ?

Copy link
Member Author

@nikosdouvlis nikosdouvlis Nov 11, 2024

Choose a reason for hiding this comment

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

Correct. I will check the clerk_go side shortly but we can push these changes independently

Copy link
Member Author

@nikosdouvlis nikosdouvlis Nov 11, 2024

Choose a reason for hiding this comment

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

The clerk-js change is also important as we couldn't remove the param before for nextjs apps.

/**
* Begin multi-domain sync flows
*/
Expand All @@ -529,17 +530,19 @@ ${error.getFullMessage()}`,
constants.QueryParameters.ClerkRedirectUrl,
authenticateContext.clerkUrl.toString(),
);
const authErrReason = AuthErrorReason.SatelliteCookieNeedsSyncing;
redirectURL.searchParams.append(constants.QueryParameters.HandshakeReason, authErrReason);

redirectURL.searchParams.append(
constants.QueryParameters.HandshakeReason,
AuthErrorReason.SatelliteCookieNeedsSyncing,
);
const headers = new Headers({ [constants.Headers.Location]: redirectURL.toString() });
return handleMaybeHandshakeStatus(authenticateContext, authErrReason, '', headers);
return handleMaybeHandshakeStatus(authenticateContext, AuthErrorReason.SatelliteCookieNeedsSyncing, '', headers);
}

// Multi-domain development sync flow
const redirectUrl = new URL(authenticateContext.clerkUrl).searchParams.get(
constants.QueryParameters.ClerkRedirectUrl,
);

if (authenticateContext.instanceType === 'development' && !authenticateContext.isSatellite && redirectUrl) {
// Dev MD sync from primary, redirect back to satellite w/ dev browser query param
const redirectBackToSatelliteUrl = new URL(redirectUrl);
Expand Down
16 changes: 8 additions & 8 deletions packages/clerk-js/src/core/clerk.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ import { assertNoLegacyProp } from '../utils/assertNoLegacyProp';
import { memoizeListenerCallback } from '../utils/memoizeStateListenerCallback';
import { RedirectUrls } from '../utils/redirectUrls';
import { AuthCookieService } from './auth/AuthCookieService';
import { CLERK_SATELLITE_URL, CLERK_SYNCED, ERROR_CODES } from './constants';
import { CLERK_SATELLITE_URL, CLERK_SUFFIXED_COOKIES, CLERK_SYNCED, ERROR_CODES } from './constants';
import {
clerkErrorInitFailed,
clerkInvalidSignInUrlFormat,
Expand Down Expand Up @@ -1635,9 +1635,6 @@ export class Clerk implements ClerkInterface {
return this.navigate(to);
}

#hasJustSynced = () => getClerkQueryParam(CLERK_SYNCED) === 'true';
#clearJustSynced = () => removeClerkQueryParam(CLERK_SYNCED);

#buildSyncUrlForDevelopmentInstances = (): string => {
const searchParams = new URLSearchParams({
[CLERK_SATELLITE_URL]: window.location.href,
Expand All @@ -1661,8 +1658,7 @@ export class Clerk implements ClerkInterface {
};

#shouldSyncWithPrimary = (): boolean => {
if (this.#hasJustSynced()) {
this.#clearJustSynced();
if (getClerkQueryParam(CLERK_SYNCED) === 'true') {
return false;
}

Expand Down Expand Up @@ -1818,7 +1814,7 @@ export class Clerk implements ClerkInterface {
}
}

this.#clearHandshakeFromUrl();
this.#clearClerkQueryParams();

this.#handleImpersonationFab();
return true;
Expand Down Expand Up @@ -2010,8 +2006,12 @@ export class Clerk implements ClerkInterface {
* The handshake payload is transported in the URL in development. In cases where FAPI is returning the handshake payload, but Clerk is being used in a client-only application,
* we remove the handshake associated parameters as they are not necessary.
*/
#clearHandshakeFromUrl = () => {
#clearClerkQueryParams = () => {
try {
removeClerkQueryParam(CLERK_SYNCED);
// @nikos: we're looking into dropping this param completely
// in the meantime, we're removing it here to keep the URL clean
removeClerkQueryParam(CLERK_SUFFIXED_COOKIES);
Comment on lines +2012 to +2014
Copy link
Member

@brkalow brkalow Nov 12, 2024

Choose a reason for hiding this comment

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

This should never make it to the client, I'm pretty sure it being added to the redirect is a bug in FAPI that needs to be fixed.

Copy link
Member Author

@nikosdouvlis nikosdouvlis Nov 12, 2024

Choose a reason for hiding this comment

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

That's exactly the case, I'll get to the bottom of it tomorrow

removeClerkQueryParam('__clerk_handshake');
removeClerkQueryParam('__clerk_help');
} catch (_) {
Expand Down
1 change: 1 addition & 0 deletions packages/clerk-js/src/core/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ export const PRESERVED_QUERYSTRING_PARAMS = [

export const CLERK_MODAL_STATE = '__clerk_modal_state';
export const CLERK_SYNCED = '__clerk_synced';
export const CLERK_SUFFIXED_COOKIES = 'suffixed_cookies';
export const CLERK_SATELLITE_URL = '__clerk_satellite_url';
export const ERROR_CODES = {
FORM_IDENTIFIER_NOT_FOUND: 'form_identifier_not_found',
Expand Down
Loading
Loading