🌱 convert Webhook check to probes (ossf#3522)

* 🌱 convert Webhook check to probes Signed-off-by: AdamKorcz <[email protected]> * Add test + nits Signed-off-by: AdamKorcz <[email protected]> * replace probe with OutcomeNotApplicable Signed-off-by: AdamKorcz <[email protected]> * return one finding per webhook Signed-off-by: Adam Korczynski <[email protected]> * change wording in def.yml Signed-off-by: Adam Korczynski <[email protected]> * change wording in def.yml and checks.md Signed-off-by: Adam Korczynski <[email protected]> * remove unused struct in test Signed-off-by: Adam Korczynski <[email protected]> * align checks.md with checks.yaml Signed-off-by: Adam Korczynski <[email protected]> * bring back experimental for webhooks Signed-off-by: Adam Korczynski <[email protected]> * change 'token' to 'secret' in probe Signed-off-by: Adam Korczynski <[email protected]> * use checker.MinResultScore instead of 0 Signed-off-by: Adam Korczynski <[email protected]> * Change test name Signed-off-by: Adam Korczynski <[email protected]> * use checker.MinResultScore instead of 0 Signed-off-by: Adam Korczynski <[email protected]> * fix typo Signed-off-by: Adam Korczynski <[email protected]> * Use checker.MaxResultScore instead of 10 Signed-off-by: Adam Korczynski <[email protected]> * rename probe Signed-off-by: Adam Korczynski <[email protected]> * remove the 'totalWebhooks' value from findings Signed-off-by: Adam Korczynski <[email protected]> --------- Signed-off-by: AdamKorcz <[email protected]> Signed-off-by: Adam Korczynski <[email protected]>
kgangerlm · Dec 5, 2023 · ec36916 · ec36916
1 parent c089856
commit ec36916
Show file tree

Hide file tree

Showing 10 changed files with 512 additions and 118 deletions.
diff --git a/checker/check_result_test.go b/checker/check_result_test.go
@@ -127,6 +127,14 @@ func TestCreateProportionalScore(t *testing.T) {
 			},
 			want: 5,
 		},
+		{
+			name: "2 and 5",
+			args: args{
+				success: 2,
+				total:   5,
+			},
+			want: 4,
+		},
 	}
 	for _, tt := range tests {
 		tt := tt

diff --git a/checks/evaluation/webhooks.go b/checks/evaluation/webhooks.go
@@ -20,42 +20,50 @@ import (
 	"github.com/ossf/scorecard/v4/checker"
 	sce "github.com/ossf/scorecard/v4/errors"
 	"github.com/ossf/scorecard/v4/finding"
+	"github.com/ossf/scorecard/v4/probes/webhooksUseSecrets"
 )
 
 // Webhooks applies the score policy for the Webhooks check.
-func Webhooks(name string, dl checker.DetailLogger,
-	r *checker.WebhooksData,
+func Webhooks(name string,
+	findings []finding.Finding, dl checker.DetailLogger,
 ) checker.CheckResult {
-	if r == nil {
-		e := sce.WithMessage(sce.ErrScorecardInternal, "empty raw data")
+	expectedProbes := []string{
+		webhooksUseSecrets.Probe,
+	}
+
+	if !finding.UniqueProbesEqual(findings, expectedProbes) {
+		e := sce.WithMessage(sce.ErrScorecardInternal, "invalid probe results")
 		return checker.CreateRuntimeErrorResult(name, e)
 	}
 
-	if len(r.Webhooks) < 1 {
-		return checker.CreateMaxScoreResult(name, "no webhooks defined")
+	if len(findings) == 1 && findings[0].Outcome == finding.OutcomeNotApplicable {
+		return checker.CreateMaxScoreResult(name, "project does not have webhook")
 	}
 
-	hasNoSecretCount := 0
-	for _, hook := range r.Webhooks {
-		if !hook.UsesAuthSecret {
-			dl.Warn(&checker.LogMessage{
-				Path: hook.Path,
-				Type: finding.FileTypeURL,
-				Text: "Webhook with no secret configured",
-			})
-			hasNoSecretCount++
+	var webhooksWithNoSecret int
+
+	totalWebhooks := len(findings)
+
+	for i := range findings {
+		f := &findings[i]
+		if f.Outcome == finding.OutcomeNegative {
+			webhooksWithNoSecret++
 		}
 	}
 
-	if hasNoSecretCount == 0 {
-		return checker.CreateMaxScoreResult(name, fmt.Sprintf("all %d hook(s) have a secret configured", len(r.Webhooks)))
+	if totalWebhooks == webhooksWithNoSecret {
+		return checker.CreateMinScoreResult(name, "no hook(s) have a secret configured")
 	}
 
-	if len(r.Webhooks) == hasNoSecretCount {
-		return checker.CreateMinScoreResult(name, fmt.Sprintf("%d hook(s) do not have a secret configured", len(r.Webhooks)))
+	if webhooksWithNoSecret == 0 {
+		msg := fmt.Sprintf("All %d of the projects webhooks are configured with a secret", totalWebhooks)
+		return checker.CreateMaxScoreResult(name, msg)
 	}
 
+	msg := fmt.Sprintf("%d out of the projects %d webhooks are configured without a secret",
+		webhooksWithNoSecret,
+		totalWebhooks)
+
 	return checker.CreateProportionalScoreResult(name,
-		fmt.Sprintf("%d/%d hook(s) with no secrets configured detected",
-			hasNoSecretCount, len(r.Webhooks)), hasNoSecretCount, len(r.Webhooks))
+		msg, totalWebhooks-webhooksWithNoSecret, totalWebhooks)
 }
diff --git a/checks/evaluation/webhooks_test.go b/checks/evaluation/webhooks_test.go
@@ -18,135 +18,219 @@ import (
 	"testing"
 
 	"github.com/ossf/scorecard/v4/checker"
-	"github.com/ossf/scorecard/v4/clients"
+	"github.com/ossf/scorecard/v4/finding"
 	scut "github.com/ossf/scorecard/v4/utests"
 )
 
 // TestWebhooks tests the webhooks check.
 func TestWebhooks(t *testing.T) {
 	t.Parallel()
-	//nolint:govet
-	type args struct {
-		name string
-		dl   checker.DetailLogger
-		r    *checker.WebhooksData
-	}
 	tests := []struct {
-		name    string
-		args    args
-		want    checker.CheckResult
-		wantErr bool
+		name     string
+		findings []finding.Finding
+		result   scut.TestReturn
 	}{
 		{
-			name: "r nil",
-			args: args{
-				name: "test_webhook_check_pass",
-				dl:   &scut.TestDetailLogger{},
+			name: "no webhooks",
+			findings: []finding.Finding{
+				{
+					Probe:   "webhooksUseSecrets",
+					Outcome: finding.OutcomeNotApplicable,
+				},
+			},
+			result: scut.TestReturn{
+				Score: checker.MaxResultScore,
 			},
-			wantErr: true,
 		},
 		{
-			name: "no webhooks",
-			args: args{
-				name: "no webhooks",
-				dl:   &scut.TestDetailLogger{},
-				r:    &checker.WebhooksData{},
+			name: "1 webhook with no secret",
+			findings: []finding.Finding{
+				{
+					Probe:   "webhooksUseSecrets",
+					Outcome: finding.OutcomeNegative,
+				},
 			},
-			want: checker.CheckResult{
-				Score: checker.MaxResultScore,
+			result: scut.TestReturn{
+				Score: checker.MinResultScore,
 			},
 		},
 		{
 			name: "1 webhook with secret",
-			args: args{
-				name: "1 webhook with secret",
-				dl:   &scut.TestDetailLogger{},
-				r: &checker.WebhooksData{
-					Webhooks: []clients.Webhook{
-						{
-							Path:           "https://github.com/owner/repo/settings/hooks/1234",
-							ID:             1234,
-							UsesAuthSecret: true,
-						},
-					},
+			findings: []finding.Finding{
+				{
+					Probe:   "webhooksUseSecrets",
+					Outcome: finding.OutcomePositive,
 				},
 			},
-			want: checker.CheckResult{
-				Score: 10,
+			result: scut.TestReturn{
+				Score: checker.MaxResultScore,
 			},
 		},
 		{
-			name: "1 webhook with no secret",
-			args: args{
-				name: "1 webhook with no secret",
-				dl:   &scut.TestDetailLogger{},
-				r: &checker.WebhooksData{
-					Webhooks: []clients.Webhook{
-						{
-							Path:           "https://github.com/owner/repo/settings/hooks/1234",
-							ID:             1234,
-							UsesAuthSecret: false,
-						},
-					},
+			name: "2 webhooks one of which has secret",
+			findings: []finding.Finding{
+				{
+					Probe:   "webhooksUseSecrets",
+					Outcome: finding.OutcomeNegative,
+				},
+				{
+					Probe:   "webhooksUseSecrets",
+					Outcome: finding.OutcomePositive,
 				},
 			},
-			want: checker.CheckResult{
-				Score: 0,
+			result: scut.TestReturn{
+				Score: 5,
 			},
 		},
 		{
-			name: "many webhooks with no secret and with secret",
-			args: args{
-				name: "many webhooks with no secret and with secret",
-				dl:   &scut.TestDetailLogger{},
-				r: &checker.WebhooksData{
-					Webhooks: []clients.Webhook{
-						{
-							Path:           "https://github.com/owner/repo/settings/hooks/1234",
-							ID:             1234,
-							UsesAuthSecret: false,
-						},
-						{
-							Path:           "https://github.com/owner/repo/settings/hooks/1111",
-							ID:             1111,
-							UsesAuthSecret: true,
-						},
-						{
-							Path:           "https://github.com/owner/repo/settings/hooks/4444",
-							ID:             4444,
-							UsesAuthSecret: true,
-						},
-						{
-							Path:           "https://github.com/owner/repo/settings/hooks/3333",
-							ID:             3333,
-							UsesAuthSecret: false,
-						},
-						{
-							Path:           "https://github.com/owner/repo/settings/hooks/2222",
-							ID:             2222,
-							UsesAuthSecret: false,
-						},
-					},
+			name: "Five webhooks three of which have secrets",
+			findings: []finding.Finding{
+				{
+					Probe:   "webhooksUseSecrets",
+					Outcome: finding.OutcomeNegative,
+				},
+				{
+					Probe:   "webhooksUseSecrets",
+					Outcome: finding.OutcomePositive,
+				},
+				{
+					Probe:   "webhooksUseSecrets",
+					Outcome: finding.OutcomePositive,
+				},
+				{
+					Probe:   "webhooksUseSecrets",
+					Outcome: finding.OutcomePositive,
+				},
+				{
+					Probe:   "webhooksUseSecrets",
+					Outcome: finding.OutcomeNegative,
 				},
 			},
-			want: checker.CheckResult{
+			result: scut.TestReturn{
 				Score: 6,
 			},
 		},
+		{
+			name: "One of 12 webhooks does not have secrets",
+			findings: []finding.Finding{
+				{
+					Probe:   "webhooksUseSecrets",
+					Outcome: finding.OutcomeNegative,
+				},
+				{
+					Probe:   "webhooksUseSecrets",
+					Outcome: finding.OutcomePositive,
+				},
+				{
+					Probe:   "webhooksUseSecrets",
+					Outcome: finding.OutcomePositive,
+				},
+				{
+					Probe:   "webhooksUseSecrets",
+					Outcome: finding.OutcomePositive,
+				},
+				{
+					Probe:   "webhooksUseSecrets",
+					Outcome: finding.OutcomePositive,
+				},
+				{
+					Probe:   "webhooksUseSecrets",
+					Outcome: finding.OutcomePositive,
+				},
+				{
+					Probe:   "webhooksUseSecrets",
+					Outcome: finding.OutcomePositive,
+				},
+				{
+					Probe:   "webhooksUseSecrets",
+					Outcome: finding.OutcomePositive,
+				},
+				{
+					Probe:   "webhooksUseSecrets",
+					Outcome: finding.OutcomePositive,
+				},
+				{
+					Probe:   "webhooksUseSecrets",
+					Outcome: finding.OutcomePositive,
+				},
+				{
+					Probe:   "webhooksUseSecrets",
+					Outcome: finding.OutcomePositive,
+				},
+				{
+					Probe:   "webhooksUseSecrets",
+					Outcome: finding.OutcomePositive,
+				},
+			},
+			result: scut.TestReturn{
+				Score: 9,
+			},
+		},
+		{
+			name: "Score should not drop below min score",
+			findings: []finding.Finding{
+				{
+					Probe:   "webhooksUseSecrets",
+					Outcome: finding.OutcomeNegative,
+				},
+				{
+					Probe:   "webhooksUseSecrets",
+					Outcome: finding.OutcomeNegative,
+				},
+				{
+					Probe:   "webhooksUseSecrets",
+					Outcome: finding.OutcomeNegative,
+				},
+				{
+					Probe:   "webhooksUseSecrets",
+					Outcome: finding.OutcomeNegative,
+				},
+				{
+					Probe:   "webhooksUseSecrets",
+					Outcome: finding.OutcomeNegative,
+				},
+				{
+					Probe:   "webhooksUseSecrets",
+					Outcome: finding.OutcomeNegative,
+				},
+				{
+					Probe:   "webhooksUseSecrets",
+					Outcome: finding.OutcomeNegative,
+				},
+				{
+					Probe:   "webhooksUseSecrets",
+					Outcome: finding.OutcomeNegative,
+				},
+				{
+					Probe:   "webhooksUseSecrets",
+					Outcome: finding.OutcomeNegative,
+				},
+				{
+					Probe:   "webhooksUseSecrets",
+					Outcome: finding.OutcomeNegative,
+				},
+				{
+					Probe:   "webhooksUseSecrets",
+					Outcome: finding.OutcomeNegative,
+				},
+				{
+					Probe:   "webhooksUseSecrets",
+					Outcome: finding.OutcomeNegative,
+				},
+			},
+			result: scut.TestReturn{
+				Score: checker.MinResultScore,
+			},
+		},
 	}
 	for _, tt := range tests {
 		tt := tt
 		t.Run(tt.name, func(t *testing.T) {
 			t.Parallel()
-			got := Webhooks(tt.args.name, tt.args.dl, tt.args.r)
-			if tt.wantErr {
-				if got.Error == nil {
-					t.Errorf("Webhooks() error = %v, wantErr %v", got.Error, tt.wantErr)
-				}
-			} else {
-				if got.Score != tt.want.Score {
-					t.Errorf("Webhooks() = %v, want %v", got.Score, tt.want.Score)
-				}
+			dl := scut.TestDetailLogger{}
+			got := Webhooks(tt.name, tt.findings, &dl)
+			if !scut.ValidateTestReturn(t, tt.name, &tt.result, &got, &dl) {
+				t.Errorf("got %v, expected %v", got, tt.result)
 			}
 		})
 	}