From 9bf1cdc9cea92ca2e970989de8ce1d231781cceb Mon Sep 17 00:00:00 2001 From: Azeem Shaikh Date: Sun, 25 Jul 2021 17:47:36 -0700 Subject: [PATCH] Update ListFiles API to return error (#746) Co-authored-by: Azeem Shaikh --- checks/automatic_dependency_update.go | 2 +- checks/binary_artifact.go | 2 +- checks/checkforcontent.go | 60 +++++---------------------- checks/checkforfile.go | 32 +++----------- checks/frozen_deps.go | 12 +++--- checks/permissions.go | 2 +- checks/security_policy.go | 4 +- clients/githubrepo/client.go | 2 +- clients/githubrepo/tarball.go | 10 +++-- clients/githubrepo/tarball_test.go | 18 +++++--- clients/repo_client.go | 2 +- 11 files changed, 49 insertions(+), 97 deletions(-) diff --git a/checks/automatic_dependency_update.go b/checks/automatic_dependency_update.go index 2ecab308..42db1e23 100644 --- a/checks/automatic_dependency_update.go +++ b/checks/automatic_dependency_update.go @@ -29,7 +29,7 @@ func init() { // AutomaticDependencyUpdate will check the repository if it contains Automatic dependency update. func AutomaticDependencyUpdate(c *checker.CheckRequest) checker.CheckResult { - r, err := CheckIfFileExists2(CheckAutomaticDependencyUpdate, c, fileExists) + r, err := CheckIfFileExists(CheckAutomaticDependencyUpdate, c, fileExists) if err != nil { return checker.CreateRuntimeErrorResult(CheckAutomaticDependencyUpdate, err) } diff --git a/checks/binary_artifact.go b/checks/binary_artifact.go index fb24c904..c11dec4c 100644 --- a/checks/binary_artifact.go +++ b/checks/binary_artifact.go @@ -34,7 +34,7 @@ func init() { // BinaryArtifacts will check the repository if it contains binary artifacts. func BinaryArtifacts(c *checker.CheckRequest) checker.CheckResult { - r, err := CheckFilesContent2("*", false, c, checkBinaryFileContent) + r, err := CheckFilesContent("*", false, c, checkBinaryFileContent) if err != nil { return checker.CreateRuntimeErrorResult(CheckBinaryArtifacts, err) } diff --git a/checks/checkforcontent.go b/checks/checkforcontent.go index 40922e8d..72d8002b 100644 --- a/checks/checkforcontent.go +++ b/checks/checkforcontent.go @@ -62,69 +62,31 @@ func isScorecardTestFile(owner, repo, fullpath string) bool { // use "./*". // - A pattern such as "*mypatern*" will match files containing mypattern in *any* directory. //nolint -func CheckFilesContent(checkName, shellPathFnPattern string, - caseSensitive bool, - c *checker.CheckRequest, - onFileContent func(path string, content []byte, - Logf func(s string, f ...interface{})) (bool, error), -) checker.CheckResult { - predicate := func(filepath string) bool { - // Filter out Scorecard's own test files. - if isScorecardTestFile(c.Owner, c.Repo, filepath) { - return false - } - // Filter out files based on path/names using the pattern. - b, err := isMatchingPath(shellPathFnPattern, filepath, caseSensitive) - if err != nil { - return false - } - return b - } - res := true - for _, file := range c.RepoClient.ListFiles(predicate) { - content, err := c.RepoClient.GetFileContent(file) - if err != nil { - return checker.MakeRetryResult(checkName, err) - } - - rr, err := onFileContent(file, content, c.Logf) - if err != nil { - return checker.MakeFailResult(checkName, err) - } - // We don't return rightway to let the onFileContent() - // handler log. - if !rr { - res = false - } - } - if res { - return checker.MakePassResult(checkName) - } - - return checker.MakeFailResult(checkName, nil) -} - -// UPGRADEv2: to rename to CheckFilesContent. -func CheckFilesContent2(shellPathFnPattern string, +func CheckFilesContent(shellPathFnPattern string, caseSensitive bool, c *checker.CheckRequest, onFileContent func(path string, content []byte, dl checker.DetailLogger) (bool, error), ) (bool, error) { - predicate := func(filepath string) bool { + predicate := func(filepath string) (bool, error) { // Filter out Scorecard's own test files. if isScorecardTestFile(c.Owner, c.Repo, filepath) { - return false + return false, nil } // Filter out files based on path/names using the pattern. b, err := isMatchingPath(shellPathFnPattern, filepath, caseSensitive) if err != nil { - return false + return false, err } - return b + return b, nil } res := true - for _, file := range c.RepoClient.ListFiles(predicate) { + matchedFiles, err := c.RepoClient.ListFiles(predicate) + if err != nil { + // nolint: wrapcheck + return false, err + } + for _, file := range matchedFiles { content, err := c.RepoClient.GetFileContent(file) if err != nil { //nolint diff --git a/checks/checkforfile.go b/checks/checkforfile.go index 8c9d9685..b6c32cbb 100644 --- a/checks/checkforfile.go +++ b/checks/checkforfile.go @@ -21,33 +21,13 @@ import ( // CheckIfFileExists downloads the tar of the repository and calls the onFile() to check // for the occurrence. func CheckIfFileExists(checkName string, c *checker.CheckRequest, onFile func(name string, - Logf func(s string, f ...interface{})) (bool, error)) checker.CheckResult { - for _, filename := range c.RepoClient.ListFiles(func(string) bool { return true }) { - rr, err := onFile(filename, c.Logf) - if err != nil { - return checker.CheckResult{ - Name: checkName, - Pass: false, - Confidence: checker.MaxResultConfidence, - Error: err, - } - } - - if rr { - return checker.MakePassResult(checkName) - } - } - const confidence = 5 - return checker.CheckResult{ - Name: checkName, - Pass: false, - Confidence: confidence, - } -} - -func CheckIfFileExists2(checkName string, c *checker.CheckRequest, onFile func(name string, dl checker.DetailLogger) (bool, error)) (bool, error) { - for _, filename := range c.RepoClient.ListFiles(func(string) bool { return true }) { + matchedFiles, err := c.RepoClient.ListFiles(func(string) (bool, error) { return true, nil }) + if err != nil { + // nolint: wrapcheck + return false, err + } + for _, filename := range matchedFiles { rr, err := onFile(filename, c.Dlogger) if err != nil { return false, err diff --git a/checks/frozen_deps.go b/checks/frozen_deps.go index bc83d17e..c46ca260 100644 --- a/checks/frozen_deps.go +++ b/checks/frozen_deps.go @@ -72,7 +72,7 @@ func FrozenDeps(c *checker.CheckRequest) checker.CheckResult { // TODO(laurent): need to support GCB pinning. func isShellScriptFreeOfInsecureDownloads(c *checker.CheckRequest) checker.CheckResult { - r, err := CheckFilesContent2("*", false, c, validateShellScriptIsFreeOfInsecureDownloads) + r, err := CheckFilesContent("*", false, c, validateShellScriptIsFreeOfInsecureDownloads) return createResultForIsShellScriptFreeOfInsecureDownloads(r, err) } @@ -105,7 +105,7 @@ func validateShellScriptIsFreeOfInsecureDownloads(pathfn string, content []byte, } func isDockerfileFreeOfInsecureDownloads(c *checker.CheckRequest) checker.CheckResult { - r, err := CheckFilesContent2("*Dockerfile*", false, c, validateDockerfileIsFreeOfInsecureDownloads) + r, err := CheckFilesContent("*Dockerfile*", false, c, validateDockerfileIsFreeOfInsecureDownloads) return createResultForIsDockerfileFreeOfInsecureDownloads(r, err) } @@ -168,7 +168,7 @@ func validateDockerfileIsFreeOfInsecureDownloads(pathfn string, content []byte, } func isDockerfilePinned(c *checker.CheckRequest) checker.CheckResult { - r, err := CheckFilesContent2("*Dockerfile*", false, c, validateDockerfileIsPinned) + r, err := CheckFilesContent("*Dockerfile*", false, c, validateDockerfileIsPinned) return createResultForIsDockerfilePinned(r, err) } @@ -271,7 +271,7 @@ func validateDockerfileIsPinned(pathfn string, content []byte, } func isGitHubWorkflowScriptFreeOfInsecureDownloads(c *checker.CheckRequest) checker.CheckResult { - r, err := CheckFilesContent2(".github/workflows/*", false, c, validateGitHubWorkflowIsFreeOfInsecureDownloads) + r, err := CheckFilesContent(".github/workflows/*", false, c, validateGitHubWorkflowIsFreeOfInsecureDownloads) return createResultForIsGitHubWorkflowScriptFreeOfInsecureDownloads(r, err) } @@ -355,7 +355,7 @@ func validateGitHubWorkflowIsFreeOfInsecureDownloads(pathfn string, content []by // Check pinning of github actions in workflows. func isGitHubActionsWorkflowPinned(c *checker.CheckRequest) checker.CheckResult { - r, err := CheckFilesContent2(".github/workflows/*", true, c, validateGitHubActionWorkflow) + r, err := CheckFilesContent(".github/workflows/*", true, c, validateGitHubActionWorkflow) return createResultForIsGitHubActionsWorkflowPinned(r, err) } @@ -415,7 +415,7 @@ func validateGitHubActionWorkflow(pathfn string, content []byte, dl checker.Deta // Check presence of lock files thru validatePackageManagerFile(). func isPackageManagerLockFilePresent(c *checker.CheckRequest) checker.CheckResult { - r, err := CheckIfFileExists2(CheckFrozenDeps, c, validatePackageManagerFile) + r, err := CheckIfFileExists(CheckFrozenDeps, c, validatePackageManagerFile) if err != nil { return checker.CreateRuntimeErrorResult(CheckFrozenDeps, err) } diff --git a/checks/permissions.go b/checks/permissions.go index 8a3d71bc..931ad977 100644 --- a/checks/permissions.go +++ b/checks/permissions.go @@ -32,7 +32,7 @@ func init() { } func leastPrivilegedTokens(c *checker.CheckRequest) checker.CheckResult { - r, err := CheckFilesContent2(".github/workflows/*", false, c, validateGitHubActionTokenPermissions) + r, err := CheckFilesContent(".github/workflows/*", false, c, validateGitHubActionTokenPermissions) return createResultForLeastPrivilegeTokens(r, err) } diff --git a/checks/security_policy.go b/checks/security_policy.go index 011e3e91..9d577eb7 100644 --- a/checks/security_policy.go +++ b/checks/security_policy.go @@ -41,7 +41,7 @@ func SecurityPolicy(c *checker.CheckRequest) checker.CheckResult { } return false, nil } - r, err := CheckIfFileExists2(CheckSecurityPolicy, c, onFile) + r, err := CheckIfFileExists(CheckSecurityPolicy, c, onFile) if err != nil { return checker.CreateRuntimeErrorResult(CheckSecurityPolicy, err) } @@ -67,7 +67,7 @@ func SecurityPolicy(c *checker.CheckRequest) checker.CheckResult { } return false, nil } - r, err = CheckIfFileExists2(CheckSecurityPolicy, dotGitHub, onFile) + r, err = CheckIfFileExists(CheckSecurityPolicy, dotGitHub, onFile) if err != nil { return checker.CreateRuntimeErrorResult(CheckSecurityPolicy, err) } diff --git a/clients/githubrepo/client.go b/clients/githubrepo/client.go index f2fccc27..acb68aa2 100644 --- a/clients/githubrepo/client.go +++ b/clients/githubrepo/client.go @@ -54,7 +54,7 @@ func (client *Client) InitRepo(owner, repoName string) error { return nil } -func (client *Client) ListFiles(predicate func(string) bool) []string { +func (client *Client) ListFiles(predicate func(string) (bool, error)) ([]string, error) { return client.tarball.listFiles(predicate) } diff --git a/clients/githubrepo/tarball.go b/clients/githubrepo/tarball.go index 3d6fca27..109284bc 100644 --- a/clients/githubrepo/tarball.go +++ b/clients/githubrepo/tarball.go @@ -201,14 +201,18 @@ func (handler *tarballHandler) extractTarball() error { return nil } -func (handler *tarballHandler) listFiles(predicate func(string) bool) []string { +func (handler *tarballHandler) listFiles(predicate func(string) (bool, error)) ([]string, error) { ret := make([]string, 0) for _, file := range handler.files { - if predicate(file) { + matches, err := predicate(file) + if err != nil { + return nil, err + } + if matches { ret = append(ret, file) } } - return ret + return ret, nil } func (handler *tarballHandler) getFileContent(filename string) ([]byte, error) { diff --git a/clients/githubrepo/tarball_test.go b/clients/githubrepo/tarball_test.go index 65e03ca2..ce93557d 100644 --- a/clients/githubrepo/tarball_test.go +++ b/clients/githubrepo/tarball_test.go @@ -28,7 +28,8 @@ import ( ) type listfileTest struct { - predicate func(string) bool + predicate func(string) (bool, error) + err error outcome []string } @@ -79,17 +80,17 @@ func TestExtractTarball(t *testing.T) { listfileTests: []listfileTest{ { // Returns all files in the tarball. - predicate: func(string) bool { return true }, + predicate: func(string) (bool, error) { return true, nil }, outcome: []string{"file0", "dir1/file1", "dir1/dir2/file2"}, }, { // Skips all files inside `dir1/dir2` directory. - predicate: func(fn string) bool { return !strings.HasPrefix(fn, "dir1/dir2") }, + predicate: func(fn string) (bool, error) { return !strings.HasPrefix(fn, "dir1/dir2"), nil }, outcome: []string{"file0", "dir1/file1"}, }, { // Skips all files. - predicate: func(fn string) bool { return false }, + predicate: func(fn string) (bool, error) { return false, nil }, outcome: []string{}, }, }, @@ -132,10 +133,15 @@ func TestExtractTarball(t *testing.T) { // Test ListFiles API. for _, listfiletest := range testcase.listfileTests { + matchedFiles, err := handler.listFiles(listfiletest.predicate) + if !errors.Is(err, listfiletest.err) { + t.Errorf("test failed: expected - %v, got - %v", listfiletest.err, err) + continue + } if !cmp.Equal(listfiletest.outcome, - handler.listFiles(listfiletest.predicate), + matchedFiles, cmpopts.SortSlices(isSortedString)) { - t.Errorf("test failed: expected - %q, got - %q", listfiletest.outcome, handler.listFiles(listfiletest.predicate)) + t.Errorf("test failed: expected - %q, got - %q", listfiletest.outcome, matchedFiles) } } diff --git a/clients/repo_client.go b/clients/repo_client.go index 951b78d0..73fa133d 100644 --- a/clients/repo_client.go +++ b/clients/repo_client.go @@ -40,7 +40,7 @@ func NewRepoUnavailableError(err error) error { type RepoClient interface { InitRepo(owner, repo string) error - ListFiles(predicate func(string) bool) []string + ListFiles(predicate func(string) (bool, error)) ([]string, error) GetFileContent(filename string) ([]byte, error) ListMergedPRs() ([]PullRequest, error) GetDefaultBranch() (BranchRef, error)