Add TE header validation for HTTP/2 and HTTP/1.1 requests #498
Conversation
|
@arturobernalg It is not our job to police the users, if they manually add headers to requests. We need to ensure strict protocol conformance of headers that HttpCore protocol interceptors generate and leave those added by the users alone. The What we might do is to have strict versions of protocol processors in addition to the default ones. |
arturobernalg
force-pushed
the
te-header
branch
2 times, most recently
from
2d6f96f
to
bedfe0c
Compare
|
arturobernalg
force-pushed
the
te-header
branch
from
912b46d
to
d70fd97
Compare
@ok2c |
arturobernalg
force-pushed
the
te-header
branch
from
0bf0a8c
to
dab8246
Compare
@arturobernalg Do not worry. This is why do reviews and cross check one another's changes. |
| * @param request the HTTP request to validate | ||
| * @throws HttpException if the {@code Connection: TE} header is missing | ||
| */ | ||
| private void validateConnectionHeader(final HttpRequest request) throws HttpException { |
There was a problem hiding this comment.
@arturobernalg You can really optimize this bit by making use of MessageSupport#parseTokens. See how this is done in DefaultContentLengthStrategy
| * @throws HttpException if the {@code Connection: TE} header is missing | ||
| */ | ||
| private void validateConnectionHeader(final HttpRequest request) throws HttpException { | ||
| final Header connectionHeader = request.getFirstHeader(HttpHeaders.CONNECTION); |
There was a problem hiding this comment.
@arturobernalg Please also note it is legal for a message to have multiple Connection headers. Just looking at the first one is not good enough. I think the same applies to TE
arturobernalg
force-pushed
the
te-header
branch
2 times, most recently
from
2dcd8b5
to
925c1f6
Compare
|
@ok2c please take another look. |
| * @param teValue the value of the {@code TE} header | ||
| * @throws HttpException if the {@code TE} header contains invalid values | ||
| */ | ||
| private void validateTEField(final String teValue) throws HttpException { |
There was a problem hiding this comment.
@arturobernalg TE tokens can also be transmitted with multiple headers, like Connection.
TE: this
TE: that, trailers
Can you change this method to use MessageSupport#parseTokens?
There was a problem hiding this comment.
done.
@arturobernalg Your current approach results in an unnecessary creation of an array and a copy of the header value, which is wasteful. Please use MessageSupport#parseTokens that avoids all that.
arturobernalg
force-pushed
the
te-header
branch
from
6354916
to
0eff61b
Compare
|
@arturobernalg Please take a look ok2c@24a7c00 This is all it takes. The cost is 4 booleans and two lambdas. |
Implemented a new RequestTE interceptor for validating the TE header in HTTP/1.1 requests, ensuring proper protocol handling. Introduced strict client and server options that allow users to enable additional protocol validation checks, such as the validation of TE headers for HTTP/2 and HTTP/1.1.
arturobernalg
force-pushed
the
te-header
branch
from
2924b60
to
c236d0b
Compare
@ok2c You’re absolutely right. I’ve made the necessary changes as per your suggestion. Thanks for the feedback! |
This PR introduces two interceptors, H2RequestTE and RequestTE, to ensure the correct validation of the TE header in both HTTP/2 and HTTP/1.1 requests:
H2RequestTE: Ensures that the TE header in HTTP/2 requests only contains the trailers directive, as required by the protocol. Any other transfer coding present will result in a ProtocolException.
RequestTE: Validates the TE header in HTTP/1.1 requests by:
Ensuring the chunked transfer coding is not explicitly listed, as it is implicitly supported.
Verifying that the Connection: TE header is present to prevent TE from being forwarded by intermediaries.