Details

    • Type: Feature Request
    • Status: Closed (View Workflow)
    • Priority: Major
    • Resolution: Done
    • Affects Version/s: None
    • Fix Version/s: 0.8.0
    • Labels:
      None

      Description

      will attach project to reproduce - this is a simpler handler that echoes the URI

      (defn my-handler [request]
      {:status 200
      :headers

      {"Content-Type" "text/html"}

      :body (str "::" (:uri request) "::")})

      In immutant: [lein immutant run]

      $ curl http://localhost:8080/%5B # correct output
      ::/%5B::
      $ curl http://localhost:8080/%5C # no output - HTTP/1.1 400 Bad Request

      Same app with ring directly: [lein ring server-headless]
      $ curl http://localhost:8080/%5B # works
      ::/%5B::
      $ curl http://localhost:8080/%5C # also workds
      ::/%5C::

        Gliffy Diagrams

        1. escaper.tgz
          1 kB
          Norman Richards

          Activity

          Hide
          norman Norman Richards added a comment -

          project with immutant and ring config around the handler

          Show
          norman Norman Richards added a comment - project with immutant and ring config around the handler
          Hide
          tcrawley Toby Crawley added a comment -

          I dug into this a bit, and it is caused by the way catalina (tomcat/jbossweb) handles %5C. Without org.apache.catalina.connector.CoyoteAdapter.ALLOW_BACKSLASH set, it rejects any request containing a %5C in the uri before even looking for a servlet to respond to the request. if ALLOW_BACKSLASH is set, any %5C's are converted to /'s, which means you won't get the data you expect. The only way I can see to cleanly get %5C's through is to make them part of the query string: http//server:8080/foo?%5C

          Let's chat some more about this on irc tomorrow if you are able.

          Show
          tcrawley Toby Crawley added a comment - I dug into this a bit, and it is caused by the way catalina (tomcat/jbossweb) handles %5C. Without org.apache.catalina.connector.CoyoteAdapter.ALLOW_BACKSLASH set, it rejects any request containing a %5C in the uri before even looking for a servlet to respond to the request. if ALLOW_BACKSLASH is set, any %5C's are converted to /'s, which means you won't get the data you expect. The only way I can see to cleanly get %5C's through is to make them part of the query string: http//server:8080/foo?%5C Let's chat some more about this on irc tomorrow if you are able.
          Hide
          norman Norman Richards added a comment -

          Pragmatically, I'm guessing that filing a bug with tomcat is not likely going to be profitable? (the code is very clearly wrong by design, to deal with a flaw in another part of the system, which is undoubtably even harder to fix)

          Not that I have any strong desire to do this, but I don't suppose it's possible to easily replace CoyoteAdapter with subclass that works?

          Show
          norman Norman Richards added a comment - Pragmatically, I'm guessing that filing a bug with tomcat is not likely going to be profitable? (the code is very clearly wrong by design, to deal with a flaw in another part of the system, which is undoubtably even harder to fix) Not that I have any strong desire to do this, but I don't suppose it's possible to easily replace CoyoteAdapter with subclass that works?
          Hide
          norman Norman Richards added a comment -

          Actually, ALLOW_BACKSLASH might be workable. It seems the correct value does at least come through in :uri. It's just the :path-info which gets hacked up by this bug:

          23:20:49,528 INFO [stdout] (http-localhost/127.0.0.1:8080-1) {:ssl-client-cert nil, :remote-addr 127.0.0.1, :scheme :http, :context , :request-method :get, :query-string nil, :content-type nil, :path-info /back/slash, :uri /back%5Cslash, :server-name localhost, :headers

          {accept */*, host localhost:8080, user-agent curl/7.23.1 (x86_64-apple-darwin11.2.0) libcurl/7.23.1 OpenSSL/1.0.1c zlib/1.2.7 libidn/1.22}

          , :content-length nil, :server-port 8080, :character-encoding nil, :body #<CoyoteInputStream org.apache.catalina.connector.CoyoteInputStream@6eece807>}

          Show
          norman Norman Richards added a comment - Actually, ALLOW_BACKSLASH might be workable. It seems the correct value does at least come through in :uri. It's just the :path-info which gets hacked up by this bug: 23:20:49,528 INFO [stdout] (http-localhost/127.0.0.1:8080-1) {:ssl-client-cert nil, :remote-addr 127.0.0.1, :scheme :http, :context , :request-method :get, :query-string nil, :content-type nil, :path-info /back/slash, :uri /back%5Cslash, :server-name localhost, :headers {accept */*, host localhost:8080, user-agent curl/7.23.1 (x86_64-apple-darwin11.2.0) libcurl/7.23.1 OpenSSL/1.0.1c zlib/1.2.7 libidn/1.22} , :content-length nil, :server-port 8080, :character-encoding nil, :body #<CoyoteInputStream org.apache.catalina.connector.CoyoteInputStream@6eece807>}
          Hide
          norman Norman Richards added a comment -

          Last bit of feedback here. Given that ALLOW_BACKSLASH is set, if I add the following middleware to remove the :path-info, all is well with URL routing.

          (defn remove-path-info-handler [handler]
          (fn [request]
          (handler (dissoc request :path-info))))

          I don't know the implications of this, but does seem to give consistent behavior.

          Show
          norman Norman Richards added a comment - Last bit of feedback here. Given that ALLOW_BACKSLASH is set, if I add the following middleware to remove the :path-info, all is well with URL routing. (defn remove-path-info-handler [handler] (fn [request] (handler (dissoc request :path-info)))) I don't know the implications of this, but does seem to give consistent behavior.
          Hide
          tcrawley Toby Crawley added a comment - - edited

          I agree that this is a bug in catalina. It looks like this exact same issue came up on TorqueBox recently (TORQUE-955), and the fix was to pass a different, unescaped path-info to the handler. I'd like to implement the same thing in Immutant so the path-info will at least be correct. I'd like to think about & discuss the implications of enabling ALLOW_BACKSLASH by default.

          Show
          tcrawley Toby Crawley added a comment - - edited I agree that this is a bug in catalina. It looks like this exact same issue came up on TorqueBox recently ( TORQUE-955 ), and the fix was to pass a different, unescaped path-info to the handler. I'd like to implement the same thing in Immutant so the path-info will at least be correct. I'd like to think about & discuss the implications of enabling ALLOW_BACKSLASH by default.
          Hide
          tcrawley Toby Crawley added a comment -
          Show
          tcrawley Toby Crawley added a comment - I've implemented the unescaped path-info: https://github.com/immutant/immutant/commit/e53573dffa738eb817b8eb4d681e9f5907597c54
          Hide
          norman Norman Richards added a comment -

          Confirmed - the path-info matches the uri in the cases I tested, so our app shows the same behavior when run in immutant and run standalone. (with respect to this URL issue)

          Show
          norman Norman Richards added a comment - Confirmed - the path-info matches the uri in the cases I tested, so our app shows the same behavior when run in immutant and run standalone. (with respect to this URL issue)
          Hide
          tcrawley Toby Crawley added a comment -

          I've turned on ALLOW_BACKSLASH by default (https://github.com/immutant/immutant/commit/218b85bd4af809e22eb0d44136b03799459208fb). Can you confirm that the latest incremental works for you? If so, I'll resolve this issue.

          Show
          tcrawley Toby Crawley added a comment - I've turned on ALLOW_BACKSLASH by default ( https://github.com/immutant/immutant/commit/218b85bd4af809e22eb0d44136b03799459208fb ). Can you confirm that the latest incremental works for you? If so, I'll resolve this issue.

            People

            • Assignee:
              tcrawley Toby Crawley
              Reporter:
              norman Norman Richards
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development