From 1b5b5dfa1b98e8ff165a21b20fbb8d3bf3b77aa8 Mon Sep 17 00:00:00 2001
From: Dave Liu <7david12liu@gmail.com>
Date: Sun, 12 Jul 2026 13:30:41 -0700
Subject: [PATCH] fix: return users to protected route after login (#96)

---
 src/pages/account/Account.tsx      |   9 +-
 src/pages/admin/AdminGuard.tsx     |   9 +-
 src/pages/login/LoginForm.jsx      |   8 +-
 src/pages/login/LoginForm.test.jsx | 191 +++++++++++++++++++++++++++++
 src/pages/login/loginReturnPath.ts |  55 +++++++++
 5 files changed, 269 insertions(+), 3 deletions(-)
 create mode 100644 src/pages/login/LoginForm.test.jsx
 create mode 100644 src/pages/login/loginReturnPath.ts

diff --git a/src/pages/account/Account.tsx b/src/pages/account/Account.tsx
index 2168141..cde9795 100644
--- a/src/pages/account/Account.tsx
+++ b/src/pages/account/Account.tsx
@@ -13,6 +13,7 @@ import {
 } from '../../services/account/accountService';
 import { formatEventDate, formatPrice } from '../../services/events/eventsService';
 import StravaSection from './StravaSection';
+import { getLocationReturnPath } from '../login/loginReturnPath';
 
 function roleLabel(role: string) {
   if (role === 'admin') return 'Admin';
@@ -347,7 +348,13 @@ function Account() {
 
   if (isLoading) return <div className="container mx-auto p-6">Loading...</div>;
   if (!isAuthenticated || !user) {
-    return <Navigate to="/login" state={{ from: location.pathname }} replace />;
+    return (
+      <Navigate
+        to="/login"
+        state={{ from: getLocationReturnPath(location) }}
+        replace
+      />
+    );
   }
   return <AccountContent user={user} />;
 }
diff --git a/src/pages/admin/AdminGuard.tsx b/src/pages/admin/AdminGuard.tsx
index 1581ea5..68115ea 100644
--- a/src/pages/admin/AdminGuard.tsx
+++ b/src/pages/admin/AdminGuard.tsx
@@ -1,6 +1,7 @@
 import React from 'react';
 import { Navigate, useLocation } from 'react-router-dom';
 import { useAuth } from '../../services/hooks/useAuth';
+import { getLocationReturnPath } from '../login/loginReturnPath';
 
 interface AdminGuardProps {
   children: React.ReactNode;
@@ -14,7 +15,13 @@ function AdminGuard({ children }: AdminGuardProps) {
     return <div className="container mx-auto p-6">Loading...</div>;
   }
   if (!isAuthenticated) {
-    return <Navigate to="/login" state={{ from: location.pathname }} replace />;
+    return (
+      <Navigate
+        to="/login"
+        state={{ from: getLocationReturnPath(location) }}
+        replace
+      />
+    );
   }
   if (!isAdmin) {
     return (
diff --git a/src/pages/login/LoginForm.jsx b/src/pages/login/LoginForm.jsx
index 3cd00da..c19758b 100644
--- a/src/pages/login/LoginForm.jsx
+++ b/src/pages/login/LoginForm.jsx
@@ -1,9 +1,12 @@
 import React, { useState } from 'react';
-import { Link } from 'react-router-dom';
+import {
+  Link, useLocation, useNavigate,
+} from 'react-router-dom';
 import HeaderImage from '../../images/activities/header_bg_1.jpg';
 import Header from '../../components/Header';
 import { useServiceLocator } from '../../services/ServiceLocatorContext';
 import SEO from '../../components/SEO';
+import { getSafeLoginReturnPath } from './loginReturnPath';
 
 function LoginForm() {
   const [email, setEmail] = useState('');
@@ -14,6 +17,8 @@ function LoginForm() {
   const [currentUser, setCurrentUser] = useState(null);
   const [isRegistering, setIsRegistering] = useState(false);
   const [resetSent, setResetSent] = useState(false);
+  const location = useLocation();
+  const navigate = useNavigate();
 
   const handleSubmit = async (e) => {
     e.preventDefault();
@@ -34,6 +39,7 @@ function LoginForm() {
       } else {
         const credential = await identityService.signIn(email, password);
         setCurrentUser(credential.user);
+        navigate(getSafeLoginReturnPath(location.state?.from), { replace: true });
       }
     } catch (loginError) {
       setError('Failed to authenticate. Please check your credentials and try again.');
diff --git a/src/pages/login/LoginForm.test.jsx b/src/pages/login/LoginForm.test.jsx
new file mode 100644
index 0000000..56d0abf
--- /dev/null
+++ b/src/pages/login/LoginForm.test.jsx
@@ -0,0 +1,191 @@
+/* eslint-env jest */
+import React from 'react';
+import PropTypes from 'prop-types';
+import {
+  MemoryRouter, Route, Routes, useLocation,
+} from 'react-router-dom';
+import { fireEvent, render, screen } from '@testing-library/react';
+import LoginForm from './LoginForm';
+import AdminGuard from '../admin/AdminGuard';
+import { useServiceLocator } from '../../services/ServiceLocatorContext';
+import { useAuth } from '../../services/hooks/useAuth';
+
+jest.mock('../../services/ServiceLocatorContext', () => ({
+  useServiceLocator: jest.fn(),
+}));
+
+jest.mock('../../services/hooks/useAuth', () => ({
+  useAuth: jest.fn(),
+}));
+
+jest.mock('../../components/Header', () => function Header() {
+  return null;
+});
+
+jest.mock('../../components/SEO', () => function SEO() {
+  return null;
+});
+
+function LocationProbe({ label }) {
+  const location = useLocation();
+  return (
+    <div>
+      {label}
+      {' '}
+      <span data-testid="location">
+        {location.pathname}
+        {location.search}
+        {location.hash}
+      </span>
+    </div>
+  );
+}
+
+LocationProbe.propTypes = {
+  label: PropTypes.string.isRequired,
+};
+
+function renderLogin(initialEntry = '/login') {
+  return render(
+    <MemoryRouter initialEntries={[initialEntry]}>
+      <Routes>
+        <Route path="/login" element={<LoginForm />} />
+        <Route path="/account" element={<LocationProbe label="Account destination" />} />
+        <Route path="/discounts" element={<LocationProbe label="Discounts destination" />} />
+        <Route
+          path="/admin/events"
+          element={(
+            <AdminGuard>
+              <LocationProbe label="Admin destination" />
+            </AdminGuard>
+          )}
+        />
+      </Routes>
+    </MemoryRouter>,
+  );
+}
+
+function submitCredentials() {
+  fireEvent.change(screen.getByLabelText('Email address'), {
+    target: { value: 'member@example.com' },
+  });
+  fireEvent.change(screen.getByLabelText('Password'), {
+    target: { value: 'correct horse battery staple' },
+  });
+  fireEvent.click(screen.getByRole('button', { name: 'Sign in' }));
+}
+
+describe('LoginForm return navigation', () => {
+  const signIn = jest.fn();
+  const register = jest.fn();
+  const sendPasswordReset = jest.fn();
+
+  beforeEach(() => {
+    jest.clearAllMocks();
+    signIn.mockResolvedValue({ user: { email: 'member@example.com' } });
+    register.mockResolvedValue({ user: { email: 'member@example.com' } });
+    useServiceLocator.mockReturnValue({
+      isReady: true,
+      services: {
+        identityService: { signIn, register, sendPasswordReset },
+      },
+    });
+    useAuth.mockReturnValue({
+      isLoading: false,
+      isAuthenticated: false,
+      isAdmin: false,
+    });
+  });
+
+  test('round trips query and hash through an actual protected-route redirect', async () => {
+    signIn.mockImplementation(async () => {
+      useAuth.mockReturnValue({
+        isLoading: false,
+        isAuthenticated: true,
+        isAdmin: true,
+      });
+      return { user: { email: 'member@example.com' } };
+    });
+    renderLogin('/admin/events?view=drafts#pending');
+
+    expect(await screen.findByRole('heading', { name: 'Sign in' })).toBeInTheDocument();
+    submitCredentials();
+
+    expect(await screen.findByText('Admin destination')).toBeInTheDocument();
+    expect(screen.getByTestId('location'))
+      .toHaveTextContent('/admin/events?view=drafts#pending');
+  });
+
+  test('returns to the requested safe internal route after sign-in', async () => {
+    renderLogin({
+      pathname: '/login',
+      state: { from: '/discounts?kind=race#active' },
+    });
+
+    submitCredentials();
+
+    expect(await screen.findByText('Discounts destination')).toBeInTheDocument();
+    expect(screen.getByTestId('location')).toHaveTextContent('/discounts?kind=race#active');
+  });
+
+  test('defaults to the account page after sign-in', async () => {
+    renderLogin();
+
+    submitCredentials();
+
+    expect(await screen.findByText('Account destination')).toBeInTheDocument();
+    expect(screen.getByTestId('location')).toHaveTextContent('/account');
+  });
+
+  test.each([
+    '//evil.example/path',
+    'https://evil.example/path',
+    '/\\evil.example/path',
+    '/%2F%2Fevil.example/path',
+    '/.//evil.example/path',
+    '/a/..//evil.example/path',
+    '/%2e%2e//evil.example/path',
+    '/safe/%2e%2e/%2F%2Fevil.example/path',
+    '/discounts%00',
+    '/discounts%',
+  ])('rejects unsafe return target %s', async (from) => {
+    renderLogin({ pathname: '/login', state: { from } });
+
+    submitCredentials();
+
+    expect(await screen.findByText('Account destination')).toBeInTheDocument();
+    expect(screen.getByTestId('location')).toHaveTextContent('/account');
+  });
+
+  test('stays on login and shows a generic error when sign-in fails', async () => {
+    signIn.mockRejectedValue(new Error('provider detail'));
+    renderLogin({ pathname: '/login', state: { from: '/discounts' } });
+
+    submitCredentials();
+
+    expect(await screen.findByText(
+      'Failed to authenticate. Please check your credentials and try again.',
+    )).toBeInTheDocument();
+    expect(screen.queryByText('Discounts destination')).not.toBeInTheDocument();
+  });
+
+  test('keeps the registration confirmation on the login page', async () => {
+    renderLogin({ pathname: '/login', state: { from: '/discounts' } });
+    fireEvent.click(screen.getByRole('button', { name: 'New here? Register' }));
+    fireEvent.change(screen.getByLabelText('Email address'), {
+      target: { value: 'member@example.com' },
+    });
+    fireEvent.change(screen.getByLabelText('Password'), {
+      target: { value: 'correct horse battery staple' },
+    });
+    fireEvent.click(screen.getByRole('button', { name: 'Create account' }));
+
+    expect(await screen.findByText('Check your inbox for a verification email.'))
+      .toBeInTheDocument();
+    expect(screen.queryByText('Discounts destination')).not.toBeInTheDocument();
+    expect(register).toHaveBeenCalledWith(
+      'member@example.com',
+      'correct horse battery staple',
+    );
+  });
+});
diff --git a/src/pages/login/loginReturnPath.ts b/src/pages/login/loginReturnPath.ts
new file mode 100644
index 0000000..5d61cff
--- /dev/null
+++ b/src/pages/login/loginReturnPath.ts
@@ -0,0 +1,55 @@
+const DEFAULT_RETURN_PATH = '/account';
+
+interface RouterLocation {
+  pathname: string;
+  search: string;
+  hash: string;
+}
+
+export function getLocationReturnPath(location: RouterLocation): string {
+  return `${location.pathname}${location.search}${location.hash}`;
+}
+
+function hasControlCharacter(value: string): boolean {
+  return Array.from(value).some((character) => {
+    const codePoint = character.codePointAt(0) as number;
+    return codePoint < 0x20 || codePoint === 0x7F;
+  });
+}
+
+function hasSingleLeadingSlash(pathname: string): boolean {
+  return pathname.startsWith('/') && !pathname.startsWith('//');
+}
+
+export function getSafeLoginReturnPath(value: unknown): string {
+  if (typeof value !== 'string' || !hasSingleLeadingSlash(value)) {
+    return DEFAULT_RETURN_PATH;
+  }
+
+  try {
+    const decodedValue = decodeURIComponent(value);
+    if (
+      !hasSingleLeadingSlash(decodedValue)
+      || decodedValue.includes('\\')
+      || hasControlCharacter(decodedValue)
+    ) {
+      return DEFAULT_RETURN_PATH;
+    }
+
+    const { origin } = window.location;
+    const returnUrl = new URL(value, origin);
+    const decodedUrl = new URL(decodedValue, origin);
+    if (
+      returnUrl.origin !== origin
+      || decodedUrl.origin !== origin
+      || !hasSingleLeadingSlash(returnUrl.pathname)
+      || !hasSingleLeadingSlash(decodedUrl.pathname)
+    ) {
+      return DEFAULT_RETURN_PATH;
+    }
+
+    return `${returnUrl.pathname}${returnUrl.search}${returnUrl.hash}`;
+  } catch {
+    return DEFAULT_RETURN_PATH;
+  }
+}