New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(backend): Limit handshake to document requests #2352
Conversation
🦋 Changeset detectedLatest commit: df9aba8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@@ -388,12 +388,13 @@ export default (QUnit: QUnit) => { | |||
assert.strictEqual(requestState.toAuth(), null); | |||
}); | |||
|
|||
test('cookieToken: returns undefined when crossOriginReferrer in development and is satellite [6n]', async assert => { | |||
test('cookieToken: returns signedIn when satellite but valid token and clientUat', async assert => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test name was inaccurate 🧐
@@ -299,6 +298,7 @@ export default (QUnit: QUnit) => { | |||
mockRequestWithCookies( | |||
{ | |||
...defaultHeaders, | |||
'sec-fetch-dest': 'empty', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that we're defaulting to sec-fetch-dest: document
, we need to explicitly set it to empty for this test case
function isRequestEligibleForHandshake(authenticateContext: { secFetchDest?: string; accept?: string }) { | ||
const { accept, secFetchDest } = authenticateContext; | ||
|
||
// NOTE: we could also check sec-fetch-mode === navigate here, but according to the spec, sec-fetch-dest: document should indicate that the request is the result of a user navigation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, you can't overwrite sec-fetch-mode
, so testing became difficult.
function handleMaybeHandshakeStatus( | ||
context: typeof authenticateContext, | ||
reason: AuthErrorReason, | ||
message: string, | ||
headers?: Headers, | ||
) { | ||
if (isRequestEligibleForHandshake(context)) { | ||
// Right now the only usage of passing in different headers is for multi-domain sync, which redirects somewhere else. | ||
// In the future if we want to decorate the handshake redirect with additional headers per call we need to tweak this logic. | ||
return handshake(context, reason, message, headers ?? buildRedirectToHandshake()); | ||
} | ||
return signedOut(context, reason, message, new Headers()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This felt like a reasonable abstraction, easy to tweak the handshake heuristic in one place going forward.
Description
Limit the handshake flow to document requests. We identify this by checking for the presence of the
Sec-Fetch-Dest
header with the valuedocument
. For browsers that don't support this header, we check that theAccept
header value starts withtext/html
, which is a reasonable enough indication that it's a document request.Checklist
npm test
runs as expected.npm run build
runs as expected.Type of change
Packages affected
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/clerk-expo
@clerk/fastify
gatsby-plugin-clerk
@clerk/localizations
@clerk/nextjs
@clerk/clerk-react
@clerk/remix
@clerk/clerk-sdk-node
@clerk/shared
@clerk/themes
@clerk/types
build/tooling/chore