Merge branch '15437-fix-xss-in-issue-tracker-service' into 'master'

Prevent XSS via custom issue tracker URL

Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/15437

See merge request !1955

Signed-off-by: Rémy Coutable <remy@rymai.me>

27 files changed, 880 insertions, 163 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 974143f9fde..7059bd846ba 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -1,5 +1,16 @@
 Please view this file on the master branch, on stable branches it's out of date.
 
+v 8.4.10
+  - Fix a window.opener bug that could lead to XSS and open redirects
+  - Prevent XSS via Git branch and tag names
+  - Prevent XSS via custom issue tracker URL
+  - Fix vulnerability that leaks private labels and milestones
+  - Prevent privilege escalation via "impersonate" feature
+  - Prevent users from deleting Webhooks via API they do not own
+  - Prevent information disclosure via milestone API
+  - Prevent information disclosure via snippet API
+  - Prevent information disclosure via new merge request page
+
 v 8.4.9
   - Remove persistent XSS vulnerability in `commit_person_link` helper
 
diff --git a/app/helpers/issues_helper.rb b/app/helpers/issues_helper.rb
index 43262d579e9..2eed9fb3ca0 100644
--- a/app/helpers/issues_helper.rb
+++ b/app/helpers/issues_helper.rb
@@ -16,31 +16,49 @@ module IssuesHelper
   def url_for_project_issues(project = @project, options = {})
     return '' if project.nil?
 
-    if options[:only_path]
-      project.issues_tracker.project_path
-    else
-      project.issues_tracker.project_url
-    end
+    url =
+      if options[:only_path]
+        project.issues_tracker.project_path
+      else
+        project.issues_tracker.project_url
+      end
+
+    # Ensure we return a valid URL to prevent possible XSS.
+    URI.parse(url).to_s
+  rescue URI::InvalidURIError
+    ''
   end
 
   def url_for_new_issue(project = @project, options = {})
     return '' if project.nil?
 
-    if options[:only_path]
-      project.issues_tracker.new_issue_path
-    else
-      project.issues_tracker.new_issue_url
-    end
+    url =
+      if options[:only_path]
+        project.issues_tracker.new_issue_path
+      else
+        project.issues_tracker.new_issue_url
+      end
+
+    # Ensure we return a valid URL to prevent possible XSS.
+    URI.parse(url).to_s
+  rescue URI::InvalidURIError
+    ''
   end
 
   def url_for_issue(issue_iid, project = @project, options = {})
     return '' if project.nil?
 
-    if options[:only_path]
-      project.issues_tracker.issue_path(issue_iid)
-    else
-      project.issues_tracker.issue_url(issue_iid)
-    end
+    url =
+      if options[:only_path]
+        project.issues_tracker.issue_path(issue_iid)
+      else
+        project.issues_tracker.issue_url(issue_iid)
+      end
+
+    # Ensure we return a valid URL to prevent possible XSS.
+    URI.parse(url).to_s
+  rescue URI::InvalidURIError
+    ''
   end
 
   def bulk_update_milestone_options
diff --git a/app/models/project_services/buildkite_service.rb b/app/models/project_services/buildkite_service.rb
index 3efbfd2eec3..861cc974ec4 100644
--- a/app/models/project_services/buildkite_service.rb
+++ b/app/models/project_services/buildkite_service.rb
@@ -26,7 +26,7 @@ class BuildkiteService < CiService
 
   prop_accessor :project_url, :token, :enable_ssl_verification
 
-  validates :project_url, presence: true, if: :activated?
+  validates :project_url, presence: true, url: true, if: :activated?
   validates :token, presence: true, if: :activated?
 
   after_save :compose_service_hook, if: :activated?
@@ -91,7 +91,7 @@ class BuildkiteService < CiService
       { type: 'text',
         name: 'project_url',
         placeholder: "#{ENDPOINT}/example/project" },
-      
+
       { type: 'checkbox',
         name: 'enable_ssl_verification',
         title: "Enable SSL verification" }
diff --git a/app/models/project_services/issue_tracker_service.rb b/app/models/project_services/issue_tracker_service.rb
index 25045224ce5..c5501e06411 100644
--- a/app/models/project_services/issue_tracker_service.rb
+++ b/app/models/project_services/issue_tracker_service.rb
@@ -21,7 +21,7 @@
 
 class IssueTrackerService < Service
 
-  validates :project_url, :issues_url, :new_issue_url, presence: true, if: :activated?
+  validates :project_url, :issues_url, :new_issue_url, presence: true, url: true, if: :activated?
 
   default_value_for :category, 'issue_tracker'
 
diff --git a/app/models/project_services/jira_service.rb b/app/models/project_services/jira_service.rb
index f6571fc063e..a9e57d70d9c 100644
--- a/app/models/project_services/jira_service.rb
+++ b/app/models/project_services/jira_service.rb
@@ -28,6 +28,8 @@ class JiraService < IssueTrackerService
   prop_accessor :username, :password, :api_url, :jira_issue_transition_id,
                 :title, :description, :project_url, :issues_url, :new_issue_url
 
+  validates :api_url, presence: true, url: true, if: :activated?
+
   before_validation :set_api_url, :set_jira_issue_transition_id
 
   before_update :reset_password
diff --git a/app/models/project_services/slack_service.rb b/app/models/project_services/slack_service.rb
index d89cf6d17b2..6af6767f12c 100644
--- a/app/models/project_services/slack_service.rb
+++ b/app/models/project_services/slack_service.rb
@@ -22,7 +22,7 @@
 class SlackService < Service
   prop_accessor :webhook, :username, :channel
   boolean_accessor :notify_only_broken_builds
-  validates :webhook, presence: true, if: :activated?
+  validates :webhook, presence: true, url: true, if: :activated?
 
   def initialize_properties
     if properties.nil?
diff --git a/spec/helpers/issues_helper_spec.rb b/spec/helpers/issues_helper_spec.rb
index ffd8ebae029..342c995a997 100644
--- a/spec/helpers/issues_helper_spec.rb
+++ b/spec/helpers/issues_helper_spec.rb
@@ -30,6 +30,18 @@ describe IssuesHelper do
       expect(url_for_project_issues).to eq ""
     end
 
+    it 'returns an empty string if project_url is invalid' do
+      expect(project).to receive_message_chain('issues_tracker.project_url') { 'javascript:alert("foo");' }
+
+      expect(url_for_project_issues(project)).to eq ''
+    end
+
+    it 'returns an empty string if project_path is invalid' do
+      expect(project).to receive_message_chain('issues_tracker.project_path') { 'javascript:alert("foo");' }
+
+      expect(url_for_project_issues(project, only_path: true)).to eq ''
+    end
+
     describe "when external tracker was enabled and then config removed" do
       before do
         @project = ext_project
@@ -68,6 +80,18 @@ describe IssuesHelper do
       expect(url_for_issue(issue.iid)).to eq ""
     end
 
+    it 'returns an empty string if issue_url is invalid' do
+      expect(project).to receive_message_chain('issues_tracker.issue_url') { 'javascript:alert("foo");' }
+
+      expect(url_for_issue(issue.iid, project)).to eq ''
+    end
+
+    it 'returns an empty string if issue_path is invalid' do
+      expect(project).to receive_message_chain('issues_tracker.issue_path') { 'javascript:alert("foo");' }
+
+      expect(url_for_issue(issue.iid, project, only_path: true)).to eq ''
+    end
+
     describe "when external tracker was enabled and then config removed" do
       before do
         @project = ext_project
@@ -105,6 +129,18 @@ describe IssuesHelper do
       expect(url_for_new_issue).to eq ""
     end
 
+    it 'returns an empty string if issue_url is invalid' do
+      expect(project).to receive_message_chain('issues_tracker.new_issue_url') { 'javascript:alert("foo");' }
+
+      expect(url_for_new_issue(project)).to eq ''
+    end
+
+    it 'returns an empty string if issue_path is invalid' do
+      expect(project).to receive_message_chain('issues_tracker.new_issue_path') { 'javascript:alert("foo");' }
+
+      expect(url_for_new_issue(project, only_path: true)).to eq ''
+    end
+
     describe "when external tracker was enabled and then config removed" do
       before do
         @project = ext_project
diff --git a/spec/models/project_services/bamboo_service_spec.rb b/spec/models/project_services/bamboo_service_spec.rb
index c34b2487ecf..7c8b5c10779 100644
--- a/spec/models/project_services/bamboo_service_spec.rb
+++ b/spec/models/project_services/bamboo_service_spec.rb
@@ -21,74 +21,226 @@
 require 'spec_helper'
 
 describe BambooService, models: true do
-  describe "Associations" do
+  describe 'Associations' do
     it { is_expected.to belong_to :project }
     it { is_expected.to have_one :service_hook }
   end
 
-  describe "Execute" do
-    let(:user)    { create(:user) }
-    let(:project) { create(:project) }
-
-    context "when a password was previously set" do
-      before do
-        @bamboo_service = BambooService.create(
-          project: create(:project),
-          properties: {
-            bamboo_url: 'http://gitlab.com',
-            username: 'mic',
-            password: "password"
-          }
-        )
+  describe 'Validations' do
+    describe '#bamboo_url' do
+      it 'does not validate the presence of bamboo_url if service is not active' do
+        bamboo_service = service
+        bamboo_service.active = false
+
+        expect(bamboo_service).not_to validate_presence_of(:bamboo_url)
       end
-  
-      it "reset password if url changed" do
-        @bamboo_service.bamboo_url = 'http://gitlab1.com'
-        @bamboo_service.save
-        expect(@bamboo_service.password).to be_nil
+
+      it 'validates the presence of bamboo_url if service is active' do
+        bamboo_service = service
+        bamboo_service.active = true
+
+        expect(bamboo_service).to validate_presence_of(:bamboo_url)
       end
-  
-      it "does not reset password if username changed" do
-        @bamboo_service.username = "some_name"
-        @bamboo_service.save
-        expect(@bamboo_service.password).to eq("password")
+    end
+
+    describe '#build_key' do
+      it 'does not validate the presence of build_key if service is not active' do
+        bamboo_service = service
+        bamboo_service.active = false
+
+        expect(bamboo_service).not_to validate_presence_of(:build_key)
       end
 
-      it "does not reset password if new url is set together with password, even if it's the same password" do
-        @bamboo_service.bamboo_url = 'http://gitlab_edited.com'
-        @bamboo_service.password = 'password'
-        @bamboo_service.save
-        expect(@bamboo_service.password).to eq("password")
-        expect(@bamboo_service.bamboo_url).to eq("http://gitlab_edited.com")
+      it 'validates the presence of build_key if service is active' do
+        bamboo_service = service
+        bamboo_service.active = true
+
+        expect(bamboo_service).to validate_presence_of(:build_key)
       end
+    end
 
-      it "should reset password if url changed, even if setter called multiple times" do
-        @bamboo_service.bamboo_url = 'http://gitlab1.com'
-        @bamboo_service.bamboo_url = 'http://gitlab1.com'
-        @bamboo_service.save
-        expect(@bamboo_service.password).to be_nil
+    describe '#username' do
+      it 'does not validate the presence of username if service is not active' do
+        bamboo_service = service
+        bamboo_service.active = false
+
+        expect(bamboo_service).not_to validate_presence_of(:username)
+      end
+
+      it 'does not validate the presence of username if username is nil' do
+        bamboo_service = service
+        bamboo_service.active = true
+        bamboo_service.password = nil
+
+        expect(bamboo_service).not_to validate_presence_of(:username)
+      end
+
+      it 'validates the presence of username if service is active and username is present' do
+        bamboo_service = service
+        bamboo_service.active = true
+        bamboo_service.password = 'secret'
+
+        expect(bamboo_service).to validate_presence_of(:username)
       end
     end
-    
-    context "when no password was previously set" do
-      before do
-        @bamboo_service = BambooService.create(
-          project: create(:project),
-          properties: {
-            bamboo_url: 'http://gitlab.com',
-            username: 'mic'
-          }
-        )
+
+    describe '#password' do
+      it 'does not validate the presence of password if service is not active' do
+        bamboo_service = service
+        bamboo_service.active = false
+
+        expect(bamboo_service).not_to validate_presence_of(:password)
       end
 
-      it "saves password if new url is set together with password" do
-        @bamboo_service.bamboo_url = 'http://gitlab_edited.com'
-        @bamboo_service.password = 'password'
-        @bamboo_service.save
-        expect(@bamboo_service.password).to eq("password")
-        expect(@bamboo_service.bamboo_url).to eq("http://gitlab_edited.com")
+      it 'does not validate the presence of password if username is nil' do
+        bamboo_service = service
+        bamboo_service.active = true
+        bamboo_service.username = nil
+
+        expect(bamboo_service).not_to validate_presence_of(:password)
       end
 
+      it 'validates the presence of password if service is active and username is present' do
+        bamboo_service = service
+        bamboo_service.active = true
+        bamboo_service.username = 'john'
+
+        expect(bamboo_service).to validate_presence_of(:password)
+      end
+    end
+  end
+
+  describe 'Callbacks' do
+    describe 'before_update :reset_password' do
+      context 'when a password was previously set' do
+        it 'resets password if url changed' do
+          bamboo_service = service
+
+          bamboo_service.bamboo_url = 'http://gitlab1.com'
+          bamboo_service.save
+
+          expect(bamboo_service.password).to be_nil
+        end
+
+        it 'does not reset password if username changed' do
+          bamboo_service = service
+
+          bamboo_service.username = 'some_name'
+          bamboo_service.save
+
+          expect(bamboo_service.password).to eq('password')
+        end
+
+        it "does not reset password if new url is set together with password, even if it's the same password" do
+          bamboo_service = service
+
+          bamboo_service.bamboo_url = 'http://gitlab_edited.com'
+          bamboo_service.password = 'password'
+          bamboo_service.save
+
+          expect(bamboo_service.password).to eq('password')
+          expect(bamboo_service.bamboo_url).to eq('http://gitlab_edited.com')
+        end
+      end
+
+      it 'saves password if new url is set together with password when no password was previously set' do
+        bamboo_service = service
+        bamboo_service.password = nil
+
+        bamboo_service.bamboo_url = 'http://gitlab_edited.com'
+        bamboo_service.password = 'password'
+        bamboo_service.save
+
+        expect(bamboo_service.password).to eq('password')
+        expect(bamboo_service.bamboo_url).to eq('http://gitlab_edited.com')
+      end
     end
   end
+
+  describe '#build_page' do
+    it 'returns a specific URL when status is 500' do
+      stub_request(status: 500)
+
+      expect(service.build_page('123', 'unused')).to eq('http://gitlab.com/browse/foo')
+    end
+
+    it 'returns a specific URL when response has no results' do
+      stub_request(body: %Q({"results":{"results":{"size":"0"}}}))
+
+      expect(service.build_page('123', 'unused')).to eq('http://gitlab.com/browse/foo')
+    end
+
+    it 'returns a build URL when bamboo_url has no trailing slash' do
+      stub_request(body: %Q({"results":{"results":{"result":{"planResultKey":{"key":"42"}}}}}))
+
+      expect(service(bamboo_url: 'http://gitlab.com').build_page('123', 'unused')).to eq('http://gitlab.com/browse/42')
+    end
+  end
+
+  describe '#commit_status' do
+    it 'sets commit status to :error when status is 500' do
+      stub_request(status: 500)
+
+      expect(service.commit_status('123', 'unused')).to eq(:error)
+    end
+
+    it 'sets commit status to "pending" when status is 404' do
+      stub_request(status: 404)
+
+      expect(service.commit_status('123', 'unused')).to eq('pending')
+    end
+
+    it 'sets commit status to "pending" when response has no results' do
+      stub_request(body: %Q({"results":{"results":{"size":"0"}}}))
+
+      expect(service.commit_status('123', 'unused')).to eq('pending')
+    end
+
+    it 'sets commit status to "success" when build state contains Success' do
+      stub_request(build_state: 'YAY Success!')
+
+      expect(service.commit_status('123', 'unused')).to eq('success')
+    end
+
+    it 'sets commit status to "failed" when build state contains Failed' do
+      stub_request(build_state: 'NO Failed!')
+
+      expect(service.commit_status('123', 'unused')).to eq('failed')
+    end
+
+    it 'sets commit status to "pending" when build state contains Pending' do
+      stub_request(build_state: 'NO Pending!')
+
+      expect(service.commit_status('123', 'unused')).to eq('pending')
+    end
+
+    it 'sets commit status to :error when build state is unknown' do
+      stub_request(build_state: 'FOO BAR!')
+
+      expect(service.commit_status('123', 'unused')).to eq(:error)
+    end
+  end
+
+  def service(bamboo_url: 'http://gitlab.com')
+    described_class.create(
+      project: build_stubbed(:empty_project),
+      properties: {
+        bamboo_url: bamboo_url,
+        username: 'mic',
+        password: 'password',
+        build_key: 'foo'
+      }
+    )
+  end
+
+  def stub_request(status: 200, body: nil, build_state: 'success')
+    bamboo_full_url = 'http://mic:password@gitlab.com/rest/api/latest/result?label=123&os_authType=basic'
+    body ||= %Q({"results":{"results":{"result":{"buildState":"#{build_state}"}}}})
+
+    WebMock.stub_request(:get, bamboo_full_url).to_return(
+      status: status,
+      headers: { 'Content-Type' => 'application/json' },
+      body: body
+    )
+  end
 end
diff --git a/spec/models/project_services/buildkite_service_spec.rb b/spec/models/project_services/buildkite_service_spec.rb
index 88cd624877a..60364df2015 100644
--- a/spec/models/project_services/buildkite_service_spec.rb
+++ b/spec/models/project_services/buildkite_service_spec.rb
@@ -26,6 +26,23 @@ describe BuildkiteService, models: true do
     it { is_expected.to have_one :service_hook }
   end
 
+  describe 'Validations' do
+    context 'when service is active' do
+      before { subject.active = true }
+
+      it { is_expected.to validate_presence_of(:project_url) }
+      it { is_expected.to validate_presence_of(:token) }
+      it_behaves_like 'issue tracker service URL attribute', :project_url
+    end
+
+    context 'when service is inactive' do
+      before { subject.active = false }
+
+      it { is_expected.not_to validate_presence_of(:project_url) }
+      it { is_expected.not_to validate_presence_of(:token) }
+    end
+  end
+
   describe 'commits methods' do
     before do
       @project = Project.new
diff --git a/spec/models/project_services/builds_email_service_spec.rb b/spec/models/project_services/builds_email_service_spec.rb
index 905379a64e3..163f56d699c 100644
--- a/spec/models/project_services/builds_email_service_spec.rb
+++ b/spec/models/project_services/builds_email_service_spec.rb
@@ -3,21 +3,62 @@ require 'spec_helper'
 describe BuildsEmailService do
   let(:build) { create(:ci_build) }
   let(:data) { Gitlab::BuildDataBuilder.build(build) }
-  let(:service) { BuildsEmailService.new }
+  let!(:project) { create(:project, :public, ci_id: 1) }
+  let(:service) { described_class.new(project: project, active: true) }
 
-  describe :execute do
-    it "sends email" do
+  describe '#execute' do
+    it 'sends email' do
       service.recipients = 'test@gitlab.com'
       data[:build_status] = 'failed'
       expect(BuildEmailWorker).to receive(:perform_async)
       service.execute(data)
     end
 
-    it "does not sends email with failed build and allowed_failure on" do
+    it 'does not send email with succeeded build and notify_only_broken_builds on' do
+      expect(service).to receive(:notify_only_broken_builds).and_return(true)
+      data[:build_status] = 'success'
+      expect(BuildEmailWorker).not_to receive(:perform_async)
+      service.execute(data)
+    end
+
+    it 'does not send email with failed build and build_allow_failure is true' do
       data[:build_status] = 'failed'
       data[:build_allow_failure] = true
       expect(BuildEmailWorker).not_to receive(:perform_async)
       service.execute(data)
     end
+
+    it 'does not send email with unknown build status' do
+      data[:build_status] = 'foo'
+      expect(BuildEmailWorker).not_to receive(:perform_async)
+      service.execute(data)
+    end
+  end
+
+  describe 'validations' do
+
+    context 'when pusher is not added' do
+      before { service.add_pusher = false }
+
+      it 'does not allow empty recipient input' do
+        service.recipients = ''
+        expect(service.valid?).to be false
+      end
+
+      it 'does allow non-empty recipient input' do
+        service.recipients = 'test@example.com'
+        expect(service.valid?).to be true
+      end
+
+    end
+
+    context 'when pusher is added' do
+      before { service.add_pusher = true }
+
+      it 'does allow non-empty recipient input' do
+        service.recipients = 'test@example.com'
+        expect(service.valid?).to be true
+      end
+    end
   end
 end
diff --git a/spec/models/project_services/campfire_service_spec.rb b/spec/models/project_services/campfire_service_spec.rb
new file mode 100644
index 00000000000..3e6da42803b
--- /dev/null
+++ b/spec/models/project_services/campfire_service_spec.rb
@@ -0,0 +1,42 @@
+# == Schema Information
+#
+# Table name: services
+#
+#  id                    :integer          not null, primary key
+#  type                  :string(255)
+#  title                 :string(255)
+#  project_id            :integer
+#  created_at            :datetime
+#  updated_at            :datetime
+#  active                :boolean          default(FALSE), not null
+#  properties            :text
+#  template              :boolean          default(FALSE)
+#  push_events           :boolean          default(TRUE)
+#  issues_events         :boolean          default(TRUE)
+#  merge_requests_events :boolean          default(TRUE)
+#  tag_push_events       :boolean          default(TRUE)
+#  note_events           :boolean          default(TRUE), not null
+#
+
+require 'spec_helper'
+
+describe CampfireService, models: true do
+  describe 'Associations' do
+    it { is_expected.to belong_to :project }
+    it { is_expected.to have_one :service_hook }
+  end
+
+  describe 'Validations' do
+    context 'when service is active' do
+      before { subject.active = true }
+
+      it { is_expected.to validate_presence_of(:token) }
+    end
+
+    context 'when service is inactive' do
+      before { subject.active = false }
+
+      it { is_expected.not_to validate_presence_of(:token) }
+    end
+  end
+end
diff --git a/spec/models/project_services/custom_issue_tracker_service_spec.rb b/spec/models/project_services/custom_issue_tracker_service_spec.rb
new file mode 100644
index 00000000000..ff976f8ec59
--- /dev/null
+++ b/spec/models/project_services/custom_issue_tracker_service_spec.rb
@@ -0,0 +1,49 @@
+# == Schema Information
+#
+# Table name: services
+#
+#  id                    :integer          not null, primary key
+#  type                  :string(255)
+#  title                 :string(255)
+#  project_id            :integer
+#  created_at            :datetime
+#  updated_at            :datetime
+#  active                :boolean          default(FALSE), not null
+#  properties            :text
+#  template              :boolean          default(FALSE)
+#  push_events           :boolean          default(TRUE)
+#  issues_events         :boolean          default(TRUE)
+#  merge_requests_events :boolean          default(TRUE)
+#  tag_push_events       :boolean          default(TRUE)
+#  note_events           :boolean          default(TRUE), not null
+#
+
+require 'spec_helper'
+
+describe CustomIssueTrackerService, models: true do
+  describe 'Associations' do
+    it { is_expected.to belong_to :project }
+    it { is_expected.to have_one :service_hook }
+  end
+
+  describe 'Validations' do
+    context 'when service is active' do
+      before { subject.active = true }
+
+      it { is_expected.to validate_presence_of(:project_url) }
+      it { is_expected.to validate_presence_of(:issues_url) }
+      it { is_expected.to validate_presence_of(:new_issue_url) }
+      it_behaves_like 'issue tracker service URL attribute', :project_url
+      it_behaves_like 'issue tracker service URL attribute', :issues_url
+      it_behaves_like 'issue tracker service URL attribute', :new_issue_url
+    end
+
+    context 'when service is inactive' do
+      before { subject.active = false }
+
+      it { is_expected.not_to validate_presence_of(:project_url) }
+      it { is_expected.not_to validate_presence_of(:issues_url) }
+      it { is_expected.not_to validate_presence_of(:new_issue_url) }
+    end
+  end
+end
diff --git a/spec/models/project_services/drone_ci_service_spec.rb b/spec/models/project_services/drone_ci_service_spec.rb
index a2cf68a9e38..3a8e67438fc 100644
--- a/spec/models/project_services/drone_ci_service_spec.rb
+++ b/spec/models/project_services/drone_ci_service_spec.rb
@@ -28,25 +28,18 @@ describe DroneCiService, models: true do
 
   describe 'validations' do
     context 'active' do
-      before { allow(subject).to receive(:activated?).and_return(true) }
+      before { subject.active = true }
 
       it { is_expected.to validate_presence_of(:token) }
       it { is_expected.to validate_presence_of(:drone_url) }
-      it { is_expected.to allow_value('ewf9843kdnfdfs89234n').for(:token) }
-      it { is_expected.to allow_value('http://ci.example.com').for(:drone_url) }
-      it { is_expected.not_to allow_value('this is not url').for(:drone_url) }
-      it { is_expected.not_to allow_value('http//noturl').for(:drone_url) }
-      it { is_expected.not_to allow_value('ftp://ci.example.com').for(:drone_url) }
+      it_behaves_like 'issue tracker service URL attribute', :drone_url
     end
 
     context 'inactive' do
-      before { allow(subject).to receive(:activated?).and_return(false) }
+      before { subject.active = false }
 
       it { is_expected.not_to validate_presence_of(:token) }
       it { is_expected.not_to validate_presence_of(:drone_url) }
-      it { is_expected.to allow_value('ewf9843kdnfdfs89234n').for(:token) }
-      it { is_expected.to allow_value('http://drone.example.com').for(:drone_url) }
-      it { is_expected.to allow_value('ftp://drone.example.com').for(:drone_url) }
     end
   end
 
diff --git a/spec/models/project_services/emails_on_push_service_spec.rb b/spec/models/project_services/emails_on_push_service_spec.rb
new file mode 100644
index 00000000000..e6f78898c82
--- /dev/null
+++ b/spec/models/project_services/emails_on_push_service_spec.rb
@@ -0,0 +1,17 @@
+require 'spec_helper'
+
+describe EmailsOnPushService do
+  describe 'Validations' do
+    context 'when service is active' do
+      before { subject.active = true }
+
+      it { is_expected.to validate_presence_of(:recipients) }
+    end
+
+    context 'when service is inactive' do
+      before { subject.active = false }
+
+      it { is_expected.not_to validate_presence_of(:recipients) }
+    end
+  end
+end
diff --git a/spec/models/external_wiki_service_spec.rb b/spec/models/project_services/external_wiki_service_spec.rb
index d37978720bf..5fe5ea7d2df 100644
--- a/spec/models/external_wiki_service_spec.rb
+++ b/spec/models/project_services/external_wiki_service_spec.rb
@@ -28,13 +28,18 @@ describe ExternalWikiService, models: true do
     it { should have_one :service_hook }
   end
 
-  describe "Validations" do
-    context "active" do
-      before do
-        subject.active = true
-      end
+  describe 'Validations' do
+    context 'when service is active' do
+      before { subject.active = true }
+
+      it { is_expected.to validate_presence_of(:external_wiki_url) }
+      it_behaves_like 'issue tracker service URL attribute', :external_wiki_url
+    end
+
+    context 'when service is inactive' do
+      before { subject.active = false }
 
-      it { should validate_presence_of :external_wiki_url }
+      it { is_expected.not_to validate_presence_of(:external_wiki_url) }
     end
   end
 
diff --git a/spec/models/project_services/flowdock_service_spec.rb b/spec/models/project_services/flowdock_service_spec.rb
index ff7fbcaa004..b7e627e6518 100644
--- a/spec/models/project_services/flowdock_service_spec.rb
+++ b/spec/models/project_services/flowdock_service_spec.rb
@@ -26,6 +26,20 @@ describe FlowdockService, models: true do
     it { is_expected.to have_one :service_hook }
   end
 
+  describe 'Validations' do
+    context 'when service is active' do
+      before { subject.active = true }
+
+      it { is_expected.to validate_presence_of(:token) }
+    end
+
+    context 'when service is inactive' do
+      before { subject.active = false }
+
+      it { is_expected.not_to validate_presence_of(:token) }
+    end
+  end
+
   describe "Execute" do
     let(:user)    { create(:user) }
     let(:project) { create(:project) }
diff --git a/spec/models/project_services/gemnasium_service_spec.rb b/spec/models/project_services/gemnasium_service_spec.rb
index ecb3ccb1673..a08f1ac229f 100644
--- a/spec/models/project_services/gemnasium_service_spec.rb
+++ b/spec/models/project_services/gemnasium_service_spec.rb
@@ -26,6 +26,22 @@ describe GemnasiumService, models: true do
     it { is_expected.to have_one :service_hook }
   end
 
+  describe 'Validations' do
+    context 'when service is active' do
+      before { subject.active = true }
+
+      it { is_expected.to validate_presence_of(:token) }
+      it { is_expected.to validate_presence_of(:api_key) }
+    end
+
+    context 'when service is inactive' do
+      before { subject.active = false }
+
+      it { is_expected.not_to validate_presence_of(:token) }
+      it { is_expected.not_to validate_presence_of(:api_key) }
+    end
+  end
+
   describe "Execute" do
     let(:user)    { create(:user) }
     let(:project) { create(:project) }
diff --git a/spec/models/project_services/gitlab_issue_tracker_service_spec.rb b/spec/models/project_services/gitlab_issue_tracker_service_spec.rb
index 3518dbd1728..7a1f106d6e3 100644
--- a/spec/models/project_services/gitlab_issue_tracker_service_spec.rb
+++ b/spec/models/project_services/gitlab_issue_tracker_service_spec.rb
@@ -26,6 +26,20 @@ describe GitlabIssueTrackerService, models: true do
     it { is_expected.to have_one :service_hook }
   end
 
+  describe 'Validations' do
+    context 'when service is active' do
+      subject { described_class.new(project: create(:project), active: true) }
+
+      it { is_expected.to validate_presence_of(:issues_url) }
+      it_behaves_like 'issue tracker service URL attribute', :issues_url
+    end
+
+    context 'when service is inactive' do
+      subject { described_class.new(project: create(:project), active: false) }
+
+      it { is_expected.not_to validate_presence_of(:issues_url) }
+    end
+  end
 
   describe 'project and issue urls' do
     let(:project) { create(:project) }
diff --git a/spec/models/project_services/hipchat_service_spec.rb b/spec/models/project_services/hipchat_service_spec.rb
index 91dd92b7c67..523c5da6966 100644
--- a/spec/models/project_services/hipchat_service_spec.rb
+++ b/spec/models/project_services/hipchat_service_spec.rb
@@ -26,6 +26,20 @@ describe HipchatService, models: true do
     it { is_expected.to have_one :service_hook }
   end
 
+  describe 'Validations' do
+    context 'when service is active' do
+      before { subject.active = true }
+
+      it { is_expected.to validate_presence_of(:token) }
+    end
+
+    context 'when service is inactive' do
+      before { subject.active = false }
+
+      it { is_expected.not_to validate_presence_of(:token) }
+    end
+  end
+
   describe "Execute" do
     let(:hipchat) { HipchatService.new }
     let(:user)    { create(:user, username: 'username') }
diff --git a/spec/models/project_services/irker_service_spec.rb b/spec/models/project_services/irker_service_spec.rb
index b783b1a576e..4ee022a5171 100644
--- a/spec/models/project_services/irker_service_spec.rb
+++ b/spec/models/project_services/irker_service_spec.rb
@@ -29,14 +29,16 @@ describe IrkerService, models: true do
   end
 
   describe 'Validations' do
-    before do
-      subject.active = true
-      subject.properties['recipients'] = _recipients
+    context 'when service is active' do
+      before { subject.active = true }
+
+      it { is_expected.to validate_presence_of(:recipients) }
     end
 
-    context 'active' do
-      let(:_recipients) { nil }
-      it { should validate_presence_of :recipients }
+    context 'when service is inactive' do
+      before { subject.active = false }
+
+      it { is_expected.not_to validate_presence_of(:recipients) }
     end
   end
 
diff --git a/spec/models/project_services/jira_service_spec.rb b/spec/models/project_services/jira_service_spec.rb
index 2f8193170ae..5309cfb99ff 100644
--- a/spec/models/project_services/jira_service_spec.rb
+++ b/spec/models/project_services/jira_service_spec.rb
@@ -26,6 +26,30 @@ describe JiraService, models: true do
     it { is_expected.to have_one :service_hook }
   end
 
+  describe 'Validations' do
+    context 'when service is active' do
+      before { subject.active = true }
+
+      it { is_expected.to validate_presence_of(:api_url) }
+      it { is_expected.to validate_presence_of(:project_url) }
+      it { is_expected.to validate_presence_of(:issues_url) }
+      it { is_expected.to validate_presence_of(:new_issue_url) }
+      it_behaves_like 'issue tracker service URL attribute', :api_url
+      it_behaves_like 'issue tracker service URL attribute', :project_url
+      it_behaves_like 'issue tracker service URL attribute', :issues_url
+      it_behaves_like 'issue tracker service URL attribute', :new_issue_url
+    end
+
+    context 'when service is inactive' do
+      before { subject.active = false }
+
+      it { is_expected.not_to validate_presence_of(:api_url) }
+      it { is_expected.not_to validate_presence_of(:project_url) }
+      it { is_expected.not_to validate_presence_of(:issues_url) }
+      it { is_expected.not_to validate_presence_of(:new_issue_url) }
+    end
+  end
+
   describe "Execute" do
     let(:user)    { create(:user) }
     let(:project) { create(:project) }
@@ -72,7 +96,7 @@ describe JiraService, models: true do
 
     context "when a password was previously set" do
       before do
-        @jira_service = JiraService.create(
+        @jira_service = JiraService.create!(
           project: create(:project),
           properties: {
             api_url: 'http://jira.example.com/rest/api/2',
diff --git a/spec/models/project_services/pivotaltracker_service_spec.rb b/spec/models/project_services/pivotaltracker_service_spec.rb
new file mode 100644
index 00000000000..f37edd4d970
--- /dev/null
+++ b/spec/models/project_services/pivotaltracker_service_spec.rb
@@ -0,0 +1,42 @@
+# == Schema Information
+#
+# Table name: services
+#
+#  id                    :integer          not null, primary key
+#  type                  :string(255)
+#  title                 :string(255)
+#  project_id            :integer
+#  created_at            :datetime
+#  updated_at            :datetime
+#  active                :boolean          default(FALSE), not null
+#  properties            :text
+#  template              :boolean          default(FALSE)
+#  push_events           :boolean          default(TRUE)
+#  issues_events         :boolean          default(TRUE)
+#  merge_requests_events :boolean          default(TRUE)
+#  tag_push_events       :boolean          default(TRUE)
+#  note_events           :boolean          default(TRUE), not null
+#
+
+require 'spec_helper'
+
+describe PivotaltrackerService, models: true do
+  describe 'Associations' do
+    it { is_expected.to belong_to :project }
+    it { is_expected.to have_one :service_hook }
+  end
+
+  describe 'Validations' do
+    context 'when service is active' do
+      before { subject.active = true }
+
+      it { is_expected.to validate_presence_of(:token) }
+    end
+
+    context 'when service is inactive' do
+      before { subject.active = false }
+
+      it { is_expected.not_to validate_presence_of(:token) }
+    end
+  end
+end
diff --git a/spec/models/project_services/pushover_service_spec.rb b/spec/models/project_services/pushover_service_spec.rb
index 96039f9491b..555d9757b47 100644
--- a/spec/models/project_services/pushover_service_spec.rb
+++ b/spec/models/project_services/pushover_service_spec.rb
@@ -27,14 +27,20 @@ describe PushoverService, models: true do
   end
 
   describe 'Validations' do
-    context 'active' do
-      before do
-        subject.active = true
-      end
+    context 'when service is active' do
+      before { subject.active = true }
 
-      it { is_expected.to validate_presence_of :api_key }
-      it { is_expected.to validate_presence_of :user_key }
-      it { is_expected.to validate_presence_of :priority }
+      it { is_expected.to validate_presence_of(:api_key) }
+      it { is_expected.to validate_presence_of(:user_key) }
+      it { is_expected.to validate_presence_of(:priority) }
+    end
+
+    context 'when service is inactive' do
+      before { subject.active = false }
+
+      it { is_expected.not_to validate_presence_of(:api_key) }
+      it { is_expected.not_to validate_presence_of(:user_key) }
+      it { is_expected.not_to validate_presence_of(:priority) }
     end
   end
 
diff --git a/spec/models/project_services/redmine_service_spec.rb b/spec/models/project_services/redmine_service_spec.rb
new file mode 100644
index 00000000000..7d14f6e8280
--- /dev/null
+++ b/spec/models/project_services/redmine_service_spec.rb
@@ -0,0 +1,49 @@
+# == Schema Information
+#
+# Table name: services
+#
+#  id                    :integer          not null, primary key
+#  type                  :string(255)
+#  title                 :string(255)
+#  project_id            :integer
+#  created_at            :datetime
+#  updated_at            :datetime
+#  active                :boolean          default(FALSE), not null
+#  properties            :text
+#  template              :boolean          default(FALSE)
+#  push_events           :boolean          default(TRUE)
+#  issues_events         :boolean          default(TRUE)
+#  merge_requests_events :boolean          default(TRUE)
+#  tag_push_events       :boolean          default(TRUE)
+#  note_events           :boolean          default(TRUE), not null
+#
+
+require 'spec_helper'
+
+describe RedmineService, models: true do
+  describe 'Associations' do
+    it { is_expected.to belong_to :project }
+    it { is_expected.to have_one :service_hook }
+  end
+
+  describe 'Validations' do
+    context 'when service is active' do
+      before { subject.active = true }
+
+      it { is_expected.to validate_presence_of(:project_url) }
+      it { is_expected.to validate_presence_of(:issues_url) }
+      it { is_expected.to validate_presence_of(:new_issue_url) }
+      it_behaves_like 'issue tracker service URL attribute', :project_url
+      it_behaves_like 'issue tracker service URL attribute', :issues_url
+      it_behaves_like 'issue tracker service URL attribute', :new_issue_url
+    end
+
+    context 'when service is inactive' do
+      before { subject.active = false }
+
+      it { is_expected.not_to validate_presence_of(:project_url) }
+      it { is_expected.not_to validate_presence_of(:issues_url) }
+      it { is_expected.not_to validate_presence_of(:new_issue_url) }
+    end
+  end
+end
diff --git a/spec/models/project_services/slack_service_spec.rb b/spec/models/project_services/slack_service_spec.rb
index a9e0afad90f..e2ef24659bd 100644
--- a/spec/models/project_services/slack_service_spec.rb
+++ b/spec/models/project_services/slack_service_spec.rb
@@ -26,13 +26,18 @@ describe SlackService, models: true do
     it { is_expected.to have_one :service_hook }
   end
 
-  describe "Validations" do
-    context "active" do
-      before do
-        subject.active = true
-      end
+  describe 'Validations' do
+    context 'when service is active' do
+      before { subject.active = true }
 
-      it { is_expected.to validate_presence_of :webhook }
+      it { is_expected.to validate_presence_of(:webhook) }
+      it_behaves_like 'issue tracker service URL attribute', :webhook
+    end
+
+    context 'when service is inactive' do
+      before { subject.active = false }
+
+      it { is_expected.not_to validate_presence_of(:webhook) }
     end
   end
 
diff --git a/spec/models/project_services/teamcity_service_spec.rb b/spec/models/project_services/teamcity_service_spec.rb
index f26b47a856c..339d641342a 100644
--- a/spec/models/project_services/teamcity_service_spec.rb
+++ b/spec/models/project_services/teamcity_service_spec.rb
@@ -21,73 +21,214 @@
 require 'spec_helper'
 
 describe TeamcityService, models: true do
-  describe "Associations" do
+  describe 'Associations' do
     it { is_expected.to belong_to :project }
     it { is_expected.to have_one :service_hook }
   end
 
-  describe "Execute" do
-    let(:user)    { create(:user) }
-    let(:project) { create(:project) }
-
-    context "when a password was previously set" do
-      before do
-        @teamcity_service = TeamcityService.create(
-          project: create(:project),
-          properties: {
-            teamcity_url: 'http://gitlab.com',
-            username: 'mic',
-            password: "password"
-          }
-        )
+  describe 'Validations' do
+    describe '#teamcity_url' do
+      it 'does not validate the presence of teamcity_url if service is not active' do
+        teamcity_service = service
+        teamcity_service.active = false
+
+        expect(teamcity_service).not_to validate_presence_of(:teamcity_url)
+      end
+
+      it 'validates the presence of teamcity_url if service is active' do
+        teamcity_service = service
+        teamcity_service.active = true
+
+        expect(teamcity_service).to validate_presence_of(:teamcity_url)
+      end
+    end
+
+    describe '#build_type' do
+      it 'does not validate the presence of build_type if service is not active' do
+        teamcity_service = service
+        teamcity_service.active = false
+
+        expect(teamcity_service).not_to validate_presence_of(:build_type)
+      end
+
+      it 'validates the presence of build_type if service is active' do
+        teamcity_service = service
+        teamcity_service.active = true
+
+        expect(teamcity_service).to validate_presence_of(:build_type)
+      end
+    end
+
+    describe '#username' do
+      it 'does not validate the presence of username if service is not active' do
+        teamcity_service = service
+        teamcity_service.active = false
+
+        expect(teamcity_service).not_to validate_presence_of(:username)
+      end
+
+      it 'does not validate the presence of username if username is nil' do
+        teamcity_service = service
+        teamcity_service.active = true
+        teamcity_service.password = nil
+
+        expect(teamcity_service).not_to validate_presence_of(:username)
       end
-  
-      it "reset password if url changed" do
-        @teamcity_service.teamcity_url = 'http://gitlab1.com'
-        @teamcity_service.save
-        expect(@teamcity_service.password).to be_nil
+
+      it 'validates the presence of username if service is active and username is present' do
+        teamcity_service = service
+        teamcity_service.active = true
+        teamcity_service.password = 'secret'
+
+        expect(teamcity_service).to validate_presence_of(:username)
       end
-  
-      it "does not reset password if username changed" do
-        @teamcity_service.username = "some_name"
-        @teamcity_service.save
-        expect(@teamcity_service.password).to eq("password")
+    end
+
+    describe '#password' do
+      it 'does not validate the presence of password if service is not active' do
+        teamcity_service = service
+        teamcity_service.active = false
+
+        expect(teamcity_service).not_to validate_presence_of(:password)
       end
 
-      it "does not reset password if new url is set together with password, even if it's the same password" do
-        @teamcity_service.teamcity_url = 'http://gitlab_edited.com'
-        @teamcity_service.password = 'password'
-        @teamcity_service.save
-        expect(@teamcity_service.password).to eq("password")
-        expect(@teamcity_service.teamcity_url).to eq("http://gitlab_edited.com")
+      it 'does not validate the presence of password if username is nil' do
+        teamcity_service = service
+        teamcity_service.active = true
+        teamcity_service.username = nil
+
+        expect(teamcity_service).not_to validate_presence_of(:password)
       end
 
-      it "should reset password if url changed, even if setter called multiple times" do
-        @teamcity_service.teamcity_url = 'http://gitlab1.com'
-        @teamcity_service.teamcity_url = 'http://gitlab1.com'
-        @teamcity_service.save
-        expect(@teamcity_service.password).to be_nil
+      it 'validates the presence of password if service is active and username is present' do
+        teamcity_service = service
+        teamcity_service.active = true
+        teamcity_service.username = 'john'
+
+        expect(teamcity_service).to validate_presence_of(:password)
       end
     end
-    
-    context "when no password was previously set" do
-      before do
-        @teamcity_service = TeamcityService.create(
-          project: create(:project),
-          properties: {
-            teamcity_url: 'http://gitlab.com',
-            username: 'mic'
-          }
-        )
+  end
+
+  describe 'Callbacks' do
+    describe 'before_update :reset_password' do
+      context 'when a password was previously set' do
+        it 'resets password if url changed' do
+          teamcity_service = service
+
+          teamcity_service.teamcity_url = 'http://gitlab1.com'
+          teamcity_service.save
+
+          expect(teamcity_service.password).to be_nil
+        end
+
+        it 'does not reset password if username changed' do
+          teamcity_service = service
+
+          teamcity_service.username = 'some_name'
+          teamcity_service.save
+
+          expect(teamcity_service.password).to eq('password')
+        end
+
+        it "does not reset password if new url is set together with password, even if it's the same password" do
+          teamcity_service = service
+
+          teamcity_service.teamcity_url = 'http://gitlab_edited.com'
+          teamcity_service.password = 'password'
+          teamcity_service.save
+
+          expect(teamcity_service.password).to eq('password')
+          expect(teamcity_service.teamcity_url).to eq('http://gitlab_edited.com')
+        end
       end
 
-      it "saves password if new url is set together with password" do
-        @teamcity_service.teamcity_url = 'http://gitlab_edited.com'
-        @teamcity_service.password = 'password'
-        @teamcity_service.save
-        expect(@teamcity_service.password).to eq("password")
-        expect(@teamcity_service.teamcity_url).to eq("http://gitlab_edited.com")
+      it 'saves password if new url is set together with password when no password was previously set' do
+        teamcity_service = service
+        teamcity_service.password = nil
+
+        teamcity_service.teamcity_url = 'http://gitlab_edited.com'
+        teamcity_service.password = 'password'
+        teamcity_service.save
+
+        expect(teamcity_service.password).to eq('password')
+        expect(teamcity_service.teamcity_url).to eq('http://gitlab_edited.com')
       end
     end
   end
+
+  describe '#build_page' do
+    it 'returns a specific URL when status is 500' do
+      stub_request(status: 500)
+
+      expect(service.build_page('123', 'unused')).to eq('http://gitlab.com/viewLog.html?buildTypeId=foo')
+    end
+
+    it 'returns a build URL when teamcity_url has no trailing slash' do
+      stub_request(body: %Q({"build":{"id":"666"}}))
+
+      expect(service(teamcity_url: 'http://gitlab.com').build_page('123', 'unused')).to eq('http://gitlab.com/viewLog.html?buildId=666&buildTypeId=foo')
+    end
+  end
+
+  describe '#commit_status' do
+    it 'sets commit status to :error when status is 500' do
+      stub_request(status: 500)
+
+      expect(service.commit_status('123', 'unused')).to eq(:error)
+    end
+
+    it 'sets commit status to "pending" when status is 404' do
+      stub_request(status: 404)
+
+      expect(service.commit_status('123', 'unused')).to eq('pending')
+    end
+
+    it 'sets commit status to "success" when build status contains SUCCESS' do
+      stub_request(build_status: 'YAY SUCCESS!')
+
+      expect(service.commit_status('123', 'unused')).to eq('success')
+    end
+
+    it 'sets commit status to "failed" when build status contains FAILURE' do
+      stub_request(build_status: 'NO FAILURE!')
+
+      expect(service.commit_status('123', 'unused')).to eq('failed')
+    end
+
+    it 'sets commit status to "pending" when build status contains Pending' do
+      stub_request(build_status: 'NO Pending!')
+
+      expect(service.commit_status('123', 'unused')).to eq('pending')
+    end
+
+    it 'sets commit status to :error when build status is unknown' do
+      stub_request(build_status: 'FOO BAR!')
+
+      expect(service.commit_status('123', 'unused')).to eq(:error)
+    end
+  end
+
+  def service(teamcity_url: 'http://gitlab.com')
+    described_class.create(
+      project: build_stubbed(:empty_project),
+      properties: {
+        teamcity_url: teamcity_url,
+        username: 'mic',
+        password: 'password',
+        build_type: 'foo'
+      }
+    )
+  end
+
+  def stub_request(status: 200, body: nil, build_status: 'success')
+    teamcity_full_url = 'http://mic:password@gitlab.com/httpAuth/app/rest/builds/branch:unspecified:any,number:123'
+    body ||= %Q({"build":{"status":"#{build_status}","id":"666"}})
+
+    WebMock.stub_request(:get, teamcity_full_url).to_return(
+      status: status,
+      headers: { 'Content-Type' => 'application/json' },
+      body: body
+    )
+  end
 end
diff --git a/spec/support/issue_tracker_service_shared_example.rb b/spec/support/issue_tracker_service_shared_example.rb
new file mode 100644
index 00000000000..b6d7436c360
--- /dev/null
+++ b/spec/support/issue_tracker_service_shared_example.rb
@@ -0,0 +1,7 @@
+RSpec.shared_examples 'issue tracker service URL attribute' do |url_attr|
+  it { is_expected.to allow_value('https://example.com').for(url_attr) }
+
+  it { is_expected.not_to allow_value('example.com').for(url_attr) }
+  it { is_expected.not_to allow_value('ftp://example.com').for(url_attr) }
+  it { is_expected.not_to allow_value('herp-and-derp').for(url_attr) }
+end