diff options
Diffstat (limited to 'lib')
| -rw-r--r-- | lib/api/branches.rb | 3 | ||||
| -rw-r--r-- | lib/api/helpers.rb | 4 | ||||
| -rw-r--r-- | lib/api/internal.rb | 38 | ||||
| -rw-r--r-- | lib/api/merge_requests.rb | 3 | ||||
| -rw-r--r-- | lib/gitlab/backend/grack_auth.rb | 47 | ||||
| -rw-r--r-- | lib/gitlab/backend/rack_attack_helpers.rb | 31 | ||||
| -rw-r--r-- | lib/gitlab/git_access.rb | 130 | ||||
| -rw-r--r-- | lib/gitlab/git_access_wiki.rb | 2 | ||||
| -rw-r--r-- | lib/gitlab/markdown.rb | 39 | ||||
| -rw-r--r-- | lib/gitlab/popen.rb | 2 | ||||
| -rw-r--r-- | lib/gitlab/satellite/merge_action.rb | 2 | ||||
| -rw-r--r-- | lib/gitlab/satellite/satellite.rb | 6 | ||||
| -rw-r--r-- | lib/gitlab/theme.rb | 2 | ||||
| -rw-r--r-- | lib/tasks/gitlab/shell.rake | 1 |
14 files changed, 205 insertions, 105 deletions
diff --git a/lib/api/branches.rb b/lib/api/branches.rb index b52d786e020..edfdf842f85 100644 --- a/lib/api/branches.rb +++ b/lib/api/branches.rb @@ -1,4 +1,5 @@ require 'mime/types' +require 'uri' module API # Projects API @@ -103,7 +104,7 @@ module API delete ":id/repository/branches/:branch" do authorize_push_project result = DeleteBranchService.new(user_project, current_user). - execute(params[:branch]) + execute(URI.unescape(params[:branch])) if result[:status] == :success { diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index a6e77002a01..be133a2920b 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -20,7 +20,7 @@ module API identifier = sudo_identifier() # If the sudo is the current user do nothing - if (identifier && !(@current_user.id == identifier || @current_user.username == identifier)) + if identifier && !(@current_user.id == identifier || @current_user.username == identifier) render_api_error!('403 Forbidden: Must be admin to use sudo', 403) unless @current_user.is_admin? @current_user = User.by_username_or_id(identifier) not_found!("No user id or username for: #{identifier}") if @current_user.nil? @@ -33,7 +33,7 @@ module API identifier ||= params[SUDO_PARAM] ||= env[SUDO_HEADER] # Regex for integers - if (!!(identifier =~ /^[0-9]+$/)) + if !!(identifier =~ /^[0-9]+$/) identifier.to_i else identifier diff --git a/lib/api/internal.rb b/lib/api/internal.rb index 753d0fcbd98..f98a17773e7 100644 --- a/lib/api/internal.rb +++ b/lib/api/internal.rb @@ -17,42 +17,40 @@ module API post "/allowed" do status 200 - actor = if params[:key_id] - Key.find_by(id: params[:key_id]) - elsif params[:user_id] - User.find_by(id: params[:user_id]) - end + actor = + if params[:key_id] + Key.find_by(id: params[:key_id]) + elsif params[:user_id] + User.find_by(id: params[:user_id]) + end unless actor return Gitlab::GitAccessStatus.new(false, 'No such user or key') end project_path = params[:project] - + # Check for *.wiki repositories. # Strip out the .wiki from the pathname before finding the # project. This applies the correct project permissions to # the wiki repository as well. - access = - if project_path.end_with?('.wiki') - project_path.chomp!('.wiki') - Gitlab::GitAccessWiki.new - else - Gitlab::GitAccess.new - end + wiki = project_path.end_with?('.wiki') + project_path.chomp!('.wiki') if wiki project = Project.find_with_namespace(project_path) if project - status = access.check( - actor, - params[:action], - project, - params[:changes] - ) + access = + if wiki + Gitlab::GitAccessWiki.new(actor, project) + else + Gitlab::GitAccess.new(actor, project) + end + + status = access.check(params[:action], params[:changes]) end - if project && status && status.allowed? + if project && access.can_read_project? status else Gitlab::GitAccessStatus.new(false, 'No such project') diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index 25b7857f4b1..f3765f5ab03 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -178,7 +178,8 @@ module API put ":id/merge_request/:merge_request_id/merge" do merge_request = user_project.merge_requests.find(params[:merge_request_id]) - allowed = ::Gitlab::GitAccess.can_push_to_branch?(current_user, user_project, merge_request.target_branch) + allowed = ::Gitlab::GitAccess.new(current_user, user_project). + can_push_to_branch?(merge_request.target_branch) if allowed if merge_request.unchecked? diff --git a/lib/gitlab/backend/grack_auth.rb b/lib/gitlab/backend/grack_auth.rb index ee877e099b1..050b5ba29dd 100644 --- a/lib/gitlab/backend/grack_auth.rb +++ b/lib/gitlab/backend/grack_auth.rb @@ -1,3 +1,4 @@ +require_relative 'rack_attack_helpers' require_relative 'shell_env' module Grack @@ -85,25 +86,41 @@ module Grack user = oauth_access_token_check(login, password) end - return user if user.present? - - # At this point, we know the credentials were wrong. We let Rack::Attack - # know there was a failed authentication attempt from this IP. This - # information is stored in the Rails cache (Redis) and will be used by - # the Rack::Attack middleware to decide whether to block requests from - # this IP. + # If the user authenticated successfully, we reset the auth failure count + # from Rack::Attack for that IP. A client may attempt to authenticate + # with a username and blank password first, and only after it receives + # a 401 error does it present a password. Resetting the count prevents + # false positives from occurring. + # + # Otherwise, we let Rack::Attack know there was a failed authentication + # attempt from this IP. This information is stored in the Rails cache + # (Redis) and will be used by the Rack::Attack middleware to decide + # whether to block requests from this IP. config = Gitlab.config.rack_attack.git_basic_auth - Rack::Attack::Allow2Ban.filter(@request.ip, config) do - # Unless the IP is whitelisted, return true so that Allow2Ban - # increments the counter (stored in Rails.cache) for the IP - if config.ip_whitelist.include?(@request.ip) - false + + if config.enabled + if user + # A successful login will reset the auth failure count from this IP + Rack::Attack::Allow2Ban.reset(@request.ip, config) else - true + banned = Rack::Attack::Allow2Ban.filter(@request.ip, config) do + # Unless the IP is whitelisted, return true so that Allow2Ban + # increments the counter (stored in Rails.cache) for the IP + if config.ip_whitelist.include?(@request.ip) + false + else + true + end + end + + if banned + Rails.logger.info "IP #{@request.ip} failed to login " \ + "as #{login} but has been temporarily banned from Git auth" + end end end - nil # No user was found + user end def authorized_request? @@ -112,7 +129,7 @@ module Grack case git_cmd when *Gitlab::GitAccess::DOWNLOAD_COMMANDS if user - Gitlab::GitAccess.new.download_access_check(user, project).allowed? + Gitlab::GitAccess.new(user, project).download_access_check.allowed? elsif project.public? # Allow clone/fetch for public projects true diff --git a/lib/gitlab/backend/rack_attack_helpers.rb b/lib/gitlab/backend/rack_attack_helpers.rb new file mode 100644 index 00000000000..8538f3f6eca --- /dev/null +++ b/lib/gitlab/backend/rack_attack_helpers.rb @@ -0,0 +1,31 @@ +# rack-attack v4.2.0 doesn't yet support clearing of keys. +# Taken from https://github.com/kickstarter/rack-attack/issues/113 +class Rack::Attack::Allow2Ban + def self.reset(discriminator, options) + findtime = options[:findtime] or raise ArgumentError, "Must pass findtime option" + + cache.reset_count("#{key_prefix}:count:#{discriminator}", findtime) + cache.delete("#{key_prefix}:ban:#{discriminator}") + end +end + +class Rack::Attack::Cache + def reset_count(unprefixed_key, period) + epoch_time = Time.now.to_i + # Add 1 to expires_in to avoid timing error: http://git.io/i1PHXA + expires_in = period - (epoch_time % period) + 1 + key = "#{(epoch_time / period).to_i}:#{unprefixed_key}" + delete(key) + end + + def delete(unprefixed_key) + store.delete("#{prefix}:#{unprefixed_key}") + end +end + +class Rack::Attack::StoreProxy::RedisStoreProxy + def delete(key, options={}) + self.del(key) + rescue Redis::BaseError + end +end diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index cb69e4b13d3..bc72b7528d5 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -3,11 +3,34 @@ module Gitlab DOWNLOAD_COMMANDS = %w{ git-upload-pack git-upload-archive } PUSH_COMMANDS = %w{ git-receive-pack } - attr_reader :params, :project, :git_cmd, :user + attr_reader :actor, :project - def self.can_push_to_branch?(user, project, ref) + def initialize(actor, project) + @actor = actor + @project = project + end + + def user + return @user if defined?(@user) + + @user = + case actor + when User + actor + when DeployKey + nil + when Key + actor.user + end + end + + def deploy_key + actor if actor.is_a?(DeployKey) + end + + def can_push_to_branch?(ref) return false unless user - + if project.protected_branch?(ref) && !(project.developers_can_push_to_protected_branch?(ref) && project.team.developer?(user)) user.can?(:push_code_to_protected_branches, project) @@ -16,51 +39,65 @@ module Gitlab end end - def check(actor, cmd, project, changes = nil) + def can_read_project? + if user + user.can?(:read_project, project) + elsif deploy_key + deploy_key.projects.include?(project) + else + false + end + end + + def check(cmd, changes = nil) case cmd when *DOWNLOAD_COMMANDS - download_access_check(actor, project) + download_access_check when *PUSH_COMMANDS - if actor.is_a? User - push_access_check(actor, project, changes) - elsif actor.is_a? DeployKey - return build_status_object(false, "Deploy key not allowed to push") - elsif actor.is_a? Key - push_access_check(actor.user, project, changes) - else - raise 'Wrong actor' - end + push_access_check(changes) else - return build_status_object(false, "Wrong command") + build_status_object(false, "Wrong command") end end - def download_access_check(actor, project) - if actor.is_a?(User) - user_download_access_check(actor, project) - elsif actor.is_a?(DeployKey) - if actor.projects.include?(project) - build_status_object(true) - else - build_status_object(false, "Deploy key not allowed to access this project") - end - elsif actor.is_a? Key - user_download_access_check(actor.user, project) + def download_access_check + if user + user_download_access_check + elsif deploy_key + deploy_key_download_access_check + else + raise 'Wrong actor' + end + end + + def push_access_check(changes) + if user + user_push_access_check(changes) + elsif deploy_key + build_status_object(false, "Deploy key not allowed to push") else raise 'Wrong actor' end end - def user_download_access_check(user, project) - if user && user_allowed?(user) && user.can?(:download_code, project) + def user_download_access_check + if user && user_allowed? && user.can?(:download_code, project) build_status_object(true) else build_status_object(false, "You don't have access") end end - def push_access_check(user, project, changes) - unless user && user_allowed?(user) + def deploy_key_download_access_check + if can_read_project? + build_status_object(true) + else + build_status_object(false, "Deploy key not allowed to access this project") + end + end + + def user_push_access_check(changes) + unless user && user_allowed? return build_status_object(false, "You don't have access") end @@ -76,27 +113,28 @@ module Gitlab # Iterate over all changes to find if user allowed all of them to be applied changes.map(&:strip).reject(&:blank?).each do |change| - status = change_access_check(user, project, change) + status = change_access_check(change) unless status.allowed? # If user does not have access to make at least one change - cancel all push return status end end - return build_status_object(true) + build_status_object(true) end - def change_access_check(user, project, change) + def change_access_check(change) oldrev, newrev, ref = change.split(' ') - action = if project.protected_branch?(branch_name(ref)) - protected_branch_action(project, oldrev, newrev, branch_name(ref)) - elsif protected_tag?(project, tag_name(ref)) - # Prevent any changes to existing git tag unless user has permissions - :admin_project - else - :push_code - end + action = + if project.protected_branch?(branch_name(ref)) + protected_branch_action(oldrev, newrev, branch_name(ref)) + elsif protected_tag?(tag_name(ref)) + # Prevent any changes to existing git tag unless user has permissions + :admin_project + else + :push_code + end if user.can?(action, project) build_status_object(true) @@ -105,15 +143,15 @@ module Gitlab end end - def forced_push?(project, oldrev, newrev) + def forced_push?(oldrev, newrev) Gitlab::ForcePushCheck.force_push?(project, oldrev, newrev) end private - def protected_branch_action(project, oldrev, newrev, branch_name) + def protected_branch_action(oldrev, newrev, branch_name) # we dont allow force push to protected branch - if forced_push?(project, oldrev, newrev) + if forced_push?(oldrev, newrev) :force_push_code_to_protected_branches elsif Gitlab::Git.blank_ref?(newrev) # and we dont allow remove of protected branch @@ -125,11 +163,11 @@ module Gitlab end end - def protected_tag?(project, tag_name) + def protected_tag?(tag_name) project.repository.tag_names.include?(tag_name) end - def user_allowed?(user) + def user_allowed? Gitlab::UserAccess.allowed?(user) end diff --git a/lib/gitlab/git_access_wiki.rb b/lib/gitlab/git_access_wiki.rb index a2177c8d548..73d99b96202 100644 --- a/lib/gitlab/git_access_wiki.rb +++ b/lib/gitlab/git_access_wiki.rb @@ -1,6 +1,6 @@ module Gitlab class GitAccessWiki < GitAccess - def change_access_check(user, project, change) + def change_access_check(change) if user.can?(:write_wiki, project) build_status_object(true) else diff --git a/lib/gitlab/markdown.rb b/lib/gitlab/markdown.rb index e02e5b9fc3d..41bb8d08924 100644 --- a/lib/gitlab/markdown.rb +++ b/lib/gitlab/markdown.rb @@ -79,15 +79,35 @@ module Gitlab # Used markdown pipelines in GitLab: # GitlabEmojiFilter - performs emoji replacement. + # SanitizationFilter - remove unsafe HTML tags and attributes # # see https://gitlab.com/gitlab-org/html-pipeline-gitlab for more filters filters = [ - HTML::Pipeline::Gitlab::GitlabEmojiFilter + HTML::Pipeline::Gitlab::GitlabEmojiFilter, + HTML::Pipeline::SanitizationFilter ] + whitelist = HTML::Pipeline::SanitizationFilter::WHITELIST + whitelist[:attributes][:all].push('class', 'id') + whitelist[:elements].push('span') + + # Remove the rel attribute that the sanitize gem adds, and remove the + # href attribute if it contains inline javascript + fix_anchors = lambda do |env| + name, node = env[:node_name], env[:node] + if name == 'a' + node.remove_attribute('rel') + if node['href'] && node['href'].match('javascript:') + node.remove_attribute('href') + end + end + end + whitelist[:transformers].push(fix_anchors) + markdown_context = { asset_root: Gitlab.config.gitlab.url, - asset_host: Gitlab::Application.config.asset_host + asset_host: Gitlab::Application.config.asset_host, + whitelist: whitelist } markdown_pipeline = HTML::Pipeline::Gitlab.new(filters).pipeline @@ -97,18 +117,14 @@ module Gitlab if options[:xhtml] saveoptions |= Nokogiri::XML::Node::SaveOptions::AS_XHTML end - text = result[:output].to_html(save_with: saveoptions) - allowed_attributes = ActionView::Base.sanitized_allowed_attributes - allowed_tags = ActionView::Base.sanitized_allowed_tags + text = result[:output].to_html(save_with: saveoptions) - text = sanitize text.html_safe, - attributes: allowed_attributes + %w(id class style), - tags: allowed_tags + %w(table tr td th) if options[:parse_tasks] text = parse_tasks(text) end - text + + text.html_safe end private @@ -352,11 +368,12 @@ module Gitlab # ActiveSupport::SafeBuffer, hence the `String.new` String.new(text).gsub(Taskable::TASK_PATTERN_HTML) do checked = $LAST_MATCH_INFO[:checked].downcase == 'x' + p_tag = $LAST_MATCH_INFO[:p_tag] if checked - "#{li_tag}#{checked_box}" + "#{li_tag}#{p_tag}#{checked_box}" else - "#{li_tag}#{unchecked_box}" + "#{li_tag}#{p_tag}#{unchecked_box}" end end end diff --git a/lib/gitlab/popen.rb b/lib/gitlab/popen.rb index fea4d2d55d2..43e07e09160 100644 --- a/lib/gitlab/popen.rb +++ b/lib/gitlab/popen.rb @@ -29,7 +29,7 @@ module Gitlab @cmd_status = wait_thr.value.exitstatus end - return @cmd_output, @cmd_status + [@cmd_output, @cmd_status] end end end diff --git a/lib/gitlab/satellite/merge_action.rb b/lib/gitlab/satellite/merge_action.rb index 25122666f5e..1f2e5f82dd5 100644 --- a/lib/gitlab/satellite/merge_action.rb +++ b/lib/gitlab/satellite/merge_action.rb @@ -97,7 +97,7 @@ module Gitlab in_locked_and_timed_satellite do |merge_repo| prepare_satellite!(merge_repo) update_satellite_source_and_target!(merge_repo) - if (merge_request.for_fork?) + if merge_request.for_fork? repository = Gitlab::Git::Repository.new(merge_repo.path) commits = Gitlab::Git::Commit.between( repository, diff --git a/lib/gitlab/satellite/satellite.rb b/lib/gitlab/satellite/satellite.rb index 70125d539da..f24c6199c44 100644 --- a/lib/gitlab/satellite/satellite.rb +++ b/lib/gitlab/satellite/satellite.rb @@ -99,11 +99,7 @@ module Gitlab heads = repo.heads.map(&:name) # update or create the parking branch - if heads.include? PARKING_BRANCH - repo.git.checkout({}, PARKING_BRANCH) - else - repo.git.checkout(default_options({ b: true }), PARKING_BRANCH) - end + repo.git.checkout(default_options({ B: true }), PARKING_BRANCH) # remove the parking branch from the list of heads ... heads.delete(PARKING_BRANCH) diff --git a/lib/gitlab/theme.rb b/lib/gitlab/theme.rb index 9799e54de5d..43093c7d27e 100644 --- a/lib/gitlab/theme.rb +++ b/lib/gitlab/theme.rb @@ -19,7 +19,7 @@ module Gitlab id ||= Gitlab.config.gitlab.default_theme - return themes[id] + themes[id] end def self.type_css_class_by_id(id) diff --git a/lib/tasks/gitlab/shell.rake b/lib/tasks/gitlab/shell.rake index 9af93300e08..e835d6cb9b7 100644 --- a/lib/tasks/gitlab/shell.rake +++ b/lib/tasks/gitlab/shell.rake @@ -112,6 +112,7 @@ namespace :gitlab do print '.' end end + puts "" unless $?.success? puts "Failed to add keys...".red |