🌱 Combine hasLicenseFile and hasLicenseFileAtTopDir probes (#3955)

* delete hasLicenseFileAtTopDir probe

Signed-off-by: Spencer Schrock <sschrock@google.com>

* increase value of having a license

the old split was 6 for having a license and 3 for having it in the expected location
but 1.5 years later, and there is still no other way we detect it. So it was effectively
worth 9 points. This change makes it actually worth 9 points.

Signed-off-by: Spencer Schrock <sschrock@google.com>

* simplify logging and scoring

Signed-off-by: Spencer Schrock <sschrock@google.com>

* ensure license findings have locations

Signed-off-by: Spencer Schrock <sschrock@google.com>

* update tests to reflect new logging

Signed-off-by: Spencer Schrock <sschrock@google.com>

* match existing detail better

Signed-off-by: Spencer Schrock <sschrock@google.com>

---------

Signed-off-by: Spencer Schrock <sschrock@google.com>

checks/evaluation/license.go

@@ -20,7 +20,6 @@ import (
 	"github.com/ossf/scorecard/v4/finding"
 	"github.com/ossf/scorecard/v4/probes/hasFSFOrOSIApprovedLicense"
 	"github.com/ossf/scorecard/v4/probes/hasLicenseFile"
-	"github.com/ossf/scorecard/v4/probes/hasLicenseFileAtTopDir"
 )
 
 // License applies the score policy for the License check.
@@ -32,7 +31,6 @@ func License(name string,
 	expectedProbes := []string{
 		hasLicenseFile.Probe,
 		hasFSFOrOSIApprovedLicense.Probe,
-		hasLicenseFileAtTopDir.Probe,
 	}
 
 	if !finding.UniqueProbesEqual(findings, expectedProbes) {
@@ -45,58 +43,20 @@ func License(name string,
 	m := make(map[string]bool)
 	for i := range findings {
 		f := &findings[i]
-		switch f.Outcome {
+		if f.Outcome == finding.OutcomePositive {
-		case finding.OutcomeNotApplicable:
-			dl.Info(&checker.LogMessage{
-				Type:   finding.FileTypeSource,
-				Offset: 1,
-				Text:   f.Message,
-			})
-		case finding.OutcomePositive:
 			switch f.Probe {
 			case hasFSFOrOSIApprovedLicense.Probe:
-				dl.Info(&checker.LogMessage{
-					Type:   finding.FileTypeSource,
-					Offset: 1,
-					Path:   f.Message,
-					Text:   "FSF or OSI recognized license",
-				})
 				score += scoreProbeOnce(f.Probe, m, 1)
-			case hasLicenseFileAtTopDir.Probe:
-				dl.Info(&checker.LogMessage{
-					Type:   finding.FileTypeSource,
-					Offset: 1,
-					Path:   f.Message,
-					Text:   "License file found in expected location",
-				})
-				score += scoreProbeOnce(f.Probe, m, 3)
 			case hasLicenseFile.Probe:
-				score += scoreProbeOnce(f.Probe, m, 6)
+				score += scoreProbeOnce(f.Probe, m, 9)
 			default:
 				e := sce.WithMessage(sce.ErrScorecardInternal, "unknown probe results")
 				return checker.CreateRuntimeErrorResult(name, e)
 			}
-		case finding.OutcomeNegative:
-			switch f.Probe {
-			case hasLicenseFileAtTopDir.Probe:
-				dl.Warn(&checker.LogMessage{
-					Type:   finding.FileTypeSource,
-					Offset: 1,
-					Path:   f.Message,
-					Text:   "License file found in unexpected location",
-				})
-			case hasFSFOrOSIApprovedLicense.Probe:
-				dl.Warn(&checker.LogMessage{
-					Type:   finding.FileTypeSource,
-					Offset: 1,
-					Path:   "",
-					Text:   f.Message,
-				})
-			}
-		default:
-			continue // for linting
 		}
 	}
+	checker.LogFindings(findings, dl)
+
 	_, defined := m[hasLicenseFile.Probe]
 	if !defined {
 		if score > 0 {

checks/evaluation/license_test.go

@@ -40,10 +40,6 @@ func TestLicense(t *testing.T) {
 					Probe:   "hasFSFOrOSIApprovedLicense",
 					Outcome: finding.OutcomePositive,
 				},
-				{
-					Probe:   "hasLicenseFileAtTopDir",
-					Outcome: finding.OutcomePositive,
-				},
 			},
 			result: scut.TestReturn{
 				Score:        checker.MaxResultScore,
@@ -60,17 +56,13 @@ func TestLicense(t *testing.T) {
 					Probe:   "hasFSFOrOSIApprovedLicense",
 					Outcome: finding.OutcomeNegative,
 				},
-				{
-					Probe:   "hasLicenseFileAtTopDir",
-					Outcome: finding.OutcomeNegative,
-				},
 			},
 			result: scut.TestReturn{
 				Score:        checker.MinResultScore,
 				NumberOfWarn: 2,
 			},
 		}, {
-			name: "Has license file but not a top level or in OSI/FSF format",
+			name: "Has license file but not OSI/FSF approved",
 			findings: []finding.Finding{
 				{
 					Probe:   "hasLicenseFile",
@@ -80,14 +72,11 @@ func TestLicense(t *testing.T) {
 					Probe:   "hasFSFOrOSIApprovedLicense",
 					Outcome: finding.OutcomeNegative,
 				},
-				{
-					Probe:   "hasLicenseFileAtTopDir",
-					Outcome: finding.OutcomeNegative,
-				},
 			},
 			result: scut.TestReturn{
-				Score:        6,
+				Score:        9,
-				NumberOfWarn: 2,
+				NumberOfWarn: 1,
+				NumberOfInfo: 1,
 			},
 		}, {
 			name: "Findings missing a probe = Error",
@@ -96,10 +85,6 @@ func TestLicense(t *testing.T) {
 					Probe:   "hasLicenseFile",
 					Outcome: finding.OutcomePositive,
 				},
-				{
-					Probe:   "hasFSFOrOSIApprovedLicense",
-					Outcome: finding.OutcomeNegative,
-				},
 			},
 			result: scut.TestReturn{
 				Score: -1,
@@ -116,37 +101,12 @@ func TestLicense(t *testing.T) {
 					Probe:   "hasFSFOrOSIApprovedLicense",
 					Outcome: finding.OutcomeNegative,
 				},
-				{
-					Probe:   "hasLicenseFileAtTopDir",
-					Outcome: finding.OutcomePositive,
-				},
 			},
 			result: scut.TestReturn{
 				Score:        9,
 				NumberOfInfo: 1,
 				NumberOfWarn: 1,
 			},
-		}, {
-			name: "Has an OSI/FSF approved license but not at top level dir",
-			findings: []finding.Finding{
-				{
-					Probe:   "hasLicenseFile",
-					Outcome: finding.OutcomePositive,
-				},
-				{
-					Probe:   "hasFSFOrOSIApprovedLicense",
-					Outcome: finding.OutcomePositive,
-				},
-				{
-					Probe:   "hasLicenseFileAtTopDir",
-					Outcome: finding.OutcomeNegative,
-				},
-			},
-			result: scut.TestReturn{
-				Score:        7,
-				NumberOfInfo: 1,
-				NumberOfWarn: 1,
-			},
 		},
 	}
 	for _, tt := range tests {

checks/license_test.go

@@ -52,10 +52,11 @@ func TestLicenseFileSubdirectory(t *testing.T) {
 			name:        "Without LICENSE",
 			inputFolder: "testdata/licensedir/withoutlicense",
 			expected: scut.TestReturn{
-				Error:        nil,
+				Error:         nil,
-				Score:        checker.MinResultScore,
+				Score:         checker.MinResultScore,
-				NumberOfWarn: 0,
+				NumberOfWarn:  1,
-				NumberOfInfo: 2,
+				NumberOfInfo:  0,
+				NumberOfDebug: 1,
 			},
 			err: nil,
 		},

probes/entries.go

@@ -32,7 +32,6 @@ import (
 	"github.com/ossf/scorecard/v4/probes/hasDangerousWorkflowUntrustedCheckout"
 	"github.com/ossf/scorecard/v4/probes/hasFSFOrOSIApprovedLicense"
 	"github.com/ossf/scorecard/v4/probes/hasLicenseFile"
-	"github.com/ossf/scorecard/v4/probes/hasLicenseFileAtTopDir"
 	"github.com/ossf/scorecard/v4/probes/hasNoGitHubWorkflowPermissionUnknown"
 	"github.com/ossf/scorecard/v4/probes/hasOSVVulnerabilities"
 	"github.com/ossf/scorecard/v4/probes/hasOpenSSFBadge"
@@ -95,7 +94,6 @@ var (
 	License = []ProbeImpl{
 		hasLicenseFile.Run,
 		hasFSFOrOSIApprovedLicense.Run,
-		hasLicenseFileAtTopDir.Run,
 	}
 	Contributors = []ProbeImpl{
 		contributorsFromOrgOrCompany.Run,

probes/hasFSFOrOSIApprovedLicense/impl.go

@@ -39,7 +39,7 @@ func Run(raw *checker.RawResults) ([]finding.Finding, string, error) {
 		return nil, "", fmt.Errorf("%w: raw", uerror.ErrNil)
 	}
 
-	if raw.LicenseResults.LicenseFiles == nil || len(raw.LicenseResults.LicenseFiles) == 0 {
+	if len(raw.LicenseResults.LicenseFiles) == 0 {
 		f, err := finding.NewWith(fs, Probe,
 			"project does not have a license file", nil,
 			finding.OutcomeNotApplicable)
@@ -50,18 +50,16 @@ func Run(raw *checker.RawResults) ([]finding.Finding, string, error) {
 	}
 
 	for _, licenseFile := range raw.LicenseResults.LicenseFiles {
-		if licenseFile.LicenseInformation.Approved {
+		if !licenseFile.LicenseInformation.Approved {
-			// Store the file path in the msg
+			continue
-			msg := licenseFile.File.Path
-
-			f, err := finding.NewWith(fs, Probe,
-				msg, nil,
-				finding.OutcomePositive)
-			if err != nil {
-				return nil, Probe, fmt.Errorf("create finding: %w", err)
-			}
-			return []finding.Finding{*f}, Probe, nil
 		}
+		licenseFile := licenseFile
+		loc := licenseFile.File.Location()
+		f, err := finding.NewPositive(fs, Probe, "FSF or OSI recognized license: "+licenseFile.LicenseInformation.Name, loc)
+		if err != nil {
+			return nil, Probe, fmt.Errorf("create finding: %w", err)
+		}
+		return []finding.Finding{*f}, Probe, nil
 	}
 
 	f, err := finding.NewWith(fs, Probe,

probes/hasLicenseFile/impl.go

@@ -40,17 +40,11 @@ func Run(raw *checker.RawResults) ([]finding.Finding, string, error) {
 	}
 
 	var findings []finding.Finding
-	var outcome finding.Outcome
-	var msg string
 
 	licenseFiles := raw.LicenseResults.LicenseFiles
 
 	if len(licenseFiles) == 0 {
-		outcome = finding.OutcomeNegative
+		f, err := finding.NewNegative(fs, Probe, "project does not have a license file", nil)
-		msg = "project does not have a license file"
-		f, err := finding.NewWith(fs, Probe,
-			msg, nil,
-			outcome)
 		if err != nil {
 			return nil, Probe, fmt.Errorf("create finding: %w", err)
 		}
@@ -59,18 +53,8 @@ func Run(raw *checker.RawResults) ([]finding.Finding, string, error) {
 	} else {
 		for _, licenseFile := range licenseFiles {
 			licenseFile := licenseFile
-			loc := &finding.Location{
+			loc := licenseFile.File.Location()
-				Type:      licenseFile.File.Type,
+			f, err := finding.NewPositive(fs, Probe, "project has a license file", loc)
-				Path:      licenseFile.File.Path,
-				LineStart: &licenseFile.File.Offset,
-				LineEnd:   &licenseFile.File.EndOffset,
-				Snippet:   &licenseFile.File.Snippet,
-			}
-			msg = "project has a license file"
-			outcome = finding.OutcomePositive
-			f, err := finding.NewWith(fs, Probe,
-				msg, loc,
-				outcome)
 			if err != nil {
 				return nil, Probe, fmt.Errorf("create finding: %w", err)
 			}

probes/hasLicenseFileAtTopDir/def.yml

@@ -1,34 +0,0 @@
-# Copyright 2023 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.
-
-id: hasLicenseFileAtTopDir
-short: Check that the project has a license file
-motivation: >
-  A license can give users information about how the source code may or may not be used. The lack of a license will impede any kind of security review or audit and creates a legal risk for potential users.
-implementation: >
-  This check will detect files in the top-level directory with any combination of the following names and extensions: LICENSE, LICENCE, COPYING, COPYRIGHT and having common extensions such as .html, .txt, or .md. It will also detect these files in a directory named LICENSES in the top directory. (Files in a LICENSES directory are typically named as their SPDX license identifier followed by an appropriate file extension, as described in the [REUSE Specification](https://reuse.software/spec/).)
-outcome:
-  - If the projects license file is found at the top level, the probe returns a single OutcomePositive (1).
-  - If the projects license file is not found at the top level, the probe returns a single OutcomeNegative (0).
-remediation:
-  effort: Low
-  text:
-    - Place the license file at the top level of the project source tree.
-ecosystem:
-  languages:
-    - all
-  clients:
-    - github
-    - gitlab
-    - localdir

probes/hasLicenseFileAtTopDir/impl.go

@@ -1,88 +0,0 @@
-// Copyright 2023 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.
-
-//nolint:stylecheck
-package hasLicenseFileAtTopDir
-
-import (
-	"embed"
-	"fmt"
-
-	"github.com/ossf/scorecard/v4/checker"
-	"github.com/ossf/scorecard/v4/finding"
-	"github.com/ossf/scorecard/v4/internal/probes"
-	"github.com/ossf/scorecard/v4/probes/internal/utils/uerror"
-)
-
-func init() {
-	probes.MustRegister(Probe, Run, []probes.CheckName{probes.License})
-}
-
-//go:embed *.yml
-var fs embed.FS
-
-const Probe = "hasLicenseFileAtTopDir"
-
-func Run(raw *checker.RawResults) ([]finding.Finding, string, error) {
-	if raw == nil {
-		return nil, "", fmt.Errorf("%w: raw", uerror.ErrNil)
-	}
-
-	if raw.LicenseResults.LicenseFiles == nil || len(raw.LicenseResults.LicenseFiles) == 0 {
-		f, err := finding.NewWith(fs, Probe,
-			"project does not have a license file", nil,
-			finding.OutcomeNotApplicable)
-		if err != nil {
-			return nil, Probe, fmt.Errorf("create finding: %w", err)
-		}
-		return []finding.Finding{*f}, Probe, nil
-	}
-
-	for _, licenseFile := range raw.LicenseResults.LicenseFiles {
-		switch licenseFile.LicenseInformation.Attribution {
-		case checker.LicenseAttributionTypeAPI, checker.LicenseAttributionTypeHeuristics:
-			// both repoAPI and scorecard (not using the API) follow checks.md
-			// for a file to be found it must have been in the correct location
-			// award location points.
-
-			// Store the file path in the msg
-			msg := licenseFile.File.Path
-			f, err := finding.NewWith(fs, Probe,
-				msg, nil,
-				finding.OutcomePositive)
-			if err != nil {
-				return nil, Probe, fmt.Errorf("create finding: %w", err)
-			}
-			return []finding.Finding{*f}, Probe, nil
-
-		case checker.LicenseAttributionTypeOther:
-			msg := "License file found in unexpected location"
-			f, err := finding.NewWith(fs, Probe,
-				msg, nil,
-				finding.OutcomeNegative)
-			if err != nil {
-				return nil, Probe, fmt.Errorf("create finding: %w", err)
-			}
-			return []finding.Finding{*f}, Probe, nil
-		}
-	}
-
-	f, err := finding.NewWith(fs, Probe,
-		"Did not find the license file at the expected location.", nil,
-		finding.OutcomeNegative)
-	if err != nil {
-		return nil, Probe, fmt.Errorf("create finding: %w", err)
-	}
-	return []finding.Finding{*f}, Probe, nil
-}

probes/hasLicenseFileAtTopDir/impl_test.go

@@ -1,150 +0,0 @@
-// Copyright 2023 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.
-
-//nolint:stylecheck
-package hasLicenseFileAtTopDir
-
-import (
-	"testing"
-
-	"github.com/google/go-cmp/cmp"
-	"github.com/google/go-cmp/cmp/cmpopts"
-
-	"github.com/ossf/scorecard/v4/checker"
-	"github.com/ossf/scorecard/v4/finding"
-	"github.com/ossf/scorecard/v4/probes/internal/utils/test"
-)
-
-func Test_Run(t *testing.T) {
-	t.Parallel()
-	//nolint:govet
-	tests := []struct {
-		name     string
-		raw      *checker.RawResults
-		outcomes []finding.Outcome
-		err      error
-	}{
-		{
-			name: "License file found and correct attribution 1",
-			raw: &checker.RawResults{
-				LicenseResults: checker.LicenseData{
-					LicenseFiles: []checker.LicenseFile{
-						{
-							File: checker.File{
-								Path: "LICENSE.md",
-							},
-							LicenseInformation: checker.License{
-								Attribution: checker.LicenseAttributionTypeAPI,
-							},
-						},
-					},
-				},
-			},
-			outcomes: []finding.Outcome{
-				finding.OutcomePositive,
-			},
-		},
-		{
-			name: "License file not found and outcome should be OutcomeNotApplicable",
-			raw: &checker.RawResults{
-				LicenseResults: checker.LicenseData{
-					LicenseFiles: []checker.LicenseFile{},
-				},
-			},
-			outcomes: []finding.Outcome{
-				finding.OutcomeNotApplicable,
-			},
-		},
-		{
-			name: "License file found and correct attribution 2",
-			raw: &checker.RawResults{
-				LicenseResults: checker.LicenseData{
-					LicenseFiles: []checker.LicenseFile{
-						{
-							File: checker.File{
-								Path: "LICENSE.md",
-							},
-							LicenseInformation: checker.License{
-								Attribution: checker.LicenseAttributionTypeHeuristics,
-							},
-						},
-					},
-				},
-			},
-			outcomes: []finding.Outcome{
-				finding.OutcomePositive,
-			},
-		},
-		{
-			name: "License file found and wrong attribution",
-			raw: &checker.RawResults{
-				LicenseResults: checker.LicenseData{
-					LicenseFiles: []checker.LicenseFile{
-						{
-							File: checker.File{
-								Path: "LICENSE.md",
-							},
-							LicenseInformation: checker.License{
-								Attribution: "wrong_attribution",
-							},
-						},
-					},
-				},
-			},
-			outcomes: []finding.Outcome{
-				finding.OutcomeNegative,
-			},
-		},
-		{
-			name: "nil license files and outcome should be OutcomeNotApplicable",
-			raw: &checker.RawResults{
-				LicenseResults: checker.LicenseData{
-					LicenseFiles: nil,
-				},
-			},
-			outcomes: []finding.Outcome{
-				finding.OutcomeNotApplicable,
-			},
-		},
-		{
-			name: "0 license files and outcome should be OutcomeNotApplicable",
-			raw: &checker.RawResults{
-				LicenseResults: checker.LicenseData{
-					LicenseFiles: []checker.LicenseFile{},
-				},
-			},
-			outcomes: []finding.Outcome{
-				finding.OutcomeNotApplicable,
-			},
-		},
-	}
-	for _, tt := range tests {
-		tt := tt // Re-initializing variable so it is not changed while executing the closure below
-		t.Run(tt.name, func(t *testing.T) {
-			t.Parallel()
-
-			findings, s, err := Run(tt.raw)
-			if !cmp.Equal(tt.err, err, cmpopts.EquateErrors()) {
-				t.Errorf("mismatch (-want +got):\n%s", cmp.Diff(tt.err, err, cmpopts.EquateErrors()))
-			}
-			if err != nil {
-				return
-			}
-			if diff := cmp.Diff(Probe, s); diff != "" {
-				t.Errorf("mismatch (-want +got):\n%s", diff)
-			}
-			test.AssertOutcomes(t, findings, tt.outcomes)
-		})
-	}
-}