Merge branch 'security-enable-image-proxy' into 'master'

Use image proxy to mitigate stealing ip addresses

Closes #2812

See merge request gitlab/gitlabhq!2926

6 files changed, 181 insertions, 1 deletions

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 59fea5766ee..4b2500b31f7 100644
--- a/spec/lib/banzai/filter/external_link_filter_spec.rb
+++ b/spec/lib/banzai/filter/external_link_filter_spec.rb
@@ -156,6 +156,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 7b0cb675551..011e3a1e2da 100644
--- a/spec/lib/banzai/filter/image_link_filter_spec.rb
+++ b/spec/lib/banzai/filter/image_link_filter_spec.rb
@@ -28,4 +28,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 483e806624c..cd932f502f3 100644
--- a/spec/lib/banzai/filter/video_link_filter_spec.rb
+++ b/spec/lib/banzai/filter/video_link_filter_spec.rb
@@ -49,4 +49,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 0a3e0962452..3a9ecd2fb81 100644
--- a/spec/lib/banzai/pipeline/gfm_pipeline_spec.rb
+++ b/spec/lib/banzai/pipeline/gfm_pipeline_spec.rb
@@ -142,4 +142,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/lib/gitlab/database_importers/self_monitoring/project/create_service_spec.rb b/spec/lib/gitlab/database_importers/self_monitoring/project/create_service_spec.rb
index 9bd0d800086..00033c5ba1c 100644
--- a/spec/lib/gitlab/database_importers/self_monitoring/project/create_service_spec.rb
+++ b/spec/lib/gitlab/database_importers/self_monitoring/project/create_service_spec.rb
@@ -130,7 +130,7 @@ describe Gitlab::DatabaseImporters::SelfMonitoring::Project::CreateService do
       end
 
       it 'returns error when saving project ID fails' do
-        allow(application_setting).to receive(:update) { false }
+        allow(application_setting).to receive(:save) { false }
 
         expect { result }.to raise_error(StandardError, 'Could not save project ID')
       end