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

Use image proxy to mitigate stealing ip addresses

See merge request gitlab/gitlab-ee!1068

app/helpers/application_settings_helper.rb

@@ -164,6 +164,10 @@ module ApplicationSettingsHelper
       :allow_local_requests_from_system_hooks,
       :dns_rebinding_protection_enabled,
       :archive_builds_in_human_readable,
+      :asset_proxy_enabled,
+      :asset_proxy_secret_key,
+      :asset_proxy_url,
+      :asset_proxy_whitelist,
       :authorized_keys_enabled,
       :auto_devops_enabled,
       :auto_devops_domain,

app/models/application_setting.rb

@@ -18,12 +18,19 @@ class ApplicationSetting < ApplicationRecord
   # fix a lot of tests using allow_any_instance_of
   include ApplicationSettingImplementation
 
+  attr_encrypted :asset_proxy_secret_key,
+                 mode: :per_attribute_iv,
+                 insecure_mode: true,
+                 key: Settings.attr_encrypted_db_key_base_truncated,
+                 algorithm: 'aes-256-cbc'
+
   serialize :restricted_visibility_levels # rubocop:disable Cop/ActiveRecordSerialize
   serialize :import_sources # rubocop:disable Cop/ActiveRecordSerialize
   serialize :disabled_oauth_sign_in_sources, Array # rubocop:disable Cop/ActiveRecordSerialize
   serialize :domain_whitelist, Array # rubocop:disable Cop/ActiveRecordSerialize
   serialize :domain_blacklist, Array # rubocop:disable Cop/ActiveRecordSerialize
   serialize :repository_storages # rubocop:disable Cop/ActiveRecordSerialize
+  serialize :asset_proxy_whitelist, Array # rubocop:disable Cop/ActiveRecordSerialize
 
   ignore_column :koding_url
   ignore_column :koding_enabled
@@ -192,6 +199,17 @@ class ApplicationSetting < ApplicationRecord
             allow_nil: true,
             numericality: { only_integer: true, greater_than_or_equal_to: 0, less_than: 65536 }
 
+  validates :asset_proxy_url,
+            presence: true,
+            allow_blank: false,
+            url: true,
+            if: :asset_proxy_enabled?
+
+  validates :asset_proxy_secret_key,
+            presence: true,
+            allow_blank: false,
+            if: :asset_proxy_enabled?
+
   SUPPORTED_KEY_TYPES.each do |type|
     validates :"#{type}_key_restriction", presence: true, key_restriction: { type: type }
   end

app/models/application_setting_implementation.rb

@@ -23,8 +23,9 @@ module ApplicationSettingImplementation
         akismet_enabled: false,
         allow_local_requests_from_web_hooks_and_services: false,
         allow_local_requests_from_system_hooks: true,
-        dns_rebinding_protection_enabled: true,
+        asset_proxy_enabled: false,
         authorized_keys_enabled: true, # TODO default to false if the instance is configured to use AuthorizedKeysCommand
+        commit_email_hostname: default_commit_email_hostname,
         container_registry_token_expire_delay: 5,
         default_artifacts_expire_in: '30 days',
         default_branch_protection: Settings.gitlab['default_branch_protection'],
@@ -33,7 +34,9 @@ module ApplicationSettingImplementation
         default_project_visibility: Settings.gitlab.default_projects_features['visibility_level'],
         default_projects_limit: Settings.gitlab['default_projects_limit'],
         default_snippet_visibility: Settings.gitlab.default_projects_features['visibility_level'],
+        diff_max_patch_bytes: Gitlab::Git::Diff::DEFAULT_MAX_PATCH_BYTES,
         disabled_oauth_sign_in_sources: [],
+        dns_rebinding_protection_enabled: true,
         domain_whitelist: Settings.gitlab['domain_whitelist'],
         dsa_key_restriction: 0,
         ecdsa_key_restriction: 0,
@@ -52,9 +55,11 @@ module ApplicationSettingImplementation
         housekeeping_gc_period: 200,
         housekeeping_incremental_repack_period: 10,
         import_sources: Settings.gitlab['import_sources'],
+        local_markdown_version: 0,
         max_artifacts_size: Settings.artifacts['max_size'],
         max_attachment_size: Settings.gitlab['max_attachment_size'],
         mirror_available: true,
+        outbound_local_requests_whitelist: [],
         password_authentication_enabled_for_git: true,
         password_authentication_enabled_for_web: Settings.gitlab['signin_enabled'],
         performance_bar_allowed_group_id: nil,
@@ -63,6 +68,8 @@ module ApplicationSettingImplementation
         plantuml_url: nil,
         polling_interval_multiplier: 1,
         project_export_enabled: true,
+        protected_ci_variables: false,
+        raw_blob_request_limit: 300,
         recaptcha_enabled: false,
         login_recaptcha_protection_enabled: false,
         repository_checks_enabled: true,
@@ -96,16 +103,10 @@ module ApplicationSettingImplementation
         user_default_internal_regex: nil,
         user_show_add_ssh_key_message: true,
         usage_stats_set_by_user_id: nil,
-        diff_max_patch_bytes: Gitlab::Git::Diff::DEFAULT_MAX_PATCH_BYTES,
-        commit_email_hostname: default_commit_email_hostname,
         snowplow_collector_hostname: nil,
         snowplow_cookie_domain: nil,
         snowplow_enabled: false,
-        snowplow_site_id: nil,
-        protected_ci_variables: false,
-        local_markdown_version: 0,
-        outbound_local_requests_whitelist: [],
-        raw_blob_request_limit: 300
+        snowplow_site_id: nil
       }
     end
 
@@ -199,6 +200,15 @@ module ApplicationSettingImplementation
     end
   end
 
+  def asset_proxy_whitelist=(values)
+    values = domain_strings_to_array(values) if values.is_a?(String)
+
+    # make sure we always whitelist the running host
+    values << Gitlab.config.gitlab.host unless values.include?(Gitlab.config.gitlab.host)
+
+    self[:asset_proxy_whitelist] = values
+  end
+
   def repository_storages
     Array(read_attribute(:repository_storages))
   end
@@ -307,6 +317,7 @@ module ApplicationSettingImplementation
 
     values
       .split(DOMAIN_LIST_SEPARATOR)
+      .map(&:strip)
       .reject(&:empty?)
       .uniq
   end

app/services/application_settings/update_service.rb

@@ -6,6 +6,8 @@ module ApplicationSettings
 
     attr_reader :params, :application_setting
 
+    MARKDOWN_CACHE_INVALIDATING_PARAMS = %w(asset_proxy_enabled asset_proxy_url asset_proxy_secret_key asset_proxy_whitelist).freeze
+
     def execute
       validate_classification_label(application_setting, :external_authorization_service_default_label)
 
@@ -25,7 +27,13 @@ module ApplicationSettings
         params[:usage_stats_set_by_user_id] = current_user.id
       end
 
-      @application_setting.update(@params)
+      @application_setting.assign_attributes(params)
+
+      if invalidate_markdown_cache?
+        @application_setting[:local_markdown_version] = @application_setting.local_markdown_version + 1
+      end
+
+      @application_setting.save
     end
 
     private
@@ -41,6 +49,11 @@ module ApplicationSettings
       @application_setting.add_to_outbound_local_requests_whitelist(values_array)
     end
 
+    def invalidate_markdown_cache?
+      !params.key?(:local_markdown_version) &&
+        (@application_setting.changes.keys & MARKDOWN_CACHE_INVALIDATING_PARAMS).any?
+    end
+
     def update_terms(terms)
       return unless terms.present?
 

changelogs/unreleased/security-enable-image-proxy.yml

+---
+title: Added image proxy to mitigate potential stealing of IP addresses
+merge_request:
+author:
+type: security

config/initializers/asset_proxy_settings.rb

+#
+# Asset proxy settings
+#
+ActiveSupport.on_load(:active_record) do
+  Banzai::Filter::AssetProxyFilter.initialize_settings
+end

config/initializers/fill_shards.rb

-if Shard.connected? && !Gitlab::Database.read_only?
+# The `table_exists?` check is needed because during our migration rollback testing,
+# `Shard.connected?` could be cached and return true even though the table doesn't exist
+if Shard.connected? && Shard.table_exists? && !Gitlab::Database.read_only?
   Shard.populate!
 end

db/migrate/20190219201635_add_asset_proxy_settings.rb

+# frozen_string_literal: true
+
+class AddAssetProxySettings < ActiveRecord::Migration[5.0]
+  include Gitlab::Database::MigrationHelpers
+
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+
+  def change
+    add_column :application_settings, :asset_proxy_enabled, :boolean, default: false, null: false
+    add_column :application_settings, :asset_proxy_url, :string # rubocop:disable Migration/AddLimitToStringColumns
+    add_column :application_settings, :asset_proxy_whitelist, :text
+    add_column :application_settings, :encrypted_asset_proxy_secret_key, :text
+    add_column :application_settings, :encrypted_asset_proxy_secret_key_iv, :string # rubocop:disable Migration/AddLimitToStringColumns
+  end
+end

db/schema.rb

@@ -279,6 +279,11 @@ ActiveRecord::Schema.define(version: 2019_08_16_151221) do
     t.boolean "allow_local_requests_from_web_hooks_and_services", default: false, null: false
     t.boolean "allow_local_requests_from_system_hooks", default: true, null: false
     t.bigint "instance_administration_project_id"
+    t.boolean "asset_proxy_enabled", default: false, null: false
+    t.string "asset_proxy_url"
+    t.text "asset_proxy_whitelist"
+    t.text "encrypted_asset_proxy_secret_key"
+    t.string "encrypted_asset_proxy_secret_key_iv"
     t.index ["custom_project_templates_group_id"], name: "index_application_settings_on_custom_project_templates_group_id"
     t.index ["file_template_project_id"], name: "index_application_settings_on_file_template_project_id"
     t.index ["instance_administration_project_id"], name: "index_applicationsettings_on_instance_administration_project_id"

doc/api/settings.md

@@ -67,7 +67,10 @@ Example response:
    "local_markdown_version": 0,
    "allow_local_requests_from_hooks_and_services": true,
    "allow_local_requests_from_web_hooks_and_services": true,
-   "allow_local_requests_from_system_hooks": false
+   "allow_local_requests_from_system_hooks": false,
+   "asset_proxy_enabled": true,
+   "asset_proxy_url": "https://assets.example.com",
+   "asset_proxy_whitelist": ["example.com", "*.example.com", "your-instance.com"]
 }
 ```
 
@@ -141,6 +144,9 @@ Example response:
   "user_show_add_ssh_key_message": true,
   "file_template_project_id": 1,
   "local_markdown_version": 0,
+  "asset_proxy_enabled": true,
+  "asset_proxy_url": "https://assets.example.com",
+  "asset_proxy_whitelist": ["example.com", "*.example.com", "your-instance.com"],
   "geo_node_allowed_ips": "0.0.0.0/0, ::/0",
   "allow_local_requests_from_hooks_and_services": true,
   "allow_local_requests_from_web_hooks_and_services": true,
@@ -186,6 +192,10 @@ are listed in the descriptions of the relevant settings.
 | `allow_local_requests_from_hooks_and_services` | boolean    | no                                   | (Deprecated: Use `allow_local_requests_from_web_hooks_and_services` instead) Allow requests to the local network from hooks and services. |
 | `allow_local_requests_from_web_hooks_and_services` | boolean    | no                                   | Allow requests to the local network from web hooks and services. |
 | `allow_local_requests_from_system_hooks` | boolean    | no                                   | Allow requests to the local network from system hooks. |
+| `asset_proxy_enabled`                    | boolean          | no                                   | (**If enabled, requires:** `asset_proxy_url`) Enable proxying of assets. GitLab restart is required to apply changes. |
+| `asset_proxy_secret_key`                 | string           | no                                   | Shared secret with the asset proxy server. GitLab restart is required to apply changes. |
+| `asset_proxy_url`                        | string           | no                                   | URL of the asset proxy server. GitLab restart is required to apply changes. |
+| `asset_proxy_whitelist`                  | string or array of strings | no                         | Assets that match these domain(s) will NOT be proxied. Wildcards allowed. Your GitLab installation URL is automatically whitelisted. GitLab restart is required to apply changes. |
 | `authorized_keys_enabled`                | boolean          | no                                   | By default, we write to the `authorized_keys` file to support Git over SSH without additional configuration. GitLab can be optimized to authenticate SSH keys via the database file. Only disable this if you have configured your OpenSSH server to use the AuthorizedKeysCommand. |
 | `auto_devops_domain`                     | string           | no                                   | Specify a domain to use by default for every project's Auto Review Apps and Auto Deploy stages. |
 | `auto_devops_enabled`                    | boolean          | no                                   | Enable Auto DevOps for projects by default. It will automatically build, test, and deploy applications based on a predefined CI/CD configuration. |

doc/security/README.md

@@ -17,3 +17,4 @@ type: index
 - [Enforce Two-factor authentication](two_factor_authentication.md)
 - [Send email confirmation on sign-up](user_email_confirmation.md)
 - [Security of running jobs](https://docs.gitlab.com/runner/security/)
+- [Proxying images](asset_proxy.md)

doc/security/asset_proxy.md

+A possible security concern when managing a public facing GitLab instance is
+the ability to steal a users IP address by referencing images in issues, comments, etc.
+
+For example, adding `![Example image](http://example.com/example.png)` to
+an issue description will cause the image to be loaded from the external
+server in order to be displayed.  However this also allows the external server
+to log the IP address of the user.
+
+One way to mitigate this is by proxying any external images to a server you
+control.  GitLab handles this by allowing you to run the "Camo" server
+[cactus/go-camo](https://github.com/cactus/go-camo#how-it-works).
+The image request is sent to the Camo server, which then makes the request for
+the original image.  This way an attacker only ever seems the IP address
+of your Camo server.
+
+Once you have your Camo server up and running, you can configure GitLab to
+proxy image requests to it.  The following settings are supported:
+
+| Attribute                 | Description |
+| ------------------------- | ----------- |
+| `asset_proxy_enabled`     | (**If enabled, requires:** `asset_proxy_url`) Enable proxying of assets. |
+| `asset_proxy_secret_key`  | Shared secret with the asset proxy server. |
+| `asset_proxy_url`         | URL of the asset proxy server. |
+| `asset_proxy_whitelist`   | Assets that match these domain(s) will NOT be proxied. Wildcards allowed. Your GitLab installation URL is automatically whitelisted. |
+
+These can be set via the [Application setting API](../api/settings.md)
+
+Note that a GitLab restart is required to apply any changes.

lib/api/entities.rb

@@ -1174,6 +1174,9 @@ module API
         attributes.delete(:performance_bar_enabled)
         attributes.delete(:allow_local_requests_from_hooks_and_services)
 
+        # let's not expose the secret key in a response
+        attributes.delete(:asset_proxy_secret_key)
+
         attributes
       end
 

lib/api/settings.rb

@@ -36,6 +36,10 @@ module API
       given akismet_enabled: ->(val) { val } do
         requires :akismet_api_key, type: String, desc: 'Generate API key at http://www.akismet.com'
       end
+      optional :asset_proxy_enabled, type: Boolean, desc: 'Enable proxying of assets'
+      optional :asset_proxy_url, type: String, desc: 'URL of the asset proxy server'
+      optional :asset_proxy_secret_key, type: String, desc: 'Shared secret with the asset proxy server'
+      optional :asset_proxy_whitelist, type: Array[String], coerce_with: Validations::Types::CommaSeparatedToArray.coerce, desc: 'Assets that match these domain(s) will NOT be proxied. Wildcards allowed. Your GitLab installation URL is automatically whitelisted.'
       optional :container_registry_token_expire_delay, type: Integer, desc: 'Authorization token duration (minutes)'
       optional :default_artifacts_expire_in, type: String, desc: "Set the default expiration time for each job's artifacts"
       optional :default_project_creation, type: Integer, values: ::Gitlab::Access.project_creation_values, desc: 'Determine if developers can create projects in the group'
@@ -128,7 +132,7 @@ module API
       optional :terminal_max_session_time, type: Integer, desc: 'Maximum time for web terminal websocket connection (in seconds). Set to 0 for unlimited time.'
       optional :usage_ping_enabled, type: Boolean, desc: 'Every week GitLab will report license usage back to GitLab, Inc.'
       optional :instance_statistics_visibility_private, type: Boolean, desc: 'When set to `true` Instance statistics will only be available to admins'
-      optional :local_markdown_version, type: Integer, desc: "Local markdown version, increase this value when any cached markdown should be invalidated"
+      optional :local_markdown_version, type: Integer, desc: 'Local markdown version, increase this value when any cached markdown should be invalidated'
       optional :allow_local_requests_from_hooks_and_services, type: Boolean, desc: 'Deprecated: Use :allow_local_requests_from_web_hooks_and_services instead. Allow requests to the local network from hooks and services.' # support legacy names, can be removed in v5
       optional :snowplow_enabled, type: Grape::API::Boolean, desc: 'Enable Snowplow tracking'
       given snowplow_enabled: ->(val) { val } do

lib/api/validations/types/comma_separated_to_array.rb

+# frozen_string_literal: true
+
+module API
+  module Validations
+    module Types
+      class CommaSeparatedToArray
+        def self.coerce
+          lambda do |value|
+            case value
+            when String
+              value.split(',').map(&:strip)
+            when Array
+              value.map { |v| v.to_s.split(',').map(&:strip) }.flatten
+            else
+              []
+            end
+          end
+        end
+      end
+    end
+  end
+end

lib/banzai/filter/asset_proxy_filter.rb

+# frozen_string_literal: true
+
+module Banzai
+  module Filter
+    # Proxy's images/assets to another server.  Reduces mixed content warnings
+    # as well as hiding the customer's IP address when requesting images.
+    # Copies the original img `src` to `data-canonical-src` then replaces the
+    # `src` with a new url to the proxy server.
+    class AssetProxyFilter < HTML::Pipeline::CamoFilter
+      def initialize(text, context = nil, result = nil)
+        super
+      end
+
+      def validate
+        needs(:asset_proxy, :asset_proxy_secret_key) if asset_proxy_enabled?
+      end
+
+      def asset_host_whitelisted?(host)
+        context[:asset_proxy_domain_regexp] ? context[:asset_proxy_domain_regexp].match?(host) : false
+      end
+
+      def self.transform_context(context)
+        context[:disable_asset_proxy] = !Gitlab.config.asset_proxy.enabled
+
+        unless context[:disable_asset_proxy]
+          context[:asset_proxy_enabled]       = !context[:disable_asset_proxy]
+          context[:asset_proxy]               = Gitlab.config.asset_proxy.url
+          context[:asset_proxy_secret_key]    = Gitlab.config.asset_proxy.secret_key
+          context[:asset_proxy_domain_regexp] = Gitlab.config.asset_proxy.domain_regexp
+        end
+
+        context
+      end
+
+      # called during an initializer. Caching the values in Gitlab.config significantly increased
+      # performance, rather than querying Gitlab::CurrentSettings.current_application_settings
+      # over and over.  However, this does mean that the Rails servers need to get restarted
+      # whenever the application settings are changed
+      def self.initialize_settings
+        application_settings           = Gitlab::CurrentSettings.current_application_settings
+        Gitlab.config['asset_proxy'] ||= Settingslogic.new({})
+
+        if application_settings.respond_to?(:asset_proxy_enabled)
+          Gitlab.config.asset_proxy['enabled']       = application_settings.asset_proxy_enabled
+          Gitlab.config.asset_proxy['url']           = application_settings.asset_proxy_url
+          Gitlab.config.asset_proxy['secret_key']    = application_settings.asset_proxy_secret_key
+          Gitlab.config.asset_proxy['whitelist']     = application_settings.asset_proxy_whitelist || [Gitlab.config.gitlab.host]
+          Gitlab.config.asset_proxy['domain_regexp'] = compile_whitelist(Gitlab.config.asset_proxy.whitelist)
+        else
+          Gitlab.config.asset_proxy['enabled']       = ::ApplicationSetting.defaults[:asset_proxy_enabled]
+        end
+      end
+
+      def self.compile_whitelist(domain_list)
+        return if domain_list.empty?
+
+        escaped = domain_list.map { |domain| Regexp.escape(domain).gsub('\*', '.*?') }
+        Regexp.new("^(#{escaped.join('|')})$", Regexp::IGNORECASE)
+      end
+    end
+  end
+end

lib/banzai/filter/external_link_filter.rb

@@ -14,10 +14,10 @@ module Banzai
           # such as on `mailto:` links. Since we've been using it, do an
           # initial parse for validity and then use Addressable
           # for IDN support, etc
-          uri = uri_strict(node['href'].to_s)
+          uri = uri_strict(node_src(node))
           if uri
-            node.set_attribute('href', uri.to_s)
-            addressable_uri = addressable_uri(node['href'])
+            node.set_attribute(node_src_attribute(node), uri.to_s)
+            addressable_uri = addressable_uri(node_src(node))
           else
             addressable_uri = nil
           end
@@ -35,6 +35,16 @@ module Banzai
 
       private
 
+      # if this is a link to a proxied image, then `src` is already the correct
+      # proxied url, so work with the `data-canonical-src`
+      def node_src_attribute(node)
+        node['data-canonical-src'] ? 'data-canonical-src' : 'href'
+      end
+
+      def node_src(node)
+        node[node_src_attribute(node)]
+      end
+
       def uri_strict(href)
         URI.parse(href)
       rescue URI::Error
@@ -72,7 +82,7 @@ module Banzai
         return unless uri
         return unless context[:emailable_links]
 
-        unencoded_uri_str = Addressable::URI.unencode(node['href'])
+        unencoded_uri_str = Addressable::URI.unencode(node_src(node))
 
         if unencoded_uri_str == node.content && idn?(uri)
           node.content = uri.normalize

lib/banzai/filter/image_link_filter.rb

@@ -18,6 +18,9 @@ module Banzai
             rel: 'noopener noreferrer'
           )
 
+          # make sure the original non-proxied src carries over to the link
+          link['data-canonical-src'] = img['data-canonical-src'] if img['data-canonical-src']
+
           link.children = img.clone
 
           img.replace(link)

lib/banzai/filter/video_link_filter.rb

@@ -23,6 +23,14 @@ module Banzai
             "'.#{ext}' = substring(@src, string-length(@src) - #{ext.size})"
           end
 
+          if context[:asset_proxy_enabled].present?
+            src_query.concat(
+              UploaderHelper::VIDEO_EXT.map do |ext|
+                "'.#{ext}' = substring(@data-canonical-src, string-length(@data-canonical-src) - #{ext.size})"
+              end
+            )
+          end
+
           "descendant-or-self::img[not(ancestor::a) and (#{src_query.join(' or ')})]"
         end
       end
@@ -48,6 +56,13 @@ module Banzai
           target: '_blank',
           rel: 'noopener noreferrer',
           title: "Download '#{element['title'] || element['alt']}'")
+
+        # make sure the original non-proxied src carries over
+        if element['data-canonical-src']
+          video['data-canonical-src'] = element['data-canonical-src']
+          link['data-canonical-src']  = element['data-canonical-src']
+        end
+
         download_paragraph = doc.document.create_element('p')
         download_paragraph.children = link
 

lib/banzai/pipeline/ascii_doc_pipeline.rb

@@ -6,12 +6,17 @@ module Banzai
       def self.filters
         FilterArray[
           Filter::AsciiDocSanitizationFilter,
+          Filter::AssetProxyFilter,
           Filter::SyntaxHighlightFilter,
           Filter::ExternalLinkFilter,
           Filter::PlantumlFilter,
           Filter::AsciiDocPostProcessingFilter
         ]
       end
+
+      def self.transform_context(context)
+        Filter::AssetProxyFilter.transform_context(context)
+      end
     end
   end
 end

lib/banzai/pipeline/gfm_pipeline.rb

@@ -19,6 +19,7 @@ module Banzai
           Filter::SpacedLinkFilter,
 
           Filter::SanitizationFilter,
+          Filter::AssetProxyFilter,
           Filter::SyntaxHighlightFilter,
 
           Filter::MathFilter,
@@ -62,7 +63,7 @@ module Banzai
       def self.transform_context(context)
         context[:only_path] = true unless context.key?(:only_path)
 
-        context
+        Filter::AssetProxyFilter.transform_context(context)
       end
     end
   end

lib/banzai/pipeline/markup_pipeline.rb

@@ -6,11 +6,16 @@ module Banzai
       def self.filters
         @filters ||= FilterArray[
           Filter::SanitizationFilter,
+          Filter::AssetProxyFilter,
           Filter::ExternalLinkFilter,
           Filter::PlantumlFilter,
           Filter::SyntaxHighlightFilter
         ]
       end
+
+      def self.transform_context(context)
+        Filter::AssetProxyFilter.transform_context(context)
+      end
     end
   end
 end

lib/banzai/pipeline/single_line_pipeline.rb

@@ -9,6 +9,7 @@ module Banzai
         @filters ||= FilterArray[
           Filter::HtmlEntityFilter,
           Filter::SanitizationFilter,
+          Filter::AssetProxyFilter,
 
           Filter::EmojiFilter,
           Filter::AutolinkFilter,
@@ -31,6 +32,8 @@ module Banzai
       end
 
       def self.transform_context(context)
+        context = Filter::AssetProxyFilter.transform_context(context)
+
         super(context).merge(
           no_sourcepos: true
         )

spec/initializers/asset_proxy_setting_spec.rb

+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

spec/lib/banzai/filter/asset_proxy_filter_spec.rb

+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

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

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

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

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

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

spec/models/application_setting_spec.rb

@@ -355,6 +355,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

spec/requests/api/settings_spec.rb

@@ -224,5 +224,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

spec/services/application_settings/update_service_spec.rb

@@ -110,6 +110,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
 

spec/support/helpers/stub_configuration.rb

@@ -105,6 +105,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))