Two serious issues have been introduced in Tomcat 7 and Tomcat 8...

[Doug Forrest <df...@opentext.com>]

The issues were introduced by r1645015 in 7.x and r1644992 in 8.x. Both issues are related to ApplicationContext.getContext(String uri).

Issue 1:

getContext("/ROOT") no longer works. In fact, it doesn't appear to be possible to get the ROOT context using this method any more since the literal uri for ROOT would be "" and the first functional line in the method rejects any uri that doesn't start with "/". This is a serious issue that will break many of my customers if they apply a patch containing this code.

Suggested fix:

if (uri.equals("/ROOT")) uri = "";

at line 269.

Issue 2:

getContext now only returns a valid context if its path matches the passed in uri exactly. This is a huge problem for multiple reasons.

This is a major change in functionality that should not have been introduced in a patch release. This code has accepted a uri that contains path information past the context root for many years across many releases. This behavior mirrors the behavior of every major servlet container available. There is a large amount of existing code out in the wild that relies on this behavior. If an intentional decision were made to change this method to require an exact match, it would be prudent to make such a change in a major release to give users and integrators an opportunity to react to the change.

I don't have a suggested fix for this issue, but I would be happy to come up with one if required.

Thank you for your time.

Doug Forrest
Principal Escalations Engineer
OpenText

Since I am brand new to this list, a quick note about me: I work in support at a very large software company. My team is responsible for level 3 support and defect fixes for a large J2EE enterprise web content management application. Our product has supported Tomcat as a servlet container for content delivery applications since the early 2000s. Many of our customers use Tomcat for their production servlet container and it is the go to choice for our internal repro environments.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org

Re: Two serious issues have been introduced in Tomcat 7 and Tomcat 8...

[Mark Thomas <ma...@apache.org>]

On 23/02/2015 18:41, Mark Thomas wrote:
> On 23/02/2015 18:01, Doug Forrest wrote:
>> The issues were introduced by r1645015 in 7.x and r1644992 in 8.x.
>> Both issues are related to ApplicationContext.getContext(String
>> uri).
>>
>> Issue 1:
>>
>> getContext("/ROOT") no longer works. In fact, it doesn't appear to be
>> possible to get the ROOT context using this method any more since the
>> literal uri for ROOT would be "" and the first functional line in the
>> method rejects any uri that doesn't start with "/". This is a serious
>> issue that will break many of my customers if they apply a patch
>> containing this code.
>>
>> Suggested fix:
>>
>> if (uri.equals("/ROOT")) uri = "";
>>
>> at line 269.
> 
> getContext("/ROOT") is just wrong. I agree it should be possible to
> obtain the ROOT context but that is not the way to do it. Given the
> Javadoc for getContext(), "" or "/" should be guaranteed to return the
> ROOT context. Anything else will depend on what contexts are deployed at
> what paths.
> 
>> Issue 2:
>>
>> getContext now only returns a valid context if its path matches the
>> passed in uri exactly. This is a huge problem for multiple reasons.
> 
> Yes, the restriction is not in line with the Javadoc for getContext()
> and needs to be relaxed.

This is fixed in trunk in r1661867. I plan on giving folks a chance to
review this before back-porting it.

Mark


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org

Re: Two serious issues have been introduced in Tomcat 7 and Tomcat 8...

[Mark Thomas <ma...@apache.org>]

On 23/02/2015 18:01, Doug Forrest wrote:
> The issues were introduced by r1645015 in 7.x and r1644992 in 8.x.
> Both issues are related to ApplicationContext.getContext(String
> uri).
> 
> Issue 1:
> 
> getContext("/ROOT") no longer works. In fact, it doesn't appear to be
> possible to get the ROOT context using this method any more since the
> literal uri for ROOT would be "" and the first functional line in the
> method rejects any uri that doesn't start with "/". This is a serious
> issue that will break many of my customers if they apply a patch
> containing this code.
> 
> Suggested fix:
> 
> if (uri.equals("/ROOT")) uri = "";
> 
> at line 269.

getContext("/ROOT") is just wrong. I agree it should be possible to
obtain the ROOT context but that is not the way to do it. Given the
Javadoc for getContext(), "" or "/" should be guaranteed to return the
ROOT context. Anything else will depend on what contexts are deployed at
what paths.

> Issue 2:
> 
> getContext now only returns a valid context if its path matches the
> passed in uri exactly. This is a huge problem for multiple reasons.

Yes, the restriction is not in line with the Javadoc for getContext()
and needs to be relaxed.

Mark

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org

RE: Two serious issues have been introduced in Tomcat 7 and Tomcat 8...

[Doug Forrest <df...@opentext.com>]

> If ServletContext.getContext(String) is actually supposed to perform
> prefix-mapping, then it explains why getContext("/ROOT") worked for
> you.

> getContext("/ROOT") worked just because getContext("/foobar") returns
> the ROOT context when there is no "foobar" application.  There is
> nothing special in the name "/ROOT" here.  Your "issue 1" and "issue
> 2" are the same.

Agreed. I don't think that "/ROOT" is inherently meaningful. It is simply a prudent choice when looking for the ROOT context because it is a path that would be handled by the root context and it is also unlikely to match the context path of another application. 

> 2. I think this needs a clarification from Servlet EG at least to fix
> their Javadoc. [1]

> As of now,
> a) There is no explicit mention of prefix matching in [1].
> It says "uripath - a String specifying the context path of another web
> application in the container."

> b) It says "The given path must be begin with /", but context path of
> the default context is an empty string (per
> ServletContext.getContextPath()).

> The prefix matching has annoying consequence that it may return a
> different web application, not the one that you expect.

> [1] http://docs.oracle.com/javaee/7/api/javax/servlet/ServletContext.html#getContext%28java.lang.String%29

Not that I disagree with getting clarification from Servlet EG, but this is a major change in functionality. If there is a decision to make this kind of change in response to a change in the spec, it should be done in a major release, not a patch. As it is, we are going to have to modify the Supported Platforms Matrix for our product and specifically exclude a couple of patch releases of Tomcat 7 and 8. I don't want to have to exclude all future patches of Tomcat 8.

Regards,

Doug Forrest

RE: Two serious issues have been introduced in Tomcat 7 and Tomcat 8...

[Doug Forrest <df...@opentext.com>]

> I agree the API is bad in the end, you don't really know which context is
> being returned. For example if the context you're requesting is not
> deployed, you'll get the root context and a random error when using it.

I'm confused about why people think this behavior is random. It is not the API's job to protect against mis-configuration. The API itself should be predictable. If you request the context for a uri and the desired context isn't deployed, you will either get null if there is no ROOT context deployed or you will get the root context. If you get the root context and the resource you are looking for is not there, you will get a predictable error when you try to dispatch a forward or include. And it will be a configuration problem that can be easily diagnosed. "Oh, my log file says it failed to include /mysite/templates/defaulttemplate.jsp. I must not have mysite.war deployed."

If we agree that the spec allows for (not requires, just allows for) stem matching, then the way it worked previously is the exact way that it should work. Again, it is the way that every other commonly used servlet container interprets the spec.

Kind regards,

Doug


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org

Re: Two serious issues have been introduced in Tomcat 7 and Tomcat 8...

[Rémy Maucherat <re...@apache.org>]

2015-02-23 20:32 GMT+01:00 Konstantin Kolinko <kn...@gmail.com>:

> If ServletContext.getContext(String) is actually supposed to perform
> prefix-mapping, then it explains why getContext("/ROOT") worked for
> you.
>
> getContext("/ROOT") worked just because getContext("/foobar") returns
> the ROOT context when there is no "foobar" application.  There is
> nothing special in the name "/ROOT" here.  Your "issue 1" and "issue
> 2" are the same.
>
> 1. I think this needs an issue in Bugzilla for Tomcat 7.
>
> 2. I think this needs a clarification from Servlet EG at least to fix
> their Javadoc. [1]
>
> I agree the API is bad in the end, you don't really know which context is
being returned. For example if the context you're requesting is not
deployed, you'll get the root context and a random error when using it.

Rémy

Re: Two serious issues have been introduced in Tomcat 7 and Tomcat 8...

[Konstantin Kolinko <kn...@gmail.com>]

2015-02-23 21:01 GMT+03:00 Doug Forrest <df...@opentext.com>:
> The issues were introduced by r1645015 in 7.x and r1644992 in 8.x. Both issues are related to ApplicationContext.getContext(String uri).
>
> Issue 1:
>
> getContext("/ROOT") no longer works. In fact, it doesn't appear to be possible to get the ROOT context using this method any more since the literal uri for ROOT would be "" and the first functional line in the method rejects any uri that doesn't start with "/". This is a serious issue that will break many of my customers if they apply a patch containing this code.
>
> Suggested fix:
>
> if (uri.equals("/ROOT")) uri = "";
>
> at line 269.
>
> Issue 2:
>
> getContext now only returns a valid context if its path matches the passed in uri exactly. This is a huge problem for multiple reasons.
>
> This is a major change in functionality that should not have been introduced in a patch release. This code has accepted a uri that contains path information past the context root for many years across many releases. This behavior mirrors the behavior of every major servlet container available. There is a large amount of existing code out in the wild that relies on this behavior. If an intentional decision were made to change this method to require an exact match, it would be prudent to make such a change in a major release to give users and integrators an opportunity to react to the change.
>
> I don't have a suggested fix for this issue, but I would be happy to come up with one if required.
>
> Thank you for your time.
>


If ServletContext.getContext(String) is actually supposed to perform
prefix-mapping, then it explains why getContext("/ROOT") worked for
you.

getContext("/ROOT") worked just because getContext("/foobar") returns
the ROOT context when there is no "foobar" application.  There is
nothing special in the name "/ROOT" here.  Your "issue 1" and "issue
2" are the same.

1. I think this needs an issue in Bugzilla for Tomcat 7.

2. I think this needs a clarification from Servlet EG at least to fix
their Javadoc. [1]

As of now,
a) There is no explicit mention of prefix matching in [1].
It says "uripath - a String specifying the context path of another web
application in the container."

b) It says "The given path must be begin with /", but context path of
the default context is an empty string (per
ServletContext.getContextPath()).

The prefix matching has annoying consequence that it may return a
different web application, not the one that you expect.


[1] http://docs.oracle.com/javaee/7/api/javax/servlet/ServletContext.html#getContext%28java.lang.String%29

Best regards,
Konstantin Kolinko

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org