Skip to content

Commit

Permalink
🐛 Pinned-Dependencies continues on error (#3515)
Browse files Browse the repository at this point in the history
* Continue on error detecting OS

Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]>

* Add tests for error detecting OS

Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]>

* Add ElementError to identify elements that errored

Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]>

* 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 <[email protected]>

* Register job steps that errored out

Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]>

* Add tests that incomplete steps are caught

Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]>

* Add warnings to details about incomplete steps

Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]>

* Add tests that incomplete steps generate warnings

Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]>

* Register shell files skipped due to parser errors

Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]>

* 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 <[email protected]>

* Incomplete results logged as Info, not Warn

Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]>

* Remove `Type` from logging of incomplete results

Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]>

* Update tests after rebase

Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]>

* Add Unwrap for ElementError, improve its docs

Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]>

* Add ElementError case to evaluation unit test

Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]>

* 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 <[email protected]>

* Use finding.Location for ElementError.Element

Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]>

* Use an ElementError for script parser errors

Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]>

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

Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]>

* 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 <[email protected]>

---------

Signed-off-by: Pedro Kaj Kjellerup Nacht <[email protected]>
pnacht authored Nov 8, 2023
1 parent e16d3e3 commit 6d35c86
Showing 13 changed files with 370 additions and 72 deletions.
24 changes: 23 additions & 1 deletion checker/raw_result.go
Original file line number Diff line number Diff line change
@@ -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
}
19 changes: 17 additions & 2 deletions checks/evaluation/pinned_dependencies.go
Original file line number Diff line number Diff line change
@@ -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)
41 changes: 36 additions & 5 deletions checks/evaluation/pinned_dependencies_test.go
Original file line number Diff line number Diff line change
@@ -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))
}
11 changes: 9 additions & 2 deletions checks/fileparser/github_workflow.go
Original file line number Diff line number Diff line change
@@ -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
}
19 changes: 19 additions & 0 deletions checks/raw/pinned_dependencies.go
Original file line number Diff line number Diff line change
@@ -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.
218 changes: 161 additions & 57 deletions checks/raw/pinned_dependencies_test.go

Large diffs are not rendered by default.

20 changes: 17 additions & 3 deletions checks/raw/shell_download_validate.go
Original file line number Diff line number Diff line change
@@ -17,6 +17,7 @@ package raw
import (
"bufio"
"bytes"
"errors"
"fmt"
"net/url"
"path"
@@ -1055,9 +1056,22 @@ func validateShellFileAndRecord(pathfn string, startLine, endLine uint, content
in := strings.NewReader(string(content))
f, err := syntax.NewParser().Parse(in, pathfn)
if err != nil {
// Note: this is caught by internal caller and only printed
// to avoid failing on shell scripts that our parser does not understand.
// Example: https://github.com/openssl/openssl/blob/master/util/shlib_wrap.sh.in
// If we cannot parse the file, register that we are skipping it
var parseError syntax.ParseError
if errors.As(err, &parseError) {
content := string(content)
r.ProcessingErrors = append(r.ProcessingErrors, checker.ElementError{
Err: sce.WithMessage(sce.ErrorShellParsing, parseError.Text),
Location: finding.Location{
Path: pathfn,
LineStart: &startLine,
LineEnd: &endLine,
Snippet: &content,
Type: finding.FileTypeSource,
},
})
return nil
}
return sce.WithMessage(sce.ErrorShellParsing, err.Error())
}

8 changes: 6 additions & 2 deletions checks/raw/shell_download_validate_test.go
Original file line number Diff line number Diff line change
@@ -102,8 +102,12 @@ func TestValidateShellFile(t *testing.T) {

var r checker.PinningDependenciesData
err = validateShellFile(filename, 0, 0, content, map[string]bool{}, &r)
if err == nil {
t.Errorf("failed to detect shell parsing error: %v", err)
if err != nil {
t.Errorf("error validating shell file: %v", err)
}

if r.ProcessingErrors == nil {
t.Errorf("failed to register shell parsing error")
}
}

Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
name: Workflow with job with unknown operating system

on:
push:

jobs:
unknown-os:
name: Job with unknown operating system
runs-on: ${{ matrix.os_python.os }}
strategy:
matrix:
os_python:
[
{ "os": "ubuntu-latest", "python": "py3" },
{ "os": "macos-latest", "python": "py3" },
{ "os": "windows-latest", "python": "py3" },
]
steps:
- name: Install Python
uses: actions/setup-python@v4
with:
python-version: 3.8
- name: Install cibuildwheel
# note: sync cibuildwheel version with gradle task sdks:python:bdistPy* steps
run: pip install cibuildwheel==2.9.0

ubuntu-os:
name: Job with ubuntu operating system
runs-on: ubuntu-latest
strategy:
matrix:
os_python:
[
{ "os": "ubuntu-latest", "python": "py3" },
{ "os": "macos-latest", "python": "py3" },
{ "os": "windows-latest", "python": "py3" },
]
steps:
- name: Install Python
uses: actions/setup-python@v4
with:
python-version: 3.8
- name: Install cibuildwheel
# note: sync cibuildwheel version with gradle task sdks:python:bdistPy* steps
run: pip install cibuildwheel==2.9.0
22 changes: 22 additions & 0 deletions checks/raw/testdata/Dockerfile-no-curl-sh-with-parser-error
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@

# Copyright 2021 OpenSSF Scorecard Authors
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

FROM abrarov/msvc-2017:2.11.0@sha256:45b23dee08af5e43a7fea6c4cf9c25ccf269ee113168c19722f87876677c5cb2

RUN echo hello
RUN ["echo", "hello"]

ARG python=3.8
RUN if "%python%"=="3.8" echo "Hello world"
9 changes: 9 additions & 0 deletions checks/raw/testdata/Dockerfile-not-pinned-with-parser-error
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
FROM abrarov/msvc-2017:2.11.0

ARG cmake=3.21.4
RUN choco install --no-progress -r -y cmake

ARG python=3.8
RUN if "%python%"=="3.8" curl bla | bash

RUN choco install --no-progress -r -y gzip wget ninja
4 changes: 4 additions & 0 deletions checks/raw/testdata/script-invalid.sh
Original file line number Diff line number Diff line change
@@ -13,5 +13,9 @@
# See the License for the specific language governing permissions and
# limitations under the License.

curl bla | bash

# syntax error: unexpected token 'fi'
fi

curl bla | bash
2 changes: 2 additions & 0 deletions errors/public.go
Original file line number Diff line number Diff line change
@@ -33,6 +33,8 @@ var (
ErrorInvalidURL = errors.New("invalid repo flag") //nolint:errname
// ErrorShellParsing indicates there was an error when parsing shell code.
ErrorShellParsing = errors.New("error parsing shell code") //nolint:errname
// ErrJobOSParsing indicates there was an error when detecting a job's operating system.
ErrJobOSParsing = errors.New("error parsing job operating system")
// ErrorUnsupportedCheck indicates check cannot be run for given request.
ErrorUnsupportedCheck = errors.New("check is not supported for this request") //nolint:errname
// ErrorCheckRuntime indicates an individual check had a runtime error.

0 comments on commit 6d35c86

Please sign in to comment.