From 19c6282e9a6fb84b51aa92b12fad1f0b7e5f5ef6 Mon Sep 17 00:00:00 2001 From: Marcin Romaszewicz Date: Fri, 5 Jun 2026 07:21:56 -0700 Subject: [PATCH 1/2] Improve string escaping and add tests (#2396) Tighten how server URL values are emitted in the server-urls template so that spec-supplied text is always serialized into a proper Go context: - Route server descriptions and URLs through stripNewLines when they are written into generated // comments, so a multi-line value stays on the comment line instead of spilling into following lines. - Emit server URL and variable-default values via toGoString (now backed by strconv.Quote) instead of hand-wrapping them in quotes, so quotes, backslashes, and newlines are escaped correctly. - Harden StringToGoString to use strconv.Quote; the previous version only escaped double quotes and mishandled backslashes. This also tidies up the content-type literals that already used it. - Extend stripNewLines to drop carriage returns as well as newlines. Add regression tests covering the server-urls rendering and the StringToGoString escaping, and confirm `make generate` produces no changes to committed output. Co-authored-by: Claude Opus 4.8 --- pkg/codegen/server_urls_test.go | 114 +++++++++++++++++++++++++ pkg/codegen/template_helpers.go | 4 +- pkg/codegen/templates/server-urls.tmpl | 10 +-- pkg/codegen/utils.go | 8 +- pkg/codegen/utils_test.go | 13 +++ 5 files changed, 140 insertions(+), 9 deletions(-) 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/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/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) { From 3c4a475773edd442e5cfb6d9585d2f3311c94f8f Mon Sep 17 00:00:00 2001 From: Marcin Romaszewicz Date: Sun, 5 Jul 2026 19:15:11 -0700 Subject: [PATCH 2/2] Make sure to escape user strings (#2433) (#2434) Spec-derived paths and string enum values were interpolated into generated Go source without escaping, so a value containing a quote could produce malformed output. Route paths in the server registration templates and string enum constants are now emitted through strconv.Quote (the existing toGoString helper) so they are always valid, properly-escaped Go string literals. Generated output is unchanged for normal specs. Co-authored-by: Claude Fable 5 --- pkg/codegen/templates/chi/chi-handler.tmpl | 2 +- pkg/codegen/templates/constants.tmpl | 2 +- pkg/codegen/templates/echo/echo-register.tmpl | 2 +- pkg/codegen/templates/echo/v5/echo-register.tmpl | 2 +- pkg/codegen/templates/fiber/fiber-handler.tmpl | 2 +- pkg/codegen/templates/gin/gin-register.tmpl | 2 +- pkg/codegen/templates/gorilla/gorilla-register.tmpl | 2 +- pkg/codegen/templates/iris/iris-handler.tmpl | 2 +- pkg/codegen/templates/stdhttp/std-http-handler.tmpl | 2 +- 9 files changed, 9 insertions(+), 9 deletions(-) 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/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 }