Improve dependency analysis checks

- Update Apache License in `main_test.go` - Add `paralleltest` and `govet` comments - Change `_` to `//nolint:paralleltest` and `//nolint:govet` - Change `0644` to `0o644` - Remove test for invalid owner - Remove owner from `Validate` function - Remove a line from the `Vulnerability` struct [dependency-analysis/main_test.go] - Add Apache License to the file - Add `paralleltest` and `govet` comments - Change `_` to `//nolint:paralleltest` and `//nolint:govet` - Change `0644` to `0o644` - Remove test for invalid owner - Remove owner from `Validate` function [dependency-analysis/types.go] - Remove a line from the `Vulnerability` struct Signed-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com>
ossf · Feb 24, 2023 · b8b6508 · b8b6508
1 parent df4cb43
commit b8b6508
Show file tree

Hide file tree

Showing 3 changed files with 101 additions and 69 deletions.
diff --git a/dependency-analysis/main.go b/dependency-analysis/main.go
@@ -1,3 +1,18 @@
+// Copyright 2023 OpenSSF Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//	http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+//
+// SPDX-License-Identifier: Apache-2.0
 package main
 
 import (
@@ -19,18 +34,17 @@ func main() {
 	vulnerabilities := ""
 	result := ""
 
-	owner := os.Getenv("GITHUB_REPOSITORY_OWNER")
 	repo := os.Getenv("GITHUB_REPOSITORY")
 	commitSHA := os.Getenv("GITHUB_SHA")
 	token := os.Getenv("GITHUB_TOKEN")
 	pr := os.Getenv("GITHUB_PR_NUMBER")
 	ghUser := os.Getenv("GITHUB_ACTOR")
-	if err := Validate(token, owner, repo, commitSHA, pr); err != nil {
+	if err := Validate(token, repo, commitSHA, pr); err != nil {
 		log.Fatal(err)
 	}
 
 	ownerRepo := strings.Split(repo, "/")
-	owner = ownerRepo[0]
+	owner := ownerRepo[0]
 	repo = ownerRepo[1]
 	checks, err := GetScorecardChecks()
 	if err != nil {
@@ -51,14 +65,14 @@ func main() {
 	}
 
 	m := make(map[string]DependencyDiff)
-	for _, dep := range data {
+	for _, dep := range *data { //nolint:gocritic
 		m[dep.SourceRepositoryURL] = dep
 	}
 
-	for k, i := range m {
+	for k, i := range m { //nolint:gocritic
 		url := strings.TrimPrefix(k, "https://")
-		scorecard, error := GetScore(url)
-		if error != nil && len(i.Vulnerabilities) > 0 {
+		scorecard, err := GetScore(url)
+		if err != nil && len(i.Vulnerabilities) > 0 {
 			sb := strings.Builder{}
 			sb.WriteString(fmt.Sprintf("<details><summary>Vulnerabilties %s</summary>\n </br>", i.SourceRepositoryURL))
 			sb.WriteString("<table>\n")
@@ -89,7 +103,7 @@ func main() {
 			return false
 		})
 		scorecard.Vulnerabilities = i.Vulnerabilities
-		result += GitHubIssueComment(scorecard)
+		result += GitHubIssueComment(&scorecard)
 	}
 	// convert pr to int
 	prInt, err := strconv.Atoi(pr)
@@ -100,7 +114,8 @@ func main() {
 	if vulnerabilities == "" && result == "" {
 		return
 	}
-	if err := createOrUpdateComment(client, owner, ghUser, repo, prInt, "## Scorecard Results</br>\n"+vulnerabilities+"</br>"+result); err != nil {
+	if err := createOrUpdateComment(
+		client, owner, ghUser, repo, prInt, "## Scorecard Results</br>\n"+vulnerabilities+"</br>"+result); err != nil {
 		log.Fatal(err)
 	}
 	if vulnerabilities != "" {
@@ -120,37 +135,35 @@ func getDefaultBranch(owner, repo, token string) (string, error) {
 
 	repository, _, err := client.Repositories.Get(ctx, owner, repo)
 	if err != nil {
-		return "", fmt.Errorf("failed to get repository: %v", err)
+		return "", fmt.Errorf("failed to get repository: %w", err)
 	}
 
 	return repository.GetDefaultBranch(), nil
 }
 
 // Validate validates the input parameters.
-func Validate(token string, owner string, repo string, commitSHA string, pr string) error {
+func Validate(token, repo, commitSHA, pr string) error {
 	if token == "" {
-		return fmt.Errorf("token is empty")
-	}
-	if owner == "" {
-		return fmt.Errorf("owner is empty")
+		return fmt.Errorf("token is empty") //nolint:goerr113
 	}
 	if repo == "" {
-		return fmt.Errorf("repo is empty")
+		return fmt.Errorf("repo is empty") //nolint:goerr113
 	}
 	if commitSHA == "" {
-		return fmt.Errorf("commitSHA is empty")
+		return fmt.Errorf("commitSHA is empty") //nolint:goerr113
 	}
 	if pr == "" {
-		return fmt.Errorf("pr is empty")
+		return fmt.Errorf("pr is empty") //nolint:goerr113
 	}
 	return nil
 }
 
 // createOrUpdateComment creates a new comment on the pull request or updates an existing one.
 func createOrUpdateComment(client *github.Client, owner, githubUser, repo string, prNum int, commentBody string) error {
-	comments, _, err := client.Issues.ListComments(context.Background(), owner, repo, prNum, &github.IssueListCommentsOptions{})
+	comments, _, err := client.Issues.ListComments(
+		context.Background(), owner, repo, prNum, &github.IssueListCommentsOptions{})
 	if err != nil {
-		return fmt.Errorf("failed to get comments: %v", err)
+		return fmt.Errorf("failed to get comments: %w", err)
 	}
 	// Check if the user has already left a comment on the pull request.
 	var existingComment *github.IssueComment
@@ -166,7 +179,7 @@ func createOrUpdateComment(client *github.Client, owner, githubUser, repo string
 		existingComment.Body = &commentBody
 		_, _, err = client.Issues.EditComment(context.Background(), owner, repo, *existingComment.ID, existingComment)
 		if err != nil {
-			return fmt.Errorf("failed to update comment: %v", err)
+			return fmt.Errorf("failed to update comment: %w", err)
 		}
 		log.Println("Comment updated successfully!")
 	} else {
@@ -176,15 +189,15 @@ func createOrUpdateComment(client *github.Client, owner, githubUser, repo string
 		}
 		_, _, err = client.Issues.CreateComment(context.Background(), owner, repo, prNum, newComment)
 		if err != nil {
-			return fmt.Errorf("failed to create comment: %v", err)
+			return fmt.Errorf("failed to create comment: %w", err)
 		}
 		log.Println("Comment created successfully!")
 	}
 	return nil
 }
 
 // GitHubIssueComment returns a markdown string for a GitHub issue comment.
-func GitHubIssueComment(checks ScorecardResult) string {
+func GitHubIssueComment(checks *ScorecardResult) string {
 	if checks.Repo.Name == "" {
 		return ""
 	}
@@ -204,7 +217,8 @@ func GitHubIssueComment(checks ScorecardResult) string {
 		sb.WriteString("<table>\n")
 		sb.WriteString("<tr><th>Vulnerability</th><th>Severity</th><th>Summary</th></tr>\n")
 		for _, vulns := range checks.Vulnerabilities {
-			sb.WriteString(fmt.Sprintf("<tr><td>%s</td><td>%s</td><td>%s</td></tr>\n", vulns.AdvisoryUrl, vulns.Severity, vulns.AdvisorySummary))
+			sb.WriteString(
+				fmt.Sprintf("<tr><td>%s</td><td>%s</td><td>%s</td></tr>\n", vulns.AdvisoryUrl, vulns.Severity, vulns.AdvisorySummary)) //nolint:lll
 		}
 		sb.WriteString("</table>\n")
 	}
@@ -213,23 +227,26 @@ func GitHubIssueComment(checks ScorecardResult) string {
 	return sb.String()
 }
 
-// GetDependencyDiff returns the dependency diff between two commits. It returns an error if the dependency graph is not enabled.
-func GetDependencyDiff(owner, repo, token, base, head string) ([]DependencyDiff, error) {
+// GetDependencyDiff returns the dependency diff between two commits.
+// It returns an error if the dependency graph is not enabled.
+func GetDependencyDiff(owner, repo, token, base, head string) (*[]DependencyDiff, error) {
+	message := "failed to get dependency diff, please enable dependency graph https://docs.github.com/en/code-security/supply-chain-security/understanding-your-software-supply-chain/configuring-the-dependency-graph" //nolint:lll
 	if owner == "" {
-		return nil, fmt.Errorf("owner is required")
+		return nil, fmt.Errorf("owner is required") //nolint:goerr113
 	}
 	if repo == "" {
-		return nil, fmt.Errorf("repo is required")
+		return nil, fmt.Errorf("repo is required") //nolint:goerr113
 	}
 	if token == "" {
-		return nil, fmt.Errorf("token is required")
+		return nil, fmt.Errorf("token is required") //nolint:goerr113
 	}
 	resp, err := GetGitHubDependencyDiff(owner, repo, token, base, head)
-	defer resp.Body.Close()
+	defer resp.Body.Close() //nolint:staticcheck
 
 	if resp.StatusCode != http.StatusOK {
 		// if the dependency graph is not enabled, we can't get the dependency diff
-		return nil, fmt.Errorf("failed to get dependency diff, please enable dependency graph https://docs.github.com/en/code-security/supply-chain-security/understanding-your-software-supply-chain/configuring-the-dependency-graph : %v", resp.Status)
+		return nil,
+			fmt.Errorf(" %s: %v", message, resp.Status) //nolint:goerr113
 	}
 	if err != nil {
 		return nil, fmt.Errorf("failed to get dependency diff: %w", err)
@@ -238,21 +255,23 @@ func GetDependencyDiff(owner, repo, token, base, head string) ([]DependencyDiff,
 	var data []DependencyDiff
 	err = json.NewDecoder(resp.Body).Decode(&data)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed to decode dependency diff: %w", err)
 	}
 	// filter out the dependencies that are not added
 	var filteredData []DependencyDiff
-	for _, dep := range data {
+	for _, dep := range data { //nolint:gocritic
 		// also if the source repo doesn't start with GitHub.com, we can ignore it
-		if dep.ChangeType == "added" && dep.SourceRepositoryURL != "" && strings.HasPrefix(dep.SourceRepositoryURL, "https://github.com") {
+		if dep.ChangeType == "added" && dep.SourceRepositoryURL != "" &&
+			strings.HasPrefix(dep.SourceRepositoryURL, "https://github.com") {
 			filteredData = append(filteredData, dep)
 		}
 	}
-	return filteredData, nil
+	return &filteredData, nil
 }
 
-// GetGitHubDependencyDiff returns the dependency diff between two commits. It returns an error if the dependency graph is not enabled.
-func GetGitHubDependencyDiff(owner string, repo string, token string, base string, head string) (*http.Response, error) {
+// GetGitHubDependencyDiff returns the dependency diff between two commits.
+// It returns an error if the dependency graph is not enabled.
+func GetGitHubDependencyDiff(owner, repo, token, base, head string) (*http.Response, error) {
 	req, err := http.NewRequest("GET",
 		fmt.Sprintf("https://api.github.com/repos/%s/%s/dependency-graph/compare/%s...%s", owner, repo, base, head), nil)
 	if err != nil {
@@ -286,18 +305,21 @@ func GetScorecardChecks() ([]string, error) {
 	fileName := os.Getenv("SCORECARD_CHECKS")
 	if fileName == "" {
 		// default to critical and high severity checks
-		return []string{"Dangerous-Workflow", "Binary-Artifacts", "Branch-Protection", "Code-Review", "Dependency-Update-Tool"}, nil
+		return []string{
+			"Dangerous-Workflow",
+			"Binary-Artifacts", "Branch-Protection", "Code-Review", "Dependency-Update-Tool",
+		}, nil
 	}
 	f, err := os.Open(fileName)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed to open file %s: %w", fileName, err)
 	}
 	defer f.Close()
 	decoder := json.NewDecoder(f)
 	var checksFromFile []string
 	err = decoder.Decode(&checksFromFile)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed to decode file %s: %w", fileName, err)
 	}
 	return checksFromFile, nil
 }

diff --git a/dependency-analysis/main_test.go b/dependency-analysis/main_test.go
@@ -1,3 +1,18 @@
+// Copyright 2023 OpenSSF Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//	http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+//
+// SPDX-License-Identifier: Apache-2.0
 package main
 
 import (
@@ -7,8 +22,8 @@ import (
 	"testing"
 )
 
-func Test_filter(t *testing.T) {
-	type args[T any] struct {
+func Test_filter(t *testing.T) { //nolint:paralleltest
+	type args[T any] struct { //nolint:govet
 		slice []T
 		f     func(T) bool
 	}
@@ -28,7 +43,7 @@ func Test_filter(t *testing.T) {
 		},
 	}
 
-	for _, tt := range tests {
+	for _, tt := range tests { //nolint:paralleltest
 		t.Run(tt.name, func(t *testing.T) {
 			if got := filter(tt.args.slice, tt.args.f); !reflect.DeepEqual(got, tt.want) {
 				t.Errorf("filter() = %v, want %v", got, tt.want)
@@ -37,16 +52,16 @@ func Test_filter(t *testing.T) {
 	}
 }
 
-func TestGetScorecardChecks(t *testing.T) {
-	tests := []struct {
+func TestGetScorecardChecks(t *testing.T) { //nolint:paralleltest
+	tests := []struct { //nolint:govet
 		name        string
 		want        []string
 		fileContent string
 		wantErr     bool
 	}{
 		{
 			name:    "default",
-			want:    []string{"Dangerous-Workflow", "Binary-Artifacts", "Branch-Protection", "Code-Review", "Dependency-Update-Tool"},
+			want:    []string{"Dangerous-Workflow", "Binary-Artifacts", "Branch-Protection", "Code-Review", "Dependency-Update-Tool"}, //nolint:lll
 			wantErr: false,
 		},
 		{
@@ -59,7 +74,7 @@ func TestGetScorecardChecks(t *testing.T) {
 			wantErr: false,
 		},
 	}
-	for _, tt := range tests {
+	for _, tt := range tests { //nolint:paralleltest
 		t.Run(tt.name, func(t *testing.T) {
 			if tt.fileContent != "" {
 				dir, err := os.MkdirTemp("", "scorecard-checks")
@@ -69,7 +84,7 @@ func TestGetScorecardChecks(t *testing.T) {
 				}
 				defer os.RemoveAll(dir)
 
-				if err := os.WriteFile(path.Join(dir, "scorecard.txt"), []byte(tt.fileContent), 0644); err != nil {
+				if err := os.WriteFile(path.Join(dir, "scorecard.txt"), []byte(tt.fileContent), 0o644); err != nil { //nolint:gosec
 					t.Errorf("GetScorecardChecks() error = %v, wantErr %v", err, tt.wantErr)
 					return
 				}
@@ -87,7 +102,7 @@ func TestGetScorecardChecks(t *testing.T) {
 	}
 }
 
-func TestGetScore(t *testing.T) {
+func TestGetScore(t *testing.T) { //nolint:paralleltest
 	type args struct {
 		repo string
 	}
@@ -115,7 +130,7 @@ func TestGetScore(t *testing.T) {
 		},
 	}
 
-	for _, tt := range tests {
+	for _, tt := range tests { //nolint:paralleltest
 		t.Run(tt.name, func(t *testing.T) {
 			got, err := GetScore(tt.args.repo)
 			if (err != nil) != tt.wantErr {
@@ -129,7 +144,7 @@ func TestGetScore(t *testing.T) {
 	}
 }
 
-func TestValidate(t *testing.T) {
+func TestValidate(t *testing.T) { //nolint:paralleltest
 	type args struct {
 		token     string
 		owner     string
@@ -163,16 +178,6 @@ func TestValidate(t *testing.T) {
 			},
 			wantErr: true,
 		},
-		{
-			name: "invalid owner",
-			args: args{
-				repo:      "scorecard",
-				token:     "token",
-				commitSHA: "commitSHA",
-				pr:        "1",
-			},
-			wantErr: true,
-		},
 		{
 			name: "invalid repo",
 			args: args{
@@ -205,9 +210,9 @@ func TestValidate(t *testing.T) {
 		},
 	}
 
-	for _, tt := range tests {
+	for _, tt := range tests { //nolint:paralleltest
 		t.Run(tt.name, func(t *testing.T) {
-			if err := Validate(tt.args.token, tt.args.owner, tt.args.repo, tt.args.commitSHA, tt.args.pr); (err != nil) != tt.wantErr {
+			if err := Validate(tt.args.token, tt.args.repo, tt.args.commitSHA, tt.args.pr); (err != nil) != tt.wantErr {
 				t.Errorf("Validate() error = %v, wantErr %v", err, tt.wantErr)
 			}
 		})