diff --git a/src/features/auth/callback.tsx b/src/features/auth/callback.tsx index addb4a2..55b7921 100644 --- a/src/features/auth/callback.tsx +++ b/src/features/auth/callback.tsx @@ -1,48 +1,51 @@ -import { useEffect, useMemo } from 'react'; -import { useNavigate, useSearchParams, Link } from 'react-router-dom'; +import { useEffect, useState } from 'react'; +import { useLocation, useNavigate, Link } from 'react-router-dom'; import { AuthLayout } from '@/components/layouts'; import { selectProjectBranding } from '@/features/project/slice'; import { Alert, Spinner } from '@/components/ui'; import { useAppDispatch, useAppSelector } from '@/app/hooks'; import { signedIn } from './slice'; import { ROUTES } from '@/routes'; +import { + CALLBACK_TOKEN_NAMES, + readCallback, + removeFromAddressBar, +} from './linkToken'; /** * OAuth / social sign-in return handler. * * Social and passkey-recovery sign-in are full-page navigations to the API, * which owns the provider redirect and the token exchange. The API sends the - * browser back here with either a bearer token or an error in the query - * string. We read it once, store it in the auth slice (which the SDK client - * and persist layer pick up), and bounce to the app. + * browser back here with either a bearer token or an error. We read it once, + * take the token out of the address bar at once, store it in the auth slice + * (which the SDK client and persist layer pick up), and bounce to the app. * * Accepted params (any of these token names): `token`, `bearerToken`, - * `access_token`; plus optional `userId`/`user_id`; or `error`. + * `access_token`; plus optional `userId`/`user_id`; or `error`. Each is read + * from the URL fragment first (`#token=…` — never sent to a server, so never + * in an access log), then from the query (links and redirects that still use + * `?token=…`). */ export function OAuthCallback() { - const [params] = useSearchParams(); + const location = useLocation(); const dispatch = useAppDispatch(); const branding = useAppSelector(selectProjectBranding); const navigate = useNavigate(); - // Parse the URL once (pure) — no setState needed. The token branch is the - // only side effect, handled in the effect below. - const result = useMemo< - { token: string; userId: string } | { error: string } - >(() => { - const token = - params.get('token') ?? - params.get('bearerToken') ?? - params.get('access_token'); - const userId = params.get('userId') ?? params.get('user_id') ?? ''; - const errParam = params.get('error') ?? params.get('error_description'); - if (errParam) return { error: errParam }; - if (token) return { token, userId }; - return { error: 'Sign-in did not complete. No token was returned.' }; - }, [params]); + // Parse the URL once, on the first render (pure: StrictMode may render + // twice). The address bar loses the token right after, so it is never + // read again. The token branch is the only side effect, handled below. + const [result] = useState(() => readCallback(location)); const error = 'error' in result ? result.error : null; + useEffect(() => { + removeFromAddressBar(location, window.history, ...CALLBACK_TOKEN_NAMES); + // Once, on mount: the location read above is the one to clean. + // eslint-disable-next-line react-hooks/exhaustive-deps + }, []); + useEffect(() => { if ('token' in result) { dispatch(signedIn({ token: result.token, userId: result.userId })); diff --git a/src/features/auth/linkToken.test.ts b/src/features/auth/linkToken.test.ts new file mode 100644 index 0000000..555c36c --- /dev/null +++ b/src/features/auth/linkToken.test.ts @@ -0,0 +1,142 @@ +// Tokens in emailed links: fragment first (never sent to a server, never in +// an access log), query fallback for links sent before, and the token leaves +// the address bar (gateway docs/tasks/link-tokens-in-fragment.md). +import { describe, expect, it } from 'vitest'; +import { + addressWithout, + linkParam, + readCallback, + removeFromAddressBar, +} from './linkToken'; + +describe('linkParam — fragment first, query fallback', () => { + it('reads a new link from the fragment (escaped "+" stays "+")', () => { + expect( + linkParam({ search: '', hash: '#token=Ab%2Bc%2Fd%3D' }, 'token'), + ).toBe('Ab+c/d='); + }); + + it('still reads a link sent before from the query', () => { + expect(linkParam({ search: '?token=old-1', hash: '' }, 'token')).toBe( + 'old-1', + ); + }); + + it('the fragment wins over the query', () => { + expect( + linkParam({ search: '?token=old-1', hash: '#token=new-1' }, 'token'), + ).toBe('new-1'); + }); + + it('several names: any name in the fragment beats every name in the query', () => { + expect( + linkParam( + { search: '?token=q', hash: '#access_token=f' }, + 'token', + 'access_token', + ), + ).toBe('f'); + }); + + it('empty when there is none (blank values count as none)', () => { + expect(linkParam({ search: '?token=', hash: '#token=%20' }, 'token')).toBe( + '', + ); + }); +}); + +describe('the token leaves the address bar', () => { + it('drops the fragment token and keeps the rest', () => { + expect( + addressWithout( + { + pathname: '/sign-in/reset/confirm', + search: '?lang=en', + hash: '#token=t1', + }, + 'token', + ), + ).toBe('/sign-in/reset/confirm?lang=en'); + }); + + it('drops a query token of an old link, keeps other params', () => { + expect( + addressWithout( + { + pathname: '/oauth/callback', + search: '?token=t1&userId=u1', + hash: '', + }, + 'token', + 'bearerToken', + 'access_token', + ), + ).toBe('/oauth/callback?userId=u1'); + }); + + it('changes nothing when there is no token', () => { + expect( + addressWithout({ pathname: '/x', search: '?a=1', hash: '' }, 'token'), + ).toBe(undefined); + }); + + it('replaces the history entry (no new one), keeping its state', () => { + const calls: unknown[][] = []; + const removed = removeFromAddressBar( + { pathname: '/sign-in/reset/confirm', search: '', hash: '#token=t1' }, + { + state: { key: 'k1' }, + replaceState: (...args: unknown[]) => void calls.push(args), + }, + 'token', + ); + expect(removed).toBe(true); + expect(calls).toEqual([[{ key: 'k1' }, '', '/sign-in/reset/confirm']]); + }); + + it('does not touch the history when there is nothing to remove', () => { + const calls: unknown[][] = []; + const removed = removeFromAddressBar( + { pathname: '/x', search: '', hash: '' }, + { + state: null, + replaceState: (...args: unknown[]) => void calls.push(args), + }, + 'token', + ); + expect(removed).toBe(false); + expect(calls).toEqual([]); + }); +}); + +describe('readCallback — /oauth/callback', () => { + it('new link: token and user id from the fragment', () => { + expect(readCallback({ search: '', hash: '#token=t1&userId=u1' })).toEqual({ + token: 't1', + userId: 'u1', + }); + }); + + it('old redirect: token from the query', () => { + expect( + readCallback({ search: '?access_token=t2&user_id=u2', hash: '' }), + ).toEqual({ + token: 't2', + userId: 'u2', + }); + }); + + it('an error wins', () => { + expect( + readCallback({ search: '?error=denied', hash: '#token=t1' }), + ).toEqual({ + error: 'denied', + }); + }); + + it('no token: says so', () => { + expect(readCallback({ search: '', hash: '' })).toEqual({ + error: 'Sign-in did not complete. No token was returned.', + }); + }); +}); diff --git a/src/features/auth/linkToken.ts b/src/features/auth/linkToken.ts new file mode 100644 index 0000000..0b84e66 --- /dev/null +++ b/src/features/auth/linkToken.ts @@ -0,0 +1,106 @@ +/** + * Secrets in emailed links and sign-in redirects. + * + * New links carry a one-time token in the URL fragment + * (`/sign-in/reset/confirm#token=…`, `/oauth/callback#token=…`). A browser + * never sends the fragment to a server, so no access log (the portal's web + * server, the ingress, a proxy) can write the token down (gateway + * docs/tasks/link-tokens-in-fragment.md). Links sent before carry the token + * in the query (`?token=…`) and keep working until they expire, so the + * fragment is read first and the query second. + * + * After reading, the page takes the token out of the address bar + * (`history.replaceState`): it is not kept in the browser history, shown on + * screen or copied with the page URL. + */ + +/** Where the page was opened: `window.location` or react-router's location. */ +export interface LinkAddress { + pathname?: string; + search: string; + hash: string; +} + +const fragmentParams = (hash: string): URLSearchParams => + new URLSearchParams(hash.replace(/^#/, '')); + +/** + * The first of `names` that has a value: every name in the fragment first, + * then every name in the query. Only the ends are trimmed. Empty string when + * there is none. + */ +export const linkParam = ( + { search, hash }: LinkAddress, + ...names: string[] +): string => { + for (const params of [fragmentParams(hash), new URLSearchParams(search)]) { + for (const name of names) { + const value = (params.get(name) ?? '').trim(); + if (value) return value; + } + } + return ''; +}; + +/** + * The address with `names` taken out of the query and the fragment, or + * undefined when none of them is there. Other parameters stay; an empty + * query or fragment is dropped completely. + */ +export const addressWithout = ( + { pathname = '', search, hash }: LinkAddress, + ...names: string[] +): string | undefined => { + const query = new URLSearchParams(search); + const fragment = fragmentParams(hash); + if (!names.some((name) => query.has(name) || fragment.has(name))) { + return undefined; + } + for (const name of names) { + query.delete(name); + fragment.delete(name); + } + const q = query.toString(); + const f = fragment.toString(); + return `${pathname}${q ? `?${q}` : ''}${f ? `#${f}` : ''}`; +}; + +/** The part of `window.history` the pages use. */ +export interface ReplaceableHistory { + state: unknown; + replaceState(data: unknown, unused: string, url?: string | null): void; +} + +/** + * Takes `names` out of the address bar. The history entry is replaced, not + * added; its state (react-router's key) is kept. Returns true when something + * was removed. + */ +export const removeFromAddressBar = ( + address: LinkAddress, + history: ReplaceableHistory, + ...names: string[] +): boolean => { + const stripped = addressWithout(address, ...names); + if (stripped === undefined) return false; + history.replaceState(history.state, '', stripped); + return true; +}; + +/** The parameter names that carry a sign-in secret on /oauth/callback. */ +export const CALLBACK_TOKEN_NAMES = ['token', 'bearerToken', 'access_token']; + +/** + * What the /oauth/callback URL says: a token (with an optional user id) or + * an error. Fragment first, then the query. + */ +export const readCallback = ( + address: LinkAddress, +): { token: string; userId: string } | { error: string } => { + const token = linkParam(address, ...CALLBACK_TOKEN_NAMES); + const userId = linkParam(address, 'userId', 'user_id'); + const errParam = linkParam(address, 'error', 'error_description'); + if (errParam) return { error: errParam }; + if (token) return { token, userId }; + return { error: 'Sign-in did not complete. No token was returned.' }; +}; diff --git a/src/features/auth/passwordReset.tsx b/src/features/auth/passwordReset.tsx index 75fb40e..2208c31 100644 --- a/src/features/auth/passwordReset.tsx +++ b/src/features/auth/passwordReset.tsx @@ -1,6 +1,6 @@ -import { useState } from 'react'; +import { useEffect, useState } from 'react'; import { Form } from 'react-final-form'; -import { Link, useSearchParams } from 'react-router-dom'; +import { Link, useLocation } from 'react-router-dom'; import { AuthLayout } from '@/components/layouts'; import { useAppSelector } from '@/app/hooks'; import { selectProjectBranding } from '@/features/project/slice'; @@ -16,6 +16,7 @@ import { useConfirmPasswordResetMutation, } from '@/services/norbix'; import { ROUTES } from '@/routes'; +import { linkParam, removeFromAddressBar } from './linkToken'; export function PasswordResetRequest() { const [request, { isLoading, isError }] = useRequestPasswordResetMutation(); @@ -75,9 +76,23 @@ export function PasswordResetRequest() { ); } +/** + * Behind the link in the password-reset email + * (`/sign-in/reset/confirm#token=…`; links sent before carry `?token=…`). + * The one-time token is read once — fragment first, then the query — and + * taken out of the address bar at once (see ./linkToken). It goes to the API + * only in the body of the confirm POST. + */ export function PasswordResetConfirm() { - const [params] = useSearchParams(); - const token = params.get('token') ?? ''; + const location = useLocation(); + // Read once, on the first render (pure: StrictMode may render twice). + const [token] = useState(() => linkParam(location, 'token')); + + useEffect(() => { + removeFromAddressBar(location, window.history, 'token'); + // Once, on mount: the location read above is the one to clean. + // eslint-disable-next-line react-hooks/exhaustive-deps + }, []); const [confirm, { isLoading, isSuccess, isError }] = useConfirmPasswordResetMutation(); const branding = useAppSelector(selectProjectBranding); diff --git a/src/features/auth/passwordResetConfirm.test.tsx b/src/features/auth/passwordResetConfirm.test.tsx new file mode 100644 index 0000000..8d00a1d --- /dev/null +++ b/src/features/auth/passwordResetConfirm.test.tsx @@ -0,0 +1,91 @@ +// @vitest-environment jsdom +// The password-reset link (/sign-in/reset/confirm#token=…): the page reads +// the one-time token from the fragment (query for links sent before), takes +// it out of the address bar at once and sends it only in the confirm POST +// body (gateway docs/tasks/link-tokens-in-fragment.md). +import { afterEach, describe, expect, it, vi } from 'vitest'; +import { + cleanup, + fireEvent, + render, + screen, + waitFor, +} from '@testing-library/react'; +import { combineReducers, configureStore } from '@reduxjs/toolkit'; +import { Provider } from 'react-redux'; +import { MemoryRouter } from 'react-router-dom'; +import auth from '@/features/auth/slice'; +import config from '@/config/slice'; +import project from '@/features/project/slice'; + +const confirm = vi.hoisted(() => ({ calls: [] as unknown[] })); + +vi.mock('@/services/norbix', () => ({ + useRequestPasswordResetMutation: () => [vi.fn(), {}], + useConfirmPasswordResetMutation: () => [ + (body: unknown) => { + confirm.calls.push(body); + return { unwrap: async () => ({}) }; + }, + { isLoading: false, isSuccess: false, isError: false }, + ], +})); + +const { PasswordResetConfirm } = await import('./passwordReset'); + +const open = (entry: string) => + render( + + + + + , + ); + +const submit = () => { + fireEvent.change(screen.getByLabelText('New password'), { + target: { value: 'Correct-Horse-9' }, + }); + fireEvent.click(screen.getByRole('button', { name: 'Update password' })); +}; + +afterEach(() => { + cleanup(); + confirm.calls = []; + vi.restoreAllMocks(); +}); + +describe('PasswordResetConfirm', () => { + it('new link: token from the fragment, address bar cleaned, token only in the POST body', async () => { + const replace = vi.spyOn(window.history, 'replaceState'); + open('/sign-in/reset/confirm#token=Ab%2Bc%3D'); + + expect(replace).toHaveBeenCalledTimes(1); + expect(replace.mock.calls[0][2]).toBe('/sign-in/reset/confirm'); + + submit(); + await waitFor(() => + expect(confirm.calls).toEqual([ + { token: 'Ab+c=', newPassword: 'Correct-Horse-9' }, + ]), + ); + }); + + it('link sent before: token from the query, query token removed', async () => { + const replace = vi.spyOn(window.history, 'replaceState'); + open('/sign-in/reset/confirm?token=old-1'); + + expect(replace.mock.calls[0][2]).toBe('/sign-in/reset/confirm'); + + submit(); + await waitFor(() => + expect(confirm.calls).toEqual([ + { token: 'old-1', newPassword: 'Correct-Horse-9' }, + ]), + ); + }); +});