✨ Support Binary-Artifacts check again for local repos (ossf#3415)

* invert workflow check and explain early exit. Signed-off-by: Spencer Schrock <[email protected]> * make workflow run validation optional. Signed-off-by: Spencer Schrock <[email protected]> * mark binary artifacts as local file friendly. Signed-off-by: Spencer Schrock <[email protected]> * add test for gradle wrapper without workflow run support Signed-off-by: Spencer Schrock <[email protected]> * fix policy tests and make their names more clear. Signed-off-by: Spencer Schrock <[email protected]> --------- Signed-off-by: Spencer Schrock <[email protected]> Signed-off-by: Allen Shearin <[email protected]>
kgangerlm · Nov 13, 2023 · 24398b3 · 24398b3
1 parent 474d26c
commit 24398b3
Show file tree

Hide file tree

Showing 4 changed files with 90 additions and 37 deletions.
diff --git a/checks/binary_artifact.go b/checks/binary_artifact.go
@@ -24,10 +24,11 @@ import (
 // CheckBinaryArtifacts is the exported name for Binary-Artifacts check.
 const CheckBinaryArtifacts string = "Binary-Artifacts"
 
-//nolint
+//nolint:gochecknoinits
 func init() {
 	supportedRequestTypes := []checker.RequestType{
 		checker.CommitBased,
+		checker.FileBased,
 	}
 	if err := registerCheck(CheckBinaryArtifacts, BinaryArtifacts, supportedRequestTypes); err != nil {
 		// this should never happen

diff --git a/checks/raw/binary_artifact.go b/checks/raw/binary_artifact.go
@@ -15,6 +15,7 @@
 package raw
 
 import (
+	"errors"
 	"fmt"
 	"path/filepath"
 	"regexp"
@@ -201,26 +202,35 @@ func gradleWrapperValidated(c clients.RepoClient) (bool, error) {
 	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 {
+	// no matching files, validation failed
+	if gradleWrapperValidatingWorkflowFile == "" {
+		return false, nil
+	}
+
+	// If validated, check that latest commit has a relevant successful run
+	runs, err := c.ListSuccessfulWorkflowRuns(gradleWrapperValidatingWorkflowFile)
+	if err != nil {
+		// some clients, such as the local file client, don't support this feature
+		// claim unvalidated, so that other parts of the check can still be used.
+		if errors.Is(err, clients.ErrUnsupportedFeature) {
 			return false, nil
 		}
-		for _, r := range runs {
-			if *r.HeadSHA == commits[0].SHA {
-				// Commit has corresponding successful run!
-				return true, 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
 }
 

diff --git a/checks/raw/binary_artifact_test.go b/checks/raw/binary_artifact_test.go
@@ -256,3 +256,35 @@ func TestBinaryArtifacts(t *testing.T) {
 		})
 	}
 }
+
+func TestBinaryArtifacts_workflow_runs_unsupported(t *testing.T) {
+	ctrl := gomock.NewController(t)
+	mockRepoClient := mockrepo.NewMockRepoClient(ctrl)
+	const jarFile = "gradle-wrapper.jar"
+	const verifyWorkflow = ".github/workflows/verify.yaml"
+	files := []string{jarFile, verifyWorkflow}
+	mockRepoClient.EXPECT().ListFiles(gomock.Any()).Return(files, nil).AnyTimes()
+	mockRepoClient.EXPECT().GetFileContent(jarFile).DoAndReturn(func(file string) ([]byte, error) {
+		content, err := os.ReadFile("../testdata/binaryartifacts/jars/gradle-wrapper.jar")
+		if err != nil {
+			return nil, fmt.Errorf("%w", err)
+		}
+		return content, nil
+	}).AnyTimes()
+	mockRepoClient.EXPECT().GetFileContent(verifyWorkflow).DoAndReturn(func(file string) ([]byte, error) {
+		content, err := os.ReadFile("../testdata/binaryartifacts/workflows/verify.yaml")
+		if err != nil {
+			return nil, fmt.Errorf("%w", err)
+		}
+		return content, nil
+	}).AnyTimes()
+
+	mockRepoClient.EXPECT().ListSuccessfulWorkflowRuns(gomock.Any()).Return(nil, clients.ErrUnsupportedFeature).AnyTimes()
+	got, err := BinaryArtifacts(mockRepoClient)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if len(got.Files) != 1 {
+		t.Errorf("expected 1 file, got %d", len(got.Files))
+	}
+}
diff --git a/policy/policy_test.go b/policy/policy_test.go
@@ -217,7 +217,7 @@ func TestIsSupportedCheck(t *testing.T) {
 	result := isSupportedCheck(checkName, requiredRequestTypes)
 
 	// Assert the result
-	expectedResult := false
+	expectedResult := true
 	if result != expectedResult {
 		t.Errorf("Unexpected result: got %v, want %v", result, expectedResult)
 	}
@@ -308,49 +308,59 @@ func TestGetEnabled(t *testing.T) {
 
 	tests := []struct {
 		name                  string
-		sp                    *ScorecardPolicy
+		policyFile            string
 		argsChecks            []string
 		requiredRequestTypes  []checker.RequestType
 		expectedEnabledChecks int
 		expectedError         bool
 	}{
 		{
-			name:                  "With ScorecardPolicy and argsChecks",
-			sp:                    &ScorecardPolicy{},
+			name:                  "checks limited to those specified by checks arg",
 			argsChecks:            []string{"Binary-Artifacts"},
-			requiredRequestTypes:  []checker.RequestType{checker.FileBased, checker.CommitBased},
-			expectedEnabledChecks: 0,
-			expectedError:         true,
+			requiredRequestTypes:  []checker.RequestType{checker.FileBased},
+			expectedEnabledChecks: 1,
+			expectedError:         false,
 		},
 		{
-			name:                  "With ScorecardPolicy and unsupported check",
-			sp:                    &ScorecardPolicy{},
+			name:                  "mix of supported and unsupported checks",
 			argsChecks:            []string{"Binary-Artifacts", "UnsupportedCheck"},
 			requiredRequestTypes:  []checker.RequestType{checker.FileBased, checker.CommitBased},
-			expectedEnabledChecks: 0,
+			expectedEnabledChecks: 1,
 			expectedError:         true,
 		},
 		{
-			name:                  "Without ScorecardPolicy and argsChecks",
-			sp:                    nil,
+			name:                  "request types limit enabled checks",
 			argsChecks:            []string{},
 			requiredRequestTypes:  []checker.RequestType{checker.FileBased, checker.CommitBased},
-			expectedEnabledChecks: 4, // All checks are enabled by default
+			expectedEnabledChecks: 5, // All checks which are FileBased and CommitBased
 			expectedError:         false,
 		},
 		{
-			name:                  "With ScorecardPolicy and missing policy",
-			sp:                    &ScorecardPolicy{},
-			argsChecks:            []string{"Binary-Artifacts"},
-			requiredRequestTypes:  []checker.RequestType{checker.FileBased, checker.CommitBased},
-			expectedEnabledChecks: 0,
-			expectedError:         true,
+			name:                  "all checks in policy file enabled",
+			policyFile:            "testdata/policy-ok.yaml",
+			argsChecks:            []string{},
+			requiredRequestTypes:  []checker.RequestType{},
+			expectedEnabledChecks: 3,
+			expectedError:         false,
 		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			enabledChecks, err := GetEnabled(tt.sp, tt.argsChecks, tt.requiredRequestTypes)
+			var sp *ScorecardPolicy
+			if tt.policyFile != "" {
+				policyBytes, err := os.ReadFile(tt.policyFile)
+				if err != nil {
+					t.Fatalf("reading policy file: %v", err)
+				}
+				pol, err := parseFromYAML(policyBytes)
+				if err != nil {
+					t.Fatalf("parsing policy file: %v", err)
+				}
+				sp = pol
+			}
+
+			enabledChecks, err := GetEnabled(sp, tt.argsChecks, tt.requiredRequestTypes)
 
 			if len(enabledChecks) != tt.expectedEnabledChecks {
 				t.Errorf("Unexpected number of enabled checks: got %v, want %v", len(enabledChecks), tt.expectedEnabledChecks)