search

jshttp / content-type

Create and parse HTTP Content-Type header
MIT License
131 stars 25 forks source link

Does not throw on some incorrect headers #25

Closed wesleytodd closed 4 weeks ago

wesleytodd commented 4 weeks ago

https://github.com/jshttp/type-is/pull/57

When updating type-is to use this package from media-typer we no longer throw on this incorrect header pattern. I am not able to debug this morning but wanted to file this to track it.

cc @dougwilson I dm'd you on slack about this but figured I could just do it here so the stuff doesn't get lost.

dougwilson commented 4 weeks ago

Idk the details, but content-type and media-typer are not interchangeable, as they are for different standard. Just make sure you are using the one appropriate for whatever you are trying to parse.

dougwilson commented 4 weeks ago

Taking a quick glance at the link, it is probably because a valid content-type header doesn't mean the type itself is a valid media type. The content-type standard is pretty open and lax vs what is a media type. The media-typer module can be used to validate media types from a content-type header if you want to check is something from the content-type header is a valid media type.

wesleytodd commented 4 weeks ago

Idk the details, but content-type and media-typer are not interchangeable, as they are for different standard. Just make sure you are using the one appropriate for whatever you are trying to parse.

Yep, that was what keyed me to this in the first place, I was trying to figure out why that package was being used on the content-type header. I am pretty sure this is the right change, and the tests pass except for the ones I brought up here, so I think this is the right change.

it is probably because a valid content-type header doesn't mean the type itself is a valid media type.

Ah, right, that makes sense. So I guess maybe I could use some advice on how to address this then. The new major of media-typer fails as well (in a different way). So either I need to update that PR to go back and fix the failure, or I need to fix those tests. Since the spec is loose, I wonder if that test which is failing is just not valid.

    var req = createRequest('text/html**')
    assert.strictEqual(typeis(req, ['text/*']), false)

Seems like a case could be made this is a bad test?

dougwilson commented 4 weeks ago

You almost certainly neeed to use both modules. One to parse the content-type header and the other to validate it is a valid media type.

dougwilson commented 4 weeks ago

Like this: https://github.com/jshttp/type-is/commit/f1bc60a6f66040ff2802b4599d1ae32981c06ddb

wesleytodd commented 4 weeks ago

WOAH! Where is that commit?

wesleytodd commented 4 weeks ago

develop. Do I need to rebase develop and try to pull in your work from there?

wesleytodd commented 4 weeks ago

Ok, I am rebasing develop and merging it into 2.x now. Thanks for the pointer, this is exactly what I needed.

wesleytodd commented 4 weeks ago

ok, that resolved it! Thank you so much for the help!

dougwilson commented 4 weeks ago

No problem:) I guess yay past me for helping with a commit