From cce183e7ce2c14b221e59f15bdf0c9cfea73ecd1 Mon Sep 17 00:00:00 2001
From: John Swanson <john.swanson@metabase.com>
Date: Wed, 3 Apr 2024 13:30:10 -0500
Subject: [PATCH] Don't throw when sent non-int `limit`/`offset` (#40821)
This middleware shouldn't throw exceptions when `limit` or `offset`
query parametesr aren't integers.
---
.../server/middleware/offset_paging.clj | 41 +++++--------------
.../server/middleware/offset_paging_test.clj | 17 ++++----
2 files changed, 18 insertions(+), 40 deletions(-)
diff --git a/src/metabase/server/middleware/offset_paging.clj b/src/metabase/server/middleware/offset_paging.clj
index d08fb532105..441196ce7e2 100644
--- a/src/metabase/server/middleware/offset_paging.clj
+++ b/src/metabase/server/middleware/offset_paging.clj
@@ -1,8 +1,6 @@
(ns metabase.server.middleware.offset-paging
(:require
- [medley.core :as m]
- [metabase.server.middleware.security :as mw.security]
- [metabase.util.i18n :refer [tru]]))
+ [medley.core :as m]))
(set! *warn-on-reflection* true)
@@ -17,15 +15,11 @@
Automatically generated by a handler in offset-paging middleware."
false)
-(defn- offset-paged? [{{:strs [page limit offset]} :query-params}]
- (or page limit offset))
-
(defn- parse-paging-params [{{:strs [limit offset]} :query-params}]
- (let [limit (or (some-> limit Integer/parseUnsignedInt)
- default-limit)
- offset (or (some-> offset Integer/parseUnsignedInt)
- default-offset)]
- {:limit limit, :offset offset}))
+ (let [limit (some-> limit parse-long)
+ offset (some-> offset parse-long)]
+ (when (or limit offset)
+ {:limit (or limit default-limit), :offset (or offset default-offset)})))
(defn- with-paging-params [request {:keys [limit offset]}]
(-> request
@@ -41,22 +35,9 @@
(it isn't stable with respect to underlying data changing, though)"
[handler]
(fn [request respond raise]
- (if-not (offset-paged? request)
- (handler request respond raise)
- (let [paging-params (try
- (parse-paging-params request)
- (catch Throwable e
- e))]
- (if (instance? Throwable paging-params)
- (let [^Throwable e paging-params]
- (respond {:status 400
- :headers (mw.security/security-headers)
- :body (merge
- (Throwable->map e)
- {:message (tru "Error parsing paging parameters: {0}" (ex-message e))})}))
- (let [{:keys [limit offset]} paging-params
- request (with-paging-params request paging-params)]
- (binding [*limit* limit
- *offset* offset
- *paged?* true]
- (handler request respond raise))))))))
+ (if-let [{:keys [limit offset] :as paging-params} (parse-paging-params request)]
+ (binding [*limit* limit
+ *offset* offset
+ *paged?* true]
+ (handler (with-paging-params request paging-params) respond raise))
+ (handler request respond raise))))
diff --git a/test/metabase/server/middleware/offset_paging_test.clj b/test/metabase/server/middleware/offset_paging_test.clj
index a2506835342..11690076e1a 100644
--- a/test/metabase/server/middleware/offset_paging_test.clj
+++ b/test/metabase/server/middleware/offset_paging_test.clj
@@ -5,8 +5,6 @@
[clojure.test :refer :all]
[metabase.server.handler :as handler]
[metabase.server.middleware.offset-paging :as mw.offset-paging]
- [metabase.server.middleware.security :as mw.security]
- [metabase.test :as mt]
[ring.mock.request :as ring.mock]
[ring.util.response :as response])
(:import
@@ -48,11 +46,10 @@
"paged?" true
"params" {"whatever" "true"}}}
(read-response (handler (ring.mock/request :get "/" {:offset "200", :limit "100", :whatever "true"}))))))
- ;; set the system clock here so this doesn't flake if we cross the second boundary between evaluating the expected
- ;; form and the actual form
- (mt/with-clock #t "2023-02-20T15:01:00-08:00[US/Pacific]"
- (testing "invalid params"
- (is (=? {:status 400
- :headers (mw.security/security-headers)
- :body {"message" #"Error parsing paging parameters.*"}}
- (read-response (handler (ring.mock/request :get "/" {:offset "abc", :limit "100"}))))))))
+ (testing "w/ non-numeric paging params, paging is disabled"
+ (is (=? {:status 200
+ :body {"limit" nil
+ "offset" nil
+ "paged?" false
+ "params" {}}}
+ (read-response (handler (ring.mock/request :get "/" {:offset "foo" :limit "bar"})))))))
--
GitLab