Hi Mark, I’ve seen your mail on the devs list. IMO you have only 2 choices:
* Option 1: decode the path passed to RD * Option 2: revert the changes brought by http://svn.apache.org/viewvc?view=revision&revision=1741019 and http://svn.apache.org/viewvc?view=revision&revision=1741024 (or at least part of it) so that the path passed to RD is not encoded (i.e. same as before). Any other choice will result in Tomcat having an important regression compared to before and XWiki won’t be able to use those new Tomcat versions (and I suspect a lot of others webapps will be in the same situations). Now I understand the problem with option 1. Users who were passing undecoded paths to RD will get broken too. However if they were doing since Tomcat was not encoding the path, I believe that they would have resulted in invalid URLs generated by the forward(), no? So maybe it’s not an issue after all. Thanks -Vincent > On 12 Jul 2016, at 09:09, Vincent Massol <vinc...@massol.net> wrote: > > Hi Mark, > >> On 11 Jul 2016, at 22:32, Mark Thomas <ma...@apache.org> wrote: >> >> On 10/07/2016 22:24, Vincent Massol wrote: >>> Ok I’ve found the issue that is causing the problem: >>> https://bz.apache.org/bugzilla/show_bug.cgi?id=59317 >>> >>> Specifically it says: >>> >>> “ >>> Async and non-async behaviours are currently the same. >>> • Both expect the path used to obtain the dispatcher to be decoded. >>> This behavior was confirmed with the Servlet EG. >>> • Both return the unencoded URI for req.getRequestURI(). That strikes >>> me as wrong. >>> " >>> >>> And >>> >>> “ >>> The restriction the the request dispatcher (or the async dispatch) must be >>> obtained with a decoded path has not changed. However, I have applied a fix >>> that ensures that the result of the call to getRequestURI() after the >>> dispatch returned an encoded URI. >>> “ >>> >>> It would be nice to have more information about " This behavior was >>> confirmed with the Servlet EG.". The Servlet spec doesn't mention anything >>> and this makes Tomcat have a different behavior than other servlet >>> containers (Jetty for example). >>> >>> I believe this change is dangerous and is going to cause regression to lots >>> of applications out there who used to work with an encoded dispatcher path >>> and that are now going to not work anymore (as is the case with XWiki, see >>> http://jira.xwiki.org/browse/XWIKI-13556). >>> >>> Could someone from the Tomcat dev team please comment on this? >> >> This is the change in question (note the corrected version numbers): >> >> Tomcat 8.0.x for 8.0.34 onwards: >> http://svn.apache.org/viewvc?view=revision&revision=1741019 >> >> Tomcat 7.0.x for 7.0.70 onwards: >> http://svn.apache.org/viewvc?view=revision&revision=1741024 >> >> This change ensured that a call to HttpServletRequest.getRequestURI() >> returned an encoded URL rather than a decoded one if called from within >> a dispatched request. The Javadoc for getRequestURI() is clear that the >> returned value is not decoded so I am confident that this change was >> correct. >> >> Tomcat has required that a decoded path is used to obtain a >> RequestDispatcher for as long as I can remember. I went back through svn >> and this dates back to at least Tomcat 4.1.x. > > As I mentioned in my first mail this is what I see: > > * With Tomcat > 7.0.69 and > 8.0.33, > …getRequestDispatcher("/bin/view/Main/test%20with%20space").forward(…) leads > to .../bin/view/Main/test%2520with%2520space (note that the precent is > encoded to %25). > > * With Tomcat <= 7.0.69 and <= 8.0.33 > …getRequestDispatcher("/bin/view/Main/test%20with%20space").forward(…) leads > to .../bin/view/Main/test%20with%20space (which is correct) > > Since you say that Tomcat has required a decoded path since at least Tomcat > 4.1.x the only thing I can think of is that before 7.0.70 and 8.0.34 Tomcat > was simply using the path as is without doing any encoding on it and starting > with the new versions you’re now encoding it. > > In any case, this change is bringing an important behavior change from the > usage of Tomcat POV (and is breaking XWiki). > >> Jetty (confirmed with 9.3.10) decodes the path provided to the >> RequestDispather. >> >> The Servlet spec has never been clear on which paths are encoded and >> which are decoded. I have been trying to get this fixed for a number of >> years (e.g. [1]) but unfortunately, the way Oracle runs the Servlet EG >> the only changes that are made to the spec are the ones Oracle wants to >> make. >> >> The clarification from the EG (linked from a related bug report, [2]) >> the was more around the behaviour of AsyncContext.dispatch(). It implied >> paths used with a RequestDispatcher were decoded but was not explicit. >> >> Having spent a fair chunk of today thinking about this, I'm leaning >> towards Tomcat being wrong not to decode the paths passed to the >> RequestDispatcher. My reasoning is as follows: >> >> Section 9.1.1 of the Servlet spec includes this example: >> <code> >> String path = “/raisins.jsp?orderno=5”; >> RequestDispatcher rd = context.getRequestDispatcher(path); >> rd.include(request, response); >> </code> >> >> It is clear from that example and the surrounding text that a query >> string can be provided with the path. What if the JSP was called >> "foo?bar.jsp" ? It is an odd name for a JSP but not an invalid one and >> names like this are often useful for exploring edge cases. The only way >> to dispatch to that JSP is to encode the '?' character. Therefore, the >> RD is going to have to decode the path. >> >> The above argument suggests that Tomcat has been handling requests for a >> RequestDispatcher incorrectly for quite some time. I worry that changing >> this behaviour might break something. On the other hand, if Jetty has >> been decoding this path and an incompatibility has not been raised until >> now, that suggests that the likelihood of breakage is lower. > > Indeed, if Tomcat were to decode the incoming path from the RD, it would work > fine again since you’re now encoding it thereafter. > > Note that I’ve not explored yet the consequences of the changes brought to > HttpServletRequest.getRequestURI() but it’s likely that it’s also affecting > us (any such change will affect the Tomcat users). Our code has probably > Tomcat-specific “hacks” to account for the past behavior. I’ll check it out > and report back here. > >> I'll start a discussion on the dev list. > > Thanks! Let me know of the outcome. > > FYI, ATM we’ve had to tell XWiki users to not use recent Tomcat versions, see > http://platform.xwiki.org/xwiki/bin/view/AdminGuide/InstallationTomcat. > Obviously we’d love to be able to use new and better versions which contain > other bugfixes and improvements. > > -Vincent > >> Mark >> >> >> [1] https://java.net/jira/browse/SERVLET_SPEC-18 >> [2] https://bz.apache.org/bugzilla/show_bug.cgi?id=57559 >> >>> >>> Thanks >>> -Vincent >>> >>> >>>> On 08 Jul 2016, at 22:00, Vincent Massol <vinc...@massol.net> wrote: >>>> >>>> Hi guys, >>>> >>>> I work on the XWiki project (http://xwiki.org) and we’ve had several >>>> reports of users telling us that XWiki is not working anymore with >>>> versions of Tomcat > 7.0.69 and > 8.0.33. It works perfectly well with >>>> those versions and lower. >>>> >>>> The issue is described in more detail at >>>> http://jira.xwiki.org/browse/XWIKI-13556 >>>> >>>> In short, I’ve tracked down one of the issues and here’s the problem we >>>> have: >>>> >>>> * We use context.getRequest().getRequestDispatcher(path).forward(…). >>>> * We are url-encoding the path. For example:path = >>>> /bin/view/Main/test%20with%20space >>>> * With Tomcat > 7.0.69 and > 8.0.33 (I’m testing with versions 8.0.36 and >>>> 7.0.59 to be precise) this generates an incoming URL of >>>> .../bin/view/Main/test%2520with%2520space in our code >>>> * With Tomcat <= 7.0.69 and <= 8.0.33 it was generating an incoming URL of >>>> .../bin/view/Main/test%20with%20space in our code >>>> >>>> Also note that with Jetty 9.2.13.v20150730 if we don’t url-encode the path >>>> passed to getRequestDispatcher(path) then Jetty generates an incoming URL >>>> of .../bin/view/Main/test with space in our code, which is of course >>>> invalid and fails. >>>> >>>> So I wanted to ask you two questions: >>>> * Would someone know the change in Tomcat that brought this difference >>>> from previous versions? >>>> * Who’s right? :) >>>> >>>> Thanks for any help >>>> >>>> -Vincent >>>> XWiki Committer --------------------------------------------------------------------- To unsubscribe, e-mail: users-unsubscr...@tomcat.apache.org For additional commands, e-mail: users-h...@tomcat.apache.org
