From e77fef7f87289e6b4d8a20c80cf2f7189a28a78d Mon Sep 17 00:00:00 2001 From: Eric Cornelissen Date: Sat, 14 Dec 2024 18:29:57 +0100 Subject: [PATCH] Improve the output of violations This improves upon the output of violations. It does so in a couple of ways. First it removes the inline suggestions - which become clutter for many results - in favor of recommending the use of `ades -explain` at the end of the human-readable report (also resolving the no-op of the `-suggestions` flag when `-json` is used). Second, it now groups the violations by the job in which they occur, reducing the amount of duplicated text in the output. Additionally this makes the order of the results in the human readable report deterministic, which is slightly nicer for humans because it is somewhat sensibly ordered, but notably resolves some flakiness in the tests. Signed-off-by: Eric Cornelissen --- RULES.md | 15 ++- cmd/ades/main.go | 26 ++++- cmd/ades/output.go | 53 ++++++---- cmd/ades/output_test.go | 186 ++++++++++++++++++++++++++--------- rules.go | 15 ++- test/explain.txtar | 4 +- test/manifest-workflow.txtar | 10 +- test/stdout.txtar | 45 +++++---- 8 files changed, 244 insertions(+), 110 deletions(-) diff --git a/RULES.md b/RULES.md index d9f46ba..dd82782 100644 --- a/RULES.md +++ b/RULES.md @@ -7,8 +7,8 @@ to address it. ## ADES100 - Expression in `run:` directive -When an expression appears in a `run:` directive you can avoid any potential attacks by extracting -the expression into an environment variable and using the environment variable instead. +When an expression appears in a `run:` directive you can avoid potential attacks by extracting the +expression into an environment variable and using the environment variable instead. For example, given the workflow snippet: @@ -34,9 +34,8 @@ it can be made safer by converting it into: ## ADES101 - Expression in `actions/github-script` script -When an expression appears in a `actions/github-script` script you can avoid any potential attacks -by extracting the expression into an environment variable and using the environment variable -instead. +When an expression appears in a `actions/github-script` script you can avoid potential attacks by +extracting the expression into an environment variable and using the environment variable instead. For example, given the workflow snippet: @@ -65,8 +64,8 @@ it can be made safer by converting it into: ## ADES102 - Expression in `roots/issue-closer` issue close message When an expression appears in the issue close message of `roots/issue-closer` it is interpreted as -an ES6-style template literal. You can avoid any potential attacks by extracting the expression into -an environment variable and using the environment variable instead. +an ES6-style template literal. You can avoid potential attacks by extracting the expression into an +environment variable and using the environment variable instead. For example, given the workflow snippet: @@ -93,7 +92,7 @@ it can be made safer by converting it into: ## ADES103 - Expression in `roots/issue-closer` pull request close message When an expression appears in the pull request close message of `roots/issue-closer` it is -interpreted as an ES6-style template literal. You can avoid any potential attacks by extracting the +interpreted as an ES6-style template literal. You can avoid potential attacks by extracting the expression into an environment variable and using the environment variable instead. For example, given the workflow snippet: diff --git a/cmd/ades/main.go b/cmd/ades/main.go index 049f0cf..b199a71 100644 --- a/cmd/ades/main.go +++ b/cmd/ades/main.go @@ -21,9 +21,12 @@ import ( "fmt" "io" "io/fs" + "maps" "os" "path/filepath" "regexp" + "slices" + "sort" "strings" "github.com/ericcornelissen/ades" @@ -64,7 +67,7 @@ var ( flagSuggestions = flag.Bool( "suggestions", false, - "Show suggested fixes inline", + "No effect (deprecated)", ) flagVersion = flag.Bool( "version", @@ -91,6 +94,11 @@ func run() int { return exitSuccess } + if *flagSuggestions { + fmt.Fprintln(os.Stderr, "The -suggestions flag is deprecated and will be removed in the future") + fmt.Fprintln(os.Stderr, "") + } + if *flagExplain != "" { explanation, err := ades.Explain(*flagExplain) if err != nil { @@ -139,8 +147,13 @@ func run() int { if *flagJson { fmt.Println(printJson(report)) } else { + targets := slices.Collect(maps.Keys(report)) + sort.Strings(targets) + separator := false - for target, violations := range report { + for _, target := range targets { + violations := report[target] + if separator { fmt.Println( /* empty line between targets */ ) } @@ -148,7 +161,7 @@ func run() int { fmt.Printf("[%s]\n", target) } - fmt.Print(printViolations(violations, *flagSuggestions)) + fmt.Print(printProjectViolations(violations)) separator = true } } @@ -156,6 +169,11 @@ func run() int { for _, targetViolations := range report { for _, fileViolations := range targetViolations { if len(fileViolations) > 0 { + if !(*flagJson) { + fmt.Println() + fmt.Println("Use -explain for more details and fix suggestions (example: 'ades -explain ADES100')") + } + return exitViolations } } @@ -417,7 +435,7 @@ Flags: -help Show this help message and exit. -json Output results in JSON format. -legal Show legal information and exit. - -suggestions Show suggested fixes inline. + -suggestions No effect (deprecated). -version Show the program version and exit. - Read workflow or manifest from stdin. diff --git a/cmd/ades/output.go b/cmd/ades/output.go index ceb348f..454dd5a 100644 --- a/cmd/ades/output.go +++ b/cmd/ades/output.go @@ -18,6 +18,8 @@ package main import ( "encoding/json" "fmt" + "maps" + "slices" "sort" "strings" @@ -60,19 +62,20 @@ func printJson(rawViolations map[string]map[string][]ades.Violation) string { return string(jsonBytes) } -func printViolations(violations map[string][]ades.Violation, suggestions bool) string { +func printProjectViolations(violations map[string][]ades.Violation) string { clean := true + files := slices.Collect(maps.Keys(violations)) + sort.Strings(files) + var sb strings.Builder - for file, fileViolations := range violations { + for _, file := range files { + fileViolations := violations[file] if cnt := len(fileViolations); cnt > 0 { clean = false sb.WriteString(fmt.Sprintf("Detected %d violation(s) in %q:", cnt, file)) sb.WriteRune('\n') - for _, violation := range fileViolations { - sb.WriteString(printViolation(&violation, suggestions)) - sb.WriteRune('\n') - } + sb.WriteString(printFileViolations(fileViolations)) } } @@ -83,21 +86,35 @@ func printViolations(violations map[string][]ades.Violation, suggestions bool) s } } -func printViolation(violation *ades.Violation, suggestions bool) string { - var sb strings.Builder - if violation.JobId == "" { - sb.WriteString(fmt.Sprintf(" step %q has %q", violation.StepId, violation.Problem)) - } else { - sb.WriteString(fmt.Sprintf(" job %q, step %q has %q", violation.JobId, violation.StepId, violation.Problem)) +func printFileViolations(violations []ades.Violation) string { + byJob := make(map[string][]ades.Violation, len(violations)) + for _, violation := range violations { + jobId := violation.JobId + if _, ok := byJob[jobId]; !ok { + byJob[jobId] = make([]ades.Violation, 0) + } + + byJob[jobId] = append(byJob[jobId], violation) } - if suggestions { - suggestion, _ := ades.Suggestion(violation) + jobs := slices.Collect(maps.Keys(byJob)) + sort.Strings(jobs) - sb.WriteString(", suggestion:\n") - sb.WriteString(suggestion) - } else { - sb.WriteString(fmt.Sprintf(" (%s)", violation.RuleId)) + var sb strings.Builder + for _, job := range jobs { + violations := byJob[job] + + if job != "" { + sb.WriteString(" ") + sb.WriteString(fmt.Sprintf("%d in job %q:", len(violations), job)) + sb.WriteRune('\n') + } + + for _, violation := range violations { + sb.WriteString(" ") + sb.WriteString(fmt.Sprintf("step %q contains %q (%s)", violation.StepId, violation.Problem, violation.RuleId)) + sb.WriteRune('\n') + } } return sb.String() diff --git a/cmd/ades/output_test.go b/cmd/ades/output_test.go index c96354e..d79bc7c 100644 --- a/cmd/ades/output_test.go +++ b/cmd/ades/output_test.go @@ -16,7 +16,6 @@ package main import ( - "strings" "testing" "github.com/ericcornelissen/ades" @@ -84,86 +83,179 @@ func TestPrintJson(t *testing.T) { } } -func TestPrintViolations(t *testing.T) { +func TestPrintProjectViolations(t *testing.T) { type TestCase struct { - name string - violations func() map[string][]ades.Violation - want string - wantSuggestions string + violations func() map[string][]ades.Violation + want string } - testCases := []TestCase{ - { - name: "No files", + testCases := map[string]TestCase{ + "No files": { violations: func() map[string][]ades.Violation { return make(map[string][]ades.Violation) }, want: `Ok -`, - wantSuggestions: `Ok `, }, - { - name: "File without violations", + "File without violations": { violations: func() map[string][]ades.Violation { - m := make(map[string][]ades.Violation) + m := make(map[string][]ades.Violation, 1) m["workflow.yml"] = make([]ades.Violation, 0) return m }, want: `Ok -`, - wantSuggestions: `Ok `, }, - { - name: "Workflow with a violation", + "Workflow with a violation": { violations: func() map[string][]ades.Violation { - m := make(map[string][]ades.Violation) - m["workflow.yml"] = make([]ades.Violation, 1) - m["workflow.yml"][0] = ades.Violation{ - JobId: "4", - StepId: "2", - Problem: "${{ foo.bar }}", - RuleId: "ADES100", + m := make(map[string][]ades.Violation, 1) + m["workflow.yml"] = []ades.Violation{ + { + JobId: "4", + StepId: "2", + Problem: "${{ foo.bar }}", + RuleId: "ADES100", + }, } + return m }, want: `Detected 1 violation(s) in "workflow.yml": - job "4", step "2" has "${{ foo.bar }}" (ADES100) + 1 in job "4": + step "2" contains "${{ foo.bar }}" (ADES100) `, - wantSuggestions: `Detected 1 violation(s) in "workflow.yml": - job "4", step "2" has "${{ foo.bar }}", suggestion:`, }, - { - name: "Manifest with a violation", + "Workflow with multiple violations in the same job": { violations: func() map[string][]ades.Violation { - m := make(map[string][]ades.Violation) - m["action.yml"] = make([]ades.Violation, 1) - m["action.yml"][0] = ades.Violation{ - StepId: "2", - Problem: "${{ foo.bar }}", - RuleId: "ADES100", + m := make(map[string][]ades.Violation, 1) + m["workflow.yml"] = []ades.Violation{ + { + JobId: "3", + StepId: "6", + Problem: "${{ foo.bar }}", + RuleId: "ADES100", + }, + { + JobId: "3", + StepId: "14", + Problem: "${{ foo.baz }}", + RuleId: "ADES101", + }, + } + + return m + }, + want: `Detected 2 violation(s) in "workflow.yml": + 2 in job "3": + step "6" contains "${{ foo.bar }}" (ADES100) + step "14" contains "${{ foo.baz }}" (ADES101) +`, + }, + "Workflow with multiple violations in different jobs": { + violations: func() map[string][]ades.Violation { + m := make(map[string][]ades.Violation, 1) + m["workflow.yml"] = []ades.Violation{ + { + JobId: "4", + StepId: "2", + Problem: "${{ foo.bar }}", + RuleId: "ADES100", + }, + { + JobId: "3", + StepId: "14", + Problem: "${{ foo.baz }}", + RuleId: "ADES101", + }, + } + + return m + }, + want: `Detected 2 violation(s) in "workflow.yml": + 1 in job "3": + step "14" contains "${{ foo.baz }}" (ADES101) + 1 in job "4": + step "2" contains "${{ foo.bar }}" (ADES100) +`, + }, + "Manifest with a violation": { + violations: func() map[string][]ades.Violation { + m := make(map[string][]ades.Violation, 1) + m["action.yml"] = []ades.Violation{ + { + StepId: "7", + Problem: "${{ foo.bar }}", + RuleId: "ADES100", + }, } + return m }, want: `Detected 1 violation(s) in "action.yml": - step "2" has "${{ foo.bar }}" (ADES100) + step "7" contains "${{ foo.bar }}" (ADES100) +`, + }, + "Manifest with multiple violations": { + violations: func() map[string][]ades.Violation { + m := make(map[string][]ades.Violation, 1) + m["action.yml"] = []ades.Violation{ + { + StepId: "4", + Problem: "${{ foo.bar }}", + RuleId: "ADES100", + }, + { + StepId: "2", + Problem: "${{ foo.baz }}", + RuleId: "ADES101", + }, + } + + return m + }, + want: `Detected 2 violation(s) in "action.yml": + step "4" contains "${{ foo.bar }}" (ADES100) + step "2" contains "${{ foo.baz }}" (ADES101) +`, + }, + "Project with multiple workflows": { + violations: func() map[string][]ades.Violation { + m := make(map[string][]ades.Violation, 2) + m["workflow-a.yml"] = []ades.Violation{ + { + JobId: "4", + StepId: "2", + Problem: "${{ foo.bar }}", + RuleId: "ADES100", + }, + } + m["workflow-b.yml"] = []ades.Violation{ + { + JobId: "3", + StepId: "14", + Problem: "${{ foo.baz }}", + RuleId: "ADES101", + }, + } + + return m + }, + want: `Detected 1 violation(s) in "workflow-a.yml": + 1 in job "4": + step "2" contains "${{ foo.bar }}" (ADES100) +Detected 1 violation(s) in "workflow-b.yml": + 1 in job "3": + step "14" contains "${{ foo.baz }}" (ADES101) `, - wantSuggestions: `Detected 1 violation(s) in "action.yml": - step "2" has "${{ foo.bar }}", suggestion:`, }, } - for _, tt := range testCases { - t.Run(tt.name, func(t *testing.T) { + for name, tt := range testCases { + t.Run(name, func(t *testing.T) { t.Parallel() - if got, want := printViolations(tt.violations(), false), tt.want; got != want { - t.Errorf("Unexpected output (got %q, want %q)", got, want) - } - - if got, prefix := printViolations(tt.violations(), true), tt.wantSuggestions; !strings.HasPrefix(got, prefix) { - t.Errorf("Unexpected prefix for output with suggestions (got %q, want %q)", got, prefix) + if got, want := printProjectViolations(tt.violations()), tt.want; got != want { + t.Errorf("Unexpected output\n=== GOT ===\n%s\n=== WANT ===\n%s", got, want) } }) } diff --git a/rules.go b/rules.go index c9c376f..50dc45e 100644 --- a/rules.go +++ b/rules.go @@ -58,9 +58,8 @@ var actionRuleActionsGitHubScript = actionRule{ id: "ADES101", title: "Expression in 'actions/github-script' script", description: ` -When an expression appears in a 'actions/github-script' script you can avoid any potential attacks -by extracting the expression into an environment variable and using the environment variable -instead. +When an expression appears in a 'actions/github-script' script you can avoid potential attacks by +extracting the expression into an environment variable and using the environment variable instead. For example, given the workflow snippet: @@ -137,8 +136,8 @@ var actionRuleRootsIssueCloserIssueCloseMessage = actionRule{ title: "Expression in 'roots/issue-closer' issue close message", description: ` When an expression appears in the issue close message of 'roots/issue-closer' it is interpreted as -an ES6-style template literal. You can avoid any potential attacks by extracting the expression into -an environment variable and using the environment variable instead. +an ES6-style template literal. You can avoid potential attacks by extracting the expression into an +environment variable and using the environment variable instead. For example, given the workflow snippet: @@ -196,7 +195,7 @@ var actionRuleRootsIssueCloserPrCloseMessage = actionRule{ title: "Expression in 'roots/issue-closer' pull request close message", description: ` When an expression appears in the pull request close message of 'roots/issue-closer' it is -interpreted as an ES6-style template literal. You can avoid any potential attacks by extracting the +interpreted as an ES6-style template literal. You can avoid potential attacks by extracting the expression into an environment variable and using the environment variable instead. For example, given the workflow snippet: @@ -288,8 +287,8 @@ var stepRuleRun = stepRule{ id: "ADES100", title: "Expression in 'run:' directive", description: ` -When an expression appears in a 'run:' directive you can avoid any potential attacks by extracting -the expression into an environment variable and using the environment variable instead. +When an expression appears in a 'run:' directive you can avoid potential attacks by extracting the +expression into an environment variable and using the environment variable instead. For example, given the workflow snippet: diff --git a/test/explain.txtar b/test/explain.txtar index bbfdf7f..0233c36 100644 --- a/test/explain.txtar +++ b/test/explain.txtar @@ -12,8 +12,8 @@ stderr 'Unknown rule "foobar"' -- snapshots/ades100-stdout.txt -- ADES100 - Expression in 'run:' directive -When an expression appears in a 'run:' directive you can avoid any potential attacks by extracting -the expression into an environment variable and using the environment variable instead. +When an expression appears in a 'run:' directive you can avoid potential attacks by extracting the +expression into an environment variable and using the environment variable instead. For example, given the workflow snippet: diff --git a/test/manifest-workflow.txtar b/test/manifest-workflow.txtar index 9d71674..28fb457 100644 --- a/test/manifest-workflow.txtar +++ b/test/manifest-workflow.txtar @@ -52,7 +52,13 @@ jobs: run: echo 'Hello ${{ inputs.name }}' -- snapshots/yml-stdout.txt -- Detected 1 violation(s) in ".github/workflows/action.yml": - job "Example unsafe job", step "Unsafe run" has "${{ inputs.name }}" (ADES100) + 1 in job "Example unsafe job": + step "Unsafe run" contains "${{ inputs.name }}" (ADES100) + +Use -explain for more details and fix suggestions (example: 'ades -explain ADES100') -- snapshots/yaml-stdout.txt -- Detected 1 violation(s) in ".github/workflows/action.yaml": - job "Example unsafe job", step "Unsafe run" has "${{ inputs.name }}" (ADES100) + 1 in job "Example unsafe job": + step "Unsafe run" contains "${{ inputs.name }}" (ADES100) + +Use -explain for more details and fix suggestions (example: 'ades -explain ADES100') diff --git a/test/stdout.txtar b/test/stdout.txtar index 938fd11..cac4cdf 100644 --- a/test/stdout.txtar +++ b/test/stdout.txtar @@ -36,8 +36,7 @@ cmp stdout $WORK/snapshots/stdin-stdout.txt # Suggestions ! exec ades -suggestions 'project/.github/workflows/workflow.yml' -cmp stdout $WORK/snapshots/suggestion-stdout.txt -! stderr . +cmp stderr $WORK/snapshots/suggestion-stdout.txt # Not found ! exec ades 'does-not-exist' @@ -95,31 +94,35 @@ jobs: {"problems":[{"target":"project/","file":".github/workflows/workflow.yml","job":"Example unsafe job","step":"Unsafe run","problem":"${{ inputs.name }}"},{"target":"project/","file":".github/workflows/workflow.yml","job":"Example unsafe job","step":"Unsafe GitHub script","problem":"${{ inputs.name }}"},{"target":"project/","file":"action.yml","job":"","step":"Unsafe run","problem":"${{ inputs.name }}"}]} -- snapshots/manifest-stdout.txt -- Detected 1 violation(s) in "project/action.yml": - step "Unsafe run" has "${{ inputs.name }}" (ADES100) + step "Unsafe run" contains "${{ inputs.name }}" (ADES100) + +Use -explain for more details and fix suggestions (example: 'ades -explain ADES100') -- snapshots/multiple-stdout.txt -- +[project/.github/workflows/workflow.yml] +Detected 2 violation(s) in "project/.github/workflows/workflow.yml": + 2 in job "Example unsafe job": + step "Unsafe run" contains "${{ inputs.name }}" (ADES100) + step "Unsafe GitHub script" contains "${{ inputs.name }}" (ADES101) + [project/action.yml] Detected 1 violation(s) in "project/action.yml": - step "Unsafe run" has "${{ inputs.name }}" (ADES100) + step "Unsafe run" contains "${{ inputs.name }}" (ADES100) -[project/.github/workflows/workflow.yml] -Detected 2 violation(s) in "project/.github/workflows/workflow.yml": - job "Example unsafe job", step "Unsafe run" has "${{ inputs.name }}" (ADES100) - job "Example unsafe job", step "Unsafe GitHub script" has "${{ inputs.name }}" (ADES101) +Use -explain for more details and fix suggestions (example: 'ades -explain ADES100') -- snapshots/stdin-stdout.txt -- Detected 2 violation(s) in "stdin": - job "Example unsafe job", step "Unsafe run" has "${{ inputs.name }}" (ADES100) - job "Example unsafe job", step "Unsafe GitHub script" has "${{ inputs.name }}" (ADES101) + 2 in job "Example unsafe job": + step "Unsafe run" contains "${{ inputs.name }}" (ADES100) + step "Unsafe GitHub script" contains "${{ inputs.name }}" (ADES101) + +Use -explain for more details and fix suggestions (example: 'ades -explain ADES100') -- snapshots/suggestion-stdout.txt -- -Detected 2 violation(s) in "project/.github/workflows/workflow.yml": - job "Example unsafe job", step "Unsafe run" has "${{ inputs.name }}", suggestion: - 1. Set `NAME: ${{ inputs.name }}` in the step's `env` map - 2. Replace all occurrences of `${{ inputs.name }}` by `$NAME` - (make sure to keep the behavior of the script the same) - job "Example unsafe job", step "Unsafe GitHub script" has "${{ inputs.name }}", suggestion: - 1. Set `NAME: ${{ inputs.name }}` in the step's `env` map - 2. Replace all occurrences of `${{ inputs.name }}` by `process.env.NAME` - (make sure to keep the behavior of the script the same) +The -suggestions flag is deprecated and will be removed in the future + -- snapshots/workflow-stdout.txt -- Detected 2 violation(s) in "project/.github/workflows/workflow.yml": - job "Example unsafe job", step "Unsafe run" has "${{ inputs.name }}" (ADES100) - job "Example unsafe job", step "Unsafe GitHub script" has "${{ inputs.name }}" (ADES101) + 2 in job "Example unsafe job": + step "Unsafe run" contains "${{ inputs.name }}" (ADES100) + step "Unsafe GitHub script" contains "${{ inputs.name }}" (ADES101) + +Use -explain for more details and fix suggestions (example: 'ades -explain ADES100')