gohttpiogo-gin

Does overriding c.Request.Body with io.NopCloser in a Gin middleware cause potential data leaks?


I'm writing a middleware in Gin to log the request body for debugging purposes. In order to read the body multiple times, I need to use io.NopCloser to reassign c.Request.Body. Here's my current implementation:

return func(c *gin.Context) {
    bodyBytes, _ := io.ReadAll(c.Request.Body)
    _ = c.Request.Body.Close()
    
    // Override the Close method of c.Request.Body
    c.Request.Body = io.NopCloser(bytes.NewBuffer(bodyBytes))

    var bodyJSON map[string]interface{}
    err := json.Unmarshal(bodyBytes, &bodyJSON)
    if err != nil {
        c.Next()
        return
    }

    logrus.WithFields(logrus.Fields{
        "method": c.Request.Method + "-request",
        "path":   c.Request.URL.Path,
        "query":  c.Request.URL.RawQuery,
        "body":   bodyJSON,
    }).Info()

    c.Next() 
}

I’m concerned about the use of io.NopCloser, which overrides the Close() method of the request body. Will this cause data leaks since the original Close() method is overidden by an no-op Close() method and does nothing to "close" the request body after I pass it to another middleware?

I’m relatively new to working with IO in Golang, so I’d appreciate any insights or suggestions for a better approach.


Solution

  • For server requests, you don't need to close the body:

    The Server will close the request body. The ServeHTTP Handler does not need to.

    For a client request, you need to close the body due to HTTP implementation particularities. When you need a copy you should use Request.GetBody when available.

    Your code is very similar to Gin middleware example code:

        body, err := ioutil.ReadAll(r.Body)
        if err != nil {
            return "", err
        }
        r.Body = ioutil.NopCloser(bytes.NewBuffer(body))
    

    So I assume it's fine. A “data leak” is mostly a resource leak, since memory leaks are prevented by the garbage collector. What you could leak is unnecessary used resources, like a database connection you don't need any more or a goroutine that does not terminate. There are no such issues here.


    Edit: Note that you get the body from the server framework, which it will close by itself. It doesn't say anything about the (possibly replaced) body received by your handler.

    Also note that in this case you don't replace a Close function with io.NopCloser, you add one. bytes.NewBuffer gives you a Buffer, which is an io.Reader but has no Close method.