Merge f1ade4b089 into e3306e1876

doc: document nuances of reitit.coercion/default-parameter-coercion

doc/ring/coercion.md

@@ -63,8 +63,6 @@ Handlers can access the coerced parameters via the `:parameters` key in the requ
                 {:status 200
                  :body {:total total}}))})
 ```
-
-
 ### Nested parameter definitions
 
 Parameters are accumulated recursively along the route tree, just like
@@ -94,6 +92,26 @@ handling for merging eg. malli `:map` schemas.
 ;         [:task-id :int]]}
 ```
 
+### Differences in behaviour for different parameters
+
+All parameter coercions *except* `:body`:
+
+1. Allow keys outside the schema (by opening up the schema using eg. `malli.util/open-schema`)
+2. Keywordize the keys (ie. header & query parameter names) of the input before coercing
+
+In contrast, the `:body` coercion:
+
+1. Uses the specified schema
+   * depending on the coercion, it can be configured as open or closed, see specific coercion docs for details
+2. Does not keywordize the keys of the input before coercion
+   * however, coercions like malli might do the keywordization when coercing json bodies, depending on configuration
+
+This admittedly confusing behaviour is retained currently due to
+backwards compatibility reasons. It can be configured by passing
+option `:reitit.coercion/parameter-coercion` to `reitit.ring/router`
+or `reitit.coercion/compile-request-coercers`. See also:
+`reitit.coercion/default-parameter-coercion`.
+
 ## Coercion Middleware
 
 Defining a coercion for a route data doesn't do anything, as it's just data. We have to attach some code to apply the actual coercion. We can use the middleware from `reitit.ring.coercion`:

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

modules/reitit-spec/src/reitit/coercion/spec.cljc

@@ -122,6 +122,8 @@
          (= (count model) 1) (first model)
          ;; here be dragons, best effort
          (every? map? model) (apply mm/meta-merge model)
+         ;; all elements are the same
+         (apply = model) (first model)
          ;; fail fast
          :else (ex/fail! ::model-error {:message "Can't merge nested clojure specs", :spec model}))
        name))

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