Commit c559c43d authored by Stan Hu's avatar Stan Hu

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
parent 9bdc9b1a
...@@ -11,6 +11,7 @@ class ApplicationController < ActionController::Base ...@@ -11,6 +11,7 @@ class ApplicationController < ActionController::Base
include EnforcesTwoFactorAuthentication include EnforcesTwoFactorAuthentication
include WithPerformanceBar include WithPerformanceBar
before_action :limit_unauthenticated_session_times
before_action :authenticate_sessionless_user! before_action :authenticate_sessionless_user!
before_action :authenticate_user! before_action :authenticate_user!
before_action :enforce_terms!, if: :should_enforce_terms? before_action :enforce_terms!, if: :should_enforce_terms?
...@@ -85,6 +86,24 @@ class ApplicationController < ActionController::Base ...@@ -85,6 +86,24 @@ class ApplicationController < ActionController::Base
end end
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 protected
def append_info_to_payload(payload) def append_info_to_payload(payload)
......
---
title: Limit the TTL for anonymous sessions to 1 hour
merge_request: 20700
author:
type: performance
...@@ -140,6 +140,7 @@ Settings.gitlab['default_projects_features'] ||= {} ...@@ -140,6 +140,7 @@ Settings.gitlab['default_projects_features'] ||= {}
Settings.gitlab['webhook_timeout'] ||= 10 Settings.gitlab['webhook_timeout'] ||= 10
Settings.gitlab['max_attachment_size'] ||= 10 Settings.gitlab['max_attachment_size'] ||= 10
Settings.gitlab['session_expire_delay'] ||= 10080 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['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['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? Settings.gitlab.default_projects_features['wiki'] = true if Settings.gitlab.default_projects_features['wiki'].nil?
......
...@@ -89,6 +89,32 @@ describe ApplicationController do ...@@ -89,6 +89,32 @@ describe ApplicationController do
end end
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 describe 'rescue from Gitlab::Git::Storage::Inaccessible' do
controller(described_class) do controller(described_class) do
def index def index
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment