Skip to content

Commit 01273ed

Browse files
mromaszewiczclaude
andcommitted
fix: don't clone request in prefix stripping
Closes: #69 Replace makeRequestForValidation (which used r.Clone()) with withPrefixStripped, which temporarily modifies the request's path fields in place and restores them after validation. r.Clone() shallow-copies the Body, so when validation consumed the body on the clone, the original request passed to the handler had an empty body, causing 'EOF' errors on decode. The new approach mutates the path fields for the duration of route finding and validation, then restores them before the handler or error handler sees the request. No clone, no body sharing, no buffering of potentially large request bodies. The r.Clone() was only done as a way to back up the request URI to send the prefix-stripped request into validation. Fixes #69 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 08dfb43 commit 01273ed

2 files changed

Lines changed: 181 additions & 35 deletions

File tree

internal/test/nethttp/oapi_validate_prefix_test.go

Lines changed: 120 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package gorilla
33
import (
44
"context"
55
_ "embed"
6+
"encoding/json"
67
"net/http"
78
"testing"
89

@@ -284,3 +285,122 @@ components:
284285
assert.Equal(t, http.StatusOK, rec.Code)
285286
assert.True(t, called, "handler should have been called when auth passes")
286287
}
288+
289+
// bodyReadableSpec defines a POST /resource with a required JSON body,
290+
// used to test that the handler can still read the body after validation.
291+
const bodyReadableSpec = `
292+
openapi: "3.0.0"
293+
info:
294+
version: 1.0.0
295+
title: TestServer
296+
paths:
297+
/resource:
298+
post:
299+
operationId: createResource
300+
requestBody:
301+
required: true
302+
content:
303+
application/json:
304+
schema:
305+
type: object
306+
required:
307+
- name
308+
properties:
309+
name:
310+
type: string
311+
additionalProperties: false
312+
responses:
313+
'204':
314+
description: No content
315+
`
316+
317+
// TestPrefix_RequestBodyReadableByHandler_WithAndWithoutPrefix is a regression
318+
// test for https://github.com/oapi-codegen/nethttp-middleware/issues/69.
319+
//
320+
// When Prefix is set, makeRequestForValidation used to clone the request via
321+
// r.Clone(), which shallow-copies the Body. Validation then consumed the body
322+
// on the clone, leaving the original body empty for the downstream handler.
323+
func TestPrefix_RequestBodyReadableByHandler_WithAndWithoutPrefix(t *testing.T) {
324+
spec, err := openapi3.NewLoader().LoadFromData([]byte(bodyReadableSpec))
325+
require.NoError(t, err)
326+
spec.Servers = nil
327+
328+
tests := []struct {
329+
name string
330+
prefix string
331+
routePath string
332+
requestPath string
333+
}{
334+
{
335+
name: "without prefix",
336+
prefix: "",
337+
routePath: "/resource",
338+
requestPath: "http://example.com/resource",
339+
},
340+
{
341+
name: "with prefix",
342+
prefix: "/api",
343+
routePath: "/api/resource",
344+
requestPath: "http://example.com/api/resource",
345+
},
346+
}
347+
348+
for _, tc := range tests {
349+
t.Run(tc.name, func(t *testing.T) {
350+
mux := http.NewServeMux()
351+
mux.HandleFunc(tc.routePath, func(w http.ResponseWriter, r *http.Request) {
352+
var payload struct {
353+
Name string `json:"name"`
354+
}
355+
if err := json.NewDecoder(r.Body).Decode(&payload); err != nil {
356+
http.Error(w, "handler failed to decode body: "+err.Error(), http.StatusBadRequest)
357+
return
358+
}
359+
assert.Equal(t, "Jamie", payload.Name)
360+
w.WriteHeader(http.StatusNoContent)
361+
})
362+
363+
mw := middleware.OapiRequestValidatorWithOptions(spec, &middleware.Options{
364+
Prefix: tc.prefix,
365+
})
366+
server := mw(mux)
367+
368+
body := map[string]string{"name": "Jamie"}
369+
rec := doPost(t, server, tc.requestPath, body)
370+
assert.Equal(t, http.StatusNoContent, rec.Code, "body: %s", rec.Body.String())
371+
})
372+
}
373+
}
374+
375+
// TestPrefix_RequestBodyReadableByHandler_ErrorHandlerWithOpts is the same
376+
// regression test but exercising the ErrorHandlerWithOpts code path.
377+
func TestPrefix_RequestBodyReadableByHandler_ErrorHandlerWithOpts(t *testing.T) {
378+
spec, err := openapi3.NewLoader().LoadFromData([]byte(bodyReadableSpec))
379+
require.NoError(t, err)
380+
spec.Servers = nil
381+
382+
mux := http.NewServeMux()
383+
mux.HandleFunc("/api/resource", func(w http.ResponseWriter, r *http.Request) {
384+
var payload struct {
385+
Name string `json:"name"`
386+
}
387+
if err := json.NewDecoder(r.Body).Decode(&payload); err != nil {
388+
http.Error(w, "handler failed to decode body: "+err.Error(), http.StatusBadRequest)
389+
return
390+
}
391+
assert.Equal(t, "Jamie", payload.Name)
392+
w.WriteHeader(http.StatusNoContent)
393+
})
394+
395+
mw := middleware.OapiRequestValidatorWithOptions(spec, &middleware.Options{
396+
Prefix: "/api",
397+
ErrorHandlerWithOpts: func(ctx context.Context, err error, w http.ResponseWriter, r *http.Request, opts middleware.ErrorHandlerOpts) {
398+
http.Error(w, err.Error(), opts.StatusCode)
399+
},
400+
})
401+
server := mw(mux)
402+
403+
body := map[string]string{"name": "Jamie"}
404+
rec := doPost(t, server, "http://example.com/api/resource", body)
405+
assert.Equal(t, http.StatusNoContent, rec.Code, "body: %s", rec.Body.String())
406+
}

oapi_validate.go

Lines changed: 61 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -169,27 +169,40 @@ func performRequestValidationForErrorHandler(next http.Handler, w http.ResponseW
169169
errorHandler(w, err.Error(), statusCode)
170170
}
171171

172-
func makeRequestForValidation(r *http.Request, options *Options) *http.Request {
172+
// withPrefixStripped temporarily strips the configured prefix from the
173+
// request's path fields, calls fn, then restores the original values.
174+
// This avoids cloning the request (which shallow-copies the Body and
175+
// causes it to be consumed during validation, leaving the handler with
176+
// an empty body — see https://github.com/oapi-codegen/nethttp-middleware/issues/69).
177+
func withPrefixStripped(r *http.Request, options *Options, fn func()) {
173178
if options == nil || options.Prefix == "" {
174-
return r
179+
fn()
180+
return
175181
}
176182

177183
// Only strip the prefix when it matches on a path segment boundary:
178184
// the path must equal the prefix exactly, or the character immediately
179185
// after the prefix must be '/'.
180186
if !hasPathPrefix(r.URL.Path, options.Prefix) {
181-
return r
187+
fn()
188+
return
182189
}
183190

184-
r = r.Clone(r.Context())
191+
origRequestURI := r.RequestURI
192+
origPath := r.URL.Path
193+
origRawPath := r.URL.RawPath
185194

186195
r.RequestURI = stripPrefix(r.RequestURI, options.Prefix)
187196
r.URL.Path = stripPrefix(r.URL.Path, options.Prefix)
188197
if r.URL.RawPath != "" {
189198
r.URL.RawPath = stripPrefix(r.URL.RawPath, options.Prefix)
190199
}
191200

192-
return r
201+
fn()
202+
203+
r.RequestURI = origRequestURI
204+
r.URL.Path = origPath
205+
r.URL.RawPath = origRawPath
193206
}
194207

195208
// hasPathPrefix reports whether path starts with prefix on a segment boundary.
@@ -210,19 +223,39 @@ func stripPrefix(s, prefix string) string {
210223

211224
// Note that this is an inline-and-modified version of `validateRequest`, with a simplified control flow and providing full access to the `error` for the `ErrorHandlerWithOpts` function.
212225
func performRequestValidationForErrorHandlerWithOpts(next http.Handler, w http.ResponseWriter, r *http.Request, router routers.Router, options *Options) {
213-
// Build a (possibly prefix-stripped) request for validation, but keep
214-
// the original so the downstream handler sees the un-modified path.
215-
validationReq := makeRequestForValidation(r, options)
226+
var route *routers.Route
227+
var pathParams map[string]string
228+
var validationErr error
229+
230+
// Temporarily strip the prefix for route finding and validation,
231+
// then restore it so the handler and error handler see the original path.
232+
withPrefixStripped(r, options, func() {
233+
var err error
234+
route, pathParams, err = router.FindRoute(r)
235+
if err != nil {
236+
validationErr = err
237+
return
238+
}
216239

217-
// Find route
218-
route, pathParams, err := router.FindRoute(validationReq)
219-
if err != nil {
240+
requestValidationInput := &openapi3filter.RequestValidationInput{
241+
Request: r,
242+
PathParams: pathParams,
243+
Route: route,
244+
}
245+
246+
if options != nil {
247+
requestValidationInput.Options = &options.Options
248+
}
249+
250+
validationErr = openapi3filter.ValidateRequest(r.Context(), requestValidationInput)
251+
})
252+
253+
// Route not found
254+
if route == nil && validationErr != nil {
220255
errOpts := ErrorHandlerOpts{
221-
// MatchedRoute will be nil, as we've not matched a route we know about
222256
StatusCode: http.StatusNotFound,
223257
}
224-
225-
options.ErrorHandlerWithOpts(r.Context(), err, w, r, errOpts)
258+
options.ErrorHandlerWithOpts(r.Context(), validationErr, w, r, errOpts)
226259
return
227260
}
228261

@@ -231,44 +264,26 @@ func performRequestValidationForErrorHandlerWithOpts(next http.Handler, w http.R
231264
Route: route,
232265
PathParams: pathParams,
233266
},
234-
// other options will be added before executing
235267
}
236268

237-
// Validate request
238-
requestValidationInput := &openapi3filter.RequestValidationInput{
239-
Request: validationReq,
240-
PathParams: pathParams,
241-
Route: route,
242-
}
243-
244-
if options != nil {
245-
requestValidationInput.Options = &options.Options
246-
}
247-
248-
err = openapi3filter.ValidateRequest(validationReq.Context(), requestValidationInput)
249-
if err == nil {
250-
// it's a valid request, so serve it with the original request
269+
if validationErr == nil {
251270
next.ServeHTTP(w, r)
252271
return
253272
}
254273

255274
var theErr error
256275

257-
switch e := err.(type) {
276+
switch e := validationErr.(type) {
258277
case openapi3.MultiError:
259278
theErr = e
260279
errOpts.StatusCode = determineStatusCodeForMultiError(e)
261280
case *openapi3filter.RequestError:
262-
// We've got a bad request
263281
theErr = e
264282
errOpts.StatusCode = http.StatusBadRequest
265283
case *openapi3filter.SecurityRequirementsError:
266284
theErr = e
267285
errOpts.StatusCode = http.StatusUnauthorized
268286
default:
269-
// This should never happen today, but if our upstream code changes,
270-
// we don't want to crash the server, so handle the unexpected error.
271-
// return http.StatusInternalServerError,
272287
theErr = fmt.Errorf("error validating route: %w", e)
273288
errOpts.StatusCode = http.StatusInternalServerError
274289
}
@@ -279,8 +294,19 @@ func performRequestValidationForErrorHandlerWithOpts(next http.Handler, w http.R
279294
// validateRequest is called from the middleware above and actually does the work
280295
// of validating a request.
281296
func validateRequest(r *http.Request, router routers.Router, options *Options) (int, error) {
282-
r = makeRequestForValidation(r, options)
297+
var statusCode int
298+
var validationErr error
299+
300+
withPrefixStripped(r, options, func() {
301+
statusCode, validationErr = doValidateRequest(r, router, options)
302+
})
303+
304+
return statusCode, validationErr
305+
}
283306

307+
// doValidateRequest performs the actual validation, called within
308+
// withPrefixStripped so the prefix is already removed from r's path.
309+
func doValidateRequest(r *http.Request, router routers.Router, options *Options) (int, error) {
284310
// Find route
285311
route, pathParams, err := router.FindRoute(r)
286312
if err != nil {

0 commit comments

Comments
 (0)