⚠️ Switch RepoClient file access to io.ReadCloser (#3912)

* change file access method to io.ReadCloser callers don't always need the full file. large files are slow and can cause crashes. Signed-off-by: Spencer Schrock <sschrock@google.com> * switch tests to hardcoded readers Previously they returned bytes or strings, which have corresponding NewReader types. Since they don't need to be closed, io.NopCloser works well to give them a fake Close. Signed-off-by: Spencer Schrock <sschrock@google.com> * switch tests which called os.ReadFile to os.Open os.File fufills io.ReadCloser, so this is an easy change Signed-off-by: Spencer Schrock <sschrock@google.com> * break tarball tests into two steps: reader and read The rest of the test was kept the same to minimize the change. Signed-off-by: Spencer Schrock <sschrock@google.com> * ossfuzz doesn't implement GetFileReader Signed-off-by: Spencer Schrock <sschrock@google.com> * appease linter during refactor Signed-off-by: Spencer Schrock <sschrock@google.com> * switch git client to new method add check which ensures git client fulfills the interface Signed-off-by: Spencer Schrock <sschrock@google.com> --------- Signed-off-by: Spencer Schrock <sschrock@google.com>
ossf · Mar 5, 2024 · d55dbd1 · d55dbd1
1 parent 90a3708
commit d55dbd1
Show file tree

Hide file tree

Showing 30 changed files with 164 additions and 162 deletions.
diff --git a/checks/fileparser/listing.go b/checks/fileparser/listing.go
@@ -17,6 +17,7 @@ package fileparser
 import (
 	"bufio"
 	"fmt"
+	"io"
 	"path"
 	"strings"
 
@@ -94,9 +95,14 @@ func OnMatchingFileContentDo(repoClient clients.RepoClient, matchPathTo PathMatc
 	}
 
 	for _, file := range matchedFiles {
-		content, err := repoClient.GetFileContent(file)
+		rc, err := repoClient.GetFileReader(file)
 		if err != nil {
-			return fmt.Errorf("error during GetFileContent: %w", err)
+			return fmt.Errorf("error during GetFileReader: %w", err)
+		}
+		content, err := io.ReadAll(rc)
+		rc.Close()
+		if err != nil {
+			return fmt.Errorf("reading from file: %w", err)
 		}
 
 		continueIter, err := onFileContent(file, content, args...)

diff --git a/checks/fileparser/listing_test.go b/checks/fileparser/listing_test.go
@@ -16,6 +16,8 @@ package fileparser
 
 import (
 	"errors"
+	"io"
+	"strings"
 	"testing"
 
 	"github.com/golang/mock/gomock"
@@ -526,7 +528,7 @@ func TestOnMatchingFileContent(t *testing.T) {
 			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()
+			mockRepo.EXPECT().GetFileReader(gomock.Any()).Return(io.NopCloser(strings.NewReader("")), nil).AnyTimes()
 
 			result := OnMatchingFileContentDo(mockRepo, PathMatcher{
 				Pattern:       tt.shellPattern,

diff --git a/checks/fuzzing_test.go b/checks/fuzzing_test.go
@@ -16,6 +16,8 @@ package checks
 
 import (
 	"errors"
+	"io"
+	"strings"
 	"testing"
 
 	"github.com/golang/mock/gomock"
@@ -144,11 +146,12 @@ func TestFuzzing(t *testing.T) {
 				}).AnyTimes()
 			mockFuzz.EXPECT().ListProgrammingLanguages().Return(tt.langs, nil).AnyTimes()
 			mockFuzz.EXPECT().ListFiles(gomock.Any()).Return(tt.fileName, nil).AnyTimes()
-			mockFuzz.EXPECT().GetFileContent(gomock.Any()).DoAndReturn(func(f string) (string, error) {
+			mockFuzz.EXPECT().GetFileReader(gomock.Any()).DoAndReturn(func(f string) (io.ReadCloser, error) {
 				if tt.wantErr {
-					return "", errors.New("error")
+					return nil, errors.New("error")
 				}
-				return tt.fileContent, nil
+				rc := io.NopCloser(strings.NewReader(tt.fileContent))
+				return rc, nil
 			}).AnyTimes()
 			dl := scut.TestDetailLogger{}
 			raw := checker.RawResults{}

diff --git a/checks/permissions_test.go b/checks/permissions_test.go
@@ -15,7 +15,7 @@
 package checks
 
 import (
-	"fmt"
+	"io"
 	"os"
 	"strings"
 	"testing"
@@ -443,12 +443,8 @@ func TestGithubTokenPermissions(t *testing.T) {
 				}
 				return files, nil
 			}).AnyTimes()
-			mockRepo.EXPECT().GetFileContent(gomock.Any()).DoAndReturn(func(fn string) ([]byte, error) {
-				content, err := os.ReadFile("./testdata/" + fn)
-				if err != nil {
-					return content, fmt.Errorf("%w", err)
-				}
-				return content, nil
+			mockRepo.EXPECT().GetFileReader(gomock.Any()).DoAndReturn(func(fn string) (io.ReadCloser, error) {
+				return os.Open("./testdata/" + fn)
 			}).AnyTimes()
 			dl := scut.TestDetailLogger{}
 			c := checker.CheckRequest{
@@ -499,11 +495,6 @@ func TestGithubTokenPermissionsLineNumber(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()
-			content, err := os.ReadFile(tt.filename)
-			if err != nil {
-				t.Errorf("cannot read file: %v", err)
-			}
-
 			p := strings.Replace(tt.filename, "./testdata/", "", 1)
 			ctrl := gomock.NewController(t)
 			mockRepo := mockrepo.NewMockRepoClient(ctrl)
@@ -514,8 +505,8 @@ func TestGithubTokenPermissionsLineNumber(t *testing.T) {
 			mockRepo.EXPECT().ListFiles(gomock.Any()).DoAndReturn(func(predicate func(string) (bool, error)) ([]string, error) {
 				return []string{p}, nil
 			}).AnyTimes()
-			mockRepo.EXPECT().GetFileContent(gomock.Any()).DoAndReturn(func(fn string) ([]byte, error) {
-				return content, nil
+			mockRepo.EXPECT().GetFileReader(gomock.Any()).DoAndReturn(func(fn string) (io.ReadCloser, error) {
+				return os.Open(tt.filename)
 			}).AnyTimes()
 			dl := scut.TestDetailLogger{}
 			c := checker.CheckRequest{

diff --git a/checks/pinned_dependencies_test.go b/checks/pinned_dependencies_test.go
@@ -15,7 +15,7 @@
 package checks
 
 import (
-	"fmt"
+	"io"
 	"os"
 	"testing"
 
@@ -58,15 +58,11 @@ func TestPinningDependencies(t *testing.T) {
 			mockRepo.EXPECT().URI().Return("github.com/ossf/scorecard").AnyTimes()
 			mockRepo.EXPECT().ListFiles(gomock.Any()).Return(tt.files, nil).AnyTimes()
 
-			mockRepo.EXPECT().GetFileContent(gomock.Any()).DoAndReturn(func(fn string) ([]byte, error) {
+			mockRepo.EXPECT().GetFileReader(gomock.Any()).DoAndReturn(func(fn string) (io.ReadCloser, error) {
 				if tt.path == "" {
 					return nil, nil
 				}
-				content, err := os.ReadFile(tt.path)
-				if err != nil {
-					return content, fmt.Errorf("%w", err)
-				}
-				return content, nil
+				return os.Open(tt.path)
 			}).AnyTimes()
 
 			dl := scut.TestDetailLogger{}

diff --git a/checks/raw/binary_artifact_test.go b/checks/raw/binary_artifact_test.go
@@ -15,7 +15,7 @@
 package raw
 
 import (
-	"fmt"
+	"io"
 	"os"
 	"testing"
 
@@ -227,13 +227,8 @@ func TestBinaryArtifacts(t *testing.T) {
 				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
+				mockRepoClient.EXPECT().GetFileReader(gomock.Any()).DoAndReturn(func(file string) (io.ReadCloser, error) {
+					return os.Open(file)
 				})
 			}
 			if tt.successfulWorkflowRuns != nil {
@@ -276,19 +271,11 @@ func TestBinaryArtifacts_workflow_runs_unsupported(t *testing.T) {
 	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
+	mockRepoClient.EXPECT().GetFileReader(jarFile).DoAndReturn(func(file string) (io.ReadCloser, error) {
+		return os.Open("../testdata/binaryartifacts/jars/gradle-wrapper.jar")
 	}).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
+	mockRepoClient.EXPECT().GetFileReader(verifyWorkflow).DoAndReturn(func(file string) (io.ReadCloser, error) {
+		return os.Open("../testdata/binaryartifacts/workflows/verify.yaml")
 	}).AnyTimes()
 
 	mockRepoClient.EXPECT().ListSuccessfulWorkflowRuns(gomock.Any()).Return(nil, clients.ErrUnsupportedFeature).AnyTimes()

diff --git a/checks/raw/dangerous_workflow_test.go b/checks/raw/dangerous_workflow_test.go
@@ -17,7 +17,7 @@ package raw
 import (
 	"context"
 	"errors"
-	"fmt"
+	"io"
 	"os"
 	"testing"
 
@@ -159,13 +159,8 @@ func TestGithubDangerousWorkflow(t *testing.T) {
 			ctrl := gomock.NewController(t)
 			mockRepoClient := mockrepo.NewMockRepoClient(ctrl)
 			mockRepoClient.EXPECT().ListFiles(gomock.Any()).Return([]string{tt.filename}, 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("../testdata/" + file)
-				if err != nil {
-					return content, fmt.Errorf("%w", err)
-				}
-				return content, nil
+			mockRepoClient.EXPECT().GetFileReader(gomock.Any()).DoAndReturn(func(file string) (io.ReadCloser, error) {
+				return os.Open("../testdata/" + file)
 			})
 
 			req := &checker.CheckRequest{

diff --git a/checks/raw/fuzzing_test.go b/checks/raw/fuzzing_test.go
@@ -16,8 +16,10 @@ package raw
 
 import (
 	"errors"
+	"io"
 	"path"
 	"regexp"
+	"strings"
 	"testing"
 
 	"github.com/golang/mock/gomock"
@@ -135,11 +137,11 @@ func Test_checkCFLite(t *testing.T) {
 			defer ctrl.Finish()
 			mockFuzz := mockrepo.NewMockRepoClient(ctrl)
 			mockFuzz.EXPECT().ListFiles(gomock.Any()).Return(tt.fileName, nil).AnyTimes()
-			mockFuzz.EXPECT().GetFileContent(gomock.Any()).DoAndReturn(func(f string) (string, error) {
+			mockFuzz.EXPECT().GetFileReader(gomock.Any()).DoAndReturn(func(f string) (io.ReadCloser, error) {
 				if tt.wantErr {
-					return "", errors.New("error")
+					return nil, errors.New("error")
 				}
-				return tt.fileContent, nil
+				return io.NopCloser(strings.NewReader(tt.fileContent)), nil
 			}).AnyTimes()
 			req := checker.CheckRequest{
 				RepoClient: mockFuzz,
@@ -486,11 +488,11 @@ func Test_checkFuzzFunc(t *testing.T) {
 			defer ctrl.Finish()
 			mockClient := mockrepo.NewMockRepoClient(ctrl)
 			mockClient.EXPECT().ListFiles(gomock.Any()).Return(tt.fileName, nil).AnyTimes()
-			mockClient.EXPECT().GetFileContent(gomock.Any()).DoAndReturn(func(f string) ([]byte, error) {
+			mockClient.EXPECT().GetFileReader(gomock.Any()).DoAndReturn(func(f string) (io.ReadCloser, error) {
 				if tt.wantErr {
 					return nil, errors.New("error")
 				}
-				return []byte(tt.fileContent), nil
+				return io.NopCloser(strings.NewReader(tt.fileContent)), nil
 			}).AnyTimes()
 			req := checker.CheckRequest{
 				RepoClient: mockClient,

diff --git a/checks/raw/github/packaging.go b/checks/raw/github/packaging.go
@@ -16,6 +16,7 @@ package github
 
 import (
 	"fmt"
+	"io"
 	"path/filepath"
 
 	"github.com/rhysd/actionlint"
@@ -37,9 +38,14 @@ func Packaging(c *checker.CheckRequest) (checker.PackagingData, error) {
 	}
 
 	for _, fp := range matchedFiles {
-		fc, err := c.RepoClient.GetFileContent(fp)
+		fr, err := c.RepoClient.GetFileReader(fp)
 		if err != nil {
-			return data, fmt.Errorf("RepoClient.GetFileContent: %w", err)
+			return data, fmt.Errorf("RepoClient.GetFileReader: %w", err)
+		}
+		fc, err := io.ReadAll(fr)
+		fr.Close()
+		if err != nil {
+			return data, fmt.Errorf("reading file: %w", err)
 		}
 
 		workflow, errs := actionlint.Parse(fc)

diff --git a/checks/raw/gitlab/packaging.go b/checks/raw/gitlab/packaging.go
@@ -16,6 +16,7 @@ package gitlab
 
 import (
 	"fmt"
+	"io"
 	"strings"
 
 	"github.com/ossf/scorecard/v4/checker"
@@ -32,9 +33,14 @@ func Packaging(c *checker.CheckRequest) (checker.PackagingData, error) {
 	}
 
 	for _, fp := range matchedFiles {
-		fc, err := c.RepoClient.GetFileContent(fp)
+		fr, err := c.RepoClient.GetFileReader(fp)
 		if err != nil {
-			return data, fmt.Errorf("RepoClient.GetFileContent: %w", err)
+			return data, fmt.Errorf("RepoClient.GetFileReader: %w", err)
+		}
+		fc, err := io.ReadAll(fr)
+		fr.Close()
+		if err != nil {
+			return data, fmt.Errorf("reading from file: %w", err)
 		}
 
 		file, found := isGitlabPackagingWorkflow(fc, fp)

diff --git a/checks/raw/gitlab/packaging_test.go b/checks/raw/gitlab/packaging_test.go
@@ -15,6 +15,7 @@
 package gitlab
 
 import (
+	"io"
 	"os"
 	"testing"
 
@@ -134,10 +135,9 @@ func TestGitlabPackagingPackager(t *testing.T) {
 			moqRepoClient.EXPECT().ListFiles(gomock.Any()).
 				Return([]string{tt.filename}, nil).AnyTimes()
 
-			moqRepoClient.EXPECT().GetFileContent(tt.filename).
-				DoAndReturn(func(b string) ([]byte, error) {
-					content, err := os.ReadFile(b)
-					return content, err
+			moqRepoClient.EXPECT().GetFileReader(tt.filename).
+				DoAndReturn(func(b string) (io.ReadCloser, error) {
+					return os.Open(b)
 				}).AnyTimes()
 
 			if tt.exists {

diff --git a/checks/raw/pinned_dependencies_test.go b/checks/raw/pinned_dependencies_test.go
@@ -15,7 +15,7 @@
 package raw
 
 import (
-	"fmt"
+	"io"
 	"os"
 	"path/filepath"
 	"strings"
@@ -1895,13 +1895,8 @@ func TestCollectDockerfilePinning(t *testing.T) {
 			mockRepoClient.EXPECT().ListFiles(gomock.Any()).Return([]string{tt.filename}, nil).AnyTimes()
 			mockRepoClient.EXPECT().GetDefaultBranchName().Return("main", nil).AnyTimes()
 			mockRepoClient.EXPECT().URI().Return("github.com/ossf/scorecard").AnyTimes()
-			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
+			mockRepoClient.EXPECT().GetFileReader(gomock.Any()).DoAndReturn(func(file string) (io.ReadCloser, error) {
+				return os.Open(file)
 			})
 
 			req := checker.CheckRequest{
@@ -1994,13 +1989,8 @@ func TestCollectGitHubActionsWorkflowPinning(t *testing.T) {
 			mockRepoClient.EXPECT().ListFiles(gomock.Any()).Return([]string{tt.filename}, nil).AnyTimes()
 			mockRepoClient.EXPECT().GetDefaultBranchName().Return("main", nil).AnyTimes()
 			mockRepoClient.EXPECT().URI().Return("github.com/ossf/scorecard").AnyTimes()
-			mockRepoClient.EXPECT().GetFileContent(gomock.Any()).DoAndReturn(func(file string) ([]byte, error) {
-				// This will read the file and return the content
-				content, err := os.ReadFile(filepath.Join("testdata", file))
-				if err != nil {
-					return content, fmt.Errorf("%w", err)
-				}
-				return content, nil
+			mockRepoClient.EXPECT().GetFileReader(gomock.Any()).DoAndReturn(func(file string) (io.ReadCloser, error) {
+				return os.Open(filepath.Join("testdata", file))
 			})
 
 			req := checker.CheckRequest{

diff --git a/checks/raw/sast_test.go b/checks/raw/sast_test.go
@@ -15,7 +15,7 @@
 package raw
 
 import (
-	"fmt"
+	"io"
 	"os"
 	"testing"
 
@@ -207,13 +207,8 @@ func TestSAST(t *testing.T) {
 			mockRepoClient.EXPECT().ListCommits().DoAndReturn(func() ([]clients.Commit, error) {
 				return tt.commits, 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("./testdata/" + file)
-				if err != nil {
-					return content, fmt.Errorf("%w", err)
-				}
-				return content, nil
+			mockRepoClient.EXPECT().GetFileReader(gomock.Any()).DoAndReturn(func(file string) (io.ReadCloser, error) {
+				return os.Open("./testdata/" + file)
 			}).AnyTimes()
 			req := checker.CheckRequest{
 				RepoClient: mockRepoClient,