-1 veto
This change is not compliant with RFC 9110.
If Tomcat sends METHOD_NOT_ALLOWED it MUST also send an Allow header.
That is why the default servlet has a dedicated method for sending
METHOD_NOT_ALLOWED.
Further comments in-line.
Mark
On 02/10/2024 10:07, micha...@apache.org wrote:
This is an automated email from the ASF dual-hosted git repository.
michaelo pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/main by this push:
new 142c45a427 BZ 69360: Inconsistent DELETE behavior between
DefaultServlet and WebdavServlet
142c45a427 is described below
commit 142c45a4271e1fd8d400196a883fb560ebded110
Author: Michael Osipov <micha...@apache.org>
AuthorDate: Wed Oct 2 11:04:44 2024 +0200
BZ 69360: Inconsistent DELETE behavior between DefaultServlet and
WebdavServlet
---
java/org/apache/catalina/servlets/WebdavServlet.java | 6 +++---
webapps/docs/changelog.xml | 5 +++++
2 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/java/org/apache/catalina/servlets/WebdavServlet.java
b/java/org/apache/catalina/servlets/WebdavServlet.java
index d32868a9b3..c2cf8f3d78 100644
--- a/java/org/apache/catalina/servlets/WebdavServlet.java
+++ b/java/org/apache/catalina/servlets/WebdavServlet.java
@@ -1696,7 +1696,7 @@ public class WebdavServlet extends DefaultServlet
implements PeriodicEventListen
if (!resource.isDirectory()) {
if (!resource.delete()) {
- resp.sendError(WebdavStatus.SC_INTERNAL_SERVER_ERROR);
+ resp.sendError(WebdavStatus.SC_METHOD_NOT_ALLOWED);
May be able to utilise sendNotAllowed() for this one. Thinking about it
probably not as WebDAV has additional methods that need to be taken
account of. It will need a dedicated method for WebDAV.
return false;
}
} else {
@@ -1705,7 +1705,7 @@ public class WebdavServlet extends DefaultServlet
implements PeriodicEventListen
deleteCollection(req, path, errorList);
if (!resource.delete()) {
- errorList.put(path,
Integer.valueOf(WebdavStatus.SC_INTERNAL_SERVER_ERROR));
+ errorList.put(path,
Integer.valueOf(WebdavStatus.SC_METHOD_NOT_ALLOWED));
This is going to be part of a multi-status response. The allow header
doesn't make sense for a multi-status response. There I think this is OK.
}
if (!errorList.isEmpty()) {
@@ -1772,7 +1772,7 @@ public class WebdavServlet extends DefaultServlet
implements PeriodicEventListen
if (!childResource.isDirectory()) {
// If it's not a collection, then it's an unknown
// error
- errorList.put(childName,
Integer.valueOf(WebdavStatus.SC_INTERNAL_SERVER_ERROR));
+ errorList.put(childName,
Integer.valueOf(WebdavStatus.SC_METHOD_NOT_ALLOWED));
As above.
}
}
}
Generally, SC_METHOD_NOT_ALLOWED is probably the right status code as
the most likely failure mode is a lack of permissions. I just wonder if
there are circumstances where we would want to return
SC_INTERNAL_SERVER_ERROR and how we would determine if that were the case.
Mark
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index ca652cd604..f4eb84d1b4 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -128,6 +128,11 @@
<code>getRelativePath()</code> method from super class with
incorrect Javadoc. (michaelo)
</fix>
+ <fix>
+ <bug>69360</bug>: Inconsistent <code>DELETE</code> behavior between
+ <code>WebdavServlet</code> and <code>DefaultServlet</code>.
+ (michaelo)
+ </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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
