From a17d2ff8f8424ac4479601e8c57edd1ece03adc7 Mon Sep 17 00:00:00 2001 From: Huixin615 Date: Sat, 13 Jun 2026 07:36:57 +0800 Subject: [PATCH] fix(mcp): surface admin-required state on settings tools page (#3527) (#3533) GET /api/mcp/config returns 403 for non-admin users, but the previous client returned the error body as MCPConfig, causing MCPServerList to crash with 'Cannot convert undefined or null to object' on Object.entries(config.mcp_servers). - api.ts: introduce MCPConfigRequestError; loadMCPConfig and updateMCPConfig now throw it (carrying status + isAdminRequired) instead of letting non-2xx bodies leak through as parsed config - tool-settings-page.tsx: render a friendly 'admin privileges required' empty state when the React Query error is an admin-required MCPConfigRequestError; keep MCPServerList resilient with Object.entries(servers ?? {}) and an empty-state for no servers - i18n: add settings.tools.adminRequired and settings.tools.empty in en-US, zh-CN and the Translations type - tests: cover 403 / 5xx / instanceof / detail-fallback for both loadMCPConfig and updateMCPConfig in tests/unit/core/mcp/api.test.ts Refs: #3527 --- .../workspace/settings/tool-settings-page.tsx | 20 ++- frontend/src/core/i18n/locales/en-US.ts | 2 + frontend/src/core/i18n/locales/types.ts | 2 + frontend/src/core/i18n/locales/zh-CN.ts | 2 + frontend/src/core/mcp/api.ts | 34 +++++ frontend/src/core/mcp/hooks.ts | 4 +- frontend/tests/unit/core/mcp/api.test.ts | 118 ++++++++++++++++++ frontend/tests/unit/core/mcp/hooks.test.ts | 84 +++++++++++++ 8 files changed, 263 insertions(+), 3 deletions(-) create mode 100644 frontend/tests/unit/core/mcp/api.test.ts create mode 100644 frontend/tests/unit/core/mcp/hooks.test.ts diff --git a/frontend/src/components/workspace/settings/tool-settings-page.tsx b/frontend/src/components/workspace/settings/tool-settings-page.tsx index dd3cd0d9d..ce5746373 100644 --- a/frontend/src/components/workspace/settings/tool-settings-page.tsx +++ b/frontend/src/components/workspace/settings/tool-settings-page.tsx @@ -9,6 +9,7 @@ import { } from "@/components/ui/item"; import { Switch } from "@/components/ui/switch"; import { useI18n } from "@/core/i18n/hooks"; +import { MCPConfigRequestError } from "@/core/mcp/api"; import { useMCPConfig, useEnableMCPServer } from "@/core/mcp/hooks"; import type { MCPServerConfig } from "@/core/mcp/types"; import { env } from "@/env"; @@ -18,6 +19,8 @@ import { SettingsSection } from "./settings-section"; export function ToolSettingsPage() { const { t } = useI18n(); const { config, isLoading, error } = useMCPConfig(); + const adminRequired = + error instanceof MCPConfigRequestError && error.isAdminRequired; return ( {isLoading ? (
{t.common.loading}
+ ) : adminRequired ? ( +
+ {t.settings.tools.adminRequired} +
) : error ? (
Error: {error.message}
) : ( @@ -37,12 +44,21 @@ export function ToolSettingsPage() { function MCPServerList({ servers, }: { - servers: Record; + servers?: Record; }) { + const { t } = useI18n(); const { mutate: enableMCPServer } = useEnableMCPServer(); + const entries = Object.entries(servers ?? {}); + if (entries.length === 0) { + return ( +
+ {t.settings.tools.empty} +
+ ); + } return (
- {Object.entries(servers).map(([name, config]) => ( + {entries.map(([name, config]) => ( diff --git a/frontend/src/core/i18n/locales/en-US.ts b/frontend/src/core/i18n/locales/en-US.ts index e5983a244..214a86728 100644 --- a/frontend/src/core/i18n/locales/en-US.ts +++ b/frontend/src/core/i18n/locales/en-US.ts @@ -495,6 +495,8 @@ export const enUS: Translations = { tools: { title: "Tools", description: "Manage the configuration and enabled status of MCP tools.", + adminRequired: "Admin privileges are required to manage MCP tools.", + empty: "No MCP tools configured.", }, channels: { title: "Channels", diff --git a/frontend/src/core/i18n/locales/types.ts b/frontend/src/core/i18n/locales/types.ts index 8cb768126..2177e34f3 100644 --- a/frontend/src/core/i18n/locales/types.ts +++ b/frontend/src/core/i18n/locales/types.ts @@ -406,6 +406,8 @@ export interface Translations { tools: { title: string; description: string; + adminRequired: string; + empty: string; }; channels: { title: string; diff --git a/frontend/src/core/i18n/locales/zh-CN.ts b/frontend/src/core/i18n/locales/zh-CN.ts index 3938d2e0b..94cb9349d 100644 --- a/frontend/src/core/i18n/locales/zh-CN.ts +++ b/frontend/src/core/i18n/locales/zh-CN.ts @@ -476,6 +476,8 @@ export const zhCN: Translations = { tools: { title: "工具", description: "管理 MCP 工具的配置和启用状态。", + adminRequired: "需要管理员权限才能管理 MCP 工具。", + empty: "暂无 MCP 工具。", }, channels: { title: "渠道", diff --git a/frontend/src/core/mcp/api.ts b/frontend/src/core/mcp/api.ts index 284c9fd25..fe057d7c4 100644 --- a/frontend/src/core/mcp/api.ts +++ b/frontend/src/core/mcp/api.ts @@ -3,8 +3,36 @@ import { getBackendBaseURL } from "@/core/config"; import type { MCPConfig } from "./types"; +export class MCPConfigRequestError extends Error { + readonly status: number; + constructor(status: number, message: string) { + super(message); + this.name = "MCPConfigRequestError"; + this.status = status; + } + get isAdminRequired(): boolean { + return this.status === 403; + } +} + +async function readErrorDetail( + response: Response, + fallback: string, +): Promise { + const error = (await response.json().catch(() => ({}))) as { + detail?: unknown; + }; + return typeof error.detail === "string" ? error.detail : fallback; +} + export async function loadMCPConfig() { const response = await fetch(`${getBackendBaseURL()}/api/mcp/config`); + if (!response.ok) { + throw new MCPConfigRequestError( + response.status, + await readErrorDetail(response, "Failed to load MCP configuration"), + ); + } return response.json() as Promise; } @@ -16,5 +44,11 @@ export async function updateMCPConfig(config: MCPConfig) { }, body: JSON.stringify(config), }); + if (!response.ok) { + throw new MCPConfigRequestError( + response.status, + await readErrorDetail(response, "Failed to update MCP configuration"), + ); + } return response.json(); } diff --git a/frontend/src/core/mcp/hooks.ts b/frontend/src/core/mcp/hooks.ts index dc17ed8c6..695392af2 100644 --- a/frontend/src/core/mcp/hooks.ts +++ b/frontend/src/core/mcp/hooks.ts @@ -1,11 +1,13 @@ import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query"; -import { loadMCPConfig, updateMCPConfig } from "./api"; +import { loadMCPConfig, MCPConfigRequestError, updateMCPConfig } from "./api"; export function useMCPConfig() { const { data, isLoading, error } = useQuery({ queryKey: ["mcpConfig"], queryFn: () => loadMCPConfig(), + retry: (count, error) => + !(error instanceof MCPConfigRequestError) && count < 3, }); return { config: data, isLoading, error }; } diff --git a/frontend/tests/unit/core/mcp/api.test.ts b/frontend/tests/unit/core/mcp/api.test.ts new file mode 100644 index 000000000..fa7909d2c --- /dev/null +++ b/frontend/tests/unit/core/mcp/api.test.ts @@ -0,0 +1,118 @@ +/** + * Tests for the error-handling behaviour of the MCP config API client. + * + * Issue #3527: when a non-admin user opens Settings → Tools, the gateway + * returns 403 `{detail: "Admin privileges required to manage MCP + * configuration."}` for `GET /api/mcp/config`. The previous client + * silently treated the 403 body as a valid `MCPConfig`, so the UI then + * crashed with `Cannot convert undefined or null to object` when it tried + * `Object.entries(config.mcp_servers)`. + * + * These tests pin the contract that non-2xx responses are surfaced as + * `MCPConfigRequestError` carrying the HTTP status and backend `detail`, + * so the React Query hook's `error` branch can render a friendly empty + * state (admin-required for 403) instead of crashing. + */ +import { beforeEach, describe, expect, test, vi } from "vitest"; + +vi.mock("@/core/api/fetcher", () => ({ + fetch: vi.fn(), +})); + +vi.mock("@/core/config", () => ({ + getBackendBaseURL: () => "", +})); + +import { fetch as fetcher } from "@/core/api/fetcher"; +import { + MCPConfigRequestError, + loadMCPConfig, + updateMCPConfig, +} from "@/core/mcp/api"; + +const mockedFetch = vi.mocked(fetcher); + +function jsonResponse(status: number, body: unknown): Response { + return new Response(JSON.stringify(body), { + status, + headers: { "Content-Type": "application/json" }, + }); +} + +beforeEach(() => { + mockedFetch.mockReset(); +}); + +describe("loadMCPConfig", () => { + test("returns parsed config on 200", async () => { + const config = { mcp_servers: { foo: { enabled: true } } }; + mockedFetch.mockResolvedValueOnce(jsonResponse(200, config)); + await expect(loadMCPConfig()).resolves.toEqual(config); + }); + + test("throws MCPConfigRequestError with isAdminRequired on 403 (issue #3527)", async () => { + mockedFetch.mockResolvedValueOnce( + jsonResponse(403, { + detail: "Admin privileges required to manage MCP configuration.", + }), + ); + await expect(loadMCPConfig()).rejects.toMatchObject({ + name: "MCPConfigRequestError", + status: 403, + isAdminRequired: true, + message: "Admin privileges required to manage MCP configuration.", + }); + }); + + test("throws MCPConfigRequestError with isAdminRequired=false on non-403 errors", async () => { + mockedFetch.mockResolvedValueOnce( + new Response("", { status: 500, statusText: "Internal Server Error" }), + ); + await expect(loadMCPConfig()).rejects.toMatchObject({ + name: "MCPConfigRequestError", + status: 500, + isAdminRequired: false, + message: "Failed to load MCP configuration", + }); + }); + + test("the rejected value is an instance of MCPConfigRequestError", async () => { + mockedFetch.mockResolvedValueOnce(jsonResponse(403, { detail: "nope" })); + await expect(loadMCPConfig()).rejects.toBeInstanceOf(MCPConfigRequestError); + }); +}); + +describe("updateMCPConfig", () => { + test("returns parsed body on 200", async () => { + mockedFetch.mockResolvedValueOnce(jsonResponse(200, { ok: true })); + await expect(updateMCPConfig({ mcp_servers: {} })).resolves.toEqual({ + ok: true, + }); + }); + + test("throws MCPConfigRequestError with isAdminRequired on 403", async () => { + mockedFetch.mockResolvedValueOnce( + jsonResponse(403, { + detail: "Admin privileges required to manage MCP configuration.", + }), + ); + await expect(updateMCPConfig({ mcp_servers: {} })).rejects.toMatchObject({ + name: "MCPConfigRequestError", + status: 403, + isAdminRequired: true, + message: "Admin privileges required to manage MCP configuration.", + }); + }); + + test("falls back to generic message on non-403 errors", async () => { + mockedFetch.mockResolvedValueOnce( + new Response("", { status: 500, statusText: "Internal Server Error" }), + ); + await expect(updateMCPConfig({ mcp_servers: {} })).rejects.toMatchObject({ + name: "MCPConfigRequestError", + status: 500, + isAdminRequired: false, + message: "Failed to update MCP configuration", + }); + }); +}); diff --git a/frontend/tests/unit/core/mcp/hooks.test.ts b/frontend/tests/unit/core/mcp/hooks.test.ts new file mode 100644 index 000000000..0dd9115ab --- /dev/null +++ b/frontend/tests/unit/core/mcp/hooks.test.ts @@ -0,0 +1,84 @@ +import { QueryClient } from "@tanstack/react-query"; +import { beforeEach, describe, expect, it, vi } from "vitest"; + +vi.mock("@/core/api/fetcher", () => ({ + fetch: vi.fn(), +})); + +import { fetch } from "@/core/api/fetcher"; +import { MCPConfigRequestError, loadMCPConfig } from "@/core/mcp/api"; + +const mockedFetch = vi.mocked(fetch); + +function makeClient() { + return new QueryClient({ + defaultOptions: { + queries: { + retryDelay: 0, + }, + }, + }); +} + +describe("useMCPConfig retry policy", () => { + beforeEach(() => { + mockedFetch.mockReset(); + }); + + it("does not retry when loadMCPConfig throws MCPConfigRequestError (403)", async () => { + mockedFetch.mockResolvedValue({ + ok: false, + status: 403, + json: async () => ({ detail: "Forbidden" }), + } as Response); + + const client = makeClient(); + await expect( + client.fetchQuery({ + queryKey: ["mcpConfig"], + queryFn: () => loadMCPConfig(), + retry: (count, error) => + !(error instanceof MCPConfigRequestError) && count < 3, + }), + ).rejects.toBeInstanceOf(MCPConfigRequestError); + + expect(mockedFetch).toHaveBeenCalledTimes(1); + }); + + it("retries up to 3 times on generic errors", async () => { + mockedFetch.mockRejectedValue(new Error("network down")); + + const client = makeClient(); + await expect( + client.fetchQuery({ + queryKey: ["mcpConfig"], + queryFn: () => loadMCPConfig(), + retry: (count, error) => + !(error instanceof MCPConfigRequestError) && count < 3, + }), + ).rejects.toThrow("network down"); + + // initial + 3 retries = 4 calls + expect(mockedFetch).toHaveBeenCalledTimes(4); + }); + + it("does not retry on MCPConfigRequestError 5xx either (deterministic typed error)", async () => { + mockedFetch.mockResolvedValue({ + ok: false, + status: 500, + json: async () => ({ detail: "Boom" }), + } as Response); + + const client = makeClient(); + await expect( + client.fetchQuery({ + queryKey: ["mcpConfig"], + queryFn: () => loadMCPConfig(), + retry: (count, error) => + !(error instanceof MCPConfigRequestError) && count < 3, + }), + ).rejects.toBeInstanceOf(MCPConfigRequestError); + + expect(mockedFetch).toHaveBeenCalledTimes(1); + }); +});