From b731f450b9f760350119fbab1bc0ea692f5248a4 Mon Sep 17 00:00:00 2001 From: laurentsimon <64505099+laurentsimon@users.noreply.github.com> Date: Mon, 23 Aug 2021 17:54:22 -0700 Subject: [PATCH] =?UTF-8?q?=E2=9C=A8=20Transition=20Vulnerabilities,=20Per?= =?UTF-8?q?missions,=20CI-Tests,=20Dependency-Update-Tool,=20Code-Reviews?= =?UTF-8?q?=20to=20structured=20details=20(#889)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * move other checks togit add -u * more checks * fixes --- checks/automatic_dependency_update.go | 24 +++++- checks/ci_tests.go | 20 +++-- checks/code_review.go | 24 ++++-- checks/contributors.go | 4 +- checks/permissions.go | 101 +++++++++++++++++++++++--- checks/vulnerabilities.go | 4 +- 6 files changed, 148 insertions(+), 29 deletions(-) diff --git a/checks/automatic_dependency_update.go b/checks/automatic_dependency_update.go index f4ff3dd7..4221b068 100644 --- a/checks/automatic_dependency_update.go +++ b/checks/automatic_dependency_update.go @@ -36,8 +36,12 @@ func AutomaticDependencyUpdate(c *checker.CheckRequest) checker.CheckResult { return checker.CreateRuntimeErrorResult(CheckDependencyUpdateTool, err) } if !r { - c.Dlogger.Warn("dependabot not detected") - c.Dlogger.Warn("renovatebot not detected") + c.Dlogger.Warn3(&checker.LogMessage{ + Text: "dependabot not detected", + }) + c.Dlogger.Warn3(&checker.LogMessage{ + Text: "renovatebot not detected", + }) return checker.CreateMinScoreResult(CheckDependencyUpdateTool, "no update tool detected") } @@ -51,11 +55,23 @@ func fileExists(name string, dl checker.DetailLogger, data FileCbData) (bool, er switch strings.ToLower(name) { case ".github/dependabot.yml": - dl.Info("dependabot detected : %s", name) + dl.Info3(&checker.LogMessage{ + Path: name, + Type: checker.FileTypeSource, + // Source file must have line number > 0. + Offset: 1, + Text: "dependabot detected", + }) // https://docs.renovatebot.com/configuration-options/ case ".github/renovate.json", ".github/renovate.json5", ".renovaterc.json", "renovate.json", "renovate.json5", ".renovaterc": - dl.Info("renovate detected: %s", name) + dl.Info3(&checker.LogMessage{ + Path: name, + Type: checker.FileTypeSource, + // Source file must have line number > 0. + Offset: 1, + Text: "renovate detected", + }) default: // Continue iterating. return true, nil diff --git a/checks/ci_tests.go b/checks/ci_tests.go index 8010c9a4..be38c685 100644 --- a/checks/ci_tests.go +++ b/checks/ci_tests.go @@ -90,7 +90,9 @@ func CITests(c *checker.CheckRequest) checker.CheckResult { } if !foundCI { - c.Dlogger.Debug("merged PR without CI test: %d", pr.Number) + c.Dlogger.Debug3(&checker.LogMessage{ + Text: fmt.Sprintf("merged PR without CI test: %d", pr.Number), + }) } } @@ -116,8 +118,12 @@ func prHasSuccessStatus(pr *clients.PullRequest, c *checker.CheckRequest) (bool, continue } if isTest(status.GetContext()) { - c.Dlogger.Debug("CI test found: pr: %d, context: %success, url: %success", pr.Number, - status.GetContext(), status.GetURL()) + c.Dlogger.Debug3(&checker.LogMessage{ + Path: status.GetURL(), + Type: checker.FileTypeURL, + Text: fmt.Sprintf("CI test found: pr: %d, context: %s", pr.Number, + status.GetContext()), + }) return true, nil } } @@ -145,8 +151,12 @@ func prHasSuccessfulCheck(pr *clients.PullRequest, c *checker.CheckRequest) (boo continue } if isTest(cr.GetApp().GetSlug()) { - c.Dlogger.Debug("CI test found: pr: %d, context: %success, url: %success", pr.Number, - cr.GetApp().GetSlug(), cr.GetURL()) + c.Dlogger.Debug3(&checker.LogMessage{ + Path: cr.GetURL(), + Type: checker.FileTypeURL, + Text: fmt.Sprintf("CI test found: pr: %d, context: %s", pr.Number, + cr.GetApp().GetSlug()), + }) return true, nil } } diff --git a/checks/code_review.go b/checks/code_review.go index bed1278f..46f7e2c9 100644 --- a/checks/code_review.go +++ b/checks/code_review.go @@ -74,11 +74,15 @@ func DoesCodeReview(c *checker.CheckRequest) checker.CheckResult { func selectBestScoreAndReason(s1, s2 int, r1, r2 string, dl checker.DetailLogger) (int, string) { if s1 > s2 { - dl.Info(r2) + dl.Info3(&checker.LogMessage{ + Text: r2, + }) return s1, r1 } - dl.Info(r1) + dl.Info3(&checker.LogMessage{ + Text: r1, + }) return s2, r2 } @@ -101,7 +105,9 @@ func githubCodeReview(c *checker.CheckRequest) (int, string, error) { foundApprovedReview := false for _, r := range pr.Reviews { if r.State == "APPROVED" { - c.Dlogger.Debug("found review approved pr: %d", pr.Number) + c.Dlogger.Debug3(&checker.LogMessage{ + Text: fmt.Sprintf("found review approved pr: %d", pr.Number), + }) totalReviewed++ foundApprovedReview = true break @@ -113,7 +119,9 @@ func githubCodeReview(c *checker.CheckRequest) (int, string, error) { // time on clicking the approve button. if !foundApprovedReview { if !pr.MergeCommit.AuthoredByCommitter { - c.Dlogger.Debug("found pr with committer different than author: %d", pr.Number) + c.Dlogger.Debug3(&checker.LogMessage{ + Text: fmt.Sprintf("found pr with committer different than author: %d", pr.Number), + }) totalReviewed++ } } @@ -184,7 +192,9 @@ func commitMessageHints(c *checker.CheckRequest) (int, string, error) { } } if isBot { - c.Dlogger.Debug("skip commit from bot account: %s", committer) + c.Dlogger.Debug3(&checker.LogMessage{ + Text: fmt.Sprintf("skip commit from bot account: %s", committer), + }) continue } @@ -194,7 +204,9 @@ func commitMessageHints(c *checker.CheckRequest) (int, string, error) { commitMessage := commit.Message if strings.Contains(commitMessage, "\nReviewed-on: ") && strings.Contains(commitMessage, "\nReviewed-by: ") { - c.Dlogger.Debug("Gerrit review found for commit '%s'", commit.SHA) + c.Dlogger.Debug3(&checker.LogMessage{ + Text: fmt.Sprintf("Gerrit review found for commit '%s'", commit.SHA), + }) totalReviewed++ continue } diff --git a/checks/contributors.go b/checks/contributors.go index fe55d989..905bb8c5 100644 --- a/checks/contributors.go +++ b/checks/contributors.go @@ -70,7 +70,9 @@ func Contributors(c *checker.CheckRequest) checker.CheckResult { names = append(names, c) } - c.Dlogger.Info("contributors work for: %v", strings.Join(names, ",")) + c.Dlogger.Info3(&checker.LogMessage{ + Text: fmt.Sprintf("contributors work for: %v", strings.Join(names, ",")), + }) reason := fmt.Sprintf("%d different companies found", len(companies)) return checker.CreateProportionalScoreResult(CheckContributors, reason, len(companies), numberCompaniesForTopScore) diff --git a/checks/permissions.go b/checks/permissions.go index 4c4f3dcb..dd7251b0 100644 --- a/checks/permissions.go +++ b/checks/permissions.go @@ -63,17 +63,38 @@ func validatePermission(key string, value interface{}, path string, if strings.EqualFold(val, "write") { if isPermissionOfInterest(key, ignoredPermissions) { - dl.Warn("'%v' permission set to '%v' in %v", key, val, path) + dl.Warn3(&checker.LogMessage{ + Path: path, + Type: checker.FileTypeSource, + // TODO: set line. + Offset: 1, + Text: fmt.Sprintf("'%v' permission set to '%v'", key, val), + // TODO: set Snippet. + }) recordPermissionWrite(key, pPermissions) } else { // Only log for debugging, otherwise // it may confuse users. - dl.Debug("'%v' permission set to '%v' in %v", key, val, path) + dl.Debug3(&checker.LogMessage{ + Path: path, + Type: checker.FileTypeSource, + // TODO: set line. + Offset: 1, + Text: fmt.Sprintf("'%v' permission set to '%v'", key, val), + // TODO: set Snippet. + }) } return nil } - dl.Info("'%v' permission set to '%v' in %v", key, val, path) + dl.Info3(&checker.LogMessage{ + Path: path, + Type: checker.FileTypeSource, + // TODO: set line correctly. + Offset: 1, + Text: fmt.Sprintf("'%v' permission set to '%v'", key, val), + // TODO: set Snippet. + }) return nil } @@ -113,15 +134,37 @@ func validatePermissions(permissions interface{}, path string, // Empty string is nil type. // It defaults to 'none' case nil: - dl.Info("permissions set to 'none' in %v", path) + dl.Info3(&checker.LogMessage{ + Path: path, + Type: checker.FileTypeSource, + // TODO: set line correctly. + Offset: 1, + Text: "permissions set to 'none'", + // TODO: set Snippet. + }) // String type. case string: if !strings.EqualFold(val, "read-all") && val != "" { - dl.Warn("permissions set to '%v' in %v", val, path) + dl.Warn3(&checker.LogMessage{ + Path: path, + Type: checker.FileTypeSource, + // TODO: set line correctly. + Offset: 1, + Text: fmt.Sprintf("permissions set to '%v'", val), + // TODO: set Snippet. + }) recordAllPermissionsWrite(pPermissions) return nil } - dl.Info("permission set to '%v' in %v", val, path) + + dl.Info3(&checker.LogMessage{ + Path: path, + Type: checker.FileTypeSource, + // TODO: set line correctly. + Offset: 1, + Text: fmt.Sprintf("permissions set to '%v'", val), + // TODO: set Snippet. + }) // Map type. case map[interface{}]interface{}: @@ -142,7 +185,12 @@ func validateTopLevelPermissions(config map[interface{}]interface{}, path string // Check if permissions are set explicitly. permissions, ok := config["permissions"] if !ok { - dl.Warn("no permission defined in %v", path) + dl.Warn3(&checker.LogMessage{ + Path: path, + Type: checker.FileTypeSource, + Offset: 1, + Text: "no permission defined", + }) recordAllPermissionsWrite(pdata.topLevelWritePermissions) return nil } @@ -178,7 +226,12 @@ func validateRunLevelPermissions(config map[interface{}]interface{}, path string // so only top-level read-only permissions need to be declared. permissions, ok := job["permissions"] if !ok { - dl.Debug("no permission defined in %v", path) + dl.Debug3(&checker.LogMessage{ + Path: path, + Type: checker.FileTypeSource, + Offset: 1, + Text: "no permission defined", + }) continue } err := validatePermissions(permissions, path, dl, @@ -396,10 +449,22 @@ func isSARIFUploadWorkflow(s, fp string, dl checker.DetailLogger) bool { // CodeQl run externally and SARIF file uploaded. func isSARIFUploadAction(s, fp string, dl checker.DetailLogger) bool { if strings.Contains(s, "github/codeql-action/upload-sarif@") { - dl.Debug("codeql SARIF upload workflow detected: %v", fp) + dl.Debug3(&checker.LogMessage{ + Path: fp, + Type: checker.FileTypeSource, + // TODO: set line. + Offset: 1, + Text: "codeql SARIF upload workflow detected", + // TODO: set Snippet. + }) return true } - dl.Debug("not a codeql upload SARIF workflow: %v", fp) + dl.Debug3(&checker.LogMessage{ + Path: fp, + Type: checker.FileTypeSource, + Offset: 1, + Text: "not a codeql upload SARIF workflow", + }) return false } @@ -409,10 +474,22 @@ func isSARIFUploadAction(s, fp string, dl checker.DetailLogger) bool { // https://docs.github.com/en/code-security/secure-coding/automatically-scanning-your-code-for-vulnerabilities-and-errors/configuring-code-scanning. func isCodeQlAnalysisWorkflow(s, fp string, dl checker.DetailLogger) bool { if strings.Contains(s, "github/codeql-action/analyze@") { - dl.Debug("codeql workflow detected: %v", fp) + dl.Debug3(&checker.LogMessage{ + Path: fp, + Type: checker.FileTypeSource, + // TODO: set line. + Offset: 1, + Text: "codeql workflow detected", + // TODO: set Snippet. + }) return true } - dl.Debug("not a codeql workflow: %v", fp) + dl.Debug3(&checker.LogMessage{ + Path: fp, + Type: checker.FileTypeSource, + Offset: 1, + Text: "not a codeql workflow", + }) return false } diff --git a/checks/vulnerabilities.go b/checks/vulnerabilities.go index e296d156..c979ea67 100644 --- a/checks/vulnerabilities.go +++ b/checks/vulnerabilities.go @@ -99,7 +99,9 @@ func HasUnfixedVulnerabilities(c *checker.CheckRequest) checker.CheckResult { // TODO: take severity into account. vulnIDs := osvResp.getVulnerabilities() if len(vulnIDs) > 0 { - c.Dlogger.Warn("HEAD is vulnerable to %s", strings.Join(vulnIDs, ", ")) + c.Dlogger.Warn3(&checker.LogMessage{ + Text: fmt.Sprintf("HEAD is vulnerable to %s", strings.Join(vulnIDs, ", ")), + }) return checker.CreateMinScoreResult(CheckVulnerabilities, "existing vulnerabilities detected") }