diff options
60 files changed, 474 insertions, 233 deletions
diff --git a/.rubocop.yml b/.rubocop.yml index 53ca2ca2191..7290d627d24 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -355,7 +355,7 @@ Style/MultilineBlockChain: Style/MultilineBlockLayout: Description: 'Ensures newlines after multiline block do statements.' - Enabled: false + Enabled: true Style/MultilineIfThen: Description: 'Do not use then for multi-line if/unless.' @@ -390,7 +390,7 @@ Style/NegatedWhile: Style/NestedTernaryOperator: Description: 'Use one expression per branch in a ternary operator.' StyleGuide: 'https://github.com/bbatsov/ruby-style-guide#no-nested-ternary' - Enabled: false + Enabled: true Style/Next: Description: 'Use `next` to skip iteration instead of a condition at the end.' @@ -400,17 +400,17 @@ Style/Next: Style/NilComparison: Description: 'Prefer x.nil? to x == nil.' StyleGuide: 'https://github.com/bbatsov/ruby-style-guide#predicate-methods' - Enabled: false + Enabled: true Style/NonNilCheck: Description: 'Checks for redundant nil checks.' StyleGuide: 'https://github.com/bbatsov/ruby-style-guide#no-non-nil-checks' - Enabled: false + Enabled: true Style/Not: Description: 'Use ! instead of not.' StyleGuide: 'https://github.com/bbatsov/ruby-style-guide#bang-not-not' - Enabled: false + Enabled: true Style/NumericLiterals: Description: >- @@ -424,7 +424,7 @@ Style/OneLineConditional: Favor the ternary operator(?:) over if/then/else/end constructs. StyleGuide: 'https://github.com/bbatsov/ruby-style-guide#ternary-operator' - Enabled: false + Enabled: true Style/OpMethod: Description: 'When defining binary operators, name the argument other.' @@ -436,7 +436,7 @@ Style/ParenthesesAroundCondition: Don't use parentheses around the condition of an if/unless/while. StyleGuide: 'https://github.com/bbatsov/ruby-style-guide#no-parens-if' - Enabled: false + Enabled: true Style/PercentLiteralDelimiters: Description: 'Use `%`-literal delimiters consistently' @@ -480,7 +480,7 @@ Style/RedundantException: Style/RedundantReturn: Description: "Don't use return where it's not required." StyleGuide: 'https://github.com/bbatsov/ruby-style-guide#no-explicit-return' - Enabled: false + Enabled: true Style/RedundantSelf: Description: "Don't use self where it's not needed." diff --git a/CHANGELOG b/CHANGELOG index b87f116af5d..f9bd0940a71 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,6 +1,7 @@ Please view this file on the master branch, on stable branches it's out of date. v 7.10.0 (unreleased) + - Allow HTML tags in Markdown input - Fix code unfold not working on Compare commits page (Stan Hu) - Include missing events and fix save functionality in admin service template settings form (Stan Hu) - Fix "Import projects from" button to show the correct instructions (Stan Hu) @@ -9,6 +10,7 @@ v 7.10.0 (unreleased) - Update poltergeist to version 1.6.0 to support PhantomJS 2.0 (Zeger-Jan van de Weg) - Fix cross references when usernames, milestones, or project names contain underscores (Stan Hu) - Disable reference creation for comments surrounded by code/preformatted blocks (Stan Hu) + - Reduce Rack Attack false positives causing 403 errors during HTTP authentication (Stan Hu) - enable line wrapping per default and remove the checkbox to toggle it (Hannes Rosenögger) - extend the commit calendar to show the actual commits made on a date (Hannes Rosenögger) - Fix a link in the patch update guide @@ -18,6 +20,7 @@ v 7.10.0 (unreleased) - Add changelog, license and contribution guide links to project sidebar. - Improve diff UI - Fix alignment of navbar toggle button (Cody Mize) + - Fix checkbox rendering for nested task lists - Identical look of selectboxes in UI - Move "Import existing repository by URL" option to button. - Improve error message when save profile has error. @@ -34,11 +37,13 @@ v 7.10.0 (unreleased) - Don't show commit comment button when user is not signed in. - Don't include system notes in issue/MR comment count. - Don't mark merge request as updated when merge status relative to target branch changes. - -v 7.9.0 + - Link note avatar to user. + - Make Git-over-SSH errors more descriptive. - Send EmailsOnPush email when branch or tag is created or deleted. + - Faster merge request processing for large repository + - Prevent doubling AJAX request with each commit visit via Turbolink -v 7.9.0 (unreleased) +v 7.9.0 - Add HipChat integration documentation (Stan Hu) - Update documentation for object_kind field in Webhook push and tag push Webhooks (Stan Hu) - Fix broken email images (Hannes Rosenögger) @@ -155,7 +160,6 @@ v 7.8.0 - Add API endpoint to fetch all changes on a MergeRequest (Jeroen van Baarsen) - View note image attachments in new tab when clicked instead of downloading them - Improve sorting logic in UI and API. Explicitly define what sorting method is used by default - - Allow more variations for commit messages closing issues (Julien Bianchi and Hannes Rosenögger) - Fix overflow at sidebar when have several items - Add notes for label changes in issue and merge requests - Show tags in commit view (Hannes Rosenögger) @@ -177,7 +181,7 @@ v 7.8.0 - Add a commit calendar to the user profile (Hannes Rosenögger) - Submit comment on command-enter - Notify all members of a group when that group is mentioned in a comment, for example: `@gitlab-org` or `@sales`. - - Extend issue clossing pattern to include "Resolve", "Resolves", "Resolved", "Resolving" and "Close" + - Extend issue clossing pattern to include "Resolve", "Resolves", "Resolved", "Resolving" and "Close" (Julien Bianchi and Hannes Rosenögger) - Fix long broadcast message cut-off on left sidebar (Visay Keo) - Add Project Avatars (Steven Thonus and Hannes Rosenögger) - Password reset token validity increased from 2 hours to 2 days since it is also send on account creation. diff --git a/Gemfile.lock b/Gemfile.lock index 513e2c643e6..7da4d3c3583 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -516,7 +516,7 @@ GEM rubyntlm (0.5.0) rubypants (0.2.0) rugged (0.21.4) - rugments (1.0.0.beta5) + rugments (1.0.0.beta6) safe_yaml (0.9.7) sanitize (2.1.0) nokogiri (>= 1.4.4) diff --git a/app/assets/javascripts/diff.js.coffee b/app/assets/javascripts/diff.js.coffee index 05f5af42571..069f91c30e1 100644 --- a/app/assets/javascripts/diff.js.coffee +++ b/app/assets/javascripts/diff.js.coffee @@ -37,8 +37,6 @@ class @Diff ) ) - $('.diff-header').stick_in_parent(recalc_every: 1, offset_top: $('.navbar').height()) - lineNumbers: (line) -> return ([0, 0]) unless line.children().length lines = line.children().slice(0, 2) diff --git a/app/assets/javascripts/merge_request.js.coffee b/app/assets/javascripts/merge_request.js.coffee index 09c202e42a5..6127d2bb480 100644 --- a/app/assets/javascripts/merge_request.js.coffee +++ b/app/assets/javascripts/merge_request.js.coffee @@ -113,8 +113,14 @@ class @MergeRequest allowed_states = ["failed", "canceled", "running", "pending", "success"] if state in allowed_states $('.ci_widget.ci-' + state).show() + switch state + when "failed", "canceled" + @setMergeButtonClass('btn-danger') + when "running", "pending" + @setMergeButtonClass('btn-warning') else $('.ci_widget.ci-error').show() + @setMergeButtonClass('btn-danger') showCiCoverage: (coverage) -> cov_html = $('<span>') @@ -144,6 +150,9 @@ class @MergeRequest this.$('.merge-in-progress').hide() this.$('.automerge_widget.already_cannot_be_merged').show() + setMergeButtonClass: (css_class) -> + $('.accept_merge_request').removeClass("btn-create").addClass(css_class) + mergeInProgress: -> $.ajax type: 'GET' diff --git a/app/assets/javascripts/project_users_select.js.coffee b/app/assets/javascripts/project_users_select.js.coffee index e22c7c11f1c..80ab1a61ab9 100644 --- a/app/assets/javascripts/project_users_select.js.coffee +++ b/app/assets/javascripts/project_users_select.js.coffee @@ -25,7 +25,7 @@ class @ProjectUsersSelect initSelection: (element, callback) -> id = $(element).val() - if id isnt "" + if id != "" && id != "-1" Api.user(id, callback) @@ -44,10 +44,7 @@ class @ProjectUsersSelect else avatar = gon.default_avatar_url - if user.id == '' - avatarMarkup = '' - else - avatarMarkup = "<div class='user-image'><img class='avatar s24' src='#{avatar}'></div>" + avatarMarkup = "<div class='user-image'><img class='avatar s24' src='#{avatar}'></div>" "<div class='user-result'> #{avatarMarkup} diff --git a/app/assets/stylesheets/pages/merge_requests.scss b/app/assets/stylesheets/pages/merge_requests.scss index d8fe339b7b3..8abd4207beb 100644 --- a/app/assets/stylesheets/pages/merge_requests.scss +++ b/app/assets/stylesheets/pages/merge_requests.scss @@ -137,30 +137,15 @@ background-color: #F1FAF1; } - &.ci-pending { - color: #548; - border-color: #548; - background-color: #F4F1FA; - } - + &.ci-pending, &.ci-running { color: $gl-warning; border-color: $gl-warning; background-color: #FAF5F1; } - &.ci-failed { - color: $gl-danger; - border-color: $gl-danger; - background-color: #FAF1F1; - } - - &.ci-canceled { - color: $gl-warning; - border-color: $gl-danger; - background-color: #FAF5F1; - } - + &.ci-failed, + &.ci-canceled, &.ci-error { color: $gl-danger; border-color: $gl-danger; diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index e9b7d7e0083..47ce8467358 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -257,7 +257,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController end def allowed_to_push_code?(project, branch) - ::Gitlab::GitAccess.can_push_to_branch?(current_user, project, branch) + ::Gitlab::GitAccess.new(current_user, project).can_push_to_branch?(branch) end def merge_request_params diff --git a/app/helpers/branches_helper.rb b/app/helpers/branches_helper.rb index 4a5edf6d101..d6eaa7d57bc 100644 --- a/app/helpers/branches_helper.rb +++ b/app/helpers/branches_helper.rb @@ -12,6 +12,6 @@ module BranchesHelper def can_push_branch?(project, branch_name) return false unless project.repository.branch_names.include?(branch_name) - ::Gitlab::GitAccess.can_push_to_branch?(current_user, project, branch_name) + ::Gitlab::GitAccess.new(current_user, project).can_push_to_branch?(branch_name) end end diff --git a/app/helpers/gitlab_markdown_helper.rb b/app/helpers/gitlab_markdown_helper.rb index f8e104b0827..7ca3f058636 100644 --- a/app/helpers/gitlab_markdown_helper.rb +++ b/app/helpers/gitlab_markdown_helper.rb @@ -29,13 +29,12 @@ module GitlabMarkdownHelper end def markdown(text, options={}) - unless (@markdown and options == @options) + unless @markdown && options == @options @options = options gitlab_renderer = Redcarpet::Render::GitlabHTML.new(self, user_color_scheme_class, { # see https://github.com/vmg/redcarpet#darling-i-packed-you-a-couple-renderers-for-lunch- - filter_html: true, with_toc_data: true, safe_links_only: true }.merge(options)) @@ -182,7 +181,7 @@ module GitlabMarkdownHelper def file_exists?(path) return false if path.nil? - return @repository.blob_at(current_sha, path).present? || @repository.tree(current_sha, path).entries.any? + @repository.blob_at(current_sha, path).present? || @repository.tree(current_sha, path).entries.any? end # Check if the path is pointing to a directory(tree) or a file(blob) @@ -190,7 +189,7 @@ module GitlabMarkdownHelper def local_path(path) return "tree" if @repository.tree(current_sha, path).entries.any? return "raw" if @repository.blob_at(current_sha, path).image? - return "blob" + "blob" end def current_sha diff --git a/app/helpers/issues_helper.rb b/app/helpers/issues_helper.rb index 15c5dcb6a25..a4bd4d30215 100644 --- a/app/helpers/issues_helper.rb +++ b/app/helpers/issues_helper.rb @@ -58,22 +58,11 @@ module IssuesHelper end def bulk_update_milestone_options - options_for_select(['None (backlog)']) + + options_for_select([['None (backlog)', -1]]) + options_from_collection_for_select(project_active_milestones, 'id', 'title', params[:milestone_id]) end - def bulk_update_assignee_options(project = @project) - options_for_select(['None (unassigned)']) + - options_from_collection_for_select(project.team.members, 'id', - 'name', params[:assignee_id]) - end - - def assignee_options(object, project = @project) - options_from_collection_for_select(project.team.members.sort_by(&:name), - 'id', 'name', object.assignee_id) - end - def milestone_options(object) options_from_collection_for_select(object.project.milestones.active, 'id', 'title', object.milestone_id) diff --git a/app/helpers/merge_requests_helper.rb b/app/helpers/merge_requests_helper.rb index 51b60770e0b..54462fd00e3 100644 --- a/app/helpers/merge_requests_helper.rb +++ b/app/helpers/merge_requests_helper.rb @@ -17,7 +17,7 @@ module MergeRequestsHelper end def new_mr_from_push_event(event, target_project) - return { + { merge_request: { source_project_id: event.project.id, target_project_id: target_project.id, diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb index 77ad7a40fab..7bf51b5b8e8 100644 --- a/app/helpers/projects_helper.rb +++ b/app/helpers/projects_helper.rb @@ -146,7 +146,7 @@ module ProjectsHelper nav_tabs << feature if project.send :"#{feature}_enabled" end - if project.issues_enabled + if project.issues_enabled || project.merge_requests_enabled nav_tabs << [:milestones, :labels] end diff --git a/app/helpers/submodule_helper.rb b/app/helpers/submodule_helper.rb index 525266fb3b5..241462e5e4c 100644 --- a/app/helpers/submodule_helper.rb +++ b/app/helpers/submodule_helper.rb @@ -49,7 +49,7 @@ module SubmoduleHelper def standard_links(host, namespace, project, commit) base = [ 'https://', host, '/', namespace, '/', project ].join('') - return base, [ base, '/tree/', commit ].join('') + [base, [ base, '/tree/', commit ].join('')] end def relative_self_links(url, commit) @@ -58,7 +58,10 @@ module SubmoduleHelper else base = [ @project.group.path, '/', url[/([^\/]*)\.git/, 1] ].join('') end - return namespace_project_path(base.namespace, base), + + [ + namespace_project_path(base.namespace, base), namespace_project_tree_path(base.namespace, base, commit) + ] end end diff --git a/app/helpers/tree_helper.rb b/app/helpers/tree_helper.rb index b6fb7a8aa5a..bf6726574ec 100644 --- a/app/helpers/tree_helper.rb +++ b/app/helpers/tree_helper.rb @@ -56,7 +56,7 @@ module TreeHelper ref ||= @ref return false unless project.repository.branch_names.include?(ref) - ::Gitlab::GitAccess.can_push_to_branch?(current_user, project, ref) + ::Gitlab::GitAccess.new(current_user, project).can_push_to_branch?(ref) end def tree_breadcrumbs(tree, max_links = 2) diff --git a/app/mailers/notify.rb b/app/mailers/notify.rb index ee27879cf40..8fcdd3bc853 100644 --- a/app/mailers/notify.rb +++ b/app/mailers/notify.rb @@ -148,7 +148,7 @@ class Notify < ActionMailer::Base headers['References'] = message_id(model) headers['X-GitLab-Project'] = "#{@project.name} | " if @project - if (headers[:subject]) + if headers[:subject] headers[:subject].prepend('Re: ') end diff --git a/app/models/concerns/taskable.rb b/app/models/concerns/taskable.rb index 410e8dc820b..bbb3b301a9f 100644 --- a/app/models/concerns/taskable.rb +++ b/app/models/concerns/taskable.rb @@ -5,7 +5,7 @@ # Used by MergeRequest and Issue module Taskable TASK_PATTERN_MD = /^(?<bullet> *[*-] *)\[(?<checked>[ xX])\]/.freeze - TASK_PATTERN_HTML = /^<li>\[(?<checked>[ xX])\]/.freeze + TASK_PATTERN_HTML = /^<li>(?<p_tag>\s*<p>)?\[(?<checked>[ xX])\]/.freeze # Change the state of a task list item for this Taskable. Edit the object's # description by finding the nth task item and changing its checkbox diff --git a/app/models/project_services/asana_service.rb b/app/models/project_services/asana_service.rb index d52214cdd69..e6e16058d41 100644 --- a/app/models/project_services/asana_service.rb +++ b/app/models/project_services/asana_service.rb @@ -82,7 +82,7 @@ automatically inspected. Leave blank to include all branches.' branch_restriction = restrict_to_branch.to_s # check the branch restriction is poplulated and branch is not included - if branch_restriction.length > 0 && branch_restriction.index(branch) == nil + if branch_restriction.length > 0 && branch_restriction.index(branch).nil? return end diff --git a/app/models/user.rb b/app/models/user.rb index 50f664a09a3..979150b4d68 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -110,6 +110,7 @@ class User < ActiveRecord::Base has_many :notes, dependent: :destroy, foreign_key: :author_id has_many :merge_requests, dependent: :destroy, foreign_key: :author_id has_many :events, dependent: :destroy, foreign_key: :author_id, class_name: "Event" + has_many :subscriptions, dependent: :destroy has_many :recent_events, -> { order "id DESC" }, foreign_key: :author_id, class_name: "Event" has_many :assigned_issues, dependent: :destroy, foreign_key: :assignee_id, class_name: "Issue" has_many :assigned_merge_requests, dependent: :destroy, foreign_key: :assignee_id, class_name: "MergeRequest" diff --git a/app/services/files/create_service.rb b/app/services/files/create_service.rb index de5322e990a..eeafefc25af 100644 --- a/app/services/files/create_service.rb +++ b/app/services/files/create_service.rb @@ -3,7 +3,7 @@ require_relative "base_service" module Files class CreateService < BaseService def execute - allowed = Gitlab::GitAccess.can_push_to_branch?(current_user, project, ref) + allowed = Gitlab::GitAccess.new(current_user, project).can_push_to_branch?(ref) unless allowed return error("You are not allowed to create file in this branch") diff --git a/app/services/files/delete_service.rb b/app/services/files/delete_service.rb index 8e73c2e2727..1497a0f883b 100644 --- a/app/services/files/delete_service.rb +++ b/app/services/files/delete_service.rb @@ -3,7 +3,7 @@ require_relative "base_service" module Files class DeleteService < BaseService def execute - allowed = ::Gitlab::GitAccess.can_push_to_branch?(current_user, project, ref) + allowed = ::Gitlab::GitAccess.new(current_user, project).can_push_to_branch?(ref) unless allowed return error("You are not allowed to push into this branch") diff --git a/app/services/files/update_service.rb b/app/services/files/update_service.rb index 328cf3a4b06..0724d3ae634 100644 --- a/app/services/files/update_service.rb +++ b/app/services/files/update_service.rb @@ -3,7 +3,7 @@ require_relative "base_service" module Files class UpdateService < BaseService def execute - allowed = ::Gitlab::GitAccess.can_push_to_branch?(current_user, project, ref) + allowed = ::Gitlab::GitAccess.new(current_user, project).can_push_to_branch?(ref) unless allowed return error("You are not allowed to push into this branch") diff --git a/app/services/issues/bulk_update_service.rb b/app/services/issues/bulk_update_service.rb index c7cd20b6b60..eb07413ee94 100644 --- a/app/services/issues/bulk_update_service.rb +++ b/app/services/issues/bulk_update_service.rb @@ -4,9 +4,9 @@ module Issues issues_ids = params.delete(:issues_ids).split(",") issue_params = params - issue_params.delete(:state_event) unless issue_params[:state_event].present? - issue_params.delete(:milestone_id) unless issue_params[:milestone_id].present? - issue_params.delete(:assignee_id) unless issue_params[:assignee_id].present? + issue_params.delete(:state_event) unless issue_params[:state_event].present? + issue_params.delete(:milestone_id) unless issue_params[:milestone_id].present? + issue_params.delete(:assignee_id) unless issue_params[:assignee_id].present? issues = Issue.where(id: issues_ids) issues.each do |issue| diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb index c61d67a7893..3371fe7d5ef 100644 --- a/app/services/issues/update_service.rb +++ b/app/services/issues/update_service.rb @@ -14,6 +14,9 @@ module Issues issue.update_nth_task(params[:task_num].to_i, false) end + params[:assignee_id] = "" if params[:assignee_id] == "-1" + params[:milestone_id] = "" if params[:milestone_id] == "-1" + old_labels = issue.labels.to_a if params.present? && issue.update_attributes(params.except(:state_event, diff --git a/app/services/merge_requests/update_service.rb b/app/services/merge_requests/update_service.rb index 870b50bb60d..0ac6dfea6fd 100644 --- a/app/services/merge_requests/update_service.rb +++ b/app/services/merge_requests/update_service.rb @@ -23,6 +23,9 @@ module MergeRequests merge_request.update_nth_task(params[:task_num].to_i, false) end + params[:assignee_id] = "" if params[:assignee_id] == "-1" + params[:milestone_id] = "" if params[:milestone_id] == "-1" + old_labels = merge_request.labels.to_a if params.present? && merge_request.update_attributes( diff --git a/app/services/projects/transfer_service.rb b/app/services/projects/transfer_service.rb index 3372cfc11d0..489e03bd5ef 100644 --- a/app/services/projects/transfer_service.rb +++ b/app/services/projects/transfer_service.rb @@ -43,6 +43,9 @@ module Projects project.namespace = new_namespace project.save! + # Notifications + project.send_move_instructions + # Move main repository unless gitlab_shell.mv_repository(old_path, new_path) raise TransferError.new('Cannot move project') diff --git a/app/views/admin/broadcast_messages/index.html.haml b/app/views/admin/broadcast_messages/index.html.haml index c0afaf16d8f..7e29311bf42 100644 --- a/app/views/admin/broadcast_messages/index.html.haml +++ b/app/views/admin/broadcast_messages/index.html.haml @@ -21,13 +21,11 @@ .form-group.js-toggle-colors-container.hide = f.label :color, "Background Color", class: 'control-label' .col-sm-10 - = f.text_field :color, placeholder: "#AA33EE", class: "form-control" - .light 6 character hex values starting with a # sign. + = f.color_field :color, value: "#AA33EE", class: "form-control" .form-group.js-toggle-colors-container.hide = f.label :font, "Font Color", class: 'control-label' .col-sm-10 - = f.text_field :font, placeholder: "#224466", class: "form-control" - .light 6 character hex values starting with a # sign. + = f.color_field :font, value: "#224466", class: "form-control" .form-group = f.label :starts_at, class: 'control-label' .col-sm-10.datetime-controls diff --git a/app/views/projects/commit/_commit_box.html.haml b/app/views/projects/commit/_commit_box.html.haml index 7409f702c5d..2579f2cac92 100644 --- a/app/views/projects/commit/_commit_box.html.haml +++ b/app/views/projects/commit/_commit_box.html.haml @@ -48,5 +48,4 @@ = preserve(gfm(escape_once(@commit.description))) :coffeescript - $ -> - $(".commit-info-row.branches").load("#{branches_namespace_project_commit_path(@project.namespace, @project, @commit.id)}") + $(".commit-info-row.branches").load("#{branches_namespace_project_commit_path(@project.namespace, @project, @commit.id)}") diff --git a/app/views/projects/diffs/_diffs.html.haml b/app/views/projects/diffs/_diffs.html.haml index 1747f36dcf3..2b9b6599a7d 100644 --- a/app/views/projects/diffs/_diffs.html.haml +++ b/app/views/projects/diffs/_diffs.html.haml @@ -1,10 +1,9 @@ -.row.prepend-top-20.append-bottom-10 - .col-md-8 - = render 'projects/diffs/stats', diffs: diffs - .col-md-4 - .btn-group.pull-right +.prepend-top-20.append-bottom-20 + .pull-right + .btn-group = inline_diff_btn = parallel_diff_btn + = render 'projects/diffs/stats', diffs: diffs - if show_diff_size_warning?(diffs) = render 'projects/diffs/warning', diffs: diffs @@ -19,3 +18,6 @@ Failed to collect changes %p Maybe diff is really big and operation failed with timeout. Try to get diff locally + +:coffeescript + $('.files .diff-header').stick_in_parent(recalc_every: 1, offset_top: $('.navbar').height()) diff --git a/app/views/projects/labels/_form.html.haml b/app/views/projects/labels/_form.html.haml index 2305fce112e..ad993db6c0b 100644 --- a/app/views/projects/labels/_form.html.haml +++ b/app/views/projects/labels/_form.html.haml @@ -16,9 +16,9 @@ .col-sm-10 .input-group .input-group-addon.label-color-preview - = f.color_field :color, placeholder: "#AA33EE", class: "form-control" + = f.color_field :color, value: "#AA33EE", class: "form-control" .help-block - 6 character hex values starting with a # sign. + Choose any color. %br Or you can choose one of suggested colors below diff --git a/app/views/projects/milestones/show.html.haml b/app/views/projects/milestones/show.html.haml index 110d8967342..25cc0030965 100644 --- a/app/views/projects/milestones/show.html.haml +++ b/app/views/projects/milestones/show.html.haml @@ -60,11 +60,12 @@ Participants %span.badge= @users.count - .pull-right - = link_to new_namespace_project_issue_path(@project.namespace, @project, issue: { milestone_id: @milestone.id }), class: "btn btn-grouped", title: "New Issue" do - %i.fa.fa-plus - New Issue - = link_to 'Browse Issues', namespace_project_issues_path(@milestone.project.namespace, @milestone.project, milestone_id: @milestone.id), class: "btn edit-milestone-link btn-grouped" + - if @project.issues_enabled + .pull-right + = link_to new_namespace_project_issue_path(@project.namespace, @project, issue: { milestone_id: @milestone.id }), class: "btn btn-grouped", title: "New Issue" do + %i.fa.fa-plus + New Issue + = link_to 'Browse Issues', namespace_project_issues_path(@milestone.project.namespace, @milestone.project, milestone_id: @milestone.id), class: "btn edit-milestone-link btn-grouped" .tab-content .tab-pane.active#tab-issues diff --git a/app/views/projects/notes/_discussion.html.haml b/app/views/projects/notes/_discussion.html.haml index f4c6fad2fed..3561ca49f81 100644 --- a/app/views/projects/notes/_discussion.html.haml +++ b/app/views/projects/notes/_discussion.html.haml @@ -2,7 +2,8 @@ .timeline-entry .timeline-entry-inner .timeline-icon - = image_tag avatar_icon(note.author_email), class: "avatar s40" + = link_to user_path(note.author) do + = image_tag avatar_icon(note.author_email), class: "avatar s40" .timeline-content - if note.for_merge_request? - if note.outdated? diff --git a/app/views/projects/notes/_note.html.haml b/app/views/projects/notes/_note.html.haml index f3d00a6f06d..71bdf5c8f2a 100644 --- a/app/views/projects/notes/_note.html.haml +++ b/app/views/projects/notes/_note.html.haml @@ -4,7 +4,8 @@ - if note.system %span.fa.fa-circle - else - = image_tag avatar_icon(note.author_email), class: "avatar s40" + = link_to user_path(note.author) do + = image_tag avatar_icon(note.author_email), class: "avatar s40" .timeline-content .note-header .note-actions @@ -21,7 +22,8 @@ %i.fa.fa-trash-o.cred Remove - if note.system - = image_tag avatar_icon(note.author_email), class: "avatar s16" + = link_to user_path(note.author) do + = image_tag avatar_icon(note.author_email), class: "avatar s16" = link_to_member(@project, note.author, avatar: false) %span.author-username = '@' + note.author.username diff --git a/app/views/projects/notes/discussions/_diff.html.haml b/app/views/projects/notes/discussions/_diff.html.haml index f717c77a898..711aa39101b 100644 --- a/app/views/projects/notes/discussions/_diff.html.haml +++ b/app/views/projects/notes/discussions/_diff.html.haml @@ -2,13 +2,13 @@ - if diff .diff-file .diff-header - - if diff.deleted_file - %span= diff.old_path - - else - %span= diff.new_path - - if diff.a_mode && diff.b_mode && diff.a_mode != diff.b_mode - %span.file-mode= "#{diff.a_mode} → #{diff.b_mode}" - %br/ + %span + - if diff.deleted_file + = diff.old_path + - else + = diff.new_path + - if diff.a_mode && diff.b_mode && diff.a_mode != diff.b_mode + %span.file-mode= "#{diff.a_mode} → #{diff.b_mode}" .diff-content %table - note.truncated_diff_lines.each do |line| diff --git a/config/gitlab.yml.example b/config/gitlab.yml.example index a85db10e019..c4a0fefb7ab 100644 --- a/config/gitlab.yml.example +++ b/config/gitlab.yml.example @@ -285,6 +285,9 @@ production: &base rack_attack: git_basic_auth: + # Rack Attack IP banning enabled + # enabled: true + # # Whitelist requests from 127.0.0.1 for web proxies (NGINX/Apache) with incorrect headers # ip_whitelist: ["127.0.0.1"] # diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index 70af7a829c4..15c1ae9466f 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -183,6 +183,7 @@ Settings['extra'] ||= Settingslogic.new({}) # Settings['rack_attack'] ||= Settingslogic.new({}) Settings.rack_attack['git_basic_auth'] ||= Settingslogic.new({}) +Settings.rack_attack.git_basic_auth['enabled'] = true if Settings.rack_attack.git_basic_auth['enabled'].nil? Settings.rack_attack.git_basic_auth['ip_whitelist'] ||= %w{127.0.0.1} Settings.rack_attack.git_basic_auth['maxretry'] ||= 10 Settings.rack_attack.git_basic_auth['findtime'] ||= 1.minute diff --git a/db/migrate/20150324155957_set_incorrect_assignee_id_to_null.rb b/db/migrate/20150324155957_set_incorrect_assignee_id_to_null.rb new file mode 100644 index 00000000000..42dc8173e46 --- /dev/null +++ b/db/migrate/20150324155957_set_incorrect_assignee_id_to_null.rb @@ -0,0 +1,6 @@ +class SetIncorrectAssigneeIdToNull < ActiveRecord::Migration + def up + execute "UPDATE issues SET assignee_id = NULL WHERE assignee_id = -1" + execute "UPDATE merge_requests SET assignee_id = NULL WHERE assignee_id = -1" + end +end diff --git a/db/schema.rb b/db/schema.rb index e1a5b70532a..4a445ae5832 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20150320234437) do +ActiveRecord::Schema.define(version: 20150324155957) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" diff --git a/doc/markdown/markdown.md b/doc/markdown/markdown.md index b66583bb363..965d8fc313f 100644 --- a/doc/markdown/markdown.md +++ b/doc/markdown/markdown.md @@ -46,14 +46,15 @@ You can also use other rich text files in GitLab. You might have to install a de GFM honors the markdown specification in how [paragraphs and line breaks are handled](http://daringfireball.net/projects/markdown/syntax#p). -A paragraph is simply one or more consecutive lines of text, separated by one or more blank lines.: +A paragraph is simply one or more consecutive lines of text, separated by one or more blank lines. +Line-breaks, or softreturns, are rendered if you end a line with two or more spaces - Roses are red + Roses are red [followed by two or more spaces] Violets are blue Sugar is sweet -Roses are red +Roses are red Violets are blue Sugar is sweet @@ -440,6 +441,8 @@ Note that inline HTML is disabled in the default Gitlab configuration, although <dd>Does *not* work **very** well. Use HTML <em>tags</em>.</dd> </dl> +See the documentation for HTML::Pipeline's [SanitizationFilter](http://www.rubydoc.info/gems/html-pipeline/HTML/Pipeline/SanitizationFilter#WHITELIST-constant) class for the list of allowed HTML tags and attributes. In addition to the default `SanitizationFilter` whitelist, GitLab allows the `class`, `id`, and `style` attributes. + ## Horizontal Rule ``` diff --git a/doc/permissions/permissions.md b/doc/permissions/permissions.md index c9928e11b2e..8cfa7f9c876 100644 --- a/doc/permissions/permissions.md +++ b/doc/permissions/permissions.md @@ -41,6 +41,11 @@ If a user is a GitLab administrator they receive all permissions. ## Group +In order for a group to appear as public and be browsable, it must contain at +least one public project. + +Any user can remove themselves from a group, unless they are the last Owner of the group. + | Action | Guest | Reporter | Developer | Master | Owner | |-------------------------|-------|----------|-----------|--------|-------| | Browse group | ✓ | ✓ | ✓ | ✓ | ✓ | @@ -48,5 +53,3 @@ If a user is a GitLab administrator they receive all permissions. | Create project in group | | | | ✓ | ✓ | | Manage group members | | | | | ✓ | | Remove group | | | | | ✓ | - -Any user can remove themselves from a group, unless they are the last Owner of the group. 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/api/users.rb b/lib/api/users.rb index 7c8b3250cd0..032a5d76e43 100644 --- a/lib/api/users.rb +++ b/lib/api/users.rb @@ -61,10 +61,10 @@ module API authenticated_as_admin! required_attributes! [:email, :password, :name, :username] attrs = attributes_for_keys [:email, :name, :password, :skype, :linkedin, :twitter, :projects_limit, :username, :bio, :can_create_group, :admin, :confirm] - user = User.build_user(attrs) admin = attrs.delete(:admin) - user.admin = admin unless admin.nil? confirm = !(attrs.delete(:confirm) =~ (/(false|f|no|0)$/i)) + user = User.build_user(attrs) + user.admin = admin unless admin.nil? user.skip_confirmation! unless confirm identity_attrs = attributes_for_keys [:provider, :extern_uid] 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 diff --git a/spec/helpers/gitlab_markdown_helper_spec.rb b/spec/helpers/gitlab_markdown_helper_spec.rb index 6ba27b536e4..c631acc591d 100644 --- a/spec/helpers/gitlab_markdown_helper_spec.rb +++ b/spec/helpers/gitlab_markdown_helper_spec.rb @@ -727,6 +727,36 @@ describe GitlabMarkdownHelper do expected = "" expect(markdown(actual)).to match(expected) end + + it 'should allow whitelisted HTML tags from the user' do + actual = '<dl><dt>Term</dt><dd>Definition</dd></dl>' + expect(markdown(actual)).to match(actual) + end + + it 'should sanitize tags that are not whitelisted' do + actual = '<textarea>no inputs allowed</textarea> <blink>no blinks</blink>' + expected = 'no inputs allowed no blinks' + expect(markdown(actual)).to match(expected) + expect(markdown(actual)).not_to match('<.textarea>') + expect(markdown(actual)).not_to match('<.blink>') + end + + it 'should allow whitelisted tag attributes from the user' do + actual = '<a class="custom">link text</a>' + expect(markdown(actual)).to match(actual) + end + + it 'should sanitize tag attributes that are not whitelisted' do + actual = '<a href="http://example.com/bar.html" foo="bar">link text</a>' + expected = '<a href="http://example.com/bar.html">link text</a>' + expect(markdown(actual)).to match(expected) + end + + it 'should sanitize javascript in attributes' do + actual = %q(<a href="javascript:alert('foo')">link text</a>) + expected = '<a>link text</a>' + expect(markdown(actual)).to match(expected) + end end describe 'markdown for empty repository' do @@ -817,6 +847,17 @@ EOT ) end + it 'should render checkboxes for nested tasks' do + rendered_text = markdown(@source_text_asterisk, parse_tasks: true) + + expect(rendered_text).to match( + /<input.*checkbox.*valid unchecked nested task/ + ) + expect(rendered_text).to match( + /<input.*checkbox.*valid checked nested task/ + ) + end + it 'should not be confused by whitespace before bullets' do rendered_text_asterisk = markdown(@source_text_asterisk, parse_tasks: true) diff --git a/spec/lib/gitlab/backend/grack_auth_spec.rb b/spec/lib/gitlab/backend/grack_auth_spec.rb index 768312f0028..d0aad54f677 100644 --- a/spec/lib/gitlab/backend/grack_auth_spec.rb +++ b/spec/lib/gitlab/backend/grack_auth_spec.rb @@ -6,7 +6,7 @@ describe Grack::Auth do let(:app) { lambda { |env| [200, {}, "Success!"] } } let!(:auth) { Grack::Auth.new(app) } - let(:env) { + let(:env) { { "rack.input" => "", "REQUEST_METHOD" => "GET", @@ -85,6 +85,17 @@ describe Grack::Auth do it "responds with status 401" do expect(status).to eq(401) end + + context "when the user is IP banned" do + before do + expect(Rack::Attack::Allow2Ban).to receive(:filter).and_return(true) + allow_any_instance_of(Rack::Request).to receive(:ip).and_return('1.2.3.4') + end + + it "responds with status 401" do + expect(status).to eq(401) + end + end end context "when authentication succeeds" do @@ -109,10 +120,49 @@ describe Grack::Auth do end context "when the user isn't blocked" do + before do + expect(Rack::Attack::Allow2Ban).to receive(:reset) + end + it "responds with status 200" do expect(status).to eq(200) end end + + context "when blank password attempts follow a valid login" do + let(:options) { Gitlab.config.rack_attack.git_basic_auth } + let(:maxretry) { options[:maxretry] - 1 } + let(:ip) { '1.2.3.4' } + + before do + allow_any_instance_of(Rack::Request).to receive(:ip).and_return(ip) + Rack::Attack::Allow2Ban.reset(ip, options) + end + + after do + Rack::Attack::Allow2Ban.reset(ip, options) + end + + def attempt_login(include_password) + password = include_password ? user.password : "" + env["HTTP_AUTHORIZATION"] = ActionController::HttpAuthentication::Basic.encode_credentials(user.username, password) + Grack::Auth.new(app) + auth.call(env).first + end + + it "repeated attempts followed by successful attempt" do + for n in 0..maxretry do + expect(attempt_login(false)).to eq(401) + end + + expect(attempt_login(true)).to eq(200) + expect(Rack::Attack::Allow2Ban.send(:banned?, ip)).to eq(nil) + + for n in 0..maxretry do + expect(attempt_login(false)).to eq(401) + end + end + end end context "when the user doesn't have access to the project" do diff --git a/spec/lib/gitlab/backend/rack_attack_helpers_spec.rb b/spec/lib/gitlab/backend/rack_attack_helpers_spec.rb new file mode 100644 index 00000000000..2ac496fd669 --- /dev/null +++ b/spec/lib/gitlab/backend/rack_attack_helpers_spec.rb @@ -0,0 +1,35 @@ +require "spec_helper" + +describe 'RackAttackHelpers' do + describe 'reset' do + let(:discriminator) { 'test-key'} + let(:maxretry) { 5 } + let(:period) { 1.minute } + let(:options) { { findtime: period, bantime: 60, maxretry: maxretry } } + + def do_filter + for i in 1..maxretry - 1 do + status = Rack::Attack::Allow2Ban.filter(discriminator, options) { true } + expect(status).to eq(false) + end + end + + def do_reset + Rack::Attack::Allow2Ban.reset(discriminator, options) + end + + before do + do_reset + end + + after do + do_reset + end + + it 'user is not banned after n - 1 retries' do + do_filter + do_reset + do_filter + end + end +end diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb index 666398eedd4..39be9d64644 100644 --- a/spec/lib/gitlab/git_access_spec.rb +++ b/spec/lib/gitlab/git_access_spec.rb @@ -1,25 +1,26 @@ require 'spec_helper' describe Gitlab::GitAccess do - let(:access) { Gitlab::GitAccess.new } + let(:access) { Gitlab::GitAccess.new(actor, project) } let(:project) { create(:project) } let(:user) { create(:user) } + let(:actor) { user } describe 'can_push_to_branch?' do describe 'push to none protected branch' do it "returns true if user is a master" do project.team << [user, :master] - expect(Gitlab::GitAccess.can_push_to_branch?(user, project, "random_branch")).to be_truthy + expect(access.can_push_to_branch?("random_branch")).to be_truthy end it "returns true if user is a developer" do project.team << [user, :developer] - expect(Gitlab::GitAccess.can_push_to_branch?(user, project, "random_branch")).to be_truthy + expect(access.can_push_to_branch?("random_branch")).to be_truthy end it "returns false if user is a reporter" do project.team << [user, :reporter] - expect(Gitlab::GitAccess.can_push_to_branch?(user, project, "random_branch")).to be_falsey + expect(access.can_push_to_branch?("random_branch")).to be_falsey end end @@ -30,17 +31,17 @@ describe Gitlab::GitAccess do it "returns true if user is a master" do project.team << [user, :master] - expect(Gitlab::GitAccess.can_push_to_branch?(user, project, @branch.name)).to be_truthy + expect(access.can_push_to_branch?(@branch.name)).to be_truthy end it "returns false if user is a developer" do project.team << [user, :developer] - expect(Gitlab::GitAccess.can_push_to_branch?(user, project, @branch.name)).to be_falsey + expect(access.can_push_to_branch?(@branch.name)).to be_falsey end it "returns false if user is a reporter" do project.team << [user, :reporter] - expect(Gitlab::GitAccess.can_push_to_branch?(user, project, @branch.name)).to be_falsey + expect(access.can_push_to_branch?(@branch.name)).to be_falsey end end @@ -51,17 +52,17 @@ describe Gitlab::GitAccess do it "returns true if user is a master" do project.team << [user, :master] - expect(Gitlab::GitAccess.can_push_to_branch?(user, project, @branch.name)).to be_truthy + expect(access.can_push_to_branch?(@branch.name)).to be_truthy end it "returns true if user is a developer" do project.team << [user, :developer] - expect(Gitlab::GitAccess.can_push_to_branch?(user, project, @branch.name)).to be_truthy + expect(access.can_push_to_branch?(@branch.name)).to be_truthy end it "returns false if user is a reporter" do project.team << [user, :reporter] - expect(Gitlab::GitAccess.can_push_to_branch?(user, project, @branch.name)).to be_falsey + expect(access.can_push_to_branch?(@branch.name)).to be_falsey end end @@ -72,7 +73,7 @@ describe Gitlab::GitAccess do before { project.team << [user, :master] } context 'pull code' do - subject { access.download_access_check(user, project) } + subject { access.download_access_check } it { expect(subject.allowed?).to be_truthy } end @@ -82,7 +83,7 @@ describe Gitlab::GitAccess do before { project.team << [user, :guest] } context 'pull code' do - subject { access.download_access_check(user, project) } + subject { access.download_access_check } it { expect(subject.allowed?).to be_falsey } end @@ -95,7 +96,7 @@ describe Gitlab::GitAccess do end context 'pull code' do - subject { access.download_access_check(user, project) } + subject { access.download_access_check } it { expect(subject.allowed?).to be_falsey } end @@ -103,7 +104,7 @@ describe Gitlab::GitAccess do describe 'without acccess to project' do context 'pull code' do - subject { access.download_access_check(user, project) } + subject { access.download_access_check } it { expect(subject.allowed?).to be_falsey } end @@ -111,17 +112,18 @@ describe Gitlab::GitAccess do describe 'deploy key permissions' do let(:key) { create(:deploy_key) } + let(:actor) { key } context 'pull code' do context 'allowed' do before { key.projects << project } - subject { access.download_access_check(key, project) } + subject { access.download_access_check } it { expect(subject.allowed?).to be_truthy } end context 'denied' do - subject { access.download_access_check(key, project) } + subject { access.download_access_check } it { expect(subject.allowed?).to be_falsey } end @@ -205,7 +207,7 @@ describe Gitlab::GitAccess do permissions_matrix[role].each do |action, allowed| context action do - subject { access.push_access_check(user, project, changes[action]) } + subject { access.push_access_check(changes[action]) } it { expect(subject.allowed?).to allowed ? be_truthy : be_falsey } end @@ -221,7 +223,7 @@ describe Gitlab::GitAccess do updated_permissions_matrix[role].each do |action, allowed| context action do - subject { access.push_access_check(user, project, changes[action]) } + subject { access.push_access_check(changes[action]) } it { expect(subject.allowed?).to allowed ? be_truthy : be_falsey } end diff --git a/spec/lib/gitlab/git_access_wiki_spec.rb b/spec/lib/gitlab/git_access_wiki_spec.rb index c31c6764091..4cb91094cb3 100644 --- a/spec/lib/gitlab/git_access_wiki_spec.rb +++ b/spec/lib/gitlab/git_access_wiki_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' describe Gitlab::GitAccessWiki do - let(:access) { Gitlab::GitAccessWiki.new } + let(:access) { Gitlab::GitAccessWiki.new(user, project) } let(:project) { create(:project) } let(:user) { create(:user) } @@ -11,7 +11,7 @@ describe Gitlab::GitAccessWiki do project.team << [user, :developer] end - subject { access.push_access_check(user, project, changes) } + subject { access.push_access_check(changes) } it { expect(subject.allowed?).to be_truthy } end |