diff --git a/README.md b/README.md index c7f9b62b848..be4da7e337a 100644 --- a/README.md +++ b/README.md @@ -439,7 +439,7 @@ Name | Description | Risk Level | Token Req [Branch-Protection](docs/checks.md#branch-protection) | Does the project use [Branch Protection](https://docs.github.com/en/free-pro-team@latest/github/administering-a-repository/about-protected-branches) ? | High | PAT (`repo` or `repo> public_repo`), GITHUB_TOKEN | certain settings are only supported with a maintainer PAT [CI-Tests](docs/checks.md#ci-tests) | Does the project run tests in CI, e.g. [GitHub Actions](https://docs.github.com/en/free-pro-team@latest/actions), [Prow](https://github.com/kubernetes/test-infra/tree/master/prow)? | Low | PAT, GITHUB_TOKEN | [CII-Best-Practices](docs/checks.md#cii-best-practices) | Does the project have an [OpenSSF (formerly CII) Best Practices Badge](https://bestpractices.coreinfrastructure.org/en)? | Low | PAT, GITHUB_TOKEN | -[Code-Review](docs/checks.md#code-review) | Does the project require code review before code is merged? | High | PAT, GITHUB_TOKEN | +[Code-Review](docs/checks.md#code-review) | Does the project practice code review before code is merged? | High | PAT, GITHUB_TOKEN | [Contributors](docs/checks.md#contributors) | Does the project have contributors from at least two different organizations? | Low | PAT, GITHUB_TOKEN | [Dangerous-Workflow](docs/checks.md#dangerous-workflow) | Does the project avoid dangerous coding patterns in GitHub Action workflows? | Critical | PAT, GITHUB_TOKEN | [Dependency-Update-Tool](docs/checks.md#dependency-update-tool) | Does the project use tools to help update its dependencies? | High | PAT, GITHUB_TOKEN | diff --git a/checks/code_review_test.go b/checks/code_review_test.go index 1b48c06093f..afc52d84eb5 100644 --- a/checks/code_review_test.go +++ b/checks/code_review_test.go @@ -186,7 +186,7 @@ func TestCodereview(t *testing.T) { }, }, expected: checker.CheckResult{ - Score: 5, + Score: 3, }, }, { @@ -211,7 +211,7 @@ func TestCodereview(t *testing.T) { }, }, expected: checker.CheckResult{ - Score: 5, + Score: 3, }, }, { diff --git a/checks/evaluation/code_review.go b/checks/evaluation/code_review.go index f838d2216c6..524a0fe1fde 100644 --- a/checks/evaluation/code_review.go +++ b/checks/evaluation/code_review.go @@ -21,7 +21,7 @@ import ( sce "github.com/ossf/scorecard/v4/errors" ) -type reviewScore = int +type reviewScore int // TODO(raghavkaul) More partial credit? E.g. approval from non-contributor, discussion liveness, // number of resolved comments, number of approvers (more eyes on a project). @@ -42,18 +42,57 @@ func CodeReview(name string, dl checker.DetailLogger, r *checker.CodeReviewData) return checker.CreateInconclusiveResult(name, "no commits found") } - numReviewed := 0 + foundHumanReviewActivity := false + foundBotReviewActivity := false + nUnreviewedHumanChanges := 0 + nUnreviewedBotChanges := 0 for i := range r.DefaultBranchChangesets { - score := reviewScoreForChangeset(&r.DefaultBranchChangesets[i]) - if score >= changesReviewed { - numReviewed += 1 + cs := &r.DefaultBranchChangesets[i] + isReviewed := reviewScoreForChangeset(cs) >= changesReviewed + isBotCommit := cs.Author.IsBot + + switch { + case isReviewed && isBotCommit: + foundBotReviewActivity = true + case isReviewed && !isBotCommit: + foundHumanReviewActivity = true + case !isReviewed && isBotCommit: + nUnreviewedBotChanges += 1 + case !isReviewed && !isBotCommit: + nUnreviewedHumanChanges += 1 + } + } + + if foundBotReviewActivity && !foundHumanReviewActivity { + reason := fmt.Sprintf("found no human review activity in the last %v changesets", len(r.DefaultBranchChangesets)) + return checker.CreateInconclusiveResult(name, reason) + } + + switch { + case nUnreviewedBotChanges > 0 && nUnreviewedHumanChanges > 0: + return checker.CreateMinScoreResult( + name, + fmt.Sprintf("found unreviewed changesets (%v human %v bot)", nUnreviewedHumanChanges, nUnreviewedBotChanges), + ) + case nUnreviewedBotChanges > 0: + return checker.CreateResultWithScore( + name, + fmt.Sprintf("all human changesets reviewed, found %v unreviewed bot changesets", nUnreviewedBotChanges), + 7, + ) + case nUnreviewedHumanChanges > 0: + score := 3 + if len(r.DefaultBranchChangesets) == 1 || nUnreviewedHumanChanges > 1 { + score = 0 } + return checker.CreateResultWithScore( + name, + fmt.Sprintf("found %v unreviewed human changesets", nUnreviewedHumanChanges), + score, + ) } - reason := fmt.Sprintf( - "%v out of last %v changesets reviewed before merge", numReviewed, len(r.DefaultBranchChangesets), - ) - return checker.CreateProportionalScoreResult(name, reason, numReviewed, len(r.DefaultBranchChangesets)) + return checker.CreateMaxScoreResult(name, "all changesets reviewed") } func reviewScoreForChangeset(changeset *checker.Changeset) (score reviewScore) { diff --git a/checks/evaluation/code_review_test.go b/checks/evaluation/code_review_test.go index 794a6823086..0d6e136dda0 100644 --- a/checks/evaluation/code_review_test.go +++ b/checks/evaluation/code_review_test.go @@ -55,10 +55,47 @@ func TestCodeReview(t *testing.T) { rawData: &checker.CodeReviewData{ DefaultBranchChangesets: []checker.Changeset{ { - Commits: []clients.Commit{{SHA: "1"}}, + Commits: []clients.Commit{{SHA: "a"}}, }, { - Commits: []clients.Commit{{SHA: "1"}}, + Commits: []clients.Commit{{SHA: "a"}}, + }, + }, + }, + }, + { + name: "all human changesets reviewed, missing review on bot changeset", + expected: scut.TestReturn{ + Score: 7, + }, + rawData: &checker.CodeReviewData{ + DefaultBranchChangesets: []checker.Changeset{ + { + Author: clients.User{Login: "alice"}, + ReviewPlatform: checker.ReviewPlatformGitHub, + RevisionID: "1", + Reviews: []clients.Review{ + { + Author: &clients.User{}, + State: "APPROVED", + }, + }, + Commits: []clients.Commit{ + { + Committer: clients.User{Login: "bob"}, + SHA: "b", + }, + }, + }, + { + Author: clients.User{Login: "alice-the-bot[bot]", IsBot: true}, + RevisionID: "b", + Commits: []clients.Commit{ + { + Committer: clients.User{Login: "alice-the-bot[bot]", IsBot: true}, + SHA: "b", + }, + }, }, }, }, @@ -80,10 +117,54 @@ func TestCodeReview(t *testing.T) { State: "APPROVED", }, }, + Commits: []clients.Commit{ + { + Committer: clients.User{Login: "bob"}, + SHA: "b", + }, + }, }, }, }, }, + + { + name: "bot commits only", + expected: scut.TestReturn{ + Score: checker.InconclusiveResultScore, + }, + rawData: &checker.CodeReviewData{ + DefaultBranchChangesets: []checker.Changeset{ + { + Author: clients.User{Login: "alice-the-bot[bot]", IsBot: true}, + ReviewPlatform: checker.ReviewPlatformGitHub, + RevisionID: "1", + Reviews: []clients.Review{ + { + Author: &clients.User{}, + State: "APPROVED", + }, + }, + Commits: []clients.Commit{ + { + Committer: clients.User{Login: "alice-the-bot[bot]", IsBot: true}, + SHA: "b", + }, + }, + }, + { + RevisionID: "b", + Commits: []clients.Commit{ + { + Committer: clients.User{Login: "alice-the-bot[bot]", IsBot: true}, + SHA: "b", + }, + }, + }, + }, + }, + }, + { name: "all changesets reviewed outside github", expected: scut.TestReturn{ @@ -94,7 +175,7 @@ func TestCodeReview(t *testing.T) { { ReviewPlatform: checker.ReviewPlatformGerrit, RevisionID: "1", - Commits: []clients.Commit{{SHA: "1"}}, + Commits: []clients.Commit{{SHA: "a"}}, }, }, }, diff --git a/clients/githubrepo/graphql.go b/clients/githubrepo/graphql.go index 073091523a6..784413fadfb 100644 --- a/clients/githubrepo/graphql.go +++ b/clients/githubrepo/graphql.go @@ -75,7 +75,8 @@ type graphqlData struct { } } Author struct { - Login githubv4.String + Login githubv4.String + ResourcePath githubv4.String } Number githubv4.Int HeadRefOid githubv4.String @@ -396,12 +397,14 @@ func commitsFrom(data *graphqlData, repoOwner, repoName string) ([]clients.Commi string(pr.Repository.Name) != repoName { continue } + openedByBot := strings.HasPrefix(string(pr.Author.ResourcePath), "/apps/") associatedPR = clients.PullRequest{ Number: int(pr.Number), HeadSHA: string(pr.HeadRefOid), MergedAt: pr.MergedAt.Time, Author: clients.User{ Login: string(pr.Author.Login), + IsBot: openedByBot, }, MergedBy: clients.User{ Login: string(pr.MergedBy.Login), diff --git a/clients/gitlabrepo/contributors.go b/clients/gitlabrepo/contributors.go index e695e35d8b2..0b36594790c 100644 --- a/clients/gitlabrepo/contributors.go +++ b/clients/gitlabrepo/contributors.go @@ -77,6 +77,7 @@ func (handler *contributorsHandler) setup() error { Companies: []string{users[0].Organization}, NumContributions: contrib.Commits, ID: int64(users[0].ID), + IsBot: users[0].Bot, } handler.contributors = append(handler.contributors, contributor) } diff --git a/clients/user.go b/clients/user.go index 60166f270d9..ffbd2ce5805 100644 --- a/clients/user.go +++ b/clients/user.go @@ -21,6 +21,7 @@ type User struct { Organizations []User NumContributions int ID int64 + IsBot bool } // RepoAssociation is how a user is associated with a repository. diff --git a/docs/checks.md b/docs/checks.md index 76ba275b10b..ae885e1b843 100644 --- a/docs/checks.md +++ b/docs/checks.md @@ -207,13 +207,13 @@ malicious code (either as a malicious contributor or as an attacker who has subverted a contributor's account), because a reviewer might detect the subversion. -The check first tries to detect whether [Branch-Protection](checks.md#branch-protection) is enabled on the -default branch with at least one required reviewer. If this fails, the check -determines whether the most recent (~30) commits have a Github-approved review +The check determines whether the most recent (~30) commits have a Github-approved review or if the merger is different from the committer (implicit review). It also performs a similar check for reviews using [Prow](https://github.com/kubernetes/test-infra/tree/master/prow#readme) (labels "lgtm" or "approved") and [Gerrit](https://www.gerritcodereview.com/) ("Reviewed-on" and "Reviewed-by"). +If recent changes are solely bot activity (e.g. dependabot, renovatebot, or custom bots), +the check returns inconclusively. Note: Requiring reviews for all changes is infeasible for some projects, such as those with only one active participant. Even a project with multiple active diff --git a/docs/checks/internal/checks.yaml b/docs/checks/internal/checks.yaml index 63c0f97ee34..9b1c8422554 100644 --- a/docs/checks/internal/checks.yaml +++ b/docs/checks/internal/checks.yaml @@ -307,13 +307,13 @@ checks: subverted a contributor's account), because a reviewer might detect the subversion. - The check first tries to detect whether [Branch-Protection](checks.md#branch-protection) is enabled on the - default branch with at least one required reviewer. If this fails, the check - determines whether the most recent (~30) commits have a Github-approved review + The check determines whether the most recent (~30) commits have a Github-approved review or if the merger is different from the committer (implicit review). It also performs a similar check for reviews using [Prow](https://github.com/kubernetes/test-infra/tree/master/prow#readme) (labels "lgtm" or "approved") and [Gerrit](https://www.gerritcodereview.com/) ("Reviewed-on" and "Reviewed-by"). + If recent changes are solely bot activity (e.g. dependabot, renovatebot, or custom bots), + the check returns inconclusively. Note: Requiring reviews for all changes is infeasible for some projects, such as those with only one active participant. Even a project with multiple active diff --git a/e2e/code_review_test.go b/e2e/code_review_test.go index b4aeee7225b..8cf18ba243f 100644 --- a/e2e/code_review_test.go +++ b/e2e/code_review_test.go @@ -104,5 +104,47 @@ var _ = Describe("E2E TEST:"+checks.CheckCodeReview, func() { Expect(repoClient.Close()).Should(BeNil()) }) + It("Should return inconclusive results for a single-maintainer project with only self- or bot changesets", func() { + dl := scut.TestDetailLogger{} + repo, err := githubrepo.MakeGithubRepo("Kromey/fast_poisson") + Expect(err).Should(BeNil()) + repoClient := githubrepo.CreateGithubRepoClient(context.Background(), logger) + err = repoClient.InitRepo(repo, "bb7b9606690c2b386dc9e2cbe0216d389ed1f078", 0) + Expect(err).Should(BeNil()) + + req := checker.CheckRequest{ + Ctx: context.Background(), + RepoClient: repoClient, + Repo: repo, + Dlogger: &dl, + } + expected := scut.TestReturn{ + Score: checker.InconclusiveResultScore, + } + result := checks.CodeReview(&req) + Expect(scut.ValidateTestReturn(nil, "use code reviews", &expected, &result, &dl)).Should(BeTrue()) + Expect(repoClient.Close()).Should(BeNil()) + }) + It("Should return minimum score for a single-maintainer project with some unreviewed human changesets", func() { + dl := scut.TestDetailLogger{} + repo, err := githubrepo.MakeGithubRepo("Kromey/fast_poisson") + Expect(err).Should(BeNil()) + repoClient := githubrepo.CreateGithubRepoClient(context.Background(), logger) + err = repoClient.InitRepo(repo, clients.HeadSHA, 0) + Expect(err).Should(BeNil()) + + req := checker.CheckRequest{ + Ctx: context.Background(), + RepoClient: repoClient, + Repo: repo, + Dlogger: &dl, + } + expected := scut.TestReturn{ + Score: checker.MinResultScore, + } + result := checks.CodeReview(&req) + Expect(scut.ValidateTestReturn(nil, "use code reviews", &expected, &result, &dl)).Should(BeTrue()) + Expect(repoClient.Close()).Should(BeNil()) + }) }) }) diff --git a/pkg/json_raw_results.go b/pkg/json_raw_results.go index a9b2cb48e3d..216588ea24b 100644 --- a/pkg/json_raw_results.go +++ b/pkg/json_raw_results.go @@ -549,33 +549,20 @@ func (r *jsonScorecardRawResult) setDefaultCommitData(changesets []checker.Chang } reviews := []jsonReview{} - for j := range cs.Commits { - mr := cs.Commits[j].AssociatedMergeRequest - if mr.Reviews == nil { - continue - } - for k := range mr.Reviews { - r := mr.Reviews[k] - reviews = append(reviews, jsonReview{ - State: r.State, - Reviewer: jsonUser{ - Login: r.Author.Login, - }, - }) - } + for j := range cs.Reviews { + r := cs.Reviews[j] + reviews = append(reviews, jsonReview{ + State: r.State, + Reviewer: jsonUser{ + Login: r.Author.Login, + }, + }) } // Only add the Merge Request opener as the PR author - authors := []jsonUser{} - for j := range cs.Commits { - mr := cs.Commits[j].AssociatedMergeRequest - if mr.Author.Login != "" { - authors = append(authors, jsonUser{ - Login: mr.Author.Login, - }) - break - } - } + authors := []jsonUser{{ + Login: cs.Author.Login, + }} r.Results.DefaultBranchChangesets = append(r.Results.DefaultBranchChangesets, jsonDefaultBranchChangeset{