[Fix #4] Stop enforcing absolute URLs for redirects. (#5)

For long, the HTTP spec has allowed relative URLs in the Location header,
see https://datatracker.ietf.org/doc/html/rfc7231#section-7.1.2.

Absolute URLs have been introduced here: cemerick@5b04323
It was trying to fix the issue reported in 2013: cemerick#42
where they claimed that friend doesn't follow the HTTP spec.

However, this lead to all sorts of problems with the clojure app
running behind an SSL/TLS proxy,
e.g. cemerick#84.

To sum up: Original friend implementation got it right by using
relative URLs for redirects but it wasn't, at the time,
strictly following the HTTP spec.
However, the HTTP spec has since been updated and there's no more reason
to use absolute URLs - they are brittle and break apps.

Use relative URL in default-unauthenticated-handler.

This is to avoid redirecting to HTTP when the user request was in fact to an HTTPS endpoint.
Such a situation happens when you run an SSL proxy in front of your plain HTTP app server.
In that case, `ring.util.request/request-url` returns _almost_ a proper endpoint,
but uses "http" not "https".

Author: jumarko

CHANGES.md

@@ -1,5 +1,14 @@
 ## [Friend](http://github.com/cemerick/friend) changelog
 
+### `0.3.x`
+
+* HTTP redirect responses now don't enforce absolute URLs - this is a revert of the change made in 0.1.2.
+  Note that if you are using ring-defaults, you need to 
+  [set `:absolute-redirects` to `false`](https://github.com/ring-clojure/ring-defaults#customizing).
+  (a new [issue submitted](https://github.com/ring-clojure/ring-defaults/issues/39) for ring-defaults)
+  See https://github.com/clj-commons/friend/issues/4.
+
+
 ### `0.3.0`
 
 * The "classic" OpenId (i.e. OpenId 1.1/2.0) workflow provided by `cemerick.friend.openid` has been

project.clj

@@ -21,7 +21,9 @@
                                   [compojure "1.1.5"]
                                   [ring "1.2.0"]
                                   [robert/hooke "1.3.0"]
-                                  [clj-http "0.3.6"]]}
+                                  [clj-http "0.3.6"]
+                                  ;; test/.../functional.clj uses the jetty adapter
+                                  [ring/ring-jetty-adapter "1.9.5"]]}
              :sanity-check {:aot :all
                             :warn-on-reflection true
                             :compile-path "target/sanity-check-aot"}

src/cemerick/friend.clj

@@ -1,8 +1,7 @@
 (ns cemerick.friend
   (:require [cemerick.friend.util :as util]
-            [clojure.set :as set])
-  (:use (ring.util [response :as response :only (redirect)])
-        [slingshot.slingshot :only (throw+ try+)])
+            [ring.util.response :as response]
+            [slingshot.slingshot :refer [throw+ try+]])
   (:refer-clojure :exclude (identity)))
 
 (defn merge-authentication
@@ -105,10 +104,9 @@ Equivalent to (complement current-authentication)."}
     ::auth-config
     :login-uri
     (#(str (:context request) %))
-    (util/resolve-absolute-uri request)
-    ring.util.response/redirect
+    response/redirect
     (assoc :session (:session request))
-    (assoc-in [:session ::unauthorized-uri] (util/original-url request))))
+    (assoc-in [:session ::unauthorized-uri] (util/relative-url request))))
 
 (defn authenticate-response
   "Adds to the response's :session for responses with a :friend/ensure-identity-request key."

src/cemerick/friend/util.clj

@@ -12,9 +12,26 @@
 ; this was never _really_ part of the API, was implemented (badly) before req/request-url was available
 (def ^:deprecated original-url req/request-url)
 
-(defn resolve-absolute-uri
+(defn ^:deprecated resolve-absolute-uri
+  "DEPRECATED. Use `relative-url`.
+
+  This function is only kept for backward compatibility.
+  It was meant to conform to an old HTTP spec that required absolute URLs for redirects
+  but that's no longer needed."
   [^String uri request]
   (-> (original-url request)
-    java.net.URI.
-    (.resolve uri)
-    str))
+      java.net.URI.
+      (.resolve uri)
+      str))
+
+
+(defn relative-url
+  "Similar to `util/original-url` (and thus `ring.util.request/request-url`)
+  except that it only uses `:uri` and `:query-string` parts of the request
+  to build the URL and ignores `:scheme` and the 'host' header.
+  This is to avoid issues with redirects using absolute URLs when the app is running behind an SSL proxy."
+  [request]
+  (str
+   (:uri request)
+   (when-let [query (:query-string request)]
+     (str "?" query))))

src/cemerick/friend/workflows.clj

@@ -1,10 +1,10 @@
 (ns cemerick.friend.workflows
   (:require [cemerick.friend :as friend]
-            [cemerick.friend.util :as util]
-            [ring.util.request :as req])
-  (:use [clojure.string :only (trim)]
-        [cemerick.friend.util :only (gets)])
-  (:import org.apache.commons.codec.binary.Base64))
+            [cemerick.friend.util :as util :refer [gets]]
+            [ring.util.request :as req]
+            [ring.util.response :as response])
+  (:import java.net.URLEncoder
+           org.apache.commons.codec.binary.Base64))
 
 (defn http-basic-deny
   [realm request]
@@ -68,14 +68,12 @@
 
 (defn interactive-login-redirect
   [{:keys [form-params params] :as request}]
-  (ring.util.response/redirect
+  (response/redirect
     (let [param (str "&login_failed=Y&username="
-                     (java.net.URLEncoder/encode (username form-params params)))
+                     (URLEncoder/encode (username form-params params)))
           ^String login-uri (-> request ::friend/auth-config :login-uri (#(str (:context request) %)))]
-      (util/resolve-absolute-uri
-        (str (if (.contains login-uri "?") login-uri (str login-uri "?"))
-          param)
-        request))))
+      (str (if (.contains login-uri "?") login-uri (str login-uri "?"))
+           param))))
 
 (defn interactive-form
   [& {:keys [login-uri credential-fn login-failure-handler redirect-on-auth?] :as form-config

test/test_friend/functional.clj

@@ -34,7 +34,7 @@
           :let [resp (http/get url)]]
     (is (http/success? resp))
     (is (= (page-bodies uri) (:body resp))))
-  
+
   (let [api-resp (http/get (url "/free-api") {:as :json})]
     (is (http/success? api-resp))
     (is (= {:data 99} (:body api-resp)))))
@@ -69,17 +69,18 @@
       ; clj-http *should* redirect us, but isn't yet; working on it: 
       ; https://github.com/dakrone/clj-http/issues/57
       (is (http/redirect? resp))
-      (is (= (url "/user/account?query-string=test") (-> resp :headers (get "location")))))
+      (is (= "/user/account?query-string=test"
+             (-> resp :headers (get "location")))))
     (check-user-role-access)
     (is (= {:roles ["test-friend.mock-app/user"]} (:body (http/get (url "/echo-roles") {:as :json}))))
-    
+
     ; deny on admin role
     (try+
       (http/get (url "/admin"))
       (assert false) ; should never get here
       (catch [:status 403] _
         (is true)))
-    
+
     (testing "logout blocks access to privileged routes"
       (is (= (page-bodies "/") (:body (http/get (url "/logout")))))
       (is (= (page-bodies "/login") (:body (http/get (url "/user/account"))))))))
@@ -100,13 +101,12 @@
         (is (= "auth-data" (post-session-data "auth-data")))
         (is (= "auth-data" (get-session-data)))
         (check-user-role-access)
-        
+
         (http/get (url "/logout"))
         (let [should-be-login-redirect (http/get (url "/user/account")
                                                  {:follow-redirects false})]
           (is (= 302 (:status should-be-login-redirect)))
-          (is (re-matches #"http://localhost:\d+/login"
-                (-> should-be-login-redirect :headers (get "location")))))
+          (is (=  "/login" (-> should-be-login-redirect :headers (get "location")))))
         ; TODO should logout blow away the session completely?
         (is (= "auth-data" (get-session-data)))))))
 
@@ -118,7 +118,7 @@
       (assert false) ; should never get here
       (catch [:status 403] resp
         (is (= "Sorry, you do not have access to this resource." (:body resp)))))
-    
+
     (http/post (url "/login") {:form-params {:username "root" :password "admin_password"}})
     (is (= (page-bodies "/hook-admin") (:body (http/get (url "/hook-admin")))))))
 
@@ -135,7 +135,7 @@
 (deftest admin-login
   (binding [clj-http.core/*cookie-store* (clj-http.cookies/cookie-store)]
     (is (= (page-bodies "/login") (:body (http/get (url "/admin")))))
-    
+
     (http/post (url "/login") {:form-params {:username "root" :password "admin_password"}})
     (is (= (page-bodies "/admin") (:body (http/get (url "/admin")))))
     (check-user-role-access)
@@ -151,13 +151,13 @@
   (binding [clj-http.core/*cookie-store* (clj-http.cookies/cookie-store)]
     (is (= (page-bodies "/login") (:body (http/get (url "/admin")))))
     (http/post (url "/login") {:form-params {:username "root" :password "admin_password"}})
-    
+
     (try+
       (http/get (url "/wat"))
       (assert false)
       (catch [:status 404] e))
     (is (= (page-bodies "/admin") (:body (http/get (url "/admin")))))
-    
+
     (is (= (page-bodies "/") (:body (http/get (url "/logout")))))
     (is (= (page-bodies "/login") (:body (http/get (url "/admin")))))))
 

test/test_friend/interactive_form.clj

@@ -1,8 +1,8 @@
 (ns test-friend.interactive-form
-  (:require [cemerick.friend :as friend])
-  (:use clojure.test
-        ring.mock.request
-        [cemerick.friend.workflows :only (interactive-form)]))
+  (:require [cemerick.friend :as friend]
+            [clojure.test :refer :all]
+            [ring.mock.request :as mock]
+            [cemerick.friend.workflows :refer (interactive-form)]))
 
 (deftest form-workflow
   (let [got-creds (atom false)
@@ -14,28 +14,28 @@
                                                         (when (and (= "open sesame" password)
                                                                    (= "Aladdin" username))
                                                           {:identity username})))]
-    (is (nil? (form-handler (request :get login-uri))))
+    (is (nil? (form-handler (mock/request :get login-uri))))
 
     (is (= {:status 302
-            :headers {"Location" "http://localhost/my_login?&login_failed=Y&username="}
+            :headers {"Location" "/my_login?&login_failed=Y&username="}
             :body ""}
-           (form-handler (request :post login-uri))))
+           (form-handler (mock/request :post login-uri))))
 
     (is (= {:status 302
-            :headers {"Location" "http://localhost/my_login?&login_failed=Y&username=foo"}
+            :headers {"Location" "/my_login?&login_failed=Y&username=foo"}
             :body ""}
-           (form-handler (assoc (request :post login-uri)
+           (form-handler (assoc (mock/request :post login-uri)
                                 :params {:username "foo"}
                                 :form-params {"username" "foo"}))))
 
     (is (= {:status 302
-            :headers {"Location" "http://localhost/my_login?&login_failed=Y&username=foobar"}
+            :headers {"Location" "/my_login?&login_failed=Y&username=foobar"}
             :body ""}
-           (form-handler (assoc (request :post login-uri)
+           (form-handler (assoc (mock/request :post login-uri)
                                 :params {:username "foo"}
                                 :form-params {"username" "foobar"}))))
 
-    (let [auth (form-handler (assoc (request :post login-uri)
+    (let [auth (form-handler (assoc (mock/request :post login-uri)
                                     :params {:username "foo"
                                              :password "open sesame"}
                                     :form-params {"username" "Aladdin"