This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch 7.0.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit aed9453c710bafce9d69c5d4ea02363d371b8a32
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Fri Jun 28 22:40:14 2019 +0100
Align use of Allow header and HTTP 405 status code
Modify the Default and WebDAV Servlets so that a 405 status code is
returned for PUT and DELETE requests when disabled via the readonly
initialisation parameter.
Align the contents of the <code>Allow</code> header with the response
code for the Default and WebDAV Servlets. For any given resource a
method that returns a 405 status code will not be listed in the Allow
header and a method listed in the Allow header will not return a 405
status code.
Based on a patch suggested by Ken Dombeck.
---
.../apache/catalina/servlets/DefaultServlet.java | 37 +++--
.../apache/catalina/servlets/WebdavServlet.java | 76 +++++-----
.../catalina/servlets/ServletOptionsBaseTest.java | 161 +++++++++++++++++++++
.../servlets/TestDefaultServletOptions.java | 61 ++++++++
.../servlets/TestWebdavServletOptions.java | 62 ++++++++
.../apache/catalina/startup/SimpleHttpClient.java | 39 +++--
webapps/docs/changelog.xml | 12 ++
7 files changed, 384 insertions(+), 64 deletions(-)
diff --git a/java/org/apache/catalina/servlets/DefaultServlet.java
b/java/org/apache/catalina/servlets/DefaultServlet.java
index 18d175f..8171ac0 100644
--- a/java/org/apache/catalina/servlets/DefaultServlet.java
+++ b/java/org/apache/catalina/servlets/DefaultServlet.java
@@ -480,24 +480,35 @@ public class DefaultServlet extends HttpServlet {
protected void doOptions(HttpServletRequest req, HttpServletResponse resp)
throws ServletException, IOException {
+ resp.setHeader("Allow", determineMethodsAllowed(req));
+ }
+
+
+ protected String determineMethodsAllowed(HttpServletRequest req) {
StringBuilder allow = new StringBuilder();
- // There is a doGet method
- allow.append("GET, HEAD");
- // There is a doPost
- allow.append(", POST");
- // There is a doPut
- allow.append(", PUT");
- // There is a doDelete
- allow.append(", DELETE");
+
+ // Start with methods that are always allowed
+ allow.append("OPTIONS, GET, HEAD, POST");
+
+ // PUT and DELETE depend on readonly
+ if (!readOnly) {
+ allow.append(", PUT, DELETE");
+ }
+
// Trace - assume disabled unless we can prove otherwise
if (req instanceof RequestFacade &&
((RequestFacade) req).getAllowTrace()) {
allow.append(", TRACE");
}
- // Always allow options
- allow.append(", OPTIONS");
- resp.setHeader("Allow", allow.toString());
+ return allow.toString();
+ }
+
+
+ protected void sendNotAllowed(HttpServletRequest req, HttpServletResponse
resp)
+ throws IOException {
+ resp.addHeader("Allow", determineMethodsAllowed(req));
+ resp.sendError(WebdavStatus.SC_METHOD_NOT_ALLOWED);
}
@@ -532,7 +543,7 @@ public class DefaultServlet extends HttpServlet {
throws ServletException, IOException {
if (readOnly) {
- resp.sendError(HttpServletResponse.SC_FORBIDDEN);
+ sendNotAllowed(req, resp);
return;
}
@@ -695,7 +706,7 @@ public class DefaultServlet extends HttpServlet {
throws ServletException, IOException {
if (readOnly) {
- resp.sendError(HttpServletResponse.SC_FORBIDDEN);
+ sendNotAllowed(req, resp);
return;
}
diff --git a/java/org/apache/catalina/servlets/WebdavServlet.java
b/java/org/apache/catalina/servlets/WebdavServlet.java
index 2bf42bc..bcc795d 100644
--- a/java/org/apache/catalina/servlets/WebdavServlet.java
+++ b/java/org/apache/catalina/servlets/WebdavServlet.java
@@ -495,11 +495,7 @@ public class WebdavServlet extends DefaultServlet {
throws ServletException, IOException {
resp.addHeader("DAV", "1,2");
-
- StringBuilder methodsAllowed = determineMethodsAllowed(resources,
- req);
-
- resp.addHeader("Allow", methodsAllowed.toString());
+ resp.addHeader("Allow", determineMethodsAllowed(req));
resp.addHeader("MS-Author-Via", "DAV");
}
@@ -515,12 +511,7 @@ public class WebdavServlet extends DefaultServlet {
throws ServletException, IOException {
if (!listings) {
- // Get allowed methods
- StringBuilder methodsAllowed = determineMethodsAllowed(resources,
- req);
-
- resp.addHeader("Allow", methodsAllowed.toString());
- resp.sendError(WebdavStatus.SC_METHOD_NOT_ALLOWED);
+ sendNotAllowed(req, resp);
return;
}
@@ -804,13 +795,7 @@ public class WebdavServlet extends DefaultServlet {
// Can't create a collection if a resource already exists at the given
// path
if (exists) {
- // Get allowed methods
- StringBuilder methodsAllowed = determineMethodsAllowed(resources,
- req);
-
- resp.addHeader("Allow", methodsAllowed.toString());
-
- resp.sendError(WebdavStatus.SC_METHOD_NOT_ALLOWED);
+ sendNotAllowed(req, resp);
return;
}
@@ -847,14 +832,14 @@ public class WebdavServlet extends DefaultServlet {
result = false;
}
- if (!result) {
- resp.sendError(WebdavStatus.SC_CONFLICT,
- WebdavStatus.getStatusText
- (WebdavStatus.SC_CONFLICT));
- } else {
+ if (result) {
resp.setStatus(WebdavStatus.SC_CREATED);
// Removing any lock-null resource which would be present
lockNullResources.remove(path);
+ } else {
+ resp.sendError(WebdavStatus.SC_CONFLICT,
+ WebdavStatus.getStatusText
+ (WebdavStatus.SC_CONFLICT));
}
}
@@ -871,7 +856,7 @@ public class WebdavServlet extends DefaultServlet {
throws ServletException, IOException {
if (readOnly) {
- resp.sendError(WebdavStatus.SC_FORBIDDEN);
+ sendNotAllowed(req, resp);
return;
}
@@ -904,6 +889,17 @@ public class WebdavServlet extends DefaultServlet {
}
String path = getRelativePath(req);
+ Object object = null;
+ try {
+ object = resources.lookup(path);
+ } catch (NamingException e) {
+ // Ignore
+ }
+
+ if (object instanceof DirContext) {
+ sendNotAllowed(req, resp);
+ return;
+ }
super.doPut(req, resp);
@@ -2659,42 +2655,46 @@ public class WebdavServlet extends DefaultServlet {
*
* @return The allowed HTTP methods
*/
- private StringBuilder determineMethodsAllowed(DirContext dirContext,
- HttpServletRequest req) {
+ @Override
+ protected String determineMethodsAllowed(HttpServletRequest req) {
- StringBuilder methodsAllowed = new StringBuilder();
boolean exists = true;
Object object = null;
try {
- String path = getRelativePath(req);
-
- object = dirContext.lookup(path);
+ object = resources.lookup(getRelativePath(req));
} catch (NamingException e) {
exists = false;
}
- if (!exists) {
- methodsAllowed.append("OPTIONS, MKCOL, PUT, LOCK");
- return methodsAllowed;
+ // These methods are always allowed. They may return a 404 (not a 405)
+ // if the resource does not exist.
+ StringBuilder methodsAllowed = new StringBuilder(
+ "OPTIONS, GET, POST, HEAD");
+
+ if (!readOnly) {
+ methodsAllowed.append(", DELETE");
+ if (!(object instanceof DirContext)) {
+ methodsAllowed.append(", PUT");
+ }
}
- methodsAllowed.append("OPTIONS, GET, HEAD, POST, DELETE");
// Trace - assume disabled unless we can prove otherwise
if (req instanceof RequestFacade &&
((RequestFacade) req).getAllowTrace()) {
methodsAllowed.append(", TRACE");
}
- methodsAllowed.append(", PROPPATCH, COPY, MOVE, LOCK, UNLOCK");
+
+ methodsAllowed.append(", LOCK, UNLOCK, PROPPATCH, COPY, MOVE");
if (listings) {
methodsAllowed.append(", PROPFIND");
}
- if (!(object instanceof DirContext)) {
- methodsAllowed.append(", PUT");
+ if (!exists) {
+ methodsAllowed.append(", MKCOL");
}
- return methodsAllowed;
+ return methodsAllowed.toString();
}
diff --git a/test/org/apache/catalina/servlets/ServletOptionsBaseTest.java
b/test/org/apache/catalina/servlets/ServletOptionsBaseTest.java
new file mode 100644
index 0000000..e121f37
--- /dev/null
+++ b/test/org/apache/catalina/servlets/ServletOptionsBaseTest.java
@@ -0,0 +1,161 @@
+/*
+ * 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.catalina.servlets;
+
+
+import java.io.File;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.Set;
+
+import javax.servlet.Servlet;
+
+import org.junit.Assert;
+import org.junit.Test;
+import org.junit.runners.Parameterized.Parameter;
+
+import static org.apache.catalina.startup.SimpleHttpClient.CRLF;
+import org.apache.catalina.Wrapper;
+import org.apache.catalina.startup.SimpleHttpClient;
+import org.apache.catalina.startup.Tomcat;
+import org.apache.catalina.startup.TomcatBaseTest;
+
+public abstract class ServletOptionsBaseTest extends TomcatBaseTest {
+
+ protected static final String COLLECTION_NAME = "collection";
+ protected static final String FILE_NAME = "file";
+ protected static final String UNKNOWN_NAME = "unknown";
+
+ @Parameter(0)
+ public boolean listings;
+
+ @Parameter(1)
+ public boolean readonly;
+
+ @Parameter(2)
+ public boolean trace;
+
+ @Parameter(3)
+ public String url;
+
+ @Parameter(4)
+ public String method;
+
+
+ /*
+ * Check that methods returned by OPTIONS are consistent with the return
+ * http status code.
+ * Method not present in options response -> 405 expected
+ * Method present in options response -> anything other than 405
expected
+ */
+ @Test
+ public void testOptions() throws Exception {
+ Tomcat tomcat = getTomcatInstance();
+
+ tomcat.getConnector().setAllowTrace(trace);
+
+ File docBase = new File(getTemporaryDirectory(), "webdav");
+ File collection = new File(docBase, COLLECTION_NAME);
+ Assert.assertTrue(collection.mkdirs());
+ File file = new File(docBase, FILE_NAME);
+ Assert.assertTrue(file.createNewFile());
+
+ addDeleteOnTearDown(docBase);
+
+ // app dir is relative to server home
+ org.apache.catalina.Context ctx =
+ tomcat.addWebapp(null, "/servlet", docBase.getAbsolutePath());
+
+ Wrapper w = Tomcat.addServlet(ctx, "servlet", createServlet());
+ w.addInitParameter("listings", Boolean.toString(listings));
+ w.addInitParameter("readonly", Boolean.toString(readonly));
+
+ ctx.addServletMapping("/*", "servlet");
+
+ tomcat.start();
+
+ OptionsHttpClient client = new OptionsHttpClient();
+ client.setPort(getPort());
+ client.setRequest(new String[] {
+ "OPTIONS /servlet/" + url + " HTTP/1.1" + CRLF +
+ "Host: localhost:" + getPort() + CRLF +
+ "Connection: close" + CRLF +
+ CRLF });
+
+ client.connect();
+ client.processRequest();
+
+ Assert.assertTrue(client.isResponse200());
+ Set<String> allowed = client.getAllowedMethods();
+
+ client.disconnect();
+ client.reset();
+
+ client.setRequest(new String[] {
+ method + " /servlet/" + url + " HTTP/1.1" + CRLF +
+ "Host: localhost:" + getPort() + CRLF +
+ "Connection: close" + CRLF +
+ CRLF });
+
+ client.connect();
+ client.processRequest();
+
+ String msg = "Listings[" + listings + "], readonly [" + readonly +
+ "], trace[ " + trace + "], url[" + url + "], method[" + method
+ "]";
+
+ Assert.assertNotNull(client.getResponseLine());
+
+ if (allowed.contains(method)) {
+ Assert.assertFalse(msg, client.isResponse405());
+ } else {
+ Assert.assertTrue(msg, client.isResponse405());
+ allowed = client.getAllowedMethods();
+ Assert.assertFalse(msg, allowed.contains(method));
+ }
+ }
+
+
+ protected abstract Servlet createServlet();
+
+
+ private static class OptionsHttpClient extends SimpleHttpClient {
+
+ @Override
+ public boolean isResponseBodyOK() {
+ return true;
+ }
+
+ public Set<String> getAllowedMethods() {
+ String valueList = null;
+ for (String header : getResponseHeaders()) {
+ if (header.startsWith("Allow:")) {
+ valueList = header.substring(6).trim();
+ break;
+ }
+ }
+ Assert.assertNotNull(valueList);
+ String[] values = valueList.split(",");
+ for (int i = 0; i < values.length; i++) {
+ values[i] = values[i].trim();
+ }
+ Set<String> allowed = new HashSet<String>();
+ allowed.addAll(Arrays.asList(values));
+
+ return allowed;
+ }
+ }
+}
diff --git a/test/org/apache/catalina/servlets/TestDefaultServletOptions.java
b/test/org/apache/catalina/servlets/TestDefaultServletOptions.java
new file mode 100644
index 0000000..8db6c90
--- /dev/null
+++ b/test/org/apache/catalina/servlets/TestDefaultServletOptions.java
@@ -0,0 +1,61 @@
+/*
+ * 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.catalina.servlets;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+
+import javax.servlet.Servlet;
+
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameters;
+
+@RunWith(Parameterized.class)
+public class TestDefaultServletOptions extends ServletOptionsBaseTest {
+
+ @Parameters
+ public static Collection<Object[]> inputs() {
+ Boolean[] booleans = new Boolean[] { Boolean.FALSE, Boolean.TRUE };
+ String[] urls = new String[] { COLLECTION_NAME, FILE_NAME,
UNKNOWN_NAME };
+ String[] methods = new String[] { "GET", "POST", "HEAD", "TRACE",
"PUT", "DELETE" };
+
+ List<Object[]> result = new ArrayList<Object[]>();
+
+ for (Boolean listingsValue : booleans) {
+ for (Boolean readOnlyValue : booleans) {
+ for (Boolean traceValue : booleans) {
+ for (String url : urls) {
+ for (String method : methods) {
+ result.add(new Object[] {
+ listingsValue, readOnlyValue, traceValue,
url, method } );
+ }
+ }
+ }
+ }
+
+ }
+ return result;
+ }
+
+
+ @Override
+ protected Servlet createServlet() {
+ return new DefaultServlet();
+ }
+}
diff --git a/test/org/apache/catalina/servlets/TestWebdavServletOptions.java
b/test/org/apache/catalina/servlets/TestWebdavServletOptions.java
new file mode 100644
index 0000000..e55aeed
--- /dev/null
+++ b/test/org/apache/catalina/servlets/TestWebdavServletOptions.java
@@ -0,0 +1,62 @@
+/*
+ * 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.catalina.servlets;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+
+import javax.servlet.Servlet;
+
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameters;
+
+@RunWith(Parameterized.class)
+public class TestWebdavServletOptions extends ServletOptionsBaseTest {
+
+ @Parameters
+ public static Collection<Object[]> inputs() {
+ Boolean[] booleans = new Boolean[] { Boolean.FALSE, Boolean.TRUE };
+ String[] urls = new String[] { COLLECTION_NAME, FILE_NAME,
UNKNOWN_NAME };
+ String[] methods = new String[] { "GET", "POST", "HEAD", "TRACE",
"PUT", "DELETE",
+ "MKCOL", "LOCK", "UNLOCK", "COPY", "MOVE", "PROPFIND",
"PROPPATCH" };
+
+ List<Object[]> result = new ArrayList<Object[]>();
+
+ for (Boolean listingsValue : booleans) {
+ for (Boolean readOnlyValue : booleans) {
+ for (Boolean traceValue : booleans) {
+ for (String url : urls) {
+ for (String method : methods) {
+ result.add(new Object[] {
+ listingsValue, readOnlyValue, traceValue,
url, method } );
+ }
+ }
+ }
+ }
+
+ }
+ return result;
+ }
+
+
+ @Override
+ protected Servlet createServlet() {
+ return new WebdavServlet();
+ }
+}
diff --git a/test/org/apache/catalina/startup/SimpleHttpClient.java
b/test/org/apache/catalina/startup/SimpleHttpClient.java
index 887fad1..7e33c76 100644
--- a/test/org/apache/catalina/startup/SimpleHttpClient.java
+++ b/test/org/apache/catalina/startup/SimpleHttpClient.java
@@ -52,6 +52,7 @@ public abstract class SimpleHttpClient {
public static final String FAIL_400 = "HTTP/1.1 400 ";
public static final String FORBIDDEN_403 = "HTTP/1.1 403 ";
public static final String FAIL_404 = "HTTP/1.1 404 ";
+ public static final String FAIL_405 = "HTTP/1.1 405 ";
public static final String TIMEOUT_408 = "HTTP/1.1 408 ";
public static final String FAIL_413 = "HTTP/1.1 413 ";
public static final String FAIL_417 = "HTTP/1.1 417 ";
@@ -394,52 +395,64 @@ public abstract class SimpleHttpClient {
responseBody = null;
}
+ public boolean responseLineStartsWith(String expected) {
+ String line = getResponseLine();
+ if (line == null) {
+ return false;
+ }
+ return line.startsWith(expected);
+ }
+
public boolean isResponse100() {
- return getResponseLine().startsWith(INFO_100);
+ return responseLineStartsWith(INFO_100);
}
public boolean isResponse200() {
- return getResponseLine().startsWith(OK_200);
+ return responseLineStartsWith(OK_200);
}
public boolean isResponse302() {
- return getResponseLine().startsWith(REDIRECT_302);
+ return responseLineStartsWith(REDIRECT_302);
}
public boolean isResponse400() {
- return getResponseLine().startsWith(FAIL_400);
+ return responseLineStartsWith(FAIL_400);
}
public boolean isResponse403() {
- return getResponseLine().startsWith(FORBIDDEN_403);
+ return responseLineStartsWith(FORBIDDEN_403);
}
public boolean isResponse404() {
- return getResponseLine().startsWith(FAIL_404);
+ return responseLineStartsWith(FAIL_404);
+ }
+
+ public boolean isResponse405() {
+ return responseLineStartsWith(FAIL_405);
}
public boolean isResponse408() {
- return getResponseLine().startsWith(TIMEOUT_408);
+ return responseLineStartsWith(TIMEOUT_408);
}
public boolean isResponse413() {
- return getResponseLine().startsWith(FAIL_413);
+ return responseLineStartsWith(FAIL_413);
}
public boolean isResponse417() {
- return getResponseLine().startsWith(FAIL_417);
+ return responseLineStartsWith(FAIL_417);
}
public boolean isResponse50x() {
- return getResponseLine().startsWith(FAIL_50X);
+ return responseLineStartsWith(FAIL_50X);
}
public boolean isResponse500() {
- return getResponseLine().startsWith(FAIL_500);
+ return responseLineStartsWith(FAIL_500);
}
public boolean isResponse501() {
- return getResponseLine().startsWith(FAIL_501);
+ return responseLineStartsWith(FAIL_501);
}
public Socket getSocket() {
@@ -447,4 +460,4 @@ public abstract class SimpleHttpClient {
}
public abstract boolean isResponseBodyOK();
-}
\ No newline at end of file
+}
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 11afd1e..0284199 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -123,6 +123,18 @@
<code>false</code> and <code>FORM</code> authentication is not being
used. (markt)
</fix>
+ <update>
+ Modify the Default and WebDAV Servlets so that a 405 status code is
+ returned for <code>PUT</code> and <code>DELETE</code> requests when
+ disabled via the <code>readonly</code> initialisation parameter.
+ </update>
+ <fix>
+ Align the contents of the <code>Allow</code> header with the response
+ code for the Default and WebDAV Servlets. For any given resource a
+ method that returns a 405 status code will not be listed in the
+ <code>Allow</code> header and a method listed in the <code>Allow</code>
+ header will not return a 405 status code. (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
