Skip to content

Commit

Permalink
Do not expose HTTP/2 pseudo headers in header maps - fixes #2941
Browse files Browse the repository at this point in the history
  • Loading branch information
@vietj
vietj committed May 11, 2019
1 parent a17f990 commit 091de1c
Show file tree
Hide file tree
Showing 5 changed files with 59 additions and 78 deletions.
3 changes: 3 additions & 0 deletions src/main/java/io/vertx/core/http/impl/Http2ClientConnection.java
View file
Expand Up @@ -338,6 +338,9 @@ void handleHeaders(Http2Headers headers, StreamPriority streamPriority, boolean
writeReset(0x01 /* PROTOCOL_ERROR */);
return;
}

headers.remove(":status");

response = new HttpClientResponseImpl(
request,
HttpVersion.HTTP_2,
Expand Down
81 changes: 31 additions & 50 deletions src/main/java/io/vertx/core/http/impl/Http2ServerRequestImpl.java
View file
Expand Up @@ -16,6 +16,7 @@
import io.netty.handler.codec.http.HttpHeaderNames;
import io.netty.handler.codec.http.HttpHeaderValues;
import io.netty.handler.codec.http.HttpRequest;
import io.netty.handler.codec.http.HttpUtil;
import io.netty.handler.codec.http.LastHttpContent;
import io.netty.handler.codec.http.multipart.Attribute;
import io.netty.handler.codec.http.multipart.HttpPostRequestDecoder;
Expand Down Expand Up @@ -63,15 +64,16 @@ public class Http2ServerRequestImpl extends VertxHttp2Stream<Http2ServerConnecti

private final String serverOrigin;
private final Http2ServerResponseImpl response;
private final Http2Headers headers;
private MultiMap headersMap;
private MultiMap params;
private HttpMethod method;
private String rawMethod;
private String absoluteURI;
private String uri;
private final MultiMap headersMap;
private final String rawMethod;
private final String scheme;
private final String host;
private final String uri;
private String path;
private String query;
private HttpMethod method;
private MultiMap params;
private String absoluteURI;
private MultiMap attributes;
private Object trace;

Expand All @@ -94,8 +96,18 @@ public Http2ServerRequestImpl(Http2ServerConnection conn, ContextInternal contex
super(conn, context, stream, writable);

this.serverOrigin = serverOrigin;
this.headers = headers;
this.streamEnded = streamEnded;
this.uri = headers.get(":path") != null ? headers.get(":path").toString() : null;
this.scheme = headers.get(":scheme") != null ? headers.get(":scheme").toString() : null;
this.rawMethod = headers.get(":method") != null ? headers.get(":method").toString() : null;
this.host = headers.get(":authority") != null ? headers.get(":authority").toString() : null;

headers.remove(":method");
headers.remove(":scheme");
headers.remove(":path");
headers.remove(":authority");
headersMap = new Http2HeadersAdaptor(headers);


String host = host();
if (host == null) {
Expand Down Expand Up @@ -313,21 +325,15 @@ public HttpVersion version() {
public HttpMethod method() {
synchronized (conn) {
if (method == null) {
String sMethod = headers.method().toString();
method = HttpUtils.toVertxMethod(sMethod);
method = HttpUtils.toVertxMethod(rawMethod);
}
return method;
}
}

@Override
public String rawMethod() {
synchronized (conn) {
if (rawMethod == null) {
rawMethod = headers.method().toString();
}
return rawMethod;
}
return rawMethod;
}

@Override
Expand All @@ -337,53 +343,33 @@ public boolean isSSL() {

@Override
public String uri() {
synchronized (conn) {
if (uri == null) {
CharSequence path = headers.path();
if (path != null) {
uri = path.toString();
}
}
return uri;
}
return uri;
}

@Override
public String path() {
synchronized (conn) {
if (path == null) {
CharSequence path = headers.path();
if (path != null) {
this.path = HttpUtils.parsePath(path.toString());
}
}
this.path = uri != null ? HttpUtils.parsePath(uri) : null;
return path;
}
}

@Override
public String query() {
synchronized (conn) {
if (query == null) {
CharSequence path = headers.path();
if (path != null) {
this.query = HttpUtils.parseQuery(path.toString());
}
}
this.query = uri != null ? HttpUtils.parseQuery(uri) : null;
return query;
}
}

@Override
public String scheme() {
CharSequence scheme = headers.scheme();
return scheme != null ? scheme.toString() : null;
return scheme;
}

@Override
public String host() {
CharSequence authority = headers.authority();
return authority != null ? authority.toString() : null;
return host;
}

@Override
Expand All @@ -401,12 +387,7 @@ public Http2ServerResponseImpl response() {

@Override
public MultiMap headers() {
synchronized (conn) {
if (headersMap == null) {
headersMap = new Http2HeadersAdaptor(headers);
}
return headersMap;
}
return headersMap;
}

@Override
Expand Down Expand Up @@ -482,9 +463,9 @@ public HttpServerRequest setExpectMultipart(boolean expect) {
checkEnded();
if (expect) {
if (postRequestDecoder == null) {
CharSequence contentType = headers.get(HttpHeaderNames.CONTENT_TYPE);
String contentType = headersMap.get(HttpHeaderNames.CONTENT_TYPE);
if (contentType != null) {
io.netty.handler.codec.http.HttpMethod method = io.netty.handler.codec.http.HttpMethod.valueOf(headers.method().toString());
io.netty.handler.codec.http.HttpMethod method = io.netty.handler.codec.http.HttpMethod.valueOf(rawMethod);
String lowerCaseContentType = contentType.toString().toLowerCase();
boolean isURLEncoded = lowerCaseContentType.startsWith(HttpHeaderValues.APPLICATION_X_WWW_FORM_URLENCODED.toString());
if ((lowerCaseContentType.startsWith(HttpHeaderValues.MULTIPART_FORM_DATA.toString()) || isURLEncoded) &&
Expand All @@ -495,7 +476,7 @@ public HttpServerRequest setExpectMultipart(boolean expect) {
HttpRequest req = new DefaultHttpRequest(
io.netty.handler.codec.http.HttpVersion.HTTP_1_1,
method,
headers.path().toString());
uri);
req.headers().add(HttpHeaderNames.CONTENT_TYPE, contentType);
postRequestDecoder = new HttpPostRequestDecoder(new NettyFileUploadDataFactory(context, this, () -> uploadHandler), req);
}
Expand Down
3 changes: 2 additions & 1 deletion src/test/java/io/vertx/core/http/Http2ClientTest.java
View file
Expand Up @@ -228,6 +228,7 @@ public void testHeaders() throws Exception {
assertEquals(2, req.headers().getAll("juu_request").size());
assertEquals("juu_request_value_1", req.headers().getAll("juu_request").get(0));
assertEquals("juu_request_value_2", req.headers().getAll("juu_request").get(1));
assertEquals(new HashSet<>(Arrays.asList("foo_request", "bar_request", "juu_request")), new HashSet<>(req.headers().names()));
reqCount.incrementAndGet();
HttpServerResponse resp = req.response();
resp.putHeader("content-type", "text/plain");
Expand All @@ -247,12 +248,12 @@ public void testHeaders() throws Exception {
assertEquals(200, resp.statusCode());
assertEquals("OK", resp.statusMessage());
assertEquals("text/plain", resp.getHeader("content-type"));
assertEquals("200", resp.getHeader(":status"));
assertEquals("foo_value", resp.getHeader("foo_response"));
assertEquals("bar_value", resp.getHeader("bar_response"));
assertEquals(2, resp.headers().getAll("juu_response").size());
assertEquals("juu_value_1", resp.headers().getAll("juu_response").get(0));
assertEquals("juu_value_2", resp.headers().getAll("juu_response").get(1));
assertEquals(new HashSet<>(Arrays.asList("content-type", "content-length", "foo_response", "bar_response", "juu_response")), new HashSet<>(resp.headers().names()));
resp.endHandler(v -> {
assertOnIOContext(ctx);
testComplete();
Expand Down
8 changes: 2 additions & 6 deletions src/test/java/io/vertx/core/http/Http2ServerTest.java
View file
Expand Up @@ -417,11 +417,9 @@ public void testGet() throws Exception {
assertEquals(HttpMethod.GET, req.method());
assertEquals(DEFAULT_HTTPS_HOST_AND_PORT, req.host());
assertEquals("/", req.path());
assertEquals(DEFAULT_HTTPS_HOST_AND_PORT, req.getHeader(":authority"));
assertTrue(req.isSSL());
assertEquals("https", req.getHeader(":scheme"));
assertEquals("/", req.getHeader(":path"));
assertEquals("GET", req.getHeader(":method"));
assertEquals("https", req.scheme());
assertEquals("/", req.uri());
assertEquals("foo_request_value", req.getHeader("Foo_request"));
assertEquals("bar_request_value", req.getHeader("bar_request"));
assertEquals(2, req.headers().getAll("juu_request").size());
Expand Down Expand Up @@ -505,7 +503,6 @@ public void testURI() throws Exception {
assertEquals("foo=foo_value&bar=bar_value_1&bar=bar_value_2", req.query());
assertEquals("/some/path?foo=foo_value&bar=bar_value_1&bar=bar_value_2", req.uri());
assertEquals("http://whatever.com/some/path?foo=foo_value&bar=bar_value_1&bar=bar_value_2", req.absoluteURI());
assertEquals("/some/path?foo=foo_value&bar=bar_value_1&bar=bar_value_2", req.getHeader(":path"));
assertEquals("whatever.com", req.host());
MultiMap params = req.params();
Set<String> names = params.names();
Expand Down Expand Up @@ -2660,7 +2657,6 @@ public void testPushPromiseClearText() throws Exception {
assertEquals(HttpVersion.HTTP_2, resp.version());
complete();
})).exceptionHandler(this::fail).pushHandler(pushedReq -> {
assertEquals("http", pushedReq.headers().get(":scheme"));
pushedReq.handler(onSuccess(pushResp -> {
pushResp.bodyHandler(buff -> {
assertEquals("the-pushed-response", buff.toString());
Expand Down
42 changes: 21 additions & 21 deletions src/test/java/io/vertx/core/http/HttpTest.java
View file
Expand Up @@ -773,11 +773,7 @@ public void testDefaultRequestHeaders() {
assertEquals(1, req.headers().size());
assertEquals("localhost:" + DEFAULT_HTTP_PORT, req.headers().get("host"));
} else {
assertEquals(4, req.headers().size());
assertEquals("https", req.headers().get(":scheme"));
assertEquals("GET", req.headers().get(":method"));
assertEquals("some-uri", req.headers().get(":path"));
assertEquals("localhost:" + DEFAULT_HTTP_PORT, req.headers().get(":authority"));
assertEquals(0, req.headers().size());
}
req.response().end();
});
Expand All @@ -791,25 +787,28 @@ public void testDefaultRequestHeaders() {

@Test
public void testRequestHeadersWithCharSequence() {
HashMap<CharSequence, String> headers = new HashMap<>();
headers.put(HttpHeaders.TEXT_HTML, "text/html");
headers.put(HttpHeaders.USER_AGENT, "User-Agent");
headers.put(HttpHeaders.APPLICATION_X_WWW_FORM_URLENCODED, "application/x-www-form-urlencoded");
HashMap<CharSequence, String> expectedHeaders = new HashMap<>();
expectedHeaders.put(HttpHeaders.TEXT_HTML, "text/html");
expectedHeaders.put(HttpHeaders.USER_AGENT, "User-Agent");
expectedHeaders.put(HttpHeaders.APPLICATION_X_WWW_FORM_URLENCODED, "application/x-www-form-urlencoded");

server.requestHandler(req -> {

assertTrue(headers.size() < req.headers().size());
MultiMap headers = req.headers();
headers.remove("host");

headers.forEach((k, v) -> assertEquals(v, req.headers().get(k)));
headers.forEach((k, v) -> assertEquals(v, req.getHeader(k)));
assertEquals(expectedHeaders.size(), headers.size());

expectedHeaders.forEach((k, v) -> assertEquals(v, headers.get(k)));
expectedHeaders.forEach((k, v) -> assertEquals(v, req.getHeader(k)));

req.response().end();
});

server.listen(testAddress, onSuccess(server -> {
HttpClientRequest req = client.request(HttpMethod.GET, testAddress, DEFAULT_HTTP_PORT, DEFAULT_HTTP_HOST, DEFAULT_TEST_URI, resp -> testComplete());

headers.forEach((k, v) -> req.headers().add(k, v));
expectedHeaders.forEach((k, v) -> req.headers().add(k, v));

req.end();
}));
Expand All @@ -828,11 +827,13 @@ public void testRequestHeadersIndividually() {
}

private void testRequestHeaders(boolean individually) {
MultiMap headers = getHeaders(10);
MultiMap expectedHeaders = getHeaders(10);

server.requestHandler(req -> {
assertTrue(headers.size() < req.headers().size());
for (Map.Entry<String, String> entry : headers) {
MultiMap headers = req.headers();
headers.remove("host");
assertEquals(expectedHeaders.size(), expectedHeaders.size());
for (Map.Entry<String, String> entry : expectedHeaders) {
assertEquals(entry.getValue(), req.headers().get(entry.getKey()));
assertEquals(entry.getValue(), req.getHeader(entry.getKey()));
}
Expand All @@ -842,11 +843,11 @@ private void testRequestHeaders(boolean individually) {
server.listen(testAddress, onSuccess(server -> {
HttpClientRequest req = client.request(HttpMethod.GET, testAddress, DEFAULT_HTTP_PORT, DEFAULT_HTTP_HOST, DEFAULT_TEST_URI, resp -> testComplete());
if (individually) {
for (Map.Entry<String, String> header : headers) {
for (Map.Entry<String, String> header : expectedHeaders) {
req.headers().add(header.getKey(), header.getValue());
}
} else {
req.headers().setAll(headers);
req.headers().setAll(expectedHeaders);
}
req.end();
}));
Expand Down Expand Up @@ -3445,9 +3446,9 @@ public void testOtherMethodRequest() {
assertEquals("COPY", r.rawMethod());
r.response().end();
}).listen(testAddress, onSuccess(s -> {
client.request(HttpMethod.OTHER, testAddress, DEFAULT_HTTP_PORT, DEFAULT_HTTP_HOST, "/somepath", resp -> {
client.request(HttpMethod.OTHER, testAddress, DEFAULT_HTTP_PORT, DEFAULT_HTTP_HOST, "/somepath", onSuccess(resp -> {
testComplete();
}).setRawMethod("COPY").end();
})).setRawMethod("COPY").end();
}));
await();
}
Expand Down Expand Up @@ -4642,7 +4643,6 @@ public void testHttpServerResponseHeadersDontContainCROrLF() throws Exception {
resp.headers().forEach(header -> {
String name = header.getKey();
switch (name.toLowerCase()) {
case ":status":
case "content-length":
break;
default:
Expand Down

0 comments on commit 091de1c

Please sign in to comment.