Skip to content

Conversation

@nikosdouvlis
Copy link
Member

@nikosdouvlis nikosdouvlis commented Jul 30, 2024

Description

We want the __clerk_handshake cookie to always be set on the ETLD+1 domain in order to support multiple apps running on the same domain, even if the apps are running on different same-level subdomains, for example, sub1.a.example.com and sub2.a.example.com

Of course, with multiple apps running, we risk collisions if more than one app needs to handshake at the same time (e.g., reopen a browser window with multiple tabs open).

We can deal with collisions in 3 different ways:

  1. Set the handshake cookie on the domain the app is running on. This however, will break for the scenario where two apps using the same PK/SK run on same-level subdomains. Imagine two apps, a.example.com and b.example.com, with the latter being an 'alias' of the former. Visiting b.example.com with an expired session token will result in a handshake redirect to a.example.com, the handshake cookie will be set on a.example.com, and the handshake will never finish because b.example.com won't be able to read it. Also, if a.example.com tries to set a cookie on b.example.com, the browser will reject it as the two do not domain-match.
  2. Suffix the cookies. The handshake cookie is rather big so we risk hitting header size limits with multiple suffixed handshake tokens on the etld+1
  3. Let it collide, but handle the collisions gracefully

This PR implements solution 3. We let the handshake cookies collide. In the extremely rare case of a collision, the app that accidentally reads a handshake cookie of a different instance will simply ignore it (signature verification will fail) and retry the handshake again if needed.

This PR also adds an infinite redirect loop protection mechanism to protect against the case where a customer accidentally added a PK/SK pair that does not match.

Checklist

  • npm test runs as expected.
  • npm run build runs as expected.
  • (If applicable) JSDoc comments have been added or updated for any package exports
  • (If applicable) Documentation has been updated

Type of change

  • 🐛 Bug fix
  • 🌟 New feature
  • 🔨 Breaking change
  • 📖 Refactoring / dependency upgrade / documentation
  • other:

@changeset-bot
Copy link

changeset-bot bot commented Jul 30, 2024

🦋 Changeset detected

Latest commit: 8a4aeec

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 9 packages
Name Type
@clerk/backend Patch
@clerk/astro Patch
@clerk/express Patch
@clerk/fastify Patch
@clerk/nextjs Patch
@clerk/remix Patch
@clerk/clerk-sdk-node Patch
@clerk/tanstack-start Patch
@clerk/testing Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@nikosdouvlis nikosdouvlis force-pushed the nikos/handshake-cookie-collision branch 2 times, most recently from 59186ef to acc9802 Compare July 31, 2024 18:04
@nikosdouvlis nikosdouvlis force-pushed the nikos/handshake-cookie-collision branch from acc9802 to 6b79914 Compare July 31, 2024 18:29
@nikosdouvlis nikosdouvlis reopened this Jul 31, 2024
@nikosdouvlis nikosdouvlis force-pushed the nikos/handshake-cookie-collision branch from 682c57c to b6b22c0 Compare August 1, 2024 09:10
@nikosdouvlis nikosdouvlis marked this pull request as ready for review August 1, 2024 09:24
@nikosdouvlis nikosdouvlis changed the title chore(repo): Wip chore(repo): Retry handshake in case of handshake cookie collision Aug 1, 2024
If a handshake loop occurs, it's going to be stopped by the infinite loop prevention mechanism and the request will terminate with a signed out state
@nikosdouvlis nikosdouvlis force-pushed the nikos/handshake-cookie-collision branch from b6b22c0 to 8a4aeec Compare August 1, 2024 09:38
@nikosdouvlis nikosdouvlis merged commit 992e596 into main Aug 1, 2024
@nikosdouvlis nikosdouvlis deleted the nikos/handshake-cookie-collision branch August 1, 2024 11:52
wobsoriano pushed a commit that referenced this pull request Aug 1, 2024
…3848)

Co-authored-by: Dimitris Klouvas <dimitris@clerk.dev>
@nikosdouvlis
Copy link
Member Author

!snapshot multiapp

@clerk-cookie
Copy link
Collaborator

Hey @nikosdouvlis - the snapshot version command generated the following package versions:

Package Version
@clerk/astro 1.0.5-multiapp.v8a4aeec
@clerk/backend 1.5.0-multiapp.v8a4aeec
@clerk/chrome-extension 1.1.8-multiapp.v8a4aeec
@clerk/clerk-js 5.10.3-multiapp.v8a4aeec
@clerk/elements 0.11.0-multiapp.v8a4aeec
@clerk/clerk-expo 2.0.1-multiapp.v8a4aeec
@clerk/express 0.0.22-multiapp.v8a4aeec
@clerk/fastify 1.0.24-multiapp.v8a4aeec
@clerk/localizations 2.5.3-multiapp.v8a4aeec
@clerk/nextjs 5.2.9-multiapp.v8a4aeec
@clerk/remix 4.2.8-multiapp.v8a4aeec
@clerk/clerk-sdk-node 5.0.21-multiapp.v8a4aeec
@clerk/tanstack-start 0.1.12-multiapp.v8a4aeec
@clerk/testing 1.2.4-multiapp.v8a4aeec

Tip: Use the snippet copy button below to quickly install the required packages.
@clerk/astro

npm i @clerk/astro@1.0.5-multiapp.v8a4aeec --save-exact

@clerk/backend

npm i @clerk/backend@1.5.0-multiapp.v8a4aeec --save-exact

@clerk/chrome-extension

npm i @clerk/chrome-extension@1.1.8-multiapp.v8a4aeec --save-exact

@clerk/clerk-js

npm i @clerk/clerk-js@5.10.3-multiapp.v8a4aeec --save-exact

@clerk/elements

npm i @clerk/elements@0.11.0-multiapp.v8a4aeec --save-exact

@clerk/clerk-expo

npm i @clerk/clerk-expo@2.0.1-multiapp.v8a4aeec --save-exact

@clerk/express

npm i @clerk/express@0.0.22-multiapp.v8a4aeec --save-exact

@clerk/fastify

npm i @clerk/fastify@1.0.24-multiapp.v8a4aeec --save-exact

@clerk/localizations

npm i @clerk/localizations@2.5.3-multiapp.v8a4aeec --save-exact

@clerk/nextjs

npm i @clerk/nextjs@5.2.9-multiapp.v8a4aeec --save-exact

@clerk/remix

npm i @clerk/remix@4.2.8-multiapp.v8a4aeec --save-exact

@clerk/clerk-sdk-node

npm i @clerk/clerk-sdk-node@5.0.21-multiapp.v8a4aeec --save-exact

@clerk/tanstack-start

npm i @clerk/tanstack-start@0.1.12-multiapp.v8a4aeec --save-exact

@clerk/testing

npm i @clerk/testing@1.2.4-multiapp.v8a4aeec --save-exact

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants