diff --git a/checker/check_result.go b/checker/check_result.go index edb98d0fe5d..00fc9172da1 100644 --- a/checker/check_result.go +++ b/checker/check_result.go @@ -16,6 +16,7 @@ package checker import ( + "errors" "fmt" "math" @@ -50,6 +51,10 @@ const ( DetailDebug ) +// errSuccessTotal indicates a runtime error because number of success cases should +// be smaller than the total cases to create a proportional score. +var errSuccessTotal = errors.New("unexpected number of success is higher than total") + // CheckResult captures result from a check run. // //nolint:govet @@ -88,6 +93,14 @@ type LogMessage struct { Remediation *rule.Remediation // Remediation information, if any. } +// ProportionalScoreWeighted is a structure that contains +// the fields to calculate weighted proportional scores. +type ProportionalScoreWeighted struct { + Success int + Total int + Weight int +} + // CreateProportionalScore creates a proportional score. func CreateProportionalScore(success, total int) int { if total == 0 { @@ -97,6 +110,40 @@ func CreateProportionalScore(success, total int) int { return int(math.Min(float64(MaxResultScore*success/total), float64(MaxResultScore))) } +// CreateProportionalScoreWeighted creates the proportional score +// between multiple successes over the total, but some proportions +// are worth more. +func CreateProportionalScoreWeighted(scores ...ProportionalScoreWeighted) (int, error) { + var ws, wt int + allWeightsZero := true + noScoreGroups := true + for _, score := range scores { + if score.Success > score.Total { + return InconclusiveResultScore, fmt.Errorf("%w: %d, %d", errSuccessTotal, score.Success, score.Total) + } + if score.Total == 0 { + continue // Group with 0 total, does not count for score + } + noScoreGroups = false + if score.Weight != 0 { + allWeightsZero = false + } + // Group with zero weight, adds nothing to the score + + ws += score.Success * score.Weight + wt += score.Total * score.Weight + } + if noScoreGroups { + return InconclusiveResultScore, nil + } + // If has score groups but no groups matter to the score, result in max score + if allWeightsZero { + return MaxResultScore, nil + } + + return int(math.Min(float64(MaxResultScore*ws/wt), float64(MaxResultScore))), nil +} + // AggregateScores adds up all scores // and normalizes the result. // Each score contributes equally. diff --git a/checker/check_result_test.go b/checker/check_result_test.go index 291f0696cde..1ec48850340 100644 --- a/checker/check_result_test.go +++ b/checker/check_result_test.go @@ -139,6 +139,273 @@ func TestCreateProportionalScore(t *testing.T) { } } +func TestCreateProportionalScoreWeighted(t *testing.T) { + t.Parallel() + type want struct { + score int + err bool + } + tests := []struct { + name string + scores []ProportionalScoreWeighted + want want + }{ + { + name: "max result with 1 group and normal weight", + scores: []ProportionalScoreWeighted{ + { + Success: 1, + Total: 1, + Weight: 10, + }, + }, + want: want{ + score: 10, + }, + }, + { + name: "min result with 1 group and normal weight", + scores: []ProportionalScoreWeighted{ + { + Success: 0, + Total: 1, + Weight: 10, + }, + }, + want: want{ + score: 0, + }, + }, + { + name: "partial result with 1 group and normal weight", + scores: []ProportionalScoreWeighted{ + { + Success: 2, + Total: 10, + Weight: 10, + }, + }, + want: want{ + score: 2, + }, + }, + { + name: "partial result with 2 groups and normal weights", + scores: []ProportionalScoreWeighted{ + { + Success: 2, + Total: 10, + Weight: 10, + }, + { + Success: 8, + Total: 10, + Weight: 10, + }, + }, + want: want{ + score: 5, + }, + }, + { + name: "partial result with 2 groups and odd weights", + scores: []ProportionalScoreWeighted{ + { + Success: 2, + Total: 10, + Weight: 8, + }, + { + Success: 8, + Total: 10, + Weight: 2, + }, + }, + want: want{ + score: 3, + }, + }, + { + name: "all groups with 0 weight, no groups matter for the score, results in max score", + scores: []ProportionalScoreWeighted{ + { + Success: 1, + Total: 1, + Weight: 0, + }, + { + Success: 1, + Total: 1, + Weight: 0, + }, + }, + want: want{ + score: 10, + }, + }, + { + name: "not all groups with 0 weight, only groups with weight matter to the score", + scores: []ProportionalScoreWeighted{ + { + Success: 1, + Total: 1, + Weight: 0, + }, + { + Success: 2, + Total: 10, + Weight: 8, + }, + { + Success: 8, + Total: 10, + Weight: 2, + }, + }, + want: want{ + score: 3, + }, + }, + { + name: "no total, results in inconclusive score", + scores: []ProportionalScoreWeighted{ + { + Success: 0, + Total: 0, + Weight: 10, + }, + }, + want: want{ + score: -1, + }, + }, + { + name: "some groups with 0 total, only groups with total matter to the score", + scores: []ProportionalScoreWeighted{ + { + Success: 0, + Total: 0, + Weight: 10, + }, + { + Success: 2, + Total: 10, + Weight: 10, + }, + }, + want: want{ + score: 2, + }, + }, + { + name: "any group with number of successes higher than total, results in inconclusive score and error", + scores: []ProportionalScoreWeighted{ + { + Success: 1, + Total: 0, + Weight: 10, + }, + }, + want: want{ + score: -1, + err: true, + }, + }, + { + name: "only groups with weight and total matter to the score", + scores: []ProportionalScoreWeighted{ + { + Success: 1, + Total: 1, + Weight: 0, + }, + { + Success: 0, + Total: 0, + Weight: 10, + }, + { + Success: 2, + Total: 10, + Weight: 8, + }, + { + Success: 8, + Total: 10, + Weight: 2, + }, + }, + want: want{ + score: 3, + }, + }, + { + name: "only groups with weight and total matter to the score but no groups have success, results in min score", + scores: []ProportionalScoreWeighted{ + { + Success: 1, + Total: 1, + Weight: 0, + }, + { + Success: 0, + Total: 0, + Weight: 10, + }, + { + Success: 0, + Total: 10, + Weight: 8, + }, + { + Success: 0, + Total: 10, + Weight: 2, + }, + }, + want: want{ + score: 0, + }, + }, + { + name: "group with 0 weight counts as max score and group with 0 total does not count", + scores: []ProportionalScoreWeighted{ + { + Success: 2, + Total: 8, + Weight: 0, + }, + { + Success: 0, + Total: 0, + Weight: 10, + }, + }, + want: want{ + score: 10, + }, + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got, err := CreateProportionalScoreWeighted(tt.scores...) + if err != nil && !tt.want.err { + t.Errorf("CreateProportionalScoreWeighted unexpected error '%v'", err) + t.Fail() + } + if err == nil && tt.want.err { + t.Errorf("CreateProportionalScoreWeighted expected error and got none") + t.Fail() + } + if got != tt.want.score { + t.Errorf("CreateProportionalScoreWeighted() = %v, want %v", got, tt.want.score) + } + }) + } +} + func TestNormalizeReason(t *testing.T) { t.Parallel() type args struct { diff --git a/checker/raw_result.go b/checker/raw_result.go index 2acaee17a47..e8834afe0e8 100644 --- a/checker/raw_result.go +++ b/checker/raw_result.go @@ -121,6 +121,7 @@ type Dependency struct { PinnedAt *string Location *File Msg *string // Only for debug messages. + Pinned *bool Type DependencyUseType } diff --git a/checks/evaluation/pinned_dependencies.go b/checks/evaluation/pinned_dependencies.go index c05038fa7ec..c0af9515214 100644 --- a/checks/evaluation/pinned_dependencies.go +++ b/checks/evaluation/pinned_dependencies.go @@ -15,26 +15,19 @@ package evaluation import ( - "errors" "fmt" "github.com/ossf/scorecard/v4/checker" "github.com/ossf/scorecard/v4/checks/fileparser" sce "github.com/ossf/scorecard/v4/errors" - "github.com/ossf/scorecard/v4/finding" "github.com/ossf/scorecard/v4/remediation" "github.com/ossf/scorecard/v4/rule" ) -var errInvalidValue = errors.New("invalid value") - -type pinnedResult int - -const ( - pinnedUndefined pinnedResult = iota - pinned - notPinned -) +type pinnedResult struct { + pinned int + total int +} // Structure to host information about pinned github // or third party dependencies. @@ -43,6 +36,20 @@ type worklowPinningResult struct { gitHubOwned pinnedResult } +// Weights used for proportional score. +// This defines the priority of pinning a dependency over other dependencies. +// The dependencies from all ecosystems are equally prioritized except +// for GitHub Actions. GitHub Actions can be GitHub-owned or from third-party +// development. The GitHub Actions ecosystem has equal priority compared to other +// ecosystems, but, within GitHub Actions, pinning third-party actions has more +// priority than pinning GitHub-owned actions. +// https://github.com/ossf/scorecard/issues/802 +const ( + gitHubOwnedActionWeight int = 2 + thirdPartyActionWeight int = 8 + normalWeight int = gitHubOwnedActionWeight + thirdPartyActionWeight +) + // PinningDependencies applies the score policy for the Pinned-Dependencies check. func PinningDependencies(name string, c *checker.CheckRequest, r *checker.PinningDependenciesData, @@ -70,7 +77,6 @@ func PinningDependencies(name string, c *checker.CheckRequest, }) continue } - if rr.Msg != nil { dl.Debug(&checker.LogMessage{ Path: rr.Location.Path, @@ -80,7 +86,20 @@ func PinningDependencies(name string, c *checker.CheckRequest, Text: *rr.Msg, Snippet: rr.Location.Snippet, }) - } else { + continue + } + if rr.Pinned == nil { + dl.Debug(&checker.LogMessage{ + Path: rr.Location.Path, + Type: rr.Location.Type, + Offset: rr.Location.Offset, + EndOffset: rr.Location.EndOffset, + Text: fmt.Sprintf("%s has empty Pinned field", rr.Type), + Snippet: rr.Location.Snippet, + }) + continue + } + if !*rr.Pinned { dl.Warn(&checker.LogMessage{ Path: rr.Location.Path, Type: rr.Location.Type, @@ -90,67 +109,37 @@ func PinningDependencies(name string, c *checker.CheckRequest, Snippet: rr.Location.Snippet, Remediation: generateRemediation(remediationMetadata, &rr), }) - - // Update the pinning status. - updatePinningResults(&rr, &wp, pr) } + // Update the pinning status. + updatePinningResults(&rr, &wp, pr) } // Generate scores and Info results. - // GitHub actions. - actionScore, err := createReturnForIsGitHubActionsWorkflowPinned(wp, dl) - if err != nil { - return checker.CreateRuntimeErrorResult(name, err) - } - - // Docker files. - dockerFromScore, err := createReturnForIsDockerfilePinned(pr, dl) - if err != nil { - return checker.CreateRuntimeErrorResult(name, err) - } - - // Docker downloads. - dockerDownloadScore, err := createReturnForIsDockerfileFreeOfInsecureDownloads(pr, dl) - if err != nil { - return checker.CreateRuntimeErrorResult(name, err) - } - - // Script downloads. - scriptScore, err := createReturnForIsShellScriptFreeOfInsecureDownloads(pr, dl) - if err != nil { - return checker.CreateRuntimeErrorResult(name, err) - } - - // Pip installs. - pipScore, err := createReturnForIsPipInstallPinned(pr, dl) - if err != nil { - return checker.CreateRuntimeErrorResult(name, err) + var scores []checker.ProportionalScoreWeighted + // Go through all dependency types + // GitHub Actions need to be handled separately since they are not in pr + scores = append(scores, createScoreForGitHubActionsWorkflow(&wp, dl)...) + // Only exisiting dependencies will be found in pr + // We will only score the ecosystem if there are dependencies + // This results in only existing ecosystems being included in the final score + for t := range pr { + logPinnedResult(dl, pr[t], string(t)) + scores = append(scores, checker.ProportionalScoreWeighted{ + Success: pr[t].pinned, + Total: pr[t].total, + Weight: normalWeight, + }) } - // Npm installs. - npmScore, err := createReturnForIsNpmInstallPinned(pr, dl) - if err != nil { - return checker.CreateRuntimeErrorResult(name, err) + if len(scores) == 0 { + return checker.CreateInconclusiveResult(name, "no dependencies found") } - // Go installs. - goScore, err := createReturnForIsGoInstallPinned(pr, dl) + score, err := checker.CreateProportionalScoreWeighted(scores...) if err != nil { return checker.CreateRuntimeErrorResult(name, err) } - // Scores may be inconclusive. - actionScore = maxScore(0, actionScore) - dockerFromScore = maxScore(0, dockerFromScore) - dockerDownloadScore = maxScore(0, dockerDownloadScore) - scriptScore = maxScore(0, scriptScore) - pipScore = maxScore(0, pipScore) - npmScore = maxScore(0, npmScore) - goScore = maxScore(0, goScore) - - score := checker.AggregateScores(actionScore, dockerFromScore, - dockerDownloadScore, scriptScore, pipScore, npmScore, goScore) - if score == checker.MaxResultScore { return checker.CreateMaxScoreResult(name, "all dependencies are pinned") } @@ -177,13 +166,13 @@ func updatePinningResults(rr *checker.Dependency, // Note: `Snippet` contains `action/name@xxx`, so we cna use it to infer // if it's a GitHub-owned action or not. gitHubOwned := fileparser.IsGitHubOwnedAction(rr.Location.Snippet) - addWorkflowPinnedResult(wp, false, gitHubOwned) + addWorkflowPinnedResult(rr, wp, gitHubOwned) return } // Update other result types. - var p pinnedResult - addPinnedResult(&p, false) + p := pr[rr.Type] + addPinnedResult(rr, &p) pr[rr.Type] = p } @@ -192,7 +181,7 @@ func generateText(rr *checker.Dependency) string { // Check if we are dealing with a GitHub action or a third-party one. gitHubOwned := fileparser.IsGitHubOwnedAction(rr.Location.Snippet) owner := generateOwnerToDisplay(gitHubOwned) - return fmt.Sprintf("%s %s not pinned by hash", owner, rr.Type) + return fmt.Sprintf("%s not pinned by hash", owner) } return fmt.Sprintf("%s not pinned by hash", rr.Type) @@ -200,149 +189,69 @@ func generateText(rr *checker.Dependency) string { func generateOwnerToDisplay(gitHubOwned bool) string { if gitHubOwned { - return "GitHub-owned" + return fmt.Sprintf("GitHub-owned %s", checker.DependencyUseTypeGHAction) } - return "third-party" + return fmt.Sprintf("third-party %s", checker.DependencyUseTypeGHAction) } -// TODO(laurent): need to support GCB pinning. -func maxScore(s1, s2 int) int { - if s1 > s2 { - return s1 +func addPinnedResult(rr *checker.Dependency, r *pinnedResult) { + if *rr.Pinned { + r.pinned += 1 } - return s2 + r.total += 1 } -// For the 'to' param, true means the file is pinning dependencies (or there are no dependencies), -// false means there are unpinned dependencies. -func addPinnedResult(r *pinnedResult, to bool) { - // If the result is `notPinned`, we keep it. - // In other cases, we always update the result. - if *r == notPinned { - return - } - - switch to { - case true: - *r = pinned - case false: - *r = notPinned - } -} - -func addWorkflowPinnedResult(w *worklowPinningResult, to, isGitHub bool) { +func addWorkflowPinnedResult(rr *checker.Dependency, w *worklowPinningResult, isGitHub bool) { if isGitHub { - addPinnedResult(&w.gitHubOwned, to) + addPinnedResult(rr, &w.gitHubOwned) } else { - addPinnedResult(&w.thirdParties, to) + addPinnedResult(rr, &w.thirdParties) } } -// Create the result for scripts. -func createReturnForIsShellScriptFreeOfInsecureDownloads(pr map[checker.DependencyUseType]pinnedResult, - dl checker.DetailLogger, -) (int, error) { - return createReturnValues(pr, checker.DependencyUseTypeDownloadThenRun, - "no insecure (not pinned by hash) dependency downloads found in shell scripts", - dl) -} - -// Create the result for docker containers. -func createReturnForIsDockerfilePinned(pr map[checker.DependencyUseType]pinnedResult, - dl checker.DetailLogger, -) (int, error) { - return createReturnValues(pr, checker.DependencyUseTypeDockerfileContainerImage, - "Dockerfile dependencies are pinned", - dl) -} - -// Create the result for docker commands. -func createReturnForIsDockerfileFreeOfInsecureDownloads(pr map[checker.DependencyUseType]pinnedResult, - dl checker.DetailLogger, -) (int, error) { - return createReturnValues(pr, checker.DependencyUseTypeDownloadThenRun, - "no insecure (not pinned by hash) dependency downloads found in Dockerfiles", - dl) -} - -// Create the result for pip install commands. -func createReturnForIsPipInstallPinned(pr map[checker.DependencyUseType]pinnedResult, - dl checker.DetailLogger, -) (int, error) { - return createReturnValues(pr, checker.DependencyUseTypePipCommand, - "Pip installs are pinned", - dl) +func logPinnedResult(dl checker.DetailLogger, p pinnedResult, name string) { + dl.Info(&checker.LogMessage{ + Text: fmt.Sprintf("%3d out of %3d %s dependencies pinned", p.pinned, p.total, name), + }) } -// Create the result for npm install commands. -func createReturnForIsNpmInstallPinned(pr map[checker.DependencyUseType]pinnedResult, - dl checker.DetailLogger, -) (int, error) { - return createReturnValues(pr, checker.DependencyUseTypeNpmCommand, - "npm installs are pinned", - dl) -} - -// Create the result for go install commands. -func createReturnForIsGoInstallPinned(pr map[checker.DependencyUseType]pinnedResult, - dl checker.DetailLogger, -) (int, error) { - return createReturnValues(pr, checker.DependencyUseTypeGoCommand, - "go installs are pinned", - dl) -} - -func createReturnValues(pr map[checker.DependencyUseType]pinnedResult, - t checker.DependencyUseType, infoMsg string, - dl checker.DetailLogger, -) (int, error) { - // Note: we don't check if the entry exists, - // as it will have the default value which is handled in the switch statement. - //nolint - r, _ := pr[t] - switch r { - default: - return checker.InconclusiveResultScore, fmt.Errorf("%w: %v", errInvalidValue, r) - case pinned, pinnedUndefined: - dl.Info(&checker.LogMessage{ - Text: infoMsg, - }) - return checker.MaxResultScore, nil - case notPinned: - // No logging needed as it's done by the checks. - return checker.MinResultScore, nil +func createScoreForGitHubActionsWorkflow(wp *worklowPinningResult, dl checker.DetailLogger, +) []checker.ProportionalScoreWeighted { + if wp.gitHubOwned.total == 0 && wp.thirdParties.total == 0 { + return []checker.ProportionalScoreWeighted{} } -} - -// Create the result. -func createReturnForIsGitHubActionsWorkflowPinned(wp worklowPinningResult, dl checker.DetailLogger) (int, error) { - return createReturnValuesForGitHubActionsWorkflowPinned(wp, - fmt.Sprintf("%ss are pinned", checker.DependencyUseTypeGHAction), - dl) -} - -func createReturnValuesForGitHubActionsWorkflowPinned(r worklowPinningResult, infoMsg string, - dl checker.DetailLogger, -) (int, error) { - score := checker.MinResultScore - - if r.gitHubOwned != notPinned { - score += 2 - dl.Info(&checker.LogMessage{ - Type: finding.FileTypeSource, - Offset: checker.OffsetDefault, - Text: fmt.Sprintf("%s %s", "GitHub-owned", infoMsg), - }) + if wp.gitHubOwned.total != 0 && wp.thirdParties.total != 0 { + logPinnedResult(dl, wp.gitHubOwned, generateOwnerToDisplay(true)) + logPinnedResult(dl, wp.thirdParties, generateOwnerToDisplay(false)) + return []checker.ProportionalScoreWeighted{ + { + Success: wp.gitHubOwned.pinned, + Total: wp.gitHubOwned.total, + Weight: gitHubOwnedActionWeight, + }, + { + Success: wp.thirdParties.pinned, + Total: wp.thirdParties.total, + Weight: thirdPartyActionWeight, + }, + } } - - if r.thirdParties != notPinned { - score += 8 - dl.Info(&checker.LogMessage{ - Type: finding.FileTypeSource, - Offset: checker.OffsetDefault, - Text: fmt.Sprintf("%s %s", "Third-party", infoMsg), - }) + if wp.gitHubOwned.total != 0 { + logPinnedResult(dl, wp.gitHubOwned, generateOwnerToDisplay(true)) + return []checker.ProportionalScoreWeighted{ + { + Success: wp.gitHubOwned.pinned, + Total: wp.gitHubOwned.total, + Weight: normalWeight, + }, + } + } + logPinnedResult(dl, wp.thirdParties, generateOwnerToDisplay(false)) + return []checker.ProportionalScoreWeighted{ + { + Success: wp.thirdParties.pinned, + Total: wp.thirdParties.total, + Weight: normalWeight, + }, } - - return score, nil } diff --git a/checks/evaluation/pinned_dependencies_test.go b/checks/evaluation/pinned_dependencies_test.go index 0a40c0da9a0..3efb32c3901 100644 --- a/checks/evaluation/pinned_dependencies_test.go +++ b/checks/evaluation/pinned_dependencies_test.go @@ -20,67 +20,208 @@ import ( "github.com/google/go-cmp/cmp" "github.com/ossf/scorecard/v4/checker" + sce "github.com/ossf/scorecard/v4/errors" scut "github.com/ossf/scorecard/v4/utests" ) -func Test_createReturnValuesForGitHubActionsWorkflowPinned(t *testing.T) { +func Test_createScoreForGitHubActionsWorkflow(t *testing.T) { t.Parallel() //nolint - type args struct { - r worklowPinningResult - infoMsg string - dl checker.DetailLogger - } - //nolint tests := []struct { - name string - args args - want int + name string + r worklowPinningResult + scores []checker.ProportionalScoreWeighted }{ { - name: "both actions workflow pinned", - args: args{ - r: worklowPinningResult{ - thirdParties: 1, - gitHubOwned: 1, + name: "GitHub-owned and Third-Party actions pinned", + r: worklowPinningResult{ + gitHubOwned: pinnedResult{ + pinned: 1, + total: 1, + }, + thirdParties: pinnedResult{ + pinned: 1, + total: 1, + }, + }, + scores: []checker.ProportionalScoreWeighted{ + { + Success: 1, + Total: 1, + Weight: 2, + }, + { + Success: 1, + Total: 1, + Weight: 8, }, - dl: &scut.TestDetailLogger{}, }, - want: 10, }, { - name: "github actions workflow pinned", - args: args{ - r: worklowPinningResult{ - thirdParties: 2, - gitHubOwned: 2, + name: "only GitHub-owned actions pinned", + r: worklowPinningResult{ + gitHubOwned: pinnedResult{ + pinned: 1, + total: 1, + }, + thirdParties: pinnedResult{ + pinned: 0, + total: 1, + }, + }, + scores: []checker.ProportionalScoreWeighted{ + { + Success: 1, + Total: 1, + Weight: 2, + }, + { + Success: 0, + Total: 1, + Weight: 8, }, - dl: &scut.TestDetailLogger{}, }, - want: 0, }, { - name: "error in github actions workflow pinned", - args: args{ - r: worklowPinningResult{ - thirdParties: 2, - gitHubOwned: 2, + name: "only Third-Party actions pinned", + r: worklowPinningResult{ + gitHubOwned: pinnedResult{ + pinned: 0, + total: 1, + }, + thirdParties: pinnedResult{ + pinned: 1, + total: 1, + }, + }, + scores: []checker.ProportionalScoreWeighted{ + { + Success: 0, + Total: 1, + Weight: 2, + }, + { + Success: 1, + Total: 1, + Weight: 8, + }, + }, + }, + { + name: "no GitHub actions pinned", + r: worklowPinningResult{ + gitHubOwned: pinnedResult{ + pinned: 0, + total: 1, + }, + thirdParties: pinnedResult{ + pinned: 0, + total: 1, + }, + }, + scores: []checker.ProportionalScoreWeighted{ + { + Success: 0, + Total: 1, + Weight: 2, + }, + { + Success: 0, + Total: 1, + Weight: 8, + }, + }, + }, + { + name: "no GitHub-owned actions and Third-party actions unpinned", + r: worklowPinningResult{ + gitHubOwned: pinnedResult{ + pinned: 0, + total: 0, + }, + thirdParties: pinnedResult{ + pinned: 0, + total: 1, + }, + }, + scores: []checker.ProportionalScoreWeighted{ + { + Success: 0, + Total: 1, + Weight: 10, + }, + }, + }, + { + name: "no Third-party actions and GitHub-owned actions unpinned", + r: worklowPinningResult{ + gitHubOwned: pinnedResult{ + pinned: 0, + total: 1, + }, + thirdParties: pinnedResult{ + pinned: 0, + total: 0, + }, + }, + scores: []checker.ProportionalScoreWeighted{ + { + Success: 0, + Total: 1, + Weight: 10, + }, + }, + }, + { + name: "no GitHub-owned actions and Third-party actions pinned", + r: worklowPinningResult{ + gitHubOwned: pinnedResult{ + pinned: 0, + total: 0, + }, + thirdParties: pinnedResult{ + pinned: 1, + total: 1, + }, + }, + scores: []checker.ProportionalScoreWeighted{ + { + Success: 1, + Total: 1, + Weight: 10, + }, + }, + }, + { + name: "no Third-party actions and GitHub-owned actions pinned", + r: worklowPinningResult{ + gitHubOwned: pinnedResult{ + pinned: 1, + total: 1, + }, + thirdParties: pinnedResult{ + pinned: 0, + total: 0, + }, + }, + scores: []checker.ProportionalScoreWeighted{ + { + Success: 1, + Total: 1, + Weight: 10, }, - dl: &scut.TestDetailLogger{}, }, - want: 0, }, } 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() - got, err := createReturnValuesForGitHubActionsWorkflowPinned(tt.args.r, tt.args.infoMsg, tt.args.dl) - if err != nil { - t.Errorf("error during createReturnValuesForGitHubActionsWorkflowPinned: %v", err) - } - if got != tt.want { - t.Errorf("createReturnValuesForGitHubActionsWorkflowPinned() = %v, want %v", got, tt.want) + dl := scut.TestDetailLogger{} + actual := createScoreForGitHubActionsWorkflow(&tt.r, &dl) + diff := cmp.Diff(tt.scores, actual) + if diff != "" { + t.Errorf("createScoreForGitHubActionsWorkflow (-want,+got) %+v", diff) } }) } @@ -90,6 +231,10 @@ func asPointer(s string) *string { return &s } +func asBoolPointer(b bool) *bool { + return &b +} + func Test_PinningDependencies(t *testing.T) { t.Parallel() @@ -99,140 +244,319 @@ func Test_PinningDependencies(t *testing.T) { expected scut.TestReturn }{ { - name: "download then run pinned debug", + name: "all dependencies pinned", dependencies: []checker.Dependency{ + { + Location: &checker.File{ + Snippet: "actions/checkout@a81bbbf8298c0fa03ea29cdc473d45769f953675", + }, + Type: checker.DependencyUseTypeGHAction, + Pinned: asBoolPointer(true), + }, + { + Location: &checker.File{ + Snippet: "other/checkout@a81bbbf8298c0fa03ea29cdc473d45769f953675", + }, + Type: checker.DependencyUseTypeGHAction, + Pinned: asBoolPointer(true), + }, + { + Location: &checker.File{}, + Type: checker.DependencyUseTypeDockerfileContainerImage, + Pinned: asBoolPointer(true), + }, { Location: &checker.File{}, - Msg: asPointer("some message"), Type: checker.DependencyUseTypeDownloadThenRun, + Pinned: asBoolPointer(true), + }, + { + Location: &checker.File{}, + Type: checker.DependencyUseTypeGoCommand, + Pinned: asBoolPointer(true), + }, + { + Location: &checker.File{}, + Type: checker.DependencyUseTypeNpmCommand, + Pinned: asBoolPointer(true), + }, + { + Location: &checker.File{}, + Type: checker.DependencyUseTypePipCommand, + Pinned: asBoolPointer(true), }, }, expected: scut.TestReturn{ Error: nil, - Score: checker.MaxResultScore, + Score: 10, NumberOfWarn: 0, - NumberOfInfo: 8, - NumberOfDebug: 1, + NumberOfInfo: 7, + NumberOfDebug: 0, }, }, { - name: "download then run pinned debug and warn", + name: "all dependencies unpinned", dependencies: []checker.Dependency{ + { + Location: &checker.File{ + Snippet: "actions/checkout@v2", + }, + Type: checker.DependencyUseTypeGHAction, + Pinned: asBoolPointer(false), + }, + { + Location: &checker.File{ + Snippet: "other/checkout@v2", + }, + Type: checker.DependencyUseTypeGHAction, + Pinned: asBoolPointer(false), + }, { Location: &checker.File{}, - Msg: asPointer("some message"), - Type: checker.DependencyUseTypeDownloadThenRun, + Type: checker.DependencyUseTypeDockerfileContainerImage, + Pinned: asBoolPointer(false), }, { Location: &checker.File{}, Type: checker.DependencyUseTypeDownloadThenRun, + Pinned: asBoolPointer(false), + }, + { + Location: &checker.File{}, + Type: checker.DependencyUseTypeGoCommand, + Pinned: asBoolPointer(false), + }, + { + Location: &checker.File{}, + Type: checker.DependencyUseTypeNpmCommand, + Pinned: asBoolPointer(false), + }, + { + Location: &checker.File{}, + Type: checker.DependencyUseTypePipCommand, + Pinned: asBoolPointer(false), }, }, expected: scut.TestReturn{ Error: nil, - Score: 7, - NumberOfWarn: 1, - NumberOfInfo: 6, - NumberOfDebug: 1, + Score: 0, + NumberOfWarn: 7, + NumberOfInfo: 7, + NumberOfDebug: 0, }, }, { - name: "various warnings", + name: "1 ecosystem pinned and 1 ecosystem unpinned", dependencies: []checker.Dependency{ { Location: &checker.File{}, Type: checker.DependencyUseTypePipCommand, + Pinned: asBoolPointer(false), }, { Location: &checker.File{}, - Type: checker.DependencyUseTypeDownloadThenRun, + Type: checker.DependencyUseTypeGoCommand, + Pinned: asBoolPointer(true), }, + }, + expected: scut.TestReturn{ + Error: nil, + Score: 5, + NumberOfWarn: 1, + NumberOfInfo: 2, + NumberOfDebug: 0, + }, + }, + { + name: "1 ecosystem partially pinned", + dependencies: []checker.Dependency{ { Location: &checker.File{}, - Type: checker.DependencyUseTypeDockerfileContainerImage, + Type: checker.DependencyUseTypePipCommand, + Pinned: asBoolPointer(false), }, { Location: &checker.File{}, - Msg: asPointer("debug message"), + Type: checker.DependencyUseTypePipCommand, + Pinned: asBoolPointer(true), }, }, expected: scut.TestReturn{ Error: nil, - Score: 4, - NumberOfWarn: 3, - NumberOfInfo: 4, - NumberOfDebug: 1, + Score: 5, + NumberOfWarn: 1, + NumberOfInfo: 1, + NumberOfDebug: 0, }, }, { - name: "unpinned pip install", + name: "no dependencies found", + dependencies: []checker.Dependency{}, + expected: scut.TestReturn{ + Error: nil, + Score: -1, + NumberOfWarn: 0, + NumberOfInfo: 0, + NumberOfDebug: 0, + }, + }, + { + name: "pinned dependency shows no warn message", dependencies: []checker.Dependency{ { Location: &checker.File{}, Type: checker.DependencyUseTypePipCommand, + Pinned: asBoolPointer(true), }, }, expected: scut.TestReturn{ Error: nil, - Score: 8, + Score: 10, + NumberOfWarn: 0, + NumberOfInfo: 1, + NumberOfDebug: 0, + }, + }, + { + name: "unpinned dependency shows warn message", + dependencies: []checker.Dependency{ + { + Location: &checker.File{}, + Type: checker.DependencyUseTypePipCommand, + Pinned: asBoolPointer(false), + }, + }, + expected: scut.TestReturn{ + Error: nil, + Score: 0, NumberOfWarn: 1, - NumberOfInfo: 7, + NumberOfInfo: 1, NumberOfDebug: 0, }, }, { - name: "undefined pip install", + name: "dependency with parsing error does not count for score and shows debug message", dependencies: []checker.Dependency{ { Location: &checker.File{}, + Msg: asPointer("some message"), Type: checker.DependencyUseTypePipCommand, - Msg: asPointer("debug message"), }, }, expected: scut.TestReturn{ Error: nil, - Score: 10, + Score: -1, NumberOfWarn: 0, - NumberOfInfo: 8, + NumberOfInfo: 0, NumberOfDebug: 1, }, }, { - name: "all dependencies pinned", + name: "dependency missing Pinned info does not count for score and shows debug message", + dependencies: []checker.Dependency{ + { + Location: &checker.File{}, + Type: checker.DependencyUseTypePipCommand, + }, + }, expected: scut.TestReturn{ Error: nil, - Score: 10, + Score: -1, + NumberOfWarn: 0, + NumberOfInfo: 0, + NumberOfDebug: 1, + }, + }, + { + name: "dependency missing Location info and no error message throws error", + dependencies: []checker.Dependency{{}}, + expected: scut.TestReturn{ + Error: sce.ErrScorecardInternal, + Score: -1, NumberOfWarn: 0, - NumberOfInfo: 8, + NumberOfInfo: 0, NumberOfDebug: 0, }, }, { - name: "Validate various warnings and info", + name: "dependency missing Location info with error message shows debug message", + dependencies: []checker.Dependency{{ + Msg: asPointer("some message"), + }}, + expected: scut.TestReturn{ + Error: nil, + Score: -1, + NumberOfWarn: 0, + NumberOfInfo: 0, + NumberOfDebug: 1, + }, + }, + { + name: "unpinned choco install", dependencies: []checker.Dependency{ { Location: &checker.File{}, - Type: checker.DependencyUseTypePipCommand, + Type: checker.DependencyUseTypeChocoCommand, + Pinned: asBoolPointer(false), }, + }, + expected: scut.TestReturn{ + Error: nil, + Score: 0, + NumberOfWarn: 1, + NumberOfInfo: 1, + NumberOfDebug: 0, + }, + }, + { + name: "unpinned Dockerfile container image", + dependencies: []checker.Dependency{ { Location: &checker.File{}, - Type: checker.DependencyUseTypeDownloadThenRun, + Type: checker.DependencyUseTypeDockerfileContainerImage, + Pinned: asBoolPointer(false), }, + }, + expected: scut.TestReturn{ + Error: nil, + Score: 0, + NumberOfWarn: 1, + NumberOfInfo: 1, + NumberOfDebug: 0, + }, + }, + { + name: "unpinned download then run", + dependencies: []checker.Dependency{ { Location: &checker.File{}, - Type: checker.DependencyUseTypeDockerfileContainerImage, + Type: checker.DependencyUseTypeDownloadThenRun, + Pinned: asBoolPointer(false), }, + }, + expected: scut.TestReturn{ + Error: nil, + Score: 0, + NumberOfWarn: 1, + NumberOfInfo: 1, + NumberOfDebug: 0, + }, + }, + { + name: "unpinned go install", + dependencies: []checker.Dependency{ { Location: &checker.File{}, - Msg: asPointer("debug message"), + Type: checker.DependencyUseTypeGoCommand, + Pinned: asBoolPointer(false), }, }, expected: scut.TestReturn{ Error: nil, - Score: 4, - NumberOfWarn: 3, - NumberOfInfo: 4, - NumberOfDebug: 1, + Score: 0, + NumberOfWarn: 1, + NumberOfInfo: 1, + NumberOfDebug: 0, }, }, { @@ -241,177 +565,253 @@ func Test_PinningDependencies(t *testing.T) { { Location: &checker.File{}, Type: checker.DependencyUseTypeNpmCommand, + Pinned: asBoolPointer(false), }, }, expected: scut.TestReturn{ Error: nil, - Score: 8, + Score: 0, NumberOfWarn: 1, - NumberOfInfo: 7, + NumberOfInfo: 1, NumberOfDebug: 0, }, }, { - name: "unpinned go install", + name: "unpinned nuget install", dependencies: []checker.Dependency{ { Location: &checker.File{}, - Type: checker.DependencyUseTypeGoCommand, + Type: checker.DependencyUseTypeNugetCommand, + Pinned: asBoolPointer(false), }, }, expected: scut.TestReturn{ Error: nil, - Score: 8, + Score: 0, NumberOfWarn: 1, - NumberOfInfo: 7, + NumberOfInfo: 1, NumberOfDebug: 0, }, }, - } - - for _, tt := range tests { - tt := tt - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - - dl := scut.TestDetailLogger{} - c := checker.CheckRequest{Dlogger: &dl} - actual := PinningDependencies("checkname", &c, - &checker.PinningDependenciesData{ - Dependencies: tt.dependencies, - }) - - if !scut.ValidateTestReturn(t, tt.name, &tt.expected, &actual, &dl) { - t.Fail() - } - }) - } -} - -func Test_createReturnValues(t *testing.T) { - t.Parallel() - - type args struct { - pr map[checker.DependencyUseType]pinnedResult - dl *scut.TestDetailLogger - t checker.DependencyUseType - } - - tests := []struct { - name string - args args - want int - }{ { - name: "returns 10 if no error and no pinnedResult", - args: args{ - t: checker.DependencyUseTypeDownloadThenRun, - dl: &scut.TestDetailLogger{}, + name: "unpinned pip install", + dependencies: []checker.Dependency{ + { + Location: &checker.File{}, + Type: checker.DependencyUseTypePipCommand, + Pinned: asBoolPointer(false), + }, + }, + expected: scut.TestReturn{ + Error: nil, + Score: 0, + NumberOfWarn: 1, + NumberOfInfo: 1, + NumberOfDebug: 0, }, - want: 10, }, { - name: "returns 10 if pinned undefined", - args: args{ - t: checker.DependencyUseTypeDownloadThenRun, - pr: map[checker.DependencyUseType]pinnedResult{ - checker.DependencyUseTypeDownloadThenRun: pinnedUndefined, + name: "2 unpinned dependencies for 1 ecosystem shows 2 warn messages", + dependencies: []checker.Dependency{ + { + Location: &checker.File{}, + Type: checker.DependencyUseTypePipCommand, + Pinned: asBoolPointer(false), }, - dl: &scut.TestDetailLogger{}, + { + Location: &checker.File{}, + Type: checker.DependencyUseTypePipCommand, + Pinned: asBoolPointer(false), + }, + }, + expected: scut.TestReturn{ + Error: nil, + Score: 0, + NumberOfWarn: 2, + NumberOfInfo: 1, + NumberOfDebug: 0, }, - want: 10, }, { - name: "returns 10 if pinned", - args: args{ - t: checker.DependencyUseTypeDownloadThenRun, - pr: map[checker.DependencyUseType]pinnedResult{ - checker.DependencyUseTypeDownloadThenRun: pinned, + name: "2 unpinned dependencies for 2 ecosystems shows 2 warn messages", + dependencies: []checker.Dependency{ + { + Location: &checker.File{}, + Type: checker.DependencyUseTypePipCommand, + Pinned: asBoolPointer(false), + }, + { + Location: &checker.File{}, + Type: checker.DependencyUseTypeGoCommand, + Pinned: asBoolPointer(false), }, - dl: &scut.TestDetailLogger{}, }, - want: 10, + expected: scut.TestReturn{ + Error: nil, + Score: 0, + NumberOfWarn: 2, + NumberOfInfo: 2, + NumberOfDebug: 0, + }, }, { - name: "returns 0 if unpinned", - args: args{ - t: checker.DependencyUseTypeDownloadThenRun, - pr: map[checker.DependencyUseType]pinnedResult{ - checker.DependencyUseTypeDownloadThenRun: notPinned, + name: "GitHub Actions ecosystem with GitHub-owned pinned", + dependencies: []checker.Dependency{ + { + Location: &checker.File{ + Snippet: "actions/checkout@a81bbbf8298c0fa03ea29cdc473d45769f953675", + }, + Type: checker.DependencyUseTypeGHAction, + Pinned: asBoolPointer(true), }, - dl: &scut.TestDetailLogger{}, }, - want: 0, + expected: scut.TestReturn{ + Error: nil, + Score: 10, + NumberOfWarn: 0, + NumberOfInfo: 1, + NumberOfDebug: 0, + }, }, - } - for _, tt := range tests { - tt := tt - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - got, err := createReturnValues(tt.args.pr, tt.args.t, "some message", tt.args.dl) - if err != nil { - t.Errorf("error during createReturnValues: %v", err) - } - if got != tt.want { - t.Errorf("createReturnValues() = %v, want %v", got, tt.want) - } - - if tt.want < 10 { - return - } - - isExpectedLog := func(logMessage checker.LogMessage, logType checker.DetailType) bool { - return logMessage.Text == "some message" && logType == checker.DetailInfo - } - if !scut.ValidateLogMessage(isExpectedLog, tt.args.dl) { - t.Errorf("test failed: log message not present: %+v", "some message") - } - }) - } -} - -func Test_maxScore(t *testing.T) { - t.Parallel() - type args struct { - s1 int - s2 int - } - tests := []struct { - name string - args args - want int - }{ { - name: "returns s1 if s1 is greater than s2", - args: args{ - s1: 10, - s2: 5, + name: "GitHub Actions ecosystem with third-party pinned", + dependencies: []checker.Dependency{ + { + Location: &checker.File{ + Snippet: "other/checkout@a81bbbf8298c0fa03ea29cdc473d45769f953675", + }, + Type: checker.DependencyUseTypeGHAction, + Pinned: asBoolPointer(true), + }, + }, + expected: scut.TestReturn{ + Error: nil, + Score: 10, + NumberOfWarn: 0, + NumberOfInfo: 1, + NumberOfDebug: 0, }, - want: 10, }, { - name: "returns s2 if s2 is greater than s1", - args: args{ - s1: 5, - s2: 10, + name: "GitHub Actions ecosystem with GitHub-owned and third-party pinned", + dependencies: []checker.Dependency{ + { + Location: &checker.File{ + Snippet: "actions/checkout@a81bbbf8298c0fa03ea29cdc473d45769f953675", + }, + Type: checker.DependencyUseTypeGHAction, + Pinned: asBoolPointer(true), + }, + { + Location: &checker.File{ + Snippet: "other/checkout@a81bbbf8298c0fa03ea29cdc473d45769f953675", + }, + Type: checker.DependencyUseTypeGHAction, + Pinned: asBoolPointer(true), + }, + }, + expected: scut.TestReturn{ + Error: nil, + Score: 10, + NumberOfWarn: 0, + NumberOfInfo: 2, + NumberOfDebug: 0, }, - want: 10, }, { - name: "returns s1 if s1 is equal to s2", - args: args{ - s1: 10, - s2: 10, + name: "GitHub Actions ecosystem with GitHub-owned and third-party unpinned", + dependencies: []checker.Dependency{ + { + Location: &checker.File{ + Snippet: "actions/checkout@v2", + }, + Type: checker.DependencyUseTypeGHAction, + Pinned: asBoolPointer(false), + }, + { + Location: &checker.File{ + Snippet: "other/checkout@v2", + }, + Type: checker.DependencyUseTypeGHAction, + Pinned: asBoolPointer(false), + }, + }, + expected: scut.TestReturn{ + Error: nil, + Score: 0, + NumberOfWarn: 2, + NumberOfInfo: 2, + NumberOfDebug: 0, + }, + }, + { + name: "GitHub Actions ecosystem with GitHub-owned pinned and third-party unpinned", + dependencies: []checker.Dependency{ + { + Location: &checker.File{ + Snippet: "actions/checkout@a81bbbf8298c0fa03ea29cdc473d45769f953675", + }, + Type: checker.DependencyUseTypeGHAction, + Pinned: asBoolPointer(true), + }, + { + Location: &checker.File{ + Snippet: "other/checkout@v2", + }, + Type: checker.DependencyUseTypeGHAction, + Pinned: asBoolPointer(false), + }, + }, + expected: scut.TestReturn{ + Error: nil, + Score: 2, + NumberOfWarn: 1, + NumberOfInfo: 2, + NumberOfDebug: 0, + }, + }, + { + name: "GitHub Actions ecosystem with GitHub-owned unpinned and third-party pinned", + dependencies: []checker.Dependency{ + { + Location: &checker.File{ + Snippet: "actions/checkout@v2", + }, + Type: checker.DependencyUseTypeGHAction, + Pinned: asBoolPointer(false), + }, + { + Location: &checker.File{ + Snippet: "other/checkout@a81bbbf8298c0fa03ea29cdc473d45769f953675", + }, + Type: checker.DependencyUseTypeGHAction, + Pinned: asBoolPointer(true), + }, + }, + expected: scut.TestReturn{ + Error: nil, + Score: 8, + NumberOfWarn: 1, + NumberOfInfo: 2, + NumberOfDebug: 0, }, - want: 10, }, } + for _, tt := range tests { tt := tt t.Run(tt.name, func(t *testing.T) { t.Parallel() - if got := maxScore(tt.args.s1, tt.args.s2); got != tt.want { - t.Errorf("maxScore() = %v, want %v", got, tt.want) + + dl := scut.TestDetailLogger{} + c := checker.CheckRequest{Dlogger: &dl} + actual := PinningDependencies("checkname", &c, + &checker.PinningDependenciesData{ + Dependencies: tt.dependencies, + }) + + if !scut.ValidateTestReturn(t, tt.name, &tt.expected, &actual, &dl) { + t.Fail() } }) } @@ -427,12 +827,12 @@ func Test_generateOwnerToDisplay(t *testing.T) { { name: "returns GitHub if gitHubOwned is true", gitHubOwned: true, - want: "GitHub-owned", + want: "GitHub-owned GitHubAction", }, { name: "returns GitHub if gitHubOwned is false", gitHubOwned: false, - want: "third-party", + want: "third-party GitHubAction", }, } for _, tt := range tests { @@ -449,50 +849,112 @@ func Test_generateOwnerToDisplay(t *testing.T) { func Test_addWorkflowPinnedResult(t *testing.T) { t.Parallel() type args struct { - w *worklowPinningResult - to bool - isGitHub bool + dependency *checker.Dependency + w *worklowPinningResult + isGitHub bool } tests := []struct { //nolint:govet name string - want pinnedResult + want *worklowPinningResult args args }{ { - name: "sets pinned to true if to is true", + name: "add pinned GitHub-owned action dependency", args: args{ + dependency: &checker.Dependency{ + Pinned: asBoolPointer(true), + }, w: &worklowPinningResult{}, - to: true, isGitHub: true, }, - want: pinned, + want: &worklowPinningResult{ + thirdParties: pinnedResult{ + pinned: 0, + total: 0, + }, + gitHubOwned: pinnedResult{ + pinned: 1, + total: 1, + }, + }, }, { - name: "sets pinned to false if to is false", + name: "add unpinned GitHub-owned action dependency", args: args{ + dependency: &checker.Dependency{ + Pinned: asBoolPointer(false), + }, w: &worklowPinningResult{}, - to: false, isGitHub: true, }, - want: notPinned, + want: &worklowPinningResult{ + thirdParties: pinnedResult{ + pinned: 0, + total: 0, + }, + gitHubOwned: pinnedResult{ + pinned: 0, + total: 1, + }, + }, + }, + { + name: "add pinned Third-Party action dependency", + args: args{ + dependency: &checker.Dependency{ + Pinned: asBoolPointer(true), + }, + w: &worklowPinningResult{}, + isGitHub: false, + }, + want: &worklowPinningResult{ + thirdParties: pinnedResult{ + pinned: 1, + total: 1, + }, + gitHubOwned: pinnedResult{ + pinned: 0, + total: 0, + }, + }, }, { - name: "sets pinned to undefined if to is false and isGitHub is false", + name: "add unpinned Third-Party action dependency", args: args{ + dependency: &checker.Dependency{ + Pinned: asBoolPointer(false), + }, w: &worklowPinningResult{}, - to: false, isGitHub: false, }, - want: pinnedUndefined, + want: &worklowPinningResult{ + thirdParties: pinnedResult{ + pinned: 0, + total: 1, + }, + gitHubOwned: pinnedResult{ + pinned: 0, + total: 0, + }, + }, }, } for _, tt := range tests { tt := tt t.Run(tt.name, func(t *testing.T) { t.Parallel() - addWorkflowPinnedResult(tt.args.w, tt.args.to, tt.args.isGitHub) - if tt.args.w.gitHubOwned != tt.want { - t.Errorf("addWorkflowPinnedResult() = %v, want %v", tt.args.w.gitHubOwned, tt.want) + addWorkflowPinnedResult(tt.args.dependency, tt.args.w, tt.args.isGitHub) + if tt.want.thirdParties != tt.args.w.thirdParties { + t.Errorf("addWorkflowPinnedResult Third-party GitHub actions mismatch (-want +got):"+ + "\nThird-party pinned: %s\nThird-party total: %s", + cmp.Diff(tt.want.thirdParties.pinned, tt.args.w.thirdParties.pinned), + cmp.Diff(tt.want.thirdParties.total, tt.args.w.thirdParties.total)) + } + if tt.want.gitHubOwned != tt.args.w.gitHubOwned { + t.Errorf("addWorkflowPinnedResult GitHub-owned GitHub actions mismatch (-want +got):"+ + "\nGitHub-owned pinned: %s\nGitHub-owned total: %s", + cmp.Diff(tt.want.gitHubOwned.pinned, tt.args.w.gitHubOwned.pinned), + cmp.Diff(tt.want.gitHubOwned.total, tt.args.w.gitHubOwned.total)) } }) } @@ -538,50 +1000,193 @@ func TestGenerateText(t *testing.T) { func TestUpdatePinningResults(t *testing.T) { t.Parallel() + type args struct { + dependency *checker.Dependency + w *worklowPinningResult + pr map[checker.DependencyUseType]pinnedResult + } + type want struct { + w *worklowPinningResult + pr map[checker.DependencyUseType]pinnedResult + } tests := []struct { //nolint:govet - name string - dependency *checker.Dependency - expectedPinningResult *worklowPinningResult - expectedPinnedResult map[checker.DependencyUseType]pinnedResult + name string + args args + want want }{ { - name: "GitHub Action - GitHub-owned", - dependency: &checker.Dependency{ - Type: checker.DependencyUseTypeGHAction, - Location: &checker.File{ - Snippet: "actions/checkout@v2", + name: "add pinned GitHub-owned action", + args: args{ + dependency: &checker.Dependency{ + Type: checker.DependencyUseTypeGHAction, + Location: &checker.File{ + Snippet: "actions/checkout@a81bbbf8298c0fa03ea29cdc473d45769f953675", + }, + Pinned: asBoolPointer(true), }, + w: &worklowPinningResult{}, + pr: make(map[checker.DependencyUseType]pinnedResult), }, - expectedPinningResult: &worklowPinningResult{ - thirdParties: 0, - gitHubOwned: 2, + want: want{ + w: &worklowPinningResult{ + thirdParties: pinnedResult{ + pinned: 0, + total: 0, + }, + gitHubOwned: pinnedResult{ + pinned: 1, + total: 1, + }, + }, + pr: make(map[checker.DependencyUseType]pinnedResult), }, - expectedPinnedResult: map[checker.DependencyUseType]pinnedResult{}, }, { - name: "Third party owned.", - dependency: &checker.Dependency{ - Type: checker.DependencyUseTypeGHAction, - Location: &checker.File{ - Snippet: "other/checkout@v2", + name: "add unpinned GitHub-owned action", + args: args{ + dependency: &checker.Dependency{ + Type: checker.DependencyUseTypeGHAction, + Location: &checker.File{ + Snippet: "actions/checkout@v2", + }, + Pinned: asBoolPointer(false), }, + w: &worklowPinningResult{}, + pr: make(map[checker.DependencyUseType]pinnedResult), }, - expectedPinningResult: &worklowPinningResult{ - thirdParties: 2, - gitHubOwned: 0, + want: want{ + w: &worklowPinningResult{ + thirdParties: pinnedResult{ + pinned: 0, + total: 0, + }, + gitHubOwned: pinnedResult{ + pinned: 0, + total: 1, + }, + }, + pr: make(map[checker.DependencyUseType]pinnedResult), + }, + }, + { + name: "add pinned Third-party action", + args: args{ + dependency: &checker.Dependency{ + Type: checker.DependencyUseTypeGHAction, + Location: &checker.File{ + Snippet: "other/checkout@ffa6706ff2127a749973072756f83c532e43ed02", + }, + Pinned: asBoolPointer(true), + }, + w: &worklowPinningResult{}, + pr: make(map[checker.DependencyUseType]pinnedResult), + }, + want: want{ + w: &worklowPinningResult{ + thirdParties: pinnedResult{ + pinned: 1, + total: 1, + }, + gitHubOwned: pinnedResult{ + pinned: 0, + total: 0, + }, + }, + pr: make(map[checker.DependencyUseType]pinnedResult), + }, + }, + { + name: "add unpinned Third-party action", + args: args{ + dependency: &checker.Dependency{ + Type: checker.DependencyUseTypeGHAction, + Location: &checker.File{ + Snippet: "other/checkout@v2", + }, + Pinned: asBoolPointer(false), + }, + w: &worklowPinningResult{}, + pr: make(map[checker.DependencyUseType]pinnedResult), + }, + want: want{ + w: &worklowPinningResult{ + thirdParties: pinnedResult{ + pinned: 0, + total: 1, + }, + gitHubOwned: pinnedResult{ + pinned: 0, + total: 0, + }, + }, + pr: make(map[checker.DependencyUseType]pinnedResult), + }, + }, + { + name: "add pinned pip install", + args: args{ + dependency: &checker.Dependency{ + Type: checker.DependencyUseTypePipCommand, + Pinned: asBoolPointer(true), + }, + w: &worklowPinningResult{}, + pr: make(map[checker.DependencyUseType]pinnedResult), + }, + want: want{ + w: &worklowPinningResult{}, + pr: map[checker.DependencyUseType]pinnedResult{ + checker.DependencyUseTypePipCommand: { + pinned: 1, + total: 1, + }, + }, + }, + }, + { + name: "add unpinned pip install", + args: args{ + dependency: &checker.Dependency{ + Type: checker.DependencyUseTypePipCommand, + Pinned: asBoolPointer(false), + }, + w: &worklowPinningResult{}, + pr: make(map[checker.DependencyUseType]pinnedResult), + }, + want: want{ + w: &worklowPinningResult{}, + pr: map[checker.DependencyUseType]pinnedResult{ + checker.DependencyUseTypePipCommand: { + pinned: 0, + total: 1, + }, + }, }, - expectedPinnedResult: map[checker.DependencyUseType]pinnedResult{}, }, } for _, tc := range tests { tc := tc t.Run(tc.name, func(t *testing.T) { t.Parallel() - wp := &worklowPinningResult{} - pr := make(map[checker.DependencyUseType]pinnedResult) - updatePinningResults(tc.dependency, wp, pr) - if tc.expectedPinningResult.thirdParties != wp.thirdParties && tc.expectedPinningResult.gitHubOwned != wp.gitHubOwned { //nolint:lll - t.Errorf("updatePinningResults mismatch (-want +got):\n%s", cmp.Diff(tc.expectedPinningResult, wp)) + updatePinningResults(tc.args.dependency, tc.args.w, tc.args.pr) + if tc.want.w.thirdParties != tc.args.w.thirdParties { + t.Errorf("updatePinningResults Third-party GitHub actions mismatch (-want +got):"+ + "\nThird-party pinned: %s\nThird-party total: %s", + cmp.Diff(tc.want.w.thirdParties.pinned, tc.args.w.thirdParties.pinned), + cmp.Diff(tc.want.w.thirdParties.total, tc.args.w.thirdParties.total)) + } + if tc.want.w.gitHubOwned != tc.args.w.gitHubOwned { + t.Errorf("updatePinningResults GitHub-owned GitHub actions mismatch (-want +got):"+ + "\nGitHub-owned pinned: %s\nGitHub-owned total: %s", + cmp.Diff(tc.want.w.gitHubOwned.pinned, tc.args.w.gitHubOwned.pinned), + cmp.Diff(tc.want.w.gitHubOwned.total, tc.args.w.gitHubOwned.total)) + } + for dependencyUseType := range tc.want.pr { + if tc.want.pr[dependencyUseType] != tc.args.pr[dependencyUseType] { + t.Errorf("updatePinningResults %s mismatch (-want +got):\npinned: %s\ntotal: %s", + dependencyUseType, + cmp.Diff(tc.want.pr[dependencyUseType].pinned, tc.args.pr[dependencyUseType].pinned), + cmp.Diff(tc.want.pr[dependencyUseType].total, tc.args.pr[dependencyUseType].total)) + } } }) } diff --git a/checks/raw/dependency_update_tool.go b/checks/raw/dependency_update_tool.go index 3af013b4088..2e2244c1b8b 100644 --- a/checks/raw/dependency_update_tool.go +++ b/checks/raw/dependency_update_tool.go @@ -136,3 +136,7 @@ var checkDependencyFileExists fileparser.DoWhileTrueOnFilename = func(name strin func asPointer(s string) *string { return &s } + +func asBoolPointer(b bool) *bool { + return &b +} diff --git a/checks/raw/pinned_dependencies.go b/checks/raw/pinned_dependencies.go index 0b93181443e..90b28bdff8a 100644 --- a/checks/raw/pinned_dependencies.go +++ b/checks/raw/pinned_dependencies.go @@ -261,7 +261,6 @@ var validateDockerfilesPinning fileparser.DoWhileTrueOnFileContent = func( if pinned || regex.MatchString(name) { // Record the asName. pinnedAsNames[asName] = true - continue } pdata.Dependencies = append(pdata.Dependencies, @@ -275,6 +274,7 @@ var validateDockerfilesPinning fileparser.DoWhileTrueOnFileContent = func( }, Name: asPointer(name), PinnedAt: asPointer(asName), + Pinned: asBoolPointer(pinnedAsNames[asName]), Type: checker.DependencyUseTypeDockerfileContainerImage, }, ) @@ -283,27 +283,26 @@ var validateDockerfilesPinning fileparser.DoWhileTrueOnFileContent = func( case len(valueList) == 1: name := valueList[0] pinned := pinnedAsNames[name] - if !pinned && !regex.MatchString(name) { - dep := checker.Dependency{ - Location: &checker.File{ - Path: pathfn, - Type: finding.FileTypeSource, - Offset: uint(child.StartLine), - EndOffset: uint(child.EndLine), - Snippet: child.Original, - }, - Type: checker.DependencyUseTypeDockerfileContainerImage, - } - parts := strings.SplitN(name, ":", 2) - if len(parts) > 0 { - dep.Name = asPointer(parts[0]) - if len(parts) > 1 { - dep.PinnedAt = asPointer(parts[1]) - } + + dep := checker.Dependency{ + Location: &checker.File{ + Path: pathfn, + Type: finding.FileTypeSource, + Offset: uint(child.StartLine), + EndOffset: uint(child.EndLine), + Snippet: child.Original, + }, + Pinned: asBoolPointer(pinned || regex.MatchString(name)), + Type: checker.DependencyUseTypeDockerfileContainerImage, + } + parts := strings.SplitN(name, ":", 2) + if len(parts) > 0 { + dep.Name = asPointer(parts[0]) + if len(parts) > 1 { + dep.PinnedAt = asPointer(parts[1]) } - pdata.Dependencies = append(pdata.Dependencies, dep) } - + pdata.Dependencies = append(pdata.Dependencies, dep) default: // That should not happen. return false, sce.WithMessage(sce.ErrScorecardInternal, errInternalInvalidDockerFile.Error()) @@ -470,26 +469,25 @@ var validateGitHubActionWorkflow fileparser.DoWhileTrueOnFileContent = func( continue } - if !isActionDependencyPinned(execAction.Uses.Value) { - dep := checker.Dependency{ - Location: &checker.File{ - Path: pathfn, - Type: finding.FileTypeSource, - Offset: uint(execAction.Uses.Pos.Line), - EndOffset: uint(execAction.Uses.Pos.Line), // `Uses` always span a single line. - Snippet: execAction.Uses.Value, - }, - Type: checker.DependencyUseTypeGHAction, - } - parts := strings.SplitN(execAction.Uses.Value, "@", 2) - if len(parts) > 0 { - dep.Name = asPointer(parts[0]) - if len(parts) > 1 { - dep.PinnedAt = asPointer(parts[1]) - } + dep := checker.Dependency{ + Location: &checker.File{ + Path: pathfn, + Type: finding.FileTypeSource, + Offset: uint(execAction.Uses.Pos.Line), + EndOffset: uint(execAction.Uses.Pos.Line), // `Uses` always span a single line. + Snippet: execAction.Uses.Value, + }, + Pinned: asBoolPointer(isActionDependencyPinned(execAction.Uses.Value)), + Type: checker.DependencyUseTypeGHAction, + } + parts := strings.SplitN(execAction.Uses.Value, "@", 2) + if len(parts) > 0 { + dep.Name = asPointer(parts[0]) + if len(parts) > 1 { + dep.PinnedAt = asPointer(parts[1]) } - pdata.Dependencies = append(pdata.Dependencies, dep) } + pdata.Dependencies = append(pdata.Dependencies, dep) } } diff --git a/checks/raw/pinned_dependencies_test.go b/checks/raw/pinned_dependencies_test.go index d1bf323d6c9..bd0e804ff31 100644 --- a/checks/raw/pinned_dependencies_test.go +++ b/checks/raw/pinned_dependencies_test.go @@ -92,8 +92,10 @@ func TestGithubWorkflowPinning(t *testing.T) { return } - if tt.warns != len(r.Dependencies) { - t.Errorf("expected %v. Got %v", tt.warns, len(r.Dependencies)) + unpinned := countUnpinned(r.Dependencies) + + if tt.warns != unpinned { + t.Errorf("expected %v. Got %v", tt.warns, unpinned) } }) } @@ -241,8 +243,10 @@ func TestNonGithubWorkflowPinning(t *testing.T) { return } - if tt.warns != len(r.Dependencies) { - t.Errorf("expected %v. Got %v", tt.warns, len(r.Dependencies)) + unpinned := countUnpinned(r.Dependencies) + + if tt.warns != unpinned { + t.Errorf("expected %v. Got %v", tt.warns, unpinned) } }) } @@ -288,8 +292,10 @@ func TestGithubWorkflowPkgManagerPinning(t *testing.T) { return } - if tt.warns != len(r.Dependencies) { - t.Errorf("expected %v. Got %v", tt.warns, len(r.Dependencies)) + unpinned := countUnpinned(r.Dependencies) + + if tt.warns != unpinned { + t.Errorf("expected %v. Got %v", tt.warns, unpinned) } }) } @@ -365,8 +371,10 @@ func TestDockerfilePinning(t *testing.T) { return } - if tt.warns != len(r.Dependencies) { - t.Errorf("expected %v. Got %v", tt.warns, len(r.Dependencies)) + unpinned := countUnpinned(r.Dependencies) + + if tt.warns != unpinned { + t.Errorf("expected %v. Got %v", tt.warns, unpinned) } }) } @@ -919,8 +927,10 @@ func TestDockerfilePinningWihoutHash(t *testing.T) { return } - if tt.warns != len(r.Dependencies) { - t.Errorf("expected %v. Got %v", tt.warns, len(r.Dependencies)) + unpinned := countUnpinned(r.Dependencies) + + if tt.warns != unpinned { + t.Errorf("expected %v. Got %v", tt.warns, unpinned) } }) } @@ -1022,8 +1032,10 @@ func TestDockerfileScriptDownload(t *testing.T) { return } - if tt.warns != len(r.Dependencies) { - t.Errorf("expected %v. Got %v", tt.warns, len(r.Dependencies)) + unpinned := countUnpinned(r.Dependencies) + + if tt.warns != unpinned { + t.Errorf("expected %v. Got %v", tt.warns, unpinned) } }) } @@ -1064,8 +1076,10 @@ func TestDockerfileScriptDownloadInfo(t *testing.T) { return } - if tt.warns != len(r.Dependencies) { - t.Errorf("expected %v. Got %v", tt.warns, len(r.Dependencies)) + unpinned := countUnpinned(r.Dependencies) + + if tt.warns != unpinned { + t.Errorf("expected %v. Got %v", tt.warns, unpinned) } }) } @@ -1140,12 +1154,14 @@ func TestShellScriptDownload(t *testing.T) { return } + unpinned := countUnpinned(r.Dependencies) + // Note: this works because all our examples // either have warns or debugs. - ws := (tt.warns == len(r.Dependencies)) && (tt.debugs == 0) - ds := (tt.debugs == len(r.Dependencies)) && (tt.warns == 0) + ws := (tt.warns == unpinned) && (tt.debugs == 0) + ds := (tt.debugs == unpinned) && (tt.warns == 0) if !ws && !ds { - t.Errorf("expected %v or %v. Got %v", tt.warns, tt.debugs, len(r.Dependencies)) + t.Errorf("expected %v or %v. Got %v", tt.warns, tt.debugs, unpinned) } }) } @@ -1192,8 +1208,10 @@ func TestShellScriptDownloadPinned(t *testing.T) { return } - if tt.warns != len(r.Dependencies) { - t.Errorf("expected %v. Got %v", tt.warns, len(r.Dependencies)) + unpinned := countUnpinned(r.Dependencies) + + if tt.warns != unpinned { + t.Errorf("expected %v. Got %v", tt.warns, unpinned) } }) } @@ -1251,8 +1269,10 @@ func TestGitHubWorflowRunDownload(t *testing.T) { return } - if tt.warns != len(r.Dependencies) { - t.Errorf("expected %v. Got %v", tt.warns, len(r.Dependencies)) + unpinned := countUnpinned(r.Dependencies) + + if tt.warns != unpinned { + t.Errorf("expected %v. Got %v", tt.warns, unpinned) } }) } @@ -1409,3 +1429,15 @@ func TestGitHubWorkInsecureDownloadsLineNumber(t *testing.T) { }) } } + +func countUnpinned(r []checker.Dependency) int { + var unpinned int + + for _, dependency := range r { + if *dependency.Pinned == false { + unpinned += 1 + } + } + + return unpinned +} diff --git a/checks/raw/shell_download_validate.go b/checks/raw/shell_download_validate.go index 524e6d6e57e..fe7c258c2d5 100644 --- a/checks/raw/shell_download_validate.go +++ b/checks/raw/shell_download_validate.go @@ -337,7 +337,8 @@ func collectFetchPipeExecute(startLine, endLine uint, node syntax.Node, cmd, pat EndOffset: endLine, Snippet: cmd, }, - Type: checker.DependencyUseTypeDownloadThenRun, + Pinned: asBoolPointer(false), + Type: checker.DependencyUseTypeDownloadThenRun, }, ) } @@ -388,7 +389,8 @@ func collectExecuteFiles(startLine, endLine uint, node syntax.Node, cmd, pathfn EndOffset: endLine, Snippet: cmd, }, - Type: checker.DependencyUseTypeDownloadThenRun, + Pinned: asBoolPointer(false), + Type: checker.DependencyUseTypeDownloadThenRun, }, ) } @@ -397,56 +399,50 @@ func collectExecuteFiles(startLine, endLine uint, node syntax.Node, cmd, pathfn // Npm install docs are here. // https://docs.npmjs.com/cli/v7/commands/npm-install -func isNpmUnpinnedDownload(cmd []string) bool { - if len(cmd) == 0 { - return false - } - +func isNpmDownload(cmd []string) bool { if !isBinaryName("npm", cmd[0]) { return false } for i := 1; i < len(cmd); i++ { // Search for get/install/update commands. - // `npm ci` wil verify all hashes are present. if strings.EqualFold(cmd[i], "install") || strings.EqualFold(cmd[i], "i") || strings.EqualFold(cmd[i], "install-test") || - strings.EqualFold(cmd[i], "update") { + strings.EqualFold(cmd[i], "update") || + strings.EqualFold(cmd[i], "ci") { return true } } return false } -func isGoUnpinnedDownload(cmd []string) bool { - if len(cmd) == 0 { - return false +func isNpmUnpinnedDownload(cmd []string) bool { + for i := 1; i < len(cmd); i++ { + // `npm ci` wil verify all hashes are present. + if strings.EqualFold(cmd[i], "ci") { + return false + } } + return true +} - if !isBinaryName("go", cmd[0]) { - return false - } +func isGoDownload(cmd []string) bool { // `Go install` will automatically look up the // go.mod and go.sum, so we don't flag it. if len(cmd) <= 2 { return false } - found := false + return isBinaryName("go", cmd[0]) && slices.Contains([]string{"get", "install"}, cmd[1]) +} + +func isGoUnpinnedDownload(cmd []string) bool { insecure := false hashRegex := regexp.MustCompile("^[A-Fa-f0-9]{40,}$") semverRegex := regexp.MustCompile(`^v\d+\.\d+\.\d+(-[0-9A-Za-z-.]+)?(\+[0-9A-Za-z-.]+)?$`) - for i := 1; i < len(cmd)-1; i++ { - // Search for get and install commands. - if slices.Contains([]string{"get", "install"}, cmd[i]) { - found = true - } - - if !found { - continue - } + for i := 1; i < len(cmd)-1; i++ { // Skip all flags // TODO skip other build flags which might take arguments for i < len(cmd)-1 && slices.Contains([]string{"-d", "-f", "-t", "-u", "-v", "-fix", "-insecure"}, cmd[i+1]) { @@ -485,7 +481,15 @@ func isGoUnpinnedDownload(cmd []string) bool { } } - return found + return true +} + +func isPipInstall(cmd []string) bool { + if len(cmd) < 2 { + return false + } + + return (isBinaryName("pip", cmd[0]) || isBinaryName("pip3", cmd[0])) && strings.EqualFold(cmd[1], "install") } func isPinnedEditableSource(pkgSource string) bool { @@ -509,28 +513,13 @@ func isFlag(cmd string) bool { } func isUnpinnedPipInstall(cmd []string) bool { - if !isBinaryName("pip", cmd[0]) && !isBinaryName("pip3", cmd[0]) { - return false - } - - isInstall := false hasNoDeps := false isEditableInstall := false isPinnedEditableInstall := true hasRequireHashes := false hasAdditionalArgs := false hasWheel := false - for i := 1; i < len(cmd); i++ { - // Search for install commands. - if strings.EqualFold(cmd[i], "install") { - isInstall = true - continue - } - - if !isInstall { - break - } - + for i := 2; i < len(cmd); i++ { // Require --no-deps to not install the dependencies when doing editable install // because we can't verify if dependencies are pinned // https://pip.pypa.io/en/stable/topics/secure-installs/#do-not-use-setuptools-directly @@ -609,7 +598,7 @@ func isUnpinnedPipInstall(cmd []string) bool { // Any other form of install is unpinned, // e.g. `pip install`. - return isInstall + return true } func isPythonCommand(cmd []string) bool { @@ -637,49 +626,52 @@ func extractPipCommand(cmd []string) ([]string, bool) { return nil, false } -func isUnpinnedPythonPipInstall(cmd []string) bool { +func isPythonPipInstall(cmd []string) bool { if !isPythonCommand(cmd) { return false } + pipCommand, ok := extractPipCommand(cmd) if !ok { return false } + + return isPipInstall(pipCommand) +} + +func isUnpinnedPythonPipInstall(cmd []string) bool { + pipCommand, _ := extractPipCommand(cmd) return isUnpinnedPipInstall(pipCommand) } -func isPipUnpinnedDownload(cmd []string) bool { - if len(cmd) == 0 { - return false - } +func isPipDownload(cmd []string) bool { + return isPipInstall(cmd) || isPythonPipInstall(cmd) +} - if isUnpinnedPipInstall(cmd) { +func isPipUnpinnedDownload(cmd []string) bool { + if isPipInstall(cmd) && isUnpinnedPipInstall(cmd) { return true } - if isUnpinnedPythonPipInstall(cmd) { + if isPythonPipInstall(cmd) && isUnpinnedPythonPipInstall(cmd) { return true } return false } -func isChocoUnpinnedDownload(cmd []string) bool { +func isChocoDownload(cmd []string) bool { // Install command is in the form 'choco install ...' if len(cmd) < 2 { return false } - if !isBinaryName("choco", cmd[0]) && !isBinaryName("choco.exe", cmd[0]) { - return false - } - - if !strings.EqualFold(cmd[1], "install") { - return false - } + return (isBinaryName("choco", cmd[0]) || isBinaryName("choco.exe", cmd[0])) && strings.EqualFold(cmd[1], "install") +} +func isChocoUnpinnedDownload(cmd []string) bool { // If this is an install command, then some variant of requirechecksum must be present. - for i := 1; i < len(cmd); i++ { + for i := 2; i < len(cmd); i++ { parts := strings.Split(cmd[i], "=") if len(parts) == 0 { continue @@ -697,22 +689,17 @@ func isChocoUnpinnedDownload(cmd []string) bool { return true } -func isUnpinnedNugetCliInstall(cmd []string) bool { +func isNugetCliInstall(cmd []string) bool { // looking for command of type nuget install ... if len(cmd) < 2 { return false } - // Search for nuget commands. - if !isBinaryName("nuget", cmd[0]) && !isBinaryName("nuget.exe", cmd[0]) { - return false - } - - // Search for install commands. - if !strings.EqualFold(cmd[1], "install") { - return false - } + // Search for nuget install commands. + return (isBinaryName("nuget", cmd[0]) || isBinaryName("nuget.exe", cmd[0])) && strings.EqualFold(cmd[1], "install") +} +func isUnpinnedNugetCliInstall(cmd []string) bool { // Assume installing a project with PackageReference (with versions) // or packages.config at the root of command if len(cmd) == 2 { @@ -740,26 +727,19 @@ func isUnpinnedNugetCliInstall(cmd []string) bool { return unpinnedDependency } -func isUnpinnedDotNetCliInstall(cmd []string) bool { +func isDotNetCliInstall(cmd []string) bool { // Search for command of type dotnet add package if len(cmd) < 4 { return false } - // Search for dotnet commands. - if !isBinaryName("dotnet", cmd[0]) && !isBinaryName("dotnet.exe", cmd[0]) { - return false - } - - // Search for add commands. - if !strings.EqualFold(cmd[1], "add") { - return false - } - - // Search for package commands (can be either the second or the third word) - if !(strings.EqualFold(cmd[2], "package") || strings.EqualFold(cmd[3], "package")) { - return false - } + // Search for dotnet add package + // where package command can be either the second or the third word + return (isBinaryName("dotnet", cmd[0]) || isBinaryName("dotnet.exe", cmd[0])) && + strings.EqualFold(cmd[1], "add") && + (strings.EqualFold(cmd[2], "package") || strings.EqualFold(cmd[3], "package")) +} +func isUnpinnedDotNetCliInstall(cmd []string) bool { unpinnedDependency := true for i := 3; i < len(cmd); i++ { // look for version flag @@ -772,12 +752,16 @@ func isUnpinnedDotNetCliInstall(cmd []string) bool { return unpinnedDependency } +func isNugetDownload(cmd []string) bool { + return isDotNetCliInstall(cmd) || isNugetCliInstall(cmd) +} + func isNugetUnpinnedDownload(cmd []string) bool { - if isUnpinnedDotNetCliInstall(cmd) { + if isDotNetCliInstall(cmd) && isUnpinnedDotNetCliInstall(cmd) { return true } - if isUnpinnedNugetCliInstall(cmd) { + if isNugetCliInstall(cmd) && isUnpinnedNugetCliInstall(cmd) { return true } @@ -799,8 +783,12 @@ func collectUnpinnedPakageManagerDownload(startLine, endLine uint, node syntax.N startLine, endLine = getLine(startLine, endLine, node) + if len(c) == 0 { + return + } + // Go get/install. - if isGoUnpinnedDownload(c) { + if isGoDownload(c) { r.Dependencies = append(r.Dependencies, checker.Dependency{ Location: &checker.File{ @@ -810,7 +798,8 @@ func collectUnpinnedPakageManagerDownload(startLine, endLine uint, node syntax.N EndOffset: endLine, Snippet: cmd, }, - Type: checker.DependencyUseTypeGoCommand, + Pinned: asBoolPointer(!isGoUnpinnedDownload(c)), + Type: checker.DependencyUseTypeGoCommand, }, ) @@ -818,7 +807,7 @@ func collectUnpinnedPakageManagerDownload(startLine, endLine uint, node syntax.N } // Pip install. - if isPipUnpinnedDownload(c) { + if isPipDownload(c) { r.Dependencies = append(r.Dependencies, checker.Dependency{ Location: &checker.File{ @@ -828,7 +817,8 @@ func collectUnpinnedPakageManagerDownload(startLine, endLine uint, node syntax.N EndOffset: endLine, Snippet: cmd, }, - Type: checker.DependencyUseTypePipCommand, + Pinned: asBoolPointer(!isPipUnpinnedDownload(c)), + Type: checker.DependencyUseTypePipCommand, }, ) @@ -836,7 +826,7 @@ func collectUnpinnedPakageManagerDownload(startLine, endLine uint, node syntax.N } // Npm install. - if isNpmUnpinnedDownload(c) { + if isNpmDownload(c) { r.Dependencies = append(r.Dependencies, checker.Dependency{ Location: &checker.File{ @@ -846,7 +836,8 @@ func collectUnpinnedPakageManagerDownload(startLine, endLine uint, node syntax.N EndOffset: endLine, Snippet: cmd, }, - Type: checker.DependencyUseTypeNpmCommand, + Pinned: asBoolPointer(!isNpmUnpinnedDownload(c)), + Type: checker.DependencyUseTypeNpmCommand, }, ) @@ -854,7 +845,7 @@ func collectUnpinnedPakageManagerDownload(startLine, endLine uint, node syntax.N } // Choco install. - if isChocoUnpinnedDownload(c) { + if isChocoDownload(c) { r.Dependencies = append(r.Dependencies, checker.Dependency{ Location: &checker.File{ @@ -864,7 +855,8 @@ func collectUnpinnedPakageManagerDownload(startLine, endLine uint, node syntax.N EndOffset: endLine, Snippet: cmd, }, - Type: checker.DependencyUseTypeChocoCommand, + Pinned: asBoolPointer(!isChocoUnpinnedDownload(c)), + Type: checker.DependencyUseTypeChocoCommand, }, ) @@ -872,7 +864,7 @@ func collectUnpinnedPakageManagerDownload(startLine, endLine uint, node syntax.N } // Nuget install. - if isNugetUnpinnedDownload(c) { + if isNugetDownload(c) { r.Dependencies = append(r.Dependencies, checker.Dependency{ Location: &checker.File{ @@ -882,7 +874,8 @@ func collectUnpinnedPakageManagerDownload(startLine, endLine uint, node syntax.N EndOffset: endLine, Snippet: cmd, }, - Type: checker.DependencyUseTypeNugetCommand, + Pinned: asBoolPointer(!isNugetUnpinnedDownload(c)), + Type: checker.DependencyUseTypeNugetCommand, }, ) @@ -977,7 +970,8 @@ func collectFetchProcSubsExecute(startLine, endLine uint, node syntax.Node, cmd, EndOffset: endLine, Snippet: cmd, }, - Type: checker.DependencyUseTypeDownloadThenRun, + Pinned: asBoolPointer(false), + Type: checker.DependencyUseTypeDownloadThenRun, }, ) } diff --git a/checks/raw/shell_download_validate_test.go b/checks/raw/shell_download_validate_test.go index 4624353e29c..efc22927367 100644 --- a/checks/raw/shell_download_validate_test.go +++ b/checks/raw/shell_download_validate_test.go @@ -242,3 +242,69 @@ func Test_isGoUnpinnedDownload(t *testing.T) { }) } } + +func Test_isNpmDownload(t *testing.T) { + type args struct { + cmd []string + } + tests := []struct { + name string + args args + want bool + }{ + { + name: "npm install", + args: args{ + cmd: []string{"npm", "install"}, + }, + want: true, + }, + { + name: "npm ci", + args: args{ + cmd: []string{"npm", "ci"}, + }, + want: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := isNpmDownload(tt.args.cmd); got != tt.want { + t.Errorf("isNpmDownload() = %v, want %v", got, tt.want) + } + }) + } +} + +func Test_isNpmUnpinnedDownload(t *testing.T) { + type args struct { + cmd []string + } + tests := []struct { + name string + args args + want bool + }{ + { + name: "npm install", + args: args{ + cmd: []string{"npm", "install"}, + }, + want: true, + }, + { + name: "npm ci", + args: args{ + cmd: []string{"npm", "ci"}, + }, + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := isNpmUnpinnedDownload(tt.args.cmd); got != tt.want { + t.Errorf("isNpmUnpinnedDownload() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/e2e/pinned_dependencies_test.go b/e2e/pinned_dependencies_test.go index c49357d337e..924a43a1ce1 100644 --- a/e2e/pinned_dependencies_test.go +++ b/e2e/pinned_dependencies_test.go @@ -49,9 +49,9 @@ var _ = Describe("E2E TEST:"+checks.CheckPinnedDependencies, func() { } expected := scut.TestReturn{ Error: nil, - Score: 4, + Score: 2, NumberOfWarn: 139, - NumberOfInfo: 3, + NumberOfInfo: 5, NumberOfDebug: 0, } result := checks.PinningDependencies(&req) @@ -74,9 +74,9 @@ var _ = Describe("E2E TEST:"+checks.CheckPinnedDependencies, func() { } expected := scut.TestReturn{ Error: nil, - Score: 4, + Score: 2, NumberOfWarn: 139, - NumberOfInfo: 3, + NumberOfInfo: 5, NumberOfDebug: 0, } result := checks.PinningDependencies(&req) @@ -110,9 +110,9 @@ var _ = Describe("E2E TEST:"+checks.CheckPinnedDependencies, func() { } expected := scut.TestReturn{ Error: nil, - Score: 4, + Score: 2, NumberOfWarn: 139, - NumberOfInfo: 3, + NumberOfInfo: 5, NumberOfDebug: 0, } result := checks.PinningDependencies(&req)