-
Notifications
You must be signed in to change notification settings - Fork 474
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
Suggestion: Change Branch-Protection check to consider rule of "change only through PRs" #2727
Comments
Is there any opposition to this? If not, feel free to assign me to it |
Overall, I could see this being some sort of partial credit. I've left some comments inline.
I think "Require branch to pass at least 1 status check before merging" also captures this to some extent. Of course this has the issue of tiers (which you raised in #3123).
I think the code review is a much more important bit, but I could see requiring a PR to possibly be worth a single point.
This could also be part of the tier 1 if #3123 is changed. |
One important detail that I discovered:
, but if I run without the admin token, for the exact same repository without changing the configs, I get
, which is the exact same result that we get when the repository requires PRs, but doesn't require a reviewer. This way, we could not differentiate a repository that does not requires PRs to a repository that requires PRs but doesn't require another reviewer. Obs: those values were gotten inside the run of Scorecard as values available on the EDIT: I was able to verify directly using the GitHub's API and the problem still applies. To test, I used the GraphQL explorer and ran the following query:
In any ways, I think this feature is still important, especially thinking that after repo rules support (#3326) all branch protection configuration should be public. |
After private discussions with @spencerschrock , I've came up with this idea on how to add this rule on the current Branch Protection score: We'd add this as another Tier-2 requirement, but as the "Require 1 reviewer" has a stronger security impact, it'll have a bigger weight over the other Tier-2 requirements. On the current implementation of this check we compute the score of each tier using the concept of a Currently the Tier 2 requirements are like this:
My idea of implementation would change this to:
Analysis on use casesRunning without admin permissions:
Running with admin permissions (or after Scorecard support for Rule Set #3326):
|
…ke changes As discussed at the issue ossf#2727, we're adding the "require PRs prior to make changes" as another requirement to tier 2. In addition to that, we're changing the weight of the tier 2 requirements so that "requiring 1 reviewer" has weight 2, while the other tier 2 requirements have weight 1
…ke changes As discussed at the issue ossf#2727, we're adding the "require PRs prior to make changes" as another requirement to tier 2. In addition to that, we're changing the weight of the tier 2 requirements so that "requiring 1 reviewer" has weight 2, while the other tier 2 requirements have weight 1
…ke changes As discussed at the issue ossf#2727, we're adding the "require PRs prior to make changes" as another requirement to tier 2. In addition to that, we're changing the weight of the tier 2 requirements so that "requiring 1 reviewer" has weight 2, while the other tier 2 requirements have weight 1 Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>
…ke changes As discussed at the issue ossf#2727, we're adding the "require PRs prior to make changes" as another requirement to tier 2. In addition to that, we're changing the weight of the tier 2 requirements so that "requiring 1 reviewer" has weight 2, while the other tier 2 requirements have weight 1 Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>
This issue is stale because it has been open for 60 days with no activity. |
…ke changes As discussed at the issue ossf#2727, we're adding the "require PRs prior to make changes" as another requirement to tier 2. In addition to that, we're changing the weight of the tier 2 requirements so that "requiring 1 reviewer" has weight 2, while the other tier 2 requirements have weight 1 Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>
…ke changes As discussed at the issue ossf#2727, we're adding the "require PRs prior to make changes" as another requirement to tier 2. In addition to that, we're changing the weight of the tier 2 requirements so that "requiring 1 reviewer" has weight 2, while the other tier 2 requirements have weight 1 Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>
* feat(branch-protection): consider if project requires PRs prior to make changes As discussed at the issue #2727, we're adding the "require PRs prior to make changes" as another requirement to tier 2. In addition to that, we're changing the weight of the tier 2 requirements so that "requiring 1 reviewer" has weight 2, while the other tier 2 requirements have weight 1 Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com> * test(branch-protection): increment and adapt testing 1. Adapt previous test cases to consider that now we'll have an aditional Info log telling that the project requires PRs to make changes. 2. Add more cases to test relevant use cases on the tier 2 level of branch protection Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com> * docs(branch-protection-check): adapt check description to consider requirement of require PRs to make changes It adds the new tier 2 requirement, but also specify that the "require at least 1 reviewer" will have doubled weight. Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com> * refactor(branch-protection-check): avoid duplicate funcions and enhance readability Made some nice-to-have improvements on project readability, making it easier easier to understand how the branch-protection score is computed. Also unified 8 different functions that were doing basically the same thing. Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com> * feat(branch-protection): standardize values received on evaluation Previously, at the evaluation part of branch protetion, the values nil and false or zero were sort of interchangeble. This commit changes the code to set as nil only the data that could not be retrieved from github -- all the others would have values as false, zero, true, etc Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com> * test(github-client): adapt and add tests to check if nil values are coherent 1. Add new test to evaluate how we're interpreting a rule with all checkboxes unchecked (most shouldn't be nil) 2. Adapt existent tests to expect non-nil values for unchecked checkboxes Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com> * feat(client-github): avoid reusing bool pointers Changes some pieces of code to prefer using pointers of bool instantiated independently. If reusing bool pointers, at some piece of code the value of the bool could inadvertently changed and it would change the value of all other fields reusing that pointer. Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com> * feat(branch-protection): enhance evaluation if scorecard was run by admin At the evaluation step we were using some non untrusted fieldds of the resposte to evaluate if Scorecard was run as admin or not. Now we're using a field provided directly from the client file. Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com> * test(branch-protection): adapt testings to say if they have admin info or not After last commit, the client will tell the evaluation files if Scorecard was run by administrator or not (i.e., if we have all the infos). This commit adapts the testings to also provide this info. Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com> * test(e2e-branch-protection): adapt number of logs after changes - 2 warns (for 'last push approval' and 'codeowners review' disabled) were added because now those informations come as 'not-nil' at the evaluation part. - 1 info was added to say that PRs are required to make changes - 1 debug was removed because it said that we couldn't retrieve 'last push approval' information, but we actually can. It was just incorrectly set as nil Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com> * Revert the 2 commits with changes around how Scorecard detects admin run Reverts commit 64c3521 and commit e2662b7. Both had chances around using clients/branch.go scructur to store the information of whether Scorecard was being run by admin or not. We decided to not change this structure for this purpose. Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com> * refactor(branch-protection): change data structure to use pointer instead of value At clients.BranchProtectionRule struct, changing RequiredPullRequestReviews to be a pointer instead of a struct value. This will allow the usage of the nil value of this structure to mean that we can't say if the repository requires reviews or not. Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com> * feat(branch-protection): use nil pointer on reviewers struct to mean we don't know if they require PRs The nil value of the struct RequiredPullRequestReviews will now mean that we can't tell whether the project requires PRs to make changes or not. When we get this case, we're printing a debug informing that we don't have this data, but also printing a warn saying that they don't require reviews, because that will be true at this case. Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com> * test(branch-protection): if we're setting the reviewers struct to nil when needed Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com> * doc(branch-protection): add code comment explaining different weight on tier 2 scores Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com> * refactor(branch-protection): avoid duplicate if branches on reviewers num comparation Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com> * docs(branch-protection): clarify commentings around data structure Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com> * refactor: clean code on parsing GitHub BP data Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com> * feat(branch-protection): ressignify the nil PullRequestReviewRule to mean PR not required Adapt translation of data from GitHub API, now for our internal data modeling, having a nil PullRequestReviewRule structure will mean that PRs are not required on the repo (can also mean we don't have data to ensure that). It also changes the order of the calls of copyNonAdminSettings and copyAdminSettings to make the first one be called first. This eases the code because the PullRequestReviewRule can be always instantiated at this function. Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com> * test(branch-protection): ensure we translate GitHub BP data as expected Ensure we're correctly translating GitHub data from the old Branch Protection config. Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com> * feat(branch-protection): adapt score evaluation after 2efeee6 Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com> * test(branch-protection): adapt testings to changes of last commits Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com> * docs(branch-protection): add TODO comments pointing refactor opportunities Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com> * fix: avoid penalyzing non-admin for dismissStaleReview Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com> * fix(branch-protection): prevent false value from API field to become nil When translating the API results, if the specific field `DismissesStaleReviews` had a false value, it was not being initiated in our data model and was remaining nil. Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com> * refactor: clarify different weight on first reviewer Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com> * refactor: enhance clarity of loggings and comments Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com> * test(branch-protection): new test to cover different rules affecting same branch Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com> * docs(branch-protection): change requirements ordering to keep admin ones together Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com> * refactor(branch-protection): simplify auxiliary function Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com> * refactor(branch-protection): fix code format to linter requirements Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com> * refactor(branch-protection): avoid unnecessary initializations and rename function Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com> * test(branch-protection): adapt test that was forgotten on commit 6858790 Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com> * refactor(branch-protection): use enums to represent tiers Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com> * refactor(branch-protection): remove nil fields of struct initialization when they dont contribute for clarification Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com> * refactor(branch-protection): simplify functions by using generics Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com> * docs(branch-protection): update docs after generate-docs run Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com> * fix(branch-protection): fix duplicated line on code Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com> * fix(branch-protection): stop exporting Tier enum Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com> * refactor(branch-protection): changing unchanged var to const Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com> * test(branch-protection): Rename test and adapt it to be consistent with its purpose I also changed the test to not require PRs, as it's how it is when a new GitHub Branch Protection config is created. The changes on the loggings numbers are due to: 1. A warning for not having DismissStaleReviews became a debug 2. Removed the warning we had for not requiring CodeOwners 3. Have a new warning for not requiring PRe Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com> --------- Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>
Is your feature request related to a problem? Please describe.
The current Branch Protection check does not consider the rule named
Require a pull request before merging
(you can check it on the GitHub page). This rule is actually very important because it forces any change on the branch to be made through PRs, and would therefore prevent malicious pushes -- that could even be made through actions with write permissions.The closest to this we currently have is the requirement of
Reviewers >= 1
to get a 6/10 score. But this might be specially difficult to solo-devs, while theRequire a pull request before merging
is possible even for a single maintainer.Describe the solution you'd like
The purpose is to make this rule enhance the Branch-Protection rule somehow. My suggestion would be to add this rule as a new tier with a 5/10 punctuation. This would give a score higher than the 3/10, which basically requires prevention of force pushes and branch deletion, and slightly lower than 6/10, which requires a reviewer other than the commiter.
Describe alternatives you've considered
The text was updated successfully, but these errors were encountered: