This is an automated email from the ASF dual-hosted git repository.
markt-asf pushed a commit to branch 10.1.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/10.1.x by this push:
new 259e938d3d Revert & re-implement "Add HTTP/2 header filtering and
associated tests"
259e938d3d is described below
commit 259e938d3dedf07f3b24189fd5032adb95b01f2a
Author: Mark Thomas <[email protected]>
AuthorDate: Mon Sep 7 15:22:24 2026 +0100
Revert & re-implement "Add HTTP/2 header filtering and associated tests"
While moving the validation into the HPACK decoder was marginally more
efficient, it made correct handling of any errors much more difficult as
the HACK decoder does not have access to the current validation state.
That is managed by Stream where the remaining header validation takes
place. All header validation now takes place in Stream which is much
cleaner. Some deprecated code will be "un-deprecated" as a result.
---
java/org/apache/coyote/http2/HPackHuffman.java | 77 ++--------
java/org/apache/coyote/http2/HpackDecoder.java | 35 ++---
java/org/apache/coyote/http2/Http2Parser.java | 2 -
.../apache/coyote/http2/LocalStrings.properties | 10 +-
java/org/apache/coyote/http2/Stream.java | 67 +++++++--
test/org/apache/coyote/http2/TestHPackHuffman.java | 2 +-
.../apache/coyote/http2/TestHttp2Section_4_3.java | 162 +++++++++++++++++++++
webapps/docs/changelog.xml | 5 +
8 files changed, 255 insertions(+), 105 deletions(-)
diff --git a/java/org/apache/coyote/http2/HPackHuffman.java
b/java/org/apache/coyote/http2/HPackHuffman.java
index 784f9f175c..625b457317 100644
--- a/java/org/apache/coyote/http2/HPackHuffman.java
+++ b/java/org/apache/coyote/http2/HPackHuffman.java
@@ -22,7 +22,6 @@ import java.util.HashSet;
import java.util.Locale;
import java.util.Set;
-import org.apache.tomcat.util.http.parser.HttpParser;
import org.apache.tomcat.util.res.StringManager;
/**
@@ -376,44 +375,44 @@ public class HPackHuffman {
DECODING_TABLE = codingTree;
}
+
/**
* Decodes a huffman encoded string into the target StringBuilder. There
must be enough space left in the buffer for
* this method to succeed.
*
- * @param data The byte buffer
- * @param length The length of data from the buffer to decode
- * @param target The target for the decompressed data
+ * @param data The byte buffer
+ * @param length The length of data from the buffer to decode
+ * @param target The target for the decompressed data
+ * @param isFieldName {@code true} if a field name is being decoded (names
have a more restrictive set of allowed
+ * characters than field values)
*
* @throws HpackException If the Huffman encoded value in HPACK headers
did not end with EOS padding
*
* @deprecated Will be removed in Tomcat 12. Use {@link
#decode(ByteBuffer, int, StringBuilder, boolean)}
*/
@Deprecated
- public static void decode(ByteBuffer data, int length, StringBuilder
target) throws HpackException {
- decode(data, length, target, false);
+ public static void decode(ByteBuffer data, int length, StringBuilder
target, boolean isFieldName)
+ throws HpackException {
+ decode(data, length, target);
}
+
/**
* Decodes a huffman encoded string into the target StringBuilder. There
must be enough space left in the buffer for
* this method to succeed.
*
- * @param data The byte buffer
- * @param length The length of data from the buffer to decode
- * @param target The target for the decompressed data
- * @param isFieldName {@code true} if a field name is being decoded (names
have a more restrictive set of allowed
- * characters than field values)
+ * @param data The byte buffer
+ * @param length The length of data from the buffer to decode
+ * @param target The target for the decompressed data
*
* @throws HpackException If the Huffman encoded value in HPACK headers
did not end with EOS padding
*/
- public static void decode(ByteBuffer data, int length, StringBuilder
target, boolean isFieldName)
- throws HpackException {
+ public static void decode(ByteBuffer data, int length, StringBuilder
target) throws HpackException {
assert data.remaining() >= length;
int treePos = 0;
boolean eosBits = true;
int eosBitCount = 0;
- boolean firstChar = true;
- char c = 'a';
for (int i = 0; i < length; ++i) {
byte b = data.get();
int bitPos = 7;
@@ -427,27 +426,7 @@ public class HPackHuffman {
// Found a zero, can't be counting EOS bits
eosBitCount = 0;
} else {
- c = (char) (val & LOW_MASK);
- if (isFieldName) {
- if (!HttpParser.isToken(c) ||
Character.isUpperCase(c)) {
- throw new IllegalArgumentException(sm
-
.getString("hpackhuffman.decode.illegalCharacterName", Character.toString(c)));
- }
- } else {
- if (firstChar) {
- if (!HttpParser.isFieldVChar(c)) {
- throw new
IllegalArgumentException(sm.getString(
-
"hpackhuffman.decode.illegalCharacterValue.start", Character.toString(c)));
- }
- firstChar = false;
- } else {
- if (!HttpParser.isFieldContent(c)) {
- throw new
IllegalArgumentException(sm.getString(
-
"hpackhuffman.decode.illegalCharacterValue", Character.toString(c)));
- }
- }
- }
- target.append(c);
+ target.append((char) (val & LOW_MASK));
treePos = 0;
eosBits = true;
// Output a character, reset eosBitCount
@@ -466,27 +445,7 @@ public class HPackHuffman {
// as an error
throw new
HpackException(sm.getString("hpackhuffman.stringLiteralEOS"));
}
- c = (char) ((val >> 16) & LOW_MASK);
- if (isFieldName) {
- if (!HttpParser.isToken(c) ||
Character.isUpperCase(c)) {
- throw new IllegalArgumentException(sm
-
.getString("hpackhuffman.decode.illegalCharacterName", Character.toString(c)));
- }
- } else {
- if (firstChar) {
- if (!HttpParser.isFieldVChar(c)) {
- throw new
IllegalArgumentException(sm.getString(
-
"hpackhuffman.decode.illegalCharacterValue.start", Character.toString(c)));
- }
- firstChar = false;
- } else {
- if (!HttpParser.isFieldContent(c)) {
- throw new
IllegalArgumentException(sm.getString(
-
"hpackhuffman.decode.illegalCharacterValue", Character.toString(c)));
- }
- }
- }
- target.append(c);
+ target.append((char) ((val >> 16) & LOW_MASK));
treePos = 0;
eosBits = true;
// Output a character, reset eosBitCount
@@ -502,10 +461,6 @@ public class HPackHuffman {
if (!eosBits) {
throw new
HpackException(sm.getString("hpackhuffman.huffmanEncodedHpackValueDidNotEndWithEOS"));
}
- if (!isFieldName && !HttpParser.isFieldVChar(c)) {
- throw new IllegalArgumentException(
-
sm.getString("hpackhuffman.decode.illegalCharacterValue.end",
Character.toString(c)));
- }
}
diff --git a/java/org/apache/coyote/http2/HpackDecoder.java
b/java/org/apache/coyote/http2/HpackDecoder.java
index b914523293..5e7fd071bc 100644
--- a/java/org/apache/coyote/http2/HpackDecoder.java
+++ b/java/org/apache/coyote/http2/HpackDecoder.java
@@ -21,7 +21,6 @@ import java.util.concurrent.atomic.AtomicReference;
import org.apache.juli.logging.Log;
import org.apache.juli.logging.LogFactory;
-import org.apache.tomcat.util.http.parser.HttpParser;
import org.apache.tomcat.util.res.StringManager;
/**
@@ -116,7 +115,7 @@ public class HpackDecoder {
buffer.position(originalPos);
return;
}
- String headerValue = readHpackString(buffer, false);
+ String headerValue = readHpackString(buffer);
if (headerValue == null) {
buffer.position(originalPos);
return;
@@ -130,7 +129,7 @@ public class HpackDecoder {
buffer.position(originalPos);
return;
}
- String headerValue = readHpackString(buffer, false);
+ String headerValue = readHpackString(buffer);
if (headerValue == null) {
buffer.position(originalPos);
return;
@@ -143,7 +142,7 @@ public class HpackDecoder {
buffer.position(originalPos);
return;
}
- String headerValue = readHpackString(buffer, false);
+ String headerValue = readHpackString(buffer);
if (headerValue == null) {
buffer.position(originalPos);
return;
@@ -205,11 +204,11 @@ public class HpackDecoder {
} else if (index != 0) {
return handleIndexedHeaderName(index);
} else {
- return readHpackString(buffer, true);
+ return readHpackString(buffer);
}
}
- private String readHpackString(ByteBuffer buffer, boolean isFieldName)
throws HpackException {
+ private String readHpackString(ByteBuffer buffer) throws HpackException {
if (!buffer.hasRemaining()) {
return null;
}
@@ -221,34 +220,18 @@ public class HpackDecoder {
}
boolean huffman = (data & 0b10000000) != 0;
if (huffman) {
- return readHuffmanString(length, buffer, isFieldName);
+ return readHuffmanString(length, buffer);
}
StringBuilder stringBuilder = new StringBuilder(length);
for (int i = 0; i < length; ++i) {
- char c = (char) (buffer.get() & 0xFF);
- if (isFieldName) {
- if (HttpParser.isToken(c) && !Character.isUpperCase(c)) {
- stringBuilder.append(c);
- } else {
- throw new IllegalArgumentException(
- sm.getString("hpackdecoder.illegalCharacterName",
Character.toString(c)));
- }
- } else {
- if ((i == 0 || i == length - 1) && HttpParser.isFieldVChar(c)
||
- i > 0 && i < length - 1 &&
HttpParser.isFieldContent(c)) {
- stringBuilder.append(c);
- } else {
- throw new IllegalArgumentException(
- sm.getString("hpackdecoder.illegalCharacterValue",
Character.toString(c)));
- }
- }
+ stringBuilder.append((char) (buffer.get() & 0xFF));
}
return stringBuilder.toString();
}
- private String readHuffmanString(int length, ByteBuffer buffer, boolean
isFieldName) throws HpackException {
+ private String readHuffmanString(int length, ByteBuffer buffer) throws
HpackException {
StringBuilder stringBuilder = new StringBuilder(length);
- HPackHuffman.decode(buffer, length, stringBuilder, isFieldName);
+ HPackHuffman.decode(buffer, length, stringBuilder);
return stringBuilder.toString();
}
diff --git a/java/org/apache/coyote/http2/Http2Parser.java
b/java/org/apache/coyote/http2/Http2Parser.java
index 8b26c99bbe..f389e3c670 100644
--- a/java/org/apache/coyote/http2/Http2Parser.java
+++ b/java/org/apache/coyote/http2/Http2Parser.java
@@ -570,8 +570,6 @@ class Http2Parser {
} catch (HpackException hpe) {
throw new
ConnectionException(sm.getString("http2Parser.processFrameHeaders.decodingFailed"),
Http2Error.COMPRESSION_ERROR, hpe);
- } catch (IllegalArgumentException iae) {
- throw new StreamException("Invalid headers",
Http2Error.PROTOCOL_ERROR, streamId, iae);
}
// switches to write mode
diff --git a/java/org/apache/coyote/http2/LocalStrings.properties
b/java/org/apache/coyote/http2/LocalStrings.properties
index 028f26fb25..d59fd26a68 100644
--- a/java/org/apache/coyote/http2/LocalStrings.properties
+++ b/java/org/apache/coyote/http2/LocalStrings.properties
@@ -46,8 +46,6 @@ hpackdecoder.addDynamic=Adding header to index [{0}] of
dynamic table with name
hpackdecoder.clearDynamic=Emptying dynamic table
hpackdecoder.emitHeader=Emitting header with name [{0}] and value [{1}]
hpackdecoder.headerTableIndexInvalid=The header table index [{0}] is not valid
as there are [{1}] static entries and [{2}] dynamic entries
-hpackdecoder.illegalCharacterName=The illegal [{0}] character was found when
decoding an HTTP/2 header field name
-hpackdecoder.illegalCharacterValue=The illegal [{0}] character was found when
decoding an HTTP/2 header field value
hpackdecoder.maxMemorySizeExceeded=The header table size [{0}] exceeds the
maximum size [{1}]
hpackdecoder.notImplemented=Not yet implemented
hpackdecoder.nullHeader=Null header at index [{0}]
@@ -56,10 +54,6 @@ hpackdecoder.useDynamic=Using header from index [{0}] of
dynamic table
hpackdecoder.useStatic=Using header from index [{0}] of static table
hpackdecoder.zeroNotValidHeaderTableIndex=Zero is not a valid header table
index
-hpackhuffman.decode.illegalCharacterName=The illegal [{0}] character was found
when decoding an HTTP/2 header field name
-hpackhuffman.decode.illegalCharacterValue=The illegal [{0}] character was
found when decoding an HTTP/2 header field value
-hpackhuffman.decode.illegalCharacterValue.end=The illegal [{0}] character was
found when decoding the final character in an HTTP/2 header field value
-hpackhuffman.decode.illegalCharacterValue.start=The illegal [{0}] character
was found when decoding the first character in an HTTP/2 header field value
hpackhuffman.huffmanEncodedHpackValueDidNotEndWithEOS=Huffman encoded value in
HPACK headers did not end with EOS padding
hpackhuffman.stringLiteralEOS=Huffman encoded value in HPACK headers contained
the EOS symbol
hpackhuffman.stringLiteralTooMuchPadding=More than 7 bits of EOS padding were
provided at the end of an Huffman encoded string literal
@@ -108,11 +102,15 @@ stream.header.empty=Connection [{0}], Stream [{1}],
Invalid empty header name
stream.header.inconsistentScheme=Connection [{0}], Stream [{1}], The scheme
[{2}] is not consistent with the TLS enabled setting of [{3}]
stream.header.invalid=Connection [{0}], Stream [{1}], The header [{2}]
contained invalid value [{3}]
stream.header.invalidConnect=Connection [{0}], Stream [{1}], The CONNECT
request was invalid as neither :scheme nor :path should be present
+stream.header.name.invalidCharacter=Connection [{0}], Stream [{1}], The
illegal character [{2}] was found when decoding an HTTP/2 header field name
[{3}]
stream.header.noPath=Connection [{0}], Stream [{1}], The [:path] pseudo header
was empty
stream.header.required=Connection [{0}], Stream [{1}], One or more required
headers was missing
stream.header.te=Connection [{0}], Stream [{1}], HTTP header [te] is not
permitted to have the value [{2}] in an HTTP/2 request
stream.header.unexpectedPseudoHeader=Connection [{0}], Stream [{1}], Pseudo
header [{2}] received after a regular header
stream.header.unknownPseudoHeader=Connection [{0}], Stream [{1}], Unknown
pseudo header [{2}] received
+stream.header.value.invalidCharacter=Connection [{0}], Stream [{1}], The
illegal character [{2}] was found when decoding an HTTP/2 header field value
[{3}]
+stream.header.value.invalidCharacter.end=Connection [{0}], Stream [{1}], The
illegal final character [{2}] was found when decoding an HTTP/2 header field
value [{3}]
+stream.header.value.invalidCharacter.start=Connection [{0}], Stream [{1}], The
illegal first character [{2}] was found when decoding an HTTP/2 header field
value [{3}]
stream.host.inconsistent=Connection [{0}], Stream [{1}], The host header [{2}]
is inconsistent with previously provided values for host [{3}] and/or port [{4}]
stream.host.sni=Connection [{0}], Stream [{1}], The host header [{2}] does not
match the SNI host [{3}]
stream.inputBuffer.copy=Copying [{0}] bytes from inBuffer to outBuffer
diff --git a/java/org/apache/coyote/http2/Stream.java
b/java/org/apache/coyote/http2/Stream.java
index 33b223333d..9bbcb1c62c 100644
--- a/java/org/apache/coyote/http2/Stream.java
+++ b/java/org/apache/coyote/http2/Stream.java
@@ -52,6 +52,7 @@ import org.apache.tomcat.util.http.HeaderUtil;
import org.apache.tomcat.util.http.Method;
import org.apache.tomcat.util.http.MimeHeaders;
import org.apache.tomcat.util.http.parser.Host;
+import org.apache.tomcat.util.http.parser.HttpParser;
import org.apache.tomcat.util.http.parser.Priority;
import org.apache.tomcat.util.net.ApplicationBufferHandler;
import org.apache.tomcat.util.net.WriteBuffer;
@@ -331,7 +332,63 @@ class Stream extends AbstractNonZeroStream implements
HeaderEmitter {
log.trace(sm.getString("stream.header.debug", getConnectionId(),
getIdAsString(), name, value));
}
- // Field header names being all lower case is enforced in HpackDecoder.
+ // Validate field name
+ if (name.isEmpty()) {
+ headerException =
+ new StreamException(sm.getString("stream.header.empty",
getConnectionId(), getIdAsString()),
+ Http2Error.PROTOCOL_ERROR, getIdAsInt());
+ // No need for further processing. The stream will be reset.
+ return;
+ }
+ for (int i = 0; i < name.length(); i++) {
+ char c = name.charAt(i);
+ // Skip pseudo headers
+ if (i == 0 && c == ':') {
+ continue;
+ }
+ if (!HttpParser.isToken(c) || Character.isUpperCase(c)) {
+ headerException =
+ new
StreamException(sm.getString("stream.header.name.invalidCharacter",
getConnectionId(),
+ getIdAsString(), Character.toString(c), name),
Http2Error.PROTOCOL_ERROR, getIdAsInt());
+ // No need for further processing. The stream will be reset.
+ return;
+ }
+ }
+
+ // Validate field value
+ for (int i = 0; i < value.length(); i++) {
+ char c = value.charAt(i);
+ if (i == 0) {
+ if (!HttpParser.isFieldVChar(c)) {
+ headerException = new StreamException(
+
sm.getString("stream.header.value.invalidCharacter.start", getConnectionId(),
+ getIdAsString(), Character.toString(c),
value),
+ Http2Error.PROTOCOL_ERROR, getIdAsInt());
+ // No need for further processing. The stream will be
reset.
+ return;
+ }
+ } else if (i == value.length() - 1) {
+ if (!HttpParser.isFieldVChar(c)) {
+ headerException = new StreamException(
+
sm.getString("stream.header.value.invalidCharacter.end", getConnectionId(),
+ getIdAsString(), Character.toString(c),
value),
+ Http2Error.PROTOCOL_ERROR, getIdAsInt());
+ // No need for further processing. The stream will be
reset.
+ return;
+ }
+ } else {
+ if (!HttpParser.isFieldContent(c)) {
+ headerException =
+ new StreamException(
+
sm.getString("stream.header.value.invalidCharacter", getConnectionId(),
+ getIdAsString(),
Character.toString(c), value),
+ Http2Error.PROTOCOL_ERROR, getIdAsInt());
+ // No need for further processing. The stream will be
reset.
+ return;
+ }
+ }
+ }
+
if (HTTP_CONNECTION_SPECIFIC_HEADERS.contains(name)) {
headerException = new StreamException(
@@ -357,14 +414,6 @@ class Stream extends AbstractNonZeroStream implements
HeaderEmitter {
return;
}
- if (name.isEmpty()) {
- headerException =
- new StreamException(sm.getString("stream.header.empty",
getConnectionId(), getIdAsString()),
- Http2Error.PROTOCOL_ERROR, getIdAsInt());
- // No need for further processing. The stream will be reset.
- return;
- }
-
boolean pseudoHeader = name.charAt(0) == ':';
if (pseudoHeader && headerState != HEADER_STATE_PSEUDO) {
diff --git a/test/org/apache/coyote/http2/TestHPackHuffman.java
b/test/org/apache/coyote/http2/TestHPackHuffman.java
index 1dc457c506..5aaf6ac8f9 100644
--- a/test/org/apache/coyote/http2/TestHPackHuffman.java
+++ b/test/org/apache/coyote/http2/TestHPackHuffman.java
@@ -38,7 +38,7 @@ public class TestHPackHuffman {
buf.get();
StringBuilder target = new StringBuilder();
- HPackHuffman.decode(buf, buf.remaining(), target, false);
+ HPackHuffman.decode(buf, buf.remaining(), target);
Assert.assertEquals("Value changed after encode/decode roundtrip",
data, target.toString());
}
diff --git a/test/org/apache/coyote/http2/TestHttp2Section_4_3.java
b/test/org/apache/coyote/http2/TestHttp2Section_4_3.java
index 0fbcfa5c35..48798b3cd1 100644
--- a/test/org/apache/coyote/http2/TestHttp2Section_4_3.java
+++ b/test/org/apache/coyote/http2/TestHttp2Section_4_3.java
@@ -17,16 +17,23 @@
package org.apache.coyote.http2;
import java.nio.ByteBuffer;
+import java.util.ArrayList;
+import java.util.List;
import org.junit.Assert;
import org.junit.Test;
+import org.apache.tomcat.util.http.Method;
+
/**
* Unit tests for Section 4.3 of <a
href="https://tools.ietf.org/html/rfc7540";>RFC 7540</a>. <br>
* The order of tests in this class is aligned with the order of the
requirements in the RFC.
*/
public class TestHttp2Section_4_3 extends Http2TestBase {
+ private static final String MARKER_HEADER_NAME = "x-marker";
+ private static final String MARKER_HEADER_VALUE = "marker-value";
+
@Test
public void testHeaderDecodingError() throws Exception {
// HTTP2 upgrade
@@ -47,6 +54,160 @@ public class TestHttp2Section_4_3 extends Http2TestBase {
}
+ /*
+ * HTTP/2 field validation rejects invalid characters in field names and
field values. The field name check has two
+ * distinct ways to fail (not a token character; a valid but upper case
token character) and the field value check
+ * has three distinct ways to fail depending on the position of the
invalid character (first, middle, last). Each
+ * of those five triggers has tests below for both the non-Huffman and
Huffman code paths. HpackEncoder uses Huffman
+ * encoding for strings longer than five characters, provided that the
encoded form is not longer than the original.
+ */
+
+ @Test
+ public void testHeaderDecodingErrorFieldNameInvalidCharacter() throws
Exception {
+ // ':' is a separator so is not a valid token character.
+ doTestHeaderDecodingErrorKeepsTableInSync("x:y", "ok");
+ }
+
+
+ @Test
+ public void testHeaderDecodingErrorFieldNameUpperCase() throws Exception {
+ // Upper case letters are valid token characters but are not permitted
in HTTP/2 header field names.
+ doTestHeaderDecodingErrorKeepsTableInSync("Xy", "ok");
+ }
+
+
+ @Test
+ public void testHeaderDecodingErrorFieldValueLeadingInvalidCharacter()
throws Exception {
+ // The first character of a field value must be a field-vchar. Space
is not.
+ doTestHeaderDecodingErrorKeepsTableInSync("x-bad", " x");
+ }
+
+
+ @Test
+ public void testHeaderDecodingErrorFieldValueTrailingInvalidCharacter()
throws Exception {
+ // The last character of a field value must be a field-vchar. Space is
not.
+ doTestHeaderDecodingErrorKeepsTableInSync("x-bad", "x ");
+ }
+
+
+ @Test
+ public void testHeaderDecodingErrorFieldValueMiddleInvalidCharacter()
throws Exception {
+ // A character in the middle of a field value must be field-content. A
control character is not.
+ doTestHeaderDecodingErrorKeepsTableInSync("x-bad", "a\u0001b");
+ }
+
+
+ @Test
+ public void testHeaderDecodingErrorHuffmanFieldNameTrailingSpace() throws
Exception {
+ // Space is not a valid token character in a field name.
+ doTestHeaderDecodingErrorKeepsTableInSync("x-bad ", "ok");
+ }
+
+
+ @Test
+ public void testHeaderDecodingErrorHuffmanFieldNameInvalidCharacter()
throws Exception {
+ // ':' is a separator so is not a valid token character.
+ doTestHeaderDecodingErrorKeepsTableInSync("x-te:st", "ok");
+ }
+
+
+ @Test
+ public void testHeaderDecodingErrorHuffmanFieldNameUpperCase() throws
Exception {
+ // Upper case letters are valid token characters but are not permitted
in HTTP/2 header field names.
+ doTestHeaderDecodingErrorKeepsTableInSync("x-Te-st", "ok");
+ }
+
+
+ @Test
+ public void
testHeaderDecodingErrorHuffmanFieldValueLeadingInvalidCharacter() throws
Exception {
+ // The first character of a field value must be a field-vchar. Space
is not.
+ doTestHeaderDecodingErrorKeepsTableInSync("x-bad", " x-value");
+ }
+
+
+ @Test
+ public void
testHeaderDecodingErrorHuffmanFieldValueTrailingInvalidCharacter() throws
Exception {
+ // The last character of a field value must be a field-vchar. Space is
not.
+ doTestHeaderDecodingErrorKeepsTableInSync("x-bad", "x-value ");
+ }
+
+
+ @Test
+ public void
testHeaderDecodingErrorHuffmanFieldValueMiddleInvalidCharacter() throws
Exception {
+ // A character in the middle of a field value must be field-content. A
control character is not.
+ doTestHeaderDecodingErrorKeepsTableInSync("x-bad",
"x-value-test-\u0001-value-test");
+ }
+
+
+ /**
+ * A header field ({@code badHeaderName} / {@code badHeaderValue}) that
{@link Stream#emitHeader(String, String)}
+ * rejects is a stream error (RFC 9113, section 8.2.1), not a connection
error, so the connection must remain open
+ * and usable. However, the invalid field is followed, in the same header
block, by another header
+ * ({@link #MARKER_HEADER_NAME}) that adds an entry to the HPACK dynamic
table. The decoder must still process that
+ * later header (i.e. keep going after detecting the invalid field) so its
dynamic table stays in sync with the
+ * encoder used by the client. This is verified with a second, otherwise
unrelated, request that references that
+ * dynamic table entry.
+ *
+ * @param badHeaderName The (possibly invalid) name to use for the
invalid header
+ * @param badHeaderValue The (possibly invalid) value to use for the
invalid header
+ */
+ private void doTestHeaderDecodingErrorKeepsTableInSync(String
badHeaderName, String badHeaderValue)
+ throws Exception {
+ // HTTP2 upgrade
+ http2Connect();
+
+ byte[] frameHeader = new byte[9];
+ ByteBuffer headersPayload = ByteBuffer.allocate(128);
+
+ List<Header> headers = new ArrayList<>(6);
+ headers.add(new Header(":method", Method.GET));
+ headers.add(new Header(":scheme", "http"));
+ headers.add(new Header(":path", "/simple"));
+ headers.add(new Header(":authority", "localhost:" + getPort()));
+ headers.add(new Header(badHeaderName, badHeaderValue));
+ // Valid header that follows the invalid one in the same header
+ // block. It must still be added to the dynamic table.
+ headers.add(new Header(MARKER_HEADER_NAME, MARKER_HEADER_VALUE));
+
+ buildGetRequest(frameHeader, headersPayload, null, headers, 3);
+ writeFrame(frameHeader, headersPayload);
+
+ // The stream must be reset. The connection must remain open.
+ parser.readFrame();
+ Assert.assertEquals("Stream (not connection) error expected for an
invalid header field",
+ "3-RST-[" + Http2Error.PROTOCOL_ERROR.getCode() + "]\n",
output.getTrace());
+ output.clearTrace();
+
+ // A second, unrelated request on a new stream. Because it uses the
+ // same name/value pair as the marker header above, the test's
+ // HpackEncoder (correctly simulating a real HTTP/2 client) will
+ // reference its dynamic table entry rather than re-sending it as a
+ // literal. This will only decode correctly on the server if the
+ // HPACK decoder kept processing the earlier header block far enough
+ // to add that entry to its own dynamic table, despite the stream
+ // being reset.
+ byte[] frameHeader2 = new byte[9];
+ ByteBuffer headersPayload2 = ByteBuffer.allocate(128);
+
+ List<Header> headers2 = new ArrayList<>(5);
+ headers2.add(new Header(":method", Method.GET));
+ headers2.add(new Header(":scheme", "http"));
+ headers2.add(new Header(":path", "/simple"));
+ headers2.add(new Header(":authority", "localhost:" + getPort()));
+ headers2.add(new Header(MARKER_HEADER_NAME, MARKER_HEADER_VALUE));
+
+ buildGetRequest(frameHeader2, headersPayload2, null, headers2, 5);
+ writeFrame(frameHeader2, headersPayload2);
+
+ parser.readFrame();
+ Assert.assertFalse(output.getTrace(),
output.getTrace().contains("RST"));
+ parser.readFrame();
+
+ Assert.assertEquals("HPACK dynamic table should still be in sync with
the client",
+ getSimpleResponseTrace(5), output.getTrace());
+ }
+
+
@Test
public void testHeaderContinuationContiguous() throws Exception {
// HTTP2 upgrade
@@ -65,6 +226,7 @@ public class TestHttp2Section_4_3 extends Http2TestBase {
// headers, body
parser.readFrame();
+ Assert.assertFalse(output.getTrace(),
output.getTrace().contains("RST"));
parser.readFrame();
Assert.assertEquals(getSimpleResponseTrace(3), output.getTrace());
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 53543b7a62..d159753fc9 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -260,6 +260,11 @@
<fix>
Additional clean-up after HTTP/2 stream reset to aid GC. (markt)
</fix>
+ <fix>
+ Revert earlier refactoring of HTTP/2 header field validation that moved
+ it earlier since the refactoring made correct handling of invalid
+ headers more difficult. (markt)
+ </fix>
</changelog>
</subsection>
<subsection name="Jasper">
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
