Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 7 commits into from
Mar 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions checks/fileparser/listing.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import (
"bufio"
"fmt"
"io"
"path"
"strings"

Expand Down Expand Up @@ -94,9 +95,14 @@
}

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)

Check warning on line 105 in checks/fileparser/listing.go

View check run for this annotation

Codecov / codecov/patch

checks/fileparser/listing.go#L105

Added line #L105 was not covered by tests
}

continueIter, err := onFileContent(file, content, args...)
Expand Down
4 changes: 3 additions & 1 deletion checks/fileparser/listing_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ package fileparser

import (
"errors"
"io"
"strings"
"testing"

"github.com/golang/mock/gomock"
Expand Down Expand Up @@ -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,
Expand Down
9 changes: 6 additions & 3 deletions checks/fuzzing_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ package checks

import (
"errors"
"io"
"strings"
"testing"

"github.com/golang/mock/gomock"
Expand Down Expand Up @@ -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{}
Expand Down
19 changes: 5 additions & 14 deletions checks/permissions_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
package checks

import (
"fmt"
"io"
"os"
"strings"
"testing"
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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)
Expand All @@ -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{
Expand Down
10 changes: 3 additions & 7 deletions checks/pinned_dependencies_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
package checks

import (
"fmt"
"io"
"os"
"testing"

Expand Down Expand Up @@ -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{}
Expand Down
27 changes: 7 additions & 20 deletions checks/raw/binary_artifact_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
package raw

import (
"fmt"
"io"
"os"
"testing"

Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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()
Expand Down
11 changes: 3 additions & 8 deletions checks/raw/dangerous_workflow_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ package raw
import (
"context"
"errors"
"fmt"
"io"
"os"
"testing"

Expand Down Expand Up @@ -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{
Expand Down
12 changes: 7 additions & 5 deletions checks/raw/fuzzing_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,10 @@ package raw

import (
"errors"
"io"
"path"
"regexp"
"strings"
"testing"

"github.com/golang/mock/gomock"
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
10 changes: 8 additions & 2 deletions checks/raw/github/packaging.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

import (
"fmt"
"io"
"path/filepath"

"github.com/rhysd/actionlint"
Expand All @@ -37,9 +38,14 @@
}

for _, fp := range matchedFiles {
fc, err := c.RepoClient.GetFileContent(fp)
fr, err := c.RepoClient.GetFileReader(fp)

Check warning on line 41 in checks/raw/github/packaging.go

View check run for this annotation

Codecov / codecov/patch

checks/raw/github/packaging.go#L41

Added line #L41 was not covered by tests
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)

Check warning on line 48 in checks/raw/github/packaging.go

View check run for this annotation

Codecov / codecov/patch

checks/raw/github/packaging.go#L43-L48

Added lines #L43 - L48 were not covered by tests
}

workflow, errs := actionlint.Parse(fc)
Expand Down
10 changes: 8 additions & 2 deletions checks/raw/gitlab/packaging.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

import (
"fmt"
"io"
"strings"

"github.com/ossf/scorecard/v4/checker"
Expand All @@ -32,9 +33,14 @@
}

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)
}

Check warning on line 39 in checks/raw/gitlab/packaging.go

View check run for this annotation

Codecov / codecov/patch

checks/raw/gitlab/packaging.go#L38-L39

Added lines #L38 - L39 were not covered by tests
fc, err := io.ReadAll(fr)
fr.Close()
if err != nil {
return data, fmt.Errorf("reading from file: %w", err)

Check warning on line 43 in checks/raw/gitlab/packaging.go

View check run for this annotation

Codecov / codecov/patch

checks/raw/gitlab/packaging.go#L43

Added line #L43 was not covered by tests
}

file, found := isGitlabPackagingWorkflow(fc, fp)
Expand Down
8 changes: 4 additions & 4 deletions checks/raw/gitlab/packaging_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
package gitlab

import (
"io"
"os"
"testing"

Expand Down Expand Up @@ -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 {
Expand Down
20 changes: 5 additions & 15 deletions checks/raw/pinned_dependencies_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
package raw

import (
"fmt"
"io"
"os"
"path/filepath"
"strings"
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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{
Expand Down
11 changes: 3 additions & 8 deletions checks/raw/sast_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
package raw

import (
"fmt"
"io"
"os"
"testing"

Expand Down Expand Up @@ -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,
Expand Down
Loading
Loading