Reject reviews by commit authors and/or committers

Testing confirmed that the deployment creator property
will reflect whoever triggered the workflow, including performing
actions like merging a PR, and does not reflect the commit author.

Instead, compare review authors and bypass users against the author
of the commit that triggered the event.

Change-type: patch
Signed-off-by: Kyle Harding <kyle@balena.io>

README.md

@@ -21,9 +21,9 @@ Deployments are approved by submitting a review with the `/deploy` command.
 
 Deployments are auto-approved if:
 
-- They originate from a previously approved commit SHA (including pull request merges).
+- They triggered by a previously approved commit SHA (including pull request merges).
 - They are initiated by an allowlisted user (e.g., Renovate) who is:
-  - The deployment creator.
+  - The author of the commit that triggered the deployment.
   - Listed in the `BYPASS_USERS` IDs.
 
 #### Manual Approval Process
@@ -38,7 +38,7 @@ For manual approvals:
 Reviewers must:
 
 - Have repository write access or higher.
-- Not be the deployment actor.
+- Not be the commit author or committer.
 - Not be a bot account.
 
 ### Security Measures
@@ -49,7 +49,7 @@ Key security features include:
 - Ensuring comment integrity (unmodified since creation).
 - Maintaining stateless operations.
 - Preventing TOCTOU attacks with atomic operations.
-- Requiring different actors for deployment and approval.
+- Requiring different actors for commit and approval.
 
 ### Example Workflow
 

src/client.ts

@@ -5,6 +5,7 @@ import type {
 } from '@octokit/webhooks-types';
 import type { components } from '@octokit/openapi-types';
 type PendingDeployment = components['schemas']['pending-deployment'];
+type Commit = components['schemas']['commit'];
 
 // // https://octokit.github.io/rest.js/v21/#repos-get-collaborator-permission-level
 // // https://docs.github.com/en/rest/collaborators/collaborators#list-repository-collaborators
@@ -85,6 +86,16 @@ type PendingDeployment = components['schemas']['pending-deployment'];
 // 	return commits;
 // }
 
+// https://octokit.github.io/rest.js/v21/#repos-get-commit
+// https://docs.github.com/en/rest/commits/commits#get-a-commit
+export async function getCommit(context: any, sha: string): Promise<Commit> {
+	const request = context.repo({
+		sha,
+	});
+	const { data: commit } = await context.octokit.rest.repos.getCommit(request);
+	return commit;
+}
+
 // 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(

src/handlers/deployment-protection-rule.ts

@@ -25,18 +25,14 @@ export async function handleDeploymentProtectionRule(
 		event,
 		deployment: {
 			id: deployment?.id,
-			creator: {
-				id: deployment?.creator.id,
-				login: deployment?.creator.login,
-			},
 			ref: deployment?.ref,
 			sha: deployment?.sha,
 		},
 	};
 
 	context.log.info(
 		'Received deployment protection rule event: %s',
-		JSON.stringify(eventDetails, null, 2),
+		JSON.stringify(eventDetails),
 	);
 
 	if (!deployment || !event || !environment || !callbackUrl) {
@@ -52,18 +48,22 @@ export async function handleDeploymentProtectionRule(
 		return;
 	}
 
+	// Get the commit that triggered the workflow run
+	const commit = await GitHubClient.getCommit(context, deployment.sha);
+
+	// Approve deployment if the commit author is in the bypass list
 	const bypassActors = process.env.BYPASS_ACTORS?.split(',') ?? [];
-	if (bypassActors.includes(deployment.creator.id.toString())) {
+	if (commit.author?.id && bypassActors.includes(commit.author.id.toString())) {
 		return context.octokit.request(`POST ${callbackUrl}`, {
 			environment_name: environment,
 			state: 'approved',
-			comment: `Approved via bypass actors list for ${deployment.creator.login}`,
+			comment: `Approved via bypass actors list for ${commit.author.login}`,
 		});
 	}
 
 	context.log.debug(
-		'Actor is not included in bypass actors: %s',
-		deployment.creator.login,
+		'Commit author is not included in bypass actors: %s',
+		commit.author?.login,
 	);
 
 	const client = await context.octokit.apps.getAuthenticated();
@@ -75,11 +75,13 @@ export async function handleDeploymentProtectionRule(
 				pull.number,
 			);
 
+			// Find an eligible review authored by a different user than the commit author or committer
 			const deployReview = reviews.find(
 				(review) =>
 					['approved', 'commented'].includes(review.state.toLowerCase()) &&
 					review.commit_id === deployment.sha &&
-					review.user.id !== deployment.creator.id &&
+					review.user.id !== commit.author?.id &&
+					review.user.id !== commit.committer?.id &&
 					review.body?.startsWith('/deploy'),
 			);
 

src/handlers/pull-request-review.ts

@@ -22,7 +22,7 @@ export async function handlePullRequestReview(context: Context) {
 
 	context.log.info(
 		'Received pull request review event: %s',
-		JSON.stringify(eventDetails, null, 2),
+		JSON.stringify(eventDetails),
 	);
 
 	if (!['approved', 'commented'].includes(review.state.toLowerCase())) {
@@ -40,44 +40,61 @@ export async function handlePullRequestReview(context: Context) {
 		return;
 	}
 
+	// Get the commit of the submitted review
+	const commit = await GitHubClient.getCommit(context, review.commit_id);
+
+	if (commit.author?.id === review.user.id) {
+		context.log.debug(
+			'Ignoring review by author of the commit: %s',
+			review.user.login,
+		);
+		return;
+	}
+
+	if (commit.committer?.id === review.user.id) {
+		context.log.debug(
+			'Ignoring review by committer of the commit: %s',
+			review.user.login,
+		);
+		return;
+	}
+
 	const workflowRuns = await GitHubClient.listWorkflowRuns(
 		context,
 		review.commit_id,
 	);
 
 	await Promise.all(
-		workflowRuns
-			.filter((workflowRun) => workflowRun.actor.id !== review.user.id)
-			.map(async (workflowRun: WorkflowRun) => {
-				const pendingDeployments = await GitHubClient.listPendingDeployments(
-					context,
+		workflowRuns.map(async (workflowRun: WorkflowRun) => {
+			const pendingDeployments = await GitHubClient.listPendingDeployments(
+				context,
+				workflowRun.id,
+			);
+
+			if (pendingDeployments.length === 0) {
+				context.log.info(
+					'No pending deployments found for workflow run %s',
 					workflowRun.id,
 				);
+				return;
+			}
 
-				if (pendingDeployments.length === 0) {
-					context.log.info(
-						'No pending deployments found for workflow run %s',
-						workflowRun.id,
-					);
-					return;
-				}
-
-				const environmentNames = pendingDeployments
-					.filter((deployment) => deployment.current_user_can_approve)
-					.filter((deployment) => deployment.environment.name !== undefined)
-					.map((deployment) => deployment.environment.name!);
+			const environmentNames = pendingDeployments
+				.filter((deployment) => deployment.current_user_can_approve)
+				.filter((deployment) => deployment.environment.name !== undefined)
+				.map((deployment) => deployment.environment.name!);
 
-				await Promise.all(
-					environmentNames.map((environmentName) =>
-						GitHubClient.reviewWorkflowRun(
-							context,
-							workflowRun.id,
-							environmentName,
-							'approved',
-							`Approved by ${review.user.login} via [review](${review.html_url})`,
-						),
+			await Promise.all(
+				environmentNames.map((environmentName) =>
+					GitHubClient.reviewWorkflowRun(
+						context,
+						workflowRun.id,
+						environmentName,
+						'approved',
+						`Approved by ${review.user.login} via [review](${review.html_url})`,
 					),
-				);
-			}),
+				),
+			);
+		}),
 	);
 }