Skip to content

Add dependency review workflow for PR vulnerability comments#7749

Merged
daveqnet merged 13 commits intomainfrom
add-dependency-review-action
Mar 25, 2026
Merged

Add dependency review workflow for PR vulnerability comments#7749
daveqnet merged 13 commits intomainfrom
add-dependency-review-action

Conversation

@daveqnet
Copy link
Copy Markdown
Contributor

@daveqnet daveqnet commented Mar 24, 2026

Unticketed

Description Of Changes

Adds a new GitHub Actions workflow (dep_review.yml) that runs dependency-review-action on PRs that modify dependency files (uv.lock, pyproject.toml, clients/package-lock.json, clients/**/package.json).

When a PR introduces a dependency with a known vulnerability, the action posts a summary comment directly on the PR conversation thread. GitHub already surfaces vulnerability counts in git push output, but these are easy to miss in the terminal. This puts the information where developers are already looking — in the PR conversation — with specific details on which packages are affected and what versions fix them.

Note on public visibility: Since this is a public repo, vulnerability details will be visible in PR comments. However, the lockfiles themselves are already public — anyone can run a scanner against them to identify the same information. We are not disclosing anything that isn't already trivially discoverable.

Current behavior:

  • Comments on PRs with a summary of newly introduced HIGH or CRITICAL vulnerabilities (diff-aware — only flags what the PR is adding, not pre-existing backlog)
  • Only posts a comment when vulnerabilities are found
  • Fails on HIGH or CRITICAL severity (with continue-on-error: true so it never actually blocks merging)

Why this approach:

  • dependency-review-action is a first-party GitHub action that uses GitHub's dependency graph diff API
  • Both actions are pinned to commit SHAs (not mutable version tags) per supply chain security best practices
  • Scoped to only run when lockfiles/manifests change, so no unnecessary CI cost

Testing:
Test commits were added during development to verify the workflow (vulnerable axios@1.7.0 for Node and ecdsa==0.18.0 for Python). Both were successfully detected and have been reverted.

Code Changes

  • Added .github/workflows/dep_review.yml — new workflow with dependency-review-action
  • Added changelog/7749-add-dependency-review-action.yaml — changelog entry

Steps to Confirm

  1. Check that the Dependency Review workflow runs on this PR
  2. Confirm a PR comment is posted summarizing vulnerabilities introduced by test commits (see PR conversation history)
  3. Confirm test commits have been reverted

Pre-Merge Checklist

  • Issue requirements met
  • All CI pipelines succeeded
  • CHANGELOG.md updated
    • Add a db-migration This indicates that a change includes a database migration label to the entry if your change includes a DB migration
    • Add a high-risk This issue suggests changes that have a high-probability of breaking existing code label to the entry if your change includes a high-risk change (i.e. potential for performance impact or unexpected regression) that should be flagged
    • Updates unreleased work already in Changelog, no new entry necessary
  • UX feedback:
    • All UX related changes have been reviewed by a designer
    • No UX review needed
  • Followup issues:
    • Followup issues created
    • No followup issues
  • Database migrations:
    • Ensure that your downrev is up to date with the latest revision on main
    • Ensure that your downgrade() migration is correct and works
      • If a downgrade migration is not possible for this change, please call this out in the PR description!
    • No migrations
  • Documentation:
    • Documentation complete, PR opened in fidesdocs
    • Documentation issue created in fidesdocs
    • If there are any new client scopes created as part of the pull request, remember to update public-facing documentation that references our scope registry
    • No documentation updates required

🤖 Generated with Claude Code

daveqnet and others added 2 commits March 24, 2026 23:30
Adds a GitHub Actions workflow that runs dependency-review-action on PRs
that modify lockfiles or dependency manifests. Posts a comment summarizing
any newly introduced vulnerabilities. Informational only — does not block
merging except on critical severity.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…orkflow

This commit intentionally introduces a known-vulnerable dependency to test
the dependency review workflow. Revert this commit after confirming the
workflow posts a PR comment.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@daveqnet daveqnet requested a review from a team as a code owner March 24, 2026 23:31
@daveqnet daveqnet requested review from gilluminate and removed request for a team March 24, 2026 23:31
@vercel
Copy link
Copy Markdown
Contributor

vercel bot commented Mar 24, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments
Project Deployment Actions Updated (UTC)
fides-plus-nightly Ignored Ignored Preview Mar 25, 2026 0:21am
fides-privacy-center Ignored Ignored Mar 25, 2026 0:21am

Request Review

@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 24, 2026

Dependency Review

The following issues were found:
  • ❌ 2 vulnerable package(s)
See the Details below.

Snapshot Warnings

⚠️: No snapshots were found for the head SHA 222975e.
Ensure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice.

Vulnerabilities

clients/sample-app/package-lock.json

NameVersionVulnerabilitySeverityPatched Version
axios1.7.0Server-Side Request Forgery in axioshigh1.7.4
axios Requests Vulnerable To Possible SSRF and Credential Leakage via Absolute URLhigh1.8.2
Axios is vulnerable to DoS attack through lack of data size checkhigh1.12.0
Axios is Vulnerable to Denial of Service via __proto__ Key in mergeConfighigh1.13.5

uv.lock

NameVersionVulnerabilitySeverityPatched Version
ecdsa0.18.0Minerva timing attack on P-256 in python-ecdsahighN/A
Only included vulnerabilities with severity high or higher.

Scanned Files

  • .github/workflows/dep_review.yml
  • clients/sample-app/package-lock.json
  • uv.lock

@daveqnet daveqnet self-assigned this Mar 24, 2026
@daveqnet daveqnet marked this pull request as draft March 24, 2026 23:33
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 24, 2026

Greptile Summary

This PR adds a new GitHub Actions workflow (dep_review.yml) that runs the first-party dependency-review-action on PRs modifying lockfiles or manifests, surfacing newly introduced HIGH/CRITICAL vulnerabilities as PR comments rather than only in terminal output. The changelog entry is correctly populated with the actual PR number.

  • Both actions are pinned to full commit SHAs — good supply-chain hygiene.
  • The path filters now include both clients/package-lock.json and clients/**/package-lock.json, covering nested lock files.
  • fail-on-severity: high with continue-on-error: true gives visibility without blocking merges — the intent is clearly documented in inline comments.
  • The comment-summary-in-pr: always setting will post a comment on every run (including clean ones), which contradicts the PR description's stated "silent on clean PRs" behaviour. The inline code comment explains the rationale (overwriting previous warnings on clean pushes), but the two descriptions conflict — one of them should be updated for clarity.
  • The uv.lock note about unofficial ecosystem support is appreciated and worth tracking going forward.

Confidence Score: 4/5

  • This PR is safe to merge — it adds a read-only CI workflow with no production code impact and previously raised concerns appear addressed.
  • The changelog PR number is correctly set to 7749, the clients/**/package-lock.json glob now covers nested lock files, both actions are pinned to commit SHAs, and the non-blocking continue-on-error: true design is clearly justified. The only remaining item is a minor inconsistency between comment-summary-in-pr: always and the PR description's "silent on clean PRs" claim — a P2 clarification, not a blocker.
  • No files require special attention — the workflow is the only substantive change and it looks correct.

Important Files Changed

Filename Overview
.github/workflows/dep_review.yml New dependency-review workflow with pinned action SHAs and sensible defaults; minor discrepancy between comment-summary-in-pr: always and the PR description's claim of "silent on clean PRs".
changelog/7749-add-dependency-review-action.yaml Changelog entry with correct PR number (7749) and appropriate type/description. No issues found.

Reviews (2): Last reviewed commit: "Switch back to comment-summary-in-pr: al..." | Re-trigger Greptile

Copy link
Copy Markdown

@claude claude bot left a comment

Choose a reason for hiding this comment

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

Dependency Review Workflow — Code Review

Good addition overall. The workflow is well-structured: actions are pinned to commit SHAs, permissions are minimal and correct (contents: read + pull-requests: write), and scoping the trigger to lockfile/manifest paths avoids unnecessary CI runs.

Critical (must fix before merge)

  • Revert the test axios commitaxios@1.7.0 with known HIGH vulnerabilities is still present in clients/sample-app/package.json and package-lock.json. The PR description calls this out explicitly. This must be dropped before merging.

Suggestions

  • fail-on-severity: high + continue-on-error: true is contradictory — the combination means HIGH severity vulnerabilities are detected but never actually block anything. This is apparently intentional, but it should be documented with a comment in the YAML so future maintainers don't remove continue-on-error: true without understanding the consequence, or alternatively so they can revisit whether enforcement is the right call.

  • comment-summary-in-pr: always adds noise on clean PRs — switching to on-failure would limit PR comments to runs that actually found something, keeping the PR thread cleaner on routine dependency bumps.

  • pr: 0 in the changelog — needs to be updated to 7749.

Nice to Have

  • The path globs for Python files (uv.lock, pyproject.toml) match root-only. Using **/uv.lock and **/pyproject.toml would be more future-proof.
  • It's worth verifying end-to-end that uv.lock changes actually surface in the dependency review comment — GitHub's Dependency Review API support for uv's lockfile format may be incomplete.

@daveqnet daveqnet added the do not merge Please don't merge yet, bad things will happen if you do label Mar 24, 2026
daveqnet and others added 6 commits March 24, 2026 23:41
…arnings

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…mber

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… races with the workflow

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…nt continue-on-error intent

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit intentionally introduces a known-vulnerable Python dependency
(CVE-2024-23342, Minerva timing attack) to test whether GitHub's dependency
graph picks up uv.lock changes. Expected result: no Python vulns in the PR
comment, confirming the uv.lock limitation. Revert before merge.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@daveqnet
Copy link
Copy Markdown
Contributor Author

Here's what the PR comment was like when I included two test vulnerable npm and pypi deps:

@daveqnet
Copy link
Copy Markdown
Contributor Author

Dependency Review

The following issues were found:
  • ❌ 2 vulnerable package(s)
See the Details below.

Snapshot Warnings

⚠️: No snapshots were found for the head SHA 222975e.
Ensure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice.

Vulnerabilities

clients/sample-app/package-lock.json

NameVersionVulnerabilitySeverityPatched Version
axios1.7.0Server-Side Request Forgery in axioshigh1.7.4
axios Requests Vulnerable To Possible SSRF and Credential Leakage via Absolute URLhigh1.8.2
Axios is vulnerable to DoS attack through lack of data size checkhigh1.12.0
Axios is Vulnerable to Denial of Service via __proto__ Key in mergeConfighigh1.13.5

uv.lock

NameVersionVulnerabilitySeverityPatched Version
ecdsa0.18.0Minerva timing attack on P-256 in python-ecdsahighN/A
Only included vulnerabilities with severity high or higher.

Scanned Files

  • .github/workflows/dep_review.yml
  • clients/sample-app/package-lock.json
  • uv.lock

daveqnet and others added 5 commits March 25, 2026 00:06
…ly documented

Revert test commits for ecdsa and axios.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
on-failure leaves stale vulnerability comments when vulns are fixed.
always ensures a clean push overwrites the previous warning.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@daveqnet daveqnet marked this pull request as ready for review March 25, 2026 00:25
Copy link
Copy Markdown

@claude claude bot left a comment

Choose a reason for hiding this comment

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

Clean, well-structured addition. Both actions are pinned to commit SHAs, permissions are minimal and correct, the trigger scope is narrow (only runs when lockfiles/manifests actually change), and the inline comments explain the non-obvious configuration choices clearly.

No critical issues. Three minor findings inline:

  1. Redundant path entryclients/package-lock.json is covered by clients/**/package-lock.json (GH Actions ** matches zero path segments too).
  2. retry-on-snapshot-warnings: false — given uv.lock isn't officially supported, silently skipping on snapshot warnings could mask gaps in Python coverage; true would make failures visible.
  3. license-check: false — worth revisiting in a follow-up to add an allow/deny license list.

One note on the PR description: it states the workflow is "silent on clean PRs", but comment-summary-in-pr: always will post a comment on every run regardless of findings. The inline comment in the workflow (# Always post so that a clean push overwrites any previous warning) explains the actual intent correctly — the PR description is just slightly inaccurate on this point.

@daveqnet daveqnet removed the do not merge Please don't merge yet, bad things will happen if you do label Mar 25, 2026
Copy link
Copy Markdown
Contributor

@eastandwestwind eastandwestwind left a comment

Choose a reason for hiding this comment

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

Looks great!

@daveqnet daveqnet added this pull request to the merge queue Mar 25, 2026
Merged via the queue into main with commit 2bf995b Mar 25, 2026
43 checks passed
@daveqnet daveqnet deleted the add-dependency-review-action branch March 25, 2026 11:47
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