Skip to content

Commit 7127e60

Browse files
idelpivnitskiyidelpivnitskiy
authored
Move validation of connection headers in HTTP/2 back to HpackDecoder (#12975)
 Motivation: #12755 added validation for presence of connection-related headers while `HpackDecoder` decodes the incoming frame. Then #12760 moved this validation from `HpackDecoder` to `DefaultHttp2Headers`. As the result, existing use-case that could use `DefaultHttp2Headers` for HTTP/2 and HTTP/1.X broke when users add any of the mentioned prohibited headers. The HTTP/1.X to HTTP/2 translation logic usually has sanitization process that removes connection-related headers. It's enough to run this validation only for incoming messages and we should preserve backward compatibility for 4.1. Modifications: - Move `isConnectionHeader` and `te` validations from `DefaultHttp2Headers` back to `HpackDecoder`; - Add tests to verify `HpackDecoder` fails incoming headers as expected; - Add tests to verify mentioned headers can be added to `DefaultHttp2Headers`; Result: Backward compatibility is preserved, while validation for connection-related headers is done in `HpackDecoder`.
1 parent ecf489f commit 7127e60

4 files changed

Lines changed: 57 additions & 47 deletions

File tree

‎codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2Headers.java‎

Lines changed: 1 addition & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -79,9 +79,6 @@ public void validateName(CharSequence name) {
7979
PlatformDependent.throwException(connectionError(
8080
PROTOCOL_ERROR, "Invalid HTTP/2 pseudo-header '%s' encountered.", name));
8181
}
82-
} else if (HttpHeaderValidationUtil.isConnectionHeader(name, true)) {
83-
PlatformDependent.throwException(connectionError(
84-
PROTOCOL_ERROR, "Illegal connection-specific header '%s' encountered.", name));
8582
}
8683
}
8784
};
@@ -176,12 +173,7 @@ protected void validateName(NameValidator<CharSequence> validator, boolean forAd
176173

177174
@Override
178175
protected void validateValue(ValueValidator<CharSequence> validator, CharSequence name, CharSequence value) {
179-
if (nameValidator() == HTTP2_NAME_VALIDATOR) {
180-
if (HttpHeaderValidationUtil.isTeNotTrailers(name, value)) {
181-
PlatformDependent.throwException(connectionError(PROTOCOL_ERROR,
182-
"Illegal value specified for the 'TE' header (only 'trailers' is allowed)."));
183-
}
184-
}
176+
// This method has a noop override for backward compatibility, see https://github.com/netty/netty/pull/12975
185177
super.validateValue(validator, name, value);
186178
}
187179

‎codec-http2/src/main/java/io/netty/handler/codec/http2/HpackDecoder.java‎

Lines changed: 10 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232
package io.netty.handler.codec.http2;
3333

3434
import io.netty.buffer.ByteBuf;
35+
import io.netty.handler.codec.http.HttpHeaderValidationUtil;
3536
import io.netty.handler.codec.http2.HpackUtil.IndexType;
3637
import io.netty.util.AsciiString;
3738

@@ -375,8 +376,8 @@ private void setDynamicTableSize(long dynamicTableSize) throws Http2Exception {
375376
hpackDynamicTable.setCapacity(dynamicTableSize);
376377
}
377378

378-
private static HeaderType validateHeader(int streamId, AsciiString name, HeaderType previousHeaderType)
379-
throws Http2Exception {
379+
private static HeaderType validateHeader(int streamId, AsciiString name, CharSequence value,
380+
HeaderType previousHeaderType) throws Http2Exception {
380381
if (hasPseudoHeaderFormat(name)) {
381382
if (previousHeaderType == HeaderType.REGULAR_HEADER) {
382383
throw streamError(streamId, PROTOCOL_ERROR,
@@ -390,42 +391,15 @@ private static HeaderType validateHeader(int streamId, AsciiString name, HeaderT
390391
}
391392
return currentHeaderType;
392393
}
393-
394-
return HeaderType.REGULAR_HEADER;
395-
}
396-
397-
private static boolean contains(Http2Headers headers, AsciiString name) {
398-
if (headers == EmptyHttp2Headers.INSTANCE) {
399-
return false;
400-
}
401-
if (headers instanceof DefaultHttp2Headers || headers instanceof ReadOnlyHttp2Headers) {
402-
return headers.contains(name);
403-
}
404-
// We can't be sure the Http2Headers implementation support contains on pseudo-headers,
405-
// so we have to use the direct accessors instead.
406-
if (Http2Headers.PseudoHeaderName.METHOD.value().equals(name)) {
407-
return headers.method() != null;
394+
if (HttpHeaderValidationUtil.isConnectionHeader(name, true)) {
395+
throw streamError(streamId, PROTOCOL_ERROR, "Illegal connection-specific header '%s' encountered.", name);
408396
}
409-
if (Http2Headers.PseudoHeaderName.SCHEME.value().equals(name)) {
410-
return headers.scheme() != null;
397+
if (HttpHeaderValidationUtil.isTeNotTrailers(name, value)) {
398+
throw streamError(streamId, PROTOCOL_ERROR,
399+
"Illegal value specified for the 'TE' header (only 'trailers' is allowed).");
411400
}
412-
if (Http2Headers.PseudoHeaderName.AUTHORITY.value().equals(name)) {
413-
return headers.authority() != null;
414-
}
415-
if (Http2Headers.PseudoHeaderName.PATH.value().equals(name)) {
416-
return headers.path() != null;
417-
}
418-
if (Http2Headers.PseudoHeaderName.STATUS.value().equals(name)) {
419-
return headers.status() != null;
420-
}
421-
// Note: We don't check PROTOCOL because the API presents no alternative way to access it.
422-
return false;
423-
}
424401

425-
private static Http2Exception illegalHeaderValue(int streamId, AsciiString name, int illegalByte, int index) {
426-
return streamError(streamId, PROTOCOL_ERROR,
427-
"Illegal header value given for header '%s': illegal byte 0x%X at index %s.",
428-
name, illegalByte, index);
402+
return HeaderType.REGULAR_HEADER;
429403
}
430404

431405
private AsciiString readName(int index) throws Http2Exception {
@@ -578,7 +552,7 @@ void appendToHeaderList(AsciiString name, AsciiString value) {
578552
try {
579553
headers.add(name, value);
580554
if (validateHeaders) {
581-
previousType = validateHeader(streamId, name, previousType);
555+
previousType = validateHeader(streamId, name, value, previousType);
582556
}
583557
} catch (IllegalArgumentException ex) {
584558
validationException = streamError(streamId, PROTOCOL_ERROR, ex,

‎codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2HeadersTest.java‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,8 @@
2020
import io.netty.util.internal.StringUtil;
2121
import org.junit.jupiter.api.Test;
2222
import org.junit.jupiter.api.function.Executable;
23+
import org.junit.jupiter.params.ParameterizedTest;
24+
import org.junit.jupiter.params.provider.CsvSource;
2325

2426
import java.util.Map.Entry;
2527

@@ -190,6 +192,15 @@ void setMustOverwritePseudoHeaders() {
190192
assertEquals(of("http"), headers.scheme());
191193
}
192194

195+
@ParameterizedTest(name = "{displayName} [{index}] name={0} value={1}")
196+
@CsvSource(value = {"upgrade,protocol1", "connection,close", "keep-alive,timeout=5", "proxy-connection,close",
197+
"transfer-encoding,chunked", "te,something-else"})
198+
void possibleToAddConnectionHeaders(String name, String value) {
199+
Http2Headers headers = newHeaders();
200+
headers.add(name, value);
201+
assertTrue(headers.contains(name, value));
202+
}
203+
193204
private static void verifyAllPseudoHeadersPresent(Http2Headers headers) {
194205
for (PseudoHeaderName pseudoName : PseudoHeaderName.values()) {
195206
assertNotNull(headers.get(pseudoName.value()));

‎codec-http2/src/test/java/io/netty/handler/codec/http2/HpackDecoderTest.java‎

Lines changed: 35 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -37,12 +37,15 @@
3737
import org.junit.jupiter.api.BeforeEach;
3838
import org.junit.jupiter.api.Test;
3939
import org.junit.jupiter.api.function.Executable;
40+
import org.junit.jupiter.params.ParameterizedTest;
41+
import org.junit.jupiter.params.provider.CsvSource;
4042
import org.mockito.MockingDetails;
4143
import org.mockito.invocation.Invocation;
4244

4345
import java.lang.reflect.Method;
4446

4547
import static io.netty.handler.codec.http2.HpackDecoder.decodeULE128;
48+
import static io.netty.handler.codec.http2.Http2Error.PROTOCOL_ERROR;
4649
import static io.netty.handler.codec.http2.Http2HeadersEncoder.NEVER_SENSITIVE;
4750
import static io.netty.util.AsciiString.EMPTY_STRING;
4851
import static io.netty.util.AsciiString.of;
@@ -765,12 +768,42 @@ public void pseudoHeaderAfterRegularHeader() throws Exception {
765768

766769
final Http2Headers decoded = new DefaultHttp2Headers();
767770

768-
assertThrows(Http2Exception.StreamException.class, new Executable() {
771+
Http2Exception.StreamException e = assertThrows(Http2Exception.StreamException.class, new Executable() {
769772
@Override
770773
public void execute() throws Throwable {
771-
hpackDecoder.decode(1, in, decoded, true);
774+
hpackDecoder.decode(3, in, decoded, true);
775+
}
776+
});
777+
assertThat(e.streamId(), is(3));
778+
assertThat(e.error(), is(PROTOCOL_ERROR));
779+
} finally {
780+
in.release();
781+
}
782+
}
783+
784+
@ParameterizedTest(name = "{displayName} [{index}] name={0} value={1}")
785+
@CsvSource(value = {"upgrade,protocol1", "connection,close", "keep-alive,timeout=5", "proxy-connection,close",
786+
"transfer-encoding,chunked", "te,something-else"})
787+
public void receivedConnectionHeader(String name, String value) throws Exception {
788+
final ByteBuf in = Unpooled.buffer(200);
789+
try {
790+
HpackEncoder hpackEncoder = new HpackEncoder(true);
791+
792+
Http2Headers toEncode = new InOrderHttp2Headers();
793+
toEncode.add(":method", "GET");
794+
toEncode.add(name, value);
795+
hpackEncoder.encodeHeaders(1, in, toEncode, NEVER_SENSITIVE);
796+
797+
final Http2Headers decoded = new DefaultHttp2Headers();
798+
799+
Http2Exception.StreamException e = assertThrows(Http2Exception.StreamException.class, new Executable() {
800+
@Override
801+
public void execute() throws Throwable {
802+
hpackDecoder.decode(3, in, decoded, true);
772803
}
773804
});
805+
assertThat(e.streamId(), is(3));
806+
assertThat(e.error(), is(PROTOCOL_ERROR));
774807
} finally {
775808
in.release();
776809
}

0 commit comments

Comments
 (0)