From 5e634c89458ba9e4ad8186456506a4d28b23cdf5 Mon Sep 17 00:00:00 2001 From: laurentsimon <64505099+laurentsimon@users.noreply.github.com> Date: Wed, 21 Jul 2021 09:02:43 -0700 Subject: [PATCH] =?UTF-8?q?=E2=9C=A8=20[migration=20to=20score]=202:=20dep?= =?UTF-8?q?endabot=20and=20binary=20artifact=20checks=20(#718)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * details-1 * nits * typo * commments * dependabot and binary artifacts checks * typo * linter * missing errors.go * linter * merge fix * dates --- checks/automatic_dependency_update.go | 19 ++++-- checks/binary_artifact.go | 26 +++++--- checks/checkforcontent.go | 54 ++++++++++++--- checks/checkforfile.go | 16 +++++ checks/errors.go | 2 + e2e/automatic_deps_test.go | 43 +++++++++--- e2e/binary_artifacts_test.go | 96 +++++++++++++++++++++++++++ errors/errors.md | 6 +- pkg/scorecard_result.go | 5 +- utests/utlib.go | 96 +++++++++++++++++++++++++++ 10 files changed, 326 insertions(+), 37 deletions(-) create mode 100644 e2e/binary_artifacts_test.go create mode 100644 utests/utlib.go diff --git a/checks/automatic_dependency_update.go b/checks/automatic_dependency_update.go index d0fb301e..2ecab308 100644 --- a/checks/automatic_dependency_update.go +++ b/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 diff --git a/checks/binary_artifact.go b/checks/binary_artifact.go index e607928e..fb24c904 100644 --- a/checks/binary_artifact.go +++ b/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 } diff --git a/checks/checkforcontent.go b/checks/checkforcontent.go index 66d7242d..0f8af8bb 100644 --- a/checks/checkforcontent.go +++ b/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 +} diff --git a/checks/checkforfile.go b/checks/checkforfile.go index 7de6812e..8c9d9685 100644 --- a/checks/checkforfile.go +++ b/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 +} diff --git a/checks/errors.go b/checks/errors.go index a1e8e5c2..7cbf569e 100644 --- a/checks/errors.go +++ b/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 diff --git a/e2e/automatic_deps_test.go b/e2e/automatic_deps_test.go index 22c81c99..489cc7d9 100644 --- a/e2e/automatic_deps_test.go +++ b/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()) }) }) }) diff --git a/e2e/binary_artifacts_test.go b/e2e/binary_artifacts_test.go new file mode 100644 index 00000000..9df3a5b1 --- /dev/null +++ b/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()) + }) + }) +}) diff --git a/errors/errors.md b/errors/errors.md index 60607297..daf86f19 100644 --- a/errors/errors.md +++ b/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)) } ``` \ No newline at end of file diff --git a/pkg/scorecard_result.go b/pkg/scorecard_result.go index 4bbbb805..b62627b1 100644 --- a/pkg/scorecard_result.go +++ b/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 diff --git a/utests/utlib.go b/utests/utlib.go new file mode 100644 index 00000000..1391ea35 --- /dev/null +++ b/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 +}