From 534bda6ba56f4cfb0824fa18556b2233f2b3abe3 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Mon, 27 Jul 2026 20:21:44 -0500 Subject: [PATCH 1/2] fix(node): bound the request body the signature middleware buffers before it verifies `require_signature` buffered the whole request body with an unlimited `body.collect()` before parsing a single header, and nothing upstream capped it. On the git write routes the 2 GB `RequestBodyLimitLayer` is applied to the inner router while `add_auth_layers` wraps it, and a later `.layer()` is the outer service, so the middleware ran first and saw the raw body. The other signed groups carry no limit layer at all and lean on axum's 2 MB `DefaultBodyLimit`, which the extractors enforce through a request extension this middleware never consults, so that default was bypassed too. `optional_signature` forwards to the same code whenever a caller merely presents signature headers, so the reach extended to `/graphql`, `/ipfs/{cid}` and the other optional-signature groups. Measured against the real router: an unsigned push with `GITLAWB_MAX_PACK_BYTES` set to 64 KiB drained the full 4 MiB body and only then returned 401. No DID, no signature, and no repo were required, and in front of it sits only a per-IP arrivals-per-hour counter, no timeout layer and no connection cap. The collect now runs through `http_body_util::Limited`, so reading stops at the limit instead of after the fact, and an over-limit request gets 413 rather than the 400 reserved for a genuinely unreadable body. The limit is per route group: `add_auth_layers` takes it and passes it to the middleware as state, the git groups keep `max_pack_bytes` so large pushes are unaffected, and the rest get a constant equal to axum's default because that is already what their extractors enforce. Verification, content-digest, clock-skew and the request reconstruction are untouched. The regression test asserts bytes actually polled out of a streaming body, not the status code, because a status assertion passes whether or not the body was drained. Neutralizing the limit turns it red; handing every group the git limit turns the non-git test red; returning 400 instead of 413 turns the status test red. This bounds per-request buffering only. Total resident memory is still O(connections x limit); a read timeout and a connection cap are what close that and are deliberately not in this change. --- crates/gitlawb-node/src/auth/mod.rs | 69 +++++- crates/gitlawb-node/src/server.rs | 67 ++++- crates/gitlawb-node/src/test_support.rs | 311 +++++++++++++++++++++++- 3 files changed, 422 insertions(+), 25 deletions(-) diff --git a/crates/gitlawb-node/src/auth/mod.rs b/crates/gitlawb-node/src/auth/mod.rs index 720fb3ae..6658ffd0 100644 --- a/crates/gitlawb-node/src/auth/mod.rs +++ b/crates/gitlawb-node/src/auth/mod.rs @@ -34,6 +34,17 @@ use gitlawb_core::http_sig::{ }; use gitlawb_core::identity::verify; +/// Per-route-group ceiling on how much request body [`require_signature`] will +/// buffer BEFORE it has authenticated anything. +/// +/// This is middleware state rather than a constant because a single number cannot +/// serve both a 2 GB git push and a 2 MB profile update: the git write group +/// passes `GITLAWB_MAX_PACK_BYTES`, every other signed group passes +/// [`crate::server::SIGNED_BODY_LIMIT`]. Making it a required piece of state means +/// a new signed route group cannot be wired up without choosing a limit. +#[derive(Clone, Copy, Debug)] +pub struct SignatureBodyLimit(pub usize); + /// Axum middleware that enforces HTTP Signature authentication (RFC 9421). /// /// Every write request must carry: @@ -48,20 +59,52 @@ use gitlawb_core::identity::verify; /// 4. Resolves the did:key to an Ed25519 VerifyingKey /// 5. Rebuilds the signing string and verifies the Ed25519 signature /// 6. Verifies Content-Digest matches the request body -pub async fn require_signature(request: Request, next: Next) -> Response { - // Buffer the body so we can verify content-digest and pass it downstream +pub async fn require_signature( + State(SignatureBodyLimit(limit)): State, + request: Request, + next: Next, +) -> Response { + verify_signature(limit, request, next).await +} + +/// The signature check itself, shared by [`require_signature`] and the optional +/// variant. `limit` bounds the pre-auth buffer; see [`SignatureBodyLimit`]. +async fn verify_signature(limit: usize, request: Request, next: Next) -> Response { + // Buffer the body so we can verify content-digest and pass it downstream. + // + // The buffer is bounded, because this runs BEFORE any identity check: an + // unauthenticated caller with no DID, no signature and no repo reaches this + // line. Nothing upstream supplies the bound. On the git write routes + // `RequestBodyLimitLayer` sits on the inner router, and a later `.layer()` is + // the OUTER service, so this middleware runs first and sees the raw body. On + // every other signed group axum's 2 MB `DefaultBodyLimit` is enforced by the + // `Bytes`/`Json` extractors through a request extension, which collecting the + // body here bypasses. `Limited` stops reading at the limit rather than reading + // everything and then measuring it, which is the property that matters. let (parts, body) = request.into_parts(); - let body_bytes = - match body.collect().await { - Ok(collected) => collected.to_bytes(), - Err(_) => return ( + let body_bytes = match http_body_util::Limited::new(body, limit).collect().await { + Ok(collected) => collected.to_bytes(), + // Over the limit: a distinct status from the 401 an unsigned request gets + // and the 400 below, so a client can tell "too big" from "not authenticated" + // and from "your body did not arrive". + Err(e) if e.is::() => return ( + StatusCode::PAYLOAD_TOO_LARGE, + Json(json!({ + "error": "body_too_large", + "message": format!("request body exceeds the {limit}-byte limit for this route"), + })), + ) + .into_response(), + Err(_) => { + return ( StatusCode::BAD_REQUEST, Json( json!({ "error": "unreadable_body", "message": "could not read request body" }), ), ) - .into_response(), - }; + .into_response() + } + }; let sig_input = parts .headers @@ -248,11 +291,17 @@ pub async fn require_signature(request: Request, next: Next) -> Response { /// Optional variant for rolling upgrades: verify and inject `AuthenticatedDid` when /// RFC 9421 signature headers are present, but allow legacy unsigned requests to /// continue when no signature attempt was made. -pub async fn optional_signature(request: Request, next: Next) -> Response { +pub async fn optional_signature( + State(SignatureBodyLimit(limit)): State, + request: Request, + next: Next, +) -> Response { let has_signature_headers = request.headers().contains_key("signature-input") || request.headers().contains_key("signature"); if has_signature_headers { - return require_signature(request, next).await; + // Same pre-auth buffer, same bound: presenting signature headers is not + // authentication, so this path is as reachable as the required one. + return verify_signature(limit, request, next).await; } next.run(request).await } diff --git a/crates/gitlawb-node/src/server.rs b/crates/gitlawb-node/src/server.rs index f4c0d3e3..6629ab1c 100644 --- a/crates/gitlawb-node/src/server.rs +++ b/crates/gitlawb-node/src/server.rs @@ -42,17 +42,37 @@ async fn graphql_playground() -> impl IntoResponse { )) } +/// Pre-auth body-buffer ceiling for every signed route group except the git write +/// routes. Deliberately equal to axum's `DefaultBodyLimit` (2 MB), which is what +/// these groups' `Bytes`/`Json` extractors already enforce: that default works +/// through a request extension the extractors consult, and `require_signature` +/// collects the raw body itself, so it never saw the cap. Matching the number +/// keeps this change behavior-preserving on the response path (nothing that used +/// to be accepted starts failing) and purely a memory fix. +pub const SIGNED_BODY_LIMIT: usize = 2 * 1024 * 1024; + /// Applies the standard auth middleware pair to a router: HTTP Signature verification /// followed by UCAN chain validation. The two layers run in this order for every /// matched request: `require_signature` first (sets `AuthenticatedDid`), then /// `require_ucan_chain` (reads it). -fn add_auth_layers(router: Router, state: AppState) -> Router { +/// +/// `body_limit` bounds the request body `require_signature` buffers before it has +/// authenticated anything (see [`auth::SignatureBodyLimit`]): `max_pack_bytes` for +/// the git write group, [`SIGNED_BODY_LIMIT`] for every other group. +fn add_auth_layers( + router: Router, + state: AppState, + body_limit: usize, +) -> Router { router .layer(middleware::from_fn_with_state( state, auth::require_ucan_chain, )) - .layer(middleware::from_fn(auth::require_signature)) + .layer(middleware::from_fn_with_state( + auth::SignatureBodyLimit(body_limit), + auth::require_signature, + )) } pub fn build_router(state: AppState) -> Router { @@ -63,7 +83,10 @@ pub fn build_router(state: AppState) -> Router { // Attach the verified DID to /graphql when a signature is present. The // layer covers only routes added before it, so /graphql/ws (added after, // read-only subscriptions) stays open. - .layer(middleware::from_fn(auth::optional_signature)) + .layer(middleware::from_fn_with_state( + auth::SignatureBodyLimit(SIGNED_BODY_LIMIT), + auth::optional_signature, + )) .route_service("/graphql/ws", GraphQLSubscription::new(schema)); // ── Task routes (write — require HTTP Signature) ─────────────────────── @@ -74,6 +97,7 @@ pub fn build_router(state: AppState) -> Router { .route("/api/v1/tasks/{id}/complete", post(tasks::complete_task)) .route("/api/v1/tasks/{id}/fail", post(tasks::fail_task)), state.clone(), + SIGNED_BODY_LIMIT, ); // ── Task routes (read — open) ────────────────────────────────────────── @@ -107,6 +131,7 @@ pub fn build_router(state: AppState) -> Router { .layer(middleware::from_fn(rate_limit::rate_limit_by_did)) .layer(axum::Extension(limiter)), state.clone(), + SIGNED_BODY_LIMIT, ) .layer(middleware::from_fn(rate_limit::rate_limit_by_ip)) .layer(axum::Extension(create_ip_limiter)); @@ -181,6 +206,7 @@ pub fn build_router(state: AppState) -> Router { axum::routing::delete(agents::deregister_agent), ), state.clone(), + SIGNED_BODY_LIMIT, ); // Body limit is raised to GITLAWB_MAX_PACK_BYTES (default 2 GB) for git @@ -205,6 +231,7 @@ pub fn build_router(state: AppState) -> Router { .layer(DefaultBodyLimit::disable()) .layer(RequestBodyLimitLayer::new(pack_limit)), state.clone(), + pack_limit, ) .layer(middleware::from_fn(rate_limit::rate_limit_by_ip)) .layer(axum::Extension(push_limiter)); @@ -216,7 +243,10 @@ pub fn build_router(state: AppState) -> Router { // stays unsigned — gating the pin index is tracked separately (#121). let ipfs_routes = Router::new() .route("/ipfs/{cid}", get(ipfs::get_by_cid)) - .layer(middleware::from_fn(auth::optional_signature)) + .layer(middleware::from_fn_with_state( + auth::SignatureBodyLimit(SIGNED_BODY_LIMIT), + auth::optional_signature, + )) .merge(Router::new().route("/api/v1/ipfs/pins", get(ipfs::list_pins))); // ── Arweave permanent anchors ────────────────────────────────────────── @@ -247,6 +277,7 @@ pub fn build_router(state: AppState) -> Router { post(bounties::dispute_bounty), ), state.clone(), + SIGNED_BODY_LIMIT, ); // ── Bounty routes (read — open) ────────────────────────────────────── @@ -262,12 +293,16 @@ pub fn build_router(state: AppState) -> Router { "/api/v1/agents/{did}/bounties", get(bounties::agent_bounty_stats), ) - .layer(middleware::from_fn(auth::optional_signature)); + .layer(middleware::from_fn_with_state( + auth::SignatureBodyLimit(SIGNED_BODY_LIMIT), + auth::optional_signature, + )); // ── Profile routes (write — require HTTP Signature) ───────────────── let profile_write_routes = add_auth_layers( Router::new().route("/api/v1/profile", axum::routing::put(profiles::set_profile)), state.clone(), + SIGNED_BODY_LIMIT, ); // ── Issue routes (write — require HTTP Signature, no rate limit) ───── @@ -282,6 +317,7 @@ pub fn build_router(state: AppState) -> Router { post(issues::create_issue_comment), ), state.clone(), + SIGNED_BODY_LIMIT, ); // ── Peer discovery routes ───────────────────────────────────────────── @@ -300,6 +336,7 @@ pub fn build_router(state: AppState) -> Router { let sync_trigger_routes = add_auth_layers( Router::new().route("/api/v1/sync/trigger", post(peers::trigger_sync)), state.clone(), + SIGNED_BODY_LIMIT, ) .layer(middleware::from_fn(rate_limit::rate_limit_by_ip)) .layer(axum::Extension(rate_limit::IpRateLimiter { @@ -316,9 +353,12 @@ pub fn build_router(state: AppState) -> Router { .route("/api/v1/peers/announce", post(peers::announce)) .route("/api/v1/sync/notify", post(peers::notify_sync)); peer_write_routes = if state.config.require_signed_peer_writes { - add_auth_layers(peer_write_routes, state.clone()) + add_auth_layers(peer_write_routes, state.clone(), SIGNED_BODY_LIMIT) } else { - peer_write_routes.layer(middleware::from_fn(auth::optional_signature)) + peer_write_routes.layer(middleware::from_fn_with_state( + auth::SignatureBodyLimit(SIGNED_BODY_LIMIT), + auth::optional_signature, + )) }; let peer_write_routes = peer_write_routes .layer(middleware::from_fn(rate_limit::rate_limit_by_ip)) @@ -417,7 +457,10 @@ pub fn build_router(state: AppState) -> Router { "/api/v1/repos/{owner}/{repo}/replicas", get(replicas::list_replicas), ) - .layer(middleware::from_fn(auth::optional_signature)); + .layer(middleware::from_fn_with_state( + auth::SignatureBodyLimit(SIGNED_BODY_LIMIT), + auth::optional_signature, + )); // git-upload-pack (clone/fetch) — same raised body limit as receive-pack so // large pack responses from the server don't get truncated on the client side. @@ -449,7 +492,13 @@ pub fn build_router(state: AppState) -> Router { ) .layer(DefaultBodyLimit::disable()) .layer(RequestBodyLimitLayer::new(pack_limit)) - .layer(middleware::from_fn(auth::optional_signature)); + // `pack_limit`, not SIGNED_BODY_LIMIT: git-upload-pack POSTs live here and + // the group has deliberately raised its ceiling, so the pre-auth buffer for + // a signed fetch negotiation must match rather than narrow it. + .layer(middleware::from_fn_with_state( + auth::SignatureBodyLimit(pack_limit), + auth::optional_signature, + )); // ── Meta ────────────────────────────────────────────────────────────── let meta_routes = Router::new() diff --git a/crates/gitlawb-node/src/test_support.rs b/crates/gitlawb-node/src/test_support.rs index be6fb7b8..ce634a7a 100644 --- a/crates/gitlawb-node/src/test_support.rs +++ b/crates/gitlawb-node/src/test_support.rs @@ -115,6 +115,7 @@ mod tests { use crate::db::{AgentTask, RepoRecord}; use axum::http::StatusCode; use chrono::Utc; + use std::sync::atomic::{AtomicUsize, Ordering}; use tower::ServiceExt; fn seed_repo(owner_did: &str, name: &str) -> RepoRecord { @@ -1695,7 +1696,10 @@ mod tests { "/api/v1/repos/{owner}/{repo}/hooks", axum::routing::get(crate::api::webhooks::list_webhooks), ) - .layer(axum::middleware::from_fn(crate::auth::optional_signature)) + .layer(axum::middleware::from_fn_with_state( + crate::auth::SignatureBodyLimit(crate::server::SIGNED_BODY_LIMIT), + crate::auth::optional_signature, + )) .with_state(state); let resp = router.oneshot(req).await.unwrap(); @@ -1816,7 +1820,10 @@ mod tests { "/{owner}/{repo}/info/refs", axum::routing::get(crate::api::repos::git_info_refs), ) - .layer(axum::middleware::from_fn(crate::auth::optional_signature)) + .layer(axum::middleware::from_fn_with_state( + crate::auth::SignatureBodyLimit(crate::server::SIGNED_BODY_LIMIT), + crate::auth::optional_signature, + )) .with_state(state.clone()) }; let path = |service: &str| format!("/{short}/ir-priv.git/info/refs?service={service}"); @@ -1915,7 +1922,10 @@ mod tests { "/{owner}/{repo}/git-receive-pack", axum::routing::post(crate::api::repos::git_receive_pack), ) - .layer(axum::middleware::from_fn(crate::auth::require_signature)) + .layer(axum::middleware::from_fn_with_state( + crate::auth::SignatureBodyLimit(crate::server::SIGNED_BODY_LIMIT), + crate::auth::require_signature, + )) .with_state(state); let req = Request::builder() @@ -1963,7 +1973,10 @@ mod tests { "/{owner}/{repo}/git-upload-pack", axum::routing::post(crate::api::repos::git_upload_pack), ) - .layer(axum::middleware::from_fn(crate::auth::optional_signature)) + .layer(axum::middleware::from_fn_with_state( + crate::auth::SignatureBodyLimit(crate::server::SIGNED_BODY_LIMIT), + crate::auth::optional_signature, + )) .with_state(state.clone()) }; // A non-empty body (git-remote-gitlawb skips the POST when the body is empty). @@ -2114,7 +2127,10 @@ mod tests { "/{owner}/{repo}/info/refs", axum::routing::get(crate::api::repos::git_info_refs), ) - .layer(axum::middleware::from_fn(crate::auth::optional_signature)) + .layer(axum::middleware::from_fn_with_state( + crate::auth::SignatureBodyLimit(crate::server::SIGNED_BODY_LIMIT), + crate::auth::optional_signature, + )) .with_state(state.clone()) }; async fn body_of(resp: axum::response::Response) -> String { @@ -2991,7 +3007,10 @@ mod tests { "/ipfs/{cid}", axum::routing::get(crate::api::ipfs::get_by_cid), ) - .layer(axum::middleware::from_fn(crate::auth::optional_signature)) + .layer(axum::middleware::from_fn_with_state( + crate::auth::SignatureBodyLimit(crate::server::SIGNED_BODY_LIMIT), + crate::auth::optional_signature, + )) .with_state(state.clone()) } async fn cid_parts(resp: axum::response::Response) -> (StatusCode, String) { @@ -5183,4 +5202,284 @@ mod tests { "result includes the deep cert matching the prefix" ); } + + // ── Pre-auth body buffering in `require_signature` ─────────────────────── + // + // `require_signature` buffers the whole body BEFORE it verifies anything, so + // an unauthenticated caller could make the node hold an arbitrary amount of + // memory just by opening a request. Nothing upstream bounded it: on the git + // write routes `RequestBodyLimitLayer` sits on the INNER router (a later + // `.layer()` is the OUTER service, so auth runs first and sees the raw body), + // and on the other eight signed groups axum's 2 MB `DefaultBodyLimit` only + // works through the extractors, which this middleware bypasses by collecting + // the body itself. + // + // Every test below asserts on bytes ACTUALLY POLLED out of the body, not just + // the status code: a status-only assertion passes whether or not the body was + // drained, which is exactly the failure mode being fixed. + + /// A streaming body of `total` bytes in `chunk`-sized frames that counts the + /// bytes a consumer actually pulls out of it. The counter is the measurement + /// the drain assertions rest on. + fn counting_body(total: usize, chunk: usize) -> (Body, std::sync::Arc) { + let polled = std::sync::Arc::new(AtomicUsize::new(0)); + let counter = polled.clone(); + let stream = futures::stream::unfold(0usize, move |sent| { + let counter = counter.clone(); + async move { + if sent >= total { + return None; + } + let n = chunk.min(total - sent); + counter.fetch_add(n, Ordering::SeqCst); + Some(( + Ok::<_, std::convert::Infallible>(bytes::Bytes::from(vec![0u8; n])), + sent + n, + )) + } + }); + (Body::from_stream(stream), polled) + } + + /// Frame size for the counting bodies. `Limited` rejects on the frame that + /// carries the total past the limit, and that frame has already been polled, + /// so the drain assertions allow exactly one frame of slack over the limit. + const DRAIN_CHUNK: usize = 8 * 1024; + + /// A test state whose git routes are capped at `max_pack_bytes`, so an + /// oversize push body is a few hundred KiB rather than gigabytes. + fn state_with_pack_limit(mut state: AppState, max_pack_bytes: usize) -> AppState { + use clap::Parser; + state.config = Arc::new(crate::config::Config::parse_from([ + "gitlawb-node", + "--max-pack-bytes", + &max_pack_bytes.to_string(), + ])); + state + } + + /// Scenario 1, the regression test for the probe that found this: an UNSIGNED + /// POST to `/{owner}/{repo}/git-receive-pack` with a body far larger than the + /// configured `GITLAWB_MAX_PACK_BYTES`. Before the fix this drained all 4 MiB + /// into memory and then returned 401 (no DID, no signature, no repo needed). + #[tokio::test] + async fn unsigned_oversize_push_is_not_drained_into_memory() { + const LIMIT: usize = 64 * 1024; + let state = state_with_pack_limit(test_state_lazy(), LIMIT); + let router = crate::server::build_router(state); + + let (body, polled) = counting_body(4 * 1024 * 1024, DRAIN_CHUNK); + let req = Request::builder() + .method(Method::POST) + .uri("/zowner/repo/git-receive-pack") + .body(body) + .unwrap(); + + let resp = router.oneshot(req).await.unwrap(); + let drained = polled.load(Ordering::SeqCst); + assert!( + drained <= LIMIT + DRAIN_CHUNK, + "unsigned push must stop reading at the pack limit, drained {drained} of 4 MiB \ + against a {LIMIT}-byte limit" + ); + assert_eq!( + resp.status(), + StatusCode::PAYLOAD_TOO_LARGE, + "an over-limit body is rejected as too large" + ); + } + + /// Scenario 2: the same hole on a non-git signed group. These eight groups + /// have no `RequestBodyLimitLayer` at all and rely on axum's 2 MB default, + /// which the middleware's own `collect()` bypasses. + #[tokio::test] + async fn unsigned_oversize_profile_write_is_not_drained_into_memory() { + let router = crate::server::build_router(test_state_lazy()); + + let (body, polled) = counting_body(6 * 1024 * 1024, DRAIN_CHUNK); + let req = Request::builder() + .method(Method::PUT) + .uri("/api/v1/profile") + .header(axum::http::header::CONTENT_TYPE, "application/json") + .body(body) + .unwrap(); + + let resp = router.oneshot(req).await.unwrap(); + let drained = polled.load(Ordering::SeqCst); + assert!( + drained <= crate::server::SIGNED_BODY_LIMIT + DRAIN_CHUNK, + "non-git signed routes stop reading at the 2 MB default, drained {drained}" + ); + assert_eq!(resp.status(), StatusCode::PAYLOAD_TOO_LARGE); + } + + /// Scenario 5: the over-limit rejection is distinguishable. It is neither the + /// 401 an unsigned request gets nor the 400 an unreadable body gets, so a + /// client can tell "too big" from "not authenticated". + #[tokio::test] + async fn over_limit_is_413_not_401_or_400() { + const LIMIT: usize = 64 * 1024; + let state = state_with_pack_limit(test_state_lazy(), LIMIT); + let router = crate::server::build_router(state); + + let (body, _polled) = counting_body(LIMIT * 4, DRAIN_CHUNK); + let resp = router + .oneshot( + Request::builder() + .method(Method::POST) + .uri("/zowner/repo/git-receive-pack") + .body(body) + .unwrap(), + ) + .await + .unwrap(); + + let status = resp.status(); + assert_ne!(status, StatusCode::UNAUTHORIZED, "size, not identity"); + assert_ne!( + status, + StatusCode::BAD_REQUEST, + "size, not an unreadable body" + ); + assert_eq!(status, StatusCode::PAYLOAD_TOO_LARGE); + } + + /// Scenario 6, degenerate cases: an empty body and a body exactly at the limit + /// are both under the cap, so they fall through to the existing signature + /// check and get the same 401 they always did. + #[tokio::test] + async fn empty_and_exactly_at_limit_bodies_behave_as_before() { + const LIMIT: usize = 64 * 1024; + let state = state_with_pack_limit(test_state_lazy(), LIMIT); + + let resp = crate::server::build_router(state.clone()) + .oneshot( + Request::builder() + .method(Method::POST) + .uri("/zowner/repo/git-receive-pack") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!( + resp.status(), + StatusCode::UNAUTHORIZED, + "an empty unsigned body still fails at the signature check, not the limit" + ); + + let (body, polled) = counting_body(LIMIT, DRAIN_CHUNK); + let resp = crate::server::build_router(state) + .oneshot( + Request::builder() + .method(Method::POST) + .uri("/zowner/repo/git-receive-pack") + .body(body) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!( + polled.load(Ordering::SeqCst), + LIMIT, + "a body exactly at the limit is read in full" + ); + assert_eq!( + resp.status(), + StatusCode::UNAUTHORIZED, + "exactly at the limit is allowed through to the signature check" + ); + } + + /// Scenario 3, the discriminator: without it the fix could "pass" by rejecting + /// everything. A correctly signed push just under `max_pack_bytes` must clear + /// the limit AND the signature check with its body intact, which means the + /// full body was read (content-digest is computed over it) and the request + /// reached the handler. The handler then 404s because no repo was seeded, + /// which is the first thing `git_receive_pack` does after auth. + #[sqlx::test] + async fn signed_push_just_under_the_pack_limit_still_reaches_the_handler(pool: PgPool) { + use gitlawb_core::http_sig::sign_request; + + const LIMIT: usize = 256 * 1024; + let state = state_with_pack_limit(test_state(pool).await, LIMIT); + let kp = Keypair::generate(); + let short = kp + .did() + .to_string() + .split(':') + .next_back() + .unwrap() + .to_string(); + + // Just under the cap, and signed over the exact bytes so content-digest + // verification proves the middleware handed the body through unmodified. + let payload = vec![0u8; LIMIT - 1]; + let path = format!("/{short}/pushrepo/git-receive-pack"); + let signed = sign_request(&kp, "POST", &path, &payload); + let len = payload.len(); + + let resp = crate::server::build_router(state) + .oneshot( + Request::builder() + .method(Method::POST) + .uri(&path) + .header("content-digest", signed.content_digest) + .header("signature-input", signed.signature_input) + .header("signature", signed.signature) + .body(Body::from(payload)) + .unwrap(), + ) + .await + .unwrap(); + + assert_ne!( + resp.status(), + StatusCode::PAYLOAD_TOO_LARGE, + "a {len}-byte push under a {LIMIT}-byte cap must not be rejected as too large" + ); + assert_eq!( + resp.status(), + StatusCode::NOT_FOUND, + "auth passed with the body intact; the handler 404s on the unseeded repo" + ); + } + + /// Scenario 4: a correctly signed, in-limit request on a non-git signed route + /// still succeeds end to end, so the 2 MB constant did not narrow anything + /// that used to work. + #[sqlx::test] + async fn signed_in_limit_profile_write_still_succeeds(pool: PgPool) { + use gitlawb_core::http_sig::sign_request; + + let kp = Keypair::generate(); + let did = kp.did().to_string(); + let payload = + serde_json::to_vec(&serde_json::json!({ "display_name": "limit-probe" })).unwrap(); + let signed = sign_request(&kp, "PUT", "/api/v1/profile", &payload); + + let resp = crate::server::build_router(test_state(pool).await) + .oneshot( + Request::builder() + .method(Method::PUT) + .uri("/api/v1/profile") + .header(axum::http::header::CONTENT_TYPE, "application/json") + .header("content-digest", signed.content_digest) + .header("signature-input", signed.signature_input) + .header("signature", signed.signature) + .body(Body::from(payload)) + .unwrap(), + ) + .await + .unwrap(); + + assert_eq!( + resp.status(), + StatusCode::OK, + "signed in-limit write succeeds" + ); + let body = json_body(resp).await; + assert_eq!(body["did"], did); + assert_eq!(body["display_name"], "limit-probe"); + } } From e8393fdc8008da2a07dd81da73b00d8f7616e2ba Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Mon, 27 Jul 2026 21:09:11 -0500 Subject: [PATCH 2/2] test(node): prove a signed push under the limit lands on disk, and an over-limit one writes nothing The body-limit guard was covered only by tests that reject. Nothing proved it still admits what it should, so the fix could have passed every test by refusing everything, and nothing would have caught a middleware that truncated or corrupted the body it hands downstream. These drive a real push: a source repo with a 300 KiB incompressible blob, an empty bare repo at the path the store resolves, and the byte-for-byte receive-pack request body captured off the wire from an actual `git push`. The body is captured rather than generated, because `git send-pack --stateless-rpc` emits remote-curl's extra RPC framing (a wrapping length plus a stray flush before PACK) and `git receive-pack --stateless-rpc` rejects it with a pack signature mismatch. The positive case sets the cap just above the body and asserts the ref moved and the tree resolves, not merely that the status was 200. That distinction is load-bearing: under a truncation mutation the status stays 200, because git reports the failure inside report-status, and only the on-disk assertion goes red. Running the whole suite under that mutation leaves exactly one test failing, so this is the only thing standing between a corrupted body and a green build. The negative case sets the cap just below the body and asserts 413 with neither the ref nor the commit present afterwards, so a rejected push cannot half-write. Its existence check peels to ^{commit}, since rev-parse echoes any well-formed 40-hex string back and would have passed vacuously. --- crates/gitlawb-node/src/test_support.rs | 283 ++++++++++++++++++++++++ 1 file changed, 283 insertions(+) diff --git a/crates/gitlawb-node/src/test_support.rs b/crates/gitlawb-node/src/test_support.rs index ce634a7a..9efd3bcc 100644 --- a/crates/gitlawb-node/src/test_support.rs +++ b/crates/gitlawb-node/src/test_support.rs @@ -5482,4 +5482,287 @@ mod tests { assert_eq!(body["did"], did); assert_eq!(body["display_name"], "limit-probe"); } + + // ── The buffered body must reach git byte-for-byte ────────────────────── + // + // Scenario 3 above proves a large signed push clears the limit and the + // signature check, but it stops at the handler's repo lookup. That leaves one + // hole: middleware that truncated or reordered the buffered body past some + // threshold would still pass it, because nothing downstream ever consumed the + // bytes. The two tests below close it by driving a REAL `git push` request + // into a REAL bare repo and asserting on the refs that land on disk. + + /// Removes a fixed path on drop, so a failing assertion still cleans up. + /// `repo_store::for_testing` pins the on-disk layout to `/tmp//.git`, + /// so these paths cannot be `tempfile::TempDir` randoms. + struct PushDirGuard(std::path::PathBuf); + + impl Drop for PushDirGuard { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.0); + } + } + + /// What [`real_push_fixture`] hands back: the exact request bytes a git client + /// sends, the commit that push carries, and the empty bare repo it targets. + struct PushFixture { + body: Vec, + new_sha: String, + bare: std::path::PathBuf, + _guards: Vec, + } + + /// Build a real push: a source repo holding one commit with `payload` bytes of + /// incompressible content, an EMPTY bare repo at the exact path `repo_store` + /// will write to, and the `git-receive-pack` request body a real `git push` + /// produces for it. + /// + /// The body is captured off the wire from a throwaway local server rather than + /// hand-assembled, so the limit tests drive the bytes a client actually sends + /// (pkt-line command list, flush, then the pack) instead of an approximation + /// that real `git receive-pack` might reject for unrelated reasons. + async fn real_push_fixture(owner_did: &str, name: &str, payload: usize) -> PushFixture { + use std::process::Command; + + let run = |args: &[&str], cwd: &std::path::Path| { + let out = Command::new("git") + .args(args) + .current_dir(cwd) + .output() + .expect("git runs"); + assert!( + out.status.success(), + "git {args:?} failed: {}", + String::from_utf8_lossy(&out.stderr) + ); + String::from_utf8_lossy(&out.stdout).trim().to_string() + }; + + let slug = owner_did.replace([':', '/'], "_"); + let mut guards = Vec::new(); + + // Source repo with one commit. The content is incompressible so the pack + // stays close to `payload`: a file of zeros would deflate to nothing and + // the request would never approach the limit under test. + let src = std::env::temp_dir().join(format!("gl-push-src-{slug}-{name}")); + let _ = std::fs::remove_dir_all(&src); + std::fs::create_dir_all(&src).unwrap(); + guards.push(PushDirGuard(src.clone())); + run(&["init", "-q", "-b", "main"], &src); + run(&["config", "user.email", "t@t"], &src); + run(&["config", "user.name", "t"], &src); + let mut blob = Vec::with_capacity(payload); + let mut x: u64 = 0x2545_f491_4f6c_dd1d; + while blob.len() < payload { + x ^= x << 13; + x ^= x >> 7; + x ^= x << 17; + blob.extend_from_slice(&x.to_le_bytes()); + } + blob.truncate(payload); + std::fs::write(src.join("big.bin"), &blob).unwrap(); + run(&["add", "big.bin"], &src); + run(&["commit", "-qm", "big"], &src); + let new_sha = run(&["rev-parse", "HEAD"], &src); + + // Empty bare repo at the path repo_store.acquire_write() resolves to. + let bare = std::path::PathBuf::from("/tmp") + .join(&slug) + .join(format!("{name}.git")); + let _ = std::fs::remove_dir_all(&bare); + std::fs::create_dir_all(bare.parent().unwrap()).unwrap(); + run(&["init", "-q", "--bare", bare.to_str().unwrap()], &src); + guards.push(PushDirGuard(bare.clone())); + + // Capture server: advertises the real (empty) bare repo so the client + // computes a genuine old→new command list, and records the POST body. + let adv_repo = bare.clone(); + let captured: Arc>> = Arc::default(); + let sink = captured.clone(); + let app = Router::new() + .route( + "/repo.git/info/refs", + axum::routing::get(move || { + let repo = adv_repo.clone(); + async move { + crate::git::smart_http::info_refs(&repo, "git-receive-pack") + .await + .expect("advertise refs") + } + }), + ) + .route( + "/repo.git/git-receive-pack", + axum::routing::post(move |body: bytes::Bytes| { + let sink = sink.clone(); + async move { + *sink.lock().unwrap() = body.to_vec(); + StatusCode::OK + } + }), + ); + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let port = listener.local_addr().unwrap().port(); + let server = tokio::spawn(async move { axum::serve(listener, app).await.unwrap() }); + + let push_src = src.clone(); + tokio::task::spawn_blocking(move || { + // The capture server answers with an empty body, so git reports a + // disconnect once it has sent the request. The request is all we want + // here, so the exit status is deliberately not asserted. + let _ = Command::new("git") + .args([ + "push", + &format!("http://127.0.0.1:{port}/repo.git"), + "HEAD:refs/heads/main", + ]) + .current_dir(&push_src) + .output() + .expect("git push runs"); + }) + .await + .unwrap(); + server.abort(); + + let body = captured.lock().unwrap().clone(); + assert!( + body.windows(4).any(|w| w == b"PACK"), + "the captured request must carry a real pack, got {} bytes", + body.len() + ); + PushFixture { + body, + new_sha, + bare, + _guards: guards, + } + } + + /// Read a ref out of a bare repo, or `None` when it does not exist. + fn bare_ref(bare: &std::path::Path, name: &str) -> Option { + let out = std::process::Command::new("git") + .args(["--git-dir", bare.to_str().unwrap(), "rev-parse", name]) + .output() + .expect("git runs"); + out.status + .success() + .then(|| String::from_utf8_lossy(&out.stdout).trim().to_string()) + } + + /// The discriminator with teeth: a signed push of a real ~300 KiB pack against + /// a limit just above it must not only return 200, it must LAND. Asserting on + /// the ref that appears in the bare repo is what catches a middleware that + /// buffers the body but hands a truncated or reordered copy downstream, which + /// a status-only assertion cannot see. + #[sqlx::test] + async fn signed_large_push_under_the_limit_lands_on_disk(pool: PgPool) { + use gitlawb_core::http_sig::sign_request; + + let kp = Keypair::generate(); + let owner_did = kp.did().to_string(); + let short = owner_did.split(':').next_back().unwrap().to_string(); + let fixture = real_push_fixture(&owner_did, "bigpush-ok", 300 * 1024).await; + let len = fixture.body.len(); + assert!( + len > 256 * 1024, + "the fixture push must be many frames long to exercise the limit, got {len} bytes" + ); + + // The cap sits just above this push, so passing means the limit admitted a + // body it was sized for, not that the limit was slack. + let state = state_with_pack_limit(test_state(pool).await, len + 1024); + state + .db + .create_repo(&seed_repo(&owner_did, "bigpush-ok")) + .await + .expect("seed repo"); + + let path = format!("/{short}/bigpush-ok.git/git-receive-pack"); + let signed = sign_request(&kp, "POST", &path, &fixture.body); + let resp = crate::server::build_router(state) + .oneshot( + Request::builder() + .method(Method::POST) + .uri(&path) + .header("content-digest", signed.content_digest) + .header("signature-input", signed.signature_input) + .header("signature", signed.signature) + .body(Body::from(fixture.body.clone())) + .unwrap(), + ) + .await + .unwrap(); + + assert_eq!( + resp.status(), + StatusCode::OK, + "a {len}-byte push under a {}-byte cap must be served", + len + 1024 + ); + assert_eq!( + bare_ref(&fixture.bare, "refs/heads/main").as_deref(), + Some(fixture.new_sha.as_str()), + "the push must take effect: refs/heads/main points at the pushed commit" + ); + assert!( + bare_ref(&fixture.bare, &format!("{}^{{tree}}", fixture.new_sha)).is_some(), + "the pushed objects must be present, not just the ref" + ); + } + + /// The negative twin: the same real push against a cap one byte below it is + /// rejected 413, and the repo is untouched. A partial write here would mean + /// the middleware forwarded a prefix of an over-limit body. + #[sqlx::test] + async fn signed_large_push_over_the_limit_is_rejected_and_writes_nothing(pool: PgPool) { + use gitlawb_core::http_sig::sign_request; + + let kp = Keypair::generate(); + let owner_did = kp.did().to_string(); + let short = owner_did.split(':').next_back().unwrap().to_string(); + let fixture = real_push_fixture(&owner_did, "bigpush-over", 300 * 1024).await; + let len = fixture.body.len(); + + let state = state_with_pack_limit(test_state(pool).await, len - 1); + state + .db + .create_repo(&seed_repo(&owner_did, "bigpush-over")) + .await + .expect("seed repo"); + + let path = format!("/{short}/bigpush-over.git/git-receive-pack"); + let signed = sign_request(&kp, "POST", &path, &fixture.body); + let resp = crate::server::build_router(state) + .oneshot( + Request::builder() + .method(Method::POST) + .uri(&path) + .header("content-digest", signed.content_digest) + .header("signature-input", signed.signature_input) + .header("signature", signed.signature) + .body(Body::from(fixture.body.clone())) + .unwrap(), + ) + .await + .unwrap(); + + assert_eq!( + resp.status(), + StatusCode::PAYLOAD_TOO_LARGE, + "a {len}-byte push against a {}-byte cap is too large", + len - 1 + ); + assert_eq!( + bare_ref(&fixture.bare, "refs/heads/main"), + None, + "a rejected push must leave no ref behind" + ); + // Peel to `^{commit}`: `rev-parse` echoes a bare 40-hex SHA back without + // checking that the object exists, so only the peeled form is evidence. + assert_eq!( + bare_ref(&fixture.bare, &format!("{}^{{commit}}", fixture.new_sha)), + None, + "a rejected push must leave no objects behind" + ); + } }