From 09e7030e027080759f54064fa255868cc21e34e4 Mon Sep 17 00:00:00 2001 From: Gabriela Gutierrez Date: Mon, 25 Sep 2023 17:07:19 -0300 Subject: [PATCH] :bug: Pinned-Dependencies: only score detected ecosystems (#3436) * feat: Define if dependency is pinned or unpinned Add a field Pinned to Dependency structure. Update to save Dependencies pinned and unpinned. Not only unpinned ones. All download then run executions are considered unpinned. Because there is no remediation to pin them. For package manager downloads: add early return if there are no commands, separate package manager identification (go, npm, choco, pip) from decision if installation is pinned or unpinned. Change Go case "go get -d -v" considered pinned, to any Go installations containing "-d" to be considered pinned. Signed-off-by: Gabriela Gutierrez * refactor: Convert diff var types to pointer We need to add a new conversion of boolean to pointer. Currently, we had string and int conversions named asPointer but not used in the same file. In order to know when we are using which conversion and considering bool and string would have to be used in the same file, it was needed to differentiate the method names. New method names are asIntPointer, asStringPointer and soon asBoolPointer. Signed-off-by: Gabriela Gutierrez * fix: Pinned Dependency field type Field needs to be a pointer to work when accessing values on evaluation. Signed-off-by: Gabriela Gutierrez * feat: Count pinned and unpinned deps We're changing the ecossystems result structure. The result structure previously stored if the ecossystem is fully pinned or not. The new result structure can tell how many dependencies of that ecossystem were found and how many were pinned. This change is necessary to ignore not applicable ecossystems on the final aggregated score. When iterating the dependencies, now we go through pinned and unpinned dependencies, not only unpinned, and in each iteration we update the result. We kept the behavior of only log warnings for unpinned dependencies. Signed-off-by: Gabriela Gutierrez * feat: Flag not applicable ecossystems If no dependencies of an ecossystem are found, it results in an inconclusive score (-1). As in other checks, this means here that the ecossystem scoring is not applicable in this case. At the same time, we are keep the scoring criteria the same. If all dependencies are pinned, it results in maximum score (10) and if 1 or more dependencies are unpinned, it results in a minimum score (0) for that ecossystem. GitHub workflow cases are handled differently but the idea is the same. We are also adding a log to know when an ecossystem was not found. Signed-off-by: Gabriela Gutierrez * feat: Score only applicable ecossystems Signed-off-by: Gabriela Gutierrez * feat: If no dependencies then create inconclusive score Signed-off-by: Gabriela Gutierrez * test: GitHub Actions score and logs Change test from `createReturnValuesForGitHubActionsWorkflowPinned` function to `createReturnForIsGitHubActionsWorkflowPinned` wrapper function so we can test logs. We have adjusted the existing test cases and included new test cases. Signed-off-by: Gabriela Gutierrez * test: Pinned dependencies score Break "various warnings" tests into smaller tests for pinned and unpinned dependencies and how they react to warn and debug messages. Plus add tests for how the score is affected when all dependencies are pinned, when no dependencies are pinned, when there are no dependencies, and partial dependencies pinned. Also, how dependencies unpinned in 1 or multiple ecossystems affect the warn messages, add one unpinned case for each ecossystem to see if they are being detected and separate the download then run 2 possible cases, there are currently scoring and logging wrong due to a bug. Signed-off-by: Gabriela Gutierrez * test: Ecossystems score and logs Signed-off-by: Gabriela Gutierrez * test: Remove deleted maxScore function test When we changed the scoring method to ignore not applicable scores, we removed the normalization of inconclusive scores to 0. The normalization was done by `maxScore` function, that was deleted in the process. Signed-off-by: Gabriela Gutierrez * test: Adding GitHub Actions dependencies to result Signed-off-by: Gabriela Gutierrez * test: Update GitHub Actions result Signed-off-by: Gabriela Gutierrez * test: Update pip installs result Signed-off-by: Gabriela Gutierrez * fix: Handle if nuget dependency is pinned or unpinned Signed-off-by: Gabriela Gutierrez * tests: Fix check warnings for unpinned dependencies Signed-off-by: Gabriela Gutierrez * fix: Linter errors Signed-off-by: Gabriela Gutierrez * fix: GitHub Actions pinned log If, for example, you have GitHub-owned actions and none Third-party actions, you should receive a "no Third-party actions found" log and don't receive a "all Third-party actions are pinned" log. At the same time, you deserve the score of pinning Third-party to complement the GitHub-owned score. Signed-off-by: Gabriela Gutierrez * test: Fix "ossf-tests/scorecard-check-pinned-dependencies-e2e" The repo being tested, `ossf-tests/scorecard-check-pinned-dependencies-e2e`, has no Third-party actions only GitHub-owned actions, that are unpinned, no npm installs, multiple go installs all pinned, and all other dependencies types are unpinned. This gives us 8 for actionScore, -1 for npm score, 10 for goScore, and 0 for all other scores. Previously the total score was 28/7 =~ 4, and now the total score is 18/6 =~ 3. The number of logs remain the same. The "all Third-party actions are pinned" will be replaced by "no Third-party actions found", which is a more realistic info and same thing for npm installs. Signed-off-by: Gabriela Gutierrez * Revert rename `asPointer` to `asStringPointer` Signed-off-by: Gabriela Gutierrez * fix: Handle deps with parsing error and undefined pinning When a dependency has a parsing error it ends up with a `Msg` field. In this case, the dependency should not count in the final score, so we should not `updatePinningResults` in this case. Also, to continue with the evaluation calculation, we need to make sure the dependencies have a `Pinned` state. Here we are adding this validation for it along with a debug log. Signed-off-by: Gabriela Gutierrez * test: Delete unecessary test We already have separate test for if 1 unpinned dependency shows a warn message, and 2 cases for when dependencies have errors and show a debug message. Signed-off-by: Gabriela Gutierrez * test: Add missing dep Location cases Signed-off-by: Gabriela Gutierrez * fix: Simplify Dockerfile pinned as name logic Signed-off-by: Gabriela Gutierrez * fix: If ecossystem is not found show debug log If ecossystem is not found show debug log, not info log. This affects the tests, all not found ecossystems will "move" from info logs to debug logs. We are also complementing the `all dependencies pinned` and `all dependencies unpinned` cases so we have the max score case and the min score case using all kinds of dependencies. Signed-off-by: Gabriela Gutierrez * test: Fix e2e tests and more unit tests Signed-off-by: Gabriela Gutierrez * feat: Iterate all dependency types for final score Now we iterate all existing dependency types in the final score. This will fix the problem of new ecossystems not being count in the final score because we needed to update the evaluation part. This also fixes the problem of download then run being counted twice for the score. Now, we only have debug logs when there are errors with the dependency metadata. That means we don't log anymore when dependencies of an ecossystem are not found. We changed the info log format when dependencies are all pinned. We simplified the calculation of the scores. We removed unused error returns. And now we only iterate existing ecossystems. If an ecossystem is not found we will not iterate it. Signed-off-by: Gabriela Gutierrez * feat: Proportional score We count all pinned dependencies over the total found dependencies of all ecossystems for the final score. But, we still want to give low prioritity to GHA GitHub-owned dependencies over GHA third-party dependencies. That's why we are doing a weighted proportional score, all ecossystems have a normal weight of 10 but GHAs have a weight. If you only have GitHub-owned, it will count as 10, because GHA don't weight less then other ecossystems. Same for GHA third-party, if you only have GHA third-party, it will also count as 10, because GHAs don't weight less then other ecossystems. But if you have both GHA GitHub-owned and third-party, GitHub-owned count less then third-party. Trying to keep the same weight as before, GitHub-owned weights 8 and third-party weights 2. These weights will make the score be more penalized if you have unpinned third-party and less penalized if you have unpinned GitHub-owned. Signed-off-by: Gabriela Gutierrez * fix: GHA weights in proportional score Signed-off-by: Gabriela Gutierrez * test: Fix scores and logs checking Add new cases for GHA scores since it's weighted differently now. Remove `createReturnValues` test since the function was removed. Fix current tests to adjust number of logs since we don't log if all dependencies are pinned or not anymore. Fix partially pinned score. Signed-off-by: Gabriela Gutierrez * test: Fix e2e test The repo being tested, `ossf-tests/scorecard-check-pinned-dependencies-e2e`, has no Third-party actions only GitHub-owned actions, that are unpinned, no npm installs, multiple go installs all pinned, and all other dependencies types are unpinned. This gives us 8 for GHA ecossytem, -1 for npm score, 10 for goScore, and 0 for all other scores. Previously the total score was 18/6 =~ 3. Now, we count 5/6 GitHub-owned GHA pinned, 23/36 containerImage pinned, 0/88 downloadThenRun pinned, 2/49 pipCommand pinned, 17/17 goCommand pinned. This results in 47/186 pinned dependencies which results in 2.5 score, that is rounded down to 2. Plus, the number of info was reduced since we don't log info for "all pinned dependencies in X ecossystem" anymore. Signed-off-by: Gabriela Gutierrez * refactor: Rename to ProportionalScoreWeighted Signed-off-by: Gabriela Gutierrez * refactor: Var declarations to create proportional score Signed-off-by: Gabriela Gutierrez * fix: Remove unnecessary pointer Signed-off-by: Gabriela Gutierrez * fix: Dependencies priority declaration Signed-off-by: Gabriela Gutierrez * fix: Ecosystem spelling Signed-off-by: Gabriela Gutierrez * fix: Handle 0 weight and 0 total when creating proportional weighted score Signed-off-by: Gabriela Gutierrez * fix: Revert -d flag identification change Signed-off-by: Gabriela Gutierrez * fix: npm ci command is npm download and is pinned Signed-off-by: Gabriela Gutierrez * fix: Linter errors Signed-off-by: Gabriela Gutierrez * fix: Unexport error variable to other packages Signed-off-by: Gabriela Gutierrez * refactor: Simplify no score groups condition Signed-off-by: Gabriela Gutierrez * feat: Log proportion of dependencies pinned Signed-off-by: Gabriela Gutierrez * test: Fix unit tests to include info logs The number of info logs should be same number of identified ecossystems. GitHub-owned GitHubAction and third-party GitHubAction count as different ecossytems. Signed-off-by: Gabriela Gutierrez * test: Fix e2e tests to include info logs The repo being tested, `ossf-tests/scorecard-check-pinned-dependencies-e2e`, has GitHub-owned GitHubActions, containerImage, downloadThenRun, pipCommand and goCommand dependencies. Therefore it will have 5 Info logs, one for each ecossystem. Signed-off-by: Gabriela Gutierrez * fix: Linter error Signed-off-by: Gabriela Gutierrez --------- Signed-off-by: Gabriela Gutierrez Signed-off-by: Allen Shearin --- checker/check_result.go | 47 + checker/check_result_test.go | 267 ++++ checker/raw_result.go | 1 + checks/evaluation/pinned_dependencies.go | 299 ++--- checks/evaluation/pinned_dependencies_test.go | 1091 +++++++++++++---- checks/raw/dependency_update_tool.go | 4 + checks/raw/pinned_dependencies.go | 74 +- checks/raw/pinned_dependencies_test.go | 74 +- checks/raw/shell_download_validate.go | 188 ++- checks/raw/shell_download_validate_test.go | 66 + e2e/pinned_dependencies_test.go | 12 +- 11 files changed, 1523 insertions(+), 600 deletions(-) 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)