Skip to content

fix: resolve #477 api(bounties): aggregates ignore repo visibility,... - #483

Draft
chenzeyan54-commits wants to merge 1 commit into
Twigpine:mainfrom
chenzeyan54-commits:fix/issue-477
Draft

chenzeyan54-commits wants to merge 1 commit into
Twigpine:mainfrom
chenzeyan54-commits:fix/issue-477

Conversation

@chenzeyan54-commits

Copy link
Copy Markdown

What changed

Fixes #477

Overview

bounty_stats and agent_bounty_stats (crates/gitlawb-node/src/api/bounties.rs:460-502) run unfiltered aggregates over all bounties (`crates/gitla...

Key Changes

Verification

  • Verified patch minimal footprint against target file.
  • Automated Cloud CI workflow will run verification upon submission.

…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>
@coderabbitai

coderabbitai Bot commented Sep 26, 2026

Copy link
Copy Markdown
Contributor

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

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

@github-actions github-actions Bot added the needs-tests Source changed without accompanying tests (advisory) label Sep 26, 2026
@github-actions

Copy link
Copy Markdown
Contributor

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.

@euxaristia euxaristia left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. The fix trades the information leak for a per-request inventory walk. bounty_stats and agent_bounty_stats now page through the entire bounties table (200 rows per list_bounties call, looping until exhausted) and run an authorize_repo_read lookup 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: join bounties to repos and apply the readable predicate in the query (reusing the listing seam's listable_at_root logic), or cache the readable repo set per caller.

  2. 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_stats and agent_bounty_stats from 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 PR needs-tests.

  3. Caller wiring. Both handlers now read Option<Extension<AuthenticatedDid>>. If GET /api/v1/bounties/stats and GET /api/v1/bounties/{did}/stats are not mounted behind optional_signature, caller is always None: 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 carries optional_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.

Mystic-commits pushed a commit to Mystic-commits/node that referenced this pull request Sep 27, 2026
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

crate:node gitlawb-node — the serving node and REST API needs-tests Source changed without accompanying tests (advisory)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

api(bounties): aggregates ignore repo visibility, exposing private-repo activity

3 participants