[MAGNOLIA-3268] SimpleUrlPattern crashes (infinite loop) for certain pattern and URL combinations Created: 04/Aug/10 Updated: 12/Aug/10 Resolved: 09/Aug/10 |
|
| Status: | Closed |
| Project: | Magnolia |
| Component/s: | core |
| Affects Version/s: | 4.3.2 |
| Fix Version/s: | 4.3.6 |
| Type: | Bug | Priority: | Critical |
| Reporter: | Benjamin Cody | Assignee: | Ondrej Chytil |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Environment: |
1.6.0_07 and 1.6.0_16, win32. |
||
| Attachments: |
|
||||||||||||
| Issue Links: |
|
||||||||||||
| Template: |
|
||||||||||||
| Acceptance criteria: |
Empty
|
||||||||||||
| Task DoD: |
[ ]*
Doc/release notes changes? Comment present?
[ ]*
Downstream builds green?
[ ]*
Solution information and context easily available?
[ ]*
Tests
[ ]*
FixVersion filled and not yet released
[ ] 
Architecture Decision Record (ADR)
|
||||||||||||
| Bug DoR: |
[ ]*
Steps to reproduce, expected, and actual results filled
[ ]*
Affected version filled
|
||||||||||||
| Testcase included: |
Yes
|
||||||||||||
| Date of First Response: | |||||||||||||
| Description |
|
As of 4.3.2, in which the constant URL_CHAR_PATTERN in SimpleUrlPattern was changed, some URL pattern matches cause hang-ups. import info.magnolia.cms.util.SimpleUrlPattern; import org.junit.Assert; import org.junit.Test; public class SimpleUrlPatternTest { @Test public void testDotDoPattern() { final SimpleUrlPattern sup = new SimpleUrlPattern("*\\.do"); Assert.assertTrue(sup.match("/path/myController.do")); //This still works Assert.assertFalse(sup.match("/.resources/enterprise-css/registration.css")); //This hangs up } } I noticed this because I have a URL bypass for Spring controller URLs which I've mapped to *.do extensions. It seems SimpleUrlPattern converts *\\.do to ()(.*\\n*)*\\.do internally, which causes an infinite loop in java.util.regex.Pattern. I tested this with both JDK 1.6.0_07 and 1.6.0_16. |
| Comments |
| Comment by Jan Haderka [ 04/Aug/10 ] |
|
proposed patch - enable line end matching by . instead of adding |
| Comment by Philipp Bärfuss [ 06/Aug/10 ] |
|
I think URL_CHAR_PATTERN is just wrong. It was meant to be a character set and not a pattern and hence should not have any qualifiers as *?+ in it. Even if it doesn't run into an endless loop the performance will be bad. So yes the patch solves the issue but I would go one step further and deprecate URL_CHAR_PATTERN. |
| Comment by Fabrizio Giustina [ 06/Aug/10 ] |
|
Just a note about why the "\n" was added: although it's not common to have newlines in a URL without that on a public website any request with a %0A in the URL ends up in displaying the magnolia login page, which is not desired. A better solution could be avoiding regexp at all, using something like ant patterns (there should be already a jira somewhere), in the meanwhile we should find a way to fix it without getting back the %0A problem... |
| Comment by Magnolia International [ 06/Aug/10 ] |
|
Ant-style patterns will also result in 401s because they won't match malformed requests either, and thus the uri-security-filter will get a "false" from the isAuthorized() method; to avoid it completely, maybe we'd a request-validating filter in front ? Something, earlier in the chain, that would return 404s (not sure where it happens at the moment) |