Skip to content

Commit bedad42

Browse files
authored
fix(backend): Exclude non-GET requests from handshake eligibility (#8045)
1 parent dd73e0c commit bedad42

File tree

6 files changed

+130
-2
lines changed

6 files changed

+130
-2
lines changed
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@clerk/backend': patch
3+
---
4+
5+
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.

packages/backend/src/tokens/__tests__/handshake.test.ts

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,7 @@ describe('HandshakeService', () => {
9494
clerkUrl: new URL('https://example.com'),
9595
frontendApi: 'api.clerk.com',
9696
instanceType: 'production',
97+
method: 'GET',
9798
usesSuffixedCookies: () => true,
9899
secFetchDest: 'document',
99100
accept: 'text/html',
@@ -139,6 +140,25 @@ describe('HandshakeService', () => {
139140
mockAuthenticateContext.accept = 'image/png';
140141
expect(handshakeService.isRequestEligibleForHandshake()).toBe(false);
141142
});
143+
144+
it('should return false for POST requests with document secFetchDest', () => {
145+
mockAuthenticateContext.method = 'POST';
146+
mockAuthenticateContext.secFetchDest = 'document';
147+
expect(handshakeService.isRequestEligibleForHandshake()).toBe(false);
148+
});
149+
150+
it('should return false for PUT requests with document secFetchDest', () => {
151+
mockAuthenticateContext.method = 'PUT';
152+
mockAuthenticateContext.secFetchDest = 'document';
153+
expect(handshakeService.isRequestEligibleForHandshake()).toBe(false);
154+
});
155+
156+
it('should return false for POST requests with text/html accept without secFetchDest', () => {
157+
mockAuthenticateContext.method = 'POST';
158+
mockAuthenticateContext.secFetchDest = undefined;
159+
mockAuthenticateContext.accept = 'text/html';
160+
expect(handshakeService.isRequestEligibleForHandshake()).toBe(false);
161+
});
142162
});
143163

144164
describe('buildRedirectToHandshake', () => {

packages/backend/src/tokens/__tests__/request.test.ts

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1925,6 +1925,39 @@ describe('tokens.authenticateRequest(options)', () => {
19251925
});
19261926
});
19271927

1928+
test('does not trigger handshake for cross-origin POST document request on primary domain', async () => {
1929+
const cookieStr = Object.entries({
1930+
__session: mockJwt,
1931+
__client_uat: '12345',
1932+
})
1933+
.map(([k, v]) => `${k}=${v}`)
1934+
.join(';');
1935+
1936+
const request = new Request('https://primary.com/dashboard', {
1937+
method: 'POST',
1938+
headers: {
1939+
...defaultHeaders,
1940+
referer: 'https://satellite.com/form',
1941+
'sec-fetch-dest': 'document',
1942+
cookie: cookieStr,
1943+
},
1944+
});
1945+
1946+
const requestState = await authenticateRequest(request, {
1947+
...mockOptions(),
1948+
publishableKey: PK_LIVE,
1949+
domain: 'primary.com',
1950+
isSatellite: false,
1951+
signInUrl: 'https://primary.com/sign-in',
1952+
});
1953+
1954+
expect(requestState).toBeSignedIn({
1955+
domain: 'primary.com',
1956+
isSatellite: false,
1957+
signInUrl: 'https://primary.com/sign-in',
1958+
});
1959+
});
1960+
19281961
test('does not trigger handshake for non-document requests', async () => {
19291962
const request = mockRequestWithCookies(
19301963
{
@@ -2205,4 +2238,62 @@ describe('tokens.authenticateRequest(options)', () => {
22052238
});
22062239
});
22072240
});
2241+
2242+
describe('POST requests with sec-fetch-dest: document', () => {
2243+
const mockPostRequest = (headers = {}, cookies = {}, requestUrl = 'http://clerk.com/path') => {
2244+
const cookieStr = Object.entries(cookies)
2245+
.map(([k, v]) => `${k}=${v}`)
2246+
.join(';');
2247+
2248+
return new Request(requestUrl, {
2249+
method: 'POST',
2250+
headers: { ...defaultHeaders, 'sec-fetch-dest': 'document', cookie: cookieStr, ...headers },
2251+
});
2252+
};
2253+
2254+
test('returns signed out instead of handshake when clientUat > 0 and no cookieToken', async () => {
2255+
const requestState = await authenticateRequest(
2256+
mockPostRequest({}, { __client_uat: '12345' }),
2257+
mockOptions({ secretKey: 'deadbeef', publishableKey: PK_LIVE }),
2258+
);
2259+
2260+
expect(requestState).toBeSignedOut({ reason: AuthErrorReason.ClientUATWithoutSessionToken });
2261+
});
2262+
2263+
test('returns signed out instead of handshake for satellite app needing sync', async () => {
2264+
const requestState = await authenticateRequest(
2265+
mockPostRequest({}, { __client_uat: '0' }),
2266+
mockOptions({
2267+
publishableKey: PK_LIVE,
2268+
secretKey: 'deadbeef',
2269+
isSatellite: true,
2270+
signInUrl: 'https://primary.dev/sign-in',
2271+
domain: 'satellite.dev',
2272+
}),
2273+
);
2274+
2275+
expect(requestState).toBeSignedOut({
2276+
reason: AuthErrorReason.SessionTokenAndUATMissing,
2277+
isSatellite: true,
2278+
signInUrl: 'https://primary.dev/sign-in',
2279+
domain: 'satellite.dev',
2280+
});
2281+
});
2282+
2283+
test('returns signed out instead of handshake when clientUat > cookieToken.iat', async () => {
2284+
const requestState = await authenticateRequest(
2285+
mockPostRequest(
2286+
{},
2287+
{
2288+
__clerk_db_jwt: 'deadbeef',
2289+
__client_uat: `${mockJwtPayload.iat + 10}`,
2290+
__session: mockJwt,
2291+
},
2292+
),
2293+
mockOptions(),
2294+
);
2295+
2296+
expect(requestState).toBeSignedOut({ reason: AuthErrorReason.SessionTokenIATBeforeClientUAT });
2297+
});
2298+
});
22082299
});

packages/backend/src/tokens/authenticateContext.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ interface AuthenticateContext extends AuthenticateRequestOptions {
1717
forwardedHost: string | undefined;
1818
forwardedProto: string | undefined;
1919
host: string | undefined;
20+
method: string;
2021
origin: string | undefined;
2122
referrer: string | undefined;
2223
secFetchDest: string | undefined;
@@ -281,6 +282,7 @@ class AuthenticateContext implements AuthenticateContext {
281282
}
282283

283284
private initHeaderValues() {
285+
this.method = this.clerkRequest.method;
284286
this.tokenInHeader = this.parseAuthorizationHeader(this.getHeader(constants.Headers.Authorization));
285287
this.origin = this.getHeader(constants.Headers.Origin);
286288
this.host = this.getHeader(constants.Headers.Host);

packages/backend/src/tokens/handshake.ts

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,14 @@ export class HandshakeService {
105105
* @returns boolean indicating if the request is eligible for handshake
106106
*/
107107
isRequestEligibleForHandshake(): boolean {
108-
const { accept, secFetchDest } = this.authenticateContext;
108+
const { accept, method, secFetchDest } = this.authenticateContext;
109+
110+
// Handshake involves a redirect to FAPI which only accepts GET requests.
111+
// Non-GET requests (e.g. POST form submissions) also set sec-fetch-dest: document,
112+
// but redirecting them would result in a 405 Method Not Allowed from FAPI.
113+
if (method !== 'GET') {
114+
return false;
115+
}
109116

110117
// 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.
111118
// Also, we check for 'iframe' because it's the value set when a doc request is made by an iframe.

packages/backend/src/tokens/request.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -475,7 +475,9 @@ export const authenticateRequest: AuthenticateRequest = (async (
475475
}
476476
}
477477
const isRequestEligibleForMultiDomainSync =
478-
authenticateContext.isSatellite && authenticateContext.secFetchDest === 'document';
478+
authenticateContext.isSatellite &&
479+
authenticateContext.secFetchDest === 'document' &&
480+
authenticateContext.method === 'GET';
479481

480482
/**
481483
* Begin multi-domain sync flows
@@ -650,6 +652,7 @@ export const authenticateRequest: AuthenticateRequest = (async (
650652
// Check for cross-origin requests from satellite domains to primary domain
651653
const shouldForceHandshakeForCrossDomain =
652654
!authenticateContext.isSatellite && // We're on primary
655+
authenticateContext.method === 'GET' && // Only GET navigations (POST form submissions set sec-fetch-dest: document too)
653656
authenticateContext.secFetchDest === 'document' && // Document navigation
654657
authenticateContext.isCrossOriginReferrer() && // Came from different domain
655658
!authenticateContext.isKnownClerkReferrer() && // Not from Clerk accounts portal or FAPI

0 commit comments

Comments
 (0)