Merge branch 'fix-mr-500' into 'master'

Fix MR 500 when viewing the MR page

Fixes #2712 

See merge request !2167

4 files changed, 38 insertions, 2 deletions

diff --git a/CHANGELOG b/CHANGELOG
index a8914baa129..d1540e9d7fb 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -8,6 +8,8 @@ v 8.3.0 (unreleased)
   - Add button to automatically merge a merge request when the build succeeds (Zeger-Jan van de Weg)
   - Provide better diagnostic message upon project creation errors (Stan Hu)
   - Bump devise to 3.5.3 to fix reset token expiring after account creation (Stan Hu)
+  - Remove api credentials from link to build_page
+  - Deprecate GitLabCiService making it to always be inactive
   - Bump gollum-lib to 4.1.0 (Stan Hu)
   - Fix broken group avatar upload under "New group" (Stan Hu)
   - Update project repositorize size and commit count during import:repos task (Stan Hu)
diff --git a/app/helpers/merge_requests_helper.rb b/app/helpers/merge_requests_helper.rb
index 6c32647594d..1dd07a2a220 100644
--- a/app/helpers/merge_requests_helper.rb
+++ b/app/helpers/merge_requests_helper.rb
@@ -27,7 +27,16 @@ module MergeRequestsHelper
   end
 
   def ci_build_details_path(merge_request)
-    merge_request.source_project.ci_service.build_page(merge_request.last_commit.sha, merge_request.source_branch)
+    build_url = merge_request.source_project.ci_service.build_page(merge_request.last_commit.sha, merge_request.source_branch)
+    return nil unless build_url
+
+    parsed_url = URI.parse(build_url)
+
+    unless parsed_url.userinfo.blank?
+      parsed_url.userinfo = ''
+    end
+
+    parsed_url.to_s
   end
 
   def merge_path_description(merge_request, separator)
diff --git a/app/models/project_services/gitlab_ci_service.rb b/app/models/project_services/gitlab_ci_service.rb
index d73182d40ac..b64d97ce75d 100644
--- a/app/models/project_services/gitlab_ci_service.rb
+++ b/app/models/project_services/gitlab_ci_service.rb
@@ -18,6 +18,11 @@
 #  note_events           :boolean          default(TRUE), not null
 #
 
+# TODO(ayufan): The GitLabCiService is deprecated and the type should be removed when the database entries are removed
 class GitlabCiService < CiService
-  # this is no longer used
+  # We override the active accessor to always make GitLabCiService disabled
+  # Otherwise the GitLabCiService can be picked, but should never be since it's deprecated
+  def active
+    false
+  end
 end
diff --git a/spec/helpers/merge_requests_helper_spec.rb b/spec/helpers/merge_requests_helper_spec.rb
index 28fb3216f60..600e1c4e9ec 100644
--- a/spec/helpers/merge_requests_helper_spec.rb
+++ b/spec/helpers/merge_requests_helper_spec.rb
@@ -1,6 +1,26 @@
 require 'spec_helper'
 
 describe MergeRequestsHelper do
+  describe 'ci_build_details_path' do
+    let(:project) { create :project }
+    let(:merge_request) { MergeRequest.new }
+    let(:ci_service) { CiService.new }
+    let(:last_commit) { Ci::Commit.new({}) }
+
+    before do
+      allow(merge_request).to receive(:source_project).and_return(project)
+      allow(merge_request).to receive(:last_commit).and_return(last_commit)
+      allow(project).to receive(:ci_service).and_return(ci_service)
+      allow(last_commit).to receive(:sha).and_return('12d65c')
+    end
+
+    it 'does not include api credentials in a link' do
+      allow(ci_service).
+        to receive(:build_page).and_return("http://secretuser:secretpass@jenkins.example.com:8888/job/test1/scm/bySHA1/12d65c")
+      expect(helper.ci_build_details_path(merge_request)).to_not match("secret")
+    end
+  end
+
   describe '#issues_sentence' do
     subject { issues_sentence(issues) }
     let(:issues) do