From de7065a03dcb473e615c8eb32cf8b14fb529ad3c Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Wed, 12 Aug 2026 11:29:52 -0500 Subject: [PATCH 1/2] backlog: amend #1112 -- the concurrent-uploads half is answered; the cross-shard half is what remains NO NEW NUMBER. The ASVS lane ran the duplicate check BEFORE drafting and it fired: #1112's own title already names both halves ("concurrent uploads AND across engine shards"), so the store-side fix is its second half, not a new item. Filing it fresh would have burned a number on a duplicate -- the failure section 7 records twice, and which I made myself today on the glyph item. VERIFIED AGAINST origin/main, not taken on report: uploads.py:309 _quota_lock = asyncio.Lock() (PR #325) uploads.py:488 async with self._quota_lock: ... :496 _scan_metas_sync() "the check-then-write is not atomic" 0 occurrences -- and there is no line 394 settings.py:439 states shards over one dir enforce ONE budget, measured with a live positive control TWO STRIKES, both false in the ALARMING direction, kept rather than deleted: - the banner's "no lock constructed in the module" and its :394 citation; - the severity paragraph's "N shards would each apply the budget, multiplying it". The real remainder is an at-most N-1 file overshoot, each bounded by max_upload_bytes. An overstated severity is not conservative -- it misdirects whoever costs the fix. WHAT KEEPS IT OPEN is one sentence: asyncio.Lock is per-process, and the module says so itself at :487. Engine sharding is the shipped default scaling axis, nothing partitions uploads_dir per shard, so N shards hold N independent locks. The fix has a known home -- ADR 0063 already puts every shard on one unified store -- so "cannot honestly reach pass" is NOT the finding. Acceptance is stated as EXECUTION with concurrent writers on at least 2 shards, never by reading. OWNER RULING recorded in the item: ASVS 2.3.4 holds at partial. CAUGHT MY OWN BLIND CHECK doing this: my first grep for the struck string ran against messagefoundry/api/uploads.py, which does not exist -- the file is messagefoundry/uploads.py. It printed 0 and I labelled it "the string is gone". Re-ran against the real path with a negative control (49 def/class lines) before believing it. --- docs/BACKLOG.md | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index e5d0ccb4..895439d2 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -5991,10 +5991,18 @@ filing. ## 1112. research an honest pass for ASVS 2.3.4 -- a quota that holds across concurrent uploads and across engine shards at once -> 🔢 **Filed 2026-08-08 - not started. RESEARCH item: the goal is an HONEST pass, and "cannot honestly reach pass" is a valid finding.** Value **6/10** · Difficulty **5/10**. ASVS **2.3.4** (L2) currently scores **partial**. The pinned verb asks for business-logic locking so a limited-quantity resource cannot be double-booked by manipulating application logic. What holds it short is the per-uploader quota in `messagefoundry/uploads.py`, which says of itself at line 394 that "the check-then-write is not atomic", with the check at `:397` and no lock constructed in the module. +> 🔢 **Filed 2026-08-08 - not started. RESEARCH item: the goal is an HONEST pass, and "cannot honestly reach pass" is a valid finding.** Value **6/10** · Difficulty **5/10**. ASVS **2.3.4** (L2) currently scores **partial**. The pinned verb asks for business-logic locking so a limited-quantity resource cannot be double-booked by manipulating application logic. What holds it short is the per-uploader quota in `messagefoundry/uploads.py`, ~~which says of itself at line 394 that "the check-then-write is not atomic", with the check at `:397` and no lock constructed in the module.~~ +> +> ⚠️ **AMENDED 2026-08-12 -- HALF OF THIS ITEM IS ANSWERED BY SHIPPED CODE, AND THE STRUCK SENTENCE ABOVE IS FALSE. The item STAYS OPEN on its second half.** PR #325 added `_quota_lock` (`uploads.py:309`) and takes it at `:488` across **both** the scan (`:496`) and the build-and-write -- releasing between them was the race. The quoted string *"the check-then-write is not atomic"* **no longer occurs in the file at all**, and there is no line 394 to cite. **The concurrent-uploads half is DONE:** within one engine process the double-book is closed. +> +> **WHAT KEEPS IT OPEN is the OTHER half named in this item's own title, and it is exactly one sentence:** `asyncio.Lock` is per-event-loop, therefore **per-process** -- the module says so at `:487`, *"One critical section per process"*. **Engine sharding is the built, shipped, default scaling axis** and nothing partitions `uploads_dir` per shard, so N shards over one directory hold **N independent locks**. +> +> **THE FIX HAS A KNOWN HOME, so "cannot honestly reach pass" is NOT the finding here.** [ADR 0063](docs/adr/0063-no-split-store-unified-store-for-sharding.md) already puts every engine shard on **one unified store**, so the cross-shard coordination point exists: move the quota accounting into the store, or take an advisory lock on the directory. **Suggested acceptance -- demonstrated by EXECUTION, not by reading:** the quota holds across N shards sharing one `uploads_dir`, with concurrent writers on **at least 2 shards**; and the single-process path keeps its current behaviour (N=1, overshoot zero). +> +> **OWNER RULING 2026-08-12: ASVS 2.3.4 HOLDS AT `partial`.** Grounds: V2.3.4 names business-logic locking as the means, and **a per-process lock does not span a resource shared across processes**; sharding is ordinary configuration here, not an exotic deployment. **Cluster:** Security / ASVS remediation research. **Priority:** P2. **Verdict:** research. -**Severity:** on a first deployment, concurrent in-flight uploads would overshoot the per-uploader file-count and byte budget, and N engine shards would each apply the budget separately, multiplying it. Every other contended resource named in the residual is locked by a shipped default. +**Severity:** on a first deployment, concurrent in-flight uploads would overshoot the per-uploader file-count and byte budget, and ~~N engine shards would each apply the budget separately, multiplying it~~ **[FALSE, and false in the ALARMING direction -- measured by `85fe7e85`, struck not deleted.** `_scan_metas_sync` is an **uncached filesystem read**, so shards sharing one `uploads_dir` enforce **ONE budget between them**. `config/settings.py:439` says so in the shipped code, with a live positive control: *"engine shards sharing one dir see each other's files and the budget does NOT multiply (measured 2026-08-10 -- two UploadStores over one dir, the second refused the same uploader at quota)"*. **The real remainder is far smaller than this sentence claimed:** an at-most **N-1 file overshoot**, one per competing shard caught between its scan and its file landing on disk, each bounded by `max_upload_bytes`.]**. Every other contended resource named in the residual is locked by a shipped default. **The pinned verb.** "Verify that business logic level locking mechanisms are used to ensure that limited quantity resources (such as theater seats or delivery slots) cannot be double-booked by manipulating the application's logic." From 6d30b58a8a5da50e1ab6cc1a5709f08abac8fede Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Wed, 12 Aug 2026 13:55:55 -0500 Subject: [PATCH 2/2] backlog: fix the ADR link prefix I got wrong in #1112's amendment test_link_resolution caught it: docs/BACKLOG.md:6000 wrote [ADR 0063](docs/adr/...) which resolves RELATIVE TO docs/, so it became docs/docs/adr/... and did not exist. The convention is unambiguous and I was the outlier: 74 links in this file use the adr/ prefix, exactly one used docs/adr/, and it was mine. Verified after fixing: zero docs/adr/ links remain, and the target file exists. I could NOT run the guard locally -- pytest dies on ModuleNotFoundError: pydantic, because a worktree venv installs fewer extras than CI. That is BACKLOG #1230 demonstrating itself on the PR that amends the ledger, an hour after its scope was ruled. CI is the authority here, which is the whole point of #1230's loud-omission half: a local run that cannot collect must not read as a clean one. --- docs/BACKLOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index 99bbc854..0e068a53 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -5997,7 +5997,7 @@ filing. > > **WHAT KEEPS IT OPEN is the OTHER half named in this item's own title, and it is exactly one sentence:** `asyncio.Lock` is per-event-loop, therefore **per-process** -- the module says so at `:487`, *"One critical section per process"*. **Engine sharding is the built, shipped, default scaling axis** and nothing partitions `uploads_dir` per shard, so N shards over one directory hold **N independent locks**. > -> **THE FIX HAS A KNOWN HOME, so "cannot honestly reach pass" is NOT the finding here.** [ADR 0063](docs/adr/0063-no-split-store-unified-store-for-sharding.md) already puts every engine shard on **one unified store**, so the cross-shard coordination point exists: move the quota accounting into the store, or take an advisory lock on the directory. **Suggested acceptance -- demonstrated by EXECUTION, not by reading:** the quota holds across N shards sharing one `uploads_dir`, with concurrent writers on **at least 2 shards**; and the single-process path keeps its current behaviour (N=1, overshoot zero). +> **THE FIX HAS A KNOWN HOME, so "cannot honestly reach pass" is NOT the finding here.** [ADR 0063](adr/0063-no-split-store-unified-store-for-sharding.md) already puts every engine shard on **one unified store**, so the cross-shard coordination point exists: move the quota accounting into the store, or take an advisory lock on the directory. **Suggested acceptance -- demonstrated by EXECUTION, not by reading:** the quota holds across N shards sharing one `uploads_dir`, with concurrent writers on **at least 2 shards**; and the single-process path keeps its current behaviour (N=1, overshoot zero). > > **OWNER RULING 2026-08-12: ASVS 2.3.4 HOLDS AT `partial`.** Grounds: V2.3.4 names business-logic locking as the means, and **a per-process lock does not span a resource shared across processes**; sharding is ordinary configuration here, not an exotic deployment.