diff --git a/.changeset/fix-auth-homeserver-query-param.md b/.changeset/fix-auth-homeserver-query-param.md new file mode 100644 index 0000000000..293e70b73c --- /dev/null +++ b/.changeset/fix-auth-homeserver-query-param.md @@ -0,0 +1,5 @@ +--- +default: patch +--- + +Fixed login landing on an empty room named "login" when the homeserver is entered as a full URL. diff --git a/src/app/pages/Router.tsx b/src/app/pages/Router.tsx index 657d13da97..50a2ad4479 100644 --- a/src/app/pages/Router.tsx +++ b/src/app/pages/Router.tsx @@ -30,6 +30,9 @@ import { DIRECT_PATH, EXPLORE_PATH, HOME_PATH, + LEGACY_LOGIN_PATH, + LEGACY_REGISTER_PATH, + LEGACY_RESET_PASSWORD_PATH, LOGIN_PATH, INBOX_PATH, REGISTER_PATH, @@ -102,6 +105,7 @@ const PublicRooms = lazy(() => import('./client/explore').then((m) => ({ default: m.PublicRooms })) ); import { setAfterLoginRedirectPath } from './afterLoginRedirectPath'; +import { legacyAuthLoader } from './legacyAuthRedirect'; import { WelcomePage } from './client/WelcomePage'; import { SidebarNav } from './client/SidebarNav'; import { MobileFriendlySidebarNav, MobileFriendlyBottomNav } from './MobileFriendly'; @@ -201,6 +205,10 @@ export const createRouter = (clientConfig: ClientConfig, screenSize: ScreenSize) } /> + + + + { const session = getFirstSession(); diff --git a/src/app/pages/afterLoginRedirectPath.test.ts b/src/app/pages/afterLoginRedirectPath.test.ts new file mode 100644 index 0000000000..9763d81ba2 --- /dev/null +++ b/src/app/pages/afterLoginRedirectPath.test.ts @@ -0,0 +1,36 @@ +import { beforeEach, describe, expect, it } from 'vitest'; +import { + deleteAfterLoginRedirectPath, + getAfterLoginRedirectPath, + setAfterLoginRedirectPath, +} from './afterLoginRedirectPath'; + +describe('afterLoginRedirectPath', () => { + beforeEach(() => { + deleteAfterLoginRedirectPath(); + }); + + it('stores an in-app path', () => { + setAfterLoginRedirectPath('/home/room/%21abc'); + expect(getAfterLoginRedirectPath()).toBe('/home/room/%21abc'); + }); + + it('ignores auth paths', () => { + setAfterLoginRedirectPath('/login/matrix.org'); + setAfterLoginRedirectPath('/register/matrix.org'); + setAfterLoginRedirectPath('/reset-password/matrix.org'); + expect(getAfterLoginRedirectPath()).toBeUndefined(); + }); + + it('ignores the root path and off-site destinations', () => { + setAfterLoginRedirectPath('/'); + setAfterLoginRedirectPath('https://example.com/'); + setAfterLoginRedirectPath('//example.com/'); + expect(getAfterLoginRedirectPath()).toBeUndefined(); + }); + + it('discards an already stored auth path', () => { + localStorage.setItem('after_login_redirect_url', '/login/http%3A/localhost%3A18448'); + expect(getAfterLoginRedirectPath()).toBeUndefined(); + }); +}); diff --git a/src/app/pages/afterLoginRedirectPath.ts b/src/app/pages/afterLoginRedirectPath.ts index 60e09da334..c4e3fbe9dc 100644 --- a/src/app/pages/afterLoginRedirectPath.ts +++ b/src/app/pages/afterLoginRedirectPath.ts @@ -1,11 +1,25 @@ +import { trimLeadingSlash } from '$utils/common'; + const AFTER_LOGIN_REDIRECT_PATH_KEY = 'after_login_redirect_url'; +const AUTH_PATH_SEGMENTS = new Set(['login', 'register', 'reset-password']); + +// A malformed auth path such as "/login/http:/host:8008" falls through to a room route, so +// auth paths and off-site URLs are never kept as an after-login destination. +const isRedirectablePath = (path: string): boolean => { + if (!path.startsWith('/') || path.startsWith('//')) return false; + const firstSegment = trimLeadingSlash(path).split('/')[0] ?? ''; + return firstSegment !== '' && !AUTH_PATH_SEGMENTS.has(firstSegment); +}; + export const setAfterLoginRedirectPath = (url: string): void => { + if (!isRedirectablePath(url)) return; localStorage.setItem(AFTER_LOGIN_REDIRECT_PATH_KEY, url); }; export const getAfterLoginRedirectPath = (): string | undefined => { const url = localStorage.getItem(AFTER_LOGIN_REDIRECT_PATH_KEY); - return url ?? undefined; + if (!url || !isRedirectablePath(url)) return undefined; + return url; }; export const deleteAfterLoginRedirectPath = (): void => { localStorage.removeItem(AFTER_LOGIN_REDIRECT_PATH_KEY); diff --git a/src/app/pages/auth/AuthLayout.tsx b/src/app/pages/auth/AuthLayout.tsx index ef0b47ac68..154fd9e506 100644 --- a/src/app/pages/auth/AuthLayout.tsx +++ b/src/app/pages/auth/AuthLayout.tsx @@ -2,15 +2,7 @@ import type { ReactNode } from 'react'; import { useCallback, useEffect } from 'react'; import { Box, Chip, Header, IconButton, Scroll, Spinner, Text, color } from 'folds'; import { ArrowClockwiseIcon } from '@phosphor-icons/react'; -import { - Outlet, - generatePath, - matchPath, - useLocation, - useNavigate, - useParams, - useSearchParams, -} from 'react-router-dom'; +import { Outlet, matchPath, useLocation, useNavigate, useSearchParams } from 'react-router-dom'; import classNames from 'classnames'; import * as PatternsCss from '$styles/Patterns.css'; @@ -24,7 +16,7 @@ import { AuthFlowsLoader } from '$components/AuthFlowsLoader'; import { AuthFlowsProvider } from '$hooks/useAuthFlows'; import type { AuthFlows } from '$hooks/useAuthFlows'; import { AuthServerProvider } from '$hooks/useAuthServer'; -import { LOGIN_PATH, REGISTER_PATH, RESET_PASSWORD_PATH } from '$pages/paths'; +import { LOGIN_PATH, REGISTER_PATH, RESET_PASSWORD_PATH, SERVER_SEARCH_PARAM } from '$pages/paths'; import { getHomePath } from '$pages/pathUtils'; import { fetch } from '$utils/fetch'; import { sizedIcon } from '$components/icons/phosphor'; @@ -48,6 +40,16 @@ const currentAuthPath = (pathname: string): string => { return LOGIN_PATH; }; +const authPathWithServer = ( + pathname: string, + searchParams: URLSearchParams, + server: string +): string => { + const params = new URLSearchParams(searchParams); + params.set(SERVER_SEARCH_PARAM, server); + return `${currentAuthPath(pathname)}?${params}`; +}; + function AuthLayoutLoading({ message }: { message: string }) { return ( @@ -128,8 +130,8 @@ function AuthSpecVersionsContent({ export function AuthLayout() { const navigate = useNavigate(); const location = useLocation(); - const { server: urlEncodedServer } = useParams(); const [searchParams] = useSearchParams(); + const urlServer = searchParams.get(SERVER_SEARCH_PARAM) ?? undefined; const isAddingAccount = searchParams.get('addAccount') === '1'; @@ -138,8 +140,7 @@ export function AuthLayout() { const homeUrl = usePathWithOrigin(getHomePath()); const defaultServer = clientDefaultServer(clientConfig); - const decodedServer = urlEncodedServer && decodeURIComponent(urlEncodedServer); - let server: string = decodedServer ?? defaultServer; + let server: string = urlServer ?? defaultServer; if (!clientAllowedServer(clientConfig, server)) { server = defaultServer; @@ -160,16 +161,12 @@ export function AuthLayout() { if (server) discoverServer(server); }, [discoverServer, server]); - // if server is mismatched with path server, update path — preserve all search params + // if server is mismatched with url server, update url — preserve all search params useEffect(() => { - if (!urlEncodedServer || decodeURIComponent(urlEncodedServer) !== server) { - const basePath = generatePath(currentAuthPath(location.pathname), { - server: encodeURIComponent(server), - }); - const search = searchParams.toString(); - navigate(`${basePath}${search ? `?${search}` : ''}`, { replace: true }); + if (urlServer !== server) { + navigate(authPathWithServer(location.pathname, searchParams, server), { replace: true }); } - }, [urlEncodedServer, navigate, location, server, searchParams]); + }, [urlServer, navigate, location, server, searchParams]); const selectServer = useCallback( (newServer: string) => { @@ -178,11 +175,7 @@ export function AuthLayout() { discoverServer(server); return; } - const basePath = generatePath(currentAuthPath(location.pathname), { - server: encodeURIComponent(newServer), - }); - const search = searchParams.toString(); - navigate(`${basePath}${search ? `?${search}` : ''}`); + navigate(authPathWithServer(location.pathname, searchParams, newServer)); }, [navigate, location, discoveryState, server, discoverServer, searchParams] ); diff --git a/src/app/pages/legacyAuthRedirect.test.ts b/src/app/pages/legacyAuthRedirect.test.ts new file mode 100644 index 0000000000..24031bcee5 --- /dev/null +++ b/src/app/pages/legacyAuthRedirect.test.ts @@ -0,0 +1,58 @@ +import { describe, expect, it } from 'vitest'; +import { legacyAuthRedirectPath } from './legacyAuthRedirect'; +import { LOGIN_PATH, REGISTER_PATH } from './paths'; + +describe('legacyAuthRedirectPath', () => { + it('moves the server segment into the query string', () => { + expect( + legacyAuthRedirectPath(LOGIN_PATH, 'https://app.sable.moe/login/matrix.org', 'matrix.org') + ).toBe('/login?server=matrix.org'); + expect( + legacyAuthRedirectPath(REGISTER_PATH, 'https://app.sable.moe/register/sable.moe', 'sable.moe') + ).toBe('/register?server=sable.moe'); + }); + + it('keeps the callback params an sso or oidc redirect brings back', () => { + expect( + legacyAuthRedirectPath( + LOGIN_PATH, + 'https://app.sable.moe/login/matrix.org?loginToken=tok', + 'matrix.org' + ) + ).toBe('/login?loginToken=tok&server=matrix.org'); + expect( + legacyAuthRedirectPath( + LOGIN_PATH, + 'https://app.sable.moe/login/matrix.org?code=c1&state=s1', + 'matrix.org' + ) + ).toBe('/login?code=c1&state=s1&server=matrix.org'); + }); + + it('keeps a server url that survived as one segment', () => { + expect( + legacyAuthRedirectPath( + LOGIN_PATH, + 'https://app.sable.moe/login/http%3Alocalhost%3A18448', + 'http:localhost:18448' + ) + ).toBe('/login?server=http%3Alocalhost%3A18448'); + }); + + it('drops a segment split by an unescaped slash', () => { + expect( + legacyAuthRedirectPath( + LOGIN_PATH, + 'https://app.sable.moe/login/http%3A/localhost%3A18448', + 'http:/localhost:18448' + ) + ).toBe('/login'); + }); + + it('redirects a bare legacy path without inventing a server', () => { + expect(legacyAuthRedirectPath(LOGIN_PATH, 'https://app.sable.moe/login/', '')).toBe('/login'); + expect( + legacyAuthRedirectPath(LOGIN_PATH, 'https://app.sable.moe/login/?addAccount=1', undefined) + ).toBe('/login?addAccount=1'); + }); +}); diff --git a/src/app/pages/legacyAuthRedirect.ts b/src/app/pages/legacyAuthRedirect.ts new file mode 100644 index 0000000000..91091301f4 --- /dev/null +++ b/src/app/pages/legacyAuthRedirect.ts @@ -0,0 +1,22 @@ +import type { LoaderFunctionArgs } from 'react-router-dom'; +import { redirect } from 'react-router-dom'; +import { SERVER_SEARCH_PARAM } from './paths'; + +// Moves the homeserver from the old path segment to the query string. A segment holding a +// slash is dropped: it is the remains of an escaped slash the hosting rewrote, unrecoverable. +export const legacyAuthRedirectPath = ( + authPath: string, + requestUrl: string, + legacyServer?: string +): string => { + const url = new URL(requestUrl); + if (legacyServer && !legacyServer.includes('/')) { + url.searchParams.set(SERVER_SEARCH_PARAM, legacyServer); + } + return `${authPath}${url.search}`; +}; + +export const legacyAuthLoader = + (authPath: string) => + ({ params, request }: LoaderFunctionArgs) => + redirect(legacyAuthRedirectPath(authPath, request.url, params['*'])); diff --git a/src/app/pages/pathUtils.test.ts b/src/app/pages/pathUtils.test.ts index d72b192770..3bc8de1858 100644 --- a/src/app/pages/pathUtils.test.ts +++ b/src/app/pages/pathUtils.test.ts @@ -1,5 +1,41 @@ import { describe, expect, it } from 'vitest'; -import { getAppPathFromHref, getSettingsPath } from './pathUtils'; +import { + getAppPathFromHref, + getLoginPath, + getRegisterPath, + getSettingsPath, + withSearchParam, +} from './pathUtils'; + +describe('getLoginPath', () => { + it('omits the homeserver when there is none', () => { + expect(getLoginPath()).toBe('/login'); + }); + + it('carries the homeserver in the query string, never in a path segment', () => { + expect(getLoginPath('matrix.org')).toBe('/login?server=matrix.org'); + expect(getLoginPath('http://localhost:18448')).toBe( + '/login?server=http%3A%2F%2Flocalhost%3A18448' + ); + expect(getRegisterPath('https://example.com/matrix')).toBe( + '/register?server=https%3A%2F%2Fexample.com%2Fmatrix' + ); + }); +}); + +describe('withSearchParam', () => { + it('merges into an existing query string', () => { + expect(withSearchParam(getLoginPath('matrix.org'), { addAccount: '1' })).toBe( + '/login?server=matrix.org&addAccount=1' + ); + }); + + it('overwrites a param that is already set', () => { + expect(withSearchParam('/login?server=matrix.org', { server: 'sable.moe' })).toBe( + '/login?server=sable.moe' + ); + }); +}); describe('getSettingsPath', () => { it('returns the settings root path', () => { diff --git a/src/app/pages/pathUtils.ts b/src/app/pages/pathUtils.ts index d0ba1d2f35..7cc514914c 100644 --- a/src/app/pages/pathUtils.ts +++ b/src/app/pages/pathUtils.ts @@ -21,6 +21,7 @@ import { INBOX_PATH, REGISTER_PATH, RESET_PASSWORD_PATH, + SERVER_SEARCH_PARAM, SETTINGS_PATH, SPACE_LOBBY_PATH, SPACE_PATH, @@ -37,9 +38,11 @@ import { export const joinPathComponent = (path: Path): string => path.pathname + path.search + path.hash; export const withSearchParam = (path: string, searchParam: Record): string => { - const params = new URLSearchParams(searchParam); + const [pathname, existingSearch] = path.split('?'); + const params = new URLSearchParams(existingSearch); + Object.entries(searchParam).forEach(([name, value]) => params.set(name, value)); - return `${path}?${params}`; + return `${pathname}?${params}`; }; export const encodeSearchParamValueArray = (ids: string[]): string => ids.join(','); export const decodeSearchParamValueArray = (idsParam: string): string[] => idsParam.split(','); @@ -81,20 +84,17 @@ export const getAppPathFromHref = (baseUrl: string, href: string): string => { return pathname + search; }; -export const getLoginPath = (server?: string): string => { - const params = server ? { server: encodeURIComponent(server) } : undefined; - return generatePath(LOGIN_PATH, params); -}; +// The homeserver rides in the query string: as a path segment a server given as a full URL +// ("http://localhost:8008") needs an escaped slash, which hosting layers normalise away. +const withServerParam = (path: string, server?: string): string => + server ? withSearchParam(path, { [SERVER_SEARCH_PARAM]: server }) : path; -export const getRegisterPath = (server?: string): string => { - const params = server ? { server: encodeURIComponent(server) } : undefined; - return generatePath(REGISTER_PATH, params); -}; +export const getLoginPath = (server?: string): string => withServerParam(LOGIN_PATH, server); -export const getResetPasswordPath = (server?: string): string => { - const params = server ? { server: encodeURIComponent(server) } : undefined; - return generatePath(RESET_PASSWORD_PATH, params); -}; +export const getRegisterPath = (server?: string): string => withServerParam(REGISTER_PATH, server); + +export const getResetPasswordPath = (server?: string): string => + withServerParam(RESET_PASSWORD_PATH, server); export const getHomePath = (): string => HOME_PATH; export const getHomeJoinPath = (): string => HOME_JOIN_PATH; diff --git a/src/app/pages/paths.ts b/src/app/pages/paths.ts index c45de24e08..7839eb5b46 100644 --- a/src/app/pages/paths.ts +++ b/src/app/pages/paths.ts @@ -1,23 +1,31 @@ export const ROOT_PATH = '/'; +export const SERVER_SEARCH_PARAM = 'server'; + export type LoginPathSearchParams = { username?: string; email?: string; loginToken?: string; }; -export const LOGIN_PATH = '/login/:server?/'; +export const LOGIN_PATH = '/login'; export type RegisterPathSearchParams = { username?: string; email?: string; token?: string; }; -export const REGISTER_PATH = '/register/:server?/'; +export const REGISTER_PATH = '/register'; export type ResetPasswordPathSearchParams = { email?: string; }; -export const RESET_PASSWORD_PATH = '/reset-password/:server?/'; +export const RESET_PASSWORD_PATH = '/reset-password'; + +// The homeserver used to be a path segment ("/login/matrix.org"); these catch links, SSO and +// OIDC callbacks minted by older builds. +export const LEGACY_LOGIN_PATH = `${LOGIN_PATH}/*`; +export const LEGACY_REGISTER_PATH = `${REGISTER_PATH}/*`; +export const LEGACY_RESET_PASSWORD_PATH = `${RESET_PASSWORD_PATH}/*`; export type SettingsPathSearchParams = { focus?: string; diff --git a/src/app/utils/oauthCallback.test.ts b/src/app/utils/oauthCallback.test.ts new file mode 100644 index 0000000000..635d4646b0 --- /dev/null +++ b/src/app/utils/oauthCallback.test.ts @@ -0,0 +1,30 @@ +import { beforeEach, describe, expect, it } from 'vitest'; +import { normalizeOAuthCallbackUrl } from './oauthCallback'; + +const goTo = (url: string) => window.history.replaceState(null, '', url); + +describe('normalizeOAuthCallbackUrl', () => { + beforeEach(() => { + goTo('/'); + }); + + it('moves a fragment callback into the query string, keeping the homeserver', () => { + goTo('/login?server=matrix.org#code=c1&state=s1'); + normalizeOAuthCallbackUrl(); + expect(window.location.pathname).toBe('/login'); + expect(window.location.search).toBe('?server=matrix.org&code=c1&state=s1'); + expect(window.location.hash).toBe(''); + }); + + it('keeps the homeserver when rebuilding the hash route', () => { + goTo('/login?server=matrix.org#code=c1&state=s1'); + normalizeOAuthCallbackUrl({ enabled: true, basename: '/' }); + expect(window.location.hash).toBe('#/login?server=matrix.org&code=c1&state=s1'); + }); + + it('leaves a url without a callback fragment alone', () => { + goTo('/login?server=matrix.org'); + normalizeOAuthCallbackUrl(); + expect(window.location.search).toBe('?server=matrix.org'); + }); +}); diff --git a/src/app/utils/oauthCallback.ts b/src/app/utils/oauthCallback.ts index 3bce43c947..2083733851 100644 --- a/src/app/utils/oauthCallback.ts +++ b/src/app/utils/oauthCallback.ts @@ -23,9 +23,11 @@ export const normalizeOAuthCallbackUrl = (hashRouter?: HashRouterConfig): void = .map((part) => trimSlash(part ?? '')) .filter(Boolean) .join('/'); + const routeParams = new URLSearchParams(url.search); + params.forEach((value, key) => routeParams.set(key, value)); url.pathname = basePath; url.search = ''; - url.hash = `/${route}?${params}`; + url.hash = `/${route}?${routeParams}`; } else { params.forEach((value, key) => url.searchParams.set(key, value)); } diff --git a/tests/e2e/fixtures/loginAccount.ts b/tests/e2e/fixtures/loginAccount.ts new file mode 100644 index 0000000000..6c26e0f3a7 --- /dev/null +++ b/tests/e2e/fixtures/loginAccount.ts @@ -0,0 +1,2 @@ +export const LOGIN_USERNAME = 'loginuser'; +export const LOGIN_PASSWORD = 'test-passw0rd'; diff --git a/tests/e2e/global-setup.ts b/tests/e2e/global-setup.ts index 310b968fc3..3bb4c73356 100644 --- a/tests/e2e/global-setup.ts +++ b/tests/e2e/global-setup.ts @@ -2,6 +2,7 @@ import { mkdir, writeFile } from 'node:fs/promises'; import { dirname } from 'node:path'; import type { FullConfig } from '@playwright/test'; import { createRoom, sendText, startContinuwuity } from './fixtures/continuwuity'; +import { LOGIN_PASSWORD, LOGIN_USERNAME } from './fixtures/loginAccount'; type InjectedSession = { baseUrl: string; @@ -17,6 +18,10 @@ export default async function globalSetup(config: FullConfig): Promise<() => Pro const hs = await startContinuwuity(); const user = await hs.register('alice', 'test-passw0rd'); + // login.spec.ts signs in for real, so it needs its own account and the homeserver url. + await hs.register(LOGIN_USERNAME, LOGIN_PASSWORD); + process.env.TEST_HOMESERVER_URL = hs.baseUrl; + const general = await createRoom(hs.baseUrl, user.accessToken, { name: 'General', preset: 'private_chat', diff --git a/tests/e2e/login.spec.ts b/tests/e2e/login.spec.ts new file mode 100644 index 0000000000..519f53a68c --- /dev/null +++ b/tests/e2e/login.spec.ts @@ -0,0 +1,50 @@ +import { expect, test } from '@playwright/test'; +import { LOGIN_PASSWORD, LOGIN_USERNAME } from './fixtures/loginAccount'; + +const CLIENT_READY_TIMEOUT = 30_000; + +// The container homeserver is reachable only by url ("http://127.0.0.1:"), which is the +// shape that used to break: as a path segment it needs %2F, and hosting rewrites that to "/". +const homeserverUrl = () => { + const url = process.env.TEST_HOMESERVER_URL; + if (!url) throw new Error('TEST_HOMESERVER_URL is not set by global setup'); + return url; +}; + +test.use({ storageState: { cookies: [], origins: [] } }); + +test.describe('password login against a homeserver given as a url', () => { + test('keeps the homeserver in the query string and lands in the app', async ({ page }) => { + await page.goto('/'); + await expect(page).toHaveURL(/\/login\?server=/); + + const server = homeserverUrl(); + await page.getByRole('textbox').first().fill(server); + await page.getByRole('textbox').first().press('Enter'); + + await expect(page).toHaveURL(`/login?server=${encodeURIComponent(server)}`); + + const username = page.getByRole('textbox', { name: 'Username' }); + await expect(username).toBeVisible({ timeout: CLIENT_READY_TIMEOUT }); + await username.fill(LOGIN_USERNAME); + await page.getByRole('textbox', { name: 'Password' }).fill(LOGIN_PASSWORD); + await page.getByRole('button', { name: 'Login' }).click(); + + await expect(page).toHaveURL(/\/home/, { timeout: CLIENT_READY_TIMEOUT }); + }); + + test('survives a reload of the login url', async ({ page }) => { + const server = homeserverUrl(); + await page.goto(`/login?server=${encodeURIComponent(server)}`); + await page.reload(); + + await expect(page).toHaveURL(`/login?server=${encodeURIComponent(server)}`); + await expect(page.getByRole('textbox').first()).toHaveValue(server); + }); + + test('redirects a link that still carries the homeserver in the path', async ({ page }) => { + await page.goto('/login/matrix.org?loginToken=tok'); + + await expect(page).toHaveURL('/login?loginToken=tok&server=matrix.org'); + }); +});