Closed msamprz closed 4 years ago
Hm, haven't seen that before. Are you able to share your repo by chance (or just the relevant pieces)?
Hey @damccorm, thanks for the response. Yes, sure. The repo is over at github.com/binary-com/deriv-app. Although due to the failing action, we have currently disabled it, but you can find the PR that enabled Labeler here, and an example of a failed run here.
Hm, so it looks like this is an issue with forks: works fine on my branch fails on fork
I'll reach out internally and figure out if that's expected scoping of permissions for the GITHUB_TOKEN and we can go from there
I just submitted a support ticket about this, but probably should have checked here first! thanks @damccorm
So it turns out that this is working as intended after all. We can't give write permissions to forks for security reasons (e.g. the forked user changes your yaml file to write bad things to your repo), so this should fail on forks.
With that said, the docs are wrong here and need to be updated. Already added https://github.com/actions/starter-workflows/pull/78 to update the template, will also follow up to update docs here.
Thanks for investigating, @damccorm. And the update seems like a good alternative for my use at least. Others can reopen this if necessary, but I'll close it as it's expected behavior.
Just pinging you, @squidsoup to loop you in.
I spoke too soon. Switching this to cron by itself won't work because we assume its going to be run on a PR. Trying to figure out what makes the most sense here. Option 1 is to just not add labels to forks (but we shouldn't throw like we do now regardless). Option 2 is to update it to filter through all pull requests.
In theory I like option 2, but we need to be careful or we'll get rate limited - along those lines, we need a way of skipping PRs that we've already processed - maybe we could add a "triaged" label or something, but that's kind of ugly.
Thoughts? My instinct is to start with option 1 - better error handling on forks - and then move on to option 2 as appropriate
@damccorm option 2 certainly sounds far more useful for us - our workflow has every developer on our project making PRs from their forks (and presumably that's a fairly common workflow).
Option 1 makes this action useless, as whoever who has write access can put the labels themselves as necessary, though in a repo with 90 percent or more contribution through forks, we really wanted to use this action to reduce manual work.
Why can't you just use the default branches labeler.yml as canonical config?
Yeah, agreed option 2 makes a lot more sense.
Why can't you just use the default branches labeler.yml as canonical config?
The issue isn't getting the labeler.yml
, the issue here is that workflows that run on forks don't have write permissions (so they can't create labels)
Could we change the config to use the repo's token, rather than the fork's token? If this works conceptually, we (exercism pps) could experiment more.
Hello @damccorm, Is there any way or workaround to trigger the action on the base repository so that the GitHub Action token will have both read/write access when there is a PR from the forked repository. And there is no need to re-write the whole code base.
What is the end solution?
We've enabled this for our repository too and are facing the same issue. Would be good to at least define some way to handle this in the workflow, so we can customize the logic. Of course the best option would be to support what was already suggested by @ibakshay:
...so that the GitHub Action token will have both read/write access when there is a PR from the forked repository. And there is no need to re-write the whole code base.
Hello @damccorm, Is there any way or workaround to trigger the action on the base repository so that the GitHub Action token will have both read/write access when there is a PR from the forked repository. And there is no need to re-write the whole code base.
Workflows are run in the context "of the repository". But when any workflow is run because a pull request triggered it, then the token is downgraded to a read-only token.
It's not "the fork's token", it's still the repository's token, it's just read-only.
This is intentional. It's so that somebody can't open a pull request that gives them a token that they could use to write to your repository.
We'll continue to investigate workarounds, but the security model here is important to keep in mind.
@ethomson Thank you very much for the detailed response. I totally agree that read/write access should not be given for the PR to all the actions for security reasons.
My proposal would be to have a flag for the repository admin to enable read/write access only to the trusted actions. This issue is raised multiple times in the GitHub Community Forum.
My proposal would be to have a flag for the repository admin to enable read/write access only to the trusted actions.
I'm completely okay with forks having the ability to apply labels and no ability to do anything else like create labels or any other write action. If someone abuses applying labels then I'll ban them.
Same problem when you try to create a comment to an issue. Which shouldn't be a problem, since anyone can do that, right?
I'm completely okay with forks having the ability to apply labels and no ability to do anything else like create labels or any other write action. If someone abuses applying labels then I'll ban them.
Indeed. We'll take a closer look at this for sure; I just wanted to explain why it's not trivial and something that we want to make sure that we get right when we do.
I'm completely okay with forks having the ability to apply labels and no ability to do anything else like create labels or any other write action. If someone abuses applying labels then I'll ban them.
And also I don't mind if the forks have the ability to create a comment on PR or issue with trusted actions
So, to be clear, if I open a PR on a repo where I am not an admin, then the Labeler action should fail?
@logankilpatrick totally correct.
Any workarounds here?
So, to be clear, if I open a PR on a repo where I am not an admin, then the Labeler action should fail?
No, it's not about your permissions. The GITHUB_TOKEN has a writable token for the repository. It's about whether you're opening a PR from a fork, or whether you're opening a PR directly in the repository.
The PR from a fork does not have a writable token and will fail. The PR from a repository will succeed since the GITHUB_TOKEN has write permissions.
@ethomson wouldn't it be possible for the token to have the same permissions as the person who initiates the action? That would prevent labels to be created anyway, but comments, for instance, shouldn't be a problem, since any one has got permission to create them.
@smay1613 surprisingly, you can grab your GitHub token and create an external request using curl
, for instance, like this answer points out.
@ethomson wouldn't it be possible for the token to have the same permissions as the person who initiates the action? That would prevent labels to be created anyway, but comments, for instance, shouldn't be a problem, since any one has got permission to create them.
We'll make some changes to this to make it easier to use, but given the security impact, we're analyzing this carefully.
@smay1613 surprisingly, you can grab your GitHub token and create an external request using curl, for instance, like this answer points out.
The token is still read-only, no matter how you use it. Its ability to be exfiltrated is why we've made it read-only for forks.
Wouldn’t all PR’s be off forks since if you don’t have wrote access, be default it will make a forked branch?
Best, Logan Kilpatrick
On Nov 5, 2019, at 11:52 AM, Edward Thomson notifications@github.com wrote:
So, to be clear, if I open a PR on a repo where I am not an admin, then the Labeler action should fail?
No, it's not about your permissions. The GITHUB_TOKEN has a writable token for the repository. It's about whether you're opening a PR from a fork, or whether you're opening a PR directly in the repository.
The PR from a fork does not have a writable token and will fail. The PR from a repository will succeed since the GITHUB_TOKEN has write permissions.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.
@ethomson is this token not persistent? So, hardcoding the read/write token would not work too?
The current workaround would be to use GitHub Apps or Probot.
Since this action doesn't work for a lot of users including me, I created some sort of alternative at https://github.com/marketplace/actions/periodic-labeler. This isn't anything fancy and the workaround is to just run it periodically from master branch. Configuration is the same as in action located here, so migration should be simple. I am already using it in @cloudalchemy org.
Since this action doesn't work for a lot of users including me, I created some sort of alternative at https://github.com/marketplace/actions/periodic-labeler. This isn't anything fancy and the workaround is to just run it periodically from master branch. Configuration is the same as in action located here, so migration should be simple. I am already using it in @cloudalchemy org.
We tried to use the periodic-labeler at https://github.com/apache/airflow and it did not work. After some investigation we found that the pagination is missing in the implementation and that labeler will run only for the oldest 30 open PRs. This might get very confusing because it seems it works, but there are no visible changes in labels (old PRs are on further pages if you have a lot of them). Fix to it waits for merging - it also contains all the explanation: https://github.com/paulfantom/periodic-labeler/pull/4
I have created a Github app (based on Probot) that takes similar configurations and does the same job and works for Forked PRs too.
More information: https://github.com/kaxil/boring-cyborg App Page: https://github.com/apps/boring-cyborg
We are already using it on Apache Airflow repo.
My issue is that the starter workflow defined works perfectly for pull requests. However, if one pushes a minor change directly to master - the labeler runs and fails with the exact same error. Why would on: [pull_request]
fire for a push to master?
The issue isn't getting the labeler.yml, the issue here is that workflows that run on forks don't have write permissions (so they can't create labels)
I don't understand this... the labels exist in the main (non forked) repo, which is exactly where I want the labels. Am I missing something? I'm not seeing the issue.
I don't understand this... the labels exist in the main (non forked) repo, which is exactly where I want the labels. Am I missing something? I'm not seeing the issue.
When a workflow runs on a pull request event, the workflow run gets a read only token to the main repository. This is so that somebody can’t fork your repository and maliciously write to your code, create harmful issues or exfiltrate tokens. Or - I’m afraid - add labels to issues. The token is explicitly read only for security.
We’re considering some other alternatives here, but at the moment, this will not work with PRs from forks.
When a workflow runs on a pull request event, the workflow run gets a read only token to the main repository.
So under what circumstances does this actually work? If the token is always read-only how does it EVER do what its' supposed to do - add labels?
So under what circumstances does this actually work? If the token is always read-only how does it EVER do what its' supposed to do - add labels?
When a pull request comes from a non-fork, as may happen within organizations.
With https://github.com/actions/labeler/pull/49 this action should work when scheduling, it must be committed to the master at the destination already.
harmful issues or exfiltrate tokens. Or - I’m afraid - add labels to issues. The token is explicitly read only for security.
So unless I'm misunderstanding, this action doesn't work for the 90% the most common (fork, branch, push to PR) github workflow :confounded:
So unless I'm misunderstanding, this action doesn't work for the 90% the most common (fork, branch, push to PR) github workflow 😖
Yeah, believe me you're not the only one having a solid WTF moment.
I don't understand why there's a WTF moment? This action was created trying to implement exactly this functionality but it's currently not possible because of how tokens work in GitHub Actions. There's absolutely nothing hard about this to understand^^
Thanks everybody for the commentary, I appreciate that this isn't helpful for many of your workflows. We're investigating some changes to token permissions to help enable fork-based workflows, but I don't have an ETA on that.
@Toflar The problem is this is a SUPER common use case - I assumed the whole reason this workflow was listed so prominently was FOR this exact use case - and the docs say nothing to the contrary. Can we please merge #50? That would have saved me like an hour of time.
@ethomson
What about using Pull Request Workflow from the target branch and not from the fork? It'd fix most security issues
I mean ignore version of the workflow file from the fork and use one from target branch instead
A common way to update a workflow is through the pull request process. You'd want to see how your workflow works in the PR process, not have to wait until it was merged to master. You want pull request validation on the workflow itself, in this case.
But also, like you said, it's also not sufficient. It wouldn't catch all security issues. It would be easy enough for me to add a new test that is print "$env['SOME_SECRET']"
and that would be no good.
I'd be fine with a labeler action that ran on the branch periodically for issues and/or PRs. It'd be nice to be able to test the workflow from a PR, but I could live without that for now (until it's sorted out). I'm sure other users would also really (REALLY) appreciate updates to the docs, indicating that commonly used PR-based actions cannot affect labels.
Suggestion for the future:
What about when running on a PR, have the action operate in a kind of "test mode". In this mode, instead of updating labels directly, it could simply posts comments to the triggering PR indicating the labels it would have updated. Then once merged, a periodic branch-based issue/PR labeling operations can happen as is currently supported.
@ethomson what you said is all correct. But let's try a possible solution
A common way to update a workflow is through the pull request process. You'd want to see how your workflow works in the PR process, not have to wait until it was merged to master. You want pull request validation on the workflow itself, in this case.
One who wants updates workflow normally has write access to the repository, so he can make a pull request from the same repo.
But also, like you said, it's also not sufficient. It wouldn't catch all security issues. It would be easy enough for me to add a new test that is print "$env['SOME_SECRET']" and that would be no good.
If workflow would uses a version from the target branch, then as I said it eliminates most security concerns. But in some cases, a malicious person can hack the system by changing other files in repo (e.g. when workflow calls other scripts from the repo). A solution for this scenario could be an option in settings to restrict access in a forked pull request (i.e. as it works now) or adding new setting to specify which files have to be secured and modified version of those files must not be used in workflows of forked pull requests
@ethomson, I have a possible solution proposal. How about having trusted Actions ?. Where the project maintainer will have the possibility to give write access to only trusted actions and can turn it off anytime If he/she feels the action is suspicious.
Hi there,
I've implemented the exact Labeler workflow as the starter-workflow template in the dir path
.github/workflows/label.yml
.I have also added the
.github/labeler.yml
file with the configuration below:The action is recognised and runs on PR, however the Labeler action resolves with the following error:
Googling seems to relate that error with invalid access to the repo by the action app, so I thought I'd create an issue and disable the action for now, but would be happy to know if there's something I have missed out that will be able to solve this issue for me.
Thanks.