Merge branch 'security-osw-stop-linking-to-packages' into 'master'

Stop linking to unrecognized package sources

See merge request gitlab/gitlabhq!2933

app/views/projects/blob/viewers/_dependency_manager.html.haml

@@ -3,9 +3,4 @@
   This project manages its dependencies using
   %strong= viewer.manager_name
 
-  - if viewer.package_name
-    and defines a #{viewer.package_type} named
-    %strong<
-      = link_to_if viewer.package_url.present?, viewer.package_name, viewer.package_url, target: '_blank', rel: 'noopener noreferrer'
-
 = link_to 'Learn more', viewer.manager_url, target: '_blank', rel: 'noopener noreferrer'

changelogs/unreleased/security-osw-stop-linking-to-packages.yml

+---
+title: Stop linking to unrecognized package sources
+merge_request: 55518
+author:
+type: security

lib/gitlab/dependency_linker/base_linker.rb

@@ -4,6 +4,7 @@ module Gitlab
   module DependencyLinker
     class BaseLinker
       URL_REGEX = %r{https?://[^'" ]+}.freeze
+      GIT_INVALID_URL_REGEX = /^git\+#{URL_REGEX}/.freeze
       REPO_REGEX = %r{[^/'" ]+/[^/'" ]+}.freeze
 
       class_attribute :file_type
@@ -29,8 +30,25 @@ module Gitlab
         highlighted_lines.join.html_safe
       end
 
+      def external_url(name, external_ref)
+        return if external_ref =~ GIT_INVALID_URL_REGEX
+
+        case external_ref
+        when /\A#{URL_REGEX}\z/
+          external_ref
+        when /\A#{REPO_REGEX}\z/
+          github_url(external_ref)
+        else
+          package_url(name)
+        end
+      end
+
       private
 
+      def package_url(_name)
+        raise NotImplementedError
+      end
+
       def link_dependencies
         raise NotImplementedError
       end

lib/gitlab/dependency_linker/composer_json_linker.rb

@@ -8,8 +8,8 @@ module Gitlab
       private
 
       def link_packages
-        link_packages_at_key("require", &method(:package_url))
-        link_packages_at_key("require-dev", &method(:package_url))
+        link_packages_at_key("require")
+        link_packages_at_key("require-dev")
       end
 
       def package_url(name)

lib/gitlab/dependency_linker/gemfile_linker.rb

@@ -3,8 +3,14 @@
 module Gitlab
   module DependencyLinker
     class GemfileLinker < MethodLinker
+      class_attribute :package_keyword
+
+      self.package_keyword = :gem
       self.file_type = :gemfile
 
+      GITHUB_REGEX = /(github:|:github\s*=>)\s*['"](?<name>[^'"]+)['"]/.freeze
+      GIT_REGEX = /(git:|:git\s*=>)\s*['"](?<name>#{URL_REGEX})['"]/.freeze
+
       private
 
       def link_dependencies
@@ -14,21 +20,35 @@ module Gitlab
 
       def link_urls
         # Link `github: "user/repo"` to https://github.com/user/repo
-        link_regex(/(github:|:github\s*=>)\s*['"](?<name>[^'"]+)['"]/, &method(:github_url))
+        link_regex(GITHUB_REGEX, &method(:github_url))
 
         # Link `git: "https://gitlab.example.com/user/repo"` to https://gitlab.example.com/user/repo
-        link_regex(/(git:|:git\s*=>)\s*['"](?<name>#{URL_REGEX})['"]/, &:itself)
+        link_regex(GIT_REGEX, &:itself)
 
         # Link `source "https://rubygems.org"` to https://rubygems.org
         link_method_call('source', URL_REGEX, &:itself)
       end
 
       def link_packages
-        # Link `gem "package_name"` to https://rubygems.org/gems/package_name
-        link_method_call('gem') do |name|
-          "https://rubygems.org/gems/#{name}"
+        packages = parse_packages
+
+        return if packages.blank?
+
+        packages.each do |package|
+          link_method_call('gem', package.name) do
+            external_url(package.name, package.external_ref)
+          end
         end
       end
+
+      def package_url(name)
+        "https://rubygems.org/gems/#{name}"
+      end
+
+      def parse_packages
+        parser = Gitlab::DependencyLinker::Parser::Gemfile.new(plain_text)
+        parser.parse(keyword: self.class.package_keyword)
+      end
     end
   end
 end

lib/gitlab/dependency_linker/gemspec_linker.rb

@@ -11,7 +11,7 @@ module Gitlab
         link_method_call('homepage', URL_REGEX, &:itself)
         link_method_call('license', &method(:license_url))
 
-        link_method_call(%w[name add_dependency add_runtime_dependency add_development_dependency]) do |name|
+        link_method_call(%w[add_dependency add_runtime_dependency add_development_dependency]) do |name|
           "https://rubygems.org/gems/#{name}"
         end
       end

lib/gitlab/dependency_linker/method_linker.rb

@@ -23,18 +23,22 @@ module Gitlab
       #   link_method_call('name')
       #   # Will link `package` in `self.name = "package"`
       def link_method_call(method_name, value = nil, &url_proc)
+        regex = method_call_regex(method_name, value)
+
+        link_regex(regex, &url_proc)
+      end
+
+      def method_call_regex(method_name, value = nil)
         method_name = regexp_for_value(method_name)
         value = regexp_for_value(value)
 
-        regex = %r{
+        %r{
           #{method_name}            # Method name
           \s*                       # Whitespace
           [(=]?                     # Opening brace or equals sign
           \s*                       # Whitespace
           ['"](?<name>#{value})['"] # Package name in quotes
         }x
-
-        link_regex(regex, &url_proc)
       end
     end
   end

lib/gitlab/dependency_linker/package.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module DependencyLinker
+    class Package
+      attr_reader :name, :git_ref, :github_ref
+
+      def initialize(name, git_ref, github_ref)
+        @name = name
+        @git_ref = git_ref
+        @github_ref = github_ref
+      end
+
+      def external_ref
+        @git_ref || @github_ref
+      end
+    end
+  end
+end

lib/gitlab/dependency_linker/package_json_linker.rb

@@ -8,7 +8,6 @@ module Gitlab
       private
 
       def link_dependencies
-        link_json('name', json["name"], &method(:package_url))
         link_json('license', &method(:license_url))
         link_json(%w[homepage url], URL_REGEX, &:itself)
 
@@ -16,25 +15,19 @@ module Gitlab
       end
 
       def link_packages
-        link_packages_at_key("dependencies", &method(:package_url))
-        link_packages_at_key("devDependencies", &method(:package_url))
+        link_packages_at_key("dependencies")
+        link_packages_at_key("devDependencies")
       end
 
-      def link_packages_at_key(key, &url_proc)
+      def link_packages_at_key(key)
         dependencies = json[key]
         return unless dependencies
 
         dependencies.each do |name, version|
-          link_json(name, version, link: :key, &url_proc)
-
-          link_json(name) do |value|
-            case value
-            when /\A#{URL_REGEX}\z/
-              value
-            when /\A#{REPO_REGEX}\z/
-              github_url(value)
-            end
-          end
+          external_url = external_url(name, version)
+
+          link_json(name, version, link: :key) { external_url }
+          link_json(name) { external_url }
         end
       end
 

lib/gitlab/dependency_linker/parser/gemfile.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module DependencyLinker
+    module Parser
+      class Gemfile < MethodLinker
+        GIT_REGEX = Gitlab::DependencyLinker::GemfileLinker::GIT_REGEX
+        GITHUB_REGEX = Gitlab::DependencyLinker::GemfileLinker::GITHUB_REGEX
+
+        def initialize(plain_text)
+          @plain_text = plain_text
+        end
+
+        # Returns a list of Gitlab::DependencyLinker::Package
+        #
+        # keyword - The package definition keyword, e.g. `:gem` for
+        # Gemfile parsing, `:pod` for Podfile.
+        def parse(keyword:)
+          plain_lines.each_with_object([]) do |line, packages|
+            name = fetch(line, method_call_regex(keyword))
+
+            next unless name
+
+            git_ref = fetch(line, GIT_REGEX)
+            github_ref = fetch(line, GITHUB_REGEX)
+
+            packages << Gitlab::DependencyLinker::Package.new(name, git_ref, github_ref)
+          end
+        end
+
+        private
+
+        def fetch(line, regex, group: :name)
+          match = line.match(regex)
+          match[group] if match
+        end
+      end
+    end
+  end
+end

lib/gitlab/dependency_linker/podfile_linker.rb

@@ -5,12 +5,21 @@ module Gitlab
     class PodfileLinker < GemfileLinker
       include Cocoapods
 
+      self.package_keyword = :pod
       self.file_type = :podfile
 
       private
 
       def link_packages
-        link_method_call('pod', &method(:package_url))
+        packages = parse_packages
+
+        return unless packages
+
+        packages.each do |package|
+          link_method_call('pod', package.name) do
+            external_url(package.name, package.external_ref)
+          end
+        end
       end
     end
   end

lib/gitlab/dependency_linker/podspec_linker.rb

@@ -19,7 +19,7 @@ module Gitlab
         link_method_call('license', &method(:license_url))
         link_regex(/license\s*=\s*\{\s*(type:|:type\s*=>)\s*#{STRING_REGEX}/, &method(:license_url))
 
-        link_method_call(%w[name dependency], &method(:package_url))
+        link_method_call('dependency', &method(:package_url))
       end
     end
   end

spec/features/projects/blobs/blob_show_spec.rb

@@ -548,10 +548,7 @@ describe 'File blob', :js do
     it 'displays an auxiliary viewer' do
       aggregate_failures do
         # shows names of dependency manager and package
-        expect(page).to have_content('This project manages its dependencies using RubyGems and defines a gem named activerecord.')
-
-        # shows a link to the gem
-        expect(page).to have_link('activerecord', href: 'https://rubygems.org/gems/activerecord')
+        expect(page).to have_content('This project manages its dependencies using RubyGems.')
 
         # shows a learn more link
         expect(page).to have_link('Learn more', href: 'https://rubygems.org/')

spec/lib/gitlab/dependency_linker/composer_json_linker_spec.rb

@@ -50,8 +50,8 @@ describe Gitlab::DependencyLinker::ComposerJsonLinker do
       %{<a href="#{url}" rel="nofollow noreferrer noopener" target="_blank">#{name}</a>}
     end
 
-    it 'links the module name' do
-      expect(subject).to include(link('laravel/laravel', 'https://packagist.org/packages/laravel/laravel'))
+    it 'does not link the module name' do
+      expect(subject).not_to include(link('laravel/laravel', 'https://packagist.org/packages/laravel/laravel'))
     end
 
     it 'links the homepage' do

spec/lib/gitlab/dependency_linker/gemfile_linker_spec.rb

@@ -41,13 +41,16 @@ describe Gitlab::DependencyLinker::GemfileLinker do
     end
 
     it 'links dependencies' do
-      expect(subject).to include(link('rails', 'https://rubygems.org/gems/rails'))
       expect(subject).to include(link('rails-deprecated_sanitizer', 'https://rubygems.org/gems/rails-deprecated_sanitizer'))
-      expect(subject).to include(link('responders', 'https://rubygems.org/gems/responders'))
-      expect(subject).to include(link('sprockets', 'https://rubygems.org/gems/sprockets'))
       expect(subject).to include(link('default_value_for', 'https://rubygems.org/gems/default_value_for'))
     end
 
+    it 'links to external dependencies' do
+      expect(subject).to include(link('rails', 'https://github.com/rails/rails'))
+      expect(subject).to include(link('responders', 'https://github.com/rails/responders'))
+      expect(subject).to include(link('sprockets', 'https://gitlab.example.com/gems/sprockets'))
+    end
+
     it 'links GitHub repos' do
       expect(subject).to include(link('rails/rails', 'https://github.com/rails/rails'))
       expect(subject).to include(link('rails/responders', 'https://github.com/rails/responders'))

spec/lib/gitlab/dependency_linker/gemspec_linker_spec.rb

@@ -43,8 +43,8 @@ describe Gitlab::DependencyLinker::GemspecLinker do
       %{<a href="#{url}" rel="nofollow noreferrer noopener" target="_blank">#{name}</a>}
     end
 
-    it 'links the gem name' do
-      expect(subject).to include(link('gitlab_git', 'https://rubygems.org/gems/gitlab_git'))
+    it 'does not link the gem name' do
+      expect(subject).not_to include(link('gitlab_git', 'https://rubygems.org/gems/gitlab_git'))
     end
 
     it 'links the license' do

spec/lib/gitlab/dependency_linker/package_json_linker_spec.rb

@@ -33,7 +33,8 @@ describe Gitlab::DependencyLinker::PackageJsonLinker do
             "express": "4.2.x",
             "bigpipe": "bigpipe/pagelet",
             "plates": "https://github.com/flatiron/plates/tarball/master",
-            "karma": "^1.4.1"
+            "karma": "^1.4.1",
+            "random": "git+https://EdOverflow@github.com/example/example.git"
           },
           "devDependencies": {
             "vows": "^0.7.0",
@@ -51,8 +52,8 @@ describe Gitlab::DependencyLinker::PackageJsonLinker do
       %{<a href="#{url}" rel="nofollow noreferrer noopener" target="_blank">#{name}</a>}
     end
 
-    it 'links the module name' do
-      expect(subject).to include(link('module-name', 'https://npmjs.com/package/module-name'))
+    it 'does not link the module name' do
+      expect(subject).not_to include(link('module-name', 'https://npmjs.com/package/module-name'))
     end
 
     it 'links the homepage' do
@@ -71,14 +72,21 @@ describe Gitlab::DependencyLinker::PackageJsonLinker do
       expect(subject).to include(link('primus', 'https://npmjs.com/package/primus'))
       expect(subject).to include(link('async', 'https://npmjs.com/package/async'))
       expect(subject).to include(link('express', 'https://npmjs.com/package/express'))
-      expect(subject).to include(link('bigpipe', 'https://npmjs.com/package/bigpipe'))
-      expect(subject).to include(link('plates', 'https://npmjs.com/package/plates'))
       expect(subject).to include(link('karma', 'https://npmjs.com/package/karma'))
       expect(subject).to include(link('vows', 'https://npmjs.com/package/vows'))
       expect(subject).to include(link('assume', 'https://npmjs.com/package/assume'))
       expect(subject).to include(link('pre-commit', 'https://npmjs.com/package/pre-commit'))
     end
 
+    it 'links dependencies to URL detected on value' do
+      expect(subject).to include(link('bigpipe', 'https://github.com/bigpipe/pagelet'))
+      expect(subject).to include(link('plates', 'https://github.com/flatiron/plates/tarball/master'))
+    end
+
+    it 'does not link to NPM when invalid git URL' do
+      expect(subject).not_to include(link('random', 'https://npmjs.com/package/random'))
+    end
+
     it 'links GitHub repos' do
       expect(subject).to include(link('bigpipe/pagelet', 'https://github.com/bigpipe/pagelet'))
     end

spec/lib/gitlab/dependency_linker/parser/gemfile_spec.rb

+require 'rails_helper'
+
+describe Gitlab::DependencyLinker::Parser::Gemfile do
+  describe '#parse' do
+    let(:file_content) do
+      <<-CONTENT.strip_heredoc
+        source 'https://rubygems.org'
+
+        gem "rails", '4.2.6', github: "rails/rails"
+        gem 'rails-deprecated_sanitizer', '~> 1.0.3'
+        gem 'responders', '~> 2.0', :github => 'rails/responders'
+        gem 'sprockets', '~> 3.6.0', git: 'https://gitlab.example.com/gems/sprockets'
+        gem 'default_value_for', '~> 3.0.0'
+      CONTENT
+    end
+
+    subject { described_class.new(file_content).parse(keyword: 'gem') }
+
+    def fetch_package(name)
+      subject.find { |package| package.name == name }
+    end
+
+    it 'returns parsed packages' do
+      expect(subject.size).to eq(5)
+      expect(subject).to all(be_a(Gitlab::DependencyLinker::Package))
+    end
+
+    it 'packages respond to name and external_ref accordingly' do
+      expect(fetch_package('rails')).to have_attributes(name: 'rails',
+                                                        github_ref: 'rails/rails',
+                                                        git_ref: nil)
+
+      expect(fetch_package('sprockets')).to have_attributes(name: 'sprockets',
+                                                            github_ref: nil,
+                                                            git_ref: 'https://gitlab.example.com/gems/sprockets')
+
+      expect(fetch_package('default_value_for')).to have_attributes(name: 'default_value_for',
+                                                                    github_ref: nil,
+                                                                    git_ref: nil)
+    end
+  end
+end

spec/lib/gitlab/dependency_linker/podfile_linker_spec.rb

@@ -43,7 +43,10 @@ describe Gitlab::DependencyLinker::PodfileLinker do
 
     it 'links packages' do
       expect(subject).to include(link('AFNetworking', 'https://cocoapods.org/pods/AFNetworking'))
-      expect(subject).to include(link('Interstellar/Core', 'https://cocoapods.org/pods/Interstellar'))
+    end
+
+    it 'links external packages' do
+      expect(subject).to include(link('Interstellar/Core', 'https://github.com/ashfurrow/Interstellar.git'))
     end
 
     it 'links Git repos' do

spec/lib/gitlab/dependency_linker/podspec_linker_spec.rb

@@ -42,8 +42,8 @@ describe Gitlab::DependencyLinker::PodspecLinker do
       %{<a href="#{url}" rel="nofollow noreferrer noopener" target="_blank">#{name}</a>}
     end
 
-    it 'links the gem name' do
-      expect(subject).to include(link('Reachability', 'https://cocoapods.org/pods/Reachability'))
+    it 'does not link the pod name' do
+      expect(subject).not_to include(link('Reachability', 'https://cocoapods.org/pods/Reachability'))
     end
 
     it 'links the license' do