fix(node): gate /ipfs/pins and /arweave/anchors behind authentication (#121)

[Gravirei]

Closes #121 (anonymous-denial half; see "Out of scope" below for the residual class).

Summary

Two node-wide metadata endpoints accepted unsigned requests, and the ?repo= variant on the Arweave anchors endpoint also accepted signed non-readers. This PR closes the anonymous-denial path on both endpoints and the authorization hole on the scoped anchor path. A follow-up commit also fixes a false-empty-page failure mode on the scoped anchor path and tightens the regression that pins the fix.

Changes

/api/v1/ipfs/pins (list_pins):

• Moved ipfs_pins_routes onto its own Router and applied optional_signature to that router before the merge with ipfs_cid_routes. The previous shape layered optional_signature on the post-merge router, which (per the axum-layer-vs-merge-pitfall memory entry) only covers routes added before the .layer() call and silently leaves merged routes outside the layer.
• Handler rejects requests without a verified AuthenticatedDid with 401.

/api/v1/arweave/anchors (list_anchors):

• Round 1: added the optional_signature middleware to the route — it was missing entirely before. When ?repo=<owner>/<name> is provided, parse the slug and call the canonical authorize_repo_read helper before the SQL query. Missing repos, quarantined mirrors, and signed non-readers all collapse to the standard 404 (repo_not_found).
• Without ?repo=: auth-only. The global anchor listing stays as auth-only — the wider visibility filtering is the #136 stale-index class (see below).
• Clamped the limit to [0, 200] before the SQL query so ?limit=-1 (or any negative) doesn't reach Postgres as LIMIT -1.
• Round 2: built the SQL filter from the canonical stored slug ({normalize_owner_key(record.owner_did)}/{record.name}) instead of the user-supplied ?repo= string. Authz accepts the full did:key:…/name form, but a literal WHERE repo = $1 against the full DID would match zero rows and return a false empty page. The fix keys the SQL on the RepoRecord the authz helper resolved, which is the only place the slug the writer actually stores is in scope.

Out of scope (recorded, not addressed here)

The global pin listing and the unscoped anchor listing are still permissionless-identity enumerable: any self-minted did:key can read them. Authentication is not authorization on these. Closing that is the #136 class (the write-time visibility-gate learning doc), deliberately not bundled with this auth slice so the review can stay focused on the wiring change.

Testing

• New build_router regressions cover the full production contract: real RFC-9421 signatures through optional_signature, real authorize_repo_read, real SQL.
• Covers unsigned 401, valid signature 200, malformed signature 401, scoped unsigned 401, scoped owner 200, scoped non-reader 404 with no anchor metadata in the body, scoped missing-repo 404 with the standard repo_not_found shape, and ?limit=-1 clamping to an empty 200.
• Round 2 added signed_scoped_anchors_full_did_form_matches_stored_slug_through_build_router, which seeds a SECOND repo's anchor under a different owner and asserts that anchor is absent from the authorized response — proving the filter excludes cross-repo rows, not just that the authorized row is present. The fixture also uses db::normalize_owner_key to derive the seed slug, matching the writer so the test reader and writer can't drift on a future change.

[github-actions]

Thanks for the contribution. A couple of things will help us review this faster:

• This changes Rust source but no tests changed. Tests are required for fixes and strongly encouraged for features.

See CONTRIBUTING.md. Update the PR and these notes will clear automatically.

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Adds optional-signature middleware for /api/v1/ipfs/pins and /api/v1/arweave/anchors, then filters list_pins and list_anchors by caller authentication and repo visibility. Adds integration tests for anonymous, authenticated, and repo-scoped access.

Changes

Auth Gating for Metadata Index Endpoints

Layer / File(s)	Summary
Router middleware wiring crates/gitlawb-node/src/server.rs	/api/v1/ipfs/pins is defined inside the IPFS router so it inherits auth::optional_signature; auth::optional_signature is added to the Arweave anchors route group.
Handler auth enforcement crates/gitlawb-node/src/api/arweave.rs, crates/gitlawb-node/src/api/ipfs.rs	list_pins now requires a caller context and filters pinned objects through repo visibility, quarantined-repo checks, and withheld-blob handling. list_anchors now accepts optional auth, validates owner/name, rejects anonymous global listings, applies repo read authorization, and filters global results by per-anchor access.
Integration tests for pins and anchors crates/gitlawb-node/src/test_support.rs	Adds SQLx tests for anonymous and authenticated pins access, plus anonymous, authenticated, owner-allowed, and non-reader-denied anchors access with and without ?repo=.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related issues

• gl/MCP read commands send unsigned requests to visibility-gated endpoints, breaking them for private-repo owners #115: Matches the same auth/visibility gating objective for read endpoints that were previously exposed without caller checks.
• list_pins / list_anchors serve stale metadata for repos made private after push (no index reconciliation on visibility downgrade) #136: Targets the same read paths and current-visibility filtering behavior for list_pins and list_anchors.

Possibly related PRs

• Gitlawb/node#25: Introduces the repo visibility machinery that list_anchors and list_pins now consume.
• Gitlawb/node#52: Establishes the authorize_repo_read read-gating pattern used by list_anchors.
• Gitlawb/node#90: Adds the object-enumeration path that list_pins now uses when building allowed hashes.

Suggested labels

kind:security, subsystem:visibility, subsystem:api, sev:medium

Suggested reviewers

• kevincodex1
• jatmn

Poem

🐇 I hop past pins with careful feet,
And anchors only show what's meet.
Signed little hops, both safe and neat,
Keep secret burrows off the street.
A carrot cheer for guarded streams! 🥕

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The code adds auth and visibility enforcement for both /ipfs/pins and /arweave/anchors as requested in #121.
Out of Scope Changes check	✅ Passed	The changes stay focused on auth/visibility hardening for the two listed metadata endpoints and the related tests.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Title check	✅ Passed	The title clearly and concisely describes the main change: authentication gating for both metadata endpoints.
Description check	✅ Passed	The description is detailed and on-topic. It explains the motivation, endpoint changes, out-of-scope work, issue reference, and comprehensive testing. It does not use every template heading or checkli…

Full details: Description check

Explanation

The description is detailed and on-topic. It explains the motivation, endpoint changes, out-of-scope work, issue reference, and comprehensive testing. It does not use every template heading or checklist item, but the required information is mostly present.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

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 lift

Avoid 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

📥 Commits

Reviewing files that changed from the base of the PR and between 8d96cab and b558bec.

📒 Files selected for processing (3)

• crates/gitlawb-node/src/api/arweave.rs
• crates/gitlawb-node/src/api/ipfs.rs
• crates/gitlawb-node/src/server.rs

[coderabbitai]

🧹 Nitpick comments (1)

crates/gitlawb-node/src/test_support.rs (1)

1968-2017: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add 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 the repo predicate 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

📥 Commits

Reviewing files that changed from the base of the PR and between b558bec and 9b71e6a.

📒 Files selected for processing (1)

• crates/gitlawb-node/src/test_support.rs

[beardthelion]

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/anchors and /ipfs/pins listings 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_signature verifies a self-made signature, register is 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 (no DELETE of either table exists, set_visibility touches neither, and both list queries are unfiltered SELECTs), the global lists can serve a now-private repo's slug, owner DID, branch names, commit SHAs, and object CIDs. Content stays gated by GET /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 via authorize_repo_read -> get_repo, which matches the owner by LIKE (full did:key: or bare short form). The result query then filters exact WHERE repo = $1 on the raw ?repo= string, but anchor rows are written short-form ({owner_short}/{name}, repos.rs:1142). So ?repo=did:key:zX/name authorizes (200) yet returns an empty list; only ?repo=zX/name returns the owner's anchors. It fails safe (returns fewer results, not more), so it is a correctness/usability bug, not a leak. The new anchors_repo_* tests pass only because seed_anchor seeds the full-DID form, which production never writes. Normalize the slug from the authorized RepoRecord before querying, and seed the production short form in tests.

• [P3] Run cargo fmt — test_support.rs:1903 fails rustfmt --check
crates/gitlawb-node/src/test_support.rs:1903
  A long assert_eq! line needs wrapping; the format gate fails. cargo fmt fixes 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.

[coderabbitai]

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

📥 Commits

Reviewing files that changed from the base of the PR and between 9b71e6a and 0fed173.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (3)

• crates/gitlawb-node/src/api/arweave.rs
• crates/gitlawb-node/src/api/ipfs.rs
• crates/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]

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 when caller is None. As a result, anonymous GET /api/v1/arweave/anchors and GET /api/v1/ipfs/pins return 200 instead of the 401 behavior this PR claims and tests for. I verified this with the PR's own focused tests: cargo test -p gitlawb-node pins_list -- --nocapture fails with pins_list_denies_anonymous returning 200 instead of 401, and cargo test -p gitlawb-node anchors_ -- --nocapture fails with anchors_global_denies_anonymous returning 200 instead of 401. Please complete the current CodeRabbit requests by returning Unauthorized before 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_authenticated inserts a bare pinned_cids row but no readable repo/object containing that SHA, while list_pins now only returns pins whose SHA is found by scanning readable repo object databases, so the count is 0 instead of 1. The anchor tests seed some/repo or the full-DID slug (did:key:.../repo), but production anchor writes use the short owner slug from repos.rs, and list_anchors now normalizes authorized ?repo= lookups to that short form, so the owner/global success tests also return count 0. 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_read hides quarantined repos before it applies visibility rules, but the new list_pins implementation bypasses that helper and iterates list_all_repos() with a direct visibility_check. A quarantined public mirror can therefore still contribute object SHAs to allowed_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 reuse authorize_repo_read semantics here or add the same is_repo_quarantined exclusion before scanning repo objects.

[beardthelion]

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
  The is_repo_quarantined skip is correct, but no pins_ 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_pins builds its allowed set with withheld_blob_oids, a separate construction from get_by_cid's allowed_blob_set_for_caller, and no pins_ 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_authenticated only proves a public anchor is visible; the per-row authorize_repo_read filter has no test that an authenticated non-reader is denied another repo's private anchor. Add that exclusion assertion, mirroring anchors_repo_denies_non_reader for the ?repo= path.

[coderabbitai]

🧹 Nitpick comments (3)

crates/gitlawb-node/src/test_support.rs (3)

2079-2240: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicated 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, and anchors_repo_denies_non_reader all repeat the identical owner_short/short_slug derivation 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 win

Missing 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 win

Consider extracting shared setup for the three pins tests.

pins_list_allows_authenticated, pins_list_excludes_quarantined_repos, and pins_list_withholds_path_scoped_blobs each repeat the same Keypair/fs_slug/short/seed_cid_repos boilerplate. 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

📥 Commits

Reviewing files that changed from the base of the PR and between beafa8a and 81a5772.

📒 Files selected for processing (1)

• crates/gitlawb-node/src/test_support.rs

[beardthelion]

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.

[Gravirei]

@kevincodex1 LGTM

[jatmn]

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 applies optional_signature, but they do not exercise server::build_router, which is the production wiring this PR had to fix. That matters here because /api/v1/ipfs/pins was previously outside the optional-signature layer; if server.rs regressed or the route were left in the old .merge(...).layer(...) shape, the signed pins_list_allows_authenticated test would still pass while the real route would never attach AuthenticatedDid and would return 401 for every signed pins request. Please add at least one integration test through build_router for 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=-1 still passes -1 into list_arweave_anchors, where it is bound directly into PostgreSQL LIMIT. 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 with q.limit.clamp(0, 200), before calling state.db.list_arweave_anchors.

[beardthelion]

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 newest limit rows and only then drops the ones the caller cannot read, so when the most recent anchors belong to unreadable repos the caller gets count=0 even 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 take limit, the way the sibling list_pins path 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 to min(200) reintroduces a 500 on ?limit=-1 (Postgres rejects LIMIT -1) with nothing to catch it. One test asserting ?limit=-1 returns a bounded 200 pins it.

Core gating is otherwise sound: anonymous and non-reader paths are correctly denied on both endpoints.

[jatmn]

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 asks list_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 calls authorize_repo_read(short, name, ...); get_repo can match either the bare public mirror row or the private canonical did:key: row because it has no canonical preference. The pins path has the same problem more directly: it scans every row from list_all_repos(), including public mirror rows, and adds their object SHAs to allowed_sha256s without 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 current q.limit.clamp(0, 200) fix addresses jatmn's and CodeRabbit's negative-limit concern, but it is still untested. A future refactor back to min(200) would send LIMIT -1 into Postgres and turn a malformed request into a 500 again. Please add a small /api/v1/arweave/anchors?limit=-1 test that asserts the endpoint returns a bounded successful response.

[jatmn]

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 through authorize_repo_read, which calls get_repo and can match either the bare public mirror row or the private canonical did: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 calls list_arweave_anchors(None, i64::MAX) and only applies the requested limit after 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 unbounded ORDER BY anchored_at DESC LIMIT 9223372036854775807 plus 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 with WHERE repo = ANY(...) ORDER BY anchored_at DESC LIMIT $limit or by using a bounded/cursor batch loop.

[jatmn]

I found issues that need to be addressed before this is ready.

Findings

• [P1] Preserve the actual scan position when a pin page is truncated
  crates/gitlawb-node/src/api/ipfs.rs:1512-1566
  The listing maintains two distinct positions: the public two-field cursor identifies the last pin returned to the caller, while the internal three-field cursor identifies the last association processed by the visibility/probe scan. That distinction is lost when a page returns at least one visible pin and then hits a walk, probe, or batch wall. The response takes the response-cursor branch and emits only next_cursor for the last visible pin, even though db_cursor has already advanced into the scan.

  On the next request, the database keyset query resumes immediately after that visible pin. It therefore re-enters the same hidden or deferred associations, hits the same wall, and produces the same next_cursor. A client cannot make progress to pins after that tail; the gl callers will keep following the returned cursor because it is presented as ordinary pagination. This is especially likely for a readable repo with many path-scoped or temporarily unclassifiable entries between visible pins.

  Please keep the public cursor and internal scan cursor separate at the response boundary. A page cut short before completing its scan needs an opaque continuation that restores the full three-field scan position, even if it also emitted visible entries. The continuation must not disclose the skipped repo association, and normal non-truncated pages should retain the existing simple public keyset cursor. Add a regression that places a visible pin before a scan-wall tail and another visible pin after it, then proves a client can retrieve the later pin across resumed requests.

• [P2] Do not acknowledge an already-pinned object before its repository association is durable
  crates/gitlawb-node/src/ipfs_pin.rs:283-293
  crates/gitlawb-node/src/pinata.rs:154-161
  This PR makes the pin-to-repository association the authority used by the scoped pin listing. The new-object writers correctly persist the pin row and association together in a transaction. The already-pinned fast paths do not provide equivalent reliability: after is_pinned or has_pinata_cid succeeds, an update_pinned_cid_repo failure is only logged and the loop continues as though the current repo had been recorded.

  A transient database failure in that exact window leaves the object physically pinned but without the association required for readers authorized on this repo to discover it. No durable retry, failed-work record, or reconciliation revisits the missing edge; recovery depends on an unrelated later push presenting the same SHA again. The failure is particularly problematic for deduplicated objects shared across repositories, because the fast path is the only opportunity for the second repository to establish its association.

  Please make association durability part of completing the already-pinned path. The implementation can propagate the error to work that is retried, enqueue durable association repair, or use another bounded retry/reconciliation mechanism; the important outcome is that a failed write is not silently reported as successfully handled. Preserve the existing deduplication behavior and avoid re-uploading an object solely to repair its association. Add failure-injection coverage proving that an association-update failure is retried or otherwise remains repairable, and that a successful retry makes the object appear in the scoped listing.

Overall guidance

This PR has accumulated a large amount of security-sensitive behavior beyond the original authentication change: visibility filtering, normalized ownership, migrations, shared-object persistence, bounded scans, encrypted continuation tokens, concurrent rate limiting, and CLI pagination/recovery. The remaining defects come from hand-offs between those layers rather than from the basic auth checks: one layer keeps state another layer cannot resume, and one writer reports success before the new reader-visible persistence contract is complete.

To avoid another review-fix cycle, please treat the affected flows as end-to-end contracts rather than isolated fixes. For each changed listing path, trace authentication and authorization through database selection, visibility classification, pagination state, client retry/resume, and final user-visible completion. For each new persistence relationship, trace both new-record and already-existing-record paths through failure, retry, restart, and the corresponding reader. Prefer a small set of scenario-driven tests that prove those full flows: visible-before-hidden-tail-visible pagination; temporary walk/probe failure followed by continuation; shared SHA first seen in one repo and then another; and association-write failure followed by repair and scoped discovery.

Please keep the patch focused on restoring these contracts. The current normal-page cursor format, pin deduplication, visibility non-disclosure guarantees, and the bounded-work limits are all useful constraints to preserve while addressing the root causes.

[beardthelion]

Re-reviewed ab36dad7 by execution on the current head. The #121 auth gate still holds: gutting the anonymous 401 in list_pins or list_anchors turns the handler and router tests red, CI is 12/12, and the doc/migration items from my prior round are addressed in af2966b4. Concurring with @jatmn's two findings on this head.

Findings

• [P1] Emit an opaque scan continuation when a truncated page also returned visible pins
  crates/gitlawb-node/src/api/ipfs.rs:1556
  At the response boundary, page_truncated with a populated response_cursor always emits next_cursor from the last visible pin and never the three-field db_cursor scan position. When the walk/probe wall hits after at least one visible pin but before the page fills, the next request resumes the public keyset after that visible row and re-enters the same hidden or deferred tail, so a later visible pin past the wall is unreachable. test_max_walks_plaintext_not_in_response_cursor covers visible-then-all-hidden, not visible-hidden-visible. Keep the public keyset cursor for filled pages; for truncated pages with visible rows, also return an opaque continuation that restores the internal scan position without disclosing skipped associations.

• [P2] Make already-pinned association writes durable before reporting success
  crates/gitlawb-node/src/ipfs_pin.rs:289
crates/gitlawb-node/src/pinata.rs:157
  The scoped listing reads associations from pinned_cid_repos, but the already-pinned fast paths only tracing::warn and continue when update_pinned_cid_repo fails after is_pinned / has_pinata_cid succeeds. A transient failure leaves the object pinned without the repo edge the listing needs, with no repair path except a later push of the same SHA. Propagate, retry, or enqueue durable repair instead of treating the iteration as complete.

Not an ask, recorded only: batch_object_types still spawns git cat-file --batch-check without process_group(0) teardown (ipfs.rs:813), unlike the smart-HTTP git children.

[beardthelion]

Re-reviewed on d9c2f80d. The prior P1 is closed: truncated pages that also return visible pins now emit truncated_cursor from the internal scan position, and test_ipfs_cursor_guard passes the visible / hidden-window / visible case. I gutted the anonymous 401 in list_pins and anonymous_pins_is_401_before_any_db_work went red; unsigned_get_pins_and_anchors_is_401_through_build_router is green. All 12 api::ipfs::tests and 8 api::arweave::tests pass; CI is 12/12 on this head. The five open Copilot inline threads are stale on this head (normalize_owner_key slug building, limit == 0 short-circuit, no normalize_owner_did duplicate).

Findings

• [P2] Parse the anchor repo slug before listing admission
  crates/gitlawb-node/src/api/arweave.rs:85
list_anchors calls check_listing_admission before it validates ?repo= with split_once('/'). A self-registered DID can send signed GET /api/v1/arweave/anchors?repo=not-a-slug until the shared global listing bucket is exhausted; each request gets 400 only after spending per-DID and global quota, with no listing work done. Move the cheap slug parse (and the zero-limit short-circuit you already have) ahead of admission, mirror the ordering in the pins handler if any cheap reject can run first, and add a regression that sets the global limiter to 1, burns it with one malformed signed ?repo=, then asserts a valid signed request is still admitted.

Not an ask, recorded only: global listings still load the full deduped catalog per request (rate-limited, not page-proportional). Mirror-only gossip rows without a local canonical twin remain the #124 residual documented in ipfs.rs.

[jatmn]

I found issues that need to be addressed before this is ready.

Overall guidance

This PR has accumulated a large amount of security, pagination, persistence, rate-limit, process-control, and client-recovery work while it has been repeatedly rebased and amended. The remaining issues share one pattern: a local fix often establishes the happy-path invariant, but adjacent boundary transitions are not kept under the same contract. Examples in this revision are converting a DB error after earlier reads, charging quota before rejecting syntactically invalid input, and starting a new fixed timeout after a cumulative deadline has already been established.

Before another incremental patch, please make one deliberate pass over each listing as an end-to-end state machine rather than addressing individual comments in isolation:

• Define the request order explicitly: authenticate → parse and validate → authorize → reserve rate-limit capacity → enumerate/query → perform bounded visibility work → serialize cursor/response. Cheap malformed input must fail before consuming shared capacity; every failure after admission must have a deliberate retry/error shape.
• Treat the listing deadline as one request-wide budget. Every nested wait, subprocess, repo acquisition, semaphore-held operation, database page, and client retry must consume the remaining budget rather than start its own independent timeout.
• Preserve error types across layer boundaries. Do not wrap database or cancellation errors in a generic internal error unless the endpoint intentionally changes their public retry semantics; test failures at the specific later operation, not only at handler entry.
• For every new persisted or opaque cursor state, trace create → serialize → resume on a fresh process/node → expiry/tamper/error → client fallback. For pin associations, trace both the initial pin and already-pinned fast paths through persistence, retry, and scoped listing.
• Add small, mutation-resistant tests at each boundary: malformed input must not consume a global slot; a late DB outage must remain retryable; near-deadline work must not exceed the request budget; and client resume must surface incomplete data rather than silently claim completion.

Keeping the next update narrowly focused on restoring these shared contracts, with regressions that fail when their ordering or error propagation is removed, should prevent another round of piecemeal review churn.

Findings

• [P2] Preserve database-outage classification for the paged pin query
  crates/gitlawb-node/src/api/ipfs.rs:1096
  The new listing loop calls list_pinned_cids_for_repos, whose database-layer errors are returned as anyhow::Error, but then explicitly wraps every such error in AppError::Internal. That bypasses the existing From<anyhow::Error> for AppError conversion, which downcasts sqlx::Error and maps PoolClosed, PoolTimedOut, I/O, and TLS failures to the retryable 503 db_unavailable response. Consequently, if the initial catalog/rule reads succeed but Postgres becomes unavailable while fetching a later page, /api/v1/ipfs/pins returns 500 internal_error rather than the retryable outage response. The root cause is converting a typed database failure to the generic internal variant at this new call site. Keep the established database error conversion for this query (for example, propagate it with ?), and add a regression that fails the page-query edge after the earlier catalog reads have completed.

• [P2] Validate the anchor repo selector before reserving listing capacity
  crates/gitlawb-node/src/api/arweave.rs:85
list_anchors calls check_listing_admission before it parses ?repo= at line 93. A signed request such as ?repo=not-a-slug therefore commits a per-DID and global slot, then returns the cheap 400 invalid repo slug without touching any listing data. Because HTTP-signature identities are permissionless, an attacker can rotate DIDs to bypass the per-DID bucket and exhaust the global Arweave listing budget, making well-formed requests receive 429 for the rest of the window. The root cause is treating syntactically invalid input as an admitted expensive listing. Parse and reject malformed repo selectors before admission; retain the existing authentication, authorization, and quota behavior once input is valid. Add a test with a one-slot global limiter showing that a malformed signed request does not prevent a subsequent valid listing.

• [P2] Make all pin-listing waits honor the cumulative deadline
  crates/gitlawb-node/src/api/ipfs.rs:1194
  The handler advertises a 120-second cumulative listing deadline and checks it before beginning a visibility walk. However, after that check, repo-store acquisition receives a fresh 30-second timeout, and the structural-object probe receives another fresh 30-second timeout at line 1396. If the request arrives at either operation with only milliseconds remaining, it can still retain a scarce walk-semaphore permit for up to 30 more seconds. This defeats the intended bound on request work and can keep all visibility-walk slots occupied after their listings should have been truncated. The root cause is using fixed operation timeouts instead of the remaining duration of the already-established deadline. Clamp acquisition and probe timeouts to listing_deadline.saturating_duration_since(now) and defer/truncate immediately when it is zero; add a near-deadline regression covering both operations.

[jatmn]

please rebase on main

[beardthelion]

Re-reviewed on 12f34d4a. I ran the handler test suites (api::ipfs::tests, api::arweave::tests: 21 passed), the production router wiring test (unsigned_get_pins_and_anchors_is_401_through_build_router), and inv22_gates (7 passed). Auth gates, live visibility filtering, walk/probe fail-closed behavior, and gl client signing all hold on this head. jatmn's July walk-budget and limit=0 items are closed here.

Second-model refute on this head found two items that survive verification and one design tradeoff worth recording.

Findings

• [P2] Skip mirror-only repo rows before building readable pairs for global metadata listings
  crates/gitlawb-node/src/api/arweave.rs:39
list_all_repos_deduped can surface a mirror-only survivor (id contains /, is_public=true, no visibility rules). readable_repo_pairs and list_pins then call listable_at_root on that row, so any authenticated DID can see pins/anchors stored under the mirror slug. The same hollow-input class blocked #244's egress sweep and is documented in the mirror-row-born-public learning. Skip slash-id mirror rows unless a canonical twin exists locally, or resolve to canonical and fail closed when none exists. I did not execute a full mirror-only repro on this head; the mechanism matches the stored vacuity probe.

• [P2] Pad opaque truncated cursors to fixed width before AEAD encryption
  crates/gitlawb-node/src/api/ipfs.rs:744
create_opaque_cursor encrypts expiry || cursor_string with length tied to the hidden row's repo slug. A stranger denied the pins can still learn slug length from truncated_cursor token length on a truncated page. Pad plaintext to a fixed width (or use a constant-size encoding) before encrypting.

Not an ask, recorded only: global listings still load the full deduped repo catalog and all visibility rules before applying the SQL limit. Listing admission and per-request deadlines bound abuse partially; a signed DID can still drive O(all repos) work per limit=1 call. Same amplification class flagged earlier; rate limit does not cap work per request.

One process note, not a finding: rebase onto current main before merge (mergeable_state: dirty).

[beardthelion]

The anonymous path is wired correctly in the handlers: I ran unsigned_get_pins_and_anchors_is_401_through_build_router on 063a5e68 and got GREEN (401 for both routes through build_router). CI is 12/12 on this head. I also read write-time-visibility-gate-leaves-derived-index-stale.md, identity-bound-is-not-authorized.md, and enumerate-all-readers-of-a-data-class-when-gating.md before diagnosing.

This is a narrow auth-only slice. It closes the unauthenticated enumeration hole but not the permissionless-identity enumeration hole #121 describes, and production routing currently blocks even legitimate signed callers.

Findings

• [P1] Wire optional_signature on the pin and anchor routes before the handlers run
  crates/gitlawb-node/src/server.rs:231
list_pins and list_anchors reject when Option<Extension<AuthenticatedDid>> is absent, but /api/v1/ipfs/pins is merged outside the optional_signature layer on ipfs_routes, and arweave_routes has no signature middleware at all. optional_signature is what verifies RFC 9421 headers and injects AuthenticatedDid. Without it, real signed GETs (including gl get_signed("/api/v1/ipfs/pins") on main) never populate the extension, so every production signed request gets the same 401 as anonymous. The new regression test only covers unsigned traffic; closed-pool tests inject .extension(AuthenticatedDid(...)) directly and do not exercise middleware.

• [P2] Filter pin and anchor listings on current read visibility for #121
  crates/gitlawb-node/src/api/ipfs.rs:2147
  After the middleware fix, any self-registered did:key can still read the full node-wide indexes. list_pins calls unfiltered list_pinned_cids(); global list_anchors calls list_arweave_anchors(None, limit) with no authorize_repo_read or visibility predicate. Identities here are permissionless, so authentication is not authorization. Issue #121 and the write-time visibility learning both require read-time filtering (or operator-only scope): a repo pushed while public and later made private still leaks slug, owner DID, ref names, and SHAs to any stranger with a keypair.

One process note, not a finding: rebasing will likely conflict with #224 and #285 on server.rs, ipfs.rs, and arweave.rs. Resolve routing conflicts against whichever PR lands first, then re-apply the auth and visibility layers here.

[jatmn]

I found issues that need to be addressed before this is ready.

Findings

• [P1] Wire the new authenticated handlers to the signature middleware
  crates/gitlawb-node/src/server.rs:226
list_pins and list_anchors now return 401 unless their request contains AuthenticatedDid, but that value is only inserted by auth::require_signature through auth::optional_signature. Neither production endpoint currently passes through that producer: the IPFS layer is applied while the router contains only /ipfs/{cid}, then /api/v1/ipfs/pins is added with a later .merge(...); /api/v1/arweave/anchors is registered in an unlayered router. Consequently, a syntactically valid RFC 9421-signed request reaches both handlers without the extension and is rejected just like an unsigned request. This breaks the updated gl ipfs list and gl node status pin reads, which now correctly use NodeClient::get_signed.

  Address the root cause by making both exact metadata routes run through the existing optional_signature middleware before handler extraction—not by weakening or manually injecting the handler’s identity check. Keep the pin content route’s existing rate-limit scope intact, preserve the unsigned 401 behavior and malformed-signature rejection, and add end-to-end build_router coverage for a valid signed request to each route. The current router regression only proves the unsigned denial, while the closed-pool tests inject AuthenticatedDid directly and therefore bypass the broken routing edge.

Guidance for completing this security change

This review has accumulated history because the change spans one authorization contract across multiple layers, rather than because the current patch has many independent defects. The remaining blocker is one root-cause mismatch: the handler now consumes an authenticated request extension, but the production route has not been connected to the middleware that creates it. Resolving that producer-to-consumer connection should close the current finding; the earlier, distinct historical leads were rechecked and are not included here as additional findings.

For this kind of endpoint hardening, please validate the whole request path as one contract before considering the work complete:

1. Route composition: identify the exact router that owns each endpoint, then verify layer placement and merge ordering. In Axum, a layer applied before a later .merge(...) does not automatically prove coverage of the subsequently merged route.
2. Identity provenance: trace the handler extractor back to its sole producer. Here AuthenticatedDid must originate from RFC 9421 verification via optional_signature; direct test insertion is useful for handler-only error tests but cannot validate production wiring.
3. Both authorization outcomes: test anonymous requests through build_router for the expected 401, then test valid signed requests through that same production router for the intended success path. Also retain a malformed-signature case where feasible, so adding the optional layer cannot silently treat invalid credentials as anonymous or authenticated.
4. Consumers and release compatibility: search every caller before changing an endpoint from anonymous to authenticated. The CLI pins callers were correctly updated to sign, but their end-to-end behavior only works once the server route consumes that signature. For denial paths, preserve the repository rule that clients surface denial rather than render it as an empty successful result.
5. Keep concerns scoped: applying optional_signature to these two index routes should not alter the existing /ipfs/{cid} content route’s rate-limit boundaries, response formats, database-error mapping, or broader visibility policy. Those are separate contracts and should remain stable unless deliberately changed and tested.

A focused fix that wires these two routes, plus production-router signed and unsigned regression coverage, is preferable to further handler or CLI rewrites.

[beardthelion]

I re-read the diff on 063a5e68, traced build_router wiring against optional_signature and AuthenticatedDid, checked prior art (write-time-visibility-gate-leaves-derived-index-stale.md, identity-bound-is-not-authorized.md, bare-router-probe-answers-the-mechanism-not-the-caller.md, enumerate-all-readers-of-a-data-class-when-gating.md), and compared the new test to list_webhooks_accepts_a_real_gl_signature_e2e. CI is green on this head (https://github.com/Gitlawb/node/actions/runs/32957947046). The anonymous rejection path is real, but production signed callers still cannot reach these handlers.

Findings

• [P1] Wire optional_signature on the pin and anchor list routes before handler extraction
  crates/gitlawb-node/src/server.rs:231
list_pins and list_anchors return Unauthorized when Option<Extension<AuthenticatedDid>> is absent, but only optional_signature verifies RFC 9421 headers and injects that extension. On ipfs_routes, the layer sits on /ipfs/{cid} and /api/v1/ipfs/pins is merged afterward, so it never runs for pins. arweave_routes has no auth layer at all. Repo-scoped reads like list_webhooks carry .layer(middleware::from_fn(auth::optional_signature)) on the whole group (server.rs:431); these two routes do not. gl already calls get_signed("/api/v1/ipfs/pins"), so valid signed GETs hit the same Unauthorized response as anonymous traffic until the middleware is wired.

• [P2] Add signed GET regression coverage through build_router for both routes
  crates/gitlawb-node/src/server.rs:635
  The new unsigned router regression in server.rs only proves headerless denial. It stays green when middleware is missing because signed requests also arrive without the extension. Mirror list_webhooks_accepts_a_real_gl_signature_e2e: sign_request over GET, send through build_router, assert success on pins and anchors. Keep the unsigned denial test. Closed-pool tests that inject .extension(AuthenticatedDid(...)) are fine for the SQL path but do not validate production wiring.

One process note, not a finding: rebasing will likely conflict with #224 and #285 on server.rs, ipfs.rs, and arweave.rs. Resolve routing against whichever lands first, then re-apply the auth layers here.

Not an ask, recorded only: this slice closes anonymous enumeration only. After middleware is fixed, any permissionless did:key can still read the full node-wide indexes because list_pinned_cids() and list_arweave_anchors() stay unfiltered. That is the #136 stale-index class documented in the write-time visibility learning; it is out of scope for this auth-only commit but should not be read as closing the full #121 confidentiality story.

[beardthelion]

Re-reviewed 063a5e68. The anonymous rejection path is real: headerless GETs through build_router get 401, and CI is green. The blocker is production wiring: handlers require AuthenticatedDid, but the routes that serve them never run optional_signature, so valid signed GETs from gl hit the same 401.

Findings

• [P1] Wire optional_signature on the pin and anchor list routes before handler extraction
  crates/gitlawb-node/src/server.rs:231
list_pins and list_anchors return Unauthorized when Option<Extension<AuthenticatedDid>> is absent, but only optional_signature verifies RFC 9421 headers and injects that extension (auth/mod.rs:251). On ipfs_routes, the layer sits on /ipfs/{cid} and /api/v1/ipfs/pins is merged afterward, so pins never see it. arweave_routes has no auth layer at all. gl already calls get_signed("/api/v1/ipfs/pins") (gl/ipfs_cmd.rs:102, gl/node.rs:244), so legitimate signed reads fail until middleware is wired. Mirror read_routes / list_webhooks, which carry .layer(middleware::from_fn(auth::optional_signature)) on the whole group.

• [P2] Add signed GET regression coverage through build_router for both routes
  crates/gitlawb-node/src/server.rs:635
unsigned_get_pins_and_anchors_is_401_through_build_router only proves headerless denial. That stays green when middleware is missing because signed requests also arrive without the extension. Mirror list_webhooks_accepts_a_real_gl_signature_e2e: sign_request over GET, send through build_router, assert success on pins and anchors. Keep the unsigned denial test. Closed-pool tests that inject .extension(AuthenticatedDid(...)) are fine for the SQL path but do not validate production wiring.

One process note, not a finding: rebasing will likely conflict with #224 and #285 on server.rs, ipfs.rs, and arweave.rs. Resolve routing against whichever lands first, then re-apply the auth layers here.

Not an ask, recorded only: this slice closes anonymous enumeration only. After middleware is fixed, any permissionless did:key can still read the full node-wide indexes because list_pinned_cids() and list_arweave_anchors() stay unfiltered. That is the #136 stale-index class in the write-time visibility learning; out of scope for this auth-only commit but not the full #121 confidentiality story.

[jatmn]

I found an issue that needs to be addressed before this is ready.

Findings

• [P1] Apply the promised repository-read gate before the scoped anchor query
  crates/gitlawb-node/src/api/arweave.rs:24
  The new AuthenticatedDid check establishes only that the request has a valid signature. It does not establish that the signer may read q.repo. Once any caller signs with a self-created DID, this handler passes the supplied repository string directly to list_arweave_anchors; that query filters by repo text only and returns anchor records without an authorization decision. A signed non-reader can therefore obtain private repository anchor and ref metadata, including ref names, old and new commit IDs, CIDs, and transaction metadata. An unreadable scoped repository also cannot receive the normal indistinguishable 404 response required for read denials.

  The root cause is treating authentication as authorization on a repository-derived read surface. Issue #121 and this PR stated a specific contract: GET /api/v1/arweave/anchors?repo=<owner>/<repo> must bind the authenticated DID to existing repository-read policy before querying or returning anchors. Please parse and resolve the scoped repository through the same canonical repository and read-authorization path used by other repo-scoped reads, return the standard 404 when the caller is anonymous or lacks access, and only then query anchors using the resolved authorized identity. Do not use the user-supplied repo string alone as the access decision.

Guidance for completing this security fix

This PR has accumulated review rounds because the endpoint is a metadata egress path with two requirements that are easy to conflate:

1. Authenticate the request: reject an unsigned or malformed request. The new middleware and AuthenticatedDid extraction address this part.
2. Authorize the requested resource: for every scoped read, decide whether that DID may read that repository before looking up or emitting its metadata. This remains missing on the ?repo= anchor path.
3. Preserve non-disclosure on denial: a caller without access must receive the same 404 shape as a missing repository; avoid revealing whether a private repository or anchor history exists.

To avoid another incremental review cycle, please validate the complete route contract through build_router, rather than only testing the handler with a pre-inserted identity extension. Seed an anchor for a private repository and cover at least an unsigned scoped request, a signed owner or authorized-reader request, and a signed non-reader request. The non-reader case should assert 404 and no returned anchor metadata; it should fail if the authorization call is removed or moved after the database query. Also keep a distinct missing-repository assertion so the denial shape stays intentionally indistinguishable. These tests should exercise the production signature middleware and the production repository authorization helper together.

The global authenticated listing and the pin endpoint have deliberately not been expanded into additional requirements here: the accepted issue allows their narrower anonymous-denial scope. Keep this change focused on completing the explicit scoped-anchor authorization and denial contract.

[beardthelion]

Re-reviewed 752e117f. Production wiring is resolved on this head: /api/v1/ipfs/pins is merged before optional_signature on ipfs_routes (server.rs:230-234), anchors carry the same layer (server.rs:243-245), and I ran the three build_router regressions locally (unsigned 401, valid signature 200, malformed signature 401). I concur with jatmn on the remaining blocker. For this slice, auth-only on the global pin listing and unscoped anchor listing is the accepted scope.

Findings

• [P1] Gate scoped anchor reads on repository visibility before querying
  crates/gitlawb-node/src/api/arweave.rs:24
  The AuthenticatedDid check only proves a valid signature. When ?repo=<owner>/<name> is present, the handler passes that string straight into list_arweave_anchors with no authorize_repo_read decision. Any self-minted did:key can therefore read private-repo anchor metadata (ref names, old/new SHAs, CIDs, tx ids), and a non-reader cannot get the standard indistinguishable 404. Parse and resolve the repo through the same canonical read-authorization path as other repo-scoped reads, return 404 when the caller lacks access, then query anchors from the authorized identity. Add build_router coverage: private-repo anchor seeded, unsigned scoped GET → 401, signed owner → 200, signed non-reader → 404 with no anchor rows in the body.

• [P2] Clamp the anchors limit before the SQL query
  crates/gitlawb-node/src/api/arweave.rs:34
q.limit.min(200) passes -1 through (min(-1, 200) == -1), and PostgreSQL rejects LIMIT -1 as a db error (500). Use q.limit.clamp(0, 200) and add a ?limit=-1 regression test.

One process note, not a finding: the PR description still claims visibility filtering and authorize_repo_read on scoped anchors; align the body with what ships once the scoped gate lands. Expect rebase conflicts with #224 and #285 on server.rs / ipfs.rs.

Not an ask, recorded only: after the scoped gate, any signed stranger can still read the full global pin index and unscoped anchor listing. That is the #136 stale-index class documented in our write-time visibility learning; jatmn and I are treating it as out of scope for this auth slice, not closed for #121 overall.

[jatmn]

I found an issue that needs to be addressed before this is ready.

Findings

• [P2] Preserve the CID resolver's rate-limit boundary when authenticating pins
  crates/gitlawb-node/src/server.rs:232
  The route reorder solves the authentication producer/consumer bug by merging /api/v1/ipfs/pins before optional_signature, but it also places pins under every other layer on that router: rate_limit_by_ip and the IpRateLimiter extension. Those layers are not generic API protection. GITLAWB_IPFS_RATE_LIMIT is documented and configured as the once-per-request flood brake for GET /ipfs/{cid}, because that content resolver can perform bounded but expensive repository scans and history walks; list_pins performs only list_pinned_cids().

  This changes two previously independent availability contracts. rate_limit_by_ip runs before optional_signature, so even unsigned /api/v1/ipfs/pins requests that ultimately receive 401 consume the source IP's resolver tokens. Signed pin polling consumes them too. Once the default 600/hour bucket is exhausted, both subsequent pin listings and unrelated legitimate /ipfs/{cid} reads from the same client or shared NAT return 429 until the window resets. CID traffic can likewise exhaust the bucket and make the newly authenticated pins endpoint unavailable. The merge base deliberately applies the limiter to /ipfs/{cid} first and merges the no-walk pins route afterward, and the earlier routing guidance explicitly asked that this boundary remain unchanged.

  Please fix the policy composition at the router boundary: keep /ipfs/{cid} in a router with optional_signature, rate_limit_by_ip, and its IpRateLimiter extension; put /api/v1/ipfs/pins in an auth-only router with optional_signature; then merge the already-layered routers (or use an equivalent structure that produces the same policies). Do not weaken the handler's authentication requirement or broaden/replace the CID limiter. Add a production-router regression using the same ConnectInfo IP that proves pin requests do not debit the CID bucket and an exhausted CID bucket does not block a valid signed pin listing, while retaining the existing unsigned/malformed-signature 401 cases and the /ipfs/{cid} second-request 429 test.

[beardthelion]

Re-reviewed on f49ed79 against origin/main @ bfc44f92. The #121 gates hold under execution: unsigned pin and anchor listings return 401 through build_router, real RFC-9421 signatures return 200, scoped signed non-readers get 404 with no anchor metadata in the body, and ?limit=-1 clamps instead of 500. I gutted the in-handler anonymous checks and the unsigned_get_pins_and_anchors_is_401 test went RED (pins 200); restore returned green. I swept all six named guards one at a time (both optional_signature layers, both in-handler checks, authorize_repo_read, and clamp(0, 200)); each sweep RED'd its paired test.

The scoped ?repo= path is new in this head and has one correctness gap.

Findings

• [P2] Normalize the scoped anchor SQL filter to the stored repo slug

crates/gitlawb-node/src/api/arweave.rs:67

After authorize_repo_read succeeds, the SQL filter uses the raw ?repo= string (format!("{owner}/{name}")). Rows are stored with normalize_owner_key(owner_did)/name (short key). I seeded a private repo and anchor, then drove the owner with a real signature: ?repo=did:key:…/name returned 200 with anchors: [], while the short-slug query returned the seeded row. Authz passes but the listing is empty, so any caller using a full DID in ?repo= gets a false empty page. Use normalize_owner_key(owner) (or the canonical slug from the repo record) when building the SQL filter, and add a regression that queries with the full DID form.

Not an ask, recorded only: unscoped global anchor and pin listings remain auth-only, not visibility-filtered (#136 class). The PR body documents that; I do not block #121 on it. A symmetric malformed-signature test for /arweave/anchors would be nice hygiene (malformed_signature_on_pins exists for pins only); the signed-success test pair already proves the layer is wired.

One process note, not a finding: this head is a slim rebase onto current main (3 files). Expect mechanical conflicts with #285/#377/#385 on server.rs when rebasing again.

[beardthelion]

Re-reviewed 8dd745e9 on merged state. The #121 anonymous path holds: unsigned pin and anchor listings return 401 through build_router, valid RFC-9421 signatures return 200, scoped signed non-readers get 404 with no anchor metadata in the body, and the CID vs pins rate-limit buckets are independent (ipfs_pins_do_not_debit_the_cid_bucket and ipfs_pins_survive_an_exhausted_cid_bucket both green). I gutted the list_pins auth check and unsigned_get_pins_and_anchors_is_401_through_build_router went RED (pins 200 vs 401), then restored.

One scoped-anchor correctness gap remains on the current head.

Findings

• [P2] Normalize the scoped anchor SQL filter to the stored repo slug
  crates/gitlawb-node/src/api/arweave.rs:67
  After authorize_repo_read succeeds, the SQL filter uses the raw ?repo= owner/name strings (format!("{owner}/{name}")). Anchor rows are stored with the bare owner slug (normalize_owner_key / {short}/name in production and in signed_scoped_anchors_owner_succeeds_through_build_router, which only requests the short form). An authorized owner requesting ?repo=did:key:…/name passes authz and returns 200 with anchors: [] while the same repo under the short slug returns the seeded row. Fail-safe, not a leak, but it breaks callers that use the full DID form. Build the SQL filter from normalize_owner_key(owner) or the canonical slug from the authorized repo record, and add a build_router regression where the owner signs with ?repo=did:key:…/name and gets the anchor back.

Not an ask, recorded only: unscoped global anchor and pin listings remain auth-only, not visibility-filtered (#136 class). The PR body documents that.

One process note, not a finding: server.rs overlaps several open PRs (#285, #377, #385, and others). Expect a rebase conflict there, not a review redo.

[beardthelion]

Re-reviewed 4242bb85. My [P2] from the last round is answered on both halves: the SQL filter is now built from the record authorize_repo_read returns, byte-identical to the sole production writer at api/repos.rs:2643, and the full-DID build_router regression I asked for is there. I checked the fix is load-bearing rather than reading it, by reverting the slug construction to the raw ?repo= string, which turned signed_scoped_anchors_full_did_form_matches_stored_slug_through_build_router red. The anonymous gates both endpoints exist for still hold: removing either one on its own turns unsigned_get_pins_and_anchors_is_401_through_build_router red. I also drove the admitted ?repo= owner forms through get_repo and compared normalize_owner_key against the SQL case expression, and could not construct an input where the queried slug is anything other than the resource that was just authorized. CI is 13/13.

One blocker, and it is on the test rather than on the fix.

Findings

• [P2] Assert that the scoped anchor filter excludes another repo's anchors
  crates/gitlawb-node/src/server.rs:979
  Replacing .list_arweave_anchors(Some(&stored_slug), limit) with None leaves the whole anchor suite green, so nothing in the suite distinguishes a correct filter from no filter at all. I ran that mutation single-threaded in a clean worktree to rule out the sqlx test-database race, and it is not a semantic no-op: None turns the scoped listing into a node-wide one. The fixture seeds exactly one anchor and the assertion is presence-only, so the half that is pinned is "the authorized row is present" and the half that is not is "unauthorized rows are absent". That second half is the one whose failure mode is a cross-repo leak rather than the fail-safe empty page the original bug produced. Seed a second repo's anchor with its own irys_tx_id and assert it is absent from the body; I confirmed that assertion passes on this head and goes red under the same mutation before suggesting it.

• [P3] Build the fixture's seed slug with normalize_owner_key
crates/gitlawb-node/src/server.rs:935
  The seed uses owner_did.split(':').next_back().unwrap(), which is the formula the anchor writer moved away from in 6c95592d. It agrees with normalize_owner_key for did:key owners and diverges for every other method, so the test that exists to pin reader and writer together is hand-rolling a third convention, and a writer-side change would leave it green.

• [P3] Correct the pins bullet in the PR description
  crates/gitlawb-node/src/server.rs:245-248
  The description says the optional_signature layer was reordered onto the merged router. The code layers ipfs_pins_routes on its own and merges afterwards. The description also does not mention the stored-slug normalization, which is the whole of this round's change.

Not an ask, recorded only: list_refs at api/repos.rs:2892 still builds its branch_cids filter as format!("{owner}/{repo}") from the raw path while the only writer stores the normalized slug, so it has the same false-empty-as-success behavior you just fixed here. It predates this branch and it already has the record in hand, so I will file it separately rather than grow this PR.

[jatmn]

lgtm

[beardthelion]

Checked head ac9ea61 at base bfc44f9. Ran five build_router regressions (unsigned 401, scoped non-reader 404 with no body leak, negative limit clamp, IPFS bucket split both directions): all green. Premise RED: disabling the list_anchors caller gate flipped unsigned_get_pins_and_anchors_is_401 (anchors returned 200). Vacuity: q.limit.min(200) without the lower clamp flipped signed_anchors_negative_limit_clamps_to_zero; removing optional_signature from the pins sub-router flipped signed_get_pins_and_anchors_succeeds (401). The Aug-15 limit note is addressed at arweave.rs:52 with clamp(0, 200) and a regression test.

Global pin and unscoped anchor listings remain auth-only without per-repo visibility filtering. That is the documented #136 class and out of scope for this PR; not blocking here.