diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx index 90cee94d..3062735b 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx @@ -2324,6 +2324,61 @@ describe("SkillsScreen frontmatter cross-check (#2248)", () => { ).toBeInTheDocument(); }); + it("still checks a SKILL.md the server typed as something other than markdown", async () => { + // The check was gated on the DISPLAY mime, so a server labelling its + // SKILL.md `text/plain` skipped a mandatory comparison while the report + // still read as clean (Copilot). It runs against the fetched bytes now. + const user = userEvent.setup(); + const lying: SkillEntry = { + ...CLEAN_SKILL, + frontmatter: { + name: "data-analysis", + description: "Not what the file says", + }, + }; + renderWithMantine( + ({ + text: skillMdFor(CLEAN_FM), + mimeType: "text/plain", + }))} + />, + ); + await user.click(screen.getByText("data-analysis")); + await user.click(screen.getByRole("button", { name: /Conformance/ })); + await waitFor(() => + expect( + screen.getByTestId("skill-frontmatter-issues"), + ).toBeInTheDocument(), + ); + }); + + it("checks a SKILL.md served as a base64 blob", async () => { + // Same gap by its other door: a blob never produced `previewParts`. + const user = userEvent.setup(); + const lying: SkillEntry = { + ...CLEAN_SKILL, + frontmatter: { name: "data-analysis", description: "Disagrees" }, + }; + renderWithMantine( + ({ + blob: btoa(skillMdFor(CLEAN_FM)), + mimeType: "application/octet-stream", + }))} + />, + ); + await user.click(screen.getByText("data-analysis")); + await user.click(screen.getByRole("button", { name: /Conformance/ })); + await waitFor(() => + expect( + screen.getByTestId("skill-frontmatter-issues"), + ).toBeInTheDocument(), + ); + }); + it("reports nothing when the served frontmatter agrees", async () => { const user = userEvent.setup(); renderWithMantine(); diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx index 39eae21a..ea24a71a 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx @@ -31,6 +31,7 @@ import { checkSkillConformance, checkSkillFrontmatterMatch, checkSkillNameCollisions, + bytesToText, skillDisplayName, skillFileBytes, skillEntriesMatch, @@ -1396,17 +1397,28 @@ export function SkillsScreen({ * first time a file happened to be fetched. */ const frontmatterIssues = useMemo(() => { - if (!selected || !showingSkillMd || previewParts === undefined) return []; - // Reconstructed from the split rather than re-derived from the payload, so - // the check reads exactly the bytes the Frontmatter section displays. - if (previewParts.frontmatter === undefined) { - return checkSkillFrontmatterMatch(selected, previewParts.body); + if (!selected || !showingSkillMd || preview === undefined) return []; + // Run against the **raw fetched bytes**, not against `previewParts`. + // + // `previewParts` is a *presentation* value: it only exists when the + // displayed MIME is recognized as markdown, so a `SKILL.md` a server + // labelled `text/plain` — or anything else — skipped this check entirely + // while the report still read as clean (Copilot). The SEP makes the + // comparison mandatory for the skill's own file regardless of how the + // server typed it, and `showingSkillMd` already establishes that this IS + // that file. Decoding the same bytes the digest is taken over also keeps + // the two answers describing one payload rather than two derivations of it. + let text: string; + try { + text = bytesToText(skillFileBytes(preview)); + } catch { + // Neither text nor blob: there are no bytes to compare, and the file + // viewer already reports the empty response. Inventing a frontmatter + // finding here would name the wrong defect. + return []; } - return checkSkillFrontmatterMatch( - selected, - `---\n${previewParts.frontmatter}\n---\n\n${previewParts.body}`, - ); - }, [selected, showingSkillMd, previewParts]); + return checkSkillFrontmatterMatch(selected, text); + }, [selected, showingSkillMd, preview]); /** * The findings rendered as list items — everything except the two that are diff --git a/clients/web/src/test/core/mcp/skillFile.test.ts b/clients/web/src/test/core/mcp/skillFile.test.ts index 35223215..f7399c9f 100644 --- a/clients/web/src/test/core/mcp/skillFile.test.ts +++ b/clients/web/src/test/core/mcp/skillFile.test.ts @@ -137,6 +137,50 @@ describe("parseSkillFrontmatter (#2248)", () => { }); }); + it("rejects a cyclic YAML alias instead of crashing on it", () => { + // A YAML document is a graph and JSON is a tree: `&m [*m]` parses cleanly + // into a self-referential array, and the field-by-field comparison is a + // recursive walk — so this crashed `--verify` and the TUI with a stack + // overflow rather than producing a finding. A hostile server taking the + // tool down is a worse outcome than any wrong verdict (Copilot). + const parsed = parseSkillFrontmatter("meta: &m [*m]"); + expect(parsed).toEqual({ error: expect.stringContaining("cyclic") }); + }); + + it("rejects a cycle through a mapping, not only an array", () => { + expect(parseSkillFrontmatter("a: &a\n self: *a")).toEqual({ + error: expect.stringContaining("cyclic"), + }); + }); + + it("accepts a value that merely appears twice as a sibling", () => { + // An alias reused across siblings is ordinary YAML and represents fine in + // JSON — detection has to be per-path, not per-graph, or this would be + // reported as a cycle. + expect(parseSkillFrontmatter("base: &b [1, 2]\nx: *b\ny: *b")).toEqual({ + fields: { base: [1, 2], x: [1, 2], y: [1, 2] }, + }); + }); + + it("rejects a frontmatter nested past the depth bound", () => { + // The other door to the same crash: legal, acyclic, and still deep enough + // to exhaust the stack, which a cycle check alone would let through. + const deep = + "a:\n" + + Array.from({ length: 80 }, (_, i) => `${" ".repeat(i + 1)}a:`).join( + "\n", + ); + expect(parseSkillFrontmatter(deep)).toEqual({ + error: expect.stringContaining("nests deeper"), + }); + }); + + it("accepts ordinary nesting well inside the bound", () => { + expect(parseSkillFrontmatter("a:\n b:\n c: 1")).toEqual({ + fields: { a: { b: { c: 1 } } }, + }); + }); + it("reports invalid YAML with the parser's own message", () => { const parsed = parseSkillFrontmatter("a: [1,"); expect("error" in parsed && parsed.error.length > 0).toBe(true); diff --git a/core/mcp/skillFile.ts b/core/mcp/skillFile.ts index 297591b2..76a098d8 100644 --- a/core/mcp/skillFile.ts +++ b/core/mcp/skillFile.ts @@ -108,6 +108,59 @@ function hasContent(yamlText: string): boolean { .some((line) => line.trim() !== "" && !line.trimStart().startsWith("#")); } +/** + * Depth bound for a parsed frontmatter graph. + * + * Generous next to anything a real `SKILL.md` carries — the format's own fields + * are flat — and far below the stack the comparison walk would need. + */ +const MAX_FRONTMATTER_DEPTH = 64; + +/** + * Why a parsed frontmatter cannot be compared, or `undefined` when it can. + * + * ⚠️ **A YAML document is a graph, not a tree, and JSON is a tree.** An alias + * can refer to its own ancestor — `meta: &m [*m]` parses without error into a + * self-referential array — and the field-by-field comparison is a recursive + * walk, so such a value crashed `--verify` and the TUI with a stack overflow + * instead of producing a finding. That is a hostile server taking the tool + * down, so it is rejected here, at the parse, rather than defended against at + * every consumer (Copilot). + * + * The depth bound closes the same hole by its other door: a legal, acyclic but + * absurdly nested document would exhaust the stack just as effectively, and a + * cycle check alone would pass it. + * + * Detection is per-PATH, not per-graph: `seen` is added on the way down and + * removed on the way back up, so a value that merely appears twice as a sibling + * — which YAML aliases make ordinary and which JSON represents perfectly well — + * is not mistaken for a cycle. + */ +function jsonGraphError( + value: unknown, + seen: Set, + depth: number, +): string | undefined { + if (typeof value !== "object" || value === null) return undefined; + if (depth > MAX_FRONTMATTER_DEPTH) { + return `Frontmatter nests deeper than ${MAX_FRONTMATTER_DEPTH} levels, which cannot be compared field by field.`; + } + const node = value as object; + if (seen.has(node)) { + return "Frontmatter contains a cyclic YAML alias, which has no JSON equivalent and cannot be compared against the listing."; + } + seen.add(node); + const members = Array.isArray(node) + ? (node as unknown[]) + : Object.values(node as Record); + for (const member of members) { + const error = jsonGraphError(member, seen, depth + 1); + if (error) return error; + } + seen.delete(node); + return undefined; +} + export function parseSkillFrontmatter(yamlText: string): ParsedFrontmatter { let parsed: unknown; try { @@ -130,5 +183,9 @@ export function parseSkillFrontmatter(yamlText: string): ParsedFrontmatter { error: "Frontmatter is not a YAML mapping of fields.", }; } + // Checked BEFORE the value escapes this module, so no consumer has to be + // cycle-safe on its own. + const graphError = jsonGraphError(parsed, new Set(), 0); + if (graphError) return { error: graphError }; return { fields: parsed as Record }; }