Merge pull request #9547 from metabase/add-check-application-type-headers-middleware

Add middleware to respond w/ error if Content-Type != application/json

src/metabase/api/alert.clj

@@ -114,11 +114,11 @@
   "Create a new Alert."
   [:as {{:keys [alert_condition card channels alert_first_only alert_above_goal]
          :as new-alert-request-body} :body}]
-  {alert_condition     pulse/AlertConditions
-   alert_first_only    s/Bool
-   alert_above_goal    (s/maybe s/Bool)
-   card                pulse/CardRef
-   channels            (su/non-empty [su/Map])}
+  {alert_condition  pulse/AlertConditions
+   alert_first_only s/Bool
+   alert_above_goal (s/maybe s/Bool)
+   card             pulse/CardRef
+   channels         (su/non-empty [su/Map])}
   ;; do various perms checks as needed. Perms for an Alert == perms for its Card. So to create an Alert you need write
   ;; perms for its Card
   (api/write-check Card (u/get-id card))

src/metabase/handler.clj

@@ -32,9 +32,9 @@
    mw.session/wrap-session-id              ; looks for a Metabase Session ID and assoc as :metabase-session-id
    mw.auth/wrap-api-key                    ; looks for a Metabase API Key on the request and assocs as :metabase-api-key
    mw.misc/maybe-set-site-url              ; set the value of `site-url` if it hasn't been set yet
+   mw.json/check-application-type-headers  ; Reject non-GET requests without Content-Type: application/json headers, we don't support them
    mw.misc/bind-user-locale                ; Binds *locale* for i18n
    wrap-cookies                            ; Parses cookies in the request map and assocs as :cookies
-   #_wrap-session                            ; reads in current HTTP session and sets :session key TODO - don't think we need this
    mw.misc/add-content-type                ; Adds a Content-Type header for any response that doesn't already have one
    mw.misc/wrap-gzip                       ; GZIP response if client can handle it
    ))

src/metabase/middleware/json.clj

@@ -4,6 +4,7 @@
              [core :as json]
              [generate :refer [add-encoder encode-str]]]
             [metabase.util :as u]
+            [metabase.util.i18n :refer [tru]]
             [ring.middleware.json :as ring.json]
             [ring.util
              [io :as rui]
@@ -62,6 +63,29 @@
         (respond ring.json/default-malformed-response))
       (handler request respond raise))))
 
+(defn check-application-type-headers
+  "We don't support API requests with any type of content encoding other than JSON so let's be nice and make that
+  explicit. Added benefit is that it reduces CSRF surface because POSTing a form with JSON content encoding isn't so
+  easy to do."
+  [handler]
+  (fn
+    [{:keys [request-method body], {:strs [content-type]} :headers, :as request} respond raise]
+    ;; GET or DELETE requests with no body we can go ahead and proceed without Content-Type headers, since they
+    ;; generally don't have bodies.
+    ;;
+    ;; POST/PUT requests always require Content-Type: application/json. GET/DELETE requests that specify any other
+    ;; content type aren't allowed.
+    (if (or (and (#{:get :delete} request-method)
+                 (nil? content-type))
+            (#'ring.json/json-request? request))
+      (handler request respond raise)
+      (respond
+       {:status  400
+        :headers {"Content-Type" "text/plain"}
+        :body    (str (tru "Metabase only supports JSON requests.")
+                      " "
+                      (tru "Make sure you set a 'Content-Type: application/json' header."))}))))
+
 
 ;;; +----------------------------------------------------------------------------------------------------------------+
 ;;; |                                            Streaming JSON Responses                                            |

test/metabase/api/notify_test.clj

@@ -17,9 +17,11 @@
 ;; database must exist or we get a 404
 (expect
   {:status 404
-   :body "Not found."}
-  (try (client/post (http/build-url "notify/db/10000" {}) {:accept  :json
-                                                           :headers {"X-METABASE-APIKEY" "test-api-key"}})
+   :body   "Not found."}
+  (try (client/post (http/build-url "notify/db/10000" {})
+                    {:accept  :json
+                     :headers {"X-METABASE-APIKEY" "test-api-key"
+                               "Content-Type"      "application/json"}})
        (catch clojure.lang.ExceptionInfo e
          (select-keys (:object (ex-data e)) [:status :body]))))
 

test/metabase/http_client.clj

@@ -78,14 +78,15 @@
 ;;; client
 
 (defn- build-request-map [credentials http-body]
-  (cond-> {:accept  :json
-           :headers {"X-METABASE-SESSION" (when credentials
-                                            (if (map? credentials)
-                                              (authenticate credentials)
-                                              credentials))}}
-    (seq http-body) (assoc
-                      :content-type :json
-                      :body         (json/generate-string http-body))))
+  (merge
+   {:accept       :json
+    :headers      {"X-METABASE-SESSION" (when credentials
+                                          (if (map? credentials)
+                                            (authenticate credentials)
+                                            credentials))}
+    :content-type :json}
+   (when (seq http-body)
+     {:body (json/generate-string http-body)})))
 
 (defn- check-status-code
   "If an EXPECTED-STATUS-CODE was passed to the client, check that the actual status code matches, or throw an exception."

test/metabase/middleware/json_test.clj

 (ns metabase.middleware.json-test
   (:require [cheshire.core :as json]
-            [expectations :refer [expect]]))
+            [clj-http.client :as http]
+            [expectations :refer [expect]]
+            [metabase.http-client :as mb-http]
+            [metabase.test.data.users :as test-users])
+  (:import clojure.lang.ExceptionInfo))
 
 ;;; JSON encoding tests
 ;; Required here so so custom Cheshire encoders are loaded
@@ -11,3 +15,16 @@
   "{\"my-bytes\":\"0xC42360D7\"}"
   (json/generate-string {:my-bytes (byte-array [196 35  96 215  8 106 108 248 183 215 244 143  17 160 53 186
                                                 213 30 116  25 87  31 123 172 207 108  47 107 191 215 76  92])}))
+
+;; Make sure we send you an informative error message if you try to send an API request without Content-Type:
+;; application/json headers
+(expect
+  {:body "Metabase only supports JSON requests. Make sure you set a Content-Type: application/json header."
+   :status 400}
+  (try
+    (http/post
+     (str mb-http/*url-prefix* (format "/user/%d/qbnewb" (test-users/user->id :crowberto)))
+     {:headers {"X-Metabase-Session" (mb-http/authenticate (test-users/user->credentials :crowberto))}})
+    (catch ExceptionInfo e
+      (let [response (:object (ex-data e))]
+        (select-keys response [:body :status])))))

test/metabase/test/data/users.clj

@@ -128,13 +128,21 @@
 
 (defonce ^:private tokens (atom {}))
 
-(defn- username->token [username]
+(defn username->token
+  "Return cached session token for a test User, logging in first if needed."
+  [username]
   (or (@tokens username)
       (u/prog1 (http/authenticate (user->credentials username))
         (swap! tokens assoc username <>))
       (throw (Exception. (format "Authentication failed for %s with credentials %s"
                                  username (user->credentials username))))))
 
+(defn clear-cached-session-tokens!
+  "Clear any cached session tokens, which may have expired or been removed. You should do this in the even you get a
+  `401` unauthenticated response, and then retry the request."
+  []
+  (reset! tokens {}))
+
 (defn- client-fn [username & args]
   (try
     (apply http/client (username->token username) args)
@@ -143,7 +151,7 @@
         (when-not (= status-code 401)
           (throw e))
         ;; If we got a 401 unauthenticated clear the tokens cache + recur
-        (reset! tokens {})
+        (clear-cached-session-tokens!)
         (apply client-fn username args)))))
 
 (defn user->client