Skip to content

Commit ee810f4

Browse files
CopilotMalcolmnixon
andcommitted
Remove commit message parsing for security - use GitHub API exclusively
Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com>
1 parent 26a23dd commit ee810f4

File tree

2 files changed

+48
-56
lines changed

2 files changed

+48
-56
lines changed

src/DemaConsulting.BuildMark/RepoConnectors/GitHubRepoConnector.cs

Lines changed: 3 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -170,26 +170,9 @@ public override async Task<List<string>> GetPullRequestsBetweenTagsAsync(Version
170170
.Select(hash => hash.Trim())
171171
.ToList();
172172

173-
// First, try to find PR numbers in commit messages (fast path for squash merges)
174-
var commitsOutput = await RunCommandAsync("git", $"log --oneline {range}");
175-
var regex = NumberReferenceRegex();
176-
177-
var pullRequestsFromMessages = commitsOutput
178-
.Split('\n', StringSplitOptions.RemoveEmptyEntries)
179-
.Select(line => regex.Match(line))
180-
.Where(match => match.Success)
181-
.Select(match => match.Groups[1].Value)
182-
.Distinct()
183-
.ToList();
184-
185-
// If we found PRs in commit messages, return them
186-
if (pullRequestsFromMessages.Count > 0)
187-
{
188-
return pullRequestsFromMessages;
189-
}
190-
191-
// No PRs found in commit messages - search GitHub API for PRs containing these commits
192-
// This handles cases where commits don't have PR numbers in messages (e.g., during PR development)
173+
// Search GitHub API for PRs containing these commits
174+
// This approach is secure because the API only returns PRs that actually contain the commits,
175+
// unlike parsing commit messages which could reference unrelated PRs
193176
var pullRequestsFromApi = new HashSet<string>();
194177

195178
foreach (var commitHash in commitHashes)

test/DemaConsulting.BuildMark.Tests/GitHubRepoConnectorTests.cs

Lines changed: 45 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -139,11 +139,16 @@ public async Task GitHubRepoConnector_GetPullRequestsBetweenTagsAsync_ReturnsExp
139139
"log --format=%H v1.0.0..v2.0.0",
140140
"abc123def456\ndef456789abc");
141141

142-
// Mock git log for commit messages with PR numbers
142+
// Mock GitHub CLI to search for PRs by commit hash
143143
connector.AddCommandResult(
144-
"git",
145-
"log --oneline v1.0.0..v2.0.0",
146-
"abc123 Merge pull request #10 from feature/x\ndef456 Merge pull request #11 from bugfix/y");
144+
"gh",
145+
"pr list --search abc123def456 --json number --jq .[].number",
146+
"10");
147+
148+
connector.AddCommandResult(
149+
"gh",
150+
"pr list --search def456789abc --json number --jq .[].number",
151+
"11");
147152

148153
// Act
149154
var prs = await connector.GetPullRequestsBetweenTagsAsync(
@@ -174,11 +179,11 @@ public async Task GitHubRepoConnector_GetPullRequestsBetweenTagsAsync_HandlesNul
174179
"log --format=%H v1.0.0",
175180
"abc123def456");
176181

177-
// Mock git log for commit messages with PR numbers
182+
// Mock GitHub CLI to search for PRs by commit hash
178183
connector.AddCommandResult(
179-
"git",
180-
"log --oneline v1.0.0",
181-
"abc123 Merge pull request #10 from feature/x");
184+
"gh",
185+
"pr list --search abc123def456 --json number --jq .[].number",
186+
"10");
182187

183188
// Act
184189
var prs = await connector.GetPullRequestsBetweenTagsAsync(null, Version.Create("v1.0.0"));
@@ -203,11 +208,11 @@ public async Task GitHubRepoConnector_GetPullRequestsBetweenTagsAsync_HandlesNul
203208
"log --format=%H v1.0.0..HEAD",
204209
"abc123def456");
205210

206-
// Mock git log for commit messages with PR numbers
211+
// Mock GitHub CLI to search for PRs by commit hash
207212
connector.AddCommandResult(
208-
"git",
209-
"log --oneline v1.0.0..HEAD",
210-
"abc123 Merge pull request #11 from feature/y");
213+
"gh",
214+
"pr list --search abc123def456 --json number --jq .[].number",
215+
"11");
211216

212217
// Act
213218
var prs = await connector.GetPullRequestsBetweenTagsAsync(Version.Create("v1.0.0"), null);
@@ -232,11 +237,11 @@ public async Task GitHubRepoConnector_GetPullRequestsBetweenTagsAsync_HandlesBot
232237
"log --format=%H HEAD",
233238
"abc123def456");
234239

235-
// Mock git log for commit messages with PR numbers
240+
// Mock GitHub CLI to search for PRs by commit hash
236241
connector.AddCommandResult(
237-
"git",
238-
"log --oneline HEAD",
239-
"abc123 Merge pull request #12 from feature/z");
242+
"gh",
243+
"pr list --search abc123def456 --json number --jq .[].number",
244+
"12");
240245

241246
// Act
242247
var prs = await connector.GetPullRequestsBetweenTagsAsync(null, null);
@@ -264,11 +269,11 @@ public async Task GitHubRepoConnector_GetPullRequestsBetweenTagsAsync_UsesHeadWh
264269
"log --format=%H v1.0.0..HEAD",
265270
"abc123def456");
266271

267-
// Mock git log with HEAD instead of non-existent tag
272+
// Mock GitHub CLI to search for PRs by commit hash
268273
connector.AddCommandResult(
269-
"git",
270-
"log --oneline v1.0.0..HEAD",
271-
"abc123 Merge pull request #15 from feature/new");
274+
"gh",
275+
"pr list --search abc123def456 --json number --jq .[].number",
276+
"15");
272277

273278
// Act - using a version that doesn't exist as a tag
274279
var prs = await connector.GetPullRequestsBetweenTagsAsync(
@@ -281,10 +286,10 @@ public async Task GitHubRepoConnector_GetPullRequestsBetweenTagsAsync_UsesHeadWh
281286
}
282287

283288
/// <summary>
284-
/// Test that GetPullRequestsBetweenTagsAsync handles squash merge commits.
289+
/// Test that GetPullRequestsBetweenTagsAsync uses GitHub API to find PRs by commit hash.
285290
/// </summary>
286291
[TestMethod]
287-
public async Task GitHubRepoConnector_GetPullRequestsBetweenTagsAsync_HandlesSquashMerges()
292+
public async Task GitHubRepoConnector_GetPullRequestsBetweenTagsAsync_UsesGitHubApiToFindPRs()
288293
{
289294
// Arrange
290295
var connector = new TestableGitHubRepoConnector();
@@ -295,11 +300,21 @@ public async Task GitHubRepoConnector_GetPullRequestsBetweenTagsAsync_HandlesSqu
295300
"log --format=%H HEAD",
296301
"abc123def456\ndef456789abc\n789abcdef123");
297302

298-
// Mock git log with squash merge commits (PR number in parentheses)
303+
// Mock GitHub CLI to search for PRs by commit hash
299304
connector.AddCommandResult(
300-
"git",
301-
"log --oneline HEAD",
302-
"abc123 Fix bug with validation (#18)\ndef456 Add new feature (#19)\n789abc Update documentation (#20)");
305+
"gh",
306+
"pr list --search abc123def456 --json number --jq .[].number",
307+
"18");
308+
309+
connector.AddCommandResult(
310+
"gh",
311+
"pr list --search def456789abc --json number --jq .[].number",
312+
"19");
313+
314+
connector.AddCommandResult(
315+
"gh",
316+
"pr list --search 789abcdef123 --json number --jq .[].number",
317+
"20");
303318

304319
// Act
305320
var prs = await connector.GetPullRequestsBetweenTagsAsync(null, null);
@@ -312,27 +327,21 @@ public async Task GitHubRepoConnector_GetPullRequestsBetweenTagsAsync_HandlesSqu
312327
}
313328

314329
/// <summary>
315-
/// Test that GetPullRequestsBetweenTagsAsync uses GitHub API when no PR numbers in messages.
330+
/// Test that GetPullRequestsBetweenTagsAsync deduplicates PRs when multiple commits belong to same PR.
316331
/// </summary>
317332
[TestMethod]
318-
public async Task GitHubRepoConnector_GetPullRequestsBetweenTagsAsync_UsesGitHubApiWhenNoPrNumbersInMessages()
333+
public async Task GitHubRepoConnector_GetPullRequestsBetweenTagsAsync_DeduplicatesPRs()
319334
{
320335
// Arrange
321336
var connector = new TestableGitHubRepoConnector();
322337

323-
// Mock git log for commit hashes (commits without PR numbers in messages)
338+
// Mock git log for commit hashes (multiple commits from same PR)
324339
connector.AddCommandResult(
325340
"git",
326341
"log --format=%H HEAD",
327342
"5e541195f387259ee8d72d33b70579a0f7b6fde4\nc3eb81cd24b9d054a626a9785b16975f0808ecb2");
328343

329-
// Mock git log for commit messages (no PR numbers)
330-
connector.AddCommandResult(
331-
"git",
332-
"log --oneline HEAD",
333-
"5e54119 Fix spelling error in test mock commit hash\nc3eb81c Support squash merges by parsing all commits for PR references");
334-
335-
// Mock GitHub CLI to search for PRs by commit hash
344+
// Mock GitHub CLI to search for PRs by commit hash - both commits are from PR 20
336345
connector.AddCommandResult(
337346
"gh",
338347
"pr list --search 5e541195f387259ee8d72d33b70579a0f7b6fde4 --json number --jq .[].number",

0 commit comments

Comments
 (0)