Publish a comment when changes with the GNU testsuite are identified

[sylvestre]

No description provided.

[tertsdiepraam]

I was able to post a comment to a PR on one of my own repositories as a test case: tertsdiepraam/aoc2021#1

permissions were not necessary for this repo. It did not work on push events though, which seems to require a workaround (see actions/github-script#273 (comment))

The YAML is:

name: PostComment

on: [pull_request]

jobs: 
  postcomment:
    runs-on: ubuntu-latest
    steps:
      - name: Publish the result if any change (spell check  test)
        uses: actions/github-script@v6
        with:
          github-token: ${{secrets.GITHUB_TOKEN}}
          script: |
              await github.rest.issues.createComment({
                issue_number: context.issue.number,
                owner: context.repo.owner,
                repo: context.repo.repo,
                body: 'Comment posted!'
              })

[tertsdiepraam]

I wonder if there is some ownership issue going on here. Of course, we wouldn't want anyone to just open a PR which suddenly posts a bunch of comments on issues and pull requests. So github might be restricting the action until its merged or something like that?

[sylvestre]

your repo is private but I trust you :)

So github might be restricting the action until its merged or something like that?

Maybe? This is why I pushed
a612308

Oh, I just noticed that we have "Issues: write" maybe it needs PullRequests too! I will try
https://github.com/uutils/coreutils/actions/runs/3207061754/jobs/5241542982

[tertsdiepraam]

I think this explains the issue:

Due to the dangers inherent to automatic processing of PRs, GitHub’s standard pull_request workflow trigger by default prevents write permissions and secrets access to the target repository. However, in some scenarios such access is needed to properly process the PR. To this end the pull_request_target workflow trigger was introduced.

https://securitylab.github.com/research/github-actions-preventing-pwn-requests/

your repo is private but I trust you :)

Oops, fixed :)

[sylvestre]

I tried with pull_request_target but the PR doesn't start ...
I just landed the commit but still

GITHUB_TOKEN Permissions
  Actions: read
  Contents: read
  Issues: read
  Metadata: read
  PullRequests: read

[tertsdiepraam]

Okay so here's what I understand from https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request_target

• Normal PR workflows run from the merge commit, hence running untrusted (because unmerged) code.
• So we cannot give them write permissions when they come from a fork, because that would mean running untrusted code with write access.
• Hence, there is pull_request_target, which runs when a PR is created but runs on the target branch. So the PR author is not able to change the code of the workflow itself and cannot run untrusted code.
• In pull_request_target we can checkout the code and run the spell check or GNU test. Though we must be careful not to run any files that we checked out, because that again means running untrusted code.
• So the safest thing to do is to keep everything as is, but add additional workflows that run on pull_request_target that get info from the previous workflow. See the ReceivePR/CommentPR example here: https://securitylab.github.com/research/github-actions-preventing-pwn-requests/

[tertsdiepraam]

I tried with pull_request_target but the PR doesn't start ...

This makes sense, because pull_request_target runs in the context of the main branch, without any knowledge of new code including the addition of the new job. I think it will only run on new PRs once its merged.

[sylvestre]

good idea, let's try then :)

[sylvestre]

Looking great here:
https://github.com/uutils/coreutils/actions/runs/3210127098/jobs/5247357217

GITHUB_TOKEN Permissions
  Actions: read
  Contents: read
  Issues: write
  Metadata: read
  PullRequests: write

fingers crossed ;)

[tertsdiepraam]

I think this might work for the comment, but will not do what you expect because it will run on main not on the merge commit. But we can at least check whether the job will succeed and work from there.

Do you mind if I give this a shot in another PR?

[sylvestre]

@tertsdiepraam
not at all :) thanks!

[tertsdiepraam]

We're good now, right?