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 10 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 docstrings
🧪 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.
blob_paths becomes the kind==blob slice of a new private object_paths (one git ls-tree -rzt per reachable commit, emitting (oid, /path, kind)), so the blob and forthcoming tree visibility gates share one walk and cannot drift. blob_paths output is byte-identical; all callers unchanged.
The tree analog of allowed_blob_set_for_caller: tree_paths = object_paths'
kind==tree slice plus each reachable commit's root tree at "/" (derived via
<commit>^{tree} over the shared reachable_commits set, since ls-tree never
emits a commit's own root). allowed_tree_set_for_caller keeps the tree oids
visibility ALLOWS the caller at some path. Fail-closed on dangling/unreachable
trees. Not yet wired into get_by_cid (U4).
…135) GET /ipfs/{cid} served tree objects of a withheld subtree unconditionally under a path-scoped rule, leaking every child filename and child oid where get_tree protects them. Gate trees against the caller's allowed-tree-set the same way blobs are gated (lazy per-repo memo, spawn_blocking, fail-closed); commits/tags still serve since they expose only root-level metadata the caller already cleared the "/" gate for. Tests (RED before this gate, GREEN after; mutation-certified load-bearing): - withheld subtree tree CID -> 404 for anon, witnessed on RAW body bytes (a tree stores child oids as raw bytes that lossy-decode to U+FFFD, so a hex contains-check is vacuous); listed reader/owner still see it (200) - root/ancestor tree, commit, tag stay served; dangling tree fails closed - visibility_check denies the withheld dir's own path /secret (T1)
Update the get_by_cid and allowed_blob_set_for_caller doc comments and the CONCEPTS.md "Structural object" entry: trees are now gated on the object-fetch surface via the allowed-tree-set; only commits/tags remain served-as-structure. The entry also records the cross-surface gap the fetch gate exposes — the replication/pin filter still exports withheld-subtree trees (pin-path leak, #172).
… docs Review follow-up on the #135 tree gate: - tree_paths computes reachable_commits ONCE and threads it to both the ls-tree walk and the root-tree pass (was computing it twice), and resolves all root trees in one `git log --no-walk --format=%T` pass instead of a per-commit `git rev-parse ^{tree}` fan-out — a tree-set walk now costs the same subprocess order as the blob walk. - Extract the shared allowed-set inner loop (allowed_set_from_pairs). - The deny test's raw-byte checks on the 404 body were vacuous (the 404 body is an opaque error string); move the structure witness to the reader's 200 response, where the tree body genuinely carries b.txt + the raw child oid. Status stays the load-bearing deny check (mutation-recertified RED). - Fix the module doc that still claimed trees are never withheld.
Add the handler- and helper-level cases the code review flagged: - ipfs_cid_dangling_tree_fails_closed_under_path_rules: a dangling tree via git mktree 404s through cid_router for anon AND owner (handler wiring of the fail-closed tree arm, not just the helper). - ipfs_cid_tree_served_when_no_path_scoped_rule: a tree CID serves 200 when no path-scoped rule exists (the skip-walk branch is not over-gating trees). - allowed_tree_set_includes_tree_shared_across_allowed_and_denied_paths: T2 content-dedup — a tree oid reachable at both an allowed and a withheld path is included (allowed-wins). - allowed_tree_set_includes_root_trees_of_all_reachable_commits: the batched root-tree pass returns every reachable commit's root, not just HEAD's.
#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.
beardthelion
force-pushed
the
fix/issue-135-ipfs-cid-tree-gate
branch
from
89d4928 to
7dec45c
Compare
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. |
… claim (#135) The test asserted a deadlock guard, but git log --stdin reads its whole revision list to EOF before emitting %T, so the naive write-then-drain form never deadlocks (verified by reverting the writer thread and running at N=40000, ~1.6 MiB each way). Rename and recomment to what it actually verifies: parity at scale plus a liveness watchdog. Test body unchanged.
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] Bound the new tree visibility walk on the public retrieval route
crates/gitlawb-node/src/api/ipfs.rs:173
Every request for a known tree CID in a repo with a path-scoped rule recomputes the complete allowed-tree set:rev-list, one recursivels-treeper reachable commit, and a root-tree pass. The memo only lasts for that one request, while/ipfs/{cid}is anonymous and the public pins index exposes valid CIDs. An unauthenticated caller can therefore repeat a tree-CID request and saturate the blocking pool/CPU with unbounded full-history work. Please add a bounded, revision-aware cross-request cache and/or a route-level work/rate limit before enabling this path. -
[P2] Try every object recorded for a content CID
crates/gitlawb-node/src/db/mod.rs:2188
The new index deliberately permits duplicate CIDs, but this unorderedLIMIT 1chooses only one mapped OID and the handler never tries another. CIDs here hash untyped raw object bytes, so a tree and a blob containing its raw tree bytes have different Git OIDs but the same CID; both can be pinned. If PostgreSQL chooses a withheld, stale, or absent object while another mapped object is readable, the endpoint returns 404 for a valid advertised CID. Return all matching OIDs and run each through the existing repository/visibility checks (with collision coverage) rather than selecting one arbitrarily. -
[P2] Canonicalize parsed CIDv1 values before the database lookup
crates/gitlawb-node/src/api/ipfs.rs:92
CidGeneric::from_straccepts valid CIDv1 multibase encodings, but this lookup uses the original request spelling. Pins are stored under the canonical base32 string produced byCid::from_git_object_bytes(...).to_string(), so an equivalent base58/base64 CID passes validation yet missespinned_cidsand returns 404. Look upcid.to_string()(or a binary canonical key) and add an alternate-encoding retrieval test.
|
Ready for re-review on
Ready for another look. |
…aps (#173) Resolve jatmn's three CHANGES_REQUESTED findings on GET /ipfs/{cid}, each verified by execution (revert -> RED, fix -> GREEN): - [P1] Rate-limit the full-history allowed-set walk per source IP, checked once right before the walk spawns (the resource sink), reusing the same RateLimiter and trusted-proxy key as the push brake. GITLAWB_IPFS_RATE_LIMIT (default 600/hr, 0 disables, bounded key map). A memo hit or a cheap non-path-scoped fetch is never braked; the key is the non-farmable client IP, not the DID. - [P2] Resolve a CID to every mapped oid (oids_for_cid) instead of LIMIT 1, and try each through the repo/visibility loop, so a withheld or absent duplicate no longer false-404s a CID that has a readable object. - [P2] Canonicalize the parsed CID (cid.to_string()) before the pinned_cids lookup so an equivalent base58/base64 spelling resolves to the canonical base32 key the pin path stores. Tests: ipfs_walk_rate_limited_per_source (shed, per-source isolation, and the must-not on a cheap non-walk fetch), oids_for_cid_returns_all_duplicates, ipfs_cid_collision_serves_readable_duplicate, ipfs_alt_encoding_cid_resolves. Full node suite green (502).
|
Addressed all three on [P1] Bound the tree-visibility walk. The full-history allowed-set walk is now rate-limited per source IP, checked once immediately before it spawns (the resource sink), reusing the same [P2] Try every object recorded for a content CID. [P2] Canonicalize CIDv1 before the lookup. The lookup keys on Full node suite green (502). |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/gitlawb-node/src/api/ipfs.rs (1)
143-159: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale invariant comment: "one request builds exactly one of the two sets."
The tree analog (
#135): a withheld subtree's tree object is gated the same way a withheld blob is, so its structure cannot leak by CID where get_tree protects it. The adjacent claim that Built lazily and only for a tree fetch (a request is one CID = one object type), so one request builds exactly one of the two sets — no double walk no longer holds: the new multi-candidate oid loop can resolve a single CID to both a blob and a tree oid in the same repo (the documented CID-collision case indb/mod.rs'soids_for_cid), which would populate bothallowed_blob_memoandallowed_tree_memowithin one request.Update the comment to reflect that both sets can now be built in the collision case.
🤖 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 - 159, Update the comment above allowed_blob_memo and allowed_tree_memo to remove the claim that one request builds exactly one set; state that the sets are built lazily and that both may be populated when a CID resolves to both blob and tree candidates.
🤖 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/ipfs.rs`:
- Around line 165-279: The walk_rate_checked guard in the per-object gating flow
only limits the first spawn_blocking walk, allowing subsequent repo/type walks
within one request. Replace the one-time check with per-request walk accounting
and enforce a cap before every allowed_blob_set_for_caller or
allowed_tree_set_for_caller invocation, rejecting or skipping once exhausted
while preserving the existing IP limiter check and fail-closed walk handling.
In `@crates/gitlawb-node/src/main.rs`:
- Around line 328-345: The periodic cleanup task must also invoke cleanup on
ipfs_rate_limiter. Update the cleanup loop to call its cleanup method alongside
the other rate limiters, ensuring expired source-IP entries are removed
regularly and the 200,000-key bound does not retain stale clients.
---
Outside diff comments:
In `@crates/gitlawb-node/src/api/ipfs.rs`:
- Around line 143-159: Update the comment above allowed_blob_memo and
allowed_tree_memo to remove the claim that one request builds exactly one set;
state that the sets are built lazily and that both may be populated when a CID
resolves to both blob and tree candidates.
🪄 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: 06879bbd-f899-4792-9943-5bc38b5533fa
📒 Files selected for processing (6)
crates/gitlawb-node/src/api/ipfs.rscrates/gitlawb-node/src/auth/mod.rscrates/gitlawb-node/src/db/mod.rscrates/gitlawb-node/src/main.rscrates/gitlawb-node/src/state.rscrates/gitlawb-node/src/test_support.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/gitlawb-node/src/test_support.rs
| for sha256_hex in &oids { | ||
| for repo in &repos { | ||
| // Repo-level read gate against THIS row's own rules (KTD2a). | ||
| let rules: &[crate::db::VisibilityRule] = rules_by_repo | ||
| .get(&repo.id) | ||
| .is_some_and(|set| set.contains(&sha256_hex)); | ||
| if !in_allowed { | ||
| .map(Vec::as_slice) | ||
| .unwrap_or(&[]); | ||
| if visibility_check(rules, repo.is_public, &repo.owner_did, caller, "/") | ||
| == Decision::Deny | ||
| { | ||
| continue; | ||
| } | ||
| } | ||
|
|
||
| // Now that we've passed the gate, read the content. | ||
| let content = match store::read_object_content(&repo_path, &sha256_hex, &obj_type) { | ||
| Ok(c) => c, | ||
| Err(e) => { | ||
| tracing::warn!(repo = %repo.name, err = %e, "error reading git object content"); | ||
| continue; | ||
| let repo_path = match state.repo_store.acquire(&repo.owner_did, &repo.name).await { | ||
| Ok(p) => p, | ||
| Err(_) => continue, | ||
| }; | ||
|
|
||
| // Check whether the object exists in this repo before any expensive | ||
| // reachability walk. This prevents random-CID spray from triggering | ||
| // full-history git walks on repos that don't carry the object. | ||
| let obj_type = match store::object_type(&repo_path, sha256_hex) { | ||
| Ok(Some(t)) => t, | ||
| Ok(None) => continue, | ||
| Err(e) => { | ||
| tracing::warn!(repo = %repo.name, err = %e, "error checking git object type"); | ||
| continue; | ||
| } | ||
| }; | ||
|
|
||
| // Per-object gating applies only when a path-scoped rule exists (KTD4); | ||
| // without one, the "/" gate above is the whole story. Under a path-scoped | ||
| // rule a `blob` is gated against the caller's allowed-blob-set and a `tree` | ||
| // against the allowed-tree-set (#135 — a withheld subtree's tree structure | ||
| // must not leak by CID where get_tree protects it). A `commit`/`tag` exposes | ||
| // only "/"-level metadata the caller already cleared the "/" gate for, so it | ||
| // falls through to serve. | ||
| let path_scoped = has_path_scoped_rule(rules); | ||
| if path_scoped && (obj_type == "blob" || obj_type == "tree") { | ||
| let is_blob = obj_type == "blob"; | ||
| let memo = if is_blob { | ||
| &mut allowed_blob_memo | ||
| } else { | ||
| &mut allowed_tree_memo | ||
| }; | ||
| if !memo.contains_key(&repo.id) { | ||
| // First actual walk of this request: brake it on the source IP | ||
| // once. A memo hit below never reaches here, so a same-request | ||
| // re-check is impossible. When no key can be resolved (e.g. a | ||
| // test `oneshot` with no peer and no trusted header) the limiter | ||
| // is simply skipped, exactly as the other IP brakes behave. | ||
| if !walk_rate_checked { | ||
| walk_rate_checked = true; | ||
| if let Some(key) = | ||
| crate::rate_limit::client_key(&headers, peer, state.push_limiter_trust) | ||
| { | ||
| if !state.ipfs_rate_limiter.check(&key).await { | ||
| return Err(AppError::TooManyRequests( | ||
| "ipfs retrieval rate limit exceeded — try again later".into(), | ||
| )); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| let rp = repo_path.clone(); | ||
| let r = rules.to_vec(); | ||
| let is_public = repo.is_public; | ||
| let owner = repo.owner_did.clone(); | ||
| let caller_for_walk = caller_owned.clone(); | ||
| // Full-history walk shells out to git — keep it off the async runtime. | ||
| // Only the fetched object's type is walked (blob XOR tree), so a tree | ||
| // fetch never pays the blob walk and vice-versa. | ||
| let walk = tokio::task::spawn_blocking(move || { | ||
| if is_blob { | ||
| allowed_blob_set_for_caller( | ||
| &rp, | ||
| &r, | ||
| is_public, | ||
| &owner, | ||
| caller_for_walk.as_deref(), | ||
| ) | ||
| } else { | ||
| allowed_tree_set_for_caller( | ||
| &rp, | ||
| &r, | ||
| is_public, | ||
| &owner, | ||
| caller_for_walk.as_deref(), | ||
| ) | ||
| } | ||
| }) | ||
| .await; | ||
| // Fail closed on EITHER a task panic (JoinError) or a walk error: | ||
| // we cannot prove the caller may read here, so skip this repo and | ||
| // let a public copy (if any) serve. Never serve on an unproven gate. | ||
| let set = match walk { | ||
| Ok(Ok(set)) => set, | ||
| Ok(Err(e)) => { | ||
| tracing::warn!(repo = %repo.name, err = %e, "allowed-set walk failed; skipping repo"); | ||
| continue; | ||
| } | ||
| Err(e) => { | ||
| tracing::warn!(repo = %repo.name, err = %e, "allowed-set walk task panicked; skipping repo"); | ||
| continue; | ||
| } | ||
| }; | ||
| memo.insert(repo.id.clone(), set); | ||
| } | ||
| let in_allowed = memo | ||
| .get(&repo.id) | ||
| .is_some_and(|set| set.contains(sha256_hex)); | ||
| if !in_allowed { | ||
| continue; | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and nearby helpers first.
ast-grep outline crates/gitlawb-node/src/api/ipfs.rs --view expanded || true
printf '\n---\n'
rg -n "walk_rate_checked|ipfs_rate_limiter|allowed_blob_set_for_caller|allowed_tree_set_for_caller|spawn_blocking|oids_for_cid|create_ip_rate_limiter|client_key" crates/gitlawb-node/src/api/ipfs.rs crates/gitlawb-node/src -g '!**/target/**'
# Show the relevant section of ipfs.rs with line numbers.
printf '\n=== ipfs.rs excerpt ===\n'
sed -n '120,320p' crates/gitlawb-node/src/api/ipfs.rsRepository: Gitlawb/node
Length of output: 19617
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the rate limiter and helper implementations referenced by ipfs.rs.
rg -n "struct .*RateLimiter|fn check\\(|create_ip_rate_limiter|ipfs_rate_limiter|client_key|allowed_blob_set_for_caller|allowed_tree_set_for_caller|has_path_scoped_rule|visibility_check|oids_for_cid" crates/gitlawb-node/src crates -g '!**/target/**'
# Narrow reads for the likely relevant definitions.
for f in \
crates/gitlawb-node/src/api/ipfs.rs \
crates/gitlawb-node/src/rate_limit.rs \
crates/gitlawb-node/src/api/mod.rs \
crates/gitlawb-node/src/lib.rs
do
if [ -f "$f" ]; then
echo
echo "=== $f ==="
wc -l "$f"
fi
doneRepository: Gitlawb/node
Length of output: 36690
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the exact region around the walk logic and the rate-limiter helper.
echo "=== crates/gitlawb-node/src/api/ipfs.rs (around the cited lines) ==="
nl -ba crates/gitlawb-node/src/api/ipfs.rs | sed -n '150,290p'
echo
echo "=== locate rate-limit helpers ==="
rg -n "client_key|ipfs_rate_limiter|walk_rate_checked|allowed_blob_set_for_caller|allowed_tree_set_for_caller|create_ip_rate_limiter|check\\(" crates/gitlawb-node/src -g '!**/target/**'Repository: Gitlawb/node
Length of output: 259
Cap full-history walks per request. walk_rate_checked only charges the first walk in a request; later repo/type misses still spawn their own spawn_blocking walk under the same /ipfs/{cid} hit. A single request can therefore fan out across many repos or duplicate blob/tree candidates while consuming just one ipfs_rate_limiter check. Consider a per-request walk cap or a cheap re-check before each walk.
🤖 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 165 - 279, The
walk_rate_checked guard in the per-object gating flow only limits the first
spawn_blocking walk, allowing subsequent repo/type walks within one request.
Replace the one-time check with per-request walk accounting and enforce a cap
before every allowed_blob_set_for_caller or allowed_tree_set_for_caller
invocation, rejecting or skipping once exhausted while preserving the existing
IP limiter check and fail-closed walk handling.
| // IPFS-walk flood brake: max `GET /ipfs/{cid}` full-history walks per client | ||
| // IP per hour. The route is anonymous and a valid tree CID makes every repeat | ||
| // request pay a fresh allowed-set walk (INV-10), so the walk (not the whole | ||
| // route) is braked on the resolved client IP. GITLAWB_IPFS_RATE_LIMIT | ||
| // overrides; 0 disables. Bounded key set — the key is a client-influenced IP. | ||
| let ipfs_limit = std::env::var("GITLAWB_IPFS_RATE_LIMIT") | ||
| .ok() | ||
| .and_then(|v| v.trim().parse::<usize>().ok()) | ||
| .unwrap_or(600); | ||
| let ipfs_rate_limiter = rate_limit::RateLimiter::new_bounded( | ||
| ipfs_limit, | ||
| std::time::Duration::from_secs(3600), | ||
| 200_000, | ||
| ); | ||
| if ipfs_limit == 0 { | ||
| tracing::warn!("GITLAWB_IPFS_RATE_LIMIT=0 — per-IP IPFS walk rate limiting disabled"); | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Clean up the IPFS limiter in the periodic cleanup task.
ipfs_rate_limiter is initialized here, but the cleanup loop at Lines 428-445 never calls cleanup() on it. Expired source-IP entries therefore remain retained until capacity pressure triggers an inline sweep, which can reject new clients after the 200,000-key cap is reached.
let push_rl = state.push_rate_limiter.clone();
+let ipfs_rl = state.ipfs_rate_limiter.clone();
...
push_rl.cleanup().await;
+ipfs_rl.cleanup().await;🤖 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/main.rs` around lines 328 - 345, The periodic cleanup
task must also invoke cleanup on ipfs_rate_limiter. Update the cleanup loop to
call its cleanup method alongside the other rate limiters, ensuring expired
source-IP entries are removed regularly and the 200,000-key bound does not
retain stale clients.
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