diff --git a/lib/gitlab/sanitizers/svg.rb b/lib/gitlab/sanitizers/svg.rb index a540c534dee..3e705687873 100644 --- a/lib/gitlab/sanitizers/svg.rb +++ b/lib/gitlab/sanitizers/svg.rb @@ -10,13 +10,19 @@ module Gitlab DATA_ATTR_PATTERN = /\Adata-(?!xml)[a-z_][\w.\u00E0-\u00F6\u00F8-\u017F\u01DD-\u02AF-]*\z/u def scrub(node) - unless Whitelist::ALLOWED_ELEMENTS.include?(node.name) - node.unlink - else + if Whitelist::ALLOWED_ELEMENTS.include?(node.name) valid_attributes = Whitelist::ALLOWED_ATTRIBUTES[node.name] + return unless valid_attributes node.attribute_nodes.each do |attr| - unless valid_attributes && valid_attributes.include?(attribute_name_with_namespace(attr)) + attr_name = attribute_name_with_namespace(attr) + + if valid_attributes.include?(attr_name) + # xlink:href is on the whitelist but we should deny any reference other than internal ids + if attr_name == 'xlink:href' && unsafe_href?(attr) + attr.unlink + end + else if Whitelist::ALLOWED_DATA_ATTRIBUTES_IN_ELEMENTS.include?(node.name) && data_attribute?(attr) # Arbitrary data attributes are allowed. Verify that the attribute # is a valid data attribute. @@ -26,6 +32,8 @@ module Gitlab end end end + else + node.unlink end end @@ -37,7 +45,9 @@ module Gitlab end end - private + def unsafe_href?(attr) + !attr.value.start_with?('#') + end def data_attribute?(attr) attr.name.start_with?('data-') diff --git a/spec/lib/gitlab/sanitizers/svg_spec.rb b/spec/lib/gitlab/sanitizers/svg_spec.rb index bb1c98f51a6..061b759bac0 100644 --- a/spec/lib/gitlab/sanitizers/svg_spec.rb +++ b/spec/lib/gitlab/sanitizers/svg_spec.rb @@ -2,12 +2,12 @@ require 'spec_helper' describe Gitlab::Sanitizers::SVG do let(:scrubber) { Gitlab::Sanitizers::SVG::Scrubber.new } - let(:namespace) { double(Nokogiri::XML::Namespace, prefix: 'xlink') } - let(:namespaced_attr) { double(Nokogiri::XML::Attr, name: 'href', namespace: namespace) } + let(:namespace) { double(Nokogiri::XML::Namespace, prefix: 'xlink', href: 'http://www.w3.org/1999/xlink') } + let(:namespaced_attr) { double(Nokogiri::XML::Attr, name: 'href', namespace: namespace, value: '#awesome_id') } context 'scrubber' do describe '#scrub' do - let(:invalid_element) { double(Nokogiri::XML::Node, name: 'invalid') } + let(:invalid_element) { double(Nokogiri::XML::Node, name: 'invalid', value: 'invalid') } let(:invalid_attribute) { double(Nokogiri::XML::Attr, name: 'invalid', namespace: nil) } let(:valid_element) { double(Nokogiri::XML::Node, name: 'use') } @@ -44,5 +44,17 @@ describe Gitlab::Sanitizers::SVG do expect(scrubber.attribute_name_with_namespace(namespaced_attr)).to eq('xlink:href') end end + + describe '#unsafe_href?' do + let(:unsafe_attr) { double(Nokogiri::XML::Attr, name: 'href', namespace: namespace, value: 'http://evilsite.example.com/random.svg') } + + it 'returns true if href attribute is an external url' do + expect(scrubber.unsafe_href?(unsafe_attr)).to be_truthy + end + + it 'returns false if href atttribute is an internal reference' do + expect(scrubber.unsafe_href?(namespaced_attr)).to be_falsey + end + end end end