cojs / co-body

Parse request bodies with co
MIT License
325 stars 42 forks source link

Support request body inflate and gunzip when Content-Encoding header is appropriately set #38

Open bminer opened 8 years ago

bminer commented 8 years ago

If a client sends an HTTP request with "Content-Encoding" header set, co-body should be able to quickly pipe that to zlib and inflate the compressed request body prior to parsing the body.

I'd be happy to help and submit a PR, but I'd like to discuss how to implement first. Any thoughts? Is this something that needs to be done in raw-body or another lib? raw-body needs the stream only, but each "buddy" in co-body needs the full HTTP request Object. Might require a bit of refactoring?

bminer commented 8 years ago

I should also mention that the old Express body-parser lib supports HTTP request inflation:

https://github.com/expressjs/body-parser/blob/master/lib/read.js#L143-L176

haoxins commented 8 years ago

SGTM

we can dep https://github.com/stream-utils/inflation

bminer commented 8 years ago

@coderhaoxin - I like it. I'll submit a PR that modifies each "buddy" (i.e. lib/json.js) to use the inflation lib.

bminer commented 8 years ago

Just waiting for this to be merged... https://github.com/stream-utils/inflation/pull/14

felixsanz commented 7 years ago

@bminer , @fengmk2

If a client sends an HTTP request with "Content-Encoding" header set, co-body should be able to quickly pipe that to zlib and inflate the compressed request body prior to parsing the body.

This PR should be reverted, as stated in #51, not all Content-Encodings are gzip. For example DeflateRaw and Brotli will make the parser fail and the body will not be available.

fengmk2 commented 7 years ago

@bminer can you fix @felixsanz's bug too?

felixsanz commented 7 years ago

@fengmk2 I think the inflate/decompress thing should be bundled in it's own middleware, like this:

app.use(bodyparser)
app.use(decompressBody) // some inflate middleware
// or app.use(brotli) <- decompress brotli bodies

This is a bodyparser, and that is the only thing that it should do. If the body is compressed or not I don't think it has anything to do with this middleware. It'll be a mess to support all encodings here (and the upcoming ones like the facebook's zstd).

bminer commented 7 years ago

I'm not so sure that I agree. If the HTTP body is compressed, the bodyparser should be able to decompress it in order to parse it. I see the argument for separate middleware, but gzip is the "standard".

What is wrong about trying to support other inflation algorithms?

bminer commented 7 years ago

Also deflate and gzip are supported by the inflation lib. Maybe Brotli can be added?

felixsanz commented 7 years ago

and what about deflateRaw? and zopli? and zstd? :/

Anyway, supporting brotli is more than nothing. Hell, even if you don't support brotli i'm happy as long as you remove the default inflate() process that is giving me errors because i'm not using deflate :D

bminer commented 7 years ago

Hmm, what errors do you get with an unsupported content encoding?

bminer commented 7 years ago

Nvm, looks like the inflation lib raises an "unsupported content encoding" error 415. Can you use a middleware to inflate the body and then set the content-encoding header to "identity"? Long term solution could be to add support for brotli et. al. to the inflation lib.

felixsanz commented 7 years ago

I can't inflate the body because there is no body :P The bodyparser doesn't parse the body because error 415.

I should close the other issue and continue just here, right?

bminer commented 7 years ago

OK, here's my understanding of co-body. First, we inflate the req stream using the inflation lib. Then, we pass that stream to raw-body to get the size-validated raw HTTP body Buffer. Finally, the body is parsed using the logic in this lib.

The problem is that co-body expects req to be the stream from which the HTTP body should be read. Rather, co-body should read from the inflated stream. Is there a way in Koa to replace req with the stream returned from inflate(req)?

If not, it would be rather difficult to break the HTTP body inflation into a separate module. Thus, the best way forward might be to add support for more compression algorithms into the inflation lib. Right now, it supports zlib stuff, but it could be modified to add support for others.

felixsanz commented 7 years ago

@bminer As i said above, there is more compression methods than inflate/gunzip. You can't inflate a body that is not deflated. It's like trying to decompress a zip file with rar specific tools.

And i don't think modify the inflate lib is a good idea because inflate is inflate and brotli is brotli. Or at least the library should be renamed because naming it "inflate" when it decompress brotli is confusing as hell

bminer commented 7 years ago

@felixsanz - Fair point about the inflation lib being called "inflation". Might be confusing if it supported brotli or zstd. Maybe a better name would be decompress-stream?