Skip to content
Permalink
Browse files Browse the repository at this point in the history
GeoJSON URL validation fix (#17990)
  • Loading branch information
noahmoss authored and rlotun committed Sep 21, 2021
1 parent 8903b1b commit 042a36e
Show file tree
Hide file tree
Showing 2 changed files with 23 additions and 12 deletions.
17 changes: 10 additions & 7 deletions src/metabase/api/geojson.clj
Expand Up @@ -58,11 +58,13 @@
(throw (ex-info (invalid-location-msg) {:status-code 400, :url url-string} e)))))

(defn- valid-geojson-url?
[url]
(or (io/resource url)
(valid-url? url)))

(defn- valid-geojson-urls?
[geojson]
(every? (fn [[_ {:keys [url]}]]
(or
(io/resource url)
(valid-url? url)))
(every? (fn [[_ {:keys [url]}]] (valid-geojson-url? url))
geojson))

(defn- validate-geojson
Expand All @@ -72,7 +74,7 @@
(s/validate CustomGeoJSON geojson)
(catch Throwable e
(throw (ex-info (tru "Invalid custom GeoJSON") {:status-code 400} e))))
(or (valid-geojson-url? geojson)
(or (valid-geojson-urls? geojson)
(throw (ex-info (invalid-location-msg) {:status-code 400}))))

(defsetting custom-geojson
Expand Down Expand Up @@ -107,9 +109,10 @@
This behaves similarly to /api/geojson/:key but doesn't require the custom map to be saved to the DB first."
[{{:keys [url]} :params} respond raise]
{url su/NonBlankString}
(api/check-superuser)
(let [decoded-url (rc/url-decode url)]
(or (io/resource decoded-url)
(valid-url? decoded-url))
(when-not (valid-geojson-url? decoded-url)
(raise (ex-info (invalid-location-msg) {:status-code 400})))
(try
(with-open [reader (io/reader (or (io/resource decoded-url)
decoded-url))
Expand Down
18 changes: 13 additions & 5 deletions test/metabase/api/geojson_test.clj
Expand Up @@ -100,10 +100,18 @@
(testing "test the endpoint that fetches JSON files given a URL"
(is (= {:type "Point"
:coordinates [37.77986 -122.429]}
((mt/user->client :rasta) :get 200 "geojson" :url test-geojson-url))))
((mt/user->client :crowberto) :get 200 "geojson" :url test-geojson-url))))
(testing "error is returned if URL connection fails"
(is (= "GeoJSON URL failed to load"
((mt/user->client :rasta) :get 400 "geojson" :url test-broken-geojson-url))))))
((mt/user->client :crowberto) :get 400 "geojson" :url test-broken-geojson-url))))
(testing "error is returned if URL is invalid"
(is (= (str "Invalid GeoJSON file location: must either start with http:// or https:// or be a relative path to "
"a file on the classpath. URLs referring to hosts that supply internal hosting metadata are "
"prohibited.")
((mt/user->client :crowberto) :get 400 "geojson" :url "file://tmp"))))
(testing "cannot be called by non-admins"
(is (= "You don't have permissions to do that."
((mt/user->client :rasta) :get 403 "geojson" :url test-geojson-url))))))

(deftest key-proxy-endpoint-test
(testing "GET /api/geojson/:key"
Expand All @@ -120,9 +128,9 @@
(is (= {:type "Point"
:coordinates [37.77986 -122.429]}
(client/client :get 200 "geojson/middle-earth"))))
(testing "try fetching an invalid key; should fail"
(is (= "Invalid custom GeoJSON key: invalid-key"
((mt/user->client :rasta) :get 400 "geojson/invalid-key")))))
(testing "try fetching an invalid key; should fail"
(is (= "Invalid custom GeoJSON key: invalid-key"
((mt/user->client :rasta) :get 400 "geojson/invalid-key")))))
(mt/with-temporary-setting-values [custom-geojson test-broken-custom-geojson]
(testing "fetching a broken URL should fail"
(is (= "GeoJSON URL failed to load"
Expand Down

0 comments on commit 042a36e

Please sign in to comment.