Use SameSite=None for EMBEDDED_SESSION and DEVICE cookies (#18824)

* Use SameSite=None for EMBEDDED_SESSION and DEVICE cookies

 - EMBEDDED_SESSION previously did not specify SameSite attribute
   and assumed that the browser would default to SameSite=None, but
   recent browsers default to SameSite=Lax which does not work
   with cross-domain full-app embedding.
 - DEVICE was previously set to SameSite=Lax causing devices to not
   be remembered during login with cross-domain full-app embedding
   resulting in superfluous "We've Noticed a New Metabase Login"
   emails. Setting it to SameSite=None is safe because the cookie is
   not used to authenticate a user.

* Only print SameSite warning if not over https

src/metabase/server/middleware/browser_cookie.clj

@@ -4,6 +4,7 @@
   cookie is deleted, it's fine; the user will just get an email saying they logged in from a new device next time
   they log in."
   (:require [java-time :as t]
+            [metabase.server.request.util :as request.u]
             [metabase.util.schema :as su]
             [ring.util.response :as resp]
             [schema.core :as s])
@@ -11,16 +12,23 @@
 
 (def ^:private browser-id-cookie-name "metabase.DEVICE")
 
-;; This cookie doesn't need to be secure, because it's only used for notification purposes
-(def ^:private cookie-options
-  {:http-only true
-   :path      "/"
-   :same-site :lax
-   ;; Set the cookie to expire 20 years from now. That should be sufficient
-   :expires   (t/format :rfc-1123-date-time (t/plus (t/zoned-date-time) (t/years 20)))})
+;; This cookie doesn't need to be secure, because it's only used for notification purposes and cannot be used for
+;; CSRF as it is not a session cookie.
+;; However, we do need to make sure it's persisted/sent as much as possible to prevent superfluous login notification
+;; emails when used with full-app embedding, which means setting SameSite=None when possible (over HTTPS) and
+;; SameSite=Lax otherwise. (See #18553)
+(defn- cookie-options
+  [request]
+  (merge {:http-only true
+          :path      "/"
+          ;; Set the cookie to expire 20 years from now. That should be sufficient
+          :expires   (t/format :rfc-1123-date-time (t/plus (t/zoned-date-time) (t/years 20)))}
+         (if (request.u/https? request)
+           {:same-site :none, :secure true}
+           {:same-site :lax})))
 
-(s/defn ^:private add-browser-id-cookie [response browser-id :- su/NonBlankString]
-  (resp/set-cookie response browser-id-cookie-name browser-id cookie-options))
+(s/defn ^:private add-browser-id-cookie [request response browser-id :- su/NonBlankString]
+  (resp/set-cookie response browser-id-cookie-name browser-id (cookie-options request)))
 
 (defn ensure-browser-id-cookie
   "Set a permanent browser identifier cookie if one is not already set."
@@ -32,5 +40,5 @@
         (handler
          (assoc request :browser-id browser-id)
          (fn [response]
-           (respond (add-browser-id-cookie response browser-id)))
+           (respond (add-browser-id-cookie request response browser-id)))
          raise)))))

src/metabase/server/middleware/session.clj

@@ -74,6 +74,7 @@
 (s/defmethod set-session-cookie :normal
   [request response {session-uuid :id} :- {:id (s/cond-pre UUID u/uuid-regex), s/Keyword s/Any}]
   (let [response       (wrap-body-if-needed response)
+        is-https?      (request.u/https? request)
         cookie-options (merge
                         {:same-site config/mb-session-cookie-samesite
                          :http-only true
@@ -89,14 +90,13 @@
                           {:max-age (* 60 (config/config-int :max-session-age))})
                         ;; If the authentication request request was made over HTTPS (hopefully always except for
                         ;; local dev instances) add `Secure` attribute so the cookie is only sent over HTTPS.
-                        (when (request.u/https? request)
-                          {:secure true})
-                        (when (= config/mb-session-cookie-samesite :none)
-                          (log/warn
-                           (str (deferred-trs "Session cookie's SameSite is configured to \"None\", but site is")
-                                (deferred-trs "served over an insecure connection. Some browsers will reject ")
-                                (deferred-trs "cookies under these conditions. ")
-                                (deferred-trs "https://www.chromestatus.com/feature/5633521622188032")))))]
+                        (when is-https?
+                          {:secure true}))]
+    (when (and (= config/mb-session-cookie-samesite :none) (not is-https?))
+      (log/warn
+       (str (deferred-trs "Session cookie's SameSite is configured to \"None\", but site is served over an insecure connection. Some browsers will reject cookies under these conditions.")
+            " "
+            "https://www.chromestatus.com/feature/5633521622188032")))
     (resp/set-cookie response metabase-session-cookie (str session-uuid) cookie-options)))
 
 (s/defmethod set-session-cookie :full-app-embed
@@ -107,7 +107,12 @@
                         {:http-only true
                          :path      "/"}
                         (when (request.u/https? request)
-                          {:secure true}))]
+                          ;; SameSite=None is required for cross-domain full-app embedding. This is safe because
+                          ;; security is provided via anti-CSRF token. Note that most browsers will only accept
+                          ;; SameSite=None with secure cookies, thus we are setting it only over HTTPS to prevent
+                          ;; the cookie from being rejected in case of same-domain embedding.
+                          {:same-site :none
+                           :secure    true}))]
     (-> response
         (resp/set-cookie metabase-embedded-session-cookie (str session-uuid) cookie-options)
         (assoc-in [:headers anti-csrf-token-header] anti-csrf-token))))

test/metabase/server/middleware/browser_cookie_test.clj

+(ns metabase.server.middleware.browser-cookie-test
+  (:require [clojure.test :refer :all]
+            [metabase.server.middleware.browser-cookie :as mw.browser-cookie]
+            [ring.mock.request :as mock]
+            [ring.util.response :as response])
+  (:import java.util.UUID))
+
+(defn- handler [request]
+  ((mw.browser-cookie/ensure-browser-id-cookie
+    ;; Return ID in the body so we can verify
+    (fn [request respond _] (respond (response/response (str (:browser-id request))))))
+   request
+   identity
+   (fn [e] (throw e))))
+
+(def ^:private browser-id-cookie-name @#'mw.browser-cookie/browser-id-cookie-name)
+
+(def ^:private test-uuid #uuid "092797dd-a82a-4748-b393-697d7bb9ab65")
+
+(deftest existing-cookie
+  (testing "do not set DEVICE cookie if one is already present"
+    (let [request  (-> (mock/request :get "https://localhost/foo")
+                       (assoc :cookies {browser-id-cookie-name {:value test-uuid}}))
+          response (handler request)]
+      (is (= (str test-uuid) (:body response)))
+      (is (nil? (:cookies response))))))
+
+(deftest no-existing-cookie
+  (testing "set DEVICE cookie with SameSite=Lax if served over HTTP"
+    (let [request    (-> (mock/request :get "http://localhost/foo"))
+          response   (handler request)
+          browser-id (:body response)]
+      (is (some? (UUID/fromString browser-id)))
+      (is (= {:value     browser-id
+              :http-only true
+              :path      "/"
+              :same-site :lax}
+             (-> (get-in response [:cookies browser-id-cookie-name])
+                 (dissoc :expires))))))
+  (testing "set DEVICE cookie with SameSite=None if served over HTTPS"
+    (let [request    (-> (mock/request :get "https://localhost/foo"))
+          response   (handler request)
+          browser-id (:body response)]
+      (is (some? (UUID/fromString browser-id)))
+      (is (= {:value     browser-id
+              :http-only true
+              :path      "/"
+              :same-site :none
+              :secure    true}
+             (-> (get-in response [:cookies browser-id-cookie-name])
+                 (dissoc :expires)))))))

test/metabase/server/middleware/session_test.clj

@@ -134,7 +134,20 @@
                        :http-only true
                        :path      "/"}}
             :headers {anti-csrf-token-header test-anti-csrf-token}}
-           (mw.session/set-session-cookie {} {} test-full-app-embed-session)))))
+           (mw.session/set-session-cookie {} {} test-full-app-embed-session))))
+  (testing "test that we can set a full-app-embedding session cookie with SameSite=None over HTTPS"
+    (is (= {:body    {}
+            :status  200
+            :cookies {embedded-session-cookie
+                      {:value     "092797dd-a82a-4748-b393-697d7bb9ab65"
+                       :http-only true
+                       :path      "/"
+                       :same-site :none
+                       :secure    true}}
+            :headers {anti-csrf-token-header test-anti-csrf-token}}
+           (mw.session/set-session-cookie {:headers {"x-forwarded-protocol" "https"}}
+                                          {}
+                                          test-full-app-embed-session)))))
 
 
 ;;; ---------------------------------------- TEST wrap-session-id middleware -----------------------------------------