Merge branch 'dm-go-get-xss' into 'security-9-3'

Fix XSS issue in go-get handling See merge request !2128

Merge branch 'dm-go-get-xss' into 'security-9-3'
Fix XSS issue in go-get handling See merge request !2128
f4a4aa0d · Robert Speicher · Simon Knox · ade602fd · f4a4aa0d · f4a4aa0d
Commit f4a4aa0d authored Jun 29, 2017 by Robert Speicher Committed by Simon Knox Aug 29, 2017
3 changed files
--- a/changelogs/unreleased/dm-go-get-xss.yml
+++ b/changelogs/unreleased/dm-go-get-xss.yml
+---
+title: Fix XSS issue in go-get handling
+merge_request:
+author:
--- a/lib/gitlab/middleware/go.rb
+++ b/lib/gitlab/middleware/go.rb
@@ -3,6 +3,10 @@
 module Gitlab
  module Middleware
    class Go
+      include ActionView::Helpers::TagHelper
+
+      PROJECT_PATH_REGEX = %r{\A(#{Gitlab::PathRegex.full_namespace_route_regex}/#{Gitlab::PathRegex.project_route_regex})/}.freeze
+
      def initialize(app)
        @app = app
      end
@@ -10,17 +14,20 @@ module Gitlab
      def call(env)
        request = Rack::Request.new(env)

-        if go_request?(request)
-          render_go_doc(request)
-        else
-          @app.call(env)
-        end
+        render_go_doc(request) || @app.call(env)
      end

      private

      def render_go_doc(request)
-        body = go_body(request)
+        return unless go_request?(request)
+
+        path = project_path(request)
+        return unless path
+
+        body = go_body(path)
+        return unless body
+
        response = Rack::Response.new(body, 200, { 'Content-Type' => 'text/html' })
        response.finish
      end
@@ -29,11 +36,13 @@ module Gitlab
        request["go-get"].to_i == 1 && request.env["PATH_INFO"].present?
      end

-      def go_body(request)
-        project_url = URI.join(Gitlab.config.gitlab.url, project_path(request))
+      def go_body(path)
+        project_url = URI.join(Gitlab.config.gitlab.url, path)
        import_prefix = strip_url(project_url.to_s)

-        "<!DOCTYPE html><html><head><meta content='#{import_prefix} git #{project_url}.git' name='go-import'></head></html>\n"
+        meta_tag = tag :meta, name: 'go-import', content: "#{import_prefix} git #{project_url}.git"
+        head_tag = content_tag :head, meta_tag
+        content_tag :html, head_tag
      end

      def strip_url(url)
@@ -44,6 +53,10 @@ module Gitlab
        path_info = request.env["PATH_INFO"]
        path_info.sub!(/^\//, '')

+        project_path_match = "#{path_info}/".match(PROJECT_PATH_REGEX)
+        return unless project_path_match
+        path = project_path_match[1]
+
        # Go subpackages may be in the form of `namespace/project/path1/path2/../pathN`.
        # In a traditional project with a single namespace, this would denote repo
        # `namespace/project` with subpath `path1/path2/../pathN`, but with nested
@@ -51,7 +64,7 @@ module Gitlab
        # `path2/../pathN`, for example.

        # We find all potential project paths out of the path segments
-        path_segments = path_info.split('/')
+        path_segments = path.split('/')
        simple_project_path = path_segments.first(2).join('/')

        # If the path is at most 2 segments long, it is a simple `namespace/project` path and we're done

--- a/spec/lib/gitlab/middleware/go_spec.rb
+++ b/spec/lib/gitlab/middleware/go_spec.rb
@@ -79,12 +79,28 @@ describe Gitlab::Middleware::Go do
          it_behaves_like 'a nested project'
        end

+        context 'with a subpackage that is not a valid project path' do
+          let(:path) { "#{project.full_path}/---subpackage" }
+
+          it_behaves_like 'a nested project'
+        end
+
        context 'without subpackages' do
          let(:path) { project.full_path }

          it_behaves_like 'a nested project'
        end
      end
+
+      context 'with a bogus path' do
+        let(:path) { "http:;url=http:&sol;&sol;www.example.com'http-equiv='refresh'x='?go-get=1" }
+
+        it 'skips go-import generation' do
+          expect(app).to receive(:call).and_return('no-go')
+
+          go
+        end
+      end
    end

    def go
@@ -100,7 +116,7 @@ describe Gitlab::Middleware::Go do
    def expect_response_with_path(response, path)
      expect(response[0]).to eq(200)
      expect(response[1]['Content-Type']).to eq('text/html')
-      expected_body = "<!DOCTYPE html><html><head><meta content='#{Gitlab.config.gitlab.host}/#{path} git http://#{Gitlab.config.gitlab.host}/#{path}.git' name='go-import'></head></html>\n"
+      expected_body = %{<html><head><meta name="go-import" content="#{Gitlab.config.gitlab.host}/#{path} git http://#{Gitlab.config.gitlab.host}/#{path}.git" /></head></html>}
      expect(response[2].body).to eq([expected_body])
    end
  end