🐛 Add GitHub committer verification #1695

laurentsimon · 2022-03-02T00:17:56Z

PR verifies the commit's signature when the committer's name is "GitHub" and the committer's login for a merged PR is "".
See #1543 for more details

In addition, it fixes reviewer names that were not populated( I caught this problem when using --format raw)

closes #1543

Verify merged commits by GitHub are genuine

clients/githubrepo/graphql.go

codecov · 2022-03-02T00:21:48Z

Codecov Report

Merging #1695 (ef812a3) into main (57b4664) will increase coverage by 3.37%.
The diff coverage is 80.00%.

❗ Current head ef812a3 differs from pull request most recent head 844ca45. Consider uploading reports for the commit 844ca45 to get more accurate results

@@            Coverage Diff             @@
##             main    #1695      +/-   ##
==========================================
+ Coverage   58.39%   61.76%   +3.37%     
==========================================
  Files          58       58              
  Lines        5910     5940      +30     
==========================================
+ Hits         3451     3669     +218     
+ Misses       2219     2026     -193     
- Partials      240      245       +5

github-actions · 2022-03-02T00:23:22Z

Integration tests success for
[ade2929]
(https://github.com/ossf/scorecard/actions/runs/1919463027)

github-actions · 2022-03-02T00:28:05Z

Integration tests success for
[816c7c4]
(https://github.com/ossf/scorecard/actions/runs/1919473087)

github-actions · 2022-03-02T02:38:01Z

Integration tests success for
[4503af5]
(https://github.com/ossf/scorecard/actions/runs/1919904755)

azeemshaikh38 · 2022-03-02T05:22:02Z

clients/githubrepo/graphql.go

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


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

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

azeemshaikh38 · 2022-03-02T05:23:32Z

clients/githubrepo/graphql.go

-		// 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)


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

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.

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

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

Created #1710

clients/githubrepo/graphql.go

azeemshaikh38 · 2022-03-02T05:26:12Z

clients/githubrepo/graphql.go

 				associatedPR.Reviews = append(associatedPR.Reviews, clients.Review{
-					State: string(review.State),
+					State:  string(review.State),
+					Author: &author,


Nit: remove the author var and create the struct here directly.

author is a pointer, I don't think you can assign a temporary pointer directly. If the compiler does not complain, I'll do that

And I was wrong, below seem to work just fine!

Author: &clients.User{ Login: string(review.Author.Login), },

github-actions · 2022-03-03T17:57:17Z

Integration tests success for
[18f0677]
(https://github.com/ossf/scorecard/actions/runs/1929562811)

github-actions · 2022-03-03T17:59:42Z

Integration tests success for
[ef812a3]
(https://github.com/ossf/scorecard/actions/runs/1929566229)

github-actions · 2022-03-03T18:04:01Z

Integration tests success for
[844ca45]
(https://github.com/ossf/scorecard/actions/runs/1929568560)

laurentsimon requested review from azeemshaikh38, inferno-chromium, justaugustus and naveensrinivasan as code owners March 2, 2022 00:17

laurentsimon temporarily deployed to integration-test March 2, 2022 00:18 Inactive

laurentsimon temporarily deployed to integration-test March 2, 2022 00:21 Inactive

laurentsimon commented Mar 2, 2022

View reviewed changes

clients/githubrepo/graphql.go Outdated Show resolved Hide resolved

laurentsimon temporarily deployed to integration-test March 2, 2022 02:32 Inactive

azeemshaikh38 reviewed Mar 2, 2022

View reviewed changes

azeemshaikh38 approved these changes Mar 3, 2022

View reviewed changes

laurentsimon temporarily deployed to integration-test March 3, 2022 17:50 Inactive

laurentsimon force-pushed the bug/committer branch from 18f0677 to ef812a3 Compare March 3, 2022 17:50

laurentsimon temporarily deployed to integration-test March 3, 2022 17:51 Inactive

laurentsimon added 4 commits March 3, 2022 17:51

Add GitHub committer verification and fix empty reviewers

630504f

update comment

44cf16c

linter

262f7bd

comments

844ca45

laurentsimon force-pushed the bug/committer branch from ef812a3 to 844ca45 Compare March 3, 2022 17:51

laurentsimon enabled auto-merge (squash) March 3, 2022 17:51

laurentsimon temporarily deployed to integration-test March 3, 2022 17:55 Inactive

laurentsimon merged commit 3c92dec into ossf:main Mar 3, 2022

azeemshaikh38 mentioned this pull request Mar 4, 2022

Breakages due to #1695 #1710

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Add GitHub committer verification #1695

🐛 Add GitHub committer verification #1695

laurentsimon commented Mar 2, 2022 •

edited

Loading

codecov bot commented Mar 2, 2022 •

edited

Loading

github-actions bot commented Mar 2, 2022

github-actions bot commented Mar 2, 2022

github-actions bot commented Mar 2, 2022

azeemshaikh38 Mar 2, 2022

laurentsimon Mar 2, 2022

azeemshaikh38 Mar 2, 2022

laurentsimon Mar 2, 2022 •

edited

Loading

laurentsimon Mar 3, 2022

ristomcgehee Mar 4, 2022

azeemshaikh38 Mar 4, 2022

azeemshaikh38 Mar 2, 2022

laurentsimon Mar 2, 2022

laurentsimon Mar 3, 2022

github-actions bot commented Mar 3, 2022

github-actions bot commented Mar 3, 2022

github-actions bot commented Mar 3, 2022

🐛 Add GitHub committer verification #1695

🐛 Add GitHub committer verification #1695

Conversation

laurentsimon commented Mar 2, 2022 • edited Loading

codecov bot commented Mar 2, 2022 • edited Loading

Codecov Report

github-actions bot commented Mar 2, 2022

github-actions bot commented Mar 2, 2022

github-actions bot commented Mar 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laurentsimon Mar 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 3, 2022

github-actions bot commented Mar 3, 2022

github-actions bot commented Mar 3, 2022

laurentsimon commented Mar 2, 2022 •

edited

Loading

codecov bot commented Mar 2, 2022 •

edited

Loading

laurentsimon Mar 2, 2022 •

edited

Loading