🐛 Pinned-Dependencies continues on error (ossf#3515)

* Continue on error detecting OS

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

* Add tests for error detecting OS

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

* Add ElementError to identify elements that errored

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

* Add Incomplete field to PinningDependenciesData

Will store all errors handled during analysis, which may lead to incomplete results.

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

* Register job steps that errored out

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

* Add tests that incomplete steps are caught

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

* Add warnings to details about incomplete steps

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

* Add tests that incomplete steps generate warnings

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

* Register shell files skipped due to parser errors

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

* Add tests showing when parser errors affect analysis

Dockerfile pinning is not affected.
Everything in a 'broken' Dockerfile RUN block is ignored
Everything in a 'broken' shell script is ignored
testdata/script-invalid.sh modified to demonstrate the above

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

* Incomplete results logged as Info, not Warn

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

* Remove `Type` from logging of incomplete results

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

* Update tests after rebase

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

* Add Unwrap for ElementError, improve its docs

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

* Add ElementError case to evaluation unit test

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

* Move ElementError to checker/raw_result

checker/raw_result defines types used to describe analysis results.

ElementError is meant to describe potential flaws in the analysis
and is therefore a sort of analysis result itself.

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

* Use finding.Location for ElementError.Element

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

* Use an ElementError for script parser errors

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

* Replace .Incomplete []error with .ProcessingErrors []ElementError

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

* Adopt from reviewer comments

- Replace ElementError's `Element *finding.Location`
  with `Location finding.Location`
- Rename ErrorJobOSParsing to ErrJobOSParsing to satisfy linter
- Fix unit test

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

---------

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>
Signed-off-by: Allen Shearin <allen.p.shearin@gmail.com>

checker/raw_result.go

@@ -15,6 +15,7 @@
 package checker
 
 import (
+	"fmt"
 	"time"
 
 	"github.com/ossf/scorecard/v4/clients"
@@ -111,7 +112,8 @@ const (
 
 // PinningDependenciesData represents pinned dependency data.
 type PinningDependenciesData struct {
-	Dependencies []Dependency
+	Dependencies     []Dependency
+	ProcessingErrors []ElementError // jobs or files with errors may have incomplete results
 }
 
 // Dependency represents a dependency.
@@ -437,3 +439,23 @@ func (f *File) Location() *finding.Location {
 
 	return loc
 }
+
+// ElementError allows us to identify the "element" that led to the given error.
+// The "element" is the specific "code under analysis" that caused the error. It should
+// describe what caused the error as precisely as possible.
+//
+// For example, if a shell parsing error occurs while parsing a Dockerfile `RUN` block
+// or a GitHub workflow's `run:` step, the "element" should point to the Dockerfile
+// lines or workflow job step that caused the failure, not just the file path.
+type ElementError struct {
+	Err      error
+	Location finding.Location
+}
+
+func (e *ElementError) Error() string {
+	return fmt.Sprintf("%s: %v", e.Err, e.Location)
+}
+
+func (e *ElementError) Unwrap() error {
+	return e.Err
+}

checks/evaluation/pinned_dependencies.go

@@ -20,6 +20,7 @@ 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"
 )
@@ -65,6 +66,16 @@ func PinningDependencies(name string, c *checker.CheckRequest,
 	//nolint:errcheck
 	remediationMetadata, _ := remediation.New(c)
 
+	for _, e := range r.ProcessingErrors {
+		e := e
+		dl.Info(&checker.LogMessage{
+			Finding: &finding.Finding{
+				Message:  generateTextIncompleteResults(e),
+				Location: &e.Location,
+			},
+		})
+	}
+
 	for i := range r.Dependencies {
 		rr := r.Dependencies[i]
 		if rr.Location == nil {
@@ -105,7 +116,7 @@ func PinningDependencies(name string, c *checker.CheckRequest,
 				Type:        rr.Location.Type,
 				Offset:      rr.Location.Offset,
 				EndOffset:   rr.Location.EndOffset,
-				Text:        generateText(&rr),
+				Text:        generateTextUnpinned(&rr),
 				Snippet:     rr.Location.Snippet,
 				Remediation: generateRemediation(remediationMetadata, &rr),
 			})
@@ -176,7 +187,7 @@ func updatePinningResults(rr *checker.Dependency,
 	pr[rr.Type] = p
 }
 
-func generateText(rr *checker.Dependency) string {
+func generateTextUnpinned(rr *checker.Dependency) string {
 	if rr.Type == checker.DependencyUseTypeGHAction {
 		// Check if we are dealing with a GitHub action or a third-party one.
 		gitHubOwned := fileparser.IsGitHubOwnedAction(rr.Location.Snippet)
@@ -187,6 +198,10 @@ func generateText(rr *checker.Dependency) string {
 	return fmt.Sprintf("%s not pinned by hash", rr.Type)
 }
 
+func generateTextIncompleteResults(e checker.ElementError) string {
+	return fmt.Sprintf("Possibly incomplete results: %s", e.Err)
+}
+
 func generateOwnerToDisplay(gitHubOwned bool) string {
 	if gitHubOwned {
 		return fmt.Sprintf("GitHub-owned %s", checker.DependencyUseTypeGHAction)

checks/evaluation/pinned_dependencies_test.go

@@ -21,6 +21,7 @@ import (
 
 	"github.com/ossf/scorecard/v4/checker"
 	sce "github.com/ossf/scorecard/v4/errors"
+	"github.com/ossf/scorecard/v4/finding"
 	scut "github.com/ossf/scorecard/v4/utests"
 )
 
@@ -239,9 +240,10 @@ func Test_PinningDependencies(t *testing.T) {
 	t.Parallel()
 
 	tests := []struct {
-		name         string
-		dependencies []checker.Dependency
-		expected     scut.TestReturn
+		name             string
+		dependencies     []checker.Dependency
+		processingErrors []checker.ElementError
+		expected         scut.TestReturn
 	}{
 		{
 			name: "all dependencies pinned",
@@ -796,6 +798,34 @@ func Test_PinningDependencies(t *testing.T) {
 				NumberOfDebug: 0,
 			},
 		},
+		{
+			name: "Skipped objects and dependencies",
+			dependencies: []checker.Dependency{
+				{
+					Location: &checker.File{},
+					Type:     checker.DependencyUseTypeNpmCommand,
+					Pinned:   asBoolPointer(false),
+				},
+				{
+					Location: &checker.File{},
+					Type:     checker.DependencyUseTypeNpmCommand,
+					Pinned:   asBoolPointer(false),
+				},
+			},
+			processingErrors: []checker.ElementError{
+				{
+					Err:      sce.ErrJobOSParsing,
+					Location: finding.Location{},
+				},
+			},
+			expected: scut.TestReturn{
+				Error:         nil,
+				Score:         0,
+				NumberOfWarn:  2, // unpinned deps
+				NumberOfInfo:  2, // 1 for npm deps, 1 for processing error
+				NumberOfDebug: 0,
+			},
+		},
 	}
 
 	for _, tt := range tests {
@@ -807,7 +837,8 @@ func Test_PinningDependencies(t *testing.T) {
 			c := checker.CheckRequest{Dlogger: &dl}
 			actual := PinningDependencies("checkname", &c,
 				&checker.PinningDependenciesData{
-					Dependencies: tt.dependencies,
+					Dependencies:     tt.dependencies,
+					ProcessingErrors: tt.processingErrors,
 				})
 
 			if !scut.ValidateTestReturn(t, tt.name, &tt.expected, &actual, &dl) {
@@ -990,7 +1021,7 @@ func TestGenerateText(t *testing.T) {
 
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {
-			result := generateText(tc.dependency)
+			result := generateTextUnpinned(tc.dependency)
 			if !cmp.Equal(tc.expectedText, result) {
 				t.Errorf("generateText mismatch (-want +got):\n%s", cmp.Diff(tc.expectedText, result))
 			}

checks/fileparser/github_workflow.go

@@ -208,8 +208,15 @@ func GetOSesForJob(job *actionlint.Job) ([]string, error) {
 	}
 
 	if len(jobOSes) == 0 {
-		return jobOSes, sce.WithMessage(sce.ErrScorecardInternal,
-			fmt.Sprintf("unable to determine OS for job: %v", GetJobName(job)))
+		// This error is caught by the caller, which is responsible for adding more
+		// precise location information
+		jobName := GetJobName(job)
+		return jobOSes, &checker.ElementError{
+			Location: finding.Location{
+				Snippet: &jobName,
+			},
+			Err: sce.ErrJobOSParsing,
+		}
 	}
 	return jobOSes, nil
 }

checks/raw/pinned_dependencies.go

@@ -15,6 +15,7 @@
 package raw
 
 import (
+	"errors"
 	"fmt"
 	"reflect"
 	"regexp"
@@ -360,6 +361,7 @@ var validateGitHubWorkflowIsFreeOfInsecureDownloads fileparser.DoWhileTrueOnFile
 			jobName = fileparser.GetJobName(job)
 		}
 		taintedFiles := make(map[string]bool)
+
 		for _, step := range job.Steps {
 			step := step
 			if !fileparser.IsStepExecKind(step, actionlint.ExecKindRun) {
@@ -382,6 +384,23 @@ var validateGitHubWorkflowIsFreeOfInsecureDownloads fileparser.DoWhileTrueOnFile
 			// https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions#jobsjob_idstepsrun.
 			shell, err := fileparser.GetShellForStep(step, job)
 			if err != nil {
+				var elementError *checker.ElementError
+				if errors.As(err, &elementError) {
+					// Add the workflow name and step ID to the element
+					lineStart := uint(step.Pos.Line)
+					elementError.Location = finding.Location{
+						Path:      pathfn,
+						Snippet:   elementError.Location.Snippet,
+						LineStart: &lineStart,
+						Type:      finding.FileTypeSource,
+					}
+
+					pdata.ProcessingErrors = append(pdata.ProcessingErrors, *elementError)
+
+					// continue instead of break because other `run` steps may declare
+					// a valid shell we can scan
+					continue
+				}
 				return false, err
 			}
 			// Skip unsupported shells. We don't support Windows shells or some Unix shells.