diff --git a/src/metabase/middleware/session.clj b/src/metabase/middleware/session.clj index b45d0d57d2612382b7eee2fcb0d7e9d952d4059c..540a655f70cfb7ac41c83e125bee7a9c480fa270 100644 --- a/src/metabase/middleware/session.clj +++ b/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) diff --git a/test/metabase/middleware/session_test.clj b/test/metabase/middleware/session_test.clj index 7d4fa890f4308f101da4e2eab3d17e9d3a52ef38..11423c54cd947a76e6aee3ead0e75bab4917db9a 100644 --- a/test/metabase/middleware/session_test.clj +++ b/test/metabase/middleware/session_test.clj @@ -1,9 +1,69 @@ (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 -----------------------------------------