Merge pull request #331 from github/fork-deployment-safety

Fork Deployment Safety 🔒
github · Dec 6, 2024 · 0c15b9a · 0c15b9a
2 parents 6b64b4a + 1cd6d59
commit 0c15b9a
Show file tree

Hide file tree

Showing 9 changed files with 319 additions and 15 deletions.
diff --git a/README.md b/README.md
@@ -626,23 +626,27 @@ What to see live examples of this Action in use?
 
 Check out some of the links below to see how others are using this Action in their projects:
 
-- [github/entitlements-config](https://github.com/github/entitlements-config/blob/076a1f0f9e8cc1f5acb8a0b8e133b0a1300c8191/.github/workflows/branch-deploy.yml)
+- [github/entitlements-config](https://github.com/github/entitlements-config/blob/a0ae9820dceaf4542335e4668316d049e466841a/.github/workflows/branch-deploy.yml)
 - [the-hideout/cloudflare](https://github.com/the-hideout/cloudflare/blob/3f3adedb729b9aba0cc324a161ad8ddd6f56141b/.github/workflows/branch-deploy.yml)
 - [the-hideout/tarkov-api](https://github.com/the-hideout/tarkov-api/blob/1677543951d5f2a848c2650eb3400178b8f9a55b/.github/workflows/branch-deploy.yml)
-- [the-hideout/stash](https://github.com/the-hideout/stash/blob/bbcf12425c63122bf1ddb5a0dff6e0eb9ad9939d/.github/workflows/branch-deploy.yml)
+- [the-hideout/stash](https://github.com/the-hideout/stash/blob/aef5a5f16b4fa6946d2eba107e7b92c5f6583c0d/.github/workflows/branch-deploy.yml)
 - [GrantBirki/blog](https://github.com/GrantBirki/blog/blob/559b9be5cc3eac923be5d7923ec9a0b50429ced2/.github/workflows/branch-deploy.yml)
 
 > Are you using this Action in a cool new way? Open a pull request to this repo to have your workflow added to the list above!
 
 ## Suggestions 🌟
 
-This section will cover a few suggestions that will help you when using this Action
+This section will cover a few suggestions and best practices that will help you when using this Action.
 
 1. Suggest Updating Pull Request Branches - You should absolutely use this option when using the `branch-deploy` Action. This option can be found in your repository's `/settings` page
-
-    ![branch-setting](https://user-images.githubusercontent.com/23362539/172939811-a8816db8-8e7c-404a-b12a-11ec5bc6e93d.png)
-
+    ![update-pr-branches](./docs//assets/update-branch-setting.png)
 2. Enable Branch Protection Settings - It is always a good idea to enable branch protection settings for your repo, especially when using this Action
+    1. Require Pull Request Reviews - Enforce that pull requests have approvals, code owner approvals, and dismiss stale pull request approvals upon new commits
+      ![use-pr-reviews](./docs/assets/pr-reviews.png)
+    2. Add Required Status Checks - Enforce that certain CI checks must pass before a pull request can be merged
+      ![use-status-checks](./docs/assets/required-ci-checks.png)
+3. If you don't need to deploy PR forks (perhaps your project is internal and not open source), you can set the `allow_forks` input to `"false"` to prevent deployments from running on forks.
+4. You should **always** (unless you have a certain restriction) use the `sha` output variable over the `ref` output variable when deploying. It is more reliable for deployments, and safer from a security perspective. More details about using commit SHAs for deployments can be found [here](./docs/deploying-commit-SHAs.md).
 
 ## Alternate Command Syntax
 

diff --git a/__tests__/functions/prechecks.test.js b/__tests__/functions/prechecks.test.js
@@ -155,6 +155,10 @@ beforeEach(() => {
   jest.spyOn(isOutdated, 'isOutdated').mockImplementation(() => {
     return {outdated: false, branch: 'test-branch'}
   })
+
+  jest.spyOn(isAdmin, 'isAdmin').mockImplementation(() => {
+    return false
+  })
 })
 
 test('runs prechecks and finds that the IssueOps command is valid for a branch deployment', async () => {
@@ -482,6 +486,242 @@ test('runs prechecks and finds that the IssueOps command is valid for a branch d
   })
 })
 
+test('runs prechecks and finds that the IssueOps command is valid for a branch deployment and is from a forked repository and the PR is approved but CI is failing and it is a noop', async () => {
+  octokit.graphql = jest.fn().mockReturnValue({
+    repository: {
+      pullRequest: {
+        reviewDecision: 'APPROVED',
+        reviews: {
+          totalCount: 4
+        },
+        commits: {
+          nodes: [
+            {
+              commit: {
+                oid: 'abcde12345',
+                checkSuites: {
+                  totalCount: 8
+                },
+                statusCheckRollup: {
+                  state: 'FAILURE'
+                }
+              }
+            }
+          ]
+        }
+      }
+    }
+  })
+  octokit.rest.pulls.get = jest.fn().mockReturnValue({
+    data: {
+      head: {
+        sha: 'abcde12345',
+        ref: 'test-ref',
+        label: 'test-repo:test-ref',
+        repo: {
+          fork: true
+        }
+      },
+      base: {
+        ref: 'base-ref'
+      }
+    },
+    status: 200
+  })
+
+  data.environmentObj.noop = true
+
+  expect(await prechecks(context, octokit, data)).toStrictEqual({
+    message:
+      '### ⚠️ Cannot proceed with deployment\n\n- reviewDecision: `APPROVED`\n- commitStatus: `FAILURE`\n\n> Your pull request is approved but CI checks are failing',
+    status: false
+  })
+})
+
+test('runs prechecks and finds that the IssueOps command is a fork and does not require reviews so it proceeds but with a warning', async () => {
+  octokit.graphql = jest.fn().mockReturnValue({
+    repository: {
+      pullRequest: {
+        reviewDecision: null,
+        reviews: {
+          totalCount: 0
+        },
+        commits: {
+          nodes: [
+            {
+              commit: {
+                oid: 'abcde12345',
+                checkSuites: {
+                  totalCount: 8
+                },
+                statusCheckRollup: {
+                  state: 'SUCCESS'
+                }
+              }
+            }
+          ]
+        }
+      }
+    }
+  })
+  octokit.rest.pulls.get = jest.fn().mockReturnValue({
+    data: {
+      head: {
+        sha: 'abcde12345',
+        ref: 'test-ref',
+        label: 'test-repo:test-ref',
+        repo: {
+          fork: true
+        }
+      },
+      base: {
+        ref: 'base-ref'
+      }
+    },
+    status: 200
+  })
+
+  expect(await prechecks(context, octokit, data)).toStrictEqual({
+    message:
+      '🎛️ CI checks have been defined but required reviewers have not been defined',
+    status: true,
+    noopMode: false,
+    ref: 'abcde12345',
+    sha: 'abcde12345'
+  })
+
+  expect(warningMock).toHaveBeenCalledWith(
+    '🚨 pull request reviews are not enforced by this repository and this operation is being performed on a fork - this operation is dangerous! You should require reviews via branch protection settings (or rulesets) to ensure that the changes being deployed are the changes that you reviewed.'
+  )
+})
+
+test('runs prechecks and rejects a pull request from a forked repository because it does not have completed reviews', async () => {
+  octokit.graphql = jest.fn().mockReturnValue({
+    repository: {
+      pullRequest: {
+        reviewDecision: 'REVIEW_REQUIRED',
+        reviews: {
+          totalCount: 0
+        },
+        commits: {
+          nodes: [
+            {
+              commit: {
+                oid: 'abcde12345',
+                checkSuites: {
+                  totalCount: 8
+                },
+                statusCheckRollup: {
+                  state: 'SUCCESS'
+                }
+              }
+            }
+          ]
+        }
+      }
+    }
+  })
+  octokit.rest.pulls.get = jest.fn().mockReturnValue({
+    data: {
+      head: {
+        sha: 'abcde12345',
+        ref: 'test-ref',
+        label: 'test-repo:test-ref',
+        repo: {
+          fork: true
+        }
+      },
+      base: {
+        ref: 'base-ref'
+      }
+    },
+    status: 200
+  })
+
+  // Even admins cannot deploy from a forked repository without reviews
+  jest.spyOn(isAdmin, 'isAdmin').mockImplementation(() => {
+    return true
+  })
+
+  // Even with skipReviews set, the PR is from a forked repository and must have reviews out of pure safety
+  data.environment = 'staging'
+  data.inputs.skipReviews = 'staging'
+
+  expect(await prechecks(context, octokit, data)).toStrictEqual({
+    message:
+      '### ⚠️ Cannot proceed with deployment\n\n- reviewDecision: `REVIEW_REQUIRED`\n\n> All deployments from forks **must** have the required reviews before they can proceed. Please ensure this PR has been reviewed and approved before trying again.',
+    status: false
+  })
+
+  expect(debugMock).toHaveBeenCalledWith(
+    'rejecting deployment from fork without required reviews - noopMode: false'
+  )
+})
+
+test('runs prechecks and rejects a pull request from a forked repository because it does not have completed reviews (noop)', async () => {
+  octokit.graphql = jest.fn().mockReturnValue({
+    repository: {
+      pullRequest: {
+        reviewDecision: 'REVIEW_REQUIRED',
+        reviews: {
+          totalCount: 0
+        },
+        commits: {
+          nodes: [
+            {
+              commit: {
+                oid: 'abcde12345',
+                checkSuites: {
+                  totalCount: 8
+                },
+                statusCheckRollup: {
+                  state: 'SUCCESS'
+                }
+              }
+            }
+          ]
+        }
+      }
+    }
+  })
+  octokit.rest.pulls.get = jest.fn().mockReturnValue({
+    data: {
+      head: {
+        sha: 'abcde12345',
+        ref: 'test-ref',
+        label: 'test-repo:test-ref',
+        repo: {
+          fork: true
+        }
+      },
+      base: {
+        ref: 'base-ref'
+      }
+    },
+    status: 200
+  })
+
+  // Even admins cannot deploy from a forked repository without reviews
+  jest.spyOn(isAdmin, 'isAdmin').mockImplementation(() => {
+    return true
+  })
+
+  // Even with skipReviews set, the PR is from a forked repository and must have reviews out of pure safety
+  data.environment = 'staging'
+  data.inputs.skipReviews = 'staging'
+  data.environmentObj.noop = true
+
+  expect(await prechecks(context, octokit, data)).toStrictEqual({
+    message:
+      '### ⚠️ Cannot proceed with deployment\n\n- reviewDecision: `REVIEW_REQUIRED`\n\n> All deployments from forks **must** have the required reviews before they can proceed. Please ensure this PR has been reviewed and approved before trying again.',
+    status: false
+  })
+
+  expect(debugMock).toHaveBeenCalledWith(
+    'rejecting deployment from fork without required reviews - noopMode: true'
+  )
+})
+
 test('runs prechecks and finds that the IssueOps command is on a PR from a forked repo and is not allowed', async () => {
   octokit.graphql = jest.fn().mockReturnValue({
     repository: {
@@ -1593,6 +1833,10 @@ test('runs prechecks and finds that no CI checks exist but reviews are defined a
     }
   })
 
+  jest.spyOn(isAdmin, 'isAdmin').mockImplementation(() => {
+    return true
+  })
+
   expect(await prechecks(context, octokit, data)).toStrictEqual({
     message:
       '✅ CI checks have not been defined and approval is bypassed due to admin rights',
@@ -1628,6 +1872,10 @@ test('runs prechecks and finds that no CI checks exist and the PR is not approve
     }
   })
 
+  jest.spyOn(isAdmin, 'isAdmin').mockImplementation(() => {
+    return true
+  })
+
   expect(await prechecks(context, octokit, data)).toStrictEqual({
     message:
       '✅ CI checks have not been defined and approval is bypassed due to admin rights',

diff --git a/dist/index.js b/dist/index.js
diff --git a/dist/index.js.map b/dist/index.js.map
diff --git a/docs/assets/pr-reviews.png b/docs/assets/pr-reviews.png
diff --git a/docs/assets/update-branch-setting.png b/docs/assets/update-branch-setting.png
diff --git a/docs/deploying-commit-SHAs.md b/docs/deploying-commit-SHAs.md
@@ -32,6 +32,8 @@ Do this:
 
 This ensures you are deploying the __exact__ commit SHA that branch-deploy has determined is safe to deploy. This is a best practice for security, reliability, and safety during deployments.
 
+Don't worry, this is still a _branch deployment_, you are just telling your deployment process to use the __exact commit SHA__ that the branch points to rather than the branch name itself which is mutable.
+
 ## Introduction
 
 Deploying commit SHAs (Secure Hash Algorithms) is a best practice in software development and deployment processes. This document explains the importance of deploying commit SHAs, focusing on aspects of security, reliability, and safety. It also provides an overview of how commit SHAs work under the hood in Git and how this contributes to the overall safety of the deployment process.