From d3462e711c0b3cc17ef47e1ffffa6f40f98b5e98 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Fri, 22 Apr 2016 23:19:55 +0200 Subject: Fix issue with impersonation --- app/controllers/admin/application_controller.rb | 8 +---- app/controllers/admin/impersonation_controller.rb | 38 ---------------------- app/controllers/admin/impersonations_controller.rb | 28 ++++++++++++++++ app/controllers/admin/users_controller.rb | 16 +++++++++ app/views/layouts/header/_default.html.haml | 2 +- 5 files changed, 46 insertions(+), 46 deletions(-) delete mode 100644 app/controllers/admin/impersonation_controller.rb create mode 100644 app/controllers/admin/impersonations_controller.rb (limited to 'app') 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..1ca3dd8228d --- /dev/null +++ b/app/controllers/admin/impersonations_controller.rb @@ -0,0 +1,28 @@ +class Admin::ImpersonationsController < Admin::ApplicationController + skip_before_action :authenticate_admin! + before_action :authenticate_impersonator! + + def destroy + redirect_path = admin_user_path(current_user) + + warden.set_user(impersonator, scope: :user) + + session[:impersonator_id] = nil + + redirect_to redirect_path + end + + private + + def user + @user ||= User.find(params[:id]) + end + + 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/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 -- cgit v1.2.1 From 0ab98a8a407cb9764c9c28554c8f077906a9eb9b Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Fri, 22 Apr 2016 21:55:49 +0000 Subject: Remove unused method --- app/controllers/admin/impersonations_controller.rb | 4 ---- 1 file changed, 4 deletions(-) (limited to 'app') diff --git a/app/controllers/admin/impersonations_controller.rb b/app/controllers/admin/impersonations_controller.rb index 1ca3dd8228d..2d64923c478 100644 --- a/app/controllers/admin/impersonations_controller.rb +++ b/app/controllers/admin/impersonations_controller.rb @@ -14,10 +14,6 @@ class Admin::ImpersonationsController < Admin::ApplicationController private - def user - @user ||= User.find(params[:id]) - end - def impersonator @impersonator ||= User.find(session[:impersonator_id]) if session[:impersonator_id] end -- cgit v1.2.1 From c6c985bc690c88f2819787824a50b22cc86cacf4 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Fri, 22 Apr 2016 21:58:09 +0000 Subject: Store original user in variable --- app/controllers/admin/impersonations_controller.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'app') diff --git a/app/controllers/admin/impersonations_controller.rb b/app/controllers/admin/impersonations_controller.rb index 2d64923c478..2db824c87ef 100644 --- a/app/controllers/admin/impersonations_controller.rb +++ b/app/controllers/admin/impersonations_controller.rb @@ -3,13 +3,13 @@ class Admin::ImpersonationsController < Admin::ApplicationController before_action :authenticate_impersonator! def destroy - redirect_path = admin_user_path(current_user) + original_user = current_user warden.set_user(impersonator, scope: :user) session[:impersonator_id] = nil - redirect_to redirect_path + redirect_to admin_user_path(original_user) end private -- cgit v1.2.1 From d5267dfd0dac8e4cab4919bf8aca611de3a5497b Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Sun, 24 Apr 2016 21:45:26 -0700 Subject: Prevent private snippets in public/internal projects from being leaked via API Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/15580 --- app/finders/snippets_finder.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'app') 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 -- cgit v1.2.1 From ef340f6e777875e1bbb38752e64ba7bea3ab2f31 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Thu, 21 Apr 2016 17:13:14 +0200 Subject: Ensure URL in all Service subclasses are valid MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- app/helpers/issues_helper.rb | 48 +++++++++++++++------- app/models/project_services/buildkite_service.rb | 4 +- .../project_services/issue_tracker_service.rb | 2 +- app/models/project_services/jira_service.rb | 2 + app/models/project_services/slack_service.rb | 2 +- 5 files changed, 39 insertions(+), 19 deletions(-) (limited to 'app') 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? -- cgit v1.2.1 From cd0750e0457f26f8165be301ad628e1830bd1e40 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Mon, 25 Apr 2016 15:46:15 +0200 Subject: Prevent private project name and namespace from leaking in the new MR view MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #15591. Signed-off-by: Rémy Coutable --- app/services/merge_requests/build_service.rb | 3 +++ 1 file changed, 3 insertions(+) (limited to 'app') 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 -- cgit v1.2.1 From be67a4843cc37790402404650cb96a6f02552b54 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Tue, 26 Apr 2016 12:55:38 -0400 Subject: Prevent privilege escalation via notes API Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/15577 --- app/services/notes/create_service.rb | 11 +++++++++++ 1 file changed, 11 insertions(+) (limited to 'app') 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 -- cgit v1.2.1