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 7 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
Gravirei
force-pushed
the
bug_fix_2
branch
2 times, most recently
from
bf9d690 to
069804a
Compare
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Re-reviewed on 069804a. The delta since my last pass is test-only (test_support.rs, +200/-84); the gates in list_pins and list_anchors are byte-identical to beafa8a where I already verified them, so this pass covers the new coverage.
All three gaps I flagged are closed, and each new test is load-bearing: with its production guard disabled in isolation, only that test fails, with the expected count.
- quarantine skip:
pins_list_excludes_quarantined_repos(0 vs 1) - path-scoped withheld-blob subtraction:
pins_list_withholds_path_scoped_blobs(secret OID leaks, 1 vs 2) - global per-row filter:
anchors_global_denies_non_reader(0 vs 1)
CodeRabbit's three nitpicks on 81a5772 (extract the pins/anchors fixtures, add the anonymous-public anchor case) are all addressed here. The full CI suite did not trigger on this head, so I ran it locally: fmt clean, clippy clean, full gitlawb-node suite green (325 passed).
One optional follow-up, not a blocker: list_pins has its own copy of the fail-closed "withheld walk failed, skip repo" arm with no test, while get_by_cid's copy is covered by ipfs_cid_walk_error_fails_closed.
Approving. @kevincodex1 this clears my earlier changes-requested; jatmn's CHANGES_REQUESTED also predates this head.
|
@kevincodex1 LGTM |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P2] Cover the production router wiring for the signed metadata endpoints
crates/gitlawb-node/src/test_support.rs:1848,crates/gitlawb-node/src/test_support.rs:2032
The new tests prove the handlers work when the test manually builds a tiny router and appliesoptional_signature, but they do not exerciseserver::build_router, which is the production wiring this PR had to fix. That matters here because/api/v1/ipfs/pinswas previously outside the optional-signature layer; ifserver.rsregressed or the route were left in the old.merge(...).layer(...)shape, the signedpins_list_allows_authenticatedtest would still pass while the real route would never attachAuthenticatedDidand would return401for every signed pins request. Please add at least one integration test throughbuild_routerfor the signed success path, especially/api/v1/ipfs/pins, so the route-layer fix is actually pinned. -
[P3] Complete CodeRabbit's request to clamp negative anchor limits
crates/gitlawb-node/src/api/arweave.rs:69
CodeRabbit's earlier negative-limit request is still valid on the current head:q.limit.min(200)caps only the upper bound, so/api/v1/arweave/anchors?limit=-1still passes-1intolist_arweave_anchors, where it is bound directly into PostgreSQLLIMIT. PostgreSQL rejects a negative limit, so a malformed query can turn this listing endpoint into an internal error instead of a bounded empty/small result. Please clamp the lower bound as well, for example withq.limit.clamp(0, 200), before callingstate.db.list_arweave_anchors.
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.
Thanks for the update. I rechecked the previously discussed paths and do not see any remaining actionable issues from my side.
@kevincodex1 LGTM
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Re-reviewed bf1066ef on the merged state, moving back to request-changes. Direction is right and most of the multi-round hardening holds: I re-ran the hidden-sha recovery attack against the XChaCha20Poly1305 cursor (no recovery, tamper rejected), the client loops are now capped, and the five open bot threads are all stale-fixed on this head. The blocker is a new availability regression in the structural-listing path.
Findings
-
[P1] Bound and offload the per-pin structural-object probe in
list_pins
crates/gitlawb-node/src/api/ipfs.rs:617
The non-blob visibility check callsstore::object_type(agit cat-file -tsubprocess) once per pinned object that is not in the caller's allowed-blob set and not inall_blobs, and it runs directly on the async worker instead of inspawn_blocking. Probes that return"blob"orNone(dangling blobs, bogus shas) never incrementpins.len(), so the page-fill break never fires and the loop scans the fullMAX_BATCHES * BATCH_SIZE(2000) rows. I seeded an on-disk public repo with a/secret/**rule and 2005 bogus-sha pins, then drovelist_pinsas a fresh stranger withlimit=1: 2000git cat-file -tspawns for acount:0response, all on one worker thread. Structural objects are pinned (pin_new_objects), so even a normal listing of a path-scoped repo with history spawns one per structural pin up tomax_visible.get_by_cidsolves the same need once per repo, before the walk, insidespawn_blocking(ipfs.rs:136,:157).rate_limit_by_didsoftens single-identity abuse, but identities are permissionless and it does not bound the per-request worker block. As a direction: batch the probe withgit cat-file --batch-check(one child per repo) and/or add an independent probe budget, and move it off the async runtime. -
[P2] Cover the structural-object path with a load-bearing test
crates/gitlawb-node/src/api/ipfs.rs:611
test_stranger_denied_private_repo_pinsandtest_orphan_empty_repo_pins_excludedgate the root check (:391) and the unknown-slug skip (:524), not the newvisible = t != "blob"logic. I fail-opened that block (visible = true) and both tests stayed green, because the private repo is filtered at root and therepo=''orphan is dropped by slug before the structural code runs. Add a case with a repo the stranger can read at root under a/secret/**rule, a real on-disk repo, a withheld-blob pin (must be excluded) and a structural pin (must be included), so it goes red if the gate is inverted. -
[P2] Do not silently drop visible pins past
MAX_WALKS
crates/gitlawb-node/src/api/ipfs.rs:544
When the 50-repo walk budget is exhausted the handler advancesdb_cursorpast the skipped pin and breaks, but if any visible pin was already collected the plaintextnext_cursoris emitted from that earlier (larger-keyset) row, so following it re-scans the same first 50 repos with a fresh budget. I reproduced it: with 50 disk-less path-scoped filler repos plus one on-disk repo holding a genuinely-visible pin past them, the visible pin never appears on any page (seen=1, anchor only); with 5 fillers the same pin is returned (seen=2). This is a completeness gap rather than a leak (truncated:trueis set, but the caller cannot reach the dropped pin), and it needs more than 50 path-scoped repos readable by one caller. -
[P3] Correct the cursor TTL comment
crates/gitlawb-node/src/api/ipfs.rs:268
create_opaque_cursordocuments "expire after 5 minutes" but the TTL is now 600s.
The crypto and gating are sound and confidentiality holds: the structural reveal is consistent with get_by_cid's KTD3 (trees and commits are served to any root reader, unchanged from main), the withheld blob stays withheld on both surfaces, and next_cursor never carries a withheld sha. @kevincodex1 holding at request-changes for the P1.
… guard, legacy pins P2: Walk budget exhaustion now signals truncated instead of silently dropping rows. db_cursor is advanced per-pin (not per-batch). When a new path-scoped repo exceeds MAX_WALKS, the loop breaks without advancing past that pin so the caller can retry with a fresh walk cache. P2: truncated_cursor is now retry-safe — the server stores the full (pinned_at, repo, sha256_hex) under a random UUID in an in-memory cache and never removes entries on lookup. Expired/unknown tokens simply don't match (fall back to start). P2: CLI advancement guard — gl ipfs list and gl node status now detect non-advancing cursors and error instead of silently looping to MAX_PAGES or returning partial counts. P2: Re-added legacy-pin compat path with explicit log warnings. Pre-Gitlawb#134 pins with empty repo/owner_did are returned to any authenticated caller with a tracing::warn! so operators can backfill repo context.
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Re-reviewed on f0d3701. Most of the long tail here is genuinely resolved: the anchor slug collision is closed by the (repo, owner_did) paired join, the repo='' orphan pins are excluded, both handlers clamp the limit, the client checks status before parsing and now pages to exhaustion, and the AEAD truncated_cursor on the MAX_BATCHES path is opaque. The gating is sound. One confidentiality leak remains, and it is the same class the PR set out to close.
Findings
-
[P1] Route the MAX_WALKS-wall cursor through the opaque token; it currently emits a withheld pin's sha256_hex in plaintext.
crates/gitlawb-node/src/api/ipfs.rs:618
When the walk budget is exhausted, line 618 sets response_cursor to skip_pos, the un-walked pin whose blob visibility was never resolved, and the emit branch at line 782 serializes it with encode_cursor (plaintext base64), preempting the AEAD path at line 784. Reproduced by execution: 51 distinct path-scoped repos with no repo on disk (so every pin is withheld), driven through list_pins as a stranger did:key, returns count:0, truncated:true, truncated_cursor:null, and next_cursor base64-decodes to2026-07-03T00:00:00Z|z6MkWALLOWNER.../wall0|wallsha00, the pinned_at, repo, and sha256_hex of a pin the caller was never shown. Same INV-13 leak the MAX_BATCHES path prevents, escaping via the walk-budget branch. It is also your still-open advancement finding, since the strict<resume then drops that pin entirely. Keep skip_pos in db_cursor only so the emit falls through to the AEAD token, and resume from the last classified pin (not skip_pos) so the visible-row loss is fixed too. Add a regression test seeding >= MAX_WALKS distinct path-scoped repos; the current cursor test only reaches the MAX_BATCHES branch. -
[P2] Stop
gl ipfs listfrom restarting at page 1 when a truncated_cursor expires.
crates/gl/src/ipfs_cmd.rs:88
cursor and truncated_cursor are both take()-n before the request, so on a 400 for an expired token the continue at line 94 re-requests with no cursor and re-collects earlier pages as duplicates. The comment says it restarts from the last visible cursor, but that value is already gone. Retain the last next_cursor across the truncated leg, or drop the token expiry since the AEAD already provides integrity. -
[P2] Bound the pin-listing walk cost under concurrency.
crates/gitlawb-node/src/api/ipfs.rs
Per request is bounded (MAX_WALKS=50, MAX_BATCHES=10, MAX_PROBES=200), but each walk is a spawn_blocking git subprocess with no timeout and no cross-request cache, so concurrent callers against deep repos can pin the blocking pool. A wall-clock timeout on batch_object_types and the blob walk, plus a small per-(repo, head) cache, would close it. -
[P3] Two hardening items. The v10 primary-key swap (sha256_hex to (repo, sha256_hex)) makes a pre-#134 binary's ON CONFLICT(sha256_hex) fail against a migrated schema; the blue/green caveat is in the comment, but a maintenance-window note would help. And the repo='' exclusion is structural (an absent map key) rather than an explicit guard, so a future refactor could regress it silently; an explicit
if pin.repo.is_empty()skip would pin it.
Only the P1 blocks.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the previously discussed paths and do not see any remaining actionable issues from my side.
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Re-reviewed on 2ed2507a. Approving. My prior blocker, the MAX_WALKS-wall plaintext next_cursor leaking a withheld pin's sha256_hex (INV-13), is closed, and I verified it load-bearing by execution rather than inspection: dropping response_cursor at the wall so the emit falls through to the AEAD token is the right fix, and re-adding that one line turns both your regression test and an all-hidden-ordering repro of mine RED (leaking the withheld pinned_at|repo|sha), GREEN with it removed. I also confirmed on this head that a visible pin sitting past the wall is still reachable exactly once across pagination (no skip, no dup), that repo='' stays denied to a non-owner, and that a timed-out walk fails closed.
Two non-blocking follow-ups, neither holds the merge:
Findings
- [P3] Correct the timeout comment
crates/gitlawb-node/src/api/ipfs.rs:643.tokio::time::timeoutdoes not cancel aspawn_blockingtask, so the blocking thread keeps running and the wall-clock timeout does not "prevent pinning the blocking pool on deep repos." It does bound the request's wait, which is a real improvement, so the code is fine; the comment just overstates it. The residual concurrent-walk amplification is pre-existing and better closed with a walk semaphore in a follow-up than here. - [P3] The expired-
truncated_cursorrestore is a no-opcrates/gl/src/ipfs_cmd.rs:76. The server emitsnext_cursorxortruncated_cursor, so on the 400 branchsaved_cursoris alreadyNone; the restore restarts at page 1 anyway and the cycle guard then stops it as incomplete. Either drop the dead branch or persist the lastnext_cursorseparately.
@kevincodex1 LGTM.
|
Thanks for staying with this one through all the rounds, @Gravirei. A lot of iteration here, especially hardening the pins/anchors gating and the cursor state machine, and it ended up in solid shape. Appreciated the responsiveness on each pass. |
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Re-reviewed on 370962e. The two commits since my last approval address the P3s I raised and introduce no regression, so I'm re-approving.
The delta is three changes: an accurate rewrite of the spawn_blocking timeout comment (timeout does not cancel the blocking task, so the thread runs to completion — correct), a clippy borrow fix, and the gl client cursor fix. The client fix now restores cursor from the last good next_cursor on an expired truncated_cursor (400) instead of restarting at page 1. I verified it load-bearing by execution: a test asserting the post-400 request resumes at the last cursor passes on this head and fails when reverted to the old saved_cursor(=None) body (which restarts at page 1). A first-page 400 with no truncated leg still bails rather than restoring, and 401/403 still surface as errors (INV-8). The server paths are untouched by this delta (the ipfs.rs change is comment-only); I re-ran the cursor-leak, legacy-pin-deny, path-scoped withholding, slug-collision, and negative-limit-clamp suites on this head and they are green.
Findings
-
[P3] The truncated-expired resume restores last_next_cursor, which points at the page before the truncated one, so that page is re-fetched and its visible pins can appear twice.
crates/gl/src/ipfs_cmd.rs. Self-limiting (the cycle guard breaks on the repeated truncated_cursor) and strictly better than the old page-1 restart, so non-blocking; a follow-up could resume exactly after the last emitted row. -
[P3] Add a committed test for the truncated-expired resume leg.
crates/gl/src/ipfs_cmd.rs. The existing tests cover happy / empty / no-identity / 401 only; nothing drives the 400-restore path that is the substance of this change, so a regression back to the page-1 restart would stay green. A test mirroring test_cmd_list_non_success_status_is_error_not_empty would lock it.
@kevincodex1 this clears my earlier notes; re-approved on the current head.
There was a problem hiding this comment.
Re-reviewed the delta since my last approval (the list_pins_paginated extraction and the new test). The security substance holds on 289b61b7: I confirmed by execution that a 401 (first leg and mid-pagination), 403, and 500 each surface as an error rather than an empty listing, that the request is still signed on every leg, and that the MAX_PAGES/MAX_ROWS bounds terminate. Two changes before this lands.
Findings
-
[P2] Make the resume test assert resumption; it currently passes with the resume logic broken.
crates/gl/src/ipfs_cmd.rs:283. Changing the 400-retry to restart at page 1 (cursor = None) leavestest_cmd_list_expired_truncated_resumes_from_last_next_cursorgreen, so it guards nothing beyond "the 400 mock was hit." Give the resumed leg a distinct pin and assert it appears, and assert the resume request carriedcursor=cGFnZS0xrather than the uncursored page-1 URL. Separately, the "matched most-recently-created first" comment is backwards: mockito matches earliest-created-first, so the two identicalcursor=cGFnZS0xmocks don't route the way the comment describes. -
[P2] Distinguish an expired
truncated_cursorfrom any other 400.crates/gl/src/ipfs_cmd.rs:113.status == 400 && had_truncatedretries on every 400 without inspecting the body, so a genuine 400 (malformed cursor, node bug, schema change) is retried; the run then endsOkand prints "No IPFS pins recorded" at exit 0, or spins to MAX_PAGES. Gate the retry on the node's expiry marker (the server returns 400 with{"error":"bad_request","message":"invalid or expired truncated_cursor"}, so match onmessage) and cap it with a counter separate frompages; bail on any other 400.
Non-blocking
- [P3] Confirm the output-format change is intentional.
crates/gl/src/ipfs_cmd.rs:54. The refactor dropped theIPFS pins (N) on {node}header, switched tosha cid pinned_at, and dropped thepinned_at[..19]trim. No test asserts rendered output, so it went in silently; if intended, a line in the PR body is enough. - [P3] Rename
_all_pinstoall_pins(:49). The underscore signals intentionally-unused, but it is read at:54and:60. - [P3]
incompletelatches on a recovered 400-retry, so a listing that fully recovers still warns "may be incomplete"; the single warning string also cannot tell MAX_PAGES apart from a mid-stream stall.
INV-8, signing, and the page/row bounds are verified intact on this head; the two blocking items are about the retry classification and the test that guards it.
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Non-blocking follow-up. My request-changes for the two P2s (the resume test that guards nothing, and the 400 retry that doesn't distinguish an expired truncated_cursor from any other 400) still stands. A deeper pass on list_pins_paginated surfaced three more items, all P3, none blocking. The node-side gating is solid: I re-ran the pins and anchors suites on this head and they pass, including the cross-owner slug-collision and cursor-no-leak cases.
Findings
-
[P3] Cap the per-response body size, or drop the MAX_ROWS protection claim.
crates/gl/src/ipfs_cmd.rs:83.MAX_ROWSonly bounds the accumulatedall_pinsacross pages;resp.json()at:123fully deserializes each page into aValuebefore the:127check runs. A single oversized response allocates unbounded heap regardless of the bound, so the comment's "protects against a hostile or buggy node" over-states what the guard does. Either bound the response body (a capped reader) or soften the comment to match. -
[P3] Stop the incomplete warning from firing on a clean recovery.
crates/gl/src/ipfs_cmd.rs:52. One expired-truncated_cursor400 latchesincomplete=truefor the rest of the run; when the restore then pages cleanly to a natural end the listing is actually complete, yet it still prints "stopped without advancing; results may be incomplete." The message is also wrong for theMAX_PAGESbranch, which did advance. Reset the flag once forward progress resumes, or split the recovered case from the genuine gave-up case with an accurate message. -
[P3] Note that the cycle guard only catches repeated cursor pairs.
crates/gl/src/ipfs_cmd.rs:137. It keys on the exact(next_cursor, truncated_cursor)pair, so a node that returns a fresh pair every page never trips it; onlyMAX_PAGES=10000stops the loop, and every leg is a fresh signed round-trip. It is bounded, so this is a robustness note rather than a hole, but a per-listing round-trip budget below 10k would cut the worst case a hostile node can impose.
The two P2s are what gate merge; these three can ride along or land in a follow-up.
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 an issue that still needs to be addressed.
Findings
- [P1] Keep the pin-index migration compatible with running nodes
crates/gitlawb-node/src/db/mod.rs:868
This migration drops the only uniqueness constraint onsha256_hex, but a pre-#134 node still runsINSERT INTO pinned_cids ... ON CONFLICT(sha256_hex) DO NOTHING. Since migrations run automatically at startup and the migration code explicitly supports blue/green and rolling deployments, the first upgraded instance makes every still-running older instance's IPFS/Pinata index write fail with “no unique or exclusion constraint matching the ON CONFLICT specification.” Those background errors are only logged, so pushes continue while their new pin mappings are silently lost. Please stage this as an expand/contract migration (or otherwise preserve the old conflict target until all old writers are gone) instead of requiring an un-enforced maintenance window.
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] Rebase on current
mainbefore applying the code fix
crates/gitlawb-node/src/api/repos.rs,crates/gitlawb-node/src/db/mod.rs,crates/gitlawb-node/src/server.rs,crates/gitlawb-node/src/test_support.rs
This head is based on26bc3f6, while currentmainis2109d08; a three-way merge has changed-in-both conflicts in these four files. They include the metadata-listing and visibility surfaces this PR changes, so please rebase on currentmainand have the resolved diff re-reviewed before addressing the finding below. -
[P1] Remove the global SHA uniqueness constraint from the per-repo pin schema
crates/gitlawb-node/src/db/mod.rs:877
Migration v10 retainsUNIQUE (sha256_hex)immediately before switching the primary key to(repo, sha256_hex). The new writers insert withON CONFLICT(repo, sha256_hex), so a Git object already recorded for repo A causes repo B's insert to fail on the still-global unique index rather than taking that conflict branch. Shared objects consequently are not recorded for later repos and disappear from their pin metadata. Please use a compatibility strategy that does not retain a conflicting global constraint while the per-repo writers are active, and add a regression that records the same SHA for two repositories.
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
-
[P1] Make the shipped migration support the new per-repo pin writes
crates/gitlawb-node/src/db/mod.rs:880
Migration v10 deliberately retains the globalsha256_hexprimary key, butrecord_pinned_cidnow inserts withON CONFLICT(repo, sha256_hex). A Git object already recorded for repo A therefore raises the old-PK violation when repo B records it; it is not the specified conflict target. Both pinning paths only log and continue, so repo B permanently lacks its ownership row and readers of B cannot see that pin. This is a normal shared-object case, not a migration edge case (the new migration test explicitly asserts the failed insert). Please make the rollout/write path handle both constraints, or defer the new per-repo writer until the contract migration is actually in place. -
[P1] Do not advance past the first pin blocked by the walk budget
crates/gitlawb-node/src/api/ipfs.rs:618
When the 51st distinct path-scoped repo is encountered, this stores that pin asdb_cursorand exits before checking it. The next query uses a strict< cursorpredicate, so the boundary pin is never revisited despite the comment saying the next request handles it. That can permanently omit an otherwise visible structural object or allowed blob. Preserve a resume position before the unprocessed row (or explicitly process it) so the truncated continuation cannot skip it. -
[P2] Resume rather than silently discard structural pins past
MAX_PROBES
crates/gitlawb-node/src/api/ipfs.rs:683
Phase 2 probes at most 200 structural candidates, but phase 3 treats every remaining unprobed candidate asfalseand still advances the database cursor. Since this path does not settruncatedor return a resumable cursor, a path-scoped repository with more than 200 pinned commits, trees, or tags produces a short response that claims to be complete and loses all subsequent structural pins. Stop before the first unprobed candidate and use the same visible/opaque continuation behavior as the walk cap. -
[P2] Bound a single pins response before deserializing it in both CLI consumers
crates/gl/src/ipfs_cmd.rs:127
The newMAX_ROWSchecks occur only afterresp.json()has materialized the whole response;gl node statushas the same sequence. A configured or malicious node can return one oversized JSON page and exhaust the CLI's memory despite the advertised one-million-row limit. Apply a response-body/content-length bound before JSON decoding (and cover both consumers), then retain the existing pagination caps. -
[P2] Recover or report partial state when the status panel's opaque cursor expires
crates/gl/src/node.rs:280
The server intentionally returns 400 for an expiredtruncated_cursor, but the status-panel loop maps every non-success response toUnavailableand drops its accumulated count.gl ipfs listalready retains the precedingnext_cursorand recovers from this normal token-expiry path. Givegl node statusequivalent recovery, or preserve and display the partial/incomplete result instead of reporting a healthy node as unavailable.
…(repo, sha256_hex) Rather than dropping the old sha256_hex PK and replacing it with a UNIQUE index on sha256_hex (which conflicts with the per-repo schema), this expand phase: • Keeps the old PK for pre-v10 ON CONFLICT(sha256_hex) compatibility. • Adds UNIQUE (repo, sha256_hex) for post-v10 ON CONFLICT(repo, sha256_hex). • Defers the PK switch to a future contract migration. The migration test now exercises both phases: 1. Verifies that duplicate SHA across repos is rejected in phase 1. 2. Applies the contract commands and verifies it succeeds in phase 2.
…, MAX_PROBES discard, response body bound, node status 400 [P1] Defer new per-repo conflict target to phase 2 record_pinned_cid and record_pinata_cid now target ON CONFLICT(sha256_hex) (the kept old PK) so both pre-v10 and post-v10 writers share the same conflict target during the expand phase. [P1] Walk budget boundary pin no longer skipped Push Some(false) for the unprocessed boundary pin and set walk_limit_idx to i+1 so the strict-< cursor never skips it permanently. [P2] Structural pins past MAX_PROBES not silently discarded Track the earliest unprobed structural candidate (probe_limit) and stop phase 3 before it, with page_truncated set for continuation. [P2] Bound response body before JSON deserialization Both CLI consumers (ipfs_cmd::list_pins_paginated, node::fetch_pins) read bytes first, check a 64 MiB cap, then deserialize. [P2] Node status panel recovers from 400 expired truncated_cursor Adds last_next_cursor tracking and restores from it on 400 + had_truncated, preserving already-accumulated counts instead of returning Unavailable.
…ively - Format ipfs_cmd.rs per cargo fmt (split long format! lines). - Replace break-on-unprobed-candidate with conservative hidden outcome + page_truncated flag, so already-visible pins later in the batch are still surfaced and the over-fetch test passes.
…s panel Only the 400 + had_truncated path should recover gracefully; transport errors and non-success rejection responses still return Unavailable.
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] Rebase this branch on the current
mainbefore resolving the functional changes
The current GitHub merge state isCONFLICTING. The base-to-head result also removes recentmainwork in the iCaptcha client, the creation-route flood brake, and the HTTP client. Please rebase and have the resolved full diff re-reviewed; choosing the branch side of those conflicts ships the regressions below. -
[P1] Do not rewrite an already-published migration version
crates/gitlawb-node/src/db/mod.rs:879
run_pending_migrationsskips every version already recorded inschema_migrations, but this replaces shipped v10 (ref_cert_unique_per_ref) with the newpinned_cidsschema. Upgraded nodes therefore never getrepo/owner_did, while the new pin writers and listing query use them and fail with undefined-column errors. They also retain v10's(repo_id, ref_name)uniqueness whileinsert_ref_certificateis now a plain insert, so the next update to an existing ref fails. Add a new migration for the pin schema and explicitly migrate the old certificate constraint instead of changing v10. -
[P1] Restore an IP-scoped brake for creation routes
crates/gitlawb-node/src/server.rs:84
This removes the outerrate_limit_by_iplayer from repo/register/fork/issue/PR creation and leaves only the per-DID bucket. A spammer can mint a freshdid:keyfor every request, so all requests reach signature verification, iCaptcha, storage, and the database without exhausting a bucket. Keep the former IP limiter or an equivalent cross-identity limiter. -
[P1] Keep the iCaptcha allowlist scoped to a full origin
crates/icaptcha-client/src/lib.rs:110
The trust check now compares only hostnames. A hostile node can advertisehttps://<configured-operator-host>:<attacker-port>/...; it passes both the allowlist andkey_trustedcheck, so the client sendsGITLAWB_ICAPTCHA_API_KEYto a different origin. Include scheme and effective port in the comparison as the removedorigin_keyimplementation did. -
[P1] Do not remove required iCaptcha proof-of-work handling
crates/icaptcha-client/src/lib.rs:194
The patch deletesChallenge.pow, the solver, andpowNoncefrom the answer request. An iCaptcha deployment that includes a required PoW challenge will reject every answer, preventinggland the remote from obtaining creation proofs. Restore the protocol field and submission (or add a verified service-version migration before removing it). -
[P1] Do not expose repo-scoped pin behavior while the schema still has a global SHA key
crates/gitlawb-node/src/db/mod.rs:2176
Phase 1 deliberately retains the globalsha256_hexprimary key, andrecord_pinned_cidconsequently doesON CONFLICT(sha256_hex) DO NOTHING. When a shared Git object is first pinned for repo A and then pushed to repo B, B's writer reports success but cannot record its(repo, owner_did)association; the new scoped listing permanently omits it for B. Keep global semantics until the compound key is actually deployed, or use a compatible association table. -
[P1] Restore bounded certificate queries
crates/gitlawb-node/src/db/mod.rs:2041
list_ref_certificatesnowfetch_alls every certificate. Both/certsand/eventscall it before serializing or applying the requested event limit, so a public high-churn repository turns a normal read (even?limit=1on events) into an unbounded database query and allocation. Reinstate SQL limits/keyset pagination and preserve bounded prefix resolution. -
[P2] Give pin pagination a budget compatible with its client
crates/gitlawb-node/src/server.rs:206
The new pins route reuses the creation limiter, which is configured for 10 requests per DID per hour. Both updated clients use the server's default 50-item page, so a caller with more than 500 visible pins receives a 429 on page 11 and cannot complete the advertised listing. Use a listing-specific limit or request a supported page size and report incompleteness. -
[P2] Preserve the first unprocessed pin at walk and probe budgets
crates/gitlawb-node/src/api/ipfs.rs:618
At both theMAX_WALKSandMAX_PROBESwalls, the code marks an unverified pin hidden and advancesdb_cursorthrough it. The resulting normal or opaque cursor resumes strictly after that row, so a readable structural pin can be omitted forever (or the prior visible cursor re-enters the same exhausted window). Stop before the first unprocessed pin and mint the continuation from the preceding safe cursor. -
[P2] Only recover from the documented expired-cursor response
crates/gl/src/ipfs_cmd.rs:115
Every 400 after atruncated_cursoris treated as expiry and retried fromlast_next_cursor; the status panel duplicates the behavior. A malformed cursor, protocol change, or node bug can therefore be hidden as a partial or successful listing. Parse and require the server's explicit expired-token error marker, and surface every other 400. -
[P2] Enforce the response-size limit before buffering the body
crates/gl/src/ipfs_cmd.rs:127
resp.bytes()fully buffers the response before the 64 MiB length check runs;gl node statushas the same sequence. A malicious or misconfigured node can exhaust process memory with a single pins page despite the claimed cap. Reject an excessiveContent-Lengthand stream through a bounded reader before JSON deserialization in both consumers.
P1 fixes: - Record_pinned_cid: DO UPDATE repo/owner_did instead of DO NOTHING so shared SHAs appear in scoped listings for each pinning repo - Record_pinata_cid: same DO UPDATE coverage + wrapper for callers without repo context P2 fixes: - MAX_WALKS: stop before the first unprocessed pin (walk_limit_idx = i) instead of marking it hidden and advancing past it - MAX_PROBES: track probe_limit and stop Phase 3 before the first unprobed structural candidate; uses walk_limit_idx.min(probe_limit) - Rebuild list_pins with proper auth, pagination, and missing helpers (derive_cursor_key, create_opaque_cursor, decode_opaque_cursor, batch_object_types) - Reject large responses via Content-Length before buffering - Only retry on 400 when server explicitly says 'expired truncated_cursor'
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.
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.