diff --git a/src/github/api/queries/github.ts b/src/github/api/queries/github.ts index 2341a55..7bc494d 100644 --- a/src/github/api/queries/github.ts +++ b/src/github/api/queries/github.ts @@ -13,6 +13,8 @@ export const PR_QUERY = ` headRefName headRefOid createdAt + updatedAt + lastEditedAt additions deletions state @@ -96,6 +98,8 @@ export const ISSUE_QUERY = ` login } createdAt + updatedAt + lastEditedAt state comments(first: 100) { nodes { diff --git a/src/github/data/fetcher.ts b/src/github/data/fetcher.ts index e6cec2c..c756e00 100644 --- a/src/github/data/fetcher.ts +++ b/src/github/data/fetcher.ts @@ -107,6 +107,38 @@ export function filterReviewsToTriggerTime< }); } +/** + * Checks if the issue/PR body was edited after the trigger time. + * This prevents a race condition where an attacker could edit the issue/PR body + * between when an authorized user triggered Claude and when Claude processes the request. + * + * @param contextData - The PR or issue data containing body and edit timestamps + * @param triggerTime - ISO timestamp of when the trigger event occurred + * @returns true if the body is safe to use, false if it was edited after trigger + */ +export function isBodySafeToUse( + contextData: { createdAt: string; updatedAt?: string; lastEditedAt?: string }, + triggerTime: string | undefined, +): boolean { + // If no trigger time is available, we can't validate - allow the body + // This maintains backwards compatibility for triggers that don't have timestamps + if (!triggerTime) return true; + + const triggerTimestamp = new Date(triggerTime).getTime(); + + // Check if the body was edited after the trigger + // Use lastEditedAt if available (more accurate for body edits), otherwise fall back to updatedAt + const lastEditTime = contextData.lastEditedAt || contextData.updatedAt; + if (lastEditTime) { + const lastEditTimestamp = new Date(lastEditTime).getTime(); + if (lastEditTimestamp >= triggerTimestamp) { + return false; + } + } + + return true; +} + type FetchDataParams = { octokits: Octokits; repository: string; @@ -273,9 +305,13 @@ export async function fetchGitHubData({ body: c.body, })); - // Add the main issue/PR body if it has content - const mainBody: CommentWithImages[] = contextData.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 + let mainBody: CommentWithImages[] = []; + if (contextData.body) { + if (isBodySafeToUse(contextData, triggerTime)) { + mainBody = [ { ...(isPR ? { @@ -289,8 +325,14 @@ export async function fetchGitHubData({ body: contextData.body, }), }, - ] - : []; + ]; + } else { + console.warn( + `Security: ${isPR ? "PR" : "Issue"} #${prNumber} body was edited after the trigger event. ` + + `Excluding body content to prevent potential injection attacks.`, + ); + } + } const allComments = [ ...mainBody, diff --git a/src/github/types.ts b/src/github/types.ts index 41e0896..4ab066f 100644 --- a/src/github/types.ts +++ b/src/github/types.ts @@ -58,6 +58,8 @@ export type GitHubPullRequest = { headRefName: string; headRefOid: string; createdAt: string; + updatedAt?: string; + lastEditedAt?: string; additions: number; deletions: number; state: string; @@ -83,6 +85,8 @@ export type GitHubIssue = { body: string; author: GitHubAuthor; createdAt: string; + updatedAt?: string; + lastEditedAt?: string; state: string; comments: { nodes: GitHubComment[]; diff --git a/test/data-fetcher.test.ts b/test/data-fetcher.test.ts index 28e3135..216a5f7 100644 --- a/test/data-fetcher.test.ts +++ b/test/data-fetcher.test.ts @@ -4,6 +4,7 @@ import { fetchGitHubData, filterCommentsToTriggerTime, filterReviewsToTriggerTime, + isBodySafeToUse, } from "../src/github/data/fetcher"; import { createMockContext, @@ -371,6 +372,139 @@ describe("filterReviewsToTriggerTime", () => { }); }); +describe("isBodySafeToUse", () => { + const triggerTime = "2024-01-15T12:00:00Z"; + + const createMockContextData = ( + createdAt: string, + updatedAt?: string, + lastEditedAt?: string, + ) => ({ + createdAt, + updatedAt, + lastEditedAt, + }); + + describe("body edit time validation", () => { + it("should return true when body was never edited", () => { + const contextData = createMockContextData("2024-01-15T10:00:00Z"); + expect(isBodySafeToUse(contextData, triggerTime)).toBe(true); + }); + + it("should return true when body was edited before trigger time", () => { + const contextData = createMockContextData( + "2024-01-15T10:00:00Z", + "2024-01-15T11:00:00Z", + "2024-01-15T11:30:00Z", + ); + expect(isBodySafeToUse(contextData, triggerTime)).toBe(true); + }); + + it("should return false when body was edited after trigger time (using updatedAt)", () => { + const contextData = createMockContextData( + "2024-01-15T10:00:00Z", + "2024-01-15T13:00:00Z", + ); + expect(isBodySafeToUse(contextData, triggerTime)).toBe(false); + }); + + it("should return false when body was edited after trigger time (using lastEditedAt)", () => { + const contextData = createMockContextData( + "2024-01-15T10:00:00Z", + undefined, + "2024-01-15T13:00:00Z", + ); + expect(isBodySafeToUse(contextData, triggerTime)).toBe(false); + }); + + it("should return false when body was edited exactly at trigger time", () => { + const contextData = createMockContextData( + "2024-01-15T10:00:00Z", + "2024-01-15T12:00:00Z", + ); + expect(isBodySafeToUse(contextData, triggerTime)).toBe(false); + }); + + it("should prioritize lastEditedAt over updatedAt", () => { + // updatedAt is after trigger, but lastEditedAt is before - should be safe + const contextData = createMockContextData( + "2024-01-15T10:00:00Z", + "2024-01-15T13:00:00Z", // updatedAt after trigger + "2024-01-15T11:00:00Z", // lastEditedAt before trigger + ); + expect(isBodySafeToUse(contextData, triggerTime)).toBe(true); + }); + }); + + describe("edge cases", () => { + it("should return true when no trigger time is provided (backward compatibility)", () => { + const contextData = createMockContextData( + "2024-01-15T10:00:00Z", + "2024-01-15T13:00:00Z", // Would normally fail + "2024-01-15T14:00:00Z", // Would normally fail + ); + expect(isBodySafeToUse(contextData, undefined)).toBe(true); + }); + + it("should handle millisecond precision correctly", () => { + // Edit 1ms after trigger - should be unsafe + const contextData = createMockContextData( + "2024-01-15T10:00:00Z", + "2024-01-15T12:00:00.001Z", + ); + expect(isBodySafeToUse(contextData, triggerTime)).toBe(false); + }); + + it("should handle edit 1ms before trigger - should be safe", () => { + const contextData = createMockContextData( + "2024-01-15T10:00:00Z", + "2024-01-15T11:59:59.999Z", + ); + expect(isBodySafeToUse(contextData, triggerTime)).toBe(true); + }); + + it("should handle various ISO timestamp formats", () => { + const contextData1 = createMockContextData( + "2024-01-15T10:00:00Z", + "2024-01-15T11:00:00Z", + ); + const contextData2 = createMockContextData( + "2024-01-15T10:00:00+00:00", + "2024-01-15T11:00:00+00:00", + ); + const contextData3 = createMockContextData( + "2024-01-15T10:00:00.000Z", + "2024-01-15T11:00:00.000Z", + ); + + expect(isBodySafeToUse(contextData1, triggerTime)).toBe(true); + expect(isBodySafeToUse(contextData2, triggerTime)).toBe(true); + expect(isBodySafeToUse(contextData3, triggerTime)).toBe(true); + }); + }); + + describe("security scenarios", () => { + it("should detect race condition attack - body edited between trigger and processing", () => { + // Simulates: Owner triggers @claude at 12:00, attacker edits body at 12:00:30 + const contextData = createMockContextData( + "2024-01-15T10:00:00Z", // Issue created + "2024-01-15T12:00:30Z", // Body edited after trigger + ); + expect(isBodySafeToUse(contextData, "2024-01-15T12:00:00Z")).toBe(false); + }); + + it("should allow body that was stable at trigger time", () => { + // Body was last edited well before the trigger + const contextData = createMockContextData( + "2024-01-15T10:00:00Z", + "2024-01-15T10:30:00Z", + "2024-01-15T10:30:00Z", + ); + expect(isBodySafeToUse(contextData, "2024-01-15T12:00:00Z")).toBe(true); + }); + }); +}); + describe("fetchGitHubData integration with time filtering", () => { it("should filter comments based on trigger time when provided", async () => { const mockOctokits = { @@ -696,4 +830,119 @@ describe("fetchGitHubData integration with time filtering", () => { // All three comments should be included as they're all before trigger time expect(result.comments.length).toBe(3); }); + + it("should exclude issue body when edited after trigger time (TOCTOU protection)", async () => { + const mockOctokits = { + graphql: jest.fn().mockResolvedValue({ + repository: { + issue: { + number: 555, + title: "Test Issue", + body: "Malicious body edited after trigger", + author: { login: "attacker" }, + createdAt: "2024-01-15T10:00:00Z", + updatedAt: "2024-01-15T12:30:00Z", // Edited after trigger + lastEditedAt: "2024-01-15T12:30:00Z", // Edited after trigger + 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: "555", + isPR: false, + triggerUsername: "trigger-user", + triggerTime: "2024-01-15T12:00:00Z", + }); + + // The body should be excluded from image processing due to TOCTOU protection + // We can verify this by checking that issue_body is NOT in the imageUrlMap keys + const hasIssueBodyInMap = Array.from(result.imageUrlMap.keys()).some( + (key) => key.includes("issue_body"), + ); + expect(hasIssueBodyInMap).toBe(false); + }); + + it("should include issue body when not edited after trigger time", async () => { + const mockOctokits = { + graphql: jest.fn().mockResolvedValue({ + repository: { + issue: { + number: 666, + title: "Test Issue", + body: "Safe body not edited after trigger", + author: { login: "author" }, + createdAt: "2024-01-15T10:00:00Z", + updatedAt: "2024-01-15T11:00:00Z", // Edited before trigger + lastEditedAt: "2024-01-15T11:00:00Z", // Edited before trigger + 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: "666", + isPR: false, + triggerUsername: "trigger-user", + triggerTime: "2024-01-15T12:00:00Z", + }); + + // The contextData should still contain the body + expect(result.contextData.body).toBe("Safe body not edited after trigger"); + }); + + it("should exclude PR body when edited after trigger time (TOCTOU protection)", async () => { + const mockOctokits = { + graphql: jest.fn().mockResolvedValue({ + repository: { + pullRequest: { + number: 777, + title: "Test PR", + body: "Malicious PR body edited after trigger", + author: { login: "attacker" }, + baseRefName: "main", + headRefName: "feature", + headRefOid: "abc123", + createdAt: "2024-01-15T10:00:00Z", + updatedAt: "2024-01-15T12:30:00Z", // Edited after trigger + 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: "777", + isPR: true, + triggerUsername: "trigger-user", + triggerTime: "2024-01-15T12:00:00Z", + }); + + // The body should be excluded from image processing due to TOCTOU protection + const hasPrBodyInMap = Array.from(result.imageUrlMap.keys()).some((key) => + key.includes("pr_body"), + ); + expect(hasPrBodyInMap).toBe(false); + }); });