From 916b08ba933215642241510a160f3471305e9ec9 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 7 Sep 2026 20:10:23 -0400 Subject: [PATCH] fix: address Copilot review round 5 on #2248 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **A cyclic YAML alias crashed the tool.** `meta: &m [*m]` parses cleanly into a self-referential array, and the frontmatter comparison is a recursive walk, so a hostile server could take down `--verify` or the TUI with a stack overflow instead of receiving a finding. Verified before fixing: it really did raise `RangeError: Maximum call stack size exceeded`. Rejected at the parse, so no consumer has to be cycle-safe on its own, and it becomes an ordinary `frontmatter-unparsable` finding. Detection is per-PATH rather than per-graph — `seen` unwinds on the way back up — so an alias reused across siblings, which is ordinary YAML and represents fine in JSON, is not mistaken for a cycle. A depth bound closes the same hole by its other door: a legal, acyclic but absurdly nested document exhausts the stack just as well, and a cycle check alone passes it. **The web frontmatter check was gated on the presentation MIME.** It ran off `previewParts`, which exists only when the displayed type is recognized as markdown — so a `SKILL.md` a server typed `text/plain`, or served as a base64 blob, skipped a mandatory comparison while the pane still read as clean. It now decodes the same fetched bytes the digest is taken over, which also stops the two answers describing different derivations of the payload. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01ND46Ci64DC3tzBy9aG81Nc Signed-off-by: cliffhall --- .../SkillsScreen/SkillsScreen.test.tsx | 55 ++++++++++++++++++ .../screens/SkillsScreen/SkillsScreen.tsx | 32 +++++++---- .../web/src/test/core/mcp/skillFile.test.ts | 44 ++++++++++++++ core/mcp/skillFile.ts | 57 +++++++++++++++++++ 4 files changed, 178 insertions(+), 10 deletions(-) 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 }; }