From f35690fdd1a167b7a015ddc2b06e55316ec20a43 Mon Sep 17 00:00:00 2001 From: Max Leske <250711+theseion@users.noreply.github.com> Date: Sat, 1 Jun 2024 14:41:35 +0200 Subject: [PATCH] feat: improve file loading robustness (#310) Until now, go-ftw didn't know what to do with YAML files that were not test files. With the new platform override files (also YAML), it is very easy to also load them when globbing. This commit improves the output for such issues and continues execution, ignoring any files that can't be loaded. --- cmd/check_test.go | 2 - cmd/run_test.go | 2 - go.sum | 2 - runner/run.go | 1 - runner/run_test.go | 10 --- runner/testdata/TestBrokenOverrideRun.yaml | 2 - .../testdata/TestBrokenPortOverrideRun.yaml | 2 - runner/testdata/TestCloudRun.yaml | 2 - runner/testdata/TestDisabledRun.yaml | 18 ---- runner/testdata/TestFailedTestsRun.yaml | 2 - runner/testdata/TestIgnoredTestsRun.yaml | 2 - runner/testdata/TestLogsRun.yaml | 2 - runner/testdata/TestOverrideRun.yaml | 2 - runner/testdata/TestRetryOnce.yaml | 2 - runner/testdata/TestRunMultipleMatches.yaml | 2 - runner/testdata/TestRunTests_Run.yaml | 2 - test/files.go | 8 +- test/files_test.go | 59 ++++++------- .../TestCheckBenchmarkCheckFiles.yaml | 2 - test/types.go | 21 +++-- test/types_test.go | 6 -- test/yaml.go | 85 ++++++++++++------- 22 files changed, 100 insertions(+), 136 deletions(-) delete mode 100644 runner/testdata/TestDisabledRun.yaml diff --git a/cmd/check_test.go b/cmd/check_test.go index 0ecdc9a9..3a6b868e 100644 --- a/cmd/check_test.go +++ b/cmd/check_test.go @@ -16,8 +16,6 @@ import ( var checkFileContents = `--- meta: author: "go-ftw" - enabled: true - name: "mock-TestRunTests_Run.yaml" description: "Test file for go-ftw" tests: - # Standard GET request diff --git a/cmd/run_test.go b/cmd/run_test.go index cae1dcd6..2e22ffa3 100644 --- a/cmd/run_test.go +++ b/cmd/run_test.go @@ -23,8 +23,6 @@ import ( var testFileContentsTemplate = `--- meta: author: "go-ftw" - enabled: true - name: "mock-TestRunTests_Run.yaml" description: "Test file for go-ftw" tests: - # Standard GET request diff --git a/go.sum b/go.sum index abb508f2..f1545f15 100644 --- a/go.sum +++ b/go.sum @@ -84,8 +84,6 @@ github.com/pkg/errors v0.9.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINE github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/rs/xid v1.5.0/go.mod h1:trrq9SKmegXys3aeAKXMUTdJsYXVwGY3RLcfgqegfbg= -github.com/rs/zerolog v1.32.0 h1:keLypqrlIjaFsbmJOBdB/qvyF8KEtCWHwobLp5l/mQ0= -github.com/rs/zerolog v1.32.0/go.mod h1:/7mN4D5sKwJLZQ2b/znpjC3/GQWY/xaDXUM0kKWRHss= github.com/rs/zerolog v1.33.0 h1:1cU2KZkvPxNyfgEmhHAz/1A9Bz+llsdYzklWFzgp0r8= github.com/rs/zerolog v1.33.0/go.mod h1:/7mN4D5sKwJLZQ2b/znpjC3/GQWY/xaDXUM0kKWRHss= github.com/russross/blackfriday/v2 v2.1.0/go.mod h1:+Rmxgy9KzJVeS9/2gXHxylqXiyQDYRxCVz55jmeOWTM= diff --git a/runner/run.go b/runner/run.go index af443741..d18f23a1 100644 --- a/runner/run.go +++ b/runner/run.go @@ -86,7 +86,6 @@ func RunTest(runContext *TestRunContext, ftwTest *test.FTWTest) error { // if we received a particular test ID, skip until we find it if needToSkipTest(runContext.Include, runContext.Exclude, &testCase) { runContext.Stats.addResultToStats(Skipped, &testCase) - log.Trace().Msgf("\tskipping %s", testCase.IdString()) continue } test.ApplyPlatformOverrides(runContext.Config, &testCase) diff --git a/runner/run_test.go b/runner/run_test.go index 555940ea..8870844e 100644 --- a/runner/run_test.go +++ b/runner/run_test.go @@ -34,9 +34,6 @@ var testConfigMap = map[string]string{ testoverride: ignore: "920400-1": "This test result must be ignored" -`, - "TestDisabledRun": `--- -mode: 'cloud' `, "TestBrokenOverrideRun": `--- testoverride: @@ -96,7 +93,6 @@ testoverride: var destinationMap = map[string]string{ "TestBrokenOverrideRun": "http://example.com:1234", - "TestDisabledRun": "http://example.com:1234", } type runTestSuite struct { @@ -352,12 +348,6 @@ func (s *runTestSuite) TestBrokenPortOverrideRun() { s.LessOrEqual(0, res.Stats.TotalFailed(), "Oops, test run failed!") } -func (s *runTestSuite) TestDisabledRun() { - res, err := Run(s.cfg, s.ftwTests, RunnerConfig{}, s.out) - s.Require().NoError(err) - s.LessOrEqual(0, res.Stats.TotalFailed(), "Oops, test run failed!") -} - func (s *runTestSuite) TestLogsRun() { res, err := Run(s.cfg, s.ftwTests, RunnerConfig{}, s.out) s.Require().NoError(err) diff --git a/runner/testdata/TestBrokenOverrideRun.yaml b/runner/testdata/TestBrokenOverrideRun.yaml index e7da3d7a..a4699a6d 100644 --- a/runner/testdata/TestBrokenOverrideRun.yaml +++ b/runner/testdata/TestBrokenOverrideRun.yaml @@ -1,8 +1,6 @@ --- meta: author: "tester" - enabled: true - name: "TestBrokenOverrideRun.yaml" description: "Example Override Test" tests: - test_id: 1 diff --git a/runner/testdata/TestBrokenPortOverrideRun.yaml b/runner/testdata/TestBrokenPortOverrideRun.yaml index 484a8f1c..a4699a6d 100644 --- a/runner/testdata/TestBrokenPortOverrideRun.yaml +++ b/runner/testdata/TestBrokenPortOverrideRun.yaml @@ -1,8 +1,6 @@ --- meta: author: "tester" - enabled: true - name: "TestBrokenPortOverrideRun.yaml" description: "Example Override Test" tests: - test_id: 1 diff --git a/runner/testdata/TestCloudRun.yaml b/runner/testdata/TestCloudRun.yaml index 1a6c3fed..ef53d3d7 100644 --- a/runner/testdata/TestCloudRun.yaml +++ b/runner/testdata/TestCloudRun.yaml @@ -1,8 +1,6 @@ --- meta: author: "tester" - enabled: true - name: "TestCloudRun.yaml" description: "Example Test" tests: - test_id: 1 diff --git a/runner/testdata/TestDisabledRun.yaml b/runner/testdata/TestDisabledRun.yaml deleted file mode 100644 index bf1a03d2..00000000 --- a/runner/testdata/TestDisabledRun.yaml +++ /dev/null @@ -1,18 +0,0 @@ ---- -meta: - author: "tester" - enabled: false - name: "TestDisabledRun.yaml" - description: "we do not care, this test is disabled" -tests: - - test_id: 1 - description: "access real external site" - stages: - - input: - dest_addr: "{{ .TestAddr }}" - port: {{ .TestPort }} - headers: - User-Agent: "ModSecurity CRS 3 Tests" - Host: "{{ .TestAddr }}" - output: - status: 1234 diff --git a/runner/testdata/TestFailedTestsRun.yaml b/runner/testdata/TestFailedTestsRun.yaml index d1f44f44..1c33145a 100644 --- a/runner/testdata/TestFailedTestsRun.yaml +++ b/runner/testdata/TestFailedTestsRun.yaml @@ -1,8 +1,6 @@ --- meta: author: "tester" - enabled: true - name: "TestFailedTestsRun.yaml" description: "Example Test" tests: - test_id: 990 diff --git a/runner/testdata/TestIgnoredTestsRun.yaml b/runner/testdata/TestIgnoredTestsRun.yaml index f8248275..f182d47a 100644 --- a/runner/testdata/TestIgnoredTestsRun.yaml +++ b/runner/testdata/TestIgnoredTestsRun.yaml @@ -1,8 +1,6 @@ --- meta: author: "tester" - enabled: true - name: "TestIgnoredTestsRun.yaml" description: "Example Test" tests: - test_id: 1 diff --git a/runner/testdata/TestLogsRun.yaml b/runner/testdata/TestLogsRun.yaml index 94e077d5..00ceddd0 100644 --- a/runner/testdata/TestLogsRun.yaml +++ b/runner/testdata/TestLogsRun.yaml @@ -1,8 +1,6 @@ --- meta: author: "tester" - enabled: true - name: "TestLogsRun.yaml" description: "Example Test" tests: - test_id: 200 diff --git a/runner/testdata/TestOverrideRun.yaml b/runner/testdata/TestOverrideRun.yaml index 88ecf01a..d8fc3211 100644 --- a/runner/testdata/TestOverrideRun.yaml +++ b/runner/testdata/TestOverrideRun.yaml @@ -1,8 +1,6 @@ --- meta: author: "tester" - enabled: true - name: "TestOverrideRun.yaml" description: "Example Override Test" tests: - test_id: 1 diff --git a/runner/testdata/TestRetryOnce.yaml b/runner/testdata/TestRetryOnce.yaml index 0c69d532..74faa867 100644 --- a/runner/testdata/TestRetryOnce.yaml +++ b/runner/testdata/TestRetryOnce.yaml @@ -1,8 +1,6 @@ --- meta: author: "tester" - enabled: true - name: "TestRetryOnce.yaml" description: "Example Test" tests: - test_id: 1 diff --git a/runner/testdata/TestRunMultipleMatches.yaml b/runner/testdata/TestRunMultipleMatches.yaml index 710750f2..da93472d 100644 --- a/runner/testdata/TestRunMultipleMatches.yaml +++ b/runner/testdata/TestRunMultipleMatches.yaml @@ -1,8 +1,6 @@ --- meta: author: "tester" - enabled: true - name: "TestRunMultipleMatches.yaml" description: "Example Test with multiple expected outputs per single rule" tests: - test_id: 1 diff --git a/runner/testdata/TestRunTests_Run.yaml b/runner/testdata/TestRunTests_Run.yaml index 8582848a..f182d47a 100644 --- a/runner/testdata/TestRunTests_Run.yaml +++ b/runner/testdata/TestRunTests_Run.yaml @@ -1,8 +1,6 @@ --- meta: author: "tester" - enabled: true - name: "TestRunTests_Run.yaml" description: "Example Test" tests: - test_id: 1 diff --git a/test/files.go b/test/files.go index 26c78d03..57cf3fd5 100644 --- a/test/files.go +++ b/test/files.go @@ -30,16 +30,18 @@ func GetTestsFromFiles(globPattern string) ([]*FTWTest, error) { } for _, filePath := range testFiles { + fileName := path.Base(filePath) + log.Trace().Msgf("Loading %s", fileName) yamlString, err := readFileContents(filePath) if err != nil { return tests, err } - ftwTest, err := GetTestFromYaml(yamlString, path.Base(filePath)) + ftwTest, err := GetTestFromYaml(yamlString, fileName) if err != nil { - log.Error().Msgf("Problem detected in file %s:\n%s\n%s", + log.Warn().Msgf("Problem detected in file %s:\n%s\n%s", filePath, yaml.FormatError(err, true, true), DescribeYamlError(err)) - return tests, err + continue } tests = append(tests, ftwTest) diff --git a/test/files_test.go b/test/files_test.go index e0097561..e414da70 100644 --- a/test/files_test.go +++ b/test/files_test.go @@ -14,36 +14,33 @@ import ( var yamlTest = ` --- - meta: - author: "tester" - enabled: true - name: "911100.yaml" - description: "Description" - rule_id: 911100 - tests: - - test_id: 1 - stages: - - input: - autocomplete_headers: false - dest_addr: "127.0.0.1" - port: 80 - headers: - User-Agent: "ModSecurity CRS 3 Tests" - Host: "localhost" - output: - no_log_contains: "id \"911100\"" - - - test_id: 2 - stages: - - input: - dest_addr: "127.0.0.1" - port: 80 - method: "OPTIONS" - headers: - User-Agent: "ModSecurity CRS 3 Tests" - Host: "localhost" - output: - no_log_contains: "id \"911100\"" +meta: + author: "tester" + description: "Description" +rule_id: 911100 +tests: + - test_id: 1 + stages: + - input: + autocomplete_headers: false + dest_addr: "127.0.0.1" + port: 80 + headers: + User-Agent: "ModSecurity CRS 3 Tests" + Host: "localhost" + output: + no_log_contains: "id \"911100\"" + - test_id: 2 + stages: + - input: + dest_addr: "127.0.0.1" + port: 80 + method: "OPTIONS" + headers: + User-Agent: "ModSecurity CRS 3 Tests" + Host: "localhost" + output: + no_log_contains: "id \"911100\"" ` var wrongYamlTest = ` @@ -64,7 +61,7 @@ func (s *filesTestSuite) TestGetTestFromYAML() { for _, ft := range tests { s.Equal("tester", ft.Meta.Author) - s.Equal("911100.yaml", ft.Meta.Name) + s.Equal("Description", ft.Meta.Description) re := regexp.MustCompile("911100.*") diff --git a/test/testdata/TestCheckBenchmarkCheckFiles.yaml b/test/testdata/TestCheckBenchmarkCheckFiles.yaml index 18636ed5..392761d6 100644 --- a/test/testdata/TestCheckBenchmarkCheckFiles.yaml +++ b/test/testdata/TestCheckBenchmarkCheckFiles.yaml @@ -1,8 +1,6 @@ --- meta: author: "csanders-git, Franziska Bühler" - enabled: true - name: "920420.yaml" description: "Description" rule_id: 920420 tests: diff --git a/test/types.go b/test/types.go index f8d3d263..59ddd5d2 100644 --- a/test/types.go +++ b/test/types.go @@ -4,6 +4,7 @@ package test import ( + "fmt" "regexp" "strconv" @@ -125,34 +126,36 @@ func applyHeadersOverride(overrides *config.Overrides, input *Input) { } } -func postLoadTestFTWTest(ftwTest *FTWTest, fileName string) { +func postLoadTestFTWTest(ftwTest *FTWTest, fileName string) error { ftwTest.FileName = fileName - postLoadRuleId(ftwTest) + if err := postLoadRuleId(ftwTest); err != nil { + return err + } for index := 0; index < len(ftwTest.Tests); index++ { postLoadTest(ftwTest.RuleId, uint(index+1), &ftwTest.Tests[index]) } + return nil } -func postLoadRuleId(ftwTest *FTWTest) { +func postLoadRuleId(ftwTest *FTWTest) error { if ftwTest.RuleId > 0 { - return + return nil } if len(ftwTest.FileName) == 0 { - log.Fatal().Msg("The rule_id field is required for the top-level test structure") + return fmt.Errorf("the rule_id field is required for the top-level test structure") } else { ruleIdString := regexp.MustCompile(`\d+`).FindString(ftwTest.FileName) if len(ruleIdString) == 0 { - log.Fatal().Msg("Failed to fall back on filename to find rule ID of test. The rule_id field is required for the top-level test structure") - return + return fmt.Errorf("failed to fall back on filename to find rule ID of test. The rule_id field is required for the top-level test structure") } ruleId, err := strconv.ParseUint(ruleIdString, 10, 0) if err != nil { - log.Fatal().Msgf("failed to parse rule ID from filename '%s'", ftwTest.FileName) - return + return fmt.Errorf("failed to parse rule ID from filename '%s'", ftwTest.FileName) } ftwTest.RuleId = uint(ruleId) } + return nil } func postLoadTest(ruleId uint, testId uint, test *schema.Test) { test.RuleId = ruleId diff --git a/test/types_test.go b/test/types_test.go index 9ef013f8..82fbdda6 100644 --- a/test/types_test.go +++ b/test/types_test.go @@ -20,8 +20,6 @@ func TestTypesTestSuite(t *testing.T) { var autocompleteHeadersDefaultYaml = `--- meta: author: "tester" - enabled: true - name: "gotest-ftw.yaml" description: "Example Test" rule_id: 123456 tests: @@ -68,8 +66,6 @@ tests: var autocompleteHeadersFalseYaml = `--- meta: author: "tester" - enabled: true - name: "gotest-ftw.yaml" description: "Example Test" rule_id: 123456 tests: @@ -119,8 +115,6 @@ tests: var autocompleteHeadersTrueYaml = `--- meta: author: "tester" - enabled: true - name: "gotest-ftw.yaml" description: "Example Test" rule_id: 123456 tests: diff --git a/test/yaml.go b/test/yaml.go index b8fd890f..65c53939 100644 --- a/test/yaml.go +++ b/test/yaml.go @@ -9,49 +9,74 @@ import ( "github.com/goccy/go-yaml" ) +type errorMap struct { + matchers []string + explanation string +} + // GetTestFromYaml will get the tests to be processed from a YAML string. func GetTestFromYaml(testYaml []byte, fileName string) (ftwTest *FTWTest, err error) { ftwTest = &FTWTest{} err = yaml.Unmarshal(testYaml, ftwTest) if err != nil { - return &FTWTest{}, err + return nil, err } - postLoadTestFTWTest(ftwTest, fileName) + if err := postLoadTestFTWTest(ftwTest, fileName); err != nil { + return nil, err + } return ftwTest, nil } func DescribeYamlError(yamlError error) string { - matched, err := regexp.MatchString(`.*int was used where sequence is expected.*`, yamlError.Error()) - if err != nil { - return err.Error() - } - if matched { - return "\nTip: This might refer to a \"status\" line being '200', where it should be '[200]'.\n" + - "The default \"status\" is a list now.\n" + - "A simple example would be like this:\n\n" + - "status: 403\n" + - "needs to be changed to:\n\n" + - "status: 403\n\n" - } - matched, err = regexp.MatchString(`.*cannot unmarshal \[]interface {} into Go struct field FTWTest.Tests of type string.*`, yamlError.Error()) - if err != nil { - return err.Error() + errorMaps := []errorMap{ + { + matchers: []string{`.*int was used where sequence is expected.*`}, + explanation: "Tip: This might refer to a \"status\" line being '200', where it should be '[200]'.\n" + + "The default \"status\" is a list now.\n" + + "A simple example would be like this:\n\n" + + "status: 403\n" + + "needs to be changed to:\n\n" + + "status: 403", + }, + { + matchers: []string{`.*cannot unmarshal \[]interface {} into Go struct field FTWTest.Tests of type string.*`}, + explanation: "Tip: This might refer to \"data\" on the test being a list of strings instead of a proper YAML multiline.\n" + + "To fix this, convert this \"data\" string list to a multiline YAML and this will be fixed.\n" + + "A simple example would be like this:\n\n" + + "data:\n" + + " - 'Hello'\n" + + " - 'World'\n" + + "can be expressed as:\n\n" + + "data: |\n" + + " Hello\n" + + " World\n\n" + + "You can also remove single/double quotes from beggining and end of text, they are not needed. See https://yaml-multiline.info/ for additional help.", + }, + { + matchers: []string{ + "The rule_id field is required for the top-level test structure", + "Failed to fall back on filename to find rule ID of test. The rule_id field is required for the top-level test structure", + "failed to parse rule ID from filename ", + }, + explanation: "The `rule_id` field is missing from this file and the rule ID could not be determined otherwise.\n" + + "This might be a YAML file that is not a test.", + }, } - if matched { - return "\nTip: This might refer to \"data\" on the test being a list of strings instead of a proper YAML multiline.\n" + - "To fix this, convert this \"data\" string list to a multiline YAML and this will be fixed.\n" + - "A simple example would be like this:\n\n" + - "data:\n" + - " - 'Hello'\n" + - " - 'World'\n" + - "can be expressed as:\n\n" + - "data: |\n" + - " Hello\n" + - " World\n\n" + - "You can also remove single/double quotes from beggining and end of text, they are not needed. See https://yaml-multiline.info/ for additional help.\n" + errorMessage := yamlError.Error() + for _, em := range errorMaps { + for _, regex := range em.matchers { + matched, err := regexp.MatchString(regex, errorMessage) + if err != nil { + return err.Error() + } + if !matched { + continue + } + return "\n" + em.explanation + "\n\n" + } } - return "We do not have an extended explanation of this error." + return "\nWe do not have an extended explanation of this error\n\n" }