Close the adversarial review round: auth hardening, data fidelity against the platform context, backend grants and docker coherence

src/app/AuthGate.tsx

@@ -29,14 +29,45 @@ import { ensureIdentity } from '@ng-eventually/polyfill';
 import { startNgSession } from '../shared/utils/ngSession';
 import { setCurrentPrincipal } from '../shared/utils/currentPrincipal';
 import { setCurrentSession } from '../shared/utils/currentSession';
+import type { AuthProvider } from '@activitypods/refine-providers';
 import { useRouter, useNavigate } from './router';
 import { initialBackendName } from '../shared/refine/providers';
 import { getBackendUrl } from '../shared/utils/runtimeConfig';
-import { buildActivityPodsAuthProvider, parseAuthFlowStage } from './auth/activityPodsAuth';
+import {
+  buildActivityPodsAuthProvider,
+  parseAuthFlowStage,
+  safeRedirectPath,
+} from './auth/activityPodsAuth';
 import { PodProviderPicker } from './auth/PodProviderPicker';
 
 type ActivityPodsAuthState = 'checking' | 'authenticated' | 'unauthenticated' | 'error';
 
+type CallbackResult = Awaited<ReturnType<AuthProvider['handleCallback']>>;
+
+/**
+ * The in-flight (or settled) `handleCallback()` exchange, keyed by the `?code=`
+ * it belongs to. React's StrictMode double-invokes effects in dev and the
+ * authorization code is single-use: a second exchange would race the first, throw
+ * on the loser, and show the fatal screen after a successful login. Both effect
+ * runs share this one promise, so the exchange happens exactly once; only the
+ * live run publishes its result. Scoped to the code value so a genuinely new
+ * login (a new code) is not mistaken for a re-entry.
+ */
+let callbackExchange: { code: string; promise: Promise<CallbackResult> } | undefined;
+
+/**
+ * Drop the OIDC callback parameters once the exchange has run. The code is
+ * consumed; leaving it in the address bar lets a later effect re-run — or a
+ * reload — treat the page as a fresh callback.
+ */
+function stripCallbackParams(): void {
+  const url = new URL(window.location.href);
+  url.searchParams.delete('code');
+  url.searchParams.delete('state');
+  url.searchParams.delete('session_state');
+  window.history.replaceState(null, '', `${url.pathname}${url.search}${url.hash}`);
+}
+
 export function AuthGate({ children }: { children: ReactNode }) {
   const { route } = useRouter();
   const navigate = useNavigate();
@@ -86,28 +117,68 @@ export function AuthGate({ children }: { children: ReactNode }) {
     let cancelled = false;
 
     async function run() {
+      const params = new URLSearchParams(window.location.search);
       const stage = parseAuthFlowStage(window.location.search);
+      // Captured before the exchange so stripping the params cannot erase what
+      // the failure decision below needs.
+      const code = params.get('code');
+      const hadFlowState = params.has('state');
 
       try {
         if (stage.stage === 'callback') {
-          const result = await provider.handleCallback();
-          if (cancelled) return;
-          const session = provider.getSession();
-          setCurrentPrincipal(result.webId);
-          setCurrentSession(session ? { webId: session.webId, token: session.token } : undefined);
-          navigate(`/login?register_app=1&redirect=${encodeURIComponent(stage.redirectPath)}`);
-          return;
+          // One exchange per code: the StrictMode double-run (both mounts see the
+          // same `?code=`) shares the in-flight promise instead of racing.
+          if (!callbackExchange || callbackExchange.code !== code) {
+            callbackExchange = { code: code ?? '', promise: provider.handleCallback() };
+          }
+          const exchange = callbackExchange;
+          try {
+            const result = await exchange.promise;
+            stripCallbackParams();
+            if (cancelled) return;
+            const session = provider.getSession();
+            setCurrentPrincipal(result.webId);
+            setCurrentSession(session ? { webId: session.webId, token: session.token } : undefined);
+            // The contract's callback result carries where to return to; honour
+            // it (validated) rather than trusting only the parsed URL.
+            const redirect = safeRedirectPath(result.redirect ?? stage.redirectPath);
+            navigate(`/login?register_app=1&redirect=${encodeURIComponent(redirect)}`);
+            return;
+          } catch (err) {
+            stripCallbackParams();
+            if (cancelled) return;
+            // A `?code=` with no flow state — a stale bookmark, any link that
+            // happens to carry the param — is not a callback the provider can
+            // complete: it had nothing to exchange. Degrade to the picker rather
+            // than the fatal screen. A callback that DID carry flow state (a
+            // `state` param) stays a visible error: it is a real attempt the
+            // provider refused.
+            if (!hadFlowState) {
+              setCurrentSession(undefined);
+              setApState('unauthenticated');
+              return;
+            }
+            throw err;
+          } finally {
+            if (callbackExchange === exchange) callbackExchange = undefined;
+          }
         }
 
         if (stage.stage === 'register_app') {
           const session = provider.getSession();
-          const registration = session ? await provider.registerApp(session.webId) : undefined;
+          if (!session) {
+            // No live session at this stage — a stale `?register_app=1` link, or
+            // a token that has expired. There is nothing to register; show the
+            // picker instead of stalling on a permanent blank `checking` screen.
+            setCurrentSession(undefined);
+            if (!cancelled) setApState('unauthenticated');
+            return;
+          }
+          const registration = await provider.registerApp(session.webId);
           if (cancelled) return;
           if (registration) {
-            if (session) {
-              setCurrentPrincipal(session.webId);
-              setCurrentSession({ webId: session.webId, token: session.token });
-            }
+            setCurrentPrincipal(session.webId);
+            setCurrentSession({ webId: session.webId, token: session.token });
             navigate(stage.redirectPath);
           }
           // If `registerApp` returned `undefined`, the browser left for the Pod's
@@ -195,9 +266,20 @@ export function AuthGate({ children }: { children: ReactNode }) {
       return null;
     }
     if (apState === 'unauthenticated') {
+      // Seed the post-login destination: an explicit (validated) `?redirect=`
+      // wins; otherwise a deep link the user opened lands them back on that page
+      // once signed in, instead of dropping everyone on `/home`. Login and the
+      // bare welcome route are not deep links.
+      const hasExplicitRedirect = new URLSearchParams(window.location.search).has('redirect');
+      const deepLink =
+        route.page !== 'login' && route.page !== 'welcome' ? window.location.pathname : undefined;
+      const loginRedirect = hasExplicitRedirect
+        ? parseAuthFlowStage(window.location.search).redirectPath
+        : deepLink ?? '/home';
       return (
         <PodProviderPicker
           authProvider={apAuthProvider!}
+          redirect={loginRedirect}
           onDemo={() => {
             setDemoMode(true);
             navigate('/home');

src/app/auth/PodProviderPicker.tsx

@@ -5,6 +5,12 @@ import { resolveActivityPodsAuthConfig } from './activityPodsAuth';
 
 interface PodProviderPickerProps {
   authProvider: AuthProvider;
+  /**
+   * Where to send the user once the flow completes. The gate computes it —
+   * a validated `?redirect=` value, or the route the user was trying to reach —
+   * so the picker never reads the raw URL itself.
+   */
+  redirect: string;
   onDemo?: () => void;
 }
 
@@ -15,13 +21,11 @@ interface PodProviderPickerProps {
  * offered as a one-click option. The user can also type any Solid Pod provider
  * URL. A "continue offline" button keeps the local demo fallback reachable.
  */
-export function PodProviderPicker({ authProvider, onDemo }: PodProviderPickerProps) {
+export function PodProviderPicker({ authProvider, redirect, onDemo }: PodProviderPickerProps) {
   const { podProviderUrl } = resolveActivityPodsAuthConfig(window.location.origin);
   const [customUrl, setCustomUrl] = useState('');
 
   const startLogin = (issuer: string) => {
-    const params = new URLSearchParams(window.location.search);
-    const redirect = params.get('redirect') || '/home';
     void authProvider.login({ issuer, redirect });
   };
 

src/app/auth/activityPodsAuth.test.ts

@@ -26,6 +26,12 @@ test('clientId falls back to origin/app when nothing is configured', () => {
   expect(cfg.clientId).toBe('http://localhost:5000/app');
 });
 
+test('clientId does not double the slash on a trailing-slash backendUrl', () => {
+  setRuntimeConfig({ backendUrl: 'http://localhost:3003/' });
+  const cfg = resolveActivityPodsAuthConfig('http://localhost:5000');
+  expect(cfg.clientId).toBe('http://localhost:3003/app');
+});
+
 test('podProviderUrl is passed through when set', () => {
   setRuntimeConfig({ podProviderUrl: 'http://localhost:3000' });
   const cfg = resolveActivityPodsAuthConfig('http://localhost:5000');
@@ -66,8 +72,22 @@ test('parseAuthFlowStage detects app registration stage', () => {
 });
 
 test('parseAuthFlowStage defaults to picker', () => {
-  expect(parseAuthFlowStage('')).toEqual({ stage: 'picker' });
-  // A bare `?redirect` does not change the stage; the redirect path is only read
-  // when the user lands back from the Pod provider.
-  expect(parseAuthFlowStage('?redirect=/events')).toEqual({ stage: 'picker' });
+  expect(parseAuthFlowStage('')).toEqual({ stage: 'picker', redirectPath: '/home' });
+  // A bare `?redirect` does not change the stage; it is carried (validated) so
+  // every stage exposes the same post-login destination.
+  expect(parseAuthFlowStage('?redirect=/events')).toEqual({
+    stage: 'picker',
+    redirectPath: '/events',
+  });
+});
+
+test('parseAuthFlowStage rejects non-same-origin redirects', () => {
+  // Absolute, protocol-relative, and backslash spellings all resolve off-origin
+  // — they must fall back to /home, never reach history.pushState.
+  expect(parseAuthFlowStage('?code=abc&redirect=https://evil.example').redirectPath).toBe('/home');
+  expect(parseAuthFlowStage('?register_app=1&redirect=//evil.example').redirectPath).toBe('/home');
+  expect(parseAuthFlowStage('?code=abc&redirect=/\\evil.example').redirectPath).toBe('/home');
+  expect(parseAuthFlowStage('?code=abc&redirect=relative/path').redirectPath).toBe('/home');
+  // A same-origin deep link survives untouched.
+  expect(parseAuthFlowStage('?code=abc&redirect=/events/42').redirectPath).toBe('/events/42');
 });

src/app/auth/activityPodsAuth.ts

@@ -19,11 +19,43 @@ export function buildActivityPodsAuthProvider(origin: string): AuthProvider {
   });
 }
 
+/**
+ * End the ActivityPods session on this device.
+ *
+ * Per the contract, `logout` clears local state — the stored OIDC token — and
+ * the token lives at ONE fixed storage key per page (`One session per page`,
+ * non-guarantees), so clearing it here ends the session for the whole app. The
+ * provider is built from the same resolved config as the gate and the data
+ * provider; the app's own session store (`utils/currentSession`) is cleared by
+ * the caller, before the reload.
+ */
+export async function logoutActivityPods(): Promise<void> {
+  await buildActivityPodsAuthProvider(window.location.origin).logout({});
+}
+
 /** Which stage of the Solid-OIDC / app-registration flow the current URL represents. */
 export type AuthFlowStage =
   | { stage: 'callback'; redirectPath: string }
   | { stage: 'register_app'; redirectPath: string }
-  | { stage: 'picker' };
+  | { stage: 'picker'; redirectPath: string };
+
+/**
+ * A same-origin post-login destination, or `/home`.
+ *
+ * Only a leading-slash path is accepted. An absolute URL, a protocol-relative
+ * `//host` value, its backslash spelling `/\host` (which the URL parser resolves
+ * the same way), or anything not starting with `/` falls back to `/home`.
+ * Validated here, in the one place the URL is parsed, so every consumer — the
+ * callback navigation and the picker's login call — inherits it. Without this an
+ * attacker-chosen `?redirect=` could leave the origin, and `history.pushState`
+ * would throw a SecurityError into the fatal screen.
+ */
+export function safeRedirectPath(raw: string | null | undefined): string {
+  if (!raw || !raw.startsWith('/') || raw.startsWith('//') || raw.startsWith('/\\')) {
+    return '/home';
+  }
+  return raw;
+}
 
 /**
  * Parse the current URL search params to decide what AuthGate should do.
@@ -34,12 +66,13 @@ export type AuthFlowStage =
  *   registered before entering the app.
  * - otherwise → show the Pod-provider picker.
  *
- * `redirect` is preserved through the stages and defaults to `/home`.
+ * `redirect` is validated (`safeRedirectPath`) and carried on every stage,
+ * defaulting to `/home`.
  */
 export function parseAuthFlowStage(search: string): AuthFlowStage {
   const params = new URLSearchParams(search);
-  const redirectPath = params.get('redirect') || '/home';
+  const redirectPath = safeRedirectPath(params.get('redirect'));
   if (params.has('code')) return { stage: 'callback', redirectPath };
   if (params.has('register_app')) return { stage: 'register_app', redirectPath };
-  return { stage: 'picker' };
+  return { stage: 'picker', redirectPath };
 }

src/app/router.test.ts

@@ -0,0 +1,75 @@
+import { expect, test } from 'bun:test';
+import { parsePath, routeToPath, eventPath, userPath, type Route } from './router';
+
+// Ids on the ActivityPods backend are absolute IRIs. The router must carry them
+// through the URL intact: encode on the way out, decode on the way in. A raw IRI
+// in a path would be read as route structure (the `/` and `:` split it, and the
+// `http:` segment falls through to the welcome page).
+
+const IRI = 'http://localhost:3000/alice/data/abc';
+
+test('an absolute-IRI event id survives routeToPath → parsePath intact', () => {
+  const route: Route = { page: 'event-detail', eventId: IRI };
+  const path = routeToPath(route);
+  expect(path).toBe(`/events/${encodeURIComponent(IRI)}`);
+  expect(parsePath(path)).toEqual({ page: 'event-detail', eventId: IRI });
+});
+
+test('an absolute-IRI event id survives every sub-route intact', () => {
+  const cases: Array<[Route, Route]> = [
+    [{ page: 'update-event', eventId: IRI }, { page: 'update-event', eventId: IRI }],
+    [{ page: 'invite', eventId: IRI }, { page: 'invite', eventId: IRI }],
+    [{ page: 'participants', eventId: IRI }, { page: 'participants', eventId: IRI }],
+    [{ page: 'meeting-points', eventId: IRI }, { page: 'meeting-points', eventId: IRI }],
+  ];
+  for (const [route, expected] of cases) {
+    expect(parsePath(routeToPath(route))).toEqual(expected);
+  }
+});
+
+test('an absolute-IRI user id survives routeToPath → parsePath intact', () => {
+  const route: Route = { page: 'user-profile', userId: IRI };
+  expect(parsePath(routeToPath(route))).toEqual({ page: 'user-profile', userId: IRI });
+});
+
+// The id ITSELF may end in a segment that names a sub-route; encoding is what
+// keeps it from being read as one.
+test('an encoded id whose tail looks like a sub-route is not mistaken for one', () => {
+  const tricky = 'http://localhost:3000/alice/data/edit';
+  expect(parsePath(routeToPath({ page: 'event-detail', eventId: tricky }))).toEqual({
+    page: 'event-detail',
+    eventId: tricky,
+  });
+  expect(parsePath(eventPath(tricky, 'participants'))).toEqual({
+    page: 'participants',
+    eventId: tricky,
+  });
+});
+
+test('eventPath and userPath are the encoded path builders', () => {
+  expect(eventPath(IRI)).toBe(`/events/${encodeURIComponent(IRI)}`);
+  expect(eventPath(IRI, 'edit')).toBe(`/events/${encodeURIComponent(IRI)}/edit`);
+  expect(userPath(IRI)).toBe(`/users/${encodeURIComponent(IRI)}`);
+});
+
+// Defensive: a raw (unencoded) IRI still parses rather than falling through —
+// the id is taken as the whole remainder and the sub-route from the tail.
+test('a raw absolute-IRI path still parses as the whole remainder', () => {
+  expect(parsePath('/events/http://localhost:3000/alice/data/abc')).toEqual({
+    page: 'event-detail',
+    eventId: IRI,
+  });
+  expect(parsePath('/events/http://localhost:3000/alice/data/abc/participants')).toEqual({
+    page: 'participants',
+    eventId: IRI,
+  });
+});
+
+test('the fixed routes are unchanged', () => {
+  expect(parsePath('/')).toEqual({ page: 'welcome' });
+  expect(parsePath('/home')).toEqual({ page: 'home' });
+  expect(parsePath('/events')).toEqual({ page: 'events' });
+  expect(parsePath('/events/new')).toEqual({ page: 'create-event' });
+  expect(parsePath('/profile/edit')).toEqual({ page: 'edit-profile' });
+  expect(parsePath('/unknown')).toEqual({ page: 'welcome' });
+});

src/app/router.tsx

@@ -34,7 +34,21 @@ export interface RouteParams {
 // Path parsing & generation
 // ============================================================================
 
-function parsePath(pathname: string): Route {
+/**
+ * Decode one path segment. Ids on the ActivityPods backend are absolute IRIs, so
+ * they are percent-encoded by `routeToPath`; a malformed escape (a raw IRI that
+ * was never encoded) is returned verbatim rather than throwing — the captured
+ * text IS the id in that case.
+ */
+function decodeSegment(segment: string): string {
+  try {
+    return decodeURIComponent(segment);
+  } catch {
+    return segment;
+  }
+}
+
+export function parsePath(pathname: string): Route {
   const path = pathname.replace(/\/+$/, '') || '/';
 
   if (path === '/' || path === '') return { page: 'welcome' };
@@ -49,10 +63,14 @@ function parsePath(pathname: string): Route {
   if (path === '/profile/share') return { page: 'share-profile' };
   if (path === '/profile/connect') return { page: 'connect' };
 
-  // /events/:id/...
-  const eventMatch = path.match(/^\/events\/([^/]+)(?:\/(.+))?$/);
+  // /events/:id/... — the id is an absolute IRI, encoded by `routeToPath` so it
+  // holds no `/`; the sub-route is matched from the tail, and a raw (unencoded)
+  // IRI still parses as the whole remainder. An ENCODED id may itself contain a
+  // `/edit` segment without being read as a sub-route, which is why encoding is
+  // mandatory on every navigation.
+  const eventMatch = path.match(/^\/events\/(.+?)(?:\/(edit|invite|participants|meeting-points))?$/);
   if (eventMatch) {
-    const eventId = eventMatch[1]!;
+    const eventId = decodeSegment(eventMatch[1]!);
     const sub = eventMatch[2];
     if (!sub) return { page: 'event-detail', eventId };
     if (sub === 'edit') return { page: 'update-event', eventId };
@@ -61,10 +79,10 @@ function parsePath(pathname: string): Route {
     if (sub === 'meeting-points') return { page: 'meeting-points', eventId };
   }
 
-  // /users/:id
-  const userMatch = path.match(/^\/users\/([^/]+)$/);
+  // /users/:id — an absolute actor URI, encoded for the same reason.
+  const userMatch = path.match(/^\/users\/(.+)$/);
   if (userMatch) {
-    return { page: 'user-profile', userId: userMatch[1]! };
+    return { page: 'user-profile', userId: decodeSegment(userMatch[1]!) };
   }
 
   return { page: 'welcome' };
@@ -77,21 +95,39 @@ export function routeToPath(route: Route): string {
     case 'events': return '/events';
     case 'create-event': return '/events/new';
     case 'login': return '/login';
-    case 'event-detail': return `/events/${route.eventId}`;
-    case 'update-event': return `/events/${route.eventId}/edit`;
-    case 'invite': return `/events/${route.eventId}/invite`;
-    case 'participants': return `/events/${route.eventId}/participants`;
-    case 'meeting-points': return `/events/${route.eventId}/meeting-points`;
+    // Ids are absolute IRIs: they MUST be percent-encoded into a single path
+    // segment, or the `/` and `:` inside them are read as route structure and
+    // navigation falls through to the welcome page. `parsePath` decodes them.
+    case 'event-detail': return `/events/${encodeURIComponent(route.eventId)}`;
+    case 'update-event': return `/events/${encodeURIComponent(route.eventId)}/edit`;
+    case 'invite': return `/events/${encodeURIComponent(route.eventId)}/invite`;
+    case 'participants': return `/events/${encodeURIComponent(route.eventId)}/participants`;
+    case 'meeting-points': return `/events/${encodeURIComponent(route.eventId)}/meeting-points`;
     case 'profile': return '/profile';
     case 'edit-profile': return '/profile/edit';
     case 'friends': return '/profile/friends';
     case 'share-profile': return '/profile/share';
     case 'connect': return '/profile/connect';
-    case 'user-profile': return `/users/${route.userId}`;
+    case 'user-profile': return `/users/${encodeURIComponent(route.userId)}`;
     case 'settings': return '/settings';
   }
 }
 
+/**
+ * Build a path to an event sub-route, encoding the id. Screens navigate by path
+ * rather than by `Route`, so this is the one place that keeps a raw IRI out of
+ * the URL structure.
+ */
+export function eventPath(eventId: string, sub?: 'edit' | 'invite' | 'participants' | 'meeting-points'): string {
+  const base = `/events/${encodeURIComponent(eventId)}`;
+  return sub ? `${base}/${sub}` : base;
+}
+
+/** Build a path to a user profile, encoding the actor URI. */
+export function userPath(userId: string): string {
+  return `/users/${encodeURIComponent(userId)}`;
+}
+
 // ============================================================================
 // Router context
 // ============================================================================