From 71a777b4fa539440a5e90684039232fd262f827d Mon Sep 17 00:00:00 2001 From: Joel Kaasinen Date: Fri, 23 Jan 2026 09:54:26 +0200 Subject: [PATCH] fix: redirect-trailing-slash-handler won't make external redirects A redirect header like Location: //malicious.com/foo Would result in a redirect to https://malicious.com/foo . We never want Locations like that out of redirect-trailing-slash-handler. Fixes #337 --- modules/reitit-ring/src/reitit/ring.cljc | 3 ++- test/cljc/reitit/ring_test.cljc | 26 +++++++++++++++++++++++- 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/modules/reitit-ring/src/reitit/ring.cljc b/modules/reitit-ring/src/reitit/ring.cljc index 4f56a8c8..54c70a80 100644 --- a/modules/reitit-ring/src/reitit/ring.cljc +++ b/modules/reitit-ring/src/reitit/ring.cljc @@ -173,7 +173,8 @@ (letfn [(maybe-redirect [{:keys [query-string] :as request} path] (if (and (seq path) (r/match-by-path (::r/router request) path)) {:status (if (= (:request-method request) :get) 301 308) - :headers {"Location" (if query-string (str path "?" query-string) path)} + :headers {"Location" (let [path (str/replace-first path #"^/+" "/")] ; Locations starting with // redirect to another hostname. Avoid these due to security implications. + (if query-string (str path "?" query-string) path))} :body ""})) (redirect-handler [request] (let [uri (:uri request)] diff --git a/test/cljc/reitit/ring_test.cljc b/test/cljc/reitit/ring_test.cljc index ca3d9a20..4cb9c35e 100644 --- a/test/cljc/reitit/ring_test.cljc +++ b/test/cljc/reitit/ring_test.cljc @@ -416,7 +416,31 @@ :get "/slash-less//" "/slash-less?kikka=kukka" :post "/with-slash" "/with-slash/?kikka=kukka" :post "/slash-less/" "/slash-less?kikka=kukka" - :post "/slash-less//" "/slash-less?kikka=kukka")))))) + :post "/slash-less//" "/slash-less?kikka=kukka")))) + + ;; See issue #337 + (testing "Avoid external redirects" + (let [app (ring/ring-handler + (ring/router [["*" {:get (constantly nil)}]]) + (ring/redirect-trailing-slash-handler)) + resp (fn [uri & [query-string]] + (let [r (app {:request-method :get :uri uri :query-string query-string})] + {:status (:status r) + :Location (get-in r [:headers "Location"])}))] + (testing "without query params" + (is (= {:status 301 :Location "/malicious.com/foo/"} (resp "//malicious.com/foo"))) + (is (= {:status 301 :Location "/malicious.com/foo"} (resp "//malicious.com/foo/"))) + (is (= {:status 301 :Location "/malicious.com/foo"} (resp "//malicious.com/foo//"))) + (is (= {:status 301 :Location "/malicious.com/foo/"} (resp "///malicious.com/foo"))) + (is (= {:status 301 :Location "/malicious.com/foo"} (resp "///malicious.com/foo/"))) + (is (= {:status 301 :Location "/malicious.com/foo"} (resp "///malicious.com/foo//")))) + (testing "with query params" + (is (= {:status 301 :Location "/malicious.com/foo/?bar=quux"} (resp "//malicious.com/foo" "bar=quux"))) + (is (= {:status 301 :Location "/malicious.com/foo?bar=quux"} (resp "//malicious.com/foo/" "bar=quux"))) + (is (= {:status 301 :Location "/malicious.com/foo?bar=quux"} (resp "//malicious.com/foo//" "bar=quux"))) + (is (= {:status 301 :Location "/malicious.com/foo/?bar=quux"} (resp "///malicious.com/foo" "bar=quux"))) + (is (= {:status 301 :Location "/malicious.com/foo?bar=quux"} (resp "///malicious.com/foo/" "bar=quux"))) + (is (= {:status 301 :Location "/malicious.com/foo?bar=quux"} (resp "///malicious.com/foo//" "bar=quux")))))))) (deftest async-ring-test (let [promise #(let [value (atom ::nil)]