Wicket
  1. Wicket
  2. WICKET-4803

UrlDecoder should log a message when invalid input is provided

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.5.8, 6.5.0
    • Fix Version/s: 1.5.10, 6.7.0, 1.4.22
    • Component/s: wicket
    • Labels:
      None

      Description

      In the class: org.apache.wicket.protocol.http.WicketURLDecoder there are two IllegalArgumentException which should be wrapped in WicketRuntimeException, otherwise they are caught by the exception handler form the servlet container (jetty, tomcat, ...) which then uses their http 500 error code configuration instead of the exception handling of wicket.

      Wrapping them would be good for consistency and help manage runtime exceptions.
      These are the two exceptions:
      throw new IllegalArgumentException("URLDecoder: Incomplete trailing escape (%) pattern");
      throw new IllegalArgumentException("URLDecoder: Illegal hex characters in escape (%) pattern - " + e.getMessage());

        Issue Links

          Activity

          Hide
          Martin Grigorov added a comment -

          Here is a link to a discussion in Tomcat dev@ mailing list about changing this behavior: http://markmail.org/thread/aymssrjhextkraip

          Show
          Martin Grigorov added a comment - Here is a link to a discussion in Tomcat dev@ mailing list about changing this behavior: http://markmail.org/thread/aymssrjhextkraip
          Martin Grigorov made changes -
          Remote Link This issue links to "Discussion about changing the behavior in Tomcat (Web Link)" [ 12059 ]
          Martin Grigorov made changes -
          Fix Version/s 1.4.22 [ 12324070 ]
          Hide
          Jan Riehn added a comment -

          Hej Martin,

          this is also my preferred solution. Thank you!

          Regards,

          Jan

          Show
          Jan Riehn added a comment - Hej Martin, this is also my preferred solution. Thank you! Regards, Jan
          Martin Grigorov made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Fix Version/s 6.7.0 [ 12323964 ]
          Resolution Fixed [ 1 ]
          Hide
          Martin Grigorov added a comment -

          From now on an info message will be logged and the problematic % character will be ignored.

          The problem with the thrown exception is that the application logs can be easily flooded by an attacker.

          Show
          Martin Grigorov added a comment - From now on an info message will be logged and the problematic % character will be ignored. The problem with the thrown exception is that the application logs can be easily flooded by an attacker.
          Martin Grigorov made changes -
          Summary Unwrapped IllegalArgumentException in WicketURLDecoder UrlDecoder should log a message when invalid input is provided
          Hide
          Martin Grigorov added a comment -

          With Servlet it is the same:

          • Jetty throws exception:
            java.lang.IllegalArgumentException: !hex:25
            at org.eclipse.jetty.util.TypeUtil.convertHexDigit(TypeUtil.java:364)
            at org.eclipse.jetty.util.UrlEncoded.decodeUtf8To(UrlEncoded.java:329)
            at org.eclipse.jetty.http.HttpURI.decodeQueryTo(HttpURI.java:638)
            at org.eclipse.jetty.server.Request.extractParameters(Request.java:213)
            at org.eclipse.jetty.server.Request.getParameter(Request.java:679)
            at com.example.EchoServlet.handle(EchoServlet.java:29)
            at com.example.EchoServlet.doGet(EchoServlet.java:24)
            at javax.servlet.http.HttpServlet.service(HttpServlet.java:707)
            at javax.servlet.http.HttpServlet.service(HttpServlet.java:820)
            at org.eclipse.jetty.servlet.ServletHolder.handle(ServletHolder.java:565)
            at org.eclipse.jetty.servlet.ServletHandler.doHandle(ServletHandler.java:479)
            at org.eclipse.jetty.server.handler.ScopedHandler.handle(ScopedHandler.java:119)
            at org.eclipse.jetty.security.SecurityHandler.handle(SecurityHandler.java:524)
          • Tomcat does nothing, just ignores the broken part of the query string

          The requested url is: http://localhost:8080/echo?a=c%%%b and Tomcat says that the value for parameter 'a' is null.

          Show
          Martin Grigorov added a comment - With Servlet it is the same: Jetty throws exception: java.lang.IllegalArgumentException: !hex:25 at org.eclipse.jetty.util.TypeUtil.convertHexDigit(TypeUtil.java:364) at org.eclipse.jetty.util.UrlEncoded.decodeUtf8To(UrlEncoded.java:329) at org.eclipse.jetty.http.HttpURI.decodeQueryTo(HttpURI.java:638) at org.eclipse.jetty.server.Request.extractParameters(Request.java:213) at org.eclipse.jetty.server.Request.getParameter(Request.java:679) at com.example.EchoServlet.handle(EchoServlet.java:29) at com.example.EchoServlet.doGet(EchoServlet.java:24) at javax.servlet.http.HttpServlet.service(HttpServlet.java:707) at javax.servlet.http.HttpServlet.service(HttpServlet.java:820) at org.eclipse.jetty.servlet.ServletHolder.handle(ServletHolder.java:565) at org.eclipse.jetty.servlet.ServletHandler.doHandle(ServletHandler.java:479) at org.eclipse.jetty.server.handler.ScopedHandler.handle(ScopedHandler.java:119) at org.eclipse.jetty.security.SecurityHandler.handle(SecurityHandler.java:524) Tomcat does nothing, just ignores the broken part of the query string The requested url is: http://localhost:8080/echo?a=c%%%b and Tomcat says that the value for parameter 'a' is null.
          Hide
          Martin Grigorov added a comment -

          Well, it has been like this since several years.
          I'm not sure whether this behavior is correct. I'll test with pure servlet to see what is the behavior.

          Maybe we should change this to log a warning instead of throwing this unhandleable exception.

          Show
          Martin Grigorov added a comment - Well, it has been like this since several years. I'm not sure whether this behavior is correct. I'll test with pure servlet to see what is the behavior. Maybe we should change this to log a warning instead of throwing this unhandleable exception.
          Hide
          Jan Riehn added a comment -

          you figured out an specific web container issue. using a tomcat, you'll receive a bad request (http-status 400). the NumberFormatException, respectively the IllegalArgumentException, could be detected by wicket.
          sorry, that i lead you astray - it's not the question, how to offer an error page for this behavior, but whether this behavior of wicket is desired.

          Show
          Jan Riehn added a comment - you figured out an specific web container issue. using a tomcat, you'll receive a bad request (http-status 400). the NumberFormatException, respectively the IllegalArgumentException, could be detected by wicket. sorry, that i lead you astray - it's not the question, how to offer an error page for this behavior, but whether this behavior of wicket is desired.
          Hide
          Martin Grigorov added a comment - - edited

          I'm not sure Wicket can do something about this.

          Issuing http://localhost:8080/%%%% (note the missing '?') leads to :
          java.lang.NumberFormatException: %%
          at org.eclipse.jetty.util.TypeUtil.parseInt(TypeUtil.java:320)
          at org.eclipse.jetty.http.HttpURI.getDecodedPath(HttpURI.java:560)
          at org.eclipse.jetty.server.AbstractHttpConnection.handleRequest(AbstractHttpConnection.java:440)
          at org.eclipse.jetty.server.BlockingHttpConnection.handleRequest(BlockingHttpConnection.java:47)
          at org.eclipse.jetty.server.AbstractHttpConnection.headerComplete(AbstractHttpConnection.java:884)
          at org.eclipse.jetty.server.AbstractHttpConnection$RequestHandler.headerComplete(AbstractHttpConnection.java:938)
          at org.eclipse.jetty.http.HttpParser.parseNext(HttpParser.java:630)
          at org.eclipse.jetty.http.HttpParser.parseAvailable(HttpParser.java:230)
          at org.eclipse.jetty.server.BlockingHttpConnection.handle(BlockingHttpConnection.java:66)
          at org.eclipse.jetty.server.bio.SocketConnector$ConnectorEndPoint.run(SocketConnector.java:254)
          at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:603)
          at org.eclipse.jetty.util.thread.QueuedThreadPool$3.run(QueuedThreadPool.java:538)
          at java.lang.Thread.run(Thread.java:662)

          i.e. again error 500.

          The only way to show a Wicket page as a handler is to add:
          <error-page>
          <error-code>500</error-code>
          <location>/error</location>
          </error-page>

          to web.xml and
          #mountPage("error", getApplicationSettings().getInternalErrorPage());

          Show
          Martin Grigorov added a comment - - edited I'm not sure Wicket can do something about this. Issuing http://localhost:8080/%%%% (note the missing '?') leads to : java.lang.NumberFormatException: %% at org.eclipse.jetty.util.TypeUtil.parseInt(TypeUtil.java:320) at org.eclipse.jetty.http.HttpURI.getDecodedPath(HttpURI.java:560) at org.eclipse.jetty.server.AbstractHttpConnection.handleRequest(AbstractHttpConnection.java:440) at org.eclipse.jetty.server.BlockingHttpConnection.handleRequest(BlockingHttpConnection.java:47) at org.eclipse.jetty.server.AbstractHttpConnection.headerComplete(AbstractHttpConnection.java:884) at org.eclipse.jetty.server.AbstractHttpConnection$RequestHandler.headerComplete(AbstractHttpConnection.java:938) at org.eclipse.jetty.http.HttpParser.parseNext(HttpParser.java:630) at org.eclipse.jetty.http.HttpParser.parseAvailable(HttpParser.java:230) at org.eclipse.jetty.server.BlockingHttpConnection.handle(BlockingHttpConnection.java:66) at org.eclipse.jetty.server.bio.SocketConnector$ConnectorEndPoint.run(SocketConnector.java:254) at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:603) at org.eclipse.jetty.util.thread.QueuedThreadPool$3.run(QueuedThreadPool.java:538) at java.lang.Thread.run(Thread.java:662) i.e. again error 500. The only way to show a Wicket page as a handler is to add: <error-page> <error-code>500</error-code> <location>/error</location> </error-page> to web.xml and #mountPage("error", getApplicationSettings().getInternalErrorPage());
          Martin Grigorov made changes -
          Affects Version/s 6.5.0 [ 12323540 ]
          Martin Funk made changes -
          Comment [ Hi,

          no quickstart needed. Just do it live http://www.wicket-library.com/wicket-examples/index.html?%%% ]
          Martin Grigorov made changes -
          Assignee Martin Grigorov [ mgrigorov ]
          Martin Grigorov made changes -
          Fix Version/s 1.5.10 [ 12323510 ]
          Hide
          Jan Riehn added a comment - - edited

          Hej Martin,

          maybe the problem is not well demonstrated. this issue can be reconstructed using the wicket 1.5.9 quickstart http://wicket.apache.org/start/quickstart.html. An invalid request like "http://localhost:8080/?%%%" throws the following exception:

          java.lang.IllegalArgumentException: URLDecoder: Illegal hex characters in escape (%) pattern - For input string: "%%"
          at org.apache.wicket.request.UrlDecoder.decode(UrlDecoder.java:162)
          at org.apache.wicket.request.UrlDecoder.decode(UrlDecoder.java:76)
          at org.apache.wicket.request.Url.decodeParameter(Url.java:601)
          at org.apache.wicket.request.Url.parseQueryParameter(Url.java:104)
          at org.apache.wicket.request.Url.parse(Url.java:243)
          at org.apache.wicket.protocol.http.servlet.ServletWebRequest.getContextRelativeUrl(ServletWebRequest.java:222)
          at org.apache.wicket.protocol.http.servlet.ServletWebRequest.<init>(ServletWebRequest.java:126)
          at org.apache.wicket.protocol.http.servlet.ServletWebRequest.<init>(ServletWebRequest.java:83)
          at org.apache.wicket.protocol.http.WebApplication.newWebRequest(WebApplication.java:413)
          at org.apache.wicket.protocol.http.WebApplication.createWebRequest(WebApplication.java:458)
          at org.apache.wicket.protocol.http.WicketFilter.processRequest(WicketFilter.java:183)
          at org.apache.wicket.protocol.http.WicketFilter.doFilter(WicketFilter.java:244)
          at org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1326)
          at org.eclipse.jetty.servlet.ServletHandler.doHandle(ServletHandler.java:479)
          at org.eclipse.jetty.server.handler.ScopedHandler.handle(ScopedHandler.java:119)
          at org.eclipse.jetty.security.SecurityHandler.handle(SecurityHandler.java:520)
          at org.eclipse.jetty.server.session.SessionHandler.doHandle(SessionHandler.java:227)
          at org.eclipse.jetty.server.handler.ContextHandler.doHandle(ContextHandler.java:940)
          at org.eclipse.jetty.servlet.ServletHandler.doScope(ServletHandler.java:409)
          at org.eclipse.jetty.server.session.SessionHandler.doScope(SessionHandler.java:186)
          at org.eclipse.jetty.server.handler.ContextHandler.doScope(ContextHandler.java:874)
          at org.eclipse.jetty.server.handler.ScopedHandler.handle(ScopedHandler.java:117)
          at org.eclipse.jetty.server.handler.HandlerWrapper.handle(HandlerWrapper.java:110)
          at org.eclipse.jetty.server.Server.handle(Server.java:349)
          at org.eclipse.jetty.server.HttpConnection.handleRequest(HttpConnection.java:441)
          at org.eclipse.jetty.server.HttpConnection$RequestHandler.headerComplete(HttpConnection.java:904)
          at org.eclipse.jetty.http.HttpParser.parseNext(HttpParser.java:565)
          at org.eclipse.jetty.http.HttpParser.parseAvailable(HttpParser.java:217)
          at org.eclipse.jetty.server.BlockingHttpConnection.handle(BlockingHttpConnection.java:50)
          at org.eclipse.jetty.server.bio.SocketConnector$ConnectorEndPoint.run(SocketConnector.java:245)
          at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:598)
          at org.eclipse.jetty.util.thread.QueuedThreadPool$3.run(QueuedThreadPool.java:533)
          at java.lang.Thread.run(Unknown Source)

          Is there any advice on how to handle such invalid requests?

          Show
          Jan Riehn added a comment - - edited Hej Martin, maybe the problem is not well demonstrated. this issue can be reconstructed using the wicket 1.5.9 quickstart http://wicket.apache.org/start/quickstart.html . An invalid request like "http://localhost:8080/?%%%" throws the following exception: java.lang.IllegalArgumentException: URLDecoder: Illegal hex characters in escape (%) pattern - For input string: "%%" at org.apache.wicket.request.UrlDecoder.decode(UrlDecoder.java:162) at org.apache.wicket.request.UrlDecoder.decode(UrlDecoder.java:76) at org.apache.wicket.request.Url.decodeParameter(Url.java:601) at org.apache.wicket.request.Url.parseQueryParameter(Url.java:104) at org.apache.wicket.request.Url.parse(Url.java:243) at org.apache.wicket.protocol.http.servlet.ServletWebRequest.getContextRelativeUrl(ServletWebRequest.java:222) at org.apache.wicket.protocol.http.servlet.ServletWebRequest.<init>(ServletWebRequest.java:126) at org.apache.wicket.protocol.http.servlet.ServletWebRequest.<init>(ServletWebRequest.java:83) at org.apache.wicket.protocol.http.WebApplication.newWebRequest(WebApplication.java:413) at org.apache.wicket.protocol.http.WebApplication.createWebRequest(WebApplication.java:458) at org.apache.wicket.protocol.http.WicketFilter.processRequest(WicketFilter.java:183) at org.apache.wicket.protocol.http.WicketFilter.doFilter(WicketFilter.java:244) at org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1326) at org.eclipse.jetty.servlet.ServletHandler.doHandle(ServletHandler.java:479) at org.eclipse.jetty.server.handler.ScopedHandler.handle(ScopedHandler.java:119) at org.eclipse.jetty.security.SecurityHandler.handle(SecurityHandler.java:520) at org.eclipse.jetty.server.session.SessionHandler.doHandle(SessionHandler.java:227) at org.eclipse.jetty.server.handler.ContextHandler.doHandle(ContextHandler.java:940) at org.eclipse.jetty.servlet.ServletHandler.doScope(ServletHandler.java:409) at org.eclipse.jetty.server.session.SessionHandler.doScope(SessionHandler.java:186) at org.eclipse.jetty.server.handler.ContextHandler.doScope(ContextHandler.java:874) at org.eclipse.jetty.server.handler.ScopedHandler.handle(ScopedHandler.java:117) at org.eclipse.jetty.server.handler.HandlerWrapper.handle(HandlerWrapper.java:110) at org.eclipse.jetty.server.Server.handle(Server.java:349) at org.eclipse.jetty.server.HttpConnection.handleRequest(HttpConnection.java:441) at org.eclipse.jetty.server.HttpConnection$RequestHandler.headerComplete(HttpConnection.java:904) at org.eclipse.jetty.http.HttpParser.parseNext(HttpParser.java:565) at org.eclipse.jetty.http.HttpParser.parseAvailable(HttpParser.java:217) at org.eclipse.jetty.server.BlockingHttpConnection.handle(BlockingHttpConnection.java:50) at org.eclipse.jetty.server.bio.SocketConnector$ConnectorEndPoint.run(SocketConnector.java:245) at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:598) at org.eclipse.jetty.util.thread.QueuedThreadPool$3.run(QueuedThreadPool.java:533) at java.lang.Thread.run(Unknown Source) Is there any advice on how to handle such invalid requests?
          Hide
          Martin Grigorov added a comment -

          There is a problem with your test. It expects that both WicketRuntimeException is being thrown and that InternalErrorPage is rendered. Those are mutual exclusive.

          Some details:

          The stack trace is:
          ain@1, prio=5, in group 'main', status: 'RUNNING'
          at org.apache.wicket.request.UrlDecoder.decode(UrlDecoder.java:89)
          at org.apache.wicket.request.UrlDecoder.decode(UrlDecoder.java:76)
          at org.apache.wicket.request.Url.decodeParameter(Url.java:601)
          at org.apache.wicket.request.Url.parseQueryParameter(Url.java:108)
          at org.apache.wicket.request.Url.parse(Url.java:243)
          at org.apache.wicket.util.tester.BaseWicketTester.executeUrl(BaseWicketTester.java:2675)
          at be.dns.wicket.TestHomePage.homepageRendersSuccessfullyPageParamWithPercentSignIncorrectUsage(TestHomePage.java:48)
          .....

          WicketRuntimeException is in wicket-core.jar. In the stack trace the only class from -core is BaseWicketTester. Url and UrlDecoder are in wicket-request which do not see WicketRuntimeException.
          Even re-working BaseWicketTester to handle this it will be a change in the behavior which will affect many applications. Currently they may expect that an exception is being thrown in some erroneous case but with this change there wont be an exception anymore but normal return with lastRenderedPage == InternalErrorPage.
          I agree that InternalErrorPage is more correct but it is a bit late to change this behavior.

          Show
          Martin Grigorov added a comment - There is a problem with your test. It expects that both WicketRuntimeException is being thrown and that InternalErrorPage is rendered. Those are mutual exclusive. Some details: The stack trace is: ain@1, prio=5, in group 'main', status: 'RUNNING' at org.apache.wicket.request.UrlDecoder.decode(UrlDecoder.java:89) at org.apache.wicket.request.UrlDecoder.decode(UrlDecoder.java:76) at org.apache.wicket.request.Url.decodeParameter(Url.java:601) at org.apache.wicket.request.Url.parseQueryParameter(Url.java:108) at org.apache.wicket.request.Url.parse(Url.java:243) at org.apache.wicket.util.tester.BaseWicketTester.executeUrl(BaseWicketTester.java:2675) at be.dns.wicket.TestHomePage.homepageRendersSuccessfullyPageParamWithPercentSignIncorrectUsage(TestHomePage.java:48) ..... WicketRuntimeException is in wicket-core.jar. In the stack trace the only class from -core is BaseWicketTester. Url and UrlDecoder are in wicket-request which do not see WicketRuntimeException. Even re-working BaseWicketTester to handle this it will be a change in the behavior which will affect many applications. Currently they may expect that an exception is being thrown in some erroneous case but with this change there wont be an exception anymore but normal return with lastRenderedPage == InternalErrorPage. I agree that InternalErrorPage is more correct but it is a bit late to change this behavior.
          Johan Heylen made changes -
          Link This issue duplicates WICKET-4493 [ WICKET-4493 ]
          Johan Heylen made changes -
          Field Original Value New Value
          Attachment WICKET-4803-quickstart-testcase-and-example.zip [ 12547779 ]
          Johan Heylen created issue -

            People

            • Assignee:
              Martin Grigorov
              Reporter:
              Johan Heylen
            • Votes:
              2 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development