Improve string escaping and add tests#2396
Conversation
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 <noreply@anthropic.com>
Greptile SummaryThis PR hardens generated server URL output against malformed Go source. It changes:
Confidence Score: 3/5This should be fixed before merging.
pkg/codegen/templates/server-urls.tmpl and the enum constant generation path for server URL variables.
|
| Filename | Overview |
|---|---|
| pkg/codegen/templates/server-urls.tmpl | Updates server URL generation to quote URL/default literals and flatten comment text, but leaves adjacent spec-controlled server variable surfaces unescaped. |
| pkg/codegen/utils.go | Switches generic Go string literal generation to strconv.Quote. |
| pkg/codegen/template_helpers.go | Extends newline stripping for generated line comments to include carriage returns. |
Comments Outside Diff (1)
-
pkg/codegen/templates/server-urls.tmpl, line 71-75 (link)Sanitize placeholder names Server URL placeholder names are still inserted raw into generated comments, parameter identifiers, and replacement string literals.
urlPlaceholdersaccepts any text except/,{, and}, so a spec can use a matching variable name containing quotes or newlines and make this template generate invalid Go or break out of the string/comment contexts that this PR is trying to harden. The placeholder name needs the same context-aware escaping or validation as the URL and default values.
Reviews (2): Last reviewed commit: "Improve string escaping and add tests" | Re-trigger Greptile
|
@greptileai - I believe you are mistaken in your finding. I am concerned about injected Go code that compiles. The finding you have outside of the scope of this PR is not actually compilable. Any injection I've tried produces corrupt Go, and a non-compiling file, which isn't exploitable. I will leave it as-is. |
|
That's a fair point, and you're right to draw that distinction. Looking at const Foo MyType = "{{$value}}"For any injection to be compilable, an attacker would need to not only close the string but also emit valid Go in a That's a meaningful distinction. A code-generation tool producing a corrupt, non-compiling file is a user-experience problem; producing a compiling file with injected behavior would be a security problem. Since you've verified the former is what actually happens here, I withdraw the concern. Sorry for the noise. |
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:
Add regression tests covering the server-urls rendering and the StringToGoString escaping, and confirm
make generateproduces no changes to committed output.