From 69ca4dc55768273316fa7bb99c9496b563ae1c1b Mon Sep 17 00:00:00 2001 From: Kyle Harding Date: Sun, 8 Dec 2024 09:46:55 -0500 Subject: [PATCH 1/2] Assert that author and committer are non-null Change-type: patch Signed-off-by: Kyle Harding --- src/handlers/deployment-protection-rule.ts | 11 +++++++++-- src/handlers/pull-request-review.ts | 11 +++++++++-- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/src/handlers/deployment-protection-rule.ts b/src/handlers/deployment-protection-rule.ts index 65e60c5..9305891 100644 --- a/src/handlers/deployment-protection-rule.ts +++ b/src/handlers/deployment-protection-rule.ts @@ -1,6 +1,7 @@ import type { Context } from 'probot'; import type { DeploymentProtectionRuleRequestedEvent } from '@octokit/webhooks-types'; import * as GitHubClient from '../client.js'; +import assert from 'assert'; export const instructionalComment = 'One or more environments require approval before deploying workflow runs.\n\n' + @@ -51,9 +52,15 @@ export async function handleDeploymentProtectionRule( // Get the commit that triggered the workflow run const commit = await GitHubClient.getCommit(context, deployment.sha); + assert(commit.author, `Failed to get author for SHA: ${deployment.sha}`); + assert( + commit.committer, + `Failed to get committer for SHA: ${deployment.sha}`, + ); + // Approve deployment if the commit author is in the bypass list const bypassActors = process.env.BYPASS_ACTORS?.split(',') ?? []; - if (commit.author?.id && bypassActors.includes(commit.author.id.toString())) { + if (bypassActors.includes(commit.author.id.toString())) { return context.octokit.request(`POST ${callbackUrl}`, { environment_name: environment, state: 'approved', @@ -63,7 +70,7 @@ export async function handleDeploymentProtectionRule( context.log.debug( 'Commit author is not included in bypass actors: %s', - commit.author?.login, + commit.author.login, ); const client = await context.octokit.apps.getAuthenticated(); diff --git a/src/handlers/pull-request-review.ts b/src/handlers/pull-request-review.ts index 7f59ce2..0884833 100644 --- a/src/handlers/pull-request-review.ts +++ b/src/handlers/pull-request-review.ts @@ -4,6 +4,7 @@ import type { WorkflowRun, } from '@octokit/webhooks-types'; import * as GitHubClient from '../client.js'; +import assert from 'assert'; export async function handlePullRequestReview(context: Context) { const { review } = context.payload as PullRequestReviewSubmittedEvent; @@ -43,7 +44,13 @@ export async function handlePullRequestReview(context: Context) { // Get the commit of the submitted review const commit = await GitHubClient.getCommit(context, review.commit_id); - if (commit.author?.id === review.user.id) { + assert(commit.author, `Failed to get author for SHA: ${review.commit_id}`); + assert( + commit.committer, + `Failed to get committer for SHA: ${review.commit_id}`, + ); + + if (commit.author.id === review.user.id) { context.log.debug( 'Ignoring review by author of the commit: %s', review.user.login, @@ -51,7 +58,7 @@ export async function handlePullRequestReview(context: Context) { return; } - if (commit.committer?.id === review.user.id) { + if (commit.committer.id === review.user.id) { context.log.debug( 'Ignoring review by committer of the commit: %s', review.user.login, From 720c460639c4fa69249e40c94687c5642cf7cc8c Mon Sep 17 00:00:00 2001 From: Kyle Harding Date: Sun, 8 Dec 2024 19:03:18 -0500 Subject: [PATCH 2/2] Remove body match requirement from approved reviews If the changes are approved for merge, there is no need to also provide the /deploy string in the body. Change-type: patch Signed-off-by: Kyle Harding --- README.md | 8 +++++--- src/handlers/deployment-protection-rule.ts | 11 +++++++---- src/handlers/pull-request-review.ts | 7 ++++++- tests/handlers/deployment-protection-rule.test.ts | 1 - 4 files changed, 18 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index e4e3eec..8529f0c 100644 --- a/README.md +++ b/README.md @@ -10,7 +10,7 @@ Deploynaut functions as a GitHub App, managing deployment approvals via [custom ### Approval Process -Deployments are approved by submitting a review with the `/deploy` command. +Deployments are approved by submitting an approved review, or a `/deploy` command in a commented review. #### Deployment Validation @@ -30,7 +30,9 @@ Deployments are auto-approved if: For manual approvals: -- An eligible reviewer submits a [review](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/reviewing-proposed-changes-in-a-pull-request#submitting-your-review) with the `/deploy` command. +- An eligible reviewer submits a [review](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/reviewing-proposed-changes-in-a-pull-request#submitting-your-review). + - Commented reviews need to start with `/deploy`. + - Approved reviews do not need to match any string. - The app approves pending deployments matching the reviewed commit SHA. #### Eligible Reviewers @@ -56,7 +58,7 @@ Key security features include: 1. Developer opens a PR and triggers a deployment. 2. App receives a deployment protection rule event. 3. If not auto-approved, the app comments on the PR with instructions. -4. Eligible reviewer submits a review with `/deploy`. +4. Eligible reviewer submits a commentedreview with `/deploy`. 5. App validates the approval and enables deployment. ## Setup diff --git a/src/handlers/deployment-protection-rule.ts b/src/handlers/deployment-protection-rule.ts index 9305891..ab79ea1 100644 --- a/src/handlers/deployment-protection-rule.ts +++ b/src/handlers/deployment-protection-rule.ts @@ -5,8 +5,8 @@ import assert from 'assert'; export const instructionalComment = 'One or more environments require approval before deploying workflow runs.\n\n' + - 'Maintainers, please inspect changes carefully for improper handling of sensitive information and submit a review with the required string.\n\n' + - '> **Files changed -> Review changes -> Comment** -> `/deploy`'; + 'Maintainers, please inspect changes carefully for improper handling of secrets or other sensitive information.\n\n' + + 'To approve pending deployments, submit an approved review, or a commented review with `/deploy`.'; export async function handleDeploymentProtectionRule( context: Context, @@ -83,13 +83,16 @@ export async function handleDeploymentProtectionRule( ); // Find an eligible review authored by a different user than the commit author or committer + // Comment reviews need to start with /deploy + // Approved reviews do not need to match any string const deployReview = reviews.find( (review) => - ['approved', 'commented'].includes(review.state.toLowerCase()) && review.commit_id === deployment.sha && review.user.id !== commit.author?.id && review.user.id !== commit.committer?.id && - review.body?.startsWith('/deploy'), + (review.state.toLowerCase() === 'approved' || + (review.state.toLowerCase() === 'commented' && + review.body?.startsWith('/deploy'))), ); if (deployReview) { diff --git a/src/handlers/pull-request-review.ts b/src/handlers/pull-request-review.ts index 0884833..ace27b8 100644 --- a/src/handlers/pull-request-review.ts +++ b/src/handlers/pull-request-review.ts @@ -31,7 +31,12 @@ export async function handlePullRequestReview(context: Context) { return; } - if (!review.body?.startsWith('/deploy')) { + // Comment reviews need to start with /deploy + // Approved reviews do not need to match any string + if ( + review.state.toLowerCase() === 'commented' && + !review.body?.startsWith('/deploy') + ) { context.log.debug('Ignoring unsupported comment'); return; } diff --git a/tests/handlers/deployment-protection-rule.test.ts b/tests/handlers/deployment-protection-rule.test.ts index 1833dbc..0e79ee5 100644 --- a/tests/handlers/deployment-protection-rule.test.ts +++ b/tests/handlers/deployment-protection-rule.test.ts @@ -192,7 +192,6 @@ describe('Deployment Protection Rule Handler', () => { .reply(200, [ { commit_id: 'test-sha', - body: '/deploy please', state: 'APPROVED', user: { login: 'test-user', id: 789 }, },