add extra field to indicate the outcome a probe should show remediati…

…on for

Signed-off-by: Spencer Schrock <[email protected]>

finding/finding.go

@@ -81,6 +81,9 @@ type Finding struct {
 	Probe       string            `json:"probe"`
 	Message     string            `json:"message"`
 	Outcome     Outcome           `json:"outcome"`
+
+	// Expected bad outcome, used to determine if Remediation should be set
+	badOutcome Outcome
 }
 
 // AnonymousFinding is a finding without a corresponding probe ID.
@@ -101,6 +104,7 @@ func FromBytes(content []byte, probeID string) (*Finding, error) {
 		Probe:       p.ID,
 		Outcome:     OutcomeFalse,
 		Remediation: p.Remediation,
+		badOutcome:  p.RemediateOnOutcome,
 	}
 	return f, nil
 }
@@ -116,6 +120,7 @@ func New(loc embed.FS, probeID string) (*Finding, error) {
 		Probe:       p.ID,
 		Outcome:     OutcomeFalse,
 		Remediation: p.Remediation,
+		badOutcome:  p.RemediateOnOutcome,
 	}
 	return f, nil
 }
@@ -216,12 +221,10 @@ func (f *Finding) WithPatch(patch *string) *Finding {
 
 // WithOutcome adds an outcome to an existing finding.
 // No copy is made.
-// WARNING: this function should be called at most once for a finding.
 func (f *Finding) WithOutcome(o Outcome) *Finding {
 	f.Outcome = o
-	// Currently only false probes have remediations.
-	// TODO(#3654) this is a temporary mechanical conversion.
-	if o != OutcomeFalse {
+	// only bad outcomes need remediation, clear if unneeded
+	if o != f.badOutcome {
 		f.Remediation = nil
 	}
 

finding/finding_test.go

@@ -190,7 +190,7 @@ func Test_FromBytes(t *testing.T) {
 			if tt.outcome != nil {
 				r = r.WithOutcome(*tt.outcome)
 			}
-			if diff := cmp.Diff(*tt.finding, *r); diff != "" {
+			if diff := cmp.Diff(*tt.finding, *r, cmpopts.IgnoreUnexported(Finding{})); diff != "" {
 				t.Errorf("mismatch (-want +got):\n%s", diff)
 			}
 		})

finding/probe.go

@@ -68,21 +68,23 @@ var supportedClients = map[string]bool{
 }
 
 type yamlProbe struct {
-	ID             string          `yaml:"id"`
-	Short          string          `yaml:"short"`
-	Motivation     string          `yaml:"motivation"`
-	Implementation string          `yaml:"implementation"`
-	Ecosystem      yamlEcosystem   `yaml:"ecosystem"`
-	Remediation    yamlRemediation `yaml:"remediation"`
+	ID                 string          `yaml:"id"`
+	Short              string          `yaml:"short"`
+	Motivation         string          `yaml:"motivation"`
+	Implementation     string          `yaml:"implementation"`
+	Ecosystem          yamlEcosystem   `yaml:"ecosystem"`
+	Remediation        yamlRemediation `yaml:"remediation"`
+	RemediateOnOutcome Outcome         `yaml:"remediateOnOutcome"`
 }
 
 //nolint:govet
 type Probe struct {
-	ID             string
-	Short          string
-	Motivation     string
-	Implementation string
-	Remediation    *Remediation
+	ID                 string
+	Short              string
+	Motivation         string
+	Implementation     string
+	Remediation        *Remediation
+	RemediateOnOutcome Outcome
 }
 
 func probeFromBytes(content []byte, probeID string) (*Probe, error) {
@@ -105,6 +107,7 @@ func probeFromBytes(content []byte, probeID string) (*Probe, error) {
 			Markdown: strings.Join(r.Remediation.Markdown, "\n"),
 			Effort:   r.Remediation.Effort,
 		},
+		RemediateOnOutcome: r.RemediateOnOutcome,
 	}, nil
 }
 
@@ -127,6 +130,9 @@ func validate(r *yamlProbe, probeID string) error {
 	if err := validateEcosystem(r.Ecosystem); err != nil {
 		return err
 	}
+	if err := validateBadOutcome(r.RemediateOnOutcome); err != nil {
+		return err
+	}
 	return nil
 }
 
@@ -157,6 +163,15 @@ func validateEcosystem(r yamlEcosystem) error {
 	return nil
 }
 
+func validateBadOutcome(o Outcome) error {
+	switch o {
+	case OutcomeTrue, OutcomeFalse, OutcomeNotApplicable, OutcomeNotAvailable, OutcomeNotSupported, OutcomeError:
+		return nil
+	default:
+		return fmt.Errorf("%w: unknown outcome: %v", errInvalid, o)
+	}
+}
+
 func validateSupportedLanguages(r yamlEcosystem) error {
 	for _, lang := range r.Languages {
 		switch clients.LanguageName(lang) {
@@ -189,7 +204,7 @@ func parseFromYAML(content []byte) (*yamlProbe, error) {
 
 	err := yaml.Unmarshal(content, &r)
 	if err != nil {
-		return nil, fmt.Errorf("%w: %w", errInvalid, err)
+		return nil, fmt.Errorf("unable to parse yaml: %w", err)
 	}
 	return &r, nil
 }

finding/probe_test.go

@@ -46,6 +46,7 @@ func Test_probeFromBytes(t *testing.T) {
 					Markdown: "step1\nstep2 [google.com](https://www.google.com/something)",
 					Effort:   RemediationEffortLow,
 				},
+				RemediateOnOutcome: OutcomeFalse,
 			},
 		},
 		{

finding/testdata/all-fields.yml

@@ -6,6 +6,7 @@ motivation: >
 implementation: >
   impl1
   impl2
+remediateOnOutcome: False
 remediation:
   effort: Low
   text:

finding/testdata/effort-high.yml

@@ -6,6 +6,7 @@ motivation: >
 implementation: >
   line1
   line2
+remediateOnOutcome: False
 remediation:
   effort: High
   text:

finding/testdata/effort-low.yml

@@ -6,6 +6,7 @@ motivation: >
 implementation: >
   line1
   line2
+remediateOnOutcome: False
 remediation:
   effort: Low
   text:

finding/testdata/metadata-variables.yml

@@ -6,6 +6,7 @@ motivation: >
 implementation: >
   line1
   line2
+remediateOnOutcome: False
 remediation:
   effort: Low
   text:

pkg/scorecard_test.go

@@ -320,9 +320,10 @@ func TestExperimentalRunProbes(t *testing.T) {
 			}
 			ignoreRemediationText := cmpopts.IgnoreFields(finding.Remediation{}, "Text", "Markdown")
 			ignoreDate := cmpopts.IgnoreFields(ScorecardResult{}, "Date")
-			if !cmp.Equal(got, tt.want, ignoreDate, ignoreRemediationText) {
+			ignoreUnexported := cmpopts.IgnoreUnexported(finding.Finding{})
+			if !cmp.Equal(got, tt.want, ignoreDate, ignoreRemediationText, ignoreUnexported) {
 				t.Errorf("expected %v, got %v", got, cmp.Diff(tt.want, got, ignoreDate,
-					ignoreRemediationText))
+					ignoreRemediationText, ignoreUnexported))
 			}
 		})
 	}

probes/dependencyUpdateToolConfigured/impl_test.go

@@ -155,7 +155,7 @@ func TestRun_Detailed(t *testing.T) {
 			if err != nil {
 				t.Fatalf("unexpected error: %v", err)
 			}
-			if diff := cmp.Diff(findings, tt.expected); diff != "" {
+			if diff := cmp.Diff(findings, tt.expected, cmpopts.IgnoreUnexported(finding.Finding{})); diff != "" {
 				t.Error(diff)
 			}
 		})

probes/fuzzed/impl_test.go

@@ -174,7 +174,7 @@ func TestRun_Detailed(t *testing.T) {
 			if err != nil {
 				t.Fatalf("unexpected error: %v", err)
 			}
-			if diff := cmp.Diff(findings, tt.expected); diff != "" {
+			if diff := cmp.Diff(findings, tt.expected, cmpopts.IgnoreUnexported(finding.Finding{})); diff != "" {
 				t.Error(diff)
 			}
 		})

probes/sastToolRunsOnAllCommits/impl_test.go

@@ -112,8 +112,9 @@ func Test_Run(t *testing.T) {
 				t.Errorf("mismatch (-want +got):\n%s", diff)
 			}
 			test.AssertOutcomes(t, findings, tt.outcomes)
-			if !cmp.Equal(tt.expectedFindings, findings, cmpopts.EquateErrors()) {
-				t.Errorf("mismatch (-want +got):\n%s", cmp.Diff(tt.expectedFindings, findings, cmpopts.EquateErrors()))
+			diff := cmp.Diff(tt.expectedFindings, findings, cmpopts.EquateErrors(), cmpopts.IgnoreUnexported(finding.Finding{}))
+			if diff != "" {
+				t.Errorf("mismatch (-want +got):\n%s", diff)
 			}
 		})
 	}

probes/testsRunInCI/impl_test.go

@@ -136,7 +136,7 @@ func Test_Run(t *testing.T) {
 			for i := range tt.findings {
 				outcome := &tt.findings[i]
 				f := &findings[i]
-				if diff := cmp.Diff(*outcome, f); diff != "" {
+				if diff := cmp.Diff(*outcome, f, cmpopts.IgnoreUnexported(finding.Finding{})); diff != "" {
 					t.Errorf("mismatch (-want +got):\n%s", diff)
 				}
 			}