From 6dcfde9299033d3b16d81985a9634092411e0cfc Mon Sep 17 00:00:00 2001 From: Spencer Schrock Date: Wed, 17 Aug 2022 14:10:49 -0700 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20Fix=20remediation=20text=20when?= =?UTF-8?q?=20Scorecard=20is=20run=20multiple=20times=20within=20a=20progr?= =?UTF-8?q?am=20(#2168)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * quick fix for wrong info in remediation text * add test for old, incorrect behavior * Rename Setup to New --- checks/evaluation/permissions.go | 11 +-- checks/evaluation/pinned_dependencies.go | 11 +-- checks/evaluation/pinned_dependencies_test.go | 3 +- checks/permissions.go | 8 +-- checks/permissions_test.go | 1 + checks/pinned_dependencies.go | 8 +-- remediation/remediations.go | 71 ++++++++----------- remediation/remediations_test.go | 51 +++++++++++++ 8 files changed, 100 insertions(+), 64 deletions(-) create mode 100644 remediation/remediations_test.go diff --git a/checks/evaluation/permissions.go b/checks/evaluation/permissions.go index 776a8430..d7146e81 100644 --- a/checks/evaluation/permissions.go +++ b/checks/evaluation/permissions.go @@ -28,13 +28,13 @@ type permissions struct { } // TokenPermissions applies the score policy for the Token-Permissions check. -func TokenPermissions(name string, dl checker.DetailLogger, r *checker.TokenPermissionsData) checker.CheckResult { +func TokenPermissions(name string, c *checker.CheckRequest, r *checker.TokenPermissionsData) checker.CheckResult { if r == nil { e := sce.WithMessage(sce.ErrScorecardInternal, "empty raw data") return checker.CreateRuntimeErrorResult(name, e) } - score, err := applyScorePolicy(r, dl) + score, err := applyScorePolicy(r, c) if err != nil { return checker.CreateRuntimeErrorResult(name, err) } @@ -48,12 +48,15 @@ func TokenPermissions(name string, dl checker.DetailLogger, r *checker.TokenPerm "tokens are read-only in GitHub workflows") } -func applyScorePolicy(results *checker.TokenPermissionsData, dl checker.DetailLogger) (int, error) { +func applyScorePolicy(results *checker.TokenPermissionsData, c *checker.CheckRequest) (int, error) { // See list https://github.blog/changelog/2021-04-20-github-actions-control-permissions-for-github_token/. // Note: there are legitimate reasons to use some of the permissions like checks, deployments, etc. // in CI/CD systems https://docs.travis-ci.com/user/github-oauth-scopes/. hm := make(map[string]permissions) + dl := c.Dlogger + //nolint:errcheck + remediaitonMetadata, _ := remediation.New(c) for _, r := range results.TokenPermissions { var msg checker.LogMessage @@ -65,7 +68,7 @@ func applyScorePolicy(results *checker.TokenPermissionsData, dl checker.DetailLo msg.Snippet = r.File.Snippet if msg.Path != "" { - msg.Remediation = remediation.CreateWorkflowPermissionRemediation(r.File.Path) + msg.Remediation = remediaitonMetadata.CreateWorkflowPermissionRemediation(r.File.Path) } } diff --git a/checks/evaluation/pinned_dependencies.go b/checks/evaluation/pinned_dependencies.go index d68c8621..89786ff3 100644 --- a/checks/evaluation/pinned_dependencies.go +++ b/checks/evaluation/pinned_dependencies.go @@ -42,7 +42,7 @@ type worklowPinningResult struct { } // PinningDependencies applies the score policy for the Pinned-Dependencies check. -func PinningDependencies(name string, dl checker.DetailLogger, +func PinningDependencies(name string, c *checker.CheckRequest, r *checker.PinningDependenciesData, ) checker.CheckResult { if r == nil { @@ -52,6 +52,9 @@ func PinningDependencies(name string, dl checker.DetailLogger, var wp worklowPinningResult pr := make(map[checker.DependencyUseType]pinnedResult) + dl := c.Dlogger + //nolint:errcheck + remediaitonMetadata, _ := remediation.New(c) for i := range r.Dependencies { rr := r.Dependencies[i] @@ -83,7 +86,7 @@ func PinningDependencies(name string, dl checker.DetailLogger, EndOffset: rr.Location.EndOffset, Text: generateText(&rr), Snippet: rr.Location.Snippet, - Remediation: generateRemediation(&rr), + Remediation: generateRemediation(remediaitonMetadata, &rr), }) // Update the pinning status. @@ -133,10 +136,10 @@ func PinningDependencies(name string, dl checker.DetailLogger, "dependency not pinned by hash detected", score, checker.MaxResultScore) } -func generateRemediation(rr *checker.Dependency) *checker.Remediation { +func generateRemediation(remediaitonMd remediation.RemediationMetadata, rr *checker.Dependency) *checker.Remediation { switch rr.Type { case checker.DependencyUseTypeGHAction: - return remediation.CreateWorkflowPinningRemediation(rr.Location.Path) + return remediaitonMd.CreateWorkflowPinningRemediation(rr.Location.Path) case checker.DependencyUseTypeDockerfileContainerImage: return remediation.CreateDockerfilePinningRemediation(rr.Name) default: diff --git a/checks/evaluation/pinned_dependencies_test.go b/checks/evaluation/pinned_dependencies_test.go index 387821fc..fa0640f8 100644 --- a/checks/evaluation/pinned_dependencies_test.go +++ b/checks/evaluation/pinned_dependencies_test.go @@ -170,7 +170,8 @@ func Test_PinningDependencies(t *testing.T) { t.Parallel() dl := scut.TestDetailLogger{} - actual := PinningDependencies("checkname", &dl, + c := checker.CheckRequest{Dlogger: &dl} + actual := PinningDependencies("checkname", &c, &checker.PinningDependenciesData{ Dependencies: tt.dependencies, }) diff --git a/checks/permissions.go b/checks/permissions.go index 44164481..0dbcbb4d 100644 --- a/checks/permissions.go +++ b/checks/permissions.go @@ -19,7 +19,6 @@ import ( "github.com/ossf/scorecard/v4/checks/evaluation" "github.com/ossf/scorecard/v4/checks/raw" sce "github.com/ossf/scorecard/v4/errors" - "github.com/ossf/scorecard/v4/remediation" ) // CheckTokenPermissions is the exported name for Token-Permissions check. @@ -39,11 +38,6 @@ func init() { // TokenPermissions will run the Token-Permissions check. func TokenPermissions(c *checker.CheckRequest) checker.CheckResult { - if err := remediation.Setup(c); err != nil { - e := sce.WithMessage(sce.ErrScorecardInternal, err.Error()) - return checker.CreateRuntimeErrorResult(CheckTokenPermissions, e) - } - rawData, err := raw.TokenPermissions(c) if err != nil { e := sce.WithMessage(sce.ErrScorecardInternal, err.Error()) @@ -56,5 +50,5 @@ func TokenPermissions(c *checker.CheckRequest) checker.CheckResult { } // Return the score evaluation. - return evaluation.TokenPermissions(CheckTokenPermissions, c.Dlogger, &rawData) + return evaluation.TokenPermissions(CheckTokenPermissions, c, &rawData) } diff --git a/checks/permissions_test.go b/checks/permissions_test.go index 2dd89f0d..f0e0ea30 100644 --- a/checks/permissions_test.go +++ b/checks/permissions_test.go @@ -364,6 +364,7 @@ func TestGithubTokenPermissions(t *testing.T) { ctrl := gomock.NewController(t) mockRepo := mockrepo.NewMockRepoClient(ctrl) + mockRepo.EXPECT().GetDefaultBranchName().Return("main", nil) main := "main" mockRepo.EXPECT().URI().Return("github.com/ossf/scorecard").AnyTimes() diff --git a/checks/pinned_dependencies.go b/checks/pinned_dependencies.go index 2e064e71..1720a2fb 100644 --- a/checks/pinned_dependencies.go +++ b/checks/pinned_dependencies.go @@ -19,7 +19,6 @@ import ( "github.com/ossf/scorecard/v4/checks/evaluation" "github.com/ossf/scorecard/v4/checks/raw" sce "github.com/ossf/scorecard/v4/errors" - "github.com/ossf/scorecard/v4/remediation" ) // CheckPinnedDependencies is the registered name for FrozenDeps. @@ -39,11 +38,6 @@ func init() { // PinningDependencies will check the repository for its use of dependencies. func PinningDependencies(c *checker.CheckRequest) checker.CheckResult { - if err := remediation.Setup(c); err != nil { - e := sce.WithMessage(sce.ErrScorecardInternal, err.Error()) - return checker.CreateRuntimeErrorResult(CheckPinnedDependencies, e) - } - rawData, err := raw.PinningDependencies(c) if err != nil { e := sce.WithMessage(sce.ErrScorecardInternal, err.Error()) @@ -55,5 +49,5 @@ func PinningDependencies(c *checker.CheckRequest) checker.CheckResult { c.RawResults.PinningDependenciesResults = rawData } - return evaluation.PinningDependencies(CheckPinnedDependencies, c.Dlogger, &rawData) + return evaluation.PinningDependencies(CheckPinnedDependencies, c, &rawData) } diff --git a/remediation/remediations.go b/remediation/remediations.go index 00015649..1a81100d 100644 --- a/remediation/remediations.go +++ b/remediation/remediations.go @@ -18,19 +18,10 @@ import ( "errors" "fmt" "strings" - "sync" "github.com/google/go-containerregistry/pkg/crane" "github.com/ossf/scorecard/v4/checker" - "github.com/ossf/scorecard/v4/clients" -) - -var ( - branch string - repo string - once *sync.Once - setupErr error ) var errInvalidArg = errors.New("invalid argument") @@ -42,53 +33,51 @@ var ( dockerfilePinText = "pin your Docker image by updating %[1]s to %[1]s@%s" ) -//nolint:gochecknoinits -func init() { - once = new(sync.Once) +// TODO fix how this info makes it checks/evaluation. +type RemediationMetadata struct { + branch string + repo string } -// Setup sets up remediation code. -func Setup(c *checker.CheckRequest) error { - once.Do(func() { - // Get the branch for remediation. - b, err := c.RepoClient.GetDefaultBranchName() - if err != nil { - if !errors.Is(err, clients.ErrUnsupportedFeature) { - setupErr = err - } - return - } +// New returns remediation relevant metadata from a CheckRequest. +func New(c *checker.CheckRequest) (RemediationMetadata, error) { + if c == nil || c.RepoClient == nil { + return RemediationMetadata{}, nil + } - branch = b - uri := c.RepoClient.URI() - parts := strings.Split(uri, "/") - if len(parts) != 3 { - setupErr = fmt.Errorf("%w: empty: %s", errInvalidArg, uri) - return - } - repo = fmt.Sprintf("%s/%s", parts[1], parts[2]) - }) - return setupErr + // Get the branch for remediation. + branch, err := c.RepoClient.GetDefaultBranchName() + if err != nil { + return RemediationMetadata{}, fmt.Errorf("GetDefaultBranchName: %w", err) + } + + uri := c.RepoClient.URI() + parts := strings.Split(uri, "/") + if len(parts) != 3 { + return RemediationMetadata{}, fmt.Errorf("%w: empty: %s", errInvalidArg, uri) + } + repo := fmt.Sprintf("%s/%s", parts[1], parts[2]) + return RemediationMetadata{branch: branch, repo: repo}, nil } // CreateWorkflowPermissionRemediation create remediation for workflow permissions. -func CreateWorkflowPermissionRemediation(filepath string) *checker.Remediation { - return createWorkflowRemediation(filepath, "permissions") +func (r *RemediationMetadata) CreateWorkflowPermissionRemediation(filepath string) *checker.Remediation { + return r.createWorkflowRemediation(filepath, "permissions") } // CreateWorkflowPinningRemediation create remediaiton for pinninn GH Actions. -func CreateWorkflowPinningRemediation(filepath string) *checker.Remediation { - return createWorkflowRemediation(filepath, "pin") +func (r *RemediationMetadata) CreateWorkflowPinningRemediation(filepath string) *checker.Remediation { + return r.createWorkflowRemediation(filepath, "pin") } -func createWorkflowRemediation(path, t string) *checker.Remediation { +func (r *RemediationMetadata) createWorkflowRemediation(path, t string) *checker.Remediation { p := strings.TrimPrefix(path, ".github/workflows/") - if branch == "" || repo == "" { + if r.branch == "" || r.repo == "" { return nil } - text := fmt.Sprintf(workflowText, repo, p, branch, t) - markdown := fmt.Sprintf(workflowMarkdown, repo, p, branch, t) + text := fmt.Sprintf(workflowText, r.repo, p, r.branch, t) + markdown := fmt.Sprintf(workflowMarkdown, r.repo, p, r.branch, t) return &checker.Remediation{ HelpText: text, diff --git a/remediation/remediations_test.go b/remediation/remediations_test.go new file mode 100644 index 00000000..efdddf47 --- /dev/null +++ b/remediation/remediations_test.go @@ -0,0 +1,51 @@ +// Copyright 2022 Security Scorecard Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package remediation + +import ( + "fmt" + "testing" + + "github.com/golang/mock/gomock" + + "github.com/ossf/scorecard/v4/checker" + mockrepo "github.com/ossf/scorecard/v4/clients/mockclients" +) + +func TestRepeatedSetup(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + + for i := 0; i < 2; i++ { + mockRepo := mockrepo.NewMockRepoClient(ctrl) + mockRepo.EXPECT().GetDefaultBranchName().Return("main", nil).AnyTimes() + uri := fmt.Sprintf("github.com/ossf/scorecard%d", i) + mockRepo.EXPECT().URI().Return(uri).AnyTimes() + + c := checker.CheckRequest{ + RepoClient: mockRepo, + } + rmd, err := New(&c) + if err != nil { + t.Error(err) + } + + want := fmt.Sprintf("ossf/scorecard%d", i) + if rmd.repo != want { + t.Errorf("failed. expected: %v, got: %v", want, rmd.repo) + } + } +}