Merge pull request #171 from github/commit-verification

Commit Verification 🔒

__tests__/runner.test.js

@@ -5,6 +5,7 @@ import { Runner } from "../src/runner";
 import * as core from "@actions/core";
 
 const nock = require("nock");
+const sha = "deadbeef";
 
 nock("https://api.github.com")
   .persist()
@@ -31,25 +32,65 @@ afterEach(() => {
 
 describe("processCommits", () => {
   test("We process commits successfully", async () => {
-    let prCommits = [{ author: { login: "robot" } }];
+    process.env["INPUT_COMMITVERIFICATION"] = "false";
+    let prCommits = [
+      { author: { login: "robot" }, verification: { verified: false }, sha },
+    ];
     jest.spyOn(pullRequest, "listCommits").mockImplementation(() => prCommits);
     expect(pullRequest.listCommits()).toBe(prCommits);
 
     let commits = await runner.processCommits("robot");
     expect(commits).toStrictEqual(true);
   });
 
-  test("We process commits unsuccessfully", async () => {
+  test("We process commits successfully with commit verification", async () => {
+    process.env["INPUT_COMMITVERIFICATION"] = "true";
     let prCommits = [
-      { author: { login: "robot" } },
-      { author: { login: "danhoerst" } },
+      { author: { login: "robot" }, verification: { verified: true }, sha },
+      { author: { login: "robot" }, verification: { verified: true }, sha },
+      { author: { login: "robot" }, verification: { verified: true }, sha },
+    ];
+    jest.spyOn(pullRequest, "listCommits").mockImplementation(() => prCommits);
+    expect(pullRequest.listCommits()).toBe(prCommits);
+
+    let commits = await runner.processCommits("robot");
+    expect(commits).toStrictEqual(true);
+  });
+
+  test("We process commits successfully with missing commit verification objects", async () => {
+    process.env["INPUT_COMMITVERIFICATION"] = "false";
+    let prCommits = [{ author: { login: "robot" }, sha }];
+    jest.spyOn(pullRequest, "listCommits").mockImplementation(() => prCommits);
+    expect(pullRequest.listCommits()).toBe(prCommits);
+
+    let commits = await runner.processCommits("robot");
+    expect(commits).toStrictEqual(true);
+  });
+});
+
+describe("processCommits without verification", () => {
+  test("We process commits unsuccessfully due to verification missing", async () => {
+    jest.clearAllMocks();
+    process.env["INPUT_COMMITVERIFICATION"] = "true";
+    let prCommits = [
+      { author: { login: "robot" }, verification: { verified: false }, sha },
     ];
     jest.spyOn(pullRequest, "listCommits").mockImplementation(() => prCommits);
     expect(pullRequest.listCommits()).toBe(prCommits);
 
     let commits = await runner.processCommits("robot");
     expect(commits).toStrictEqual(false);
   });
+
+  test("We process commits unsuccessfully with missing commit verification objects", async () => {
+    process.env["INPUT_COMMITVERIFICATION"] = "true";
+    let prCommits = [{ author: { login: "robot" }, sha }];
+    jest.spyOn(pullRequest, "listCommits").mockImplementation(() => prCommits);
+    expect(pullRequest.listCommits()).toBe(prCommits);
+
+    let commits = await runner.processCommits("robot");
+    expect(commits).toStrictEqual(false);
+  });
 });
 
 describe("processDiff", () => {
@@ -142,11 +183,14 @@ describe("processPrivilegedReviewer", () => {
     process.env["INPUT_CHECKCOMMITS"] = "true";
     process.env["INPUT_CHECKLABELS"] = "true";
     process.env["INPUT_CHECKDIFF"] = "true";
+    process.env["INPUT_COMMITVERIFICATION"] = "false";
     let prLabels = [{ name: "bug" }, { name: "feature-request" }];
     jest.spyOn(pullRequest, "listLabels").mockImplementation(() => prLabels);
     expect(pullRequest.listLabels()).toBe(prLabels);
 
-    let prCommits = [{ author: { login: "robot" } }];
+    let prCommits = [
+      { author: { login: "robot" }, verification: { verified: false }, sha },
+    ];
     jest.spyOn(pullRequest, "listCommits").mockImplementation(() => prCommits);
     expect(pullRequest.listCommits()).toBe(prCommits);
 
@@ -175,8 +219,33 @@ index 2f4e8d9..93c2072 100644
     process.env["INPUT_CHECKCOMMITS"] = "true";
 
     let prCommits = [
-      { author: { login: "robot" } },
-      { author: { login: "malicious" } },
+      { author: { login: "robot" }, verification: { verified: false }, sha },
+      {
+        author: { login: "malicious" },
+        verification: { verified: false },
+        sha,
+      },
+    ];
+    jest.spyOn(pullRequest, "listCommits").mockImplementation(() => prCommits);
+    expect(pullRequest.listCommits()).toBe(prCommits);
+
+    let processed = await runner.processPrivilegedReviewer("robot", {
+      labels: ["bug", "feature-request"],
+    });
+    expect(processed).toStrictEqual(false);
+  });
+
+  test("We process commits unsuccessfully due to missing verification", async () => {
+    process.env["INPUT_CHECKCOMMITS"] = "true";
+    process.env["INPUT_COMMITVERIFICATION"] = "true";
+
+    let prCommits = [
+      { author: { login: "robot" }, verification: { verified: false }, sha },
+      {
+        author: { login: "malicious" },
+        verification: { verified: false },
+        sha,
+      },
     ];
     jest.spyOn(pullRequest, "listCommits").mockImplementation(() => prCommits);
     expect(pullRequest.listCommits()).toBe(prCommits);
@@ -189,8 +258,8 @@ index 2f4e8d9..93c2072 100644
 
   test("We allow bad commits when the option to check them is not set", async () => {
     let prCommits = [
-      { author: { login: "robot" } },
-      { author: { login: "malicious" } },
+      { author: { login: "robot" }, sha },
+      { author: { login: "malicious" }, sha },
     ];
     jest.spyOn(pullRequest, "listCommits").mockImplementation(() => prCommits);
     expect(pullRequest.listCommits()).toBe(prCommits);
@@ -203,6 +272,7 @@ index 2f4e8d9..93c2072 100644
 
   test("We process labels unsuccessfully with the option enabled", async () => {
     process.env["INPUT_CHECKLABELS"] = "true";
+    process.env["INPUT_COMMITVERIFICATION"] = "false";
     let prLabels = [{ name: "bug" }, { name: "feature-request" }];
     jest.spyOn(pullRequest, "listLabels").mockImplementation(() => prLabels);
     expect(pullRequest.listLabels()).toBe(prLabels);
@@ -226,6 +296,7 @@ index 2f4e8d9..93c2072 100644
 
   test("We process the diff unsuccessfully with the option enabled", async () => {
     process.env["INPUT_CHECKDIFF"] = "true";
+    process.env["INPUT_COMMITVERIFICATION"] = "false";
     let prDiff = `| diff --git a/.github/workflows/check-dist.yml b/.github/workflows/check-dist.yml
 index 2f4e8d9..93c2072 100644
 --- a/.github/workflows/check-dist.yml

README.md

@@ -86,12 +86,14 @@ Here are the configuration options for this Action:
 | `checkCommits` | yes | `"true"`                                       | An option to check that every commit in the PR is made from the privileged requester |
 | `checkDiff` | yes | `"true"`                                       | An option to check that the PR diff only has a removal diff, with no additions - This option defaults to `"true"` but it can be disabled by setting it to `"false"` |
 | `checkLabels` | yes | `"true"`                                       | An option to check that the labels on the PR match those defined in the privileged requester config |
+| `commitVerification` | yes | `"false"` | Whether or not to validate all commits have proper verification via GPG signed commits |
 
 ### Outputs 📤
 
 | Output | Description |
 | ------ | ----------- |
 | `approved` | The string `"true"` if the privileged-requester approved the pull request |
+| `commits_verified` | The string `"true"` if all commits in the PR are signed/verified |
 
 ## First Time Setup
 

action.yml

@@ -39,9 +39,15 @@ inputs:
     description: 'An option to check that the labels on the PR match those defined in the privileged requester config'
     required: true
     default: 'true'
+  commitVerification:
+    description: 'Whether or not to validate all commits have proper verification via GPG signed commits'
+    required: true
+    default: 'false'
 outputs:
   approved: # output will be available to future steps
     description: "Whether or not the PR was approved - 'true' or 'false'"
+  commits_verified:
+    description: The string "true" if all commits in the PR are signed/verified
 runs:
   using: 'node20'
   main: 'dist/index.js'

src/runner.js

@@ -13,19 +13,51 @@ class Runner {
     core.info(
       `Commits: Comparing the PR commits to verify that they are all from ${privileged_requester_username}`,
     );
-    for (const [, commit] of Object.entries(this.pullRequest.listCommits())) {
-      let commitAuthor = commit.author.login.toLowerCase();
+
+    const useCommitVerification = core.getBooleanInput("commitVerification");
+    let allCommitsVerified = true;
+
+    const commits = Object.entries(await this.pullRequest.listCommits());
+
+    core.debug(`checking commits: ${commits.length}`);
+
+    for (const [, commit] of commits) {
+      const commitAuthor = commit.author.login.toLowerCase();
+      const commitVerification = commit?.verification?.verified;
+      const sha = commit?.sha;
+
+      core.debug(`checking commit: ${sha}`);
+
+      // check if the commit is verified
+      if (!commitVerification) {
+        allCommitsVerified = false;
+        if (useCommitVerification === true) {
+          core.warning(`Unexpected unverified commit - sha: ${sha}`);
+
+          // if we are using commit verification and the commit is not signed, return false
+          return false;
+        }
+      }
 
       if (commitAuthor !== privileged_requester_username) {
         core.warning(
-          `Unexpected commit author found by ${commitAuthor}! Commits should be authored by ${privileged_requester_username} I will not proceed with the privileged reviewer process.`,
+          `Unexpected commit author found by ${commitAuthor}! Commits should be authored by ${privileged_requester_username}. I will not proceed with the privileged reviewer process - sha: ${sha}`,
         );
         return false;
       }
     }
+
     core.info(
       `Commits: All commits are made by ${privileged_requester_username}. Success!`,
     );
+
+    core.setOutput("commits_verified", allCommitsVerified);
+
+    if (allCommitsVerified === true) {
+      core.info("Commits: All commits are verified. Success!");
+    }
+
+    // if we make it this far, we have verified that all commits are from the privileged requester
     return true;
   }