Support new MB_SESSION_COOKIES=true option

src/metabase/middleware/session.clj

@@ -84,9 +84,14 @@
        (merge
         {:same-site :lax
          :http-only true
-         :path      "/"
-         ;; max-session age-is in minutes; Max-Age= directive should be in seconds
-         :max-age   (* 60 (config/config-int :max-session-age))}
+         :path      "/"}
+        ;; If the env var `MB_SESSION_COOKIES=true`, do not set the `Max-Age` directive; cookies with no `Max-Age` and
+        ;; no `Expires` directives are session cookies, and are deleted when the browser is closed
+        ;;
+        ;; See https://developer.mozilla.org/en-US/docs/Web/HTTP/Cookies#Session_cookies
+        (when-not (config/config-bool :mb-session-cookies)
+          ;; max-session age-is in minutes; Max-Age= directive should be in seconds
+          {: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 (https-request? request)

test/metabase/middleware/session_test.clj

 (ns metabase.middleware.session-test
-  (:require [expectations :refer [expect]]
+  (:require [environ.core :as env]
+            [expectations :refer [expect]]
             [metabase.api.common :refer [*current-user* *current-user-id*]]
             [metabase.middleware.session :as mw.session]
             [metabase.test.data.users :as test-users]
-            [ring.mock.request :as mock]))
+            [ring.mock.request :as mock])
+  (:import java.util.UUID
+           org.joda.time.DateTime))
+
+;;; ----------------------------------------------- set-session-cookie -----------------------------------------------
+
+;; let's see whether we can set a Session cookie using the default options
+(let [uuid (UUID/randomUUID)]
+  (expect
+    ;; should unset the old SESSION_ID if it's present
+    {"metabase.SESSION_ID"
+     {:value   nil
+      :expires (DateTime. 0)
+      :path    "/"}
+     "metabase.SESSION"
+     {:value     (str uuid)
+      :same-site :lax
+      :http-only true
+      :path      "/"
+      :max-age   1209600}}
+    (-> (mw.session/set-session-cookie {} {} uuid)
+        :cookies)))
+
+;; if `MB_SESSION_COOKIES=true` we shouldn't set a `Max-Age`
+(let [uuid (UUID/randomUUID)]
+  (expect
+    {:value     (str uuid)
+     :same-site :lax
+     :http-only true
+     :path      "/"}
+    (let [env env/env]
+      (with-redefs [env/env (assoc env :mb-session-cookies "true")]
+        (-> (mw.session/set-session-cookie {} {} uuid)
+            (get-in [:cookies "metabase.SESSION"]))))))
+
+;; if request is an HTTPS request then we should set `:secure true`. There are several different headers we check for
+;; this. Make sure they all work.
+(defn- secure-cookie-for-headers? [headers]
+  (-> (mw.session/set-session-cookie {:headers headers} {} (UUID/randomUUID))
+      (get-in [:cookies "metabase.SESSION" :secure])
+      boolean))
+
+(expect true  (secure-cookie-for-headers? {"x-forwarded-proto" "https"}))
+(expect false (secure-cookie-for-headers? {"x-forwarded-proto" "http"}))
+
+(expect true  (secure-cookie-for-headers? {"x-forwarded-protocol" "https"}))
+(expect false (secure-cookie-for-headers? {"x-forwarded-protocol" "http"}))
+
+(expect true  (secure-cookie-for-headers? {"x-url-scheme" "https"}))
+(expect false (secure-cookie-for-headers? {"x-url-scheme" "http"}))
+
+(expect true  (secure-cookie-for-headers? {"x-forwarded-ssl" "on"}))
+(expect false (secure-cookie-for-headers? {"x-forwarded-ssl" "off"}))
+
+(expect true  (secure-cookie-for-headers? {"front-end-https" "on"}))
+(expect false (secure-cookie-for-headers? {"front-end-https" "off"}))
+
+(expect true  (secure-cookie-for-headers? {"origin" "https://mysite.com"}))
+(expect false (secure-cookie-for-headers? {"origin" "http://mysite.com"}))
+
 
 ;;; ---------------------------------------- TEST wrap-session-id middleware -----------------------------------------