Merge branch 'validate_token_and_url_format_for_gitlab_ci' into 'master'

Validate format of project_url and token for GitLab CI service.

If `project_url` and `token` for are invalid, [service_hook creation](https://gitlab.com/gitlab-org/gitlab-ce/blob/7-13-stable/app/models/project_services/gitlab_ci_service.rb#L30-34) will silently fail due to validation of URL in `WebHook`.

Given that token is a sequence of numbers and letters for GitLab CI making sure that there are no unexpected characters should be enough to prevent service_hook being nil. 

Fixes #1997 

See merge request !987

4 files changed, 47 insertions, 4 deletions

diff --git a/app/models/project_services/gitlab_ci_service.rb b/app/models/project_services/gitlab_ci_service.rb
index c284e19fe50..5aaa4e85cbc 100644
--- a/app/models/project_services/gitlab_ci_service.rb
+++ b/app/models/project_services/gitlab_ci_service.rb
@@ -22,8 +22,12 @@ class GitlabCiService < CiService
   API_PREFIX = "api/v1"
 
   prop_accessor :project_url, :token
-  validates :project_url, presence: true, if: :activated?
-  validates :token, presence: true, if: :activated?
+  validates :project_url,
+    presence: true,
+    format: { with: /\A#{URI.regexp(%w(http https))}\z/, message: "should be a valid url" }, if: :activated?
+  validates :token,
+    presence: true,
+    format: { with: /\A([A-Za-z0-9]+)\z/ },  if: :activated?
 
   after_save :compose_service_hook, if: :activated?
 
diff --git a/spec/models/project_services/gitlab_ci_service_spec.rb b/spec/models/project_services/gitlab_ci_service_spec.rb
index fedc37c9b94..a14384c87b4 100644
--- a/spec/models/project_services/gitlab_ci_service_spec.rb
+++ b/spec/models/project_services/gitlab_ci_service_spec.rb
@@ -26,6 +26,33 @@ describe GitlabCiService do
     it { is_expected.to have_one(:service_hook) }
   end
 
+  describe 'validations' do
+    context 'active' do
+      before { allow(subject).to receive(:activated?).and_return(true) }
+
+      it { is_expected.to validate_presence_of(:token) }
+      it { is_expected.to validate_presence_of(:project_url) }
+      it { is_expected.to allow_value('ewf9843kdnfdfs89234n').for(:token) }
+      it { is_expected.to allow_value('http://ci.example.com/project/1').for(:project_url) }
+      it { is_expected.not_to allow_value('token with spaces').for(:token) }
+      it { is_expected.not_to allow_value('token/with%spaces').for(:token) }
+      it { is_expected.not_to allow_value('this is not url').for(:project_url) }
+      it { is_expected.not_to allow_value('http//noturl').for(:project_url) }
+      it { is_expected.not_to allow_value('ftp://ci.example.com/projects/3').for(:project_url) }
+    end
+
+    context 'inactive' do
+      before { allow(subject).to receive(:activated?).and_return(false) }
+
+      it { is_expected.not_to validate_presence_of(:token) }
+      it { is_expected.not_to validate_presence_of(:project_url) }
+      it { is_expected.to allow_value('ewf9843kdnfdfs89234n').for(:token) }
+      it { is_expected.to allow_value('http://ci.example.com/project/1').for(:project_url) }
+      it { is_expected.to allow_value('token with spaces').for(:token) }
+      it { is_expected.to allow_value('ftp://ci.example.com/projects/3').for(:project_url) }
+    end
+  end
+
   describe 'commits methods' do
     before do
       @service = GitlabCiService.new
diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb
index 4178bb2e836..5bd8206b890 100644
--- a/spec/requests/api/projects_spec.rb
+++ b/spec/requests/api/projects_spec.rb
@@ -89,7 +89,7 @@ describe API::API, api: true  do
 
         it 'returns projects in the correct order when ci_enabled_first parameter is passed' do
           [project, project2, project3].each{ |project| project.build_missing_services }
-          project2.gitlab_ci_service.update(active: true, token: "token", project_url: "url")
+          project2.gitlab_ci_service.update(active: true, token: "token", project_url: "http://ci.example.com/projects/1")
           get api('/projects', user), { ci_enabled_first: 'true' }
           expect(response.status).to eq(200)
           expect(json_response).to be_an Array
diff --git a/spec/requests/api/services_spec.rb b/spec/requests/api/services_spec.rb
index 51c543578df..6d29a28580a 100644
--- a/spec/requests/api/services_spec.rb
+++ b/spec/requests/api/services_spec.rb
@@ -7,7 +7,7 @@ describe API::API, api: true  do
 
   describe "POST /projects/:id/services/gitlab-ci" do
     it "should update gitlab-ci settings" do
-      put api("/projects/#{project.id}/services/gitlab-ci", user), token: 'secret-token', project_url: "http://ci.example.com/projects/1"
+      put api("/projects/#{project.id}/services/gitlab-ci", user), token: 'secrettoken', project_url: "http://ci.example.com/projects/1"
 
       expect(response.status).to eq(200)
     end
@@ -17,6 +17,18 @@ describe API::API, api: true  do
 
       expect(response.status).to eq(400)
     end
+
+    it "should return if the format of token is invalid" do
+      put api("/projects/#{project.id}/services/gitlab-ci", user), token: 'token-with dashes and spaces%', project_url: "http://ci.example.com/projects/1", active: true
+
+      expect(response.status).to eq(404)
+    end
+
+    it "should return if the format of token is invalid" do
+      put api("/projects/#{project.id}/services/gitlab-ci", user), token: 'token-with dashes and spaces%', project_url: "ftp://ci.example/projects/1", active: true
+
+      expect(response.status).to eq(404)
+    end
   end
 
   describe "DELETE /projects/:id/services/gitlab-ci" do