From 7bdcbfbcb37c589282dcd7b5411433b6f4f35304 Mon Sep 17 00:00:00 2001 From: Optic00 Date: Sun, 26 Jul 2026 10:51:18 +0200 Subject: [PATCH] fix(web): tell an access refusal apart from an outage Every loader error except AbortError lands in the same catch and returns the last-good data flagged stale, which the connection banner renders as "reconnecting". A 401 or 403 therefore looks exactly like a dead radio: the herd stays on screen, the bar keeps promising a reconnect that will never come, and nothing says what is actually wrong. It is self-sustaining, too. The escalation latch clears only via markLive(), which needs a provably live poll, so under a persistent refusal it never clears, and the root loader's navigation fast path then returns stale data without even attempting a fetch. This is not only a reverse-proxy concern, though that is where it hurts most. The bridge itself answers 403 from checkAccess and guard, for "host not allowed", "cross-origin rejected" and "device not authorised", so a misconfigured COLLIE_PUBLIC_HOSTS or a device that is not on the allowlist presents the same way: reconnecting forever, no diagnosis. The loaders now classify the failure. A 401 or 403 sets a new authError flag alongside the existing error flag, everything else is unchanged. ApiError already carried the status; it was simply never read, so the classification needs no new plumbing beyond a narrow exported predicate. The flag is also remembered per session, because the latched navigation path returns without fetching and would otherwise drop the diagnosis the moment the user navigates. The banner branches on it ahead of the connection state machine: no /api/config probe, no reconnect spinner, no escalation clock, since none of those describe a refusal. The copy deliberately does not name a cause. The flag covers 401 and 403 alike and a 403 has three quite different meanings here, so naming one would be wrong more often than right. What was missing was the single fact the old behaviour hid: this is not the network. Reload is offered because it is what lets a fronting proxy serve its sign-in page. The new field is required rather than optional so that every construction site has to state it, which is why a few unrelated test fixtures gained a line. Scope note: the service worker answers every non-/api/ navigation from the precache, so a proxy's sign-in page at / is still unreachable from an installed PWA. That is a separate question about navigation routing and is deliberately not touched here. --- web/src/components/connection-banner.test.tsx | 25 ++++++++- web/src/components/connection-banner.tsx | 46 +++++++++++++++- web/src/components/update-banner.test.tsx | 1 + .../components/update-check-control.test.tsx | 1 + web/src/hooks/use-polling.test.ts | 1 + web/src/lib/api.ts | 5 ++ web/src/lib/loaders.test.ts | 53 +++++++++++++++++++ web/src/lib/loaders.ts | 42 ++++++++++++++- web/src/routes/detail.test.tsx | 2 + web/src/routes/root.tsx | 2 +- 10 files changed, 172 insertions(+), 6 deletions(-) diff --git a/web/src/components/connection-banner.test.tsx b/web/src/components/connection-banner.test.tsx index 3384ba2..13d9c35 100644 --- a/web/src/components/connection-banner.test.tsx +++ b/web/src/components/connection-banner.test.tsx @@ -33,12 +33,18 @@ function setOnline(value: boolean) { // hooks are re-read) — RouterProvider re-rendered with the same static route element would bail out. let rerenderBanner: () => void = () => {}; -function renderBanner(props: { bridge?: "connected" | "disconnected"; error?: boolean } = {}) { +function renderBanner( + props: { bridge?: "connected" | "disconnected"; error?: boolean; authError?: boolean } = {}, +) { function Harness() { const [, setN] = useState(0); rerenderBanner = () => setN((n) => n + 1); return ( - + ); } const router = createMemoryRouter([{ path: "/", element: }]); @@ -59,6 +65,21 @@ afterEach(() => { }); describe("ConnectionBanner — the single connection surface", () => { + it("shows the auth refusal with Reload and no connection treatment", () => { + h.trouble = true; + h.lost = true; + renderBanner({ authError: true }); + + expect(screen.getByRole("alert")).toHaveTextContent( + "Access refused. This is not a connection problem.", + ); + expect(screen.getByRole("button", { name: "Reload" })).toBeInTheDocument(); + expect(screen.queryByText("Reconnecting…")).toBeNull(); + expect(screen.queryByRole("button", { name: "Retry" })).toBeNull(); + expect(document.querySelector(".animate-spin")).toBeNull(); + expect(cfg.calls).toBe(0); + }); + it("renders nothing while healthy — no bar at all", () => { renderBanner({ bridge: "connected" }); expect(screen.queryByRole("status")).toBeNull(); diff --git a/web/src/components/connection-banner.tsx b/web/src/components/connection-banner.tsx index 0e816cb..ff17924 100644 --- a/web/src/components/connection-banner.tsx +++ b/web/src/components/connection-banner.tsx @@ -16,6 +16,8 @@ interface ConnectionBannerProps { bridge: BridgeStatus | undefined; /** The last snapshot fetch failed (stale data on screen). */ error: boolean; + /** The failed snapshot request was rejected with HTTP 401 or 403. */ + authError: boolean; } // The result of the /api/config probe (which never touches Herdr): "unknown" until it resolves, @@ -40,7 +42,49 @@ export const EXIT_MS = 200; // disagree; `connecting` is poll-truth (isConnecting) — navigator.onLine is COPY-only (it picks the // red cause), never a gate. Threshold lockstep with the shared clock is proven in use-connection-lost; // here we own the amber→red→green state machine and the smooth mount/unmount. -export function ConnectionBanner({ bridge, error }: ConnectionBannerProps) { +export function ConnectionBanner({ bridge, error, authError }: ConnectionBannerProps) { + if (authError) return ; + return ; +} + +// A refusal is not an outage, so it gets its own surface ahead of the connection state machine: no +// probe, no reconnect spinner, no escalation clock. The copy stays deliberately non-specific about +// the cause. The flag covers 401 and 403 alike, and a 403 can equally mean "this device is not +// allowlisted", "host not allowed" or "cross-origin rejected", so naming any one of them would be +// wrong more often than right. What the operator needs here is the one fact the old behaviour hid: +// this is not the network. Reload is the useful action either way, because it is what lets a +// fronting proxy serve its sign-in page. +function AuthErrorBanner() { + return ( +
+
+
+ + + Access refused. This is not a connection problem. + + +
+
+
+ ); +} + +function ConnectionStateBanner({ bridge, error }: Omit) { const stalled = useLoadingStalled(); const connecting = isConnecting({ bridge, error, stalled }); const trouble = useConnectionTrouble(connecting); diff --git a/web/src/components/update-banner.test.tsx b/web/src/components/update-banner.test.tsx index 4974d5d..a60b973 100644 --- a/web/src/components/update-banner.test.tsx +++ b/web/src/components/update-banner.test.tsx @@ -62,6 +62,7 @@ function homeData(update: UpdateInfo | undefined): HomeData { snoozedUntil: null, update, error: false, + authError: false, }; } diff --git a/web/src/components/update-check-control.test.tsx b/web/src/components/update-check-control.test.tsx index 1089060..95b5a60 100644 --- a/web/src/components/update-check-control.test.tsx +++ b/web/src/components/update-check-control.test.tsx @@ -35,6 +35,7 @@ function homeData(update: UpdateInfo | undefined): HomeData { snoozedUntil: null, update, error: false, + authError: false, }; } diff --git a/web/src/hooks/use-polling.test.ts b/web/src/hooks/use-polling.test.ts index 927d95a..1ca52e6 100644 --- a/web/src/hooks/use-polling.test.ts +++ b/web/src/hooks/use-polling.test.ts @@ -57,6 +57,7 @@ function makeData(agents: AgentView[], shellPanes: AgentView[] = []): HomeData { snoozedUntil: null, update: undefined, error: false, + authError: false, }; } diff --git a/web/src/lib/api.ts b/web/src/lib/api.ts index e237e9f..94b86e1 100644 --- a/web/src/lib/api.ts +++ b/web/src/lib/api.ts @@ -26,6 +26,11 @@ class ApiError extends Error { } } +/** True when an API request failed with the given HTTP status. */ +export function isApiErrorStatus(error: unknown, status: number): boolean { + return error instanceof ApiError && error.status === status; +} + // Every request gets a deadline so a black-holed connection (phone sleep/wake, a Tailscale route // that goes dark) can't leave a fetch pending forever — which would zombify the app: the poller // gates on `revalidator.state === "idle"` and never fires again, and route navigations wait on a diff --git a/web/src/lib/loaders.test.ts b/web/src/lib/loaders.test.ts index 025d5f6..6969120 100644 --- a/web/src/lib/loaders.test.ts +++ b/web/src/lib/loaders.test.ts @@ -16,18 +16,33 @@ afterEach(() => { const failSnapshot = () => server.use(http.get("/api/snapshot", () => new HttpResponse(null, { status: 500 }))); +const rejectSnapshot = (status: 401 | 403) => + server.use(http.get("/api/snapshot", () => new HttpResponse(null, { status }))); + const failPane = () => server.use(http.get(/\/api\/pane\/[^/]+$/, () => new HttpResponse(null, { status: 500 }))); +const rejectPane = (status: 401 | 403) => + server.use(http.get(/\/api\/pane\/[^/]+$/, () => new HttpResponse(null, { status }))); + describe("rootLoader", () => { it("returns the live snapshot on success", async () => { const { rootLoader } = await import("./loaders"); const data = await rootLoader(); expect(data.error).toBe(false); + expect(data.authError).toBe(false); expect(data.bridge).toBe("connected"); expect(data.agents).toHaveLength(2); }); + it.each([401, 403] as const)("marks a %i response as an auth error", async (status) => { + rejectSnapshot(status); + const { rootLoader } = await import("./loaders"); + const data = await rootLoader(); + expect(data.error).toBe(true); + expect(data.authError).toBe(true); + }); + it("keeps the last-good herd (flagged error) when a refresh fails", async () => { const { rootLoader } = await import("./loaders"); await rootLoader(); // prime the cache with a good snapshot @@ -36,11 +51,20 @@ describe("rootLoader", () => { const stale = await rootLoader(); expect(stale.error).toBe(true); + expect(stale.authError).toBe(false); expect(stale.bridge).toBe("connected"); // from the cached snapshot expect(stale.agents).toHaveLength(2); expect(stale.agents[0]!.paneId).toBe(fixtureAgents[0]!.paneId); }); + it("does not mark a network error as an auth error", async () => { + const { rootLoader } = await import("./loaders"); + vi.spyOn(globalThis, "fetch").mockRejectedValue(new TypeError("network failed")); + const data = await rootLoader(); + expect(data.error).toBe(true); + expect(data.authError).toBe(false); + }); + it("returns empty + error when there is no last-good snapshot", async () => { failSnapshot(); const { rootLoader } = await import("./loaders"); @@ -59,6 +83,7 @@ describe("rootLoader", () => { vi.spyOn(globalThis, "fetch").mockRejectedValue(new DOMException("timed out", "TimeoutError")); const data = await rootLoader(); expect(data.error).toBe(true); + expect(data.authError).toBe(false); expect(data.bridge).toBeUndefined(); expect(data.agents).toEqual([]); }); @@ -91,10 +116,19 @@ describe("paneLoader", () => { const { paneLoader } = await import("./loaders"); const data = await paneLoader({ params: { paneId: "w1:p1" } }); expect(data.error).toBe(false); + expect(data.authError).toBe(false); expect(data.paneId).toBe("w1:p1"); expect(data.text).toBe("hello from the pane"); }); + it.each([401, 403] as const)("marks a %i response as an auth error", async (status) => { + rejectPane(status); + const { paneLoader } = await import("./loaders"); + const data = await paneLoader({ params: { paneId: "w1:p1" } }); + expect(data.error).toBe(true); + expect(data.authError).toBe(true); + }); + it("keeps the last-good pane text (flagged error) when a refresh fails", async () => { const { paneLoader } = await import("./loaders"); await paneLoader({ params: { paneId: "w1:p1" } }); // prime per-pane cache @@ -103,6 +137,7 @@ describe("paneLoader", () => { const stale = await paneLoader({ params: { paneId: "w1:p1" } }); expect(stale.error).toBe(true); + expect(stale.authError).toBe(false); expect(stale.text).toBe("hello from the pane"); expect(stale.paneId).toBe("w1:p1"); }); @@ -128,6 +163,7 @@ describe("paneLoader", () => { const stale = await paneLoader({ params: { paneId: "w1:p1" } }); expect(stale.error).toBe(true); + expect(stale.authError).toBe(false); expect(stale.text).toBe("hello from the pane"); expect(stale.paneId).toBe("w1:p1"); }); @@ -275,6 +311,23 @@ describe("loaders — offline navigation fast path", () => { expect(data.agents).toHaveLength(2); }); + it("keeps the last auth classification on the navigation fast path", async () => { + rejectSnapshot(401); + const { rootLoader } = await import("./loaders"); + const { latchLost } = await import("./connection-health"); + + const rejected = await rootLoader({ request: new Request("http://localhost/") }); + expect(rejected.authError).toBe(true); + latchLost(); + + const fetchSpy = vi.spyOn(globalThis, "fetch"); + const data = await rootLoader({ request: new Request("http://localhost/space/w1") }); + + expect(fetchSpy).not.toHaveBeenCalled(); + expect(data.error).toBe(true); + expect(data.authError).toBe(true); + }); + it("a revalidation (same url) still really fetches while latched — polls keep probing", async () => { const { rootLoader } = await import("./loaders"); const { latchLost } = await import("./connection-health"); diff --git a/web/src/lib/loaders.ts b/web/src/lib/loaders.ts index 38fef17..b71b66d 100644 --- a/web/src/lib/loaders.ts +++ b/web/src/lib/loaders.ts @@ -13,7 +13,7 @@ // no flag, no race — and because a navigation aborts any in-flight revalidation, the nav is instant // even while a poll's doomed fetch is still hanging. -import { fetchPane, fetchSnapshot } from "@/lib/api"; +import { fetchPane, fetchSnapshot, isApiErrorStatus } from "@/lib/api"; import { isLostLatched } from "@/lib/connection-health"; import { SESSION_PARAM, normalizeSession } from "@/lib/session"; import type { @@ -75,6 +75,8 @@ export interface HomeData { update: UpdateInfo | undefined; /** True when this render is the last-good snapshot after a failed refresh. */ error: boolean; + /** True when the failed refresh was rejected with HTTP 401 or 403. */ + authError: boolean; } export interface PaneData { @@ -91,12 +93,32 @@ export interface PaneData { * the degraded (stale-text) path, where the guard's fresh fetch will reject a mismatch anyway. */ revision: number; error: boolean; + /** True when the failed refresh was rejected with HTTP 401 or 403. */ + authError: boolean; } // Keep-previous-data cache is now PER-SESSION: switching sessions must not show the other session's // herd flagged as stale. Keyed by session name ("" = primary). const lastSnapshot = new Map(); +// A latched navigation skips the network, so retain whether the last real outcome for each session +// was an auth rejection. Store only rejected sessions; every other real outcome removes the marker. +const authErrorSessions = new Set(); + +function rememberAuthError(session: string | undefined, authError: boolean): void { + const key = session ?? ""; + if (authError) authErrorSessions.add(key); + else authErrorSessions.delete(key); +} + +function hasAuthError(session: string | undefined): boolean { + return authErrorSessions.has(session ?? ""); +} + +function isAuthError(error: unknown): boolean { + return isApiErrorStatus(error, 401) || isApiErrorStatus(error, 403); +} + // The URL each loader last RAN for — the nav-vs-revalidate discriminator for the offline fast path (see // the header comment). Module-scoped so it survives revalidations (the loader re-runs every poll) and // resets on a full reload — same lifetime as the caches. `lastRootUrl` is enough for the root loader @@ -128,6 +150,7 @@ function toHomeData(snap: SnapshotResponse, session: string | undefined, error: snoozedUntil: snap.notifications?.snoozedUntil ?? null, update: snap.update, error, + authError: error && hasAuthError(session), }; } @@ -151,6 +174,7 @@ function staleHome(session: string | undefined): HomeData { snoozedUntil: null, update: undefined, error: true, + authError: hasAuthError(session), }; } @@ -173,9 +197,11 @@ export async function rootLoader({ request }: { request?: Request } = {}): Promi try { const snap = await fetchSnapshot(session, request?.signal); lastSnapshot.set(session ?? "", snap); + rememberAuthError(session, false); return toHomeData(snap, session, false); } catch (e) { if (isAbortError(e)) throw e; // superseded revalidation — let React Router drop it + rememberAuthError(session, isAuthError(e)); // Keep the last good herd on screen, flagged so the ConnectionBanner can say "reconnecting…". return staleHome(session); } @@ -254,6 +280,7 @@ function stalePane(paneId: string, session: string | undefined, lines: number): requestedLines: lines, revision: 0, error: true, + authError: hasAuthError(session), }; } @@ -289,9 +316,20 @@ export async function paneLoader({ const read: PaneReadResponse = await fetchPane(paneId, lines, session, request?.signal); const text = read.text || lastPaneText.get(key) || ""; rememberPaneText(key, text); - return { paneId, session, text, truncated: read.truncated, requestedLines: lines, revision: read.revision, error: false }; + rememberAuthError(session, false); + return { + paneId, + session, + text, + truncated: read.truncated, + requestedLines: lines, + revision: read.revision, + error: false, + authError: false, + }; } catch (e) { if (isAbortError(e)) throw e; // superseded revalidation — let React Router drop it + rememberAuthError(session, isAuthError(e)); // Genuine network / server failure: show stale text flagged as degraded. return stalePane(paneId, session, lines); } diff --git a/web/src/routes/detail.test.tsx b/web/src/routes/detail.test.tsx index eebd808..dfb1161 100644 --- a/web/src/routes/detail.test.tsx +++ b/web/src/routes/detail.test.tsx @@ -46,6 +46,7 @@ const connected = (agents: AgentView[], shellPanes: AgentView[] = []): HomeData snoozedUntil: null, update: undefined, error: false, + authError: false, }); function makeRouter(initialPath: string, homeLoader: () => HomeData) { @@ -68,6 +69,7 @@ function makeRouter(initialPath: string, homeLoader: () => HomeData) { requestedLines: 600, revision: 0, error: false, + authError: false, }), element: , }, diff --git a/web/src/routes/root.tsx b/web/src/routes/root.tsx index 9540d59..7ab2ded 100644 --- a/web/src/routes/root.tsx +++ b/web/src/routes/root.tsx @@ -43,7 +43,7 @@ export function RootLayout() { in amber "reconnecting…" only after ≥4s of sustained trouble (the flicker fix), escalates to a red "not connected" cause + Retry/Reload at ≥15s, and flashes green on recovery. Reads the same shared-clock signals as the header dog, so the two always agree. */} - + );