✨ Binary artifact exception for gradle-wrapper.jar when using validation action (ossf#2039)

* implement binary artifacts exception for validated gradle-wrapper.jar files

* add tests for binary artifact gradle wrapper verification exception

* fix issues for linter

* expect added jar in TestBinaryArtifacts Jar file test

* improve readability of raw/binary_artifact

* Binary-Artifact request types no longer includes FileBased

* add version requirement capability to gradle action check

* Refactor exception from checks/raw to checks/evaluation

* remove unnecessary len(files)

* flatten application of exception by moving to another function

* revert refactor to checks/evaluation

* flatten removal of validated wrappers

* create fileExists function

Co-authored-by: laurentsimon <64505099+laurentsimon@users.noreply.github.com>

Author: 2 people

checks/binary_artifact.go

@@ -27,7 +27,6 @@ const CheckBinaryArtifacts string = "Binary-Artifacts"
 //nolint
 func init() {
 	supportedRequestTypes := []checker.RequestType{
-		checker.FileBased,
 		checker.CommitBased,
 	}
 	if err := registerCheck(CheckBinaryArtifacts, BinaryArtifacts, supportedRequestTypes); err != nil {

checks/binary_artifact_test.go

@@ -41,7 +41,7 @@ func TestBinaryArtifacts(t *testing.T) {
 			inputFolder: "testdata/binaryartifacts/jars",
 			err:         nil,
 			expected: checker.CheckResult{
-				Score: 9,
+				Score: 8,
 			},
 		},
 		{

checks/raw/binary_artifact.go

@@ -17,18 +17,35 @@ package raw
 import (
 	"fmt"
 	"path/filepath"
+	"regexp"
 	"strings"
 	"unicode"
 
+	semver "github.com/Masterminds/semver/v3"
 	"github.com/h2non/filetype"
 	"github.com/h2non/filetype/types"
+	"github.com/rhysd/actionlint"
 
 	"github.com/ossf/scorecard/v4/checker"
 	"github.com/ossf/scorecard/v4/checks/fileparser"
 	"github.com/ossf/scorecard/v4/clients"
 	sce "github.com/ossf/scorecard/v4/errors"
 )
 
+var (
+	gradleWrapperValidationActionRegex             = regexp.MustCompile(`^gradle\/wrapper-validation-action@v?(.+)$`)
+	gradleWrapperValidationActionVersionConstraint = mustParseConstraint(`>= 1.0.0`)
+)
+
+// mustParseConstraint attempts parse of semver constraint, panics if fail.
+func mustParseConstraint(c string) *semver.Constraints {
+	if c, err := semver.NewConstraint(c); err != nil {
+		panic(fmt.Errorf("failed to parse constraint: %w", err))
+	} else {
+		return c
+	}
+}
+
 // BinaryArtifacts retrieves the raw data for the Binary-Artifacts check.
 func BinaryArtifacts(c clients.RepoClient) (checker.BinaryArtifactData, error) {
 	files := []checker.File{}
@@ -39,11 +56,45 @@ func BinaryArtifacts(c clients.RepoClient) (checker.BinaryArtifactData, error) {
 	if err != nil {
 		return checker.BinaryArtifactData{}, fmt.Errorf("%w", err)
 	}
+	// Ignore validated gradle-wrapper.jar files if present
+	files, err = excludeValidatedGradleWrappers(c, files)
+	if err != nil {
+		return checker.BinaryArtifactData{}, fmt.Errorf("%w", err)
+	}
 
 	// No error, return the files.
 	return checker.BinaryArtifactData{Files: files}, nil
 }
 
+// excludeValidatedGradleWrappers returns the subset of files not confirmed
+// to be Action-validated gradle-wrapper.jar files.
+func excludeValidatedGradleWrappers(c clients.RepoClient, files []checker.File) ([]checker.File, error) {
+	// Check if gradle-wrapper.jar present
+	if !fileExists(files, "gradle-wrapper.jar") {
+		return files, nil
+	}
+	// Gradle wrapper JARs present, so check that they are validated
+	ok, err := gradleWrapperValidated(c)
+	if err != nil {
+		return files, fmt.Errorf(
+			"failure checking for Gradle wrapper validating Action: %w", err)
+	}
+	if !ok {
+		// Gradle Wrappers not validated
+		return files, nil
+	}
+	// It has been confirmed that latest commit has validated JARs!
+	// Remove Gradle wrapper JARs from files.
+	filterFiles := []checker.File{}
+	for _, f := range files {
+		if filepath.Base(f.Path) != "gradle-wrapper.jar" {
+			filterFiles = append(filterFiles, f)
+		}
+	}
+	files = filterFiles
+	return files, nil
+}
+
 var checkBinaryFileContent fileparser.DoWhileTrueOnFileContent = func(path string, content []byte,
 	args ...interface{},
 ) (bool, error) {
@@ -127,3 +178,91 @@ func isText(content []byte) bool {
 	}
 	return true
 }
+
+// gradleWrapperValidated checks for the gradle-wrapper-verify action being
+// used in a non-failing workflow on the latest commit.
+func gradleWrapperValidated(c clients.RepoClient) (bool, error) {
+	gradleWrapperValidatingWorkflowFile := ""
+	err := fileparser.OnMatchingFileContentDo(c, fileparser.PathMatcher{
+		Pattern:       ".github/workflows/*",
+		CaseSensitive: false,
+	}, checkWorkflowValidatesGradleWrapper, &gradleWrapperValidatingWorkflowFile)
+	if err != nil {
+		return false, fmt.Errorf("%w", err)
+	}
+	if gradleWrapperValidatingWorkflowFile != "" {
+		// If validated, check that latest commit has a relevant successful run
+		runs, err := c.ListSuccessfulWorkflowRuns(gradleWrapperValidatingWorkflowFile)
+		if err != nil {
+			return false, fmt.Errorf("failure listing workflow runs: %w", err)
+		}
+		commits, err := c.ListCommits()
+		if err != nil {
+			return false, fmt.Errorf("failure listing commits: %w", err)
+		}
+		if len(commits) < 1 || len(runs) < 1 {
+			return false, nil
+		}
+		for _, r := range runs {
+			if *r.HeadSHA == commits[0].SHA {
+				// Commit has corresponding successful run!
+				return true, nil
+			}
+		}
+	}
+	return false, nil
+}
+
+// checkWorkflowValidatesGradleWrapper checks that the current workflow file
+// is indeed using the gradle/wrapper-validation-action action, else continues.
+func checkWorkflowValidatesGradleWrapper(path string, content []byte, args ...interface{}) (bool, error) {
+	validatingWorkflowFile, ok := args[0].(*string)
+	if !ok {
+		return false, fmt.Errorf("checkWorkflowValidatesGradleWrapper expects arg[0] of type *string: %w", errInvalidArgType)
+	}
+
+	action, errs := actionlint.Parse(content)
+	if len(errs) > 0 {
+		return true, errs[0]
+	}
+
+	for _, j := range action.Jobs {
+		for _, s := range j.Steps {
+			ea, ok := s.Exec.(*actionlint.ExecAction)
+			if !ok {
+				continue
+			}
+			if ea.Uses == nil {
+				continue
+			}
+			sms := gradleWrapperValidationActionRegex.FindStringSubmatch(ea.Uses.Value)
+			if len(sms) > 1 {
+				v, err := semver.NewVersion(sms[1])
+				if err != nil {
+					// Couldn't parse version, hopefully another step has
+					// a correct one.
+					continue
+				}
+				if !gradleWrapperValidationActionVersionConstraint.Check(v) {
+					// Version out of acceptable range.
+					continue
+				}
+				// OK! This is it.
+				*validatingWorkflowFile = filepath.Base(path)
+				return true, nil
+			}
+		}
+	}
+	return true, nil
+}
+
+// fileExists checks if a file of name name exists, including within
+// subdirectories.
+func fileExists(files []checker.File, name string) bool {
+	for _, f := range files {
+		if filepath.Base(f.Path) == name {
+			return true
+		}
+	}
+	return false
+}

checks/raw/binary_artifact_test.go

@@ -21,54 +21,149 @@ import (
 
 	"github.com/golang/mock/gomock"
 
+	"github.com/ossf/scorecard/v4/clients"
 	mockrepo "github.com/ossf/scorecard/v4/clients/mockclients"
 )
 
+func strptr(s string) *string {
+	return &s
+}
+
 // TestBinaryArtifact tests the BinaryArtifact checker.
 func TestBinaryArtifacts(t *testing.T) {
 	t.Parallel()
 	tests := []struct {
-		name   string
-		err    error
-		files  []string
-		expect int
+		name                   string
+		err                    error
+		files                  [][]string
+		successfulWorkflowRuns []clients.WorkflowRun
+		commits                []clients.Commit
+		getFileContentCount    int
+		expect                 int
 	}{
 		{
 			name: "Jar file",
 			err:  nil,
-			files: []string{
-				"../testdata/binaryartifacts/jars/aws-java-sdk-core-1.11.571.jar",
+			files: [][]string{
+				{"../testdata/binaryartifacts/jars/aws-java-sdk-core-1.11.571.jar"},
 			},
-			expect: 1,
+			getFileContentCount: 1,
+			expect:              1,
 		},
 		{
 			name: "Mach-O ARM64 executable",
 			err:  nil,
-			files: []string{
-				"../testdata/binaryartifacts/executables/darwin-arm64-bt",
+			files: [][]string{
+				{"../testdata/binaryartifacts/executables/darwin-arm64-bt"},
 			},
-			expect: 1,
+			getFileContentCount: 1,
+			expect:              1,
 		},
 		{
 			name: "non binary file",
 			err:  nil,
-			files: []string{
-				"../testdata/licensedir/withlicense/LICENSE",
+			files: [][]string{
+				{"../testdata/licensedir/withlicense/LICENSE"},
 			},
+			getFileContentCount: 1,
 		},
 		{
 			name: "non binary file",
 			err:  nil,
-			files: []string{
-				"../doesnotexist",
+			files: [][]string{
+				{"../doesnotexist"},
 			},
+			getFileContentCount: 1,
 		},
 		{
 			name: "printable character .lib",
 			err:  nil,
-			files: []string{
-				"../testdata/binaryartifacts/printable.lib",
+			files: [][]string{
+				{"../testdata/binaryartifacts/printable.lib"},
+			},
+			getFileContentCount: 1,
+		},
+		{
+			name: "gradle-wrapper.jar without verification action",
+			err:  nil,
+			files: [][]string{
+				{"../testdata/binaryartifacts/jars/gradle-wrapper.jar"},
+				{},
+			},
+			getFileContentCount: 1,
+			expect:              1,
+		},
+		{
+			name: "gradle-wrapper.jar with verification action",
+			err:  nil,
+			files: [][]string{
+				{"../testdata/binaryartifacts/jars/gradle-wrapper.jar"},
+				{
+					"../testdata/binaryartifacts/workflows/nonverify.yml",
+					"../testdata/binaryartifacts/workflows/verify.yml",
+				},
+			},
+			successfulWorkflowRuns: []clients.WorkflowRun{
+				{
+					HeadSHA: strptr("sha-a"),
+				},
+			},
+			commits: []clients.Commit{
+				{
+					SHA: "sha-a",
+				},
+				{
+					SHA: "sha-old",
+				},
+			},
+			getFileContentCount: 3,
+			expect:              0,
+		},
+		{
+			name: "gradle-wrapper.jar with non-verification action",
+			err:  nil,
+			files: [][]string{
+				{"../testdata/binaryartifacts/jars/gradle-wrapper.jar"},
+				{"../testdata/binaryartifacts/workflows/nonverify.yml"},
 			},
+			getFileContentCount: 2,
+			expect:              1,
+		},
+		{
+			name: "gradle-wrapper.jar with verification-failing commit",
+			err:  nil,
+			files: [][]string{
+				{"../testdata/binaryartifacts/jars/gradle-wrapper.jar"},
+				{"../testdata/binaryartifacts/workflows/verify.yml"},
+			},
+			successfulWorkflowRuns: []clients.WorkflowRun{
+				{
+					HeadSHA: strptr("sha-old"),
+				},
+			},
+			commits: []clients.Commit{
+				{
+					SHA: "sha-a",
+				},
+				{
+					SHA: "sha-old",
+				},
+			},
+			getFileContentCount: 2,
+			expect:              1,
+		},
+		{
+			name: "gradle-wrapper.jar with outdated verification action",
+			err:  nil,
+			files: [][]string{
+				{"../testdata/binaryartifacts/jars/gradle-wrapper.jar"},
+				{
+					"../testdata/binaryartifacts/workflows/nonverify.yml",
+					"../testdata/binaryartifacts/workflows/verify-outdated-action.yml",
+				},
+			},
+			getFileContentCount: 3,
+			expect:              1,
 		},
 	}
 	for _, tt := range tests {
@@ -78,15 +173,25 @@ func TestBinaryArtifacts(t *testing.T) {
 
 			ctrl := gomock.NewController(t)
 			mockRepoClient := mockrepo.NewMockRepoClient(ctrl)
-			mockRepoClient.EXPECT().ListFiles(gomock.Any()).Return(tt.files, nil)
-			mockRepoClient.EXPECT().GetFileContent(gomock.Any()).DoAndReturn(func(file string) ([]byte, error) {
-				// This will read the file and return the content
-				content, err := os.ReadFile(file)
-				if err != nil {
-					return content, fmt.Errorf("%w", err)
-				}
-				return content, nil
-			})
+			for _, files := range tt.files {
+				mockRepoClient.EXPECT().ListFiles(gomock.Any()).Return(files, nil)
+			}
+			for i := 0; i < tt.getFileContentCount; i++ {
+				mockRepoClient.EXPECT().GetFileContent(gomock.Any()).DoAndReturn(func(file string) ([]byte, error) {
+					// This will read the file and return the content
+					content, err := os.ReadFile(file)
+					if err != nil {
+						return content, fmt.Errorf("%w", err)
+					}
+					return content, nil
+				})
+			}
+			if tt.successfulWorkflowRuns != nil {
+				mockRepoClient.EXPECT().ListSuccessfulWorkflowRuns(gomock.Any()).Return(tt.successfulWorkflowRuns, nil)
+			}
+			if tt.commits != nil {
+				mockRepoClient.EXPECT().ListCommits().Return(tt.commits, nil)
+			}
 
 			f, err := BinaryArtifacts(mockRepoClient)