fix: address Copilot review round 5 on #2248

**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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ND46Ci64DC3tzBy9aG81Nc
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-09-07 20:10:23 -04:00
commit 916b08ba93

View file

@ -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<object>,
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<string, unknown>);
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<string, unknown> };
}