Merge remote-tracking branch 'dev/master' into 'master'

13 files changed, 96 insertions, 66 deletions

diff --git a/app/controllers/admin/application_controller.rb b/app/controllers/admin/application_controller.rb
index 9083bfb41cf..cf795d977ce 100644
--- a/app/controllers/admin/application_controller.rb
+++ b/app/controllers/admin/application_controller.rb
@@ -6,12 +6,6 @@ class Admin::ApplicationController < ApplicationController
   layout 'admin'
 
   def authenticate_admin!
-    return render_404 unless current_user.is_admin?
-  end
-
-  def authorize_impersonator!
-    if session[:impersonator_id]
-      User.find_by!(username: session[:impersonator_id]).admin?
-    end
+    render_404 unless current_user.is_admin?
   end
 end
diff --git a/app/controllers/admin/impersonation_controller.rb b/app/controllers/admin/impersonation_controller.rb
deleted file mode 100644
index bf98af78615..00000000000
--- a/app/controllers/admin/impersonation_controller.rb
+++ /dev/null
@@ -1,38 +0,0 @@
-class Admin::ImpersonationController < Admin::ApplicationController
-  skip_before_action :authenticate_admin!, only: :destroy
-
-  before_action :user
-  before_action :authorize_impersonator!
-
-  def create
-    if @user.blocked?
-      flash[:alert] = "You cannot impersonate a blocked user"
-
-      redirect_to admin_user_path(@user)
-    else
-      session[:impersonator_id] = current_user.username
-      session[:impersonator_return_to] = admin_user_path(@user)
-
-      warden.set_user(user, scope: 'user')
-
-      flash[:alert] = "You are impersonating #{user.username}."
-
-      redirect_to root_path
-    end
-  end
-
-  def destroy
-    redirect = session[:impersonator_return_to]
-
-    warden.set_user(user, scope: 'user')
-
-    session[:impersonator_return_to] = nil
-    session[:impersonator_id] = nil
-
-    redirect_to redirect || root_path
-  end
-
-  def user
-    @user ||= User.find_by!(username: params[:id] || session[:impersonator_id])
-  end
-end
diff --git a/app/controllers/admin/impersonations_controller.rb b/app/controllers/admin/impersonations_controller.rb
new file mode 100644
index 00000000000..2db824c87ef
--- /dev/null
+++ b/app/controllers/admin/impersonations_controller.rb
@@ -0,0 +1,24 @@
+class Admin::ImpersonationsController < Admin::ApplicationController
+  skip_before_action :authenticate_admin!
+  before_action :authenticate_impersonator!
+
+  def destroy
+    original_user = current_user
+
+    warden.set_user(impersonator, scope: :user)
+
+    session[:impersonator_id] = nil
+
+    redirect_to admin_user_path(original_user)
+  end
+
+  private
+
+  def impersonator
+    @impersonator ||= User.find(session[:impersonator_id]) if session[:impersonator_id]
+  end
+
+  def authenticate_impersonator!
+    render_404 unless impersonator && impersonator.is_admin? && !impersonator.blocked?
+  end
+end
diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb
index 9abf08d0e19..b8976fa09a9 100644
--- a/app/controllers/admin/users_controller.rb
+++ b/app/controllers/admin/users_controller.rb
@@ -31,6 +31,22 @@ class Admin::UsersController < Admin::ApplicationController
     user
   end
 
+  def impersonate
+    if user.blocked?
+      flash[:alert] = "You cannot impersonate a blocked user"
+
+      redirect_to admin_user_path(user)
+    else
+      session[:impersonator_id] = current_user.id
+
+      warden.set_user(user, scope: :user)
+
+      flash[:alert] = "You are now impersonating #{user.username}"
+
+      redirect_to root_path
+    end
+  end
+
   def block
     if user.block
       redirect_back_or_admin_user(notice: "Successfully blocked")
diff --git a/app/finders/snippets_finder.rb b/app/finders/snippets_finder.rb
index a41172816b8..01cbf91c658 100644
--- a/app/finders/snippets_finder.rb
+++ b/app/finders/snippets_finder.rb
@@ -51,7 +51,7 @@ class SnippetsFinder
     snippets = project.snippets.fresh
 
     if current_user
-      if project.team.member?(current_user.id)
+      if project.team.member?(current_user.id) || current_user.admin?
         snippets
       else
         snippets.public_and_internal
diff --git a/app/helpers/issues_helper.rb b/app/helpers/issues_helper.rb
index afe1e11a0da..198d39455d7 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 1ed42c4f3e7..b4418ba9284 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 fd65027f084..7092b757549 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/app/services/merge_requests/build_service.rb b/app/services/merge_requests/build_service.rb
index fa34753c4fd..3544752d47a 100644
--- a/app/services/merge_requests/build_service.rb
+++ b/app/services/merge_requests/build_service.rb
@@ -7,6 +7,9 @@ module MergeRequests
       merge_request.can_be_created = false
       merge_request.compare_commits = []
       merge_request.source_project = project unless merge_request.source_project
+
+      merge_request.target_project = nil unless can?(current_user, :read_project, merge_request.target_project)
+
       merge_request.target_project ||= (project.forked_from_project || project)
       merge_request.target_branch ||= merge_request.target_project.default_branch
 
diff --git a/app/services/notes/create_service.rb b/app/services/notes/create_service.rb
index 2bb312bb252..01586994813 100644
--- a/app/services/notes/create_service.rb
+++ b/app/services/notes/create_service.rb
@@ -5,6 +5,8 @@ module Notes
       note.author = current_user
       note.system = false
 
+      return unless valid_project?(note)
+
       if note.save
         # Finish the harder work in the background
         NewNoteWorker.perform_in(2.seconds, note.id, params)
@@ -13,5 +15,14 @@ module Notes
 
       note
     end
+
+    private
+
+    def valid_project?(note)
+      return false unless project
+      return true if note.for_commit?
+
+      note.noteable.try(:project) == project
+    end
   end
 end
diff --git a/app/views/layouts/header/_default.html.haml b/app/views/layouts/header/_default.html.haml
index 3beb8ff7c0d..cde9e1b918b 100644
--- a/app/views/layouts/header/_default.html.haml
+++ b/app/views/layouts/header/_default.html.haml
@@ -15,7 +15,7 @@
           - if current_user
             - if session[:impersonator_id]
               %li.impersonation
-                = link_to stop_impersonation_admin_users_path, method: :delete, title: 'Stop Impersonation', data: { toggle: 'tooltip', placement: 'bottom', container: 'body' } do
+                = link_to admin_impersonation_path, method: :delete, title: 'Stop Impersonation', data: { toggle: 'tooltip', placement: 'bottom', container: 'body' } do
                   = icon('user-secret fw')
             - if current_user.is_admin?
               %li