From 5c9787c72354af2e55721ab87d3cd0797c419c97 Mon Sep 17 00:00:00 2001 From: Ossie Irondi Date: Sat, 18 Jul 2026 17:54:18 -0500 Subject: [PATCH 01/11] fix(validators): parse YAML frontmatter properly, contain bundle references MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Post-merge review of #33 (issue #10 hardening) found two P1 gaps: - readFrontmatterField/checkSkillFrontmatter tokenized frontmatter text instead of parsing YAML, so description: null/~/[]/{} or a bare comment all passed the required-description gate as truthy strings. Both validate-plugin-ports.mjs and validate-agent-skills.mjs now share scripts/lib/frontmatter.mjs, which parses the block with the yaml package and requires a trimmed non-empty string. - The SKILL.md reference guard skipped bare scripts/, cli/, assets/, hooks/ references entirely (issue #10 requires those shipped resources to exist) and joined paths without rejecting '..' or confirming containment, so a crafted tools/../../... reference could resolve to a file outside the plugin bundle. scripts/lib/bundle-refs.mjs now validates every unambiguous bundle reference, rejects traversal segments, and asserts the resolved path stays inside the plugin root. Widening bare-reference validation surfaced two genuine false positives (cross-skill delegation text in awesome-readme, a generic example in harness-audit) — reworded both so they no longer read as ambiguous same-bundle paths. Added table-driven regression fixtures for both (scripts/lib/frontmatter-test.mjs, scripts/lib/bundle-refs-test.mjs), wired into npm test. --- .../skills/awesome-readme/SKILL.md | 4 +- .../skills/harness-audit/SKILL.md | 2 +- package.json | 5 +- scripts/lib/bundle-refs-test.mjs | 45 +++++++++ scripts/lib/bundle-refs.mjs | 26 +++++ scripts/lib/frontmatter-test.mjs | 61 ++++++++++++ scripts/lib/frontmatter.mjs | 33 +++++++ scripts/validate-agent-skills.mjs | 54 ++--------- scripts/validate-plugin-ports.mjs | 96 +++++++------------ 9 files changed, 211 insertions(+), 115 deletions(-) create mode 100644 scripts/lib/bundle-refs-test.mjs create mode 100644 scripts/lib/bundle-refs.mjs create mode 100644 scripts/lib/frontmatter-test.mjs create mode 100644 scripts/lib/frontmatter.mjs diff --git a/claude-code/plugins/awesome-readme/skills/awesome-readme/SKILL.md b/claude-code/plugins/awesome-readme/skills/awesome-readme/SKILL.md index 6bb11f1..e971b83 100644 --- a/claude-code/plugins/awesome-readme/skills/awesome-readme/SKILL.md +++ b/claude-code/plugins/awesome-readme/skills/awesome-readme/SKILL.md @@ -60,7 +60,7 @@ If neither (1) nor (2) is set up and the user picks "Yes, generate via gpt-image When a workflow needs an image, invoke the **gpt-image-2** skill directly: ```bash -bash scripts/gen.sh \ +bash gen.sh \ --prompt "" \ --out docs/.png ``` @@ -68,7 +68,7 @@ bash scripts/gen.sh \ For reference-image conditioning (remix an existing logo, restyle a screenshot, etc.): ```bash -bash scripts/gen.sh \ +bash gen.sh \ --prompt "" \ --ref /absolute/path/to/reference.png \ --out docs/.png diff --git a/claude-code/plugins/harness-audit/skills/harness-audit/SKILL.md b/claude-code/plugins/harness-audit/skills/harness-audit/SKILL.md index 465999a..2d29624 100644 --- a/claude-code/plugins/harness-audit/skills/harness-audit/SKILL.md +++ b/claude-code/plugins/harness-audit/skills/harness-audit/SKILL.md @@ -64,7 +64,7 @@ For each artifact, score `✅ Present / ⚠️ Partial / ❌ Missing` with concr 2. **Rules directory** — per-domain coding standards in `rules/`, `.cursor/rules/`, `docs/rules/`, `docs/solutions/`, etc. Imperative rules with reasoning. For repos with exported APIs, this includes an API documentation policy or ADR. 3. **Lint config with rich error messages** — ESLint / Biome / SwiftLint / ruff / clippy with custom rules whose messages explain why and how to fix. 4. **Pre-commit hooks** — Husky / lefthook / pre-commit / native git hooks running formatters, type checks, or targeted tests before push/commit. -5. **Test suite agents can drive** — one-liner to run tests (`npm test`, `./scripts/test.sh`, `cargo test`, etc.). Fast enough and deterministic enough for agents. +5. **Test suite agents can drive**: one-liner to run tests (`npm test`, `make test`, `cargo test`, etc.). Fast enough and deterministic enough for agents. 6. **PR review automation** — CodeRabbit, AI reviewer workflows, persona reviewers, or at minimum CI that gates PRs. 7. **Repo-scoped skills/prompts** — `.agents/skills/`, `.pi/skills/`, `.claude/skills/`, `.codex/`, `.pi/prompts/`, or package-scoped resources. Prefer 5-10 deep resources over many shallow ones. 8. **Garbage-collection cadence** — documented recurring loop that converts agent/PR feedback into permanent rules, lints, docs, or skills. diff --git a/package.json b/package.json index a87a377..796ae11 100644 --- a/package.json +++ b/package.json @@ -53,7 +53,7 @@ "lint": "biome lint .", "lint:fix": "biome check --write .", "prepare": "husky", - "test": "node extensions/autopilot/v2-smoke-test.mjs && node extensions/question/question-smoke-test.mjs && node extensions/conditional-hooks/smoke-test.mjs && python3 skills/diataxis-docs-site/tests/test_create_site.py && node scripts/validate-package-contents.mjs", + "test": "node extensions/autopilot/v2-smoke-test.mjs && node extensions/question/question-smoke-test.mjs && node extensions/conditional-hooks/smoke-test.mjs && node scripts/lib/frontmatter-test.mjs && node scripts/lib/bundle-refs-test.mjs && python3 skills/diataxis-docs-site/tests/test_create_site.py && node scripts/validate-package-contents.mjs", "typecheck": "tsc --noEmit", "validate:skills": "node scripts/validate-agent-skills.mjs" }, @@ -74,7 +74,8 @@ "husky": "9.1.7", "lint-staged": "16.4.0", "typebox": "latest", - "typescript": "latest" + "typescript": "latest", + "yaml": "^2.8.2" }, "engines": { "node": ">=20" diff --git a/scripts/lib/bundle-refs-test.mjs b/scripts/lib/bundle-refs-test.mjs new file mode 100644 index 0000000..5485c8b --- /dev/null +++ b/scripts/lib/bundle-refs-test.mjs @@ -0,0 +1,45 @@ +import assert from "node:assert/strict"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { resolveBundleReference } from "./bundle-refs.mjs"; + +const tmp = fs.mkdtempSync(path.join(os.tmpdir(), "bundle-refs-")); +const base = path.join(tmp, "skills", "fixture-plugin"); +fs.mkdirSync(path.join(base, "scripts"), { recursive: true }); +fs.writeFileSync(path.join(base, "scripts", "gen.sh"), "#!/bin/sh\necho ok\n"); +fs.mkdirSync(path.join(tmp, "secret"), { recursive: true }); +fs.writeFileSync(path.join(tmp, "secret", "outside.md"), "should never be reachable\n"); + +// ok: reference stays inside base and the file exists. +{ + const result = resolveBundleReference(base, "scripts/gen.sh"); + assert.equal(result.ok, true, "existing in-bundle script must resolve ok"); + assert.equal(result.resolved, path.join(base, "scripts", "gen.sh")); +} + +// missing: reference stays inside base but no file exists there. +const MISSING_CASES = ["scripts/does-not-exist.sh", "assets/missing-logo.png", "tools/absent-tool.ts"]; +for (const pathPart of MISSING_CASES) { + const result = resolveBundleReference(base, pathPart); + assert.equal(result.ok, false, `${pathPart}: expected not ok`); + assert.equal(result.reason, "missing", `${pathPart}: expected reason "missing"`); +} + +// traversal: reference walks out of base, even when it lands on a real file +// elsewhere in the repo (the actual attack this guards against). +const TRAVERSAL_CASES = [ + "scripts/../../secret/outside.md", + "../../secret/outside.md", + "scripts/../../../secret/outside.md", + "scripts/./gen.sh", +]; +for (const pathPart of TRAVERSAL_CASES) { + const result = resolveBundleReference(base, pathPart); + assert.equal(result.ok, false, `${pathPart}: expected not ok`); + assert.equal(result.reason, "traversal", `${pathPart}: expected reason "traversal"`); +} + +fs.rmSync(tmp, { recursive: true, force: true }); + +console.log("bundle reference resolution fixtures passed"); diff --git a/scripts/lib/bundle-refs.mjs b/scripts/lib/bundle-refs.mjs new file mode 100644 index 0000000..8ff714c --- /dev/null +++ b/scripts/lib/bundle-refs.mjs @@ -0,0 +1,26 @@ +import fs from "node:fs"; +import path from "node:path"; + +/** + * Resolve a local bundle reference (e.g. `scripts/gen.sh`) against `base` + * and report whether it is safe and exists. + * + * - `traversal`: the reference contains a "." or ".." path segment, or its + * resolved path falls outside `base` — rejected before ever touching disk. + * - `missing`: the reference stays inside `base` but no file exists there. + * - ok: the reference stays inside `base` and resolves to an existing file. + */ +export function resolveBundleReference(base, pathPart) { + if (pathPart.split("/").some((segment) => segment === "." || segment === "..")) { + return { ok: false, reason: "traversal" }; + } + const resolved = path.join(base, pathPart); + const rel = path.relative(base, resolved); + if (rel.startsWith("..") || path.isAbsolute(rel)) { + return { ok: false, reason: "traversal" }; + } + if (!fs.existsSync(resolved)) { + return { ok: false, reason: "missing", resolved }; + } + return { ok: true, resolved }; +} diff --git a/scripts/lib/frontmatter-test.mjs b/scripts/lib/frontmatter-test.mjs new file mode 100644 index 0000000..8c1b7da --- /dev/null +++ b/scripts/lib/frontmatter-test.mjs @@ -0,0 +1,61 @@ +import assert from "node:assert/strict"; +import { isNonEmptyString, parseFrontmatter } from "./frontmatter.mjs"; + +const FILE = "fixture.md"; + +function frontmatterBlock(descriptionLine) { + return `---\nname: fixture\n${descriptionLine}\n---\n\nBody text.\n`; +} + +// Valid `description` values: parseFrontmatter must return exactly the +// expected string, and isNonEmptyString must accept it. +const VALID_DESCRIPTIONS = [ + { label: "plain scalar", line: "description: hello world", expected: "hello world" }, + { label: "double-quoted", line: 'description: "hello world"', expected: "hello world" }, + { label: "single-quoted", line: "description: 'hello world'", expected: "hello world" }, + { label: "folded block scalar", line: "description: >\n hello\n world", expected: "hello world\n" }, + { label: "literal block scalar", line: "description: |\n hello\n world", expected: "hello\nworld\n" }, +]; + +for (const { label, line, expected } of VALID_DESCRIPTIONS) { + const frontmatter = parseFrontmatter(frontmatterBlock(line), FILE); + assert.equal(frontmatter.description, expected, `${label}: unexpected parsed value`); + assert.ok(isNonEmptyString(frontmatter.description), `${label}: expected to be accepted as non-empty string`); +} + +// Invalid `description` values: parseFrontmatter still succeeds (the +// frontmatter block itself is valid YAML), but isNonEmptyString must reject +// the resulting value. These are the exact false-negative shapes the +// hand-rolled tokenizer previously accepted. +const INVALID_DESCRIPTIONS = [ + { label: "YAML null (explicit)", line: "description: null" }, + { label: "YAML null (tilde)", line: "description: ~" }, + { label: "empty sequence", line: "description: []" }, + { label: "empty mapping", line: "description: {}" }, + { label: "comment-only (parses to null)", line: "description: # just a comment" }, + { label: "empty string", line: 'description: ""' }, + { label: "whitespace-only string", line: 'description: " "' }, + { label: "field absent", line: "other: value" }, +]; + +for (const { label, line } of INVALID_DESCRIPTIONS) { + const frontmatter = parseFrontmatter(frontmatterBlock(line), FILE); + assert.ok(!isNonEmptyString(frontmatter.description), `${label}: expected to be rejected as non-empty string`); +} + +// Malformed frontmatter blocks: parseFrontmatter itself must throw. +const MALFORMED_BLOCKS = [ + { + label: "malformed YAML (unterminated quoted string)", + content: '---\nname: fixture\ndescription: "unterminated\n---\n', + }, + { label: "missing frontmatter block entirely", content: "# just a heading\n\nno frontmatter here\n" }, + { label: "unterminated frontmatter (no closing ---)", content: "---\nname: fixture\ndescription: hi\n" }, + { label: "frontmatter is a sequence, not a mapping", content: "---\n- one\n- two\n---\n" }, +]; + +for (const { label, content } of MALFORMED_BLOCKS) { + assert.throws(() => parseFrontmatter(content, FILE), undefined, `${label}: expected parseFrontmatter to throw`); +} + +console.log("frontmatter parser fixtures passed"); diff --git a/scripts/lib/frontmatter.mjs b/scripts/lib/frontmatter.mjs new file mode 100644 index 0000000..952675c --- /dev/null +++ b/scripts/lib/frontmatter.mjs @@ -0,0 +1,33 @@ +import { parse as parseYaml } from "yaml"; + +/** + * Parse a leading `--- ... ---` YAML frontmatter block into a mapping. + * Throws when the block is absent, unterminated, not valid YAML, or does + * not parse to a mapping (e.g. a bare scalar or sequence document). + */ +export function parseFrontmatter(content, filePath) { + const normalized = content.replace(/\r\n/g, "\n"); + if (!normalized.startsWith("---\n")) { + throw new Error(`${filePath}: missing YAML frontmatter block`); + } + const end = normalized.indexOf("\n---", 4); + if (end === -1) { + throw new Error(`${filePath}: unterminated YAML frontmatter block`); + } + const raw = normalized.slice(4, end); + let data; + try { + data = parseYaml(raw); + } catch (err) { + throw new Error(`${filePath}: frontmatter is not valid YAML: ${err.message}`); + } + if (data === null || typeof data !== "object" || Array.isArray(data)) { + throw new Error(`${filePath}: frontmatter must be a YAML mapping`); + } + return data; +} + +/** True only for a trimmed, non-empty string value (rejects null, numbers, collections). */ +export function isNonEmptyString(value) { + return typeof value === "string" && value.trim().length > 0; +} diff --git a/scripts/validate-agent-skills.mjs b/scripts/validate-agent-skills.mjs index eaacc43..b6460a6 100644 --- a/scripts/validate-agent-skills.mjs +++ b/scripts/validate-agent-skills.mjs @@ -1,6 +1,7 @@ #!/usr/bin/env node import { readdir, readFile } from "node:fs/promises"; import { basename, join, relative } from "node:path"; +import { isNonEmptyString, parseFrontmatter } from "./lib/frontmatter.mjs"; const SKILL_NAME_RE = /^[a-z0-9]+(?:-[a-z0-9]+)*$/; const DESCRIPTION_MAX_LENGTH = 1024; @@ -24,47 +25,6 @@ async function walk(dir) { return out; } -function parseFrontmatter(content, filePath) { - const normalized = content.replace(/\r\n/g, "\n"); - if (!normalized.startsWith("---\n")) { - throw new Error(`${filePath}: missing YAML frontmatter block`); - } - - const end = normalized.indexOf("\n---", 4); - if (end === -1) { - throw new Error(`${filePath}: unterminated YAML frontmatter block`); - } - - const lines = normalized.slice(4, end).split("\n"); - const fields = new Map(); - - for (let i = 0; i < lines.length; i += 1) { - const line = lines[i]; - if (!line.trim() || line.trimStart().startsWith("#")) continue; - if (line.startsWith(" ") || line.startsWith("\t")) continue; - - const match = line.match(/^([A-Za-z0-9_-]+):\s*(.*)$/); - if (!match) continue; - - const [, key, rawValue] = match; - const value = rawValue.trim(); - if (["|", ">", "|-", ">-"].includes(value)) { - const block = []; - for (let j = i + 1; j < lines.length; j += 1) { - const next = lines[j]; - if (next && !next.startsWith(" ") && !next.startsWith("\t")) break; - block.push(next.trim()); - i = j; - } - fields.set(key, block.filter(Boolean).join(" ").trim()); - } else { - fields.set(key, value.replace(/^['\"]|['\"]$/g, "")); - } - } - - return fields; -} - async function validateSkillsShConfig(skillNames) { const file = "skills.sh.json"; let parsed; @@ -111,19 +71,19 @@ const warnings = []; const byName = new Map(); for (const file of files) { - let fields; + let frontmatter; try { - fields = parseFrontmatter(await readFile(file, "utf8"), file); + frontmatter = parseFrontmatter(await readFile(file, "utf8"), file); } catch (error) { errors.push(error.message); continue; } - const name = fields.get("name"); - const description = fields.get("description"); + const name = frontmatter.name; + const description = frontmatter.description; const expectedName = basename(file.replace(/\/SKILL\.md$/, "")); - if (!name) errors.push(`${file}: missing required frontmatter field: name`); + if (!isNonEmptyString(name)) errors.push(`${file}: missing required frontmatter field: name`); else { if (name !== expectedName) errors.push(`${file}: name '${name}' must match parent directory '${expectedName}'`); if (!SKILL_NAME_RE.test(name)) errors.push(`${file}: name '${name}' must be lowercase kebab-case`); @@ -132,7 +92,7 @@ for (const file of files) { byName.set(name, existing); } - if (!description) errors.push(`${file}: missing required frontmatter field: description`); + if (!isNonEmptyString(description)) errors.push(`${file}: missing required frontmatter field: description`); else if (description.length > DESCRIPTION_MAX_LENGTH) { errors.push(`${file}: description is ${description.length} chars; max is ${DESCRIPTION_MAX_LENGTH}`); } diff --git a/scripts/validate-plugin-ports.mjs b/scripts/validate-plugin-ports.mjs index d9407e1..eb45887 100644 --- a/scripts/validate-plugin-ports.mjs +++ b/scripts/validate-plugin-ports.mjs @@ -14,6 +14,8 @@ import fs from "node:fs"; import path from "node:path"; import { fileURLToPath } from "node:url"; +import { resolveBundleReference } from "./lib/bundle-refs.mjs"; +import { isNonEmptyString, parseFrontmatter } from "./lib/frontmatter.mjs"; const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), ".."); const pluginsDir = path.join(repoRoot, "claude-code", "plugins"); @@ -68,31 +70,6 @@ function checkReadme(plugin) { } } -/** - * Parse a leading `--- ... ---` YAML frontmatter block. - * Returns the raw frontmatter text, or null when absent. - */ -function readFrontmatter(text) { - const match = /^---\r?\n([\s\S]*?)\r?\n---\r?\n/.exec(text); - return match ? match[1] : null; -} - -function readFrontmatterField(frontmatter, field) { - const lines = frontmatter.split(/\r?\n/); - const index = lines.findIndex((line) => line.startsWith(`${field}:`)); - if (index === -1) return null; - - const raw = lines[index].slice(field.length + 1).trim(); - if (raw !== ">" && raw !== "|") return raw.replace(/^["']|["']$/g, ""); - - const block = []; - for (const line of lines.slice(index + 1)) { - if (!/^\s+/.test(line)) break; - block.push(line.trim()); - } - return block.join(raw === ">" ? " " : "\n").trim(); -} - /** Path to a plugin's canonical SKILL.md (skills//SKILL.md). */ function skillPath(plugin) { return path.join(pluginsDir, plugin, "skills", plugin, "SKILL.md"); @@ -105,59 +82,43 @@ function checkSkillFrontmatter(plugin) { fail(plugin, `missing SKILL.md at skills/${plugin}/SKILL.md`); return; } - const frontmatter = readFrontmatter(fs.readFileSync(file, "utf8")); - if (frontmatter === null) { - fail(plugin, "SKILL.md has no YAML frontmatter"); + let frontmatter; + try { + frontmatter = parseFrontmatter(fs.readFileSync(file, "utf8"), file); + } catch (err) { + fail(plugin, err.message); return; } - const name = readFrontmatterField(frontmatter, "name"); - if (!name) { + const name = frontmatter.name; + if (!isNonEmptyString(name)) { fail(plugin, "SKILL.md frontmatter has no name field"); return; } if (name !== plugin) { fail(plugin, `SKILL.md frontmatter name "${name}" does not match plugin "${plugin}"`); } - const description = readFrontmatterField(frontmatter, "description"); - if (!description) fail(plugin, "SKILL.md frontmatter has no description field"); + if (!isNonEmptyString(frontmatter.description)) { + fail(plugin, "SKILL.md frontmatter has no description field"); + } } // --- Check 4: SKILL.md references resolve to files on disk --------------- -// A SKILL.md path reference is only validated when it is unambiguously a -// reference to *this plugin's own bundle*: -// -// - Any `${CLAUDE_PLUGIN_ROOT}/...` path — explicitly plugin-root-relative. -// - A bare `workflows/`, `references/`, or `tools/` path — these subdir -// names are skill-bundle vocabulary, effectively never used for anything -// else. -// -// Bare `scripts/`, `cli/`, `assets/`, `hooks/` paths are NOT validated unless -// `${CLAUDE_PLUGIN_ROOT}`-prefixed: those are universal repo conventions and -// appear as illustrative examples (an audit skill citing `./scripts/test.sh`) -// or cross-skill delegation (`scripts/gen.sh` belonging to another skill). -// Bare filenames (e.g. `data-layer.md`) are likewise not validated — too -// easily confused with filenames inside code examples. +// A SKILL.md path reference is only matched when it is unambiguously a +// reference to *this plugin's own bundle*: either `${CLAUDE_PLUGIN_ROOT}/...` +// (explicitly plugin-root-relative) or a bare `workflows/`, `references/`, +// `tools/`, `scripts/`, `cli/`, `assets/`, or `hooks/` path — this fixed +// vocabulary of skill-bundle subdirectory names is what makes the match +// unambiguous. Every match is validated: it must resolve to a file that +// exists, and the resolved path must stay inside the plugin's own root +// (rejects `..` traversal segments that could otherwise walk out of the +// bundle and match an unrelated file elsewhere in the repo). const REF_EXT = "md|ts|tsx|sh|json|js|mjs|cjs|py"; const ALL_SUBDIRS = ["workflows", "references", "tools", "scripts", "cli", "assets", "hooks"]; -const BARE_OK_SUBDIRS = new Set(["workflows", "references", "tools"]); const REF_PATTERN = new RegExp( `(\\$\\{CLAUDE_PLUGIN_ROOT\\}/)?((?:skills/[a-z0-9-]+/)?(?:${ALL_SUBDIRS.join("|")})/[A-Za-z0-9._/-]+\\.(?:${REF_EXT}))`, "g", ); -/** First bundle-subdir component of a reference path (after any skills//). */ -function refSubdir(pathPart) { - return pathPart.replace(/^skills\/[a-z0-9-]+\//, "").split("/")[0]; -} - -/** Resolve a SKILL.md reference to an absolute path in the plugin port. */ -function resolveRef(plugin, prefixed, pathPart) { - // `${CLAUDE_PLUGIN_ROOT}/...` is plugin-root-relative; a bare reference is - // relative to the skill directory (skills//). - const base = prefixed ? path.join(pluginsDir, plugin) : path.join(pluginsDir, plugin, "skills", plugin); - return path.join(base, pathPart); -} - function checkSkillReferences(plugin) { const file = skillPath(plugin); if (!fs.existsSync(file)) return; // already reported by checkSkillFrontmatter @@ -165,13 +126,22 @@ function checkSkillReferences(plugin) { const seen = new Set(); for (const [, prefix, pathPart] of text.matchAll(REF_PATTERN)) { const prefixed = Boolean(prefix); - if (!prefixed && !BARE_OK_SUBDIRS.has(refSubdir(pathPart))) continue; // biome-ignore lint/suspicious/noTemplateCurlyInString: literal "${CLAUDE_PLUGIN_ROOT}" placeholder text, not a JS template const ref = (prefixed ? "${CLAUDE_PLUGIN_ROOT}/" : "") + pathPart; if (seen.has(ref)) continue; seen.add(ref); - if (!fs.existsSync(resolveRef(plugin, prefixed, pathPart))) { - fail(plugin, `SKILL.md references missing file: ${ref}`); + + // `${CLAUDE_PLUGIN_ROOT}/...` is plugin-root-relative; a bare reference + // is relative to the skill directory (skills//). + const base = prefixed ? path.join(pluginsDir, plugin) : path.join(pluginsDir, plugin, "skills", plugin); + const result = resolveBundleReference(base, pathPart); + if (!result.ok) { + fail( + plugin, + result.reason === "traversal" + ? `SKILL.md reference escapes the plugin bundle: ${ref}` + : `SKILL.md references missing file: ${ref}`, + ); } } } From d86fdb6a0252a8b202bfc70817e602952cd8d959 Mon Sep 17 00:00:00 2001 From: Ossie Irondi Date: Sat, 18 Jul 2026 17:54:32 -0500 Subject: [PATCH 02/11] fix(packaging): stop generated Python bytecode from reaching the pack MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Merge-SHA CI's npm run pack:dry included codex/plugins/html-docs/scripts/__pycache__/markdown_to_html.cpython-312.pyc (41.0 kB) — the html-docs fixture verifier imports markdown_to_html.py via importlib during its own junk-artifact scan step in validate-codex-plugins.mjs, and CPython writes the .pyc cache after that scan runs, so it survives into a later npm run pack:dry. - Pass PYTHONDONTWRITEBYTECODE=1 to the fixture-verifier subprocess so it is never generated in the first place. - Add a pack-content assertion in validate-package-contents.mjs (already wired into npm test) that fails the build if any __pycache__/ or *.py[cod] path ever appears in the npm pack --dry-run file list. - Add __pycache__/ and *.py[cod] to .gitignore for local git-status hygiene. --- .gitignore | 2 ++ scripts/validate-codex-plugins.mjs | 6 +++++- scripts/validate-package-contents.mjs | 13 +++++++++---- 3 files changed, 16 insertions(+), 5 deletions(-) diff --git a/.gitignore b/.gitignore index 5a11619..a9e42e0 100644 --- a/.gitignore +++ b/.gitignore @@ -1,6 +1,8 @@ node_modules/ .pnpm-store/ .DS_Store +__pycache__/ +*.py[cod] .env .env.* !.env.example diff --git a/scripts/validate-codex-plugins.mjs b/scripts/validate-codex-plugins.mjs index 601ac2e..667daa0 100644 --- a/scripts/validate-codex-plugins.mjs +++ b/scripts/validate-codex-plugins.mjs @@ -157,7 +157,11 @@ for (const plugin of plugins) { const fixtureVerifier = path.join(pluginRoot, "scripts", "verify_fixtures.py"); if (fs.existsSync(fixtureVerifier)) { - const result = spawnSync("python3", [fixtureVerifier], { cwd: repoRoot, encoding: "utf8" }); + const result = spawnSync("python3", [fixtureVerifier], { + cwd: repoRoot, + encoding: "utf8", + env: { ...process.env, PYTHONDONTWRITEBYTECODE: "1" }, + }); if (result.status !== 0) { fail(plugin, `fixture verifier failed: ${(result.stderr || result.stdout).trim()}`); } else if (result.stdout.trim()) { diff --git a/scripts/validate-package-contents.mjs b/scripts/validate-package-contents.mjs index b8d85ad..bf0e53d 100644 --- a/scripts/validate-package-contents.mjs +++ b/scripts/validate-package-contents.mjs @@ -11,7 +11,8 @@ if (result.status !== 0) { } const [pack] = JSON.parse(result.stdout); -const packedPaths = new Set(pack.files.map((file) => file.path)); +const packedPaths = pack.files.map((file) => file.path); +const packedPathSet = new Set(packedPaths); const requiredPaths = [ "skills/diataxis-docs-site/SKILL.md", "skills/diataxis-docs-site/agents/openai.yaml", @@ -20,10 +21,14 @@ const requiredPaths = [ "skills/github-wiki/SKILL.md", "skills/github-wiki/agents/openai.yaml", ]; -const missing = requiredPaths.filter((requiredPath) => !packedPaths.has(requiredPath)); +const missing = requiredPaths.filter((requiredPath) => !packedPathSet.has(requiredPath)); +const forbidden = packedPaths.filter( + (packedPath) => /(?:^|\/)__pycache__(?:\/|$)/.test(packedPath) || /\.py[cod]$/.test(packedPath), +); -if (missing.length > 0) { - console.error(`Missing required package files:\n${missing.join("\n")}`); +if (missing.length > 0 || forbidden.length > 0) { + if (missing.length > 0) console.error(`Missing required package files:\n${missing.join("\n")}`); + if (forbidden.length > 0) console.error(`Forbidden generated artifacts in package:\n${forbidden.join("\n")}`); process.exit(1); } From bbd7dce90276af0309843324f4d4fcb4abcd722a Mon Sep 17 00:00:00 2001 From: Ossie Irondi Date: Sat, 18 Jul 2026 17:54:43 -0500 Subject: [PATCH 03/11] fix(ask-codex): restore codex-binary prerequisite, fix dead schema URL The rebase that squashed into #33 dropped 'Requires the codex binary on PATH' from plugin.json and marketplace.json while README.md and skills/ask-codex/SKILL.md kept it, breaking the four-surface consistency issue #10 requires. Restore the identical sentence to both, keeping the plugin.json/marketplace.json descriptions byte-for-byte equal (required by validate-plugin-ports.mjs's marketplace description check). Also replace marketplace.json's dead $schema (https://anthropic.com/claude-code/marketplace.schema.json, 404) with the live SchemaStore Claude Code marketplace schema. Editor-only, not a runtime blocker, but free to fix alongside the rest of this scope. --- .claude-plugin/marketplace.json | 4 ++-- claude-code/plugins/ask-codex/.claude-plugin/plugin.json | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index 7d7b657..be5cd0e 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -1,5 +1,5 @@ { - "$schema": "https://anthropic.com/claude-code/marketplace.schema.json", + "$schema": "https://json.schemastore.org/claude-code-marketplace.json", "name": "agentic-utilities", "description": "Curated Claude Code plugins from agentic-utilities — a harness-agnostic collection of skills, plugins, and extensions across pi, Claude Code, and Codex. This marketplace exposes the Claude Code plugins.", "owner": { @@ -54,7 +54,7 @@ }, { "name": "ask-codex", - "description": "OpenAI Codex CLI integration for Claude Code. Wraps `codex exec` and `codex resume` with model selection (gpt-5.4 / gpt-5.3-codex / gpt-5.4-nano), reasoning effort control (high/medium/low), sandbox modes (read-only / workspace-write / danger-full-access), session resumption, and stderr suppression for thinking tokens.", + "description": "OpenAI Codex CLI integration for Claude Code. Wraps `codex exec` and `codex resume` with model selection (gpt-5.4 / gpt-5.3-codex / gpt-5.4-nano), reasoning effort control (high/medium/low), sandbox modes (read-only / workspace-write / danger-full-access), session resumption, and stderr suppression for thinking tokens. Requires the `codex` binary on PATH.", "version": "0.1.0", "author": { "name": "Ossie Irondi" diff --git a/claude-code/plugins/ask-codex/.claude-plugin/plugin.json b/claude-code/plugins/ask-codex/.claude-plugin/plugin.json index 6fc83e0..2e15644 100644 --- a/claude-code/plugins/ask-codex/.claude-plugin/plugin.json +++ b/claude-code/plugins/ask-codex/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "ask-codex", "version": "0.1.0", - "description": "OpenAI Codex CLI integration for Claude Code. Wraps `codex exec` and `codex resume` with model selection (gpt-5.4 / gpt-5.3-codex / gpt-5.4-nano), reasoning effort control (high/medium/low), sandbox modes (read-only / workspace-write / danger-full-access), session resumption, and stderr suppression for thinking tokens.", + "description": "OpenAI Codex CLI integration for Claude Code. Wraps `codex exec` and `codex resume` with model selection (gpt-5.4 / gpt-5.3-codex / gpt-5.4-nano), reasoning effort control (high/medium/low), sandbox modes (read-only / workspace-write / danger-full-access), session resumption, and stderr suppression for thinking tokens. Requires the `codex` binary on PATH.", "author": { "name": "Ossie Irondi" }, From e9ea68ac46b16c277d0bd54ca64ec847debaa0f8 Mon Sep 17 00:00:00 2001 From: Ossie Irondi Date: Sat, 18 Jul 2026 18:22:37 -0500 Subject: [PATCH 04/11] fix(validators): end-to-end reference extraction and filesystem containment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two P1 gaps from the SHA-pinned re-review: - REF_EXT excluded ordinary asset types (png, svg, jpg, jpeg, gif, webp), so a reference like assets/missing-logo.png never reached matching at all — the prior bundle-refs-test.mjs fixture called resolveBundleReference directly, never the actual regex extraction used in production, so it never proved the real path worked. extractBundleReferences moves into scripts/lib/bundle-refs.mjs (shared, testable) and widens REF_EXT to cover real asset extensions used in the repo's own plugins. - A token-boundary bug let ${CLAUDE_PLUGIN_ROOT}/../tools/outside.ts get rematched as the bare reference tools/outside.ts, silently dropping the traversal prefix — a same-named file inside the bundle could then make an escaping reference pass. extractBundleReferences now requires a proper token boundary before a match (a negative lookbehind rejects matches preceded by a word/path character) and captures leading ../ segments as part of the reference itself, so traversal is detected downstream instead of disappearing. Also closes the 'contained file' gap: resolveBundleReference is pure again (no filesystem access, matching the lib/ boundary rule), only '..' segments count as traversal (a safe scripts/./gen.sh no longer false- positives), and the filesystem-facing checks (existence, regular-file, symlink real-path containment) move to a new exported checkReferenceOnDisk in validate-plugin-ports.mjs — the validator, not the lib. A directory or an escaping symlink now fails even when the lexical path looks contained. validate-plugin-ports.mjs gains a guarded main() so its exported checkReferenceOnDisk can be imported by tests without running a full validation pass against the real repo. Extended bundle-refs-test.mjs with extractor+resolver integration cases (the exact assets/missing-logo.png and CLAUDE_PLUGIN_ROOT-traversal scenarios from the review, including a same-named-file-inside-bundle adversarial case) and checkReferenceOnDisk fixtures (directory, symlink escape, safe symlink, safe dot-segment). Verified via stash: these tests fail against the pre-fix code (a SyntaxError on the missing export). --- scripts/lib/bundle-refs-test.mjs | 166 +++++++++++++++++++++++++----- scripts/lib/bundle-refs.mjs | 49 +++++++-- scripts/validate-plugin-ports.mjs | 121 ++++++++++++++-------- 3 files changed, 256 insertions(+), 80 deletions(-) diff --git a/scripts/lib/bundle-refs-test.mjs b/scripts/lib/bundle-refs-test.mjs index 5485c8b..6c4f85a 100644 --- a/scripts/lib/bundle-refs-test.mjs +++ b/scripts/lib/bundle-refs-test.mjs @@ -2,44 +2,162 @@ import assert from "node:assert/strict"; import fs from "node:fs"; import os from "node:os"; import path from "node:path"; -import { resolveBundleReference } from "./bundle-refs.mjs"; +import { checkReferenceOnDisk } from "../validate-plugin-ports.mjs"; +import { extractBundleReferences, resolveBundleReference } from "./bundle-refs.mjs"; + +const SUBDIRS = ["workflows", "references", "tools", "scripts", "cli", "assets", "hooks"]; +const EXTENSIONS = [ + "md", + "ts", + "tsx", + "sh", + "json", + "js", + "mjs", + "cjs", + "py", + "png", + "jpg", + "jpeg", + "gif", + "svg", + "webp", +]; +function extract(text) { + return extractBundleReferences(text, { subdirs: SUBDIRS, extensions: EXTENSIONS }); +} + +// --- extractBundleReferences: token boundaries and asset coverage -------- + +{ + // A production-realistic image reference must actually be extracted — the + // prior REF_EXT list excluded image extensions entirely, so this never + // reached resolveBundleReference at all regardless of what it returned. + const refs = extract("See `assets/missing-logo.png` for the logo."); + assert.equal(refs.length, 1); + assert.deepEqual(refs[0], { prefixed: false, pathPart: "assets/missing-logo.png", ref: "assets/missing-logo.png" }); +} + +{ + // A reference embedded mid-path, preceded by a word/path character, must + // not "resync" and match as if it started a fresh token. + const refs = extract("xtools/outside.ts should never match as a bare reference"); + assert.equal(refs.length, 0, "mid-identifier text must not match"); +} + +{ + // The exact adversarial case from the review: a ${CLAUDE_PLUGIN_ROOT}/../ + // reference must keep its traversal prefix attached to the captured + // pathPart, not get reinterpreted as the bare tail `tools/outside.ts`. + // biome-ignore lint/suspicious/noTemplateCurlyInString: literal "${CLAUDE_PLUGIN_ROOT}" placeholder text, not a JS template + const refs = extract("Run ${CLAUDE_PLUGIN_ROOT}/../tools/outside.ts now."); + assert.equal(refs.length, 1, "must extract exactly one reference, not a bare-tail duplicate"); + assert.deepEqual(refs[0], { + prefixed: true, + pathPart: "../tools/outside.ts", + // biome-ignore lint/suspicious/noTemplateCurlyInString: literal "${CLAUDE_PLUGIN_ROOT}" placeholder text, not a JS template + ref: "${CLAUDE_PLUGIN_ROOT}/../tools/outside.ts", + }); +} + +// --- resolveBundleReference: pure lexical containment (no filesystem) ---- + +{ + const result = resolveBundleReference("/tmp/base", "scripts/gen.sh"); + assert.equal(result.ok, true); + assert.equal(result.resolved, path.join("/tmp/base", "scripts", "gen.sh")); +} + +{ + // A lone "." (current-dir) segment is a safe no-op and must be accepted, + // not rejected as traversal. + const result = resolveBundleReference("/tmp/base", "scripts/./gen.sh"); + assert.equal(result.ok, true, "scripts/./gen.sh must not be treated as traversal"); +} + +for (const pathPart of ["scripts/../../secret/outside.md", "../../secret/outside.md", "../tools/outside.ts"]) { + const result = resolveBundleReference("/tmp/base", pathPart); + assert.equal(result.ok, false, `${pathPart}: expected not ok`); + assert.equal(result.reason, "traversal", `${pathPart}: expected reason "traversal"`); +} + +// --- checkReferenceOnDisk: filesystem boundary (existence, regular-file, +// symlink containment) lives at the validator, not in the pure lib --------- const tmp = fs.mkdtempSync(path.join(os.tmpdir(), "bundle-refs-")); const base = path.join(tmp, "skills", "fixture-plugin"); fs.mkdirSync(path.join(base, "scripts"), { recursive: true }); +fs.mkdirSync(path.join(base, "tools"), { recursive: true }); fs.writeFileSync(path.join(base, "scripts", "gen.sh"), "#!/bin/sh\necho ok\n"); +// A directory whose name matches the reference vocabulary + extension +// pattern (e.g. a stray "tool.ts" directory) must not pass as a file. +fs.mkdirSync(path.join(base, "tools", "tool.ts"), { recursive: true }); +// A real file planted at the *bare* traversal-tail location, to prove the +// adversarial same-named-file case: even though tools/outside.ts genuinely +// exists inside the bundle, a reference that actually reads +// ${CLAUDE_PLUGIN_ROOT}/../tools/outside.ts must still be rejected as +// traversal — it must never be matched against this coincidental file. +fs.writeFileSync(path.join(base, "tools", "outside.ts"), "// decoy, must never be reached via traversal\n"); fs.mkdirSync(path.join(tmp, "secret"), { recursive: true }); -fs.writeFileSync(path.join(tmp, "secret", "outside.md"), "should never be reachable\n"); +fs.writeFileSync(path.join(tmp, "secret", "real-target.md"), "should never be reachable\n"); +fs.symlinkSync(path.join(tmp, "secret", "real-target.md"), path.join(base, "scripts", "escape-link.md")); +fs.symlinkSync(path.join(base, "scripts", "gen.sh"), path.join(base, "scripts", "safe-link.sh")); -// ok: reference stays inside base and the file exists. { - const result = resolveBundleReference(base, "scripts/gen.sh"); - assert.equal(result.ok, true, "existing in-bundle script must resolve ok"); - assert.equal(result.resolved, path.join(base, "scripts", "gen.sh")); + // missing: lexically contained, no file on disk. + const resolved = path.join(base, "assets", "missing-logo.png"); + assert.equal(checkReferenceOnDisk(base, resolved).reason, "missing"); } -// missing: reference stays inside base but no file exists there. -const MISSING_CASES = ["scripts/does-not-exist.sh", "assets/missing-logo.png", "tools/absent-tool.ts"]; -for (const pathPart of MISSING_CASES) { - const result = resolveBundleReference(base, pathPart); - assert.equal(result.ok, false, `${pathPart}: expected not ok`); - assert.equal(result.reason, "missing", `${pathPart}: expected reason "missing"`); +{ + // not-a-file: a directory sitting at the exact reference path. + const resolved = path.join(base, "tools", "tool.ts"); + assert.equal(checkReferenceOnDisk(base, resolved).reason, "not-a-file"); } -// traversal: reference walks out of base, even when it lands on a real file -// elsewhere in the repo (the actual attack this guards against). -const TRAVERSAL_CASES = [ - "scripts/../../secret/outside.md", - "../../secret/outside.md", - "scripts/../../../secret/outside.md", - "scripts/./gen.sh", -]; -for (const pathPart of TRAVERSAL_CASES) { - const result = resolveBundleReference(base, pathPart); - assert.equal(result.ok, false, `${pathPart}: expected not ok`); - assert.equal(result.reason, "traversal", `${pathPart}: expected reason "traversal"`); +{ + // symlink-escape: the reference's own path is inside the bundle, but it + // points at a real target outside it. + const resolved = path.join(base, "scripts", "escape-link.md"); + assert.equal(checkReferenceOnDisk(base, resolved).reason, "symlink-escape"); } +{ + // A symlink whose target stays inside the bundle is a legitimate file. + const resolved = path.join(base, "scripts", "safe-link.sh"); + assert.equal(checkReferenceOnDisk(base, resolved).reason, "ok"); +} + +{ + // A genuine in-bundle file. + const resolved = path.join(base, "scripts", "gen.sh"); + assert.equal(checkReferenceOnDisk(base, resolved).reason, "ok"); +} + +// --- End-to-end: extractor -> resolver -> on-disk check, chained exactly +// the way validate-plugin-ports.mjs's checkSkillReferences runs them ------- + +function validateReference(skillMdText, pluginBase) { + const [entry] = extract(skillMdText); + assert.ok(entry, "expected exactly one extracted reference"); + const lexical = resolveBundleReference(pluginBase, entry.pathPart); + if (!lexical.ok) return lexical.reason; + return checkReferenceOnDisk(pluginBase, lexical.resolved).reason; +} + +assert.equal( + validateReference("See `assets/missing-logo.png` for the logo.", base), + "missing", + "production pipeline must flag a missing PNG asset reference", +); + +assert.equal( + // biome-ignore lint/suspicious/noTemplateCurlyInString: literal "${CLAUDE_PLUGIN_ROOT}" placeholder text, not a JS template + validateReference("Run ${CLAUDE_PLUGIN_ROOT}/../tools/outside.ts now.", base), + "traversal", + "production pipeline must reject the traversal reference even though tools/outside.ts genuinely exists in the bundle", +); + fs.rmSync(tmp, { recursive: true, force: true }); console.log("bundle reference resolution fixtures passed"); diff --git a/scripts/lib/bundle-refs.mjs b/scripts/lib/bundle-refs.mjs index 8ff714c..38a22ff 100644 --- a/scripts/lib/bundle-refs.mjs +++ b/scripts/lib/bundle-refs.mjs @@ -1,17 +1,47 @@ -import fs from "node:fs"; import path from "node:path"; /** - * Resolve a local bundle reference (e.g. `scripts/gen.sh`) against `base` - * and report whether it is safe and exists. + * Extract every unambiguous local bundle reference from `text`. * - * - `traversal`: the reference contains a "." or ".." path segment, or its - * resolved path falls outside `base` — rejected before ever touching disk. - * - `missing`: the reference stays inside `base` but no file exists there. - * - ok: the reference stays inside `base` and resolves to an existing file. + * A reference is unambiguous only when it starts at a token boundary — not + * preceded by a word/path character, so a match can never "resync" mid-path + * and swallow part of a larger expression (e.g. `${CLAUDE_PLUGIN_ROOT}/../tools/x.ts` + * must never be reinterpreted as the bare reference `tools/x.ts`). Leading + * `../` segments are captured as part of the reference itself, not silently + * dropped, so traversal is detected downstream instead of disappearing. + */ +export function extractBundleReferences(text, { subdirs, extensions }) { + const pattern = new RegExp( + `(? segment === ".."); +} + +/** + * Resolve a local bundle reference against `base`, purely lexically: no + * filesystem access. Rejects traversal (".." segments, or a normalized + * result outside `base`) before any I/O would happen. Callers are + * responsible for the filesystem-facing checks (existence, regular-file, + * symlink containment) once a reference resolves `ok`. */ export function resolveBundleReference(base, pathPart) { - if (pathPart.split("/").some((segment) => segment === "." || segment === "..")) { + if (hasTraversalSegment(pathPart)) { return { ok: false, reason: "traversal" }; } const resolved = path.join(base, pathPart); @@ -19,8 +49,5 @@ export function resolveBundleReference(base, pathPart) { if (rel.startsWith("..") || path.isAbsolute(rel)) { return { ok: false, reason: "traversal" }; } - if (!fs.existsSync(resolved)) { - return { ok: false, reason: "missing", resolved }; - } return { ok: true, resolved }; } diff --git a/scripts/validate-plugin-ports.mjs b/scripts/validate-plugin-ports.mjs index eb45887..320c34a 100644 --- a/scripts/validate-plugin-ports.mjs +++ b/scripts/validate-plugin-ports.mjs @@ -14,7 +14,7 @@ import fs from "node:fs"; import path from "node:path"; import { fileURLToPath } from "node:url"; -import { resolveBundleReference } from "./lib/bundle-refs.mjs"; +import { extractBundleReferences, resolveBundleReference } from "./lib/bundle-refs.mjs"; import { isNonEmptyString, parseFrontmatter } from "./lib/frontmatter.mjs"; const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), ".."); @@ -108,41 +108,66 @@ function checkSkillFrontmatter(plugin) { // (explicitly plugin-root-relative) or a bare `workflows/`, `references/`, // `tools/`, `scripts/`, `cli/`, `assets/`, or `hooks/` path — this fixed // vocabulary of skill-bundle subdirectory names is what makes the match -// unambiguous. Every match is validated: it must resolve to a file that -// exists, and the resolved path must stay inside the plugin's own root -// (rejects `..` traversal segments that could otherwise walk out of the -// bundle and match an unrelated file elsewhere in the repo). -const REF_EXT = "md|ts|tsx|sh|json|js|mjs|cjs|py"; +// unambiguous. Every match is validated: it must resolve to an existing +// *regular file* whose real (symlink-resolved) path stays inside the +// plugin's own root — rejects `..` traversal, directories masquerading as +// files, and symlinks that escape the bundle. +const REF_EXT = ["md", "ts", "tsx", "sh", "json", "js", "mjs", "cjs", "py", "png", "jpg", "jpeg", "gif", "svg", "webp"]; const ALL_SUBDIRS = ["workflows", "references", "tools", "scripts", "cli", "assets", "hooks"]; -const REF_PATTERN = new RegExp( - `(\\$\\{CLAUDE_PLUGIN_ROOT\\}/)?((?:skills/[a-z0-9-]+/)?(?:${ALL_SUBDIRS.join("|")})/[A-Za-z0-9._/-]+\\.(?:${REF_EXT}))`, - "g", -); + +/** True if the real (symlink-resolved) path of `resolved` stays inside the real path of `base`. */ +function realpathContained(base, resolved) { + const realBase = fs.realpathSync(base); + const realResolved = fs.realpathSync(resolved); + const rel = path.relative(realBase, realResolved); + return !rel.startsWith("..") && !path.isAbsolute(rel); +} + +/** + * Filesystem-facing half of reference validation, applied only after + * `resolveBundleReference` has already confirmed `resolved` is lexically + * inside `base`. Requires an existing *regular file* whose real path (after + * resolving symlinks) also stays inside `base` — a directory or a symlink + * that escapes the bundle is rejected even though the lexical path was fine. + */ +export function checkReferenceOnDisk(base, resolved) { + let stat; + try { + stat = fs.lstatSync(resolved); + } catch { + return { reason: "missing" }; + } + if (stat.isSymbolicLink() && !realpathContained(base, resolved)) { + return { reason: "symlink-escape" }; + } + if (!fs.statSync(resolved).isFile()) { + return { reason: "not-a-file" }; + } + return { reason: "ok" }; +} + +const ON_DISK_MESSAGES = { + missing: (ref) => `SKILL.md references missing file: ${ref}`, + "symlink-escape": (ref) => `SKILL.md reference resolves outside the plugin bundle via symlink: ${ref}`, + "not-a-file": (ref) => `SKILL.md reference is not a regular file: ${ref}`, +}; function checkSkillReferences(plugin) { const file = skillPath(plugin); if (!fs.existsSync(file)) return; // already reported by checkSkillFrontmatter const text = fs.readFileSync(file, "utf8"); - const seen = new Set(); - for (const [, prefix, pathPart] of text.matchAll(REF_PATTERN)) { - const prefixed = Boolean(prefix); - // biome-ignore lint/suspicious/noTemplateCurlyInString: literal "${CLAUDE_PLUGIN_ROOT}" placeholder text, not a JS template - const ref = (prefixed ? "${CLAUDE_PLUGIN_ROOT}/" : "") + pathPart; - if (seen.has(ref)) continue; - seen.add(ref); - + const refs = extractBundleReferences(text, { subdirs: ALL_SUBDIRS, extensions: REF_EXT }); + for (const { prefixed, pathPart, ref } of refs) { // `${CLAUDE_PLUGIN_ROOT}/...` is plugin-root-relative; a bare reference // is relative to the skill directory (skills//). const base = prefixed ? path.join(pluginsDir, plugin) : path.join(pluginsDir, plugin, "skills", plugin); - const result = resolveBundleReference(base, pathPart); - if (!result.ok) { - fail( - plugin, - result.reason === "traversal" - ? `SKILL.md reference escapes the plugin bundle: ${ref}` - : `SKILL.md references missing file: ${ref}`, - ); + const lexical = resolveBundleReference(base, pathPart); + if (!lexical.ok) { + fail(plugin, `SKILL.md reference escapes the plugin bundle: ${ref}`); + continue; } + const { reason } = checkReferenceOnDisk(base, lexical.resolved); + if (reason !== "ok") fail(plugin, ON_DISK_MESSAGES[reason](ref)); } } @@ -222,26 +247,32 @@ function checkJunk() { } // --- Run ----------------------------------------------------------------- -const plugins = listPlugins(); -const manifestDescriptions = new Map(); -for (const plugin of plugins) { - const description = checkPluginManifest(plugin); - if (description) manifestDescriptions.set(plugin, description); - checkReadme(plugin); - checkSkillFrontmatter(plugin); - checkSkillReferences(plugin); -} -checkMarketplace(plugins, manifestDescriptions); -checkCatalog(plugins); -checkJunk(); +// Guarded so a test can `import { checkReferenceOnDisk } from "./validate-plugin-ports.mjs"` +// without triggering a full validation run against the real repository. +function main() { + const plugins = listPlugins(); + const manifestDescriptions = new Map(); + for (const plugin of plugins) { + const description = checkPluginManifest(plugin); + if (description) manifestDescriptions.set(plugin, description); + checkReadme(plugin); + checkSkillFrontmatter(plugin); + checkSkillReferences(plugin); + } + checkMarketplace(plugins, manifestDescriptions); + checkCatalog(plugins); + checkJunk(); -if (failures.length > 0) { - console.error(`\n✗ plugin-port validation failed (${failures.length} issue(s)):\n`); - for (const { plugin, message } of failures) { - console.error(` [${plugin}] ${message}`); + if (failures.length > 0) { + console.error(`\n✗ plugin-port validation failed (${failures.length} issue(s)):\n`); + for (const { plugin, message } of failures) { + console.error(` [${plugin}] ${message}`); + } + console.error(""); + process.exit(1); } - console.error(""); - process.exit(1); + + console.log(`✓ plugin-port validation passed (${plugins.length} plugins)`); } -console.log(`✓ plugin-port validation passed (${plugins.length} plugins)`); +if (import.meta.url === `file://${process.argv[1]}`) main(); From 9f783d2536f85a86fffef6a51b827e4b4c2267de Mon Sep 17 00:00:00 2001 From: Ossie Irondi Date: Sat, 18 Jul 2026 18:22:50 -0500 Subject: [PATCH 05/11] fix(validators): reject near-miss frontmatter closing delimiters The closing-delimiter search used indexOf("\n---"), which matches any line merely *starting* with three dashes: ---not-a-delimiter, ----, and --- trailing junk all silently closed the block early (or passed as a 'valid' close) instead of being rejected. Replace with a regex requiring an exact standalone --- line (only trailing spaces/tabs before the newline or end of string). Added the three near-miss cases as negative fixtures. Verified via stash: they fail against the pre-fix code with an actual assertion failure ('Missing expected exception'), not just a load error. --- scripts/lib/frontmatter-test.mjs | 12 ++++++++++++ scripts/lib/frontmatter.mjs | 13 ++++++++++--- 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/scripts/lib/frontmatter-test.mjs b/scripts/lib/frontmatter-test.mjs index 8c1b7da..459306d 100644 --- a/scripts/lib/frontmatter-test.mjs +++ b/scripts/lib/frontmatter-test.mjs @@ -52,6 +52,18 @@ const MALFORMED_BLOCKS = [ { label: "missing frontmatter block entirely", content: "# just a heading\n\nno frontmatter here\n" }, { label: "unterminated frontmatter (no closing ---)", content: "---\nname: fixture\ndescription: hi\n" }, { label: "frontmatter is a sequence, not a mapping", content: "---\n- one\n- two\n---\n" }, + // Near-miss closing delimiters: a naive `indexOf("\n---")` search treats + // any line merely *starting* with "---" as the closer. None of these are + // an exact "---" line, so none may terminate the block. + { + label: "near-miss closer: trailing text with no separator", + content: "---\nname: fixture\ndescription: hi\n---not-a-delimiter\n", + }, + { label: "near-miss closer: extra dash", content: "---\nname: fixture\ndescription: hi\n----\n" }, + { + label: "near-miss closer: trailing junk after a space", + content: "---\nname: fixture\ndescription: hi\n--- trailing junk\n", + }, ]; for (const { label, content } of MALFORMED_BLOCKS) { diff --git a/scripts/lib/frontmatter.mjs b/scripts/lib/frontmatter.mjs index 952675c..99fec0c 100644 --- a/scripts/lib/frontmatter.mjs +++ b/scripts/lib/frontmatter.mjs @@ -5,16 +5,23 @@ import { parse as parseYaml } from "yaml"; * Throws when the block is absent, unterminated, not valid YAML, or does * not parse to a mapping (e.g. a bare scalar or sequence document). */ +// The closing delimiter must be an exact "---" line (optionally followed by +// trailing spaces/tabs before the newline or end of string) — not merely a +// line that starts with "---". Without this, "---not-a-delimiter", "----", +// and "--- trailing junk" all satisfy a naive `indexOf("\n---")` search and +// silently close the block early or swallow content past the real closer. +const CLOSING_DELIMITER = /^---\n([\s\S]*?)\n---[ \t]*(?:\n|$)/; + export function parseFrontmatter(content, filePath) { const normalized = content.replace(/\r\n/g, "\n"); if (!normalized.startsWith("---\n")) { throw new Error(`${filePath}: missing YAML frontmatter block`); } - const end = normalized.indexOf("\n---", 4); - if (end === -1) { + const match = CLOSING_DELIMITER.exec(normalized); + if (!match) { throw new Error(`${filePath}: unterminated YAML frontmatter block`); } - const raw = normalized.slice(4, end); + const raw = match[1]; let data; try { data = parseYaml(raw); From fb5e4c7198e4a5be17284b0c2f3664e66370eec0 Mon Sep 17 00:00:00 2001 From: Ossie Irondi Date: Sat, 18 Jul 2026 18:23:03 -0500 Subject: [PATCH 06/11] fix(awesome-readme): remove broken bash gen.sh commands, delegate explicitly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The prior round rewrote SKILL.md's bash scripts/gen.sh to bash gen.sh to avoid a validator false positive, but that command never worked either: no gen.sh exists at awesome-readme's own cwd (it belongs to the delegated gpt-image-2 skill). The workflows still said bash scripts/gen.sh, so SKILL.md and the workflows now disagreed on top of both being wrong. Remove the literal bash command blocks entirely in SKILL.md, Create.md, and Improve.md and replace them with explicit parameter-list delegation instructions to the gpt-image-2 skill, matching what the docs already say to do ("invoke the gpt-image-2 skill directly"). This is a content correctness fix, not a rewrite to dodge the reference matcher — with the matcher already fixed to cover scripts/*.sh, a bare gen.sh path would have been caught as either a false positive again or, if scoped to scripts/, a real missing-file failure either way. --- .../skills/awesome-readme/SKILL.md | 20 ++++--------- .../skills/awesome-readme/workflows/Create.md | 29 ++++--------------- .../awesome-readme/workflows/Improve.md | 23 ++++----------- 3 files changed, 17 insertions(+), 55 deletions(-) diff --git a/claude-code/plugins/awesome-readme/skills/awesome-readme/SKILL.md b/claude-code/plugins/awesome-readme/skills/awesome-readme/SKILL.md index e971b83..c939fc9 100644 --- a/claude-code/plugins/awesome-readme/skills/awesome-readme/SKILL.md +++ b/claude-code/plugins/awesome-readme/skills/awesome-readme/SKILL.md @@ -57,24 +57,16 @@ If neither (1) nor (2) is set up and the user picks "Yes, generate via gpt-image ### How to invoke -When a workflow needs an image, invoke the **gpt-image-2** skill directly: +When a workflow needs an image, invoke the installed **gpt-image-2** skill directly (do not try to locate or run its script yourself; the skill loader resolves that). Pass: -```bash -bash gen.sh \ - --prompt "" \ - --out docs/.png -``` +- `--prompt ""` +- `--out docs/.png` -For reference-image conditioning (remix an existing logo, restyle a screenshot, etc.): +For reference-image conditioning (remix an existing logo, restyle a screenshot, etc.), also pass: -```bash -bash gen.sh \ - --prompt "" \ - --ref /absolute/path/to/reference.png \ - --out docs/.png -``` +- `--ref /absolute/path/to/reference.png` -The script lives inside the gpt-image-2 skill's directory; the standard skill loader resolves it. Pass the user's prompt through raw — only polish wording when the user explicitly asks for it. After the image lands, display/attach it in the response; never stop at "saved to docs/x.png". +Pass the user's prompt through raw. Only polish wording when the user explicitly asks for it. After the image lands, display/attach it in the response; never stop at "saved to docs/x.png". ### Prompt patterns for README elements diff --git a/claude-code/plugins/awesome-readme/skills/awesome-readme/workflows/Create.md b/claude-code/plugins/awesome-readme/skills/awesome-readme/workflows/Create.md index c5aaf46..c06869a 100755 --- a/claude-code/plugins/awesome-readme/skills/awesome-readme/workflows/Create.md +++ b/claude-code/plugins/awesome-readme/skills/awesome-readme/workflows/Create.md @@ -99,34 +99,15 @@ Follow this 10-section order — story before installation: If the user said yes in Step 2, invoke the **gpt-image-2** skill — once per visual. Pass the user's intent through raw; only polish wording when the user explicitly asked for "fancy" or "make it look great". -**Pattern:** - -```bash -bash scripts/gen.sh \ - --prompt "" \ - --out docs/.png -``` +**Pattern:** invoke the installed gpt-image-2 skill (do not try to locate or run its script yourself) with `--prompt ""` and `--out docs/.png`. **Example invocations for a typical Story-mode README:** -```bash -# Hero banner with embedded tagline -bash scripts/gen.sh \ - --prompt " README hero banner. . Embedded tagline reads: ''." \ - --out docs/hero.png - -# Architecture diagram -bash scripts/gen.sh \ - --prompt "Architecture diagram for . connects to via . Excalidraw / hand-drawn style. Clear labels on each node." \ - --out docs/architecture.png - -# Workflow illustration -bash scripts/gen.sh \ - --prompt "Workflow illustration: step 1 , step 2 , step 3 . Numbered left-to-right flow with annotated arrows." \ - --out docs/workflow.png -``` +- Hero banner with embedded tagline: `--prompt " README hero banner. . Embedded tagline reads: ''." --out docs/hero.png` +- Architecture diagram: `--prompt "Architecture diagram for . connects to via . Excalidraw / hand-drawn style. Clear labels on each node." --out docs/architecture.png` +- Workflow illustration: `--prompt "Workflow illustration: step 1 , step 2 , step 3 . Numbered left-to-right flow with annotated arrows." --out docs/workflow.png` -For reference-conditioned remixes (e.g., "restyle this existing logo as ukiyo-e"), add `--ref /absolute/path/to/reference.png`. +For reference-conditioned remixes (e.g., "restyle this existing logo as ukiyo-e"), also pass `--ref /absolute/path/to/reference.png`. After each call: 1. Confirm the output file exists at the `--out` path. diff --git a/claude-code/plugins/awesome-readme/skills/awesome-readme/workflows/Improve.md b/claude-code/plugins/awesome-readme/skills/awesome-readme/workflows/Improve.md index 3f7d366..f74ebab 100755 --- a/claude-code/plugins/awesome-readme/skills/awesome-readme/workflows/Improve.md +++ b/claude-code/plugins/awesome-readme/skills/awesome-readme/workflows/Improve.md @@ -130,24 +130,13 @@ For each section: If the user said yes in Step 3, invoke the **gpt-image-2** skill — once per visual. See SKILL.md "Image Generation" and Create.md Step 5 for the full pattern. -```bash -# Architecture diagram -bash scripts/gen.sh \ - --prompt "Architecture diagram for . connects to via . Excalidraw / hand-drawn style. Clear labels on each node." \ - --out docs/architecture.png - -# Workflow illustration -bash scripts/gen.sh \ - --prompt "Workflow illustration: step 1 , step 2 , step 3 . Numbered left-to-right flow with annotated arrows." \ - --out docs/workflow.png - -# Comparison visual (if applicable) -bash scripts/gen.sh \ - --prompt "Side-by-side comparison: '' vs ''. Two columns, contrasting treatment, clear winner indicated." \ - --out docs/comparison.png -``` +Invoke the installed gpt-image-2 skill (do not try to locate or run its script yourself): + +- Architecture diagram: `--prompt "Architecture diagram for . connects to via . Excalidraw / hand-drawn style. Clear labels on each node." --out docs/architecture.png` +- Workflow illustration: `--prompt "Workflow illustration: step 1 , step 2 , step 3 . Numbered left-to-right flow with annotated arrows." --out docs/workflow.png` +- Comparison visual (if applicable): `--prompt "Side-by-side comparison: '' vs ''. Two columns, contrasting treatment, clear winner indicated." --out docs/comparison.png` -Pass the user's prompt through raw — only polish wording when they explicitly asked for it. Reference each output in the README with descriptive alt text. If `gen.sh` fails, name the failing layer in one sentence and ask the user how to proceed; do not silently fall back to a weaker model. +Pass the user's prompt through raw. Only polish wording when they explicitly asked for it. Reference each output in the README with descriptive alt text. If image generation fails, name the failing layer in one sentence and ask the user how to proceed; do not silently fall back to a weaker model. ## Step 8: Validate and Deliver From 256d4988571b3a26d8195541af1fb26b6e471b6c Mon Sep 17 00:00:00 2001 From: Ossie Irondi Date: Sat, 18 Jul 2026 18:23:16 -0500 Subject: [PATCH 07/11] fix(packaging): reorder pack-content gate, sync yaml into package-lock.json MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit npm test (which ran validate-package-contents.mjs) executed before validate:plugins in the check script. The html-docs fixture verifier that generates .pyc lives inside validate:plugins, so a PYTHONDONTWRITEBYTECODE regression would pass the pack-content assertion first and only produce the artifact afterward — a later standalone npm run pack:dry would still exit zero with the bytecode included. Move the assertion to run after validate:plugins instead. Verified by manually reverting the PYTHONDONTWRITEBYTECODE fix: the reordered check now fails in the same run with 'Forbidden generated artifacts in package', where it previously would have passed silently. Also regenerate package-lock.json via npm install --package-lock-only so the yaml direct devDependency (added when the shared frontmatter parser landed) is reflected in the root packages[""].devDependencies block, matching package.json. CI's npm ci previously succeeded only because yaml@2.8.3 was already present transitively; npm ci now runs clean against the synced lockfile with a 2-line diff and no unrelated drift. --- package-lock.json | 3 ++- package.json | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/package-lock.json b/package-lock.json index ec5e255..9fb0f87 100644 --- a/package-lock.json +++ b/package-lock.json @@ -23,7 +23,8 @@ "husky": "9.1.7", "lint-staged": "16.4.0", "typebox": "latest", - "typescript": "latest" + "typescript": "latest", + "yaml": "^2.8.2" }, "engines": { "node": ">=20" diff --git a/package.json b/package.json index 796ae11..6a2abf9 100644 --- a/package.json +++ b/package.json @@ -42,7 +42,7 @@ ] }, "scripts": { - "check": "npm run lint && npm run typecheck && npm test && npm run validate:plugins && npm run validate:skills && node scripts/list-resources.mjs", + "check": "npm run lint && npm run typecheck && npm test && npm run validate:plugins && node scripts/validate-package-contents.mjs && npm run validate:skills && node scripts/list-resources.mjs", "list": "node scripts/list-resources.mjs", "validate:plugins": "node scripts/validate-plugin-ports.mjs && node scripts/validate-codex-plugins.mjs", "install:bash-guard:global": "node scripts/install-bash-guard-global.mjs", @@ -53,7 +53,7 @@ "lint": "biome lint .", "lint:fix": "biome check --write .", "prepare": "husky", - "test": "node extensions/autopilot/v2-smoke-test.mjs && node extensions/question/question-smoke-test.mjs && node extensions/conditional-hooks/smoke-test.mjs && node scripts/lib/frontmatter-test.mjs && node scripts/lib/bundle-refs-test.mjs && python3 skills/diataxis-docs-site/tests/test_create_site.py && node scripts/validate-package-contents.mjs", + "test": "node extensions/autopilot/v2-smoke-test.mjs && node extensions/question/question-smoke-test.mjs && node extensions/conditional-hooks/smoke-test.mjs && node scripts/lib/frontmatter-test.mjs && node scripts/lib/bundle-refs-test.mjs && python3 skills/diataxis-docs-site/tests/test_create_site.py", "typecheck": "tsc --noEmit", "validate:skills": "node scripts/validate-agent-skills.mjs" }, From e42fc934f0143c98855a8b73e3749a9b83ffc4b2 Mon Sep 17 00:00:00 2001 From: Ossie Irondi Date: Sat, 18 Jul 2026 18:46:36 -0500 Subject: [PATCH 08/11] fix(validators): realpath every containment candidate, URL-safe CLI guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two P1 gaps from the SHA-pinned re-review of the prior remediation: - checkReferenceOnDisk only realpath-checked when the *final* path component was itself a symlink (lstatSync().isSymbolicLink()). A symlinked *parent* directory resolves away silently during normal path resolution, so a plain regular file sitting behind one (scripts/linked-dir -> , scripts/linked-dir/payload.md) was seen as an ordinary contained file and returned {reason:"ok"}. realpathSync now runs unconditionally on every candidate, and a dangling or cyclic symlink (ENOENT/ELOOP) maps to a controlled "missing" result instead of an uncaught throw. - The direct-execution guard compared `import.meta.url === \`file://${process.argv[1]}\`` as raw strings. import.meta.url percent-encodes characters like spaces; the interpolated argv string does not, so a checkout path containing a space made the comparison silently false and main() never ran — `node validate-plugin-ports.mjs` would exit 0 having done nothing. Rebuilt the comparison as native filesystem paths instead of URL strings (fileURLToPath(import.meta.url) vs realpathSync(argv[1])), which also fixes a second issue the first attempt at this introduced: macOS resolves /tmp to /private/tmp, and import.meta.url reflects that resolved path while path.resolve() alone does not follow the symlink. Added symlinked-parent-directory-escape, dangling-symlink, and cyclic-symlink fixtures to bundle-refs-test.mjs, and a new cli-entrypoint-test.mjs that copies the real script into a temp directory whose path contains a space and runs it as a subprocess, asserting main() actually executed (non-zero exit, 'plugins directory not found' on stderr) rather than silently exiting 0. Verified via stash: the symlinked-parent case returns {reason:"ok"} and the space-path smoke test exits 0 with no output against the pre-fix code. --- package.json | 2 +- scripts/cli-entrypoint-test.mjs | 48 +++++++++++++++++++ scripts/lib/bundle-refs-test.mjs | 76 +++++++++++++++++++++++++++++++ scripts/validate-plugin-ports.mjs | 51 ++++++++++++++------- 4 files changed, 160 insertions(+), 17 deletions(-) create mode 100644 scripts/cli-entrypoint-test.mjs diff --git a/package.json b/package.json index 6a2abf9..7f4bf15 100644 --- a/package.json +++ b/package.json @@ -53,7 +53,7 @@ "lint": "biome lint .", "lint:fix": "biome check --write .", "prepare": "husky", - "test": "node extensions/autopilot/v2-smoke-test.mjs && node extensions/question/question-smoke-test.mjs && node extensions/conditional-hooks/smoke-test.mjs && node scripts/lib/frontmatter-test.mjs && node scripts/lib/bundle-refs-test.mjs && python3 skills/diataxis-docs-site/tests/test_create_site.py", + "test": "node extensions/autopilot/v2-smoke-test.mjs && node extensions/question/question-smoke-test.mjs && node extensions/conditional-hooks/smoke-test.mjs && node scripts/lib/frontmatter-test.mjs && node scripts/lib/bundle-refs-test.mjs && node scripts/cli-entrypoint-test.mjs && python3 skills/diataxis-docs-site/tests/test_create_site.py", "typecheck": "tsc --noEmit", "validate:skills": "node scripts/validate-agent-skills.mjs" }, diff --git a/scripts/cli-entrypoint-test.mjs b/scripts/cli-entrypoint-test.mjs new file mode 100644 index 0000000..61606b3 --- /dev/null +++ b/scripts/cli-entrypoint-test.mjs @@ -0,0 +1,48 @@ +import assert from "node:assert/strict"; +import { spawnSync } from "node:child_process"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; + +// Regression for a URL-unsafe direct-execution guard: a naive +// `import.meta.url === \`file://${process.argv[1]}\`` comparison fails +// silently for any checkout path containing a space (import.meta.url +// percent-encodes it, the raw argv string does not), so `node +// validate-plugin-ports.mjs` would exit 0 having never called main(). +// +// This copies the real script and its lib dependencies into a temp +// directory whose path contains a space, symlinks node_modules alongside +// it so "yaml" still resolves, and runs it as a real subprocess. There is +// no claude-code/plugins directory in the copy, so a correctly-invoked +// main() must fail loudly with "plugins directory not found" — proof that +// main() actually ran, not just that the subprocess exited. + +const scriptsDir = path.resolve(path.dirname(fileURLToPath(import.meta.url))); +const repoRoot = path.resolve(scriptsDir, ".."); + +const tmp = fs.mkdtempSync(path.join(os.tmpdir(), "cli-entrypoint-")); +const spacedDir = path.join(tmp, "path with a space"); +fs.mkdirSync(path.join(spacedDir, "scripts", "lib"), { recursive: true }); + +for (const file of ["validate-plugin-ports.mjs"]) { + fs.copyFileSync(path.join(scriptsDir, file), path.join(spacedDir, "scripts", file)); +} +for (const file of ["bundle-refs.mjs", "frontmatter.mjs"]) { + fs.copyFileSync(path.join(scriptsDir, "lib", file), path.join(spacedDir, "scripts", "lib", file)); +} +fs.symlinkSync(path.join(repoRoot, "node_modules"), path.join(spacedDir, "node_modules")); + +const targetScript = path.join(spacedDir, "scripts", "validate-plugin-ports.mjs"); +const result = spawnSync(process.execPath, [targetScript], { encoding: "utf8" }); + +fs.rmSync(tmp, { recursive: true, force: true }); + +assert.notEqual(result.status, 0, "main() must run and fail against the fixture's absent claude-code/plugins dir"); +assert.match( + result.stderr, + /plugins directory not found/, + "stderr must show main() actually executed listPlugins(), proving the space-containing path was recognized as direct execution", +); + +console.log("CLI entrypoint direct-execution fixtures passed"); diff --git a/scripts/lib/bundle-refs-test.mjs b/scripts/lib/bundle-refs-test.mjs index 6c4f85a..6921f4d 100644 --- a/scripts/lib/bundle-refs-test.mjs +++ b/scripts/lib/bundle-refs-test.mjs @@ -45,6 +45,21 @@ function extract(text) { assert.equal(refs.length, 0, "mid-identifier text must not match"); } +{ + // Terminal boundary: a coincidental prefix of a longer filename must not + // be truncated into a match for a shorter, unrelated real asset name. + const refs = extract("See `assets/logo.png.bak` for the backup."); + assert.equal(refs.length, 0, "assets/logo.png.bak must not extract as assets/logo.png"); +} + +{ + // A safe leading "./" (same-directory prefix) is a common, harmless + // convention and must be captured, not silently unmatched. + const refs = extract("Run `./scripts/gen.sh` now."); + assert.equal(refs.length, 1, "./scripts/gen.sh must be extracted"); + assert.deepEqual(refs[0], { prefixed: false, pathPart: "./scripts/gen.sh", ref: "./scripts/gen.sh" }); +} + { // The exact adversarial case from the review: a ${CLAUDE_PLUGIN_ROOT}/../ // reference must keep its traversal prefix attached to the captured @@ -87,6 +102,10 @@ for (const pathPart of ["scripts/../../secret/outside.md", "../../secret/outside const tmp = fs.mkdtempSync(path.join(os.tmpdir(), "bundle-refs-")); const base = path.join(tmp, "skills", "fixture-plugin"); fs.mkdirSync(path.join(base, "scripts"), { recursive: true }); +fs.mkdirSync(path.join(base, "assets"), { recursive: true }); +// A real file at the shorter, truncated path a terminal-boundary bug would +// coincidentally match against (assets/logo.png.bak -> assets/logo.png). +fs.writeFileSync(path.join(base, "assets", "logo.png"), "not a real png, just fixture bytes\n"); fs.mkdirSync(path.join(base, "tools"), { recursive: true }); fs.writeFileSync(path.join(base, "scripts", "gen.sh"), "#!/bin/sh\necho ok\n"); // A directory whose name matches the reference vocabulary + extension @@ -102,6 +121,19 @@ fs.mkdirSync(path.join(tmp, "secret"), { recursive: true }); fs.writeFileSync(path.join(tmp, "secret", "real-target.md"), "should never be reachable\n"); fs.symlinkSync(path.join(tmp, "secret", "real-target.md"), path.join(base, "scripts", "escape-link.md")); fs.symlinkSync(path.join(base, "scripts", "gen.sh"), path.join(base, "scripts", "safe-link.sh")); +// A symlinked *parent directory* escaping the bundle, with an ordinary +// regular file sitting behind it. lstat on the final component alone can't +// see this: path resolution follows the intermediate symlink transparently, +// so the final component looks like a plain contained file unless the whole +// resolved path is realpath-checked. +fs.mkdirSync(path.join(tmp, "secret", "linked-target"), { recursive: true }); +fs.writeFileSync(path.join(tmp, "secret", "linked-target", "payload.md"), "should never be reachable\n"); +fs.symlinkSync(path.join(tmp, "secret", "linked-target"), path.join(base, "scripts", "linked-dir")); +// A dangling symlink (target does not exist). +fs.symlinkSync(path.join(base, "scripts", "does-not-exist.sh"), path.join(base, "scripts", "dangling.sh")); +// A cyclic symlink pair. +fs.symlinkSync(path.join(base, "scripts", "cycle-b"), path.join(base, "scripts", "cycle-a")); +fs.symlinkSync(path.join(base, "scripts", "cycle-a"), path.join(base, "scripts", "cycle-b")); { // missing: lexically contained, no file on disk. @@ -134,6 +166,32 @@ fs.symlinkSync(path.join(base, "scripts", "gen.sh"), path.join(base, "scripts", assert.equal(checkReferenceOnDisk(base, resolved).reason, "ok"); } +{ + // The exact adversarial case from the review: an escaping *parent* + // directory symlink, reached through a final component that is itself a + // plain regular file (not a symlink). + const resolved = path.join(base, "scripts", "linked-dir", "payload.md"); + assert.equal( + checkReferenceOnDisk(base, resolved).reason, + "symlink-escape", + "a file behind a symlinked parent directory must be rejected as escaping", + ); +} + +{ + // A dangling symlink must fail as a controlled validation reason, not + // throw an uncaught ENOENT out of realpathSync. + const resolved = path.join(base, "scripts", "dangling.sh"); + assert.equal(checkReferenceOnDisk(base, resolved).reason, "missing"); +} + +{ + // A cyclic symlink pair must fail as a controlled validation reason, not + // throw an uncaught ELOOP out of realpathSync. + const resolved = path.join(base, "scripts", "cycle-a"); + assert.equal(checkReferenceOnDisk(base, resolved).reason, "missing"); +} + // --- End-to-end: extractor -> resolver -> on-disk check, chained exactly // the way validate-plugin-ports.mjs's checkSkillReferences runs them ------- @@ -158,6 +216,24 @@ assert.equal( "production pipeline must reject the traversal reference even though tools/outside.ts genuinely exists in the bundle", ); +assert.equal( + validateReference("Run `./scripts/gen.sh` now.", base), + "ok", + "production pipeline must accept a safe leading ./ prefix", +); + +{ + // The exact terminal-boundary adversarial case, end to end: even with a + // real file sitting at the truncated path (assets/logo.png), the + // coincidental-prefix text must never extract a match to validate at all. + const refs = extract("See `assets/logo.png.bak` for the backup."); + assert.equal( + refs.length, + 0, + "production pipeline must never match assets/logo.png.bak against the real assets/logo.png file", + ); +} + fs.rmSync(tmp, { recursive: true, force: true }); console.log("bundle reference resolution fixtures passed"); diff --git a/scripts/validate-plugin-ports.mjs b/scripts/validate-plugin-ports.mjs index 320c34a..3466fef 100644 --- a/scripts/validate-plugin-ports.mjs +++ b/scripts/validate-plugin-ports.mjs @@ -115,32 +115,32 @@ function checkSkillFrontmatter(plugin) { const REF_EXT = ["md", "ts", "tsx", "sh", "json", "js", "mjs", "cjs", "py", "png", "jpg", "jpeg", "gif", "svg", "webp"]; const ALL_SUBDIRS = ["workflows", "references", "tools", "scripts", "cli", "assets", "hooks"]; -/** True if the real (symlink-resolved) path of `resolved` stays inside the real path of `base`. */ -function realpathContained(base, resolved) { - const realBase = fs.realpathSync(base); - const realResolved = fs.realpathSync(resolved); - const rel = path.relative(realBase, realResolved); - return !rel.startsWith("..") && !path.isAbsolute(rel); -} - /** * Filesystem-facing half of reference validation, applied only after * `resolveBundleReference` has already confirmed `resolved` is lexically - * inside `base`. Requires an existing *regular file* whose real path (after - * resolving symlinks) also stays inside `base` — a directory or a symlink - * that escapes the bundle is rejected even though the lexical path was fine. + * inside `base`. `realpathSync` is required unconditionally (not just when + * the final path component is itself a symlink) because a symlinked + * *parent* directory resolves away silently otherwise: `lstatSync` on the + * final component follows all intermediate symlinks as a normal part of + * path resolution, so a plain file sitting behind a symlinked directory + * looks like an ordinary contained file unless the whole path is + * realpath-resolved and re-checked for containment. A dangling or cyclic + * symlink makes `realpathSync` throw (ENOENT / ELOOP); that is a controlled + * "missing" result here, not an uncaught crash. */ export function checkReferenceOnDisk(base, resolved) { - let stat; + let real; try { - stat = fs.lstatSync(resolved); + real = fs.realpathSync(resolved); } catch { return { reason: "missing" }; } - if (stat.isSymbolicLink() && !realpathContained(base, resolved)) { + const realBase = fs.realpathSync(base); + const rel = path.relative(realBase, real); + if (rel.startsWith("..") || path.isAbsolute(rel)) { return { reason: "symlink-escape" }; } - if (!fs.statSync(resolved).isFile()) { + if (!fs.statSync(real).isFile()) { return { reason: "not-a-file" }; } return { reason: "ok" }; @@ -275,4 +275,23 @@ function main() { console.log(`✓ plugin-port validation passed (${plugins.length} plugins)`); } -if (import.meta.url === `file://${process.argv[1]}`) main(); +// A plain `import.meta.url === \`file://${process.argv[1]}\`` string +// comparison is not URL-safe: import.meta.url percent-encodes characters +// like spaces, while the raw argv path does not, so a checkout path +// containing a space silently fails the check and main() never runs. +// Comparing filesystem paths instead of URL strings sidesteps encoding +// entirely: fileURLToPath decodes import.meta.url back to a native path +// (already realpath-resolved by Node for a directly-executed script — e.g. +// through macOS's /tmp -> /private/tmp symlink), and realpathSync resolves +// the same way on the argv side so both sides compare on equal footing +// regardless of spaces, separators, or symlinked ancestor directories. +function isDirectExecution() { + if (!process.argv[1]) return false; + try { + return fileURLToPath(import.meta.url) === fs.realpathSync(process.argv[1]); + } catch { + return false; + } +} + +if (isDirectExecution()) main(); From 23504038c2dcc967a19e9ad77f30ace98fb4b6a7 Mon Sep 17 00:00:00 2001 From: Ossie Irondi Date: Sat, 18 Jul 2026 18:47:09 -0500 Subject: [PATCH 09/11] fix(validators): require a terminal boundary, capture safe leading ./ Two more production-shape gaps in extractBundleReferences from the SHA-pinned re-review (also flagged independently by CodeRabbit): - The regex had a leading token boundary but no trailing one, so a coincidental prefix of a longer filename truncated into a match: assets/logo.png.bak extracted assets/logo.png, because the greedy path-character class backtracks to find any valid extension match rather than requiring the match to reach the actual end of the token. A same-prefixed real file could then validate text that was never actually a supported asset reference. Added a trailing negative lookahead so nothing word/dot/hyphen-like may immediately follow the matched extension. - ./scripts/gen.sh extracted nothing: the leading-boundary lookbehind correctly blocks resynchronization after a slash, but that also meant a safe, common ./ same-directory prefix was never recognized as part of a reference at all, a false negative on legitimate content. Widened the existing leading-segment group (already used to preserve traversal prefixes) so it captures ./ the same way it already captures ../. Extended bundle-refs-test.mjs (already touched in the prior commit) with the exact assets/logo.png.bak and ./scripts/gen.sh cases from the review, plus an end-to-end fixture proving a real assets/logo.png file in the bundle still can't be coincidentally matched by the .bak text. Verified via stash: assets/logo.png.bak produces 1 match (not 0) against the pre-fix regex. --- scripts/lib/bundle-refs.mjs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/scripts/lib/bundle-refs.mjs b/scripts/lib/bundle-refs.mjs index 38a22ff..1dd53d2 100644 --- a/scripts/lib/bundle-refs.mjs +++ b/scripts/lib/bundle-refs.mjs @@ -6,13 +6,17 @@ import path from "node:path"; * A reference is unambiguous only when it starts at a token boundary — not * preceded by a word/path character, so a match can never "resync" mid-path * and swallow part of a larger expression (e.g. `${CLAUDE_PLUGIN_ROOT}/../tools/x.ts` - * must never be reinterpreted as the bare reference `tools/x.ts`). Leading - * `../` segments are captured as part of the reference itself, not silently - * dropped, so traversal is detected downstream instead of disappearing. + * must never be reinterpreted as the bare reference `tools/x.ts`) — and ends + * at one, so a coincidental prefix of a longer filename (e.g. + * `assets/logo.png.bak`) can't be truncated into a match for a real asset + * (`assets/logo.png`) that happens to exist. Leading `./` and `../` + * segments are captured as part of the reference itself, not silently + * dropped or left unmatched, so both a safe same-directory prefix and real + * traversal are visible downstream instead of disappearing. */ export function extractBundleReferences(text, { subdirs, extensions }) { const pattern = new RegExp( - `(? Date: Sat, 18 Jul 2026 18:47:21 -0500 Subject: [PATCH 10/11] docs(awesome-readme): add --ref example to Improve.md (CodeRabbit) Create.md and SKILL.md both document the optional --ref flag for reference-image conditioning; Improve.md, which handles existing READMEs where a user might want to restyle an existing logo or diagram, did not. Add the same one-line guidance for consistency. --- .../awesome-readme/skills/awesome-readme/workflows/Improve.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/claude-code/plugins/awesome-readme/skills/awesome-readme/workflows/Improve.md b/claude-code/plugins/awesome-readme/skills/awesome-readme/workflows/Improve.md index f74ebab..428b20d 100755 --- a/claude-code/plugins/awesome-readme/skills/awesome-readme/workflows/Improve.md +++ b/claude-code/plugins/awesome-readme/skills/awesome-readme/workflows/Improve.md @@ -136,6 +136,8 @@ Invoke the installed gpt-image-2 skill (do not try to locate or run its script y - Workflow illustration: `--prompt "Workflow illustration: step 1 , step 2 , step 3 . Numbered left-to-right flow with annotated arrows." --out docs/workflow.png` - Comparison visual (if applicable): `--prompt "Side-by-side comparison: '' vs ''. Two columns, contrasting treatment, clear winner indicated." --out docs/comparison.png` +For reference-conditioned remixes (e.g., restyling an existing logo or diagram), also pass `--ref /absolute/path/to/reference.png`. + Pass the user's prompt through raw. Only polish wording when they explicitly asked for it. Reference each output in the README with descriptive alt text. If image generation fails, name the failing layer in one sentence and ask the user how to proceed; do not silently fall back to a weaker model. ## Step 8: Validate and Deliver From a9215910dbecf37ac75db3c9fddd8c09e1bf67c9 Mon Sep 17 00:00:00 2001 From: Ossie Irondi Date: Sat, 18 Jul 2026 18:58:29 -0500 Subject: [PATCH 11/11] fix(validators): include / in the terminal reference boundary The terminal negative lookahead added for the .bak truncation bug was (?![\w.-]), omitting / even though / is itself part of the accepted path-character class. That left a second truncation path open: assets/logo.png/extra still extracted assets/logo.png, because a following slash satisfied the lookahead. If assets/logo.png exists, the validator approves that truncated prefix and never sees the actual longer token. Add / to the lookahead: (?![\w./-]). This is exactly what CodeRabbit's now-resolved thread proposed; the prior commit implemented the word/dot/hyphen half but dropped the slash. Added unit fixtures for assets/logo.pngx (no separator at all) and assets/logo.png/child.bin (the slash-tail case), plus an end-to-end production-path fixture proving the pipeline never extracts a match for the nested-path text even with a real assets/logo.png file present in the bundle. Verified via stash: the new slash-tail fixture extracts 1 match (not 0) against the pre-fix regex. --- scripts/lib/bundle-refs-test.mjs | 32 ++++++++++++++++++++++++++++++++ scripts/lib/bundle-refs.mjs | 18 +++++++++++------- 2 files changed, 43 insertions(+), 7 deletions(-) diff --git a/scripts/lib/bundle-refs-test.mjs b/scripts/lib/bundle-refs-test.mjs index 6921f4d..6af3bf9 100644 --- a/scripts/lib/bundle-refs-test.mjs +++ b/scripts/lib/bundle-refs-test.mjs @@ -52,6 +52,23 @@ function extract(text) { assert.equal(refs.length, 0, "assets/logo.png.bak must not extract as assets/logo.png"); } +{ + // Same class of bug, different character: an extra word character + // directly after the extension (no separator at all) must also not + // truncate into a match. + const refs = extract("See `assets/logo.pngx` for the logo."); + assert.equal(refs.length, 0, "assets/logo.pngx must not extract as assets/logo.png"); +} + +{ + // The "/" character is itself part of the accepted path alphabet, so a + // terminal boundary that only rejects word/dot/hyphen characters still + // lets a nested path truncate into a match for its own leading segment: + // assets/logo.png/child.bin must not extract as assets/logo.png. + const refs = extract("See `assets/logo.png/child.bin` for the asset."); + assert.equal(refs.length, 0, "assets/logo.png/child.bin must not extract as assets/logo.png"); +} + { // A safe leading "./" (same-directory prefix) is a common, harmless // convention and must be captured, not silently unmatched. @@ -234,6 +251,21 @@ assert.equal( ); } +{ + // The exact production-path shape from the final re-review: a nested + // path whose leading segment coincides with the real assets/logo.png + // file. "/" is part of the accepted path alphabet, so a boundary that + // only rejects word/dot/hyphen characters still truncates this into a + // match for the shorter, real file — end to end, this must never + // extract a match to validate at all. + const refs = extract("See `assets/logo.png/child.bin` for the nested asset."); + assert.equal( + refs.length, + 0, + "production pipeline must never match assets/logo.png/child.bin against the real assets/logo.png file", + ); +} + fs.rmSync(tmp, { recursive: true, force: true }); console.log("bundle reference resolution fixtures passed"); diff --git a/scripts/lib/bundle-refs.mjs b/scripts/lib/bundle-refs.mjs index 1dd53d2..6c62cc7 100644 --- a/scripts/lib/bundle-refs.mjs +++ b/scripts/lib/bundle-refs.mjs @@ -7,16 +7,20 @@ import path from "node:path"; * preceded by a word/path character, so a match can never "resync" mid-path * and swallow part of a larger expression (e.g. `${CLAUDE_PLUGIN_ROOT}/../tools/x.ts` * must never be reinterpreted as the bare reference `tools/x.ts`) — and ends - * at one, so a coincidental prefix of a longer filename (e.g. - * `assets/logo.png.bak`) can't be truncated into a match for a real asset - * (`assets/logo.png`) that happens to exist. Leading `./` and `../` - * segments are captured as part of the reference itself, not silently - * dropped or left unmatched, so both a safe same-directory prefix and real - * traversal are visible downstream instead of disappearing. + * at one, so a coincidental prefix of a longer path (e.g. + * `assets/logo.png.bak` or `assets/logo.png/child.bin`) can't be truncated + * into a match for a real asset (`assets/logo.png`) that happens to exist. + * The terminal boundary must reject `/` as well as word/dot/hyphen + * characters — `/` is itself part of the accepted path alphabet, so a + * shorter match immediately followed by more path is still a truncation, + * not a genuine end of token. Leading `./` and `../` segments are captured + * as part of the reference itself, not silently dropped or left unmatched, + * so both a safe same-directory prefix and real traversal are visible + * downstream instead of disappearing. */ export function extractBundleReferences(text, { subdirs, extensions }) { const pattern = new RegExp( - `(?