Skip to content

fix(backend): Fix unhandled handshake error #2541

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
brkalow merged 2 commits into from
Jan 10, 2024
Merged

fix(backend): Fix unhandled handshake error #2541

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
648ef82
fix(backend): Fix unhandled handshake error
brkalow Jan 10, 2024
f8a1026
fix(backend): Handle development verification failure
brkalow Jan 10, 2024
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/few-rings-push.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@clerk/backend': patch
---

Fix an error in the handshake flow where the request would throw an unhandled error when verification of the handshake payload fails.
11 changes: 6 additions & 5 deletions integration/tests/handshake.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -827,26 +827,27 @@ test.describe('Client handshake @generic', () => {
}),
redirect: 'manual',
});
expect(res.status).toBe(500);
expect(res.status).toBe(200);
});

test('Handshake result - prod - skew - clock ahead', async () => {
test('Handshake result - prod - session token expired and handshake stale', async () => {
const config = generateConfig({
mode: 'live',
});
const { token: currentSessionToken, claims } = config.generateToken({ state: 'expired' });
const { token } = config.generateToken({ state: 'expired' });
const cookiesToSet = [`__session=${token};path=/`, 'foo=bar;path=/;domain=example.com'];
const handshake = await config.generateHandshakeToken(cookiesToSet);
const res = await fetch(app.serverUrl + '/', {
headers: new Headers({
'X-Publishable-Key': config.pk,
'X-Secret-Key': config.sk,
Cookie: `__clerk_handshake=${handshake}`,
Cookie: `__clerk_handshake=${handshake};__session=${currentSessionToken};__client_uat=${claims.iat}`,
'Sec-Fetch-Dest': 'document',
}),
redirect: 'manual',
});
expect(res.status).toBe(500);
expect(res.status).toBe(307);
});

test('Handshake result - prod - mismatched keys', async () => {
Expand All @@ -866,6 +867,6 @@ test.describe('Client handshake @generic', () => {
}),
redirect: 'manual',
});
expect(res.status).toBe(500);
expect(res.status).toBe(200);
});
});
20 changes: 17 additions & 3 deletions packages/backend/src/tokens/request.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,8 +124,8 @@ export async function authenticateRequest(

if (
instanceType === 'development' &&
(error.reason === TokenVerificationErrorReason.TokenExpired ||
error.reason === TokenVerificationErrorReason.TokenNotActiveYet)
(error?.reason === TokenVerificationErrorReason.TokenExpired ||
error?.reason === TokenVerificationErrorReason.TokenNotActiveYet)
) {
error.tokenCarrier = 'cookie';
// This probably means we're dealing with clock skew
Expand Down Expand Up @@ -204,7 +204,21 @@ ${error.getFullMessage()}`,
* If we have a handshakeToken, resolve the handshake and attempt to return a definitive signed in or signed out state.
*/
if (authenticateContext.handshakeToken) {
return resolveHandshake();
try {
return await resolveHandshake();
} catch (error) {
// If for some reason the handshake token is invalid or stale, we ignore it and continue trying to authenticate the request.
// Worst case, the handshake will trigger again and return a refreshed token.
Comment on lines +210 to +211

@panteliselef panteliselef Jan 10, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is it possible that this would lead to an infinite redirect loop, if FAPI will not return a fresh token ?

@brkalow brkalow Jan 10, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, it seems less likely that FAPI would return a stale token / invalid handshake payload though. For some reason we're seeing stale handshake cookies here, so the only solution is to move past the handshake handling and attempt to authenticate the request like normal.

if (error instanceof TokenVerificationError && instanceType === 'development') {
if (error.reason === TokenVerificationErrorReason.TokenInvalidSignature) {
throw new Error(
`Clerk: Handshake token verification failed due to an invalid signature. If you have switched Clerk keys locally, clear your cookies and try again.`,
);
}

throw new Error(`Clerk: Handshake token verification failed: ${error.getFullMessage()}.`);
}
}
}

/**
Expand Down