🌱 maintainer annotations: improve annotation file validation (ossf#4162)

* validate check names against full list Signed-off-by: Raghav Kaul <[email protected]> * tests: close file Signed-off-by: Raghav Kaul <[email protected]> * update Signed-off-by: Raghav Kaul <[email protected]> * make private Signed-off-by: Raghav Kaul <[email protected]> * Restructure imports Signed-off-by: Raghav Kaul <[email protected]> * update Signed-off-by: Raghav Kaul <[email protected]> --------- Signed-off-by: Raghav Kaul <[email protected]>
ware · Jul 2, 2024 · 28337f1 · 28337f1
1 parent 9f9afa0
commit 28337f1
Show file tree

Hide file tree

Showing 51 changed files with 182 additions and 110 deletions.
diff --git a/config/config.go b/config/config.go
@@ -23,6 +23,7 @@ import (
 	"gopkg.in/yaml.v3"
 
 	sce "github.com/ossf/scorecard/v5/errors"
+	"github.com/ossf/scorecard/v5/internal/checknames"
 )
 
 var (
@@ -45,19 +46,19 @@ func parseFile(c *Config, content []byte) error {
 	return nil
 }
 
-func isValidCheck(check string, checks []string) bool {
-	for _, validCheck := range checks {
-		if strings.EqualFold(check, validCheck) {
+func isValidCheck(check string) bool {
+	for _, c := range checknames.AllValidChecks {
+		if strings.EqualFold(c, check) {
 			return true
 		}
 	}
 	return false
 }
 
-func validate(c Config, checks []string) error {
+func validate(c Config) error {
 	for _, annotation := range c.Annotations {
 		for _, check := range annotation.Checks {
-			if !isValidCheck(check, checks) {
+			if !isValidCheck(check) {
 				return fmt.Errorf("%w: %s", errInvalidCheck, check)
 			}
 		}
@@ -71,7 +72,7 @@ func validate(c Config, checks []string) error {
 }
 
 // Parse reads the configuration file from the repo, stored in scorecard.yml, and returns a `Config`.
-func Parse(r io.Reader, checks []string) (Config, error) {
+func Parse(r io.Reader) (Config, error) {
 	c := Config{}
 	// Find scorecard.yml file in the repository's root
 	content, err := io.ReadAll(r)
@@ -84,7 +85,7 @@ func Parse(r io.Reader, checks []string) (Config, error) {
 		return Config{}, fmt.Errorf("fail to parse configuration file: %w", err)
 	}
 
-	err = validate(c, checks)
+	err = validate(c)
 	if err != nil {
 		return Config{}, fmt.Errorf("configuration file is not valid: %w", err)
 	}

diff --git a/config/config_test.go b/config/config_test.go
@@ -12,45 +12,40 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-// Warning: config cannot import checks. This is why we declare a different package here
-// and import both config and checks to test config.
-package config_test
+package config
 
 import (
 	"os"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
-
-	"github.com/ossf/scorecard/v5/checks"
-	"github.com/ossf/scorecard/v5/config"
 )
 
 func Test_Parse_Checks(t *testing.T) {
 	t.Parallel()
 	tests := []struct {
 		name       string
 		configPath string
-		want       config.Config
+		want       Config
 		wantErr    bool
 	}{
 		{
 			name:       "Annotation on a single check",
 			configPath: "testdata/single_check.yml",
-			want: config.Config{
-				Annotations: []config.Annotation{
+			want: Config{
+				Annotations: []Annotation{
 					{
 						Checks:  []string{"binary-artifacts"},
-						Reasons: []config.ReasonGroup{{Reason: "test-data"}},
+						Reasons: []ReasonGroup{{Reason: "test-data"}},
 					},
 				},
 			},
 		},
 		{
 			name:       "Annotation on all checks",
 			configPath: "testdata/all_checks.yml",
-			want: config.Config{
-				Annotations: []config.Annotation{
+			want: Config{
+				Annotations: []Annotation{
 					{
 						Checks: []string{
 							"binary-artifacts",
@@ -72,19 +67,19 @@ func Test_Parse_Checks(t *testing.T) {
 							"token-permissions",
 							"vulnerabilities",
 						},
-						Reasons: []config.ReasonGroup{{Reason: "test-data"}},
+						Reasons: []ReasonGroup{{Reason: "test-data"}},
 					},
 				},
 			},
 		},
 		{
 			name:       "Annotating all reasons",
 			configPath: "testdata/all_reasons.yml",
-			want: config.Config{
-				Annotations: []config.Annotation{
+			want: Config{
+				Annotations: []Annotation{
 					{
 						Checks: []string{"binary-artifacts"},
-						Reasons: []config.ReasonGroup{
+						Reasons: []ReasonGroup{
 							{Reason: "test-data"},
 							{Reason: "remediated"},
 							{Reason: "not-applicable"},
@@ -98,15 +93,15 @@ func Test_Parse_Checks(t *testing.T) {
 		{
 			name:       "Multiple annotations",
 			configPath: "testdata/multiple_annotations.yml",
-			want: config.Config{
-				Annotations: []config.Annotation{
+			want: Config{
+				Annotations: []Annotation{
 					{
 						Checks:  []string{"binary-artifacts"},
-						Reasons: []config.ReasonGroup{{Reason: "test-data"}},
+						Reasons: []ReasonGroup{{Reason: "test-data"}},
 					},
 					{
 						Checks:  []string{"pinned-dependencies"},
-						Reasons: []config.ReasonGroup{{Reason: "not-applicable"}},
+						Reasons: []ReasonGroup{{Reason: "not-applicable"}},
 					},
 				},
 			},
@@ -124,17 +119,13 @@ func Test_Parse_Checks(t *testing.T) {
 	}
 	for _, tt := range tests {
 		tt := tt // Re-initializing variable so it is not changed while executing the closure below
-		allChecks := []string{}
-		for check := range checks.GetAll() {
-			allChecks = append(allChecks, check)
-		}
 		t.Run(tt.name, func(t *testing.T) {
 			t.Parallel()
 			r, err := os.Open(tt.configPath)
 			if err != nil {
 				t.Fatalf("Could not open config test file: %s", tt.configPath)
 			}
-			result, err := config.Parse(r, allChecks)
+			result, err := Parse(r)
 			if (err != nil) != tt.wantErr {
 				t.Fatalf("Unexpected error during Parse: got %v, wantErr %v", err, tt.wantErr)
 			}

diff --git a/internal/checknames/checknames.go b/internal/checknames/checknames.go
@@ -0,0 +1,64 @@
+// Copyright 2024 OpenSSF 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 checknames
+
+type CheckName = string
+
+// Redefining check names here to avoid circular imports.
+const (
+	BinaryArtifacts      CheckName = "Binary-Artifacts"
+	BranchProtection     CheckName = "Branch-Protection"
+	CIIBestPractices     CheckName = "CII-Best-Practices"
+	CITests              CheckName = "CI-Tests"
+	CodeReview           CheckName = "Code-Review"
+	Contributors         CheckName = "Contributors"
+	DangerousWorkflow    CheckName = "Dangerous-Workflow"
+	DependencyUpdateTool CheckName = "Dependency-Update-Tool"
+	Fuzzing              CheckName = "Fuzzing"
+	License              CheckName = "License"
+	Maintained           CheckName = "Maintained"
+	Packaging            CheckName = "Packaging"
+	PinnedDependencies   CheckName = "Pinned-Dependencies"
+	SAST                 CheckName = "SAST"
+	SBOM                 CheckName = "SBOM"
+	SecurityPolicy       CheckName = "Security-Policy"
+	SignedReleases       CheckName = "Signed-Releases"
+	TokenPermissions     CheckName = "Token-Permissions"
+	Vulnerabilities      CheckName = "Vulnerabilities"
+	Webhooks             CheckName = "Webhooks"
+)
+
+var AllValidChecks []string = []string{
+	BinaryArtifacts,
+	BranchProtection,
+	CIIBestPractices,
+	CITests,
+	CodeReview,
+	Contributors,
+	DangerousWorkflow,
+	DependencyUpdateTool,
+	Fuzzing,
+	License,
+	Maintained,
+	Packaging,
+	PinnedDependencies,
+	SAST,
+	SBOM,
+	SecurityPolicy,
+	SignedReleases,
+	TokenPermissions,
+	Vulnerabilities,
+	Webhooks,
+}
diff --git a/internal/probes/probes.go b/internal/probes/probes.go
@@ -20,39 +20,14 @@ import (
 	"github.com/ossf/scorecard/v5/checker"
 	"github.com/ossf/scorecard/v5/errors"
 	"github.com/ossf/scorecard/v5/finding"
-)
-
-type CheckName string
-
-// Redefining check names here to avoid circular imports.
-const (
-	BinaryArtifacts      CheckName = "Binary-Artifacts"
-	BranchProtection     CheckName = "Branch-Protection"
-	CIIBestPractices     CheckName = "CII-Best-Practices"
-	CITests              CheckName = "CI-Tests"
-	CodeReview           CheckName = "Code-Review"
-	Contributors         CheckName = "Contributors"
-	DangerousWorkflow    CheckName = "Dangerous-Workflow"
-	DependencyUpdateTool CheckName = "Dependency-Update-Tool"
-	Fuzzing              CheckName = "Fuzzing"
-	License              CheckName = "License"
-	Maintained           CheckName = "Maintained"
-	Packaging            CheckName = "Packaging"
-	PinnedDependencies   CheckName = "Pinned-Dependencies"
-	SAST                 CheckName = "SAST"
-	SBOM                 CheckName = "SBOM"
-	SecurityPolicy       CheckName = "Security-Policy"
-	SignedReleases       CheckName = "Signed-Releases"
-	TokenPermissions     CheckName = "Token-Permissions"
-	Vulnerabilities      CheckName = "Vulnerabilities"
-	Webhooks             CheckName = "Webhooks"
+	"github.com/ossf/scorecard/v5/internal/checknames"
 )
 
 type Probe struct {
 	Name                      string
 	Implementation            ProbeImpl
 	IndependentImplementation IndependentProbeImpl
-	RequiredRawData           []CheckName
+	RequiredRawData           []checknames.CheckName
 }
 
 type ProbeImpl func(*checker.RawResults) ([]finding.Finding, string, error)
@@ -62,7 +37,7 @@ type IndependentProbeImpl func(*checker.CheckRequest) ([]finding.Finding, string
 // registered is the mapping of all registered probes.
 var registered = map[string]Probe{}
 
-func MustRegister(name string, impl ProbeImpl, requiredRawData []CheckName) {
+func MustRegister(name string, impl ProbeImpl, requiredRawData []checknames.CheckName) {
 	err := register(Probe{
 		Name:            name,
 		Implementation:  impl,

diff --git a/internal/probes/probes_test.go b/internal/probes/probes_test.go
@@ -21,6 +21,7 @@ import (
 
 	"github.com/ossf/scorecard/v5/checker"
 	"github.com/ossf/scorecard/v5/finding"
+	. "github.com/ossf/scorecard/v5/internal/checknames"
 )
 
 func emptyImpl(r *checker.RawResults) ([]finding.Finding, string, error) {

diff --git a/pkg/scorecard.go b/pkg/scorecard.go
@@ -180,11 +180,7 @@ func runScorecard(ctx context.Context,
 		if r != nil {
 			defer r.Close()
 			logger.Info(fmt.Sprintf("using maintainer annotations: %s", path))
-			checks := []string{}
-			for check := range checksToRun {
-				checks = append(checks, check)
-			}
-			c, err := config.Parse(r, checks)
+			c, err := config.Parse(r)
 			if err != nil {
 				logger.Info(fmt.Sprintf("couldn't parse maintainer annotations: %v", err))
 			}

diff --git a/pkg/scorecard_result.go b/pkg/scorecard_result.go
@@ -402,7 +402,7 @@ func populateRawResults(request *checker.CheckRequest, probesToRun []string, ret
 			return fmt.Errorf("getting probe %q: %w", probeName, err)
 		}
 		for _, checkName := range p.RequiredRawData {
-			checkName := string(checkName)
+			checkName := checkName
 			if !seen[checkName] {
 				err := assignRawData(checkName, request, ret)
 				if err != nil {

diff --git a/probes/archived/impl.go b/probes/archived/impl.go
@@ -20,12 +20,13 @@ import (
 
 	"github.com/ossf/scorecard/v5/checker"
 	"github.com/ossf/scorecard/v5/finding"
+	"github.com/ossf/scorecard/v5/internal/checknames"
 	"github.com/ossf/scorecard/v5/internal/probes"
 	"github.com/ossf/scorecard/v5/probes/internal/utils/uerror"
 )
 
 func init() {
-	probes.MustRegister(Probe, Run, []probes.CheckName{probes.Maintained})
+	probes.MustRegister(Probe, Run, []checknames.CheckName{checknames.Maintained})
 }
 
 //go:embed *.yml

diff --git a/probes/blocksDeleteOnBranches/impl.go b/probes/blocksDeleteOnBranches/impl.go
@@ -21,12 +21,13 @@ import (
 
 	"github.com/ossf/scorecard/v5/checker"
 	"github.com/ossf/scorecard/v5/finding"
+	"github.com/ossf/scorecard/v5/internal/checknames"
 	"github.com/ossf/scorecard/v5/internal/probes"
 	"github.com/ossf/scorecard/v5/probes/internal/utils/uerror"
 )
 
 func init() {
-	probes.MustRegister(Probe, Run, []probes.CheckName{probes.BranchProtection})
+	probes.MustRegister(Probe, Run, []checknames.CheckName{checknames.BranchProtection})
 }
 
 //go:embed *.yml

diff --git a/probes/blocksForcePushOnBranches/impl.go b/probes/blocksForcePushOnBranches/impl.go
@@ -21,12 +21,13 @@ import (
 
 	"github.com/ossf/scorecard/v5/checker"
 	"github.com/ossf/scorecard/v5/finding"
+	"github.com/ossf/scorecard/v5/internal/checknames"
 	"github.com/ossf/scorecard/v5/internal/probes"
 	"github.com/ossf/scorecard/v5/probes/internal/utils/uerror"
 )
 
 func init() {
-	probes.MustRegister(Probe, Run, []probes.CheckName{probes.BranchProtection})
+	probes.MustRegister(Probe, Run, []checknames.CheckName{checknames.BranchProtection})
 }
 
 //go:embed *.yml

diff --git a/probes/branchProtectionAppliesToAdmins/impl.go b/probes/branchProtectionAppliesToAdmins/impl.go
@@ -21,13 +21,14 @@ import (
 
 	"github.com/ossf/scorecard/v5/checker"
 	"github.com/ossf/scorecard/v5/finding"
+	"github.com/ossf/scorecard/v5/internal/checknames"
 	"github.com/ossf/scorecard/v5/internal/probes"
 	"github.com/ossf/scorecard/v5/probes/internal/utils/branchprotection"
 	"github.com/ossf/scorecard/v5/probes/internal/utils/uerror"
 )
 
 func init() {
-	probes.MustRegister(Probe, Run, []probes.CheckName{probes.BranchProtection})
+	probes.MustRegister(Probe, Run, []checknames.CheckName{checknames.BranchProtection})
 }
 
 //go:embed *.yml

diff --git a/probes/branchesAreProtected/impl.go b/probes/branchesAreProtected/impl.go
@@ -21,12 +21,13 @@ import (
 
 	"github.com/ossf/scorecard/v5/checker"
 	"github.com/ossf/scorecard/v5/finding"
+	"github.com/ossf/scorecard/v5/internal/checknames"
 	"github.com/ossf/scorecard/v5/internal/probes"
 	"github.com/ossf/scorecard/v5/probes/internal/utils/uerror"
 )
 
 func init() {
-	probes.MustRegister(Probe, Run, []probes.CheckName{probes.BranchProtection})
+	probes.MustRegister(Probe, Run, []checknames.CheckName{checknames.BranchProtection})
 }
 
 //go:embed *.yml