fix(auth): carry the homeserver in the query string instead of the path

.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.

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)
         <Route path={RESET_PASSWORD_PATH} element={<ResetPassword />} />
       </Route>
 
+      <Route path={LEGACY_LOGIN_PATH} loader={legacyAuthLoader(LOGIN_PATH)} />
+      <Route path={LEGACY_REGISTER_PATH} loader={legacyAuthLoader(REGISTER_PATH)} />
+      <Route path={LEGACY_RESET_PASSWORD_PATH} loader={legacyAuthLoader(RESET_PASSWORD_PATH)} />
+
       <Route
         loader={() => {
           const session = getFirstSession();

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();
+  });
+});

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);

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 (
     <Box justifyContent="Center" alignItems="Center" gap="200">
@@ -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]
   );

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');
+  });
+});

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['*']));

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', () => {

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, string>): 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;