Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DefaultServlet no longer supports POST and OPTIONS and returns a 405 instead #10231

Closed
ekupcik opened this issue Aug 4, 2023 · 11 comments · Fixed by #10232
Closed

DefaultServlet no longer supports POST and OPTIONS and returns a 405 instead #10231

ekupcik opened this issue Aug 4, 2023 · 11 comments · Fixed by #10232
Assignees
Labels
Bug For general bugs on Jetty side
Milestone

Comments

@ekupcik
Copy link

ekupcik commented Aug 4, 2023

Jetty version(s)
12 beta 4

Description
Bug or feature? That is probably the question here. Most of the time it is probably used only for a index.jsp that does a redirect etc. But in my case it can happen that it is also called using a POST. And others might expect OPTIONS to work there. As this used to work in previous versions and also works for example in Tomcat I would consider this at least quite unexpected and as long there is no really good reason for not supporting POST and OPTIONS the old behavior should be restored IMHO

Edit: I just realized that this seems to affect only EE10 while it still should work in EE9? Is that on purpose or a bug?

@ekupcik ekupcik added the Bug For general bugs on Jetty side label Aug 4, 2023
@joakime
Copy link
Contributor

joakime commented Aug 4, 2023

DefaultServlet was never supposed to support POST, having it operate on POST in ee8/ee9 is considered a bug. (we don't change anything in the DefaultServlet, so POST is meaningless).

Accepting arbitrary POST data on the request to DefaultServlet is also considered a bad idea for several reasons.

  1. we don't read the POST body, so that breaks the HTTP/1.1 persistence on the connection if used.
  2. we don't do anything with the POST body, so that's useless network traffic that can be used for nefarious behaviors.

Now OPTIONS on DefaultServlet, hmm.
I bet we could hardcode that to just return Allow: GET, HEAD, OPTIONS

joakime added a commit that referenced this issue Aug 4, 2023
+ HEAD support & testing
+ TRACE disabled & testing
+ OPTIONS support & testing
+ POST disabled & testing
@joakime joakime linked a pull request Aug 4, 2023 that will close this issue
@joakime
Copy link
Contributor

joakime commented Aug 4, 2023

Opened PR #10232

joakime added a commit that referenced this issue Aug 4, 2023
+ HEAD support & testing
+ TRACE disabled & testing
+ OPTIONS support & testing
+ POST disabled & testing
@joakime joakime moved this to 🏗 In progress in Jetty 12.0.1 - FROZEN Aug 4, 2023
@joakime joakime added this to the 12.0.x milestone Aug 4, 2023
@joakime joakime self-assigned this Aug 4, 2023
@ekupcik
Copy link
Author

ekupcik commented Aug 4, 2023

Ok, i understand that point that the DefaultServlet wouldn't do anything with the POST body data anyway. But in my case the POST is intercepted by a servlet filter which handles the POST data and and then just forwards the request to the filter chain which then happens to end on the welcome page which then returns the 405. The welcome page doesn't do anything with the POST data, it just didn't complain about being called with a POST.

I can subclass DefaultServlet or maybe it would be cleaner to let the filter do a redirect instead anyway. So i am fine. But i don't have any idea whether I am the only one on the world or whether there are others that might run into this too. And as this used to work for years and also works on Tomcat it should at least be pointed out as a potentially breaking change.

Or maybe you could add an init-param for restoring the old behaviour?

joakime added a commit that referenced this issue Aug 5, 2023
Shows how to override method support
on DefaultServlet
@joakime joakime moved this to 🏗 In progress in Jetty 12.0.0 Aug 5, 2023
@joakime
Copy link
Contributor

joakime commented Aug 5, 2023

To override you would do (this is directly from our own test cases)

    public class WibbleDefaultServlet extends DefaultServlet
    {
        @Override
        protected void service(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException
        {
            switch (req.getMethod())
            {
                case "POST":
                case "WIBBLE":
                    // Disregard the method given, use GET instead.
                    doGet(req, resp);
                    return;
                default:
                    super.service(req, resp);
            }
        }
    }

@joakime joakime moved this from 🏗 In progress to 👀 In review in Jetty 12.0.0 Aug 5, 2023
joakime added a commit that referenced this issue Aug 5, 2023
* Issue #10231 - ee10 & ee9 DefaultServlet HTTP method support.

+ HEAD support & testing
+ TRACE disabled & testing
+ OPTIONS support & testing
+ POST disabled & testing
@sbordet
Copy link
Contributor

sbordet commented Aug 5, 2023

@ekupcik A redirect from the filter would be best because half-processing a POST in the filter and then forwarding to a welcome page is a very uncommon way of handling a request (I cannot imagine what client would do a POST with a body, get back index.html and be happy about it, can you detail?).

Best would be to follow the Post/Redirect/Get pattern.

@joakime
Copy link
Contributor

joakime commented Aug 5, 2023

PR #10232 merged.

@joakime joakime closed this as completed Aug 5, 2023
@github-project-automation github-project-automation bot moved this from 👀 In review to ✅ Done in Jetty 12.0.0 Aug 5, 2023
@ekupcik
Copy link
Author

ekupcik commented Aug 5, 2023

@sbordet You are absolutely right. But the POST is actually not comming from outside but it is more a flaw in the application. The filter is an authentication / login page filter and the login can consist of multiple pages like login page followed by 2fa followed by legal disclaimer etc. And while the simple case when only the login page is used does redirect after the POST at least some of these additional pages don't do that and just forward the request to chain after they have processed their part. Not nice but as this has been working for ages nobody cared enough to change that.

So the reason why I suggested to restore the old behaviour was not because I think that it makes total sense for the welcome files to support POST but more like whether this change is worth potentially breaking an unknown number of applications. Sure, it might be more a problem of the application but is it necessary that jetty changes an really old behaviour that has been here for ages thus potentially breaking compatibility? Even if supporting this old behaviour was just a coincidence.

@sbordet sbordet reopened this Aug 5, 2023
@sbordet
Copy link
Contributor

sbordet commented Aug 5, 2023

@joakime @gregw for sake of backwards compatibility, should we add a parameter to DefaultServlet to support specific HTTP methods?

I.e. rather than the currently hardcoded list of GET, HEAD, OPTIONS we make that list a parameter so people can configure it.

@gregw
Copy link
Contributor

gregw commented Aug 5, 2023

@sbordet I'm not sure what the exact use-case for configuring this would be? Anything that can intercept the request and actually handle a POST or a TRACE can also intercept the OPTIONS and make it say what they like.

In, @ekupcik case, he appears to have a filter that will do a login conversation, intercepting some requests that would otherwise go through to the default servlet. I assume that just some of the URIs processed by that filter in just some states will accept a POST request. It seams wrong for the default servlet to declare it accepts POST for all URIs. Better for the filter to intercept OPTIONS and say what it wants on specific URIs.

Or if they really want, they can add a filter for just the default servlet (by name), which handles all OPTIONS requests. So there is the ability to extend this behaviour without adding a configuration mechanism for the 0.001% case.

@ekupcik
Copy link
Author

ekupcik commented Aug 6, 2023

@gregw Ignore the filter. If anything then it is just an example how people could end up sending a POST to the default servlet. Sometimes people including myself do stupid things. And in the past you could do stupid things and as long as your request had just the wrong method and you didn't expect that the POST data is available in any way you were fine.

After all there is no definition that says that a POST request must contain form data. Or you can do a POST but use the query parameters. That just works fine. I am not saying that this is the right or typical way and in particular not for the default servlet / welcome page. Definitly not. But is it possible to do that? Absolutly. Do I have any numbers? Of course not.

Making this configurable wouldn't mean controling just the OPTIONS response. As you have written this wouldn't make any sense. But whether the old behaviour that mapped doPost() to doGet() should be reenabled to restore compatibility with previous Jetty versions or not. And if it is then OPTIONS should reflect this automatically.

Oh boy, If only i knew what I would start here. I already feel guilty of wasting people's time considering that subclassing DefaultServlet etc. took just a few minutes.

@sbordet
Copy link
Contributor

sbordet commented Aug 6, 2023

Oh boy, If only i knew what I would start here. I already feel guilty of wasting people's time considering that subclassing DefaultServlet etc. took just a few minutes.

Don't worry, it's valuable feedback.

Discussions like these are a good reference for others too, so it's worthwhile having them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For general bugs on Jetty side
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants