Skip to content

llm: align dependency review with CVSS v3.0 and dependency scope - #175

Open
sognefej wants to merge 1 commit into
mainfrom
security-review/cvss-alignment
Open

llm: align dependency review with CVSS v3.0 and dependency scope#175
sognefej wants to merge 1 commit into
mainfrom
security-review/cvss-alignment

Conversation

@sognefej

Copy link
Copy Markdown
Contributor

📔 Objective

CVSS version. Specifies CVSS v3.0 for scoring, and explains how to get there from what GitHub publishes — advisories emit CVSS:3.1 vectors, 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 development finding 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

  • Alert query returned only the first page, silently dropping alerts past 100. Page order is not severity-ranked, so a critical could be dropped. Now --paginate --slurp piped to jq (gh rejects --slurp with --jq) so the sort is global, not per-page.
  • Query claimed to sort by severity but never sorted.
  • cvss_v3.score returns 0 not null when an advisory has no v3 vector, making high-severity findings appear to score zero. Now guarded.
  • Grype: --only-fixed filters by fix availability, not severity, and --fail-on sets the exit code without filtering output — these were conflated in one mislabelled example. Column is FIXED IN, default set includes EPSS/RISK, and FIXED IN disappears when nothing has a fix.
  • Query now surfaces 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.

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.
@sognefej
sognefej requested a review from a team as a code owner July 29, 2026 18:25
@github-actions

github-actions Bot commented Jul 29, 2026

Copy link
Copy Markdown

Plugin Validation Report — PR #175

Plugin: bitwarden-security-engineer (1.3.0 → 1.3.1)
Scope: plugin.json, CHANGELOG.md, skills/reviewing-dependencies/SKILL.md (the PR also touches .claude-plugin/marketplace.json, README.md, and .cspell.json — all three verified correct and required)

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

Check Result
plugin.json manifest Valid JSON; name kebab-case, version semver, description/author/homepage/repository/keywords present
Version consistency 1.3.1 in all four locations: plugin.json:3, .claude-plugin/marketplace.json:57, root README.md:20, CHANGELOG.md:8
CHANGELOG format Keep a Changelog header + both spec links, reverse-chronological ## [X.Y.Z] - YYYY-MM-DD, standard categories, date matches today (2026-07-29)
Agent agents/bitwarden-security-engineer.md — path in plugin.json resolves, name 27 chars lowercase-hyphen, model: opus, color: red valid, system prompt well over 20 chars, Skill present in tools:
Skills 9 of 9 have valid frontmatter with name + description; every name matches its directory
Changed skill size SKILL.md = 216 lines / ~1,634 words — under the 500-line limit, mid-range of the 1,000–3,000 word target
File references All references//examples/ targets across the plugin resolve on disk, including the cross-skill hop in perform-security-review
${CLAUDE_PLUGIN_ROOT} Correct in all 4 sites that need it
Hooks / MCP / commands None declared — nothing to validate
Security scan Clean — no secrets, tokens, keys, or credentials in any changed line; no settings.local.json tracked in git; no settings/permission changes; no dangerous auto-approvals; no http:///ws:// endpoints
jq/shell correctness --slurp correctly paired with --paginate; .[][] correctly flattens the slurped array-of-pages (verified by executing the pipeline); all field paths match the Dependabot alerts schema; $c safe inside single quotes; the sort= claim (created/updated/epss_percentage only) is accurate

⚠️ Warnings — recommended before merge

W1 — dependency.scope can be null, and the new scope-first table has no row for it

plugins/bitwarden-security-engineer/skills/reviewing-dependencies/SKILL.md:54, table at :64-68

Line 54 states scope "reports development or runtime", but GitHub's Dependabot alert schema allows dependency.scope to be null (unset where GitHub cannot determine execution scope). Line 62 instructs "Resolve scope first," yet the table only has development and runtime rows — an alert with scope: null leaves no defined path, and the likely failure mode is defaulting to the lenient branch.

Fix: soften line 54 to "development, runtime, or absent," and add a table row:

| `scope` absent or `null` | Treat as `runtime` until proven otherwise; determine scope manually from `manifest` and the dependency declaration |

W2 — The development → "Not Affected / Informational" path needs a build-time-execution carve-out

SKILL.md:54, 62, 66

"A build-time-only package has no live process in production for an attacker to reach" (line 54) holds for the shipped artifact but not for the build pipeline — and this same skill warns at line 198 that postinstall scripts "execute arbitrary code during npm install." For Bitwarden specifically, a malicious-code or RCE advisory in a devDependency that runs in CI (repo write access, signing material, release secrets) is a supply-chain incident, not Informational.

Fix: add an exception to the development row — advisories describing malicious code, install-script execution, arbitrary code execution at build time, or anything touching CI secret handling stay on the runtime table regardless of scope.

W3 — Verify the --fail-on exit code before publishing "code 2"

SKILL.md:139

The comment asserts "Exit non-zero (code 2)". Grype's own --fail-on help text reads "set the return code to 1 if a vulnerability is found with a severity >= the given severity." grype is not installed in this environment, so this could not be settled locally. If someone writes a CI gate on [ $? -eq 2 ] and the real code is 1, the gate silently passes on vulnerable images.

Fix: confirm against the pinned 0.116.x, or write "exits non-zero" and drop the number. The rest of the line — that --fail-on sets the exit code and does not filter output — is correct, as is the --only-fixed correction above it.

W4 — PATCH bump carries an ### Added section of behavior-changing entries

plugins/bitwarden-security-engineer/CHANGELOG.md:8-14

## [1.3.1] is a PATCH, but its ### Added block lists three backward-compatible additions, including a new triage outcome (a development finding declared Not Affected / Informational) and a new decision row. .claude/CLAUDE.md maps "new features, backward-compatible additions" to MINOR, and PATCH to "bug fixes, documentation updates." The scope-first decision table changes agent triage behavior, not just prose. CI only checks version consistency, not bump magnitude, so this will not fail automatically.

Fix — pick one:

  • echo "y" | ./scripts/bump-plugin-version.sh bitwarden-security-engineer 1.4.0, retitle the CHANGELOG heading, and hand-edit README.md:20; or
  • if the intent is documentation correction only, move the three ### Added bullets under ### Changed and keep 1.3.1.

Minor notes

# 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) and bitwarden-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.sh only walks agents/*/AGENT.md (directory form), so this plugin's flat-file agent — including its tools: array — is skipped by CI entirely. Worth extending the script to also walk agents/*.md.
  • skills/perform-security-review/SKILL.md:64 — the subagent-facing instruction uses the skill-relative path references/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.sh and ./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; the lint.yml CI job is authoritative here.
  • grype and gh — 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 development become 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 overrides entry 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-on correction and the fix.state JSON 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.

@github-actions

github-actions Bot commented Jul 29, 2026

Copy link
Copy Markdown

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the reviewing-dependencies skill rewrite in bitwarden-security-engineer, covering the Dependabot alert-gathering query, the CVSS v3.0 guidance, the new scope-first triage tables, and the corrected Grype examples. The jq pipeline was traced against fixture data for the paginated .[][] flatten, the cvss_v3.vector_string guard, and the severity sort_by — all behave as documented, and the --slurp/--jq and Dependabot sort= constraints noted in the comments are accurate. Version bump, changelog entry, cspell dictionary addition, and README table are all present and consistent; lint and marketplace validation pass on CI. Two findings, neither blocking.

Code Review Details
  • ⚠️ : Scope-first triage evaluates reachability only in production, so build-time dependencies with malicious-code, install-script, or build-tampering advisories are routed to Not Affected / Informational despite the CI pipeline being a live target
    • plugins/bitwarden-security-engineer/skills/reviewing-dependencies/SKILL.md:62-70
  • ♻️ : Patch bump on a release with an ### Added section conflicts with the repo's MINOR-for-additions rule and the plugin's own 1.1.0/1.3.0 precedent
    • plugins/bitwarden-security-engineer/CHANGELOG.md:8

Comment on lines +62 to +70
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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ IMPORTANT: Scope-first triage judges reachability only in production, so build-pipeline compromise is silently excluded.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ 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.0

The ### Fixed entries alone would justify a patch, but the release as a whole is additive.

@sognefej

Copy link
Copy Markdown
Contributor Author

Having some more eyes on this to make sure it's not suppressing real issues.

@withinfocus

Copy link
Copy Markdown
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants