diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 0b55fcf..67daa41 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -55,11 +55,10 @@ tests to the `rules_test.go` file. The details depend on whether it's a rule for with `uses:`) or for other steps, but defining the rule is the same regardless. To define a rule you need to create an instance of the `rule` type. This involves giving the rule an -id, title, and description as well as a function to extract what needs to be analyzed and a function -that builds a suggestion for fixing a violation. The id, title, and description are simple text -values. The extraction function needs to return a string to be analyzed for a given `JobStep`. The -suggestion functions needs to return a suggestion string for a given `Violation`. Lastly, you need -to add the rule to the `RULES.md` documentation file in the same format as the `--explain` output. +id, title, and description as well as a function to extract what needs to be analyzed. The id, +title, and description are simple text values. The extraction function needs to return a string to +be analyzed for a given `JobStep`. Lastly, you need to add the rule to the `RULES.md` documentation +file in the same format as the `--explain` output. Note that if multiple things could be checked for one action or step construct, they should be defined as separate rules. @@ -78,6 +77,6 @@ the `stepRule` type. You also need to add the rule to the `stepRules` slice. ### Testing Every new rule needs to be tested. The rule id, title, and description are tested automatically. The -`appliesTo`, `extractFrom`, and `suggestion` functions require dedicated unit tests. For this, it is -recommended to follow the lead of the tests for existing rules. Additionally, a test case should be -added to the `test/rules.txtar` test file. +`appliesTo` and `extractFrom` functions require dedicated unit tests. For this, it is recommended to +follow the lead of the tests for existing rules. Additionally, a test case should be added to the +`test/rules.txtar` test file. diff --git a/rules.go b/rules.go index 7795a02..3a270ac 100644 --- a/rules.go +++ b/rules.go @@ -25,7 +25,6 @@ import ( type rule struct { extractFrom func(step *JobStep) string - suggestion func(violation *Violation) string fix func(violation *Violation) []fix id string title string @@ -83,7 +82,6 @@ it can be made safer by converting it into: extractFrom: func(step *JobStep) string { return step.With["script"] }, - suggestion: suggestJavaScriptEnv, }, } @@ -101,9 +99,6 @@ upgrade the action to a non-vulnerable version.`, extractFrom: func(step *JobStep) string { return step.With["summary"] }, - suggestion: func(_ *Violation) string { - return " 1. Upgrade to a non-vulnerable version, see GHSA-4xqx-pqpj-9fqw" - }, }, } @@ -121,9 +116,6 @@ mitigate this, upgrade the action to a non-vulnerable version.`, extractFrom: func(step *JobStep) string { return step.With["tag"] }, - suggestion: func(_ *Violation) string { - return " 1. Upgrade to a non-vulnerable version, see GHSA-hgx2-4pp9-357g" - }, }, } @@ -141,9 +133,6 @@ this, upgrade the action to a non-vulnerable version.`, extractFrom: func(step *JobStep) string { return step.With["sha"] }, - suggestion: func(_ *Violation) string { - return " 1. Upgrade to a non-vulnerable version, see v1.2.0 release notes" - }, }, } @@ -179,7 +168,6 @@ it can be made safer by converting it into: extractFrom: func(step *JobStep) string { return step.With["issue-close-message"] }, - suggestion: suggestJavaScriptLiteralEnv, fix: func(violation *Violation) []fix { var step JobStep switch source := (violation.source).(type) { @@ -238,7 +226,6 @@ it can be made safer by converting it into: extractFrom: func(step *JobStep) string { return step.With["pr-close-message"] }, - suggestion: suggestJavaScriptLiteralEnv, }, } @@ -276,7 +263,6 @@ it can be made safer by converting it into: extractFrom: func(step *JobStep) string { return step.With["cmd"] }, - suggestion: suggestShellEnv, }, } @@ -333,7 +319,6 @@ it can be made safer by converting it into: extractFrom: func(step *JobStep) string { return step.Run }, - suggestion: suggestShellEnv, }, } @@ -371,17 +356,6 @@ func Explain(ruleId string) (string, error) { return explanation, nil } -// Suggestion returns a suggestion for the violation. -func Suggestion(violation *Violation) (string, error) { - ruleId := violation.RuleId - r, err := findRule(ruleId) - if err != nil { - return "", err - } - - return r.suggestion(violation), nil -} - // Fix produces a set of fixes to address the violation if possible. If the return value is nil the // violation cannot be fixed automatically. func Fix(violation *Violation) ([]fix, error) { @@ -452,33 +426,6 @@ func fixReplaceIn(s, old, new string) fix { } } -func suggestJavaScriptEnv(violation *Violation) string { - return suggestUseEnv(violation.Problem, "process.env.", "") -} - -func suggestJavaScriptLiteralEnv(violation *Violation) string { - return suggestUseEnv(violation.Problem, "${process.env.", "}") -} - -func suggestShellEnv(violation *Violation) string { - return suggestUseEnv(violation.Problem, "$", "") -} - -func suggestUseEnv(problem, pre, post string) string { - var sb strings.Builder - - name := getVariableNameForExpression(problem) - replacement := pre + name + post - - sb.WriteString(fmt.Sprintf(" 1. Set `%s: %s` in the step's `env` map", name, problem)) - sb.WriteRune('\n') - sb.WriteString(fmt.Sprintf(" 2. Replace all occurrences of `%s` by `%s`", problem, replacement)) - sb.WriteRune('\n') - sb.WriteString(" (make sure to keep the behavior of the script the same)") - - return sb.String() -} - func getVariableNameForExpression(expression string) string { name := expression[strings.LastIndex(expression, ".")+1:] name = strings.TrimRight(name, "}") diff --git a/rules_test.go b/rules_test.go index d5852f8..c82d06f 100644 --- a/rules_test.go +++ b/rules_test.go @@ -53,24 +53,6 @@ func TestActionRuleActionsGithubScript(t *testing.T) { t.Error(err) } }) - - t.Run("Suggestion", func(t *testing.T) { - violation := Violation{ - JobId: "4", - StepId: "2", - Problem: "${{ foo.bar }}", - RuleId: "ADES101", - } - - got := actionRuleActionsGitHubScript.rule.suggestion(&violation) - want := ` 1. Set ` + "`" + `BAR: ${{ foo.bar }}` + "`" + ` in the step's ` + "`" + `env` + "`" + ` map - 2. Replace all occurrences of ` + "`" + `${{ foo.bar }}` + "`" + ` by ` + "`" + `process.env.BAR` + "`" + ` - (make sure to keep the behavior of the script the same)` - - if got != want { - t.Errorf("Unexpected suggestion (got %q, want %q)", got, want) - } - }) } func TestActionRuleAtlassianGajiraCreate(t *testing.T) { @@ -135,22 +117,6 @@ func TestActionRuleAtlassianGajiraCreate(t *testing.T) { t.Error(err) } }) - - t.Run("Suggestion", func(t *testing.T) { - violation := Violation{ - JobId: "4", - StepId: "2", - Problem: "${{ foo.bar }}", - RuleId: "ADES202", - } - - got := actionRuleAtlassianGajiraCreate.rule.suggestion(&violation) - want := " 1. Upgrade to a non-vulnerable version, see GHSA-4xqx-pqpj-9fqw" - - if got != want { - t.Errorf("Unexpected suggestion (got %q, want %q)", got, want) - } - }) } func TestActionRuleEriccornelissenGitTagAnnotationAction(t *testing.T) { @@ -215,22 +181,6 @@ func TestActionRuleEriccornelissenGitTagAnnotationAction(t *testing.T) { t.Error(err) } }) - - t.Run("Suggestion", func(t *testing.T) { - violation := Violation{ - JobId: "4", - StepId: "2", - Problem: "${{ foo.bar }}", - RuleId: "ADES200", - } - - got := actionRuleEriccornelissenGitTagAnnotationAction.rule.suggestion(&violation) - want := " 1. Upgrade to a non-vulnerable version, see GHSA-hgx2-4pp9-357g" - - if got != want { - t.Errorf("Unexpected suggestion (got %q, want %q)", got, want) - } - }) } func TestActionRuleKcebGitMessageAction(t *testing.T) { @@ -295,22 +245,6 @@ func TestActionRuleKcebGitMessageAction(t *testing.T) { t.Error(err) } }) - - t.Run("Suggestion", func(t *testing.T) { - violation := Violation{ - JobId: "4", - StepId: "2", - Problem: "${{ foo.bar }}", - RuleId: "ADES201", - } - - got := actionRuleKcebGitMessageAction.rule.suggestion(&violation) - want := " 1. Upgrade to a non-vulnerable version, see v1.2.0 release notes" - - if got != want { - t.Errorf("Unexpected suggestion (got %q, want %q)", got, want) - } - }) } func TestActionRulesRootsIssueCloser(t *testing.T) { @@ -344,24 +278,6 @@ func TestActionRulesRootsIssueCloser(t *testing.T) { t.Error(err) } }) - - t.Run("Suggestion", func(t *testing.T) { - violation := Violation{ - JobId: "3", - StepId: "14", - Problem: "${{ hello.world }}", - RuleId: "ADES102", - } - - got := actionRuleRootsIssueCloserIssueCloseMessage.rule.suggestion(&violation) - want := ` 1. Set ` + "`" + `WORLD: ${{ hello.world }}` + "`" + ` in the step's ` + "`" + `env` + "`" + ` map - 2. Replace all occurrences of ` + "`" + `${{ hello.world }}` + "`" + ` by ` + "`" + `${process.env.WORLD}` + "`" + ` - (make sure to keep the behavior of the script the same)` - - if got != want { - t.Errorf("Unexpected suggestion (got %q, want %q)", got, want) - } - }) }) t.Run("pr-close-message", func(t *testing.T) { @@ -394,24 +310,6 @@ func TestActionRulesRootsIssueCloser(t *testing.T) { t.Error(err) } }) - - t.Run("Suggestion", func(t *testing.T) { - violation := Violation{ - JobId: "3", - StepId: "14", - Problem: "${{ hello.world }}", - RuleId: "ADES103", - } - - got := actionRuleRootsIssueCloserPrCloseMessage.rule.suggestion(&violation) - want := ` 1. Set ` + "`" + `WORLD: ${{ hello.world }}` + "`" + ` in the step's ` + "`" + `env` + "`" + ` map - 2. Replace all occurrences of ` + "`" + `${{ hello.world }}` + "`" + ` by ` + "`" + `${process.env.WORLD}` + "`" + ` - (make sure to keep the behavior of the script the same)` - - if got != want { - t.Errorf("Unexpected suggestion (got %q, want %q)", got, want) - } - }) }) } @@ -444,24 +342,6 @@ func TestActionRuleSergeysovaJqAction(t *testing.T) { t.Error(err) } }) - - t.Run("Suggestion", func(t *testing.T) { - violation := Violation{ - JobId: "3", - StepId: "14", - Problem: "${{ github.event.inputs.file }}", - RuleId: "ADES104", - } - - got := actionRuleSergeysovaJqAction.rule.suggestion(&violation) - want := ` 1. Set ` + "`" + `FILE: ${{ github.event.inputs.file }}` + "`" + ` in the step's ` + "`" + `env` + "`" + ` map - 2. Replace all occurrences of ` + "`" + `${{ github.event.inputs.file }}` + "`" + ` by ` + "`" + `$FILE` + "`" + ` - (make sure to keep the behavior of the script the same)` - - if got != want { - t.Errorf("Unexpected suggestion (got %q, want %q)", got, want) - } - }) } func TestStepRuleRun(t *testing.T) { @@ -501,24 +381,6 @@ func TestStepRuleRun(t *testing.T) { t.Error(err) } }) - - t.Run("Suggestion", func(t *testing.T) { - violation := Violation{ - JobId: "4", - StepId: "2", - Problem: "${{ foo.bar }}", - RuleId: "ADES100", - } - - got := stepRuleRun.rule.suggestion(&violation) - want := ` 1. Set ` + "`" + `BAR: ${{ foo.bar }}` + "`" + ` in the step's ` + "`" + `env` + "`" + ` map - 2. Replace all occurrences of ` + "`" + `${{ foo.bar }}` + "`" + ` by ` + "`" + `$BAR` + "`" + ` - (make sure to keep the behavior of the script the same)` - - if got != want { - t.Errorf("Unexpected suggestion (got %q, want %q)", got, want) - } - }) } func TestAllRules(t *testing.T) { @@ -663,55 +525,6 @@ func TestFix(t *testing.T) { }) } -func TestSuggestion(t *testing.T) { - t.Run("Existing rules", func(t *testing.T) { - testCases := allRules() - - for _, tt := range testCases { - t.Run(tt.id, func(t *testing.T) { - t.Parallel() - - violation := Violation{ - RuleId: tt.id, - } - - suggestion, err := Suggestion(&violation) - if err != nil { - t.Fatalf("Unexpected error: %#v", err) - } - - if suggestion == "" { - t.Error("Unexpected empty suggestion") - } - }) - } - }) - - t.Run("Missing rules", func(t *testing.T) { - type TestCase = string - - testCases := map[string]TestCase{ - "valid but unknown id": "ADES000", - "invalid id": "foobar", - } - - for name, tt := range testCases { - t.Run(name, func(t *testing.T) { - t.Parallel() - - violation := Violation{ - RuleId: tt, - } - - _, err := Suggestion(&violation) - if err == nil { - t.Fatal("Expected an error, got none") - } - }) - } - }) -} - func TestFindRule(t *testing.T) { t.Run("Action rules", func(t *testing.T) { for _, testCases := range actionRules { diff --git a/test/stdout.txtar b/test/stdout.txtar index cac4cdf..397e055 100644 --- a/test/stdout.txtar +++ b/test/stdout.txtar @@ -36,7 +36,7 @@ cmp stdout $WORK/snapshots/stdin-stdout.txt # Suggestions ! exec ades -suggestions 'project/.github/workflows/workflow.yml' -cmp stderr $WORK/snapshots/suggestion-stdout.txt +cmp stderr $WORK/snapshots/suggestion-stderr.txt # Not found ! exec ades 'does-not-exist' @@ -116,7 +116,7 @@ Detected 2 violation(s) in "stdin": step "Unsafe GitHub script" contains "${{ inputs.name }}" (ADES101) Use -explain for more details and fix suggestions (example: 'ades -explain ADES100') --- snapshots/suggestion-stdout.txt -- +-- snapshots/suggestion-stderr.txt -- The -suggestions flag is deprecated and will be removed in the future -- snapshots/workflow-stdout.txt --