asvs: approval compensation, upload quota atomicity, TOTP SHA-256 cutover, MLLP inbound pacing - #325
Merged
Merged
Conversation
…2.3.3) ApprovalGate.approve moves the row to 'approved' BEFORE running the executor. That ordering is load-bearing -- it guards the double-approve race -- and it stays. The gap was what happened when the executor then raised: the row was left asserting an operation that never ran, and because record_audit came AFTER the execute, no approval.approved row was written either. The store therefore carried a released approval with no recorded outcome at all, which is worse than the item describes. The tree already knew about this hazard and had defended exactly one of the three registered operations. _purge's comment states it verbatim -- "a raise would strand the row approved-but-unexecuted" -- and works around it locally by returning a skip result instead of raising. _replay and _config_reload had no such protection, and _config_reload deliberately surfaces ConfigReloadDenied as a raise. So the mitigation existed, in a comment, for one executor out of three. Fixed at the gate rather than per-executor: on a raise the row is rolled 'approved' -> 'failed', an approval.failed row is audited against both identities, and the ORIGINAL error is re-raised so the caller still sees it. Retry-instead-of-compensate was considered and rejected on evidence. The three operations are not uniformly replay-safe: cancel_queued is naturally idempotent, replay_dead would re-requeue, and _config_reload bumps the cluster config version with propagate=True, so a re-drive is not a no-op. Compensation is the only shape that is correct for all three. decide_pending_approval grows a from_status parameter (default 'pending', so every existing caller is unchanged) across the protocol and all three backends. Guarding the compensation on 'approved' means it can never clobber a row another caller rejected or expired, and a re-drive moves zero rows. No CHECK constraint exists on status in any backend, so 'failed' needs no migration. except Exception is deliberate and is not a swallow -- any executor failure must compensate, and the original is re-raised. BaseException is NOT caught, so a cancelled approve is not recorded as a failure. If the compensating transition itself fails it is logged loudly and the original error still wins, because a store that is unreachable here must not mask the error that actually explains the failure. The audit detail records the exception TYPE, never its message: executor text can carry connection names, paths or params, and the audit log is not a PHI sink. Asserted by test. _purge's comment is updated in the same commit. Its premise -- that a raise strands the row -- is false as of this change, and leaving it would be the stale-premise defect the ASVS record exists to catch. The skip behaviour stays, because a non-quiesced outbound is a retryable precondition miss rather than a failed operation. WATCHED FAIL: with the source reverted and the tests present, all three new tests fail, and the first fails on exactly the right assertion -- "assert 'approved' == 'failed'" -- so the defect is demonstrated, not assumed. 12 passed in tests/test_approvals.py with the fix; 113 passed across test_approvals + test_store + test_api. mypy strict is the instrument that checks the three-backend signature change, because the SQL Server and Postgres store legs silently SKIP on this box and would only fail in CI. Clean on all six edited files. The 21 mypy errors in the tree are all import-not-found for optional extras (dicom, fhir, webauthn) in four files this change does not touch.
…ct it and pin it
Two shipped artifacts asserted that engine shards sharing one uploads_dir each
get a fresh per-uploader quota: config/settings.py called it "a documented
residual, same shape as the summary-rate cap", and uploads.py called the quota
"per-process per-uploads_dir". The ASVS 2.3.4 residual inherits the same claim
("N engine shards multiply the budget"), and the backlog item built its
worked example on it -- warning that a fix closing only the concurrency limb
would leave "the shard-multiplication half untouched".
There is no shard-multiplication half. Measured 2026-08-10 by execution, not
by reading: two UploadStore instances over ONE directory, quota 3. Shard A
refused file 4 (live positive control -- the cap engages). Shard B, at the
same dir, ALSO refused. The budget does not multiply.
The mechanism is that _scan_metas_sync is an uncached filesystem read: it
re-walks the root and decrypts every sidecar on every call, so any process at
that dir sees every other process's files. Shards pointed at SEPARATE dirs do
get separate budgets, but that is per-directory scoping by construction, not
the shared-dir case both comments described.
What survives is the check-then-write race, and it is smaller than the
multiplication claim: each concurrently in-flight upload can overshoot by at
most one file, itself bounded by max_upload_bytes. Shards compound the race
(more concurrency) but not the budget. Both comments now say that, with the
measurement, instead of the old claim.
Pinned by test, so the correction cannot silently regress back into a
per-process budget -- which WOULD be the double-booking ASVS 2.3.4 forbids.
The test shares one cipher across both stores ON PURPOSE, and says why: real
shards run off one unified store and therefore one keyring/DEK. An earlier
draft let the fixture mint a key per store, which made shard B skip shard A's
sidecars as undecryptable and manufactured a per-process budget that does not
exist. That failure was an artifact of the fixture and would have "confirmed"
the very claim this commit refutes.
This corrects the record only. The check-then-write race itself is the next
layer and is not touched here.
…l section (ASVS 2.3.4) The quota scanned the uploader's sidecars and then wrote, with the lock released in between, so two concurrent uploads each read a stale count and both proceeded. That is the double-booking of a limited-quantity resource ASVS 2.3.4 is about, reachable by ordinary concurrency rather than by any special access. Fixed by serialising the whole build-and-write behind an asyncio.Lock, not just the check: releasing between the check and the write IS the race. The throughput cost is acceptable and nowhere near the data plane -- this is the operator diagnostic-upload surface and each pass is already bounded by max_bytes. WATCHED FAIL, made deterministic rather than timing-dependent: the sidecar scan is slowed so both coroutines are guaranteed to overlap. With the lock removed and the test present, a quota of 1 admits TWO files. With the lock, exactly one wins and the other is refused on quota, with only the winner on disk. The residual is now stated precisely instead of vaguely. The critical section is per-process, so N shards sharing one dir can still overshoot by at most N-1 files -- one per shard mid-write while another scans. On the shipped single-process deployment N is 1 and the overshoot is zero. Closing the multi-shard remainder needs a cross-process mechanism (an advisory lock on the dir, or moving the accounting into the unified store); it is NOT closed here and the docstring says so, because a comment implying otherwise would be the compensating-control-on-a-false-premise defect this chapter keeps finding. Deliberately not done: optimistic write-then-verify-then-rollback would also close the cross-process half, but it writes PHI to disk before deciding it is over quota. The existing design refuses BEFORE anything is written and that property is worth more than the remaining N-1.
… refutes docs/SECURITY.md called ECH for outbound SNI "infeasible" and said a working client "would require a third-party TLS stack -- violating the no-new-dependency rule for a security-core path". Both halves are false, and the counter-evidence is in this repository. tools/ech-sidecar is a working out-of-process ECH client whose go.mod reads "stdlib-only, no dependencies -- builds offline with GOPROXY=off" and whose every import in main.go is Go stdlib (context, crypto/tls, encoding/*, net, net/http, ...). ADR 0139 records it. That is the compensating-control-on-a-false-premise shape SDS-3.7 forbids, and it is the worst place for it: a reader weighing whether to ACCEPT this residual was being told the control could not be built, while the tree contained one. The paragraph now says the narrower thing that is actually true: the ENGINE PROCESS cannot do ECH, because Python 3.14's stdlib ssl exposes no ECH API and there is no SVCB/HTTPS resolver in scope. The sidecar exists but is not wired into any egress path, not built by CI, and not distributed (pyproject.toml excludes tools/ from sdist and wheel). THE ACCEPTED RESIDUAL IS UNCHANGED -- the destination SNI is visible on the outbound handshake either way. Only the premise is corrected, and the correction is recorded in place rather than silently overwritten. This is true under EITHER branch of the unruled G19 (keep or retire the sidecar), so it does not wait on that ruling. Not touched: ADR 0093 section 3 carries the same framing, but an ADR is a dated decision record and ADR 0139 already post-dates it. Rewriting it would be rewriting history rather than the record; flagged for the owner instead. 285 passed across the nine tests that read docs/SECURITY.md from disk.
…that named it G19 ruled RETIRE. Nothing built, tested, linted or version-pinned the tree. Measured before removing, not asserted: four tracked files (.gitignore, README.md, go.mod, main.go), main.go 312 lines, zero module dependencies, and NO Go toolchain anywhere in CI -- zero matches for setup-go / go-version / go build across .github/, against a live positive control of 14 files matching setup-python. Keeping it would have bought a fourth Dependabot ecosystem, the first COMPILED-language CodeQL leg (both current languages are interpreted, so a shape change rather than a matrix row), a toolchain pin with no hash-lock analogue beside three existing lockfiles, and a fourteenth required context against a set of thirteen -- for a control no measurement has shown reachable: the 2026-07-20 DoH type-65 probe found no healthcare counterparty publishing an ECHConfig, against a working Cloudflare control. ZERO CELLS MOVE. ASVS 12.1.5 was an accepted fail before and after. Custody verified BEFORE deletion rather than trusted from a commit message: all four files are present in the private vault at tools/ech-sidecar/ and that vault is pushed, so the code survives in two places, not just git history. Nothing operational goes with it. The engine-side fail-closed routing (ech_sidecar_url_from_settings, egress_route_from_settings) and tests/test_ech_egress.py stay exactly as they were -- they refuse a non-loopback sidecar, refuse ech_egress without ech_sidecar, and error rather than silently falling back to a SNI-leaking direct hop. An operator supplies their own terminator; samples/ech-sidecar/README.md is unchanged. Four records named the deleted path and are reconciled here: - rest.py's docstring pointed at tools/ech-sidecar/ AND asserted it was "proven to hide the SNI against a real ECH endpoint". Both had to go: the path now resolves to nothing, and #1011 already flagged that the "proven" claim needed whatever evidence actually backs it. - test_ech_egress.py's docstring said the real Go sidecar is "proven separately (tools/ech-sidecar)". Replaced, and the file now states plainly what it does NOT prove: no ECH is originated anywhere in this suite, so no SNI-hiding claim may rest on it. - ADR 0139's status block filed the re-originator under "Deferred" while the tree shipped -- understating what existed. Amended in place with the ruling and the measurement. The DECISION is untouched; Increment 1 still stands. - docs/SECURITY.md carried my own 2026-08-10 correction, which cited the tree as live counter-evidence. That correction now needed its own correction: amended to past tense, and I dropped its claim that pyproject.toml "excludes tools/ from both sdist and wheel" -- verified, sdist is an ALLOW-LIST (only-include) and I found no separate wheel target, so the original wording was both stronger and less accurate than the truth. NOT fixed here, handed over: docs/testing/master-test-plan/16-security-phi- and-supply-chain.md still says the re-originator "now ships" at :217, :253, :323 and :888. Those rows are dense P2 planning text in a document this change does not own, and SEC-71's pass criterion is precisely "a dated owner decision covering all three: keep or retire" -- which this ruling supplies, so the rows want reconciling by whoever owns that plan rather than rewriting in passing. 94 passed on test_ech_egress + test_communications_inventory; 586 passed across the adr/inventory/docs selection. ruff and mypy clean on every touched file (the 21 mypy errors are pre-existing import-not-found for optional extras in four files this does not touch).
…(G19 part b) A bare `fail` cannot distinguish two very different states: one nobody has got to, and one no amount of correct code can move. Prose in `residual` can say which, but prose carries no date -- so the second kind rots invisibly, because the blocking condition is external and can lift without anyone noticing. That is not hypothetical. The G19 retirement decision landed today on a DoH probe finding no counterparty publishing an ECHConfig, and at ruling time that measurement was three weeks old with NO recorded cadence for re-running it. The single fact that would have reversed the decision was going stale unwatched. This makes that computable instead. Seven fields, all mandatory: blocked_by, reason, evidence, unblock_signal, unblock_probe, checked_on, recheck_days. The unblock half is the whole point -- a signal nobody can probe, or a probe with no date, cannot go stale visibly. A PARTIAL record is refused at load, because a reason with no probe reads as diligence and carries none: the compensating-control-on-a-false- premise shape arriving through the fix. Modelled on the decision_closed precedent -- on the Cell rather than loose TOML, so the renderer can surface it. A blocker nobody can see is one a pass walks straight past. DELIBERATELY NOT a seventh verdict value: that would change the denominator and every renderer, and every existing count in every document would silently mean something else. DELIBERATELY not `na` either -- ASVS 5.0 dropped 4.0's clause letting a documented exclusion preserve a compliance claim, so `na` buys nothing and misdescribes what was assessed. The requirement applies, it was assessed, and it failed. HONEST LIMIT, stated in the docstring so it is not overclaimed: overdue blockers are REPORTED, not enforced. Making a stale probe red the gate would block unrelated pull requests on a calendar date -- attention bought at a cost nobody agreed to. --status prints the count where humans and CI already look, including when it is ZERO, so "no blockers" and "the section was dropped from the renderer" cannot look alike. Promoting it to a hard failure is a one-line change and a deliberate decision. Scoped to verdict `fail` on purpose. A blocked PARTIAL is a real thing (the V10 relying-party cells are the obvious candidates), but admitting one is a ruling, not a default -- so the loader refuses it and says so. WATCHED FAIL, 6/6 negative controls before the tests were written: a missing field, a blocker on a non-fail verdict, a non-ISO checked_on, and recheck_days = 0 are each REFUSED; the real file loads; and a well-formed but 400-day-stale record LOADS and reports "RE-PROBE OVERDUE: 12.1.5 by 310d". Now pinned by 12 tests, including one parametrised over all seven fields. 138 passed in tests/test_asvs_scorecard.py.
BREAKING for any enrolled authenticator. Ruled under G18 on 2026-08-11, on the argument that this is the one moment the cutover is free: there is no per-user TOTP algorithm column, so it is a cutover rather than a migration, and at zero deployments there are zero enrolled users to re-enrol. That argument expires on first deployment and never returns. THE DANGEROUS PROPERTY, and why this is not a one-line change. The engine computes with its digest and the authenticator computes with whatever otpauth_uri advertised. If those two ever disagree, NOTHING RAISES -- codes simply never match, for every user, with no diagnostic. So the advertised name is DERIVED from the digest (_TOTP_ALGORITHM = _TOTP_DIGEST().name.upper()) rather than written beside it, which makes editing one without the other impossible instead of merely discouraged. hashlib's .name is exactly the otpauth spelling, and the derivation is correct for all three RFC 6238 permitted digests. A test pins that it stays derived. KNOWN COST, documented at the call site rather than discovered later: Google Authenticator historically IGNORES the otpauth `algorithm` parameter and computes SHA-1 regardless, so its codes will never match this engine. Most modern authenticators honour it (1Password, Bitwarden, Aegis, FreeOTP, Authy). That is a support burden, not a security one, and the fix for an operator hitting it is an app that honours the parameter. The RFC 6238 conformance test moves to the SHA-256 rows of the same Appendix B table rather than being deleted -- and is now asserted at the RFC's own EIGHT digits instead of truncated to six, which tests strictly more of the truncation math than before. The vectors were derived independently (a hand-rolled HMAC-SHA256 HOTP, no engine code) and match the published table on all six rows. Note the seeds differ per digest: SHA-1 uses the 20-byte ASCII seed, SHA-256 the 32-byte one, and pairing the SHA-1 seed with the SHA-256 rows silently produces non-matching codes -- called out in the test so the next reader does not have to rediscover it. _SECRET_BYTES stays at 20 DELIBERATELY, and its comment no longer cites a retired algorithm as its rationale. RFC 6238 R6 says the key SHOULD match the HMAC output length (32 bytes for SHA-256), but that clause is about interop convention rather than strength; 160 bits is ample against HMAC-SHA256, and 32 bytes would lengthen manual entry from 32 to 52 base32 characters on a screen an operator types from. Swept the tree for other TOTP algorithm claims: none in docs/, packaging/ or the web console. The only remaining SHA-1 mention was the secret-length comment, corrected here. 543 passed across the totp/mfa/auth selection; ruff and mypy clean.
…VS 2.4.1 / 15.2.2) The engine had no bound on messages per second from an accepted peer in ANY configuration -- verified by grep against a live positive control (zero matches in transports/mllp.py, ten in transports/http_listener.py on the identical pattern), with docs/SECURITY.md stating the absence itself. A sender able to reach the NIC-bound data plane could submit unbounded messages, each durably persisted before its ACK. IT HAD TO BE A PACER, NOT A LIMITER, and that is the whole design. The count-and-log invariant forbids accept-and-drop, so discarding was never available. NAKing would mean refusing clinical messages the engine can process. Closing the connection moves the loss outside our boundary where we cannot count it. A store-side quota bounds retention, not intake. Pacing the READ is the only option that satisfies the invariant BY CONSTRUCTION: the excess is never framed, so it never becomes a received message the invariant would then oblige us to account for, and TCP applies the back-pressure itself. The wait sits BEFORE the read and never around the handler. Delaying anything after decode would pace a message already counted -- the same control with none of the property. A bounded delay rather than a hard stop, deliberately: refusing to read at all holds the connection open and keeps consuming a max_connections slot, so a flood of paced peers could exhaust the slot budget and BECOME the denial of service this exists to prevent. The delay is exactly the bucket's deficit, so it is bounded by messages/rate. Scoped per connection, not per peer IP. MLLP peers are unauthenticated and identified only by address, so a per-IP budget collapses under NAT or a shared integration host and would throttle unrelated feeds sharing an egress address. A peer opening more connections is bounded by max_connections instead. SHIPS OFF, and that is a ruled exception to this module's "key absent -> secure default" convention rather than an oversight -- stated at the constant. A rate limit on a clinical interface is only safe at a number derived from a real feed profile, and this project has none; a guessed default would throttle real traffic, which is worse than the unbounded intake it guards. So ASVS 2.4.1 stays `partial` on the shipped default and the record will say why. That is the honest outcome of the ruling, not a shortfall in this change. THE LOAD-BEARING TEST IS NOT THAT PACING HAPPENS -- it is test_pacing_never_drops_a_message: 12 messages at 20/s with burst 2 all arrive, all ACKed, in order. Note it still PASSES with the pacer neutered, and that is correct: it asserts a SAFETY property that must hold either way, while test_pacing_actually_delays_the_reads asserts the liveness half. Watched fail: neutering charge() to return 0.0 reds three tests including the end-to-end one. The timing assertion is a LOWER bound only. An upper bound would pin scheduler timing and make this the flaky test someone deletes. 638 passed across the mllp/transport selection; 198 across the docs selection after documenting both settings in docs/CONNECTIONS.md. ruff and mypy clean.
…ession-b-fec292 # Conflicts: # docs/SECURITY.md # messagefoundry/transports/rest.py # tests/test_ech_egress.py
…own build falsified it The MLLP pacing control (e0e979d) made three things false at once, and all three had to move together or the record contradicts the code: 1. docs/SECURITY.md's ingest-plane row asserted "no message-rate or volume limit exists ... and nothing in transports/, config/ or pipeline/ exposes a messages-per-second control". transports/mllp.py now exposes exactly that. 2. tests/test_security_doc_rate_limits.py PINNED that sentence, so the guard was enforcing a stale fact. It is updated rather than deleted -- the row must now state BOTH that a control exists and that it ships OFF, because either half alone misleads: "exists" implies the shipped default is bounded, and "none" is simply false. 3. The ASVS 2.4.1 cell's absence claim asserted MLLPSource carries no rate limiting. Re-scored vault-side in the same act. The Scope column reads in-process rather than a new token, and that is the accurate word: one bucket per connection, coordinating across neither engine shards nor peers. The row keeps naming the honest gaps -- an off default bounds nothing, and the raw-TCP inbound never got this control. This is the shape the ASVS programme keeps finding in other people's work, so it is worth being plain that it was mine this time: I shipped a control and left three artifacts asserting it did not exist. 238 passed across the four suites that parse this document.
wshallwshall
enabled auto-merge (squash)
August 11, 2026 16:57
wshallwshall
disabled auto-merge
August 11, 2026 16:57
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The ASVS block's engine work: five owner rulings executed, each with a watched fail rather than an
assumed one.
Item numbers are deliberately omitted from this description. This branch touches
messagefoundry/andedits neither ledger file, so naming them here would arm the backlog-hygiene context against a file
this branch is forbidden to change. Banner text has gone to the ledger writer separately.
Contains a BREAKING change
TOTP moves from SHA-1 to SHA-256. This is a cutover, not a migration -- there is no per-user
algorithm column -- and it is free only because nothing is deployed.
Known cost, documented at the call site: Google Authenticator historically ignores the
otpauthalgorithmparameter and computes SHA-1 regardless, so existing codes from it will not match. Mostother authenticators honour the parameter. This belongs in the changelog.
A pre-existing failure on
main, NOT introduced heretests/test_dast_claims.pyfails onmainas it stands. Verified independently from two directions:the current
mainpush-to-main CI run is a failure, and running that module againstmain's ledgercontent alone reproduces it. The trigger is one prose clause in
docs/BACKLOG.md, a file this branchdoes not touch.
It reached
mainthrough the docs-only blind mode: doc-drift guards are gated oncode == 'true', sothey are skipped on a docs-only PR and fire on the push to
mainafterwards. A fix is with the ledgerwriter.
Expect that failure here regardless of merge order, and do not attribute it to this change.
One part of this branch is intentionally a no-op
The ECH sidecar retirement was built here without knowing
mainhad already done it -- the base was 48commits stale.
main's version was better on all three conflicting files, somainwas taken for eachand the duplicate ADR amendment was deleted; the clean auto-merge had otherwise left both dispositions
present, one asserting a false retirement date. This PR does not retire the sidecar -- that already
landed.
Verification
Full suite on the merged tree: 11,750 passed, 861 skipped, 1 failed -- the single failure being the
pre-existing one above.
ruff format,ruff checkandmypy --strictclean on every file touched.Caveat carried rather than buried: those 861 skips include the SQL Server and Postgres store legs,
and this branch changes an approval function's signature across all three backends.
mypy --strictiswhat actually checked that locally; the DB legs first execute in CI.