Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Does not work with pull_request_target #54

Open
slorber opened this issue Feb 18, 2021 · 6 comments
Open

Does not work with pull_request_target #54

slorber opened this issue Feb 18, 2021 · 6 comments
Labels
wontfix This will not be worked on

Comments

@slorber
Copy link
Contributor

slorber commented Feb 18, 2021

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.

image

We can also see the problem in the Action logs:

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

image

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

image

cc @developit

@slorber
Copy link
Contributor Author

slorber commented Feb 18, 2021

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
Copy link
Contributor Author

slorber commented Mar 3, 2021

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
Copy link
Member

@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?

@developit developit added the wontfix This will not be worked on label Jun 21, 2021
@slorber
Copy link
Contributor Author

slorber commented Jun 23, 2021

@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
Copy link
Member

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
Copy link
Contributor Author

slorber commented Jun 24, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

2 participants