Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
46 changes: 42 additions & 4 deletions src/github/data/fetcher.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -207,6 +234,7 @@ type FetchDataParams = {
triggerUsername?: string;
triggerTime?: string;
originalTitle?: string;
originalBody?: string | null;
includeCommentsByActor?: string;
excludeCommentsByActor?: string;
};
Expand All @@ -233,6 +261,7 @@ export async function fetchGitHubData({
triggerUsername,
triggerTime,
originalTitle,
originalBody,
includeCommentsByActor,
excludeCommentsByActor,
}: FetchDataParams): Promise<FetchDataResult> {
Expand Down Expand Up @@ -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
Expand Down
3 changes: 3 additions & 0 deletions src/modes/tag/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
fetchGitHubData,
extractTriggerTimestamp,
extractOriginalTitle,
extractOriginalBody,
} from "../../github/data/fetcher";
import { createPrompt, generateDefaultPrompt } from "../../create-prompt";
import { isEntityContext } from "../../github/context";
Expand Down Expand Up @@ -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,
Expand All @@ -88,6 +90,7 @@ export const tagMode: Mode = {
triggerUsername: context.actor,
triggerTime,
originalTitle,
originalBody,
includeCommentsByActor: context.inputs.includeCommentsByActor,
excludeCommentsByActor: context.inputs.excludeCommentsByActor,
});
Expand Down
195 changes: 195 additions & 0 deletions test/data-fetcher.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { describe, expect, it, jest, test } from "bun:test";
import {
extractTriggerTimestamp,
extractOriginalTitle,
extractOriginalBody,
fetchGitHubData,
filterCommentsToTriggerTime,
filterReviewsToTriggerTime,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -1099,6 +1149,151 @@ 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("");
});
Comment on lines +1265 to +1296
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test Coverage - Missing security-critical scenario

This test validates that null originalBody sets body to empty string, but doesn't test the security-critical scenario where:

  • Webhook has originalBody: null (no body at trigger time)
  • GraphQL data shows a body with lastEditedAt timestamp after the trigger
  • We should use the empty string from webhook, not the potentially malicious GraphQL body

Consider adding triggerTime and lastEditedAt to this test to verify the webhook's null takes precedence even when timestamp validation would reject the GraphQL body.

});

describe("filterCommentsByActor", () => {
Expand Down
Loading