From e10704e26dd4bf4fe5976aac77001205a252caea Mon Sep 17 00:00:00 2001 From: Tomas Srnka Date: Sat, 1 Aug 2026 14:41:48 +0200 Subject: [PATCH 1/3] fix(api): return the documented error body when a request is too large A request whose body exceeded the 16 MiB limit came back as request too large{"code":413,"message":"reading failed: HTTP request too large"} Two writers produced that: gin-contrib/size commits a plain-text "request too large" body itself, and the OpenAPI validator's error handler then appends the JSON envelope to the already-committed response. The result parses as neither text nor JSON, so a client reading {code, message} off the body gets undefined for both and can only report something like "[undefined] no message". Replace the size limiter with a local one built on http.MaxBytesReader that records the 413 status but writes nothing, leaving the error handler as the single writer of the response. Guard the error handler against appending to a response that something upstream already committed, so this class of corrupt body cannot come back through another middleware. --- packages/api/go.mod | 1 - packages/api/go.sum | 2 - packages/api/internal/middleware/bodylimit.go | 50 +++++++ packages/api/internal/utils/error.go | 8 + .../api/internal/utils/error_envelope_test.go | 138 ++++++++++++++++++ packages/api/main.go | 3 +- 6 files changed, 197 insertions(+), 5 deletions(-) create mode 100644 packages/api/internal/middleware/bodylimit.go create mode 100644 packages/api/internal/utils/error_envelope_test.go diff --git a/packages/api/go.mod b/packages/api/go.mod index 0b4eb1a35e..9c357d05b3 100644 --- a/packages/api/go.mod +++ b/packages/api/go.mod @@ -27,7 +27,6 @@ require ( github.com/flowchartsman/retry v1.2.0 github.com/getkin/kin-openapi v0.145.0 github.com/gin-contrib/cors v1.7.6 - github.com/gin-contrib/size v1.0.2 github.com/gin-gonic/gin v1.12.0 github.com/go-redis/redis_rate/v10 v10.0.1 github.com/gogo/status v1.1.1 diff --git a/packages/api/go.sum b/packages/api/go.sum index 0c6ee36ebf..c6c53637f9 100644 --- a/packages/api/go.sum +++ b/packages/api/go.sum @@ -316,8 +316,6 @@ github.com/getkin/kin-openapi v0.145.0 h1:htBX+Q7SevVaCUqymFegUKzH2WCbewl9tsmyn2 github.com/getkin/kin-openapi v0.145.0/go.mod h1:3BH9M9XDe/y9M5DSvEocVYAYq1w0qrhJHjC/vZi0AaY= github.com/gin-contrib/cors v1.7.6 h1:3gQ8GMzs1Ylpf70y8bMw4fVpycXIeX1ZemuSQIsnQQY= github.com/gin-contrib/cors v1.7.6/go.mod h1:Ulcl+xN4jel9t1Ry8vqph23a60FwH9xVLd+3ykmTjOk= -github.com/gin-contrib/size v1.0.2 h1:rW5bgj7+SwDmnOlZ9lJV8lDYWTrllQAspi3WP8GtqHE= -github.com/gin-contrib/size v1.0.2/go.mod h1:kKbz/Ervg8tpnrhrBpTO5m4wuQNTVlEqTb1MgkRrmFY= github.com/gin-contrib/sse v1.1.1 h1:uGYpNwTacv5R68bSGMapo62iLTRa9l5zxGCps4hK6ko= github.com/gin-contrib/sse v1.1.1/go.mod h1:QXzuVkA0YO7o/gun03UI1Q+FTI8ZV/n5t03kIQAI89s= github.com/gin-gonic/gin v1.12.0 h1:b3YAbrZtnf8N//yjKeU2+MQsh2mY5htkZidOM7O0wG8= diff --git a/packages/api/internal/middleware/bodylimit.go b/packages/api/internal/middleware/bodylimit.go new file mode 100644 index 0000000000..82ef9d514e --- /dev/null +++ b/packages/api/internal/middleware/bodylimit.go @@ -0,0 +1,50 @@ +package middleware + +import ( + "errors" + "io" + "net/http" + + "github.com/gin-gonic/gin" +) + +// RequestSizeLimiter caps how much of a request body the server will read. +// +// It deliberately writes no response of its own: it only records the 413 status +// on the context and lets the OpenAPI validator's error handler render the +// documented {code, message} error body. Writing here would commit a plain-text +// body that the error handler then appends JSON to, leaving clients with a +// response they cannot parse. +func RequestSizeLimiter(limit int64) gin.HandlerFunc { + return func(c *gin.Context) { + c.Request.Body = &limitedBody{ + ctx: c, + body: http.MaxBytesReader(c.Writer, c.Request.Body, limit), + } + + c.Next() + } +} + +type limitedBody struct { + ctx *gin.Context + body io.ReadCloser +} + +func (b *limitedBody) Read(p []byte) (int, error) { + n, err := b.body.Read(p) + + var tooLarge *http.MaxBytesError + if errors.As(err, &tooLarge) { + // The rest of the body is never read, so the connection cannot be reused. + b.ctx.Header("Connection", "close") + // Only records the status, nothing is written to the client yet. + b.ctx.Status(http.StatusRequestEntityTooLarge) + } + + return n, err +} + +func (b *limitedBody) Close() error { + return b.body.Close() +} diff --git a/packages/api/internal/utils/error.go b/packages/api/internal/utils/error.go index b89d315cde..94459f4bda 100644 --- a/packages/api/internal/utils/error.go +++ b/packages/api/internal/utils/error.go @@ -47,6 +47,14 @@ func ErrorHandler(c *gin.Context, message string, statusCode int) { c.Error(errMsg) + // Something upstream already committed a response. Appending the error + // envelope to it would produce a body no client can parse, so leave it be. + if c.Writer.Written() { + c.Abort() + + return + } + // Handle forbidden errors if after, ok := strings.CutPrefix(message, forbiddenErrPrefix); ok { c.AbortWithStatusJSON( diff --git a/packages/api/internal/utils/error_envelope_test.go b/packages/api/internal/utils/error_envelope_test.go new file mode 100644 index 0000000000..133506930c --- /dev/null +++ b/packages/api/internal/utils/error_envelope_test.go @@ -0,0 +1,138 @@ +package utils + +import ( + "context" + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/getkin/kin-openapi/openapi3filter" + "github.com/gin-gonic/gin" + middleware "github.com/oapi-codegen/gin-middleware" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + apispec "github.com/e2b-dev/infra/packages/api/internal/api" + custommw "github.com/e2b-dev/infra/packages/api/internal/middleware" +) + +const testBodyLimit = 1 << 10 + +// envelopeRouter mirrors the middleware chain main.go puts in front of every +// request: the body size limiter, then the OpenAPI validator wired to +// ErrorHandler. +func envelopeRouter(t *testing.T, authOK bool) *gin.Engine { + t.Helper() + + swagger, err := apispec.GetSpec() + require.NoError(t, err) + swagger.Servers = nil + + authFn := func(context.Context, *openapi3filter.AuthenticationInput) error { + if authOK { + return nil + } + + return http.ErrNoCookie + } + + r := gin.New() + r.Use( + custommw.RequestSizeLimiter(testBodyLimit), + middleware.OapiRequestValidatorWithOptions(swagger, &middleware.Options{ + ErrorHandler: func(c *gin.Context, message string, fallbackStatusCode int) { + ErrorHandler(c, message, max(c.Writer.Status(), fallbackStatusCode)) + }, + MultiErrorHandler: MultiErrorHandler, + Options: openapi3filter.Options{ + AuthenticationFunc: authFn, + MultiError: true, + }, + SilenceServersWarning: true, + }), + ) + r.POST("/templates", func(c *gin.Context) { c.JSON(http.StatusAccepted, gin.H{"ok": true}) }) + + return r +} + +// TestErrorResponsesUseTheDocumentedEnvelope covers EN-919: every way a +// template build request can be rejected has to answer with the Error schema +// the spec promises, so clients can report something better than +// "[undefined] no message". +func TestErrorResponsesUseTheDocumentedEnvelope(t *testing.T) { + t.Parallel() + + oversized := fmt.Sprintf(`{"dockerfile":%q,"memoryMB":512,"cpuCount":2}`, strings.Repeat("A", testBodyLimit*4)) + + tests := []struct { + name string + authOK bool + method string + path string + body string + contentType string + wantStatus int + }{ + { + name: "body over the size limit", authOK: true, + method: http.MethodPost, path: "/templates", body: oversized, + contentType: "application/json", wantStatus: http.StatusRequestEntityTooLarge, + }, + { + name: "unauthenticated", authOK: false, + method: http.MethodPost, path: "/templates", body: `{"dockerfile":"FROM alpine"}`, + contentType: "application/json", wantStatus: http.StatusBadRequest, + }, + { + name: "malformed json", authOK: true, + method: http.MethodPost, path: "/templates", body: `{not json`, + contentType: "application/json", wantStatus: http.StatusBadRequest, + }, + { + name: "body does not match the schema", authOK: true, + method: http.MethodPost, path: "/templates", body: `{}`, + contentType: "application/json", wantStatus: http.StatusBadRequest, + }, + { + name: "deprecated multipart upload", authOK: true, + method: http.MethodPost, path: "/templates", body: `dockerfile=x`, + contentType: "multipart/form-data; boundary=x", wantStatus: http.StatusBadRequest, + }, + { + name: "unknown route", authOK: true, + method: http.MethodPost, path: "/no-such-endpoint", body: `{}`, + contentType: "application/json", wantStatus: http.StatusNotFound, + }, + { + name: "method not allowed", authOK: true, + method: http.MethodPatch, path: "/templates", body: `{}`, + contentType: "application/json", wantStatus: http.StatusNotFound, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + req := httptest.NewRequestWithContext(t.Context(), tt.method, tt.path, strings.NewReader(tt.body)) + req.Header.Set("Content-Type", tt.contentType) + req.Header.Set("Authorization", "Bearer token") + + rr := httptest.NewRecorder() + envelopeRouter(t, tt.authOK).ServeHTTP(rr, req) + + assert.Equal(t, tt.wantStatus, rr.Code) + + var envelope apispec.Error + require.NoError(t, json.Unmarshal(rr.Body.Bytes(), &envelope), + "error response is not the documented Error schema: %q", rr.Body.String()) + + assert.NotZero(t, envelope.Code, "error response has no code: %q", rr.Body.String()) + assert.NotEmpty(t, envelope.Message, "error response has no message: %q", rr.Body.String()) + }) + } +} diff --git a/packages/api/main.go b/packages/api/main.go index dcef272af2..a77cf20506 100644 --- a/packages/api/main.go +++ b/packages/api/main.go @@ -18,7 +18,6 @@ import ( "github.com/getkin/kin-openapi/openapi3" "github.com/getkin/kin-openapi/openapi3filter" - limits "github.com/gin-contrib/size" "github.com/gin-gonic/gin" "github.com/google/uuid" middleware "github.com/oapi-codegen/gin-middleware" @@ -174,7 +173,7 @@ func NewGinServer(ctx context.Context, config cfg.Config, tel *telemetry.Client, // Use our validation middleware to check all requests against the // OpenAPI schema. r.Use( - limits.RequestSizeLimiter(maxUploadLimit), + customMiddleware.RequestSizeLimiter(maxUploadLimit), middleware.OapiRequestValidatorWithOptions(swagger, &middleware.Options{ ErrorHandler: func(c *gin.Context, message string, fallbackStatusCode int) { From ba4daa2962ccd4d4da42188adbc9b799e935a48a Mon Sep 17 00:00:00 2001 From: Tomas Srnka Date: Sat, 1 Aug 2026 14:48:06 +0200 Subject: [PATCH 2/3] test(api): cover the size limiter and the already-committed response guard Both live in packages the end-to-end envelope test does not belong to, so Go attributed no coverage to them. Test each in its own package instead. --- .../api/internal/middleware/bodylimit_test.go | 83 +++++++++++++++++++ .../api/internal/utils/error_envelope_test.go | 18 ++++ 2 files changed, 101 insertions(+) create mode 100644 packages/api/internal/middleware/bodylimit_test.go diff --git a/packages/api/internal/middleware/bodylimit_test.go b/packages/api/internal/middleware/bodylimit_test.go new file mode 100644 index 0000000000..99e6a10c09 --- /dev/null +++ b/packages/api/internal/middleware/bodylimit_test.go @@ -0,0 +1,83 @@ +package middleware + +import ( + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/gin-gonic/gin" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestRequestSizeLimiter(t *testing.T) { + t.Parallel() + + const limit = 16 + + tests := []struct { + name string + body string + wantErr bool + wantRead string + wantStatus int + wantConnHdr string + }{ + { + name: "body within the limit is read in full", + body: strings.Repeat("a", limit), + wantRead: strings.Repeat("a", limit), + // Nothing was rejected, so the handler decides the status. + wantStatus: http.StatusOK, + }, + { + name: "body over the limit fails the read", + body: strings.Repeat("a", limit+1), + wantErr: true, + wantStatus: http.StatusRequestEntityTooLarge, + wantConnHdr: "close", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + var ( + read []byte + readErr error + ) + + r := gin.New() + r.Use(RequestSizeLimiter(limit)) + r.POST("/", func(c *gin.Context) { + read, readErr = io.ReadAll(c.Request.Body) + require.NoError(t, c.Request.Body.Close()) + + if readErr == nil { + c.Status(http.StatusOK) + } + }) + + req := httptest.NewRequestWithContext(t.Context(), http.MethodPost, "/", strings.NewReader(tt.body)) + rr := httptest.NewRecorder() + r.ServeHTTP(rr, req) + + if tt.wantErr { + var tooLarge *http.MaxBytesError + require.ErrorAs(t, readErr, &tooLarge) + } else { + require.NoError(t, readErr) + assert.Equal(t, tt.wantRead, string(read)) + } + + assert.Equal(t, tt.wantStatus, rr.Code) + assert.Equal(t, tt.wantConnHdr, rr.Header().Get("Connection")) + // The limiter must never write a body of its own — the error + // handler is the single writer of the error response. + assert.Empty(t, rr.Body.String()) + }) + } +} diff --git a/packages/api/internal/utils/error_envelope_test.go b/packages/api/internal/utils/error_envelope_test.go index 133506930c..970a65cf33 100644 --- a/packages/api/internal/utils/error_envelope_test.go +++ b/packages/api/internal/utils/error_envelope_test.go @@ -136,3 +136,21 @@ func TestErrorResponsesUseTheDocumentedEnvelope(t *testing.T) { }) } } + +// TestErrorHandlerLeavesACommittedResponseAlone is the backstop for the corrupt +// body in EN-919: whatever a middleware has already written, ErrorHandler must +// not append the envelope to it. +func TestErrorHandlerLeavesACommittedResponseAlone(t *testing.T) { + t.Parallel() + + rr := httptest.NewRecorder() + c, _ := gin.CreateTestContext(rr) + c.Request = httptest.NewRequestWithContext(t.Context(), http.MethodPost, "/templates", strings.NewReader("{}")) + + c.String(http.StatusRequestEntityTooLarge, "request too large") + + ErrorHandler(c, "reading failed: request too large", http.StatusRequestEntityTooLarge) + + assert.True(t, c.IsAborted(), "the request should still be aborted") + assert.Equal(t, "request too large", rr.Body.String(), "the committed body must be left untouched") +} From 659c30229015f6c7078921e62a298440669240b5 Mon Sep 17 00:00:00 2001 From: Tomas Srnka Date: Mon, 3 Aug 2026 11:24:47 +0200 Subject: [PATCH 3/3] style: strip narration comments Co-Authored-By: Claude Fable 5 --- packages/api/internal/middleware/bodylimit.go | 9 +++------ .../api/internal/middleware/bodylimit_test.go | 10 ++++------ packages/api/internal/utils/error.go | 3 +-- .../api/internal/utils/error_envelope_test.go | 16 ++++++---------- 4 files changed, 14 insertions(+), 24 deletions(-) diff --git a/packages/api/internal/middleware/bodylimit.go b/packages/api/internal/middleware/bodylimit.go index 82ef9d514e..701f0e3028 100644 --- a/packages/api/internal/middleware/bodylimit.go +++ b/packages/api/internal/middleware/bodylimit.go @@ -10,11 +10,9 @@ import ( // RequestSizeLimiter caps how much of a request body the server will read. // -// It deliberately writes no response of its own: it only records the 413 status -// on the context and lets the OpenAPI validator's error handler render the -// documented {code, message} error body. Writing here would commit a plain-text -// body that the error handler then appends JSON to, leaving clients with a -// response they cannot parse. +// On overflow it records the 413 status without writing a body: the OpenAPI +// validator's error handler is the sole writer of the documented error +// envelope, and a body committed here would corrupt it. func RequestSizeLimiter(limit int64) gin.HandlerFunc { return func(c *gin.Context) { c.Request.Body = &limitedBody{ @@ -38,7 +36,6 @@ func (b *limitedBody) Read(p []byte) (int, error) { if errors.As(err, &tooLarge) { // The rest of the body is never read, so the connection cannot be reused. b.ctx.Header("Connection", "close") - // Only records the status, nothing is written to the client yet. b.ctx.Status(http.StatusRequestEntityTooLarge) } diff --git a/packages/api/internal/middleware/bodylimit_test.go b/packages/api/internal/middleware/bodylimit_test.go index 99e6a10c09..77e77aac98 100644 --- a/packages/api/internal/middleware/bodylimit_test.go +++ b/packages/api/internal/middleware/bodylimit_test.go @@ -26,10 +26,9 @@ func TestRequestSizeLimiter(t *testing.T) { wantConnHdr string }{ { - name: "body within the limit is read in full", - body: strings.Repeat("a", limit), - wantRead: strings.Repeat("a", limit), - // Nothing was rejected, so the handler decides the status. + name: "body within the limit is read in full", + body: strings.Repeat("a", limit), + wantRead: strings.Repeat("a", limit), wantStatus: http.StatusOK, }, { @@ -75,8 +74,7 @@ func TestRequestSizeLimiter(t *testing.T) { assert.Equal(t, tt.wantStatus, rr.Code) assert.Equal(t, tt.wantConnHdr, rr.Header().Get("Connection")) - // The limiter must never write a body of its own — the error - // handler is the single writer of the error response. + // The limiter must never write a body; the error handler is the sole writer. assert.Empty(t, rr.Body.String()) }) } diff --git a/packages/api/internal/utils/error.go b/packages/api/internal/utils/error.go index 94459f4bda..2b1ffa734f 100644 --- a/packages/api/internal/utils/error.go +++ b/packages/api/internal/utils/error.go @@ -47,8 +47,7 @@ func ErrorHandler(c *gin.Context, message string, statusCode int) { c.Error(errMsg) - // Something upstream already committed a response. Appending the error - // envelope to it would produce a body no client can parse, so leave it be. + // A response is already committed upstream; appending the envelope would corrupt it. if c.Writer.Written() { c.Abort() diff --git a/packages/api/internal/utils/error_envelope_test.go b/packages/api/internal/utils/error_envelope_test.go index 970a65cf33..e59df207b5 100644 --- a/packages/api/internal/utils/error_envelope_test.go +++ b/packages/api/internal/utils/error_envelope_test.go @@ -21,9 +21,8 @@ import ( const testBodyLimit = 1 << 10 -// envelopeRouter mirrors the middleware chain main.go puts in front of every -// request: the body size limiter, then the OpenAPI validator wired to -// ErrorHandler. +// envelopeRouter mirrors main.go's middleware chain: the body size limiter, +// then the OpenAPI validator wired to ErrorHandler. func envelopeRouter(t *testing.T, authOK bool) *gin.Engine { t.Helper() @@ -59,10 +58,8 @@ func envelopeRouter(t *testing.T, authOK bool) *gin.Engine { return r } -// TestErrorResponsesUseTheDocumentedEnvelope covers EN-919: every way a -// template build request can be rejected has to answer with the Error schema -// the spec promises, so clients can report something better than -// "[undefined] no message". +// TestErrorResponsesUseTheDocumentedEnvelope covers EN-919: every rejection +// path must answer with the Error schema the spec promises. func TestErrorResponsesUseTheDocumentedEnvelope(t *testing.T) { t.Parallel() @@ -137,9 +134,8 @@ func TestErrorResponsesUseTheDocumentedEnvelope(t *testing.T) { } } -// TestErrorHandlerLeavesACommittedResponseAlone is the backstop for the corrupt -// body in EN-919: whatever a middleware has already written, ErrorHandler must -// not append the envelope to it. +// TestErrorHandlerLeavesACommittedResponseAlone is the EN-919 backstop: +// ErrorHandler must not append the envelope to an already-committed response. func TestErrorHandlerLeavesACommittedResponseAlone(t *testing.T) { t.Parallel()