[MAGNOLIA-6025] Magnolia returns internal server error for selectors and request parameters with value '%' Created: 17/Dec/14 Updated: 28/Aug/15 Resolved: 09/Jan/15 |
|
| Status: | Closed |
| Project: | Magnolia |
| Component/s: | None |
| Affects Version/s: | 5.3.6 |
| Fix Version/s: | 5.3.7 |
| Type: | Bug | Priority: | Neutral |
| Reporter: | Richard Gange | Assignee: | Jaroslav Simak |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | support | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| 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
|
||||||||||||
| Date of First Response: | |||||||||||||
| Description |
|
If you manipulate any Magnolia URL by adding a (possibly dummy) request parameter with value '%' Magnolia returns an ugly 500 internal server error from the filter chain. I can reproduce this on any Magnolia site. E.g. This results in a stack trace in the log: java.lang.IllegalArgumentException: URLDecoder: Incomplete trailing escape (%) pattern at java.net.URLDecoder.decode(URLDecoder.java:187) at info.magnolia.cms.filters.ContentTypeFilter.doFilter(ContentTypeFilter.java:95) at info.magnolia.cms.filters.AbstractMgnlFilter.doFilter(AbstractMgnlFilter.java:89) at info.magnolia.cms.filters.MgnlFilterChain.doFilter(MgnlFilterChain.java:80) at info.magnolia.cms.filters.ContextFilter.doFilter(ContextFilter.java:129) at info.magnolia.cms.filters.AbstractMgnlFilter.doFilter(AbstractMgnlFilter.java:89) at info.magnolia.cms.filters.MgnlFilterChain.doFilter(MgnlFilterChain.java:80) at info.magnolia.cms.filters.CompositeFilter.doFilter(CompositeFilter.java:65) at info.magnolia.cms.filters.AbstractMgnlFilter.doFilter(AbstractMgnlFilter.java:89) at info.magnolia.cms.filters.SafeDestroyMgnlFilterWrapper.doFilter(SafeDestroyMgnlFilterWrapper.java:106) at info.magnolia.cms.filters.MgnlFilterDispatcher.doDispatch(MgnlFilterDispatcher.java:66) at info.magnolia.cms.filters.MgnlMainFilter.doFilter(MgnlMainFilter.java:107) at info.magnolia.cms.filters.MgnlMainFilter.doFilter(MgnlMainFilter.java:93) at org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1645) at org.eclipse.jetty.servlet.ServletHandler.doHandle(ServletHandler.java:564) at org.eclipse.jetty.server.handler.ScopedHandler.handle(ScopedHandler.java:143) at org.eclipse.jetty.security.SecurityHandler.handle(SecurityHandler.java:596) at org.eclipse.jetty.server.session.SessionHandler.doHandle(SessionHandler.java:221) at org.eclipse.jetty.server.handler.ContextHandler.doHandle(ContextHandler.java:1111) at org.eclipse.jetty.servlet.ServletHandler.doScope(ServletHandler.java:498) at org.eclipse.jetty.server.session.SessionHandler.doScope(SessionHandler.java:183) at org.eclipse.jetty.server.handler.ContextHandler.doScope(ContextHandler.java:1045) at org.eclipse.jetty.server.handler.ScopedHandler.handle(ScopedHandler.java:141) at org.eclipse.jetty.server.Dispatcher.forward(Dispatcher.java:191) at org.eclipse.jetty.server.Dispatcher.error(Dispatcher.java:77) at org.eclipse.jetty.server.handler.ErrorHandler.handle(ErrorHandler.java:91) at org.eclipse.jetty.server.Response.sendError(Response.java:589) at org.eclipse.jetty.server.Response.sendError(Response.java:547) at org.eclipse.jetty.servlet.ServletHandler.doHandle(ServletHandler.java:626) [..] Same error also happens when '%' sign is in selector parameters: java.lang.IllegalArgumentException: URLDecoder: Incomplete trailing escape (%) pattern java.net.URLDecoder.decode(URLDecoder.java:187) info.magnolia.cms.core.AggregationState.setSelector(AggregationState.java:247) info.magnolia.cms.filters.RepositoryMappingFilter.doFilter(RepositoryMappingFilter.java:97) info.magnolia.cms.filters.AbstractMgnlFilter.doFilter(AbstractMgnlFilter.java:89) info.magnolia.cms.filters.MgnlFilterChain.doFilter(MgnlFilterChain.java:79) info.magnolia.cms.filters.CompositeFilter.doFilter(CompositeFilter.java:65) info.magnolia.cms.filters.AbstractMgnlFilter.doFilter(AbstractMgnlFilter.java:89) info.magnolia.cms.filters.MgnlFilterChain.doFilter(MgnlFilterChain.java:79) info.magnolia.cms.filters.MgnlFilterChain.doFilter(MgnlFilterChain.java:74) info.magnolia.cms.filters.MgnlFilterChain.doFilter(MgnlFilterChain.java:81) info.magnolia.cms.filters.MgnlFilterChain.doFilter(MgnlFilterChain.java:81) info.magnolia.cms.filters.MgnlFilterChain.doFilter(MgnlFilterChain.java:81) info.magnolia.cms.filters.MgnlFilterChain.doFilter(MgnlFilterChain.java:81) info.magnolia.cms.filters.MgnlFilterChain.doFilter(MgnlFilterChain.java:81) info.magnolia.cms.filters.MgnlFilterChain.doFilter(MgnlFilterChain.java:81) info.magnolia.cms.filters.MgnlFilterChain.doFilter(MgnlFilterChain.java:81) info.magnolia.cms.filters.MgnlFilterChain.doFilter(MgnlFilterChain.java:81) info.magnolia.cms.filters.MgnlFilterChain.doFilter(MgnlFilterChain.java:81) info.magnolia.cms.filters.MgnlFilterChain.doFilter(MgnlFilterChain.java:81) info.magnolia.cms.filters.MgnlFilterChain.doFilter(MgnlFilterChain.java:81) info.magnolia.cms.filters.CompositeFilter.doFilter(CompositeFilter.java:65) info.magnolia.cms.filters.AbstractMgnlFilter.doFilter(AbstractMgnlFilter.java:89) info.magnolia.cms.filters.MgnlFilterChain.doFilter(MgnlFilterChain.java:79) info.magnolia.cms.filters.VirtualUriFilter.doFilter(VirtualUriFilter.java:69) info.magnolia.cms.filters.AbstractMgnlFilter.doFilter(AbstractMgnlFilter.java:89) info.magnolia.cms.filters.MgnlFilterChain.doFilter(MgnlFilterChain.java:79) info.magnolia.module.cache.executor.Bypass.processCacheRequest(Bypass.java:58) info.magnolia.module.cache.executor.CompositeExecutor.processCacheRequest(CompositeExecutor.java:66) info.magnolia.module.cache.filter.CacheFilter.doFilter(CacheFilter.java:153) info.magnolia.cms.filters.OncePerRequestAbstractMgnlFilter.doFilter(OncePerRequestAbstractMgnlFilter.java:59) info.magnolia.cms.filters.MgnlFilterChain.doFilter(MgnlFilterChain.java:79) info.magnolia.cms.i18n.I18nContentSupportFilter.doFilter(I18nContentSupportFilter.java:74) info.magnolia.cms.filters.AbstractMgnlFilter.doFilter(AbstractMgnlFilter.java:89) info.magnolia.cms.filters.MgnlFilterChain.doFilter(MgnlFilterChain.java:79) info.magnolia.cms.filters.RangeSupportFilter.doFilter(RangeSupportFilter.java:84) info.magnolia.cms.filters.AbstractMgnlFilter.doFilter(AbstractMgnlFilter.java:89) info.magnolia.cms.filters.MgnlFilterChain.doFilter(MgnlFilterChain.java:79) info.magnolia.cms.security.BaseSecurityFilter.doFilter(BaseSecurityFilter.java:57) info.magnolia.cms.filters.AbstractMgnlFilter.doFilter(AbstractMgnlFilter.java:89) info.magnolia.cms.filters.MgnlFilterChain.doFilter(MgnlFilterChain.java:79) info.magnolia.cms.filters.MgnlFilterChain.doFilter(MgnlFilterChain.java:81) info.magnolia.cms.security.SecurityCallbackFilter.doFilter(SecurityCallbackFilter.java:80) info.magnolia.cms.filters.OncePerRequestAbstractMgnlFilter.doFilter(OncePerRequestAbstractMgnlFilter.java:59) info.magnolia.cms.filters.MgnlFilterChain.doFilter(MgnlFilterChain.java:79) info.magnolia.cms.security.LogoutFilter.doFilter(LogoutFilter.java:94) info.magnolia.cms.filters.OncePerRequestAbstractMgnlFilter.doFilter(OncePerRequestAbstractMgnlFilter.java:59) info.magnolia.cms.filters.MgnlFilterChain.doFilter(MgnlFilterChain.java:79) info.magnolia.module.templatingkit.filters.SiteMergeFilter.doFilter(SiteMergeFilter.java:112) info.magnolia.cms.filters.AbstractMgnlFilter.doFilter(AbstractMgnlFilter.java:89) info.magnolia.cms.filters.MgnlFilterChain.doFilter(MgnlFilterChain.java:79) info.magnolia.cms.filters.MultiChannelFilter.doFilter(MultiChannelFilter.java:82) info.magnolia.cms.filters.OncePerRequestAbstractMgnlFilter.doFilter(OncePerRequestAbstractMgnlFilter.java:59) info.magnolia.cms.filters.MgnlFilterChain.doFilter(MgnlFilterChain.java:79) info.magnolia.module.cache.filter.GZipFilter.doFilter(GZipFilter.java:73) info.magnolia.cms.filters.OncePerRequestAbstractMgnlFilter.doFilter(OncePerRequestAbstractMgnlFilter.java:59) info.magnolia.cms.filters.MgnlFilterChain.doFilter(MgnlFilterChain.java:79) info.magnolia.cms.filters.MgnlFilterChain.doFilter(MgnlFilterChain.java:81) info.magnolia.cms.security.auth.login.LoginFilter.doFilter(LoginFilter.java:120) info.magnolia.cms.filters.AbstractMgnlFilter.doFilter(AbstractMgnlFilter.java:89) info.magnolia.cms.filters.MgnlFilterChain.doFilter(MgnlFilterChain.java:79) info.magnolia.cms.filters.MgnlFilterChain.doFilter(MgnlFilterChain.java:81) info.magnolia.cms.filters.CosMultipartRequestFilter.doFilter(CosMultipartRequestFilter.java:87) info.magnolia.cms.filters.OncePerRequestAbstractMgnlFilter.doFilter(OncePerRequestAbstractMgnlFilter.java:59) info.magnolia.cms.filters.MgnlFilterChain.doFilter(MgnlFilterChain.java:79) info.magnolia.module.devicedetection.filter.DeviceDetectionFilter.doFilter(DeviceDetectionFilter.java:71) info.magnolia.cms.filters.OncePerRequestAbstractMgnlFilter.doFilter(OncePerRequestAbstractMgnlFilter.java:59) info.magnolia.cms.filters.MgnlFilterChain.doFilter(MgnlFilterChain.java:79) info.magnolia.cms.filters.ContentTypeFilter.doFilter(ContentTypeFilter.java:103) info.magnolia.cms.filters.AbstractMgnlFilter.doFilter(AbstractMgnlFilter.java:89) info.magnolia.cms.filters.MgnlFilterChain.doFilter(MgnlFilterChain.java:79) info.magnolia.cms.filters.ContextFilter.doFilter(ContextFilter.java:128) info.magnolia.cms.filters.AbstractMgnlFilter.doFilter(AbstractMgnlFilter.java:89) info.magnolia.cms.filters.MgnlFilterChain.doFilter(MgnlFilterChain.java:79) info.magnolia.cms.filters.CompositeFilter.doFilter(CompositeFilter.java:65) info.magnolia.cms.filters.AbstractMgnlFilter.doFilter(AbstractMgnlFilter.java:89) info.magnolia.cms.filters.SafeDestroyMgnlFilterWrapper.doFilter(SafeDestroyMgnlFilterWrapper.java:107) info.magnolia.cms.filters.MgnlFilterDispatcher.doDispatch(MgnlFilterDispatcher.java:67) info.magnolia.cms.filters.MgnlMainFilter.doFilter(MgnlMainFilter.java:108) info.magnolia.cms.filters.MgnlMainFilter.doFilter(MgnlMainFilter.java:94) |
| Comments |
| Comment by Richard Gange [ 17/Dec/14 ] |
|
Customer Care Score: 5+2+5+3+5+5=25 |
| Comment by Jaroslav Simak [ 07/Jan/15 ] |
|
Use case where % in parameter (or selector) returns error 500 is when percent-decoding pattern is not complete (% must be followed by two hexadecimal characters - see http://en.wikipedia.org/wiki/Percent-encoding). I was thinking about encoding the query string (selector) parameter values, but that might lead to double encoded characters which is not acceptable. Solution might be to simply check if query string (selector) parameter value starts (or ends) with % and is followed by at least two hexadecimal characters. If not then we could simply replace % sign with %25 (which encoded) to solve the problem. |
| Comment by Jan Haderka [ 07/Jan/15 ] |
|
but what if it does? e.g. http://...this/positive?is=%false |
| Comment by Jaroslav Simak [ 07/Jan/15 ] |
|
This is decoded in our current impl as http://...this/positive?is=�lse and set as originalUrl in AggregationState .. so this would remain the same if i would apply the solution mentioned above. Imho we should expect that all urls are properly encoded so we can decode them in our code. And afaict its good practice to encode all urls that are generated. I should also mention that we use http://docs.oracle.com/javase/7/docs/api/java/net/URLDecoder.html. But we should not use it imho, as it says in its description that its meant for HTML forms (and not all urls with parameters were submitted from a form). |
| Comment by Espen Jervidalo [ 15/Jan/15 ] |
|
I added fix Version 5.2.10 |
| Comment by Espen Jervidalo [ 15/Jan/15 ] |
|
Correct me if I'm wrong, but the solution is not what was proposed in the comments. |
| Comment by Jaroslav Simak [ 15/Jan/15 ] |
|
I had a discussion with Jan and we decided to go with 400 solution. I should have mention it in the comment, sorry about that. |
| Comment by Espen Jervidalo [ 20/Jan/15 ] |
|
Ok, I removed fixVersions 4.5 and 5.2. The customer uses Magnolia 5.3.5. |
| Comment by Jan Haderka [ 21/Jan/15 ] |
|
To 5.4 you mean, no? |