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

fix(nextjs,backend): Support handshake in iframes #3555

Merged
merged 11 commits into from
Jul 11, 2024

Conversation

anagstef
Copy link
Member

@anagstef anagstef commented Jun 12, 2024

Description

Adds support for handshake inside iframes.

This PR makes requests that have the Sec-Fetch-Dest value set to iframe to be eligible for handshake.

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:

@anagstef anagstef self-assigned this Jun 12, 2024
Copy link

changeset-bot bot commented Jun 12, 2024

🦋 Changeset detected

Latest commit: 203414f

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/nextjs Patch
@clerk/astro Patch
@clerk/express Patch
@clerk/fastify 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

@anagstef
Copy link
Member Author

!snapshot

@clerk-cookie
Copy link
Collaborator

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

Package Version
@clerk/backend 1.3.0-snapshot.v1ec5424
@clerk/chrome-extension 1.0.18-snapshot.v1ec5424
@clerk/clerk-js 5.7.0-snapshot.v1ec5424
@clerk/elements 0.7.0-snapshot.v1ec5424
@clerk/clerk-expo 1.2.1-snapshot.v1ec5424
@clerk/express 0.0.12-snapshot.v1ec5424
@clerk/fastify 1.0.14-snapshot.v1ec5424
gatsby-plugin-clerk 5.0.0-beta.45
@clerk/nextjs 5.2.0-snapshot.v1ec5424
@clerk/clerk-react 5.2.4-snapshot.v1ec5424
@clerk/remix 4.1.1-snapshot.v1ec5424
@clerk/clerk-sdk-node 5.0.11-snapshot.v1ec5424
@clerk/shared 2.3.0-snapshot.v1ec5424
@clerk/tanstack-start 0.1.1-snapshot.v1ec5424
@clerk/testing 1.1.7-snapshot.v1ec5424
@clerk/ui 0.1.1-snapshot.v1ec5424

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

npm i @clerk/[email protected] --save-exact

@clerk/chrome-extension

npm i @clerk/[email protected] --save-exact

@clerk/clerk-js

npm i @clerk/[email protected] --save-exact

@clerk/elements

npm i @clerk/[email protected] --save-exact

@clerk/clerk-expo

npm i @clerk/[email protected] --save-exact

@clerk/express

npm i @clerk/[email protected] --save-exact

@clerk/fastify

npm i @clerk/[email protected] --save-exact

gatsby-plugin-clerk

npm i [email protected] --save-exact

@clerk/nextjs

npm i @clerk/[email protected] --save-exact

@clerk/clerk-react

npm i @clerk/[email protected] --save-exact

@clerk/remix

npm i @clerk/[email protected] --save-exact

@clerk/clerk-sdk-node

npm i @clerk/[email protected] --save-exact

@clerk/shared

npm i @clerk/[email protected] --save-exact

@clerk/tanstack-start

npm i @clerk/[email protected] --save-exact

@clerk/testing

npm i @clerk/[email protected] --save-exact

@clerk/ui

npm i @clerk/[email protected] --save-exact

@@ -48,7 +48,7 @@ function isRequestEligibleForHandshake(authenticateContext: { secFetchDest?: str
const { accept, secFetchDest } = authenticateContext;

// NOTE: we could also check sec-fetch-mode === navigate here, but according to the spec, sec-fetch-dest: document should indicate that the request is the data of a user navigation.
if (secFetchDest === 'document') {
if (secFetchDest === 'document' || secFetchDest === 'iframe') {
Copy link
Member Author

Choose a reason for hiding this comment

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

ℹ️ When document requests happend from inside an iframe the Sec-Fetch-Dest value is set to iframe. Fetch/XHR requests do not use this value.

Copy link
Member

Choose a reason for hiding this comment

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

I propose exporting a SAFE_SEC_FETCH_DESTS array from shared and then consuming it in backend and nextjs, and just checking SAFE_SEC_FETCH_DESTS.includes(secFetchDest)

@anagstef anagstef marked this pull request as ready for review June 13, 2024 17:28
@@ -48,7 +48,7 @@ function isRequestEligibleForHandshake(authenticateContext: { secFetchDest?: str
const { accept, secFetchDest } = authenticateContext;

// NOTE: we could also check sec-fetch-mode === navigate here, but according to the spec, sec-fetch-dest: document should indicate that the request is the data of a user navigation.
if (secFetchDest === 'document') {
if (secFetchDest === 'document' || secFetchDest === 'iframe') {
Copy link
Member

Choose a reason for hiding this comment

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

I propose exporting a SAFE_SEC_FETCH_DESTS array from shared and then consuming it in backend and nextjs, and just checking SAFE_SEC_FETCH_DESTS.includes(secFetchDest)

.changeset/poor-knives-laugh.md Outdated Show resolved Hide resolved
@@ -121,6 +121,7 @@ const isServerActionRequest = (req: Request) => {
const isPageRequest = (req: Request): boolean => {
return (
req.headers.get(constants.Headers.SecFetchDest) === 'document' ||
req.headers.get(constants.Headers.SecFetchDest) === 'iframe' ||
Copy link
Member

Choose a reason for hiding this comment

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

❓ why do we need this as part of this PR? Is this related to the handshake mechanism?

Copy link
Member

Choose a reason for hiding this comment

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

I believe we need this in order for pages wrapped with protect work properly in the first load via an iframe

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not a handshake particular thing, but as @panteliselef said, it will help with protect and iframes.

@anagstef
Copy link
Member Author

!snapshot

@anagstef
Copy link
Member Author

!snapshot

@clerk-cookie
Copy link
Collaborator

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

Package Version
@clerk/astro 0.0.2-snapshot.v27f3f6b
@clerk/backend 1.3.1-snapshot.v27f3f6b
@clerk/chrome-extension 1.1.3-snapshot.v27f3f6b
@clerk/clerk-js 5.8.1-snapshot.v27f3f6b
@clerk/clerk-expo 1.2.6-snapshot.v27f3f6b
@clerk/express 0.0.16-snapshot.v27f3f6b
@clerk/fastify 1.0.18-snapshot.v27f3f6b
gatsby-plugin-clerk 5.0.0-beta.45
@clerk/nextjs 5.2.3-snapshot.v27f3f6b
@clerk/remix 4.2.2-snapshot.v27f3f6b
@clerk/clerk-sdk-node 5.0.15-snapshot.v27f3f6b
@clerk/tanstack-start 0.1.6-snapshot.v27f3f6b
@clerk/testing 1.1.10-snapshot.v27f3f6b

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

npm i @clerk/[email protected] --save-exact

@clerk/backend

npm i @clerk/[email protected] --save-exact

@clerk/chrome-extension

npm i @clerk/[email protected] --save-exact

@clerk/clerk-js

npm i @clerk/[email protected] --save-exact

@clerk/clerk-expo

npm i @clerk/[email protected] --save-exact

@clerk/express

npm i @clerk/[email protected] --save-exact

@clerk/fastify

npm i @clerk/[email protected] --save-exact

gatsby-plugin-clerk

npm i [email protected] --save-exact

@clerk/nextjs

npm i @clerk/[email protected] --save-exact

@clerk/remix

npm i @clerk/[email protected] --save-exact

@clerk/clerk-sdk-node

npm i @clerk/[email protected] --save-exact

@clerk/tanstack-start

npm i @clerk/[email protected] --save-exact

@clerk/testing

npm i @clerk/[email protected] --save-exact

@anagstef
Copy link
Member Author

!snapshot

@clerk-cookie
Copy link
Collaborator

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

Package Version
@clerk/astro 0.0.2-snapshot.v88c02de
@clerk/backend 1.3.1-snapshot.v88c02de
@clerk/chrome-extension 1.1.3-snapshot.v88c02de
@clerk/clerk-js 5.8.1-snapshot.v88c02de
@clerk/clerk-expo 1.2.6-snapshot.v88c02de
@clerk/express 0.0.16-snapshot.v88c02de
@clerk/fastify 1.0.18-snapshot.v88c02de
gatsby-plugin-clerk 5.0.0-beta.45
@clerk/nextjs 5.2.3-snapshot.v88c02de
@clerk/remix 4.2.2-snapshot.v88c02de
@clerk/clerk-sdk-node 5.0.15-snapshot.v88c02de
@clerk/tanstack-start 0.1.6-snapshot.v88c02de
@clerk/testing 1.1.10-snapshot.v88c02de

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

npm i @clerk/[email protected] --save-exact

@clerk/backend

npm i @clerk/[email protected] --save-exact

@clerk/chrome-extension

npm i @clerk/[email protected] --save-exact

@clerk/clerk-js

npm i @clerk/[email protected] --save-exact

@clerk/clerk-expo

npm i @clerk/[email protected] --save-exact

@clerk/express

npm i @clerk/[email protected] --save-exact

@clerk/fastify

npm i @clerk/[email protected] --save-exact

gatsby-plugin-clerk

npm i [email protected] --save-exact

@clerk/nextjs

npm i @clerk/[email protected] --save-exact

@clerk/remix

npm i @clerk/[email protected] --save-exact

@clerk/clerk-sdk-node

npm i @clerk/[email protected] --save-exact

@clerk/tanstack-start

npm i @clerk/[email protected] --save-exact

@clerk/testing

npm i @clerk/[email protected] --save-exact

@anagstef anagstef enabled auto-merge (squash) July 11, 2024 16:16
@anagstef anagstef merged commit 5642b26 into main Jul 11, 2024
16 checks passed
@anagstef anagstef deleted the stefanos/fix-iframe-handshake branch July 11, 2024 16:28
wobsoriano pushed a commit that referenced this pull request Jul 11, 2024
This change makes requests that have the `Sec-Fetch-Dest` value set to `iframe` to be eligible for handshake.

Co-authored-by: Bryce Kalow <[email protected]>
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.

None yet

7 participants