[AI-52] ci: Track external Claude plugin pins with Renovate - #232
Open
SaintPatrck wants to merge 20 commits into
Open
[AI-52] ci: Track external Claude plugin pins with Renovate#232SaintPatrck wants to merge 20 commits into
SaintPatrck wants to merge 20 commits into
Conversation
Security audit of a third-party Claude Code plugin at a pinned commit, before it is vendored into this marketplace. Covers the plugin manifest, MCP server configuration, bundled or runtime-fetched dependencies, tool permission scope, failure-mode behavior of network-dependent checks, prompt-injection surface, secrets, and license. Runs as an isolated bitwarden-security-engineer subagent on the fable model, blocking, so it can be triggered manually or by a CI workflow that posts the report as a PR review comment. Ships a bundled gather-evidence.sh script so allowed-tools can pre-approve one script invocation instead of enumerating every git/npm/tar/hash command, keeping the skill runnable unattended. Bumps bitwarden-security-engineer 1.3.0 -> 1.4.0 (minor, additive skill).
… js-beautify - Reject any $repo_url that isn't https:// or git@ before cloning, and add -- separators on clone/checkout, closing an ext:: remote-helper RCE and option-injection path via a leading-dash argument. - Guard the openssl hash and tar extraction so a failure (including GNU tar refusing a path-traversal member) emits an explicit marker instead of aborting the script under set -e before the scratch path is printed. - Pin js-beautify to the exact version already used successfully in manual dry runs; the bare @* wildcard let npm's alias/tarball spec syntax resolve to an arbitrary package.
…ection, output path, cleanup - gather-evidence.sh: guard against missing arguments; emit NPM_VIEW_FAILED, NPM_PACK_FAILED, ATTESTATIONS_UNAVAILABLE, and MULTIPLE_NPM_PACKAGES_DETECTED markers instead of failing silently with || true; add -- before the package spec on npm view/pack so a dash-leading spec can't be read as a flag (verified: without --, npm silently mis-parses it; with --, it's rejected as an invalid package name instead). npm flags must stay before -- or npm also swallows them as positional args, including --registry. - scratch dir now uses a fixed, predictable /tmp/plugin-audit.XXXXXXXX prefix instead of a bare mktemp -d, so cleanup can be scoped to it specifically. - SKILL.md: any stderr marker must be named in the report's Not done field, not read as a clean result; default OUTPUT_FILE moves to /plugin-audits/ instead of a repo-visible path; js-beautify's grant is now tied to an explicit step (deobfuscating a minified server entry point); added an unconditional final cleanup step removing the scratch directory, matching perform-security-review's precedent in this same plugin; README gets a usage example for the skill's positional-argument invocation. Tar/symlink extraction and the ext:: remote-helper claims from the same review round were independently tested against this environment's tar and git and did not reproduce; not changed. The npm-registry attestations @-encoding claim also tested identically either way; not changed.
…ehind a script Removes the gh api --method GET allowed-tools grant: gh api is never invoked by this skill or gather-evidence.sh, only warned against as a mutation to avoid. Moves the js-beautify invocation into a new beautify.sh script, mirroring gather-evidence.sh. The version now lives in a scoped package.json/lockfile that Renovate's npm manager already tracks, instead of a string in SKILL.md's frontmatter that could only be bumped by hand. Uses npm rather than pnpm since npm is already a hard dependency of this skill (gather-evidence.sh shells out to it), while pnpm/corepack availability in the skill's actual CI execution environment (a reusable workflow in bitwarden/gh-actions) can't be verified from this repo.
Bash(rm -rf /tmp/plugin-audit.*) is a literal prefix match on the whole command string, so extra paths appended to the same invocation (e.g. rm -rf /tmp/plugin-audit.ab12cd34 ~/src) would also match and run unprompted. That matters here specifically: this skill treats the cloned repo as adversarial, and step 7 has the agent compose the rm invocation from a path it derives after running that untrusted content through several tools. Moves the deletion into cleanup.sh, mirroring the beautify.sh remedy shape. It accepts exactly one argument and refuses to delete anything that doesn't match gather-evidence.sh's exact mktemp pattern (/tmp/plugin-audit.XXXXXXXX), so an appended or malformed path is rejected by the script instead of pre-approved by the allowed-tools rule.
Verified the beautify.sh CLI usage (js-beautify <file> -o <out>) behaves identically on 2.0.3 against a sample minified file before bumping.
…caller's cwd An unspecified output path previously resolved to ./pretty.js relative to whatever directory the agent happened to be in, so the de-minified copy of a third-party bundle landed in the working repo instead of the scratch directory step 7 cleans up. Defaulting to the input file's own directory keeps it inside that scratch directory.
Deleting the clone buys nothing on an ephemeral CI runner, and locally it leaves only inert files in /tmp. That payoff doesn't justify a third script, a wildcard allowed-tools grant, and a path guard that has to hold against adversarial input. The case pattern it used matched separators anyway, so /tmp/plugin-audit.../../.. passed validation and resolved to /.
The audit tells the agent to read every file in an untrusted tree, report any secret it finds with the value, and the caller posts that report to a public PR comment. A committed symlink turns that into an exfiltration primitive with no prompt injection required: docs/notes.txt pointing at ../../../.git/config makes the agent read the runner's GITHUB_TOKEN and dutifully publish it, because every component behaved as designed. Cloning with core.symlinks=false writes each committed symlink as a plain file holding its target path, and the config persists into the repo so the detached checkout stays safe too. Tarball members get recorded and deleted, since tar refuses to write through an existing symlink but will happily create one pointing anywhere. Both inventories land in symlinks.txt, where a target outside the tree is itself a finding.
The script's own messages already say what evidence is missing, so listing the marker names here only couples SKILL.md to the script: a new marker means editing both files, and a forgotten one reads as an approved omission. NO_NPM_PACKAGE_DETECTED stays named because it calls for extra manual work rather than just a Not done entry.
SKILL.md carried a prose copy of the script's output layout, which only the script knows at runtime and which drifts the moment either side changes. The script now ends with an inventory: SCRATCH_DIR as a named key rather than a positional last line, one row per artifact that exists, and a NOT COLLECTED list of the markers it emitted. SKILL.md keeps the judgment that does not belong in a shell script, namely that a NOT COLLECTED entry goes in Not done and an escaping symlink target is a finding. Every label in the inventory is a fixed string or a count. Nothing derived from the audited repo is echoed there, because script output reads as more trustworthy to the agent than the file contents it describes. A row is printed only for a non-empty file, and a failed npm view now has its error-object output removed, so a fetch that failed cannot leave behind something a later read mistakes for registry metadata. Also moves the URL validation above mktemp and adds an EXIT trap, so a rejected URL creates no scratch directory and a mid-script failure removes the partial clone instead of orphaning adversarial content that the caller never learned the path to.
A 404 from the attestations endpoint means the package is unsigned, which
is a security-relevant answer rather than a transport error. Without -f
curl exited 0 and saved {"error":"Not found"} to attestations.json, so no
marker fired and the agent was handed a file that reads as attestation
data. The inventory would then vouch for it as collected evidence.
-f makes that a failure, the file is removed so nothing can be mistaken for
provenance, and the marker says to treat the package as unsigned unless
proven otherwise. --max-time bounds a stalled connection, which previously
could hang the audit with no ceiling.
The report is published verbatim to a public pull request comment, and the skill previously told the agent to find hardcoded secrets and describe each finding concretely, with nothing saying the value itself must stay out. A secret the audited plugin committed would be disclosed by us on discovery, and one belonging to the auditing machine would be exfiltrated by the same sentence that asked for it. States the publication boundary next to the existing untrusted-data boundary, since both govern everything the agent does rather than any one step, and restates it in the Where field where the instinct to paste a token actually lands. Location, kind, and a SHA-256 prefix keep a finding verifiable by a human without republishing the credential.
…n one npm audit exits non-zero both when it finds vulnerabilities, which is a real report, and when it fails outright, in which case it writes an error object to stdout. The exit status cannot tell those apart, so a registry or proxy failure landed that error object in audit.json, where a non-empty file read as collected evidence and no marker fired. An audit that never ran was presented as an audit that found nothing. Keys off auditReportVersion, which only a real report carries, so a vulnerabilities-found report survives while a failure removes the file and raises the marker. That also subsumes the missing-lockfile case, where audit never ran and the file was never created, which is why there is now one message for both rather than two that ask for the same action.
Adds the aikido plugin (AikidoSec/aikido-claude-plugin) as a SHA-pinned external entry at commit 1353c9d, the same commit already validated by the auditing-external-claude-plugins skill. Introduces a separate 'External plugins' README section, mirroring how Anthropic's own marketplace presents third-party entries: not copied into this repo, referenced at a fixed commit.
An external plugin entry pins an upstream commit that nothing watches, so the pin only moves when someone thinks to go look at the upstream repository. A JSONata custom manager reads the URL and SHA out of each url-sourced marketplace entry and follows the upstream branch by digest, so a stale pin arrives as a pull request against the manifest instead of sitting unnoticed.
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE This PR adds a JSONata custom manager to Code Review DetailsNo findings. |
SaintPatrck
marked this pull request as ready for review
September 4, 2026 16:54
SaintPatrck
force-pushed
the
feat/vendor-aikido-plugin
branch
from
September 4, 2026 21:33
a3dee6e to
6577cce
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🎟️ Tracking
https://bitwarden.atlassian.net/browse/AI-52
📔 Objective
An external plugin entry pins an upstream commit, and nothing watches that commit, so a pin only moves when one of us remembers to go look at the upstream repository. This adds a JSONata custom manager to our Renovate config that reads the URL and SHA out of every url-sourced marketplace entry and follows the upstream branch by digest, so a stale pin arrives as a pull request against the manifest. Because that pull request edits
.claude-plugin/marketplace.json, it trips the same path filter the external plugin audit in #230 watches, and a refresh gets audited the way the original pin was.The
packageRulesentry alongside it is what makes that actually happen. Our shared config setsminimumReleaseAge: 7 days, and a branch digest carries no release timestamp to age against, so under the defaulttimestamp-requiredbehavior the update parks on the Dependency Dashboard waiting on a timestamp that never arrives.AikidoSec/aikido-claude-pluginpublishes no tags and no releases, so tracking a branch by digest is the only option available, and the manager assumes that branch ismain. An upstream on a differently named branch needs its own override.Verified by running Renovate locally in dry-run with the pin rewound to an older upstream commit: it resolves
renovate/aikido-digestcarrying the current SHA and reports the branch as not pending, and removing the rule puts that same update back topendingChecks: true.Stacked on #228, which adds the first external entry. Admitting
renovate[bot]past the audit workflow's actor gates is handled in bitwarden/gh-actions#917.