BUG number of required reviewers is only 0 alert even though is set to 1

[godofredoc]

Describe the bug
flutter/flutter has branch protection enabled and required reviewers to 1 but scorecards still show an alert with a description that is set to 0

Reproduction steps
Steps to reproduce the behavior:

1. Set branch protection of a repository
2. Set required reviewers to 1, add a group that is exempt from the rule(this is used for bot accounts)
3. Run scorecards

Expected behavior
If reviewers is set to >1 the alert should not be triggered

Additional context
The token we are using to run scorecards with doesn't have admin access. I wonder if this a permissions problem.

[laurentsimon]

Thanks for reporting!

Can you give a link about the add a group that is exempt from the rule setting?

I ran scorecard and got the following (with my personal token):

{
      "details": [
        "Info: 'force pushes' disabled on branch 'master'",
        "Info: 'allow deletion' disabled on branch 'master'",
        "Info: status check found to merge onto on branch 'master'",
        "Warn: number of required reviewers is only 1 on branch 'master'"
      ],
      "score": 8,

Do you see something different in your results/dashboard?

[laurentsimon]

follow-up: you're right, your latest action run has:
branch protection is not maximal on development and all release branches: Warn: number of required reviewers is only 0 on branch 'master'

Can you run scorecard yourself with your token via command line using our docker image as explained in https://github.com/ossf/scorecard#docker and copy the result here? docker run -e GITHUB_AUTH_TOKEN=token gcr.io/openssf/scorecard:stable --show-details --repo=https://github.com/flutter/flutter

Additionally, please run docker run -e SCORECARD_V6=1 -e GITHUB_AUTH_TOKEN=token gcr.io/openssf/scorecard:stable --show-details --repo=https://github.com/flutter/flutter --format raw and past the results

Fyi, I get the following with my own token

{
        "protection": {
          "required-reviewer-count": 1,
          "allows-deletions": false,
          "allows-force-pushes": false,
          "requires-code-owner-review": false,
          "required-linear-history": true,
          "dismisses-stale-reviews": null,
          "enforces-admin": null,
          "requires-status-checks": null,
          "requires-updated-branches-to-merge": null,
          "status-checks-contexts": [
            "ci.yaml validation"
          ]
        },
        "name": "master"

Thanks for your help!

[godofredoc]

Output of docker run -e GITHUB_AUTH_TOKEN=token gcr.io/openssf/scorecard:stable --show-details --repo=https://github.com/flutter/flutter :

-------|-----------------------------------------------------------------------------------------------------------------------|
| 3 / 10  | Branch-Protection      | branch protection is not       | Info: 'force pushes' disabled                                                                                                                    | https://github.com/ossf/scorecard/blob/38be00c31f4f078120219ec3aaa76560835199fb/docs/checks.md#branch-protection      |
|         |                        | maximal on development and all | on branch 'master' Info:                                                                                                                         |                                                                                                                       |
|         |                        | release branches               | 'allow deletion' disabled on                                                                                                                     |                                                                                                                       |
|         |                        |                                | branch 'master' Info: status                                                                                                                     |                                                                                                                       |
|         |                        |                                | check found to merge onto on                                                                                                                     |                                                                                                                       |
|         |                        |                                | branch 'master' Warn: number                                                                                                                     |                                                                                                                       |
|         |                        |                                | of required reviewers is only                                                                                                                    |                                                                                                                       |
|         |                        |                                | 0 on branch 'master'                                                                                                                             |                                                                                                                       |

Output of docker run -e SCORECARD_V6=1 -e GITHUB_AUTH_TOKEN=token gcr.io/openssf/scorecard:stable --show-details --repo=https://github.com/flutter/flutter --format raw:

"branch-protections": [
            {
                "protection": {
                    "required-reviewer-count": 0,
                    "allows-deletions": false,
                    "allows-force-pushes": false,
                    "requires-code-owner-review": false,
                    "required-linear-history": true,
                    "dismisses-stale-reviews": null,
                    "enforces-admin": null,
                    "requires-status-checks": null,
                    "requires-updated-branches-to-merge": null,
                    "status-checks-contexts": [
                        "ci.yaml validation"
                    ]
                },
                "name": "master"
            }
        ],

This other option is checked in that group:

[laurentsimon]

Very interesting!

Which scopes do you use for your PAT?

[godofredoc]

It has the following scopes: public_repo, read:discussion, read:org, read:repo_hook

[laurentsimon]

I could not reproduce the problem with my PAT. Let's try something else first. Can you head over to https://docs.github.com/en/graphql/overview/explorer, log in as you, and run this graphQl command and paste your results?

{
  repository(owner: "flutter", name: "flutter") {
    defaultBranchRef{
      name
      branchProtectionRule{
        requiredApprovingReviewCount
      }
      refUpdateRule{
        requiredApprovingReviewCount
      }
    }
    refs(first: 100, refPrefix: "refs/heads/", query: "master") {
      nodes {
        name
        refUpdateRule {
          requiredApprovingReviewCount
        }
      }
    }
  }
}

Mines look like:

{
  "data": {
    "repository": {
      "defaultBranchRef": {
        "name": "master",
        "branchProtectionRule": null, // Note: indicate I don't have admin access
        "refUpdateRule": {
          "requiredApprovingReviewCount": 1
        }
      },
      "refs": {
        "nodes": [
          {
            "name": "master",
            "refUpdateRule": {
              "requiredApprovingReviewCount": 1
            }
          }
        ]
      }
    }
  }
}

[godofredoc]

I think I know what is happening:

The PAT was generated from an account which belongs to the group allowed to bypass pullrequest requirements. This is the output:

{
  "data": {
    "repository": {
      "defaultBranchRef": {
        "name": "master",
        "branchProtectionRule": null,
        "refUpdateRule": {
          "requiredApprovingReviewCount": 0
        }
      },
      "refs": {
        "nodes": [
          {
            "name": "master",
            "refUpdateRule": {
              "requiredApprovingReviewCount": 0
            }
          }
        ]
      }
    }
  }
}

The result of running with an account that does not belong to group allowed to bypass the codereview requirements is:

{
  "data": {
    "repository": {
      "defaultBranchRef": {
        "name": "master",
        "branchProtectionRule": null,
        "refUpdateRule": {
          "requiredApprovingReviewCount": 1
        }
      },
      "refs": {
        "nodes": [
          {
            "name": "master",
            "refUpdateRule": {
              "requiredApprovingReviewCount": 1
            }
          }
        ]
      }
    }
  }
}

I thought the APIs were returning the data in the same structure as the UI but it seems like it returns them calculated for the user making the request.

I believe the fix for this issue is to create the PAT using an account that does not belong to any special group and maybe adding a note to the documentation. E.g. for flutter we want to validate the permissions from the flutter-hackers point of view.

[laurentsimon]

Good catch. That's a really interesting behavior. @azeemsgoogle @jeffmendoza is this expected behavior?

I think we also need to retrieve the settings "Allowed specified actors..." so it can be reported and validated, otherwise we are missing important info about the repo itself.

[azeemshaikh38]

Very interesting find, thanks @godofredoc! I think we need help from the GitHub team here. Will follow up with them and update here.