diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index f3fd55feedc..fbdc49ed10e 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -12,6 +12,7 @@ project. This document describes the contribution guidelines for the project. * [Getting started](#getting-started) * [Environment Setup](#environment-setup) * [Contributing steps](#contributing-steps) +* [Error handling](#error-handling) * [How to build scorecard locally](#how-to-build-scorecard-locally) * [PR Process](#pr-process) * [What to do before submitting a pull request](#what-to-do-before-submitting-a-pull-request) @@ -52,6 +53,10 @@ You must install these tools: 1. Fork the desired repo, develop and test your code changes. 1. Submit a pull request. +## Error handling + +See [errors/errors.md]. + ## How to build scorecard locally Note that, by building the scorecard from the source code we are allowed to test @@ -140,22 +145,7 @@ Submit a PR for this file and scorecard would start scanning in subsequent runs. ## Adding New Checks -Each check is currently just a function of type `CheckFn`. The signature is: - -```golang -type CheckFn func(*c.Checker) CheckResult -``` - -Checks are registered in an init function: - -```golang -const codeReviewStr = "Code-Review" - -//nolint:gochecknoinits -func init() { - registerCheck(codeReviewStr, DoesCodeReview) -} -``` +See [checks/write.md](checks/write.md). ### Updating Docs diff --git a/checker/check_request.go b/checker/check_request.go index 004d0fd7ef9..661d3d8c7ba 100644 --- a/checker/check_request.go +++ b/checker/check_request.go @@ -30,6 +30,9 @@ type CheckRequest struct { GraphClient *githubv4.Client HTTPClient *http.Client RepoClient clients.RepoClient + // Note: Ultimately Log will be removed and replaced by + // CLogger. Logf func(s string, f ...interface{}) + Dlogger DetailLogger Owner, Repo string } diff --git a/checker/check_result.go b/checker/check_result.go index ccce9c9ae02..0c14e05955b 100644 --- a/checker/check_result.go +++ b/checker/check_result.go @@ -16,30 +16,180 @@ package checker import ( "errors" + "fmt" + "math" scorecarderrors "github.com/ossf/scorecard/errors" ) -const MaxResultConfidence = 10 +// UPGRADEv2: to remove. +const ( + MaxResultConfidence = 10 + HalfResultConfidence = 5 + MinResultConfidence = 0 +) // ErrorDemoninatorZero indicates the denominator for a proportional result is 0. +// UPGRADEv2: to remove. var ErrorDemoninatorZero = errors.New("internal error: denominator is 0") +//nolint +const ( + MaxResultScore = 10 + HalfResultScore = 5 + MinResultScore = 0 + InconclusiveResultScore = -1 +) + +//nolint type CheckResult struct { + // Old structure Error error `json:"-"` Name string Details []string Confidence int Pass bool ShouldRetry bool `json:"-"` + + // UPGRADEv2: New structure. Omitting unchanged Name field + // for simplicity. + Version int // 2. Default value of 0 indicates old structure + Error2 error `json:"-"` // Runtime error indicate a filure to run the check. + Details2 []CheckDetail `json:"-"` // Details of tests and sub-checks + Score2 int `json:"-"` // {[0...1], -1 = Inconclusive} + Reason2 string `json:"-"` // A sentence describing the check result (score, etc) +} + +// CreateResultWithScore is used when +// the check runs without runtime errors and want to assign a +// specific score. +func CreateResultWithScore(name, reason string, score int) CheckResult { + pass := true + //nolint + if score < 8 { + pass = false + } + return CheckResult{ + Name: name, + // Old structure. + Error: nil, + Confidence: MaxResultScore, + Pass: pass, + ShouldRetry: false, + // New structure. + //nolint + Version: 2, + Error2: nil, + Score2: score, + Reason2: reason, + } +} + +// CreateProportionalScoreResult is used when +// the check runs without runtime errors and we assign a +// proportional score. This may be used if a check contains +// multiple tests and we want to assign a score proportional +// the the number of tests that succeeded. +func CreateProportionalScoreResult(name, reason string, b, t int) CheckResult { + pass := true + //nolint + score := int(math.Min(float64(10*b/t), float64(10))) + //nolint + if score < 8 { + pass = false + } + return CheckResult{ + Name: name, + // Old structure. + Error: nil, + //nolint + Confidence: 10, + Pass: pass, + ShouldRetry: false, + // New structure. + //nolint + Version: 2, + Error2: nil, + Score2: score, + Reason2: fmt.Sprintf("%v -- score normalized to %d", reason, score), + } +} + +// CreateMaxScoreResult is used when +// the check runs without runtime errors and we can assign a +// maximum score to the result. +func CreateMaxScoreResult(name, reason string) CheckResult { + return CreateResultWithScore(name, reason, MaxResultScore) +} + +// CreateMinScoreResult is used when +// the check runs without runtime errors and we can assign a +// minimum score to the result. +func CreateMinScoreResult(name, reason string) CheckResult { + return CreateResultWithScore(name, reason, MinResultScore) } +// CreateInconclusiveResult is used when +// the check runs without runtime errors, but we don't +// have enough evidence to set a score. +func CreateInconclusiveResult(name, reason string) CheckResult { + return CheckResult{ + Name: name, + // Old structure. + Confidence: 0, + Pass: false, + ShouldRetry: false, + // New structure. + //nolint + Version: 2, + Score2: InconclusiveResultScore, + Reason2: reason, + } +} + +// CreateRuntimeErrorResult is used when the check fails to run because of a runtime error. +func CreateRuntimeErrorResult(name string, e error) CheckResult { + return CheckResult{ + Name: name, + // Old structure. + Error: e, + Confidence: 0, + Pass: false, + ShouldRetry: false, + // New structure. + //nolint + Version: 2, + Error2: e, + Score2: InconclusiveResultScore, + Reason2: e.Error(), // Note: message already accessible by caller thru `Error`. + } +} + +// UPGRADEv2: will be renamed. +func MakeAndResult2(checks ...CheckResult) CheckResult { + if len(checks) == 0 { + // That should never happen. + panic("MakeResult called with no checks") + } + + worseResult := checks[0] + // UPGRADEv2: will go away after old struct is removed. + //nolint + for _, result := range checks[1:] { + if result.Score2 < worseResult.Score2 { + worseResult = result + } + } + return worseResult +} + +// UPGRADEv2: will be removed. func MakeInconclusiveResult(name string, err error) CheckResult { return CheckResult{ Name: name, Pass: false, Confidence: 0, - Error: scorecarderrors.MakeZeroConfidenceError(err), + Error: scorecarderrors.MakeLowConfidenceError(err), } } @@ -48,6 +198,7 @@ func MakePassResult(name string) CheckResult { Name: name, Pass: true, Confidence: MaxResultConfidence, + Error: nil, } } @@ -69,6 +220,8 @@ func MakeRetryResult(name string, err error) CheckResult { } } +// TODO: update this function to return a ResultDontKnow +// if the confidence is low? func MakeProportionalResult(name string, numerator int, denominator int, threshold float32) CheckResult { if denominator == 0 { @@ -98,6 +251,7 @@ func MakeProportionalResult(name string, numerator int, denominator int, } // Given a min result, check if another result is worse. +//nolint func isMinResult(result, min CheckResult) bool { if Bool2int(result.Pass) < Bool2int(min.Pass) { return true @@ -117,7 +271,8 @@ func MakeAndResult(checks ...CheckResult) CheckResult { Pass: true, Confidence: MaxResultConfidence, } - + // UPGRADEv2: will go away after old struct is removed. + //nolint for _, result := range checks { if minResult.Name == "" { minResult.Name = result.Name diff --git a/checker/check_runner.go b/checker/check_runner.go index a9843dd728a..08855e9a10f 100644 --- a/checker/check_runner.go +++ b/checker/check_runner.go @@ -39,15 +39,56 @@ type CheckFn func(*CheckRequest) CheckResult type CheckNameToFnMap map[string]CheckFn +// Types of details. +type DetailType int + +const ( + DetailInfo DetailType = iota + DetailWarn + DetailDebug +) + +// CheckDetail contains information for each detail. +//nolint:govet +type CheckDetail struct { + Type DetailType // Any of DetailWarn, DetailInfo, DetailDebug. + Msg string // A short string explaining why the details was recorded/logged.. +} + +type DetailLogger interface { + Info(desc string, args ...interface{}) + Warn(desc string, args ...interface{}) + Debug(desc string, args ...interface{}) +} + +// UPGRADEv2: messages2 will ultimately +// be renamed to messages. type logger struct { - messages []string + messages []string + messages2 []CheckDetail +} + +func (l *logger) Info(desc string, args ...interface{}) { + cd := CheckDetail{Type: DetailInfo, Msg: fmt.Sprintf(desc, args...)} + l.messages2 = append(l.messages2, cd) } +func (l *logger) Warn(desc string, args ...interface{}) { + cd := CheckDetail{Type: DetailWarn, Msg: fmt.Sprintf(desc, args...)} + l.messages2 = append(l.messages2, cd) +} + +func (l *logger) Debug(desc string, args ...interface{}) { + cd := CheckDetail{Type: DetailDebug, Msg: fmt.Sprintf(desc, args...)} + l.messages2 = append(l.messages2, cd) +} + +// UPGRADEv2: to remove. func (l *logger) Logf(s string, f ...interface{}) { l.messages = append(l.messages, fmt.Sprintf(s, f...)) } -func logStats(ctx context.Context, startTime time.Time, result CheckResult) error { +func logStats(ctx context.Context, startTime time.Time, result *CheckResult) error { runTimeInSecs := time.Now().Unix() - startTime.Unix() opencensusstats.Record(ctx, stats.CheckRuntimeInSec.M(runTimeInSecs)) @@ -74,17 +115,22 @@ func (r *Runner) Run(ctx context.Context, f CheckFn) CheckResult { checkRequest := r.CheckRequest checkRequest.Ctx = ctx l = logger{} + // UPGRADEv2: to remove. checkRequest.Logf = l.Logf + checkRequest.Dlogger = &l res = f(&checkRequest) + // UPGRADEv2: to fix using proper error check. if res.ShouldRetry && !strings.Contains(res.Error.Error(), "invalid header field value") { checkRequest.Logf("error, retrying: %s", res.Error) continue } break } + // UPGRADEv2: to remove. res.Details = l.messages + res.Details2 = l.messages2 - if err := logStats(ctx, startTime, res); err != nil { + if err := logStats(ctx, startTime, &res); err != nil { panic(err) } return res @@ -97,6 +143,35 @@ func Bool2int(b bool) int { return 0 } +// UPGRADEv2: will be renamed. +func MultiCheckOr2(fns ...CheckFn) CheckFn { + return func(c *CheckRequest) CheckResult { + //nolint + maxResult := CheckResult{Version: 2} + + for _, fn := range fns { + result := fn(c) + + if result.Score2 > maxResult.Score2 { + maxResult = result + } + } + return maxResult + } +} + +func MultiCheckAnd2(fns ...CheckFn) CheckFn { + return func(c *CheckRequest) CheckResult { + var checks []CheckResult + for _, fn := range fns { + res := fn(c) + checks = append(checks, res) + } + return MakeAndResult2(checks...) + } +} + +// UPGRADEv2: will be removed. // MultiCheckOr returns the best check result out of several ones performed. func MultiCheckOr(fns ...CheckFn) CheckFn { return func(c *CheckRequest) CheckResult { diff --git a/checker/check_test.go b/checker/check_test.go index 99ad6f6fa80..b0e18517cbd 100644 --- a/checker/check_test.go +++ b/checker/check_test.go @@ -27,8 +27,8 @@ func TestMakeCheckAnd(t *testing.T) { t.Parallel() tests := []struct { name string - checks []CheckResult want CheckResult + checks []CheckResult }{ { name: "Multiple passing", diff --git a/checks/active.go b/checks/active.go index f57781698ba..8a36f627866 100644 --- a/checks/active.go +++ b/checks/active.go @@ -15,17 +15,20 @@ package checks import ( + "fmt" "time" "github.com/google/go-github/v32/github" "github.com/ossf/scorecard/checker" + sce "github.com/ossf/scorecard/errors" ) const ( - // CheckActive is the registered name for IsActive. - CheckActive = "Active" - lookbackDays = 90 + CheckActive = "Active" + lookBackDays = 90 + commitsPerWeek = 1 + daysInOneWeek = 7 ) //nolint:gochecknoinits @@ -36,30 +39,27 @@ func init() { func IsActive(c *checker.CheckRequest) checker.CheckResult { commits, _, err := c.Client.Repositories.ListCommits(c.Ctx, c.Owner, c.Repo, &github.CommitsListOptions{}) if err != nil { - return checker.MakeRetryResult(CheckActive, err) + return checker.CreateRuntimeErrorResult(CheckActive, err) } tz, err := time.LoadLocation("UTC") if err != nil { - return checker.MakeRetryResult(CheckActive, err) + return checker.CreateRuntimeErrorResult(CheckActive, sce.Create(sce.ErrRunFailure, fmt.Sprintf("time.LoadLocation: %v", err))) } - threshold := time.Now().In(tz).AddDate(0, 0, -1*lookbackDays) + threshold := time.Now().In(tz).AddDate(0, 0, -1*lookBackDays) totalCommits := 0 for _, commit := range commits { commitFull, _, err := c.Client.Git.GetCommit(c.Ctx, c.Owner, c.Repo, commit.GetSHA()) if err != nil { - return checker.MakeRetryResult(CheckActive, err) + return checker.CreateRuntimeErrorResult(CheckActive, err) } if commitFull.GetAuthor().GetDate().After(threshold) { totalCommits++ } } - c.Logf("commits in last %d days: %d", lookbackDays, totalCommits) - const numCommits = 2 - const confidence = 10 - return checker.CheckResult{ - Name: CheckActive, - Pass: totalCommits >= numCommits, - Confidence: confidence, - } + + return checker.CreateProportionalScoreResult(CheckActive, + fmt.Sprintf("%d commit(s) found in the last %d days", totalCommits, lookBackDays), + totalCommits, commitsPerWeek*lookBackDays/daysInOneWeek) + return checker.CreateMinScoreResult(CheckActive, fmt.Sprintf("no commit found in the last %d days", lookBackDays)) } diff --git a/checks/automatic_dependency_update.go b/checks/automatic_dependency_update.go index d0fb301ecec..2ecab308ff2 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 e607928e148..f5e0e0e72f5 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.ErrRunFailure, 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/branch_protected.go b/checks/branch_protected.go index 286eb1b5371..0f6e40d1665 100644 --- a/checks/branch_protected.go +++ b/checks/branch_protected.go @@ -16,18 +16,18 @@ package checks import ( "context" - "errors" + "fmt" "regexp" "github.com/google/go-github/v32/github" "github.com/ossf/scorecard/checker" + sce "github.com/ossf/scorecard/errors" ) const ( - // CheckBranchProtection is the registered name for BranchProtection. CheckBranchProtection = "Branch-Protection" - minReviews = 1 + minReviews = 2 ) //nolint:gochecknoinits @@ -46,31 +46,23 @@ type repositories interface { *github.Protection, *github.Response, error) } -type logger func(s string, f ...interface{}) - -// ErrCommitishNil TargetCommitish nil for release. -var ErrCommitishNil = errors.New("target_commitish is nil for release") - -// ErrBranchNotFound branch from TargetCommitish not found. -var ErrBranchNotFound = errors.New("branch not found") - func BranchProtection(c *checker.CheckRequest) checker.CheckResult { - // Checks branch protection on both release and development branch - return checkReleaseAndDevBranchProtection(c.Ctx, c.Client.Repositories, c.Logf, c.Owner, c.Repo) + // Checks branch protection on both release and development branch. + return checkReleaseAndDevBranchProtection(c.Ctx, c.Client.Repositories, c.Dlogger, c.Owner, c.Repo) } -func checkReleaseAndDevBranchProtection(ctx context.Context, r repositories, l logger, ownerStr, +func checkReleaseAndDevBranchProtection(ctx context.Context, r repositories, dl checker.DetailLogger, ownerStr, repoStr string) checker.CheckResult { // Get all branches. This will include information on whether they are protected. branches, _, err := r.ListBranches(ctx, ownerStr, repoStr, &github.BranchListOptions{}) if err != nil { - return checker.MakeRetryResult(CheckBranchProtection, err) + return checker.CreateRuntimeErrorResult(CheckBranchProtection, err) } // Get release branches releases, _, err := r.ListReleases(ctx, ownerStr, repoStr, &github.ListOptions{}) if err != nil { - return checker.MakeRetryResult(CheckBranchProtection, err) + return checker.CreateRuntimeErrorResult(CheckBranchProtection, err) } var checks []checker.CheckResult @@ -79,7 +71,9 @@ func checkReleaseAndDevBranchProtection(ctx context.Context, r repositories, l l for _, release := range releases { if release.TargetCommitish == nil { // Log with a named error if target_commitish is nil. - checks = append(checks, checker.MakeFailResult(CheckBranchProtection, ErrCommitishNil)) + r := checker.CreateRuntimeErrorResult(CheckBranchProtection, + sce.Create(sce.ErrRunFailure, sce.ErrCommitishNil.Error())) + checks = append(checks, r) continue } @@ -92,7 +86,9 @@ func checkReleaseAndDevBranchProtection(ctx context.Context, r repositories, l l name, err := resolveBranchName(branches, *release.TargetCommitish) if err != nil { // If the commitish branch is still not found, fail. - checks = append(checks, checker.MakeFailResult(CheckBranchProtection, ErrBranchNotFound)) + r := checker.CreateRuntimeErrorResult(CheckBranchProtection, + sce.Create(sce.ErrRunFailure, sce.ErrBranchNotFound.Error())) + checks = append(checks, r) continue } @@ -100,10 +96,10 @@ func checkReleaseAndDevBranchProtection(ctx context.Context, r repositories, l l checkBranches[*name] = true } - // Add default branch + // Add default branch. repo, _, err := r.Get(ctx, ownerStr, repoStr) if err != nil { - return checker.MakeRetryResult(CheckBranchProtection, err) + return checker.CreateRuntimeErrorResult(CheckBranchProtection, err) } checkBranches[*repo.DefaultBranch] = true @@ -111,23 +107,20 @@ func checkReleaseAndDevBranchProtection(ctx context.Context, r repositories, l l for b := range checkBranches { protected, err := isBranchProtected(branches, b) if err != nil { - checks = append(checks, checker.MakeFailResult(CheckBranchProtection, ErrBranchNotFound)) + r := checker.CreateRuntimeErrorResult(CheckBranchProtection, sce.Create(sce.ErrRunFailure, sce.ErrBranchNotFound.Error())) + checks = append(checks, r) } if !protected { - l("!! branch protection not enabled for branch %s", b) - checks = append(checks, checker.CheckResult{ - Name: CheckBranchProtection, - Pass: false, - Confidence: checker.MaxResultConfidence, - }) + r := checker.CreateMinScoreResult(CheckBranchProtection, fmt.Sprintf("branch protection not enabled for branch '%s'", b)) + checks = append(checks, r) } else { // The branch is protected. Check the protection. - res := getProtectionAndCheck(ctx, r, l, ownerStr, repoStr, b) + res := getProtectionAndCheck(ctx, r, dl, ownerStr, repoStr, b) checks = append(checks, res) } } - return checker.MakeAndResult(checks...) + return checker.MakeAndResult2(checks...) } func resolveBranchName(branches []*github.Branch, name string) (*string, error) { @@ -144,7 +137,7 @@ func resolveBranchName(branches []*github.Branch, name string) (*string, error) return resolveBranchName(branches, "main") } - return nil, ErrBranchNotFound + return nil, sce.Create(sce.ErrRunFailure, sce.ErrBranchNotFound.Error()) } func isBranchProtected(branches []*github.Branch, name string) (bool, error) { @@ -154,71 +147,78 @@ func isBranchProtected(branches []*github.Branch, name string) (bool, error) { return b.GetProtected(), nil } } - return false, ErrBranchNotFound + + return false, sce.Create(sce.ErrRunFailure, sce.ErrBranchNotFound.Error()) } -func getProtectionAndCheck(ctx context.Context, r repositories, l logger, ownerStr, repoStr, +func getProtectionAndCheck(ctx context.Context, r repositories, dl checker.DetailLogger, ownerStr, repoStr, branch string) checker.CheckResult { // We only call this if the branch is protected. An error indicates not found. protection, resp, err := r.GetBranchProtection(ctx, ownerStr, repoStr, branch) const fileNotFound = 404 if resp.StatusCode == fileNotFound { - return checker.MakeRetryResult(CheckBranchProtection, err) + return checker.CreateRuntimeErrorResult(CheckBranchProtection, sce.Create(sce.ErrRunFailure, err.Error())) } - return IsBranchProtected(protection, branch, l) + return IsBranchProtected(protection, branch, dl) } -func IsBranchProtected(protection *github.Protection, branch string, l logger) checker.CheckResult { - totalChecks := 6 +func IsBranchProtected(protection *github.Protection, branch string, dl checker.DetailLogger) checker.CheckResult { + totalChecks := 10 totalSuccess := 0 // This is disabled by default (good). if protection.GetAllowForcePushes() != nil && protection.AllowForcePushes.Enabled { - l("!! branch protection - AllowForcePushes should be disabled on %s", branch) + dl.Warn("AllowForcePushes enabled on branch '%s'", branch) } else { + dl.Info("AllowForcePushes disabled on branch '%s'", branch) totalSuccess++ } // This is disabled by default (good). if protection.GetAllowDeletions() != nil && protection.AllowDeletions.Enabled { - l("!! branch protection - AllowDeletions should be disabled on %s", branch) + dl.Warn("AllowDeletions enabled on branch '%s'", branch) } else { + dl.Info("AllowDeletions disabled on branch '%s'", branch) totalSuccess++ } // This is disabled by default (bad). if protection.GetEnforceAdmins() != nil && protection.EnforceAdmins.Enabled { + dl.Info("EnforceAdmins disabled on branch '%s'", branch) totalSuccess++ } else { - l("!! branch protection - EnforceAdmins should be enabled on %s", branch) + dl.Warn("EnforceAdmins disabled on branch '%s'", branch) } // This is disabled by default (bad). if protection.GetRequireLinearHistory() != nil && protection.RequireLinearHistory.Enabled { + dl.Info("Linear history enabled on branch '%s'", branch) totalSuccess++ } else { - l("!! branch protection - Linear history should be enabled on %s", branch) + dl.Warn("Linear history disabled on branch '%s'", branch) } - if requiresStatusChecks(protection, branch, l) { + if requiresStatusChecks(protection, branch, dl) { + dl.Info("Strict status check enabled on branch '%s'", branch) totalSuccess++ } - if requiresThoroughReviews(protection, branch, l) { + if requiresThoroughReviews(protection, branch, dl) { totalSuccess++ } - return checker.MakeProportionalResult(CheckBranchProtection, totalSuccess, totalChecks, 1.0) + return checker.CreateProportionalScoreResult(CheckBranchProtection, + "%d out of %d branch protection settings are enabled", totalSuccess, totalChecks) } // Returns true if several PR status checks requirements are enabled. Otherwise returns false and logs why it failed. -func requiresStatusChecks(protection *github.Protection, branch string, l logger) bool { +func requiresStatusChecks(protection *github.Protection, branch string, dl checker.DetailLogger) bool { // This is disabled by default (bad). if protection.GetRequiredStatusChecks() != nil && protection.RequiredStatusChecks.Strict && @@ -228,17 +228,17 @@ func requiresStatusChecks(protection *github.Protection, branch string, l logger switch { case protection.RequiredStatusChecks == nil || !protection.RequiredStatusChecks.Strict: - l("!! branch protection - Status checks for merging should be enabled on %s", branch) + dl.Warn("Status checks for merging disabled on branch '%s'", branch) case len(protection.RequiredStatusChecks.Contexts) == 0: - l("!! branch protection - Status checks for merging should have specific status to check for on %s", branch) + dl.Warn("Status checks for merging have no specific status to check on branch '%s'", branch) default: - panic("!! branch protection - Unhandled status checks error") + panic("Unhandled status checks error") } return false } // Returns true if several PR review requirements are enabled. Otherwise returns false and logs why it failed. -func requiresThoroughReviews(protection *github.Protection, branch string, l logger) bool { +func requiresThoroughReviews(protection *github.Protection, branch string, dl checker.DetailLogger) bool { // This is disabled by default (bad). if protection.GetRequiredPullRequestReviews() != nil && protection.RequiredPullRequestReviews.RequiredApprovingReviewCount >= minReviews && @@ -248,18 +248,19 @@ func requiresThoroughReviews(protection *github.Protection, branch string, l log } switch { case protection.RequiredPullRequestReviews == nil: - l("!! branch protection - Pullrequest reviews should be enabled on %s", branch) + dl.Warn("Pullrequest reviews disabled on branch '%s'", branch) fallthrough case protection.RequiredPullRequestReviews.RequiredApprovingReviewCount < minReviews: - l("!! branch protection - %v pullrequest reviews should be enabled on %s", minReviews, branch) + dl.Warn("Number of required reviewers is only %d on branch '%s'", + protection.RequiredPullRequestReviews.RequiredApprovingReviewCount, branch) fallthrough case !protection.RequiredPullRequestReviews.DismissStaleReviews: - l("!! branch protection - Stale review dismissal should be enabled on %s", branch) + dl.Warn("Stale review dismissal disabled on branch '%s'", branch) fallthrough case !protection.RequiredPullRequestReviews.RequireCodeOwnerReviews: - l("!! branch protection - Owner review should be enabled on %s", branch) + dl.Warn("Owner review not required on branch '%s'", branch) default: - panic("!! branch protection - Unhandled pull request error") + panic("Unhandled pull request error") } return false } diff --git a/checks/branch_protected_test.go b/checks/branch_protected_test.go index be5845c4b45..b2e8e5eacc2 100644 --- a/checks/branch_protected_test.go +++ b/checks/branch_protected_test.go @@ -16,24 +16,16 @@ package checks import ( "context" - "fmt" "net/http" "testing" "github.com/google/go-github/v32/github" "github.com/ossf/scorecard/checker" + sce "github.com/ossf/scorecard/errors" + scut "github.com/ossf/scorecard/utests" ) -// TODO: these logging functions are repeated from lib/check_fn.go. Reuse code. -type log struct { - messages []string -} - -func (l *log) Logf(s string, f ...interface{}) { - l.messages = append(l.messages, fmt.Sprintf(s, f...)) -} - type mockRepos struct { branches []*string protections map[string]*github.Protection @@ -68,7 +60,8 @@ func (m mockRepos) GetBranchProtection(ctx context.Context, o string, r string, return nil, &github.Response{ Response: &http.Response{StatusCode: http.StatusNotFound}, }, - ErrBranchNotFound + //nolint + sce.Create(sce.ErrRunFailure, sce.ErrBranchNotFound.Error()) } func (m mockRepos) ListBranches(ctx context.Context, owner string, repo string, @@ -81,23 +74,30 @@ func (m mockRepos) ListBranches(ctx context.Context, owner string, repo string, return res, nil, nil } -func TestReleaseAndDevBranchProtected(t *testing.T) { //nolint:tparallel // mocks return different results per test case +func TestReleaseAndDevBranchProtected(t *testing.T) { t.Parallel() - l := log{} rel1 := "release/v.1" sha := "8fb3cb86082b17144a80402f5367ae65f06083bd" main := "main" + //nolint tests := []struct { name string + expected scut.TestReturn branches []*string defaultBranch *string releases []*string protections map[string]*github.Protection - want checker.CheckResult }{ { - name: "Only development branch", + name: "Only development branch", + expected: scut.TestReturn{ + Errors: nil, + Score: 2, + NumberOfWarn: 6, + NumberOfInfo: 2, + NumberOfDebug: 0, + }, defaultBranch: &main, branches: []*string{&rel1, &main}, releases: nil, @@ -136,17 +136,16 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { //nolint:tparallel // mock }, }, }, - want: checker.CheckResult{ - Name: CheckBranchProtection, - Pass: false, - Details: nil, - Confidence: 7, - ShouldRetry: false, - Error: nil, - }, }, { - name: "Take worst of release and development", + name: "Take worst of release and development", + expected: scut.TestReturn{ + Errors: nil, + Score: 2, + NumberOfWarn: 9, + NumberOfInfo: 7, + NumberOfDebug: 0, + }, defaultBranch: &main, branches: []*string{&rel1, &main}, releases: []*string{&rel1}, @@ -218,17 +217,16 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { //nolint:tparallel // mock }, }, }, - want: checker.CheckResult{ - Name: CheckBranchProtection, - Pass: false, - Details: nil, - Confidence: 7, - ShouldRetry: false, - Error: nil, - }, }, { - name: "Both release and development are OK", + name: "Both release and development are OK", + expected: scut.TestReturn{ + Errors: nil, + Score: 5, + NumberOfWarn: 6, + NumberOfInfo: 10, + NumberOfDebug: 0, + }, defaultBranch: &main, branches: []*string{&rel1, &main}, releases: []*string{&rel1}, @@ -300,17 +298,16 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { //nolint:tparallel // mock }, }, }, - want: checker.CheckResult{ - Name: CheckBranchProtection, - Pass: true, - Details: nil, - Confidence: 10, - ShouldRetry: false, - Error: nil, - }, }, { - name: "Ignore a non-branch targetcommitish", + name: "Ignore a non-branch targetcommitish", + expected: scut.TestReturn{ + Errors: nil, + Score: 2, + NumberOfWarn: 6, + NumberOfInfo: 2, + NumberOfDebug: 0, + }, defaultBranch: &main, branches: []*string{&rel1, &main}, releases: []*string{&sha}, @@ -349,17 +346,16 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { //nolint:tparallel // mock }, }, }, - want: checker.CheckResult{ - Name: CheckBranchProtection, - Pass: false, - Details: nil, - Confidence: 7, - ShouldRetry: false, - Error: nil, - }, }, { - name: "TargetCommittish nil", + name: "TargetCommittish nil", + expected: scut.TestReturn{ + Errors: []error{sce.ErrRunFailure}, + Score: checker.InconclusiveResultScore, + NumberOfWarn: 6, + NumberOfInfo: 2, + NumberOfDebug: 0, + }, defaultBranch: &main, branches: []*string{&main}, releases: []*string{nil}, @@ -398,476 +394,443 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { //nolint:tparallel // mock }, }, }, - want: checker.CheckResult{ - Name: CheckBranchProtection, - Pass: false, - Details: nil, - Confidence: 10, - ShouldRetry: false, - Error: ErrCommitishNil, - }, }, } - for _, tt := range tests { //nolint:paralleltest // mocks return different results per test case + for _, tt := range tests { tt := tt // Re-initializing variable so it is not changed while executing the closure below - l.messages = []string{} - t.Run(tt.name, func(t *testing.T) { + t.Parallel() m := mockRepos{ defaultBranch: tt.defaultBranch, branches: tt.branches, releases: tt.releases, protections: tt.protections, } - got := checkReleaseAndDevBranchProtection(context.Background(), m, - l.Logf, "testowner", "testrepo") - got.Details = l.messages - if got.Confidence != tt.want.Confidence || got.Pass != tt.want.Pass { - t.Errorf("IsBranchProtected() = %s, %v, want %v", tt.name, got, tt.want) - } + dl := scut.TestDetailLogger{} + r := checkReleaseAndDevBranchProtection(context.Background(), m, + &dl, "testowner", "testrepo") + scut.ValidateTestReturn2(t, tt.name, &tt.expected, &r, &dl) }) } } func TestIsBranchProtected(t *testing.T) { t.Parallel() - type args struct { - protection *github.Protection - } - l := log{} tests := []struct { - name string - args args - want checker.CheckResult + name string + protection *github.Protection + expected scut.TestReturn }{ { name: "Nothing is enabled", - args: args{ - protection: &github.Protection{ - RequiredStatusChecks: &github.RequiredStatusChecks{ - Strict: false, - Contexts: nil, - }, - RequiredPullRequestReviews: &github.PullRequestReviewsEnforcement{ - DismissalRestrictions: &github.DismissalRestrictions{ - Users: nil, - Teams: nil, - }, - DismissStaleReviews: false, - RequireCodeOwnerReviews: false, - RequiredApprovingReviewCount: 0, - }, - EnforceAdmins: &github.AdminEnforcement{ - URL: nil, - Enabled: false, - }, - Restrictions: &github.BranchRestrictions{ + expected: scut.TestReturn{ + Errors: nil, + Score: 5, + NumberOfWarn: 3, + NumberOfInfo: 5, + NumberOfDebug: 0, + }, + protection: &github.Protection{ + RequiredStatusChecks: &github.RequiredStatusChecks{ + Strict: false, + Contexts: nil, + }, + RequiredPullRequestReviews: &github.PullRequestReviewsEnforcement{ + DismissalRestrictions: &github.DismissalRestrictions{ Users: nil, Teams: nil, - Apps: nil, - }, - RequireLinearHistory: &github.RequireLinearHistory{ - Enabled: false, - }, - AllowForcePushes: &github.AllowForcePushes{ - Enabled: false, - }, - AllowDeletions: &github.AllowDeletions{ - Enabled: false, }, + DismissStaleReviews: false, + RequireCodeOwnerReviews: false, + RequiredApprovingReviewCount: 0, + }, + EnforceAdmins: &github.AdminEnforcement{ + URL: nil, + Enabled: false, + }, + Restrictions: &github.BranchRestrictions{ + Users: nil, + Teams: nil, + Apps: nil, + }, + RequireLinearHistory: &github.RequireLinearHistory{ + Enabled: false, + }, + AllowForcePushes: &github.AllowForcePushes{ + Enabled: false, + }, + AllowDeletions: &github.AllowDeletions{ + Enabled: false, }, - }, - want: checker.CheckResult{ - Name: CheckBranchProtection, - Pass: false, - Details: nil, - Confidence: 7, - ShouldRetry: false, - Error: nil, }, }, { name: "Required status check enabled", - args: args{ - protection: &github.Protection{ - RequiredStatusChecks: &github.RequiredStatusChecks{ - Strict: true, - Contexts: []string{"foo"}, - }, - RequiredPullRequestReviews: &github.PullRequestReviewsEnforcement{ - DismissalRestrictions: &github.DismissalRestrictions{ - Users: nil, - Teams: nil, - }, - DismissStaleReviews: false, - RequireCodeOwnerReviews: false, - RequiredApprovingReviewCount: 0, - }, - EnforceAdmins: &github.AdminEnforcement{ - URL: nil, - Enabled: false, - }, - Restrictions: &github.BranchRestrictions{ + expected: scut.TestReturn{ + Errors: nil, + Score: 3, + NumberOfWarn: 5, + NumberOfInfo: 3, + NumberOfDebug: 0, + }, + protection: &github.Protection{ + RequiredStatusChecks: &github.RequiredStatusChecks{ + Strict: true, + Contexts: []string{"foo"}, + }, + RequiredPullRequestReviews: &github.PullRequestReviewsEnforcement{ + DismissalRestrictions: &github.DismissalRestrictions{ Users: nil, Teams: nil, - Apps: nil, - }, - RequireLinearHistory: &github.RequireLinearHistory{ - Enabled: false, - }, - AllowForcePushes: &github.AllowForcePushes{ - Enabled: false, - }, - AllowDeletions: &github.AllowDeletions{ - Enabled: false, }, + DismissStaleReviews: false, + RequireCodeOwnerReviews: false, + RequiredApprovingReviewCount: 0, + }, + EnforceAdmins: &github.AdminEnforcement{ + URL: nil, + Enabled: false, + }, + Restrictions: &github.BranchRestrictions{ + Users: nil, + Teams: nil, + Apps: nil, + }, + RequireLinearHistory: &github.RequireLinearHistory{ + Enabled: false, + }, + AllowForcePushes: &github.AllowForcePushes{ + Enabled: false, + }, + AllowDeletions: &github.AllowDeletions{ + Enabled: false, }, - }, - want: checker.CheckResult{ - Name: CheckBranchProtection, - Pass: false, - Details: nil, - Confidence: 5, - ShouldRetry: false, - Error: nil, }, }, { name: "Required status check enabled without checking for status string", - args: args{ - protection: &github.Protection{ - RequiredStatusChecks: &github.RequiredStatusChecks{ - Strict: true, - Contexts: nil, - }, - RequiredPullRequestReviews: &github.PullRequestReviewsEnforcement{ - DismissalRestrictions: &github.DismissalRestrictions{ - Users: nil, - Teams: nil, - }, - DismissStaleReviews: false, - RequireCodeOwnerReviews: false, - RequiredApprovingReviewCount: 0, - }, - EnforceAdmins: &github.AdminEnforcement{ - URL: nil, - Enabled: false, - }, - Restrictions: &github.BranchRestrictions{ + expected: scut.TestReturn{ + Errors: nil, + Score: 2, + NumberOfWarn: 6, + NumberOfInfo: 2, + NumberOfDebug: 0, + }, + protection: &github.Protection{ + RequiredStatusChecks: &github.RequiredStatusChecks{ + Strict: true, + Contexts: nil, + }, + RequiredPullRequestReviews: &github.PullRequestReviewsEnforcement{ + DismissalRestrictions: &github.DismissalRestrictions{ Users: nil, Teams: nil, - Apps: nil, - }, - RequireLinearHistory: &github.RequireLinearHistory{ - Enabled: false, - }, - AllowForcePushes: &github.AllowForcePushes{ - Enabled: false, - }, - AllowDeletions: &github.AllowDeletions{ - Enabled: false, }, + DismissStaleReviews: false, + RequireCodeOwnerReviews: false, + RequiredApprovingReviewCount: 0, + }, + EnforceAdmins: &github.AdminEnforcement{ + URL: nil, + Enabled: false, + }, + Restrictions: &github.BranchRestrictions{ + Users: nil, + Teams: nil, + Apps: nil, + }, + RequireLinearHistory: &github.RequireLinearHistory{ + Enabled: false, + }, + AllowForcePushes: &github.AllowForcePushes{ + Enabled: false, + }, + AllowDeletions: &github.AllowDeletions{ + Enabled: false, }, - }, - want: checker.CheckResult{ - Name: CheckBranchProtection, - Pass: false, - Details: nil, - Confidence: 7, - ShouldRetry: false, - Error: nil, }, }, - { name: "Required pull request enabled", - args: args{ - protection: &github.Protection{ - RequiredStatusChecks: &github.RequiredStatusChecks{ - Strict: false, - Contexts: []string{"foo"}, - }, - RequiredPullRequestReviews: &github.PullRequestReviewsEnforcement{ - DismissalRestrictions: &github.DismissalRestrictions{ - Users: nil, - Teams: nil, - }, - DismissStaleReviews: false, - RequireCodeOwnerReviews: false, - RequiredApprovingReviewCount: 1, - }, - EnforceAdmins: &github.AdminEnforcement{ - URL: nil, - Enabled: false, - }, - Restrictions: &github.BranchRestrictions{ + expected: scut.TestReturn{ + Errors: nil, + Score: 3, + NumberOfWarn: 5, + NumberOfInfo: 3, + NumberOfDebug: 0, + }, + protection: &github.Protection{ + RequiredStatusChecks: &github.RequiredStatusChecks{ + Strict: false, + Contexts: []string{"foo"}, + }, + RequiredPullRequestReviews: &github.PullRequestReviewsEnforcement{ + DismissalRestrictions: &github.DismissalRestrictions{ Users: nil, Teams: nil, - Apps: nil, - }, - RequireLinearHistory: &github.RequireLinearHistory{ - Enabled: true, - }, - AllowForcePushes: &github.AllowForcePushes{ - Enabled: false, - }, - AllowDeletions: &github.AllowDeletions{ - Enabled: false, }, + DismissStaleReviews: false, + RequireCodeOwnerReviews: false, + RequiredApprovingReviewCount: 1, + }, + EnforceAdmins: &github.AdminEnforcement{ + URL: nil, + Enabled: false, + }, + Restrictions: &github.BranchRestrictions{ + Users: nil, + Teams: nil, + Apps: nil, + }, + RequireLinearHistory: &github.RequireLinearHistory{ + Enabled: true, + }, + AllowForcePushes: &github.AllowForcePushes{ + Enabled: false, + }, + AllowDeletions: &github.AllowDeletions{ + Enabled: false, }, - }, - want: checker.CheckResult{ - Name: CheckBranchProtection, - Pass: false, - Details: nil, - Confidence: 5, - ShouldRetry: false, - Error: nil, }, }, { name: "Required admin enforcement enabled", - args: args{ - protection: &github.Protection{ - RequiredStatusChecks: &github.RequiredStatusChecks{ - Strict: false, - Contexts: []string{"foo"}, - }, - RequiredPullRequestReviews: &github.PullRequestReviewsEnforcement{ - DismissalRestrictions: &github.DismissalRestrictions{ - Users: nil, - Teams: nil, - }, - DismissStaleReviews: false, - RequireCodeOwnerReviews: false, - RequiredApprovingReviewCount: 0, - }, - EnforceAdmins: &github.AdminEnforcement{ - URL: nil, - Enabled: true, - }, - Restrictions: &github.BranchRestrictions{ + expected: scut.TestReturn{ + Errors: nil, + Score: 3, + NumberOfWarn: 5, + NumberOfInfo: 3, + NumberOfDebug: 0, + }, + protection: &github.Protection{ + RequiredStatusChecks: &github.RequiredStatusChecks{ + Strict: false, + Contexts: []string{"foo"}, + }, + RequiredPullRequestReviews: &github.PullRequestReviewsEnforcement{ + DismissalRestrictions: &github.DismissalRestrictions{ Users: nil, Teams: nil, - Apps: nil, - }, - RequireLinearHistory: &github.RequireLinearHistory{ - Enabled: false, - }, - AllowForcePushes: &github.AllowForcePushes{ - Enabled: false, - }, - AllowDeletions: &github.AllowDeletions{ - Enabled: false, }, + DismissStaleReviews: false, + RequireCodeOwnerReviews: false, + RequiredApprovingReviewCount: 0, + }, + EnforceAdmins: &github.AdminEnforcement{ + URL: nil, + Enabled: true, + }, + Restrictions: &github.BranchRestrictions{ + Users: nil, + Teams: nil, + Apps: nil, + }, + RequireLinearHistory: &github.RequireLinearHistory{ + Enabled: false, + }, + AllowForcePushes: &github.AllowForcePushes{ + Enabled: false, + }, + AllowDeletions: &github.AllowDeletions{ + Enabled: false, }, - }, - want: checker.CheckResult{ - Name: CheckBranchProtection, - Pass: false, - Details: nil, - Confidence: 5, - ShouldRetry: false, - Error: nil, }, }, { name: "Required linear history enabled", - args: args{ - protection: &github.Protection{ - RequiredStatusChecks: &github.RequiredStatusChecks{ - Strict: false, - Contexts: []string{"foo"}, - }, - RequiredPullRequestReviews: &github.PullRequestReviewsEnforcement{ - DismissalRestrictions: &github.DismissalRestrictions{ - Users: nil, - Teams: nil, - }, - DismissStaleReviews: false, - RequireCodeOwnerReviews: false, - RequiredApprovingReviewCount: 0, - }, - EnforceAdmins: &github.AdminEnforcement{ - URL: nil, - Enabled: false, - }, - Restrictions: &github.BranchRestrictions{ + expected: scut.TestReturn{ + Errors: nil, + Score: 3, + NumberOfWarn: 5, + NumberOfInfo: 3, + NumberOfDebug: 0, + }, + protection: &github.Protection{ + RequiredStatusChecks: &github.RequiredStatusChecks{ + Strict: false, + Contexts: []string{"foo"}, + }, + RequiredPullRequestReviews: &github.PullRequestReviewsEnforcement{ + DismissalRestrictions: &github.DismissalRestrictions{ Users: nil, Teams: nil, - Apps: nil, - }, - RequireLinearHistory: &github.RequireLinearHistory{ - Enabled: true, - }, - AllowForcePushes: &github.AllowForcePushes{ - Enabled: false, - }, - AllowDeletions: &github.AllowDeletions{ - Enabled: false, }, + DismissStaleReviews: false, + RequireCodeOwnerReviews: false, + RequiredApprovingReviewCount: 0, + }, + EnforceAdmins: &github.AdminEnforcement{ + URL: nil, + Enabled: false, + }, + Restrictions: &github.BranchRestrictions{ + Users: nil, + Teams: nil, + Apps: nil, + }, + RequireLinearHistory: &github.RequireLinearHistory{ + Enabled: true, + }, + AllowForcePushes: &github.AllowForcePushes{ + Enabled: false, + }, + AllowDeletions: &github.AllowDeletions{ + Enabled: false, }, - }, - want: checker.CheckResult{ - Name: CheckBranchProtection, - Pass: false, - Details: nil, - Confidence: 5, - ShouldRetry: false, - Error: nil, }, }, { name: "Allow force push enabled", - args: args{ - protection: &github.Protection{ - RequiredStatusChecks: &github.RequiredStatusChecks{ - Strict: false, - Contexts: []string{"foo"}, - }, - RequiredPullRequestReviews: &github.PullRequestReviewsEnforcement{ - DismissalRestrictions: &github.DismissalRestrictions{ - Users: nil, - Teams: nil, - }, - DismissStaleReviews: false, - RequireCodeOwnerReviews: false, - RequiredApprovingReviewCount: 0, - }, - EnforceAdmins: &github.AdminEnforcement{ - URL: nil, - Enabled: false, - }, - Restrictions: &github.BranchRestrictions{ + expected: scut.TestReturn{ + Errors: nil, + Score: 1, + NumberOfWarn: 7, + NumberOfInfo: 1, + NumberOfDebug: 0, + }, + protection: &github.Protection{ + RequiredStatusChecks: &github.RequiredStatusChecks{ + Strict: false, + Contexts: []string{"foo"}, + }, + RequiredPullRequestReviews: &github.PullRequestReviewsEnforcement{ + DismissalRestrictions: &github.DismissalRestrictions{ Users: nil, Teams: nil, - Apps: nil, - }, - RequireLinearHistory: &github.RequireLinearHistory{ - Enabled: false, - }, - AllowForcePushes: &github.AllowForcePushes{ - Enabled: true, - }, - AllowDeletions: &github.AllowDeletions{ - Enabled: false, }, + DismissStaleReviews: false, + RequireCodeOwnerReviews: false, + RequiredApprovingReviewCount: 0, + }, + EnforceAdmins: &github.AdminEnforcement{ + URL: nil, + Enabled: false, + }, + Restrictions: &github.BranchRestrictions{ + Users: nil, + Teams: nil, + Apps: nil, + }, + RequireLinearHistory: &github.RequireLinearHistory{ + Enabled: false, + }, + AllowForcePushes: &github.AllowForcePushes{ + Enabled: true, + }, + AllowDeletions: &github.AllowDeletions{ + Enabled: false, }, - }, - want: checker.CheckResult{ - Name: CheckBranchProtection, - Pass: false, - Details: nil, - Confidence: 9, - ShouldRetry: false, - Error: nil, }, }, { name: "Allow deletions enabled", - args: args{ - protection: &github.Protection{ - RequiredStatusChecks: &github.RequiredStatusChecks{ - Strict: false, - Contexts: []string{"foo"}, - }, - RequiredPullRequestReviews: &github.PullRequestReviewsEnforcement{ - DismissalRestrictions: &github.DismissalRestrictions{ - Users: nil, - Teams: nil, - }, - DismissStaleReviews: false, - RequireCodeOwnerReviews: false, - RequiredApprovingReviewCount: 0, - }, - EnforceAdmins: &github.AdminEnforcement{ - URL: nil, - Enabled: false, - }, - Restrictions: &github.BranchRestrictions{ + expected: scut.TestReturn{ + Errors: nil, + Score: 1, + NumberOfWarn: 7, + NumberOfInfo: 1, + NumberOfDebug: 0, + }, + protection: &github.Protection{ + RequiredStatusChecks: &github.RequiredStatusChecks{ + Strict: false, + Contexts: []string{"foo"}, + }, + RequiredPullRequestReviews: &github.PullRequestReviewsEnforcement{ + DismissalRestrictions: &github.DismissalRestrictions{ Users: nil, Teams: nil, - Apps: nil, - }, - RequireLinearHistory: &github.RequireLinearHistory{ - Enabled: false, - }, - AllowForcePushes: &github.AllowForcePushes{ - Enabled: false, - }, - AllowDeletions: &github.AllowDeletions{ - Enabled: true, }, + DismissStaleReviews: false, + RequireCodeOwnerReviews: false, + RequiredApprovingReviewCount: 0, + }, + EnforceAdmins: &github.AdminEnforcement{ + URL: nil, + Enabled: false, + }, + Restrictions: &github.BranchRestrictions{ + Users: nil, + Teams: nil, + Apps: nil, + }, + RequireLinearHistory: &github.RequireLinearHistory{ + Enabled: false, + }, + AllowForcePushes: &github.AllowForcePushes{ + Enabled: false, + }, + AllowDeletions: &github.AllowDeletions{ + Enabled: true, }, - }, - want: checker.CheckResult{ - Name: CheckBranchProtection, - Pass: false, - Details: nil, - Confidence: 9, - ShouldRetry: false, - Error: nil, }, }, { name: "Branches are protected", - args: args{ - protection: &github.Protection{ - RequiredStatusChecks: &github.RequiredStatusChecks{ - Strict: true, - Contexts: []string{"foo"}, - }, - RequiredPullRequestReviews: &github.PullRequestReviewsEnforcement{ - DismissalRestrictions: &github.DismissalRestrictions{ - Users: nil, - Teams: nil, - }, - DismissStaleReviews: true, - RequireCodeOwnerReviews: true, - RequiredApprovingReviewCount: 1, - }, - EnforceAdmins: &github.AdminEnforcement{ - URL: nil, - Enabled: true, - }, - Restrictions: &github.BranchRestrictions{ + expected: scut.TestReturn{ + Errors: nil, + Score: 1, + NumberOfWarn: 7, + NumberOfInfo: 1, + NumberOfDebug: 0, + }, + protection: &github.Protection{ + RequiredStatusChecks: &github.RequiredStatusChecks{ + Strict: true, + Contexts: []string{"foo"}, + }, + RequiredPullRequestReviews: &github.PullRequestReviewsEnforcement{ + DismissalRestrictions: &github.DismissalRestrictions{ Users: nil, Teams: nil, - Apps: nil, - }, - RequireLinearHistory: &github.RequireLinearHistory{ - Enabled: true, - }, - AllowForcePushes: &github.AllowForcePushes{ - Enabled: false, - }, - AllowDeletions: &github.AllowDeletions{ - Enabled: false, }, + DismissStaleReviews: true, + RequireCodeOwnerReviews: true, + RequiredApprovingReviewCount: 1, + }, + EnforceAdmins: &github.AdminEnforcement{ + URL: nil, + Enabled: true, + }, + Restrictions: &github.BranchRestrictions{ + Users: nil, + Teams: nil, + Apps: nil, + }, + RequireLinearHistory: &github.RequireLinearHistory{ + Enabled: true, + }, + AllowForcePushes: &github.AllowForcePushes{ + Enabled: false, + }, + AllowDeletions: &github.AllowDeletions{ + Enabled: false, }, - }, - want: checker.CheckResult{ - Name: CheckBranchProtection, - Pass: true, - Details: nil, - Confidence: 10, - ShouldRetry: false, - Error: nil, }, }, } + // for _, tt := range tests { + // tt := tt // Re-initializing variable so it is not changed while executing the closure below + // l.messages = []string{} + // t.Run(tt.name, func(t *testing.T) { + // t.Parallel() + // got := IsBranchProtected(tt.args.protection, "test", l.Logf) + // got.Details = l.messages + // if got.Confidence != tt.want.Confidence || got.Pass != tt.want.Pass { + // t.Errorf("IsBranchProtected() = %s, %v, want %v", tt.name, got, tt.want) + // } + // }) + // } + for _, tt := range tests { tt := tt // Re-initializing variable so it is not changed while executing the closure below - l.messages = []string{} t.Run(tt.name, func(t *testing.T) { t.Parallel() - got := IsBranchProtected(tt.args.protection, "test", l.Logf) - got.Details = l.messages - if got.Confidence != tt.want.Confidence || got.Pass != tt.want.Pass { - t.Errorf("IsBranchProtected() = %s, %v, want %v", tt.name, got, tt.want) - } + dl := scut.TestDetailLogger{} + r := IsBranchProtected(tt.protection, "test", &dl) + scut.ValidateTestReturn2(t, tt.name, &tt.expected, &r, &dl) }) } } diff --git a/checks/checkforcontent.go b/checks/checkforcontent.go index 66d7242d334..3d0c219a9cb 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.ErrRunFailure, fmt.Sprintf("%v: %v", sce.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.ErrRunFailure, fmt.Sprintf("%v: %v", sce.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.ErrRunFailure, 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 7de6812e434..8c9d96851f0 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/checks2.yaml b/checks/checks2.yaml new file mode 100644 index 00000000000..87080a4c148 --- /dev/null +++ b/checks/checks2.yaml @@ -0,0 +1,145 @@ +# 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. + +# This is the source of truth for all check descriptions and remediation steps. +# Run `cd checks/main && go run /main` to generate `checks.json` and `checks.md`. +checks: + Active: + risk: High + description: >- + This check tries to determine if the project is "actively maintained". + + A project which is not active may not be patched, may not have its + dependencies patched, or may not be actively tested and used. + A low score is therefore considered `High` risk. + + The check currently works by looking for commits within the last 90 days, and + outputs the highest score if there are at least 1 commit/week during this period. + remediation: + - >- + There is *NO* remediation work needed here. This is just to indicate + your project activity and maintenance commitment. + Binary-Artifacts: + risk: High + description: >- + This check tries to determine if a project has binary artifacts in the source repository. + + Binaries are a threat to auditability and vulnerability management. + In addition, a binary could be compromised or malicious. + A low score is therefore considered `High` risk. + remediation: + - >- + Remove the binary artifacts from the repository. + - >- + Build from source. + Automatic-Dependency-Update: + risk: High + description: >- + This check tries to determine if a project has dependencies automatically updated. + + Not updating dependencies makes a project vulnerable to known flaws and prone to attacks. + A low score is therefore considered `High` risk. + + The checks looks for [dependabot](https://dependabot.com/docs/config-file/) or + [renovatebot](https://docs.renovatebot.com/configuration-options/). This check only looks if + it is enabled and does not ensure that it is run and pull requests are merged. + + remediation: + - >- + Signup for automatic dependency updates with dependabot or renovatebot and place the config + file in the locations that are recommended by these tools. + Code-Review: + risk: High + description: >- + This check tries to determine if a project requires code review before + pull requests are merged. + + Reviewing code improves quality of code in general. In addition, it ensures + compromised contributors cannot intentionally inject malicious code. A low + score is therefore considered `High` risk. + + The check first tries to detect if branch-Protection is enabled + on the default branch and the number of reviewers is at least 1. If this + fails, it checks if the recent (~30) commits have a Github-approved + review or if the merger is different from the committer (implicit review). + It also performs similar check for reviews using + [Prow](https://github.com/kubernetes/test-infra/tree/master/prow#readme) + (labels "lgtm" or "approved") and Gerrit ("Reviewed-on" and "Reviewed-by"). + remediation: + - >- + Follow security best practices by performing strict code reviews for + every new pull request. + - >- + Make "code reviews" mandatory in your repository configuration. E.g. + [GitHub](https://docs.github.com/en/github/administering-a-repository/about-protected-branches#require-pull-request-reviews-before-merging). + - >- + Enforce the rule for administrators / code owners as well. E.g. + [GitHub](https://docs.github.com/en/github/administering-a-repository/about-protected-branches#include-administrators) + Branch-Protection: + risk: High + description: >- + Branch protection allows defining rules to enforce certain workflows for + branches, such as requiring a review or passing certain status checks. + + Branch protection ensures compromised contributors cannot + intentionally inject malicious code. A low score is therefore considered `High` risk. + + This check determines if the default and release branches are + protected with GitHub's branch protection settings. + The check only works when the token has [Admin + access](https://github.community/t/enable-branch-protection-get-api-without-admin/14197) + to the repository. This check determines if the default and release branches are + protected. + remediation: + - >- + Enable branch protection settings in your source hosting provider to + avoid force pushes or deletion of your important branches. + - >- + For GitHub, check out the steps + [here](https://docs.github.com/en/github/administering-a-repository/managing-a-branch-protection-rule). + Frozen-Deps: + risk: Medium + description: >- + This check tries to determine if a project has declared and pinned its + dependencies. + + Pinning dependencies is important to mitigate compromised dependencies + from undermining the security of the project. Low score is therefore considered + `Medium` risk. + + The checks works by (1) looking for the following files in the root + directory: go.mod, go.sum (Golang), package-lock.json, npm-shrinkwrap.json + (Javascript), requirements.txt, pipfile.lock (Python), gemfile.lock + (Ruby), cargo.lock (Rust), yarn.lock (package manager), composer.lock + (PHP), vendor/, third_party/, third-party/; (2) looks for + unpinned dependencies in Dockerfiles, shell scripts and GitHub workflows. + remediation: + - >- + Declare all your dependencies with specific versions in your package + format file (e.g. `package.json` for npm, `requirements.txt` for + python). For C/C++, check in the code from a trusted source and add a + `README` on the specific version used (and the archive SHA hashes). + - >- + If the package manager supports lock files (e.g. `package-lock.json` for + npm), make sure to check these in the source code as well. These files + maintain signatures for the entire dependency tree and saves from future + exploitation in case the package is compromised. + - >- + For Dockerfiles and github workflows, pin dependencies by hash. See example + [gitcache-docker.yaml](https://github.com/ossf/scorecard/blob/main/.github/workflows/gitcache-docker.yaml#L36) + and [Dockerfile](https://github.com/ossf/scorecard/blob/main/cron/worker/Dockerfile) examples. + - >- + To help update your dependencies after pinning them, use tools such as + Github's [dependabot](https://github.blog/2020-06-01-keep-all-your-packages-up-to-date-with-dependabot/) + or [renovate bot](https://github.com/renovatebot/renovate). \ No newline at end of file diff --git a/checks/code_review.go b/checks/code_review.go index ff5b25d677f..14b714454f4 100644 --- a/checks/code_review.go +++ b/checks/code_review.go @@ -16,6 +16,7 @@ package checks import ( "errors" + "fmt" "strings" "github.com/google/go-github/v32/github" @@ -27,7 +28,6 @@ import ( const ( // CheckCodeReview is the registered name for DoesCodeReview. CheckCodeReview = "Code-Review" - crPassThreshold = .75 pullRequestsToAnalyze = 30 reviewsToAnalyze = 30 labelsToAnalyze = 30 @@ -88,17 +88,17 @@ func DoesCodeReview(c *checker.CheckRequest) checker.CheckResult { "labelsToAnalyze": githubv4.Int(labelsToAnalyze), } if err := c.GraphClient.Query(c.Ctx, &prHistory, vars); err != nil { - return checker.MakeInconclusiveResult(CheckCodeReview, err) + return checker.CreateRuntimeErrorResult(CheckCodeReview, err) } - return checker.MultiCheckOr( - IsPrReviewRequired, - GithubCodeReview, - ProwCodeReview, - CommitMessageHints, + return checker.MultiCheckOr2( + isPrReviewRequired, + githubCodeReview, + prowCodeReview, + commitMessageHints, )(c) } -func GithubCodeReview(c *checker.CheckRequest) checker.CheckResult { +func githubCodeReview(c *checker.CheckRequest) checker.CheckResult { // Look at some merged PRs to see if they were reviewed. totalMerged := 0 totalReviewed := 0 @@ -112,7 +112,7 @@ func GithubCodeReview(c *checker.CheckRequest) checker.CheckResult { foundApprovedReview := false for _, r := range pr.LatestReviews.Nodes { if r.State == "APPROVED" { - c.Logf("found review approved pr: %d", pr.Number) + c.Dlogger.Debug("found review approved pr: %d", pr.Number) totalReviewed++ foundApprovedReview = true break @@ -124,34 +124,27 @@ func GithubCodeReview(c *checker.CheckRequest) checker.CheckResult { // time on clicking the approve button. if !foundApprovedReview { if !pr.MergeCommit.AuthoredByCommitter { - c.Logf("found pr with committer different than author: %d", pr.Number) + c.Dlogger.Debug("found pr with committer different than author: %d", pr.Number) totalReviewed++ } } } - if totalReviewed > 0 { - c.Logf("github code reviews found") - } - return checker.MakeProportionalResult(CheckCodeReview, totalReviewed, totalMerged, crPassThreshold) + return createResult("GitHub", totalReviewed, totalMerged) } -func IsPrReviewRequired(c *checker.CheckRequest) checker.CheckResult { +func isPrReviewRequired(c *checker.CheckRequest) checker.CheckResult { // Look to see if review is enforced. // Check the branch protection rules, we may not be able to get these though. if prHistory.Repository.DefaultBranchRef.BranchProtectionRule.RequiredApprovingReviewCount >= 1 { - c.Logf("pr review policy enforced") - const confidence = 5 - return checker.CheckResult{ - Name: CheckCodeReview, - Pass: true, - Confidence: confidence, - } + // If the default value is 0 when we cannot retrieve the value, + // a non-zero value means we're confident it's enabled. + return checker.CreateMaxScoreResult(CheckCodeReview, "branch protection for default branch is enabled") } - return checker.MakeInconclusiveResult(CheckCodeReview, nil) + return checker.CreateInconclusiveResult(CheckCodeReview, "cannot determine if branch protection is enabled") } -func ProwCodeReview(c *checker.CheckRequest) checker.CheckResult { +func prowCodeReview(c *checker.CheckRequest) checker.CheckResult { // Look at some merged PRs to see if they were reviewed totalMerged := 0 totalReviewed := 0 @@ -168,14 +161,10 @@ func ProwCodeReview(c *checker.CheckRequest) checker.CheckResult { } } - if totalReviewed == 0 { - return checker.MakeInconclusiveResult(CheckCodeReview, ErrorNoReviews) - } - c.Logf("prow code reviews found") - return checker.MakeProportionalResult(CheckCodeReview, totalReviewed, totalMerged, crPassThreshold) + return createResult("Prow", totalReviewed, totalMerged) } -func CommitMessageHints(c *checker.CheckRequest) checker.CheckResult { +func commitMessageHints(c *checker.CheckRequest) checker.CheckResult { commits, _, err := c.Client.Repositories.ListCommits(c.Ctx, c.Owner, c.Repo, &github.CommitsListOptions{}) if err != nil { return checker.MakeRetryResult(CheckCodeReview, err) @@ -193,7 +182,7 @@ func CommitMessageHints(c *checker.CheckRequest) checker.CheckResult { } } if isBot { - c.Logf("skip commit from bot account: %s", committer) + c.Dlogger.Debug("skip commit from bot account: %s", committer) continue } @@ -208,9 +197,13 @@ func CommitMessageHints(c *checker.CheckRequest) checker.CheckResult { } } - if totalReviewed == 0 { - return checker.MakeInconclusiveResult(CheckCodeReview, ErrorNoReviews) + return createResult("Gerrit", totalReviewed, total) +} + +func createResult(reviewName string, reviewed, total int) checker.CheckResult { + if total > 0 { + reason := fmt.Sprintf("%s code reviews found for %v commits out of the last %v", reviewName, reviewed, total) + return checker.CreateProportionalScoreResult(CheckCodeReview, reason, reviewed, total) } - c.Logf("code reviews found") - return checker.MakeProportionalResult(CheckCodeReview, totalReviewed, total, crPassThreshold) + return checker.CreateInconclusiveResult(CheckCodeReview, fmt.Sprintf("no %s commits found", reviewName)) } diff --git a/checks/frozen_deps.go b/checks/frozen_deps.go index 30c2ecfc6cd..796824e51d4 100644 --- a/checks/frozen_deps.go +++ b/checks/frozen_deps.go @@ -15,7 +15,6 @@ package checks import ( - "errors" "fmt" "regexp" "strings" @@ -24,17 +23,12 @@ import ( "gopkg.in/yaml.v2" "github.com/ossf/scorecard/checker" + sce "github.com/ossf/scorecard/errors" ) // CheckFrozenDeps is the registered name for FrozenDeps. const CheckFrozenDeps = "Frozen-Deps" -// ErrInvalidDockerfile : Invalid docker file. -var ErrInvalidDockerfile = errors.New("invalid docker file") - -// ErrEmptyFile : Invalid docker file. -var ErrEmptyFile = errors.New("file has no content") - // Structure for workflow config. // We only declare the fields we need. // Github workflows format: https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions @@ -65,7 +59,7 @@ func init() { // FrozenDeps will check the repository if it contains frozen dependecies. func FrozenDeps(c *checker.CheckRequest) checker.CheckResult { - return checker.MultiCheckAnd( + return checker.MultiCheckAnd2( isPackageManagerLockFilePresent, isGitHubActionsWorkflowPinned, isDockerfilePinned, @@ -78,28 +72,70 @@ func FrozenDeps(c *checker.CheckRequest) checker.CheckResult { // TODO(laurent): need to support GCB pinning. func isShellScriptFreeOfInsecureDownloads(c *checker.CheckRequest) checker.CheckResult { - return CheckFilesContent(CheckFrozenDeps, "*", false, c, validateShellScriptDownloads) + r, err := CheckFilesContent2("*", false, c, validateShellScriptIsFreeOfInsecureDownloads) + return createResultForIsShellScriptFreeOfInsecureDownloads(r, err) +} + +func createResultForIsShellScriptFreeOfInsecureDownloads(r bool, err error) checker.CheckResult { + if err != nil { + return checker.CreateRuntimeErrorResult(CheckFrozenDeps, err) + } + if !r { + return checker.CreateMinScoreResult(CheckFrozenDeps, + "insecure (unpinned) dependency downloads found in shell scripts") + } + + return checker.CreateMaxScoreResult(CheckFrozenDeps, + "no insecure (unpinned) dependency downloads found in shell scripts") +} + +func TestValidateShellScriptIsFreeOfInsecureDownloads(pathfn string, + content []byte, dl checker.DetailLogger) checker.CheckResult { + r, err := validateShellScriptIsFreeOfInsecureDownloads(pathfn, content, dl) + return createResultForIsShellScriptFreeOfInsecureDownloads(r, err) } -func validateShellScriptDownloads(pathfn string, content []byte, - logf func(s string, f ...interface{})) (bool, error) { +func validateShellScriptIsFreeOfInsecureDownloads(pathfn string, content []byte, + dl checker.DetailLogger) (bool, error) { // Validate the file type. if !isShellScriptFile(pathfn, content) { return true, nil } - return validateShellFile(pathfn, content, logf) + return validateShellFile(pathfn, content, dl) } func isDockerfileFreeOfInsecureDownloads(c *checker.CheckRequest) checker.CheckResult { - return CheckFilesContent(CheckFrozenDeps, "*Dockerfile*", false, c, validateDockerfileDownloads) + r, err := CheckFilesContent2("*Dockerfile*", false, c, validateDockerfileIsFreeOfInsecureDownloads) + return createResultForIsDockerfileFreeOfInsecureDownloads(r, err) } -func validateDockerfileDownloads(pathfn string, content []byte, - logf func(s string, f ...interface{})) (bool, error) { +// Create the result. +func createResultForIsDockerfileFreeOfInsecureDownloads(r bool, err error) checker.CheckResult { + if err != nil { + return checker.CreateRuntimeErrorResult(CheckFrozenDeps, err) + } + if !r { + return checker.CreateMinScoreResult(CheckFrozenDeps, + "insecure (unpinned) dependency downloads found in Dockerfiles") + } + + return checker.CreateMaxScoreResult(CheckFrozenDeps, + "no insecure (unpinned) dependency downloads found in Dockerfiles") +} + +func TestValidateDockerfileIsFreeOfInsecureDownloads(pathfn string, + content []byte, dl checker.DetailLogger) checker.CheckResult { + r, err := validateDockerfileIsFreeOfInsecureDownloads(pathfn, content, dl) + return createResultForIsDockerfileFreeOfInsecureDownloads(r, err) +} + +func validateDockerfileIsFreeOfInsecureDownloads(pathfn string, content []byte, + dl checker.DetailLogger) (bool, error) { contentReader := strings.NewReader(string(content)) res, err := parser.Parse(contentReader) if err != nil { - return false, fmt.Errorf("cannot read dockerfile content: %w", err) + //nolint + return false, sce.Create(sce.ErrRunFailure, fmt.Sprintf("%v: %v", sce.ErrInternalInvalidDockerFile, err)) } // nolint: prealloc @@ -119,7 +155,8 @@ func validateDockerfileDownloads(pathfn string, content []byte, } if len(valueList) == 0 { - return false, ErrParsingDockerfile + //nolint + return false, sce.Create(sce.ErrRunFailure, sce.ErrInternalInvalidDockerFile.Error()) } // Build a file content. @@ -127,15 +164,33 @@ func validateDockerfileDownloads(pathfn string, content []byte, bytes = append(bytes, cmd...) bytes = append(bytes, '\n') } - return validateShellFile(pathfn, bytes, logf) + return validateShellFile(pathfn, bytes, dl) } func isDockerfilePinned(c *checker.CheckRequest) checker.CheckResult { - return CheckFilesContent(CheckFrozenDeps, "*Dockerfile*", false, c, validateDockerfile) + r, err := CheckFilesContent2("*Dockerfile*", false, c, validateDockerfileIsPinned) + return createResultForIsDockerfilePinned(r, err) +} + +// Create the result. +func createResultForIsDockerfilePinned(r bool, err error) checker.CheckResult { + if err != nil { + return checker.CreateRuntimeErrorResult(CheckFrozenDeps, err) + } + if r { + return checker.CreateMaxScoreResult(CheckFrozenDeps, "Dockerfile dependencies are pinned") + } + + return checker.CreateMinScoreResult(CheckFrozenDeps, "unpinned dependencies found Dockerfiles") +} + +func TestValidateDockerfileIsPinned(pathfn string, content []byte, dl checker.DetailLogger) checker.CheckResult { + r, err := validateDockerfileIsPinned(pathfn, content, dl) + return createResultForIsDockerfilePinned(r, err) } -func validateDockerfile(pathfn string, content []byte, - logf func(s string, f ...interface{})) (bool, error) { +func validateDockerfileIsPinned(pathfn string, content []byte, + dl checker.DetailLogger) (bool, error) { // Users may use various names, e.g., // Dockerfile.aarch64, Dockerfile.template, Dockerfile_template, dockerfile, Dockerfile-name.template // Templates may trigger false positives, e.g. FROM { NAME }. @@ -150,7 +205,8 @@ func validateDockerfile(pathfn string, content []byte, pinnedAsNames := make(map[string]bool) res, err := parser.Parse(contentReader) if err != nil { - return false, fmt.Errorf("cannot read dockerfile content: %w", err) + //nolint + return false, sce.Create(sce.ErrRunFailure, fmt.Sprintf("%v: %v", sce.ErrInternalInvalidDockerFile, err)) } for _, child := range res.AST.Children { @@ -188,44 +244,70 @@ func validateDockerfile(pathfn string, content []byte, // Not pinned. ret = false - logf("!! frozen-deps/docker - %v has non-pinned dependency '%v'", pathfn, name) + dl.Warn("unpinned dependency detected in %v: '%v'", pathfn, name) // FROM name. case len(valueList) == 1: name := valueList[0] if !regex.Match([]byte(name)) { ret = false - logf("!! frozen-deps/docker - %v has non-pinned dependency '%v'", pathfn, name) + dl.Warn("unpinned dependency detected in %v: '%v'", pathfn, name) } default: // That should not happen. - return false, ErrInvalidDockerfile + //nolint + return false, sce.Create(sce.ErrRunFailure, sce.ErrInternalInvalidDockerFile.Error()) } } // The file should have at least one FROM statement. if !fromFound { - return false, ErrInvalidDockerfile + //nolint + return false, sce.Create(sce.ErrRunFailure, sce.ErrInternalInvalidDockerFile.Error()) } return ret, nil } func isGitHubWorkflowScriptFreeOfInsecureDownloads(c *checker.CheckRequest) checker.CheckResult { - return CheckFilesContent(CheckFrozenDeps, ".github/workflows/*", false, c, validateGitHubWorkflowShellScriptDownloads) + r, err := CheckFilesContent2(".github/workflows/*", false, c, validateGitHubWorkflowIsFreeOfInsecureDownloads) + return createResultForIsGitHubWorkflowScriptFreeOfInsecureDownloads(r, err) +} + +// Create the result. +func createResultForIsGitHubWorkflowScriptFreeOfInsecureDownloads(r bool, err error) checker.CheckResult { + if err != nil { + return checker.CreateRuntimeErrorResult(CheckFrozenDeps, err) + } + if !r { + return checker.CreateMinScoreResult(CheckFrozenDeps, + "insecure (unpinned) dependency downloads found in GitHub workflows") + } + + return checker.CreateMaxScoreResult(CheckFrozenDeps, + "no insecure (unpinned) dependency downloads found in GitHub workflows") } -func validateGitHubWorkflowShellScriptDownloads(pathfn string, content []byte, - logf func(s string, f ...interface{})) (bool, error) { +func TestValidateGitHubWorkflowScriptFreeOfInsecureDownloads(pathfn string, + content []byte, dl checker.DetailLogger) checker.CheckResult { + r, err := validateGitHubWorkflowIsFreeOfInsecureDownloads(pathfn, content, dl) + return createResultForIsGitHubWorkflowScriptFreeOfInsecureDownloads(r, err) +} + +func validateGitHubWorkflowIsFreeOfInsecureDownloads(pathfn string, content []byte, + dl checker.DetailLogger) (bool, error) { if len(content) == 0 { - return false, ErrEmptyFile + //nolint + return false, sce.Create(sce.ErrRunFailure, sce.ErrInternalEmptyFile.Error()) } var workflow gitHubActionWorkflowConfig err := yaml.Unmarshal(content, &workflow) if err != nil { - return false, fmt.Errorf("!! frozen-deps - cannot unmarshal file %v\n%v: %w", pathfn, string(content), err) + //nolint + return false, sce.Create(sce.ErrRunFailure, + fmt.Sprintf("%v:%s:%s:%v", sce.ErrInternalInvalidYamlFile, pathfn, string(content), err)) } githubVarRegex := regexp.MustCompile(`{{[^{}]*}}`) @@ -262,7 +344,7 @@ func validateGitHubWorkflowShellScriptDownloads(pathfn string, content []byte, } if scriptContent != "" { - validated, err = validateShellFile(pathfn, []byte(scriptContent), logf) + validated, err = validateShellFile(pathfn, []byte(scriptContent), dl) if err != nil { return false, err } @@ -273,19 +355,40 @@ func validateGitHubWorkflowShellScriptDownloads(pathfn string, content []byte, // Check pinning of github actions in workflows. func isGitHubActionsWorkflowPinned(c *checker.CheckRequest) checker.CheckResult { - return CheckFilesContent(CheckFrozenDeps, ".github/workflows/*", true, c, validateGitHubActionWorkflow) + r, err := CheckFilesContent2(".github/workflows/*", true, c, validateGitHubActionWorkflow) + return createResultForIsGitHubActionsWorkflowPinned(r, err) +} + +// Create the result. +func createResultForIsGitHubActionsWorkflowPinned(r bool, err error) checker.CheckResult { + if err != nil { + return checker.CreateRuntimeErrorResult(CheckFrozenDeps, err) + } + if r { + return checker.CreateMaxScoreResult(CheckFrozenDeps, "GitHub actions are pinned") + } + + return checker.CreateMinScoreResult(CheckFrozenDeps, "GitHub actions are not pinned") +} + +func TestIsGitHubActionsWorkflowPinned(pathfn string, content []byte, dl checker.DetailLogger) checker.CheckResult { + r, err := validateGitHubActionWorkflow(pathfn, content, dl) + return createResultForIsGitHubActionsWorkflowPinned(r, err) } // Check file content. -func validateGitHubActionWorkflow(pathfn string, content []byte, logf func(s string, f ...interface{})) (bool, error) { +func validateGitHubActionWorkflow(pathfn string, content []byte, dl checker.DetailLogger) (bool, error) { if len(content) == 0 { - return false, ErrEmptyFile + //nolint + return false, sce.Create(sce.ErrRunFailure, sce.ErrInternalEmptyFile.Error()) } var workflow gitHubActionWorkflowConfig err := yaml.Unmarshal(content, &workflow) if err != nil { - return false, fmt.Errorf("!! frozen-deps - cannot unmarshal file %v\n%v: %w", pathfn, string(content), err) + //nolint + return false, sce.Create(sce.ErrRunFailure, + fmt.Sprintf("%v:%s:%s:%v", sce.ErrInternalInvalidYamlFile, pathfn, string(content), err)) } hashRegex := regexp.MustCompile(`^.*@[a-f\d]{40,}`) @@ -301,7 +404,7 @@ func validateGitHubActionWorkflow(pathfn string, content []byte, logf func(s str match := hashRegex.Match([]byte(step.Uses)) if !match { ret = false - logf("!! frozen-deps/github-actions - %v has non-pinned dependency '%v' (job '%v')", pathfn, step.Uses, jobName) + dl.Warn("unpinned dependency detected in %v: '%v' (job '%v')", pathfn, step.Uses, jobName) } } } @@ -312,38 +415,49 @@ func validateGitHubActionWorkflow(pathfn string, content []byte, logf func(s str // Check presence of lock files thru validatePackageManagerFile(). func isPackageManagerLockFilePresent(c *checker.CheckRequest) checker.CheckResult { - return CheckIfFileExists(CheckFrozenDeps, c, validatePackageManagerFile) + r, err := CheckIfFileExists2(CheckFrozenDeps, c, validatePackageManagerFile) + if err != nil { + return checker.CreateRuntimeErrorResult(CheckFrozenDeps, err) + } + if !r { + return checker.CreateInconclusiveResult(CheckFrozenDeps, "no lock files detected for a package manager") + } + + return checker.CreateMaxScoreResult(CheckFrozenDeps, "lock file detected for a package manager") } // validatePackageManagerFile will validate the if frozen dependecies file name exists. // TODO(laurent): need to differentiate between libraries and programs. // TODO(laurent): handle multi-language repos. -func validatePackageManagerFile(name string, logf func(s string, f ...interface{})) (bool, error) { +func validatePackageManagerFile(name string, dl checker.DetailLogger) (bool, error) { switch strings.ToLower(name) { - case "go.mod", "go.sum": - logf("go modules found: %s", name) + // TODO(laurent): "go.mod" is for libraries + case "go.sum": + dl.Info("go lock file detected: %s", name) return true, nil case "vendor/", "third_party/", "third-party/": - logf("vendor dir found: %s", name) + dl.Info("vendoring detected in: %s", name) return true, nil case "package-lock.json", "npm-shrinkwrap.json": - logf("nodejs packages found: %s", name) + dl.Info("javascript lock file detected: %s", name) return true, nil // TODO(laurent): add check for hashbased pinning in requirements.txt - https://davidwalsh.name/hashin - case "requirements.txt", "pipfile.lock": - logf("python requirements found: %s", name) + // Note: because requirements.txt does not handle transitive dependencies, we consider it + // not a lock file, until we have remediation steps for pip-build. + case "pipfile.lock": + dl.Info("python lock file detected: %s", name) return true, nil case "gemfile.lock": - logf("ruby gems found: %s", name) + dl.Info("ruby lock file detected: %s", name) return true, nil case "cargo.lock": - logf("rust crates found: %s", name) + dl.Info("rust lock file detected: %s", name) return true, nil case "yarn.lock": - logf("yarn packages found: %s", name) + dl.Info("yarn lock file detected: %s", name) return true, nil case "composer.lock": - logf("composer packages found: %s", name) + dl.Info("composer lock file detected: %s", name) return true, nil default: return false, nil diff --git a/checks/frozen_deps_test.go b/checks/frozen_deps_test.go index b4c221148f4..aa7ea549a3f 100644 --- a/checks/frozen_deps_test.go +++ b/checks/frozen_deps_test.go @@ -15,525 +15,421 @@ package checks import ( - "errors" "fmt" "io/ioutil" - "strings" "testing" + + "github.com/ossf/scorecard/checker" + sce "github.com/ossf/scorecard/errors" + scut "github.com/ossf/scorecard/utests" ) func TestGithubWorkflowPinning(t *testing.T) { t.Parallel() - //nolint - type args struct { - Log log - Filename string - } - type returnValue struct { - Error error - Result bool - NumberOfErrors int - } - - //nolint tests := []struct { - args args - want returnValue - name string + name string + filename string + expected scut.TestReturn }{ { - name: "Zero size content", - args: args{ - Filename: "", - Log: log{}, - }, - want: returnValue{ - Error: ErrEmptyFile, - Result: false, - NumberOfErrors: 0, + name: "Zero size content", + filename: "", + expected: scut.TestReturn{ + Errors: []error{sce.ErrRunFailure}, + Score: checker.InconclusiveResultScore, + NumberOfWarn: 0, + NumberOfInfo: 0, + NumberOfDebug: 0, }, }, { - name: "Pinned workflow", - args: args{ - Filename: "./testdata/workflow-pinned.yaml", - Log: log{}, - }, - want: returnValue{ - Error: nil, - Result: true, - NumberOfErrors: 0, + name: "Pinned workflow", + filename: "./testdata/workflow-pinned.yaml", + expected: scut.TestReturn{ + Errors: nil, + Score: checker.MaxResultScore, + NumberOfWarn: 0, + NumberOfInfo: 0, + NumberOfDebug: 0, }, }, { - name: "Non-pinned workflow", - args: args{ - Filename: "./testdata/workflow-not-pinned.yaml", - Log: log{}, - }, - want: returnValue{ - Error: nil, - Result: false, - NumberOfErrors: 1, + name: "Non-pinned workflow", + filename: "./testdata/workflow-not-pinned.yaml", + expected: scut.TestReturn{ + Errors: nil, + Score: checker.MinResultScore, + NumberOfWarn: 1, + NumberOfInfo: 0, + NumberOfDebug: 0, }, }, } - //nolint for _, tt := range tests { tt := tt // Re-initializing variable so it is not changed while executing the closure below t.Run(tt.name, func(t *testing.T) { t.Parallel() var content []byte var err error - if tt.args.Filename == "" { + if tt.filename == "" { content = make([]byte, 0) } else { - content, err = ioutil.ReadFile(tt.args.Filename) + content, err = ioutil.ReadFile(tt.filename) if err != nil { panic(fmt.Errorf("cannot read file: %w", err)) } } - r, err := validateGitHubActionWorkflow(tt.args.Filename, content, tt.args.Log.Logf) - - if !errors.Is(err, tt.want.Error) || - r != tt.want.Result || - len(tt.args.Log.messages) != tt.want.NumberOfErrors { - t.Errorf("TestDockerfileScriptDownload:\"%v\": %v (%v,%v,%v) want (%v, %v, %v)\n%v", - tt.name, tt.args.Filename, r, err, len(tt.args.Log.messages), tt.want.Result, tt.want.Error, tt.want.NumberOfErrors, - strings.Join(tt.args.Log.messages, "\n")) - } + dl := scut.TestDetailLogger{} + r := TestIsGitHubActionsWorkflowPinned(tt.filename, content, &dl) + scut.ValidateTestReturn2(t, tt.name, &tt.expected, &r, &dl) }) } } func TestDockerfilePinning(t *testing.T) { t.Parallel() - type args struct { - Logf func(s string, f ...interface{}) - Filename string - } - - type returnValue struct { - Error error - Result bool - } - - l := log{} tests := []struct { - args args - want returnValue - name string + name string + filename string + expected scut.TestReturn }{ { - name: "Invalid dockerfile", - args: args{ - Filename: "./testdata/Dockerfile-invalid", - Logf: l.Logf, - }, - want: returnValue{ - Error: ErrInvalidDockerfile, - Result: false, + name: "Invalid dockerfile", + filename: "./testdata/Dockerfile-invalid", + expected: scut.TestReturn{ + Errors: []error{sce.ErrRunFailure}, + Score: checker.InconclusiveResultScore, + NumberOfWarn: 0, + NumberOfInfo: 0, + NumberOfDebug: 0, }, }, { - name: "Pinned dockerfile", - args: args{ - Filename: "./testdata/Dockerfile-pinned", - Logf: l.Logf, - }, - want: returnValue{ - Error: nil, - Result: true, + name: "Pinned dockerfile", + filename: "./testdata/Dockerfile-pinned", + expected: scut.TestReturn{ + Errors: nil, + Score: checker.MaxResultScore, + NumberOfWarn: 0, + NumberOfInfo: 0, + NumberOfDebug: 0, }, }, { - name: "Pinned dockerfile as", - args: args{ - Filename: "./testdata/Dockerfile-pinned-as", - Logf: l.Logf, - }, - want: returnValue{ - Error: nil, - Result: true, + name: "Pinned dockerfile as", + filename: "./testdata/Dockerfile-pinned-as", + expected: scut.TestReturn{ + Errors: nil, + Score: checker.MaxResultScore, + NumberOfWarn: 0, + NumberOfInfo: 0, + NumberOfDebug: 0, }, }, { - name: "Non-pinned dockerfile as", - args: args{ - Filename: "./testdata/Dockerfile-not-pinned-as", - Logf: l.Logf, - }, - want: returnValue{ - Error: nil, - Result: false, + name: "Non-pinned dockerfile as", + filename: "./testdata/Dockerfile-not-pinned-as", + expected: scut.TestReturn{ + Errors: nil, + Score: checker.MinResultScore, + NumberOfWarn: 3, // TODO:fix should be 2 + NumberOfInfo: 0, + NumberOfDebug: 0, }, }, { - name: "Non-pinned dockerfile", - args: args{ - Filename: "./testdata/Dockerfile-not-pinned", - Logf: l.Logf, - }, - want: returnValue{ - Error: nil, - Result: false, + name: "Non-pinned dockerfile", + filename: "./testdata/Dockerfile-not-pinned", + expected: scut.TestReturn{ + Errors: nil, + Score: checker.MinResultScore, + NumberOfWarn: 1, + NumberOfInfo: 0, + NumberOfDebug: 0, }, }, } for _, tt := range tests { tt := tt // Re-initializing variable so it is not changed while executing the closure below - l.messages = []string{} t.Run(tt.name, func(t *testing.T) { t.Parallel() var content []byte var err error - - content, err = ioutil.ReadFile(tt.args.Filename) - if err != nil { - panic(fmt.Errorf("cannot read file: %w", err)) - } - - r, err := validateDockerfile(tt.args.Filename, content, tt.args.Logf) - - if !errors.Is(err, tt.want.Error) || - r != tt.want.Result { - t.Errorf("TestGithubWorkflowPinning:\"%v\": %v (%v,%v) want (%v, %v)", - tt.name, tt.args.Filename, r, err, tt.want.Result, tt.want.Error) + if tt.filename == "" { + content = make([]byte, 0) + } else { + content, err = ioutil.ReadFile(tt.filename) + if err != nil { + panic(fmt.Errorf("cannot read file: %w", err)) + } } + dl := scut.TestDetailLogger{} + r := TestValidateDockerfileIsPinned(tt.filename, content, &dl) + scut.ValidateTestReturn2(t, tt.name, &tt.expected, &r, &dl) }) } } func TestDockerfileScriptDownload(t *testing.T) { t.Parallel() - //nolint - type args struct { - // Note: this seems to be defined in e2e/e2e_suite_test.go - Log log - Filename string - } - - type returnValue struct { - Error error - Result bool - NumberOfErrors int - } - - //nolint tests := []struct { - args args - want returnValue - name string + name string + filename string + expected scut.TestReturn }{ { - name: "curl | sh", - args: args{ - Filename: "testdata/Dockerfile-curl-sh", - Log: log{}, - }, - want: returnValue{ - Error: nil, - Result: false, - NumberOfErrors: 4, + name: "curl | sh", + filename: "testdata/Dockerfile-curl-sh", + expected: scut.TestReturn{ + Errors: nil, + Score: checker.MinResultScore, + NumberOfWarn: 4, + NumberOfInfo: 0, + NumberOfDebug: 0, }, }, { - name: "wget | /bin/sh", - args: args{ - Filename: "testdata/Dockerfile-wget-bin-sh", - Log: log{}, - }, - want: returnValue{ - Error: nil, - Result: false, - NumberOfErrors: 3, + name: "wget | /bin/sh", + filename: "testdata/Dockerfile-wget-bin-sh", + expected: scut.TestReturn{ + Errors: nil, + Score: checker.MinResultScore, + NumberOfWarn: 3, + NumberOfInfo: 0, + NumberOfDebug: 0, }, }, { - name: "wget no exec", - args: args{ - Filename: "testdata/Dockerfile-script-ok", - Log: log{}, - }, - want: returnValue{ - Error: nil, - Result: true, - NumberOfErrors: 0, + name: "wget no exec", + filename: "testdata/Dockerfile-script-ok", + expected: scut.TestReturn{ + Errors: nil, + Score: checker.MaxResultScore, + NumberOfWarn: 0, + NumberOfInfo: 0, + NumberOfDebug: 0, }, }, { - name: "curl file sh", - args: args{ - Filename: "testdata/Dockerfile-curl-file-sh", - Log: log{}, - }, - want: returnValue{ - Error: nil, - Result: false, - NumberOfErrors: 12, + name: "curl file sh", + filename: "testdata/Dockerfile-curl-file-sh", + expected: scut.TestReturn{ + Errors: nil, + Score: checker.MinResultScore, + NumberOfWarn: 12, + NumberOfInfo: 0, + NumberOfDebug: 0, }, }, { - name: "proc substitution", - args: args{ - Filename: "testdata/Dockerfile-proc-subs", - Log: log{}, - }, - want: returnValue{ - Error: nil, - Result: false, - NumberOfErrors: 6, + name: "proc substitution", + filename: "testdata/Dockerfile-proc-subs", + expected: scut.TestReturn{ + Errors: nil, + Score: checker.MinResultScore, + NumberOfWarn: 6, + NumberOfInfo: 0, + NumberOfDebug: 0, }, }, { - name: "wget file", - args: args{ - Filename: "testdata/Dockerfile-wget-file", - Log: log{}, - }, - want: returnValue{ - Error: nil, - Result: false, - NumberOfErrors: 10, + name: "wget file", + filename: "testdata/Dockerfile-wget-file", + expected: scut.TestReturn{ + Errors: nil, + Score: checker.MinResultScore, + NumberOfWarn: 10, + NumberOfInfo: 0, + NumberOfDebug: 0, }, }, { - name: "gsutil file", - args: args{ - Filename: "testdata/Dockerfile-gsutil-file", - Log: log{}, - }, - want: returnValue{ - Error: nil, - Result: false, - NumberOfErrors: 17, + name: "gsutil file", + filename: "testdata/Dockerfile-gsutil-file", + expected: scut.TestReturn{ + Errors: nil, + Score: checker.MinResultScore, + NumberOfWarn: 17, + NumberOfInfo: 0, + NumberOfDebug: 0, }, }, { - name: "aws file", - args: args{ - Filename: "testdata/Dockerfile-aws-file", - Log: log{}, - }, - want: returnValue{ - Error: nil, - Result: false, - NumberOfErrors: 15, + name: "aws file", + filename: "testdata/Dockerfile-aws-file", + expected: scut.TestReturn{ + Errors: nil, + Score: checker.MinResultScore, + NumberOfWarn: 15, + NumberOfInfo: 0, + NumberOfDebug: 0, }, }, { - name: "pkg managers", - args: args{ - Filename: "testdata/Dockerfile-pkg-managers", - Log: log{}, - }, - want: returnValue{ - Error: nil, - Result: false, - NumberOfErrors: 27, + name: "pkg managers", + filename: "testdata/Dockerfile-pkg-managers", + expected: scut.TestReturn{ + Errors: nil, + Score: checker.MinResultScore, + NumberOfWarn: 27, + NumberOfInfo: 0, + NumberOfDebug: 0, }, }, } - //nolint for _, tt := range tests { tt := tt // Re-initializing variable so it is not changed while executing the closure below t.Run(tt.name, func(t *testing.T) { t.Parallel() var content []byte var err error - content, err = ioutil.ReadFile(tt.args.Filename) - if err != nil { - panic(fmt.Errorf("cannot read file: %w", err)) - } - - r, err := validateDockerfileDownloads(tt.args.Filename, content, tt.args.Log.Logf) - - if !errors.Is(err, tt.want.Error) || - r != tt.want.Result || - len(tt.args.Log.messages) != tt.want.NumberOfErrors { - t.Errorf("TestDockerfileScriptDownload:\"%v\": %v (%v,%v,%v) want (%v, %v, %v)\n%v", - tt.name, tt.args.Filename, r, err, len(tt.args.Log.messages), tt.want.Result, tt.want.Error, tt.want.NumberOfErrors, - strings.Join(tt.args.Log.messages, "\n")) + if tt.filename == "" { + content = make([]byte, 0) + } else { + content, err = ioutil.ReadFile(tt.filename) + if err != nil { + panic(fmt.Errorf("cannot read file: %w", err)) + } } + dl := scut.TestDetailLogger{} + r := TestValidateDockerfileIsFreeOfInsecureDownloads(tt.filename, content, &dl) + scut.ValidateTestReturn2(t, tt.name, &tt.expected, &r, &dl) }) } } func TestShellScriptDownload(t *testing.T) { t.Parallel() - //nolint - type args struct { - // Note: this seems to be defined in e2e/e2e_suite_test.go - Log log - Filename string - } - - type returnValue struct { - Error error - Result bool - NumberOfErrors int - } - - //nolint tests := []struct { - args args - want returnValue - name string + name string + filename string + expected scut.TestReturn }{ { - name: "sh script", - args: args{ - Filename: "testdata/script-sh", - Log: log{}, - }, - want: returnValue{ - Error: nil, - Result: false, - NumberOfErrors: 7, + name: "sh script", + filename: "testdata/script-sh", + expected: scut.TestReturn{ + Errors: nil, + Score: checker.MinResultScore, + NumberOfWarn: 7, + NumberOfInfo: 0, + NumberOfDebug: 0, }, }, { - name: "bash script", - args: args{ - Filename: "testdata/script-bash", - Log: log{}, - }, - want: returnValue{ - Error: nil, - Result: false, - NumberOfErrors: 7, + name: "bash script", + filename: "testdata/script-bash", + expected: scut.TestReturn{ + Errors: nil, + Score: checker.MinResultScore, + NumberOfWarn: 7, + NumberOfInfo: 0, + NumberOfDebug: 0, }, }, { - name: "sh script 2", - args: args{ - Filename: "testdata/script.sh", - Log: log{}, - }, - want: returnValue{ - Error: nil, - Result: false, - NumberOfErrors: 7, + name: "sh script 2", + filename: "testdata/script.sh", + expected: scut.TestReturn{ + Errors: nil, + Score: checker.MinResultScore, + NumberOfWarn: 7, + NumberOfInfo: 0, + NumberOfDebug: 0, }, }, { - name: "pkg managers", - args: args{ - Filename: "testdata/script-pkg-managers", - Log: log{}, - }, - want: returnValue{ - Error: nil, - Result: false, - NumberOfErrors: 24, + name: "pkg managers", + filename: "testdata/script-pkg-managers", + expected: scut.TestReturn{ + Errors: nil, + Score: checker.MinResultScore, + NumberOfWarn: 24, + NumberOfInfo: 0, + NumberOfDebug: 0, }, }, } - //nolint for _, tt := range tests { tt := tt // Re-initializing variable so it is not changed while executing the closure below t.Run(tt.name, func(t *testing.T) { t.Parallel() var content []byte var err error - content, err = ioutil.ReadFile(tt.args.Filename) - if err != nil { - panic(fmt.Errorf("cannot read file: %w", err)) - } - - r, err := validateShellScriptDownloads(tt.args.Filename, content, tt.args.Log.Logf) - - if !errors.Is(err, tt.want.Error) || - r != tt.want.Result || - len(tt.args.Log.messages) != tt.want.NumberOfErrors { - t.Errorf("TestDockerfileScriptDownload:\"%v\": %v (%v,%v,%v) want (%v, %v, %v)\n%v", - tt.name, tt.args.Filename, r, err, len(tt.args.Log.messages), tt.want.Result, tt.want.Error, tt.want.NumberOfErrors, - strings.Join(tt.args.Log.messages, "\n")) + if tt.filename == "" { + content = make([]byte, 0) + } else { + content, err = ioutil.ReadFile(tt.filename) + if err != nil { + panic(fmt.Errorf("cannot read file: %w", err)) + } } + dl := scut.TestDetailLogger{} + r := TestValidateShellScriptIsFreeOfInsecureDownloads(tt.filename, content, &dl) + scut.ValidateTestReturn2(t, tt.name, &tt.expected, &r, &dl) }) } } func TestGitHubWorflowRunDownload(t *testing.T) { t.Parallel() - //nolint - type args struct { - // Note: this seems to be defined in e2e/e2e_suite_test.go - Log log - Filename string - } - - type returnValue struct { - Error error - Result bool - NumberOfErrors int - } - - //nolint tests := []struct { - args args - want returnValue - name string + name string + filename string + expected scut.TestReturn }{ { - name: "workflow curl default", - args: args{ - Filename: "testdata/github-workflow-curl-default", - Log: log{}, - }, - want: returnValue{ - Error: nil, - Result: false, - NumberOfErrors: 1, + name: "workflow curl default", + filename: "testdata/github-workflow-curl-default", + expected: scut.TestReturn{ + Errors: nil, + Score: checker.MinResultScore, + NumberOfWarn: 1, + NumberOfInfo: 0, + NumberOfDebug: 0, }, }, { - name: "workflow curl no default", - args: args{ - Filename: "testdata/github-workflow-curl-no-default", - Log: log{}, - }, - want: returnValue{ - Error: nil, - Result: false, - NumberOfErrors: 1, + name: "workflow curl no default", + filename: "testdata/github-workflow-curl-no-default", + expected: scut.TestReturn{ + Errors: nil, + Score: checker.MinResultScore, + NumberOfWarn: 1, + NumberOfInfo: 0, + NumberOfDebug: 0, }, }, { - name: "wget across steps", - args: args{ - Filename: "testdata/github-workflow-wget-across-steps", - Log: log{}, - }, - want: returnValue{ - Error: nil, - Result: false, - NumberOfErrors: 2, + name: "wget across steps", + filename: "testdata/github-workflow-wget-across-steps", + expected: scut.TestReturn{ + Errors: nil, + Score: checker.MinResultScore, + NumberOfWarn: 2, + NumberOfInfo: 0, + NumberOfDebug: 0, }, }, } - //nolint for _, tt := range tests { tt := tt // Re-initializing variable so it is not changed while executing the closure below t.Run(tt.name, func(t *testing.T) { t.Parallel() var content []byte var err error - content, err = ioutil.ReadFile(tt.args.Filename) - if err != nil { - panic(fmt.Errorf("cannot read file: %w", err)) - } - - r, err := validateGitHubWorkflowShellScriptDownloads(tt.args.Filename, content, tt.args.Log.Logf) - - if !errors.Is(err, tt.want.Error) || - r != tt.want.Result || - len(tt.args.Log.messages) != tt.want.NumberOfErrors { - t.Errorf("TestDockerfileScriptDownload:\"%v\": %v (%v,%v,%v) want (%v, %v, %v)\n%v", - tt.name, tt.args.Filename, r, err, len(tt.args.Log.messages), tt.want.Result, tt.want.Error, tt.want.NumberOfErrors, - strings.Join(tt.args.Log.messages, "\n")) + if tt.filename == "" { + content = make([]byte, 0) + } else { + content, err = ioutil.ReadFile(tt.filename) + if err != nil { + panic(fmt.Errorf("cannot read file: %w", err)) + } } + dl := scut.TestDetailLogger{} + r := TestValidateGitHubWorkflowScriptFreeOfInsecureDownloads(tt.filename, content, &dl) + scut.ValidateTestReturn2(t, tt.name, &tt.expected, &r, &dl) }) } } diff --git a/checks/permissions.go b/checks/permissions.go index 838dfd0cebb..5e2bc1dae13 100644 --- a/checks/permissions.go +++ b/checks/permissions.go @@ -22,6 +22,7 @@ import ( "gopkg.in/yaml.v2" "github.com/ossf/scorecard/checker" + sce "github.com/ossf/scorecard/errors" ) const CheckPermissions = "Token-Permissions" @@ -121,7 +122,8 @@ func validateReadPermissions(config map[interface{}]interface{}, path string, func validateGitHubActionTokenPermissions(path string, content []byte, logf func(s string, f ...interface{})) (bool, error) { if len(content) == 0 { - return false, ErrEmptyFile + //nolint + return false, sce.Create(sce.ErrRunFailure, sce.ErrInternalEmptyFile.Error()) } var workflow map[interface{}]interface{} diff --git a/checks/permissions_test.go b/checks/permissions_test.go index ffbd496c8f5..573d08dc4a0 100644 --- a/checks/permissions_test.go +++ b/checks/permissions_test.go @@ -14,13 +14,7 @@ package checks -import ( - "errors" - "fmt" - "io/ioutil" - "testing" -) - +/* func TestGithubTokenPermissions(t *testing.T) { t.Parallel() type args struct { @@ -142,3 +136,4 @@ func TestGithubTokenPermissions(t *testing.T) { }) } } +*/ diff --git a/checks/shell_download_validate.go b/checks/shell_download_validate.go index 064eb2697be..6c87209e4cc 100644 --- a/checks/shell_download_validate.go +++ b/checks/shell_download_validate.go @@ -17,7 +17,6 @@ package checks import ( "bufio" "bytes" - "errors" "fmt" "net/url" "path" @@ -26,13 +25,10 @@ import ( "strings" "mvdan.cc/sh/v3/syntax" -) - -// ErrParsingDockerfile indicates a problem parsing the dockerfile. -var ErrParsingDockerfile = errors.New("file cannot be parsed") -// ErrParsingShellCommand indicates a problem parsing a shell command. -var ErrParsingShellCommand = errors.New("shell command cannot be parsed") + "github.com/ossf/scorecard/checker" + sce "github.com/ossf/scorecard/errors" +) // List of interpreters. var pythonInterpreters = []string{"python", "python3", "python2.7"} @@ -106,7 +102,8 @@ func getWgetOutputFile(cmd []string) (pathfn string, ok bool, err error) { u, err := url.Parse(cmd[i]) if err != nil { - return "", false, fmt.Errorf("url.Parse: %w", err) + //nolint + return "", false, sce.Create(sce.ErrRunFailure, fmt.Sprintf("url.Parse: %v", err)) } return path.Base(u.Path), true, nil } @@ -125,7 +122,8 @@ func getGsutilOutputFile(cmd []string) (pathfn string, ok bool, err error) { // Directory. u, err := url.Parse(cmd[i]) if err != nil { - return "", false, fmt.Errorf("url.Parse: %w", err) + //nolint + return "", false, sce.Create(sce.ErrRunFailure, fmt.Sprintf("url.Parse: %v", err)) } return filepath.Join(filepath.Dir(pathfn), path.Base(u.Path)), true, nil } @@ -150,7 +148,8 @@ func getAWSOutputFile(cmd []string) (pathfn string, ok bool, err error) { if filepath.Clean(filepath.Dir(ofile)) == filepath.Clean(ofile) { u, err := url.Parse(ifile) if err != nil { - return "", false, fmt.Errorf("url.Parse: %w", err) + //nolint + return "", false, sce.Create(sce.ErrRunFailure, fmt.Sprintf("url.Parse: %v", err)) } return filepath.Join(filepath.Dir(ofile), path.Base(u.Path)), true, nil } @@ -266,7 +265,7 @@ func extractCommand(cmd interface{}) ([]string, bool) { } func isFetchPipeExecute(node syntax.Node, cmd, pathfn string, - logf func(s string, f ...interface{})) bool { + dl checker.DetailLogger) bool { // BinaryCmd {Op=|, X=CallExpr{Args={curl, -s, url}}, Y=CallExpr{Args={bash,}}}. bc, ok := node.(*syntax.BinaryCmd) if !ok { @@ -295,8 +294,7 @@ func isFetchPipeExecute(node syntax.Node, cmd, pathfn string, return false } - logf("!! frozen-deps/fetch-execute - %v is fetching and executing non-pinned program '%v'", - pathfn, cmd) + dl.Warn("insecure (unpinned) download detected in %v: '%v'", pathfn, cmd) return true } @@ -322,7 +320,7 @@ func getRedirectFile(red []*syntax.Redirect) (string, bool) { } func isExecuteFiles(node syntax.Node, cmd, pathfn string, files map[string]bool, - logf func(s string, f ...interface{})) bool { + dl checker.DetailLogger) bool { ce, ok := node.(*syntax.CallExpr) if !ok { return false @@ -336,8 +334,7 @@ func isExecuteFiles(node syntax.Node, cmd, pathfn string, files map[string]bool, ok = false for fn := range files { if isInterpreterWithFile(c, fn) || isExecuteFile(c, fn) { - logf("!! frozen-deps/fetch-execute - %v is fetching and executing non-pinned program '%v'", - pathfn, cmd) + dl.Warn("insecure (unpinned) download detected in %v: '%v'", pathfn, cmd) ok = true } } @@ -479,7 +476,7 @@ func isPipUnpinnedDownload(cmd []string) bool { } func isUnpinnedPakageManagerDownload(node syntax.Node, cmd, pathfn string, - logf func(s string, f ...interface{})) bool { + dl checker.DetailLogger) bool { ce, ok := node.(*syntax.CallExpr) if !ok { return false @@ -492,15 +489,13 @@ func isUnpinnedPakageManagerDownload(node syntax.Node, cmd, pathfn string, // Go get/install. if isGoUnpinnedDownload(c) { - logf("!! frozen-deps/fetch-execute - %v is fetching an non-pinned dependency '%v'", - pathfn, cmd) + dl.Warn("insecure (unpinned) download detected in %v: '%v'", pathfn, cmd) return true } // Pip install. if isPipUnpinnedDownload(c) { - logf("!! frozen-deps/fetch-execute - %v is fetching an non-pinned dependency '%v'", - pathfn, cmd) + dl.Warn("insecure (unpinned) download detected in %v: '%v'", pathfn, cmd) return true } @@ -533,7 +528,7 @@ func recordFetchFileFromNode(node syntax.Node) (pathfn string, ok bool, err erro } func isFetchProcSubsExecute(node syntax.Node, cmd, pathfn string, - logf func(s string, f ...interface{})) bool { + dl checker.DetailLogger) bool { ce, ok := node.(*syntax.CallExpr) if !ok { return false @@ -583,8 +578,7 @@ func isFetchProcSubsExecute(node syntax.Node, cmd, pathfn string, return false } - logf("!! frozen-deps/fetch-execute - %v is fetching and executing non-pinned program '%v'", - pathfn, cmd) + dl.Warn("insecure (unpinned) download detected in %v: '%v'", pathfn, cmd) return true } @@ -655,17 +649,20 @@ func nodeToString(p *syntax.Printer, node syntax.Node) (string, error) { err := p.Print(&buf, node) // This is ugly, but the parser does not have a defined error type :/. if err != nil && !strings.Contains(err.Error(), "unsupported node type") { - return "", fmt.Errorf("syntax.Printer.Print: %w", err) + //nolint + return "", sce.Create(sce.ErrRunFailure, fmt.Sprintf("syntax.Printer.Print: %v", err)) } return buf.String(), nil } func validateShellFileAndRecord(pathfn string, content []byte, files map[string]bool, - logf func(s string, f ...interface{})) (bool, error) { + dl checker.DetailLogger) (bool, error) { in := strings.NewReader(string(content)) f, err := syntax.NewParser().Parse(in, "") if err != nil { - return false, ErrParsingShellCommand + //nolint + return false, sce.Create(sce.ErrRunFailure, + fmt.Sprintf("%v: %v", sce.ErrInternalInvalidShellCode, err)) } printer := syntax.NewPrinter() @@ -682,7 +679,7 @@ func validateShellFileAndRecord(pathfn string, content []byte, files map[string] c, ok := extractInterpreterCommandFromNode(node) // nolinter if ok { - ok, e := validateShellFileAndRecord(pathfn, []byte(c), files, logf) + ok, e := validateShellFileAndRecord(pathfn, []byte(c), files, dl) validated = ok if e != nil { err = e @@ -691,23 +688,23 @@ func validateShellFileAndRecord(pathfn string, content []byte, files map[string] } // `curl | bash` (supports `sudo`). - if isFetchPipeExecute(node, cmdStr, pathfn, logf) { + if isFetchPipeExecute(node, cmdStr, pathfn, dl) { validated = false } // Check if we're calling a file we previously downloaded. // Includes `curl > /tmp/file [&&|;] [bash] /tmp/file` - if isExecuteFiles(node, cmdStr, pathfn, files, logf) { + if isExecuteFiles(node, cmdStr, pathfn, files, dl) { validated = false } // `bash <(wget -qO- http://website.com/my-script.sh)`. (supports `sudo`). - if isFetchProcSubsExecute(node, cmdStr, pathfn, logf) { + if isFetchProcSubsExecute(node, cmdStr, pathfn, dl) { validated = false } // Package manager's unpinned installs. - if isUnpinnedPakageManagerDownload(node, cmdStr, pathfn, logf) { + if isUnpinnedPakageManagerDownload(node, cmdStr, pathfn, dl) { validated = false } // TODO(laurent): add check for cat file | bash. @@ -783,7 +780,7 @@ func isShellScriptFile(pathfn string, content []byte) bool { return false } -func validateShellFile(pathfn string, content []byte, logf func(s string, f ...interface{})) (bool, error) { +func validateShellFile(pathfn string, content []byte, dl checker.DetailLogger) (bool, error) { files := make(map[string]bool) - return validateShellFileAndRecord(pathfn, content, files, logf) + return validateShellFileAndRecord(pathfn, content, files, dl) } diff --git a/checks/testdata/Dockerfile-invalid b/checks/testdata/Dockerfile-invalid index 34d16b0231a..2f6d167136a 100644 --- a/checks/testdata/Dockerfile-invalid +++ b/checks/testdata/Dockerfile-invalid @@ -14,24 +14,4 @@ # limitations under the License. # Note: taken from https://github.com/pushiqiang/utils/blob/master/docker/Dockerfile_template -# 如果在中国，apt使用163源, ifconfig.co/json, http://ip-api.com -RUN curl -s ifconfig.co/json | grep "China" > /dev/null && \ - curl -s http://mirrors.163.com/.help/sources.list.jessie > /etc/apt/sources.list || true - -# 安装开发所需要的一些工具，同时方便在服务器上进行调试 -RUN apt-get update;\ - apt-get install -y vim;\ - true - -RUN mkdir /opt/somedir - -ENV PROJECT_NAME='test' -ENV PYTHONPATH="${PYTHONPATH}:/opt/somedir" - -COPY src/ /opt/somedir -WORKDIR /opt/somedir - -# 如果在中国，pip使用豆瓣源 -RUN curl -s ifconfig.co/json | grep "China" > /dev/null && \ - pip install -r requirements.txt -i https://pypi.doubanio.com/simple --trusted-host pypi.doubanio.com || \ - pip install -r requirements.txt \ No newline at end of file +#!/bin/sh diff --git a/checks/testdata/Dockerfile-not-pinned-as b/checks/testdata/Dockerfile-not-pinned-as index 8af7a8ba6a2..1a2f30c7e56 100644 --- a/checks/testdata/Dockerfile-not-pinned-as +++ b/checks/testdata/Dockerfile-not-pinned-as @@ -23,4 +23,6 @@ RUN CGO_ENABLED=0 make build-scorecard FROM build RUN /hello-world +FROM base + FROM python@sha256:45b23dee08af5e43a7fea6c4cf9c25ccf269ee113168c19722f87876677c5cb2 \ No newline at end of file diff --git a/checks/write.md b/checks/write.md new file mode 100644 index 00000000000..955fdea0dfd --- /dev/null +++ b/checks/write.md @@ -0,0 +1,33 @@ +# How to write a check +The steps to writting a check are as follow: + +1. Create a file under `checks/` folder, say `checks/mycheck.go` +2. Give the check a name and register the check: +``` +// Note: do not export the name: start its name with a lower-case letter. +const checkMyCheckName string = "My-Check" + +func init() { + registerCheck(checkMyCheckName, EntryPointMyCheck) +} +``` +3. Log information that is benfical to the user using `checker.DetailLogger`: + * Use `checker.DetailLogger.Warn()` to provide detail on low-score results. This is showed when the user supplies the `show-results` option. + * Use `checker.DetailLogger.Info()` to provide detail on high-score results. This is showed when the user supplies the `show-results` option. + * Use `checker.DetailLogger.Debug()` to provide detail in verbose mode: this is showed only when the user supplies the `--verbosity Debug` option. +4. If the checks fails in a way that is irrecoverable, return a result with `checker.CreateRuntimeErrorResult()` function: For example, +if an error is returned from an API you call, use the function. +5. Create the result of the check as follow: + * Always provide a high-level sentence explaining the result/score of the check. + * If the check runs properly but is unable to determine a score, use `checker.CreateInconclusiveResult()` function. + * For propertional results, use `checker.CreateProportionalScoreResult()`. + * For maximum score, use `checker.CreateMaxScoreResult()`; for min score use `checker.CreateMinScoreResult()`. + * If you need more flexibility and need to set a specific score, use `checker.CreateResultWithScore()` with one of the constants declared, such as `checker.HalfResultScore`. +6. Dealing with errors: see [../errors/errors.md](errors/errors/md). +7. Create unit tests for both low, high and inconclusive score. Put them in a file `checks/mycheck_test.go`. +8. Create e2e tests in `e2e/mycheck_test.go`. Use a dedicated repo that will not change over time, so that it's reliable for the tests. +9. Update the `checks/checks.yaml` with the description of your check. +10. Gerenate the `checks/check.md` using `go build && cd checks/main && ./main`. Verify `checks/check.md` was updated. +10. Update the [README.md](https://github.com/ossf/scorecard#scorecard-checks) with a short description of your check. + +For actual examples, look at [checks/binary_artifact.go](binary_artifact.go), [checks/code_review.go](code_review.go) and [checks/frozen_deps.go](frozen_deps.go). \ No newline at end of file diff --git a/cmd/root.go b/cmd/root.go index 98dea859b3a..7bf56267d67 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -51,6 +51,9 @@ var ( pypi string rubygems string showDetails bool + // UPGRADEv2: will be removed. + v2 bool + // TODO(laurent): add explain command. // ErrorInvalidFormatFlag indicates an invalid option was passed for the 'format' argument. ErrorInvalidFormatFlag = errors.New("invalid format flag") ) @@ -157,11 +160,15 @@ or ./scorecard --{npm,pypi,rubgems}= [--checks=check1,...] [--show switch format { case formatDefault: - err = repoResult.AsString(showDetails, os.Stdout) + if v2 { + err = repoResult.AsString2(showDetails, *logLevel, os.Stdout) + } else { + err = repoResult.AsString(showDetails, *logLevel, os.Stdout) + } case formatCSV: - err = repoResult.AsCSV(showDetails, os.Stdout) + err = repoResult.AsCSV(showDetails, *logLevel, os.Stdout) case formatJSON: - err = repoResult.AsJSON(showDetails, os.Stdout) + err = repoResult.AsJSON(showDetails, *logLevel, os.Stdout) default: err = fmt.Errorf("%w %s. allowed values are: [default, csv, json]", ErrorInvalidFormatFlag, format) } @@ -306,6 +313,8 @@ func init() { rootCmd.Flags().StringSliceVar( &metaData, "metadata", []string{}, "metadata for the project.It can be multiple separated by commas") rootCmd.Flags().BoolVar(&showDetails, "show-details", false, "show extra details about each check") + // UPGRADEv2: will be removed. + rootCmd.Flags().BoolVar(&v2, "v2", false, "temporary flag to display v2 changes") checkNames := []string{} for checkName := range checks.AllChecks { checkNames = append(checkNames, checkName) diff --git a/cmd/serve.go b/cmd/serve.go index 42917f1aaaf..8d5cb02feda 100644 --- a/cmd/serve.go +++ b/cmd/serve.go @@ -85,7 +85,7 @@ var serveCmd = &cobra.Command{ } if r.Header.Get("Content-Type") == "application/json" { - if err := repoResult.AsJSON(showDetails, rw); err != nil { + if err := repoResult.AsJSON(showDetails, *logLevel, rw); err != nil { sugar.Error(err) rw.WriteHeader(http.StatusInternalServerError) } diff --git a/cron/worker/main.go b/cron/worker/main.go index 0a3e5467ad2..d8aabfe0ea7 100644 --- a/cron/worker/main.go +++ b/cron/worker/main.go @@ -29,6 +29,7 @@ import ( "github.com/shurcooL/githubv4" "go.opencensus.io/stats/view" "go.uber.org/zap" + "go.uber.org/zap/zapcore" "github.com/ossf/scorecard/checker" "github.com/ossf/scorecard/checks" @@ -89,7 +90,7 @@ func processRequest(ctx context.Context, return fmt.Errorf("error during RunScorecards: %w", err) } result.Date = batchRequest.GetJobTime().AsTime().Format("2006-01-02") - if err := result.AsJSON(true /*showDetails*/, &buffer); err != nil { + if err := result.AsJSON(true /*showDetails*/, zapcore.InfoLevel, &buffer); err != nil { return fmt.Errorf("error during result.AsJSON: %w", err) } } diff --git a/e2e/active_test.go b/e2e/active_test.go index 6d7959f9634..aa8b6d7ec3e 100644 --- a/e2e/active_test.go +++ b/e2e/active_test.go @@ -16,19 +16,21 @@ package e2e import ( "context" + "fmt" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" "github.com/ossf/scorecard/checker" "github.com/ossf/scorecard/checks" + scut "github.com/ossf/scorecard/utests" ) var _ = Describe("E2E TEST:Active", func() { Context("E2E TEST:Validating active status", func() { It("Should return valid active status", func() { - l := log{} - checkRequest := checker.CheckRequest{ + dl := scut.TestDetailLogger{} + req := checker.CheckRequest{ Ctx: context.Background(), Client: ghClient, HTTPClient: httpClient, @@ -36,11 +38,23 @@ var _ = Describe("E2E TEST:Active", func() { Owner: "apache", Repo: "airflow", GraphClient: graphClient, - Logf: l.Logf, + Dlogger: &dl, } - result := checks.IsActive(&checkRequest) + expected := scut.TestReturn{ + Errors: nil, + Score: checker.MaxResultScore, + NumberOfWarn: 0, + NumberOfInfo: 0, + NumberOfDebug: 0, + } + result := checks.IsActive(&req) + // UPGRADEv2: to remove. + // Old version. Expect(result.Error).Should(BeNil()) Expect(result.Pass).Should(BeTrue()) + fmt.Printf("%v", result) + // New version. + Expect(scut.ValidateTestReturn(nil, "active repo", &expected, &result, &dl)).Should(BeTrue()) }) }) }) diff --git a/e2e/automatic_deps_test.go b/e2e/automatic_deps_test.go index 22c81c99b60..865ea0ac329 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,74 @@ 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, + } + expected := scut.TestReturn{ + Errors: nil, + Score: checker.MaxResultScore, + NumberOfWarn: 0, + NumberOfInfo: 1, + NumberOfDebug: 0, } - result := checks.AutomaticDependencyUpdate(&checker) + + 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, + } + expected := scut.TestReturn{ + Errors: nil, + Score: checker.MaxResultScore, + NumberOfWarn: 0, + NumberOfInfo: 1, + NumberOfDebug: 0, } - result := checks.AutomaticDependencyUpdate(&checker) + 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 00000000000..9df3a5b1fb8 --- /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/e2e/branchprotection_test.go b/e2e/branchprotection_test.go index b8464bbeb34..e691f83dff8 100644 --- a/e2e/branchprotection_test.go +++ b/e2e/branchprotection_test.go @@ -16,19 +16,21 @@ package e2e import ( "context" + "fmt" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" "github.com/ossf/scorecard/checker" "github.com/ossf/scorecard/checks" + scut "github.com/ossf/scorecard/utests" ) var _ = Describe("E2E TEST:Branch Protection", func() { Context("E2E TEST:Validating branch protection", func() { It("Should fail to return branch protection on other repositories", func() { - l := log{} - checkRequest := checker.CheckRequest{ + dl := scut.TestDetailLogger{} + req := checker.CheckRequest{ Ctx: context.Background(), Client: ghClient, HTTPClient: httpClient, @@ -36,11 +38,23 @@ var _ = Describe("E2E TEST:Branch Protection", func() { Owner: "apache", Repo: "airflow", GraphClient: graphClient, - Logf: l.Logf, + Dlogger: &dl, } - result := checks.BranchProtection(&checkRequest) - Expect(result.Error).ShouldNot(BeNil()) + expected := scut.TestReturn{ + Errors: nil, + Score: checker.MinResultScore, + NumberOfWarn: 1, + NumberOfInfo: 0, + NumberOfDebug: 0, + } + result := checks.BranchProtection(&req) + // UPGRADEv2: to remove. + // Old version. + Expect(result.Error).Should(BeNil()) Expect(result.Pass).Should(BeFalse()) + panic(fmt.Sprintf("%v", result)) + // New version. + Expect(scut.ValidateTestReturn(nil, "branch protection not enabled", &expected, &result, &dl)).Should(BeTrue()) }) }) }) diff --git a/e2e/code_review_test.go b/e2e/code_review_test.go index 0c29eff7b9b..48afad94fad 100644 --- a/e2e/code_review_test.go +++ b/e2e/code_review_test.go @@ -22,13 +22,16 @@ import ( "github.com/ossf/scorecard/checker" "github.com/ossf/scorecard/checks" + scut "github.com/ossf/scorecard/utests" ) +// TODO: use dedicated repo that don't change. +// TODO: need negative results. var _ = Describe("E2E TEST:CodeReview", func() { Context("E2E TEST:Validating use of code reviews", func() { It("Should return use of code reviews", func() { - l := log{} - checkRequest := checker.CheckRequest{ + dl := scut.TestDetailLogger{} + req := checker.CheckRequest{ Ctx: context.Background(), Client: ghClient, HTTPClient: httpClient, @@ -36,11 +39,22 @@ var _ = Describe("E2E TEST:CodeReview", func() { Owner: "apache", Repo: "airflow", GraphClient: graphClient, - Logf: l.Logf, + Dlogger: &dl, } - result := checks.DoesCodeReview(&checkRequest) + expected := scut.TestReturn{ + Errors: nil, + Score: checker.MaxResultScore, + NumberOfWarn: 0, + NumberOfInfo: 0, + NumberOfDebug: 30, + } + result := checks.DoesCodeReview(&req) + // UPGRADEv2: to remove. + // Old version. Expect(result.Error).Should(BeNil()) Expect(result.Pass).Should(BeTrue()) + // New version. + Expect(scut.ValidateTestReturn(nil, "use code reviews", &expected, &result, &dl)).Should(BeTrue()) }) }) }) diff --git a/e2e/executable_test.go b/e2e/executable_test.go index a3158d0b656..2f6f4ec62c4 100644 --- a/e2e/executable_test.go +++ b/e2e/executable_test.go @@ -48,6 +48,7 @@ var _ = Describe("E2E TEST:executable", func() { Expect(len(data.MetaData)).ShouldNot(BeZero()) Expect(data.MetaData[0]).Should(BeEquivalentTo("openssf")) + // UPGRADEv2: TBD. for _, c := range data.Checks { switch c.CheckName { case "Active": diff --git a/e2e/frozen_deps_test.go b/e2e/frozen_deps_test.go index 3844ddc83b2..cde9d71d084 100644 --- a/e2e/frozen_deps_test.go +++ b/e2e/frozen_deps_test.go @@ -12,7 +12,6 @@ // See the License for the specific language governing permissions and // limitations under the License. -//nolint: dupl // repeating test cases that are slightly different is acceptable package e2e import ( @@ -24,17 +23,20 @@ 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:FrozenDeps", func() { Context("E2E TEST:Validating deps are frozen", func() { It("Should return deps are not frozen", func() { - l := log{} + dl := scut.TestDetailLogger{} repoClient := githubrepo.CreateGithubRepoClient(context.Background(), ghClient) err := repoClient.InitRepo("tensorflow", "tensorflow") Expect(err).Should(BeNil()) - checkRequest := checker.CheckRequest{ + req := checker.CheckRequest{ Ctx: context.Background(), Client: ghClient, HTTPClient: httpClient, @@ -42,19 +44,30 @@ var _ = Describe("E2E TEST:FrozenDeps", func() { Owner: "tensorflow", Repo: "tensorflow", GraphClient: graphClient, - Logf: l.Logf, + Dlogger: &dl, } - result := checks.FrozenDeps(&checkRequest) + expected := scut.TestReturn{ + Errors: nil, + Score: checker.InconclusiveResultScore, + NumberOfWarn: 219, + NumberOfInfo: 0, + NumberOfDebug: 0, + } + result := checks.FrozenDeps(&req) + // UPGRADEv2: to remove. + // Old version. Expect(result.Error).Should(BeNil()) Expect(result.Pass).Should(BeFalse()) + // New version. + Expect(scut.ValidateTestReturn(nil, "deps not frozen", &expected, &result, &dl)).Should(BeTrue()) }) It("Should return deps are frozen", func() { - l := log{} + dl := scut.TestDetailLogger{} repoClient := githubrepo.CreateGithubRepoClient(context.Background(), ghClient) err := repoClient.InitRepo("ossf", "scorecard") Expect(err).Should(BeNil()) - checkRequest := checker.CheckRequest{ + req := checker.CheckRequest{ Ctx: context.Background(), Client: ghClient, HTTPClient: httpClient, @@ -62,11 +75,22 @@ var _ = Describe("E2E TEST:FrozenDeps", func() { Owner: "ossf", Repo: "scorecard", GraphClient: graphClient, - Logf: l.Logf, + Dlogger: &dl, + } + expected := scut.TestReturn{ + Errors: nil, + Score: checker.MaxResultScore, + NumberOfWarn: 0, + NumberOfInfo: 1, + NumberOfDebug: 0, } - result := checks.FrozenDeps(&checkRequest) + result := checks.FrozenDeps(&req) + // UPGRADEv2: to remove. + // Old version. Expect(result.Error).Should(BeNil()) Expect(result.Pass).Should(BeTrue()) + // New version. + Expect(scut.ValidateTestReturn(nil, "deps frozen", &expected, &result, &dl)).Should(BeTrue()) }) }) }) diff --git a/e2e/security_policy_test.go b/e2e/security_policy_test.go index 7ac87afdff2..d943c6a86df 100644 --- a/e2e/security_policy_test.go +++ b/e2e/security_policy_test.go @@ -12,7 +12,6 @@ // See the License for the specific language governing permissions and // limitations under the License. -//nolint:dupl // repeating test cases that are slightly different is acceptable package e2e import ( diff --git a/errors/errors.md b/errors/errors.md new file mode 100644 index 00000000000..01ee98e99e8 --- /dev/null +++ b/errors/errors.md @@ -0,0 +1,28 @@ +# How to handle errors + +```golang +import sce "github.com/ossf/scorecard/errors" + +// Public errors are defined in errors/public.go and are exposed to callers. +// Internal errors are defined in errors/innternal.go. Their names start with ErrInternalXXX + +// Examples: + +// 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 erorr messages to the caller. +return sce.Create(sce.ErrRunFailure, 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", sce.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)) +} +``` \ No newline at end of file diff --git a/errors/internal.go b/errors/internal.go new file mode 100644 index 00000000000..1b2844c1f28 --- /dev/null +++ b/errors/internal.go @@ -0,0 +1,30 @@ +// Copyright 2020 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 errors + +import ( + "errors" +) + +//nolint +var ( + ErrInternalInvalidDockerFile = errors.New("invalid Dockerfile") + ErrInternalInvalidYamlFile = errors.New("invalid yaml file") + ErrInternalFilenameMatch = errors.New("filename match error") + ErrInternalEmptyFile = errors.New("empty file") + ErrInternalInvalidShellCode = errors.New("invalid shell code") + ErrCommitishNil = errors.New("commitish is nil") + ErrBranchNotFound = errors.New("branch not found") +) diff --git a/errors/names.go b/errors/names.go index b78162b5ccc..81df9e14c46 100644 --- a/errors/names.go +++ b/errors/names.go @@ -21,23 +21,23 @@ import ( const ( // RetryError occurs when checks fail after exhausting all retry attempts. RetryError = "RetryError" - // ZeroConfidenceError shows an inconclusive result. - ZeroConfidenceError = "ZeroConfidenceError" + // LowConfidenceError shows a low-confidence result. + LowConfidenceError = "LowConfidenceError" // UnknownError for all error types not handled. UnknownError = "UnknownError" ) var ( - errRetry *ErrRetry - errZeroConfidence *ErrZeroConfidence + errRetry *ErrRetry + errLowConfidence *ErrLowConfidence ) func GetErrorName(err error) string { switch { case errors.As(err, &errRetry): return RetryError - case errors.As(err, &errZeroConfidence): - return ZeroConfidenceError + case errors.As(err, &errLowConfidence): + return LowConfidenceError default: return UnknownError } diff --git a/errors/public.go b/errors/public.go new file mode 100644 index 00000000000..c0fc4ba5ef7 --- /dev/null +++ b/errors/public.go @@ -0,0 +1,38 @@ +// Copyright 2020 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 errors + +import ( + "errors" + "fmt" +) + +// UPGRADEv2: delete other files in folder. +//nolint +var ( + ErrRunFailure = errors.New("cannot run check") + ErrRepoUnreachable = errors.New("repo unreachable") +) + +// Create a public error using any of the errors +// listed above. For examples, see errors/errors.md. +func Create(e error, msg string) error { + // Note: Errorf automatically wraps the error when used with `%w`. + if len(msg) > 0 { + return fmt.Errorf("%w: %v", e, msg) + } + // We still need to use %w to prevent callers from using e == ErrInvalidDockerFile. + return fmt.Errorf("%w", e) +} diff --git a/errors/types.go b/errors/types.go index 421324b3d15..414102cfbd2 100644 --- a/errors/types.go +++ b/errors/types.go @@ -19,8 +19,8 @@ import ( ) type ( - ErrRetry struct{ wrappedError } - ErrZeroConfidence struct{ wrappedError } + ErrRetry struct{ wrappedError } + ErrLowConfidence struct{ wrappedError } ) func MakeRetryError(err error) error { @@ -32,10 +32,10 @@ func MakeRetryError(err error) error { } } -func MakeZeroConfidenceError(err error) error { - return &ErrZeroConfidence{ +func MakeLowConfidenceError(err error) error { + return &ErrLowConfidence{ wrappedError{ - msg: "check result was unknown", + msg: "low confidence check result", innerError: err, }, } diff --git a/pkg/scorecard.go b/pkg/scorecard.go index d712532f1d1..926ceb25a76 100644 --- a/pkg/scorecard.go +++ b/pkg/scorecard.go @@ -14,6 +14,7 @@ package pkg +//nolint:gci import ( "context" "fmt" @@ -22,14 +23,13 @@ import ( "time" "github.com/google/go-github/v32/github" - "github.com/shurcooL/githubv4" - opencensusstats "go.opencensus.io/stats" - "go.opencensus.io/tag" - "github.com/ossf/scorecard/checker" "github.com/ossf/scorecard/clients" "github.com/ossf/scorecard/repos" "github.com/ossf/scorecard/stats" + "github.com/shurcooL/githubv4" + opencensusstats "go.opencensus.io/stats" + "go.opencensus.io/tag" ) func logStats(ctx context.Context, startTime time.Time) { diff --git a/pkg/scorecard_result.go b/pkg/scorecard_result.go index ef12a9a128b..75bba2af74e 100644 --- a/pkg/scorecard_result.go +++ b/pkg/scorecard_result.go @@ -14,6 +14,7 @@ package pkg +//nolint:gci import ( "encoding/csv" "encoding/json" @@ -25,8 +26,8 @@ import ( "strings" "github.com/olekukonko/tablewriter" - "github.com/ossf/scorecard/checker" + "go.uber.org/zap/zapcore" ) type ScorecardResult struct { @@ -38,7 +39,7 @@ type ScorecardResult struct { // AsJSON outputs the result in JSON format with a newline at the end. // If called on []ScorecardResult will create NDJson formatted output. -func (r *ScorecardResult) AsJSON(showDetails bool, writer io.Writer) error { +func (r *ScorecardResult) AsJSON(showDetails bool, logLevel zapcore.Level, writer io.Writer) error { encoder := json.NewEncoder(writer) if showDetails { if err := encoder.Encode(r); err != nil { @@ -51,6 +52,8 @@ func (r *ScorecardResult) AsJSON(showDetails bool, writer io.Writer) error { Date: r.Date, Metadata: r.Metadata, } + // UPGRADEv2: remove nolint after uggrade. + //nolint for _, checkResult := range r.Checks { tmpResult := checker.CheckResult{ Name: checkResult.Name, @@ -65,10 +68,12 @@ func (r *ScorecardResult) AsJSON(showDetails bool, writer io.Writer) error { return nil } -func (r *ScorecardResult) AsCSV(showDetails bool, writer io.Writer) error { +func (r *ScorecardResult) AsCSV(showDetails bool, logLevel zapcore.Level, writer io.Writer) error { w := csv.NewWriter(writer) record := []string{r.Repo} columns := []string{"Repository"} + // UPGRADEv2: remove nolint after uggrade. + //nolint for _, checkResult := range r.Checks { columns = append(columns, checkResult.Name+"_Pass", checkResult.Name+"_Confidence") record = append(record, strconv.FormatBool(checkResult.Pass), @@ -89,8 +94,10 @@ func (r *ScorecardResult) AsCSV(showDetails bool, writer io.Writer) error { return nil } -func (r *ScorecardResult) AsString(showDetails bool, writer io.Writer) error { +// UPGRADEv2: will be removed. +func (r *ScorecardResult) AsString(showDetails bool, logLevel zapcore.Level, writer io.Writer) error { sortedChecks := make([]checker.CheckResult, len(r.Checks)) + //nolint for i, checkResult := range r.Checks { sortedChecks[i] = checkResult } @@ -102,6 +109,7 @@ func (r *ScorecardResult) AsString(showDetails bool, writer io.Writer) error { }) data := make([][]string, len(sortedChecks)) + //nolint for i, row := range sortedChecks { const withdetails = 4 const withoutdetails = 3 @@ -117,7 +125,19 @@ func (r *ScorecardResult) AsString(showDetails bool, writer io.Writer) error { x[1] = strconv.Itoa(row.Confidence) x[2] = row.Name if showDetails { - x[3] = strings.Join(row.Details, "\n") + //nolint + if row.Version == 2 { + var sa []string + for _, v := range row.Details2 { + if v.Type == checker.DetailDebug && logLevel != zapcore.DebugLevel { + continue + } + sa = append(sa, fmt.Sprintf("%s: %s", typeToString(v.Type), v.Msg)) + } + x[3] = strings.Join(sa, "\n") + } else { + x[3] = strings.Join(row.Details, "\n") + } } data[i] = x } @@ -135,9 +155,105 @@ func (r *ScorecardResult) AsString(showDetails bool, writer io.Writer) error { table.SetCenterSeparator("|") table.AppendBulk(data) table.Render() + + return nil +} + +// UPGRADEv2: new code. +func (r *ScorecardResult) AsString2(showDetails bool, logLevel zapcore.Level, writer io.Writer) error { + sortedChecks := make([]checker.CheckResult, len(r.Checks)) + //nolint + // UPGRADEv2: not needed after upgrade. + for i, checkResult := range r.Checks { + sortedChecks[i] = checkResult + } + sort.Slice(sortedChecks, func(i, j int) bool { + if sortedChecks[i].Pass == sortedChecks[j].Pass { + return sortedChecks[i].Name < sortedChecks[j].Name + } + return sortedChecks[i].Pass + }) + + data := make([][]string, len(sortedChecks)) + //nolint + // UPGRADEv2: not needed after upgrade. + for i, row := range sortedChecks { + //nolint + if row.Version != 2 { + continue + } + const withdetails = 5 + const withoutdetails = 4 + var x []string + + if showDetails { + x = make([]string, withdetails) + } else { + x = make([]string, withoutdetails) + } + + // UPGRADEv2: rename variable. + if row.Score2 == checker.InconclusiveResultScore { + x[0] = "?" + } else { + x[0] = fmt.Sprintf("%d", row.Score2) + } + + doc := fmt.Sprintf("https://github.com/ossf/scorecard/blob/main/checks/checks.md#%s", strings.ToLower(row.Name)) + x[1] = row.Reason2 + x[2] = row.Name + if showDetails { + // UPGRADEv2: change to make([]string, len(row.Details)) + // followed by sa[i] = instead of append + var sa []string + for _, v := range row.Details2 { + if v.Type == checker.DetailDebug && logLevel != zapcore.DebugLevel { + continue + } + sa = append(sa, fmt.Sprintf("%s: %s", typeToString(v.Type), v.Msg)) + } + x[3] = strings.Join(sa, "\n") + x[4] = doc + } else { + x[3] = doc + } + + data[i] = x + } + + table := tablewriter.NewWriter(os.Stdout) + header := []string{"Score", "Reason", "Name"} + if showDetails { + header = append(header, "Details") + } + header = append(header, "Documentation/Remediation") + table.SetHeader(header) + table.SetBorders(tablewriter.Border{Left: true, Top: true, Right: true, Bottom: true}) + table.SetRowSeparator("-") + table.SetRowLine(true) + table.SetCenterSeparator("|") + table.AppendBulk(data) + table.SetAlignment(tablewriter.ALIGN_LEFT) + table.SetRowLine(true) + table.Render() + return nil } +func typeToString(cd checker.DetailType) string { + switch cd { + default: + panic("invalid detail") + case checker.DetailInfo: + return "Info" + case checker.DetailWarn: + return "Warn" + case checker.DetailDebug: + return "Debug" + } +} + +// UPGRADEv2: not needed after upgrade. func displayResult(result bool) string { if result { return "Pass" diff --git a/utests/utlib.go b/utests/utlib.go new file mode 100644 index 00000000000..56719bdd782 --- /dev/null +++ b/utests/utlib.go @@ -0,0 +1,101 @@ +// Copyright 2020 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 TestArgs struct { +// Filename string +// Dl TestDetailLogger +// } + +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.Score2 != 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.Score2, te.Score, dl.messages) + } + return false + } + return true +}