search

jshttp / type-is

Infer the content-type of a request.
MIT License
226 stars 32 forks source link

Error thrown by media-typer when Content-Type header not set #31

Closed thierer closed 6 years ago

thierer commented 6 years ago

I ran into an issue when using express 4.16.2 and body-parser.urlencoded to handle a POST request with an optional body.

When the body was empty, the request had Content-Length: 0 but no Content-Type header. That caused an error "TypeError: argument string is required" which was ultimately thrown by

https://github.com/jshttp/media-typer/blob/d49d41ffd0bb5a0655fa44a59df2ec0bfc835b16/index.js#L141

but I think the root cause is

https://github.com/jshttp/type-is/blob/9e88be851cc628364ad8842433dce32437ea4e73/index.js#L139-L142 (or any of the methods typeis() calls) because none of them check if the Content-Type header is set.

Now, the fix was easy (just set Content-Type even for an empty body) and it might well be that the specs require that for all POST requests anyway.

So the only reason I'm opening this issue is that I wonder if it's really the intended behaviour, as the comment for typeofrequest() states

https://github.com/jshttp/type-is/blob/9e88be851cc628364ad8842433dce32437ea4e73/index.js#L101

which obviously isn't true, as media-typer.parse() throws before that can happen...

dougwilson commented 6 years ago

Yea, it should definately not throw, just return false. We have the no content type test https://github.com/jshttp/type-is/blob/master/test/test.js#L42-L49 but maybe the test is not working correctly? Is it possible you can put together a reproduction that only uses this module so we can see it? Alternatively you're welcome to make a PR šŸ‘ Just made the code change and add a test that fails without the code change and passes with the code change.

thierer commented 6 years ago

I think the throw is triggered by the combination of Content-Length being set and Content-Type not being set, which the test might not cover (because if Content-Length is not set, typeofrequest() already bails early).

I'll come up with either a test case or a PR and get back to you.

dougwilson commented 6 years ago

Gotcha! :+1: I tried a few different things, following your description but still just getting the return value false and no thrown error. Not sure how to reproduce. I'll wait on one of your two things because I'm not sure how to fix it if I cannot reproduce. Also maybe also want to double check you're using the latest version of this module, in case we are using different versions and that's why I'm not seeing it.

thierer commented 6 years ago

Sorry for the noise. The error is actually caught, but I accidentally had "Break on all exceptions" enabled šŸ¤¦ā€ā™‚ļø.

dougwilson commented 6 years ago

Ah, gotcha. It's no problem at all.