Merge branch 'main' into fix/osv-scanner-panics

.github/workflows/codeql-analysis.yml

@@ -62,7 +62,7 @@ jobs:
     # Initializes the CodeQL tools for scanning.
     - name: Initialize CodeQL
 
-      uses: github/codeql-action/init@7df0ce34898d659f95c0c4a09eaa8d4e32ee64db # v1
+      uses: github/codeql-action/init@b2c19fb9a2a485599ccf4ed5d65527d94bc57226 # v1
       with:
         languages: ${{ matrix.language }}
         queries: +security-extended
@@ -74,7 +74,7 @@ jobs:
     # Autobuild attempts to build any compiled languages  (C/C++, C#, or Java).
     # If this step fails, then you should remove it and run the build manually (see below)
     - name: Autobuild
-      uses: github/codeql-action/autobuild@7df0ce34898d659f95c0c4a09eaa8d4e32ee64db # v1
+      uses: github/codeql-action/autobuild@b2c19fb9a2a485599ccf4ed5d65527d94bc57226 # v1
 
     # ℹ️ Command-line programs to run using the OS shell.
     # 📚 https://git.io/JvXDl
@@ -88,4 +88,4 @@ jobs:
     #   make release
 
     - name: Perform CodeQL Analysis
-      uses: github/codeql-action/analyze@7df0ce34898d659f95c0c4a09eaa8d4e32ee64db # v1
+      uses: github/codeql-action/analyze@b2c19fb9a2a485599ccf4ed5d65527d94bc57226 # v1

.github/workflows/scorecard-analysis.yml

@@ -47,6 +47,6 @@ jobs:
           retention-days: 5
 
       - name: "Upload SARIF results"
-        uses: github/codeql-action/upload-sarif@7df0ce34898d659f95c0c4a09eaa8d4e32ee64db # v1
+        uses: github/codeql-action/upload-sarif@b2c19fb9a2a485599ccf4ed5d65527d94bc57226 # v1
         with:
           sarif_file: results.sarif

checker/raw_result.go

@@ -315,7 +315,8 @@ const (
 // DangerousWorkflowData contains raw results
 // for dangerous workflow check.
 type DangerousWorkflowData struct {
-	Workflows []DangerousWorkflow
+	Workflows    []DangerousWorkflow
+	NumWorkflows int
 }
 
 // DangerousWorkflow represents a dangerous workflow.
@@ -334,6 +335,7 @@ type WorkflowJob struct {
 // TokenPermissionsData represents data about a permission failure.
 type TokenPermissionsData struct {
 	TokenPermissions []TokenPermission
+	NumTokens        int
 }
 
 // PermissionLocation represents a declaration type.

checks/evaluation/ci_tests.go

@@ -62,7 +62,7 @@ func CITests(name string, c *checker.CITestData, dl checker.DetailLogger) checke
 			// Log message says commit, but really we only care about PRs, and
 			// use only one commit (branch HEAD) to refer to all commits in a PR
 			dl.Debug(&checker.LogMessage{
-				Text: fmt.Sprintf("merged PR without CI test at HEAD: %s", r.HeadSHA),
+				Text: fmt.Sprintf("merged PR %d without CI test at HEAD: %s", r.PullRequestNumber, r.HeadSHA),
 			})
 		}
 	}

checks/evaluation/code_review.go

@@ -16,6 +16,7 @@ package evaluation
 
 import (
 	"fmt"
+	"strings"
 
 	"github.com/ossf/scorecard/v4/checker"
 	sce "github.com/ossf/scorecard/v4/errors"
@@ -46,6 +47,8 @@ func CodeReview(name string, dl checker.DetailLogger, r *checker.CodeReviewData)
 	foundBotReviewActivity := false
 	nUnreviewedHumanChanges := 0
 	nUnreviewedBotChanges := 0
+
+	var unreviewedHumanRevisionIDs, unreviewedBotRevisionIDs []string
 	for i := range r.DefaultBranchChangesets {
 		cs := &r.DefaultBranchChangesets[i]
 		isReviewed := reviewScoreForChangeset(cs) >= changesReviewed
@@ -58,11 +61,26 @@ func CodeReview(name string, dl checker.DetailLogger, r *checker.CodeReviewData)
 			foundHumanReviewActivity = true
 		case !isReviewed && isBotCommit:
 			nUnreviewedBotChanges += 1
+			unreviewedBotRevisionIDs = append(unreviewedBotRevisionIDs, cs.RevisionID)
 		case !isReviewed && !isBotCommit:
 			nUnreviewedHumanChanges += 1
+			unreviewedHumanRevisionIDs = append(unreviewedHumanRevisionIDs, cs.RevisionID)
 		}
 	}
 
+	// Let's include non-compliant revision IDs in details
+	if len(unreviewedHumanRevisionIDs) > 0 {
+		dl.Debug(&checker.LogMessage{
+			Text: fmt.Sprintf("List of revision IDs not reviewed by humans:%s", strings.Join(unreviewedHumanRevisionIDs, ",")),
+		})
+	}
+
+	if len(unreviewedBotRevisionIDs) > 0 {
+		dl.Debug(&checker.LogMessage{
+			Text: fmt.Sprintf("List of bot revision IDs not reviewed by humans:%s", strings.Join(unreviewedBotRevisionIDs, ",")),
+		})
+	}
+
 	if foundBotReviewActivity && !foundHumanReviewActivity {
 		reason := fmt.Sprintf("found no human review activity in the last %v changesets", len(r.DefaultBranchChangesets))
 		return checker.CreateInconclusiveResult(name, reason)

checks/evaluation/code_review_test.go

@@ -50,7 +50,8 @@ func TestCodeReview(t *testing.T) {
 		{
 			name: "NoReviews",
 			expected: scut.TestReturn{
-				Score: checker.MinResultScore,
+				Score:         checker.MinResultScore,
+				NumberOfDebug: 1,
 			},
 			rawData: &checker.CodeReviewData{
 				DefaultBranchChangesets: []checker.Changeset{
@@ -66,7 +67,8 @@ func TestCodeReview(t *testing.T) {
 		{
 			name: "Unreviewed human and bot changes",
 			expected: scut.TestReturn{
-				Score: checker.MinResultScore,
+				Score:         checker.MinResultScore,
+				NumberOfDebug: 1,
 			},
 			rawData: &checker.CodeReviewData{
 				DefaultBranchChangesets: []checker.Changeset{
@@ -82,7 +84,8 @@ func TestCodeReview(t *testing.T) {
 		{
 			name: "all human changesets reviewed, missing review on bot changeset",
 			expected: scut.TestReturn{
-				Score: 7,
+				Score:         7,
+				NumberOfDebug: 1,
 			},
 			rawData: &checker.CodeReviewData{
 				DefaultBranchChangesets: []checker.Changeset{
@@ -147,7 +150,8 @@ func TestCodeReview(t *testing.T) {
 		{
 			name: "bot commits only",
 			expected: scut.TestReturn{
-				Score: checker.InconclusiveResultScore,
+				Score:         checker.InconclusiveResultScore,
+				NumberOfDebug: 1,
 			},
 			rawData: &checker.CodeReviewData{
 				DefaultBranchChangesets: []checker.Changeset{

checks/evaluation/contributors.go

@@ -60,7 +60,7 @@ func Contributors(name string, dl checker.DetailLogger,
 
 	sort.Strings(names)
 
-	if len(name) > 0 {
+	if len(names) > 0 {
 		dl.Info(&checker.LogMessage{
 			Text: fmt.Sprintf("contributors work for %v", strings.Join(names, ",")),
 		})

checks/evaluation/dangerous_workflow.go

@@ -30,6 +30,10 @@ func DangerousWorkflow(name string, dl checker.DetailLogger,
 		return checker.CreateRuntimeErrorResult(name, e)
 	}
 
+	if r.NumWorkflows == 0 {
+		return checker.CreateInconclusiveResult(name, "no workflows found")
+	}
+
 	for _, e := range r.Workflows {
 		var text string
 		switch e.Type {

checks/evaluation/dangerous_workflow_test.go

@@ -43,7 +43,23 @@ func TestDangerousWorkflow(t *testing.T) {
 				r:    &checker.DangerousWorkflowData{},
 			},
 			want: checker.CheckResult{
-				Score:   10,
+				Score:   checker.InconclusiveResultScore,
+				Reason:  "no workflows found",
+				Version: 2,
+				Name:    "DangerousWorkflow",
+			},
+		},
+		{
+			name: "DangerousWorkflow - found workflows, none dangerous",
+			args: args{
+				name: "DangerousWorkflow",
+				dl:   &scut.TestDetailLogger{},
+				r: &checker.DangerousWorkflowData{
+					NumWorkflows: 5,
+				},
+			},
+			want: checker.CheckResult{
+				Score:   checker.MaxResultScore,
 				Reason:  "no dangerous workflow patterns detected",
 				Version: 2,
 				Name:    "DangerousWorkflow",
@@ -55,6 +71,7 @@ func TestDangerousWorkflow(t *testing.T) {
 				name: "DangerousWorkflow",
 				dl:   &scut.TestDetailLogger{},
 				r: &checker.DangerousWorkflowData{
+					NumWorkflows: 1,
 					Workflows: []checker.DangerousWorkflow{
 						{
 							Type: checker.DangerousWorkflowUntrustedCheckout,
@@ -82,6 +99,7 @@ func TestDangerousWorkflow(t *testing.T) {
 				name: "DangerousWorkflow",
 				dl:   &scut.TestDetailLogger{},
 				r: &checker.DangerousWorkflowData{
+					NumWorkflows: 1,
 					Workflows: []checker.DangerousWorkflow{
 						{
 							Type: checker.DangerousWorkflowScriptInjection,
@@ -109,6 +127,7 @@ func TestDangerousWorkflow(t *testing.T) {
 				name: "DangerousWorkflow",
 				dl:   &scut.TestDetailLogger{},
 				r: &checker.DangerousWorkflowData{
+					NumWorkflows: 1,
 					Workflows: []checker.DangerousWorkflow{
 						{
 							Type: "foobar",

checks/evaluation/permissions/permissions.go

@@ -40,6 +40,10 @@ func TokenPermissions(name string, c *checker.CheckRequest, r *checker.TokenPerm
 		return checker.CreateRuntimeErrorResult(name, e)
 	}
 
+	if r.NumTokens == 0 {
+		return checker.CreateInconclusiveResult(name, "no github tokens found")
+	}
+
 	score, err := applyScorePolicy(r, c)
 	if err != nil {
 		return checker.CreateRuntimeErrorResult(name, err)

checks/permissions_test.go

@@ -240,9 +240,9 @@ func TestGithubTokenPermissions(t *testing.T) {
 			filenames: []string{"./testdata/script.sh"},
 			expected: scut.TestReturn{
 				Error:         nil,
-				Score:         checker.MaxResultScore,
+				Score:         checker.InconclusiveResultScore,
 				NumberOfWarn:  0,
-				NumberOfInfo:  2,
+				NumberOfInfo:  0,
 				NumberOfDebug: 0,
 			},
 		},
@@ -386,7 +386,7 @@ func TestGithubTokenPermissions(t *testing.T) {
 
 			ctrl := gomock.NewController(t)
 			mockRepo := mockrepo.NewMockRepoClient(ctrl)
-			mockRepo.EXPECT().GetDefaultBranchName().Return("main", nil)
+			mockRepo.EXPECT().GetDefaultBranchName().Return("main", nil).AnyTimes()
 
 			main := "main"
 			mockRepo.EXPECT().URI().Return("github.com/ossf/scorecard").AnyTimes()

checks/raw/dangerous_workflow.go

@@ -102,6 +102,8 @@ var validateGitHubActionWorkflowPatterns fileparser.DoWhileTrueOnFileContent = f
 		return true, nil
 	}
 
+	pdata.NumWorkflows += 1
+
 	workflow, errs := actionlint.Parse(content)
 	if len(errs) > 0 && workflow == nil {
 		return false, fileparser.FormatActionlintError(errs)

checks/raw/permissions.go

@@ -84,6 +84,8 @@ var validateGitHubActionTokenPermissions fileparser.DoWhileTrueOnFileContent = f
 		return true, nil
 	}
 
+	pdata.results.NumTokens += 1
+
 	workflow, errs := actionlint.Parse(content)
 	if len(errs) > 0 && workflow == nil {
 		return false, fileparser.FormatActionlintError(errs)

checks/sast.go

@@ -50,7 +50,7 @@ func init() {
 
 // SAST runs SAST check.
 func SAST(c *checker.CheckRequest) checker.CheckResult {
-	sastScore, sastErr := sastToolInCheckRuns(c)
+	sastScore, nonCompliantPRs, sastErr := sastToolInCheckRuns(c)
 	if sastErr != nil {
 		return checker.CreateRuntimeErrorResult(CheckSAST, sastErr)
 	}
@@ -96,6 +96,9 @@ func SAST(c *checker.CheckRequest) checker.CheckResult {
 		case codeQlScore == checker.MaxResultScore:
 			const sastWeight = 3
 			const codeQlWeight = 7
+			c.Dlogger.Debug(&checker.LogMessage{
+				Text: getNonCompliantPRMessage(nonCompliantPRs),
+			})
 			score := checker.AggregateScoresWithWeight(map[int]int{sastScore: sastWeight, codeQlScore: codeQlWeight})
 			return checker.CreateResultWithScore(CheckSAST, "SAST tool detected but not run on all commmits", score)
 		default:
@@ -117,6 +120,9 @@ func SAST(c *checker.CheckRequest) checker.CheckResult {
 			return checker.CreateMaxScoreResult(CheckSAST, "SAST tool is run on all commits")
 		}
 
+		c.Dlogger.Debug(&checker.LogMessage{
+			Text: getNonCompliantPRMessage(nonCompliantPRs),
+		})
 		return checker.CreateResultWithScore(CheckSAST,
 			checker.NormalizeReason("SAST tool is not run on all commits", sastScore), sastScore)
 	}
@@ -125,15 +131,16 @@ func SAST(c *checker.CheckRequest) checker.CheckResult {
 	return checker.CreateRuntimeErrorResult(CheckSAST, sce.WithMessage(sce.ErrScorecardInternal, "contact team"))
 }
 
-func sastToolInCheckRuns(c *checker.CheckRequest) (int, error) {
+func sastToolInCheckRuns(c *checker.CheckRequest) (int, map[int]int, error) {
 	commits, err := c.RepoClient.ListCommits()
 	if err != nil {
-		return checker.InconclusiveResultScore,
+		return checker.InconclusiveResultScore, nil,
 			sce.WithMessage(sce.ErrScorecardInternal, fmt.Sprintf("RepoClient.ListCommits: %v", err))
 	}
 
 	totalMerged := 0
 	totalTested := 0
+	nonCompliantPRs := make(map[int]int)
 	for i := range commits {
 		pr := commits[i].AssociatedMergeRequest
 		// TODO(#575): We ignore associated PRs if Scorecard is being run on a fork
@@ -142,9 +149,10 @@ func sastToolInCheckRuns(c *checker.CheckRequest) (int, error) {
 			continue
 		}
 		totalMerged++
+		checked := false
 		crs, err := c.RepoClient.ListCheckRunsForRef(pr.HeadSHA)
 		if err != nil {
-			return checker.InconclusiveResultScore,
+			return checker.InconclusiveResultScore, nil,
 				sce.WithMessage(sce.ErrScorecardInternal, fmt.Sprintf("Client.Checks.ListCheckRunsForRef: %v", err))
 		}
 		// Note: crs may be `nil`: in this case
@@ -163,15 +171,19 @@ func sastToolInCheckRuns(c *checker.CheckRequest) (int, error) {
 					Text: fmt.Sprintf("tool detected: %v", cr.App.Slug),
 				})
 				totalTested++
+				checked = true
 				break
 			}
 		}
+		if !checked {
+			nonCompliantPRs[pr.Number] = pr.Number
+		}
 	}
 	if totalMerged == 0 {
 		c.Dlogger.Warn(&checker.LogMessage{
 			Text: "no pull requests merged into dev branch",
 		})
-		return checker.InconclusiveResultScore, nil
+		return checker.InconclusiveResultScore, nil, nil
 	}
 
 	if totalTested == totalMerged {
@@ -184,7 +196,7 @@ func sastToolInCheckRuns(c *checker.CheckRequest) (int, error) {
 		})
 	}
 
-	return checker.CreateProportionalScore(totalTested, totalMerged), nil
+	return checker.CreateProportionalScore(totalTested, totalMerged), nonCompliantPRs, nil
 }
 
 func codeQLInCheckDefinitions(c *checker.CheckRequest) (int, error) {
@@ -366,3 +378,14 @@ func findLine(content, data []byte) (uint, error) {
 
 	return 0, nil
 }
+
+func getNonCompliantPRMessage(intMap map[int]int) string {
+	var sb strings.Builder
+	for _, value := range intMap {
+		if len(sb.String()) != 0 {
+			sb.WriteString(", ")
+		}
+		sb.WriteString(fmt.Sprintf("%d", value))
+	}
+	return fmt.Sprintf("List of pull requests without CI test: %s", sb.String())
+}

clients/gitlabrepo/branches.go

@@ -71,7 +71,8 @@ func (handler *branchesHandler) setup() error {
 
 			projectStatusChecks, resp, err := handler.glClient.ExternalStatusChecks.ListProjectStatusChecks(
 				handler.repourl.project, &gitlab.ListOptions{})
-			if err != nil && resp.StatusCode != 404 {
+			if (err != nil || resp.StatusCode != 404) &&
+				resp.StatusCode != 401 { // 401: permissions. pass token authorization issues silently
 				handler.errSetup = fmt.Errorf("request for external status checks failed with error %w", err)
 				return
 			}

clients/gitlabrepo/releases.go

@@ -70,9 +70,12 @@ func releasesFrom(data []*gitlab.Release) []clients.Release {
 	for _, r := range data {
 		release := clients.Release{
 			TagName:         r.TagName,
-			URL:             r.Assets.Links[0].DirectAssetURL,
 			TargetCommitish: r.CommitPath,
 		}
+		if len(r.Assets.Links) > 0 {
+			release.URL = r.Assets.Links[0].DirectAssetURL
+		}
+
 		for _, a := range r.Assets.Sources {
 			release.Assets = append(release.Assets, clients.ReleaseAsset{
 				Name: a.Format,

clients/gitlabrepo/tarball.go

@@ -244,8 +244,6 @@ func (handler *tarballHandler) getFileContent(filename string) ([]byte, error) {
 		fmt.Printf("err: %v\n", err)
 		return nil, fmt.Errorf("error during tarballHandler.setup: %w", err)
 	}
-	fmt.Printf("handler.tempDir: %v\n", handler.tempDir)
-	fmt.Printf("filename: %v\n", filename)
 	content, err := os.ReadFile(filepath.Join(handler.tempDir, filename))
 	if err != nil {
 		return content, fmt.Errorf("os.ReadFile: %w", err)