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 2 commits 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: REQUEST CHANGES Reviewed the Code Review Details
Still open from the previous review:
All three findings point at the same theme the author asked reviewers to check — whether the new scope gate can suppress real issues. The build-pipeline gap and the null-scope gap are both cases where the gate returns "not affected" for something that still warrants action. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| 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. |
Claude Configuration Validation — PR #175Scope reviewed:
No agents, commands, hooks, MCP configs, Verdict: 1 error (must fix) + 3 warnings (should fix). Structure, manifest, marketplace consistency, formatting, spelling, and the security scan are all clean. Errors (must fix)1. Version bump level is PATCH but the changelog documents feature additions — major
These change the skill's decision output for inputs that previously routed to "update immediately" — that is added capability, not a bug fix. Co-located Remediation: re-bump Warnings (should fix)2.
|
| Check | Result |
|---|---|
settings.local.json committed to git |
✅ Not tracked |
| Hardcoded secrets in changed files | ✅ None (no API keys, tokens, passwords; gh api uses bitwarden/ai-plugins placeholders only) |
| Permission scoping | ✅ N/A — no settings file changed in this PR |
| Dangerous command auto-approvals | ✅ N/A — no settings file changed in this PR |
| Overly broad file access | ✅ N/A — no permissions declared |
| Malformed agent/command/hook definitions | ✅ N/A — none changed |
Shell commands documented in the skill (gh api, npm audit, dotnet list package, grype) are read-only or advisory. The one mutating example, npm audit fix --force (line 181), is explicitly annotated "may introduce breaking changes" and is not auto-approved anywhere.
Lint (matches the lint.yml CI workflow)
pnpm is unavailable in this environment, so the pinned tool versions were run directly:
prettier@3.8.3 --checkon all changed files — All matched files use Prettier code style.cspell@10.0.0 linton the changed markdown — no spelling errors. The newepssentry in.cspell.json:41correctly covers the added Grype/Dependabot terminology.
Pre-existing issues (not introduced by this PR)
Surfaced by the plugin-validator sweep; out of scope for this PR but worth a follow-up:
plugins/bitwarden-security-engineer/agents/bitwarden-security-engineer.md:6-12— theskills:block declares 6 of the plugin's 9 skills;auditing-hackerone-vulns,bitwarden-security-context, andperform-security-revieware omitted, and the "Skill Routing" section (lines 31-38) has the same gap, so the agent has no routing path to them.plugins/bitwarden-security-engineer/agents/bitwarden-security-engineer.md:3— the agent description has no<example>blocks, unlike four of the repo's other agents; this weakens auto-delegation.plugins/bitwarden-security-engineer/README.md:11-19— the skills table omitsauditing-hackerone-vulnsandbitwarden-security-context.plugins/bitwarden-security-engineer/skills/perform-security-review/references/security-review-rubric.md:52— reaches across a skill boundary with../../analyzing-code-security/references/framework-checklists.md. The target exists, but every other cross-boundary reference in this plugin uses${CLAUDE_PLUGIN_ROOT}, which is rename-safe.
Recommendation
Request changes — solely for the semver level (error 1). The substantive content of this PR is a genuine improvement: it fixes a real alert-dropping bug in the Dependabot query, corrects two inaccurate Grype claims, and adds a scope-first triage path that prevents pointless urgency on build-time-only findings. Warnings 2-4 are small hardening items that can ride along with the version fix.
|
I am testing the new reusable workflow on this PR, so you may see some extra runs. |
| 2. **Is it a direct or transitive dependency?** — Transitive vulnerabilities may be harder to fix but still pose real risk. | ||
| 3. **What is the CVSS score and exploit availability?** — A high CVSS with a public exploit needs immediate action. A medium CVSS with no known exploit can be scheduled. | ||
| 4. **What versions are affected and what versions fix it?** — Check if updating is a minor bump or a breaking change. | ||
| 1. **Does the dependency ship, or is it build-time only?** — Start here; it usually decides the outcome. `scope` on the alert reports `development` or `runtime`. Treat `development` as a strong signal, not proof: confirm the package is absent from every shipped artifact (extension, desktop, web vault, CLI, server image) before relying on it, because some build-time packages inline code into published output. A build-time-only package has no live process in production for an attacker to reach. |
There was a problem hiding this comment.
dependency.scope is nullable, and neither Step 2 nor the Step 3 scope table says what to do when it is absent.
Details and fix
The Dependabot alerts API documents dependency.scope as development, runtime, or null — GitHub cannot always resolve execution scope from a manifest, so the field is frequently empty in practice (commonly on non-npm ecosystems). The Step 1 query now surfaces scope verbatim, so a null will reach the triager.
Step 2 item 1 asserts scope "reports development or runtime", and the Step 3 scope table has rows only for those two values. A null-scope alert therefore matches no row at the exact point where the whole triage now branches. Best case the reader falls through to the runtime table; worst case they infer "no scope reported" means "not a runtime dependency" and land on Not Affected / Informational for something that ships. Naming the safe default removes the guess.
Suggested — append to item 1:
scopemay also be absent; GitHub cannot always resolve it. Treat a missingscopeasruntimeuntil manifest inspection proves otherwise.
and add a row to the scope table:
| `scope` absent or unresolved | Treat as `runtime` and use the table below until manifest inspection proves the dependency is build-time only |Reference: List Dependabot alerts for a repository (dependency.scope: development, runtime, null)
There was a problem hiding this comment.
💭 @sognefej this looks like a legit gap. I double-checked, and the cited reference does state scope could be null. I didn't confirm if that's true in practice though. I think the suggested update is worth making, if you can confirm that's the actual behavior.
There was a problem hiding this comment.
Roger, I'll do some testing 👍
3 participants
📔 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.