From 41149cf74e3162836b3b978dda1c1b4dbaadbe3f Mon Sep 17 00:00:00 2001 From: Rama Date: Tue, 14 Nov 2017 21:38:24 +0530 Subject: [PATCH 1/3] netty better contentype handling --- .../io/grpc/netty/NettyServerHandler.java | 61 ++++++++++++++++--- .../io/grpc/netty/NettyServerHandlerTest.java | 61 +++++++++++++++++-- 2 files changed, 107 insertions(+), 15 deletions(-) diff --git a/netty/src/main/java/io/grpc/netty/NettyServerHandler.java b/netty/src/main/java/io/grpc/netty/NettyServerHandler.java index d5432148cb4..b2167e5551a 100644 --- a/netty/src/main/java/io/grpc/netty/NettyServerHandler.java +++ b/netty/src/main/java/io/grpc/netty/NettyServerHandler.java @@ -32,6 +32,8 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import io.grpc.Attributes; +import io.grpc.InternalMetadata; +import io.grpc.InternalStatus; import io.grpc.Metadata; import io.grpc.ServerStreamTracer; import io.grpc.Status; @@ -54,6 +56,7 @@ import io.netty.handler.codec.http2.DefaultHttp2ConnectionEncoder; import io.netty.handler.codec.http2.DefaultHttp2FrameReader; import io.netty.handler.codec.http2.DefaultHttp2FrameWriter; +import io.netty.handler.codec.http2.DefaultHttp2Headers; import io.netty.handler.codec.http2.DefaultHttp2LocalFlowController; import io.netty.handler.codec.http2.DefaultHttp2RemoteFlowController; import io.netty.handler.codec.http2.Http2Connection; @@ -375,15 +378,40 @@ private void onHeadersRead(ChannelHandlerContext ctx, int streamId, Http2Headers throws Http2Exception { if (!teWarningLogged && !TE_TRAILERS.equals(headers.get(TE_HEADER))) { logger.warning(String.format("Expected header TE: %s, but %s is received. This means " - + "some intermediate proxy may not support trailers", - TE_TRAILERS, headers.get(TE_HEADER))); + + "some intermediate proxy may not support trailers", + TE_TRAILERS, headers.get(TE_HEADER))); teWarningLogged = true; } try { + // Verify that the Content-Type is correct in the request. - verifyContentType(streamId, headers); - String method = determineMethod(streamId, headers); + CharSequence contentType = headers.get(CONTENT_TYPE_HEADER); + if (contentType == null) { + respondWithHttpError( + ctx, streamId, 415, Status.Code.INTERNAL, "Content-Type is missing from the request"); + return; + } + String contentTypeString = contentType.toString(); + if (!GrpcUtil.isGrpcContentType(contentTypeString)) { + respondWithHttpError(ctx, streamId, 415, Status.Code.INTERNAL, + String.format("Content-Type '%s' is not supported", contentTypeString)); + return; + } + + if (!HTTP_METHOD.equals(headers.method())) { + respondWithHttpError(ctx, streamId, 405, Status.Code.INTERNAL, + String.format("Method '%s' is not supported", headers.method())); + return; + } + // Remove the leading slash of the path and get the fully qualified method name + CharSequence path = headers.path(); + if (path.charAt(0) != '/') { + respondWithHttpError(ctx, streamId, 404, Status.Code.UNIMPLEMENTED, + String.format("Expected path to start with /: %s", path)); + return; + } + String method = path.subSequence(1, path.length()).toString(); // The Http2Stream object was put by AbstractHttp2ConnectionHandler before calling this // method. @@ -400,7 +428,7 @@ private void onHeadersRead(ChannelHandlerContext ctx, int streamId, Http2Headers maxMessageSize, statsTraceCtx, transportTracer); - String authority = getOrUpdateAuthority((AsciiString)headers.authority()); + String authority = getOrUpdateAuthority((AsciiString) headers.authority()); NettyServerStream stream = new NettyServerStream( ctx.channel(), state, @@ -411,10 +439,7 @@ private void onHeadersRead(ChannelHandlerContext ctx, int streamId, Http2Headers transportListener.streamCreated(stream, method, metadata); state.onStreamAllocated(); http2Stream.setProperty(streamKey, state); - - } catch (Http2Exception e) { - throw e; - } catch (Throwable e) { + } catch (Exception e) { logger.log(Level.WARNING, "Exception in onHeadersRead()", e); // Throw an exception that will get handled by onStreamError. throw newStreamException(streamId, e); @@ -647,6 +672,24 @@ private void verifyContentType(int streamId, Http2Headers headers) throws Http2E } } + private void respondWithHttpError( + ChannelHandlerContext ctx, int streamId, int code, Status.Code statusCode, String msg) { + Metadata metadata = new Metadata(); + metadata.put(InternalStatus.CODE_KEY, statusCode.toStatus()); + metadata.put(InternalStatus.MESSAGE_KEY, msg); + byte[][] serialized = InternalMetadata.serialize(metadata); + + Http2Headers headers = new DefaultHttp2Headers(true, serialized.length / 2) + .status("" + code) + .set(CONTENT_TYPE_HEADER, "text/plain; encoding=utf-8"); + for (int i = 0; i < serialized.length; i += 2) { + headers.add(new AsciiString(serialized[i], false), new AsciiString(serialized[i + 1], false)); + } + encoder().writeHeaders(ctx, streamId, headers, 0, false, ctx.newPromise()); + ByteBuf msgBuf = ByteBufUtil.writeUtf8(ctx.alloc(), msg); + encoder().writeData(ctx, streamId, msgBuf, 0, true, ctx.newPromise()); + } + private Http2Stream requireHttp2Stream(int streamId) { Http2Stream stream = connection().stream(streamId); if (stream == null) { diff --git a/netty/src/test/java/io/grpc/netty/NettyServerHandlerTest.java b/netty/src/test/java/io/grpc/netty/NettyServerHandlerTest.java index acf2710dbc9..73916c668b0 100644 --- a/netty/src/test/java/io/grpc/netty/NettyServerHandlerTest.java +++ b/netty/src/test/java/io/grpc/netty/NettyServerHandlerTest.java @@ -29,6 +29,7 @@ import static io.grpc.netty.Utils.TE_HEADER; import static io.grpc.netty.Utils.TE_TRAILERS; import static io.netty.buffer.Unpooled.directBuffer; +import static io.netty.handler.codec.http2.Http2CodecUtil.DEFAULT_PRIORITY_WEIGHT; import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; @@ -53,6 +54,7 @@ import com.google.common.io.ByteStreams; import com.google.common.truth.Truth; import io.grpc.Attributes; +import io.grpc.InternalStatus; import io.grpc.Metadata; import io.grpc.ServerStreamTracer; import io.grpc.Status; @@ -110,6 +112,9 @@ public class NettyServerHandlerTest extends NettyHandlerTestBase Date: Wed, 15 Nov 2017 11:41:04 +0530 Subject: [PATCH 2/3] moved path handling up to early return 404 --- .../io/grpc/netty/NettyServerHandler.java | 25 +++++++++++++------ .../io/grpc/netty/NettyServerHandlerTest.java | 18 +++++++++++++ 2 files changed, 35 insertions(+), 8 deletions(-) diff --git a/netty/src/main/java/io/grpc/netty/NettyServerHandler.java b/netty/src/main/java/io/grpc/netty/NettyServerHandler.java index b2167e5551a..b6789a9b9b6 100644 --- a/netty/src/main/java/io/grpc/netty/NettyServerHandler.java +++ b/netty/src/main/java/io/grpc/netty/NettyServerHandler.java @@ -385,6 +385,23 @@ private void onHeadersRead(ChannelHandlerContext ctx, int streamId, Http2Headers try { + // Remove the leading slash of the path and get the fully qualified method name + CharSequence path = headers.path(); + + if (path == null) { + respondWithHttpError(ctx, streamId, 404, Status.Code.UNIMPLEMENTED, + "Expected path but is missing"); + return; + } + + if (path.charAt(0) != '/') { + respondWithHttpError(ctx, streamId, 404, Status.Code.UNIMPLEMENTED, + String.format("Expected path to start with /: %s", path)); + return; + } + + String method = path.subSequence(1, path.length()).toString(); + // Verify that the Content-Type is correct in the request. CharSequence contentType = headers.get(CONTENT_TYPE_HEADER); if (contentType == null) { @@ -404,14 +421,6 @@ private void onHeadersRead(ChannelHandlerContext ctx, int streamId, Http2Headers String.format("Method '%s' is not supported", headers.method())); return; } - // Remove the leading slash of the path and get the fully qualified method name - CharSequence path = headers.path(); - if (path.charAt(0) != '/') { - respondWithHttpError(ctx, streamId, 404, Status.Code.UNIMPLEMENTED, - String.format("Expected path to start with /: %s", path)); - return; - } - String method = path.subSequence(1, path.length()).toString(); // The Http2Stream object was put by AbstractHttp2ConnectionHandler before calling this // method. diff --git a/netty/src/test/java/io/grpc/netty/NettyServerHandlerTest.java b/netty/src/test/java/io/grpc/netty/NettyServerHandlerTest.java index 73916c668b0..5303c837f91 100644 --- a/netty/src/test/java/io/grpc/netty/NettyServerHandlerTest.java +++ b/netty/src/test/java/io/grpc/netty/NettyServerHandlerTest.java @@ -446,6 +446,24 @@ public void headersWithInvalidMethodShouldFail() throws Exception { eq(DEFAULT_PRIORITY_WEIGHT), eq(false), eq(0), eq(false), any(ChannelPromise.class)); } + @Test + public void headersWithMissingPathShouldFail() throws Exception { + manualSetUp(); + Http2Headers headers = new DefaultHttp2Headers() + .method(HTTP_METHOD) + .set(CONTENT_TYPE_HEADER, CONTENT_TYPE_GRPC); + ByteBuf headersFrame = headersFrame(STREAM_ID, headers); + channelRead(headersFrame); + Http2Headers responseHeaders = new DefaultHttp2Headers() + .set(InternalStatus.CODE_KEY.name(), String.valueOf(Code.UNIMPLEMENTED.value())) + .set(InternalStatus.MESSAGE_KEY.name(), "Expected path but is missing") + .status("" + 404) + .set(CONTENT_TYPE_HEADER, "text/plain; encoding=utf-8"); + + verifyWrite().writeHeaders(eq(ctx()), eq(STREAM_ID), eq(responseHeaders), eq(0), + eq(DEFAULT_PRIORITY_WEIGHT), eq(false), eq(0), eq(false), any(ChannelPromise.class)); + } + @Test public void headersWithInvalidPathShouldFail() throws Exception { manualSetUp(); From 60f73f6839ce2463538b2c4896bd3f0a35f66795 Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Wed, 15 Nov 2017 11:06:58 -0800 Subject: [PATCH 3/3] Remove dead methods --- .../io/grpc/netty/NettyServerHandler.java | 27 ------------------- 1 file changed, 27 deletions(-) diff --git a/netty/src/main/java/io/grpc/netty/NettyServerHandler.java b/netty/src/main/java/io/grpc/netty/NettyServerHandler.java index b6789a9b9b6..4836300fe3c 100644 --- a/netty/src/main/java/io/grpc/netty/NettyServerHandler.java +++ b/netty/src/main/java/io/grpc/netty/NettyServerHandler.java @@ -668,19 +668,6 @@ public boolean visit(Http2Stream stream) throws Http2Exception { }); } - private void verifyContentType(int streamId, Http2Headers headers) throws Http2Exception { - CharSequence contentType = headers.get(CONTENT_TYPE_HEADER); - if (contentType == null) { - throw Http2Exception.streamError(streamId, Http2Error.REFUSED_STREAM, - "Content-Type is missing from the request"); - } - String contentTypeString = contentType.toString(); - if (!GrpcUtil.isGrpcContentType(contentTypeString)) { - throw Http2Exception.streamError(streamId, Http2Error.REFUSED_STREAM, - "Content-Type '%s' is not supported", contentTypeString); - } - } - private void respondWithHttpError( ChannelHandlerContext ctx, int streamId, int code, Status.Code statusCode, String msg) { Metadata metadata = new Metadata(); @@ -708,20 +695,6 @@ private Http2Stream requireHttp2Stream(int streamId) { return stream; } - private String determineMethod(int streamId, Http2Headers headers) throws Http2Exception { - if (!HTTP_METHOD.equals(headers.method())) { - throw Http2Exception.streamError(streamId, Http2Error.REFUSED_STREAM, - "Method '%s' is not supported", headers.method()); - } - // Remove the leading slash of the path and get the fully qualified method name - CharSequence path = headers.path(); - if (path.charAt(0) != '/') { - throw Http2Exception.streamError(streamId, Http2Error.REFUSED_STREAM, - "Malformatted path: %s", path); - } - return path.subSequence(1, path.length()).toString(); - } - /** * Returns the server stream associated to the given HTTP/2 stream object. */