Availability of raw body bytes when using parsers

[stevegraham]

We're implementing HTTP signatures in our Elixir applications. We need to be able to hash the request body in order to verify the overall request signature. As Cowboy only allows the body to be read once and Plug does not keep the raw bytes from the socket anywhere doing this is impossible when using any of the Plug parsers.

Verifying the signature of incoming requests is a widespread pattern (Twilio, Stripe, etc) and IMO something any web library should not make it difficult to do.

I am proposing some solutions but want to begin a general discussion around design with the maintainers before writing code:

• Plug.Conn.read_body/2 should store the raw body somewhere in there Conn's structure. If so where? Another field on the struct, or as private data? (This could allow read_body to be called multiple times idempotently during a request, hiding the leaky cowboy abstraction).

• Plug.Conn.read_body/2 should allow the registration of callback functions that are called with the tuple return value of the function.

[josevalim]

We explicitly chose to follow Cowboy semantics here because we don't want to keep the body around unless we have to. So it is not much about leaky abstraction but rather a design decision. The reason for that is that keeping the whole body around can be a very bad idea, especially for large requests, as it will lead to memory bloat.

I have typically solved those issues by reading and parsing the body before Plug.Parsers and storing the parsed params inside body_params. This should be straight-forward for JSON requests. Plug.Parsers will notice the request was handled and just move forward accordingly.

I understand this has the trade-off you will have to put the logic to handle those requests early in your stack but any other option means keeping the body around and we would like to actively discourage that.

Thoughts?

[stevegraham]

read_body will read 8_000_000 bytes by default, the JSON parser won't parse any payload larger than that unless configured to. So if read_body actually puts that data somewhere the default worst case is an extra 8MB of memory allocation. Is that terrible?

It's a good point that not everyone needs this functionality and so will not want to pay this cost, but by making this behaviour configurable, i.e. a preserve_req_body parser configuration option, or providing some callback mechanism makes the cost opt-in for those that do need it.

[stevegraham]

In this case of HTTP signatures a callback would allow you to receive the body, perform the digest operation on it, put the result somewhere, discard the body. This negates the need to keep the unparsed request body.

[josevalim]

I am still not sold on something like preserve_req_body because it is easy to accidentally enable it for all requests by mistake. Then someone adds a file upload feature later, bumps the limit to 100MB, and now you have 100MB of data in memory per upload request.

A callback implies adding hooks around something... but is reading a JSON body something that warrants adding this complexity? To set this callback, you will already need a custom plug that knows which routes the callback should be injected on. This plug should also track the content-type and the signature header. So most of the work is already there. The only part missing is reading the body and decoding it, which is about 3 lines of code:

{:ok, body, conn} = Plug.Conn.read_body(conn, ...your opts...)
verify_signature!(conn, body)
%{conn | body_params: Jason.decode!(body)}

Thoughts?

[stevegraham]

IMO there isn't a reasonable case where a Plug parser would be parsing a 100MB file upload, i.e. you would probably not use multipart to upload 100MB file. A separate upload endpoint where you can give visual progress updates to the user is a more reasonable implementation.

I don't think it's as simple as 3 lines of code either, as none of Plug's parsers are so brief!

[josevalim]

@stevegraham the plug parsers are not as brief because they need to handle other concerns that you don't necessarily have to.

[josevalim]

For instance, we need to pattern match on different results to raise readable error messages because the errors are coming from the guts of Plug. But for your own application, I would say {:ok, body, conn} = Plug.Conn.read_body(conn, ...your opts...) is good enough because in case of invalid results the source of the MatchError is coming from your own code and not deep inside a library.

[stevegraham]

It's simple if I don't mind my app potentially 500-ing instead of returning a useful error response.

Because read_body is called in each of the parser implementations rather than the wrapper for the application to handle this itself in a flexible way, i.e. be agnostic as to the underlying media type, it would have to provide its own parsers for all media types it wishes to support. This does not sound like a very reasonable solution to me. Especially as the primary function my application wishes to do is not parse the body.

[josevalim]

Because read_body is called in each of the parser implementations rather than the wrapper for the application to handle this itself in a flexible way, i.e. be agnostic as to the underlying media type

Yes, but everytime this comes up it is always about json and that's very straight-forward to handle. I would agree with those points if you were handling multipart but that's never the case.

Especially as the primary function my application wishes to do is not parse the body.

Once you need to verify the body, understanding the shape of the body and validating it becomes your responsibility. Decoding is the smallest part of it.

At the end of the day, I don't think the complexity of a callback is worth here, so we don't plan to introduce this feature in Plug. Thanks!

[josevalim]

It's simple if I don't mind my app potentially 500-ing instead of returning a useful error response.

It is unlikely that the payload they send to you will pass the limit and the other error, which is a timeout one, raises a 500 anyway. I would personally be fine with raising a MatchError here because of those factors, but ofc it is your choice to make.

[stevegraham]

Yes, but everytime this comes up it is always about json and that's very straight-forward to handle. I would agree with those points if you were handling multipart but that's never the case.

It's not just JSON. It's any media type. Our API accepts x-www-form-urlencoded and JSON media types.

Once you need to verify the body, understanding the shape of the body and validating it becomes your responsibility. Decoding is the smallest part of it.

We would like to authenticate the request via signature regardless of the underlying media type. The body contents are opaque to the the HTTP signatures scheme. All that is needed is a cryptographic hash of the body. This is clearly not a parser concern.

At the end of the day, I don't think the complexity of a callback is worth here, so we don't plan to introduce this feature in Plug. Thanks!

The discussion has steered toward a callback as compromise for not storing a copy of the body during the request. Personally I don't feel you presented a good argument for not doing that, but in the interests of validating the proposals I was prepared to discuss all options.

I disagree with the complexity, a quick search of the GH issues reveal this is a frequent an recurring problem. It is the job of a framework to abstract these common tasks.

It's your prerogative to shut down the discussion but I don't think that it's very productive.

[josevalim]

The building block for the functionality is there: you can read the body.

If you want to provide your own mechanism that reads the body and then caches it for further processing, or even wrap around it to support callbacks, you can do that on top of the existing APIs.

I agree that a new mechanism won't work with Plug.Parsers but Plug.Parsers is just one of the ways to read the body. It is not necessarily the only one. Plug's responsibility is to make sure those things are possible. We don't need to provide a built-in plug that is capable of handling all kinds of requests. Plug is a library and not a framework.

For those reasons, I would prefer if folks built this feature set on top of the existing APIs rather than causing us to pack more inside Plug. The solution for this can be as simple (implement a simple plug for json) or as complex (implement an improved Plug.Parsers) as you want it to be.

[stevegraham]

The building block for the functionality is there: you can read the body.

There is no way to do this in an unobtrusive manner, i.e. without affecting downstream plugs. But ok. 🤷🏻‍♂️

I agree that a new mechanism won't work with Plug.Parsers but Plug.Parsers is just one of the ways to read the body. It is not necessarily the only one. Plug's responsibility is to make sure those things are possible. We don't need to provide a built-in plug that is capable of handling all kinds of requests.

Plug could make it easier for a user to override this behaviour, i.e. by reading the request body in one place, e.g. Plug.Parsers.call/2 instead of each submodule. That way I would only need to provide one stand-in "parser" module.

Plug is a library and not a framework.

They are semantically equivalent IMO, e.g. Apple calls libraries frameworks and vice versa

For those reasons, I would prefer if folks built this feature set on top of the existing APIs rather than causing us to pack more inside Plug. The solution for this can be as simple (implement a simple plug for json) or as complex (implement an improved Plug.Parsers) as you want it to be.

Ultimately, being able to read the body once is IMO an unreasonable assumption for Plug to make. It's fine for Cowboy given the level of abstraction it operates at.

Clearly our opinions are divergent here, but thanks for taking the time to discuss the matter.

[josevalim]

Plug could make it easier for a user to override this behaviour, i.e. by reading the request body in one place, e.g. Plug.Parsers.call/2 instead of each submodule. That way I would only need to provide one stand-in "parser" module.

I agree with this but I can't think of anyway of doing so that wouldn't break backwards compatibility. Thoughts?

They are semantically equivalent IMO, e.g. Apple calls libraries frameworks and vice versa

Fine. :) We don't need to agree on the definition of library vs framework. My point is that I don't think this warrants a change to Plug.Conn because it provides other ways to achieve this. I am fine with pushing this complexity to Plug.Parsers though, as it is building on top.