Harden VNC UI setup API calls against expired auth redirects

[jamesachurchill]

Summary

The VNC setup UI should handle expired setup sessions cleanly when API calls return 401 or an HTML login redirect instead of the expected JSON payload.

Problem

Several VNC UI actions expect JSON from setup API endpoints:

• VNC availability probing;
• VNC install/repair;
• reboot request after repair;
• VNC clipboard read/write.

If the setup session expires, these calls can receive a 401 or a redirect to /login. Without explicit handling, the UI may attempt to parse the wrong response shape, surface a confusing error, or silently fail.

Proposed shape

• Send Accept: application/json on VNC setup API calls.
• Detect 401 responses.
• Detect redirected responses whose final path is /login.
• Show a clear expired-login message asking the user to refresh/sign in again.
• Avoid continuing repair/reboot/poll loops after auth expiry is detected.

Acceptance criteria

• VNC availability check surfaces a clear login-expired state on 401 or login redirect.
• VNC repair action stops and shows a clear error on auth expiry.
• Reboot request after repair stops and shows a clear error on auth expiry.
• VNC clipboard read/write does not try to parse a login page as JSON.
• Unit or component tests cover at least one 401 path and one redirected-login path.

[jamesachurchill]

Review

Endorsed — the proposed shape is correct, and it's cheaper than it looks because the middleware already does the hard half. Findings below, then implementation recommendations.

Findings

1. Middleware already branches on Accept — the header is the keystone change [repo-grounded]
src/middleware.ts step 5: requests whose Accept includes application/json get { error: "Authentication required" } with status 401; everything else gets NextResponse.redirect(/login?redirect=…). None of the VNC UI fetches currently set Accept, so they all fall into the redirect branch. Adding Accept: application/json flips every call site into a clean, machine-detectable 401 with no server changes.

2. Current failure modes split by HTTP method [repo-grounded]

• GETs (checkVnc, clipboard read, reboot poll): the browser follows the redirect to /login, which returns 200 HTML. res.ok is true, so res.json() either throws (checkVnc surfaces Unexpected token '<'… as the error text) or is swallowed by .catch(() => ({})) (clipboard read fails silently — exactly the issue's fourth bullet).
• POSTs (run-step, system/power, clipboard write): NextResponse.redirect defaults to 307, the browser replays the POST against /login, and it surfaces as 405. The middleware's own MCP-bypass comment confirms this shape ("POSTs surface as 405, GETs return HTML that JSON.parse chokes on"). Users currently see vnc_install failed (HTTP 405) / Clipboard write failed (HTTP 405) — misleading.

3. The Accept header alone does not fix the post-reboot poll loop [repo-grounded]
In handleRepairAndReboot, the poll does if (!probe.ok) continue;. A 401 just keeps the loop spinning for the full 5-minute timeout, then shows "Reboot timed out waiting for the VNC service" — wrong diagnosis. The loop needs an explicit auth-expiry break. This is the one acceptance criterion the header change doesn't cover on its own.

4. Session survives reboot — auth expiry mid-repair is bounded, not default [repo-grounded]
production-server.js persists data/.session-secret (mode 0600) and reloads it on boot, and the cookie carries an absolute exp (src/lib/auth.ts). So repair→reboot does not inherently invalidate the session; the failure case here is the cookie's exp lapsing during the window. Worth handling (per finding 3) but not the common path.

5. Two affected endpoints are shared, not VNC-specific [repo-grounded]
/setup-api/install/run-step and /setup-api/system/power are used beyond the VNC panel. That argues for one shared helper rather than six inline patches — other panels inherit the fix for free later.

Recommended shape

Add a small helper in src/lib (e.g. fetch-setup-json.ts) used by all six call sites in VNCApp.tsx (checkVnc, repair POST, reboot POST, poll probe, clipboard GET, clipboard POST):

export type SetupFetchResult<T> =
  | { kind: "ok"; data: T; res: Response }
  | { kind: "auth-expired" }
  | { kind: "error"; res: Response };

export async function fetchSetupJson<T>(input: string, init?: RequestInit): Promise<SetupFetchResult<T>> {
  const res = await fetch(input, {
    ...init,
    headers: { Accept: "application/json", ...(init?.headers ?? {}) },
  });
  if (res.status === 401) return { kind: "auth-expired" };
  if (res.redirected && new URL(res.url).pathname === "/login") return { kind: "auth-expired" };
  if (!res.ok) return { kind: "error", res };
  return { kind: "ok", data: await res.json().catch(() => ({} as T)), res };
}

• res.redirected + final-URL check covers both redirect shapes: GET→200 HTML at /login and POST→405 at /login (the browser exposes the final URL either way). [reviewer judgment]
• Avoid redirect: "manual" — it yields an opaqueredirect response that hides status and destination, making the check less precise, and it changes behaviour for any legitimate redirects. [reviewer judgment]
• Belt-and-braces: treating a content-type that isn't application/json on a 2xx as auth-expired-or-error would also catch any future proxy that rewrites responses, but the two checks above are sufficient for the shapes this stack actually produces. Optional. [reviewer judgment]

Component behaviour on auth-expired:

• checkVnc → setStatus("error") with a distinct login-expired message (and ideally a distinct state so the UI can render a "Sign in again" affordance rather than the generic VNC-error panel).
• Repair / reboot → setRepairState("failed") + login-expired message; do not proceed to the reboot POST if the repair POST already returned auth-expired.
• Poll loop → break immediately on auth-expired with the login-expired message instead of continue-ing to timeout (finding 3).
• Clipboard read → surface the expired state (a one-shot toast is enough) rather than the current silent return; clipboard write → replace the HTTP-405 message with the login-expired one.
• Consider debouncing/latching the expired state so the noVNC clipboard event storm doesn't re-fire the message repeatedly. [reviewer judgment]

Tests

src/tests/components/vnc-app.test.tsx already stubs global fetch with vi.stubGlobal, so both required cases slot in cleanly [repo-grounded]:

• 401 path: stub resolves { ok: false, status: 401, redirected: false, json: … } → assert login-expired message renders and repair/poll stops.
• Redirected-login path: stub resolves { ok: true, status: 200, redirected: true, url: "http://box/login?redirect=…", json: () => Promise.reject(…) } → assert same.
• Worth one extra case: poll loop receives auth-expired mid-poll and exits before the 5-minute timeout (fake timers already available via vitest).

Acceptance criteria — one addition suggested

The five listed criteria are all reachable with the shape above. Suggest adding: "the post-repair reboot poll loop terminates promptly on auth expiry rather than running to the 5-minute timeout" — it's implied by criterion 3 but is the one spot where the fix isn't just the shared helper.

[KrasimirKralev]

Triage: bug (user-visible breakage on session expiry). The VNC panel's fetches assume JSON and choke on the middleware's login redirect — same failure class we've hardened elsewhere. Fix shape: a shared fetchSetupApi() helper that detects 401/HTML and surfaces a re-login prompt instead of a parse error.

[KrasimirKralev]

Verified against the code — VALID, with one precision: on session expiry, /setup-api/* calls without an Accept: application/json header get a 307 HTML redirect to /login (middleware only returns 401 JSON for Accept: application/json or /api/* paths), and VNCApp sends no Accept header on any call. Per-flow:

• checkVnc (availability probe): literally crashes — no res.ok check, res.json() parses the login HTML → parse error surfaced to the user
• install/repair + reboot POSTs: parse-guarded but show a generic HTTP nnn with no expired-login detection
• reboot poll loop: the /login redirect returns 200 HTML → probe.ok is true → polls to the full 5-minute timeout instead of halting
• clipboard read/write: parse-guarded, same missing detection

Quick wins: send Accept: application/json on all VNC calls (flips middleware to clean 401s), then a small shared helper that detects 401/res.redirected and surfaces a re-login prompt + halts poll loops.