Generalize CheckFileContent functions (#1670)

Co-authored-by: Azeem Shaikh <azeems@google.com>

Author: azeemshaikh38

checks/dangerous_workflow.go

@@ -98,23 +98,37 @@ func DangerousWorkflow(c *checker.CheckRequest) checker.CheckResult {
 	data := patternCbData{
 		workflowPattern: make(map[dangerousResults]bool),
 	}
-	err := fileparser.CheckFilesContent(".github/workflows/*", false,
-		c, validateGitHubActionWorkflowPatterns, &data)
+	err := fileparser.OnMatchingFileContentDo(c.RepoClient, fileparser.PathMatcher{
+		Pattern:       ".github/workflows/*",
+		CaseSensitive: false,
+	},
+		validateGitHubActionWorkflowPatterns, c.Dlogger, &data)
 	return createResultForDangerousWorkflowPatterns(data, err)
 }
 
 // Check file content.
-func validateGitHubActionWorkflowPatterns(path string, content []byte, dl checker.DetailLogger,
-	data fileparser.FileCbData) (bool, error) {
+var validateGitHubActionWorkflowPatterns fileparser.DoWhileTrueOnFileContent = func(path string,
+	content []byte,
+	args ...interface{}) (bool, error) {
 	if !fileparser.IsWorkflowFile(path) {
 		return true, nil
 	}
 
+	if len(args) != 2 {
+		return false, fmt.Errorf(
+			"validateGitHubActionWorkflowPatterns requires exactly 2 arguments: %w", errInvalidArgLength)
+	}
+
 	// Verify the type of the data.
-	pdata, ok := data.(*patternCbData)
+	pdata, ok := args[1].(*patternCbData)
+	if !ok {
+		return false, fmt.Errorf(
+			"validateGitHubActionWorkflowPatterns expects arg[0] of type *patternCbData: %w", errInvalidArgType)
+	}
+	dl, ok := args[0].(checker.DetailLogger)
 	if !ok {
-		// This never happens.
-		panic("invalid type")
+		return false, fmt.Errorf(
+			"validateGitHubActionWorkflowPatterns expects arg[1] of type checker.DetailLogger: %w", errInvalidArgType)
 	}
 
 	if !fileparser.CheckFileContainsCommands(content, "#") {

checks/fileparser/listing.go

@@ -20,16 +20,16 @@ import (
 	"path"
 	"strings"
 
-	"github.com/ossf/scorecard/v4/checker"
 	"github.com/ossf/scorecard/v4/clients"
 	sce "github.com/ossf/scorecard/v4/errors"
 )
 
 // isMatchingPath uses 'pattern' to shell-match the 'path' and its filename
 // 'caseSensitive' indicates the match should be case-sensitive. Default: no.
-func isMatchingPath(pattern, fullpath string, caseSensitive bool) (bool, error) {
-	if !caseSensitive {
-		pattern = strings.ToLower(pattern)
+func isMatchingPath(fullpath string, matchPathTo PathMatcher) (bool, error) {
+	pattern := matchPathTo.Pattern
+	if !matchPathTo.CaseSensitive {
+		pattern = strings.ToLower(matchPathTo.Pattern)
 		fullpath = strings.ToLower(fullpath)
 	}
 
@@ -55,87 +55,30 @@ func isTestdataFile(fullpath string) bool {
 		strings.Contains(fullpath, "/testdata/")
 }
 
-// FileCbData is any data the caller can act upon
-// to keep state.
-type FileCbData interface{}
-
-// FileContentCb is the callback.
-// The bool returned indicates whether the CheckFilesContent2
-// should continue iterating over files or not.
-type FileContentCb func(path string, content []byte,
-	dl checker.DetailLogger, data FileCbData) (bool, error)
-
-// CheckFilesContent downloads the tar of the repository and calls the onFileContent() function
-// shellPathFnPattern is used for https://golang.org/pkg/path/#Match
-// Warning: the pattern is used to match (1) the entire path AND (2) the filename alone. This means:
-// 	- To scope the search to a directory, use "./dirname/*". Example, for the root directory,
-// 		use "./*".
-//	- A pattern such as "*mypatern*" will match files containing mypattern in *any* directory.
-func CheckFilesContent(shellPathFnPattern string,
-	caseSensitive bool,
-	c *checker.CheckRequest,
-	onFileContent FileContentCb,
-	data FileCbData,
-) error {
-	predicate := func(filepath string) (bool, error) {
-		// Filter out test files.
-		if isTestdataFile(filepath) {
-			return false, nil
-		}
-		// Filter out files based on path/names using the pattern.
-		b, err := isMatchingPath(shellPathFnPattern, filepath, caseSensitive)
-		if err != nil {
-			return false, err
-		}
-		return b, nil
-	}
-
-	matchedFiles, err := c.RepoClient.ListFiles(predicate)
-	if err != nil {
-		// nolint: wrapcheck
-		return err
-	}
-
-	for _, file := range matchedFiles {
-		content, err := c.RepoClient.GetFileContent(file)
-		if err != nil {
-			//nolint
-			return err
-		}
-
-		continueIter, err := onFileContent(file, content, c.Dlogger, data)
-		if err != nil {
-			return err
-		}
-
-		if !continueIter {
-			break
-		}
-	}
-
-	return nil
+// PathMatcher represents a query for a filepath.
+type PathMatcher struct {
+	Pattern       string
+	CaseSensitive bool
 }
 
-// FileContentCbV6 is the callback.
-// The bool returned indicates whether the CheckFilesContent2
-// should continue iterating over files or not.
-type FileContentCbV6 func(path string, content []byte, data FileCbData) (bool, error)
-
-// CheckFilesContentV6 is the same as CheckFilesContent
-// but for use with separated check/policy code.
-func CheckFilesContentV6(shellPathFnPattern string,
-	caseSensitive bool,
-	repoClient clients.RepoClient,
-	onFileContent FileContentCbV6,
-	data FileCbData,
-) error {
+// DoWhileTrueOnFileContent takes a filepath, its content and
+// optional variadic args. It returns a boolean indicating whether
+// iterating over next files should continue.
+type DoWhileTrueOnFileContent func(path string, content []byte, args ...interface{}) (bool, error)
+
+// OnMatchingFileContentDo matches all files listed by `repoClient` against `matchPathTo`
+// and on every successful match, runs onFileContent fn on the file's contents.
+// Continues iterating along the matched files until onFileContent returns
+// either a false value or an error.
+func OnMatchingFileContentDo(repoClient clients.RepoClient, matchPathTo PathMatcher,
+	onFileContent DoWhileTrueOnFileContent, args ...interface{}) error {
 	predicate := func(filepath string) (bool, error) {
 		// Filter out test files.
 		if isTestdataFile(filepath) {
 			return false, nil
 		}
 		// Filter out files based on path/names using the pattern.
-		b, err := isMatchingPath(shellPathFnPattern, filepath, caseSensitive)
+		b, err := isMatchingPath(filepath, matchPathTo)
 		if err != nil {
 			return false, err
 		}
@@ -144,18 +87,16 @@ func CheckFilesContentV6(shellPathFnPattern string,
 
 	matchedFiles, err := repoClient.ListFiles(predicate)
 	if err != nil {
-		// nolint: wrapcheck
-		return err
+		return fmt.Errorf("error during ListFiles: %w", err)
 	}
 
 	for _, file := range matchedFiles {
 		content, err := repoClient.GetFileContent(file)
 		if err != nil {
-			//nolint
-			return err
+			return fmt.Errorf("error during GetFileContent: %w", err)
 		}
 
-		continueIter, err := onFileContent(file, content, data)
+		continueIter, err := onFileContent(file, content, args...)
 		if err != nil {
 			return err
 		}

checks/fileparser/listing_test.go

@@ -20,7 +20,6 @@ import (
 
 	"github.com/golang/mock/gomock"
 
-	"github.com/ossf/scorecard/v4/checker"
 	mockrepo "github.com/ossf/scorecard/v4/clients/mockclients"
 )
 
@@ -316,7 +315,10 @@ func Test_isMatchingPath(t *testing.T) {
 		tt := tt // Re-initializing variable so it is not changed while executing the closure below
 		t.Run(tt.name, func(t *testing.T) {
 			t.Parallel()
-			got, err := isMatchingPath(tt.args.pattern, tt.args.fullpath, tt.args.caseSensitive)
+			got, err := isMatchingPath(tt.args.fullpath, PathMatcher{
+				Pattern:       tt.args.pattern,
+				CaseSensitive: tt.args.caseSensitive,
+			})
 			if (err != nil) != tt.wantErr {
 				t.Errorf("isMatchingPath() error = %v, wantErr %v", err, tt.wantErr)
 				return
@@ -385,8 +387,8 @@ func Test_isTestdataFile(t *testing.T) {
 	}
 }
 
-// TestCheckFilesContentV6 tests the CheckFilesContentV6 function.
-func TestCheckFilesContentV6(t *testing.T) {
+// TestOnMatchingFileContentDo tests the OnMatchingFileContent function.
+func TestOnMatchingFileContent(t *testing.T) {
 	t.Parallel()
 	//nolint
 	tests := []struct {
@@ -448,50 +450,6 @@ func TestCheckFilesContentV6(t *testing.T) {
 				"Dockerfile.template.template.template.template",
 			},
 		},
-	}
-
-	for _, tt := range tests {
-		tt := tt // Re-initializing variable so it is not changed while executing the closure below
-		t.Run(tt.name, func(t *testing.T) {
-			t.Parallel()
-			x := func(path string, content []byte, data FileCbData) (bool, error) {
-				if tt.shouldFuncFail {
-					//nolint
-					return false, errors.New("test error")
-				}
-				if tt.shouldGetPredicateFail {
-					return false, nil
-				}
-				return true, nil
-			}
-
-			ctrl := gomock.NewController(t)
-			mockRepo := mockrepo.NewMockRepoClient(ctrl)
-			mockRepo.EXPECT().ListFiles(gomock.Any()).Return(tt.files, nil).AnyTimes()
-			mockRepo.EXPECT().GetFileContent(gomock.Any()).Return(nil, nil).AnyTimes()
-
-			result := CheckFilesContentV6(tt.shellPattern, tt.caseSensitive, mockRepo, x, x)
-
-			if tt.wantErr && result == nil {
-				t.Errorf("CheckFilesContentV6() = %v, want %v test name %v", result, tt.wantErr, tt.name)
-			}
-		})
-	}
-}
-
-// TestCheckFilesContent tests the CheckFilesContent function.
-func TestCheckFilesContent(t *testing.T) {
-	t.Parallel()
-	//nolint
-	tests := []struct {
-		name                   string
-		wantErr                bool
-		shellPattern           string
-		caseSensitive          bool
-		shouldFuncFail         bool
-		shouldGetPredicateFail bool
-		files                  []string
-	}{
 		{
 			name:          "no files",
 			shellPattern:  "Dockerfile",
@@ -548,8 +506,7 @@ func TestCheckFilesContent(t *testing.T) {
 		tt := tt // Re-initializing variable so it is not changed while executing the closure below
 		t.Run(tt.name, func(t *testing.T) {
 			t.Parallel()
-			x := func(path string, content []byte,
-				dl checker.DetailLogger, data FileCbData) (bool, error) {
+			x := func(path string, content []byte, args ...interface{}) (bool, error) {
 				if tt.shouldFuncFail {
 					//nolint
 					return false, errors.New("test error")
@@ -565,14 +522,13 @@ func TestCheckFilesContent(t *testing.T) {
 			mockRepo.EXPECT().ListFiles(gomock.Any()).Return(tt.files, nil).AnyTimes()
 			mockRepo.EXPECT().GetFileContent(gomock.Any()).Return(nil, nil).AnyTimes()
 
-			c := checker.CheckRequest{
-				RepoClient: mockRepo,
-			}
-
-			result := CheckFilesContent(tt.shellPattern, tt.caseSensitive, &c, x, x)
+			result := OnMatchingFileContentDo(mockRepo, PathMatcher{
+				Pattern:       tt.shellPattern,
+				CaseSensitive: tt.caseSensitive,
+			}, x)
 
 			if tt.wantErr && result == nil {
-				t.Errorf("CheckFilesContentV6() = %v, want %v test name %v", result, tt.wantErr, tt.name)
+				t.Errorf("OnMatchingFileContentDo() = %v, want %v test name %v", result, tt.wantErr, tt.name)
 			}
 		})
 	}

checks/fuzzing.go

@@ -36,11 +36,13 @@ func init() {
 
 func checkCFLite(c *checker.CheckRequest) (bool, error) {
 	result := false
-	e := fileparser.CheckFilesContent(".clusterfuzzlite/Dockerfile", true, c,
-		func(path string, content []byte, dl checker.DetailLogger, data fileparser.FileCbData) (bool, error) {
-			result = fileparser.CheckFileContainsCommands(content, "#")
-			return false, nil
-		}, nil)
+	e := fileparser.OnMatchingFileContentDo(c.RepoClient, fileparser.PathMatcher{
+		Pattern:       ".clusterfuzzlite/Dockerfile",
+		CaseSensitive: true,
+	}, func(path string, content []byte, args ...interface{}) (bool, error) {
+		result = fileparser.CheckFileContainsCommands(content, "#")
+		return false, nil
+	}, nil)
 	if e != nil {
 		return result, fmt.Errorf("%w", e)
 	}