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 21 commits into
Conversation
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
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
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.
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Both of jatmn's blockers are resolved on this head: the build_router integration test genuinely pins the pins wiring (it fails 401 if the route drops optional_signature, while the mini-router test stays green), and the anchor limit is clamped. One correctness issue remains before this is ready.
Findings
-
[P2] Apply the visibility filter before the LIMIT on the global anchor listing
crates/gitlawb-node/src/api/arweave.rs:69
list_arweave_anchors(None, limit)takes the newestlimitrows and only then drops the ones the caller cannot read, so when the most recent anchors belong to unreadable repos the caller getscount=0even though they have readable anchors further back. The limit clamps to 200 and there is no cursor, so on a busy node a caller's readable anchors become unreachable, not just under-counted. It is fail-closed (never over-discloses), but the endpoint stops returning a legitimate caller's own data. Filter on visibility first, then takelimit, the way the siblinglist_pinspath already does (it fetches the full set, then filters). -
[P3] Add a negative-limit test for the anchors endpoint
crates/gitlawb-node/src/api/arweave.rs:69
clamp(0, 200)is load-bearing but untested: a refactor back tomin(200)reintroduces a 500 on?limit=-1(Postgres rejectsLIMIT -1) with nothing to catch it. One test asserting?limit=-1returns a bounded 200 pins it.
Core gating is otherwise sound: anonymous and non-reader paths are correctly denied on both endpoints.
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] Filter global anchors before applying the limit
crates/gitlawb-node/src/api/arweave.rs:69
Beardthelion's current review item is still valid on this head: the global path askslist_arweave_anchors(None, limit)for the newest limited rows, and only then drops rows the caller cannot read. If the newest 200 anchors belong to unreadable repos, an authenticated caller gets an empty response even when older readable anchors exist, and there is no cursor to reach them. Please apply the visibility filter before taking the requested limit, matching the fail-closed shape used by the pins path. -
[P2] Do not let mirror rows satisfy the new current-visibility filters
crates/gitlawb-node/src/api/arweave.rs:87,crates/gitlawb-node/src/api/ipfs.rs:216
The new filters still resolve visibility through raw mirror rows in the mirror+canonical case. Anchor rows are stored as the short owner slug, so the global anchor filter callsauthorize_repo_read(short, name, ...);get_repocan match either the bare public mirror row or the private canonicaldid:key:row because it has no canonical preference. The pins path has the same problem more directly: it scans every row fromlist_all_repos(), including public mirror rows, and adds their object SHAs toallowed_sha256swithout collapsing to the canonical survivor. A non-reader can therefore still receive anchor/pin metadata whenever the mirror row is chosen or scanned even though the canonical repo rules would deny it. Please resolve these filters against the same canonical/deduped visibility source used by the listing surfaces, or make this PR depend on and test the canonical-row preference before relying on these gates. -
[P3] Add a regression test for negative anchor limits
crates/gitlawb-node/src/api/arweave.rs:69
The currentq.limit.clamp(0, 200)fix addresses jatmn's and CodeRabbit's negative-limit concern, but it is still untested. A future refactor back tomin(200)would sendLIMIT -1into Postgres and turn a malformed request into a 500 again. Please add a small/api/v1/arweave/anchors?limit=-1test that asserts the endpoint returns a bounded successful response.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the changed paths and found issues that still need to be addressed.
Findings
-
[P2] Complete the mirror-row fix for repo-scoped anchor lookups
crates/gitlawb-node/src/api/arweave.rs:57
jatmn's mirror-row request is only fixed for the new global filters. The?repo=path still gates throughauthorize_repo_read, which callsget_repoand can match either the bare public mirror row or the private canonicaldid:key:row because that lookup has no canonical preference. If a caller requests/api/v1/arweave/anchors?repo=<short-owner>/<name>for a repo that has both rows, the public mirror row can satisfy the read gate, then this handler normalizes back to the same short slug and returns the anchor rows even though the canonical repo rules would deny the caller. Please resolve the repo-scoped path against the same canonical/deduped visibility source used by the global filter, or make the canonical-row preference land before relying on this gate, and add a mirror+canonical regression test for the?repo=anchor endpoint. -
[P2] Keep the global anchor listing bounded in SQL
crates/gitlawb-node/src/api/arweave.rs:85
The global path now callslist_arweave_anchors(None, i64::MAX)and only applies the requestedlimitafter every anchor row has been loaded, filtered, and collected in Rust. This endpoint is reachable by any signed DID, and anchors are written on every announced push/ref update, so the first authenticated global-list request on a large node can force an unboundedORDER BY anchored_at DESC LIMIT 9223372036854775807plus a full result materialization just to return at most 200 rows. Please keep the visibility-before-limit behavior without removing the database bound, for example by querying only readable repo slugs withWHERE repo = ANY(...) ORDER BY anchored_at DESC LIMIT $limitor by using a bounded/cursor batch loop.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Must fix before merge (blocking)
B1. /api/v1/ipfs/pins is not under optional_signature -> always 401
crates/gitlawb-node/src/server.rs:217-220
let ipfs_routes = Router::new()
.route("/ipfs/{cid}", get(ipfs::get_by_cid))
.layer(middleware::from_fn(auth::optional_signature)) // covers only /ipfs/{cid}
.merge(Router::new().route("/api/v1/ipfs/pins", get(ipfs::list_pins))); // added AFTER -> uncovered
Because the layer is applied before the .merge(), the handler's
Option<Extension> is None for EVERY request (signed or not).
The handler then hits its if caller.is_none() -> 401 guard (ipfs.rs:407),
so the endpoint returns 401 for all callers. It is completely unusable. The
comment at server.rs:213-216 ("/api/v1/ipfs/pins stays unsigned - gating ...
tracked separately (#121)") is stale and contradicts this PR's intent.
Fix: move the layer after the merge (it then also covers /ipfs/{cid}), and
update the comment:
let ipfs_routes = Router::new()
.route("/ipfs/{cid}", get(ipfs::get_by_cid))
.merge(Router::new().route("/api/v1/ipfs/pins", get(ipfs::list_pins)))
.layer(middleware::from_fn(auth::optional_signature));
This PR did NOT modify server.rs at all (it is absent from the diff), so the
wiring fix is missing entirely.
B2. Production pin writers record empty repo/owner_did -> listing is inert
- crates/gitlawb-node/src/ipfs_pin.rs:133 calls db.record_pinned_cid(&sha, &cid)
(2-arg), hard-coding repo='', owner_did=''. - crates/gitlawb-node/src/pinata.rs:114 calls db.record_pinata_cid(...) (2-arg),
same empty context. - crates/gitlawb-node/src/db/mod.rs:2266-2268 and 2487-2489: the 2-arg wrappers
delegate to _full(..., "", ""). - The new _full variants (db/mod.rs:2276, 2493) have NO production caller.
Consequence: every newly-pinned CID is stored with repo=''. list_pins builds
its readable set from list_all_repos_deduped (real short/name slugs) and queries
list_pinned_cids_for_repos filtered by (repo, owner_did) IN (...); rows with
repo='' match nothing and are skipped (ipfs.rs orphan branch). So the listing
returns NO newly-pinned content for anyone - the feature is inert in production
(fail-closed, no leak, but useless). Worse, record_pinata_cid_full's
ON CONFLICT(sha256_hex) DO UPDATE SET repo = EXCLUDED.repo, owner_did = EXCLUDED.owner_did
(db/mod.rs, pinata SQL) CLOBBERS the v11 migration backfill to '' whenever a
Pinata write touches a legacy row, so even backfilled pins disappear over time.
pin_new_objects already has repo_path: &Path in scope (ipfs_pin.rs:99), so the
owner/repo slug is derivable. Fix: pass the real repo/owner_did (e.g., from
repo_path or the push context) into record_pinned_cid_full / record_pinata_cid_full,
and change ON CONFLICT to
SET repo = COALESCE(NULLIF(EXCLUDED.repo,''), pinned_cids.repo), ...
so an empty-context write never overwrites a populated association.
Should fix before merge (major)
M1. Non-cancellable blocking walk is a DoS vector
ipfs.rs:626-629 (comment) + :629 and :703 spawn_blocking wrapping
allowed_blob_set_for_caller / batch_object_types, guarded only by
tokio::time::timeout(60s/30s). The code itself notes the timeout does NOT cancel
the blocking task, so a timed-out walk leaks an orphaned thread + live git child.
One request can spawn up to MAX_WALKS=50 of these; a handful of concurrent
authenticated (permissionless) callers can exhaust the default 512-thread blocking
pool and starve the node. allowed_blobs_by_repo (ipfs.rs:538) also retains up to
50 full-history blob-OID sets (multi-GB for large repos).
Fix: kill the child process on timeout (e.g., CommandExt::kill_on_drop or explicit
child.kill() in a drop guard), cap total concurrent walks node-wide, and bound the
retained allowed-set size.
M2. Transient walk error permanently hides a repo's pins and advances the cursor
ipfs.rs:642 and :645: on walk timeout/panic/error the code inserts
(HashSet::new(), PathBuf::new()) into allowed_blobs_by_repo and continues, so
EVERY pin of that repo is classified hidden for the rest of the request, while
db_cursor is still advanced past them. A transient fault therefore permanently
drops legitimate (including structural) pins from pagination. Fail-closed (no leak)
but a real availability/correctness bug.
Fix: on walk failure, treat the repo as "defer/skip with page_truncated" rather
than "all hidden + advance", or retry the walk once; do not advance db_cursor past
undecided pins.
M3. probe_limit computed lazily -> visible structural pins dropped
Phase 2 (ipfs.rs:669 if probe_count >= MAX_PROBES) breaks out of a HashMap
iteration and folds probe_limit only from the CURRENT repo's candidates; repos
iterated later keep probe_limit = usize::MAX, so their unprobed structural
candidates fall through the defensive unwrap_or(false) in Phase 3 and are silently
dropped as hidden. Reachable when >=2 path-scoped repos exhaust the 200 probe
budget. (Corroborated by S2, S8, S10.)
Fix: compute probe_limit once, up front, as the minimum candidate index over ALL
structural_candidates (or cap probe budget per repo so every candidate is probed),
and set page_truncated whenever Phase 3 stops before batch end.
M4. gl ipfs list is not paginated; list_pins_paginated is dead code
crates/gl/src/ipfs_cmd.rs:93-94 marks list_pins_paginated #[expect(dead_code)],
and cmd_list (ipfs_cmd.rs, unchanged) still performs a single unpaginated fetch.
The PR description claims "CLI clients were updated," but only node.rs fetch_pins
was wired. The gl ipfs ... list path therefore truncates to one page (<=200 pins).
Fix: call list_pins_paginated from the list command (or remove the dead code and
paginate inline); or explicitly scope the claim.
Minor / should-address
- M5. page_filled suppresses the truncated signal. ipfs.rs:774-803: when a page is
both full and truncated (walk/probe budget hit), the if/else if emits only
next_cursor and drops the truncated flag + truncated_cursor. Pagination still
works via next_cursor, but the bounded-scan signal is lost. Emit truncated
alongside next_cursor when both hold. - M6. Spurious truncated_cursor on an empty 10th batch. ipfs.rs:776
page_truncated = batch_count >= MAX_BATCHEStriggers even when the final (10th)
batch is empty (true end of data), yielding a needless truncated_cursor. - M7. Client body-size guard is partly ineffective. node.rs / ipfs_cmd.rs check
Content-Length but skip it when the header is absent/chunked, then call
resp.bytes().await (node.rs:308) which already buffers the whole body before the
body.len() check (node.rs:312). A hostile/buggy node can exhaust memory on a
chunked oversized response. Also both clients break and drop the over-cap page
(silent under-report); fetch_pins has no incomplete flag at all. - M8. test_truncated_cursor_does_not_leak_hidden_sha is a misleading test.
ipfs.rs:966: its XOR "known-plaintext attack" simulation is a strawman - it would
also pass for any unique-nonce stream cipher and does not validate the AEAD claim
(the assert_ne! is trivially true). Only the substring check and wrong-key decode
are load-bearing. The crypto itself is sound (see Positives); replace the XOR block
with a real tamper test (flip a ciphertext/tag byte -> expect None). - M9. Stale comments / unused branches. ipfs.rs still references a 1-tuple cursor
format in a comment (pinned_at only) though the code uses a 3-tuple;
pins.truncate(max_visible) is redundant after the loop bound.
Observation - PR claim vs. diff mismatch (arweave anchors)
The PR description states it also secures GET /api/v1/arweave/anchors, but
arweave.rs is ABSENT from this diff, and server.rs:223 wires
/api/v1/arweave/anchors with NO optional_signature layer and no visibility gating
in the current tree. Either the anchors work landed in another PR/commit, or this
claim is stale/incomplete. Please confirm the anchors endpoint is actually secured
in the deployed code; if it is meant to be part of this PR, it is missing.
B1: server.rs — move optional_signature layer after .merge() so /api/v1/ipfs/pins receives auth. B2: thread repo_slug/owner_did through ipfs_pin::pin_new_objects and pinata::pin_new_objects so production pin writes record the correct repo context. Use COALESCE(NULLIF(...)) in ON CONFLICT so empty- context writes never overwrite a populated association. M1: add AtomicBool cancellation flag to blob_paths / batch_object_types so spawn_blocking walks and probes are cooperatively cancellable on timeout. Set the flag when tokio::time::timeout fires, checked between git ls-tree calls and in the cat-file poll loop.
M2: on walk failure, set page_truncated + walk_limit_idx and break instead of caching an empty result mask (which permanently hid all pins from that repo and advanced the cursor past them). M3: when MAX_PROBES is exhausted, fold every remaining structural candidate index into probe_limit (not just the current repo's), preventing silent loss of unprobed candidates in later repos. M4: wire list_pins_paginated into cmd_list so gl ipfs list uses cursor-based pagination instead of a single unpaginated fetch. M5: emit truncated: true whenever page_truncated is set, regardless of whether the page is also full (page_filled). M6: track batch_hit_limit explicitly; only signal truncated from MAX_BATCHES when the batch limit was actually hit (not on natural scan exhaustion). M7: add body-size checks on error-path resp.text() reads in both ipfs_cmd.rs and node.rs. M8/M9: remove #[expect(dead_code)] from list_pins_paginated.
…y result and continue Breaking out of the per-pin loop on walk failure caused pins from other repos later in the batch to be skipped entirely, breaking the test_ipfs_cursor_guard test (which relies on visible pins coexisting in the same batch as a repo with a persistently broken walk path). Instead, cache the failed result and let subsequent pins from other repos be processed normally; page_truncated + walk_limit_idx still signal the caller to retry.
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] Wire and enforce the Arweave anchor visibility gate
crates/gitlawb-node/src/server.rs:223
The PR still mounts/api/v1/arweave/anchorsas a plain route, andlist_anchorstakes onlyState/Querybefore callingdb.list_arweave_anchors(q.repo.as_deref(), limit). There is nooptional_signature, no anonymous global rejection, and no repo-read authorization for?repo=..., so anonymous callers can still enumerate global or private-repo anchor metadata (owner_did, refs, old/new SHAs, CIDs, tx IDs, URLs). This leaves the Arweave half of #121 open. -
[P2] Record repo attribution even when the object is already pinned
crates/gitlawb-node/src/ipfs_pin.rs:117
The new listing query filters by the stored(repo, owner_did)pair, but the production writers skip beforerecord_pinned_cid_fullorrecord_pinata_cid_fullwheneveris_pinned(sha)/has_pinata_cid(sha)finds the SHA anywhere on the node. A normal shared Git object can be pinned first by repo A, then pushed by repo B; repo B's writercontinues and never records B's(repo_slug, owner_did), so readers who can see B but not A do not see their own pinned object. The Pinata path has the same state loss atcrates/gitlawb-node/src/pinata.rs:100. -
[P2] Do not classify unprobed structural pins as hidden
crates/gitlawb-node/src/api/ipfs.rs:748
TheMAX_PROBESpath still drops visible commit/tree/tag pins when the current repo has more structural candidates than the remaining probe budget. The code truncatesto_checktoremaining, incrementsprobe_count, and probes only that prefix, but it never folds the unprobed tail of the samecandidatesvector intoprobe_limit. Phase 3 then reaches thosepin_outcome == Nonerows, misses them instructural_cache, falls throughunwrap_or(false), and advancesdb_cursor, so visible structural pins can disappear from pagination permanently. Probe timeout/error has the same shape: the empty result map is treated as hidden rather than deferred. -
[P2] Return a resumable cursor when the first row cannot be classified
crates/gitlawb-node/src/api/ipfs.rs:647
If the first sorted pin belongs to a path-scoped repo andrepo_store.acquirefails or the allowed-blob walk times out/errors, the handler setspage_truncated = trueandwalk_limit_idx = 0. Phase 3 immediately breaks before setting eitherresponse_cursorordb_cursor, and response construction can emit{"truncated": true}with nonext_cursorortruncated_cursor. Bothgl ipfs listandgl node statusstop when both cursors are absent, so older visible pins behind a transient first-row failure are silently hidden and may even display as zero pins. -
[P2] Enforce the client body cap before buffering chunked responses
crates/gl/src/ipfs_cmd.rs:169
The new 64 MiB cap still only prechecksContent-Length; when a malicious or broken node sends a chunked pins response without that header,resp.bytes().awaitbuffers the entire body before the length check runs. That leavesgl ipfs listvulnerable to the same single-response memory exhaustion the guard is supposed to prevent.gl node statusrepeats the same pattern atcrates/gl/src/node.rs:311.
- P1: gate arweave anchor endpoint with optional_signature middleware - P2: record repo/owner_did on already-pinned CIDs (ipfs_pin + pinata) - P2: fold unprobed structural candidates into probe_limit (not dropped as hidden) - P2: save cursor at break point so opaque fallback doesn't loop to batch start - P2: replace Content-Length-only cap with streaming capped_response()
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] Filter the signed global anchor listing by repository visibility
crates/gitlawb-node/src/api/arweave.rs:52
The?repo=branch callsauthorize_repo_read, but an authenticated caller which omits that parameter falls straight through tolist_arweave_anchors(None, limit). That database path has no visibility predicate, so any unrelated DID can enumerate the owner, repo, refs, old/new SHAs, CID, and permanent transaction metadata of private or otherwise unreadable repositories. This leaves the privacy issue in the previously raised anchor review unresolved; construct the caller's readable repo set (and apply it before the limit), or do not expose a global listing. -
[P1] Do not advance the opaque continuation past the first deferred pin
crates/gitlawb-node/src/api/ipfs.rs:799
When the walk/probe bound is hit, this savesbatch[i], explicitly the first unprocessed row, intodb_cursorand returns it astruncated_cursor. Resumption uses the strict tuple predicate inlist_pinned_cids_for_repos((pinned_at, repo, sha256_hex) < cursor), so that exact row is never re-evaluated. A structural commit/tree at the MAX_WALKS or MAX_PROBES boundary can be visible, but is permanently omitted from the listing. Keep the cursor at the last processed row, or use an inclusive retry cursor, and cover the boundary with a visible structural pin. -
[P1] Bound and actually cancel the expensive visibility walks
crates/gitlawb-node/src/api/ipfs.rs:667
The timeout only drops the join handle and sets anAtomicBool; it does not interrupt the blockinggitchild currently running. In particular,blob_pathsfirst runs uncancellableassert_all_refs_are_commits/git rev-list, and its later cancellation check runs only between completedgit ls-treeinvocations. Because this newly expensive route has no rate or concurrency limit beyond a valid signature, repeated throwaway-DID requests can leave blocking-pool workers and git children running past the 60-second request timeout. Kill or time-bound child processes and apply a node-wide concurrency/rate budget for these walks. -
[P2] Do not overwrite the only repo association for a shared Git object
crates/gitlawb-node/src/db/mod.rs:2313
Phase 1 deliberately retainssha256_hexas the sole primary key, but this update replaces the one(repo, owner_did)association whenever an already-pinned object is pushed to another repository. For example, pushing a shared blob to public repo A and then private repo B makes the scoped query stop returning that pin to A's readers (and the reverse order hides it from B). A single mutable association cannot implement the new per-readable-repo listing; use a separate association table or defer this listing behavior until the compound-key migration is actually deployed. -
[P2] Preserve historical non-tip pins during the migration
crates/gitlawb-node/src/db/mod.rs:924
The v11 backfill can only associate a legacy pin when its CID equals abranch_cidsentry, which represents current ref tips. Existing pins for blobs, trees, and history do not meet that condition and are stamped with empty repo/owner values; the new listing only queries real readable(repo, owner_did)pairs, so those records disappear for their owners immediately after upgrade. Either migrate the required object-to-repo provenance, retain a safe compatibility path for unclassified legacy pins, or explicitly postpone the gated listing until that data exists. -
[P2] Query anchors using the normalized slug after authorization
crates/gitlawb-node/src/api/arweave.rs:55
authorize_repo_readaccepts a fulldid:key:.../nameowner and normalizes it while resolving the repository, but the subsequent database query uses the original query string. Anchors are persisted under the normalized short slug, so the authorized full-DID request returns an empty list while the equivalent short-DID request returns data. Pass the resolved normalized slug tolist_arweave_anchors(and add a full-DID scoped-listing test).
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Re-reviewed at 1d57860. I ran the changed paths both ways rather than reading them. The core visibility work holds up: jatmn's CLI-paging P2 is fixed (gl ipfs list now checks status before parse and pages next_cursor/truncated_cursor to exhaustion), the v11 migration is a proper new highest-version migration with an upgrade-path test and a deterministic backfill, the pin cursor is a real AEAD opaque token, and the empty-repo legacy-pin path is denied with a committed test. Three issues remain, one a read-boundary leak.
Findings
-
[P1] Filter the global anchor listing by per-caller read authorization
crates/gitlawb-node/src/api/arweave.rs:42
With?repo=absent, an authenticated caller falls straight through tolist_arweave_anchors(None, limit)— an unfilteredSELECT ... FROM arweave_anchors— even though the docstring promises "anchors for all repos the caller can read." I seeded a now-private repo's anchor and drove the endpoint as a stranger: the scoped path correctly denies (repo not found), but the global path returns that repo'snew_sha/cid. The only thing preventing a direct private-repo leak is the write-timeannouncegate, so any repo made private after it was anchored stays enumerable (ref history + CIDs) by the lowest-privilege authenticated caller, and nothing deletes existing anchor rows. Filter the global branch to repos the caller can actually read. This is the residual of CodeRabbit's "global listing bypasses visibility" thread — the anonymous branch was fixed, the authenticated one was not. -
[P2] Do not advance the cursor past a visible pin skipped by the walk budget
crates/gitlawb-node/src/api/ipfs.rs:800
Same P2 as last round; not fixed on this head. At theMAX_WALKSboundary Phase 3 savesdb_cursor = batch[i](the first unprocessed pin) and the keyset resume is strictly<(db/mod.rs:2447), so that pin is skipped on the next page and never returned. The comment ("db_cursor stays at the last processed pin so no row is skipped") contradicts the code. I reproduced it by shrinkingMAX_WALKSover real repos: a visible structural commit pin past the wall is dropped and never reappears across six pages following both cursor kinds, while the identical pin placed before the wall is returned. Savebatch[i-1](last processed) or make the boundary resume inclusive. -
[P2] Clamp the negative limit in list_anchors
crates/gitlawb-node/src/api/arweave.rs:52
let limit = q.limit.min(200)caps the top but not the bottom, so a negative?limit=reaches Postgres asLIMIT -1and 500s. Executed:?limit=-1, both scoped-as-owner and node-wide, returnsInternal("...LIMIT must not be negative"). Any authorized caller turns one GET into a guaranteed 500. Useq.limit.clamp(0, 200), matchinglist_pins.
Clear the P1 and the two P2s and this is close.
P1: - Filter global arweave anchor listing by per-caller readable repos - Use batch[i-1] (last processed) not batch[i] at walk/probe boundary - Kill git children on cancellation; add walk concurrency semaphore and per-DID rate limiter for IPFS pin listing P2: - Pass normalized slug (not raw query string) to anchor DB query - Clamp negative limit with clamp(0, 200) - Add pinned_cid_repos junction table for shared-object associations - Preserve historical non-tip pins with legacy-fallback entries
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Re-reviewed the current head a65a43bc on the merged state. This head isn't in a mergeable state yet: CI is red, and I reproduced it locally on a clean Postgres (cargo test -p gitlawb-node api::ipfs::tests -> 492 passed, 4 failed, same as test (stable)/test (beta)).
Findings
-
[P1] Restore the visible/structural pins the junction JOIN now drops
crates/gitlawb-node/src/db/mod.rs(pins listing query) +crates/gitlawb-node/src/api/ipfs.rs
Four pin-listing tests fail on this head, all on "authorized data must appear" assertions:test_structural_pin_included_withheld_blob_excluded(ipfs.rs:1583 "structural tree pin must appear for stranger", KTD3),test_orphan_empty_repo_pins_excluded(:1708 "legit pin must appear"),test_max_walks_plaintext_not_in_response_cursor(:1262 "must return the visible pin"), andtest_ipfs_cursor_guard(:1032). None of these test functions changed in the delta, so this is a production regression.Root cause: the listing was rewritten from
FROM pinned_cids WHERE (repo, owner_did) IN (...)toFROM pinned_cids p JOIN pinned_cid_repos pr ON pr.sha256_hex = p.sha256_hex WHERE (pr.repo, pr.owner_did) IN (...). EveryINSERT INTO pinned_cid_reposis gatedif !repo.is_empty() && !owner_did.is_empty(), and nothing writes a junction row for walk-discovered structural objects, so structural tree/commit pins and any non-junction-backed pin are silently dropped. It fails safe (over-withholds, no leak), but it breaks the listing and its regression tests. -
[P1, process] The confidentiality suite is currently vacuous
The only two green security tests on this head are the deny tests (test_stranger_denied_private_repo_pins,test_truncated_cursor_does_not_leak_hidden_sha) — they pass because the listing now returns less, not because the gate is proven. With every must-appear test red, the deny results are empty for the wrong reason, so the leak properties can't be certified here. This needs to be green before a confidentiality verdict is meaningful.
Prior findings status (from 1d57860c)
- Negative
limit-> 500 is fixed (q.limit.clamp(0, 200)). - The global Arweave anchor listing now scopes to
readable_repo_pairs+list_arweave_anchors_for_repos, which is the right shape; I'll re-verify it by execution once CI is green (a red branch with vacuous listings can't confirm it). - The visible-pin-past-
MAX_WALKSitem is not resolved —test_max_walks_plaintext_not_in_response_cursoris one of the four failures.
Get CI green (the junction JOIN needs to include the structural and legacy pins the tests expect), then I'll re-run the full deny/allow/structural matrix for the confidentiality verdict.
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 repos out of the global anchor listing
crates/gitlawb-node/src/api/arweave.rs:90
The global branch builds its readable set withlist_all_repos(), even though that API deliberately includes quarantined and mirror rows and is documented as unsuitable for listing surfaces.readable_repo_pairsthen treats a public quarantined mirror as readable and passes its pair tolist_arweave_anchors_for_repos, exposing its repo/ref/SHA/CID anchor metadata to any signed caller before an operator releases it. Use the deduped, quarantine-filtered repo list (as the pin listing does) before applying visibility checks. -
[P1] Do not release a walk permit while the timed-out Git walk is still running
crates/gitlawb-node/src/api/ipfs.rs:659
On the 60-second timeout, this drops the join handle and then the semaphore permit, but the blocking task can still be inassert_all_refs_are_commits: itsfor-each-refandcat-filecommands are not cancellation-aware. A caller can therefore repeatedly time out path-scoped walks and admit new ones while the old Git children/blocking workers remain alive, defeating the new node-wide concurrency bound. Make every subprocess in the walk killable/polled and retain capacity until its task has actually terminated (or put the work behind an owned bounded queue). -
[P2] Return a resumable continuation when the first pin has to be deferred
crates/gitlawb-node/src/api/ipfs.rs:824
Ifrepo_store.acquirefails, a walk times out, or all permits are occupied forbatch[0], Phase 3 intentionally leavesdb_cursorunset. The response is thentruncated: truewith neither cursor. Bothgl ipfs listandgl node statustreat that as end-of-list, so every older visible pin is silently omitted. The cursor protocol needs a safe retry/continuation representation for the pre-row state rather than signalling a terminal page. -
[P2] Reject a zero walk-concurrency configuration
crates/gitlawb-node/src/config.rs:242
GITLAWB_WALK_CONCURRENCY_LIMIT=0is accepted and builds a zero-permit semaphore. Every path-scoped pin listing then failstry_acquire_owned, hits the cursorless truncated response above, and becomes empty to the CLI. Add arange(1..)parser (or explicitly define and implement a useful zero-disabled mode) so this configuration cannot silently disable the endpoint. -
[P2] Do not display an incomplete pin traversal as an authoritative total
crates/gl/src/node.rs:257
fetch_pinsbreaks on its page cap, row cap, or a repeated cursor, but then returnsPins(total)and the status dashboard rendersPinned CIDs: <total>. A buggy node or a listing larger than the safety bounds therefore produces a credible undercount with no incomplete/unavailable indication. Carry an incomplete state throughPinsPaneland show that the count is partial (or mark the panel unavailable). -
[P2] Enforce the documented authentication response for global anchors
crates/gitlawb-node/src/api/arweave.rs:81
The PR describes this route as requiring authentication, but an anonymous global request returns200 {"anchors":[],"count":0}. That differs from the pin index's explicit401and leaves clients unable to distinguish an empty authorized result from a missing credential. ReturnUnauthorizedbefore the global query when no authenticated DID is present.
- P1 (arweave): return 401 for anonymous anchor requests - P1 (ipfs): hold semaphore permit inside spawn_blocking closure - P2 (ipfs): fallback to batch[0] cursor for deferred first pin - P2 (config): reject walk_concurrency_limit=0 via clap range - P2 (arweave): return Unauthorized error for anonymous requests - P2 (node): carry incomplete flag through PinsPanel for truncated traversals
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] Do not backfill pin ownership by content CID
crates/gitlawb-node/src/db/mod.rs:933
Migration v11 joinspinned_cidstobranch_cidsoncid, although the CID is derived from the raw object bytes and does not encode the Git object type. A private blob can deliberately contain the same bytes as a public commit, giving the two different Git OIDs the same CID. On upgrade the private pin is then associated with the public repo and copied intopinned_cid_repos, so a public caller can list its private SHA/CID. Backfill by the Git SHA (or another type- and repo-bound association), rather than the content CID. -
[P1] Enforce authentication before handling a scoped anchor query
crates/gitlawb-node/src/api/arweave.rs:55
The?repo=branch callsauthorize_repo_readbefore the anonymous check. That helper allowscaller = Nonefor a public repo, so an unauthenticated request to/api/v1/arweave/anchors?repo=<public-owner>/<repo>succeeds and enumerates its anchor metadata. This contradicts the endpoint documentation and the PR's authentication contract; reject missing authentication before either listing branch. -
[P1] Make the complete visibility preflight cancellable
crates/gitlawb-node/src/git/visibility_pack.rs:25
The new 60-second timeout only reachesrun_git; every walk first runsassert_all_refs_are_commits, whosefor-each-refandcat-filechildren block inoutput/wait_with_outputwithout observingcancelled. A slow or ref-heavy path-scoped repo can therefore retain its detached blocking task and semaphore permit after the handler timed out. Four such requests consume the default global capacity and make subsequent path-scoped listings continually defer/truncate. Thread the cancellation/kill handling through these two subprocesses too. -
[P2] Resume at, rather than past, a deferred first pin
crates/gitlawb-node/src/api/ipfs.rs:825
When the first batch row cannot obtain a walk permit, its walk/acquire fails, or a walk/probe bound is reached, Phase 3 storesbatch[0]asdb_cursor. The listing SQL resumes strictly before that tuple, so the returned opaque cursor skips the unprocessed pin instead of re-evaluating it. A temporary capacity failure can permanently omit an otherwise visible pin from clients that correctly follow the continuation. Preserve the incoming cursor or encode a retry state that resumes inclusively at the deferred row. -
[P2] Keep the CLI traversal within the server's pagination quota
crates/gl/src/ipfs_cmd.rs:116
gl ipfs listrequests the 50-item default page size, while the new per-DID limit permits only 60 requests per hour. A normal node with more than 3,000 visible pins receives a 429 on page 61 and the command aborts, despite promising to list all CIDs. Request the maximum supported page size and/or make the server quota pagination-aware, with an explicit partial result if completion cannot be obtained. -
[P2] Do not present an incomplete empty traversal as "no pins"
crates/gl/src/ipfs_cmd.rs:56
list_pins_paginatedcan return(empty, incomplete)after its page/cursor guard fires, butcmd_listunconditionally prints "No IPFS pins".gl node statushas the same issue atcrates/gl/src/node.rs:343, converting an incomplete zero count intoPinsPanel::Empty. A buggy node or a server-side deferred-first-row response can therefore be displayed as an authoritative empty listing. Carry the incomplete state through the empty branches and render it as partial/unavailable. -
[P2] Do not write untrusted node error bodies directly to the terminal
crates/gl/src/ipfs_cmd.rs:149
The new pagination path interpolates a configured node's response body directly intoanyhowerrors. A malicious or compromised node can return ANSI/OSC control sequences in an otherwise bounded 4xx body and inject terminal output when the CLI renders the error. Omit the body or sanitize it to a safe printable excerpt before including it in diagnostics.
- P1 (db/mod.rs): backfill pinned_cids repo by sha256_hex=bc.sha, not cid alone - P1 (arweave.rs): reject unauthenticated requests before the ?repo= branch - P1 (visibility_pack.rs): make assert_all_refs_are_commits cancellable - P2 (ipfs.rs): resume at batch_cursor when first pin is deferred - P2 (ipfs_cmd.rs): request limit=200; sanitize error bodies; handle incomplete empty - P2 (node.rs): return Unavailable when count==0 && incomplete
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 deferred path-scoped pins resumable
crates/gitlawb-node/src/api/ipfs.rs:663
If the first sorted path-scoped pin cannot acquire a walk permit (or its walk fails), Phase 3 restores the incoming cursor, which isNoneon the first page. The response is therefore{pins: [], count: 0, truncated: true}with neither continuation cursor. The CLI consumers ignoretruncatedwhen both cursor fields are absent and report an authoritative empty listing. For a later deferred row, the response cursor instead points to the preceding row, so the next request retries the same failing row and cannot reach older pins. Return a durable retry/continuation state (or an explicit unavailable response), and have both clients treat cursorless truncation as incomplete. -
[P1] Keep full pagination usable with the default rate limit
crates/gitlawb-node/src/config.rs:245
The new 60-requests/hour per-DID limit is charged for every cursor continuation.gl ipfs listuses 200-row pages, so a normal listing with more than 12,000 visible pins receives a 429 on page 61 and exits with an error;gl node statusleaves the 50-row default and becomes unavailable after 3,000 pins. The server quota or client behavior needs to allow a full traversal, or the clients must surface a deliberate partial result rather than claiming/failing a complete listing. -
[P1] Bound total visibility-walk work per listing request
crates/gitlawb-node/src/api/ipfs.rs:650
Each walk has a 60-second timeout, but a single request can serially attemptMAX_WALKS(50) distinct path-scoped repos. Four signed requests can consequently hold the four global permits for roughly 50 minutes, preventing normal callers from performing visibility walks. Stop the request after a bounded total deadline/work budget instead of applying only a per-walk timeout. -
[P2] Do not silently drop legacy non-tip pins in migration v13
crates/gitlawb-node/src/db/mod.rs:992
The migration says its empty(repo, owner_did)association keeps historical blobs, trees, and old commits listable, butlist_pinned_cids_for_reposunconditionally filtersCOALESCE(pr.repo, p.repo) != ''. Those migrated records are therefore invisible to every authenticated caller, while the new CLI reports a complete list. Preserve a safe association/provenance path for these records or explicitly retain an authorized compatibility behavior. -
[P2] Add a non-sybil cost limit for the expensive listing
crates/gitlawb-node/src/api/ipfs.rs:444
The new throttle is keyed only byAuthenticatedDid, but any valid signeddid:keyis accepted without registration or ownership gating. An attacker can mint a new DID per request and bypass the 60/hour bucket, still forcing repeated repository/rule queries and up to the visibility-walk boundary. Add a trusted-source/IP or global cost limiter (or require a non-sybil capability) alongside the DID bucket. -
[P2] Return each pinned object only once when it belongs to multiple readable repos
crates/gitlawb-node/src/db/mod.rs:2497
The junction table correctly stores one association per repo, but the listing joins every matching association without grouping or deduplication. A reader of two repos sharing the same Git object receives two rows for one SHA/CID, and bothgl ipfs listandgl node statusoverstate the number of pinned objects. Deduplicate by pinned object while retaining a visibility-safe association for the authorization check. -
[P2] Put an aggregate memory bound on
gl ipfs list
crates/gl/src/ipfs_cmd.rs:111
The 64 MiB cap applies to each response, but the paginator retains every parsed JSON value inall_pinsuntil its one-million-row cap. A malicious node can send many legal, under-limit pages with oversized fields and fresh cursors, making the CLI retain hundreds of GiB before the row limit is reached. Cap total retained bytes/output or render pages incrementally with a strict aggregate budget. -
[P2] Avoid slicing untrusted UTF-8 on a byte boundary
crates/gl/src/ipfs_cmd.rs:94
sanitize_bodyusesbody.len()and then slices&body[..500]; an error body with a multibyte character crossing byte 500 panics the CLI instead of producing a sanitized diagnostic. Truncate on character boundaries before filtering. -
[P2] Stop paginating after repeated empty pages without progress
crates/gl/src/ipfs_cmd.rs:197
Both pagination consumers accept an emptypinspage with a fresh cursor indefinitely; their cycle guard only rejects an exact repeated cursor pair. A buggy or hostile node can return fresh cursors and empty pages for 10,000 signed requests, with no useful result. Enforce a small consecutive-empty-page/progress bound and report the traversal incomplete.
- P1 (ipfs.rs): durable cursor from last batch pin when all deferred - P1 (ipfs_cmd.rs): handle 429 as incomplete; raise page size to 500 - P1 (ipfs.rs): 120s total listing deadline for visibility walks - P2 (db/mod.rs): LEGACY sentinel for orphaned pins + inclusive filter - P2 (state.rs + ipfs.rs): global rate limiter alongside per-DID bucket - P2 (db/mod.rs): deduplicate pinned objects by sha256_hex - P2 (ipfs_cmd.rs): 512 MiB aggregate memory bound; UTF-8-safe truncation; consecutive-empty-page guard (max 5)
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Re-reviewed the merged state at f5a4fbb. This resolves both blockers from my last pass: the four must-appear pin tests are green again alongside the two deny tests, so the confidentiality suite is load-bearing rather than vacuous, and CI is green. I re-ran the leak invariants against this head by mutation and drove the paths that regressed before: the AEAD truncated cursor is locked (flipping it to base64-plaintext turns test_max_walks_plaintext_not_in_response_cursor red), the repo='' orphan is excluded (a seeded LEGACY junction row returns nothing to a stranger), and the arweave ?repo= collision (private canonical + public bare mirror sharing a slug) is denied at authorize_repo_read before the query runs. The core is sound. What remains is resource-bounding, one migration-rollout gap, and test coverage.
Findings
-
[P2] Keep
pinned_cids.repo/owner_diddefaulted (or nullable) until old writers drain
crates/gitlawb-node/src/db/mod.rs:958(migration v11)
v11 addsrepo/owner_did, backfills them, and doesALTER COLUMN ... SET NOT NULLin the same migration, with noSET DEFAULT. On a shared-Postgres rolling deploy, a still-running pre-v11 binary whoserecord_pinned_cidinserts(sha256_hex, cid, pinned_at)will hit a NOT NULL violation the moment the new node migrates the DB, failing pin writes for the upgrade window. This is the contract half of expand/contract landing before old writers are drained. Either addALTER COLUMN repo SET DEFAULT ''/owner_did SET DEFAULT ''in v11, or move the twoSET NOT NULLstatements into a later migration shipped after the fleet is fully upgraded. -
[P2] Bound Phase 2 the same way Phase 1 is bounded
crates/gitlawb-node/src/api/ipfs.rs:831
The Phase 1 visibility walks acquire awalk_semaphorepermit and honorlisting_deadline; the Phase 2git cat-file --batch-checkprobes do neither.batch_object_typesis spawned with no permit and no deadline check, so under concurrent load the probe subprocesses aren't capped by the semaphore, and a request that spends its wholeMAX_PROBESbudget can run past the 120s deadline that Phase 1 respects. Acquire a permit into the probe closure and add thelisting_deadlinecheck at the top of the Phase 2 loop, folding the current repo's candidates intoprobe_limiton the deadline path as theMAX_PROBES-exhausted branch already does. -
[P2] Wrap
repo_store.acquire()in a timeout so a stall doesn't pin a walk permit
crates/gitlawb-node/src/api/ipfs.rs:699
The walk permit is acquired at line 686 and thenacquire()runs at 699 before the timeout-wrapped walk. A slow Tigris fetch insideacquire()therefore holds the permit unbounded, since only the walk itself (724) has atokio::time::timeout. Wrap theacquire()call the same way and, on elapsed, take the existing acquisition-failed branch (page_truncated, empty allowed set) so the permit drops promptly. -
[P2] The
'LEGACY'sentinel rows are fetched every request but never emitted
crates/gitlawb-node/src/db/mod.rs:2504
The v13 migration inserts arepo='LEGACY'junction row per orphan pin "to show to every authenticated caller," and the listing query returns them viaOR COALESCE(pr.repo, p.repo) = 'LEGACY'. But the handler dropsrepo='LEGACY'as an unknown slug (repos_by_slugis keyed{short}/{name}), so the rows are always discarded — I confirmed a stranger sees nothing. So the migration's stated intent isn't realized and every request pays to fetch and skip them, which can also push visible pins past the scan bound. Either drop theOR ...='LEGACY'clause and the v13 insert, or special-caserepo=='LEGACY'in the handler to emit them; don't leave it fetching rows it always throws away. -
[P2] Add tests for
arweave.rs list_anchors
crates/gitlawb-node/src/api/arweave.rs:51
The anchor listing gained the same auth + per-row visibility gating as the pin listing but ships with zero committed tests. Given this endpoint's history, it should carry at least: anonymousauth=Nonereturns 401 before any DB work; a stranger?repo=on a private repo is denied (not served the anchors); and the global path scopes toreadable_repo_pairs(a stranger doesn't receive anchors from repos unreadable at root). The code reads correct on all three, but there's no RED/GREEN guard. -
[P3] Charge the amplification-prone path on a real bound, not a shared node-wide bucket
crates/gitlawb-node/src/api/ipfs.rs:446
The global limiter keys on the constant string"global", so the whole node shares one ~120/hr pin-listing budget. Identities are permissionless, so a caller minting two DIDs can spend the shared bucket and 429 every legitimate caller for the window. The per-DID limiter plus the walk semaphore already bound the expensive work; consider replacing the shared request-count cap with a global in-flight-walk concurrency cap so honest paginating callers aren't denied by unrelated traffic. -
[P3] The gl client aggregate-memory guard is dead code
crates/gl/src/ipfs_cmd.rs:227
The 512 MiB aggregate cap only adds toall_pins_byteswhen a pin is a JSON string, but pins are objects, soas_str()is always None and the guard never fires. Measure the real size (serde_json::to_string(pin).map(|s| s.len())) and mirror it innode.rs::fetch_pins.
The confidentiality direction is solid and the deny suite is now doing real work. Address the migration-rollout gap and the two resource bounds and I'm happy to approve.
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] Do not put the pin-list rate limit in a single global bucket
crates/gitlawb-node/src/main.rs:388
Every signed request debits the same"global"key before the per-DID check, but production gives that bucket only2 * GITLAWB_IPFS_LIST_RATE_LIMITentries per hour (120 with the default). Two throwaway identities can consume all 120 successful requests; even one identity can drain the global bucket by continuing to make requests after its per-DID quota, because the global debit happens first. Then/api/v1/ipfs/pinsreturns 429 for every other user for the rest of the hour. This endpoint is now required by bothgl ipfs listand the signed status panel, so use a viable independently configured global budget and/or key the overload protection by a trusted client source rather than allowing one caller to exhaust a node-wide bucket. -
[P2] Deduplicate shared pin associations before applying the page cursor
crates/gitlawb-node/src/db/mod.rs:2495
The new junction table intentionally permits one SHA to be associated with multiple readable repos, but this query orders and keyset-pages those association rows and only removes duplicate SHAs in a fresh in-memoryHashSetafterwards. Withlimit=1, a pin shared by two readable repos is returned from the higher-sorting association on page one; the next cursor advances only past that association, so page two returns the same SHA/CID through the second association.gl ipfs listappends both pages, producing duplicate pins/counts and burning the bounded pagination budget. Select one association per SHA before theLIMIT/keyset predicate, or make the cursor and response contract association-aware without returning the object twice. -
[P2] Enforce the aggregate response-memory limit for object-shaped pins
crates/gl/src/ipfs_cmd.rs:223
The new 512 MiB guard incrementsall_pins_bytesonly when a pin is itself a JSON string, whereas the server emits each pin as a JSON object. Consequently a hostile or malfunctioning node can return many individually capped object pages with fresh cursors and every retainedValuecontributes zero to the aggregate check beforeall_pins.extend(pins). Account for the decoded/serialized object data (or retain a bounded typed representation) before appending so the advertised aggregate cap actually constrains client memory.
…SET DEFAULT in v11, arweave tests
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] Add a non-sybil admission limit before pin-list work
crates/gitlawb-node/src/api/ipfs.rs:444
The new limiter is keyed only by the signed DID, butoptional_signatureaccepts a freshly generateddid:keywithout any registration or capability check. An attacker can rotate keys to bypass the 60/hour bucket and keep the four visibility-walk permits occupied (while also repeating the global repo/rule queries), forcing legitimate path-scoped listings to defer or degrade. Apply a trusted-source/global/in-flight cost limit before the enumeration; the per-DID limit can remain a secondary guard. -
[P1] Return a continuation when the first path-scoped pin is deferred
crates/gitlawb-node/src/api/ipfs.rs:679
With all walk permits occupied, the first row is deferred and Phase 3 restoresdb_cursorto the incoming cursor, which isNoneon page one. The successful response is thereforetruncated: truewith nonext_cursorortruncated_cursor; both GL consumers treat it as a completed empty listing. Temporary saturation can consequently report zero accessible pins and gives the client no way to resume. Return an opaque retry state for the initial position (or an explicit retriable failure), and treat a cursorless truncation as incomplete in both consumers. -
[P2] Do not select one shared-pin association before path visibility is checked
crates/gitlawb-node/src/db/mod.rs:2502
The SQL deduplicates shared objects by choosing the alphabetically first root-readable repo association, andlist_pinsthen performs the path-scoped object check only for that one repo. If that association reaches the object only through a denied path while another readable associated repo exposes it through an allowed path, the handler drops an object the caller is entitled to list. Defer deduplication until after per-association path visibility is evaluated, or choose an association proven visible. -
[P2] Make the client traversal fit the new page quota
crates/gl/src/node.rs:272
gl node statusrequests the 50-item default page and treats the resulting 429 as unavailable, so a normal node with more than 3,000 pins cannot show its count under the new 60-requests/hour per-DID limit.gl ipfs listtries to request 500 rows atcrates/gl/src/ipfs_cmd.rs:144, but the server clamps pages to 200 atcrates/gitlawb-node/src/api/ipfs.rs:483, so it likewise cannot complete inventories over 12,000 pins in an hour. Align the server page/limit policy with the clients and retain/report a partial status on quota exhaustion. -
[P2] Bound fresh-cursor empty pages in the status panel
crates/gl/src/node.rs:323
Unlikegl ipfs list,fetch_pinshas no consecutive-empty-page guard. A buggy or malicious node can reply with empty pages and fresh cursors, bypass the exact-token cycle check, and makegl node statussend up to 10,000 signed requests before it gives up. Apply the same small empty-page/progress bound used by the list command and render the result as incomplete.
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Re-reviewed 68833c26 on the merged state, concurring with jatmn's block. The rate-limiter rework and the SQL dedup are the right direction, and my prior resource-bound findings (the Phase 2 permit and deadline, the acquire() timeout, the v11 SET DEFAULT, the arweave tests) are resolved. But the new dedup introduces an under-return I reproduced against the real handler, and the admission-limit finding holds.
Findings
-
[P2] Order the shared-pin dedup after per-association visibility, not before
crates/gitlawb-node/src/db/mod.rs:2502
TheROW_NUMBER() ... WHERE rn = 1collapses a shared object to its alphabetically-first root-readable association, andlist_pinsthen runs the path-scoped check for that one repo only (api/ipfs.rs:639). I reproduced the drop through the real handler: the same blob pinned inaaa(a/secret/**deny rule, sorts first) andbbb(no rule), both public, is returned to a stranger when associated withbbbalone, but disappears once theaaaassociation is added, because the dedup picksaaaand its rule hides the blob. The caller loses a pin they can read viabbb. Defer the dedup until after per-association visibility is evaluated, or pick an association proven visible; a committed test seeding one object across two readable repos with divergent path rules locks it. -
[P1] The pin-list enumeration needs a cost limit a fresh DID cannot rotate past
crates/gitlawb-node/src/api/ipfs.rs:444
Concurring with jatmn here. With the global bucket removed, the only admission gate is the per-DID limiter, andoptional_signatureaccepts any self-minteddid:key, so key rotation drives the two per-request enumeration queries and holds the walk permits with no un-rotatable cap. The per-DID limit is fine as a secondary guard, but the expensive path needs a global, in-flight, or trusted-source bound.
jatmn's remaining findings (page-one truncation without a resumable cursor, and the two client paging bounds) stand as written; I concur with the block. @Gravirei the dedup direction is sound, just reorder it after the per-repo visibility check, and I can share the repro test.
- [P1] Add global non-sybil rate limiter (fixed key, max_keys=1) before pin-list enumeration; DID rotation can no longer bypass cost guard - [P1] Keep Phase 1 fallback cursor when batch_cursor is None on page 1, so deferred first-row truncation produces a retry-able truncated_cursor - [P2] Remove ROW_NUMBER dedup from list_pinned_cids_for_repos SQL; deduplicate by sha256_hex in Phase 3 after per-association path visibility is evaluated - [P2] Request limit=200 (server max) in both gl ipfs list and gl node status; handle 429 as incomplete in status panel - [P2] Add consecutive-empty-page guard (MAX_CONSECUTIVE_EMPTY=5) to fetch_pins in gl node status, matching ipfs_cmd.rs
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Re-reviewed 49b5554b on the merged state, driving the changed paths through the real handler rather than reading them. The client paging fixes land, the non-sybil admission limit is the right shape, and the truncated_cursor stays AEAD-opaque (no plaintext withheld sha). Four issues remain, two of them the same under/over-return class my last pass flagged, still open.
The root of the first two: moving the shared-pin dedup out of SQL and into a per-request seen_shas set cannot dedup correctly across either the visibility boundary or the page boundary. Both are reproduced against the real handler.
Findings
-
[P2] Order the shared-pin dedup after per-association visibility
crates/gitlawb-node/src/api/ipfs.rs:930
seen_shas.insert(sha)marks the sha seen before the path-visibility decision (939-970), so a hidden association that sorts first suppresses a later visible one. I drove it through the real handler: the same blob pinned in a/secret/**repo (zhid, laterpinned_atso it sorts first underORDER BY pinned_at DESC) and a no-rules repo (avis), both public, returns an empty list to a stranger who can read it viaavis. Marking the sha seen only on emit flips the repro GREEN with the full ipfs suite (7/7) still passing. -
[P2] Dedup shared-pin associations in a way that survives the page boundary
crates/gitlawb-node/src/api/ipfs.rs:930
seen_shasis rebuilt per request, so an object with two readable associations that lands at a full-page boundary is emitted twice: page 1 setsnext_cursorto its first association, page 2 resumes< thatwhich still admits the object's tail association, and the freshseen_shasre-emits it. Reproduced withlimit=1and one object pinned in two readable repos: it appears on both pages. The removed SQLROW_NUMBERguaranteed one row per sha across the whole result; the per-request set cannot. This and the finding above are the same root cause (handler-side per-request dedup); the durable fix is to dedup in the query (visibility-aware) or advance the cursor past all of an emitted object's associations. -
[P2] Do not resume past a visible pin when the first path-scoped pin defers on page 1
crates/gitlawb-node/src/api/ipfs.rs:783
On page 1 with the first pin deferred (walk permits exhausted,walk_limit_idx == 0), Phase 3 breaks ati == 0; thebatch_cursor.is_some()guard (904) is a no-op because page-1batch_cursoris None, sodb_cursorkeeps the line-784 last-batch-row fallback and the emittedtruncated_cursorresumes past a visible pin later in the batch. Reproduced: with all four walk permits held, page 1 returns empty plus atruncated_cursor; following that cursor after the permits free returns empty too, and the visible pin never surfaces. The all-deferred resume must re-fetch the batch from its start (the retry-at-initial-position state from the original finding), not advance to the last batch row. -
[P2] Charge the global admission bucket after the per-DID check, not before
crates/gitlawb-node/src/api/ipfs.rs:446
The global limiter is consulted (andcheckcharges the bucket) at 446, before the per-DID 429 at 454. So a single DID already over its per-DID budget still charges the shared 1200/hr global bucket on every subsequent work-free request, drains it, and 429s every other caller for the rest of the window. Reproduced with a global cap of 3 and a per-DID cap of 1: one DID's rejected requests drain the bucket and a second, within-budget DID is then refused. Charge the global bucket only after the per-DID check passes; the anti-rotation property is preserved (each fresh DID passes per-DID at count 1, then charges global) while a single DID's contribution is capped at its own budget. -
[P3] Lock the non-sybil admission limit with a test
crates/gitlawb-node/src/api/ipfs.rs:446
The limiter is the right shape (fixed"global"key so DID rotation cannot bypass it,new_bounded(..., 1)bounds the map, checked before the enumeration), but it ships with no test. A committed test that rotates DIDs past the global cap and asserts the shed, plus one that reddens if the check is removed, keeps this P1 guard from silently regressing. -
[P3] Document the new rate-limit knobs
crates/gitlawb-node/src/config.rs:256
GITLAWB_IPFS_LIST_GLOBAL_RATE_LIMITand the earlierGITLAWB_IPFS_LIST_RATE_LIMITare enforced but undocumented (no README / .env.example). Add them so operators can tune the 1200/hr global and 60/hr per-DID caps.
The client paging work (#4/#5) and the admission-limit shape are solid; the four items above are the remaining blockers. @Gravirei I have repro tests for all four and can share them.
…at emit, SQL dedup, all-deferred retry, rate-limit test, docs
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] Deduplicate a shared pin only after evaluating each association's visibility
crates/gitlawb-node/src/db/mod.rs:2507
TheROW_NUMBERquery picks the alphabetically first readable repo association for each SHA beforelist_pinscan apply its path-scoped object check. If the same object is associated withaaa(where it is hidden by/secret/**) andbbb(where it is readable), the query returns onlyaaa; the handler filters it out and never evaluatesbbb. This under-reports pins that the signed caller is allowed to see. Make the dedup visibility-aware, or preserve every association until the handler has selected a visible one while retaining the page-boundary guarantee. -
[P2] Return a resumable result when the first visibility walk is deferred
crates/gitlawb-node/src/api/ipfs.rs:788
When every first-batch pin is path-scoped andwalk_semaphorehas no permit, Phase 1 setstruncatedwithwalk_limit_idx = 0but leaves bothbatch_cursoranddb_cursorasNone. Phase 3 preserves thatNone, so the response is{ "pins": [], "truncated": true }with neither cursor. Bothgl ipfs listandgl node statustreat absent cursors as completion and report an empty/zero listing, never retrying once capacity becomes available. Return a token that represents the initial position (or a retriable error), and cover this response shape in the clients. -
[P3] Bound retained pagination cursor data in the CLI
crates/gl/src/ipfs_cmd.rs:236
The response-size cap does not bound memory retained across pages: a node can return a fresh, near-64 MiBnext_cursorortruncated_cursoron every response. Each token is cloned into the next request and retained inseen_cursors, but cursor bytes are excluded fromall_pins_bytes; the 10,000-page loop can therefore consume hundreds of gigabytes.gl node statushas the same unbounded cursor set. Reject or mark the listing incomplete for oversized cursors and account all retained cursor/set bytes against a bounded budget.
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Re-reviewed the current head by driving the changed paths through the real code. Concurring with all three of your open findings, @jatmn — the two P2s reproduce.
Findings
-
[P2] Make the pin dedup visibility-aware (concur)
crates/gitlawb-node/src/db/mod.rs:2507
Confirmed by execution: seeded one object associated with two readable reposaaaandbbband calledlist_pinned_cids_for_reposdirectly. With theROW_NUMBER() ... ORDER BY COALESCE(pr.repo,p.repo) ASC/WHERE rn=1dedup it returns onlyaaa; dropping thern=1filter returns both. So the SQL collapses to the alphabetically-first association beforelist_pinsruns its path-scoped check — an object hidden underaaa's/secret/**but readable viabbbis under-reported. Either make the dedup visibility-aware or keep every association until the handler has selected a visible one. -
[P2] Emit a resumable token when the first walk is deferred (concur)
crates/gitlawb-node/src/api/ipfs.rs:788
Confirmed by trace: on page 1batch_cursorisNone, so when every first-batch pin is path-scoped andwalk_semaphorehas no permit, the response is{pins:[],truncated:true}with neithernext_cursornortruncated_cursor(thei==0arm at 914-916 only setsdb_cursorwhenbatch_cursor.is_some()). Bothgl ipfs listandgl node statustreat absent cursors as completion and report empty. Return a token representing the initial position (or a retriable error) and cover the shape in the clients. The same no-cursor skip exists on the siblingwalk_limit_idx != 0/probe_limit == 0path atpin[0]— close both arms together. -
[P3] Bound retained cursor bytes in the CLI (concur)
crates/gl/src/ipfs_cmd.rs:236
all_pins_bytescounts only serialized pin objects, so cursor bytes escape the aggregate cap.seen_cursorsretains every distinct pair and the cycle-detector keys on the exact pair, so a node returning a fresh ~64 MiB cursor per page can driveseen_cursorstoMAX_PAGES × 64 MiB. Account all retained cursor/set bytes against a bounded budget, or mark the listing incomplete on an oversized cursor.gl node statusshares the loop. -
[P3] Test the rate-limit reorder's stated benefit
crates/gitlawb-node/src/api/ipfs.rs:437
The reorder charges the global bucket only after the per-DID check, per the comment "a single over-budget DID does not drain the shared global bucket."global_rate_limiter_sheds_after_budget_exhausteduses three distinct DIDs and stays green if the two checks are swapped back, so nothing pins that property. Add a single-DID case that exhausts one DID's budget and asserts the global bucket is untouched.
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.