Skip to content

Commit 0dcad3a

Browse files
✨ 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 <[email protected]> * 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 <[email protected]> --------- Signed-off-by: Spencer Schrock <[email protected]>
1 parent b556d93 commit 0dcad3a

File tree

2 files changed

+79
-12
lines changed

2 files changed

+79
-12
lines changed

checker/check_result.go

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import (
2020
"fmt"
2121
"math"
2222

23+
sce "github.com/ossf/scorecard/v4/errors"
2324
"github.com/ossf/scorecard/v4/finding"
2425
"github.com/ossf/scorecard/v4/rule"
2526
)
@@ -175,8 +176,15 @@ func NormalizeReason(reason string, score int) string {
175176

176177
// CreateResultWithScore is used when
177178
// the check runs without runtime errors, and we want to assign a
178-
// specific score.
179+
// specific score. The score must be between [MinResultScore] and [MaxResultScore].
180+
// Callers who want [InconclusiveResultScore] must use [CreateInconclusiveResult] instead.
181+
//
182+
// Passing an invalid score results in a runtime error result as if created by [CreateRuntimeErrorResult].
179183
func CreateResultWithScore(name, reason string, score int) CheckResult {
184+
if score < MinResultScore || score > MaxResultScore {
185+
err := sce.CreateInternal(sce.ErrScorecardInternal, fmt.Sprintf("invalid score (%d), please report this", score))
186+
return CreateRuntimeErrorResult(name, err)
187+
}
180188
return CheckResult{
181189
Name: name,
182190
Version: 2,
@@ -193,15 +201,8 @@ func CreateResultWithScore(name, reason string, score int) CheckResult {
193201
// the number of tests that succeeded.
194202
func CreateProportionalScoreResult(name, reason string, b, t int) CheckResult {
195203
score := CreateProportionalScore(b, t)
196-
return CheckResult{
197-
Name: name,
198-
// Old structure.
199-
// New structure.
200-
Version: 2,
201-
Error: nil,
202-
Score: score,
203-
Reason: NormalizeReason(reason, score),
204-
}
204+
reason = NormalizeReason(reason, score)
205+
return CreateResultWithScore(name, reason, score)
205206
}
206207

207208
// CreateMaxScoreResult is used when

checker/check_result_test.go

Lines changed: 68 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,9 @@ import (
1919
"testing"
2020

2121
"github.com/google/go-cmp/cmp"
22+
"github.com/google/go-cmp/cmp/cmpopts"
23+
24+
sce "github.com/ossf/scorecard/v4/errors"
2225
)
2326

2427
func TestAggregateScores(t *testing.T) {
@@ -493,12 +496,58 @@ func TestCreateResultWithScore(t *testing.T) {
493496
Score: 1,
494497
},
495498
},
499+
{
500+
name: "inconclusive score is not valid",
501+
args: args{
502+
name: "name",
503+
reason: "reason",
504+
score: InconclusiveResultScore,
505+
},
506+
want: CheckResult{
507+
Name: "name",
508+
Reason: "internal error: invalid score (-1), please report this",
509+
Version: 2,
510+
Score: -1,
511+
Error: sce.ErrScorecardInternal,
512+
},
513+
},
514+
{
515+
name: "score too low",
516+
args: args{
517+
name: "name",
518+
reason: "reason",
519+
score: -3,
520+
},
521+
want: CheckResult{
522+
Name: "name",
523+
Reason: "internal error: invalid score (-3), please report this",
524+
Version: 2,
525+
Score: -1,
526+
Error: sce.ErrScorecardInternal,
527+
},
528+
},
529+
{
530+
name: "score too high",
531+
args: args{
532+
name: "name",
533+
reason: "reason",
534+
score: MaxResultScore + 2,
535+
},
536+
want: CheckResult{
537+
Name: "name",
538+
Reason: "internal error: invalid score (12), please report this",
539+
Version: 2,
540+
Score: -1,
541+
Error: sce.ErrScorecardInternal,
542+
},
543+
},
496544
}
497545
for _, tt := range tests {
498546
tt := tt
499547
t.Run(tt.name, func(t *testing.T) {
500548
t.Parallel()
501-
if got := CreateResultWithScore(tt.args.name, tt.args.reason, tt.args.score); !cmp.Equal(got, tt.want) {
549+
got := CreateResultWithScore(tt.args.name, tt.args.reason, tt.args.score)
550+
if !cmp.Equal(got, tt.want, cmpopts.EquateErrors()) {
502551
t.Errorf("CreateResultWithScore() = %v, want %v", got, cmp.Diff(got, tt.want))
503552
}
504553
})
@@ -548,12 +597,29 @@ func TestCreateProportionalScoreResult(t *testing.T) {
548597
Version: 2,
549598
},
550599
},
600+
{
601+
name: "negative proportion, score too low",
602+
args: args{
603+
name: "name",
604+
reason: "reason",
605+
b: -2,
606+
t: 1,
607+
},
608+
want: CheckResult{
609+
Name: "name",
610+
Reason: "internal error: invalid score (-20), please report this",
611+
Version: 2,
612+
Score: -1,
613+
Error: sce.ErrScorecardInternal,
614+
},
615+
},
551616
}
552617
for _, tt := range tests {
553618
tt := tt
554619
t.Run(tt.name, func(t *testing.T) {
555620
t.Parallel()
556-
if got := CreateProportionalScoreResult(tt.args.name, tt.args.reason, tt.args.b, tt.args.t); !cmp.Equal(got, tt.want) {
621+
got := CreateProportionalScoreResult(tt.args.name, tt.args.reason, tt.args.b, tt.args.t)
622+
if !cmp.Equal(got, tt.want, cmpopts.EquateErrors()) {
557623
t.Errorf("CreateProportionalScoreResult() = %v, want %v", got, cmp.Diff(got, tt.want))
558624
}
559625
})

0 commit comments

Comments
 (0)