From 09b7150e0af549b352cf6ac923db9335b4eacc2e Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Tue, 4 Aug 2026 12:15:12 -0500 Subject: [PATCH 1/2] ci(semgrep): the project SAST gate scanned two directories, not the project (BACKLOG #334) semgrep is a REQUIRED, BLOCKING merge context (.github/required-contexts.txt:78, asserted by test_security_posture.py's _BLOCKING_SECURITY_JOBS) and it scanned the allow-list `messagefoundry tee`. Its sibling bandit in the same file was moved off that exact shape deliberately and scans `-r .` minus an explicit --exclude set. 59 tracked .py files that bandit already covers -- messagefoundry_webconsole/ (33), scripts/ (24, the security tooling itself), docker/ (2) -- were held to none of this project's own dangerous-sink rules. SCOPE is now `.` minus bandit's exclude set, name-for-name. An allow-list cannot be kept in step with "the project" by hand; `.` minus explicit excludes cannot go stale when the next package is added. The `./` prefix bandit uses is dropped on purpose: semgrep matches --exclude as a GLOB, not a path, so `./tests` would match nothing and the flag would be inert. CLEAN AT THAT BAR, which this change must establish itself because --error is blocking. Verified by AST emulation of all five rules over the 339 in-scope files: 0 findings. The only two rule-matching sinks in the whole tree are under tests/ (an eval taint fixture at tests/fixtures/handler_taint/handler-security.py:47, a pickle.loads at tests/test_sandbox_codec.py:161), both excluded. NOT verified with real semgrep -- it has no supported Windows install, so the first CI run on the PR is the real check. TESTS. tests/test_lint_scope_parity.py gains the semgrep arm (targets + exclude parity with bandit). Beyond what the item asked for, three assertions cover ways this gate can go green while meaning nothing: - no --include. It NARROWS the scan to matching paths, so `--include messagefoundry` rebuilds the retired allow-list while the positional target still reads `.`. A targets-only check reports green on the exact regression this item exists to retire. - no `./` prefix on a semgrep --exclude. The set comparison normalises `./` off BOTH sides, so on its own it would call `./tests` and `tests` identical while one of them excludes nothing. - --error still present. Without it semgrep exits 0 on findings: a required context that prints every match and passes. test_security_posture.py's neutering scan cannot catch this -- it matches ADDED idioms (`|| true`, `--exit-zero`), never a REMOVED enforcement flag. All 13 falsifications go red, one or more per assertion: revert to `messagefoundry tee`; --include in both spellings; drop --error; copy bandit's ./ prefix verbatim; drop one exclude; add a bandit-only exclude; rename the step; retarget --config; mangle the --exclude flag name (non-vacuity); add a second positional; narrow the positional; drop every exclude. Baseline and restore both 8 passed. DOCS CORRECTED IN THE SAME COMMIT, because each describes this step and rested on its old state: - docs/adr/0034 residual row mitigated an unpinned pip bootstrap with "semgrep is not a required context (tests/test_required_contexts.py)". That is false in the repo's own records, and the cited file says the reverse of what it was cited for. A compensating control must not rest on a false premise (CLAUDE.md section 11) -- and this change increases what rides on that bootstrap rather than reducing it. Mitigation withdrawn, residual re-accepted on the uv row's grounds. - docs/Secure_Build_Scorecard_MEFOR.md:56 carried a nit that the .semgrep header still called semgrep "advisory". This change resolves it; the row now records the scope correction too. - .semgrep/messagefoundry.yml header said the rules run "advisory for one cycle, then flip to blocking". They are blocking. The replacement states that from two checkable facts rather than dating it: this file's history is squashed, so an "advisory until " claim is not verifiable from this repo. THE --exclude tests JUSTIFICATION WAS REWRITTEN rather than shipped as drafted. It claimed the exclude is load-bearing because "without it the widened scan reds on them". scripts/ci/assert_semgrep_handler_taint.py:41 -- run by the NEXT step of the SAME job -- records that semgrep's default .semgrepignore already excludes tests/, which is why that step copies its fixture to a temp dir to get it analysed at all. Both could not be true, and this lane cannot run semgrep to settle it. The exclude is kept for parity with bandit, which is the real and sufficient reason and the one the parity test enforces; the comment now says that instead of predicting a red it cannot verify. The glob-anchoring aside was re-measured over the right population: it was measured under messagefoundry/ and tee/ while the claim it supports is about the new repo-wide scope. Across the whole tracked tree the only nested directory matching any excluded name is packaging/messagefoundry-webconsole/tests, excluded explicitly on both sides anyway. .github/required-contexts.txt is deliberately NOT edited: the context NAME does not change, only its argv. --- .github/workflows/security.yml | 46 +++- .semgrep/messagefoundry.yml | 13 +- docs/Secure_Build_Scorecard_MEFOR.md | 2 +- ...is-triage-policy-accepted-risk-register.md | 2 +- tests/test_lint_scope_parity.py | 203 ++++++++++++++++++ 5 files changed, 258 insertions(+), 8 deletions(-) diff --git a/.github/workflows/security.yml b/.github/workflows/security.yml index 8e205483..8d98086d 100644 --- a/.github/workflows/security.yml +++ b/.github/workflows/security.yml @@ -408,9 +408,49 @@ jobs: # (1.172.0 is what `~=1.90` resolved to on 2026-07-29, so this pin is a no-op today.) python -m pip install --upgrade pip "semgrep==1.172.0" # --error: any finding fails the step — and the job, now that this gate is blocking. - # Includes `tee/` (in-tree vendored SOUP) so the standalone relay is held to the same - # dangerous-sink rules as the engine. - semgrep --config .semgrep --error --metrics off messagefoundry tee + # + # SCOPE = the whole repo minus the excludes below, which is the bandit exclude set in this + # same file EXACTLY. It was the allow-list `messagefoundry tee`, which never covered + # scripts/ (the security tooling itself), messagefoundry_webconsole/ (the operator UI) or + # docker/ — 59 tracked .py files that the sibling bandit gate already scans, held to none of + # this project's own dangerous-sink rules. An allow-list cannot be kept in step with "the + # project" by hand; `.` minus explicit excludes cannot go stale when a package is added. + # `tee/` (in-tree vendored SOUP) stays in scope for the reason it always was: the standalone + # relay is held to the SAME bar as the engine. Widening cost nothing — clean at this bar. + # tests/harness/samples — intentional non-production idioms. This exclude is here for + # PARITY WITH BANDIT — which is what the parity test enforces — + # NOT as a prediction about findings. semgrep's own default + # .semgrepignore already excludes tests/: that is the recorded + # fact scripts/ci/assert_semgrep_handler_taint.py:41 depends on, + # and the reason that step copies its fixture to a temp dir to + # get it analysed at all. So this flag is explicit and probably + # redundant, stated that way on purpose — a gate's scope must be + # readable from its own argv, never inherited from a tool default + # a version bump can change. (The only two sinks in the whole + # tree that match these rules do sit under tests/: an `eval` + # taint fixture and a `pickle.loads` sandbox test.) + # packaging/messagefoundry-webconsole/tests — the same, one directory deeper + # ide/ — TypeScript; no Python to scan + # docs/benchmarks/results — archived measurement artifacts, not maintained source + # The `./` prefix bandit uses is DROPPED on purpose: semgrep matches --exclude as a GLOB, + # not a path, so `./tests` matches nothing and the flag would be inert — copying bandit's + # string byte-for-byte yields an exclude list that excludes nothing. (Whether tests/ would + # then actually be SCANNED is the separate question the default .semgrepignore above + # answers; the point is that this gate's scope must not rest on that answer.) + # Whether those globs are root-anchored or match a bare directory NAME at any depth is not + # settled here, and is not load-bearing — measured over the WHOLE tracked tree, which is + # the population this scan now covers: the only nested directory matching any of these + # names is packaging/messagefoundry-webconsole/tests, excluded explicitly on both sides + # anyway (2026-08-04). + # tests/test_lint_scope_parity.py fails if this scope and bandit's drift apart again. It + # also asserts, separately, that no --exclude here carries the `./` prefix (its normalised + # set comparison would otherwise call `./tests` and `tests` identical), that no `--include` + # re-narrows the scan behind the positional `.`, and that `--error` survives. + semgrep --config .semgrep --error --metrics off \ + --exclude tests --exclude harness --exclude samples --exclude ide \ + --exclude docs/benchmarks/results --exclude packaging/messagefoundry-webconsole/tests \ + --exclude .venv --exclude node_modules \ + . - name: Handler-config taint rules — validate, regression gate, scan samples (ADR 0144 Inc 3) run: | # semgrep is already installed by the step above (same job). This is why the packaged rules diff --git a/.semgrep/messagefoundry.yml b/.semgrep/messagefoundry.yml index 253f4542..18cb456b 100644 --- a/.semgrep/messagefoundry.yml +++ b/.semgrep/messagefoundry.yml @@ -1,7 +1,14 @@ # MessageFoundry SAST rules — high-signal patterns for a PHI-handling HL7 engine. These complement -# bandit; each targets a sink that untrusted HL7/config data must never reach unguarded. All rules -# have ZERO matches in the current tree (verified at authoring), so the gate starts clean; the -# security.yml job runs them advisory for one cycle, then flips to blocking (CI-1). +# bandit; each targets a sink that untrusted HL7/config data must never reach unguarded. +# +# These rules are BLOCKING. The security.yml `semgrep` job runs them with --error, and that job is a +# required context (.github/required-contexts.txt:78, asserted by tests/test_security_posture.py's +# _BLOCKING_SECURITY_JOBS), so a single match fails the build. Stated from those two checkable facts +# rather than dated: this file's history was squashed, so "advisory until " is not something a +# reader can verify here, and the previous header's "runs them advisory for one cycle" outlived the +# flip it described. Scope is the WHOLE REPO minus that job's excludes — deliberately not an +# allow-list. This header deliberately makes no claim that the tree is currently clean: the gate is +# what keeps it clean, and the gate is the only thing that can still say so at a later date. rules: - id: mf-no-shell-true languages: [python] diff --git a/docs/Secure_Build_Scorecard_MEFOR.md b/docs/Secure_Build_Scorecard_MEFOR.md index d32d683b..298ccab9 100644 --- a/docs/Secure_Build_Scorecard_MEFOR.md +++ b/docs/Secure_Build_Scorecard_MEFOR.md @@ -53,7 +53,7 @@ Status tags use the built / designed-but-deferred / aspirational taxonomy (SDS |---|--------|--------|-------------------------------------| | 1 | Per-interface threat model & secure design | **Built — Strong** | `docs/security/THREAT-MODEL.md` (STRIDE-lite over 6 trust boundaries L16-30; 6 interface tables; risky-third-party 15.1.4 L90 + dangerous-functionality 15.1.5 L104). The ReDoS/single-parser guard is a real blocking pytest gate, not a doc claim: `tests/test_security_static.py` (`test_no_catastrophic_regex_in_source` / `test_single_json_parser` / `test_single_url_parser` / `test_xml_parsers_are_confined_to_the_allowlist` + planted-`(a+)+` meta-tests for the inline **and** the constant-passed shape). Scope corrected 2026-07-22: it is **not** repo-wide — the clauses walk `messagefoundry/`, `messagefoundry_webconsole/` and `harness/` (`_SOURCE_ROOTS`), deliberately excluding `tests/` and `scripts/`, whose own corpora and release tooling would trip the scanners on false positives; the per-clause root registry records why. Caveat resolved 2026-07-14: the STRIDE-table TLS "deferred" (L40, L74) intra-doc contradiction and the stale `console/service_control.py` path were corrected. The STRIDE rows now say built (matching the residual section + `api/tls.py`/`transports/mllp.py`) and the subprocess row points at `messagefoundry/service.py`. | | 2 | Secure coding practices enforced | **Built — Strong** *(grade holds; asserted evidence overstated)* | `parsing/peek.py` + `parsing/validate.py` (ingress validation), `store/store.py` (bound-parameter SQL; f-strings interpolate only literal column names per STORE-4), `api/security.py` (deny-by-default 401/403), `store/crypto.py` (AES-GCM) + `auth/passwords.py` (argon2id). Overstated: "PW.5 all Pass" is false. The cited `SDS-CONFORMANCE-REVIEW-2026-06-12.md` marks REST/SOAP (:74), file-handler (:75) and TLS-in-transit (:76) Partial, and that review is an explicit self-review, not independent. The four specific items credited (ingress / param-SQL / fail-closed authz / vetted crypto) are Pass. | -| 3 | Blocking security gates in CI, red-on-regression | **Built — Strong** | `.github/workflows/security.yml`: 7 jobs, no `continue-on-error`, none `if:`-gated, so all run on `pull_request`: pip-audit, npm-audit, bandit, gitleaks (`fetch-depth: 0`), `semgrep --error` (→ `.semgrep/messagefoundry.yml`, 5 dangerous-sink ERROR rules), crypto-inventory, forbidden-content. Honest residual verified: `continue-on-error: true` appears only on `sbom` (L104) + `trivy` (L137), both additionally cron/dispatch-only, never PR-gating. Nit: `.semgrep` header comment is stale, still calls semgrep "advisory". | +| 3 | Blocking security gates in CI, red-on-regression | **Built — Strong** | `.github/workflows/security.yml`: 7 jobs, no `continue-on-error`, none `if:`-gated, so all run on `pull_request`: pip-audit, npm-audit, bandit, gitleaks (`fetch-depth: 0`), `semgrep --error` (→ `.semgrep/messagefoundry.yml`, 5 dangerous-sink ERROR rules), crypto-inventory, forbidden-content. Honest residual verified: `continue-on-error: true` appears only on `sbom` (L104) + `trivy` (L137), both additionally cron/dispatch-only, never PR-gating. Nit resolved 2026-08-04 (BACKLOG #334): the `.semgrep` header no longer calls semgrep "advisory" — it now states the rules are blocking and names the required context. Scope corrected in the same change: `semgrep --error` scanned the two-directory allow-list `messagefoundry tee`, and now scans the whole repo minus bandit's exact `--exclude` set (59 more tracked `.py` files: `messagefoundry_webconsole/`, `scripts/`, `docker/`), pinned by `tests/test_lint_scope_parity.py`. | | 4 | Dependency & supply-chain integrity | **Built — Strong** (3 sub-items open) | `security.yml` pip-audit runs DEP-1 (`uv lock --check` + byte-diff + `pip install --require-hashes` + `pip-audit -r requirements.lock`), daily SCA cron; `dependabot.yml` (uv/actions/npm) + fail-closed published-GHSA auto-merge gate (`dependabot-auto-merge.yml`). `release.yml` tag-time blocking CycloneDX SBOM + Sigstore keyless + SLSA `attest-build-provenance` + PyPI Trusted Publishing/PEP 740. Open (`SDS-REMEDIATION-PLAN.md`): CI SBOM blocking (4.1/:63), signed-commit/DCO (4.3/:65), PS.3 build-input archival (:64). | | 5 | Secrets hygiene | **Built — Strong** | `security.yml` gitleaks full-history + forbidden-content, both blocking; `scripts/security/scan_forbidden.py` fail-closed (exit 1 on hit, exit 2 on a vacuous or under-loaded scan; customer-name / site-code / routable-IPv4 / worktree-slug / home-path detectors) shared by `.pre-commit-config.yaml` and the CI job, both scanning the WHOLE tracked tree. The token list is externalized -- never committed -- and CI asserts a per-section detector floor so a partial or mangled token source fails the build rather than passing green; `.claude/settings.json` deny-list on `.env*`/`secrets/**`/`*.key`/`*.pem`/`*.pfx`/`*.db`. Secrets are `MEFOR_*`-env-only (`config/settings.py`). | | 6 | Secure-by-default, fail-closed configuration | **Built — Strong** (at-rest fail-closed for every PHI posture) | `messagefoundry/__main__.py` loopback-only no-auth refusal (:914-920), `--allow-insecure-bind` that cannot relax the no-auth or Posture-B refusals (:104-111), off-loopback TLS-revocation refusal (:1285-1338), keyless-PHI refusal in every env (:989-1004); `config/settings.py` `require_mfa=True` (:1391). At-rest mechanism (verified 2026-07-14): `serve` requires an env (`__main__.py:938`) and `require_posture()` (`settings.py:1625`) refuses a keyless start for any unresolved/custom-env posture; `_KNOWN_ENV_POSTURE` (`settings.py:1560`) is `dev→SYNTHETIC`, `staging→PHI`, `prod→PHI`, so the keyless-PHI gate refuses every PHI posture. No PHI posture runs cleartext. Keyless `IdentityCipher` cleartext (`store/crypto.py:454`) is reachable only on a synthetic posture or the audited `allow_unencrypted_phi` opt-out; `require_encryption=False` (:301) is a stricter-still guard, not the switch. Residual (risk-accepted, register standing note): an operator could misuse a synthetic-declared env for real PHI. Like the `allow_unencrypted_phi` opt-out, that is an operator responsibility, not an engine gap. | diff --git a/docs/adr/0034-static-analysis-triage-policy-accepted-risk-register.md b/docs/adr/0034-static-analysis-triage-policy-accepted-risk-register.md index f4b1c6d4..bf2aab56 100644 --- a/docs/adr/0034-static-analysis-triage-policy-accepted-risk-register.md +++ b/docs/adr/0034-static-analysis-triage-policy-accepted-risk-register.md @@ -357,7 +357,7 @@ implied: |---|---| | **`release.yml`'s `sigstore==4.4.0`** | **Not a gap — an owner decision this change must not invert.** Routing `sigstore` through the lock **would** resolve it to **4.5.0** — stated counterfactually because that is what it is: `sigstore` is deliberately **absent** from `uv.lock` and from all six exports (0 hits), and the 4.5.0 figure was measured in a scratch resolve. A reader who greps `uv.lock`, finds nothing, and concludes this row is stale would re-open the very decision it exists to preserve. ADR 0034 pins 4.4.0 because `.github/dependabot.yml` sets `cooldown.default-days: 5` and 4.5.0 was <48 h old. Routing `sigstore` through the lock would pin the *signing* toolchain fresher than the repo's own update policy allows — inverting that policy at the highest-privilege point in the build (the job holding `contents`/`id-token`/`attestations: write`). Left exactly as it is. Re-evaluate when 4.5.0 ages out. | | **The `uv` bootstrap** (`security.yml`, `python -m pip install --upgrade pip "uv==0.12.0"`) | **Permanently circular: you cannot hash-lock `uv` with `uv`.** That install produces every lock this repo commits. `uv` stays an inline `==` pin, and `pip` remains the sole registered *name* in `SECURITY_YML_ACCEPTED_UNPINNED`. Note it is also the pip that runs the **six exports and the diff gate** — the `--require-hashes` install two steps later *downgrades* pip to the locked version afterwards, so the DEP-1 step's own posture is unchanged by this work. *Cheap out-of-band fix that removes it entirely:* `astral-sh/setup-uv@c771a70e…` is already SHA-pinned and used in 9 places (`ci.yml` ×6, `quality-advisory.yml` ×2, the resync ×1); swapping it in deletes the install. Separate change. | -| **`security.yml`'s unpinned `pip` in the `semgrep` step** — `python -m pip install --upgrade pip "semgrep==1.172.0"` | **The SECOND surviving bootstrap, named because an undercounted inventory is how a real finding goes invisible.** The semgrep row below explains only the `[otel]` conflict that keeps *semgrep* inline; this row records that the same line is also an **unpinned `pip` fetch**. So two `--upgrade pip` bootstraps remain in the file, not one — now asserted as an exact count by `test_security_yml_pip_bootstrap_count_is_exact`, since `SECURITY_YML_ACCEPTED_UNPINNED` registers the *name* `pip` and cannot tell two accepted bootstraps from twenty. Mitigating: `semgrep` is **not** a required context (`tests/test_required_contexts.py`), so this one does not sit on the merge path. It disappears whenever the semgrep row's `[tool.uv] conflicts` recipe is taken. | +| **`security.yml`'s unpinned `pip` in the `semgrep` step** — `python -m pip install --upgrade pip "semgrep==1.172.0"` | **The SECOND surviving bootstrap, named because an undercounted inventory is how a real finding goes invisible.** The semgrep row below explains only the `[otel]` conflict that keeps *semgrep* inline; this row records that the same line is also an **unpinned `pip` fetch**. So two `--upgrade pip` bootstraps remain in the file, not one — now asserted as an exact count by `test_security_yml_pip_bootstrap_count_is_exact`, since `SECURITY_YML_ACCEPTED_UNPINNED` registers the *name* `pip` and cannot tell two accepted bootstraps from twenty. **Mitigation WITHDRAWN 2026-08-04 (BACKLOG #334) — it rested on a false premise.** This row previously read *"Mitigating: `semgrep` is not a required context (`tests/test_required_contexts.py`), so this one does not sit on the merge path."* That is false in the repo's own records: `semgrep (project SAST rules)` is at `.github/required-contexts.txt:78`, and `tests/test_security_posture.py`'s `_BLOCKING_SECURITY_JOBS` names `semgrep` and asserts that membership. `tests/test_required_contexts.py` never claimed the opposite — it pins the required *set*, which contains it; the citation was to a file that says the reverse of what it was cited for. So this bootstrap **does** sit on the merge path, and #334 widened that same step's scan from a two-directory allow-list to the whole repo, which *increases* what rides on it. Re-accepted with that known, on the `uv` row's grounds (a bootstrap `pip` cannot hash-lock itself). It disappears whenever the semgrep row's `[tool.uv] conflicts` recipe is taken. | | **`quality-advisory.yml`'s `pipx install ruff`** | **Outside the guard's regex and outside Scorecard's.** `test_ci_venv_pinning.py`'s `_PIP_INSTALL` matches `pip`/`pip3`/`python -m pip` only, so the unpinned fallback branch is invisible to every existing guard — and because it is not a `pip install`, **no alert exists to close**. `pipx` has no `--require-hashes`, so fixing it means changing the install mechanism, not the pin. Recorded, not done. | | **`semgrep`** | **Excluded by decision.** `semgrep==1.172.0` requires `opentelemetry-sdk>=1.37,<1.38` while the project's `[otel]` extra resolves 1.44. In a plain group the universal resolve silently **downgrades the shipped otel runtime** in all four pre-existing DEP-1 artifacts — measured and bisected to semgrep alone (the other tools give DIFFS=0). The only fix is `[tool.uv] conflicts = [[{ extra = "otel" }, { group = "semgrep-tools" }]]`, which declares a **product extra** and a **CI scanner** permanently mutually exclusive (`uv sync --all-extras --all-groups` would stop working) and still forces a `click 8.4.1 → 8.4.2` re-resolve across all four artifacts. Pinning a *scanner*'s supply chain is not worth a lasting constraint on a shipped surface. The recipe is written down here so a future owner can flip it in one commit rather than re-deriving the analysis. | | **The 5 editable `pip install -e ".[…]"` sites** + 7 `uv pip install --system -e` sites | Structurally unhashable; §3's original rationale is correct for these and stands. | diff --git a/tests/test_lint_scope_parity.py b/tests/test_lint_scope_parity.py index d5626b93..fbb66e5a 100644 --- a/tests/test_lint_scope_parity.py +++ b/tests/test_lint_scope_parity.py @@ -16,11 +16,19 @@ These tests read both configurations and compare them, so the next person to narrow one has to narrow the other. They assert the CONTRACT, not any particular scope: widen or narrow freely, provided both sides move together. + +The semgrep arm below is CI-vs-CI, not hook-vs-CI, and that asymmetry is deliberate rather than a +forgotten half: there is no semgrep pre-commit hook to compare against, because semgrep has no +supported Windows install (docs/releases/BACKLOG-MULTISESSION-PLAN.md) and this is a Windows-first +project. So semgrep's scope is pinned to its sibling CI gate, bandit — the two scan the same +checkout in the same job file for the same reason, and a path excluded from one but not the other +means one of them is enforcing a standard the other is not. """ from __future__ import annotations import re +import shlex from pathlib import Path from typing import Any @@ -123,3 +131,198 @@ def test_ci_bandit_scans_the_repo_not_an_allow_list() -> None: assert re.search(r"bandit\s+-r\s+\.", ci), ( f"CI bandit must scan `-r .` with explicit --exclude, not an allow-list of dirs; got: {ci!r}" ) + + +def _ci_command(run: str, program: str) -> str: + """The single-line form of the `program ...` command inside a multi-line CI step body. + + Joins backslash continuations and DROPS comment lines, so every assertion below is made against + the argv that actually runs. That is load-bearing rather than tidiness: the first draft of the + exclude extractor ran its regex over the whole step body and duly reported an exclude named + ``matches``, mined out of the English sentence "semgrep's --exclude matches GLOBS" in the comment + above the command. Prose must never be able to change what a gate test measures. + """ + joined = run.replace("\\\n", " ") + line = next( + ( + ln + for ln in joined.splitlines() + if ln.strip().startswith(f"{program} ") and not ln.strip().startswith("#") + ), + None, + ) + assert line is not None, f"no `{program} ...` command found in the step body: {run!r}" + return line.strip() + + +def _semgrep_command() -> str: + """The REPO-WIDE semgrep command, from the step selected by its exact name. + + Two couplings a future editor should know about, both deliberate: + + 1. This selects by step NAME, so renaming "Run the MessageFoundry rules" breaks both semgrep + tests here. A loud failure is the point — the alternative is a test that silently starts + asserting about some other command. + 2. The semgrep job has TWO `run:` steps, and the second (ADR 0144 Inc 3) deliberately scans an + ALLOW-LIST (samples/config) with a DIFFERENT rules file. A loose fragment would match the + wrong invocation and assert the opposite of what is intended. Note that no step name in that + job contains the string "semgrep", so `_ci_step_run(_SECURITY, "semgrep")` raises rather than + quietly returning one of them. + + The `--config .semgrep` re-check guards (2): even if the name match were retargeted, a step + pointing at the packaged handler rules is not the gate these tests are about. It is checked + against the COMMAND, not the step body, so a passing mention of the rules dir in a comment + cannot satisfy it. + """ + command = _ci_command(_ci_step_run(_SECURITY, "Run the MessageFoundry rules"), "semgrep") + assert "--config .semgrep" in command, ( + "the repo-wide semgrep step no longer points at the .semgrep/ project rules, so these tests " + f"would be asserting scope against a different rule set entirely; got: {command!r}" + ) + return command + + +#: Long flags on the semgrep command that CONSUME the following token as their value. +#: +#: Hand-maintained, and that carries a real maintenance obligation: a NEW value-taking flag added to +#: the command without being added here turns its value into a phantom positional "target" and reds +#: ``test_ci_semgrep_scans_the_repo_not_an_allow_list`` — with a message about SCOPE, for a change +#: that had nothing to do with scope. The direction is deliberate (fail loud, make the next flag a +#: considered edit rather than a silent one), but read this set first when that test reds unexpectedly. +_SEMGREP_VALUE_FLAGS = frozenset({"--config", "--exclude", "--include", "--metrics"}) + + +def _semgrep_targets(command: str) -> list[str]: + """The POSITIONAL targets of the semgrep invocation — i.e. what it is pointed at. + + Deliberately not a regex on the command text: "does it end in a dot?" is a different question + from "what is this pointed at?", and only the second is the contract. shlex-splits, then drops + flags and the values consumed by them. + + NOT the whole scope on its own — see ``_semgrep_includes``. `--include` narrows what a positional + target expands to, so "targets" and "scope" coincide only while no `--include` is present, which + is why the test asserts both. + """ + targets: list[str] = [] + skip_next = False + for token in shlex.split(command)[1:]: # [1:] drops the `semgrep` program name + if skip_next: + skip_next = False + continue + if token.startswith("-"): + flag, sep, _value = token.partition("=") + skip_next = flag in _SEMGREP_VALUE_FLAGS and not sep + continue + targets.append(token) + return targets + + +def _semgrep_includes(command: str) -> set[str]: + """`--include` values, which NARROW the scan to paths matching them. + + Extracted separately because `--include` is the allow-list in disguise: `semgrep ... --include + messagefoundry --include tee .` scans exactly the two directories BACKLOG #334 exists to retire, + while the positional target still reads `.`. A targets-only assertion reports green on it. + """ + return {m.group(1).strip() for m in re.finditer(r"--include[= ]([^\s\\]+)", command)} + + +def _semgrep_excludes(command: str) -> set[str]: + """semgrep's excludes are a REPEATED flag taking one glob each; bandit's is one comma list. + + Returns the values VERBATIM. Normalising is left to the caller precisely because a normalisation + DIFFERENCE between the two gates is one of the things worth asserting — see the `./` check in + ``test_semgrep_and_bandit_exclude_the_same_paths``, which would be erased by normalising here. + """ + return {m.group(1).strip() for m in re.finditer(r"--exclude[= ]([^\s\\]+)", command)} + + +def test_ci_semgrep_scans_the_repo_not_an_allow_list() -> None: + """`messagefoundry tee` left scripts/ (the security tooling itself), messagefoundry_webconsole/ + and docker/ — 59 tracked .py files the sibling bandit gate already scans — covered by none of the + project's own dangerous-sink rules. Scanning `.` minus explicit excludes cannot go stale when the + next package is added.""" + command = _semgrep_command() + + targets = _semgrep_targets(command) + assert targets == ["."], ( + "CI semgrep must scan `.` with explicit --exclude, not an allow-list of dirs; it scans " + f"{targets!r}. An allow-list cannot be kept in step with 'the project' by hand — that is " + "precisely how the old `messagefoundry tee` scope came to miss scripts/." + ) + + # An `--include` re-narrows the scan WITHOUT touching the positional target, so the assertion + # above stays green through it. That is the whole regression this item exists to prevent, in the + # one shape a "what is it pointed at?" check cannot see — so it is asserted on its own terms. + includes = _semgrep_includes(command) + assert not includes, ( + f"CI semgrep is pointed at `.` but restricted with --include {sorted(includes)}. semgrep's " + "--include NARROWS the scan to matching paths, so this rebuilds the retired allow-list while " + "the positional target still reads `.` — the scope regression this test exists to catch, " + "wearing the argv of the fix." + ) + + +def test_ci_semgrep_still_fails_the_build_on_a_finding() -> None: + """Scope is only half the gate: semgrep exits 0 on findings unless `--error` is passed. + + Without it this job reports every match and then goes GREEN — a required context that cannot + fail, which is strictly worse than the narrow scope it replaced, because the narrow scope at + least still red on what it did see. `tests/test_security_posture.py`'s neutering scan cannot + catch this: it matches ADDED idioms (`|| true`, `--exit-zero`), never a REMOVED enforcement flag. + Widening the scan from 280 to 339 files is what makes this worth its own assertion. + """ + command = _semgrep_command() + assert "--error" in command, ( + "the repo-wide semgrep command dropped `--error`, so semgrep exits 0 on findings: every " + "match across the whole scanned tree would be printed and the required context would still " + f"report success. Got: {command!r}" + ) + + +def test_semgrep_and_bandit_exclude_the_same_paths() -> None: + """The two blocking SAST gates scan the same checkout, so they must agree on what is out of scope. + + Unlike the bandit hook-vs-CI test above, nothing is subtracted here: both sides are CI + invocations over the identical tree, so .venv/node_modules must appear on BOTH — there is no + pre-commit "these are untracked anyway" carve-out to grant. + + This does NOT assert that the two excludes have the same MEANING: bandit's are paths, semgrep's + are globs, and whether semgrep anchors them at the repo root is not settled by reading a string. + It asserts only that the two gates name the same set, which is the part that drifts — plus the + one normalisation difference that comparing normalised sets would otherwise hide. + """ + semgrep_cmd = _semgrep_command() + semgrep_raw = _semgrep_excludes(semgrep_cmd) + + bandit_cmd = _ci_command(_ci_step_run(_SECURITY, "Scan source for insecure patterns"), "bandit") + m = re.search(r"--exclude[= ]([^\s\\]+)", bandit_cmd) + assert m, f"CI bandit step has no --exclude: {bandit_cmd!r}" + bandit_raw = {p.strip() for p in m.group(1).split(",")} + + # Non-vacuity, BEFORE any comparison: two empty sets compare equal. An extractor that quietly + # stopped matching — a renamed flag, a reflowed line — would make this test report PASS while + # comparing nothing at all. Prove each instrument still sees something first. + assert semgrep_raw, f"the semgrep --exclude extractor matched nothing in: {semgrep_cmd!r}" + assert bandit_raw, f"the bandit --exclude extractor matched nothing in: {bandit_cmd!r}" + + # The set comparison below normalises `./` off BOTH sides, so on its own it would report parity + # for `./tests` vs `tests` — two strings that do NOT mean the same thing to the two tools. + # bandit's --exclude takes PATHS, where `./tests` is fine; semgrep's takes GLOBS, where `./tests` + # matches nothing and the flag is simply inert. So the most likely way to break this scope is to + # "fix" the drift by copying bandit's string byte-for-byte. Asserted before the normalisation. + dot_slash = sorted(p for p in semgrep_raw if p.startswith("./")) + assert not dot_slash, ( + f"semgrep --exclude values {dot_slash} carry bandit's `./` prefix. semgrep matches --exclude " + "as a GLOB, so `./tests` excludes nothing and the flag is inert — while the set comparison " + "in this same test normalises `./` away and would report the two gates in perfect parity." + ) + + semgrep_paths = {p.removeprefix("./").rstrip("/") for p in semgrep_raw} + bandit_paths = {p.removeprefix("./").rstrip("/") for p in bandit_raw} + + assert semgrep_paths == bandit_paths, ( + f"SAST scope drifted: semgrep excludes {sorted(semgrep_paths)}, bandit excludes " + f"{sorted(bandit_paths)}. A path excluded from ONE gate only is scanned by one and not the " + "other, which is the same class of silent divergence that left scripts/ out of CI bandit." + ) From 5fa21e74a9caf70a74c9de64588034905025462c Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Tue, 4 Aug 2026 12:16:40 -0500 Subject: [PATCH 2/2] docs(backlog): flip #334 to CLOSED now that semgrep scans the repo (BACKLOG #334) Banner-only, isolated from the code commit that precedes it (975e28c6). WHAT THE BANNER NOW RECORDS, beyond "done": the three things a reader of the body below would otherwise get wrong. - The body's "56 tracked .py files / 32 / 22 / 2" is a STALE MEASUREMENT, not a different scope. Re-measured 2026-08-04: 59 newly scanned (33 webconsole, 24 scripts, 2 docker), 339 in scope, up from 280. - Every security.yml:NNN anchor in the body has MOVED. The command was :413 and is now :449; the job still starts at :393. The body is left as the record of the problem as filed -- house rule for this lane is banner lines only. - The scan is clean by AST emulation of all five rules, NOT by a real semgrep run. semgrep has no supported Windows install, so the first CI run on the PR is the real check, and the banner says so rather than implying a verification that did not happen. CENSUS NOT RECOMPUTED. This commit flips ONE banner and touches nothing else in the file: the diff is a single line (3251), and it was verified BY NUMBER -- the nearest `## .` heading above it is `## 334.` at 3249 -- rather than by the banner's text, which a byte-identical banner pasted from another item would satisfy just as well. The ranked-table row for #334 (:208) and the four census distribution lines are deliberately untouched and are now stale with respect to this close. Note for whoever reconciles them: docs/releases/SCHEDULABLE-BACKLOG-MULTISESSION-PLAN.md was amended on 2026-08-04 by owner ruling (df9c4d54, which is on origin/main but not in this branch's base) to say the ranked table and census are recomputed by whoever writes the ledger last, from open-bannered headings -- no longer owner-only. That ruling postdates this lane's dispatch and its precondition is that the re-derivation happens as one operation with the stale-banner sweep, which is not this lane's scope. The status gate passes (278 items, one status each), but that is not the evidence for the paragraph above: it validates that a banner is present and self-consistent, never that it belongs to the item it sits under. --- docs/BACKLOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index b06921b7..c4066ef1 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -3146,7 +3146,7 @@ Honestly bounded: **this is build-time only.** No PHI path, no running-engine su ## 334. semgrep, a required blocking gate, scans a two-directory allow-list -> 🔢 **Filed 2026-08-01 — not started.** Value **5/10** · Difficulty **2/10** · _fill-in_. `security.yml:413` is still `semgrep --config .semgrep --error --metrics off messagefoundry tee` while bandit next door scans `-r .` at `:359`, and `tests/test_lint_scope_parity.py` — the control cited as stopping exactly this drift — mentions semgrep nowhere, so the project-specific rules directory silently skips the separately-versioned console wheel; bandit and CodeQL cover the sinks today, so this is breadth parity with a live compensating control, and the fix is one argument list mirroring bandit's `--exclude` plus one parity arm modelled on `:119-125`. +> ✅ **Status CLOSED (built 2026-08-04).** `semgrep --config .semgrep --error --metrics off messagefoundry tee` is now `semgrep --config .semgrep --error --metrics off --exclude … .` carrying bandit's exclude set from the same file, name-for-name, so the project's own dangerous-sink rules now cover **59** tracked `.py` files they never saw: `messagefoundry_webconsole/` (33), `scripts/` (24), `docker/` (2). **The body's "56 / 32 / 22" is a stale measurement, not a different scope** — re-measured 2026-08-04 at **339** in-scope files, up from 280. Clean at that bar (0 findings, AST emulation of all five rules); **not run with real semgrep, which has no supported Windows install** — the first CI run on the PR is the real check. `tests/test_lint_scope_parity.py` carries the parity arm the item asked for, plus three assertions it did not: no `--include` (it re-narrows the scan behind a positional `.`, so a targets-only check reads green on this very regression), no `./` prefix on a semgrep `--exclude` (a glob, not a path — and the set comparison normalises `./` off both sides), and `--error` still present (without it the widened gate prints every finding and exits 0). **Every `security.yml:NNN` anchor in the body below has moved** — the command is now at `:449`; the job still starts at `:393`. Two claims elsewhere rested on this item's old state and were corrected in the same commit: ADR 0034's residual row mitigated an unpinned `pip` bootstrap with *"semgrep is **not** a required context"* (it is — `.github/required-contexts.txt:78`), and `docs/Secure_Build_Scorecard_MEFOR.md:56` carried a now-resolved nit about the `.semgrep` header still calling the rules "advisory". **Cluster:** Security / CI gates. **Priority:** P2. **Verdict:** build. **Severity:** low.