From 2528706c918f25fbfa87cc91e393098e6e2e7351 Mon Sep 17 00:00:00 2001 From: Amit Kumar Date: Fri, 24 Apr 2026 18:12:13 +0000 Subject: [PATCH] fix(ui): route MCP console 401s through shared auth banner (RAN-36) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace raw fetch() in useMCP with mcpRequest() so a 401 on /mcp flips the same auth store /api/* uses. This makes the AuthRequiredBanner render on the MCP route when the session expires, instead of leaving the 401 stuck in local toolsError / per-call error state. - api-client: extract rawRequest helper that defaults Content-Type, attaches credentials, and signals useAuthStore on 401. apiFetch reuses it; new mcpRequest export returns the raw Response so MCP can read Mcp-Session-Id and parse SSE bodies the JSON shape can't expose. - useMCP: rpc() routes through mcpRequest; drops the now-redundant Content-Type / credentials it inherits from the helper. - Providers: keep the QueryCache/MutationCache 401 gate as a defensive layer; comment updated to reflect that the HTTP boundary is now the primary signal (signalUnauthorized is idempotent). - Tests: api-client unit tests cover apiFetch + mcpRequest 401 → store flip; new MCPConsole.test renders the route with /mcp returning 401 and asserts AuthRequiredBanner appears. Co-Authored-By: Paperclip --- ui/src/components/layout/Providers.tsx | 13 ++-- ui/src/hooks/api/useMCP.ts | 8 ++- ui/src/lib/__tests__/api-client.test.ts | 70 ++++++++++++++++++++- ui/src/lib/api-client.ts | 32 ++++++++-- ui/src/routes/__tests__/MCPConsole.test.tsx | 43 +++++++++++++ 5 files changed, 151 insertions(+), 15 deletions(-) create mode 100644 ui/src/routes/__tests__/MCPConsole.test.tsx diff --git a/ui/src/components/layout/Providers.tsx b/ui/src/components/layout/Providers.tsx index 981bb47..5323221 100644 --- a/ui/src/components/layout/Providers.tsx +++ b/ui/src/components/layout/Providers.tsx @@ -9,12 +9,13 @@ import { BrowserRouter } from "react-router-dom"; import { useUIStore } from "@/stores/ui"; import { useAuthStore } from "@/stores/auth"; -// Global 401 gate: any /api/* fetch that throws an ApiErrorResponse -// with status === 401 flips the auth store so AuthRequiredBanner can -// render a visible "Sign in required" affordance. Wired to BOTH -// QueryCache and MutationCache — a 401 on a write action (note -// create/update/delete) must surface the banner just the same as a -// read-path failure. +// Defensive 401 gate. The HTTP boundary (apiFetch / mcpRequest in +// lib/api-client.ts) is the primary signal for AuthRequiredBanner — it +// already flips the auth store on every 401 it sees. This React Query +// hook is kept so any consumer that synthesises an ApiErrorResponse +// outside the fetch path (e.g. tests, future non-HTTP transports) still +// surfaces the banner. signalUnauthorized is idempotent, so the +// double-signal on real /api/* 401s is harmless. function gateUnauthorized(error: unknown) { const status = (error as { status?: number })?.status ?? 0; if (status === 401) { diff --git a/ui/src/hooks/api/useMCP.ts b/ui/src/hooks/api/useMCP.ts index c35f37e..b519faa 100644 --- a/ui/src/hooks/api/useMCP.ts +++ b/ui/src/hooks/api/useMCP.ts @@ -1,4 +1,5 @@ import { useState, useCallback, useRef, useEffect } from "react"; +import { mcpRequest } from "@/lib/api-client"; export interface MCPCallRecord { id: string; @@ -25,14 +26,15 @@ async function rpc( body: unknown, ): Promise<{ json: unknown; sessionId: string | null }> { const headers: Record = { - "Content-Type": "application/json", "Accept": "application/json, text/event-stream", }; if (sessionId) headers["Mcp-Session-Id"] = sessionId; - const res = await fetch("/mcp", { + // Routed through mcpRequest so 401s flip the shared auth store and + // surface the AuthRequiredBanner on /mcp instead of being swallowed + // into local toolsError / per-call error state. + const res = await mcpRequest("/mcp", { method: "POST", - credentials: "include", headers, body: JSON.stringify(body), }); diff --git a/ui/src/lib/__tests__/api-client.test.ts b/ui/src/lib/__tests__/api-client.test.ts index 076a13c..85ddfd5 100644 --- a/ui/src/lib/__tests__/api-client.test.ts +++ b/ui/src/lib/__tests__/api-client.test.ts @@ -1,7 +1,15 @@ -import { describe, it, expect, vi } from "vitest"; +import { afterEach, describe, it, expect, vi } from "vitest"; import { http, HttpResponse } from "msw"; import { server } from "@/test/msw"; -import { apiFetch, ApiErrorResponse, initAuth } from "../api-client"; +import { apiFetch, ApiErrorResponse, initAuth, mcpRequest } from "../api-client"; +import { useAuthStore } from "@/stores/auth"; + +afterEach(() => { + // Auth store is module-level; reset after every test or a 401 in one + // case leaks `authRequired = true` into the next assertion. No render + // happens here, so we mutate the store directly without React.act. + useAuthStore.getState().clear(); +}); describe("apiFetch", () => { it("returns parsed json on 200", async () => { @@ -114,6 +122,17 @@ describe("apiFetch", () => { } }); + it("flips the shared auth store on a 401 (so AuthRequiredBanner renders)", async () => { + server.use( + http.get("/api/expired", () => + HttpResponse.json({ error: "no session" }, { status: 401 }), + ), + ); + expect(useAuthStore.getState().authRequired).toBe(false); + await expect(apiFetch("/api/expired")).rejects.toBeInstanceOf(ApiErrorResponse); + expect(useAuthStore.getState().authRequired).toBe(true); + }); + it("does not set Authorization header on data-path fetch even when a key exists in a meta tag", async () => { const meta = document.createElement("meta"); meta.setAttribute("name", "docsiq-api-key"); @@ -139,3 +158,50 @@ describe("apiFetch", () => { } }); }); + +describe("mcpRequest", () => { + it("returns the raw Response so MCP can read Mcp-Session-Id and SSE bodies", async () => { + server.use( + http.post("/mcp", () => + HttpResponse.json( + { jsonrpc: "2.0", id: 1, result: { ok: true } }, + { headers: { "Mcp-Session-Id": "sess-42" } }, + ), + ), + ); + const res = await mcpRequest("/mcp", { + method: "POST", + body: JSON.stringify({ jsonrpc: "2.0", id: 1, method: "ping" }), + }); + expect(res.status).toBe(200); + expect(res.headers.get("Mcp-Session-Id")).toBe("sess-42"); + expect(useAuthStore.getState().authRequired).toBe(false); + }); + + it("flips the shared auth store on a 401 so the /mcp surface honours AuthRequiredBanner", async () => { + server.use( + http.post("/mcp", () => + HttpResponse.json({ error: "session expired" }, { status: 401 }), + ), + ); + expect(useAuthStore.getState().authRequired).toBe(false); + const res = await mcpRequest("/mcp", { method: "POST", body: "{}" }); + expect(res.status).toBe(401); + expect(useAuthStore.getState().authRequired).toBe(true); + }); + + it("sends credentials: 'include' and defaults Content-Type for string bodies", async () => { + const spy = vi.spyOn(globalThis, "fetch").mockResolvedValue( + new Response("{}", { status: 200, headers: { "content-type": "application/json" } }), + ); + try { + await mcpRequest("/mcp", { method: "POST", body: JSON.stringify({}) }); + const init = (spy.mock.calls[0][1] ?? {}) as RequestInit; + expect(init.credentials).toBe("include"); + const hdrs = new Headers(init.headers); + expect(hdrs.get("Content-Type")).toBe("application/json"); + } finally { + spy.mockRestore(); + } + }); +}); diff --git a/ui/src/lib/api-client.ts b/ui/src/lib/api-client.ts index fb4b18c..e09b6e9 100644 --- a/ui/src/lib/api-client.ts +++ b/ui/src/lib/api-client.ts @@ -1,4 +1,5 @@ import type { ApiError } from "@/types/api"; +import { useAuthStore } from "@/stores/auth"; // Before cookies are set the first time, the UI may have been shipped a // one-shot bearer via the meta tag (legacy). We exchange it for a cookie @@ -59,16 +60,29 @@ function isBrowserManagedBody(body: BodyInit): boolean { return false; } -export async function apiFetch( - path: string, - init: RequestInit = {}, -): Promise { +// Shared low-level request helper. Centralises the auth-required signal +// so a 401 from any HTTP surface (apiFetch on /api/*, mcpRequest on +// /mcp/*) flips the same auth store the AuthRequiredBanner reads from. +// Returns the raw Response so callers that need headers or non-JSON +// framing (e.g. MCP's SSE event-stream + Mcp-Session-Id) can handle it. +async function rawRequest(path: string, init: RequestInit = {}): Promise { if (sessionReady) await sessionReady; const headers = new Headers(init.headers); if (init.body && !headers.has("Content-Type") && !isBrowserManagedBody(init.body)) { headers.set("Content-Type", "application/json"); } const res = await fetch(path, { ...init, headers, credentials: "include" }); + if (res.status === 401) { + useAuthStore.getState().signalUnauthorized(); + } + return res; +} + +export async function apiFetch( + path: string, + init: RequestInit = {}, +): Promise { + const res = await rawRequest(path, init); if (!res.ok) { let body: ApiError = { error: `HTTP ${res.status}` }; try { @@ -81,3 +95,13 @@ export async function apiFetch( if (res.status === 204) return undefined as T; return res.json() as Promise; } + +// MCP can't use apiFetch() directly because it needs Mcp-Session-Id from +// response headers and may receive an SSE text/event-stream body that the +// generic JSON shape can't expose. mcpRequest mirrors apiFetch's +// session-bootstrap, Content-Type defaulting, credentials, and shared +// 401 → auth-store gate, then hands the caller the raw Response for +// MCP-specific framing. +export async function mcpRequest(path: string, init: RequestInit = {}): Promise { + return rawRequest(path, init); +} diff --git a/ui/src/routes/__tests__/MCPConsole.test.tsx b/ui/src/routes/__tests__/MCPConsole.test.tsx new file mode 100644 index 0000000..ddc02be --- /dev/null +++ b/ui/src/routes/__tests__/MCPConsole.test.tsx @@ -0,0 +1,43 @@ +import { afterEach, describe, expect, it } from "vitest"; +import { act } from "react"; +import { render, screen, waitFor } from "@testing-library/react"; +import { MemoryRouter } from "react-router-dom"; +import { http, HttpResponse } from "msw"; +import { server } from "@/test/msw"; +import MCPConsole from "@/routes/MCPConsole"; +import { AuthRequiredBanner } from "@/components/layout/AuthRequiredBanner"; +import { useAuthStore } from "@/stores/auth"; + +afterEach(() => { + // Auth store is module-level — without a reset between tests the 401 + // banner from a prior assertion leaks into the next render. + act(() => useAuthStore.getState().clear()); +}); + +describe("MCPConsole route", () => { + it("renders the shared AuthRequiredBanner when /mcp returns 401", async () => { + server.use( + http.post("/mcp", () => + HttpResponse.json( + { error: "session expired" }, + { status: 401, headers: { "Content-Type": "application/json" } }, + ), + ), + ); + + render( + + + + , + ); + + // The hook fires `initialize` on mount; the 401 must flip the auth + // store via mcpRequest so the shared banner renders on this route. + const banner = await screen.findByTestId("auth-required-banner"); + expect(banner).toBeInTheDocument(); + expect(screen.getByText(/sign in required/i)).toBeInTheDocument(); + + await waitFor(() => expect(useAuthStore.getState().authRequired).toBe(true)); + }); +});