Cannot read request body more than once

[blakeSaucier]

This is known behavior of ASP.Net - specifically the behavior of Stream/StreamReader.

When connecting multiple HttpHandler's in Giraffe, sometimes more than one handler will need to read the request body. This results in subsequent reads of the request body from returning an empty string.

An Example

You may have an 'authHandler' which reads and hashes the request body.

let authHandler next ctx = task {
    let! body = ctx.ReadBodyFromRequestAsync()
    let header = ctx.Request.Headers.["Auth-Signature"].ToString()
    let hashed = hash body
    if header = hashed then
        return! next ctx
    else
        return! setStatusCode 401 earlyReturn ctx
}

Later on, another handler may need to read the body:

let webhookHandler next ctx = task {
    let! body = ctx.ReadBodyFromRequestAsync()
        ...
    return! setStatusCode 200 next ctx
}

They would be connected:

POST >=> choose [
    route "/webhook" >=> authHandler >=> webhookHandler
]

In this example, the webhookHandler will read an empty body from ctx.ReadBodyFromRequestAsync(). I would expect this to return the full request body.

I'm getting this behavior using Giraffe 4.1.0 on a net5.0 web project.

Possible solution

As per this blog post, the HttpRequest can be set up to be read more than once. This can be done in the ctx.ReadBodyFromRequestAsync() extension.

I'd be happy to put together a PR for this.

Thanks,

[dustinmoris]

You are correct that it is possible to read the body more than once if you implement some kind of buffer but it is normally not recommended to read the body more than once like this. The problem is that ASP.NET Core, particularly in C#, makes it difficult to do what you need, but when you take one step back and actually think about it from a holistic point of view, it is kind of strange and almost wrong to have to read a request body twice. A web server retrieves a HTTP request and reads the body once and then performs any work which is necessary to return a response. This is how it should be and if a framework makes this difficult then IMHO the developer has picked the wrong web framework. However, having said that, luckily Giraffe makes this kind of implementation relatively easy. You can define an auth handler which accepts another handler to process the request on a successful authentication (in your case signature verification).

For example:

    let hash (value : string) : string =
        value // Do your hashing algorithm

    let authHandler (successHandler : string -> HttpHandler) : HttpHandler =
        fun (next : HttpFunc) (ctx : HttpContext) ->
            task {
                let! body = ctx.ReadBodyFromRequestAsync()
                let header = ctx.Request.Headers.["Auth-Signature"].ToString()
                let hashed = hash body
                return!
                    match header = hashed with
                    | true  -> successHandler body next ctx
                    | false -> setStatusCode 401 earlyReturn ctx
            }

    let handleWeekhook (verifiedData : string) : HttpHandler =
        fun next ctx ->
            task {
                // Do something with the body (=verifiedData)
                // ...
                return! setStatusCode 200 next ctx
            }

    let someApp : HttpHandler =
        POST >=> choose [
            route "/webhook" >=> authHandler handleWeekhook
        ]

This is much more elegant in my opinion and you can get away without having to implement suboptimal hacks like buffering the response, especially since you wouldn't even need to do this every single time. I am sure there are many routes which don't require the body to be signed and these handlers would have to take an unnecessary performance hit if the buffering was implemented in a central place.

Besides that, the above pattern will make it much harder to implement wrong routes. For example, in your original example you could prepend the auth handler in front of a chain where no handler afterwards actually needs to access the body. Possibly some copy paste mistake from another route and so you'd do an unnecessary step of validating a signature and buffering the body when it's not required at all. In my example such a "copy and paste" mistake would be much harder to make, because the auth handler itself requires another handler which actually relies on having a verified body, otherwise what's the point in verifying it.

These are just my two cents and I know it is very opinionated so I'm sure some developers will disagree with me, but that would be my advice and hence why I'd rather not implement functionality to buffer the request body when I think that it's not really required in most use cases, at least not in Giraffe I believe.

EDIT:

Anyways, let me know what you think. I realise my answer sounds very strongly opinionated and I don't mean that there is no room for buffering the body, just wanted to point out an alternative which I personally think is actually a better solution but let me know how you feel about this approach and if that sufficiently solves your issue or if you still feel like there is a need to implement such a helper function :)

[blakeSaucier]

Thanks for your response and great work on this project.

I agree this is an odd use case. It's unusual for the request body to be read more than once after it hits the server.

Thank you for your example code, but I do prefer having separate handlers which are composed together via >=> for the following reasons:

• Although we could compose handlers together by taking them as parameters in other handlers (as you've shown), Giraffe encourages composition through the 'fish' operator. From the (excellent) documentation:

It is the main combinator in Giraffe which allows composing many smaller HttpHandler functions into a bigger web application:

• The authHandler can be applied to a list of routes:

    let webhookRouter =
        POST >=> authHandler >=> choose [
            route "/webhookA" >=> handleWeekhookA
            route "/webhookB" >=> handleWeekhookB
            route "/webhookC" >=> handleWeekhookC
        ]

• Consistency with Auth examples in both the documentation and the Samples repo. Often we see route "/account" >=> authorize >=> accountHandler or similar. There are also examples similar to the router I sketched above. Of course, the Auth examples are reading tokens in the Header and are not concerned with the request body.
• Yes, there would be routes which do not require the body to be signed and typos happen. Wouldn't this criticism also apply to Giraffe's existing approach for Auth handlers? In fact, any handler could be accidentally put before or after another one as their types would align.

I haven't investigated the performance implications for buffering. Any idea what the hit would be? I agree that would be a concern. I can work on some benchmarks for this.

Anyhow, these are the reasons why I thought it would be a useful change and why I think it fits with the philosophy of Giraffe. I agree this is largely a matter of opinion - let me know what you think.

[dustinmoris]

Ok, I've added a new extension method which I think will solve this for you:
https://github.com/giraffe-fsharp/Giraffe/blob/develop/src/Giraffe/ModelBinding.fs#L373

Would you be able to copy this into your application and see if it sufficiently addresses your use case?

[blakeSaucier]

Thank you, that's perfect!