fix(auth): clear invalid login token to prevent setup wizard loop#38468
fix(auth): clear invalid login token to prevent setup wizard loop#38468jaiswalism wants to merge 3 commits into
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
|
|
WalkthroughWraps startup user synchronization in a try/catch to centralize error handling; classifies auth errors to clear local auth state and logout when needed. Also clears stored login tokens and removes Changes
Sequence Diagram(s)(Skipped — changes are limited and do not introduce a new multi-component control flow requiring visualization.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR fixes an infinite redirect loop that occurred when users had invalid authentication tokens (e.g., after MongoDB restore or workspace ID changes). The fix ensures invalid tokens are properly cleared when login attempts fail, preventing users from getting stuck on the registration/setup wizard page.
Changes:
- Clear invalid authentication tokens when token-based login fails
- Add error handling for user data synchronization failures during startup
- Remove invalid URL parameters after failed login attempts
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| apps/meteor/client/views/root/hooks/useLoginViaQuery.ts | Clears invalid tokens and URL parameters when token login fails; marks async function call as void |
| apps/meteor/client/startup/startup.ts | Wraps user data synchronization in try/catch to handle auth failures gracefully |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/meteor/client/views/root/hooks/useLoginViaQuery.ts`:
- Line 38: Replace the direct call to the internal Meteor API
Accounts._unstoreLoginToken() in useLoginViaQuery with the project's established
wrapper; import and call the wrapper exported from
apps/meteor/client/meteor/overrides/unstoreLoginToken.ts (the same wrapper used
by AuthenticationProvider.tsx) instead of calling Accounts._unstoreLoginToken()
directly so the internal API is accessed consistently via the override.
🧹 Nitpick comments (2)
apps/meteor/client/views/root/hooks/useLoginViaQuery.ts (2)
37-38: Remove the code comment per coding guidelines.The comment on line 37 should be removed as per the coding guidelines for TypeScript/JavaScript files which state to avoid code comments in the implementation.
♻️ Proposed fix
} catch (error) { console.error('Failed to login with token', error); - // Clear invalid tokens, this prevents getting stuck on registration page Accounts._unstoreLoginToken();As per coding guidelines:
**/*.{ts,tsx,js}: Avoid code comments in the implementation.
26-46: Consider extracting duplicate search parameter cleanup logic.The search parameter cleanup logic (lines 26-34 and 39-46) is duplicated between the success and error paths. You could extract this into a helper function to reduce duplication.
♻️ Optional refactor to reduce duplication
useEffect(() => { + const clearSearchParams = () => { + const { resumeToken: _, userId: __, ...search } = router.getSearchParameters(); + router.navigate( + { + pathname: router.getLocationPathname(), + search, + }, + { replace: true }, + ); + }; + const handleLogin = async () => { const { resumeToken } = router.getSearchParameters(); if (!resumeToken) { return; } try { await loginWithToken(resumeToken); const routeName = router.getRouteName(); if (!routeName) { router.navigate('/home'); } - const { resumeToken: _, userId: __, ...search } = router.getSearchParameters(); - - router.navigate( - { - pathname: router.getLocationPathname(), - search, - }, - { replace: true }, - ); + clearSearchParams(); } catch (error) { console.error('Failed to login with token', error); Accounts._unstoreLoginToken(); - const { resumeToken: _, userId: __, ...search } = router.getSearchParameters(); - router.navigate( - { - pathname: router.getLocationPathname(), - search, - }, - { replace: true }, - ); + clearSearchParams(); } };
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/meteor/client/startup/startup.ts`:
- Around line 29-50: The catch block around synchronizeUserData currently clears
auth and calls removeLocalUserData() + Meteor.logout() for any error; instead,
inspect the caught error for authentication-specific indicators (e.g.,
error.error === 'unauthorized', error.code === 401, or other auth flags returned
by sdk.stream().ready()/sdk.rest.get('/v1/me')) and only perform
removeLocalUserData() and Meteor.logout() when an auth failure is detected; for
non-auth errors, log the error and allow retries without clearing local state.
Ensure this logic is implemented in the same catch handling surrounding
synchronizeUserData so only genuine auth failures trigger logout.
|
Updated startup error handling to only clear auth state and log out on authentication-specific failures. Non-auth errors are now logged and allowed to retry without clearing local state. Also refactored auth error detection into a small helper for clarity. |
|
@MartinSchoeler @lucas-a-pelegrino could you please take a look when you have time? Thanks! |
Fix: Clear Invalid Auth Tokens to Prevent Registration Page Redirect Loop
Closes #38467
Problem
Users with stale or invalid authentication tokens (e.g., after a MongoDB restore, workspace ID change, or token corruption) could get stuck in a redirect loop where they were continuously sent to the Registration/Setup Wizard instead of the login page.
The sequence was:
userId === nulluseRedirectToSetupWizardredirected to/setup-wizardwhen!userId && setupWizardState === 'pending'Root Cause
The useLoginViaQuery hook attempted to resume login using an invalid token but did not clear the token when login failed. This left the application in an inconsistent state: logged out but still holding an auth token that kept triggering resume attempts and setup redirects.
Solution
This PR ensures that invalid authentication tokens are properly cleared when a token-based login attempt fails.
Changes
1. client/views/root/hooks/useLoginViaQuery.ts
2. client/startup/startup.ts
Wrapped synchronizeUserData() in a try/catch block. If user data synchronization fails (commonly due to invalid auth state):
This is defensive cleanup and does not change normal startup behavior.
Impact
Files Changed
Related Issues
Summary by CodeRabbit