Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions .changeset/post-handshake-405-fix.md
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
---
'@clerk/backend': patch
---

Fix POST requests with `sec-fetch-dest: document` incorrectly triggering handshake redirects, resulting in 405 errors from FAPI. Non-GET requests (e.g. native form submissions) are now excluded from handshake and multi-domain sync eligibility.
20 changes: 20 additions & 0 deletions packages/backend/src/tokens/__tests__/handshake.test.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -94,6 +94,7 @@ describe('HandshakeService', () => {
clerkUrl: new URL('https://example.com'),
frontendApi: 'api.clerk.com',
instanceType: 'production',
method: 'GET',
usesSuffixedCookies: () => true,
secFetchDest: 'document',
accept: 'text/html',
Expand DownExpand Up@@ -139,6 +140,25 @@ describe('HandshakeService', () => {
mockAuthenticateContext.accept = 'image/png';
expect(handshakeService.isRequestEligibleForHandshake()).toBe(false);
});

it('should return false for POST requests with document secFetchDest', () => {
mockAuthenticateContext.method = 'POST';
mockAuthenticateContext.secFetchDest = 'document';
expect(handshakeService.isRequestEligibleForHandshake()).toBe(false);
});

it('should return false for PUT requests with document secFetchDest', () => {
mockAuthenticateContext.method = 'PUT';
mockAuthenticateContext.secFetchDest = 'document';
expect(handshakeService.isRequestEligibleForHandshake()).toBe(false);
});

it('should return false for POST requests with text/html accept without secFetchDest', () => {
mockAuthenticateContext.method = 'POST';
mockAuthenticateContext.secFetchDest = undefined;
mockAuthenticateContext.accept = 'text/html';
expect(handshakeService.isRequestEligibleForHandshake()).toBe(false);
});
});

describe('buildRedirectToHandshake', () => {
Expand Down
91 changes: 91 additions & 0 deletions packages/backend/src/tokens/__tests__/request.test.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -1295,7 +1295,7 @@
expect(requestState.toAuth()).toBeSignedInToAuth();
});

describe('refreshToken', async () => {

Check warning on line 1298 in packages/backend/src/tokens/__tests__/request.test.ts

View workflow job for this annotation

GitHub Actions/ Static analysis

Async arrow function has no 'await' expression
test('returns signed in with valid refresh token cookie if token is expired and refresh token exists', async () => {
server.use(
http.get('https://api.clerk.test/v1/jwks', () => {
Expand DownExpand Up@@ -1925,6 +1925,39 @@
});
});

test('does not trigger handshake for cross-origin POST document request on primary domain', async () => {
const cookieStr = Object.entries({
__session: mockJwt,
__client_uat: '12345',
})
.map(([k, v]) => `${k}=${v}`)
.join(';');

const request = new Request('https://primary.com/dashboard', {
method: 'POST',
headers: {
...defaultHeaders,
referer: 'https://satellite.com/form',
'sec-fetch-dest': 'document',
cookie: cookieStr,
},
});

const requestState = await authenticateRequest(request, {
...mockOptions(),
publishableKey: PK_LIVE,
domain: 'primary.com',
isSatellite: false,
signInUrl: 'https://primary.com/sign-in',
});

expect(requestState).toBeSignedIn({
domain: 'primary.com',
isSatellite: false,
signInUrl: 'https://primary.com/sign-in',
});
});

test('does not trigger handshake for non-document requests', async () => {
const request = mockRequestWithCookies(
{
Expand DownExpand Up@@ -2205,4 +2238,62 @@
});
});
});

describe('POST requests with sec-fetch-dest: document', () => {
const mockPostRequest = (headers = {}, cookies = {}, requestUrl = 'http://clerk.com/path') => {
const cookieStr = Object.entries(cookies)
.map(([k, v]) => `${k}=${v}`)
.join(';');

return new Request(requestUrl, {
method: 'POST',
headers: { ...defaultHeaders, 'sec-fetch-dest': 'document', cookie: cookieStr, ...headers },
});
};

test('returns signed out instead of handshake when clientUat > 0 and no cookieToken', async () => {
const requestState = await authenticateRequest(
mockPostRequest({}, { __client_uat: '12345' }),
mockOptions({ secretKey: 'deadbeef', publishableKey: PK_LIVE }),
);

expect(requestState).toBeSignedOut({ reason: AuthErrorReason.ClientUATWithoutSessionToken });
});

test('returns signed out instead of handshake for satellite app needing sync', async () => {
const requestState = await authenticateRequest(
mockPostRequest({}, { __client_uat: '0' }),
mockOptions({
publishableKey: PK_LIVE,
secretKey: 'deadbeef',
isSatellite: true,
signInUrl: 'https://primary.dev/sign-in',
domain: 'satellite.dev',
}),
);

expect(requestState).toBeSignedOut({
reason: AuthErrorReason.SessionTokenAndUATMissing,
isSatellite: true,
signInUrl: 'https://primary.dev/sign-in',
domain: 'satellite.dev',
});
});

test('returns signed out instead of handshake when clientUat > cookieToken.iat', async () => {
const requestState = await authenticateRequest(
mockPostRequest(
{},
{
__clerk_db_jwt: 'deadbeef',
__client_uat: `${mockJwtPayload.iat + 10}`,
__session: mockJwt,
},
),
mockOptions(),
);

expect(requestState).toBeSignedOut({ reason: AuthErrorReason.SessionTokenIATBeforeClientUAT });
});
});
});
2 changes: 2 additions & 0 deletions packages/backend/src/tokens/authenticateContext.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -17,6 +17,7 @@ interface AuthenticateContext extends AuthenticateRequestOptions {
forwardedHost: string | undefined;
forwardedProto: string | undefined;
host: string | undefined;
method: string;
origin: string | undefined;
referrer: string | undefined;
secFetchDest: string | undefined;
Expand DownExpand Up@@ -281,6 +282,7 @@ class AuthenticateContext implements AuthenticateContext {
}

private initHeaderValues() {
this.method = this.clerkRequest.method;
this.tokenInHeader = this.parseAuthorizationHeader(this.getHeader(constants.Headers.Authorization));
this.origin = this.getHeader(constants.Headers.Origin);
this.host = this.getHeader(constants.Headers.Host);
Expand Down
9 changes: 8 additions & 1 deletion packages/backend/src/tokens/handshake.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -105,7 +105,14 @@ export class HandshakeService {
* @returns boolean indicating if the request is eligible for handshake
*/
isRequestEligibleForHandshake(): boolean {
const { accept, secFetchDest } = this.authenticateContext;
const { accept, method, secFetchDest } = this.authenticateContext;

// Handshake involves a redirect to FAPI which only accepts GET requests.
// Non-GET requests (e.g. POST form submissions) also set sec-fetch-dest: document,
// but redirecting them would result in a 405 Method Not Allowed from FAPI.
if (method !== 'GET') {
return false;
}

// 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.
// Also, we check for 'iframe' because it's the value set when a doc request is made by an iframe.
Expand Down
5 changes: 4 additions & 1 deletion packages/backend/src/tokens/request.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -475,7 +475,9 @@ export const authenticateRequest: AuthenticateRequest = (async (
}
}
const isRequestEligibleForMultiDomainSync =
authenticateContext.isSatellite && authenticateContext.secFetchDest === 'document';
authenticateContext.isSatellite &&
authenticateContext.secFetchDest === 'document' &&
authenticateContext.method === 'GET';
Comment thread
coderabbitai[bot] marked this conversation as resolved.

/**
* Begin multi-domain sync flows
Expand DownExpand Up@@ -650,6 +652,7 @@ export const authenticateRequest: AuthenticateRequest = (async (
// Check for cross-origin requests from satellite domains to primary domain
const shouldForceHandshakeForCrossDomain =

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not related to the PR, but this check looks off. It looks like it will trigger for any cross origin referrer, regardless of if satellites are involved or not, which does not seem like the intent from the original PR or the comments etc. I wonder if we could tighten that up somehow? 🤔

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@Ephem this is correct, we currently have no way to know if an app is using satellites here when on a primary (non-satellite) domain, so this is a best effort check.

!authenticateContext.isSatellite && // We're on primary
authenticateContext.method === 'GET' && // Only GET navigations (POST form submissions set sec-fetch-dest: document too)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I want to make sure I understand this correctly. My understanding is that in this scenario we should make a handshake, but we can't. According to #6238, we do the shouldForceHandshakeForCrossDomain because a satellite might have changed the auth state, so we need to handshake to make sure we have the latest state?

This check bypasses that for POSTs, essentially saying we don't need a handshake for POSTs, which means we might return a different auth state here than the satellite? I'd imagine having the correct auth state would be more important for POSTs, not less though?

Removing this check will make it hit the isRequestEligibleForHandshake instead which would fail, essentially saying we needed a handshake, but couldn't do it.

For the specific case we've discussed, the authenticateContext.method === 'GET' check would clearly be fine since satellites are not involved, see other comment, what I worry about is the general case.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Right. This is a case where a handshake would be appropriate if it was a GET navigation request, but it's a POST instead.

So yeah, this essentially makes the POST request get handled by the app server (regardless of auth state) instead of forcing a redirect to a handshake endpoint from the Clerk middleware.

authenticateContext.secFetchDest === 'document' && // Document navigation
authenticateContext.isCrossOriginReferrer() && // Came from different domain
!authenticateContext.isKnownClerkReferrer() && // Not from Clerk accounts portal or FAPI
Expand Down
Loading
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions .changeset/post-handshake-405-fix.md
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
---
'@clerk/backend': patch
---

Fix POST requests with `sec-fetch-dest: document` incorrectly triggering handshake redirects, resulting in 405 errors from FAPI. Non-GET requests (e.g. native form submissions) are now excluded from handshake and multi-domain sync eligibility.
20 changes: 20 additions & 0 deletions packages/backend/src/tokens/__tests__/handshake.test.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -94,6 +94,7 @@ describe('HandshakeService', () => {
clerkUrl: new URL('https://example.com'),
frontendApi: 'api.clerk.com',
instanceType: 'production',
method: 'GET',
usesSuffixedCookies: () => true,
secFetchDest: 'document',
accept: 'text/html',
Expand DownExpand Up@@ -139,6 +140,25 @@ describe('HandshakeService', () => {
mockAuthenticateContext.accept = 'image/png';
expect(handshakeService.isRequestEligibleForHandshake()).toBe(false);
});

it('should return false for POST requests with document secFetchDest', () => {
mockAuthenticateContext.method = 'POST';
mockAuthenticateContext.secFetchDest = 'document';
expect(handshakeService.isRequestEligibleForHandshake()).toBe(false);
});

it('should return false for PUT requests with document secFetchDest', () => {
mockAuthenticateContext.method = 'PUT';
mockAuthenticateContext.secFetchDest = 'document';
expect(handshakeService.isRequestEligibleForHandshake()).toBe(false);
});

it('should return false for POST requests with text/html accept without secFetchDest', () => {
mockAuthenticateContext.method = 'POST';
mockAuthenticateContext.secFetchDest = undefined;
mockAuthenticateContext.accept = 'text/html';
expect(handshakeService.isRequestEligibleForHandshake()).toBe(false);
});
});

describe('buildRedirectToHandshake', () => {
Expand Down
91 changes: 91 additions & 0 deletions packages/backend/src/tokens/__tests__/request.test.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -1295,7 +1295,7 @@
expect(requestState.toAuth()).toBeSignedInToAuth();
});

describe('refreshToken', async () => {

Check warning on line 1298 in packages/backend/src/tokens/__tests__/request.test.ts

View workflow job for this annotation

GitHub Actions/ Static analysis

Async arrow function has no 'await' expression
test('returns signed in with valid refresh token cookie if token is expired and refresh token exists', async () => {
server.use(
http.get('https://api.clerk.test/v1/jwks', () => {
Expand DownExpand Up@@ -1925,6 +1925,39 @@
});
});

test('does not trigger handshake for cross-origin POST document request on primary domain', async () => {
const cookieStr = Object.entries({
__session: mockJwt,
__client_uat: '12345',
})
.map(([k, v]) => `${k}=${v}`)
.join(';');

const request = new Request('https://primary.com/dashboard', {
method: 'POST',
headers: {
...defaultHeaders,
referer: 'https://satellite.com/form',
'sec-fetch-dest': 'document',
cookie: cookieStr,
},
});

const requestState = await authenticateRequest(request, {
...mockOptions(),
publishableKey: PK_LIVE,
domain: 'primary.com',
isSatellite: false,
signInUrl: 'https://primary.com/sign-in',
});

expect(requestState).toBeSignedIn({
domain: 'primary.com',
isSatellite: false,
signInUrl: 'https://primary.com/sign-in',
});
});

test('does not trigger handshake for non-document requests', async () => {
const request = mockRequestWithCookies(
{
Expand DownExpand Up@@ -2205,4 +2238,62 @@
});
});
});

describe('POST requests with sec-fetch-dest: document', () => {
const mockPostRequest = (headers = {}, cookies = {}, requestUrl = 'http://clerk.com/path') => {
const cookieStr = Object.entries(cookies)
.map(([k, v]) => `${k}=${v}`)
.join(';');

return new Request(requestUrl, {
method: 'POST',
headers: { ...defaultHeaders, 'sec-fetch-dest': 'document', cookie: cookieStr, ...headers },
});
};

test('returns signed out instead of handshake when clientUat > 0 and no cookieToken', async () => {
const requestState = await authenticateRequest(
mockPostRequest({}, { __client_uat: '12345' }),
mockOptions({ secretKey: 'deadbeef', publishableKey: PK_LIVE }),
);

expect(requestState).toBeSignedOut({ reason: AuthErrorReason.ClientUATWithoutSessionToken });
});

test('returns signed out instead of handshake for satellite app needing sync', async () => {
const requestState = await authenticateRequest(
mockPostRequest({}, { __client_uat: '0' }),
mockOptions({
publishableKey: PK_LIVE,
secretKey: 'deadbeef',
isSatellite: true,
signInUrl: 'https://primary.dev/sign-in',
domain: 'satellite.dev',
}),
);

expect(requestState).toBeSignedOut({
reason: AuthErrorReason.SessionTokenAndUATMissing,
isSatellite: true,
signInUrl: 'https://primary.dev/sign-in',
domain: 'satellite.dev',
});
});

test('returns signed out instead of handshake when clientUat > cookieToken.iat', async () => {
const requestState = await authenticateRequest(
mockPostRequest(
{},
{
__clerk_db_jwt: 'deadbeef',
__client_uat: `${mockJwtPayload.iat + 10}`,
__session: mockJwt,
},
),
mockOptions(),
);

expect(requestState).toBeSignedOut({ reason: AuthErrorReason.SessionTokenIATBeforeClientUAT });
});
});
});
2 changes: 2 additions & 0 deletions packages/backend/src/tokens/authenticateContext.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -17,6 +17,7 @@ interface AuthenticateContext extends AuthenticateRequestOptions {
forwardedHost: string | undefined;
forwardedProto: string | undefined;
host: string | undefined;
method: string;
origin: string | undefined;
referrer: string | undefined;
secFetchDest: string | undefined;
Expand DownExpand Up@@ -281,6 +282,7 @@ class AuthenticateContext implements AuthenticateContext {
}

private initHeaderValues() {
this.method = this.clerkRequest.method;
this.tokenInHeader = this.parseAuthorizationHeader(this.getHeader(constants.Headers.Authorization));
this.origin = this.getHeader(constants.Headers.Origin);
this.host = this.getHeader(constants.Headers.Host);
Expand Down
9 changes: 8 additions & 1 deletion packages/backend/src/tokens/handshake.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -105,7 +105,14 @@ export class HandshakeService {
* @returns boolean indicating if the request is eligible for handshake
*/
isRequestEligibleForHandshake(): boolean {
const { accept, secFetchDest } = this.authenticateContext;
const { accept, method, secFetchDest } = this.authenticateContext;

// Handshake involves a redirect to FAPI which only accepts GET requests.
// Non-GET requests (e.g. POST form submissions) also set sec-fetch-dest: document,
// but redirecting them would result in a 405 Method Not Allowed from FAPI.
if (method !== 'GET') {
return false;
}

// 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.
// Also, we check for 'iframe' because it's the value set when a doc request is made by an iframe.
Expand Down
5 changes: 4 additions & 1 deletion packages/backend/src/tokens/request.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -475,7 +475,9 @@ export const authenticateRequest: AuthenticateRequest = (async (
}
}
const isRequestEligibleForMultiDomainSync =
authenticateContext.isSatellite && authenticateContext.secFetchDest === 'document';
authenticateContext.isSatellite &&
authenticateContext.secFetchDest === 'document' &&
authenticateContext.method === 'GET';
Comment thread
coderabbitai[bot] marked this conversation as resolved.

/**
* Begin multi-domain sync flows
Expand DownExpand Up@@ -650,6 +652,7 @@ export const authenticateRequest: AuthenticateRequest = (async (
// Check for cross-origin requests from satellite domains to primary domain
const shouldForceHandshakeForCrossDomain =

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not related to the PR, but this check looks off. It looks like it will trigger for any cross origin referrer, regardless of if satellites are involved or not, which does not seem like the intent from the original PR or the comments etc. I wonder if we could tighten that up somehow? 🤔

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@Ephem this is correct, we currently have no way to know if an app is using satellites here when on a primary (non-satellite) domain, so this is a best effort check.

!authenticateContext.isSatellite && // We're on primary
authenticateContext.method === 'GET' && // Only GET navigations (POST form submissions set sec-fetch-dest: document too)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I want to make sure I understand this correctly. My understanding is that in this scenario we should make a handshake, but we can't. According to #6238, we do the shouldForceHandshakeForCrossDomain because a satellite might have changed the auth state, so we need to handshake to make sure we have the latest state?

This check bypasses that for POSTs, essentially saying we don't need a handshake for POSTs, which means we might return a different auth state here than the satellite? I'd imagine having the correct auth state would be more important for POSTs, not less though?

Removing this check will make it hit the isRequestEligibleForHandshake instead which would fail, essentially saying we needed a handshake, but couldn't do it.

For the specific case we've discussed, the authenticateContext.method === 'GET' check would clearly be fine since satellites are not involved, see other comment, what I worry about is the general case.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Right. This is a case where a handshake would be appropriate if it was a GET navigation request, but it's a POST instead.

So yeah, this essentially makes the POST request get handled by the app server (regardless of auth state) instead of forcing a redirect to a handshake endpoint from the Clerk middleware.

authenticateContext.secFetchDest === 'document' && // Document navigation
authenticateContext.isCrossOriginReferrer() && // Came from different domain
!authenticateContext.isKnownClerkReferrer() && // Not from Clerk accounts portal or FAPI
Expand Down
Loading
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions .changeset/post-handshake-405-fix.md
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
---
'@clerk/backend': patch
---

Fix POST requests with `sec-fetch-dest: document` incorrectly triggering handshake redirects, resulting in 405 errors from FAPI. Non-GET requests (e.g. native form submissions) are now excluded from handshake and multi-domain sync eligibility.
20 changes: 20 additions & 0 deletions packages/backend/src/tokens/__tests__/handshake.test.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -94,6 +94,7 @@ describe('HandshakeService', () => {
clerkUrl: new URL('https://example.com'),
frontendApi: 'api.clerk.com',
instanceType: 'production',
method: 'GET',
usesSuffixedCookies: () => true,
secFetchDest: 'document',
accept: 'text/html',
Expand DownExpand Up@@ -139,6 +140,25 @@ describe('HandshakeService', () => {
mockAuthenticateContext.accept = 'image/png';
expect(handshakeService.isRequestEligibleForHandshake()).toBe(false);
});

it('should return false for POST requests with document secFetchDest', () => {
mockAuthenticateContext.method = 'POST';
mockAuthenticateContext.secFetchDest = 'document';
expect(handshakeService.isRequestEligibleForHandshake()).toBe(false);
});

it('should return false for PUT requests with document secFetchDest', () => {
mockAuthenticateContext.method = 'PUT';
mockAuthenticateContext.secFetchDest = 'document';
expect(handshakeService.isRequestEligibleForHandshake()).toBe(false);
});

it('should return false for POST requests with text/html accept without secFetchDest', () => {
mockAuthenticateContext.method = 'POST';
mockAuthenticateContext.secFetchDest = undefined;
mockAuthenticateContext.accept = 'text/html';
expect(handshakeService.isRequestEligibleForHandshake()).toBe(false);
});
});

describe('buildRedirectToHandshake', () => {
Expand Down
91 changes: 91 additions & 0 deletions packages/backend/src/tokens/__tests__/request.test.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -1295,7 +1295,7 @@
expect(requestState.toAuth()).toBeSignedInToAuth();
});

describe('refreshToken', async () => {

Check warning on line 1298 in packages/backend/src/tokens/__tests__/request.test.ts

View workflow job for this annotation

GitHub Actions/ Static analysis

Async arrow function has no 'await' expression
test('returns signed in with valid refresh token cookie if token is expired and refresh token exists', async () => {
server.use(
http.get('https://api.clerk.test/v1/jwks', () => {
Expand DownExpand Up@@ -1925,6 +1925,39 @@
});
});

test('does not trigger handshake for cross-origin POST document request on primary domain', async () => {
const cookieStr = Object.entries({
__session: mockJwt,
__client_uat: '12345',
})
.map(([k, v]) => `${k}=${v}`)
.join(';');

const request = new Request('https://primary.com/dashboard', {
method: 'POST',
headers: {
...defaultHeaders,
referer: 'https://satellite.com/form',
'sec-fetch-dest': 'document',
cookie: cookieStr,
},
});

const requestState = await authenticateRequest(request, {
...mockOptions(),
publishableKey: PK_LIVE,
domain: 'primary.com',
isSatellite: false,
signInUrl: 'https://primary.com/sign-in',
});

expect(requestState).toBeSignedIn({
domain: 'primary.com',
isSatellite: false,
signInUrl: 'https://primary.com/sign-in',
});
});

test('does not trigger handshake for non-document requests', async () => {
const request = mockRequestWithCookies(
{
Expand DownExpand Up@@ -2205,4 +2238,62 @@
});
});
});

describe('POST requests with sec-fetch-dest: document', () => {
const mockPostRequest = (headers = {}, cookies = {}, requestUrl = 'http://clerk.com/path') => {
const cookieStr = Object.entries(cookies)
.map(([k, v]) => `${k}=${v}`)
.join(';');

return new Request(requestUrl, {
method: 'POST',
headers: { ...defaultHeaders, 'sec-fetch-dest': 'document', cookie: cookieStr, ...headers },
});
};

test('returns signed out instead of handshake when clientUat > 0 and no cookieToken', async () => {
const requestState = await authenticateRequest(
mockPostRequest({}, { __client_uat: '12345' }),
mockOptions({ secretKey: 'deadbeef', publishableKey: PK_LIVE }),
);

expect(requestState).toBeSignedOut({ reason: AuthErrorReason.ClientUATWithoutSessionToken });
});

test('returns signed out instead of handshake for satellite app needing sync', async () => {
const requestState = await authenticateRequest(
mockPostRequest({}, { __client_uat: '0' }),
mockOptions({
publishableKey: PK_LIVE,
secretKey: 'deadbeef',
isSatellite: true,
signInUrl: 'https://primary.dev/sign-in',
domain: 'satellite.dev',
}),
);

expect(requestState).toBeSignedOut({
reason: AuthErrorReason.SessionTokenAndUATMissing,
isSatellite: true,
signInUrl: 'https://primary.dev/sign-in',
domain: 'satellite.dev',
});
});

test('returns signed out instead of handshake when clientUat > cookieToken.iat', async () => {
const requestState = await authenticateRequest(
mockPostRequest(
{},
{
__clerk_db_jwt: 'deadbeef',
__client_uat: `${mockJwtPayload.iat + 10}`,
__session: mockJwt,
},
),
mockOptions(),
);

expect(requestState).toBeSignedOut({ reason: AuthErrorReason.SessionTokenIATBeforeClientUAT });
});
});
});
2 changes: 2 additions & 0 deletions packages/backend/src/tokens/authenticateContext.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -17,6 +17,7 @@ interface AuthenticateContext extends AuthenticateRequestOptions {
forwardedHost: string | undefined;
forwardedProto: string | undefined;
host: string | undefined;
method: string;
origin: string | undefined;
referrer: string | undefined;
secFetchDest: string | undefined;
Expand DownExpand Up@@ -281,6 +282,7 @@ class AuthenticateContext implements AuthenticateContext {
}

private initHeaderValues() {
this.method = this.clerkRequest.method;
this.tokenInHeader = this.parseAuthorizationHeader(this.getHeader(constants.Headers.Authorization));
this.origin = this.getHeader(constants.Headers.Origin);
this.host = this.getHeader(constants.Headers.Host);
Expand Down
9 changes: 8 additions & 1 deletion packages/backend/src/tokens/handshake.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -105,7 +105,14 @@ export class HandshakeService {
* @returns boolean indicating if the request is eligible for handshake
*/
isRequestEligibleForHandshake(): boolean {
const { accept, secFetchDest } = this.authenticateContext;
const { accept, method, secFetchDest } = this.authenticateContext;

// Handshake involves a redirect to FAPI which only accepts GET requests.
// Non-GET requests (e.g. POST form submissions) also set sec-fetch-dest: document,
// but redirecting them would result in a 405 Method Not Allowed from FAPI.
if (method !== 'GET') {
return false;
}

// 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.
// Also, we check for 'iframe' because it's the value set when a doc request is made by an iframe.
Expand Down
5 changes: 4 additions & 1 deletion packages/backend/src/tokens/request.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -475,7 +475,9 @@ export const authenticateRequest: AuthenticateRequest = (async (
}
}
const isRequestEligibleForMultiDomainSync =
authenticateContext.isSatellite && authenticateContext.secFetchDest === 'document';
authenticateContext.isSatellite &&
authenticateContext.secFetchDest === 'document' &&
authenticateContext.method === 'GET';
Comment thread
coderabbitai[bot] marked this conversation as resolved.

/**
* Begin multi-domain sync flows
Expand DownExpand Up@@ -650,6 +652,7 @@ export const authenticateRequest: AuthenticateRequest = (async (
// Check for cross-origin requests from satellite domains to primary domain
const shouldForceHandshakeForCrossDomain =

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not related to the PR, but this check looks off. It looks like it will trigger for any cross origin referrer, regardless of if satellites are involved or not, which does not seem like the intent from the original PR or the comments etc. I wonder if we could tighten that up somehow? 🤔

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@Ephem this is correct, we currently have no way to know if an app is using satellites here when on a primary (non-satellite) domain, so this is a best effort check.

!authenticateContext.isSatellite && // We're on primary
authenticateContext.method === 'GET' && // Only GET navigations (POST form submissions set sec-fetch-dest: document too)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I want to make sure I understand this correctly. My understanding is that in this scenario we should make a handshake, but we can't. According to #6238, we do the shouldForceHandshakeForCrossDomain because a satellite might have changed the auth state, so we need to handshake to make sure we have the latest state?

This check bypasses that for POSTs, essentially saying we don't need a handshake for POSTs, which means we might return a different auth state here than the satellite? I'd imagine having the correct auth state would be more important for POSTs, not less though?

Removing this check will make it hit the isRequestEligibleForHandshake instead which would fail, essentially saying we needed a handshake, but couldn't do it.

For the specific case we've discussed, the authenticateContext.method === 'GET' check would clearly be fine since satellites are not involved, see other comment, what I worry about is the general case.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Right. This is a case where a handshake would be appropriate if it was a GET navigation request, but it's a POST instead.

So yeah, this essentially makes the POST request get handled by the app server (regardless of auth state) instead of forcing a redirect to a handshake endpoint from the Clerk middleware.

authenticateContext.secFetchDest === 'document' && // Document navigation
authenticateContext.isCrossOriginReferrer() && // Came from different domain
!authenticateContext.isKnownClerkReferrer() && // Not from Clerk accounts portal or FAPI
Expand Down
Loading
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions .changeset/post-handshake-405-fix.md
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
---
'@clerk/backend': patch
---

Fix POST requests with `sec-fetch-dest: document` incorrectly triggering handshake redirects, resulting in 405 errors from FAPI. Non-GET requests (e.g. native form submissions) are now excluded from handshake and multi-domain sync eligibility.
20 changes: 20 additions & 0 deletions packages/backend/src/tokens/__tests__/handshake.test.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -94,6 +94,7 @@ describe('HandshakeService', () => {
clerkUrl: new URL('https://example.com'),
frontendApi: 'api.clerk.com',
instanceType: 'production',
method: 'GET',
usesSuffixedCookies: () => true,
secFetchDest: 'document',
accept: 'text/html',
Expand DownExpand Up@@ -139,6 +140,25 @@ describe('HandshakeService', () => {
mockAuthenticateContext.accept = 'image/png';
expect(handshakeService.isRequestEligibleForHandshake()).toBe(false);
});

it('should return false for POST requests with document secFetchDest', () => {
mockAuthenticateContext.method = 'POST';
mockAuthenticateContext.secFetchDest = 'document';
expect(handshakeService.isRequestEligibleForHandshake()).toBe(false);
});

it('should return false for PUT requests with document secFetchDest', () => {
mockAuthenticateContext.method = 'PUT';
mockAuthenticateContext.secFetchDest = 'document';
expect(handshakeService.isRequestEligibleForHandshake()).toBe(false);
});

it('should return false for POST requests with text/html accept without secFetchDest', () => {
mockAuthenticateContext.method = 'POST';
mockAuthenticateContext.secFetchDest = undefined;
mockAuthenticateContext.accept = 'text/html';
expect(handshakeService.isRequestEligibleForHandshake()).toBe(false);
});
});

describe('buildRedirectToHandshake', () => {
Expand Down
91 changes: 91 additions & 0 deletions packages/backend/src/tokens/__tests__/request.test.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -1295,7 +1295,7 @@
expect(requestState.toAuth()).toBeSignedInToAuth();
});

describe('refreshToken', async () => {

Check warning on line 1298 in packages/backend/src/tokens/__tests__/request.test.ts

View workflow job for this annotation

GitHub Actions/ Static analysis

Async arrow function has no 'await' expression
test('returns signed in with valid refresh token cookie if token is expired and refresh token exists', async () => {
server.use(
http.get('https://api.clerk.test/v1/jwks', () => {
Expand DownExpand Up@@ -1925,6 +1925,39 @@
});
});

test('does not trigger handshake for cross-origin POST document request on primary domain', async () => {
const cookieStr = Object.entries({
__session: mockJwt,
__client_uat: '12345',
})
.map(([k, v]) => `${k}=${v}`)
.join(';');

const request = new Request('https://primary.com/dashboard', {
method: 'POST',
headers: {
...defaultHeaders,
referer: 'https://satellite.com/form',
'sec-fetch-dest': 'document',
cookie: cookieStr,
},
});

const requestState = await authenticateRequest(request, {
...mockOptions(),
publishableKey: PK_LIVE,
domain: 'primary.com',
isSatellite: false,
signInUrl: 'https://primary.com/sign-in',
});

expect(requestState).toBeSignedIn({
domain: 'primary.com',
isSatellite: false,
signInUrl: 'https://primary.com/sign-in',
});
});

test('does not trigger handshake for non-document requests', async () => {
const request = mockRequestWithCookies(
{
Expand DownExpand Up@@ -2205,4 +2238,62 @@
});
});
});

describe('POST requests with sec-fetch-dest: document', () => {
const mockPostRequest = (headers = {}, cookies = {}, requestUrl = 'http://clerk.com/path') => {
const cookieStr = Object.entries(cookies)
.map(([k, v]) => `${k}=${v}`)
.join(';');

return new Request(requestUrl, {
method: 'POST',
headers: { ...defaultHeaders, 'sec-fetch-dest': 'document', cookie: cookieStr, ...headers },
});
};

test('returns signed out instead of handshake when clientUat > 0 and no cookieToken', async () => {
const requestState = await authenticateRequest(
mockPostRequest({}, { __client_uat: '12345' }),
mockOptions({ secretKey: 'deadbeef', publishableKey: PK_LIVE }),
);

expect(requestState).toBeSignedOut({ reason: AuthErrorReason.ClientUATWithoutSessionToken });
});

test('returns signed out instead of handshake for satellite app needing sync', async () => {
const requestState = await authenticateRequest(
mockPostRequest({}, { __client_uat: '0' }),
mockOptions({
publishableKey: PK_LIVE,
secretKey: 'deadbeef',
isSatellite: true,
signInUrl: 'https://primary.dev/sign-in',
domain: 'satellite.dev',
}),
);

expect(requestState).toBeSignedOut({
reason: AuthErrorReason.SessionTokenAndUATMissing,
isSatellite: true,
signInUrl: 'https://primary.dev/sign-in',
domain: 'satellite.dev',
});
});

test('returns signed out instead of handshake when clientUat > cookieToken.iat', async () => {
const requestState = await authenticateRequest(
mockPostRequest(
{},
{
__clerk_db_jwt: 'deadbeef',
__client_uat: `${mockJwtPayload.iat + 10}`,
__session: mockJwt,
},
),
mockOptions(),
);

expect(requestState).toBeSignedOut({ reason: AuthErrorReason.SessionTokenIATBeforeClientUAT });
});
});
});
2 changes: 2 additions & 0 deletions packages/backend/src/tokens/authenticateContext.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -17,6 +17,7 @@ interface AuthenticateContext extends AuthenticateRequestOptions {
forwardedHost: string | undefined;
forwardedProto: string | undefined;
host: string | undefined;
method: string;
origin: string | undefined;
referrer: string | undefined;
secFetchDest: string | undefined;
Expand DownExpand Up@@ -281,6 +282,7 @@ class AuthenticateContext implements AuthenticateContext {
}

private initHeaderValues() {
this.method = this.clerkRequest.method;
this.tokenInHeader = this.parseAuthorizationHeader(this.getHeader(constants.Headers.Authorization));
this.origin = this.getHeader(constants.Headers.Origin);
this.host = this.getHeader(constants.Headers.Host);
Expand Down
9 changes: 8 additions & 1 deletion packages/backend/src/tokens/handshake.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -105,7 +105,14 @@ export class HandshakeService {
* @returns boolean indicating if the request is eligible for handshake
*/
isRequestEligibleForHandshake(): boolean {
const { accept, secFetchDest } = this.authenticateContext;
const { accept, method, secFetchDest } = this.authenticateContext;

// Handshake involves a redirect to FAPI which only accepts GET requests.
// Non-GET requests (e.g. POST form submissions) also set sec-fetch-dest: document,
// but redirecting them would result in a 405 Method Not Allowed from FAPI.
if (method !== 'GET') {
return false;
}

// 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.
// Also, we check for 'iframe' because it's the value set when a doc request is made by an iframe.
Expand Down
5 changes: 4 additions & 1 deletion packages/backend/src/tokens/request.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -475,7 +475,9 @@ export const authenticateRequest: AuthenticateRequest = (async (
}
}
const isRequestEligibleForMultiDomainSync =
authenticateContext.isSatellite && authenticateContext.secFetchDest === 'document';
authenticateContext.isSatellite &&
authenticateContext.secFetchDest === 'document' &&
authenticateContext.method === 'GET';
Comment thread
coderabbitai[bot] marked this conversation as resolved.

/**
* Begin multi-domain sync flows
Expand DownExpand Up@@ -650,6 +652,7 @@ export const authenticateRequest: AuthenticateRequest = (async (
// Check for cross-origin requests from satellite domains to primary domain
const shouldForceHandshakeForCrossDomain =

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not related to the PR, but this check looks off. It looks like it will trigger for any cross origin referrer, regardless of if satellites are involved or not, which does not seem like the intent from the original PR or the comments etc. I wonder if we could tighten that up somehow? 🤔

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@Ephem this is correct, we currently have no way to know if an app is using satellites here when on a primary (non-satellite) domain, so this is a best effort check.

!authenticateContext.isSatellite && // We're on primary
authenticateContext.method === 'GET' && // Only GET navigations (POST form submissions set sec-fetch-dest: document too)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I want to make sure I understand this correctly. My understanding is that in this scenario we should make a handshake, but we can't. According to #6238, we do the shouldForceHandshakeForCrossDomain because a satellite might have changed the auth state, so we need to handshake to make sure we have the latest state?

This check bypasses that for POSTs, essentially saying we don't need a handshake for POSTs, which means we might return a different auth state here than the satellite? I'd imagine having the correct auth state would be more important for POSTs, not less though?

Removing this check will make it hit the isRequestEligibleForHandshake instead which would fail, essentially saying we needed a handshake, but couldn't do it.

For the specific case we've discussed, the authenticateContext.method === 'GET' check would clearly be fine since satellites are not involved, see other comment, what I worry about is the general case.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Right. This is a case where a handshake would be appropriate if it was a GET navigation request, but it's a POST instead.

So yeah, this essentially makes the POST request get handled by the app server (regardless of auth state) instead of forcing a redirect to a handshake endpoint from the Clerk middleware.

authenticateContext.secFetchDest === 'document' && // Document navigation
authenticateContext.isCrossOriginReferrer() && // Came from different domain
!authenticateContext.isKnownClerkReferrer() && // Not from Clerk accounts portal or FAPI
Expand Down
Loading
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions .changeset/post-handshake-405-fix.md
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
---
'@clerk/backend': patch
---

Fix POST requests with `sec-fetch-dest: document` incorrectly triggering handshake redirects, resulting in 405 errors from FAPI. Non-GET requests (e.g. native form submissions) are now excluded from handshake and multi-domain sync eligibility.
20 changes: 20 additions & 0 deletions packages/backend/src/tokens/__tests__/handshake.test.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -94,6 +94,7 @@ describe('HandshakeService', () => {
clerkUrl: new URL('https://example.com'),
frontendApi: 'api.clerk.com',
instanceType: 'production',
method: 'GET',
usesSuffixedCookies: () => true,
secFetchDest: 'document',
accept: 'text/html',
Expand DownExpand Up@@ -139,6 +140,25 @@ describe('HandshakeService', () => {
mockAuthenticateContext.accept = 'image/png';
expect(handshakeService.isRequestEligibleForHandshake()).toBe(false);
});

it('should return false for POST requests with document secFetchDest', () => {
mockAuthenticateContext.method = 'POST';
mockAuthenticateContext.secFetchDest = 'document';
expect(handshakeService.isRequestEligibleForHandshake()).toBe(false);
});

it('should return false for PUT requests with document secFetchDest', () => {
mockAuthenticateContext.method = 'PUT';
mockAuthenticateContext.secFetchDest = 'document';
expect(handshakeService.isRequestEligibleForHandshake()).toBe(false);
});

it('should return false for POST requests with text/html accept without secFetchDest', () => {
mockAuthenticateContext.method = 'POST';
mockAuthenticateContext.secFetchDest = undefined;
mockAuthenticateContext.accept = 'text/html';
expect(handshakeService.isRequestEligibleForHandshake()).toBe(false);
});
});

describe('buildRedirectToHandshake', () => {
Expand Down
91 changes: 91 additions & 0 deletions packages/backend/src/tokens/__tests__/request.test.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -1295,7 +1295,7 @@
expect(requestState.toAuth()).toBeSignedInToAuth();
});

describe('refreshToken', async () => {

Check warning on line 1298 in packages/backend/src/tokens/__tests__/request.test.ts

View workflow job for this annotation

GitHub Actions/ Static analysis

Async arrow function has no 'await' expression
test('returns signed in with valid refresh token cookie if token is expired and refresh token exists', async () => {
server.use(
http.get('https://api.clerk.test/v1/jwks', () => {
Expand DownExpand Up@@ -1925,6 +1925,39 @@
});
});

test('does not trigger handshake for cross-origin POST document request on primary domain', async () => {
const cookieStr = Object.entries({
__session: mockJwt,
__client_uat: '12345',
})
.map(([k, v]) => `${k}=${v}`)
.join(';');

const request = new Request('https://primary.com/dashboard', {
method: 'POST',
headers: {
...defaultHeaders,
referer: 'https://satellite.com/form',
'sec-fetch-dest': 'document',
cookie: cookieStr,
},
});

const requestState = await authenticateRequest(request, {
...mockOptions(),
publishableKey: PK_LIVE,
domain: 'primary.com',
isSatellite: false,
signInUrl: 'https://primary.com/sign-in',
});

expect(requestState).toBeSignedIn({
domain: 'primary.com',
isSatellite: false,
signInUrl: 'https://primary.com/sign-in',
});
});

test('does not trigger handshake for non-document requests', async () => {
const request = mockRequestWithCookies(
{
Expand DownExpand Up@@ -2205,4 +2238,62 @@
});
});
});

describe('POST requests with sec-fetch-dest: document', () => {
const mockPostRequest = (headers = {}, cookies = {}, requestUrl = 'http://clerk.com/path') => {
const cookieStr = Object.entries(cookies)
.map(([k, v]) => `${k}=${v}`)
.join(';');

return new Request(requestUrl, {
method: 'POST',
headers: { ...defaultHeaders, 'sec-fetch-dest': 'document', cookie: cookieStr, ...headers },
});
};

test('returns signed out instead of handshake when clientUat > 0 and no cookieToken', async () => {
const requestState = await authenticateRequest(
mockPostRequest({}, { __client_uat: '12345' }),
mockOptions({ secretKey: 'deadbeef', publishableKey: PK_LIVE }),
);

expect(requestState).toBeSignedOut({ reason: AuthErrorReason.ClientUATWithoutSessionToken });
});

test('returns signed out instead of handshake for satellite app needing sync', async () => {
const requestState = await authenticateRequest(
mockPostRequest({}, { __client_uat: '0' }),
mockOptions({
publishableKey: PK_LIVE,
secretKey: 'deadbeef',
isSatellite: true,
signInUrl: 'https://primary.dev/sign-in',
domain: 'satellite.dev',
}),
);

expect(requestState).toBeSignedOut({
reason: AuthErrorReason.SessionTokenAndUATMissing,
isSatellite: true,
signInUrl: 'https://primary.dev/sign-in',
domain: 'satellite.dev',
});
});

test('returns signed out instead of handshake when clientUat > cookieToken.iat', async () => {
const requestState = await authenticateRequest(
mockPostRequest(
{},
{
__clerk_db_jwt: 'deadbeef',
__client_uat: `${mockJwtPayload.iat + 10}`,
__session: mockJwt,
},
),
mockOptions(),
);

expect(requestState).toBeSignedOut({ reason: AuthErrorReason.SessionTokenIATBeforeClientUAT });
});
});
});
2 changes: 2 additions & 0 deletions packages/backend/src/tokens/authenticateContext.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -17,6 +17,7 @@ interface AuthenticateContext extends AuthenticateRequestOptions {
forwardedHost: string | undefined;
forwardedProto: string | undefined;
host: string | undefined;
method: string;
origin: string | undefined;
referrer: string | undefined;
secFetchDest: string | undefined;
Expand DownExpand Up@@ -281,6 +282,7 @@ class AuthenticateContext implements AuthenticateContext {
}

private initHeaderValues() {
this.method = this.clerkRequest.method;
this.tokenInHeader = this.parseAuthorizationHeader(this.getHeader(constants.Headers.Authorization));
this.origin = this.getHeader(constants.Headers.Origin);
this.host = this.getHeader(constants.Headers.Host);
Expand Down
9 changes: 8 additions & 1 deletion packages/backend/src/tokens/handshake.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -105,7 +105,14 @@ export class HandshakeService {
* @returns boolean indicating if the request is eligible for handshake
*/
isRequestEligibleForHandshake(): boolean {
const { accept, secFetchDest } = this.authenticateContext;
const { accept, method, secFetchDest } = this.authenticateContext;

// Handshake involves a redirect to FAPI which only accepts GET requests.
// Non-GET requests (e.g. POST form submissions) also set sec-fetch-dest: document,
// but redirecting them would result in a 405 Method Not Allowed from FAPI.
if (method !== 'GET') {
return false;
}

// 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.
// Also, we check for 'iframe' because it's the value set when a doc request is made by an iframe.
Expand Down
5 changes: 4 additions & 1 deletion packages/backend/src/tokens/request.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -475,7 +475,9 @@ export const authenticateRequest: AuthenticateRequest = (async (
}
}
const isRequestEligibleForMultiDomainSync =
authenticateContext.isSatellite && authenticateContext.secFetchDest === 'document';
authenticateContext.isSatellite &&
authenticateContext.secFetchDest === 'document' &&
authenticateContext.method === 'GET';
Comment thread
coderabbitai[bot] marked this conversation as resolved.

/**
* Begin multi-domain sync flows
Expand DownExpand Up@@ -650,6 +652,7 @@ export const authenticateRequest: AuthenticateRequest = (async (
// Check for cross-origin requests from satellite domains to primary domain
const shouldForceHandshakeForCrossDomain =

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not related to the PR, but this check looks off. It looks like it will trigger for any cross origin referrer, regardless of if satellites are involved or not, which does not seem like the intent from the original PR or the comments etc. I wonder if we could tighten that up somehow? 🤔

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@Ephem this is correct, we currently have no way to know if an app is using satellites here when on a primary (non-satellite) domain, so this is a best effort check.

!authenticateContext.isSatellite && // We're on primary
authenticateContext.method === 'GET' && // Only GET navigations (POST form submissions set sec-fetch-dest: document too)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I want to make sure I understand this correctly. My understanding is that in this scenario we should make a handshake, but we can't. According to #6238, we do the shouldForceHandshakeForCrossDomain because a satellite might have changed the auth state, so we need to handshake to make sure we have the latest state?

This check bypasses that for POSTs, essentially saying we don't need a handshake for POSTs, which means we might return a different auth state here than the satellite? I'd imagine having the correct auth state would be more important for POSTs, not less though?

Removing this check will make it hit the isRequestEligibleForHandshake instead which would fail, essentially saying we needed a handshake, but couldn't do it.

For the specific case we've discussed, the authenticateContext.method === 'GET' check would clearly be fine since satellites are not involved, see other comment, what I worry about is the general case.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Right. This is a case where a handshake would be appropriate if it was a GET navigation request, but it's a POST instead.

So yeah, this essentially makes the POST request get handled by the app server (regardless of auth state) instead of forcing a redirect to a handshake endpoint from the Clerk middleware.

authenticateContext.secFetchDest === 'document' && // Document navigation
authenticateContext.isCrossOriginReferrer() && // Came from different domain
!authenticateContext.isKnownClerkReferrer() && // Not from Clerk accounts portal or FAPI
Expand Down
Loading
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions .changeset/post-handshake-405-fix.md
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
---
'@clerk/backend': patch
---

Fix POST requests with `sec-fetch-dest: document` incorrectly triggering handshake redirects, resulting in 405 errors from FAPI. Non-GET requests (e.g. native form submissions) are now excluded from handshake and multi-domain sync eligibility.
20 changes: 20 additions & 0 deletions packages/backend/src/tokens/__tests__/handshake.test.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -94,6 +94,7 @@ describe('HandshakeService', () => {
clerkUrl: new URL('https://example.com'),
frontendApi: 'api.clerk.com',
instanceType: 'production',
method: 'GET',
usesSuffixedCookies: () => true,
secFetchDest: 'document',
accept: 'text/html',
Expand DownExpand Up@@ -139,6 +140,25 @@ describe('HandshakeService', () => {
mockAuthenticateContext.accept = 'image/png';
expect(handshakeService.isRequestEligibleForHandshake()).toBe(false);
});

it('should return false for POST requests with document secFetchDest', () => {
mockAuthenticateContext.method = 'POST';
mockAuthenticateContext.secFetchDest = 'document';
expect(handshakeService.isRequestEligibleForHandshake()).toBe(false);
});

it('should return false for PUT requests with document secFetchDest', () => {
mockAuthenticateContext.method = 'PUT';
mockAuthenticateContext.secFetchDest = 'document';
expect(handshakeService.isRequestEligibleForHandshake()).toBe(false);
});

it('should return false for POST requests with text/html accept without secFetchDest', () => {
mockAuthenticateContext.method = 'POST';
mockAuthenticateContext.secFetchDest = undefined;
mockAuthenticateContext.accept = 'text/html';
expect(handshakeService.isRequestEligibleForHandshake()).toBe(false);
});
});

describe('buildRedirectToHandshake', () => {
Expand Down
91 changes: 91 additions & 0 deletions packages/backend/src/tokens/__tests__/request.test.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -1295,7 +1295,7 @@
expect(requestState.toAuth()).toBeSignedInToAuth();
});

describe('refreshToken', async () => {

Check warning on line 1298 in packages/backend/src/tokens/__tests__/request.test.ts

View workflow job for this annotation

GitHub Actions/ Static analysis

Async arrow function has no 'await' expression
test('returns signed in with valid refresh token cookie if token is expired and refresh token exists', async () => {
server.use(
http.get('https://api.clerk.test/v1/jwks', () => {
Expand DownExpand Up@@ -1925,6 +1925,39 @@
});
});

test('does not trigger handshake for cross-origin POST document request on primary domain', async () => {
const cookieStr = Object.entries({
__session: mockJwt,
__client_uat: '12345',
})
.map(([k, v]) => `${k}=${v}`)
.join(';');

const request = new Request('https://primary.com/dashboard', {
method: 'POST',
headers: {
...defaultHeaders,
referer: 'https://satellite.com/form',
'sec-fetch-dest': 'document',
cookie: cookieStr,
},
});

const requestState = await authenticateRequest(request, {
...mockOptions(),
publishableKey: PK_LIVE,
domain: 'primary.com',
isSatellite: false,
signInUrl: 'https://primary.com/sign-in',
});

expect(requestState).toBeSignedIn({
domain: 'primary.com',
isSatellite: false,
signInUrl: 'https://primary.com/sign-in',
});
});

test('does not trigger handshake for non-document requests', async () => {
const request = mockRequestWithCookies(
{
Expand DownExpand Up@@ -2205,4 +2238,62 @@
});
});
});

describe('POST requests with sec-fetch-dest: document', () => {
const mockPostRequest = (headers = {}, cookies = {}, requestUrl = 'http://clerk.com/path') => {
const cookieStr = Object.entries(cookies)
.map(([k, v]) => `${k}=${v}`)
.join(';');

return new Request(requestUrl, {
method: 'POST',
headers: { ...defaultHeaders, 'sec-fetch-dest': 'document', cookie: cookieStr, ...headers },
});
};

test('returns signed out instead of handshake when clientUat > 0 and no cookieToken', async () => {
const requestState = await authenticateRequest(
mockPostRequest({}, { __client_uat: '12345' }),
mockOptions({ secretKey: 'deadbeef', publishableKey: PK_LIVE }),
);

expect(requestState).toBeSignedOut({ reason: AuthErrorReason.ClientUATWithoutSessionToken });
});

test('returns signed out instead of handshake for satellite app needing sync', async () => {
const requestState = await authenticateRequest(
mockPostRequest({}, { __client_uat: '0' }),
mockOptions({
publishableKey: PK_LIVE,
secretKey: 'deadbeef',
isSatellite: true,
signInUrl: 'https://primary.dev/sign-in',
domain: 'satellite.dev',
}),
);

expect(requestState).toBeSignedOut({
reason: AuthErrorReason.SessionTokenAndUATMissing,
isSatellite: true,
signInUrl: 'https://primary.dev/sign-in',
domain: 'satellite.dev',
});
});

test('returns signed out instead of handshake when clientUat > cookieToken.iat', async () => {
const requestState = await authenticateRequest(
mockPostRequest(
{},
{
__clerk_db_jwt: 'deadbeef',
__client_uat: `${mockJwtPayload.iat + 10}`,
__session: mockJwt,
},
),
mockOptions(),
);

expect(requestState).toBeSignedOut({ reason: AuthErrorReason.SessionTokenIATBeforeClientUAT });
});
});
});
2 changes: 2 additions & 0 deletions packages/backend/src/tokens/authenticateContext.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -17,6 +17,7 @@ interface AuthenticateContext extends AuthenticateRequestOptions {
forwardedHost: string | undefined;
forwardedProto: string | undefined;
host: string | undefined;
method: string;
origin: string | undefined;
referrer: string | undefined;
secFetchDest: string | undefined;
Expand DownExpand Up@@ -281,6 +282,7 @@ class AuthenticateContext implements AuthenticateContext {
}

private initHeaderValues() {
this.method = this.clerkRequest.method;
this.tokenInHeader = this.parseAuthorizationHeader(this.getHeader(constants.Headers.Authorization));
this.origin = this.getHeader(constants.Headers.Origin);
this.host = this.getHeader(constants.Headers.Host);
Expand Down
9 changes: 8 additions & 1 deletion packages/backend/src/tokens/handshake.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -105,7 +105,14 @@ export class HandshakeService {
* @returns boolean indicating if the request is eligible for handshake
*/
isRequestEligibleForHandshake(): boolean {
const { accept, secFetchDest } = this.authenticateContext;
const { accept, method, secFetchDest } = this.authenticateContext;

// Handshake involves a redirect to FAPI which only accepts GET requests.
// Non-GET requests (e.g. POST form submissions) also set sec-fetch-dest: document,
// but redirecting them would result in a 405 Method Not Allowed from FAPI.
if (method !== 'GET') {
return false;
}

// 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.
// Also, we check for 'iframe' because it's the value set when a doc request is made by an iframe.
Expand Down
5 changes: 4 additions & 1 deletion packages/backend/src/tokens/request.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -475,7 +475,9 @@ export const authenticateRequest: AuthenticateRequest = (async (
}
}
const isRequestEligibleForMultiDomainSync =
authenticateContext.isSatellite && authenticateContext.secFetchDest === 'document';
authenticateContext.isSatellite &&
authenticateContext.secFetchDest === 'document' &&
authenticateContext.method === 'GET';
Comment thread
coderabbitai[bot] marked this conversation as resolved.

/**
* Begin multi-domain sync flows
Expand DownExpand Up@@ -650,6 +652,7 @@ export const authenticateRequest: AuthenticateRequest = (async (
// Check for cross-origin requests from satellite domains to primary domain
const shouldForceHandshakeForCrossDomain =

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not related to the PR, but this check looks off. It looks like it will trigger for any cross origin referrer, regardless of if satellites are involved or not, which does not seem like the intent from the original PR or the comments etc. I wonder if we could tighten that up somehow? 🤔

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@Ephem this is correct, we currently have no way to know if an app is using satellites here when on a primary (non-satellite) domain, so this is a best effort check.

!authenticateContext.isSatellite && // We're on primary
authenticateContext.method === 'GET' && // Only GET navigations (POST form submissions set sec-fetch-dest: document too)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I want to make sure I understand this correctly. My understanding is that in this scenario we should make a handshake, but we can't. According to #6238, we do the shouldForceHandshakeForCrossDomain because a satellite might have changed the auth state, so we need to handshake to make sure we have the latest state?

This check bypasses that for POSTs, essentially saying we don't need a handshake for POSTs, which means we might return a different auth state here than the satellite? I'd imagine having the correct auth state would be more important for POSTs, not less though?

Removing this check will make it hit the isRequestEligibleForHandshake instead which would fail, essentially saying we needed a handshake, but couldn't do it.

For the specific case we've discussed, the authenticateContext.method === 'GET' check would clearly be fine since satellites are not involved, see other comment, what I worry about is the general case.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Right. This is a case where a handshake would be appropriate if it was a GET navigation request, but it's a POST instead.

So yeah, this essentially makes the POST request get handled by the app server (regardless of auth state) instead of forcing a redirect to a handshake endpoint from the Clerk middleware.

authenticateContext.secFetchDest === 'document' && // Document navigation
authenticateContext.isCrossOriginReferrer() && // Came from different domain
!authenticateContext.isKnownClerkReferrer() && // Not from Clerk accounts portal or FAPI
Expand Down
Loading
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions .changeset/post-handshake-405-fix.md
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
---
'@clerk/backend': patch
---

Fix POST requests with `sec-fetch-dest: document` incorrectly triggering handshake redirects, resulting in 405 errors from FAPI. Non-GET requests (e.g. native form submissions) are now excluded from handshake and multi-domain sync eligibility.
20 changes: 20 additions & 0 deletions packages/backend/src/tokens/__tests__/handshake.test.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -94,6 +94,7 @@ describe('HandshakeService', () => {
clerkUrl: new URL('https://example.com'),
frontendApi: 'api.clerk.com',
instanceType: 'production',
method: 'GET',
usesSuffixedCookies: () => true,
secFetchDest: 'document',
accept: 'text/html',
Expand DownExpand Up@@ -139,6 +140,25 @@ describe('HandshakeService', () => {
mockAuthenticateContext.accept = 'image/png';
expect(handshakeService.isRequestEligibleForHandshake()).toBe(false);
});

it('should return false for POST requests with document secFetchDest', () => {
mockAuthenticateContext.method = 'POST';
mockAuthenticateContext.secFetchDest = 'document';
expect(handshakeService.isRequestEligibleForHandshake()).toBe(false);
});

it('should return false for PUT requests with document secFetchDest', () => {
mockAuthenticateContext.method = 'PUT';
mockAuthenticateContext.secFetchDest = 'document';
expect(handshakeService.isRequestEligibleForHandshake()).toBe(false);
});

it('should return false for POST requests with text/html accept without secFetchDest', () => {
mockAuthenticateContext.method = 'POST';
mockAuthenticateContext.secFetchDest = undefined;
mockAuthenticateContext.accept = 'text/html';
expect(handshakeService.isRequestEligibleForHandshake()).toBe(false);
});
});

describe('buildRedirectToHandshake', () => {
Expand Down
91 changes: 91 additions & 0 deletions packages/backend/src/tokens/__tests__/request.test.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -1295,7 +1295,7 @@
expect(requestState.toAuth()).toBeSignedInToAuth();
});

describe('refreshToken', async () => {

Check warning on line 1298 in packages/backend/src/tokens/__tests__/request.test.ts

View workflow job for this annotation

GitHub Actions/ Static analysis

Async arrow function has no 'await' expression
test('returns signed in with valid refresh token cookie if token is expired and refresh token exists', async () => {
server.use(
http.get('https://api.clerk.test/v1/jwks', () => {
Expand DownExpand Up@@ -1925,6 +1925,39 @@
});
});

test('does not trigger handshake for cross-origin POST document request on primary domain', async () => {
const cookieStr = Object.entries({
__session: mockJwt,
__client_uat: '12345',
})
.map(([k, v]) => `${k}=${v}`)
.join(';');

const request = new Request('https://primary.com/dashboard', {
method: 'POST',
headers: {
...defaultHeaders,
referer: 'https://satellite.com/form',
'sec-fetch-dest': 'document',
cookie: cookieStr,
},
});

const requestState = await authenticateRequest(request, {
...mockOptions(),
publishableKey: PK_LIVE,
domain: 'primary.com',
isSatellite: false,
signInUrl: 'https://primary.com/sign-in',
});

expect(requestState).toBeSignedIn({
domain: 'primary.com',
isSatellite: false,
signInUrl: 'https://primary.com/sign-in',
});
});

test('does not trigger handshake for non-document requests', async () => {
const request = mockRequestWithCookies(
{
Expand DownExpand Up@@ -2205,4 +2238,62 @@
});
});
});

describe('POST requests with sec-fetch-dest: document', () => {
const mockPostRequest = (headers = {}, cookies = {}, requestUrl = 'http://clerk.com/path') => {
const cookieStr = Object.entries(cookies)
.map(([k, v]) => `${k}=${v}`)
.join(';');

return new Request(requestUrl, {
method: 'POST',
headers: { ...defaultHeaders, 'sec-fetch-dest': 'document', cookie: cookieStr, ...headers },
});
};

test('returns signed out instead of handshake when clientUat > 0 and no cookieToken', async () => {
const requestState = await authenticateRequest(
mockPostRequest({}, { __client_uat: '12345' }),
mockOptions({ secretKey: 'deadbeef', publishableKey: PK_LIVE }),
);

expect(requestState).toBeSignedOut({ reason: AuthErrorReason.ClientUATWithoutSessionToken });
});

test('returns signed out instead of handshake for satellite app needing sync', async () => {
const requestState = await authenticateRequest(
mockPostRequest({}, { __client_uat: '0' }),
mockOptions({
publishableKey: PK_LIVE,
secretKey: 'deadbeef',
isSatellite: true,
signInUrl: 'https://primary.dev/sign-in',
domain: 'satellite.dev',
}),
);

expect(requestState).toBeSignedOut({
reason: AuthErrorReason.SessionTokenAndUATMissing,
isSatellite: true,
signInUrl: 'https://primary.dev/sign-in',
domain: 'satellite.dev',
});
});

test('returns signed out instead of handshake when clientUat > cookieToken.iat', async () => {
const requestState = await authenticateRequest(
mockPostRequest(
{},
{
__clerk_db_jwt: 'deadbeef',
__client_uat: `${mockJwtPayload.iat + 10}`,
__session: mockJwt,
},
),
mockOptions(),
);

expect(requestState).toBeSignedOut({ reason: AuthErrorReason.SessionTokenIATBeforeClientUAT });
});
});
});
2 changes: 2 additions & 0 deletions packages/backend/src/tokens/authenticateContext.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -17,6 +17,7 @@ interface AuthenticateContext extends AuthenticateRequestOptions {
forwardedHost: string | undefined;
forwardedProto: string | undefined;
host: string | undefined;
method: string;
origin: string | undefined;
referrer: string | undefined;
secFetchDest: string | undefined;
Expand DownExpand Up@@ -281,6 +282,7 @@ class AuthenticateContext implements AuthenticateContext {
}

private initHeaderValues() {
this.method = this.clerkRequest.method;
this.tokenInHeader = this.parseAuthorizationHeader(this.getHeader(constants.Headers.Authorization));
this.origin = this.getHeader(constants.Headers.Origin);
this.host = this.getHeader(constants.Headers.Host);
Expand Down
9 changes: 8 additions & 1 deletion packages/backend/src/tokens/handshake.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -105,7 +105,14 @@ export class HandshakeService {
* @returns boolean indicating if the request is eligible for handshake
*/
isRequestEligibleForHandshake(): boolean {
const { accept, secFetchDest } = this.authenticateContext;
const { accept, method, secFetchDest } = this.authenticateContext;

// Handshake involves a redirect to FAPI which only accepts GET requests.
// Non-GET requests (e.g. POST form submissions) also set sec-fetch-dest: document,
// but redirecting them would result in a 405 Method Not Allowed from FAPI.
if (method !== 'GET') {
return false;
}

// 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.
// Also, we check for 'iframe' because it's the value set when a doc request is made by an iframe.
Expand Down
5 changes: 4 additions & 1 deletion packages/backend/src/tokens/request.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -475,7 +475,9 @@ export const authenticateRequest: AuthenticateRequest = (async (
}
}
const isRequestEligibleForMultiDomainSync =
authenticateContext.isSatellite && authenticateContext.secFetchDest === 'document';
authenticateContext.isSatellite &&
authenticateContext.secFetchDest === 'document' &&
authenticateContext.method === 'GET';
Comment thread
coderabbitai[bot] marked this conversation as resolved.

/**
* Begin multi-domain sync flows
Expand DownExpand Up@@ -650,6 +652,7 @@ export const authenticateRequest: AuthenticateRequest = (async (
// Check for cross-origin requests from satellite domains to primary domain
const shouldForceHandshakeForCrossDomain =

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not related to the PR, but this check looks off. It looks like it will trigger for any cross origin referrer, regardless of if satellites are involved or not, which does not seem like the intent from the original PR or the comments etc. I wonder if we could tighten that up somehow? 🤔

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@Ephem this is correct, we currently have no way to know if an app is using satellites here when on a primary (non-satellite) domain, so this is a best effort check.

!authenticateContext.isSatellite && // We're on primary
authenticateContext.method === 'GET' && // Only GET navigations (POST form submissions set sec-fetch-dest: document too)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I want to make sure I understand this correctly. My understanding is that in this scenario we should make a handshake, but we can't. According to #6238, we do the shouldForceHandshakeForCrossDomain because a satellite might have changed the auth state, so we need to handshake to make sure we have the latest state?

This check bypasses that for POSTs, essentially saying we don't need a handshake for POSTs, which means we might return a different auth state here than the satellite? I'd imagine having the correct auth state would be more important for POSTs, not less though?

Removing this check will make it hit the isRequestEligibleForHandshake instead which would fail, essentially saying we needed a handshake, but couldn't do it.

For the specific case we've discussed, the authenticateContext.method === 'GET' check would clearly be fine since satellites are not involved, see other comment, what I worry about is the general case.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Right. This is a case where a handshake would be appropriate if it was a GET navigation request, but it's a POST instead.

So yeah, this essentially makes the POST request get handled by the app server (regardless of auth state) instead of forcing a redirect to a handshake endpoint from the Clerk middleware.

authenticateContext.secFetchDest === 'document' && // Document navigation
authenticateContext.isCrossOriginReferrer() && // Came from different domain
!authenticateContext.isKnownClerkReferrer() && // Not from Clerk accounts portal or FAPI
Expand Down
Loading
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions .changeset/post-handshake-405-fix.md
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
---
'@clerk/backend': patch
---

Fix POST requests with `sec-fetch-dest: document` incorrectly triggering handshake redirects, resulting in 405 errors from FAPI. Non-GET requests (e.g. native form submissions) are now excluded from handshake and multi-domain sync eligibility.
20 changes: 20 additions & 0 deletions packages/backend/src/tokens/__tests__/handshake.test.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -94,6 +94,7 @@ describe('HandshakeService', () => {
clerkUrl: new URL('https://example.com'),
frontendApi: 'api.clerk.com',
instanceType: 'production',
method: 'GET',
usesSuffixedCookies: () => true,
secFetchDest: 'document',
accept: 'text/html',
Expand DownExpand Up@@ -139,6 +140,25 @@ describe('HandshakeService', () => {
mockAuthenticateContext.accept = 'image/png';
expect(handshakeService.isRequestEligibleForHandshake()).toBe(false);
});

it('should return false for POST requests with document secFetchDest', () => {
mockAuthenticateContext.method = 'POST';
mockAuthenticateContext.secFetchDest = 'document';
expect(handshakeService.isRequestEligibleForHandshake()).toBe(false);
});

it('should return false for PUT requests with document secFetchDest', () => {
mockAuthenticateContext.method = 'PUT';
mockAuthenticateContext.secFetchDest = 'document';
expect(handshakeService.isRequestEligibleForHandshake()).toBe(false);
});

it('should return false for POST requests with text/html accept without secFetchDest', () => {
mockAuthenticateContext.method = 'POST';
mockAuthenticateContext.secFetchDest = undefined;
mockAuthenticateContext.accept = 'text/html';
expect(handshakeService.isRequestEligibleForHandshake()).toBe(false);
});
});

describe('buildRedirectToHandshake', () => {
Expand Down
91 changes: 91 additions & 0 deletions packages/backend/src/tokens/__tests__/request.test.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -1295,7 +1295,7 @@
expect(requestState.toAuth()).toBeSignedInToAuth();
});

describe('refreshToken', async () => {

Check warning on line 1298 in packages/backend/src/tokens/__tests__/request.test.ts

View workflow job for this annotation

GitHub Actions/ Static analysis

Async arrow function has no 'await' expression
test('returns signed in with valid refresh token cookie if token is expired and refresh token exists', async () => {
server.use(
http.get('https://api.clerk.test/v1/jwks', () => {
Expand DownExpand Up@@ -1925,6 +1925,39 @@
});
});

test('does not trigger handshake for cross-origin POST document request on primary domain', async () => {
const cookieStr = Object.entries({
__session: mockJwt,
__client_uat: '12345',
})
.map(([k, v]) => `${k}=${v}`)
.join(';');

const request = new Request('https://primary.com/dashboard', {
method: 'POST',
headers: {
...defaultHeaders,
referer: 'https://satellite.com/form',
'sec-fetch-dest': 'document',
cookie: cookieStr,
},
});

const requestState = await authenticateRequest(request, {
...mockOptions(),
publishableKey: PK_LIVE,
domain: 'primary.com',
isSatellite: false,
signInUrl: 'https://primary.com/sign-in',
});

expect(requestState).toBeSignedIn({
domain: 'primary.com',
isSatellite: false,
signInUrl: 'https://primary.com/sign-in',
});
});

test('does not trigger handshake for non-document requests', async () => {
const request = mockRequestWithCookies(
{
Expand DownExpand Up@@ -2205,4 +2238,62 @@
});
});
});

describe('POST requests with sec-fetch-dest: document', () => {
const mockPostRequest = (headers = {}, cookies = {}, requestUrl = 'http://clerk.com/path') => {
const cookieStr = Object.entries(cookies)
.map(([k, v]) => `${k}=${v}`)
.join(';');

return new Request(requestUrl, {
method: 'POST',
headers: { ...defaultHeaders, 'sec-fetch-dest': 'document', cookie: cookieStr, ...headers },
});
};

test('returns signed out instead of handshake when clientUat > 0 and no cookieToken', async () => {
const requestState = await authenticateRequest(
mockPostRequest({}, { __client_uat: '12345' }),
mockOptions({ secretKey: 'deadbeef', publishableKey: PK_LIVE }),
);

expect(requestState).toBeSignedOut({ reason: AuthErrorReason.ClientUATWithoutSessionToken });
});

test('returns signed out instead of handshake for satellite app needing sync', async () => {
const requestState = await authenticateRequest(
mockPostRequest({}, { __client_uat: '0' }),
mockOptions({
publishableKey: PK_LIVE,
secretKey: 'deadbeef',
isSatellite: true,
signInUrl: 'https://primary.dev/sign-in',
domain: 'satellite.dev',
}),
);

expect(requestState).toBeSignedOut({
reason: AuthErrorReason.SessionTokenAndUATMissing,
isSatellite: true,
signInUrl: 'https://primary.dev/sign-in',
domain: 'satellite.dev',
});
});

test('returns signed out instead of handshake when clientUat > cookieToken.iat', async () => {
const requestState = await authenticateRequest(
mockPostRequest(
{},
{
__clerk_db_jwt: 'deadbeef',
__client_uat: `${mockJwtPayload.iat + 10}`,
__session: mockJwt,
},
),
mockOptions(),
);

expect(requestState).toBeSignedOut({ reason: AuthErrorReason.SessionTokenIATBeforeClientUAT });
});
});
});
2 changes: 2 additions & 0 deletions packages/backend/src/tokens/authenticateContext.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -17,6 +17,7 @@ interface AuthenticateContext extends AuthenticateRequestOptions {
forwardedHost: string | undefined;
forwardedProto: string | undefined;
host: string | undefined;
method: string;
origin: string | undefined;
referrer: string | undefined;
secFetchDest: string | undefined;
Expand DownExpand Up@@ -281,6 +282,7 @@ class AuthenticateContext implements AuthenticateContext {
}

private initHeaderValues() {
this.method = this.clerkRequest.method;
this.tokenInHeader = this.parseAuthorizationHeader(this.getHeader(constants.Headers.Authorization));
this.origin = this.getHeader(constants.Headers.Origin);
this.host = this.getHeader(constants.Headers.Host);
Expand Down
9 changes: 8 additions & 1 deletion packages/backend/src/tokens/handshake.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -105,7 +105,14 @@ export class HandshakeService {
* @returns boolean indicating if the request is eligible for handshake
*/
isRequestEligibleForHandshake(): boolean {
const { accept, secFetchDest } = this.authenticateContext;
const { accept, method, secFetchDest } = this.authenticateContext;

// Handshake involves a redirect to FAPI which only accepts GET requests.
// Non-GET requests (e.g. POST form submissions) also set sec-fetch-dest: document,
// but redirecting them would result in a 405 Method Not Allowed from FAPI.
if (method !== 'GET') {
return false;
}

// 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.
// Also, we check for 'iframe' because it's the value set when a doc request is made by an iframe.
Expand Down
5 changes: 4 additions & 1 deletion packages/backend/src/tokens/request.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -475,7 +475,9 @@ export const authenticateRequest: AuthenticateRequest = (async (
}
}
const isRequestEligibleForMultiDomainSync =
authenticateContext.isSatellite && authenticateContext.secFetchDest === 'document';
authenticateContext.isSatellite &&
authenticateContext.secFetchDest === 'document' &&
authenticateContext.method === 'GET';
Comment thread
coderabbitai[bot] marked this conversation as resolved.

/**
* Begin multi-domain sync flows
Expand DownExpand Up@@ -650,6 +652,7 @@ export const authenticateRequest: AuthenticateRequest = (async (
// Check for cross-origin requests from satellite domains to primary domain
const shouldForceHandshakeForCrossDomain =

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not related to the PR, but this check looks off. It looks like it will trigger for any cross origin referrer, regardless of if satellites are involved or not, which does not seem like the intent from the original PR or the comments etc. I wonder if we could tighten that up somehow? 🤔

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@Ephem this is correct, we currently have no way to know if an app is using satellites here when on a primary (non-satellite) domain, so this is a best effort check.

!authenticateContext.isSatellite && // We're on primary
authenticateContext.method === 'GET' && // Only GET navigations (POST form submissions set sec-fetch-dest: document too)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I want to make sure I understand this correctly. My understanding is that in this scenario we should make a handshake, but we can't. According to #6238, we do the shouldForceHandshakeForCrossDomain because a satellite might have changed the auth state, so we need to handshake to make sure we have the latest state?

This check bypasses that for POSTs, essentially saying we don't need a handshake for POSTs, which means we might return a different auth state here than the satellite? I'd imagine having the correct auth state would be more important for POSTs, not less though?

Removing this check will make it hit the isRequestEligibleForHandshake instead which would fail, essentially saying we needed a handshake, but couldn't do it.

For the specific case we've discussed, the authenticateContext.method === 'GET' check would clearly be fine since satellites are not involved, see other comment, what I worry about is the general case.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Right. This is a case where a handshake would be appropriate if it was a GET navigation request, but it's a POST instead.

So yeah, this essentially makes the POST request get handled by the app server (regardless of auth state) instead of forcing a redirect to a handshake endpoint from the Clerk middleware.

authenticateContext.secFetchDest === 'document' && // Document navigation
authenticateContext.isCrossOriginReferrer() && // Came from different domain
!authenticateContext.isKnownClerkReferrer() && // Not from Clerk accounts portal or FAPI
Expand Down
Loading