diff --git a/src/api/routes.ts b/src/api/routes.ts index 7908188a13..8aa8a78408 100644 --- a/src/api/routes.ts +++ b/src/api/routes.ts @@ -228,7 +228,7 @@ import { evaluateEscalation } from "../loop-escalation"; import { buildResultsPayload } from "../results-payload"; import { buildProgressSnapshot } from "../loop-progress"; import { validateIdeaSubmission, buildTaskGraph, buildClaimPlan } from "../idea-intake"; -import { loadPrAiReviewFindings } from "../mcp/pr-ai-review-findings"; +import { loadPrAiReviewFindings, assertContributorOwnsPullRequest } from "../mcp/pr-ai-review-findings"; import { buildMcpCompatibilityMetadata, LATEST_RECOMMENDED_MCP_VERSION, @@ -3463,6 +3463,17 @@ export function createApp() { if (!login) return c.json({ error: "login_required" }, 400); const unauthorized = await requireContributorAccess(c, login); if (unauthorized) return unauthorized; + // requireContributorAccess only proves the caller IS `login`; it does not prove `login` authored this PR. + // Mirror the MCP tool's guard order (server.ts getPrAiReviewFindings) so the REST surface can't leak another + // contributor's AI-review findings: 404 when the PR doesn't exist, 403 when it exists but belongs to someone + // else (#8659). + const pullRequest = await getPullRequest(c.env, fullName, number); + if (!pullRequest) return c.json({ error: "not_found" }, 404); + try { + assertContributorOwnsPullRequest(pullRequest.authorLogin, login); + } catch { + return c.json({ error: "forbidden" }, 403); + } return c.json(await loadPrAiReviewFindings(c.env, { repoFullName: fullName, pullNumber: number, login })); }); diff --git a/test/unit/routes-pr-ai-review-findings.test.ts b/test/unit/routes-pr-ai-review-findings.test.ts index 2f686aa61e..096d63bb2f 100644 --- a/test/unit/routes-pr-ai-review-findings.test.ts +++ b/test/unit/routes-pr-ai-review-findings.test.ts @@ -1,6 +1,6 @@ import { describe, expect, it } from "vitest"; import { createApp } from "../../src/api/routes"; -import { markAiReviewPublished, putCachedAiReview, upsertRepositoryFromGitHub } from "../../src/db/repositories"; +import { markAiReviewPublished, putCachedAiReview, upsertPullRequestFromGitHub, upsertRepositoryFromGitHub } from "../../src/db/repositories"; import { upsertRepoFocusManifest } from "../../src/signals/focus-manifest-loader"; import { createTestEnv } from "../helpers/d1"; @@ -8,13 +8,16 @@ import { createTestEnv } from "../helpers/d1"; // loopover_get_pr_ai_review_findings MCP tool. The route validates, gates on requireContributorAccess, then // delegates to loadPrAiReviewFindings (whose own logic is covered by pr-ai-review-findings.test.ts). These // tests therefore pin the ROUTE's contract: each delegated status passes through, and every guard branch -// (invalid number, missing login, non-owning login) is rejected before any data is read. +// (invalid number, missing login, non-owning/non-existent PR) is rejected before any data is read. const apiHeaders = (env: Env) => ({ authorization: `Bearer ${env.LOOPOVER_API_TOKEN}` }); const PATH = "/v1/repos/acme/widgets/pulls/11/ai-review-findings"; -async function seedRepo(env: Env, aiReviewMode: string) { +// Seed PR #11 authored by the requesting login (miner1) so the ownership guard (#8659) passes and the pass-through +// tests reach loadPrAiReviewFindings; individual tests override the author when they need a 403. +async function seedRepo(env: Env, aiReviewMode: string, prAuthor = "miner1") { await upsertRepositoryFromGitHub(env, { name: "widgets", full_name: "acme/widgets", private: false, owner: { login: "acme" } }); await upsertRepoFocusManifest(env, "acme/widgets", { settings: { aiReviewMode } }); + await upsertPullRequestFromGitHub(env, "acme/widgets", { number: 11, title: "PR 11", state: "open", user: { login: prAuthor }, head: { sha: "sha-1" }, labels: [], body: "x" }); } describe("GET /v1/repos/:owner/:repo/pulls/:number/ai-review-findings (#6619)", () => { @@ -109,4 +112,31 @@ describe("GET /v1/repos/:owner/:repo/pulls/:number/ai-review-findings (#6619)", const text = JSON.stringify(await response.json()); expect(text).not.toMatch(/wallet|hotkey|coldkey|trust score|reward estimate/i); }); + + it("403s a caller requesting a PR authored by someone else, without leaking its findings (#8659)", async () => { + // The caller's own session matches `login=miner1`, but PR #11 was authored by other-miner. The route must + // apply the same ownership guard the MCP tool does and reject with 403 -- not return the other author's data. + const app = createApp(); + const env = createTestEnv(); + await seedRepo(env, "advisory", "other-miner"); + await putCachedAiReview(env, "acme/widgets", 11, "sha-1", "advisory", { notes: "Private review.", reviewerCount: 1 }); + await markAiReviewPublished(env, "acme/widgets", 11, "sha-1"); + + const response = await app.request(`${PATH}?login=miner1`, { headers: apiHeaders(env) }, env); + expect(response.status).toBe(403); + await expect(response.json()).resolves.toEqual({ error: "forbidden" }); + }); + + it("404s when the target PR does not exist, distinct from the 403 for a wrong-author PR (#8659)", async () => { + // Repo seeded, AI review on, but no PR #99 record: the ownership guard fetches the PR first, so a missing PR + // is a 404, kept distinct from the 403 an existing-but-not-yours PR returns. + const app = createApp(); + const env = createTestEnv(); + await upsertRepositoryFromGitHub(env, { name: "widgets", full_name: "acme/widgets", private: false, owner: { login: "acme" } }); + await upsertRepoFocusManifest(env, "acme/widgets", { settings: { aiReviewMode: "advisory" } }); + + const response = await app.request(`/v1/repos/acme/widgets/pulls/99/ai-review-findings?login=miner1`, { headers: apiHeaders(env) }, env); + expect(response.status).toBe(404); + await expect(response.json()).resolves.toEqual({ error: "not_found" }); + }); });