diff --git a/config.example.yaml b/config.example.yaml index 3ff3f525..5dd88655 100644 --- a/config.example.yaml +++ b/config.example.yaml @@ -29,6 +29,7 @@ server: ready: /ready # Custom readiness endpoint path gzip: minlength: 1024 # Min response size (bytes) before gzip applies; smaller responses skip compression + bodylimit: 10485760 # Max request body size in bytes (default 10 MB); raise for large uploads. 0 = default; negative rejected. responsetime: enabled: false # Opt-in X-Response-Time header (per-request processing time); off by default — OTel provides richer latency telemetry. SERVER_RESPONSETIME_ENABLED=true to restore. diff --git a/config/config.go b/config/config.go index 37d3d34c..97caa7c4 100644 --- a/config/config.go +++ b/config/config.go @@ -331,6 +331,7 @@ func loadDefaults(k *koanf.Koanf) error { "server.path.health": "/health", "server.path.ready": "/ready", "server.gzip.minlength": 1024, + "server.bodylimit": DefaultBodyLimitBytes, // Database defaults not provided for deterministic behavior // Database will only be enabled when explicitly configured diff --git a/config/config_test.go b/config/config_test.go index ebf33deb..79b2a606 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -51,6 +51,7 @@ func TestLoadWithDefaults(t *testing.T) { assert.Equal(t, 5*time.Second, cfg.Server.Timeout.Middleware) assert.Equal(t, 10*time.Second, cfg.Server.Timeout.Shutdown) assert.Equal(t, 1024, cfg.Server.Gzip.MinLength) + assert.Equal(t, int64(10*1024*1024), cfg.Server.BodyLimit) assert.False(t, cfg.Server.ResponseTime.Enabled, "X-Response-Time header must default to opt-out") // Database should be disabled by default (no defaults provided) @@ -363,6 +364,7 @@ func TestLoadDefaultsInternalFunction(t *testing.T) { assert.Equal(t, "60s", k.String("server.timeout.idle")) assert.Equal(t, "5s", k.String("server.timeout.middleware")) assert.Equal(t, "10s", k.String("server.timeout.shutdown")) + assert.Equal(t, int64(10*1024*1024), k.Int64("server.bodylimit")) // Database defaults should NOT be provided assert.Equal(t, "", k.String("database.type")) @@ -786,7 +788,7 @@ func clearEnvironmentVariables() { "SERVER_HOST", "SERVER_PORT", "SERVER_TIMEOUT_READ", "SERVER_TIMEOUT_WRITE", "SERVER_TIMEOUT_IDLE", "SERVER_TIMEOUT_MIDDLEWARE", "SERVER_TIMEOUT_SHUTDOWN", "SERVER_PATH_BASE", "SERVER_PATH_HEALTH", "SERVER_PATH_READY", "SERVER_GZIP_MINLENGTH", - "SERVER_RESPONSETIME_ENABLED", + "SERVER_BODYLIMIT", "SERVER_RESPONSETIME_ENABLED", "DATABASE_TYPE", "DATABASE_HOST", "DATABASE_PORT", testDatabaseDatabase, testDatabaseUsername, "DATABASE_PASSWORD", "DATABASE_TLS_MODE", testDatabaseMaxConns, "DATABASE_POOL_IDLE_CONNECTIONS", diff --git a/config/types.go b/config/types.go index ca43bd73..ce84c33a 100644 --- a/config/types.go +++ b/config/types.go @@ -94,6 +94,11 @@ type ServerConfig struct { Path PathConfig `koanf:"path" json:"path" yaml:"path" toml:"path" mapstructure:"path"` Gzip GzipConfig `koanf:"gzip" json:"gzip" yaml:"gzip" toml:"gzip" mapstructure:"gzip"` + // BodyLimit is the maximum request body size in bytes. A value of 0 resolves + // to the framework default (10 MB) at wire-up; a negative value is rejected by + // config validation. + BodyLimit int64 `koanf:"bodylimit" json:"bodylimit" yaml:"bodylimit" toml:"bodylimit" mapstructure:"bodylimit"` + ResponseTime ResponseTimeConfig `koanf:"responsetime" json:"responsetime" yaml:"responsetime" toml:"responsetime" mapstructure:"responsetime"` // LogRoutes toggles the per-route "Route registered" startup log lines diff --git a/config/validation.go b/config/validation.go index 62922136..f84d9224 100644 --- a/config/validation.go +++ b/config/validation.go @@ -36,6 +36,12 @@ const ( // Connection layers compare against this constant to decide whether to // apply per-connection timezone setup. TimezoneDisabledSentinel = "-" + + // DefaultBodyLimitBytes is the maximum request body size (10 MB) applied when + // server.bodylimit is unset or resolves to a non-positive value. Single source + // of truth for both the koanf default (loadDefaults) and the server-side + // wire-up fallback (server.SetupMiddlewares). + DefaultBodyLimitBytes int64 = 10 * 1024 * 1024 ) // Messaging reconnection defaults @@ -305,6 +311,13 @@ func validateServer(cfg *ServerConfig) error { return NewValidationError("server.gzip.minlength", errMustBeNonNegative) } + // A negative body limit is an operator error (a typo silently reverting to the + // default is worse than a startup failure). Zero is permitted and resolves to + // the framework default at wire-up (see server.SetupMiddlewares). + if cfg.BodyLimit < 0 { + return NewValidationError("server.bodylimit", errMustBeNonNegative) + } + return nil } diff --git a/config/validation_test.go b/config/validation_test.go index c091a62e..bf88afcf 100644 --- a/config/validation_test.go +++ b/config/validation_test.go @@ -447,6 +447,20 @@ func TestValidateServerFailures(t *testing.T) { }, expectedError: "server.gzip.minlength", }, + { + name: "negative_bodylimit", + cfg: ServerConfig{ + Port: 8080, + Timeout: TimeoutConfig{ + Read: 15 * time.Second, + Write: 30 * time.Second, + Middleware: 5 * time.Second, + Shutdown: 10 * time.Second, + }, + BodyLimit: -1, + }, + expectedError: "server.bodylimit", + }, } for _, tt := range tests { diff --git a/go.mod b/go.mod index 80e0d68f..f4120dfb 100644 --- a/go.mod +++ b/go.mod @@ -21,8 +21,8 @@ require ( github.com/knadh/koanf/providers/file v1.2.1 github.com/knadh/koanf/providers/rawbytes v1.0.0 github.com/knadh/koanf/v2 v2.3.5 - github.com/labstack/echo-opentelemetry v0.0.2 - github.com/labstack/echo/v5 v5.2.1 + github.com/labstack/echo-opentelemetry v0.0.3 + github.com/labstack/echo/v5 v5.3.0 github.com/rabbitmq/amqp091-go v1.12.0 github.com/redis/go-redis/v9 v9.21.0 github.com/rs/zerolog v1.35.1 diff --git a/go.sum b/go.sum index dfe882f7..42d15d8f 100644 --- a/go.sum +++ b/go.sum @@ -116,10 +116,10 @@ github.com/kr/pretty v0.3.1 h1:flRD4NNwYAUpkphVc1HcthR4KEIFJ65n8Mw5qdRn3LE= github.com/kr/pretty v0.3.1/go.mod h1:hoEshYVHaxMs3cyo3Yncou5ZscifuDolrwPKZanG3xk= github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY= github.com/kr/text v0.2.0/go.mod h1:eLer722TekiGuMkidMxC/pM04lWEeraHUUmBw8l2grE= -github.com/labstack/echo-opentelemetry v0.0.2 h1:zNzIDYf2uXSYgpBXcuwRELrnOOFSRvMaC41DTF80ke8= -github.com/labstack/echo-opentelemetry v0.0.2/go.mod h1:kBwoqFuXPxpM9fxbs++asMsI42uOufQjuYJut3qqg6w= -github.com/labstack/echo/v5 v5.2.1 h1:TzpIksY6zLMzV0T0ycYbvTEoj9w6o6AcL5twg182VTY= -github.com/labstack/echo/v5 v5.2.1/go.mod h1:SyvlSdObGjRXeQfCCXW/sybkZdOOQZBmpKF0bvALaeo= +github.com/labstack/echo-opentelemetry v0.0.3 h1:r4HAlV3PniSlUmsqpXQ+YwFxo9RNuEOm2bGU9hAOhUY= +github.com/labstack/echo-opentelemetry v0.0.3/go.mod h1:gGPMgJmQkDreqWyEzfPccPhaT8B3XLag2AByizhNoM4= +github.com/labstack/echo/v5 v5.3.0 h1:KT74Mprk053PQEHwSZdeCDIz1BigTZOZhavMD0c9Fjs= +github.com/labstack/echo/v5 v5.3.0/go.mod h1:Q3j2+clBRgJr0O3DDONQeXNsM7RHgSwUhcuo47unqm8= github.com/lann/builder v0.0.0-20180802200727-47ae307949d0 h1:SOEGU9fKiNWd/HOJuq6+3iTQz8KNCLtVX6idSoTLdUw= github.com/lann/builder v0.0.0-20180802200727-47ae307949d0/go.mod h1:dXGbAdH5GtBTC4WfIxhKZfyBF/HBFgRZSWwZ9g/He9o= github.com/lann/ps v0.0.0-20150810152359-62de8c46ede0 h1:P6pPBnrTSX3DEVR4fDembhRWSsG5rVo6hYhAB/ADZrk= @@ -218,8 +218,8 @@ go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.63.0 h1:RbKq8BG go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.63.0/go.mod h1:h06DGIukJOevXaj/xrNjhi/2098RZzcLTbc0jDAUbsg= go.opentelemetry.io/contrib/instrumentation/runtime v0.69.0 h1:MtkMsuRo3zEXTTMALfyrszwCDZTkB6wolyPjbwFAdq0= go.opentelemetry.io/contrib/instrumentation/runtime v0.69.0/go.mod h1:FYTxnpsm+UPD0erZNq20GvnM8T2YQHiHtT2vokdpoac= -go.opentelemetry.io/contrib/propagators/b3 v1.39.0 h1:PI7pt9pkSnimWcp5sQhUA9OzLbc3Ba4sL+VEUTNsxrk= -go.opentelemetry.io/contrib/propagators/b3 v1.39.0/go.mod h1:5gV/EzPnfYIwjzj+6y8tbGW2PKWhcsz5e/7twptRVQY= +go.opentelemetry.io/contrib/propagators/b3 v1.42.0 h1:B2Pew5ufEtgkjLF+tSkXjgYZXQr9m7aCm1wLKB0URbU= +go.opentelemetry.io/contrib/propagators/b3 v1.42.0/go.mod h1:iPgUcSEF5DORW6+yNbdw/YevUy+QqJ508ncjhrRSCjc= go.opentelemetry.io/otel v1.44.0 h1:JjwHmHpA4iZ3wBxluu2fbbE7j4kqlE8jXyAyPXH7HqU= go.opentelemetry.io/otel v1.44.0/go.mod h1:BMgjTHL9WPRlRjL2oZCBTL4whCGtXch2H4BhOPIAyYc= go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploggrpc v0.20.0 h1:rydZ9sxbcFdm/oWrVyfLTjHIygMgv0bEeMd+3B/BvoM= diff --git a/renovate.json b/renovate.json index 278a8e9e..34d927e5 100644 --- a/renovate.json +++ b/renovate.json @@ -4,6 +4,17 @@ "config:recommended" ], "postUpdateOptions": ["gomodTidy"], + "packageRules": [ + { + "description": "Keep the echo/v5 engine and its OpenTelemetry companion on one PR so the engine and its instrumentation always update together.", + "matchDatasources": ["go"], + "matchPackageNames": [ + "github.com/labstack/echo/v5", + "github.com/labstack/echo-opentelemetry" + ], + "groupName": "echo (engine + otel)" + } + ], "customManagers": [ { "customType": "regex", diff --git a/server/constants.go b/server/constants.go index 638ab547..4503f021 100644 --- a/server/constants.go +++ b/server/constants.go @@ -11,6 +11,7 @@ const ( errCodeConflict = "CONFLICT" errCodeTooManyRequests = "TOO_MANY_REQUESTS" errCodeServiceUnavailable = "SERVICE_UNAVAILABLE" + errCodeMethodNotAllowed = "METHOD_NOT_ALLOWED" errCodeInternalError = "INTERNAL_ERROR" ) diff --git a/server/handler.go b/server/handler.go index 0362437f..c9a1e353 100644 --- a/server/handler.go +++ b/server/handler.go @@ -206,9 +206,29 @@ type PathParam struct { // including any group/base-path prefix (e.g. "/api/cards/:cardId/status"). // It is the template the application registered, NOT the concrete URL // (use Request().URL.Path for that). Empty before routing completes and on -// unmatched (404) requests; on 405 the engine sets the best-matching route's -// template (engine-defined, not a contract). -func (c HandlerContext) RouteTemplate() string { return c.ectx.Path() } +// unmatched (404) requests; on a top-level 405 the engine sets the best-matching +// route's template (engine-defined, not a contract). Note the asymmetry under a +// middleware-bearing group: a wrong-method or unmatched sub-path there resolves +// to the group's implicit catch-all (a 404), so it returns "" rather than a +// best-match template. +func (c HandlerContext) RouteTemplate() string { + if c.ectx.RouteInfo().Method == echo.RouteNotFound { + return "" // group implicit catch-all (echo v5.3.0): unmatched, no template + } + return c.ectx.Path() +} + +// isUnmatchedRoute reports whether the request did not resolve to a real +// application route: the global 404/405 sentinel fallbacks OR a group's +// implicit catch-all. echo v5.3.0 restored v4 per-group auto-404 routes for +// middleware-bearing groups; their RouteInfo has an empty Name but +// Method == echo.RouteNotFound, which the old Name-only check missed. +func (c HandlerContext) isUnmatchedRoute() bool { + ri := c.ectx.RouteInfo() + return ri.Name == echo.NotFoundRouteName || + ri.Name == echo.MethodNotAllowedRouteName || + ri.Method == echo.RouteNotFound +} // PathParams returns the matched path parameters in route-template order. // The returned slice is a defensive copy: safe to retain past the request; @@ -216,10 +236,13 @@ func (c HandlerContext) RouteTemplate() string { return c.ectx.Path() } // route matched (pre-route, 404, 405) — parameter state is only meaningful // for a matched route. func (c HandlerContext) PathParams() []PathParam { - // Param names are stamped only on a full method+path match; on 404/405 the - // pooled context's value slots can pair with a PREVIOUS request's names - // (phantom params), so treat unmatched requests as having no parameters. - if name := c.ectx.RouteInfo().Name; name == echo.NotFoundRouteName || name == echo.MethodNotAllowedRouteName { + // Param names are stamped only on a real method+path match. On an unmatched + // request — the global 404/405 fallback, or a middleware-bearing group's + // implicit catch-all — echo may still leave values in the pooled slots: stale + // names from a prior pooled request, or the catch-all's own synthetic wildcard + // ("*") capture. Neither is a real application parameter, so treat unmatched + // requests as having none. + if c.isUnmatchedRoute() { return []PathParam{} } // echo's PathValues() returns a slice header ALIASING the pooled context's diff --git a/server/handler_test.go b/server/handler_test.go index 346862f0..fbbeda5f 100644 --- a/server/handler_test.go +++ b/server/handler_test.go @@ -2620,3 +2620,185 @@ func TestTypedHandlerPathAllocsStable(t *testing.T) { assert.LessOrEqual(t, got, float64(typedHandlerPathMaxAllocs), "typed request path allocs/op regressed beyond the ADR-026 baseline") } + +// TestHandlerContextGroupCatchAllUnmatched verifies the param-state guard treats a +// middleware-bearing group's implicit catch-all as unmatched. echo v5.3.0 restored the v4 +// behavior where a group carrying middleware auto-registers "/*" RouteNotFound routes (see +// group.go Group.Use) so the middleware still runs on unmatched sub-paths and method +// mismatches. Those catch-all hits have an empty RouteInfo.Name (NOT a sentinel) but +// Method == echo.RouteNotFound, so PathParams/RouteTemplate must key on Method: dropping +// that clause regresses the unmatched cases to a phantom "*" param and the "/api/*" template. +func TestHandlerContextGroupCatchAllUnmatched(t *testing.T) { + cfg := &config.Config{App: config.AppConfig{Env: "development"}} + obs := &unmatchedRouteObservation{} + + e := echo.New() + // Pass-through group middleware mirroring scheduler/module.go's sysGroup.Use(...): its + // presence is what makes echo register the implicit catch-all, and being group-level it + // runs (and captures) even when no real route matched. + capture := func(next echo.HandlerFunc) echo.HandlerFunc { + return func(ec *echo.Context) error { + c := newHandlerContext(ec, cfg) + obs.params = c.PathParams() + obs.template = c.RouteTemplate() + obs.captured = true + return next(ec) + } + } + g := e.Group("/api", capture) + g.GET("/users/:id", func(ec *echo.Context) error { return ec.NoContent(http.StatusOK) }) + // A nested middleware-bearing group registers its OWN implicit catch-all; the guard + // must treat its unmatched sub-paths identically (Method == echo.RouteNotFound). + sub := g.Group("/v1", capture) + sub.GET("/items/:id", func(ec *echo.Context) error { return ec.NoContent(http.StatusOK) }) + + // Warm the router and context pool with a matched parameterized request so stale param + // names sit in the pooled backing array — the phantom-param regression vector. + warm := httptest.NewRequestWithContext(context.Background(), http.MethodGet, "/api/users/7", http.NoBody) + e.ServeHTTP(httptest.NewRecorder(), warm) + + serve := func(method, target string) int { + *obs = unmatchedRouteObservation{} + req := httptest.NewRequestWithContext(context.Background(), method, target, http.NoBody) + rec := httptest.NewRecorder() + e.ServeHTTP(rec, req) + return rec.Code + } + + t.Run("unmatched_sub_path", func(t *testing.T) { + status := serve(http.MethodGet, "/api/nope/x") + require.True(t, obs.captured, "group middleware must run on the implicit catch-all") + assert.Equal(t, http.StatusNotFound, status) + assert.Empty(t, obs.params, "PathParams must be empty on a group catch-all") + assert.Empty(t, obs.template, "RouteTemplate must be empty on a group catch-all") + }) + + t.Run("wrong_method", func(t *testing.T) { + // echo v5.3.0's group catch-all shadows the 405: a wrong-method request under a + // middleware-bearing group returns 404 (not 405). This is the headline E51/C51.1 change. + status := serve(http.MethodPost, "/api/users/42") + require.True(t, obs.captured, "group middleware must run on the implicit catch-all") + assert.Equal(t, http.StatusNotFound, status, "wrong-method under a group is shadowed to 404 by the catch-all") + assert.Empty(t, obs.params, "PathParams must be empty when no route method matched") + assert.Empty(t, obs.template, "RouteTemplate must be empty on a group catch-all") + }) + + t.Run("nested_group_unmatched", func(t *testing.T) { + status := serve(http.MethodGet, "/api/v1/nope") + require.True(t, obs.captured, "nested group middleware must run on its implicit catch-all") + assert.Equal(t, http.StatusNotFound, status) + assert.Empty(t, obs.params, "PathParams must be empty on a nested group catch-all") + assert.Empty(t, obs.template, "RouteTemplate must be empty on a nested group catch-all") + }) + + t.Run("real_match_unaffected", func(t *testing.T) { + status := serve(http.MethodGet, "/api/users/42") + require.True(t, obs.captured, "group middleware must run on a real match") + assert.Equal(t, http.StatusOK, status) + assert.Equal(t, []PathParam{{Name: "id", Value: "42"}}, obs.params, + "the guard must not strip params from a real match") + assert.Equal(t, "/api/users/:id", obs.template) + }) + + t.Run("nested_real_match_unaffected", func(t *testing.T) { + status := serve(http.MethodGet, "/api/v1/items/9") + require.True(t, obs.captured, "nested group middleware must run on a real match") + assert.Equal(t, http.StatusOK, status) + assert.Equal(t, []PathParam{{Name: "id", Value: "9"}}, obs.params) + assert.Equal(t, "/api/v1/items/:id", obs.template) + }) +} + +// TestHandlerContextGlobalUnmatchedTemplate pins the deliberate asymmetry between the +// PathParams() guard (full isUnmatchedRoute — empty on 404 AND 405) and RouteTemplate()'s +// NARROW guard (empty only for a group catch-all): on a top-level 405 RouteTemplate() must +// still report the engine's best-match template, while PathParams() is empty. +func TestHandlerContextGlobalUnmatchedTemplate(t *testing.T) { + cfg := &config.Config{App: config.AppConfig{Env: "development"}} + obs := &unmatchedRouteObservation{} + + e := echo.New() + // Global middleware runs on every request incl. the global 404/405 fallbacks. + e.Use(func(next echo.HandlerFunc) echo.HandlerFunc { + return func(ec *echo.Context) error { + c := newHandlerContext(ec, cfg) + obs.params = c.PathParams() + obs.template = c.RouteTemplate() + obs.captured = true + return next(ec) + } + }) + e.GET("/widget/:id", func(ec *echo.Context) error { return ec.NoContent(http.StatusOK) }) + + serve := func(method, target string) int { + *obs = unmatchedRouteObservation{} + req := httptest.NewRequestWithContext(context.Background(), method, target, http.NoBody) + rec := httptest.NewRecorder() + e.ServeHTTP(rec, req) + return rec.Code + } + + t.Run("global_404", func(t *testing.T) { + status := serve(http.MethodGet, "/nothing/here") + require.True(t, obs.captured) + assert.Equal(t, http.StatusNotFound, status) + assert.Empty(t, obs.params) + assert.Empty(t, obs.template, "global 404 has no matched template") + }) + + t.Run("global_405_keeps_best_match_template", func(t *testing.T) { + status := serve(http.MethodPost, "/widget/5") + require.True(t, obs.captured) + assert.Equal(t, http.StatusMethodNotAllowed, status, "a top-level wrong-method is a real 405") + assert.Empty(t, obs.params, "PathParams is empty on a 405") + assert.Equal(t, "/widget/:id", obs.template, + "RouteTemplate's narrow guard must preserve the engine best-match template on a global 405") + }) +} + +type trailingBytesReq struct { + Name string `json:"name"` +} + +// TestBindJSONBodyTrailingContent pins echo v5.3.0's stricter JSON deserialize (the default +// serializer decodes with json.Unmarshal instead of a streaming json.Decoder) from both +// sides of the tolerance boundary: trailing NON-whitespace garbage is now rejected with a +// 400 bind error (v5.2.1 silently ignored it), while trailing whitespace — a newline from a +// CLI/`curl`, say — is still accepted because json.Unmarshal ignores it. +func TestBindJSONBodyTrailingContent(t *testing.T) { + e := echo.New() + v := NewValidator() + require.NotNil(t, v) + e.Validator = v + + binder := NewRequestBinder() + cfg := &config.Config{App: config.AppConfig{Env: "development"}} + handler := func(req trailingBytesReq, _ HandlerContext) (helloResp, IAPIError) { + return helloResp{Message: req.Name}, nil + } + h := WrapHandler(handler, binder, cfg) + + bind := func(body string) *httptest.ResponseRecorder { + req := httptest.NewRequestWithContext(context.Background(), http.MethodPost, "/test", strings.NewReader(body)) + req.Header.Set(echo.HeaderContentType, echo.MIMEApplicationJSON) + rec := httptest.NewRecorder() + require.NoError(t, h(e.NewContext(req, rec))) + return rec + } + + t.Run("rejects_trailing_garbage", func(t *testing.T) { + rec := bind(`{"name":"x"}garbage`) + assert.Equal(t, http.StatusBadRequest, rec.Code) + + var resp APIResponse + require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &resp)) + require.NotNil(t, resp.Error) + assert.Equal(t, "BAD_REQUEST", resp.Error.Code) + assert.Equal(t, "Invalid request data", resp.Error.Message) + }) + + t.Run("accepts_trailing_whitespace", func(t *testing.T) { + rec := bind("{\"name\":\"x\"}\n") + assert.Equal(t, http.StatusOK, rec.Code, "a trailing newline is whitespace and must still bind") + }) +} diff --git a/server/middleware.go b/server/middleware.go index d144a8fe..30d7162e 100644 --- a/server/middleware.go +++ b/server/middleware.go @@ -133,8 +133,15 @@ func SetupMiddlewares(e *echo.Echo, log logger.Logger, cfg *config.Config, obser // This prevents goroutine panics when the context is canceled mid-flight. e.Use(timeoutEcho(cfg.Server.Timeout.Middleware)) - // Body limit - e.Use(middleware.BodyLimit(10 * 1024 * 1024)) // 10 MB + // Body limit — configurable via server.bodylimit (bytes). Config validation + // rejects a negative value; this <=0 fallback is defense-in-depth for callers + // that construct the server directly (bypassing Validate) and for an explicit + // 0, so the limit can never be silently disabled. + bodyLimit := cfg.Server.BodyLimit + if bodyLimit <= 0 { + bodyLimit = config.DefaultBodyLimitBytes + } + e.Use(middleware.BodyLimit(bodyLimit)) // Gzip — skip compressing tiny responses (the gzip header/overhead can exceed // the savings for small JSON); threshold is configurable via server.gzip.minlength. diff --git a/server/middleware_test.go b/server/middleware_test.go index 046c70ab..0dc703bc 100644 --- a/server/middleware_test.go +++ b/server/middleware_test.go @@ -452,6 +452,58 @@ func TestMiddlewareBodyLimit(t *testing.T) { }) } +func TestMiddlewareBodyLimitFromConfig(t *testing.T) { + log := logger.New("disabled", false) + + newEngine := func(limit int64) *echo.Echo { + e := echo.New() + cfg := &config.Config{ + App: config.AppConfig{Rate: config.RateConfig{Limit: 100}}, + Server: config.ServerConfig{ + Timeout: config.TimeoutConfig{Middleware: 30 * time.Second}, + BodyLimit: limit, + }, + } + SetupMiddlewares(e, log, cfg, true, testHealthPath, testReadyPath) + e.POST("/test", func(c *echo.Context) error { + return c.JSON(http.StatusOK, map[string]string{"status": "ok"}) + }) + return e + } + + post := func(e *echo.Echo, size int) int { + body := strings.NewReader(strings.Repeat("x", size)) + req := httptest.NewRequestWithContext(context.Background(), http.MethodPost, "/test", body) + req.Header.Set("Content-Type", "application/json") + rec := httptest.NewRecorder() + e.ServeHTTP(rec, req) + return rec.Code + } + + t.Run("configured_limit_is_enforced", func(t *testing.T) { + e := newEngine(1024) // 1 KB + assert.Equal(t, http.StatusOK, post(e, 512)) + assert.Equal(t, http.StatusRequestEntityTooLarge, post(e, 2048)) + }) + + t.Run("non_positive_limit_falls_back_to_default", func(t *testing.T) { + // Both 0 and a negative limit must resolve to the shared 10 MB default rather than + // disabling the cap. Pin the boundary tightly against DefaultBodyLimitBytes: a body + // just under it passes, one just over it is rejected. (A negative reaches the <=0 + // guard only for direct SetupMiddlewares callers; config.Validate rejects it on the + // Load path — see config/validation.go.) + underDefault := int(config.DefaultBodyLimitBytes) - 1024 + overDefault := int(config.DefaultBodyLimitBytes) + 1024 + for _, limit := range []int64{0, -1} { + e := newEngine(limit) + assert.Equal(t, http.StatusOK, post(e, underDefault), + "limit %d must fall back to the 10 MB default (accepts an under-default body)", limit) + assert.Equal(t, http.StatusRequestEntityTooLarge, post(e, overDefault), + "limit %d must fall back to the 10 MB default (rejects an over-default body)", limit) + } + }) +} + func TestGzipMiddleware(t *testing.T) { e := echo.New() log := logger.New("disabled", false) diff --git a/server/server.go b/server/server.go index a2a602a6..671fa901 100644 --- a/server/server.go +++ b/server/server.go @@ -351,6 +351,8 @@ func statusToErrorCode(status int) string { return errCodeForbidden case http.StatusNotFound: return errCodeNotFound + case http.StatusMethodNotAllowed: + return errCodeMethodNotAllowed case http.StatusConflict: return errCodeConflict case http.StatusTooManyRequests: diff --git a/server/server_test.go b/server/server_test.go index 3c31ee7f..9d168f74 100644 --- a/server/server_test.go +++ b/server/server_test.go @@ -370,6 +370,7 @@ func TestStatusToErrorCodeMappings(t *testing.T) { {status: http.StatusUnauthorized, code: "UNAUTHORIZED"}, {status: http.StatusForbidden, code: "FORBIDDEN"}, {status: http.StatusNotFound, code: "NOT_FOUND"}, + {status: http.StatusMethodNotAllowed, code: "METHOD_NOT_ALLOWED"}, {status: http.StatusConflict, code: "CONFLICT"}, {status: http.StatusTooManyRequests, code: "TOO_MANY_REQUESTS"}, {status: http.StatusServiceUnavailable, code: "SERVICE_UNAVAILABLE"}, @@ -381,6 +382,27 @@ func TestStatusToErrorCodeMappings(t *testing.T) { } } +// TestServer405ProducesMethodNotAllowedEnvelope proves the wiring end-to-end: a real 405 +// flowing through the framework error handler carries error code METHOD_NOT_ALLOWED in the +// response envelope (before this mapping existed it fell through to INTERNAL_ERROR). The +// route is top-level (no group middleware), so a wrong method yields a genuine 405 rather +// than a group catch-all 404. +func TestServer405ProducesMethodNotAllowedEnvelope(t *testing.T) { + srv := newTestServer("", "", "") + srv.echo.GET("/widget/:id", func(c *echo.Context) error { return c.NoContent(http.StatusOK) }) + + req := httptest.NewRequestWithContext(context.Background(), http.MethodPost, "/widget/5", http.NoBody) + rec := httptest.NewRecorder() + srv.echo.ServeHTTP(rec, req) + + require.Equal(t, http.StatusMethodNotAllowed, rec.Code) + var resp APIResponse + require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &resp)) + require.NotNil(t, resp.Error) + assert.Equal(t, "METHOD_NOT_ALLOWED", resp.Error.Code, + "a real 405 must carry the METHOD_NOT_ALLOWED envelope code, not INTERNAL_ERROR") +} + func TestServerConfiguration(t *testing.T) { tests := []struct { name string diff --git a/wiki/migrations.md b/wiki/migrations.md index 56990259..1e2117d4 100644 --- a/wiki/migrations.md +++ b/wiki/migrations.md @@ -17,7 +17,7 @@ A plain `vX.Y.Z` is your current node. `=>` a local path (dev `replace`) means t **3 — Select the hop chain** on the Ladder: every edge strictly to the right of CURRENT, up to and including TARGET. Never apply an edge at/left of CURRENT. ``` -v0.39.1 ─E40─ v0.40.0 ─E401─ v0.40.1 ─E41─ v0.41.0 ─E42─ v0.42.0 ─E43─ v0.43.0 ─E44─ v0.44.0 ─E45─ v0.45.0 ─E49─ v0.49.0 ─E50─ v0.50.0 +v0.39.1 ─E40─ v0.40.0 ─E401─ v0.40.1 ─E41─ v0.41.0 ─E42─ v0.42.0 ─E43─ v0.43.0 ─E44─ v0.44.0 ─E45─ v0.45.0 ─E49─ v0.49.0 ─E50─ v0.50.0 ─E51─ v0.51.0 ``` > v0.46.0–v0.48.0 shipped additive-only changes (route template/path-param accessors, raw-route descriptors, module-contributed global middleware — adopt-only, no migration atoms), so E49 is the next hop after v0.45.0 and applies when crossing from any of v0.45.0–v0.48.0 to v0.49.0. @@ -33,6 +33,7 @@ v0.39.1 ─E40─ v0.40.0 ─E401─ v0.40.1 ─E41─ v0.41.0 ─E42─ v0.42.0 | E45 | v0.44.0 → v0.45.0 | compile-break | 9 | C45.1 C45.2 C45.3 C45.4 C45.5 C45.6 | outbox re-delivery count | | E49 | v0.45.0 → v0.49.0 | silent-config | 6 | none | multi-tenant outbox timeout guards / stale `messaging.*` + `database.manager.*` values / reconnect delay keys go live / mode-aware cache pool / unit-less duration guard | | E50 | v0.49.0 → v0.50.0 | config-break | 4 | none | Flyway migrate surfaces unparseable/failure output as an error; non-empty DB passwords < 8 bytes rejected at config validation + migrate; dev CORS wildcard opt-in; `multitenant.resolver.order` now REQUIRED for `type: composite` (no default — composite deployments fail to start until they declare one) | +| E51 | v0.50.0 → v0.51.0 | silent-behavior (adopt-only) | 3 | none | none | **4 — Read each atom's gate before acting.** Every atom carries `when: match | no-match | always`: - **`when: match`** → act only if `detect` returns ≥1 line (an API/arity/interface change, or a config key you set). @@ -608,6 +609,37 @@ v0.39.1 ─E40─ v0.40.0 ─E401─ v0.40.1 ─E41─ v0.41.0 ─E42─ v0.42.0 - verify: `make run` — a composite config with no order aborts naming `multitenant.resolver.order` (`required when multitenant.resolver.type is 'composite' — no implicit default`); once set, startup succeeds. Then send a request carrying both a valid subdomain/path tenant and a conflicting `X-Tenant-ID`: the resolved tenant is whichever source you put first. `go test ./config/ ./server/ ./multitenant/` - ref: ADR-039 · config/validation.go: validateResolverOrder · server/middleware.go: compositeSubResolvers · config/types.go: DefaultResolverOrder +## E51 · v0.50.0 → v0.51.0 — echo/v5 v5.3.0 (group implicit-404 revert + stricter JSON bind + configurable body limit) + +- gist: The `github.com/labstack/echo/v5` bump v5.2.1 → v5.3.0 is behavior-affecting, not a pure version bump — adopt-only for consumers (no code migration required, no exported go-bricks signature changes). Three observable shifts: (1) echo restored v4's behavior where a middleware-bearing group auto-registers an implicit `/*` catch-all, so group middleware (the scheduler `/_sys` CIDR gate, the debug auth gate, any app sub-group with middleware) now ALSO runs on unmatched sub-paths and wrong-method requests under its prefix — a defense-in-depth win — and a wrong-method request under such a group returns 404 (no `Allow` header) instead of 405; go-bricks intentionally KEEPS echo's new default (does NOT set `NoGroupAutoRegister404Routes`) to preserve the gate-coverage win and hardens `HandlerContext.PathParams()`/`RouteTemplate()` to still report "unmatched" for the catch-all. (2) JSON binding is stricter — a request body with trailing NON-whitespace after the top-level JSON value (a second value or stray bytes) is now rejected (400) where v5.2.1 silently accepted it; trailing whitespace still binds (echo switched `Deserialize` from `json.Decoder` to `json.Unmarshal` + a pooled buffer, also a small per-bind allocation win). (3) A new `server.bodylimit` config (int64 bytes, default 10 MB) makes the request body cap configurable. +- build-caught: none +- preflight: none +- exit: `go get github.com/gaborage/go-bricks@v0.51.0 && go mod tidy && go build ./... && go test ./...` + +### [C51.1] Middleware-bearing groups auto-register an implicit `/*` catch-all (405 → 404 under a group; gate now covers unmatched sub-paths) · silent-behavior · when: match + +- detect: `git grep -nE 'StatusMethodNotAllowed|MethodNotAllowed|405|Allow\b|/_sys' -- '*_test.go'` then keep hits that assert a wrong-method response (or an `Allow` header) for a path UNDER a middleware-bearing group prefix +- gate: match = you have a test/client/monitor that expects 405 + an `Allow` header for a wrong-method request under a group prefix (e.g. `/_sys/*`, the debug group, or any app sub-group with middleware), OR you relied on that group's middleware NOT running for unmatched sub-paths. On echo v5.3.0 the group's implicit `/*` catch-all shadows echo's automatic 405 for the WHOLE prefix: both an unmatched sub-path AND a wrong-method request to an existing route under the group now return 404 (no `Allow`), with the group middleware (CIDR gate, auth gate) running first — so an unmatched sub-path under a gated prefix is now denied by the gate instead of falling through. Scope: this is limited to routes under a middleware-bearing group. no-match = TOP-LEVEL routes (not under such a group), real matched routes, and the global 404/405 fallbacks are unaffected — a wrong-method request to a top-level route still returns 405 + `Allow`. +- apply: none required — this is a security-positive default the framework keeps deliberately. Update any test/monitor that asserted 405 + `Allow` under a group prefix to expect 404, and confirm nothing depended on group middleware being skipped for unmatched sub-paths. +- verify: `go test ./...` # a wrong-method request under `/_sys/...` returns 404 through the CIDR gate; tests asserting 405/`Allow` under a middleware group now expect 404 +- ref: echo/v5 v5.3.0 · labstack/echo#530 · CHANGELOG 0.51.0 + +### [C51.2] JSON bind rejects trailing bytes after the top-level value · silent-behavior · when: match + +- detect: audit any client/producer that POSTs to this service and appends content after the JSON document (concatenated objects, a trailing newline-delimited record, stray bytes) — not reliably greppable in this repo +- gate: match = a caller sends a request body with extra NON-whitespace after the top-level JSON value (a second JSON value or stray bytes) — v5.2.1's `json.Decoder`-based bind silently accepted (and ignored) the trailing content; v5.3.0's `json.Unmarshal`-based bind rejects the whole body with 400. Trailing whitespace (a newline, spaces) is still accepted, and well-formed single-document bodies are unaffected and gain a small per-bind allocation win. no-match = your callers send exactly one JSON value per body, unaffected. +- apply: fix the offending client to send exactly one JSON value per request body; there is no opt-out. +- verify: `go test ./...` # then POST a body with trailing non-whitespace (e.g. a second JSON value) and confirm a 400 (previously 200) +- ref: echo/v5 v5.3.0 · CHANGELOG 0.51.0 + +### [C51.3] New `server.bodylimit` config caps request body size (default 10 MB) · silent-behavior · when: no-match + +- detect: `git grep -nEi '(^[[:space:]]*|\.)bodylimit[[:space:]]*:|SERVER_BODYLIMIT'` +- gate: no-match = you leave `server.bodylimit` unset, so the new default governs — the accepted request body is capped at 10 MB (10485760 bytes) and a larger body is rejected with 413 before the handler runs. match = you set an explicit **positive** byte count, which then governs (raises or lowers the cap); an explicit `0` resolves to the 10 MB default and a negative value is rejected at config validation. +- apply: leave unset for the 10 MB default OR set `server.bodylimit` (int64 bytes, env `SERVER_BODYLIMIT`) to a positive value to raise it for large-upload/bulk-import endpoints or lower it to tighten the boundary. +- verify: `make run` then POST a body larger than the configured cap # rejected with 413; a body under the cap is accepted +- ref: echo/v5 v5.3.0 · server config · CHANGELOG 0.51.0 + --- _The sections below are reference material: the two config-key rename lookup tables (linked from atoms C401.1 and C41.7), followed by pre-v0.39 changes retained for consumers upgrading from older releases._ diff --git a/wiki/observability.md b/wiki/observability.md index 213a3a04..e3ef20ee 100644 --- a/wiki/observability.md +++ b/wiki/observability.md @@ -10,6 +10,8 @@ GoBricks provides production-grade observability built on OpenTelemetry: distrib **Per-Subsystem Instrumented Tracers:** The framework ships three OTel tracers under matching scopes. `go-bricks/database` emits CLIENT-kind spans per query. `go-bricks/messaging` emits PRODUCER/CONSUMER spans per AMQP publish/consume. `go-bricks/httpclient` emits CLIENT-kind spans per outbound HTTP call — one parent "Do" span (the logical request rollup) and one child attempt span per retry attempt — and injects `traceparent` headers via the OTel propagator so downstream services join the trace. When `observability.enabled` is false no spans are emitted; the `database` tracer additionally short-circuits before building any span attributes (true zero overhead), while `messaging`/`httpclient` route into the global no-op provider (spans dropped, attribute construction not yet skipped — a tracked follow-up). See [httpclient.md#tracing](httpclient.md#tracing) for the span tree, attribute reference, and status-mapping rules. +**Group-Scoped 404 Route Labels (echo v5.3.0):** After the echo v5.3.0 upgrade, a 404 for an unmatched sub-path (or a wrong-method request) under a middleware-bearing group — e.g. the scheduler `/_sys` CIDR-gated group or the debug group — now resolves to that group's implicit `/*` catch-all, so its incoming-request span and metrics carry `http.route = "//*"` (span name ` //*` — the span keeps the request method, e.g. `GET //*`, or `POST //*` for a wrong-method request) instead of the previous empty-route / bare-`GET` bucket. This is a low-cardinality change (one new series per middleware-bearing group prefix); operators with dashboards or alerts keyed on `http.route` for those 404s should expect the new series and re-point any query that matched the old empty/`GET` bucket. + **Go Runtime Metrics:** Auto-exports memory, goroutines, CPU, scheduler latency, GC config when `observability.enabled: true`. Follows [OpenTelemetry semantic conventions](https://opentelemetry.io/docs/specs/semconv/runtime/go-metrics/) **Export Timeout Configuration:** GoBricks uses environment-aware export timeouts to balance fail-fast feedback (development) with network resilience (production): diff --git a/wiki/startup_defaults.md b/wiki/startup_defaults.md index d12a5b1a..fdabd45c 100644 --- a/wiki/startup_defaults.md +++ b/wiki/startup_defaults.md @@ -35,6 +35,21 @@ app: observability: 30s # More time for remote OTLP endpoints ``` +## Server Request Body Limit + +`server.bodylimit` (int64 bytes; env `SERVER_BODYLIMIT`) caps the accepted HTTP request body size, rejecting an over-cap request with `413 Request Entity Too Large`. A request with a known `Content-Length` above the cap is rejected up front, before the handler runs; a chunked / unknown-length body is bounded by a limited reader instead, so the 413 surfaces when the read crosses the cap while the handler consumes the body: + +| Setting | Default | Purpose | +|---------|---------|---------| +| `server.bodylimit` | 10 MB (10485760 bytes) | Maximum accepted HTTP request body size | + +Raise it for endpoints that accept large uploads or bulk imports, or lower it to tighten the boundary: + +```yaml +server: + bodylimit: 26214400 # 25 MB — allow larger uploads +``` + ## Messaging Pre-Warm Readiness Wait In single-tenant mode, startup pre-warms the messaging publisher and then waits for it to report `IsReady()`, bounded by `messaging.reconnect.readytimeout` (default 5s — the same key and budget as the per-publish readiness pre-flight; see [context_deadlines.md](context_deadlines.md)). A publisher that isn't ready in time logs a WARN and startup continues — the wait never fails startup; the publish-time pre-flight still absorbs a slow first publish. The wait (`ConnectionPreWarmer.awaitPublisherReady`) is context-aware and reports a distinct cancellation outcome when its `ctx` is canceled, rather than mislabeling it as a readiness timeout — but that path only fires for callers that pass a cancelable context. On the framework's own boot path (`app/lifecycle.go`'s `prepareRuntime`), pre-warm runs with `context.Background()` and the OS signal handler is installed later (`waitForShutdownOrServerError`, after `prepareRuntime` returns), so a shutdown signal received during pre-warm does **not** abort the wait — it runs to ready-or-`readytimeout` regardless.