diff --git a/clients/cli/README.md b/clients/cli/README.md index aeacebd6..4e2336fe 100644 --- a/clients/cli/README.md +++ b/clients/cli/README.md @@ -381,6 +381,21 @@ wrong" and "this skill could not be fully checked" are different answers: | `failed` | `7` | Something SEP-2640 makes a MUST was broken — an error-severity finding, a digest or size mismatch, or an unreadable manifest file. | | `incomplete` | `8` | Nothing checked was wrong, but the read bounds stopped the walk before it finished. See `incomplete` in the report for the reason. | +**The run is bounded, and says when a bound bit.** Three limits, all reported as +`incomplete` (`8`) rather than as a pass or a failure, because an entry that was +not read has not been cleared of anything: + +| Bound | Limit | Why | +| --- | --- | --- | +| Per skill | 512 manifest entries / 16 MiB | SEP-2640's own interoperability limits. | +| Per skill, on the wire | 16 MiB actually served | The declared sizes are server-controlled; this one cannot be lied past. | +| Per run | 256 skills / 64 MiB | SEP-2640 bounds a skill and deliberately does not bound a *catalog*. Every entry costs at least one `resources/read`, so without this a large listing — hostile or merely big — is unbounded work against the tool inspecting it. | + +The run bound is this tool's, not the spec's. A skill past it is still reported, +with its static conformance findings and an `incomplete` reason saying nothing +about its files was checked; verify it on its own with `--method skills/get +--uri ` to get a verdict for it. + A **warning** never produces `7`. That distinction matters most for `resources: "dynamic"`, which is a *conforming* wire form for generated content: it means integrity cannot be verified, which is worth reporting, but failing CI for it would tell server diff --git a/clients/cli/src/cli.ts b/clients/cli/src/cli.ts index fe19ee84..28ccd0dc 100644 --- a/clients/cli/src/cli.ts +++ b/clients/cli/src/cli.ts @@ -752,7 +752,7 @@ async function parseArgs(argv?: string[]): Promise { ) .option( "--verify", - "Run the SEP-2640 conformance and digest checks over the skills returned, emit one JSON report per skill on stdout, and exit 7 if any fails. Use with --method skills/list or --method skills/get.", + "Run the SEP-2640 conformance and digest checks over the skills returned, emit one JSON report per skill on stdout, and exit 7 if any fails or 8 if any could not be fully checked within the read bounds. Use with --method skills/list or --method skills/get.", ) .option( "--connect-timeout ", diff --git a/clients/web/src/test/core/mcp/skills.test.ts b/clients/web/src/test/core/mcp/skills.test.ts index ebcf1a11..04a250b5 100644 --- a/clients/web/src/test/core/mcp/skills.test.ts +++ b/clients/web/src/test/core/mcp/skills.test.ts @@ -1154,6 +1154,27 @@ describe("checkSkillNameCollisions (#2248)", () => { expect(first?.message).toContain("skill://c/r/SKILL.md"); }); + it("bounds a large collision group instead of transcribing it", () => { + // Duplicate names are legal and SEP-2640 puts no ceiling on a catalog, so + // naming every other member made both the work and the generated text + // O(N²) — a server controls N, which turns a legal listing into a denial + // of service against the tool sent to inspect it (Copilot). + const N = 500; + const collisions = checkSkillNameCollisions( + Array.from({ length: N }, (_, i) => at(`skill://s${i}/r/SKILL.md`, "r")), + ); + expect(collisions.size).toBe(N); + const message = collisions.get("skill://s0/r/SKILL.md")?.message ?? ""; + // Three named, the rest counted — enough to see what the collision IS and + // where to look, without a transcript of the catalog. + expect(message).toMatch(/and 496 more/); + expect(message).toContain("499 other skills in this listing also declare"); + // The bound is on the message, so its length cannot grow with the catalog. + expect(message.length).toBeLessThan(400); + // Still never names itself. + expect(message).not.toContain("skill://s0/r/SKILL.md"); + }); + it("does not report the SAME skill listed twice as a collision", () => { // A repeated entry is a different defect from two skills sharing a name, // and calling it this one would be a wrong diagnosis rather than a missing diff --git a/clients/web/src/test/core/mcp/skillsVerification.test.ts b/clients/web/src/test/core/mcp/skillsVerification.test.ts index 0f874339..e6ff5b07 100644 --- a/clients/web/src/test/core/mcp/skillsVerification.test.ts +++ b/clients/web/src/test/core/mcp/skillsVerification.test.ts @@ -1,11 +1,15 @@ import { describe, it, expect, vi } from "vitest"; import type { InspectorClientProtocol } from "@inspector/core/mcp/inspectorClientProtocol.js"; import type { SkillEntry } from "@inspector/core/mcp/skillsSchemas.js"; -import { sha256Digest } from "@inspector/core/mcp/skills.js"; +import { + SKILL_MAX_CATALOG_SKILLS, + sha256Digest, +} from "@inspector/core/mcp/skills.js"; import { AuthRecoveryRequiredError } from "@inspector/core/auth/challenge.js"; import { allSkillsVerified, anySkillFailed, + utf8Length, verifySkills, } from "@inspector/core/mcp/skillsVerification.js"; @@ -691,6 +695,101 @@ describe("verifySkills (#2248)", () => { expect(report.incomplete).toMatch(/actually served/); }); + it("charges non-ASCII text in UTF-8 bytes, not UTF-16 units", async () => { + // `text.length` undercharged every non-ASCII payload by up to 3×, so a + // decoy block of emoji kept the counter under 16 MiB while the wire + // carried twice that, and the walk read on (Copilot). Each block below is + // 3 MiB of UTF-16 units and 12 MiB of UTF-8 bytes, so two cross the limit + // under correct accounting and six would be needed under the old one. + const emoji = "🙂".repeat(1.5 * 1024 * 1024); // 2 units each, 4 bytes each + const { skill } = await truncatable({ name: "emoji", count: 20 }); + const readResource = vi.fn(async () => ({ + result: { + contents: [{ uri: "skill://elsewhere/decoy.md", text: emoji }], + }, + })); + const client = { readResource } as unknown as InspectorClientProtocol; + const [report] = await verifySkills(client, [skill]); + expect(readResource.mock.calls.length).toBe(3); + expect(report.incomplete).toMatch(/actually served/); + }); + + it("counts UTF-8 length exactly as TextEncoder does", async () => { + // The counter is hand-rolled to avoid allocating a copy of a payload a + // hostile server sized, so it is pinned against the reference encoder — + // including the surrogate cases that are the only reason it is not a + // one-liner. + const cases = [ + "", + "plain ascii", + "café", // 2-byte + "日本語", // 3-byte + "🙂👍", // surrogate pairs, 4-byte + "a🙂b", + "\ud83d", // lone HIGH surrogate — U+FFFD, 3 bytes + "\udc4d", // lone LOW surrogate + "\ud83d\ud83d", // two highs in a row: neither pairs + "end\ud83d", // unpaired high at the very end + ]; + const encoder = new TextEncoder(); + for (const value of cases) { + expect(utf8Length(value)).toBe(encoder.encode(value).byteLength); + } + }); + + it("stops reading once the run's catalog budget is spent", async () => { + // The per-skill caps bound what ONE entry costs; nothing bounded how many + // entries there are, and SEP-2640 puts no ceiling on a catalog — so a + // listing of a hundred thousand conforming skills made `--verify` run + // indefinitely (Copilot). + // Each skill carries its OWN SKILL.md, whose frontmatter matches its + // listing entry — otherwise every report is `failed` on a frontmatter + // mismatch and the budget is not what the test is measuring. + const enc = new TextEncoder(); + const mdFor = (i: number) => + `---\nname: s${i}\ndescription: A demo\n---\n\n# s${i}\n`; + const many = await Promise.all( + Array.from( + { length: SKILL_MAX_CATALOG_SKILLS + 5 }, + async (_, i): Promise => { + const bytes = enc.encode(mdFor(i)); + return entry({ + uri: `skill://s${i}/SKILL.md`, + frontmatter: { name: `s${i}`, description: "A demo" }, + resources: [ + { + uri: `skill://s${i}/SKILL.md`, + digest: await sha256Digest(bytes), + size: bytes.byteLength, + }, + ], + }); + }, + ), + ); + const readResource = vi.fn(async (uri: string) => ({ + result: { + contents: [ + { uri, text: mdFor(Number(/s(\d+)/.exec(uri)?.[1] ?? "0")) }, + ], + }, + })); + const client = { readResource } as unknown as InspectorClientProtocol; + const reports = await verifySkills(client, many); + // Every entry is still REPORTED — the static checks cost no I/O, so a + // skill past the budget is not silently dropped from the output. + expect(reports).toHaveLength(SKILL_MAX_CATALOG_SKILLS + 5); + expect(readResource.mock.calls.length).toBe(SKILL_MAX_CATALOG_SKILLS); + // …and the remainder says so, rather than passing or failing. + const past = reports.slice(SKILL_MAX_CATALOG_SKILLS); + for (const report of past) { + expect(report.outcome).toBe("incomplete"); + expect(report.incomplete).toMatch(/catalog budget/); + expect(report.files).toHaveLength(0); + } + expect(reports[0].outcome).toBe("verified"); + }); + it("is not incomplete when the budget is crossed by the LAST entry", async () => { // Crossing the line on the final row stopped nothing: every manifest entry // was fetched and checked. Reporting "Stopped after 4 of 4" there both diff --git a/core/mcp/skills.ts b/core/mcp/skills.ts index 8ae718d5..61dd7593 100644 --- a/core/mcp/skills.ts +++ b/core/mcp/skills.ts @@ -54,6 +54,28 @@ export const SKILL_MAX_RESOURCE_ENTRIES = 512; /** Maximum total size, in bytes, of a single skill's resources (16 MiB). */ export const SKILL_MAX_TOTAL_BYTES = 16 * 1024 * 1024; +/** + * Maximum skills one verification run will actually read from, and the byte + * ceiling across all of them. + * + * ⚠️ **Not SEP-2640 limits — they are this tool's own.** The SEP bounds a + * single skill and deliberately does not bound a catalog: `skills/list` may be + * arbitrarily long, and a page may hold arbitrarily many entries, so the + * cursor-walk's page cap constrains nothing here. Every entry costs at least + * one `resources/read`, so an unbounded catalog is unbounded work and + * unbounded transfer against the tool sent to inspect it — a `--verify` in CI + * that never returns (Copilot). + * + * Entries past either bound are reported as `incomplete` rather than dropped + * or failed: they were not checked, which is neither a pass nor a verdict + * against the server. A host wanting more is not wrong — these are safety + * limits, not conformance ones — which is why the reason names them. + */ +export const SKILL_MAX_CATALOG_SKILLS = 256; + +/** @see {@link SKILL_MAX_CATALOG_SKILLS} — 64 MiB across the whole run. */ +export const SKILL_MAX_CATALOG_BYTES = 64 * 1024 * 1024; + /** The suffix every skill URI ends with; the segment before it is the name. */ export const SKILL_FILE_SUFFIX = "/SKILL.md"; @@ -971,6 +993,13 @@ export function checkSkillFrontmatterMatch( return issues; } +/** + * How many colliding URIs a `duplicate-name` message names before it counts the + * rest. Three is enough to show the shape of the collision; the count carries + * the scale. + */ +const COLLISION_SAMPLE = 3; + /** * Findings that can only be computed over the **whole listing**, keyed by the * entry they belong to (its normalized URI identity). @@ -1025,11 +1054,35 @@ export function checkSkillNameCollisions( const uris = [...identities].sort(); for (const entry of group) { const self = skillUriIdentity(entry.uri); - const others = uris.filter((uri) => uri !== self); + // ⚠️ A bounded SAMPLE, taken with an early exit — not + // `uris.filter(...)` and not the whole list in the message. Duplicate + // names are legal and SEP-2640 puts no ceiling on a catalog, so a group + // of N made both the work and the generated text O(N²): every one of N + // entries scanned all N URIs and embedded the other N−1 (Copilot). A + // server controls N, which turns a legal listing into a denial of + // service against the tool meant to inspect it. + const sample: string[] = []; + for (const uri of uris) { + if (uri === self) continue; + sample.push(uri); + if (sample.length === COLLISION_SAMPLE) break; + } + const unshown = identities.size - 1 - sample.length; + // Naming a few and counting the rest keeps the finding actionable — a + // reader needs to see that it IS a collision and where to look, not a + // transcript of the catalog. + const others = + unshown > 0 + ? `${sample.join(", ")}, and ${unshown} more` + : sample.join(", "); + const subject = + identities.size === 2 + ? "Another skill in this listing also declares" + : `${identities.size - 1} other skills in this listing also declare`; issues.set(self, { code: "duplicate-name", severity: "warning", - message: `Another skill in this listing also declares the name "${name}" (${others.join(", ")}). This is legal — a consumer must tell them apart by their URIs rather than collapsing or preferring one.`, + message: `${subject} the name "${name}" (${others}). This is legal — a consumer must tell them apart by their URIs rather than collapsing or preferring one.`, }); } } diff --git a/core/mcp/skillsVerification.ts b/core/mcp/skillsVerification.ts index c4ee2da9..c034b465 100644 --- a/core/mcp/skillsVerification.ts +++ b/core/mcp/skillsVerification.ts @@ -33,6 +33,8 @@ import { checkSkillFrontmatterMatch, checkSkillNameCollisions, skillDisplayName, + SKILL_MAX_CATALOG_BYTES, + SKILL_MAX_CATALOG_SKILLS, SKILL_MAX_RESOURCE_ENTRIES, SKILL_MAX_TOTAL_BYTES, skillFileBytes, @@ -219,6 +221,42 @@ function contentsFor(result: unknown, uri: string): ReadContents | undefined { return undefined; } +/** + * How many bytes a string occupies as UTF-8, without encoding a copy of it. + * + * `TextEncoder` would be the obvious answer and allocates a second buffer for + * a payload that may already be megabytes — and this is called on responses a + * hostile server chose the size of, which is the case the count exists to + * bound. Counting is O(n) and allocates nothing. + * + * A high surrogate is only worth 4 bytes when a low surrogate actually follows + * it. An unpaired one encodes as U+FFFD, which is 3 — the same as any other + * BMP character in that range, so it needs no special case beyond not + * consuming the next unit. + */ +export function utf8Length(value: string): number { + let total = 0; + for (let i = 0; i < value.length; i += 1) { + const code = value.charCodeAt(i); + if (code < 0x80) { + total += 1; + } else if (code < 0x800) { + total += 2; + } else if (code >= 0xd800 && code <= 0xdbff) { + const next = i + 1 < value.length ? value.charCodeAt(i + 1) : 0; + if (next >= 0xdc00 && next <= 0xdfff) { + total += 4; + i += 1; + } else { + total += 3; + } + } else { + total += 3; + } + } + return total; +} + /** * What a `resources/read` response cost to receive, charged against the byte * budget regardless of whether any of it is usable. @@ -230,12 +268,13 @@ function contentsFor(result: unknown, uri: string): ReadContents | undefined { * or send an enormous blob that is not valid base64. Both leave the decode * paths empty-handed while the bytes have already crossed the wire. * - * The figure is an **approximation, and deliberately never an undercount by - * more than a small factor**: `text` is charged in UTF-16 code units (UTF-8 is - * between 1× and 3× that for the same string) and `blob` in base64 characters - * (roughly 4/3 of the bytes it decodes to, so an overcharge). Exactness is not - * the point — this is a safety limit, not an accounting figure, and the caller - * substitutes the exact decoded length whenever it has one. + * `text` is charged in **UTF-8 bytes**, the unit the limit is written in. + * Charging `text.length` instead — UTF-16 code units — undercharged every + * non-ASCII payload by up to 3×, so a decoy block of emoji or CJK kept the + * counter under 16 MiB while the wire carried far more, and the walk read on + * (Copilot). `blob` is charged in base64 characters, which is ~4/3 of what it + * decodes to: an OVERcharge, and deliberately left as one, since a blob that + * fails to decode has no byte count to be exact about. */ function responseBytes(result: unknown): number { const contents = (result as { contents?: unknown })?.contents; @@ -244,7 +283,7 @@ function responseBytes(result: unknown): number { for (const block of contents) { if (typeof block !== "object" || block === null) continue; const { text, blob } = block as { text?: unknown; blob?: unknown }; - if (typeof text === "string") total += text.length; + if (typeof text === "string") total += utf8Length(text); if (typeof blob === "string") total += blob.length; } return total; @@ -278,7 +317,21 @@ export async function verifySkills( // reports no collision: there is no listing to collide within. const collisions = checkSkillNameCollisions(entries); const reports: SkillVerifyReport[] = []; + // ⚠️ Run-level budgets, on top of the per-skill ones below. The per-skill + // caps bound what ONE entry can cost; nothing bounded how many entries there + // are, and SEP-2640 puts no ceiling on a catalog — so a listing of a hundred + // thousand skills, each individually conforming, made `--verify` run + // indefinitely and transfer unboundedly (Copilot). See + // {@link SKILL_MAX_CATALOG_SKILLS}. + let walkedSkills = 0; + let catalogBytes = 0; for (const entry of entries) { + // Static checks still run for every entry — they cost no I/O, so a skill + // past the budget is still reported on, just not read. What stops is the + // reading. + const withinBudget = + walkedSkills < SKILL_MAX_CATALOG_SKILLS && + catalogBytes <= SKILL_MAX_CATALOG_BYTES; // The entry's own SKILL.md, read once and used twice — for its digest and // for the frontmatter cross-check. Reading it twice would double the load // on the server and, worse, could compare a digest against one snapshot @@ -313,9 +366,10 @@ export async function verifySkills( // 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); - let incomplete = - manifest.length < declared.length + const manifest = withinBudget ? boundedManifest(declared) : []; + let incomplete = !withinBudget + ? `Not read: this run already reached its catalog budget of ${SKILL_MAX_CATALOG_SKILLS} skills / ${SKILL_MAX_CATALOG_BYTES} bytes. Nothing about this skill's files has been checked — verify it on its own with \`--method skills/get --uri\` to get a verdict.` + : manifest.length < declared.length ? `Only ${manifest.length} of ${declared.length} manifest entries were read: the skill exceeds the ${SKILL_MAX_RESOURCE_ENTRIES}-entry / ${SKILL_MAX_TOTAL_BYTES}-byte interoperability limits, so the rest were not fetched and cannot be reported on.` : undefined; // ⚠️ Bytes ACTUALLY RECEIVED, which is the only budget a server cannot @@ -436,7 +490,7 @@ export async function verifySkills( // because a cap excluded it or because the byte budget broke the loop // first, still gets the fallback: the frontmatter comparison is mandatory // and must not be lost to a limit that exists to bound unrelated files. - if (!selfAttempted) { + if (withinBudget && !selfAttempted) { // Recorded as a file result, not swallowed. Because a dynamic skill has // no manifest rows, `files` would otherwise stay empty and its only static // finding is a warning — so an unreadable SKILL.md returned `ok: true` @@ -485,6 +539,13 @@ export async function verifySkills( } } + // Charged after this entry's reads, so the skill that crosses the run + // budget is still fully reported rather than half-read. The NEXT one stops. + if (withinBudget) { + walkedSkills += 1; + catalogBytes += receivedBytes; + } + const entryText = entryBytes === undefined ? undefined : bytesToText(entryBytes);