summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorStan Hu <stanhu@gmail.com>2018-07-18 11:18:14 -0700
committerStan Hu <stanhu@gmail.com>2018-07-18 12:39:51 -0700
commitc559c43dafb75005f5589c473729054845bb498b (patch)
tree590ffab29094fa7a64f8c1e0cc14552b76a2876a
parent9bdc9b1ae69a62ad764d8ae59baa43a4a0be1d3a (diff)
downloadgitlab-ce-c559c43dafb75005f5589c473729054845bb498b.tar.gz
Limit the TTL for anonymous sessions to 1 hour
By default, all sessions are given the same expiration time configured in the session store (e.g. 1 week). However, unauthenticated users can generate a lot of sessions, primarily for CSRF verification. It makes sense to reduce the TTL for unauthenticated to something much lower than the default (e.g. 1 hour) to limit Redis memory. In addition, Rails creates a new session after login, so the short TTL doesn't even need to be extended. Closes #48101
-rw-r--r--app/controllers/application_controller.rb19
-rw-r--r--changelogs/unreleased/sh-limit-unauthenticated-session-times.yml5
-rw-r--r--config/initializers/1_settings.rb1
-rw-r--r--spec/controllers/application_controller_spec.rb26
4 files changed, 51 insertions, 0 deletions
diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index f45fcd4d900..eeceb99c8d2 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -11,6 +11,7 @@ class ApplicationController < ActionController::Base
include EnforcesTwoFactorAuthentication
include WithPerformanceBar
+ before_action :limit_unauthenticated_session_times
before_action :authenticate_sessionless_user!
before_action :authenticate_user!
before_action :enforce_terms!, if: :should_enforce_terms?
@@ -85,6 +86,24 @@ class ApplicationController < ActionController::Base
end
end
+ # By default, all sessions are given the same expiration time configured in
+ # the session store (e.g. 1 week). However, unauthenticated users can
+ # generate a lot of sessions, primarily for CSRF verification. It makes
+ # sense to reduce the TTL for unauthenticated to something much lower than
+ # the default (e.g. 1 hour) to limit Redis memory. In addition, Rails
+ # creates a new session after login, so the short TTL doesn't even need to
+ # be extended.
+ def limit_unauthenticated_session_times
+ return if current_user
+
+ # Rack sets this header, but not all tests may have it: https://github.com/rack/rack/blob/fdcd03a3c5a1c51d1f96fc97f9dfa1a9deac0c77/lib/rack/session/abstract/id.rb#L251-L259
+ return unless request.env['rack.session.options']
+
+ # This works because Rack uses these options every time a request is handled:
+ # https://github.com/rack/rack/blob/fdcd03a3c5a1c51d1f96fc97f9dfa1a9deac0c77/lib/rack/session/abstract/id.rb#L342
+ request.env['rack.session.options'][:expire_after] = Settings.gitlab['unauthenticated_session_expire_delay']
+ end
+
protected
def append_info_to_payload(payload)
diff --git a/changelogs/unreleased/sh-limit-unauthenticated-session-times.yml b/changelogs/unreleased/sh-limit-unauthenticated-session-times.yml
new file mode 100644
index 00000000000..44a46b4115e
--- /dev/null
+++ b/changelogs/unreleased/sh-limit-unauthenticated-session-times.yml
@@ -0,0 +1,5 @@
+---
+title: Limit the TTL for anonymous sessions to 1 hour
+merge_request: 20700
+author:
+type: performance
diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb
index 4b9cc59ec45..44bc72a7185 100644
--- a/config/initializers/1_settings.rb
+++ b/config/initializers/1_settings.rb
@@ -140,6 +140,7 @@ Settings.gitlab['default_projects_features'] ||= {}
Settings.gitlab['webhook_timeout'] ||= 10
Settings.gitlab['max_attachment_size'] ||= 10
Settings.gitlab['session_expire_delay'] ||= 10080
+Settings.gitlab['unauthenticated_session_expire_delay'] ||= 1.hour.to_i
Settings.gitlab.default_projects_features['issues'] = true if Settings.gitlab.default_projects_features['issues'].nil?
Settings.gitlab.default_projects_features['merge_requests'] = true if Settings.gitlab.default_projects_features['merge_requests'].nil?
Settings.gitlab.default_projects_features['wiki'] = true if Settings.gitlab.default_projects_features['wiki'].nil?
diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb
index 74f362fd7fc..f1165c73847 100644
--- a/spec/controllers/application_controller_spec.rb
+++ b/spec/controllers/application_controller_spec.rb
@@ -89,6 +89,32 @@ describe ApplicationController do
end
end
+ describe 'session expiration' do
+ controller(described_class) do
+ def index
+ render text: 'authenticated'
+ end
+ end
+
+ context 'authenticated user' do
+ it 'does not set the expire_after option' do
+ sign_in(create(:user))
+
+ get :index
+
+ expect(request.env['rack.session.options'][:expire_after]).to be_nil
+ end
+ end
+
+ context 'unauthenticated user' do
+ it 'sets the expire_after option' do
+ get :index
+
+ expect(request.env['rack.session.options'][:expire_after]).to eq(Settings.gitlab['unauthenticated_session_expire_delay'])
+ end
+ end
+ end
+
describe 'rescue from Gitlab::Git::Storage::Inaccessible' do
controller(described_class) do
def index