Reapply [workflows] Split pr-code-format into two parts to make it more secure (#78215) (#80495)

tstellar · web-flow · commit 2120f574103c · 2024-03-22T11:45:51.000-07:00
Actions triggered by pull_request_target events have access to all repository secrets, so it is unsafe to use them when executing untrusted code. The pr-code-format workflow does not execute any untrusted code, but it passes untrused input into clang-format. An attacker could use this to exploit a flaw in clang-format and potentially gain access to the repository secrets. By splitting the workflow, we can use the pull_request target which is more secure and isolate the issue write permissions in a separate job. The pull_request target also makes it easier to test changes to the code-format-helepr.py script, because the version of the script from the pull request will be used rather than the version of the script from main. Fixes #77142
diff --git a/.github/workflows/issue-write.yml b/.github/workflows/issue-write.yml
@@ -0,0 +1,128 @@
+name: Comment on an issue
+
+on:
+  workflow_run:
+    workflows: ["Check code formatting"]
+    types:
+      - completed
+
+permissions:
+  contents: read
+
+jobs:
+  pr-comment:
+    runs-on: ubuntu-latest
+    permissions:
+      pull-requests: write
+    if: >
+      github.event.workflow_run.event == 'pull_request'
+    steps:
+      - name: 'Download artifact'
+        uses: actions/download-artifact@6b208ae046db98c579e8a3aa621ab581ff575935 # v4.1.1
+        with:
+          github-token: ${{ secrets.ISSUE_WRITE_DOWNLOAD_ARTIFACT }}
+          run-id: ${{ github.event.workflow_run.id }}
+          name: workflow-args
+
+      - name: 'Comment on PR'
+        uses: actions/github-script@v3
+        with:
+          github-token: ${{ secrets.GITHUB_TOKEN }}
+          script: |
+            var fs = require('fs');
+            const comments = JSON.parse(fs.readFileSync('./comments'));
+            if (!comments) {
+              return;
+            }
+
+            let runInfo = await github.actions.getWorkflowRun({
+              owner: context.repo.owner,
+              repo: context.repo.repo,
+              run_id: context.payload.workflow_run.id
+            });
+
+            console.log(runInfo);
+
+
+            // Query to find the number of the pull request that triggered this job.
+            // The associated pull requests are based off of the branch name, so if
+            // you create a pull request for a branch, close it, and then create
+            // another pull request with the same branch, then this query will return
+            // two associated pull requests.  This is why we have to fetch all the
+            // associated pull requests and then iterate through them to find the
+            // one that is open.
+            const gql_query = `
+              query($repo_owner : String!, $repo_name : String!, $branch: String!) {
+                repository(owner: $repo_owner, name: $repo_name) {
+                  ref (qualifiedName: $branch) {
+                    associatedPullRequests(first: 100) {
+                      nodes {
+                        baseRepository {
+                          owner {
+                            login
+                          }
+                        }
+                        number
+                        state
+                      }
+                    }
+                  }
+                }
+              }
+            `
+            const gql_variables = {
+              repo_owner: runInfo.data.head_repository.owner.login,
+              repo_name: runInfo.data.head_repository.name,
+              branch: runInfo.data.head_branch
+            }
+            const gql_result = await github.graphql(gql_query, gql_variables);
+            console.log(gql_result);
+            console.log(gql_result.repository.ref.associatedPullRequests.nodes);
+
+            var pr_number = 0;
+            gql_result.repository.ref.associatedPullRequests.nodes.forEach((pr) => {
+              if (pr.baseRepository.owner.login = context.repo.owner && pr.state == 'OPEN') {
+                pr_number = pr.number;
+              }
+            });
+            if (pr_number == 0) {
+              console.log("Error retrieving pull request number");
+              return;
+            }
+            
+            await comments.forEach(function (comment) {
+              if (comment.id) {
+                // Security check: Ensure that this comment was created by
+                // the github-actions bot, so a malicious input won't overwrite
+                // a user's comment.
+                github.issues.getComment({
+                  owner: context.repo.owner,
+                  repo: context.repo.repo,
+                  comment_id: comment.id
+                }).then((old_comment) => {
+                  console.log(old_comment);
+                  if (old_comment.data.user.login != "github-actions[bot]") {
+                    console.log("Invalid comment id: " + comment.id);
+                    return;
+                  }
+                  github.issues.updateComment({
+                    owner: context.repo.owner,
+                    repo: context.repo.repo,
+                    issue_number: pr_number,
+                    comment_id: comment.id,
+                    body: comment.body
+                  });
+                });
+              } else {
+                github.issues.createComment({
+                  owner: context.repo.owner,
+                  repo: context.repo.repo,
+                  issue_number: pr_number,
+                  body: comment.body
+                });
+              }
+            });
+
+      - name: Dump comments file
+        if: always()
+        run: cat comments
diff --git a/.github/workflows/pr-code-format.yml b/.github/workflows/pr-code-format.yml
@@ -1,12 +1,9 @@
 name: "Check code formatting"
 on:
-  pull_request_target:
+  pull_request:
     branches:
       - main
 
-permissions:
-  pull-requests: write
-
 jobs:
   code_formatter:
     runs-on: ubuntu-latest
@@ -31,12 +28,13 @@ jobs:
           separator: ","
           skip_initial_fetch: true
 
-      # We need to make sure that we aren't executing/using any code from the
-      # PR for security reasons as we're using pull_request_target. Checkout
-      # the target branch with the necessary files.
+      # We need to pull the script from the main branch, so that we ensure
+      # we get the latest version of this script.
       - name: Fetch code formatting utils
         uses: actions/checkout@v4
         with:
+          reository: ${{ github.repository }}
+          ref: ${{ github.base_ref }}
           sparse-checkout: |
             llvm/utils/git/requirements_formatting.txt
             llvm/utils/git/code-format-helper.py
@@ -77,8 +75,16 @@ jobs:
         # the merge base.
         run: |
           python ./code-format-tools/llvm/utils/git/code-format-helper.py \
+            --write-comment-to-file \
             --token ${{ secrets.GITHUB_TOKEN }} \
             --issue-number $GITHUB_PR_NUMBER \
             --start-rev $(git merge-base $START_REV $END_REV) \
             --end-rev $END_REV \
             --changed-files "$CHANGED_FILES"
+
+      - uses: actions/upload-artifact@26f96dfa697d77e81fd5907df203aa23a56210a8 #v4.3.0
+        if: always()
+        with:
+          name: workflow-args
+          path: |
+            comments
diff --git a/llvm/utils/git/code-format-helper.py b/llvm/utils/git/code-format-helper.py
@@ -44,6 +44,7 @@ class FormatArgs:
     token: str = None
     verbose: bool = True
     issue_number: int = 0
+    write_comment_to_file: bool = False
 
     def __init__(self, args: argparse.Namespace = None) -> None:
         if not args is None:
@@ -53,12 +54,14 @@ def __init__(self, args: argparse.Namespace = None) -> None:
             self.token = args.token
             self.changed_files = args.changed_files
             self.issue_number = args.issue_number
+            self.write_comment_to_file = args.write_comment_to_file
 
 
 class FormatHelper:
     COMMENT_TAG = "<!--LLVM CODE FORMAT COMMENT: {fmt}-->"
     name: str
     friendly_name: str
+    comment: dict = None
 
     @property
     def comment_tag(self) -> str:
@@ -119,6 +122,13 @@ def update_pr(self, comment_text: str, args: FormatArgs, create_new: bool) -> No
         comment_text = self.comment_tag + "\n\n" + comment_text
 
         existing_comment = self.find_comment(pr)
+
+        if args.write_comment_to_file:
+            self.comment = {"body": comment_text}
+            if existing_comment:
+                self.comment["id"] = existing_comment.id
+            return
+
         if existing_comment:
             existing_comment.edit(comment_text)
         elif create_new:
@@ -310,6 +320,8 @@ def hook_main():
         if fmt.has_tool():
             if not fmt.run(args.changed_files, args):
                 failed_fmts.append(fmt.name)
+            if fmt.comment:
+                comments.append(fmt.comment)
         else:
             print(f"Couldn't find {fmt.name}, can't check " + fmt.friendly_name.lower())
 
@@ -350,6 +362,11 @@ def hook_main():
         type=str,
         help="Comma separated list of files that has been changed",
     )
+    parser.add_argument(
+        "--write-comment-to-file",
+        action="store_true",
+        help="Don't post comments on the PR, instead write the comments and metadata a file called 'comment'",
+    )
 
     args = FormatArgs(parser.parse_args())
 
@@ -358,9 +375,18 @@ def hook_main():
         changed_files = args.changed_files.split(",")
 
     failed_formatters = []
+    comments = []
     for fmt in ALL_FORMATTERS:
         if not fmt.run(changed_files, args):
             failed_formatters.append(fmt.name)
+        if fmt.comment:
+            comments.append(fmt.comment)
+
+    if len(comments):
+        with open("comments", "w") as f:
+            import json
+
+            json.dump(comments, f)
 
     if len(failed_formatters) > 0:
         print(f"error: some formatters failed: {' '.join(failed_formatters)}")
