Repository navigation
Description
Follow-up to #1335 (merged as-is per maintainer decision, with this issue tracking the known debt).
1. 🔴 GET /v0/servers 500 path leaks internal error detail to the client (primary)
#1335 refactored the list error handling into ListServersError (internal/api/handlers/v0/list_errors.go) and, in the process, changed the genuine-failure branch from:
return nil, huma.Error500InternalServerError("Failed to get registry list")to:
return huma.Error500InternalServerError("Failed to get registry list", err) // err is newThat extra err is not server-side only. Verified against huma v2.38.0:
Error500InternalServerError(msg, errs...)→NewError(error.go:231) stores each err as&ErrorDetail{Message: err.Error()}inErrorModel.Errors.ErrorModel.Errorsis JSON-serialized:`json:"errors,omitempty"`(error.go:91).- No custom
huma.NewErroroverride or response transformer exists ininternal/, so the default serialization applies.
So on a real DB failure, the response body on the public, unauthenticated GET /v0/servers endpoint now contains the wrapped error from internal/database/postgres.go:262 (error iterating rows: <pgx error>). pgx/pgconn errors can carry SQLSTATE codes, constraint/table/column names, and internal messages → verbose-error information disclosure (CWE-209).
This also diverges from the rest of the codebase, which logs err server-side and returns only a generic message: servers.go:190, servers.go:220, edit.go:79, status.go:127/234/250.
Fix: drop err from the 500 call, keep the server-side log.
log.Printf("list servers failed: %v", err)
return huma.Error500InternalServerError("Failed to get registry list")Test gap: list_errors_test.go asserts status codes only. Add an assertion that the 500 response body does not contain the raw error string.
2. 🟡 PR description claim: "Prevents superfluous response.WriteHeader warnings"
This part of #1335 isn't actually achieved. The warning originates inside huma's transformAndWrite (huma.go): ctx.SetStatus(status) at ~line 619 (first WriteHeader), then if the body-marshal write fails and ctx.Err() == context.Canceled, ctx.SetStatus(499) at ~line 627 (second WriteHeader) → the warning. Returning huma.NewError(499, …) flows through the same WriteErr → writeResponse → transformAndWrite path, so the double-SetStatus is unchanged — it's status-code-agnostic.
The 499 status + dropped error-level log from #1335 are real improvements; the WriteHeader suppression is not. Either accept the warning as a benign huma-internal artifact, or pursue an early return that doesn't invoke the error-response writer (as the original issue #1323 suggested).
3. 🟢 Nits
- Reuse
statusClientClosed(internal/api/router/router.go:28) instead of the literal499inlist_errors.go. - Cancellation is now classified in two places — the handler (
list_errors.go) andMetricTelemetryMiddleware(router.go:90). Keep them in sync. - The 499 branch also passes
errinto the serialized body — harmless (client has disconnected) but inconsistent; could omit. - The guard only covers the list handler; the sibling handlers and the second
error iterating rowssite (postgres.go:479) keep the old behavior. Fine for now since those don't leakerr, but the cancellation-logging cleanup is partial vs. what /v0/servers logs benign client cancellations as "list servers failed" and triggers "superfluous response.WriteHeader" warnings #1323 suggested.
Generated from a code review of #1335.
Activity
fix: client-cancelled GET /v0/servers returns 499 without error log (#…
fix: don't leak internal error detail in GET /v0/servers 500 response (…