HpackDecoder treats invalid pseudo-headers as stream level errors #8046
Conversation
bryce-anderson
force-pushed
the
issue_8043_HeaderValidationError
branch
from
3c84618
to
92bc5cc
Compare
|
|
||
| // we have read all of our headers. See if we have exceeded our maxHeaderListSize. We must | ||
| // we have read all of our headers so now perform validation steps. We must |
There was a problem hiding this comment.
this switches to the imperative mid-sentence, which feels awkward
| @@ -551,6 +556,14 @@ public void appendToHeaderList(CharSequence name, CharSequence value) { | |||
| if (!exceededMaxLength) { | |||
| headers.add(name, value); | |||
| } | |||
| // Only consider the first validation exception | |||
| if (validate && validationException == null) { | |||
There was a problem hiding this comment.
can this conditional and the one above be else if?
There was a problem hiding this comment.
I think I must be misunderstanding your suggestion. If it was an else if with the conditional above it would not validate unless exceededMaxLength == true, in which case it doesn't matter.
I was deliberate in letting the header be added even if validation was going to fail since there is a world where we would be interested in the results, but I could see wanting to refactor the checks such that we only add the header if it passes both checks.
There was a problem hiding this comment.
@bryce-anderson yeah I think only adding the header if it passes the checks would be a good idea as otherwise we will not use these anyway right ?
There was a problem hiding this comment.
In the common (maybe all) case they will just be discarded. For the case of exceededMaxLength == true it made sense not to store it since it was too big, but the invalid pseudo-header case is less obvious. I'll change it to not add if it fails either validation if for nothing else than to be consistent.
| @@ -119,24 +120,25 @@ | |||
| * This method assumes the entire header block is contained in {@code in}. | |||
| */ | |||
| public void decode(int streamId, ByteBuf in, Http2Headers headers, boolean validateHeaders) throws Http2Exception { | |||
| Http2HeadersSink sink = new Http2HeadersSink(headers, maxHeaderListSize); | |||
| decode(in, sink, validateHeaders); | |||
| Http2HeadersSink sink = new Http2HeadersSink(streamId, headers, maxHeaderListSize, validateHeaders); | |||
There was a problem hiding this comment.
I know it wasn't introduced by this PR but it would be nice to not have to introduce object allocation for each decode. Seems like this can be handled as local state in the now private decode method.
There was a problem hiding this comment.
Do you mind if we defer this for a later commit? I think the private decode method is really heavy at this time and don't really want to add more state to it, at least as part of another behavior change.
fwiw, I suspect this is a really good candidate for stack allocation and even if it's not, it's super short lived garbage that only happens once on a complete HEADERS block.
There was a problem hiding this comment.
I am +1 to defer this for a followup as its not introduced by this change
Motivation: The HTTP/2 spec dictates that invalid pseudo-headers should cause the request/response to be treated as malformed (8.1.2.1), and the recourse for that is to treat the situation as a stream error of type PROTOCOL_ERROR (8.1.2.6). However, we're treating them as a connection error with the connection being immediately torn down and the HPACK state potentially being corrupted. Modifications: The HpackDecoder now throws a StreamException for validation failures and throwing is deffered until the end of of the decode phase to ensure that the HPACK state isn't corrupted by returning early. Result: Behavior more closely aligned with the HTTP/2 spec. Fixes netty#8043.
bryce-anderson
force-pushed
the
issue_8043_HeaderValidationError
branch
from
92bc5cc
to
16301ad
Compare
|
@bryce-anderson thanks! |
Motivation:
The HTTP/2 spec dictates that invalid pseudo-headers should cause the
request/response to be treated as malformed (8.1.2.1), and the recourse
for that is to treat the situation as a stream error of type
PROTOCOL_ERROR (8.1.2.6). However, we're treating them as a connection
error with the connection being immediately torn down and the HPACK
state potentially being corrupted.
Modifications:
The HpackDecoder now throws a StreamException for validation failures
and throwing is deffered until the end of of the decode phase to ensure
that the HPACK state isn't corrupted by returning early.
Result:
Behavior more closely aligned with the HTTP/2 spec.
Fixes #8043.