From 1ee916e2a41f86abc2f353d28d182f5eaeb24e25 Mon Sep 17 00:00:00 2001 From: shin-core <153108882+shin-core@users.noreply.github.com> Date: Sun, 26 Jul 2026 08:16:29 +0900 Subject: [PATCH] fix(api): enforce PR ownership on the ai-review-findings REST route 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. --- src/api/routes.ts | 13 ++++++- .../unit/routes-pr-ai-review-findings.test.ts | 36 +++++++++++++++++-- 2 files changed, 45 insertions(+), 4 deletions(-) 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" }); + }); });