Make sure to escape user strings (#2433) - #2434
Conversation
Sorry, something went wrong.
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 <noreply@anthropic.com>
Greptile SummaryThis PR fixes a code-injection bug where spec-derived path strings and string enum values were interpolated raw into Go source, allowing a value containing a double-quote to produce malformed generated code. All seven router-backend templates and constants.tmpl are updated to pipe the relevant values through the existing toGoString helper (strconv.Quote), which adds the surrounding double-quotes and escapes any special characters.
Confidence Score: 4/5Safe to merge — the template changes are mechanically correct, generated output is byte-for-byte identical for normal (no-special-char) specs, and all backends are updated consistently. The fix is straightforward and low-risk, but there is no new regression test in internal/test/issues/ that exercises the boundary case (a string enum value or route path containing a double-quote). The project’s CI guard (make generate) cannot detect a future re-introduction of the unescaped interpolation without such a fixture. constants.tmpl — the enum-escaping logic is the more novel part of the change and most benefits from a targeted test fixture. Important Files Changed
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
pkg/codegen/templates/constants.tmpl:12
**Missing regression test for the injection fix**
The project convention (as codified in the review rules and via `internal/test/issues/`) is that bug fixes include a regression test keyed to the issue number. This PR has no new entry under `internal/test/issues/` covering a string enum value that contains a quote (e.g. `fo"o`) or a route path with a backslash or quote. Without such a fixture, CI's `make generate` guard cannot detect a future regression that re-introduces the unescaped interpolation, and there is no `_test.go` exercising the boundary condition.
Reviews (1): Last reviewed commit: "Make sure to escape user strings (#2433)" | Re-trigger Greptile |
Sorry, something went wrong.
| 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 -}} |
There was a problem hiding this comment.
Missing regression test for the injection fix
The project convention (as codified in the review rules and via internal/test/issues/) is that bug fixes include a regression test keyed to the issue number. This PR has no new entry under internal/test/issues/ covering a string enum value that contains a quote (e.g. fo"o) or a route path with a backslash or quote. Without such a fixture, CI's make generate guard cannot detect a future regression that re-introduces the unescaped interpolation, and there is no _test.go exercising the boundary condition.
Prompt To Fix With AIThis is a comment left during a code review.
Path: pkg/codegen/templates/constants.tmpl
Line: 12
Comment:
**Missing regression test for the injection fix**
The project convention (as codified in the review rules and via `internal/test/issues/`) is that bug fixes include a regression test keyed to the issue number. This PR has no new entry under `internal/test/issues/` covering a string enum value that contains a quote (e.g. `fo"o`) or a route path with a backslash or quote. Without such a fixture, CI's `make generate` guard cannot detect a future regression that re-introduces the unescaped interpolation, and there is no `_test.go` exercising the boundary condition.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Sorry, something went wrong.
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.