Skip to content

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

Description

@JSONbored

⚠️ Definition of Done: this issue must be completed in full, in a single PR. Do not split this
work across multiple PRs, and do not defer any Deliverable below to a follow-up issue. A PR that
satisfies only some of the Deliverables, stubs a required test, or leaves a checkbox
partially-done does NOT resolve this issue and will be closed.

Context

src/mcp/server.ts:4533-4551 (getPrAiReviewFindings, the MCP tool handler) fetches the target PR
and calls assertContributorOwnsPullRequest(pullRequest.authorLogin, input.login) (line 4551)
before returning findings — it throws "Forbidden: this tool only returns AI-review findings for your
own pull requests" if the PR's actual author doesn't match the caller's login.

src/api/routes.ts:3458-3466, the REST route exposing the identical capability
(GET /v1/repos/:owner/:repo/pulls/:number/ai-review-findings), only calls
requireContributorAccess(c, login) — which verifies the caller's own session matches the login
query parameter, not that the target PR belongs to that login — and then calls
loadPrAiReviewFindings directly. It never calls getPullRequest or
assertContributorOwnsPullRequest; the ownership-check machinery isn't present in the route at all.
src/mcp/pr-ai-review-findings.ts:106-130 (loadPrAiReviewFindings) itself performs no ownership
check either — assertContributorOwnsPullRequest (line 132) is a separately-exported helper called
from exactly one place in the entire repo: src/mcp/server.ts:4551.

The route's own test file, test/unit/routes-pr-ai-review-findings.test.ts:7-11, documents the
intended contract ("every guard branch (invalid number, missing login, non-owning login) is
rejected before any data is read") but contains no test that actually seeds a PR with a different
author and confirms rejection — because the route currently has no code path that would reject it.

Consequence: any authenticated contributor, using a real session matching their own login, can query
this REST endpoint for any PR number in any repo they can read — not just PRs they authored — and
receive that PR's structured AI-review findings (file paths, line numbers, severity, review body
text), by simply passing their own login as the login query parameter. The MCP tool correctly
blocks this exact request pattern; the REST endpoint for the identical underlying data does not.

Requirements

  • Add the same ownership check the MCP tool already performs to the REST route: fetch the target PR
    (getPullRequest) and call assertContributorOwnsPullRequest(pullRequest.authorLogin, login)
    before calling loadPrAiReviewFindings, mirroring src/mcp/server.ts:4533-4551 exactly.
  • Return a 404/not_found response (matching the MCP tool's behavior) when the PR itself doesn't
    exist, distinct from the 403 returned when the PR exists but belongs to a different author.

Deliverables

  • GET /v1/repos/:owner/:repo/pulls/:number/ai-review-findings calls getPullRequest and
    assertContributorOwnsPullRequest before returning findings, exactly mirroring the MCP tool's
    guard order.
  • A new test seeding a PR authored by other-miner in a test repo, requesting the route as a
    session-authenticated miner1 (?login=miner1), asserting a 403/forbidden response instead
    of the current 200 with real findings data.
  • A regression test confirming the actual owning login still receives 200 with correct
    findings (proving the fix doesn't break the legitimate case).
  • A test confirming a non-existent PR number returns 404/not_found, not a raw error or an
    empty 200.

All four Deliverables are required in the same PR.

Test Coverage Requirements

src/** is measured by codecov/patch (99%+ target, branch-counted). The new tests must exercise
the previously-missing ownership-rejection branch, the still-passing owning-login branch, and the
not-found branch — all three, since this is a security-relevant access-control fix and every branch
of the guard must be provably covered, matching this repo's usual bar for auth-boundary code (see
test/unit/access-boundary.test.ts's existing template for the house style used for this class of
fix).

Expected Outcome

The REST ai-review-findings endpoint enforces the exact same per-PR ownership check the MCP tool
already enforces for the identical data, closing an access-control gap that today lets any
contributor read any other contributor's AI-review findings for any PR in a readable repo.

Links & Resources

  • src/mcp/server.ts:4533-4551 (getPrAiReviewFindings, the already-correct MCP tool to mirror)
  • src/api/routes.ts:3458-3466 (the REST route to fix)
  • src/mcp/pr-ai-review-findings.ts:106-130 (loadPrAiReviewFindings), :132
    (assertContributorOwnsPullRequest, the helper to reuse)
  • test/unit/routes-pr-ai-review-findings.test.ts:7-11 (existing test file documenting the intended,
    currently-unenforced contract)

Metadata

Metadata

Assignees

No one assigned

    Labels

    gittensor:bugGittensor-scored bug fix — scores a 0.05x multiplier.help wantedExtra attention is needed

    Projects

    No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions