Merge pull request #12855 from vespa-engine/bjorncs/jdisc-unknown-methods

Return status code 405 for unknown HTTP methods

2 files changed, 32 insertions, 6 deletions

diff --git a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/JDiscHttpServlet.java b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/JDiscHttpServlet.java
index 5cbe7320f0e..a6b2deb4681 100644
--- a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/JDiscHttpServlet.java
+++ b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/JDiscHttpServlet.java
@@ -4,6 +4,7 @@ package com.yahoo.jdisc.http.server.jetty;
 import com.yahoo.container.logging.AccessLogEntry;
 import com.yahoo.jdisc.Metric;
 import com.yahoo.jdisc.handler.OverloadException;
+import com.yahoo.jdisc.http.HttpRequest.Method;
 
 import javax.servlet.ServletException;
 import javax.servlet.annotation.WebServlet;
@@ -11,12 +12,12 @@ import javax.servlet.http.HttpServlet;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 import java.io.IOException;
-import java.util.Arrays;
 import java.util.Enumeration;
-import java.util.HashSet;
 import java.util.Set;
 import java.util.logging.Level;
 import java.util.logging.Logger;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
 
 import static com.yahoo.jdisc.http.core.HttpServletRequestUtils.getConnection;
 
@@ -78,8 +79,6 @@ class JDiscHttpServlet extends HttpServlet {
         dispatchHttpRequest(request, response);
     }
 
-    private static final Set<String> JETTY_UNSUPPORTED_METHODS = new HashSet<>(Arrays.asList("PATCH"));
-
     /**
      * Override to set connector attribute before the request becomes an upgrade request in the web socket case.
      * (After the upgrade, the HttpConnection is no longer available.)
@@ -93,10 +92,20 @@ class JDiscHttpServlet extends HttpServlet {
         context.metric.add(JettyHttpServer.Metrics.NUM_REQUESTS, 1, metricContext);
         context.metric.add(JettyHttpServer.Metrics.JDISC_HTTP_REQUESTS, 1, metricContext);
 
-        if (JETTY_UNSUPPORTED_METHODS.contains(request.getMethod().toUpperCase())) {
+
+        Set<String> servletSupportedMethods =
+                Stream.of(Method.OPTIONS, Method.GET, Method.HEAD, Method.POST, Method.PUT, Method.DELETE, Method.TRACE)
+                        .map(Method::name)
+                        .collect(Collectors.toSet());
+        String method = request.getMethod().toUpperCase();
+        if (servletSupportedMethods.contains(method)) {
+            super.service(request, response);
+        } else if (method.equals(Method.PATCH.name())) {
+            // PATCH method is not handled by the Servlet spec
             dispatchHttpRequest(request, response);
         } else {
-            super.service(request, response);
+            // Divergence from HTTP / Servlet spec: JDisc returns 405 for both unknown and known (but unsupported) methods.
+            response.sendError(HttpServletResponse.SC_METHOD_NOT_ALLOWED);
         }
     }
 
diff --git a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/JDiscHttpServletTest.java b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/JDiscHttpServletTest.java
index 7a645337967..230f59cbb34 100644
--- a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/JDiscHttpServletTest.java
+++ b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/JDiscHttpServletTest.java
@@ -14,11 +14,14 @@ import org.apache.http.client.methods.HttpOptions;
 import org.apache.http.client.methods.HttpPatch;
 import org.apache.http.client.methods.HttpPost;
 import org.apache.http.client.methods.HttpPut;
+import org.apache.http.client.methods.HttpRequestBase;
 import org.apache.http.client.methods.HttpTrace;
 import org.junit.Test;
 
+import java.io.IOException;
 import java.net.URI;
 
+import static com.yahoo.jdisc.Response.Status.METHOD_NOT_ALLOWED;
 import static com.yahoo.jdisc.Response.Status.OK;
 import static org.hamcrest.CoreMatchers.is;
 import static org.hamcrest.MatcherAssert.assertThat;
@@ -51,6 +54,15 @@ public class JDiscHttpServletTest {
         assertThat(driver.close(), is(true));
     }
 
+    @Test
+    public void requireThatServerResponds405ToUnknownMethods() throws IOException {
+        TestDriver driver = TestDrivers.newInstance(newEchoHandler());
+        final URI uri = driver.client().newUri("/status.html");
+        driver.client().execute(new UnknownMethodHttpRequest(uri))
+                .expectStatusCode(is(METHOD_NOT_ALLOWED));
+        assertThat(driver.close(), is(true));
+    }
+
     private static RequestHandler newEchoHandler() {
         return new AbstractRequestHandler() {
 
@@ -60,4 +72,9 @@ public class JDiscHttpServletTest {
             }
         };
     }
+
+    private static class UnknownMethodHttpRequest extends HttpRequestBase {
+        UnknownMethodHttpRequest(URI uri) { setURI(uri); }
+        @Override public String getMethod() { return "UNKNOWN_METHOD"; }
+    }
 }