Merge pull request #776 from metosin/feat/337-fix-external-redirect

fix: redirect-trailing-slash-handler won't make external redirects

Author: opqdonut

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)]

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)]