Skip to content
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.

Sign up for GitHub

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

🐛 Add GitHub committer verification #1695

Merged
merged 4 commits into from
Mar 3, 2022
Merged

🐛 Add GitHub committer verification #1695

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
630504f
Add GitHub committer verification and fix empty reviewers
laurentsimon Mar 2, 2022
44cf16c
update comment
laurentsimon Mar 2, 2022
262f7bd
linter
laurentsimon Mar 2, 2022
844ca45
comments
laurentsimon Mar 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 24 additions & 3 deletions clients/githubrepo/graphql.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ package githubrepo

import (
"context"
"errors"
"fmt"
"strings"
"sync"
Expand All @@ -36,6 +37,8 @@ const (
commitsToAnalyze = 30
)

var errorInvalidCommitterLogin = errors.New("cannot retrieve committer login")

// nolint: govet
type graphqlData struct {
Repository struct {
Expand All @@ -58,6 +61,10 @@ type graphqlData struct {
Login *string
}
}
Signature struct {
IsValid bool
WasSignedByGitHub bool
}
AssociatedPullRequests struct {
Nodes []struct {
Repository struct {
Expand Down Expand Up @@ -191,15 +198,26 @@ func (handler *graphqlHandler) isArchived() (bool, error) {
return handler.archived, nil
}

// nolint: unparam
func commitsFrom(data *graphqlData, repoOwner, repoName string) ([]clients.Commit, error) {
ret := make([]clients.Commit, 0)
for _, commit := range data.Repository.Object.Commit.History.Nodes {
var committer string
if commit.Committer.User.Login != nil {
// Find the commit's committer.
if commit.Committer.User.Login != nil && *commit.Committer.User.Login != "" {
committer = *commit.Committer.User.Login
} else if commit.Committer.Name != nil &&
// Username "GitHub" may indicate the commit was committed by GitHub.
// We verify that the commit is signed by GitHub, because the name can be spoofed.
*commit.Committer.Name == "GitHub" &&
Comment on lines +208 to +211
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason we want to check committer.name if we already have wasSignedByGitHub bool?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just to verify our assumptions that the name should be "GitHub". Not exactly needed, agreed.

commit.Signature.IsValid &&
commit.Signature.WasSignedByGitHub {
committer = "github"
}
// TODO(#1543): Figure out a way to safely get committer if `User.Login` is `nil`.

if committer == "" {
return ret, fmt.Errorf("commit %s: %w", commit.Oid, errorInvalidCommitterLogin)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not leave committer as nil if this is the case? This might cause a lot of errors in the cron job.

Copy link
Contributor Author

@laurentsimon laurentsimon Mar 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's try to see what happens in the integration tests before using this code for the weekly cron. It'd be great to be able to report the committer's login reliably.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so I'll keep code as-is. Hope this does not backfire

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm running with the latest main code, and I think this is causing an error for me:

$ ./scorecard --repo=dart-lang/sdk --checks=Fuzzing
Starting [Fuzzing]
2022/03/03 19:26:00 internal error: ListCommits:error during graphqlHandler.setup: commit d3e36fab5eb55706a36144fe41abc2546ffa0b55: cannot retrieve committer login
panic: internal error: ListCommits:error during graphqlHandler.setup: commit d3e36fab5eb55706a36144fe41abc2546ffa0b55: cannot retrieve committer login

goroutine 1 [running]:
log.Panic({0xc000667ab8, 0xc0002be1c0, 0x100df38})
        log/log.go:354 +0x65
github.com/ossf/scorecard/v4/cmd.rootCmd(0xc000470000)
        github.com/ossf/scorecard/v4/cmd/root.go:138 +0x751
github.com/ossf/scorecard/v4/cmd.New.func2(0xc000421680, {0xe9e91a, 0x2, 0x2})
        github.com/ossf/scorecard/v4/cmd/root.go:62 +0x1d
github.com/spf13/cobra.(*Command).execute(0xc000421680, {0xc0000a8160, 0x2, 0x2})
        github.com/spf13/[email protected]/command.go:860 +0x5f8
github.com/spf13/cobra.(*Command).ExecuteC(0xc000421680)
        github.com/spf13/[email protected]/command.go:974 +0x3bc
github.com/spf13/cobra.(*Command).Execute(...)
        github.com/spf13/[email protected]/command.go:902
main.main()
        github.com/ossf/scorecard/v4/main.go:27 +0x25

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created #1710

}

var associatedPR clients.PullRequest
for i := range commit.AssociatedPullRequests.Nodes {
pr := commit.AssociatedPullRequests.Nodes[i]
Expand All @@ -226,6 +244,9 @@ func commitsFrom(data *graphqlData, repoOwner, repoName string) ([]clients.Commi
for _, review := range pr.Reviews.Nodes {
associatedPR.Reviews = append(associatedPR.Reviews, clients.Review{
State: string(review.State),
Author: &clients.User{
Login: string(review.Author.Login),
},
})
}
break
Expand Down