From 5c18671f158f8077f822877ce5c1fa192199aeda Mon Sep 17 00:00:00 2001 From: Stefanos Anagnostou Date: Tue, 24 Sep 2024 16:12:50 +0300 Subject: [PATCH] fix(backend): Drop the `__clerk_refresh` debugging query param (#4213) --- .changeset/tender-apes-smile.md | 5 ++ integration/tests/handshake.test.ts | 40 ++++----- packages/backend/src/constants.ts | 1 - .../src/tokens/__tests__/request.test.ts | 10 ++- packages/backend/src/tokens/authStatus.ts | 2 +- packages/backend/src/tokens/request.ts | 82 ++++++++----------- 6 files changed, 67 insertions(+), 73 deletions(-) create mode 100644 .changeset/tender-apes-smile.md diff --git a/.changeset/tender-apes-smile.md b/.changeset/tender-apes-smile.md new file mode 100644 index 0000000000..f4e9d344e3 --- /dev/null +++ b/.changeset/tender-apes-smile.md @@ -0,0 +1,5 @@ +--- +"@clerk/backend": patch +--- + +Drop the `__clerk_refresh` debugging query param and use only the `__clerk_hs_reason` param for all scenarios. diff --git a/integration/tests/handshake.test.ts b/integration/tests/handshake.test.ts index e79d1dd80c..887b45b0a4 100644 --- a/integration/tests/handshake.test.ts +++ b/integration/tests/handshake.test.ts @@ -162,7 +162,7 @@ test.describe('Client handshake @generic', () => { expect(res.headers.get('location')).toBe( `https://${config.pkHost}/v1/client/handshake?redirect_url=${encodeURIComponent( `${app.serverUrl}/`, - )}&suffixed_cookies=false&__clerk_hs_reason=session-token-expired&__clerk_refresh=no-cookie${devBrowserQuery}`, + )}&suffixed_cookies=false&__clerk_hs_reason=session-token-expired-refresh-non-eligible-no-refresh-cookie${devBrowserQuery}`, ); }); @@ -185,7 +185,7 @@ test.describe('Client handshake @generic', () => { expect(res.headers.get('location')).toBe( `https://${config.pkHost}/v1/client/handshake?redirect_url=${encodeURIComponent( `${app.serverUrl}/`, - )}&suffixed_cookies=false&__clerk_hs_reason=session-token-expired&__clerk_refresh=no-cookie`, + )}&suffixed_cookies=false&__clerk_hs_reason=session-token-expired-refresh-non-eligible-no-refresh-cookie`, ); }); @@ -209,7 +209,7 @@ test.describe('Client handshake @generic', () => { expect(res.headers.get('location')).toBe( `https://${config.pkHost}/v1/client/handshake?redirect_url=${encodeURIComponent( `${app.serverUrl}/`, - )}&suffixed_cookies=false&__clerk_hs_reason=session-token-expired&__clerk_refresh=no-cookie`, + )}&suffixed_cookies=false&__clerk_hs_reason=session-token-expired-refresh-non-eligible-no-refresh-cookie`, ); }); @@ -232,7 +232,7 @@ test.describe('Client handshake @generic', () => { expect(res.headers.get('location')).toBe( `https://${config.pkHost}/v1/client/handshake?redirect_url=${encodeURIComponent( `${app.serverUrl}/`, - )}&suffixed_cookies=false&__clerk_hs_reason=session-token-not-active-yet&__clerk_refresh=no-cookie${devBrowserQuery}`, + )}&suffixed_cookies=false&__clerk_hs_reason=session-token-nbf${devBrowserQuery}`, ); }); @@ -256,7 +256,7 @@ test.describe('Client handshake @generic', () => { expect(res.headers.get('location')).toBe( `https://${config.pkHost}/v1/client/handshake?redirect_url=${encodeURIComponent( `${app.serverUrl}/`, - )}&suffixed_cookies=false&__clerk_hs_reason=session-token-not-active-yet&__clerk_refresh=no-cookie${devBrowserQuery}`, + )}&suffixed_cookies=false&__clerk_hs_reason=session-token-nbf${devBrowserQuery}`, ); }); @@ -280,7 +280,7 @@ test.describe('Client handshake @generic', () => { expect(res.headers.get('location')).toBe( `https://example.com/clerk/v1/client/handshake?redirect_url=${encodeURIComponent( `${app.serverUrl}/`, - )}&suffixed_cookies=false&__clerk_hs_reason=session-token-expired&__clerk_refresh=no-cookie${devBrowserQuery}`, + )}&suffixed_cookies=false&__clerk_hs_reason=session-token-expired-refresh-non-eligible-no-refresh-cookie${devBrowserQuery}`, ); }); @@ -304,7 +304,7 @@ test.describe('Client handshake @generic', () => { expect(res.headers.get('location')).toBe( `https://example.com/clerk/v1/client/handshake?redirect_url=${encodeURIComponent( `${app.serverUrl}/`, - )}&suffixed_cookies=false&__clerk_hs_reason=session-token-expired&__clerk_refresh=no-cookie`, + )}&suffixed_cookies=false&__clerk_hs_reason=session-token-expired-refresh-non-eligible-no-refresh-cookie`, ); }); @@ -328,7 +328,7 @@ test.describe('Client handshake @generic', () => { expect(res.headers.get('location')).toBe( `https://${config.pkHost}/v1/client/handshake?redirect_url=${encodeURIComponent( `${app.serverUrl}/`, - )}&suffixed_cookies=false&__clerk_hs_reason=session-token-expired&__clerk_refresh=no-cookie${devBrowserQuery}`, + )}&suffixed_cookies=false&__clerk_hs_reason=session-token-expired-refresh-non-eligible-no-refresh-cookie${devBrowserQuery}`, ); }); @@ -352,7 +352,7 @@ test.describe('Client handshake @generic', () => { expect(res.headers.get('location')).toBe( `https://clerk.example.com/v1/client/handshake?redirect_url=${encodeURIComponent( `${app.serverUrl}/`, - )}&suffixed_cookies=false&__clerk_hs_reason=session-token-expired&__clerk_refresh=no-cookie`, + )}&suffixed_cookies=false&__clerk_hs_reason=session-token-expired-refresh-non-eligible-no-refresh-cookie`, ); }); @@ -373,7 +373,7 @@ test.describe('Client handshake @generic', () => { expect(res.headers.get('location')).toBe( `https://${config.pkHost}/v1/client/handshake?redirect_url=${encodeURIComponent( `${app.serverUrl}/`, - )}&suffixed_cookies=false&__clerk_hs_reason=client-uat-but-no-session-token&__clerk_refresh=no-cookie${devBrowserQuery}`, + )}&suffixed_cookies=false&__clerk_hs_reason=client-uat-but-no-session-token${devBrowserQuery}`, ); }); @@ -394,7 +394,7 @@ test.describe('Client handshake @generic', () => { expect(res.headers.get('location')).toBe( `https://${config.pkHost}/v1/client/handshake?redirect_url=${encodeURIComponent( `${app.serverUrl}/`, - )}&suffixed_cookies=false&__clerk_hs_reason=client-uat-but-no-session-token&__clerk_refresh=no-cookie`, + )}&suffixed_cookies=false&__clerk_hs_reason=client-uat-but-no-session-token`, ); }); @@ -495,7 +495,7 @@ test.describe('Client handshake @generic', () => { expect(res.headers.get('location')).toBe( `https://clerk.example.com/v1/client/handshake?redirect_url=${encodeURIComponent( app.serverUrl + '/', - )}&suffixed_cookies=false&__clerk_hs_reason=satellite-needs-syncing&__clerk_refresh=no-cookie`, + )}&suffixed_cookies=false&__clerk_hs_reason=satellite-needs-syncing`, ); }); @@ -532,7 +532,7 @@ test.describe('Client handshake @generic', () => { expect(res.headers.get('location')).toBe( `https://${config.pkHost}/v1/client/handshake?redirect_url=${encodeURIComponent( `${app.serverUrl}/`, - )}&suffixed_cookies=false&__clerk_hs_reason=dev-browser-missing&__clerk_refresh=no-cookie`, + )}&suffixed_cookies=false&__clerk_hs_reason=dev-browser-missing`, ); }); @@ -555,7 +555,7 @@ test.describe('Client handshake @generic', () => { expect(res.headers.get('location')).toBe( `https://${config.pkHost}/v1/client/handshake?redirect_url=${encodeURIComponent( `${app.serverUrl}/`, - )}hello%3Ffoo%3Dbar&suffixed_cookies=false&__clerk_hs_reason=session-token-expired&__clerk_refresh=no-cookie${devBrowserQuery}`, + )}hello%3Ffoo%3Dbar&suffixed_cookies=false&__clerk_hs_reason=session-token-expired-refresh-non-eligible-no-refresh-cookie${devBrowserQuery}`, ); }); @@ -578,7 +578,7 @@ test.describe('Client handshake @generic', () => { expect(res.headers.get('location')).toBe( `https://${config.pkHost}/v1/client/handshake?redirect_url=${encodeURIComponent( `${app.serverUrl}/`, - )}hello%3Ffoo%3Dbar&suffixed_cookies=false&__clerk_hs_reason=session-token-expired&__clerk_refresh=no-cookie`, + )}hello%3Ffoo%3Dbar&suffixed_cookies=false&__clerk_hs_reason=session-token-expired-refresh-non-eligible-no-refresh-cookie`, ); }); @@ -601,7 +601,7 @@ test.describe('Client handshake @generic', () => { }); expect(res.status).toBe(307); expect(res.headers.get('location')).toBe( - `https://${config.pkHost}/v1/client/handshake?redirect_url=https%3A%2F%2Fexample.com%2Fhello%3Ffoo%3Dbar&suffixed_cookies=false&__clerk_hs_reason=session-token-expired&__clerk_refresh=no-cookie${devBrowserQuery}`, + `https://${config.pkHost}/v1/client/handshake?redirect_url=https%3A%2F%2Fexample.com%2Fhello%3Ffoo%3Dbar&suffixed_cookies=false&__clerk_hs_reason=session-token-expired-refresh-non-eligible-no-refresh-cookie${devBrowserQuery}`, ); }); @@ -624,7 +624,7 @@ test.describe('Client handshake @generic', () => { }); expect(res.status).toBe(307); expect(res.headers.get('location')).toBe( - `https://${config.pkHost}/v1/client/handshake?redirect_url=https%3A%2F%2Fexample.com%2Fhello%3Ffoo%3Dbar&suffixed_cookies=false&__clerk_hs_reason=session-token-expired&__clerk_refresh=no-cookie`, + `https://${config.pkHost}/v1/client/handshake?redirect_url=https%3A%2F%2Fexample.com%2Fhello%3Ffoo%3Dbar&suffixed_cookies=false&__clerk_hs_reason=session-token-expired-refresh-non-eligible-no-refresh-cookie`, ); }); @@ -647,7 +647,7 @@ test.describe('Client handshake @generic', () => { }); expect(res.status).toBe(307); expect(res.headers.get('location')).toBe( - `https://${config.pkHost}/v1/client/handshake?redirect_url=https%3A%2F%2Fexample.com%3A3213%2Fhello%3Ffoo%3Dbar&suffixed_cookies=false&__clerk_hs_reason=session-token-expired&__clerk_refresh=no-cookie${devBrowserQuery}`, + `https://${config.pkHost}/v1/client/handshake?redirect_url=https%3A%2F%2Fexample.com%3A3213%2Fhello%3Ffoo%3Dbar&suffixed_cookies=false&__clerk_hs_reason=session-token-expired-refresh-non-eligible-no-refresh-cookie${devBrowserQuery}`, ); }); @@ -670,7 +670,7 @@ test.describe('Client handshake @generic', () => { }); expect(res.status).toBe(307); expect(res.headers.get('location')).toBe( - `https://${config.pkHost}/v1/client/handshake?redirect_url=https%3A%2F%2Fexample.com%3A3213%2Fhello%3Ffoo%3Dbar&suffixed_cookies=false&__clerk_hs_reason=session-token-expired&__clerk_refresh=no-cookie`, + `https://${config.pkHost}/v1/client/handshake?redirect_url=https%3A%2F%2Fexample.com%3A3213%2Fhello%3Ffoo%3Dbar&suffixed_cookies=false&__clerk_hs_reason=session-token-expired-refresh-non-eligible-no-refresh-cookie`, ); }); @@ -799,7 +799,7 @@ test.describe('Client handshake @generic', () => { expect(res.headers.get('location')).toBe( `https://${config.pkHost}/v1/client/handshake?redirect_url=${encodeURIComponent( `${app.serverUrl}/`, - )}&suffixed_cookies=false&__clerk_hs_reason=dev-browser-sync&__clerk_refresh=no-cookie&__clerk_db_jwt=asdf`, + )}&suffixed_cookies=false&__clerk_hs_reason=dev-browser-sync&__clerk_db_jwt=asdf`, ); }); diff --git a/packages/backend/src/constants.ts b/packages/backend/src/constants.ts index 93372987da..9b7d92e83e 100644 --- a/packages/backend/src/constants.ts +++ b/packages/backend/src/constants.ts @@ -32,7 +32,6 @@ const QueryParameters = { HandshakeHelp: '__clerk_help', LegacyDevBrowser: '__dev_session', HandshakeReason: '__clerk_hs_reason', - RefreshTokenError: '__clerk_refresh', } as const; const Headers = { diff --git a/packages/backend/src/tokens/__tests__/request.test.ts b/packages/backend/src/tokens/__tests__/request.test.ts index e72c8904b1..7cd1ff3f4a 100644 --- a/packages/backend/src/tokens/__tests__/request.test.ts +++ b/packages/backend/src/tokens/__tests__/request.test.ts @@ -13,7 +13,7 @@ import { import runtime from '../../runtime'; import { jsonOk } from '../../util/testUtils'; import { AuthErrorReason, type AuthReason, AuthStatus, type RequestState } from '../authStatus'; -import { authenticateRequest } from '../request'; +import { authenticateRequest, RefreshTokenErrorReason } from '../request'; import type { AuthenticateRequestOptions } from '../types'; const PK_TEST = 'pk_test_Y2xlcmsuaW5zcGlyZWQucHVtYS03NC5sY2wuZGV2JA'; @@ -238,7 +238,9 @@ export default (QUnit: QUnit) => { const requestState = await authenticateRequest(mockRequestWithHeaderAuth(), mockOptions()); - assertHandshake(assert, requestState, { reason: AuthErrorReason.SessionTokenExpired }); + assertHandshake(assert, requestState, { + reason: `${AuthErrorReason.SessionTokenExpired}-refresh-${RefreshTokenErrorReason.NonEligibleNoCookie}`, + }); assert.strictEqual(requestState.toAuth(), null); }); @@ -554,7 +556,9 @@ export default (QUnit: QUnit) => { mockOptions(), ); - assertHandshake(assert, requestState, { reason: AuthErrorReason.SessionTokenExpired }); + assertHandshake(assert, requestState, { + reason: `${AuthErrorReason.SessionTokenExpired}-refresh-${RefreshTokenErrorReason.NonEligibleNoCookie}`, + }); assert.true(/^JWT is expired/.test(requestState.message || '')); assert.strictEqual(requestState.toAuth(), null); }); diff --git a/packages/backend/src/tokens/authStatus.ts b/packages/backend/src/tokens/authStatus.ts index ec836e6a85..f43cff7549 100644 --- a/packages/backend/src/tokens/authStatus.ts +++ b/packages/backend/src/tokens/authStatus.ts @@ -66,7 +66,7 @@ export const AuthErrorReason = { SessionTokenMissing: 'session-token-missing', SessionTokenExpired: 'session-token-expired', SessionTokenIATBeforeClientUAT: 'session-token-iat-before-client-uat', - SessionTokenNotActiveYet: 'session-token-not-active-yet', + SessionTokenNBF: 'session-token-nbf', SessionTokenIatInTheFuture: 'session-token-iat-in-the-future', SessionTokenWithoutClientUAT: 'session-token-but-no-client-uat', UnexpectedError: 'unexpected-error', diff --git a/packages/backend/src/tokens/request.ts b/packages/backend/src/tokens/request.ts index b0d3af2cbd..4e31dc04f4 100644 --- a/packages/backend/src/tokens/request.ts +++ b/packages/backend/src/tokens/request.ts @@ -16,16 +16,16 @@ import { verifyHandshakeToken } from './handshake'; import type { AuthenticateRequestOptions } from './types'; import { verifyToken } from './verify'; -const RefreshTokenErrorReason = { - NoCookie: 'no-cookie', - NonEligible: 'non-eligible', +export const RefreshTokenErrorReason = { + NonEligibleNoCookie: 'non-eligible-no-refresh-cookie', + NonEligibleNonGet: 'non-eligible-non-get', InvalidSessionToken: 'invalid-session-token', MissingApiClient: 'missing-api-client', MissingSessionToken: 'missing-session-token', MissingRefreshToken: 'missing-refresh-token', - SessionTokenDecodeFailed: 'session-token-decode-failed', - FetchNetworkError: 'fetch-network-error', - UnexpectedRefreshError: 'unexpected-refresh-error', + ExpiredSessionTokenDecodeFailed: 'expired-session-token-decode-failed', + FetchError: 'fetch-error', + UnexpectedSDKError: 'unexpected-sdk-error', } as const; function assertSignInUrlExists(signInUrl: string | undefined, key: string): asserts signInUrl is string { @@ -110,13 +110,7 @@ export async function authenticateRequest( return updatedURL; } - function buildRedirectToHandshake({ - handshakeReason, - refreshError, - }: { - handshakeReason: AuthErrorReason; - refreshError: string; - }) { + function buildRedirectToHandshake({ handshakeReason }: { handshakeReason: string }) { const redirectUrl = removeDevBrowserFromURL(authenticateContext.clerkUrl); const frontendApiNoProtocol = authenticateContext.frontendApi.replace(/http(s)?:\/\//, ''); @@ -124,7 +118,6 @@ export async function authenticateRequest( url.searchParams.append('redirect_url', redirectUrl?.href || ''); url.searchParams.append('suffixed_cookies', authenticateContext.suffixedCookies.toString()); url.searchParams.append(constants.QueryParameters.HandshakeReason, handshakeReason); - url.searchParams.append(constants.QueryParameters.RefreshTokenError, refreshError); if (authenticateContext.instanceType === 'development' && authenticateContext.devBrowserToken) { url.searchParams.append(constants.QueryParameters.DevBrowser, authenticateContext.devBrowserToken); @@ -238,8 +231,8 @@ ${error.getFullMessage()}`, return { data: null, error: { - message: 'Unable to decode session token.', - cause: { reason: RefreshTokenErrorReason.SessionTokenDecodeFailed, errors: decodedErrors }, + message: 'Unable to decode the expired session token.', + cause: { reason: RefreshTokenErrorReason.ExpiredSessionTokenDecodeFailed, errors: decodedErrors }, }, }; } @@ -261,7 +254,7 @@ ${error.getFullMessage()}`, data: null, error: { message: `Fetch unexpected error`, - cause: { reason: RefreshTokenErrorReason.FetchNetworkError, errors: err.errors }, + cause: { reason: RefreshTokenErrorReason.FetchError, errors: err.errors }, }, }; } @@ -305,22 +298,14 @@ ${error.getFullMessage()}`, function handleMaybeHandshakeStatus( authenticateContext: AuthenticateContext, - reason: AuthErrorReason, + reason: string, message: string, headers?: Headers, - refreshError?: string, ): SignedInState | SignedOutState | HandshakeState { if (isRequestEligibleForHandshake(authenticateContext)) { - // If a refresh error is not passed in, we default to 'no-cookie' or 'non-eligible'. - refreshError = - refreshError || - (authenticateContext.refreshTokenInCookie - ? RefreshTokenErrorReason.NonEligible - : RefreshTokenErrorReason.NoCookie); - // 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. - const handshakeHeaders = headers ?? buildRedirectToHandshake({ handshakeReason: reason, refreshError }); + const handshakeHeaders = headers ?? buildRedirectToHandshake({ handshakeReason: reason }); // Chrome aggressively caches inactive tabs. If we don't set the header here, // all 307 redirects will be cached and the handshake will end up in an infinite loop. @@ -451,10 +436,6 @@ ${error.getFullMessage()}`, ); const authErrReason = AuthErrorReason.SatelliteCookieNeedsSyncing; redirectURL.searchParams.append(constants.QueryParameters.HandshakeReason, authErrReason); - const refreshTokenError = authenticateContext.refreshTokenInCookie - ? RefreshTokenErrorReason.NonEligible - : RefreshTokenErrorReason.NoCookie; - redirectURL.searchParams.append(constants.QueryParameters.RefreshTokenError, refreshTokenError); const headers = new Headers({ [constants.Headers.Location]: redirectURL.toString() }); return handleMaybeHandshakeStatus(authenticateContext, authErrReason, '', headers); @@ -477,10 +458,6 @@ ${error.getFullMessage()}`, redirectBackToSatelliteUrl.searchParams.append(constants.QueryParameters.ClerkSynced, 'true'); const authErrReason = AuthErrorReason.PrimaryRespondsToSyncing; redirectBackToSatelliteUrl.searchParams.append(constants.QueryParameters.HandshakeReason, authErrReason); - const refreshTokenError = authenticateContext.refreshTokenInCookie - ? RefreshTokenErrorReason.NonEligible - : RefreshTokenErrorReason.NoCookie; - redirectBackToSatelliteUrl.searchParams.append(constants.QueryParameters.RefreshTokenError, refreshTokenError); const headers = new Headers({ [constants.Headers.Location]: redirectBackToSatelliteUrl.toString() }); return handleMaybeHandshakeStatus(authenticateContext, authErrReason, '', headers); @@ -537,9 +514,7 @@ ${error.getFullMessage()}`, return signedOut(authenticateContext, AuthErrorReason.UnexpectedError); } - let refreshError: string = authenticateContext.refreshTokenInCookie - ? RefreshTokenErrorReason.NonEligible - : RefreshTokenErrorReason.NoCookie; + let refreshError: string | null; if (isRequestEligibleForRefresh(err, authenticateContext, request)) { const { data, error } = await attemptRefresh(authenticateContext); @@ -552,7 +527,16 @@ ${error.getFullMessage()}`, if (error?.cause?.reason) { refreshError = error.cause.reason; } else { - refreshError = RefreshTokenErrorReason.UnexpectedRefreshError; + refreshError = RefreshTokenErrorReason.UnexpectedSDKError; + } + } else { + if (request.method !== 'GET') { + refreshError = RefreshTokenErrorReason.NonEligibleNonGet; + } else if (!authenticateContext.refreshTokenInCookie) { + refreshError = RefreshTokenErrorReason.NonEligibleNoCookie; + } else { + //refresh error is not applicable if token verification error is not 'session-token-expired' + refreshError = null; } } @@ -567,10 +551,8 @@ ${error.getFullMessage()}`, if (reasonToHandshake) { return handleMaybeHandshakeStatus( authenticateContext, - convertTokenVerificationErrorReasonToAuthErrorReason(err.reason), + convertTokenVerificationErrorReasonToAuthErrorReason({ tokenError: err.reason, refreshError }), err.getFullMessage(), - undefined, - refreshError, ); } @@ -592,14 +574,18 @@ export const debugRequestState = (params: RequestState) => { return { isSignedIn, proxyUrl, reason, message, publishableKey, isSatellite, domain }; }; -const convertTokenVerificationErrorReasonToAuthErrorReason = ( - reason: TokenVerificationErrorReason, -): AuthErrorReason => { - switch (reason) { +const convertTokenVerificationErrorReasonToAuthErrorReason = ({ + tokenError, + refreshError, +}: { + tokenError: TokenVerificationErrorReason; + refreshError: string | null; +}): string => { + switch (tokenError) { case TokenVerificationErrorReason.TokenExpired: - return AuthErrorReason.SessionTokenExpired; + return `${AuthErrorReason.SessionTokenExpired}-refresh-${refreshError}`; case TokenVerificationErrorReason.TokenNotActiveYet: - return AuthErrorReason.SessionTokenNotActiveYet; + return AuthErrorReason.SessionTokenNBF; case TokenVerificationErrorReason.TokenIatInTheFuture: return AuthErrorReason.SessionTokenIatInTheFuture; default: