-
Notifications
You must be signed in to change notification settings - Fork 2
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
Potential feature/change: Have disable_forks
default as True
#257
Comments
Hi, I don't suppose you have any thoughts on the above please? Happy to help implement them with a PR but just keen to know what direction would be good and if it would be supported? Thanks! |
Unfortunately, there is no hope that you get any further support here (#246 was 2+y ago). But recent news is that the Cloud Foundry community is now maintaining a fork of this resource at cloudfoundry-community/github-pr-resource. On our Concourse installation at Gstack, that one is working fine. Could you give a try and provide feedback there? (the new resource is a drop-in replacement; you only have to switch |
Context
At the moment you can configure the resource and set
disable_forks
on it where it will disable triggering of the resource if the pull request's fork repository is different to the configured repository.If you do not set this key then it will default to
false
.Was added in #66
Problem
If you do not want forks to run then you must remember to disable it for every resource.
I've found myself having to go through every pipeline we have recently to make sure we hadn't forgotten this key. It's very easy to forget a key, especially if you didn't read the docs properly or didn't even know that you needed to be aware of the problems of PRs from forks.
What could the implications be of missing one of these?
Proposal
I think it is much safer for users of the resource to have to opt in to allowing forks rather than having that be the default case where they need to know/remember to opt out. By default this resource should promote safe usage and have people explicitly opt into things that they need to think about from a security point of view.
Some potential ways that might be able to do this (from someone who knows very little about Go):
disable_forks
to be a required value so you must explicit choose to allow forks (is this possible in Go?)disable_forks
to be trueenable_forks
key with default valuefalse
instead and deprecatedisable_forks
I hope this is useful. Very happy to hear counter arguments. One downside would be that this would be a breaking change.
The text was updated successfully, but these errors were encountered: