Skip to content

Commit

Permalink
Replace errors.As with Is (#901)
Browse files Browse the repository at this point in the history
Co-authored-by: Azeem Shaikh <azeems@google.com>
  • Loading branch information
azeemshaikh38 and azeemsgoogle committed Aug 25, 2021
1 parent 46a655d commit 41d0ce3
Show file tree
Hide file tree
Showing 7 changed files with 29 additions and 42 deletions.
2 changes: 1 addition & 1 deletion checks/security_policy.go
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ func SecurityPolicy(c *checker.CheckRequest) checker.CheckResult {
if r {
return checker.CreateMaxScoreResult(CheckSecurityPolicy, "security policy file detected")
}
case !errors.As(err, &errIgnore):
case !errors.Is(err, errIgnore):
return checker.CreateRuntimeErrorResult(CheckSecurityPolicy, err)
}
return checker.CreateMinScoreResult(CheckSecurityPolicy, "security policy file not detected")
Expand Down
58 changes: 23 additions & 35 deletions clients/githubrepo/tarball.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,7 @@ func extractAndValidateArchivePath(path, dest string) (string, error) {
// Check for ZipSlip: https://snyk.io/research/zip-slip-vulnerability
cleanpath := filepath.Join(dest, names[1])
if !strings.HasPrefix(cleanpath, filepath.Clean(dest)+string(os.PathSeparator)) {
//nolint:wrapcheck
return "", sce.Create(sce.ErrScorecardInternal, fmt.Sprintf("%v: %v", errZipSlip, names[1]))
return "", fmt.Errorf("%w: %s", errZipSlip, names[1])
}
return cleanpath, nil
}
Expand All @@ -73,24 +72,27 @@ type tarballHandler struct {
func (handler *tarballHandler) init(ctx context.Context, repo *github.Repository) error {
// Cleanup any previous state.
if err := handler.cleanup(); err != nil {
return fmt.Errorf("error during githubrepo cleanup: %w", err)
// nolint: wrapcheck
return sce.Create(sce.ErrScorecardInternal, err.Error())
}

// Setup temp dir/files and download repo tarball.
if err := handler.getTarball(ctx, repo); errors.As(err, &errTarballNotFound) {
if err := handler.getTarball(ctx, repo); errors.Is(err, errTarballNotFound) {
log.Printf("unable to get tarball %v. Skipping...", err)
return nil
} else if err != nil {
return err
// nolint: wrapcheck
return sce.Create(sce.ErrScorecardInternal, err.Error())
}

// Extract file names and content from tarball.
err := handler.extractTarball()
if errors.As(err, &errTarballCorrupted) {
if errors.Is(err, errTarballCorrupted) {
log.Printf("unable to extract tarball %v. Skipping...", err)
return nil
}
return err
// nolint: wrapcheck
return sce.Create(sce.ErrScorecardInternal, err.Error())
}

func (handler *tarballHandler) getTarball(ctx context.Context, repo *github.Repository) error {
Expand All @@ -99,39 +101,33 @@ func (handler *tarballHandler) getTarball(ctx context.Context, repo *github.Repo
url = strings.Replace(url, "{/ref}", "", 1)
req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil)
if err != nil {
//nolint:wrapcheck
return sce.Create(sce.ErrScorecardInternal, fmt.Sprintf("http.NewRequestWithContext: %v", err))
return fmt.Errorf("http.NewRequestWithContext: %w", err)
}
resp, err := http.DefaultClient.Do(req)
if err != nil {
//nolint:wrapcheck
return sce.Create(sce.ErrScorecardInternal, fmt.Sprintf("http.DefaultClient.Do: %v", err))
return fmt.Errorf("http.DefaultClient.Do: %w", err)
}
defer resp.Body.Close()

// Handle 400/404 errors
switch resp.StatusCode {
case http.StatusNotFound, http.StatusBadRequest:
//nolint:wrapcheck
return sce.CreateInternal(errTarballNotFound, fmt.Sprintf("%v: %v: %v", errTarballNotFound, *repo.URL, err))
return fmt.Errorf("%w: %s", errTarballNotFound, url)
}

// Create a temp file. This automatically appends a random number to the name.
tempDir, err := ioutil.TempDir("", repoDir)
if err != nil {
//nolint:wrapcheck
return sce.Create(sce.ErrScorecardInternal, fmt.Sprintf("ioutil.TempDir: %v", err))
return fmt.Errorf("ioutil.TempDir: %w", err)
}
repoFile, err := ioutil.TempFile(tempDir, repoFilename)
if err != nil {
//nolint:wrapcheck
return sce.Create(sce.ErrScorecardInternal, fmt.Sprintf("ioutil.TempFile: %v", err))
return fmt.Errorf("ioutil.TempFile: %w", err)
}
defer repoFile.Close()
if _, err := io.Copy(repoFile, resp.Body); err != nil {
// nolint: wrapcheck
// This can happen if the incoming tarball is corrupted/server gateway times out.
return sce.CreateInternal(errTarballNotFound, fmt.Sprintf("io.Copy: %v", err))
return fmt.Errorf("%w io.Copy: %v", errTarballNotFound, err)
}

handler.tempDir = tempDir
Expand All @@ -144,13 +140,11 @@ func (handler *tarballHandler) extractTarball() error {
// nolint: gomnd
in, err := os.OpenFile(handler.tempTarFile, os.O_RDONLY, 0o644)
if err != nil {
//nolint:wrapcheck
return sce.Create(sce.ErrScorecardInternal, fmt.Sprintf("os.OpenFile: %v", err))
return fmt.Errorf("os.OpenFile: %w", err)
}
gz, err := gzip.NewReader(in)
if err != nil {
//nolint:wrapcheck
return sce.CreateInternal(errTarballCorrupted, fmt.Sprintf("gzip.NewReader: %v: %v", handler.tempTarFile, err))
return fmt.Errorf("%w: gzip.NewReader %v %v", errTarballCorrupted, handler.tempTarFile, err)
}
tr := tar.NewReader(gz)
for {
Expand All @@ -159,8 +153,7 @@ func (handler *tarballHandler) extractTarball() error {
break
}
if err != nil {
//nolint:wrapcheck
return sce.CreateInternal(errTarballCorrupted, fmt.Sprintf("tarReader.Next: %v", err))
return fmt.Errorf("%w tarReader.Next: %v", errTarballCorrupted, err)
}

switch header.Typeflag {
Expand Down Expand Up @@ -188,22 +181,19 @@ func (handler *tarballHandler) extractTarball() error {
if _, err := os.Stat(filepath.Dir(filenamepath)); os.IsNotExist(err) {
// nolint: gomnd
if err := os.Mkdir(filepath.Dir(filenamepath), 0o755); err != nil {
//nolint:wrapcheck
return sce.Create(sce.ErrScorecardInternal, fmt.Sprintf("os.Mkdir: %v", err))
return fmt.Errorf("os.Mkdir: %w", err)
}
}
outFile, err := os.Create(filenamepath)
if err != nil {
//nolint:wrapcheck
return sce.Create(sce.ErrScorecardInternal, fmt.Sprintf("os.Create: %v", err))
return fmt.Errorf("os.Create: %w", err)
}

// nolint: gosec
// Potential for DoS vulnerability via decompression bomb.
// Since such an attack will only impact a single shard, ignoring this for now.
if _, err := io.Copy(outFile, tr); err != nil {
// nolint: wrapcheck
return sce.CreateInternal(errTarballCorrupted, fmt.Sprintf("io.Copy: %v", err))
return fmt.Errorf("%w io.Copy: %v", errTarballCorrupted, err)
}
outFile.Close()
handler.files = append(handler.files,
Expand Down Expand Up @@ -235,16 +225,14 @@ func (handler *tarballHandler) listFiles(predicate func(string) (bool, error)) (
func (handler *tarballHandler) getFileContent(filename string) ([]byte, error) {
content, err := ioutil.ReadFile(filepath.Join(handler.tempDir, filename))
if err != nil {
//nolint:wrapcheck
return content, sce.Create(sce.ErrScorecardInternal, fmt.Sprintf("ioutil.ReadFile: %v", err))
return content, fmt.Errorf("ioutil.ReadFile: %w", err)
}
return content, nil
}

func (handler *tarballHandler) cleanup() error {
if err := os.RemoveAll(handler.tempDir); err != nil && !os.IsNotExist(err) {
//nolint:wrapcheck
return sce.Create(sce.ErrScorecardInternal, fmt.Sprintf("os.Remove: %v", err))
return fmt.Errorf("os.Remove: %w", err)
}
// Remove old files so we don't iterate through them.
handler.files = nil
Expand Down
2 changes: 1 addition & 1 deletion clients/githubrepo/tarball_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ func TestExtractTarball(t *testing.T) {
// Test GetFileContent API.
for _, getcontenttest := range testcase.getcontentTests {
content, err := handler.getFileContent(getcontenttest.filename)
if getcontenttest.err != nil && !errors.As(err, &getcontenttest.err) {
if getcontenttest.err != nil && !errors.Is(err, getcontenttest.err) {
t.Errorf("test failed: expected - %v, got - %v", getcontenttest.err, err)
}
if getcontenttest.err == nil && !cmp.Equal(getcontenttest.output, content) {
Expand Down
1 change: 0 additions & 1 deletion cmd/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,6 @@ or ./scorecard --{npm,pypi,rubgems}=<package_name> [--checks=check1,...] [--show
githubClient := github.NewClient(httpClient)
graphClient := githubv4.NewClient(httpClient)
repoClient := githubrepo.CreateGithubRepoClient(ctx, githubClient, graphClient)
defer repoClient.Close()

repoResult, err := pkg.RunScorecards(ctx, repo, enabledChecks, repoClient, httpClient, githubClient, graphClient)
if err != nil {
Expand Down
2 changes: 1 addition & 1 deletion cron/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ func GetShardSize() (int, error) {
// GetWebhookURL returns the webhook URL to ping on a successful cron job completion.
func GetWebhookURL() (string, error) {
url, err := getStringConfigValue(webhookURL, configYAML, "WebhookURL", "webhook-url")
if err != nil && !errors.As(err, &ErrorEmptyConfigValue) {
if err != nil && !errors.Is(err, ErrorEmptyConfigValue) {
return url, err
}
return url, nil
Expand Down
5 changes: 2 additions & 3 deletions cron/worker/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ func processRequest(ctx context.Context,
for _, repoURL := range repoURLs {
log.Printf("Running Scorecard for repo: %s", repoURL.URL())
result, err := pkg.RunScorecards(ctx, repoURL, checksToRun, repoClient, httpClient, githubClient, graphClient)
if errors.As(err, &errIgnore) {
if errors.Is(err, errIgnore) {
// Not accessible repo - continue.
continue
}
Expand All @@ -104,7 +104,7 @@ func processRequest(ctx context.Context,
}
for checkIndex := range result.Checks {
check := &result.Checks[checkIndex]
if !errors.As(check.Error2, &sce.ErrScorecardInternal) {
if !errors.Is(check.Error2, sce.ErrScorecardInternal) {
continue
}
errorMsg := fmt.Sprintf("check %s has a runtime error: %v", check.Name, check.Error2)
Expand Down Expand Up @@ -203,7 +203,6 @@ func main() {
}

repoClient, httpClient, githubClient, graphClient, logger := createNetClients(ctx)
defer repoClient.Close()

exporter, err := startMetricsExporter()
if err != nil {
Expand Down
1 change: 1 addition & 0 deletions pkg/scorecard.go
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ func RunScorecards(ctx context.Context,
//nolint:wrapcheck
return ScorecardResult{}, err
}
defer repoClient.Close()

commits, err := repoClient.ListCommits()
if err != nil {
Expand Down

0 comments on commit 41d0ce3

Please sign in to comment.