Skip to content

fix(clerk-js): Correct multisession sign in redirect behavior #6456

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
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
89 changes: 89 additions & 0 deletions packages/clerk-js/src/ui/common/__tests__/withRedirect.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import React from 'react';
import { render } from '../../../testUtils';
import { bindCreateFixtures } from '../../utils/test/createFixtures';
import { withRedirect } from '../withRedirect';
import { sessionExistsAndRedirectUrlPresent } from '../../../utils/componentGuards';

const { createFixtures } = bindCreateFixtures('SignIn');

Expand Down Expand Up @@ -41,3 +42,91 @@ describe('withRedirect', () => {
expect(fixtures.router.navigate).not.toHaveBeenCalledWith('/');
});
});

describe('sessionExistsAndRedirectUrlPresent', () => {
beforeEach(() => {
// Set up window.location.href for tests
Object.defineProperty(window, 'location', {
value: { href: 'http://test.host/' },
writable: true,
});
});

it('returns true when user is signed in and redirect_url is present', async () => {
const { wrapper, fixtures } = await createFixtures(f => {
f.withUser({});
});

// Mock window.location.search to include redirect_url
Object.defineProperty(window, 'location', {
value: {
href: 'http://test.host/',
search: '?redirect_url=https%3A%2F%2Fexample.com'
},
writable: true,
});

const TestComponent = () => <div>Test</div>;
const WithHOC = withRedirect(
TestComponent,
sessionExistsAndRedirectUrlPresent,
() => '/redirected',
);

render(<WithHOC />, { wrapper });

expect(fixtures.router.navigate).toHaveBeenCalledWith('/redirected');
});

it('returns false when user is not signed in', async () => {
const { wrapper, fixtures } = await createFixtures(f => {
// No user signed in
});

// Mock window.location.search to include redirect_url
Object.defineProperty(window, 'location', {
value: {
href: 'http://test.host/',
search: '?redirect_url=https%3A%2F%2Fexample.com'
},
writable: true,
});

const TestComponent = () => <div>Test</div>;
const WithHOC = withRedirect(
TestComponent,
sessionExistsAndRedirectUrlPresent,
() => '/redirected',
);

render(<WithHOC />, { wrapper });

expect(fixtures.router.navigate).not.toHaveBeenCalled();
});

it('returns false when redirect_url is not present', async () => {
const { wrapper, fixtures } = await createFixtures(f => {
f.withUser({});
});

// Mock window.location.search without redirect_url
Object.defineProperty(window, 'location', {
value: {
href: 'http://test.host/',
search: '?other_param=value'
},
writable: true,
});

const TestComponent = () => <div>Test</div>;
const WithHOC = withRedirect(
TestComponent,
sessionExistsAndRedirectUrlPresent,
() => '/redirected',
);

render(<WithHOC />, { wrapper });

expect(fixtures.router.navigate).not.toHaveBeenCalled();
});
});
24 changes: 21 additions & 3 deletions packages/clerk-js/src/ui/common/withRedirect.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import React from 'react';

import { warnings } from '../../core/warnings';
import type { ComponentGuard } from '../../utils';
import { sessionExistsAndSingleSessionModeEnabled } from '../../utils';
import { sessionExistsAndSingleSessionModeEnabled, sessionExistsAndRedirectUrlPresent } from '../../utils';
import { useEnvironment, useOptions, useSignInContext, useSignUpContext } from '../contexts';
import { useRouter } from '../router';
import type { AvailableComponentProps } from '../types';
Expand Down Expand Up @@ -58,9 +58,18 @@ export const withRedirectToAfterSignIn = <P extends AvailableComponentProps>(Com

const HOC = (props: P) => {
const signInCtx = useSignInContext();

// Combined guard: redirect if user is signed in AND either:
// 1. Single session mode is enabled, OR
// 2. There's a redirect_url in the query parameters
const combinedGuard: ComponentGuard = (clerk, environment) => {
return sessionExistsAndSingleSessionModeEnabled(clerk, environment) ||
sessionExistsAndRedirectUrlPresent(clerk);
};
Comment on lines +61 to +68
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Extract duplicated combined guard logic.

The same combined guard logic is duplicated in both withRedirectToAfterSignIn and withRedirectToAfterSignUp functions.

Extract the combined guard to reduce duplication:

+// Combined guard: redirect if user is signed in AND either:
+// 1. Single session mode is enabled, OR
+// 2. There's a redirect_url in the query parameters
+const sessionExistsAndEitherSingleSessionOrRedirectUrl: ComponentGuard = (clerk, environment) => {
+  return sessionExistsAndSingleSessionModeEnabled(clerk, environment) || 
+         sessionExistsAndRedirectUrlPresent(clerk);
+};
+
 export const withRedirectToAfterSignIn = <P extends AvailableComponentProps>(Component: ComponentType<P>) => {
   const displayName = Component.displayName || Component.name || 'Component';
   Component.displayName = displayName;
 
   const HOC = (props: P) => {
     const signInCtx = useSignInContext();
-    
-    // Combined guard: redirect if user is signed in AND either:
-    // 1. Single session mode is enabled, OR
-    // 2. There's a redirect_url in the query parameters
-    const combinedGuard: ComponentGuard = (clerk, environment) => {
-      return sessionExistsAndSingleSessionModeEnabled(clerk, environment) || 
-             sessionExistsAndRedirectUrlPresent(clerk);
-    };
-    
     
     return withRedirect(
       Component,
-      combinedGuard,
+      sessionExistsAndEitherSingleSessionOrRedirectUrl,

Apply the same change to withRedirectToAfterSignUp.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Combined guard: redirect if user is signed in AND either:
// 1. Single session mode is enabled, OR
// 2. There's a redirect_url in the query parameters
const combinedGuard: ComponentGuard = (clerk, environment) => {
return sessionExistsAndSingleSessionModeEnabled(clerk, environment) ||
sessionExistsAndRedirectUrlPresent(clerk);
};
// Combined guard: redirect if user is signed in AND either:
// 1. Single session mode is enabled, OR
// 2. There's a redirect_url in the query parameters
const sessionExistsAndEitherSingleSessionOrRedirectUrl: ComponentGuard = (clerk, environment) => {
return sessionExistsAndSingleSessionModeEnabled(clerk, environment) ||
sessionExistsAndRedirectUrlPresent(clerk);
};
export const withRedirectToAfterSignIn = <P extends AvailableComponentProps>(
Component: ComponentType<P>
) => {
const displayName = Component.displayName || Component.name || 'Component';
Component.displayName = displayName;
const HOC = (props: P) => {
const signInCtx = useSignInContext();
return withRedirect(
Component,
sessionExistsAndEitherSingleSessionOrRedirectUrl,
);
};
HOC.displayName = `withRedirectToAfterSignIn(${displayName})`;
return HOC;
};
🤖 Prompt for AI Agents
In packages/clerk-js/src/ui/common/withRedirect.tsx around lines 61 to 68, the
combined guard logic is duplicated in both withRedirectToAfterSignIn and
withRedirectToAfterSignUp functions. Extract this combined guard logic into a
single reusable function or constant outside these functions, then replace the
duplicated code in both withRedirectToAfterSignIn and withRedirectToAfterSignUp
with calls to this extracted combined guard to eliminate duplication.


return withRedirect(
Component,
sessionExistsAndSingleSessionModeEnabled,
combinedGuard,
({ clerk }) => signInCtx.sessionTaskUrl || signInCtx.afterSignInUrl || clerk.buildAfterSignInUrl(),
signInCtx.sessionTaskUrl
? warnings.cannotRenderSignInComponentWhenTaskExists
Expand All @@ -79,9 +88,18 @@ export const withRedirectToAfterSignUp = <P extends AvailableComponentProps>(Com

const HOC = (props: P) => {
const signUpCtx = useSignUpContext();

// Combined guard: redirect if user is signed in AND either:
// 1. Single session mode is enabled, OR
// 2. There's a redirect_url in the query parameters
const combinedGuard: ComponentGuard = (clerk, environment) => {
return sessionExistsAndSingleSessionModeEnabled(clerk, environment) ||
sessionExistsAndRedirectUrlPresent(clerk);
};

return withRedirect(
Component,
sessionExistsAndSingleSessionModeEnabled,
combinedGuard,
({ clerk }) => signUpCtx.sessionTaskUrl || signUpCtx.afterSignUpUrl || clerk.buildAfterSignUpUrl(),
signUpCtx.sessionTaskUrl
? warnings.cannotRenderSignUpComponentWhenTaskExists
Expand Down
11 changes: 11 additions & 0 deletions packages/clerk-js/src/utils/componentGuards.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,17 @@ export const sessionExistsAndSingleSessionModeEnabled: ComponentGuard = (clerk,
return !!(clerk.session && environment?.authConfig.singleSessionMode);
};

export const sessionExistsAndRedirectUrlPresent: ComponentGuard = (clerk) => {
if (!clerk.session) {
return false;
}

const urlParams = new URLSearchParams(window.location.search);
const redirectUrl = urlParams.get('redirect_url');

return !!redirectUrl;
};
Comment on lines +13 to +22
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add SSR safety check for window object access.

The function directly accesses window.location.search which will cause runtime errors in server-side rendering environments where window is undefined.

Apply this diff to add SSR safety:

 export const sessionExistsAndRedirectUrlPresent: ComponentGuard = (clerk) => {
   if (!clerk.session) {
     return false;
   }
   
+  // Guard against SSR environments where window is undefined
+  if (typeof window === 'undefined') {
+    return false;
+  }
+  
   const urlParams = new URLSearchParams(window.location.search);
   const redirectUrl = urlParams.get('redirect_url');
   
   return !!redirectUrl;
 };
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export const sessionExistsAndRedirectUrlPresent: ComponentGuard = (clerk) => {
if (!clerk.session) {
return false;
}
const urlParams = new URLSearchParams(window.location.search);
const redirectUrl = urlParams.get('redirect_url');
return !!redirectUrl;
};
export const sessionExistsAndRedirectUrlPresent: ComponentGuard = (clerk) => {
if (!clerk.session) {
return false;
}
// Guard against SSR environments where window is undefined
if (typeof window === 'undefined') {
return false;
}
const urlParams = new URLSearchParams(window.location.search);
const redirectUrl = urlParams.get('redirect_url');
return !!redirectUrl;
};
🤖 Prompt for AI Agents
In packages/clerk-js/src/utils/componentGuards.ts around lines 13 to 22, the
function accesses window.location.search directly, which breaks in server-side
rendering environments where window is undefined. To fix this, add a check to
ensure window is defined before accessing window.location.search. If window is
undefined, return false early to prevent runtime errors during SSR.


export const noUserExists: ComponentGuard = clerk => {
return !clerk.user;
};
Expand Down
Loading