ci(dependency-review): fail only on advisories the base doesn't have

.github/workflows/ci.yml

@@ -1221,9 +1221,10 @@ jobs:
   # base and of its head compared, over every manifest the graph reads,
   # the lockfiles of the root and of the end-to-end servers and the
   # pinned Python tools among them. It fails on a package the pull
-  # request adds, or moves to another version, with a known
-  # vulnerability of moderate severity or worse, or under a license not
-  # on the list below, and lists what moved in the run's summary, which
+  # request adds, or moves to another version, under a license not on
+  # the list below, or with a known vulnerability of moderate severity or
+  # worse that the base doesn't have, and lists what moved in the run's
+  # summary, which
   # a person reads before merging Renovate's lock file maintenance
   # (GOVERNANCE.md#dependency-updates). On a pull request only, as ci-ok
   # expects: what the merge queue's tree and a push to main bring in came
@@ -1243,7 +1244,10 @@ jobs:
     steps:
       - uses: actions/dependency-review-action@a1d282b36b6f3519aa1f3fc636f609c47dddb294 # v5.0.0
         with:
-          fail-on-severity: moderate
+          # The licenses only: the action fails on every advisory of a
+          # version that comes, even one the version that goes had too,
+          # which leaves main as it was. The step below judges those.
+          vulnerability-check: false
           # Every scope, not only the packages the app runs: the
           # development tools run in CI and on contributors' machines,
           # and build the site that is deployed.
@@ -1279,9 +1283,13 @@ jobs:
           # Scorecard's scores come from another service, and the summary
           # below names every package that moved already.
           show-openssf-scorecard: false
-      # The action's summary names the problems it found. This lists
-      # every package added, changed or removed, per manifest, from the
-      # same comparison, whether the review passed or not. gh fetches it
+      # The action's summary names the licenses it refused. This fails
+      # on an advisory of moderate severity or worse that a version
+      # which comes has and no version which goes, of the same package
+      # in the same manifest, had: one the base has already is listed,
+      # and Dependabot's alerts on main report it. It also lists every
+      # package added, changed or removed, per manifest, from the same
+      # comparison, whether the licenses passed or not. gh fetches it
       # again rather than a step reading the action's output, which a
       # lock file maintenance can make too large to pass to a step.
       - if: success() || failure()
@@ -1294,7 +1302,7 @@ jobs:
           node-version-file: .nvmrc
       # The script imports no package, so nothing is installed.
       - if: success() || failure()
-        name: npm run dependencies:summary -- <changes.json>
+        name: npm run dependencies:review -- <changes.json>
         env:
           GH_TOKEN: ${{ github.token }}
           REPO: ${{ github.repository }}
@@ -1303,7 +1311,7 @@ jobs:
         run: |
           changes="$RUNNER_TEMP/changes.json"
           gh api --paginate --slurp "repos/$REPO/dependency-graph/compare/$BASE...$HEAD?per_page=100" > "$changes"
-          npm run dependencies:summary -- "$changes"
+          npm run dependencies:review -- "$changes"
 
   # When a job that tests the tree fails, the run's result is settled, so
   # the jobs still going are canceled rather than left to finish: the

docs/testing.md

@@ -24,7 +24,7 @@ npm run browsers  # once: Chromium's headless shell, which npm run check and npm
 npm run lint      # Biome: the formatting and lint rules of the TypeScript, JSON and CSS (docs/style.md)
 npm run format    # Biome: rewrite those files as it formats them
 npm run coverage:summary   # the coverage the last tests measured, per package, as a table
-npm run dependencies:summary -- changes.json   # the packages GitHub's comparison of two commits' dependency graphs adds, changes and removes, as tables, as CI's dependency-review job writes them
+npm run dependencies:review -- changes.json    # the advisories and the packages GitHub's comparison of two commits' dependency graphs adds, changes and removes, as tables, as CI's dependency-review job writes them; fails on an advisory the base doesn't have
 npm run lint:markdown      # markdownlint over the project's Markdown, as npm run check runs it
 npm run lint:spelling      # cspell, American English, over the Markdown and en.json, as npm run check runs it
 npm run lint:prose         # the docs' prose rules, per page kind (docs/style.md#the-prose-check), as npm run check runs it; -- docs/srs.md for one page
@@ -639,7 +639,7 @@ jobs run side by side, each
 | `docs` | deterministic | the docs' links, every file's license, and `publiccode.yml` ([Documentation](#documentation)) |
 | `workflows` | deterministic | the workflows and local actions, by actionlint and zizmor (`npm run lint:workflows`, [The workflows](#the-workflows)) |
 | `commits` | deterministic | on a pull request only: each commit it adds follows Conventional Commits, by commitlint ([CONTRIBUTING.md](../CONTRIBUTING.md#commit-messages)), and its body or a commit closes its issue, or the body says it's one step of it (`npm run check:closes`; not for Renovate's) |
-| `dependency-review` | deterministic | on a pull request only, skipped on every other run: no package it adds or changes has a known vulnerability of moderate severity or worse, or a license off the list, by GitHub's dependency graph; every package it adds, changes or removes goes to the run's summary ([Dependency review](#dependency-review)) |
+| `dependency-review` | deterministic | on a pull request only, skipped on every other run: no package it adds or changes has a known vulnerability of moderate severity or worse that the base doesn't have, or a license off the list, by GitHub's dependency graph; every package it adds, changes or removes goes to the run's summary ([Dependency review](#dependency-review)) |
 | `caches` | deterministic | nothing: on `main`, after `check` or `reuse`, it deletes all but the newest of `main`'s Turbo caches, of the CodeQL databases per language, and of each end-to-end server's images, and all but the two newest of the browsers' system libraries per browser. Its token can write (`actions: write`), as only the `fail-fast` jobs' can besides. It runs none of the repository's code, and its failure fails neither the run nor `ci-ok` |
 | `fail-fast (<job>)` | deterministic | nothing: one per job that tests the tree, `commits` and `dependency-review` aside, needing that job alone. When it fails, outside `main` and not for a fork's pull request, it cancels the run ([Canceled runs](#canceled-runs)); when it passes, it is skipped. Its token can write (`actions: write`), and it runs none of the repository's code |
 | `warm`, `warm-images`, `warm-browsers`, `warm-pip` | deterministic | nothing: on `main`, when the run reuses another, they save only the caches the skipped jobs would have saved and `main` lacks (`releasing.md`), failing neither the run nor `ci-ok` |
@@ -858,7 +858,7 @@ maintainer account still publishes a validly signed version.
 
 CI's `dependency-review` job runs GitHub's
 [dependency-review-action](https://github.com/actions/dependency-review-action)
-on a pull request. The action compares GitHub's dependency graph of
+on a pull request. The job compares GitHub's dependency graph of
 the pull request's base and of its head. The graph covers every
 manifest GitHub reads here: the root's `package-lock.json`, those of
 the end-to-end servers in `e2e/pod/servers/`, the
@@ -867,22 +867,34 @@ on a package the pull request adds, or moves to another version, that
 has either of these:
 
 - a known vulnerability of moderate severity or worse, in GitHub's
-  advisory database, in any scope, the development tools included;
+  advisory database, in any scope, the development tools included, that
+  the base doesn't have;
 - a license that isn't on the job's `allow-licenses` list, the licenses
   the project can ship a package under
   ([style.md](style.md#licenses)).
 
+The base has a vulnerability already when a version of the same package
+that the pull request removes, in the same manifest, has the same
+advisory. Moving a package from one vulnerable version to another, as
+lock file maintenance does when no fixed version can be locked yet,
+leaves `main` as it was, so the job lists that advisory as carried over
+and passes. A new version that fixes one advisory and has another still
+fails. GitHub's Dependabot alerts on `main`, not this job, report the
+advisories that `main` already has, and Renovate's vulnerability alerts
+propose the fixed versions
+([renovate.json5](../renovate.json5)).
+
 A package whose license GitHub doesn't know is reported, and doesn't
 fail the job. A package already accepted under a license that can't be
 on the list is in the job's `allow-dependencies-licenses`, with its
 reason in a comment. None of them ships in the site: an end-to-end test
 server, build and release tools, and the license linter.
 
-The action names what it found in the run's summary.
-`npm run dependencies:summary` lists every package added, changed or
-removed, per manifest, with its scope and license, from the same
-comparison. That list is what a maintainer reads before merging
-Renovate's lock file maintenance
+The action checks the licenses and names the ones it refused in the
+run's summary. `npm run dependencies:review` judges the advisories, and
+lists them and every package added, changed or removed, per manifest,
+with its scope and license, from the same comparison. That list is what
+a maintainer reads before merging Renovate's lock file maintenance
 ([GOVERNANCE.md](../GOVERNANCE.md#dependency-updates)).
 
 The job runs on a pull request only. The packages the merge queue's
@@ -893,6 +905,8 @@ the repository's dependency graph, which an admin turns on in
 Settings › Advanced Security. Without it the job fails with
 "Dependency review is not supported on this repository", and `ci-ok`
 fails with it on every pull request.
+Dependabot alerts, turned on in the same place, report what the job
+lets through as carried over.
 
 ### Code scanning
 

package.json

@@ -56,7 +56,7 @@
     "check:keys": "node -e \"import('./scripts/checkQueryKeys.ts').then((m) => m.run(process))\"",
     "ci:gate": "node -e \"import('./scripts/ciGate.ts').then((m) => m.run(process))\"",
     "check:providers": "node -e \"import('./scripts/checkProviders.ts').then((m) => m.run(process))\"",
-    "dependencies:summary": "node -e \"import('./scripts/dependencyChanges.ts').then((m) => m.run(process))\" --",
+    "dependencies:review": "node -e \"import('./scripts/dependencyChanges.ts').then((m) => m.run(process))\" --",
     "coverage:summary": "node -e \"import('./scripts/coverageSummary.ts').then((m) => m.run(process))\"",
     "release:commit": "node -e \"import('./scripts/releaseCommit.ts').then((m) => m.run(process))\" --",
     "release:version": "node -e \"import('./scripts/releaseVersion.ts').then((m) => m.run(process))\" --",

scripts/dependencyChanges.test.ts

@@ -6,7 +6,14 @@ import { mkdtempSync, readFileSync, writeFileSync } from "node:fs";
 import { tmpdir } from "node:os";
 import { join } from "node:path";
 import { describe, expect, it, vi } from "vitest";
-import { type Change, changes, render, run } from "./dependencyChanges.ts";
+import {
+  advisories,
+  type Change,
+  changes,
+  render,
+  renderAdvisories,
+  run,
+} from "./dependencyChanges.ts";
 
 function change(
   change_type: Change["change_type"],
@@ -64,6 +71,22 @@ const TABLES = [
   "",
 ].join("\n");
 
+/** An advisory on a version, at a severity. */
+function advisory(
+  id: string,
+  severity: "critical" | "high" | "moderate" | "low" = "moderate",
+): NonNullable<Change["vulnerabilities"]>[number] {
+  return {
+    severity,
+    advisory_ghsa_id: id,
+    advisory_summary: `${id}: a | in the summary`,
+    advisory_url: `https://github.com/advisories/${id}`,
+  };
+}
+
+const NO_ADVISORIES =
+  "## Advisories\n\nNo added version has an advisory of moderate severity or worse.\n";
+
 describe("changes", () => {
   it("reads gh's pages, each a list of changes, as one list", () => {
     const [a, b, c] = CHANGES;
@@ -95,30 +118,189 @@ describe("render", () => {
   });
 });
 
+describe("advisories", () => {
+  it("carries over an advisory that a removed version of the package in the manifest had", () => {
+    const moved = change("added", "ip-address", "10.7.2", {
+      vulnerabilities: [advisory("GHSA-a")],
+    });
+    expect(
+      advisories([
+        change("removed", "ip-address", "10.5.0", { vulnerabilities: [advisory("GHSA-a")] }),
+        moved,
+      ]),
+    ).toEqual({
+      introduced: [],
+      carried: [
+        {
+          manifest: "package-lock.json",
+          name: "ip-address",
+          version: "10.7.2",
+          vulnerability: advisory("GHSA-a"),
+        },
+      ],
+    });
+  });
+
+  it("introduces an advisory the removed version didn't have, though it had another", () => {
+    const { introduced, carried } = advisories([
+      change("removed", "a", "1.0.0", { vulnerabilities: [advisory("GHSA-a")] }),
+      change("added", "a", "2.0.0", { vulnerabilities: [advisory("GHSA-a"), advisory("GHSA-b")] }),
+    ]);
+    expect(introduced.map(({ vulnerability }) => vulnerability.advisory_ghsa_id)).toEqual([
+      "GHSA-b",
+    ]);
+    expect(carried.map(({ vulnerability }) => vulnerability.advisory_ghsa_id)).toEqual(["GHSA-a"]);
+  });
+
+  it("introduces an advisory that only another manifest, or another package, had", () => {
+    const { introduced, carried } = advisories([
+      change("removed", "a", "1.0.0", {
+        manifest: "e2e/pod/servers/jss/package-lock.json",
+        vulnerabilities: [advisory("GHSA-a")],
+      }),
+      change("removed", "b", "1.0.0", { vulnerabilities: [advisory("GHSA-a")] }),
+      change("added", "a", "1.0.0", { vulnerabilities: [advisory("GHSA-a")] }),
+    ]);
+    expect(introduced).toHaveLength(1);
+    expect(carried).toEqual([]);
+  });
+
+  it("introduces the advisory of a package the pull request adds", () => {
+    expect(
+      advisories([change("added", "new", "1.0.0", { vulnerabilities: [advisory("GHSA-a")] })])
+        .introduced,
+    ).toHaveLength(1);
+  });
+
+  it("leaves out the advisories below moderate, and the versions without any", () => {
+    expect(
+      advisories([
+        change("added", "a", "1.0.0", { vulnerabilities: [advisory("GHSA-a", "low")] }),
+        change("added", "b", "1.0.0"),
+      ]),
+    ).toEqual({ introduced: [], carried: [] });
+  });
+
+  it("counts every severity from moderate up", () => {
+    expect(
+      advisories([
+        change("added", "a", "1.0.0", {
+          vulnerabilities: [advisory("GHSA-a", "critical"), advisory("GHSA-b", "high")],
+        }),
+      ]).introduced,
+    ).toHaveLength(2);
+  });
+});
+
+describe("renderAdvisories", () => {
+  it("lists the introduced advisories and those carried over, in a table each", () => {
+    expect(
+      renderAdvisories([
+        change("removed", "a", "1.0.0", { vulnerabilities: [advisory("GHSA-a")] }),
+        change("added", "a", "2.0.0", {
+          vulnerabilities: [advisory("GHSA-a"), advisory("GHSA-b", "high")],
+        }),
+      ]),
+    ).toBe(
+      [
+        "## Advisories",
+        "",
+        "1 introduced, 1 carried over from the base, of moderate severity or worse. The review fails on the introduced ones.",
+        "",
+        "### Introduced",
+        "",
+        "| Manifest | Package | Advisory | Severity | Summary |",
+        "|---|---|---|---|---|",
+        "| `package-lock.json` | `a` 2.0.0 | [GHSA-b](https://github.com/advisories/GHSA-b) | high | GHSA-b: a \\| in the summary |",
+        "",
+        "### Carried over from the base",
+        "",
+        "| Manifest | Package | Advisory | Severity | Summary |",
+        "|---|---|---|---|---|",
+        "| `package-lock.json` | `a` 2.0.0 | [GHSA-a](https://github.com/advisories/GHSA-a) | moderate | GHSA-a: a \\| in the summary |",
+        "",
+      ].join("\n"),
+    );
+  });
+
+  it("leaves out a table with nothing in it", () => {
+    const text = renderAdvisories([
+      change("added", "a", "1.0.0", { vulnerabilities: [advisory("GHSA-a")] }),
+    ]);
+    expect(text).toContain("### Introduced");
+    expect(text).not.toContain("### Carried over from the base");
+  });
+
+  it("says so when there are none", () => {
+    expect(renderAdvisories(CHANGES)).toBe(NO_ADVISORIES);
+  });
+});
+
 describe("run", () => {
-  function file(): string {
+  function file(list: readonly Change[] = CHANGES): string {
     const path = join(mkdtempSync(join(tmpdir(), "solid-memo-dependencies-")), "changes.json");
-    writeFileSync(path, JSON.stringify([CHANGES]));
+    writeFileSync(path, JSON.stringify([list]));
     return path;
   }
 
-  it("adds the tables to the run's summary", () => {
+  it("adds the advisories and the tables to the run's summary", () => {
     const summary = join(mkdtempSync(join(tmpdir(), "solid-memo-summary-")), "summary.md");
     writeFileSync(summary, "before\n");
-    run({ argv: ["node", file()], env: { GITHUB_STEP_SUMMARY: summary } });
-    expect(readFileSync(summary, "utf8")).toBe(`before\n${TABLES}`);
+    const process: Parameters<typeof run>[0] = {
+      argv: ["node", file()],
+      env: { GITHUB_STEP_SUMMARY: summary },
+    };
+    run(process);
+    expect(readFileSync(summary, "utf8")).toBe(`before\n${NO_ADVISORIES}\n${TABLES}`);
+    expect(process.exitCode).toBeUndefined();
   });
 
   it("prints them without a summary", () => {
     const log = vi.spyOn(console, "log").mockImplementation(() => {});
     run({ argv: ["node", file()], env: {} });
-    expect(log).toHaveBeenCalledWith(TABLES);
+    expect(log).toHaveBeenCalledWith(`${NO_ADVISORIES}\n${TABLES}`);
+    log.mockRestore();
+  });
+
+  it("passes on an advisory carried over from the base", () => {
+    const log = vi.spyOn(console, "log").mockImplementation(() => {});
+    const process: Parameters<typeof run>[0] = {
+      argv: [
+        "node",
+        file([
+          change("removed", "a", "1.0.0", { vulnerabilities: [advisory("GHSA-a")] }),
+          change("added", "a", "2.0.0", { vulnerabilities: [advisory("GHSA-a")] }),
+        ]),
+      ],
+      env: {},
+    };
+    run(process);
+    expect(process.exitCode).toBeUndefined();
+    log.mockRestore();
+  });
+
+  it("fails on an advisory the pull request introduces, and names it", () => {
+    const log = vi.spyOn(console, "log").mockImplementation(() => {});
+    const error = vi.spyOn(console, "error").mockImplementation(() => {});
+    const process: Parameters<typeof run>[0] = {
+      argv: [
+        "node",
+        file([change("added", "a", "2.0.0", { vulnerabilities: [advisory("GHSA-b")] })]),
+      ],
+      env: {},
+    };
+    run(process);
+    expect(process.exitCode).toBe(1);
+    expect(error).toHaveBeenCalledWith(
+      "package-lock.json » a@2.0.0: GHSA-b (moderate), which the base doesn't have: https://github.com/advisories/GHSA-b",
+    );
     log.mockRestore();
+    error.mockRestore();
   });
 
   it("needs the file", () => {
     expect(() => run({ argv: ["node"], env: {} })).toThrow(
-      "Usage: npm run dependencies:summary -- <changes.json>",
+      "Usage: npm run dependencies:review -- <changes.json>",
     );
   });
 });