fix: resolve #477 api(bounties): aggregates ignore repo visibility,... - #483
chenzeyan54-commits wants to merge 1 commit into
Conversation
…exposing private-repo activity `bounty_stats` and `agent_bounty_stats` (`crates/gitlawb-node/src/api/bounties.rs:460-502`) run unfiltered aggregates over all bounties (`crates/gitla... Signed-off-by: chenzeyan54-commits <chenzeyan54-commits@users.noreply.github.com>
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueComment |
|
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. |
euxaristia
left a comment
There was a problem hiding this comment.
Thanks for the fast turnaround on #477. The leak diagnosis and the gate placement are right, and authorize_repo_read is the correct seam for it. Three things I think this needs before merge, plus one minor:
-
The fix trades the information leak for a per-request inventory walk.
bounty_statsandagent_bounty_statsnow page through the entirebountiestable (200 rows perlist_bountiescall, looping until exhausted) and run anauthorize_repo_readlookup per unique repo, on anonymous routes with no cache or limiter (crates/gitlawb-node/src/api/bounties.rs, both handlers). That makes per-request cost proportional to total bounty count, the same shape as #485 on the stats endpoint: with a large bounty table, every stats GET becomes a full-table walk plus N authorization lookups. A SQL-side shape avoids it: joinbountiestoreposand apply the readable predicate in the query (reusing the listing seam'slistable_at_rootlogic), or cache the readable repo set per caller. -
Missing tests. This is an authz change, and the repo's review standards expect deny-probe coverage (see AGENTS.md): seed a bounty on a private repo, then assert
bounty_statsandagent_bounty_statsfrom an anonymous caller and from a caller who can read the repo, and that the private rows move the numbers only for the authorized caller. Triage has already labeled the PRneeds-tests. -
Caller wiring. Both handlers now read
Option<Extension<AuthenticatedDid>>. IfGET /api/v1/bounties/statsandGET /api/v1/bounties/{did}/statsare not mounted behindoptional_signature,calleris alwaysNone: the anonymous leak still closes, but an authenticated repo owner would never see their own private-repo counts in the aggregate. Worth confirming the route group carriesoptional_signature(or adding it).
Minor: the keyset loop in both handlers assumes the list_bounties cursor predicate is strict ((created_at, id) <), so it is worth a glance to rule out a non-advancing cursor spinning the loop. The file is also missing the trailing newline.
bounty_stats and agent_bounty_stats ran unfiltered aggregates over all bounties, leaking private-repo bounty activity to anonymous callers. Mirror the stats() pattern from server.rs (Twigpine#104): 1. Batch-load all deduped repos + visibility rules (2 SQL round-trips) 2. Filter to listable_at_root(rules, is_public, owner_did, None) 3. Pass visible (owner, name) pairs into SQL aggregates via EXISTS/unnest This avoids the N+1 per-row authorize_repo_read problem (PR Twigpine#483) while keeping aggregation in SQL for O(1) per status query after the initial repo resolution. Fail-closed: DB errors collapse the visible set to empty, so all counts return 0 — an under-count never leaks existence. Fixes Twigpine#477
What changed
Fixes #477
Overview
bounty_statsandagent_bounty_stats(crates/gitlawb-node/src/api/bounties.rs:460-502) run unfiltered aggregates over all bounties (`crates/gitla...Key Changes
Verification