Fix: Setting a very high Session Timeout causes instance to stop working (#27347)

* Limit session timeout amount to less than 1 million

* Limit the max timeout to 100 years

* Validate on BE too

* Typo

* Add comment to FE

* Fix BE validation

* Fix BE validation

* Add BE test

* Change FE validation to mirror BE validation

* Add unit test for SessionTimeoutSetting

* Refactor FE unit tests

* Move comment

* Add check for positive amount to BE validation

* Add more BE tests

* Refactor validate function

* Remove cleanup()

* Use ToBeInTheDocument instead

* Use getByText

* Remove conditional expect

* Remove unused import

* Refactor for clarity

* Formatting

* Validate session-timeout in getter too

* Add docstring to check-session-timeout and make private

* Change getter to print warning string instead of throw exception

* Format import

* i18n and use log/warn in getter, and throw 400 in setter

* Reorder require

* Use cam's suggestion

Co-authored-by: Cam Saul <1455846+camsaul@users.noreply.github.com>

Co-authored-by: Cam Saul <1455846+camsaul@users.noreply.github.com>

enterprise/frontend/src/metabase-enterprise/auth/components/SessionTimeoutSetting/SessionTimeoutSetting.tsx

@@ -30,13 +30,21 @@ interface SessionTimeoutSettingProps {
   onChange: (value: TimeoutValue | null) => void;
 }
 
-const validate = (
-  setting: SessionTimeoutSettingProps["setting"],
-  value: TimeoutValue,
-) =>
-  setting.value == null || value.amount > 0
-    ? null
-    : t`Timeout must be greater than 0`;
+// This should mirror the BE validation of the session-timeout setting.
+const validate = (value: TimeoutValue) => {
+  if (value.amount <= 0) {
+    return t`Timeout must be greater than 0`;
+  }
+  // We need to limit the duration from being too large because
+  // the year of the expires date must be 4 digits (#25253)
+  const unitsPerDay = { hours: 24, minutes: 24 * 60 }[value.unit] as number;
+  const days = value.amount / unitsPerDay;
+  const daysIn100Years = 365.25 * 100;
+  if (days >= daysIn100Years) {
+    return t`Timeout must be less than 100 years`;
+  }
+  return null;
+};
 
 const SessionTimeoutSetting = ({
   setting,
@@ -48,7 +56,7 @@ const SessionTimeoutSetting = ({
     setValue(prev => ({ ...prev, ...newValue }));
   };
 
-  const error = validate(setting, value);
+  const error = validate(value);
 
   const handleCommitSettings = (value: TimeoutValue | null) => {
     !error && onChange(value);
@@ -80,6 +88,7 @@ const SessionTimeoutSetting = ({
           <SessionTimeoutInputContainer>
             <SessionTimeoutInput
               type="number"
+              data-testid="session-timeout-input"
               placeholder=""
               value={value?.amount.toString()}
               onChange={e =>

enterprise/frontend/src/metabase-enterprise/auth/components/SessionTimeoutSetting/SessionTimeoutSetting.unit.spec.tsx

+import React from "react";
+import { render, cleanup, screen } from "@testing-library/react";
+import SessionTimeoutSetting from "metabase-enterprise/auth/components/SessionTimeoutSetting";
+
+describe("SessionTimeoutSetting", () => {
+  beforeAll(() => {
+    window.HTMLElement.prototype.scrollIntoView = jest.fn();
+  });
+
+  const SUCCEED_TEST_CASES = [
+    { value: { amount: 1, unit: "minutes" } },
+    { value: { amount: 1, unit: "hours" } },
+    { value: { amount: 60 * 24 * 365.25 * 100 - 1, unit: "minutes" } },
+    { value: { amount: 24 * 365.25 * 100 - 1, unit: "hours" } },
+  ];
+
+  const FAIL_TEST_CASES = [
+    {
+      value: { amount: 0, unit: "minutes" },
+      error: "Timeout must be greater than 0",
+    },
+    {
+      value: { amount: 0, unit: "hours" },
+      error: "Timeout must be greater than 0",
+    },
+    {
+      value: { amount: 60 * 24 * 365.25 * 100, unit: "minutes" },
+      error: "Timeout must be less than 100 years",
+    },
+    {
+      value: { amount: 24 * 365.25 * 100, unit: "hours" },
+      error: "Timeout must be less than 100 years",
+    },
+  ];
+
+  SUCCEED_TEST_CASES.map(({ value }) => {
+    it(`validates ${value.amount} ${value.unit} correctly`, () => {
+      const setting = { value: value, key: "...", default: "..." };
+      render(<SessionTimeoutSetting setting={setting} onChange={jest.fn()} />);
+      expect(screen.queryByText(/Timeout must be/)).not.toBeInTheDocument();
+    });
+  });
+
+  FAIL_TEST_CASES.map(({ value, error }) => {
+    it(`validates ${value.amount} ${value.unit} correctly`, () => {
+      const setting = { value: value, key: "...", default: "..." };
+      render(<SessionTimeoutSetting setting={setting} onChange={jest.fn()} />);
+      expect(screen.getByText(error)).toBeInTheDocument();
+    });
+  });
+});

src/metabase/server/middleware/session.clj

@@ -28,7 +28,7 @@
    [metabase.public-settings.premium-features :as premium-features]
    [metabase.server.request.util :as request.u]
    [metabase.util :as u]
-   [metabase.util.i18n :as i18n :refer [deferred-trs deferred-tru tru]]
+   [metabase.util.i18n :as i18n :refer [deferred-trs deferred-tru trs tru]]
    [ring.util.response :as response]
    [schema.core :as s]
    [toucan.db :as db])
@@ -359,21 +359,52 @@
 ;;; |                                              reset-cookie-timeout                                             |
 ;;; +----------------------------------------------------------------------------------------------------------------+
 
+(defn- check-session-timeout
+  "Returns nil if the [[session-timeout]] value is valid. Otherwise returns an error key."
+  [timeout]
+  (when (some? timeout)
+    (let [{:keys [unit amount]} timeout
+          units-in-24-hours (case unit
+                              "seconds" (* 60 60 24)
+                              "minutes" (* 60 24)
+                              "hours"   24)
+          units-in-100-years (* units-in-24-hours 365.25 100)]
+      (cond
+        (not (pos? amount))
+        :amount-must-be-positive
+        (>= amount units-in-100-years)
+        :amount-must-be-less-than-100-years))))
+
 (defsetting session-timeout
   ;; Should be in the form {:amount 60 :unit "minutes"} where the unit is one of "seconds", "minutes" or "hours".
   ;; The amount is nillable.
   (deferred-tru "Time before inactive users are logged out. By default, sessions last indefinitely.")
-  :type       :json
-  :default    nil)
+  :type    :json
+  :default nil
+  :getter  (fn []
+             (let [value (setting/get-value-of-type :json :session-timeout)]
+               (if-let [error-key (check-session-timeout value)]
+                 (do (log/warn (case error-key
+                                 :amount-must-be-positive            (trs "Session timeout amount must be positive.")
+                                 :amount-must-be-less-than-100-years (trs "Session timeout must be less than 100 years.")))
+                     nil)
+                 value)))
+  :setter  (fn [new-value]
+             (when-let [error-key (check-session-timeout new-value)]
+               (throw (ex-info (case error-key
+                                 :amount-must-be-positive            (tru "Session timeout amount must be positive.")
+                                 :amount-must-be-less-than-100-years (tru "Session timeout must be less than 100 years."))
+                               {:status-code 400})))
+             (setting/set-value-of-type! :json :session-timeout new-value)))
 
 (defn session-timeout->seconds
-  "Convert a session timeout setting to seconds."
+  "Convert the session-timeout setting value to seconds."
   [{:keys [unit amount]}]
   (when amount
     (-> (case unit
           "seconds" amount
           "minutes" (* amount 60)
-          "hours"  (* amount 3600))
+          "hours"   (* amount 3600))
         (max 60)))) ; Ensure a minimum of 60 seconds so a user can't lock themselves out
 
 (defn session-timeout-seconds

test/metabase/server/middleware/session_test.clj

 (ns metabase.server.middleware.session-test
   (:require
+   [cheshire.core :as json]
    [clojure.string :as str]
    [clojure.test :refer :all]
    [environ.core :as env]
@@ -12,8 +13,7 @@
    [metabase.models :refer [PermissionsGroupMembership Session User]]
    [metabase.public-settings :as public-settings]
    [metabase.public-settings.premium-features :as premium-features]
-   [metabase.public-settings.premium-features-test
-    :as premium-features-test]
+   [metabase.public-settings.premium-features-test :as premium-features-test]
    [metabase.server.middleware.session :as mw.session]
    [metabase.test :as mt]
    [metabase.util.i18n :as i18n]
@@ -397,7 +397,57 @@
 
 ;;; ----------------------------------------------------- Session timeout -----------------------------------------------------
 
-(deftest session-timeout-tests
+(deftest session-timeout-validation-test
+  (testing "Setting the session timeout should fail if the timeout isn't positive"
+    (is (thrown-with-msg?
+         java.lang.Exception
+         #"Session timeout amount must be positive"
+         (mw.session/session-timeout! {:unit "hours", :amount 0})))
+    (is (thrown-with-msg?
+         java.lang.Exception
+         #"Session timeout amount must be positive"
+         (mw.session/session-timeout! {:unit "minutes", :amount -1}))))
+  (testing "Setting the session timeout should fail if the timeout is too large"
+    (is (thrown-with-msg?
+         java.lang.Exception
+         #"Session timeout must be less than 100 years"
+         (mw.session/session-timeout! {:unit "hours", :amount (* 100 365.25 24)})))
+    (is (thrown-with-msg?
+         java.lang.Exception
+         #"Session timeout must be less than 100 years"
+         (mw.session/session-timeout! {:unit "minutes", :amount (* 100 365.25 24 60)}))))
+  (testing "Setting the session timeout shouldn't fail if the timeout is between 0 and 100 years exclusive"
+    (is (some? (mw.session/session-timeout! {:unit "minutes", :amount 1})))
+    (is (some? (mw.session/session-timeout! {:unit "hours", :amount 1})))
+    (is (some? (mw.session/session-timeout! {:unit "minutes", :amount (dec (* 100 365.25 24 60))})))
+    (is (some? (mw.session/session-timeout! {:unit "hours", :amount (dec (* 100 365.25 24))}))))
+  (testing "Setting an invalid timeout via PUT /api/setting/:key endpoint should return a 400 status code"
+    (is (= "Session timeout amount must be positive."
+           (mt/user-http-request :crowberto :put 400 "setting/session-timeout" {:value {:unit "hours", :amount -1}})))))
+
+(deftest session-timeout-env-var-validation-test
+  (let [set-and-get (fn [timeout]
+                      (mt/with-temp-env-var-value [mb-session-timeout (json/generate-string timeout)]
+                        (mw.session/session-timeout)))]
+    (testing "Setting the session timeout with env var should work with valid timeouts"
+      (doseq [timeout [{:unit "hours", :amount 1}
+                       {:unit "hours", :amount (dec (* 100 365.25 24))}]]
+        (is (= timeout
+               (set-and-get timeout)))))
+    (testing "Setting the session timeout via the env var should fail if the timeout isn't positive"
+      (doseq [amount [0 -1]
+              :let [timeout {:unit "hours", :amount amount}]]
+        (is (nil? (set-and-get timeout)))
+        (is (= [[:warn nil "Session timeout amount must be positive."]]
+               (mt/with-log-messages-for-level :warn (set-and-get timeout))))))
+    (testing "Setting the session timeout via env var should fail if the timeout is too large"
+      (doseq [timeout [{:unit "hours", :amount (* 100 365.25 24)}
+                       {:unit "minutes", :amount (* 100 365.25 24 60)}]]
+        (is (nil? (set-and-get timeout)))
+        (is (= [[:warn nil "Session timeout must be less than 100 years."]]
+               (mt/with-log-messages-for-level :warn (set-and-get timeout))))))))
+
+(deftest session-timeout-test
   (let [request-time (t/zoned-date-time "2022-01-01T00:00:00.000Z")
         session-id   "8df268ab-00c0-4b40-9413-d66b966b696a"
         response     {:body    "some body",