Merge pull request #5502 from vespa-engine/bjorncs/fix-invalid-uri-handling

Return bad request response when encoding in query is invalid

2 files changed, 34 insertions, 10 deletions

diff --git a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/HttpRequestFactory.java b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/HttpRequestFactory.java
index a005ea7d96e..95f26e8bc1b 100644
--- a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/HttpRequestFactory.java
+++ b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/HttpRequestFactory.java
@@ -5,6 +5,7 @@ import com.yahoo.jdisc.Response;
 import com.yahoo.jdisc.http.HttpRequest;
 import com.yahoo.jdisc.http.servlet.ServletRequest;
 import com.yahoo.jdisc.service.CurrentContainer;
+import org.eclipse.jetty.util.Utf8Appendable;
 
 import javax.servlet.http.HttpServletRequest;
 import java.net.InetSocketAddress;
@@ -21,15 +22,19 @@ import static com.yahoo.jdisc.http.core.HttpServletRequestUtils.getConnection;
 class HttpRequestFactory {
 
     public static HttpRequest newJDiscRequest(CurrentContainer container, HttpServletRequest servletRequest) {
-        HttpRequest httpRequest = HttpRequest.newServerRequest(
-                container,
-                getUri(servletRequest),
-                HttpRequest.Method.valueOf(servletRequest.getMethod()),
-                HttpRequest.Version.fromString(servletRequest.getProtocol()),
-                new InetSocketAddress(servletRequest.getRemoteAddr(), servletRequest.getRemotePort()),
-                getConnection(servletRequest).getCreatedTimeStamp());
-        httpRequest.context().put(ServletRequest.JDISC_REQUEST_X509CERT, getCertChain(servletRequest));
-        return httpRequest;
+        try {
+            HttpRequest httpRequest = HttpRequest.newServerRequest(
+                    container,
+                    getUri(servletRequest),
+                    HttpRequest.Method.valueOf(servletRequest.getMethod()),
+                    HttpRequest.Version.fromString(servletRequest.getProtocol()),
+                    new InetSocketAddress(servletRequest.getRemoteAddr(), servletRequest.getRemotePort()),
+                    getConnection(servletRequest).getCreatedTimeStamp());
+            httpRequest.context().put(ServletRequest.JDISC_REQUEST_X509CERT, getCertChain(servletRequest));
+            return httpRequest;
+        } catch (Utf8Appendable.NotUtf8Exception e) {
+            throw createBadQueryException(e);
+        }
     }
 
     public static URI getUri(HttpServletRequest servletRequest) {
@@ -37,10 +42,14 @@ class HttpRequestFactory {
         try {
             return URI.create(servletRequest.getRequestURL() + (query != null ? '?' + query : ""));
         } catch (IllegalArgumentException e) {
-            throw new RequestException(Response.Status.BAD_REQUEST, "Query violates RFC 2396", e);
+            throw createBadQueryException(e);
         }
     }
 
+    private static RequestException createBadQueryException(IllegalArgumentException e) {
+        return new RequestException(Response.Status.BAD_REQUEST, "Query violates RFC 2396: " + e.getMessage(), e);
+    }
+
     public static void copyHeaders(HttpServletRequest from, HttpRequest to) {
         for (Enumeration<String> it = from.getHeaderNames(); it.hasMoreElements(); ) {
             String key = it.nextElement();
@@ -50,6 +59,7 @@ class HttpRequestFactory {
         }
     }
 
+    // TODO Remove this ugly, non-complete escaping in Vespa 7
     private static String extraQuote(String queryString) {
         // TODO: Use an URI builder
         if (queryString == null) return null;
diff --git a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/HttpRequestFactoryTest.java b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/HttpRequestFactoryTest.java
index 476718ac906..39ad25244df 100644
--- a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/HttpRequestFactoryTest.java
+++ b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/HttpRequestFactoryTest.java
@@ -513,6 +513,20 @@ public class HttpRequestFactoryTest {
         }
     }
 
+    @Test
+    public final void illegal_unicode_in_query_throws_requestexception() {
+        try {
+            HttpRequestFactory.newJDiscRequest(
+                    new MockContainer(),
+                    new MockRequest("http://example.com/search?query=%c0%ae"));
+            fail("Above statement should throw");
+        } catch (RequestException e) {
+            assertThat(e.getResponseStatus(), is(Response.Status.BAD_REQUEST));
+            assertThat(e.getMessage(), equalTo("Query violates RFC 2396: Not valid UTF8! byte C0 in state 0"));
+        }
+    }
+
+
     private static final class MockContainer implements CurrentContainer {
 
         @Override