Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Does not throw on some incorrect headers #25

Closed
wesleytodd opened this issue Aug 31, 2024 · 10 comments
Closed

Does not throw on some incorrect headers #25

wesleytodd opened this issue Aug 31, 2024 · 10 comments

Comments

@wesleytodd
Copy link
Member

jshttp/type-is#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.

@wesleytodd wesleytodd changed the title Does not throwon some incorrect headers Does not throw on some incorrect headers Aug 31, 2024
@dougwilson
Copy link
Contributor

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
Copy link
Contributor

dougwilson commented Aug 31, 2024

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
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Contributor

Like this: jshttp/type-is@f1bc60a

@wesleytodd
Copy link
Member Author

WOAH! Where is that commit?

@wesleytodd
Copy link
Member Author

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

@wesleytodd
Copy link
Member Author

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

@wesleytodd
Copy link
Member Author

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

@dougwilson
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants