fix(node): close quarantine bypass on encrypted blobs and CID serve - #276
fix(node): close quarantine bypass on encrypted blobs and CID serve#276Ayush7614 wants to merge 3 commits into
Conversation
Route encrypted discovery/fetch/replicate through authorize_repo_read so
quarantined mirrors are opaque 404s. Exclude quarantined rows from
list_all_repos so GET /ipfs/{cid} cannot serve their objects.
|
Warning Review limit reached
Next review available in: 47 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughRepository quarantine is now enforced consistently across encrypted-blob, visibility, repository enumeration, and IPFS CID endpoints. Shared authorization returns opaque 404 responses, repository listing excludes quarantined rows, and IPFS serving filters mirror twins of quarantined logical repositories. ChangesQuarantine-aware repository access
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Caller
participant RepositoryRoute
participant authorize_repo_read
participant RepositoryDatabase
Caller->>RepositoryRoute: request repository resource
RepositoryRoute->>authorize_repo_read: authorize read
authorize_repo_read->>RepositoryDatabase: load repo and quarantine state
RepositoryDatabase-->>authorize_repo_read: record and rules
authorize_repo_read-->>RepositoryRoute: authorized record or opaque 404
RepositoryRoute-->>Caller: resource response
sequenceDiagram
participant Caller
participant get_by_cid
participant RepositoryDatabase
participant ObjectStore
Caller->>get_by_cid: request CID
get_by_cid->>RepositoryDatabase: fetch quarantined repositories
RepositoryDatabase-->>get_by_cid: quarantine rows
get_by_cid->>RepositoryDatabase: inspect candidate repositories
get_by_cid->>ObjectStore: check object type
ObjectStore-->>get_by_cid: object result
get_by_cid-->>Caller: content or opaque 404
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
the
needs-tests
|
Thanks for the contribution. A couple of things will help us review this faster:
See CONTRIBUTING.md. Update the PR and these notes will clear automatically. |
beardthelion
added
crate:node
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
The two production changes are correct and both are load-bearing. I reverted each one and watched the matching test go red: dropping the WHERE quarantined = FALSE makes get_by_cid_skips_quarantined_public_repo serve the blob (200, public bytes), and restoring the old encrypted.rs gate makes the discovery route hand back the blob index. The ordering the fix depends on is right too, since authorize_repo_read checks quarantine at api/mod.rs:54 before visibility_check at :58, and I confirmed by driving the handler that a quarantined repo now 404s for its own owner in both the full did:key: and bare-key forms.
What holds the round is the route next door.
Findings
-
[P1] Gate
withheld-pathson quarantine as well
crates/gitlawb-node/src/api/visibility.rs:213
This handler is mounted in the same anonymously-reachable group as the three routes you fixed (server.rs:430-449) and still does the bareget_repoplusvisibility_checkthat the PR removed fromencrypted.rs. I drove it against a public-but-quarantined repo: it returns 200 with{"withheld":["/secret/**"],"reinclude":[]}whileencrypted-blobson that same repo correctly returns 404. So an anonymous caller still learns the repo is admitted here and learns its private-subtree layout, and because the owner short-circuit sits insidevisibility_check, the row's claimed owner gets the same answer. It also has a cross-node consequence: a peer'swithheld-pathsanswer picks the mirror mode insync.rs. Swapping the resolve forlet (record, rules) = crate::api::authorize_repo_read(&state, &owner, &repo, caller, "/").await?;is the same shape as yourencrypted.rschange; I applied exactly that and ran it, and the quarantined request became 404 while a released repo still returns its globs. -
[P2] Add an owner-caller regression in both DID forms
crates/gitlawb-node/src/test_support.rs:4233,:4290
Both new tests drive onlyanon_get/cid_anon. The code is right, and I verified that separately, but an anonymous pass cannot distinguish "dropped before the gate" from "denied by the gate", and those come apart the moment someone moves the quarantine check belowvisibility_checkor turns the SQL predicate into an in-loop skip after the visibility branch. The invariant this PR implements is that a quarantined repo is withheld from every reader including its owner, and the tree already has that shape for the feed collector infeed_quarantined_mirror_withheld_from_ownerandfeed_quarantined_full_did_repo_withheld_from_owner. Mirror those here: one owner-caller case per surface, in the fulldid:key:and bare-key forms. -
[P2] Gate
list_visibilityon quarantine
crates/gitlawb-node/src/api/visibility.rs:174
Same bareget_repofollowed byrequire_owner, no quarantine drop, so the caller matching the row'sowner_didgets 200 with every rule'spath_glob,mode,reader_dids, andcreated_by. On a quarantined mirror thatowner_didcame off the wire with the row, so "the owner" here is whoever asserted the DID. Narrower reach than the one above since it needs that DID, which is why it is not P1, but it is the same invariant and the same one-line fix. -
[P3] Extend the release control to all three encrypted routes
crates/gitlawb-node/src/test_support.rs:4272
The deny loop coversencrypted-blobs,encrypted-blobs/replicate, andencrypted-blob/{oid}, but the release control only re-requests the first. The other two have no must-not-over-drop side, so a change that 404s them even after release would still pass. Worth noting for the third leg specifically: I reordered the loop and confirmed that on revert it fails at 500 rather than 200, because the handler gets past the gate and then dies inipfs_pin::cat. It detects the regression, it just is not demonstrating the leak its name claims.
One process note, not a finding: the body ticks "CI test (stable) with Postgres", but the suite has not run on this head. The workflow is sitting at action_required awaiting approval, which is the normal shape for a fork PR. I approved it so it runs. I also ran the quarantine suite locally against your head and it is 16 green.
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.
What checks out
The two production changes in this diff are correct and load-bearing for the single physical row cases they target:
- Routing the three encrypted handlers through
authorize_repo_readcloses the quarantine bypass that used bareget_repo+visibility_check. I reverted this hunk locally and the new deny test goes red. - Adding
WHERE quarantined = FALSEtolist_all_reposcloses the CID serve bypass for a quarantined public row with no unquarantined sibling. I reverted this hunk locally andget_by_cid_skips_quarantined_public_reposerves the blob again.
CI on head bcc7fb4 is green (fmt + clippy, test (stable), MSRV, cargo audit, Docker smoke). None of the findings below are regressions this PR introduced; they are sibling gaps, an incomplete multi-row closure on the CID path, or test holes.
Findings
-
[P1] Gate
withheld-pathson quarantine (pre-existing sibling gap)
crates/gitlawb-node/src/api/visibility.rs:208
This handler is mounted beside the three routes you fixed (server.rs:430-449) and still uses the oldencrypted.rspattern: bareget_repoplusvisibility_check, with nois_repo_quarantined/authorize_repo_readshort-circuit. On a public-but-quarantined repo,visibility_checkallows"/"for anonymous callers when there is no matching deny rule, so the handler returns 200 withwithheld/reincludeinstead of the opaque 404 thatauthorize_repo_readwould return. That discloses both admission and private-subtree layout. The owner short-circuit insidevisibility_check(visibility.rs:79-83) means the row's claimed owner gets the same answer. Downstream,sync.rs:245-256andglclone'sfetch_withhelduse this response to choose promisor mirror mode and sparse-clone withhold sets, so the leak is not local to discovery. Please resolve throughauthorize_repo_readthe same wayencrypted.rsnow does (or an equivalent quarantine-first gate). -
[P1]
get_by_cidstill serves via an unquarantined mirror twin while slug routes 404 (incomplete CID fix / comment overclaim)
crates/gitlawb-node/src/api/ipfs.rs:81-116,crates/gitlawb-node/src/db/mod.rs:1234-1238
The newlist_all_reposfilter drops only rows withquarantined = TRUEon that physical row. When a node holds both a quarantined canonical row (UUIDid) and a non-quarantined mirror row ({owner}/{name}) for the same logical repo — a shape the tree explicitly supports (get_repo_prefers_canonical_over_mirror,authorize_repo_read_denies_private_canonical_even_with_public_mirror; mirror upsert setsquarantined = falsewheneverget_repoalready finds a canonical row atsync.rs:290-309) — slug endpoints 404 throughauthorize_repo_readbecauseget_repoprefers the quarantined canonical, butget_by_cidstill iterates the surviving mirror row, passesvisibility_check(mirror is hardcoded public with no rules), and serves the object. That contradicts the comment claiming the same fail-closed posture asauthorize_repo_read(ipfs.rs:43-44). The feed collector already guards this class by consultinglist_quarantined_reposand dropping any row naming a quarantined logical repo (events.rs:66-103);get_by_cidneeds the same logical-repo quarantine fold before serving. This is not the separately tracked stale-public-mirror bypass (#124, explicitly deferred atipfs.rs:55-57). -
[P2] Gate
list_visibilityon quarantine (pre-existing sibling gap)
crates/gitlawb-node/src/api/visibility.rs:174
This owner-authenticated route still doesget_repo+require_ownerwith no quarantine drop. A caller matching the row'sowner_didgets 200 with every rule'spath_glob,mode,reader_dids, andcreated_byeven while the mirror is quarantined. Narrower reach thanwithheld-pathsbecause it needs that DID, but it violates the same "withheld from every reader" invariant stated inevents.rs:58-65. Same one-line shape as theencrypted.rsfix. -
[P2] Add owner-caller regression tests for the new surfaces (test gap)
crates/gitlawb-node/src/test_support.rs:4233,:4290
Both new HTTP tests drive only anonymous callers (anon_get/cid_anon). That cannot distinguish "quarantine checked before visibility" from "denied later by visibility", and those diverge if someone moves the quarantine check belowvisibility_checkor relies only on the SQL filter inlist_all_repos. The tree already has owner-withheld coverage for feeds (feed_quarantined_mirror_withheld_from_owner,feed_quarantined_full_did_repo_withheld_from_owner). Please mirror that here: one owner-caller case per surface (encrypted discovery and CID serve), in both bare-key and fulldid:key:owner forms. -
[P3] Extend the encrypted release control to all three routes (test gap)
crates/gitlawb-node/src/test_support.rs:4272
The deny loop coversencrypted-blobs,encrypted-blobs/replicate, andencrypted-blob/{oid}, but the post-release control only re-requestsencrypted-blobs. A regression that kept replicate/get at 404 after release would still pass. Please assert 200 (or the appropriate success shape) on all three afterset_repo_quarantine(..., false). -
[P3] Exercise mirror-admission row shape and dual-row CID deny in HTTP tests (test gap)
crates/gitlawb-node/src/test_support.rs:4233,:4290;crates/gitlawb-node/src/db/mod.rs:4623
The new integration tests quarantine viacreate_repo+set_repo_quarantineon a UUIDid. Production mirror admission usesupsert_mirror_repowith a slash-formid(owner/name) and setsquarantinedonly on first insert. The DB unit testlist_all_repos_excludes_quarantinedcovers the SQL filter for mirror rows only. No HTTP test drives the admission path or the canonical-quarantined / mirror-unquarantined coexistence case that the P1 CID finding above depends on.
Reviewer follow-up
beardthelion's CHANGES_REQUESTED review on head bcc7fb4 covers the withheld-paths gate, list_visibility gate, owner-caller tests, and partial release control. None of those are addressed on the current head. Reconciliation also surfaced the dual-row get_by_cid bypass above, which is in scope for this PR's CID serve change and comment but was not in that review.
Out of scope for this round
The following sibling surfaces were inspected and are real, but tracked or deferred separately and not counted as blockers on this PR: stale-public mirror CID serve (#124, ipfs.rs:55-57), ungated global list_pins / arweave anchors (#121), GraphQL/REST listing when a mirror row survives dedup, and owner mutations (merge/close/webhooks) on quarantined repos.
Gate withheld-paths and list_visibility through authorize_repo_read. Fold logical-repo quarantine into get_by_cid so a public mirror twin cannot serve while its canonical is quarantined. Expand owner, release, mirror-admission, and dual-row coverage.
|
@beardthelion @jatmn addressed the review findings on this head: P1
P2 P3 Ready for another look. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/gitlawb-node/src/api/visibility.rs (1)
169-198: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDuplicate
list_visibility_rulesquery inlist_visibility.
authorize_repo_readalready fetches and returns the repo's visibility rules, but they're discarded via_rulesand then re-fetched identically on Line 180 (state.db.list_visibility_rules(&record.id)). Reuse the tuple's rules instead of querying twice.♻️ Proposed fix
- let (record, _rules) = + let (record, rules) = crate::api::authorize_repo_read(&state, &owner, &repo, Some(&auth.0), "/").await?; require_owner(&record, &auth.0)?; - let rules = state.db.list_visibility_rules(&record.id).await?; let rules_json: Vec<_> = rules🤖 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/visibility.rs` around lines 169 - 198, Reuse the visibility rules returned by authorize_repo_read in list_visibility instead of discarding them and calling state.db.list_visibility_rules again. Rename the tuple binding from _rules to rules and keep the existing serialization and response behavior unchanged.
🤖 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 89-98: Restrict the CID quarantine filter around quarantined and
repo candidate matching to quarantined mirror rows, so a quarantined mirror
cannot suppress a healthy canonical repository with the same owner/name. Update
the `quarantined.iter().any(...)` logic and preserve consistency with
`authorize_repo_read`; alternatively, if retaining whole-logical-repo
suppression, align the related gated routes and comments/tests accordingly.
---
Nitpick comments:
In `@crates/gitlawb-node/src/api/visibility.rs`:
- Around line 169-198: Reuse the visibility rules returned by
authorize_repo_read in list_visibility instead of discarding them and calling
state.db.list_visibility_rules again. Rename the tuple binding from _rules to
rules and keep the existing serialization and response behavior unchanged.
🪄 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 Plus
Run ID: a209c5b1-c4ab-42c6-afea-05fd43666349
📒 Files selected for processing (5)
crates/gitlawb-node/src/api/encrypted.rscrates/gitlawb-node/src/api/ipfs.rscrates/gitlawb-node/src/api/visibility.rscrates/gitlawb-node/src/db/mod.rscrates/gitlawb-node/src/test_support.rs
Only drop CID candidates when a quarantined canonical (UUID) twin exists, so a quarantined mirror cannot suppress a healthy canonical. Reuse authorize_repo_read rules in list_visibility (no double fetch).
|
@coderabbitai addressed your review on
@beardthelion @jatmn FYI — this tightens the dual-row fold from the prior round without changing the quarantined-canonical + healthy-mirror deny case. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
ast-grep outline crates/gitlawb-node/src/api/ipfs.rs --items all --type function --match 'get_by_cid'
ast-grep outline crates/gitlawb-node/src/api/visibility.rs --items all --type function --match 'list_visibility'
rg -n -C 5 --type rust \
'get_by_cid_serves_canonical_when_only_mirror_quarantined|list_quarantined_repos|list_visibility|list_visibility_rules|authorize_repo_read' \
crates/gitlawb-node/src/api/ipfs.rs \
crates/gitlawb-node/src/api/visibility.rs \
crates/gitlawb-node/src/test_support.rsLength of output: 15232 🏁 Script executed: #!/bin/bash
set -euo pipefail
sed -n '62,165p' crates/gitlawb-node/src/api/ipfs.rs
sed -n '4580,4655p' crates/gitlawb-node/src/test_support.rsLength of output: 7676
The original findings are addressed. 🐇 ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
3 participants
Summary
list/get/replicate) usedvisibility_checkalone after a bareget_repo, skipping the quarantine short-circuit inauthorize_repo_read. A public-but-quarantined mirror still exposed its encrypted blob index (and would serve envelopes).GET /ipfs/{cid}iterateslist_all_repos(), which previously had noquarantined = FALSEfilter (unlike listing/dedup paths). Visibility alone admitted quarantined public rows, so objects remained reachable by CID.authorize_repo_read;list_all_reposexcludes quarantined rows (its only caller is the CID serve path).Why (direct PR)
Real quarantine invariant break on two surfaces. Not covered by existing issues/PRs (#110/#126 are visibility/CID gating; #135 is withheld-subtree trees — different).
Test plan
encrypted_blobs_quarantined_repo_opaque_404— discovery + replicate + get 404 while quarantined; no CID/oid leak; released repo lists againget_by_cid_skips_quarantined_public_repo— quarantined public repo 404s CID; released serves contentlist_all_repos_excludes_quarantined— DB enumeration skips sick mirrors-D warningscleantest (stable)with PostgresSummary by CodeRabbit
Bug Fixes
Tests