diff --git a/checker/raw_result.go b/checker/raw_result.go index bf5132b7e45..28f71f2c50a 100644 --- a/checker/raw_result.go +++ b/checker/raw_result.go @@ -200,12 +200,13 @@ type CodeReviewData struct { type ReviewPlatform = string const ( - ReviewPlatformGitHub ReviewPlatform = "GitHub" - ReviewPlatformProw ReviewPlatform = "Prow" - ReviewPlatformGerrit ReviewPlatform = "Gerrit" - ReviewPlatformPhabricator ReviewPlatform = "Phabricator" - ReviewPlatformPiper ReviewPlatform = "Piper" - ReviewPlatformUnknown ReviewPlatform = "Unknown" + ReviewPlatformGitHub ReviewPlatform = "GitHub" + ReviewPlatformProw ReviewPlatform = "Prow" + ReviewPlatformGerrit ReviewPlatform = "Gerrit" + ReviewPlatformPhabricator ReviewPlatform = "Phabricator" + ReviewPlatformPiper ReviewPlatform = "Piper" + ReviewPlatformCommitMessageReview ReviewPlatform = "CommitMessageReview" + ReviewPlatformUnknown ReviewPlatform = "Unknown" ) type Changeset struct { diff --git a/checks/raw/code_review.go b/checks/raw/code_review.go index c343bf16c18..ec607d6a00f 100644 --- a/checks/raw/code_review.go +++ b/checks/raw/code_review.go @@ -25,8 +25,9 @@ import ( ) var ( - rePhabricatorRevID = regexp.MustCompile(`Differential Revision:[^\r\n]*(D\d+)`) - rePiperRevID = regexp.MustCompile(`PiperOrigin-RevId:\s*(\d{3,})`) + rePhabricatorRevID = regexp.MustCompile(`(?i)Differential Revision:[^\r\n]*(D\d+)`) + rePiperRevID = regexp.MustCompile(`(?i)PiperOrigin-RevId:\s*(\d{3,})`) + reReviewedBy = regexp.MustCompile(`(?im)^Reviewed-by:\s*(.+)$`) ) // CodeReview retrieves the raw data for the Code-Review check. @@ -112,6 +113,31 @@ func getPiperRevisionID(c *clients.Commit) string { return match[1] } +func getCommitMessageReviews(c *clients.Commit) []clients.Review { + matches := reReviewedBy.FindAllStringSubmatch(c.Message, -1) + var reviews []clients.Review + for _, match := range matches { + if len(match) > 1 { + reviewerName := strings.TrimSpace(match[1]) + if reviewerName != "" { + user := clients.User{Login: reviewerName} + reviews = append(reviews, clients.Review{ + State: "APPROVED", + Author: &user, + }) + } + } + } + return reviews +} + +func getCommitMessageReviewedByRevisionID(c *clients.Commit) string { + if reReviewedBy.MatchString(c.Message) { + return c.SHA + } + return "" +} + type revisionInfo struct { Platform checker.ReviewPlatform ID string @@ -133,6 +159,9 @@ func detectCommitRevisionInfo(c *clients.Commit) revisionInfo { if revisionID := getPiperRevisionID(c); revisionID != "" { return revisionInfo{checker.ReviewPlatformPiper, revisionID} } + if revisionID := getCommitMessageReviewedByRevisionID(c); revisionID != "" { + return revisionInfo{checker.ReviewPlatformCommitMessageReview, revisionID} + } return revisionInfo{checker.ReviewPlatformUnknown, ""} } @@ -164,6 +193,9 @@ func getChangesets(commits []clients.Commit) []checker.Changeset { if rev.Platform == checker.ReviewPlatformGitHub { newChangeset.Reviews = getGithubReviews(&commits[i]) newChangeset.Author = getGithubAuthor(&commits[i]) + } else if rev.Platform == checker.ReviewPlatformCommitMessageReview { + newChangeset.Reviews = getCommitMessageReviews(&commits[i]) + newChangeset.Author = commits[i].Committer } changesetsByRevInfo[rev] = newChangeset diff --git a/checks/raw/code_review_test.go b/checks/raw/code_review_test.go index 0346eb63226..d19780481ce 100644 --- a/checks/raw/code_review_test.go +++ b/checks/raw/code_review_test.go @@ -111,6 +111,11 @@ func Test_getChangesets(t *testing.T) { Message: "followup\nReviewed-on: server.url \nReviewed-by:user-123", SHA: "def", } + + reviewedByCommit = clients.Commit{ + Message: "Fix issue 4730\n\nReviewed-by: reviewer@example.com", + SHA: "123abc456def", + } ) tests := []struct { @@ -384,6 +389,24 @@ func Test_getChangesets(t *testing.T) { }, }, }, + { + name: "commit message with reviewed-by", + commits: []clients.Commit{reviewedByCommit}, + expected: []checker.Changeset{ + { + ReviewPlatform: checker.ReviewPlatformCommitMessageReview, + RevisionID: "123abc456def", + Commits: []clients.Commit{reviewedByCommit}, + Reviews: []clients.Review{ + { + State: "APPROVED", + Author: &clients.User{Login: "reviewer@example.com"}, + }, + }, + Author: clients.User{}, + }, + }, + }, } for _, tt := range tests { diff --git a/checks/raw/dangerous_workflow.go b/checks/raw/dangerous_workflow.go index 1d8f5ca794f..6dfb232a68f 100644 --- a/checks/raw/dangerous_workflow.go +++ b/checks/raw/dangerous_workflow.go @@ -45,8 +45,12 @@ func containsUntrustedContextPattern(variable string) bool { `head_commit\.message|` + `head_commit\.author\.email|` + `head_commit\.author\.name|` + + `head_commit\.committer\.email|` + + `head_commit\.committer\.name|` + `commits.*\.author\.email|` + `commits.*\.author\.name|` + + `commits.*\.committer\.email|` + + `commits.*\.committer\.name|` + `blocked_user\.name|` + `blocked_user\.email|` + `pull_request\.head\.ref|` + diff --git a/checks/raw/dangerous_workflow_test.go b/checks/raw/dangerous_workflow_test.go index c6431b7b127..973f26a6ec7 100644 --- a/checks/raw/dangerous_workflow_test.go +++ b/checks/raw/dangerous_workflow_test.go @@ -85,6 +85,26 @@ func TestUntrustedContextVariables(t *testing.T) { variable: "github.event.commits[2].author.email", expected: true, }, + { + name: "commits committer name", + variable: "github.event.commits[2].committer.name", + expected: true, + }, + { + name: "commits committer email", + variable: "github.event.commits[2].committer.email", + expected: true, + }, + { + name: "head_commit committer name", + variable: "github.event.head_commit.committer.name", + expected: true, + }, + { + name: "head_commit committer email", + variable: "github.event.head_commit.committer.email", + expected: true, + }, { name: "blocked_user name", variable: "github.event.pull_request.organization.blocked_user.name", diff --git a/clients/githubrepo/searchCommits.go b/clients/githubrepo/searchCommits.go index cdde327dcff..83162944d2b 100644 --- a/clients/githubrepo/searchCommits.go +++ b/clients/githubrepo/searchCommits.go @@ -16,7 +16,9 @@ package githubrepo import ( "context" + "errors" "fmt" + "net/http" "strings" "github.com/google/go-github/v82/github" @@ -49,7 +51,11 @@ func (handler *searchCommitsHandler) search(request clients.SearchCommitsOptions query, &github.SearchOptions{ListOptions: github.ListOptions{PerPage: 100}}) if err != nil { - return nil, fmt.Errorf("Search.Code: %w", err) + var gerr *github.ErrorResponse + if errors.As(err, &gerr) && gerr.Response.StatusCode == http.StatusUnprocessableEntity { + return nil, nil // Return empty list on 422 + } + return nil, fmt.Errorf("Search.Commits: %w", err) } return searchCommitsResponseFrom(resp), nil diff --git a/clients/githubrepo/searchCommits_test.go b/clients/githubrepo/searchCommits_test.go index 3dd252c63c8..6be5750d69a 100644 --- a/clients/githubrepo/searchCommits_test.go +++ b/clients/githubrepo/searchCommits_test.go @@ -15,9 +15,15 @@ package githubrepo import ( + "context" "errors" + "io" + "net/http" + "strings" "testing" + "github.com/google/go-github/v82/github" + "github.com/ossf/scorecard/v5/clients" ) @@ -76,3 +82,40 @@ func TestSearchCommitsBuildQuery(t *testing.T) { }) } } + +func TestSearchCommitsHandle422(t *testing.T) { + t.Parallel() + handler := searchCommitsHandler{ + ghClient: github.NewClient(&http.Client{ + Transport: &mockErrorTransport{ + statusCode: http.StatusUnprocessableEntity, + }, + }), + ctx: context.Background(), + repourl: &Repo{ + commitSHA: clients.HeadSHA, + owner: "testowner", + repo: "testrepo", + }, + } + + commits, err := handler.search(clients.SearchCommitsOptions{Author: "testbot"}) + if err != nil { + t.Fatalf("expected no error, got: %v", err) + } + if len(commits) != 0 { + t.Fatalf("expected 0 commits, got: %d", len(commits)) + } +} + +type mockErrorTransport struct { + statusCode int +} + +func (m *mockErrorTransport) RoundTrip(req *http.Request) (*http.Response, error) { + return &http.Response{ + StatusCode: m.statusCode, + Body: io.NopCloser(strings.NewReader(`{"message": "Validation Failed"}`)), + Header: make(http.Header), + }, nil +} diff --git a/docs/checks.md b/docs/checks.md index 3c9325711cb..45d81d76366 100644 --- a/docs/checks.md +++ b/docs/checks.md @@ -209,7 +209,8 @@ an approval on GitHub 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"). +"lgtm" or "approved"), [Gerrit](https://www.gerritcodereview.com/) ("Reviewed-on" and "Reviewed-by"), +and commit messages ("Reviewed-by"). If recent changes are solely bot activity (e.g. Dependabot, Renovate bot, or custom bots), the check returns inconclusively. diff --git a/docs/checks/internal/checks.yaml b/docs/checks/internal/checks.yaml index bab6591a16f..bf9df4f254f 100644 --- a/docs/checks/internal/checks.yaml +++ b/docs/checks/internal/checks.yaml @@ -307,7 +307,8 @@ checks: 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"). + "lgtm" or "approved"), [Gerrit](https://www.gerritcodereview.com/) ("Reviewed-on" and "Reviewed-by"), + and commit messages ("Reviewed-by"). If recent changes are solely bot activity (e.g. Dependabot, Renovate bot, or custom bots), the check returns inconclusively. diff --git a/probes/codeApproved/impl_test.go b/probes/codeApproved/impl_test.go index 841a59c65dc..c5008bd667a 100644 --- a/probes/codeApproved/impl_test.go +++ b/probes/codeApproved/impl_test.go @@ -328,6 +328,37 @@ func TestProbeCodeApproved(t *testing.T) { finding.OutcomeFalse, }, }, + { + name: "reviewed by commit message gives true outcome", + rawResults: &checker.RawResults{ + CodeReviewResults: checker.CodeReviewData{ + DefaultBranchChangesets: []checker.Changeset{ + { + ReviewPlatform: checker.ReviewPlatformCommitMessageReview, + Commits: []clients.Commit{ + { + SHA: "sha", + Committer: clients.User{Login: "developer"}, + Message: "Fix issue\nReviewed-by: reviewer@example.com", + }, + }, + Reviews: []clients.Review{ + { + State: "APPROVED", + Author: &clients.User{Login: "reviewer@example.com"}, + }, + }, + Author: clients.User{ + Login: "developer", + }, + }, + }, + }, + }, + expectedOutcomes: []finding.Outcome{ + finding.OutcomeTrue, + }, + }, } for _, tt := range probeTests {