✨ GitLab: Documentation and cleaner errors (ossf#2821)

* Return inconclusive if there are no workflows Signed-off-by: Raghav Kaul <[email protected]> * Return inconclusive if we don't have any workflows Signed-off-by: Raghav Kaul <[email protected]> * logging fixes Signed-off-by: Raghav Kaul <[email protected]> * fix panic Signed-off-by: Raghav Kaul <[email protected]> * Update README.md Signed-off-by: Raghav Kaul <[email protected]> * skip error when getting external status checks (requires full api access) Signed-off-by: Raghav Kaul <[email protected]> * update Signed-off-by: Raghav Kaul <[email protected]> * fix dangerous workflow test Signed-off-by: Raghav Kaul <[email protected]> --------- Signed-off-by: Raghav Kaul <[email protected]> Signed-off-by: Avishay <[email protected]>
balteravishay · May 29, 2023 · 74d6973 · 74d6973
1 parent 2527357
commit 74d6973
Show file tree

Hide file tree

Showing 12 changed files with 66 additions and 31 deletions.
diff --git a/README.md b/README.md
diff --git a/checker/raw_result.go b/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.

diff --git a/checks/evaluation/contributors.go b/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, ",")),
 		})

diff --git a/checks/evaluation/dangerous_workflow.go b/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 {

diff --git a/checks/evaluation/dangerous_workflow_test.go b/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",

diff --git a/checks/evaluation/permissions/permissions.go b/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)

diff --git a/checks/permissions_test.go b/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()

diff --git a/checks/raw/dangerous_workflow.go b/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)

diff --git a/checks/raw/permissions.go b/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)

diff --git a/clients/gitlabrepo/branches.go b/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
 			}

diff --git a/clients/gitlabrepo/releases.go b/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,

diff --git a/clients/gitlabrepo/tarball.go b/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)