fix(node): make a verified signature single-use on mutation routes (#253 phase 1) - #261
Open
beardthelion wants to merge 19 commits into
Open
fix(node): make a verified signature single-use on mutation routes (#253 phase 1)#261beardthelion wants to merge 19 commits into
beardthelion wants to merge 19 commits into
Conversation
added 19 commits
July 27, 2026 08:13
Five tests mounted under the production require_signature middleware, so the signature is really verified rather than injected. Three assert the fixed behavior and are #[ignore]d, since no replay defense exists yet: a captured signature must be single-use, whitespace-padded variants of it must be rejected as replays too, and a future-dated `created` must not verify. Each was confirmed red with the attribute removed. Whoever fixes #253 drops the attributes. Two pin properties the proposed fix depends on and are green now. The 300s backward skew budget must not be narrowed when the forward bound is tightened, because the body is fully buffered before check_created runs and a large pack spends that long uploading. And an unknown Signature-Input parameter must keep verifying, which is what makes emitting a `nonce` a one-directional rollout rather than a flag day. Both were confirmed load-bearing by mutation: narrowing the window to 30s reddens the first, rejecting unknown params reddens the second.
…D_COMPONENTS sign_request held the covered-component list twice: once as a string literal that became the wire Signature-Input, and once as the COVERED_COMPONENTS const passed to build_signing_string. Nothing kept them in sync, so adding a component to the const would make every client sign over four components while advertising three, and the mismatch surfaced as a panic at the .expect() rather than an error. Both now derive from the const. Two guards cover it: one asserts the emitted header parses back to exactly COVERED_COMPONENTS, the other asserts every covered component has a request-derived value. Verified load-bearing by adding @authority to the const, which turns the second guard red with a message naming the component, where three unrelated tests instead panic inside sign_request. The .expect() remains but is now unreachable while the second guard holds; a genuine removal needs sign_request to return Result, which changes eight call sites and belongs with the @authority work. Groundwork for #253. No wire-format change.
check_created compared (now - created).abs() against a single 300s bound, so a future-dated `created` was accepted just as readily as a past one. A signer could pre-date by 300s and hold a signature valid across a ~600s span, doubling the window a captured signature is useful in. Split into two bounds: reject when created is more than MAX_FUTURE_SKEW_SECS (30s) ahead, and separately when it is more than MAX_SIGNATURE_AGE_SECS (300s) old. Both are named constants now, with the backward one carrying the reason it must not shrink: the verifier buffers the whole request body before this check runs, so a large pack spends its upload time inside that budget. Observed RED before implementing (future_dated_created_is_rejected failed on a +250s signature) and GREEN after. The backward budget's guard was verified load-bearing by narrowing the constant to 30s, which turns backward_skew_budget_is_not_narrowed red on its 280s case.
sign_request now draws 128 bits from OsRng and appends ;nonce="..." to the Signature-Input parameter tail. The tail is what @signature-params is built from, so the nonce is covered by the signature without touching COVERED_COMPONENTS, and a verifier that predates it keeps verifying because it rebuilds @signature-params from the received header text. This gives the spent-signature ledger a short fixed-width key and makes two otherwise byte-identical mutations distinguishable. HttpSignature::parse now exposes the nonce, absent on a pre-nonce signer. All eight production sign_request call sites inherit it with no edits.
Migration v12 adds consumed_signatures, keyed on a fixed-width hex SHA-256 digest of the canonical signature key rather than any raw attacker-supplied value, with a CHECK enforcing the width at the schema level. consume_signature decides in one statement: a CTE counts the identity's live rows and the INSERT ... ON CONFLICT DO NOTHING arbitrates. Two concurrent replays of the same signature cannot both win, which a SELECT-then-INSERT would not give. The three outcomes are distinct so a caller can answer a replay and a full identity ledger with different codes. Row count is capped per keyid at 512 live rows. Hashing the key bounds bytes per row; only the cap bounds row count, and the routes this protects carry no rate limiter while identities are permissionless. The sweep joins the existing 300s cleanup loop rather than adding a task. Nothing is wired into middleware yet.
…#253) require_signature now inserts a SignatureIdentity extension alongside AuthenticatedDid, carrying the keyid, the optional nonce, and a hex SHA-256 of the signing string it just verified against. Hashing the reconstruction rather than the Signature header is the point. HttpSignature::parse trims and the header is not a covered component, so whitespace variants are distinct header bytes that rebuild to one signing string. A header-keyed ledger would let a single leading space defeat it while the signature still verifies. The signing string also embeds keyid and created through @signature-params, so the key cannot collide across DIDs. No status codes and no control flow change; nothing consumes the extension yet.
consume_signature charges every verified signature against the v12 ledger and rejects the second use with 409 signature_replayed, carrying the code in an X-Gitlawb-Error header as well as the body because send_signed hands the response back untouched and reading the JSON consumes it by value. The key is (keyid, nonce) when the signer sent one and the signing-string hash otherwise, domain-separated so the two schemes cannot collide. The fallback is what makes whitespace padding of the Signature header useless: those variants are distinct header bytes that reconstruct to one signing string. Ordering is load-bearing. Axum runs the last layer first, so the ledger is listed first and runs last: a request with a good signature but a rejected UCAN must not burn its key on the way to being denied. A regression test drives the real build_router and goes red if the two layers are swapped, which the suite previously did not catch. Reads are skipped by method. write_routes chains PUT/DELETE/GET on the visibility path and gl drives the GET through get_signed, so ledgering it would put a write on the signed read path. Replaying a read spends no side effect. Failure is closed in both directions: a missing SignatureIdentity is a 500, not a pass-through, so a wrong layer order cannot silently delete the defense, and a ledger error is a 503. A full per-identity ledger is a retryable 429, distinct from a replay. GraphQL mutations sit behind optional_signature and are out of scope; a captured signature over one stays replayable.
Defaults to false. When on, a signature with no nonce is rejected on write routes with 400 signature_nonce_required, which closes the signing-string fallback once every client emits a nonce. Enforcement lives in consume_signature, not require_signature. The latter also backs optional_signature, so enforcing there would reject nonce-less signatures on authenticated reads and /graphql for un-upgraded clients, the same coupling the ledger was kept out of that layer to avoid. The check sits after the GET/HEAD skip so it never reaches a read, and before the ledger is charged so a refused request spends nothing. clap parses the env var as a bool rather than testing for presence, so setting it to 0 or empty refuses to start instead of silently enforcing. A subprocess test drives all four values. Documented in README and .env.example, including the federation precondition: three of the node's own outbound signed calls are peer traffic, so with this and GITLAWB_REQUIRE_SIGNED_PEER_WRITES both on, peers still running a pre-nonce binary can no longer write.
…ing it (#253) send_signed now recognises the ledger rejections from status plus x-gitlawb-error before anything reads the body, and returns an error. None of them is retried. A signature_replayed 409 means the node already consumed that signature, so the first delivery reached the handler and its side effect happened; re-signing and resending would apply the mutation a second time, which is the duplicate write the ledger exists to prevent. The iCaptcha 403 loop is safe to retry only because that request is rejected before the handler runs, and it is untouched here. The 429 and 503 cases did not apply the write, but retrying them automatically would hammer a node already saying not now. A bare 409 still comes back as Ok, because init, repo and mirror inspect repo_exists themselves. The MCP module needed more than the ledger codes. Thirty-four call sites did resp.json().await? with no status inspection, so any denial body deserialized cleanly into Value and returned to the caller as a successful tool result. git_refs was worse: it reads pkt-lines, so a denial parsed to an empty ref list and looked like a repository with no refs. All of them now go through one json_ok helper that bails on a non-2xx with the node's own message.
… collision (#253) Version 12 was not free. The in-flight IPFS CID branch already numbers four migrations 11 through 14, and it sits behind main, so once it rebases past main's v11 they shift to 12 through 15. This migration moves to 16, above that whole range. The collision mattered because the runner only asked whether a version was applied, never under which name, so whichever branch merged second had its migration silently skipped: no error, no warning, and schema_migrations still reading healthy while the table it needed was missing. It now compares the recorded name and aborts startup naming both sides and the remedy, which turns an undetectable skip into a boot error for this collision and every future one. Safe to fail closed: no migration version has ever been renamed across the file's 28 revisions, so no existing database can trip the new check. The upgrade-path test derives its baseline from the catalogue rather than hardcoding a version, so it stays a true upgrade test when those four migrations land here.
…aims (#253) The ledger charges a durable row before the handler runs any authorization or existence check, and require_signature resolves the key from the did:key itself with no lookup and no registration. So any freshly minted keypair could force that write on a repo it does not own or that does not exist: a POST to a nonexistent repo's hooks verifies, charges a row, then 404s. Measured at ~428 bytes on disk and ~983 bytes of WAL per request, on five route groups that carried no limiter at all. GITLAWB_SIGNED_WRITE_RATE_LIMIT (default 600/IP/hour, 0 disables) puts a per-IP brake on its own bucket in front of those five, layered outside add_auth_layers so it runs before signature verification burns CPU and before the ledger is charged. Charging before the handler stays correct; it was the missing outer bound that was the problem. Three claims corrected to match the code: The README said a signature is spent once on every write route. It is not: announce and sync/notify only reach the ledger when GITLAWB_REQUIRE_SIGNED_PEER_WRITES is on, and the default routes them through optional_signature, which verifies but never spends. The add_auth_layers comment said consuming last avoids penalizing a request that was never going to run. It does not: anything layered inside the router runs after the ledger, so a request refused by the per-DID throttle, by iCaptcha, or by the handler has already spent its signature. The whitespace replay test claimed to prove a header-keyed ledger is defeated by one space, but signed via sign_request, which always emits a nonce, so it took the nonce arm and never touched the header at all. It now signs nonce-less and drives the fallback it describes.
…#253) send_signed now converts all five ledger denials, keyed on the error code rather than the status, and gl task uses the status-then-parse shape its siblings use. Before this, 400 signature_nonce_required and 500 signature_identity_missing came back as Ok and gl task printed the error body as a successful result with exit 0, so a script keying on the exit code treated the task as created. The node message is now capped and sanitized before it reaches a terminal. Interpolating it raw let a hostile node clear the screen and print its own success line inside the message that exists to say the write was refused. This reuses read_body_capped and sanitize_node_msg, promoted out of sync.rs rather than copied a third time. gl init compared the node's prose instead of its error code: it continued on any message containing "already", and the replay message reads "this signature has already been used". A 409 replay therefore printed "Repository already exists, continuing" and told the user to push to a repo that was never created. It now compares error == repo_exists, which is the only code the node renders for that case. The registration path had the same tolerance for nothing: the node returns 201 unconditionally and register_agent upserts, so there is no already registered reply to swallow. Two comments were wrong. iCaptcha is checked inside the handler, after the ledger charge, so the retry is safe because send_once re-signs with a fresh nonce, not because the request was rejected early; the test now asserts the two attempts carry different Signature-Input values. And a spent signature means the node admitted the request, not that it applied it, since the ledger is charged before the handler runs. Known limit: denial detection reads the X-Gitlawb-Error header only. A proxy that strips it returns the denial to the caller as Ok. Reading the body instead would mean rebuilding the response, which needs a new manifest dependency this branch should not carry.
…anicking
parse found '(' and ')' independently with no ordering check, so a header whose
')' came first sliced backwards and panicked. Signature-Input: sig1=)( was
enough, with no keypair and no registration, and optional_signature dispatches
into require_signature whenever a signature-input header is present, so a plain
GET on a read route reached it too. The process survives and drops the
connection, but it is an unauthenticated panic in the auth path.
Pre-existing rather than introduced here; fixed because this branch is already
in the file.
Audited the rest of parse for the same class while there: the open-ended slice
after ')' cannot invert, and the prefix strips, base64 decode, created parse and
DID parse all return errors rather than indexing. The reversed slice was the
only reachable panic. A table test covers seventeen malformed inputs and asserts
none of them panics.
… the TTL (#253) Three ways the ledger's identity and uniqueness assumptions were weaker than their comments claimed. An empty nonce satisfied GITLAWB_REQUIRE_SIGNATURE_NONCE, because parse turns nonce="" into Some("") and the check only asked whether it was None. It also made the nonce arm of ledger_key constant per identity, so that client got one accepted mutation per window and replay rejections thereafter. Both the flag and the key now go through one unique_nonce helper so they cannot disagree: with the flag on, a short nonce is refused with its own code, and with the flag off it falls back to the signing-string hash, which is unique by construction. The per-identity cap counted the DID exactly as it appeared on the wire, but did:key resolves through multibase, so one keypair has many valid spellings that all resolve to the same key. Counting by string gave each spelling its own 512 row budget. The cap now counts the resolved public key. Single-use never depended on this, since a replay must reproduce the signed bytes, and a test pins that both ways. The TTL was flush with the acceptance window, which is right on one node and wrong across several: the sweep uses the clock of whichever instance runs it, so an instance running ahead deleted rows that another still accepted. The TTL is now derived from the two skew bounds plus an explicit margin, so widening either bound carries it along. Also corrected the ledger's sizing comments against measurement (~430 bytes per row including indexes, not ~100, and on-disk peak is roughly double the cap because the sweep tick is independent of the TTL), noted that a wrong-width sig_hash would surface as a phantom 503, and tightened a must-not test that asserted only "not 201" to assert the exact status and error code.
…refused announce (#253) A multi-ref push fans out one signed notify per ref, all under the node's own key, so a 600-branch mirror push charges a peer's ledger 600 times and trips the 512-row cap. Refs past it were warn-logged once each and never federated: no retry, no queue, and nothing an operator could correlate. The per-IP peer brake does not catch it first, being 600 per hour against a burst of seconds. Only 429 and 503 are retried, matching how gl classifies the same rejections: those mean the write did not happen. A 409 replay is never retried, because it means the peer already admitted that request, and a transport error is not retried either, since the request may have arrived with only the response lost. Retrying is safe against the ledger because each attempt re-signs with a fresh nonce, so it lands under a key the peer has not seen. The retry budget is shared across the whole fan-out rather than per ref. A per-ref bound multiplies by the ref count, which would keep a detached background task alive for hours against a wedged peer. Refs that still fail now produce one summary line naming the count and the reason instead of a warning per ref. Batching the fan-out into a single request remains the structural fix and is a wire-format change this branch does not take. Bootstrap peer announce had no arm for a non-2xx at all, so a node whose announces were being rejected looked exactly like one succeeding. It now logs the status and the error code, matching its two sibling outbound calls. The status handling moved into a pure classifier so the decision is unit-tested rather than asserted against a log line.
The ledger fails closed, so a fault confined to the new table returns 503 on
every REST mutation while the handlers' own queries stay healthy. Its only
output was tracing lines, which gave an operator no numerator and no
denominator: a node rejecting every write looked like a healthy one. This is the
condition attached to shipping the ledger enforcing rather than behind a shadow
flag.
gitlawb_signature_ledger_total{outcome} covers all eight terminal exits of
consume_signature, so the series sum is the traffic the layer saw and each
outcome's share is readable against it. The label takes one of eight literals
fixed at the call sites, so cardinality is 8 and nothing keyed on a DID, key
fingerprint, path or nonce can reach it. Labelling by identity here would be a
second amplification vector on the same routes this branch just rate-limited.
Deliberately a new counter rather than the existing auth ones. A replay is not
an auth failure: the signature verified and the DID resolved. Folding it in
would leave gitlawb_auth_failures_total meaning "invalid, or valid but already
spent, or our own database is down", and the 503 arm says nothing about the
caller's credentials at all.
Also fixes a race in metrics::init, which guarded on REGISTRY being set but
published REGISTRY last, so two concurrent callers both passed the guard and the
second panicked on the INFO expect. A std::sync::Once now gives it the
semantics those expects already assumed. Production calls init once, so this
only ever bit concurrent tests.
The node emits six ledger denial codes and gl recognised five. A signed write refused with signature_nonce_too_short came back as Ok, so the caller had to work out for itself that nothing had been written. The code was added to the node after the client's list was written, and the two halves agreed only by having been typed the same way twice in two crates. SignatureDenial in gitlawb-core is now the single source of truth. The node builds ledger_rejection from it, so the wire strings and statuses come from one place, and gl matches it with no wildcard arm. Adding a variant is a compile error in the client until the client handles it: verified by adding a seventh variant and watching cargo build -p gl fail with E0004. Deliberately not non_exhaustive. That attribute would force a wildcard arm downstream, which is the exact silent fallthrough the type exists to prevent. The guarantee is over the codes this repo's node emits; from_code still returns Option, because a client talks to nodes it does not control and an unknown code must stay unrecognised at runtime rather than being guessed at. Wire format is unchanged, and pinned: one test asserts every code and status against a literal table so renaming a variant cannot quietly change the protocol, and another asserts the node still emits each code in both the header and the body.
…des (#253) Two defects in the collision guard this branch added. It could not see the row that will trip it. The loop iterates the versions the current build defines, so a recorded version the catalogue no longer mentions is invisible, which is exactly what this branch's own renumber from v12 to v16 left behind. A rollback recreates it: an older binary finds no v12, re-runs a CREATE TABLE IF NOT EXISTS as a no-op, and records v12 again. The orphan then sits unseen until another branch claims that version and the node bails at boot. The check now asks the database what it has recorded rather than iterating what the build knows, and warns on anything the catalogue does not define. Warn, not fail: making it fatal would break a rollback, which is what an operator reaches for when the new build is already broken. The bail message also sent the reader to the wrong fix. It said to renumber, which is right for a real collision and wrong for an orphan, where the remedy is to drop the stale row. It now names both cases. And the collision was reported as a transient outage. is_likely_permanent_db_error only downcast to sqlx::Error, so a code-level numbering bug was logged as "database unavailable during startup; retrying" forever while /ready said the database was initializing. It is now a distinct error type, classified as permanent, latched, and given its own readiness code. Classification and latch are one function returning both bits, so the retry loop cannot get the backoff right while leaving the readiness payload lying: the two values have no other source.
…253) The per-IP brake this branch added returns 429 on the same five route groups that already return 429 signature_ledger_full from the ledger. The brake sent plain text with no X-Gitlawb-Error, so two unrelated conditions shared one status and the only discriminator was the absence of a header, which is exactly the signal a proxy may strip. Three things broke on that. gl printed "invalid JSON response" for a rate limit, because several commands parse the body before checking the status and the brake's body is not JSON. Those routes had no brake before this branch, so this branch made it reachable. A survey found the same shape at 14 more sites; all are converted, each keeping its existing message prefix. One is left: init's create-repo path needs the parsed body on its tolerated-failure branch, noted for follow-up. The federation retry treated every 429 and 503 as retryable, including a peer's own per-IP brake, which it can never clear inside a 60s budget against a 3600s window. On a peer running the default config /sync/notify never reaches the ledger at all, so every 429 the sender could see was the brake: the whole retry path spent on the one class it cannot recover from, and the retries push the sender past the peer's 600/hour bucket, which is shared with announce. It now gates on the code, so a 429 or 503 with any other code, or none, is fatal. 409 and transport errors stay never-retried. rate_limited is not a new wire string: AppError::TooManyRequests already emitted it. Both it and the brake now take it from the shared enum, so they cannot drift, and the enum is renamed from SignatureDenial to NodeDenial because a rate limit is not a signature denial and the type's real subject is the X-Gitlawb-Error vocabulary. Every existing code and status is unchanged, pinned by the existing test. Also documents what the ledger counter does not cover: the brake rejects upstream of the layer, so its 429s appear in no series.
Contributor
📝 WalkthroughWalkthroughThis change adds nonce-backed HTTP signatures, durable single-use signature enforcement, standardized denial codes, signed-write rate limiting, bounded peer retries, schema-conflict reporting, and denial-aware handling across CLI and MCP clients. ChangesReplay protection and denial handling
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Collaborator
Author
|
@jatmn if you have time for this one, the parts most worth your attention are the three where a mistake would be invisible rather than loud. Layer ordering. The ledger is listed first in The GET and HEAD skip in The two fail-closed paths. A missing verified identity is a 500 and a ledger error is a 503. Both exist so the defense cannot silently disappear: if either one passed the request through instead, the guard would vanish while every test stayed green. Worth trying to drive them to pass through. More generally, the useful attack surface here is a permissionless one. Any caller can mint a fresh On the scope limits in the description (GraphQL mutations still replayable, cross-node replay still open, peer routes unledgered in the default config): those are decisions rather than oversights, and each has its reasoning in the body, so please do not feel you need to re-argue them. That said, if while you are in there you find that one of them is worse than I have assumed, or reachable in a way I have not described, I would genuinely like to know. Filed as #257 and the open half of #253 if it is easier to comment there. Happy to walk through any of it if something reads oddly. |
beardthelion
added
crate:core
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/gl/src/init.rs (1)
148-160: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCreate-repo is the one write left parsing the body before the status.
The branch reads
repo_resultfrom the unconditionalresp.json()above it, so a non-JSON denial on this route (proxy 502, text/plain 429) fails withinvalid JSON from create repoand never reachesrepo_already_exists— the exact mismatchjson_or_denialwas introduced to remove everywhere else in this PR. Still an error, just the wrong one.Reading the body under a cap keeps the
repo_existstolerance intact:♻️ Route the denial through the shared helper, keeping the tolerance
- let repo_status = resp.status(); - let repo_result: Value = resp.json().await.context("invalid JSON from create repo")?; - - if !repo_status.is_success() { + let repo_status = resp.status(); + if !repo_status.is_success() { + let raw = crate::http::read_body_capped(&mut resp, 64 * 1024).await; + let repo_result: Value = serde_json::from_slice(&raw).unwrap_or(Value::Null); // Key on the node's structured code, never on its prose: the replay // denial's message is "this signature has already been used - sign a // fresh request", which a `contains("already")` check read as "the repo // is already there, carry on" and reported success for a repo that was // never created. if !repo_already_exists(&repo_result) { let msg = repo_result["message"].as_str().unwrap_or("unknown error"); anyhow::bail!( "create repo failed ({repo_status}): {}", crate::http::sanitize_node_msg(msg) ); } println!(" Repository already exists — continuing."); } else { + let _repo_result: Value = resp.json().await.context("invalid JSON from create repo")?; println!(" Repository created."); }Requires binding the response as
let mut resp = client.post(...).🤖 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/gl/src/init.rs` around lines 148 - 160, Update the create-repository flow around repo_result to bind the HTTP response as mutable and route its body through the shared json_or_denial helper before checking repo_already_exists. Preserve the existing tolerance for repo_exists responses and continue sanitizing unexpected error messages, while enforcing the helper’s capped body handling for non-JSON denials.
🤖 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.
Inline comments:
In `@crates/gitlawb-node/src/api/repos.rs`:
- Around line 816-843: Split the documentation currently preceding
NotifyAttempt: keep only the final result-summary sentence as the enum’s
rustdoc, and move the classification-rule explanation onto the
classify_notify_status function that implements the retry gate. Preserve the
existing documentation content while attaching each part to its intended symbol.
In `@crates/gl/src/mcp.rs`:
- Around line 1303-1317: The MCP response handling must use the shared capped
and sanitized denial path. Update json_ok to read responses through
read_body_capped, parse successful JSON from the bounded body, and sanitize
node-derived message/error text with sanitize_node_msg before returning errors;
update git_refs at crates/gl/src/mcp.rs:805-811 to avoid buffering resp.bytes()
before rejecting non-2xx and use the same helpers. Preserve existing success
behavior while ensuring both paths enforce the response-size cap and remove
terminal control/bidi characters.
---
Nitpick comments:
In `@crates/gl/src/init.rs`:
- Around line 148-160: Update the create-repository flow around repo_result to
bind the HTTP response as mutable and route its body through the shared
json_or_denial helper before checking repo_already_exists. Preserve the existing
tolerance for repo_exists responses and continue sanitizing unexpected error
messages, while enforcing the helper’s capped body handling for non-JSON
denials.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fc24c30a-418b-4378-b82e-d94c2aba1518
📒 Files selected for processing (29)
.env.exampleREADME.mdcrates/gitlawb-core/src/http_sig.rscrates/gitlawb-core/src/lib.rscrates/gitlawb-core/src/node_denial.rscrates/gitlawb-node/src/api/peers.rscrates/gitlawb-node/src/api/repos.rscrates/gitlawb-node/src/auth/mod.rscrates/gitlawb-node/src/config.rscrates/gitlawb-node/src/db/mod.rscrates/gitlawb-node/src/error.rscrates/gitlawb-node/src/main.rscrates/gitlawb-node/src/metrics.rscrates/gitlawb-node/src/rate_limit.rscrates/gitlawb-node/src/server.rscrates/gitlawb-node/src/state.rscrates/gitlawb-node/src/test_support.rscrates/gl/src/http.rscrates/gl/src/init.rscrates/gl/src/issue.rscrates/gl/src/mcp.rscrates/gl/src/peer.rscrates/gl/src/pr.rscrates/gl/src/profile.rscrates/gl/src/register.rscrates/gl/src/repo.rscrates/gl/src/sync.rscrates/gl/src/task.rscrates/gl/src/webhook.rs
Comment on lines
+816
to
+843
| /// Classify a peer's `/sync/notify` response. | ||
| /// | ||
| /// The gate is the peer's `X-Gitlawb-Error` code, not the status. Only two | ||
| /// codes mean "the write did not happen and another attempt can clear it": | ||
| /// `signature_ledger_full` (the per-identity spent-signature cap, which drains | ||
| /// as entries expire) and `signature_ledger_unavailable`. Everything else is | ||
| /// `Fatal`, including a 429 or 503 carrying any other code and one carrying no | ||
| /// code at all. | ||
| /// | ||
| /// Status alone is not enough because the peer's per-client flood brake answers | ||
| /// 429 on this route too, and on a peer running the default config (where | ||
| /// `require_signed_peer_writes` is off) `/sync/notify` never reaches the ledger | ||
| /// at all — so every 429 the sender can see is the brake. Retrying that spends | ||
| /// the whole budget on the one class it can never recover from (the brake's | ||
| /// window is an hour, the budget is a minute) while pushing the sender further | ||
| /// into the peer's peer-write bucket, which `/peers/announce` shares. | ||
| /// | ||
| /// The status still has to agree with the code, so a stray `signature_ledger_full` | ||
| /// header on a 409 cannot turn "the peer already applied this" into a retry. | ||
| /// The result of one `/sync/notify` attempt, with the reason kept for the | ||
| /// end-of-fan-out summary. | ||
| #[must_use] | ||
| enum NotifyAttempt { | ||
| Delivered, | ||
| Retryable(String), | ||
| Fatal(String), | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Two doc comments were merged onto NotifyAttempt, leaving classify_notify_status undocumented.
Lines 816-834 document the classification rule (status-vs-code gating), but they are attached to the NotifyAttempt enum; the intended one-liner for NotifyAttempt is the last sentence at Lines 835-836. Rustdoc for the enum now describes a function, and the retry gate's rationale is not on the function that implements it.
📝 Proposed fix: split the two doc blocks
-/// Classify a peer's `/sync/notify` response.
-///
-/// The gate is the peer's `X-Gitlawb-Error` code, not the status. Only two
-/// codes mean "the write did not happen and another attempt can clear it":
-/// `signature_ledger_full` (the per-identity spent-signature cap, which drains
-/// as entries expire) and `signature_ledger_unavailable`. Everything else is
-/// `Fatal`, including a 429 or 503 carrying any other code and one carrying no
-/// code at all.
-///
-/// Status alone is not enough because the peer's per-client flood brake answers
-/// 429 on this route too, and on a peer running the default config (where
-/// `require_signed_peer_writes` is off) `/sync/notify` never reaches the ledger
-/// at all — so every 429 the sender can see is the brake. Retrying that spends
-/// the whole budget on the one class it can never recover from (the brake's
-/// window is an hour, the budget is a minute) while pushing the sender further
-/// into the peer's peer-write bucket, which `/peers/announce` shares.
-///
-/// The status still has to agree with the code, so a stray `signature_ledger_full`
-/// header on a 409 cannot turn "the peer already applied this" into a retry.
-/// The result of one `/sync/notify` attempt, with the reason kept for the
+/// The result of one `/sync/notify` attempt, with the reason kept for the
/// end-of-fan-out summary.
#[must_use]
enum NotifyAttempt {
Delivered,
Retryable(String),
Fatal(String),
}
+/// Classify a peer's `/sync/notify` response.
+///
+/// The gate is the peer's `X-Gitlawb-Error` code, not the status. Only two
+/// codes mean "the write did not happen and another attempt can clear it":
+/// `signature_ledger_full` (the per-identity spent-signature cap, which drains
+/// as entries expire) and `signature_ledger_unavailable`. Everything else is
+/// `Fatal`, including a 429 or 503 carrying any other code and one carrying no
+/// code at all.
+///
+/// Status alone is not enough because the peer's per-client flood brake answers
+/// 429 on this route too, and on a peer running the default config (where
+/// `require_signed_peer_writes` is off) `/sync/notify` never reaches the ledger
+/// at all — so every 429 the sender can see is the brake.
+///
+/// The status still has to agree with the code, so a stray `signature_ledger_full`
+/// header on a 409 cannot turn "the peer already applied this" into a retry.
fn classify_notify_status(status: reqwest::StatusCode, code: Option<&str>) -> NotifyDisposition {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// Classify a peer's `/sync/notify` response. | |
| /// | |
| /// The gate is the peer's `X-Gitlawb-Error` code, not the status. Only two | |
| /// codes mean "the write did not happen and another attempt can clear it": | |
| /// `signature_ledger_full` (the per-identity spent-signature cap, which drains | |
| /// as entries expire) and `signature_ledger_unavailable`. Everything else is | |
| /// `Fatal`, including a 429 or 503 carrying any other code and one carrying no | |
| /// code at all. | |
| /// | |
| /// Status alone is not enough because the peer's per-client flood brake answers | |
| /// 429 on this route too, and on a peer running the default config (where | |
| /// `require_signed_peer_writes` is off) `/sync/notify` never reaches the ledger | |
| /// at all — so every 429 the sender can see is the brake. Retrying that spends | |
| /// the whole budget on the one class it can never recover from (the brake's | |
| /// window is an hour, the budget is a minute) while pushing the sender further | |
| /// into the peer's peer-write bucket, which `/peers/announce` shares. | |
| /// | |
| /// The status still has to agree with the code, so a stray `signature_ledger_full` | |
| /// header on a 409 cannot turn "the peer already applied this" into a retry. | |
| /// The result of one `/sync/notify` attempt, with the reason kept for the | |
| /// end-of-fan-out summary. | |
| #[must_use] | |
| enum NotifyAttempt { | |
| Delivered, | |
| Retryable(String), | |
| Fatal(String), | |
| } | |
| /// The result of one `/sync/notify` attempt, with the reason kept for the | |
| /// end-of-fan-out summary. | |
| #[must_use] | |
| enum NotifyAttempt { | |
| Delivered, | |
| Retryable(String), | |
| Fatal(String), | |
| } | |
| /// Classify a peer's `/sync/notify` response. | |
| /// | |
| /// The gate is the peer's `X-Gitlawb-Error` code, not the status. Only two | |
| /// codes mean "the write did not happen and another attempt can clear it": | |
| /// `signature_ledger_full` (the per-identity spent-signature cap, which drains | |
| /// as entries expire) and `signature_ledger_unavailable`. Everything else is | |
| /// `Fatal`, including a 429 or 503 carrying any other code and one carrying no | |
| /// code at all. | |
| /// | |
| /// Status alone is not enough because the peer's per-client flood brake answers | |
| /// 429 on this route too, and on a peer running the default config (where | |
| /// `require_signed_peer_writes` is off) `/sync/notify` never reaches the ledger | |
| /// at all — so every 429 the sender can see is the brake. | |
| /// | |
| /// The status still has to agree with the code, so a stray `signature_ledger_full` | |
| /// header on a 409 cannot turn "the peer already applied this" into a retry. | |
| fn classify_notify_status(status: reqwest::StatusCode, code: Option<&str>) -> NotifyDisposition { |
🤖 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/api/repos.rs` around lines 816 - 843, Split the
documentation currently preceding NotifyAttempt: keep only the final
result-summary sentence as the enum’s rustdoc, and move the classification-rule
explanation onto the classify_notify_status function that implements the retry
gate. Preserve the existing documentation content while attaching each part to
its intended symbol.
Comment on lines
+1303
to
+1317
| async fn json_ok(what: &str, resp: reqwest::Response) -> Result<Value> { | ||
| let status = resp.status(); | ||
| let body: Value = resp | ||
| .json() | ||
| .await | ||
| .with_context(|| format!("{what} failed ({status}): response was not JSON"))?; | ||
| if !status.is_success() { | ||
| let msg = body["message"] | ||
| .as_str() | ||
| .or_else(|| body["error"].as_str()) | ||
| .unwrap_or("unknown error"); | ||
| anyhow::bail!("{what} failed ({status}): {msg}"); | ||
| } | ||
| Ok(body) | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the shared helpers and their current visibility/signatures.
rg -nP -C3 --type=rust 'fn (read_body_capped|sanitize_node_msg)\s*\('
# Where are they already used, and is mcp.rs among them?
rg -nP --type=rust '\b(read_body_capped|sanitize_node_msg)\s*\(' crates/gl/srcRepository: Gitlawb/node
Length of output: 150
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search the whole repo for the helper names and nearby HTTP body handling.
rg -nP -C3 --type=rust 'fn (read_body_capped|sanitize_node_msg)\s*\(' .
printf '\n---\n'
rg -nP --type=rust '\b(read_body_capped|sanitize_node_msg|resp\.json\(\)|resp\.bytes\(\))\b' crates .Repository: Gitlawb/node
Length of output: 3990
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the MCP implementations around the reported lines.
sed -n '780,830p' crates/gl/src/mcp.rs
printf '\n---\n'
sed -n '1290,1325p' crates/gl/src/mcp.rs
printf '\n---\n'
# Show the shared helper implementations for direct comparison.
sed -n '230,255p' crates/gl/src/http.rs
printf '\n---\n'
sed -n '335,390p' crates/gl/src/http.rsRepository: Gitlawb/node
Length of output: 7397
Use the shared capped/sanitized denial path in MCP
json_ok still reads the whole response with resp.json() and bails with the node’s raw message/error, and git_refs still buffers resp.bytes() before rejecting non-2xx. Reuse read_body_capped + sanitize_node_msg here so a hostile node can’t force an unbounded read or smuggle terminal control/bidi characters into MCP errors.
📍 Affects 1 file
crates/gl/src/mcp.rs#L1303-L1317(this comment)crates/gl/src/mcp.rs#L805-L811
🤖 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/gl/src/mcp.rs` around lines 1303 - 1317, The MCP response handling
must use the shared capped and sanitized denial path. Update json_ok to read
responses through read_body_capped, parse successful JSON from the bounded body,
and sanitize node-derived message/error text with sanitize_node_msg before
returning errors; update git_refs at crates/gl/src/mcp.rs:805-811 to avoid
buffering resp.bytes() before rejecting non-2xx and use the same helpers.
Preserve existing success behavior while ensuring both paths enforce the
response-size cap and remove terminal control/bidi characters.
jatmn
requested changes
Jul 28, 2026
jatmn
left a comment
jatmn
left a comment
Collaborator
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P2] Route MCP error handling through the capped, sanitized denial helpers
crates/gl/src/mcp.rs:1303-1317
json_okstill callsresp.json()with no byte cap and interpolates the node's rawmessage/errorinto tool errors withoutsanitize_node_msg. Every other denial-aware path this PR adds incrates/gl/src/http.rs(read_body_capped,json_or_denial,send_signed→into_error) caps hostile bodies and strips terminal control/bidi characters before they reach a caller. MCP is explicitly in scope for this change — the PR addsjson_okto stop 4xx/5xx bodies from deserializing as successful tool results — but left this helper on the old pattern. A malicious or compromised node can force an unbounded allocation on a failed tool call and smuggle terminal control sequences into MCP-visible errors. -
[P2] Cap
git_refsdenial bodies before buffering
crates/gl/src/mcp.rs:805-811
git_refscallsresp.bytes().await?before checkingstatus, so a non-2xx response is fully buffered even when the call will fail. The status check itself is correct (the test atmcp::tests::test_git_refs_denial_errors_not_empty_refscovers that), but the read is uncapped. Please read throughread_body_cappedfor denials the same waysync.rsandjson_or_denialdo, and only pass the bounded body toparse_info_refson success. -
[P2] Emit
X-Gitlawb-Erroron the receive-packinfo/refsrate limit
crates/gitlawb-node/src/error.rs:137-167,crates/gitlawb-node/src/api/repos.rs:556-563
This PR makes flood-brake 429s machine-readable by givingrate_limit::too_many_requests()both a JSONerror: rate_limitedbody and anX-Gitlawb-Errorheader, and it changesAppError::TooManyRequeststo use the same wire code in the JSON body. ButAppError::IntoResponsestill omits the header.git_info_refsapplies the sharedpush_rate_limiterinline and returnsAppError::TooManyRequests; the followinggit-receive-packPOST hits the same bucket throughrate_limit_by_ip→too_many_requests(), which does carry the header.glclassifies denials from the header only, so the two steps of a push can return different 429 shapes from one limiter. Please route this path through the same response helper as the middleware brake, or add the header inAppError::TooManyRequests'sIntoResponse.
Follow-up (not blocking merge)
-
[P3] Serialize the per-identity ledger cap check under concurrency
crates/gitlawb-node/src/db/mod.rs:1530-1559
TheliveCOUNT + conditional INSERT can both pass when two distinct signatures from the same identity arrive in parallel atn == 511, so the 512-row ceiling can be exceeded by roughly the in-flight request count. Replay atomicity is fine (sig_hashPK); this is a separate TOCTOU on the cap. Worth hardening if you need a strict ceiling under burst concurrency, but the per-IP brake bounds how fast rows accumulate and overshoot is bounded by parallelism, not unbounded. -
[P3] Finish the
json_or_denialmigration on PR/issue comment writes
crates/gl/src/pr.rs:437-450,crates/gl/src/pr.rs:473-486,crates/gl/src/issue.rs:311-324
Sibling commands in the same files were migrated; these three were not.send_signedstill catches everyNodeDenialwith a header before they run, so this is polish for the proxy-strips-header / non-JSON-body cases you already document inhttp.rs, not a silent-success gap. -
[P3] Finish the
gl initcreate-repo status/body ordering
crates/gl/src/init.rs:145-148
Tracked in #260. Signature ledger denials are caught bysend_signed; the gap is non-JSON error bodies surfacing as parse failures. Fine to land here or in the follow-up you already filed. -
[P3] Move the retry-gate rustdoc onto
classify_notify_status
crates/gitlawb-node/src/api/repos.rs:816-843
Docs-only. CodeRabbit's split is right: keep the one-line result summary onNotifyAttemptand move the classification rationale onto the function.
Author hotspot check
I spent extra time on the three areas you called out to me:
Layer ordering — The add_auth_layers stack is correct for Axum's last-listed-runs-first semantics: require_signature → require_ucan_chain → consume_signature → handler. send_full_auth through the real build_router plus a_ucan_rejection_does_not_burn_the_signature would catch a UCAN/ledger swap. The ordering argument in server.rs matches the code.
GET/HEAD skip — The only GET behind add_auth_layers is visibility::list_visibility on the chained PUT/DELETE/GET route. That handler only reads and returns rules; HEAD is axum-derived from the same GET handler. I did not find a mutation reachable through GET or HEAD on a ledgered router.
Fail-closed paths — Missing SignatureIdentity returns 500 signature_identity_missing; ledger DB errors return 503 signature_ledger_unavailable. Neither path calls next.run(). Metrics tests cover both outcomes.
Permissionless cap — Cap counting on key_fingerprint (not wire DID spelling) looks correct for the stated model. A concurrent TOCTOU at the cap boundary is possible (see follow-up above) but is bounded by request parallelism and the per-IP signed-write brake, not a permissionless bypass of the cap scheme itself.
I did not find the documented scope limits (GraphQL mutation replay #257, cross-node replay, default-config unledgered peer routes) to be worse or more reachable than you described. Post-ledger signature spend on handler/per-DID/iCaptcha rejection is an explicit documented tradeoff, not a defect.
CodeRabbit
The two actionable inline comments map to the P2 MCP finding and the P3 doc split above. The init.rs nit is in follow-up and #260.
Merge gate
Mergeable, no conflicts. All required CI checks green on head 2f4995bae0d0cfaf052194371a0e3931a503ed1a. Coordinate migration v16 ordering with #173 before either PR lands — that is an operational sequencing note, not a code defect in this diff.
6 tasks
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.
Phase 1 of #253. Deliberately not "closes": #253 also covers host binding, and this PR does not do that, so it must not auto-close on merge. See Scope below.
A captured RFC 9421 signature is currently a bearer credential. The node verifies it and keeps no record that it did, so the same signed request can be delivered again and applied again. Five probes against
mainat 111cff7 confirmed it: one signature POSTed to/api/v1/tasksthree times produced three201s and three rows, the same signature was accepted under three differentHostheaders, acreated250s in the future was accepted, and four whitespace variants of theSignatureheader all verified.This makes a signature single-use on mutation routes and tightens the acceptance window.
What changed
sign_requestnow emits a 128-bitnoncein theSignature-Inputparameter tail. The tail is what@signature-paramsis built from, so the nonce is covered by the signature without touching the covered-component list, and a verifier that predates it keeps working because it rebuilds@signature-paramsfrom the received header text.check_createdis one-sided. It accepted(now - created).abs() > 300, which let a signer pre-datecreatedand hold a signature valid for twice the intended span. Forward skew is now 30s; the 300s backward budget is unchanged, because the verifier buffers the whole body before this check and a large pack spends its upload time inside that budget.A new migration adds
consumed_signatures, and aconsume_signaturelayer charges every verified signature against it. The key is(identity, nonce)when the signer sent one and the signing-string hash otherwise, domain-separated. The fallback is what makes whitespace padding useless: those variants are distinct header bytes that reconstruct to one signing string, so keying on the reconstruction collapses them to one entry. Keying on the header would not have.Layer order is load-bearing. Axum runs the last layer first, so the ledger is listed first and runs last: a request with a good signature but a rejected UCAN must not burn its key on the way to being denied. A test drives the real
build_routerand fails if the two layers are swapped.Reads are skipped by method.
write_routeschains PUT/DELETE/GET on the visibility path andgldrives that GET signed, so ledgering it would put a database write on the signed read path.Failure is closed in both directions: a missing verified identity is a 500 rather than a pass-through, so a wrong layer order cannot silently delete the defence, and a ledger error is a 503.
GITLAWB_REQUIRE_SIGNATURE_NONCE(default false) closes the hash fallback once every client emits a nonce. It is enforced in the ledger layer only, not inrequire_signature, which also backsoptional_signature: enforcing there would reject nonce-less signatures on authenticated reads and/graphqlfor un-upgraded clients.GITLAWB_SIGNED_WRITE_RATE_LIMIT(default 600/IP/hour,0disables) puts a per-IP brake in front of the five write groups that had none. Without it, any freshly minted keypair could force a durable ledger row on a repo it does not own or that does not exist: the request verifies, charges a row, then 404s. Measured at ~428 bytes on disk and ~983 bytes of WAL per request.glfails loudly on every denial instead of handing back a response that pretty-prints like success, and the node message is capped and sanitised before it reaches a terminal. The denial codes live in one enum ingitlawb-corethat the client matches exhaustively, so adding a code without handling it in the client is a compile error.Scope, stated plainly
GraphQL mutations are not covered (#257).
/graphqlcarriesoptional_signature, not the auth stack, andMutationRootexposescreate_task,claim_task,complete_taskandfail_task. A captured signature over a GraphQL mutation stays replayable after this PR. Closing it means either extending the ledger to the signed half of/graphqlor moving those mutations behind the auth stack, both larger than this change.Cross-node replay stays open, which is the other half of #253 and why this PR does not close it. Nothing binds a signature to a host, so one capture is still accepted once by each node. Binding
@authorityis a bidirectional flag day (an old client fails on a new node and a new client fails on an old node, because the verifier builds the signing string from the client's component list), so it is separate work. This PR closes single-node replay, not the issue.Peer routes are only covered when signed peer writes are on. With
GITLAWB_REQUIRE_SIGNED_PEER_WRITES=false, which is the default,announceandsync/notifytakeoptional_signatureand are verified but never spent.Denial detection is header-only.
glkeys onX-Gitlawb-Error. A proxy that strips unknownX-headers returns the denial to the caller as a plain response. Reading the code from the body instead would mean rebuilding the response, which needs a dependency this branch should not carry.Known issues not fixed here
All of these are filed, so please do not open a PR against them without checking the issue first.
Retry-Afteron the ledger-full 429, which tells a client to retry without telling it when.gl init's create-repo path parses the body before checking the status, and parses with.context(...)?rather than a fallback, so a non-JSON error body (the flood brake, a proxy error page) becomes a parse error and the status is lost. It is the only site that loses the status this way; the other capture-parse-then-check sites useunwrap_or_default(), so the status still surfaces there.Operational notes
Migration version 16 is deliberate, not the next free number. #173 currently numbers four migrations 11 through 14 and sits behind main, so on rebase they shift to 12 through 15. Whichever of the two merged second previously had its migration silently skipped, because the runner keyed on the version integer alone and never compared the recorded name. It now compares the name and refuses to start on a mismatch, naming both sides and the remedy, and warns about recorded versions this build does not define so an orphan row is visible before it becomes a boot failure. #173 and this PR need to agree on ordering before either merges.
gitlawb_signature_ledger_total{outcome}counts all eight outcomes of the layer, so the series sum is the traffic the layer saw. Note the brake rejects upstream of the layer, so its 429s appear in no series here.One thing worth confirming before deploy: whether the production nodes share a Postgres. If they do the ledger is globally correct; if not it is per-node, which is the assumption the fail-closed behaviour is written against. I could not determine it from the repo.
Verification
1,109 tests,
cargo fmt --allandcargo clippy --workspace --all-targets -- -D warningsclean. Every guard here was proven load-bearing by reverting the exact production line it protects and observing the test go red: the layer order, the GET skip, the fail-closed paths, the ledger's atomic insert, the per-identity cap, the sweep cutoff, the nonce validation, the rate brake, the retry gating, the migration name check, and the compile-time denial-code guard. The two acceptance tests filed with the issue are green with their#[ignore]removed.Summary by CodeRabbit
New Features
Bug Fixes
Documentation