chore(repo): Retry handshake in case of handshake cookie collision (#3848)

Co-authored-by: Dimitris Klouvas <[email protected]>

Author: nikosdouvlis

.changeset/sixty-mugs-clap.md

@@ -0,0 +1,5 @@
+---
+"@clerk/backend": patch
+---
+
+Retry handshake in case of handshake cookie collision in order to support multiple apps on same-level subdomains

integration/tests/sessions/root-subdomain-prod-instances.test.ts

@@ -302,4 +302,77 @@ test.describe('root and subdomain production apps @sessions', () => {
       await u[1].po.expect.toBeSignedIn();
     });
   });
+
+  /**
+   * This smoke test verifies that the session is not shared between different apps running on different same-level subdomains, while
+   * using different Clerk instances. For extra details, look at the previous test.
+   *
+   * sub1.test.com <> clerk-instance-1
+   * sub2.test.com <> clerk-instance-2
+   *
+   */
+  test.describe('multiple apps different same-level subdomains  for different production instances', () => {
+    const hosts = ['sub-1.multiple-apps-e2e.clerk.app', 'sub-2.multiple-apps-e2e.clerk.app'];
+    let fakeUsers: FakeUser[];
+    let server: Server;
+    let apps: Array<{ app: Application; serverUrl: string }>;
+
+    test.beforeAll(async () => {
+      apps = await Promise.all([prepareApplication('sessions-prod-1'), prepareApplication('sessions-prod-2')]);
+
+      // TODO: Move this into createProxyServer
+      const ssl: Pick<ServerOptions, 'ca' | 'cert' | 'key'> = {
+        cert: fs.readFileSync(constants.CERTS_DIR + '/sessions.pem'),
+        key: fs.readFileSync(constants.CERTS_DIR + '/sessions-key.pem'),
+      };
+
+      server = createProxyServer({
+        ssl,
+        targets: {
+          [hosts[0]]: apps[0].serverUrl,
+          [hosts[1]]: apps[1].serverUrl,
+        },
+      });
+
+      const u = apps.map(a => createTestUtils({ app: a.app }));
+      fakeUsers = await Promise.all(u.map(u => u.services.users.createFakeUser()));
+      await Promise.all([
+        await u[0].services.users.createBapiUser(fakeUsers[0]),
+        await u[1].services.users.createBapiUser(fakeUsers[1]),
+      ]);
+    });
+
+    test.afterAll(async () => {
+      await Promise.all(fakeUsers.map(u => u.deleteIfExists()));
+      await Promise.all(apps.map(({ app }) => app.teardown()));
+      server.close();
+    });
+
+    test('the cookies are independent for the root and sub domains', async ({ context }) => {
+      const pages = await Promise.all([context.newPage(), context.newPage()]);
+      const u = [
+        createTestUtils({ app: apps[0].app, page: pages[0], context }),
+        createTestUtils({ app: apps[1].app, page: pages[1], context }),
+      ];
+
+      await u[0].page.goto(`https://${hosts[0]}`);
+      await u[0].po.signIn.goTo();
+      await u[0].po.signIn.signInWithEmailAndInstantPassword(fakeUsers[0]);
+      await u[0].po.expect.toBeSignedIn();
+      const tab0User = await u[0].po.clerk.getClientSideUser();
+      // make sure that the backend user now matches the user we signed in with on the client
+      expect((await u[0].page.evaluate(() => fetch('/api/me').then(r => r.json()))).userId).toBe(tab0User.id);
+
+      u[1].po.expect.toBeHandshake(await u[1].page.goto(`https://${hosts[1]}`));
+      await u[1].po.expect.toBeSignedOut();
+      expect((await u[1].page.evaluate(() => fetch('/api/me').then(r => r.json()))).userId).toBe(null);
+
+      await u[1].po.signIn.goTo();
+      await u[1].po.signIn.signInWithEmailAndInstantPassword(fakeUsers[1]);
+      await u[1].po.expect.toBeSignedIn();
+      const tab1User = await u[1].po.clerk.getClientSideUser();
+      // make sure that the backend user now matches the user we signed in with on the client
+      expect((await u[1].page.evaluate(() => fetch('/api/me').then(r => r.json()))).userId).toBe(tab1User.id);
+    });
+  });
 });

packages/backend/src/constants.ts

@@ -19,6 +19,7 @@ const Cookies = {
   ClientUat: '__client_uat',
   Handshake: '__clerk_handshake',
   DevBrowser: '__clerk_db_jwt',
+  RedirectCount: '__clerk_redirect_count',
 } as const;
 
 const QueryParameters = {

packages/backend/src/tokens/authenticateContext.ts

@@ -26,6 +26,7 @@ interface AuthenticateContextInterface extends AuthenticateRequestOptions {
   // handshake-related values
   devBrowserToken: string | undefined;
   handshakeToken: string | undefined;
+  handshakeRedirectLoopCounter: number;
   // url derived from headers
   clerkUrl: URL;
   // cookie or header session token
@@ -106,6 +107,7 @@ class AuthenticateContext {
     // 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 {

packages/backend/src/tokens/handshake.ts

@@ -38,7 +38,8 @@ async function verifyHandshakeJwt(token: string, { key }: VerifyJwtOptions): Pro
 }
 
 /**
- * Similar to our verifyToken flow for Clerk-issued JWTs, but this verification flow is for our signed handshake payload. The handshake payload requires fewer verification steps.
+ * Similar to our verifyToken flow for Clerk-issued JWTs, but this verification flow is for our signed handshake payload.
+ * The handshake payload requires fewer verification steps.
  */
 export async function verifyHandshakeToken(
   token: string,

packages/backend/src/tokens/request.ts

@@ -173,9 +173,19 @@ ${error.getFullMessage()}`,
     if (isRequestEligibleForHandshake(authenticateContext)) {
       // Right now the only usage of passing in different headers is for multi-domain sync, which redirects somewhere else.
       // In the future if we want to decorate the handshake redirect with additional headers per call we need to tweak this logic.
-      return handshake(authenticateContext, reason, message, headers ?? buildRedirectToHandshake());
+      const handshakeHeaders = headers ?? buildRedirectToHandshake();
+      // Introduce the mechanism to protect for infinite handshake redirect loops
+      // using a cookie and returning true if it's infinite redirect loop or false if we can
+      // proceed with triggering handshake.
+      const isRedirectLoop = setHandshakeInfiniteRedirectionLoopHeaders(handshakeHeaders);
+      if (isRedirectLoop) {
+        const msg = `Clerk: Refreshing the session token resulted in an infinite redirect loop. This usually means that your Clerk instance keys do not match - make sure to copy the correct publishable and secret keys from the Clerk dashboard.`;
+        console.log(msg);
+        return signedOut(authenticateContext, reason, message);
+      }
+      return handshake(authenticateContext, reason, message, handshakeHeaders);
     }
-    return signedOut(authenticateContext, reason, message, new Headers());
+    return signedOut(authenticateContext, reason, message);
   }
 
   async function authenticateRequestWithTokenInHeader() {
@@ -193,6 +203,34 @@ ${error.getFullMessage()}`,
     }
   }
 
+  // We want to prevent infinite handshake redirection loops.
+  // We incrementally set a `__clerk_redirection_loop` cookie, and when it loops 3 times, we throw an error.
+  // We also utilize the `referer` header to skip the prefetch requests.
+  function setHandshakeInfiniteRedirectionLoopHeaders(headers: Headers): boolean {
+    if (authenticateContext.handshakeRedirectLoopCounter === 3) {
+      return true;
+    }
+
+    const newCounterValue = authenticateContext.handshakeRedirectLoopCounter + 1;
+    const cookieName = constants.Cookies.RedirectCount;
+    headers.append('Set-Cookie', `${cookieName}=${newCounterValue}; SameSite=Lax; HttpOnly; Max-Age=3`);
+    return false;
+  }
+
+  function handleHandshakeTokenVerificationErrorInDevelopment(error: TokenVerificationError) {
+    // In development, the handshake token is being transferred in the URL as a query parameter, so there is no
+    // possibility of collision with a handshake token of another app running on the same local domain
+    // (etc one app on localhost:3000 and one on localhost:3001).
+    // Therefore, if the handshake token is invalid, it is likely that the user has switched Clerk keys locally.
+    // We make sure to throw a descriptive error message and then stop the handshake flow in every case,
+    // to avoid the possibility of an infinite loop.
+    if (error.reason === TokenVerificationErrorReason.TokenInvalidSignature) {
+      const msg = `Clerk: Handshake token verification failed due to an invalid signature. If you have switched Clerk keys locally, clear your cookies and try again.`;
+      throw new Error(msg);
+    }
+    throw new Error(`Clerk: Handshake token verification failed: ${error.getFullMessage()}.`);
+  }
+
   async function authenticateRequestWithTokenInCookie() {
     const hasActiveClient = authenticateContext.clientUat;
     const hasSessionToken = !!authenticateContext.sessionTokenInCookie;
@@ -210,34 +248,22 @@ ${error.getFullMessage()}`,
       try {
         return await resolveHandshake();
       } catch (error) {
-        // If for some reason the handshake token is invalid or stale, we ignore it and continue trying to authenticate the request.
-        // Worst case, the handshake will trigger again and return a refreshed token.
-        if (error instanceof TokenVerificationError) {
-          if (authenticateContext.instanceType === 'development') {
-            if (error.reason === TokenVerificationErrorReason.TokenInvalidSignature) {
-              throw new Error(
-                `Clerk: Handshake token verification failed due to an invalid signature. If you have switched Clerk keys locally, clear your cookies and try again.`,
-              );
-            }
-
-            throw new Error(`Clerk: Handshake token verification failed: ${error.getFullMessage()}.`);
-          }
-
-          if (
-            error.reason === TokenVerificationErrorReason.TokenInvalidSignature ||
-            error.reason === TokenVerificationErrorReason.InvalidSecretKey
-          ) {
-            // Avoid infinite redirect loops due to incorrect secret-keys
-            return signedOut(
-              authenticateContext,
-              AuthErrorReason.UnexpectedError,
-              `Clerk: Handshake token verification failed with "${error.reason}"`,
-            );
-          }
+        // In production, the handshake token is being transferred as a cookie, so there is a possibility of collision
+        // with a handshake token of another app running on the same etld+1 domain.
+        // For example, if one app is running on sub1.clerk.com and another on sub2.clerk.com, the handshake token
+        // cookie for both apps will be set on etld+1 (clerk.com) so there's a possibility that one app will accidentally
+        // use the handshake token of a different app during the handshake flow.
+        // In this scenario, verification will fail with TokenInvalidSignature. In contrast to the development case,
+        // we need to allow the flow to continue so the app eventually retries another handshake with the correct token.
+        // We need to make sure, however, that we don't allow the flow to continue indefinitely, so we throw an error after X
+        // retries to avoid an infinite loop. An infinite loop can happen if the customer switched Clerk keys for their prod app.
+
+        // Check the handleHandshakeTokenVerificationErrorInDevelopment function for the development case.
+        if (error instanceof TokenVerificationError && authenticateContext.instanceType === 'development') {
+          handleHandshakeTokenVerificationErrorInDevelopment(error);
         }
       }
     }
-
     /**
      * Otherwise, check for "known unknown" auth states that we can resolve with a handshake.
      */

packages/fastify/src/__tests__/__snapshots__/constants.test.ts.snap

@@ -8,6 +8,7 @@ exports[`constants from environment variables 1`] = `
     "ClientUat": "__client_uat",
     "DevBrowser": "__clerk_db_jwt",
     "Handshake": "__clerk_handshake",
+    "RedirectCount": "__clerk_redirect_count",
     "Session": "__session",
   },
   "Headers": {