[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:
Cloners
is cloned by MAGNOLIA-6272 Magnolia returns internal server erro... Closed
causality
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.
http://www.magnolia-cms.com/product/features.html?test=%

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:
http://localhost:8080/demo-project/about~aa=dd%~.html

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.
The filter-chain will be aborted and a 400 error code will be returned.

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.
Still needs to be backported to 5.3 though.

Comment by Jan Haderka [ 21/Jan/15 ]

To 5.4 you mean, no?

Generated at Mon Feb 12 04:10:36 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.