This is an automated email from the ASF dual-hosted git repository.

markt pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/tomcat.git


The following commit(s) were added to refs/heads/master by this push:
     new a3fc47d  Return correct response code for HTTP/2 when method is not 
implemented
a3fc47d is described below

commit a3fc47dd70725387679a693a00fcd64c876a4a56
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Tue Jun 16 14:26:49 2020 +0100

    Return correct response code for HTTP/2 when method is not implemented
---
 java/jakarta/servlet/http/HttpServlet.java         |  36 +++-----
 test/jakarta/servlet/http/TestHttpServlet.java     | 102 +++++++++++++++++++++
 .../coyote/http11/TestHttp11InputBufferCRLF.java   |   1 -
 test/org/apache/coyote/http2/TestHttpServlet.java  |  59 ++++++++++++
 webapps/docs/changelog.xml                         |   6 ++
 5 files changed, 181 insertions(+), 23 deletions(-)

diff --git a/java/jakarta/servlet/http/HttpServlet.java 
b/java/jakarta/servlet/http/HttpServlet.java
index fde9847..ba98853 100644
--- a/java/jakarta/servlet/http/HttpServlet.java
+++ b/java/jakarta/servlet/http/HttpServlet.java
@@ -171,13 +171,8 @@ public abstract class HttpServlet extends GenericServlet {
     protected void doGet(HttpServletRequest req, HttpServletResponse resp)
         throws ServletException, IOException
     {
-        String protocol = req.getProtocol();
         String msg = lStrings.getString("http.method_get_not_supported");
-        if (protocol.endsWith("1.1")) {
-            resp.sendError(HttpServletResponse.SC_METHOD_NOT_ALLOWED, msg);
-        } else {
-            resp.sendError(HttpServletResponse.SC_BAD_REQUEST, msg);
-        }
+        sendMethodNotAllowed(req, resp, msg);
     }
 
 
@@ -311,13 +306,8 @@ public abstract class HttpServlet extends GenericServlet {
     protected void doPost(HttpServletRequest req, HttpServletResponse resp)
         throws ServletException, IOException {
 
-        String protocol = req.getProtocol();
         String msg = lStrings.getString("http.method_post_not_supported");
-        if (protocol.endsWith("1.1")) {
-            resp.sendError(HttpServletResponse.SC_METHOD_NOT_ALLOWED, msg);
-        } else {
-            resp.sendError(HttpServletResponse.SC_BAD_REQUEST, msg);
-        }
+        sendMethodNotAllowed(req, resp, msg);
     }
 
 
@@ -366,13 +356,8 @@ public abstract class HttpServlet extends GenericServlet {
     protected void doPut(HttpServletRequest req, HttpServletResponse resp)
         throws ServletException, IOException {
 
-        String protocol = req.getProtocol();
         String msg = lStrings.getString("http.method_put_not_supported");
-        if (protocol.endsWith("1.1")) {
-            resp.sendError(HttpServletResponse.SC_METHOD_NOT_ALLOWED, msg);
-        } else {
-            resp.sendError(HttpServletResponse.SC_BAD_REQUEST, msg);
-        }
+        sendMethodNotAllowed(req, resp, msg);
     }
 
 
@@ -414,12 +399,19 @@ public abstract class HttpServlet extends GenericServlet {
                             HttpServletResponse resp)
         throws ServletException, IOException {
 
-        String protocol = req.getProtocol();
         String msg = lStrings.getString("http.method_delete_not_supported");
-        if (protocol.endsWith("1.1")) {
-            resp.sendError(HttpServletResponse.SC_METHOD_NOT_ALLOWED, msg);
-        } else {
+        sendMethodNotAllowed(req, resp, msg);
+    }
+
+
+    private void sendMethodNotAllowed(HttpServletRequest req, 
HttpServletResponse resp, String msg) throws IOException {
+        String protocol = req.getProtocol();
+        // Note: Tomcat reports "" for HTTP/0.9 although some implementations
+        //       may report HTTP/0.9
+        if (protocol.length() == 0 || protocol.endsWith("0.9") || 
protocol.endsWith("1.0")) {
             resp.sendError(HttpServletResponse.SC_BAD_REQUEST, msg);
+        } else {
+            resp.sendError(HttpServletResponse.SC_METHOD_NOT_ALLOWED, msg);
         }
     }
 
diff --git a/test/jakarta/servlet/http/TestHttpServlet.java 
b/test/jakarta/servlet/http/TestHttpServlet.java
index 2473799..f1ae25b 100644
--- a/test/jakarta/servlet/http/TestHttpServlet.java
+++ b/test/jakarta/servlet/http/TestHttpServlet.java
@@ -28,7 +28,10 @@ import jakarta.servlet.ServletException;
 import org.junit.Assert;
 import org.junit.Test;
 
+import org.apache.catalina.Context;
 import org.apache.catalina.core.StandardContext;
+import org.apache.catalina.startup.SimpleHttpClient;
+import org.apache.catalina.startup.TesterServlet;
 import org.apache.catalina.startup.Tomcat;
 import org.apache.catalina.startup.TomcatBaseTest;
 import org.apache.tomcat.util.buf.ByteChunk;
@@ -191,6 +194,105 @@ public class TestHttpServlet extends TomcatBaseTest {
     }
 
 
+    @Test
+    public void testUnimplementedMethodHttp09() throws Exception {
+        doTestUnimplementedMethod("0.9");
+    }
+
+
+    @Test
+    public void testUnimplementedMethodHttp10() throws Exception {
+        doTestUnimplementedMethod("1.0");
+    }
+
+
+    @Test
+    public void testUnimplementedMethodHttp11() throws Exception {
+        doTestUnimplementedMethod("1.1");
+    }
+
+
+    /*
+     * See org.aoache.coyote.http2.TestHttpServlet for the HTTP/2 version of
+     * this test. It was placed in that package because it needed access to
+     * package private classes.
+     */
+
+
+    private void doTestUnimplementedMethod(String httpVersion) {
+        StringBuilder request = new StringBuilder("PUT /test");
+        boolean isHttp09 = "0.9".equals(httpVersion);
+        boolean isHttp10 = "1.0".equals(httpVersion);
+
+        if (!isHttp09) {
+            request.append(" HTTP/");
+            request.append(httpVersion);
+        }
+        request.append(SimpleHttpClient.CRLF);
+
+        request.append("Host: localhost:8080");
+        request.append(SimpleHttpClient.CRLF);
+
+        request.append("Connection: close");
+        request.append(SimpleHttpClient.CRLF);
+
+        request.append(SimpleHttpClient.CRLF);
+
+        Client client = new Client(request.toString(), 
"0.9".equals(httpVersion));
+
+        client.doRequest();
+
+        if (isHttp09) {
+            Assert.assertTrue( client.getResponseBody(), 
client.getResponseBody().contains(" 400 "));
+        } else if (isHttp10) {
+            Assert.assertTrue(client.getResponseLine(), 
client.isResponse400());
+        } else {
+            Assert.assertTrue(client.getResponseLine(), 
client.isResponse405());
+        }
+    }
+
+
+    private class Client extends SimpleHttpClient {
+
+        public Client(String request, boolean isHttp09) {
+            setRequest(new String[] {request});
+            setUseHttp09(isHttp09);
+        }
+
+        private Exception doRequest() {
+
+            Tomcat tomcat = getTomcatInstance();
+
+            Context root = tomcat.addContext("", TEMP_DIR);
+            Tomcat.addServlet(root, "TesterServlet", new TesterServlet());
+            root.addServletMappingDecoded("/test", "TesterServlet");
+
+            try {
+                tomcat.start();
+                setPort(tomcat.getConnector().getLocalPort());
+                setRequestPause(20);
+
+                // Open connection
+                connect();
+
+                processRequest(); // blocks until response has been read
+
+                // Close the connection
+                disconnect();
+            } catch (Exception e) {
+                e.printStackTrace();
+                return e;
+            }
+            return null;
+        }
+
+        @Override
+        public boolean isResponseBodyOK() {
+            return false;
+        }
+    }
+
+
     private static class Bug57602ServletOuter extends HttpServlet {
 
         private static final long serialVersionUID = 1L;
diff --git a/test/org/apache/coyote/http11/TestHttp11InputBufferCRLF.java 
b/test/org/apache/coyote/http11/TestHttp11InputBufferCRLF.java
index ee033a1..786a95a 100644
--- a/test/org/apache/coyote/http11/TestHttp11InputBufferCRLF.java
+++ b/test/org/apache/coyote/http11/TestHttp11InputBufferCRLF.java
@@ -186,7 +186,6 @@ public class TestHttp11InputBufferCRLF extends 
TomcatBaseTest {
                 // Open connection
                 connect();
 
-                setRequest(request);
                 processRequest(); // blocks until response has been read
 
                 // Close the connection
diff --git a/test/org/apache/coyote/http2/TestHttpServlet.java 
b/test/org/apache/coyote/http2/TestHttpServlet.java
new file mode 100644
index 0000000..99209b5
--- /dev/null
+++ b/test/org/apache/coyote/http2/TestHttpServlet.java
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You 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 org.apache.coyote.http2;
+
+import java.nio.ByteBuffer;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+/*
+ * Implement here rather than jakarta.servlet.http.TestHttpServley because it
+ * needs access to package private classes.
+ */
+public class TestHttpServlet extends Http2TestBase {
+
+    @Test
+    public void testUnimplementedMethodHttp2() throws Exception {
+        http2Connect();
+
+        // Build a POST request for a Servlet that does not implement POST
+        byte[] headersFrameHeader = new byte[9];
+        ByteBuffer headersPayload = ByteBuffer.allocate(128);
+        byte[] dataFrameHeader = new byte[9];
+        ByteBuffer dataPayload = ByteBuffer.allocate(0);
+
+        buildPostRequest(headersFrameHeader, headersPayload, false, null, -1, 
"/empty", dataFrameHeader, dataPayload,
+                null, null, null, 3);
+
+        // Write the headers
+        writeFrame(headersFrameHeader, headersPayload);
+        // Body
+        writeFrame(dataFrameHeader, dataPayload);
+
+        parser.readFrame(true);
+        parser.readFrame(true);
+
+        String trace = output.getTrace();
+        String[] lines = trace.split("\n");
+
+        // Check the response code
+        Assert.assertEquals("3-HeadersStart", lines[0]);
+        Assert.assertEquals("3-Header-[:status]-[405]", lines[1]);
+    }
+
+}
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index b181385..0aef8ae 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -69,6 +69,12 @@
         Add <code>application/wasm</code> to the media types recognised by
         Tomcat. Based on a PR by Thiago Henrique H├╝pner. (markt)
       </add>
+      <fix>
+        Fix a bug in <code>HttpServlet</code> so that a <code>405</code>
+        response is returned for an HTTP/2 request if the mapped servlet does
+        implement the requested method rather than the more general
+        <code>400</code> response. (markt)
+      </fix>
     </changelog>
   </subsection>
   <subsection name="Coyote">


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Reply via email to