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/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. 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." + )