fix(node): gate /ipfs/pins and /arweave/anchors behind authentication (#121)#134
fix(node): gate /ipfs/pins and /arweave/anchors behind authentication (#121)#134Gravirei wants to merge 60 commits into
Conversation
Gitlawb#126) The IPFS visibility gate used withheld_blob_oids (a deny-set enumerating only reachable blobs), so a dangling/unreachable blob was absent from the set and served in cleartext to anonymous callers. Flip to an allowed-set (allowed_blob_set_for_caller) that enumerates reachable blobs the caller may read: a dangling blob has no path, is never in the set, and 404s.
Move store::read_object before the allowed_blob_set_for_caller spawn_blocking call so random-CID spray against repos with path-scoped rules cannot trigger full-history git walks on repos that don't carry the object.
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. |
|
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:
📝 WalkthroughWalkthroughAdds optional-signature middleware for ChangesAuth Gating for Metadata Index Endpoints
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
186-203: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftAvoid exposing the node-wide pin index to every signed DID.
This blocks anonymous callers, but any authenticated DID still reaches
list_pinned_cids()and receives the full node-wide CID index. If authenticated users are not all node-wide admins, private-repo CIDs remain enumerable. Gate this with a real node-wide permission or make pins repo-scoped and authorize repo read before returning entries.🤖 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 186 - 203, The current auth check in list_pins only blocks anonymous callers, but still lets any authenticated DID reach list_pinned_cids() and see the full node-wide pin index. Update list_pins to enforce a real node-wide permission using the authenticated caller from AuthenticatedDid before returning pins, or change the data model so pins are repo-scoped and authorize repo read access per caller. Keep the existing unauthorized path for unauthenticated requests, and ensure the final result only includes entries the caller is allowed to see.
🤖 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/arweave.rs`:
- Around line 45-57: The global anchor listing path in the arweave handler still
exposes private repo metadata because it only checks that a caller exists before
calling list_arweave_anchors(None, limit). Update the handler in the arweave API
to enforce per-repo read visibility for the no-repo case, or require q.repo to
be present, or gate global listing behind a node-admin capability. Use the
existing caller check and the list_arweave_anchors call site to locate the fix.
- Around line 52-55: The Arweave anchor query is only capping the upper bound in
the handler, so negative `q.limit` values can still reach the database and fail
in `list_arweave_anchors`. Update the limit handling in `arweave.rs` to clamp
the request value to a minimum of zero and a maximum of 200 before passing it
into `state.db.list_arweave_anchors`, keeping the existing `q.repo.as_deref()`
flow unchanged.
---
Outside diff comments:
In `@crates/gitlawb-node/src/api/ipfs.rs`:
- Around line 186-203: The current auth check in list_pins only blocks anonymous
callers, but still lets any authenticated DID reach list_pinned_cids() and see
the full node-wide pin index. Update list_pins to enforce a real node-wide
permission using the authenticated caller from AuthenticatedDid before returning
pins, or change the data model so pins are repo-scoped and authorize repo read
access per caller. Keep the existing unauthorized path for unauthenticated
requests, and ensure the final result only includes entries the caller is
allowed to see.
🪄 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: f9fd5b6a-451e-4f4f-a04b-f97da029b822
📒 Files selected for processing (3)
crates/gitlawb-node/src/api/arweave.rscrates/gitlawb-node/src/api/ipfs.rscrates/gitlawb-node/src/server.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/gitlawb-node/src/test_support.rs (1)
1968-2017: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a repo-scoped public success case and assert the filter actually narrows the result set.
These tests only exercise
?repo=against a single private repo with a single anchor. That leaves two intended contracts unpinned: anonymous access to a public repo-scoped listing, and returning only anchors for the requested repo. A regression that blanket-denies public?repo=reads or ignores therepopredicate would still pass here.🤖 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/test_support.rs` around lines 1968 - 2017, The `anchors_repo_denies_anonymous_on_private` and `anchors_repo_allows_owner` tests only cover a single private repo, so they do not prove that `?repo=` works for public repositories or that the repo filter is actually applied. Add a repo-scoped success case for a public repo in the `anchors_router`/`seed_anchor` test area, and make the assertion check that only anchors for the requested repo are returned (not just the count). Use the existing `anchors_repo_*` test patterns and the `anchors_router` request helpers to locate the right spot.
🤖 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/test_support.rs`:
- Around line 1968-2017: The `anchors_repo_denies_anonymous_on_private` and
`anchors_repo_allows_owner` tests only cover a single private repo, so they do
not prove that `?repo=` works for public repositories or that the repo filter is
actually applied. Add a repo-scoped success case for a public repo in the
`anchors_router`/`seed_anchor` test area, and make the assertion check that only
anchors for the requested repo are returned (not just the count). Use the
existing `anchors_repo_*` test patterns and the `anchors_router` request helpers
to locate the right spot.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 13b6010e-7968-4a2e-ae89-754bc8ff29d1
📒 Files selected for processing (1)
crates/gitlawb-node/src/test_support.rs
beardthelion
added
crate:node
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
This closes the anonymous hole in #121 and the ?repo= anchor path is correctly visibility-gated, so the direction is right. But the global listings need to filter on current visibility, not just require authentication, and there is a real leak path that auth alone does not cover. I traced the write and read sides end to end against the merged state.
The index tables are correctly gated on the write side: pinning (repos.rs:1055,:1167, behind withheld.is_some()) and anchoring (repos.rs:1249, behind announce) only run for anonymously-root-readable repos, and object_list excludes withheld blobs. So a repo that is private at push time is never indexed. CodeRabbit's finding is still valid though, by a different mechanism than a missing write gate: the gate is evaluated once at push, visibility is mutable afterward, and nothing reconciles the index.
Findings
-
[P2] Filter the global
/arweave/anchorsand/ipfs/pinslistings on current visibility, not just authentication
crates/gitlawb-node/src/api/arweave.rs:53,crates/gitlawb-node/src/api/ipfs.rs(list_pins)
This confirms and extends CodeRabbit's open finding on the global anchor listing. Requiring authentication does not authorize: identities here are permissionless (optional_signatureverifies a self-made signature,registeris open), so any throwaway DID still reads the global lists. And because index rows are never reconciled when a repo is made private after a public push (noDELETEof either table exists,set_visibilitytouches neither, and both list queries are unfilteredSELECTs), the global lists can serve a now-private repo's slug, owner DID, branch names, commit SHAs, and object CIDs. Content stays gated byGET /ipfs/{cid}(#110/#133), so this is metadata disclosure. The same gap applies to/ipfs/pins, which is not flagged elsewhere. Resolve each row's repo and apply the current-visibility check before returning it (or restrict the global listing to a node-admin capability). Full mechanism and repro in #136. -
[P2] Fix the
?repo=gate-vs-filter representation drift (and the test that hides it)
crates/gitlawb-node/src/api/arweave.rs:51
The gate resolves the repo viaauthorize_repo_read->get_repo, which matches the owner byLIKE(fulldid:key:or bare short form). The result query then filters exactWHERE repo = $1on the raw?repo=string, but anchor rows are written short-form ({owner_short}/{name},repos.rs:1142). So?repo=did:key:zX/nameauthorizes (200) yet returns an empty list; only?repo=zX/namereturns the owner's anchors. It fails safe (returns fewer results, not more), so it is a correctness/usability bug, not a leak. The newanchors_repo_*tests pass only becauseseed_anchorseeds the full-DID form, which production never writes. Normalize the slug from the authorizedRepoRecordbefore querying, and seed the production short form in tests. -
[P3] Run
cargo fmt—test_support.rs:1903failsrustfmt --check
crates/gitlawb-node/src/test_support.rs:1903
A longassert_eq!line needs wrapping; the format gate fails.cargo fmtfixes it.
Notes (non-blocking): the parse-fail branch returns NotFound("repo not found") while the gate path returns RepoNotFound; both 404 and deny is indistinguishable from miss, so no oracle, but returning RepoNotFound on both is more consistent. ?limit=-1 reaches Postgres as LIMIT -1 (pre-existing). The new tests use local routers rather than build_router, so the layering fix that is the core of this PR is not asserted end to end; a test through build_router would lock it. The router refactor itself is correct: the old .merge-after-.layer left /ipfs/pins unsigned, and this fixes it.
The ?repo= gating pattern is the right one. Extending the same current-visibility filtering to the two global listings (per #136) is what completes the #121 fix.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/arweave.rs`:
- Around line 36-37: The anonymous global anchor listing path in the arweave API
still allows `list_arweave_anchors(None, limit)` to run when `repo` is absent,
which should be rejected instead. Update the handler in `arweave.rs` around
`caller`/`list_arweave_anchors` so that when no repository is specified and
`auth` yields `None`, the request returns a 401 before any query is executed.
Keep the authenticated and repo-scoped paths unchanged, and make sure the
`list_arweave_anchors` call only happens after a valid caller has been
established.
In `@crates/gitlawb-node/src/api/ipfs.rs`:
- Around line 195-203: The pin listing handler is still continuing when auth is
missing, so anonymous requests get a filtered success response instead of being
rejected. In the IPFS API handler around the caller/auth extraction and
`list_pinned_cids` flow, add an explicit unauthorized check when `caller` is
`None` before loading pins, and return the proper 401 early from this function
instead of proceeding into the database query and filter logic.
- Around line 220-229: The repo selection logic in list_pins is bypassing the
quarantine gate because it calls visibility_check directly instead of the shared
authorization path used by authorize_repo_read. Update the readable-repo
derivation in the loop over repos to preserve the quarantine exclusion before
applying repo-level visibility, preferably by reusing authorize_repo_read or
extracting the quarantine filter into a shared helper referenced by both
list_pins and authorize_repo_read. Ensure the fix is applied around the repo
iteration and visibility_check flow so quarantined repos never contribute CIDs
to the pins index.
🪄 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: e012ef44-6106-4e47-a97e-b4a94070946d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
crates/gitlawb-node/src/api/arweave.rscrates/gitlawb-node/src/api/ipfs.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
…tion ✓ P3 blocker fixed: cargo fmt applied — the format gate will pass. ✓ P3 cleanup resolved: withheld_blob_oids is still used by replication code in repos.rs, so it stays. • P2 follow-up: Tree/commit disclosure tracked in Gitlawb#135 — out of scope here.
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] Complete CodeRabbit's request to reject anonymous global listings
crates/gitlawb-node/src/api/arweave.rs:36,crates/gitlawb-node/src/api/ipfs.rs:195
The latest patch still only makes the signature optional and then proceeds whencallerisNone. As a result, anonymousGET /api/v1/arweave/anchorsandGET /api/v1/ipfs/pinsreturn200instead of the401behavior this PR claims and tests for. I verified this with the PR's own focused tests:cargo test -p gitlawb-node pins_list -- --nocapturefails withpins_list_denies_anonymousreturning200instead of401, andcargo test -p gitlawb-node anchors_ -- --nocapturefails withanchors_global_denies_anonymousreturning200instead of401. Please complete the current CodeRabbit requests by returningUnauthorizedbefore querying/filtering whenever the global listing has no authenticated caller. -
[P1] Fix the new endpoint tests so they pass and cover the production data shape
crates/gitlawb-node/src/test_support.rs:1889,crates/gitlawb-node/src/test_support.rs:1960,crates/gitlawb-node/src/test_support.rs:2011
The new tests currently seed rows that the new implementation filters out, so the PR's own coverage is red and does not prove the intended production behavior.pins_list_allows_authenticatedinserts a barepinned_cidsrow but no readable repo/object containing that SHA, whilelist_pinsnow only returns pins whose SHA is found by scanning readable repo object databases, so the count is0instead of1. The anchor tests seedsome/repoor the full-DID slug (did:key:.../repo), but production anchor writes use the short owner slug fromrepos.rs, andlist_anchorsnow normalizes authorized?repo=lookups to that short form, so the owner/global success tests also return count0. Please seed the same repo/object and short-slug shapes that the push/anchor paths actually write, then keep the assertions on the filtered results. -
[P2] Preserve the quarantine gate when deriving readable repos for pins
crates/gitlawb-node/src/api/ipfs.rs:220
authorize_repo_readhides quarantined repos before it applies visibility rules, but the newlist_pinsimplementation bypasses that helper and iterateslist_all_repos()with a directvisibility_check. A quarantined public mirror can therefore still contribute object SHAs toallowed_sha256s, and any matching pinned CID is returned even though the rest of the read surface treats that repo as nonexistent until quarantine is cleared. Please reuseauthorize_repo_readsemantics here or add the sameis_repo_quarantinedexclusion before scanning repo objects.
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
The core gates verify as resolved on the current head: anonymous global listings return 401 before any query, the pin scan skips quarantined repos, and the tests now seed the production slug and object shapes. The earlier P1/P1#2/P2 all check out on this head. What's left is regression coverage on the new exclusion branches, which currently have no test exercising them.
Findings
-
[P3] Cover the quarantine exclusion in
list_pins
crates/gitlawb-node/src/api/ipfs.rs:235
Theis_repo_quarantinedskip is correct, but nopins_test seeds a quarantined repo, so nothing guards it against regression. Seed a quarantined mirror that shares an object SHA with a pinned CID (and no readable non-quarantined repo carrying that SHA), then assert the pin is withheld. -
[P3] Cover the path-scoped withheld-blob exclusion in
list_pins
crates/gitlawb-node/src/api/ipfs.rs:260
list_pinsbuilds its allowed set withwithheld_blob_oids, a separate construction fromget_by_cid'sallowed_blob_set_for_caller, and nopins_test sets a path-scoped rule, so this subtraction is uncovered. Add a public repo with a/secret/**rule where a withheld blob OID is pinned, and assert an authenticated non-reader's listing excludes that CID while the visible-object pins remain. This is the branch that would leak a private blob SHA through the pin index if the subtraction regresses. -
[P3] Add a negative case for the global authenticated anchor filter
crates/gitlawb-node/src/api/arweave.rs:76
anchors_global_allows_authenticatedonly proves a public anchor is visible; the per-rowauthorize_repo_readfilter has no test that an authenticated non-reader is denied another repo's private anchor. Add that exclusion assertion, mirroringanchors_repo_denies_non_readerfor the?repo=path.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/gitlawb-node/src/test_support.rs (3)
2079-2240: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated owner/short-slug boilerplate across the five anchors tests.
anchors_global_allows_authenticated,anchors_global_denies_non_reader,anchors_repo_denies_anonymous_on_private,anchors_repo_allows_owner, andanchors_repo_denies_non_readerall repeat the identicalowner_short/short_slugderivation and repo/anchor seeding sequence. A small shared helper (e.g.,seed_owned_repo_with_anchor(is_public, repo_name) -> (Keypair, String /*owner_did*/, String /*short_slug*/)) would cut the duplication and centralize the short-slug convention these tests depend on.🤖 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/test_support.rs` around lines 2079 - 2240, The five anchors tests repeat the same owner DID to short-slug derivation and repo/anchor seeding logic, so factor that setup into a shared helper in test_support.rs and reuse it from anchors_global_allows_authenticated, anchors_global_denies_non_reader, anchors_repo_denies_anonymous_on_private, anchors_repo_allows_owner, and anchors_repo_denies_non_reader. Have the helper create the Keypair, derive owner_short/short_slug, seed either a public or private repo, and insert the anchor so the tests only vary by visibility and request/assertion details.
2146-2177: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing positive-path coverage: anonymous
?repo=access to a public repo's anchors.The
?repo=tests only cover a private repo (deny-anonymous here, allow-owner at 2180-2212, deny-non-reader at 2214-2240). There's no test asserting an anonymous caller can still read anchors for a public repo via?repo=, even though the production comments emphasize that gating must not break legitimate anonymous access to public content. Adding that case would directly validate the PR's stated goal of not over-restricting public repos.🤖 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/test_support.rs` around lines 2146 - 2177, Add missing positive-path coverage for anonymous `?repo=` access on a public repo in the `anchors_repo_denies_anonymous_on_private` area. Create a new test alongside the existing `anchors_router` / `list_anchors` cases that seeds a public repo and anchor, calls the `GET /api/v1/arweave/anchors?repo=...` path with `anon_get`, and asserts `StatusCode::OK` (or the expected success response) to verify `?repo=` still allows anonymous reads for public content.
1881-2030: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting shared setup for the three pins tests.
pins_list_allows_authenticated,pins_list_excludes_quarantined_repos, andpins_list_withholds_path_scoped_blobseach repeat the same Keypair/fs_slug/short/seed_cid_reposboilerplate. Extracting a small helper (e.g., returning(Keypair, owner_did, CidFixture)) would reduce duplication and the risk of the three tests silently drifting apart on the owner-DID/slug convention.♻️ Sketch of a shared fixture helper
struct PinFixture { owner: gitlawb_core::identity::Keypair, owner_did: String, fx: CidFixture, } fn seed_pin_owner(bare_names: &[&str]) -> PinFixture { use gitlawb_core::identity::Keypair; let owner = Keypair::generate(); let owner_did = owner.did().to_string(); let fs_slug = owner_did.replace([':', '/'], "_"); let short = owner_did.split(':').next_back().unwrap().to_string(); let fx = seed_cid_repos(&fs_slug, &short, bare_names); PinFixture { owner, owner_did, fx } }🤖 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/test_support.rs` around lines 1881 - 2030, The three pins tests repeat the same owner setup, slug derivation, and seed_cid_repos call, so extract that boilerplate into a small shared helper to keep them aligned. Add a fixture/helper near pins_list_allows_authenticated, pins_list_excludes_quarantined_repos, and pins_list_withholds_path_scoped_blobs that returns the generated Keypair, owner_did, and CidFixture (or equivalent), and update each test to use it for fs_slug/short and repository seeding. Keep the existing test-specific assertions and repo/quarantine/visibility setup 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.
Nitpick comments:
In `@crates/gitlawb-node/src/test_support.rs`:
- Around line 2079-2240: The five anchors tests repeat the same owner DID to
short-slug derivation and repo/anchor seeding logic, so factor that setup into a
shared helper in test_support.rs and reuse it from
anchors_global_allows_authenticated, anchors_global_denies_non_reader,
anchors_repo_denies_anonymous_on_private, anchors_repo_allows_owner, and
anchors_repo_denies_non_reader. Have the helper create the Keypair, derive
owner_short/short_slug, seed either a public or private repo, and insert the
anchor so the tests only vary by visibility and request/assertion details.
- Around line 2146-2177: Add missing positive-path coverage for anonymous
`?repo=` access on a public repo in the
`anchors_repo_denies_anonymous_on_private` area. Create a new test alongside the
existing `anchors_router` / `list_anchors` cases that seeds a public repo and
anchor, calls the `GET /api/v1/arweave/anchors?repo=...` path with `anon_get`,
and asserts `StatusCode::OK` (or the expected success response) to verify
`?repo=` still allows anonymous reads for public content.
- Around line 1881-2030: The three pins tests repeat the same owner setup, slug
derivation, and seed_cid_repos call, so extract that boilerplate into a small
shared helper to keep them aligned. Add a fixture/helper near
pins_list_allows_authenticated, pins_list_excludes_quarantined_repos, and
pins_list_withholds_path_scoped_blobs that returns the generated Keypair,
owner_did, and CidFixture (or equivalent), and update each test to use it for
fs_slug/short and repository seeding. Keep the existing test-specific assertions
and repo/quarantine/visibility setup unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 77f2dac8-2f19-4e9e-bb77-0aa66c200e5f
📒 Files selected for processing (1)
crates/gitlawb-node/src/test_support.rs
Gravirei
force-pushed
the
bug_fix_2
branch
2 times, most recently
from
bf9d690 to
069804a
Compare
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Re-reviewed on 069804a. The delta since my last pass is test-only (test_support.rs, +200/-84); the gates in list_pins and list_anchors are byte-identical to beafa8a where I already verified them, so this pass covers the new coverage.
All three gaps I flagged are closed, and each new test is load-bearing: with its production guard disabled in isolation, only that test fails, with the expected count.
- quarantine skip:
pins_list_excludes_quarantined_repos(0 vs 1) - path-scoped withheld-blob subtraction:
pins_list_withholds_path_scoped_blobs(secret OID leaks, 1 vs 2) - global per-row filter:
anchors_global_denies_non_reader(0 vs 1)
CodeRabbit's three nitpicks on 81a5772 (extract the pins/anchors fixtures, add the anonymous-public anchor case) are all addressed here. The full CI suite did not trigger on this head, so I ran it locally: fmt clean, clippy clean, full gitlawb-node suite green (325 passed).
One optional follow-up, not a blocker: list_pins has its own copy of the fail-closed "withheld walk failed, skip repo" arm with no test, while get_by_cid's copy is covered by ipfs_cid_walk_error_fails_closed.
Approving. @kevincodex1 this clears my earlier changes-requested; jatmn's CHANGES_REQUESTED also predates this head.
|
@kevincodex1 LGTM |
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
-
[P2] Cover the production router wiring for the signed metadata endpoints
crates/gitlawb-node/src/test_support.rs:1848,crates/gitlawb-node/src/test_support.rs:2032
The new tests prove the handlers work when the test manually builds a tiny router and appliesoptional_signature, but they do not exerciseserver::build_router, which is the production wiring this PR had to fix. That matters here because/api/v1/ipfs/pinswas previously outside the optional-signature layer; ifserver.rsregressed or the route were left in the old.merge(...).layer(...)shape, the signedpins_list_allows_authenticatedtest would still pass while the real route would never attachAuthenticatedDidand would return401for every signed pins request. Please add at least one integration test throughbuild_routerfor the signed success path, especially/api/v1/ipfs/pins, so the route-layer fix is actually pinned. -
[P3] Complete CodeRabbit's request to clamp negative anchor limits
crates/gitlawb-node/src/api/arweave.rs:69
CodeRabbit's earlier negative-limit request is still valid on the current head:q.limit.min(200)caps only the upper bound, so/api/v1/arweave/anchors?limit=-1still passes-1intolist_arweave_anchors, where it is bound directly into PostgreSQLLIMIT. PostgreSQL rejects a negative limit, so a malformed query can turn this listing endpoint into an internal error instead of a bounded empty/small result. Please clamp the lower bound as well, for example withq.limit.clamp(0, 200), before callingstate.db.list_arweave_anchors.
sqlx prepared statements do not support semicolon-delimited multi-statement queries. Split into three separate execute calls.
P2: Remove the legacy-pin empty-repo compatibility path that exposed unmapped legacy pinned_cids rows as unconditionally readable to every authenticated caller. Legacy rows with empty repo/owner_did are now invisible (they don't match the UNNEST pair filter). P2: Add a non-leaking truncated_cursor resume mechanism. When the keyset scan hits MAX_BATCHES before accepting any visible pin (all hidden rows), the response now includes a truncated_cursor that encodes only the pinned_at timestamp (no repo or SHA). The caller can echo this back as ?truncated_cursor=<token> to resume scanning past the hidden window. The SQL query for truncated_cursor uses a (pinned_at < ) condition that avoids passing any hidden-blob identifier into the query.
…n CLI P2: Replace pinned_at-in-base64 truncated_cursor with a genuinely opaque token. The server stores (pinned_at, repo, sha256_hex) under a random UUID in an in-memory Mutex<HashMap> and returns only the UUID. Callers echo it back via ?truncated_cursor= and the server looks up the full cursor — no hidden-row metadata is ever exposed (no pinned_at, repo, or sha256_hex leaks through the wire). Entries TTL at 5 minutes. P2: Both gl ipfs list (ipfs_cmd.rs) and gl node status (node.rs) now follow truncated_cursor when next_cursor is absent and truncated is true, enabling recovery past hidden-only scan windows. P2: With the full DB cursor preserved in the cache, the resume query uses the complete (pinned_at, repo, sha256_hex) 3-tuple, so no rows are skipped due to same-timestamp grouping.
… guard, legacy pins P2: Walk budget exhaustion now signals truncated instead of silently dropping rows. db_cursor is advanced per-pin (not per-batch). When a new path-scoped repo exceeds MAX_WALKS, the loop breaks without advancing past that pin so the caller can retry with a fresh walk cache. P2: truncated_cursor is now retry-safe — the server stores the full (pinned_at, repo, sha256_hex) under a random UUID in an in-memory cache and never removes entries on lookup. Expired/unknown tokens simply don't match (fall back to start). P2: CLI advancement guard — gl ipfs list and gl node status now detect non-advancing cursors and error instead of silently looping to MAX_PAGES or returning partial counts. P2: Re-added legacy-pin compat path with explicit log warnings. Pre-Gitlawb#134 pins with empty repo/owner_did are returned to any authenticated caller with a tracing::warn! so operators can backfill repo context.
…mory truncated cursor cache
P1: Remove empty ("", "") pair from UNNEST query set in list_pins.
Unmapped legacy v10-backfilled pins (repo='', owner_did='') are no longer
returned to any signed caller. The visibility gate cannot be applied to
rows without repo context, so serve nothing until the row is backfilled.
P2: Replace server-side Arc<Mutex<HashMap>> truncated_cursor cache with
self-encoded opaque tokens. The cursor (pinned_at|repo|sha256_hex) is
base64-encoded directly into the response, same format as next_cursor.
This eliminates: O(n^2) contention under the mutex, per-caller abuse
vector (unbounded map growth), durability gap (lost on restart/routed to
other machine), and silent restart at page 1 on expired/missing tokens.
Drops the AppState.truncated_cursors field and all related init sites.
…alize_owner_key
P1: truncated_cursor is now a self-contained opaque token using
HMAC-SHA256 authenticated encryption (XOR stream cipher keyed by
the node's Ed25519 seed). The caller cannot decode hidden-row
metadata (pinned_at, repo, sha256_hex) from the wire format.
Token format: base64(nonce_16 || expiry_8 || encrypted || hmac_tag_32).
Expires after 5 minutes. Survives restart and cross-node routing.
P2: Local IPFS pin recording in repos.rs now builds the repo slug
using crate::db::normalize_owner_key() instead of
owner_did.split(':').next_back(), matching the same normalization
used by list_pins, Pinata, and Arweave paths.
…on, deny-test P1: Replace HMAC-SHA256 CTR (repeating keystream leak) with XChaCha20Poly1305 AEAD. Token = base64(nonce_24 || ciphertext). Expiry embedded in plaintext, authenticated by AEAD tag. Hidden sha256_hex is not recoverable even with known pinned_at/repo prefix. P2: Derive dedicated cursor subkey via HKDF-SHA256 with domain-separated info string (b"gitlawb-ipfs-cursor-v1") instead of feeding the raw Ed25519 signing seed into the cipher primitive. P2: Add per-DID rate limiting to ipfs routes via rate_limit_by_did middleware. Binds the cost of up to 50 allowed_blob_set_for_caller walks per list_pins request. P2: Add pagination to and readers. Both now follow next_cursor/truncated_cursor with an advancement guard and surface incomplete state when pagination stalls. P2: Return 400 BadRequest when truncated_cursor is present but undecodable, instead of silently restarting at page 1. P3: Add deny-test confirming hidden sha256_hex does not appear in the clear within token bytes, and that wrong-key decode fails.
The ordinary cursor path (base64-encoded pinned_at|repo|sha256_hex) previously returned None on decode failure, silently restarting at page 1. Now returns BadRequest with a descriptive message, matching the truncated_cursor error behavior. GL readers (gl ipfs list, gl node status) now paginate through next_cursor/truncated_cursor with an advancement guard and surface incomplete state when pagination cannot progress.
…ALKS P2: Add absolute page cap (10k) and row cap (1M) to both GL pagination loops, plus a seen-cursor set for cycle detection. Previously only had a single-slot repeat guard and 10-empty-page cap. P2: Increase truncated_cursor TTL from 300s to 600s. Client restarts from last visible cursor on 400 (expired/bad token). P2: Advance db_cursor past the pin that exhausts MAX_WALKS so forward progress remains possible on retry. P3: Refactor allowed_blob_set_for_caller to also return the full set of all blob oids. Use this in list_pins to emit structural objects (commits/trees — absent from the blob set) when root / is accessible (KTD3), matching get_by_cid behavior. P3: Strengthen truncated-cursor no-leak test with real known-plaintext XOR recovery attempt against the AEAD ciphertext (which must fail). P3: Commit stranger-denied deny-test (private repo, stranger caller) and orphan repo='' deny-test (legacy empty-repo pins excluded).
…; verify structural objects via store::object_type
…ommit) still run structural-object check
…add structural test, fix cursor TTL comment
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Re-reviewed on f0d3701. Most of the long tail here is genuinely resolved: the anchor slug collision is closed by the (repo, owner_did) paired join, the repo='' orphan pins are excluded, both handlers clamp the limit, the client checks status before parsing and now pages to exhaustion, and the AEAD truncated_cursor on the MAX_BATCHES path is opaque. The gating is sound. One confidentiality leak remains, and it is the same class the PR set out to close.
Findings
-
[P1] Route the MAX_WALKS-wall cursor through the opaque token; it currently emits a withheld pin's sha256_hex in plaintext.
crates/gitlawb-node/src/api/ipfs.rs:618
When the walk budget is exhausted, line 618 sets response_cursor to skip_pos, the un-walked pin whose blob visibility was never resolved, and the emit branch at line 782 serializes it with encode_cursor (plaintext base64), preempting the AEAD path at line 784. Reproduced by execution: 51 distinct path-scoped repos with no repo on disk (so every pin is withheld), driven through list_pins as a stranger did:key, returns count:0, truncated:true, truncated_cursor:null, and next_cursor base64-decodes to2026-07-03T00:00:00Z|z6MkWALLOWNER.../wall0|wallsha00, the pinned_at, repo, and sha256_hex of a pin the caller was never shown. Same INV-13 leak the MAX_BATCHES path prevents, escaping via the walk-budget branch. It is also your still-open advancement finding, since the strict<resume then drops that pin entirely. Keep skip_pos in db_cursor only so the emit falls through to the AEAD token, and resume from the last classified pin (not skip_pos) so the visible-row loss is fixed too. Add a regression test seeding >= MAX_WALKS distinct path-scoped repos; the current cursor test only reaches the MAX_BATCHES branch. -
[P2] Stop
gl ipfs listfrom restarting at page 1 when a truncated_cursor expires.
crates/gl/src/ipfs_cmd.rs:88
cursor and truncated_cursor are both take()-n before the request, so on a 400 for an expired token the continue at line 94 re-requests with no cursor and re-collects earlier pages as duplicates. The comment says it restarts from the last visible cursor, but that value is already gone. Retain the last next_cursor across the truncated leg, or drop the token expiry since the AEAD already provides integrity. -
[P2] Bound the pin-listing walk cost under concurrency.
crates/gitlawb-node/src/api/ipfs.rs
Per request is bounded (MAX_WALKS=50, MAX_BATCHES=10, MAX_PROBES=200), but each walk is a spawn_blocking git subprocess with no timeout and no cross-request cache, so concurrent callers against deep repos can pin the blocking pool. A wall-clock timeout on batch_object_types and the blob walk, plus a small per-(repo, head) cache, would close it. -
[P3] Two hardening items. The v10 primary-key swap (sha256_hex to (repo, sha256_hex)) makes a pre-#134 binary's ON CONFLICT(sha256_hex) fail against a migrated schema; the blue/green caveat is in the comment, but a maintenance-window note would help. And the repo='' exclusion is structural (an absent map key) rather than an explicit guard, so a future refactor could regress it silently; an explicit
if pin.repo.is_empty()skip would pin it.
Only the P1 blocks.
- [P1] Stop leaking skip_pos sha256_hex in next_cursor plaintext; keep only db_cursor advancement so emit falls through to AEAD token. - [P1] Add regression test test_max_walks_plaintext_not_in_response_cursor seeding 55+ repos to exercise both MAX_WALKS paths (visible pin first → safe next_cursor; hidden-only page → AEAD opaque token). - [P2] Retain cursor across expired truncated_cursor in gl CLI so we don't restart at page 1. - [P2] Add 60s/30s wall-clock timeouts on spawn_blocking walks and batch_object_types probes to bound concurrency cost. - [P3] Add explicit pin.repo.is_empty() guard for robustness. - [P3] Strengthen v10 blue/green maintenance-window note.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the previously discussed paths and do not see any remaining actionable issues from my side.
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Re-reviewed on 2ed2507a. Approving. My prior blocker, the MAX_WALKS-wall plaintext next_cursor leaking a withheld pin's sha256_hex (INV-13), is closed, and I verified it load-bearing by execution rather than inspection: dropping response_cursor at the wall so the emit falls through to the AEAD token is the right fix, and re-adding that one line turns both your regression test and an all-hidden-ordering repro of mine RED (leaking the withheld pinned_at|repo|sha), GREEN with it removed. I also confirmed on this head that a visible pin sitting past the wall is still reachable exactly once across pagination (no skip, no dup), that repo='' stays denied to a non-owner, and that a timed-out walk fails closed.
Two non-blocking follow-ups, neither holds the merge:
Findings
- [P3] Correct the timeout comment
crates/gitlawb-node/src/api/ipfs.rs:643.tokio::time::timeoutdoes not cancel aspawn_blockingtask, so the blocking thread keeps running and the wall-clock timeout does not "prevent pinning the blocking pool on deep repos." It does bound the request's wait, which is a real improvement, so the code is fine; the comment just overstates it. The residual concurrent-walk amplification is pre-existing and better closed with a walk semaphore in a follow-up than here. - [P3] The expired-
truncated_cursorrestore is a no-opcrates/gl/src/ipfs_cmd.rs:76. The server emitsnext_cursorxortruncated_cursor, so on the 400 branchsaved_cursoris alreadyNone; the restore restarts at page 1 anyway and the cycle guard then stops it as incomplete. Either drop the dead branch or persist the lastnext_cursorseparately.
@kevincodex1 LGTM.
|
Thanks for staying with this one through all the rounds, @Gravirei. A lot of iteration here, especially hardening the pins/anchors gating and the cursor state machine, and it ended up in solid shape. Appreciated the responsiveness on each pass. |
Closes #121
Summary
Two node-wide metadata endpoints were served without authentication or visibility filtering, leaking private-repo metadata.
Changes
/api/v1/ipfs/pins(list_pins):optional_signaturemiddleware to the route401 Unauthorizedfor anonymous callers, stopping anonymous node-wide CID enumeration/api/v1/arweave/anchors(list_anchors):optional_signaturemiddleware to the route?repo=<owner>/<name>is provided: gates onauthorize_repo_read(deny → 404), preventing anchor metadata leaks for private repos?repo=: requires authentication for the global anchor listingTesting
Summary by CodeRabbit
?repo=<owner>/<name>is strictly validated and unauthorized repos are hidden; results are capped (limit ≤ 200) and returned with correctcount.401(anonymous) and200/404visibility behavior for both global and?repo=scopes.