From 5e1131318b1a32468dce42efafbee07531c1e1e5 Mon Sep 17 00:00:00 2001 From: Dave Shanley Date: Tue, 19 Jul 2022 07:07:01 -0400 Subject: [PATCH] Added no-http-verbs-in-path rule. Also make it a little easier to update tests when rules are added. --- functions/functions.go | 1 + functions/functions_test.go | 2 +- functions/openapi/no_http_verbs_in_path.go | 66 +++++++++++++++++++ .../openapi/no_http_verbs_in_path_test.go | 62 +++++++++++++++++ rulesets/rule_fixes.go | 3 + rulesets/ruleset_functions.go | 20 ++++++ rulesets/rulesets.go | 2 + rulesets/rulesets_test.go | 23 ++++--- 8 files changed, 168 insertions(+), 11 deletions(-) create mode 100644 functions/openapi/no_http_verbs_in_path.go create mode 100644 functions/openapi/no_http_verbs_in_path_test.go diff --git a/functions/functions.go b/functions/functions.go index 4602fd7e..ce87e0c0 100644 --- a/functions/functions.go +++ b/functions/functions.go @@ -96,6 +96,7 @@ func MapBuiltinFunctions() Functions { funcs["oasDocumentSchema"] = openapi_functions.OASSchema{} funcs["oasAPIServers"] = openapi_functions.APIServers{} funcs["noAmbiguousPaths"] = openapi_functions.AmbiguousPaths{} + funcs["noVerbsInPath"] = openapi_functions.VerbsInPaths{} funcs["oasOpErrorResponse"] = openapi_functions.Operation4xResponse{} }) diff --git a/functions/functions_test.go b/functions/functions_test.go index 61ca6b6d..70a94e5f 100644 --- a/functions/functions_test.go +++ b/functions/functions_test.go @@ -7,5 +7,5 @@ import ( func TestMapBuiltinFunctions(t *testing.T) { funcs := MapBuiltinFunctions() - assert.Len(t, funcs.GetAllFunctions(), 40) + assert.Len(t, funcs.GetAllFunctions(), 41) } diff --git a/functions/openapi/no_http_verbs_in_path.go b/functions/openapi/no_http_verbs_in_path.go new file mode 100644 index 00000000..64b377bd --- /dev/null +++ b/functions/openapi/no_http_verbs_in_path.go @@ -0,0 +1,66 @@ +// Copyright 2022 Dave Shanley / Quobix +// SPDX-License-Identifier: MIT + +package openapi + +import ( + "fmt" + "github.com/daveshanley/vacuum/model" + "github.com/pb33f/libopenapi/utils" + "gopkg.in/yaml.v3" + "strings" +) + +// VerbsInPaths Checks to make sure that no HTTP verbs have been used +type VerbsInPaths struct { +} + +// GetSchema returns a model.RuleFunctionSchema defining the schema of the VerbsInPath rule. +func (vp VerbsInPaths) GetSchema() model.RuleFunctionSchema { + return model.RuleFunctionSchema{Name: "noVerbsInPath"} +} + +// RunRule will execute the VerbsInPath rule, based on supplied context and a supplied []*yaml.Node slice. +func (vp VerbsInPaths) RunRule(nodes []*yaml.Node, context model.RuleFunctionContext) []model.RuleFunctionResult { + + if len(nodes) <= 0 { + return nil + } + + var results []model.RuleFunctionResult + + ops := context.Index.GetPathsNode() + + var opPath string + + if ops != nil { + for i, op := range ops.Content { + if i%2 == 0 { + opPath = op.Value + continue + } + path := fmt.Sprintf("$.paths.%s", opPath) + containsVerb, verb := checkPath(opPath) + if containsVerb { + results = append(results, model.RuleFunctionResult{ + Message: fmt.Sprintf("Path `%s` contains an HTTP Verb `%s`", opPath, verb), + StartNode: op, + EndNode: op, + Path: path, + Rule: context.Rule, + }) + } + } + } + return results +} + +func checkPath(path string) (bool, string) { + segs := strings.Split(path, "/")[1:] + for _, seg := range segs { + if utils.IsHttpVerb(strings.ToLower(seg)) { + return true, seg + } + } + return false, "" +} diff --git a/functions/openapi/no_http_verbs_in_path_test.go b/functions/openapi/no_http_verbs_in_path_test.go new file mode 100644 index 00000000..58de87c2 --- /dev/null +++ b/functions/openapi/no_http_verbs_in_path_test.go @@ -0,0 +1,62 @@ +package openapi + +import ( + "github.com/daveshanley/vacuum/model" + "github.com/pb33f/libopenapi/index" + "github.com/pb33f/libopenapi/utils" + "github.com/stretchr/testify/assert" + "gopkg.in/yaml.v3" + "testing" +) + +func TestVerbsInPaths_GetSchema(t *testing.T) { + def := VerbsInPaths{} + assert.Equal(t, "noVerbsInPath", def.GetSchema().Name) +} + +func TestVerbsInPaths_RunRule(t *testing.T) { + def := VerbsInPaths{} + res := def.RunRule(nil, model.RuleFunctionContext{}) + assert.Len(t, res, 0) +} + +func TestVerbsInPaths_Success(t *testing.T) { + + yml := `openapi: 3.0.0 +paths: + '/oh/no/post/man': + get: + summary: bad path + '/this/one/is/OK': + get: + summary: good path + '/i/am/going/to/get/failed': + get: + summary: not a good one. + '/will/you/patch/my/code': + get: + summary: this is also doomed + '/put/my/cake/away': + get: + summary: another bad one. + '/this/is/fine': + get: + summary: all done squire` + path := "$" + + var rootNode yaml.Node + yaml.Unmarshal([]byte(yml), &rootNode) + + nodes, _ := utils.FindNodes([]byte(yml), path) + + rule := buildOpenApiTestRuleAction(path, "verbsInPath", "", nil) + ctx := buildOpenApiTestContext(model.CastToRuleAction(rule.Then), nil) + ctx.Rule = &rule + ctx.Index = index.NewSpecIndex(&rootNode) + + def := VerbsInPaths{} + res := def.RunRule(nodes, ctx) + + assert.Len(t, res, 4) + +} diff --git a/rulesets/rule_fixes.go b/rulesets/rule_fixes.go index 79ea37bf..d685f706 100644 --- a/rulesets/rule_fixes.go +++ b/rulesets/rule_fixes.go @@ -143,6 +143,9 @@ const ( "are the same thing. Make sure every path and the variables used are unique and do conflict with one another. Check the ordering of variables " + "and the naming of path segments." + noVerbsInPathFix string = "When HTTP verbs (get/post/put etc) are used in path segments, it muddies the semantics of REST and creates a confusing and " + + "inconsistent experience. It's highly recommended that verbs are not used in path segments. Replace those HTTP verbs with more meaningful nouns." + operationsErrorResponseFix string = "Make sure each operation defines at least one 4xx error response. 4xx Errors are " + "used to inform clients they are using the API incorrectly, with bad input, or malformed requests. An API with no errors" + "defined is really hard to navigate." diff --git a/rulesets/ruleset_functions.go b/rulesets/ruleset_functions.go index 2cdb1f55..0479f8a0 100644 --- a/rulesets/ruleset_functions.go +++ b/rulesets/ruleset_functions.go @@ -1148,6 +1148,26 @@ func NoAmbiguousPaths() *model.Rule { } } +// GetNoVerbsInPathRule will check all paths to make sure not HTTP verbs have been used as a segment. +func GetNoVerbsInPathRule() *model.Rule { + return &model.Rule{ + Name: "Paths cannot contain HTTP verbs as segments", + Id: noVerbsInPath, + Formats: model.AllFormats, + Description: "Path segments must not contain an HTTP verb", + Given: "$", + Resolved: false, + Recommended: true, + RuleCategory: model.RuleCategories[model.CategoryOperations], + Type: style, + Severity: warn, + Then: model.RuleAction{ + Function: "noVerbsInPath", + }, + HowToFix: noVerbsInPathFix, + } +} + // GetOperationErrorResponseRule will return the rule for checking for a 4xx response defined in operations. func GetOperationErrorResponseRule() *model.Rule { return &model.Rule{ diff --git a/rulesets/rulesets.go b/rulesets/rulesets.go index 3738c5b1..a2237187 100644 --- a/rulesets/rulesets.go +++ b/rulesets/rulesets.go @@ -29,6 +29,7 @@ const ( allPaths = "$.paths[*]" allOperations = "[?(@.get || @.post || @.put || @.patch || @.delete || @.trace || @.options || @.head)]" + noVerbsInPath = "no-http-verbs-in-path" noAmbiguousPaths = "no-ambiguous-paths" operationErrorResponse = "operation-4xx-response" operationSuccessResponse = "operation-success-response" @@ -300,6 +301,7 @@ func generateDefaultOpenAPIRuleSet() *RuleSet { rules[oas3ValidSchemaExample] = GetOAS3ExamplesRule() rules[oas2ValidSchemaExample] = GetOAS2ExamplesRule() rules[noAmbiguousPaths] = NoAmbiguousPaths() + rules[noVerbsInPath] = GetNoVerbsInPathRule() rules[operationErrorResponse] = GetOperationErrorResponseRule() rules[oas2Schema] = GetOAS2SchemaRule() rules[oas3Schema] = GetOAS3SchemaRule() diff --git a/rulesets/rulesets_test.go b/rulesets/rulesets_test.go index d15f8451..63d40b50 100644 --- a/rulesets/rulesets_test.go +++ b/rulesets/rulesets_test.go @@ -5,11 +5,14 @@ import ( "testing" ) +var totalRules = 52 +var totalRecommendedRules = 41 + func TestBuildDefaultRuleSets(t *testing.T) { rs := BuildDefaultRuleSets() assert.NotNil(t, rs.GenerateOpenAPIDefaultRuleSet()) - assert.Len(t, rs.GenerateOpenAPIDefaultRuleSet().Rules, 51) + assert.Len(t, rs.GenerateOpenAPIDefaultRuleSet().Rules, totalRules) } @@ -121,10 +124,10 @@ func TestRuleSet_GetConfiguredRules_All(t *testing.T) { // read spec and parse to dashboard. rs := BuildDefaultRuleSets() ruleSet := rs.GenerateOpenAPIDefaultRuleSet() - assert.Len(t, ruleSet.Rules, 51) + assert.Len(t, ruleSet.Rules, totalRules) ruleSet = rs.GenerateOpenAPIRecommendedRuleSet() - assert.Len(t, ruleSet.Rules, 40) + assert.Len(t, ruleSet.Rules, totalRecommendedRules) } @@ -140,7 +143,7 @@ rules: def := BuildDefaultRuleSets() rs, _ := CreateRuleSetFromData([]byte(yaml)) override := def.GenerateRuleSetFromSuppliedRuleSet(rs) - assert.Len(t, override.Rules, 40) + assert.Len(t, override.Rules, totalRecommendedRules) assert.Len(t, override.RuleDefinitions, 1) } @@ -173,7 +176,7 @@ rules: def := BuildDefaultRuleSets() rs, _ := CreateRuleSetFromData([]byte(yaml)) override := def.GenerateRuleSetFromSuppliedRuleSet(rs) - assert.Len(t, override.Rules, 51) + assert.Len(t, override.Rules, totalRules) assert.Len(t, override.RuleDefinitions, 1) } @@ -189,7 +192,7 @@ rules: def := BuildDefaultRuleSets() rs, _ := CreateRuleSetFromData([]byte(yaml)) override := def.GenerateRuleSetFromSuppliedRuleSet(rs) - assert.Len(t, override.Rules, 39) + assert.Len(t, override.Rules, totalRecommendedRules-1) assert.Len(t, override.RuleDefinitions, 1) } @@ -205,7 +208,7 @@ rules: def := BuildDefaultRuleSets() rs, _ := CreateRuleSetFromData([]byte(yaml)) override := def.GenerateRuleSetFromSuppliedRuleSet(rs) - assert.Len(t, override.Rules, 40) + assert.Len(t, override.Rules, totalRecommendedRules) assert.Equal(t, "hint", override.Rules["operation-success-response"].Severity) } @@ -265,7 +268,7 @@ rules: def := BuildDefaultRuleSets() rs, _ := CreateRuleSetFromData([]byte(yaml)) newrs := def.GenerateRuleSetFromSuppliedRuleSet(rs) - assert.Len(t, newrs.Rules, 52) + assert.Len(t, newrs.Rules, totalRules+1) assert.Equal(t, true, newrs.Rules["fish-cakes"].Recommended) assert.Equal(t, "yummy sea food", newrs.Rules["fish-cakes"].Description) @@ -290,7 +293,7 @@ rules: def := BuildDefaultRuleSets() rs, _ := CreateRuleSetFromData([]byte(yaml)) repl := def.GenerateRuleSetFromSuppliedRuleSet(rs) - assert.Len(t, repl.Rules, 51) + assert.Len(t, repl.Rules, totalRules) assert.Equal(t, true, repl.Rules["info-contact"].Recommended) assert.Equal(t, "yummy sea food", repl.Rules["info-contact"].Description) @@ -315,7 +318,7 @@ rules: def := BuildDefaultRuleSets() rs, _ := CreateRuleSetFromData([]byte(yaml)) repl := def.GenerateRuleSetFromSuppliedRuleSet(rs) - assert.Len(t, repl.Rules, 51) + assert.Len(t, repl.Rules, totalRules) assert.Equal(t, true, repl.Rules["info-contact"].Recommended) assert.Equal(t, "yummy sea food", repl.Rules["info-contact"].Description)