diff --git a/packages/junior-github/src/plugin.ts b/packages/junior-github/src/plugin.ts index 556fd63767..2e964e2e50 100644 --- a/packages/junior-github/src/plugin.ts +++ b/packages/junior-github/src/plugin.ts @@ -16,6 +16,7 @@ import { normalizePermissions, readGrantPermissions, } from "./permissions.js"; +import { assertGitHubPullRequestApprovalDenied } from "./pull-request-review-policy.js"; import { createGitHubTools } from "./tools.js"; import { createGitHubWebhookRoute } from "./webhooks/handler.js"; import { @@ -489,6 +490,11 @@ function assertGitHubWriteAllowed(input: { operation?: string; upstreamUrl: URL; }): void { + assertGitHubPullRequestApprovalDenied({ + ...(input.bodyText !== undefined ? { bodyText: input.bodyText } : {}), + method: input.method, + upstreamUrl: input.upstreamUrl, + }); if (input.operation === "github.issue.create") return; if (input.operation === "github.issue.update") return; if (input.operation === "github.pull.create") return; diff --git a/packages/junior-github/src/pull-request-review-policy.ts b/packages/junior-github/src/pull-request-review-policy.ts new file mode 100644 index 0000000000..3f38e6743b --- /dev/null +++ b/packages/junior-github/src/pull-request-review-policy.ts @@ -0,0 +1,71 @@ +/** + * Deterministic pull request review policy enforced before credential grant. + */ +import { EgressPolicyDenied } from "@sentry/junior-plugin-api"; +import { isRecord } from "./credential-support.js"; + +/** Deny APPROVE while allowing change requests, comments, and dismissals. */ +export function assertGitHubPullRequestApprovalDenied(input: { + bodyText?: string; + method: string; + upstreamUrl: URL; +}): void { + if ( + input.method !== "POST" || + input.upstreamUrl.hostname.toLowerCase() !== "api.github.com" + ) { + return; + } + const match = input.upstreamUrl.pathname + .toLowerCase() + .match( + /^\/repos\/[^/]+\/[^/]+\/pulls\/[^/]+\/reviews(?:\/[^/]+\/(events))?$/, + ); + if (!match) return; + + const isEventsPath = match[1] === "events"; + const bodyText = input.bodyText?.trim() ?? ""; + // Empty create body is a pending review. The events path always needs a body. + if (!bodyText) { + if (isEventsPath) { + throw new EgressPolicyDenied( + "GitHub pull request review submissions must include a parseable non-APPROVE event so Junior can enforce the no-approve policy.", + ); + } + return; + } + + let body: unknown; + try { + body = JSON.parse(bodyText); + } catch { + throw new EgressPolicyDenied( + "GitHub pull request review requests must use JSON bodies so Junior can enforce the no-approve policy.", + ); + } + if (!isRecord(body)) { + throw new EgressPolicyDenied( + "GitHub pull request review requests must use JSON object bodies so Junior can enforce the no-approve policy.", + ); + } + + let event: string | undefined; + if ("event" in body) { + if (typeof body.event !== "string" || body.event.trim().length === 0) { + throw new EgressPolicyDenied( + "GitHub pull request review submissions must include a parseable non-APPROVE event so Junior can enforce the no-approve policy.", + ); + } + event = body.event.trim().toUpperCase(); + } + if (event === "APPROVE") { + throw new EgressPolicyDenied( + "Junior cannot approve GitHub pull requests. Request changes, leave a comment review, or dismiss Junior's own review instead.", + ); + } + if (isEventsPath && event === undefined) { + throw new EgressPolicyDenied( + "GitHub pull request review submissions must include a parseable non-APPROVE event so Junior can enforce the no-approve policy.", + ); + } +} diff --git a/packages/junior-github/tests/github-plugin.test.ts b/packages/junior-github/tests/github-plugin.test.ts index 01f83220b5..ef51fbf94a 100644 --- a/packages/junior-github/tests/github-plugin.test.ts +++ b/packages/junior-github/tests/github-plugin.test.ts @@ -1924,6 +1924,19 @@ Conversation: \`local:test:old-conversation\` }); await expect( grantForEgress({ + bodyText: JSON.stringify({ event: "REQUEST_CHANGES", body: "nits" }), + method: "POST", + url: "https://api.github.com/repos/getsentry/junior/pulls/780/reviews", + }), + ).resolves.toMatchObject({ + name: "installation-write", + access: "write", + leaseScope: "repository:getsentry/junior", + reason: "github.installation-write", + }); + await expect( + grantForEgress({ + bodyText: JSON.stringify({ event: "COMMENT", body: "looks fine" }), method: "POST", url: "https://api.github.com/repos/getsentry/junior/pulls/780/reviews/99/events", }), @@ -1998,6 +2011,47 @@ Conversation: \`local:test:old-conversation\` ); }); + it("denies GitHub pull request approvals while allowing non-approve reviews", async () => { + await expect( + grantForEgress({ + bodyText: JSON.stringify({ event: "APPROVE", body: "lgtm" }), + method: "POST", + url: "https://api.github.com/repos/getsentry/junior/pulls/780/reviews", + }), + ).rejects.toThrow("Junior cannot approve GitHub pull requests"); + await expect( + grantForEgress({ + bodyText: JSON.stringify({ event: "approve" }), + method: "POST", + url: "https://api.github.com/repos/getsentry/junior/pulls/780/reviews/99/events", + }), + ).rejects.toThrow("Junior cannot approve GitHub pull requests"); + await expect( + grantForEgress({ + method: "POST", + url: "https://api.github.com/repos/getsentry/junior/pulls/780/reviews/99/events", + }), + ).rejects.toThrow( + "review submissions must include a parseable non-APPROVE event", + ); + await expect( + grantForEgress({ + bodyText: "event=APPROVE", + method: "POST", + url: "https://api.github.com/repos/getsentry/junior/pulls/780/reviews", + }), + ).rejects.toThrow("must use JSON bodies"); + await expect( + grantForEgress({ + bodyText: JSON.stringify({ event: 1 }), + method: "POST", + url: "https://api.github.com/repos/getsentry/junior/pulls/780/reviews", + }), + ).rejects.toThrow( + "review submissions must include a parseable non-APPROVE event", + ); + }); + it("preserves installed App permissions on repository-scoped write credentials", async () => { const privateKey = generateKeyPairSync("rsa", { modulusLength: 2048 }) .privateKey.export({ type: "pkcs8", format: "pem" }) diff --git a/packages/junior/src/chat/egress/credentialed.ts b/packages/junior/src/chat/egress/credentialed.ts index 91d098a3e6..bfe1b259ce 100644 --- a/packages/junior/src/chat/egress/credentialed.ts +++ b/packages/junior/src/chat/egress/credentialed.ts @@ -314,20 +314,42 @@ async function requestBodyBytes( return await request.arrayBuffer(); } -function isGrantSelectionBodyVisible(input: { +type GitHubBodyInspection = "graphql" | "pull-request-review"; + +/** Identify GitHub writes whose body determines whether a grant is safe. */ +function githubBodyInspection(input: { provider: string; + requestMethod: string; upstreamUrl: URL; -}): boolean { - return ( - input.provider === "github" && - input.upstreamUrl.hostname.toLowerCase() === "api.github.com" && - input.upstreamUrl.pathname.toLowerCase().endsWith("/graphql") - ); +}): GitHubBodyInspection | undefined { + if ( + input.provider !== "github" || + input.requestMethod.toUpperCase() !== "POST" || + input.upstreamUrl.hostname.toLowerCase() !== "api.github.com" + ) { + return undefined; + } + const pathname = input.upstreamUrl.pathname.toLowerCase(); + if (pathname.endsWith("/graphql")) { + return "graphql"; + } + return /^\/repos\/[^/]+\/[^/]+\/pulls\/[^/]+\/reviews(?:\/[^/]+\/events)?$/.test( + pathname, + ) + ? "pull-request-review" + : undefined; +} + +function grantSelectionBodyTooLargeMessage( + inspection: GitHubBodyInspection, +): string { + return inspection === "graphql" + ? "GitHub GraphQL request body is too large for Junior to inspect before issuing credentials." + : "GitHub pull request review request body is too large for Junior to inspect before issuing credentials."; } function grantSelectionBodyText(input: { body: ArrayBuffer | undefined; - operation?: string; provider: string; request: Request; upstreamUrl: URL; @@ -336,15 +358,14 @@ function grantSelectionBodyText(input: { return undefined; } if (input.body.byteLength > GRANT_SELECTION_BODY_TEXT_LIMIT_BYTES) { - if ( - !input.operation && - input.provider === "github" && - input.request.method.toUpperCase() === "POST" && - input.upstreamUrl.hostname.toLowerCase() === "api.github.com" && - input.upstreamUrl.pathname.toLowerCase().endsWith("/graphql") - ) { + const inspection = githubBodyInspection({ + provider: input.provider, + requestMethod: input.request.method, + upstreamUrl: input.upstreamUrl, + }); + if (inspection) { throw new EgressPolicyDenied( - "GitHub GraphQL request body is too large for Junior to inspect before issuing credentials.", + grantSelectionBodyTooLargeMessage(inspection), ); } return undefined; @@ -579,8 +600,9 @@ export async function executeCredentialedEgressRequest(input: { request, upstreamUrl, } = input; - const bodyForGrantSelection = isGrantSelectionBodyVisible({ + const bodyForGrantSelection = githubBodyInspection({ provider, + requestMethod: request.method, upstreamUrl, }) ? await requestBodyBytes(request) @@ -590,7 +612,6 @@ export async function executeCredentialedEgressRequest(input: { grantSelection = await selectSandboxEgressGrant({ bodyText: grantSelectionBodyText({ body: bodyForGrantSelection, - ...(operation ? { operation } : {}), provider, request, upstreamUrl, diff --git a/packages/junior/tests/integration/sandbox-egress-proxy.test.ts b/packages/junior/tests/integration/sandbox-egress-proxy.test.ts index 3665cb9f2a..ac6189e634 100644 --- a/packages/junior/tests/integration/sandbox-egress-proxy.test.ts +++ b/packages/junior/tests/integration/sandbox-egress-proxy.test.ts @@ -1419,6 +1419,44 @@ describe("sandbox egress proxy integration", () => { }); }); + it("denies oversized raw GitHub pull request review submits before credential injection", async () => { + await registerGitHubPlugin(); + const credentialToken = modules.session.createSandboxEgressCredentialToken({ + credentials: { actor: { type: "user", userId: ACTOR_ID } }, + egressId: EGRESS_ID, + ttlMs: 60_000, + }); + const networkPolicy = modules.policy.buildSandboxEgressNetworkPolicy({ + credentialToken, + }); + const forwardURL = forwardUrlFor(networkPolicy, GITHUB_API_HOST); + const upstreamFetch = vi.fn(); + + const response = await modules.proxy.proxySandboxEgressRequest( + proxiedRequest({ + body: JSON.stringify({ + event: "APPROVE", + body: "x".repeat(70 * 1024), + }), + forwardURL, + method: "POST", + upstreamHost: GITHUB_API_HOST, + upstreamPath: "/repos/getsentry/junior/pulls/780/reviews", + }), + { + fetch: upstreamFetch as typeof fetch, + verifyOidc: async () => ({ sandbox_id: EGRESS_ID }), + }, + ); + + expect(response.status).toBe(403); + await expect(response.json()).resolves.toEqual({ + error: + "GitHub pull request review request body is too large for Junior to inspect before issuing credentials.", + }); + expect(upstreamFetch).not.toHaveBeenCalled(); + }); + it("records plugin write auth needs over earlier read failures", async () => { await registerManagedEgressPlugin({ issueCredential(ctx) { diff --git a/packages/junior/tests/unit/plugins/plugin-egress.test.ts b/packages/junior/tests/unit/plugins/plugin-egress.test.ts index 6e8e4b918d..8ec34636ef 100644 --- a/packages/junior/tests/unit/plugins/plugin-egress.test.ts +++ b/packages/junior/tests/unit/plugins/plugin-egress.test.ts @@ -1,5 +1,6 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { defineJuniorPlugin } from "@sentry/junior-plugin-api"; +import { githubPlugin } from "@sentry/junior-github"; import { createPluginEgress } from "@/chat/egress/plugin"; import { pluginCatalogRuntime } from "@/chat/plugins/catalog-runtime"; import { setPlugins } from "@/chat/plugins/agent-hooks"; @@ -187,4 +188,33 @@ describe("plugin egress", () => { expect(response.status).toBe(403); await expect(response.text()).resolves.toBe("forbidden"); }); + + it("denies pull request approvals on plugin egress even when operation is set", async () => { + setPlugins([githubPlugin()]); + const fetchMock = vi.fn(); + const egress = createPluginEgress({ + credentialContext: { actor: { type: "user", userId: "U123" } }, + fetch: fetchMock as unknown as typeof fetch, + pluginAuth: authOrchestration(), + }); + + const response = await egress.fetch({ + provider: "github", + operation: "github.pull.update", + request: new Request( + "https://api.github.com/repos/getsentry/junior/pulls/780/reviews", + { + method: "POST", + body: JSON.stringify({ event: "APPROVE", body: "lgtm" }), + }, + ), + }); + + expect(response.status).toBe(403); + await expect(response.json()).resolves.toEqual({ + error: + "Junior cannot approve GitHub pull requests. Request changes, leave a comment review, or dismiss Junior's own review instead.", + }); + expect(fetchMock).not.toHaveBeenCalled(); + }); });