Skip to content
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

feat(nextjs,backend,integration): Introduce dynamic keys from clerkMiddleware #3525

Merged
merged 39 commits into from
Jul 1, 2024

Conversation

LauraBeatris
Copy link
Member

@LauraBeatris LauraBeatris commented Jun 5, 2024

Description

Resolves SDK-1253, SDK-1811, SDK-1770

This PR introduces dynamic keys from clerkMiddleware, solving the following problems:

  • auth() cannot assert the session token signature due to an undefined secret key, leading to a runtime error.
  • auth.createRedirect doesn't redirect to signInUrl or signUpUrl since those aren't propagated as well.

The propagation is done by encrypting an x-clerk-request-data header and sending it from the middleware runtime to the application runtime, where it's going to be decrypted using a key. The key used for encryption is resolved based on the following conditions:

  • If security-sensitive options, such as secretKey are provided, then CLERK_ENCRYPTION_KEY is required
  • Otherwise, fallbacks to CLERK_SECRET_KEY, which is going to have backward compatibility.

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:

@LauraBeatris LauraBeatris self-assigned this Jun 5, 2024
Copy link

changeset-bot bot commented Jun 5, 2024

🦋 Changeset detected

Latest commit: c458ab7

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

This PR includes changesets to release 7 packages
Name Type
@clerk/backend Minor
@clerk/nextjs Minor
@clerk/express Patch
@clerk/fastify Patch
@clerk/remix Patch
@clerk/clerk-sdk-node Patch
@clerk/tanstack-start 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

@LauraBeatris LauraBeatris changed the title feat(clerk-nextjs): Propagate clerkMiddleware options to application server feat(nextjs): Propagate clerkMiddleware options to application server Jun 5, 2024
@LauraBeatris LauraBeatris force-pushed the propagate-nextjs-middleware-options branch from 4dd4f25 to 31cd84c Compare June 5, 2024 20:59
@LauraBeatris LauraBeatris force-pushed the propagate-nextjs-middleware-options branch 4 times, most recently from ecf05b8 to daf8b1c Compare June 7, 2024 17:15
packages/nextjs/src/app-router/server/ClerkProvider.tsx Outdated Show resolved Hide resolved
packages/nextjs/src/app-router/server/auth.ts Outdated Show resolved Hide resolved
packages/nextjs/src/server/errors.ts Outdated Show resolved Hide resolved
packages/nextjs/src/server/utils.ts Show resolved Hide resolved
@nikosdouvlis
Copy link
Member

I know that this is still a draft PR, so don't worry about this comment yet. Once everything's is in place, let's also add an e2e test covering this flow. Feel free to ping anyone from team-sdk about this :)

packages/nextjs/src/app-router/server/ClerkProvider.tsx Outdated Show resolved Hide resolved
packages/nextjs/src/server/clerkMiddleware.ts Outdated Show resolved Hide resolved
packages/nextjs/src/server/utils.ts Outdated Show resolved Hide resolved
@LauraBeatris LauraBeatris force-pushed the propagate-nextjs-middleware-options branch 2 times, most recently from c3f1524 to 27ba688 Compare June 12, 2024 17:39
@LauraBeatris LauraBeatris force-pushed the propagate-nextjs-middleware-options branch 2 times, most recently from c782f11 to 4c4ee19 Compare June 12, 2024 19:40
@LauraBeatris LauraBeatris force-pushed the propagate-nextjs-middleware-options branch 3 times, most recently from 9ba87be to 99893b4 Compare June 12, 2024 19:45
@LauraBeatris LauraBeatris force-pushed the propagate-nextjs-middleware-options branch 2 times, most recently from fffc077 to 220cff9 Compare June 13, 2024 16:13
@LauraBeatris LauraBeatris force-pushed the propagate-nextjs-middleware-options branch from 20adf12 to a9c0509 Compare July 1, 2024 21:44
@LauraBeatris LauraBeatris force-pushed the propagate-nextjs-middleware-options branch from a9c0509 to c458ab7 Compare July 1, 2024 22:08
@LauraBeatris LauraBeatris merged commit f1847b7 into main Jul 1, 2024
15 checks passed
@LauraBeatris LauraBeatris deleted the propagate-nextjs-middleware-options branch July 1, 2024 22:23
@dimkl
Copy link
Contributor

dimkl commented Jul 2, 2024

❓ Why did we choose the approach of the clerkClient() Proxy instead of exposing a getClerkClient()?
Even though we have already introduced the Proxy in the @clerk/clerk-sdk-node to load dynamically the env variables, I think that we could avoid using it here, and introduce a more specific API to avoid any confusion over the clerkClient.

cc: @LauraBeatris , @nikosdouvlis , @BRKalow

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