Bug 62527 - Lookupname Validation in ResourceBase
Summary: Lookupname Validation in ResourceBase
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 8
Classification: Unclassified
Component: Catalina (show other bugs)
Version: 8.5.x-trunk
Hardware: PC Mac OS X 10.1
: P2 enhancement (vote)
Target Milestone: ----
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-07-10 06:34 UTC by Gurkan Erdogdu
Modified: 2018-07-10 12:23 UTC (History)
0 users



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Gurkan Erdogdu 2018-07-10 06:34:16 UTC
Hello all,

In the svn commit: r1831256 - java/org/apache/tomcat/util/descripto/web/ResourceBase.java , we have changed the lookupname validation which checks that lookup starts with java:/ namespace. But, as extensively used in Apache TomEE, users have already been using openejb:/ namespace in their lookup names and all working until this commit. Can we do something configurable for this validation check?

Regards.
Gurkan
Comment 1 Remy Maucherat 2018-07-10 09:14:50 UTC
I think r1831256 should be partially reverted due to this existing use. I plan to do it shortly unless there's a disagreement.
Comment 2 Mark Thomas 2018-07-10 09:17:59 UTC
This behaviour is required by the Java EE spec. I am therefore moving this to an enhancement to add an option to ignore the specification requirement.

Implementation looks to be non-trivial. If implemented, moving the check to the NamingContextListener looks like the best option as that has access to the Context/Server so this could be configured per Context rather than globally.

Fixing the non-specification compliant usage would be a better solution.
Comment 3 Remy Maucherat 2018-07-10 09:35:15 UTC
Well, ok, but there is existing use and I don't see a benefit of being strict here (it's not like a URL or HTTP element where security issues can occur). So the fix sounds more like a regression than an improvement to me. I think it would be ok to do it in Tomcat.next, but probably not here, at least not by default.
Comment 4 romain.manni-bucau 2018-07-10 09:47:44 UTC
Yes, issue is really it breaks a lot of users (we already lived that years ago and all apps needed to be reconfigured).

There is a flag for strict compliance in tomcat already so maybe this is a behavior falling into that "disabled by default but activable" behavior. Will not hurt users. Also note that it conflicts with JNDI spec which enables to have any namespace so you can read the spec from the point of view matching your need in such a case.
Comment 5 Mark Thomas 2018-07-10 11:21:35 UTC
I've had another read of the relevant bits of the Java EE spec and I can't see a clear reason for this limitation. There are some hints that suggest it is to do with the scopes of the various defined namespaces and making sure the application sees the JNDI context it expects. But I don't see why that should stop some other namespace being used - obviously with the onus on the user to make sure they are using it correctly / it behaves as they expect w.r.t. returned types, shared/unique instances, visibility scope etc.

Given that this change was only applied because it was noticed that the validation was missing while reviewing the lookup name implementation - rather than as a result of a bug report or similar - then I've no objection to the validation being reverted if that is the preferred option. Note it would need to be reverted from all current versions.
Comment 6 Remy Maucherat 2018-07-10 12:23:22 UTC
Nice, it's a lot simpler this way !
The fix will be in 9.0.11, 8.5.33, 8.0.54 and 7.0.91.