diff --git a/src/github/data/fetcher.ts b/src/github/data/fetcher.ts index fb344ac3a..8d0e6ebbe 100644 --- a/src/github/data/fetcher.ts +++ b/src/github/data/fetcher.ts @@ -71,6 +71,33 @@ export function extractOriginalTitle( return undefined; } +/** + * Extracts the original body from the GitHub webhook payload. + * This is the body as it existed when the trigger event occurred, + * preventing TOCTOU attacks where an attacker edits the body after + * the trigger but before the action reads it. + * + * @param context - Parsed GitHub context from webhook + * @returns The original body string, null (no body), or undefined (not available) + */ +export function extractOriginalBody( + context: ParsedGitHubContext, +): string | null | undefined { + if (isIssueCommentEvent(context)) { + return context.payload.issue?.body; + } else if (isPullRequestEvent(context)) { + return context.payload.pull_request?.body; + } else if (isPullRequestReviewEvent(context)) { + return context.payload.pull_request?.body; + } else if (isPullRequestReviewCommentEvent(context)) { + return context.payload.pull_request?.body; + } else if (isIssuesEvent(context)) { + return context.payload.issue?.body; + } + + return undefined; +} + /** * Filters comments to only include those that existed in their final state before the trigger time. * This prevents malicious actors from editing comments after the trigger to inject harmful content. @@ -207,6 +234,7 @@ type FetchDataParams = { triggerUsername?: string; triggerTime?: string; originalTitle?: string; + originalBody?: string | null; includeCommentsByActor?: string; excludeCommentsByActor?: string; }; @@ -233,6 +261,7 @@ export async function fetchGitHubData({ triggerUsername, triggerTime, originalTitle, + originalBody, includeCommentsByActor, excludeCommentsByActor, }: FetchDataParams): Promise { @@ -399,12 +428,21 @@ export async function fetchGitHubData({ body: c.body, })); - // Add the main issue/PR body if it has content and wasn't edited after trigger - // This prevents a TOCTOU race condition where an attacker could edit the body - // between when an authorized user triggered Claude and when Claude processes the request + // Use the original body from the webhook payload if provided (TOCTOU protection). + // The webhook payload captures the body at event time, before any attacker edits. + if (originalBody !== undefined) { + contextData.body = originalBody ?? ""; + } + + // Add the main issue/PR body if it has content and wasn't edited after trigger. + // When originalBody is provided, the body is already safe (from webhook payload). + // Otherwise, fall back to timestamp-based validation. let mainBody: CommentWithImages[] = []; if (contextData.body) { - if (isBodySafeToUse(contextData, triggerTime)) { + if ( + originalBody !== undefined || + isBodySafeToUse(contextData, triggerTime) + ) { mainBody = [ { ...(isPR diff --git a/src/modes/tag/index.ts b/src/modes/tag/index.ts index 1135c517c..89f688810 100644 --- a/src/modes/tag/index.ts +++ b/src/modes/tag/index.ts @@ -12,6 +12,7 @@ import { fetchGitHubData, extractTriggerTimestamp, extractOriginalTitle, + extractOriginalBody, } from "../../github/data/fetcher"; import { createPrompt, generateDefaultPrompt } from "../../create-prompt"; import { isEntityContext } from "../../github/context"; @@ -79,6 +80,7 @@ export const tagMode: Mode = { const triggerTime = extractTriggerTimestamp(context); const originalTitle = extractOriginalTitle(context); + const originalBody = extractOriginalBody(context); const githubData = await fetchGitHubData({ octokits: octokit, @@ -88,6 +90,7 @@ export const tagMode: Mode = { triggerUsername: context.actor, triggerTime, originalTitle, + originalBody, includeCommentsByActor: context.inputs.includeCommentsByActor, excludeCommentsByActor: context.inputs.excludeCommentsByActor, }); diff --git a/test/data-fetcher.test.ts b/test/data-fetcher.test.ts index 90359628b..98f52e572 100644 --- a/test/data-fetcher.test.ts +++ b/test/data-fetcher.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it, jest, test } from "bun:test"; import { extractTriggerTimestamp, extractOriginalTitle, + extractOriginalBody, fetchGitHubData, filterCommentsToTriggerTime, filterReviewsToTriggerTime, @@ -106,6 +107,55 @@ describe("extractOriginalTitle", () => { }); }); +describe("extractOriginalBody", () => { + it("should extract body from IssueCommentEvent on PR", () => { + const body = extractOriginalBody(mockPullRequestCommentContext); + expect(body).toBe("This PR fixes the memory leak issue reported in #788"); + }); + + it("should extract body from PullRequestReviewEvent", () => { + const body = extractOriginalBody(mockPullRequestReviewContext); + expect(body).toBe( + "This PR improves error handling across all API endpoints", + ); + }); + + it("should extract body from PullRequestReviewCommentEvent", () => { + const body = extractOriginalBody(mockPullRequestReviewCommentContext); + expect(body).toBe( + "This PR optimizes the search algorithm for better performance", + ); + }); + + it("should extract body from pull_request event", () => { + const body = extractOriginalBody(mockPullRequestOpenedContext); + expect(body).toBe( + "## Summary\n\nThis PR adds JWT-based authentication to the API.\n\n## Changes\n\n- Added auth middleware\n- Added login endpoint\n- Added JWT token generation\n\n/claude please review the security aspects", + ); + }); + + it("should extract body from issues event", () => { + const body = extractOriginalBody(mockIssueOpenedContext); + expect(body).toBe( + "## Description\n\nThe application crashes immediately after launching.\n\n## Steps to reproduce\n\n1. Install the app\n2. Launch it\n3. See crash\n\n/claude please help me fix this", + ); + }); + + it("should return undefined for event without body", () => { + const context = createMockContext({ + eventName: "issue_comment", + payload: { + comment: { + id: 123, + body: "test", + }, + } as any, + }); + const body = extractOriginalBody(context); + expect(body).toBeUndefined(); + }); +}); + describe("filterCommentsToTriggerTime", () => { const createMockComment = ( createdAt: string, @@ -1099,6 +1149,186 @@ describe("fetchGitHubData integration with time filtering", () => { "Original Title (from webhook at trigger time)", ); }); + + it("should use originalBody when provided instead of fetched body", async () => { + const mockOctokits = { + graphql: jest.fn().mockResolvedValue({ + repository: { + pullRequest: { + number: 123, + title: "Test PR", + body: "Malicious body injected after trigger", + author: { login: "author" }, + createdAt: "2024-01-15T10:00:00Z", + additions: 10, + deletions: 5, + state: "OPEN", + commits: { totalCount: 1, nodes: [] }, + files: { nodes: [] }, + comments: { nodes: [] }, + reviews: { nodes: [] }, + }, + }, + user: { login: "trigger-user" }, + }), + rest: jest.fn() as any, + }; + + const result = await fetchGitHubData({ + octokits: mockOctokits as any, + repository: "test-owner/test-repo", + prNumber: "123", + isPR: true, + triggerUsername: "trigger-user", + originalBody: "Original safe body from webhook", + }); + + expect(result.contextData.body).toBe("Original safe body from webhook"); + }); + + it("should use fetched body when originalBody is not provided", async () => { + const mockOctokits = { + graphql: jest.fn().mockResolvedValue({ + repository: { + pullRequest: { + number: 123, + title: "Test PR", + body: "Fetched body from GraphQL", + author: { login: "author" }, + createdAt: "2024-01-15T10:00:00Z", + additions: 10, + deletions: 5, + state: "OPEN", + commits: { totalCount: 1, nodes: [] }, + files: { nodes: [] }, + comments: { nodes: [] }, + reviews: { nodes: [] }, + }, + }, + user: { login: "trigger-user" }, + }), + rest: jest.fn() as any, + }; + + const result = await fetchGitHubData({ + octokits: mockOctokits as any, + repository: "test-owner/test-repo", + prNumber: "123", + isPR: true, + triggerUsername: "trigger-user", + }); + + expect(result.contextData.body).toBe("Fetched body from GraphQL"); + }); + + it("should use original body from webhook even if body was edited after trigger (TOCTOU prevention)", async () => { + const mockOctokits = { + graphql: jest.fn().mockResolvedValue({ + repository: { + pullRequest: { + number: 123, + title: "Test PR", + body: "Malicious body (edited after trigger via GraphQL)", + author: { login: "author" }, + createdAt: "2024-01-15T10:00:00Z", + lastEditedAt: "2024-01-15T12:30:00Z", // Edited after trigger + additions: 10, + deletions: 5, + state: "OPEN", + commits: { totalCount: 1, nodes: [] }, + files: { nodes: [] }, + comments: { nodes: [] }, + reviews: { nodes: [] }, + }, + }, + user: { login: "trigger-user" }, + }), + rest: jest.fn() as any, + }; + + const result = await fetchGitHubData({ + octokits: mockOctokits as any, + repository: "test-owner/test-repo", + prNumber: "123", + isPR: true, + triggerUsername: "trigger-user", + triggerTime: "2024-01-15T12:00:00Z", + originalBody: "Original safe body (from webhook at trigger time)", + }); + + // Body should be from webhook, not the malicious GraphQL-fetched version + expect(result.contextData.body).toBe( + "Original safe body (from webhook at trigger time)", + ); + }); + + it("should handle null originalBody by setting body to empty string", async () => { + const mockOctokits = { + graphql: jest.fn().mockResolvedValue({ + repository: { + issue: { + number: 123, + title: "Test Issue", + body: "Some body from GraphQL", + author: { login: "author" }, + createdAt: "2024-01-15T10:00:00Z", + state: "OPEN", + labels: { nodes: [] }, + comments: { nodes: [] }, + }, + }, + user: { login: "trigger-user" }, + }), + rest: jest.fn() as any, + }; + + const result = await fetchGitHubData({ + octokits: mockOctokits as any, + repository: "test-owner/test-repo", + prNumber: "123", + isPR: false, + triggerUsername: "trigger-user", + originalBody: null, + }); + + // null originalBody means the issue had no body at trigger time + expect(result.contextData.body).toBe(""); + }); + + it("should use null originalBody over malicious GraphQL body edited after trigger", async () => { + const mockOctokits = { + graphql: jest.fn().mockResolvedValue({ + repository: { + issue: { + number: 123, + title: "Test Issue", + body: "Malicious body added after trigger", + author: { login: "author" }, + createdAt: "2024-01-15T10:00:00Z", + lastEditedAt: "2024-01-15T12:30:00Z", // Edited after trigger + state: "OPEN", + labels: { nodes: [] }, + comments: { nodes: [] }, + }, + }, + user: { login: "trigger-user" }, + }), + rest: jest.fn() as any, + }; + + const result = await fetchGitHubData({ + octokits: mockOctokits as any, + repository: "test-owner/test-repo", + prNumber: "123", + isPR: false, + triggerUsername: "trigger-user", + triggerTime: "2024-01-15T12:00:00Z", + originalBody: null, + }); + + // Webhook says no body at trigger time — attacker-added GraphQL body must not be used + expect(result.contextData.body).toBe(""); + }); }); describe("filterCommentsByActor", () => {