Merge 7a707f042b 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-core/src/reitit/regex.cljc

@@ -0,0 +1,121 @@
+(ns reitit.regex
+  (:require [clojure.set :as set]
+            [clojure.string :as str]
+            [reitit.core :as r]))
+
+(defn compile-regex-route
+  "Given a route vector [path route-data], returns a map with:
+   - :pattern: a compiled regex pattern built from the path segments,
+   - :group-keys: vector of parameter keys in order,
+   - :route-data: the provided route data,
+   - :original-segments: original path segments for path generation,
+   - :template: the original path template for Match objects."
+  [[path route-data]]
+  (let [;; Normalize route-data to ensure it's a map with :name
+        route-data (if (keyword? route-data)
+                     {:name route-data}
+                     route-data)
+
+        ;; Store the original path template for Match objects
+        template (if (str/starts-with? path "/")
+                   path
+                   (str "/" path))
+
+        ;; Handle paths with or without leading slashes
+        normalized-path (cond-> path
+                                (str/starts-with? path "/") (subs 1))
+
+        ;; Split into segments, handling empty paths
+        segments (if (empty? normalized-path)
+                   []
+                   (str/split normalized-path #"/"))
+
+        ;; Store original segments for path generation
+        original-segments segments
+
+        compiled-segments
+        (map (fn [seg]
+               (if (str/starts-with? seg ":")
+                 (let [param-key   (keyword (subs seg 1))
+                       param-regex (get-in route-data [:parameters :path param-key])]
+                   (if (and param-regex (instance? java.util.regex.Pattern param-regex))
+                     (str "(" (.pattern ^java.util.regex.Pattern param-regex) ")")
+                     ;; Fallback: match any non-slash characters.
+                     "([^/]+)"))
+                 (java.util.regex.Pattern/quote seg)))
+             segments)
+
+        ;; Create the pattern string, handling special case for root path
+        pattern-str (if (empty? segments)
+                      "^/?$"  ;; Match root path with optional trailing slash
+                      (str "^/" (str/join "/" compiled-segments) "$"))
+
+        group-keys (->> segments
+                        (filter #(str/starts-with? % ":"))
+                        (map #(keyword (subs % 1)))
+                        (vec))]
+
+    {:pattern    (re-pattern pattern-str)
+     :group-keys group-keys
+     :route-data route-data
+     :original-segments original-segments
+     :template template}))
+
+(defn- generate-path
+  "Generate a path from a route and path parameters."
+  [route path-params]
+  (if (empty? (:original-segments route))
+    "/"
+    (str "/" (str/join "/"
+                       (map (fn [segment]
+                              (if (str/starts-with? segment ":")
+                                (let [param-key (keyword (subs segment 1))]
+                                  (get path-params param-key ""))
+                                segment))
+                            (:original-segments route))))))
+
+(defrecord RegexRouter [compiled-routes]
+  r/Router
+  (router-name [_] :regex-router)
+
+  (routes [_]
+    (mapv (fn [{:keys [route-data original-segments]}]
+            [(str "/" (str/join "/" original-segments)) route-data])
+          compiled-routes))
+
+  (compiled-routes [_] compiled-routes)
+
+  (options [_] {})
+
+  (route-names [_]
+    (keep (comp :name :route-data) compiled-routes))
+
+  (match-by-path [_ path]
+    (some (fn [{:keys [pattern group-keys route-data template]}]
+            (when-let [matches (re-matches pattern path)]
+              (let [params (zipmap group-keys (rest matches))]
+                (r/->Match template route-data nil params path))))
+          compiled-routes))
+
+  (match-by-name [this name]
+    (r/match-by-name this name {}))
+
+  (match-by-name [router name path-params]
+    (when-let [{:keys [group-keys route-data template] :as route}
+               (first (filter #(= name (get-in % [:route-data :name])) (r/compiled-routes router)))]
+      ;; Check if all required params are provided
+      (let [required-params (set group-keys)
+            provided-params (set (keys path-params))]
+        (if (every? #(contains? provided-params %) required-params)
+          ;; All required params provided, return a Match
+          (let [path (generate-path route path-params)]
+            (r/->Match template route-data nil path-params path))
+          ;; Some required params missing, return a PartialMatch
+          (let [missing (set/difference required-params provided-params)]
+            (r/->PartialMatch template route-data nil path-params missing)))))))
+
+(defn create-regex-router
+  "Create a RegexRouter from a vector of routes.
+   Each route should be a vector [path route-data]."
+  [routes]
+  (->RegexRouter (mapv compile-regex-route routes)))

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/regex_test.cljc

@@ -0,0 +1,278 @@
+(ns reitit.regex-test
+  (:require [clojure.test :refer [deftest is testing]]
+            [reitit.core :as r]
+            [reitit.regex :as rt.regex]))
+
+(defn re-=
+  "A custom equality function that handles regex patterns specially.
+   Returns true if a and b are equal, with special handling for regex patterns.
+   Also handles comparing records by their map representation."
+  [a b]
+  (cond
+    ;; Handle record comparison by using their map representation
+    (and (instance? clojure.lang.IRecord a)
+         (instance? clojure.lang.IRecord b))
+    (re-= (into {} a) (into {} b))
+
+    ;; If both are regex patterns, compare their string representations
+    (and (instance? java.util.regex.Pattern a)
+         (instance? java.util.regex.Pattern b))
+    (= (str a) (str b))
+
+    ;; If one is a regex and the other isn't, they're not equal
+    (or (instance? java.util.regex.Pattern a)
+        (instance? java.util.regex.Pattern b))
+    false
+
+    ;; For maps, compare each key-value pair using regex-aware-equals
+    (and (map? a) (map? b))
+    (and (= (set (keys a)) (set (keys b)))
+         (every? #(re-= (get a %) (get b %)) (keys a)))
+
+    ;; For sequences, compare each element using regex-aware-equals
+    (and (sequential? a) (sequential? b))
+    (and (= (count a) (count b))
+         (every? identity (map re-= a b)))
+
+    ;; For sets, convert to sequences and compare
+    (and (set? a) (set? b))
+    (re-= (seq a) (seq b))
+
+    ;; For everything else, use regular equality
+    :else
+    (= a b)))
+
+(def routes
+  (rt.regex/create-regex-router
+    [["" ::home]
+     [":item-id" {:name ::item
+                  :parameters {:path {:item-id #"[a-z]{16,20}"}}}]
+     ["inbox" ::inbox]
+     ["teams" ::teams]
+     ["teams/:team-id-b58/members" {:name ::->members
+                                    :parameters {:path {:team-id-b58 #"[a-z]"}}}]]))
+
+(deftest regex-match-by-path-test
+  (testing "Basic path matching"
+    (is (= (r/map->Match {:path "/"
+                          :path-params {}
+                          :data {:name ::home}
+                          :template "/"
+                          :result nil})
+           (r/match-by-path routes "/")))
+
+    (is (= (r/map->Match {:path "/inbox"
+                          :path-params {}
+                          :data {:name ::inbox}
+                          :template "/inbox"
+                          :result nil})
+           (r/match-by-path routes "/inbox")))
+
+    (is (= (r/map->Match {:path "/teams"
+                          :path-params {}
+                          :data {:name ::teams}
+                          :template "/teams"
+                          :result nil})
+           (r/match-by-path routes "/teams"))))
+
+  (testing "Path with regex parameter"
+    (let [valid-id "abcdefghijklmnopq"] ; 17 lowercase letters
+      (is (re-= (r/map->Match {:path (str "/" valid-id)
+                               :path-params {:item-id valid-id}
+                               :data {:name ::item
+                                      :parameters {:path {:item-id #"[a-z]{16,20}"}}},
+                               :template "/:item-id"
+                               :result nil})
+                (r/match-by-path routes (str "/" valid-id)))))
+
+    ;; Invalid parameter cases
+    (is (nil? (r/match-by-path routes "/abcdefg")) "Too short")
+    (is (nil? (r/match-by-path routes "/abcdefghijklmnopqRST")) "Contains uppercase")
+    (is (nil? (r/match-by-path routes "/abcdefghijklmn1234")) "Contains digits"))
+
+  (testing "Nested path with parameter"
+    (is (re-= (r/map->Match {:path "/teams/a/members"
+                             :path-params {:team-id-b58 "a"}
+                             :data {:name ::->members
+                                    :parameters {:path {:team-id-b58 #"[a-z]"}}}
+                             :template "/teams/:team-id-b58/members"
+                             :result nil})
+              (r/match-by-path routes "/teams/a/members")))
+
+    (is (nil? (r/match-by-path routes "/teams/abc/members")) "Multiple characters")
+    (is (nil? (r/match-by-path routes "/teams/1/members")) "Digit instead of letter"))
+
+  (testing "Non-matching paths"
+    (is (nil? (r/match-by-path routes "/unknown")))
+    (is (nil? (r/match-by-path routes "/team"))) ; 'team' not 'teams'
+    (is (nil? (r/match-by-path routes "/teams/extra/segments/here")))))
+
+(deftest regex-match-by-name-test
+  (testing "Basic match-by-name functionality"
+    ;; Root path
+    (is (re-= (r/map->Match {:path "/"
+                             :path-params {}
+                             :data {:name ::home}
+                             :template "/"
+                             :result nil})
+              (r/match-by-name routes ::home)))
+
+    ;; Static paths
+    (is (re-= (r/map->Match {:path "/inbox"
+                             :path-params {}
+                             :data {:name ::inbox}
+                             :template "/inbox"
+                             :result nil})
+              (r/match-by-name routes ::inbox)))
+
+    (is (re-= (r/map->Match {:path "/teams"
+                             :path-params {}
+                             :data {:name ::teams}
+                             :template "/teams"
+                             :result nil})
+              (r/match-by-name routes ::teams)))
+
+    ;; Path with parameter
+    (let [valid-id "abcdefghijklmnopq"]
+      (is (re-= (r/map->Match {:path (str "/" valid-id)
+                               :path-params {:item-id valid-id}
+                               :data {:name ::item
+                                      :parameters {:path {:item-id #"[a-z]{16,20}"}}}
+                               :template "/:item-id"
+                               :result nil})
+                (r/match-by-name routes ::item {:item-id valid-id}))))
+
+    ;; Nested path with parameter
+    (is (re-= (r/map->Match {:path "/teams/a/members"
+                             :path-params {:team-id-b58 "a"}
+                             :data {:name ::->members
+                                    :parameters {:path {:team-id-b58 #"[a-z]"}}}
+                             :template "/teams/:team-id-b58/members"
+                             :result nil})
+              (r/match-by-name routes ::->members {:team-id-b58 "a"}))))
+
+  (testing "Path round-trip matching"
+    ;; Test that paths generated by match-by-name can be successfully matched by match-by-path
+    (let [valid-id "abcdefghijklmnopq"
+          match (r/match-by-name routes ::item {:item-id valid-id})
+          path (:path match)]
+
+      (is (some? path) "Should generate a valid path")
+      (is (re-= match (r/match-by-path routes path))
+          "match-by-path should find the same route that generated the path"))
+
+    (let [match (r/match-by-name routes ::->members {:team-id-b58 "a"})
+          path (:path match)]
+
+      (is (some? path) "Should generate a valid path")
+      (is (re-= match (r/match-by-path routes path))
+          "match-by-path should find the same route that generated the path")))
+
+  (testing "Partial match with missing parameters"
+    ;; Test that routes with missing parameters return PartialMatch
+    (let [partial-match (r/match-by-name routes ::item {})]
+      (is (instance? reitit.core.PartialMatch partial-match)
+          "Should return a PartialMatch when params are missing")
+      (is (= #{:item-id} (:required partial-match))
+          "PartialMatch should indicate the required parameters")
+      (is (re-= (r/map->PartialMatch {:template "/:item-id"
+                                      :data {:name ::item
+                                             :parameters {:path {:item-id #"[a-z]{16,20}"}}}
+                                      :path-params {}
+                                      :required #{:item-id}
+                                      :result nil})
+                partial-match)))
+
+    ;; Test for a nested path with missing parameters
+    (let [partial-match (r/match-by-name routes ::->members {})]
+      (is (instance? reitit.core.PartialMatch partial-match)
+          "Should return a PartialMatch for nested paths too")
+      (is (= #{:team-id-b58} (:required partial-match))
+          "PartialMatch should indicate the required parameters")))
+
+  (testing "Match with invalid parameters"
+    ;; Invalid parameters (that don't match the regex) still produce a Match
+    (let [match (r/match-by-name routes ::item {:item-id "too-short"})
+          path (:path match)]
+
+      (is (instance? reitit.core.Match match)
+          "Should produce a Match even with invalid parameters")
+      (is (= "/too-short" path)
+          "Path should contain the provided parameter value")
+      (is (nil? (r/match-by-path routes path))
+          "Path with invalid parameter shouldn't be matchable by match-by-path")))
+
+  (testing "Non-existent routes"
+    (is (nil? (r/match-by-name routes ::non-existent))
+        "Should return nil for non-existent routes")))
+
+(deftest regex-router-edge-cases-test
+  (testing "Empty router"
+    (let [empty-router (rt.regex/create-regex-router [])]
+      (is (nil? (r/match-by-path empty-router "/any/path")))))
+
+  (testing "Handling trailing slashes"
+    (is (nil? (r/match-by-path routes "/inbox/")))
+
+    (let [router-with-trailing-slash (rt.regex/create-regex-router [["inbox/" ::inbox-with-slash]])]
+      (is (nil? (r/match-by-path router-with-trailing-slash "/inbox/")))
+      (is (some? (r/match-by-path router-with-trailing-slash "/inbox")))))
+
+  (testing "Complex path patterns"
+    (let [complex-router (rt.regex/create-regex-router
+                           [["articles/:year/:month/:slug"
+                             {:name ::article
+                              :parameters {:path {:year #"\d{4}"
+                                                  :month #"\d{2}"
+                                                  :slug #"[a-z0-9\-]+"}}}]
+                            ["files/:path*"
+                             {:name ::file-path}]])]
+
+      ;; Test article route with valid params
+      (let [match (r/match-by-name complex-router ::article
+                                   {:year "2023" :month "02" :slug "test-article"})]
+        (is (instance? reitit.core.Match match)
+            "Should return a Match for complex routes with valid params")
+        (is (= "/articles/2023/02/test-article" (:path match))
+            "Path should be constructed correctly"))
+
+      ;; Test match-by-path with the generated path
+      (let [match (r/match-by-path complex-router "/articles/2023/02/test-article")]
+        (is (some? match)
+            "Should match a valid article path")
+        (is (= {:year "2023", :month "02", :slug "test-article"}
+               (:path-params match))
+            "Should extract all parameters correctly"))
+
+      ;; Test invalid path
+      (is (nil? (r/match-by-path complex-router "/articles/202/02/test-article"))
+          "Should not match an invalid year (3 digits)")
+
+      ;; Test partial params
+      (let [partial-match (r/match-by-name complex-router ::article {:year "2023"})]
+        (is (instance? reitit.core.PartialMatch partial-match)
+            "Should return PartialMatch when some params are missing")
+        (is (= #{:month :slug} (:required partial-match))
+            "Should indicate which params are missing")))))
+
+(deftest custom-router-features-test
+  (testing "Router information access"
+    ;; Test that router information methods work properly
+    (is (= :regex-router (r/router-name routes))
+        "Should return the correct router name")
+
+    (is (seq (r/routes routes))
+        "Should return the list of routes")
+
+    (is (= (set [::home ::item ::inbox ::teams ::->members])
+           (set (r/route-names routes)))
+        "Should return all route names"))
+
+  (testing "Compiled routes access"
+    (let [compiled (r/compiled-routes routes)]
+      (is (seq compiled)
+          "Should return compiled routes")
+      (is (every? :pattern compiled)
+          "Every compiled route should have a pattern")
+      (is (every? :route-data compiled)
+          "Every compiled route should have route data"))))

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