Change HttpOnly cookie name -> metabase.SESSION

frontend/src/metabase/lib/cookies.js

-import { clearGoogleAuthCredentials } from "metabase/lib/auth";
-
 import Cookies from "js-cookie";
 
-export const METABASE_SESSION_COOKIE = "metabase.SESSION_ID";
+// METABASE_SESSION_COOKIE is only used for e2e tests. In normal usage cookie is set automatically by login endpoints
+export const METABASE_SESSION_COOKIE = "metabase.SESSION";
 export const METABASE_SEEN_ALERT_SPLASH_COOKIE = "metabase.SEEN_ALERT_SPLASH";
 
 // Handles management of Metabase cookie work
 let MetabaseCookies = {
-  // set the session cookie.  if sessionId is null, clears the cookie
-  setSessionCookie: function(sessionId) {
-    const options = {
-      path: window.MetabaseRoot || "/",
-      expires: 14,
-      secure: window.location.protocol === "https:",
-    };
-
-    try {
-      if (sessionId) {
-        // set a session cookie
-        Cookies.set(METABASE_SESSION_COOKIE, sessionId, options);
-      } else {
-        sessionId = Cookies.get(METABASE_SESSION_COOKIE);
-
-        // delete the current session cookie and Google Auth creds
-        Cookies.remove(METABASE_SESSION_COOKIE);
-        clearGoogleAuthCredentials();
-
-        return sessionId;
-      }
-    } catch (e) {
-      console.error("setSessionCookie:", e);
-    }
-  },
-
   setHasSeenAlertSplash: hasSeen => {
     const options = {
       path: window.MetabaseRoot || "/",

frontend/test/legacy-selenium/auth/login.spec.js

@@ -10,6 +10,8 @@ import {
   describeE2E,
 } from "../support/utils";
 
+import { METABASE_SESSION_COOKIE } from "metabase/lib/cookies";
+
 jasmine.DEFAULT_TIMEOUT_INTERVAL = 60000;
 
 describeE2E("auth/login", () => {
@@ -36,7 +38,7 @@ describeE2E("auth/login", () => {
       await waitForUrl(driver, `${server.host}/`);
       const sessionCookie = await driver
         .manage()
-        .getCookie("metabase.SESSION_ID");
+        .getCookie(METABASE_SESSION_COOKIE);
       sessionId = sessionCookie.value;
     });
 
@@ -55,7 +57,7 @@ describeE2E("auth/login", () => {
     beforeEach(async () => {
       await driver.get(`${server.host}/`);
       await driver.manage().deleteAllCookies();
-      await driver.manage().addCookie("metabase.SESSION_ID", sessionId);
+      await driver.manage().addCookie(METABASE_SESSION_COOKIE, sessionId);
     });
 
     it("is logged in", async () => {

src/metabase/middleware/session.clj

@@ -4,7 +4,8 @@
              [config :as config]
              [db :as mdb]
              [public-settings :as public-settings]]
-            [metabase.api.common :refer [*current-user* *current-user-id* *current-user-permissions-set* *is-superuser?*]]
+            [metabase.api.common :refer [*current-user* *current-user-id* *current-user-permissions-set*
+                                         *is-superuser?*]]
             [metabase.core.initialization-status :as init-status]
             [metabase.models
              [session :refer [Session]]
@@ -16,35 +17,63 @@
            java.util.UUID
            org.joda.time.DateTime))
 
-(def ^:private ^String metabase-session-cookie "metabase.SESSION_ID")
-(def ^:private ^String metabase-session-header "x-metabase-session")
+;; How do authenticated API requests work? Metabase first looks for a cookie called `metabase.SESSION`. This is the
+;; normal way of doing things; this cookie gets set automatically upon login. `metabase.SESSION` is an HttpOnly
+;; cookie and thus can't be viewed by FE code.
+;;
+;; If that cookie is isn't present, we look for the `metabase.SESSION_ID`, which is the old session cookie set in
+;; 0.31.x and older. Unlike `metabase.SESSION`, this cookie was set directly by the frontend and thus was not
+;; HttpOnly; for 0.32.x we'll continue to accept it rather than logging every one else out on upgrade. (We've
+;; switched to a new Cookie name for 0.32.x because the new cookie includes a `path` attribute, thus browsers consider
+;; it to be a different Cookie; Ring cookie middleware does not handle multiple cookies with the same name.)
+;;
+;; Finally we'll check for the presence of a `X-Metabase-Session` header. If that isn't present, you don't have a
+;; Session ID and thus are definitely not authenticated
+(def ^:private ^String metabase-session-cookie        "metabase.SESSION")
+(def ^:private ^String metabase-legacy-session-cookie "metabase.SESSION_ID") ; this can be removed in 0.33.x
+(def ^:private ^String metabase-session-header        "x-metabase-session")
+
+(defn- clear-cookie [response cookie-name]
+  (resp/set-cookie response cookie-name nil {:expires (DateTime. 0)}))
+
+(defn- wrap-body-if-needed
+  "You can't add a cookie (by setting the `:cookies` key of a response) if the response is an unwrapped JSON response;
+  wrap `response` if needed."
+  [response]
+  (if (and (map? response) (contains? response :body))
+    response
+    {:body response, :status 200}))
 
 (s/defn set-session-cookie
   "Add a `Set-Cookie` header to `response` to persist the Metabase session."
   [response, session-id :- UUID]
-  (if-not (and (map? response) (:body response))
-    (recur {:body response, :status 200} session-id)
-    (resp/set-cookie
-     response
-     metabase-session-cookie
-     (str session-id)
-     (merge
-      {:same-site :lax
-       :http-only true
-       :path      "/api"
-       :max-age   (config/config-int :max-session-age)}
-      ;; If Metabase is running over HTTPS (hopefully always except for local dev instances) then make sure to make this
-      ;; cookie HTTPS-only
-      (when (some-> (public-settings/site-url) URL. .getProtocol (= "https"))
-        {:secure true})))))
+  (-> response
+      wrap-body-if-needed
+      (clear-cookie metabase-legacy-session-cookie)
+      (resp/set-cookie
+       metabase-session-cookie
+       (str session-id)
+       (merge
+        {:same-site :lax
+         :http-only true
+         :path      "/api"
+         :max-age   (config/config-int :max-session-age)}
+        ;; If Metabase is running over HTTPS (hopefully always except for local dev instances) then make sure to
+        ;; make this cookie HTTPS-only
+        (when (some-> (public-settings/site-url) URL. .getProtocol (= "https"))
+          {:secure true})))))
 
 (defn clear-session-cookie
   "Add a header to `response` to clear the current Metabase session cookie."
   [response]
-  (resp/set-cookie response metabase-session-cookie nil {:expires (DateTime. 0)}))
+  (-> response
+      wrap-body-if-needed
+      (clear-cookie metabase-session-cookie)
+      (clear-cookie metabase-legacy-session-cookie)))
 
 (defn- wrap-session-id* [{:keys [cookies headers] :as request}]
   (let [session-id (or (get-in cookies [metabase-session-cookie :value])
+                       (get-in cookies [metabase-legacy-session-cookie :value])
                        (headers metabase-session-header))]
     (if (seq session-id)
       (assoc request :metabase-session-id session-id)