Skip to content

Ci: report high and critical production dependency advisories in validation - #799

Open
AmaadMartin wants to merge 2 commits into
mainfrom
feat/ci-production-dependency-audit
Open

Ci: report high and critical production dependency advisories in validation#799
AmaadMartin wants to merge 2 commits into
mainfrom
feat/ci-production-dependency-audit

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 8, 2026

Copy link
Copy Markdown
Owner

Please ensure you have read the contribution guide before creating a pull request.

Link to Issue or Description of Change

  1. Link to an existing issue (if applicable):
    Related: adm-zip range ^0.5.17 pins consumers to a version with an unpatched DoS (GHSA-xcpc-8h2w-3j85) google/adk-js#601
  2. Or, if no issue exists, describe the change:
    Problem: The validation workflow checks secrets, build, tests, coverage, lint, format and docs, but it never looks at the dependency tree. A high-severity advisory (GHSA-xcpc-8h2w-3j85, adm-zip < 0.6.0) therefore shipped in three releases of @google/adk before a downstream user reported it. CI gave no signal at any point.

Solution: Add an audit-dependencies job that runs npm audit --omit=dev --audit-level=high and writes the report to the run summary. The job reports and does not gate. npm audit has no way to acknowledge a finding, so a blocking gate would fail every open pull request the moment a new advisory lands against a dependency we already ship. The escalation path is one line: once the production tree is clean at high, drop continue-on-error.

Two decisions worth stating:

  • A job, not a step in run-tests. run-tests is a 3-OS matrix, so a step would audit three times for an OS-independent check. Its steps are also sequential, so an audit step would not run when the build or the tests fail. The job needs no npm install and no setup-node, because npm audit resolves the tree from package-lock.json and the runner already provides npm.
  • Advisory, not blocking. The production tree carries 11 high/critical findings today. Six of them are one root cause, the semver-major @mikro-orm/sqlite@7.1.11 bump, which a CI-configuration change cannot take. An allowlist was rejected: npm audit has no ignore flag, so it would mean a custom script plus a JSON file, and it would still red every open pull request on the next new advisory.

Scope is CI configuration and CONTRIBUTING.md only. The adm-zip range bump is tracked separately and is not in this change.

One thing I kept. A simplicity review asked me to delete the Publish audit report step, because the job log already prints the same bytes. I kept it: writing the report to the run summary is the point of the job, and it is what makes the finding visible without expanding a job log. I took the rest of that review — the setup-node step, the if: always() guard, the missing-log fallback, and the duplicated policy prose in the workflow comment are all gone.

Collision check. I listed the 300 open pull requests on the fork and diffed every plausibly adjacent one. No open or closed pull request adds a dependency audit. Six open pull requests touch .github/workflows/validation.yaml (#507, #649, #651, #669, #713, #756) and none of them audits dependencies; #651 is the closest, and it asserts that declared production dependencies install, which is a different check. I branched from main and did not stack.

Testing Plan

Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.
Unit Tests:
[ ] I have added or updated unit tests for my change.
[ ] All unit tests pass locally.

No unit tests. The change adds no executable source: two jobs' worth of workflow configuration and a Markdown section. The vitest coverage include globs reach only core/src, dev/src and integrations/src, so no coverage number moves. I verified the shell instead, as below.

Manual End-to-End (E2E) Tests:

I executed the two step bodies exactly as committed, read out of the YAML file, under bash -e with RUNNER_TEMP and GITHUB_STEP_SUMMARY set to temp files. My local npm mirror has no audit endpoint, so the local runs pass --registry=https://registry.npmjs.org/; the workflow does not.

Case Audit exit Summary verdict Publish exit
This repository 1 advisory / does not block, 13241-byte report 0
Scratch package, no dependencies 0 No high or critical advisories, found 0 vulnerabilities 0
Log file absent n/a (no audit output captured), fence closed 0

The audit reports 36 vulnerabilities (3 low, 22 moderate, 10 high, 1 critical) over 552 production packages, including adm-zip with GHSA-xcpc-8h2w-3j85.

Both guards were mutation-tested, and both mutations produce a wrong result:

  • Delete set -o pipefail: the audit step exits 0 over 11 findings, so the summary prints No high or critical advisories in the production dependency tree. directly above the report listing them.
  • Delete --audit-level=high: in a scratch package whose only advisory is moderate, npm audit --omit=dev exits 1 where --audit-level=high exits 0. The flag is what makes the verdict line mean "high or critical" rather than "any advisory".

steps.audit.outcome is used rather than conclusion because GitHub documents conclusion as the value after continue-on-error is applied, which is always success here.

On this pull request the job ran green in 12 seconds with no install step, and its log reports the same 36 vulnerabilities including adm-zip / GHSA-xcpc-8h2w-3j85. The three run-tests legs are unchanged and pass.

Reproduce locally:

python3 -c "import yaml;print(sorted(yaml.safe_load(open('.github/workflows/validation.yaml'))['jobs']))"
npm audit --omit=dev --audit-level=high; echo "exit=$?"

Checklist

[x] I have read the CONTRIBUTING.md document.
[x] I have performed a self-review of my own code.
[x] I have commented my code, particularly in hard-to-understand areas.
[ ] I have added tests that prove my fix is effective or that my feature works.
[ ] New and existing unit tests pass locally with my changes.

Amaad Martin added 2 commits August 8, 2026 03:22
The validation workflow never looked at the dependency tree, so
GHSA-xcpc-8h2w-3j85 shipped in three releases of @google/adk before a
downstream user reported it (google#601).

Add an audit-dependencies job that runs npm audit --omit=dev
--audit-level=high and writes the report to the run summary. The job
reports and does not gate: npm audit cannot acknowledge a finding, so a
blocking gate would fail every open pull request the moment a new
advisory lands against a dependency we already ship.
Drop the setup-node step: it supplies no node-version, so it pins
nothing over the npm the runner already provides, and license-check.yml
is checkout-plus-run for the same reason.

Drop `if: always()` and the missing-log fallback. `continue-on-error`
already lets the publish step run, and `tee` creates the log before the
audit can fail, so neither guard has a reachable case.

Move the --omit=dev and advisory-policy prose out of the workflow
comment; CONTRIBUTING.md is the one copy. Tighten that section too.
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.

1 participant