From 60d5f403e6164c13675dd94b3eae3e7cf3a51828 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 7 Sep 2026 21:44:26 -0400 Subject: [PATCH] fix: address Copilot review round 11 on #2248 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Reads were bounded by entry count but not by total bytes.** A manifest can sit at exactly 512 entries and declare a gigabyte each, so round 9's cap still let a server dictate unbounded bandwidth and time after `size-limit-exceeded` had already been reported. `boundedManifest` caps on both interoperability limits now. Sizes are summed exactly as `totalSkillBytes` sums them, so the bound and the finding that reports the overage cannot disagree about the total. A read is skipped only when the running total would CROSS the limit, so a conforming skill — at most 16 MiB by definition — is never truncated; there is a test for that, because a bound that protected against a hostile server by giving a wrong answer about a good one would be a poor trade. The self-file fallback still applies, so the mandatory frontmatter check survives either cap. **The `vitest.shared.mts` pin rationale had gone stale.** It said `yaml` is reached only through `test-servers/src`, which this PR made untrue by importing it from `core/mcp/skillFile.ts`. That comment is what a future dependency-placement change would read before removing or reclassifying the pin, so it now names the `core/` path and the bundler `external` lists that follow from it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01ND46Ci64DC3tzBy9aG81Nc Signed-off-by: cliffhall --- .../test/core/mcp/skillsVerification.test.ts | 55 ++++++++++++++ core/mcp/skillsVerification.ts | 71 +++++++++++++++---- vitest.shared.mts | 12 +++- 3 files changed, 121 insertions(+), 17 deletions(-) diff --git a/clients/web/src/test/core/mcp/skillsVerification.test.ts b/clients/web/src/test/core/mcp/skillsVerification.test.ts index 6a72d1b5..5f642447 100644 --- a/clients/web/src/test/core/mcp/skillsVerification.test.ts +++ b/clients/web/src/test/core/mcp/skillsVerification.test.ts @@ -547,6 +547,61 @@ describe("verifySkills (#2248)", () => { ); }); + it("bounds reads by the total-byte limit, not only the entry count", async () => { + // A manifest can sit at exactly 512 entries and declare a gigabyte each, + // so bounding the count alone still let a server dictate unbounded + // bandwidth after `size-limit-exceeded` had already been reported + // (Copilot). + const huge = 8 * 1024 * 1024; // two of these cross the 16 MiB bound + const skill: SkillEntry = { + uri: "skill://fat/SKILL.md", + frontmatter: { name: "fat", description: "Enormous files" }, + resources: [ + { + uri: "skill://fat/SKILL.md", + digest: `sha256:${"a".repeat(64)}`, + size: huge, + }, + { + uri: "skill://fat/b.md", + digest: `sha256:${"a".repeat(64)}`, + size: huge, + }, + { + uri: "skill://fat/c.md", + digest: `sha256:${"a".repeat(64)}`, + size: huge, + }, + ], + }; + 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]); + // Two fit exactly; the third would cross, so it is never requested. + expect(readResource).toHaveBeenCalledTimes(2); + expect(report.conformance).toEqual( + expect.arrayContaining([ + expect.objectContaining({ code: "size-limit-exceeded" }), + ]), + ); + }); + + it("does not truncate a conforming manifest", async () => { + // A conforming skill totals at most 16 MiB by definition, so the bound + // must never shorten one — otherwise it would trade a hostile-server + // protection for a wrong answer about a good server. + const skill = await entry(); + const { client, readResource } = clientServing({ + "skill://demo/SKILL.md": SKILL_MD, + "skill://demo/ref.md": REF, + }); + const [report] = await verifySkills(client, [skill]); + expect(readResource).toHaveBeenCalledTimes(2); + expect(report.files).toHaveLength(2); + }); + 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 diff --git a/core/mcp/skillsVerification.ts b/core/mcp/skillsVerification.ts index 6dced8ae..666dd263 100644 --- a/core/mcp/skillsVerification.ts +++ b/core/mcp/skillsVerification.ts @@ -34,13 +34,18 @@ import { checkSkillNameCollisions, skillDisplayName, SKILL_MAX_RESOURCE_ENTRIES, + SKILL_MAX_TOTAL_BYTES, skillFileBytes, skillUriIdentity, verifySkillResource, type SkillIssue, type SkillVerification, } from "./skills.js"; -import { DYNAMIC_RESOURCES, type SkillEntry } from "./skillsSchemas.js"; +import { + DYNAMIC_RESOURCES, + type SkillEntry, + type SkillResource, +} from "./skillsSchemas.js"; /** One manifest entry's outcome. `read-error` means the fetch itself failed. */ export type SkillFileStatus = SkillVerification["status"] | "read-error"; @@ -71,11 +76,12 @@ export interface SkillVerifyReport { /** * 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. + * ⚠️ Capped at `SKILL_MAX_RESOURCE_ENTRIES` entries **and** + * `SKILL_MAX_TOTAL_BYTES` of declared content. A manifest over either bound + * is already reported by `resource-limit-exceeded` / `size-limit-exceeded`, + * and reading it anyway would let a server dictate unbounded round trips or + * bandwidth — 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 @@ -108,6 +114,33 @@ function reasonOf(err: unknown): string { return err instanceof Error ? err.message : String(err); } +/** + * The prefix of a manifest that may be read: at most + * {@link SKILL_MAX_RESOURCE_ENTRIES} entries, and at most + * {@link SKILL_MAX_TOTAL_BYTES} of declared content. + */ +function boundedManifest( + declared: readonly SkillResource[], +): readonly SkillResource[] { + const kept: SkillResource[] = []; + let bytes = 0; + for (const resource of declared) { + if (kept.length >= SKILL_MAX_RESOURCE_ENTRIES) break; + // Only a usable, non-negative size counts — matching `totalSkillBytes`, so + // the bound and the finding that reports the overage agree on the total. + const size = + typeof resource.size === "number" && + Number.isSafeInteger(resource.size) && + resource.size >= 0 + ? resource.size + : 0; + if (bytes + size > SKILL_MAX_TOTAL_BYTES) break; + bytes += size; + kept.push(resource); + } + return kept; +} + /** Result shape of one `resources/read`, narrowed to what a digest needs. */ interface ReadContents { text?: string; @@ -196,14 +229,24 @@ export async function verifySkills( 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); + // ⚠️ **Bounded on BOTH interoperability limits, because the manifest is + // server-controlled.** `checkSkillConformance` reports when either is + // exceeded — as warnings, since the SEP makes them bounds rather than MUSTs + // — but checking constrains nothing, and this loop then downloads whatever + // was advertised anyway. + // + // The entry count alone is not enough: a manifest can sit at exactly 512 + // entries and declare a gigabyte each, so bounding only the count still let + // a server dictate unbounded bandwidth and time (Copilot). Both overages + // are reported by `resource-limit-exceeded` / `size-limit-exceeded`; what + // stops here is *reading* them. + // + // Sizes are summed the way `totalSkillBytes` sums them — an entry + // declaring none contributes nothing, which cannot overstate the total and + // is bounded by the count cap regardless. A read is skipped only when the + // running total would CROSS the limit, so a conforming skill (≤ 16 MiB in + // total, by definition) is never truncated. + const manifest = boundedManifest(declared); const entryIdentity = skillUriIdentity(entry.uri); // Compared by NORMALIZED identity, like every other URI comparison here — // `checkSkillConformance` already accepts a manifest self-entry written in diff --git a/vitest.shared.mts b/vitest.shared.mts index 6f169e48..27d252ad 100644 --- a/vitest.shared.mts +++ b/vitest.shared.mts @@ -65,9 +65,15 @@ export function vitestSharedPaths(clientDir: string) { // is a statement about the installed tree, not about the manifests — check // the tree before adding or removing an entry. // - // `express` and `yaml` are reached only through `test-servers/src` — - // express by the http/oauth servers, yaml by `load-config.ts` — which is - // root-owned code with no manifest of its own. + // `express` is reached only through `test-servers/src` (the http/oauth + // servers), which is root-owned code with no manifest of its own. + // + // `yaml` was too — `load-config.ts` — but is now also a `core/` runtime + // import: `core/mcp/skillFile.ts` parses a served SKILL.md's frontmatter + // for the SEP-2640 cross-check (#2248). That matters to anyone revisiting + // this pin: it is no longer removable by retiring a test-server path, and + // as a dependency `core/` imports it is additionally named in all three + // bundler `external` lists. // // Pointing these at `/node_modules` is what broke when the MCP // packages moved to the root (#1970): express was never declared by a client