Allow no SP after status-code for HTTP/1.x responses #2247
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Motivation: While RFC7230, section 3.1.2 (https://datatracker.ietf.org/doc/html/rfc7230#section-3.1.2) requires a `SP` before a `reason-phrase`, even if it's an empty phrase, many implementations don't include that `SP` character. Many clients parse a response without trailing `SP` character with no error. Those implementations are: - Apache HTTP Client - JDK HttpClient - Netty - Tomcat (https://bz.apache.org/bugzilla/show_bug.cgi?id=60362) - Go HttpClient (golang/go@d71d08a#diff-0e6945dfde3e4dcaba7cfd394a85328b8137c42e152b4486dbfb6756ad69a779R145-R146) - Rust HttpClient (hyperium/http#345) - httpd with Passenger (https://bugzilla.redhat.com/show_bug.cgi?id=1032733) Moreover, neither HTTP/2 nor HTTP/3 support reason-phrase. For this reasons, many HTTP/1.x implementations (Tomcat, Rust, etc.) decided to drop including the reason-phrase in serialized response for performance reasons. Instead, they use the reason-phrase defined by RFC for all known status codes and use `Unknown` for non-standard status codes. Modifications: - `HttpObjectDecoder`: second `SP` is optional for `HttpResponseDecoder`; - Enhance tests to verify new behavior; Result: ServiceTalk HttpClient parses responses with no `SP` after status code. Example: `HTTP/1.1 200\r\n`.
tkountis
approved these changes
Jun 9, 2022
| @@ -131,19 +138,14 @@ void illegalPrefaceCharacter() { | |||
|
|
|||
| @Test | |||
| void noVersion() { | |||
| assertDecoderException("200 OK" + "\r\n", "Invalid start-line"); | |||
| assertDecoderException("200 OK" + "\r\n", "Invalid HTTP version"); | |||
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation:
While RFC7230, section 3.1.2
(https://datatracker.ietf.org/doc/html/rfc7230#section-3.1.2) requires a
SPbefore areason-phrase, even if it's an empty phrase, manyimplementations don't include that
SPcharacter. Many clients parse aresponse without trailing
SPcharacter with no error. Thoseimplementations are:
Moreover, neither HTTP/2 nor HTTP/3 support reason-phrase. For this
reasons, many HTTP/1.x implementations (Tomcat, Rust, etc.) decided to
drop including the reason-phrase in serialized response for performance
reasons. Instead, they use the reason-phrase defined by RFC for all
known status codes and use
Unknownfor non-standard status codes.Modifications:
HttpObjectDecoder: secondSPis optional forHttpResponseDecoder;Result:
ServiceTalk HttpClient parses responses with no
SPafter status code.Example:
HTTP/1.1 200\r\n.