History

Report expired, not healthy, when an MCP connection's credential is missing (#1582)

checkHealth built its connector from whatever values it had. A missing value is skipped by the renderer, so the probe dialled unauthenticated, and any server that lists tools without auth answered -- discoverTools succeeding maps to healthy. A connection whose credential was gone therefore reported healthy, which is the one status that must never appear in that state, because health is what tells a user to re-authenticate. Mirrors the OpenAPI health check. resolveTools stays ungated on purpose.

Sergio Fernández committed 2 months ago

Commit e8e97c41 parent 9c35f263 files · +146 −0Browse files

Unified · Split

3 files
  1. .changeset/mcp-health-missing-credential.md +11 −0
  2. packages/plugins/mcp/src/sdk/health-missing-credential.test.ts +111 −0
  3. packages/plugins/mcp/src/sdk/plugin.ts +24 −0
.changeset/mcp-health-missing-credential.md+11−0
@@ -0,0 +1,11 @@
1---
2"executor": patch
3---
4
5**An MCP health check no longer reports `healthy` when the connection's credential is missing**
6
7Rendering skips an auth placement whose value is unresolved — that is the renderer's documented behaviour, and callers own the missing-value policy. The MCP health check had no such policy, so it dialled unauthenticated, and any server that lists tools without auth answered. `discoverTools` succeeding maps straight to `healthy`, so a connection whose credential was gone reported as healthy.
8
9Health status is the signal telling a user to re-authenticate, which makes `healthy` the one answer it must never give in that state. The check now reports `expired` with the unresolved input names, mirroring the OpenAPI health check, which already did exactly this.
10
11The MCP tool-invocation path already refused for the same reason. `resolveTools` is deliberately left alone — its own comment records that discovery tolerating unresolved credentials is intended, since an open server lists tools unauthenticated.
packages/plugins/mcp/src/sdk/health-missing-credential.test.ts+111−0
@@ -0,0 +1,111 @@
1// ---------------------------------------------------------------------------
2// A health check must not report `healthy` for a connection whose credential is
3// missing.
4//
5// Rendering SKIPS a placement whose value is unresolved — that is the renderer's
6// documented behaviour, and callers own the policy. The invoke path already
7// refuses (it names "dialing unauthenticated" as the thing it is avoiding), and
8// the OpenAPI health check reports `expired`. The MCP health check had neither,
9// so it dialled unauthenticated, and any server that lists tools without auth
10// answered — reporting a connection with no credential as healthy.
11//
12// That matters more than an ordinary wrong status: health is the signal telling
13// a user to re-authenticate, so `healthy` is the one answer it must never give
14// when the credential is gone.
15//
16// Driven against the plugin's own `checkHealth` rather than through a live
17// connection, because the precondition under test — a connection that EXISTS but
18// whose credential does not resolve — is exactly the state the connection APIs
19// are designed to prevent you from creating. Calling the seam directly is the
20// only way to construct it without faking the thing being tested.
21// ---------------------------------------------------------------------------
22
23import { describe, expect, it } from "@effect/vitest";
24import { Effect, Layer } from "effect";
25import { FetchHttpClient } from "effect/unstable/http";
26
27import { mcpPlugin } from "./plugin";
28
29const ENDPOINT = "https://mcp.example.test/sse";
30
31/** A remote MCP integration whose api-key method needs one input. */
32const config = {
33 transport: "remote" as const,
34 endpoint: ENDPOINT,
35 remoteTransport: "streamable-http" as const,
36 authenticationTemplate: [
37 {
38 slug: "api_key",
39 kind: "apikey" as const,
40 placements: [{ carrier: "header" as const, name: "X-Api-Key", variable: "token" }],
41 },
42 ],
43};
44
45/** Answers anything with 200 — standing in for a server that lists tools with no
46 * auth at all, which is what turned a missing credential into `healthy`. */
47const permissiveClientLayer = FetchHttpClient.layer.pipe(
48 Layer.provide(
49 Layer.succeed(FetchHttpClient.Fetch)(
50 (async (_input: RequestInfo | URL) =>
51 new Response("{}", {
52 status: 200,
53 headers: { "content-type": "application/json" },
54 })) as typeof globalThis.fetch,
55 ),
56 ),
57);
58
59const checkHealthWith = (values: Record<string, string | null>) =>
60 Effect.gen(function* () {
61 const plugin = mcpPlugin();
62 const checkHealth = (plugin as { readonly checkHealth?: unknown }).checkHealth;
63 if (typeof checkHealth !== "function") {
64 return yield* Effect.die("mcpPlugin no longer exposes checkHealth");
65 }
66 return yield* (
67 checkHealth as (input: {
68 readonly ctx: { readonly httpClientLayer: typeof permissiveClientLayer };
69 readonly credential: {
70 readonly config: typeof config;
71 readonly values: Record<string, string | null>;
72 readonly template: string;
73 readonly connection: string;
74 readonly integration: string;
75 };
76 }) => Effect.Effect<{ readonly status: string; readonly detail?: string }>
77 )({
78 ctx: { httpClientLayer: permissiveClientLayer },
79 credential: {
80 config,
81 values,
82 template: "api_key",
83 connection: "main",
84 integration: "health_mcp",
85 },
86 });
87 });
88
89describe("MCP health check with an unresolved credential", () => {
90 it.effect("reports expired, not healthy, when the api-key input is missing", () =>
91 Effect.gen(function* () {
92 const health = yield* checkHealthWith({});
93
94 // The fetch above answers everything 200, so nothing except the gate
95 // stands between this and `healthy`.
96 expect(health.status).not.toBe("healthy");
97 expect(health.status).toBe("expired");
98 expect(String(health.detail ?? "")).toContain("token");
99 }),
100 );
101
102 it.effect("does not short-circuit when the input IS resolved", () =>
103 Effect.gen(function* () {
104 // The other half: a gate that returned `expired` unconditionally would
105 // satisfy the test above while breaking every healthy connection.
106 const health = yield* checkHealthWith({ token: "sk-present" });
107
108 expect(health.status).not.toBe("expired");
109 }),
110 );
111});
packages/plugins/mcp/src/sdk/plugin.ts+24−0
@@ -1559,6 +1559,30 @@ export const mcpPlugin = definePlugin((options?: McpPluginOptions) => { · 1558 unmodified lines
1559 if (!parsed) {1559 if (!parsed) {
1560 return { status: "unknown" as const, checkedAt: Date.now() } satisfies HealthCheckResult;1560 return { status: "unknown" as const, checkedAt: Date.now() } satisfies HealthCheckResult;
1561 }1561 }
1562 // An unresolved apikey input reports `expired`, not `healthy`.
1563 //
1564 // Rendering skips a placement whose value is missing, so without this the
1565 // probe dials UNAUTHENTICATED — and any server that lists tools without
1566 // auth answers, making a connection whose credential is gone report as
1567 // healthy. Health is the signal that tells a user to re-authenticate, so
1568 // that is the one status it must never give here. The invoke path already
1569 // refuses for the same reason, and the OpenAPI health check reports
1570 // `expired` in exactly this case.
1571 if (parsed.transport === "remote") {
1572 const method = selectAuthMethod(parsed, String(credential.template));
1573 if (method?.kind === "apikey") {
1574 const missing = requiredPlacementVariables(method.placements).filter(
1575 (variable) => credential.values[variable] == null,
1576 );
1577 if (missing.length > 0) {
1578 return {
1579 status: "expired" as const,
1580 checkedAt: Date.now(),
1581 detail: `Connection has no resolvable credential value for input(s): ${missing.join(", ")}.`,
1582 } satisfies HealthCheckResult;
1583 }
1584 }
1585 }
1562 const connector = yield* buildConnectorInput(1586 const connector = yield* buildConnectorInput(
1563 parsed,1587 parsed,
1564 credential.values,1588 credential.values,