From 304552fd229d64f4845df9406b4d5a76edcf0158 Mon Sep 17 00:00:00 2001
From: Cal Herries <39073188+calherries@users.noreply.github.com>
Date: Thu, 5 Jan 2023 11:22:55 +0100
Subject: [PATCH] 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>
---
.../SessionTimeoutSetting.tsx | 25 ++++++---
.../SessionTimeoutSetting.unit.spec.tsx | 51 +++++++++++++++++
src/metabase/server/middleware/session.clj | 41 ++++++++++++--
.../server/middleware/session_test.clj | 56 ++++++++++++++++++-
4 files changed, 157 insertions(+), 16 deletions(-)
create mode 100644 enterprise/frontend/src/metabase-enterprise/auth/components/SessionTimeoutSetting/SessionTimeoutSetting.unit.spec.tsx
diff --git a/enterprise/frontend/src/metabase-enterprise/auth/components/SessionTimeoutSetting/SessionTimeoutSetting.tsx b/enterprise/frontend/src/metabase-enterprise/auth/components/SessionTimeoutSetting/SessionTimeoutSetting.tsx
index 7cd23083c0e..883acdee4ca 100644
--- a/enterprise/frontend/src/metabase-enterprise/auth/components/SessionTimeoutSetting/SessionTimeoutSetting.tsx
+++ b/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 =>
diff --git a/enterprise/frontend/src/metabase-enterprise/auth/components/SessionTimeoutSetting/SessionTimeoutSetting.unit.spec.tsx b/enterprise/frontend/src/metabase-enterprise/auth/components/SessionTimeoutSetting/SessionTimeoutSetting.unit.spec.tsx
new file mode 100644
index 00000000000..712c0e0fe90
--- /dev/null
+++ b/enterprise/frontend/src/metabase-enterprise/auth/components/SessionTimeoutSetting/SessionTimeoutSetting.unit.spec.tsx
@@ -0,0 +1,51 @@
+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();
+ });
+ });
+});
diff --git a/src/metabase/server/middleware/session.clj b/src/metabase/server/middleware/session.clj
index 333679e0b57..8784a6c9f69 100644
--- a/src/metabase/server/middleware/session.clj
+++ b/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
diff --git a/test/metabase/server/middleware/session_test.clj b/test/metabase/server/middleware/session_test.clj
index f3fd325bcb8..c6be49b1f4c 100644
--- a/test/metabase/server/middleware/session_test.clj
+++ b/test/metabase/server/middleware/session_test.clj
@@ -1,5 +1,6 @@
(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",
--
GitLab