Merge branch 'security-remove-caching-from-api-project-raw-endpoint' into 'master'

Disable caching on API project/raw endpoint

See merge request gitlab-org/security/gitlab!49

app/controllers/application_controller.rb

@@ -5,6 +5,7 @@ require 'fogbugz'
 
 class ApplicationController < ActionController::Base
   include Gitlab::GonHelper
+  include Gitlab::NoCacheHeaders
   include GitlabRoutingHelper
   include PageLayoutHelper
   include SafeParamsHelper
@@ -55,7 +56,6 @@ class ApplicationController < ActionController::Base
   # Adds `no-store` to the DEFAULT_CACHE_CONTROL, to prevent security
   # concerns due to caching private data.
   DEFAULT_GITLAB_CACHE_CONTROL = "#{ActionDispatch::Http::Cache::Response::DEFAULT_CACHE_CONTROL}, no-store"
-  DEFAULT_GITLAB_CONTROL_NO_CACHE = "#{DEFAULT_GITLAB_CACHE_CONTROL}, no-cache"
 
   rescue_from Encoding::CompatibilityError do |exception|
     log_exception(exception)
@@ -247,9 +247,9 @@ class ApplicationController < ActionController::Base
   end
 
   def no_cache_headers
-    headers['Cache-Control'] = DEFAULT_GITLAB_CONTROL_NO_CACHE
-    headers['Pragma'] = 'no-cache' # HTTP 1.0 compatibility
-    headers['Expires'] = 'Fri, 01 Jan 1990 00:00:00 GMT'
+    DEFAULT_GITLAB_NO_CACHE_HEADERS.each do |k, v|
+      headers[k] = v
+    end
   end
 
   def default_headers

changelogs/unreleased/security-remove-caching-from-api-project-raw-endpoint.yml

+---
+title: Disable caching of repository/files/:file_path/raw API endpoint
+merge_request:
+author:
+type: security

lib/api/files.rb

@@ -127,6 +127,7 @@ module API
       get ":id/repository/files/:file_path/raw", requirements: FILE_ENDPOINT_REQUIREMENTS do
         assign_file_vars!
 
+        no_cache_headers
         set_http_headers(blob_data)
 
         send_git_blob @repo, @blob

lib/api/helpers/headers_helpers.rb

@@ -3,6 +3,8 @@
 module API
   module Helpers
     module HeadersHelpers
+      include Gitlab::NoCacheHeaders
+
       def set_http_headers(header_data)
         header_data.each do |key, value|
           if value.is_a?(Enumerable)
@@ -12,6 +14,12 @@ module API
           header "X-Gitlab-#{key.to_s.split('_').collect(&:capitalize).join('-')}", value.to_s
         end
       end
+
+      def no_cache_headers
+        DEFAULT_GITLAB_NO_CACHE_HEADERS.each do |k, v|
+          header k, v
+        end
+      end
     end
   end
 end

lib/gitlab/no_cache_headers.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module NoCacheHeaders
+    DEFAULT_GITLAB_NO_CACHE_HEADERS = {
+      'Cache-Control' => "#{ActionDispatch::Http::Cache::Response::DEFAULT_CACHE_CONTROL}, no-store, no-cache",
+      'Pragma'        => 'no-cache', # HTTP 1.0 compatibility
+      'Expires'       => 'Fri, 01 Jan 1990 00:00:00 GMT'
+    }.freeze
+
+    def no_cache_headers
+      raise "#no_cache_headers is not implemented for this object"
+    end
+  end
+end

qa/qa/specs/features/api/3_create/repository/files_spec.rb

@@ -59,5 +59,48 @@ module QA
         a_hash_including(message: '202 Accepted')
       )
     end
+
+    describe 'raw file access' do
+      let(:svg_file) do
+        <<-SVG
+          <?xml version="1.0" standalone="no"?>
+          <!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">
+
+          <svg version="1.1" baseProfile="full" xmlns="http://www.w3.org/2000/svg">
+            <polygon id="triangle" points="0,0 0,50 50,0" fill="#009900" stroke="#004400"/>
+            <script type="text/javascript">
+               alert("surprise");
+            </script>
+          </svg>
+        SVG
+      end
+
+      it 'sets no-cache headers as expected' do
+        create_project_request = Runtime::API::Request.new(@api_client, '/projects')
+        post create_project_request.url, path: project_name, name: project_name
+
+        create_file_request = Runtime::API::Request.new(@api_client, "/projects/#{sanitized_project_path}/repository/files/test.svg")
+        post create_file_request.url, branch: 'master', content: svg_file, commit_message: 'Add test.svg'
+
+        get_file_request = Runtime::API::Request.new(@api_client, "/projects/#{sanitized_project_path}/repository/files/test.svg/raw", ref: 'master')
+
+        3.times do
+          response = get get_file_request.url
+
+          # Subsequent responses aren't cached, so headers should match from
+          #   request to request, especially a 200 response rather than a 304
+          #   (indicating a cached response.) Further, :content_disposition
+          #   should include `attachment` for all responses.
+          #
+          expect(response.headers[:cache_control]).to include("no-store")
+          expect(response.headers[:cache_control]).to include("no-cache")
+          expect(response.headers[:pragma]).to eq("no-cache")
+          expect(response.headers[:expires]).to eq("Fri, 01 Jan 1990 00:00:00 GMT")
+          expect(response.headers[:content_disposition]).to include("attachment")
+          expect(response.headers[:content_disposition]).not_to include("inline")
+          expect(response.headers[:content_type]).to include("image/svg+xml")
+        end
+      end
+    end
   end
 end

spec/lib/gitlab/no_cache_headers_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::NoCacheHeaders do
+  class NoCacheTester
+    include Gitlab::NoCacheHeaders
+  end
+
+  describe "#no_cache_headers" do
+    subject { NoCacheTester.new }
+
+    it "raises a RuntimeError" do
+      expect { subject.no_cache_headers }.to raise_error(RuntimeError)
+    end
+  end
+end

spec/requests/api/files_spec.rb

@@ -447,6 +447,18 @@ describe API::Files do
         expect(response).to have_gitlab_http_status(200)
       end
 
+      it 'sets no-cache headers' do
+        url = route('.gitignore') + "/raw"
+        expect(Gitlab::Workhorse).to receive(:send_git_blob)
+
+        get api(url, current_user), params: params
+
+        expect(response.headers["Cache-Control"]).to include("no-store")
+        expect(response.headers["Cache-Control"]).to include("no-cache")
+        expect(response.headers["Pragma"]).to eq("no-cache")
+        expect(response.headers["Expires"]).to eq("Fri, 01 Jan 1990 00:00:00 GMT")
+      end
+
       context 'when mandatory params are not given' do
         it_behaves_like '400 response' do
           let(:request) { get api(route("any%2Ffile"), current_user) }