Conversation
…d bound Four review-gate items from PR block#4401 round 3 (Wren's binding hold at 0845a09, amended by Sami's alias and read-bound findings): 1. New buzz-media regression test: a real tower_http TimeoutError, wrapped the way axum wraps body errors, driven through process_video_upload's StreamReader read loop must surface as MediaError::RequestBodyTimeout / 408 — never Io / 500. Honest scope per Sami's mutation pass (M3): this pins the conversion chain, not the sniff/routing decision, which lives upstream in upload_blob and is stated as uncovered rather than implied-covered. Mutating either conversion arm (the read-loop TimedOut arm or the stream map's IdleTimeout arm) fails the test. 2. MEDIA_READ_TIMEOUT restored 300s: commit 4 silently tightened media reads 300s -> 60s while relaxing uploads (found by Sami). 300s preserves the bound reads already had and covers the multi-call pre-header path: the read handler awaits several sequential storage calls, each independently allowed up to 60s by rust-s3's per-call default, so a 60s request deadline could cancel a sequence whose individual calls are all within their own budgets. 3. Route-precedence regression test for the legacy /media/upload alias: the literal lives in the upload sub-router while /media/{sha256_ext} lives in the read sub-router, merged. If axum ever resolved the literal under the param capture, the alias would inherit the tight read wall-clock and the commit-3 regression would survive on exactly one route. A slow-but-progressing PUT /media/upload must complete under discriminating bounds (read deadline tighter than the upload duration), and GET /media/upload must answer 405 from the literal, not 408 from the param route's deadline. 4. Comment wording: "any duration"-shaped claims on the idle bound now qualify completion by the 3600s MEDIA_UPLOAD_CEILING instead of contradicting it. No production code changes beyond the MEDIA_READ_TIMEOUT constant and doc comments. Additive on 0845a09, no rebase. Co-authored-by: npub1qyvc0c5kl4gqv2fd97fsk46tu378sqgy35vc83rvgfwne90sel7s0ed67d <011987e296fd5006292d2f930b574be47c7801048d1983c46c425d3c95f0cffd@buzz.block.builderlab.xyz> Signed-off-by: npub1qyvc0c5kl4gqv2fd97fsk46tu378sqgy35vc83rvgfwne90sel7s0ed67d <011987e296fd5006292d2f930b574be47c7801048d1983c46c425d3c95f0cffd@buzz.block.builderlab.xyz>
…d bound Four review-gate items from PR block#4401 round 3 (Wren's binding hold at 0845a09, amended by Sami's alias and read-bound findings): 1. New buzz-media regression test: a real tower_http TimeoutError, wrapped the way axum wraps body errors, driven through process_video_upload's StreamReader read loop must surface as MediaError::RequestBodyTimeout / 408 — never Io / 500. Honest scope per Sami's mutation pass (M3): this pins the conversion chain, not the sniff/routing decision, which lives upstream in upload_blob and is stated as uncovered rather than implied-covered. Mutating either conversion arm (the read-loop TimedOut arm or the stream map's IdleTimeout arm) fails the test. 2. MEDIA_READ_TIMEOUT restored 300s: commit 4 silently tightened media reads 300s -> 60s while relaxing uploads (found by Sami). 300s preserves the bound reads already had and covers the multi-call pre-header path: the read handler awaits several sequential storage calls, each independently allowed up to 60s by rust-s3's per-call default, so a 60s request deadline could cancel a sequence whose individual calls are all within their own budgets. 3. Route-precedence regression test for the legacy /media/upload alias: the literal lives in the upload sub-router while /media/{sha256_ext} lives in the read sub-router, merged. If axum ever resolved the literal under the param capture, the alias would inherit the tight read wall-clock and the commit-3 regression would survive on exactly one route. A slow-but-progressing PUT /media/upload must complete under discriminating bounds (read deadline tighter than the upload duration), and GET /media/upload must answer 405 from the literal, not 408 from the param route's deadline. 4. Comment wording: "any duration"-shaped claims on the idle bound now qualify completion by the 3600s MEDIA_UPLOAD_CEILING instead of contradicting it. No production code changes beyond the MEDIA_READ_TIMEOUT constant and doc comments. Additive on 0845a09, no rebase. Co-authored-by: npub1qyvc0c5kl4gqv2fd97fsk46tu378sqgy35vc83rvgfwne90sel7s0ed67d <011987e296fd5006292d2f930b574be47c7801048d1983c46c425d3c95f0cffd@buzz.block.builderlab.xyz> Signed-off-by: npub1qyvc0c5kl4gqv2fd97fsk46tu378sqgy35vc83rvgfwne90sel7s0ed67d <011987e296fd5006292d2f930b574be47c7801048d1983c46c425d3c95f0cffd@buzz.block.builderlab.xyz> Signed-off-by: Tal Weiss <major.tal@gmail.com>
230ac85 to
83f1d18
Compare
…d bound Four review-gate items from PR block#4401 round 3 (Wren's binding hold at 0845a09, amended by Sami's alias and read-bound findings): 1. New buzz-media regression test: a real tower_http TimeoutError, wrapped the way axum wraps body errors, driven through process_video_upload's StreamReader read loop must surface as MediaError::RequestBodyTimeout / 408 — never Io / 500. Honest scope per Sami's mutation pass (M3): this pins the conversion chain, not the sniff/routing decision, which lives upstream in upload_blob and is stated as uncovered rather than implied-covered. Mutating either conversion arm (the read-loop TimedOut arm or the stream map's IdleTimeout arm) fails the test. 2. MEDIA_READ_TIMEOUT restored 300s: commit 4 silently tightened media reads 300s -> 60s while relaxing uploads (found by Sami). 300s preserves the bound reads already had and covers the multi-call pre-header path: the read handler awaits several sequential storage calls, each independently allowed up to 60s by rust-s3's per-call default, so a 60s request deadline could cancel a sequence whose individual calls are all within their own budgets. 3. Route-precedence regression test for the legacy /media/upload alias: the literal lives in the upload sub-router while /media/{sha256_ext} lives in the read sub-router, merged. If axum ever resolved the literal under the param capture, the alias would inherit the tight read wall-clock and the commit-3 regression would survive on exactly one route. A slow-but-progressing PUT /media/upload must complete under discriminating bounds (read deadline tighter than the upload duration), and GET /media/upload must answer 405 from the literal, not 408 from the param route's deadline. 4. Comment wording: "any duration"-shaped claims on the idle bound now qualify completion by the 3600s MEDIA_UPLOAD_CEILING instead of contradicting it. No production code changes beyond the MEDIA_READ_TIMEOUT constant and doc comments. Additive on 0845a09, no rebase. Co-authored-by: npub1qyvc0c5kl4gqv2fd97fsk46tu378sqgy35vc83rvgfwne90sel7s0ed67d <011987e296fd5006292d2f930b574be47c7801048d1983c46c425d3c95f0cffd@buzz.block.builderlab.xyz> Signed-off-by: npub1qyvc0c5kl4gqv2fd97fsk46tu378sqgy35vc83rvgfwne90sel7s0ed67d <011987e296fd5006292d2f930b574be47c7801048d1983c46c425d3c95f0cffd@buzz.block.builderlab.xyz> Signed-off-by: Tal Weiss <major.tal@gmail.com>
83f1d18 to
dc2ead6
Compare
dc2ead6 to
1b96b09
Compare
…d bound Four review-gate items from PR block#4401 round 3 (Wren's binding hold at 0845a09, amended by Sami's alias and read-bound findings): 1. New buzz-media regression test: a real tower_http TimeoutError, wrapped the way axum wraps body errors, driven through process_video_upload's StreamReader read loop must surface as MediaError::RequestBodyTimeout / 408 — never Io / 500. Honest scope per Sami's mutation pass (M3): this pins the conversion chain, not the sniff/routing decision, which lives upstream in upload_blob and is stated as uncovered rather than implied-covered. Mutating either conversion arm (the read-loop TimedOut arm or the stream map's IdleTimeout arm) fails the test. 2. MEDIA_READ_TIMEOUT restored 300s: commit 4 silently tightened media reads 300s -> 60s while relaxing uploads (found by Sami). 300s preserves the bound reads already had and covers the multi-call pre-header path: the read handler awaits several sequential storage calls, each independently allowed up to 60s by rust-s3's per-call default, so a 60s request deadline could cancel a sequence whose individual calls are all within their own budgets. 3. Route-precedence regression test for the legacy /media/upload alias: the literal lives in the upload sub-router while /media/{sha256_ext} lives in the read sub-router, merged. If axum ever resolved the literal under the param capture, the alias would inherit the tight read wall-clock and the commit-3 regression would survive on exactly one route. A slow-but-progressing PUT /media/upload must complete under discriminating bounds (read deadline tighter than the upload duration), and GET /media/upload must answer 405 from the literal, not 408 from the param route's deadline. 4. Comment wording: "any duration"-shaped claims on the idle bound now qualify completion by the 3600s MEDIA_UPLOAD_CEILING instead of contradicting it. No production code changes beyond the MEDIA_READ_TIMEOUT constant and doc comments. Additive on 0845a09, no rebase. Co-authored-by: npub1qyvc0c5kl4gqv2fd97fsk46tu378sqgy35vc83rvgfwne90sel7s0ed67d <011987e296fd5006292d2f930b574be47c7801048d1983c46c425d3c95f0cffd@buzz.block.builderlab.xyz> Signed-off-by: npub1qyvc0c5kl4gqv2fd97fsk46tu378sqgy35vc83rvgfwne90sel7s0ed67d <011987e296fd5006292d2f930b574be47c7801048d1983c46c425d3c95f0cffd@buzz.block.builderlab.xyz> Signed-off-by: Tal Weiss <major.tal@gmail.com>
🔐 Codex Security Review
|
|
@block/buzz-oss-team - poke on this one, gently. It is green and merges clean against main, so no workflow approval is needed just to look at it. Reason I am pinging instead of waiting: one of the three issues in here is #4424, where the relay bounds request bytes but never request time. Valid headers, then withhold the body, and the task parks forever. Unauthenticated. I sat 200 sockets on it to confirm. I also just rewrote the description. It was stale, still describing only the original large-push bug from back when that was all this was, which made the diff size look alarming for no reason. It now explains why three issues ended up in one PR (short version: same middleware, and each fix makes the next one necessary). If you would rather review the DoS part on its own, say so and I will split #4424 out. Either way works, I just do not want it sitting. |
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested. Reviewed 4ab4f786085a23fe6126529861840eff6048ceee...36fab5314173da1d6c34f170ee6eccce41a22dcb.
P1: The new API wall deadline can strand committed events without audit or delivery
The inline finding describes a concrete postcommit cancellation path, not just an ambiguous timeout response. With audit backpressure, the new deadline cancels event processing after DB insertion but before audit enqueue and before fanout/workflow scheduling. Reposting the same event with fresh HTTP authorization returns duplicate: and does not repair the lost work. Ownership transfer, create-only community provisioning, and invite claim also await publication after committing; their retries are not a universal recovery mechanism.
I reproduced the event cancellation with a paused-clock standalone harness using the verbatim production timeout, audit-enqueue, and dispatch-scheduling functions. DB insertion/duplicate semantics and downstream delivery were stubbed, so this is a control-flow reproduction, not live relay/Postgres integration:
BASE: original=200 OK retry=200 OK stored=true audit=Some("00") dispatched=true
HEAD: original=408 Request Timeout retry=200 OK stored=true audit=None dispatched=false
Scope and smallest safe correction
The exact four-byte probe shim on both Git services is a narrow compatibility fix; I found no auth bypass or repository-data disclosure in that changed path. Its local PROBE_BODY_TIMEOUT already bounds the newly introduced unauthenticated read. The API/media-wide timeout policy is not required to make that probe safe. I recommend taking the offered split: keep the push/fetch compatibility shim and local body deadline here, and review #4424’s wider policy separately. That separate fix should bound request-body consumption without cancelling irreversible mutations, or provide tested durable/idempotent recovery. No new user-facing feature was found in the diff, but new global timeout semantics are meaningful scope expansion.
Media disposition
I did not establish a second blocking media defect. The 3600s whole-upload ceiling can still interrupt storage/publication after a valid body finishes near the deadline; Blossom auth is checked before those operations, not continuously through them. A missing sidecar leaves an unservable orphan and content-addressed retry can repair it. S3 cancellation/late-commit behavior was not independently reproduced, so I am recording this as a residual availability/orphan risk, not claiming corruption or an auth bypass. The idle timeout/error-classification changes themselves preserve the reviewed 408/413/500 distinction.
Validation and limits
- Inspected all ten changed files and traced Git auth/authorization, body-limit layering, API postcommit paths, media body/error/storage lifecycle, and retry behavior. Independent Git/router/media review lanes were consolidated rather than treated as a blanket safety certificate.
- The unchanged probe implementation and all four new probe unit tests passed in an isolated package with key dependencies pinned to the production versions. The cancellation harness reproduced the regression. Neither result is a full
buzz-relaybuild/test run. - I did not run the ignored live-relay/MinIO Git E2E or independently reproduce the author’s live push/fetch measurements. The current GitHub check snapshot does not show Rust build/test execution, and the security-review comment marks this exact range as requiring a fresh review. Green metadata checks are not evidence those validations ran.
Exit criteria: remove the outer API mutation deadline (the minimal split), or demonstrate cancellation-safe durable postcommit completion/retry through the real event and membership mutation paths. Retain the local Git probe deadline and obtain exact-head relay test plus live large-request workflow evidence before merge.
| .layer(TimeoutLayer::with_status_code( | ||
| StatusCode::REQUEST_TIMEOUT, | ||
| timeout, | ||
| )) |
There was a problem hiding this comment.
[P1] Do not time out the entire API mutation future
This helper is applied to /events and other writes, but their postcommit work is not cancellation-safe. ingest_event inserts the event (handlers/ingest.rs:3172-3204), then dispatch_persistent_event awaits the intentionally bounded audit queue before spawning fanout/workflows (handlers/event.rs:358-374,574-597). If the queue is full when this 60s wall deadline expires, TimeoutLayer drops that future: the DB event remains, but its audit entry and delivery/workflow task are lost. The same event retried with a fresh NIP-98 header exits as duplicate: at ingest.rs:3206-3211, before either can be repaired. This can occur under sustained audit pressure, or when a valid slow body consumes most of the total budget and only a short postcommit wait remains.
A paused-clock seam harness using the verbatim timeout and audit/dispatch functions reproduces base=200/audit+dispatch versus head=408/stored/no-audit/no-dispatch; DB storage is stubbed, not live integration. The new collector tests prove cancellation, not mutation recovery.
Bound body reception rather than the mutation service future, or split this unrelated API policy out while retaining the local Git probe timeout. Simply returning 408 after commit is not a safe failure mode here.
Signed-off-by: Tal Weiss <major.tal@gmail.com>
36fab53 to
45a34b0
Compare
MajorTal
changed the title
Git sends the same unauthenticated four-byte flush-packet probe before any smart HTTP POST whose body exceeds http.postBuffer — not just large pushes. Fetch negotiations cross the 1 MiB decoded threshold when the client's divergent local history contributes enough have lines (an in-limit repo plus ~21k unpushed divergent branches suffices; measured live at MAX_MANIFEST_REFS = 10_000 with divergent client history: 1.46 MiB decoded negotiation, probe fired, fetch got 401). Parameterize the receive-pack probe middleware over the service's (request MIME, result MIME) pair — the predicate was already otherwise service-agnostic — and mount it on the upload-pack POST route as well. All other predicate legs are unchanged: no Authorization / Content-Encoding / Transfer-Encoding headers, a single exact Content-Length of 4, and an exact 0000 body; near misses fall through to the normal authenticated path. Tests: the existing exactness/path-independence matrix now runs against both services (2 path shapes + 11 near-misses each), plus a cross-service case proving each route rejects the other service's request MIME. Fixes block#4423 Co-authored-by: npub1qyvc0c5kl4gqv2fd97fsk46tu378sqgy35vc83rvgfwne90sel7s0ed67d <011987e296fd5006292d2f930b574be47c7801048d1983c46c425d3c95f0cffd@buzz.block.builderlab.xyz> Signed-off-by: npub1qyvc0c5kl4gqv2fd97fsk46tu378sqgy35vc83rvgfwne90sel7s0ed67d <011987e296fd5006292d2f930b574be47c7801048d1983c46c425d3c95f0cffd@buzz.block.builderlab.xyz> Signed-off-by: Tal Weiss <major.tal@gmail.com>
Admitting the 0000 probe means reading four unauthenticated body bytes. Give that read its own 60 s deadline that fails closed into the normal authenticated path, so a client that sends the probe headers and then withholds the body cannot park a task. The wider request-time policy for the API and media routers is a separate change (block#4424). Co-authored-by: npub1qyvc0c5kl4gqv2fd97fsk46tu378sqgy35vc83rvgfwne90sel7s0ed67d <011987e296fd5006292d2f930b574be47c7801048d1983c46c425d3c95f0cffd@buzz.block.builderlab.xyz> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Tal Weiss <major.tal@gmail.com>
The unit lane selects buzz-relay tests by explicit nextest expression, and none of its terms matched api::git::transport::track_c_tests, so the four compatibility-probe tests were compiled by clippy but executed by no hosted job. They need no database or storage, so they belong in the infra-free lane. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Tal Weiss <major.tal@gmail.com>
45a34b0 to
8e51810
Compare
2 participants
Split per review: this PR is only the Git smart HTTP probe. The request-time policy for the API and media routers lives in #7753, which also fixes the post-commit cancellation Carl found.
What this fixes
#2880. When a push is bigger than
http.postBuffer, Git first sends a tiny unauthenticated POST with only the 4-byte flush packet0000. We answered401, Git gave up, large pushes were impossible. The fix admits exactly that probe: noAuthorization, noContent-Encoding, noTransfer-Encoding, exactly oneContent-Length: 4, body exactly0000. Anything else falls back to the normal auth path.#4423. The same probe fires before big fetches. Same middleware, parameterized over the (request MIME, result MIME) pair and mounted on upload-pack too.
Probe body deadline. Admitting the probe means reading four unauthenticated body bytes. That read has its own 10 s deadline that fails closed into normal auth, so a client that sends the probe headers and withholds the body cannot park a task. Git and curl send the headers and the
0000in the same segment, so a real probe never waits on it. This is the only timeout in this PR.Commits
Four, rebased on current main, all signed off:
fix(git): allow large smart HTTP pushesfix(git): admit the compatibility probe on upload-pack toofix(git): bound the compatibility probe's body readci(unit): run the git probe regression tests in the unit laneChanges since the last push: the probe deadline went from 60 s to 10 s, the fallthrough branch now documents the invariant it relies on (both handlers extract
GitAuthfrom the request parts before touching the body, so a candidate withoutAuthorizationis answered 401 without decoding), the four probe tests are wired intojust test-unit(no term in its nextest expression matched them, so hosted CI compiled them and ran none), and two commits that carried an npub as git author are now authored normally.Tests
Content-Length: 4thenTransfer-Encoding: chunked, clones the bytes back byte for byte.Review done before this push
We ran Block's own gates and reviews locally on this exact head, macOS Apple Silicon:
just fmt-check,just clippy(workspace,-D warnings),just test-unit(cargo-nextest),cargo-deny check,just file-size-check,just security-review-check: clean.scripts/postgres-test-run.shagainst Postgres and Redis containers): 390 passed.Content-Length, over- and under-delivered bodies, chunked, mixed-case headers,Expect: 100-continue) and confirmed the predicate and the fail-closed path. Its non-blocking items are the changes listed above.Hosted Rust CI has not run on this head; it needs a maintainer to approve the workflow for this fork branch. The Codex security review needs a fresh authorization for the new range.
Fixes #2880
Fixes #4423
🤖 Generated with Claude Code