Skip to content

Commit 8dd294b

Browse files
committed
fix(gl,node): make MCP git_refs advertisement-complete and unbrake signed reads
gl: the MCP git_refs path parsed the smart-HTTP ref advertisement out of a lossy UTF-8 decode of a silently capped body. Split read_body_capped into a raw CappedBytes reader plus a text view: byte consumers keep non-UTF-8 ref names intact (no U+FFFD corruption of names or pkt-line accounting), and a capped or mid-read-failed body now errors as incomplete instead of a valid prefix parsing as the complete answer. Tests cover empty-200, prefix-at-cap, read-failure-after-prefix, and non-UTF-8 ref names. node: the signed-write IP brake classified whole routers, but write_routes chains PUT/DELETE/GET on the visibility path and gl signs that owner-only GET — so reads spent the write quota and listings 429ed behind shared NAT. Add an explicit opt-in skip_reads to IpRateLimiter (GET/HEAD exempt), set only on the signed-write brake; every read-braking bucket (e.g. /ipfs) stays as-is. Unit test pins the middleware seam; test_support E2E proves the brake still rejects writes while serving the visibility GET.
1 parent f37f29d commit 8dd294b

6 files changed

Lines changed: 499 additions & 16 deletions

File tree

‎crates/gitlawb-node/src/api/peers.rs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -717,6 +717,7 @@ mod tests {
717717
.layer(Extension(IpRateLimiter {
718718
limiter: RateLimiter::new(60, Duration::from_secs(3600)),
719719
trust: TrustedProxy::None,
720+
skip_reads: false,
720721
}))
721722
.with_state(state);
722723
let mut req =
@@ -765,6 +766,7 @@ mod tests {
765766
.layer(Extension(IpRateLimiter {
766767
limiter: RateLimiter::new(1, Duration::from_secs(3600)),
767768
trust: TrustedProxy::None,
769+
skip_reads: false,
768770
}))
769771
.with_state(state);
770772
let peer = "203.0.113.5:5000".parse::<SocketAddr>().unwrap();

‎crates/gitlawb-node/src/rate_limit.rs‎

Lines changed: 90 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ use std::sync::Arc;
44
use std::time::{Duration, Instant};
55

66
use axum::extract::{ConnectInfo, Request};
7-
use axum::http::{HeaderMap, StatusCode};
7+
use axum::http::{HeaderMap, Method, StatusCode};
88
use axum::middleware::Next;
99
use axum::response::{IntoResponse, Response};
1010
use axum::Json;
@@ -361,6 +361,18 @@ pub fn client_key(
361361
pub struct IpRateLimiter {
362362
pub limiter: RateLimiter,
363363
pub trust: TrustedProxy,
364+
/// Exempt GET/HEAD from this bucket. The signed-write brake classifies
365+
/// whole routers, but `write_routes` chains PUT/DELETE/GET on one path
366+
/// (`/api/v1/repos/{owner}/{repo}/visibility`, `server.rs`), and `gl
367+
/// visibility list` deliberately signs that owner-only GET — so without
368+
/// this the read spends the write quota and a normal listing 429s after a
369+
/// few writes from the same IP (one NAT egress makes the coupling vivid).
370+
/// The method check is the same general form `consume_signature` uses for
371+
/// its ledger skip: a read has no write side effect for the brake to
372+
/// protect. Leave this `false` on any limiter whose job IS braking reads —
373+
/// the `/ipfs/{cid}` brake exists precisely to cap GET-driven history
374+
/// walks, so a blanket skip inside `rate_limit_by_ip` would delete it.
375+
pub skip_reads: bool,
364376
}
365377

366378
/// Infallible extractor for the socket peer address from `ConnectInfo`. Yields
@@ -418,10 +430,16 @@ pub async fn rate_limit_by_ip(request: Request, next: Next) -> Response {
418430
.map(|c| c.0);
419431

420432
if let Some(limiter) = limiter {
421-
if let Some(key) = client_key(request.headers(), peer, limiter.trust) {
422-
if !limiter.limiter.check(&key).await {
423-
tracing::warn!(key = %key, path = %request.uri().path(), "per-IP rate limit exceeded");
424-
return too_many_requests();
433+
// Opt-in read exemption (see `IpRateLimiter::skip_reads`): only a
434+
// limiter that declares itself write-only skips GET/HEAD, so the /ipfs
435+
// read brake and every other read-facing bucket keep braking reads.
436+
let is_read = matches!(*request.method(), Method::GET | Method::HEAD);
437+
if !(limiter.skip_reads && is_read) {
438+
if let Some(key) = client_key(request.headers(), peer, limiter.trust) {
439+
if !limiter.limiter.check(&key).await {
440+
tracing::warn!(key = %key, path = %request.uri().path(), "per-IP rate limit exceeded");
441+
return too_many_requests();
442+
}
425443
}
426444
}
427445
}
@@ -757,6 +775,7 @@ mod tests {
757775
let router = ip_limited_router(IpRateLimiter {
758776
limiter: RateLimiter::new(2, Duration::from_secs(60)),
759777
trust: TrustedProxy::None,
778+
skip_reads: false,
760779
});
761780
let peer: SocketAddr = "203.0.113.7:5000".parse().unwrap();
762781
// Two requests inside the budget, the third over it (shared Arc state).
@@ -773,6 +792,7 @@ mod tests {
773792
let router = ip_limited_router(IpRateLimiter {
774793
limiter: RateLimiter::new(1, Duration::from_secs(60)),
775794
trust: TrustedProxy::None,
795+
skip_reads: false,
776796
});
777797
let a: SocketAddr = "203.0.113.1:1".parse().unwrap();
778798
let b: SocketAddr = "203.0.113.2:1".parse().unwrap();
@@ -781,6 +801,70 @@ mod tests {
781801
assert_eq!(post_from(&router, a).await, StatusCode::TOO_MANY_REQUESTS);
782802
}
783803

804+
/// `skip_reads` mechanics, isolated from the production router: on a
805+
/// limiter that declares itself write-only, GET and HEAD never touch the
806+
/// bucket — they pass an already-exhausted one AND never debit it — while
807+
/// a mutation from the same peer stays braked. On a limiter that does not
808+
/// opt in (the /ipfs read brake), GET is braked exactly as before. The
809+
/// production wiring (the signed-write brake on `write_routes`, whose one
810+
/// GET is the signed visibility listing) is proven end-to-end by
811+
/// `signed_write_brake_rejects_writes_but_serves_visibility_get` in
812+
/// `test_support`; this pins the middleware seam it rides on.
813+
#[tokio::test]
814+
async fn skip_reads_exempts_get_and_head_but_still_brakes_writes() {
815+
use tower::ServiceExt;
816+
let router = |skip_reads: bool| {
817+
axum::Router::new()
818+
.route(
819+
"/thing",
820+
axum::routing::get(|| async { StatusCode::OK })
821+
.head(|| async { StatusCode::OK })
822+
.put(|| async { StatusCode::OK }),
823+
)
824+
.layer(axum::middleware::from_fn(rate_limit_by_ip))
825+
.layer(axum::Extension(IpRateLimiter {
826+
limiter: RateLimiter::new(1, Duration::from_secs(60)),
827+
trust: TrustedProxy::None,
828+
skip_reads,
829+
}))
830+
};
831+
let peer: SocketAddr = "203.0.113.9:5000".parse().unwrap();
832+
let send = |router: &axum::Router, method: axum::http::Method| {
833+
let mut req = axum::http::Request::builder()
834+
.method(method)
835+
.uri("/thing")
836+
.body(axum::body::Body::empty())
837+
.unwrap();
838+
req.extensions_mut().insert(ConnectInfo(peer));
839+
let router = router.clone();
840+
async move { router.oneshot(req).await.unwrap().status() }
841+
};
842+
843+
// Opted-in limiter: the PUT spends the single slot...
844+
let r = router(true);
845+
assert_eq!(send(&r, axum::http::Method::PUT).await, StatusCode::OK);
846+
// ...reads from the same peer still pass the exhausted bucket...
847+
assert_eq!(send(&r, axum::http::Method::GET).await, StatusCode::OK);
848+
assert_eq!(send(&r, axum::http::Method::HEAD).await, StatusCode::OK);
849+
// ...and did not debit it behind the scenes some other way — the next
850+
// mutation is still the one that gets braked.
851+
assert_eq!(
852+
send(&r, axum::http::Method::PUT).await,
853+
StatusCode::TOO_MANY_REQUESTS,
854+
"the write brake must keep braking writes while exempting reads"
855+
);
856+
857+
// Non-opted-in limiter: GET is braked like any other method, so the
858+
// exemption cannot silently spread to read-braking buckets like /ipfs.
859+
let r = router(false);
860+
assert_eq!(send(&r, axum::http::Method::GET).await, StatusCode::OK);
861+
assert_eq!(
862+
send(&r, axum::http::Method::GET).await,
863+
StatusCode::TOO_MANY_REQUESTS,
864+
"a limiter that did not opt in must keep braking reads"
865+
);
866+
}
867+
784868
// ── creation-route layering: IP brake runs BEFORE auth ──────────────
785869

786870
/// A minimal stand-in for the creation route group: an inner "auth" layer
@@ -826,6 +910,7 @@ mod tests {
826910
let router = ip_limited_over_auth_router(IpRateLimiter {
827911
limiter: RateLimiter::new(1, Duration::from_secs(60)),
828912
trust: TrustedProxy::None,
913+
skip_reads: false,
829914
});
830915
let peer: SocketAddr = "203.0.113.42:7000".parse().unwrap();
831916

‎crates/gitlawb-node/src/server.rs‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,9 +98,15 @@ pub fn build_router(state: AppState) -> Router {
9898
// quotas. Each group layers it OUTSIDE `add_auth_layers` (axum runs the last
9999
// `.layer` first) so an over-limit request is rejected before signature
100100
// verification burns CPU and before the ledger is charged.
101+
// `skip_reads`: `write_routes` chains PUT/DELETE/GET on the visibility
102+
// path, and `gl visibility list` signs that owner-only GET. A write-only
103+
// brake must not let reads spend the write quota (or a listing starts
104+
// 429ing behind a shared NAT after a handful of writes), mirroring the
105+
// method skip `consume_signature` applies to the ledger for the same GET.
101106
let signed_write_ip_limiter = rate_limit::IpRateLimiter {
102107
limiter: state.signed_write_rate_limiter.clone(),
103108
trust: state.push_limiter_trust,
109+
skip_reads: true,
104110
};
105111

106112
// ── Task routes (write — require HTTP Signature) ───────────────────────
@@ -132,6 +138,7 @@ pub fn build_router(state: AppState) -> Router {
132138
let create_ip_limiter = rate_limit::IpRateLimiter {
133139
limiter: state.create_ip_rate_limiter.clone(),
134140
trust: state.push_limiter_trust,
141+
skip_reads: false,
135142
};
136143
let creation_routes = add_auth_layers(
137144
Router::new()
@@ -236,6 +243,7 @@ pub fn build_router(state: AppState) -> Router {
236243
let push_limiter = rate_limit::IpRateLimiter {
237244
limiter: state.push_rate_limiter.clone(),
238245
trust: state.push_limiter_trust,
246+
skip_reads: false,
239247
};
240248
let git_write_routes = add_auth_layers(
241249
Router::new()
@@ -263,6 +271,7 @@ pub fn build_router(state: AppState) -> Router {
263271
let ipfs_limiter = rate_limit::IpRateLimiter {
264272
limiter: state.ipfs_rate_limiter.clone(),
265273
trust: state.push_limiter_trust,
274+
skip_reads: false,
266275
};
267276
let ipfs_routes = Router::new()
268277
.route("/ipfs/{cid}", get(ipfs::get_by_cid))
@@ -363,6 +372,7 @@ pub fn build_router(state: AppState) -> Router {
363372
.layer(axum::Extension(rate_limit::IpRateLimiter {
364373
limiter: state.sync_trigger_rate_limiter.clone(),
365374
trust: state.push_limiter_trust,
375+
skip_reads: false,
366376
}));
367377

368378
// announce + notify keep their rolling-upgrade signature behavior (unsigned
@@ -383,6 +393,7 @@ pub fn build_router(state: AppState) -> Router {
383393
.layer(axum::Extension(rate_limit::IpRateLimiter {
384394
limiter: state.peer_write_rate_limiter.clone(),
385395
trust: state.push_limiter_trust,
396+
skip_reads: false,
386397
}));
387398

388399
// ── Read routes — open for public repos ───────────────────────────────

‎crates/gitlawb-node/src/test_support.rs‎

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1987,6 +1987,111 @@ mod tests {
19871987
);
19881988
}
19891989

1990+
/// The signed-write IP brake is write-only. `write_routes` chains
1991+
/// PUT/DELETE/GET on the visibility path, and `gl visibility list` signs
1992+
/// that owner-only GET, so a brake that counted methods it does not exist
1993+
/// for would let ordinary reads spend the write quota — behind one NAT
1994+
/// egress, a few writes would start 429ing every listing. Through the
1995+
/// PRODUCTION router with a single-slot bucket: the first write spends the
1996+
/// quota, a second write (and a DELETE) is 429, and the authenticated
1997+
/// visibility GET from the SAME exhausted IP is still served. MUTATION
1998+
/// (RED): set `skip_reads: false` on `signed_write_ip_limiter` in
1999+
/// `server.rs` (or drop the method check in `rate_limit_by_ip`) and the
2000+
/// final GET comes back 429.
2001+
#[sqlx::test]
2002+
async fn signed_write_brake_rejects_writes_but_serves_visibility_get(pool: PgPool) {
2003+
use gitlawb_core::http_sig::sign_request;
2004+
2005+
let kp = Keypair::generate();
2006+
let owner_did = kp.did().to_string();
2007+
// Short owner form in the URL path, as in the real-signature test above:
2008+
// no colons, so the signed @path matches path_and_query() byte-for-byte.
2009+
let short = owner_did.split(':').next_back().unwrap().to_string();
2010+
2011+
let mut state = test_state(pool).await;
2012+
// Single-slot bucket: the FIRST successful write exhausts it.
2013+
state.signed_write_rate_limiter =
2014+
crate::rate_limit::RateLimiter::new(1, Duration::from_secs(3600));
2015+
state.push_limiter_trust = crate::rate_limit::TrustedProxy::None;
2016+
let repo = seed_repo(&owner_did, "brake-repo");
2017+
state.db.create_repo(&repo).await.expect("seed repo");
2018+
let router = crate::server::build_router(state);
2019+
2020+
let peer: std::net::SocketAddr = "203.0.113.61:5000".parse().unwrap();
2021+
let path = format!("/api/v1/repos/{short}/brake-repo/visibility");
2022+
let put_body: &[u8] = br#"{"path_glob":"/","reader_dids":[]}"#;
2023+
// Each request is freshly signed — the brake is outermost, but a reused
2024+
// signature would trip the replay ledger and muddy which gate answered.
2025+
let signed_req = |method: &str, body: &'static [u8]| {
2026+
let signed = sign_request(&kp, method, &path, body);
2027+
let mut req = Request::builder()
2028+
.method(method)
2029+
.uri(&path)
2030+
.header(axum::http::header::CONTENT_TYPE, "application/json")
2031+
.header("content-digest", signed.content_digest)
2032+
.header("signature-input", signed.signature_input)
2033+
.header("signature", signed.signature)
2034+
.body(Body::from(body))
2035+
.unwrap();
2036+
req.extensions_mut()
2037+
.insert(axum::extract::ConnectInfo(peer));
2038+
req
2039+
};
2040+
2041+
// First write passes the brake, reaches the handler, and succeeds —
2042+
// debiting the single-slot bucket.
2043+
let resp = router
2044+
.clone()
2045+
.oneshot(signed_req("PUT", put_body))
2046+
.await
2047+
.unwrap();
2048+
assert!(
2049+
resp.status().is_success(),
2050+
"the first signed write from an IP must pass the brake, got {}",
2051+
resp.status()
2052+
);
2053+
2054+
// Second write from the same IP: braked with 429 before auth runs.
2055+
let resp = router
2056+
.clone()
2057+
.oneshot(signed_req("PUT", put_body))
2058+
.await
2059+
.unwrap();
2060+
assert_eq!(
2061+
resp.status(),
2062+
StatusCode::TOO_MANY_REQUESTS,
2063+
"an exhausted signed-write bucket must reject a PUT"
2064+
);
2065+
// A DELETE is a mutation too and shares the exhausted bucket.
2066+
let resp = router
2067+
.clone()
2068+
.oneshot(signed_req("DELETE", put_body))
2069+
.await
2070+
.unwrap();
2071+
assert_eq!(
2072+
resp.status(),
2073+
StatusCode::TOO_MANY_REQUESTS,
2074+
"an exhausted signed-write bucket must reject a DELETE"
2075+
);
2076+
2077+
// The signed visibility GET from the SAME IP bypasses the write brake
2078+
// and is fully served: 200 with the rule the first PUT created — the
2079+
// read did not merely dodge a 429, it reached the handler as the owner.
2080+
let resp = router.oneshot(signed_req("GET", b"")).await.unwrap();
2081+
assert_eq!(
2082+
resp.status(),
2083+
StatusCode::OK,
2084+
"the authenticated visibility GET must be served despite the exhausted write bucket"
2085+
);
2086+
let bytes = axum::body::to_bytes(resp.into_body(), usize::MAX)
2087+
.await
2088+
.unwrap();
2089+
assert!(
2090+
String::from_utf8_lossy(&bytes).contains("path_glob"),
2091+
"the owner sees the visibility rules through the exhausted-bucket IP"
2092+
);
2093+
}
2094+
19902095
/// A signed request body reused by the request-target trio below. Non-empty so
19912096
/// the content-digest the signature covers is a real hash rather than the
19922097
/// empty-body constant, which keeps `@path` the only component under test.

0 commit comments

Comments
 (0)