Details

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

      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::

      1. escaper.tgz
        1 kB
        Norman Richards

        Activity

        Hide
        Norman Richards
        added a comment -

        project with immutant and ring config around the handler

        Show
        Norman Richards
        added a comment - project with immutant and ring config around the handler
        Hide
        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
        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 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 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 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 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 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 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
        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
        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
        Toby Crawley
        added a comment -
        Show
        Toby Crawley
        added a comment - I've implemented the unescaped path-info: https://github.com/immutant/immutant/commit/e53573dffa738eb817b8eb4d681e9f5907597c54
        Hide
        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 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
        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
        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:
            Toby Crawley
            Reporter:
            Norman Richards
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: