diff --git a/clients/web/README.md b/clients/web/README.md index 8a427970..619865ab 100644 --- a/clients/web/README.md +++ b/clients/web/README.md @@ -96,7 +96,9 @@ Nothing _enforces_ the boundary — no path alias keys off it, and the coverage The Tools, Resources, Prompts and Skills screens each expose a `data-testid` plus a small set of `data-*` attributes, so a headless driver can `waitForSelector` on a deterministic signal rather than on visible copy. `scripts/smoke-web-tabs.mjs` -drives all three against `test-servers/configs/web-tabs-http.json` ([#2148](https://github.com/modelcontextprotocol/inspector/issues/2148)). +drives the first three of those four against `test-servers/configs/web-tabs-http.json` +([#2148](https://github.com/modelcontextprotocol/inspector/issues/2148)); Skills +publishes the same contract but is not smoked yet ([#2234](https://github.com/modelcontextprotocol/inspector/issues/2234)). Treat them as a public contract, for the same reason as the Apps ones below: | Attribute | Where | Meaning | diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx index 1356b156..e3cd52db 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx @@ -6,6 +6,7 @@ import { sha256Digest, textToBytes } from "@inspector/core/mcp/skills"; import { renderWithMantine, screen, + waitFor, within, } from "../../../test/renderWithMantine"; import { @@ -335,6 +336,100 @@ describe("SkillsScreen", () => { expect(preview).toHaveTextContent("from a blob"); }); + it("keeps the newest verdict when two verifications of one row overlap", async () => { + // Same row, same manifest — so the manifest key cannot tell these apart. + // Without a per-row attempt token the older read finishing last would + // overwrite the newer verdict and leave the UI reporting stale bytes. + const user = userEvent.setup(); + const resolvers: ((value: { text: string }) => void)[] = []; + const onReadSkillFile = vi.fn( + () => + new Promise<{ text: string }>((resolve) => { + resolvers.push(resolve); + }), + ); + renderWithMantine( + , + ); + await user.click(screen.getByText("data-analysis")); + const manifest = screen.getByTestId("skill-manifest"); + const rowVerify = within(manifest).getAllByRole("button", { + name: "Verify", + })[0]; + await user.click(rowVerify); + await user.click(rowVerify); + expect(resolvers).toHaveLength(2); + + // The SECOND read answers first with the matching bytes, then the first + // read answers with bytes that would verify as a mismatch. + resolvers[1]({ text: SELF_TEXT }); + expect(await screen.findByText("verified")).toBeInTheDocument(); + resolvers[0]({ text: "stale bytes\n" }); + // Still the newer verdict. + expect(await screen.findByText("verified")).toBeInTheDocument(); + expect(screen.queryByText("mismatch")).not.toBeInTheDocument(); + }); + + it("disables Verify all while a batch is running", async () => { + // The concurrency cap is per invocation, so a second click would start a + // second pool of four rather than reusing the first. + const user = userEvent.setup(); + const pending: ((value: { text: string }) => void)[] = []; + const onReadSkillFile = vi.fn( + () => + new Promise<{ text: string }>((resolve) => { + pending.push(resolve); + }), + ); + renderWithMantine( + , + ); + await user.click(screen.getByText("data-analysis")); + const verifyAll = screen.getByRole("button", { name: /Verify all/ }); + await user.click(verifyAll); + expect(verifyAll).toBeDisabled(); + // Release every read the batch started; the button frees only once the + // whole batch settles, not once the first file does. + await waitFor(() => expect(pending.length).toBeGreaterThan(0)); + for (const resolve of pending) resolve({ text: SELF_TEXT }); + await waitFor(() => expect(verifyAll).not.toBeDisabled()); + }); + + it("renders every duplicate finding rather than collapsing them", async () => { + // Three identical URIs produce two `duplicate-resource` findings with the + // same code and URI. A key built from those alone would make React drop + // the extras — hiding findings in exactly the malformed input this view is + // for. + const user = userEvent.setup(); + const dup = { + uri: "skill://data-analysis/SKILL.md", + digest: SELF_DIGEST, + size: textToBytes(SELF_TEXT).byteLength, + }; + renderWithMantine( + , + ); + await user.click(screen.getByText("data-analysis")); + const issues = screen.getByTestId("skill-issues"); + expect(within(issues).getAllByText("duplicate-resource")).toHaveLength(2); + }); + it("shows the SKILL.md preview on demand", async () => { const user = userEvent.setup(); renderWithMantine(); diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx index faab7e7b..6dc6fa9a 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx @@ -1,4 +1,4 @@ -import { useCallback, useMemo, useState } from "react"; +import { useCallback, useMemo, useRef, useState } from "react"; import { Alert, Badge, @@ -42,11 +42,18 @@ import { */ const VERIFY_CONCURRENCY = 4; -/** Per-row verification progress. */ -type FileState = +/** + * Per-row verification progress. `attempt` is the click that produced it: two + * verifications of the SAME row in the SAME manifest (a double click, or a row + * button pressed while "Verify all" is running) are not distinguished by the + * manifest key, so without it an older read finishing last would overwrite the + * newer verdict and leave the UI reporting bytes it no longer fetched. + */ +type FileState = { attempt: number } & ( | { status: "pending" } | { status: "done"; verification: SkillVerification } - | { status: "error"; message: string }; + | { status: "error"; message: string } +); /** * Verification verdicts plus the manifest they belong to. Rows are keyed by @@ -277,6 +284,12 @@ export function SkillsScreen({ files: {}, }); const [previewState, setPreviewState] = useState({ key: null }); + // True while a "Verify all" batch is in flight; disables the button so a + // second click cannot stack another pool of workers on top. + const [batchRunning, setBatchRunning] = useState(false); + // Monotonic per-row attempt token. A ref because it is claimed inside an + // event handler, never during render. + const nextAttempt = useRef(0); const filtered = useMemo(() => { const needle = search.trim().toLowerCase(); @@ -342,24 +355,32 @@ export function SkillsScreen({ */ const verifyRow = useCallback( async (index: number, resource: SkillResource, key: string) => { + // Claimed synchronously, so two verifications of this row are ordered + // before either read starts. + const attempt = (nextAttempt.current += 1); const write = (state: FileState) => setVerification((prev) => { // `null` is the un-adopted initial manifest; any other mismatch is a // continuation from a manifest that has since been invalidated. if (prev.key !== null && prev.key !== key) return prev; const files = prev.key === key ? prev.files : {}; + // A newer attempt for this row already wrote — an older read + // finishing last must not overwrite it. + const held = files[index]; + if (held !== undefined && held.attempt > attempt) return prev; return { key, files: { ...files, [index]: state } }; }); - write({ status: "pending" }); + write({ attempt, status: "pending" }); try { const contents = await onReadSkillFile(resource.uri); const result = await verifySkillResource( resource, skillFileBytes(contents), ); - write({ status: "done", verification: result }); + write({ attempt, status: "done", verification: result }); } catch (err) { write({ + attempt, status: "error", message: err instanceof Error ? err.message : String(err), }); @@ -386,7 +407,13 @@ export function SkillsScreen({ } }; const workers = Math.min(VERIFY_CONCURRENCY, manifest.length); - void Promise.all(Array.from({ length: workers }, () => worker())); + setBatchRunning(true); + // The concurrency cap is per invocation, so without the button being + // disabled below, a second click would start a second pool of four and a + // third would make it twelve — the flood the cap exists to prevent. + void Promise.all(Array.from({ length: workers }, () => worker())).finally( + () => setBatchRunning(false), + ); }, [manifest, manifestKey, verifyRow]); const showSkillMd = useCallback(() => { @@ -509,9 +536,15 @@ export function SkillsScreen({ ) : ( - {issues.map((issue) => ( + {issues.map((issue, index) => ( @@ -542,7 +575,8 @@ export function SkillsScreen({ Verify all diff --git a/clients/web/src/test/core/mcp/skills.test.ts b/clients/web/src/test/core/mcp/skills.test.ts index 06e933d0..f489c7fe 100644 --- a/clients/web/src/test/core/mcp/skills.test.ts +++ b/clients/web/src/test/core/mcp/skills.test.ts @@ -9,6 +9,7 @@ import { checkSkillConformance, getSkillsExtension, isSkillsExtensionSupported, + normalizeSkillUri, sha256Digest, skillDisplayName, skillNameFromUri, @@ -112,11 +113,47 @@ describe("skillNameFromUri", () => { expect(skillNameFromUri("SKILL.md")).toBeUndefined(); }); + it("returns undefined for a relative string", () => { + // SEP-2640 requires a full resource URI; treating `demo/SKILL.md` as one + // would let a non-conforming entry report a name and pass the path check. + expect(skillNameFromUri("demo/SKILL.md")).toBeUndefined(); + }); + + it("reads the name off the RESOLVED path, not the raw string", () => { + expect(skillNameFromUri("skill://acme/wrong/../demo/SKILL.md")).toBe( + "demo", + ); + }); + it("returns undefined when the segment before the suffix is empty", () => { expect(skillNameFromUri("skill:///SKILL.md")).toBeUndefined(); }); }); +describe("normalizeSkillUri", () => { + it("resolves traversal segments", () => { + expect(normalizeSkillUri("skill://acme/billing/refunds/../other.md")).toBe( + "skill://acme/billing/other.md", + ); + }); + + it("rejects a relative string, which is not a resource URI", () => { + expect(normalizeSkillUri("demo/SKILL.md")).toBeUndefined(); + }); + + it("rejects an opaque-path URI, which the parser does not normalize", () => { + // `skill:demo/../x.md` parses but keeps its `..` verbatim, so containment + // could not be decided on it — accepting it would reopen the hole. + expect(normalizeSkillUri("skill:demo/SKILL.md")).toBeUndefined(); + }); + + it("leaves an already-normal URI alone", () => { + expect(normalizeSkillUri("skill://demo/SKILL.md")).toBe( + "skill://demo/SKILL.md", + ); + }); +}); + describe("skillDisplayName", () => { it("prefers the declared frontmatter name", () => { expect(skillDisplayName(entry())).toBe("demo"); @@ -148,12 +185,14 @@ describe("checkSkillConformance", () => { expect(issues[0].severity).toBe("error"); }); - it("reports a missing description as a warning", () => { + it("reports a missing description as an error", () => { + // SEP-2640 requires `description`, so an absent one is a format violation + // and must not read as "0 errors" in the conformance summary. const issues = checkSkillConformance( entry({ frontmatter: { name: "demo" } }), ); expect(issues.map((i) => i.code)).toEqual(["missing-description"]); - expect(issues[0].severity).toBe("warning"); + expect(issues[0].severity).toBe("error"); }); it("reports a URI that does not carry a skill path", () => { @@ -235,6 +274,35 @@ describe("checkSkillConformance", () => { expect(issues[0].resourceUri).toBe("skill://demo/ref.md"); }); + it("reports an entry that traverses out of the skill root", () => { + // The raw string starts with the root; the resolved path does not. A + // prefix check alone would miss this and report `Conforms`. + const issues = checkSkillConformance( + entry({ + uri: "skill://demo/refunds/SKILL.md", + frontmatter: { name: "refunds", description: "d" }, + resources: [ + { uri: "skill://demo/refunds/SKILL.md", digest: DIGEST, size: 1 }, + { uri: "skill://demo/refunds/../other.md", digest: DIGEST, size: 1 }, + ], + }), + ); + expect(issues.map((i) => i.code)).toEqual(["resource-outside-skill-root"]); + }); + + it("reports an unparseable manifest entry as outside the root", () => { + // Nothing can establish that a non-URI is inside a root. + const issues = checkSkillConformance( + entry({ + resources: [ + { uri: "skill://demo/SKILL.md", digest: DIGEST, size: 20 }, + { uri: "not a uri", digest: DIGEST, size: 1 }, + ], + }), + ); + expect(issues.map((i) => i.code)).toEqual(["resource-outside-skill-root"]); + }); + it("reports a manifest entry outside the skill root", () => { const issues = checkSkillConformance( entry({ diff --git a/clients/web/src/test/core/mcp/state/managedSkillsState.test.ts b/clients/web/src/test/core/mcp/state/managedSkillsState.test.ts index 2cda1537..df22fae7 100644 --- a/clients/web/src/test/core/mcp/state/managedSkillsState.test.ts +++ b/clients/web/src/test/core/mcp/state/managedSkillsState.test.ts @@ -6,6 +6,7 @@ import { SKILLS_MAX_PAGES, SKILLS_PAGE_LIMIT_MESSAGE, } from "@inspector/core/mcp/state/managedSkillsState"; +import { SdkError, SdkErrorCode } from "@modelcontextprotocol/client"; import { FakeInspectorClient } from "@inspector/core/mcp/__tests__/fakeInspectorClient"; function skill(name: string): SkillEntry { @@ -216,6 +217,36 @@ describe("ManagedSkillsState", () => { expect(state.getError()).toBe(failure); }); + describe("Protocol-entry attribution", () => { + it("attributes a decode rejection to its skills/list Protocol entry", async () => { + // Without this the Protocol tab renders an invalid `skills/list` result + // as a clean success — the same gap every managed list closed in #1953. + client.setStatus("connected"); + const rejection = new SdkError( + SdkErrorCode.InvalidResult, + "Invalid result for skills/list: skills required", + ); + client.listSkills.mockRejectedValueOnce(rejection); + await expect(state.refresh()).rejects.toThrow(rejection); + expect(client.markResponseRejected).toHaveBeenCalledWith( + "skills/list", + rejection.message, + ); + }); + + it("does NOT attribute a transport failure", async () => { + // No response frame arrived, so the last-answered id still points at an + // EARLIER successful call; marking it would stamp "Rejected by the + // Inspector" onto an exchange that worked. + client.setStatus("connected"); + client.listSkills.mockRejectedValueOnce( + new SdkError(SdkErrorCode.ConnectionClosed, "Connection closed"), + ); + await expect(state.refresh()).rejects.toThrow(); + expect(client.markResponseRejected).not.toHaveBeenCalled(); + }); + }); + it("wraps a non-Error rejection", async () => { client.setStatus("connected"); client.listSkills.mockRejectedValueOnce("just a string"); diff --git a/core/mcp/skills.ts b/core/mcp/skills.ts index f7c776d5..586b110d 100644 --- a/core/mcp/skills.ts +++ b/core/mcp/skills.ts @@ -91,16 +91,49 @@ export function isSkillsExtensionSupported( return getSkillsExtension(capabilities) !== undefined; } +/** + * A skill URI in normalized form, or `undefined` when it is not one. + * + * Two things a raw string comparison gets wrong, and both matter: + * + * 1. **`..` segments.** `skill://acme/billing/refunds/../other.md` starts with + * the advertised root but resolves outside it. Containment has to be decided + * on the resolved path, so every check below goes through the parser. + * 2. **Relative strings.** SEP-2640 requires a full resource URI, and + * `demo/SKILL.md` is not one — it fails to parse and is reported as + * `malformed-uri` rather than quietly treated as a skill path. + * + * An **opaque-path** URI (`skill:demo/SKILL.md`, no authority) parses but is + * NOT normalized by the parser — its `..` segments survive verbatim — so it is + * rejected too: containment could not be decided on it, and silently accepting + * one would reintroduce exactly the hole this function closes. + */ +export function normalizeSkillUri(uri: string): string | undefined { + let parsed: URL; + try { + parsed = new URL(uri); + } catch { + return undefined; + } + return parsed.pathname.startsWith("/") ? parsed.href : undefined; +} + /** * The final `` segment of a skill URI — the segment *before* * `/SKILL.md`, not the filename. SEP-2640 requires it to equal * `frontmatter.name`, which is what makes a skill's name recoverable from its * URI alone. Returns `undefined` when the URI does not have that shape, which * is itself a conformance finding. + * + * Read off the **normalized** URI, so a traversal segment cannot produce a + * name the resolved path does not actually carry. */ export function skillNameFromUri(uri: string): string | undefined { - if (!uri.endsWith(SKILL_FILE_SUFFIX)) return undefined; - const path = uri.slice(0, -SKILL_FILE_SUFFIX.length); + const normalized = normalizeSkillUri(uri); + if (normalized === undefined || !normalized.endsWith(SKILL_FILE_SUFFIX)) { + return undefined; + } + const path = normalized.slice(0, -SKILL_FILE_SUFFIX.length); const segment = path.slice(path.lastIndexOf("/") + 1); return segment.length > 0 ? segment : undefined; } @@ -171,10 +204,12 @@ export function checkSkillConformance(entry: SkillEntry): SkillIssue[] { }); } if (!entry.frontmatter.description?.trim()) { + // An error, not a warning: SEP-2640 requires `description` on every skill, + // so an absent one is a format violation and must not read as "0 errors". issues.push({ code: "missing-description", - severity: "warning", - message: "frontmatter.description is missing or empty.", + severity: "error", + message: "frontmatter.description is required but missing or empty.", }); } if (uriName === undefined) { @@ -237,12 +272,17 @@ export function checkSkillConformance(entry: SkillEntry): SkillIssue[] { const seenUris = new Set(); // Relative references resolve against the skill root, so every manifest entry // must live under it. A URI outside that prefix is either a typo or a server - // claiming integrity over a file that is not part of this skill. Left - // `undefined` for a malformed entry URI — there is no root to measure - // against, and `malformed-uri` already reports that. - const root = entry.uri.endsWith(SKILL_FILE_SUFFIX) - ? `${entry.uri.slice(0, -SKILL_FILE_SUFFIX.length)}/` - : undefined; + // claiming integrity over a file that is not part of this skill. Computed + // from the NORMALIZED entry URI, and compared against normalized resource + // URIs, so a `..` segment cannot walk out of the root while still matching it + // as a string. Left `undefined` for a malformed entry URI — there is no root + // to measure against, and `malformed-uri` already reports that. + const normalizedEntryUri = normalizeSkillUri(entry.uri); + const root = + normalizedEntryUri !== undefined && + normalizedEntryUri.endsWith(SKILL_FILE_SUFFIX) + ? `${normalizedEntryUri.slice(0, -SKILL_FILE_SUFFIX.length)}/` + : undefined; for (const resource of entry.resources) { if (seenUris.has(resource.uri)) { @@ -255,13 +295,18 @@ export function checkSkillConformance(entry: SkillEntry): SkillIssue[] { }); } seenUris.add(resource.uri); - if (root !== undefined && !resource.uri.startsWith(root)) { - issues.push({ - code: "resource-outside-skill-root", - severity: "error", - message: `Manifest entry is outside the skill root "${root}".`, - resourceUri: resource.uri, - }); + if (root !== undefined) { + const normalized = normalizeSkillUri(resource.uri); + // An unparseable entry URI is outside the root by construction: nothing + // can establish that it is inside one. + if (normalized === undefined || !normalized.startsWith(root)) { + issues.push({ + code: "resource-outside-skill-root", + severity: "error", + message: `Manifest entry does not resolve inside the skill root "${root}".`, + resourceUri: resource.uri, + }); + } } if (resource.digest === undefined) { issues.push({ diff --git a/core/mcp/state/managedSkillsState.ts b/core/mcp/state/managedSkillsState.ts index 80866333..0429e821 100644 --- a/core/mcp/state/managedSkillsState.ts +++ b/core/mcp/state/managedSkillsState.ts @@ -7,8 +7,7 @@ * not have: a top-level `ServerCapabilities` key to gate on (skills is a * *server-declared extension*, read from `capabilities.extensions`), and a * per-list `list_changed` notification to debounce and turn into a sidebar - * indicator - * (SEP-2640 defines none). Subclassing would mean widening the base's + * indicator (SEP-2640 defines none). Subclassing would mean widening the base's * capability gate and inventing a list-changed event nothing dispatches — two * changes to shared machinery to serve one caller. The cursor walk below is the * only behavior actually shared, and it is nine lines. @@ -20,8 +19,8 @@ */ import type { InspectorClientProtocol } from "../inspectorClientProtocol.js"; -import type { SkillEntry } from "../skillsSchemas.js"; -import { LIST_MAX_PAGES } from "../listSalvage.js"; +import { SKILLS_LIST_METHOD, type SkillEntry } from "../skillsSchemas.js"; +import { LIST_MAX_PAGES, isClientDecodeRejection } from "../listSalvage.js"; import { isTerminalStatus } from "../types.js"; import type { RequestMetadata } from "../types.js"; import { TypedEventTarget } from "../typedEventTarget.js"; @@ -201,7 +200,17 @@ export class ManagedSkillsState extends TypedEventTarget { const skill = FIXTURE_SKILLS.find( (candidate) => `skill://${candidate.path}/SKILL.md` === uri, ); - if (!skill) throw new Error(`Unknown skill uri: ${uri}`); + // `-32602`, not a plain `Error`: the method contract says an unknown skill + // URI is invalid params, and a generic throw would be mapped to a server + // failure — making the fixture non-conforming outside its three documented + // bad cases, which is the opposite of what it is for. + if (!skill) { + throw new ProtocolError( + ProtocolErrorCode.InvalidParams, + `Unknown skill uri: ${uri}`, + ); + } // The envelope (`{ skill }`) is the conforming shape, and the only one the // Inspector accepts — see `GetSkillResultSchema`. return { skill: toEntry(skill) }; @@ -232,7 +254,11 @@ export function readSkillFile( }; } -/** The private handler registry the SDK dispatches through. */ +/** + * The private handler registry the SDK dispatches through. Reached ONLY to wrap + * `resources/read` — see the module header for why that one has no public + * equivalent. + */ interface RawHandlerHost { _requestHandlers: Map< string, @@ -241,38 +267,45 @@ interface RawHandlerHost { } interface UriRequest { - params?: { uri?: string; cursor?: string }; + params?: { uri?: string }; } +const ListSkillsParamsSchema = z.object({ cursor: z.string().optional() }); +const GetSkillParamsSchema = z.object({ uri: z.string() }); + /** * Wire `skills/list`, `skills/get` and the `skill://` half of `resources/read` * onto an `McpServer`. - * - * `resources/read` is wrapped rather than replaced: a `skill://` URI is - * answered here and everything else falls through to whatever the SDK - * registered, so a config can serve ordinary resources alongside its skills. */ export function wireSkillsHandlers(mcpServer: McpServer): void { - const registry = (mcpServer.server as unknown as RawHandlerHost) - ._requestHandlers; + const lowLevel = mcpServer.server; - registry.set("skills/list", async (request) => { - const req = request as UriRequest; - return listSkillsPage(req.params?.cursor); - }); + lowLevel.setRequestHandler( + "skills/list", + { params: ListSkillsParamsSchema }, + async (params) => listSkillsPage(params.cursor), + ); - registry.set("skills/get", async (request) => { - const req = request as UriRequest; - return getSkillEntry(req.params?.uri ?? ""); - }); + lowLevel.setRequestHandler( + "skills/get", + { params: GetSkillParamsSchema }, + async (params) => getSkillEntry(params.uri), + ); + // Wrapped, not registered: a `skill://` URI is answered here and everything + // else falls through to whatever the SDK registered, so a config can serve + // ordinary resources alongside its skills. + const registry = (lowLevel as unknown as RawHandlerHost)._requestHandlers; const sdkResourcesRead = registry.get("resources/read"); registry.set("resources/read", async (request, ctx) => { const req = request as UriRequest; const skillFile = readSkillFile(req.params?.uri ?? ""); if (skillFile) return skillFile; if (!sdkResourcesRead) { - throw new Error(`Unknown resource: ${req.params?.uri}`); + throw new ProtocolError( + ProtocolErrorCode.InvalidParams, + `Unknown resource: ${req.params?.uri}`, + ); } return sdkResourcesRead(request, ctx); });