From 1b74c8181fed38a8a0acc311e4a801e28e047345 Mon Sep 17 00:00:00 2001 From: NCHILLC Date: Tue, 13 Oct 2026 10:48:50 -0400 Subject: [PATCH] fix(auth): a session the API refuses ends at /login, not at a default shell (QA sweep ONB-04) A token issued before a password change or reset still has a live `exp`, so `requireToken` lets it through, but the API refuses it (`pwchanged:{userId}`). The auth layout loader swallowed that 401 as a "graceful fallback" and the page came back 200 as an "Unnamed" shell on openinspection.dev with no Dispatch item, on every reload. The loader now ends the session on a 401 from the session-context call (`destroyUserSession`: both cookies cleared, 302 to `/login`), the same place a visitor with no cookie lands. Only 401 does this; a 5xx or a thrown fetch keeps the defaults, since the token may be fine. Tests: `auth-layout.loader.test.ts` (red against the unfixed loader), and the password-reset e2e now keeps the pre-reset session and expects 302 to `/login` for it, with the fresh session as the control. fork-log row added; the NET-06 ceiling about a password-changed token bounced to `returnTo` is amended. Co-Authored-By: Claude Sonnet 5.5 --- app/routes/auth-layout.loader.test.ts | 71 +++++++++++++++++++++++++++ app/routes/auth-layout.tsx | 11 ++++- docs/develop/fork-log.md | 3 +- tests/e2e/auth-password-reset.spec.ts | 18 +++++++ 4 files changed, 101 insertions(+), 2 deletions(-) create mode 100644 app/routes/auth-layout.loader.test.ts diff --git a/app/routes/auth-layout.loader.test.ts b/app/routes/auth-layout.loader.test.ts new file mode 100644 index 00000000..92398474 --- /dev/null +++ b/app/routes/auth-layout.loader.test.ts @@ -0,0 +1,71 @@ +// ONB-04 — a session the API no longer honours (issued before a password change +// or reset, or its user was removed) used to load the workspace shell at 200 +// with default branding: "Unnamed", openinspection.dev, no Dispatch. The layout +// loader swallowed the 401 from the session-context call as a "graceful +// fallback". It must end the session and send the visitor to /login instead. +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { loader } from '~/routes/auth-layout'; +import { createLoadContext } from '~/lib/load-context'; + +const { contextGet } = vi.hoisted(() => ({ contextGet: vi.fn() })); +vi.mock('~/lib/api-client.server', () => ({ + createApi: () => ({ sessionContext: { context: { $get: contextGet } } }), +})); + +// Braces matter: vitest calls a function returned from a hook as its teardown, +// and `mockReset()` returns the mock itself. +beforeEach(() => { + contextGet.mockReset(); +}); + +type LoaderArgs = Parameters[0]; + +// `requireToken` reads `exp` from the payload and nothing else (the API is what +// verifies the signature), so a live session is a payload with a future `exp`. +// Same hand-rolled request as login-saas-bounce.test.ts: happy-dom drops a +// `Cookie` header set on `new Request(...)`. +const LIVE_COOKIE = `__Host-inspector_token=h.${btoa(JSON.stringify({ exp: 4102444800 }))}.s`; + +function args(pathname = '/inspections'): LoaderArgs { + const request = { + url: `http://app.example.com${pathname}`, + headers: { get: (name: string) => (name.toLowerCase() === 'cookie' ? LIVE_COOKIE : null) }, + }; + return { request, context: createLoadContext({}), params: {} } as unknown as LoaderArgs; +} + +const json = (body: unknown, status = 200) => new Response(JSON.stringify(body), { status }); + +describe('auth layout loader — session the API refuses (ONB-04)', () => { + it('a 401 clears the session and redirects to /login instead of rendering a default shell', async () => { + contextGet.mockResolvedValue(json({ success: false, error: 'Token has been invalidated' }, 401)); + const thrown = await loader(args()).then( + () => null, + (e: unknown) => e, + ); + expect(thrown).toBeInstanceOf(Response); + const res = thrown as Response; + expect(res.status).toBe(302); + expect(res.headers.get('Location')).toBe('/login'); + expect(res.headers.get('Set-Cookie')).toMatch(/__Host-inspector_token=;.*Max-Age=0/); + }); + + it('a server error keeps the defaults: the token may be fine, so the session is not ended', async () => { + contextGet.mockResolvedValue(json({ success: false }, 500)); + const res = await loader(args()); + expect((res as { data: unknown }).data).toEqual({ context: null }); + }); + + it('an unreachable API keeps the defaults too', async () => { + contextGet.mockRejectedValue(new TypeError('fetch failed')); + const res = await loader(args()); + expect((res as { data: unknown }).data).toEqual({ context: null }); + }); + + it('a 200 hands the session context to the page', async () => { + const ctx = { user: { locale: null }, branding: { defaultLocale: null } }; + contextGet.mockResolvedValue(json({ success: true, data: ctx })); + const res = await loader(args()); + expect((res as { data: unknown }).data).toEqual({ context: ctx }); + }); +}); diff --git a/app/routes/auth-layout.tsx b/app/routes/auth-layout.tsx index 1474ebb4..28df0bf5 100644 --- a/app/routes/auth-layout.tsx +++ b/app/routes/auth-layout.tsx @@ -2,7 +2,7 @@ import { useCallback, useEffect, useMemo, useState } from "react"; import { data, Outlet, useLoaderData, useLocation, useNavigate, useNavigation } from "react-router"; import { uiLocaleStampFor } from "../../server/lib/i18n/ui-locale"; import type { Route } from "./+types/auth-layout"; -import { requireToken } from "~/lib/session.server"; +import { destroyUserSession, requireToken } from "~/lib/session.server"; import { createApi } from "~/lib/api-client.server"; import { CommandPalette, CommandPaletteProvider } from "~/components/CommandPalette"; import { Sidebar, MobileHeader } from "~/components/Sidebar"; @@ -35,9 +35,11 @@ function useDelayedFlag(active: boolean, delayMs: number): boolean { export async function loader({ request, context }: Route.LoaderArgs) { const token = await requireToken(context, request); let sessionContext: SessionContext | null = null; + let refused = false; try { const api = createApi(context, { token }); const res = await api.sessionContext.context.$get(); + refused = res.status === 401; if (res.ok) { const body = (await res.json()) as Record; sessionContext = body.data as SessionContext; @@ -45,6 +47,13 @@ export async function loader({ request, context }: Route.LoaderArgs) { } catch { // Graceful fallback — layout renders with defaults } + // A 401 is the API saying this token is no longer a session: issued before a + // password change or reset, or its user was removed. `requireToken` cannot see + // that (the payload still reads as live), and the fallback above would paint + // an "Unnamed" shell at 200 for a signed-out visitor. Only 401 ends the + // session; a 5xx or a throw keeps the defaults, since the token may be fine. + // Outside the try so the thrown redirect is not swallowed by its catch. + if (refused) throw await destroyUserSession(context, request); // i18n activation (#269) — the DATABASE half of locale resolution. // diff --git a/docs/develop/fork-log.md b/docs/develop/fork-log.md index 3db73c96..b9e7608f 100644 --- a/docs/develop/fork-log.md +++ b/docs/develop/fork-log.md @@ -319,7 +319,8 @@ deliberate departure rather than work upstream simply has not done yet. | `editor` notes reach the doc on idle (2026-10-12 QA sweep CORE-01; no upstream fix) | `setNotes` still echoes into `results` per keystroke, but now also schedules the doc write 600 ms after the last change, and `pagehide` / `visibilitychange: hidden` flush it; blur (`commitNotes`) writes at once. The path moved out of `collab-findings-api.ts` into `app/lib/collab/finding-notes.ts`. Dictation (`DictationButton`) and the comment typeahead pick call `setNotes`, so they ride the same timer; `insertComment` and `cloneLast` flush a pending note before they write. Spec: `app/lib/collab/finding-notes.test.tsx` | The doc write waited for the textarea's blur, so a reload, a discarded tab or a crash before blur lost the note while the header said "Synced". `git log main..upstream/main -- app/lib/collab/` is empty, so upstream has the same blur-only design. The caret-jump concern behind blur-only still holds for per-keystroke writes: the idle write commits the text the textarea already shows, so the rebuilt `results` carries an identical string. Not changed: the "Synced" indicator itself (NET-06) | | `auth` first-run link (2026-10-12 QA sweep ONB-01; no upstream fix) | The `/login` loader in `app/routes/login.tsx`, on a profile with `hasSetupWizard`, asks `GET /api/auth/setup-status` and redirects to `/setup` when the answer is an explicit `isSetUp: false`. Any other outcome (refusal, unreachable API, malformed body) shows the login form. `/` already lands on `/login`, so one loader covers both. Spec: the ONB-01 block in `app/routes/login-saas-bounce.test.ts` | A fresh instance's `/` went to a login form with no way to `/setup`, which was findable only by already knowing the URL. `git log main..upstream/main` on `app/routes/login.tsx` and `setup.tsx` has no setup-link change (the hits are 2FA, invite links and submit latches). `/setup` already redirects to `/login` once users exist; the explicit-`false` rule keeps the two from looping. The loader's call is one more in-process API call on an unauthenticated standalone `/login`, so `lint:middleware-budget` was re-baselined (`login.tsx` fan-out 2 -> 3, total 541 -> 542); that gate runs only in CI, which is where it was caught. Not changed: ONB-02 (operator-only agent-terms note on `/setup`) | | `wizard` network drop (2026-10-12 QA sweep NET-02; no upstream fix) | `app/routes/inspections.tsx` exports a `clientAction` that runs `guardWizardNetwork` (`app/lib/wizard-submit.ts`): a create whose fetch dies comes back as `{ ok: false, error: { code: "NETWORK" } }`, the shape the wizard already holds open on (`8632b8f7`), so the draft stays and Create is the retry; the agent and client typeaheads get an empty list; any other intent, a redirect or a non-network error is rethrown. The wizard submits nothing when `isOffline()` (`app/lib/network-failure.ts`, which `isNetworkFailure` now also uses) and skips the conflict and holiday loads and the typeahead while offline (read when the debounce fires, not when it is scheduled). Spec: `guardWizardNetwork` in `app/lib/wizard-submit.test.ts`, the offline case in `NewInspectionWizard.test.tsx` | A failed fetcher is routed by React Router to the route's error boundary, which unmounted the wizard with the inspector's draft. `git log main..upstream/main` on the wizard files has only focus-latch and overlay work. The `clientAction` is its own chunk (`splitRouteModules: "enforce"`), fetched on first use, hence the `isOffline()` pre-check. **Known ceiling:** a conflict or holiday load that fails mid-flight while `navigator.onLine` still says true (a bar of signal, no route) still unmounts the wizard. `file-size-baseline.json` raised for `inspections.tsx` (929 -> 934) and `NewInspectionWizard.tsx` (562 -> 571) | -| `editor` session expiry (2026-10-13 QA sweep NET-06; no upstream fix) | `usePresence` (`app/hooks/usePresence.ts`) has a fourth status, `"expired"`: a presence socket that closes without having opened asks `GET /api/auth/me` with the browser's own cookie, and a 401 reads as expired. A 200, an unreachable API or `isOffline()` leaves "Connecting…" / "Reconnecting…". The retries carry on, so logging in from another tab reconnects this one. `MobileSyncPill` and `FooterBar` show that status as `SignInAgainLink`, an anchor to `/login?returnTo=` this page with its `?section=` / `?item=`. The `/login` loader no longer bounces a token past its `exp` to `returnTo` (`isTokenExpired`, now exported from `app/lib/session.server.ts`). Specs: `app/hooks/usePresence.test.ts`, the expired case in `MobileSyncPill.test.tsx`, the NET-06 case in `app/routes/login-saas-bounce.test.ts` | A browser is never told the status of a refused WebSocket handshake, so once the 24-hour token ran out the editor retried under "Reconnecting…" with the edits held in IndexedDB, and the next navigation went to a bare `/login`. `git log main..upstream/main` by keyword (401, session, socket, collab) and on `app/lib/collab` and `app/hooks` has no such change. The check hangs off the presence socket, not the Yjs one: it is the status both readouts already show, and both sockets are refused by the same cookie check (`server/api/inspections/core.ts`, `collab.ts`). The API's JWT middleware lets a request with no cookie through, and `collab/ws` answers a plain GET with 426 before it checks auth, so the socket's own URL cannot be the probe. **Known ceilings:** a token invalidated by a password change but not yet past `exp` is still bounced from `/login` to `returnTo`; under saas `/login` redirects to the portal and drops `returnTo`; on an origin where the browser refuses the `__Host-` cookie (plain http that is not `localhost`) the readout says expired | +| `editor` session expiry (2026-10-13 QA sweep NET-06; no upstream fix) | `usePresence` (`app/hooks/usePresence.ts`) has a fourth status, `"expired"`: a presence socket that closes without having opened asks `GET /api/auth/me` with the browser's own cookie, and a 401 reads as expired. A 200, an unreachable API or `isOffline()` leaves "Connecting…" / "Reconnecting…". The retries carry on, so logging in from another tab reconnects this one. `MobileSyncPill` and `FooterBar` show that status as `SignInAgainLink`, an anchor to `/login?returnTo=` this page with its `?section=` / `?item=`. The `/login` loader no longer bounces a token past its `exp` to `returnTo` (`isTokenExpired`, now exported from `app/lib/session.server.ts`). Specs: `app/hooks/usePresence.test.ts`, the expired case in `MobileSyncPill.test.tsx`, the NET-06 case in `app/routes/login-saas-bounce.test.ts` | A browser is never told the status of a refused WebSocket handshake, so once the 24-hour token ran out the editor retried under "Reconnecting…" with the edits held in IndexedDB, and the next navigation went to a bare `/login`. `git log main..upstream/main` by keyword (401, session, socket, collab) and on `app/lib/collab` and `app/hooks` has no such change. The check hangs off the presence socket, not the Yjs one: it is the status both readouts already show, and both sockets are refused by the same cookie check (`server/api/inspections/core.ts`, `collab.ts`). The API's JWT middleware lets a request with no cookie through, and `collab/ws` answers a plain GET with 426 before it checks auth, so the socket's own URL cannot be the probe. **Known ceilings:** a token invalidated by a password change but not yet past `exp` is still bounced from `/login` to `returnTo`, where the auth layout now ends the session (ONB-04 row below) and the visitor lands on a bare `/login`, `returnTo` dropped; under saas `/login` redirects to the portal and drops `returnTo`; on an origin where the browser refuses the `__Host-` cookie (plain http that is not `localhost`) the readout says expired | +| `auth` stale session ends at /login (2026-10-13 QA sweep ONB-04; no upstream fix) | The loader in `app/routes/auth-layout.tsx` ends the session (`destroyUserSession`: both cookies cleared, 302 to `/login`) when the API answers its session-context call with 401. Only 401: a 5xx or a thrown fetch still falls back to the default shell. Specs: `app/routes/auth-layout.loader.test.ts`; the end-to-end case sits in `tests/e2e/auth-password-reset.spec.ts` (a token from before a reset gets 302 to `/login`) | A token issued before a password change or reset passes `requireToken` (its `exp` is live) and is refused by the API (`pwchanged:{userId}`), but the loader swallowed that refusal as a "graceful fallback", so the page came back 200 with "Unnamed", `openinspection.dev` and no Dispatch, and a reload did the same. `git log main..upstream/main` by keyword (stale session, pwchanged, password reset) and on the auth-boundary files finds only the TOTP and invite-link fixes, neither of which touches this. **Known ceilings:** only a document load runs the layout loader, so a stale session that navigates client-side between child routes sees each child's own best-effort fallback until the next full load; `returnTo` is not carried; no `?reason=` notice on `/login` (the upstream plan floated `password-changed`, which would need a message in both catalogs) | ## Rules diff --git a/tests/e2e/auth-password-reset.spec.ts b/tests/e2e/auth-password-reset.spec.ts index 67b76872..8e189cdf 100644 --- a/tests/e2e/auth-password-reset.spec.ts +++ b/tests/e2e/auth-password-reset.spec.ts @@ -123,6 +123,9 @@ test.describe('Reset password', () => { expect(adminToken, 'admin must authenticate').toBeTruthy(); const joinToken = await inviteMember(request, adminToken, email); await acceptInvite(request, joinToken, initialPassword); + // A session from BEFORE the reset, kept to prove the reset ends it (ONB-04). + const staleToken = await loginApi(request, email, initialPassword); + expect(staleToken, 'initial password must authenticate').toBeTruthy(); // 2. Request a reset through the real UI; the email sink captures the link. await page.goto(`${BASE_URL}/forgot-password`, { timeout: NAV_TIMEOUT, waitUntil: 'networkidle' }); @@ -154,5 +157,20 @@ test.describe('Reset password', () => { headers: { 'Content-Type': 'application/json', ...csrfHeaders().headers }, }); expect(stale.status(), 'old password must be rejected after reset').toBe(401); + + // 7. A session from before the reset is ended at the page, not rendered as a + // default shell (ONB-04). Asserted on status and Location, like + // route-auth-boundary.spec.ts. The fresh session is the control: if every + // cookie bounced to /login this would say nothing about the stale one. + const asPage = (token: string) => + request.get(`${BASE_URL}/inspections`, { + maxRedirects: 0, + headers: { Cookie: `__Host-inspector_token=${token}` }, + }); + const fresh = await asPage(newToken); + expect(fresh.headers()['location'] ?? '', 'a live session must not be bounced').not.toMatch(/\/login/); + const old = await asPage(staleToken); + expect(old.status(), 'a session from before the reset must not render').toBe(302); + expect(old.headers()['location']).toMatch(/\/login/); }); });