Skip to content
Open
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 number Diff line number Diff 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 number Diff line number Diff 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 Down Expand 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 number Diff line number Diff line change
Expand Up @@ -1185,7 +1185,7 @@
expect(requestState.toAuth()).toBeSignedInToAuth();
});

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

Check warning on line 1188 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 Down Expand Up @@ -1815,6 +1815,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 Down Expand Up @@ -2095,4 +2128,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 number Diff line number Diff 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 Down Expand 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 number Diff line number Diff 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 number Diff line number Diff 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';

/**
* Begin multi-domain sync flows
Expand Down Expand 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
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
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
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
Member Author

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