--- name: fix-scan-finding description: >- Fix a CRITICAL Trivy finding that is failing CI in this repo (a vulnerability, misconfiguration, or secret from security-scan.yml or image-scan.yml), or add, review, or retire an entry in .trivyignore.yaml. Drives the decision the gate forces: fix it upstream, or accept it with an expiry. Read it before the first scanner command or security-motivated dependency bump, including on a gate that is already green. Trigger on "CVE", "trivy", "trivyignore", "code scanning", "scan finding", "vulnerability", "image scan", or a red Trivy check on a pull request. NOT for choosing what CI scans or for routine dependency bumps with no finding behind them. --- # Fixing a security-scan finding **Audience:** a contributor or agent working a CVE, Trivy, or code-scan finding in this repository -- fixing a red CRITICAL gate, or verifying a finding on a gate that is already green. No prior Trivy expertise assumed. **Prerequisites:** `trivy` on PATH (`brew install trivy`, or see the Trivy install docs), `bun` at the version in the root `package.json` `engines`, and Docker with `buildx` for image findings. No cloud credentials are needed -- everything here is local and read-only against public registries. ## What CI scans | Workflow | Job | Reads | |---|---|---| | `.github/workflows/security-scan.yml` | `Trivy filesystem scan (vulnerabilities)` | every lockfile in the tree | | `.github/workflows/security-scan.yml` | `Trivy config scan (Dockerfiles)` | the Dockerfiles only: Trivy's misconfiguration scanner does not read GitHub Actions workflows | | `.github/workflows/security-scan.yml` | `Trivy secret scan` | the working tree | | `.github/workflows/image-scan.yml` | `Trivy image scan (built from source, )` | the `Dockerfile` built from the PR, once per platform the release publishes (`linux/amd64`, `linux/arm64`) | | `.github/workflows/image-scan.yml` | `Trivy image scan (published latest, )` | `ms2data/malloy-publisher:latest` on both platforms, weekly schedule and dispatch | Each job uploads its full SARIF to Code Scanning, then runs a second scan scoped to CRITICAL with `exit-code: 1`. CRITICAL fails the job, and the secret gate fails on HIGH as well: a committed credential is an incident whatever its rating. GitHub's own "Trivy" Code Scanning check on a pull request is set, in the repository's code scanning settings, to fail only on critical alerts. It treats every alert in a file the PR touches as new, and a lockfile is one file, so at a high threshold every dependency PR went red on alerts already open on `main`. If that check goes red on high again, the setting was changed; that is the fix, not a lockfile edit. GitHub rates an alert by the SARIF's CVSS score, not Trivy's label, so the two can disagree: a Trivy MEDIUM can show as high, and a Trivy HIGH with CVSS 9.0 or more is critical to GitHub and still fails the check. HIGHs still land in Code Scanning, and one with a reachable fix is still worth fixing. Every job reads the same `.trivyignore.yaml`. On a pull request from a fork the SARIF upload is skipped (the token is read-only), but the gate still runs, so read the job log there. ## The rule **Prefer the upstream fix. Accept only what you cannot fix.** In order: 1. **Refresh the lockfile** if the manifest already allows a patched version. No ignore entry. 2. **Move the parent** that pins the vulnerable version, when a newer parent release in range fixes it. One move, N CVEs. 3. **Force the transitive version** with a root `package.json` `resolutions` entry only when 2 is genuinely blocked -- in range where possible, and only if it actually clears the finding (verify; see the traps below). 4. **Remove the component** from the image when nothing at runtime uses it. 5. **Accept with `expired_at`** when no reachable fix exists, with a `statement:` that is the review record. Do not skip to 5 because it is quick. Do not do 2 or 3 without the delta check in step B. ## A. Establish the real finding set Do not trust the GitHub Code Scanning alert list. GitHub maps Trivy's CVE severity onto its own scale, so genuine CRITICALs can appear there as `high`. Always scan locally with the gate's flags: ```bash trivy fs . --scanners vuln --severity CRITICAL \ --ignorefile .trivyignore.yaml \ --skip-dirs node_modules --skip-dirs .venv \ --skip-dirs '**/dist' --skip-dirs '**/build' \ --format json --quiet > /tmp/crit.json jq -r '.Results[]? | .Target as $t | (.Vulnerabilities // [])[] | "\($t)\t\(.PkgName)@\(.InstalledVersion)\t\(.VulnerabilityID)\tfix=\(.FixedVersion)"' \ /tmp/crit.json | sort -u ``` - **Quote the `**/...` globs.** Unquoted, zsh expands them, Trivy prints its usage text, and the run looks like a failure of the tree rather than of the command. - **Confirm the scan actually covered files.** A Report Summary with no rows, or JSON with no `Results` key at all (what Trivy emits for a directory with no lockfiles), means nothing was scanned and `exit=0` proves nothing. Running from the wrong directory is the usual cause. A gate that scanned nothing is the one failure mode that looks exactly like success. - **For image findings, build what CI builds.** CI builds and scans both `linux/amd64` and `linux/arm64`, with the DuckDB version derived from Malloy. A local build covers only the platform you ask for, and the per-architecture base images and prebuilt binaries differ enough that the two report different findings. Build the platform whose job is red (`linux/amd64` or `linux/arm64`), and pass `APT_REFRESH` as the CI builds do, or a stale cached `apt-get upgrade` layer can report Debian packages CI has already upgraded: ```bash docker buildx build --platform --load \ --build-arg DUCKDB_VERSION=$(node scripts/duckdb-version.js) \ --build-arg APT_REFRESH=$(date -u +%F) -t publisher:scan . trivy image publisher:scan --scanners vuln --severity CRITICAL --ignorefile .trivyignore.yaml ``` - **Run once more with `--include-dev-deps`.** Trivy skips devDependencies in Node lockfiles by default, and so does the gate. On this repo the flag has surfaced CRITICALs in dev tooling in the root `bun.lock` (`dompurify`, `ejs`, `shell-quote`) that the gate does not see. A finding that appears only with the flag is not gating, but it is real. Then group by root cause before touching anything -- see [references/root-cause-grouping.md](references/root-cause-grouping.md). Several CVEs against one lockfile are usually *one* lagging parent, not several pins. ## B. Before any version move: the delta check Never raise a pinned version or add a resolution without checking what the old version was protecting. **If a pin carries a comment, that comment is the spec** -- read it, then verify whether it still holds against the target version rather than assuming either way. 1. **Read the pin rationale.** A pinned version or `resolutions` entry often carries a reason in a nearby comment or in the commit that added it: `git log -S'"": ""' -- package.json`. 2. **Check the version line, not only the fixed version.** A fix published only on a new major is unreachable when every parent release still declares the old major. Check what the parent's latest release declares, and what the registry actually has: ```bash npm view dist-tags npm view @latest dependencies. ``` **Always check the latest public release before accepting a finding as unfixable.** The newest version on the registry may carry the fix even when the scanner's "Fixed Version" or your current major suggests otherwise, and a release may have shipped since the advisory was written. When the fix exists only on a release that is a breaking change (a new major, or a parent that has to move with it), do not decide alone: tell the user what the migration would involve -- which APIs or behavior change, which files and consumers are affected, whether the impact is runtime or build-time only, and what testing it needs -- and let them choose between migrating now and accepting with an expiry until the migration is scheduled. orval is the worked example: npm had 8.38.0 with the fix, but forcing it broke the only consumer, so the evidence went to the user as a choice rather than a silent accept. 3. **Test the change against the real consumer**, empirically. When a resolution moves a consumer across a major, run the consumer's own call against both versions in scratch installs and diff the output. The worked example is in [references/ecosystems.md](references/ecosystems.md) (one package on two majors). 4. **Look for held-back companions.** Grep for comments referencing the pinned version -- a package "held back to track X" must move with X or it is silently mismatched. 5. **Check what consumes the output** if the move could change behavior at a boundary: generated code, a wire format, a file another tool reads. A patch bump usually cannot; a major can. Then classify and act: - **No behavioral impact** -> apply, build, test, done. - **Impact found** -> state the specific impact, the blast radius, and the migration steps, and **get the user's approval before migrating**. Do not silently absorb a migration into a CVE fix. Present it as a choice (move and migrate now / review the diff first / investigate deeper / stay and accept) with the consequence of each. - **Build-time-only impact** (codegen, test tooling) still needs approval, but say so plainly -- it is a much smaller ask than a runtime change, and mislabeling it either way wastes the decision. ## C. Apply the fix Match the ecosystem; details and commands, including the Bun traps that make a targeted refresh rewrite half the tree, are in [references/ecosystems.md](references/ecosystems.md). Whatever you touch, the **stale-pin comment is part of the diff**. A pin comment that survives its own removal becomes a lie the next reader believes -- rewrite it to describe the new state and the invariant that still matters, not the change you made. Run builds, image builds, and test suites with `run_in_background: true`; an image build for a platform other than your machine's runs under emulation and takes minutes. ## D. Verify The test suite is the oracle for "did I break it"; the scanner is the oracle for "did I fix it". Both, in that order, and neither substitutes for the other. 1. **Re-run the step A scan and read what remains.** A move that clears four of five CVEs is a partial fix, and the fifth is the one that matters. 2. **Run what exercises the moved dependency:** `bun run test` for server dependencies; `bun install --frozen-lockfile && bun run generate-clients` in `packages/server/k6-tests` for its codegen; an image build of the red job's platform (step A) for anything in the `Dockerfile`. 3. **Do not trust a codegen exit code.** `bun run generate-clients` exits 0 even when it generates no files. Count the output files, and diff them against the previous version. 4. **For a Dockerfile change, confirm the component is gone from the built image,** for example `docker run --rm --entrypoint sh publisher:scan -c 'cd /publisher && bun why esbuild'`, then re-scan the image. ## E. If it cannot be fixed: accept it properly An acceptance has three parts, and skipping any one of them is how a suppression becomes permanent by accident: 1. **A path-scoped entry.** A bare `id:` suppresses the finding repo-wide, including in a lockfile added next month that inherits the same vulnerable dependency. `paths:` resolve against the scan root, which is the repository root: `packages/server/k6-tests/bun.lock`, not `k6-tests/bun.lock`. An entry with the wrong prefix parses fine and silently never matches. Verify the scoping holds: copy the affected `bun.lock` and `package.json` to a new directory, re-scan, and confirm the finding still fires there. Delete the copy. 2. **`expired_at`** on anything that will be fixed (`yyyy-mm-dd`; Trivy stops honouring the entry that day and the gate reds again). Omit it **only** for a genuine won't-fix, where an expiry would red the gate on a date certain for something nobody intends to change -- that is recurring noise, not a forcing function. A risk-accepted entry must name the compensating control, not assert safety. 3. **A `statement:`** that says the package and version, how it is reached, why the vulnerable path is not exercised here, and what upgrade retires the entry. The statement in `.trivyignore.yaml`, plus the PR description, is the review record -- an entry without one is an unexplained suppression. Follow the shape of the entries already in the file rather than inventing a new one. **OS-package (Debian) findings in an image scan cannot be path-scoped.** They carry no package path, so no `paths:` value matches them: tested on the built image with CVE-2019-16224 (`liblmdb0`), a bare `id:` suppressed it, while `paths: ["nonexistent"]`, `paths: ["usr/share/doc/liblmdb0/copyright"]`, and `paths: ["var/lib/dpkg/status"]` all left it firing. So an OS-package acceptance must be a bare `id:` entry. That is safe here because the filesystem scan never reports OS packages, so the entry cannot hide a lockfile finding; to make up for the missing scope, the `statement:` must name the Debian package and the image it was found in. Prefer fixing these in the `Dockerfile` first (see [references/ecosystems.md](references/ecosystems.md)): Trivy `Status: fixed` means `apt-get upgrade` clears it; `Status: affected` means Debian has no fix yet, so first ask what installed the package (`apt-cache rdepends --installed ` inside the image) -- removing an unused parent package is option 4 of the rule, and is how this repo's unfixed `liblmdb0` and `libxml2` findings were cleared rather than accepted. ## The traps - **A printed "Fixed Version" can be unreachable through the parent.** orval's 11 CRITICALs in `packages/server/k6-tests` are fixed only in 8.21.0+ (npm `latest` is 8.38.0), but `@grafana/openapi-to-k6` 0.3.2 and its latest, 0.4.1, both declare `orval ^7.5.0`. Forcing orval 8.38.0 makes `bun run generate-clients` fail under both ("Cannot read properties of undefined", no files written). So the finding is unfixable on this line, not un-upgraded, and was accepted with `expired_at`. Always check `npm view dist-tags` and test forcing the fixed version against the real consumer before accepting -- and before planning a move around it. - **Forcing a version can look like a fix and not be one.** A resolution to the newest 7.x of orval cleared the one finding 7.x fixes and left the 8.x-only ones standing. Always re-scan after a resolution; never assume the version you chose covers every CVE you targeted. - **`bun update ` adds it as a direct dependency.** Run against `basic-ftp` and `protobufjs`, it wrote both into the root `package.json` `dependencies` at their latest majors (6.2.1 and 8.8.0) instead of refreshing them in range. Use `resolutions` for a transitive package. - **Deleting a lockfile entry does not re-resolve just that entry.** Removing stale entries from `bun.lock` and running `bun install --lockfile-only` re-resolved most of the tree, moving unrelated packages such as the whole AWS SDK. That is not a targeted fix. - **Bun `resolutions` are top-level only.** A nested key such as `"snowflake-sdk/fast-xml-parser"` is accepted and silently ignored; the lockfile does not change. - **Build-time-only dependencies are still reported.** A CVE in codegen or test tooling (`packages/server/k6-tests`, `e2e`, `examples/data-app` each have their own lockfile) is not in the image. It still fails the filesystem gate, so fix it or accept it with that reasoning stated -- do not argue it away without an entry. - **A devDependencies-only CVE is invisible without `--include-dev-deps`.** Neither the gate nor the default local scan passes that flag. When sweeping one vulnerable chain repo-wide, re-run with it before concluding a lockfile is unaffected -- a clean default scan is not evidence the chain is absent, only that nothing in it is a production dependency. - **A clean `bun.lock` scan can still hide nested copies.** After this repo's lockfile refresh reported zero HIGH, the image scan of the same tree still found nested `lodash` 4.17.21, `ip-address` 10.0.1 and `ws` 5.2.4 that the filesystem scan never listed. Scan the built image before calling a Node finding gone, or list every lockfile key still at the vulnerable version. - **A production dependency's peers ship.** `@vitejs/plugin-react` in `packages/app` `dependencies` pulled `vite`, and through it `esbuild` (with a Go stdlib CVE in its binary), into the image despite `bun install --production`. `bun why ` inside the built image names the path. - **Your architecture is not the whole gate.** CI scans the image on both published platforms, and a local build scans only the one you built; the other can carry findings yours does not. Build the red job's `--platform` before concluding an image finding is gone. - **Yesterday's quiet CVE can red you today.** A newly published advisory against a version you already had will fail a gate that was green last week, with no change on your side. A red gate is not evidence your branch introduced the finding -- check the advisory date before hunting your diff. The weekly scheduled runs exist to surface exactly this. - **Unfixed criticals are not exempt.** The gates do not set `ignore-unfixed`, so a critical with no upstream patch blocks like any other. When there is nothing to upgrade to, the answer is an explicit path-scoped `.trivyignore.yaml` entry with a `statement:` -- a reviewed acceptance, not a silent one. - **Renaming a scan job changes its check name.** Status checks match by job name. If a maintainer has made a `Trivy ...` check required in branch protection, a rename leaves the old required check pending forever. Ask a maintainer before renaming one. ## Finishing State plainly which findings are **fixed** (with the version that fixed them), which are **accepted** (with the expiry and why no fix is reachable), and which are **still open**. If a change needed approval and you got it, say what was approved. Never report a finding as fixed on the strength of a scan that covered no targets. ### The handoff is not done until CI is green A green local run is necessary and not sufficient. A dependency move changes the resolved tree for every consumer, and CI is the only place the image is built and scanned on both published platforms the way the gate runs it, and the only place the full build and test matrix runs against the new lockfile. Local runs also use your machine's `node_modules` and Docker cache, which can hold artifacts CI resolves differently. So end every fix by handing the user the PR step and **waiting for CI**: 1. Ask the user whether they want you to push the branch and open a PR against `main`, or would rather do it themselves. Give the exact commands either way, for example: ```bash git add git commit -m "fix(deps): " git push -u origin gh pr create --repo malloydata/publisher --base main --title "" --body-file <file> gh pr checks <pr-number> --repo malloydata/publisher --watch ``` From a fork, push to the fork and open the PR against `malloydata/publisher` `main`. 2. Wait for the **`Trivy ...` checks** to pass on that PR. That is the gate the work exists to clear, and it re-runs against the PR head with `.trivyignore.yaml` as committed. 3. Wait for the **build and test jobs** to pass as well. They exercise the moved dependency; the scan does not. 4. If any of those go red, the fix is not done: read the failure, and treat a break in a *different* package as the expected shape of a shared-dependency regression rather than an unrelated flake. Report the fix as **verified locally, pending CI** until those checks report. Do not describe a dependency move as safe or complete on local evidence alone.