Bug 34805 - warn about invalid security constraint url patterns
Summary: warn about invalid security constraint url patterns
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 5
Classification: Unclassified
Component: Catalina (show other bugs)
Version: Nightly Build
Hardware: Other All
: P2 enhancement (vote)
Target Milestone: ---
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2005-05-08 20:20 UTC by Ralf Hauser
Modified: 2011-04-08 17:51 UTC (History)
0 users



Attachments
RealmBase.java.patch (1.90 KB, patch)
2005-05-08 20:30 UTC, Ralf Hauser
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ralf Hauser 2005-05-08 20:20:07 UTC
since that is by far not anything like regular expressions or anything known
e.g. from ant's build.xml
Comment 1 Ralf Hauser 2005-05-08 20:30:53 UTC
Created attachment 14967 [details]
RealmBase.java.patch

for example /login.do will not be matched by 
  <url-pattern>/login*</url-pattern>
as per section 11.2 of the Servlet API
Specification
(http://java.sun.com/aboutJava/communityprocess/first/jsr053/servlet23_PFD.pdf)
see also http://java.sun.com/dtd/web-app_2_3.dtd
Comment 2 william.barker 2005-05-09 01:46:00 UTC
I'm -1 to the patch, as is.  A <url-pattern>/login*</url-pattern> is a 
perfectly valid (if somewhat strange :) exact-match pattern, so Tomcat can't 
fault it.

I'm +1 to adding a log.warn to SecurityCollection.addPattern for questionable 
patterns like this, since it could only reduce the questions on tomcat-user.
Comment 3 Yoav Shapira 2005-07-21 22:15:13 UTC
Warning added to SecurityCollection.  Thanks for this useful suggestion.
Comment 4 Milt Taylor 2006-08-01 08:41:09 UTC
I think there might be a bug in this implementation which is reporting
legitimate url-mappings as questionable.

If I understand the original intent of this patch, should not the line that
currently appears as:

if (pattern.charAt(pattern.length()-1) != '/') {

should be:

if (pattern.charAt(pattern.length()-2) != '/') {


What do you think?
Comment 5 Takayuki Kaneko 2006-08-14 10:19:56 UTC
(In reply to comment #4)

I'm approving of comment #4.
And I think that level of loggings should be unified.   

if (log.isDebugEnabled()) {
  log.debug(...);
}

or

if (log.isWarnEnabled()) {
  log.warn(...);
}

Thanks.
Comment 6 Ralf Hauser 2006-08-14 12:59:52 UTC
Thanks for pointing out my counting error.

see bug 39364 for a discussion of the broader context of such constraint urls.
Comment 7 Marius Scurtescu 2007-08-09 15:14:14 UTC
It seems that this patch was applied to SecurityCollection, but without the fix
mentioned in comment #4.

Submitted a separate bug to fix SecurityCollection, bug 43079
Comment 8 Mark Thomas 2011-04-08 17:51:48 UTC
This was fixed for all currently supported Tomcat versions some time ago.