fix(github): Deny pull request approvals at egress

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;

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.",
+    );
+  }
+}

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" })

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,

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) {

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();
+  });
 });