Fix #544 - error dispatch should always be handled as GET

[markt-asf]

@stuartwdouglas Yes, this should have a change log entry. I was expecting a lot of discussion on this one so I left that for later. I'll make sure one is added before the final merge.

@gregw My concern with that approach is that it is then impossible for the error page to determine what the actual method used was. I can imagine that causing issues for error pages for 405 responses. Do we want to add a RequestDispatcher.ERROR_METHOD request attribute? I am a little worried changing the behaviour of HttpServletRequest.getMethod() even for a good reason such as this, could break more things than it fixes.

[stuartwdouglas]

I think changing getMethod() would definitely break compatibility. I would image there is logs of existing code that uses this to log details about the failed request.

[gregw]

This PR is already changing behaviour and breaking compatibility, just only for some Servlets. Any Servlet that was well written and correctly handled an error dispatch to doPost might now break when that request turns up in doGet.

This seams like a very specific partial fix. I think we should fix this for all Servlets (with a breaking change) or find some other way to retrofit protection.

I think we should go as far as saying that by default an error dispatch is always a GET with not content available via the request methods (i.e. if it was a POST, you cannot read it). If we are really concerned about breaking applications, then we can add an element to <error-page> to enable/disable the original request being delivered.

[markt-asf]

Request bodies are permissible for GET requests so I think the following would address the security concerns whilst requiring minimal rework if the error page needed access to the original request method:

• containers MUST use GET for all error dispatches
• containers MUST provide the original request method (i.e. what HttpServletRequest.getMethod() would have returned immediately before the error dispatch) as the request attribute RequestDispatcher.ERROR_METHOD.

If this gets consensus, I'll update the PR. If not, we can continue discussion.

[markt-asf]

It looks like we have consensus for this. Unless there are objections in the next ~72 hours, I intend to merge this.