From 2b2dd82638fba24bd5d5a12a033ba1a1a7919eae Mon Sep 17 00:00:00 2001 From: Gabriela Gutierrez Date: Tue, 14 Feb 2023 17:40:23 +0000 Subject: [PATCH 01/46] 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 --- checker/raw_result.go | 1 + checks/raw/pinned_dependencies.go | 74 +++++++------- checks/raw/shell_download_validate.go | 140 ++++++++++++-------------- 3 files changed, 103 insertions(+), 112 deletions(-) diff --git a/checker/raw_result.go b/checker/raw_result.go index 2acaee17a47..3589a4ee631 100644 --- a/checker/raw_result.go +++ b/checker/raw_result.go @@ -122,6 +122,7 @@ type Dependency struct { Location *File Msg *string // Only for debug messages. Type DependencyUseType + Pinned bool } // MaintainedData contains the raw results diff --git a/checks/raw/pinned_dependencies.go b/checks/raw/pinned_dependencies.go index 0b93181443e..3723d8c0e5e 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: pinned || regex.MatchString(name), 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: 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: 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/shell_download_validate.go b/checks/raw/shell_download_validate.go index 524e6d6e57e..599b8d59ce5 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: 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: false, + Type: checker.DependencyUseTypeDownloadThenRun, }, ) } @@ -397,11 +399,7 @@ 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 } @@ -419,50 +417,39 @@ func isNpmUnpinnedDownload(cmd []string) bool { return false } -func isGoUnpinnedDownload(cmd []string) bool { - if len(cmd) == 0 { - return false - } - - 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 := 2; i < len(cmd); i++ { + // The -d flag instructs get to download but not install the packages + if strings.EqualFold(cmd[i], "-d") { + return false } // 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]) { + if slices.Contains([]string{"-f", "-t", "-u", "-v", "-fix", "-insecure"}, cmd[i]) { // Record the flag -insecure - if cmd[i+1] == "-insecure" { + if cmd[i] == "-insecure" { insecure = true } - i++ + continue } - if i+1 >= len(cmd) { - // this is case go get -d -v - return false - } // TODO check more than one package - pkg := cmd[i+1] + pkg := cmd[i] // Consider strings that are not URLs as local folders // which are pinned. regex := regexp.MustCompile(`\w+\.\w+/\w+`) @@ -485,7 +472,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 +504,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 +589,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 +617,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 @@ -799,8 +782,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 +797,8 @@ func collectUnpinnedPakageManagerDownload(startLine, endLine uint, node syntax.N EndOffset: endLine, Snippet: cmd, }, - Type: checker.DependencyUseTypeGoCommand, + Pinned: !isGoUnpinnedDownload(c), + Type: checker.DependencyUseTypeGoCommand, }, ) @@ -818,7 +806,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 +816,8 @@ func collectUnpinnedPakageManagerDownload(startLine, endLine uint, node syntax.N EndOffset: endLine, Snippet: cmd, }, - Type: checker.DependencyUseTypePipCommand, + Pinned: !isPipUnpinnedDownload(c), + Type: checker.DependencyUseTypePipCommand, }, ) @@ -836,7 +825,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 +835,8 @@ func collectUnpinnedPakageManagerDownload(startLine, endLine uint, node syntax.N EndOffset: endLine, Snippet: cmd, }, - Type: checker.DependencyUseTypeNpmCommand, + Pinned: false, + Type: checker.DependencyUseTypeNpmCommand, }, ) @@ -854,7 +844,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 +854,8 @@ func collectUnpinnedPakageManagerDownload(startLine, endLine uint, node syntax.N EndOffset: endLine, Snippet: cmd, }, - Type: checker.DependencyUseTypeChocoCommand, + Pinned: !isChocoUnpinnedDownload(c), + Type: checker.DependencyUseTypeChocoCommand, }, ) @@ -977,7 +968,8 @@ func collectFetchProcSubsExecute(startLine, endLine uint, node syntax.Node, cmd, EndOffset: endLine, Snippet: cmd, }, - Type: checker.DependencyUseTypeDownloadThenRun, + Pinned: false, + Type: checker.DependencyUseTypeDownloadThenRun, }, ) } From 9638a92c3c0eb9b292b0753c2b1b33b407c79927 Mon Sep 17 00:00:00 2001 From: Gabriela Gutierrez Date: Thu, 16 Feb 2023 10:33:50 +0000 Subject: [PATCH 02/46] 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 --- attestor/policy/attestation_policy_test.go | 10 ++++----- checks/evaluation/pinned_dependencies_test.go | 8 +++---- checks/raw/dependency_update_tool.go | 22 +++++++++---------- checks/raw/fuzzing.go | 16 +++++++------- checks/raw/pinned_dependencies.go | 14 ++++++------ cron/worker/worker_test.go | 4 ++-- dependencydiff/dependencydiff_test.go | 14 ++++++------ dependencydiff/mapping.go | 6 ++--- pkg/json_raw_results.go | 4 ++-- 9 files changed, 49 insertions(+), 49 deletions(-) diff --git a/attestor/policy/attestation_policy_test.go b/attestor/policy/attestation_policy_test.go index 61e222b906d..6b958ae5be6 100644 --- a/attestor/policy/attestation_policy_test.go +++ b/attestor/policy/attestation_policy_test.go @@ -379,7 +379,7 @@ func TestCheckCodeReviewed(t *testing.T) { } } -func asPointer(s string) *string { +func asStringPointer(s string) *string { return &s } @@ -399,7 +399,7 @@ func TestNoUnpinnedDependencies(t *testing.T) { raw: &checker.RawResults{ PinningDependenciesResults: checker.PinningDependenciesData{ Dependencies: []checker.Dependency{ - {Name: asPointer("foo"), PinnedAt: asPointer("abcdef")}, + {Name: asStringPointer("foo"), PinnedAt: asStringPointer("abcdef")}, }, }, }, @@ -413,7 +413,7 @@ func TestNoUnpinnedDependencies(t *testing.T) { raw: &checker.RawResults{ PinningDependenciesResults: checker.PinningDependenciesData{ Dependencies: []checker.Dependency{ - {Name: asPointer("foo"), PinnedAt: nil}, + {Name: asStringPointer("foo"), PinnedAt: nil}, }, }, }, @@ -427,7 +427,7 @@ func TestNoUnpinnedDependencies(t *testing.T) { raw: &checker.RawResults{ PinningDependenciesResults: checker.PinningDependenciesData{ Dependencies: []checker.Dependency{ - {Name: asPointer("foo"), PinnedAt: nil}, + {Name: asStringPointer("foo"), PinnedAt: nil}, }, }, }, @@ -439,7 +439,7 @@ func TestNoUnpinnedDependencies(t *testing.T) { raw: &checker.RawResults{ PinningDependenciesResults: checker.PinningDependenciesData{ Dependencies: []checker.Dependency{ - {Name: asPointer("second-pkg"), Location: &checker.File{Path: "bar"}, PinnedAt: nil}, + {Name: asStringPointer("second-pkg"), Location: &checker.File{Path: "bar"}, PinnedAt: nil}, }, }, }, diff --git a/checks/evaluation/pinned_dependencies_test.go b/checks/evaluation/pinned_dependencies_test.go index 0a40c0da9a0..9e457b626e6 100644 --- a/checks/evaluation/pinned_dependencies_test.go +++ b/checks/evaluation/pinned_dependencies_test.go @@ -86,7 +86,7 @@ func Test_createReturnValuesForGitHubActionsWorkflowPinned(t *testing.T) { } } -func asPointer(s string) *string { +func asStringPointer(s string) *string { return &s } @@ -103,7 +103,7 @@ func Test_PinningDependencies(t *testing.T) { dependencies: []checker.Dependency{ { Location: &checker.File{}, - Msg: asPointer("some message"), + Msg: asStringPointer("some message"), Type: checker.DependencyUseTypeDownloadThenRun, }, }, @@ -120,7 +120,7 @@ func Test_PinningDependencies(t *testing.T) { dependencies: []checker.Dependency{ { Location: &checker.File{}, - Msg: asPointer("some message"), + Msg: asStringPointer("some message"), Type: checker.DependencyUseTypeDownloadThenRun, }, { @@ -153,7 +153,7 @@ func Test_PinningDependencies(t *testing.T) { }, { Location: &checker.File{}, - Msg: asPointer("debug message"), + Msg: asStringPointer("debug message"), }, }, expected: scut.TestReturn{ diff --git a/checks/raw/dependency_update_tool.go b/checks/raw/dependency_update_tool.go index 3af013b4088..b0b3e2faee0 100644 --- a/checks/raw/dependency_update_tool.go +++ b/checks/raw/dependency_update_tool.go @@ -49,8 +49,8 @@ func DependencyUpdateTool(c clients.RepoClient) (checker.DependencyUpdateToolDat if commits[i].Committer.ID == dependabotID { tools = append(tools, checker.Tool{ Name: "Dependabot", - URL: asPointer("https://github.com/dependabot"), - Desc: asPointer("Automated dependency updates built into GitHub"), + URL: asStringPointer("https://github.com/dependabot"), + Desc: asStringPointer("Automated dependency updates built into GitHub"), Files: []checker.File{{}}, }) break @@ -74,8 +74,8 @@ var checkDependencyFileExists fileparser.DoWhileTrueOnFilename = func(name strin case ".github/dependabot.yml", ".github/dependabot.yaml": *ptools = append(*ptools, checker.Tool{ Name: "Dependabot", - URL: asPointer("https://github.com/dependabot"), - Desc: asPointer("Automated dependency updates built into GitHub"), + URL: asStringPointer("https://github.com/dependabot"), + Desc: asStringPointer("Automated dependency updates built into GitHub"), Files: []checker.File{ { Path: name, @@ -90,8 +90,8 @@ var checkDependencyFileExists fileparser.DoWhileTrueOnFilename = func(name strin "renovate.json5", ".renovaterc": *ptools = append(*ptools, checker.Tool{ Name: "RenovateBot", - URL: asPointer("https://github.com/renovatebot/renovate"), - Desc: asPointer("Automated dependency updates. Multi-platform and multi-language."), + URL: asStringPointer("https://github.com/renovatebot/renovate"), + Desc: asStringPointer("Automated dependency updates. Multi-platform and multi-language."), Files: []checker.File{ { Path: name, @@ -103,8 +103,8 @@ var checkDependencyFileExists fileparser.DoWhileTrueOnFilename = func(name strin case ".pyup.yml": *ptools = append(*ptools, checker.Tool{ Name: "PyUp", - URL: asPointer("https://pyup.io/"), - Desc: asPointer("Automated dependency updates for Python."), + URL: asStringPointer("https://pyup.io/"), + Desc: asStringPointer("Automated dependency updates for Python."), Files: []checker.File{ { Path: name, @@ -116,8 +116,8 @@ var checkDependencyFileExists fileparser.DoWhileTrueOnFilename = func(name strin case ".lift.toml", ".lift/config.toml": *ptools = append(*ptools, checker.Tool{ Name: "Sonatype Lift", - URL: asPointer("https://lift.sonatype.com"), - Desc: asPointer("Automated dependency updates. Multi-platform and multi-language."), + URL: asStringPointer("https://lift.sonatype.com"), + Desc: asStringPointer("Automated dependency updates. Multi-platform and multi-language."), Files: []checker.File{ { Path: name, @@ -133,6 +133,6 @@ var checkDependencyFileExists fileparser.DoWhileTrueOnFilename = func(name strin return true, nil } -func asPointer(s string) *string { +func asStringPointer(s string) *string { return &s } diff --git a/checks/raw/fuzzing.go b/checks/raw/fuzzing.go index a8a161d9847..b6b9d41c8aa 100644 --- a/checks/raw/fuzzing.go +++ b/checks/raw/fuzzing.go @@ -62,8 +62,8 @@ var languageFuzzSpecs = map[clients.LanguageName]languageFuzzConfig{ filePattern: "*_test.go", funcPattern: `func\s+Fuzz\w+\s*\(\w+\s+\*testing.F\)`, Name: fuzzerBuiltInGo, - URL: asPointer("https://go.dev/doc/fuzz/"), - Desc: asPointer( + URL: asStringPointer("https://go.dev/doc/fuzz/"), + Desc: asStringPointer( "Go fuzzing intelligently walks through the source code to report failures and find vulnerabilities."), }, // Fuzz patterns for Haskell based on property-based testing. @@ -127,8 +127,8 @@ func Fuzzing(c *checker.CheckRequest) (checker.FuzzingData, error) { fuzzers = append(fuzzers, checker.Tool{ Name: fuzzerClusterFuzzLite, - URL: asPointer("https://github.com/google/clusterfuzzlite"), - Desc: asPointer("continuous fuzzing solution that runs as part of Continuous Integration (CI) workflows"), + URL: asStringPointer("https://github.com/google/clusterfuzzlite"), + Desc: asStringPointer("continuous fuzzing solution that runs as part of Continuous Integration (CI) workflows"), // TODO: File. }, ) @@ -142,8 +142,8 @@ func Fuzzing(c *checker.CheckRequest) (checker.FuzzingData, error) { fuzzers = append(fuzzers, checker.Tool{ Name: oneFuzz, - URL: asPointer("https://github.com/microsoft/onefuzz"), - Desc: asPointer("Enables continuous developer-driven fuzzing to proactively harden software prior to release."), + URL: asStringPointer("https://github.com/microsoft/onefuzz"), + Desc: asStringPointer("Enables continuous developer-driven fuzzing to proactively harden software prior to release."), // TODO: File. }, ) @@ -157,8 +157,8 @@ func Fuzzing(c *checker.CheckRequest) (checker.FuzzingData, error) { fuzzers = append(fuzzers, checker.Tool{ Name: fuzzerOSSFuzz, - URL: asPointer("https://github.com/google/oss-fuzz"), - Desc: asPointer("Continuous Fuzzing for Open Source Software"), + URL: asStringPointer("https://github.com/google/oss-fuzz"), + Desc: asStringPointer("Continuous Fuzzing for Open Source Software"), // TODO: File. }, ) diff --git a/checks/raw/pinned_dependencies.go b/checks/raw/pinned_dependencies.go index 3723d8c0e5e..faf570fcd6a 100644 --- a/checks/raw/pinned_dependencies.go +++ b/checks/raw/pinned_dependencies.go @@ -272,8 +272,8 @@ var validateDockerfilesPinning fileparser.DoWhileTrueOnFileContent = func( EndOffset: uint(child.EndLine), Snippet: child.Original, }, - Name: asPointer(name), - PinnedAt: asPointer(asName), + Name: asStringPointer(name), + PinnedAt: asStringPointer(asName), Pinned: pinned || regex.MatchString(name), Type: checker.DependencyUseTypeDockerfileContainerImage, }, @@ -297,9 +297,9 @@ var validateDockerfilesPinning fileparser.DoWhileTrueOnFileContent = func( } parts := strings.SplitN(name, ":", 2) if len(parts) > 0 { - dep.Name = asPointer(parts[0]) + dep.Name = asStringPointer(parts[0]) if len(parts) > 1 { - dep.PinnedAt = asPointer(parts[1]) + dep.PinnedAt = asStringPointer(parts[1]) } } pdata.Dependencies = append(pdata.Dependencies, dep) @@ -394,7 +394,7 @@ var validateGitHubWorkflowIsFreeOfInsecureDownloads fileparser.DoWhileTrueOnFile if err := validateShellFile(pathfn, uint(execRun.Run.Pos.Line), uint(execRun.Run.Pos.Line), script, taintedFiles, pdata); err != nil { pdata.Dependencies = append(pdata.Dependencies, checker.Dependency{ - Msg: asPointer(err.Error()), + Msg: asStringPointer(err.Error()), }) } } @@ -482,9 +482,9 @@ var validateGitHubActionWorkflow fileparser.DoWhileTrueOnFileContent = func( } parts := strings.SplitN(execAction.Uses.Value, "@", 2) if len(parts) > 0 { - dep.Name = asPointer(parts[0]) + dep.Name = asStringPointer(parts[0]) if len(parts) > 1 { - dep.PinnedAt = asPointer(parts[1]) + dep.PinnedAt = asStringPointer(parts[1]) } } pdata.Dependencies = append(pdata.Dependencies, dep) diff --git a/cron/worker/worker_test.go b/cron/worker/worker_test.go index d1b04f7d396..cb9177df30d 100644 --- a/cron/worker/worker_test.go +++ b/cron/worker/worker_test.go @@ -23,7 +23,7 @@ import ( "github.com/ossf/scorecard/v4/cron/data" ) -func asPointer(i int32) *int32 { +func asIntPointer(i int32) *int32 { return &i } @@ -38,7 +38,7 @@ func TestResultFilename(t *testing.T) { name: "Basic", req: &data.ScorecardBatchRequest{ JobTime: timestamppb.New(time.Date(1979, time.October, 12, 1, 2, 3, 0, time.UTC)), - ShardNum: asPointer(42), + ShardNum: asIntPointer(42), }, want: "1979.10.12/010203/shard-0000042", }, diff --git a/dependencydiff/dependencydiff_test.go b/dependencydiff/dependencydiff_test.go index 9a4699fcdb6..688f3d24351 100644 --- a/dependencydiff/dependencydiff_test.go +++ b/dependencydiff/dependencydiff_test.go @@ -126,11 +126,11 @@ func Test_mapDependencyEcosystemNaming(t *testing.T) { deps: []dependency{ { Name: "dependency_1", - Ecosystem: asPointer("not_supported"), + Ecosystem: asStringPointer("not_supported"), }, { Name: "dependency_2", - Ecosystem: asPointer("gomod"), + Ecosystem: asStringPointer("gomod"), }, }, errWanted: errInvalid, @@ -140,7 +140,7 @@ func Test_mapDependencyEcosystemNaming(t *testing.T) { deps: []dependency{ { Name: "dependency_3", - Ecosystem: asPointer("foobar"), + Ecosystem: asStringPointer("foobar"), }, }, errWanted: errMappingNotFound, @@ -150,19 +150,19 @@ func Test_mapDependencyEcosystemNaming(t *testing.T) { deps: []dependency{ { Name: "dependency_4", - Ecosystem: asPointer("gomod"), + Ecosystem: asStringPointer("gomod"), }, { Name: "dependency_5", - Ecosystem: asPointer("pip"), + Ecosystem: asStringPointer("pip"), }, { Name: "dependency_6", - Ecosystem: asPointer("cargo"), + Ecosystem: asStringPointer("cargo"), }, { Name: "dependency_7", - Ecosystem: asPointer("actions"), + Ecosystem: asStringPointer("actions"), }, }, }, diff --git a/dependencydiff/mapping.go b/dependencydiff/mapping.go index 285fd88173d..7d05fd7609c 100644 --- a/dependencydiff/mapping.go +++ b/dependencydiff/mapping.go @@ -22,7 +22,7 @@ import ( type ecosystem string // OSV ecosystem naming data source: https://ossf.github.io/osv-schema/#affectedpackage-field -//nolint +// nolint const ( // The Go ecosystem. ecosystemGo ecosystem = "Go" @@ -101,7 +101,7 @@ func mapDependencyEcosystemNaming(deps []dependency) error { // Iff. the ecosystem is not empty and the mapping entry is not found, we will return an error. return fmt.Errorf("error mapping dependency ecosystem: %w", err) } - deps[i].Ecosystem = asPointer(string(mappedEcosys)) + deps[i].Ecosystem = asStringPointer(string(mappedEcosys)) } return nil } @@ -116,6 +116,6 @@ func toEcosystem(e string) (ecosystem, error) { return "", fmt.Errorf("%w for github entry %s", errMappingNotFound, e) } -func asPointer(s string) *string { +func asStringPointer(s string) *string { return &s } diff --git a/pkg/json_raw_results.go b/pkg/json_raw_results.go index 629bee123b9..85adcd2a1f0 100644 --- a/pkg/json_raw_results.go +++ b/pkg/json_raw_results.go @@ -292,7 +292,7 @@ type jsonRawResults struct { DependencyPinning jsonPinningDependenciesData `json:"dependencyPinning"` } -func asPointer(s string) *string { +func asStringPointer(s string) *string { return &s } @@ -312,7 +312,7 @@ func (r *jsonScorecardRawResult) addTokenPermissionsRawResults(tp *checker.Token } p := jsonTokenPermission{ - LocationType: asPointer(string(*t.LocationType)), + LocationType: asStringPointer(string(*t.LocationType)), Name: t.Name, Value: t.Value, Type: string(t.Type), From e497a2081125b6c60c501d33dd17e66fe519c800 Mon Sep 17 00:00:00 2001 From: Gabriela Gutierrez Date: Thu, 16 Feb 2023 10:36:36 +0000 Subject: [PATCH 03/46] fix: Pinned Dependency field type Field needs to be a pointer to work when accessing values on evaluation. Signed-off-by: Gabriela Gutierrez --- attestor/policy/attestation_policy_test.go | 2 +- checker/raw_result.go | 2 +- checks/evaluation/pinned_dependencies_test.go | 4 +- checks/raw/dependency_update_tool.go | 4 ++ checks/raw/fuzzing.go | 6 +- checks/raw/pinned_dependencies.go | 6 +- checks/raw/shell_download_validate.go | 14 ++-- pkg/json_raw_results.go | 6 +- pkg/json_raw_results_test.go | 64 +++++++++---------- remediation/remediations_test.go | 14 ++-- 10 files changed, 63 insertions(+), 59 deletions(-) diff --git a/attestor/policy/attestation_policy_test.go b/attestor/policy/attestation_policy_test.go index 6b958ae5be6..5e8f4d54096 100644 --- a/attestor/policy/attestation_policy_test.go +++ b/attestor/policy/attestation_policy_test.go @@ -651,7 +651,7 @@ func TestAttestationPolicy_EvaluateResults(t *testing.T) { raw: &checker.RawResults{ PinningDependenciesResults: checker.PinningDependenciesData{ Dependencies: []checker.Dependency{ - {Name: asPointer("foo"), PinnedAt: asPointer("abcdef")}, + {Name: asStringPointer("foo"), PinnedAt: asStringPointer("abcdef")}, }, }, }, diff --git a/checker/raw_result.go b/checker/raw_result.go index 3589a4ee631..8f63dc1fa59 100644 --- a/checker/raw_result.go +++ b/checker/raw_result.go @@ -122,7 +122,7 @@ type Dependency struct { Location *File Msg *string // Only for debug messages. Type DependencyUseType - Pinned bool + Pinned *bool } // MaintainedData contains the raw results diff --git a/checks/evaluation/pinned_dependencies_test.go b/checks/evaluation/pinned_dependencies_test.go index 9e457b626e6..6d90d09d8b0 100644 --- a/checks/evaluation/pinned_dependencies_test.go +++ b/checks/evaluation/pinned_dependencies_test.go @@ -186,7 +186,7 @@ func Test_PinningDependencies(t *testing.T) { { Location: &checker.File{}, Type: checker.DependencyUseTypePipCommand, - Msg: asPointer("debug message"), + Msg: asStringPointer("debug message"), }, }, expected: scut.TestReturn{ @@ -224,7 +224,7 @@ func Test_PinningDependencies(t *testing.T) { }, { Location: &checker.File{}, - Msg: asPointer("debug message"), + Msg: asStringPointer("debug message"), }, }, expected: scut.TestReturn{ diff --git a/checks/raw/dependency_update_tool.go b/checks/raw/dependency_update_tool.go index b0b3e2faee0..a6ee391a25b 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 asStringPointer(s string) *string { return &s } + +func asBoolPointer(b bool) *bool { + return &b +} diff --git a/checks/raw/fuzzing.go b/checks/raw/fuzzing.go index b6b9d41c8aa..b5c9f0d910e 100644 --- a/checks/raw/fuzzing.go +++ b/checks/raw/fuzzing.go @@ -85,7 +85,7 @@ var languageFuzzSpecs = map[clients.LanguageName]languageFuzzConfig{ // or their indirect imports through the higher-level Hspec or Tasty testing frameworks. funcPattern: `import\s+(qualified\s+)?Test\.((Hspec|Tasty)\.)?(QuickCheck|Hedgehog|Validity|SmallCheck)`, Name: fuzzerPropertyBasedHaskell, - Desc: asPointer( + Desc: asStringPointer( "Property-based testing in Haskell generates test instances randomly or exhaustively " + "and test that specific properties are satisfied."), }, @@ -100,7 +100,7 @@ var languageFuzzSpecs = map[clients.LanguageName]languageFuzzConfig{ // Look for direct imports of fast-check. funcPattern: `(from\s+['"]fast-check['"]|require\(\s*['"]fast-check['"]\s*\))`, Name: fuzzerPropertyBasedJavaScript, - Desc: asPointer( + Desc: asStringPointer( "Property-based testing in JavaScript generates test instances randomly or exhaustively " + "and test that specific properties are satisfied."), }, @@ -109,7 +109,7 @@ var languageFuzzSpecs = map[clients.LanguageName]languageFuzzConfig{ // Look for direct imports of fast-check. funcPattern: `(from\s+['"]fast-check['"]|require\(\s*['"]fast-check['"]\s*\))`, Name: fuzzerPropertyBasedTypeScript, - Desc: asPointer( + Desc: asStringPointer( "Property-based testing in TypeScript generates test instances randomly or exhaustively " + "and test that specific properties are satisfied."), }, diff --git a/checks/raw/pinned_dependencies.go b/checks/raw/pinned_dependencies.go index faf570fcd6a..1054f847824 100644 --- a/checks/raw/pinned_dependencies.go +++ b/checks/raw/pinned_dependencies.go @@ -274,7 +274,7 @@ var validateDockerfilesPinning fileparser.DoWhileTrueOnFileContent = func( }, Name: asStringPointer(name), PinnedAt: asStringPointer(asName), - Pinned: pinned || regex.MatchString(name), + Pinned: asBoolPointer(pinned || regex.MatchString(name)), Type: checker.DependencyUseTypeDockerfileContainerImage, }, ) @@ -292,7 +292,7 @@ var validateDockerfilesPinning fileparser.DoWhileTrueOnFileContent = func( EndOffset: uint(child.EndLine), Snippet: child.Original, }, - Pinned: pinned || regex.MatchString(name), + Pinned: asBoolPointer(pinned || regex.MatchString(name)), Type: checker.DependencyUseTypeDockerfileContainerImage, } parts := strings.SplitN(name, ":", 2) @@ -477,7 +477,7 @@ var validateGitHubActionWorkflow fileparser.DoWhileTrueOnFileContent = func( EndOffset: uint(execAction.Uses.Pos.Line), // `Uses` always span a single line. Snippet: execAction.Uses.Value, }, - Pinned: isActionDependencyPinned(execAction.Uses.Value), + Pinned: asBoolPointer(isActionDependencyPinned(execAction.Uses.Value)), Type: checker.DependencyUseTypeGHAction, } parts := strings.SplitN(execAction.Uses.Value, "@", 2) diff --git a/checks/raw/shell_download_validate.go b/checks/raw/shell_download_validate.go index 599b8d59ce5..8e53f8e9887 100644 --- a/checks/raw/shell_download_validate.go +++ b/checks/raw/shell_download_validate.go @@ -337,7 +337,7 @@ func collectFetchPipeExecute(startLine, endLine uint, node syntax.Node, cmd, pat EndOffset: endLine, Snippet: cmd, }, - Pinned: false, + Pinned: asBoolPointer(false), Type: checker.DependencyUseTypeDownloadThenRun, }, ) @@ -389,7 +389,7 @@ func collectExecuteFiles(startLine, endLine uint, node syntax.Node, cmd, pathfn EndOffset: endLine, Snippet: cmd, }, - Pinned: false, + Pinned: asBoolPointer(false), Type: checker.DependencyUseTypeDownloadThenRun, }, ) @@ -797,7 +797,7 @@ func collectUnpinnedPakageManagerDownload(startLine, endLine uint, node syntax.N EndOffset: endLine, Snippet: cmd, }, - Pinned: !isGoUnpinnedDownload(c), + Pinned: asBoolPointer(!isGoUnpinnedDownload(c)), Type: checker.DependencyUseTypeGoCommand, }, ) @@ -816,7 +816,7 @@ func collectUnpinnedPakageManagerDownload(startLine, endLine uint, node syntax.N EndOffset: endLine, Snippet: cmd, }, - Pinned: !isPipUnpinnedDownload(c), + Pinned: asBoolPointer(!isPipUnpinnedDownload(c)), Type: checker.DependencyUseTypePipCommand, }, ) @@ -835,7 +835,7 @@ func collectUnpinnedPakageManagerDownload(startLine, endLine uint, node syntax.N EndOffset: endLine, Snippet: cmd, }, - Pinned: false, + Pinned: asBoolPointer(false), Type: checker.DependencyUseTypeNpmCommand, }, ) @@ -854,7 +854,7 @@ func collectUnpinnedPakageManagerDownload(startLine, endLine uint, node syntax.N EndOffset: endLine, Snippet: cmd, }, - Pinned: !isChocoUnpinnedDownload(c), + Pinned: asBoolPointer(!isChocoUnpinnedDownload(c)), Type: checker.DependencyUseTypeChocoCommand, }, ) @@ -968,7 +968,7 @@ func collectFetchProcSubsExecute(startLine, endLine uint, node syntax.Node, cmd, EndOffset: endLine, Snippet: cmd, }, - Pinned: false, + Pinned: asBoolPointer(false), Type: checker.DependencyUseTypeDownloadThenRun, }, ) diff --git a/pkg/json_raw_results.go b/pkg/json_raw_results.go index 85adcd2a1f0..7da73b64475 100644 --- a/pkg/json_raw_results.go +++ b/pkg/json_raw_results.go @@ -331,7 +331,7 @@ func (r *jsonScorecardRawResult) addTokenPermissionsRawResults(tp *checker.Token Offset: t.File.Offset, } if t.File.Snippet != "" { - p.File.Snippet = asPointer(t.File.Snippet) + p.File.Snippet = asStringPointer(t.File.Snippet) } } @@ -361,7 +361,7 @@ func (r *jsonScorecardRawResult) addPackagingRawResults(pk *checker.PackagingDat } if p.File.Snippet != "" { - jpk.File.Snippet = asPointer(p.File.Snippet) + jpk.File.Snippet = asStringPointer(p.File.Snippet) } for _, run := range p.Runs { @@ -419,7 +419,7 @@ func (r *jsonScorecardRawResult) addDangerousWorkflowRawResults(df *checker.Dang Type: string(e.Type), } if e.File.Snippet != "" { - v.File.Snippet = asPointer(e.File.Snippet) + v.File.Snippet = asStringPointer(e.File.Snippet) } if e.Job != nil { v.Job = &jsonWorkflowJob{ diff --git a/pkg/json_raw_results_test.go b/pkg/json_raw_results_test.go index 82adcc8b116..46d350417db 100644 --- a/pkg/json_raw_results_test.go +++ b/pkg/json_raw_results_test.go @@ -26,7 +26,7 @@ import ( "github.com/ossf/scorecard/v4/clients" ) -func TestAsPointer(t *testing.T) { +func TestAsStringPointer(t *testing.T) { t.Parallel() tests := []struct { //nolint:govet @@ -37,17 +37,17 @@ func TestAsPointer(t *testing.T) { { name: "test_empty_string", input: "", - expected: asPointer(""), + expected: asStringPointer(""), }, { name: "test_non_empty_string", input: "test", - expected: asPointer("test"), + expected: asStringPointer("test"), }, { name: "test_number_string", input: "123", - expected: asPointer("123"), + expected: asStringPointer("123"), }, } @@ -55,9 +55,9 @@ func TestAsPointer(t *testing.T) { tt := tt t.Run(tt.name, func(t *testing.T) { t.Parallel() - result := asPointer(tt.input) + result := asStringPointer(tt.input) if *result != *tt.expected { - t.Errorf("asPointer() = %v, want %v", result, tt.expected) + t.Errorf("asStringPointer() = %v, want %v", result, tt.expected) } }) } @@ -110,7 +110,7 @@ func TestJsonScorecardRawResult_AddPackagingRawResults(t *testing.T) { input: &checker.PackagingData{ Packages: []checker.Package{ { - Msg: asPointer("testMsg"), + Msg: asStringPointer("testMsg"), }, }, }, @@ -193,8 +193,8 @@ func TestJsonScorecardRawResult_AddTokenPermissionsRawResults(t *testing.T) { LocationType: &loc, Type: checker.PermissionLevelUndeclared, Job: &checker.WorkflowJob{ - Name: asPointer("testJobName"), - ID: asPointer("testJobID"), + Name: asStringPointer("testJobName"), + ID: asStringPointer("testJobID"), }, File: &checker.File{ Path: "testPath", @@ -250,8 +250,8 @@ func TestJsonScorecardRawResult_AddDependencyPinningRawResults(t *testing.T) { EndOffset: 5, Snippet: "testSnippet", }, - Name: asPointer("testDependency"), - PinnedAt: asPointer("testPinnedAt"), + Name: asStringPointer("testDependency"), + PinnedAt: asStringPointer("testPinnedAt"), Type: checker.DependencyUseTypeGHAction, }, }, @@ -264,8 +264,8 @@ func TestJsonScorecardRawResult_AddDependencyPinningRawResults(t *testing.T) { Dependencies: []checker.Dependency{ { Location: nil, - Name: asPointer("testDependency"), - PinnedAt: asPointer("testPinnedAt"), + Name: asStringPointer("testDependency"), + PinnedAt: asStringPointer("testPinnedAt"), Type: checker.DependencyUseTypeGHAction, }, }, @@ -309,8 +309,8 @@ func TestJsonScorecardRawResult_AddDangerousWorkflowRawResults(t *testing.T) { }, Type: checker.DangerousWorkflowScriptInjection, Job: &checker.WorkflowJob{ - Name: asPointer("testJob"), - ID: asPointer("testID"), + Name: asStringPointer("testJob"), + ID: asStringPointer("testID"), }, }, }, @@ -437,7 +437,7 @@ func TestJsonScorecardRawResult_AddMaintainedRawResults(t *testing.T) { CreatedAt: time.Now(), Issues: []clients.Issue{ { - URI: asPointer("testUrl"), + URI: asStringPointer("testUrl"), Author: &clients.User{ Login: "testLogin", }, @@ -960,8 +960,8 @@ func TestAddFuzzingRawResults(t *testing.T) { Fuzzers: []checker.Tool{ { Name: "fuzzer1", - URL: asPointer("https://example.com/fuzzer1"), - Desc: asPointer("Fuzzer 1 description"), + URL: asStringPointer("https://example.com/fuzzer1"), + Desc: asStringPointer("Fuzzer 1 description"), Files: []checker.File{ { Path: "path/to/fuzzer1/file1", @@ -973,8 +973,8 @@ func TestAddFuzzingRawResults(t *testing.T) { }, { Name: "fuzzer2", - URL: asPointer("https://example.com/fuzzer2"), - Desc: asPointer("Fuzzer 2 description"), + URL: asStringPointer("https://example.com/fuzzer2"), + Desc: asStringPointer("Fuzzer 2 description"), Files: []checker.File{ { Path: "path/to/fuzzer2/file1", @@ -992,8 +992,8 @@ func TestAddFuzzingRawResults(t *testing.T) { expectedFuzzers := []jsonTool{ { Name: "fuzzer1", - URL: asPointer("https://example.com/fuzzer1"), - Desc: asPointer("Fuzzer 1 description"), + URL: asStringPointer("https://example.com/fuzzer1"), + Desc: asStringPointer("Fuzzer 1 description"), Files: []jsonFile{ { Path: "path/to/fuzzer1/file1", @@ -1005,8 +1005,8 @@ func TestAddFuzzingRawResults(t *testing.T) { }, { Name: "fuzzer2", - URL: asPointer("https://example.com/fuzzer2"), - Desc: asPointer("Fuzzer 2 description"), + URL: asStringPointer("https://example.com/fuzzer2"), + Desc: asStringPointer("Fuzzer 2 description"), Files: []jsonFile{ { Path: "path/to/fuzzer2/file1", @@ -1080,8 +1080,8 @@ func TestJsonScorecardRawResult(t *testing.T) { Fuzzers: []checker.Tool{ { Name: "fuzzer1", - URL: asPointer("https://example.com/fuzzer1"), - Desc: asPointer("fuzzer1 description"), + URL: asStringPointer("https://example.com/fuzzer1"), + Desc: asStringPointer("fuzzer1 description"), Files: []checker.File{ {Path: "fuzzers/fuzzer1/foo"}, {Path: "fuzzers/fuzzer1/bar"}, @@ -1089,8 +1089,8 @@ func TestJsonScorecardRawResult(t *testing.T) { }, { Name: "fuzzer2", - URL: asPointer("https://example.com/fuzzer2"), - Desc: asPointer("fuzzer2 description"), + URL: asStringPointer("https://example.com/fuzzer2"), + Desc: asStringPointer("fuzzer2 description"), Files: []checker.File{ {Path: "fuzzers/fuzzer2/foo"}, {Path: "fuzzers/fuzzer2/bar"}, @@ -1184,8 +1184,8 @@ func TestJsonScorecardRawResult(t *testing.T) { expectedFuzzers := []jsonTool{ { Name: "fuzzer1", - URL: asPointer("https://example.com/fuzzer1"), - Desc: asPointer("fuzzer1 description"), + URL: asStringPointer("https://example.com/fuzzer1"), + Desc: asStringPointer("fuzzer1 description"), Files: []jsonFile{ {Path: "fuzzers/fuzzer1/foo"}, {Path: "fuzzers/fuzzer1/bar"}, @@ -1193,8 +1193,8 @@ func TestJsonScorecardRawResult(t *testing.T) { }, { Name: "fuzzer2", - URL: asPointer("https://example.com/fuzzer1"), - Desc: asPointer("fuzzer1 description"), + URL: asStringPointer("https://example.com/fuzzer1"), + Desc: asStringPointer("fuzzer1 description"), Files: []jsonFile{ {Path: "fuzzers/fuzzer2/foo"}, {Path: "fuzzers/fuzzer2/bar"}, diff --git a/remediation/remediations_test.go b/remediation/remediations_test.go index dce119881d5..e80b72ca4cf 100644 --- a/remediation/remediations_test.go +++ b/remediation/remediations_test.go @@ -52,7 +52,7 @@ func TestRepeatedSetup(t *testing.T) { } } -func asPointer(s string) *string { +func asStringPointer(s string) *string { return &s } @@ -89,7 +89,7 @@ func TestCreateDockerfilePinningRemediation(t *testing.T) { { name: "image name no tag", dep: checker.Dependency{ - Name: asPointer("foo"), + Name: asStringPointer("foo"), Type: checker.DependencyUseTypeDockerfileContainerImage, }, expected: &rule.Remediation{ @@ -101,8 +101,8 @@ func TestCreateDockerfilePinningRemediation(t *testing.T) { // github.com/ossf/scorecard/issues/2581 name: "image name with tag", dep: checker.Dependency{ - Name: asPointer("amazoncorretto"), - PinnedAt: asPointer("11"), + Name: asStringPointer("amazoncorretto"), + PinnedAt: asStringPointer("11"), Type: checker.DependencyUseTypeDockerfileContainerImage, }, expected: &rule.Remediation{ @@ -113,7 +113,7 @@ func TestCreateDockerfilePinningRemediation(t *testing.T) { { name: "unknown image", dep: checker.Dependency{ - Name: asPointer("not-found"), + Name: asStringPointer("not-found"), Type: checker.DependencyUseTypeDockerfileContainerImage, }, expected: nil, @@ -121,8 +121,8 @@ func TestCreateDockerfilePinningRemediation(t *testing.T) { { name: "unknown tag", dep: checker.Dependency{ - Name: asPointer("foo"), - PinnedAt: asPointer("not-found"), + Name: asStringPointer("foo"), + PinnedAt: asStringPointer("not-found"), Type: checker.DependencyUseTypeDockerfileContainerImage, }, expected: nil, From 3f3dd8034fa7fece6284b42a49f5d80aa920fbdb Mon Sep 17 00:00:00 2001 From: Gabriela Gutierrez Date: Mon, 28 Aug 2023 18:38:54 +0000 Subject: [PATCH 04/46] 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 --- checks/evaluation/pinned_dependencies.go | 45 +++++++++--------------- 1 file changed, 16 insertions(+), 29 deletions(-) diff --git a/checks/evaluation/pinned_dependencies.go b/checks/evaluation/pinned_dependencies.go index c05038fa7ec..b26e07e4856 100644 --- a/checks/evaluation/pinned_dependencies.go +++ b/checks/evaluation/pinned_dependencies.go @@ -28,13 +28,10 @@ import ( 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. @@ -80,7 +77,7 @@ func PinningDependencies(name string, c *checker.CheckRequest, Text: *rr.Msg, Snippet: rr.Location.Snippet, }) - } else { + } else if !*rr.Pinned { dl.Warn(&checker.LogMessage{ Path: rr.Location.Path, Type: rr.Location.Type, @@ -90,11 +87,11 @@ func PinningDependencies(name string, c *checker.CheckRequest, Snippet: rr.Location.Snippet, Remediation: generateRemediation(remediationMetadata, &rr), }) + } // Update the pinning status. updatePinningResults(&rr, &wp, pr) } - } // Generate scores and Info results. // GitHub actions. @@ -177,13 +174,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) + var p pinnedResult = pr[rr.Type] + addPinnedResult(rr, &p) pr[rr.Type] = p } @@ -213,28 +210,18 @@ func maxScore(s1, s2 int) int { return s2 } -// 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 addPinnedResult(rr *checker.Dependency, r *pinnedResult) { + if *rr.Pinned { + r.pinned += 1 } + r.total += 1 } -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) } } From 309158aee2bb93083e66f08f7bdd06e1bea3750a Mon Sep 17 00:00:00 2001 From: Gabriela Gutierrez Date: Mon, 28 Aug 2023 19:07:47 +0000 Subject: [PATCH 05/46] 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 --- checks/evaluation/pinned_dependencies.go | 72 ++++++++++++++++-------- 1 file changed, 48 insertions(+), 24 deletions(-) diff --git a/checks/evaluation/pinned_dependencies.go b/checks/evaluation/pinned_dependencies.go index b26e07e4856..321fd21e080 100644 --- a/checks/evaluation/pinned_dependencies.go +++ b/checks/evaluation/pinned_dependencies.go @@ -15,7 +15,6 @@ package evaluation import ( - "errors" "fmt" "github.com/ossf/scorecard/v4/checker" @@ -26,8 +25,6 @@ import ( "github.com/ossf/scorecard/v4/rule" ) -var errInvalidValue = errors.New("invalid value") - type pinnedResult struct { pinned int total int @@ -89,9 +86,9 @@ func PinningDependencies(name string, c *checker.CheckRequest, }) } - // Update the pinning status. - updatePinningResults(&rr, &wp, pr) - } + // Update the pinning status. + updatePinningResults(&rr, &wp, pr) + } // Generate scores and Info results. // GitHub actions. @@ -231,6 +228,7 @@ func createReturnForIsShellScriptFreeOfInsecureDownloads(pr map[checker.Dependen ) (int, error) { return createReturnValues(pr, checker.DependencyUseTypeDownloadThenRun, "no insecure (not pinned by hash) dependency downloads found in shell scripts", + "no dependency downloads found in shell scripts", dl) } @@ -240,6 +238,7 @@ func createReturnForIsDockerfilePinned(pr map[checker.DependencyUseType]pinnedRe ) (int, error) { return createReturnValues(pr, checker.DependencyUseTypeDockerfileContainerImage, "Dockerfile dependencies are pinned", + "no Dockerfile dependencies found", dl) } @@ -249,6 +248,7 @@ func createReturnForIsDockerfileFreeOfInsecureDownloads(pr map[checker.Dependenc ) (int, error) { return createReturnValues(pr, checker.DependencyUseTypeDownloadThenRun, "no insecure (not pinned by hash) dependency downloads found in Dockerfiles", + "no dependency downloads found in Dockerfiles", dl) } @@ -257,7 +257,8 @@ func createReturnForIsPipInstallPinned(pr map[checker.DependencyUseType]pinnedRe dl checker.DetailLogger, ) (int, error) { return createReturnValues(pr, checker.DependencyUseTypePipCommand, - "Pip installs are pinned", + "pip installs are pinned", + "no pip installs found", dl) } @@ -267,6 +268,7 @@ func createReturnForIsNpmInstallPinned(pr map[checker.DependencyUseType]pinnedRe ) (int, error) { return createReturnValues(pr, checker.DependencyUseTypeNpmCommand, "npm installs are pinned", + "no npm installs found", dl) } @@ -276,27 +278,32 @@ func createReturnForIsGoInstallPinned(pr map[checker.DependencyUseType]pinnedRes ) (int, error) { return createReturnValues(pr, checker.DependencyUseTypeGoCommand, "go installs are pinned", + "no go installs found", dl) } func createReturnValues(pr map[checker.DependencyUseType]pinnedResult, - t checker.DependencyUseType, infoMsg string, + t checker.DependencyUseType, maxResultMsg string, inconclusiveResultMsg 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. + // If there are zero dependencies of this type, it will generate an inconclusive score, + // so we can disconsider it in the aggregated score. + // If all dependencies of this type are pinned, it will get a maximum score. + // If 1 or more dependencies of this type are unpinned, it will get a minimum score. //nolint - r, _ := pr[t] - switch r { - default: - return checker.InconclusiveResultScore, fmt.Errorf("%w: %v", errInvalidValue, r) - case pinned, pinnedUndefined: + r := pr[t] + + if r.total == 0 { dl.Info(&checker.LogMessage{ - Text: infoMsg, + Text: inconclusiveResultMsg, + }) + return checker.InconclusiveResultScore, nil + } else if r.total == r.pinned { + dl.Info(&checker.LogMessage{ + Text: maxResultMsg, }) return checker.MaxResultScore, nil - case notPinned: - // No logging needed as it's done by the checks. + } else { return checker.MinResultScore, nil } } @@ -305,29 +312,46 @@ func createReturnValues(pr map[checker.DependencyUseType]pinnedResult, func createReturnForIsGitHubActionsWorkflowPinned(wp worklowPinningResult, dl checker.DetailLogger) (int, error) { return createReturnValuesForGitHubActionsWorkflowPinned(wp, fmt.Sprintf("%ss are pinned", checker.DependencyUseTypeGHAction), + fmt.Sprintf("%ss found", checker.DependencyUseTypeGHAction), dl) } -func createReturnValuesForGitHubActionsWorkflowPinned(r worklowPinningResult, infoMsg string, - dl checker.DetailLogger, +func createReturnValuesForGitHubActionsWorkflowPinned(r worklowPinningResult, maxResultMsg string, + inconclusiveResultMsg string, dl checker.DetailLogger, ) (int, error) { + if r.gitHubOwned.total == 0 { + dl.Info(&checker.LogMessage{ + Text: fmt.Sprintf("%s %s", "no GitHub-owned", inconclusiveResultMsg), + }) + } + + if r.thirdParties.total == 0 { + dl.Info(&checker.LogMessage{ + Text: fmt.Sprintf("%s %s", "no Third-party", inconclusiveResultMsg), + }) + } + + if r.gitHubOwned.total == 0 && r.thirdParties.total == 0 { + return checker.InconclusiveResultScore, nil + } + score := checker.MinResultScore - if r.gitHubOwned != notPinned { + if r.gitHubOwned.total == r.gitHubOwned.pinned { score += 2 dl.Info(&checker.LogMessage{ Type: finding.FileTypeSource, Offset: checker.OffsetDefault, - Text: fmt.Sprintf("%s %s", "GitHub-owned", infoMsg), + Text: fmt.Sprintf("%s %s", "GitHub-owned", maxResultMsg), }) } - if r.thirdParties != notPinned { + if r.thirdParties.total == r.thirdParties.pinned { score += 8 dl.Info(&checker.LogMessage{ Type: finding.FileTypeSource, Offset: checker.OffsetDefault, - Text: fmt.Sprintf("%s %s", "Third-party", infoMsg), + Text: fmt.Sprintf("%s %s", "Third-party", maxResultMsg), }) } From 14f69b1b3e58f0962fa5da5ca1e693433bbfbbb8 Mon Sep 17 00:00:00 2001 From: Gabriela Gutierrez Date: Mon, 28 Aug 2023 19:12:54 +0000 Subject: [PATCH 06/46] feat: Score only applicable ecossystems Signed-off-by: Gabriela Gutierrez --- checks/evaluation/pinned_dependencies.go | 31 +++++++++--------------- 1 file changed, 12 insertions(+), 19 deletions(-) diff --git a/checks/evaluation/pinned_dependencies.go b/checks/evaluation/pinned_dependencies.go index 321fd21e080..bf18d170790 100644 --- a/checks/evaluation/pinned_dependencies.go +++ b/checks/evaluation/pinned_dependencies.go @@ -133,17 +133,18 @@ func PinningDependencies(name string, c *checker.CheckRequest, 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 no dependencies of an ecossystem are found, it results in an inconclusive score. + // We filter out inconclusive scores so only applicable ecossystems are considered in + // the final score. + scores := []int{actionScore, dockerFromScore, dockerDownloadScore, scriptScore, pipScore, npmScore, goScore} + conclusiveScores := []int{} + for i := range scores { + if scores[i] != checker.InconclusiveResultScore { + conclusiveScores = append(conclusiveScores, scores[i]) + } + } + + score := checker.AggregateScores(conclusiveScores...) if score == checker.MaxResultScore { return checker.CreateMaxScoreResult(name, "all dependencies are pinned") @@ -199,14 +200,6 @@ func generateOwnerToDisplay(gitHubOwned bool) string { return "third-party" } -// TODO(laurent): need to support GCB pinning. -func maxScore(s1, s2 int) int { - if s1 > s2 { - return s1 - } - return s2 -} - func addPinnedResult(rr *checker.Dependency, r *pinnedResult) { if *rr.Pinned { r.pinned += 1 From 6ff571bfd896e5fc6e76fd5da4f851efdd49aa0c Mon Sep 17 00:00:00 2001 From: Gabriela Gutierrez Date: Mon, 28 Aug 2023 21:55:12 +0000 Subject: [PATCH 07/46] feat: If no dependencies then create inconclusive score Signed-off-by: Gabriela Gutierrez --- checks/evaluation/pinned_dependencies.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/checks/evaluation/pinned_dependencies.go b/checks/evaluation/pinned_dependencies.go index bf18d170790..454798328fc 100644 --- a/checks/evaluation/pinned_dependencies.go +++ b/checks/evaluation/pinned_dependencies.go @@ -144,6 +144,10 @@ func PinningDependencies(name string, c *checker.CheckRequest, } } + if len(conclusiveScores) == 0 { + return checker.CreateInconclusiveResult(name, "no dependencies found") + } + score := checker.AggregateScores(conclusiveScores...) if score == checker.MaxResultScore { From 7b15634bce079450dfb0086698dd2cdab13bdd53 Mon Sep 17 00:00:00 2001 From: Gabriela Gutierrez Date: Tue, 29 Aug 2023 15:52:38 +0000 Subject: [PATCH 08/46] 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 --- checks/evaluation/pinned_dependencies_test.go | 160 +++++++++++++++--- 1 file changed, 139 insertions(+), 21 deletions(-) diff --git a/checks/evaluation/pinned_dependencies_test.go b/checks/evaluation/pinned_dependencies_test.go index 6d90d09d8b0..e128f621de5 100644 --- a/checks/evaluation/pinned_dependencies_test.go +++ b/checks/evaluation/pinned_dependencies_test.go @@ -23,64 +23,182 @@ import ( scut "github.com/ossf/scorecard/v4/utests" ) -func Test_createReturnValuesForGitHubActionsWorkflowPinned(t *testing.T) { +func Test_createReturnForIsGitHubActionsWorkflowPinned(t *testing.T) { t.Parallel() //nolint type args struct { - r worklowPinningResult - infoMsg string - dl checker.DetailLogger + r worklowPinningResult + dl *scut.TestDetailLogger + } + type want struct { + score int + logs []string } //nolint tests := []struct { name string args args - want int + want want }{ { - name: "both actions workflow pinned", + name: "GitHub-owned and Third-Party actions pinned", args: args{ r: worklowPinningResult{ - thirdParties: 1, - gitHubOwned: 1, + thirdParties: pinnedResult{ + pinned: 1, + total: 1, + }, + gitHubOwned: pinnedResult{ + pinned: 1, + total: 1, + }, }, dl: &scut.TestDetailLogger{}, }, - want: 10, + want: want{ + score: 10, + logs: []string{"GitHub-owned GitHubActions are pinned", "Third-party GitHubActions are pinned"}, + }, }, { - name: "github actions workflow pinned", + name: "only GitHub-owned actions pinned", args: args{ r: worklowPinningResult{ - thirdParties: 2, - gitHubOwned: 2, + thirdParties: pinnedResult{ + pinned: 0, + total: 1, + }, + gitHubOwned: pinnedResult{ + pinned: 1, + total: 1, + }, }, dl: &scut.TestDetailLogger{}, }, - want: 0, + want: want{ + score: 2, + logs: []string{"GitHub-owned GitHubActions are pinned"}, + }, }, { - name: "error in github actions workflow pinned", + name: "only Third-Party actions pinned", args: args{ r: worklowPinningResult{ - thirdParties: 2, - gitHubOwned: 2, + thirdParties: pinnedResult{ + pinned: 1, + total: 1, + }, + gitHubOwned: pinnedResult{ + pinned: 0, + total: 1, + }, }, dl: &scut.TestDetailLogger{}, }, - want: 0, + want: want{ + score: 8, + logs: []string{"Third-party GitHubActions are pinned"}, + }, + }, + { + name: "no GitHub actions pinned", + args: args{ + r: worklowPinningResult{ + thirdParties: pinnedResult{ + pinned: 0, + total: 1, + }, + gitHubOwned: pinnedResult{ + pinned: 0, + total: 1, + }, + }, + dl: &scut.TestDetailLogger{}, + }, + want: want{ + score: 0, + logs: []string{}, + }, + }, + { + name: "no GitHub actions", + args: args{ + r: worklowPinningResult{ + thirdParties: pinnedResult{ + pinned: 0, + total: 0, + }, + gitHubOwned: pinnedResult{ + pinned: 0, + total: 0, + }, + }, + dl: &scut.TestDetailLogger{}, + }, + want: want{ + score: -1, + logs: []string{"no GitHub-owned GitHubActions found", "no Third-party GitHubActions found"}, + }, + }, + { + name: "no GitHub-owned actions", + args: args{ + r: worklowPinningResult{ + thirdParties: pinnedResult{ + pinned: 0, + total: 1, + }, + gitHubOwned: pinnedResult{ + pinned: 0, + total: 0, + }, + }, + dl: &scut.TestDetailLogger{}, + }, + want: want{ + score: 2, + logs: []string{"no GitHub-owned GitHubActions found"}, + }, + }, + { + name: "no Third-party actions", + args: args{ + r: worklowPinningResult{ + thirdParties: pinnedResult{ + pinned: 0, + total: 0, + }, + gitHubOwned: pinnedResult{ + pinned: 0, + total: 1, + }, + }, + dl: &scut.TestDetailLogger{}, + }, + want: want{ + score: 8, + logs: []string{"no Third-party GitHubActions found"}, + }, }, } 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) + got, err := createReturnForIsGitHubActionsWorkflowPinned(tt.args.r, tt.args.dl) if err != nil { - t.Errorf("error during createReturnValuesForGitHubActionsWorkflowPinned: %v", err) + t.Errorf("error during createReturnForIsGitHubActionsWorkflowPinned: %v", err) } - if got != tt.want { - t.Errorf("createReturnValuesForGitHubActionsWorkflowPinned() = %v, want %v", got, tt.want) + if got != tt.want.score { + t.Errorf("createReturnForIsGitHubActionsWorkflowPinned() = %v, want %v", got, tt.want.score) + } + for _, log := range tt.want.logs { + isExpectedLog := func(logMessage checker.LogMessage, logType checker.DetailType) bool { + return logMessage.Text == log && logType == checker.DetailInfo + } + if !scut.ValidateLogMessage(isExpectedLog, tt.args.dl) { + t.Errorf("test failed: log message not present: %+v", log) + } } }) } From 104f9693a4843dfbf8f1dcb5cc7fde91b1f60abc Mon Sep 17 00:00:00 2001 From: Gabriela Gutierrez Date: Tue, 29 Aug 2023 16:37:38 +0000 Subject: [PATCH 09/46] 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 --- checks/evaluation/pinned_dependencies_test.go | 293 +++++++++++++++--- 1 file changed, 250 insertions(+), 43 deletions(-) diff --git a/checks/evaluation/pinned_dependencies_test.go b/checks/evaluation/pinned_dependencies_test.go index e128f621de5..7009ab2a175 100644 --- a/checks/evaluation/pinned_dependencies_test.go +++ b/checks/evaluation/pinned_dependencies_test.go @@ -208,6 +208,10 @@ func asStringPointer(s string) *string { return &s } +func asBoolPointer(b bool) *bool { + return &b +} + func Test_PinningDependencies(t *testing.T) { t.Parallel() @@ -217,94 +221,119 @@ 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{}, - Msg: asStringPointer("some message"), - Type: checker.DependencyUseTypeDownloadThenRun, + Type: checker.DependencyUseTypePipCommand, + Pinned: asBoolPointer(true), }, }, expected: scut.TestReturn{ Error: nil, - Score: checker.MaxResultScore, + Score: 10, NumberOfWarn: 0, NumberOfInfo: 8, - NumberOfDebug: 1, + NumberOfDebug: 0, }, }, { - name: "download then run pinned debug and warn", + name: "all dependencies unpinned", dependencies: []checker.Dependency{ { Location: &checker.File{}, - Msg: asStringPointer("some message"), - Type: checker.DependencyUseTypeDownloadThenRun, - }, - { - Location: &checker.File{}, - Type: checker.DependencyUseTypeDownloadThenRun, + Type: checker.DependencyUseTypePipCommand, + Pinned: asBoolPointer(false), }, }, expected: scut.TestReturn{ Error: nil, - Score: 7, + Score: 0, NumberOfWarn: 1, - NumberOfInfo: 6, - NumberOfDebug: 1, + NumberOfInfo: 7, + NumberOfDebug: 0, }, }, { - name: "various warnings", + name: "1 ecossystem pinned and 1 ecossystem 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: 7, + NumberOfDebug: 0, + }, + }, + { + name: "1 ecossystem partially pinned", + dependencies: []checker.Dependency{ { Location: &checker.File{}, - Type: checker.DependencyUseTypeDockerfileContainerImage, + Type: checker.DependencyUseTypePipCommand, + Pinned: asBoolPointer(false), }, { Location: &checker.File{}, - Msg: asStringPointer("debug message"), + Type: checker.DependencyUseTypePipCommand, + Pinned: asBoolPointer(true), }, }, expected: scut.TestReturn{ Error: nil, - Score: 4, - NumberOfWarn: 3, - NumberOfInfo: 4, - NumberOfDebug: 1, + Score: 0, + NumberOfWarn: 1, + NumberOfInfo: 7, + NumberOfDebug: 0, }, }, { - name: "unpinned pip install", + name: "no dependencies found", + dependencies: []checker.Dependency{}, + expected: scut.TestReturn{ + Error: nil, + Score: -1, + NumberOfWarn: 0, + NumberOfInfo: 8, + 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, - NumberOfWarn: 1, - NumberOfInfo: 7, + Score: 10, + NumberOfWarn: 0, + NumberOfInfo: 8, NumberOfDebug: 0, }, }, { - name: "undefined pip install", + name: "pinned dependency with debug message", dependencies: []checker.Dependency{ { Location: &checker.File{}, + Msg: asStringPointer("some message"), Type: checker.DependencyUseTypePipCommand, - Msg: asStringPointer("debug message"), + Pinned: asBoolPointer(true), }, }, expected: scut.TestReturn{ @@ -316,41 +345,155 @@ func Test_PinningDependencies(t *testing.T) { }, }, { - name: "all dependencies pinned", + name: "unpinned dependency shows warn message", + dependencies: []checker.Dependency{ + { + Location: &checker.File{}, + Type: checker.DependencyUseTypePipCommand, + Pinned: asBoolPointer(false), + }, + }, expected: scut.TestReturn{ Error: nil, - Score: 10, - NumberOfWarn: 0, - NumberOfInfo: 8, + Score: 0, + NumberOfWarn: 1, + NumberOfInfo: 7, NumberOfDebug: 0, }, }, { - name: "Validate various warnings and info", + name: "unpinned dependency with debug message shows no warn message", dependencies: []checker.Dependency{ { Location: &checker.File{}, + Msg: asStringPointer("some message"), Type: checker.DependencyUseTypePipCommand, + Pinned: asBoolPointer(false), }, + }, + expected: scut.TestReturn{ + Error: nil, + Score: 0, + NumberOfWarn: 0, + NumberOfInfo: 7, + NumberOfDebug: 1, + }, + }, + { + name: "unpinned dependency shows warn message", + dependencies: []checker.Dependency{ { Location: &checker.File{}, + Msg: asStringPointer("some message"), Type: checker.DependencyUseTypeDownloadThenRun, + Pinned: asBoolPointer(false), }, + { + Location: &checker.File{}, + Type: checker.DependencyUseTypeDownloadThenRun, + Pinned: asBoolPointer(false), + }, + }, + expected: scut.TestReturn{ + Error: nil, + Score: 0, + NumberOfWarn: 1, + NumberOfInfo: 6, + NumberOfDebug: 1, + }, + }, + // TODO: choco installs should score for Pinned-Dependencies + // { + // name: "unpinned choco install", + // dependencies: []checker.Dependency{ + // { + // Location: &checker.File{}, + // Type: checker.DependencyUseTypeChocoCommand, + // Pinned: asBoolPointer(false), + // }, + // }, + // expected: scut.TestReturn{ + // Error: nil, + // Score: 0, + // NumberOfWarn: 1, + // NumberOfInfo: 7, + // NumberOfDebug: 0, + // }, + // }, + { + name: "unpinned Dockerfile container image", + dependencies: []checker.Dependency{ { Location: &checker.File{}, Type: checker.DependencyUseTypeDockerfileContainerImage, + Pinned: asBoolPointer(false), }, + }, + expected: scut.TestReturn{ + Error: nil, + Score: 0, + NumberOfWarn: 1, + NumberOfInfo: 7, + NumberOfDebug: 0, + }, + }, + // TODO: Due to a bug download then run is score twice in shell scripts + // and Dockerfile, the NumberOfInfo should be 7 + { + name: "unpinned download then run in Dockerfile", + dependencies: []checker.Dependency{ + { + Location: &checker.File{ + Path: "Dockerfile", + }, + Type: checker.DependencyUseTypeDownloadThenRun, + Pinned: asBoolPointer(false), + }, + }, + expected: scut.TestReturn{ + Error: nil, + Score: 0, + NumberOfWarn: 1, + NumberOfInfo: 6, + NumberOfDebug: 0, + }, + }, + // TODO: Due to a bug download then run is score twice in shell scripts + // and Dockerfile, the NumberOfInfo should be 7 + { + name: "unpinned download then run in shell scripts", + dependencies: []checker.Dependency{ + { + Location: &checker.File{ + Path: "bash.sh", + }, + Type: checker.DependencyUseTypeDownloadThenRun, + Pinned: asBoolPointer(false), + }, + }, + expected: scut.TestReturn{ + Error: nil, + Score: 0, + NumberOfWarn: 1, + NumberOfInfo: 6, + NumberOfDebug: 0, + }, + }, + { + name: "unpinned go install", + dependencies: []checker.Dependency{ { Location: &checker.File{}, - Msg: asStringPointer("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: 7, + NumberOfDebug: 0, }, }, { @@ -359,32 +502,96 @@ 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, NumberOfDebug: 0, }, }, + // TODO: nuget installs should score for Pinned-Dependencies + // { + // name: "unpinned nuget install", + // dependencies: []checker.Dependency{ + // { + // Location: &checker.File{}, + // Type: checker.DependencyUseTypeNugetCommand, + // Pinned: asBoolPointer(false), + // }, + // }, + // expected: scut.TestReturn{ + // Error: nil, + // Score: 0, + // NumberOfWarn: 1, + // NumberOfInfo: 7, + // NumberOfDebug: 0, + // }, + // }, { - name: "unpinned go install", + name: "unpinned pip install", dependencies: []checker.Dependency{ { Location: &checker.File{}, - Type: checker.DependencyUseTypeGoCommand, + Type: checker.DependencyUseTypePipCommand, + Pinned: asBoolPointer(false), }, }, expected: scut.TestReturn{ Error: nil, - Score: 8, + Score: 0, NumberOfWarn: 1, NumberOfInfo: 7, NumberOfDebug: 0, }, }, + { + name: "2 unpinned dependencies for 1 ecossystem shows 2 warn messages", + dependencies: []checker.Dependency{ + { + Location: &checker.File{}, + Type: checker.DependencyUseTypePipCommand, + Pinned: asBoolPointer(false), + }, + { + Location: &checker.File{}, + Type: checker.DependencyUseTypePipCommand, + Pinned: asBoolPointer(false), + }, + }, + expected: scut.TestReturn{ + Error: nil, + Score: 0, + NumberOfWarn: 2, + NumberOfInfo: 7, + NumberOfDebug: 0, + }, + }, + { + name: "2 unpinned dependencies for 2 ecossystems 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), + }, + }, + expected: scut.TestReturn{ + Error: nil, + Score: 0, + NumberOfWarn: 2, + NumberOfInfo: 6, + NumberOfDebug: 0, + }, + }, } for _, tt := range tests { From 5f67732eb61af84da8b66e6b14953f63b53fe338 Mon Sep 17 00:00:00 2001 From: Gabriela Gutierrez Date: Tue, 29 Aug 2023 16:48:25 +0000 Subject: [PATCH 10/46] test: Ecossystems score and logs Signed-off-by: Gabriela Gutierrez --- checks/evaluation/pinned_dependencies_test.go | 63 ++++++++++++------- 1 file changed, 39 insertions(+), 24 deletions(-) diff --git a/checks/evaluation/pinned_dependencies_test.go b/checks/evaluation/pinned_dependencies_test.go index 7009ab2a175..8928042f824 100644 --- a/checks/evaluation/pinned_dependencies_test.go +++ b/checks/evaluation/pinned_dependencies_test.go @@ -615,56 +615,63 @@ func Test_PinningDependencies(t *testing.T) { 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", + name: "no dependencies", args: args{ - t: checker.DependencyUseTypeDownloadThenRun, + t: checker.DependencyUseTypePipCommand, dl: &scut.TestDetailLogger{}, }, - want: 10, + want: -1, }, { - name: "returns 10 if pinned undefined", + name: "all dependencies pinned", args: args{ - t: checker.DependencyUseTypeDownloadThenRun, + t: checker.DependencyUseTypePipCommand, + dl: &scut.TestDetailLogger{}, pr: map[checker.DependencyUseType]pinnedResult{ - checker.DependencyUseTypeDownloadThenRun: pinnedUndefined, + checker.DependencyUseTypePipCommand: { + pinned: 1, + total: 1, + }, }, - dl: &scut.TestDetailLogger{}, }, want: 10, }, { - name: "returns 10 if pinned", + name: "all dependencies unpinned", args: args{ - t: checker.DependencyUseTypeDownloadThenRun, + t: checker.DependencyUseTypePipCommand, + dl: &scut.TestDetailLogger{}, pr: map[checker.DependencyUseType]pinnedResult{ - checker.DependencyUseTypeDownloadThenRun: pinned, + checker.DependencyUseTypePipCommand: { + pinned: 0, + total: 1, }, - dl: &scut.TestDetailLogger{}, }, - want: 10, + }, + want: 0, }, { - name: "returns 0 if unpinned", + name: "1 or more dependencies unpinned", args: args{ - t: checker.DependencyUseTypeDownloadThenRun, + t: checker.DependencyUseTypePipCommand, + dl: &scut.TestDetailLogger{}, pr: map[checker.DependencyUseType]pinnedResult{ - checker.DependencyUseTypeDownloadThenRun: notPinned, + checker.DependencyUseTypePipCommand: { + pinned: 1, + total: 2, + }, }, - dl: &scut.TestDetailLogger{}, }, want: 0, }, @@ -673,7 +680,7 @@ func Test_createReturnValues(t *testing.T) { 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) + got, err := createReturnValues(tt.args.pr, tt.args.t, "all dependencies are pinned", "no dependencies found", tt.args.dl) if err != nil { t.Errorf("error during createReturnValues: %v", err) } @@ -681,15 +688,23 @@ func Test_createReturnValues(t *testing.T) { t.Errorf("createReturnValues() = %v, want %v", got, tt.want) } - if tt.want < 10 { - return + switch tt.want { + case -1: + isExpectedLog := func(logMessage checker.LogMessage, logType checker.DetailType) bool { + return logMessage.Text == "no dependencies found" && logType == checker.DetailInfo } - + if !scut.ValidateLogMessage(isExpectedLog, tt.args.dl) { + t.Errorf("test failed: log message not present: %+v", "no dependencies found") + } + case 0: + return + case 10: isExpectedLog := func(logMessage checker.LogMessage, logType checker.DetailType) bool { - return logMessage.Text == "some message" && logType == checker.DetailInfo + return logMessage.Text == "all dependencies are pinned" && logType == checker.DetailInfo } if !scut.ValidateLogMessage(isExpectedLog, tt.args.dl) { - t.Errorf("test failed: log message not present: %+v", "some message") + t.Errorf("test failed: log message not present: %+v", "all dependencies are pinned") + } } }) } From 5691e0d6de302c3f7b75e831f3e071a8ad73cab8 Mon Sep 17 00:00:00 2001 From: Gabriela Gutierrez Date: Tue, 29 Aug 2023 16:51:09 +0000 Subject: [PATCH 11/46] 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 --- checks/evaluation/pinned_dependencies_test.go | 47 ------------------- 1 file changed, 47 deletions(-) diff --git a/checks/evaluation/pinned_dependencies_test.go b/checks/evaluation/pinned_dependencies_test.go index 8928042f824..f2523a0be86 100644 --- a/checks/evaluation/pinned_dependencies_test.go +++ b/checks/evaluation/pinned_dependencies_test.go @@ -710,53 +710,6 @@ func Test_createReturnValues(t *testing.T) { } } -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, - }, - want: 10, - }, - { - name: "returns s2 if s2 is greater than s1", - args: args{ - s1: 5, - s2: 10, - }, - want: 10, - }, - { - name: "returns s1 if s1 is equal to s2", - args: args{ - s1: 10, - s2: 10, - }, - 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) - } - }) - } -} - func Test_generateOwnerToDisplay(t *testing.T) { t.Parallel() tests := []struct { //nolint:govet From 84757eadb02fadbd326db052f14a6fe665e197aa Mon Sep 17 00:00:00 2001 From: Gabriela Gutierrez Date: Tue, 29 Aug 2023 17:00:27 +0000 Subject: [PATCH 12/46] test: Adding GitHub Actions dependencies to result Signed-off-by: Gabriela Gutierrez --- checks/evaluation/pinned_dependencies_test.go | 110 ++++++++++++++---- 1 file changed, 85 insertions(+), 25 deletions(-) diff --git a/checks/evaluation/pinned_dependencies_test.go b/checks/evaluation/pinned_dependencies_test.go index f2523a0be86..c6c46446c88 100644 --- a/checks/evaluation/pinned_dependencies_test.go +++ b/checks/evaluation/pinned_dependencies_test.go @@ -275,7 +275,7 @@ func Test_PinningDependencies(t *testing.T) { NumberOfInfo: 7, NumberOfDebug: 0, }, - }, + }, { name: "1 ecossystem partially pinned", dependencies: []checker.Dependency{ @@ -378,7 +378,7 @@ func Test_PinningDependencies(t *testing.T) { NumberOfInfo: 7, NumberOfDebug: 1, }, - }, + }, { name: "unpinned dependency shows warn message", dependencies: []checker.Dependency{ @@ -642,7 +642,7 @@ func Test_createReturnValues(t *testing.T) { checker.DependencyUseTypePipCommand: { pinned: 1, total: 1, - }, + }, }, }, want: 10, @@ -656,9 +656,9 @@ func Test_createReturnValues(t *testing.T) { checker.DependencyUseTypePipCommand: { pinned: 0, total: 1, + }, }, }, - }, want: 0, }, { @@ -670,7 +670,7 @@ func Test_createReturnValues(t *testing.T) { checker.DependencyUseTypePipCommand: { pinned: 1, total: 2, - }, + }, }, }, want: 0, @@ -692,17 +692,17 @@ func Test_createReturnValues(t *testing.T) { case -1: isExpectedLog := func(logMessage checker.LogMessage, logType checker.DetailType) bool { return logMessage.Text == "no dependencies found" && logType == checker.DetailInfo - } + } if !scut.ValidateLogMessage(isExpectedLog, tt.args.dl) { t.Errorf("test failed: log message not present: %+v", "no dependencies found") } case 0: return case 10: - isExpectedLog := func(logMessage checker.LogMessage, logType checker.DetailType) bool { + isExpectedLog := func(logMessage checker.LogMessage, logType checker.DetailType) bool { return logMessage.Text == "all dependencies are pinned" && logType == checker.DetailInfo - } - if !scut.ValidateLogMessage(isExpectedLog, tt.args.dl) { + } + if !scut.ValidateLogMessage(isExpectedLog, tt.args.dl) { t.Errorf("test failed: log message not present: %+v", "all dependencies are pinned") } } @@ -742,50 +742,110 @@ 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)) } }) } From 5509c476f3f00ddaf25c9e0df55ff9b1ac9f5793 Mon Sep 17 00:00:00 2001 From: Gabriela Gutierrez Date: Tue, 29 Aug 2023 17:00:35 +0000 Subject: [PATCH 13/46] test: Update GitHub Actions result Signed-off-by: Gabriela Gutierrez --- checks/evaluation/pinned_dependencies_test.go | 148 ++++++++++++++---- 1 file changed, 120 insertions(+), 28 deletions(-) diff --git a/checks/evaluation/pinned_dependencies_test.go b/checks/evaluation/pinned_dependencies_test.go index c6c46446c88..a30889a3a5d 100644 --- a/checks/evaluation/pinned_dependencies_test.go +++ b/checks/evaluation/pinned_dependencies_test.go @@ -18,7 +18,6 @@ import ( "testing" "github.com/google/go-cmp/cmp" - "github.com/ossf/scorecard/v4/checker" scut "github.com/ossf/scorecard/v4/utests" ) @@ -891,50 +890,143 @@ 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), }, - 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)) } }) } From 9a787c44866eee236795893f8d094d70053cfab2 Mon Sep 17 00:00:00 2001 From: Gabriela Gutierrez Date: Tue, 29 Aug 2023 18:38:07 +0000 Subject: [PATCH 14/46] test: Update pip installs result Signed-off-by: Gabriela Gutierrez --- checks/evaluation/pinned_dependencies_test.go | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/checks/evaluation/pinned_dependencies_test.go b/checks/evaluation/pinned_dependencies_test.go index a30889a3a5d..e5cc23ca969 100644 --- a/checks/evaluation/pinned_dependencies_test.go +++ b/checks/evaluation/pinned_dependencies_test.go @@ -1012,6 +1012,46 @@ func TestUpdatePinningResults(t *testing.T) { 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, + }, + }, + }, + }, } for _, tc := range tests { tc := tc @@ -1028,6 +1068,14 @@ func TestUpdatePinningResults(t *testing.T) { 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)) + } + } }) } } From 423cac868fbfba787948f7911ec6bf027963a6a5 Mon Sep 17 00:00:00 2001 From: Gabriela Gutierrez Date: Tue, 18 Apr 2023 10:43:18 -0300 Subject: [PATCH 15/46] fix: Handle if nuget dependency is pinned or unpinned Signed-off-by: Gabriela Gutierrez --- checks/raw/shell_download_validate.go | 47 +++++++++++---------------- 1 file changed, 19 insertions(+), 28 deletions(-) diff --git a/checks/raw/shell_download_validate.go b/checks/raw/shell_download_validate.go index 8e53f8e9887..fe08b1ac895 100644 --- a/checks/raw/shell_download_validate.go +++ b/checks/raw/shell_download_validate.go @@ -680,22 +680,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 { @@ -723,26 +718,17 @@ 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 @@ -755,12 +741,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 } @@ -863,7 +853,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{ @@ -873,6 +863,7 @@ func collectUnpinnedPakageManagerDownload(startLine, endLine uint, node syntax.N EndOffset: endLine, Snippet: cmd, }, + Pinned: asBoolPointer(!isNugetUnpinnedDownload(c)), Type: checker.DependencyUseTypeNugetCommand, }, ) From 9056875b48a66d1e5940ce421a7b44860264d719 Mon Sep 17 00:00:00 2001 From: Gabriela Gutierrez Date: Tue, 29 Aug 2023 21:30:10 +0000 Subject: [PATCH 16/46] tests: Fix check warnings for unpinned dependencies Signed-off-by: Gabriela Gutierrez --- checks/raw/pinned_dependencies_test.go | 74 ++++++++++++++++++-------- 1 file changed, 53 insertions(+), 21 deletions(-) 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 +} From c1a81fe1f3fa83d591129b89e5f8e1c7ffc3ff6a Mon Sep 17 00:00:00 2001 From: Gabriela Gutierrez Date: Tue, 29 Aug 2023 21:58:56 +0000 Subject: [PATCH 17/46] fix: Linter errors Signed-off-by: Gabriela Gutierrez --- checker/raw_result.go | 2 +- checks/evaluation/pinned_dependencies.go | 9 +++++---- checks/evaluation/pinned_dependencies_test.go | 18 ++++++++++++------ checks/raw/fuzzing.go | 4 +++- checks/raw/shell_download_validate.go | 6 ++++-- 5 files changed, 25 insertions(+), 14 deletions(-) diff --git a/checker/raw_result.go b/checker/raw_result.go index 8f63dc1fa59..e8834afe0e8 100644 --- a/checker/raw_result.go +++ b/checker/raw_result.go @@ -121,8 +121,8 @@ type Dependency struct { PinnedAt *string Location *File Msg *string // Only for debug messages. - Type DependencyUseType Pinned *bool + Type DependencyUseType } // MaintainedData contains the raw results diff --git a/checks/evaluation/pinned_dependencies.go b/checks/evaluation/pinned_dependencies.go index 454798328fc..2805ddd594e 100644 --- a/checks/evaluation/pinned_dependencies.go +++ b/checks/evaluation/pinned_dependencies.go @@ -181,7 +181,7 @@ func updatePinningResults(rr *checker.Dependency, } // Update other result types. - var p pinnedResult = pr[rr.Type] + p := pr[rr.Type] addPinnedResult(rr, &p) pr[rr.Type] = p } @@ -290,17 +290,18 @@ func createReturnValues(pr map[checker.DependencyUseType]pinnedResult, //nolint r := pr[t] - if r.total == 0 { + switch r.total { + case 0: dl.Info(&checker.LogMessage{ Text: inconclusiveResultMsg, }) return checker.InconclusiveResultScore, nil - } else if r.total == r.pinned { + case r.pinned: dl.Info(&checker.LogMessage{ Text: maxResultMsg, }) return checker.MaxResultScore, nil - } else { + default: return checker.MinResultScore, nil } } diff --git a/checks/evaluation/pinned_dependencies_test.go b/checks/evaluation/pinned_dependencies_test.go index e5cc23ca969..bb1446536b1 100644 --- a/checks/evaluation/pinned_dependencies_test.go +++ b/checks/evaluation/pinned_dependencies_test.go @@ -18,6 +18,7 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "github.com/ossf/scorecard/v4/checker" scut "github.com/ossf/scorecard/v4/utests" ) @@ -30,8 +31,8 @@ func Test_createReturnForIsGitHubActionsWorkflowPinned(t *testing.T) { dl *scut.TestDetailLogger } type want struct { - score int logs []string + score int } //nolint tests := []struct { @@ -679,7 +680,8 @@ func Test_createReturnValues(t *testing.T) { tt := tt t.Run(tt.name, func(t *testing.T) { t.Parallel() - got, err := createReturnValues(tt.args.pr, tt.args.t, "all dependencies are pinned", "no dependencies found", tt.args.dl) + got, err := createReturnValues(tt.args.pr, tt.args.t, "all dependencies are pinned", + "no dependencies found", tt.args.dl) if err != nil { t.Errorf("error during createReturnValues: %v", err) } @@ -837,12 +839,14 @@ func Test_addWorkflowPinnedResult(t *testing.T) { t.Parallel() 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", + 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", + 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)) } @@ -1059,12 +1063,14 @@ func TestUpdatePinningResults(t *testing.T) { t.Parallel() 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", + 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", + 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)) } diff --git a/checks/raw/fuzzing.go b/checks/raw/fuzzing.go index b5c9f0d910e..a53b70e44c4 100644 --- a/checks/raw/fuzzing.go +++ b/checks/raw/fuzzing.go @@ -143,7 +143,9 @@ func Fuzzing(c *checker.CheckRequest) (checker.FuzzingData, error) { checker.Tool{ Name: oneFuzz, URL: asStringPointer("https://github.com/microsoft/onefuzz"), - Desc: asStringPointer("Enables continuous developer-driven fuzzing to proactively harden software prior to release."), + Desc: asStringPointer( + "Enables continuous developer-driven fuzzing to proactively harden software prior to release.", + ), // TODO: File. }, ) diff --git a/checks/raw/shell_download_validate.go b/checks/raw/shell_download_validate.go index fe08b1ac895..b809b403e7b 100644 --- a/checks/raw/shell_download_validate.go +++ b/checks/raw/shell_download_validate.go @@ -725,7 +725,9 @@ func isDotNetCliInstall(cmd []string) bool { } // 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")) + 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 { @@ -864,7 +866,7 @@ func collectUnpinnedPakageManagerDownload(startLine, endLine uint, node syntax.N Snippet: cmd, }, Pinned: asBoolPointer(!isNugetUnpinnedDownload(c)), - Type: checker.DependencyUseTypeNugetCommand, + Type: checker.DependencyUseTypeNugetCommand, }, ) From c8ee14a01131a7aeeb20eb53a1b062f30f663cfb Mon Sep 17 00:00:00 2001 From: Gabriela Gutierrez Date: Tue, 29 Aug 2023 22:35:38 +0000 Subject: [PATCH 18/46] 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 --- checks/evaluation/pinned_dependencies.go | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/checks/evaluation/pinned_dependencies.go b/checks/evaluation/pinned_dependencies.go index 2805ddd594e..977f6c1f5c3 100644 --- a/checks/evaluation/pinned_dependencies.go +++ b/checks/evaluation/pinned_dependencies.go @@ -337,20 +337,24 @@ func createReturnValuesForGitHubActionsWorkflowPinned(r worklowPinningResult, ma if r.gitHubOwned.total == r.gitHubOwned.pinned { score += 2 - dl.Info(&checker.LogMessage{ - Type: finding.FileTypeSource, - Offset: checker.OffsetDefault, - Text: fmt.Sprintf("%s %s", "GitHub-owned", maxResultMsg), - }) + if r.gitHubOwned.total != 0 { + dl.Info(&checker.LogMessage{ + Type: finding.FileTypeSource, + Offset: checker.OffsetDefault, + Text: fmt.Sprintf("%s %s", "GitHub-owned", maxResultMsg), + }) + } } if r.thirdParties.total == r.thirdParties.pinned { score += 8 - dl.Info(&checker.LogMessage{ - Type: finding.FileTypeSource, - Offset: checker.OffsetDefault, - Text: fmt.Sprintf("%s %s", "Third-party", maxResultMsg), - }) + if r.thirdParties.total != 0 { + dl.Info(&checker.LogMessage{ + Type: finding.FileTypeSource, + Offset: checker.OffsetDefault, + Text: fmt.Sprintf("%s %s", "Third-party", maxResultMsg), + }) + } } return score, nil From 7f7b89decef6486c8bdb6714b6c7ec8d9d4d6b50 Mon Sep 17 00:00:00 2001 From: Gabriela Gutierrez Date: Tue, 29 Aug 2023 22:59:34 +0000 Subject: [PATCH 19/46] 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 --- e2e/pinned_dependencies_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/e2e/pinned_dependencies_test.go b/e2e/pinned_dependencies_test.go index c49357d337e..d736bdd9419 100644 --- a/e2e/pinned_dependencies_test.go +++ b/e2e/pinned_dependencies_test.go @@ -49,7 +49,7 @@ var _ = Describe("E2E TEST:"+checks.CheckPinnedDependencies, func() { } expected := scut.TestReturn{ Error: nil, - Score: 4, + Score: 3, NumberOfWarn: 139, NumberOfInfo: 3, NumberOfDebug: 0, @@ -74,7 +74,7 @@ var _ = Describe("E2E TEST:"+checks.CheckPinnedDependencies, func() { } expected := scut.TestReturn{ Error: nil, - Score: 4, + Score: 3, NumberOfWarn: 139, NumberOfInfo: 3, NumberOfDebug: 0, @@ -110,7 +110,7 @@ var _ = Describe("E2E TEST:"+checks.CheckPinnedDependencies, func() { } expected := scut.TestReturn{ Error: nil, - Score: 4, + Score: 3, NumberOfWarn: 139, NumberOfInfo: 3, NumberOfDebug: 0, From a1b178152a8830341b6dbbe13eeca0060f5a4ea7 Mon Sep 17 00:00:00 2001 From: Gabriela Gutierrez Date: Thu, 31 Aug 2023 12:31:25 +0000 Subject: [PATCH 20/46] Revert rename `asPointer` to `asStringPointer` Signed-off-by: Gabriela Gutierrez --- attestor/policy/attestation_policy_test.go | 12 ++-- checks/evaluation/pinned_dependencies_test.go | 8 +-- checks/raw/dependency_update_tool.go | 22 +++---- checks/raw/fuzzing.go | 24 ++++--- checks/raw/pinned_dependencies.go | 14 ++-- cron/worker/worker_test.go | 4 +- dependencydiff/dependencydiff_test.go | 14 ++-- dependencydiff/mapping.go | 6 +- pkg/json_raw_results.go | 10 +-- pkg/json_raw_results_test.go | 64 +++++++++---------- remediation/remediations_test.go | 14 ++-- 11 files changed, 95 insertions(+), 97 deletions(-) diff --git a/attestor/policy/attestation_policy_test.go b/attestor/policy/attestation_policy_test.go index 5e8f4d54096..61e222b906d 100644 --- a/attestor/policy/attestation_policy_test.go +++ b/attestor/policy/attestation_policy_test.go @@ -379,7 +379,7 @@ func TestCheckCodeReviewed(t *testing.T) { } } -func asStringPointer(s string) *string { +func asPointer(s string) *string { return &s } @@ -399,7 +399,7 @@ func TestNoUnpinnedDependencies(t *testing.T) { raw: &checker.RawResults{ PinningDependenciesResults: checker.PinningDependenciesData{ Dependencies: []checker.Dependency{ - {Name: asStringPointer("foo"), PinnedAt: asStringPointer("abcdef")}, + {Name: asPointer("foo"), PinnedAt: asPointer("abcdef")}, }, }, }, @@ -413,7 +413,7 @@ func TestNoUnpinnedDependencies(t *testing.T) { raw: &checker.RawResults{ PinningDependenciesResults: checker.PinningDependenciesData{ Dependencies: []checker.Dependency{ - {Name: asStringPointer("foo"), PinnedAt: nil}, + {Name: asPointer("foo"), PinnedAt: nil}, }, }, }, @@ -427,7 +427,7 @@ func TestNoUnpinnedDependencies(t *testing.T) { raw: &checker.RawResults{ PinningDependenciesResults: checker.PinningDependenciesData{ Dependencies: []checker.Dependency{ - {Name: asStringPointer("foo"), PinnedAt: nil}, + {Name: asPointer("foo"), PinnedAt: nil}, }, }, }, @@ -439,7 +439,7 @@ func TestNoUnpinnedDependencies(t *testing.T) { raw: &checker.RawResults{ PinningDependenciesResults: checker.PinningDependenciesData{ Dependencies: []checker.Dependency{ - {Name: asStringPointer("second-pkg"), Location: &checker.File{Path: "bar"}, PinnedAt: nil}, + {Name: asPointer("second-pkg"), Location: &checker.File{Path: "bar"}, PinnedAt: nil}, }, }, }, @@ -651,7 +651,7 @@ func TestAttestationPolicy_EvaluateResults(t *testing.T) { raw: &checker.RawResults{ PinningDependenciesResults: checker.PinningDependenciesData{ Dependencies: []checker.Dependency{ - {Name: asStringPointer("foo"), PinnedAt: asStringPointer("abcdef")}, + {Name: asPointer("foo"), PinnedAt: asPointer("abcdef")}, }, }, }, diff --git a/checks/evaluation/pinned_dependencies_test.go b/checks/evaluation/pinned_dependencies_test.go index bb1446536b1..d7455e64822 100644 --- a/checks/evaluation/pinned_dependencies_test.go +++ b/checks/evaluation/pinned_dependencies_test.go @@ -204,7 +204,7 @@ func Test_createReturnForIsGitHubActionsWorkflowPinned(t *testing.T) { } } -func asStringPointer(s string) *string { +func asPointer(s string) *string { return &s } @@ -331,7 +331,7 @@ func Test_PinningDependencies(t *testing.T) { dependencies: []checker.Dependency{ { Location: &checker.File{}, - Msg: asStringPointer("some message"), + Msg: asPointer("some message"), Type: checker.DependencyUseTypePipCommand, Pinned: asBoolPointer(true), }, @@ -366,7 +366,7 @@ func Test_PinningDependencies(t *testing.T) { dependencies: []checker.Dependency{ { Location: &checker.File{}, - Msg: asStringPointer("some message"), + Msg: asPointer("some message"), Type: checker.DependencyUseTypePipCommand, Pinned: asBoolPointer(false), }, @@ -384,7 +384,7 @@ func Test_PinningDependencies(t *testing.T) { dependencies: []checker.Dependency{ { Location: &checker.File{}, - Msg: asStringPointer("some message"), + Msg: asPointer("some message"), Type: checker.DependencyUseTypeDownloadThenRun, Pinned: asBoolPointer(false), }, diff --git a/checks/raw/dependency_update_tool.go b/checks/raw/dependency_update_tool.go index a6ee391a25b..2e2244c1b8b 100644 --- a/checks/raw/dependency_update_tool.go +++ b/checks/raw/dependency_update_tool.go @@ -49,8 +49,8 @@ func DependencyUpdateTool(c clients.RepoClient) (checker.DependencyUpdateToolDat if commits[i].Committer.ID == dependabotID { tools = append(tools, checker.Tool{ Name: "Dependabot", - URL: asStringPointer("https://github.com/dependabot"), - Desc: asStringPointer("Automated dependency updates built into GitHub"), + URL: asPointer("https://github.com/dependabot"), + Desc: asPointer("Automated dependency updates built into GitHub"), Files: []checker.File{{}}, }) break @@ -74,8 +74,8 @@ var checkDependencyFileExists fileparser.DoWhileTrueOnFilename = func(name strin case ".github/dependabot.yml", ".github/dependabot.yaml": *ptools = append(*ptools, checker.Tool{ Name: "Dependabot", - URL: asStringPointer("https://github.com/dependabot"), - Desc: asStringPointer("Automated dependency updates built into GitHub"), + URL: asPointer("https://github.com/dependabot"), + Desc: asPointer("Automated dependency updates built into GitHub"), Files: []checker.File{ { Path: name, @@ -90,8 +90,8 @@ var checkDependencyFileExists fileparser.DoWhileTrueOnFilename = func(name strin "renovate.json5", ".renovaterc": *ptools = append(*ptools, checker.Tool{ Name: "RenovateBot", - URL: asStringPointer("https://github.com/renovatebot/renovate"), - Desc: asStringPointer("Automated dependency updates. Multi-platform and multi-language."), + URL: asPointer("https://github.com/renovatebot/renovate"), + Desc: asPointer("Automated dependency updates. Multi-platform and multi-language."), Files: []checker.File{ { Path: name, @@ -103,8 +103,8 @@ var checkDependencyFileExists fileparser.DoWhileTrueOnFilename = func(name strin case ".pyup.yml": *ptools = append(*ptools, checker.Tool{ Name: "PyUp", - URL: asStringPointer("https://pyup.io/"), - Desc: asStringPointer("Automated dependency updates for Python."), + URL: asPointer("https://pyup.io/"), + Desc: asPointer("Automated dependency updates for Python."), Files: []checker.File{ { Path: name, @@ -116,8 +116,8 @@ var checkDependencyFileExists fileparser.DoWhileTrueOnFilename = func(name strin case ".lift.toml", ".lift/config.toml": *ptools = append(*ptools, checker.Tool{ Name: "Sonatype Lift", - URL: asStringPointer("https://lift.sonatype.com"), - Desc: asStringPointer("Automated dependency updates. Multi-platform and multi-language."), + URL: asPointer("https://lift.sonatype.com"), + Desc: asPointer("Automated dependency updates. Multi-platform and multi-language."), Files: []checker.File{ { Path: name, @@ -133,7 +133,7 @@ var checkDependencyFileExists fileparser.DoWhileTrueOnFilename = func(name strin return true, nil } -func asStringPointer(s string) *string { +func asPointer(s string) *string { return &s } diff --git a/checks/raw/fuzzing.go b/checks/raw/fuzzing.go index a53b70e44c4..a8a161d9847 100644 --- a/checks/raw/fuzzing.go +++ b/checks/raw/fuzzing.go @@ -62,8 +62,8 @@ var languageFuzzSpecs = map[clients.LanguageName]languageFuzzConfig{ filePattern: "*_test.go", funcPattern: `func\s+Fuzz\w+\s*\(\w+\s+\*testing.F\)`, Name: fuzzerBuiltInGo, - URL: asStringPointer("https://go.dev/doc/fuzz/"), - Desc: asStringPointer( + URL: asPointer("https://go.dev/doc/fuzz/"), + Desc: asPointer( "Go fuzzing intelligently walks through the source code to report failures and find vulnerabilities."), }, // Fuzz patterns for Haskell based on property-based testing. @@ -85,7 +85,7 @@ var languageFuzzSpecs = map[clients.LanguageName]languageFuzzConfig{ // or their indirect imports through the higher-level Hspec or Tasty testing frameworks. funcPattern: `import\s+(qualified\s+)?Test\.((Hspec|Tasty)\.)?(QuickCheck|Hedgehog|Validity|SmallCheck)`, Name: fuzzerPropertyBasedHaskell, - Desc: asStringPointer( + Desc: asPointer( "Property-based testing in Haskell generates test instances randomly or exhaustively " + "and test that specific properties are satisfied."), }, @@ -100,7 +100,7 @@ var languageFuzzSpecs = map[clients.LanguageName]languageFuzzConfig{ // Look for direct imports of fast-check. funcPattern: `(from\s+['"]fast-check['"]|require\(\s*['"]fast-check['"]\s*\))`, Name: fuzzerPropertyBasedJavaScript, - Desc: asStringPointer( + Desc: asPointer( "Property-based testing in JavaScript generates test instances randomly or exhaustively " + "and test that specific properties are satisfied."), }, @@ -109,7 +109,7 @@ var languageFuzzSpecs = map[clients.LanguageName]languageFuzzConfig{ // Look for direct imports of fast-check. funcPattern: `(from\s+['"]fast-check['"]|require\(\s*['"]fast-check['"]\s*\))`, Name: fuzzerPropertyBasedTypeScript, - Desc: asStringPointer( + Desc: asPointer( "Property-based testing in TypeScript generates test instances randomly or exhaustively " + "and test that specific properties are satisfied."), }, @@ -127,8 +127,8 @@ func Fuzzing(c *checker.CheckRequest) (checker.FuzzingData, error) { fuzzers = append(fuzzers, checker.Tool{ Name: fuzzerClusterFuzzLite, - URL: asStringPointer("https://github.com/google/clusterfuzzlite"), - Desc: asStringPointer("continuous fuzzing solution that runs as part of Continuous Integration (CI) workflows"), + URL: asPointer("https://github.com/google/clusterfuzzlite"), + Desc: asPointer("continuous fuzzing solution that runs as part of Continuous Integration (CI) workflows"), // TODO: File. }, ) @@ -142,10 +142,8 @@ func Fuzzing(c *checker.CheckRequest) (checker.FuzzingData, error) { fuzzers = append(fuzzers, checker.Tool{ Name: oneFuzz, - URL: asStringPointer("https://github.com/microsoft/onefuzz"), - Desc: asStringPointer( - "Enables continuous developer-driven fuzzing to proactively harden software prior to release.", - ), + URL: asPointer("https://github.com/microsoft/onefuzz"), + Desc: asPointer("Enables continuous developer-driven fuzzing to proactively harden software prior to release."), // TODO: File. }, ) @@ -159,8 +157,8 @@ func Fuzzing(c *checker.CheckRequest) (checker.FuzzingData, error) { fuzzers = append(fuzzers, checker.Tool{ Name: fuzzerOSSFuzz, - URL: asStringPointer("https://github.com/google/oss-fuzz"), - Desc: asStringPointer("Continuous Fuzzing for Open Source Software"), + URL: asPointer("https://github.com/google/oss-fuzz"), + Desc: asPointer("Continuous Fuzzing for Open Source Software"), // TODO: File. }, ) diff --git a/checks/raw/pinned_dependencies.go b/checks/raw/pinned_dependencies.go index 1054f847824..128b2fc7589 100644 --- a/checks/raw/pinned_dependencies.go +++ b/checks/raw/pinned_dependencies.go @@ -272,8 +272,8 @@ var validateDockerfilesPinning fileparser.DoWhileTrueOnFileContent = func( EndOffset: uint(child.EndLine), Snippet: child.Original, }, - Name: asStringPointer(name), - PinnedAt: asStringPointer(asName), + Name: asPointer(name), + PinnedAt: asPointer(asName), Pinned: asBoolPointer(pinned || regex.MatchString(name)), Type: checker.DependencyUseTypeDockerfileContainerImage, }, @@ -297,9 +297,9 @@ var validateDockerfilesPinning fileparser.DoWhileTrueOnFileContent = func( } parts := strings.SplitN(name, ":", 2) if len(parts) > 0 { - dep.Name = asStringPointer(parts[0]) + dep.Name = asPointer(parts[0]) if len(parts) > 1 { - dep.PinnedAt = asStringPointer(parts[1]) + dep.PinnedAt = asPointer(parts[1]) } } pdata.Dependencies = append(pdata.Dependencies, dep) @@ -394,7 +394,7 @@ var validateGitHubWorkflowIsFreeOfInsecureDownloads fileparser.DoWhileTrueOnFile if err := validateShellFile(pathfn, uint(execRun.Run.Pos.Line), uint(execRun.Run.Pos.Line), script, taintedFiles, pdata); err != nil { pdata.Dependencies = append(pdata.Dependencies, checker.Dependency{ - Msg: asStringPointer(err.Error()), + Msg: asPointer(err.Error()), }) } } @@ -482,9 +482,9 @@ var validateGitHubActionWorkflow fileparser.DoWhileTrueOnFileContent = func( } parts := strings.SplitN(execAction.Uses.Value, "@", 2) if len(parts) > 0 { - dep.Name = asStringPointer(parts[0]) + dep.Name = asPointer(parts[0]) if len(parts) > 1 { - dep.PinnedAt = asStringPointer(parts[1]) + dep.PinnedAt = asPointer(parts[1]) } } pdata.Dependencies = append(pdata.Dependencies, dep) diff --git a/cron/worker/worker_test.go b/cron/worker/worker_test.go index cb9177df30d..d1b04f7d396 100644 --- a/cron/worker/worker_test.go +++ b/cron/worker/worker_test.go @@ -23,7 +23,7 @@ import ( "github.com/ossf/scorecard/v4/cron/data" ) -func asIntPointer(i int32) *int32 { +func asPointer(i int32) *int32 { return &i } @@ -38,7 +38,7 @@ func TestResultFilename(t *testing.T) { name: "Basic", req: &data.ScorecardBatchRequest{ JobTime: timestamppb.New(time.Date(1979, time.October, 12, 1, 2, 3, 0, time.UTC)), - ShardNum: asIntPointer(42), + ShardNum: asPointer(42), }, want: "1979.10.12/010203/shard-0000042", }, diff --git a/dependencydiff/dependencydiff_test.go b/dependencydiff/dependencydiff_test.go index 688f3d24351..9a4699fcdb6 100644 --- a/dependencydiff/dependencydiff_test.go +++ b/dependencydiff/dependencydiff_test.go @@ -126,11 +126,11 @@ func Test_mapDependencyEcosystemNaming(t *testing.T) { deps: []dependency{ { Name: "dependency_1", - Ecosystem: asStringPointer("not_supported"), + Ecosystem: asPointer("not_supported"), }, { Name: "dependency_2", - Ecosystem: asStringPointer("gomod"), + Ecosystem: asPointer("gomod"), }, }, errWanted: errInvalid, @@ -140,7 +140,7 @@ func Test_mapDependencyEcosystemNaming(t *testing.T) { deps: []dependency{ { Name: "dependency_3", - Ecosystem: asStringPointer("foobar"), + Ecosystem: asPointer("foobar"), }, }, errWanted: errMappingNotFound, @@ -150,19 +150,19 @@ func Test_mapDependencyEcosystemNaming(t *testing.T) { deps: []dependency{ { Name: "dependency_4", - Ecosystem: asStringPointer("gomod"), + Ecosystem: asPointer("gomod"), }, { Name: "dependency_5", - Ecosystem: asStringPointer("pip"), + Ecosystem: asPointer("pip"), }, { Name: "dependency_6", - Ecosystem: asStringPointer("cargo"), + Ecosystem: asPointer("cargo"), }, { Name: "dependency_7", - Ecosystem: asStringPointer("actions"), + Ecosystem: asPointer("actions"), }, }, }, diff --git a/dependencydiff/mapping.go b/dependencydiff/mapping.go index 7d05fd7609c..285fd88173d 100644 --- a/dependencydiff/mapping.go +++ b/dependencydiff/mapping.go @@ -22,7 +22,7 @@ import ( type ecosystem string // OSV ecosystem naming data source: https://ossf.github.io/osv-schema/#affectedpackage-field -// nolint +//nolint const ( // The Go ecosystem. ecosystemGo ecosystem = "Go" @@ -101,7 +101,7 @@ func mapDependencyEcosystemNaming(deps []dependency) error { // Iff. the ecosystem is not empty and the mapping entry is not found, we will return an error. return fmt.Errorf("error mapping dependency ecosystem: %w", err) } - deps[i].Ecosystem = asStringPointer(string(mappedEcosys)) + deps[i].Ecosystem = asPointer(string(mappedEcosys)) } return nil } @@ -116,6 +116,6 @@ func toEcosystem(e string) (ecosystem, error) { return "", fmt.Errorf("%w for github entry %s", errMappingNotFound, e) } -func asStringPointer(s string) *string { +func asPointer(s string) *string { return &s } diff --git a/pkg/json_raw_results.go b/pkg/json_raw_results.go index 7da73b64475..629bee123b9 100644 --- a/pkg/json_raw_results.go +++ b/pkg/json_raw_results.go @@ -292,7 +292,7 @@ type jsonRawResults struct { DependencyPinning jsonPinningDependenciesData `json:"dependencyPinning"` } -func asStringPointer(s string) *string { +func asPointer(s string) *string { return &s } @@ -312,7 +312,7 @@ func (r *jsonScorecardRawResult) addTokenPermissionsRawResults(tp *checker.Token } p := jsonTokenPermission{ - LocationType: asStringPointer(string(*t.LocationType)), + LocationType: asPointer(string(*t.LocationType)), Name: t.Name, Value: t.Value, Type: string(t.Type), @@ -331,7 +331,7 @@ func (r *jsonScorecardRawResult) addTokenPermissionsRawResults(tp *checker.Token Offset: t.File.Offset, } if t.File.Snippet != "" { - p.File.Snippet = asStringPointer(t.File.Snippet) + p.File.Snippet = asPointer(t.File.Snippet) } } @@ -361,7 +361,7 @@ func (r *jsonScorecardRawResult) addPackagingRawResults(pk *checker.PackagingDat } if p.File.Snippet != "" { - jpk.File.Snippet = asStringPointer(p.File.Snippet) + jpk.File.Snippet = asPointer(p.File.Snippet) } for _, run := range p.Runs { @@ -419,7 +419,7 @@ func (r *jsonScorecardRawResult) addDangerousWorkflowRawResults(df *checker.Dang Type: string(e.Type), } if e.File.Snippet != "" { - v.File.Snippet = asStringPointer(e.File.Snippet) + v.File.Snippet = asPointer(e.File.Snippet) } if e.Job != nil { v.Job = &jsonWorkflowJob{ diff --git a/pkg/json_raw_results_test.go b/pkg/json_raw_results_test.go index 46d350417db..82adcc8b116 100644 --- a/pkg/json_raw_results_test.go +++ b/pkg/json_raw_results_test.go @@ -26,7 +26,7 @@ import ( "github.com/ossf/scorecard/v4/clients" ) -func TestAsStringPointer(t *testing.T) { +func TestAsPointer(t *testing.T) { t.Parallel() tests := []struct { //nolint:govet @@ -37,17 +37,17 @@ func TestAsStringPointer(t *testing.T) { { name: "test_empty_string", input: "", - expected: asStringPointer(""), + expected: asPointer(""), }, { name: "test_non_empty_string", input: "test", - expected: asStringPointer("test"), + expected: asPointer("test"), }, { name: "test_number_string", input: "123", - expected: asStringPointer("123"), + expected: asPointer("123"), }, } @@ -55,9 +55,9 @@ func TestAsStringPointer(t *testing.T) { tt := tt t.Run(tt.name, func(t *testing.T) { t.Parallel() - result := asStringPointer(tt.input) + result := asPointer(tt.input) if *result != *tt.expected { - t.Errorf("asStringPointer() = %v, want %v", result, tt.expected) + t.Errorf("asPointer() = %v, want %v", result, tt.expected) } }) } @@ -110,7 +110,7 @@ func TestJsonScorecardRawResult_AddPackagingRawResults(t *testing.T) { input: &checker.PackagingData{ Packages: []checker.Package{ { - Msg: asStringPointer("testMsg"), + Msg: asPointer("testMsg"), }, }, }, @@ -193,8 +193,8 @@ func TestJsonScorecardRawResult_AddTokenPermissionsRawResults(t *testing.T) { LocationType: &loc, Type: checker.PermissionLevelUndeclared, Job: &checker.WorkflowJob{ - Name: asStringPointer("testJobName"), - ID: asStringPointer("testJobID"), + Name: asPointer("testJobName"), + ID: asPointer("testJobID"), }, File: &checker.File{ Path: "testPath", @@ -250,8 +250,8 @@ func TestJsonScorecardRawResult_AddDependencyPinningRawResults(t *testing.T) { EndOffset: 5, Snippet: "testSnippet", }, - Name: asStringPointer("testDependency"), - PinnedAt: asStringPointer("testPinnedAt"), + Name: asPointer("testDependency"), + PinnedAt: asPointer("testPinnedAt"), Type: checker.DependencyUseTypeGHAction, }, }, @@ -264,8 +264,8 @@ func TestJsonScorecardRawResult_AddDependencyPinningRawResults(t *testing.T) { Dependencies: []checker.Dependency{ { Location: nil, - Name: asStringPointer("testDependency"), - PinnedAt: asStringPointer("testPinnedAt"), + Name: asPointer("testDependency"), + PinnedAt: asPointer("testPinnedAt"), Type: checker.DependencyUseTypeGHAction, }, }, @@ -309,8 +309,8 @@ func TestJsonScorecardRawResult_AddDangerousWorkflowRawResults(t *testing.T) { }, Type: checker.DangerousWorkflowScriptInjection, Job: &checker.WorkflowJob{ - Name: asStringPointer("testJob"), - ID: asStringPointer("testID"), + Name: asPointer("testJob"), + ID: asPointer("testID"), }, }, }, @@ -437,7 +437,7 @@ func TestJsonScorecardRawResult_AddMaintainedRawResults(t *testing.T) { CreatedAt: time.Now(), Issues: []clients.Issue{ { - URI: asStringPointer("testUrl"), + URI: asPointer("testUrl"), Author: &clients.User{ Login: "testLogin", }, @@ -960,8 +960,8 @@ func TestAddFuzzingRawResults(t *testing.T) { Fuzzers: []checker.Tool{ { Name: "fuzzer1", - URL: asStringPointer("https://example.com/fuzzer1"), - Desc: asStringPointer("Fuzzer 1 description"), + URL: asPointer("https://example.com/fuzzer1"), + Desc: asPointer("Fuzzer 1 description"), Files: []checker.File{ { Path: "path/to/fuzzer1/file1", @@ -973,8 +973,8 @@ func TestAddFuzzingRawResults(t *testing.T) { }, { Name: "fuzzer2", - URL: asStringPointer("https://example.com/fuzzer2"), - Desc: asStringPointer("Fuzzer 2 description"), + URL: asPointer("https://example.com/fuzzer2"), + Desc: asPointer("Fuzzer 2 description"), Files: []checker.File{ { Path: "path/to/fuzzer2/file1", @@ -992,8 +992,8 @@ func TestAddFuzzingRawResults(t *testing.T) { expectedFuzzers := []jsonTool{ { Name: "fuzzer1", - URL: asStringPointer("https://example.com/fuzzer1"), - Desc: asStringPointer("Fuzzer 1 description"), + URL: asPointer("https://example.com/fuzzer1"), + Desc: asPointer("Fuzzer 1 description"), Files: []jsonFile{ { Path: "path/to/fuzzer1/file1", @@ -1005,8 +1005,8 @@ func TestAddFuzzingRawResults(t *testing.T) { }, { Name: "fuzzer2", - URL: asStringPointer("https://example.com/fuzzer2"), - Desc: asStringPointer("Fuzzer 2 description"), + URL: asPointer("https://example.com/fuzzer2"), + Desc: asPointer("Fuzzer 2 description"), Files: []jsonFile{ { Path: "path/to/fuzzer2/file1", @@ -1080,8 +1080,8 @@ func TestJsonScorecardRawResult(t *testing.T) { Fuzzers: []checker.Tool{ { Name: "fuzzer1", - URL: asStringPointer("https://example.com/fuzzer1"), - Desc: asStringPointer("fuzzer1 description"), + URL: asPointer("https://example.com/fuzzer1"), + Desc: asPointer("fuzzer1 description"), Files: []checker.File{ {Path: "fuzzers/fuzzer1/foo"}, {Path: "fuzzers/fuzzer1/bar"}, @@ -1089,8 +1089,8 @@ func TestJsonScorecardRawResult(t *testing.T) { }, { Name: "fuzzer2", - URL: asStringPointer("https://example.com/fuzzer2"), - Desc: asStringPointer("fuzzer2 description"), + URL: asPointer("https://example.com/fuzzer2"), + Desc: asPointer("fuzzer2 description"), Files: []checker.File{ {Path: "fuzzers/fuzzer2/foo"}, {Path: "fuzzers/fuzzer2/bar"}, @@ -1184,8 +1184,8 @@ func TestJsonScorecardRawResult(t *testing.T) { expectedFuzzers := []jsonTool{ { Name: "fuzzer1", - URL: asStringPointer("https://example.com/fuzzer1"), - Desc: asStringPointer("fuzzer1 description"), + URL: asPointer("https://example.com/fuzzer1"), + Desc: asPointer("fuzzer1 description"), Files: []jsonFile{ {Path: "fuzzers/fuzzer1/foo"}, {Path: "fuzzers/fuzzer1/bar"}, @@ -1193,8 +1193,8 @@ func TestJsonScorecardRawResult(t *testing.T) { }, { Name: "fuzzer2", - URL: asStringPointer("https://example.com/fuzzer1"), - Desc: asStringPointer("fuzzer1 description"), + URL: asPointer("https://example.com/fuzzer1"), + Desc: asPointer("fuzzer1 description"), Files: []jsonFile{ {Path: "fuzzers/fuzzer2/foo"}, {Path: "fuzzers/fuzzer2/bar"}, diff --git a/remediation/remediations_test.go b/remediation/remediations_test.go index e80b72ca4cf..dce119881d5 100644 --- a/remediation/remediations_test.go +++ b/remediation/remediations_test.go @@ -52,7 +52,7 @@ func TestRepeatedSetup(t *testing.T) { } } -func asStringPointer(s string) *string { +func asPointer(s string) *string { return &s } @@ -89,7 +89,7 @@ func TestCreateDockerfilePinningRemediation(t *testing.T) { { name: "image name no tag", dep: checker.Dependency{ - Name: asStringPointer("foo"), + Name: asPointer("foo"), Type: checker.DependencyUseTypeDockerfileContainerImage, }, expected: &rule.Remediation{ @@ -101,8 +101,8 @@ func TestCreateDockerfilePinningRemediation(t *testing.T) { // github.com/ossf/scorecard/issues/2581 name: "image name with tag", dep: checker.Dependency{ - Name: asStringPointer("amazoncorretto"), - PinnedAt: asStringPointer("11"), + Name: asPointer("amazoncorretto"), + PinnedAt: asPointer("11"), Type: checker.DependencyUseTypeDockerfileContainerImage, }, expected: &rule.Remediation{ @@ -113,7 +113,7 @@ func TestCreateDockerfilePinningRemediation(t *testing.T) { { name: "unknown image", dep: checker.Dependency{ - Name: asStringPointer("not-found"), + Name: asPointer("not-found"), Type: checker.DependencyUseTypeDockerfileContainerImage, }, expected: nil, @@ -121,8 +121,8 @@ func TestCreateDockerfilePinningRemediation(t *testing.T) { { name: "unknown tag", dep: checker.Dependency{ - Name: asStringPointer("foo"), - PinnedAt: asStringPointer("not-found"), + Name: asPointer("foo"), + PinnedAt: asPointer("not-found"), Type: checker.DependencyUseTypeDockerfileContainerImage, }, expected: nil, From 5adf329bfa44c6b157974342833f1eb8e29507b8 Mon Sep 17 00:00:00 2001 From: Gabriela Gutierrez Date: Thu, 31 Aug 2023 13:40:26 +0000 Subject: [PATCH 21/46] 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 --- checks/evaluation/pinned_dependencies.go | 17 ++++++++-- checks/evaluation/pinned_dependencies_test.go | 33 +++++++++---------- 2 files changed, 29 insertions(+), 21 deletions(-) diff --git a/checks/evaluation/pinned_dependencies.go b/checks/evaluation/pinned_dependencies.go index 977f6c1f5c3..486efa2882a 100644 --- a/checks/evaluation/pinned_dependencies.go +++ b/checks/evaluation/pinned_dependencies.go @@ -64,7 +64,6 @@ func PinningDependencies(name string, c *checker.CheckRequest, }) continue } - if rr.Msg != nil { dl.Debug(&checker.LogMessage{ Path: rr.Location.Path, @@ -74,7 +73,20 @@ func PinningDependencies(name string, c *checker.CheckRequest, Text: *rr.Msg, Snippet: rr.Location.Snippet, }) - } else if !*rr.Pinned { + 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, @@ -85,7 +97,6 @@ func PinningDependencies(name string, c *checker.CheckRequest, Remediation: generateRemediation(remediationMetadata, &rr), }) } - // Update the pinning status. updatePinningResults(&rr, &wp, pr) } diff --git a/checks/evaluation/pinned_dependencies_test.go b/checks/evaluation/pinned_dependencies_test.go index d7455e64822..7db5ccc7502 100644 --- a/checks/evaluation/pinned_dependencies_test.go +++ b/checks/evaluation/pinned_dependencies_test.go @@ -327,55 +327,52 @@ func Test_PinningDependencies(t *testing.T) { }, }, { - name: "pinned dependency with debug message", + name: "unpinned dependency shows warn message", dependencies: []checker.Dependency{ { Location: &checker.File{}, - Msg: asPointer("some message"), Type: checker.DependencyUseTypePipCommand, - Pinned: asBoolPointer(true), + Pinned: asBoolPointer(false), }, }, expected: scut.TestReturn{ Error: nil, - Score: 10, - NumberOfWarn: 0, - NumberOfInfo: 8, - NumberOfDebug: 1, + Score: 0, + NumberOfWarn: 1, + NumberOfInfo: 7, + NumberOfDebug: 0, }, }, { - name: "unpinned dependency shows warn message", + 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, - Pinned: asBoolPointer(false), }, }, expected: scut.TestReturn{ Error: nil, - Score: 0, - NumberOfWarn: 1, - NumberOfInfo: 7, - NumberOfDebug: 0, + Score: -1, + NumberOfWarn: 0, + NumberOfInfo: 8, + NumberOfDebug: 1, }, }, { - name: "unpinned dependency with debug message shows no warn message", + name: "dependency missing Pinned info does not count for score and shows debug message", dependencies: []checker.Dependency{ { Location: &checker.File{}, - Msg: asPointer("some message"), Type: checker.DependencyUseTypePipCommand, - Pinned: asBoolPointer(false), }, }, expected: scut.TestReturn{ Error: nil, - Score: 0, + Score: -1, NumberOfWarn: 0, - NumberOfInfo: 7, + NumberOfInfo: 8, NumberOfDebug: 1, }, }, From 1ad766b44120de73681bfa6486c6e6d1df59e4ee Mon Sep 17 00:00:00 2001 From: Gabriela Gutierrez Date: Thu, 31 Aug 2023 13:42:44 +0000 Subject: [PATCH 22/46] 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 --- checks/evaluation/pinned_dependencies_test.go | 23 ------------------- 1 file changed, 23 deletions(-) diff --git a/checks/evaluation/pinned_dependencies_test.go b/checks/evaluation/pinned_dependencies_test.go index 7db5ccc7502..20832f5b987 100644 --- a/checks/evaluation/pinned_dependencies_test.go +++ b/checks/evaluation/pinned_dependencies_test.go @@ -376,29 +376,6 @@ func Test_PinningDependencies(t *testing.T) { NumberOfDebug: 1, }, }, - { - name: "unpinned dependency shows warn message", - dependencies: []checker.Dependency{ - { - Location: &checker.File{}, - Msg: asPointer("some message"), - Type: checker.DependencyUseTypeDownloadThenRun, - Pinned: asBoolPointer(false), - }, - { - Location: &checker.File{}, - Type: checker.DependencyUseTypeDownloadThenRun, - Pinned: asBoolPointer(false), - }, - }, - expected: scut.TestReturn{ - Error: nil, - Score: 0, - NumberOfWarn: 1, - NumberOfInfo: 6, - NumberOfDebug: 1, - }, - }, // TODO: choco installs should score for Pinned-Dependencies // { // name: "unpinned choco install", From 7a2a3a463140e27c61f59e96f828fda4952ad2ef Mon Sep 17 00:00:00 2001 From: Gabriela Gutierrez Date: Thu, 31 Aug 2023 14:18:39 +0000 Subject: [PATCH 23/46] test: Add missing dep Location cases Signed-off-by: Gabriela Gutierrez --- checks/evaluation/pinned_dependencies_test.go | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/checks/evaluation/pinned_dependencies_test.go b/checks/evaluation/pinned_dependencies_test.go index 20832f5b987..3b0e11127df 100644 --- a/checks/evaluation/pinned_dependencies_test.go +++ b/checks/evaluation/pinned_dependencies_test.go @@ -20,6 +20,7 @@ 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" ) @@ -376,6 +377,30 @@ func Test_PinningDependencies(t *testing.T) { 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: 0, + NumberOfDebug: 0, + }, + }, + { + 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: 8, + NumberOfDebug: 1, + }, + }, // TODO: choco installs should score for Pinned-Dependencies // { // name: "unpinned choco install", From 4412f0ee158e61ee34f214b9df84ac0d1addfeae Mon Sep 17 00:00:00 2001 From: Gabriela Gutierrez Date: Thu, 31 Aug 2023 15:08:52 +0000 Subject: [PATCH 24/46] fix: Simplify Dockerfile pinned as name logic Signed-off-by: Gabriela Gutierrez --- checks/raw/pinned_dependencies.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/checks/raw/pinned_dependencies.go b/checks/raw/pinned_dependencies.go index 128b2fc7589..90b28bdff8a 100644 --- a/checks/raw/pinned_dependencies.go +++ b/checks/raw/pinned_dependencies.go @@ -274,7 +274,7 @@ var validateDockerfilesPinning fileparser.DoWhileTrueOnFileContent = func( }, Name: asPointer(name), PinnedAt: asPointer(asName), - Pinned: asBoolPointer(pinned || regex.MatchString(name)), + Pinned: asBoolPointer(pinnedAsNames[asName]), Type: checker.DependencyUseTypeDockerfileContainerImage, }, ) From 512659cd8ca3f5f619b07de146d914df6a220e6e Mon Sep 17 00:00:00 2001 From: Gabriela Gutierrez Date: Thu, 31 Aug 2023 15:43:38 +0000 Subject: [PATCH 25/46] 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 --- checks/evaluation/pinned_dependencies.go | 6 +- checks/evaluation/pinned_dependencies_test.go | 136 +++++++++++++----- 2 files changed, 105 insertions(+), 37 deletions(-) diff --git a/checks/evaluation/pinned_dependencies.go b/checks/evaluation/pinned_dependencies.go index 486efa2882a..c0fa7c340c5 100644 --- a/checks/evaluation/pinned_dependencies.go +++ b/checks/evaluation/pinned_dependencies.go @@ -303,7 +303,7 @@ func createReturnValues(pr map[checker.DependencyUseType]pinnedResult, switch r.total { case 0: - dl.Info(&checker.LogMessage{ + dl.Debug(&checker.LogMessage{ Text: inconclusiveResultMsg, }) return checker.InconclusiveResultScore, nil @@ -329,13 +329,13 @@ func createReturnValuesForGitHubActionsWorkflowPinned(r worklowPinningResult, ma inconclusiveResultMsg string, dl checker.DetailLogger, ) (int, error) { if r.gitHubOwned.total == 0 { - dl.Info(&checker.LogMessage{ + dl.Debug(&checker.LogMessage{ Text: fmt.Sprintf("%s %s", "no GitHub-owned", inconclusiveResultMsg), }) } if r.thirdParties.total == 0 { - dl.Info(&checker.LogMessage{ + dl.Debug(&checker.LogMessage{ Text: fmt.Sprintf("%s %s", "no Third-party", inconclusiveResultMsg), }) } diff --git a/checks/evaluation/pinned_dependencies_test.go b/checks/evaluation/pinned_dependencies_test.go index 3b0e11127df..9773c148610 100644 --- a/checks/evaluation/pinned_dependencies_test.go +++ b/checks/evaluation/pinned_dependencies_test.go @@ -224,6 +224,40 @@ func Test_PinningDependencies(t *testing.T) { { 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{}, + 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, @@ -241,6 +275,40 @@ func Test_PinningDependencies(t *testing.T) { { 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{}, + 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, @@ -250,8 +318,8 @@ func Test_PinningDependencies(t *testing.T) { expected: scut.TestReturn{ Error: nil, Score: 0, - NumberOfWarn: 1, - NumberOfInfo: 7, + NumberOfWarn: 7, + NumberOfInfo: 0, NumberOfDebug: 0, }, }, @@ -273,8 +341,8 @@ func Test_PinningDependencies(t *testing.T) { Error: nil, Score: 5, NumberOfWarn: 1, - NumberOfInfo: 7, - NumberOfDebug: 0, + NumberOfInfo: 1, + NumberOfDebug: 6, }, }, { @@ -295,8 +363,8 @@ func Test_PinningDependencies(t *testing.T) { Error: nil, Score: 0, NumberOfWarn: 1, - NumberOfInfo: 7, - NumberOfDebug: 0, + NumberOfInfo: 0, + NumberOfDebug: 7, }, }, { @@ -306,8 +374,8 @@ func Test_PinningDependencies(t *testing.T) { Error: nil, Score: -1, NumberOfWarn: 0, - NumberOfInfo: 8, - NumberOfDebug: 0, + NumberOfInfo: 0, + NumberOfDebug: 8, }, }, { @@ -323,8 +391,8 @@ func Test_PinningDependencies(t *testing.T) { Error: nil, Score: 10, NumberOfWarn: 0, - NumberOfInfo: 8, - NumberOfDebug: 0, + NumberOfInfo: 1, + NumberOfDebug: 7, }, }, { @@ -340,8 +408,8 @@ func Test_PinningDependencies(t *testing.T) { Error: nil, Score: 0, NumberOfWarn: 1, - NumberOfInfo: 7, - NumberOfDebug: 0, + NumberOfInfo: 0, + NumberOfDebug: 7, }, }, { @@ -357,8 +425,8 @@ func Test_PinningDependencies(t *testing.T) { Error: nil, Score: -1, NumberOfWarn: 0, - NumberOfInfo: 8, - NumberOfDebug: 1, + NumberOfInfo: 0, + NumberOfDebug: 9, }, }, { @@ -373,8 +441,8 @@ func Test_PinningDependencies(t *testing.T) { Error: nil, Score: -1, NumberOfWarn: 0, - NumberOfInfo: 8, - NumberOfDebug: 1, + NumberOfInfo: 0, + NumberOfDebug: 9, }, }, { @@ -397,8 +465,8 @@ func Test_PinningDependencies(t *testing.T) { Error: nil, Score: -1, NumberOfWarn: 0, - NumberOfInfo: 8, - NumberOfDebug: 1, + NumberOfInfo: 0, + NumberOfDebug: 9, }, }, // TODO: choco installs should score for Pinned-Dependencies @@ -432,8 +500,8 @@ func Test_PinningDependencies(t *testing.T) { Error: nil, Score: 0, NumberOfWarn: 1, - NumberOfInfo: 7, - NumberOfDebug: 0, + NumberOfInfo: 0, + NumberOfDebug: 7, }, }, // TODO: Due to a bug download then run is score twice in shell scripts @@ -453,8 +521,8 @@ func Test_PinningDependencies(t *testing.T) { Error: nil, Score: 0, NumberOfWarn: 1, - NumberOfInfo: 6, - NumberOfDebug: 0, + NumberOfInfo: 0, + NumberOfDebug: 6, }, }, // TODO: Due to a bug download then run is score twice in shell scripts @@ -474,8 +542,8 @@ func Test_PinningDependencies(t *testing.T) { Error: nil, Score: 0, NumberOfWarn: 1, - NumberOfInfo: 6, - NumberOfDebug: 0, + NumberOfInfo: 0, + NumberOfDebug: 6, }, }, { @@ -491,8 +559,8 @@ func Test_PinningDependencies(t *testing.T) { Error: nil, Score: 0, NumberOfWarn: 1, - NumberOfInfo: 7, - NumberOfDebug: 0, + NumberOfInfo: 0, + NumberOfDebug: 7, }, }, { @@ -508,8 +576,8 @@ func Test_PinningDependencies(t *testing.T) { Error: nil, Score: 0, NumberOfWarn: 1, - NumberOfInfo: 7, - NumberOfDebug: 0, + NumberOfInfo: 0, + NumberOfDebug: 7, }, }, // TODO: nuget installs should score for Pinned-Dependencies @@ -543,8 +611,8 @@ func Test_PinningDependencies(t *testing.T) { Error: nil, Score: 0, NumberOfWarn: 1, - NumberOfInfo: 7, - NumberOfDebug: 0, + NumberOfInfo: 0, + NumberOfDebug: 7, }, }, { @@ -565,8 +633,8 @@ func Test_PinningDependencies(t *testing.T) { Error: nil, Score: 0, NumberOfWarn: 2, - NumberOfInfo: 7, - NumberOfDebug: 0, + NumberOfInfo: 0, + NumberOfDebug: 7, }, }, { @@ -587,8 +655,8 @@ func Test_PinningDependencies(t *testing.T) { Error: nil, Score: 0, NumberOfWarn: 2, - NumberOfInfo: 6, - NumberOfDebug: 0, + NumberOfInfo: 0, + NumberOfDebug: 6, }, }, } From 9731d8a08247f11f7a35f0c8a69791ab5973552f Mon Sep 17 00:00:00 2001 From: Gabriela Gutierrez Date: Thu, 31 Aug 2023 16:51:37 +0000 Subject: [PATCH 26/46] test: Fix e2e tests and more unit tests Signed-off-by: Gabriela Gutierrez --- checks/evaluation/pinned_dependencies_test.go | 62 ++++++++++++++++--- e2e/pinned_dependencies_test.go | 12 ++-- 2 files changed, 58 insertions(+), 16 deletions(-) diff --git a/checks/evaluation/pinned_dependencies_test.go b/checks/evaluation/pinned_dependencies_test.go index 9773c148610..fd4c5090a5d 100644 --- a/checks/evaluation/pinned_dependencies_test.go +++ b/checks/evaluation/pinned_dependencies_test.go @@ -31,8 +31,12 @@ func Test_createReturnForIsGitHubActionsWorkflowPinned(t *testing.T) { r worklowPinningResult dl *scut.TestDetailLogger } + type log struct { + text string + detailType checker.DetailType + } type want struct { - logs []string + logs []log score int } //nolint @@ -58,7 +62,16 @@ func Test_createReturnForIsGitHubActionsWorkflowPinned(t *testing.T) { }, want: want{ score: 10, - logs: []string{"GitHub-owned GitHubActions are pinned", "Third-party GitHubActions are pinned"}, + logs: []log{ + { + text: "GitHub-owned GitHubActions are pinned", + detailType: checker.DetailInfo, + }, + { + text: "Third-party GitHubActions are pinned", + detailType: checker.DetailInfo, + }, + }, }, }, { @@ -78,7 +91,12 @@ func Test_createReturnForIsGitHubActionsWorkflowPinned(t *testing.T) { }, want: want{ score: 2, - logs: []string{"GitHub-owned GitHubActions are pinned"}, + logs: []log{ + { + text: "GitHub-owned GitHubActions are pinned", + detailType: checker.DetailInfo, + }, + }, }, }, { @@ -98,7 +116,12 @@ func Test_createReturnForIsGitHubActionsWorkflowPinned(t *testing.T) { }, want: want{ score: 8, - logs: []string{"Third-party GitHubActions are pinned"}, + logs: []log{ + { + text: "Third-party GitHubActions are pinned", + detailType: checker.DetailInfo, + }, + }, }, }, { @@ -118,7 +141,7 @@ func Test_createReturnForIsGitHubActionsWorkflowPinned(t *testing.T) { }, want: want{ score: 0, - logs: []string{}, + logs: []log{}, }, }, { @@ -138,7 +161,16 @@ func Test_createReturnForIsGitHubActionsWorkflowPinned(t *testing.T) { }, want: want{ score: -1, - logs: []string{"no GitHub-owned GitHubActions found", "no Third-party GitHubActions found"}, + logs: []log{ + { + text: "no GitHub-owned GitHubActions found", + detailType: checker.DetailDebug, + }, + { + text: "no Third-party GitHubActions found", + detailType: checker.DetailDebug, + }, + }, }, }, { @@ -158,7 +190,12 @@ func Test_createReturnForIsGitHubActionsWorkflowPinned(t *testing.T) { }, want: want{ score: 2, - logs: []string{"no GitHub-owned GitHubActions found"}, + logs: []log{ + { + text: "no GitHub-owned GitHubActions found", + detailType: checker.DetailDebug, + }, + }, }, }, { @@ -178,7 +215,12 @@ func Test_createReturnForIsGitHubActionsWorkflowPinned(t *testing.T) { }, want: want{ score: 8, - logs: []string{"no Third-party GitHubActions found"}, + logs: []log{ + { + text: "no Third-party GitHubActions found", + detailType: checker.DetailDebug, + }, + }, }, }, } @@ -195,7 +237,7 @@ func Test_createReturnForIsGitHubActionsWorkflowPinned(t *testing.T) { } for _, log := range tt.want.logs { isExpectedLog := func(logMessage checker.LogMessage, logType checker.DetailType) bool { - return logMessage.Text == log && logType == checker.DetailInfo + return logMessage.Text == log.text && logType == log.detailType } if !scut.ValidateLogMessage(isExpectedLog, tt.args.dl) { t.Errorf("test failed: log message not present: %+v", log) @@ -759,7 +801,7 @@ func Test_createReturnValues(t *testing.T) { switch tt.want { case -1: isExpectedLog := func(logMessage checker.LogMessage, logType checker.DetailType) bool { - return logMessage.Text == "no dependencies found" && logType == checker.DetailInfo + return logMessage.Text == "no dependencies found" && logType == checker.DetailDebug } if !scut.ValidateLogMessage(isExpectedLog, tt.args.dl) { t.Errorf("test failed: log message not present: %+v", "no dependencies found") diff --git a/e2e/pinned_dependencies_test.go b/e2e/pinned_dependencies_test.go index d736bdd9419..03d249304af 100644 --- a/e2e/pinned_dependencies_test.go +++ b/e2e/pinned_dependencies_test.go @@ -51,8 +51,8 @@ var _ = Describe("E2E TEST:"+checks.CheckPinnedDependencies, func() { Error: nil, Score: 3, NumberOfWarn: 139, - NumberOfInfo: 3, - NumberOfDebug: 0, + NumberOfInfo: 1, + NumberOfDebug: 2, } result := checks.PinningDependencies(&req) Expect(scut.ValidateTestReturn(nil, "dependencies check", &expected, &result, &dl)).Should(BeTrue()) @@ -76,8 +76,8 @@ var _ = Describe("E2E TEST:"+checks.CheckPinnedDependencies, func() { Error: nil, Score: 3, NumberOfWarn: 139, - NumberOfInfo: 3, - NumberOfDebug: 0, + NumberOfInfo: 1, + NumberOfDebug: 2, } result := checks.PinningDependencies(&req) Expect(scut.ValidateTestReturn(nil, "dependencies check", &expected, &result, &dl)).Should(BeTrue()) @@ -112,8 +112,8 @@ var _ = Describe("E2E TEST:"+checks.CheckPinnedDependencies, func() { Error: nil, Score: 3, NumberOfWarn: 139, - NumberOfInfo: 3, - NumberOfDebug: 0, + NumberOfInfo: 1, + NumberOfDebug: 2, } result := checks.PinningDependencies(&req) Expect(scut.ValidateTestReturn(nil, "dependencies check", &expected, &result, &dl)).Should(BeTrue()) From 08bd85efa186e7ed433ff1b1559dbbf5d4efbee3 Mon Sep 17 00:00:00 2001 From: Gabriela Gutierrez Date: Thu, 31 Aug 2023 22:57:22 +0000 Subject: [PATCH 27/46] 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 --- checks/evaluation/pinned_dependencies.go | 191 ++---------- checks/evaluation/pinned_dependencies_test.go | 274 +++++++----------- e2e/pinned_dependencies_test.go | 6 +- 3 files changed, 146 insertions(+), 325 deletions(-) diff --git a/checks/evaluation/pinned_dependencies.go b/checks/evaluation/pinned_dependencies.go index c0fa7c340c5..de27c3726d8 100644 --- a/checks/evaluation/pinned_dependencies.go +++ b/checks/evaluation/pinned_dependencies.go @@ -102,64 +102,27 @@ func PinningDependencies(name string, c *checker.CheckRequest, } // 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) - } - - // Npm installs. - npmScore, err := createReturnForIsNpmInstallPinned(pr, dl) - if err != nil { - return checker.CreateRuntimeErrorResult(name, err) - } - - // Go installs. - goScore, err := createReturnForIsGoInstallPinned(pr, dl) - if err != nil { - return checker.CreateRuntimeErrorResult(name, err) - } - - // If no dependencies of an ecossystem are found, it results in an inconclusive score. - // We filter out inconclusive scores so only applicable ecossystems are considered in - // the final score. - scores := []int{actionScore, dockerFromScore, dockerDownloadScore, scriptScore, pipScore, npmScore, goScore} - conclusiveScores := []int{} - for i := range scores { - if scores[i] != checker.InconclusiveResultScore { - conclusiveScores = append(conclusiveScores, scores[i]) - } - } - - if len(conclusiveScores) == 0 { + var scores []int + // Go through all dependency types + // GitHub Actions need to be handled separately since they are not in pr + // We should not score GitHub Actions if there are no GitHub Actions + if wp.gitHubOwned.total != 0 || wp.thirdParties.total != 0 { + score := createReturnValuesForGitHubActionsWorkflowPinned(wp, dl) + scores = append(scores, score) + } + // Only exisiting dependencies will be found in pr + // We will only score the ecossystem if there are dependencies + // This results in only existing ecossystems being included in the final score + for t := range pr { + score := createReturnValues(pr, t, dl) + scores = append(scores, score) + } + + if len(scores) == 0 { return checker.CreateInconclusiveResult(name, "no dependencies found") } - score := checker.AggregateScores(conclusiveScores...) + score := checker.AggregateScores(scores...) if score == checker.MaxResultScore { return checker.CreateMaxScoreResult(name, "all dependencies are pinned") @@ -230,120 +193,28 @@ func addWorkflowPinnedResult(rr *checker.Dependency, w *worklowPinningResult, is } } -// 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", - "no 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", - "no Dockerfile dependencies found", - 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", - "no 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", - "no pip installs found", - dl) -} - -// 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", - "no npm installs found", - 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", - "no go installs found", - dl) -} - func createReturnValues(pr map[checker.DependencyUseType]pinnedResult, - t checker.DependencyUseType, maxResultMsg string, inconclusiveResultMsg string, - dl checker.DetailLogger, -) (int, error) { - // If there are zero dependencies of this type, it will generate an inconclusive score, - // so we can disconsider it in the aggregated score. + t checker.DependencyUseType, dl checker.DetailLogger, +) int { + // If there are zero dependencies of this type, it will not reach this function, + // so its not gonna be included it in the aggregated score. // If all dependencies of this type are pinned, it will get a maximum score. // If 1 or more dependencies of this type are unpinned, it will get a minimum score. //nolint r := pr[t] - switch r.total { - case 0: - dl.Debug(&checker.LogMessage{ - Text: inconclusiveResultMsg, - }) - return checker.InconclusiveResultScore, nil - case r.pinned: + if r.total == r.pinned { dl.Info(&checker.LogMessage{ - Text: maxResultMsg, + Text: fmt.Sprintf("all %ss are pinned", t), }) - return checker.MaxResultScore, nil - default: - return checker.MinResultScore, nil + return checker.MaxResultScore } -} -// Create the result. -func createReturnForIsGitHubActionsWorkflowPinned(wp worklowPinningResult, dl checker.DetailLogger) (int, error) { - return createReturnValuesForGitHubActionsWorkflowPinned(wp, - fmt.Sprintf("%ss are pinned", checker.DependencyUseTypeGHAction), - fmt.Sprintf("%ss found", checker.DependencyUseTypeGHAction), - dl) + return checker.MinResultScore } -func createReturnValuesForGitHubActionsWorkflowPinned(r worklowPinningResult, maxResultMsg string, - inconclusiveResultMsg string, dl checker.DetailLogger, -) (int, error) { - if r.gitHubOwned.total == 0 { - dl.Debug(&checker.LogMessage{ - Text: fmt.Sprintf("%s %s", "no GitHub-owned", inconclusiveResultMsg), - }) - } - - if r.thirdParties.total == 0 { - dl.Debug(&checker.LogMessage{ - Text: fmt.Sprintf("%s %s", "no Third-party", inconclusiveResultMsg), - }) - } - - if r.gitHubOwned.total == 0 && r.thirdParties.total == 0 { - return checker.InconclusiveResultScore, nil - } - +func createReturnValuesForGitHubActionsWorkflowPinned(r worklowPinningResult, dl checker.DetailLogger, +) int { score := checker.MinResultScore if r.gitHubOwned.total == r.gitHubOwned.pinned { @@ -352,7 +223,7 @@ func createReturnValuesForGitHubActionsWorkflowPinned(r worklowPinningResult, ma dl.Info(&checker.LogMessage{ Type: finding.FileTypeSource, Offset: checker.OffsetDefault, - Text: fmt.Sprintf("%s %s", "GitHub-owned", maxResultMsg), + Text: fmt.Sprintf("all %s %ss are pinned", generateOwnerToDisplay(true), checker.DependencyUseTypeGHAction), }) } } @@ -363,10 +234,10 @@ func createReturnValuesForGitHubActionsWorkflowPinned(r worklowPinningResult, ma dl.Info(&checker.LogMessage{ Type: finding.FileTypeSource, Offset: checker.OffsetDefault, - Text: fmt.Sprintf("%s %s", "Third-party", maxResultMsg), + Text: fmt.Sprintf("all %s %ss are pinned", generateOwnerToDisplay(false), checker.DependencyUseTypeGHAction), }) } } - return score, nil + return score } diff --git a/checks/evaluation/pinned_dependencies_test.go b/checks/evaluation/pinned_dependencies_test.go index fd4c5090a5d..a0078bc2602 100644 --- a/checks/evaluation/pinned_dependencies_test.go +++ b/checks/evaluation/pinned_dependencies_test.go @@ -24,19 +24,15 @@ import ( scut "github.com/ossf/scorecard/v4/utests" ) -func Test_createReturnForIsGitHubActionsWorkflowPinned(t *testing.T) { +func Test_createReturnValuesForGitHubActionsWorkflowPinned(t *testing.T) { t.Parallel() //nolint type args struct { r worklowPinningResult dl *scut.TestDetailLogger } - type log struct { - text string - detailType checker.DetailType - } type want struct { - logs []log + logs []string score int } //nolint @@ -62,15 +58,9 @@ func Test_createReturnForIsGitHubActionsWorkflowPinned(t *testing.T) { }, want: want{ score: 10, - logs: []log{ - { - text: "GitHub-owned GitHubActions are pinned", - detailType: checker.DetailInfo, - }, - { - text: "Third-party GitHubActions are pinned", - detailType: checker.DetailInfo, - }, + logs: []string{ + "all GitHub-owned GitHubActions are pinned", + "all third-party GitHubActions are pinned", }, }, }, @@ -91,11 +81,8 @@ func Test_createReturnForIsGitHubActionsWorkflowPinned(t *testing.T) { }, want: want{ score: 2, - logs: []log{ - { - text: "GitHub-owned GitHubActions are pinned", - detailType: checker.DetailInfo, - }, + logs: []string{ + "all GitHub-owned GitHubActions are pinned", }, }, }, @@ -116,11 +103,8 @@ func Test_createReturnForIsGitHubActionsWorkflowPinned(t *testing.T) { }, want: want{ score: 8, - logs: []log{ - { - text: "Third-party GitHubActions are pinned", - detailType: checker.DetailInfo, - }, + logs: []string{ + "all third-party GitHubActions are pinned", }, }, }, @@ -141,16 +125,16 @@ func Test_createReturnForIsGitHubActionsWorkflowPinned(t *testing.T) { }, want: want{ score: 0, - logs: []log{}, + logs: []string{}, }, }, { - name: "no GitHub actions", + name: "no GitHub-owned actions and Third-party actions unpinned", args: args{ r: worklowPinningResult{ thirdParties: pinnedResult{ pinned: 0, - total: 0, + total: 1, }, gitHubOwned: pinnedResult{ pinned: 0, @@ -160,25 +144,36 @@ func Test_createReturnForIsGitHubActionsWorkflowPinned(t *testing.T) { dl: &scut.TestDetailLogger{}, }, want: want{ - score: -1, - logs: []log{ - { - text: "no GitHub-owned GitHubActions found", - detailType: checker.DetailDebug, + score: 2, + logs: []string{}, + }, + }, + { + name: "no Third-party actions and GitHub-owned actions unpinned", + args: args{ + r: worklowPinningResult{ + thirdParties: pinnedResult{ + pinned: 0, + total: 0, }, - { - text: "no Third-party GitHubActions found", - detailType: checker.DetailDebug, + gitHubOwned: pinnedResult{ + pinned: 0, + total: 1, }, }, + dl: &scut.TestDetailLogger{}, + }, + want: want{ + score: 8, + logs: []string{}, }, }, { - name: "no GitHub-owned actions", + name: "no GitHub-owned actions and Third-party actions pinned", args: args{ r: worklowPinningResult{ thirdParties: pinnedResult{ - pinned: 0, + pinned: 1, total: 1, }, gitHubOwned: pinnedResult{ @@ -189,17 +184,14 @@ func Test_createReturnForIsGitHubActionsWorkflowPinned(t *testing.T) { dl: &scut.TestDetailLogger{}, }, want: want{ - score: 2, - logs: []log{ - { - text: "no GitHub-owned GitHubActions found", - detailType: checker.DetailDebug, - }, + score: 10, + logs: []string{ + "all third-party GitHubActions are pinned", }, }, }, { - name: "no Third-party actions", + name: "no Third-party actions and GitHub-owned actions pinned", args: args{ r: worklowPinningResult{ thirdParties: pinnedResult{ @@ -207,19 +199,16 @@ func Test_createReturnForIsGitHubActionsWorkflowPinned(t *testing.T) { total: 0, }, gitHubOwned: pinnedResult{ - pinned: 0, + pinned: 1, total: 1, }, }, dl: &scut.TestDetailLogger{}, }, want: want{ - score: 8, - logs: []log{ - { - text: "no Third-party GitHubActions found", - detailType: checker.DetailDebug, - }, + score: 10, + logs: []string{ + "all GitHub-owned GitHubActions are pinned", }, }, }, @@ -228,16 +217,13 @@ func Test_createReturnForIsGitHubActionsWorkflowPinned(t *testing.T) { 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 := createReturnForIsGitHubActionsWorkflowPinned(tt.args.r, tt.args.dl) - if err != nil { - t.Errorf("error during createReturnForIsGitHubActionsWorkflowPinned: %v", err) - } + got := createReturnValuesForGitHubActionsWorkflowPinned(tt.args.r, tt.args.dl) if got != tt.want.score { t.Errorf("createReturnForIsGitHubActionsWorkflowPinned() = %v, want %v", got, tt.want.score) } for _, log := range tt.want.logs { isExpectedLog := func(logMessage checker.LogMessage, logType checker.DetailType) bool { - return logMessage.Text == log.text && logType == log.detailType + return logMessage.Text == log && logType == checker.DetailInfo } if !scut.ValidateLogMessage(isExpectedLog, tt.args.dl) { t.Errorf("test failed: log message not present: %+v", log) @@ -310,7 +296,7 @@ func Test_PinningDependencies(t *testing.T) { Error: nil, Score: 10, NumberOfWarn: 0, - NumberOfInfo: 8, + NumberOfInfo: 7, NumberOfDebug: 0, }, }, @@ -384,7 +370,7 @@ func Test_PinningDependencies(t *testing.T) { Score: 5, NumberOfWarn: 1, NumberOfInfo: 1, - NumberOfDebug: 6, + NumberOfDebug: 0, }, }, { @@ -406,7 +392,7 @@ func Test_PinningDependencies(t *testing.T) { Score: 0, NumberOfWarn: 1, NumberOfInfo: 0, - NumberOfDebug: 7, + NumberOfDebug: 0, }, }, { @@ -417,7 +403,7 @@ func Test_PinningDependencies(t *testing.T) { Score: -1, NumberOfWarn: 0, NumberOfInfo: 0, - NumberOfDebug: 8, + NumberOfDebug: 0, }, }, { @@ -434,7 +420,7 @@ func Test_PinningDependencies(t *testing.T) { Score: 10, NumberOfWarn: 0, NumberOfInfo: 1, - NumberOfDebug: 7, + NumberOfDebug: 0, }, }, { @@ -451,7 +437,7 @@ func Test_PinningDependencies(t *testing.T) { Score: 0, NumberOfWarn: 1, NumberOfInfo: 0, - NumberOfDebug: 7, + NumberOfDebug: 0, }, }, { @@ -468,7 +454,7 @@ func Test_PinningDependencies(t *testing.T) { Score: -1, NumberOfWarn: 0, NumberOfInfo: 0, - NumberOfDebug: 9, + NumberOfDebug: 1, }, }, { @@ -484,7 +470,7 @@ func Test_PinningDependencies(t *testing.T) { Score: -1, NumberOfWarn: 0, NumberOfInfo: 0, - NumberOfDebug: 9, + NumberOfDebug: 1, }, }, { @@ -508,33 +494,15 @@ func Test_PinningDependencies(t *testing.T) { Score: -1, NumberOfWarn: 0, NumberOfInfo: 0, - NumberOfDebug: 9, + NumberOfDebug: 1, }, }, - // TODO: choco installs should score for Pinned-Dependencies - // { - // name: "unpinned choco install", - // dependencies: []checker.Dependency{ - // { - // Location: &checker.File{}, - // Type: checker.DependencyUseTypeChocoCommand, - // Pinned: asBoolPointer(false), - // }, - // }, - // expected: scut.TestReturn{ - // Error: nil, - // Score: 0, - // NumberOfWarn: 1, - // NumberOfInfo: 7, - // NumberOfDebug: 0, - // }, - // }, { - name: "unpinned Dockerfile container image", + name: "unpinned choco install", dependencies: []checker.Dependency{ { Location: &checker.File{}, - Type: checker.DependencyUseTypeDockerfileContainerImage, + Type: checker.DependencyUseTypeChocoCommand, Pinned: asBoolPointer(false), }, }, @@ -543,20 +511,16 @@ func Test_PinningDependencies(t *testing.T) { Score: 0, NumberOfWarn: 1, NumberOfInfo: 0, - NumberOfDebug: 7, + NumberOfDebug: 0, }, }, - // TODO: Due to a bug download then run is score twice in shell scripts - // and Dockerfile, the NumberOfInfo should be 7 { - name: "unpinned download then run in Dockerfile", + name: "unpinned Dockerfile container image", dependencies: []checker.Dependency{ { - Location: &checker.File{ - Path: "Dockerfile", - }, - Type: checker.DependencyUseTypeDownloadThenRun, - Pinned: asBoolPointer(false), + Location: &checker.File{}, + Type: checker.DependencyUseTypeDockerfileContainerImage, + Pinned: asBoolPointer(false), }, }, expected: scut.TestReturn{ @@ -564,20 +528,16 @@ func Test_PinningDependencies(t *testing.T) { Score: 0, NumberOfWarn: 1, NumberOfInfo: 0, - NumberOfDebug: 6, + NumberOfDebug: 0, }, }, - // TODO: Due to a bug download then run is score twice in shell scripts - // and Dockerfile, the NumberOfInfo should be 7 { - name: "unpinned download then run in shell scripts", + name: "unpinned download then run", dependencies: []checker.Dependency{ { - Location: &checker.File{ - Path: "bash.sh", - }, - Type: checker.DependencyUseTypeDownloadThenRun, - Pinned: asBoolPointer(false), + Location: &checker.File{}, + Type: checker.DependencyUseTypeDownloadThenRun, + Pinned: asBoolPointer(false), }, }, expected: scut.TestReturn{ @@ -585,7 +545,7 @@ func Test_PinningDependencies(t *testing.T) { Score: 0, NumberOfWarn: 1, NumberOfInfo: 0, - NumberOfDebug: 6, + NumberOfDebug: 0, }, }, { @@ -602,7 +562,7 @@ func Test_PinningDependencies(t *testing.T) { Score: 0, NumberOfWarn: 1, NumberOfInfo: 0, - NumberOfDebug: 7, + NumberOfDebug: 0, }, }, { @@ -619,27 +579,26 @@ func Test_PinningDependencies(t *testing.T) { Score: 0, NumberOfWarn: 1, NumberOfInfo: 0, - NumberOfDebug: 7, + NumberOfDebug: 0, + }, + }, + { + name: "unpinned nuget install", + dependencies: []checker.Dependency{ + { + Location: &checker.File{}, + Type: checker.DependencyUseTypeNugetCommand, + Pinned: asBoolPointer(false), + }, + }, + expected: scut.TestReturn{ + Error: nil, + Score: 0, + NumberOfWarn: 1, + NumberOfInfo: 0, + NumberOfDebug: 0, }, }, - // TODO: nuget installs should score for Pinned-Dependencies - // { - // name: "unpinned nuget install", - // dependencies: []checker.Dependency{ - // { - // Location: &checker.File{}, - // Type: checker.DependencyUseTypeNugetCommand, - // Pinned: asBoolPointer(false), - // }, - // }, - // expected: scut.TestReturn{ - // Error: nil, - // Score: 0, - // NumberOfWarn: 1, - // NumberOfInfo: 7, - // NumberOfDebug: 0, - // }, - // }, { name: "unpinned pip install", dependencies: []checker.Dependency{ @@ -654,7 +613,7 @@ func Test_PinningDependencies(t *testing.T) { Score: 0, NumberOfWarn: 1, NumberOfInfo: 0, - NumberOfDebug: 7, + NumberOfDebug: 0, }, }, { @@ -676,7 +635,7 @@ func Test_PinningDependencies(t *testing.T) { Score: 0, NumberOfWarn: 2, NumberOfInfo: 0, - NumberOfDebug: 7, + NumberOfDebug: 0, }, }, { @@ -698,7 +657,7 @@ func Test_PinningDependencies(t *testing.T) { Score: 0, NumberOfWarn: 2, NumberOfInfo: 0, - NumberOfDebug: 6, + NumberOfDebug: 0, }, }, } @@ -729,19 +688,15 @@ func Test_createReturnValues(t *testing.T) { dl *scut.TestDetailLogger t checker.DependencyUseType } + type want struct { + score int + log string + } tests := []struct { name string args args - want int + want want }{ - { - name: "no dependencies", - args: args{ - t: checker.DependencyUseTypePipCommand, - dl: &scut.TestDetailLogger{}, - }, - want: -1, - }, { name: "all dependencies pinned", args: args{ @@ -754,7 +709,10 @@ func Test_createReturnValues(t *testing.T) { }, }, }, - want: 10, + want: want{ + score: 10, + log: "all pipCommands are pinned", + }, }, { name: "all dependencies unpinned", @@ -768,7 +726,9 @@ func Test_createReturnValues(t *testing.T) { }, }, }, - want: 0, + want: want{ + score: 0, + }, }, { name: "1 or more dependencies unpinned", @@ -782,39 +742,29 @@ func Test_createReturnValues(t *testing.T) { }, }, }, - want: 0, + want: want{ + score: 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, "all dependencies are pinned", - "no dependencies found", tt.args.dl) - if err != nil { - t.Errorf("error during createReturnValues: %v", err) - } - if got != tt.want { + got := createReturnValues(tt.args.pr, tt.args.t, tt.args.dl) + if got != tt.want.score { t.Errorf("createReturnValues() = %v, want %v", got, tt.want) } - switch tt.want { - case -1: - isExpectedLog := func(logMessage checker.LogMessage, logType checker.DetailType) bool { - return logMessage.Text == "no dependencies found" && logType == checker.DetailDebug - } - if !scut.ValidateLogMessage(isExpectedLog, tt.args.dl) { - t.Errorf("test failed: log message not present: %+v", "no dependencies found") - } - case 0: + if tt.want.score != 10 { return - case 10: - isExpectedLog := func(logMessage checker.LogMessage, logType checker.DetailType) bool { - return logMessage.Text == "all dependencies are pinned" && logType == checker.DetailInfo - } - if !scut.ValidateLogMessage(isExpectedLog, tt.args.dl) { - t.Errorf("test failed: log message not present: %+v", "all dependencies are pinned") - } + } + + isExpectedLog := func(logMessage checker.LogMessage, logType checker.DetailType) bool { + return logMessage.Text == tt.want.log && logType == checker.DetailInfo + } + if !scut.ValidateLogMessage(isExpectedLog, tt.args.dl) { + t.Errorf("test failed: log message not present: %+v", tt.want.log) } }) } diff --git a/e2e/pinned_dependencies_test.go b/e2e/pinned_dependencies_test.go index 03d249304af..b7c49543751 100644 --- a/e2e/pinned_dependencies_test.go +++ b/e2e/pinned_dependencies_test.go @@ -52,7 +52,7 @@ var _ = Describe("E2E TEST:"+checks.CheckPinnedDependencies, func() { Score: 3, NumberOfWarn: 139, NumberOfInfo: 1, - NumberOfDebug: 2, + NumberOfDebug: 0, } result := checks.PinningDependencies(&req) Expect(scut.ValidateTestReturn(nil, "dependencies check", &expected, &result, &dl)).Should(BeTrue()) @@ -77,7 +77,7 @@ var _ = Describe("E2E TEST:"+checks.CheckPinnedDependencies, func() { Score: 3, NumberOfWarn: 139, NumberOfInfo: 1, - NumberOfDebug: 2, + NumberOfDebug: 0, } result := checks.PinningDependencies(&req) Expect(scut.ValidateTestReturn(nil, "dependencies check", &expected, &result, &dl)).Should(BeTrue()) @@ -113,7 +113,7 @@ var _ = Describe("E2E TEST:"+checks.CheckPinnedDependencies, func() { Score: 3, NumberOfWarn: 139, NumberOfInfo: 1, - NumberOfDebug: 2, + NumberOfDebug: 0, } result := checks.PinningDependencies(&req) Expect(scut.ValidateTestReturn(nil, "dependencies check", &expected, &result, &dl)).Should(BeTrue()) From e52be0bba39bcad96467ed32bbfdb95490d43e71 Mon Sep 17 00:00:00 2001 From: Gabriela Gutierrez Date: Mon, 4 Sep 2023 16:23:06 +0000 Subject: [PATCH 28/46] 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 --- checker/check_result.go | 22 ++++++ checks/evaluation/pinned_dependencies.go | 89 ++++++++++-------------- 2 files changed, 59 insertions(+), 52 deletions(-) diff --git a/checker/check_result.go b/checker/check_result.go index 5e0dd21946f..d20f6972a1d 100644 --- a/checker/check_result.go +++ b/checker/check_result.go @@ -88,6 +88,14 @@ type LogMessage struct { Remediation *rule.Remediation // Remediation information, if any. } +// ProportionalScoreWithWeight is a structure that contains +// the fields to calculate weighted proportional scores. +type ProportionalScoreWithWeight struct { + Success int + Total int + Weight int +} + // CreateProportionalScore creates a proportional score. func CreateProportionalScore(success, total int) int { if total == 0 { @@ -97,6 +105,20 @@ func CreateProportionalScore(success, total int) int { return int(math.Min(float64(MaxResultScore*success/total), float64(MaxResultScore))) } +// CreateProportionalScoreWithWeight creates the proportional score +// between multiple successes over the total, but some proportions +// are worth more. +func CreateProportionalScoreWithWeight(scores ...*ProportionalScoreWithWeight) int { + ws := 0 + wt := 0 + for _, score := range scores { + ws += score.Success * score.Weight + wt += score.Total * score.Weight + } + + return int(math.Min(float64(MaxResultScore*ws/wt), float64(MaxResultScore))) +} + // AggregateScores adds up all scores // and normalizes the result. // Each score contributes equally. diff --git a/checks/evaluation/pinned_dependencies.go b/checks/evaluation/pinned_dependencies.go index de27c3726d8..3b468ae4dfe 100644 --- a/checks/evaluation/pinned_dependencies.go +++ b/checks/evaluation/pinned_dependencies.go @@ -20,7 +20,6 @@ import ( "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" ) @@ -102,27 +101,26 @@ func PinningDependencies(name string, c *checker.CheckRequest, } // Generate scores and Info results. - var scores []int + var scores []*checker.ProportionalScoreWithWeight // Go through all dependency types // GitHub Actions need to be handled separately since they are not in pr - // We should not score GitHub Actions if there are no GitHub Actions - if wp.gitHubOwned.total != 0 || wp.thirdParties.total != 0 { - score := createReturnValuesForGitHubActionsWorkflowPinned(wp, dl) - scores = append(scores, score) - } + scores = append(scores, createScoreForGitHubActionsWorkflow(&wp)...) // Only exisiting dependencies will be found in pr // We will only score the ecossystem if there are dependencies // This results in only existing ecossystems being included in the final score for t := range pr { - score := createReturnValues(pr, t, dl) - scores = append(scores, score) + scores = append(scores, &checker.ProportionalScoreWithWeight{ + Success: pr[t].pinned, + Total: pr[t].total, + Weight: 10, + }) } if len(scores) == 0 { return checker.CreateInconclusiveResult(name, "no dependencies found") } - score := checker.AggregateScores(scores...) + score := checker.CreateProportionalScoreWithWeight(scores...) if score == checker.MaxResultScore { return checker.CreateMaxScoreResult(name, "all dependencies are pinned") @@ -193,51 +191,38 @@ func addWorkflowPinnedResult(rr *checker.Dependency, w *worklowPinningResult, is } } -func createReturnValues(pr map[checker.DependencyUseType]pinnedResult, - t checker.DependencyUseType, dl checker.DetailLogger, -) int { - // If there are zero dependencies of this type, it will not reach this function, - // so its not gonna be included it in the aggregated score. - // If all dependencies of this type are pinned, it will get a maximum score. - // If 1 or more dependencies of this type are unpinned, it will get a minimum score. - //nolint - r := pr[t] - - if r.total == r.pinned { - dl.Info(&checker.LogMessage{ - Text: fmt.Sprintf("all %ss are pinned", t), - }) - return checker.MaxResultScore +func createScoreForGitHubActionsWorkflow(wp *worklowPinningResult) []*checker.ProportionalScoreWithWeight { + if wp.gitHubOwned.total == 0 && wp.thirdParties.total == 0 { + return []*checker.ProportionalScoreWithWeight{} } - - return checker.MinResultScore -} - -func createReturnValuesForGitHubActionsWorkflowPinned(r worklowPinningResult, dl checker.DetailLogger, -) int { - score := checker.MinResultScore - - if r.gitHubOwned.total == r.gitHubOwned.pinned { - score += 2 - if r.gitHubOwned.total != 0 { - dl.Info(&checker.LogMessage{ - Type: finding.FileTypeSource, - Offset: checker.OffsetDefault, - Text: fmt.Sprintf("all %s %ss are pinned", generateOwnerToDisplay(true), checker.DependencyUseTypeGHAction), - }) + if wp.gitHubOwned.total != 0 && wp.thirdParties.total != 0 { + return []*checker.ProportionalScoreWithWeight{ + { + Success: wp.gitHubOwned.pinned, + Total: wp.gitHubOwned.total, + Weight: 8, + }, + { + Success: wp.thirdParties.pinned, + Total: wp.thirdParties.total, + Weight: 2, + }, } } - - if r.thirdParties.total == r.thirdParties.pinned { - score += 8 - if r.thirdParties.total != 0 { - dl.Info(&checker.LogMessage{ - Type: finding.FileTypeSource, - Offset: checker.OffsetDefault, - Text: fmt.Sprintf("all %s %ss are pinned", generateOwnerToDisplay(false), checker.DependencyUseTypeGHAction), - }) + if wp.gitHubOwned.total != 0 { + return []*checker.ProportionalScoreWithWeight{ + { + Success: wp.gitHubOwned.pinned, + Total: wp.gitHubOwned.total, + Weight: 10, + }, } } - - return score + return []*checker.ProportionalScoreWithWeight{ + { + Success: wp.thirdParties.pinned, + Total: wp.thirdParties.total, + Weight: 10, + }, + } } From 29be82770aad31079abde8af64a39d5c9c750c3c Mon Sep 17 00:00:00 2001 From: Gabriela Gutierrez Date: Mon, 4 Sep 2023 18:28:13 +0000 Subject: [PATCH 29/46] fix: GHA weights in proportional score Signed-off-by: Gabriela Gutierrez --- checks/evaluation/pinned_dependencies.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/checks/evaluation/pinned_dependencies.go b/checks/evaluation/pinned_dependencies.go index 3b468ae4dfe..a1d9048abeb 100644 --- a/checks/evaluation/pinned_dependencies.go +++ b/checks/evaluation/pinned_dependencies.go @@ -200,12 +200,12 @@ func createScoreForGitHubActionsWorkflow(wp *worklowPinningResult) []*checker.Pr { Success: wp.gitHubOwned.pinned, Total: wp.gitHubOwned.total, - Weight: 8, + Weight: 2, }, { Success: wp.thirdParties.pinned, Total: wp.thirdParties.total, - Weight: 2, + Weight: 8, }, } } From 5dc6464c2deb7f7e1c81b7e9ab4da91ce063527a Mon Sep 17 00:00:00 2001 From: Gabriela Gutierrez Date: Mon, 4 Sep 2023 19:10:48 +0000 Subject: [PATCH 30/46] 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 --- checks/evaluation/pinned_dependencies_test.go | 498 ++++++++++-------- 1 file changed, 272 insertions(+), 226 deletions(-) diff --git a/checks/evaluation/pinned_dependencies_test.go b/checks/evaluation/pinned_dependencies_test.go index a0078bc2602..e0c25daea11 100644 --- a/checks/evaluation/pinned_dependencies_test.go +++ b/checks/evaluation/pinned_dependencies_test.go @@ -24,191 +24,191 @@ import ( 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 - dl *scut.TestDetailLogger - } - type want struct { - logs []string - score int - } - //nolint tests := []struct { - name string - args args - want want + name string + r worklowPinningResult + scores []*checker.ProportionalScoreWithWeight }{ { name: "GitHub-owned and Third-Party actions pinned", - args: args{ - r: worklowPinningResult{ - thirdParties: pinnedResult{ - pinned: 1, - total: 1, - }, - gitHubOwned: pinnedResult{ - pinned: 1, - total: 1, - }, + r: worklowPinningResult{ + gitHubOwned: pinnedResult{ + pinned: 1, + total: 1, + }, + thirdParties: pinnedResult{ + pinned: 1, + total: 1, }, - dl: &scut.TestDetailLogger{}, }, - want: want{ - score: 10, - logs: []string{ - "all GitHub-owned GitHubActions are pinned", - "all third-party GitHubActions are pinned", + scores: []*checker.ProportionalScoreWithWeight{ + { + Success: 1, + Total: 1, + Weight: 2, + }, + { + Success: 1, + Total: 1, + Weight: 8, }, }, }, { name: "only GitHub-owned actions pinned", - args: args{ - r: worklowPinningResult{ - thirdParties: pinnedResult{ - pinned: 0, - total: 1, - }, - gitHubOwned: pinnedResult{ - pinned: 1, - total: 1, - }, + r: worklowPinningResult{ + gitHubOwned: pinnedResult{ + pinned: 1, + total: 1, + }, + thirdParties: pinnedResult{ + pinned: 0, + total: 1, }, - dl: &scut.TestDetailLogger{}, }, - want: want{ - score: 2, - logs: []string{ - "all GitHub-owned GitHubActions are pinned", + scores: []*checker.ProportionalScoreWithWeight{ + { + Success: 1, + Total: 1, + Weight: 2, + }, + { + Success: 0, + Total: 1, + Weight: 8, }, }, }, { name: "only Third-Party actions pinned", - args: args{ - r: worklowPinningResult{ - thirdParties: pinnedResult{ - pinned: 1, - total: 1, - }, - gitHubOwned: pinnedResult{ - pinned: 0, - total: 1, - }, + r: worklowPinningResult{ + gitHubOwned: pinnedResult{ + pinned: 0, + total: 1, + }, + thirdParties: pinnedResult{ + pinned: 1, + total: 1, }, - dl: &scut.TestDetailLogger{}, }, - want: want{ - score: 8, - logs: []string{ - "all third-party GitHubActions are pinned", + scores: []*checker.ProportionalScoreWithWeight{ + { + Success: 0, + Total: 1, + Weight: 2, + }, + { + Success: 1, + Total: 1, + Weight: 8, }, }, }, { name: "no GitHub actions pinned", - args: args{ - r: worklowPinningResult{ - thirdParties: pinnedResult{ - pinned: 0, - total: 1, - }, - gitHubOwned: pinnedResult{ - pinned: 0, - total: 1, - }, + r: worklowPinningResult{ + gitHubOwned: pinnedResult{ + pinned: 0, + total: 1, + }, + thirdParties: pinnedResult{ + pinned: 0, + total: 1, }, - dl: &scut.TestDetailLogger{}, }, - want: want{ - score: 0, - logs: []string{}, + scores: []*checker.ProportionalScoreWithWeight{ + { + Success: 0, + Total: 1, + Weight: 2, + }, + { + Success: 0, + Total: 1, + Weight: 8, + }, }, }, { name: "no GitHub-owned actions and Third-party actions unpinned", - args: args{ - r: worklowPinningResult{ - thirdParties: pinnedResult{ - pinned: 0, - total: 1, - }, - gitHubOwned: pinnedResult{ - pinned: 0, - total: 0, - }, + r: worklowPinningResult{ + gitHubOwned: pinnedResult{ + pinned: 0, + total: 0, + }, + thirdParties: pinnedResult{ + pinned: 0, + total: 1, }, - dl: &scut.TestDetailLogger{}, }, - want: want{ - score: 2, - logs: []string{}, + scores: []*checker.ProportionalScoreWithWeight{ + { + Success: 0, + Total: 1, + Weight: 10, + }, }, }, { name: "no Third-party actions and GitHub-owned actions unpinned", - args: args{ - r: worklowPinningResult{ - thirdParties: pinnedResult{ - pinned: 0, - total: 0, - }, - gitHubOwned: pinnedResult{ - pinned: 0, - total: 1, - }, + r: worklowPinningResult{ + gitHubOwned: pinnedResult{ + pinned: 0, + total: 1, + }, + thirdParties: pinnedResult{ + pinned: 0, + total: 0, }, - dl: &scut.TestDetailLogger{}, }, - want: want{ - score: 8, - logs: []string{}, + scores: []*checker.ProportionalScoreWithWeight{ + { + Success: 0, + Total: 1, + Weight: 10, + }, }, }, { name: "no GitHub-owned actions and Third-party actions pinned", - args: args{ - r: worklowPinningResult{ - thirdParties: pinnedResult{ - pinned: 1, - total: 1, - }, - gitHubOwned: pinnedResult{ - pinned: 0, - total: 0, - }, + r: worklowPinningResult{ + gitHubOwned: pinnedResult{ + pinned: 0, + total: 0, + }, + thirdParties: pinnedResult{ + pinned: 1, + total: 1, }, - dl: &scut.TestDetailLogger{}, }, - want: want{ - score: 10, - logs: []string{ - "all third-party GitHubActions are pinned", + scores: []*checker.ProportionalScoreWithWeight{ + { + Success: 1, + Total: 1, + Weight: 10, }, }, }, { name: "no Third-party actions and GitHub-owned actions pinned", - args: args{ - r: worklowPinningResult{ - thirdParties: pinnedResult{ - pinned: 0, - total: 0, - }, - gitHubOwned: pinnedResult{ - pinned: 1, - total: 1, - }, + r: worklowPinningResult{ + gitHubOwned: pinnedResult{ + pinned: 1, + total: 1, + }, + thirdParties: pinnedResult{ + pinned: 0, + total: 0, }, - dl: &scut.TestDetailLogger{}, }, - want: want{ - score: 10, - logs: []string{ - "all GitHub-owned GitHubActions are pinned", + scores: []*checker.ProportionalScoreWithWeight{ + { + Success: 1, + Total: 1, + Weight: 10, }, }, }, @@ -217,17 +217,10 @@ func Test_createReturnValuesForGitHubActionsWorkflowPinned(t *testing.T) { 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 := createReturnValuesForGitHubActionsWorkflowPinned(tt.args.r, tt.args.dl) - if got != tt.want.score { - t.Errorf("createReturnForIsGitHubActionsWorkflowPinned() = %v, want %v", got, tt.want.score) - } - for _, log := range tt.want.logs { - isExpectedLog := func(logMessage checker.LogMessage, logType checker.DetailType) bool { - return logMessage.Text == log && logType == checker.DetailInfo - } - if !scut.ValidateLogMessage(isExpectedLog, tt.args.dl) { - t.Errorf("test failed: log message not present: %+v", log) - } + actual := createScoreForGitHubActionsWorkflow(&tt.r) + diff := cmp.Diff(tt.scores, actual) + if diff != "" { + t.Errorf("createScoreForGitHubActionsWorkflow (-want,+got) %+v", diff) } }) } @@ -296,7 +289,7 @@ func Test_PinningDependencies(t *testing.T) { Error: nil, Score: 10, NumberOfWarn: 0, - NumberOfInfo: 7, + NumberOfInfo: 0, NumberOfDebug: 0, }, }, @@ -369,7 +362,7 @@ func Test_PinningDependencies(t *testing.T) { Error: nil, Score: 5, NumberOfWarn: 1, - NumberOfInfo: 1, + NumberOfInfo: 0, NumberOfDebug: 0, }, }, @@ -389,7 +382,7 @@ func Test_PinningDependencies(t *testing.T) { }, expected: scut.TestReturn{ Error: nil, - Score: 0, + Score: 5, NumberOfWarn: 1, NumberOfInfo: 0, NumberOfDebug: 0, @@ -419,7 +412,7 @@ func Test_PinningDependencies(t *testing.T) { Error: nil, Score: 10, NumberOfWarn: 0, - NumberOfInfo: 1, + NumberOfInfo: 0, NumberOfDebug: 0, }, }, @@ -660,111 +653,164 @@ func Test_PinningDependencies(t *testing.T) { 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 - } - type want struct { - score int - log string - } - tests := []struct { - name string - args args - want want - }{ { - name: "all dependencies pinned", - args: args{ - t: checker.DependencyUseTypePipCommand, - dl: &scut.TestDetailLogger{}, - pr: map[checker.DependencyUseType]pinnedResult{ - checker.DependencyUseTypePipCommand: { - pinned: 1, - total: 1, + name: "GitHub Actions ecossystem with GitHub-owned pinned", + dependencies: []checker.Dependency{ + { + Location: &checker.File{ + Snippet: "actions/checkout@a81bbbf8298c0fa03ea29cdc473d45769f953675", }, + Type: checker.DependencyUseTypeGHAction, + Pinned: asBoolPointer(true), }, }, - want: want{ - score: 10, - log: "all pipCommands are pinned", + expected: scut.TestReturn{ + Error: nil, + Score: 10, + NumberOfWarn: 0, + NumberOfInfo: 0, + NumberOfDebug: 0, }, }, { - name: "all dependencies unpinned", - args: args{ - t: checker.DependencyUseTypePipCommand, - dl: &scut.TestDetailLogger{}, - pr: map[checker.DependencyUseType]pinnedResult{ - checker.DependencyUseTypePipCommand: { - pinned: 0, - total: 1, + name: "GitHub Actions ecossystem with third-party pinned", + dependencies: []checker.Dependency{ + { + Location: &checker.File{ + Snippet: "other/checkout@a81bbbf8298c0fa03ea29cdc473d45769f953675", }, + Type: checker.DependencyUseTypeGHAction, + Pinned: asBoolPointer(true), }, }, - want: want{ - score: 0, + expected: scut.TestReturn{ + Error: nil, + Score: 10, + NumberOfWarn: 0, + NumberOfInfo: 0, + NumberOfDebug: 0, }, }, { - name: "1 or more dependencies unpinned", - args: args{ - t: checker.DependencyUseTypePipCommand, - dl: &scut.TestDetailLogger{}, - pr: map[checker.DependencyUseType]pinnedResult{ - checker.DependencyUseTypePipCommand: { - pinned: 1, - total: 2, + name: "GitHub Actions ecossystem 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), }, }, - want: want{ - score: 0, + expected: scut.TestReturn{ + Error: nil, + Score: 10, + NumberOfWarn: 0, + NumberOfInfo: 0, + NumberOfDebug: 0, + }, + }, + { + name: "GitHub Actions ecossystem 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: 0, + NumberOfDebug: 0, + }, + }, + { + name: "GitHub Actions ecossystem 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: 0, + NumberOfDebug: 0, + }, + }, + { + name: "GitHub Actions ecossystem 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: 0, + NumberOfDebug: 0, }, }, } + for _, tt := range tests { tt := tt t.Run(tt.name, func(t *testing.T) { t.Parallel() - got := createReturnValues(tt.args.pr, tt.args.t, tt.args.dl) - if got != tt.want.score { - t.Errorf("createReturnValues() = %v, want %v", got, tt.want) - } - if tt.want.score != 10 { - return - } + dl := scut.TestDetailLogger{} + c := checker.CheckRequest{Dlogger: &dl} + actual := PinningDependencies("checkname", &c, + &checker.PinningDependenciesData{ + Dependencies: tt.dependencies, + }) - isExpectedLog := func(logMessage checker.LogMessage, logType checker.DetailType) bool { - return logMessage.Text == tt.want.log && logType == checker.DetailInfo - } - if !scut.ValidateLogMessage(isExpectedLog, tt.args.dl) { - t.Errorf("test failed: log message not present: %+v", tt.want.log) + if !scut.ValidateTestReturn(t, tt.name, &tt.expected, &actual, &dl) { + t.Fail() } }) } From e8667cc2a2b111cc799d00fd863c75d81b031e25 Mon Sep 17 00:00:00 2001 From: Gabriela Gutierrez Date: Mon, 4 Sep 2023 19:44:31 +0000 Subject: [PATCH 31/46] 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 --- e2e/pinned_dependencies_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/e2e/pinned_dependencies_test.go b/e2e/pinned_dependencies_test.go index b7c49543751..8ba87a7a67f 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: 3, + Score: 2, NumberOfWarn: 139, - NumberOfInfo: 1, + NumberOfInfo: 0, NumberOfDebug: 0, } result := checks.PinningDependencies(&req) @@ -74,9 +74,9 @@ var _ = Describe("E2E TEST:"+checks.CheckPinnedDependencies, func() { } expected := scut.TestReturn{ Error: nil, - Score: 3, + Score: 2, NumberOfWarn: 139, - NumberOfInfo: 1, + NumberOfInfo: 0, NumberOfDebug: 0, } result := checks.PinningDependencies(&req) @@ -110,9 +110,9 @@ var _ = Describe("E2E TEST:"+checks.CheckPinnedDependencies, func() { } expected := scut.TestReturn{ Error: nil, - Score: 3, + Score: 2, NumberOfWarn: 139, - NumberOfInfo: 1, + NumberOfInfo: 0, NumberOfDebug: 0, } result := checks.PinningDependencies(&req) From 407773c54d8605522fb15675028138c7fc9176d8 Mon Sep 17 00:00:00 2001 From: Gabriela Gutierrez Date: Wed, 6 Sep 2023 19:26:49 +0000 Subject: [PATCH 32/46] refactor: Rename to ProportionalScoreWeighted Signed-off-by: Gabriela Gutierrez --- checker/check_result.go | 8 ++++---- checks/evaluation/pinned_dependencies.go | 16 ++++++++-------- checks/evaluation/pinned_dependencies_test.go | 18 +++++++++--------- 3 files changed, 21 insertions(+), 21 deletions(-) diff --git a/checker/check_result.go b/checker/check_result.go index d20f6972a1d..293c8ddaaf0 100644 --- a/checker/check_result.go +++ b/checker/check_result.go @@ -88,9 +88,9 @@ type LogMessage struct { Remediation *rule.Remediation // Remediation information, if any. } -// ProportionalScoreWithWeight is a structure that contains +// ProportionalScoreWeighted is a structure that contains // the fields to calculate weighted proportional scores. -type ProportionalScoreWithWeight struct { +type ProportionalScoreWeighted struct { Success int Total int Weight int @@ -105,10 +105,10 @@ func CreateProportionalScore(success, total int) int { return int(math.Min(float64(MaxResultScore*success/total), float64(MaxResultScore))) } -// CreateProportionalScoreWithWeight creates the proportional score +// CreateProportionalScoreWeighted creates the proportional score // between multiple successes over the total, but some proportions // are worth more. -func CreateProportionalScoreWithWeight(scores ...*ProportionalScoreWithWeight) int { +func CreateProportionalScoreWeighted(scores ...*ProportionalScoreWeighted) int { ws := 0 wt := 0 for _, score := range scores { diff --git a/checks/evaluation/pinned_dependencies.go b/checks/evaluation/pinned_dependencies.go index a1d9048abeb..a303b2587c2 100644 --- a/checks/evaluation/pinned_dependencies.go +++ b/checks/evaluation/pinned_dependencies.go @@ -101,7 +101,7 @@ func PinningDependencies(name string, c *checker.CheckRequest, } // Generate scores and Info results. - var scores []*checker.ProportionalScoreWithWeight + 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)...) @@ -109,7 +109,7 @@ func PinningDependencies(name string, c *checker.CheckRequest, // We will only score the ecossystem if there are dependencies // This results in only existing ecossystems being included in the final score for t := range pr { - scores = append(scores, &checker.ProportionalScoreWithWeight{ + scores = append(scores, &checker.ProportionalScoreWeighted{ Success: pr[t].pinned, Total: pr[t].total, Weight: 10, @@ -120,7 +120,7 @@ func PinningDependencies(name string, c *checker.CheckRequest, return checker.CreateInconclusiveResult(name, "no dependencies found") } - score := checker.CreateProportionalScoreWithWeight(scores...) + score := checker.CreateProportionalScoreWeighted(scores...) if score == checker.MaxResultScore { return checker.CreateMaxScoreResult(name, "all dependencies are pinned") @@ -191,12 +191,12 @@ func addWorkflowPinnedResult(rr *checker.Dependency, w *worklowPinningResult, is } } -func createScoreForGitHubActionsWorkflow(wp *worklowPinningResult) []*checker.ProportionalScoreWithWeight { +func createScoreForGitHubActionsWorkflow(wp *worklowPinningResult) []*checker.ProportionalScoreWeighted { if wp.gitHubOwned.total == 0 && wp.thirdParties.total == 0 { - return []*checker.ProportionalScoreWithWeight{} + return []*checker.ProportionalScoreWeighted{} } if wp.gitHubOwned.total != 0 && wp.thirdParties.total != 0 { - return []*checker.ProportionalScoreWithWeight{ + return []*checker.ProportionalScoreWeighted{ { Success: wp.gitHubOwned.pinned, Total: wp.gitHubOwned.total, @@ -210,7 +210,7 @@ func createScoreForGitHubActionsWorkflow(wp *worklowPinningResult) []*checker.Pr } } if wp.gitHubOwned.total != 0 { - return []*checker.ProportionalScoreWithWeight{ + return []*checker.ProportionalScoreWeighted{ { Success: wp.gitHubOwned.pinned, Total: wp.gitHubOwned.total, @@ -218,7 +218,7 @@ func createScoreForGitHubActionsWorkflow(wp *worklowPinningResult) []*checker.Pr }, } } - return []*checker.ProportionalScoreWithWeight{ + return []*checker.ProportionalScoreWeighted{ { Success: wp.thirdParties.pinned, Total: wp.thirdParties.total, diff --git a/checks/evaluation/pinned_dependencies_test.go b/checks/evaluation/pinned_dependencies_test.go index e0c25daea11..0dd4bdf307f 100644 --- a/checks/evaluation/pinned_dependencies_test.go +++ b/checks/evaluation/pinned_dependencies_test.go @@ -30,7 +30,7 @@ func Test_createScoreForGitHubActionsWorkflow(t *testing.T) { tests := []struct { name string r worklowPinningResult - scores []*checker.ProportionalScoreWithWeight + scores []*checker.ProportionalScoreWeighted }{ { name: "GitHub-owned and Third-Party actions pinned", @@ -44,7 +44,7 @@ func Test_createScoreForGitHubActionsWorkflow(t *testing.T) { total: 1, }, }, - scores: []*checker.ProportionalScoreWithWeight{ + scores: []*checker.ProportionalScoreWeighted{ { Success: 1, Total: 1, @@ -69,7 +69,7 @@ func Test_createScoreForGitHubActionsWorkflow(t *testing.T) { total: 1, }, }, - scores: []*checker.ProportionalScoreWithWeight{ + scores: []*checker.ProportionalScoreWeighted{ { Success: 1, Total: 1, @@ -94,7 +94,7 @@ func Test_createScoreForGitHubActionsWorkflow(t *testing.T) { total: 1, }, }, - scores: []*checker.ProportionalScoreWithWeight{ + scores: []*checker.ProportionalScoreWeighted{ { Success: 0, Total: 1, @@ -119,7 +119,7 @@ func Test_createScoreForGitHubActionsWorkflow(t *testing.T) { total: 1, }, }, - scores: []*checker.ProportionalScoreWithWeight{ + scores: []*checker.ProportionalScoreWeighted{ { Success: 0, Total: 1, @@ -144,7 +144,7 @@ func Test_createScoreForGitHubActionsWorkflow(t *testing.T) { total: 1, }, }, - scores: []*checker.ProportionalScoreWithWeight{ + scores: []*checker.ProportionalScoreWeighted{ { Success: 0, Total: 1, @@ -164,7 +164,7 @@ func Test_createScoreForGitHubActionsWorkflow(t *testing.T) { total: 0, }, }, - scores: []*checker.ProportionalScoreWithWeight{ + scores: []*checker.ProportionalScoreWeighted{ { Success: 0, Total: 1, @@ -184,7 +184,7 @@ func Test_createScoreForGitHubActionsWorkflow(t *testing.T) { total: 1, }, }, - scores: []*checker.ProportionalScoreWithWeight{ + scores: []*checker.ProportionalScoreWeighted{ { Success: 1, Total: 1, @@ -204,7 +204,7 @@ func Test_createScoreForGitHubActionsWorkflow(t *testing.T) { total: 0, }, }, - scores: []*checker.ProportionalScoreWithWeight{ + scores: []*checker.ProportionalScoreWeighted{ { Success: 1, Total: 1, From a68194b3241a49c819fbb92f8a3590477d4cbdff Mon Sep 17 00:00:00 2001 From: Gabriela Gutierrez Date: Wed, 6 Sep 2023 19:35:38 +0000 Subject: [PATCH 33/46] refactor: Var declarations to create proportional score Signed-off-by: Gabriela Gutierrez --- checker/check_result.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/checker/check_result.go b/checker/check_result.go index 293c8ddaaf0..c76e38dcf82 100644 --- a/checker/check_result.go +++ b/checker/check_result.go @@ -109,8 +109,7 @@ func CreateProportionalScore(success, total int) int { // between multiple successes over the total, but some proportions // are worth more. func CreateProportionalScoreWeighted(scores ...*ProportionalScoreWeighted) int { - ws := 0 - wt := 0 + var ws, wt int for _, score := range scores { ws += score.Success * score.Weight wt += score.Total * score.Weight From aa23b1579d8840fe49a73149e6149ddaa6410438 Mon Sep 17 00:00:00 2001 From: Gabriela Gutierrez Date: Wed, 6 Sep 2023 19:47:37 +0000 Subject: [PATCH 34/46] fix: Remove unnecessary pointer Signed-off-by: Gabriela Gutierrez --- checker/check_result.go | 2 +- checks/evaluation/pinned_dependencies.go | 14 +++++++------- checks/evaluation/pinned_dependencies_test.go | 18 +++++++++--------- 3 files changed, 17 insertions(+), 17 deletions(-) diff --git a/checker/check_result.go b/checker/check_result.go index c76e38dcf82..c6f8de0bad2 100644 --- a/checker/check_result.go +++ b/checker/check_result.go @@ -108,7 +108,7 @@ func CreateProportionalScore(success, total int) int { // CreateProportionalScoreWeighted creates the proportional score // between multiple successes over the total, but some proportions // are worth more. -func CreateProportionalScoreWeighted(scores ...*ProportionalScoreWeighted) int { +func CreateProportionalScoreWeighted(scores ...ProportionalScoreWeighted) int { var ws, wt int for _, score := range scores { ws += score.Success * score.Weight diff --git a/checks/evaluation/pinned_dependencies.go b/checks/evaluation/pinned_dependencies.go index a303b2587c2..84d4dff0663 100644 --- a/checks/evaluation/pinned_dependencies.go +++ b/checks/evaluation/pinned_dependencies.go @@ -101,7 +101,7 @@ func PinningDependencies(name string, c *checker.CheckRequest, } // Generate scores and Info results. - var scores []*checker.ProportionalScoreWeighted + 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)...) @@ -109,7 +109,7 @@ func PinningDependencies(name string, c *checker.CheckRequest, // We will only score the ecossystem if there are dependencies // This results in only existing ecossystems being included in the final score for t := range pr { - scores = append(scores, &checker.ProportionalScoreWeighted{ + scores = append(scores, checker.ProportionalScoreWeighted{ Success: pr[t].pinned, Total: pr[t].total, Weight: 10, @@ -191,12 +191,12 @@ func addWorkflowPinnedResult(rr *checker.Dependency, w *worklowPinningResult, is } } -func createScoreForGitHubActionsWorkflow(wp *worklowPinningResult) []*checker.ProportionalScoreWeighted { +func createScoreForGitHubActionsWorkflow(wp *worklowPinningResult) []checker.ProportionalScoreWeighted { if wp.gitHubOwned.total == 0 && wp.thirdParties.total == 0 { - return []*checker.ProportionalScoreWeighted{} + return []checker.ProportionalScoreWeighted{} } if wp.gitHubOwned.total != 0 && wp.thirdParties.total != 0 { - return []*checker.ProportionalScoreWeighted{ + return []checker.ProportionalScoreWeighted{ { Success: wp.gitHubOwned.pinned, Total: wp.gitHubOwned.total, @@ -210,7 +210,7 @@ func createScoreForGitHubActionsWorkflow(wp *worklowPinningResult) []*checker.Pr } } if wp.gitHubOwned.total != 0 { - return []*checker.ProportionalScoreWeighted{ + return []checker.ProportionalScoreWeighted{ { Success: wp.gitHubOwned.pinned, Total: wp.gitHubOwned.total, @@ -218,7 +218,7 @@ func createScoreForGitHubActionsWorkflow(wp *worklowPinningResult) []*checker.Pr }, } } - return []*checker.ProportionalScoreWeighted{ + return []checker.ProportionalScoreWeighted{ { Success: wp.thirdParties.pinned, Total: wp.thirdParties.total, diff --git a/checks/evaluation/pinned_dependencies_test.go b/checks/evaluation/pinned_dependencies_test.go index 0dd4bdf307f..cc4f6fce015 100644 --- a/checks/evaluation/pinned_dependencies_test.go +++ b/checks/evaluation/pinned_dependencies_test.go @@ -30,7 +30,7 @@ func Test_createScoreForGitHubActionsWorkflow(t *testing.T) { tests := []struct { name string r worklowPinningResult - scores []*checker.ProportionalScoreWeighted + scores []checker.ProportionalScoreWeighted }{ { name: "GitHub-owned and Third-Party actions pinned", @@ -44,7 +44,7 @@ func Test_createScoreForGitHubActionsWorkflow(t *testing.T) { total: 1, }, }, - scores: []*checker.ProportionalScoreWeighted{ + scores: []checker.ProportionalScoreWeighted{ { Success: 1, Total: 1, @@ -69,7 +69,7 @@ func Test_createScoreForGitHubActionsWorkflow(t *testing.T) { total: 1, }, }, - scores: []*checker.ProportionalScoreWeighted{ + scores: []checker.ProportionalScoreWeighted{ { Success: 1, Total: 1, @@ -94,7 +94,7 @@ func Test_createScoreForGitHubActionsWorkflow(t *testing.T) { total: 1, }, }, - scores: []*checker.ProportionalScoreWeighted{ + scores: []checker.ProportionalScoreWeighted{ { Success: 0, Total: 1, @@ -119,7 +119,7 @@ func Test_createScoreForGitHubActionsWorkflow(t *testing.T) { total: 1, }, }, - scores: []*checker.ProportionalScoreWeighted{ + scores: []checker.ProportionalScoreWeighted{ { Success: 0, Total: 1, @@ -144,7 +144,7 @@ func Test_createScoreForGitHubActionsWorkflow(t *testing.T) { total: 1, }, }, - scores: []*checker.ProportionalScoreWeighted{ + scores: []checker.ProportionalScoreWeighted{ { Success: 0, Total: 1, @@ -164,7 +164,7 @@ func Test_createScoreForGitHubActionsWorkflow(t *testing.T) { total: 0, }, }, - scores: []*checker.ProportionalScoreWeighted{ + scores: []checker.ProportionalScoreWeighted{ { Success: 0, Total: 1, @@ -184,7 +184,7 @@ func Test_createScoreForGitHubActionsWorkflow(t *testing.T) { total: 1, }, }, - scores: []*checker.ProportionalScoreWeighted{ + scores: []checker.ProportionalScoreWeighted{ { Success: 1, Total: 1, @@ -204,7 +204,7 @@ func Test_createScoreForGitHubActionsWorkflow(t *testing.T) { total: 0, }, }, - scores: []*checker.ProportionalScoreWeighted{ + scores: []checker.ProportionalScoreWeighted{ { Success: 1, Total: 1, From e47830748be3d9cb59b8190527e1c0aebc1fd2ea Mon Sep 17 00:00:00 2001 From: Gabriela Gutierrez Date: Wed, 6 Sep 2023 20:49:34 +0000 Subject: [PATCH 35/46] fix: Dependencies priority declaration Signed-off-by: Gabriela Gutierrez --- checks/evaluation/pinned_dependencies.go | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/checks/evaluation/pinned_dependencies.go b/checks/evaluation/pinned_dependencies.go index 84d4dff0663..b97903c5b5e 100644 --- a/checks/evaluation/pinned_dependencies.go +++ b/checks/evaluation/pinned_dependencies.go @@ -36,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 ecossystems are equally prioritized except +// for GitHub Actions. GitHub Actions can be GitHub-owned or from third-party +// development. The GitHub Actions ecossystem has equal priority compared to other +// ecossystems, 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, @@ -112,7 +126,7 @@ func PinningDependencies(name string, c *checker.CheckRequest, scores = append(scores, checker.ProportionalScoreWeighted{ Success: pr[t].pinned, Total: pr[t].total, - Weight: 10, + Weight: normalWeight, }) } @@ -200,12 +214,12 @@ func createScoreForGitHubActionsWorkflow(wp *worklowPinningResult) []checker.Pro { Success: wp.gitHubOwned.pinned, Total: wp.gitHubOwned.total, - Weight: 2, + Weight: gitHubOwnedActionWeight, }, { Success: wp.thirdParties.pinned, Total: wp.thirdParties.total, - Weight: 8, + Weight: thirdPartyActionWeight, }, } } @@ -214,7 +228,7 @@ func createScoreForGitHubActionsWorkflow(wp *worklowPinningResult) []checker.Pro { Success: wp.gitHubOwned.pinned, Total: wp.gitHubOwned.total, - Weight: 10, + Weight: normalWeight, }, } } @@ -222,7 +236,7 @@ func createScoreForGitHubActionsWorkflow(wp *worklowPinningResult) []checker.Pro { Success: wp.thirdParties.pinned, Total: wp.thirdParties.total, - Weight: 10, + Weight: normalWeight, }, } } From 99f4dc60e41385d4105ce5cfa33d1f1375c83f08 Mon Sep 17 00:00:00 2001 From: Gabriela Gutierrez Date: Wed, 13 Sep 2023 18:10:23 +0000 Subject: [PATCH 36/46] fix: Ecosystem spelling Signed-off-by: Gabriela Gutierrez --- checks/evaluation/pinned_dependencies.go | 10 +++++----- checks/evaluation/pinned_dependencies_test.go | 20 +++++++++---------- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/checks/evaluation/pinned_dependencies.go b/checks/evaluation/pinned_dependencies.go index b97903c5b5e..7453f740b2f 100644 --- a/checks/evaluation/pinned_dependencies.go +++ b/checks/evaluation/pinned_dependencies.go @@ -38,10 +38,10 @@ type worklowPinningResult struct { // Weights used for proportional score. // This defines the priority of pinning a dependency over other dependencies. -// The dependencies from all ecossystems are equally prioritized except +// 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 ecossystem has equal priority compared to other -// ecossystems, but, within GitHub Actions, pinning third-party actions has more +// 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 ( @@ -120,8 +120,8 @@ func PinningDependencies(name string, c *checker.CheckRequest, // GitHub Actions need to be handled separately since they are not in pr scores = append(scores, createScoreForGitHubActionsWorkflow(&wp)...) // Only exisiting dependencies will be found in pr - // We will only score the ecossystem if there are dependencies - // This results in only existing ecossystems being included in the final score + // 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 { scores = append(scores, checker.ProportionalScoreWeighted{ Success: pr[t].pinned, diff --git a/checks/evaluation/pinned_dependencies_test.go b/checks/evaluation/pinned_dependencies_test.go index cc4f6fce015..bcd6cb791be 100644 --- a/checks/evaluation/pinned_dependencies_test.go +++ b/checks/evaluation/pinned_dependencies_test.go @@ -345,7 +345,7 @@ func Test_PinningDependencies(t *testing.T) { }, }, { - name: "1 ecossystem pinned and 1 ecossystem unpinned", + name: "1 ecosystem pinned and 1 ecosystem unpinned", dependencies: []checker.Dependency{ { Location: &checker.File{}, @@ -367,7 +367,7 @@ func Test_PinningDependencies(t *testing.T) { }, }, { - name: "1 ecossystem partially pinned", + name: "1 ecosystem partially pinned", dependencies: []checker.Dependency{ { Location: &checker.File{}, @@ -610,7 +610,7 @@ func Test_PinningDependencies(t *testing.T) { }, }, { - name: "2 unpinned dependencies for 1 ecossystem shows 2 warn messages", + name: "2 unpinned dependencies for 1 ecosystem shows 2 warn messages", dependencies: []checker.Dependency{ { Location: &checker.File{}, @@ -632,7 +632,7 @@ func Test_PinningDependencies(t *testing.T) { }, }, { - name: "2 unpinned dependencies for 2 ecossystems shows 2 warn messages", + name: "2 unpinned dependencies for 2 ecosystems shows 2 warn messages", dependencies: []checker.Dependency{ { Location: &checker.File{}, @@ -654,7 +654,7 @@ func Test_PinningDependencies(t *testing.T) { }, }, { - name: "GitHub Actions ecossystem with GitHub-owned pinned", + name: "GitHub Actions ecosystem with GitHub-owned pinned", dependencies: []checker.Dependency{ { Location: &checker.File{ @@ -673,7 +673,7 @@ func Test_PinningDependencies(t *testing.T) { }, }, { - name: "GitHub Actions ecossystem with third-party pinned", + name: "GitHub Actions ecosystem with third-party pinned", dependencies: []checker.Dependency{ { Location: &checker.File{ @@ -692,7 +692,7 @@ func Test_PinningDependencies(t *testing.T) { }, }, { - name: "GitHub Actions ecossystem with GitHub-owned and third-party pinned", + name: "GitHub Actions ecosystem with GitHub-owned and third-party pinned", dependencies: []checker.Dependency{ { Location: &checker.File{ @@ -718,7 +718,7 @@ func Test_PinningDependencies(t *testing.T) { }, }, { - name: "GitHub Actions ecossystem with GitHub-owned and third-party unpinned", + name: "GitHub Actions ecosystem with GitHub-owned and third-party unpinned", dependencies: []checker.Dependency{ { Location: &checker.File{ @@ -744,7 +744,7 @@ func Test_PinningDependencies(t *testing.T) { }, }, { - name: "GitHub Actions ecossystem with GitHub-owned pinned and third-party unpinned", + name: "GitHub Actions ecosystem with GitHub-owned pinned and third-party unpinned", dependencies: []checker.Dependency{ { Location: &checker.File{ @@ -770,7 +770,7 @@ func Test_PinningDependencies(t *testing.T) { }, }, { - name: "GitHub Actions ecossystem with GitHub-owned unpinned and third-party pinned", + name: "GitHub Actions ecosystem with GitHub-owned unpinned and third-party pinned", dependencies: []checker.Dependency{ { Location: &checker.File{ From 7000eb57ec89f9dc24b02812b92572c5411d240f Mon Sep 17 00:00:00 2001 From: Gabriela Gutierrez Date: Wed, 13 Sep 2023 20:19:22 +0000 Subject: [PATCH 37/46] fix: Handle 0 weight and 0 total when creating proportional weighted score Signed-off-by: Gabriela Gutierrez --- checker/check_result.go | 27 ++- checker/check_result_test.go | 267 +++++++++++++++++++++++ checks/evaluation/pinned_dependencies.go | 5 +- 3 files changed, 296 insertions(+), 3 deletions(-) diff --git a/checker/check_result.go b/checker/check_result.go index c6f8de0bad2..5ff0438f591 100644 --- a/checker/check_result.go +++ b/checker/check_result.go @@ -108,14 +108,37 @@ func CreateProportionalScore(success, total int) int { // CreateProportionalScoreWeighted creates the proportional score // between multiple successes over the total, but some proportions // are worth more. -func CreateProportionalScoreWeighted(scores ...ProportionalScoreWeighted) int { +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("%d successes is higher than %d total", score.Success, score.Total) + } + if score.Total != 0 { + noScoreGroups = false + } else { + // Group with 0 total, does not count for score + continue + } + 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))) + return int(math.Min(float64(MaxResultScore*ws/wt), float64(MaxResultScore))), nil } // AggregateScores adds up all scores diff --git a/checker/check_result_test.go b/checker/check_result_test.go index 291f0696cde..7c847f6dad4 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 no weight does not matter to the score and group with 0 total does not count to the score, results in max score", + 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/checks/evaluation/pinned_dependencies.go b/checks/evaluation/pinned_dependencies.go index 7453f740b2f..12405bb92dc 100644 --- a/checks/evaluation/pinned_dependencies.go +++ b/checks/evaluation/pinned_dependencies.go @@ -134,7 +134,10 @@ func PinningDependencies(name string, c *checker.CheckRequest, return checker.CreateInconclusiveResult(name, "no dependencies found") } - score := checker.CreateProportionalScoreWeighted(scores...) + score, err := checker.CreateProportionalScoreWeighted(scores...) + if err != nil { + return checker.CreateRuntimeErrorResult(name, err) + } if score == checker.MaxResultScore { return checker.CreateMaxScoreResult(name, "all dependencies are pinned") From 63904936f0de5bd25fe1dffa5507b65c51a70704 Mon Sep 17 00:00:00 2001 From: Gabriela Gutierrez Date: Wed, 13 Sep 2023 21:03:56 +0000 Subject: [PATCH 38/46] fix: Revert -d flag identification change Signed-off-by: Gabriela Gutierrez --- checks/raw/shell_download_validate.go | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/checks/raw/shell_download_validate.go b/checks/raw/shell_download_validate.go index b809b403e7b..6cbbc47dd74 100644 --- a/checks/raw/shell_download_validate.go +++ b/checks/raw/shell_download_validate.go @@ -432,24 +432,23 @@ func isGoUnpinnedDownload(cmd []string) bool { 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 := 2; i < len(cmd); i++ { - // The -d flag instructs get to download but not install the packages - if strings.EqualFold(cmd[i], "-d") { - return false - } - + for i := 1; i < len(cmd)-1; i++ { // Skip all flags // TODO skip other build flags which might take arguments - if slices.Contains([]string{"-f", "-t", "-u", "-v", "-fix", "-insecure"}, cmd[i]) { + for i < len(cmd)-1 && slices.Contains([]string{"-d", "-f", "-t", "-u", "-v", "-fix", "-insecure"}, cmd[i+1]) { // Record the flag -insecure - if cmd[i] == "-insecure" { + if cmd[i+1] == "-insecure" { insecure = true } - continue + i++ } + if i+1 >= len(cmd) { + // this is case go get -d -v + return false + } // TODO check more than one package - pkg := cmd[i] + pkg := cmd[i+1] // Consider strings that are not URLs as local folders // which are pinned. regex := regexp.MustCompile(`\w+\.\w+/\w+`) From 318736245e1cfe70c3e0cf98275c9db1e557ffde Mon Sep 17 00:00:00 2001 From: Gabriela Gutierrez Date: Wed, 13 Sep 2023 21:22:09 +0000 Subject: [PATCH 39/46] fix: npm ci command is npm download and is pinned Signed-off-by: Gabriela Gutierrez --- checks/raw/shell_download_validate.go | 16 +++++- checks/raw/shell_download_validate_test.go | 66 ++++++++++++++++++++++ 2 files changed, 79 insertions(+), 3 deletions(-) diff --git a/checks/raw/shell_download_validate.go b/checks/raw/shell_download_validate.go index 6cbbc47dd74..fe7c258c2d5 100644 --- a/checks/raw/shell_download_validate.go +++ b/checks/raw/shell_download_validate.go @@ -406,17 +406,27 @@ func isNpmDownload(cmd []string) bool { 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 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 +} + func isGoDownload(cmd []string) bool { // `Go install` will automatically look up the // go.mod and go.sum, so we don't flag it. @@ -826,7 +836,7 @@ func collectUnpinnedPakageManagerDownload(startLine, endLine uint, node syntax.N EndOffset: endLine, Snippet: cmd, }, - Pinned: asBoolPointer(false), + Pinned: asBoolPointer(!isNpmUnpinnedDownload(c)), Type: checker.DependencyUseTypeNpmCommand, }, ) 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) + } + }) + } +} From 2484ddc2ffbdf72d74995a7bd7e7620c03cae19b Mon Sep 17 00:00:00 2001 From: Gabriela Gutierrez Date: Wed, 13 Sep 2023 22:21:00 +0000 Subject: [PATCH 40/46] fix: Linter errors Signed-off-by: Gabriela Gutierrez --- checker/check_result.go | 7 ++++++- checker/check_result_test.go | 2 +- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/checker/check_result.go b/checker/check_result.go index 5ff0438f591..30b1a7b9c1a 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 @@ -114,7 +119,7 @@ func CreateProportionalScoreWeighted(scores ...ProportionalScoreWeighted) (int, noScoreGroups := true for _, score := range scores { if score.Success > score.Total { - return InconclusiveResultScore, fmt.Errorf("%d successes is higher than %d total", score.Success, score.Total) + return InconclusiveResultScore, fmt.Errorf("%w: %d, %d", ErrSuccessTotal, score.Success, score.Total) } if score.Total != 0 { noScoreGroups = false diff --git a/checker/check_result_test.go b/checker/check_result_test.go index 7c847f6dad4..1ec48850340 100644 --- a/checker/check_result_test.go +++ b/checker/check_result_test.go @@ -368,7 +368,7 @@ func TestCreateProportionalScoreWeighted(t *testing.T) { }, }, { - name: "group with no weight does not matter to the score and group with 0 total does not count to the score, results in max score", + name: "group with 0 weight counts as max score and group with 0 total does not count", scores: []ProportionalScoreWeighted{ { Success: 2, From 24f3a8fb44baf176d279d05af2e3a353661ad1d4 Mon Sep 17 00:00:00 2001 From: Gabriela Gutierrez Date: Fri, 15 Sep 2023 18:54:01 +0000 Subject: [PATCH 41/46] fix: Unexport error variable to other packages Signed-off-by: Gabriela Gutierrez --- checker/check_result.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/checker/check_result.go b/checker/check_result.go index 30b1a7b9c1a..3b1cfceb592 100644 --- a/checker/check_result.go +++ b/checker/check_result.go @@ -51,9 +51,9 @@ const ( DetailDebug ) -// ErrSuccessTotal indicates a runtime error because number of success cases should +// 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") +var errSuccessTotal = errors.New("unexpected number of success is higher than total") // CheckResult captures result from a check run. // @@ -119,7 +119,7 @@ func CreateProportionalScoreWeighted(scores ...ProportionalScoreWeighted) (int, noScoreGroups := true for _, score := range scores { if score.Success > score.Total { - return InconclusiveResultScore, fmt.Errorf("%w: %d, %d", ErrSuccessTotal, score.Success, score.Total) + return InconclusiveResultScore, fmt.Errorf("%w: %d, %d", errSuccessTotal, score.Success, score.Total) } if score.Total != 0 { noScoreGroups = false From f83946248c61af84788403bb7a8f48d6be5e6753 Mon Sep 17 00:00:00 2001 From: Gabriela Gutierrez Date: Fri, 15 Sep 2023 19:15:01 +0000 Subject: [PATCH 42/46] refactor: Simplify no score groups condition Signed-off-by: Gabriela Gutierrez --- checker/check_result.go | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/checker/check_result.go b/checker/check_result.go index 3b1cfceb592..ceee77cd1bf 100644 --- a/checker/check_result.go +++ b/checker/check_result.go @@ -121,12 +121,10 @@ func CreateProportionalScoreWeighted(scores ...ProportionalScoreWeighted) (int, if score.Success > score.Total { return InconclusiveResultScore, fmt.Errorf("%w: %d, %d", errSuccessTotal, score.Success, score.Total) } - if score.Total != 0 { - noScoreGroups = false - } else { - // Group with 0 total, does not count for score - continue + if score.Total == 0 { + continue // Group with 0 total, does not count for score } + noScoreGroups = false if score.Weight != 0 { allWeightsZero = false } From ca725e05a490a394fd51717271af2214fa5dbd5a Mon Sep 17 00:00:00 2001 From: Gabriela Gutierrez Date: Tue, 19 Sep 2023 14:17:05 +0000 Subject: [PATCH 43/46] feat: Log proportion of dependencies pinned Signed-off-by: Gabriela Gutierrez --- checks/evaluation/pinned_dependencies.go | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/checks/evaluation/pinned_dependencies.go b/checks/evaluation/pinned_dependencies.go index 12405bb92dc..bd042578184 100644 --- a/checks/evaluation/pinned_dependencies.go +++ b/checks/evaluation/pinned_dependencies.go @@ -118,11 +118,12 @@ func PinningDependencies(name string, c *checker.CheckRequest, 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)...) + 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, @@ -180,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) @@ -188,9 +189,9 @@ 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) } func addPinnedResult(rr *checker.Dependency, r *pinnedResult) { @@ -208,11 +209,19 @@ func addWorkflowPinnedResult(rr *checker.Dependency, w *worklowPinningResult, is } } -func createScoreForGitHubActionsWorkflow(wp *worklowPinningResult) []checker.ProportionalScoreWeighted { +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), + }) +} + +func createScoreForGitHubActionsWorkflow(wp *worklowPinningResult, dl checker.DetailLogger) []checker.ProportionalScoreWeighted { if wp.gitHubOwned.total == 0 && wp.thirdParties.total == 0 { return []checker.ProportionalScoreWeighted{} } 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, @@ -227,6 +236,7 @@ func createScoreForGitHubActionsWorkflow(wp *worklowPinningResult) []checker.Pro } } if wp.gitHubOwned.total != 0 { + logPinnedResult(dl, wp.gitHubOwned, generateOwnerToDisplay(true)) return []checker.ProportionalScoreWeighted{ { Success: wp.gitHubOwned.pinned, @@ -235,6 +245,7 @@ func createScoreForGitHubActionsWorkflow(wp *worklowPinningResult) []checker.Pro }, } } + logPinnedResult(dl, wp.thirdParties, generateOwnerToDisplay(false)) return []checker.ProportionalScoreWeighted{ { Success: wp.thirdParties.pinned, From f6ccf914a8be62003a10cf2b435a654655de77ac Mon Sep 17 00:00:00 2001 From: Gabriela Gutierrez Date: Tue, 19 Sep 2023 14:48:31 +0000 Subject: [PATCH 44/46] 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 --- checks/evaluation/pinned_dependencies_test.go | 49 ++++++++++--------- 1 file changed, 25 insertions(+), 24 deletions(-) diff --git a/checks/evaluation/pinned_dependencies_test.go b/checks/evaluation/pinned_dependencies_test.go index bcd6cb791be..3efb32c3901 100644 --- a/checks/evaluation/pinned_dependencies_test.go +++ b/checks/evaluation/pinned_dependencies_test.go @@ -217,7 +217,8 @@ func Test_createScoreForGitHubActionsWorkflow(t *testing.T) { 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() - actual := createScoreForGitHubActionsWorkflow(&tt.r) + dl := scut.TestDetailLogger{} + actual := createScoreForGitHubActionsWorkflow(&tt.r, &dl) diff := cmp.Diff(tt.scores, actual) if diff != "" { t.Errorf("createScoreForGitHubActionsWorkflow (-want,+got) %+v", diff) @@ -289,7 +290,7 @@ func Test_PinningDependencies(t *testing.T) { Error: nil, Score: 10, NumberOfWarn: 0, - NumberOfInfo: 0, + NumberOfInfo: 7, NumberOfDebug: 0, }, }, @@ -340,7 +341,7 @@ func Test_PinningDependencies(t *testing.T) { Error: nil, Score: 0, NumberOfWarn: 7, - NumberOfInfo: 0, + NumberOfInfo: 7, NumberOfDebug: 0, }, }, @@ -362,7 +363,7 @@ func Test_PinningDependencies(t *testing.T) { Error: nil, Score: 5, NumberOfWarn: 1, - NumberOfInfo: 0, + NumberOfInfo: 2, NumberOfDebug: 0, }, }, @@ -384,7 +385,7 @@ func Test_PinningDependencies(t *testing.T) { Error: nil, Score: 5, NumberOfWarn: 1, - NumberOfInfo: 0, + NumberOfInfo: 1, NumberOfDebug: 0, }, }, @@ -412,7 +413,7 @@ func Test_PinningDependencies(t *testing.T) { Error: nil, Score: 10, NumberOfWarn: 0, - NumberOfInfo: 0, + NumberOfInfo: 1, NumberOfDebug: 0, }, }, @@ -429,7 +430,7 @@ func Test_PinningDependencies(t *testing.T) { Error: nil, Score: 0, NumberOfWarn: 1, - NumberOfInfo: 0, + NumberOfInfo: 1, NumberOfDebug: 0, }, }, @@ -503,7 +504,7 @@ func Test_PinningDependencies(t *testing.T) { Error: nil, Score: 0, NumberOfWarn: 1, - NumberOfInfo: 0, + NumberOfInfo: 1, NumberOfDebug: 0, }, }, @@ -520,7 +521,7 @@ func Test_PinningDependencies(t *testing.T) { Error: nil, Score: 0, NumberOfWarn: 1, - NumberOfInfo: 0, + NumberOfInfo: 1, NumberOfDebug: 0, }, }, @@ -537,7 +538,7 @@ func Test_PinningDependencies(t *testing.T) { Error: nil, Score: 0, NumberOfWarn: 1, - NumberOfInfo: 0, + NumberOfInfo: 1, NumberOfDebug: 0, }, }, @@ -554,7 +555,7 @@ func Test_PinningDependencies(t *testing.T) { Error: nil, Score: 0, NumberOfWarn: 1, - NumberOfInfo: 0, + NumberOfInfo: 1, NumberOfDebug: 0, }, }, @@ -571,7 +572,7 @@ func Test_PinningDependencies(t *testing.T) { Error: nil, Score: 0, NumberOfWarn: 1, - NumberOfInfo: 0, + NumberOfInfo: 1, NumberOfDebug: 0, }, }, @@ -588,7 +589,7 @@ func Test_PinningDependencies(t *testing.T) { Error: nil, Score: 0, NumberOfWarn: 1, - NumberOfInfo: 0, + NumberOfInfo: 1, NumberOfDebug: 0, }, }, @@ -605,7 +606,7 @@ func Test_PinningDependencies(t *testing.T) { Error: nil, Score: 0, NumberOfWarn: 1, - NumberOfInfo: 0, + NumberOfInfo: 1, NumberOfDebug: 0, }, }, @@ -627,7 +628,7 @@ func Test_PinningDependencies(t *testing.T) { Error: nil, Score: 0, NumberOfWarn: 2, - NumberOfInfo: 0, + NumberOfInfo: 1, NumberOfDebug: 0, }, }, @@ -649,7 +650,7 @@ func Test_PinningDependencies(t *testing.T) { Error: nil, Score: 0, NumberOfWarn: 2, - NumberOfInfo: 0, + NumberOfInfo: 2, NumberOfDebug: 0, }, }, @@ -668,7 +669,7 @@ func Test_PinningDependencies(t *testing.T) { Error: nil, Score: 10, NumberOfWarn: 0, - NumberOfInfo: 0, + NumberOfInfo: 1, NumberOfDebug: 0, }, }, @@ -687,7 +688,7 @@ func Test_PinningDependencies(t *testing.T) { Error: nil, Score: 10, NumberOfWarn: 0, - NumberOfInfo: 0, + NumberOfInfo: 1, NumberOfDebug: 0, }, }, @@ -713,7 +714,7 @@ func Test_PinningDependencies(t *testing.T) { Error: nil, Score: 10, NumberOfWarn: 0, - NumberOfInfo: 0, + NumberOfInfo: 2, NumberOfDebug: 0, }, }, @@ -739,7 +740,7 @@ func Test_PinningDependencies(t *testing.T) { Error: nil, Score: 0, NumberOfWarn: 2, - NumberOfInfo: 0, + NumberOfInfo: 2, NumberOfDebug: 0, }, }, @@ -765,7 +766,7 @@ func Test_PinningDependencies(t *testing.T) { Error: nil, Score: 2, NumberOfWarn: 1, - NumberOfInfo: 0, + NumberOfInfo: 2, NumberOfDebug: 0, }, }, @@ -791,7 +792,7 @@ func Test_PinningDependencies(t *testing.T) { Error: nil, Score: 8, NumberOfWarn: 1, - NumberOfInfo: 0, + NumberOfInfo: 2, NumberOfDebug: 0, }, }, @@ -826,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 { From 9baf5471a6252e476645c783b151321db7f51752 Mon Sep 17 00:00:00 2001 From: Gabriela Gutierrez Date: Tue, 19 Sep 2023 14:52:26 +0000 Subject: [PATCH 45/46] 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 --- e2e/pinned_dependencies_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/e2e/pinned_dependencies_test.go b/e2e/pinned_dependencies_test.go index 8ba87a7a67f..924a43a1ce1 100644 --- a/e2e/pinned_dependencies_test.go +++ b/e2e/pinned_dependencies_test.go @@ -51,7 +51,7 @@ var _ = Describe("E2E TEST:"+checks.CheckPinnedDependencies, func() { Error: nil, Score: 2, NumberOfWarn: 139, - NumberOfInfo: 0, + NumberOfInfo: 5, NumberOfDebug: 0, } result := checks.PinningDependencies(&req) @@ -76,7 +76,7 @@ var _ = Describe("E2E TEST:"+checks.CheckPinnedDependencies, func() { Error: nil, Score: 2, NumberOfWarn: 139, - NumberOfInfo: 0, + NumberOfInfo: 5, NumberOfDebug: 0, } result := checks.PinningDependencies(&req) @@ -112,7 +112,7 @@ var _ = Describe("E2E TEST:"+checks.CheckPinnedDependencies, func() { Error: nil, Score: 2, NumberOfWarn: 139, - NumberOfInfo: 0, + NumberOfInfo: 5, NumberOfDebug: 0, } result := checks.PinningDependencies(&req) From b986c65fd7355de8432d47384d66539d5d6b901c Mon Sep 17 00:00:00 2001 From: Gabriela Gutierrez Date: Tue, 19 Sep 2023 14:58:15 +0000 Subject: [PATCH 46/46] fix: Linter error Signed-off-by: Gabriela Gutierrez --- checks/evaluation/pinned_dependencies.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/checks/evaluation/pinned_dependencies.go b/checks/evaluation/pinned_dependencies.go index bd042578184..c0af9515214 100644 --- a/checks/evaluation/pinned_dependencies.go +++ b/checks/evaluation/pinned_dependencies.go @@ -215,7 +215,8 @@ func logPinnedResult(dl checker.DetailLogger, p pinnedResult, name string) { }) } -func createScoreForGitHubActionsWorkflow(wp *worklowPinningResult, dl checker.DetailLogger) []checker.ProportionalScoreWeighted { +func createScoreForGitHubActionsWorkflow(wp *worklowPinningResult, dl checker.DetailLogger, +) []checker.ProportionalScoreWeighted { if wp.gitHubOwned.total == 0 && wp.thirdParties.total == 0 { return []checker.ProportionalScoreWeighted{} }