diff --git a/checker/check_result.go b/checker/check_result.go index 56b07992..0a1d360d 100644 --- a/checker/check_result.go +++ b/checker/check_result.go @@ -58,6 +58,10 @@ const ( FileTypeURL ) +// OffsetDefault is used if we can't determine the offset, for example when referencing a file but not a +// specific location in the file. +const OffsetDefault = 1 + // LogMessage is a structure that encapsulates detail's information. // This allows updating the definition easily. //nolint diff --git a/checks/dependency_update_tool.go b/checks/dependency_update_tool.go index 76e576cd..11feeb20 100644 --- a/checks/dependency_update_tool.go +++ b/checks/dependency_update_tool.go @@ -61,20 +61,18 @@ func fileExists(name string, dl checker.DetailLogger, data fileparser.FileCbData switch strings.ToLower(name) { case ".github/dependabot.yml": dl.Info3(&checker.LogMessage{ - Path: name, - Type: checker.FileTypeSource, - // Source file must have line number > 0. - Offset: 1, + Path: name, + Type: checker.FileTypeSource, + Offset: checker.OffsetDefault, Text: "dependabot detected", }) // https://docs.renovatebot.com/configuration-options/ case ".github/renovate.json", ".github/renovate.json5", ".renovaterc.json", "renovate.json", "renovate.json5", ".renovaterc": dl.Info3(&checker.LogMessage{ - Path: name, - Type: checker.FileTypeSource, - // Source file must have line number > 0. - Offset: 1, + Path: name, + Type: checker.FileTypeSource, + Offset: checker.OffsetDefault, Text: "renovate detected", }) default: diff --git a/checks/packaging.go b/checks/packaging.go index 793b8a50..e258a663 100644 --- a/checks/packaging.go +++ b/checks/packaging.go @@ -62,20 +62,18 @@ func Packaging(c *checker.CheckRequest) checker.CheckResult { } if len(runs) > 0 { c.Dlogger.Info3(&checker.LogMessage{ - Path: fp, - Type: checker.FileTypeSource, - // Source file must have line number > 0. - Offset: 1, + Path: fp, + Type: checker.FileTypeSource, + Offset: checker.OffsetDefault, Text: fmt.Sprintf("GitHub publishing workflow used in run %s", runs[0].URL), }) return checker.CreateMaxScoreResult(CheckPackaging, "publishing workflow detected") } c.Dlogger.Info3(&checker.LogMessage{ - Path: fp, - Type: checker.FileTypeSource, - // Source file must have line number > 0. - Offset: 1, + Path: fp, + Type: checker.FileTypeSource, + Offset: checker.OffsetDefault, Text: "GitHub publishing workflow not used in runs", }) } diff --git a/checks/permissions.go b/checks/permissions.go index fb85a7a9..d79af779 100644 --- a/checks/permissions.go +++ b/checks/permissions.go @@ -60,13 +60,16 @@ func validatePermission(permissionKey string, permissionValue *actionlint.Permis return sce.WithMessage(sce.ErrScorecardInternal, errInvalidGitHubWorkflow.Error()) } val := permissionValue.Value.Value + lineNumber := checker.OffsetDefault + if permissionValue.Value.Pos != nil { + lineNumber = permissionValue.Value.Pos.Line + } if strings.EqualFold(val, "write") { if isPermissionOfInterest(permissionKey, ignoredPermissions) { dl.Warn3(&checker.LogMessage{ - Path: path, - Type: checker.FileTypeSource, - // TODO: set line. - Offset: 1, + Path: path, + Type: checker.FileTypeSource, + Offset: lineNumber, Text: fmt.Sprintf("'%v' permission set to '%v'", permissionKey, val), // TODO: set Snippet. }) @@ -75,10 +78,9 @@ func validatePermission(permissionKey string, permissionValue *actionlint.Permis // Only log for debugging, otherwise // it may confuse users. dl.Debug3(&checker.LogMessage{ - Path: path, - Type: checker.FileTypeSource, - // TODO: set line. - Offset: 1, + Path: path, + Type: checker.FileTypeSource, + Offset: lineNumber, Text: fmt.Sprintf("'%v' permission set to '%v'", permissionKey, val), // TODO: set Snippet. }) @@ -87,10 +89,9 @@ func validatePermission(permissionKey string, permissionValue *actionlint.Permis } dl.Info3(&checker.LogMessage{ - Path: path, - Type: checker.FileTypeSource, - // TODO: set line correctly. - Offset: 1, + Path: path, + Type: checker.FileTypeSource, + Offset: lineNumber, Text: fmt.Sprintf("'%v' permission set to '%v'", permissionKey, val), // TODO: set Snippet. }) @@ -125,22 +126,23 @@ func validatePermissions(permissions *actionlint.Permissions, path string, scopeIsSet := permissions != nil && len(permissions.Scopes) > 0 if permissions == nil || (!allIsSet && !scopeIsSet) { dl.Info3(&checker.LogMessage{ - Path: path, - Type: checker.FileTypeSource, - // TODO: set line correctly. - Offset: 1, + Path: path, + Type: checker.FileTypeSource, + Offset: checker.OffsetDefault, Text: "permissions set to 'none'", - // TODO: set Snippet. }) } if allIsSet { val := permissions.All.Value + lineNumber := checker.OffsetDefault + if permissions.All.Pos != nil { + lineNumber = permissions.All.Pos.Line + } if !strings.EqualFold(val, "read-all") && val != "" { dl.Warn3(&checker.LogMessage{ - Path: path, - Type: checker.FileTypeSource, - // TODO: set line correctly. - Offset: 1, + Path: path, + Type: checker.FileTypeSource, + Offset: lineNumber, Text: fmt.Sprintf("permissions set to '%v'", val), // TODO: set Snippet. }) @@ -149,10 +151,9 @@ func validatePermissions(permissions *actionlint.Permissions, path string, } dl.Info3(&checker.LogMessage{ - Path: path, - Type: checker.FileTypeSource, - // TODO: set line correctly. - Offset: 1, + Path: path, + Type: checker.FileTypeSource, + Offset: lineNumber, Text: fmt.Sprintf("permissions set to '%v'", val), // TODO: set Snippet. }) @@ -170,7 +171,7 @@ func validateTopLevelPermissions(workflow *actionlint.Workflow, path string, dl.Warn3(&checker.LogMessage{ Path: path, Type: checker.FileTypeSource, - Offset: 1, + Offset: checker.OffsetDefault, Text: "no permission defined", }) recordAllPermissionsWrite(pdata.topLevelWritePermissions) @@ -189,10 +190,14 @@ func validateRunLevelPermissions(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 { + lineNumber := checker.OffsetDefault + if job.Pos != nil { + lineNumber = job.Pos.Line + } dl.Debug3(&checker.LogMessage{ Path: path, Type: checker.FileTypeSource, - Offset: 1, + Offset: lineNumber, Text: "no permission defined", }) continue diff --git a/checks/permissions_test.go b/checks/permissions_test.go index 74ad7de3..1b08c0fc 100644 --- a/checks/permissions_test.go +++ b/checks/permissions_test.go @@ -286,3 +286,58 @@ func TestGithubTokenPermissions(t *testing.T) { }) } } + +func TestGithubTokenPermissionsLineNumber(t *testing.T) { + t.Parallel() + tests := []struct { + name string + filename string + expected []struct { + lineNumber int + } + }{ + { + name: "Job level write permission", + filename: "./testdata/github-workflow-permissions-run-no-codeql-write.yaml", + expected: []struct { + lineNumber int + }{ + { + lineNumber: 22, + }, + }, + }, + { + name: "Workflow level write permission", + filename: "./testdata/github-workflow-permissions-writeall.yaml", + expected: []struct { + lineNumber int + }{ + { + lineNumber: 16, + }, + }, + }, + } + 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() + content, err := os.ReadFile(tt.filename) + if err != nil { + t.Errorf("cannot read file: %v", err) + } + dl := scut.TestDetailLogger{} + testValidateGitHubActionTokenPermissions(tt.filename, content, &dl) + for _, expectedLog := range tt.expected { + isExpectedLog := func(logMessage checker.LogMessage, logType checker.DetailType) bool { + return logMessage.Offset == expectedLog.lineNumber && logMessage.Path == tt.filename && + logType == checker.DetailWarn + } + if !scut.ValidateLogMessage(isExpectedLog, &dl) { + t.Errorf("test failed: log message not present: %+v", tt.expected) + } + } + }) + } +} diff --git a/checks/pinned_dependencies.go b/checks/pinned_dependencies.go index 2bd5854e..f637b6d3 100644 --- a/checks/pinned_dependencies.go +++ b/checks/pinned_dependencies.go @@ -154,19 +154,19 @@ func createReturnValuesForGitHubActionsWorkflowPinned(r worklowPinningResult, in if r.gitHubOwned != notPinned { score += 2 - // TODO: set Snippet and line numbers. dl.Info3(&checker.LogMessage{ - Type: checker.FileTypeSource, - Text: fmt.Sprintf("%s %s", "GitHub-owned", infoMsg), + Type: checker.FileTypeSource, + Offset: checker.OffsetDefault, + Text: fmt.Sprintf("%s %s", "GitHub-owned", infoMsg), }) } if r.thirdParties != notPinned { score += 8 - // TODO: set Snippet and line numbers. dl.Info3(&checker.LogMessage{ - Type: checker.FileTypeSource, - Text: fmt.Sprintf("%s %s", "Third-party", infoMsg), + Type: checker.FileTypeSource, + Offset: checker.OffsetDefault, + Text: fmt.Sprintf("%s %s", "Third-party", infoMsg), }) } diff --git a/checks/sast.go b/checks/sast.go index af70b6cd..bfffd13f 100644 --- a/checks/sast.go +++ b/checks/sast.go @@ -184,8 +184,7 @@ func codeQLInCheckDefinitions(c *checker.CheckRequest) (int, error) { c.Dlogger.Debug3(&checker.LogMessage{ Path: result.Path, Type: checker.FileTypeSource, - // Source file must have line number > 0. - Offset: 1, + Offset: checker.OffsetDefault, Text: "CodeQL detected", }) } diff --git a/checks/security_policy.go b/checks/security_policy.go index 8ac829e1..a0109d7d 100644 --- a/checks/security_policy.go +++ b/checks/security_policy.go @@ -46,20 +46,18 @@ func SecurityPolicy(c *checker.CheckRequest) checker.CheckResult { strings.EqualFold(name, ".github/security.md") || strings.EqualFold(name, "docs/security.md") { c.Dlogger.Info3(&checker.LogMessage{ - Path: name, - Type: checker.FileTypeSource, - // Source file must have line number > 0. - Offset: 1, + Path: name, + Type: checker.FileTypeSource, + Offset: checker.OffsetDefault, Text: "security policy detected", }) *pdata = true return false, nil } else if isSecurityRstFound(name) { c.Dlogger.Info3(&checker.LogMessage{ - Path: name, - Type: checker.FileTypeSource, - // Source file must have line number > 0. - Offset: 1, + Path: name, + Type: checker.FileTypeSource, + Offset: checker.OffsetDefault, Text: "security policy detected", }) *pdata = true @@ -97,10 +95,9 @@ func SecurityPolicy(c *checker.CheckRequest) checker.CheckResult { strings.EqualFold(name, ".github/security.md") || strings.EqualFold(name, "docs/security.md") { dl.Info3(&checker.LogMessage{ - Path: name, - Type: checker.FileTypeSource, - // Source file must have line number > 0. - Offset: 1, + Path: name, + Type: checker.FileTypeSource, + Offset: checker.OffsetDefault, Text: "security policy detected in .github folder", }) *pdata = true