diff options
48 files changed, 485 insertions, 169 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index b3daec7e6ed..ca52a3a7444 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,23 @@ documentation](doc/development/changelog.md) for instructions on adding your own entry. +## 11.9.1 (2019-03-25) + +### Fixed (7 changes) + +- Fix issue that caused the "Show all activity" button to appear on top of the mini pipeline status dropdown on the merge request page. !26274 +- Fix duplicated bottom match line on merge request parallel diff view. !26402 +- Allow users who can push to protected branches to create protected branches via CLI. !26413 +- Add missing .gitlab-ci.yml to Android template. !26415 +- Refresh commit count after repository head changes. !26473 +- Set proper default-branch for repository on GitHub Import. !26476 +- GitHub importer: Use the project creator to create branches from forks. !26510 + +### Changed (1 change) + +- Upgrade to Gitaly v1.27.1. !26533 + + ## 11.9.0 (2019-03-22) ### Security (24 changes) diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION index 5db08bf2dc5..08002f86cc8 100644 --- a/GITALY_SERVER_VERSION +++ b/GITALY_SERVER_VERSION @@ -1 +1 @@ -1.27.0 +1.27.1 @@ -1 +1 @@ -11.9.0 +11.9.1 diff --git a/app/assets/javascripts/issue.js b/app/assets/javascripts/issue.js index 94b78907d9a..b3508f36cf9 100644 --- a/app/assets/javascripts/issue.js +++ b/app/assets/javascripts/issue.js @@ -16,7 +16,9 @@ export default class Issue { Issue.createMrDropdownWrap = document.querySelector('.create-mr-dropdown-wrap'); Issue.initMergeRequests(); - Issue.initRelatedBranches(); + if (document.querySelector('#related-branches')) { + Issue.initRelatedBranches(); + } this.closeButtons = $('a.btn-close'); this.reopenButtons = $('a.btn-reopen'); diff --git a/app/assets/stylesheets/pages/merge_requests.scss b/app/assets/stylesheets/pages/merge_requests.scss index 126b00af552..ef6552fb265 100644 --- a/app/assets/stylesheets/pages/merge_requests.scss +++ b/app/assets/stylesheets/pages/merge_requests.scss @@ -806,7 +806,7 @@ .merge-request-tabs-holder { top: $header-height; - z-index: 300; + z-index: 250; background-color: $white-light; border-bottom: 1px solid $border-color; diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index b9d02a62fc3..2cb40697b5c 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -39,6 +39,7 @@ class Projects::IssuesController < Projects::ApplicationController before_action :authorize_create_merge_request_from!, only: [:create_merge_request] before_action :authorize_import_issues!, only: [:import_csv] + before_action :authorize_download_code!, only: [:related_branches] before_action :set_suggested_issues_feature_flags, only: [:new] diff --git a/app/models/broadcast_message.rb b/app/models/broadcast_message.rb index 2d237383e60..1c95abdd9ee 100644 --- a/app/models/broadcast_message.rb +++ b/app/models/broadcast_message.rb @@ -4,7 +4,7 @@ class BroadcastMessage < ActiveRecord::Base include CacheMarkdownField include Sortable - cache_markdown_field :message, pipeline: :broadcast_message + cache_markdown_field :message, pipeline: :broadcast_message, whitelisted: true validates :message, presence: true validates :starts_at, presence: true diff --git a/app/models/concerns/cache_markdown_field.rb b/app/models/concerns/cache_markdown_field.rb index 1a8570b80c3..15d8d58b9b5 100644 --- a/app/models/concerns/cache_markdown_field.rb +++ b/app/models/concerns/cache_markdown_field.rb @@ -7,6 +7,7 @@ # cache_markdown_field :foo # cache_markdown_field :bar # cache_markdown_field :baz, pipeline: :single_line +# cache_markdown_field :baz, whitelisted: true # # Corresponding foo_html, bar_html and baz_html fields should exist. module CacheMarkdownField @@ -37,7 +38,15 @@ module CacheMarkdownField end def html_fields - markdown_fields.map {|field| html_field(field) } + markdown_fields.map { |field| html_field(field) } + end + + def html_fields_whitelisted + markdown_fields.each_with_object([]) do |field, fields| + if @data[field].fetch(:whitelisted, false) + fields << html_field(field) + end + end end end @@ -149,13 +158,18 @@ module CacheMarkdownField alias_method :attributes_before_markdown_cache, :attributes def attributes attrs = attributes_before_markdown_cache + html_fields = cached_markdown_fields.html_fields + whitelisted = cached_markdown_fields.html_fields_whitelisted + exclude_fields = html_fields - whitelisted - attrs.delete('cached_markdown_version') - - cached_markdown_fields.html_fields.each do |field| + exclude_fields.each do |field| attrs.delete(field) end + if whitelisted.empty? + attrs.delete('cached_markdown_version') + end + attrs end diff --git a/app/models/project.rb b/app/models/project.rb index 7d6f7fd2c58..4039af7a330 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1378,6 +1378,7 @@ class Project < ActiveRecord::Base repository.raw_repository.write_ref('HEAD', "refs/heads/#{branch}") repository.copy_gitattributes(branch) repository.after_change_head + ProjectCacheWorker.perform_async(self.id, [], [:commit_count]) reload_default_branch else errors.add(:base, "Could not change HEAD: branch '#{branch}' does not exist") diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb index cf257ed47c8..6f2070243e6 100644 --- a/app/policies/project_policy.rb +++ b/app/policies/project_policy.rb @@ -177,7 +177,6 @@ class ProjectPolicy < BasePolicy enable :read_cycle_analytics enable :award_emoji enable :read_pages_content - enable :read_release end # These abilities are not allowed to admins that are not members of the project, @@ -203,6 +202,7 @@ class ProjectPolicy < BasePolicy enable :read_deployment enable :read_merge_request enable :read_sentry_issue + enable :read_release end # We define `:public_user_access` separately because there are cases in gitlab-ee diff --git a/app/serializers/diff_file_entity.rb b/app/serializers/diff_file_entity.rb index 13711070a46..066e30cd3bb 100644 --- a/app/serializers/diff_file_entity.rb +++ b/app/serializers/diff_file_entity.rb @@ -57,7 +57,7 @@ class DiffFileEntity < DiffFileBaseEntity diff_file.diff_lines_for_serializer end - expose :is_fully_expanded, if: -> (diff_file, _) { Feature.enabled?(:expand_diff_full_file) && diff_file.text? } do |diff_file| + expose :is_fully_expanded, if: -> (diff_file, _) { Feature.enabled?(:expand_diff_full_file, default_enabled: true) && diff_file.text? } do |diff_file| diff_file.fully_expanded? end diff --git a/app/views/projects/issues/show.html.haml b/app/views/projects/issues/show.html.haml index 3a674da6e87..819d3c4ec76 100644 --- a/app/views/projects/issues/show.html.haml +++ b/app/views/projects/issues/show.html.haml @@ -80,8 +80,9 @@ #merge-requests{ data: { url: referenced_merge_requests_project_issue_path(@project, @issue) } } // This element is filled in using JavaScript. - #related-branches{ data: { url: related_branches_project_issue_path(@project, @issue) } } - // This element is filled in using JavaScript. + - if can?(current_user, :download_code, @project) + #related-branches{ data: { url: related_branches_project_issue_path(@project, @issue) } } + // This element is filled in using JavaScript. .content-block.emoji-block.emoji-block-sticky .row diff --git a/app/views/projects/protected_branches/shared/_index.html.haml b/app/views/projects/protected_branches/shared/_index.html.haml index 4997770321e..539b184e5c2 100644 --- a/app/views/projects/protected_branches/shared/_index.html.haml +++ b/app/views/projects/protected_branches/shared/_index.html.haml @@ -12,7 +12,7 @@ %p By default, protected branches are designed to: %ul - %li prevent their creation, if not already created, from everybody except users who are allowed to merge + %li prevent their creation, if not already created, from everybody except Maintainers %li prevent pushes from everybody except Maintainers %li prevent <strong>anyone</strong> from force pushing to the branch %li prevent <strong>anyone</strong> from deleting the branch diff --git a/app/workers/project_cache_worker.rb b/app/workers/project_cache_worker.rb index d27b5e62574..b31099bc670 100644 --- a/app/workers/project_cache_worker.rb +++ b/app/workers/project_cache_worker.rb @@ -27,6 +27,7 @@ class ProjectCacheWorker # rubocop: enable CodeReuse/ActiveRecord def update_statistics(project, statistics = []) + return if Gitlab::Database.read_only? return unless try_obtain_lease_for(project.id, :update_statistics) Rails.logger.info("Updating statistics for project #{project.id}") diff --git a/changelogs/unreleased/disallow-guests-to-access-releases.yml b/changelogs/unreleased/disallow-guests-to-access-releases.yml new file mode 100644 index 00000000000..f2d518108d2 --- /dev/null +++ b/changelogs/unreleased/disallow-guests-to-access-releases.yml @@ -0,0 +1,5 @@ +--- +title: Disallow guest users from accessing Releases +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-56224.yml b/changelogs/unreleased/security-56224.yml new file mode 100644 index 00000000000..a4e274e6ca5 --- /dev/null +++ b/changelogs/unreleased/security-56224.yml @@ -0,0 +1,5 @@ +--- +title: Hide "related branches" when user does not have permission +merge_request: +author: +type: security diff --git a/changelogs/unreleased/use-untrusted-regexp.yml b/changelogs/unreleased/use-untrusted-regexp.yml new file mode 100644 index 00000000000..dd7f1bcaca1 --- /dev/null +++ b/changelogs/unreleased/use-untrusted-regexp.yml @@ -0,0 +1,5 @@ +--- +title: Use UntrustedRegexp for matching refs policy +merge_request: +author: +type: security diff --git a/doc/ci/yaml/README.md b/doc/ci/yaml/README.md index 985895acce3..816d12a8dd4 100644 --- a/doc/ci/yaml/README.md +++ b/doc/ci/yaml/README.md @@ -340,6 +340,19 @@ job: - branches ``` +Pattern matching is case-sensitive by default. Use `i` flag modifier, like +`/pattern/i` to make a pattern case-insensitive: + +```yaml +job: + # use regexp + only: + - /^issue-.*$/i + # use special keyword + except: + - branches +``` + In this example, `job` will run only for refs that are tagged, or if a build is explicitly requested via an API trigger or a [Pipeline Schedule][schedules]: diff --git a/doc/user/project/protected_branches.md b/doc/user/project/protected_branches.md index 480cc921d76..2060b5dd4a2 100644 --- a/doc/user/project/protected_branches.md +++ b/doc/user/project/protected_branches.md @@ -10,7 +10,7 @@ created protected branches. By default, a protected branch does four simple things: - it prevents its creation, if not already created, from everybody except users - who are allowed to merge + with Maintainer permission - it prevents pushes from everybody except users with Maintainer permission - it prevents **anyone** from force pushing to the branch - it prevents **anyone** from deleting the branch diff --git a/lib/gitlab/checks/branch_check.rb b/lib/gitlab/checks/branch_check.rb index ad926739752..1dbd564fb6f 100644 --- a/lib/gitlab/checks/branch_check.rb +++ b/lib/gitlab/checks/branch_check.rb @@ -59,6 +59,8 @@ module Gitlab def protected_branch_creation_checks logger.log_timed(LOG_MESSAGES[:protected_branch_creation_checks]) do + break if user_access.can_push_to_branch?(branch_name) + unless user_access.can_merge_to_branch?(branch_name) raise GitAccess::UnauthorizedError, ERROR_MESSAGES[:create_protected_branch] end diff --git a/lib/gitlab/ci/build/policy/refs.rb b/lib/gitlab/ci/build/policy/refs.rb index df5f5ffc253..360424bec11 100644 --- a/lib/gitlab/ci/build/policy/refs.rb +++ b/lib/gitlab/ci/build/policy/refs.rb @@ -35,8 +35,8 @@ module Gitlab # patterns can be matched only when branch or tag is used # the pattern matching does not work for merge requests pipelines if pipeline.branch? || pipeline.tag? - if pattern.first == "/" && pattern.last == "/" - Regexp.new(pattern[1...-1]) =~ pipeline.ref + if regexp = Gitlab::UntrustedRegexp::RubySyntax.fabricate(pattern) + regexp.match?(pipeline.ref) else pattern == pipeline.ref end diff --git a/lib/gitlab/ci/pipeline/expression/lexeme/pattern.rb b/lib/gitlab/ci/pipeline/expression/lexeme/pattern.rb index d7e6dacf068..2b719c9c6fc 100644 --- a/lib/gitlab/ci/pipeline/expression/lexeme/pattern.rb +++ b/lib/gitlab/ci/pipeline/expression/lexeme/pattern.rb @@ -13,13 +13,13 @@ module Gitlab def initialize(regexp) @value = regexp - unless Gitlab::UntrustedRegexp.valid?(@value) + unless Gitlab::UntrustedRegexp::RubySyntax.valid?(@value) raise Lexer::SyntaxError, 'Invalid regular expression!' end end def evaluate(variables = {}) - Gitlab::UntrustedRegexp.fabricate(@value) + Gitlab::UntrustedRegexp::RubySyntax.fabricate!(@value) rescue RegexpError raise Expression::RuntimeError, 'Invalid regular expression!' end diff --git a/lib/gitlab/config/entry/legacy_validation_helpers.rb b/lib/gitlab/config/entry/legacy_validation_helpers.rb index d3ab5625743..0a629075302 100644 --- a/lib/gitlab/config/entry/legacy_validation_helpers.rb +++ b/lib/gitlab/config/entry/legacy_validation_helpers.rb @@ -45,17 +45,15 @@ module Gitlab end def validate_regexp(value) - !value.nil? && Regexp.new(value.to_s) && true - rescue RegexpError, TypeError - false + Gitlab::UntrustedRegexp::RubySyntax.valid?(value) end def validate_string_or_regexp(value) return true if value.is_a?(Symbol) return false unless value.is_a?(String) - if value.first == '/' && value.last == '/' - validate_regexp(value[1...-1]) + if Gitlab::UntrustedRegexp::RubySyntax.matches_syntax?(value) + validate_regexp(value) else true end diff --git a/lib/gitlab/config/entry/validators.rb b/lib/gitlab/config/entry/validators.rb index 25bfa50f829..d348e11b753 100644 --- a/lib/gitlab/config/entry/validators.rb +++ b/lib/gitlab/config/entry/validators.rb @@ -120,17 +120,13 @@ module Gitlab private - def look_like_regexp?(value) - value.is_a?(String) && value.start_with?('/') && - value.end_with?('/') + def matches_syntax?(value) + Gitlab::UntrustedRegexp::RubySyntax.matches_syntax?(value) end def validate_regexp(value) - look_like_regexp?(value) && - Regexp.new(value.to_s[1...-1]) && - true - rescue RegexpError - false + matches_syntax?(value) && + Gitlab::UntrustedRegexp::RubySyntax.valid?(value) end end @@ -149,7 +145,7 @@ module Gitlab def validate_string_or_regexp(value) return false unless value.is_a?(String) - return validate_regexp(value) if look_like_regexp?(value) + return validate_regexp(value) if matches_syntax?(value) true end diff --git a/lib/gitlab/diff/file.rb b/lib/gitlab/diff/file.rb index dbee47a19ee..dce80bf21de 100644 --- a/lib/gitlab/diff/file.rb +++ b/lib/gitlab/diff/file.rb @@ -158,7 +158,10 @@ module Gitlab new_blob || old_blob end - attr_writer :highlighted_diff_lines + def highlighted_diff_lines=(value) + clear_memoization(:diff_lines_for_serializer) + @highlighted_diff_lines = value + end # Array of Gitlab::Diff::Line objects def diff_lines @@ -314,19 +317,21 @@ module Gitlab # This adds the bottom match line to the array if needed. It contains # the data to load more context lines. def diff_lines_for_serializer - lines = highlighted_diff_lines + strong_memoize(:diff_lines_for_serializer) do + lines = highlighted_diff_lines - return if lines.empty? - return if blob.nil? + next if lines.empty? + next if blob.nil? - last_line = lines.last + last_line = lines.last - if last_line.new_pos < total_blob_lines(blob) && !deleted_file? - match_line = Gitlab::Diff::Line.new("", 'match', nil, last_line.old_pos, last_line.new_pos) - lines.push(match_line) - end + if last_line.new_pos < total_blob_lines(blob) && !deleted_file? + match_line = Gitlab::Diff::Line.new("", 'match', nil, last_line.old_pos, last_line.new_pos) + lines.push(match_line) + end - lines + lines + end end def fully_expanded? diff --git a/lib/gitlab/github_import/importer/pull_request_importer.rb b/lib/gitlab/github_import/importer/pull_request_importer.rb index 72451e5e01e..1b293ddc7c7 100644 --- a/lib/gitlab/github_import/importer/pull_request_importer.rb +++ b/lib/gitlab/github_import/importer/pull_request_importer.rb @@ -89,7 +89,7 @@ module Gitlab return if project.repository.branch_exists?(source_branch) - project.repository.add_branch(project.owner, source_branch, pull_request.source_branch_sha) + project.repository.add_branch(project.creator, source_branch, pull_request.source_branch_sha) rescue Gitlab::Git::CommandError => e Gitlab::Sentry.track_acceptable_exception(e, extra: { diff --git a/lib/gitlab/github_import/importer/repository_importer.rb b/lib/gitlab/github_import/importer/repository_importer.rb index e2dfb00dcc5..6d48c6a15b4 100644 --- a/lib/gitlab/github_import/importer/repository_importer.rb +++ b/lib/gitlab/github_import/importer/repository_importer.rb @@ -5,6 +5,7 @@ module Gitlab module Importer class RepositoryImporter include Gitlab::ShellAdapter + include Gitlab::Utils::StrongMemoize attr_reader :project, :client, :wiki_formatter @@ -17,7 +18,7 @@ module Gitlab # Returns true if we should import the wiki for the project. # rubocop: disable CodeReuse/ActiveRecord def import_wiki? - client.repository(project.import_source)&.has_wiki && + client_repository&.has_wiki && !project.wiki_repository_exists? && Gitlab::GitalyClient::RemoteService.exists?(wiki_url) end @@ -52,6 +53,7 @@ module Gitlab refmap = Gitlab::GithubImport.refmap project.repository.fetch_as_mirror(project.import_url, refmap: refmap, forced: true, remote_name: 'github') + project.change_head(default_branch) if default_branch true rescue Gitlab::Git::Repository::NoRepository, Gitlab::Shell::Error => e fail_import("Failed to import the repository: #{e.message}") @@ -82,6 +84,18 @@ module Gitlab project.import_state.mark_as_failed(message) false end + + private + + def default_branch + client_repository&.default_branch + end + + def client_repository + strong_memoize(:client_repository) do + client.repository(project.import_source) + end + end end end end diff --git a/lib/gitlab/untrusted_regexp.rb b/lib/gitlab/untrusted_regexp.rb index ba1137313d8..14126b6ec06 100644 --- a/lib/gitlab/untrusted_regexp.rb +++ b/lib/gitlab/untrusted_regexp.rb @@ -35,6 +35,10 @@ module Gitlab matches end + def match?(text) + text.present? && scan(text).present? + end + def replace(text, rewrite) RE2.Replace(text, regexp, rewrite) end @@ -43,37 +47,6 @@ module Gitlab self.source == other.source end - # Handles regular expressions with the preferred RE2 library where possible - # via UntustedRegex. Falls back to Ruby's built-in regular expression library - # when the syntax would be invalid in RE2. - # - # One difference between these is `(?m)` multi-line mode. Ruby regex enables - # this by default, but also handles `^` and `$` differently. - # See: https://www.regular-expressions.info/modifiers.html - def self.with_fallback(pattern, multiline: false) - UntrustedRegexp.new(pattern, multiline: multiline) - rescue RegexpError - Regexp.new(pattern) - end - - def self.valid?(pattern) - !!self.fabricate(pattern) - rescue RegexpError - false - end - - def self.fabricate(pattern) - matches = pattern.match(%r{^/(?<regexp>.+)/(?<flags>[ismU]*)$}) - - raise RegexpError, 'Invalid regular expression!' if matches.nil? - - expression = matches[:regexp] - flags = matches[:flags] - expression.prepend("(?#{flags})") if flags.present? - - self.new(expression, multiline: false) - end - private attr_reader :regexp diff --git a/lib/gitlab/untrusted_regexp/ruby_syntax.rb b/lib/gitlab/untrusted_regexp/ruby_syntax.rb new file mode 100644 index 00000000000..91f300f97d0 --- /dev/null +++ b/lib/gitlab/untrusted_regexp/ruby_syntax.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +module Gitlab + class UntrustedRegexp + # This class implements support for Ruby syntax of regexps + # and converts that to RE2 representation: + # /<regexp>/<flags> + class RubySyntax + PATTERN = %r{^/(?<regexp>.+)/(?<flags>[ismU]*)$}.freeze + + # Checks if pattern matches a regexp pattern + # but does not enforce it's validity + def self.matches_syntax?(pattern) + pattern.is_a?(String) && pattern.match(PATTERN).present? + end + + # The regexp can match the pattern `/.../`, but may not be fabricatable: + # it can be invalid or incomplete: `/match ( string/` + def self.valid?(pattern) + !!self.fabricate(pattern) + end + + def self.fabricate(pattern) + self.fabricate!(pattern) + rescue RegexpError + nil + end + + def self.fabricate!(pattern) + raise RegexpError, 'Pattern is not string!' unless pattern.is_a?(String) + + matches = pattern.match(PATTERN) + raise RegexpError, 'Invalid regular expression!' if matches.nil? + + expression = matches[:regexp] + flags = matches[:flags] + expression.prepend("(?#{flags})") if flags.present? + + UntrustedRegexp.new(expression, multiline: false) + end + end + end +end diff --git a/spec/features/issues/user_creates_branch_and_merge_request_spec.rb b/spec/features/issues/user_creates_branch_and_merge_request_spec.rb index 693ad89069c..0a006011c89 100644 --- a/spec/features/issues/user_creates_branch_and_merge_request_spec.rb +++ b/spec/features/issues/user_creates_branch_and_merge_request_spec.rb @@ -1,6 +1,7 @@ require 'rails_helper' describe 'User creates branch and merge request on issue page', :js do + let(:membership_level) { :developer } let(:user) { create(:user) } let!(:project) { create(:project, :repository) } let(:issue) { create(:issue, project: project, title: 'Cherry-Coloured Funk') } @@ -17,7 +18,7 @@ describe 'User creates branch and merge request on issue page', :js do context 'when signed in' do before do - project.add_developer(user) + project.add_user(user, membership_level) sign_in(user) end @@ -167,6 +168,39 @@ describe 'User creates branch and merge request on issue page', :js do expect(page).not_to have_css('.create-mr-dropdown-wrap') end end + + context 'when related branch exists' do + let!(:project) { create(:project, :repository, :private) } + let(:branch_name) { "#{issue.iid}-foo" } + + before do + project.repository.create_branch(branch_name, 'master') + + visit project_issue_path(project, issue) + end + + context 'when user is developer' do + it 'shows related branches' do + expect(page).to have_css('#related-branches') + + wait_for_requests + + expect(page).to have_content(branch_name) + end + end + + context 'when user is guest' do + let(:membership_level) { :guest } + + it 'does not show related branches' do + expect(page).not_to have_css('#related-branches') + + wait_for_requests + + expect(page).not_to have_content(branch_name) + end + end + end end private diff --git a/spec/lib/gitlab/checks/branch_check_spec.rb b/spec/lib/gitlab/checks/branch_check_spec.rb index 12beeecd470..8d5ab27a17c 100644 --- a/spec/lib/gitlab/checks/branch_check_spec.rb +++ b/spec/lib/gitlab/checks/branch_check_spec.rb @@ -108,64 +108,86 @@ describe Gitlab::Checks::BranchCheck do end context 'protected branch creation feature is enabled' do - context 'user is not allowed to create protected branches' do + context 'user can push to branch' do before do allow(user_access) - .to receive(:can_merge_to_branch?) + .to receive(:can_push_to_branch?) .with('feature') - .and_return(false) + .and_return(true) end - it 'raises an error' do - expect { subject.validate! }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to create protected branches on this project.') + it 'does not raise an error' do + expect { subject.validate! }.not_to raise_error end end - context 'user is allowed to create protected branches' do + context 'user cannot push to branch' do before do allow(user_access) - .to receive(:can_merge_to_branch?) + .to receive(:can_push_to_branch?) .with('feature') - .and_return(true) - - allow(project.repository) - .to receive(:branch_names_contains_sha) - .with(newrev) - .and_return(['branch']) + .and_return(false) end - context "newrev isn't in any protected branches" do + context 'user cannot merge to branch' do before do - allow(ProtectedBranch) - .to receive(:any_protected?) - .with(project, ['branch']) + allow(user_access) + .to receive(:can_merge_to_branch?) + .with('feature') .and_return(false) end it 'raises an error' do - expect { subject.validate! }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You can only use an existing protected branch ref as the basis of a new protected branch.') + expect { subject.validate! }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to create protected branches on this project.') end end - context 'newrev is included in a protected branch' do + context 'user can merge to branch' do before do - allow(ProtectedBranch) - .to receive(:any_protected?) - .with(project, ['branch']) + allow(user_access) + .to receive(:can_merge_to_branch?) + .with('feature') .and_return(true) + + allow(project.repository) + .to receive(:branch_names_contains_sha) + .with(newrev) + .and_return(['branch']) end - context 'via web interface' do - let(:protocol) { 'web' } + context "newrev isn't in any protected branches" do + before do + allow(ProtectedBranch) + .to receive(:any_protected?) + .with(project, ['branch']) + .and_return(false) + end - it 'allows branch creation' do - expect { subject.validate! }.not_to raise_error + it 'raises an error' do + expect { subject.validate! }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You can only use an existing protected branch ref as the basis of a new protected branch.') end end - context 'via SSH' do - it 'raises an error' do - expect { subject.validate! }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You can only create protected branches using the web interface and API.') + context 'newrev is included in a protected branch' do + before do + allow(ProtectedBranch) + .to receive(:any_protected?) + .with(project, ['branch']) + .and_return(true) + end + + context 'via web interface' do + let(:protocol) { 'web' } + + it 'allows branch creation' do + expect { subject.validate! }.not_to raise_error + end + end + + context 'via SSH' do + it 'raises an error' do + expect { subject.validate! }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You can only create protected branches using the web interface and API.') + end end end end diff --git a/spec/lib/gitlab/ci/build/policy/refs_spec.rb b/spec/lib/gitlab/ci/build/policy/refs_spec.rb index b4ddbf89b70..ec0450643c3 100644 --- a/spec/lib/gitlab/ci/build/policy/refs_spec.rb +++ b/spec/lib/gitlab/ci/build/policy/refs_spec.rb @@ -92,10 +92,23 @@ describe Gitlab::Ci::Build::Policy::Refs do .to be_satisfied_by(pipeline) end + it 'is satisfied when case-insensitive regexp matches pipeline ref' do + expect(described_class.new(['/DOCS-.*/i'])) + .to be_satisfied_by(pipeline) + end + it 'is not satisfied when regexp does not match pipeline ref' do expect(described_class.new(['/fix-.*/'])) .not_to be_satisfied_by(pipeline) end end + + context 'malicious regexp' do + let(:pipeline) { build_stubbed(:ci_pipeline, ref: malicious_text) } + + subject { described_class.new([malicious_regexp_ruby]) } + + include_examples 'malicious regexp' + end end end diff --git a/spec/lib/gitlab/ci/pipeline/expression/lexeme/pattern_spec.rb b/spec/lib/gitlab/ci/pipeline/expression/lexeme/pattern_spec.rb index 3ebc2e94727..cff7f57ceff 100644 --- a/spec/lib/gitlab/ci/pipeline/expression/lexeme/pattern_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/expression/lexeme/pattern_spec.rb @@ -85,7 +85,7 @@ describe Gitlab::Ci::Pipeline::Expression::Lexeme::Pattern do end it 'raises error if evaluated regexp is not valid' do - allow(Gitlab::UntrustedRegexp).to receive(:valid?).and_return(true) + allow(Gitlab::UntrustedRegexp::RubySyntax).to receive(:valid?).and_return(true) regexp = described_class.new('/invalid ( .*/') diff --git a/spec/lib/gitlab/ci/trace/stream_spec.rb b/spec/lib/gitlab/ci/trace/stream_spec.rb index 38626f728d7..e45ea1c2528 100644 --- a/spec/lib/gitlab/ci/trace/stream_spec.rb +++ b/spec/lib/gitlab/ci/trace/stream_spec.rb @@ -414,7 +414,7 @@ describe Gitlab::Ci::Trace::Stream, :clean_gitlab_redis_cache do context 'malicious regexp' do let(:data) { malicious_text } - let(:regex) { malicious_regexp } + let(:regex) { malicious_regexp_re2 } include_examples 'malicious regexp' end diff --git a/spec/lib/gitlab/diff/file_spec.rb b/spec/lib/gitlab/diff/file_spec.rb index 611c3e946ed..cc36060f864 100644 --- a/spec/lib/gitlab/diff/file_spec.rb +++ b/spec/lib/gitlab/diff/file_spec.rb @@ -72,6 +72,13 @@ describe Gitlab::Diff::File do expect(diff_file.diff_lines_for_serializer.last.type).to eq('match') end + context 'when called multiple times' do + it 'only adds bottom match line once' do + expect(diff_file.diff_lines_for_serializer.size).to eq(31) + expect(diff_file.diff_lines_for_serializer.size).to eq(31) + end + end + context 'when deleted' do let(:commit) { project.commit('d59c60028b053793cecfb4022de34602e1a9218e') } let(:diff_file) { commit.diffs.diff_file_with_old_path('files/js/commit.js.coffee') } diff --git a/spec/lib/gitlab/github_import/importer/pull_request_importer_spec.rb b/spec/lib/gitlab/github_import/importer/pull_request_importer_spec.rb index 37c3fae7cb7..680de47de2b 100644 --- a/spec/lib/gitlab/github_import/importer/pull_request_importer_spec.rb +++ b/spec/lib/gitlab/github_import/importer/pull_request_importer_spec.rb @@ -273,10 +273,10 @@ describe Gitlab::GithubImport::Importer::PullRequestImporter, :clean_gitlab_redi mr.state = 'opened' mr.save - # Ensure the project owner is creating the branches because the + # Ensure the project creator is creating the branches because the # merge request author may not have access to push to this - # repository. - allow(project.repository).to receive(:add_branch).with(project.owner, anything, anything).and_call_original + # repository. The project owner may also be a group. + allow(project.repository).to receive(:add_branch).with(project.creator, anything, anything).and_call_original importer.insert_git_data(mr, exists) diff --git a/spec/lib/gitlab/github_import/importer/repository_importer_spec.rb b/spec/lib/gitlab/github_import/importer/repository_importer_spec.rb index 47233ea6ee2..41810a8ec03 100644 --- a/spec/lib/gitlab/github_import/importer/repository_importer_spec.rb +++ b/spec/lib/gitlab/github_import/importer/repository_importer_spec.rb @@ -179,6 +179,17 @@ describe Gitlab::GithubImport::Importer::RepositoryImporter do describe '#import_repository' do it 'imports the repository' do + repo = double(:repo, default_branch: 'develop') + + expect(client) + .to receive(:repository) + .with('foo/bar') + .and_return(repo) + + expect(project) + .to receive(:change_head) + .with('develop') + expect(project) .to receive(:ensure_repository) diff --git a/spec/lib/gitlab/json_cache_spec.rb b/spec/lib/gitlab/json_cache_spec.rb index b7dc8234bdf..b82c09af306 100644 --- a/spec/lib/gitlab/json_cache_spec.rb +++ b/spec/lib/gitlab/json_cache_spec.rb @@ -146,6 +146,18 @@ describe Gitlab::JsonCache do expect(cache.read(key, BroadcastMessage)).to be_nil end + + it 'gracefully handles excluded fields from attributes during serialization' do + allow(backend).to receive(:read) + .with(expanded_key) + .and_return(broadcast_message.attributes.except("message_html").to_json) + + result = cache.read(key, BroadcastMessage) + + BroadcastMessage.cached_markdown_fields.html_fields.each do |field| + expect(result.public_send(field)).to be_nil + end + end end context 'when the cached value is an array' do @@ -327,7 +339,9 @@ describe Gitlab::JsonCache do .with(expanded_key) .and_return('{') - expect(cache.read(key, BroadcastMessage)).to be_nil + result = cache.fetch(key, as: BroadcastMessage) { 'block result' } + + expect(result).to eq 'block result' end it 'gracefully handles an empty hash' do @@ -335,7 +349,7 @@ describe Gitlab::JsonCache do .with(expanded_key) .and_return('{}') - expect(cache.read(key, BroadcastMessage)).to be_a(BroadcastMessage) + expect(cache.fetch(key, as: BroadcastMessage)).to be_a(BroadcastMessage) end it 'gracefully handles unknown attributes' do @@ -343,17 +357,19 @@ describe Gitlab::JsonCache do .with(expanded_key) .and_return(broadcast_message.attributes.merge(unknown_attribute: 1).to_json) - expect(cache.read(key, BroadcastMessage)).to be_nil + result = cache.fetch(key, as: BroadcastMessage) { 'block result' } + + expect(result).to eq 'block result' end it 'gracefully handles excluded fields from attributes during serialization' do - backend.write(expanded_key, broadcast_message.to_json) + allow(backend).to receive(:read) + .with(expanded_key) + .and_return(broadcast_message.attributes.except("message_html").to_json) result = cache.fetch(key, as: BroadcastMessage) { 'block result' } - excluded_fields = BroadcastMessage.cached_markdown_fields.html_fields - - (excluded_fields + ['cached_markdown_version']).each do |field| + BroadcastMessage.cached_markdown_fields.html_fields.each do |field| expect(result.public_send(field)).to be_nil end end diff --git a/spec/lib/gitlab/route_map_spec.rb b/spec/lib/gitlab/route_map_spec.rb index d672f7b5675..a39c774429e 100644 --- a/spec/lib/gitlab/route_map_spec.rb +++ b/spec/lib/gitlab/route_map_spec.rb @@ -60,7 +60,7 @@ describe Gitlab::RouteMap do subject do map = described_class.new(<<-"MAP".strip_heredoc) - - source: '#{malicious_regexp}' + - source: '#{malicious_regexp_re2}' public: '/' MAP diff --git a/spec/lib/gitlab/untrusted_regexp/ruby_syntax_spec.rb b/spec/lib/gitlab/untrusted_regexp/ruby_syntax_spec.rb new file mode 100644 index 00000000000..005d41580de --- /dev/null +++ b/spec/lib/gitlab/untrusted_regexp/ruby_syntax_spec.rb @@ -0,0 +1,72 @@ +require 'fast_spec_helper' +require 'support/shared_examples/malicious_regexp_shared_examples' + +describe Gitlab::UntrustedRegexp::RubySyntax do + describe '.matches_syntax?' do + it 'returns true if regexp is valid' do + expect(described_class.matches_syntax?('/some .* thing/')) + .to be true + end + + it 'returns true if regexp is invalid, but resembles regexp' do + expect(described_class.matches_syntax?('/some ( thing/')) + .to be true + end + end + + describe '.valid?' do + it 'returns true if regexp is valid' do + expect(described_class.valid?('/some .* thing/')) + .to be true + end + + it 'returns false if regexp is invalid' do + expect(described_class.valid?('/some ( thing/')) + .to be false + end + end + + describe '.fabricate' do + context 'when regexp is valid' do + it 'fabricates regexp without flags' do + expect(described_class.fabricate('/some .* thing/')).not_to be_nil + end + end + + context 'when regexp is a raw pattern' do + it 'returns error' do + expect(described_class.fabricate('some .* thing')).to be_nil + end + end + end + + describe '.fabricate!' do + context 'when regexp is using /regexp/ scheme with flags' do + it 'fabricates regexp with a single flag' do + regexp = described_class.fabricate!('/something/i') + + expect(regexp).to eq Gitlab::UntrustedRegexp.new('(?i)something') + expect(regexp.scan('SOMETHING')).to be_one + end + + it 'fabricates regexp with multiple flags' do + regexp = described_class.fabricate!('/something/im') + + expect(regexp).to eq Gitlab::UntrustedRegexp.new('(?im)something') + end + + it 'fabricates regexp without flags' do + regexp = described_class.fabricate!('/something/') + + expect(regexp).to eq Gitlab::UntrustedRegexp.new('something') + end + end + + context 'when regexp is a raw pattern' do + it 'raises an error' do + expect { described_class.fabricate!('some .* thing') } + .to raise_error(RegexpError) + end + end + end +end diff --git a/spec/lib/gitlab/untrusted_regexp_spec.rb b/spec/lib/gitlab/untrusted_regexp_spec.rb index 0a6ac0aa294..9d483f13a5e 100644 --- a/spec/lib/gitlab/untrusted_regexp_spec.rb +++ b/spec/lib/gitlab/untrusted_regexp_spec.rb @@ -2,48 +2,6 @@ require 'fast_spec_helper' require 'support/shared_examples/malicious_regexp_shared_examples' describe Gitlab::UntrustedRegexp do - describe '.valid?' do - it 'returns true if regexp is valid' do - expect(described_class.valid?('/some ( thing/')) - .to be false - end - - it 'returns true if regexp is invalid' do - expect(described_class.valid?('/some .* thing/')) - .to be true - end - end - - describe '.fabricate' do - context 'when regexp is using /regexp/ scheme with flags' do - it 'fabricates regexp with a single flag' do - regexp = described_class.fabricate('/something/i') - - expect(regexp).to eq described_class.new('(?i)something') - expect(regexp.scan('SOMETHING')).to be_one - end - - it 'fabricates regexp with multiple flags' do - regexp = described_class.fabricate('/something/im') - - expect(regexp).to eq described_class.new('(?im)something') - end - - it 'fabricates regexp without flags' do - regexp = described_class.fabricate('/something/') - - expect(regexp).to eq described_class.new('something') - end - end - - context 'when regexp is a raw pattern' do - it 'raises an error' do - expect { described_class.fabricate('some .* thing') } - .to raise_error(RegexpError) - end - end - end - describe '#initialize' do subject { described_class.new(pattern) } @@ -92,11 +50,41 @@ describe Gitlab::UntrustedRegexp do end end + describe '#match?' do + subject { described_class.new(regexp).match?(text) } + + context 'malicious regexp' do + let(:text) { malicious_text } + let(:regexp) { malicious_regexp_re2 } + + include_examples 'malicious regexp' + end + + context 'matching regexp' do + let(:regexp) { 'foo' } + let(:text) { 'foo' } + + it 'returns an array of nil matches' do + is_expected.to eq(true) + end + end + + context 'non-matching regexp' do + let(:regexp) { 'boo' } + let(:text) { 'foo' } + + it 'returns an array of nil matches' do + is_expected.to eq(false) + end + end + end + describe '#scan' do subject { described_class.new(regexp).scan(text) } + context 'malicious regexp' do let(:text) { malicious_text } - let(:regexp) { malicious_regexp } + let(:regexp) { malicious_regexp_re2 } include_examples 'malicious regexp' end diff --git a/spec/models/broadcast_message_spec.rb b/spec/models/broadcast_message_spec.rb index 89839709131..30ca07d5d2c 100644 --- a/spec/models/broadcast_message_spec.rb +++ b/spec/models/broadcast_message_spec.rb @@ -95,6 +95,12 @@ describe BroadcastMessage do end end + describe '#attributes' do + it 'includes message_html field' do + expect(subject.attributes.keys).to include("cached_markdown_version", "message_html") + end + end + describe '#active?' do it 'is truthy when started and not ended' do message = build(:broadcast_message) diff --git a/spec/models/concerns/cache_markdown_field_spec.rb b/spec/models/concerns/cache_markdown_field_spec.rb index 447279f19a8..7d555f15e39 100644 --- a/spec/models/concerns/cache_markdown_field_spec.rb +++ b/spec/models/concerns/cache_markdown_field_spec.rb @@ -23,6 +23,7 @@ describe CacheMarkdownField do include CacheMarkdownField cache_markdown_field :foo cache_markdown_field :baz, pipeline: :single_line + cache_markdown_field :zoo, whitelisted: true def self.add_attr(name) self.attribute_names += [name] @@ -35,7 +36,7 @@ describe CacheMarkdownField do add_attr :cached_markdown_version - [:foo, :foo_html, :bar, :baz, :baz_html].each do |name| + [:foo, :foo_html, :bar, :baz, :baz_html, :zoo, :zoo_html].each do |name| add_attr(name) end @@ -84,8 +85,8 @@ describe CacheMarkdownField do end describe '.attributes' do - it 'excludes cache attributes' do - expect(thing.attributes.keys.sort).to eq(%w[bar baz foo]) + it 'excludes cache attributes that is blacklisted by default' do + expect(thing.attributes.keys.sort).to eq(%w[bar baz cached_markdown_version foo zoo zoo_html]) end end @@ -297,7 +298,12 @@ describe CacheMarkdownField do it 'saves the changes using #update_columns' do expect(thing).to receive(:persisted?).and_return(true) expect(thing).to receive(:update_columns) - .with("foo_html" => updated_html, "baz_html" => "", "cached_markdown_version" => cache_version) + .with( + "foo_html" => updated_html, + "baz_html" => "", + "zoo_html" => "", + "cached_markdown_version" => cache_version + ) thing.refresh_markdown_cache! end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 71bd7972436..3beddaeddbd 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -2388,6 +2388,12 @@ describe Project do project.change_head(project.default_branch) end + it 'updates commit count' do + expect(ProjectCacheWorker).to receive(:perform_async).with(project.id, [], [:commit_count]) + + project.change_head(project.default_branch) + end + it 'copies the gitattributes' do expect(project.repository).to receive(:copy_gitattributes).with(project.default_branch) project.change_head(project.default_branch) diff --git a/spec/policies/project_policy_spec.rb b/spec/policies/project_policy_spec.rb index 772d1fbee2b..c12c4677af1 100644 --- a/spec/policies/project_policy_spec.rb +++ b/spec/policies/project_policy_spec.rb @@ -15,7 +15,7 @@ describe ProjectPolicy do read_project_for_iids read_issue_iid read_label read_milestone read_project_snippet read_project_member read_note create_project create_issue create_note upload_file create_merge_request_in - award_emoji read_release + award_emoji ] end @@ -24,7 +24,7 @@ describe ProjectPolicy do download_code fork_project create_project_snippet update_issue admin_issue admin_label admin_list read_commit_status read_build read_container_image read_pipeline read_environment read_deployment - read_merge_request download_wiki_code read_sentry_issue + read_merge_request download_wiki_code read_sentry_issue read_release ] end diff --git a/spec/requests/api/releases_spec.rb b/spec/requests/api/releases_spec.rb index 1f317971a66..71ec091c42c 100644 --- a/spec/requests/api/releases_spec.rb +++ b/spec/requests/api/releases_spec.rb @@ -4,12 +4,14 @@ describe API::Releases do let(:project) { create(:project, :repository, :private) } let(:maintainer) { create(:user) } let(:reporter) { create(:user) } + let(:guest) { create(:user) } let(:non_project_member) { create(:user) } let(:commit) { create(:commit, project: project) } before do project.add_maintainer(maintainer) project.add_reporter(reporter) + project.add_guest(guest) project.repository.add_tag(maintainer, 'v0.1', commit.id) project.repository.add_tag(maintainer, 'v0.2', commit.id) @@ -66,6 +68,24 @@ describe API::Releases do end end + context 'when user is a guest' do + it 'responds 403 Forbidden' do + get api("/projects/#{project.id}/releases", guest) + + expect(response).to have_gitlab_http_status(:forbidden) + end + + context 'when project is public' do + let(:project) { create(:project, :repository, :public) } + + it 'responds 200 OK' do + get api("/projects/#{project.id}/releases", guest) + + expect(response).to have_gitlab_http_status(:ok) + end + end + end + context 'when user is not a project member' do it 'cannot find the project' do get api("/projects/#{project.id}/releases", non_project_member) @@ -189,6 +209,24 @@ describe API::Releases do end end end + + context 'when user is a guest' do + it 'responds 403 Forbidden' do + get api("/projects/#{project.id}/releases/v0.1", guest) + + expect(response).to have_gitlab_http_status(:forbidden) + end + + context 'when project is public' do + let(:project) { create(:project, :repository, :public) } + + it 'responds 200 OK' do + get api("/projects/#{project.id}/releases/v0.1", guest) + + expect(response).to have_gitlab_http_status(:ok) + end + end + end end context 'when specified tag is not found in the project' do diff --git a/spec/support/shared_examples/malicious_regexp_shared_examples.rb b/spec/support/shared_examples/malicious_regexp_shared_examples.rb index db69b75c0c8..a86050e2cf2 100644 --- a/spec/support/shared_examples/malicious_regexp_shared_examples.rb +++ b/spec/support/shared_examples/malicious_regexp_shared_examples.rb @@ -2,7 +2,8 @@ require 'timeout' shared_examples 'malicious regexp' do let(:malicious_text) { 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa!' } - let(:malicious_regexp) { '(?i)^(([a-z])+.)+[A-Z]([a-z])+$' } + let(:malicious_regexp_re2) { '(?i)^(([a-z])+.)+[A-Z]([a-z])+$' } + let(:malicious_regexp_ruby) { '/^(([a-z])+.)+[A-Z]([a-z])+$/i' } it 'takes under a second' do expect { Timeout.timeout(1) { subject } }.not_to raise_error diff --git a/vendor/project_templates/android.tar.gz b/vendor/project_templates/android.tar.gz Binary files differindex 3df17a0d9a6..277aedaa1ca 100644 --- a/vendor/project_templates/android.tar.gz +++ b/vendor/project_templates/android.tar.gz |