From 32fb0666529d8ad565a2cb2c3f621a293c1a060c Mon Sep 17 00:00:00 2001 From: Maxence Maireaux Date: Thu, 11 Jun 2026 09:52:59 +0200 Subject: [PATCH 1/2] fix(api): harden request parsing against panics and oversized bodies parsePageSize panicked on a non-numeric pageSize (e.g. ?pageSize=abc) and the router had no recoverer, so the client got a reset connection instead of a 400. Negative and unbounded page sizes were also passed straight to the ledger. - parsePageSize returns an error instead of panicking; reject < 1 and clamp to maxPageSize (100); list handlers return 400 VALIDATION on bad input - add middleware.Recoverer as a safety net - cap request bodies with http.MaxBytesReader (1 MiB) ahead of the audit middleware, which buffers the whole body, to prevent memory-exhaustion DoS Adds unit tests for parsePageSize bounds and a list handler 400 case. --- pkg/api/handler_balances_list.go | 6 +++- pkg/api/handler_holds_list.go | 6 +++- pkg/api/handler_transactions_list.go | 6 +++- pkg/api/handler_wallets_list.go | 6 +++- pkg/api/router.go | 7 +++++ pkg/api/utils.go | 30 +++++++++++++----- pkg/api/utils_test.go | 47 ++++++++++++++++++++++++++++ 7 files changed, 96 insertions(+), 12 deletions(-) diff --git a/pkg/api/handler_balances_list.go b/pkg/api/handler_balances_list.go index f4d4236..f551ba6 100644 --- a/pkg/api/handler_balances_list.go +++ b/pkg/api/handler_balances_list.go @@ -8,12 +8,16 @@ import ( ) func (m *MainHandler) listBalancesHandler(w http.ResponseWriter, r *http.Request) { - query := readPaginatedRequest(r, func(r *http.Request) wallet.ListBalances { + query, err := readPaginatedRequest(r, func(r *http.Request) wallet.ListBalances { return wallet.ListBalances{ WalletID: chi.URLParam(r, "walletID"), Metadata: getQueryMap(r.URL.Query(), "metadata"), } }) + if err != nil { + badRequest(w, ErrorCodeValidation, err) + return + } holds, err := m.manager.ListBalances(r.Context(), query) if err != nil { diff --git a/pkg/api/handler_holds_list.go b/pkg/api/handler_holds_list.go index 30d9286..559aac6 100644 --- a/pkg/api/handler_holds_list.go +++ b/pkg/api/handler_holds_list.go @@ -7,12 +7,16 @@ import ( ) func (m *MainHandler) listHoldsHandler(w http.ResponseWriter, r *http.Request) { - query := readPaginatedRequest(r, func(r *http.Request) wallet.ListHolds { + query, err := readPaginatedRequest(r, func(r *http.Request) wallet.ListHolds { return wallet.ListHolds{ WalletID: r.URL.Query().Get("walletID"), Metadata: getQueryMap(r.URL.Query(), "metadata"), } }) + if err != nil { + badRequest(w, ErrorCodeValidation, err) + return + } holds, err := m.manager.ListHolds(r.Context(), query) if err != nil { diff --git a/pkg/api/handler_transactions_list.go b/pkg/api/handler_transactions_list.go index c9a6318..bed1eb5 100644 --- a/pkg/api/handler_transactions_list.go +++ b/pkg/api/handler_transactions_list.go @@ -7,11 +7,15 @@ import ( ) func (m *MainHandler) listTransactions(w http.ResponseWriter, r *http.Request) { - query := readPaginatedRequest[wallet.ListTransactions](r, func(r *http.Request) wallet.ListTransactions { + query, err := readPaginatedRequest[wallet.ListTransactions](r, func(r *http.Request) wallet.ListTransactions { return wallet.ListTransactions{ WalletID: r.URL.Query().Get("walletID"), } }) + if err != nil { + badRequest(w, ErrorCodeValidation, err) + return + } transactions, err := m.manager.ListTransactions(r.Context(), query) if err != nil { internalError(w, r, err) diff --git a/pkg/api/handler_wallets_list.go b/pkg/api/handler_wallets_list.go index 6eee169..aa48a95 100644 --- a/pkg/api/handler_wallets_list.go +++ b/pkg/api/handler_wallets_list.go @@ -7,13 +7,17 @@ import ( ) func (m *MainHandler) listWalletsHandler(w http.ResponseWriter, r *http.Request) { - query := readPaginatedRequest[wallet.ListWallets](r, func(r *http.Request) wallet.ListWallets { + query, err := readPaginatedRequest[wallet.ListWallets](r, func(r *http.Request) wallet.ListWallets { return wallet.ListWallets{ Metadata: getQueryMap(r.URL.Query(), "metadata"), Name: r.URL.Query().Get("name"), ExpandBalances: r.URL.Query().Get("expand") == "balances", } }) + if err != nil { + badRequest(w, ErrorCodeValidation, err) + return + } response, err := m.manager.ListWallets(r.Context(), query) if err != nil { internalError(w, r, err) diff --git a/pkg/api/router.go b/pkg/api/router.go index e3e8144..d9092c9 100644 --- a/pkg/api/router.go +++ b/pkg/api/router.go @@ -16,6 +16,11 @@ import ( "github.com/go-chi/chi/v5/middleware" ) +// maxRequestBodyBytes caps the size of request bodies the service will read. +// The JSON payloads handled here are small; this protects the service (and the +// audit middleware, which buffers the whole body) from memory-exhaustion DoS. +const maxRequestBodyBytes = 1 << 20 // 1 MiB + func NewRouter( manager *wallet.Manager, healthController *sharedhealth.HealthController, @@ -25,8 +30,10 @@ func NewRouter( ) *chi.Mux { r := chi.NewRouter() + r.Use(middleware.Recoverer) r.Use(func(handler http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + r.Body = http.MaxBytesReader(w, r.Body, maxRequestBodyBytes) w.Header().Set("Content-Type", "application/json") handler.ServeHTTP(w, r) }) diff --git a/pkg/api/utils.go b/pkg/api/utils.go index 6b4f154..eb061fb 100644 --- a/pkg/api/utils.go +++ b/pkg/api/utils.go @@ -2,6 +2,7 @@ package api import ( "encoding/json" + "fmt" "io" "net/http" "strconv" @@ -14,7 +15,10 @@ import ( wallet "github.com/formancehq/wallets/pkg" ) -const defaultLimit = 15 +const ( + defaultLimit = 15 + maxPageSize = 100 +) func notFound(w http.ResponseWriter) { w.WriteHeader(http.StatusNotFound) @@ -81,31 +85,41 @@ func parsePaginationToken(r *http.Request) string { return r.URL.Query().Get("cursor") } -func parsePageSize(r *http.Request) int { +func parsePageSize(r *http.Request) (int, error) { pageSize := r.URL.Query().Get("pageSize") if pageSize == "" { - return defaultLimit + return defaultLimit, nil } v, err := strconv.ParseInt(pageSize, 10, 32) if err != nil { - panic(err) + return 0, fmt.Errorf("invalid pageSize: %w", err) + } + if v < 1 { + return 0, fmt.Errorf("pageSize must be a positive integer") + } + if v > maxPageSize { + v = maxPageSize } - return int(v) + return int(v), nil } -func readPaginatedRequest[T any](r *http.Request, f func(r *http.Request) T) wallet.ListQuery[T] { +func readPaginatedRequest[T any](r *http.Request, f func(r *http.Request) T) (wallet.ListQuery[T], error) { + pageSize, err := parsePageSize(r) + if err != nil { + return wallet.ListQuery[T]{}, err + } var payload T if f != nil { payload = f(r) } return wallet.ListQuery[T]{ Pagination: wallet.Pagination{ - Limit: parsePageSize(r), + Limit: pageSize, PaginationToken: parsePaginationToken(r), }, Payload: payload, - } + }, nil } func getQueryMap(m map[string][]string, key string) map[string]string { diff --git a/pkg/api/utils_test.go b/pkg/api/utils_test.go index cd91170..155201b 100644 --- a/pkg/api/utils_test.go +++ b/pkg/api/utils_test.go @@ -24,6 +24,53 @@ import ( "github.com/stretchr/testify/require" ) +func TestParsePageSize(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + query string + expected int + expectErr bool + }{ + {query: "", expected: defaultLimit}, + {query: "pageSize=20", expected: 20}, + {query: "pageSize=100", expected: maxPageSize}, + {query: "pageSize=100000", expected: maxPageSize}, + {query: "pageSize=abc", expectErr: true}, + {query: "pageSize=0", expectErr: true}, + {query: "pageSize=-5", expectErr: true}, + } { + tc := tc + t.Run(tc.query, func(t *testing.T) { + t.Parallel() + req := httptest.NewRequest(http.MethodGet, "/?"+tc.query, nil) + v, err := parsePageSize(req) + if tc.expectErr { + require.Error(t, err) + return + } + require.NoError(t, err) + require.Equal(t, tc.expected, v) + }) + } +} + +func TestListHandlerRejectsInvalidPageSize(t *testing.T) { + t.Parallel() + + testEnv := newTestEnv( + WithListAccounts(func(ctx context.Context, ledger string, query wallet.ListAccountsQuery) (*wallet.AccountsCursorResponseCursor, error) { + return &wallet.AccountsCursorResponseCursor{}, nil + }), + ) + req := httptest.NewRequest(http.MethodGet, "/wallets?pageSize=abc", nil) + rec := httptest.NewRecorder() + testEnv.Router().ServeHTTP(rec, req) + + require.Equal(t, http.StatusBadRequest, rec.Result().StatusCode) + require.Equal(t, ErrorCodeValidation, readErrorResponse(t, rec).ErrorCode) +} + func readErrorResponse(t *testing.T, rec *httptest.ResponseRecorder) *sharedapi.ErrorResponse { t.Helper() ret := &sharedapi.ErrorResponse{} From 7545f1ba248ba7c1ec1c40289891bb38589d00d0 Mon Sep 17 00:00:00 2001 From: Maxence Maireaux Date: Thu, 11 Jun 2026 12:43:00 +0200 Subject: [PATCH 2/2] fix(api): return 413 for oversized request bodies instead of 500 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address review feedback: http.MaxBytesReader surfaced its overflow error through the audit middleware's io.ReadAll, which maps any non-EOF read error to 500 — so a >1 MiB body returned "500 failed to read request body" instead of 413. Replace it with a limitRequestBody middleware that reads the body bounded to maxRequestBodyBytes+1, rejects oversize with 413 (REQUEST_TOO_LARGE), and resets the (bounded) body for the audit middleware and handlers. Adds TestRequestBodyTooLarge (>1 MiB → 413). --- pkg/api/router.go | 41 ++++++++++++++++++++++++++++++++++++++++- pkg/api/utils_test.go | 21 +++++++++++++++++++++ 2 files changed, 61 insertions(+), 1 deletion(-) diff --git a/pkg/api/router.go b/pkg/api/router.go index d9092c9..c6486a1 100644 --- a/pkg/api/router.go +++ b/pkg/api/router.go @@ -1,6 +1,9 @@ package api import ( + "bytes" + "encoding/json" + "io" "net/http" "github.com/ThreeDotsLabs/watermill/message" @@ -21,6 +24,42 @@ import ( // audit middleware, which buffers the whole body) from memory-exhaustion DoS. const maxRequestBodyBytes = 1 << 20 // 1 MiB +// limitRequestBody reads the request body up to maxRequestBodyBytes and rejects +// anything larger with 413. It buffers the (bounded) body and resets r.Body so +// the audit middleware and the handlers can still read it. +// +// We do this instead of http.MaxBytesReader because the audit middleware reads +// the body with io.ReadAll *before* the handler and turns any non-EOF error +// (including MaxBytesReader's overflow error) into a 500 — so an oversized body +// would surface as "500 failed to read request body" instead of 413. +func limitRequestBody(handler http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Body != nil && r.Body != http.NoBody { + buf, err := io.ReadAll(io.LimitReader(r.Body, maxRequestBodyBytes+1)) + _ = r.Body.Close() + if err != nil { + w.WriteHeader(http.StatusBadRequest) + _ = json.NewEncoder(w).Encode(sharedapi.ErrorResponse{ + ErrorCode: ErrorCodeValidation, + ErrorMessage: "failed to read request body", + }) + return + } + if int64(len(buf)) > maxRequestBodyBytes { + w.WriteHeader(http.StatusRequestEntityTooLarge) + _ = json.NewEncoder(w).Encode(sharedapi.ErrorResponse{ + ErrorCode: "REQUEST_TOO_LARGE", + ErrorMessage: "request body too large", + }) + return + } + r.Body = io.NopCloser(bytes.NewReader(buf)) + r.ContentLength = int64(len(buf)) + } + handler.ServeHTTP(w, r) + }) +} + func NewRouter( manager *wallet.Manager, healthController *sharedhealth.HealthController, @@ -33,11 +72,11 @@ func NewRouter( r.Use(middleware.Recoverer) r.Use(func(handler http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - r.Body = http.MaxBytesReader(w, r.Body, maxRequestBodyBytes) w.Header().Set("Content-Type", "application/json") handler.ServeHTTP(w, r) }) }) + r.Use(limitRequestBody) r.Use(httpaudit.Middleware(publisher, "audit-events", "wallets", nil)) r.Get("/_healthcheck", healthController.Check) diff --git a/pkg/api/utils_test.go b/pkg/api/utils_test.go index 155201b..5c5f318 100644 --- a/pkg/api/utils_test.go +++ b/pkg/api/utils_test.go @@ -71,6 +71,27 @@ func TestListHandlerRejectsInvalidPageSize(t *testing.T) { require.Equal(t, ErrorCodeValidation, readErrorResponse(t, rec).ErrorCode) } +func TestRequestBodyTooLarge(t *testing.T) { + t.Parallel() + + testEnv := newTestEnv( + WithAddMetadataToAccount(func(ctx context.Context, ledger, account, ik string, metadata map[string]string) error { + return nil + }), + ) + + // A body larger than maxRequestBodyBytes must be rejected with 413 before + // the audit middleware reads it — not surface as a 500. + body := bytes.NewReader(make([]byte, (1<<20)+1024)) + req := httptest.NewRequest(http.MethodPost, "/wallets", body) + req.Header.Set("Content-Type", "application/json") + rec := httptest.NewRecorder() + testEnv.Router().ServeHTTP(rec, req) + + require.Equal(t, http.StatusRequestEntityTooLarge, rec.Result().StatusCode) + require.Equal(t, "REQUEST_TOO_LARGE", readErrorResponse(t, rec).ErrorCode) +} + func readErrorResponse(t *testing.T, rec *httptest.ResponseRecorder) *sharedapi.ErrorResponse { t.Helper() ret := &sharedapi.ErrorResponse{}