From 875b6f694efec2267a5f9da54ffabc51cec59de7 Mon Sep 17 00:00:00 2001 From: laurentsimon <64505099+laurentsimon@users.noreply.github.com> Date: Mon, 2 May 2022 10:11:50 -0700 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20Ignore=20shell=20parsing=20error?= =?UTF-8?q?s=20when=20reporting=20results=20(#1878)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * ignore parsing errors * updates --- checks/cii_best_practices.go | 1 - checks/pinned_dependencies.go | 54 ++++++++------------------ checks/pinned_dependencies_test.go | 47 ++++++++++++++++++++++ checks/shell_download_validate.go | 10 ++--- checks/shell_download_validate_test.go | 4 +- 5 files changed, 69 insertions(+), 47 deletions(-) diff --git a/checks/cii_best_practices.go b/checks/cii_best_practices.go index b6c13ea1c25..f24ad36afe5 100644 --- a/checks/cii_best_practices.go +++ b/checks/cii_best_practices.go @@ -24,7 +24,6 @@ import ( // CheckCIIBestPractices is the registered name for CIIBestPractices. const CheckCIIBestPractices = "CII-Best-Practices" - //nolint:gochecknoinits func init() { if err := registerCheck(CheckCIIBestPractices, CIIBestPractices, nil); err != nil { diff --git a/checks/pinned_dependencies.go b/checks/pinned_dependencies.go index 950150727d9..7c4a66a4503 100644 --- a/checks/pinned_dependencies.go +++ b/checks/pinned_dependencies.go @@ -15,6 +15,7 @@ package checks import ( + "errors" "fmt" "regexp" "strings" @@ -235,14 +236,6 @@ func createReturnForIsShellScriptFreeOfInsecureDownloads(r pinnedResult, dl, err) } -func testValidateShellScriptIsFreeOfInsecureDownloads(pathfn string, - content []byte, dl checker.DetailLogger, -) (int, error) { - var r pinnedResult - _, err := validateShellScriptIsFreeOfInsecureDownloads(pathfn, content, dl, &r) - return createReturnForIsShellScriptFreeOfInsecureDownloads(r, dl, err) -} - var validateShellScriptIsFreeOfInsecureDownloads fileparser.DoWhileTrueOnFileContent = func( pathfn string, content []byte, @@ -252,6 +245,7 @@ var validateShellScriptIsFreeOfInsecureDownloads fileparser.DoWhileTrueOnFileCon return false, fmt.Errorf( "validateShellScriptIsFreeOfInsecureDownloads requires exactly 2 arguments: %w", errInvalidArgLength) } + pdata := dataAsResultPointer(args[1]) dl := dataAsDetailLogger(args[0]) @@ -260,10 +254,14 @@ var validateShellScriptIsFreeOfInsecureDownloads fileparser.DoWhileTrueOnFileCon addPinnedResult(pdata, true) return true, nil } - r, err := validateShellFile(pathfn, 0, 0 /*unknown*/, content, map[string]bool{}, dl) if err != nil { - return false, err + // Ignore parsing errors. + if errors.Is(err, sce.ErrorShellParsing) { + addPinnedResult(pdata, true) + } + + return false, nil } addPinnedResult(pdata, r) @@ -288,14 +286,6 @@ func createReturnForIsDockerfileFreeOfInsecureDownloads(r pinnedResult, dl, err) } -func testValidateDockerfileIsFreeOfInsecureDownloads(pathfn string, - content []byte, dl checker.DetailLogger, -) (int, error) { - var r pinnedResult - _, err := validateDockerfileIsFreeOfInsecureDownloads(pathfn, content, dl, &r) - return createReturnForIsDockerfileFreeOfInsecureDownloads(r, dl, err) -} - func isDockerfile(pathfn string, content []byte) bool { if strings.HasSuffix(pathfn, ".go") || strings.HasSuffix(pathfn, ".c") || @@ -368,6 +358,10 @@ var validateDockerfileIsFreeOfInsecureDownloads fileparser.DoWhileTrueOnFileCont r, err := validateShellFile(pathfn, uint(child.StartLine)-1, uint(child.EndLine)-1, bytes, taintedFiles, dl) if err != nil { + // Ignore parsing errors. + if errors.Is(err, sce.ErrorShellParsing) { + addPinnedResult(pdata, true) + } return false, err } addPinnedResult(pdata, r) @@ -392,12 +386,6 @@ func createReturnForIsDockerfilePinned(r pinnedResult, dl checker.DetailLogger, dl, err) } -func testValidateDockerfileIsPinned(pathfn string, content []byte, dl checker.DetailLogger) (int, error) { - var r pinnedResult - _, err := validateDockerfileIsPinned(pathfn, content, dl, &r) - return createReturnForIsDockerfilePinned(r, dl, err) -} - var validateDockerfileIsPinned fileparser.DoWhileTrueOnFileContent = func( pathfn string, content []byte, @@ -532,14 +520,6 @@ func createReturnForIsGitHubWorkflowScriptFreeOfInsecureDownloads(r pinnedResult dl, err) } -func testValidateGitHubWorkflowScriptFreeOfInsecureDownloads(pathfn string, - content []byte, dl checker.DetailLogger, -) (int, error) { - var r pinnedResult - _, err := validateGitHubWorkflowIsFreeOfInsecureDownloads(pathfn, content, dl, &r) - return createReturnForIsGitHubWorkflowScriptFreeOfInsecureDownloads(r, dl, err) -} - // validateGitHubWorkflowIsFreeOfInsecureDownloads checks if the workflow file downloads dependencies that are unpinned. // Returns true if the check should continue executing after this file. var validateGitHubWorkflowIsFreeOfInsecureDownloads fileparser.DoWhileTrueOnFileContent = func( @@ -612,6 +592,10 @@ var validateGitHubWorkflowIsFreeOfInsecureDownloads fileparser.DoWhileTrueOnFile validated, err := validateShellFile(pathfn, uint(execRun.Run.Pos.Line), uint(execRun.Run.Pos.Line), script, taintedFiles, dl) if err != nil { + // Ignore parsing errors. + if errors.Is(err, sce.ErrorShellParsing) { + addPinnedResult(pdata, true) + } return false, err } addPinnedResult(pdata, validated) @@ -640,12 +624,6 @@ func createReturnForIsGitHubActionsWorkflowPinned(r worklowPinningResult, dl che dl, err) } -func testIsGitHubActionsWorkflowPinned(pathfn string, content []byte, dl checker.DetailLogger) (int, error) { - var r worklowPinningResult - _, err := validateGitHubActionWorkflow(pathfn, content, dl, &r) - return createReturnForIsGitHubActionsWorkflowPinned(r, dl, err) -} - func generateOwnerToDisplay(gitHubOwned bool) string { if gitHubOwned { return "GitHub-owned" diff --git a/checks/pinned_dependencies_test.go b/checks/pinned_dependencies_test.go index 2d9f120d095..862b6fa9763 100644 --- a/checks/pinned_dependencies_test.go +++ b/checks/pinned_dependencies_test.go @@ -1115,6 +1115,17 @@ func TestShellScriptDownload(t *testing.T) { NumberOfDebug: 0, }, }, + { + name: "invalid shell script", + filename: "./testdata/script-invalid.sh", + expected: scut.TestReturn{ + Error: nil, + Score: checker.MaxResultScore, + NumberOfWarn: 0, + NumberOfInfo: 1, + NumberOfDebug: 1, + }, + }, } for _, tt := range tests { tt := tt // Re-initializing variable so it is not changed while executing the closure below @@ -1614,3 +1625,39 @@ func Test_maxScore(t *testing.T) { }) } } + +func testValidateShellScriptIsFreeOfInsecureDownloads(pathfn string, + content []byte, dl checker.DetailLogger, +) (int, error) { + var r pinnedResult + _, err := validateShellScriptIsFreeOfInsecureDownloads(pathfn, content, dl, &r) + return createReturnForIsShellScriptFreeOfInsecureDownloads(r, dl, err) +} + +func testValidateDockerfileIsFreeOfInsecureDownloads(pathfn string, + content []byte, dl checker.DetailLogger, +) (int, error) { + var r pinnedResult + _, err := validateDockerfileIsFreeOfInsecureDownloads(pathfn, content, dl, &r) + return createReturnForIsDockerfileFreeOfInsecureDownloads(r, dl, err) +} + +func testValidateDockerfileIsPinned(pathfn string, content []byte, dl checker.DetailLogger) (int, error) { + var r pinnedResult + _, err := validateDockerfileIsPinned(pathfn, content, dl, &r) + return createReturnForIsDockerfilePinned(r, dl, err) +} + +func testValidateGitHubWorkflowScriptFreeOfInsecureDownloads(pathfn string, + content []byte, dl checker.DetailLogger, +) (int, error) { + var r pinnedResult + _, err := validateGitHubWorkflowIsFreeOfInsecureDownloads(pathfn, content, dl, &r) + return createReturnForIsGitHubWorkflowScriptFreeOfInsecureDownloads(r, dl, err) +} + +func testIsGitHubActionsWorkflowPinned(pathfn string, content []byte, dl checker.DetailLogger) (int, error) { + var r worklowPinningResult + _, err := validateGitHubActionWorkflow(pathfn, content, dl, &r) + return createReturnForIsGitHubActionsWorkflowPinned(r, dl, err) +} diff --git a/checks/shell_download_validate.go b/checks/shell_download_validate.go index 3e9c28c5107..3a5d8ffccb2 100644 --- a/checks/shell_download_validate.go +++ b/checks/shell_download_validate.go @@ -17,7 +17,6 @@ package checks import ( "bufio" "bytes" - "errors" "fmt" "net/url" "path" @@ -599,8 +598,8 @@ func isChocoUnpinnedDownload(cmd []string) bool { str := parts[0] if strings.EqualFold(str, "--requirechecksum") || - strings.EqualFold(str, "--requirechecksums") || - strings.EqualFold(str, "--require-checksums") { + strings.EqualFold(str, "--requirechecksums") || + strings.EqualFold(str, "--require-checksums") { return false } } @@ -995,12 +994,11 @@ func validateShellFile(pathfn string, startLine, endLine uint, content []byte, taintedFiles map[string]bool, dl checker.DetailLogger, ) (bool, error) { r, err := validateShellFileAndRecord(pathfn, startLine, endLine, content, taintedFiles, dl) - if err != nil && errors.Is(err, sce.ErrorShellParsing) { - // Discard and print this particular error for now. + if err != nil { + // Print this particular error. dl.Debug(&checker.LogMessage{ Text: err.Error(), }) - err = nil } return r, err } diff --git a/checks/shell_download_validate_test.go b/checks/shell_download_validate_test.go index dfad86ca653..be8d4720eec 100644 --- a/checks/shell_download_validate_test.go +++ b/checks/shell_download_validate_test.go @@ -101,7 +101,7 @@ func TestValidateShellFile(t *testing.T) { } dl := scut.TestDetailLogger{} _, err = validateShellFile(filename, 0, 0, content, map[string]bool{}, &dl) - if err != nil { - t.Errorf("failed to discard shell parsing error: %v", err) + if err == nil { + t.Errorf("failed to detect shell parsing error: %v", err) } }