⚠️ Allow probes to specify their own bad outcomes

checker/check_result.go

@@ -22,7 +22,6 @@ import (
 
 	sce "github.com/ossf/scorecard/v4/errors"
 	"github.com/ossf/scorecard/v4/finding"
-	"github.com/ossf/scorecard/v4/finding/probe"
 )
 
 type (
@@ -86,13 +85,13 @@ type LogMessage struct {
 	Finding *finding.Finding
 
 	// Non-structured results.
-	Text        string             // A short string explaining why the detail was recorded/logged.
-	Path        string             // Fullpath to the file.
-	Type        finding.FileType   // Type of file.
-	Offset      uint               // Offset in the file of Path (line for source/text files).
-	EndOffset   uint               // End of offset in the file, e.g. if the command spans multiple lines.
-	Snippet     string             // Snippet of code
-	Remediation *probe.Remediation // Remediation information, if any.
+	Text        string               // A short string explaining why the detail was recorded/logged.
+	Path        string               // Fullpath to the file.
+	Type        finding.FileType     // Type of file.
+	Offset      uint                 // Offset in the file of Path (line for source/text files).
+	EndOffset   uint                 // End of offset in the file, e.g. if the command spans multiple lines.
+	Snippet     string               // Snippet of code
+	Remediation *finding.Remediation // Remediation information, if any.
 }
 
 // ProportionalScoreWeighted is a structure that contains

checker/raw_result.go

@@ -20,7 +20,6 @@ import (
 
 	"github.com/ossf/scorecard/v4/clients"
 	"github.com/ossf/scorecard/v4/finding"
-	"github.com/ossf/scorecard/v4/finding/probe"
 )
 
 // RawResults contains results before a policy
@@ -126,7 +125,7 @@ type Dependency struct {
 	Location    *File
 	Msg         *string // Only for debug messages.
 	Pinned      *bool
-	Remediation *probe.Remediation
+	Remediation *finding.Remediation
 	Type        DependencyUseType
 }
 

checks/dangerous_workflow_test.go

@@ -0,0 +1,98 @@
+// 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 checks
+
+import (
+	"context"
+	"io"
+	"os"
+	"testing"
+
+	"github.com/golang/mock/gomock"
+
+	"github.com/ossf/scorecard/v4/checker"
+	mockrepo "github.com/ossf/scorecard/v4/clients/mockclients"
+	scut "github.com/ossf/scorecard/v4/utests"
+)
+
+func TestDangerousWorkflow(t *testing.T) {
+	t.Parallel()
+	tests := []struct {
+		name          string
+		workflowPaths []string
+		err           error
+		expected      scut.TestReturn
+	}{
+		{
+			name:          "no workflows is an inconclusive score",
+			workflowPaths: nil,
+			err:           nil,
+			expected: scut.TestReturn{
+				Score: checker.InconclusiveResultScore,
+			},
+		},
+		{
+			name:          "untrusted checkout is a failing score",
+			workflowPaths: []string{".github/workflows/github-workflow-dangerous-pattern-untrusted-checkout.yml"},
+			err:           nil,
+			expected: scut.TestReturn{
+				Score:        checker.MinResultScore,
+				NumberOfWarn: 1,
+			},
+		},
+		{
+			name:          "script injection is a failing score",
+			workflowPaths: []string{".github/workflows/github-workflow-dangerous-pattern-untrusted-script-injection.yml"},
+			err:           nil,
+			expected: scut.TestReturn{
+				Score:        checker.MinResultScore,
+				NumberOfWarn: 1,
+			},
+		},
+		{
+			name: "only safe workflows is passing score",
+			workflowPaths: []string{
+				".github/workflows/github-workflow-dangerous-pattern-safe-trigger.yml",
+				".github/workflows/github-workflow-dangerous-pattern-trusted-checkout.yml",
+			},
+			err: nil,
+			expected: scut.TestReturn{
+				Score: checker.MaxResultScore,
+			},
+		},
+	}
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			dl := scut.TestDetailLogger{}
+			ctrl := gomock.NewController(t)
+			mockRepoClient := mockrepo.NewMockRepoClient(ctrl)
+			mockRepoClient.EXPECT().ListFiles(gomock.Any()).Return(tt.workflowPaths, nil)
+			mockRepoClient.EXPECT().GetFileReader(gomock.Any()).DoAndReturn(func(file string) (io.ReadCloser, error) {
+				return os.Open("./testdata/" + file)
+			}).AnyTimes()
+
+			req := &checker.CheckRequest{
+				Ctx:        context.Background(),
+				RepoClient: mockRepoClient,
+				Dlogger:    &dl,
+			}
+
+			result := DangerousWorkflow(req)
+			scut.ValidateTestReturn(t, tt.name, &tt.expected, &result, &dl)
+		})
+	}
+}

checks/evaluation/binary_artifacts.go

@@ -18,7 +18,7 @@ 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/freeOfUnverifiedBinaryArtifacts"
+	"github.com/ossf/scorecard/v4/probes/hasUnverifiedBinaryArtifacts"
 )
 
 // BinaryArtifacts applies the score policy for the Binary-Artifacts check.
@@ -27,21 +27,21 @@ func BinaryArtifacts(name string,
 	dl checker.DetailLogger,
 ) checker.CheckResult {
 	expectedProbes := []string{
-		freeOfUnverifiedBinaryArtifacts.Probe,
+		hasUnverifiedBinaryArtifacts.Probe,
 	}
 
 	if !finding.UniqueProbesEqual(findings, expectedProbes) {
 		e := sce.WithMessage(sce.ErrScorecardInternal, "invalid probe results")
 		return checker.CreateRuntimeErrorResult(name, e)
 	}
 
-	if findings[0].Outcome == finding.OutcomeTrue {
+	if findings[0].Outcome == finding.OutcomeFalse {
 		return checker.CreateMaxScoreResult(name, "no binaries found in the repo")
 	}
 
 	for i := range findings {
 		f := &findings[i]
-		if f.Outcome != finding.OutcomeFalse {
+		if f.Outcome != finding.OutcomeTrue {
 			continue
 		}
 		dl.Warn(&checker.LogMessage{

checks/evaluation/binary_artifacts_test.go

@@ -20,16 +20,17 @@ 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/hasUnverifiedBinaryArtifacts"
 	scut "github.com/ossf/scorecard/v4/utests"
 )
 
 // TestBinaryArtifacts tests the binary artifacts check.
 func TestBinaryArtifacts(t *testing.T) {
 	t.Parallel()
 	lineStart := uint(123)
-	falseFinding := finding.Finding{
-		Probe:   "freeOfUnverifiedBinaryArtifacts",
-		Outcome: finding.OutcomeFalse,
+	unverifiedBinaryFinding := finding.Finding{
+		Probe:   hasUnverifiedBinaryArtifacts.Probe,
+		Outcome: finding.OutcomeTrue,
 
 		Location: &finding.Location{
 			Path:      "path",
@@ -47,8 +48,8 @@ func TestBinaryArtifacts(t *testing.T) {
 			name: "no binary artifacts",
 			findings: []finding.Finding{
 				{
-					Probe:   "freeOfUnverifiedBinaryArtifacts",
-					Outcome: finding.OutcomeTrue,
+					Probe:   hasUnverifiedBinaryArtifacts.Probe,
+					Outcome: finding.OutcomeFalse,
 				},
 			},
 			result: scut.TestReturn{
@@ -58,7 +59,7 @@ func TestBinaryArtifacts(t *testing.T) {
 		{
 			name: "one binary artifact",
 			findings: []finding.Finding{
-				falseFinding,
+				unverifiedBinaryFinding,
 			},
 			result: scut.TestReturn{
 				Score:        9,
@@ -68,24 +69,8 @@ func TestBinaryArtifacts(t *testing.T) {
 		{
 			name: "two binary artifact",
 			findings: []finding.Finding{
-				{
-					Probe:   "freeOfUnverifiedBinaryArtifacts",
-					Outcome: finding.OutcomeFalse,
-					Location: &finding.Location{
-						Path:      "path",
-						Type:      finding.FileTypeBinary,
-						LineStart: &lineStart,
-					},
-				},
-				{
-					Probe:   "freeOfUnverifiedBinaryArtifacts",
-					Outcome: finding.OutcomeFalse,
-					Location: &finding.Location{
-						Path:      "path",
-						Type:      finding.FileTypeBinary,
-						LineStart: &lineStart,
-					},
-				},
+				unverifiedBinaryFinding,
+				unverifiedBinaryFinding,
 			},
 			result: scut.TestReturn{
 				Score:        8,
@@ -95,11 +80,11 @@ func TestBinaryArtifacts(t *testing.T) {
 		{
 			name: "five binary artifact",
 			findings: []finding.Finding{
-				falseFinding,
-				falseFinding,
-				falseFinding,
-				falseFinding,
-				falseFinding,
+				unverifiedBinaryFinding,
+				unverifiedBinaryFinding,
+				unverifiedBinaryFinding,
+				unverifiedBinaryFinding,
+				unverifiedBinaryFinding,
 			},
 			result: scut.TestReturn{
 				Score:        5,
@@ -109,18 +94,18 @@ func TestBinaryArtifacts(t *testing.T) {
 		{
 			name: "twelve binary artifact - ensure score doesn't drop below min",
 			findings: []finding.Finding{
-				falseFinding,
-				falseFinding,
-				falseFinding,
-				falseFinding,
-				falseFinding,
-				falseFinding,
-				falseFinding,
-				falseFinding,
-				falseFinding,
-				falseFinding,
-				falseFinding,
-				falseFinding,
+				unverifiedBinaryFinding,
+				unverifiedBinaryFinding,
+				unverifiedBinaryFinding,
+				unverifiedBinaryFinding,
+				unverifiedBinaryFinding,
+				unverifiedBinaryFinding,
+				unverifiedBinaryFinding,
+				unverifiedBinaryFinding,
+				unverifiedBinaryFinding,
+				unverifiedBinaryFinding,
+				unverifiedBinaryFinding,
+				unverifiedBinaryFinding,
 			},
 			result: scut.TestReturn{
 				Score:        checker.MinResultScore,

checks/evaluation/branch_protection_test.go

@@ -429,6 +429,7 @@ func branchFinding(probe, branch string, outcome finding.Outcome) finding.Findin
 	}
 }
 
+//nolint:gocritic // not worried about param size / efficiency since this is a test
 func withValue(f finding.Finding, k, v string) finding.Finding {
 	f.Values[k] = v
 	return f

checks/evaluation/dangerous_workflow.go

@@ -43,18 +43,12 @@ func DangerousWorkflow(name string,
 	// Log all detected dangerous workflows
 	for i := range findings {
 		f := &findings[i]
-		if f.Outcome == finding.OutcomeFalse {
+		if f.Outcome == finding.OutcomeTrue {
 			if f.Location == nil {
 				e := sce.WithMessage(sce.ErrScorecardInternal, "invalid probe results")
 				return checker.CreateRuntimeErrorResult(name, e)
 			}
-			dl.Warn(&checker.LogMessage{
-				Path:    f.Location.Path,
-				Type:    f.Location.Type,
-				Offset:  *f.Location.LineStart,
-				Text:    f.Message,
-				Snippet: *f.Location.Snippet,
-			})
+			checker.LogFinding(dl, f, checker.DetailWarn)
 		}
 	}
 
@@ -82,7 +76,7 @@ func hasDWWithUntrustedCheckout(findings []finding.Finding) bool {
 	for i := range findings {
 		f := &findings[i]
 		if f.Probe == hasDangerousWorkflowUntrustedCheckout.Probe {
-			if f.Outcome == finding.OutcomeFalse {
+			if f.Outcome == finding.OutcomeTrue {
 				return true
 			}
 		}
@@ -94,7 +88,7 @@ func hasDWWithScriptInjection(findings []finding.Finding) bool {
 	for i := range findings {
 		f := &findings[i]
 		if f.Probe == hasDangerousWorkflowScriptInjection.Probe {
-			if f.Outcome == finding.OutcomeFalse {
+			if f.Outcome == finding.OutcomeTrue {
 				return true
 			}
 		}