✨ enforce check scores are between the min and max (#3769)

* enforce check scores are between the min and max if the score is invalid, the Error field is set and the score is replaced with an inconclusive result score. Signed-off-by: Spencer Schrock <sschrock@google.com> * exclude inconclusive result score Callers who want the score should use the CreateInconclusiveResult function. The goal is partly to enforce a consistent coding style, and partly to limit proportions which score to -1 accidentally. Signed-off-by: Spencer Schrock <sschrock@google.com> --------- Signed-off-by: Spencer Schrock <sschrock@google.com>
ossf · Jan 19, 2024 · 0dcad3a · 0dcad3a
1 parent b556d93
commit 0dcad3a
Show file tree

Hide file tree

Showing 2 changed files with 79 additions and 12 deletions.
diff --git a/checker/check_result.go b/checker/check_result.go
@@ -20,6 +20,7 @@ import (
 	"fmt"
 	"math"
 
+	sce "github.com/ossf/scorecard/v4/errors"
 	"github.com/ossf/scorecard/v4/finding"
 	"github.com/ossf/scorecard/v4/rule"
 )
@@ -175,8 +176,15 @@ func NormalizeReason(reason string, score int) string {
 
 // CreateResultWithScore is used when
 // the check runs without runtime errors, and we want to assign a
-// specific score.
+// specific score. The score must be between [MinResultScore] and [MaxResultScore].
+// Callers who want [InconclusiveResultScore] must use [CreateInconclusiveResult] instead.
+//
+// Passing an invalid score results in a runtime error result as if created by [CreateRuntimeErrorResult].
 func CreateResultWithScore(name, reason string, score int) CheckResult {
+	if score < MinResultScore || score > MaxResultScore {
+		err := sce.CreateInternal(sce.ErrScorecardInternal, fmt.Sprintf("invalid score (%d), please report this", score))
+		return CreateRuntimeErrorResult(name, err)
+	}
 	return CheckResult{
 		Name:    name,
 		Version: 2,
@@ -193,15 +201,8 @@ func CreateResultWithScore(name, reason string, score int) CheckResult {
 // the number of tests that succeeded.
 func CreateProportionalScoreResult(name, reason string, b, t int) CheckResult {
 	score := CreateProportionalScore(b, t)
-	return CheckResult{
-		Name: name,
-		// Old structure.
-		// New structure.
-		Version: 2,
-		Error:   nil,
-		Score:   score,
-		Reason:  NormalizeReason(reason, score),
-	}
+	reason = NormalizeReason(reason, score)
+	return CreateResultWithScore(name, reason, score)
 }
 
 // CreateMaxScoreResult is used when

diff --git a/checker/check_result_test.go b/checker/check_result_test.go
@@ -19,6 +19,9 @@ import (
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
+
+	sce "github.com/ossf/scorecard/v4/errors"
 )
 
 func TestAggregateScores(t *testing.T) {
@@ -493,12 +496,58 @@ func TestCreateResultWithScore(t *testing.T) {
 				Score:   1,
 			},
 		},
+		{
+			name: "inconclusive score is not valid",
+			args: args{
+				name:   "name",
+				reason: "reason",
+				score:  InconclusiveResultScore,
+			},
+			want: CheckResult{
+				Name:    "name",
+				Reason:  "internal error: invalid score (-1), please report this",
+				Version: 2,
+				Score:   -1,
+				Error:   sce.ErrScorecardInternal,
+			},
+		},
+		{
+			name: "score too low",
+			args: args{
+				name:   "name",
+				reason: "reason",
+				score:  -3,
+			},
+			want: CheckResult{
+				Name:    "name",
+				Reason:  "internal error: invalid score (-3), please report this",
+				Version: 2,
+				Score:   -1,
+				Error:   sce.ErrScorecardInternal,
+			},
+		},
+		{
+			name: "score too high",
+			args: args{
+				name:   "name",
+				reason: "reason",
+				score:  MaxResultScore + 2,
+			},
+			want: CheckResult{
+				Name:    "name",
+				Reason:  "internal error: invalid score (12), please report this",
+				Version: 2,
+				Score:   -1,
+				Error:   sce.ErrScorecardInternal,
+			},
+		},
 	}
 	for _, tt := range tests {
 		tt := tt
 		t.Run(tt.name, func(t *testing.T) {
 			t.Parallel()
-			if got := CreateResultWithScore(tt.args.name, tt.args.reason, tt.args.score); !cmp.Equal(got, tt.want) {
+			got := CreateResultWithScore(tt.args.name, tt.args.reason, tt.args.score)
+			if !cmp.Equal(got, tt.want, cmpopts.EquateErrors()) {
 				t.Errorf("CreateResultWithScore() = %v, want %v", got, cmp.Diff(got, tt.want))
 			}
 		})
@@ -548,12 +597,29 @@ func TestCreateProportionalScoreResult(t *testing.T) {
 				Version: 2,
 			},
 		},
+		{
+			name: "negative proportion, score too low",
+			args: args{
+				name:   "name",
+				reason: "reason",
+				b:      -2,
+				t:      1,
+			},
+			want: CheckResult{
+				Name:    "name",
+				Reason:  "internal error: invalid score (-20), please report this",
+				Version: 2,
+				Score:   -1,
+				Error:   sce.ErrScorecardInternal,
+			},
+		},
 	}
 	for _, tt := range tests {
 		tt := tt
 		t.Run(tt.name, func(t *testing.T) {
 			t.Parallel()
-			if got := CreateProportionalScoreResult(tt.args.name, tt.args.reason, tt.args.b, tt.args.t); !cmp.Equal(got, tt.want) {
+			got := CreateProportionalScoreResult(tt.args.name, tt.args.reason, tt.args.b, tt.args.t)
+			if !cmp.Equal(got, tt.want, cmpopts.EquateErrors()) {
 				t.Errorf("CreateProportionalScoreResult() = %v, want %v", got, cmp.Diff(got, tt.want))
 			}
 		})