From 86e2a96eb6bc89ba7fae2f62fc171a165e82ecea Mon Sep 17 00:00:00 2001 From: Maurice Date: Sat, 8 Aug 2026 22:53:47 +0200 Subject: [PATCH] fix(client): a deliberate logout should not book a return ticket Pressing "log out" clears isAuthenticated while the page you were on is still mounted, so ProtectedRoute re-renders for it and sends you to /login?redirect=. Log out from the dashboard and the next login goes to the dashboard - beating the startup destination every time after the first, which is exactly how it looked: worked once, never again. A session that ended on its own still gets the return ticket, because coming back to where you were is the whole point of that parameter. A deliberate sign-out is a fresh start and now gets none, so the startup destination decides. The Navbar already hinted at this with its unused noRedirect state; the flag lives in the store instead, since all seven call sites of logout() need it, not just that one. --- client/src/App.test.tsx | 27 ++++++++++++++++++++++++++- client/src/App.tsx | 5 +++++ client/src/store/authStore.ts | 16 +++++++++++++++- 3 files changed, 46 insertions(+), 2 deletions(-) diff --git a/client/src/App.test.tsx b/client/src/App.test.tsx index 91d3db58b..788dd1f03 100644 --- a/client/src/App.test.tsx +++ b/client/src/App.test.tsx @@ -1,6 +1,6 @@ import React from 'react' import { render, screen, waitFor, act } from '@testing-library/react' -import { MemoryRouter } from 'react-router' +import { MemoryRouter, useLocation } from 'react-router' import { describe, it, expect, beforeEach, vi } from 'vitest' import { http, HttpResponse } from 'msw' import { server } from '../tests/helpers/msw/server' @@ -30,10 +30,19 @@ vi.mock('./hooks/useInAppNotificationListener.ts', () => ({ // ── Helpers ──────────────────────────────────────────────────────────────────── +/** Reports the router's current location, so a test can assert where App sent it. */ +function LocationProbe() { + const loc = useLocation() + return {loc.pathname + loc.search} +} + +const currentPath = () => screen.getByTestId('loc').textContent + function renderApp(initialPath = '/') { return render( + ) } @@ -166,6 +175,22 @@ describe('ProtectedRoute — unauthenticated', () => { await waitFor(() => expect(screen.getByText('Login')).toBeInTheDocument()) }) + // A session that ran out should return you to the page you were on. Pressing + // "log out" should not: clearing isAuthenticated re-renders this route for the + // page still on screen, and the ?redirect= it stamped there beat the user's + // startup destination on every login after the first. + it('FE-COMP-APP-034: keeps the return ticket when the session merely ended', async () => { + seedAuth({ isAuthenticated: false, loggingOut: false }) + renderApp('/trips/1/files') + await waitFor(() => expect(currentPath()).toBe('/login?redirect=%2Ftrips%2F1%2Ffiles')) + }) + + it('FE-COMP-APP-035: drops it on a deliberate sign-out, so the startup destination decides', async () => { + seedAuth({ isAuthenticated: false, loggingOut: true }) + renderApp('/dashboard') + await waitFor(() => expect(currentPath()).toBe('/login')) + }) + it('FE-COMP-APP-005: /trips/42 redirects to /login when not authenticated', async () => { seedAuth({ isAuthenticated: false }) renderApp('/trips/42') diff --git a/client/src/App.tsx b/client/src/App.tsx index 701d6d6af..fdbf90e54 100644 --- a/client/src/App.tsx +++ b/client/src/App.tsx @@ -82,6 +82,7 @@ function ProtectedRoute({ children, adminRequired = false, addonId }: ProtectedR const user = useAuthStore((s) => s.user) const isLoading = useAuthStore((s) => s.isLoading) const appRequireMfa = useAuthStore((s) => s.appRequireMfa) + const loggingOut = useAuthStore((s) => s.loggingOut) const addonStore = useAddonStore() const { t } = useTranslation() const location = useLocation() @@ -99,6 +100,10 @@ function ProtectedRoute({ children, adminRequired = false, addonId }: ProtectedR } if (!isAuthenticated) { + // A session that ended on its own should come back to where it left off; a + // deliberate sign-out is a fresh start and gets no return ticket, so the + // startup destination decides where the next login lands. + if (loggingOut) return const redirectParam = encodeURIComponent(location.pathname + location.search + location.hash) return } diff --git a/client/src/store/authStore.ts b/client/src/store/authStore.ts index 48b161ebc..b0e8f9d42 100644 --- a/client/src/store/authStore.ts +++ b/client/src/store/authStore.ts @@ -33,6 +33,10 @@ interface AuthState { * outage doesn't render as a blank, error-free page that looks like lost data. * Transient, never persisted. #1283 */ authCheckFailed: boolean + /** The user pressed "log out" — as opposed to a session that simply ended. + * Read by ProtectedRoute: a deliberate sign-out should not leave a + * ?redirect= pointing back at the page they just left. Transient. */ + loggingOut: boolean error: string | null demoMode: boolean devMode: boolean @@ -95,6 +99,7 @@ export const useAuthStore = create()( isAuthenticated: false, isLoading: true, authCheckFailed: false, + loggingOut: false, error: null, demoMode: localStorage.getItem('demo_mode') === 'true', devMode: false, @@ -120,6 +125,7 @@ export const useAuthStore = create()( set({ user: data.user, isAuthenticated: true, + loggingOut: false, isLoading: false, error: null, }) @@ -145,6 +151,7 @@ export const useAuthStore = create()( set({ user: data.user, isAuthenticated: true, + loggingOut: false, isLoading: false, error: null, }) @@ -170,6 +177,7 @@ export const useAuthStore = create()( set({ user: data.user, isAuthenticated: true, + loggingOut: false, isLoading: false, error: null, }) @@ -188,7 +196,11 @@ export const useAuthStore = create()( logout: async () => { // 1. Gate first so any in-flight flush/syncAll bails before we wipe the DB. setAuthed(false) - set({ isAuthenticated: false }) + // Flagged in the same update that drops the session: clearing isAuthenticated + // re-renders ProtectedRoute for whatever page is still on screen, and without + // this it would stamp a ?redirect= back to it — which then beats the user's + // startup destination on the next login. + set({ isAuthenticated: false, loggingOut: true }) // 2. Stop background sync triggers (30s interval, WS pre-reconnect hook, listeners). unregisterSyncTriggers() // 3. Tear down the live connection. @@ -233,6 +245,7 @@ export const useAuthStore = create()( set({ user: data.user, isAuthenticated: true, + loggingOut: false, isLoading: false, authCheckFailed: false, }) @@ -336,6 +349,7 @@ export const useAuthStore = create()( set({ user: data.user, isAuthenticated: true, + loggingOut: false, isLoading: false, demoMode: true, error: null,