From 88c1b895758e27825c0058d64b2a5f90cdf0cf58 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 7 Sep 2026 21:21:49 -0400 Subject: [PATCH] fix: address Copilot review round 9 on #2248 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Verification reads were unbounded.** The 512-entry limit is checked by `checkSkillConformance` — as a warning, since the SEP makes it an interoperability bound rather than a MUST — but checking it constrains nothing, so a hostile server advertising a million entries had the tool perform a million sequential reads after the report already knew the manifest was over. Capped at `SKILL_MAX_RESOURCE_ENTRIES`, with the finding still reported so bounding the work does not silence the reason it was unnecessary. `manifestListsSelf` is computed over the READ slice, so a self-entry pushed past the cap still reaches the fallback: the frontmatter comparison is mandatory and must not be lost to a limit that bounds other files. **A malformed skill URI still produced a directory root.** `skillUriIdentity` falls back to the raw string, so `not a uri/SKILL.md` yielded the root `not a uri` and enabled the Directory section — letting the UI build a request from a URI the conformance checks had already rejected. The comment above it claimed otherwise. `normalizeSkillUri` now, with the rule written down: identity is for COMPARING two spellings, not for deciding a URI is well-formed enough to build a request from. The existing regression test passed against a weaker input that failed for lacking the suffix and never reached the fallback; it is a table of three shapes now. **The test-server guide overstated its own coverage,** claiming four of the five conformance scenarios map onto fixtures. Three do: there is no size-mismatch fixture, because the fixture can override an advertised digest and nothing else, so that path is covered by unit tests. Corrected, with what adding one would take. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01ND46Ci64DC3tzBy9aG81Nc Signed-off-by: cliffhall --- .../SkillsScreen/SkillsScreen.test.tsx | 30 +++++++++ .../screens/SkillsScreen/SkillsScreen.tsx | 12 +++- .../test/core/mcp/skillsVerification.test.ts | 65 +++++++++++++++++++ core/mcp/skillsVerification.ts | 24 ++++++- docs/test-servers.md | 25 ++++--- 5 files changed, 143 insertions(+), 13 deletions(-) diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx index aaa2064b..89d5517a 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx @@ -2206,6 +2206,36 @@ describe("SkillsScreen directory browsing (#2248)", () => { ); }); + it.each([ + ["no /SKILL.md suffix", "not-a-uri"], + // Ends with the suffix and so LOOKS addressable, but does not parse. The + // identity fallback returned the raw string here, producing the "root" + // `not a uri` and enabling a directory request built from a URI the + // conformance checks had already rejected (Copilot). + ["unparseable but suffixed", "not a uri/SKILL.md"], + ["relative, not a full URI", "demo/SKILL.md"], + ])( + "renders no Directory section for a malformed skill URI (%s)", + async (_label, uri) => { + const user = userEvent.setup(); + const odd: SkillEntry = { + uri, + frontmatter: { name: "odd", description: "d" }, + resources: [], + }; + renderWithMantine( + , + ); + await user.click(screen.getByText("odd")); + expect( + screen.queryByRole("button", { name: /Directory/ }), + ).not.toBeInTheDocument(); + }, + ); + it("renders no Directory section for a skill whose URI is malformed", async () => { // There is no root to browse, and `malformed-uri` already reports it in // Conformance. diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx index 135a922d..fe7e6875 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx @@ -35,6 +35,7 @@ import { skillDisplayName, skillFileBytes, skillEntriesMatch, + normalizeSkillUri, skillUriIdentity, SKILL_FILE_SUFFIX, totalSkillBytes, @@ -1129,8 +1130,15 @@ export function SkillsScreen({ */ const skillRoot = useMemo(() => { if (!selected) return undefined; - const normalized = skillUriIdentity(selected.uri); - return normalized.endsWith(SKILL_FILE_SUFFIX) + // `normalizeSkillUri`, NOT `skillUriIdentity`: the latter falls back to the + // raw string when parsing fails, so `not a uri/SKILL.md` yielded the "root" + // `not a uri` and enabled the Directory section — letting the UI send a + // directory request derived from a URI the conformance checks had already + // rejected as malformed (Copilot). Identity is the right tool for + // COMPARING two spellings; it is the wrong one for deciding that a URI is + // well-formed enough to build a request from. + const normalized = normalizeSkillUri(selected.uri); + return normalized !== undefined && normalized.endsWith(SKILL_FILE_SUFFIX) ? normalized.slice(0, -SKILL_FILE_SUFFIX.length) : undefined; }, [selected]); diff --git a/clients/web/src/test/core/mcp/skillsVerification.test.ts b/clients/web/src/test/core/mcp/skillsVerification.test.ts index db6d7aec..6a72d1b5 100644 --- a/clients/web/src/test/core/mcp/skillsVerification.test.ts +++ b/clients/web/src/test/core/mcp/skillsVerification.test.ts @@ -518,6 +518,71 @@ describe("verifySkills (#2248)", () => { expect(report.files[0].status).toBe("verified"); }); + it("bounds reads at the interoperability limit, not at the manifest length", async () => { + // The 512-entry limit is CHECKED but constrains nothing, so a hostile + // server advertising far more had the tool perform that many sequential + // reads after the report already knew the manifest was over (Copilot). + const skill: SkillEntry = { + uri: "skill://huge/SKILL.md", + frontmatter: { name: "huge", description: "Too many files" }, + resources: Array.from({ length: 900 }, (_, i) => ({ + uri: i === 0 ? "skill://huge/SKILL.md" : `skill://huge/f${i}.md`, + digest: `sha256:${"a".repeat(64)}`, + size: 1, + })), + }; + const readResource = vi.fn(async (uri: string) => ({ + result: { contents: [{ uri, text: "x" }] }, + })); + const client = { readResource } as unknown as InspectorClientProtocol; + const [report] = await verifySkills(client, [skill]); + expect(readResource).toHaveBeenCalledTimes(512); + expect(report.files).toHaveLength(512); + // …and the overage is still REPORTED, so bounding the reads does not + // silence the finding that made them unnecessary. + expect(report.conformance).toEqual( + expect.arrayContaining([ + expect.objectContaining({ code: "resource-limit-exceeded" }), + ]), + ); + }); + + it("still reads the entry's own file when the cap would exclude it", async () => { + // The frontmatter comparison is mandatory and must not be lost to a limit + // that exists to bound *other* files — so a self-entry pushed past the cap + // by a bloated manifest reaches the fallback read. + const skillMd = "---\nname: huge\ndescription: Served\n---\n\n# H\n"; + const skill: SkillEntry = { + uri: "skill://huge/SKILL.md", + frontmatter: { name: "huge", description: "Listed" }, + resources: [ + ...Array.from({ length: 600 }, (_, i) => ({ + uri: `skill://huge/f${i}.md`, + digest: `sha256:${"a".repeat(64)}`, + size: 1, + })), + // Beyond the 512 cap. + { + uri: "skill://huge/SKILL.md", + digest: `sha256:${"a".repeat(64)}`, + size: 1, + }, + ], + }; + const readResource = vi.fn(async (uri: string) => ({ + result: { + contents: [{ uri, text: uri.endsWith("/SKILL.md") ? skillMd : "x" }], + }, + })); + const client = { readResource } as unknown as InspectorClientProtocol; + const [report] = await verifySkills(client, [skill]); + expect(readResource).toHaveBeenCalledWith( + "skill://huge/SKILL.md", + undefined, + ); + expect(report.frontmatter).toHaveLength(1); + }); + it("reports every skill it was given, in order", async () => { const a = await entry(); const b = await entry({ uri: "skill://demo/SKILL.md" }); diff --git a/core/mcp/skillsVerification.ts b/core/mcp/skillsVerification.ts index 6bb2f96d..6dced8ae 100644 --- a/core/mcp/skillsVerification.ts +++ b/core/mcp/skillsVerification.ts @@ -33,6 +33,7 @@ import { checkSkillFrontmatterMatch, checkSkillNameCollisions, skillDisplayName, + SKILL_MAX_RESOURCE_ENTRIES, skillFileBytes, skillUriIdentity, verifySkillResource, @@ -68,7 +69,13 @@ export interface SkillVerifyReport { */ frontmatter: SkillIssue[]; /** - * One entry per manifest file, in manifest order. + * One entry per manifest file **read**, in manifest order. + * + * ⚠️ Capped at `SKILL_MAX_RESOURCE_ENTRIES`. A manifest longer than that is + * already reported by the `resource-limit-exceeded` warning, and reading all + * of it would let a server dictate an unbounded number of round trips — so + * this can be SHORTER than the declared manifest, and a consumer must not + * read its length as the manifest's. * * ⚠️ **Not necessarily empty for a `"dynamic"` skill.** Such a skill has no * manifest rows, but a failed read of its own `SKILL.md` — the file the @@ -187,8 +194,16 @@ export async function verifySkills( let entryBytes: Uint8Array | undefined; const files: SkillFileReport[] = []; - const manifest = + const declared = entry.resources === DYNAMIC_RESOURCES ? [] : entry.resources; + // ⚠️ **Bounded, because the manifest is server-controlled.** The 512-entry + // limit is CHECKED by `checkSkillConformance` — as a warning, since the SEP + // makes it an interoperability bound rather than a MUST — but checking it + // constrains nothing, so a hostile or broken server advertising a million + // entries had the tool perform a million sequential reads after the report + // already knew the manifest was over the limit (Copilot). The overage is + // reported by `resource-limit-exceeded`; reading it is what stops here. + const manifest = declared.slice(0, SKILL_MAX_RESOURCE_ENTRIES); const entryIdentity = skillUriIdentity(entry.uri); // Compared by NORMALIZED identity, like every other URI comparison here — // `checkSkillConformance` already accepts a manifest self-entry written in @@ -244,7 +259,10 @@ export async function verifySkills( // // Gated on `manifestListsSelf` rather than on `entryBytes`, so a self-entry // the loop already tried and FAILED to read is not read a second time — its - // failure is recorded there. + // failure is recorded there. Note this is computed over the READ slice, so + // a self-entry pushed past the cap by a bloated manifest still reaches the + // fallback: the frontmatter comparison is mandatory and must not be lost to + // a limit that exists to bound unrelated files. if (!manifestListsSelf) { // Recorded as a file result, not swallowed. Because a dynamic skill has // no manifest rows, `files` would otherwise stay empty and its only static diff --git a/docs/test-servers.md b/docs/test-servers.md index 7acfe797..4b568162 100644 --- a/docs/test-servers.md +++ b/docs/test-servers.md @@ -145,14 +145,23 @@ They are the client-side obligations SEP-2640 makes testable from a hostile server, which is how the [`modelcontextprotocol/conformance`](https://github.com/modelcontextprotocol/conformance) harness grades a *client*: it stands up a server and watches what the client -does. Four of its five skills scenarios map onto a fixture here — a digest -mismatch (`tampered-notes`), a size mismatch, a frontmatter mismatch -(`lying-listing`), and a read of a file the manifest does not list -(`stale-manifest`). The fifth, **no-prefetch**, is a negative: it passes only if -connecting and calling `skills/list` produces *no* `resources/read` at all. The -Inspector satisfies it structurally — nothing is fetched until a user selects a -skill or presses Verify, which is why every round trip on the Skills screen is a -button rather than an effect. +does. **Three** of its five skills scenarios map onto a fixture here — a digest +mismatch (`tampered-notes`), a frontmatter mismatch (`lying-listing`), and a +read of a file the manifest does not list (`stale-manifest`). + +The other two are covered, but not by this fixture, and the distinction is worth +keeping honest: + +- **Size mismatch** has no fixture. `test-servers/src/skills.ts` can override an + advertised *digest* and nothing else, so the size path — which + `verifySkillResource` checks first, before hashing — is exercised by unit + tests rather than against a live server. Adding it would mean an + `advertisedSize` override beside the digest one. +- **No-prefetch** is a negative and could not have a fixture: it passes only if + connecting and calling `skills/list` produces *no* `resources/read` at all. + The Inspector satisfies it structurally — nothing is fetched until a user + selects a skill or presses Verify, which is why every round trip on the Skills + screen is a button rather than an effect. ## Cancelling a call