From 73cc21b7d9017401ecc515a560e180b28d4b0693 Mon Sep 17 00:00:00 2001 From: bgard68 <30295154+bgard68@users.noreply.github.com> Date: Mon, 17 Aug 2026 08:44:38 -0500 Subject: [PATCH] docs: correct the container scan write-up and record the August findings Three documents described the pipeline as it was rather than as it is, and the one claim that was outright false had survived two rounds of changes to the thing it describes. security-remediation.md said under M10 that "Trivy reports rather than blocks", with the reasoning that base-image CVEs move on Microsoft's schedule so failing the build would train everyone to ignore a red X. That stopped being true on 2026-08-02, when the gate was turned on precisely because `ignore-unfixed` makes the argument collapse: Trivy drops what has no patch, so what it still reports is fixable by definition. The M10 entry now records that, plus the two defects found on 2026-08-17 - the severity filter that never applied, and the upload that was skipped exactly when it had something to upload - and the fact that digest pins need refreshing as routine maintenance, which is how CVE-2026-62901 sat unpatched. SECURITY.md said the scan blocks without saying on what. It now states the CRITICAL/HIGH scope, that lower severities still reach the Security tab, and why that scope is trustworthy: gate-probes.yml parses the workflow and rejects a gate built on a SARIF-format scan, where trivy-action silently ignores the severity filter. lessons.md and dependency-audit.md gain the August material, written as a troubleshooting order rather than a narrative, because the first check would have saved most of the time: a required check red on the base branch blocks every open pull request behind it. Then the two Dependabot failures that no rebase can fix - one bump changing five packages.lock.json files, and codeql-action sub-actions that must move in one commit - the nanoid advisory on the SPA, and the two Dependabot behaviours that mislead: `@dependabot rebase` reporting "already up-to-date" when the branch does not contain your base commits, and check results belonging to a commit rather than a branch, which is how a pull request can look one tick from mergeable while its branch would revert a security fix. check-docs-drift.sh gains "reports rather than blocks" as a known-false claim, so this particular drift cannot return. Negative-tested: passes as written, fails when the retracted sentence is reintroduced, passes again once removed. Co-Authored-By: Claude Opus 5 --- SECURITY.md | 13 +++ docs/development/dependency-audit.md | 107 ++++++++++++++++++++++- docs/development/security-remediation.md | 32 ++++++- docs/lessons.md | 54 ++++++++++++ scripts/check-docs-drift.sh | 2 + 5 files changed, 205 insertions(+), 3 deletions(-) diff --git a/SECURITY.md b/SECURITY.md index cf12cab..2e47fc2 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -76,8 +76,21 @@ install from a committed lock file, so what deploys is the tree that was reviewed. The deploy authenticates to Azure with OIDC — there is no stored publish profile or password. +The container scan blocks on **fixable CRITICAL and HIGH** findings. `ignore-unfixed` +drops anything without a patch, so what remains is actionable by definition. +Lower severities are still uploaded to the Security tab, where they can be seen +coming before they are worth failing a build over. + A separate job, `gate-probes.yml`, shows each of those gates something it must reject and fails if any of them does not. That exists because a check which cannot fail looks exactly like a check with nothing to report: the container scan ran for its entire life with `exit-code: 0`, reporting findings faithfully and blocking nothing, until 28 had accumulated behind a passing job. + +That probe is why the severity scope above is trustworthy. It parses +`container-build.yml` rather than grepping it, and fails if no Trivy step can +actually fail the job — including the two ways that are easy to miss: +`continue-on-error` with nothing checking the step's outcome, and a gate built on +a SARIF-format scan, where `trivy-action` silently ignores the severity filter +and blocks on everything. The second was a real defect, found on 2026-08-17 and +fixed the same day; the probe now refuses it. diff --git a/docs/development/dependency-audit.md b/docs/development/dependency-audit.md index 1f929aa..1dcc6ed 100644 --- a/docs/development/dependency-audit.md +++ b/docs/development/dependency-audit.md @@ -136,6 +136,111 @@ makes that guarantee explicit rather than incidental. --- -_See also: [Testing guide](testing.md) · [CI/CD pipeline testing](../deployment/pipeline.md)._ +## 5. Worked example — the stuck Dependabot queue (August 2026) + +Twenty-two Dependabot pull requests were open and red. None of them was individually wrong; they were +blocked by four separate causes, none of which a rebase could fix. It is worth reading as a +troubleshooting order, because the first check would have saved most of the time. + +### Check the base branch before any individual PR + +`Build API image & scan` is a required check, and it had gone red on `main` on a scheduled scan. A +required check failing on the base branch blocks **every** open pull request behind it. Nineteen of +the twenty-two were queued behind a failure none of them caused and none could clear. + +```bash +gh run list --workflow container-build.yml --branch main --limit 5 +``` + +The cause was **CVE-2026-62901** (HIGH, .NET denial of service) in runtime 10.0.10, inside the +digest-pinned `aspnet:10.0` base image. Microsoft had already published 10.0.11; nothing was +rebuilding to collect it. Digest pinning buys reproducibility and costs automatic patches, so the +pins need refreshing as routine maintenance: + +```bash +docker buildx imagetools inspect mcr.microsoft.com/dotnet/sdk:10.0 --format '{{.Manifest.Digest}}' +docker buildx imagetools inspect mcr.microsoft.com/dotnet/aspnet:10.0 --format '{{.Manifest.Digest}}' +``` + +> **Re-running the workflow does not help.** A re-run replays the original commit — same stale +> digests, same old workflow file. Only a new head commit picks up a fixed base. + +### `NU1004` — one bump, several lock files + +``` +error NU1004: The project references todoapp.application whose dependencies has changed. +The packages lock file is inconsistent with the project dependencies +so restore can't be run in locked mode. +``` + +Bumping `Microsoft.EntityFrameworkCore` in one project changes **every** `packages.lock.json` that +resolves it transitively — five of them here. Dependabot regenerates only the lock file belonging to +the project it edited, so the pull request is born failing and stays that way. Nothing about the base +branch is wrong, so rebasing changes nothing. + +Fix by hand, moving the whole family in one commit: + +```bash +dotnet restore TodoApp.sln --force-evaluate # regenerate every lock file +dotnet restore TodoApp.sln --locked-mode # prove the CI check now passes +dotnet build TodoApp.sln -c Release && dotnet test TodoApp.sln -c Release +``` + +### `codeql-action` sub-actions must move together + +``` +Loaded a configuration file for version '4.37.6', but running version '4.37.4' +``` + +`init`, `analyze` and `upload-sarif` must run the same version. Dependabot opens one pull request per +sub-action, so each one *creates* that mismatch on its own branch — whichever lands first breaks the +build. They cannot go green separately and have to move in one commit. Verify the tag SHA rather than +trusting the PR title; those said 4.37.6 while 4.37.7 was current. + +### The npm side — a real advisory, held open by nothing rebuilding + +`deploy.yml` gates the SPA on `npm audit --audit-level=high`, and it had been red since 8 August on: + +| Severity | Package | Advisory | Path | +| -------- | ------- | -------- | ---- | +| **High** | `nanoid` 3.3.16 | [GHSA-2v37-7h3g-55p8](https://github.com/advisories/GHSA-2v37-7h3g-55p8) — custom generators can loop indefinitely when size is zero | transitive via `postcss` 8.5.25 | + +Patched in **3.3.18**, which `postcss`'s `^3.3.16` range already accepts — so it is a lock-file-only +change with no `package.json` edit. Same shape as the base image: the fix existed, nothing had +rebuilt to collect it. + +> **Watch what `npm audit fix` touches.** It made the right `nanoid` change, but the npm in use +> (11.x on node 24) *also* stripped `libc` fields from six optional platform-specific packages — +> metadata selecting between glibc and musl builds of native binaries. Unrelated churn inside a +> security fix is how a platform-resolution bug arrives unnoticed. Check the diff; if it reaches +> beyond the advisory, patch the single entry by hand and verify the integrity hash against +> `registry.npmjs.org`. + +### Two Dependabot behaviours worth knowing + +- **`@dependabot rebase` can be a silent no-op.** It replies *"already up-to-date with ``"* + when its change still applies cleanly — which is **not** the same as containing your base commits. + Verify rather than believe it, and use `@dependabot recreate` to rebuild from the current base: + + ```bash + git fetch origin "+refs/pull//head:refs/remotes/pr/" + git merge-base --is-ancestor pr/ && echo "has the fix" || echo "does NOT" + ``` + +- **Check results belong to a commit, not a branch.** Merging a fix into the base does not turn a + pull request's stale red X green. Here that mattered: two branches still carried the vulnerable + `nanoid` 3.3.16 while displaying checks from a week earlier, so merging either would have + **reverted** the security fix. + +### The durable fix + +Both structural problems — lock files and the `codeql-action` trio — recur every time those packages +update. `groups` in `dependabot.yml` makes Dependabot open one pull request per group rather than one +per package, so related updates move together by default. + +--- + +_See also: [Testing guide](testing.md) · [CI/CD pipeline testing](../deployment/pipeline.md) · +[Lessons learned](../lessons.md#security-scanning-codeql-gitleaks--dependabot)._ > **← Back to the main [README](../../README.md).** diff --git a/docs/development/security-remediation.md b/docs/development/security-remediation.md index b75467a..5d93844 100644 --- a/docs/development/security-remediation.md +++ b/docs/development/security-remediation.md @@ -286,13 +286,41 @@ been: - scans with Trivy and uploads SARIF to the Security tab — closing the container-scanning gap listed as still-open under M6. -Trivy reports rather than blocks: base-image CVEs appear and are fixed on Microsoft's schedule, -not ours, so failing the build would just train everyone to ignore a red X. +**Trivy blocks, and has since 2026-08-02.** The original reasoning here was that base-image CVEs are +fixed on Microsoft's schedule rather than ours, so failing the build would only train everyone to +ignore a red X. That argument does not survive `ignore-unfixed: true`, which is set on the same step: +Trivy drops everything without a patch, so whatever it still reports is fixable by definition. The 28 +findings sitting in the Security tab were not waiting on Microsoft — Microsoft had already shipped, +and nothing was rebuilding to collect it. + +**The gate is CRITICAL/HIGH, and only became so on 2026-08-17.** `severity: CRITICAL,HIGH` had been +set on a step using `format: sarif`, and `trivy-action` ignores `severity` when it builds SARIF — it +logs `Building SARIF report with all severities` and scans everything. `exit-code: '1'` on that step +therefore failed the job on any fixable finding at any severity, including LOW and MEDIUM, while +every comment and input in the file said CRITICAL,HIGH. The gate now hangs off the table-format scan, +where the severity input is honoured; SARIF is a separate, non-blocking step that still uploads every +severity, so the Security tab keeps the full picture. + +**The scan no longer discards its own findings.** Failing the Trivy step skipped `upload-sarif`, so +the Security tab received results only on runs that had nothing to report — the findings were thrown +away in precisely the case they were collected for. A red run printed `exit code 1` and named no CVE. +The scan now runs with `continue-on-error`, the upload runs under `always()`, and a later step raises +the failure; blocking behaviour is unchanged. The table pass prints the CVEs and their fixed versions +into the log, so a failing run says out loud what it wants bumped. **Now closed.** Base images are pinned by digest, not tag — a tag is mutable, so the same Dockerfile could otherwise produce a different image tomorrow. The refresh command is in a comment above each `FROM`, and the container-build workflow proves the digests still resolve. +**Digest pins need refreshing on a schedule, and nothing does it automatically.** Pinning by digest +buys reproducibility at the cost of not receiving base-image patches. On 2026-08-17 the pinned +`aspnet:10.0` still carried **CVE-2026-62901** (HIGH, .NET denial of service) in runtime 10.0.10, +patched by Microsoft in 10.0.11; the weekly scheduled scan went red and, because the container job is +a required check, blocked all 22 open Dependabot pull requests behind a failure none of them caused. +The refresh is the same maintenance done on 2026-08-02 — read the current digests with +`docker buildx imagetools inspect mcr.microsoft.com/dotnet/aspnet:10.0 --format '{{.Manifest.Digest}}'` +and update both `FROM` lines. + --- ### M11 — Actions pinned to mutable tags diff --git a/docs/lessons.md b/docs/lessons.md index 7cbc87b..4c3c9aa 100644 --- a/docs/lessons.md +++ b/docs/lessons.md @@ -24,6 +24,11 @@ The ones that cost the most time — jump to the section for the full story: - **Drag-and-drop is dead on mobile** → [frontend notes](development/frontend-notes.md): the native HTML5 DnD API is touch-blind. - **Light mode is ignored in a phone browser** → [frontend notes](development/frontend-notes.md#darklight-mode--mobile-browsers-force-darken-a-light-only-page): mobile auto-dark force-darkens a light page; opt out with `color-scheme: only light` (the `only` keyword — `light dark` does not opt out). - **CodeQL flags a log line as "log forging"** → [Security scanning](#security-scanning-codeql-gitleaks--dependabot): user input (e.g. `Request.Path`) logged even via structured logging; strip `\r`/`\n` before logging. +- **Every Dependabot PR is red and none of them caused it** → [Security scanning](#security-scanning-codeql-gitleaks--dependabot): a required check failing on `main` blocks every open PR behind it; fix `main` first, then re-run. +- **A red CI gate names no CVE, just `exit code 1`** → [Security scanning](#security-scanning-codeql-gitleaks--dependabot): Trivy writing SARIF prints nothing readable, and failing the step skips the upload. +- **`severity: CRITICAL,HIGH` on a Trivy step does nothing** → [Security scanning](#security-scanning-codeql-gitleaks--dependabot): `trivy-action` ignores `severity` when `format: sarif`, so the gate blocks on every severity. +- **A NuGet bump fails with `NU1004` in CI but the PR looks fine** → [Security scanning](#security-scanning-codeql-gitleaks--dependabot): one bump changes several `packages.lock.json` files; Dependabot regenerates only one. +- **`@dependabot rebase` says "already up-to-date" but the branch is behind** → [Security scanning](#security-scanning-codeql-gitleaks--dependabot): that means "my change still applies", not "contains your base commits" — use `@dependabot recreate`. ## Database (SQLite vs Azure SQL) @@ -116,6 +121,55 @@ Making the repo public does **not** expose your Actions secrets — provided you GitHub-side protections (secret scanning, push protection, the `protect-main` ruleset) — lives in **[Secret hygiene](deployment/secret-hygiene.md)**. +### A gate that works but can't report reads as noise (August 2026) + +Twenty-two Dependabot PRs sat blocked for two weeks. The instinct was that CI had become noisy and the +old runs were clutter worth deleting. Deleting them would have destroyed the evidence and left two +live HIGH vulnerabilities in place. Both gates were working correctly the entire time; neither could +say what it had found. + +- **One red required check on `main` blocks every open PR.** `Build API image & scan` went red on a + scheduled scan, and because it is required, all 22 PRs queued behind a failure none of them caused + and none could clear. Check `main`'s own status before investigating any individual PR. +- **A failing Trivy step skips `upload-sarif`.** SARIF is machine-readable, so a red run printed + `exit code 1` and named no CVE — and because the step failed, the findings never reached the + Security tab. The tab received results *only* on runs with nothing to report. Fix: scan with + `continue-on-error`, upload under `always()`, raise the failure in a later step, and add a + `format: table` pass so the log names the CVE and its fixed version. +- **`trivy-action` ignores `severity` when `format: sarif`.** It logs `Building SARIF report with all + severities` and scans everything, so a gate built on that step blocks on LOW and MEDIUM while the + configuration claims CRITICAL,HIGH. Gate on the table scan, where the input is honoured, and keep + SARIF as a separate non-blocking reporter. +- **Digest-pinned base images do not receive patches.** Pinning buys reproducibility and costs you + automatic security updates. `aspnet:10.0` carried **CVE-2026-62901** (HIGH, .NET DoS) in runtime + 10.0.10 until the pins were refreshed to pick up 10.0.11. +- **Re-running a workflow replays the old commit.** It does not pick up a fixed base branch — the + re-run used the same stale digests and the same old workflow file. Only a new head commit helps. + +### Dependabot cannot express "these must land together" (August 2026) + +Two failure modes where the PR is born red and no amount of rebasing helps, because nothing about the +base branch is wrong: + +- **Lock files.** Bumping one package changes every `packages.lock.json` that resolves it + transitively — five of them here for a single `Microsoft.EntityFrameworkCore` bump. Dependabot + regenerates only the one belonging to the project it edited, so `dotnet restore --locked-mode` + fails with `NU1004`. Fix by hand: bump the family together, run + `dotnet restore TodoApp.sln --force-evaluate`, and commit every regenerated lock file. +- **`codeql-action` sub-actions.** `init`, `analyze` and `upload-sarif` must run the same version, or + the job fails with `Loaded a configuration file for version X, but running version Y`. Dependabot + opens one PR per sub-action, so each one *creates* that mismatch. They have to move in one commit. + +Both are avoidable with `groups` in `dependabot.yml`, which makes Dependabot open one PR per group. + +- **`@dependabot rebase` can be a silent no-op.** It replies *"already up-to-date with "* when + its change still applies cleanly — which is not the same as containing your base commits. Verify + with `git merge-base --is-ancestor ` rather than taking the reply at face + value; use `@dependabot recreate` to actually rebuild from the current base. +- **Check results belong to a commit, not a branch.** Merging a fix into the base does not turn a + PR's stale red X green, and the PR can keep showing a check from days earlier. That stale check is + also how a PR can look one green tick from mergeable while its branch would *revert* a security fix. + ## Config / secrets - Passwordless DB access uses a managed identity: enable system-assigned identity, then `CREATE USER ... FROM EXTERNAL PROVIDER` with `db_datareader` / `db_datawriter` / `db_ddladmin` roles. diff --git a/scripts/check-docs-drift.sh b/scripts/check-docs-drift.sh index 8836209..30e1e95 100644 --- a/scripts/check-docs-drift.sh +++ b/scripts/check-docs-drift.sh @@ -102,6 +102,8 @@ check_absent "double-submit companion cookie" \ "CSRF uses a double-submit cookie (replaced: the SPA cannot read an API-domain cookie)" || drift=1 check_absent "refresh token is persisted so a page reload" \ "the refresh token is persisted client-side (it is an httpOnly cookie)" || drift=1 +check_absent "reports rather than blocks" \ + "the container scan reports without blocking (it fails the build on fixable CRITICAL/HIGH)" || drift=1 [ "$drift" -eq 0 ] && ok "no known-false claims present" # ---- 3. Controls the docs promise must actually be wired ----------------------