-
Notifications
You must be signed in to change notification settings - Fork 1.4k
fix: use original title from webhook payload instead of fetched title #793
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,8 @@ import type { Octokits } from "../api/client"; | |
| import { ISSUE_QUERY, PR_QUERY, USER_QUERY } from "../api/queries/github"; | ||
| import { | ||
| isIssueCommentEvent, | ||
| isIssuesEvent, | ||
| isPullRequestEvent, | ||
| isPullRequestReviewEvent, | ||
| isPullRequestReviewCommentEvent, | ||
| type ParsedGitHubContext, | ||
|
|
@@ -40,6 +42,31 @@ export function extractTriggerTimestamp( | |
| return undefined; | ||
| } | ||
|
|
||
| /** | ||
| * Extracts the original title from the GitHub webhook payload. | ||
| * This is the title as it existed when the trigger event occurred. | ||
| * | ||
| * @param context - Parsed GitHub context from webhook | ||
| * @returns The original title string or undefined if not available | ||
| */ | ||
| export function extractOriginalTitle( | ||
| context: ParsedGitHubContext, | ||
| ): string | undefined { | ||
| if (isIssueCommentEvent(context)) { | ||
| return context.payload.issue?.title; | ||
| } else if (isPullRequestEvent(context)) { | ||
| return context.payload.pull_request?.title; | ||
| } else if (isPullRequestReviewEvent(context)) { | ||
| return context.payload.pull_request?.title; | ||
| } else if (isPullRequestReviewCommentEvent(context)) { | ||
| return context.payload.pull_request?.title; | ||
| } else if (isIssuesEvent(context)) { | ||
| return context.payload.issue?.title; | ||
| } | ||
|
|
||
| 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. | ||
|
|
@@ -146,6 +173,7 @@ type FetchDataParams = { | |
| isPR: boolean; | ||
| triggerUsername?: string; | ||
| triggerTime?: string; | ||
| originalTitle?: string; | ||
| }; | ||
|
|
||
| export type GitHubFileWithSHA = GitHubFile & { | ||
|
|
@@ -169,6 +197,7 @@ export async function fetchGitHubData({ | |
| isPR, | ||
| triggerUsername, | ||
| triggerTime, | ||
| originalTitle, | ||
| }: FetchDataParams): Promise<FetchDataResult> { | ||
| const [owner, repo] = repository.split("/"); | ||
| if (!owner || !repo) { | ||
|
|
@@ -354,6 +383,11 @@ export async function fetchGitHubData({ | |
| triggerDisplayName = await fetchUserDisplayName(octokits, triggerUsername); | ||
| } | ||
|
|
||
| // Use the original title from the webhook payload if provided | ||
| if (originalTitle !== undefined) { | ||
| contextData.title = originalTitle; | ||
| } | ||
|
Comment on lines
+386
to
+389
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Security: Missing sanitization for title field The title is assigned without sanitization, but other user-controlled content (bodies, comments) goes through
Recommendation: Apply Reference: CWE-74 (Improper Neutralization of Special Elements)
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems worth fixing? |
||
|
|
||
| return { | ||
| contextData, | ||
| comments, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| import { describe, expect, it, jest } from "bun:test"; | ||
| import { | ||
| extractTriggerTimestamp, | ||
| extractOriginalTitle, | ||
| fetchGitHubData, | ||
| filterCommentsToTriggerTime, | ||
| filterReviewsToTriggerTime, | ||
|
|
@@ -9,6 +10,7 @@ import { | |
| import { | ||
| createMockContext, | ||
| mockIssueCommentContext, | ||
| mockPullRequestCommentContext, | ||
| mockPullRequestReviewContext, | ||
| mockPullRequestReviewCommentContext, | ||
| mockPullRequestOpenedContext, | ||
|
|
@@ -63,6 +65,47 @@ describe("extractTriggerTimestamp", () => { | |
| }); | ||
| }); | ||
|
|
||
| describe("extractOriginalTitle", () => { | ||
| it("should extract title from IssueCommentEvent on PR", () => { | ||
| const title = extractOriginalTitle(mockPullRequestCommentContext); | ||
| expect(title).toBe("Fix: Memory leak in user service"); | ||
| }); | ||
|
|
||
| it("should extract title from PullRequestReviewEvent", () => { | ||
| const title = extractOriginalTitle(mockPullRequestReviewContext); | ||
| expect(title).toBe("Refactor: Improve error handling in API layer"); | ||
| }); | ||
|
|
||
| it("should extract title from PullRequestReviewCommentEvent", () => { | ||
| const title = extractOriginalTitle(mockPullRequestReviewCommentContext); | ||
| expect(title).toBe("Performance: Optimize search algorithm"); | ||
| }); | ||
|
|
||
| it("should extract title from pull_request event", () => { | ||
| const title = extractOriginalTitle(mockPullRequestOpenedContext); | ||
| expect(title).toBe("Feature: Add user authentication"); | ||
| }); | ||
|
|
||
| it("should extract title from issues event", () => { | ||
| const title = extractOriginalTitle(mockIssueOpenedContext); | ||
| expect(title).toBe("Bug: Application crashes on startup"); | ||
| }); | ||
|
|
||
| it("should return undefined for event without title", () => { | ||
| const context = createMockContext({ | ||
| eventName: "issue_comment", | ||
| payload: { | ||
| comment: { | ||
| id: 123, | ||
| body: "test", | ||
| }, | ||
| } as any, | ||
| }); | ||
| const title = extractOriginalTitle(context); | ||
| expect(title).toBeUndefined(); | ||
| }); | ||
| }); | ||
|
|
||
| describe("filterCommentsToTriggerTime", () => { | ||
| const createMockComment = ( | ||
| createdAt: string, | ||
|
|
@@ -945,4 +988,115 @@ describe("fetchGitHubData integration with time filtering", () => { | |
| ); | ||
| expect(hasPrBodyInMap).toBe(false); | ||
| }); | ||
|
|
||
| it("should use originalTitle when provided instead of fetched title", async () => { | ||
| const mockOctokits = { | ||
| graphql: jest.fn().mockResolvedValue({ | ||
| repository: { | ||
| pullRequest: { | ||
| number: 123, | ||
| title: "Fetched Title From GraphQL", | ||
| body: "PR body", | ||
| 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", | ||
| originalTitle: "Original Title From Webhook", | ||
| }); | ||
|
|
||
| expect(result.contextData.title).toBe("Original Title From Webhook"); | ||
| }); | ||
|
|
||
| it("should use fetched title when originalTitle is not provided", async () => { | ||
| const mockOctokits = { | ||
| graphql: jest.fn().mockResolvedValue({ | ||
| repository: { | ||
| pullRequest: { | ||
| number: 123, | ||
| title: "Fetched Title From GraphQL", | ||
| body: "PR body", | ||
| 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.title).toBe("Fetched Title From GraphQL"); | ||
| }); | ||
|
Comment on lines
+1026
to
+1061
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test Coverage: Missing security validation test Consider adding a test that explicitly validates the TOCTOU prevention use case: it("should use original title from webhook even if title was edited after trigger", async () => {
// Scenario:
// 1. PR created with title "Original Title"
// 2. @claude triggered at timestamp T
// 3. Attacker edits title to "Malicious Title" after T
// 4. GraphQL would fetch "Malicious Title" but originalTitle should override it
const mockOctokits = {
graphql: jest.fn().mockResolvedValue({
repository: {
pullRequest: {
title: "Malicious Title (edited after trigger)", // GraphQL fetched
// ... other fields
},
},
}),
};
const result = await fetchGitHubData({
octokits: mockOctokits as any,
repository: "test-owner/test-repo",
prNumber: "123",
isPR: true,
originalTitle: "Original Title (from webhook at trigger time)",
});
expect(result.contextData.title).toBe("Original Title (from webhook at trigger time)");
});This would explicitly document and validate the security benefit this PR provides. |
||
|
|
||
| it("should use original title from webhook even if title was edited after trigger", async () => { | ||
| const mockOctokits = { | ||
| graphql: jest.fn().mockResolvedValue({ | ||
| repository: { | ||
| pullRequest: { | ||
| number: 123, | ||
| title: "Edited Title (from GraphQL)", | ||
| body: "PR body", | ||
| 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", | ||
| originalTitle: "Original Title (from webhook at trigger time)", | ||
| }); | ||
|
|
||
| expect(result.contextData.title).toBe( | ||
| "Original Title (from webhook at trigger time)", | ||
| ); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Quality: Inconsistent optional chaining pattern
This function uses optional chaining (
context.payload.issue?.title) while the similarextractTriggerTimestamp()function (lines 31-43) doesn't. This creates ambiguity about whether these fields are guaranteed to exist after the type guard checks.Consider standardizing the approach:
extractTriggerTimestamp()as well