fix(visibility): gate repo-scoped read surfaces on visibility (#120)#157
Conversation
Gitlawb#126) The IPFS visibility gate used withheld_blob_oids (a deny-set enumerating only reachable blobs), so a dangling/unreachable blob was absent from the set and served in cleartext to anonymous callers. Flip to an allowed-set (allowed_blob_set_for_caller) that enumerates reachable blobs the caller may read: a dangling blob has no path, is never in the set, and 404s.
Move store::read_object before the allowed_blob_set_for_caller spawn_blocking call so random-CID spray against repos with path-scoped rules cannot trigger full-history git walks on repos that don't carry the object.
…tion ✓ P3 blocker fixed: cargo fmt applied — the format gate will pass. ✓ P3 cleanup resolved: withheld_blob_oids is still used by replication code in repos.rs, so it stays. • P2 follow-up: Tree/commit disclosure tracked in Gitlawb#135 — out of scope here.
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:
📝 WalkthroughWalkthroughRepo-scoped read handlers for certs, issues, labels, bounties, and stars now accept optional authenticated callers and authorize via ChangesVisibility gating for repo-scoped read endpoints
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 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 |
Gravirei
force-pushed
the
fix/issue-120-repo-scoped-read-visibility-gate
branch
from
0df4808 to
fa3c67b
Compare
![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/certs.rs (1)
48-55: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick winScope the certificate lookup to the authorized repo.
Line 48 discards the authorized
record, then Line 53 fetches by certificate id only. A caller who can read repo A could request/repos/A/certs/{cert_id_from_repo_B}and receive repo B certificate metadata. Validatecert.repo_id == record.idor add a repo-scoped DB lookup, returning the same 404 as a missing cert.Proposed guard
- let (_record, _rules) = + let (record, _rules) = crate::api::authorize_repo_read(&state, &owner, &name, caller, "/").await?; let cert = state .db .get_ref_certificate(&id) .await? .ok_or_else(|| AppError::NotFound(format!("certificate {id}")))?; + if cert.repo_id != record.id { + return Err(AppError::NotFound(format!("certificate {id}"))); + }Based on learnings, access-denied and not-found cases should remain externally indistinguishable.
🤖 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/certs.rs` around lines 48 - 55, The certificate lookup in `get_ref_certificate` is only using the certificate id and ignores the authorized repo returned by `authorize_repo_read`, so it can expose metadata from another repo. Update the cert retrieval path in `certs.rs` to scope the lookup to the authorized `record` (either by adding a repo-scoped DB query or by validating `cert.repo_id == record.id` before returning). If the cert is missing or belongs to a different repo, return the same `AppError::NotFound` path used for a missing certificate so access-denied and not-found remain indistinguishable.Source: Learnings
🧹 Nitpick comments (1)
crates/gitlawb-node/src/test_support.rs (1)
2394-2633: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the existing
anon_gethelper for anonymous requests.These new tests hand-build anonymous GET requests via
Request::builder()...inline, duplicating the already-definedanon_get(uri)helper (line 1113) used by earlier tests in this same file.🤖 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 2394 - 2633, The new anonymous GET tests are duplicating request construction instead of using the existing anon_get helper. Update the private-repo gate tests in test_support.rs to call anon_get(...) for each anonymous GET request, matching the style already used elsewhere in this file, and keep signed_request_as only for authenticated cases. This should be applied across the certs, issues, labels, bounties, and stars test cases while preserving the same URIs and assertions.
🤖 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/issues.rs`:
- Around line 195-199: The issue is that the authorized repository result is
discarded and issue comments are fetched by issue_id alone, which can return
comments from a different repo. Update the issues handler in
authorize_repo_read/list_issue_comments flow to use the returned record (or a
repo-scoped comments query) to confirm the issue belongs to the authorized repo
before loading comments. Keep the same external behavior by returning a 404 for
missing or mismatched issues so access-denied and not-found remain
indistinguishable.
In `@crates/gitlawb-node/src/test_support.rs`:
- Around line 2439-2453: The owner-admission tests in
get_cert_gate_admits_owner_on_private, get_issue_gate_admits_owner_on_private,
and list_issue_comments_gate_admits_owner_on_private are only proving a 404
path, so they do not distinguish gate admission from gate denial. Update each
test to seed a real cert/issue/comment that the owner can successfully read,
then assert a success status (or otherwise confirm the handler returned the
resource) instead of relying on a nonexistent id. Keep the existing private-repo
setup and signed_request_as flow, but make the asserted outcome prove that
authorize_repo_read actually allowed the owner through.
---
Outside diff comments:
In `@crates/gitlawb-node/src/api/certs.rs`:
- Around line 48-55: The certificate lookup in `get_ref_certificate` is only
using the certificate id and ignores the authorized repo returned by
`authorize_repo_read`, so it can expose metadata from another repo. Update the
cert retrieval path in `certs.rs` to scope the lookup to the authorized `record`
(either by adding a repo-scoped DB query or by validating `cert.repo_id ==
record.id` before returning). If the cert is missing or belongs to a different
repo, return the same `AppError::NotFound` path used for a missing certificate
so access-denied and not-found remain indistinguishable.
---
Nitpick comments:
In `@crates/gitlawb-node/src/test_support.rs`:
- Around line 2394-2633: The new anonymous GET tests are duplicating request
construction instead of using the existing anon_get helper. Update the
private-repo gate tests in test_support.rs to call anon_get(...) for each
anonymous GET request, matching the style already used elsewhere in this file,
and keep signed_request_as only for authenticated cases. This should be applied
across the certs, issues, labels, bounties, and stars test cases while
preserving the same URIs and assertions.
🪄 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: 0620aa0f-40bb-4126-abe7-25356db3f429
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
crates/gitlawb-node/src/api/bounties.rscrates/gitlawb-node/src/api/certs.rscrates/gitlawb-node/src/api/issues.rscrates/gitlawb-node/src/api/labels.rscrates/gitlawb-node/src/api/mod.rscrates/gitlawb-node/src/api/stars.rscrates/gitlawb-node/src/test_support.rs
💤 Files with no reviewable changes (1)
- crates/gitlawb-node/src/api/mod.rs
beardthelion
added
crate:node
Gravirei
force-pushed
the
fix/issue-120-repo-scoped-read-visibility-gate
branch
2 times, most recently
from
20793cc to
9c46106
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/gitlawb-node/src/test_support.rs (1)
2544-2578: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBare repo at
/tmp/<slug>not cleaned up.
init_barewrites to/tmp/<slug>/secret-repo.gitbut nothing removes it afterward, unlikeadvertisement_serves_real_refs_only_to_authorized_callers/ipfs_cid_*tests which wrap disk paths in aDirGuard. Same pattern repeats inget_issue_gate_admits_owner_on_private(Lines 2605-2651) andlist_issue_comments_gate_admits_owner_on_private(Lines 2678-2733). Harmless for correctness (unique slugs per test) but accumulates stale dirs across CI runs.♻️ Suggested cleanup pattern
- let repo_dir = std::path::PathBuf::from("/tmp") - .join(&slug) - .join("secret-repo.git"); - let _ = std::fs::remove_dir_all(&repo_dir); - std::fs::create_dir_all(repo_dir.parent().unwrap()).unwrap(); - crate::git::store::init_bare(&repo_dir).unwrap(); + let repo_dir = std::path::PathBuf::from("/tmp") + .join(&slug) + .join("secret-repo.git"); + let _ = std::fs::remove_dir_all(&repo_dir); + std::fs::create_dir_all(repo_dir.parent().unwrap()).unwrap(); + crate::git::store::init_bare(&repo_dir).unwrap(); + let _guard = DirGuard(std::path::PathBuf::from("/tmp").join(&slug));🤖 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 2544 - 2578, The test helper in list_issues_gate_admits_owner_on_private leaves the bare repo under /tmp/<slug> behind after init_bare, which should be cleaned up like the other disk-backed tests. Wrap the repo path setup in a DirGuard-style cleanup or add an equivalent teardown around the repo_dir used by init_bare, and apply the same fix to the matching private-repo tests get_issue_gate_admits_owner_on_private and list_issue_comments_gate_admits_owner_on_private so all temporary /tmp/<slug> directories are removed automatically.
🤖 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 2544-2578: The test helper in
list_issues_gate_admits_owner_on_private leaves the bare repo under /tmp/<slug>
behind after init_bare, which should be cleaned up like the other disk-backed
tests. Wrap the repo path setup in a DirGuard-style cleanup or add an equivalent
teardown around the repo_dir used by init_bare, and apply the same fix to the
matching private-repo tests get_issue_gate_admits_owner_on_private and
list_issue_comments_gate_admits_owner_on_private so all temporary /tmp/<slug>
directories are removed automatically.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ea628843-6841-4e81-b74a-bf6be1d14976
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
crates/gitlawb-node/src/api/bounties.rscrates/gitlawb-node/src/api/certs.rscrates/gitlawb-node/src/api/issues.rscrates/gitlawb-node/src/api/labels.rscrates/gitlawb-node/src/api/mod.rscrates/gitlawb-node/src/api/stars.rscrates/gitlawb-node/src/test_support.rs
💤 Files with no reviewable changes (1)
- crates/gitlawb-node/src/api/mod.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- crates/gitlawb-node/src/api/certs.rs
- crates/gitlawb-node/src/api/labels.rs
- crates/gitlawb-node/src/api/bounties.rs
- crates/gitlawb-node/src/api/stars.rs
- crates/gitlawb-node/src/api/issues.rs
Gravirei
force-pushed
the
fix/issue-120-repo-scoped-read-visibility-gate
branch
from
9c46106 to
3ec0acc
Compare
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the contribution. I do not see any actionable issues from my review.
@kevincodex1 LGTM
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
The gate additions on the 8 handlers are correct and the drift guard enforces them (I confirmed by removing get_cert's gate: every_repo_scoped_handler_is_gated fails). But the PR does not close #120 for the bounty class, and it locks the owner out of the one bounty endpoint it does gate.
Findings
-
[P1] Gate
get_bountyandlist_all_bounties— the #120 bounty leak is still open
crates/gitlawb-node/src/api/bounties.rs:150and:137
list_repo_bountiesis gated, but the same private-repo bounty metadata is served unauthenticated byGET /api/v1/bounties/{id}andGET /api/v1/bounties. Neither takes a caller or callsauthorize_repo_read, and their route groupbounty_read_routes(server.rs:240) has nooptional_signaturelayer.create_bountyread-gates, so bounties legitimately sit on private repos carryingrepo_owner/repo_name/issue_id/title/creator_did. I seeded a private repo with a bounty; an anonymous GET to both endpoints returned 200 with"repo_name":"secret-repo", the issue id, title, and creator DID. Resolve each bounty's repo and runauthorize_repo_readat"/"before returning; drop unreadable rows in the list. The drift guard cannot see these because they key on a global id, notPath<(owner, repo)>. -
[P2]
list_repo_bountiesdenies the owner in production; the admit test masks it
crates/gitlawb-node/src/server.rs:240
bounty_read_routesis merged withoutoptional_signature(and there is no global layer), solist_repo_bountiesalways seesauth = Noneand denies the owner on a private repo, not just anonymous callers. I confirmed with a real RFC-9421 signed owner request: 404 against the route with no layer (the production wiring), 200 with the layer added. The owner-admit test misses this becausesigned_request_asinjects theAuthenticatedDidextension directly (test_support.rs:104) instead of routing through the middleware. Attachoptional_signaturetobounty_read_routes(or move the route intoread_routes), and route the gate tests through the real middleware. -
[P2] Add cross-repo IDOR regression tests for the two fixes that lack them
crates/gitlawb-node/src/api/issues.rs:204andcrates/gitlawb-node/src/api/certs.rs:57
Thelist_issue_commentsissue-belongs-to-repo check andget_cert'scert.repo_id != record.idguard each close a real cross-repo IDOR but neither is covered. I removed the comments check and the whole issue test set still passed;get_cert's admit test uses a matchingrepo_id, so the!=branch never runs. Add a repo-A-authorized / repo-B-resource-id test asserting 404 for each. -
[P3] Cover the vacuous deny tests and skipped degenerate states
crates/gitlawb-node/src/test_support.rs
Theget_cert/get_issue/list_issue_commentsdeny tests target a nonexistent id, so they 404 with or without the gate (I removedget_cert's gate and its deny test still passed); seed a matching-repo_idresource so the 404 comes only from the gate. Also untested: a quarantined repo, a public repo anon read (the gate must not over-deny), and owner bare-key vs full did:key (#153/#156).
The gating direction is right and the drift guard is a solid backstop; the bounty leak is the blocker. @kevincodex1 holding this until the bounty readers are gated.
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
The gating direction is right and my prior blocker is resolved: all ten repo-scoped readers route through authorize_repo_read, the drift guard enforces the eight repo-scoped ones (I stripped get_cert's gate and both its deny test and the drift guard go red), is_owner normalizes without cross-method collision, and the deny tests seed real resources so they're load-bearing. Two things block, and the two new global-id gates ship untested.
Findings
-
[P1] Clamp
limitand authorize each repo once inlist_all_bounties— the diff makes an anonymous GET an unbounded DB scan
crates/gitlawb-node/src/api/bounties.rs:137
The handler fetchesq.limitrows (unclamped:list_bountiesbinds it straight intoLIMIT, db/mod.rs:2964) then callsauthorize_repo_readper row, each running three sequential queries (get_repo+is_repo_quarantined+list_visibility_rules, mod.rs:45-57). The route carries onlyoptional_signatureand the outer layer isTraceLayer, no rate limit, soGET /api/v1/bounties?limit=<huge>costs1 + 3Nserial queries for an unauthenticated caller, N growable by anyone who cancreate_bounty. Pre-PR this was one query. A negativelimitalso reaches Postgres and 500s (confirmed). Clamp with theraw_limit.clamp(1, 200)pattern (repos.rs:313) and memoize the per-repo decision. Same clamp applies tolist_repo_bounties. -
[P2] Return the identical 404 body in
get_bountyon an authz denial — it currently leaks the private repo's owner/name
crates/gitlawb-node/src/api/bounties.rs:150
get_bountyresolves the bounty first (NotFound("bounty {id} not found")when absent) then authorizes (RepoNotFound("{owner}/{name}")when private/quarantined). Both are 404 but the bodies differ — I drove it: a missing id returns{"error":"not_found",…}while an id on a private repo returns{"error":"repo_not_found","message":"repository 'did:key:…/secret-repo' not found"}, so an anonymous holder of a bounty id learns it maps to a private repo and reads that repo's owner/name.get_certin this same PR already returns a byte-identicalNotFound("certificate {id}")on both branches (certs.rs:55,58) — mirror that. -
[P2] Add gate tests for
get_bountyandlist_all_bounties— the two new confidentiality branches have none
crates/gitlawb-node/src/api/bounties.rs:137
These are the global-id handlersauthz_guardcannot see (it keys onPath<(owner, repo)>), so no automated backstop; a crate-wide grep finds zero tests for either.list_all_bounties'sif authorize_repo_read(...).is_ok()filter is the only thing keeping private-repo bounties out of the anonymous global feed. Add an anon-deny test forget_bountyon a private repo and alist_all_bountiesfilter test (one private + one public bounty, assert anon sees only the public one), driven throughbuild_router+ a real signature likelist_repo_bounties_gate_admits_owner_on_private(test_support.rs:2963). -
[P3]
list_issue_commentsresolves an issue-id prefix for the existence check but fetches comments by the exact string
crates/gitlawb-node/src/api/issues.rs:204
The belongs-to guard callsgit_issues::get_issue, which resolves an 8-char prefix to the full id, but the next line doeslist_issue_comments(&issue_id)with the raw param. I drove it: a prefix passes the gate then returns{"comments":[]}for an issue that has a comment, and an ambiguous prefix 500s instead of 404s. Bind the DB fetch to the idget_issuereturns. -
[P3]
list_all_bountiesfilters after the limit, so a non-owner can get zero of many readable bounties
crates/gitlawb-node/src/api/bounties.rs:137
It fetches the newestlimitrows then drops unreadable ones with no cursor. I drove it: with three newest private bounties and one older public bounty, an anon?limit=3returns{"bounties":[]}even though the public one is readable. Paginate by a stable cursor and filter before the page limit.
Separate follow-up issue (out of this PR's scope): the bounty mutation endpoints (claim/submit/approve/cancel/dispute) fetch by global id and return the full BountyRecord with no authorize_repo_read, so claim_bounty discloses a private repo's owner/name/title to any authenticated agent — the same leak class this PR closes on the read side.
@kevincodex1 holding for the amplification and the get_bounty error-channel.
…resolved issue id in comments
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the updates. I rechecked the current bounty/read-gate paths and found issues that still need to be addressed.
Findings
-
[P1] Do not treat a denied repo as allowed after the first bounty
crates/gitlawb-node/src/api/bounties.rs:154
The global bounty feed records only that a repo key has been seen, not whetherauthorize_repo_readadmitted that repo. When the first bounty from a private repo is denied,seen.insert(key)still runs; the next bounty from that same private repo then hitsseen.contains(&key)and is pushed intoallowedwithout any authz check. An anonymousGET /api/v1/bountiescan therefore leak every later bounty from a private repo that has more than one bounty in the fetched window. Please memoize the authorization result per repo, not just the repo key, and add a regression with two private bounties from the same repo. -
[P2] Keep paging past hidden bounties instead of stopping at the first 1000 rows
crates/gitlawb-node/src/api/bounties.rs:145
The over-fetch fix still applies the SQLLIMITbefore visibility filtering: withlimit=200, the handler fetches at most the newest 1000 rows, filters private rows in memory, and never looks beyond that window. If those 1000 rows are private and an older public bounty exists, anonymous callers get an empty/short page even though readable bounties are available. Please page/scan with a bounded loop until the requested number of visible rows is collected, or otherwise move the visibility decision ahead of the page limit, and cover the hidden-window case in tests.
beardthelion
dismissed stale reviews from themself
Head advanced to 6fd29c9; superseded by my re-review on the current head.
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
jatmn already has the two I'd otherwise lead with, and I re-verified both by execution: the list_all_bounties memoize leak (P1, bounties.rs:154) still reproduces on this head (two bounties on one private repo, an anonymous GET /api/v1/bounties returns the second), and it clears with a per-repo decision cache; the paging window (P2, bounties.rs:145) is real too. The prior round's other fixes hold: get_bounty no longer leaks the repo identity on denial and the limit clamp kills the negative-limit 500, both confirmed by reverting them and watching the tests go red.
One blocker those reviews don't cover, plus a note on the paging fix.
Findings
- [P2] Gate
star_repoandunstar_repoon repo read, the same asget_star_status
crates/gitlawb-node/src/api/stars.rs:17and:52
This PR gated the star-count read (get_star_status) and the module doc now says "Star count is gated on repo read access," but both write handlers still do a plainget_repoand then returnstar_count, confirm the repo exists, and insert or delete a star on a repo the caller cannot read. Driving a signed non-reader at a private repo:PUT /api/v1/repos/{owner}/{repo}/starreturns201 {"star_count":1,"status":"starred"}andDELETEreturns200, so any self-registered keypair gets an existence oracle, the star count, and an unauthorized write. Swap theget_repolookup in both handlers forauthorize_repo_read(&state, &owner, &repo, Some(caller), "/"); the non-reader then gets the same 404 as an absent repo, and the owner is still admitted (I checked both directions). The doc comment overclaims until this lands.
Note on the paging fix (jatmn's P2, bounties.rs:145)
A direction, not a measured claim: prefer moving the visibility filter into the query over a bounded scan-until-N-visible loop. list_all_bounties already over-fetches up to 1000 rows and runs one authorize_repo_read (three sequential queries) per distinct repo, so a single anonymous GET can cost on the order of 1000 fetched rows plus ~3000 serial authz queries. A loop that keeps fetching until it has collected the requested number of visible rows can amplify that further, whereas filtering to the caller's readable repos in SQL fixes the window and the cost together.
The gating direction is right and the drift guard is a good backstop. Out of scope for #120 but worth its own issue: the bounty mutation endpoints (claim, submit, approve, cancel, dispute) fetch by global id and return the full BountyRecord with no read gate, the same leak class on untouched write handlers.
…ties, gate star/unstar
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the updates. I rechecked the current visibility-gated bounty paths and found issues that still need to be addressed.
Findings
-
[P1] Sign the CLI and MCP reads that now require repo visibility
crates/gl/src/issue.rs:218,crates/gl/src/cert.rs:71,crates/gl/src/bounty.rs:196,crates/gl/src/repo.rs:661,crates/gl/src/star.rs:118,crates/gl/src/mcp.rs:973
The server handlers now correctly useoptional_signatureplusauthorize_repo_read, but the real clients for those reads were not updated to send a signature.gl issue list/show/comments,gl cert list/show,gl bounty list/show,gl bounty show,gl repo label-list,gl star count, and the issue section ingl statusall build unsigned clients and call.get(...); the MCP tools build a client with the loaded keypair but still use unsigned.get(...)for the newly gated issue/bounty reads. As a result, the owner/authorized-reader cases covered by the server tests are not reachable through the shipped CLI/MCP for private repos: they arrive as anonymous and get the same 404 as a stranger. Please load the identity for these gated reads and callget_signedwhen a keypair is available, while keeping anonymous reads for public repos working. -
[P2] Move global bounty visibility filtering ahead of the scan loop
crates/gitlawb-node/src/api/bounties.rs:147
The current fix follows the bounded scan-until-visible direction, but the loop still stops after scanning 10,000 rows and then returns whatever visible rows it found with no cursor,has_more, or incomplete marker. With 10,000 newer hidden bounties and an older public bounty, an anonymousGET /api/v1/bounties?limit=1still returns a complete-looking empty page even though a readable bounty exists; the new regression covers six hidden rows, not the capped path. Please push the readable-repo predicate into the query, or otherwise filter before applying the page limit, so the endpoint cannot present partial results as complete. If the implementation keeps a bounded scan loop instead, return an explicit incomplete/failure state when the cap is reached before filling the requested page.
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
jatmn's two findings hold and I verified both: the unsigned CLI/MCP reads (NodeClient::new(node, None) + .get(), so an authorized owner arrives anonymous and 404s on their own private repo; get_signed already exists at crates/gl/src/http.rs:50), and the capped-scan completeness gap. The read-surface core is solid and I confirmed it by execution: the memoize decision-cache, the multi-page loop, and the star/unstar gate each go RED when neutralized and GREEN on this head, and every BountyRecord reader is gated (get_bounty 404, list_repo/list_all per-row) or aggregate-only.
Two things jatmn's findings don't cover:
Findings
-
[P2] Gate
claim_bountyon repo read
crates/gitlawb-node/src/api/bounties.rs:211
A permissionless signed non-reader POSTing/api/v1/bounties/{id}/claimon a private repo's open bounty gets a 200 with the full record (repo_owner/repo_name/title/creator_did) and becomes its claimant. It is the same data class this PR closes on the read side, reached through a mutation; pre-existing rather than introduced here, but it belongs with the rest of #120. Mirrorget_bounty: onauthorize_repo_read(...).is_err()returnNotFound("bounty {id} not found"). A bareauthorize_repo_read(...)?is not enough on its own; itsRepoNotFoundbody still echoesowner/repo. -
[P2] Fix the OFFSET paging instability while reworking the scan
crates/gitlawb-node/src/api/bounties.rs:135,crates/gitlawb-node/src/db/mod.rs:2959
Separate from jatmn's cap point:list_bountiesorders bycreated_at DESCwith noidtie-break, and the new loop pages by growing OFFSET, so an ordinary concurrent insert shifts every later offset and a readable bounty is duplicated or skipped across the page boundary (reproduced: page 1[b00..b09], page 2[b09, ...]). The same loop is also an anonymous cost lever: no rate limiter, up tomax_rows=10_000rows scanned via deep OFFSET plus up to ~10kauthorize_repo_readcalls per request. The repo already has the pattern that fixes cap-completeness, instability, and cost together:list_ref_updates_keyset((timestamp, id) < (after_ts, after_id),ORDER BY timestamp DESC, id DESC). Reusing it resolves this and jatmn's P2 in one change.
Once claim is gated and the feed moves to the keyset cursor, this is close.
beardthelion
dismissed
their stale review
Superseded by my re-review on 119bfb1 (the memoize/paging/star fixes are verified load-bearing here); dismissing so my state reflects the current head.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the updates. I rechecked the previously discussed paths and do not see any remaining actionable issues from my side.
@beardthelion LGTM
beardthelion
dismissed
their stale review
Superseded by re-review of the new head 0be5e42.
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Re-reviewed the new head 0be5e42 against my earlier blockers and jatmn's. All five are resolved, and I verified each by execution against a local Postgres rather than by reading.
claim_bountynow gates onauthorize_repo_readbefore the status check and the mutation, and maps an authz failure to the samebounty {id} not foundso the body carries no repo path (bounties.rs:245). A signed non-reader gets 404 with no owner/name/title and no status flip; reverting the gate turns that into 200 plus a real claim, so the gate is load-bearing.list_all_bountiesmoved from OFFSET to a(created_at, id)keyset and the scan cap now returns 422Incompleteinstead of a complete-looking partial page (bounties.rs:158,db/mod.rs). Confirmed the keyset is stable across a page boundary under a concurrent insert where OFFSET duplicated a row, and that a feed with 10k+ hidden rows returns 422, not a partial 200.?limitis clamped on both bounty read handlers; a negative limit no longer reaches Postgres asLIMIT -1(500).- The gated reads now sign via
get_authed, so an owner reaches their own private-repo bounty (signed 200) where an anon gets 404. The net-new gates in this PR (bounties/certs/issues/labels/stars) all have their CLI/MCP clients converted, and the drift guard was tightened correctly (the eightknown_ungatedentries removed now that their gates landed).
Non-blocking follow-ups, none of which should hold this PR:
- The list-style clients (
bounty/issue/cert list,label-list,issue comments) parse the body without checkingresp.status(), so a transient node 5xx renders as "No X" and exits 0. The single-itemshowpaths already check status; worth aligning the list paths. signed_clientusesload_keypair_from_dir(dir).ok(), which collapses "no identity" (fine, unsigned public read) and "identity present but unreadable" (a real error) into an unsigned request. On a caller-gated read the second case surfaces as a false "not found" rather than an identity error.- The scan cap can return 422 for an honest query on a very large feed when the caller's visible rows sit past the window. Exposing the internal keyset cursor as a client page param would let those queries page instead.
- Two behaviors here ship without a test: the
claim_bountygate and the 422 cap path. Both are quick to add.
@kevincodex1 good to merge once you're happy with the above.
Summary
Add
authorize_repo_readvisibility gates to 8 repo-scoped read handlers that previously only checked repo existence, leaking private-repo metadata to anonymous callers.Motivation & context
Closes #120
Several repo-scoped read handlers verified the repo exists (
get_repo) but never ran the visibility gate, so a private repo's ref certs, issues, labels, bounties, and star count were accessible without authentication.Kind of change
What changed
certs.rs:list_certs,get_cert— gate ref certificate visibilityissues.rs:list_issues,get_issue,list_issue_comments— gate issue visibilitylabels.rs:list_labels— gate label visibilitybounties.rs:list_repo_bounties— gate bounty visibility (had no repo lookup at all)stars.rs:get_star_status— gate star count visibilityapi/mod.rs: removed gated handlers fromknown_ungatedallowlistHow a reviewer can verify
cargo check -p gitlawb-node cargo test -p gitlawb-node -- api::authz_guardBefore you request review
Summary by CodeRabbit
Bug Fixes
Tests