From 2e3e505a8c56c8df3b106d63a9db9dd8f2df7309 Mon Sep 17 00:00:00 2001 From: Azeem Shaikh Date: Fri, 11 Feb 2022 15:48:58 -0800 Subject: [PATCH] Simplify DetailLogger interface (#1628) Co-authored-by: Azeem Shaikh --- checker/check_runner.go | 9 +- checker/detail_logger.go | 14 +-- checker/detail_logger_impl.go | 45 ++----- checks/ci_tests.go | 11 +- checks/contributors.go | 5 +- checks/dangerous_workflow.go | 33 ++--- checks/evaluation/binary_artifacts.go | 7 +- checks/evaluation/branch_protection.go | 18 ++- checks/evaluation/code_review.go | 29 +++-- checks/evaluation/dependency_update_tool.go | 17 +-- checks/evaluation/security_policy.go | 9 +- checks/evaluation/vulnerabilities.go | 5 +- checks/license.go | 9 +- checks/packaging.go | 49 ++++---- checks/permissions.go | 132 +++++++++++--------- checks/pinned_dependencies.go | 31 +++-- checks/sast.go | 45 ++++--- checks/shell_download_validate.go | 22 ++-- checks/signed_releases.go | 28 +++-- pkg/common.go | 2 +- utests/utlib.go | 37 ++---- 21 files changed, 299 insertions(+), 258 deletions(-) diff --git a/checker/check_runner.go b/checker/check_runner.go index e7099a44..7c4e7ad3 100644 --- a/checker/check_runner.go +++ b/checker/check_runner.go @@ -87,15 +87,18 @@ func (r *Runner) Run(ctx context.Context, c Check) CheckResult { checkRequest.Dlogger = &l res = c.Fn(&checkRequest) if res.Error2 != nil && errors.Is(res.Error2, sce.ErrRepoUnreachable) { - checkRequest.Dlogger.Warn("%v", res.Error2) + checkRequest.Dlogger.Warn(&LogMessage{ + Text: fmt.Sprintf("%v", res.Error2), + }) continue } break } // Set details. - res.Details2 = l.messages2 - for _, d := range l.messages2 { + // TODO(#1393): Remove. + res.Details2 = l.Flush() + for _, d := range res.Details2 { res.Details = append(res.Details, d.Msg.Text) } diff --git a/checker/detail_logger.go b/checker/detail_logger.go index 17a63aef..0c09f3a0 100644 --- a/checker/detail_logger.go +++ b/checker/detail_logger.go @@ -16,13 +16,9 @@ package checker // DetailLogger logs a CheckDetail struct. type DetailLogger interface { - Info(desc string, args ...interface{}) - Warn(desc string, args ...interface{}) - Debug(desc string, args ...interface{}) - - // Functions to use for moving to SARIF format. - // UPGRADEv3: to rename. - Info3(msg *LogMessage) - Warn3(msg *LogMessage) - Debug3(msg *LogMessage) + Info(msg *LogMessage) + Warn(msg *LogMessage) + Debug(msg *LogMessage) + // Flush resets the logger state and returns collected logs. + Flush() []CheckDetail } diff --git a/checker/detail_logger_impl.go b/checker/detail_logger_impl.go index 1e246abd..6aa1e3c1 100644 --- a/checker/detail_logger_impl.go +++ b/checker/detail_logger_impl.go @@ -14,55 +14,36 @@ package checker -import ( - "fmt" -) - -// UPGRADEv2: messages2 will ultimately -// be renamed to messages. type logger struct { - messages2 []CheckDetail + logs []CheckDetail } -func (l *logger) Info(desc string, args ...interface{}) { - cd := CheckDetail{Type: DetailInfo, Msg: LogMessage{Text: fmt.Sprintf(desc, args...)}} - l.messages2 = append(l.messages2, cd) -} - -func (l *logger) Warn(desc string, args ...interface{}) { - cd := CheckDetail{Type: DetailWarn, Msg: LogMessage{Text: fmt.Sprintf(desc, args...)}} - l.messages2 = append(l.messages2, cd) -} - -func (l *logger) Debug(desc string, args ...interface{}) { - cd := CheckDetail{Type: DetailDebug, Msg: LogMessage{Text: fmt.Sprintf(desc, args...)}} - l.messages2 = append(l.messages2, cd) -} - -// UPGRADEv3: to rename. -func (l *logger) Info3(msg *LogMessage) { +func (l *logger) Info(msg *LogMessage) { cd := CheckDetail{ Type: DetailInfo, Msg: *msg, } - cd.Msg.Version = 3 - l.messages2 = append(l.messages2, cd) + l.logs = append(l.logs, cd) } -func (l *logger) Warn3(msg *LogMessage) { +func (l *logger) Warn(msg *LogMessage) { cd := CheckDetail{ Type: DetailWarn, Msg: *msg, } - cd.Msg.Version = 3 - l.messages2 = append(l.messages2, cd) + l.logs = append(l.logs, cd) } -func (l *logger) Debug3(msg *LogMessage) { +func (l *logger) Debug(msg *LogMessage) { cd := CheckDetail{ Type: DetailDebug, Msg: *msg, } - cd.Msg.Version = 3 - l.messages2 = append(l.messages2, cd) + l.logs = append(l.logs, cd) +} + +func (l *logger) Flush() []CheckDetail { + ret := l.logs + l.logs = nil + return ret } diff --git a/checks/ci_tests.go b/checks/ci_tests.go index 0c53dd6e..2b87cb41 100644 --- a/checks/ci_tests.go +++ b/checks/ci_tests.go @@ -83,8 +83,9 @@ func CITests(c *checker.CheckRequest) checker.CheckResult { } if !foundCI { - c.Dlogger.Debug3(&checker.LogMessage{ - Text: fmt.Sprintf("merged PR without CI test: %d", pr.Number), + c.Dlogger.Debug(&checker.LogMessage{ + Text: fmt.Sprintf("merged PR without CI test: %d", pr.Number), + Version: 3, }) } } @@ -109,11 +110,12 @@ func prHasSuccessStatus(pr *clients.PullRequest, c *checker.CheckRequest) (bool, continue } if isTest(status.Context) || isTest(status.TargetURL) { - c.Dlogger.Debug3(&checker.LogMessage{ + c.Dlogger.Debug(&checker.LogMessage{ Path: status.URL, Type: checker.FileTypeURL, Text: fmt.Sprintf("CI test found: pr: %d, context: %s", pr.Number, status.Context), + Version: 3, }) return true, nil } @@ -136,11 +138,12 @@ func prHasSuccessfulCheck(pr *clients.PullRequest, c *checker.CheckRequest) (boo continue } if isTest(cr.App.Slug) { - c.Dlogger.Debug3(&checker.LogMessage{ + c.Dlogger.Debug(&checker.LogMessage{ Path: cr.URL, Type: checker.FileTypeURL, Text: fmt.Sprintf("CI test found: pr: %d, context: %s", pr.Number, cr.App.Slug), + Version: 3, }) return true, nil } diff --git a/checks/contributors.go b/checks/contributors.go index 6cc16b13..e979f3dc 100644 --- a/checks/contributors.go +++ b/checks/contributors.go @@ -73,8 +73,9 @@ func Contributors(c *checker.CheckRequest) checker.CheckResult { names = append(names, c) } - c.Dlogger.Info3(&checker.LogMessage{ - Text: fmt.Sprintf("contributors work for: %v", strings.Join(names, ",")), + c.Dlogger.Info(&checker.LogMessage{ + Text: fmt.Sprintf("contributors work for: %v", strings.Join(names, ",")), + Version: 3, }) reason := fmt.Sprintf("%d different companies found", len(companies)) diff --git a/checks/dangerous_workflow.go b/checks/dangerous_workflow.go index 66f4a030..281a3e07 100644 --- a/checks/dangerous_workflow.go +++ b/checks/dangerous_workflow.go @@ -298,11 +298,12 @@ func checkJobForUntrustedCodeCheckout(job *actionlint.Job, path string, } if strings.Contains(ref.Value.Value, "github.event.pull_request") { line := fileparser.GetLineNumber(step.Pos) - dl.Warn3(&checker.LogMessage{ - Path: path, - Type: checker.FileTypeSource, - Offset: line, - Text: fmt.Sprintf("untrusted code checkout '%v'", ref.Value.Value), + dl.Warn(&checker.LogMessage{ + Path: path, + Type: checker.FileTypeSource, + Offset: line, + Text: fmt.Sprintf("untrusted code checkout '%v'", ref.Value.Value), + Version: 3, // TODO: set Snippet. }) // Detected untrusted checkout. @@ -411,11 +412,12 @@ func checkSecretInScript(script string, pos *actionlint.Pos, path string, variable := strings.Trim(script[s:s+e+2], " ") if strings.Contains(variable, "secrets.") { line := fileparser.GetLineNumber(pos) - dl.Warn3(&checker.LogMessage{ - Path: path, - Type: checker.FileTypeSource, - Offset: line, - Text: fmt.Sprintf("secret accessible to pull requests '%v'", variable), + dl.Warn(&checker.LogMessage{ + Path: path, + Type: checker.FileTypeSource, + Offset: line, + Text: fmt.Sprintf("secret accessible to pull requests '%v'", variable), + Version: 3, // TODO: set Snippet. }) pdata.workflowPattern[secretsViaPullRequests] = true @@ -442,11 +444,12 @@ func checkVariablesInScript(script string, pos *actionlint.Pos, path string, variable := script[s+3 : s+e] if containsUntrustedContextPattern(variable) { line := fileparser.GetLineNumber(pos) - dl.Warn3(&checker.LogMessage{ - Path: path, - Type: checker.FileTypeSource, - Offset: line, - Text: fmt.Sprintf("script injection with untrusted input '%v'", variable), + dl.Warn(&checker.LogMessage{ + Path: path, + Type: checker.FileTypeSource, + Offset: line, + Text: fmt.Sprintf("script injection with untrusted input '%v'", variable), + Version: 3, // TODO: set Snippet. }) pdata.workflowPattern[scriptInjection] = true diff --git a/checks/evaluation/binary_artifacts.go b/checks/evaluation/binary_artifacts.go index ee67076d..a347f5b4 100644 --- a/checks/evaluation/binary_artifacts.go +++ b/checks/evaluation/binary_artifacts.go @@ -34,10 +34,11 @@ func BinaryArtifacts(name string, dl checker.DetailLogger, score := checker.MaxResultScore for _, f := range r.Files { - dl.Warn3(&checker.LogMessage{ + dl.Warn(&checker.LogMessage{ Path: f.Path, Type: checker.FileTypeBinary, - Offset: f.Offset, - Text: "binary detected", + Offset: f.Offset, + Text: "binary detected", + Version: 3, }) // We remove one point for each binary. score-- diff --git a/checks/evaluation/branch_protection.go b/checks/evaluation/branch_protection.go index beed6ab8..41a5a48b 100644 --- a/checks/evaluation/branch_protection.go +++ b/checks/evaluation/branch_protection.go @@ -15,6 +15,8 @@ package evaluation import ( + "fmt" + "github.com/ossf/scorecard/v4/checker" sce "github.com/ossf/scorecard/v4/errors" ) @@ -60,7 +62,9 @@ func BranchProtection(name string, dl checker.DetailLogger, // so it does not provide any guarantees. protected := !(b.Protected != nil && !*b.Protected) if !protected { - dl.Warn("branch protection not enabled for branch '%s'", b.Name) + dl.Warn(&checker.LogMessage{ + Text: fmt.Sprintf("branch protection not enabled for branch '%s'", b.Name), + }) } score.scores.basic, score.maxes.basic = basicNonAdminProtection(&b, dl) score.scores.adminBasic, score.maxes.adminBasic = basicAdminProtection(&b, dl) @@ -223,7 +227,9 @@ func info(dl checker.DetailLogger, doLogging bool, desc string, args ...interfac return } - dl.Info(desc, args...) + dl.Info(&checker.LogMessage{ + Text: fmt.Sprintf(desc, args...), + }) } func debug(dl checker.DetailLogger, doLogging bool, desc string, args ...interface{}) { @@ -231,7 +237,9 @@ func debug(dl checker.DetailLogger, doLogging bool, desc string, args ...interfa return } - dl.Debug(desc, args...) + dl.Debug(&checker.LogMessage{ + Text: fmt.Sprintf(desc, args...), + }) } func warn(dl checker.DetailLogger, doLogging bool, desc string, args ...interface{}) { @@ -239,7 +247,9 @@ func warn(dl checker.DetailLogger, doLogging bool, desc string, args ...interfac return } - dl.Warn(desc, args...) + dl.Warn(&checker.LogMessage{ + Text: fmt.Sprintf(desc, args...), + }) } func basicNonAdminProtection(branch *checker.BranchProtectionData, dl checker.DetailLogger) (int, int) { diff --git a/checks/evaluation/code_review.go b/checks/evaluation/code_review.go index 9a48a6fd..71a6e6f9 100644 --- a/checks/evaluation/code_review.go +++ b/checks/evaluation/code_review.go @@ -69,8 +69,9 @@ func CodeReview(name string, dl checker.DetailLogger, // Only show all warnings if all fail. // We should not show warning if at least one succeeds, as this is confusing. for k := range totalReviewed { - dl.Warn3(&checker.LogMessage{ - Text: fmt.Sprintf("no %s reviews found", k), + dl.Warn(&checker.LogMessage{ + Text: fmt.Sprintf("no %s reviews found", k), + Version: 3, }) } @@ -132,9 +133,10 @@ func isReviewedOnGitHub(c *checker.DefaultBranchCommit, dl checker.DetailLogger) for _, r := range mr.Reviews { if r.State == "APPROVED" { - dl.Debug3(&checker.LogMessage{ + dl.Debug(&checker.LogMessage{ Text: fmt.Sprintf("%s #%d merge request approved", reviewPlatformGitHub, mr.Number), + Version: 3, }) return true } @@ -145,9 +147,10 @@ func isReviewedOnGitHub(c *checker.DefaultBranchCommit, dl checker.DetailLogger) // time on clicking the approve button. if c.Committer.Login != "" && c.Committer.Login != mr.Author.Login { - dl.Debug3(&checker.LogMessage{ + dl.Debug(&checker.LogMessage{ Text: fmt.Sprintf("%s #%d merge request approved", reviewPlatformGitHub, mr.Number), + Version: 3, }) return true } @@ -157,8 +160,9 @@ func isReviewedOnGitHub(c *checker.DefaultBranchCommit, dl checker.DetailLogger) func isReviewedOnProw(c *checker.DefaultBranchCommit, dl checker.DetailLogger) bool { if isBot(c.Committer.Login) { - dl.Debug3(&checker.LogMessage{ - Text: fmt.Sprintf("skip commit from bot account: %s", c.Committer.Login), + dl.Debug(&checker.LogMessage{ + Text: fmt.Sprintf("skip commit from bot account: %s", c.Committer.Login), + Version: 3, }) return true } @@ -166,9 +170,10 @@ func isReviewedOnProw(c *checker.DefaultBranchCommit, dl checker.DetailLogger) b if c.MergeRequest != nil && !c.MergeRequest.MergedAt.IsZero() { for _, l := range c.MergeRequest.Labels { if l == "lgtm" || l == "approved" { - dl.Debug3(&checker.LogMessage{ + dl.Debug(&checker.LogMessage{ Text: fmt.Sprintf("%s #%d merge request approved", reviewPlatformProw, c.MergeRequest.Number), + Version: 3, }) return true } @@ -179,8 +184,9 @@ func isReviewedOnProw(c *checker.DefaultBranchCommit, dl checker.DetailLogger) b func isReviewedOnGerrit(c *checker.DefaultBranchCommit, dl checker.DetailLogger) bool { if isBot(c.Committer.Login) { - dl.Debug3(&checker.LogMessage{ - Text: fmt.Sprintf("skip commit from bot account: %s", c.Committer.Login), + dl.Debug(&checker.LogMessage{ + Text: fmt.Sprintf("skip commit from bot account: %s", c.Committer.Login), + Version: 3, }) return true } @@ -188,8 +194,9 @@ func isReviewedOnGerrit(c *checker.DefaultBranchCommit, dl checker.DetailLogger) m := c.CommitMessage if strings.Contains(m, "\nReviewed-on: ") && strings.Contains(m, "\nReviewed-by: ") { - dl.Debug3(&checker.LogMessage{ - Text: fmt.Sprintf("%s commit approved", reviewPlatformGerrit), + dl.Debug(&checker.LogMessage{ + Text: fmt.Sprintf("%s commit approved", reviewPlatformGerrit), + Version: 3, }) return true } diff --git a/checks/evaluation/dependency_update_tool.go b/checks/evaluation/dependency_update_tool.go index 5ef935c6..6ecb7c6c 100644 --- a/checks/evaluation/dependency_update_tool.go +++ b/checks/evaluation/dependency_update_tool.go @@ -31,13 +31,15 @@ func DependencyUpdateTool(name string, dl checker.DetailLogger, // Apply the policy evaluation. if r.Tools == nil || len(r.Tools) == 0 { - dl.Warn3(&checker.LogMessage{ + dl.Warn(&checker.LogMessage{ Text: `dependabot config file not detected in source location. We recommend setting this configuration in code so it can be easily verified by others.`, + Version: 3, }) - dl.Warn3(&checker.LogMessage{ + dl.Warn(&checker.LogMessage{ Text: `renovatebot config file not detected in source location. We recommend setting this configuration in code so it can be easily verified by others.`, + Version: 3, }) return checker.CreateMinScoreResult(name, "no update tool detected") } @@ -56,11 +58,12 @@ func DependencyUpdateTool(name string, dl checker.DetailLogger, // Note: only one file per tool is present, // so we do not iterate thru all entries. - dl.Info3(&checker.LogMessage{ - Path: r.Tools[0].ConfigFiles[0].Path, - Type: r.Tools[0].ConfigFiles[0].Type, - Offset: r.Tools[0].ConfigFiles[0].Offset, - Text: fmt.Sprintf("%s detected", r.Tools[0].Name), + dl.Info(&checker.LogMessage{ + Path: r.Tools[0].ConfigFiles[0].Path, + Type: r.Tools[0].ConfigFiles[0].Type, + Offset: r.Tools[0].ConfigFiles[0].Offset, + Text: fmt.Sprintf("%s detected", r.Tools[0].Name), + Version: 3, }) // High score result. diff --git a/checks/evaluation/security_policy.go b/checks/evaluation/security_policy.go index ebf8ca82..170c2bf7 100644 --- a/checks/evaluation/security_policy.go +++ b/checks/evaluation/security_policy.go @@ -33,16 +33,17 @@ func SecurityPolicy(name string, dl checker.DetailLogger, r *checker.SecurityPol for _, f := range r.Files { msg := checker.LogMessage{ - Path: f.Path, - Type: f.Type, - Offset: f.Offset, + Path: f.Path, + Type: f.Type, + Offset: f.Offset, + Version: 3, } if msg.Type == checker.FileTypeURL { msg.Text = "security policy detected in org repo" } else { msg.Text = "security policy detected" } - dl.Info3(&msg) + dl.Info(&msg) } return checker.CreateMaxScoreResult(name, "security policy file detected") diff --git a/checks/evaluation/vulnerabilities.go b/checks/evaluation/vulnerabilities.go index 7d53ce19..231c78ed 100644 --- a/checks/evaluation/vulnerabilities.go +++ b/checks/evaluation/vulnerabilities.go @@ -42,8 +42,9 @@ func Vulnerabilities(name string, dl checker.DetailLogger, } if len(IDs) > 0 { - dl.Warn3(&checker.LogMessage{ - Text: fmt.Sprintf("HEAD is vulnerable to %s", strings.Join(IDs, ", ")), + dl.Warn(&checker.LogMessage{ + Text: fmt.Sprintf("HEAD is vulnerable to %s", strings.Join(IDs, ", ")), + Version: 3, }) return checker.CreateResultWithScore(name, fmt.Sprintf("%v existing vulnerabilities detected", len(IDs)), score) diff --git a/checks/license.go b/checks/license.go index 3259f102..abe01108 100644 --- a/checks/license.go +++ b/checks/license.go @@ -109,10 +109,11 @@ func LicenseCheck(c *checker.CheckRequest) checker.CheckResult { pdata := fileparser.FileGetCbDataAsBoolPointer(data) if checkLicense(name) { - c.Dlogger.Info3(&checker.LogMessage{ - Path: name, - Type: checker.FileTypeSource, - Offset: 1, + c.Dlogger.Info(&checker.LogMessage{ + Path: name, + Type: checker.FileTypeSource, + Offset: 1, + Version: 3, }) *pdata = true return false, nil diff --git a/checks/packaging.go b/checks/packaging.go index 73ca3612..21f990b0 100644 --- a/checks/packaging.go +++ b/checks/packaging.go @@ -66,25 +66,28 @@ func Packaging(c *checker.CheckRequest) checker.CheckResult { return checker.CreateRuntimeErrorResult(CheckPackaging, e) } if len(runs) > 0 { - c.Dlogger.Info3(&checker.LogMessage{ - Path: fp, - Type: checker.FileTypeSource, - Offset: checker.OffsetDefault, - Text: fmt.Sprintf("GitHub publishing workflow used in run %s", runs[0].URL), + c.Dlogger.Info(&checker.LogMessage{ + Path: fp, + Type: checker.FileTypeSource, + Offset: checker.OffsetDefault, + Text: fmt.Sprintf("GitHub publishing workflow used in run %s", runs[0].URL), + Version: 3, }) return checker.CreateMaxScoreResult(CheckPackaging, "publishing workflow detected") } - c.Dlogger.Debug3(&checker.LogMessage{ - Path: fp, - Type: checker.FileTypeSource, - Offset: checker.OffsetDefault, - Text: "GitHub publishing workflow not used in runs", + c.Dlogger.Debug(&checker.LogMessage{ + Path: fp, + Type: checker.FileTypeSource, + Offset: checker.OffsetDefault, + Text: "GitHub publishing workflow not used in runs", + Version: 3, }) } - c.Dlogger.Warn3(&checker.LogMessage{ - Text: "no GitHub publishing workflow detected", + c.Dlogger.Warn(&checker.LogMessage{ + Text: "no GitHub publishing workflow detected", + Version: 3, }) return checker.CreateInconclusiveResult(CheckPackaging, @@ -207,21 +210,23 @@ func isPackagingWorkflow(workflow *actionlint.Workflow, fp string, dl checker.De continue } - dl.Info3(&checker.LogMessage{ - Path: fp, - Type: checker.FileTypeSource, - Offset: fileparser.GetLineNumber(job.Pos), - Text: matcher.LogText, + dl.Info(&checker.LogMessage{ + Path: fp, + Type: checker.FileTypeSource, + Offset: fileparser.GetLineNumber(job.Pos), + Text: matcher.LogText, + Version: 3, }) return true } } - dl.Debug3(&checker.LogMessage{ - Path: fp, - Type: checker.FileTypeSource, - Offset: checker.OffsetDefault, - Text: "not a publishing workflow", + dl.Debug(&checker.LogMessage{ + Path: fp, + Type: checker.FileTypeSource, + Offset: checker.OffsetDefault, + Text: "not a publishing workflow", + Version: 3, }) return false } diff --git a/checks/permissions.go b/checks/permissions.go index 9a7d88b3..31030cd7 100644 --- a/checks/permissions.go +++ b/checks/permissions.go @@ -97,33 +97,36 @@ func validatePermission(permissionKey permission, permissionValue *actionlint.Pe lineNumber := fileparser.GetLineNumber(permissionValue.Value.Pos) if strings.EqualFold(val, "write") { if isPermissionOfInterest(permissionKey, ignoredPermissions) { - dl.Warn3(&checker.LogMessage{ - Path: path, - Type: checker.FileTypeSource, - Offset: lineNumber, - Text: fmt.Sprintf("%s '%v' permission set to '%v'", permLevel, permissionKey, val), + dl.Warn(&checker.LogMessage{ + Path: path, + Type: checker.FileTypeSource, + Offset: lineNumber, + Text: fmt.Sprintf("%s '%v' permission set to '%v'", permLevel, permissionKey, val), + Version: 3, // TODO: set Snippet. }) recordPermissionWrite(pPermissions, permissionKey) } else { // Only log for debugging, otherwise // it may confuse users. - dl.Debug3(&checker.LogMessage{ - Path: path, - Type: checker.FileTypeSource, - Offset: lineNumber, - Text: fmt.Sprintf("%s '%v' permission set to '%v'", permLevel, permissionKey, val), + dl.Debug(&checker.LogMessage{ + Path: path, + Type: checker.FileTypeSource, + Offset: lineNumber, + Text: fmt.Sprintf("%s '%v' permission set to '%v'", permLevel, permissionKey, val), + Version: 3, // TODO: set Snippet. }) } return nil } - dl.Info3(&checker.LogMessage{ - Path: path, - Type: checker.FileTypeSource, - Offset: lineNumber, - Text: fmt.Sprintf("%s '%v' permission set to '%v'", permLevel, permissionKey, val), + dl.Info(&checker.LogMessage{ + Path: path, + Type: checker.FileTypeSource, + Offset: lineNumber, + Text: fmt.Sprintf("%s '%v' permission set to '%v'", permLevel, permissionKey, val), + Version: 3, // TODO: set Snippet. }) return nil @@ -170,33 +173,36 @@ func validatePermissions(permissions *actionlint.Permissions, permLevel, path st allIsSet := permissions != nil && permissions.All != nil && permissions.All.Value != "" scopeIsSet := permissions != nil && len(permissions.Scopes) > 0 if permissions == nil || (!allIsSet && !scopeIsSet) { - dl.Info3(&checker.LogMessage{ - Path: path, - Type: checker.FileTypeSource, - Offset: checker.OffsetDefault, - Text: fmt.Sprintf("%s permissions set to 'none'", permLevel), + dl.Info(&checker.LogMessage{ + Path: path, + Type: checker.FileTypeSource, + Offset: checker.OffsetDefault, + Text: fmt.Sprintf("%s permissions set to 'none'", permLevel), + Version: 3, }) } if allIsSet { val := permissions.All.Value lineNumber := fileparser.GetLineNumber(permissions.All.Pos) if !strings.EqualFold(val, "read-all") && val != "" { - dl.Warn3(&checker.LogMessage{ - Path: path, - Type: checker.FileTypeSource, - Offset: lineNumber, - Text: fmt.Sprintf("%s permissions set to '%v'", permLevel, val), + dl.Warn(&checker.LogMessage{ + Path: path, + Type: checker.FileTypeSource, + Offset: lineNumber, + Text: fmt.Sprintf("%s permissions set to '%v'", permLevel, val), + Version: 3, // TODO: set Snippet. }) recordAllPermissionsWrite(pdata, permLevel, path) return nil } - dl.Info3(&checker.LogMessage{ - Path: path, - Type: checker.FileTypeSource, - Offset: lineNumber, - Text: fmt.Sprintf("%s permissions set to '%v'", permLevel, val), + dl.Info(&checker.LogMessage{ + Path: path, + Type: checker.FileTypeSource, + Offset: lineNumber, + Text: fmt.Sprintf("%s permissions set to '%v'", permLevel, val), + Version: 3, // TODO: set Snippet. }) } else /* scopeIsSet == true */ if err := validateMapPermissions(permissions.Scopes, @@ -210,11 +216,12 @@ func validateTopLevelPermissions(workflow *actionlint.Workflow, path string, dl checker.DetailLogger, pdata *permissionCbData) error { // Check if permissions are set explicitly. if workflow.Permissions == nil { - dl.Warn3(&checker.LogMessage{ - Path: path, - Type: checker.FileTypeSource, - Offset: checker.OffsetDefault, - Text: fmt.Sprintf("no %s permission defined", topLevelPermission), + dl.Warn(&checker.LogMessage{ + Path: path, + Type: checker.FileTypeSource, + Offset: checker.OffsetDefault, + Text: fmt.Sprintf("no %s permission defined", topLevelPermission), + Version: 3, }) recordAllPermissionsWrite(pdata, topLevelPermission, path) return nil @@ -232,11 +239,12 @@ func validatejobLevelPermissions(workflow *actionlint.Workflow, path string, // For most workflows, no write permissions are needed, // so only top-level read-only permissions need to be declared. if job.Permissions == nil { - dl.Debug3(&checker.LogMessage{ - Path: path, - Type: checker.FileTypeSource, - Offset: fileparser.GetLineNumber(job.Pos), - Text: fmt.Sprintf("no %s permission defined", jobLevelPermission), + dl.Debug(&checker.LogMessage{ + Path: path, + Type: checker.FileTypeSource, + Offset: fileparser.GetLineNumber(job.Pos), + Text: fmt.Sprintf("no %s permission defined", jobLevelPermission), + Version: 3, }) recordAllPermissionsWrite(pdata, jobLevelPermission, path) continue @@ -472,22 +480,24 @@ func isSARIFUploadAction(workflow *actionlint.Workflow, fp string, dl checker.De continue } if strings.HasPrefix(uses.Value, "github/codeql-action/upload-sarif@") { - dl.Debug3(&checker.LogMessage{ - Path: fp, - Type: checker.FileTypeSource, - Offset: fileparser.GetLineNumber(uses.Pos), - Text: "codeql SARIF upload workflow detected", + dl.Debug(&checker.LogMessage{ + Path: fp, + Type: checker.FileTypeSource, + Offset: fileparser.GetLineNumber(uses.Pos), + Text: "codeql SARIF upload workflow detected", + Version: 3, // TODO: set Snippet. }) return true } } } - dl.Debug3(&checker.LogMessage{ - Path: fp, - Type: checker.FileTypeSource, - Offset: checker.OffsetDefault, - Text: "not a codeql upload SARIF workflow", + dl.Debug(&checker.LogMessage{ + Path: fp, + Type: checker.FileTypeSource, + Offset: checker.OffsetDefault, + Text: "not a codeql upload SARIF workflow", + Version: 3, }) return false } @@ -504,22 +514,24 @@ func isCodeQlAnalysisWorkflow(workflow *actionlint.Workflow, fp string, dl check continue } if strings.HasPrefix(uses.Value, "github/codeql-action/analyze@") { - dl.Debug3(&checker.LogMessage{ - Path: fp, - Type: checker.FileTypeSource, - Offset: fileparser.GetLineNumber(uses.Pos), - Text: "codeql workflow detected", + dl.Debug(&checker.LogMessage{ + Path: fp, + Type: checker.FileTypeSource, + Offset: fileparser.GetLineNumber(uses.Pos), + Text: "codeql workflow detected", + Version: 3, // TODO: set Snippet. }) return true } } } - dl.Debug3(&checker.LogMessage{ - Path: fp, - Type: checker.FileTypeSource, - Offset: checker.OffsetDefault, - Text: "not a codeql workflow", + dl.Debug(&checker.LogMessage{ + Path: fp, + Type: checker.FileTypeSource, + Offset: checker.OffsetDefault, + Text: "not a codeql workflow", + Version: 3, }) return false } diff --git a/checks/pinned_dependencies.go b/checks/pinned_dependencies.go index 5b991a6a..4aae6643 100644 --- a/checks/pinned_dependencies.go +++ b/checks/pinned_dependencies.go @@ -161,19 +161,21 @@ func createReturnValuesForGitHubActionsWorkflowPinned(r worklowPinningResult, in if r.gitHubOwned != notPinned { score += 2 - dl.Info3(&checker.LogMessage{ - Type: checker.FileTypeSource, - Offset: checker.OffsetDefault, - Text: fmt.Sprintf("%s %s", "GitHub-owned", infoMsg), + dl.Info(&checker.LogMessage{ + Type: checker.FileTypeSource, + Offset: checker.OffsetDefault, + Text: fmt.Sprintf("%s %s", "GitHub-owned", infoMsg), + Version: 3, }) } if r.thirdParties != notPinned { score += 8 - dl.Info3(&checker.LogMessage{ - Type: checker.FileTypeSource, - Offset: checker.OffsetDefault, - Text: fmt.Sprintf("%s %s", "Third-party", infoMsg), + dl.Info(&checker.LogMessage{ + Type: checker.FileTypeSource, + Offset: checker.OffsetDefault, + Text: fmt.Sprintf("%s %s", "Third-party", infoMsg), + Version: 3, }) } @@ -198,7 +200,9 @@ func createReturnValues(r pinnedResult, infoMsg string, dl checker.DetailLogger, default: panic("invalid value") case pinned, pinnedUndefined: - dl.Info(infoMsg) + dl.Info(&checker.LogMessage{ + Text: infoMsg, + }) return checker.MaxResultScore, nil case notPinned: // No logging needed as it's done by the checks. @@ -429,13 +433,14 @@ func validateDockerfileIsPinned(pathfn string, content []byte, // Not pinned. ret = false - dl.Warn3(&checker.LogMessage{ + dl.Warn(&checker.LogMessage{ Path: pathfn, Type: checker.FileTypeSource, Offset: uint(child.StartLine), EndOffset: uint(child.EndLine), Text: "docker image not pinned by hash", Snippet: child.Original, + Version: 3, }) // FROM name. @@ -444,13 +449,14 @@ func validateDockerfileIsPinned(pathfn string, content []byte, pinned := pinnedAsNames[name] if !pinned && !regex.MatchString(name) { ret = false - dl.Warn3(&checker.LogMessage{ + dl.Warn(&checker.LogMessage{ Path: pathfn, Type: checker.FileTypeSource, Offset: uint(child.StartLine), EndOffset: uint(child.EndLine), Text: "docker image not pinned by hash", Snippet: child.Original, + Version: 3, }) } @@ -654,12 +660,13 @@ func validateGitHubActionWorkflow(pathfn string, content []byte, // Example: action-name@hash match := hashRegex.MatchString(execAction.Uses.Value) if !match { - dl.Warn3(&checker.LogMessage{ + dl.Warn(&checker.LogMessage{ Path: pathfn, Type: checker.FileTypeSource, Offset: uint(execAction.Uses.Pos.Line), EndOffset: uint(execAction.Uses.Pos.Line), // `Uses` always span a single line. Snippet: execAction.Uses.Value, Text: fmt.Sprintf("%s action not pinned by hash", owner), + Version: 3, }) } diff --git a/checks/sast.go b/checks/sast.go index e50969d4..dedc1625 100644 --- a/checks/sast.go +++ b/checks/sast.go @@ -141,10 +141,11 @@ func sastToolInCheckRuns(c *checker.CheckRequest) (int, error) { continue } if sastTools[cr.App.Slug] { - c.Dlogger.Debug3(&checker.LogMessage{ - Path: cr.URL, - Type: checker.FileTypeURL, - Text: "tool detected", + c.Dlogger.Debug(&checker.LogMessage{ + Path: cr.URL, + Type: checker.FileTypeURL, + Text: "tool detected", + Version: 3, }) totalTested++ break @@ -152,19 +153,22 @@ func sastToolInCheckRuns(c *checker.CheckRequest) (int, error) { } } if totalMerged == 0 { - c.Dlogger.Warn3(&checker.LogMessage{ - Text: "no pull requests merged into dev branch", + c.Dlogger.Warn(&checker.LogMessage{ + Text: "no pull requests merged into dev branch", + Version: 3, }) return checker.InconclusiveResultScore, nil } if totalTested == totalMerged { - c.Dlogger.Info3(&checker.LogMessage{ - Text: fmt.Sprintf("all commits (%v) are checked with a SAST tool", totalMerged), + c.Dlogger.Info(&checker.LogMessage{ + Text: fmt.Sprintf("all commits (%v) are checked with a SAST tool", totalMerged), + Version: 3, }) } else { - c.Dlogger.Warn3(&checker.LogMessage{ - Text: fmt.Sprintf("%v commits out of %v are checked with a SAST tool", totalTested, totalMerged), + c.Dlogger.Warn(&checker.LogMessage{ + Text: fmt.Sprintf("%v commits out of %v are checked with a SAST tool", totalTested, totalMerged), + Version: 3, }) } @@ -184,25 +188,28 @@ func codeQLInCheckDefinitions(c *checker.CheckRequest) (int, error) { } for _, result := range resp.Results { - c.Dlogger.Debug3(&checker.LogMessage{ - Path: result.Path, - Type: checker.FileTypeSource, - Offset: checker.OffsetDefault, - Text: "CodeQL detected", + c.Dlogger.Debug(&checker.LogMessage{ + Path: result.Path, + Type: checker.FileTypeSource, + Offset: checker.OffsetDefault, + Text: "CodeQL detected", + Version: 3, }) } // TODO: check if it's enabled as cron or presubmit. // TODO: check which branches it is enabled on. We should find main. if resp.Hits > 0 { - c.Dlogger.Info3(&checker.LogMessage{ - Text: "SAST tool detected: CodeQL", + c.Dlogger.Info(&checker.LogMessage{ + Text: "SAST tool detected: CodeQL", + Version: 3, }) return checker.MaxResultScore, nil } - c.Dlogger.Warn3(&checker.LogMessage{ - Text: "CodeQL tool not detected", + c.Dlogger.Warn(&checker.LogMessage{ + Text: "CodeQL tool not detected", + Version: 3, }) return checker.MinResultScore, nil } diff --git a/checks/shell_download_validate.go b/checks/shell_download_validate.go index be45a6ab..83a0df9c 100644 --- a/checks/shell_download_validate.go +++ b/checks/shell_download_validate.go @@ -324,13 +324,14 @@ func isFetchPipeExecute(startLine, endLine uint, node syntax.Node, cmd, pathfn s } startLine, endLine = getLine(startLine, endLine, node) - dl.Warn3(&checker.LogMessage{ + dl.Warn(&checker.LogMessage{ Path: pathfn, Type: checker.FileTypeSource, Offset: startLine, EndOffset: endLine, Snippet: cmd, Text: "insecure (not pinned by hash) download detected", + Version: 3, }) return true } @@ -372,13 +373,14 @@ func isExecuteFiles(startLine, endLine uint, node syntax.Node, cmd, pathfn strin startLine, endLine = getLine(startLine, endLine, node) for fn := range files { if isInterpreterWithFile(c, fn) || isExecuteFile(c, fn) { - dl.Warn3(&checker.LogMessage{ + dl.Warn(&checker.LogMessage{ Path: pathfn, Type: checker.FileTypeSource, Offset: startLine, EndOffset: endLine, Snippet: cmd, Text: "insecure (not pinned by hash) download-then-run", + Version: 3, }) ok = true } @@ -589,39 +591,42 @@ func isUnpinnedPakageManagerDownload(startLine, endLine uint, node syntax.Node, // Go get/install. if isGoUnpinnedDownload(c) { - dl.Warn3(&checker.LogMessage{ + dl.Warn(&checker.LogMessage{ Path: pathfn, Type: checker.FileTypeSource, Offset: startLine, EndOffset: endLine, Snippet: cmd, Text: "go installation not pinned by hash", + Version: 3, }) return true } // Pip install. if isPipUnpinnedDownload(c) { - dl.Warn3(&checker.LogMessage{ + dl.Warn(&checker.LogMessage{ Path: pathfn, Type: checker.FileTypeSource, Offset: startLine, EndOffset: endLine, Snippet: cmd, Text: "pip installation not pinned by hash", + Version: 3, }) return true } // Npm install. if isNpmUnpinnedDownload(c) { - dl.Warn3(&checker.LogMessage{ + dl.Warn(&checker.LogMessage{ Path: pathfn, Type: checker.FileTypeSource, Offset: startLine, EndOffset: endLine, Snippet: cmd, Text: "npm installation not pinned by hash", + Version: 3, }) return true } @@ -707,13 +712,14 @@ func isFetchProcSubsExecute(startLine, endLine uint, node syntax.Node, cmd, path startLine, endLine = getLine(startLine, endLine, node) - dl.Warn3(&checker.LogMessage{ + dl.Warn(&checker.LogMessage{ Path: pathfn, Type: checker.FileTypeSource, Offset: startLine, EndOffset: endLine, Snippet: cmd, Text: "insecure (not pinned by hash) download-then-run", + Version: 3, }) return true } @@ -946,7 +952,9 @@ func validateShellFile(pathfn string, startLine, endLine uint, r, err := validateShellFileAndRecord(pathfn, startLine, endLine, content, taintedFiles, dl) if err != nil && errors.Is(err, sce.ErrorShellParsing) { // Discard and print this particular error for now. - dl.Debug(err.Error()) + dl.Debug(&checker.LogMessage{ + Text: err.Error(), + }) err = nil } return r, err diff --git a/checks/signed_releases.go b/checks/signed_releases.go index 4d31ee7a..0bd04579 100644 --- a/checks/signed_releases.go +++ b/checks/signed_releases.go @@ -52,18 +52,20 @@ func SignedReleases(c *checker.CheckRequest) checker.CheckResult { if len(r.Assets) == 0 { continue } - c.Dlogger.Debug3(&checker.LogMessage{ - Text: fmt.Sprintf("GitHub release found: %s", r.TagName), + c.Dlogger.Debug(&checker.LogMessage{ + Text: fmt.Sprintf("GitHub release found: %s", r.TagName), + Version: 3, }) totalReleases++ signed := false for _, asset := range r.Assets { for _, suffix := range artifactExtensions { if strings.HasSuffix(asset.Name, suffix) { - c.Dlogger.Info3(&checker.LogMessage{ - Path: asset.URL, - Type: checker.FileTypeURL, - Text: fmt.Sprintf("signed release artifact: %s", asset.Name), + c.Dlogger.Info(&checker.LogMessage{ + Path: asset.URL, + Type: checker.FileTypeURL, + Text: fmt.Sprintf("signed release artifact: %s", asset.Name), + Version: 3, }) signed = true break @@ -75,10 +77,11 @@ func SignedReleases(c *checker.CheckRequest) checker.CheckResult { } } if !signed { - c.Dlogger.Warn3(&checker.LogMessage{ - Path: r.URL, - Type: checker.FileTypeURL, - Text: fmt.Sprintf("release artifact %s not signed", r.TagName), + c.Dlogger.Warn(&checker.LogMessage{ + Path: r.URL, + Type: checker.FileTypeURL, + Text: fmt.Sprintf("release artifact %s not signed", r.TagName), + Version: 3, }) } if totalReleases >= releaseLookBack { @@ -87,8 +90,9 @@ func SignedReleases(c *checker.CheckRequest) checker.CheckResult { } if totalReleases == 0 { - c.Dlogger.Warn3(&checker.LogMessage{ - Text: "no GitHub releases found", + c.Dlogger.Warn(&checker.LogMessage{ + Text: "no GitHub releases found", + Version: 3, }) // Generic summary. return checker.CreateInconclusiveResult(CheckSignedReleases, "no releases found") diff --git a/pkg/common.go b/pkg/common.go index ec72ddec..8cfc8c6f 100644 --- a/pkg/common.go +++ b/pkg/common.go @@ -28,7 +28,7 @@ func textToMarkdown(s string) string { // DetailToString turns a detail information into a string. func DetailToString(d *checker.CheckDetail, logLevel log.Level) string { - // UPGRADEv3: remove switch statement. + // TODO(#1393): remove switch statement. switch d.Msg.Version { case 3: if d.Type == checker.DetailDebug && logLevel != log.DebugLevel { diff --git a/utests/utlib.go b/utests/utlib.go index 51f09372..4f4d825f 100644 --- a/utests/utlib.go +++ b/utests/utlib.go @@ -41,27 +41,7 @@ type TestDetailLogger struct { } // Info implements DetailLogger.Info. -func (l *TestDetailLogger) Info(desc string, args ...interface{}) { - cd := checker.CheckDetail{Type: checker.DetailInfo, Msg: checker.LogMessage{Text: fmt.Sprintf(desc, args...)}} - l.messages = append(l.messages, cd) -} - -// Warn implements DetailLogger.Warn. -func (l *TestDetailLogger) Warn(desc string, args ...interface{}) { - cd := checker.CheckDetail{Type: checker.DetailWarn, Msg: checker.LogMessage{Text: fmt.Sprintf(desc, args...)}} - l.messages = append(l.messages, cd) -} - -// Debug implements DetailLogger.Debug. -func (l *TestDetailLogger) Debug(desc string, args ...interface{}) { - cd := checker.CheckDetail{Type: checker.DetailDebug, Msg: checker.LogMessage{Text: fmt.Sprintf(desc, args...)}} - l.messages = append(l.messages, cd) -} - -// UPGRADEv3: to rename. - -// Info3 implements DetailLogger.Info3. -func (l *TestDetailLogger) Info3(msg *checker.LogMessage) { +func (l *TestDetailLogger) Info(msg *checker.LogMessage) { cd := checker.CheckDetail{ Type: checker.DetailInfo, Msg: *msg, @@ -70,8 +50,8 @@ func (l *TestDetailLogger) Info3(msg *checker.LogMessage) { l.messages = append(l.messages, cd) } -// Warn3 implements DetailLogger.Warn3. -func (l *TestDetailLogger) Warn3(msg *checker.LogMessage) { +// Warn implements DetailLogger.Warn. +func (l *TestDetailLogger) Warn(msg *checker.LogMessage) { cd := checker.CheckDetail{ Type: checker.DetailWarn, Msg: *msg, @@ -80,8 +60,8 @@ func (l *TestDetailLogger) Warn3(msg *checker.LogMessage) { l.messages = append(l.messages, cd) } -// Debug3 implements DetailLogger.Debug3. -func (l *TestDetailLogger) Debug3(msg *checker.LogMessage) { +// Debug implements DetailLogger.Debug. +func (l *TestDetailLogger) Debug(msg *checker.LogMessage) { cd := checker.CheckDetail{ Type: checker.DetailDebug, Msg: *msg, @@ -90,6 +70,13 @@ func (l *TestDetailLogger) Debug3(msg *checker.LogMessage) { l.messages = append(l.messages, cd) } +// Flush implements DetailLogger.Flush. +func (l *TestDetailLogger) Flush() []checker.CheckDetail { + ret := l.messages + l.messages = nil + return ret +} + func getTestReturn(cr *checker.CheckResult, logger *TestDetailLogger) (*TestReturn, error) { ret := new(TestReturn) for _, v := range logger.messages {