Bug 61692 - CGIServlet should handle additional HTTP methods, for example MKCALENDAR, MKCOL, PROPFIND, PROPPATCH
Summary: CGIServlet should handle additional HTTP methods, for example MKCALENDAR, MKC...
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 9
Classification: Unclassified
Component: Catalina (show other bugs)
Version: 9.0.1
Hardware: All All
: P2 enhancement (vote)
Target Milestone: -----
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-10-28 16:25 UTC by jm009
Modified: 2018-09-13 13:45 UTC (History)
0 users



Attachments
Allow additional HTTP methods by configuration (6.29 KB, patch)
2017-10-28 16:25 UTC, jm009
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description jm009 2017-10-28 16:25:00 UTC
Created attachment 35474 [details]
Allow additional HTTP methods by configuration

I am working on running Nextcloud (PHP web application) with the Tomcat webserver.
Nextcloud serves different ressources with WebDAV, and that requires additional HTTP methods.

Currently CGIServlet only handles the GET, POST, HEAD (and OPTIONS, TRACE) HTTP methods.

In addition I have had to implement the following HTTP methods: DELETE MKCALENDAR, MKCOL, PROPFIND, PROPPATCH, PUT, REPORT.
Because there are tons of other HTTP methods nowadays, I suggest to make the whole think configurable. See for example https://github.com/nextcloud/server/issues/6644
Alternatively, CGIServlet could just handle any HTTP method (that means just forward the method string to the CGI script, without limiting to a pre configured list).
Attached is a patch, that works fine for me.
Comment 1 Christopher Schultz 2017-10-31 14:23:05 UTC
I don't like this patch (-1) for a number of reasons.

1. It uses many regular expressions during each request: poor performance
2. It requires configuration for what should be pass-through semantics

Why not simply replace doGet, doPost, etc. with service(Request,Response) and pass everything through to the underlying CGI without all that overhead?
Comment 2 jm009 2017-11-01 12:03:23 UTC
(In reply to Christopher Schultz from comment #1)
> I don't like this patch (-1) for a number of reasons.
> 
> 1. It uses many regular expressions during each request: poor performance

Patterns are compiled in init(), not for each request.

Let Pattern p = Pattern.compile("A|B|C|D").
Then
p.matcher(String).matches
should be as efficient as
"A".equals(String) || "B".equals(String) || "C".equals(String) || "D".equals(String).



> 2. It requires configuration for what should be pass-through semantics
> 
> Why not simply replace doGet, doPost, etc. with service(Request,Response)
> and pass everything through to the underlying CGI without all that overhead?

Yep, I mentioned that as alternative solution.

Just to make sure, I got your point: What do you mean by "pass-through semantics"?

How to determine in this case, if request body data (a.k.a. "POST data") should be forwarded to the servlet?

- Forward any time?
or
- Check HttpServletRequest.getInputStream() != null && !HttpServletRequest.getInputStream().isFinished()?

This is, what Apache HTTPD does (simply accapts all method names and forwards them to the CGI script).
I am wondering, if there might be cases, where forwarding request body data to a CGI script, that is not prepared for it, might result in security problems.
Comment 3 jm009 2017-11-01 21:56:22 UTC
(In reply to jm009 from comment #2)
> How to determine in this case, if request body data (a.k.a. "POST data")
> should be forwarded to the servlet?

If I get it right, this reverse proxy servlet

https://github.com/mitre/HTTP-Proxy-Servlet

decides to handle request body data, if either http header CONTENT_LENGTH or TRANSFER_ENCODING is set.
Comment 4 Mark Thomas 2017-11-16 20:28:14 UTC
Currently, the CGI servlet only handles a request body if the method is POST and the Content-Length header is set.

Unless there are objections then I propose the following:
- allow any HTTP method and let the CGI script decide how to handle it
- except HEAD, TRACE and OPTIONS which will still be handled by HttpServlet
- pass the request body for any method if Content-Length or Transfer-Encoding is set

Since this is a change in functionality, I propose to only make this change in 9.0.x.
Comment 5 jm009 2017-11-22 05:02:03 UTC
> Unless there are objections then I propose the following:
> - allow any HTTP method and let the CGI script decide how to handle it

Sounds good.


> - except HEAD, TRACE and OPTIONS which will still be handled by HttpServlet

Shouldn't the servlet handle OPTIONS, because Tomcat doesn't know which methods are handled by the servlet? I would say yes, but to be honest, I have not much experience with details of HTTP traffic.
Comment 6 Mark Thomas 2017-11-22 19:44:52 UTC
Good point. Fixing that starts heading in the direction of the original patch again. More thought required.
Comment 7 Mark Thomas 2017-12-12 15:18:14 UTC
I've been thinking about this some more and experimenting with different ideas. Trying to reconcile the combination of maximising backwards compatibility, supporting any HTTP method and having the correct response for OPTIONS is not easy. Something has to give.

My current thinking is to add a single new option, enableAllHttpMethods. If this is enabled, every request regardless of method will be passed to the CGI script and it will be expected to respond appropriately. This maximises backwards compatibility while enabling scripts to be deployed that support any methods. The downside is that if you enable this option the script has to handle OPTIONS, TRACE and any unrecognised methods. That seems a reasonable compromise to me.

For the request body, I plan to go with the option of passing the request body for any method if Content-Length or Transfer-Encoding is set.

This approach has the advantage that it can be applied to all supported Tomcat versions.

I'll leave this a few days for folks to comment and if the consensus is favourable, I'll implement this approach for the next round of releases.
Comment 8 Konstantin Kolinko 2017-12-12 15:38:22 UTC
(In reply to Mark Thomas from comment #7)
> My current thinking is to add a single new option, enableAllHttpMethods. If
> this is enabled, every request regardless of method will be passed to the
> CGI script and it will be expected to respond appropriately.

Alternative idea:
- a regexp option that is applied as a filter to method names.

If "OPTIONS" method matches the regexp filter then it means that OPTIONS method is processed by underlying CGI script.


If "OPTIONS" method does not match the regexp filter, we can implement support for OPTIONS method in Java in the following way:
1. Iterate over a list of known HTTP methods - see IANA registry [1]
2. Retain only those that match the regexp filter.

[1] https://www.iana.org/assignments/http-methods/http-methods.xhtml
Comment 9 Mark Thomas 2018-07-11 20:23:29 UTC
Another idea.

Add a new option cgiMethods which is a comma separated list of methods to pass to the CGI script (or * for all methods). Requests using one of those methods are always passed to the script. Default value is "GET,POST" to match current behaviour. The request body is passed to the script for any method if Content-Length or Transfer-Encoding is set.

Requests for HEAD, OPTIONS and TRACE are handled by the servlet (which may delegate to the super class) unless the method is listed in cgiMethods. The value of cgiMethods will be used to help populate the OPTIONS response.

Requests using any other method gets a 405.

I think this gives:
- backwards compatible by default
- support for arbitrary HTTP methods
- the ability to pass any sub-set of HTTP methods to the CGi script
Comment 10 Mark Thomas 2018-09-13 13:45:58 UTC
I hadn't forgotten about this. I just hadn't found the time to come back to it and implement it.

Fixed in:
- trunk for 9.0.13 onwards
- 8.5.x for 8.5.35 onwards
- 7.0.x for 7.0.91 onwards