-
Notifications
You must be signed in to change notification settings - Fork 787
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
Make Status line parsing less strict #7104
base: series/0.23
Are you sure you want to change the base?
Make Status line parsing less strict #7104
Conversation
"HTTP/1.1 200\r\n", | ||
"Content-Type: text/plain\r\n", | ||
"Content-Length: 5\r\n\r\n", | ||
"helloHTTP/1.1 200\r\n", // this is the crucial part |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the https://datatracker.ietf.org/doc/html/rfc7230#section-3.1.2
status-line = HTTP-version SP status-code SP reason-phrase CRLF
If I'm not wrong, the whitespace char placed before the reason phrase is mandatory to be. But the reason phrase indeed may be empty, it's a good catch!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Damn you're right, this SP is mandatory.
So Ember is correctly parsing status lines, there is no issue here. But I think some servers might remove this SP in their HTTP response when they don't add a reason phrase since there would be nothing left to separate. Having something less strict on this point might be a good thing WDYT ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think some servers might remove this SP in their HTTP response since there is nothing to separate
That's a very controversial thing, to be honest. From my perspective, we should align with the spec when things are defined univocally. But maybe other folks over http4s have another opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the run at the moment, but it's Postel's Law, for it's pros and cons.
Questions I consider when we diverge:
- Is there a security implication to relaxing it?
- Do other major implementations accept it? i.e., are we converging toward a de facto spec?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me be the responder to Ross's questions.
Is there a security implication to relaxing it?
Definitely, it isn't.
Do other major implementations accept it?
I neither have a counterexample of this behaviour. Have seen in the wild only strict behaviour.
are we converging toward a de facto spec?
Our implementation is strict, but I am confident it's aligned with the spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are far more seasoned folks to speak about that, but it generally reminds me of the question of parsing filenames in Multipart where we also have strict behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Do other major implementations accept it? i.e., are we converging toward a de facto spec?
@jcchevalier-ledger the one's I'd be interested in are if it works with the JDK http client and if it works with Netty. If neither of those implementations accepts this, then it seems like a bug report for the server :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's definitely a server bug, and I'd report it as such if you have a contact. As for us, we are granted discretion:
Unless noted otherwise, a recipient MAY attempt to recover a usable protocol element from an invalid construct. HTTP does not define specific error handling mechanisms except when they have a direct impact on security, since different applications of the protocol require different error handling strategies. For example, a Web browser might wish to transparently recover from a response where the Location header field doesn't parse according to the ABNF, whereas a systems control client might consider any form of error recovery to be dangerous.
There is clear benefit to a user interoperating with a non-compliant implementation: @jcchevalier-ledger has an itch, and this scratches it at minimal cost to the rest of us. The slippery slope is that as any of us add workarounds, the burden gradually shifts from non-compliant implementations to everyone else implementing undocumented, de facto protocols in violation of the RFC. At the bottom of a similar slippery slope, we find browser engines trying to digest decades of HTML abuse... and Google's increasingly being the only one that "works".
This is why I ask what the bigger implementations do. Sometimes reality diverges from the RFC. We've made exceptions on cookies. For the health of HTTP, I try hard to not be the first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello guys !
Well we started to have this error when we switched the HTTP client of our application from sttp fs2 client to ember, so I guess the sttp fs2 (which if I'm not mistaken relies on Netty) handles this non-compliant behavior, (that's why I though it was an ember bug at first).
also added some comments
fixes #7103
see comments