Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion src/api/routes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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 }));
});

Expand Down
36 changes: 33 additions & 3 deletions test/unit/routes-pr-ai-review-findings.test.ts
Original file line number Diff line number Diff line change
@@ -1,20 +1,23 @@
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";

// #6619: GET /v1/repos/:owner/:repo/pulls/:number/ai-review-findings — the REST mirror of the
// 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)", () => {
Expand Down Expand Up @@ -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" });
});
});