fix(node): gate GET /ipfs/{cid} tree objects so a withheld subtree's structure can't leak (#135)#173
fix(node): gate GET /ipfs/{cid} tree objects so a withheld subtree's structure can't leak (#135)#173beardthelion wants to merge 52 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughPath-scoped IPFS visibility now applies to blob and tree objects using caller-specific reachable allow-sets. CID resolution uses ChangesPath-scoped object visibility
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant IPFSHandler
participant Database
participant VisibilityPack
participant GitRepo
Caller->>IPFSHandler: Request CID
IPFSHandler->>Database: Resolve CID through pinned_cids
Database-->>IPFSHandler: Candidate Git OIDs
IPFSHandler->>VisibilityPack: Compute caller blob or tree allow-set
VisibilityPack->>GitRepo: Enumerate reachable commits and paths
GitRepo-->>VisibilityPack: Reachable object paths
VisibilityPack-->>IPFSHandler: Allowed object OIDs
IPFSHandler-->>Caller: Serve object or return 404
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
beardthelion
added
crate:node
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/gitlawb-node/src/api/ipfs.rs (1)
143-209: 🚀 Performance & Scalability | 🔵 TrivialThe blob/tree gating, memo selection, and fail-closed arms look correct.
One operational note:
/ipfs/{cid}is reachable anonymously, and under path-scoped rules each request against an object that exists in a repo triggers a full-history reachability walk (onegit ls-tree -rztper reachable commit, plus the root-tree pass for trees). The memo is request-scoped only, so a spray of valid blob/tree CIDs against a large-history repo re-runs the walk on every request. Consider a bounded cross-request allow-set cache (keyed by repo id + head oid + caller) and/or rate limiting on this route to cap the cost.🤖 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/ipfs.rs` around lines 143 - 209, Mitigate repeated full-history walks in the /ipfs/{cid} path by adding a bounded cross-request cache for computed blob/tree allow-sets, keyed by repository ID, current head OID, object type, and caller identity; invalidate or naturally bypass entries when the head changes. Implement this around allowed_blob_set_for_caller, allowed_tree_set_for_caller, and the existing memo lookup, and consider adding rate limiting for anonymous requests to further cap abuse.crates/gitlawb-node/src/git/visibility_pack.rs (1)
391-416: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid
ARG_MAXinroot_tree_pairs.Passing every reachable commit to
git logonargvscales with history size and can fail on very large repos. When that happens, the tree CID path for path-scoped rules skips the repo and the object falls through to a 404. Feed the commits over stdin instead (git log --stdin --no-walk=unsorted --format=%T).🤖 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/git/visibility_pack.rs` around lines 391 - 416, Update root_tree_pairs to avoid placing all commit IDs in argv: invoke git log with --stdin alongside --no-walk=unsorted and --format=%T, write the commits joined by newlines to the child process stdin, and handle stdin/command errors consistently with the existing context and status checks. Remove the argument expansion of commits while preserving the existing tree-pair parsing.
🤖 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/api/ipfs.rs`:
- Around line 143-209: Mitigate repeated full-history walks in the /ipfs/{cid}
path by adding a bounded cross-request cache for computed blob/tree allow-sets,
keyed by repository ID, current head OID, object type, and caller identity;
invalidate or naturally bypass entries when the head changes. Implement this
around allowed_blob_set_for_caller, allowed_tree_set_for_caller, and the
existing memo lookup, and consider adding rate limiting for anonymous requests
to further cap abuse.
In `@crates/gitlawb-node/src/git/visibility_pack.rs`:
- Around line 391-416: Update root_tree_pairs to avoid placing all commit IDs in
argv: invoke git log with --stdin alongside --no-walk=unsorted and --format=%T,
write the commits joined by newlines to the child process stdin, and handle
stdin/command errors consistently with the existing context and status checks.
Remove the argument expansion of commits while preserving the existing tree-pair
parsing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 335f1ebc-783c-4552-bfd6-ebc5894e4d9a
📒 Files selected for processing (4)
crates/gitlawb-node/src/api/ipfs.rscrates/gitlawb-node/src/git/visibility_pack.rscrates/gitlawb-node/src/test_support.rscrates/gitlawb-node/src/visibility.rs
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Make the CID tests and lookup use the identifier published by the pin path
crates/gitlawb-node/src/test_support.rs:2064
These new assertions requestcid_for_oid(...), whose multihash is a Git object ID. The real pin path instead storesCID(sha256(raw object content)), andgl ipfs getsends that stored CID back to this handler. Even with SHA-256 Git repositories those values differ because Git hashes"<type> <len>\\0" + content;get_by_cidtherefore treats a real pinned CID as a nonexistent OID and returns 404 before this new tree gate runs. Please make the serving lookup use the same CID-to-object mapping as pinning (or make the two identifiers deliberately identical), and cover a CID produced from the fixture object's raw bytes rather than encoding its OID. -
[P2] Do not pass the complete history as
git logarguments
crates/gitlawb-node/src/git/visibility_pack.rs:395
root_tree_pairsadds every reachable commit to the process argv. On a long history this exceedsARG_MAX(about 32k SHA-256 OIDs on a 2 MiB limit), so spawninggit logfails; the handler treats that walk error as a denial and returns 404 even to an authorized caller requesting a reachable/root tree. Please complete CodeRabbit's pending root-tree request by feeding commit IDs throughgit log --stdinor by batching/root-resolving them during the existing per-commit traversal.
89d4928 to
7dec45c
Compare
#135) get_by_cid treated the CID's sha2-256 digest as a git oid and cat-file'd it, but a real pin CID digests the raw object content (Cid::from_git_object_bytes), not the framed git object, so every pinned CID 404'd before the #135 tree gate could run. Resolve the incoming CID to its oid through the pinned_cids table (new Db::oid_for_cid + idx_pinned_cids_cid) and gate on that oid; a CID never pinned here is an opaque 404, uniform with a genuine not-found and a visibility denial. The tree-gate tests now build the request CID the way the pin path does (pin_cid_for: read raw bytes, Cid::from_git_object_bytes, record_pinned_cid) instead of from the oid, so they exercise the gate on a production CID rather than an identifier that never occurs. RED before the serve fix (the served-object assertions 404), GREEN after. Also feed root_tree_pairs' commit set to 'git log --stdin' on stdin instead of argv: a long history overflowed ARG_MAX, failing the walk, and the caller fail-closed 404s an authorized reader of a reachable/root tree. Oids are written from a separate thread while the main thread drains stdout, so large input and output cannot deadlock on the pipe buffers; a scale test over 2500 commits guards it. Resolves jatmn's P1 and P2 on #173.
The index was appended to the v1 bundle, which is recorded once in schema_migrations and then skipped, so a node already past v1 would never create it. Move it to a new v11 migration and add an upgrade-path test that drops the index plus its migration record and asserts run_migrations() recreates it. Follow-up to jatmn's P1/P2 on #173; addresses INV-7 caught in pre-push review.
|
Both addressed as of P1 (CID → object mapping). You're right that the lookup and the tests diverged from the pin path. P2 (git log argv). Rebased onto main. |
PR3 of the #62 served-git hardening stack (timeout #165 and teardown wiring #150 are merged). A bounded semaphore caps how many upload-pack / receive-pack / info-refs operations run at once; past the cap a request is shed with a clean 503 + Retry-After before spawning another git subprocess, instead of exhausting the PID/thread table. A permit is acquired at the top of each of the three handlers and held for the whole op, releasing on return. The cap is a portable backstop: the compose pids_limit is absent on Fly, whose 500-connection cap is a different axis. Size --max-concurrent-git-ops (GITLAWB_MAX_CONCURRENT_GIT_OPS, default 128) below the process budget. Range 1..=1_048_576 so 0 (shed everything) and an oversized value that would panic tokio's Semaphore at boot are clean CLI errors. Known gap, tracked separately: info/refs and the withheld-blob (upload_pack_excluding) path are not duration-bounded and do not reap their git child on client disconnect, so a hung git on those two paths holds its slot until it exits and live git can briefly exceed the cap. The main pack path (run_git_service) tears its group down on drop. Tests: Overloaded maps to 503 + Retry-After; the config knob defaults and rejects out-of-range; git_permit sheds at capacity and releases; and each of the three endpoints sheds with 503 when the semaphore is exhausted (load-bearing: drop the permit line and the endpoint test goes red).
Add max_concurrent_git_pushes (default 32) and max_concurrent_reads_per_caller (default 16), both clap range(1..=1_048_576) so an oversized value is a clean CLI error, not a Semaphore::new boot panic. The per-caller knob documents that per-source-IP keying is only as granular as GITLAWB_TRUSTED_PROXY. Wiring lands in the following commits; these are the config surface for the #174 concurrency-fairness fix. Resolves jatmn P1a/P1b groundwork on #174.
git-receive-pack now draws from a separate git_write_semaphore (max_concurrent_git_pushes) instead of the shared pool, so a flood of anonymous reads can no longer shed an authenticated push at admission (jatmn P1a). The shared field is renamed git_read_semaphore and continues to gate upload-pack and both info/refs advertisements. The write permit stays above acquire_write so it precedes the Tigris fresh-acquire (INV-10). Handler-layer tests: write-pool shed (503), and a cross-boundary proof that an exhausted read pool does NOT shed a push; both mutation-checked (routing receive-pack back to the read pool flips each RED). 497 tests pass. Part of #174.
Adds PerCallerConcurrency, a bounded-keyed in-flight limiter (distinct from the request-rate RateLimiter) so no single caller monopolizes the served-git read pool. Each caller (per-DID when signed via optional_signature, else per-source-IP via client_key) may hold at most max_concurrent_reads_per_caller concurrent reads; over that it sheds 503. The key map is self-bounding (a key is dropped when its in-flight count hits zero) with a reject-before-insert max_keys backstop so a key farm can't grow it (INV-15). Applied in git_upload_pack and both info/refs advertisements, acquired after the visibility gate so a denied request never consumes a slot (KTD7). Primitive unit-tested (cap + self-bounding + reject-before-insert) and mutation-checked. Handler-layer SC2: same-caller sheds while a different caller passes, proven on BOTH git_info_refs and git_upload_pack with independent mutation probes; plus a None-key bypass test. Per-source-IP keying is trust-config dependent, documented on the config knob. 502 tests pass. Part of #174.
info_refs ran a bare Command::output() with no timeout and no process-group teardown, so a hung git pinned its concurrency slot indefinitely and a client disconnect orphaned the child (jatmn P1b). Extract the timeout + process_group(0) + KillGroupOnDrop core from run_git_service into a shared drive_git_child, and route info_refs through it with an injectable git_bin. A hung advertisement now aborts with GitServiceTimeout (mapped to 504); disconnect reaps the group. run_git_service's teardown tests all pass through the shared core (proving the group teardown info_refs inherits), the real-git filter tests cover the advertisement happy path, and a new watchdog-bounded test proves a hung advertisement times out. 503 tests pass. Part of #174.
The filtered-pack path ran the whole rev-list + pack-objects build inside a spawn_blocking, so an outer tokio timeout could not cancel the blocking thread and a client disconnect orphaned the git child while the permit freed (jatmn P1b, the second gap path). Split it: rev-list enumeration stays blocking off the runtime (rev_list_keep), but the streaming pack-objects stage now runs under the shared drive_git_child on the async side, so it is duration-bounded (GitServiceTimeout -> 504) and its process group is reaped on disconnect. build_filtered_pack becomes async and takes a git_bin seam + timeout; upload_pack_excluding threads the git_service_timeout through. A watchdog-bounded test proves a hung pack-objects times out (rev-list fast, pack-objects hangs). The refactor's happy path is covered by the existing filtered-pack correctness and real-git partial-clone/fetch tests, all still green; disconnect/group-teardown is the shared drive_git_child code proven by the run_git_service tests. 504 tests pass. Part of #174.
#62) The max_concurrent_git_ops and git_service_timeout_secs doc-comments (and .env.example) described the info/refs and withheld-blob paths as unbounded follow-up gaps. Both are now closed (#174): the comments reflect the read/write pool split, the per-caller sub-cap, and that every capped path is duration-bounded with process-group teardown. Verified the pattern-doc pre-ship checklist: every git_permit / write-permit / per-caller site holds only a timeout+teardown git path, and all three size knobs are range(1..=1_048_576). Closes the #174 work. No behavior change.
Review of the served-git concurrency cap found no P0/P1; these are the verified P2 follow-ups. config: the max_concurrent_git_ops doc overclaimed that "every capped path is duration-bounded." The rev-list object enumeration in the withheld-blob path still runs in an uncancellable spawn_blocking, so a stuck rev-list can hold its slot until git exits. Scope the guarantee to the streaming stages and name the residual. Also tighten the fairness claim: the receive-pack advertisement shares the read pool (a shed advertisement is a cheap retryable GET); only the push POST is on the isolated write pool. api/repos: add info_refs_per_caller_cap_keys_on_did_not_ip, the missing handler proof that a signed caller is keyed by its DID, not its source IP. Filling the DID slot sheds a request from a free IP; collapsing read_caller_key to its IP arm turns the assertion green-not-503 (mutation-verified RED). api/repos: extract acquire_read_caller_permit so both read handlers share one shed path instead of a duplicated match block. rate_limit: recover from a poisoned PerCallerConcurrency mutex instead of panicking. The critical section is pure counter arithmetic and cannot poison the lock, but a panic there would brick the limiter for every caller. 505 tests pass; clippy -D warnings and fmt clean. Part of #174.
The read-pool knob was referenced by the push and per-caller entries' comments but had no example line of its own, so operators couldn't discover it from the template. Add it with the config default (128).
…ng enumeration can't pin a git permit (#174)
…ID (#174) read_caller_key returned the authenticated DID when a caller signed, dropping the source-IP key. Public read routes accept any valid did:key via optional_signature with no admission step, so one host could mint N disposable DIDs and hold max_concurrent_reads_per_caller slots under each, multiplying its budget N-fold and filling the global read pool, while the same host unsigned was capped on its IP. Key the read sub-cap on the resolved source IP for every caller, signed or not, mirroring the push path's IpRateLimiter which already throttles on source IP for this exact DID-farm reason. Drops the now-unused caller_did parameter at both call sites (git_info_refs, git_upload_pack). Inverts info_refs_per_caller_cap_keys_on_did_not_ip into info_refs_per_caller_cap_keys_on_ip_not_did: fill one source IP's slot, then two requests signed under different DIDs from that same IP both shed 503 (farm defeated), while a signed request from a different IP keeps its own budget. RED on the DID-keyed tree, GREEN after.
) git_info_refs acquired git_read_semaphore for BOTH services, so the push handshake (GET /info/refs?service=git-receive-pack) competed in the global read pool. An anonymous clone flood could exhaust that pool and shed a legitimate push with 503 during its required advertisement phase, before it ever reached git_write_semaphore on the POST. The write pool exists precisely so anonymous reads cannot shed an authenticated push, but only the POST drew from it. Select the pool by service: the receive-pack advertisement (phase one of a push) now draws from git_write_semaphore, like the git-receive-pack POST, so a saturated read pool cannot starve it. The per-IP push_rate_limiter that already brakes the advertisement stays as the anti-flood control, and the advertisement stays reader-visible with no new auth requirement. Because the receive-pack branch is now a write-path op, it no longer consumes a read per-caller slot. Handler-layer proofs: with the read pool at zero the receive-pack advertisement survives while the upload-pack advertisement sheds; with the write pool at zero the receive-pack advertisement sheds while upload-pack is unaffected; and a receive-pack advertisement from an IP whose read per-caller budget is full still gets through (mutation-checked, RED when the skip is neutralized).
…#174) The withheld-blob classification walk (blob_paths) fanned out blocking git children with no deadline and no process-group teardown: git for-each-ref, git cat-file, git rev-list, a git ls-tree per commit, and an uncounted git rev-parse (via store::head_commit). A hung or pathologically slow child pinned the caller's served-git permit for the whole hang, and on client disconnect the spawn_blocking task and its git children ran on, orphaned. blob_paths is the shared core of five callers: the upload-pack serve path (holds a read permit) AND, inside git_receive_pack, the post-push replication and encrypt-then-pin walks (hold the write permit U2 reserves for pushes). So the same unbounded walk could pin either pool, and leaving the write-side twin unbounded would have made U2's reservation a claim that does not match behavior. Bound every git child at the blob_paths spawn seam on the blocking side: each child runs in its own process group with a watchdog thread that SIGTERMs (then SIGKILLs) the group on one shared deadline spanning the whole walk, and retains admission until the group is reaped. This is the blocking-side counterpart of smart_http::drive_git_child (spawn_blocking cannot be cancelled by an async timeout). blob_paths stays sync, so all five callers keep their signatures and the 32 classification tests are unchanged; because every caller funnels through blob_paths, one seam bounds both the serve and replication paths. The previously unbounded store::head_commit child becomes a bounded git rev-parse inside the walk. A walk that hits its deadline carries GitServiceTimeout, which the serve handler now maps to 504 rather than a generic 500. Proof: a fake git that hangs on rev-list makes blob_paths return GitServiceTimeout within the watchdog budget (not block on the child) and the recorded process-group leader is reaped, not orphaned; neutralizing the watchdog kill makes it hang past the budget (RED). The 32 real-git classification tests stay green through the refactor, including detached-HEAD, non-standard-ref, and deleted-in-history cases.
GITLAWB_GIT_SERVICE_TIMEOUT_SECS bounds the info/refs advertisement too: smart_http::info_refs drives it through drive_git_child under this timeout, with a passing test proving the 504. The old note claimed it does not. It also claimed the withheld-blob path is unbounded; after the blob_paths seam bound (this PR) the walk is bounded and reaped, by a fixed internal deadline rather than this env var, so the line now states that precisely instead of overclaiming this setting covers it.
Code review found run_bounded_git's watchdog could return a spurious 504 and signal a recycled process group. The watchdog runs off a wall clock on its own thread; done_tx.send() only fires after child.wait() reaps the leader, so a walk that finished within microseconds of the deadline took the watchdog's Timeout branch, discarded a fully-captured successful result, and returned GitServiceTimeout (a 504 for a walk that actually completed). Worse, the Timeout branch SIGTERMed -pgid unconditionally after the leader was reaped, so a recycled pgid could be signalled, the exact hazard smart_http guards via disarm-after-wait. Set a reaped AtomicBool the instant the main thread reaps the child; the watchdog checks it before every kill and stands down if the leader is already reaped. Gate the timeout verdict on !status.success(), so a child that exited on its own is never reported as a timeout even if the watchdog fired late. Add the survived-SIGKILL warn smart_http's reap already emits, for operator visibility on a wedged (D-state) git. The hung-walk test stays green (a killed child exits by signal, not success, so it still surfaces GitServiceTimeout and reaps the group) and the 32 real-git classification tests stay green (a fast walk is never spuriously killed).
…nnot starve the write pool (#174) U2 moved the receive-pack info/refs advertisement onto git_write_semaphore to keep an anonymous read flood from starving the push handshake. But the advertisement is anon-reachable on public repos and holds its write permit across the slow acquire_fresh Tigris download, and the only per-source brake on it was the push RATE limiter, not a concurrency cap. So a multi-source flood of receive-pack advertisements could hold the write pool's slots across those downloads and shed authenticated pushes (both the advertisement and the owner-gated git-receive-pack POST draw from the same pool). U2 thus introduced the first anonymous consumer of the write pool the state doc promised anon could never reach; the plan's residual note (no worse than the POST) was wrong, because the POST is owner-gated and the advertisement is not. Add git_push_advert_per_caller, a per-source concurrency sub-cap on the receive-pack advertisement keyed on the resolved source IP (the same PerCallerConcurrency mechanism U1 uses for reads), sized to an eighth of the write pool so a single source holds at most that share and saturating the pool takes many distinct source IPs, each also braked by the per-IP push rate limiter. The upload-pack advertisement keeps its read-pool per-caller cap; the owner-gated POST is unchanged. Correct the state doc for git_write_semaphore accordingly. Handler-layer proof: a source at its receive-pack advertisement cap sheds 503 (RED before the acquisition, 500-not-503), while a different source and the upload-pack advertisement are unaffected. Full suite 510 green.
…d timeout (#174) Close the reasoned-not-run gaps from the code review by making the walk's git binary and timeout injectable, then driving the missing branches with a real handler and a fake git instead of reasoning about them. - Add state.git_bin and *_bounded variants of the walk entry points taking (git_bin, timeout); the served handlers (upload-pack serve, receive-pack replication and full-scan and encrypt-pin, and the ipfs gate) now pass the operator-configured GITLAWB_GIT_SERVICE_TIMEOUT_SECS, so the whole walk is bounded by the same budget as the other served-git ops rather than a fixed constant. The git_bin-less wrappers stay for the real-git classification tests. Newly vetted by execution (not reasoning): - receive-pack replication path is bounded: replication_withheld_set with an injected hung git returns within the budget and fails closed, so it cannot pin the write permit git_receive_pack holds across it. - a hung withheld-blob walk on the upload-pack POST returns 504 (real handler, real repo on disk, injected hung git), proving the GitServiceTimeout -> git_service_app_error wiring end to end. - the watchdog status-gate: a child that exits successfully is not reported as a timeout even when the watchdog fired (mutation-checked: drop the guard -> RED). - SIGKILL escalation: a SIGTERM-ignoring child is still reaped via SIGKILL and the group is gone; a truly uninterruptible D-state child (unreapable by any signal) is the documented residual, matching the async teardown. - the advertisement per-source cap sizing never derives 0. Full gitlawb-node suite 515 green.
…a fixed const (#174) Follow-up to threading the configured timeout into the walk: the walk now honors GITLAWB_GIT_SERVICE_TIMEOUT_SECS on both the serve and replication paths, so the README no longer says a fixed internal deadline.
The serve path read the object with a blocking git cat-file directly on the Axum worker and buffered the whole stdout with no size bound, so a large public blob (enumerable from the pins index) could block a runtime thread or exhaust memory. Precheck the object size (cat-file -s), then run size + read + CID verify inside spawn_blocking, withholding anything over a configurable cap (ipfs_max_served_object_bytes, default 32 MiB). A content-addressed serve cannot verify a streamed body, so there is no streaming arm: buffer-verify-then-serve up to the cap, reject larger with zero body bytes. The F2 integrity check moves into the blocking closure so no unverified bytes are ever assembled. A cost counter guards the size cap both ways.
…fra errors (#173) Two code-review follow-ups on the F1/F6 work: - pin_sources_for_oid now LIMITs to MAX_PIN_SOURCES, making the resolver's per-source work a true O(MAX_PIN_SOURCES) ceiling (INV-10) regardless of an insert-side overshoot or the uncounted first-pinner union row. The record_pin_source doc no longer overclaims a hard ceiling: under Postgres READ COMMITTED concurrent inserts of distinct repos for one object can overshoot the count guard by a small bounded amount, which the read-side LIMIT now absorbs. - The bounded serve read distinguishes a git spawn/IO failure (ReadErr, logged and skipped) from a genuine not-found (Gone), restoring the object-read warn log the spawn_blocking refactor dropped, so an infra failure is not silently rendered as a 404.
…ors as 503 (#173) Two findings from an independent adversarial pass (Grok 4.5): - The MAX_PIN_SOURCES LIMIT added in the prior review fix sat on the whole first-pinner-plus-sources UNION with a lexicographic ORDER BY, so for a legacy pin (source in pinned_cids.repo_id but not yet in pin_repo_sources) an attacker could push the same object from MAX_PIN_SOURCES repos whose grindable ids sort before the public source and evict it from the window, turning a public CID that served 200 into a 404. Always include the first-pinner (a single row by PK) and apply the LIMIT only to the additional pin_repo_sources rows, so the original source can never be evicted. Bound stays O(MAX_PIN_SOURCES + 1). A new test drives a legacy public first-pinner plus MAX_PIN_SOURCES lower-sorting attacker sources and asserts the public object still serves (RED on the old whole-union LIMIT, GREEN after). - The F6 ReadErr split logged an infra read failure but still returned an opaque 404, indistinguishable from a genuine not-found. Mark the search truncated on a git spawn/IO failure so a wholly-unserved request tails to a retryable 503, making the infra-vs-not-found distinction client-visible.
|
Addressed on F1, retain every source repository. A new F2, legacy provider-CID rows. F3, rate limit before the preload. A non-consuming F4, drain cat-file while writing. F6, bounded off-worker read. The read moves into F5, bound concurrent walks. This is the ipfs-walk concurrency cap already in #174 ( |
…concurrency base Rebased onto #174 (fix/served-git-concurrency-cap). #173's incremental history cannot replay commit-by-commit because #174 rewrote the same handler files in parallel; this single commit carries the fully-integrated tree (identical to the verified merge result), with the follow-up fixes as separate commits on top. Brings in #173: GET /ipfs/{cid} CID->oid resolution via pinned_cids, per-caller path-scoped blob/tree/commit-tag gating (#135/#173 F1-F6), and the pin-source provenance table. Adapts #173's tree/commit-tag walks to #174's run_bounded_git so the held /ipfs walk-concurrency permit is duration-safe (F5).
P1-A: gate_and_serve held the /ipfs walk permit across a bare repo_store.acquire; wrap it in git_acquire_timeout_secs (mirroring #174's own handler) so a cold/hung Tigris acquire cannot pin the global walk slot. On expiry skip the repo and mark the search truncated (retryable 503, not a false 404). P1-B: the local-IPFS pin path recorded Kubo's provider Hash as pinned_cids.cid; for objects above the block size that is a dag-pb root that does not hash the raw content, so GET /ipfs/{cid} listed then 404'd them under the F2 integrity check. Record the locally-computed raw-content CID, mirroring the pinata twin.
…kfill pinata provenance P2-E: the object-type probe and the F6 size+read ran bare git cat-file inside spawn_blocking while the /ipfs walk permit was held, so a wedged cat-file could pin the global walk slot for the request's life. Bound both under git_service_timeout_secs; on timeout free the slot (truncated -> retryable 503) and skip the repo. P2-D: the pinata already-pinned branch now backfills NULL first-pinner provenance in lockstep with the ipfs_pin skip branch (consistency; the object was already resolvable via the pin_repo_sources union).
A provenanced object has a bounded source set (first-pinner + up to MAX_PIN_SOURCES additional). With the walk ceiling at 16 == MAX_PIN_SOURCES, an authorizing public source sorting after 16 path-scoped denials was never reached: the ceiling truncated the search and returned a false 503 for a readable object. Raise the ceiling to MAX_PIN_SOURCES + 1 so the whole bounded provenance set is always tried; the legacy scan stays bounded by MAX_LEGACY_PROBES_PER_REQUEST.
…chIncomplete - ipfs_pin::pin_new_objects returns the provider Hash (not the raw resolver key), matching the pinata twin's contract; the DB cid stays the raw content CID. The return is logging-only here, so this is drift-avoidance, not a functional fix. - AppError::SearchIncomplete now carries Retry-After: 1 like Overloaded, so both retryable 503s from the /ipfs handler advertise the retry hint consistently.
…n fallback
An attacker who pins a public object from MAX_PIN_SOURCES repos before the
legitimate public source registers fills pin_repo_sources (record_pin_source is
first-N-wins and drops later sources silently); the buried public source is then
never reached because a non-empty provenance set suppressed the legacy scan, so
anon GET /ipfs/{cid} 404s forever for a public object (re-breaks F1).
U1: db::pin_sources_at_cap reports whether pin_repo_sources is at MAX_PIN_SOURCES
for an oid (the only observable signal that a servable source may have been
dropped, since the write cap never overshoots).
U2: get_by_cid falls back to the bounded legacy scan on a provenance miss when the
set is empty OR at_cap. The scan gates every repo through the real per-caller gate,
so it finds the buried public copy. A complete (non-full) set still fast-404s, so
ordinary denials never fan out to O(repos) (INV-10/F3); the fallback honors the
is_throttled peek.
Regression ipfs_cid_buried_public_source_still_serves_via_scan_fallback: 404 before,
200 after; pin_sources_at_cap_flips_at_max covers the boundary.
ce02f70 to
d26cf6d
Compare
|
Rebased onto #174 ( Because the branch history couldn't replay commit-by-commit onto #174 (the same handler files were rewritten in parallel), this is a clean linear reconstruction: one integration commit carrying the fully-integrated tree, plus the follow-up fixes as separate commits. The diff against #174 is what a straight replay would have produced. Integrating the two paths surfaced a few more issues, fixed here:
One security fix in the F1 pin-source area, worth a close look: A few design calls I've settled, with the reasoning inline so they're on the record:
Full @jatmn re-requesting review — the design notes above are settled with the reasoning inline, so the most useful pass is on correctness: the scan-fallback path and the walk-permit bounding are the two spots worth trying to break. |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Keep IPFS admission held until cancelled work has actually stopped
crates/gitlawb-node/src/api/ipfs.rs:168
The IPFS global and per-source permits are handler locals, but this handler starts uncancellablespawn_blockinggit work at the object probe, visibility-walk, and object-read sites. Dropping a request future releases both permits while that work continues; the test at line 1294 explicitly documents that behavior. The timeout arms have the same problem forstore::{object_type,object_size,read_object_content}: they abandonCommand::output()work and release admission even though neither the thread norgit cat-filehas been reaped. A client can repeatedly cancel a request (or make the commands time out) and run arbitrarily more workers/children thanMAX_CONCURRENT_IPFS_WALKSand the per-source cap. Transfer admission ownership to the task/reaper and use bounded process teardown for the probe/read paths, so it is released only after the work is gone. -
[P1] Preserve read admission through the filtered-pack reaper
crates/gitlawb-node/src/api/repos.rs:943
The path-scoped upload-pack branch keeps_permitand_caller_permitonly in the handler, then callsupload_pack_excluding, whoserev-list/pack-objectswork receives noAdmissionGuard. On disconnect, the handler drops_holdimmediately whileKillGroupOnDropcontinues reaping the Git process group, so repeated cancelled filtered clones bypass the read and per-caller concurrency caps. Pass the admission guard into this filtered-pack path and retain it through process-group reap, as the plain upload-pack path does. -
[P1] Do not discard a later push while an encryption task is finishing
crates/gitlawb-node/src/api/repos.rs:1441
Per-repo coalescing drops every push that arrives while the first detached task is in flight. That task computes its recipient/object snapshot beforeencrypt_and_pin; if a second push adds a withheld blob after that snapshot but before the first task finishes encryption or anchoring, it is coalesced and no task ever processes the new blob. Unless another push happens later, the authorized recovery copy is permanently absent. Coalescing needs a dirty/requeue mechanism (or a durable reconciliation pass) rather than treating an in-flight snapshot as coverage for later pushes. -
[P2] Wire the advertised IPFS repo-walk limit into the resolver
crates/gitlawb-node/src/main.rs:377
GITLAWB_IPFS_MAX_REPOS_WALKEDis parsed and documented as the per-request/ipfsfan-out cap, but it is never read in production. State always uses the fixed 17 history-walk and 256 legacy-probe constants instead, so setting the advertised default/override has no effect; an operator trying to restrict a legacy request to one or 64 repos can still incur 256 acquire/git probes. Use the parsed configuration for the actual resolver budget (with the existing incomplete-search response), or remove/correct the knob and its documentation. -
[P2] Do not treat best-effort source provenance as a complete source set
crates/gitlawb-node/src/ipfs_pin.rs:137
A later repository sharing an already-pinned object records its source with a best-effort call that only logs database errors. The resolver then treats any nonempty, below-cap source set as complete and skips its fallback scan. If a private first source is recorded and the source insertion for a later public copy fails transiently, subsequent anonymous CID requests try only the private source and return 404 even though the public repository contains a readable copy. Make source recording durable/retriable, or preserve an incompleteness signal that keeps the bounded fallback available after a failed insert. -
[P2] Separate route request limiting from per-probe work accounting
crates/gitlawb-node/src/api/ipfs.rs:522
The route middleware already chargesipfs_rate_limiteronce for every/ipfs/{cid}request, but the legacy scan charges that same bucket again for every candidate probe. Consequently a one-probe legacy request consumes two of the documented “requests per hour” allowance, and withGITLAWB_IPFS_RATE_LIMIT=1it is rejected inside its first probe after the route has admitted it. Use a separate work-budget limiter or avoid the second charge; the current single setting cannot simultaneously implement the documented request limit and per-probe accounting. -
[P2] Migrate or stop advertising legacy provider-CID rows
crates/gitlawb-node/src/db/mod.rs:875
Existingpinned_cids.cidvalues were provider CIDs, while this change now stores raw-object CIDs and rejects any requested CID whose bytes do not recompute to that raw CID. Migrations 11–13 add only indexes/provenance/sources, andlist_pinned_cidscontinues to advertise the unchanged old value. After an upgrade, clients receive a CID from/api/v1/ipfs/pinsthat the new/ipfs/{cid}path deliberately returns as 404; a re-pin skip does not rewrite it. Backfill the resolver key, retain a safe compatibility mapping, or omit these stale records until they are repaired.
The path-scoped upload-pack branch kept the read and per-caller permits as handler locals and called upload_pack_excluding with no AdmissionGuard, so a cancelled filtered clone released admission while KillGroupOnDrop was still reaping the git group, bypassing the read and per-caller concurrency caps. drive_git_child now returns the disarmed guard on success so one guard rides both build_filtered_pack stages (rev-list then pack-objects); the handler builds the guard from the permits as the plain path does. Adds a disconnect regression and an INV-22 gate row, both proven load-bearing.
get_by_cid held the global walk permit and the per-source permit as handler locals, so dropping the request future released both while the spawn_blocking probe, visibility walk, and object read kept running; a cancel-spam or timeout client could exceed MAX_CONCURRENT_IPFS_WALKS and the per-source cap, and the bare cat-file probe/read children had no teardown at all. Move the gated serve pipeline into a detached task that owns an AdmissionGuard, awaited by the handler, so admission releases only after the bounded work is gone; back the /ipfs probe and read with run_bounded_git twins for process-group teardown. Adds cancel-mid-walk, timeout-reap, and cancel-spam regressions plus two load-bearing INV-22 rows. Also reflows one U1 test line to satisfy cargo fmt.
Per-repo coalescing dropped every push that arrived while a repo's encryption task was in flight, and no reconciliation ever processed it, so a withheld blob added by the coalesced push had its recovery copy permanently absent. The drop also silently skipped the coalesced push's local-IPFS pin work. EncryptInflight becomes a dirty-flag map; the detached task loops, and the atomic check-and-clear sits at the task tail, unconditional on the has_path_scoped_rule gate and walk success (a public or rules-free repo would otherwise exit before it ran). Each requeue re-reads repo state fresh and re-enumerates the pin half through the fail-closed full scan, never bare list_all_objects, so a coalesced rule change is honored and no withheld or dangling object leaks. At-most-one-task-per-repo is preserved.
The knob was parsed and documented as the per-request /ipfs fan-out cap but never read in production: AppState seeded the legacy-probe budget from a fixed 256 constant, so setting the knob did nothing and an operator could still incur 256 acquire/probe operations per request. Seed ipfs_max_legacy_probes from the knob (default raised to 256 to preserve shipped behavior); leave the history-walk ceiling on its MAX_PIN_SOURCES+1 constant so a provenanced request is never truncated into a false 503. Updates the README and .env.example to the shipped behavior and adds cap-honored and plumbing tests.
The route middleware charged ipfs_rate_limiter once per request while the legacy scan and provenance walk charged the same bucket per probe/walk, so a one-probe request cost two tokens and GITLAWB_IPFS_RATE_LIMIT=1 admitted the request at the route then 429'd it at the pre-scan peek. Add a bounded ipfs_work_rate_limiter, move the in-scan, per-walk, and pre-scan charges onto it, and leave the route limiter as the pure once-per-request brake. The work-budget capacity derives from the route limit with a floor of one full legacy-probe budget per window (no new operator knob), so a default-config deep search never self-throttles. Adds it to the periodic sweep, updates all three AppState construction sites, and rewrites the contradictory limiter comments.
The three warn-only record sites in the detached post-push task dropped a transient DB failure silently, leaving a permanently incomplete pin-source set that makes the resolver 404 a valid public copy. Wrap them in a bounded retry (inserts are idempotent via ON CONFLICT DO NOTHING); on exhaustion the warn still fires so behavior degrades to today's. Documents the residual crash/outage window that only a reconciliation sweep retires.
Before this PR the pin path stored the provider CID (Kubo dag-pb / Pinata) in pinned_cids.cid, which the new resolver recomputes-and-404s, while /api/v1/ipfs/pins still advertises the stale value. Repair a legacy row in the already-pinned skip branch: gate on a cheap stored-CID codec check (only a non-raw-codec CID reads bytes, so raw rows keep the DB-only skip cost), recompute the raw CID, and rewrite it while stashing the old value in a new v14 legacy_provider_cid column (distinct from pinata_cid, which gates the Pinata pin-skip). Rows whose bytes are gone stay withheld. Documents that the deferred one-shot sweep, not this opportunistic path, fully retires the window.
Code review found each candidate's size and content reads were each granted a full git_service_timeout, so a single served candidate could hold the /ipfs walk permit for 2x the timeout. Share one deadline across both stages (mirrors build_filtered_pack) so the read holds admission for one timeout total, and correct the admission-bound comment: the worst case is O(candidates x timeout) bounded by the walk concurrency cap, not a single deadline.
… pin the coalescing key A cross-model adversarial pass found that run_post_push_replication holds the per-repo coalescing key until requeue_or_release, but pin_new_objects reached it only after two unbounded git reads: the U7 legacy-repair read and the pre-existing pin read, both plain Command::output with no teardown. A wedged cat-file on a stuck backend hung the task forever, so the key was held until process death and later pushes only marked it dirty without spawning a replacement (the same class this PR closed on the /ipfs serve path). Add read_object_bounded (the bounded twins under one shared deadline) and use it at both sites; a timeout skips that object and lets the task proceed to requeue_or_release. Adds a wedge-reap regression proven load-bearing two ways.
|
All seven addressed and pushed (nine commits on Keep IPFS admission held until work stops — the gated serve pipeline now runs in a detached task that owns the Preserve read admission through the filtered-pack reaper — Do not discard a later push while an encryption task is finishing — Wire the advertised IPFS repo-walk limit into the resolver — Do not treat best-effort source provenance as a complete set — bounded retry around the Separate route request limiting from per-probe work accounting — a separate bounded Migrate or stop advertising legacy provider-CID rows — opportunistic repair on the already-pinned skip path: a cheap codec gate first (a raw CIDv1 key is already correct and reads no bytes), then recompute and rewrite, stashing the old value in a new v14 Two further bits of hardening on the same surface while I was in here: the Re-requesting your review. |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Make the write-acquire timeout release its advisory lock
crates/gitlawb-node/src/api/repos.rs:1497
The timeout can cancelacquire_writeafter it has acquired the session-levelpg_try_advisory_lock, but before it returns aRepoWriteGuard.acquire_writethen awaits the Tigris exists/download path, while the only unlock isRepoWriteGuard::release; dropping the future therefore leaves the pooled session holding the lock. A stalled storage backend makes the request return 503, but subsequent pushes to that repository can remain locked out (and reusing that connection can recursively acquire the session lock). Make acquisition cancellation-safe insideRepoStoreand add a post-lock storage-timeout regression test. -
[P1] Do not clear coalesced push work before a refresh has been obtained
crates/gitlawb-node/src/api/repos.rs:961
requeue_or_release()clears the dirty bit before the requeue pass reloads repository state. Ifget_repo_by_idfails, orlist_visibility_rules(...).await.ok()hides an error, the code changes the pass towithheld = Noneand exits on the next clean check. A push that arrived while the first task was running is then never included in the pin/encryption recovery pass, with no later push required to trigger it. Preserve or durably retry the dirty state until a refreshed replication pass has completed; do not turn a rules-read failure into an empty policy. -
[P1] Do not treat incomplete pin-source bookkeeping as an exhaustive provenance set
crates/gitlawb-node/src/api/ipfs.rs:324
The resolver skips its all-repository fallback whenever the source set is nonempty and below the cap, but source writes are separate best-effort operations. For example, an object first pinned from a private source can later be present in a public repository while that laterrecord_pin_sourceexhausts its retries (the Pinata path does not retry it at all). The stored below-cap set then contains only the private source, so/ipfs/{cid}returns 404 even though the public repository has the pinned object. Record the source atomically with the pin or persist incompleteness and retain a safe fallback on a provenance miss. -
[P1] Migrate legacy provider-CID pins before changing the resolver contract
crates/gitlawb-node/src/ipfs_pin.rs:258
Existing rows are repaired only when a future push delta happens to contain the object. Normal Git negotiation omits objects the node already has, and this change adds no migration or startup sweep, so upgraded nodes continue to advertise their legacy provider CID from/api/v1/ipfs/pinswhile the new/ipfs/{cid}integrity check deliberately withholds it. This leaves previously pinned data unresolvable indefinitely in the common no-repush case. Backfill the existing rows (with a bounded, recoverable sweep) or retain a retrieval-compatible mapping until migration completes.
2 participants
What
GET /ipfs/{cid}servedtreeobjects of a withheld subtree to callers denied that subtree. A git tree body is<mode> <name>\0<raw-oid>per entry, so fetching the tree CID of a withheld directory returned every child filename and child oid in cleartext, recursively. Blob content was already protected; this closes the structure leak so the CID surface matches whatget_treeenforces on the REST path.Approach
Tree objects are now gated against a caller-aware allowed-tree-set, the mirror of the existing
allowed_blob_set_for_caller:object_pathswalk (git ls-tree -rztper reachable commit) thatblob_pathsand the newtree_pathsboth filter, so the two gates cannot drift.blob_pathsoutput is byte-identical, so its callers are unaffected.tree_pathsis thekind == "tree"slice plus each reachable commit's root tree (resolved in onegit log --format=%Tpass, sincels-treenever emits a commit's own root).get_by_cidgates ablobagainst the allowed-blob-set and atreeagainst the allowed-tree-set (lazy per repo, off the async runtime, fail-closed on any walk error). Commits and tags stay served: they expose only root-level metadata the caller already cleared the/gate for.The withheld directory's own tree (path
/secret) is denied, not just its descendants, so parity withget_treeholds.Reachability and scope
get_by_cidresolves a CID by treating its sha256 digest as a git oid, which only matches in sha256 repos; production repos currently init--object-format=sha1, so the endpoint is largely dormant until a sha256 migration. This lands the gate ahead of that. The replication/pin path exports the same withheld-tree structure to IPFS independently of the object format and is the more urgent sibling, tracked separately in #172.Tests
Deny paths driven through the real handler:
get_tree).blob_pathsoutput is asserted byte-identical after the walk refactor.Closes #135.
Summary by CodeRabbit
New Features
/ipfs/{cid}to resolve via pinned CID → git-object OID mapping with visibility-aware tree handling.Bug Fixes
Tests
Chores