From 8a255d736300f3e3452a84387ab128cc7b9176a8 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Thu, 13 Aug 2026 00:18:01 +0000 Subject: [PATCH 1/4] fix(github): Deny pull request approvals at egress Block APPROVE on review create/submit while still allowing request-changes, comment reviews, and dismissals. Inspect review REST bodies before credential grant so oversized approve attempts cannot skip the check. Co-Authored-By: David Cramer --- packages/junior-github/src/plugin.ts | 6 ++ packages/junior-github/src/write-policy.ts | 86 +++++++++++++++++++ .../junior-github/tests/github-plugin.test.ts | 47 ++++++++++ .../junior/src/chat/egress/credentialed.ts | 70 ++++++++++++--- .../integration/sandbox-egress-proxy.test.ts | 55 ++++++++++++ 5 files changed, 253 insertions(+), 11 deletions(-) create mode 100644 packages/junior-github/src/write-policy.ts diff --git a/packages/junior-github/src/plugin.ts b/packages/junior-github/src/plugin.ts index eef1138d83..122ef1c2c9 100644 --- a/packages/junior-github/src/plugin.ts +++ b/packages/junior-github/src/plugin.ts @@ -78,6 +78,7 @@ import { type GitHubGrantName, type GitHubGrantReason, } from "./credential-support.js"; +import { assertGitHubPullRequestApprovalDenied } from "./write-policy.js"; /** Configure the built-in GitHub plugin manifest and hooks. */ export interface GitHubPluginOptions { @@ -534,6 +535,11 @@ function assertGitHubWriteAllowed(input: { `GitHub pull request creation must use the github_createPullRequest tool so Junior can own idempotency and the conversation footer. ${CREATE_TOOL_ROUTING_GUIDANCE}`, ); } + assertGitHubPullRequestApprovalDenied({ + ...(input.bodyText !== undefined ? { bodyText: input.bodyText } : {}), + method: input.method, + upstreamUrl: input.upstreamUrl, + }); } function grantForAccess( diff --git a/packages/junior-github/src/write-policy.ts b/packages/junior-github/src/write-policy.ts new file mode 100644 index 0000000000..d658609b9f --- /dev/null +++ b/packages/junior-github/src/write-policy.ts @@ -0,0 +1,86 @@ +/** + * Deterministic GitHub write denials enforced before credential grant. + */ +import { EgressPolicyDenied } from "@sentry/junior-plugin-api"; +import { isRecord } from "./credential-support.js"; + +function isGitHubApiUrl(upstreamUrl: URL): boolean { + return upstreamUrl.hostname.toLowerCase() === "api.github.com"; +} + +function isGitHubPullRequestReviewCreateRestRequest( + method: string, + upstreamUrl: URL, +): boolean { + return ( + method === "POST" && + isGitHubApiUrl(upstreamUrl) && + /^\/repos\/[^/]+\/[^/]+\/pulls\/[^/]+\/reviews$/.test( + upstreamUrl.pathname.toLowerCase(), + ) + ); +} + +function isGitHubPullRequestReviewEventRestRequest( + method: string, + upstreamUrl: URL, +): boolean { + return ( + method === "POST" && + isGitHubApiUrl(upstreamUrl) && + /^\/repos\/[^/]+\/[^/]+\/pulls\/[^/]+\/reviews\/[^/]+\/events$/.test( + upstreamUrl.pathname.toLowerCase(), + ) + ); +} + +function parseGitHubPullRequestReviewEvent( + bodyText: string | undefined, +): string | undefined { + if (typeof bodyText !== "string" || bodyText.trim().length === 0) { + return undefined; + } + let parsed: unknown; + try { + parsed = JSON.parse(bodyText); + } catch { + return undefined; + } + if (!isRecord(parsed) || typeof parsed.event !== "string") { + return undefined; + } + const event = parsed.event.trim().toUpperCase(); + return event.length > 0 ? event : undefined; +} + +/** Deny APPROVE while still allowing REQUEST_CHANGES, COMMENT, and dismissals. */ +export function assertGitHubPullRequestApprovalDenied(input: { + bodyText?: string; + method: string; + upstreamUrl: URL; +}): void { + const isReviewCreate = isGitHubPullRequestReviewCreateRestRequest( + input.method, + input.upstreamUrl, + ); + const isReviewEvent = isGitHubPullRequestReviewEventRestRequest( + input.method, + input.upstreamUrl, + ); + if (!isReviewCreate && !isReviewEvent) { + return; + } + + const event = parseGitHubPullRequestReviewEvent(input.bodyText); + 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 (isReviewEvent && 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.", + ); + } + // Pending review creates may omit event. Keep those allowed. +} diff --git a/packages/junior-github/tests/github-plugin.test.ts b/packages/junior-github/tests/github-plugin.test.ts index f51ac6bb38..c3e81e8c35 100644 --- a/packages/junior-github/tests/github-plugin.test.ts +++ b/packages/junior-github/tests/github-plugin.test.ts @@ -1907,6 +1907,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", }), @@ -1981,6 +1994,40 @@ 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: "{", + 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", + ); + }); + 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..de8ee3a656 100644 --- a/packages/junior/src/chat/egress/credentialed.ts +++ b/packages/junior/src/chat/egress/credentialed.ts @@ -314,17 +314,70 @@ async function requestBodyBytes( return await request.arrayBuffer(); } +function isGitHubApiHost(upstreamUrl: URL): boolean { + return upstreamUrl.hostname.toLowerCase() === "api.github.com"; +} + +function isGitHubGraphqlPath(upstreamUrl: URL): boolean { + return upstreamUrl.pathname.toLowerCase().endsWith("/graphql"); +} + +/** REST review submits need body inspection so Junior can deny APPROVE. */ +function isGitHubPullRequestReviewSubmitPath(upstreamUrl: URL): boolean { + const pathname = upstreamUrl.pathname.toLowerCase(); + return ( + /^\/repos\/[^/]+\/[^/]+\/pulls\/[^/]+\/reviews$/.test(pathname) || + /^\/repos\/[^/]+\/[^/]+\/pulls\/[^/]+\/reviews\/[^/]+\/events$/.test( + pathname, + ) + ); +} + function isGrantSelectionBodyVisible(input: { provider: string; + requestMethod: string; upstreamUrl: URL; }): boolean { + if (input.provider !== "github" || !isGitHubApiHost(input.upstreamUrl)) { + return false; + } + if (isGitHubGraphqlPath(input.upstreamUrl)) { + return true; + } return ( - input.provider === "github" && - input.upstreamUrl.hostname.toLowerCase() === "api.github.com" && - input.upstreamUrl.pathname.toLowerCase().endsWith("/graphql") + input.requestMethod.toUpperCase() === "POST" && + isGitHubPullRequestReviewSubmitPath(input.upstreamUrl) ); } +function grantSelectionBodyRequiresInspection(input: { + operation?: string; + provider: string; + request: Request; + upstreamUrl: URL; +}): boolean { + if (input.operation || input.provider !== "github") { + return false; + } + if (input.request.method.toUpperCase() !== "POST") { + return false; + } + if (!isGitHubApiHost(input.upstreamUrl)) { + return false; + } + return ( + isGitHubGraphqlPath(input.upstreamUrl) || + isGitHubPullRequestReviewSubmitPath(input.upstreamUrl) + ); +} + +function grantSelectionBodyTooLargeMessage(upstreamUrl: URL): string { + if (isGitHubGraphqlPath(upstreamUrl)) { + return "GitHub GraphQL request body is too large for Junior to inspect before issuing credentials."; + } + return "GitHub pull request review request body is too large for Junior to inspect before issuing credentials."; +} + function grantSelectionBodyText(input: { body: ArrayBuffer | undefined; operation?: string; @@ -336,15 +389,9 @@ 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") - ) { + if (grantSelectionBodyRequiresInspection(input)) { throw new EgressPolicyDenied( - "GitHub GraphQL request body is too large for Junior to inspect before issuing credentials.", + grantSelectionBodyTooLargeMessage(input.upstreamUrl), ); } return undefined; @@ -581,6 +628,7 @@ export async function executeCredentialedEgressRequest(input: { } = input; const bodyForGrantSelection = isGrantSelectionBodyVisible({ provider, + requestMethod: request.method, upstreamUrl, }) ? await requestBodyBytes(request) diff --git a/packages/junior/tests/integration/sandbox-egress-proxy.test.ts b/packages/junior/tests/integration/sandbox-egress-proxy.test.ts index bda64b7bc9..b14653ecd8 100644 --- a/packages/junior/tests/integration/sandbox-egress-proxy.test.ts +++ b/packages/junior/tests/integration/sandbox-egress-proxy.test.ts @@ -1372,6 +1372,61 @@ describe("sandbox egress proxy integration", () => { }); }); + it("denies oversized raw GitHub pull request review submits before credential injection", async () => { + await registerGitHubPlugin(); + const records: EmittedLogRecord[] = []; + const unregister = modules.logging.registerLogRecordSink((record) => { + records.push(record); + }); + 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(); + + let response: Response; + try { + 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 }), + }, + ); + } finally { + unregister(); + } + + 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(); + expect( + records.find( + (record) => record.eventName === "sandbox.egress.policy.denied", + )?.attributes, + ).toMatchObject({ + "app.sandbox.egress.policy.reason": + "GitHub pull request review request body is too large for Junior to inspect before issuing credentials.", + }); + }); + it("records plugin write auth needs over earlier read failures", async () => { await registerManagedEgressPlugin({ issueCredential(ctx) { From 8bad2283c5f85fa80eae01c230302e7cfe2feec5 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Thu, 13 Aug 2026 00:23:40 +0000 Subject: [PATCH 2/4] refactor(github): Simplify approval egress policy Co-Authored-By: David Cramer --- packages/junior-github/src/plugin.ts | 40 +++++++- packages/junior-github/src/write-policy.ts | 86 ----------------- .../junior-github/tests/github-plugin.test.ts | 9 -- .../junior/src/chat/egress/credentialed.ts | 93 +++++++------------ .../integration/sandbox-egress-proxy.test.ts | 47 +++------- 5 files changed, 90 insertions(+), 185 deletions(-) delete mode 100644 packages/junior-github/src/write-policy.ts diff --git a/packages/junior-github/src/plugin.ts b/packages/junior-github/src/plugin.ts index 122ef1c2c9..d93493eb9a 100644 --- a/packages/junior-github/src/plugin.ts +++ b/packages/junior-github/src/plugin.ts @@ -78,7 +78,6 @@ import { type GitHubGrantName, type GitHubGrantReason, } from "./credential-support.js"; -import { assertGitHubPullRequestApprovalDenied } from "./write-policy.js"; /** Configure the built-in GitHub plugin manifest and hooks. */ export interface GitHubPluginOptions { @@ -499,6 +498,45 @@ function isGitHubPullCreateGraphqlMutation( ).test(parsed.normalized); } +/** Deny APPROVE while allowing change requests, comments, and dismissals. */ +function assertGitHubPullRequestApprovalDenied(input: { + bodyText?: string; + method: string; + upstreamUrl: URL; +}): void { + if (input.method !== "POST" || !isGitHubApiUrl(input.upstreamUrl)) { + return; + } + const match = input.upstreamUrl.pathname + .toLowerCase() + .match( + /^\/repos\/[^/]+\/[^/]+\/pulls\/[^/]+\/reviews(?:\/[^/]+\/(events))?$/, + ); + if (!match) return; + + let event: string | undefined; + if (input.bodyText?.trim()) { + try { + const body: unknown = JSON.parse(input.bodyText); + if (isRecord(body) && typeof body.event === "string") { + event = body.event.trim().toUpperCase() || undefined; + } + } catch { + // The events endpoint fails closed below. A create without event is pending. + } + } + 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 (match[1] === "events" && 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.", + ); + } +} + function assertGitHubWriteAllowed(input: { bodyText?: string; method: string; diff --git a/packages/junior-github/src/write-policy.ts b/packages/junior-github/src/write-policy.ts deleted file mode 100644 index d658609b9f..0000000000 --- a/packages/junior-github/src/write-policy.ts +++ /dev/null @@ -1,86 +0,0 @@ -/** - * Deterministic GitHub write denials enforced before credential grant. - */ -import { EgressPolicyDenied } from "@sentry/junior-plugin-api"; -import { isRecord } from "./credential-support.js"; - -function isGitHubApiUrl(upstreamUrl: URL): boolean { - return upstreamUrl.hostname.toLowerCase() === "api.github.com"; -} - -function isGitHubPullRequestReviewCreateRestRequest( - method: string, - upstreamUrl: URL, -): boolean { - return ( - method === "POST" && - isGitHubApiUrl(upstreamUrl) && - /^\/repos\/[^/]+\/[^/]+\/pulls\/[^/]+\/reviews$/.test( - upstreamUrl.pathname.toLowerCase(), - ) - ); -} - -function isGitHubPullRequestReviewEventRestRequest( - method: string, - upstreamUrl: URL, -): boolean { - return ( - method === "POST" && - isGitHubApiUrl(upstreamUrl) && - /^\/repos\/[^/]+\/[^/]+\/pulls\/[^/]+\/reviews\/[^/]+\/events$/.test( - upstreamUrl.pathname.toLowerCase(), - ) - ); -} - -function parseGitHubPullRequestReviewEvent( - bodyText: string | undefined, -): string | undefined { - if (typeof bodyText !== "string" || bodyText.trim().length === 0) { - return undefined; - } - let parsed: unknown; - try { - parsed = JSON.parse(bodyText); - } catch { - return undefined; - } - if (!isRecord(parsed) || typeof parsed.event !== "string") { - return undefined; - } - const event = parsed.event.trim().toUpperCase(); - return event.length > 0 ? event : undefined; -} - -/** Deny APPROVE while still allowing REQUEST_CHANGES, COMMENT, and dismissals. */ -export function assertGitHubPullRequestApprovalDenied(input: { - bodyText?: string; - method: string; - upstreamUrl: URL; -}): void { - const isReviewCreate = isGitHubPullRequestReviewCreateRestRequest( - input.method, - input.upstreamUrl, - ); - const isReviewEvent = isGitHubPullRequestReviewEventRestRequest( - input.method, - input.upstreamUrl, - ); - if (!isReviewCreate && !isReviewEvent) { - return; - } - - const event = parseGitHubPullRequestReviewEvent(input.bodyText); - 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 (isReviewEvent && 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.", - ); - } - // Pending review creates may omit event. Keep those allowed. -} diff --git a/packages/junior-github/tests/github-plugin.test.ts b/packages/junior-github/tests/github-plugin.test.ts index c3e81e8c35..c77a2dfad0 100644 --- a/packages/junior-github/tests/github-plugin.test.ts +++ b/packages/junior-github/tests/github-plugin.test.ts @@ -2017,15 +2017,6 @@ Conversation: \`local:test:old-conversation\` ).rejects.toThrow( "review submissions must include a parseable non-APPROVE event", ); - await expect( - grantForEgress({ - bodyText: "{", - 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", - ); }); it("preserves installed App permissions on repository-scoped write credentials", async () => { diff --git a/packages/junior/src/chat/egress/credentialed.ts b/packages/junior/src/chat/egress/credentialed.ts index de8ee3a656..22f2fc0e3f 100644 --- a/packages/junior/src/chat/egress/credentialed.ts +++ b/packages/junior/src/chat/egress/credentialed.ts @@ -314,68 +314,40 @@ async function requestBodyBytes( return await request.arrayBuffer(); } -function isGitHubApiHost(upstreamUrl: URL): boolean { - return upstreamUrl.hostname.toLowerCase() === "api.github.com"; -} - -function isGitHubGraphqlPath(upstreamUrl: URL): boolean { - return upstreamUrl.pathname.toLowerCase().endsWith("/graphql"); -} - -/** REST review submits need body inspection so Junior can deny APPROVE. */ -function isGitHubPullRequestReviewSubmitPath(upstreamUrl: URL): boolean { - const pathname = upstreamUrl.pathname.toLowerCase(); - return ( - /^\/repos\/[^/]+\/[^/]+\/pulls\/[^/]+\/reviews$/.test(pathname) || - /^\/repos\/[^/]+\/[^/]+\/pulls\/[^/]+\/reviews\/[^/]+\/events$/.test( - pathname, - ) - ); -} - -function isGrantSelectionBodyVisible(input: { - provider: string; - requestMethod: string; - upstreamUrl: URL; -}): boolean { - if (input.provider !== "github" || !isGitHubApiHost(input.upstreamUrl)) { - return false; - } - if (isGitHubGraphqlPath(input.upstreamUrl)) { - return true; - } - return ( - input.requestMethod.toUpperCase() === "POST" && - isGitHubPullRequestReviewSubmitPath(input.upstreamUrl) - ); -} +type GitHubBodyInspection = "graphql" | "pull-request-review"; -function grantSelectionBodyRequiresInspection(input: { +/** Identify raw GitHub writes whose body determines whether a grant is safe. */ +function githubBodyInspection(input: { operation?: string; provider: string; - request: Request; + requestMethod: string; upstreamUrl: URL; -}): boolean { - if (input.operation || input.provider !== "github") { - return false; - } - if (input.request.method.toUpperCase() !== "POST") { - return false; +}): GitHubBodyInspection | undefined { + if ( + input.operation || + input.provider !== "github" || + input.requestMethod.toUpperCase() !== "POST" || + input.upstreamUrl.hostname.toLowerCase() !== "api.github.com" + ) { + return undefined; } - if (!isGitHubApiHost(input.upstreamUrl)) { - return false; + const pathname = input.upstreamUrl.pathname.toLowerCase(); + if (pathname.endsWith("/graphql")) { + return "graphql"; } - return ( - isGitHubGraphqlPath(input.upstreamUrl) || - isGitHubPullRequestReviewSubmitPath(input.upstreamUrl) - ); + return /^\/repos\/[^/]+\/[^/]+\/pulls\/[^/]+\/reviews(?:\/[^/]+\/events)?$/.test( + pathname, + ) + ? "pull-request-review" + : undefined; } -function grantSelectionBodyTooLargeMessage(upstreamUrl: URL): string { - if (isGitHubGraphqlPath(upstreamUrl)) { - return "GitHub GraphQL request body is too large for Junior to inspect before issuing credentials."; - } - return "GitHub pull request review request body is too large for Junior to inspect before issuing credentials."; +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: { @@ -389,9 +361,15 @@ function grantSelectionBodyText(input: { return undefined; } if (input.body.byteLength > GRANT_SELECTION_BODY_TEXT_LIMIT_BYTES) { - if (grantSelectionBodyRequiresInspection(input)) { + const inspection = githubBodyInspection({ + ...(input.operation ? { operation: input.operation } : {}), + provider: input.provider, + requestMethod: input.request.method, + upstreamUrl: input.upstreamUrl, + }); + if (inspection) { throw new EgressPolicyDenied( - grantSelectionBodyTooLargeMessage(input.upstreamUrl), + grantSelectionBodyTooLargeMessage(inspection), ); } return undefined; @@ -626,7 +604,8 @@ export async function executeCredentialedEgressRequest(input: { request, upstreamUrl, } = input; - const bodyForGrantSelection = isGrantSelectionBodyVisible({ + const bodyForGrantSelection = githubBodyInspection({ + ...(operation ? { operation } : {}), provider, requestMethod: request.method, upstreamUrl, diff --git a/packages/junior/tests/integration/sandbox-egress-proxy.test.ts b/packages/junior/tests/integration/sandbox-egress-proxy.test.ts index b14653ecd8..088735c801 100644 --- a/packages/junior/tests/integration/sandbox-egress-proxy.test.ts +++ b/packages/junior/tests/integration/sandbox-egress-proxy.test.ts @@ -1374,10 +1374,6 @@ describe("sandbox egress proxy integration", () => { it("denies oversized raw GitHub pull request review submits before credential injection", async () => { await registerGitHubPlugin(); - const records: EmittedLogRecord[] = []; - const unregister = modules.logging.registerLogRecordSink((record) => { - records.push(record); - }); const credentialToken = modules.session.createSandboxEgressCredentialToken({ credentials: { actor: { type: "user", userId: ACTOR_ID } }, egressId: EGRESS_ID, @@ -1389,27 +1385,22 @@ describe("sandbox egress proxy integration", () => { const forwardURL = forwardUrlFor(networkPolicy, GITHUB_API_HOST); const upstreamFetch = vi.fn(); - let response: Response; - try { - 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", + const response = await modules.proxy.proxySandboxEgressRequest( + proxiedRequest({ + body: JSON.stringify({ + event: "APPROVE", + body: "x".repeat(70 * 1024), }), - { - fetch: upstreamFetch as typeof fetch, - verifyOidc: async () => ({ sandbox_id: EGRESS_ID }), - }, - ); - } finally { - unregister(); - } + 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({ @@ -1417,14 +1408,6 @@ describe("sandbox egress proxy integration", () => { "GitHub pull request review request body is too large for Junior to inspect before issuing credentials.", }); expect(upstreamFetch).not.toHaveBeenCalled(); - expect( - records.find( - (record) => record.eventName === "sandbox.egress.policy.denied", - )?.attributes, - ).toMatchObject({ - "app.sandbox.egress.policy.reason": - "GitHub pull request review request body is too large for Junior to inspect before issuing credentials.", - }); }); it("records plugin write auth needs over earlier read failures", async () => { From 04ae95d2007b3bb1d53e08347d8a29d630235ae2 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Thu, 13 Aug 2026 00:29:32 +0000 Subject: [PATCH 3/4] fix(github): Fail closed on non-JSON review creates Require parseable JSON review bodies on POST /reviews so form-encoded or malformed APPROVE payloads cannot skip the no-approve gate. Empty create bodies remain allowed for pending reviews. --- packages/junior-github/src/plugin.ts | 42 +++++++++++++++---- .../junior-github/tests/github-plugin.test.ts | 16 +++++++ 2 files changed, 49 insertions(+), 9 deletions(-) diff --git a/packages/junior-github/src/plugin.ts b/packages/junior-github/src/plugin.ts index d93493eb9a..7f2be709e7 100644 --- a/packages/junior-github/src/plugin.ts +++ b/packages/junior-github/src/plugin.ts @@ -514,23 +514,47 @@ function assertGitHubPullRequestApprovalDenied(input: { ); 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 (input.bodyText?.trim()) { - try { - const body: unknown = JSON.parse(input.bodyText); - if (isRecord(body) && typeof body.event === "string") { - event = body.event.trim().toUpperCase() || undefined; - } - } catch { - // The events endpoint fails closed below. A create without event is pending. + 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 (match[1] === "events" && event === undefined) { + 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 c77a2dfad0..9dd064e1ea 100644 --- a/packages/junior-github/tests/github-plugin.test.ts +++ b/packages/junior-github/tests/github-plugin.test.ts @@ -2017,6 +2017,22 @@ Conversation: \`local:test:old-conversation\` ).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 () => { From e5e76332aaf492fc7862b8cc3483423984723d57 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Thu, 13 Aug 2026 00:43:58 +0000 Subject: [PATCH 4/4] fix(github): Inspect review bodies on plugin egress Stop skipping GitHub body inspection when an operation is set. Plugin-tool egress always carries an operation, and that short-circuit let APPROVE create-review requests bypass the no-approve gate. --- .../junior/src/chat/egress/credentialed.ts | 8 +---- .../tests/unit/plugins/plugin-egress.test.ts | 30 +++++++++++++++++++ 2 files changed, 31 insertions(+), 7 deletions(-) diff --git a/packages/junior/src/chat/egress/credentialed.ts b/packages/junior/src/chat/egress/credentialed.ts index 22f2fc0e3f..bfe1b259ce 100644 --- a/packages/junior/src/chat/egress/credentialed.ts +++ b/packages/junior/src/chat/egress/credentialed.ts @@ -316,15 +316,13 @@ async function requestBodyBytes( type GitHubBodyInspection = "graphql" | "pull-request-review"; -/** Identify raw GitHub writes whose body determines whether a grant is safe. */ +/** Identify GitHub writes whose body determines whether a grant is safe. */ function githubBodyInspection(input: { - operation?: string; provider: string; requestMethod: string; upstreamUrl: URL; }): GitHubBodyInspection | undefined { if ( - input.operation || input.provider !== "github" || input.requestMethod.toUpperCase() !== "POST" || input.upstreamUrl.hostname.toLowerCase() !== "api.github.com" @@ -352,7 +350,6 @@ function grantSelectionBodyTooLargeMessage( function grantSelectionBodyText(input: { body: ArrayBuffer | undefined; - operation?: string; provider: string; request: Request; upstreamUrl: URL; @@ -362,7 +359,6 @@ function grantSelectionBodyText(input: { } if (input.body.byteLength > GRANT_SELECTION_BODY_TEXT_LIMIT_BYTES) { const inspection = githubBodyInspection({ - ...(input.operation ? { operation: input.operation } : {}), provider: input.provider, requestMethod: input.request.method, upstreamUrl: input.upstreamUrl, @@ -605,7 +601,6 @@ export async function executeCredentialedEgressRequest(input: { upstreamUrl, } = input; const bodyForGrantSelection = githubBodyInspection({ - ...(operation ? { operation } : {}), provider, requestMethod: request.method, upstreamUrl, @@ -617,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/unit/plugins/plugin-egress.test.ts b/packages/junior/tests/unit/plugins/plugin-egress.test.ts index 6e8e4b918d..522811c4af 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.review.create", + 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(); + }); });