Repository navigation
fix(plugins): complete issue #10 port hardening - #33
Conversation
|
|
Warning Review limit reached
Next review available in: 51 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Heightened post-merge reviewReviewed the immutable squash range Findings
Evidence that passed
Narrow follow-up scope
|
…ckaging hygiene, ask-codex metadata) (#34) * fix(validators): parse YAML frontmatter properly, contain bundle references 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. * fix(packaging): stop generated Python bytecode from reaching the pack 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. * 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. * fix(validators): end-to-end reference extraction and filesystem containment 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). * 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. * fix(awesome-readme): remove broken bash gen.sh commands, delegate explicitly 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. * fix(packaging): reorder pack-content gate, sync yaml into package-lock.json 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. * fix(validators): realpath every containment candidate, URL-safe CLI guard 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 -> <outside>, 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. * 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. * 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. * 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.
1 participant
Summary
Closes #10
Verification
bun run lintbun run typecheckbun run testbun run validate:pluginsbun run validate:skillsbun run listbun pm pack --dry-run --ignore-scripts(228 files, 9.27 MB; no local artifacts)pi -ne -e . --print "Reply exactly: plugin smoke ok"gitleaks detect --no-git --source claude-code/plugins --config .gitleaks.toml --redact --verbosegitleaks git . --config .gitleaks.toml --redact --log-opts="origin/main..HEAD" --verboseMain refresh
Merged fresh
origin/mainafter PR #31 landed. Onlypackage-lock.jsonentered this branch; no source files conflicted or auto-merged.