🐛 Fix Code-Review check for projects using Reviewed-by in commit messages

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 {

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

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 {

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|` +

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",

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

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
+}

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.
 

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.
 

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 {