diff --git a/app/controllers/concerns/issuable_collections.rb b/app/controllers/concerns/issuable_collections.rb index 22b39f47bf0c977af4854ebd3e61825764029101..a2c96f5d63557f934049ab44d5cbaf35033020d5 100644 --- a/app/controllers/concerns/issuable_collections.rb +++ b/app/controllers/concerns/issuable_collections.rb @@ -1,5 +1,6 @@ module IssuableCollections extend ActiveSupport::Concern + include CookiesHelper include SortingHelper include Gitlab::IssuableMetadata include Gitlab::Utils::StrongMemoize @@ -107,11 +108,14 @@ module IssuableCollections end def set_sort_order_from_cookie - cookies[remember_sorting_key] = params[:sort] if params[:sort].present? + sort_param = params[:sort] if params[:sort].present? # fallback to legacy cookie value for backward compatibility - cookies[remember_sorting_key] ||= cookies['issuable_sort'] - cookies[remember_sorting_key] = update_cookie_value(cookies[remember_sorting_key]) - params[:sort] = cookies[remember_sorting_key] + sort_param ||= cookies['issuable_sort'] + sort_param ||= cookies[remember_sorting_key] + + sort_value = update_cookie_value(sort_param) + set_secure_cookie(remember_sorting_key, sort_value) + params[:sort] = sort_value end def remember_sorting_key diff --git a/app/controllers/projects/application_controller.rb b/app/controllers/projects/application_controller.rb index b4f814fd3a4f62d06966d86403c92a83e1397e74..695ffd90a852b1261e5a08092b726e0e4bbd9b62 100644 --- a/app/controllers/projects/application_controller.rb +++ b/app/controllers/projects/application_controller.rb @@ -1,4 +1,5 @@ class Projects::ApplicationController < ApplicationController + include CookiesHelper include RoutableActions include ChecksCollaboration @@ -74,7 +75,7 @@ class Projects::ApplicationController < ApplicationController end def apply_diff_view_cookie! - cookies.permanent[:diff_view] = params.delete(:view) if params[:view].present? + set_secure_cookie(:diff_view, params.delete(:view), permanent: true) if params[:view].present? end def require_pages_enabled! diff --git a/app/helpers/cookies_helper.rb b/app/helpers/cookies_helper.rb new file mode 100644 index 0000000000000000000000000000000000000000..3a7e99871900e2f7fbcbb3de26a195042fe853cb --- /dev/null +++ b/app/helpers/cookies_helper.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +module CookiesHelper + def set_secure_cookie(key, value, httponly: false, permanent: false) + cookie_jar = permanent ? cookies.permanent : cookies + + cookie_jar[key] = { value: value, secure: Gitlab.config.gitlab.https, httponly: httponly } + end +end diff --git a/changelogs/unreleased/sh-set-secure-cookies.yml b/changelogs/unreleased/sh-set-secure-cookies.yml new file mode 100644 index 0000000000000000000000000000000000000000..da741288b42c5d181e6c857e5c07e75296d00856 --- /dev/null +++ b/changelogs/unreleased/sh-set-secure-cookies.yml @@ -0,0 +1,5 @@ +--- +title: Set issuable_sort, diff_view, and perf_bar_enabled cookies to secure when possible +merge_request: 21442 +author: +type: security diff --git a/spec/controllers/concerns/issuable_collections_spec.rb b/spec/controllers/concerns/issuable_collections_spec.rb index c1f42bbb9d7aa167b8ccc4d690dfc36020af2dd0..d16a3464495149fbce5186abaae0f79198573860 100644 --- a/spec/controllers/concerns/issuable_collections_spec.rb +++ b/spec/controllers/concerns/issuable_collections_spec.rb @@ -21,6 +21,34 @@ describe IssuableCollections do controller end + describe '#set_set_order_from_cookie' do + describe 'when sort param given' do + let(:cookies) { {} } + let(:params) { { sort: 'downvotes_asc' } } + + it 'sets the cookie with the right values and flags' do + allow(controller).to receive(:cookies).and_return(cookies) + + controller.send(:set_sort_order_from_cookie) + + expect(cookies['issue_sort']).to eq({ value: 'popularity', secure: false, httponly: false }) + end + end + + describe 'when cookie exists' do + let(:cookies) { { 'issue_sort' => 'id_asc' } } + let(:params) { {} } + + it 'sets the cookie with the right values and flags' do + allow(controller).to receive(:cookies).and_return(cookies) + + controller.send(:set_sort_order_from_cookie) + + expect(cookies['issue_sort']).to eq({ value: 'created_asc', secure: false, httponly: false }) + end + end + end + describe '#page_count_for_relation' do let(:params) { { state: 'opened' } }