Fix multiple OpenAPI generation issues with new AST-based generator (#18554)

* Regexp metacharacter `.` should be escaped when used literally

The paths including `/.well-known/` in the Vault API could currently
technically be invoked with any random character in place of the dot.

* Replace implementation of OpenAPI path translator with regexp AST-based one

* Add changelog

* Typo fix from PR review - thanks!

Co-authored-by: Anton Averchenkov <84287187+averche@users.noreply.github.com>

* Add comment based on review feedback

* Change style of error handling as suggested in code review

* Make a further tweak to the handling of the error case

* Add more tests, testing cases which fail with the previous implementation

* Resolve issue with a test, and improve comment

---------

Co-authored-by: Anton Averchenkov <84287187+averche@users.noreply.github.com>

changelog/18554.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+openapi: Fix many incorrect details in generated API spec, by using better techniques to parse path regexps
+```

sdk/framework/openapi.go

@@ -1,9 +1,11 @@
 package framework
 
 import (
+	"errors"
 	"fmt"
 	"reflect"
 	"regexp"
+	"regexp/syntax"
 	"sort"
 	"strconv"
 	"strings"
@@ -194,23 +196,12 @@ var OASStdRespNoContent = &OASResponse{
 	Description: "empty body",
 }
 
-// Regex for handling optional and named parameters in paths, and string cleanup.
+// Regex for handling fields in paths, and string cleanup.
 // Predefined here to avoid substantial recompilation.
 
-// Capture optional path elements in ungreedy (?U) fashion
-// Both "(leases/)?renew" and "(/(?P<name>.+))?" formats are detected
-var optRe = regexp.MustCompile(`(?U)\([^(]*\)\?|\(/\(\?P<[^(]*\)\)\?`)
-
 var (
-	altFieldsGroupRe = regexp.MustCompile(`\(\?P<\w+>\w+(\|\w+)+\)`)              // Match named groups that limit options, e.g. "(?<foo>a|b|c)"
-	altFieldsRe      = regexp.MustCompile(`\w+(\|\w+)+`)                          // Match an options set, e.g. "a|b|c"
-	altRe            = regexp.MustCompile(`\((.*)\|(.*)\)`)                       // Capture alternation elements, e.g. "(raw/?$|raw/(?P<path>.+))"
-	altRootsRe       = regexp.MustCompile(`^\(([\w\-_]+(?:\|[\w\-_]+)+)\)(/.*)$`) // Pattern starting with alts, e.g. "(root1|root2)/(?P<name>regex)"
-	cleanCharsRe     = regexp.MustCompile("[()^$?]")                              // Set of regex characters that will be stripped during cleaning
-	cleanSuffixRe    = regexp.MustCompile(`/\?\$?$`)                              // Path suffix patterns that will be stripped during cleaning
 	nonWordRe    = regexp.MustCompile(`[^\w]+`)  // Match a sequence of non-word characters
 	pathFieldsRe = regexp.MustCompile(`{(\w+)}`) // Capture OpenAPI-style named parameters, e.g. "lookup/{urltoken}",
-	reqdRe           = regexp.MustCompile(`\(?\?P<(\w+)>[^)]*\)?`)                // Capture required parameters, e.g. "(?P<name>regex)"
 	wsRe         = regexp.MustCompile(`\s+`)     // Match whitespace, to be compressed during cleaning
 )
 
@@ -236,7 +227,22 @@ func documentPath(p *Path, specialPaths *logical.Paths, requestResponsePrefix st
 	}
 
 	// Convert optional parameters into distinct patterns to be processed independently.
-	paths := expandPattern(p.Pattern)
+	forceUnpublished := false
+	paths, err := expandPattern(p.Pattern)
+	if err != nil {
+		if errors.Is(err, errUnsupportableRegexpOperationForOpenAPI) {
+			// Pattern cannot be transformed into sensible OpenAPI paths. In this case, we override the later
+			// processing to use the regexp, as is, as the path, and behave as if Unpublished was set on every
+			// operation (meaning the operations will not be represented in the OpenAPI document).
+			//
+			// This allows a human reading the OpenAPI document to notice that, yes, a path handler does exist,
+			// even though it was not able to contribute actual OpenAPI operations.
+			forceUnpublished = true
+			paths = []string{p.Pattern}
+		} else {
+			return err
+		}
+	}
 
 	for _, path := range paths {
 		// Construct a top level PathItem which will be populated as the path is processed.
@@ -305,7 +311,7 @@ func documentPath(p *Path, specialPaths *logical.Paths, requestResponsePrefix st
 		// with descriptions, properties and examples from the framework.Path data.
 		for opType, opHandler := range operations {
 			props := opHandler.Properties()
-			if props.Unpublished {
+			if props.Unpublished || forceUnpublished {
 				continue
 			}
 
@@ -554,85 +560,198 @@ func specialPathMatch(path string, specialPaths []string) bool {
 
 // expandPattern expands a regex pattern by generating permutations of any optional parameters
 // and changing named parameters into their {openapi} equivalents.
-func expandPattern(pattern string) []string {
+func expandPattern(pattern string) ([]string, error) {
-	var paths []string
+	// Happily, the Go regexp library exposes its underlying "parse to AST" functionality, so we can rely on that to do
-
+	// the hard work of interpreting the regexp syntax.
-	// Determine if the pattern starts with an alternation for multiple roots
+	rx, err := syntax.Parse(pattern, syntax.Perl)
-	// example (root1|root2)/(?P<name>regex) -> match['(root1|root2)/(?P<name>regex)','root1|root2','/(?P<name>regex)']
+	if err != nil {
-	match := altRootsRe.FindStringSubmatch(pattern)
+		// This should be impossible to reach, since regexps have previously been compiled with MustCompile in
-	if len(match) == 3 {
+		// Backend.init.
-		var expandedRoots []string
+		panic(err)
-		for _, root := range strings.Split(match[1], "|") {
-			expandedRoots = append(expandedRoots, expandPattern(root+match[2])...)
-		}
-		return expandedRoots
 	}
 
-	// GenericNameRegex adds a regex that complicates our parsing. It is much easier to
+	paths, err := collectPathsFromRegexpAST(rx)
-	// detect and remove it now than to compensate for in the other regexes.
+	if err != nil {
+		return nil, err
+	}
+
+	return paths, nil
+}
+
+type pathCollector struct {
+	strings.Builder
+	conditionalSlashAppendedAtLength int
+}
+
+// collectPathsFromRegexpAST performs a depth-first recursive walk through a regexp AST, collecting an OpenAPI-style
+// path as it goes.
 //
-	// example: (?P<foo>\\w(([\\w-.]+)?\\w)?) -> (?P<foo>)
+// Each time it encounters alternation (a|b) or an optional part (a?), it forks its processing to produce additional
-	base := GenericNameRegex("")
+// results, to account for each possibility. Note: This does mean that an input pattern with lots of these regexp
-	start := strings.Index(base, ">")
+// features can produce a lot of different OpenAPI endpoints. At the time of writing, the most complex known example is
-	end := strings.LastIndex(base, ")")
+//
-	regexToRemove := ""
+//	"issuer/" + framework.GenericNameRegex(issuerRefParam) + "/crl(/pem|/der|/delta(/pem|/der)?)?"
-	if start != -1 && end != -1 && end > start {
+//
-		regexToRemove = base[start+1 : end]
+// in the PKI secrets engine which expands to 6 separate paths.
+//
+// Each named capture group - i.e. (?P<name>something here) - is replaced with an OpenAPI parameter - i.e. {name} - and
+// the subtree of regexp AST inside the parameter is completely skipped.
+func collectPathsFromRegexpAST(rx *syntax.Regexp) ([]string, error) {
+	pathCollectors, err := collectPathsFromRegexpASTInternal(rx, []*pathCollector{{}})
+	if err != nil {
+		return nil, err
+	}
+	paths := make([]string, 0, len(pathCollectors))
+	for _, collector := range pathCollectors {
+		if collector.conditionalSlashAppendedAtLength != collector.Len() {
+			paths = append(paths, collector.String())
+		}
+	}
+	return paths, nil
 }
-	pattern = strings.ReplaceAll(pattern, regexToRemove, "")
 
-	// Simplify named fields that have limited options, e.g. (?P<foo>a|b|c) -> (<P<foo>.+)
+var errUnsupportableRegexpOperationForOpenAPI = errors.New("path regexp uses an operation that cannot be translated to an OpenAPI pattern")
-	pattern = altFieldsGroupRe.ReplaceAllStringFunc(pattern, func(s string) string {
-		return altFieldsRe.ReplaceAllString(s, ".+")
-	})
 
-	// Initialize paths with the original pattern or the halves of an
+func collectPathsFromRegexpASTInternal(rx *syntax.Regexp, appendingTo []*pathCollector) ([]*pathCollector, error) {
-	// alternation, which is also present in some patterns.
+	var err error
-	matches := altRe.FindAllStringSubmatch(pattern, -1)
+
-	if len(matches) > 0 {
+	// Depending on the type of this regexp AST node (its Op, i.e. operation), figure out whether it contributes any
-		paths = []string{matches[0][1], matches[0][2]}
+	// characters to the URL path, and whether we need to recurse through child AST nodes.
+	//
+	// Each element of the appendingTo slice tracks a separate path, defined by the alternatives chosen when traversing
+	// the | and ? conditional regexp features, and new elements are added as each of these features are traversed.
+	//
+	// To share this slice across multiple recursive calls of this function, it is passed down as a parameter to each
+	// recursive call, potentially modified throughout this switch block, and passed back up as a return value at the
+	// end of this function - the parent call uses the return value to update its own local variable.
+	switch rx.Op {
+
+	// These AST operations are leaf nodes (no children), that match zero characters, so require no processing at all
+	case syntax.OpEmptyMatch: // e.g. (?:)
+	case syntax.OpBeginLine: // i.e. ^ when (?m)
+	case syntax.OpEndLine: // i.e. $ when (?m)
+	case syntax.OpBeginText: // i.e. \A, or ^ when (?-m)
+	case syntax.OpEndText: // i.e. \z, or $ when (?-m)
+	case syntax.OpWordBoundary: // i.e. \b
+	case syntax.OpNoWordBoundary: // i.e. \B
+
+	// OpConcat simply represents multiple parts of the pattern appearing one after the other, so just recurse through
+	// those pieces.
+	case syntax.OpConcat:
+		for _, child := range rx.Sub {
+			appendingTo, err = collectPathsFromRegexpASTInternal(child, appendingTo)
+			if err != nil {
+				return nil, err
+			}
+		}
+
+	// OpLiteral is a literal string in the pattern - append it to the paths we are building.
+	case syntax.OpLiteral:
+		for _, collector := range appendingTo {
+			collector.WriteString(string(rx.Rune))
+		}
+
+	// OpAlternate, i.e. a|b, means we clone all of the pathCollector instances we are currently accumulating paths
+	// into, and independently recurse through each alternate option.
+	case syntax.OpAlternate: // i.e |
+		var totalAppendingTo []*pathCollector
+		lastIndex := len(rx.Sub) - 1
+		for index, child := range rx.Sub {
+			var childAppendingTo []*pathCollector
+			if index == lastIndex {
+				// Optimization: last time through this loop, we can simply re-use the existing set of pathCollector
+				// instances, as we no longer need to preserve them unmodified to make further copies of.
+				childAppendingTo = appendingTo
 			} else {
-		paths = []string{pattern}
+				for _, collector := range appendingTo {
+					newCollector := new(pathCollector)
+					newCollector.WriteString(collector.String())
+					newCollector.conditionalSlashAppendedAtLength = collector.conditionalSlashAppendedAtLength
+					childAppendingTo = append(childAppendingTo, newCollector)
 				}
+			}
+			childAppendingTo, err = collectPathsFromRegexpASTInternal(child, childAppendingTo)
+			if err != nil {
+				return nil, err
+			}
+			totalAppendingTo = append(totalAppendingTo, childAppendingTo...)
+		}
+		appendingTo = totalAppendingTo
 
-	// Expand all optional regex elements into two paths. This approach is really only useful up to 2 optional
+	// OpQuest, i.e. a?, is much like an alternation between exactly two options, one of which is the empty string.
-	// groups, but we probably don't want to deal with the exponential increase beyond that anyway.
+	case syntax.OpQuest:
-	for i := 0; i < len(paths); i++ {
+		child := rx.Sub[0]
-		p := paths[i]
+		var childAppendingTo []*pathCollector
+		for _, collector := range appendingTo {
+			newCollector := new(pathCollector)
+			newCollector.WriteString(collector.String())
+			newCollector.conditionalSlashAppendedAtLength = collector.conditionalSlashAppendedAtLength
+			childAppendingTo = append(childAppendingTo, newCollector)
+		}
+		childAppendingTo, err = collectPathsFromRegexpASTInternal(child, childAppendingTo)
+		if err != nil {
+			return nil, err
+		}
+		appendingTo = append(appendingTo, childAppendingTo...)
 
-		// match is a 2-element slice that will have a start and end index
+		// Many Vault path patterns end with `/?` to accept paths that end with or without a slash. Our current
-		// for the left-most match of a regex of form: (lease/)?
+		// convention for generating the OpenAPI is to strip away these slashes. To do that, this very special case
-		match := optRe.FindStringIndex(p)
+		// detects when we just appended a single conditional slash, and records the length of the path at this point,
-
+		// so we can later discard this path variant, if nothing else is appended to it later.
-		if match != nil {
+		if child.Op == syntax.OpLiteral && string(child.Rune) == "/" {
-			// create a path that includes the optional element but without
+			for _, collector := range childAppendingTo {
-			// parenthesis or the '?' character.
+				collector.conditionalSlashAppendedAtLength = collector.Len()
-			paths[i] = p[:match[0]] + p[match[0]+1:match[1]-2] + p[match[1]:]
-
-			// create a path that excludes the optional element.
-			paths = append(paths, p[:match[0]]+p[match[1]:])
-			i--
 			}
 		}
 
-	// Replace named parameters (?P<foo>) with {foo}
+	// OpCapture, i.e. ( ) or (?P<name> ), a capturing group
-	var replacedPaths []string
+	case syntax.OpCapture:
-
+		if rx.Name == "" {
-	for _, path := range paths {
+			// In Vault, an unnamed capturing group is not actually used for capturing.
-		result := reqdRe.FindAllStringSubmatch(path, -1)
+			// We treat it exactly the same as OpConcat.
-		if result != nil {
+			for _, child := range rx.Sub {
-			for _, p := range result {
+				appendingTo, err = collectPathsFromRegexpASTInternal(child, appendingTo)
-				par := p[1]
+				if err != nil {
-				path = strings.Replace(path, p[0], fmt.Sprintf("{%s}", par), 1)
+					return nil, err
 				}
 			}
-		// Final cleanup
+		} else {
-		path = cleanSuffixRe.ReplaceAllString(path, "")
+			// A named capturing group is replaced with the OpenAPI parameter syntax, and the regexp inside the group
-		path = cleanCharsRe.ReplaceAllString(path, "")
+			// is NOT added to the OpenAPI path.
-		replacedPaths = append(replacedPaths, path)
+			for _, builder := range appendingTo {
+				builder.WriteRune('{')
+				builder.WriteString(rx.Name)
+				builder.WriteRune('}')
+			}
 		}
 
-	return replacedPaths
+	// Any other kind of operation is a problem, and will trigger an error, resulting in the pattern being left out of
+	// the OpenAPI entirely - that's better than generating a path which is incorrect.
+	//
+	// The Op types we expect to hit the default condition are:
+	//
+	//     OpCharClass    - i.e. [something]
+	//     OpAnyCharNotNL - i.e. .
+	//     OpAnyChar      - i.e. (?s:.)
+	//     OpStar         - i.e. *
+	//     OpPlus         - i.e. +
+	//     OpRepeat       - i.e. {N}, {N,M}, etc.
+	//
+	// In any of these conditions, there is no sensible translation of the path to OpenAPI syntax. (Note, this only
+	// applies to these appearing outside of a named capture group, otherwise they are handled in the previous case.)
+	//
+	// At the time of writing, the only pattern in the builtin Vault plugins that hits this codepath is the ".*"
+	// pattern in the KVv2 secrets engine, which is not a valid path, but rather, is a catch-all used to implement
+	// custom error handling behaviour to guide users who attempt to treat a KVv2 as a KVv1. It is already marked as
+	// Unpublished, so is withheld from the OpenAPI anyway.
+	//
+	// For completeness, one other Op type exists, OpNoMatch, which is never generated by syntax.Parse - only by
+	// subsequent Simplify in preparation to Compile, which is not used here.
+	default:
+		return nil, errUnsupportableRegexpOperationForOpenAPI
+	}
+
+	return appendingTo, nil
 }
 
 // schemaType is a subset of the JSON Schema elements used as a target

sdk/framework/openapi_test.go

@@ -18,75 +18,6 @@ import (
 )
 
 func TestOpenAPI_Regex(t *testing.T) {
-	t.Run("Required", func(t *testing.T) {
-		tests := []struct {
-			input    string
-			captures []string
-		}{
-			{`/foo/bar/(?P<val>.*)`, []string{"val"}},
-			{`/foo/bar/` + GenericNameRegex("val"), []string{"val"}},
-			{`/foo/bar/` + GenericNameRegex("first") + "/b/" + GenericNameRegex("second"), []string{"first", "second"}},
-			{`/foo/bar`, []string{}},
-		}
-
-		for _, test := range tests {
-			result := reqdRe.FindAllStringSubmatch(test.input, -1)
-			if len(result) != len(test.captures) {
-				t.Fatalf("Capture error (%s): expected %d matches, actual: %d", test.input, len(test.captures), len(result))
-			}
-
-			for i := 0; i < len(result); i++ {
-				if result[i][1] != test.captures[i] {
-					t.Fatalf("Capture error (%s): expected %s, actual: %s", test.input, test.captures[i], result[i][1])
-				}
-			}
-		}
-	})
-	t.Run("Optional", func(t *testing.T) {
-		input := "foo/(maybe/)?bar"
-		expStart := len("foo/")
-		expEnd := len(input) - len("bar")
-
-		match := optRe.FindStringIndex(input)
-		if diff := deep.Equal(match, []int{expStart, expEnd}); diff != nil {
-			t.Fatal(diff)
-		}
-
-		input = "/foo/maybe/bar"
-		match = optRe.FindStringIndex(input)
-		if match != nil {
-			t.Fatalf("Expected nil match (%s), got %+v", input, match)
-		}
-	})
-	t.Run("Alternation", func(t *testing.T) {
-		input := `(raw/?$|raw/(?P<path>.+))`
-
-		matches := altRe.FindAllStringSubmatch(input, -1)
-		exp1 := "raw/?$"
-		exp2 := "raw/(?P<path>.+)"
-		if matches[0][1] != exp1 || matches[0][2] != exp2 {
-			t.Fatalf("Capture error. Expected %s and %s, got %v", exp1, exp2, matches[0][1:])
-		}
-
-		input = `/foo/bar/` + GenericNameRegex("val")
-
-		matches = altRe.FindAllStringSubmatch(input, -1)
-		if matches != nil {
-			t.Fatalf("Expected nil match (%s), got %+v", input, matches)
-		}
-	})
-	t.Run("Alternation Fields", func(t *testing.T) {
-		input := `/foo/bar/(?P<type>auth|database|secret)/(?P<blah>a|b)`
-
-		act := altFieldsGroupRe.ReplaceAllStringFunc(input, func(s string) string {
-			return altFieldsRe.ReplaceAllString(s, ".+")
-		})
-
-		exp := "/foo/bar/(?P<type>.+)/(?P<blah>.+)"
-		if act != exp {
-			t.Fatalf("Replace error. Expected %s, got %v", exp, act)
-		}
-	})
 	t.Run("Path fields", func(t *testing.T) {
 		input := `/foo/bar/{inner}/baz/{outer}`
 
@@ -111,21 +42,6 @@ func TestOpenAPI_Regex(t *testing.T) {
 			regex  *regexp.Regexp
 			output string
 		}{
-			{
-				input:  `ab?cde^fg(hi?j$k`,
-				regex:  cleanCharsRe,
-				output: "abcdefghijk",
-			},
-			{
-				input:  `abcde/?`,
-				regex:  cleanSuffixRe,
-				output: "abcde",
-			},
-			{
-				input:  `abcde/?$`,
-				regex:  cleanSuffixRe,
-				output: "abcde",
-			},
 			{
 				input:  `abcde`,
 				regex:  wsRe,
@@ -152,62 +68,99 @@ func TestOpenAPI_ExpandPattern(t *testing.T) {
 		inPattern   string
 		outPathlets []string
 	}{
+		// A simple string without regexp metacharacters passes through as is
 		{"rekey/backup", []string{"rekey/backup"}},
+		// A trailing regexp anchor metacharacter is removed
 		{"rekey/backup$", []string{"rekey/backup"}},
+		// As is a leading one
+		{"^rekey/backup", []string{"rekey/backup"}},
+		// Named capture groups become OpenAPI parameters
 		{"auth/(?P<path>.+?)/tune$", []string{"auth/{path}/tune"}},
 		{"auth/(?P<path>.+?)/tune/(?P<more>.*?)$", []string{"auth/{path}/tune/{more}"}},
+		// Even if the capture group contains very complex regexp structure inside it
+		{"something/(?P<something>(a|b(c|d))|e+|f{1,3}[ghi-k]?.*)", []string{"something/{something}"}},
+		// A question-mark results in a result without and with the optional path part
 		{"tools/hash(/(?P<urlalgorithm>.+))?", []string{
 			"tools/hash",
 			"tools/hash/{urlalgorithm}",
 		}},
+		// Multiple question-marks evaluate each possible combination
 		{"(leases/)?renew(/(?P<url_lease_id>.+))?", []string{
 			"leases/renew",
 			"leases/renew/{url_lease_id}",
 			"renew",
 			"renew/{url_lease_id}",
 		}},
+		// GenericNameRegex is one particular way of writing a named capture group, so behaves the same
 		{`config/ui/headers/` + GenericNameRegex("header"), []string{"config/ui/headers/{header}"}},
+		// The question-mark behaviour is still works when the question-mark is directly applied to a named capture group
 		{`leases/lookup/(?P<prefix>.+?)?`, []string{
 			"leases/lookup/",
 			"leases/lookup/{prefix}",
 		}},
+		// Optional trailing slashes at the end of the path get stripped - even if appearing deep inside an alternation
 		{`(raw/?$|raw/(?P<path>.+))`, []string{
 			"raw",
 			"raw/{path}",
 		}},
+		// OptionalParamRegex is also another way of writing a named capture group, that is optional
 		{"lookup" + OptionalParamRegex("urltoken"), []string{
 			"lookup",
 			"lookup/{urltoken}",
 		}},
+		// Optional trailign slashes at the end of the path get stripped in simpler cases too
 		{"roles/?$", []string{
 			"roles",
 		}},
 		{"roles/?", []string{
 			"roles",
 		}},
+		// Non-optional trailing slashes remain... although don't do this, it breaks HelpOperation!
+		// (Existing real examples of this pattern being fixed via https://github.com/hashicorp/vault/pull/18571)
 		{"accessors/$", []string{
 			"accessors/",
 		}},
+		// GenericNameRegex and OptionalParamRegex still work when concatenated
 		{"verify/" + GenericNameRegex("name") + OptionalParamRegex("urlalgorithm"), []string{
 			"verify/{name}",
 			"verify/{name}/{urlalgorithm}",
 		}},
+		// Named capture groups that specify enum-like parameters work as expected
 		{"^plugins/catalog/(?P<type>auth|database|secret)/(?P<name>.+)$", []string{
 			"plugins/catalog/{type}/{name}",
 		}},
 		{"^plugins/catalog/(?P<type>auth|database|secret)/?$", []string{
 			"plugins/catalog/{type}",
 		}},
+		// Alternations between various literal path segments work
 		{"(pathOne|pathTwo)/", []string{"pathOne/", "pathTwo/"}},
 		{"(pathOne|pathTwo)/" + GenericNameRegex("name"), []string{"pathOne/{name}", "pathTwo/{name}"}},
 		{
 			"(pathOne|path-2|Path_3)/" + GenericNameRegex("name"),
 			[]string{"Path_3/{name}", "path-2/{name}", "pathOne/{name}"},
 		},
+		// They still work when combined with GenericNameWithAtRegex
+		{"(creds|sts)/" + GenericNameWithAtRegex("name"), []string{
+			"creds/{name}",
+			"sts/{name}",
+		}},
+		// And when they're somewhere other than the start of the pattern
+		{"keys/generate/(internal|exported|kms)", []string{
+			"keys/generate/exported",
+			"keys/generate/internal",
+			"keys/generate/kms",
+		}},
+		// If a plugin author makes their list operation support both singular and plural forms, the OpenAPI notices
+		{"rolesets?/?", []string{"roleset", "rolesets"}},
+		// Complex nested alternation and question-marks are correctly interpreted
+		{"crl(/pem|/delta(/pem)?)?", []string{"crl", "crl/delta", "crl/delta/pem", "crl/pem"}},
 	}
 
 	for i, test := range tests {
-		out := expandPattern(test.inPattern)
+		out, err := expandPattern(test.inPattern)
+		if err != nil {
+			t.Fatal(err)
+		}
 		sort.Strings(out)
 		if !reflect.DeepEqual(out, test.outPathlets) {
 			t.Fatalf("Test %d: Expected %v got %v", i, test.outPathlets, out)
@@ -215,6 +168,30 @@ func TestOpenAPI_ExpandPattern(t *testing.T) {
 	}
 }
 
+func TestOpenAPI_ExpandPattern_ReturnsError(t *testing.T) {
+	tests := []struct {
+		inPattern string
+		outError  error
+	}{
+		// None of these regexp constructs are allowed outside of named capture groups
+		{"[a-z]", errUnsupportableRegexpOperationForOpenAPI},
+		{".", errUnsupportableRegexpOperationForOpenAPI},
+		{"a+", errUnsupportableRegexpOperationForOpenAPI},
+		{"a*", errUnsupportableRegexpOperationForOpenAPI},
+		// So this pattern, which is a combination of two of the above isn't either - this pattern occurs in the KV
+		// secrets engine for its catch-all error handler, which provides a helpful hint to people treating a KV v2 as
+		// a KV v1.
+		{".*", errUnsupportableRegexpOperationForOpenAPI},
+	}
+
+	for i, test := range tests {
+		_, err := expandPattern(test.inPattern)
+		if err != test.outError {
+			t.Fatalf("Test %d: Expected %q got %q", i, test.outError, err)
+		}
+	}
+}
+
 func TestOpenAPI_SplitFields(t *testing.T) {
 	fields := map[string]*FieldSchema{
 		"a": {Description: "path"},

vault/identity_store_oidc.go

@@ -216,7 +216,7 @@ func oidcPaths(i *IdentityStore) []*framework.Path {
 			HelpDescription: "List all named OIDC keys",
 		},
 		{
-			Pattern: "oidc/.well-known/openid-configuration/?$",
+			Pattern: "oidc/\\.well-known/openid-configuration/?$",
 			Callbacks: map[logical.Operation]framework.OperationFunc{
 				logical.ReadOperation: i.pathOIDCDiscovery,
 			},
@@ -224,7 +224,7 @@ func oidcPaths(i *IdentityStore) []*framework.Path {
 			HelpDescription: "Query this path to retrieve the configured OIDC Issuer and Keys endpoints, response types, subject types, and signing algorithms used by the OIDC backend.",
 		},
 		{
-			Pattern: "oidc/.well-known/keys/?$",
+			Pattern: "oidc/\\.well-known/keys/?$",
 			Callbacks: map[logical.Operation]framework.OperationFunc{
 				logical.ReadOperation: i.pathOIDCReadPublicKeys,
 			},

vault/identity_store_oidc_provider.go

@@ -389,7 +389,7 @@ func oidcProviderPaths(i *IdentityStore) []*framework.Path {
 			HelpDescription: "List all configured OIDC providers in the identity backend.",
 		},
 		{
-			Pattern: "oidc/provider/" + framework.GenericNameRegex("name") + "/.well-known/openid-configuration",
+			Pattern: "oidc/provider/" + framework.GenericNameRegex("name") + "/\\.well-known/openid-configuration",
 			Fields: map[string]*framework.FieldSchema{
 				"name": {
 					Type:        framework.TypeString,
@@ -405,7 +405,7 @@ func oidcProviderPaths(i *IdentityStore) []*framework.Path {
 			HelpDescription: "Query this path to retrieve the configured OIDC Issuer and Keys endpoints, response types, subject types, and signing algorithms used by the OIDC backend.",
 		},
 		{
-			Pattern: "oidc/provider/" + framework.GenericNameRegex("name") + "/.well-known/keys",
+			Pattern: "oidc/provider/" + framework.GenericNameRegex("name") + "/\\.well-known/keys",
 			Fields: map[string]*framework.FieldSchema{
 				"name": {
 					Type:        framework.TypeString,

vault/logical_system_test.go

@@ -3618,64 +3618,11 @@ func TestSystemBackend_InternalUIMount(t *testing.T) {
 	}
 }
 
-func TestSystemBackend_OASGenericMount(t *testing.T) {
-	_, b, rootToken := testCoreSystemBackend(t)
-	var oapi map[string]interface{}
-
-	// Check that default paths are present with a root token
-	req := logical.TestRequest(t, logical.ReadOperation, "internal/specs/openapi")
-	req.Data["generic_mount_paths"] = true
-	req.ClientToken = rootToken
-	resp, err := b.HandleRequest(namespace.RootContext(nil), req)
-	if err != nil {
-		t.Fatalf("err: %v", err)
-	}
-
-	body := resp.Data["http_raw_body"].([]byte)
-	err = jsonutil.DecodeJSON(body, &oapi)
-	if err != nil {
-		t.Fatalf("err: %v", err)
-	}
-
-	doc, err := framework.NewOASDocumentFromMap(oapi)
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	pathSamples := []struct {
-		path string
-		tag  string
-	}{
-		{"/auth/token/lookup", "auth"},
-		{"/cubbyhole/{path}", "secrets"},
-		{"/identity/group/id", "identity"},
-		{"/{secret_mount_path}/.*", "secrets"},
-		{"/sys/policy", "system"},
-	}
-
-	for _, path := range pathSamples {
-		if doc.Paths[path.path] == nil {
-			t.Fatalf("didn't find expected path '%s'.", path)
-		}
-		tag := doc.Paths[path.path].Get.Tags[0]
-		if tag != path.tag {
-			t.Fatalf("path: %s; expected tag: %s, actual: %s", path.path, tag, path.tag)
-		}
-	}
-
-	// Simple check of response size (which is much larger than most
-	// Vault responses), mainly to catch mass omission of expected path data.
-	const minLen = 70000
-	if len(body) < minLen {
-		t.Fatalf("response size too small; expected: min %d, actual: %d", minLen, len(body))
-	}
-}
-
 func TestSystemBackend_OpenAPI(t *testing.T) {
 	_, b, rootToken := testCoreSystemBackend(t)
-	var oapi map[string]interface{}
 
 	// Ensure no paths are reported if there is no token
+	{
 		req := logical.TestRequest(t, logical.ReadOperation, "internal/specs/openapi")
 		resp, err := b.HandleRequest(namespace.RootContext(nil), req)
 		if err != nil {
@@ -3683,6 +3630,7 @@ func TestSystemBackend_OpenAPI(t *testing.T) {
 		}
 
 		body := resp.Data["http_raw_body"].([]byte)
+		var oapi map[string]interface{}
 		err = jsonutil.DecodeJSON(body, &oapi)
 		if err != nil {
 			t.Fatalf("err: %v", err)
@@ -3707,16 +3655,22 @@ func TestSystemBackend_OpenAPI(t *testing.T) {
 		if diff := deep.Equal(oapi, exp); diff != nil {
 			t.Fatal(diff)
 		}
+	}
 
-	// Check that default paths are present with a root token
+	// Check that default paths are present with a root token (with and without generic_mount_paths)
-	req = logical.TestRequest(t, logical.ReadOperation, "internal/specs/openapi")
+	for _, genericMountPaths := range []bool{false, true} {
+		req := logical.TestRequest(t, logical.ReadOperation, "internal/specs/openapi")
+		if genericMountPaths {
+			req.Data["generic_mount_paths"] = true
+		}
 		req.ClientToken = rootToken
-	resp, err = b.HandleRequest(namespace.RootContext(nil), req)
+		resp, err := b.HandleRequest(namespace.RootContext(nil), req)
 		if err != nil {
 			t.Fatalf("err: %v", err)
 		}
 
-	body = resp.Data["http_raw_body"].([]byte)
+		body := resp.Data["http_raw_body"].([]byte)
+		var oapi map[string]interface{}
 		err = jsonutil.DecodeJSON(body, &oapi)
 		if err != nil {
 			t.Fatalf("err: %v", err)
@@ -3727,26 +3681,41 @@ func TestSystemBackend_OpenAPI(t *testing.T) {
 			t.Fatal(err)
 		}
 
+		expectedSecretPrefix := "/secret/"
+		if genericMountPaths {
+			expectedSecretPrefix = "/{secret_mount_path}/"
+		}
+
 		pathSamples := []struct {
 			path        string
 			tag         string
+			unpublished bool
 		}{
-		{"/auth/token/lookup", "auth"},
+			{path: "/auth/token/lookup", tag: "auth"},
-		{"/cubbyhole/{path}", "secrets"},
+			{path: "/cubbyhole/{path}", tag: "secrets"},
-		{"/identity/group/id", "identity"},
+			{path: "/identity/group/id", tag: "identity"},
-		{"/secret/.*", "secrets"},
+			{path: expectedSecretPrefix + "^.*$", unpublished: true},
-		{"/sys/policy", "system"},
+			{path: "/sys/policy", tag: "system"},
 		}
 
 		for _, path := range pathSamples {
 			if doc.Paths[path.path] == nil {
-			t.Fatalf("didn't find expected path %q.", path)
+				t.Fatalf("didn't find expected path %q.", path.path)
 			}
-		tag := doc.Paths[path.path].Get.Tags[0]
+			getOperation := doc.Paths[path.path].Get
+			if getOperation == nil && !path.unpublished {
+				t.Fatalf("path: %s; expected a get operation, but it was absent", path.path)
+			}
+			if getOperation != nil && path.unpublished {
+				t.Fatalf("path: %s; expected absent get operation, but it was present", path.path)
+			}
+			if !path.unpublished {
+				tag := getOperation.Tags[0]
 				if tag != path.tag {
 					t.Fatalf("path: %s; expected tag: %s, actual: %s", path.path, tag, path.tag)
 				}
 			}
+		}
 
 		// Simple check of response size (which is much larger than most
 		// Vault responses), mainly to catch mass omission of expected path data.
@@ -3754,16 +3723,18 @@ func TestSystemBackend_OpenAPI(t *testing.T) {
 		if len(body) < minLen {
 			t.Fatalf("response size too small; expected: min %d, actual: %d", minLen, len(body))
 		}
+	}
 
 	// Test path-help response
-	req = logical.TestRequest(t, logical.HelpOperation, "rotate")
+	{
+		req := logical.TestRequest(t, logical.HelpOperation, "rotate")
 		req.ClientToken = rootToken
-	resp, err = b.HandleRequest(namespace.RootContext(nil), req)
+		resp, err := b.HandleRequest(namespace.RootContext(nil), req)
 		if err != nil {
 			t.Fatalf("err: %v", err)
 		}
 
-	doc = resp.Data["openapi"].(*framework.OASDocument)
+		doc := resp.Data["openapi"].(*framework.OASDocument)
 		if len(doc.Paths) != 1 {
 			t.Fatalf("expected 1 path, actual: %d", len(doc.Paths))
 		}
@@ -3772,6 +3743,7 @@ func TestSystemBackend_OpenAPI(t *testing.T) {
 			t.Fatalf("expected to find path '/rotate'")
 		}
 	}
+}
 
 func TestSystemBackend_PathWildcardPreflight(t *testing.T) {
 	core, b, _ := testCoreSystemBackend(t)