Merge branch 'security-12-1-enable-image-proxy' into '12-1-stable'

Use image proxy to mitigate stealing ip addresses

See merge request gitlab/gitlabhq!3231

10 files changed, 323 insertions, 0 deletions

diff --git a/spec/initializers/asset_proxy_setting_spec.rb b/spec/initializers/asset_proxy_setting_spec.rb
new file mode 100644
index 00000000000..42e4d4aa594
--- /dev/null
+++ b/spec/initializers/asset_proxy_setting_spec.rb
@@ -0,0 +1,13 @@
+require 'spec_helper'
+
+describe 'Asset proxy settings initialization' do
+  describe '#asset_proxy' do
+    it 'defaults to disabled' do
+      expect(Banzai::Filter::AssetProxyFilter).to receive(:initialize_settings)
+
+      require_relative '../../config/initializers/asset_proxy_settings'
+
+      expect(Gitlab.config.asset_proxy.enabled).to be_falsey
+    end
+  end
+end
diff --git a/spec/lib/banzai/filter/asset_proxy_filter_spec.rb b/spec/lib/banzai/filter/asset_proxy_filter_spec.rb
new file mode 100644
index 00000000000..b7f45421b2a
--- /dev/null
+++ b/spec/lib/banzai/filter/asset_proxy_filter_spec.rb
@@ -0,0 +1,95 @@
+require 'spec_helper'
+
+describe Banzai::Filter::AssetProxyFilter do
+  include FilterSpecHelper
+
+  def image(path)
+    %(<img src="#{path}" />)
+  end
+
+  it 'does not replace if disabled' do
+    stub_asset_proxy_setting(enabled: false)
+
+    context = described_class.transform_context({})
+    src     = 'http://example.com/test.png'
+    doc     = filter(image(src), context)
+
+    expect(doc.at_css('img')['src']).to eq src
+  end
+
+  context 'during initialization' do
+    after do
+      Gitlab.config.asset_proxy['enabled'] = false
+    end
+
+    it '#initialize_settings' do
+      stub_application_setting(asset_proxy_enabled: true)
+      stub_application_setting(asset_proxy_secret_key: 'shared-secret')
+      stub_application_setting(asset_proxy_url: 'https://assets.example.com')
+      stub_application_setting(asset_proxy_whitelist: %w(gitlab.com *.mydomain.com))
+
+      described_class.initialize_settings
+
+      expect(Gitlab.config.asset_proxy.enabled).to be_truthy
+      expect(Gitlab.config.asset_proxy.secret_key).to eq 'shared-secret'
+      expect(Gitlab.config.asset_proxy.url).to eq 'https://assets.example.com'
+      expect(Gitlab.config.asset_proxy.whitelist).to eq %w(gitlab.com *.mydomain.com)
+      expect(Gitlab.config.asset_proxy.domain_regexp).to eq /^(gitlab\.com|.*?\.mydomain\.com)$/i
+    end
+  end
+
+  context 'when properly configured' do
+    before do
+      stub_asset_proxy_setting(enabled: true)
+      stub_asset_proxy_setting(secret_key: 'shared-secret')
+      stub_asset_proxy_setting(url: 'https://assets.example.com')
+      stub_asset_proxy_setting(whitelist: %W(gitlab.com *.mydomain.com #{Gitlab.config.gitlab.host}))
+      stub_asset_proxy_setting(domain_regexp: described_class.compile_whitelist(Gitlab.config.asset_proxy.whitelist))
+      @context = described_class.transform_context({})
+    end
+
+    it 'replaces img src' do
+      src     = 'http://example.com/test.png'
+      new_src = 'https://assets.example.com/08df250eeeef1a8cf2c761475ac74c5065105612/687474703a2f2f6578616d706c652e636f6d2f746573742e706e67'
+      doc     = filter(image(src), @context)
+
+      expect(doc.at_css('img')['src']).to eq new_src
+      expect(doc.at_css('img')['data-canonical-src']).to eq src
+    end
+
+    it 'skips internal images' do
+      src      = "#{Gitlab.config.gitlab.url}/test.png"
+      doc      = filter(image(src), @context)
+
+      expect(doc.at_css('img')['src']).to eq src
+    end
+
+    it 'skip relative urls' do
+      src = "/test.png"
+      doc = filter(image(src), @context)
+
+      expect(doc.at_css('img')['src']).to eq src
+    end
+
+    it 'skips single domain' do
+      src = "http://gitlab.com/test.png"
+      doc = filter(image(src), @context)
+
+      expect(doc.at_css('img')['src']).to eq src
+    end
+
+    it 'skips single domain and ignores url in query string' do
+      src = "http://gitlab.com/test.png?url=http://example.com/test.png"
+      doc = filter(image(src), @context)
+
+      expect(doc.at_css('img')['src']).to eq src
+    end
+
+    it 'skips wildcarded domain' do
+      src = "http://images.mydomain.com/test.png"
+      doc = filter(image(src), @context)
+
+      expect(doc.at_css('img')['src']).to eq src
+    end
+  end
+end
diff --git a/spec/lib/banzai/filter/external_link_filter_spec.rb b/spec/lib/banzai/filter/external_link_filter_spec.rb
index 2acbe05f082..9cb5f835b7f 100644
--- a/spec/lib/banzai/filter/external_link_filter_spec.rb
+++ b/spec/lib/banzai/filter/external_link_filter_spec.rb
@@ -154,6 +154,18 @@ describe Banzai::Filter::ExternalLinkFilter do
         expect(doc_email.to_html).to include('http://xn--example-6p25f.com/</a>')
       end
     end
+
+    context 'autolinked image' do
+      let(:html) { %q(<a href="https://assets.example.com/6d8b/634c" data-canonical-src="http://exa%F0%9F%98%84mple.com/test.png"><img src="http://exa%F0%9F%98%84mple.com/test.png" data-canonical-src="http://exa%F0%9F%98%84mple.com/test.png"></a>) }
+      let(:doc)  { filter(html) }
+
+      it_behaves_like 'an external link with rel attribute'
+
+      it 'adds a toolip with punycode' do
+        expect(doc.to_html).to include('class="has-tooltip"')
+        expect(doc.to_html).to include('title="http://xn--example-6p25f.com/test.png"')
+      end
+    end
   end
 
   context 'for links that look malicious' do
diff --git a/spec/lib/banzai/filter/image_link_filter_spec.rb b/spec/lib/banzai/filter/image_link_filter_spec.rb
index c84b98eb225..f9a830aeff9 100644
--- a/spec/lib/banzai/filter/image_link_filter_spec.rb
+++ b/spec/lib/banzai/filter/image_link_filter_spec.rb
@@ -26,4 +26,11 @@ describe Banzai::Filter::ImageLinkFilter do
     doc = filter(%Q(<p>test #{image('/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg')} inline</p>))
     expect(doc.to_html).to match %r{^<p>test <a[^>]*><img[^>]*></a> inline</p>$}
   end
+
+  it 'keep the data-canonical-src' do
+    doc = filter(%q(<img src="http://assets.example.com/6cd/4d7" data-canonical-src="http://example.com/test.png" />))
+
+    expect(doc.at_css('img')['src']).to eq doc.at_css('a')['href']
+    expect(doc.at_css('img')['data-canonical-src']).to eq doc.at_css('a')['data-canonical-src']
+  end
 end
diff --git a/spec/lib/banzai/filter/video_link_filter_spec.rb b/spec/lib/banzai/filter/video_link_filter_spec.rb
index 81dda0687f3..d63bf4d64c1 100644
--- a/spec/lib/banzai/filter/video_link_filter_spec.rb
+++ b/spec/lib/banzai/filter/video_link_filter_spec.rb
@@ -47,4 +47,26 @@ describe Banzai::Filter::VideoLinkFilter do
       expect(element['src']).to eq '/path/my_image.jpg'
     end
   end
+
+  context 'when asset proxy is enabled' do
+    it 'uses the correct src' do
+      stub_asset_proxy_setting(enabled: true)
+
+      proxy_src = 'https://assets.example.com/6d8b63'
+      canonical_src = 'http://example.com/test.mp4'
+      image = %(<img src="#{proxy_src}" data-canonical-src="#{canonical_src}" />)
+      container = filter(image, asset_proxy_enabled: true).children.first
+
+      expect(container['class']).to eq 'video-container'
+
+      video, paragraph = container.children
+
+      expect(video['src']).to eq proxy_src
+      expect(video['data-canonical-src']).to eq canonical_src
+
+      link = paragraph.children.first
+
+      expect(link['href']).to eq proxy_src
+    end
+  end
 end
diff --git a/spec/lib/banzai/pipeline/gfm_pipeline_spec.rb b/spec/lib/banzai/pipeline/gfm_pipeline_spec.rb
index 469692f7b5a..02408622499 100644
--- a/spec/lib/banzai/pipeline/gfm_pipeline_spec.rb
+++ b/spec/lib/banzai/pipeline/gfm_pipeline_spec.rb
@@ -140,4 +140,48 @@ describe Banzai::Pipeline::GfmPipeline do
       expect(output).to include(Gitlab::Routing.url_helpers.milestone_path(milestone))
     end
   end
+
+  describe 'asset proxy' do
+    let(:project) { create(:project, :public) }
+    let(:image)   { '![proxy](http://example.com/test.png)' }
+    let(:proxy)   { 'https://assets.example.com/08df250eeeef1a8cf2c761475ac74c5065105612/687474703a2f2f6578616d706c652e636f6d2f746573742e706e67' }
+    let(:version) { Gitlab::CurrentSettings.current_application_settings.local_markdown_version }
+
+    before do
+      stub_asset_proxy_setting(enabled: true)
+      stub_asset_proxy_setting(secret_key: 'shared-secret')
+      stub_asset_proxy_setting(url: 'https://assets.example.com')
+      stub_asset_proxy_setting(whitelist: %W(gitlab.com *.mydomain.com #{Gitlab.config.gitlab.host}))
+      stub_asset_proxy_setting(domain_regexp: Banzai::Filter::AssetProxyFilter.compile_whitelist(Gitlab.config.asset_proxy.whitelist))
+    end
+
+    it 'replaces a lazy loaded img src' do
+      output = described_class.to_html(image, project: project)
+      doc    = Nokogiri::HTML.fragment(output)
+      result = doc.css('img').first
+
+      expect(result['data-src']).to eq(proxy)
+    end
+
+    it 'autolinks images to the proxy' do
+      output = described_class.to_html(image, project: project)
+      doc    = Nokogiri::HTML.fragment(output)
+      result = doc.css('a').first
+
+      expect(result['href']).to eq(proxy)
+      expect(result['data-canonical-src']).to eq('http://example.com/test.png')
+    end
+
+    it 'properly adds tooltips to link for IDN images' do
+      image  = '![proxy](http://exa😄mple.com/test.png)'
+      proxy  = 'https://assets.example.com/6d8b634c412a23c6bfe1b2963f174febf5635ddd/687474703a2f2f6578612546302539462539382538346d706c652e636f6d2f746573742e706e67'
+      output = described_class.to_html(image, project: project)
+      doc    = Nokogiri::HTML.fragment(output)
+      result = doc.css('a').first
+
+      expect(result['href']).to eq(proxy)
+      expect(result['data-canonical-src']).to eq('http://exa%F0%9F%98%84mple.com/test.png')
+      expect(result['title']).to eq 'http://xn--example-6p25f.com/test.png'
+    end
+  end
 end
diff --git a/spec/models/application_setting_spec.rb b/spec/models/application_setting_spec.rb
index ab6f6dfe720..904185f4140 100644
--- a/spec/models/application_setting_spec.rb
+++ b/spec/models/application_setting_spec.rb
@@ -344,6 +344,71 @@ describe ApplicationSetting do
         end
       end
     end
+
+    context 'asset proxy settings' do
+      before do
+        subject.asset_proxy_enabled = true
+      end
+
+      describe '#asset_proxy_url' do
+        it { is_expected.not_to allow_value('').for(:asset_proxy_url) }
+        it { is_expected.to allow_value(http).for(:asset_proxy_url) }
+        it { is_expected.to allow_value(https).for(:asset_proxy_url) }
+        it { is_expected.not_to allow_value(ftp).for(:asset_proxy_url) }
+
+        it 'is not required when asset proxy is disabled' do
+          subject.asset_proxy_enabled = false
+          subject.asset_proxy_url = ''
+
+          expect(subject).to be_valid
+        end
+      end
+
+      describe '#asset_proxy_secret_key' do
+        it { is_expected.not_to allow_value('').for(:asset_proxy_secret_key) }
+        it { is_expected.to allow_value('anything').for(:asset_proxy_secret_key) }
+
+        it 'is not required when asset proxy is disabled' do
+          subject.asset_proxy_enabled = false
+          subject.asset_proxy_secret_key = ''
+
+          expect(subject).to be_valid
+        end
+
+        it 'is encrypted' do
+          subject.asset_proxy_secret_key = 'shared secret'
+
+          expect(subject.encrypted_asset_proxy_secret_key).to be_present
+          expect(subject.encrypted_asset_proxy_secret_key).not_to eq(subject.asset_proxy_secret_key)
+        end
+      end
+
+      describe '#asset_proxy_whitelist' do
+        context 'when given an Array' do
+          it 'sets the domains and adds current running host' do
+            setting.asset_proxy_whitelist = ['example.com', 'assets.example.com']
+            expect(setting.asset_proxy_whitelist).to eq(['example.com', 'assets.example.com', 'localhost'])
+          end
+        end
+
+        context 'when given a String' do
+          it 'sets multiple domains with spaces' do
+            setting.asset_proxy_whitelist = 'example.com *.example.com'
+            expect(setting.asset_proxy_whitelist).to eq(['example.com', '*.example.com', 'localhost'])
+          end
+
+          it 'sets multiple domains with newlines and a space' do
+            setting.asset_proxy_whitelist = "example.com\n *.example.com"
+            expect(setting.asset_proxy_whitelist).to eq(['example.com', '*.example.com', 'localhost'])
+          end
+
+          it 'sets multiple domains with commas' do
+            setting.asset_proxy_whitelist = "example.com, *.example.com"
+            expect(setting.asset_proxy_whitelist).to eq(['example.com', '*.example.com', 'localhost'])
+          end
+        end
+      end
+    end
   end
 
   context 'restrict creating duplicates' do
diff --git a/spec/requests/api/settings_spec.rb b/spec/requests/api/settings_spec.rb
index 8a60980fe80..f4697df99d0 100644
--- a/spec/requests/api/settings_spec.rb
+++ b/spec/requests/api/settings_spec.rb
@@ -158,5 +158,33 @@ describe API::Settings, 'Settings' do
         expect(json_response['error']).to eq('plantuml_url is missing')
       end
     end
+
+    context 'asset_proxy settings' do
+      it 'updates application settings' do
+        put api('/application/settings', admin),
+          params: {
+            asset_proxy_enabled: true,
+            asset_proxy_url: 'http://assets.example.com',
+            asset_proxy_secret_key: 'shared secret',
+            asset_proxy_whitelist: ['example.com', '*.example.com']
+          }
+
+        expect(response).to have_gitlab_http_status(200)
+        expect(json_response['asset_proxy_enabled']).to be(true)
+        expect(json_response['asset_proxy_url']).to eq('http://assets.example.com')
+        expect(json_response['asset_proxy_secret_key']).to be_nil
+        expect(json_response['asset_proxy_whitelist']).to eq(['example.com', '*.example.com', 'localhost'])
+      end
+
+      it 'allows a string for asset_proxy_whitelist' do
+        put api('/application/settings', admin),
+          params: {
+            asset_proxy_whitelist: 'example.com, *.example.com'
+          }
+
+        expect(response).to have_gitlab_http_status(200)
+        expect(json_response['asset_proxy_whitelist']).to eq(['example.com', '*.example.com', 'localhost'])
+      end
+    end
   end
 end
diff --git a/spec/services/application_settings/update_service_spec.rb b/spec/services/application_settings/update_service_spec.rb
index a641828faa5..7eb27016140 100644
--- a/spec/services/application_settings/update_service_spec.rb
+++ b/spec/services/application_settings/update_service_spec.rb
@@ -62,6 +62,39 @@ describe ApplicationSettings::UpdateService do
     end
   end
 
+  describe 'markdown cache invalidators' do
+    shared_examples 'invalidates markdown cache' do |attribute|
+      let(:params) { attribute }
+
+      it 'increments cache' do
+        expect { subject.execute }.to change(application_settings, :local_markdown_version).by(1)
+      end
+    end
+
+    it_behaves_like 'invalidates markdown cache', { asset_proxy_enabled: true }
+    it_behaves_like 'invalidates markdown cache', { asset_proxy_url: 'http://test.com' }
+    it_behaves_like 'invalidates markdown cache', { asset_proxy_secret_key: 'another secret' }
+    it_behaves_like 'invalidates markdown cache', { asset_proxy_whitelist: ['domain.com'] }
+
+    context 'when also setting the local_markdown_version' do
+      let(:params) { { asset_proxy_enabled: true, local_markdown_version: 12 } }
+
+      it 'does not increment' do
+        expect { subject.execute }.to change(application_settings, :local_markdown_version).to(12)
+      end
+    end
+
+    context 'do not invalidate if value does not change' do
+      let(:params) { { asset_proxy_enabled: true, asset_proxy_secret_key: 'secret', asset_proxy_url: 'http://test.com' } }
+
+      it 'does not increment' do
+        described_class.new(application_settings, admin, params).execute
+
+        expect { described_class.new(application_settings, admin, params).execute }.not_to change(application_settings, :local_markdown_version)
+      end
+    end
+  end
+
   describe 'performance bar settings' do
     using RSpec::Parameterized::TableSyntax
 
diff --git a/spec/support/helpers/stub_configuration.rb b/spec/support/helpers/stub_configuration.rb
index 049702be1f6..49b0619f05d 100644
--- a/spec/support/helpers/stub_configuration.rb
+++ b/spec/support/helpers/stub_configuration.rb
@@ -99,6 +99,10 @@ module StubConfiguration
     allow(Gitlab.config.gitlab_shell).to receive_messages(to_settings(messages))
   end
 
+  def stub_asset_proxy_setting(messages)
+    allow(Gitlab.config.asset_proxy).to receive_messages(to_settings(messages))
+  end
+
   def stub_rack_attack_setting(messages)
     allow(Gitlab.config.rack_attack).to receive(:git_basic_auth).and_return(messages)
     allow(Gitlab.config.rack_attack.git_basic_auth).to receive_messages(to_settings(messages))