diff --git a/pkg/codegen/server_urls_test.go b/pkg/codegen/server_urls_test.go index 2105649bfb..1fbc7e4577 100644 --- a/pkg/codegen/server_urls_test.go +++ b/pkg/codegen/server_urls_test.go @@ -1,7 +1,11 @@ package codegen import ( + "go/ast" + "go/parser" + "go/token" "testing" + "text/template" "github.com/getkin/kin-openapi/openapi3" "github.com/stretchr/testify/assert" @@ -57,6 +61,116 @@ func TestUsedAndUndeclaredVariables(t *testing.T) { assert.Equal(t, []string{"path"}, undeclared, "URL placeholder not in variables must be reported (#2005)") } +// renderServerURLs parses the embedded templates and renders the +// server-urls template for spec, mirroring the setup in Generate. +func renderServerURLs(t *testing.T, spec *openapi3.T) string { + t.Helper() + // Generate registers `opts` before parsing; mirror that here so the + // (unrelated) framework templates parse cleanly. + TemplateFunctions["opts"] = func() Configuration { return globalState.options } + tmpl := template.New("oapi-codegen").Funcs(TemplateFunctions) + require.NoError(t, LoadTemplates(templates, tmpl)) + out, err := GenerateServerURLs(tmpl, spec) + require.NoError(t, err) + return out +} + +// injectSentinel is the identifier a malicious spec tries to smuggle in +// as a real top-level declaration. The two escape techniques covered are: +// - a newline that breaks out of a `//` line comment, and +// - a `"` that breaks out of a `const … = "…"` string literal, +// +// both of which would land `var injectSentinel = …` at package scope. +const injectSentinel = "OapiCodegenInjectedPwned" + +// TestServerURLInjection checks that attacker-controlled spec text (server +// description, URL, and variable default), which flows into generated Go line +// comments and string literals, cannot break out into an executable top-level +// declaration. +func TestServerURLInjection(t *testing.T) { + // commentBreakout escapes a `//` comment, declares the sentinel, then + // re-opens a comment so the surrounding template text stays valid. + commentBreakout := "benign\nvar " + injectSentinel + " = 1\n//" + // literalBreakout escapes a `"…"` string literal, declares the + // sentinel, then re-opens a literal to swallow the template's closing + // quote. + literalBreakout := `https://api.example.com"; var ` + injectSentinel + ` = 1; var _ = "` + + t.Run("newline in description cannot escape the doc comment", func(t *testing.T) { + out := renderServerURLs(t, &openapi3.T{Servers: openapi3.Servers{ + {URL: "https://api.example.com", Description: commentBreakout}, + }}) + assertSentinelNotDeclared(t, out) + }) + + t.Run("description in the function (variable-bearing) form cannot escape", func(t *testing.T) { + out := renderServerURLs(t, &openapi3.T{Servers: openapi3.Servers{ + {URL: "https://api.example.com/{tenant}", Description: commentBreakout}, + }}) + assertSentinelNotDeclared(t, out) + }) + + t.Run("quote in URL cannot escape the const string literal", func(t *testing.T) { + out := renderServerURLs(t, &openapi3.T{Servers: openapi3.Servers{ + {URL: literalBreakout}, + }}) + assertSentinelNotDeclared(t, out) + }) + + t.Run("quote in a variable default cannot escape the const string literal", func(t *testing.T) { + out := renderServerURLs(t, &openapi3.T{Servers: openapi3.Servers{ + { + URL: "https://api.example.com/{base}", + Variables: map[string]*openapi3.ServerVariable{ + // non-enum, used → emits `const …BaseVariableDefault = ""` + "base": {Default: `v2"; var ` + injectSentinel + ` = 1; var _ = "`}, + }, + }, + }}) + assertSentinelNotDeclared(t, out) + }) +} + +// assertSentinelNotDeclared parses the generated output as a package body +// and fails if it is not valid Go or if injectSentinel appears as a +// top-level declared identifier (i.e. the payload escaped its comment or +// string literal and became real source). +func assertSentinelNotDeclared(t *testing.T, out string) { + t.Helper() + src := "package p\n" + out + fset := token.NewFileSet() + file, err := parser.ParseFile(fset, "gen.go", src, parser.ParseComments) + require.NoError(t, err, "generated output must parse as valid Go:\n%s", src) + + for _, name := range topLevelDeclNames(file) { + assert.NotEqual(t, injectSentinel, name, + "injected identifier became a real top-level declaration:\n%s", src) + } +} + +// topLevelDeclNames returns every identifier declared at file scope. +func topLevelDeclNames(file *ast.File) []string { + var names []string + for _, decl := range file.Decls { + switch d := decl.(type) { + case *ast.FuncDecl: + names = append(names, d.Name.Name) + case *ast.GenDecl: + for _, spec := range d.Specs { + switch s := spec.(type) { + case *ast.ValueSpec: + for _, n := range s.Names { + names = append(names, n.Name) + } + case *ast.TypeSpec: + names = append(names, s.Name.Name) + } + } + } + } + return names +} + func TestBuildServerURLTypeDefinitions(t *testing.T) { t.Run("synthesises one TypeDefinition per enum-typed used variable", func(t *testing.T) { spec := &openapi3.T{ diff --git a/pkg/codegen/template_helpers.go b/pkg/codegen/template_helpers.go index 1b46105323..a72d2911ce 100644 --- a/pkg/codegen/template_helpers.go +++ b/pkg/codegen/template_helpers.go @@ -322,8 +322,10 @@ func toStringArray(sarr []string) string { return `[]string{` + s + `}` } +// stripNewLines removes newlines so untrusted spec text stays inside a single +// generated `//` line comment instead of breaking out into real Go source. func stripNewLines(s string) string { - r := strings.NewReplacer("\n", "") + r := strings.NewReplacer("\n", "", "\r", "") return r.Replace(s) } diff --git a/pkg/codegen/templates/chi/chi-handler.tmpl b/pkg/codegen/templates/chi/chi-handler.tmpl index 72c57a6be6..cc07375f92 100644 --- a/pkg/codegen/templates/chi/chi-handler.tmpl +++ b/pkg/codegen/templates/chi/chi-handler.tmpl @@ -43,7 +43,7 @@ ErrorHandlerFunc: options.ErrorHandlerFunc, } {{end}} {{range .}}r.Group(func(r chi.Router) { -r.{{.Method | lower | title }}(options.BaseURL+"{{.Path | swaggerUriToChiUri}}", wrapper.{{.HandlerName}}) +r.{{.Method | lower | title }}(options.BaseURL+{{.Path | swaggerUriToChiUri | toGoString}}, wrapper.{{.HandlerName}}) }) {{end}} return r diff --git a/pkg/codegen/templates/constants.tmpl b/pkg/codegen/templates/constants.tmpl index 07a03a2a38..2af795c97b 100644 --- a/pkg/codegen/templates/constants.tmpl +++ b/pkg/codegen/templates/constants.tmpl @@ -9,7 +9,7 @@ const ( // Defines values for {{$Enum.TypeName}}. const ( {{range $name, $value := $Enum.GetValues}} - {{$name}} {{$Enum.TypeName}} = {{$Enum.ValueWrapper}}{{$value}}{{$Enum.ValueWrapper -}} + {{$name}} {{$Enum.TypeName}} = {{if $Enum.ValueWrapper}}{{$value | toGoString}}{{else}}{{$value}}{{end -}} {{end}} ) {{if not $.SkipEnumValidate}} diff --git a/pkg/codegen/templates/echo/echo-register.tmpl b/pkg/codegen/templates/echo/echo-register.tmpl index def0917e31..d161fca37d 100644 --- a/pkg/codegen/templates/echo/echo-register.tmpl +++ b/pkg/codegen/templates/echo/echo-register.tmpl @@ -47,6 +47,6 @@ func RegisterHandlersWithOptions(router EchoRouter, si ServerInterface, options Handler: si, } {{end}} -{{range .}}router.{{.Method}}(options.BaseURL + "{{.Path | swaggerUriToEchoUri}}", wrapper.{{.HandlerName}}, options.OperationMiddlewares["{{.MiddlewareKey}}"]...) +{{range .}}router.{{.Method}}(options.BaseURL + {{.Path | swaggerUriToEchoUri | toGoString}}, wrapper.{{.HandlerName}}, options.OperationMiddlewares["{{.MiddlewareKey}}"]...) {{end}} } diff --git a/pkg/codegen/templates/echo/v5/echo-register.tmpl b/pkg/codegen/templates/echo/v5/echo-register.tmpl index d311fefbe2..95ccf32316 100644 --- a/pkg/codegen/templates/echo/v5/echo-register.tmpl +++ b/pkg/codegen/templates/echo/v5/echo-register.tmpl @@ -47,6 +47,6 @@ func RegisterHandlersWithOptions(router EchoRouter, si ServerInterface, options Handler: si, } {{end}} -{{range .}}router.{{.Method}}(options.BaseURL + "{{.Path | swaggerUriToEchoUri}}", wrapper.{{.HandlerName}}, options.OperationMiddlewares["{{.MiddlewareKey}}"]...) +{{range .}}router.{{.Method}}(options.BaseURL + {{.Path | swaggerUriToEchoUri | toGoString}}, wrapper.{{.HandlerName}}, options.OperationMiddlewares["{{.MiddlewareKey}}"]...) {{end}} } diff --git a/pkg/codegen/templates/fiber/fiber-handler.tmpl b/pkg/codegen/templates/fiber/fiber-handler.tmpl index 5ac8be929f..8460db83c1 100644 --- a/pkg/codegen/templates/fiber/fiber-handler.tmpl +++ b/pkg/codegen/templates/fiber/fiber-handler.tmpl @@ -22,6 +22,6 @@ for _, m := range options.Middlewares { } {{end}} {{range .}} -router.{{.Method | lower | title }}(options.BaseURL+"{{.Path | swaggerUriToFiberUri}}", wrapper.{{.HandlerName}}) +router.{{.Method | lower | title }}(options.BaseURL+{{.Path | swaggerUriToFiberUri | toGoString}}, wrapper.{{.HandlerName}}) {{end}} } diff --git a/pkg/codegen/templates/gin/gin-register.tmpl b/pkg/codegen/templates/gin/gin-register.tmpl index 4bc17b3c74..43b9ae2504 100644 --- a/pkg/codegen/templates/gin/gin-register.tmpl +++ b/pkg/codegen/templates/gin/gin-register.tmpl @@ -28,6 +28,6 @@ func RegisterHandlersWithOptions(router gin.IRouter, si ServerInterface, options {{end}} {{range . -}} - router.{{.Method }}(options.BaseURL+"{{.Path | swaggerUriToGinUri }}", wrapper.{{.HandlerName}}) + router.{{.Method }}(options.BaseURL+{{.Path | swaggerUriToGinUri | toGoString}}, wrapper.{{.HandlerName}}) {{end -}} } diff --git a/pkg/codegen/templates/gorilla/gorilla-register.tmpl b/pkg/codegen/templates/gorilla/gorilla-register.tmpl index b16f3fc9fc..853ab32c35 100644 --- a/pkg/codegen/templates/gorilla/gorilla-register.tmpl +++ b/pkg/codegen/templates/gorilla/gorilla-register.tmpl @@ -43,7 +43,7 @@ ErrorHandlerFunc: options.ErrorHandlerFunc, } {{end}} {{range .}} -r.HandleFunc(options.BaseURL+"{{.Path | swaggerUriToGorillaUri }}", wrapper.{{.HandlerName}}).Methods({{.Method | httpMethodConstant}}) +r.HandleFunc(options.BaseURL+{{.Path | swaggerUriToGorillaUri | toGoString}}, wrapper.{{.HandlerName}}).Methods({{.Method | httpMethodConstant}}) {{end}} return r } diff --git a/pkg/codegen/templates/iris/iris-handler.tmpl b/pkg/codegen/templates/iris/iris-handler.tmpl index c0c5b23cc5..f2fb56e442 100644 --- a/pkg/codegen/templates/iris/iris-handler.tmpl +++ b/pkg/codegen/templates/iris/iris-handler.tmpl @@ -17,7 +17,7 @@ func RegisterHandlersWithOptions(router *iris.Application, si ServerInterface, o Handler: si, } {{end}} -{{range .}}router.{{.Method | lower | title}}(options.BaseURL + "{{.Path | swaggerUriToIrisUri}}", wrapper.{{.HandlerName}}) +{{range .}}router.{{.Method | lower | title}}(options.BaseURL + {{.Path | swaggerUriToIrisUri | toGoString}}, wrapper.{{.HandlerName}}) {{end}} router.Build() } diff --git a/pkg/codegen/templates/server-urls.tmpl b/pkg/codegen/templates/server-urls.tmpl index ccb1786e73..b0cf2a86a5 100644 --- a/pkg/codegen/templates/server-urls.tmpl +++ b/pkg/codegen/templates/server-urls.tmpl @@ -3,8 +3,8 @@ {{ $undeclared := .UndeclaredPlaceholders }} {{ if and (eq 0 (len $usedVars)) (eq 0 (len $undeclared)) }} {{/* URLs without variables (declared or used) are straightforward, so we'll create them a constant */}} -// {{ .GoName }} defines the Server URL for {{ if len .OAPISchema.Description }}{{ .OAPISchema.Description }}{{ else }}{{ .OAPISchema.URL }}{{ end }} -const {{ .GoName}} = "{{ .OAPISchema.URL }}" +// {{ .GoName }} defines the Server URL for {{ if len .OAPISchema.Description }}{{ stripNewLines .OAPISchema.Description }}{{ else }}{{ stripNewLines .OAPISchema.URL }}{{ end }} +const {{ .GoName}} = {{ .OAPISchema.URL | toGoString }} {{ else }} {{/* URLs with variables are not straightforward, as we may need multiple types, and so will model them as a function */}} @@ -36,7 +36,7 @@ const {{ .GoName}} = "{{ .OAPISchema.URL }}" type {{ $prefix }} string {{ if $v.Default }} // {{ $prefix }}Default is the default value for the `{{ $k }}` variable for {{ $goName }} - const {{ $prefix }}Default = "{{ $v.Default }}" + const {{ $prefix }}Default = {{ $v.Default | toGoString }} {{ end }} {{ end }} {{ end }} @@ -55,7 +55,7 @@ const {{ .GoName}} = "{{ .OAPISchema.URL }}" {{ end }} -// New{{ .GoName }} constructs the Server URL for {{ .OAPISchema.Description }}, with the provided variables. +// New{{ .GoName }} constructs the Server URL for {{ stripNewLines .OAPISchema.Description }}, with the provided variables. func New{{ .GoName }}({{ .NewServerFunctionParams }}) (string, error) { {{ range $k, $v := $usedVars }} {{- if gt (len $v.Enum) 0 -}} @@ -64,7 +64,7 @@ func New{{ .GoName }}({{ .NewServerFunctionParams }}) (string, error) { } {{ end -}} {{ end }} - u := "{{ .OAPISchema.URL }}" + u := {{ .OAPISchema.URL | toGoString }} {{ range $k, $v := $usedVars }} {{- $placeholder := printf "{%s}" $k -}} diff --git a/pkg/codegen/templates/stdhttp/std-http-handler.tmpl b/pkg/codegen/templates/stdhttp/std-http-handler.tmpl index 63fccd2445..676748599d 100644 --- a/pkg/codegen/templates/stdhttp/std-http-handler.tmpl +++ b/pkg/codegen/templates/stdhttp/std-http-handler.tmpl @@ -49,7 +49,7 @@ func HandlerWithOptions(si ServerInterface, options StdHTTPServerOptions) http.H ErrorHandlerFunc: options.ErrorHandlerFunc, } {{end}} -{{range .}}m.HandleFunc({{.Method | httpMethodConstant}}+" "+options.BaseURL+"{{.Path | swaggerUriToStdHttpUri}}", wrapper.{{.HandlerName}}) +{{range .}}m.HandleFunc({{.Method | httpMethodConstant}}+" "+options.BaseURL+{{.Path | swaggerUriToStdHttpUri | toGoString}}, wrapper.{{.HandlerName}}) {{end}} return m } diff --git a/pkg/codegen/utils.go b/pkg/codegen/utils.go index bc1763ed5b..496faf5648 100644 --- a/pkg/codegen/utils.go +++ b/pkg/codegen/utils.go @@ -905,10 +905,12 @@ func PathToTypeName(path []string) string { } // StringToGoString takes an arbitrary string and converts it to a valid Go string literal, -// including the quotes. For instance, `foo "bar"` would be converted to `"foo \"bar\""` +// including the quotes. For instance, `foo "bar"` would be converted to `"foo \"bar\""`. +// +// strconv.Quote escapes backslashes, newlines and other control characters too, +// so untrusted spec text cannot break out of the generated string literal. func StringToGoString(in string) string { - esc := strings.ReplaceAll(in, "\"", "\\\"") - return fmt.Sprintf("\"%s\"", esc) + return strconv.Quote(in) } // StringToGoComment renders a possible multi-line string as a valid Go-Comment. diff --git a/pkg/codegen/utils_test.go b/pkg/codegen/utils_test.go index 9afe8b2d2e..df37c6d77a 100644 --- a/pkg/codegen/utils_test.go +++ b/pkg/codegen/utils_test.go @@ -544,6 +544,19 @@ func TestStringToGoStringValue(t *testing.T) { expected: `"application/json; foo=\"bar\""`, message: "string with quotes should include escape characters", }, + { + // The previous implementation only escaped `"`, so a backslash + // before a quote (`\"`) escaped the escaping and let untrusted + // spec text break out of the generated string literal. + input: `a\"; var Evil = 1; var _ = "`, + expected: `"a\\\"; var Evil = 1; var _ = \""`, + message: "backslashes must be escaped so a quote cannot break out of the literal", + }, + { + input: "line1\nline2", + expected: `"line1\nline2"`, + message: "newlines must be escaped so they cannot terminate the literal", + }, } for _, testCase := range testCases { t.Run(testCase.message, func(t *testing.T) {