feat: remember original URL and redirect after login#18308
feat: remember original URL and redirect after login#18308FelixMalfait merged 20 commits intomainfrom
Conversation
Implement a return-to-path mechanism that preserves the user's intended destination across authentication flows, including cross-domain redirects (e.g. app.twenty.com -> workspace.twenty.com) and magic link flows. Uses a layered persistence strategy: - Jotai atom for in-memory SPA navigation - sessionStorage with TTL for tab-switch resilience (magic links) - URL query parameter for cross-domain propagation Includes path validation to prevent open redirects, automatic cleanup after successful login, and comprehensive test coverage. Made-with: Cursor
There was a problem hiding this comment.
3 issues found across 13 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/twenty-front/src/modules/auth/hooks/useReturnToPath.ts">
<violation number="1" location="packages/twenty-front/src/modules/auth/hooks/useReturnToPath.ts:35">
P2: Validate the sessionStorage return-to-path before returning it, otherwise a tampered entry can bypass the path whitelist/exclusion logic.</violation>
</file>
<file name="packages/twenty-front/src/modules/app/hooks/useInitializeQueryParamState.ts">
<violation number="1" location="packages/twenty-front/src/modules/app/hooks/useInitializeQueryParamState.ts:49">
P2: decodeURIComponent can throw on malformed query values, which will abort initializeQueryParamState and prevent other handlers from running. Wrap decoding in a try/catch (or guard) like the billingCheckoutSession handler to avoid breaking initialization on bad URLs.</violation>
</file>
<file name="packages/twenty-front/src/modules/auth/utils/isValidReturnToPath.ts">
<violation number="1" location="packages/twenty-front/src/modules/auth/utils/isValidReturnToPath.ts:20">
P2: Reject protocol-relative paths (`//...`) to avoid treating external URLs as valid return paths.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
packages/twenty-front/src/modules/auth/hooks/useReturnToPath.ts
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/modules/app/hooks/useInitializeQueryParamState.ts
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/modules/auth/utils/isValidReturnToPath.ts
Outdated
Show resolved
Hide resolved
Greptile SummaryImplements a layered return-to-path mechanism using Jotai atoms, sessionStorage with TTL, and URL query parameters to preserve user navigation across authentication flows. Key Changes:
Issues Found:
Confidence Score: 2/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User visits protected page while logged out] --> B{PageChangeEffect detects unauthenticated}
B --> C[Save current path to returnToPathState]
C --> D[Write to sessionStorage with TTL]
D --> E[Navigate to login page]
E --> F[User completes authentication]
F --> G{usePageChangeEffectNavigateLocation}
G --> H[Read returnToPath from state]
H --> I{returnToPath exists?}
I -->|Yes| J[Navigate to saved path]
I -->|No| K[Navigate to defaultHomePagePath]
J --> L{User on protected page + logged in?}
L -->|Yes| M[Clear returnToPath from state and storage]
N[Alternative: Session expires] --> O[GraphQL request fails with UNAUTHENTICATED]
O --> P[useApolloFactory saves current path]
P --> C
Q[Query param: ?returnToPath=/path] --> R[useInitializeQueryParamState]
R --> S[Decode and validate path]
S --> T[Store in state and sessionStorage]
U[Cross-domain redirect] --> V[WorkspaceProviderEffect]
V --> W[Preserve pathname and search params]
W --> X[Include returnToPath in URL]
Last reviewed commit: c29ee5e |
| if (!path.startsWith('/')) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Protocol-relative URLs like //evil.com pass this check and could enable open redirects.
| if (!path.startsWith('/')) { | |
| return false; | |
| } | |
| if (!path.startsWith('/') || path.startsWith('//')) { | |
| return false; | |
| } |
| returnToPath: (value: string) => { | ||
| const decodedValue = decodeURIComponent(value); | ||
|
|
||
| if (decodedValue.startsWith('/') && decodedValue !== '/') { | ||
| store.set(returnToPathState.atom, decodedValue); | ||
| writeReturnToPathToSessionStorage(decodedValue); | ||
| } | ||
| }, |
There was a problem hiding this comment.
Inconsistent validation - should use isValidReturnToPath instead of custom checks to ensure protocol-relative URLs and excluded paths are properly rejected.
| returnToPath: (value: string) => { | |
| const decodedValue = decodeURIComponent(value); | |
| if (decodedValue.startsWith('/') && decodedValue !== '/') { | |
| store.set(returnToPathState.atom, decodedValue); | |
| writeReturnToPathToSessionStorage(decodedValue); | |
| } | |
| }, | |
| returnToPath: (value: string) => { | |
| const decodedValue = decodeURIComponent(value); | |
| if (isValidReturnToPath(decodedValue)) { | |
| store.set(returnToPathState.atom, decodedValue); | |
| writeReturnToPathToSessionStorage(decodedValue); | |
| } | |
| }, |
| import { useCallback } from 'react'; | ||
|
|
||
| import { billingCheckoutSessionState } from '@/auth/states/billingCheckoutSessionState'; | ||
| import { returnToPathState } from '@/auth/states/returnToPathState'; | ||
| import { type BillingCheckoutSession } from '@/auth/types/billingCheckoutSession.type'; | ||
| import { writeReturnToPathToSessionStorage } from '@/auth/utils/returnToPathSessionStorage'; | ||
| import { BILLING_CHECKOUT_SESSION_DEFAULT_VALUE } from '@/billing/constants/BillingCheckoutSessionDefaultValue'; | ||
| import deepEqual from 'deep-equal'; | ||
| import { useStore } from 'jotai'; |
There was a problem hiding this comment.
Missing import for isValidReturnToPath needed for consistent validation.
| import { useCallback } from 'react'; | |
| import { billingCheckoutSessionState } from '@/auth/states/billingCheckoutSessionState'; | |
| import { returnToPathState } from '@/auth/states/returnToPathState'; | |
| import { type BillingCheckoutSession } from '@/auth/types/billingCheckoutSession.type'; | |
| import { writeReturnToPathToSessionStorage } from '@/auth/utils/returnToPathSessionStorage'; | |
| import { BILLING_CHECKOUT_SESSION_DEFAULT_VALUE } from '@/billing/constants/BillingCheckoutSessionDefaultValue'; | |
| import deepEqual from 'deep-equal'; | |
| import { useStore } from 'jotai'; | |
| import { useCallback } from 'react'; | |
| import { billingCheckoutSessionState } from '@/auth/states/billingCheckoutSessionState'; | |
| import { returnToPathState } from '@/auth/states/returnToPathState'; | |
| import { type BillingCheckoutSession } from '@/auth/types/billingCheckoutSession.type'; | |
| import { isValidReturnToPath } from '@/auth/utils/isValidReturnToPath'; | |
| import { writeReturnToPathToSessionStorage } from '@/auth/utils/returnToPathSessionStorage'; | |
| import { BILLING_CHECKOUT_SESSION_DEFAULT_VALUE } from '@/billing/constants/BillingCheckoutSessionDefaultValue'; | |
| import deepEqual from 'deep-equal'; | |
| import { useStore } from 'jotai'; |
- Block protocol-relative URLs (//evil.com) in isValidReturnToPath - Validate sessionStorage values through isValidReturnToPath before use - Wrap decodeURIComponent in try/catch to handle malformed URIs - Use shared isValidReturnToPath in useInitializeQueryParamState Made-with: Cursor
Extract ONGOING_USER_CREATION_PATHS and ONBOARDING_PATHS as shared constants from usePageChangeEffectNavigateLocation, and compose AUTH_AND_ONBOARDING_PATHS from them in PageChangeEffect instead of maintaining a third redundant list. Made-with: Cursor
One export per file: extract ONGOING_USER_CREATION_PATHS and ONBOARDING_PATHS into dedicated constant files under auth/constants. Made-with: Cursor
|
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:38096 This environment will automatically shut down after 5 hours. |
- Include location.hash in saved return-to-path so anchor links survive the login redirect - Derive EXCLUDED_PATH_PREFIXES from ONGOING_USER_CREATION_PATHS and ONBOARDING_PATHS instead of maintaining a separate hardcoded list Made-with: Cursor
| ) { | ||
| redirectToWorkspaceDomain( | ||
| getWorkspaceUrl(getPublicWorkspaceData.workspaceUrls), | ||
| window.location.pathname, | ||
| getCurrentSearchParams(), | ||
| ); | ||
| } | ||
| }, [ |
There was a problem hiding this comment.
Bug: The location.hash is not preserved during cross-domain workspace redirects in WorkspaceProviderEffect.tsx, breaking anchor links, which contradicts the PR's stated goal.
Severity: MEDIUM
Suggested Fix
Modify the calls to redirectToWorkspaceDomain in WorkspaceProviderEffect.tsx. Ensure the location.hash is included when constructing the redirect URL, similar to the implementation in useApolloFactory.ts and PageChangeEffect.tsx. The path should be constructed using window.location.pathname, getCurrentSearchParams(), and window.location.hash.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location:
packages/twenty-front/src/modules/workspace/components/WorkspaceProviderEffect.tsx#L59-L66
Potential issue: In `WorkspaceProviderEffect.tsx`, when a user in a multi-workspace
setup is redirected to their specific workspace domain, the URL's hash fragment is lost.
This happens because the call to `redirectToWorkspaceDomain` uses
`getCurrentSearchParams()`, which omits `location.hash`. This behavior is inconsistent
with other parts of the application, such as `PageChangeEffect.tsx`, where the hash is
explicitly preserved. As a result, any anchor links in the original URL will not work
after the redirect, which contradicts the PR's stated intent to "include location.hash
in saved return-to-path so anchor links survive the login redirect."
Did we get this right? 👍 / 👎 to inform future reviews.
packages/twenty-front/src/hooks/usePageChangeEffectNavigateLocation.ts
Outdated
Show resolved
Hide resolved
Use window.location instead of React's location hook in PageChangeEffect to avoid adding location.pathname/search/hash as effect dependencies. This prevents the effect from re-firing on every route change, which could cause timing issues for other E2E tests. Also add networkidle waits in return-to-path E2E tests for CI stability. Made-with: Cursor
In multi-workspace CI, the page may redirect across origins (localhost → apple.localhost), making sessionStorage checks unreliable. Focus on verifying end-to-end redirect behavior instead. Made-with: Cursor
networkidle hangs when SSE/WebSocket connections are active. domcontentloaded is sufficient since subsequent actions already wait for specific elements or URLs. Made-with: Cursor
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/twenty-e2e-testing/tests/authentication/return-to-path.spec.ts">
<violation number="1" location="packages/twenty-e2e-testing/tests/authentication/return-to-path.spec.ts:23">
P2: `locator.isVisible()` returns immediately without waiting for the element to appear, so this can skip workspace selection when the button renders slightly later and make the login helper flaky. Prefer waiting for visibility with a timeout (or keep the previous waitFor + try/catch).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
packages/twenty-e2e-testing/tests/authentication/return-to-path.spec.ts
Outdated
Show resolved
Hide resolved
…/twenty into feat/return-to-path-after-login
| } | ||
|
|
||
| setReturnToPath(path); | ||
| writeReturnToPathToSessionStorage(path); |
There was a problem hiding this comment.
this should be handled by our atom api (we arelady write to localStorage and cookies, should be the same here). Btw, why not writing to localStorage here?
| import { RETURN_TO_PATH_TTL_MS } from '@/auth/constants/ReturnToPathTtl'; | ||
| import { isReturnToPathSessionStorageEntry } from '@/auth/utils/isReturnToPathSessionStorageEntry'; | ||
|
|
||
| export const readReturnToPathFromSessionStorage = (): string | null => { |
There was a problem hiding this comment.
I wonder if all this could be avoid with proper atom tooling
| AppPath.ResetPassword, | ||
| ].map(extractPathPrefix); | ||
|
|
||
| export const isValidReturnToPath = (path: string): boolean => { |
There was a problem hiding this comment.
looks a bit "duplicated" with isReturnToPathSessionStorageEntry.ts. Not checking the same but this one is more accurate
…rect After a cross-domain workspace redirect, the Jotai atom and sessionStorage are empty on the new origin. The returnToPath query parameter was only being read inside a useEffect (via initializeQueryParamState), which runs after the render that computes the navigate location — causing the return path to be missed. Add a synchronous fallback that reads returnToPath directly from the URL search params during render, so the first navigation goes to the correct destination. Made-with: Cursor
The Jotai atom already persists across SPA navigation (like Recoil). The only case where state is lost is cross-domain redirects, which is already handled by URL search param propagation via buildSearchParamsFromUrlSyncedStates + initializeQueryParamState. Delete 6 files: writeReturnToPathToSessionStorage, readReturnToPathFromSessionStorage, clearReturnToPathFromSessionStorage, isReturnToPathSessionStorageEntry, ReturnToPathSessionKey, ReturnToPathTtl. Simplify useReturnToPath to pure atom get/set. Made-with: Cursor
The /settings/accounts page has API dependencies that can delay the full load event in CI. Switch from the default 'load' to 'commit' (URL changed + response received) and increase timeout to 30s. Made-with: Cursor
|
Hey @FelixMalfait! After you've done the QA of your Pull Request, you can mark it as done here. Thank you! |
## Summary - Implement a return-to-path mechanism that preserves the user's intended destination across authentication flows (login, magic link, cross-domain redirects) - Uses layered persistence: Jotai atom (in-memory), sessionStorage with TTL (tab-switch resilience), URL query parameter (cross-domain propagation) - Includes path validation to prevent open redirects, automatic cleanup after successful login, and comprehensive test coverage - Replaces the unused `previousUrlState` with a robust `returnToPathState` system ## Test plan - [ ] Visit a deep link (e.g. `/objects/tasks`) while logged out — should redirect to login, then back to `/objects/tasks` after logging in - [ ] Visit an OAuth authorize link while logged out — should redirect to login, then to the authorize page - [ ] Test magic link flow: click sign-in link that opens new tab — should still redirect to original destination - [ ] Test cross-domain: visit `app.twenty.com/objects/tasks` — should preserve path through workspace domain redirect - [ ] Verify auth/onboarding paths are excluded from being saved as return paths - [ ] Verify return-to-path is cleared after successful navigation - [ ] All 215 existing `usePageChangeEffectNavigateLocation` tests pass Made with [Cursor](https://cursor.com) (cherry picked from commit 6351c6c)
Summary
previousUrlStatewith a robustreturnToPathStatesystemTest plan
/objects/tasks) while logged out — should redirect to login, then back to/objects/tasksafter logging inapp.twenty.com/objects/tasks— should preserve path through workspace domain redirectusePageChangeEffectNavigateLocationtests passMade with Cursor