diff options
author | Robert Speicher <robert@gitlab.com> | 2018-07-19 01:23:25 +0300 |
---|---|---|
committer | Robert Speicher <robert@gitlab.com> | 2018-07-19 01:23:25 +0300 |
commit | 98eccfc44c597ba14939659ca3b9150197129961 (patch) | |
tree | c8386e810e23e532df0c695316ac6f9c223f1ae7 | |
parent | 1df32177a8dfd0f1f948a48ee9cf87ba74f43417 (diff) | |
parent | c559c43dafb75005f5589c473729054845bb498b (diff) |
Merge branch 'sh-limit-unauthenticated-session-times' into 'master'
Limit the TTL for anonymous sessions to 1 hour
Closes #48101
See merge request gitlab-org/gitlab-ce!20700
-rw-r--r-- | app/controllers/application_controller.rb | 19 | ||||
-rw-r--r-- | changelogs/unreleased/sh-limit-unauthenticated-session-times.yml | 5 | ||||
-rw-r--r-- | config/initializers/1_settings.rb | 1 | ||||
-rw-r--r-- | spec/controllers/application_controller_spec.rb | 26 |
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 |