From e49d9b97a4c20a29696503b9414f86c9d3e0b96c Mon Sep 17 00:00:00 2001 From: Juraj Martinka Date: Thu, 21 Apr 2022 07:20:44 +0200 Subject: [PATCH 1/4] Add ring-jetty-adapter dev dependency to make it possible to run tests. Without it, `lein test` produces the following error when executing test/test_friend/functional.clj: ``` Syntax error (ClassNotFoundException) compiling at (ring/adapter/jetty.clj:1:1). org.eclipse.jetty.util.component.AggregateLifeCycle ``` --- project.clj | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/project.clj b/project.clj index 0fdb869..06c2341 100644 --- a/project.clj +++ b/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"} From f1b9c0add7a384c0ff3c7f387b4cf0d68639f32e Mon Sep 17 00:00:00 2001 From: Juraj Martinka Date: Thu, 21 Apr 2022 07:37:02 +0200 Subject: [PATCH 2/4] Use relative URLs for redirects. This fixes https://github.com/clj-commons/friend/issues/4. 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: https://github.com/cemerick/friend/commit/5b0432385bc86f94ae32f7094a3d096803555e58 It was trying to fix the issue reported in 2013: https://github.com/cemerick/friend/issues/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. https://github.com/cemerick/friend/issues/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. --- src/cemerick/friend.clj | 8 +++----- src/cemerick/friend/util.clj | 7 ------- src/cemerick/friend/workflows.clj | 20 +++++++++----------- test/test_friend/functional.clj | 19 +++++++++---------- test/test_friend/interactive_form.clj | 24 ++++++++++++------------ 5 files changed, 33 insertions(+), 45 deletions(-) diff --git a/src/cemerick/friend.clj b/src/cemerick/friend.clj index b046daa..d3ef368 100644 --- a/src/cemerick/friend.clj +++ b/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,8 +104,7 @@ 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)))) diff --git a/src/cemerick/friend/util.clj b/src/cemerick/friend/util.clj index e578a7b..6573387 100644 --- a/src/cemerick/friend/util.clj +++ b/src/cemerick/friend/util.clj @@ -11,10 +11,3 @@ ; 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 - [^String uri request] - (-> (original-url request) - java.net.URI. - (.resolve uri) - str)) diff --git a/src/cemerick/friend/workflows.clj b/src/cemerick/friend/workflows.clj index 9e324ae..c4de00c 100644 --- a/src/cemerick/friend/workflows.clj +++ b/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 diff --git a/test/test_friend/functional.clj b/test/test_friend/functional.clj index ae4f438..9391024 100644 --- a/test/test_friend/functional.clj +++ b/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))))) @@ -72,14 +72,14 @@ (is (= (url "/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 +100,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 +117,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 +134,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 +150,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"))))))) diff --git a/test/test_friend/interactive_form.clj b/test/test_friend/interactive_form.clj index 5541354..549cfbc 100644 --- a/test/test_friend/interactive_form.clj +++ b/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" From c920dff1cd9a953ec64135516eca8625f7cc7a25 Mon Sep 17 00:00:00 2001 From: Juraj Martinka Date: Thu, 21 Apr 2022 08:38:04 +0200 Subject: [PATCH 3/4] 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". --- CHANGES.md | 6 ++++++ src/cemerick/friend.clj | 2 +- src/cemerick/friend/util.clj | 12 ++++++++++++ test/test_friend/functional.clj | 3 ++- 4 files changed, 21 insertions(+), 2 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 8e90bd3..34e717b 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,5 +1,11 @@ ## [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. + 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 diff --git a/src/cemerick/friend.clj b/src/cemerick/friend.clj index d3ef368..211320c 100644 --- a/src/cemerick/friend.clj +++ b/src/cemerick/friend.clj @@ -106,7 +106,7 @@ Equivalent to (complement current-authentication)."} (#(str (:context request) %)) 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." diff --git a/src/cemerick/friend/util.clj b/src/cemerick/friend/util.clj index 6573387..69c7814 100644 --- a/src/cemerick/friend/util.clj +++ b/src/cemerick/friend/util.clj @@ -11,3 +11,15 @@ ; 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 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)))) diff --git a/test/test_friend/functional.clj b/test/test_friend/functional.clj index 9391024..bc9e420 100644 --- a/test/test_friend/functional.clj +++ b/test/test_friend/functional.clj @@ -69,7 +69,8 @@ ; 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})))) From 817104362020eb9dd4197252797ecf551fbc5cba Mon Sep 17 00:00:00 2001 From: Juraj Martinka Date: Sun, 24 Apr 2022 11:53:46 +0200 Subject: [PATCH 4/4] Review fixes. --- CHANGES.md | 3 +++ src/cemerick/friend/util.clj | 12 ++++++++++++ 2 files changed, 15 insertions(+) diff --git a/CHANGES.md b/CHANGES.md index 34e717b..db43c58 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -3,6 +3,9 @@ ### `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. diff --git a/src/cemerick/friend/util.clj b/src/cemerick/friend/util.clj index 69c7814..9449996 100644 --- a/src/cemerick/friend/util.clj +++ b/src/cemerick/friend/util.clj @@ -12,6 +12,18 @@ ; 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 ^: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)) + (defn relative-url "Similar to `util/original-url` (and thus `ring.util.request/request-url`)