diff --git a/docs/ASVS-ASSESSMENT-METHOD.md b/docs/ASVS-ASSESSMENT-METHOD.md index 7c0e2bdd..a325fc04 100644 --- a/docs/ASVS-ASSESSMENT-METHOD.md +++ b/docs/ASVS-ASSESSMENT-METHOD.md @@ -60,7 +60,7 @@ These are the real disputes. They are here so the next assessor reaches the same | Cell | Verdict | Which rule, and why | |---|---|---| -| **5.4.3** | `fail` | Rule 3. A scan *hook* exists but its only shipped implementation is `_no_scan`, and there is **no configuration key at all** — an operator must **author** the scanner. Supplying a control is not configuring one. | +| **5.4.3** | `na` | **Rule 1, and it moved for the same reason 11.7.1 did.** Antivirus scanning of inbound content is an **enterprise-provided** control — the deploying organisation's AV/EDR/ICAP stack over the drop directory, the SFTP landing zone and the upload path — so the verb's subject is outside the declared scope of §2. ⚠️ Previously scored `fail` under rule 3 and cited here as the worked example of one, on the reasoning that *a scan hook exists but its only shipped implementation is `_no_scan` and there is no configuration key at all, so an operator must author the scanner*. **That reasoning is still true of the code** — it simply answers rule 3's question, and rule 1 runs first. ⛔ **This `na` is WEAKER than 11.7.1's and its rationale says so on the cell:** the engine *does* ship a scan seam, so this is a control the product **could** implement, which makes the verdict conditional on the enterprise actually covering those paths. It therefore carries a **deployment requirement**, and a consult to test that premise on outbound-initiated SFTP pulls is filed in the ledger. **CLOSED by owner decision (2026-08-02); do not re-derive it.** | | **15.2.5** | `partial` | Rule 5, **not** rule 3. `[sandbox].mode` ships `off`, but `subprocess` mode is real and was verified by executing it. A working control that ships off. | | **11.7.1** | `na` | **Rule 1 — the hardest call in this table, and it moved.** The verb is *"full memory encryption is in use"*: a property of the **CPU, firmware and hypervisor**, not of the three software artifacts in §2. Outside the declared scope, so rule 1 fires before rule 3 is ever reached. **The objection this has to answer, because it is a good one:** ADR 0152's rungs 1–2 *do* ship in-engine, so the engine is not silent on this cell. But that code **reports on and gates against** the platform property — it never provides it. Reporting is not implementing (§2's first guard). ⚠️ Previously scored `fail` under rule 3 and cited here as the worked example of one. That reading was not wrong on its own terms; it answered *"does code implement the verb"* without first asking *"is the verb's subject in scope"*, and rule 1 runs first. **This cell has moved four times in eighteen days — it is CLOSED by owner decision (2026-08-02); do not re-derive it.** ⛔ It buys **no** Level 3 claim: see §2.1. | | **3.7.3** | `fail` | Rule 3. One off-site navigation, a bare 303, no interstitial and no cancel. `oidc_enabled=False` removes the **trigger**, not a control. | @@ -135,6 +135,24 @@ level was achieved anyway. Any published attestation must say which requirements something weaker than "verified at Level 3". Writing `na` in the record and "Level 3 verified" in a brochure is the failure mode this section exists to prevent. +### 2.1a ⛔ The pinned corpus cannot settle a question about ASVS's *prose* + +**A corpus that cannot express a class of claim cannot refute one.** `[scorecard].corpus_sha256` pins +the ASVS **requirements** — `req_id`, text, level. It carries **no chapter prose**: no assessment +guidance, no scoping discussion, no definitions. So a claim about *what the standard says* outside a +requirement's own text is **structurally uncheckable** against it, and every check will come back clean. + +This is not hypothetical. A false statement — *"ASVS reserves non-applicable for functionality-based +exclusions"* — survived **two independent assessors** and reached a signed-adjacent risk-acceptance +block, because every one of them verified against the corpus and the corpus had nothing to say. What +`0x03` actually says is the opposite: it excludes requirements *"where configuration is outside the +application's responsibility."* One fetch of the chapter settled it; no amount of corpus checking could +have. + +**So: to cite ASVS prose, fetch the chapter at the `v5.0.0` tag and quote it verbatim.** Never +paraphrase it from memory, from an earlier assessment, or from another agent — and never treat a green +corpus check as evidence about a claim the corpus cannot represent. + ### 2.2 A count movement is not a posture movement — read the counts accordingly **The single most misreadable thing this method produces is a change in the bucket totals.** Counts move @@ -146,19 +164,36 @@ for four different reasons, and only one of them means the software got better: | A cell was **read for the first time** (`unverified` → anything) | **No** | The survey advanced. A cell moving `unverified` → `pass` is a *discovery*, not an improvement | | A **scope boundary was stated** (→ `na`) | **No** | The requirement left the denominator. Identical code, smaller question | | A **rule was applied more carefully** (re-grade in either direction) | **No** | The assessment got more accurate. Some of these move *down* | +| **The standard moved** (a new ASVS release changes requirement text, levels, or the requirement count) | **No** | The denominator changed. Zero code changed and zero assessment work happened — and this is the cause most easily mistaken for the survey advancing | **The worked example, and it is recent.** On 2026-08-02 the fail count went **3 → 2** when 11.7.1 was scoped out under rule 1. **Zero lines of engine code changed.** A reader comparing only the fail counts across those two days would conclude a defect was fixed; nothing was. The rationale is on the cell and the boundary is in §2, but neither is visible to someone reading a total. -**Two obligations follow, and they are cheap:** +**⛔ And the converse is true, and worse: a posture movement with NO count movement.** Everything above +teaches you to interrogate a number that *moved*. The more dangerous class **hides in stillness** — code +changes, an evidence anchor drifts off the line it was pinned to, and the recorded verdict quietly stops +describing the code. **The total does not move at all.** Stability reads as "nothing to see." + +This is not hypothetical and it is not cause 4. Cause 4 is a deliberate act — someone re-read a cell and +graded it better. This is an evidence pointer breaking **on its own**, while every reader sees a total +that looks fine. Measured on this project on 2026-08-02: **seven anchors across six cells had drifted**, +and not one bucket total moved. Nobody could have caught it from a number. You catch it **only** if a +gate is watching, which is the entire reason the drift gate exists (ADR 0156). + +**Three obligations follow, and they are cheap:** -1. **Never report a bucket total as a trend without naming which of the four causes moved it.** "Fails - went from 3 to 2" is not a finding. "One cell was scoped out; no code changed" is. +1. **Never report a bucket total as a trend without naming which cause moved it.** "Fails went from 3 + to 2" is not a finding. "One cell was scoped out; no code changed" is. 2. **When a count improves, state what would have had to happen for it to mean an improvement, and - whether that happened.** This is the same discipline as a negative control: a number that can only - go one way is not measuring anything. + whether that happened.** Same discipline as a negative control: a number that can only go one way is + not measuring anything. +3. **A stable count is not evidence of a stable posture unless the anchors were re-verified in the same + pass.** Report the drift check alongside the total, or you are publishing the freshness of the last + check rather than of the software. ⚠️ **Always state the pinned ASVS version with any total**, so a + denominator change shows up as a version change instead of as progress — the corpus is pinned by + digest in `[scorecard]`, and a 5.0.x patch release would move requirement text and counts on its own. ⚠️ **This cuts against us more often than for us, which is why it is written down.** The survey is incomplete, so most future movement will come from cause 2 — cells being read for the first time — and diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index 6c5b1fb2..f6d7f8b1 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -412,7 +412,7 @@ Ordered by value descending, then difficulty ascending (cheapest first at equal | **#24 DICOM** | **Med–High** | **done (Phases 1 + 2)** | L | **Adopter-driven** — a radiology practice on Corepoint DICOM Gear wants to adopt. ✅ **Phases 1 + 2 SHIPPED** ([ADR 0025](adr/0025-dicom-codec-store-connectors.md) Accepted): pure codec + DIMSE **C-STORE SCP** + code-first SR→HL7 Handler (Phase 1, PR #439); **C-STORE SCU + C-ECHO + DICOMweb STOW-RS** outbound (Phase 2, `rest.py` reuse — no new dep). **MWL/Q-R/inbound-DICOMweb declined/deferred.** Did **not** need #7. | | **Meta — v0.3 cut** | Low | **done** | S | ✅ The v0.3-candidate wave was cut as **`0.2.10`** (Plan-5; ADR 0023 inbound-HTTP among others). Next buildable set planned in PLAN-6. | | **#30 version-update check** | Low | **done** | M | ✅ Shipped `0.2.10` (ADR 0026, PR #618) — but as a **zero-egress local lock-diff** (no PyPI call), resolving the on-prem tension; the live-egress variant stays off-by-default / deferred. | -| **ASVS 11.7.1 in-use memory encryption** | Low | **drop (N/A)** | XL | Already scored **N/A** — a hardware/OS/hypervisor capability, unachievable for pure-Python on-prem. | +| **ASVS 11.7.1 in-use memory encryption** | Low | **drop (N/A)** | XL | **`na` on the record** (closed by owner decision 2026-08-02; do not re-score). ⚠️ The verdict is right but the *reason* here is not: "unachievable for pure-Python on-prem" is **not** the ground. The engine does ship rungs 1–2 — they **report on** the platform property rather than **provide** it. The ground is that the verb names a CPU/firmware/hypervisor property, outside the declared scope of three software artifacts. See `docs/ASVS-ASSESSMENT-METHOD.md` §2. | | **#18 git-offering** | Low | **confirm-decline** | M | Buyers already run git/ADO/GHE — a non-problem. Fold conventions into #33; AGPL-compat entanglement. | | **#25 JMS** | Low | **confirm-decline** (as named) | M | Java-broker artifact vs the **no-broker identity** (the staged SQLite queue *is* the durability story). Keep only a *generic* AMQP/Kafka on-trigger candidate. | | **#26 visual/template authoring** | Low | **confirm-decline** | S | Code-first IS the differentiator (recorded #411). The failure mode is a "guided editor" drifting toward declarative *logic* authoring. | @@ -426,7 +426,7 @@ Ordered by value descending, then difficulty ascending (cheapest first at equal **Top strategic calls** *(2026-06-19; updated 2026-06-28)*: 1. ✅ **v0.2 locked and shipped** through `0.2.10` (#28/#29 evidence published; #22b shipped). The release-close move is done. 2. **The connector + codec backlog largely shipped, on its triggers** — #7 (first slice) / #23 (SMTP) / #24 / #31 / #32 all landed; #25 stays declined. The discipline held: each shipped against a real adopter/contract or as an additive opt-in, never speculative. The remaining transport tails (#23 IMAP/POP, #7 SOAP-reply) stay demand-gated. -3. **Treat the ASVS L3 residuals as closed/N-A, not a staffing queue** — #377 + #378 merged; 11.7.1 is N/A; WebAuthn #11 buys zero ASVS movement. The only live security item is the cheap least-priv default flip. +3. **Treat the ASVS L3 residuals as closed/N-A, not a staffing queue** — #377 + #378 merged; 11.7.1 is N/A (still true, and re-confirmed on the record 2026-08-02 — but on a different ground than this line assumed; see the 11.7.1 row above); WebAuthn #11 buys zero ASVS movement. The only live security item is the cheap least-priv default flip. ⚠️ **Do not read "residuals are closed" as a posture summary** — the survey is incomplete and most cells have never been read against the requirement text, so this line describes a *staffing* judgement, not coverage. 4. ✅ **The v0.3-candidate set was cut as `0.2.10`** (anchored on ADR 0023 inbound-HTTP). The next buildable set is the actionable **#33** + the **#40** AWS campaigns + owner-decision **#60** — planned in PLAN-6, still ADRs-first and demand-aware. (**#41** shipped as ADR 0047; **#61** as ADR 0048 / #641; **#52** is the parity index → #65–#85; **#39** was built then 🪦 retired 2026-07-01.) 5. **Re-confirm the #26 visual-authoring decline loudly** — the strategic failure mode is an audit or "guided editor" quietly drifting toward declarative *logic* authoring. @@ -6387,7 +6387,7 @@ Two findings are worth surfacing here. **Posture B scores worse on Fails than Po **Build history.** *Partial build (PLAN-9 Wave 1, 2026-07-10 — branch `plan9-secmem`):* best-effort `mlock`/`VirtualLock` + `memset`-zeroize of the unwrapped DEK and the plaintext buffers the code owns landed in `store/crypto.py`; `mfenc:v1` ciphertext byte-identity is preserved and the public cipher seam is unchanged. *Close (2026-07-13):* the partial was verified complete against the full code-owned mutable-buffer surface (DEK + retired keys + `encrypt`/`decrypt` plaintext — no further mutable buffer remains to wipe), a full-path zeroize-verification test was added, and the residual disposition was documented and risk-accepted (see the banner). The residual is a documented *partial* of ASVS 13.3.3, not a technical close: CPython immutable `str`/`bytes` (caller plaintext, the returned marker, cryptography's `decrypt()` output) and cryptography's internal OpenSSL key copy are unreachable to wipe (documented in the module docstring), and **11.7.1** full in-use memory encryption is a host/hypervisor deployment requirement accepted via signed risk-acceptance, not code. -**Closes (ASVS 5.0 L3):** 11.7.1, 11.7.2, 13.3.3 · *(classes 2 and 4)* — **scope addressed, not verdict**: 13.3.3/11.7.1 remain **Fail** and 11.7.2 **Partial** (accepted with a deployment requirement + signed risk-acceptance, see banner), not passing. +**Closes (ASVS 5.0 L3):** 11.7.1, 11.7.2, 13.3.3 · *(classes 2 and 4)* — **scope addressed, not verdict**: 11.7.2 is **Partial** (accepted with a deployment requirement + signed risk-acceptance, see banner), not passing. ⚠️ **Verdicts corrected 2026-08-02 — do not read the original clause as current.** It said *"13.3.3/11.7.1 remain **Fail**"*. Neither is a Fail on the record: **11.7.1 is `na`** (closed by owner decision, out of declared scope — a CPU/firmware property, not one of the three assessed software artifacts), and **13.3.3 is `unverified`** — never read against the requirement text, which is explicitly **not** a verdict of any kind. The verdict of record is the scorecard, never this ledger; take any current figure from there. **Scope:** Add zeroization of plaintext PHI and key material after use, and mlock-style anti-swap protection where the platform allows. Decide the disposition of full memory encryption (TME/SGX/SEV, confidential VM) — enforce as a deployment requirement, or accept and document. @@ -8509,6 +8509,65 @@ The three surfaces now share **one** liveness helper, because they had been disa --- +## 353. Gate the risk-acceptance register against the scorecard: nothing compares its cell lists to the record + +> 🚧 **Status OPEN (filed 2026-08-02).** The ASVS risk-acceptance register is **ungated prose**. No CI check has ever compared the cell ids in its signed sign-off blocks against the verdict of record, and a manual cross-check found the lists had drifted substantially with **zero** alarm. + +**Cluster:** Security & Compliance. **Priority:** P2. **Verdict:** file now, build on owner green-light. **Severity:** medium-high — the artifact that records *accepted risk* can disagree with the artifact that records *what the risks are*, indefinitely and silently. + +**The finding that motivates it.** Asked to fix one sign-off block, a cross-check of **all eight** against `asvs-scorecard.toml` returned **29 entries that are not carried residuals**, in three classes: + +| Class | Count | Why it is wrong | +|---|---:|---| +| `unverified` | 22 | ⛔ **A signed acceptance of a risk that was never assessed.** The cell has never been read against the ASVS requirement text at any commit. Present in **every one of the eight blocks**. | +| `na` | 3 | Out of declared scope — there is no residual to carry. | +| `pass` | 4 | The cell passes — there is no residual to accept. | + +⚠️ **Those counts are ONE measurement by ONE session and are not independently confirmed.** They should not be load-bearing for any decision until a second implementation reproduces them — **which is exactly what this gate would be.** Treat the number as the reason to build the check, never as an established fact. *(Caveat raised by the coordinator session, and it is the right one.)* + +**What the check is.** Roughly fifteen lines, stdlib only: `tomllib`-load the scorecard, regex the register's sign-off table rows, and for every cell id in every block compare against the record. Fail on `unverified`, `na` or `pass` appearing in an acceptance list. It needs no new dependency and runs in milliseconds. + +**Design notes, because a gate written carelessly here would be worse than none:** + +- **Print what it scanned.** Block count, row count, ids-per-block. A gate that finds nothing because its regex stopped matching the table is indistinguishable from a clean one — that failure mode has already fired twice on this project. +- **Prove it can go red before trusting a green.** Plant a known-bad id in a fixture and assert the check rejects it, and plant a clean fixture and assert it passes. Both directions, or the check is measuring nothing. +- **Do not auto-correct.** The gate must **report**, never rewrite. The cell lists sit inside *signed* acceptances, and silently editing signed content to satisfy a checker is a worse defect than the drift it fixes. +- **The register lives in the vault**, so this belongs with the existing `asvs-scorecard.yml` workflow rather than in the public engine repo. + +**⛔ Not to be built without the owner's go-ahead.** A new security-doc gate can block merges, and that is the owner's decision like any other enforcing control. This item exists so the finding is durable, not to authorise the build. + +**Related:** ADR 0156 (scorecard as data — the record this would check against); the §2 banner in the register recording the same finding; and the standing question this does **not** answer, which is the owner's alone: *what the 2026-07-14 signature actually covered*, given 22 of its ids had never been examined on that date either. + +--- + +## 352. Consult on enterprise AV coverage for SFTP- and file-connector ingest from outside the domain (ASVS 5.4.3 premise check) + +> 🚧 **Status OPEN (filed 2026-08-02).** ASVS **5.4.3** was recorded `na` on 2026-08-02 on the ground that antivirus scanning is an **enterprise-provided** control. This item exists to *test that premise* against the one ingest path most likely to fall outside it, rather than assume it. + +**Cluster:** Security & Compliance. **Priority:** P2. **Verdict:** consult, then decide. **Severity:** medium — the verdict of a closed cell rests on the answer. + +**The question, for Gabe (enterprise security):** how does the enterprise scanning stack handle files that MessageFoundry *collects* rather than receives — specifically the **SFTP/FTPS remote-file source** and the **file connector** — when the origin is **outside the organisation's domain**? + +The distinction matters because these two paths do not look like the case AV coverage is usually designed around: + +- **The engine pulls, the perimeter doesn't see a delivery.** A gateway or mail-path scanner inspects content arriving *at* the enterprise. `RemoteFileSource` reaches *out* to a partner's SFTP/FTPS server and retrieves bytes over an encrypted session, landing them straight in the engine's working area. There is no inbound delivery event for a perimeter scanner to act on. +- **On-access scanning depends on where the file lands.** If the drop directory is on a host and volume the EDR agent actually watches, on-access scanning may cover it. If it is a network share, a container volume, or a path excluded for performance (integration hosts frequently are), it may not. +- **The origin is a partner, not the enterprise.** These feeds come from outside the domain by definition, so "internal traffic is trusted" does not apply. + +**What we need out of the conversation, stated as answers not opinions:** +1. Is content retrieved by an outbound-initiated SFTP/FTPS pull scanned at all — and by what, at what point? +2. Are integration hosts' drop/working directories inside on-access scanning, or excluded? +3. What happens on detection — quarantine, delete, alert-only — and does MessageFoundry learn about it, or does the file simply vanish underneath a running connector? +4. Is there an ICAP or equivalent service endpoint the engine *could* call, if we later decide to make scanning a shipped, configurable control? + +**Why this is filed rather than assumed.** The `na` on 5.4.3 records its own three exposures, and the load-bearing one is that **the engine ships a scan seam** (`set_scan_hook` / `scan_inbound_file`, fail-closed on both axes when installed) — so unlike full memory encryption, this is a control the product *could* implement. The cell's ground therefore depends on the enterprise actually covering these paths. If the answer to (1) or (2) is "no", the deployment requirement attached to that `na` is not satisfied for this class of feed and the cell should be reopened by the owner. + +**Do not** treat this item's existence as reopening 5.4.3. That cell is closed by owner decision; only the owner reopens it, and only with an explicit instruction. + +**Related:** the scan seam at [`transports/file.py`](../messagefoundry/transports/file.py) (`set_scan_hook`, `scan_inbound_file`) and its remote sibling in [`transports/remotefile.py`](../messagefoundry/transports/remotefile.py); the deployment requirement recorded with the 5.4.3 ruling. + +--- + ## 347. A PHI-at-rest assertion that can pass for the wrong reason — short substring vs. random ciphertext > 🚧 **Status OPEN (filed 2026-08-02).** `tests/test_store_encryption.py:95` asserts `raw.startswith(MARKER_PREFIX) and "DOE" not in raw` — three characters of a 76-character body — as the proof that a patient surname is unreadable at rest. **The instrument is wrong in both directions.** It **fails when encryption worked perfectly** (the value is encrypted under `make_cipher(generate_key())`, a fresh random key every run, so the base64 body is fresh random text and base64's alphabet contains `D`, `O` and `E`), and — the half that matters — it would **PASS on a weak encoding that merely happened to avoid those three characters**. A test that can pass for the wrong reason is a false assurance about PHI; one that occasionally fails for the wrong reason is only noise. **The flake is what made someone look; it is not what is wrong.** diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index fdb709e4..c41c4efd 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -1473,6 +1473,9 @@ and a PHI weakening under **strict enforcement** (`enforcement = enforce`, the d | `allow_unverified_alert_smtp_tls` | bool | `false` | the **acknowledgment** required to start an enforcing PHI instance whose `[alerts]` SMTP hop does not authenticate the relay — i.e. `[alerts].email_use_tls = false` (cleartext) or `[alerts].email_tls_verify = false` (encrypted but accepts any certificate) ([#323](BACKLOG.md)). Covers BOTH shapes deliberately: cleartext is strictly worse than unauthenticated TLS, so gating only the second would hand an operator a bypass onto the worse posture. Without it `serve` refuses to start (exit 2); with it the start is permitted and named in a WARNING-level `AUDIT:` line. An **acknowledgment switch rather than the clamped `MEFOR_ALLOW_INSECURE_TLS` escape** the connectors use, because this cell is constructed outside the `active_hop_posture` scope where that clamp would be inert. A **loosening** — `security_loosenings()` reports it, so it is never silent | | `memory_encryption_operator_declared` | bool | `false` | **`[BUILT]` ([ADR 0152](adr/0152-in-use-data-protection-for-phi-platform-memory-encryption-attestation-asvs-11-7-1.md) rung 2, ASVS 11.7.1):** the operator's **declaration** that this host provides hardware memory encryption (AMD SEV-SNP / Intel TDX), so PHI is protected in RAM **while it is being processed**. The engine cannot verify it — a local CPU flag is emitted by the OS whose integrity the requirement protects against — so this records **who took responsibility**, the same discipline as `MEFOR_TLS_REVOCATION_ATTESTED`. It is deliberately **not** called "attested": in confidential computing that word means a CPU-signed quote verified against the silicon vendor's root PKI (ADR 0152 rung 3, **not built**). An **exposed** PHI instance without it **warns and starts** — on every environment, at both `enforcement` settings; it refuses only if `require_memory_encryption_declaration` is also set. A **positive platform read-out does not substitute for it** (a read-out must never relax a control). **Loopback and synthetic instances are byte-identical** (never consulted). If the platform read-out positively contradicts this, the contradiction is **warned at start and reported** as `memory_encryption_readout_contradicts_declaration` on `GET /security/posture` — but **never refused** (the read-out is a self-report, not evidence, and has known false negatives: driver not loaded, container without the device node mapped, Azure CVM paravisor). **Setting this does not make the instance ASVS 11.7.1-compliant** — see the read-out note below the table. Env: `MEFOR_SECURITY_MEMORY_ENCRYPTION_OPERATOR_DECLARED` | | `require_memory_encryption_declaration` | bool | `false` | **`[BUILT]` (ADR 0152 rung 2):** turn the row-12 warning above into a **refusal** — an **exposed** PHI instance with no `memory_encryption_operator_declared` then **refuses to start** under `enforcement=enforce` (and still warns under `warn`). **Opt-in by design, and the default is load-bearing:** the property is a **host** property that no operator can satisfy on Windows (the read-out is always `null` there), and "exposed" includes the recommended loopback-behind-proxy topology, so a refusal by default would stop working dev/staging/prod deployments from booting on upgrade over something they cannot change. Same scoping rule as `[security].allowed_client_networks`' companion refusal (ADR 0151): a new refusal fires only on a new opt-in. Set it in an estate that has standardized on confidential-computing hosts and wants a missing declaration to be fatal. Env: `MEFOR_SECURITY_REQUIRE_MEMORY_ENCRYPTION_DECLARATION` | +| `organization_domains` | list[str] | `[]` | **`[BUILT]` (ASVS 3.7.3):** domains that count as **inside** your organization. The console interposes a "you are leaving this site" page, with a cancel, before any navigation to a destination **not** covered here. ASVS asks about destinations outside the application's **control**, and control is *organisational* rather than topological — your own AD FS is a different host, a different origin, and squarely yours — so this is a declared domain list, **not** a same-origin test. Matched on a **label boundary**: `hospital.example` covers `adfs.hospital.example` and **not** `evilhospital.example` (a bare suffix test would admit the lookalike, which is the failure that makes an interstitial worse than none). **Empty is the STRICT position, not the lax one:** with nothing declared, *every* absolute `http(s)` destination is treated as external and gets the page — including your own IdP. Declaring your domains here is the correct fix for that, **not** `external_link_allowlist`. Entries are bare domains: a URL, scheme or `*` wildcard is refused at config load, because all three look right and match nothing. Env: `MEFOR_SECURITY_ORGANIZATION_DOMAINS` | +| `external_link_interstitial` | bool | `true` | **`[BUILT]` (ASVS 3.7.3):** show the "you are leaving this site" page at all. Setting it `false` means the console navigates off-site with **no notification and no cancel** — that is the control itself, so this is a posture decision rather than a convenience one, and `serve` prints a warning naming it at every start. The federated sign-in leg is affected: with the interstitial on and the IdP outside `organization_domains`, `GET /ui/oidc/start` renders the page and the flow is minted only on confirm (`POST`), which also closes the standing hole where any external page could begin a sign-in by linking to the start leg. Env: `MEFOR_SECURITY_EXTERNAL_LINK_INTERSTITIAL` | +| `external_link_allowlist` | list[str] | `[]` | **`[BUILT]` (ASVS 3.7.3) — ⚠️ THE AUDITED ESCAPE, AND IT LOWERS SECURITY.** Destinations listed here are navigated to with **no notification and no cancel**, which is precisely what the requirement asks for. It exists for legitimate high-volume external destinations an operator does not want to declare as their own domain. Same label-boundary matching. Non-empty makes `serve` print a warning **naming every entry individually** — never a count, because "3 destinations exempted" is the shape of message that lets an entry nobody intended sit in a list for a year. **Prefer `organization_domains`**: declaring a domain you control is a statement about scope; allowlisting one you do not is a waiver. Env: `MEFOR_SECURITY_EXTERNAL_LINK_ALLOWLIST` | | `require_sign_in` | bool | `true` | authenticate every request | | `require_mfa` | bool | `true` | second factor (native TOTP or a WebAuthn passkey), enforced as an **access gate** since ASVS 6.3.3 — an MFA-pending session is refused on *every* authorized route with `403` + `X-MFA-Required: 1`, and a browser session is redirected to `/ui/mfa`. **The enrolment path is a deliberate exemption, not a re-route:** `/ui/mfa` itself and the account/enrolment routes (`GET /ui/account`, the password and factor-enrolment routes) are declared `allow_mfa_pending=True` ([`messagefoundry_webconsole/_auth.py`](../messagefoundry_webconsole/_auth.py), `routes/account.py`), so a user with **no** factor enrolled is not stranded — send them to **`/ui/account`** to enrol TOTP or a passkey. Say "redirected to", not "confined to": `/ui/mfa` renders a code field only once TOTP is enrolled and a passkey button only once WebAuthn is, so a zero-factor user who is told they cannot leave that page is looking at a page with no form. | | `require_mfa_scope` | `"administrators"` \| `"every_local_account"` | `"every_local_account"` | **Which local accounts must ENROL a factor** when `require_mfa` is on (ASVS 6.3.3). An account that has already enrolled one must always satisfy it, under either value — this dial only decides who is required to enrol in the first place. `administrators` restores the pre-6.3.3 posture and is reported as a **loosening** on `GET /security/posture` (advisory, not a refusal: refusing to boot on it would break every existing deployment on upgrade). Directory (AD/Kerberos) identities are out of scope under either value — their MFA is delegated to the directory. **Operator note:** under the default a non-interactive **local bearer-token service account** becomes MFA-pending and cannot enrol unattended. The two available fixes are **make it an AD principal** or **set this to `administrators`** — mTLS is **not** a third: a cert-identity is exempt from the MFA gate but is admitted on exactly one route (`GET /service/identity`), so it cannot carry a working service account (see the [`[api]`](#api) `tls_client_cert_identities` row). Env: `MEFOR_SECURITY_REQUIRE_MFA_SCOPE` | @@ -1550,8 +1553,10 @@ SEV-SNP or Intel TDX host — [SYSTEM-REQUIREMENTS.md](SYSTEM-REQUIREMENTS.md#hardware-memory-encryption--required-for-an-asvs-level-3-phi-deployment) states the requirement and the (verified) availability picture, which today is **not reachable for a Windows guest on on-premises Hyper-V or ESXi**. On a host that does not provide the property, the honest -configuration is **not** to set this: leave it unset, keep the startup warning, and disclose 11.7.1 as -**Partial**. Reaching for `[security].enforcement = warn` is the wrong lever — that is the global +configuration is **not** to set this: leave it unset, keep the startup warning, and assess 11.7.1 +against your own deployment. *(Corrected 2026-08-02: this previously said "disclose 11.7.1 as +**Partial**" — pre-filling a verdict you had not reached, and one this project no longer holds.)* +Reaching for `[security].enforcement = warn` is the wrong lever — that is the global refuse/warn dial and downgrades every other posture refusal at the same time; nothing about this control requires it, because it never refuses unless you opt in via `require_memory_encryption_declaration`. The step-by-step is in OFF-LOOPBACK-DEPLOYMENT.md diff --git a/docs/PHI.md b/docs/PHI.md index d7a73866..f8691e76 100644 --- a/docs/PHI.md +++ b/docs/PHI.md @@ -583,7 +583,13 @@ it is carried as a **stated deployment requirement** (§10) accepted via a signe (ASVS-L3-RISK-ACCEPTANCE-REGISTER.md theme 5), not code. The compensating controls are the documented restricted-service-account + volume-encryption posture (§10) on a single-tenant host: keep the decrypted-secret window inside an OS-isolated process whose memory and -swap an attacker cannot reach without already owning the host. +swap an attacker cannot reach without already owning the host. ⚠️ **Both halves are operator-asserted +and engine-unchecked — say so whenever this is offered as compensating.** §2 records it directly: there +is **no** `[security].volume_encryption_declared` setting at HEAD and **nothing in the engine verifies +that FDE is on**. So this mitigates only where the operator actually applied it, and the engine cannot +tell you whether they did. *(Qualified 2026-08-02: the sentence previously read as though the posture +were a control the product supplies. A compensating control must not rest on a false premise — +`CLAUDE.md` §11 — and an unenforced prerequisite offered as a control is that premise.)* **Since [ADR 0152](adr/0152-in-use-data-protection-for-phi-platform-memory-encryption-attestation-asvs-11-7-1.md) the residual is *measured and surfaced*, not only asserted `[BUILT]`.** Three changes, none of which diff --git a/docs/SECURITY.md b/docs/SECURITY.md index aab44b52..f408bcb6 100644 --- a/docs/SECURITY.md +++ b/docs/SECURITY.md @@ -591,7 +591,7 @@ by an RBAC grant — the same basis as the JSON `/me/*` routes. **Unauthenticated `/ui` routes (10).** `GET`/`POST /ui/login`, `POST /ui/logout`, `GET /ui/sso`, `POST /ui/csp-report`, `GET`/`POST /ui/reauth`, `POST /ui/reauth/webauthn` and `GET`/`POST /ui/mfa` -(plus `GET /ui/oidc/start` and `GET /ui/oidc/callback` when federation is enabled). The three +(plus `GET`/`POST /ui/oidc/start` and `GET /ui/oidc/callback` when federation is enabled). The three `/ui/reauth*` routes authenticate the session cookie **manually** rather than through `require_ui`, because a gate that demanded a fresh step-up to *perform* a step-up would deadlock. The two `/ui/mfa` routes (ASVS 6.3.3) are the same shape for the same reason: `require_ui` 303s every @@ -1083,7 +1083,7 @@ one-to-one — that is why the bind/exposure posture occupies two rows and the A | Bind / exposure posture — refusing arms | `[api].host` loopback-ness, `tls_terminated_upstream`, `trusted_proxies`, `public_origin`, `serve_ui`; derived `ui_exposed`, `admin_exposed`; `[security].enforcement`; declared data class | auth off on a non-loopback bind; `/ui` exposed without the required origin/TLS declarations; `admin_exposed` + PHI + `enforcing` + `require_mfa` explicitly opted out | **DENY at startup** — `serve` prints an error and exits **2**. The refuse/warn dial is `[security].enforcement` (default `enforce`), **not** `production`: the auth-off and `/ui`-exposure arms refuse **unconditionally**, and the `require_mfa` arm refuses when the declared data class is PHI **and** enforcement is `enforce` — which includes the non-production `dev` and `staging` environments, both of which derive PHI — and warns otherwise. `[security].allow_single_factor_admin_when_exposed = true` downgrades that one arm to permitted-but-audited. The same attributes force the session cookie's `Secure` flag + HSTS, and permit WebAuthn `rp_id` derivation from the request URL **only** on a loopback bind with no proxy declared | loopback, nothing declared | `[api].*`, `[security].enforcement`, `[security].allow_single_factor_admin_when_exposed`, `[ai].data_class`/`environment` | | Bind / exposure posture — dual-control arm | `admin_exposed` × `[approvals].enabled` × declared data class | `admin_exposed` **and** PHI **and** `[approvals].enabled` off — high-value actions complete on one caller's authority | **LOG** — a startup **WARNING only, on every instance including production**; `serve` does **not** refuse. The refuse arm is an explicit unresolved owner fork recorded in `__main__.py`, not a shipped control | approvals off | `[approvals].enabled` | | Pending federated-login flows, per client IP | the `client_ip` recorded on each staged flow | ≥ **16** pending flows from this address (`DEFAULT_PER_IP_CAP`, no knob), or ≥ `oidc_flow_cache_max` (**512**) engine-wide; 300 s TTL; **reject-when-full, never evict** (evict-oldest would turn a start-leg flood into a login DoS) | **DENY** the start leg — `FlowCacheFullError` → **303** to `/ui/login?e=rate_limited`, WARNING-logged, deliberately **never** audited so a flood cannot amplify into `audit_log` growth | 16 / 512 / 300 s | `[auth].oidc_flow_cache_max`, `oidc_flow_ttl_seconds` | -| `Sec-Fetch-Mode` on the federated sign-in legs | the browser fetch-metadata header on `GET /ui/sso`, `GET /ui/oidc/start`, `GET /ui/oidc/callback` | header **present** and not `navigate` (absent = allowed, for non-browser clients). Distinct from the `Sec-Fetch-Site` row below: a different header, a different surface, and `assert_same_origin` deliberately does **not** run on the callback leg, whose `Sec-Fetch-Site` is legitimately cross-site | **DENY** — 303 → `/ui/login?e=sso_failed`\|`oidc_failed`, plus an **audited** `auth.login_failed` row carrying the closed-set slug `non_navigation_fetch`. Evaluated **after** the login limiter, so the audit write is itself rate-bounded | on | (no knob) | +| `Sec-Fetch-Mode` on the federated sign-in legs | the browser fetch-metadata header on `GET /ui/sso`, `POST /ui/oidc/start`, `GET /ui/oidc/callback` | header **present** and not `navigate` (absent = allowed, for non-browser clients). Distinct from the `Sec-Fetch-Site` row below: a different header, a different surface, and `assert_same_origin` deliberately does **not** run on the callback leg, whose `Sec-Fetch-Site` is legitimately cross-site | **DENY** — 303 → `/ui/login?e=sso_failed`\|`oidc_failed`, plus an **audited** `auth.login_failed` row carrying the closed-set slug `non_navigation_fetch`. Evaluated **after** the login limiter, so the audit write is itself rate-bounded | on | (no knob) | | Instance environment posture × claimed AI data scope | `[ai].derived_posture()` (from `[ai].environment` / `data_class` / `production`; an unresolved posture defaults to the **strictest** ceiling) re-resolved server-side through `resolve_effective_policy` on every `POST /ai/chat` | the effective mode is not `managed_endpoint`, or the request's `data_scope` exceeds the server-enforced ceiling (the engine-broker MVP enforces `code_only` regardless of what the caller claims) | **DENY** — **409** on the mode mismatch, **403** on scope excess; each audited `ai.assist` with PHI-safe metadata only | `mode = byo`, `data_scope = code_only` | `[ai].mode`, `[ai].data_scope`, `[ai].environment`/`data_class`/`production` | | Gated operation × requester-vs-approver identity × hold age | the pending-approval record: the operation name, the requesting identity, and the hold's creation time | `[approvals].enabled` **and** the operation is in `[approvals].operations` and has no approved unexpired release; the approver is the requester; the hold is older than `expiry_hours` | **DENY** the immediate execution — **202** hold + `approval.requested` audit; **403** on self-approval; **409** once expired or already decided | off; `['connection_purge','dead_letter_replay']`; 72 h | `[approvals].enabled`, `operations`, `expiry_hours` | | mTLS client-certificate subject | the qualified subject-RDN / SAN names of a **verified** peer certificate | exact match against a deny-by-default map (empty map = feature off) | **ALLOW** — resolve to that principal's Identity (RBAC then authorizes); a disabled account grants none | `{}` = off | `[api].tls_client_cert_identities` (requires `tls_client_ca_file`) | @@ -1409,7 +1409,7 @@ Comparative properties on the dimensions the table's four columns cannot carry: **Where each pathway is enforced, and what turns it on:** Local → `POST /auth/login` + `POST /ui/login` (always available); AD → the same two routes with `provider=ad` (`[auth].ad_enabled`); Kerberos → `POST /auth/negotiate` + `GET /ui/sso` (`[auth].kerberos_enabled`, default off); OIDC → -`GET /ui/oidc/start` + `GET /ui/oidc/callback`, registered **only** when `[auth].oidc_enabled` (default +`GET`/`POST /ui/oidc/start` + `GET /ui/oidc/callback`, registered **only** when `[auth].oidc_enabled` (default off, and it additionally requires `ad_enabled`); mTLS → `GET /service/identity`, active only when `[api].tls_client_cert_identities` **and** `[api].tls_client_ca_file` are both set (default `{}` = off). @@ -1477,7 +1477,7 @@ threshold, the switch that disables it, and — the part that matters for "not d | 4 | **argon2 concurrency cap** | executor exhaustion under a login flood | an instance semaphore sized `max(2, min(8, cpu_count))`; every hash/verify runs off the event loop | none | n/a | | 5 | **Request-body cap + field limits** | oversized/ambiguous auth requests | 1 MiB (the `/uploads` routes alone admit up to `[store].max_upload_bytes`), a **required** `Content-Length` for any body (a chunked body is refused **411**), and CL+TE ambiguous framing refused **400** — all as ASGI middleware ahead of every route | none | n/a | | 6 | **Pre-auth client-network gate** | reaching the auth surface at all from an unlisted network | membership in `[security].allowed_client_networks` | `[]` = no restriction (the default) | limiters 1–3 | -| 7 | **Federated pending-flow bound** (`FlowCache.put`, **reject-when-full**) | flooding the OIDC start leg (`GET /ui/oidc/start`) to exhaust engine memory or deny federated sign-in | 16 pending flows per client IP, 512 engine-wide, 300 s TTL. It **rejects** rather than evicts — evict-oldest would turn a start-leg flood into a login DoS for legitimate users | **none** — and `oidc_flow_cache_max = 0` is not an opt-out either: `put` refuses at `len(entries) >= global_cap`, so `0` rejects **every** federated sign-in (`FlowCacheFullError` on the first flow, an OIDC denial of service). No validator floors it; treat it as a security-relevant value | limiter 2 (the same routes charge `allow_login_attempt` first) | +| 7 | **Federated pending-flow bound** (`FlowCache.put`, **reject-when-full**) | flooding the OIDC start leg (`POST /ui/oidc/start` — the GET renders the 3.7.3 interstitial and stages nothing, so it is not a lever) to exhaust engine memory or deny federated sign-in | 16 pending flows per client IP, 512 engine-wide, 300 s TTL. It **rejects** rather than evicts — evict-oldest would turn a start-leg flood into a login DoS for legitimate users | **none** — and `oidc_flow_cache_max = 0` is not an opt-out either: `put` refuses at `len(entries) >= global_cap`, so `0` rejects **every** federated sign-in (`FlowCacheFullError` on the first flow, an OIDC denial of service). No validator floors it; treat it as a security-relevant value | limiter 2 (the same routes charge `allow_login_attempt` first) | | 8 | **WebAuthn pending-ceremony bound** (`ChallengeCache.put`) | flooding passkey registration/assertion ceremonies | 16 pending ceremonies per **user** (evicts that *same* user's oldest, so one principal can never deny another's), 4096 engine-wide (**refuses** with a cause-naming `ChallengeCacheFullError`), 120 s TTL | none | limiter 3 on the assertion **finish** leg only — `POST /ui/reauth/webauthn` (`routes/core.py:689`) and the error re-render inside `POST /ui/reauth` (`:612`) charge `allow_reauth_attempt`. The routes that *stage* a ceremony — the thing `ChallengeCache.put` actually bounds — charge **no** limiter: `POST /ui/account/webauthn/enroll`, `POST /ui/account/webauthn/verify`, and `GET /ui/reauth`, which re-stages fresh assertion options on **every** render. There this bound plus cookie-holder-only reachability is all there is | | 9 | **JWKS min-refetch floor** (`JwksCache.get_key`) | unauthenticated `kid`-driven refetch amplification against the IdP on the OIDC callback leg — the sibling of control 7 on the *other* federated leg | one upstream fetch per **300 s**, globally (`[auth].oidc_jwks_min_refetch_seconds`), plus a `_MAX_JWKS_BYTES` **512 KiB** response-body cap and a 3600 s key TTL. Within the floor an unknown `kid` raises `JwksError` and that login fails (a still-cached key is served even past the soft TTL rather than fail while throttled) | `oidc_jwks_min_refetch_seconds = 0` — no validator floor, so this **is** a genuine opt-out, and it restores the amplification | limiter 2 and control 7 (the same legs charge `allow_login_attempt` and stage a bounded flow first) | @@ -1522,7 +1522,7 @@ the recovery path. Controls 4–6 are covered in their own rows. | `POST /auth/mfa-verify` | sign-in window | an **authenticated** route drawing the sign-in budget (it is a mid-login challenge); also feeds the per-account lockout | | `POST /ui/login` | sign-in window | 429 carries `Retry-After: 30` | | `GET /ui/sso` | sign-in window | the token-bearing leg only; the RFC 4559 challenge leg is deliberately unthrottled | -| `GET /ui/oidc/start`, `GET /ui/oidc/callback` | sign-in window | one browser login charges it **twice** | +| `POST /ui/oidc/start`, `GET /ui/oidc/callback` | sign-in window | one browser login charges it **twice**. ⚠️ The start leg is a **POST** since the ASVS 3.7.3 interstitial: `GET /ui/oidc/start` now renders the "you are leaving this site" page and mints **no** flow, so it charges no limiter — the flow starts only when the operator confirms. | | `POST /me/password` | per-actor ceremony budget | **not** the sign-in window | | `POST /me/reauth` | per-actor ceremony budget | | | `POST /me/mfa/confirm` | per-actor ceremony budget | | diff --git a/docs/SYSTEM-REQUIREMENTS.md b/docs/SYSTEM-REQUIREMENTS.md index d06e3c38..a8636c25 100644 --- a/docs/SYSTEM-REQUIREMENTS.md +++ b/docs/SYSTEM-REQUIREMENTS.md @@ -47,9 +47,12 @@ only the host can. On a host that does not provide it: - the engine **still runs** — nothing here is a functional requirement, and every other PHI control (at-rest encryption, retention, audit, RBAC, transport) is unaffected; -- ASVS 11.7.1 is capped at **Partial**, not Pass, and that cap is a **hardware fact about your - deployment**, not a gap in the software. Disclose it in your own assessment rather than working - around it; +- ASVS 11.7.1 concerns a **host property**, not a software one — full memory encryption is provided by + the CPU, firmware and hypervisor. **Assess and disclose it against your own deployment**, on your own + evidence, rather than working around it. *(Corrected 2026-08-02: this previously told you to + "disclose 11.7.1 as **Partial**". That was wrong twice over — it put a specific verdict in your + assessment that you had not reached, and this project no longer holds that verdict either. What + verdict your deployment warrants is yours to determine; nothing here should pre-fill it.)* - an **exposed** PHI instance **warns at every start** until the decision is recorded — `[security].memory_encryption_operator_declared = true` is the operator's declaration that the host provides it. The engine **starts either way**: this is a host property, not a config error, and one no diff --git a/docs/adr/0152-in-use-data-protection-for-phi-platform-memory-encryption-attestation-asvs-11-7-1.md b/docs/adr/0152-in-use-data-protection-for-phi-platform-memory-encryption-attestation-asvs-11-7-1.md index 0c2dd953..565a8061 100644 --- a/docs/adr/0152-in-use-data-protection-for-phi-platform-memory-encryption-attestation-asvs-11-7-1.md +++ b/docs/adr/0152-in-use-data-protection-for-phi-platform-memory-encryption-attestation-asvs-11-7-1.md @@ -266,6 +266,57 @@ it". A prior assessment used exactly this rationale and the 2026-07-22 re-score argument may still be winnable, but it must survive adversarial review before being signed again, and it is strictly weaker than measuring the property. +> ### ⛔ AMENDMENT 2026-08-02 — this option is now ADOPTED, and the paragraph above is why it took four tries +> +> **11.7.1 is `na`. Owner ruling, LOCKED as of 2026-08-02.** This paragraph is the strongest attack on +> that ruling and it is answered here rather than left to be rediscovered — it is the record of one of +> the four verdict moves, and the one that went the other way. +> +> **The paragraph's stated ground is FALSIFIED, and it was my claim too.** *"Its enumerated grounds are +> absent functionality and external processes acting on the application"* is **not what ASVS says.** +> Verbatim, `0x03-What-is-the-ASVS.md` at the `v5.0.0` tag: +> +> > "Conversely, ASVS generally excludes requirements that are not directly relevant to the application +> > or **where configuration is outside the application's responsibility**. For example, DNS issues are +> > typically managed by a separate team or function." +> +> That is a **responsibility/environment** exclusion — precisely the category this paragraph asserts the +> standard does not provide. The error survived because the project's pinned ASVS corpus is +> **requirements-only** and holds no chapter prose, so nobody checking against it could ever catch a +> wrong claim about what the standard *says*. It was re-asserted independently by two later assessors, +> including in a signed-adjacent register block, before being caught by fetching the chapter. +> +> **And the adopted ruling does not rest on the ground this paragraph rejects.** It is not *"the control +> exists but the platform provides it"*. It is **rule 1 against a positively-declared scope**: +> [`ASVS-ASSESSMENT-METHOD.md`](../ASVS-ASSESSMENT-METHOD.md) §2 declares the assessed subject as three +> **software artifacts** — engine, web console, IDE extension, assessed as source. *"Full memory +> encryption is in use"* names a CPU/firmware/hypervisor property, which is not among them, so rule 1 +> fires before rule 3 is reached. The rungs 1–2 objection is answered rather than dodged: that code +> **reports on and gates against** the platform property; it never **provides** it. +> +> **The adversarial review this paragraph demanded has happened.** Two independent multi-agent research +> runs with 3-vote refutation (25 claims verified, 12 killed in one; 8 confirmed of 25 in the other), +> plus a second session that attacked the ruling directly and surfaced this very paragraph. What it +> found: **no attestation on any platform asserts memory encryption as a property** — AWS Nitro carries +> PCRs and is processor-agnostic hypervisor isolation; SEV-SNP's VCEK-signed measurement is *launch-time* +> integrity. So *"strictly weaker than measuring the property"* assumed measuring was achievable. **It is +> not, on any platform, today.** +> +> ⚠️ **What this does NOT buy, stated because the temptation is exactly here:** no Level 3 claim. ASVS +> 4.0's *"may still claim full ASVS compliance"* clause was **dropped** in 5.0, and OWASP retains +> normative authority over which requirements sit at which level. See +> [`ASVS-ASSESSMENT-METHOD.md`](../ASVS-ASSESSMENT-METHOD.md) §2.1. The published attestation was +> narrowed in the same change. +> +> **Also honest:** the fail count moved 3 → 2 with **zero engine code changed**. The posture did not +> improve; the scope got stated (§2.2). +> +> **LOCKED.** The scorecard cell carries `decision_closed = true` with a pinned verdict, and +> `scripts/asvs/scorecard.py` now **refuses to load** a closed cell whose verdict has moved off its pin. +> Reopening takes an explicit owner instruction — not a sweep's judgement, and not a rediscovery of this +> paragraph. **"Do nothing and carry the Fail"** below remains recorded, but is no longer the fallback: +> it answers a question rule 1 says is never reached. + **Rewrite the PHI path to `bytearray`/`memoryview` with explicit wiping.** Rejected. Architectural, defeated by CPython's copy semantics in the parse/transform path, and it produces memory *hygiene* rather than memory *encryption* — the exact distinction OWASP drew when it deleted V8.3.6 and kept diff --git a/messagefoundry/__main__.py b/messagefoundry/__main__.py index 55729857..49cae9b0 100644 --- a/messagefoundry/__main__.py +++ b/messagefoundry/__main__.py @@ -2237,6 +2237,33 @@ def _serve(args: argparse.Namespace) -> int: # should be told about the concrete misconfiguration before the platform-property one, and a # gate that jumped the queue would silently change which error every existing exposed-PHI test # (and every existing exposed-PHI deployment) reports. + # ASVS 3.7.3: the "you are leaving this site" interstitial. Two knobs can weaken it, and BOTH are + # announced at start rather than discovered in a later assessment. Warn-only by design: neither is + # a PHI-safety property and refusing on an operator's deliberate UX decision would be a + # self-inflicted availability failure — the same reasoning as the read-out below. + _ext_allow = list(settings.security.external_link_allowlist) + if not settings.security.external_link_interstitial: + print( + "warning: [security].external_link_interstitial=false — the console will navigate " + "OFF-SITE with no notification and no cancel. This is the ASVS 3.7.3 control; disabling " + "it is a posture decision, not a convenience one.", + file=sys.stderr, + ) + elif _ext_allow: + # Named individually, never counted. "3 destinations exempted" is the shape of message that + # lets an entry nobody intended sit in a list for a year. + print( + "warning: [security].external_link_allowlist exempts " + f"{', '.join(repr(d) for d in _ext_allow)} from the off-site interstitial (ASVS 3.7.3) — " + "navigation to these destinations shows no notification and offers no cancel.", + file=sys.stderr, + ) + # NOT warned: an empty `organization_domains`. It is the STRICT position (every off-site + # destination is interstitialed) and it is the shipped default, so a note here would print on + # every stock start — `test_serve_loopback_emits_no_new_stderr` catches exactly that, and it is + # right to. Start-time output is for a posture that is WEAKER than the default, not for the + # default itself. The guidance that matters — declare your domains rather than reaching for the + # allowlist escape — belongs in docs/CONFIGURATION.md, where it is, and not in every boot log. # # PHI is plaintext in CPython heap while it is being processed — an HL7 body is `str` end to end # by design, and every parse/transform step allocates a fresh immutable copy no application code diff --git a/messagefoundry/api/_ui_seam.py b/messagefoundry/api/_ui_seam.py index 5d5a81b4..51146970 100644 --- a/messagefoundry/api/_ui_seam.py +++ b/messagefoundry/api/_ui_seam.py @@ -81,7 +81,14 @@ #: backend without the lever, so an older console simply ignores it; a separate seam rather than a #: correction to v15 because v15 is a SecurityPosture change and folding an unrelated DTO into it would #: make that note describe a field set it does not cover. -ENGINE_UI_SEAM: int = 16 +#: v17 (ASVS 3.7.3): the external-navigation interstitial policy — `organization_domains`, +#: `external_link_interstitial`, `external_link_allowlist` and `oidc_authorization_host`. Passed as +#: CONFIG for the same reason `oidc_enabled` is: `create_managed_app` attaches the AuthService inside +#: the lifespan, long after `mount_ui` has fixed the route table, so a registrar reading it off +#: `app.state.auth` would register nothing in production while passing every test that constructs the +#: app with `auth=` directly. Additive with defaults, and the defaults are the STRICT position — an +#: older or partial caller gets the interstitial on every absolute destination, never none. +ENGINE_UI_SEAM: int = 17 @dataclass(frozen=True, slots=True) @@ -211,3 +218,17 @@ class UiDeps: #: table. A registrar that gated on ``app.state.auth`` would therefore register nothing in #: production while passing every test that constructs the app with ``auth=`` directly. oidc_enabled: bool = False + #: ASVS 3.7.3 (seam v17). Domains that count as INSIDE the organization — the interstitial is + #: shown for anything else. Matched on a LABEL boundary by ``messagefoundry_webconsole._external``. + #: EMPTY is the STRICT position, not the lax one: every absolute http(s) destination is external. + organization_domains: tuple[str, ...] = () + #: Whether to interpose the "you are leaving this site" page at all. On by default; off is a + #: posture decision and the serve gate says so. + external_link_interstitial: bool = True + #: ⚠️ The audited escape — destinations navigated to with NO notification and NO cancel, which is + #: exactly what 3.7.3 asks for. Non-empty produces a startup warning naming every entry. + external_link_allowlist: tuple[str, ...] = () + #: Host of the configured IdP authorization endpoint, for DISPLAY on the interstitial. Derived + #: from settings, never from request input — if the destination came from the request the + #: interstitial would itself be an open redirect, which is worse than having no interstitial. + oidc_authorization_host: str = "" diff --git a/messagefoundry/api/app.py b/messagefoundry/api/app.py index 4a6033e3..cc1a1833 100644 --- a/messagefoundry/api/app.py +++ b/messagefoundry/api/app.py @@ -1037,6 +1037,9 @@ def create_app( # app.state.auth -- create_managed_app attaches the service in the lifespan, AFTER mount_ui # has already fixed the route table. oidc_enabled: bool = False, + # ASVS 3.7.3 (seam v17): the configured IdP authorization endpoint, for the interstitial's + # DISPLAY host. Config, never request input — see UiDeps.oidc_authorization_host. + oidc_authorization_endpoint: str = "", webauthn_rp_from_request: bool = True, exposure_protected: bool = False, loopback: bool = False, @@ -4989,9 +4992,34 @@ async def _reauthorize() -> Identity | None: # Either source may know: create_managed_app passes the config flag; a caller that # constructs with auth= directly (tests, embedders) gets it from the live service. ui_oidc_enabled = oidc_enabled or bool(getattr(auth, "oidc_enabled", False)) + # ASVS 3.7.3 (seam v17). Read off security_settings when present; the fallbacks are the + # STRICT position, so a caller that constructs without them gets the interstitial on every + # absolute destination rather than silently getting none. + _sec = security_settings + + def _oidc_authorization_host(endpoint: str) -> str: + """ASCII/punycode host of the configured IdP endpoint, for DISPLAY only. + + Local rather than imported from ``messagefoundry_webconsole._external``: the console is + deliberately not imported at module scope here (see the note above ``create_app``). An + unparseable endpoint yields ``""``, which the console treats as *unknown destination* and + therefore as a reason to SHOW the interstitial, never to skip it. + """ + from urllib.parse import urlsplit + + try: + host = (urlsplit(endpoint).hostname or "").strip().lower() + return host.encode("idna").decode("ascii").lower() if host else "" + except (ValueError, UnicodeError): + return "" + deps = UiDeps( engine_seam=ENGINE_UI_SEAM, oidc_enabled=ui_oidc_enabled, + organization_domains=tuple(getattr(_sec, "organization_domains", ()) or ()), + external_link_interstitial=bool(getattr(_sec, "external_link_interstitial", True)), + external_link_allowlist=tuple(getattr(_sec, "external_link_allowlist", ()) or ()), + oidc_authorization_host=_oidc_authorization_host(oidc_authorization_endpoint), get_engine=_get_engine, get_gate=_get_gate, cookie_secure=_cookie_secure, @@ -5727,6 +5755,9 @@ async def _audit_upload_prune(meta: UploadedFileMeta) -> None: ws_allowed_origins=ws_allowed_origins, serve_ui=serve_ui, oidc_enabled=bool(auth_settings is not None and auth_settings.oidc_enabled), + oidc_authorization_endpoint=( + (auth_settings.oidc_authorization_endpoint or "") if auth_settings is not None else "" + ), public_origin=public_origin, webauthn_rp_from_request=webauthn_rp_from_request, exposure_protected=exposure_protected, diff --git a/messagefoundry/config/settings.py b/messagefoundry/config/settings.py index 321c49df..d32764a0 100644 --- a/messagefoundry/config/settings.py +++ b/messagefoundry/config/settings.py @@ -3629,6 +3629,60 @@ class SecuritySettings(_Section): handles_real_patient_data: bool | None = None # was [ai].data_class = "phi" production_instance: bool | None = None # was [ai].production + # ── Leaving the organization: the ASVS 3.7.3 interstitial ──────── + # Domains that count as INSIDE the organization. ASVS 3.7.3 asks for a notification when the user + # is sent somewhere "outside the application's CONTROL", and control is organisational rather than + # topological — an operator's own AD FS is a different host, a different origin, and squarely + # theirs. Matched on a LABEL boundary, so "hospital.example" covers "adfs.hospital.example" and + # NOT "evilhospital.example"; a bare endswith would admit the lookalike. + # + # EMPTY (the default) is deliberately the strict position, not the lax one: with nothing declared, + # every absolute http(s) destination is treated as external and gets the interstitial. An operator + # who configures nothing is warned too often, never too little. + organization_domains: list[str] = Field(default_factory=list) + # The interstitial itself. On by default (ADR-less: this IS the 3.7.3 control). Turning it off is + # a posture decision, not a convenience one, and the serve gate says so. + external_link_interstitial: bool = True + # ⚠️ THE AUDITED ESCAPE, and it LOWERS SECURITY. Destinations here are navigated to with no + # notification and no cancel — precisely what 3.7.3 asks for. It exists because operators have + # legitimate high-volume external destinations they do not want to declare as their own domain. + # Same label-boundary matching. Non-empty produces a startup warning naming every entry; the + # method's rule is that a signed relaxation is never a Pass, so this is the delta, not the default. + external_link_allowlist: list[str] = Field(default_factory=list) + + @field_validator("organization_domains", "external_link_allowlist", mode="before") + @classmethod + def _split_domain_list(cls, value: object) -> object: + """Accept a comma/whitespace-separated string as well as a list — parity with the other + list-valued settings here, so an env-var override does not need TOML array syntax.""" + if isinstance(value, str): + return [part for part in value.replace(",", " ").split() if part] + return value + + @field_validator("organization_domains", "external_link_allowlist", mode="after") + @classmethod + def _check_domains_are_bare_hosts(cls, value: list[str]) -> list[str]: + """Reject a URL or a wildcard where a domain belongs. + + ``https://hospital.example/`` and ``*.hospital.example`` both look right and both silently + match NOTHING under label-boundary comparison — the operator would believe they had declared + an internal domain and get an interstitial on every internal link, or worse, believe they had + allowlisted something that is still being warned about. Failing at config load is the only + place this is cheap to notice. + """ + cleaned: list[str] = [] + for raw in value: + item = raw.strip().lower().lstrip(".") + if not item: + continue + if "/" in item or ":" in item or "*" in item: + raise ValueError( + f"{item!r} must be a bare domain such as 'hospital.example', not a URL, scheme " + "or wildcard — subdomains are matched automatically on a label boundary" + ) + cleaned.append(item) + return cleaned + @field_validator("allowed_client_networks", mode="before") @classmethod def _split_client_networks(cls, v: object) -> object: @@ -4075,6 +4129,29 @@ def security_loosenings( "in cleartext (still refused on a production-PHI bind)", ) ) + if not sec.external_link_interstitial: + out.append( + ( + "external_link_interstitial", + "the console navigates OFF-SITE with no notification and no cancel — an operator can be " + "sent to a third-party site (including an identity provider) with no chance to stop it " + "(ASVS 3.7.3)", + ) + ) + if sec.external_link_allowlist: + # Reported even though the switch is a LIST rather than a bool, because the completeness floor + # only pins bools and an exempted list would be an unreported loosening by omission. Entries + # are named individually: a count would say "3 destinations are exempt" without saying which, + # which is the shape that lets an entry nobody intended survive a posture review. + out.append( + ( + "external_link_allowlist", + "these destinations are exempt from the off-site interstitial and are navigated to " + "with no notification and no cancel: " + + ", ".join(sec.external_link_allowlist) + + " (ASVS 3.7.3)", + ) + ) if not sec.require_sign_in: out.append( ( diff --git a/messagefoundry_webconsole/__init__.py b/messagefoundry_webconsole/__init__.py index 100e4748..186466be 100644 --- a/messagefoundry_webconsole/__init__.py +++ b/messagefoundry_webconsole/__init__.py @@ -45,7 +45,7 @@ # If cross-seam support is ever genuinely wanted, re-widen this set AND add the CI matrix that # installs the MIN and MAX supported engine builds — the claim and its test land together, or not # at all. -SUPPORTED_ENGINE_SEAMS: frozenset[int] = frozenset({16}) +SUPPORTED_ENGINE_SEAMS: frozenset[int] = frozenset({17}) #: The vendored static assets shipped in THIS wheel (mounted at /ui/static by :func:`mount_ui`). STATIC_DIR = Path(__file__).parent / "static" diff --git a/messagefoundry_webconsole/_external.py b/messagefoundry_webconsole/_external.py new file mode 100644 index 00000000..84c3217c --- /dev/null +++ b/messagefoundry_webconsole/_external.py @@ -0,0 +1,115 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""Is this navigation leaving the organization, and what host do we tell the operator it goes to? + +ASVS 3.7.3 asks for a notification, with a cancel, when the user is sent to a URL **outside the +application's control**. Control is *organisational*, not topological: a hospital's own AD FS is a +different host, a different origin, and still squarely inside the operator's control. So the test here +is a declared domain list (``[security].organization_domains``), not same-origin. + +**Everything in this module is pure.** No settings import, no request, no I/O — the predicate is the +part that has to be right, and it is the part worth testing exhaustively. + +Two failure modes drove the details, both from the 3.7.3 research: + +* **A suffix test without a dot boundary is a hole.** ``evilhospital.example`` ends with ``hospital.example``. + Matching must be on a label boundary or the allowlist silently admits the attacker's lookalike. +* **The displayed host must be what the browser will actually resolve.** An IDN homograph + (Cyrillic ``а`` in ``аmazon.example``) renders identically to the Latin form, so showing the decoded + Unicode is showing the operator a lie. We display the **punycode/ASCII** form, which is what DNS + gets, and say so when the two differ. +""" + +from __future__ import annotations + +from urllib.parse import urlsplit + +#: Schemes we will render an interstitial for. Anything else (``javascript:``, ``data:``, ``file:``) +#: is not a navigation we should be helping the operator complete, so callers treat it as a hard +#: refusal rather than as an external link to warn about. +NAVIGABLE_SCHEMES: frozenset[str] = frozenset({"http", "https"}) + + +def host_of(url: str) -> str: + """The lowercase ASCII host of ``url``, or ``""`` if it has none we can trust. + + Returns the **IDNA/punycode** form deliberately — see the module docstring. A host that cannot be + encoded (malformed IDN, empty label) returns ``""``, which every caller treats as *not internal*: + failing toward showing the interstitial is the safe direction. + """ + try: + host = (urlsplit(url).hostname or "").strip().lower() + except ValueError: + # urlsplit raises on things like an invalid IPv6 literal. Not parseable is not internal. + return "" + if not host: + return "" + try: + # ``encode("idna")`` rejects empty labels and over-long ones, which is why it is preferred + # here over a bare ``str`` compare: it is the same normalisation the resolver will apply. + return host.encode("idna").decode("ascii").lower() + except UnicodeError: + return "" + + +def is_idn_disguised(url: str) -> bool: + """True when the host renders as one thing and resolves as another. + + A homograph attack is invisible by construction — that is its whole point — so the interstitial + needs to say "this is not the ASCII you think it is" rather than rely on the operator spotting a + Cyrillic ``а``. Any host that survives IDNA encoding into a ``xn--`` label qualifies. + """ + return "xn--" in host_of(url) + + +def _matches_domain(host: str, domain: str) -> bool: + """``host`` is ``domain`` or a subdomain of it — matched on a LABEL boundary. + + ``evilhospital.example`` must not match ``hospital.example``. A plain ``endswith`` says it does. + """ + domain = domain.strip().lower().lstrip(".") + if not domain or not host: + return False + return host == domain or host.endswith("." + domain) + + +def is_external(url: str, organization_domains: list[str] | tuple[str, ...]) -> bool: + """Does following ``url`` take the operator outside the organisation? + + ``True`` means *show the interstitial*. The default is deliberately biased that way: an empty + ``organization_domains`` makes every absolute http(s) URL external, so an operator who configures + nothing gets the notification rather than silently getting none. + + Relative URLs (``/ui/...``) are never external — they cannot leave the origin. + """ + if not url or url.startswith("/"): + return False + scheme = urlsplit(url).scheme.lower() + if scheme and scheme not in NAVIGABLE_SCHEMES: + # Not a navigation we warn about; callers refuse these outright. + return True + host = host_of(url) + if not host: + return True + return not any(_matches_domain(host, d) for d in organization_domains) + + +def is_allowlisted(url: str, allowlist: list[str] | tuple[str, ...]) -> bool: + """Has the operator explicitly exempted this destination from the interstitial? + + ⚠️ This is the **audited escape**, and it lowers security by design: an allowlisted destination + is navigated to with no notification and no cancel, which is precisely what ASVS 3.7.3 asks for. + It exists because operators have legitimate high-traffic internal destinations on domains they + do not want to declare wholesale. The serve gate warns when it is non-empty. + + Matched on the same label boundary as :func:`is_external`, for the same reason. + """ + host = host_of(url) + if not host: + return False + return any(_matches_domain(host, d) for d in allowlist) + + +def display_host(url: str) -> str: + """The host to SHOW the operator — ASCII, so it matches what the browser resolves.""" + return host_of(url) or "(unreadable destination)" diff --git a/messagefoundry_webconsole/pages/account.py b/messagefoundry_webconsole/pages/account.py index 5a663e5c..a9ab9378 100644 --- a/messagefoundry_webconsole/pages/account.py +++ b/messagefoundry_webconsole/pages/account.py @@ -25,6 +25,7 @@ __all__ = [ "account_page", + "leaving_site", "login", "oidc_landing", "mfa_confirm_page", @@ -157,6 +158,71 @@ def oidc_landing() -> Markup: ) +def leaving_site( + *, + destination_host: str, + continue_action: str, + idn_disguised: bool = False, + cancel_href: str = "/ui/login", + purpose: str = "", +) -> Markup: + """The ASVS 3.7.3 interstitial: you are leaving, here is where to, and you may cancel. + + **The destination URL is deliberately NOT a parameter of this page.** Only the host is passed, for + display. The actual URL lives in server-side flow state and is looked up by the POST handler — if + the page carried the target, the interstitial would itself be an open redirect, which is strictly + worse than having no interstitial at all. + + **``destination_host`` must already be punycode/ASCII** (see ``_external.display_host``). Showing + the decoded Unicode would show the operator a host that is not the one the browser resolves, which + makes this page actively misleading rather than merely useless. ``idn_disguised`` surfaces the + case where the two differ, because a homograph is invisible by construction and cannot be left to + the reader to spot. + + Continue is a **form POST**, not a link. Two reasons: the POST target carries no URL so it cannot + be pointed anywhere else, and it puts the navigation behind the console's existing same-origin / + ``Sec-Fetch-Site`` check — which also closes the pre-existing hole where any external page could + start a federated sign-in just by linking to the start leg. + + No JavaScript: the ``/ui`` CSP is strict, with no nonce and no ``script-src`` relaxation. + """ + warn = ( + el( + "p", + el("strong", "This address uses non-ASCII characters."), + " It is shown above in its encoded form, which is what your browser will actually look " + "up. A name that looks familiar may not be the site you expect.", + class_="warn", + ) + if idn_disguised + else Markup("") + ) + reason = el("p", purpose, class_="muted") if purpose else Markup("") + body = el( + "div", + el("h1", wordmark(tm=True)), + el("p", el("strong", "You are leaving this site.")), + reason, + el("p", "You will be taken to:"), + el("p", el("code", destination_host)), + warn, + el( + "form", + el("button", "Continue", type="submit"), + method="post", + action=continue_action, + class_="login", + ), + el("p", el("a", "Cancel", href=cancel_href), class_="muted"), + class_="card", + ) + # ASVS 7.4.4: `nav=Markup("")` would suppress the shared chrome AND its sign-out control, + # stranding a signed-in operator on a page with no way out. `minimal_nav()` is what the + # sibling interstitial (`oidc_landing`) uses for exactly this reason. The first version of + # this page used the bare Markup("") and was wrong. + return page("Leaving this site", body, nav=minimal_nav()) + + def sso_challenge() -> Markup: """The HTML body of the RFC 4559 401 challenge (L5c, ADR 0068 §9): a browser configured for Windows SSO retries the request with its Negotiate token and never renders this; one that diff --git a/messagefoundry_webconsole/routes/oidc.py b/messagefoundry_webconsole/routes/oidc.py index 9bd582d2..a548fd2d 100644 --- a/messagefoundry_webconsole/routes/oidc.py +++ b/messagefoundry_webconsole/routes/oidc.py @@ -2,11 +2,14 @@ # Copyright (C) 2026 MessageFoundry Organization and contributors """W4-5 (ADR 0142): the browser federated-login legs — OIDC authorization-code + PKCE, default-OFF. -Two GET routes, both unauthenticated, modelled closely on ``routes/sso.py``: +Three routes, all unauthenticated, modelled closely on ``routes/sso.py``: -* ``/ui/oidc/start`` mints a server-side flow, drops an opaque flow id in a short-lived ``__Host-`` - cookie, and 303s the browser to the IdP. -* ``/ui/oidc/callback`` re-binds cookie + ``state``, redeems the code, and lands the session. +* ``GET /ui/oidc/start`` renders the ASVS 3.7.3 "you are leaving this site" interstitial and stages + **nothing** — when the IdP is inside ``[security].organization_domains`` it delegates straight to + the POST leg instead. +* ``POST /ui/oidc/start`` mints a server-side flow, drops an opaque flow id in a short-lived + ``__Host-`` cookie, and 303s the browser to the IdP. +* ``GET /ui/oidc/callback`` re-binds cookie + ``state``, redeems the code, and lands the session. **Registration is self-gating.** ``register`` returns before declaring either route unless ``[auth].oidc_enabled`` is set, so with federation off the two paths are not in the route table at all @@ -14,10 +17,18 @@ Two deliberate departures a reviewer will want to check rather than "fix": -* **No same-origin assertion on either leg.** ``assert_same_origin`` rejects any request whose - ``Sec-Fetch-Site`` is ``cross-site``, and the IdP's redirect back here is *legitimately* a top-level - cross-site navigation. Adding it would 403 every real federated login while every hermetic test - still passed (test clients send no ``Sec-Fetch`` headers). ``routes/sso.py`` does not call it either. +* **No same-origin assertion on the CALLBACK leg**, and that carve-out is specific to it. + ``assert_same_origin`` rejects any request whose ``Sec-Fetch-Site`` is ``cross-site``, and the IdP's + redirect back here is *legitimately* a top-level cross-site navigation. Adding it there would 403 + every real federated login while every hermetic test still passed (test clients send no + ``Sec-Fetch`` headers). ``routes/sso.py`` does not call it either. + ⛔ **The START legs are the opposite case and DO assert it** (ASVS 3.5.1). They are reached from our + own login page or our own interstitial form — never legitimately cross-site. Without the assertion + the 3.7.3 GET/POST split merely MOVES the drive-by sign-in hole from the GET to the POST: a + cross-site ``
`` is still ``Sec-Fetch-Mode: navigate``, so the navigate check does + not stop it. The first version of that split shipped without the assertion and its commit message + claimed otherwise; this is the correction. A bookmarked or typed navigation is unaffected — + ``Sec-Fetch-Site: none`` is not cross-site, and a request carrying neither header raises nothing. * **The callback returns 200 + a meta refresh, never a 303.** See :func:`pages.oidc_landing`. Ordering rule inherited from ``sso.py``: **every audit-writing branch sits behind the rate limiter.** @@ -38,11 +49,13 @@ from .. import pages from .._auth import ( + assert_same_origin, clear_oidc_flow_cookie, oidc_flow_cookie_name, set_oidc_flow_cookie, set_session_cookie, ) +from .._external import is_allowlisted, is_external, is_idn_disguised _log = logging.getLogger(__name__) @@ -77,8 +90,84 @@ def register(app: FastAPI, deps: UiDeps) -> None: if not deps.oidc_enabled: return + def _interstitial_needed() -> bool: + """Does the configured IdP sit outside the organization (ASVS 3.7.3)? + + Decided ENTIRELY from configuration — never from request input. The destination shown to the + operator, and the decision to show it at all, both come from ``deps``; a version of this that + took the URL from the request would make the interstitial an open redirect, which is strictly + worse than not having one. + """ + if not deps.external_link_interstitial: + return False + host = deps.oidc_authorization_host + if not host: + # Unknown destination is not a reason to skip the warning. + return True + url = f"https://{host}/" + if is_allowlisted(url, deps.external_link_allowlist): + return False + return is_external(url, deps.organization_domains) + @app.get("/ui/oidc/start") + async def ui_oidc_interstitial(request: Request) -> Response: + """ASVS 3.7.3: interpose "you are leaving this site", with a cancel, before the IdP hop. + + **No flow is staged here.** The old GET minted a PKCE flow and redirected in one step; the + flow now starts only when the operator confirms. Two things fall out of that: the bounded + flow cache cannot be drained by anyone who can cause a GET (it REJECTS when full, so that was + a login-DoS lever), and the confirm step is a POST behind the console's same-origin check — + which also closes the standing hole where any external page could start a federated sign-in + just by linking here. + + When the IdP is INSIDE ``organization_domains`` this page is skipped and the POST leg runs + directly, because ASVS asks about destinations outside the application's CONTROL and an + operator's own AD FS is not one. + """ + auth = get_auth(request) + if auth is None or not auth.oidc_enabled: + return RedirectResponse("/ui/login?e=oidc_unavailable", status_code=303) + if not _interstitial_needed(): + return await ui_oidc_start(request) + host = deps.oidc_authorization_host or "(not configured)" + return HTMLResponse( + pages.leaving_site( + destination_host=host, + continue_action="/ui/oidc/start", + idn_disguised=is_idn_disguised(f"https://{host}/"), + cancel_href="/ui/login", + purpose="Continuing will take you to your organization's sign-in provider.", + ), + status_code=200, + ) + + @app.post("/ui/oidc/start") async def ui_oidc_start(request: Request) -> Response: + """Confirmed: mint the flow and hand the browser to the IdP. + + ⚠️ **The rate-limit branch below must stay INSIDE this decorated handler.** + ``tests/test_security_doc_rate_limits.py`` reads the console's throttle shapes by walking the + AST of *decorated* route functions and looking for an ``allow_login_attempt`` branch. Hoisting + this body into a plain helper — which the first draft of the 3.7.3 split did — leaves the + limiter working and makes the gate blind to it, so the documented per-route breach shape + silently stops being checked. A control the checker cannot see is the failure mode this + codebase keeps rediscovering; the duplication of one ``await`` in the GET leg is cheaper. + """ + # ⛔ ASVS 3.5.1 — FIRST STATEMENT, and it must stay first. This is a document-initiated form + # POST from our own interstitial, so unlike the callback leg it is NEVER legitimately + # cross-site and the module docstring's "no same-origin assertion on either leg" carve-out + # does NOT extend here. + # + # Without it the 3.7.3 split MOVES the drive-by sign-in hole from GET to POST rather than + # closing it: a cross-site is still `Sec-Fetch-Mode: navigate`, so the + # navigate check below waves it through and a foreign page can still mint a flow and bounce + # the operator to the IdP. The first version of this change shipped without it and its commit + # message claimed the opposite — corrected here. + # + # Ordering note: this precedes the rate limiter deliberately, and does not violate the + # module's "every audit-writing branch sits behind the limiter" rule — it raises 403 and + # writes NO audit row, so it cannot be used as an audit-log amplifier. + assert_same_origin(request) auth = get_auth(request) if auth is None or not auth.oidc_enabled: # Disabled: redirect WITHOUT auditing — the sso.py anti-flood carve-out. Note this reads diff --git a/packaging/messagefoundry-webconsole/tests/test_webui.py b/packaging/messagefoundry-webconsole/tests/test_webui.py index c0ee5cc3..e5370383 100644 --- a/packaging/messagefoundry-webconsole/tests/test_webui.py +++ b/packaging/messagefoundry-webconsole/tests/test_webui.py @@ -5115,7 +5115,10 @@ async def test_oidc_login_link_tracks_availability(engine: Engine) -> None: # BOTH outcomes are 303, so the status alone cannot tell AC-8 compliance from the exact # regression this guards. Assert the DESTINATION: the start leg must still reach the IdP # while the advisory flag is set, not bounce to ?e=oidc_unavailable. - r = await c.get("/ui/oidc/start", follow_redirects=False) + # ASVS 3.7.3: flow-minting moved to the POST leg — the GET now renders the "leaving this + # site" interstitial and stages nothing. AC-8 is a property of the leg that ATTEMPTS, so + # this targets the POST. + r = await c.post("/ui/oidc/start", follow_redirects=False) assert r.status_code == 303 assert r.headers["location"].startswith("https://idp.example/authorize?") @@ -5124,7 +5127,14 @@ async def test_oidc_start_redirects_to_the_idp_and_sets_the_flow_cookie(engine: service = _oidc_service(engine) await service.initialize() async with _oidc_client(engine, service) as c: - r = await c.get("/ui/oidc/start", follow_redirects=False) + # ASVS 3.7.3: the GET renders the interstitial and mints NOTHING; the 303 + PKCE params + + # flow cookie this test is about now belong to the POST leg. Asserted here rather than + # retargeted silently, because "the start leg 303s to the IdP" stopped being true of the GET. + interstitial = await c.get("/ui/oidc/start", follow_redirects=False) + assert interstitial.status_code == 200 + assert "leaving" in interstitial.text.lower() + assert "set-cookie" not in {k.lower() for k in interstitial.headers} + r = await c.post("/ui/oidc/start", follow_redirects=False) assert r.status_code == 303 location = r.headers["location"] assert location.startswith("https://idp.example/authorize?") @@ -5242,7 +5252,14 @@ def _b64u_uint(v: int) -> str: await service.set_ad_group_map([("cn=mf-admins,dc=x", "administrator")], actor="admin") async with _oidc_client(engine, service) as c: - start = await c.get("/ui/oidc/start", follow_redirects=False) + # ASVS 3.7.3: a real operator now traverses the interstitial, so the round trip does too -- + # GET renders "you are leaving this site" and stages nothing, the Continue POST mints the + # flow and 303s. Kept as two hops rather than shortcut to the POST: this test's value is that + # it walks the path a browser actually walks. + leaving = await c.get("/ui/oidc/start", follow_redirects=False) + assert leaving.status_code == 200 + assert "leaving" in leaving.text.lower() + start = await c.post("/ui/oidc/start", follow_redirects=False) assert start.status_code == 303 params = dict(parse_qsl(urlsplit(start.headers["location"]).query)) flow_id = c.cookies.get("mf_oidc_flow") diff --git a/scripts/asvs/scorecard.py b/scripts/asvs/scorecard.py index 55b3276a..7168135f 100644 --- a/scripts/asvs/scorecard.py +++ b/scripts/asvs/scorecard.py @@ -116,6 +116,13 @@ class Cell: last_verified: str = "" verified_at: str = "" reviewed_by: str = "" + #: Owner has closed this cell: it is excluded from surveys, sweeps and rescores, and the loader + #: refuses it if the verdict has moved off the pin recorded alongside. Modelled on the Cell rather + #: than left as loose TOML so the renderer can surface it — a closure nobody can see is one a pass + #: will walk straight past, which is how this cell moved four times in eighteen days. + decision_closed: bool = False + decision_closed_on: str = "" + decision_closed_by: str = "" evidence: tuple[Anchor, ...] = () absence: tuple[Absence, ...] = () @@ -194,6 +201,34 @@ def load_scorecard(path: Path) -> list[Cell]: "recording the reason for non-applicability is the one MUST in ASVS 5.0's assessment " "chapter (docs/ASVS-ASSESSMENT-METHOD.md §1)" ) + # A cell the OWNER has closed is not re-scorable by a survey, sweep or agent. The stop was + # written in prose first and prose is not a gate: the reason this cell needed closing at all + # is that four different passes each believed they were doing careful work, and a rationale + # they could read was never what stopped them. `decision_closed_verdict` pins the verdict as + # of the ruling, so a later verdict change is DETECTABLE rather than merely discouraged. + # + # Deliberately not a warning. The cost of a false stop is one conversation with the owner; the + # cost of a silent re-score is a posture document that disagrees with the record and is + # discovered months later by a reader — which has already happened here, four times in + # eighteen days on the one cell this rule was written for. + if raw.get("decision_closed") is True: + pinned = str(raw.get("decision_closed_verdict", "")).lower() + if not pinned: + raise ScorecardError( + f"cell {raw.get('id')!r}: `decision_closed = true` without " + "`decision_closed_verdict` — the pin is what makes the closure checkable, so a " + "closure without one is a comment, not a control" + ) + if verdict != pinned: + raise ScorecardError( + f"cell {raw.get('id')!r}: verdict is {verdict!r} but this cell is CLOSED at " + f"{pinned!r} (`decision_closed = true`, closed " + f"{raw.get('decision_closed_on', 'date not recorded')} by " + f"{raw.get('decision_closed_by', 'owner')}). Re-scoring a closed cell needs an " + "explicit owner instruction — not a sweep's own judgement. If you hold one, move " + "the pin in the SAME commit and say so in the message; if you do not, revert the " + "verdict. See `decision_reopen_requires` on the cell" + ) for a in raw.get("absence", []): if not str(a.get("mutation", "")).strip(): raise ScorecardError( @@ -216,6 +251,9 @@ def load_scorecard(path: Path) -> list[Cell]: verdict=verdict, # type: ignore[arg-type] residual=str(raw.get("residual", "")), posture=str(raw.get("posture", "single")), + decision_closed=raw.get("decision_closed") is True, + decision_closed_on=str(raw.get("decision_closed_on", "")), + decision_closed_by=str(raw.get("decision_closed_by", "")), last_verified=str(raw.get("last_verified", "")), verified_at=str(raw.get("verified_at", "")), reviewed_by=str(raw.get("reviewed_by", "")), @@ -521,6 +559,29 @@ def render_current(cells: list[Cell], *, anchor_sha: str) -> str: for c in sorted((c for c in cells if c.verdict in open_states), key=lambda c: _sort_key(c.id)): seen = c.last_verified or "—" lines.append(f"| {c.id} | L{c.level} | **{c.verdict}** | {seen} | {c.residual[:150]} |") + + # Closed cells render even though they are not "open", and the reason is a defect this renderer + # caused. 11.7.1 was closed by owner decision while it was a `fail`, so its STOP text surfaced + # here — then the same ruling moved it to `na`, it dropped out of `open_states`, and the record's + # rendered face went silent about the one cell that had just been the subject of a ruling. A + # closure that is visible only while the verdict happens to be open is not a closure. + closed = sorted((c for c in cells if c.decision_closed), key=lambda c: _sort_key(c.id)) + if closed: + lines += [ + "", + "## Closed by owner decision — do not re-score", + "", + "These cells are **excluded from surveys, sweeps and rescores**, whatever a pass's own", + "instructions say. Re-scoring one needs an explicit owner instruction; the loader refuses", + "a closed cell whose verdict has moved off its pin, so this is enforced, not advisory.", + "", + "| Cell | L | Verdict | Closed | By |", + "|---|---|---|---|---|", + ] + for c in closed: + when = c.decision_closed_on or "—" + who = c.decision_closed_by or "owner" + lines.append(f"| {c.id} | L{c.level} | **{c.verdict}** | {when} | {who} |") return chr(10).join(lines) + chr(10) diff --git a/tests/golden/webconsole_seam.snapshot b/tests/golden/webconsole_seam.snapshot index 3bcfbb5b..7ada857d 100644 --- a/tests/golden/webconsole_seam.snapshot +++ b/tests/golden/webconsole_seam.snapshot @@ -5,7 +5,7 @@ # This is a GOLDEN gate: any diff means the seam contract changed - see the test's failure hint. ## ENGINE_UI_SEAM -16 +17 ## dataclass messagefoundry.api._ui_seam.UiDeps engine_seam @@ -16,6 +16,10 @@ default_scan_limit core admin oidc_enabled +organization_domains +external_link_interstitial +external_link_allowlist +oidc_authorization_host ## dataclass messagefoundry.api._ui_seam.CoreHandlers list_connections diff --git a/tests/test_asvs_scorecard.py b/tests/test_asvs_scorecard.py index ffdc13c6..b1df934e 100644 --- a/tests/test_asvs_scorecard.py +++ b/tests/test_asvs_scorecard.py @@ -417,6 +417,89 @@ def test_na_with_a_rationale_is_accepted(tmp_path: Path) -> None: assert load_scorecard(sc)[0].verdict == "na" +def test_a_closed_cell_whose_verdict_still_matches_its_pin_loads(tmp_path: Path) -> None: + """The green half. Without this, the red tests below could pass by refusing every closed cell.""" + sc = _scorecard_file( + tmp_path, + '[[cell]]\nid = "11.7.1"\nlevel = 3\nverdict = "na"\nresidual = "out of declared scope"\n' + 'decision_closed = true\ndecision_closed_verdict = "na"\n' + 'decision_closed_on = "2026-08-02"\ndecision_closed_by = "owner"\n', + ) + assert load_scorecard(sc)[0].verdict == "na" + + +def test_rescoring_a_closed_cell_is_refused(tmp_path: Path) -> None: + """The whole point: a survey cannot quietly re-grade a cell the owner closed. + + This was prose before it was a gate, and prose is not what stops a sweep — the cell this rule + exists for moved FOUR times in eighteen days, each pass believing it was doing careful work. A + rationale they could read was never the thing that stopped them. + """ + sc = _scorecard_file( + tmp_path, + '[[cell]]\nid = "11.7.1"\nlevel = 3\nverdict = "fail"\nresidual = "re-graded by a sweep"\n' + 'decision_closed = true\ndecision_closed_verdict = "na"\n', + ) + with pytest.raises(ScorecardError, match="CLOSED at"): + load_scorecard(sc) + + +def test_a_closure_without_a_pinned_verdict_is_refused(tmp_path: Path) -> None: + """A closure with nothing to compare against is a comment, not a control. + + The failure mode it forecloses: someone writes `decision_closed = true`, believes the cell is + protected, and the checker has no way to tell a re-score from the original verdict. + """ + sc = _scorecard_file( + tmp_path, + '[[cell]]\nid = "11.7.1"\nlevel = 3\nverdict = "na"\nresidual = "out of declared scope"\n' + "decision_closed = true\n", + ) + with pytest.raises(ScorecardError, match="the pin is what makes the closure checkable"): + load_scorecard(sc) + + +def test_a_closed_cell_is_rendered_even_when_its_verdict_is_not_an_open_state() -> None: + """The regression this exists for: a closure visible only while the verdict is open. + + 11.7.1 was closed while it was a `fail`, so its stop text surfaced in the open-cells table — then + the same ruling moved it to `na`, it dropped out of `open_states`, and the rendered record went + silent about the one cell that had just been ruled on. Closure visibility must not depend on which + verdict the cell happens to hold. + """ + out = render_current( + [ + Cell( + id="11.7.1", + level=3, + verdict="na", + residual="out of declared scope", + decision_closed=True, + decision_closed_on="2026-08-02", + decision_closed_by="owner", + ) + ], + anchor_sha="x", + ) + assert "Closed by owner decision" in out + assert "| 11.7.1 | L3 | **na** | 2026-08-02 | owner |" in out + + +def test_the_closed_section_is_absent_when_no_cell_is_closed() -> None: + """Negative control on REACH: the heading must not appear for a scorecard with no closures.""" + out = render_current([Cell(id="1.1.1", level=1, verdict="pass")], anchor_sha="x") + assert "Closed by owner decision" not in out + + +def test_an_unclosed_cell_is_unaffected_by_the_closure_rule(tmp_path: Path) -> None: + """Negative control on the rule's REACH: it must not police cells that never opted in.""" + sc = _scorecard_file( + tmp_path, + '[[cell]]\nid = "1.1.1"\nlevel = 1\nverdict = "fail"\nresidual = "no control"\n', + ) + assert load_scorecard(sc)[0].verdict == "fail" + + def test_needs_review_is_a_valid_verdict(tmp_path: Path) -> None: """Parking a contested cell beats forcing a premature verdict — that is what flip-flops.""" sc = _scorecard_file(tmp_path, '[[cell]]\nid = "1.1.1"\nlevel = 1\nverdict = "needs-review"\n') diff --git a/tests/test_external_link_interstitial.py b/tests/test_external_link_interstitial.py new file mode 100644 index 00000000..0b6583ba --- /dev/null +++ b/tests/test_external_link_interstitial.py @@ -0,0 +1,115 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""The ASVS 3.7.3 predicate: is this navigation leaving the organisation? + +The interstitial itself is UI; this is the part that decides whether it appears, so it is the part +that has to be right. Two of these tests exist because the research on 3.7.3 named the exact ways a +naive implementation is worse than none: a suffix match without a label boundary admits the +attacker's lookalike, and a decoded IDN shows the operator a host that is not the one resolved. +""" + +from __future__ import annotations + +import pytest + +from messagefoundry_webconsole._external import ( + display_host, + host_of, + is_allowlisted, + is_external, + is_idn_disguised, +) + +ORG = ["hospital.example"] + + +# --- the label-boundary hole ----------------------------------------------------------------- + + +def test_a_lookalike_domain_is_external_not_internal() -> None: + """`evilhospital.example` ENDS WITH `hospital.example`. A bare endswith() calls it internal. + + This is the single most valuable test in the file: getting it wrong turns the allowlist into the + attacker's tool, and the failure is silent — the operator sees no warning at all. + """ + assert is_external("https://evilhospital.example/login", ORG) is True + + +def test_the_org_domain_itself_and_its_subdomains_are_internal() -> None: + assert is_external("https://hospital.example/x", ORG) is False + assert is_external("https://adfs.hospital.example/adfs/ls", ORG) is False + assert is_external("https://deep.sub.hospital.example/x", ORG) is False + + +def test_a_third_party_idp_is_external() -> None: + """The case that decides the SSO leg: Entra is trusted, and is not the hospital.""" + assert is_external("https://login.microsoftonline.com/tenant/oauth2/authorize", ORG) is True + + +# --- secure-by-default ------------------------------------------------------------------------ + + +def test_with_no_org_domains_configured_everything_absolute_is_external() -> None: + """An operator who configures nothing must get the notification, not silently get none.""" + assert is_external("https://adfs.hospital.example/x", []) is True + + +def test_relative_urls_are_never_external() -> None: + """They cannot leave the origin, so warning on them would train click-through for nothing.""" + assert is_external("/ui/login", ORG) is False + assert is_external("/ui/messages?q=1", []) is False + + +@pytest.mark.parametrize( + "url", ["javascript:alert(1)", "data:text/html,x", "file:///etc/passwd"] +) +def test_non_navigable_schemes_are_refused_as_external(url: str) -> None: + """Not a navigation we should help complete. Callers refuse; `True` keeps them out of the + silent-pass branch.""" + assert is_external(url, ORG) is True + + +def test_an_unparseable_host_is_treated_as_external() -> None: + """Fail toward showing the interstitial. `host_of` returning '' must never read as internal.""" + assert host_of("https://") == "" + assert is_external("https://", ORG) is True + + +# --- the IDN homograph ------------------------------------------------------------------------ + + +def test_an_idn_homograph_host_is_reported_in_punycode_not_unicode() -> None: + """Cyrillic 'а' + 'mazon.com' renders identically to the Latin form. + + Showing the decoded Unicode would show the operator a host that is NOT the one resolved, which + makes the interstitial actively misleading — worse than absent. + """ + homograph = "https://аmazon.example/" + assert display_host(homograph).startswith("xn--") + assert display_host(homograph) != "amazon.com" + assert is_idn_disguised(homograph) is True + + +def test_a_plain_ascii_host_is_not_flagged_as_disguised() -> None: + """Negative control on the flag's REACH — without it, a warning that fires on everything + carries no information.""" + assert is_idn_disguised("https://adfs.hospital.example/x") is False + assert display_host("https://adfs.hospital.example/x") == "adfs.hospital.example" + + +def test_a_homograph_of_an_org_domain_is_still_external() -> None: + """The two defences composed: the lookalike must not inherit the org's internal status.""" + assert is_external("https://hospital.examplе/x", ORG) is True # Cyrillic 'о' + + +# --- the audited escape ----------------------------------------------------------------------- + + +def test_the_allowlist_exempts_a_destination_and_respects_the_label_boundary() -> None: + assert is_allowlisted("https://docs.vendor.example/help", ["vendor.example"]) is True + assert is_allowlisted("https://notvendor.example/help", ["vendor.example"]) is False + + +def test_an_empty_allowlist_exempts_nothing() -> None: + """Reach control: the escape must do nothing until an operator opts in.""" + assert is_allowlisted("https://anything.example/", []) is False diff --git a/tests/test_security_doc_drift.py b/tests/test_security_doc_drift.py index 3553af88..f7890811 100644 --- a/tests/test_security_doc_drift.py +++ b/tests/test_security_doc_drift.py @@ -430,6 +430,16 @@ # ALERT — no login, session or authorization outcome turns on it (contrast its sibling # `bootstrap_expiry_hours`, which DISABLES the account and is therefore an inventoried input). "bootstrap_warn_hours", + # ASVS 3.7.3: destinations exempted from the "you are leaving this site" interstitial. It + # decides whether the operator is SHOWN A NOTIFICATION before an outbound navigation — not + # whether any request is authorized. No login, session, permission or authorization outcome + # turns on it, and it is never read on an inbound request path at all. + # + # ⚠️ It IS a security-relevant setting and it LOWERS security when non-empty, which is why the + # serve gate warns and names every entry. That makes it a settings-reference concern, not an + # 8.1.3/8.1.4 contextual-input one — the two are different questions and this list is the + # place the difference gets recorded rather than assumed. + "external_link_allowlist", } ) diff --git a/tests/test_ui_oidc_interstitial_route.py b/tests/test_ui_oidc_interstitial_route.py new file mode 100644 index 00000000..7ac987a7 --- /dev/null +++ b/tests/test_ui_oidc_interstitial_route.py @@ -0,0 +1,224 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""ASVS 3.7.3 at the ROUTE level: does the interstitial actually interpose? + +``test_external_link_interstitial.py`` proves the predicate decides correctly. That is necessary and +not sufficient — a correct predicate wired to nothing still ships a console that redirects off-site +silently. These tests exercise the registered routes. + +The registrar is driven directly with a hand-built :class:`UiDeps` rather than through ``create_app``: +the policy reaches the route as CONFIG (seam v17) precisely so it does not depend on the AuthService, +and testing it that way keeps the test honest about which layer is under test. +""" + +from __future__ import annotations + +from typing import Any + +import pytest +from fastapi import FastAPI +from starlette.testclient import TestClient + +from messagefoundry.api._ui_seam import UiDeps +from messagefoundry_webconsole.routes import oidc as oidc_routes + + +class _FakeAuth: + """The narrow slice of AuthService the start legs touch before they would reach the IdP.""" + + oidc_enabled = True + oidc_flow_ttl_seconds = 300 + + def allow_login_attempt(self, _client: str | None) -> bool: + return True + + async def audit_oidc_reject(self, _reason: str) -> None: # pragma: no cover - not reached here + return None + + +def _client(**policy: Any) -> TestClient: + app = FastAPI() + deps = UiDeps( + engine_seam=0, + get_engine=lambda: None, + get_gate=lambda: None, + cookie_secure=lambda *_a, **_k: False, + default_scan_limit=100, + core=None, # type: ignore[arg-type] + admin=None, # type: ignore[arg-type] + oidc_enabled=True, + **policy, + ) + oidc_routes.register(app, deps) + app.state.auth = _FakeAuth() + app.state.public_origin = "" # start leg bails before the IdP; we assert on the interstitial + return TestClient(app, follow_redirects=False) + + +def test_an_external_idp_gets_the_interstitial_not_a_redirect() -> None: + """The control itself: a third-party IdP must produce a page with a destination and a cancel.""" + r = _client( + organization_domains=("hospital.example",), + oidc_authorization_host="login.microsoftonline.com", + ).get("/ui/oidc/start") + assert r.status_code == 200 + body = r.text + assert "You are leaving this site" in body or "leaving" in body.lower() + assert "login.microsoftonline.com" in body + assert 'method="post"' in body # Continue is a form, not a link + assert "Cancel" in body + + +def test_an_internal_idp_is_not_interstitialed() -> None: + """An operator's own AD FS is a different host and still inside their control. + + Asserted as "not the interstitial" rather than "is a 303": the start leg has several legitimate + 303 outcomes and pinning one would make this test about flow plumbing instead of about 3.7.3. + """ + r = _client( + organization_domains=("hospital.example",), + oidc_authorization_host="adfs.hospital.example", + ).get("/ui/oidc/start") + assert r.status_code != 200 or "leaving" not in r.text.lower() + + +def test_with_no_org_domains_even_a_plausible_idp_is_interstitialed() -> None: + """Secure-by-default reaches the route, not just the predicate.""" + r = _client(organization_domains=(), oidc_authorization_host="adfs.hospital.example").get( + "/ui/oidc/start" + ) + assert r.status_code == 200 + assert "adfs.hospital.example" in r.text + + +def test_the_allowlist_escape_suppresses_the_interstitial() -> None: + """The documented escape works — and this test exists so the warning in the docs is not a lie.""" + r = _client( + organization_domains=("hospital.example",), + external_link_allowlist=("login.microsoftonline.com",), + oidc_authorization_host="login.microsoftonline.com", + ).get("/ui/oidc/start") + assert r.status_code != 200 or "leaving" not in r.text.lower() + + +def test_turning_the_interstitial_off_suppresses_it() -> None: + r = _client( + organization_domains=("hospital.example",), + external_link_interstitial=False, + oidc_authorization_host="login.microsoftonline.com", + ).get("/ui/oidc/start") + assert r.status_code != 200 or "leaving" not in r.text.lower() + + +def test_an_unconfigured_idp_host_still_gets_the_interstitial() -> None: + """Unknown destination is not a reason to skip the warning — fail toward showing it.""" + r = _client(organization_domains=("hospital.example",), oidc_authorization_host="").get( + "/ui/oidc/start" + ) + assert r.status_code == 200 + + +def test_the_interstitial_page_carries_no_destination_url_to_post_back() -> None: + """⭐ The one that stops this being an open redirect. + + If the rendered page carried the target URL in a form field or query string, the POST leg would + be steerable by anyone who could get an operator to load a crafted page — an interstitial that is + itself an open redirect, which is strictly worse than having none. The destination must live in + configuration and server-side state only. + """ + r = _client( + organization_domains=("hospital.example",), + oidc_authorization_host="login.microsoftonline.com", + ).get("/ui/oidc/start") + assert r.status_code == 200 + assert "https://login.microsoftonline.com" not in r.text # host shown, full URL never + assert 'action="/ui/oidc/start"' in r.text # posts back to us, carrying nothing + + +def test_a_cross_site_post_to_the_start_leg_is_refused() -> None: + """⛔ ASVS 3.5.1 — the assertion the rest of this file could not make. + + Every other test here sends NO `Sec-Fetch-*` headers, so the origin guard never fires and they + would all pass just as happily with it deleted. This one supplies the header a real browser sends + on a cross-site form submission and asserts the 403. + + Why it matters specifically to 3.7.3: the GET/POST split moved flow-minting behind a POST, and a + cross-site `` is still `Sec-Fetch-Mode: navigate` — so the navigate check does + NOT stop it. Without the origin guard the split relocates the drive-by sign-in hole rather than + closing it, which is what the first version of this change did while claiming otherwise. + """ + c = _client( + organization_domains=("hospital.example",), + oidc_authorization_host="login.microsoftonline.com", + ) + r = c.post( + "/ui/oidc/start", headers={"Sec-Fetch-Site": "cross-site", "Sec-Fetch-Mode": "navigate"} + ) + assert r.status_code == 403 + + +def test_a_same_origin_post_is_not_refused_by_the_guard() -> None: + """Reach control. Without it, a guard that 403s EVERYTHING would pass the test above.""" + c = _client( + organization_domains=("hospital.example",), + oidc_authorization_host="login.microsoftonline.com", + ) + r = c.post( + "/ui/oidc/start", headers={"Sec-Fetch-Site": "same-origin", "Sec-Fetch-Mode": "navigate"} + ) + assert r.status_code != 403 + + +def test_a_cross_site_GET_renders_the_interstitial_and_stages_nothing() -> None: + """The GET is deliberately NOT origin-guarded, and the reason is worth stating precisely. + + An external page CAN link here and get our interstitial rendered. That is acceptable because the + page is **side-effect free**: no flow is staged, no cookie is set, nothing is minted, and the only + way onward is a Continue button that POSTs — and the POST *is* guarded (see the two tests above). + So a drive-by can cause a harmless page to render; it cannot cause a sign-in to start. + + ⚠️ Asserted here rather than left implicit because the first version of this change claimed the + split "closed the standing hole where any external page could start a federated sign-in by linking + here". Half true: the LINK no longer starts one. What actually closes it is the origin guard on + the POST, not the split. + """ + c = _client( + organization_domains=("hospital.example",), + oidc_authorization_host="login.microsoftonline.com", + ) + r = c.get( + "/ui/oidc/start", headers={"Sec-Fetch-Site": "cross-site", "Sec-Fetch-Mode": "navigate"} + ) + assert r.status_code == 200 + assert "leaving" in r.text.lower() + assert not r.cookies # nothing staged + + +def test_a_cross_site_GET_with_an_INTERNAL_idp_is_refused() -> None: + """⛔ The asymmetry, asserted so it cannot be forgotten. + + With an internal IdP there is no interstitial, so the GET delegates straight to the minting leg — + and inherits its origin guard. The consequence a reader needs: for an INTERNAL-IdP deployment the + GET still mints a flow, exactly as before this change. The bounded-flow-cache DoS lever is closed + for external IdPs (where the interstitial interposes) and is merely ORIGIN-GUARDED for internal + ones. The earlier claim that the split closed it outright was too broad. + """ + c = _client( + organization_domains=("hospital.example",), + oidc_authorization_host="adfs.hospital.example", + ) + r = c.get( + "/ui/oidc/start", headers={"Sec-Fetch-Site": "cross-site", "Sec-Fetch-Mode": "navigate"} + ) + assert r.status_code == 403 + + +@pytest.mark.parametrize("method", ["get", "post"]) +def test_both_start_legs_exist(method: str) -> None: + """The GET renders, the POST acts. Losing either silently breaks sign-in or the control.""" + c = _client( + organization_domains=("hospital.example",), + oidc_authorization_host="login.microsoftonline.com", + ) + r = getattr(c, method)("/ui/oidc/start") + assert r.status_code != 405