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 21 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.
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
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 every full-history walk, not just the first one in a request
crates/gitlawb-node/src/api/ipfs.rs:216
walk_rate_checkedconsumes one IP quota token and then remains true while the nested CID-candidate × repository loops continue. A shared pinned blob/tree CID can be present in any number of public path-scoped repositories (and a CID can intentionally have multiple OID candidates); when the caller is denied in each, the request starts oneallowed_*_set_for_callerfull-history walk per repository after paying for only the first. Thus 600 requests per IP can still cause 600 × Nrev-list/ls-treewalks and exhaust the blocking pool/CPU. Charge or cap every spawned walk (and add a multi-repository regression) rather than treating the first check as a request-wide authorization for unbounded work. -
[P1] Do not leave resolvable pinned CIDs on the unbounded all-repository probe path
crates/gitlawb-node/src/api/ipfs.rs:117
The newpinned_cidslookup makes the CIDs published by the unsigned pin index reach this loop, but the handler still acquires every readable repository and runs the synchronousgit cat-file -tprobe before it can determine which repository contains the OID. The IP limiter is only consulted later, after a path-scoped blob/tree is found, so a CID for an old, stale, or unscoped pin can repeatedly force O(repositories) local probes (and cold Tigris existence/download work) without consuming a quota token. This reactivates the availability problem tracked in #164 for the now-functional CID contract; associate pins with their owning repository/rows or apply a route-level bound before scanning, and move the blocking probe off the async runtime. -
[P2] Include the IPFS limiter in the periodic expiry sweep
crates/gitlawb-node/src/main.rs:428
The new limiter has the same client-controlled key space and one-hour window as the other bounded IP limiters, but the cleanup task clones and cleans only the older limiters.RateLimiter::checkperforms a global expiry sweep only when its map is already full, so a distributed request burst can retain up to 200,000 expired IP windows indefinitely during normal traffic. Clonestate.ipfs_rate_limiterhere and callcleanup()with the other limiter cleanup calls.
…er (#173) Two review findings on the /ipfs/{cid} retrieval path. Bound the full-history walk fan-out per request. The ipfs_rate_limiter check fires once per request, but within a single request the object can exist under path-scoped rules in many repos, and each distinct repo pays its own spawn_blocking allowed-set walk (the memo only dedups the same repo). One request could therefore fan out to O(repos) walks for a single rate-limiter token (INV-10). Add MAX_HISTORY_WALKS_PER_REQUEST (16): once that many walks have run, no further walk is spawned for the rest of the request. This also closes jatmn's open P1 ("bound every full-history walk, not just the first one"). The ceiling uses a plain break, not a whole-search break. The budget persists across the outer oid-candidate loop, so a later candidate servable WITHOUT a walk (a commit/tag, or a no-rule public copy) is still served, while any further walk it would need re-trips the guard and is skipped. Breaking the whole search would 404 that free candidate for no amplification benefit. Sweep the ipfs limiter in the periodic cleanup task. ipfs_rate_limiter was the one bounded limiter the 300s cleanup loop never called cleanup() on, so its map sat full of stale source-IP entries until an inline capacity sweep reclaimed them at the 200k cap. The six cleanup calls now live in AppState::sweep_rate_limiters, which the loop drives, so the set is testable and a new limiter has one place to be added. Tests (each RED->GREEN verified by execution): - ipfs_walk_fanout_capped_per_request: cap+1 deniers precede a readable copy; capped -> 404, neutralizing the break -> 200. - ipfs_walk_cap_still_serves_walk_free_candidate: a multi-oid CID whose blob candidate burns the budget still serves its walk-free commit candidate (200); a whole-search break -> 404. - sweep_rate_limiters_includes_ipfs_limiter: drives the sweep and asserts the ipfs limiter's expired entry is evicted; dropping its cleanup() -> entry survives.
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 quarantined repositories out of the CID search
crates/gitlawb-node/src/api/ipfs.rs:185
The new CID-to-OID lookup makes this route reach real pinned objects, but itslist_all_repos()scan includes quarantined mirror rows and the loop never applies the quarantine gate used by the normal serve/clone handlers. A root-readable quarantined mirror can therefore return an on-disk pinned object through/ipfs/{cid}, despite quarantine being explicitly defined as hidden from serve, clone, and listings. Filter the scan to non-quarantined rows (or select and skip the flag) and cover a quarantined CID with a 404 regression test. -
[P1] Do not serve unreachable commit and tag objects under path rules
crates/gitlawb-node/src/api/ipfs.rs:215
The new resolver also activates CIDs recorded by the full-scan pin path. That path deliberately includes dangling non-blob objects, while this handler only proves reachability for blobs and trees; commits and tags fall through after the root gate. Thus a dangling commit/tag in a path-scoped repository can be pinned and then served anonymously even though it has no authorized reachable path, exposing commit/tag messages and structural metadata. Apply a reachable authorization check to these types too (or exclude unreachable non-blobs from pinning) and add a dangling commit/tag denial test. -
[P1] Debit every full-history walk from the IPFS quota
crates/gitlawb-node/src/api/ipfs.rs:245
walk_rate_checkedcharges the source IP only before the first walk, yet the same request can launch sixteen full-historyspawn_blockingwalks. At the configured default of 600, one source can therefore induce up to 9,600 walks per hour, defeating the stated per-IP walk brake and allowing a known CID across path-scoped repositories to consume the blocking pool and CPU. Consume quota for each spawned walk (or explicitly account for the whole request's walk budget) instead of suppressing the later checks. -
[P2] Continue searching for a walk-free readable copy after the fan-out cap
crates/gitlawb-node/src/api/ipfs.rs:242
Reaching the 16-walk ceiling usesbreak, which exits the entire repository loop for the current OID. Becauselist_all_repos()is ordered by mutableupdated_at, newer path-scoped rows that deny a shared object can consume the budget before an older no-rule public copy is considered; the request then returns an opaque 404 for content that remains publicly readable. This is also the behavior asserted byipfs_walk_fanout_capped_per_request, despite the surrounding comment claiming a later walk-free public copy is still served. Skip only candidates that require another walk and continue scanning for cheap readable candidates. -
[P2] Sign
gl ipfs getrequests when an identity is available
crates/gitlawb-node/src/api/ipfs.rs:119
Resolving actual pin CIDs makes the endpoint usable for path-authorized objects, butgl ipfs getstill constructsNodeClientwithNone, exposes no identity directory, and always calls the unsignedget. Owners and listed readers therefore receive the opaque anonymous 404 for every private/path-scoped object that the CLI can now resolve publicly. GiveGetthe same identity loading and signed-read path asList(or explicitly restrict and document it as public-only).
The CID resolver loop gated only on visibility, and list_all_repos() does not
filter quarantined mirror rows, so a public quarantined mirror served its pinned
objects via GET /ipfs/{cid} despite quarantine being hidden from serve/clone/
listings. Prefetch the quarantined ids and skip them in the loop, before the
visibility check so the mirror's own owner also 404s.
RED->GREEN: ipfs_cid_quarantined_repo_withheld_from_anon_and_owner serves 200
(anon+owner) without the skip, 404 with it; baseline pre-quarantine 200 confirms
the object is otherwise servable.
…173, F3+F4) F3: the per-IP walk brake charged one token per REQUEST (a walk_rate_checked latch), while a request spawns up to MAX_HISTORY_WALKS_PER_REQUEST walks, so one IP could drive 16x its quota of full-history walks. Debit one token per spawned walk and drop the latch; a memo hit or walk-free candidate is still never charged. F4: hitting the walk cap used a plain break that exited the repo loop for the current oid, abandoning a walk-free readable copy (list_all_repos is ORDER BY updated_at DESC, so a newer path-scoped denier can precede an older no-rule public copy) and returning an opaque 404 for publicly-readable content. Use continue to skip only the walk-requiring candidate and keep scanning; walks is incremented only inside the walk block, so the amplification bound is unchanged. RED->GREEN: ipfs_walk_quota_debited_per_walk (one request, quota 1, 2 deniers -> 429 on the 2nd walk; was 404). ipfs_walk_fanout_capped_per_request flipped from 404 to 200 + x-git-hash == the blob served from the no-rule public copy. Existing ipfs_walk_rate_limited_per_source and ipfs_walk_cap_still_serves_walk_free_candidate stay green.
The /ipfs/{cid} resolver now serves path-scoped objects to authorized readers,
but `gl ipfs get` built NodeClient with None and always sent an unsigned request,
so an owner or listed reader received the opaque anonymous 404 for content they
can read. Add a --dir identity arg (like `list`), load the keypair best-effort,
and use get_authed (signs iff a keypair is present, unsigned fallback keeps public
reads working).
RED->GREEN: test_cmd_get_signs_when_identity_present drives a signature-matching
mock — 501 (unmatched) while unsigned, 200 once signed. test_cmd_get_anonymous_
denial_is_error guards the must-not: a 404 surfaces as an error, not masked success.
…, F2) The full-scan pin path deliberately pins dangling (unreachable) non-blob objects, but the CID resolver only proved reachability for blobs and trees; commit and tag objects fell through to serve. A dangling commit/tag in a path-scoped repo could therefore be served anonymously by CID, leaking its message and structural metadata. Add reachable_commit_tag_oids (reachable commits via rev-list --all UNION annotated-tag objects at refs) and gate commit/tag under a path-scoped rule against it, exactly as blob/tree are gated against their allowed-sets. The three walks are unified into one cap+per-walk-quota path, so commit/tag reachability walks share the fan-out ceiling and IP quota and cannot bypass them (R6). RED->GREEN: ipfs_cid_dangling_commit_and_tag_fail_closed_under_path_rules (a dangling commit AND annotated tag, sentinel messages, must 404 for anon+owner with no leak; served 200 + sentinel before the fix). Reachable commit/tag still serve: ipfs_cid_gate_withholds_blob_from_unauthorized stays green. ipfs_walk_commit_tag_ candidate_respects_the_walk_cap (was ipfs_walk_cap_still_serves_walk_free_candidate) proves commit/tag walks respect the cap. Full node (508) + gl (268) suites green.
…173) Addresses the code-review findings on the F2 reachability gate: - P2: reachable_commit_tag_oids routed through reachable_commits, which runs assert_all_refs_are_commits and bails when any ref peels to a non-commit (an annotated tag of a tree is pushable through receive-pack). That fail-closed the whole repo, 404ing every reachable commit/tag CID for a legitimate reader. Decouple: enumerate reachable commits with a bare rev-list --all (+ HEAD) and no ref-commit assertion. A dangling object is still absent from rev-list and the ref walk, so no dangling object is admitted (no leak) — only availability recovered. - P3: for-each-ref only lists ref tips, so a nested tag-of-a-tag's inner tag object (reachable via the outer ref tag, and pinnable) was omitted and its CID 404'd. Peel each tag's chain so every reachable tag object is included. - P3: spell out the commit|tag match arms (memo select + walk dispatch) with an unreachable! default so a future added gated type fails loud instead of silently routing to the reachable-commit/tag set. RED->GREEN: ipfs_cid_reachable_commit_served_despite_non_commit_ref (reachable commit 404'd by the guard bail, now 200). ipfs_cid_nested_tag_inner_object_served (inner tag 404'd without the peel loop, now 200; RED confirmed by neutering the peel). Dangling commit/tag still 404 (ipfs_cid_dangling_commit_and_tag...). Full node suite 510 green, fmt + clippy clean.
|
All five addressed on Quarantine (F1). The resolver loop gated only on visibility, and Dangling commit/tag (F2). The per-object gate only covered blob/tree; a dangling commit/tag under a path rule fell through to serve, leaking its message. Added Two design notes on this one. First, commit/tag are now walk-gated, so a shared reachable commit behind more than the cap of path-scoped deniers with the budget already spent will 404 rather than spawn an unbounded number of walks; that is the fan-out ceiling applying to commit/tag too. Second, the commit/tag reachability deliberately does not run the Per-walk quota (F3). The IP brake charged one token per request while a request spawns up to 16 walks. Now debits per spawned walk; a memo hit or walk-free candidate is still never charged. Fan-out (F4). The cap used a plain
Each fix is RED->GREEN with the guard reverted to confirm it is load-bearing. |
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 new reachability tests independent of the runner's Git identity
crates/gitlawb-node/src/test_support.rs:2814
Thecommit-treefixture and the annotatedgit tag -afixture both rely on Git's ambient author/committer configuration. The beta and stable CI jobs therefore fail at these two new tests withAuthor identity unknown/Committer identity unknownbefore exercising the handler (509 passed, 2 failed). Set the identity explicitly for these fixture commands (or in the isolated test repository) so the required suite is portable and green. -
[P1] Bound the CID resolver before it probes every repository
crates/gitlawb-node/src/api/ipfs.rs:204
Resolving a real pinned CID now scans every root-visible repository and callsrepo_store.acquire()plus the synchronousgit cat-file -tprobe before reaching the new limiter. A stale/absent CID from the public pins index, or a CID whose copies are in unscoped repositories, therefore bypasses the limiter and can repeatedly trigger O(all repos) blocking subprocesses; cold repositories additionally cause Tigris downloads and disk writes. Apply a route-level budget/rate limit before the scan (and move the blocking probe off the async worker), or retain repository provenance with the pin so this public endpoint does not fan out across the node. -
[P2] Preserve reachable annotated tags when
HEADis detached at one
crates/gitlawb-node/src/git/visibility_pack.rs:213
The new commit/tag set includesHEADinrev-list, which only yields the peeled commit, but seeds tag collection exclusively fromfor-each-ref. A bare repository can have detachedHEADpointing at an annotated tag with no ref at that tag; that tag is reachable and pinnable, yet never entersworklist, so its CID incorrectly 404s for an authorized reader under a path-scoped rule. Treat a tag-valuedHEADas an additional tag-chain seed and cover the detached-head case. -
[P2] Do not make a valid tag-of-tree disable all allowed tree reads
crates/gitlawb-node/src/git/visibility_pack.rs:142
allowed_tree_set_for_callerstill goes throughreachable_commits, whoseassert_all_refs_are_commitsrejects an annotated tag pointing at a tree. Such tags are valid (the new commit/tag path explicitly handles them), but the resulting walk error makes every tree CID in that repository—including the root and public subtrees—fall through to 404 for its owner and readers. Compute the tree reachability set without rejecting unrelated non-commit tag refs, while retaining a fail-closed policy for objects whose visibility cannot be established. -
[P2] Do not spend the walk ceiling before checking a later allowed scoped copy
crates/gitlawb-node/src/api/ipfs.rs:279
The global 16-walk budget is consumed by each newer path-scoped repository containing the same CID. After 16 deny paths, a later repository where the caller is actually allowed also needs an allowed-set walk, but this branch skips it and returns an opaque 404. Since repositories are ordered byupdated_at, a user can arrange the deny copies ahead of the authorized one; the current test only covers a later no-rule copy. Preserve the resource bound without turning an allowed path-scoped CID into a false not-found response. -
[P2] Continue scanning when an earlier scoped duplicate has exhausted the IP bucket
crates/gitlawb-node/src/api/ipfs.rs:294
Returning 429 immediately for the first walk that exceeds the per-IP quota prevents the resolver from reaching a later unscoped public copy of the same content, even though that copy needs no walk. A newer scoped duplicate can thus make an otherwise ordinary public CID retrieval fail solely due to repository order. Skip the walk-requiring candidate (or otherwise separate the quota decision from walk-free candidates) and retain the existing protection for expensive work. -
[P2] Do not silently discard an explicitly selected identity
crates/gl/src/ipfs_cmd.rs:98
gl ipfs get --dir <path>converts a missing, unreadable, or corruptidentity.pemintoNoneand sends an anonymous request. For a path-scoped object the authorized user then receives the endpoint's opaque 404 instead of the actionable key-load error, defeating the new signed-read behavior;gl ipfs listcorrectly propagates this same error. Keep unsigned fallback for intentionally anonymous use, but propagate failures for an explicit--dirand add coverage for it.
The commit-tree and git tag -a fixtures for the /ipfs/{cid} reachability
tests run directly in a bare clone, which does not inherit the source
repo's local identity. On a CI runner with no ambient identity both abort
with Author/Committer identity unknown before the handler runs (the
test (stable)/(beta) failures). Configure user.email/user.name on each
bare clone at the seeding seam.
The resolver's per-repo existence probe shells out to git cat-file -t but ran directly on the async worker, so a scan across many repos ties up the runtime a subprocess at a time. Offload it via spawn_blocking, matching the reachability walk. Output is unchanged (covered by the existing resolver suite). This addresses the async-worker half of the review finding; bounding the per-request repo fan-out cleanly needs pin provenance and is tracked as a follow-up (the route-level rate-limit alternative conflicts with the walk-only brake invariant in ipfs_walk_rate_limited_per_source).
allowed_tree_set_for_caller -> tree_paths -> reachable_commits ran
assert_all_refs_are_commits, which bails the whole walk when any ref peels
to a non-commit. A pushable annotated tag pointing at a tree (git tag -a
<tree>, accepted by receive-pack) therefore fail-closed 404'd every tree
CID in the repo -- root and public subtrees included -- for its owner and
readers, not just the offending tag.
Split the enumeration: reachable_commit_oids (no guard) for the tree path,
which feeds ONLY the /ipfs/{cid} tree gate where absence = fail-closed 404,
so a tolerant walk over-withholds and never leaks. blob_paths keeps the
strict reachable_commits because it also feeds the serve/replication filters
(withheld_blob_oids / replicable_objects), where a missed reachable object
under-withholds. rev-list --all skips the tag-of-tree cleanly, so the
reachable-commit set stays complete; a tree reachable only via such a tag is
correctly excluded.
…g the request The per-IP walk quota returned 429 on the first walk-requiring candidate that exhausted the source IP's bucket, ending the whole request. A later walk-free readable copy of the same CID (a no-rule public copy) was never reached, so a public CID 404d/429d solely because a newer path-scoped duplicate sorts ahead of it under updated_at DESC. Check the quota BEFORE spending walk budget; on exhaustion set a throttled flag and continue (skip only this walk-requiring candidate), mirroring the walk-cap continue. Return 429 from the fall-through only when nothing was servable AND a candidate was actually throttled, else the existing 404. The brake still bites (ipfs_walk_rate_limited_per_source stays green: a lone throttled candidate still 429s) and each walk is still debited per walk.
gl ipfs get loaded the keypair with .ok(), so an explicit --dir pointing at a missing/corrupt keystore was silently converted to an anonymous request. An authorized reader then got the node's opaque 404 for a path-scoped object instead of the actionable 'Run gl identity new' error that gl ipfs list already surfaces. Branch on dir: an explicit --dir propagates the load error with ?; only the default (no --dir) keeps the best-effort unsigned fallback so get stays usable for genuinely public content.
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 repository probes before the walk limiter
crates/gitlawb-node/src/api/ipfs.rs:231
oids_for_cidnow makes real pinned CIDs reach this loop, but every request still callsrepo_store.acquireand spawnsgit cat-file -tfor every root-visible repository before eitherMAX_HISTORY_WALKS_PER_REQUESToripfs_rate_limiteris checked. A caller can obtain a valid CID from the public pins endpoint and repeatedly force O(repository-count) subprocesses; on a cold cache,acquirealso performs Tigris existence/download work. CIDs found only in no-rule repositories never debit the new limiter at all. Please retain pin-to-repository provenance (or otherwise bound and pre-limit the probe phase) so the new CID resolver does not expose an unbounded anonymous fan-out. -
[P2] Do not turn an authorized path-scoped copy into a 404 at the walk cap
crates/gitlawb-node/src/api/ipfs.rs:302
After 16 earlier matching path-scoped repositories consume the walk budget, thiscontinueskips every later matching repository whose visibility still needs an allow-set calculation. Sincelist_all_reposorders by mutableupdated_at, the first 16 can deny a caller while a later path-scoped copy authorizes that caller for the same object/CID; the handler then returns the opaque 404 even though the object is readable. The current regression only preserves a later walk-free/no-rule copy. Keep the work bound, but arrange candidate selection/provenance or an explicit bounded-result response so an authorized scoped copy is not silently reported as absent. -
[P2] Include annotated tags reachable only through detached HEAD
crates/gitlawb-node/src/git/visibility_pack.rs:229
The tag worklist is seeded only fromfor-each-ref. A bare repository may have a detached/directHEADnaming an annotated tag with no remaining ref:rev-list --all HEAD(used above) peels that tag to its commit, whilefor-each-refcontains no tag row, so the tag object is omitted fromreachable_commit_tag_oids. Its pinned CID is consequently rejected under a path-scoped rule even for an authorized reader. Seed a tag-valued HEAD (and its tag chain) into this walk, and cover that detached-HEAD case.
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