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

fix: compatible extra semicolon on content-type header #51

Closed
wants to merge 1 commit into from

Conversation

fengmk2
Copy link

@fengmk2 fengmk2 commented Jun 19, 2023

No description provided.

@fengmk2
Copy link
Author

fengmk2 commented Jun 19, 2023

@dougwilson I think compatible it on type-is is better than koa level.

@dougwilson
Copy link
Contributor

Thanks! I'm not sure the spec allows a semicolon like that, I will need tk check.

@dougwilson
Copy link
Contributor

dougwilson commented Jun 19, 2023

Yea, checking out the latest RFC, even, it doesn't seem like this is allowed: https://httpwg.org/specs/rfc9110.html#parameter

Each parameter starts with a ; and then would consist of a name, = and the value. A single ; at the end like this would represent an incomplete parameter.

Can you elaborate morw on this change? Where does it fit in to the specs? I tried to look but I could be missing.

Thia module is designed to reject anything outside of the specs on purpose.

@fengmk2
Copy link
Author

fengmk2 commented Jun 19, 2023

Yes, the standard specification does not contain an extra semicolon inside. This is a feedback from a real application developer, presumably because the http client implementation that calls their service is not sufficiently standardized to cause them to think that this is a problem with the koa application framework.
Instead of fixing it here, I'll see if I can fix it at the koa or eggjs level.

@fengmk2 fengmk2 closed this Jun 19, 2023
@dougwilson
Copy link
Contributor

Ah, gotcha. I mean, I'm not 1000% against the idea, but the issue I always end up with these are just what exactly are the exceptions to the rules? Like in this case it accepts foo/bar;a=b;; but then should foo/bar;;a=b be ok too?

The question ends up being what exactly are the exceptions to the rules and how far to go until it turns around and becomes some kind of security vulnerability where multiple systems (like firewalls and proxies) and involved where they are interpreting the same request differently?

@fengmk2 fengmk2 reopened this Jun 19, 2023
@fengmk2
Copy link
Author

fengmk2 commented Jun 19, 2023

I think we just need to fix the values that end with semicolon.
foo/bar;;a=b we should not to fixed it.

@dougwilson
Copy link
Contributor

Ok, I see. What is this behavior from? Are we trying to match the behavior of a different web server or just a client? If we are to do this, I would like to know what it is to document and also match how it is doing this so we know we are making it compatible on purpose.

@dougwilson
Copy link
Contributor

dougwilson commented Jun 19, 2023

Also we'll probably want to have this as a opt-in behavor for non-standards parsing. That usually appeases the sec bug reporters. This is also what node.js has done for it's http parser (they call it insecureHTTPParser but we can just label it as like strict or quirks or something).

@dougwilson
Copy link
Contributor

dougwilson commented Jun 19, 2023

And, really, in this non-strict/quirks mode, it could just chop the string at the first semi instead of doing the work to validate the media type string is valid or not. That would probably be the most flexible and match what a lot of impls who don't validate the media type are doing (and it would work for this case here too).

@fengmk2 fengmk2 closed this Nov 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants