llm: align dependency review with CVSS v3.0 and dependency scope - #175
llm: align dependency review with CVSS v3.0 and dependency scope#175sognefej wants to merge 1 commit into
Conversation
Specify CVSS v3.0 as the version used for Bitwarden's vulnerability reporting and triage in the reviewing-dependencies skill, and explain how to reach a v3.0 score from the v3.1 vectors GitHub publishes. Resolve dependency scope before fix availability when deciding on action. A build-time-only finding confirmed absent from every shipped artifact is declared Not Affected / Informational and tracked with the next routine parent-tooling upgrade, rather than being routed to "update immediately" by the fix-availability table. Fix defects found while testing the skill against live Dependabot data: - The alert query returned only the first page, silently dropping alerts beyond 100. Page order is not severity-ranked, so a critical finding could be dropped. - The severity sort was claimed in a comment but never performed, and with --paginate alone it would have sorted per page rather than globally. - cvss_v3.score reads 0 rather than null when an advisory carries no v3 vector, making high-severity findings appear to score zero. - Grype: --only-fixed filters by fix availability rather than severity, and --fail-on sets the exit code without filtering output. The table column is FIXED IN, the default set also includes EPSS and RISK, and FIXED IN is omitted entirely when no finding has a fix (verified against grype 0.116.1). Surface scope, manifest, vulnerable_range, and first_patched in the gathering query, since the assessment steps ask about them.
Plugin Validation Report — PR #175Plugin: Verdict: ✅ No blocking errors. 4 warnings recommended before merge, 11 minor notes. Structure, manifest, frontmatter, version bookkeeping, file references, and secret scanning all pass. Every finding below is in content quality or technical accuracy of the new guidance, not in plugin validity. ✅ Passing checks
|
| # | Location | Note |
|---|---|---|
| M1 | SKILL.md:40 |
Verified by executing the jq: an unmapped or null severity sorts ahead of critical, because the map lookup yields null and jq orders null lowest. The four keys are correct for this endpoint today, so it only bites on a future GitHub value — but it corrupts exactly the ordering the query exists to provide. Fix: sort_by({critical:0,high:1,medium:2,low:3}[.severity] // 99) |
| M2 | SKILL.md:57 |
The v3.1→v3.0 transcription is missing the mechanical detail that makes it fail: a v3.0 calculator rejects a string beginning CVSS:3.1/; the prefix must be rewritten to CVSS:3.0/. Also worth one clause — v3.1 redefined Roundup, so the recomputed v3.0 score can differ from GitHub's published v3.1 score, and the published number must not be quoted as the v3.0 score |
| M3 | SKILL.md:21, 36 |
"cvss_v3.score returns 0 rather than null" is stated as fact. The guard keys on vector_string so it is correct under either behavior — the assertion is the risk. GitHub's schema declares score nullable; the legacy security_advisory.cvss object is the field known to emit 0.0. Soften to "may return 0" |
| M4 | SKILL.md:14-15 |
The comment credits --slurp with merging pages into one array; --slurp wraps each page body in an outer array, and the flattening is done by .[][] at line 25. .[][] is correct as written, but a reader who believes slurp already flattened may "simplify" it to .[] and silently restore per-page sorting. Also worth noting --slurp requires gh ≥ 2.53 (the doc pins a grype version but not a gh version) |
| M5 | SKILL.md:44 |
(pre-existing, untouched by the diff) The severity-filter example has no --paginate and no per_page, so it truncates at the default 30 — the exact trap now documented in detail 30 lines above. Fix: gh api --paginate ".../dependabot/alerts?severity=critical&state=open&per_page=100" |
| M6 | skills/triaging-security-findings/SKILL.md:53 |
(out of PR scope, but the reason M5 matters) This sibling skill still carries the original single-page query, and agents/bitwarden-security-engineer.md:34 routes "Dependabot alerts" to triaging-security-findings while :37 routes "Dependabot triage" to reviewing-dependencies. Both routes exist, so the pagination bug stays reachable on a default dispatch path. Fix the query there too, or replace it with a pointer to Step 1 here |
| M7 | SKILL.md:56 |
Step 2 item 3 sends the reader to npm ls, but the alert already exposes dependency.relationship (direct/transitive) and the jq projection omits it. Adding relationship: .dependency.relationship, answers the question from the same output — keep npm ls for finding the pinning parent. Also, the npm ls advice does not cover NuGet, which this skill explicitly supports |
| M8 | SKILL.md:146 |
Enumerating a volatile default column set pinned to a patch release invites staleness (older grype used FIXED-IN; the EPSS header has rendered as EPSS%). grype is not installed here, so the 0.116.1 verification claimed in the CHANGELOG could not be independently confirmed. Consider keeping the dynamic-column sentence and leaning on the -o json contract at line 148, which is accurate |
| M9 | SKILL.md:66, 70 |
The skill now mandates a written "Not Affected / Informational" declaration but never says where it goes or what it contains — the term appears nowhere else in the plugin and maps to no Dependabot dismissal reason. Name the fields (alert number, package, scope, artifacts checked, method, conclusion, review date) and the destination: gh api -X PATCH /repos/bitwarden/ai-plugins/dependabot/alerts/{n} -f state=dismissed -f dismissed_reason=not_used -f dismissed_comment="..." |
| M10 | SKILL.md:3 |
The description does not cover the CVSS scoring role this PR just gave the skill — "score this CVE in CVSS v3.0" matches none of the five triggers, and no other skill in the plugin mentions CVSS. Room remains at ~314 chars to add "score a CVE in CVSS v3.0" / "convert a CVSS v3.1 vector" |
| M11 | SKILL.md:81, :10-22 |
Style: the last decision-table cell is a full multi-sentence paragraph (252 chars) — better as a prose note beneath the table; and 12 lines of "why this is shaped this way" rationale sit inside a bash fence users copy-paste — better as imperative bullets under the block |
Pre-existing issues (not introduced here; separate housekeeping PR)
plugins/bitwarden-security-engineer/README.md:11-19— Skills table documents 7 of 9 skills;auditing-hackerone-vulns(1.2.0) andbitwarden-security-context(1.3.0) are missing.agents/bitwarden-security-engineer.md:3— agent description has no<example>blocks to sharpen automatic delegation.scripts/validate-plugin-structure.shonly walksagents/*/AGENT.md(directory form), so this plugin's flat-file agent — including itstools:array — is skipped by CI entirely. Worth extending the script to also walkagents/*.md.skills/perform-security-review/SKILL.md:64— the subagent-facing instruction uses the skill-relative pathreferences/security-review-rubric.md; a freshly spawned agent resolves from the repo root, so it should use${CLAUDE_PLUGIN_ROOT}/skills/perform-security-review/references/.... (The same path at line 82 is fine — orchestrator context.)
What could not be verified in this environment
Reported for transparency rather than left implied:
./scripts/validate-plugin-structure.shand./scripts/validate-marketplace.sh— blocked by sandbox approval. All the checks they perform were carried out manually and pass (see table above).pnpm run lint/prettier --check— blocked. Markdown table alignment on the two new tables could not be confirmed byte-exact; thelint.ymlCI job is authoritative here.grypeandgh— not installed / no network, so W3 and M8 rest on documented tool behavior rather than execution.
Positive notes
- The pagination fix is the right one, and the reasoning is captured where a future editor will see it — which is precisely why the un-paginated leftovers in M5/M6 stand out as oversights rather than design.
- Scope-before-severity is the correct triage order, and it is stated without letting
developmentbecome an escape hatch ("strong signal, not proof"; "'It's a devDependency' is not by itself an assessment"). - The transitive rows now distinguish "fix installable" from "a parent pins the vulnerable version" — the distinction that actually determines whether an
overridesentry is safe. - The query now emits exactly the fields Steps 2 and 3 interrogate, so the workflow is self-contained.
- v4.0 is correctly called out as non-mappable to v3.0 rather than silently reused.
- The
--only-fixed/--fail-oncorrection and thefix.stateJSON pointer fix real defects in the prior guidance. - CHANGELOG entries are specific about both behavior change and rationale.
Recommendation: approve after W1–W4. None of the four is large: W1 and W2 are table rows, W3 is a one-word change or a version check, W4 is a bump-or-recategorize decision.
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Reviewed the Code Review Details
|
| Resolve scope first. A package that never reaches a shipped artifact cannot be attacked in production, and treating it as though it can produces pointless urgency and risky churn. | ||
|
|
||
| | Scope | Action | | ||
| | ------------------------------------------------------------------------ | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | ||
| | `development`, confirmed absent from every shipped artifact | Declare **Not Affected / Informational**. Record the reachability reasoning explicitly, then track the bump with the next routine upgrade of the parent tooling rather than as a hotfix | | ||
| | `development`, but the package or its output lands in a shipped artifact | Treat as `runtime` and use the table below | | ||
| | `runtime` | Use the table below | | ||
|
|
||
| A `development` finding still needs the declaration written down. "It's a devDependency" is not by itself an assessment — state which artifacts were checked and why the code path cannot be reached in production. |
There was a problem hiding this comment.
Details and fix
Line 62 and the development row both reason exclusively about production: "a package that never reaches a shipped artifact cannot be attacked in production" and "no live process in production for an attacker to reach." That is correct as far as it goes, but a build-time dependency still executes with access to CI runners, signing material, registry tokens, and the build output itself. event-stream, ua-parser-js, and xz-utils were all reachable purely through the build/install path.
As written, an advisory for arbitrary code execution or malicious code in a build tool that ships nothing gets routed to Not Affected / Informational and deferred to "the next routine upgrade of the parent tooling." That is the wrong outcome for that class.
Suggest carving it out — e.g. an extra row before the development/absent row:
| `development`, but the advisory is malicious code, install-script execution, or build tampering | Treat as urgent regardless of shipping. The build pipeline is a live target: CI credentials, signing material, and the produced artifact are all in reach |and softening line 54's closing sentence to scope the claim to production runtime rather than to attackers generally.
Reference: CWE-1357, SLSA build-integrity threats
| The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), | ||
| and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). | ||
|
|
||
| ## [1.3.1] - 2026-07-29 |
There was a problem hiding this comment.
♻️ DEBT: Patch bump for a release carrying an ### Added section conflicts with the repo's versioning rule and this plugin's own precedent.
Details and fix
.claude/CLAUDE.md assigns MINOR to "new features, backward-compatible additions" and PATCH to "bug fixes, documentation updates, security patches." This entry adds a new Step 2 assessment question, a new scope-resolution decision table, and a new transitive-pin decision row — all filed under ### Added.
The plugin's history bumps MINOR for exactly this shape of change: 1.3.0 added ADR-alignment guidance to existing skills, and 1.1.0 revised threat-modeling guidance. 1.0.1 is the PATCH precedent, and it was "Simplified ... guidance" with a ### Changed section only.
Suggest 1.4.0:
echo "y" | ./scripts/bump-plugin-version.sh bitwarden-security-engineer 1.4.0The ### Fixed entries alone would justify a patch, but the release as a whole is additive.
|
Having some more eyes on this to make sure it's not suppressing real issues. |
|
The validator found a lot of wins I'd say, but are out of scope for this PR (pretty sure I wrote these skills before I built the validator workflow). Please follow up with something once / if this lands. |
📔 Objective
CVSS version. Specifies CVSS v3.0 for scoring, and explains how to get there from what GitHub publishes — advisories emit
CVSS:3.1vectors, which map directly onto v3.0 since the base metrics are identical. v4.0-only advisories are flagged as needing manual scoring.Scope before fix availability. The action decision now resolves dependency scope first. A
developmentfinding confirmed absent from every shipped artifact is declared Not Affected / Informational and tracked with the next routine parent-tooling upgrade. Previously the table looked only at fix availability, so a build-time-only devDependency with a patch available was routed to "update immediately" — and in the transitive case that invited an override that can break the parent pinning it.Fixes
--paginate --slurppiped tojq(gh rejects--slurpwith--jq) so the sort is global, not per-page.cvss_v3.scorereturns0notnullwhen an advisory has no v3 vector, making high-severity findings appear to score zero. Now guarded.--only-fixedfilters by fix availability, not severity, and--fail-onsets the exit code without filtering output — these were conflated in one mislabelled example. Column isFIXED IN, default set includesEPSS/RISK, andFIXED INdisappears when nothing has a fix.scope,manifest,vulnerable_range,first_patched— fields the assessment steps ask about but never retrieved.Validation
Commands were extracted from the skill and run against live Dependabot data rather than retyped. Grype claims verified against an installed 0.116.1 binary with fixtures for fixed, unfixed, and mixed states. Lint, structure, and marketplace checks pass; versions consistent at 1.3.1.