From f6054a7c408f36e955254c2e246a264ddf4b7d4c Mon Sep 17 00:00:00 2001 From: Matthew Davidson Date: Mon, 23 Jul 2018 00:45:50 -0400 Subject: [PATCH 1/3] Add URL-decoding support Enables matching URLs with %-encoded chars, and decodes path params. Currently Reitit cannot handle %-encoded paths, even though they can be quite common (like `/search/my random search string`) E.g.: Successfully match `/space in path` to `/space%20in%20path` Match `/path/:param1` to `/path/foo%20bar` and see `:param1` => `"foo bar"` in path params Does not apply %-decoding to routes, only URLs Does not guarantee decoding '+' into a space. (Java's URLDecoder supports it, but it shouldn't for URL path part.) --- modules/reitit-core/src/reitit/core.cljc | 4 +-- modules/reitit-core/src/reitit/impl.cljc | 25 +++++++++--------- modules/reitit-core/src/reitit/segment.cljc | 7 ++--- test/cljc/reitit/core_test.cljc | 29 ++++++++++++++++++++- 4 files changed, 47 insertions(+), 18 deletions(-) diff --git a/modules/reitit-core/src/reitit/core.cljc b/modules/reitit-core/src/reitit/core.cljc index 62a77c6d..0249a489 100644 --- a/modules/reitit-core/src/reitit/core.cljc +++ b/modules/reitit-core/src/reitit/core.cljc @@ -241,7 +241,7 @@ (route-names [_] names) (match-by-path [_ path] - (impl/fast-get data path)) + (impl/fast-get data (impl/url-decode path))) (match-by-name [_ name] (if-let [match (impl/fast-get lookup name)] (match nil))) @@ -321,7 +321,7 @@ (route-names [_] names) (match-by-path [_ path] - (if (#?(:clj .equals :cljs =) p path) + (if (#?(:clj .equals :cljs =) p (impl/url-decode path)) match)) (match-by-name [_ name] (if (= n name) diff --git a/modules/reitit-core/src/reitit/impl.cljc b/modules/reitit-core/src/reitit/impl.cljc index d5a41969..064b3a34 100644 --- a/modules/reitit-core/src/reitit/impl.cljc +++ b/modules/reitit-core/src/reitit/impl.cljc @@ -45,6 +45,17 @@ (defn contains-wilds? [path] (boolean (some wild-or-catch-all-param? (segments path)))) +(defn url-encode [s] + (some-> s + #?(:clj (URLEncoder/encode "UTF-8") + :cljs (js/encodeURIComponent)) + #?(:clj (.replace "+" "%20")))) + +(defn url-decode [s] + (some-> s #?(:clj (URLDecoder/decode "UTF-8") + :cljs (js/decodeURIComponent)))) + + ;; ;; https://github.com/pedestal/pedestal/blob/master/route/src/io/pedestal/http/route/path.clj ;; @@ -93,7 +104,7 @@ (let [{:keys [path-re path-params]} route] (fn [path] (when-let [m (re-matches path-re path)] - (zipmap path-params (rest m)))))) + (zipmap path-params (map url-decode (rest m))))))) ;; ;; Routing (c) Metosin @@ -108,7 +119,7 @@ (merge $ {:path path :matcher (if (contains-wilds? path) (path-matcher $) - #(if (#?(:clj .equals, :cljs =) path %) {})) + #(if (#?(:clj .equals, :cljs =) path (url-decode %)) {})) :result result :data data}) (dissoc $ :path-re :path-constraints) @@ -166,16 +177,6 @@ ;; Path-parameters, see https://github.com/metosin/reitit/issues/75 ;; -(defn url-encode [s] - (some-> s - #?(:clj (URLEncoder/encode "UTF-8") - :cljs (js/encodeURIComponent)) - #?(:clj (.replace "+" "%20")))) - -(defn url-decode [s] - (some-> s #?(:clj (URLDecoder/decode "UTF-8") - :cljs (js/decodeURIComponent)))) - (defprotocol IntoString (into-string [_])) diff --git a/modules/reitit-core/src/reitit/segment.cljc b/modules/reitit-core/src/reitit/segment.cljc index d1a1dda2..35cd9cad 100644 --- a/modules/reitit-core/src/reitit/segment.cljc +++ b/modules/reitit-core/src/reitit/segment.cljc @@ -39,9 +39,10 @@ (-lookup [_ [p & ps] path-params] (if (nil? p) (if match (assoc match :path-params path-params)) - (or (-lookup (impl/fast-get children' p) ps path-params) - (some #(-lookup (impl/fast-get children' %) ps (assoc path-params % p)) wilds) - (-catch-all children' catch-all path-params p ps)))))))) + (let [p (impl/url-decode p)] + (or (-lookup (impl/fast-get children' p) ps path-params) + (some #(-lookup (impl/fast-get children' %) ps (assoc path-params % p)) wilds) + (-catch-all children' catch-all path-params p ps))))))))) (defn insert [root path data] (-insert (or root (segment)) (impl/segments path) (map->Match {:data data}))) diff --git a/test/cljc/reitit/core_test.cljc b/test/cljc/reitit/core_test.cljc index 19091509..7fc68cb8 100644 --- a/test/cljc/reitit/core_test.cljc +++ b/test/cljc/reitit/core_test.cljc @@ -51,6 +51,22 @@ #"^missing path-params for route /api/ipa/:size -> \#\{:size\}$" (r/match-by-name! router ::beer)))))) + (testing "URL-encoded" + (let [router (r/router [["/one-param-path/:param"] + ["/two-param-path/:param1/:param2"] + ["/catchall/*remaining-path"] + ["/space in path"]] {:router r}) + decoded-params #(-> router (r/match-by-path %) :path-params) + decoded-param #(-> (decoded-params %) :param) + decoded-catchall #(-> (decoded-params %) :remaining-path)] + (println "Testing" (r/router-name router)) + (is (= "foo bar" (decoded-param "/one-param-path/foo%20bar"))) + (is (= {:param1 "foo bar" :param2 "baz qux"} (decoded-params "/two-param-path/foo%20bar/baz%20qux"))) + (is (= "foo bar" (decoded-catchall "/catchall/foo%20bar"))) + (is (= "foo bar" (decoded-catchall "/catchall/foo%20bar"))) + (is (= "!#$&'()*+,/:;=?@[]" (decoded-param "/one-param-path/%21%23%24%26%27%28%29%2A%2B%2C%2F%3A%3B%3D%3F%40%5B%5D"))) + (is (= "/space in path" (-> router (r/match-by-path "/space%20in%20path") :template))))) + (testing "complex" (let [router (r/router [["/:abba" ::abba] @@ -100,7 +116,18 @@ #"can't create :lookup-router with wildcard routes" (r/lookup-router (r/resolve-routes - ["/api/:version/ping"] {})))))) + ["/api/:version/ping"] {}))))) + + (testing "URL-decoding" + (let [router (r/router [["/space in path"]] {:router r}) + matched-template #(-> router (r/match-by-path %) :template)] + (is (= "/space in path" (matched-template "/space%20in%20path")))) + + (testing "should only apply to real URLs, not configured routes" + (let [router (r/router [["/percent%20in%20path"]] {:router r}) + matched-template #(-> router (r/match-by-path %) :template)] + (is (= "/percent%20in%20path" (matched-template "/percent%2520in%2520path"))) + (is (not= "/percent%20in%20path" (matched-template "/percent%20in%20path"))))))) r/lookup-router :lookup-router r/single-static-path-router :single-static-path-router From 35656c3da641c99281623cead777303ef8e5075c Mon Sep 17 00:00:00 2001 From: Matthew Davidson Date: Tue, 7 Aug 2018 20:50:03 -0400 Subject: [PATCH 2/3] Extract lookup struct-generating code --- modules/reitit-core/src/reitit/core.cljc | 76 ++++++++++++++++-------- 1 file changed, 52 insertions(+), 24 deletions(-) diff --git a/modules/reitit-core/src/reitit/core.cljc b/modules/reitit-core/src/reitit/core.cljc index 3a094326..d3f0d8e7 100644 --- a/modules/reitit-core/src/reitit/core.cljc +++ b/modules/reitit-core/src/reitit/core.cljc @@ -165,6 +165,23 @@ :compile (fn [[_ {:keys [handler]}] _] handler) :conflicts (partial throw-on-conflicts! path-conflicts-str)}) +(defn- linear-router-lookup-structs + "Returns a 2-item vec of lookup structures. + + The first is a vec of Routes. + The second is a map of route names to lookup fns." + [compiled-routes] + (reduce + (fn [[pl nl] [p {:keys [name] :as data} result]] + (let [{:keys [path-params] :as route} (impl/create [p data result]) + f #(if-let [path (impl/path-for route %)] + (->Match p data result % path) + (->PartialMatch p data result % path-params))] + [(conj pl route) + (if name (assoc nl name f) nl)])) + [[] {}] + compiled-routes)) + (defn linear-router "Creates a linear-router from resolved routes and optional expanded options. See [[router]] for available options" @@ -172,15 +189,7 @@ (linear-router compiled-routes {})) ([compiled-routes opts] (let [names (find-names compiled-routes opts) - [pl nl] (reduce - (fn [[pl nl] [p {:keys [name] :as data} result]] - (let [{:keys [path-params] :as route} (impl/create [p data result]) - f #(if-let [path (impl/path-for route %)] - (->Match p data result % path) - (->PartialMatch p data result % path-params))] - [(conj pl route) - (if name (assoc nl name f) nl)])) - [[] {}] compiled-routes) + [pl nl] (linear-router-lookup-structs compiled-routes) lookup (impl/fast-map nl) routes (uncompile-routes compiled-routes)] ^{:type ::router} @@ -209,6 +218,21 @@ (if-let [match (impl/fast-get lookup name)] (match (impl/path-params path-params)))))))) +(defn- lookup-router-lookup-structs + "Returns a 2-item vec of lookup structures. + + The first is a map of paths to Matches. + The second is a map of route names to Matches." + [compiled-routes] + (reduce + (fn [[pl nl] [p {:keys [name] :as data} result]] + [(assoc pl p (->Match p data result {} p)) + (if name + (assoc nl name #(->Match p data result % p)) + nl)]) + [{} {}] + compiled-routes)) + (defn lookup-router "Creates a lookup-router from resolved routes and optional expanded options. See [[router]] for available options" @@ -222,12 +246,7 @@ {:wilds wilds :routes compiled-routes}))) (let [names (find-names compiled-routes opts) - [pl nl] (reduce - (fn [[pl nl] [p {:keys [name] :as data} result]] - [(assoc pl p (->Match p data result {} p)) - (if name - (assoc nl name #(->Match p data result % p)) - nl)]) [{} {}] compiled-routes) + [pl nl] (lookup-router-lookup-structs compiled-routes) data (impl/fast-map pl) lookup (impl/fast-map nl) routes (uncompile-routes compiled-routes)] @@ -252,6 +271,23 @@ (if-let [match (impl/fast-get lookup name)] (match (impl/path-params path-params)))))))) +(defn- segment-router-lookup-structs + "Returns a 2-item vec of lookup structures. + + The first is a prefix-tree of segments and associated Matches. + The second is a map of route names to Matches or PartialMatches." + [compiled-routes] + (reduce + (fn [[pl nl] [p {:keys [name] :as data} result]] + (let [{:keys [path-params] :as route} (impl/create [p data result]) + f #(if-let [path (impl/path-for route %)] + (->Match p data result % path) + (->PartialMatch p data result % path-params))] + [(segment/insert pl p (->Match p data result nil nil)) + (if name (assoc nl name f) nl)])) + [nil {}] + compiled-routes)) + (defn segment-router "Creates a special prefix-tree style segment router from resolved routes and optional expanded options. See [[router]] for available options" @@ -259,15 +295,7 @@ (segment-router compiled-routes {})) ([compiled-routes opts] (let [names (find-names compiled-routes opts) - [pl nl] (reduce - (fn [[pl nl] [p {:keys [name] :as data} result]] - (let [{:keys [path-params] :as route} (impl/create [p data result]) - f #(if-let [path (impl/path-for route %)] - (->Match p data result % path) - (->PartialMatch p data result % path-params))] - [(segment/insert pl p (->Match p data result nil nil)) - (if name (assoc nl name f) nl)])) - [nil {}] compiled-routes) + [pl nl] (segment-router-lookup-structs compiled-routes) lookup (impl/fast-map nl) routes (uncompile-routes compiled-routes)] ^{:type ::router} From ec051a0c9dbe0534b03547c2611c33e495ff13e7 Mon Sep 17 00:00:00 2001 From: Matthew Davidson Date: Sun, 5 Aug 2018 18:58:31 -0400 Subject: [PATCH 3/3] Decode %-encoded URL path params Also adds utility fn map-kv which is convenient for en/decoding both maps (like path-params) and vectors (like path parts) Converts path-params fn to use map-kv --- modules/reitit-core/src/reitit/core.cljc | 8 ++--- modules/reitit-core/src/reitit/impl.cljc | 28 ++++++++++++----- modules/reitit-core/src/reitit/segment.cljc | 9 +++--- test/cljc/reitit/core_test.cljc | 34 ++++++--------------- 4 files changed, 38 insertions(+), 41 deletions(-) diff --git a/modules/reitit-core/src/reitit/core.cljc b/modules/reitit-core/src/reitit/core.cljc index d3f0d8e7..70a5680a 100644 --- a/modules/reitit-core/src/reitit/core.cljc +++ b/modules/reitit-core/src/reitit/core.cljc @@ -209,7 +209,7 @@ (reduce (fn [_ ^Route route] (if-let [path-params ((:matcher route) path)] - (reduced (->Match (:path route) (:data route) (:result route) path-params path)))) + (reduced (->Match (:path route) (:data route) (:result route) (impl/url-decode-coll path-params) path)))) nil pl)) (match-by-name [_ name] (if-let [match (impl/fast-get lookup name)] @@ -263,7 +263,7 @@ (route-names [_] names) (match-by-path [_ path] - (impl/fast-get data (impl/url-decode path))) + (impl/fast-get data path)) (match-by-name [_ name] (if-let [match (impl/fast-get lookup name)] (match nil))) @@ -314,7 +314,7 @@ (match-by-path [_ path] (if-let [match (segment/lookup pl path)] (-> (:data match) - (assoc :path-params (:path-params match)) + (assoc :path-params (impl/url-decode-coll (:path-params match))) (assoc :path path)))) (match-by-name [_ name] (if-let [match (impl/fast-get lookup name)] @@ -352,7 +352,7 @@ (route-names [_] names) (match-by-path [_ path] - (if (#?(:clj .equals :cljs =) p (impl/url-decode path)) + (if (#?(:clj .equals :cljs =) p path) match)) (match-by-name [_ name] (if (= n name) diff --git a/modules/reitit-core/src/reitit/impl.cljc b/modules/reitit-core/src/reitit/impl.cljc index ea29670f..6f954807 100644 --- a/modules/reitit-core/src/reitit/impl.cljc +++ b/modules/reitit-core/src/reitit/impl.cljc @@ -19,6 +19,17 @@ (java.util HashMap Map) (java.net URLEncoder URLDecoder)))) +(defn map-kv + "Applies a function to every value of a map. + + Also works on vectors. Maintains key for maps, order for vectors." + [f coll] + (reduce-kv + (fn [m k v] + (assoc m k (f v))) + (empty coll) + coll)) + (defn wild? [s] (contains? #{\: \*} (first (str s)))) @@ -93,7 +104,7 @@ (let [{:keys [path-re path-params]} route] (fn [path] (when-let [m (re-matches path-re path)] - (zipmap path-params (map url-decode (rest m))))))) + (zipmap path-params (rest m)))))) ;; ;; Routing (c) Metosin @@ -108,7 +119,7 @@ (merge $ {:path path :matcher (if (contains-wilds? path) (path-matcher $) - #(if (#?(:clj .equals, :cljs =) path (url-decode %)) {})) + #(if (#?(:clj .equals, :cljs =) path %) {})) :result result :data data}) (dissoc $ :path-re :path-constraints) @@ -203,6 +214,11 @@ s) :cljs (js/decodeURIComponent (str/replace s "+" " "))))) +(defn url-decode-coll + "URL-decodes maps and vectors" + [coll] + (map-kv url-decode coll)) + (defprotocol IntoString (into-string [_])) @@ -233,13 +249,9 @@ (into-string [_])) (defn path-params - "shallow transform of the path parameters values into strings" + "Convert parameters' values into URL-encoded strings, suitable for URL paths" [params] - (reduce-kv - (fn [m k v] - (assoc m k (url-encode (into-string v)))) - {} - params)) + (map-kv #(url-encode (into-string %)) params)) (defn query-string "shallow transform of query parameters into query string" diff --git a/modules/reitit-core/src/reitit/segment.cljc b/modules/reitit-core/src/reitit/segment.cljc index 35cd9cad..929cf28b 100644 --- a/modules/reitit-core/src/reitit/segment.cljc +++ b/modules/reitit-core/src/reitit/segment.cljc @@ -38,11 +38,10 @@ (segment children wilds catch-all match)))) (-lookup [_ [p & ps] path-params] (if (nil? p) - (if match (assoc match :path-params path-params)) - (let [p (impl/url-decode p)] - (or (-lookup (impl/fast-get children' p) ps path-params) - (some #(-lookup (impl/fast-get children' %) ps (assoc path-params % p)) wilds) - (-catch-all children' catch-all path-params p ps))))))))) + (when match (assoc match :path-params path-params)) + (or (-lookup (impl/fast-get children' p) ps path-params) + (some #(-lookup (impl/fast-get children' %) ps (assoc path-params % p)) wilds) + (-catch-all children' catch-all path-params p ps)))))))) (defn insert [root path data] (-insert (or root (segment)) (impl/segments path) (map->Match {:data data}))) diff --git a/test/cljc/reitit/core_test.cljc b/test/cljc/reitit/core_test.cljc index 7817d365..8a7e3482 100644 --- a/test/cljc/reitit/core_test.cljc +++ b/test/cljc/reitit/core_test.cljc @@ -51,21 +51,18 @@ #"^missing path-params for route /api/ipa/:size -> \#\{:size\}$" (r/match-by-name! router ::beer)))))) - (testing "URL-encoded" - (let [router (r/router [["/one-param-path/:param"] + (testing "decode %-encoded path params" + (let [router (r/router [["/one-param-path/:param1"] ["/two-param-path/:param1/:param2"] - ["/catchall/*remaining-path"] - ["/space in path"]] {:router r}) + ["/catchall/*remaining-path"]] {:router r}) decoded-params #(-> router (r/match-by-path %) :path-params) - decoded-param #(-> (decoded-params %) :param) - decoded-catchall #(-> (decoded-params %) :remaining-path)] - (println "Testing" (r/router-name router)) - (is (= "foo bar" (decoded-param "/one-param-path/foo%20bar"))) + decoded-param1 #(-> (decoded-params %) :param1) + decoded-remaining-path #(-> (decoded-params %) :remaining-path)] + (is (= "foo bar" (decoded-param1 "/one-param-path/foo%20bar"))) (is (= {:param1 "foo bar" :param2 "baz qux"} (decoded-params "/two-param-path/foo%20bar/baz%20qux"))) - (is (= "foo bar" (decoded-catchall "/catchall/foo%20bar"))) - (is (= "foo bar" (decoded-catchall "/catchall/foo%20bar"))) - (is (= "!#$&'()*+,/:;=?@[]" (decoded-param "/one-param-path/%21%23%24%26%27%28%29%2A%2B%2C%2F%3A%3B%3D%3F%40%5B%5D"))) - (is (= "/space in path" (-> router (r/match-by-path "/space%20in%20path") :template))))) + (is (= "foo bar" (decoded-remaining-path "/catchall/foo%20bar"))) + (is (= "!#$&'()*+,/:;=?@[]" + (decoded-param1 "/one-param-path/%21%23%24%26%27%28%29%2A%2B%2C%2F%3A%3B%3D%3F%40%5B%5D"))))) (testing "complex" (let [router (r/router @@ -116,18 +113,7 @@ #"can't create :lookup-router with wildcard routes" (r/lookup-router (r/resolve-routes - ["/api/:version/ping"] {}))))) - - (testing "URL-decoding" - (let [router (r/router [["/space in path"]] {:router r}) - matched-template #(-> router (r/match-by-path %) :template)] - (is (= "/space in path" (matched-template "/space%20in%20path")))) - - (testing "should only apply to real URLs, not configured routes" - (let [router (r/router [["/percent%20in%20path"]] {:router r}) - matched-template #(-> router (r/match-by-path %) :template)] - (is (= "/percent%20in%20path" (matched-template "/percent%2520in%2520path"))) - (is (not= "/percent%20in%20path" (matched-template "/percent%20in%20path"))))))) + ["/api/:version/ping"] {})))))) r/lookup-router :lookup-router r/single-static-path-router :single-static-path-router