diff --git a/clients/web/src/hooks/useServerCommands.test.tsx b/clients/web/src/hooks/useServerCommands.test.tsx index e7b6a48e..004f0062 100644 --- a/clients/web/src/hooks/useServerCommands.test.tsx +++ b/clients/web/src/hooks/useServerCommands.test.tsx @@ -863,12 +863,13 @@ describe("onReadSkillFile (#2234)", () => { }); }); - it("accepts a sole block whose uri the server echoed back differently", async () => { - // `resources/read` answers the URI it was asked for, so a single-block - // response IS that block even when the echo differs in form. + it("accepts a block whose uri the server echoed back in an equivalent form", async () => { + // A resolved `..` is the same resource; `normalizeSkillUri` is what says so. const c = client({ readResource: vi.fn().mockResolvedValue({ - result: { contents: [{ uri: "SKILL://DEMO/reference.md", text: "x" }] }, + result: { + contents: [{ uri: "skill://demo/x/../reference.md", text: "x" }], + }, }), }); const h = harness({ client: c }); @@ -877,6 +878,35 @@ describe("onReadSkillFile (#2234)", () => { }); }); + it("refuses a sole block for a DIFFERENT uri rather than verifying it", async () => { + // The dangerous fallback: treating "the only block" as "the block we asked + // for" would hash `other.md`'s bytes against `reference.md`'s advertised + // digest — and could report that as `verified`. + const c = client({ + readResource: vi.fn().mockResolvedValue({ + result: { contents: [{ uri: "skill://demo/other.md", text: "wrong" }] }, + }), + }); + const h = harness({ client: c }); + await expect(h.api().onReadSkillFile(skillUri)).rejects.toThrow( + /returned no content .*skill:\/\/demo\/other\.md/, + ); + }); + + it("refuses an unparseable echoed uri rather than matching it to another", async () => { + // Two unparseable URIs must not compare equal just because both normalize + // to `undefined`. + const c = client({ + readResource: vi.fn().mockResolvedValue({ + result: { contents: [{ uri: "not a uri", text: "wrong" }] }, + }), + }); + const h = harness({ client: c }); + await expect(h.api().onReadSkillFile("also not a uri")).rejects.toThrow( + /returned no content/, + ); + }); + it("passes a blob block through as a blob", async () => { const c = client({ readResource: vi.fn().mockResolvedValue({ diff --git a/clients/web/src/hooks/useServerCommands.tsx b/clients/web/src/hooks/useServerCommands.tsx index 1fb7f052..90457e78 100644 --- a/clients/web/src/hooks/useServerCommands.tsx +++ b/clients/web/src/hooks/useServerCommands.tsx @@ -36,6 +36,7 @@ import type { GetPromptState } from "../components/screens/PromptsScreen/Prompts import type { ReadResourceState } from "../components/screens/ResourcesScreen/ResourcesScreen"; import type { SkillFileContents } from "../utils/skillFileBytes"; import type { SkillEntry } from "@inspector/core/mcp/skillsSchemas.js"; +import { normalizeSkillUri } from "@inspector/core/mcp/skills.js"; import { UrlElicitationErrorToastMessage } from "../components/elements/Toasts/UrlElicitationErrorToastMessage"; import { errorCodeOf, @@ -954,14 +955,31 @@ export function useServerCommands({ const onReadSkillFile = useCallback( async (uri: string): Promise => { const result = await onReadResourceContents(uri); - // `resources/read` answers the URI it was asked for, so a single-block - // response is that block even when the server echoes the URI back in a - // slightly different form; an exact match wins when there are several. - const block = - result.contents.find((c) => c.uri === uri) ?? - (result.contents.length === 1 ? result.contents[0] : undefined); + // The returned URI must be the one asked for. A tempting fallback — + // "a single-block response must be the block we asked for" — is right + // for a viewer and WRONG here: these bytes are about to be hashed + // against `uri`'s advertised digest, so accepting a block the server + // labelled `b.md` would verify one file's content against another + // file's digest and could report that as `verified`. Knowing which + // bytes were hashed is the whole point. + // + // A normalized match is still accepted, because a server may echo the + // URI back in a different but equivalent form (a resolved `..`, a + // percent-encoding difference); `normalizeSkillUri` returns `undefined` + // for anything unparseable, and two `undefined`s must not compare equal. + const wanted = normalizeSkillUri(uri); + const block = result.contents.find((c) => { + if (c.uri === uri) return true; + const got = normalizeSkillUri(c.uri); + return got !== undefined && got === wanted; + }); if (!block) { - throw new Error(`resources/read returned no content for ${uri}`); + throw new Error( + `resources/read returned no content for ${uri}` + + (result.contents.length > 0 + ? ` (got ${result.contents.map((c) => c.uri).join(", ")})` + : ""), + ); } // `contents` is a union of the text and blob shapes, each with its own // payload field required — so `in` is what narrows it, not a `typeof` on diff --git a/test-servers/src/skills.ts b/test-servers/src/skills.ts index b04444b7..ace50d87 100644 --- a/test-servers/src/skills.ts +++ b/test-servers/src/skills.ts @@ -96,53 +96,81 @@ interface FixtureFile { interface FixtureSkill { /** The `` segment; `skill:///SKILL.md` is the entry URI. */ path: string; - frontmatter: Record; + /** The SAME object the served `SKILL.md` was built from — see `skillMd`. */ + frontmatter: Frontmatter; /** `"dynamic"` for a generated skill with no enumerable manifest. */ files: FixtureFile[] | "dynamic"; } -function skillMd(name: string, description: string, body: string): string { - return `---\nname: ${name}\ndescription: ${description}\n---\n\n${body}\n`; +interface Frontmatter { + name: string; + description: string; } +/** + * The `SKILL.md` for one skill, built FROM its frontmatter object. + * + * SEP-2640 requires the frontmatter a server lists to match the frontmatter in + * the file it serves, field for field. Writing the two out separately let them + * drift — and did: three fixtures listed one description and served another, + * which is an undocumented extra violation that would have made phase 3's + * frontmatter check report a finding these fixtures were not built to + * demonstrate. Deriving one from the other makes that class of drift + * impossible rather than merely fixed. + */ +function skillMd(frontmatter: Frontmatter, body: string): string { + return `---\nname: ${frontmatter.name}\ndescription: ${frontmatter.description}\n---\n\n${body}\n`; +} + +const DATA_ANALYSIS_FM: Frontmatter = { + name: "data-analysis", + description: "Analyze a CSV and summarize its columns", +}; const DATA_ANALYSIS_MD = skillMd( - "data-analysis", - "Analyze a CSV and summarize its columns", + DATA_ANALYSIS_FM, "# Data analysis\n\nLoad the CSV, then follow `reference.md` for the column rules.", ); const DATA_ANALYSIS_REF = "# Column rules\n\nNumeric columns get min/max/mean; text columns get a value count.\n"; +const TAMPERED_FM: Frontmatter = { + name: "tampered-notes", + description: "A skill whose manifest digest does not match its served bytes", +}; const TAMPERED_MD = skillMd( - "tampered-notes", - "A skill whose manifest digest does not match its served bytes", + TAMPERED_FM, "# Tampered notes\n\nThe digest advertised for `notes.md` is wrong on purpose.", ); const TAMPERED_NOTES = "# Notes\n\nThese bytes hash to something other than what the manifest claims.\n"; +const DYNAMIC_FM: Frontmatter = { + name: "dynamic-report", + description: "A skill whose files are generated per request", +}; const DYNAMIC_MD = skillMd( - "dynamic-report", - "A skill whose files are generated per request", + DYNAMIC_FM, "# Dynamic report\n\nThis skill's file set is generated, so it advertises no manifest.", ); // The frontmatter says `right-name` while the URI segment says `wrong-folder`, // breaking the one structural invariant SEP-2640 states outright: the segment -// before /SKILL.md must equal frontmatter.name. +// before /SKILL.md must equal frontmatter.name. That is this fixture's ONLY +// violation — its listed and served frontmatter agree, as the SEP requires. +const MISMATCHED_FM: Frontmatter = { + name: "right-name", + description: + "A skill whose URI path segment disagrees with its frontmatter name", +}; const MISMATCHED_MD = skillMd( - "right-name", - "A skill whose URI path segment disagrees with its frontmatter name", + MISMATCHED_FM, "# Mismatched name\n\nServed from `wrong-folder/` while claiming the name `right-name`.", ); const FIXTURE_SKILLS: FixtureSkill[] = [ { path: "data-analysis", - frontmatter: { - name: "data-analysis", - description: "Analyze a CSV and summarize its columns", - }, + frontmatter: DATA_ANALYSIS_FM, files: [ { uri: "skill://data-analysis/SKILL.md", @@ -158,10 +186,7 @@ const FIXTURE_SKILLS: FixtureSkill[] = [ }, { path: "tampered-notes", - frontmatter: { - name: "tampered-notes", - description: "A skill whose manifest digest does not match its bytes", - }, + frontmatter: TAMPERED_FM, files: [ { uri: "skill://tampered-notes/SKILL.md", @@ -181,18 +206,12 @@ const FIXTURE_SKILLS: FixtureSkill[] = [ }, { path: "dynamic-report", - frontmatter: { - name: "dynamic-report", - description: "A skill whose files are generated per request", - }, + frontmatter: DYNAMIC_FM, files: "dynamic", }, { path: "wrong-folder", - frontmatter: { - name: "right-name", - description: "A skill whose URI segment disagrees with its name", - }, + frontmatter: MISMATCHED_FM, files: [ { uri: "skill://wrong-folder/SKILL.md",