Merge branch 'security-fix/control-headers-11-2' into 'security-11-2'

: [11.2] Resolve "Sensitive information is stored in browser history"

See merge request gitlab/gitlabhq!2560

4 files changed, 65 insertions, 7 deletions

diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index e5b38898a67..4daa76116e2 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -40,6 +40,8 @@ class ApplicationController < ActionController::Base
     :git_import_enabled?, :gitlab_project_import_enabled?,
     :manifest_import_enabled?
 
+  DEFAULT_GITLAB_CACHE_CONTROL = "#{ActionDispatch::Http::Cache::Response::DEFAULT_CACHE_CONTROL}, no-store".freeze
+
   rescue_from Encoding::CompatibilityError do |exception|
     log_exception(exception)
     render "errors/encoding", layout: "errors", status: 500
@@ -229,6 +231,13 @@ class ApplicationController < ActionController::Base
     headers['X-XSS-Protection'] = '1; mode=block'
     headers['X-UA-Compatible'] = 'IE=edge'
     headers['X-Content-Type-Options'] = 'nosniff'
+
+    if current_user
+      # Adds `no-store` to the DEFAULT_CACHE_CONTROL, to prevent security
+      # concerns due to caching private data.
+      headers['Cache-Control'] = DEFAULT_GITLAB_CACHE_CONTROL
+      headers["Pragma"] = "no-cache" # HTTP 1.0 compatibility
+    end
   end
 
   def validate_user_service_ticket!
diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb
index fbf116e533b..13432adb5af 100644
--- a/spec/controllers/application_controller_spec.rb
+++ b/spec/controllers/application_controller_spec.rb
@@ -694,4 +694,30 @@ describe ApplicationController do
       expect(response).to have_gitlab_http_status(403)
     end
   end
+
+  context 'control headers' do
+    controller(described_class) do
+      def index
+        render json: :ok
+      end
+    end
+
+    context 'user not logged in' do
+      it 'sets the default headers' do
+        get :index
+
+        expect(response.headers['Cache-Control']).to be_nil
+      end
+    end
+
+    context 'user logged in' do
+      it 'sets the default headers' do
+        sign_in(user)
+
+        get :index
+
+        expect(response.headers['Cache-Control']).to eq 'max-age=0, private, must-revalidate, no-store'
+      end
+    end
+  end
 end
diff --git a/spec/controllers/uploads_controller_spec.rb b/spec/controllers/uploads_controller_spec.rb
index bcf289f36a9..f7af482b508 100644
--- a/spec/controllers/uploads_controller_spec.rb
+++ b/spec/controllers/uploads_controller_spec.rb
@@ -5,6 +5,13 @@ shared_examples 'content not cached without revalidation' do
   end
 end
 
+shared_examples 'content not cached without revalidation and no-store' do
+  it 'ensures content will not be cached without revalidation' do
+    # Fixed in newer versions of ActivePack, it will only output a single `private`.
+    expect(subject['Cache-Control']).to eq('max-age=0, private, must-revalidate, private, no-store')
+  end
+end
+
 describe UploadsController do
   let!(:user) { create(:user, avatar: fixture_file_upload("spec/fixtures/dk.png", "image/png")) }
 
@@ -177,7 +184,7 @@ describe UploadsController do
             expect(response).to have_gitlab_http_status(200)
           end
 
-          it_behaves_like 'content not cached without revalidation' do
+          it_behaves_like 'content not cached without revalidation and no-store' do
             subject do
               get :show, model: 'user', mounted_as: 'avatar', id: user.id, filename: 'dk.png'
 
@@ -239,7 +246,7 @@ describe UploadsController do
             expect(response).to have_gitlab_http_status(200)
           end
 
-          it_behaves_like 'content not cached without revalidation' do
+          it_behaves_like 'content not cached without revalidation and no-store' do
             subject do
               get :show, model: 'project', mounted_as: 'avatar', id: project.id, filename: 'dk.png'
 
@@ -292,7 +299,7 @@ describe UploadsController do
                 expect(response).to have_gitlab_http_status(200)
               end
 
-              it_behaves_like 'content not cached without revalidation' do
+              it_behaves_like 'content not cached without revalidation and no-store' do
                 subject do
                   get :show, model: 'project', mounted_as: 'avatar', id: project.id, filename: 'dk.png'
 
@@ -344,7 +351,7 @@ describe UploadsController do
             expect(response).to have_gitlab_http_status(200)
           end
 
-          it_behaves_like 'content not cached without revalidation' do
+          it_behaves_like 'content not cached without revalidation and no-store' do
             subject do
               get :show, model: 'group', mounted_as: 'avatar', id: group.id, filename: 'dk.png'
 
@@ -388,7 +395,7 @@ describe UploadsController do
                 expect(response).to have_gitlab_http_status(200)
               end
 
-              it_behaves_like 'content not cached without revalidation' do
+              it_behaves_like 'content not cached without revalidation and no-store' do
                 subject do
                   get :show, model: 'group', mounted_as: 'avatar', id: group.id, filename: 'dk.png'
 
@@ -445,7 +452,7 @@ describe UploadsController do
             expect(response).to have_gitlab_http_status(200)
           end
 
-          it_behaves_like 'content not cached without revalidation' do
+          it_behaves_like 'content not cached without revalidation and no-store' do
             subject do
               get :show, model: 'note', mounted_as: 'attachment', id: note.id, filename: 'dk.png'
 
@@ -498,7 +505,7 @@ describe UploadsController do
                 expect(response).to have_gitlab_http_status(200)
               end
 
-              it_behaves_like 'content not cached without revalidation' do
+              it_behaves_like 'content not cached without revalidation and no-store' do
                 subject do
                   get :show, model: 'note', mounted_as: 'attachment', id: note.id, filename: 'dk.png'
 
diff --git a/spec/features/projects_spec.rb b/spec/features/projects_spec.rb
index 56ed0c936a6..1e9613e1866 100644
--- a/spec/features/projects_spec.rb
+++ b/spec/features/projects_spec.rb
@@ -257,6 +257,22 @@ describe 'Project' do
     end
   end
 
+  context 'content is not cached after signing out', :js do
+    let(:user) { create(:user, project_view: 'activity') }
+    let(:project) { create(:project, :repository) }
+
+    it 'does not load activity', :js do
+      project.add_maintainer(user)
+      sign_in(user)
+      visit project_path(project)
+      sign_out(user)
+
+      page.evaluate_script('window.history.back()')
+
+      expect(page).not_to have_selector('.event-item')
+    end
+  end
+
   def remove_with_confirm(button_text, confirm_with)
     click_button button_text
     fill_in 'confirm_name_input', with: confirm_with