Repository: getkin/kin-openapi (go)

Issue:
http: invalid Read on closed Body error comes in AuthenticationFunc with request body validation
I have a POST request with the request body parameter. I am using `OapiRequestValidatorWithOptions` to validate and authenticate each and every request. However, it gave `http: invalid Read on closed Body` error and fails to bind the JSON to the request object. 

I was able to avoid the error by setting ExcludeRequestBody so ValidateRequest skips request body validation as shown in the example below. 

```
validator := middleware.OapiRequestValidatorWithOptions(swagger,
		&middleware.Options{
			Options: openapi3filter.Options{
				ExcludeRequestBody: true,
				AuthenticationFunc: NewAuthenticator(middleware.AuthConfig{}),
			},
		})
```
However, I would like to continue to use ValidateRequest. 
Is there any solution to this? 

Pull request:
openapi3filter: don't consume request body in `AuthenticatorFunc`


As noted in #743, some users of the `openapi3filter`, largely via
`oapi-codegen` and `openapi.tanna.dev/go/validator`[0] are seeing that
when using an `AuthenticationFunc`, this can lead to errors such as:

    request body has an error: value is required but missing

Or:

    request body has an error: reading failed: http: invalid Read on closed Body

This turns out to be down to the fact that a call to the
`AuthenticationFunc` may read the request body, but due to the way that
Go's `io.Reader`s can only be read once, this then means the body cannot
be read by anyone else further in the request chain.

To resolve this, we can make sure that each iteration of the
`AuthenticationFunc` gets a fresh copy of the request body (in its own
`io.Reader`).

This follows logic that exists within the `ValidateRequestBody` method,
with an additional layer of caching the raw bytes from the body.

We also add tests to validate:

- calling the `ValidateRequest` body function with an
  `AuthenticationFunc` that reads the request body
- using an integration test with a test HTTP server also works,
  indicating that the HTTP handler can then read the request body itself

Closes #743.

[0]: https://gitlab.com/jamietanna/httptest-openapi/-/issues/5



Patch:
diff --git a/openapi3filter/validate_request.go b/openapi3filter/validate_request.go
index 27c8131..13e2247 100644
--- a/openapi3filter/validate_request.go
+++ b/openapi3filter/validate_request.go
@@ -416,6 +416,23 @@ func validateSecurityRequirement(ctx context.Context, input *RequestValidationIn
 		securitySchemes = components.SecuritySchemes
 	}
 
+	// NOTE that because we could have an `AuthenticationFunc` that reads the request body, we need to provide a fresh `io.Reader` to each iteration of the loop. To make this more performant, we can read the request body once into memory (which may be costly) and then create a fresh `io.Reader` for each `AuthenticationFunc`
+	var data []byte
+
+	if input.Request != nil && input.Request.Body != http.NoBody && input.Request.Body != nil {
+		defer input.Request.Body.Close()
+
+		var err error
+		data, err = io.ReadAll(input.Request.Body)
+		if err != nil {
+			return &RequestError{
+				Input:  input,
+				Reason: "reading failed",
+				Err:    err,
+			}
+		}
+	}
+
 	// For each scheme for the requirement
 	for _, name := range names {
 		var securityScheme *openapi3.SecurityScheme
@@ -431,6 +448,26 @@ func validateSecurityRequirement(ctx context.Context, input *RequestValidationIn
 			}
 		}
 		scopes := securityRequirement[name]
+
+		// if there was a request body, then make sure we provide a new copy of the body in the `input`
+		if data != nil {
+			var err error
+			// Put the data back into the input
+			input.Request.Body = nil
+			if input.Request.GetBody != nil {
+				if input.Request.Body, err = input.Request.GetBody(); err != nil {
+					input.Request.Body = nil
+				}
+			}
+			if input.Request.Body == nil {
+				input.Request.ContentLength = int64(len(data))
+				input.Request.GetBody = func() (io.ReadCloser, error) {
+					return io.NopCloser(bytes.NewReader(data)), nil
+				}
+				input.Request.Body, _ = input.Request.GetBody() // no error return
+			}
+		}
+
 		if err := f(ctx, &AuthenticationInput{
 			RequestValidationInput: input,
 			SecuritySchemeName:     name,
@@ -440,5 +477,24 @@ func validateSecurityRequirement(ctx context.Context, input *RequestValidationIn
 			return err
 		}
 	}
+
+	// if there was a request body, then make sure we put it back into the `input`
+	if data != nil {
+		var err error
+		// Put the data back into the input
+		input.Request.Body = nil
+		if input.Request.GetBody != nil {
+			if input.Request.Body, err = input.Request.GetBody(); err != nil {
+				input.Request.Body = nil
+			}
+		}
+		if input.Request.Body == nil {
+			input.Request.ContentLength = int64(len(data))
+			input.Request.GetBody = func() (io.ReadCloser, error) {
+				return io.NopCloser(bytes.NewReader(data)), nil
+			}
+			input.Request.Body, _ = input.Request.GetBody() // no error return
+		}
+	}
 	return nil
 }


Test patch:
diff --git a/openapi3filter/issue743_test.go b/openapi3filter/issue743_test.go
new file mode 100644
index 0000000..24122e1
--- /dev/null
+++ b/openapi3filter/issue743_test.go
@@ -0,0 +1,207 @@
+package openapi3filter_test
+
+import (
+	"context"
+	"fmt"
+	"io"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+
+	"github.com/getkin/kin-openapi/openapi3"
+	"github.com/getkin/kin-openapi/openapi3filter"
+	"github.com/getkin/kin-openapi/routers/gorillamux"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestValidateRequestWithAnAuthenticatorFunc_CanConsumeTheRequestBody(t *testing.T) {
+	const spec = `
+openapi: 3.0.0
+info:
+  title: 'Validator'
+  version: 0.0.1
+paths:
+  /test:
+    post:
+      requestBody:
+        required: true
+        content:
+          text/plain:
+            schema:
+              type: string
+      responses:
+        '200':
+          description: Created
+components:
+  securitySchemes:
+    BearerAuth:
+      type: http
+      scheme: bearer
+      bearerFormat: JWT
+security:
+  - BearerAuth: [ ]
+`
+
+	loader := openapi3.NewLoader()
+	doc, err := loader.LoadFromData([]byte(spec))
+	require.NoError(t, err)
+
+	err = doc.Validate(loader.Context)
+	require.NoError(t, err)
+
+	router, err := gorillamux.NewRouter(doc)
+	require.NoError(t, err)
+
+	req, err := http.NewRequest(http.MethodPost, "/test", strings.NewReader("something"))
+	require.NoError(t, err)
+
+	req.Header.Set("Content-Type", "text/plain")
+
+	route, pathParams, err := router.FindRoute(req)
+	require.NoError(t, err)
+
+	err = openapi3filter.ValidateRequest(
+		context.Background(),
+		&openapi3filter.RequestValidationInput{
+			Request:    req,
+			PathParams: pathParams,
+			Route:      route,
+			Options: &openapi3filter.Options{
+				AuthenticationFunc: func(ctx context.Context, ai *openapi3filter.AuthenticationInput) error {
+					defer req.Body.Close()
+
+					// NOTE that reading from `req.Body` appears to trigger the underlying issue raised in https://github.com/getkin/kin-openapi/issues/743
+					// this doesn't seem to occur when using `req.GetBody()`, but as that's less common to do, we should support both types
+					body, err := io.ReadAll(ai.RequestValidationInput.Request.Body)
+					assert.NoError(t, err)
+
+					// and make sure the parsed body was correct
+					assert.Equal(t, []byte("something"), body)
+
+					return nil
+				},
+			},
+		},
+	)
+
+	require.NoError(t, err)
+}
+
+func TestValidateRequestWithAnAuthenticatorFunc_CanConsumeTheRequestBodyAndThenBeParsedByARouter(t *testing.T) {
+	const spec = `
+openapi: 3.0.0
+info:
+  title: 'Validator'
+  version: 0.0.1
+paths:
+  /test:
+    post:
+      requestBody:
+        required: true
+        content:
+          text/plain:
+            schema:
+              type: string
+      responses:
+        '200':
+          description: Created
+components:
+  securitySchemes:
+    BearerAuth:
+      type: http
+      scheme: bearer
+      bearerFormat: JWT
+security:
+  - BearerAuth: [ ]
+`
+
+	loader := openapi3.NewLoader()
+	doc, err := loader.LoadFromData([]byte(spec))
+	require.NoError(t, err)
+
+	err = doc.Validate(loader.Context)
+	require.NoError(t, err)
+
+	router, err := gorillamux.NewRouter(doc)
+	require.NoError(t, err)
+
+	called := false
+	authenticatorFunc := func(ctx context.Context, ai *openapi3filter.AuthenticationInput) error {
+		defer ai.RequestValidationInput.Request.Body.Close()
+
+		// NOTE that reading from `req.Body` appears to trigger the underlying issue raised in https://github.com/getkin/kin-openapi/issues/743
+		// this doesn't seem to occur when using `req.GetBody()`, but as that's less common to do, we should support both types
+		body, err := io.ReadAll(ai.RequestValidationInput.Request.Body)
+		assert.NoError(t, err)
+
+		// and make sure the parsed body was correct
+		assert.Equal(t, []byte("something"), body)
+
+		return nil
+	}
+
+	use := func(r *http.ServeMux, middlewares ...func(next http.Handler) http.Handler) http.Handler {
+		var s http.Handler
+		s = r
+
+		for _, mw := range middlewares {
+			s = mw(s)
+		}
+
+		return s
+	}
+
+	validationMiddleware := func(next http.Handler) http.Handler {
+		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			route, pathParams, err := router.FindRoute(r)
+			if err != nil {
+				fmt.Println(err.Error())
+				w.WriteHeader(http.StatusInternalServerError)
+				return
+			}
+
+			err = openapi3filter.ValidateRequest(r.Context(), &openapi3filter.RequestValidationInput{
+				Request:    r,
+				PathParams: pathParams,
+				Route:      route,
+				Options: &openapi3filter.Options{
+					AuthenticationFunc: authenticatorFunc,
+				},
+			})
+			require.NoError(t, err)
+
+			next.ServeHTTP(w, r)
+		})
+	}
+
+	r := http.NewServeMux()
+	r.HandleFunc("/test", func(w http.ResponseWriter, r *http.Request) {
+		called = true
+		defer r.Body.Close()
+
+		body, err := io.ReadAll(r.Body)
+		assert.NoError(t, err)
+
+		// and make sure the parsed body was correct
+		assert.Equal(t, []byte("something"), body)
+
+		w.WriteHeader(http.StatusNoContent)
+	})
+
+	ts := httptest.NewServer(use(r, validationMiddleware))
+
+	defer ts.Close()
+
+	req, err := http.NewRequest(http.MethodPost, ts.URL+"/test", strings.NewReader("something"))
+	require.NoError(t, err)
+
+	req.Header.Set("Content-Type", "text/plain")
+
+	resp, err := http.DefaultClient.Do(req)
+	require.NoError(t, err)
+
+	assert.Equal(t, http.StatusNoContent, resp.StatusCode)
+	assert.True(t, called, "The POST /test endpoint should have been called, but it wasn't")
+}