Merge branch 'rs-sanitize-submodule-urls' into 'security'

Sanitize submodule URLs before linking to them in the file tree view See merge request !2084

Merge branch 'rs-sanitize-submodule-urls' into 'security'
Sanitize submodule URLs before linking to them in the file tree view See merge request !2084
9ae401cf · Douwe Maan · Bob Van Landuyt · da13d1af · 9ae401cf · 9ae401cf
--- a/app/helpers/submodule_helper.rb
+++ b/app/helpers/submodule_helper.rb
 module SubmoduleHelper
  include Gitlab::ShellAdapter

+  VALID_SUBMODULE_PROTOCOLS = %w[http https git ssh].freeze
+
  # links to files listing for submodule if submodule is a project on this server
  def submodule_links(submodule_item, ref = nil, repository = @repository)
    url = repository.submodule_url_for(ref, submodule_item.path)

-    return url, nil unless url =~ /([^\/:]+)\/([^\/]+(?:\.git)?)\Z/
-
-    namespace = $1
-    project = $2
-    project.chomp!('.git')
+    if url =~ /([^\/:]+)\/([^\/]+(?:\.git)?)\Z/
+      namespace, project = $1, $2
+      project.sub!(/\.git\z/, '')

-    if self_url?(url, namespace, project)
-      return namespace_project_path(namespace, project),
-        namespace_project_tree_path(namespace, project,
-                                    submodule_item.id)
-    elsif relative_self_url?(url)
-      relative_self_links(url, submodule_item.id)
-    elsif github_dot_com_url?(url)
-      standard_links('github.com', namespace, project, submodule_item.id)
-    elsif gitlab_dot_com_url?(url)
-      standard_links('gitlab.com', namespace, project, submodule_item.id)
+      if self_url?(url, namespace, project)
+        [namespace_project_path(namespace, project),
+         namespace_project_tree_path(namespace, project, submodule_item.id)]
+      elsif relative_self_url?(url)
+        relative_self_links(url, submodule_item.id)
+      elsif github_dot_com_url?(url)
+        standard_links('github.com', namespace, project, submodule_item.id)
+      elsif gitlab_dot_com_url?(url)
+        standard_links('gitlab.com', namespace, project, submodule_item.id)
+      else
+        [sanitize_submodule_url(url), nil]
+      end
    else
-      return url, nil
+      [sanitize_submodule_url(url), nil]
    end
  end

@@ -73,4 +75,16 @@ def relative_self_links(url, commit)
      namespace_project_tree_path(namespace, base, commit)
    ]
  end
+
+  def sanitize_submodule_url(url)
+    uri = URI.parse(url)
+
+    if uri.scheme.in?(VALID_SUBMODULE_PROTOCOLS)
+      uri.to_s
+    else
+      nil
+    end
+  rescue URI::InvalidURIError
+    nil
+  end
 end
--- a/changelogs/unreleased/rs-sanitize-submodule-urls.yml
+++ b/changelogs/unreleased/rs-sanitize-submodule-urls.yml
+---
+title: Sanitize submodule URLs before linking to them in the file tree view
+merge_request:
+author:
--- a/spec/helpers/submodule_helper_spec.rb
+++ b/spec/helpers/submodule_helper_spec.rb
@@ -109,6 +109,18 @@
    end

    context 'submodule on unsupported' do
+      it 'sanitizes unsupported protocols' do
+        stub_url('javascript:alert("XSS");')
+
+        expect(helper.submodule_links(submodule_item)).to eq([nil, nil])
+      end
+
+      it 'sanitizes unsupported protocols disguised as a repository URL' do
+        stub_url('javascript:alert("XSS");foo/bar.git')
+
+        expect(helper.submodule_links(submodule_item)).to eq([nil, nil])
+      end
+
      it 'returns original' do
        stub_url('http://mygitserver.com/gitlab-org/gitlab-ce')
        expect(submodule_links(submodule_item)).to eq([repo.submodule_url_for, nil])