Bug 51940 - Form Authentication Valve should restore request body on PUT method
Summary: Form Authentication Valve should restore request body on PUT method
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 6
Classification: Unclassified
Component: Catalina (show other bugs)
Version: 6.0.33
Hardware: PC Linux
: P2 normal (vote)
Target Milestone: default
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-03 16:15 UTC by Nicholas Sushkin
Modified: 2011-10-31 13:06 UTC (History)
0 users



Attachments
Patch fixing PUT handling with form authentication (1.77 KB, patch)
2011-10-07 17:40 UTC, Nicholas Sushkin
Details | Diff
2011-10-20_tc6_51940_v3.patch (6.48 KB, patch)
2011-10-20 10:39 UTC, Konstantin Kolinko
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nicholas Sushkin 2011-10-03 16:15:05 UTC
In Tomcat 6 (and 7), Form Authentication valve restores the original request after a POST with successful authentication and redirect is followed by the client's GET. In case of the POST, the valve also restores the original request's body. However, it doesn't do that for a PUT. To be consistent, Tomcat should restore the body on PUT as well.

The patch would be in FormAuthenticator.restoreRequest(Request, Session) [1], to change from

if ("POST".equalsIgnoreCase(saved.getMethod())) {

to

if ("POST".equalsIgnoreCase(saved.getMethod()) ||
"PUT".equalsIgnoreCase(saved.getMethod())
) {

[1] http://svn.apache.org/viewvc/tomcat/tc6.0.x/trunk/java/org/apache/catalina/authenticator/FormAuthenticator.java?view=markup#l450


Maybe related to Bug #48692.


This issue was discussed on users mailing list archived at

http://markmail.org/thread/klafrhln32v3zcau

and

http://mail-archives.apache.org/mod_mbox/tomcat-users/201109.mbox/%3C3052451.ZX31eH6Cz8@strela%3E


Regarding "Re: Should Form Authentication Valve restore request body on a PUT?", 
on Thursday, September 29, 2011 17:04:27,
Christopher Schultz wrote to Tomcat Users List <users@tomcat.apache.org>

> ...
> The servlet spec (v3.0, SRV 13.6.3.1) has this to say:
> "
> If the form based login is invoked because of an HTTP request, the
> original request parameters must be preserved by the container for use
> if, on successful authentication, it redirects the call to the
> requested resource.
> "
> 
> It doesn't say what kinds of HTTP verbs should or should not be
> supported, but GET and PUT seem entirely obvious. It doesn't say that
> the request body needs to be maintained, only the "request
> parameters". Since the servlet specification doesn't have any
> provisions for fetching request parameters from PUT operations, I
> suppose the spec therefore doesn't directly recommend that PUT bodies
> be stored for later use like when POST is used.
> ...
> On the face of it, that seems reasonable. I haven't read-through the
> code that then replays the saved-request so I'm not sure if there's
> more to be done.


Regarding "Re: Should Form Authentication Valve restore request body on a PUT?", 
on Friday, September 30, 2011 13:10:55,
Mark Thomas wrote to Tomcat Users List <users@tomcat.apache.org>

> I'd have no objection so the proposed change.
> 
> Mark
Comment 1 Nicholas Sushkin 2011-10-07 17:06:46 UTC
Regarding "Re: Should Form Authentication Valve restore request body on a PUT?", 
on Friday, October 07, 2011 10:13:00,
Christopher Schultz wrote to Tomcat Users List <users@tomcat.apache.org>

> Nicholas,
> 
> On 10/6/2011 10:08 PM, Nicholas Sushkin wrote:
> > I now reconfigured DefaultServlet in conf/web.xml with
> > readonly=false. Now, an unauthenticated PUT (with or without a
> > body) returns 204 No Content instead of the login form. Seems like
> > a bug. Should I add this behavior to Bug #51940 or a new bug?
> 
> I'll bet what is happening is that your PUT request is being forwarded
> without modification to the login page, and your login page is some
> static content. Is that right?
> 
> If that's what's happening, the DefaultServlet is handling the
> request, seeing that it is a PUT, and then complaining that it's
> read-only. When you make the DefaultServlet read-write you tell the
> DefaultServlet to accept uploads, and you'll probably end up
> overwriting your login form with the request entity (oops).
> 
> It looks like the authenticator code needs to transform the PUT
> request into a GET (or POST?) so that the DefaultServlet doesn't try
> to do an upload.
> 
> I think you'd have similar problems if trying to use a JSP for your
> login-page, because JSPs can't accept PUT requests unless specifically
> configured to do so.
> 
> Since you're just hacking, try setting the request method to "GET"
> when you detect a PUT request that requires authentication.
Comment 2 Nicholas Sushkin 2011-10-07 17:40:01 UTC
Created attachment 27729 [details]
Patch fixing PUT handling with form authentication

Implemented Charles's suggestion. Before forwarding to the login page, but after saving the request, in the request, set method (type) to GET.

Also, in save/restore of a request, when the method is PUT, allowed saving/restoring of the method body.

Tested this patch on unauthenticated GET, PUT, POST, and DELETE.
Comment 3 Mark Thomas 2011-10-10 15:13:20 UTC
POST and PUT are not the only methods that may have a request body. Request bodies are described in RFC2616 for OPTION re
Comment 4 Mark Thomas 2011-10-10 15:17:37 UTC
Try again, this time with the full message.

RFC 2616 explicitly discusses OPTIONS and a request body. However, any HTTP request method may use a request body [1] although in many cases it will be ignored.

It appears that the right thing to do here is to save any request body we find and restore it after authentication regardless of request method.

[1] http://tech.groups.yahoo.com/group/rest-discuss/message/9962
Comment 5 Mark Thomas 2011-10-10 15:48:59 UTC
Thanks for the patch. I used it as the basis for the fox that has been committed.

The fix has been applied to trunk and 7.0.x and will be included in 7.0.23 onwards.

The fix has also been proposed for 6.0.x.
Comment 6 Nicholas Sushkin 2011-10-10 17:08:25 UTC
Perfect. I am glad Roy Fielding clarified the usage of request body in HTTP and REST. It makes sense and the code is now simpler and more generic.

How does that request for the bug fix to be included into 6.0.x work? Is that Release Manager for 6.0.x Jean-Frederic who needs to approve it? I see that both Bug Fixes and Enhancements are still accepted for 6.0.x ( http://wiki.apache.org/tomcat/TomcatVersions ). It would be most useful to me if the fix was included in Tomcat 6. Thank you.
Comment 7 Nicholas Sushkin 2011-10-10 17:29:14 UTC
BTW, in your change to 7.0, there seems to be a slight inconsistency in the log message. In other log messages, when you say "context [{n}]", you pass context.getName(). In this message, you passed context.getServletContext().getContextPath() leftover from my patch.

http://svn.apache.org/viewvc/tomcat/tc7.0.x/trunk/java/org/apache/catalina/authenticator/FormAuthenticator.java?view=markup#l375

I wasn't going to say anything, but if you're revisiting this file, might as well fix the typo in "somethign" :)

http://svn.apache.org/viewvc/tomcat/tc7.0.x/trunk/java/org/apache/catalina/authenticator/FormAuthenticator.java?view=markup#l626
Comment 8 Mark Thomas 2011-10-10 18:58:14 UTC
Thanks for the review. Those issues have been fixed.

Any proposal for back-port needs a minimum of 3 +1 votes from committers. Once
it has the votes, a committer (usually the proposer but if doesn't have to be)
will commit it and it is included in the next release.

The RM role is 'just' one of creating the release. It has no special role to
play in the back-port process.
Comment 9 Konstantin Kolinko 2011-10-11 13:15:18 UTC
1. I tried to test this in trunk, and replaying a POST request fails for me.

Using the following standalone HTML page:
[[[
<FORM action="http://localhost:8080/examples/jsp/security/protected/index.jsp" method="POST">
<input name="role">
<input type="submit">
</FORM>
]]]

- Configure a sample user with a role of "tomcat" and start Tomcat
- Start a new web browser and load the above page
- Type "aaa" and press submit.
- Respond to FORM authentication.
- The protected page is displayed.
Expected: "You have not been granted role aaaa"
Actual: no such message.

I tried to debug this, but I do not see anything wrong in FormAuthenticator.
The body and method were saved and restored. Request#parametersParsed was false.

Then, setting a breakpoint in Request#parseParameters() I see that
Request#usingInputStream is true and thus parseParameters() exits early.

This is reproducible in 7.0.22.
This does not happen in 6.0.33 - it replays POSTs correctly.


2. request.getCoyoteRequest().method().setString("GET");
is seen as GET in access log.  Not much of an issue though, as anyway we return not what was originally requested by client.

I think that to fix this one can restore the original method in FormAuthenticator#forwardToLoginPage(..) when disp.forward() call returns.
Comment 10 Mark Thomas 2011-10-19 18:02:15 UTC
(In reply to comment #9)
> 1. I tried to test this in trunk, and replaying a POST request fails for me.

That is a side-effect of r987955 which is not directly related to this bug.

> 2. request.getCoyoteRequest().method().setString("GET");
> is seen as GET in access log.  Not much of an issue though, as anyway we return
> not what was originally requested by client.
> 
> I think that to fix this one can restore the original method in
> FormAuthenticator#forwardToLoginPage(..) when disp.forward() call returns.

That would work. I'm on the fence. We can record what was requested or what we process.
Comment 11 Konstantin Kolinko 2011-10-20 10:28:18 UTC
(In reply to comment #10)
> (In reply to comment #9)
> > 1. I tried to test this in trunk, and replaying a POST request fails for me.
> 
> That is a side-effect of r987955 which is not directly related to this bug.

Confirming that r1186379 / r1186383 fixed it.

Tomcat 6 is not affected by this issue.


> 
> > 2. request.getCoyoteRequest().method().setString("GET");
> > is seen as GET in access log.  Not much of an issue though, as anyway we return
> > not what was originally requested by client.
> > 
> > I think that to fix this one can restore the original method in
> > FormAuthenticator#forwardToLoginPage(..) when disp.forward() call returns.
> 
> That would work. I'm on the fence. We can record what was requested or what we
> process.

Fixed this in r1186712 / r1186719
Comment 12 Konstantin Kolinko 2011-10-20 10:39:24 UTC
Created attachment 27825 [details]
2011-10-20_tc6_51940_v3.patch

TC 6 version of the patch
In trunk these are r1181028, r1181136, r1186378, r1186712
Comment 13 Mark Thomas 2011-10-31 13:06:53 UTC
Fixed in 6.0.x and will be included in 6.0.34 onwards.