Skip to content

Commit

Permalink
Ignore reviews by deployment creators, not specifically authors
Browse files Browse the repository at this point in the history 
Getting a list of commit authors is an async operation that
leaves a small window for a time-of-use attack.

Prefer to use data from the initial event context where possible
and avoid extra async calls to the GitHub API.

Change-type: patch
Signed-off-by: Kyle Harding <kyle@balena.io>
  • Loading branch information
@klutchell
klutchell committed Dec 4, 2024
1 parent 7ae7504 commit 2b1fa79
Show file tree
Hide file tree
Showing 5 changed files with 32 additions and 73 deletions.
28 changes: 10 additions & 18 deletions src/client.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,28 +69,20 @@
// return deployments;
// }

// export async function listPullRequestContributors(
// // https://octokit.github.io/rest.js/v21/#pulls-list-commits
// // https://docs.github.com/en/rest/pulls/pulls#list-commits-on-a-pull-request
// export async function listPullRequestCommits(
// context: any,
// prNumber: number,
// ): Promise<string[]> {
// const commits = await this.listPullRequestCommits(context, prNumber);
// return commits.map((c: any) => c.author.id);
// ): Promise<any[]> {
// const request = context.repo({
// pull_number: prNumber,
// });
// const { data: commits } =
// await context.octokit.rest.pulls.listCommits(request);
// return commits;
// }

// https://octokit.github.io/rest.js/v21/#pulls-list-commits
// https://docs.github.com/en/rest/pulls/pulls#list-commits-on-a-pull-request
export async function listPullRequestCommits(
context: any,
prNumber: number,
): Promise<any[]> {
const request = context.repo({
pull_number: prNumber,
});
const { data: commits } =
await context.octokit.rest.pulls.listCommits(request);
return commits;
}

// https://octokit.github.io/rest.js/v21/#pulls-list-reviews
// https://docs.github.com/en/rest/pulls/reviews#list-reviews-for-a-pull-request
export async function listPullRequestReviews(
Expand Down
6 changes: 0 additions & 6 deletions src/handlers/deployment-protection-rule.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,17 +74,11 @@ export async function handleDeploymentProtectionRule(
pull.number,
);

const commits = await GitHubClient.listPullRequestCommits(
context,
pull.number,
);

const deployReview = reviews.find(
(review) =>
review.state !== 'CHANGES_REQUESTED' &&
review.commit_id === deployment.sha &&
review.user.id !== deployment.creator.id &&
!commits.map((c: any) => c.author.id).includes(review.user.id) &&
review.body.startsWith('/deploy'),
);

Expand Down
20 changes: 3 additions & 17 deletions src/handlers/pull-request-review.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,7 @@ import type { PullRequestReviewSubmittedEvent } from '@octokit/webhooks-types';
import * as GitHubClient from '../client.js';

export async function handlePullRequestReview(context: Context) {
const { review, pull_request } =
context.payload as PullRequestReviewSubmittedEvent;
const { review } = context.payload as PullRequestReviewSubmittedEvent;

const eventDetails = {
review: {
Expand All @@ -16,10 +15,6 @@ export async function handlePullRequestReview(context: Context) {
login: review.user.login,
},
},
pull_request: {
// eslint-disable-next-line id-denylist
number: pull_request.number,
},
};

context.log.info(
Expand All @@ -37,17 +32,7 @@ export async function handlePullRequestReview(context: Context) {
return;
}

const commits = await GitHubClient.listPullRequestCommits(
context,
pull_request.number,
);

if (commits.map((c: any) => c.author.id).includes(review.user.id)) {
context.log.info('Ignoring review by commit author: %s', review.user.login);
return;
}

const runs = await GitHubClient.listWorkflowRuns(context, commits[0].sha);
const runs = await GitHubClient.listWorkflowRuns(context, review.commit_id);

for (const run of runs) {
const deployments = await GitHubClient.listPendingDeployments(
Expand All @@ -65,6 +50,7 @@ export async function handlePullRequestReview(context: Context) {

const environments = deployments
.filter((deployment) => deployment.current_user_can_approve)
.filter((deployment) => deployment.creator.id !== review.user.id)
.map((deployment) => deployment.environment.name);

for (const environment of environments) {
Expand Down
18 changes: 2 additions & 16 deletions test/handlers/deployment-protection-rule.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -117,8 +117,6 @@ describe('Deployment Protection Rule Handler', () => {
.reply(200, { id: 456 })
.get('/repos/test-org/test-repo/pulls/123/reviews')
.reply(200, [])
.get('/repos/test-org/test-repo/pulls/123/commits')
.reply(200, [{ author: { id: 123 } }])
.get('/repos/test-org/test-repo/issues/123/comments')
.reply(200, [])
.post('/repos/test-org/test-repo/issues/123/comments', {
Expand All @@ -144,8 +142,6 @@ describe('Deployment Protection Rule Handler', () => {
.reply(200, { id: 456 })
.get('/repos/test-org/test-repo/pulls/123/reviews')
.reply(200, [])
.get('/repos/test-org/test-repo/pulls/123/commits')
.reply(200, [{ author: { id: 123 } }])
.get('/repos/test-org/test-repo/issues/123/comments')
.reply(200, [])
.post('/repos/test-org/test-repo/issues/123/comments', {
Expand Down Expand Up @@ -195,8 +191,6 @@ describe('Deployment Protection Rule Handler', () => {
user: { login: 'test-user', id: 789 },
},
])
.get('/repos/test-org/test-repo/pulls/123/commits')
.reply(200, [{ author: { id: 123 } }])
.post(
'/repos/test-org/test-repo/actions/runs/1234/deployment_protection_rule',
)
Expand Down Expand Up @@ -227,8 +221,6 @@ describe('Deployment Protection Rule Handler', () => {
user: { login: 'test-user', id: 789 },
},
])
.get('/repos/test-org/test-repo/pulls/123/commits')
.reply(200, [{ author: { id: 123 } }])
.post(
'/repos/test-org/test-repo/actions/runs/1234/deployment_protection_rule',
)
Expand All @@ -244,7 +236,7 @@ describe('Deployment Protection Rule Handler', () => {
expect(mock.pendingMocks()).toStrictEqual([]);
});

test('ignores reviews by commit authors', async () => {
test('ignores reviews by deployment creator', async () => {
const mock = nock('https://api.github.com')
.post('/app/installations/12345678/access_tokens')
.reply(200, { token: 'test', permissions: { issues: 'write' } })
Expand All @@ -256,11 +248,9 @@ describe('Deployment Protection Rule Handler', () => {
commit_id: 'test-sha',
body: '/deploy please',
state: 'COMMENTED',
user: { login: 'test-user', id: 789 },
user: { login: 'test-user', id: 5 },
},
])
.get('/repos/test-org/test-repo/pulls/123/commits')
.reply(200, [{ author: { id: 789 } }])
.get('/repos/test-org/test-repo/issues/123/comments')
.reply(200, [])
.post('/repos/test-org/test-repo/issues/123/comments', {
Expand Down Expand Up @@ -293,8 +283,6 @@ describe('Deployment Protection Rule Handler', () => {
user: { login: 'test-user', id: 789 },
},
])
.get('/repos/test-org/test-repo/pulls/123/commits')
.reply(200, [{ author: { id: 123 } }])
.get('/repos/test-org/test-repo/issues/123/comments')
.reply(200, [])
.post('/repos/test-org/test-repo/issues/123/comments', {
Expand All @@ -320,8 +308,6 @@ describe('Deployment Protection Rule Handler', () => {
.reply(200, { id: 456 })
.get('/repos/test-org/test-repo/pulls/123/reviews')
.reply(200, [])
.get('/repos/test-org/test-repo/pulls/123/commits')
.reply(200, [{ author: { id: 123 } }])
.get('/repos/test-org/test-repo/issues/123/comments')
.reply(200, [
{
Expand Down
33 changes: 17 additions & 16 deletions test/handlers/pull-request-review.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,6 @@ const testFixtures = {
id: 789,
},
},
pull_request: {
// eslint-disable-next-line id-denylist
number: 2,
},
installation: { id: 12345678 },
repository: {
owner: {
Expand Down Expand Up @@ -62,14 +58,16 @@ describe('Pull Request Review Handler', () => {
const mock = nock('https://api.github.com')
.post('/app/installations/12345678/access_tokens')
.reply(200, { token: 'test', permissions: { issues: 'write' } })
.get('/repos/test-org/test-repo/pulls/2/commits')
.reply(200, [{ author: { id: 123 } }])
.get('/repos/test-org/test-repo/actions/runs')
.query(true)
.reply(200, { workflow_runs: [{ id: 1234 }] })
.get('/repos/test-org/test-repo/actions/runs/1234/pending_deployments')
.reply(200, [
{ environment: { name: 'test' }, current_user_can_approve: true },
{
environment: { name: 'test' },
creator: { id: 123 },
current_user_can_approve: true,
},
])
.post(
'/repos/test-org/test-repo/actions/runs/1234/deployment_protection_rule',
Expand Down Expand Up @@ -121,7 +119,7 @@ describe('Pull Request Review Handler', () => {
expect(nock.pendingMocks()).toStrictEqual([]);
});

test('ignores review by commit authors', async () => {
test('ignores review by deployment creator', async () => {
const payload = {
...testFixtures.pull_request_review,
review: {
Expand All @@ -136,8 +134,17 @@ describe('Pull Request Review Handler', () => {
const mock = nock('https://api.github.com')
.post('/app/installations/12345678/access_tokens')
.reply(200, { token: 'test', permissions: { issues: 'write' } })
.get('/repos/test-org/test-repo/pulls/2/commits')
.reply(200, [{ author: { id: 123 } }]);
.get('/repos/test-org/test-repo/actions/runs')
.query(true)
.reply(200, { workflow_runs: [{ id: 1234 }] })
.get('/repos/test-org/test-repo/actions/runs/1234/pending_deployments')
.reply(200, [
{
environment: { name: 'test' },
creator: { id: 123 },
current_user_can_approve: true,
},
]);

await probot.receive({
name: 'pull_request_review',
Expand All @@ -151,8 +158,6 @@ describe('Pull Request Review Handler', () => {
const mock = nock('https://api.github.com')
.post('/app/installations/12345678/access_tokens')
.reply(200, { token: 'test', permissions: { issues: 'write' } })
.get('/repos/test-org/test-repo/pulls/2/commits')
.reply(200, [{ author: { id: 123 } }])
.get('/repos/test-org/test-repo/actions/runs')
.query(true)
.reply(200, { workflow_runs: [] });
Expand All @@ -169,8 +174,6 @@ describe('Pull Request Review Handler', () => {
const mock = nock('https://api.github.com')
.post('/app/installations/12345678/access_tokens')
.reply(200, { token: 'test', permissions: { issues: 'write' } })
.get('/repos/test-org/test-repo/pulls/2/commits')
.reply(200, [{ author: { id: 123 } }])
.get('/repos/test-org/test-repo/actions/runs')
.query(true)
.reply(200, { workflow_runs: [{ id: 1234 }] })
Expand All @@ -189,8 +192,6 @@ describe('Pull Request Review Handler', () => {
const mock = nock('https://api.github.com')
.post('/app/installations/12345678/access_tokens')
.reply(200, { token: 'test', permissions: { issues: 'write' } })
.get('/repos/test-org/test-repo/pulls/2/commits')
.reply(200, [{ author: { id: 123 } }])
.get('/repos/test-org/test-repo/actions/runs')
.query(true)
.reply(200, { workflow_runs: [{ id: 1234 }] })
Expand Down

0 comments on commit 2b1fa79

Please sign in to comment.