echo: don't echo Go error messages into HTTP responses#2370
Conversation
The echo wrapper template emits four parameter-bind error paths
(path / query / header / cookie) that currently surface the raw
runtime/Bind*ParameterWithOptions error into the HTTP response:
return echo.NewHTTPError(
http.StatusBadRequest,
fmt.Sprintf("Invalid format for parameter X: %s", err))
The string returned by err.Error() is implementation detail of
runtime/types/parsing — it can include internal struct field names,
package-local error wording, or library version-dependent text.
Production servers shouldn't echo that to API consumers.
Replace each with:
// echo (v4)
return echo.NewHTTPError(
http.StatusBadRequest,
fmt.Sprintf("Invalid format for parameter X: '%s'", <user-value>),
).SetInternal(err)
// echo v5 — same idea, different API:
return echo.NewHTTPError(
http.StatusBadRequest,
fmt.Sprintf("Invalid format for parameter X: '%s'", <user-value>),
).Wrap(err)
The HTTP body now reflects what the caller sent (`ctx.Param(...)` /
`ctx.QueryParams().Get(...)` / `valueList[0]` / `cookie.Value`), and
the original err is preserved on the echo HTTPError as either
SetInternal (v4) or Wrap (v5) so it still flows to server logs but
never to the wire.
Regenerated test fixtures and examples to match.
Greptile SummaryThis PR changes the echo and echo-v5 server templates so that 400 Bad Request error responses no longer include the raw Go error string — instead they echo back the user-supplied value in quotes (e.g.
Confidence Score: 3/5The template logic is correct and generated fixtures are internally consistent, but the change unconditionally rewrites the HTTP 400 response body for all echo users without a migration path. The four error-message sites in both echo templates are updated correctly and symmetrically, and all regenerated files match the templates with no unrelated drift. However, every generated echo server will emit a different 400 body on the next codegen run — clients, integration tests, or monitoring rules that pattern-match on the previous error wording will silently break. The project's convention is to gate unconditional wire-format changes behind a compatibility or output flag, which is absent here. There is also no new behavioral test to lock in the new format going forward. pkg/codegen/templates/echo/echo-wrappers.tmpl and its v5 counterpart are the source of truth; the generated files are fine on their own.
|
| Filename | Overview |
|---|---|
| pkg/codegen/templates/echo/echo-wrappers.tmpl | Core source of all changes: replaces %s err-in-message with '%s' user-value + .SetInternal(err) for path/query/header/cookie error paths; unconditional behavior change with no compatibility flag. |
| pkg/codegen/templates/echo/v5/echo-wrappers.tmpl | Echo v5 equivalent of the same four-site change; uses .Wrap(err) instead of .SetInternal(err) matching v5's API — correct and symmetric with the v4 template. |
| internal/test/parameters/echo/gen/server.gen.go | All 44 error-message sites regenerated consistently with the template; no unrelated drift observed, but no new behavior test accompanies the fixture update. |
| internal/test/parameters/echov5/server.gen.go | Echo v5 parameter test fixtures regenerated symmetrically with v4; changes are consistent with the template. |
| examples/petstore-expanded/echo/api/petstore-server.gen.go | Petstore echo example regenerated; 4 error-path lines updated correctly, no unrelated changes. |
| examples/petstore-expanded/echo-v5/api/petstore-server.gen.go | Petstore echo-v5 example regenerated; 4 error-path lines updated with .Wrap(err) correctly. |
Reviews (1): Last reviewed commit: "echo: don't put Go err.Error() into HTTP..." | Re-trigger Greptile
| if err != nil { | ||
| return echo.NewHTTPError(http.StatusBadRequest, fmt.Sprintf("Invalid format for parameter {{.ParamName}}: %s", err)) | ||
| return echo.NewHTTPError(http.StatusBadRequest, fmt.Sprintf("Invalid format for parameter {{.ParamName}}: '%s'", ctx.Param("{{.ParamName}}"))).SetInternal(err) | ||
| } |
There was a problem hiding this comment.
This modifies the HTTP 400 response body for every generated echo server unconditionally. Per the project's practice, any change to generated output that alters observable wire behavior should be opt-in via a new compatibility or output-options flag, not a silent change to all users. Downstream consumers — integration tests, API gateways checking error bodies, client libraries parsing these messages — will silently break without a migration path. The same applies to the query, header, and cookie error paths below (and in the v5 template). Consider gating behind a flag like output-options.echo-error-include-value (or similar) so existing deployments can keep the current behavior.
| err = runtime.BindQueryParameterWithOptions("{{.Style}}", {{.Explode}}, {{.Required}}, "{{.ParamName}}", ctx.QueryParams(), ¶ms.{{.GoName}}, runtime.BindQueryParameterOptions{Type: "{{.SchemaType}}", Format: "{{.SchemaFormat}}"}) | ||
| if err != nil { | ||
| return echo.NewHTTPError(http.StatusBadRequest, fmt.Sprintf("Invalid format for parameter {{.ParamName}}: %s", err)) | ||
| return echo.NewHTTPError(http.StatusBadRequest, fmt.Sprintf("Invalid format for parameter {{.ParamName}}: '%s'", ctx.QueryParams().Get("{{.ParamName}}"))).SetInternal(err) | ||
| } |
There was a problem hiding this comment.
QueryParams().Get() drops all but the first value for multi-value parameters
For array-typed query parameters sent as repeated keys (e.g. ?tags=foo&tags=bar), ctx.QueryParams().Get("tags") returns only "foo". When binding fails, the error message Invalid format for parameter tags: 'foo' omits bar entirely, which may be confusing when the actual submitted multi-value list was the real source of the parsing failure. Consider using ctx.QueryParams()["paramName"] joined with commas, or ctx.Request().URL.RawQuery, to give a more faithful representation of what the caller actually sent.
| var value int32 | ||
| err = runtime.BindStyledParameterWithOptions("simple", "p", cookie.Value, &value, runtime.BindStyledParameterOptions{ParamLocation: runtime.ParamLocationCookie, Explode: false, Required: false, Type: "integer", Format: "int32"}) | ||
| if err != nil { | ||
| return echo.NewHTTPError(http.StatusBadRequest, fmt.Sprintf("Invalid format for parameter p: %s", err)) | ||
| return echo.NewHTTPError(http.StatusBadRequest, fmt.Sprintf("Invalid format for parameter p: '%s'", cookie.Value)).SetInternal(err) |
There was a problem hiding this comment.
The test fixtures were regenerated, but no test was added to internal/test/parameters/echo/ (or the echo-v5 counterpart) that actually issues a malformed request and asserts the new '<value>'-style response body. Without this, a future template edit could silently revert the format without CI catching it. A table-driven test covering path/query/header/cookie bad-value cases would make the contract explicit.
What this is
A small change to the echo and echo-v5 server templates so that the error responses generated servers send back to clients don't include the underlying Go error text.
What's changing today
When a request to a generated echo server has a parameter that doesn't parse (e.g. a path parameter expecting an int but the client sent
"foo"), the response currently looks like this:Everything after the colon is whatever
err.Error()happened to return — it comes fromruntime.BindStyledParameterWithOptions/BindQueryParameterWithOptionsand the underlyingstrconv/timeparser. That text isn't part of any contract — it's whatever Go's standard library or oapi-codegen-runtime felt like saying at the time. Sending it on the wire means:Why
Two reasons:
Security / info disclosure. Pentest reports routinely flag echoing raw
err.Error()back to API callers as low-but-real info leakage. The current message can include Go stdlib parser internals (strconv.ParseInt: …), runtime/types implementation detail, or library-version-dependent wording. Each message in isolation looks innocuous, but the cumulative shape exposes how parameters get parsed server-side. Production-conscious teams currently have to fork or post-process this template to satisfy that finding — that's the whole reason we've maintained a local override for over a year.More useful for the actual caller. When somebody sends
id=foo, the helpful thing to tell them is "the value'foo'didn't fit parameterid". The current message tells them which Go function failed and how, which is information for whoever wrote the server, not whoever is calling it. Quoting the offending value puts the response back in the API consumer's frame.The original error isn't lost — it stays attached to the HTTPError (via
SetInternalin v4 /Wrapin v5), so server-side error handlers still log it.What it changes to
The same response becomes:
i.e. the value the caller actually sent. The original error is still attached to the echo HTTPError (via
SetInternal(err)in v4 andWrap(err)in v5), so server-side error handlers — including echo's default — still log the diagnostic. Just nothing leaks to the wire.Concretely, the diff
In
pkg/codegen/templates/echo/echo-wrappers.tmplandpkg/codegen/templates/echo/v5/echo-wrappers.tmpl, four spots (path / query / header / cookie parameter binds) change from:<user-value>is the right input source for that location:ctx.Param("X")ctx.QueryParams().Get("X")valueList[0]cookie.ValueFor
pkg/codegen/templates/echo/v5/echo-wrappers.tmpl,.SetInternal(err)becomes.Wrap(err)(v5's equivalent —*echo.HTTPErrordoesn't haveSetInternal, but it does have aWrap(err)method that attaches an inner error).What about the other framework templates?
Out of scope for this PR. The chi / gorilla / fiber / gin / iris / stdhttp templates have the same shape (
http.Error(w, err.Error(), …)), and the same argument applies, but I want this PR to be small and easy to review. Happy to do those in follow-ups if this lands.Why this is safe
Internal/ wrapped) so logging is unaffected.Tests
Regenerated all the affected test fixtures (
internal/test/parameters/echo,internal/test/parameters/echov5,internal/test/strict-server/echo, the petstore-expanded examples).go test ./...is clean across the root, examples, and internal/test modules.Background
We've shipped this as a local template override for over a year and it's surfaced no issues — the change is small, the contract is preserved, and the request/response shape is unchanged for valid traffic. Upstreaming so we don't have to maintain the override.