✨ [migration to score] 2: dependabot and binary artifact checks (#718)

* details-1

* nits

* typo

* commments

* dependabot and binary artifacts checks

* typo

* linter

* missing errors.go

* linter

* merge fix

* dates

checks/automatic_dependency_update.go

@@ -29,23 +29,28 @@ func init() {
 
 // AutomaticDependencyUpdate will check the repository if it contains Automatic dependency update.
 func AutomaticDependencyUpdate(c *checker.CheckRequest) checker.CheckResult {
-	result := CheckIfFileExists(CheckAutomaticDependencyUpdate, c, fileExists)
-	if !result.Pass {
-		result.Confidence = 3
+	r, err := CheckIfFileExists2(CheckAutomaticDependencyUpdate, c, fileExists)
+	if err != nil {
+		return checker.CreateRuntimeErrorResult(CheckAutomaticDependencyUpdate, err)
 	}
-	return result
+	if !r {
+		return checker.CreateMinScoreResult(CheckAutomaticDependencyUpdate, "no tool detected [dependabot|renovabot]")
+	}
+
+	// High score result.
+	return checker.CreateMaxScoreResult(CheckAutomaticDependencyUpdate, "tool detected")
 }
 
 // fileExists will validate the if frozen dependencies file name exists.
-func fileExists(name string, logf func(s string, f ...interface{})) (bool, error) {
+func fileExists(name string, dl checker.DetailLogger) (bool, error) {
 	switch strings.ToLower(name) {
 	case ".github/dependabot.yml":
-		logf("dependabot config found: %s", name)
+		dl.Info("dependabot detected : %s", name)
 		return true, nil
 		// https://docs.renovatebot.com/configuration-options/
 	case ".github/renovate.json", ".github/renovate.json5", ".renovaterc.json", "renovate.json",
 		"renovate.json5", ".renovaterc":
-		logf("renovate config found: %s", name)
+		dl.Info("renovate detected: %s", name)
 		return true, nil
 	default:
 		return false, nil

checks/binary_artifact.go

@@ -22,22 +22,31 @@ import (
 	"github.com/h2non/filetype/types"
 
 	"github.com/ossf/scorecard/checker"
+	sce "github.com/ossf/scorecard/errors"
 )
 
+const CheckBinaryArtifacts string = "Binary-Artifacts"
+
 //nolint
 func init() {
 	registerCheck(CheckBinaryArtifacts, BinaryArtifacts)
 }
 
-const CheckBinaryArtifacts string = "Binary-Artifacts"
-
 // BinaryArtifacts  will check the repository if it contains binary artifacts.
 func BinaryArtifacts(c *checker.CheckRequest) checker.CheckResult {
-	return CheckFilesContent(CheckBinaryArtifacts, "*", false, c, checkBinaryFileContent)
+	r, err := CheckFilesContent2("*", false, c, checkBinaryFileContent)
+	if err != nil {
+		return checker.CreateRuntimeErrorResult(CheckBinaryArtifacts, err)
+	}
+	if !r {
+		return checker.CreateMinScoreResult(CheckBinaryArtifacts, "binaries present in source code")
+	}
+
+	return checker.CreateMaxScoreResult(CheckBinaryArtifacts, "no binaries found in the repo")
 }
 
 func checkBinaryFileContent(path string, content []byte,
-	logf func(s string, f ...interface{})) (bool, error) {
+	dl checker.DetailLogger) (bool, error) {
 	binaryFileTypes := map[string]bool{
 		"crx":     true,
 		"deb":     true,
@@ -78,15 +87,16 @@ func checkBinaryFileContent(path string, content []byte,
 	var t types.Type
 	var err error
 	if t, err = filetype.Get(content); err != nil {
-		return false, fmt.Errorf("failed in getting the content type %w", err)
+		//nolint
+		return false, sce.Create(sce.ErrScorecardInternal, fmt.Sprintf("filetype.Get:%v", err))
 	}
 
 	if _, ok := binaryFileTypes[t.Extension]; ok {
-		logf("!! binary-artifact %s", path)
+		dl.Warn("binary found: %s", path)
 		return false, nil
 	} else if _, ok := binaryFileTypes[filepath.Ext(path)]; ok {
-		// falling back to file based extension.
-		logf("!! binary-artifact %s", path)
+		// Falling back to file based extension.
+		dl.Warn("binary found: %s", path)
 		return false, nil
 	}
 

checks/checkforcontent.go

@@ -15,17 +15,14 @@
 package checks
 
 import (
-	"errors"
 	"fmt"
 	"path"
 	"strings"
 
 	"github.com/ossf/scorecard/checker"
+	sce "github.com/ossf/scorecard/errors"
 )
 
-// ErrReadFile indicates the header size does not match the size of the file.
-var ErrReadFile = errors.New("could not read entire file")
-
 // IsMatchingPath uses 'pattern' to shell-match the 'path' and its filename
 // 'caseSensitive' indicates the match should be case-sensitive. Default: no.
 func isMatchingPath(pattern, fullpath string, caseSensitive bool) (bool, error) {
@@ -37,13 +34,15 @@ func isMatchingPath(pattern, fullpath string, caseSensitive bool) (bool, error)
 	filename := path.Base(fullpath)
 	match, err := path.Match(pattern, fullpath)
 	if err != nil {
-		return false, fmt.Errorf("match error: %w", err)
+		//nolint
+		return false, sce.Create(sce.ErrScorecardInternal, fmt.Sprintf("%v: %v", errInternalFilenameMatch, err))
 	}
 
 	// No match on the fullpath, let's try on the filename only.
 	if !match {
 		if match, err = path.Match(pattern, filename); err != nil {
-			return false, fmt.Errorf("match error: %w", err)
+			//nolint
+			return false, sce.Create(sce.ErrScorecardInternal, fmt.Sprintf("%v: %v", errInternalFilenameMatch, err))
 		}
 	}
 
@@ -77,7 +76,6 @@ func CheckFilesContent(checkName, shellPathFnPattern string,
 		// Filter out files based on path/names using the pattern.
 		b, err := isMatchingPath(shellPathFnPattern, filepath, caseSensitive)
 		if err != nil {
-			c.Logf("error during isMatchingPath: %v", err)
 			return false
 		}
 		return b
@@ -99,10 +97,50 @@ func CheckFilesContent(checkName, shellPathFnPattern string,
 			res = false
 		}
 	}
-
 	if res {
 		return checker.MakePassResult(checkName)
 	}
 
 	return checker.MakeFailResult(checkName, nil)
 }
+
+// UPGRADEv2: to rename to CheckFilesContent.
+func CheckFilesContent2(shellPathFnPattern string,
+	caseSensitive bool,
+	c *checker.CheckRequest,
+	onFileContent func(path string, content []byte,
+		dl checker.DetailLogger) (bool, error),
+) (bool, error) {
+	predicate := func(filepath string) bool {
+		// Filter out Scorecard's own test files.
+		if isScorecardTestFile(c.Owner, c.Repo, filepath) {
+			return false
+		}
+		// Filter out files based on path/names using the pattern.
+		b, err := isMatchingPath(shellPathFnPattern, filepath, caseSensitive)
+		if err != nil {
+			return false
+		}
+		return b
+	}
+	res := true
+	for _, file := range c.RepoClient.ListFiles(predicate) {
+		content, err := c.RepoClient.GetFileContent(file)
+		if err != nil {
+			//nolint
+			return false, sce.Create(sce.ErrScorecardInternal, err.Error())
+		}
+
+		rr, err := onFileContent(file, content, c.Dlogger)
+		if err != nil {
+			return false, err
+		}
+		// We don't return rightway to let the onFileContent()
+		// handler log.
+		if !rr {
+			res = false
+		}
+	}
+
+	return res, nil
+}

checks/checkforfile.go

@@ -44,3 +44,19 @@ func CheckIfFileExists(checkName string, c *checker.CheckRequest, onFile func(na
 		Confidence: confidence,
 	}
 }
+
+func CheckIfFileExists2(checkName string, c *checker.CheckRequest, onFile func(name string,
+	dl checker.DetailLogger) (bool, error)) (bool, error) {
+	for _, filename := range c.RepoClient.ListFiles(func(string) bool { return true }) {
+		rr, err := onFile(filename, c.Dlogger)
+		if err != nil {
+			return false, err
+		}
+
+		if rr {
+			return true, nil
+		}
+	}
+
+	return false, nil
+}

checks/errors.go

@@ -1,3 +1,5 @@
+// Copyright 2021 Security Scorecard 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

e2e/automatic_deps_test.go

@@ -12,6 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+//nolint:dupl
 package e2e
 
 import (
@@ -23,47 +24,73 @@ import (
 	"github.com/ossf/scorecard/checker"
 	"github.com/ossf/scorecard/checks"
 	"github.com/ossf/scorecard/clients/githubrepo"
+	scut "github.com/ossf/scorecard/utests"
 )
 
+// TODO: use dedicated repo that don't change.
+// TODO: need negative results.
 var _ = Describe("E2E TEST:Automatic-Dependency-Update", func() {
 	Context("E2E TEST:Validating dependencies are automatically updated", func() {
 		It("Should return deps are automatically updated for dependabot", func() {
-			l := log{}
+			dl := scut.TestDetailLogger{}
 			repoClient := githubrepo.CreateGithubRepoClient(context.Background(), ghClient)
 			err := repoClient.InitRepo("ossf", "scorecard")
 			Expect(err).Should(BeNil())
 
-			checker := checker.CheckRequest{
+			req := checker.CheckRequest{
 				Ctx:         context.Background(),
 				Client:      ghClient,
 				RepoClient:  repoClient,
 				Owner:       "ossf",
 				Repo:        "scorecard",
 				GraphClient: graphClient,
-				Logf:        l.Logf,
+				Dlogger:     &dl,
 			}
-			result := checks.AutomaticDependencyUpdate(&checker)
+			expected := scut.TestReturn{
+				Errors:        nil,
+				Score:         checker.MaxResultScore,
+				NumberOfWarn:  0,
+				NumberOfInfo:  1,
+				NumberOfDebug: 0,
+			}
+
+			result := checks.AutomaticDependencyUpdate(&req)
+			// UPGRADEv2: to remove.
+			// Old version.
 			Expect(result.Error).Should(BeNil())
 			Expect(result.Pass).Should(BeTrue())
+			// New version.
+			Expect(scut.ValidateTestReturn(nil, "dependabot", &expected, &result, &dl)).Should(BeTrue())
 		})
 		It("Should return deps are automatically updated for renovatebot", func() {
-			l := log{}
+			dl := scut.TestDetailLogger{}
 			repoClient := githubrepo.CreateGithubRepoClient(context.Background(), ghClient)
 			err := repoClient.InitRepo("netlify", "netlify-cms")
 			Expect(err).Should(BeNil())
 
-			checker := checker.CheckRequest{
+			req := checker.CheckRequest{
 				Ctx:         context.Background(),
 				Client:      ghClient,
 				RepoClient:  repoClient,
 				Owner:       "netlify",
 				Repo:        "netlify-cms",
 				GraphClient: graphClient,
-				Logf:        l.Logf,
+				Dlogger:     &dl,
 			}
-			result := checks.AutomaticDependencyUpdate(&checker)
+			expected := scut.TestReturn{
+				Errors:        nil,
+				Score:         checker.MaxResultScore,
+				NumberOfWarn:  0,
+				NumberOfInfo:  1,
+				NumberOfDebug: 0,
+			}
+			result := checks.AutomaticDependencyUpdate(&req)
+			// UPGRADEv2: to remove.
+			// Old version.
 			Expect(result.Error).Should(BeNil())
 			Expect(result.Pass).Should(BeTrue())
+			// New version.
+			Expect(scut.ValidateTestReturn(nil, "renovabot", &expected, &result, &dl)).Should(BeTrue())
 		})
 	})
 })

e2e/binary_artifacts_test.go

@@ -0,0 +1,96 @@
+// Copyright 2021 Security Scorecard 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.
+
+//nolint:dupl
+package e2e
+
+import (
+	"context"
+
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+
+	"github.com/ossf/scorecard/checker"
+	"github.com/ossf/scorecard/checks"
+	"github.com/ossf/scorecard/clients/githubrepo"
+	scut "github.com/ossf/scorecard/utests"
+)
+
+// TODO: use dedicated repo that don't change.
+// TODO: need negative results.
+var _ = Describe("E2E TEST:Binary-Artifacts", func() {
+	Context("E2E TEST:Binary artifacts are not present in source code", func() {
+		It("Should return not binary artifacts in source code", func() {
+			dl := scut.TestDetailLogger{}
+			repoClient := githubrepo.CreateGithubRepoClient(context.Background(), ghClient)
+			err := repoClient.InitRepo("ossf", "scorecard")
+			Expect(err).Should(BeNil())
+
+			req := checker.CheckRequest{
+				Ctx:         context.Background(),
+				Client:      ghClient,
+				RepoClient:  repoClient,
+				Owner:       "ossf",
+				Repo:        "scorecard",
+				GraphClient: graphClient,
+				Dlogger:     &dl,
+			}
+			expected := scut.TestReturn{
+				Errors:        nil,
+				Score:         checker.MaxResultScore,
+				NumberOfWarn:  0,
+				NumberOfInfo:  0,
+				NumberOfDebug: 0,
+			}
+
+			result := checks.BinaryArtifacts(&req)
+			// UPGRADEv2: to remove.
+			// Old version.
+			Expect(result.Error).Should(BeNil())
+			Expect(result.Pass).Should(BeTrue())
+			// New version.
+			Expect(scut.ValidateTestReturn(nil, "no binary artifacts", &expected, &result, &dl)).Should(BeTrue())
+		})
+		It("Should return binary artifacts present in source code", func() {
+			dl := scut.TestDetailLogger{}
+			repoClient := githubrepo.CreateGithubRepoClient(context.Background(), ghClient)
+			err := repoClient.InitRepo("a1ive", "grub2-filemanager")
+			Expect(err).Should(BeNil())
+
+			req := checker.CheckRequest{
+				Ctx:         context.Background(),
+				Client:      ghClient,
+				RepoClient:  repoClient,
+				Owner:       "a1ive",
+				Repo:        "grub2-filemanager",
+				GraphClient: graphClient,
+				Dlogger:     &dl,
+			}
+			expected := scut.TestReturn{
+				Errors:        nil,
+				Score:         checker.MinResultScore,
+				NumberOfWarn:  1,
+				NumberOfInfo:  0,
+				NumberOfDebug: 0,
+			}
+			result := checks.BinaryArtifacts(&req)
+			// UPGRADEv2: to remove.
+			// Old version.
+			Expect(result.Error).Should(BeNil())
+			Expect(result.Pass).Should(BeFalse())
+			// New version.
+			Expect(scut.ValidateTestReturn(nil, " binary artifacts", &expected, &result, &dl)).Should(BeTrue())
+		})
+	})
+})

errors/errors.md

@@ -11,18 +11,18 @@ import sce "github.com/ossf/scorecard/errors"
 // Return a standard check run failure, with an error message from an internal error.
 // We only create internal errors for errors that may happen in several places in the code: this provides
 // consistent error messages to the caller. 
-return sce.Create(sce.ErrRunFailure, errInternalInvalidYamlFile.Error())
+return sce.Create(sce.ErrScorecardInternal, errInternalInvalidYamlFile.Error())
 
 // Return a standard check run failure, with an error message from an internal error and an API call error.
 err := dependency.apiCall()
 if err != nil {
-    return sce.Create(sce.ErrRunFailure, fmt.Sprintf("%v: %v", errInternalSomething, err))
+    return sce.Create(sce.ErrScorecardInternal, fmt.Sprintf("%v: %v", errInternalSomething, err))
 }
 
 // Return a standard check run failure, only with API call error. Use this format when there is no internal error associated
 // to the failure. In many cases, we don't need internal errors.
 err := dependency.apiCall()
 if err != nil {
-    return sce.Create(sce.ErrRunFailure, fmt.Sprintf("dependency.apiCall: %v", err))
+    return sce.Create(sce.ErrScorecardInternal, fmt.Sprintf("dependency.apiCall: %v", err))
 }
 ```

pkg/scorecard_result.go

@@ -201,10 +201,9 @@ func (r *ScorecardResult) AsString2(showDetails bool, logLevel zapcore.Level, wr
 		x[2] = row.Name
 		if showDetails {
 			details, show := detailsToString(row.Details2, logLevel)
-			if !show {
-				continue
+			if show {
+				x[3] = details
 			}
-			x[3] = details
 			x[4] = doc
 		} else {
 			x[3] = doc

utests/utlib.go

@@ -0,0 +1,96 @@
+// Copyright 2021 Security Scorecard 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.
+
+package utests
+
+import (
+	"errors"
+	"fmt"
+	"testing"
+
+	"github.com/ossf/scorecard/checker"
+)
+
+func validateDetailTypes(messages []checker.CheckDetail, nw, ni, nd int) bool {
+	enw := 0
+	eni := 0
+	end := 0
+	for _, v := range messages {
+		switch v.Type {
+		default:
+			panic(fmt.Sprintf("invalid type %v", v.Type))
+		case checker.DetailInfo:
+			eni++
+		case checker.DetailDebug:
+			end++
+		case checker.DetailWarn:
+			enw++
+		}
+	}
+	return enw == nw &&
+		eni == ni &&
+		end == nd
+}
+
+type TestDetailLogger struct {
+	messages []checker.CheckDetail
+}
+
+type TestReturn struct {
+	Errors        []error
+	Score         int
+	NumberOfWarn  int
+	NumberOfInfo  int
+	NumberOfDebug int
+}
+
+func (l *TestDetailLogger) Info(desc string, args ...interface{}) {
+	cd := checker.CheckDetail{Type: checker.DetailInfo, Msg: fmt.Sprintf(desc, args...)}
+	l.messages = append(l.messages, cd)
+}
+
+func (l *TestDetailLogger) Warn(desc string, args ...interface{}) {
+	cd := checker.CheckDetail{Type: checker.DetailWarn, Msg: fmt.Sprintf(desc, args...)}
+	l.messages = append(l.messages, cd)
+}
+
+func (l *TestDetailLogger) Debug(desc string, args ...interface{}) {
+	cd := checker.CheckDetail{Type: checker.DetailDebug, Msg: fmt.Sprintf(desc, args...)}
+	l.messages = append(l.messages, cd)
+}
+
+//nolint
+func ValidateTestReturn(t *testing.T, name string, te *TestReturn,
+	tr *checker.CheckResult, dl *TestDetailLogger) bool {
+	for _, we := range te.Errors {
+		if !errors.Is(tr.Error2, we) {
+			if t != nil {
+				t.Errorf("%v: invalid error returned: %v is not of type %v",
+					name, tr.Error, we)
+			}
+			return false
+		}
+	}
+	// UPGRADEv2: update name.
+	if tr.Score != te.Score ||
+		!validateDetailTypes(dl.messages, te.NumberOfWarn,
+			te.NumberOfInfo, te.NumberOfDebug) {
+		if t != nil {
+			t.Errorf("%v: Got (score=%v) expected (%v)\n%v",
+				name, tr.Score, te.Score, dl.messages)
+		}
+		return false
+	}
+	return true
+}