fix(node): bound the request body the signature middleware buffers before it verifies - #262
Open
beardthelion wants to merge 2 commits into
Open
fix(node): bound the request body the signature middleware buffers before it verifies#262beardthelion wants to merge 2 commits into
beardthelion wants to merge 2 commits into
Conversation
…fore 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.
… 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.
Contributor
📝 WalkthroughWalkthroughSignature authentication now uses configurable bounded request-body buffering. Routes pass shared or Git pack-specific limits through Axum state, and integration tests cover oversized requests, bounded draining, status codes, and Git push outcomes. ChangesSignature body limits
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Client
participant SignatureMiddleware
participant LimitedBody
participant RouteHandler
Client->>SignatureMiddleware: Send signed or unsigned request
SignatureMiddleware->>LimitedBody: Buffer body up to configured limit
LimitedBody-->>SignatureMiddleware: Body, 413, or 400 result
SignatureMiddleware->>RouteHandler: Forward verified request
RouteHandler-->>Client: Application response
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/gitlawb-node/src/server.rs (1)
225-237: 🩺 Stability & Availability | 🔵 TrivialMemory ceiling on the push path is now
max_pack_bytes × in-flight unauthenticated pushes.Bounding the pre-auth buffer is the right fix, but with the 2 GB default a handful of concurrent unsigned receive-pack POSTs can still pin a lot of RAM before any identity check. The per-IP limiter in front helps only for repeat offenders from one source. Worth pairing this with a global in-flight concurrency cap (e.g.
tower::limit::GlobalConcurrencyLimitLayeron the git write group) or a request read timeout, tracked separately if it's out of scope here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/gitlawb-node/src/server.rs` around lines 225 - 237, The git write routes lack a global in-flight concurrency safeguard, allowing concurrent unauthenticated pushes to consume excessive memory. Update the git_write_routes middleware stack around add_auth_layers to apply a global concurrency limit (for example, tower’s GlobalConcurrencyLimitLayer) to the entire git write group, while preserving the existing body limit, pack_limit, and per-IP rate-limiting behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/gitlawb-node/src/server.rs`:
- Around line 225-237: The git write routes lack a global in-flight concurrency
safeguard, allowing concurrent unauthenticated pushes to consume excessive
memory. Update the git_write_routes middleware stack around add_auth_layers to
apply a global concurrency limit (for example, tower’s
GlobalConcurrencyLimitLayer) to the entire git write group, while preserving the
existing body limit, pack_limit, and per-IP rate-limiting behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 640e8a1a-6854-45da-888c-605aa10cb57f
📒 Files selected for processing (3)
crates/gitlawb-node/src/auth/mod.rscrates/gitlawb-node/src/server.rscrates/gitlawb-node/src/test_support.rs
beardthelion
added
crate:node
jatmn
requested changes
Jul 28, 2026
jatmn
left a comment
jatmn
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for the contribution. I do not see any actionable issues from my review.
Merge and checks
- Mergeability:
MERGEABLE; GitHub merge state isBLOCKED(policy/review gate, not a conflict). - Required PR checks on head
e8393fdc8008da2a07dd81da73b00d8f7616e2ba: fmt + clippy, test (stable), test (beta), build --release, cargo audit, MSRV, Docker smoke, and the rest of the PR Checks workflow are green.Analyze (swift)was still pending on the CodeQL workflow at review time; it is unrelated to this Rust-only diff.
What I verified
- The core fix is sound: per-group
SignatureBodyLimitwired throughadd_auth_layersandoptional_signature,http_body_util::Limitedon pre-authcollect(), and413 body_too_largefor over-limit streams close the unbounded pre-auth drain on both git and non-git signed surfaces. - Production wiring matches the stated intent: non-git signed groups use
SIGNED_BODY_LIMIT(2_097_152, matching axum’s default), git write/read groups usepack_limit. - All eight new body-limit tests passed locally, including the real receive-pack e2e fixtures.
cargo clippy -p gitlawb-node -- -D warningsis clean.
Out of scope / follow-up (not blockers)
- Residual memory still scales with concurrent connections × per-route limit; read timeout and global in-flight cap are appropriately deferred.
- Header-before-buffer fast reject for incomplete signature attempts (one header present, or missing headers on
require_signatureroutes) is real hardening in the same area but pre-existing ordering this PR does not worsen; worth a separate follow-up if desired. - Overlap with #261 (replay ledger) will need merge coordination but does not conflict with this limit wiring.
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
2 participants
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.
require_signaturebuffers the whole request body with an unlimitedbody.collect()before it parses a single header, and nothing upstream caps it.On the git write routes the pack-size
RequestBodyLimitLayer(2 GB by default, 500 MB in production) is applied to the inner router whileadd_auth_layerswraps it. A later.layer()is the outer service, so the middleware runs first and sees the raw body; the limit never applies to it. The other signed groups carry no limit layer at all and rely on axum's 2 MBDefaultBodyLimit, which the extractors enforce through a request extension this middleware never consults, so that default is bypassed as well.Measured against the real router, with
GITLAWB_MAX_PACK_BYTESset to 64 KiB and a 4 MiB body on an unsigned push:The full body is read, then the request is rejected for having no signature. No DID, no signature and no repo are needed to get there. In front of it sits only a per-IP arrivals-per-hour counter, and there is no timeout layer and no connection cap on
axum::serve.The reach is wider than the git routes:
optional_signatureforwards into the same code whenever a caller merely presents signature headers, so/graphql,/ipfs/{cid}and the other optional-signature groups have it too. Fifteen route groups in total.The fix
The collect now runs through
http_body_util::Limited, so reading stops at the limit rather than after the fact, and an over-limit request gets 413 instead of the 400 reserved for a genuinely unreadable body.The limit is per route group.
add_auth_layerstakes it and hands it to the middleware as state; the git groups keepmax_pack_bytesso large pushes are unaffected, and the rest get a constant equal to axum's default, which is already what their extractors enforce. That makes the change behavior-preserving on the response path and purely a memory fix. Signature verification, the content-digest check, clock skew, and the request reconstruction are untouched.Verification
The regression test asserts bytes actually polled out of a streaming body rather than the status code, because a status assertion passes whether or not the body was drained.
There is also an end-to-end pair driving a real push: a source repo with a 300 KiB blob and the byte-for-byte receive-pack body captured off the wire from an actual
git push. Under the limit it asserts the ref moved and the tree resolves, not merely a 200. That distinction is load-bearing, since a truncated body still returns 200 with the failure inside report-status; only the on-disk assertion catches it. Running the whole suite under a truncation mutation leaves exactly one test failing. Over the limit it asserts 413 with neither the ref nor the commit present, so a rejected push cannot half-write.Each guard was checked by reverting it: neutralizing the limit turns the drain tests red, handing every group the git limit turns the non-git test red, and returning 400 instead of 413 turns the status test red.
518 tests pass, fmt and clippy are clean.
Residual
This bounds per-request buffering only. It does not bound how many such requests are in flight, or how long each may take to send its body, so total resident memory is still proportional to concurrent connections times the limit.
To be precise about what does exist, since the first version of this paragraph was wrong: the Fly configs do cap connections (
hard_limit = 500), and they do setidle_timeout = 120. Neither helps. The connection cap is sized above what a 1024 MB machine can hold, and a body dripping a byte every few seconds is never idle. There is no request-duration or body-read timeout in process, andrate_limit_by_ipcounts arrivals per window rather than bounding concurrency.Filed as #263 with the measurements.
Summary by CodeRabbit
New Features
Bug Fixes
413 Payload Too Largeresponse.