Skip to content

Commit

Permalink
Use merge commit ref for reviews
Browse files Browse the repository at this point in the history
This makes it less likely that a malicious change would
be buried in an earlier commit, and missed when only
reviewing the tip of HEAD.

CHange-type: patch
Signed-off-by: Kyle Harding <kyle@balena.io>
  • Loading branch information
klutchell committed Oct 15, 2024
1 parent 9b26d19 commit c16bf03
Show file tree
Hide file tree
Showing 6 changed files with 53 additions and 7 deletions.
4 changes: 4 additions & 0 deletions __tests__/approval.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ describe('ApprovalProcess', () => {

mockGitHubClient = {
getPullRequestHeadSha: jest.fn(),
getPullRequestMergeRef: jest.fn(),
getRefSha: jest.fn(),
getAuthenticatedUser: jest.fn(),
getPullRequestRepository: jest.fn(),
findCommitComment: jest.fn(),
Expand Down Expand Up @@ -51,6 +53,8 @@ describe('ApprovalProcess', () => {
describe('run', () => {
beforeEach(() => {
mockGitHubClient.getPullRequestHeadSha.mockReturnValue('test-sha')
mockGitHubClient.getPullRequestMergeRef.mockReturnValue('pull/1/merge')
mockGitHubClient.getRefSha.mockResolvedValue('test-sha')
mockGitHubClient.getAuthenticatedUser.mockResolvedValue({
id: 'test-user-id'
})
Expand Down
4 changes: 4 additions & 0 deletions __tests__/client.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,10 @@ describe('GitHubClient', () => {
expect(gitHubClient.getPullRequestHeadSha()).toBe('testSha')
})

test('getPullRequestMergeRef returns the correct ref', () => {
expect(gitHubClient.getPullRequestMergeRef()).toBe('pull/1/merge')
})

test('getPullRequestAuthors returns the correct author IDs', async () => {
const mockCommits = [
{
Expand Down
25 changes: 22 additions & 3 deletions dist/index.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion dist/index.js.map

Large diffs are not rendered by default.

8 changes: 5 additions & 3 deletions src/approval.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@ class ApprovalProcess {
}

async run() {
const prHeadSha = this.gitHubClient.getPullRequestHeadSha()
const ref = this.gitHubClient.getPullRequestMergeRef()
const commitSha = await this.gitHubClient.getRefSha(ref)
const tokenUser = await this.gitHubClient.getAuthenticatedUser()

// used for validation purposes only
Expand All @@ -19,14 +20,15 @@ class ApprovalProcess {
this.config.commentHeader,
this.config.commentFooter
].join('\n\n')

let comment = await this.gitHubClient.findCommitComment(
prHeadSha,
commitSha,
tokenUser.id,
commitCommentBody
)
if (!comment) {
comment = await this.gitHubClient.createCommitComment(
prHeadSha,
commitSha,
commitCommentBody
)
}
Expand Down
17 changes: 17 additions & 0 deletions src/client.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,23 @@ class GitHubClient {
return this.context.payload.pull_request.head.sha
}

getPullRequestMergeRef() {
if (!this.context.payload.pull_request) {
throw new Error('No pull request found in context!')
}
return `pull/${this.context.payload.pull_request.number}/merge`
}

// https://octokit.github.io/rest.js/v18/#git-get-ref
// https://docs.github.com/en/rest/git/refs?apiVersion=2022-11-28#get-a-reference
async getRefSha(ref) {
const { data } = await this.octokit.rest.git.getRef({
...this.context.repo,
ref
})
return data.object.sha
}

async getPullRequestAuthors() {
const commits = await this.getPullRequestCommits()
return commits.map(c => c.author.id)
Expand Down

0 comments on commit c16bf03

Please sign in to comment.