🐛 Fix remediation text when Scorecard is run multiple times within a program (#2168)

* quick fix for wrong info in remediation text

* add test for old, incorrect  behavior

* Rename Setup to New

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

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:

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

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

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

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

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,

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