Wicket
  1. Wicket
  2. WICKET-4260

UrlRenderer renders invalid relative URLs if first segment contains colon

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.5.3
    • Fix Version/s: 1.5.6, 6.0.0-beta2
    • Component/s: wicket
    • Labels:
      None

      Description

      Seen on Wicket 1.5.3.

      If a relative url of a link starts with a path segment containing a colon then the whole uri will be regarded as absolute uri, so typically browsers will complain that there is no handle for the protocol foo in foo:bar/dee/per.

      See also the attached quickstart. The start page contains three links, one relative with colon, one absolute and one to a mounted page without colon for comparison.
      The application also has a static switch to add an extended urlrenderer, prepending "./" if needed. This fix is merely a quick shot and there might be better alternatives.

      1. wicket-4260.tar.bz2
        21 kB
        Andreas Köhler
      2. WICKET-4260.path
        1 kB
        Pedro Santos
      3. WICKET-4260.diff
        4 kB
        Sven Meier
      4. ieDotSlashBug.zip
        25 kB
        Chris Colman

        Issue Links

          Activity

          Hide
          Andreas Köhler added a comment -

          Quickstart

          Show
          Andreas Köhler added a comment - Quickstart
          Hide
          Pedro Santos added a comment -

          Hi Andreas, it's weird to have the URL renderer behavior differently because of the first segment content. IMO it would be better if we start to always render relative to the base URLs to have the dot slash prepended.
          I'm sending your patch back (MyUrlRenderer) with this improvement.
          I'm 1+ for this change, if the team agree we can already start to change tests expectations.

          Show
          Pedro Santos added a comment - Hi Andreas, it's weird to have the URL renderer behavior differently because of the first segment content. IMO it would be better if we start to always render relative to the base URLs to have the dot slash prepended. I'm sending your patch back (MyUrlRenderer) with this improvement. I'm 1+ for this change, if the team agree we can already start to change tests expectations.
          Hide
          Igor Vaynberg added a comment -

          be careful to make sure that it doesnt break anything, in the past it has caused us headaches..

          things like

          does ./foo resolve to /foo when on "/" and /bar/foo when on "/bar" in all browsers. etc.

          Show
          Igor Vaynberg added a comment - be careful to make sure that it doesnt break anything, in the past it has caused us headaches.. things like does ./foo resolve to /foo when on "/" and /bar/foo when on "/bar" in all browsers. etc.
          Hide
          Andreas Köhler added a comment -

          Yes, I would prefer not to break everything else But feel free to patch as you like.
          Still, this bug needs fixing, of course, as right now those links do not work at all.

          BTW, should not ./foo resolve to /foo when on /bar?

          Show
          Andreas Köhler added a comment - Yes, I would prefer not to break everything else But feel free to patch as you like. Still, this bug needs fixing, of course, as right now those links do not work at all. BTW, should not ./foo resolve to /foo when on /bar?
          Hide
          Igor Vaynberg added a comment -

          yes, shouldve been /bar/baz

          Show
          Igor Vaynberg added a comment - yes, shouldve been /bar/baz
          Hide
          Pedro Santos added a comment -

          Quoting the RFC 2396 at http://www.ietf.org/rfc/rfc2396.txt
          " Authors should be aware that a path segment which contains a colon
          character cannot be used as the first segment of a relative URI path
          (e.g., "this:that"), because it would be mistaken for a scheme name."

          Sounds like it would make sense if we improve the UrlRenderer#shouldRenderAsFull method to return true if the first segment has a colon. Fix this ticket but may break some apps because users may have code expecting relative URLs.

          Show
          Pedro Santos added a comment - Quoting the RFC 2396 at http://www.ietf.org/rfc/rfc2396.txt " Authors should be aware that a path segment which contains a colon character cannot be used as the first segment of a relative URI path (e.g., "this:that"), because it would be mistaken for a scheme name." Sounds like it would make sense if we improve the UrlRenderer#shouldRenderAsFull method to return true if the first segment has a colon. Fix this ticket but may break some apps because users may have code expecting relative URLs.
          Hide
          Sven Meier added a comment -

          Alternative patch.

          Show
          Sven Meier added a comment - Alternative patch.
          Hide
          Martin Grigorov added a comment -

          The patch looks quite simple. I'm +1 to commit it in 1.5.x/master once 1.5.5 is out so we can have enough time to test it in different browsers before 1.5.6 is out.

          Show
          Martin Grigorov added a comment - The patch looks quite simple. I'm +1 to commit it in 1.5.x/master once 1.5.5 is out so we can have enough time to test it in different browsers before 1.5.6 is out.
          Hide
          Sven Meier added a comment -

          All relative urls are now explicitely relative, i.e. start with a dot or dot-dot.

          Tested with IE, Firefox and Chrome and everything worked as expected (e.g. "./foo" when on "/").

          Show
          Sven Meier added a comment - All relative urls are now explicitely relative, i.e. start with a dot or dot-dot. Tested with IE, Firefox and Chrome and everything worked as expected (e.g. "./foo" when on "/").
          Hide
          Sven Meier added a comment -

          Just tested with Safari, works too.

          Show
          Sven Meier added a comment - Just tested with Safari, works too.
          Hide
          Chris Colman added a comment -

          Quickstart that shows IE problem with extra ./ inserted before path when /landing page is redirected to /logon page if not authenticated.

          Happens on IE 8 (and possibly other IE versions)
          Does not happen on FireFox nor Chrome.

          Does not happen when running on Jetty

          Happens on Tomcat BUT only when app is installed as a virtual host - it does not happen when the app is installed as a servlet in the root context/host.

          Using the quickstart create a .war using mvn war:war

          Create an entry in your hosts file:

          127.0.0.1 thingy.com.au

          Rename the .war to ROOT.war and place under:
          <tomcat>/virtualhosts/thingy

          Add new host element to server.xml:

          <Host name="thingy.com.au" debug="0" appBase="virtualhosts/thingy" unpackWARs="true" autoDeploy="true" xmlValidation="false" xmlNamespaceAware="false">

          <Valve className="org.apache.catalina.valves.FastCommonAccessLogValve"
          directory="logs" prefix="thing_access_log." suffix=".txt"
          pattern="common" resolveHosts="false"/>

          <Context path="/manager" debug="5" docBase="/servers/tomcat/webapps/manager" privileged="true"/>

          </Host>

          To reproduce:

          Point your IE browser to:

          http://thingy.com.au/landing

          this will attempt a redirect to thingy.com.au/logon
          which should display the error.

          Hit refresh and the logon page will display. Click on Logon link and problem should display again. Hit refresh and landing page will now display properly.

          Show
          Chris Colman added a comment - Quickstart that shows IE problem with extra ./ inserted before path when /landing page is redirected to /logon page if not authenticated. Happens on IE 8 (and possibly other IE versions) Does not happen on FireFox nor Chrome. Does not happen when running on Jetty Happens on Tomcat BUT only when app is installed as a virtual host - it does not happen when the app is installed as a servlet in the root context/host. Using the quickstart create a .war using mvn war:war Create an entry in your hosts file: 127.0.0.1 thingy.com.au Rename the .war to ROOT.war and place under: <tomcat>/virtualhosts/thingy Add new host element to server.xml: <Host name="thingy.com.au" debug="0" appBase="virtualhosts/thingy" unpackWARs="true" autoDeploy="true" xmlValidation="false" xmlNamespaceAware="false"> <Valve className="org.apache.catalina.valves.FastCommonAccessLogValve" directory="logs" prefix="thing_access_log." suffix=".txt" pattern="common" resolveHosts="false"/> <Context path="/manager" debug="5" docBase="/servers/tomcat/webapps/manager" privileged="true"/> </Host> To reproduce: Point your IE browser to: http://thingy.com.au/landing this will attempt a redirect to thingy.com.au/logon which should display the error. Hit refresh and the logon page will display. Click on Logon link and problem should display again. Hit refresh and landing page will now display properly.
          Hide
          Sven Meier added a comment -

          need to invesigate IE8 issue

          Show
          Sven Meier added a comment - need to invesigate IE8 issue
          Hide
          Martin Grigorov added a comment -

          This change also exposed a bug in Jetty 7.6.2 and earlier: https://bugs.eclipse.org/bugs/show_bug.cgi?id=374475
          Basically the redirect will fail if it contains non-ASCII chars.
          The fix will be in Jetty 7.6.3.

          Show
          Martin Grigorov added a comment - This change also exposed a bug in Jetty 7.6.2 and earlier: https://bugs.eclipse.org/bugs/show_bug.cgi?id=374475 Basically the redirect will fail if it contains non-ASCII chars. The fix will be in Jetty 7.6.3.
          Hide
          Chris Colman added a comment -

          I'm not sure how many web apps have a colon in the paths but I know (unfortunately) that a lot of corporates are locked into IE and this fix breaks IE under the conditions I outlined above which means some of our significant corporate customers now have a broken app - they need to hit Refresh after logging in to get 'over' the error page.

          Is is possible to make this fix a configurable one so that those sites that don't have colons in their path can disable the addition of the './' prefix? Or better still, make that the default and sites that want to use a colon in the paths need to explicitly turn it on.

          Show
          Chris Colman added a comment - I'm not sure how many web apps have a colon in the paths but I know (unfortunately) that a lot of corporates are locked into IE and this fix breaks IE under the conditions I outlined above which means some of our significant corporate customers now have a broken app - they need to hit Refresh after logging in to get 'over' the error page. Is is possible to make this fix a configurable one so that those sites that don't have colons in their path can disable the addition of the './' prefix? Or better still, make that the default and sites that want to use a colon in the paths need to explicitly turn it on.
          Hide
          Sven Meier added a comment -

          I don't have an IE at hand at the moment, but I tested the ieDotSlashBug application on Tomcat with root context (Root.war).

          A redirect to "./logon" was handled differently on Jetty and Tomcat:

          • Jetty redirects to "http://localhost:8080/logon", as org.eclipse.jetty.server.Response#sendRedirect() uses a canonical path (i.e. all "." and ".." factored out)
          • Tomcat redirects to "http://localhost:8080/./logon", obviously leaving the dot in Wicket's relative url as is.

          For reference an excerpt of HttpServletResponse#sendRedirect()'s javadoc:

          "... This method can accept relative URLs; the servlet container must convert the relative URL to an absolute URL before sending the response to the client..."

          Show
          Sven Meier added a comment - I don't have an IE at hand at the moment, but I tested the ieDotSlashBug application on Tomcat with root context (Root.war). A redirect to "./logon" was handled differently on Jetty and Tomcat: Jetty redirects to "http://localhost:8080/logon", as org.eclipse.jetty.server.Response#sendRedirect() uses a canonical path (i.e. all "." and ".." factored out) Tomcat redirects to "http://localhost:8080/./logon", obviously leaving the dot in Wicket's relative url as is. For reference an excerpt of HttpServletResponse#sendRedirect()'s javadoc: "... This method can accept relative URLs; the servlet container must convert the relative URL to an absolute URL before sending the response to the client..."
          Hide
          Sven Meier added a comment -

          >Is is possible to make this fix a configurable one

          I'd rather not make this configurable.

          I'll investigate whether this is a known issue for Tomcat. If there's no fix, we can remove a leading "./" from the url before passing it to HttpServletResponse#sendRedirect().

          Show
          Sven Meier added a comment - >Is is possible to make this fix a configurable one I'd rather not make this configurable. I'll investigate whether this is a known issue for Tomcat. If there's no fix, we can remove a leading "./" from the url before passing it to HttpServletResponse#sendRedirect().
          Hide
          Chris Colman added a comment - - edited

          Some observations:

          Here's the requests that the server sees - both get redirects to the logon page which is correct because the user is not yet authenticated

          IE browser requesting /landing

          127.0.0.1 - - [05/Apr/2012:01:16:13 +1000] "GET /landing HTTP/1.1" 302 -
          127.0.0.1 - - [05/Apr/2012:01:16:13 +1000] "GET /./logon?3 HTTP/1.1" 404 976

          FireFox browser requesting /landing

          127.0.0.1 - - [05/Apr/2012:01:18:12 +1000] "GET /landing HTTP/1.1" 302 -
          127.0.0.1 - - [05/Apr/2012:01:18:12 +1000] "GET /logon?2 HTTP/1.1" 200 1619

          This makes me believe that it may not be a Tomcat bug because it works correctly for FireFox and Chrome. It may be that these browsers can handle the ./ and automatically remove it possibly?

          Show
          Chris Colman added a comment - - edited Some observations: Here's the requests that the server sees - both get redirects to the logon page which is correct because the user is not yet authenticated IE browser requesting /landing 127.0.0.1 - - [05/Apr/2012:01:16:13 +1000] "GET /landing HTTP/1.1" 302 - 127.0.0.1 - - [05/Apr/2012:01:16:13 +1000] "GET /./logon?3 HTTP/1.1" 404 976 FireFox browser requesting /landing 127.0.0.1 - - [05/Apr/2012:01:18:12 +1000] "GET /landing HTTP/1.1" 302 - 127.0.0.1 - - [05/Apr/2012:01:18:12 +1000] "GET /logon?2 HTTP/1.1" 200 1619 This makes me believe that it may not be a Tomcat bug because it works correctly for FireFox and Chrome. It may be that these browsers can handle the ./ and automatically remove it possibly?
          Hide
          Sven Meier added a comment -

          Could you test whether it works when you mount LandingPage on "nested/landing"?

          It seems Tomcat happily redirects to "http://localhost:8080/../logon" - I'd be interested whether IE can handle that.

          Show
          Sven Meier added a comment - Could you test whether it works when you mount LandingPage on "nested/landing"? It seems Tomcat happily redirects to "http://localhost:8080/../logon" - I'd be interested whether IE can handle that.
          Hide
          Chris Colman added a comment -

          I've just tested it and found IE works fine with "nested/landing"

          Show
          Chris Colman added a comment - I've just tested it and found IE works fine with "nested/landing"
          Hide
          Chris Colman added a comment -

          Seems weird that IE handles "../" but not "./" though it is IE afterall so 'weird' becomes the expectation

          Do you have any ideas on the best approach for fixing this? We have temporarily reverted to 1.5.5 to get our site working for IE customers again but obviously some day soon we'll need to be able to upgrade to 1.5.6 or a snapshot.

          Show
          Chris Colman added a comment - Seems weird that IE handles "../" but not "./" though it is IE afterall so 'weird' becomes the expectation Do you have any ideas on the best approach for fixing this? We have temporarily reverted to 1.5.5 to get our site working for IE customers again but obviously some day soon we'll need to be able to upgrade to 1.5.6 or a snapshot.
          Hide
          Sven Meier added a comment -

          >IE handles "../" but not "./" ... Do you have any ideas on the best approach for fixing this?

          Strange, yes.

          I'd say we keep this ticket as it is now (i.e. all relative urls prefixed with "./" or "../").
          But we'll let ServletWebResponse remove a leading "./" before passing the url to HttpServletRequest#sendRedirect().
          This does no harm and as you stated is essential as a workaround for the Tomcat/IE combination.

          Show
          Sven Meier added a comment - >IE handles "../" but not "./" ... Do you have any ideas on the best approach for fixing this? Strange, yes. I'd say we keep this ticket as it is now (i.e. all relative urls prefixed with "./" or "../"). But we'll let ServletWebResponse remove a leading "./" before passing the url to HttpServletRequest#sendRedirect(). This does no harm and as you stated is essential as a workaround for the Tomcat/IE combination.
          Hide
          Chris Colman added a comment -

          Sounds like a good solution. If you add a patch to 1.5.x in the git repository I can pull it and test it on both my quickstart and the real app whenever you're ready.

          Show
          Chris Colman added a comment - Sounds like a good solution. If you add a patch to 1.5.x in the git repository I can pull it and test it on both my quickstart and the real app whenever you're ready.
          Hide
          Sven Meier added a comment -

          >I can pull it and test it

          Please do so, thanks.

          Show
          Sven Meier added a comment - >I can pull it and test it Please do so, thanks.
          Hide
          Chris Colman added a comment -

          Yep, works fine now. Thanks!

          Show
          Chris Colman added a comment - Yep, works fine now. Thanks!
          Hide
          Sven Meier added a comment -

          Now working with sendRedirect() for Tomcat+IE too.

          Show
          Sven Meier added a comment - Now working with sendRedirect() for Tomcat+IE too.
          Hide
          Martin Grigorov added a comment -

          Filed a bug at Tomcat issue tracker for this problem: https://issues.apache.org/bugzilla/show_bug.cgi?id=53062

          Show
          Martin Grigorov added a comment - Filed a bug at Tomcat issue tracker for this problem: https://issues.apache.org/bugzilla/show_bug.cgi?id=53062

            People

            • Assignee:
              Sven Meier
              Reporter:
              Andreas Köhler
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development