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 ----------------------