From c00c1b28f949913f7aa64b3863ec2efaeda535d1 Mon Sep 17 00:00:00 2001 From: Scott Mitchell Date: Fri, 9 Oct 2020 18:37:14 -0700 Subject: [PATCH] AbstractNettyHttpServerTest payload validation bug (#1173) Motivation: AbstractNettyHttpServerTest attempts to validate that the payload is read in the same boundaries that it was written. This is not guaranteed and the payload maybe chunked differently than written. Modifications: - Verify the aggregated payload is received as expected so we are agnostic to intermediate patching. Result: More reliable verification of payload contents. --- .../gradle/spotbugs/test-exclusions.xml | 8 +++ .../netty/AbstractNettyHttpServerTest.java | 21 ++---- ...NettyHttpServerConnectionAcceptorTest.java | 3 +- .../http/netty/NettyHttpServerTest.java | 66 +++++++++---------- 4 files changed, 47 insertions(+), 51 deletions(-) diff --git a/servicetalk-http-netty/gradle/spotbugs/test-exclusions.xml b/servicetalk-http-netty/gradle/spotbugs/test-exclusions.xml index b7019c6bbc..a76a973518 100644 --- a/servicetalk-http-netty/gradle/spotbugs/test-exclusions.xml +++ b/servicetalk-http-netty/gradle/spotbugs/test-exclusions.xml @@ -47,4 +47,12 @@ + + + + + + + + diff --git a/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/AbstractNettyHttpServerTest.java b/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/AbstractNettyHttpServerTest.java index 090ca6bec0..35fe78124d 100644 --- a/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/AbstractNettyHttpServerTest.java +++ b/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/AbstractNettyHttpServerTest.java @@ -58,7 +58,6 @@ import java.net.InetSocketAddress; import java.net.StandardSocketOptions; -import java.util.List; import java.util.concurrent.ExecutionException; import java.util.function.Function; import java.util.function.Supplier; @@ -79,10 +78,8 @@ import static java.nio.charset.StandardCharsets.US_ASCII; import static java.util.Objects.requireNonNull; import static org.hamcrest.MatcherAssert.assertThat; -import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.is; import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertTrue; import static org.junit.Assume.assumeThat; public abstract class AbstractNettyHttpServerTest { @@ -258,16 +255,15 @@ void assertResponse(final StreamingHttpResponse response, final HttpProtocolVers } void assertResponse(final StreamingHttpResponse response, final HttpProtocolVersion version, - final HttpResponseStatus status, final List expectedPayloadChunksAsStrings) + final HttpResponseStatus status, final String expectedPayload) throws ExecutionException, InterruptedException { assertEquals(status, response.status()); assertEquals(version, response.version()); - final List bodyAsListOfStrings = getBodyAsListOfStrings(response); - if (expectedPayloadChunksAsStrings.isEmpty()) { - assertTrue(bodyAsListOfStrings.isEmpty()); - } else { - assertThat(bodyAsListOfStrings, contains(expectedPayloadChunksAsStrings.toArray())); - } + String actualPayload = response.payloadBody().collect(StringBuilder::new, (sb, chunk) -> { + sb.append(chunk.toString(US_ASCII)); + return sb; + }).toFuture().get().toString(); + assertThat(actualPayload, is(expectedPayload)); } Publisher getChunkPublisherFromStrings(final String... texts) { @@ -282,11 +278,6 @@ Buffer getChunkFromString(final String text) { return DEFAULT_ALLOCATOR.fromAscii(text); } - static List getBodyAsListOfStrings(final StreamingHttpResponse response) - throws ExecutionException, InterruptedException { - return awaitIndefinitelyNonNull(response.payloadBody().map(c -> c.toString(US_ASCII))); - } - static T awaitSingleIndefinitelyNonNull(Single single) { try { return requireNonNull(single.toFuture().get()); diff --git a/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/NettyHttpServerConnectionAcceptorTest.java b/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/NettyHttpServerConnectionAcceptorTest.java index c078079067..3f5b16e615 100644 --- a/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/NettyHttpServerConnectionAcceptorTest.java +++ b/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/NettyHttpServerConnectionAcceptorTest.java @@ -48,7 +48,6 @@ import static io.servicetalk.http.netty.AbstractNettyHttpServerTest.ExecutorSupplier.CACHED; import static io.servicetalk.http.netty.AbstractNettyHttpServerTest.ExecutorSupplier.IMMEDIATE; import static io.servicetalk.http.netty.TestServiceStreaming.SVC_ECHO; -import static java.util.Collections.singletonList; import static java.util.concurrent.TimeUnit.MILLISECONDS; import static org.hamcrest.Matchers.anyOf; import static org.hamcrest.Matchers.instanceOf; @@ -140,7 +139,7 @@ public void testAcceptConnection() throws Exception { getChunkPublisherFromStrings("hello")); request.headers().set(CONTENT_LENGTH, "5"); final StreamingHttpResponse response = makeRequest(request); - assertResponse(response, HTTP_1_1, OK, singletonList("hello")); + assertResponse(response, HTTP_1_1, OK, "hello"); if (!filterMode.expectAccept) { throw new AssertionError("Expected filter to reject connection"); } diff --git a/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/NettyHttpServerTest.java b/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/NettyHttpServerTest.java index f7cd351fc5..10042a65b6 100644 --- a/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/NettyHttpServerTest.java +++ b/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/NettyHttpServerTest.java @@ -75,8 +75,6 @@ import static io.servicetalk.http.netty.TestServiceStreaming.SVC_THROW_ERROR; import static java.nio.charset.StandardCharsets.US_ASCII; import static java.util.Arrays.asList; -import static java.util.Collections.emptyList; -import static java.util.Collections.singletonList; import static java.util.concurrent.TimeUnit.MILLISECONDS; import static java.util.concurrent.TimeUnit.SECONDS; import static org.hamcrest.Matchers.is; @@ -125,14 +123,14 @@ public static Collection clientExecutors() { public void testGetNoRequestPayloadWithoutResponseLastChunk() throws Exception { final StreamingHttpRequest request = reqRespFactory.newRequest(GET, SVC_COUNTER_NO_LAST_CHUNK); final StreamingHttpResponse response = makeRequest(request); - assertResponse(response, HTTP_1_1, OK, singletonList("Testing1\n")); + assertResponse(response, HTTP_1_1, OK, "Testing1\n"); } @Test public void testGetNoRequestPayload() throws Exception { final StreamingHttpRequest request = reqRespFactory.newRequest(GET, SVC_COUNTER); final StreamingHttpResponse response = makeRequest(request); - assertResponse(response, HTTP_1_1, OK, singletonList("Testing1\n")); + assertResponse(response, HTTP_1_1, OK, "Testing1\n"); } @Test @@ -145,7 +143,7 @@ public void testGetEchoPayloadContentLength() throws Exception { }); request.headers().set(CONTENT_LENGTH, "5"); final StreamingHttpResponse response = makeRequest(request); - assertResponse(response, HTTP_1_1, OK, singletonList("hello")); + assertResponse(response, HTTP_1_1, OK, "hello"); } @Test @@ -153,7 +151,7 @@ public void testGetEchoPayloadChunked() throws Exception { final StreamingHttpRequest request = reqRespFactory.newRequest(GET, SVC_ECHO).payloadBody( getChunkPublisherFromStrings("hello")); final StreamingHttpResponse response = makeRequest(request); - assertResponse(response, HTTP_1_1, OK, singletonList("hello")); + assertResponse(response, HTTP_1_1, OK, "hello"); } @Test @@ -162,7 +160,7 @@ public void testGetRot13Payload() throws Exception { getChunkPublisherFromStrings("hello")); request.headers().set(CONTENT_LENGTH, "5"); final StreamingHttpResponse response = makeRequest(request); - assertResponse(response, HTTP_1_1, OK, singletonList("uryyb")); + assertResponse(response, HTTP_1_1, OK, "uryyb"); } @Test @@ -170,36 +168,36 @@ public void testGetIgnoreRequestPayload() throws Exception { final StreamingHttpRequest request = reqRespFactory.newRequest(GET, SVC_COUNTER).payloadBody( getChunkPublisherFromStrings("hello")); final StreamingHttpResponse response = makeRequest(request); - assertResponse(response, HTTP_1_1, OK, singletonList("Testing1\n")); + assertResponse(response, HTTP_1_1, OK, "Testing1\n"); } @Test public void testGetNoRequestPayloadNoResponsePayload() throws Exception { final StreamingHttpRequest request = reqRespFactory.newRequest(GET, SVC_NO_CONTENT); final StreamingHttpResponse response = makeRequest(request); - assertResponse(response, HTTP_1_1, NO_CONTENT, emptyList()); + assertResponse(response, HTTP_1_1, NO_CONTENT, ""); } @Test public void testMultipleGetsNoRequestPayloadWithoutResponseLastChunk() throws Exception { final StreamingHttpRequest request1 = reqRespFactory.newRequest(GET, SVC_COUNTER_NO_LAST_CHUNK); final StreamingHttpResponse response1 = makeRequest(request1); - assertResponse(response1, HTTP_1_1, OK, singletonList("Testing1\n")); + assertResponse(response1, HTTP_1_1, OK, "Testing1\n"); final StreamingHttpRequest request2 = reqRespFactory.newRequest(GET, SVC_COUNTER_NO_LAST_CHUNK); final StreamingHttpResponse response2 = makeRequest(request2); - assertResponse(response2, HTTP_1_1, OK, singletonList("Testing2\n")); + assertResponse(response2, HTTP_1_1, OK, "Testing2\n"); } @Test public void testMultipleGetsNoRequestPayload() throws Exception { final StreamingHttpRequest request1 = reqRespFactory.newRequest(GET, SVC_COUNTER); final StreamingHttpResponse response1 = makeRequest(request1); - assertResponse(response1, HTTP_1_1, OK, singletonList("Testing1\n")); + assertResponse(response1, HTTP_1_1, OK, "Testing1\n"); final StreamingHttpRequest request2 = reqRespFactory.newRequest(GET, SVC_COUNTER); final StreamingHttpResponse response2 = makeRequest(request2); - assertResponse(response2, HTTP_1_1, OK, singletonList("Testing2\n")); + assertResponse(response2, HTTP_1_1, OK, "Testing2\n"); } @Test @@ -208,13 +206,13 @@ public void testMultipleGetsEchoPayloadContentLength() throws Exception { getChunkPublisherFromStrings("hello")); request1.headers().set(CONTENT_LENGTH, "5"); final StreamingHttpResponse response1 = makeRequest(request1); - assertResponse(response1, HTTP_1_1, OK, singletonList("hello")); + assertResponse(response1, HTTP_1_1, OK, "hello"); final StreamingHttpRequest request2 = reqRespFactory.newRequest(GET, SVC_ECHO).payloadBody( getChunkPublisherFromStrings("hello")); request2.headers().set(CONTENT_LENGTH, "5"); final StreamingHttpResponse response2 = makeRequest(request2); - assertResponse(response2, HTTP_1_1, OK, singletonList("hello")); + assertResponse(response2, HTTP_1_1, OK, "hello"); } @Test @@ -222,12 +220,12 @@ public void testMultipleGetsEchoPayloadChunked() throws Exception { final StreamingHttpRequest request1 = reqRespFactory.newRequest(GET, SVC_ECHO).payloadBody( getChunkPublisherFromStrings("hello")); final StreamingHttpResponse response1 = makeRequest(request1); - assertResponse(response1, HTTP_1_1, OK, singletonList("hello")); + assertResponse(response1, HTTP_1_1, OK, "hello"); final StreamingHttpRequest request2 = reqRespFactory.newRequest(GET, SVC_ECHO).payloadBody( getChunkPublisherFromStrings("hello")); final StreamingHttpResponse response2 = makeRequest(request2); - assertResponse(response2, HTTP_1_1, OK, singletonList("hello")); + assertResponse(response2, HTTP_1_1, OK, "hello"); } @Test @@ -236,20 +234,20 @@ public void testMultipleGetsIgnoreRequestPayload() throws Exception { getChunkPublisherFromStrings("hello")); request1.headers().set(CONTENT_LENGTH, "5"); final StreamingHttpResponse response1 = makeRequest(request1); - assertResponse(response1, HTTP_1_1, OK, singletonList("Testing1\n")); + assertResponse(response1, HTTP_1_1, OK, "Testing1\n"); final StreamingHttpRequest request2 = reqRespFactory.newRequest(GET, SVC_COUNTER).payloadBody( getChunkPublisherFromStrings("hello")); request2.headers().set(CONTENT_LENGTH, "5"); final StreamingHttpResponse response2 = makeRequest(request2); - assertResponse(response2, HTTP_1_1, OK, singletonList("Testing2\n")); + assertResponse(response2, HTTP_1_1, OK, "Testing2\n"); } @Test public void testHttp10CloseConnection() throws Exception { final StreamingHttpRequest request = reqRespFactory.newRequest(GET, SVC_COUNTER).version(HTTP_1_0); final StreamingHttpResponse response = makeRequest(request); - assertResponse(response, HTTP_1_0, OK, singletonList("Testing1\n")); + assertResponse(response, HTTP_1_0, OK, "Testing1\n"); assertFalse(response.headers().contains(CONNECTION)); assertConnectionClosed(); @@ -260,13 +258,13 @@ public void testHttp10KeepAliveConnection() throws Exception { final StreamingHttpRequest request1 = reqRespFactory.newRequest(GET, SVC_COUNTER).version(HTTP_1_0); request1.headers().set("connection", "keep-alive"); final StreamingHttpResponse response1 = makeRequest(request1); - assertResponse(response1, HTTP_1_0, OK, singletonList("Testing1\n")); + assertResponse(response1, HTTP_1_0, OK, "Testing1\n"); assertTrue(response1.headers().contains(CONNECTION, KEEP_ALIVE)); final StreamingHttpRequest request2 = reqRespFactory.newRequest(GET, SVC_COUNTER).version(HTTP_1_0); request2.headers().set("connection", "keep-alive"); final StreamingHttpResponse response2 = makeRequest(request2); - assertResponse(response2, HTTP_1_0, OK, singletonList("Testing2\n")); + assertResponse(response2, HTTP_1_0, OK, "Testing2\n"); assertTrue(response1.headers().contains(CONNECTION, KEEP_ALIVE)); } @@ -275,7 +273,7 @@ public void testHttp11CloseConnection() throws Exception { final StreamingHttpRequest request = reqRespFactory.newRequest(GET, SVC_COUNTER); request.headers().set("connection", "close"); final StreamingHttpResponse response = makeRequest(request); - assertResponse(response, HTTP_1_1, OK, singletonList("Testing1\n")); + assertResponse(response, HTTP_1_1, OK, "Testing1\n"); assertTrue(response.headers().contains(CONNECTION, CLOSE)); assertConnectionClosed(); @@ -285,12 +283,12 @@ public void testHttp11CloseConnection() throws Exception { public void testHttp11KeepAliveConnection() throws Exception { final StreamingHttpRequest request1 = reqRespFactory.newRequest(GET, SVC_COUNTER); final StreamingHttpResponse response1 = makeRequest(request1); - assertResponse(response1, HTTP_1_1, OK, singletonList("Testing1\n")); + assertResponse(response1, HTTP_1_1, OK, "Testing1\n"); assertFalse(response1.headers().contains(CONNECTION)); final StreamingHttpRequest request2 = reqRespFactory.newRequest(GET, SVC_COUNTER); final StreamingHttpResponse response2 = makeRequest(request2); - assertResponse(response2, HTTP_1_1, OK, singletonList("Testing2\n")); + assertResponse(response2, HTTP_1_1, OK, "Testing2\n"); assertFalse(response2.headers().contains(CONNECTION)); } @@ -299,7 +297,7 @@ public void testHttp11KeepAliveConnection() throws Exception { public void testGracefulShutdownWhileIdle() throws Exception { final StreamingHttpRequest request1 = reqRespFactory.newRequest(GET, SVC_COUNTER); final StreamingHttpResponse response1 = makeRequest(request1); - assertResponse(response1, HTTP_1_1, OK, singletonList("Testing1\n")); + assertResponse(response1, HTTP_1_1, OK, "Testing1\n"); assertFalse(response1.headers().contains(CONNECTION)); // Use a very high timeout for the graceful close. It should happen quite quickly because there are no @@ -323,7 +321,7 @@ public void testGracefulShutdownWhileReadingPayload() throws Exception { publisher.onNext(getChunkFromString("Hello")); publisher.onComplete(); - assertResponse(response1, HTTP_1_1, OK, singletonList("Hello")); + assertResponse(response1, HTTP_1_1, OK, "Hello"); assertFalse(response1.headers().contains(CONNECTION)); // Eventually this should be assertTrue assertConnectionClosed(); @@ -434,7 +432,7 @@ public void testDeferCloseConnection() throws Exception { public void testSynchronousError() throws Exception { final StreamingHttpRequest request = reqRespFactory.newRequest(GET, SVC_THROW_ERROR); final StreamingHttpResponse response = makeRequest(request); - assertResponse(response, HTTP_1_1, INTERNAL_SERVER_ERROR, emptyList()); + assertResponse(response, HTTP_1_1, INTERNAL_SERVER_ERROR, ""); assertTrue(response.headers().contains(CONTENT_LENGTH, ZERO)); } @@ -442,7 +440,7 @@ public void testSynchronousError() throws Exception { public void testSingleError() throws Exception { final StreamingHttpRequest request = reqRespFactory.newRequest(GET, SVC_SINGLE_ERROR); final StreamingHttpResponse response = makeRequest(request); - assertResponse(response, HTTP_1_1, INTERNAL_SERVER_ERROR, emptyList()); + assertResponse(response, HTTP_1_1, INTERNAL_SERVER_ERROR, ""); assertTrue(response.headers().contains(CONTENT_LENGTH, ZERO)); } @@ -489,20 +487,20 @@ public void testErrorDuringRead() throws Exception { assertEquals(HTTP_1_1, response.version()); final BlockingIterator httpPayloadChunks = response.payloadBody().toIterable().iterator(); - assertEquals("Goodbye", httpPayloadChunks.next().toString(US_ASCII)); - assertEquals("cruel", httpPayloadChunks.next().toString(US_ASCII)); - assertEquals("world!", httpPayloadChunks.next().toString(US_ASCII)); - + StringBuilder sb = new StringBuilder(); // Due to a race condition, the exception cause here can vary. // If the socket closure is delayed slightly (for example, by delaying the Publisher.error(...) on the server) // then the client throws ClosedChannelException. However if the socket closure happens quickly enough, // the client throws NativeIoException (KQueue) or IOException (NIO). try { - httpPayloadChunks.next(); + while (httpPayloadChunks.hasNext()) { + sb.append(httpPayloadChunks.next().toString(US_ASCII)); + } fail("Server should close upon receiving the request"); } catch (RuntimeException wrapped) { // BlockingIterator wraps assertClientTransportInboundClosed(wrapped.getCause()); } + assertEquals("Goodbyecruelworld!", sb.toString()); assertConnectionClosed(); // Client inbound channel closed - should be same exception as above Throwable clientThrowable = ((NettyConnectionContext) streamingHttpConnection().connectionContext())