From 5d2f67ce0be311399ae0ea984521b41dedcffde5 Mon Sep 17 00:00:00 2001 From: Scott Mitchell Date: Thu, 3 Dec 2015 16:48:12 -0800 Subject: [PATCH] HttpConversionUtil does not account for COOKIE compression Motivation: The HTTP/2 RFC allows for COOKIE values to be split into individual header elements to get more benefit from compression (https://tools.ietf.org/html/rfc7540#section-8.1.2.5). HttpConversionUtil was not accounting for this behavior. Modifications: - Modify HttpConversionUtil to support compressing and decompressing the COOKIE values Result: HttpConversionUtil is compatible with https://tools.ietf.org/html/rfc7540#section-8.1.2.5) Fixes https://github.com/netty/netty/issues/4457 --- .../codec/http2/DefaultHttp2Headers.java | 23 +++++-- .../codec/http2/HttpConversionUtil.java | 64 ++++++++++++----- .../HttpToHttp2ConnectionHandlerTest.java | 21 ++++++ .../http2/InboundHttp2ToHttpAdapterTest.java | 69 +++++++++++++++++++ .../main/java/io/netty/util/AsciiString.java | 2 +- .../java/io/netty/util/ByteProcessor.java | 10 +++ .../io/netty/util/AsciiStringMemoryTest.java | 25 +++++++ 7 files changed, 192 insertions(+), 22 deletions(-) diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2Headers.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2Headers.java index 5b54fd6187..63bb561787 100644 --- a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2Headers.java +++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2Headers.java @@ -14,17 +14,16 @@ */ package io.netty.handler.codec.http2; +import static io.netty.handler.codec.http2.Http2Error.PROTOCOL_ERROR; +import static io.netty.handler.codec.http2.Http2Exception.connectionError; +import static io.netty.util.AsciiString.CASE_SENSITIVE_HASHER; +import static io.netty.util.AsciiString.isUpperCase; import io.netty.handler.codec.CharSequenceValueConverter; import io.netty.handler.codec.DefaultHeaders; import io.netty.util.AsciiString; import io.netty.util.ByteProcessor; import io.netty.util.internal.PlatformDependent; -import static io.netty.handler.codec.http2.Http2Error.PROTOCOL_ERROR; -import static io.netty.handler.codec.http2.Http2Exception.connectionError; -import static io.netty.util.AsciiString.CASE_SENSITIVE_HASHER; -import static io.netty.util.AsciiString.isUpperCase; - public class DefaultHttp2Headers extends DefaultHeaders implements Http2Headers { private static final ByteProcessor HTTP2_NAME_VALIDATOR_PROCESSOR = new ByteProcessor() { @@ -113,6 +112,20 @@ public class DefaultHttp2Headers return super.clear(); } + @Override + public boolean equals(Object o) { + if (!(o instanceof Http2Headers)) { + return false; + } + + return equals((Http2Headers) o, CASE_SENSITIVE_HASHER); + } + + @Override + public int hashCode() { + return hashCode(CASE_SENSITIVE_HASHER); + } + @Override public Http2Headers method(CharSequence value) { set(PseudoHeaderName.METHOD.value(), value); diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/HttpConversionUtil.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/HttpConversionUtil.java index e4d7e94d37..f9e6429bfb 100644 --- a/codec-http2/src/main/java/io/netty/handler/codec/http2/HttpConversionUtil.java +++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/HttpConversionUtil.java @@ -14,6 +14,18 @@ */ package io.netty.handler.codec.http2; +import static io.netty.handler.codec.http.HttpScheme.HTTP; +import static io.netty.handler.codec.http.HttpScheme.HTTPS; +import static io.netty.handler.codec.http.HttpUtil.isAsteriskForm; +import static io.netty.handler.codec.http.HttpUtil.isOriginForm; +import static io.netty.handler.codec.http2.Http2Error.PROTOCOL_ERROR; +import static io.netty.handler.codec.http2.Http2Exception.connectionError; +import static io.netty.handler.codec.http2.Http2Exception.streamError; +import static io.netty.util.AsciiString.EMPTY_STRING; +import static io.netty.util.ByteProcessor.FIND_SEMI_COLON; +import static io.netty.util.internal.ObjectUtil.checkNotNull; +import static io.netty.util.internal.StringUtil.isNullOrEmpty; +import static io.netty.util.internal.StringUtil.length; import io.netty.handler.codec.http.DefaultFullHttpRequest; import io.netty.handler.codec.http.DefaultFullHttpResponse; import io.netty.handler.codec.http.FullHttpMessage; @@ -34,18 +46,6 @@ import io.netty.util.AsciiString; import java.net.URI; import java.util.Map.Entry; -import static io.netty.handler.codec.http.HttpScheme.HTTP; -import static io.netty.handler.codec.http.HttpScheme.HTTPS; -import static io.netty.handler.codec.http.HttpUtil.isAsteriskForm; -import static io.netty.handler.codec.http.HttpUtil.isOriginForm; -import static io.netty.handler.codec.http2.Http2Error.PROTOCOL_ERROR; -import static io.netty.handler.codec.http2.Http2Exception.connectionError; -import static io.netty.handler.codec.http2.Http2Exception.streamError; -import static io.netty.util.AsciiString.EMPTY_STRING; -import static io.netty.util.internal.ObjectUtil.checkNotNull; -import static io.netty.util.internal.StringUtil.isNullOrEmpty; -import static io.netty.util.internal.StringUtil.length; - /** * Provides utility methods and constants for the HTTP/2 to HTTP conversion */ @@ -327,9 +327,33 @@ public final class HttpConversionUtil { final AsciiString aName = AsciiString.of(entry.getKey()).toLowerCase(); if (!HTTP_TO_HTTP2_HEADER_BLACKLIST.contains(aName)) { // https://tools.ietf.org/html/rfc7540#section-8.1.2.2 makes a special exception for TE - if (!aName.contentEqualsIgnoreCase(HttpHeaderNames.TE) || - AsciiString.contentEqualsIgnoreCase(entry.getValue(), HttpHeaderValues.TRAILERS)) { - out.add(aName, AsciiString.of(entry.getValue())); + if (aName.contentEqualsIgnoreCase(HttpHeaderNames.TE) && + !AsciiString.contentEqualsIgnoreCase(entry.getValue(), HttpHeaderValues.TRAILERS)) { + throw new IllegalArgumentException("Invalid value for " + HttpHeaderNames.TE + ": " + + entry.getValue()); + } + if (aName.contentEqualsIgnoreCase(HttpHeaderNames.COOKIE)) { + AsciiString value = AsciiString.of(entry.getValue()); + // split up cookies to allow for better compression + // https://tools.ietf.org/html/rfc7540#section-8.1.2.5 + int index = value.forEachByte(FIND_SEMI_COLON); + if (index != -1) { + int start = 0; + do { + out.add(HttpHeaderNames.COOKIE, value.subSequence(start, index, false)); + // skip 2 characters "; " (see https://tools.ietf.org/html/rfc6265#section-4.2.1) + start = index + 2; + } while (start < value.length() && + (index = value.forEachByte(start, value.length() - start, FIND_SEMI_COLON)) != -1); + if (start >= value.length()) { + throw new IllegalArgumentException("cookie value is of unexpected format: " + value); + } + out.add(HttpHeaderNames.COOKIE, value.subSequence(start, value.length(), false)); + } else { + out.add(HttpHeaderNames.COOKIE, value); + } + } else { + out.add(aName, entry.getValue()); } } } @@ -446,7 +470,15 @@ public final class HttpConversionUtil { throw streamError(streamId, PROTOCOL_ERROR, "Invalid HTTP/2 header '%s' encountered in translation to HTTP/1.x", name); } - output.add(AsciiString.of(name), AsciiString.of(value)); + if (HttpHeaderNames.COOKIE.equals(name)) { + // combine the cookie values into 1 header entry. + // https://tools.ietf.org/html/rfc7540#section-8.1.2.5 + String existingCookie = output.getAsString(HttpHeaderNames.COOKIE); + output.set(HttpHeaderNames.COOKIE, + (existingCookie != null) ? (existingCookie + "; " + value) : value); + } else { + output.add(name, value); + } } } } diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/HttpToHttp2ConnectionHandlerTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/HttpToHttp2ConnectionHandlerTest.java index 99b179da17..3e55910f94 100644 --- a/codec-http2/src/test/java/io/netty/handler/codec/http2/HttpToHttp2ConnectionHandlerTest.java +++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/HttpToHttp2ConnectionHandlerTest.java @@ -140,6 +140,27 @@ public class HttpToHttp2ConnectionHandlerTest { verifyHeadersOnly(http2Headers, writePromise, clientChannel.writeAndFlush(request, writePromise)); } + @Test + public void testMultipleCookieEntriesAreCombined() throws Exception { + bootstrapEnv(2, 1, 0); + final FullHttpRequest request = new DefaultFullHttpRequest(HTTP_1_1, GET, + "http://my-user_name@www.example.org:5555/example"); + final HttpHeaders httpHeaders = request.headers(); + httpHeaders.setInt(HttpConversionUtil.ExtensionHeaderNames.STREAM_ID.text(), 5); + httpHeaders.set(HttpHeaderNames.HOST, "my-user_name@www.example.org:5555"); + httpHeaders.set(HttpConversionUtil.ExtensionHeaderNames.SCHEME.text(), "http"); + httpHeaders.set(HttpHeaderNames.COOKIE, "a=b; c=d; e=f"); + final Http2Headers http2Headers = + new DefaultHttp2Headers().method(new AsciiString("GET")).path(new AsciiString("/example")) + .authority(new AsciiString("www.example.org:5555")).scheme(new AsciiString("http")) + .add(HttpHeaderNames.COOKIE, "a=b") + .add(HttpHeaderNames.COOKIE, "c=d") + .add(HttpHeaderNames.COOKIE, "e=f"); + + ChannelPromise writePromise = newPromise(); + verifyHeadersOnly(http2Headers, writePromise, clientChannel.writeAndFlush(request, writePromise)); + } + @Test public void testOriginFormRequestTargetHandled() throws Exception { bootstrapEnv(2, 1, 0); diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/InboundHttp2ToHttpAdapterTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/InboundHttp2ToHttpAdapterTest.java index 4aa9f27db8..3549feb135 100644 --- a/codec-http2/src/test/java/io/netty/handler/codec/http2/InboundHttp2ToHttpAdapterTest.java +++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/InboundHttp2ToHttpAdapterTest.java @@ -157,6 +157,75 @@ public class InboundHttp2ToHttpAdapterTest { } } + @Test + public void clientRequestSingleHeaderCookieSplitIntoMultipleEntries() throws Exception { + boostrapEnv(1, 1, 1); + final FullHttpRequest request = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, + "/some/path/resource2", true); + try { + HttpHeaders httpHeaders = request.headers(); + httpHeaders.set(HttpConversionUtil.ExtensionHeaderNames.SCHEME.text(), "https"); + httpHeaders.set(HttpHeaderNames.HOST, "example.org"); + httpHeaders.setInt(HttpConversionUtil.ExtensionHeaderNames.STREAM_ID.text(), 3); + httpHeaders.setInt(HttpHeaderNames.CONTENT_LENGTH, 0); + httpHeaders.set(HttpHeaderNames.COOKIE, "a=b; c=d; e=f"); + final Http2Headers http2Headers = new DefaultHttp2Headers().method(new AsciiString("GET")). + scheme(new AsciiString("https")).authority(new AsciiString("example.org")) + .path(new AsciiString("/some/path/resource2")) + .add(HttpHeaderNames.COOKIE, "a=b") + .add(HttpHeaderNames.COOKIE, "c=d") + .add(HttpHeaderNames.COOKIE, "e=f"); + runInChannel(clientChannel, new Http2Runnable() { + @Override + public void run() { + frameWriter.writeHeaders(ctxClient(), 3, http2Headers, 0, true, newPromiseClient()); + ctxClient().flush(); + } + }); + awaitRequests(); + ArgumentCaptor requestCaptor = ArgumentCaptor.forClass(FullHttpMessage.class); + verify(serverListener).messageReceived(requestCaptor.capture()); + capturedRequests = requestCaptor.getAllValues(); + assertEquals(request, capturedRequests.get(0)); + } finally { + request.release(); + } + } + + @Test + public void clientRequestSingleHeaderCookieSplitIntoMultipleEntries2() throws Exception { + boostrapEnv(1, 1, 1); + final FullHttpRequest request = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, + "/some/path/resource2", true); + try { + HttpHeaders httpHeaders = request.headers(); + httpHeaders.set(HttpConversionUtil.ExtensionHeaderNames.SCHEME.text(), "https"); + httpHeaders.set(HttpHeaderNames.HOST, "example.org"); + httpHeaders.setInt(HttpConversionUtil.ExtensionHeaderNames.STREAM_ID.text(), 3); + httpHeaders.setInt(HttpHeaderNames.CONTENT_LENGTH, 0); + httpHeaders.set(HttpHeaderNames.COOKIE, "a=b; c=d; e=f"); + final Http2Headers http2Headers = new DefaultHttp2Headers().method(new AsciiString("GET")). + scheme(new AsciiString("https")).authority(new AsciiString("example.org")) + .path(new AsciiString("/some/path/resource2")) + .add(HttpHeaderNames.COOKIE, "a=b; c=d") + .add(HttpHeaderNames.COOKIE, "e=f"); + runInChannel(clientChannel, new Http2Runnable() { + @Override + public void run() { + frameWriter.writeHeaders(ctxClient(), 3, http2Headers, 0, true, newPromiseClient()); + ctxClient().flush(); + } + }); + awaitRequests(); + ArgumentCaptor requestCaptor = ArgumentCaptor.forClass(FullHttpMessage.class); + verify(serverListener).messageReceived(requestCaptor.capture()); + capturedRequests = requestCaptor.getAllValues(); + assertEquals(request, capturedRequests.get(0)); + } finally { + request.release(); + } + } + @Test public void clientRequestSingleHeaderNonAsciiShouldThrow() throws Exception { boostrapEnv(1, 1, 1); diff --git a/common/src/main/java/io/netty/util/AsciiString.java b/common/src/main/java/io/netty/util/AsciiString.java index 38ce3243e8..ceb5bf0ca5 100644 --- a/common/src/main/java/io/netty/util/AsciiString.java +++ b/common/src/main/java/io/netty/util/AsciiString.java @@ -276,7 +276,7 @@ public final class AsciiString implements CharSequence, Comparable } private int forEachByte0(int index, int length, ByteProcessor visitor) throws Exception { - final int len = offset + length; + final int len = offset + index + length; for (int i = offset + index; i < len; ++i) { if (!visitor.process(value[i])) { return i - offset; diff --git a/common/src/main/java/io/netty/util/ByteProcessor.java b/common/src/main/java/io/netty/util/ByteProcessor.java index 2847813c6f..ef5929e8b6 100644 --- a/common/src/main/java/io/netty/util/ByteProcessor.java +++ b/common/src/main/java/io/netty/util/ByteProcessor.java @@ -120,6 +120,16 @@ public interface ByteProcessor { } }; + /** + * Aborts on a {@code CR (';')}. + */ + ByteProcessor FIND_SEMI_COLON = new ByteProcessor() { + @Override + public boolean process(byte value) throws Exception { + return value != ';'; + } + }; + /** * @return {@code true} if the processor wants to continue the loop and handle the next byte in the buffer. * {@code false} if the processor wants to stop handling bytes and abort the loop. diff --git a/common/src/test/java/io/netty/util/AsciiStringMemoryTest.java b/common/src/test/java/io/netty/util/AsciiStringMemoryTest.java index 808ace76a7..0f2468aed6 100644 --- a/common/src/test/java/io/netty/util/AsciiStringMemoryTest.java +++ b/common/src/test/java/io/netty/util/AsciiStringMemoryTest.java @@ -17,6 +17,7 @@ package io.netty.util; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotEquals; +import io.netty.util.ByteProcessor.IndexOfProcessor; import java.util.Random; import java.util.concurrent.atomic.AtomicReference; @@ -102,6 +103,18 @@ public class AsciiStringMemoryTest { assertEquals(bAsciiString.length(), bCount.get().intValue()); } + @Test + public void forEachWithIndexEndTest() throws Exception { + assertNotEquals(-1, aAsciiString.forEachByte(aAsciiString.length() - 1, + 1, new IndexOfProcessor(aAsciiString.byteAt(aAsciiString.length() - 1)))); + } + + @Test + public void forEachWithIndexBeginTest() throws Exception { + assertNotEquals(-1, aAsciiString.forEachByte(0, + 1, new IndexOfProcessor(aAsciiString.byteAt(0)))); + } + @Test public void forEachDescTest() throws Exception { final AtomicReference aCount = new AtomicReference(0); @@ -128,6 +141,18 @@ public class AsciiStringMemoryTest { assertEquals(bAsciiString.length(), bCount.get().intValue()); } + @Test + public void forEachDescWithIndexEndTest() throws Exception { + assertNotEquals(-1, bAsciiString.forEachByteDesc(bAsciiString.length() - 1, + 1, new IndexOfProcessor(bAsciiString.byteAt(bAsciiString.length() - 1)))); + } + + @Test + public void forEachDescWithIndexBeginTest() throws Exception { + assertNotEquals(-1, bAsciiString.forEachByteDesc(0, + 1, new IndexOfProcessor(bAsciiString.byteAt(0)))); + } + @Test public void subSequenceTest() { final int start = 12;