search

xeqi / peridot

a basic api for interacting with ring apps
145 stars 20 forks source link

follow-redirect doesn't handle relative uris properly #17

Closed philandstuff closed 10 years ago

philandstuff commented 10 years ago

When following a redirect with a relative uri, peridot will default the :server-name to "localhost" rather than maintaining the server-name from the previous request. Here's an example failing test that I expect to pass:

(ns demo-peridot.core-test
  (:require [clojure.test :refer :all]
            [peridot.core :refer :all]
            [ring.util.response :as resp]))

(defn main-handler [req]
  (condp = (:uri req)
    "/" (resp/redirect "/foo")
    "/foo" (resp/response "hooray!")))

(defn vhosting-handler [req]
  (condp = (:server-name req)
    "foo.com" (main-handler req)
    (resp/not-found "not found")))

(deftest a-test
  (testing "Should keep same server-name when following redirect"
    (is (= 200
           (-> (session vhosting-handler)
               (request "http://foo.com/")
               (follow-redirect)
               :response
               :status)))))

Strict HTTP 1.1 requires an absolute URL in the Location: header, but most browsers will accept a relative URL. I'm not sure what peridot should do -- should it emulate browsers or should it reject relative URLs entirely? Either way, defaulting to localhost doesn't feel like the right behaviour.

glenjamin commented 10 years ago

Copying the browser behaviour seems like the right thing to do here

On 9 May 2014, at 08:22, Philip Potter notifications@github.com wrote:

When following a redirect with a relative uri, peridot will default the :server-name to "localhost" rather than maintaining the server-name from the previous request. Here's an example failing test that I expect to pass:

(ns demo-peridot.core-test (:require [clojure.test :refer :all] [peridot.core :refer :all] [ring.util.response :as resp]))

(defn main-handler [req](condp = %28:uri req%29) "/foo" (resp/response "hooray!")))

(defn vhosting-handler [req](condp = %28:server-name req%29 "foo.com" %28main-handler req) (resp/not-found "not found")))

(deftest a-test (testing "Should keep same server-name when following redirect" (is (= 200 (-> (session vhosting-handler) (request "http://foo.com/") (follow-redirect) :response :status))))) Strict HTTP 1.1 requires an absolute URL in the Location: header, but most browsers will accept a relative URL. I'm not sure what peridot should do -- should it emulate browsers or should it reject relative URLs entirely? Either way, defaulting to localhost doesn't feel like the right behaviour.

— Reply to this email directly or view it on GitHub.

glenjamin commented 10 years ago

Should be fixed in 8c39b30, I can't publish to clojars, so would need @xeqi to cut a new release with the fix.

philandstuff commented 10 years ago

@glenjamin wow, thanks very much! Looks like it should fix my original problem, will test it when I get back to work on Monday.

philandstuff commented 10 years ago

Have tested 0.3.0 locally and it works great. Thanks for the fix!