modelcontextprotocol/inspector
Watch
1
0
Fork
You've already forked inspector
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 2

fix: address Copilot review round 8 on #2251

Browse source 
- useServerCommands: require the returned URI to match the requested one
  in `onReadSkillFile`. The "a single-block response must be the block we
  asked for" fallback is right for a viewer and wrong here: these bytes
  are hashed against the requested 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 it `verified`. A
  normalized equivalent is still accepted (a server may echo a resolved
  `..`), but two unparseable URIs do not compare equal, and the error
  names what came back instead.
- test-servers/skills.ts: build each `SKILL.md` FROM its frontmatter
  object. Three fixtures listed one description and served another, which
  is an undocumented extra violation — SEP-2640 requires the listed and
  served frontmatter to be identical — and 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 drift impossible
  rather than merely fixed.

The PR description's severity table also still described the round-1
contract (limits as errors, digest/size/description as warnings) while
the code says the reverse. Rewritten to match, with the MUST/SHOULD split
stated rather than implied.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018kfrH3uRNUM4JAk1reKtyY
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-09-04 23:46:56 -04:00
commit 7e291f92f0
3 changed files with 106 additions and 39 deletions
Download patch file Download diff file Expand all files Collapse all files

38
clients/web/src/hooks/useServerCommands.test.tsx
View file

@ -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({

32
clients/web/src/hooks/useServerCommands.tsx
View file

@ -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<SkillFileContents> => {
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

75
test-servers/src/skills.ts
View file

@ -96,53 +96,81 @@ interface FixtureFile {
interface FixtureSkill {
/** The `<skill-path>` segment; `skill://<path>/SKILL.md` is the entry URI. */
path: string;
frontmatter: Record<string, unknown>;
/** 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",