fix: use original body from webhook payload for TOCTOU hardening (#904)
* fix: use original body from webhook payload for TOCTOU hardening * test: add null originalBody + edited GraphQL body TOCTOU scenario
This commit is contained in:
David Dworken
GitHub
parent
006aaf2935
commit
f09dc9a6a3
@ -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<FetchDataResult> {
|
||||
@ -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
|
||||
|
||||
@ -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,
|
||||
});
|
||||
|
||||
@ -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", () => {
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user