update per feedback

Signed-off-by: cpanato <[email protected]>

checker/raw_result.go

@@ -79,8 +79,8 @@ type WebhooksData struct {
 // WebhookData contains the raw results
 // for webhook check.
 type WebhookData struct {
-	ID        *int64
-	HasSecret *bool
+	ID             int64
+	UsesAuthSecret bool
 }
 
 // BranchProtectionsData contains the raw results

checks/raw/webhook.go

@@ -33,9 +33,10 @@ func WebHook(c *checker.CheckRequest) (checker.WebhooksData, error) {
 
 	hooks := []checker.WebhookData{}
 	for _, hook := range hooksResp {
+
 		v := checker.WebhookData{
-			ID:        &hook.ID,
-			HasSecret: &hook.HasSecret,
+			ID:             hook.ID,
+			UsesAuthSecret: hook.UsesAuthSecret,
 			// Note: add fields if needed.
 		}
 		hooks = append(hooks, v)

checks/raw/webhooks_test.go

@@ -31,12 +31,12 @@ func TestWebhooks(t *testing.T) {
 	t.Parallel()
 	//nolint
 	tests := []struct {
-		name              string
-		err               error
-		wantErr           bool
-		expectedHasSecret int
-		expected          scut.TestReturn
-		webhookResponse   []*clients.Webhook
+		name                   string
+		err                    error
+		wantErr                bool
+		expectedUsesAuthSecret int
+		expected               scut.TestReturn
+		webhookResponse        []*clients.Webhook
 	}{
 		{
 			name:            "No Webhooks",
@@ -49,41 +49,41 @@ func TestWebhooks(t *testing.T) {
 			err:     sce.ErrScorecardInternal,
 		},
 		{
-			name:              "Webhook with no secret",
-			wantErr:           false,
-			expectedHasSecret: 0,
+			name:                   "Webhook with no secret",
+			wantErr:                false,
+			expectedUsesAuthSecret: 0,
 			webhookResponse: []*clients.Webhook{
 				{
-					HasSecret: false,
+					UsesAuthSecret: false,
 				},
 			},
 		},
 		{
-			name:              "Webhook with secrets",
-			wantErr:           false,
-			expectedHasSecret: 2,
+			name:                   "Webhook with secrets",
+			wantErr:                false,
+			expectedUsesAuthSecret: 2,
 			webhookResponse: []*clients.Webhook{
 				{
-					HasSecret: true,
+					UsesAuthSecret: true,
 				},
 				{
-					HasSecret: true,
+					UsesAuthSecret: true,
 				},
 			},
 		},
 		{
-			name:              "Webhook with secrets and some without defined secrets",
-			wantErr:           false,
-			expectedHasSecret: 1,
+			name:                   "Webhook with secrets and some without defined secrets",
+			wantErr:                false,
+			expectedUsesAuthSecret: 1,
 			webhookResponse: []*clients.Webhook{
 				{
-					HasSecret: true,
+					UsesAuthSecret: true,
 				},
 				{
-					HasSecret: false,
+					UsesAuthSecret: false,
 				},
 				{
-					HasSecret: false,
+					UsesAuthSecret: false,
 				},
 			},
 		},
@@ -117,13 +117,13 @@ func TestWebhooks(t *testing.T) {
 			if !tt.wantErr {
 				gotHasSecret := 0
 				for _, gotHook := range got.Webhook {
-					if *gotHook.HasSecret {
+					if gotHook.UsesAuthSecret {
 						gotHasSecret++
 					}
 				}
 
-				if gotHasSecret != tt.expectedHasSecret {
-					t.Errorf("Webhooks() got = %v, want %v", gotHasSecret, tt.expectedHasSecret)
+				if gotHasSecret != tt.expectedUsesAuthSecret {
+					t.Errorf("Webhooks() got = %v, want %v", gotHasSecret, tt.expectedUsesAuthSecret)
 				}
 			}
 

checks/webhook.go

@@ -16,8 +16,13 @@ package checks
 
 import (
 	"fmt"
+	"strconv"
+	"strings"
+
+	"github.com/olekukonko/tablewriter"
 
 	"github.com/ossf/scorecard/v4/checker"
+	"github.com/ossf/scorecard/v4/checks/raw"
 	sce "github.com/ossf/scorecard/v4/errors"
 )
 
@@ -34,33 +39,58 @@ func init() {
 	}
 }
 
-// WebHooks run Contributors check.
+// WebHooks run Webhooks check.
 func WebHooks(c *checker.CheckRequest) checker.CheckResult {
-	hooks, err := c.RepoClient.ListWebhooks()
+	rawData, err := raw.WebHook(c)
 	if err != nil {
-		e := sce.WithMessage(sce.ErrScorecardInternal, fmt.Sprintf("Client.Repositories.ListWebhooks: %v", err))
+		e := sce.WithMessage(sce.ErrScorecardInternal, err.Error())
 		return checker.CreateRuntimeErrorResult(CheckWebHooks, e)
 	}
 
-	if len(hooks) < 1 {
+	if len(rawData.Webhook) < 1 {
 		return checker.CreateMaxScoreResult(CheckWebHooks, "no webhooks defined")
 	}
 
-	hasSecretToCount := 0
-	for _, hook := range hooks {
-		if !hook.HasSecret {
-			hasSecretToCount++
+	hasNoSecretCount := 0
+	for _, hook := range rawData.Webhook {
+		if !hook.UsesAuthSecret {
+			c.Dlogger.Warn(&checker.LogMessage{
+				Path: fmt.Sprintf("https://%s/settings/hooks/%d", c.RepoClient.URI(), hook.ID),
+				Type: checker.FileTypeURL,
+				Text: "Webhook with no secret configured",
+			})
+			hasNoSecretCount++
 		}
 	}
 
-	if hasSecretToCount == 0 {
-		return checker.CreateMaxScoreResult(CheckWebHooks, "all webhooks have secrets defined")
+	generateWebhookTable(c.RepoClient.URI(), rawData.Webhook)
+
+	if hasNoSecretCount == 0 {
+		return checker.CreateMaxScoreResult(CheckWebHooks, fmt.Sprintf("all %d hook(s) have a secret configured", len(rawData.Webhook)))
 	}
 
-	if len(hooks) == hasSecretToCount {
-		return checker.CreateMinScoreResult(CheckWebHooks, "webhooks with no secrets configured detected")
+	if len(rawData.Webhook) == hasNoSecretCount {
+		return checker.CreateMinScoreResult(CheckWebHooks, fmt.Sprintf("%d hook(s) do not have a secret configured", len(rawData.Webhook)))
 	}
 
 	return checker.CreateProportionalScoreResult(CheckWebHooks,
-		"webhooks with no secrets configured detected", hasSecretToCount, len(hooks))
+		fmt.Sprintf("%d out of %d hook(s) with no secrets configured detected", hasNoSecretCount, len(rawData.Webhook)), hasNoSecretCount, len(rawData.Webhook))
+}
+
+func generateWebhookTable(repo string, data []checker.WebhookData) {
+	tableString := &strings.Builder{}
+	table := tablewriter.NewWriter(tableString)
+	table.SetAutoWrapText(false)
+	table.SetHeader([]string{"GitHub Webhook", "Uses Auth Secret"})
+
+	for _, hook := range data {
+		table.Append([]string{fmt.Sprintf("https://%s/settings/hooks/%d", repo, hook.ID), strconv.FormatBool(hook.UsesAuthSecret)})
+		// https: //github.com/cpanato/testing-ci-providers/settings/hooks/289347313
+	}
+
+	table.SetBorders(tablewriter.Border{Left: true, Top: true, Right: true, Bottom: true})
+	table.SetCenterSeparator("|")
+	table.Render()
+
+	fmt.Println(tableString.String())
 }

checks/webhook_test.go

@@ -23,18 +23,21 @@ import (
 	"github.com/ossf/scorecard/v4/checker"
 	"github.com/ossf/scorecard/v4/clients"
 	mockrepo "github.com/ossf/scorecard/v4/clients/mockclients"
+	scut "github.com/ossf/scorecard/v4/utests"
 )
 
 func TestWebhooks(t *testing.T) {
 	t.Parallel()
 	tests := []struct {
 		expected checker.CheckResult
+		uri      string
 		err      error
 		name     string
 		webhooks []*clients.Webhook
 	}{
 		{
 			name: "No Webhooks",
+			uri:  "github.com/owner/repo",
 			expected: checker.CheckResult{
 				Pass:  true,
 				Score: 10,
@@ -44,43 +47,50 @@ func TestWebhooks(t *testing.T) {
 		},
 		{
 			name: "With Webhooks and secret set",
+			uri:  "github.com/owner/repo",
 			expected: checker.CheckResult{
 				Pass:  true,
 				Score: 10,
 			},
 			err: nil,
 			webhooks: []*clients.Webhook{
 				{
-					HasSecret: true,
+					ID:             12345,
+					UsesAuthSecret: true,
 				},
 			},
 		},
 		{
 			name: "With Webhooks and no secret set",
+			uri:  "github.com/owner/repo",
 			expected: checker.CheckResult{
 				Pass:  false,
 				Score: 0,
 			},
 			err: nil,
 			webhooks: []*clients.Webhook{
 				{
-					HasSecret: false,
+					ID:             12345,
+					UsesAuthSecret: false,
 				},
 			},
 		},
 		{
 			name: "With 2 Webhooks with and whitout secrets configured",
+			uri:  "github.com/owner/repo",
 			expected: checker.CheckResult{
 				Pass:  false,
 				Score: 5,
 			},
 			err: nil,
 			webhooks: []*clients.Webhook{
 				{
-					HasSecret: false,
+					ID:             12345,
+					UsesAuthSecret: false,
 				},
 				{
-					HasSecret: true,
+					ID:             54321,
+					UsesAuthSecret: true,
 				},
 			},
 		},
@@ -101,9 +111,13 @@ func TestWebhooks(t *testing.T) {
 				return tt.webhooks, tt.err
 			}).MaxTimes(1)
 
+			mockRepo.EXPECT().URI().Return(tt.uri).AnyTimes()
+
+			dl := scut.TestDetailLogger{}
 			req := checker.CheckRequest{
 				RepoClient: mockRepo,
 				Ctx:        context.TODO(),
+				Dlogger:    &dl,
 			}
 			res := WebHooks(&req)
 			if tt.err != nil {

clients/githubrepo/webhook.go

@@ -55,9 +55,10 @@ func (handler *webhookHandler) setup() error {
 		}
 
 		for _, hook := range hooks {
+
 			repoHook := &clients.Webhook{
-				ID:        hook.GetID(),
-				HasSecret: hasSecret(hook.Config),
+				ID:             hook.GetID(),
+				UsesAuthSecret: getAuthSecret(hook.Config),
 			}
 			handler.webhook = append(handler.webhook, repoHook)
 		}
@@ -66,7 +67,7 @@ func (handler *webhookHandler) setup() error {
 	return handler.errSetup
 }
 
-func hasSecret(config map[string]interface{}) bool {
+func getAuthSecret(config map[string]interface{}) bool {
 	if val, ok := config["secret"]; ok {
 		if val != nil {
 			return true

clients/webhook.go

@@ -16,6 +16,6 @@ package clients
 
 // Webhook represents VCS Webhook.
 type Webhook struct {
-	ID        int64
-	HasSecret bool
+	ID             int64
+	UsesAuthSecret bool
 }

docs/checks.md

@@ -603,13 +603,11 @@ possible.
 
 ## Webhooks 
 
-Risk: `High`  (possible service can be accessed by third-party)
+Risk: `High`  (service possibly accessible to third parties)
 
-This check validates if the webhook defined in the repository have a token configured to authenticate the origin of the request to make sure that is a trusted request.
-
+This check determines whether the webhook defined in the repository has a token configured to authenticate the origins of requests.
 
 **Remediation steps**
-- Check if your service supports the token authentication.
-- If that supports set the secret in the Webhook configuration.
-- If does not support, consider implementing this support, more information can be found on <https://docs.github.com/en/developers/webhooks-and-events/webhooks/securing-your-webhooks>.
-
+- Check whether your service supports token authentication.
+- If there is support for token authentication, set the secret in the webhook configuration. See [Setting up a webhook](https://docs.github.com/en/developers/webhooks-and-events/webhooks/creating-webhooks#setting-up-a-webhook)
+- If there is no support for token authentication, consider implementing it by following [these directions](https://docs.github.com/en/developers/webhooks-and-events/webhooks/securing-your-webhooks).

docs/checks/internal/checks.yaml

@@ -738,13 +738,13 @@ checks:
     repos: GitHub
     short: This check validate if the webhook defined in the repository have a token configured.
     description: |
-      Risk: `High`  (possible service can be accessed by third-party)
+      Risk: `High`  (service possibly accessible to third parties)
 
-      This check validates if the webhook defined in the repository have a token configured to authenticate the origin of the request to make sure that is a trusted request.
+      This check determines whether the webhook defined in the repository has a token configured to authenticate the origins of requests.
     remediation:
-      - >-
-        Check if your service supports the token authentication.
-      - >-
-        If that supports set the secret in the Webhook configuration.
-      - >-
-        If does not support, consider implementing this support, more information can be found on <https://docs.github.com/en/developers/webhooks-and-events/webhooks/securing-your-webhooks>.
+    - >-
+      Check whether your service supports token authentication.
+    - >-
+      If there is support for token authentication, set the secret in the webhook configuration. See [Setting up a webhook](https://docs.github.com/en/developers/webhooks-and-events/webhooks/creating-webhooks#setting-up-a-webhook)
+    - >-
+      If there is no support for token authentication, consider implementing it by following [these directions](https://docs.github.com/en/developers/webhooks-and-events/webhooks/securing-your-webhooks).