From fecef4f9ed5ad063b8c726dc7fca4eb69dba69a3 Mon Sep 17 00:00:00 2001 From: Radoslav Dimitrov Date: Fri, 5 Jun 2026 03:08:33 -0400 Subject: [PATCH] fix: don't leak internal error detail in GET /v0/servers 500 response (#1338) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Fixes the information-disclosure regression introduced by #1335 (item 1 of #1337). `ListServersError` passed the raw `err` into `huma.Error500InternalServerError(...)`. huma serializes extra error args into the response body's `errors` array (`ErrorModel.Errors`, `json:"errors,omitempty"`), so on the **public, unauthenticated** `GET /v0/servers` endpoint a genuine DB failure echoed the internal wrapped error from `internal/database/postgres.go` (`error iterating rows: `) back to the client. pgx/pgconn errors can carry SQLSTATE codes, constraint/table/column names, and internal messages (CWE-209). ## Change - Drop `err` from the 500 call so only the generic message is returned; keep `log.Printf` for the server-side detail — matching the sibling handlers (`servers.go:190/220`, `edit.go`, `status.go`). - Add a regression test (`TestListServersError_realFailureDoesNotLeakDetail`) that marshals the 500 error and asserts the internal detail is absent from the client-visible body. Existing tests only asserted status codes, which is why the leak slipped through. The 499 client-cancelled path is unchanged (the client has already disconnected, so nothing is leaked there). ## Test plan - [x] `go test ./internal/api/handlers/v0/... -run ListServersError` (3/3 pass) - [x] `go build ./...`, `go vet`, `golangci-lint` (no new findings in changed files) ## Note This leaves the non-security items in #1337 (correcting the `superfluous WriteHeader` claim, `statusClientClosed` const reuse, broader cancellation-guard scope) for separate follow-up. Closing #1337 here since the only security-impacting item is addressed; reopen/split if you'd prefer to track the nits independently. Closes #1337 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 (1M context) --- internal/api/handlers/v0/list_errors.go | 5 ++++- internal/api/handlers/v0/list_errors_test.go | 14 ++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/internal/api/handlers/v0/list_errors.go b/internal/api/handlers/v0/list_errors.go index 8ef89a1..b83f04e 100644 --- a/internal/api/handlers/v0/list_errors.go +++ b/internal/api/handlers/v0/list_errors.go @@ -17,5 +17,8 @@ func ListServersError(ctx context.Context, err error) error { return huma.NewError(499, "Client closed request", err) } log.Printf("list servers failed: %v", err) - return huma.Error500InternalServerError("Failed to get registry list", err) + // Do not pass err here: huma serializes extra error args into the response + // body, which would leak internal (e.g. pgx) error detail to clients. Log it + // server-side only, like the sibling handlers in servers.go. + return huma.Error500InternalServerError("Failed to get registry list") } diff --git a/internal/api/handlers/v0/list_errors_test.go b/internal/api/handlers/v0/list_errors_test.go index 9baca41..7c50d38 100644 --- a/internal/api/handlers/v0/list_errors_test.go +++ b/internal/api/handlers/v0/list_errors_test.go @@ -2,6 +2,7 @@ package v0_test import ( "context" + "encoding/json" "errors" "testing" @@ -29,3 +30,16 @@ func TestListServersError_realFailure(t *testing.T) { require.True(t, errors.As(err, &se)) assert.Equal(t, 500, se.GetStatus()) } + +// The 500 response body must not echo the internal error back to the client, +// since it can carry DB/driver detail (CWE-209). +func TestListServersError_realFailureDoesNotLeakDetail(t *testing.T) { + const internal = "database unavailable: dsn=postgres://user:pw@internal-host/db" + err := v0.ListServersError(context.Background(), errors.New(internal)) + require.Error(t, err) + + body, marshalErr := json.Marshal(err) + require.NoError(t, marshalErr) + assert.NotContains(t, string(body), "database unavailable") + assert.NotContains(t, string(body), "internal-host") +}