From bda67784c01a210fcb032c96507f71e48a1bd597 Mon Sep 17 00:00:00 2001 From: zpencer Date: Thu, 14 Sep 2017 15:26:36 -0700 Subject: [PATCH] core,netty,okhttp: strip outbound headers with reserved names (#3098) These will be stripped: CONTENT_TYPE, TE, USER_AGENT --- .../main/java/io/grpc/internal/GrpcUtil.java | 6 ++ .../java/io/grpc/netty/NettyClientStream.java | 2 - netty/src/main/java/io/grpc/netty/Utils.java | 15 +++- .../test/java/io/grpc/netty/UtilsTest.java | 73 +++++++++++++++++++ .../src/main/java/io/grpc/okhttp/Headers.java | 5 ++ .../io/grpc/okhttp/OkHttpClientStream.java | 2 - .../test/java/io/grpc/okhttp/HeadersTest.java | 60 +++++++++++++++ 7 files changed, 156 insertions(+), 7 deletions(-) create mode 100644 okhttp/src/test/java/io/grpc/okhttp/HeadersTest.java diff --git a/core/src/main/java/io/grpc/internal/GrpcUtil.java b/core/src/main/java/io/grpc/internal/GrpcUtil.java index a8d88998a9..49e170b4fe 100644 --- a/core/src/main/java/io/grpc/internal/GrpcUtil.java +++ b/core/src/main/java/io/grpc/internal/GrpcUtil.java @@ -100,6 +100,12 @@ public final class GrpcUtil { public static final Metadata.Key CONTENT_TYPE_KEY = Metadata.Key.of("content-type", Metadata.ASCII_STRING_MARSHALLER); + /** + * {@link io.grpc.Metadata.Key} for the Transfer encoding. + */ + public static final Metadata.Key TE_HEADER = + Metadata.Key.of("te", Metadata.ASCII_STRING_MARSHALLER); + /** * {@link io.grpc.Metadata.Key} for the Content-Type request/response header. */ diff --git a/netty/src/main/java/io/grpc/netty/NettyClientStream.java b/netty/src/main/java/io/grpc/netty/NettyClientStream.java index d3c0d5396b..41f046695c 100644 --- a/netty/src/main/java/io/grpc/netty/NettyClientStream.java +++ b/netty/src/main/java/io/grpc/netty/NettyClientStream.java @@ -29,7 +29,6 @@ import io.grpc.Metadata; import io.grpc.MethodDescriptor; import io.grpc.Status; import io.grpc.internal.AbstractClientStream; -import io.grpc.internal.GrpcUtil; import io.grpc.internal.Http2ClientStreamTransportState; import io.grpc.internal.StatsTraceContext; import io.grpc.internal.WritableBuffer; @@ -121,7 +120,6 @@ class NettyClientStream extends AbstractClientStream { } else { httpMethod = Utils.HTTP_METHOD; } - headers.discardAll(GrpcUtil.USER_AGENT_KEY); Http2Headers http2Headers = Utils.convertClientHeaders(headers, scheme, defaultPath, authority, httpMethod, userAgent); diff --git a/netty/src/main/java/io/grpc/netty/Utils.java b/netty/src/main/java/io/grpc/netty/Utils.java index 2d8dc17e67..7716edf282 100644 --- a/netty/src/main/java/io/grpc/netty/Utils.java +++ b/netty/src/main/java/io/grpc/netty/Utils.java @@ -17,7 +17,6 @@ package io.grpc.netty; import static io.grpc.internal.GrpcUtil.CONTENT_TYPE_KEY; -import static io.grpc.internal.GrpcUtil.USER_AGENT_KEY; import static io.grpc.internal.TransportFrameUtil.toHttp2Headers; import static io.grpc.internal.TransportFrameUtil.toRawSerializedHeaders; import static io.netty.util.CharsetUtil.UTF_8; @@ -55,9 +54,9 @@ class Utils { public static final AsciiString HTTP = AsciiString.of("http"); public static final AsciiString CONTENT_TYPE_HEADER = AsciiString.of(CONTENT_TYPE_KEY.name()); public static final AsciiString CONTENT_TYPE_GRPC = AsciiString.of(GrpcUtil.CONTENT_TYPE_GRPC); - public static final AsciiString TE_HEADER = AsciiString.of("te"); + public static final AsciiString TE_HEADER = AsciiString.of(GrpcUtil.TE_HEADER.name()); public static final AsciiString TE_TRAILERS = AsciiString.of(GrpcUtil.TE_TRAILERS); - public static final AsciiString USER_AGENT = AsciiString.of(USER_AGENT_KEY.name()); + public static final AsciiString USER_AGENT = AsciiString.of(GrpcUtil.USER_AGENT_KEY.name()); public static final Resource DEFAULT_BOSS_EVENT_LOOP_GROUP = new DefaultEventLoopGroupResource(1, "grpc-default-boss-ELG"); @@ -108,6 +107,11 @@ class Utils { Preconditions.checkNotNull(authority, "authority"); Preconditions.checkNotNull(method, "method"); + // Discard any application supplied duplicates of the reserved headers + headers.discardAll(CONTENT_TYPE_KEY); + headers.discardAll(GrpcUtil.TE_HEADER); + headers.discardAll(GrpcUtil.USER_AGENT_KEY); + return GrpcHttp2OutboundHeaders.clientRequestHeaders( toHttp2Headers(headers), authority, @@ -118,6 +122,11 @@ class Utils { } public static Http2Headers convertServerHeaders(Metadata headers) { + // Discard any application supplied duplicates of the reserved headers + headers.discardAll(CONTENT_TYPE_KEY); + headers.discardAll(GrpcUtil.TE_HEADER); + headers.discardAll(GrpcUtil.USER_AGENT_KEY); + return GrpcHttp2OutboundHeaders.serverResponseHeaders(toHttp2Headers(headers)); } diff --git a/netty/src/test/java/io/grpc/netty/UtilsTest.java b/netty/src/test/java/io/grpc/netty/UtilsTest.java index 46df5c46ef..4a14c67319 100644 --- a/netty/src/test/java/io/grpc/netty/UtilsTest.java +++ b/netty/src/test/java/io/grpc/netty/UtilsTest.java @@ -19,10 +19,16 @@ package io.grpc.netty; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertSame; +import io.grpc.Metadata; import io.grpc.Status; +import io.grpc.internal.GrpcUtil; import io.netty.channel.ConnectTimeoutException; +import io.netty.handler.codec.http2.DefaultHttp2Headers; import io.netty.handler.codec.http2.Http2Error; import io.netty.handler.codec.http2.Http2Exception; +import io.netty.handler.codec.http2.Http2Headers; +import io.netty.util.AsciiString; +import java.util.Map; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -30,6 +36,10 @@ import org.junit.runners.JUnit4; /** Unit tests for {@link Utils}. */ @RunWith(JUnit4.class) public class UtilsTest { + private final Metadata.Key userKey = + Metadata.Key.of("user-key", Metadata.ASCII_STRING_MARSHALLER); + private final String userValue = "user-value"; + @Test public void testStatusFromThrowable() { Status s = Status.CANCELLED.withDescription("msg"); @@ -43,6 +53,69 @@ public class UtilsTest { assertStatusEquals(Status.UNKNOWN.withCause(t), Utils.statusFromThrowable(t)); } + @Test + public void convertClientHeaders_sanitizes() { + Metadata metaData = new Metadata(); + + // Intentionally being explicit here rather than relying on any pre-defined lists of headers, + // since the goal of this test is to validate the correctness of such lists in the first place. + metaData.put(GrpcUtil.CONTENT_TYPE_KEY, "to-be-removed"); + metaData.put(GrpcUtil.USER_AGENT_KEY, "to-be-removed"); + metaData.put(GrpcUtil.TE_HEADER, "to-be-removed"); + metaData.put(userKey, userValue); + + String scheme = "https"; + String userAgent = "user-agent"; + String method = "POST"; + String authority = "authority"; + String path = "//testService/test"; + + Http2Headers output = + Utils.convertClientHeaders( + metaData, + new AsciiString(scheme), + new AsciiString(path), + new AsciiString(authority), + new AsciiString(method), + new AsciiString(userAgent)); + DefaultHttp2Headers headers = new DefaultHttp2Headers(); + for (Map.Entry entry : output) { + headers.add(entry.getKey(), entry.getValue()); + } + + // 7 reserved headers, 1 user header + assertEquals(7 + 1, headers.size()); + // Check the 3 reserved headers that are non pseudo + // Users can not create pseudo headers keys so no need to check for them here + assertEquals(GrpcUtil.CONTENT_TYPE_GRPC, + headers.get(GrpcUtil.CONTENT_TYPE_KEY.name()).toString()); + assertEquals(userAgent, headers.get(GrpcUtil.USER_AGENT_KEY.name()).toString()); + assertEquals(GrpcUtil.TE_TRAILERS, headers.get(GrpcUtil.TE_HEADER.name()).toString()); + // Check the user header is in tact + assertEquals(userValue, headers.get(userKey.name()).toString()); + } + + @Test + public void convertServerHeaders_sanitizes() { + Metadata metaData = new Metadata(); + + // Intentionally being explicit here rather than relying on any pre-defined lists of headers, + // since the goal of this test is to validate the correctness of such lists in the first place. + metaData.put(GrpcUtil.CONTENT_TYPE_KEY, "to-be-removed"); + metaData.put(GrpcUtil.TE_HEADER, "to-be-removed"); + metaData.put(GrpcUtil.USER_AGENT_KEY, "to-be-removed"); + metaData.put(userKey, userValue); + + Http2Headers output = Utils.convertServerHeaders(metaData); + DefaultHttp2Headers headers = new DefaultHttp2Headers(); + for (Map.Entry entry : output) { + headers.add(entry.getKey(), entry.getValue()); + } + // 2 reserved headers, 1 user header + assertEquals(2 + 1, headers.size()); + assertEquals(Utils.CONTENT_TYPE_GRPC, headers.get(GrpcUtil.CONTENT_TYPE_KEY.name())); + } + private static void assertStatusEquals(Status expected, Status actual) { assertEquals(expected.getCode(), actual.getCode()); assertEquals(expected.getDescription(), actual.getDescription()); diff --git a/okhttp/src/main/java/io/grpc/okhttp/Headers.java b/okhttp/src/main/java/io/grpc/okhttp/Headers.java index a51676ff29..9e5aa40e10 100644 --- a/okhttp/src/main/java/io/grpc/okhttp/Headers.java +++ b/okhttp/src/main/java/io/grpc/okhttp/Headers.java @@ -52,6 +52,11 @@ class Headers { Preconditions.checkNotNull(defaultPath, "defaultPath"); Preconditions.checkNotNull(authority, "authority"); + // Discard any application supplied duplicates of the reserved headers + headers.discardAll(GrpcUtil.CONTENT_TYPE_KEY); + headers.discardAll(GrpcUtil.TE_HEADER); + headers.discardAll(GrpcUtil.USER_AGENT_KEY); + // 7 is the number of explicit add calls below. List
okhttpHeaders = new ArrayList
(7 + InternalMetadata.headerCount(headers)); diff --git a/okhttp/src/main/java/io/grpc/okhttp/OkHttpClientStream.java b/okhttp/src/main/java/io/grpc/okhttp/OkHttpClientStream.java index 311f396e6f..e833a0ef43 100644 --- a/okhttp/src/main/java/io/grpc/okhttp/OkHttpClientStream.java +++ b/okhttp/src/main/java/io/grpc/okhttp/OkHttpClientStream.java @@ -25,7 +25,6 @@ import io.grpc.Metadata; import io.grpc.MethodDescriptor; import io.grpc.Status; import io.grpc.internal.AbstractClientStream; -import io.grpc.internal.GrpcUtil; import io.grpc.internal.Http2ClientStreamTransportState; import io.grpc.internal.StatsTraceContext; import io.grpc.internal.WritableBuffer; @@ -127,7 +126,6 @@ class OkHttpClientStream extends AbstractClientStream { useGet = true; defaultPath += "?" + BaseEncoding.base64().encode(payload); } - metadata.discardAll(GrpcUtil.USER_AGENT_KEY); synchronized (state.lock) { state.streamReady(metadata, defaultPath); } diff --git a/okhttp/src/test/java/io/grpc/okhttp/HeadersTest.java b/okhttp/src/test/java/io/grpc/okhttp/HeadersTest.java new file mode 100644 index 0000000000..7314cd6946 --- /dev/null +++ b/okhttp/src/test/java/io/grpc/okhttp/HeadersTest.java @@ -0,0 +1,60 @@ +/* + * Copyright 2017, gRPC Authors All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.grpc.okhttp; + +import static com.google.common.truth.Truth.assertThat; +import static org.junit.Assert.assertEquals; + +import io.grpc.Metadata; +import io.grpc.internal.GrpcUtil; +import io.grpc.okhttp.internal.framed.Header; +import java.util.List; +import org.junit.Test; + +public class HeadersTest { + @Test + public void createRequestHeaders_sanitizes() { + Metadata metaData = new Metadata(); + + // Intentionally being explicit here rather than relying on any pre-defined lists of headers, + // since the goal of this test is to validate the correctness of such lists in the first place. + metaData.put(GrpcUtil.CONTENT_TYPE_KEY, "to-be-removed"); + metaData.put(GrpcUtil.USER_AGENT_KEY, "to-be-removed"); + metaData.put(GrpcUtil.TE_HEADER, "to-be-removed"); + + + Metadata.Key userKey = Metadata.Key.of("user-key", Metadata.ASCII_STRING_MARSHALLER); + String userValue = "user-value"; + metaData.put(userKey, userValue); + + String path = "//testServerice/test"; + String authority = "localhost"; + String userAgent = "useragent"; + + List
headers = Headers.createRequestHeaders(metaData, path, authority, userAgent); + + // 7 reserved headers, 1 user header + assertEquals(7 + 1, headers.size()); + // Check the 3 reserved headers that are non pseudo + // Users can not create pseudo headers keys so no need to check for them here + assertThat(headers).contains(Headers.CONTENT_TYPE_HEADER); + assertThat(headers).contains(new Header(GrpcUtil.USER_AGENT_KEY.name(), userAgent)); + assertThat(headers).contains(new Header(GrpcUtil.TE_HEADER.name(), GrpcUtil.TE_TRAILERS)); + // Check the user header is in tact + assertThat(headers).contains(new Header(userKey.name(), userValue)); + } +}