Does not work with pull_request_target

[slorber]

Edit: make sure to check my security issue comment here: #54 (comment)

---

When using pull_request_target, the action compares master against master, instead of comparing master against the PR.

This bug repo PR demonstrates the problem:
#53

I expect changes to be reported in comments according to my changes but no changes were reported.

We can also see the problem in the Action logs:

https://github.com/preactjs/compressed-size-action/runs/1929330272?check_suite_focus=true

The commit hash is used 2 times and is the current HEAD of master:

cc @developit

[slorber]

EDIT: DON'T DO THIS, ITS INSECURE (#54 (comment))

---

A workaround is to force the checkout on pull request head sha with actions/checkout@v2 before this action runs

jobs:
  report-build-size-diff:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v2
        with:
          ref: ${{ github.event.pull_request.head.sha }}
      - uses: preactjs/compressed-size-action@v2
        with:
          repo-token: "${{ secrets.GITHUB_TOKEN }}"
          build-script: "build"
          pattern: "./packages/core/dist/**/*.{js,css}"

This is good enough, but it's probably worth solving this in the action directly by forcing a fetch on context.payload.pull_request.head.sha at the beginning.

[slorber]

After discussing with security at Github and Facebook, my workaround works but is insecure and can lead to the GH token being stolen.

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

When using pull_request_target, you should NEVER run the code from the PR branch in any case.
A malicious attacker can find a way to access the "repo-token" input.
It is not present in INPUT_ env variables (like other inputs), but the token is indeed available somewhere in the memory and can be accessed.

---

So, in its current form, this GH action does not work well for open-source as it can't security work for PRs from forks.

To make this work, we need 2 actions:

• One that builds master/pr-branch with pull_request (read access for forks)
• One that comments the PR with pull_request_target (write access for forks)

[developit]

@slorber the conclusion you came to is effectively why I've held off on supporting this 👍

That being said, one of the possibilities that seems worth considering is if there's a way to report information via a check status rather than a PR comment, since those do not require write access to the primary repository. I wasn't able to get this working, but it seems like the right path forward. Thoughts?

[slorber]

@developit git commit statuses work great for some use-cases but the maintainer would have to check it to see potential size regressions, which will likely remain unseen in many cases. Afaik there's no "warning" status and it's either a success or failure.

It would just be a bit more convenient than checking the GH action log directly, but not much.

Another possibility is to create a generic package that would expose a lambda, that we can easily deploy to Vercel/Netlify to post the comment. I guess other tools could benefit from this as many try to post comments to PR.

[developit]

I don't want to force folks to use an external service - it's a SPOF at best, and a privacy/complexity problem at worst.

One of the things I've been pondering is building a generic "post comment" action that accepts a JSON comment payload as an artifact. Then compressed-size-action could produce the "comment description" artifact via pull_request, then the post-comment-action could use that via pull_request_target to post the comment (which would be secure since it doesn't execute or interact with any repo code).

[slorber]

Yes that should work, also what I suggested by using 2 actions above.

FYI we are using this action to post a lighthouse comment, so it can probably be re-used, as long as you provide the message as a string.

https://github.com/marocchino/sticky-pull-request-comment

      - name: Add Lighthouse stats as comment
        id: comment_to_pr
        uses: marocchino/sticky-pull-request-comment@v2.0.0
        with:
          GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
          number: ${{ github.event.pull_request.number }}
          header: lighthouse
          message: ${{ steps.format_lighthouse_score.outputs.comment }}

building a generic "post comment" action that accepts a JSON comment payload as an artifact

Why JSON instead of string? If this action was able to provide the current comment as a string artifact, I could probably use today the workflow using this action + sticky-pull-request-comment