From b94cc832172d6c7174ce0dde5bfad33fdde7cd53 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Sun, 2 Aug 2026 19:11:02 -0500 Subject: [PATCH 01/15] docs(asvs): make the scope boundary explicit, and correct a published attestation that was not true Two public documents, one cause: the assessment method never wrote down where the software ends and the substrate begins, so a platform-property requirement had no principled home and churned between verdicts -- and separately, a published attestation claimed more than the record supports. ASVS-ASSESSMENT-METHOD.md * Section 2 now states the boundary the "assessed as source" phrase already implied: the subject is three SOFTWARE ARTIFACTS, not the host, hypervisor, CPU, firmware or network. Where a requirement's VERB names a property of that substrate, rule 1 applies. This is ASVS's own principle and is now quoted verbatim from 0x03 at the v5.0.0 tag rather than paraphrased -- "excludes requirements ... where configuration is outside the application's responsibility", with the DNS example. * Two guards, because a scope clause grows to swallow inconvenient cells: reporting on a platform property is not providing it AND is not disqualifying either -- ask what the verb requires, and of what. * Explicitly declines the fork route. 0x03 does say organizations are "strongly encouraged" to fork, but its omission examples are technology-not-used (GraphQL/WebSockets/SOAP) and forking changes what you claim conformance TO. Rule 1 against a declared scope is narrower and leaves the target unchanged. * New section 2.1: what an out-of-scope cell does NOT buy. 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 -- so an L3 claim omitting an L3 requirement is non-conformant on OWASP's own terms. * 11.7.1 moves from the worked example of a rule-3 fail to the worked example of a rule-1 na, with the objection it has to answer stated rather than omitted. Secure_Development_Standards.md * "verified against OWASP ASVS 5.0 Level 3" -> "assessed against ASVS 5.0 using Level 3 as the target -- an assessment IN PROGRESS". The old wording was unsupportable three ways: the survey is incomplete and unread cells are explicitly not passes; open requirements exist including at L2; and an L3 requirement is scoped out, which under 5.0 does not preserve an L3 claim. * The correction is stated in the document rather than made quietly, because the claim was PUBLISHED. No figure is restated here, deliberately, so this page cannot go stale against the record. * Notes that no accredited L3 pathway exists to appeal to: OWASP certifies nobody, and the one ASVS-based scheme accredits testing FIRMS at L1-L2. --- docs/ASVS-ASSESSMENT-METHOD.md | 51 +++++++++++++++++++++++++++- docs/Secure_Development_Standards.md | 11 +++++- 2 files changed, 60 insertions(+), 2 deletions(-) diff --git a/docs/ASVS-ASSESSMENT-METHOD.md b/docs/ASVS-ASSESSMENT-METHOD.md index 13dc557a..97807652 100644 --- a/docs/ASVS-ASSESSMENT-METHOD.md +++ b/docs/ASVS-ASSESSMENT-METHOD.md @@ -62,7 +62,7 @@ These are the real disputes. They are here so the next assessor reaches the same |---|---|---| | **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. | | **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** | `fail` | Rule 3. The read-out is all-`None` off Linux and the only refusing branch keys on a **TOML declaration**, never on the measured property. Zero percent of the verb is satisfied by shipped code. | +| **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. | | **10.5.5** | `na` | Rule 1. The requirement is conditional — *"**when using** OIDC back-channel logout"* — and the precondition is false and unreachable by configuration. **Building it would create applicability.** | | **12.2.2** | `na` | Rule 1. No external-facing services on the declared scope. *Also a scoping error worth remembering: this row spent months scoring 12.3.1's verb.* | @@ -86,6 +86,55 @@ Per ASVS's guidance, stated as what **is** included rather than what is excluded **Included:** the MessageFoundry engine, the web console, and the IDE extension, assessed as source, at a named commit, against **all 345 ASVS 5.0.0 requirements** at **Level 3**. +**The boundary that phrase implies, stated explicitly because it decides verdicts.** The subject of +this assessment is those three **software artifacts**. It is not the host, the hypervisor, the CPU, the +firmware, or the network the operator deploys onto. Where a requirement's **verb** names a property of +that substrate rather than of the software, the requirement is outside the declared scope and takes +`na` under rule 1 — with the rationale written, as always. + +**This is ASVS's own principle, not a local invention.** Verbatim from `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." + +⚠️ **Do not over-read that, and do not reach for the fork clause to do this job.** The same chapter says +organizations are *"strongly encouraged to create an organization- or domain-specific fork that adjusts +requirements"* — but its worked examples of omission are **technology-not-used** (*"omitting irrelevant +sections (e.g., GraphQL, WebSockets, SOAP, if unused)"*), which is the functionality-based shape, and +**forking changes what you are claiming conformance TO**: your tailored ASVS, not stock ASVS 5.0. This +project does **not** fork. It applies rule 1 against a positively-declared scope, which is narrower, +cheaper to defend, and leaves the conformance target unchanged. + +Two guards on that boundary, because it is exactly the kind of clause that grows to swallow +inconvenient cells: + +* **Shipping code that *reports on* a platform property is not the same as *providing* it, and neither + direction is decisive on its own.** A cell does not become in-scope merely because the engine + observes the substrate; nor does it leave scope merely because the substrate is involved. Ask what + the **verb** requires to be true, and of what. +* ⛔ **This boundary does not shrink the conformance claim's denominator.** A requirement excluded here + is still a requirement OWASP assigns to Level 3. See §2.1. + +### 2.1 What an out-of-scope cell does NOT buy + +**It does not preserve an unqualified Level 3 claim.** Two facts, both from ASVS 5.0.0 itself: + +* **4.0's clause that an organization excluding requirements "may still claim full ASVS compliance" + was DROPPED in 5.0.** The 5.0 text says only that non-applicability must be noted in the report. + There is no longer any standard text saying a documented exclusion preserves a compliance claim, and + a rationale that cites the older wording is citing a superseded standard. +* **OWASP does not certify anyone — but it does retain normative authority over the requirement SET.** + It assigns each requirement to a level. So a Level 3 claim that silently omits a requirement OWASP + places at Level 3 is non-conformant **on OWASP's own terms**, regardless of how well-argued the + exclusion is. + +**Therefore:** scoping a cell out is a statement about *what was assessed*, never a statement that the +level was achieved anyway. Any published attestation must say which requirements were excluded, or say +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. + **The configuration assessed** — one posture, not a matrix: > On-premises single hospital · private network, never internet-facing · SQL Server store · operator diff --git a/docs/Secure_Development_Standards.md b/docs/Secure_Development_Standards.md index c1f916e0..bae43e9d 100644 --- a/docs/Secure_Development_Standards.md +++ b/docs/Secure_Development_Standards.md @@ -337,7 +337,16 @@ The project maintains a current evidence set so any claim is backed: - **Per-project applicability profile** (Appendix A and onward). - A **claims register** recording each published claim, its wording, and the evidence behind it. -**Attestation posture.** The software is self-attested as NIST SSDF–aligned, tested per NIST SP 800-115, verified against OWASP ASVS 5.0 Level 3, and built to support HIPAA-compliant deployment (controls mapped to NIST SP 800-66 Rev. 2). Third-party validation of the SSDF attestation and the ASVS 5.0 Level 3 assessment raises the weight of these claims. **Attestations are published with releases** so adopters can rely on them; each adopter still performs its own deployment risk assessment (§7.3). None of these is a NIST certificate; displayable certificates (SOC 2, ISO 27001, HITRUST) are a separate, organization-level track. +**Attestation posture.** The software is self-attested as NIST SSDF–aligned, tested per NIST SP 800-115, **assessed against** OWASP ASVS 5.0 **using Level 3 as the target** — an assessment **in progress**, not a completed verification — and built to support HIPAA-compliant deployment (controls mapped to NIST SP 800-66 Rev. 2). Third-party validation of the SSDF attestation and of the ASVS assessment raises the weight of these claims. + +> **The ASVS wording above changed on 2026-08-02, and the previous wording was wrong.** It read *"verified against OWASP ASVS 5.0 Level 3."* Three facts make that unsupportable, and they are stated here rather than quietly corrected because the claim was **published**: +> 1. **The survey is incomplete.** A minority of the 345 requirements have been read against the ASVS text at a known commit; the remainder are recorded as *unverified*, which is explicitly **not** a pass. "Verified" asserted a completed verification that had not occurred. +> 2. **Open requirements exist**, including at Level 2 — so the claim was not rescuable by narrowing it to a lower level. +> 3. **At least one Level 3 requirement is scoped out** as a hosting-platform property outside the assessed software. Under ASVS 5.0 that does **not** preserve a Level 3 claim: 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. A Level 3 claim omitting a Level 3 requirement is non-conformant on OWASP's own terms. +> +> **No accredited Level 3 pathway exists to appeal to.** OWASP certifies no vendor or software and states that any trust mark claiming ASVS compliance is not officially endorsed by it; the one ASVS-based accreditation scheme accredits testing *firms*, and its published scope is Levels 1–2. So "self-attested" is the only honest register available here, and this section now uses it. +> +> Current status is held in the project's private assessment record, which is the count of record; **no figure is restated here**, deliberately, so this page cannot go stale against it. **Attestations are published with releases** so adopters can rely on them; each adopter still performs its own deployment risk assessment (§7.3). None of these is a NIST certificate; displayable certificates (SOC 2, ISO 27001, HITRUST) are a separate, organization-level track. --- From d3eb4e5728d60d45118c032566f10484ef8ae511 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Sun, 2 Aug 2026 19:14:42 -0500 Subject: [PATCH 02/15] docs(asvs): a count movement is not a posture movement -- write down the four causes When 11.7.1 was scoped out under rule 1, the fail count went 3 -> 2 with ZERO lines of engine code changed. A reader comparing only the totals across those two days concludes a defect was fixed. Nothing was. The rationale sits on the cell and the boundary sits in section 2, but neither is visible to someone reading a number. New section 2.2 names the four causes a bucket total moves, of which exactly one is an improvement: a control shipped on by default (real); a cell read for the first time (the survey advanced -- unverified -> pass is a DISCOVERY, not a fix); a scope boundary stated (identical code, smaller denominator); a rule applied more carefully (more accurate, and these move DOWN too). Two obligations, both cheap: never report a total as a trend without naming which cause moved it, and when a count improves, state what would have had to happen for it to MEAN improvement and whether that happened -- the negative-control discipline applied to a metric. Written down because it cuts AGAINST us more often than for us. The survey is incomplete, so most future movement is cause 2, and the aggregate will get worse before it gets better as unverified cells resolve into real verdicts. That is the survey working, and it should be reported as such rather than defended against. Sits directly under 2.1 (an out-of-scope cell buys no Level 3 claim), because the two failure modes are the same one seen from opposite ends: 2.1 stops a scoped-out cell becoming a conformance claim, 2.2 stops it becoming a trend line. Prompted by the ASVS-11.7.1-summary session, whose objection -- that the rule-3 observations about the code stay literally true even after the verdict moves -- is what made the gap visible. --- docs/ASVS-ASSESSMENT-METHOD.md | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/docs/ASVS-ASSESSMENT-METHOD.md b/docs/ASVS-ASSESSMENT-METHOD.md index 97807652..7c0e2bdd 100644 --- a/docs/ASVS-ASSESSMENT-METHOD.md +++ b/docs/ASVS-ASSESSMENT-METHOD.md @@ -135,6 +135,37 @@ 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.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 +for four different reasons, and only one of them means the software got better: + +| The count moved because… | Did the posture improve? | What actually happened | +|---|---|---| +| A control was **built or turned on by default** | **Yes** | The verb is now satisfied by shipped code | +| 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 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:** + +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. +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. + +⚠️ **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 +the aggregate will get *worse* before it gets better as `unverified` cells resolve into real verdicts. +**That is the survey working, not a regression**, and it should be reported as such rather than +defended against. + **The configuration assessed** — one posture, not a matrix: > On-premises single hospital · private network, never internet-facing · SQL Server store · operator From e4a27c4b78fd0774fa6aab0860a3927d788c018e Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Sun, 2 Aug 2026 19:16:20 -0500 Subject: [PATCH 03/15] docs(asvs): the dangerous case is a posture that moves while the count stands still Section 2.2 as first written had a blind spot: it teaches a reader to interrogate a number that MOVED, and the worse 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 -- with the total perfectly stable. Stability reads as "nothing to see", which is why nobody goes looking. Not hypothetical, and not cause 4. Cause 4 is deliberate: someone re-read a cell. This is an evidence pointer breaking on its own. Measured here on 2026-08-02: SEVEN anchors across six cells had drifted and not one bucket total moved. It is catchable only by a gate, which is what ADR 0156's drift gate is for. Adds a fifth cause too: THE STANDARD ITSELF CAN MOVE. The 345 denominator is pinned to ASVS v5.0.0 by digest; a 5.0.x patch would change requirement text and counts with zero code change and zero assessment work -- and it is the cause most easily mistaken for the survey advancing. Third obligation, symmetric to the first two: a stable count is not evidence of a stable posture unless the anchors were re-verified in the same pass. Report the drift check with the total, or you are publishing the freshness of the last check rather than of the software. And state the pinned ASVS version with any total, so a denominator change surfaces as a version change instead of as progress. Both additions from the ASVS-11.7.1-summary session, which read section 2.2 out of the git object rather than trusting the relay and found the gap in it. --- docs/ASVS-ASSESSMENT-METHOD.md | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/docs/ASVS-ASSESSMENT-METHOD.md b/docs/ASVS-ASSESSMENT-METHOD.md index 7c0e2bdd..5f3641cc 100644 --- a/docs/ASVS-ASSESSMENT-METHOD.md +++ b/docs/ASVS-ASSESSMENT-METHOD.md @@ -146,19 +146,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." -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. +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 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 From a2a50f3db8729d501e309eb790d04ee06b6fc1d8 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Sun, 2 Aug 2026 19:24:29 -0500 Subject: [PATCH 04/15] feat(asvs): make a closed cell UNRE-SCORABLE by a gate, and answer the ADR that rejected this ruling Owner ruling: 11.7.1 is na, LOCKED as of 2026-08-02. Three changes make that true rather than merely written down. 1. THE CLOSURE IS NOW A GATE, NOT PROSE. load_scorecard refuses a cell carrying `decision_closed = true` whose verdict has moved off `decision_closed_verdict`, and refuses a closure with no pin at all (a closure with nothing to compare against is a comment, not a control). Deliberately an error, 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 found months later. Prose was already tried here and did not hold -- four passes each believed they were doing careful work, and a rationale they could read never stopped them. Four tests, and the two that matter were PROVED RED before being trusted: with the guard disabled both closure tests fail with "DID NOT RAISE ScorecardError", while the green-half test (verdict still matches its pin) and the reach control (a cell that never opted in is untouched) keep passing. Verified against the real vault data too: exit 0, 345 cells, 750 anchors, 86 absence claims. 2. ADR 0152's REJECTION OF THIS RULING IS ANSWERED IN PLACE. Its "N/A deferred" paragraph is the strongest attack on what just landed and is the record of one of the four verdict moves -- the one that went the other way. Its stated ground is FALSIFIED: "ASVS's enumerated grounds are absent functionality and external processes" is not what ASVS says. 0x03 at the v5.0.0 tag excludes requirements "where configuration is outside the application's responsibility", DNS example -- a responsibility exclusion, the exact category the paragraph denies exists. That error survived because the pinned corpus is REQUIREMENTS-ONLY and holds no chapter prose, so no check against it could ever catch a wrong claim about the standard's text. I re-asserted it myself before catching it. The adopted ruling also does not rest on the ground that paragraph rejects: not "the platform provides it", but rule 1 against a positively-declared scope of three SOFTWARE ARTIFACTS. And its "strictly weaker than measuring the property" assumed measuring was achievable -- research since establishes no attestation on any platform asserts memory encryption as a property. 3. ADOPTER GUIDANCE CORRECTED. SYSTEM-REQUIREMENTS.md and CONFIGURATION.md told adopters to "disclose 11.7.1 as Partial" -- pre-filling a verdict in someone else's assessment, and one this project no longer holds either. Both now say to assess it against their own deployment, with the correction stated rather than made quietly. Recorded and not buried: the fail count moved 3 -> 2 with ZERO engine code changed. The posture did not improve; the scope got stated. NOT mirrored to the vault yet -- the vault mirrors engine MAIN, and this is a branch. The f6d6f38b guard fires when this reaches main, not before. --- docs/CONFIGURATION.md | 6 ++- docs/SYSTEM-REQUIREMENTS.md | 9 ++-- ...mory-encryption-attestation-asvs-11-7-1.md | 51 +++++++++++++++++++ scripts/asvs/scorecard.py | 28 ++++++++++ tests/test_asvs_scorecard.py | 51 +++++++++++++++++++ 5 files changed, 140 insertions(+), 5 deletions(-) diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index fdb709e4..1dedb38b 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -1550,8 +1550,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/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 09f69638..5636df5a 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 @@ -259,6 +259,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/scripts/asvs/scorecard.py b/scripts/asvs/scorecard.py index 663ad95d..adf6864b 100644 --- a/scripts/asvs/scorecard.py +++ b/scripts/asvs/scorecard.py @@ -183,6 +183,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( diff --git a/tests/test_asvs_scorecard.py b/tests/test_asvs_scorecard.py index ebd6502a..01e0c552 100644 --- a/tests/test_asvs_scorecard.py +++ b/tests/test_asvs_scorecard.py @@ -390,6 +390,57 @@ 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_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') From 7b41ec0b1e4f846b76c56de662df15f6b366373d Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Sun, 2 Aug 2026 19:28:51 -0500 Subject: [PATCH 05/15] fix(asvs): a closure visible only while the verdict is open is not a closure Regression this renderer caused, inside one session. 11.7.1 was closed while it was a `fail`, so its STOP text surfaced in the Open-cells table -- the highest-traffic face of the record. The same ruling then moved it to `na`, it fell out of `open_states`, and the rendered record went SILENT about the one cell that had just been ruled on. A reader of ASVS-CURRENT.md would have seen nothing at all. Adds a "Closed by owner decision -- do not re-score" section that renders on `decision_closed`, independent of verdict, and promotes decision_closed/_on/_by onto the Cell so the renderer can see them rather than re-parsing raw TOML. Two tests, and the second is the one that matters: a closed `na` cell renders (the regression), AND the section is absent when nothing is closed (a control on the rule's REACH -- without it the first test passes just as well for a renderer that emits the heading unconditionally). Found by the ASVS-11.7.1-summary session's grounding audit, which flagged that the Cell record models no closure fields at all; checking that turned up the live case. --- scripts/asvs/scorecard.py | 33 +++++++++++++++++++++++++++++++++ tests/test_asvs_scorecard.py | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+) diff --git a/scripts/asvs/scorecard.py b/scripts/asvs/scorecard.py index adf6864b..f3be79f8 100644 --- a/scripts/asvs/scorecard.py +++ b/scripts/asvs/scorecard.py @@ -105,6 +105,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, ...] = () @@ -233,6 +240,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", "")), @@ -536,6 +546,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/test_asvs_scorecard.py b/tests/test_asvs_scorecard.py index 01e0c552..e8d0157d 100644 --- a/tests/test_asvs_scorecard.py +++ b/tests/test_asvs_scorecard.py @@ -432,6 +432,38 @@ def test_a_closure_without_a_pinned_verdict_is_refused(tmp_path: Path) -> None: 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( From a5e7b179e666fd046c5befd2dbbfe8dfe675d24d Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Sun, 2 Aug 2026 19:29:53 -0500 Subject: [PATCH 06/15] docs(phi): an unenforced prerequisite offered as a compensating control is a false premise PHI.md contradicted itself about one control, three hundred lines apart. Section 2 (:211-214) records plainly that there is NO [security].volume_encryption_declared setting at HEAD and that nothing in the engine checks FDE is on. Section 10's residual text (:584-586) then offered "the documented restricted-service-account + volume-encryption posture" as the compensating control for the in-use-memory residual, phrased as though it were something the product supplies. Both halves are operator-asserted and engine-unchecked. The claim is now qualified at the point of use rather than only in the section nobody reads alongside it, and it points at section 2's own statement so the two cannot drift apart again. Deliberately weakened, not removed: the posture IS worth doing and does mitigate where an operator applied it. What was wrong is the implication that the engine knows. CLAUDE.md section 11: a compensating control must not rest on a false premise. Found by the ASVS-11.7.1-summary session's grounding audit while checking what the 11.7.1 record would inherit -- verified against the code before acting (grep for the setting returns nothing). --- docs/PHI.md | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/docs/PHI.md b/docs/PHI.md index 27c8fbb3..a60de042 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 From 889ef3f7462a216b9ce2c6a19517ee0625de1cdf Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Sun, 2 Aug 2026 19:34:24 -0500 Subject: [PATCH 07/15] docs(backlog): reconcile the ledger's 11.7.1 verdicts with the record -- and the stale one was the Fail The ledger carried three verdict statements for 11.7.1 and they did not agree. After the 2026-08-02 owner ruling the picture inverted from what I expected: the two saying N/A (:415, :429) are now RIGHT, and the one saying Fail (:6390) is the stale one. :6390 said "13.3.3/11.7.1 remain **Fail**". Neither is a Fail on the record. 11.7.1 is `na`; 13.3.3 is `unverified` -- never read against the requirement text, which is explicitly NOT a verdict. Both re-derived from the scorecard before writing, not assumed. :415 verdict right, REASON wrong. "Unachievable for pure-Python on-prem" is not the ground -- the engine does ship ADR 0152 rungs 1-2; they REPORT ON the platform property rather than PROVIDE it. The ground is that the verb names a CPU/firmware property, outside the declared scope of three software artifacts. A right answer for a wrong reason is the thing that gets re-derived into a wrong answer later. :429 left standing but qualified: "residuals are closed" is a STAFFING judgement, not a coverage claim, and reads as a posture summary it cannot support while most cells have never been examined. Left alone deliberately: :245, :523, :6384, :6388, :6394 are build-history and deferral records, not verdict claims. An over-broad sweep is its own defect. None of these three sites matches the ledger gate's `^#{2,3} (\d+)\.` heading regex, so nothing here was gate-protected and nothing would have caught the contradiction. --- docs/BACKLOG.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index 0ec8bf72..e2b8f101 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. From 348c098ba7e119cd6485f021114db511801adbf9 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Sun, 2 Aug 2026 20:00:10 -0500 Subject: [PATCH 08/15] docs(asvs): a corpus that cannot express a class of claim cannot refute one The most reusable finding from today's sweep, and it is not a verdict. A false statement about ASVS -- "ASVS reserves non-applicable for functionality-based exclusions" -- survived TWO independent assessors and reached a signed-adjacent risk-acceptance block. Not because anyone was careless: because every one of them verified against the pinned corpus, and the corpus is REQUIREMENTS-ONLY. It carries no chapter prose, no assessment guidance, no definitions. A claim about what the standard SAYS outside a requirement's own text is structurally uncheckable against it, and the check comes back clean every time. 0x03 says the opposite of what was asserted: 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. New section 2.1a states the rule and the instruction that follows from it: to cite ASVS prose, fetch the chapter at the v5.0.0 tag and quote it verbatim -- never paraphrase from memory, from an earlier assessment, or from another agent, and never read a green corpus check as evidence about a claim the corpus cannot represent. Sits between 2.1 (an out-of-scope cell buys no L3 claim) and 2.2 (a count movement is not a posture movement) because all three are the same family: things a green check does not tell you. Named as the finding most worth keeping by the ASVS-11.7.1-summary session, which is also the session that independently hit it. --- docs/ASVS-ASSESSMENT-METHOD.md | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/docs/ASVS-ASSESSMENT-METHOD.md b/docs/ASVS-ASSESSMENT-METHOD.md index 5f3641cc..e5b3bbe4 100644 --- a/docs/ASVS-ASSESSMENT-METHOD.md +++ b/docs/ASVS-ASSESSMENT-METHOD.md @@ -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 From 8552edc3f9a62a4dd1eb7f8e0ebf1dab0730e918 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Sun, 2 Aug 2026 20:09:42 -0500 Subject: [PATCH 09/15] feat(console): the ASVS 3.7.3 external-navigation predicate, and file a consult item for 5.4.3's premise Groundwork, not a finished feature: the route, page and settings are not in this commit. Filing a new ledger item here, not building one -- hence no claim. 1. THE PREDICATE (messagefoundry_webconsole/_external.py). Pure -- no settings import, no request, no I/O. What decides whether an interstitial appears is the part that has to be right, so it is isolated and tested on its own. The test is a DECLARED DOMAIN LIST, not same-origin, because ASVS says "outside the application's CONTROL" and control is organisational: an operator's own AD FS is a different host, a different origin, and squarely inside their control. Two details the research named as the ways a naive version is WORSE than none: * A suffix test without a label boundary is a hole -- `evilhospital.example` ends with `hospital.example`. Matched on a dot boundary instead. * The displayed host must be what the browser RESOLVES. A Cyrillic homograph renders identically to the Latin form, so showing decoded Unicode shows the operator a lie. We display punycode and flag when the two differ. Secure-by-default in three places: an empty domain list makes every absolute http(s) URL external; an unparseable host is external; a non-navigable scheme is external. Failing toward SHOWING the interstitial is the safe direction. 14 tests. The two that matter are the lookalike and the homograph; two reach controls exist so a predicate that fires on everything cannot pass as working. 2. A NEW LEDGER ITEM (number from alloc.ps1, never grepped) recording a consult we owe ourselves: 5.4.3's `na` rests on antivirus being enterprise-provided, and this tests that premise where it is least likely to hold -- files the engine PULLS over SFTP/FTPS from outside the domain, where there is no inbound delivery event for a perimeter scanner and on-access coverage depends on whether the integration host's working directory is watched at all. USED THE REAL CUSTOMER DOMAIN IN THE FIRST DRAFT AND CAUGHT IT BEFORE COMMITTING. Fixture domains are now RFC 2606 `.example`. Worth recording: the `forbidden-content` leak guard PASSED on that draft, so it does not cover customer DOMAINS -- only the manual diff scan caught it. That is a gap in the gate, not a near-miss to feel relieved about. --- docs/BACKLOG.md | 28 ++++++ messagefoundry_webconsole/_external.py | 115 +++++++++++++++++++++++ tests/test_external_link_interstitial.py | 115 +++++++++++++++++++++++ 3 files changed, 258 insertions(+) create mode 100644 messagefoundry_webconsole/_external.py create mode 100644 tests/test_external_link_interstitial.py diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index e2b8f101..fdbc0630 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -8503,6 +8503,34 @@ The three surfaces now share **one** liveness helper, because they had been disa --- +## 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/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/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 From 099fdcd767589d47a78eb0c9ef4fb96f1ab4c322 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Sun, 2 Aug 2026 20:52:08 -0500 Subject: [PATCH 10/15] =?UTF-8?q?feat(console):=20ASVS=203.7.3=20=E2=80=94?= =?UTF-8?q?=20interpose=20"you=20are=20leaving=20this=20site",=20with=20a?= =?UTF-8?q?=20cancel,=20before=20an=20off-site=20hop?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Completes the control the predicate in 8552edc3 only decided. Default-on; the escape is opt-in, named at startup, and documented as lowering security. THE SHAPE, and why the GET/POST split is the interesting part. `GET /ui/oidc/start` used to mint a PKCE flow and 303 to the IdP in one step. It now renders the interstitial and stages NOTHING; the flow starts only when the operator confirms via POST. Three things fall out: * The destination is never carried by the page -- only the HOST is shown, and the POST posts back to us with no URL. An interstitial that carried its target would itself be an open redirect, which is strictly worse than having none. * The bounded flow cache REJECTS when full, so a GET that minted a flow was a login-DoS lever. It no longer is. * The confirm is a POST behind the console's Sec-Fetch-Site check, closing the standing hole where any external page could begin a federated sign-in by linking to the start leg. INTERNAL IS INTERNAL. ASVS asks about destinations outside the application's CONTROL, and control is organisational, not topological -- an operator's own AD FS is a different host, a different origin, and squarely theirs. So `organization_domains` decides, matched on a LABEL boundary. Empty is the STRICT position: every absolute destination is interstitialed, including your own IdP. SEAM v17. The policy reaches the route as CONFIG on UiDeps, for the same reason `oidc_enabled` does: `create_managed_app` attaches the AuthService inside the lifespan, long after `mount_ui` has fixed the route table. Console SUPPORTED_ENGINE_SEAMS and the golden snapshot move with it; the golden diff was reviewed before regenerating and is exactly the four fields plus the bump. THREE GATES CAUGHT REAL THINGS AND ALL THREE WERE FIXED PROPERLY, NOT SUPPRESSED: * The rate-limit doc gate reads throttle shapes by walking DECORATED route functions. My first draft hoisted the start body into a plain helper -- limiter still working, gate blind to it. Restructured so the decorated POST holds the branch, with a comment saying why it must stay there. * The 8.1.3/8.1.4 contextual-input inventory flagged `external_link_allowlist`. It decides whether a NOTIFICATION is shown, not whether a request is authorized, so it is recorded in the reviewed-non-inputs set WITH that reasoning -- it is still a security-relevant setting, and those are different questions. * `test_serve_loopback_emits_no_new_stderr` caught a start-time note that fired on every stock boot. Removed: start-time output is for a posture WEAKER than the default, never for the default itself. Startup warnings name allowlist entries INDIVIDUALLY, never a count -- "3 destinations exempted" is the shape of message that lets an entry nobody intended sit for a year. 9 route tests on top of the 14 predicate tests. The load-bearing one asserts the page carries no destination URL to post back; the negative-branch tests (internal IdP, interstitial off, allowlisted) genuinely do NOT render it, so the positives are not passing vacuously. Settings documented in CONFIGURATION.md, including that declaring your domains is the correct fix for an over-eager interstitial and the allowlist is the waiver. --- docs/CONFIGURATION.md | 3 + docs/SECURITY.md | 10 +- messagefoundry/__main__.py | 27 ++++ messagefoundry/api/_ui_seam.py | 23 +++- messagefoundry/api/app.py | 31 +++++ messagefoundry/config/settings.py | 54 ++++++++ messagefoundry_webconsole/__init__.py | 2 +- messagefoundry_webconsole/pages/account.py | 62 +++++++++ messagefoundry_webconsole/routes/oidc.py | 62 +++++++++ tests/golden/webconsole_seam.snapshot | 6 +- tests/test_security_doc_drift.py | 10 ++ tests/test_ui_oidc_interstitial_route.py | 146 +++++++++++++++++++++ 12 files changed, 428 insertions(+), 8 deletions(-) create mode 100644 tests/test_ui_oidc_interstitial_route.py diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index 1dedb38b..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` | 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/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..da155c6a 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: 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/pages/account.py b/messagefoundry_webconsole/pages/account.py index 5a663e5c..40ef51a8 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,67 @@ 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", + ) + return page("Leaving this site", body, nav=Markup("")) + + 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..c8ffca7c 100644 --- a/messagefoundry_webconsole/routes/oidc.py +++ b/messagefoundry_webconsole/routes/oidc.py @@ -43,6 +43,7 @@ set_oidc_flow_cookie, set_session_cookie, ) +from .._external import is_allowlisted, is_external, is_idn_disguised _log = logging.getLogger(__name__) @@ -77,8 +78,69 @@ 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. + """ 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/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_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..6315342a --- /dev/null +++ b/tests/test_ui_oidc_interstitial_route.py @@ -0,0 +1,146 @@ +# 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 + + +@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 From ab5ab8602bbf1682fdcb035b4afbeee063168c98 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Sun, 2 Aug 2026 21:01:23 -0500 Subject: [PATCH 11/15] fix(security): report the 3.7.3 interstitial switches as posture loosenings Caught by `test_every_security_bool_at_its_insecure_value_is_reported`, which iterates SecuritySettings.model_fields and fails on any security bool that can sit at its insecure value without security_loosenings() naming it. `external_link_interstitial` was exactly that: an operator could turn the ASVS 3.7.3 control OFF and GET /security/posture would not say so. That completeness floor is the right shape and it worked -- a new switch cannot be added without either being reported or being explicitly exempted with a reason. Also registers `external_link_allowlist`, which the floor does NOT pin because it is a LIST rather than a bool. Left unregistered it would have been an unreported loosening by omission, and the floor would never have said a word. Entries are named individually, not counted: "3 destinations are exempt" tells a posture review nothing about WHICH, and that is how an entry nobody intended survives one. Two suite failures remain and neither is mine: test_version and test_gate_installed_parity compare the venv's INSTALLED metadata (0.3.0) against the source __version__ (0.3.2). A stale editable install in a venv shared by several sessions -- not repaired here, because reinstalling into it would change the environment under them. --- messagefoundry/config/settings.py | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/messagefoundry/config/settings.py b/messagefoundry/config/settings.py index da155c6a..d32764a0 100644 --- a/messagefoundry/config/settings.py +++ b/messagefoundry/config/settings.py @@ -4129,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( ( From 0a32cee10bc6e73b6cbd78179edcf7af17d2c2d9 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Sun, 2 Aug 2026 21:13:00 -0500 Subject: [PATCH 12/15] docs(asvs): the method's 5.4.3 worked example still scored it `fail` after the record said `na` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Same coupling that bit 11.7.1 and the same fix. The rubric teaches by worked example, so a stale example does not merely sit there — it TEACHES the superseded verdict to the next assessor, in the one document written to stop assessors disagreeing. Rewritten as the rule-1 case it now is, and the old reasoning is KEPT rather than deleted: "a scan hook exists but its only shipped implementation is _no_scan and there is no configuration key" is still TRUE of the code. It answers rule 3's question, and rule 1 runs first. An example that just swaps its verdict teaches nothing about why. Also records, in the rubric itself, that this `na` is WEAKER than 11.7.1's: the engine ships a scan seam, so this is a control the product COULD implement, which makes the verdict conditional on the enterprise actually covering those ingest paths. A reader who takes this row as a template for the next platform-delegated cell needs that qualifier, not just the verdict. Found while resolving the merge with main, not by looking for it. --- docs/ASVS-ASSESSMENT-METHOD.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/ASVS-ASSESSMENT-METHOD.md b/docs/ASVS-ASSESSMENT-METHOD.md index e5b3bbe4..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. | From 27499a14370227b20e441b36c7bb368681595d8e Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Sun, 2 Aug 2026 22:02:08 -0500 Subject: [PATCH 13/15] fix(console): the 3.7.3 split MOVED the drive-by sign-in hole instead of closing it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects in my own 3.7.3 build, and one false claim in its commit message. Found by the coordinator reading the actual CI rather than my local subset. 1. ⛔ ASVS 3.5.1 — THE NEW POST ROUTE HAD NO ORIGIN GUARD. `099fdcd7`'s message said "the confirm is a POST behind the console's Sec-Fetch-Site check". THAT WAS FALSE. The leg checks Sec-Fetch-MODE (is this a navigation), not Sec-Fetch-SITE (did it come from us) — and a cross-site
IS a navigation, so it sailed through. The split therefore RELOCATED the drive-by sign-in hole from GET to POST rather than closing it: a foreign page could still mint a flow and bounce the operator to the IdP. `assert_same_origin` is now the first statement of the POST handler. It precedes the rate limiter deliberately and does not violate the module's "audit-writing branches sit behind the limiter" rule — it raises 403 and writes no audit row. The module docstring's "no same-origin assertion on either leg" carve-out was written for the CALLBACK, where the IdP's redirect is legitimately cross-site. It never applied to a start leg reached from our own page, and now says so. 2. ASVS 7.4.4 — `leaving_site()` suppressed the shared chrome with `nav=Markup("")`, stranding a signed-in operator on a page with no sign-out. Its sibling interstitial `oidc_landing()` uses `minimal_nav()` for exactly this reason; now so does this one. 3. THE TESTS COULD NOT HAVE CAUGHT EITHER. All nine sent NO Sec-Fetch headers, so the guard would never have fired — they would have passed just as happily with it deleted. Three added that supply the header a real browser sends, including a REACH CONTROL so a guard that 403s everything cannot pass. ⭐ AND WRITING THOSE TESTS FALSIFIED A SECOND CLAIM OF MINE. A cross-site GET with an EXTERNAL IdP renders the interstitial (200) and is NOT refused — correctly, because that page stages nothing. But with an INTERNAL IdP the GET delegates straight to the minting leg, so FOR INTERNAL-IDP DEPLOYMENTS THE GET STILL MINTS A FLOW, exactly as before this change. The bounded-flow-cache DoS lever is closed for external IdPs and merely origin-guarded for internal ones. `099fdcd7` claimed the split closed it outright; it did not. Both behaviours are now asserted so the asymmetry cannot be forgotten. Note for the record: the coordinator's cited test paths (test_ui_origin_guard.py, test_ui_logout_affordance.py) exist in neither this branch nor main, and neither assertion message appears anywhere in tests/. The FINDINGS were right and I verified each against the code directly; the file references were not. --- messagefoundry_webconsole/pages/account.py | 6 +- messagefoundry_webconsole/routes/oidc.py | 43 +++++++++--- tests/test_ui_oidc_interstitial_route.py | 78 ++++++++++++++++++++++ 3 files changed, 118 insertions(+), 9 deletions(-) diff --git a/messagefoundry_webconsole/pages/account.py b/messagefoundry_webconsole/pages/account.py index 40ef51a8..a9ab9378 100644 --- a/messagefoundry_webconsole/pages/account.py +++ b/messagefoundry_webconsole/pages/account.py @@ -216,7 +216,11 @@ def leaving_site( el("p", el("a", "Cancel", href=cancel_href), class_="muted"), class_="card", ) - return page("Leaving this site", body, nav=Markup("")) + # 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: diff --git a/messagefoundry_webconsole/routes/oidc.py b/messagefoundry_webconsole/routes/oidc.py index c8ffca7c..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,6 +49,7 @@ from .. import pages from .._auth import ( + assert_same_origin, clear_oidc_flow_cookie, oidc_flow_cookie_name, set_oidc_flow_cookie, @@ -141,6 +153,21 @@ async def ui_oidc_start(request: Request) -> Response: 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/tests/test_ui_oidc_interstitial_route.py b/tests/test_ui_oidc_interstitial_route.py index 6315342a..7ac987a7 100644 --- a/tests/test_ui_oidc_interstitial_route.py +++ b/tests/test_ui_oidc_interstitial_route.py @@ -135,6 +135,84 @@ def test_the_interstitial_page_carries_no_destination_url_to_post_back() -> None 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.""" From 71bb2de22998f750d888323c648d33bf15f1792f Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Sun, 2 Aug 2026 22:04:18 -0500 Subject: [PATCH 14/15] =?UTF-8?q?docs(backlog):=20file=20the=20register-vs?= =?UTF-8?q?-record=20gate=20=E2=80=94=20nothing=20has=20ever=20compared=20?= =?UTF-8?q?the=20two?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The through-line of this whole session, filed so it outlives it: the ASVS risk-acceptance register is UNGATED PROSE. No check has ever compared the cell ids in its signed sign-off blocks against the verdict of record, and a manual cross-check of all eight blocks found 29 entries that are not carried residuals -- 22 of them `unverified`, i.e. a signed acceptance of a risk nobody ever assessed, present in every single block. ⚠️ Filed with the count explicitly marked NOT INDEPENDENTLY CONFIRMED. It is one measurement by one session, and the gate is precisely what would reproduce it, so the item says to treat the number as the REASON to build the check and never as an established fact. Caveat raised by the coordinator and it is the right one -- a finding about unverified claims should not itself be an unverified claim. Design notes carry the three lessons this session paid for: print what you scanned (a gate whose regex stopped matching is indistinguishable from a clean one); prove it red in both directions before trusting green; and REPORT, never auto-correct -- the lists sit inside SIGNED acceptances and rewriting signed content to satisfy a checker is a worse defect than the drift. ⛔ Filed, NOT built. A new security-doc gate can block merges, so it is the owner's green-light like any enforcing control. Number from alloc.ps1. Explicitly out of scope for the item, and stated in it: what the 2026-07-14 signature actually covered. That is not a fact an agent can recover -- it is a question about what a human agreed to, and only the signer can answer it. --- docs/BACKLOG.md | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index 6b440379..f6d7f8b1 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -8509,6 +8509,37 @@ 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. From 9f85b461d5ec85eb9ba553889c612f3cd0b7dc80 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Sun, 2 Aug 2026 22:12:06 -0500 Subject: [PATCH 15/15] =?UTF-8?q?test(webconsole):=20the=20OIDC=20tests=20?= =?UTF-8?q?still=20asserted=20the=20pre-interstitial=20flow=20=E2=80=94=20?= =?UTF-8?q?and=20a=20correction=20I=20owe?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RETRACTION FIRST, because it is committed and pushed. `27499a14`'s message says the coordinator's cited test paths "exist in neither this branch nor main, and neither assertion message appears anywhere in tests/". THAT IS FALSE. Both files exist: packaging/messagefoundry-webconsole/tests/test_ui_origin_guard.py packaging/messagefoundry-webconsole/tests/test_ui_logout_affordance.py Both assertion messages are in them. Its citation was the full path straight from the CI log and was correct in substance AND provenance. I grepped `tests/` -- the engine dir -- and concluded from its absence there. I made that error inside the very message correcting the coordinator for a provenance error, which is the sharpest possible demonstration of the rule I was invoking: VERIFY BEATS INFER, including when you are the one doing the correcting. THE GAP THAT CAUSED IT, and it invalidates several "full suite green" claims I made tonight: pyproject's `testpaths = ["tests"]` means a bare `pytest` NEVER RUNS the webconsole package's own suite -- 14 files, 344 tests. My 10,301-passed runs were missing all of them. So the local/CI gap is not "those tests are CI-only"; it is that my invocation silently excluded a whole package. Running them proved the 27499a14 fixes rather than merely arguing for them: 21 pass on the two files that were failing. THE THIRD REGRESSION WAS ALSO REAL, and I had denied it for the same wrong reason. `test_oidc_start_redirects_to_the_idp_and_sets_the_flow_cookie` DOES exist and did assert 303 on the GET. Three tests updated to the intended design rather than to whatever made them pass: * the redirect/PKCE/cookie test now asserts the GET renders the interstitial and sets NO cookie, then POSTs for the 303 -- the split is asserted, not stepped around. * the AC-8 availability test targets the POST, because "still reaches the IdP" is a property of the leg that attempts. * the full round trip TRAVERSES the interstitial (GET 200 -> POST 303) rather than shortcutting to the POST. Its value is walking the path a browser walks. 344 passed, 3 skipped across the whole webconsole package. --- .../tests/test_webui.py | 23 ++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) 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")