[llvm] Reapply [workflows] Split pr-code-format into two parts to make it more secure (#78215) (PR #80495)
Tom Stellard via llvm-commits
llvm-commits at lists.llvm.org
Fri Feb 16 06:53:37 PST 2024
https://github.com/tstellar updated https://github.com/llvm/llvm-project/pull/80495
>From 3925d28616e6f1c132a5d20e02636a062304fec0 Mon Sep 17 00:00:00 2001
From: Tom Stellard <tstellar at redhat.com>
Date: Fri, 2 Feb 2024 11:31:23 -0800
Subject: [PATCH 1/2] Reapply [workflows] Split pr-code-format into two parts
to make it more secure (#78215)
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
---
.github/workflows/issue-write.yml | 122 +++++++++++++++++++++++++++
.github/workflows/pr-code-format.yml | 20 +++--
llvm/utils/git/code-format-helper.py | 26 ++++++
3 files changed, 162 insertions(+), 6 deletions(-)
create mode 100644 .github/workflows/issue-write.yml
diff --git a/.github/workflows/issue-write.yml b/.github/workflows/issue-write.yml
new file mode 100644
index 00000000000000..d2285a0f5f8ebd
--- /dev/null
+++ b/.github/workflows/issue-write.yml
@@ -0,0 +1,122 @@
+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 at 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 at 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.
+ 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 malisious 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
index 5223089ee8a93d..30bb043b36ca9b 100644
--- a/.github/workflows/pr-code-format.yml
+++ b/.github/workflows/pr-code-format.yml
@@ -1,7 +1,5 @@
name: "Check code formatting"
-on: pull_request_target
-permissions:
- pull-requests: write
+on: pull_request
jobs:
code_formatter:
@@ -27,12 +25,14 @@ 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 a version of the script that supports the --wirte-comment-to-file
+ # option.
- name: Fetch code formatting utils
uses: actions/checkout at v4
with:
+ reository: ${{ github.repository }}
+ ref: ${{ github.base_ref }}
sparse-checkout: |
llvm/utils/git/requirements_formatting.txt
llvm/utils/git/code-format-helper.py
@@ -73,8 +73,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 at 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
index 8a29a57d8d16bd..efbd906e4bf651 100755
--- 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:
@@ -309,6 +319,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())
@@ -349,6 +361,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())
@@ -357,9 +374,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)}")
>From 0c678db9645b37ccda901d117128a5dff284ebb7 Mon Sep 17 00:00:00 2001
From: Tom Stellard <tstellar at redhat.com>
Date: Fri, 16 Feb 2024 06:44:15 -0800
Subject: [PATCH 2/2] Fix typos
---
.github/workflows/issue-write.yml | 2 +-
.github/workflows/pr-code-format.yml | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/.github/workflows/issue-write.yml b/.github/workflows/issue-write.yml
index d2285a0f5f8ebd..1e4ace7a0ce739 100644
--- a/.github/workflows/issue-write.yml
+++ b/.github/workflows/issue-write.yml
@@ -87,7 +87,7 @@ jobs:
await comments.forEach(function (comment) {
if (comment.id) {
// Security check: Ensure that this comment was created by
- // the github-actions bot, so a malisious input won't overwrite
+ // the github-actions bot, so a malicious input won't overwrite
// a user's comment.
github.issues.getComment({
owner: context.repo.owner,
diff --git a/.github/workflows/pr-code-format.yml b/.github/workflows/pr-code-format.yml
index 30bb043b36ca9b..3521a2149f37c6 100644
--- a/.github/workflows/pr-code-format.yml
+++ b/.github/workflows/pr-code-format.yml
@@ -26,7 +26,7 @@ jobs:
skip_initial_fetch: true
# We need to pull the script from the main branch, so that we ensure
- # we get a version of the script that supports the --wirte-comment-to-file
+ # we get a version of the script that supports the --write-comment-to-file
# option.
- name: Fetch code formatting utils
uses: actions/checkout at v4
More information about the llvm-commits
mailing list