Skip to content

fix(api): enforce PR ownership on the ai-review-findings REST route - #8663

Merged
JSONbored merged 1 commit into
JSONbored:mainfrom
shin-core:fix/ai-review-findings-route-ownership-8659
Jul 25, 2026
Merged

fix(api): enforce PR ownership on the ai-review-findings REST route#8663
JSONbored merged 1 commit into
JSONbored:mainfrom
shin-core:fix/ai-review-findings-route-ownership-8659

Conversation

@shin-core

Copy link
Copy Markdown
Contributor

What & why

Closes #8659.

GET /v1/repos/:owner/:repo/pulls/:number/ai-review-findings (src/api/routes.ts) only called requireContributorAccess(c, login) — which verifies the caller's session matches the login query param, not that login authored the target PR — then delegated straight to loadPrAiReviewFindings. Neither the route nor loadPrAiReviewFindings performs an ownership check.

Consequence: any authenticated contributor, passing their own login, could query this endpoint for any PR number in any repo they can read and receive that PR's structured AI-review findings (file paths, line numbers, severity, review body). The equivalent MCP tool (server.ts getPrAiReviewFindings) already blocks this exact pattern via getPullRequest + assertContributorOwnsPullRequest; the REST surface for the identical data did not.

The fix

Mirror the MCP tool's guard order on the REST route, before loadPrAiReviewFindings runs:

  • fetch the PR with getPullRequest; 404 { error: "not_found" } when it doesn't exist;
  • assertContributorOwnsPullRequest(pullRequest.authorLogin, login)403 { error: "forbidden" } when the PR exists but belongs to another author.

No change to loadPrAiReviewFindings or any other route.

Tests

Verified bug-catching: reverting the guard makes both the 403 and 404 tests fail (the leak reproduces).

Validation

  • test/unit/routes-pr-ai-review-findings.test.ts: 10 tests pass; typecheck clean; git diff --check clean.
  • 100% of the changed source lines and branches covered (measured on the diff).
  • Branched off current main, mergeable-clean.

GET /v1/repos/:owner/:repo/pulls/:number/ai-review-findings only called
requireContributorAccess (which proves the caller IS the login query param),
never that the login authored the target PR -- so any authenticated contributor
could read ANY PR's structured AI-review findings by passing their own login.
The equivalent MCP tool (server.ts getPrAiReviewFindings) already guards this by
fetching the PR and calling assertContributorOwnsPullRequest. Mirror that guard
order on the REST route: 404 when the PR doesn't exist, 403 when it exists but
belongs to another author, before loadPrAiReviewFindings runs. Adds route tests
for the 403 (wrong-author) and 404 (missing-PR) cases; the existing pass-through
tests now seed the requesting login's own PR so they still reach the delegate.
@shin-core
shin-core requested a review from JSONbored as a code owner July 25, 2026 23:16
@superagent-security superagent-security Bot added the contributor:flagged Contributor flagged for review by trust analysis. label Jul 25, 2026
@superagent-security

Copy link
Copy Markdown
Contributor

🚨 Contributor flagged. Click here for more info: Superagent Dashboard

@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@codecov

codecov Bot commented Jul 25, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.01%. Comparing base (ee390f5) to head (1ee916e).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8663      +/-   ##
==========================================
- Coverage   93.77%   91.01%   -2.76%     
==========================================
  Files         798       97     -701     
  Lines       79475    24895   -54580     
  Branches    24082     4656   -19426     
==========================================
- Hits        74525    22659   -51866     
+ Misses       3565     1971    -1594     
+ Partials     1385      265    -1120     
Flag Coverage Δ
backend 95.30% <100.00%> (+0.26%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/api/routes.ts 95.30% <100.00%> (+<0.01%) ⬆️

... and 701 files with indirect coverage changes

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 25, 2026
@loopover-orb

loopover-orb Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Warning

⏸️ LoopOver review result - manual review recommended

Review updated: 2026-07-25 23:37:00 UTC

2 files · 1 AI reviewer · no blockers · CI green · unstable

⏸️ Suggested Action - Manual Review

Review summary
This closes a real vulnerability where any authenticated contributor could read another contributor's PR AI-review findings via the REST route, since requireContributorAccess only checks caller-identity-equals-login, not PR-authorship. The fix correctly mirrors the MCP tool's guard order (getPullRequest → assertContributorOwnsPullRequest, 404 before 403) and the new tests validate both the 403 and 404 paths plus prove the bug reproduces when the guard is reverted. The change is narrow, well-scoped to the described issue (#8659), and the existing pass-through tests were updated to seed an owning PR so they still exercise the delegate correctly.

Nits — 3 non-blocking
  • src/api/routes.ts:3471,3475 — the external brief's 'magic number' flag for 404/403 is a non-issue; every other route in this file returns bare status codes the same way, so introducing a constant here would be inconsistent.
  • The doubled comment above the new guard (inline + block) is a bit verbose given the PR description already explains the rationale well; could be trimmed to one line.
  • Consider whether assertContributorOwnsPullRequest's error message/type could be inspected instead of blanket try/catch, to avoid accidentally masking an unrelated thrown error as a 403 — but this matches the existing MCP tool's pattern (server.ts) so it's consistent, not a defect.
Flagged checks (non-blocking)
  • Contributor trust — Contributor flagged for review

Decision drivers

  • ✅ Code review — No blockers (1 reviewer)
  • ✅ Gate result — Passing (No configured blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #8659
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 48 registered-repo PR(s), 23 merged, 0 issue(s).
Contributor context ✅ Confirmed Gittensor contributor shin-core; Gittensor profile; 48 PR(s), 0 issue(s).
Improvement ✅ Minor risk: clean · value: minor
Linked issue satisfaction

Addressed
The PR adds the getPullRequest + assertContributorOwnsPullRequest guard directly to the REST route mirroring the MCP tool's order, returning 404 for missing PRs and 403 for non-owning logins, and includes tests for the 403, 404, and passing regression cases as required.

Review context
  • Author: shin-core
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: TypeScript, JavaScript, Solidity, Dart, Python, CSS, PHP, Rust
  • Official Gittensor activity: 48 PR(s), 0 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Keep the PR focused and include validation evidence before maintainer review.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
🧪 Chat with LoopOver

Ask LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.

  • @loopover ask &lt;question&gt; answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat &lt;question&gt; answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

Full command reference: https://loopover.ai/docs/loopover-commands

🧪 Experimental — new and may change.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.

  • Re-run LoopOver review

@loopover-orb loopover-orb Bot 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.

LoopOver approves — the gate is satisfied and CI is green.

@JSONbored
JSONbored merged commit cff95fc into JSONbored:main Jul 25, 2026
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contributor:flagged Contributor flagged for review by trust analysis. gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(orb): REST ai-review-findings route lets any contributor read another contributor's AI-review findings

2 participants