From 86641016878f17b641ebe1907d8a76df7ace34b2 Mon Sep 17 00:00:00 2001
From: jianglai <jianglai@google.com>
Date: Thu, 2 Aug 2018 12:14:22 -0700
Subject: [PATCH] Make web WHOIS more resilient to malformed requests

We are seeing some web WHOIS HTTP(S) requests made to our endpoints without the Host header specified. This is an error according to the HTTP/1.1 spec. However we do not want to spam our logs with errors that are outside of our control. Do not throw and return a 400 response instead.

Also re-worked the logic a bit to only return HSTS headers if we send a redirect response, not any other error responses. The tests are re-arrange to correspond with the logical flow in the code.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=207143230
---
 .../handler/WebWhoisRedirectHandler.java      |  18 ++-
 .../handler/WebWhoisRedirectHandlerTest.java  | 143 ++++++++++++------
 2 files changed, 108 insertions(+), 53 deletions(-)

diff --git a/java/google/registry/proxy/handler/WebWhoisRedirectHandler.java b/java/google/registry/proxy/handler/WebWhoisRedirectHandler.java
index b0b77062a..7e4b9a635 100644
--- a/java/google/registry/proxy/handler/WebWhoisRedirectHandler.java
+++ b/java/google/registry/proxy/handler/WebWhoisRedirectHandler.java
@@ -22,6 +22,7 @@ import static io.netty.handler.codec.http.HttpHeaderNames.LOCATION;
 import static io.netty.handler.codec.http.HttpHeaderValues.KEEP_ALIVE;
 import static io.netty.handler.codec.http.HttpHeaderValues.TEXT_PLAIN;
 import static io.netty.handler.codec.http.HttpMethod.GET;
+import static io.netty.handler.codec.http.HttpResponseStatus.BAD_REQUEST;
 import static io.netty.handler.codec.http.HttpResponseStatus.FORBIDDEN;
 import static io.netty.handler.codec.http.HttpResponseStatus.FOUND;
 import static io.netty.handler.codec.http.HttpResponseStatus.METHOD_NOT_ALLOWED;
@@ -30,6 +31,7 @@ import static io.netty.handler.codec.http.HttpResponseStatus.OK;
 import static io.netty.handler.codec.http.HttpVersion.HTTP_1_1;
 
 import com.google.common.base.Splitter;
+import com.google.common.base.Strings;
 import com.google.common.flogger.FluentLogger;
 import io.netty.channel.ChannelFuture;
 import io.netty.channel.ChannelFutureListener;
@@ -88,6 +90,8 @@ public class WebWhoisRedirectHandler extends SimpleChannelInboundHandler<HttpReq
     // We only support GET, any other HTTP method should result in 405 error.
     if (!msg.method().equals(GET)) {
       response = new DefaultFullHttpResponse(HTTP_1_1, METHOD_NOT_ALLOWED);
+    } else if (Strings.isNullOrEmpty(msg.headers().get(HOST))) {
+      response = new DefaultFullHttpResponse(HTTP_1_1, BAD_REQUEST);
     } else {
       // All HTTP/1.1 request must contain a Host header with the format "host:[port]".
       // See https://tools.ietf.org/html/rfc2616#section-14.23
@@ -104,14 +108,16 @@ public class WebWhoisRedirectHandler extends SimpleChannelInboundHandler<HttpReq
         response = new DefaultFullHttpResponse(HTTP_1_1, isHttps ? FOUND : MOVED_PERMANENTLY);
         String redirectUrl = String.format("https://%s/", isHttps ? redirectHost : host);
         response.headers().set(LOCATION, redirectUrl);
+        // Add HSTS header to HTTPS response.
+        if (isHttps) {
+          response
+              .headers()
+              .set(HSTS_HEADER_NAME, String.format("max-age=%d", HSTS_MAX_AGE.getSeconds()));
+        }
       }
     }
-    // Add HSTS header to HTTPS response.
-    if (isHttps) {
-      response
-          .headers()
-          .set(HSTS_HEADER_NAME, String.format("max-age=%d", HSTS_MAX_AGE.getSeconds()));
-    }
+
+    // Common headers that need to be set on any response.
     response
         .headers()
         .set(CONTENT_TYPE, TEXT_PLAIN)
diff --git a/javatests/google/registry/proxy/handler/WebWhoisRedirectHandlerTest.java b/javatests/google/registry/proxy/handler/WebWhoisRedirectHandlerTest.java
index f7ad2efe7..3bbadcaec 100644
--- a/javatests/google/registry/proxy/handler/WebWhoisRedirectHandlerTest.java
+++ b/javatests/google/registry/proxy/handler/WebWhoisRedirectHandlerTest.java
@@ -19,6 +19,7 @@ import static google.registry.proxy.TestUtils.assertHttpResponseEquivalent;
 import static google.registry.proxy.TestUtils.makeHttpGetRequest;
 import static google.registry.proxy.TestUtils.makeHttpPostRequest;
 import static google.registry.proxy.TestUtils.makeHttpResponse;
+import static io.netty.handler.codec.http.HttpHeaderNames.HOST;
 
 import io.netty.channel.embedded.EmbeddedChannel;
 import io.netty.handler.codec.http.FullHttpRequest;
@@ -35,13 +36,16 @@ public class WebWhoisRedirectHandlerTest {
   private static final String REDIRECT_HOST = "www.example.com";
   private static final String TARGET_HOST = "whois.nic.tld";
 
-  private WebWhoisRedirectHandler redirectHandler;
   private EmbeddedChannel channel;
   private FullHttpRequest request;
   private FullHttpResponse response;
 
+  private void setupChannel(boolean isHttps) {
+    channel = new EmbeddedChannel(new WebWhoisRedirectHandler(isHttps, REDIRECT_HOST));
+  }
+
   private static FullHttpResponse makeRedirectResponse(
-      HttpResponseStatus status, String location, boolean keepAlive, boolean isHttps) {
+      HttpResponseStatus status, String location, boolean keepAlive, boolean hsts) {
     FullHttpResponse response = makeHttpResponse("", status);
     response.headers().set("content-type", "text/plain").set("content-length", "0");
     if (location != null) {
@@ -50,16 +54,66 @@ public class WebWhoisRedirectHandlerTest {
     if (keepAlive) {
       response.headers().set("connection", "keep-alive");
     }
-    if (isHttps) {
+    if (hsts) {
       response.headers().set("Strict-Transport-Security", "max-age=31536000");
     }
     return response;
   }
 
+  // HTTP redirect tests.
+
+  @Test
+  public void testSuccess_http_notGet() {
+    setupChannel(false);
+    request = makeHttpPostRequest("", TARGET_HOST, "/");
+    // No inbound message passed to the next handler.
+    assertThat(channel.writeInbound(request)).isFalse();
+    response = channel.readOutbound();
+    assertHttpResponseEquivalent(
+        response, makeRedirectResponse(HttpResponseStatus.METHOD_NOT_ALLOWED, null, true, false));
+    assertThat(channel.isActive()).isTrue();
+  }
+
+  @Test
+  public void testSuccess_http_badHost() {
+    setupChannel(false);
+    request = makeHttpGetRequest("", "/");
+    // No inbound message passed to the next handler.
+    assertThat(channel.writeInbound(request)).isFalse();
+    response = channel.readOutbound();
+    assertHttpResponseEquivalent(
+        response, makeRedirectResponse(HttpResponseStatus.BAD_REQUEST, null, true, false));
+    assertThat(channel.isActive()).isTrue();
+  }
+
+  @Test
+  public void testSuccess_http_noHost() {
+    setupChannel(false);
+    request = makeHttpGetRequest("", "/");
+    request.headers().remove(HOST);
+    // No inbound message passed to the next handler.
+    assertThat(channel.writeInbound(request)).isFalse();
+    response = channel.readOutbound();
+    assertHttpResponseEquivalent(
+        response, makeRedirectResponse(HttpResponseStatus.BAD_REQUEST, null, true, false));
+    assertThat(channel.isActive()).isTrue();
+  }
+
+  @Test
+  public void testSuccess_http_healthCheck() {
+    setupChannel(false);
+    request = makeHttpPostRequest("", TARGET_HOST, "/");
+    // No inbound message passed to the next handler.
+    assertThat(channel.writeInbound(request)).isFalse();
+    response = channel.readOutbound();
+    assertHttpResponseEquivalent(
+        response, makeRedirectResponse(HttpResponseStatus.METHOD_NOT_ALLOWED, null, true, false));
+    assertThat(channel.isActive()).isTrue();
+  }
+
   @Test
   public void testSuccess_http_redirectToHttps() {
-    redirectHandler = new WebWhoisRedirectHandler(false, REDIRECT_HOST);
-    channel = new EmbeddedChannel(redirectHandler);
+    setupChannel(false);
     request = makeHttpGetRequest(TARGET_HOST, "/");
     // No inbound message passed to the next handler.
     assertThat(channel.writeInbound(request)).isFalse();
@@ -73,8 +127,7 @@ public class WebWhoisRedirectHandlerTest {
 
   @Test
   public void testSuccess_http_redirectToHttps_hostAndPort() {
-    redirectHandler = new WebWhoisRedirectHandler(false, REDIRECT_HOST);
-    channel = new EmbeddedChannel(redirectHandler);
+    setupChannel(false);
     request = makeHttpGetRequest(TARGET_HOST + ":80", "/");
     // No inbound message passed to the next handler.
     assertThat(channel.writeInbound(request)).isFalse();
@@ -88,8 +141,7 @@ public class WebWhoisRedirectHandlerTest {
 
   @Test
   public void testSuccess_http_redirectToHttps_noKeepAlive() {
-    redirectHandler = new WebWhoisRedirectHandler(false, REDIRECT_HOST);
-    channel = new EmbeddedChannel(redirectHandler);
+    setupChannel(false);
     request = makeHttpGetRequest(TARGET_HOST, "/");
     request.headers().set("connection", "close");
     // No inbound message passed to the next handler.
@@ -102,10 +154,11 @@ public class WebWhoisRedirectHandlerTest {
     assertThat(channel.isActive()).isFalse();
   }
 
+  // HTTPS redirect tests.
+
   @Test
-  public void testSuccess_http_notGet() {
-    redirectHandler = new WebWhoisRedirectHandler(false, REDIRECT_HOST);
-    channel = new EmbeddedChannel(redirectHandler);
+  public void testSuccess_https_notGet() {
+    setupChannel(true);
     request = makeHttpPostRequest("", TARGET_HOST, "/");
     // No inbound message passed to the next handler.
     assertThat(channel.writeInbound(request)).isFalse();
@@ -116,22 +169,45 @@ public class WebWhoisRedirectHandlerTest {
   }
 
   @Test
-  public void testSuccess_http_healthCheck() {
-    redirectHandler = new WebWhoisRedirectHandler(false, REDIRECT_HOST);
-    channel = new EmbeddedChannel(redirectHandler);
-    request = makeHttpPostRequest("", TARGET_HOST, "/");
+  public void testSuccess_https_badHost() {
+    setupChannel(true);
+    request = makeHttpGetRequest("", "/");
     // No inbound message passed to the next handler.
     assertThat(channel.writeInbound(request)).isFalse();
     response = channel.readOutbound();
     assertHttpResponseEquivalent(
-        response, makeRedirectResponse(HttpResponseStatus.METHOD_NOT_ALLOWED, null, true, false));
+        response, makeRedirectResponse(HttpResponseStatus.BAD_REQUEST, null, true, false));
+    assertThat(channel.isActive()).isTrue();
+  }
+
+  @Test
+  public void testSuccess_https_noHost() {
+    setupChannel(true);
+    request = makeHttpGetRequest("", "/");
+    request.headers().remove(HOST);
+    // No inbound message passed to the next handler.
+    assertThat(channel.writeInbound(request)).isFalse();
+    response = channel.readOutbound();
+    assertHttpResponseEquivalent(
+        response, makeRedirectResponse(HttpResponseStatus.BAD_REQUEST, null, true, false));
+    assertThat(channel.isActive()).isTrue();
+  }
+
+  @Test
+  public void testSuccess_https_healthCheck() {
+    setupChannel(true);
+    request = makeHttpGetRequest("health-check.invalid", "/");
+    // No inbound message passed to the next handler.
+    assertThat(channel.writeInbound(request)).isFalse();
+    response = channel.readOutbound();
+    assertHttpResponseEquivalent(
+        response, makeRedirectResponse(HttpResponseStatus.FORBIDDEN, null, true, false));
     assertThat(channel.isActive()).isTrue();
   }
 
   @Test
   public void testSuccess_https_redirectToDestination() {
-    redirectHandler = new WebWhoisRedirectHandler(true, REDIRECT_HOST);
-    channel = new EmbeddedChannel(redirectHandler);
+    setupChannel(true);
     request = makeHttpGetRequest(TARGET_HOST, "/");
     // No inbound message passed to the next handler.
     assertThat(channel.writeInbound(request)).isFalse();
@@ -144,8 +220,7 @@ public class WebWhoisRedirectHandlerTest {
 
   @Test
   public void testSuccess_https_redirectToDestination_noKeepAlive() {
-    redirectHandler = new WebWhoisRedirectHandler(true, REDIRECT_HOST);
-    channel = new EmbeddedChannel(redirectHandler);
+    setupChannel(true);
     request = makeHttpGetRequest(TARGET_HOST, "/");
     request.headers().set("connection", "close");
     // No inbound message passed to the next handler.
@@ -156,30 +231,4 @@ public class WebWhoisRedirectHandlerTest {
         makeRedirectResponse(HttpResponseStatus.FOUND, "https://www.example.com/", false, true));
     assertThat(channel.isActive()).isFalse();
   }
-
-  @Test
-  public void testSuccess_https_notGet() {
-    redirectHandler = new WebWhoisRedirectHandler(true, REDIRECT_HOST);
-    channel = new EmbeddedChannel(redirectHandler);
-    request = makeHttpPostRequest("", TARGET_HOST, "/");
-    // No inbound message passed to the next handler.
-    assertThat(channel.writeInbound(request)).isFalse();
-    response = channel.readOutbound();
-    assertHttpResponseEquivalent(
-        response, makeRedirectResponse(HttpResponseStatus.METHOD_NOT_ALLOWED, null, true, true));
-    assertThat(channel.isActive()).isTrue();
-  }
-
-  @Test
-  public void testSuccess_https_healthCheck() {
-    redirectHandler = new WebWhoisRedirectHandler(true, REDIRECT_HOST);
-    channel = new EmbeddedChannel(redirectHandler);
-    request = makeHttpGetRequest("health-check.invalid", "/");
-    // No inbound message passed to the next handler.
-    assertThat(channel.writeInbound(request)).isFalse();
-    response = channel.readOutbound();
-    assertHttpResponseEquivalent(
-        response, makeRedirectResponse(HttpResponseStatus.FORBIDDEN, null, true, true));
-    assertThat(channel.isActive()).isTrue();
-  }
 }