Merge pull request #2956 from metabase/fix-is-url

Fix handling of localhost and ports in is-url? 

src/metabase/util.clj

@@ -307,6 +307,13 @@
       [default args]))
 
 
+(defmacro ignore-exceptions
+  "Simple macro which wraps the given expression in a try/catch block and ignores the exception if caught."
+  {:style/indent 0}
+  [& body]
+  `(try ~@body (catch Throwable ~'_)))
+
+
 ;; TODO - rename to `email?`
 (defn is-email?
   "Is STRING a valid email address?"
@@ -317,15 +324,18 @@
 
 ;; TODO - rename to `url?`
 (defn is-url?
-  "Is STRING a valid HTTP/HTTPS URL?"
+  "Is STRING a valid HTTP/HTTPS URL? (This only handles `localhost` and domains like `metabase.com`; URLs containing IP addresses will return `false`.)"
   ^Boolean [^String s]
-  (boolean (when s
-             (when-let [^java.net.URL url (try (java.net.URL. s)
-                                               (catch java.net.MalformedURLException _ ; TODO - use ignore-exceptions
-                                                 nil))]
-               (when (and (.getProtocol url) (.getAuthority url))
-                 (and (re-matches #"^https?$" (.getProtocol url))           ; these are both automatically downcased
-                      (re-matches #"^.+\..{2,}$" (.getAuthority url)))))))) ; this is the part like 'google.com'. Make sure it contains at least one period and 2+ letter TLD
+  (boolean (when (seq s)
+             (when-let [^java.net.URL url (ignore-exceptions (java.net.URL. s))]
+               ;; these are both automatically downcased
+               (let [protocol (.getProtocol url)
+                     host     (.getHost url)]
+                 (and protocol
+                      host
+                      (re-matches #"^https?$" protocol)
+                      (or (re-matches #"^.+\..{2,}$" host) ; 2+ letter TLD
+                          (= host "localhost"))))))))
 
 
 ;; TODO - This should be made into a separate `sequence-of-maps?` function and a `maybe?` function
@@ -525,12 +535,6 @@
                                                      (last args)
                                                      [(last args)]))))
 
-(defmacro ignore-exceptions
-  "Simple macro which wraps the given expression in a try/catch block and ignores the exception if caught."
-  {:style/indent 0}
-  [& body]
-  `(try ~@body (catch Throwable ~'_)))
-
 (defn deref-with-timeout
   "Call `deref` on a FUTURE and throw an exception if it takes more than TIMEOUT-MS."
   [futur timeout-ms]

test/metabase/util_test.clj

@@ -67,6 +67,10 @@
 (expect true (is-url? "https://amazon.co.uk"))
 (expect true (is-url? "http://google.com?q=my-query&etc"))
 (expect true (is-url? "http://www.cool.com"))
+(expect true (is-url? "http://localhost/"))
+(expect true (is-url? "http://localhost:3000"))
+(expect true (is-url? "http://www.cool.com:3000"))
+(expect true (is-url? "http://localhost:3000/auth/reset_password/144_f98987de-53ca-4335-81da-31bb0de8ea2b#new"))
 (expect false (is-url? "google.com"))                      ; missing protocol
 (expect false (is-url? "ftp://metabase.com"))              ; protocol isn't HTTP/HTTPS
 (expect false (is-url? "http://metabasecom"))              ; no period / TLD