diff options
137 files changed, 2834 insertions, 332 deletions
diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION index 95e26be3ee6..ede3db7b44f 100644 --- a/GITALY_SERVER_VERSION +++ b/GITALY_SERVER_VERSION @@ -1 +1 @@ -1.53.2 +1.53.3 diff --git a/GITLAB_WORKHORSE_VERSION b/GITLAB_WORKHORSE_VERSION index df5119ec64e..d139a75408e 100644 --- a/GITLAB_WORKHORSE_VERSION +++ b/GITLAB_WORKHORSE_VERSION @@ -1 +1 @@ -8.7.0 +8.7.1 diff --git a/app/assets/javascripts/behaviors/markdown/render_math.js b/app/assets/javascripts/behaviors/markdown/render_math.js index a68936d79e2..53867b3096b 100644 --- a/app/assets/javascripts/behaviors/markdown/render_math.js +++ b/app/assets/javascripts/behaviors/markdown/render_math.js @@ -1,6 +1,5 @@ -import $ from 'jquery'; -import { __ } from '~/locale'; import flash from '~/flash'; +import { s__, sprintf } from '~/locale'; // Renders math using KaTeX in any element with the // `js-render-math` class @@ -10,21 +9,131 @@ import flash from '~/flash'; // <code class="js-render-math"></div> // -// Loop over all math elements and render math -function renderWithKaTeX(elements, katex) { - elements.each(function katexElementsLoop() { - const mathNode = $('<span></span>'); - const $this = $(this); - - const display = $this.attr('data-math-style') === 'display'; - try { - katex.render($this.text(), mathNode.get(0), { displayMode: display, throwOnError: false }); - mathNode.insertAfter($this); - $this.remove(); - } catch (err) { - throw err; +const MAX_MATH_CHARS = 1000; +const MAX_RENDER_TIME_MS = 2000; + +// These messages might be used with inline errors in the future. Keep them around. For now, we will +// display a single error message using flash(). + +// const CHAR_LIMIT_EXCEEDED_MSG = sprintf( +// s__( +// 'math|The following math is too long. For performance reasons, math blocks are limited to %{maxChars} characters. Try splitting up this block, or include an image instead.', +// ), +// { maxChars: MAX_MATH_CHARS }, +// ); +// const RENDER_TIME_EXCEEDED_MSG = s__( +// "math|The math in this entry is taking too long to render. Any math below this point won't be shown. Consider splitting it among multiple entries.", +// ); + +const RENDER_FLASH_MSG = sprintf( + s__( + 'math|The math in this entry is taking too long to render and may not be displayed as expected. For performance reasons, math blocks are also limited to %{maxChars} characters. Consider splitting up large formulae, splitting math blocks among multiple entries, or using an image instead.', + ), + { maxChars: MAX_MATH_CHARS }, +); + +// Wait for the browser to reflow the layout. Reflowing SVG takes time. +// This has to wrap the inner function, otherwise IE/Edge throw "invalid calling object". +const waitForReflow = fn => { + window.requestAnimationFrame(fn); +}; + +/** + * Renders math blocks sequentially while protecting against DoS attacks. Math blocks have a maximum character limit of MAX_MATH_CHARS. If rendering math takes longer than MAX_RENDER_TIME_MS, all subsequent math blocks are skipped and an error message is shown. + */ +class SafeMathRenderer { + /* + How this works: + + The performance bottleneck in rendering math is in the browser trying to reflow the generated SVG. + During this time, the JS is blocked and the page becomes unresponsive. + We want to render math blocks one by one until a certain time is exceeded, after which we stop + rendering subsequent math blocks, to protect against DoS. However, browsers do reflowing in an + asynchronous task, so we can't time it synchronously. + + SafeMathRenderer essentially does the following: + 1. Replaces all math blocks with placeholders so that they're not mistakenly rendered twice. + 2. Places each placeholder element in a queue. + 3. Renders the element at the head of the queue and waits for reflow. + 4. After reflow, gets the elapsed time since step 3 and repeats step 3 until the queue is empty. + */ + queue = []; + totalMS = 0; + + constructor(elements, katex) { + this.elements = elements; + this.katex = katex; + + this.renderElement = this.renderElement.bind(this); + this.render = this.render.bind(this); + } + + renderElement() { + if (!this.queue.length) { + return; } - }); + + const el = this.queue.shift(); + const text = el.textContent; + + el.removeAttribute('style'); + + if (this.totalMS >= MAX_RENDER_TIME_MS || text.length > MAX_MATH_CHARS) { + if (!this.flashShown) { + flash(RENDER_FLASH_MSG); + this.flashShown = true; + } + + // Show unrendered math code + const codeElement = document.createElement('pre'); + codeElement.className = 'code'; + codeElement.textContent = el.textContent; + el.parentNode.replaceChild(codeElement, el); + + // Render the next math + this.renderElement(); + } else { + this.startTime = Date.now(); + + try { + el.innerHTML = this.katex.renderToString(text, { + displayMode: el.getAttribute('data-math-style') === 'display', + throwOnError: true, + maxSize: 20, + maxExpand: 20, + }); + } catch { + // Don't show a flash for now because it would override an existing flash message + el.textContent = s__('math|There was an error rendering this math block'); + // el.style.color = '#d00'; + el.className = 'katex-error'; + } + + // Give the browser time to reflow the svg + waitForReflow(() => { + const deltaTime = Date.now() - this.startTime; + this.totalMS += deltaTime; + + this.renderElement(); + }); + } + } + + render() { + // Replace math blocks with a placeholder so they aren't rendered twice + this.elements.forEach(el => { + const placeholder = document.createElement('span'); + placeholder.style.display = 'none'; + placeholder.setAttribute('data-math-style', el.getAttribute('data-math-style')); + placeholder.textContent = el.textContent; + el.parentNode.replaceChild(placeholder, el); + this.queue.push(placeholder); + }); + + // If we wait for the browser thread to settle down a bit, math rendering becomes 5-10x faster + // and less prone to timeouts. + setTimeout(this.renderElement, 400); + } } export default function renderMath($els) { @@ -34,7 +143,8 @@ export default function renderMath($els) { import(/* webpackChunkName: 'katex' */ 'katex/dist/katex.min.css'), ]) .then(([katex]) => { - renderWithKaTeX($els, katex); + const renderer = new SafeMathRenderer($els.get(), katex); + renderer.render(); }) - .catch(() => flash(__('An error occurred while rendering KaTeX'))); + .catch(() => {}); } diff --git a/app/controllers/concerns/issuable_actions.rb b/app/controllers/concerns/issuable_actions.rb index 6fa2f75be33..0287380fb1b 100644 --- a/app/controllers/concerns/issuable_actions.rb +++ b/app/controllers/concerns/issuable_actions.rb @@ -111,7 +111,7 @@ module IssuableActions end notes = prepare_notes_for_rendering(notes) - notes = notes.reject { |n| n.cross_reference_not_visible_for?(current_user) } + notes = notes.select { |n| n.visible_for?(current_user) } discussions = Discussion.build_collection(notes, issuable) diff --git a/app/controllers/concerns/notes_actions.rb b/app/controllers/concerns/notes_actions.rb index 0098c4cdf4c..52c3a34ffe4 100644 --- a/app/controllers/concerns/notes_actions.rb +++ b/app/controllers/concerns/notes_actions.rb @@ -29,7 +29,7 @@ module NotesActions end notes = prepare_notes_for_rendering(notes) - notes = notes.reject { |n| n.cross_reference_not_visible_for?(current_user) } + notes = notes.select { |n| n.visible_for?(current_user) } notes_json[:notes] = if use_note_serializer? diff --git a/app/controllers/concerns/uploads_actions.rb b/app/controllers/concerns/uploads_actions.rb index f5d35379e10..60a68cec3c3 100644 --- a/app/controllers/concerns/uploads_actions.rb +++ b/app/controllers/concerns/uploads_actions.rb @@ -127,4 +127,8 @@ module UploadsActions def model strong_memoize(:model) { find_model } end + + def workhorse_authorize_request? + action_name == 'authorize' + end end diff --git a/app/controllers/groups/runners_controller.rb b/app/controllers/groups/runners_controller.rb index f8e32451b02..af2b2cbd1fd 100644 --- a/app/controllers/groups/runners_controller.rb +++ b/app/controllers/groups/runners_controller.rb @@ -3,7 +3,7 @@ class Groups::RunnersController < Groups::ApplicationController # Proper policies should be implemented per # https://gitlab.com/gitlab-org/gitlab-ce/issues/45894 - before_action :authorize_admin_pipeline! + before_action :authorize_admin_group! before_action :runner, only: [:edit, :update, :destroy, :pause, :resume, :show] @@ -50,10 +50,6 @@ class Groups::RunnersController < Groups::ApplicationController @runner ||= @group.runners.find(params[:id]) end - def authorize_admin_pipeline! - return render_404 unless can?(current_user, :admin_pipeline, group) - end - def runner_params params.require(:runner).permit(Ci::Runner::FORM_EDITABLE) end diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index f4d381244d9..a2e6f878f90 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -12,6 +12,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo skip_before_action :merge_request, only: [:index, :bulk_update] before_action :whitelist_query_limiting, only: [:assign_related_issues, :update] before_action :authorize_update_issuable!, only: [:close, :edit, :update, :remove_wip, :sort] + before_action :authorize_test_reports!, only: [:test_reports] before_action :set_issuables_index, only: [:index] before_action :authenticate_user!, only: [:assign_related_issues] before_action :check_user_can_push_to_source_branch!, only: [:rebase] @@ -188,7 +189,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo def pipeline_status render json: PipelineSerializer .new(project: @project, current_user: @current_user) - .represent_status(@merge_request.head_pipeline) + .represent_status(head_pipeline) end def ci_environments_status @@ -238,6 +239,13 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo private + def head_pipeline + strong_memoize(:head_pipeline) do + pipeline = @merge_request.head_pipeline + pipeline if can?(current_user, :read_pipeline, pipeline) + end + end + def ci_environments_status_on_merge_result? params[:environment_target] == 'merge_commit' end @@ -336,4 +344,9 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo render json: { status_reason: 'Unknown error' }, status: :internal_server_error end end + + def authorize_test_reports! + # MergeRequest#actual_head_pipeline is the pipeline accessed in MergeRequest#compare_reports. + return render_404 unless can?(current_user, :read_build, merge_request.actual_head_pipeline) + end end diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index a841859621e..d5c63c4e495 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -21,13 +21,18 @@ class SessionsController < Devise::SessionsController prepend_before_action :ensure_password_authentication_enabled!, if: :password_based_login?, only: [:create] before_action :auto_sign_in_with_provider, only: [:new] + before_action :store_unauthenticated_sessions, only: [:new] + before_action :save_failed_login, if: :action_new_and_failed_login? before_action :load_recaptcha - after_action :log_failed_login, only: [:new], if: :failed_login? + after_action :log_failed_login, if: :action_new_and_failed_login? + + helper_method :captcha_enabled?, :captcha_on_login_required? helper_method :captcha_enabled? CAPTCHA_HEADER = 'X-GitLab-Show-Login-Captcha'.freeze + MAX_FAILED_LOGIN_ATTEMPTS = 5 def new set_minimum_password_length @@ -71,10 +76,14 @@ class SessionsController < Devise::SessionsController request.headers[CAPTCHA_HEADER] && Gitlab::Recaptcha.enabled? end + def captcha_on_login_required? + Gitlab::Recaptcha.enabled_on_login? && unverified_anonymous_user? + end + # From https://github.com/plataformatec/devise/wiki/How-To:-Use-Recaptcha-with-Devise#devisepasswordscontroller def check_captcha return unless user_params[:password].present? - return unless captcha_enabled? + return unless captcha_enabled? || captcha_on_login_required? return unless Gitlab::Recaptcha.load_configurations! if verify_recaptcha @@ -116,10 +125,28 @@ class SessionsController < Devise::SessionsController Gitlab::AppLogger.info("Failed Login: username=#{user_params[:login]} ip=#{request.remote_ip}") end + def action_new_and_failed_login? + action_name == 'new' && failed_login? + end + + def save_failed_login + session[:failed_login_attempts] ||= 0 + session[:failed_login_attempts] += 1 + end + def failed_login? (options = request.env["warden.options"]) && options[:action] == "unauthenticated" end + # storing sessions per IP lets us check if there are associated multiple + # anonymous sessions with one IP and prevent situations when there are + # multiple attempts of logging in + def store_unauthenticated_sessions + return if current_user + + Gitlab::AnonymousSession.new(request.remote_ip, session_id: request.session.id).store_session_id_per_ip + end + # Handle an "initial setup" state, where there's only one user, it's an admin, # and they require a password change. # rubocop: disable CodeReuse/ActiveRecord @@ -230,6 +257,18 @@ class SessionsController < Devise::SessionsController @ldap_servers ||= Gitlab::Auth::LDAP::Config.available_servers end + def unverified_anonymous_user? + exceeded_failed_login_attempts? || exceeded_anonymous_sessions? + end + + def exceeded_failed_login_attempts? + session.fetch(:failed_login_attempts, 0) > MAX_FAILED_LOGIN_ATTEMPTS + end + + def exceeded_anonymous_sessions? + Gitlab::AnonymousSession.new(request.remote_ip).stored_sessions >= MAX_FAILED_LOGIN_ATTEMPTS + end + def authentication_method if user_params[:otp_attempt] "two-factor" diff --git a/app/controllers/uploads_controller.rb b/app/controllers/uploads_controller.rb index 94bd18f70d4..2adfeab182e 100644 --- a/app/controllers/uploads_controller.rb +++ b/app/controllers/uploads_controller.rb @@ -2,6 +2,7 @@ class UploadsController < ApplicationController include UploadsActions + include WorkhorseRequest UnknownUploadModelError = Class.new(StandardError) @@ -21,7 +22,8 @@ class UploadsController < ApplicationController before_action :upload_mount_satisfied? before_action :find_model before_action :authorize_access!, only: [:show] - before_action :authorize_create_access!, only: [:create] + before_action :authorize_create_access!, only: [:create, :authorize] + before_action :verify_workhorse_api!, only: [:authorize] def uploader_class PersonalFileUploader @@ -72,7 +74,7 @@ class UploadsController < ApplicationController end def render_unauthorized - if current_user + if current_user || workhorse_authorize_request? render_404 else authenticate_user! diff --git a/app/helpers/application_settings_helper.rb b/app/helpers/application_settings_helper.rb index 4bf9b708401..24236b55e0f 100644 --- a/app/helpers/application_settings_helper.rb +++ b/app/helpers/application_settings_helper.rb @@ -162,6 +162,10 @@ module ApplicationSettingsHelper :allow_local_requests_from_hooks_and_services, :dns_rebinding_protection_enabled, :archive_builds_in_human_readable, + :asset_proxy_enabled, + :asset_proxy_secret_key, + :asset_proxy_url, + :asset_proxy_whitelist, :authorized_keys_enabled, :auto_devops_enabled, :auto_devops_domain, @@ -228,6 +232,7 @@ module ApplicationSettingsHelper :recaptcha_enabled, :recaptcha_private_key, :recaptcha_site_key, + :login_recaptcha_protection_enabled, :receive_max_input_size, :repository_checks_enabled, :repository_storages, diff --git a/app/helpers/labels_helper.rb b/app/helpers/labels_helper.rb index db4f29cd996..bed6eb90209 100644 --- a/app/helpers/labels_helper.rb +++ b/app/helpers/labels_helper.rb @@ -72,7 +72,7 @@ module LabelsHelper end def label_tooltip_title(label) - label.description + Sanitize.clean(label.description) end def suggested_colors diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index 8e558487c1c..6aadbbc9d03 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -16,12 +16,19 @@ class ApplicationSetting < ApplicationRecord # fix a lot of tests using allow_any_instance_of include ApplicationSettingImplementation + attr_encrypted :asset_proxy_secret_key, + mode: :per_attribute_iv, + insecure_mode: true, + key: Settings.attr_encrypted_db_key_base_truncated, + algorithm: 'aes-256-cbc' + serialize :restricted_visibility_levels # rubocop:disable Cop/ActiveRecordSerialize serialize :import_sources # rubocop:disable Cop/ActiveRecordSerialize serialize :disabled_oauth_sign_in_sources, Array # rubocop:disable Cop/ActiveRecordSerialize serialize :domain_whitelist, Array # rubocop:disable Cop/ActiveRecordSerialize serialize :domain_blacklist, Array # rubocop:disable Cop/ActiveRecordSerialize serialize :repository_storages # rubocop:disable Cop/ActiveRecordSerialize + serialize :asset_proxy_whitelist, Array # rubocop:disable Cop/ActiveRecordSerialize ignore_column :koding_url ignore_column :koding_enabled @@ -68,11 +75,11 @@ class ApplicationSetting < ApplicationRecord validates :recaptcha_site_key, presence: true, - if: :recaptcha_enabled + if: :recaptcha_or_login_protection_enabled validates :recaptcha_private_key, presence: true, - if: :recaptcha_enabled + if: :recaptcha_or_login_protection_enabled validates :akismet_api_key, presence: true, @@ -180,6 +187,17 @@ class ApplicationSetting < ApplicationRecord allow_nil: true, numericality: { only_integer: true, greater_than_or_equal_to: 0, less_than: 65536 } + validates :asset_proxy_url, + presence: true, + allow_blank: false, + url: true, + if: :asset_proxy_enabled? + + validates :asset_proxy_secret_key, + presence: true, + allow_blank: false, + if: :asset_proxy_enabled? + SUPPORTED_KEY_TYPES.each do |type| validates :"#{type}_key_restriction", presence: true, key_restriction: { type: type } end @@ -280,4 +298,8 @@ class ApplicationSetting < ApplicationRecord def self.cache_backend Gitlab::ThreadMemoryCache.cache_backend end + + def recaptcha_or_login_protection_enabled + recaptcha_enabled || login_recaptcha_protection_enabled + end end diff --git a/app/models/application_setting_implementation.rb b/app/models/application_setting_implementation.rb index df4caed175d..17ed3f3688d 100644 --- a/app/models/application_setting_implementation.rb +++ b/app/models/application_setting_implementation.rb @@ -21,8 +21,9 @@ module ApplicationSettingImplementation after_sign_up_text: nil, akismet_enabled: false, allow_local_requests_from_hooks_and_services: false, - dns_rebinding_protection_enabled: true, + asset_proxy_enabled: false, authorized_keys_enabled: true, # TODO default to false if the instance is configured to use AuthorizedKeysCommand + commit_email_hostname: default_commit_email_hostname, container_registry_token_expire_delay: 5, default_artifacts_expire_in: '30 days', default_branch_protection: Settings.gitlab['default_branch_protection'], @@ -31,7 +32,9 @@ module ApplicationSettingImplementation default_project_visibility: Settings.gitlab.default_projects_features['visibility_level'], default_projects_limit: Settings.gitlab['default_projects_limit'], default_snippet_visibility: Settings.gitlab.default_projects_features['visibility_level'], + diff_max_patch_bytes: Gitlab::Git::Diff::DEFAULT_MAX_PATCH_BYTES, disabled_oauth_sign_in_sources: [], + dns_rebinding_protection_enabled: true, domain_whitelist: Settings.gitlab['domain_whitelist'], dsa_key_restriction: 0, ecdsa_key_restriction: 0, @@ -50,6 +53,7 @@ module ApplicationSettingImplementation housekeeping_gc_period: 200, housekeeping_incremental_repack_period: 10, import_sources: Settings.gitlab['import_sources'], + local_markdown_version: 0, max_artifacts_size: Settings.artifacts['max_size'], max_attachment_size: Settings.gitlab['max_attachment_size'], mirror_available: true, @@ -61,7 +65,9 @@ module ApplicationSettingImplementation plantuml_url: nil, polling_interval_multiplier: 1, project_export_enabled: true, + protected_ci_variables: false, recaptcha_enabled: false, + login_recaptcha_protection_enabled: false, repository_checks_enabled: true, repository_storages: ['default'], require_two_factor_authentication: false, @@ -92,11 +98,7 @@ module ApplicationSettingImplementation user_default_external: false, user_default_internal_regex: nil, user_show_add_ssh_key_message: true, - usage_stats_set_by_user_id: nil, - diff_max_patch_bytes: Gitlab::Git::Diff::DEFAULT_MAX_PATCH_BYTES, - commit_email_hostname: default_commit_email_hostname, - protected_ci_variables: false, - local_markdown_version: 0 + usage_stats_set_by_user_id: nil } end @@ -139,17 +141,20 @@ module ApplicationSettingImplementation end def domain_whitelist_raw=(values) - self.domain_whitelist = [] - self.domain_whitelist = values.split(DOMAIN_LIST_SEPARATOR) - self.domain_whitelist.reject! { |d| d.empty? } - self.domain_whitelist + self.domain_whitelist = domain_string_to_array(values) end def domain_blacklist_raw=(values) - self.domain_blacklist = [] - self.domain_blacklist = values.split(DOMAIN_LIST_SEPARATOR) - self.domain_blacklist.reject! { |d| d.empty? } - self.domain_blacklist + self.domain_blacklist = domain_string_to_array(values) + end + + def asset_proxy_whitelist=(values) + values = domain_string_to_array(values) if values.is_a?(String) + + # make sure we always whitelist the running host + values << Gitlab.config.gitlab.host unless values.include?(Gitlab.config.gitlab.host) + + self[:asset_proxy_whitelist] = values end def domain_blacklist_file=(file) @@ -276,4 +281,10 @@ module ApplicationSettingImplementation def expire_performance_bar_allowed_user_ids_cache Gitlab::PerformanceBar.expire_allowed_user_ids_cache end + + def domain_string_to_array(values) + domain_list = values.split(DOMAIN_LIST_SEPARATOR).map(&:strip) + domain_list.reject! { |d| d.empty? } + domain_list + end end diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index 2262282e647..6fc4ac80955 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -203,6 +203,7 @@ module Ci scope :for_sha, -> (sha) { where(sha: sha) } scope :for_source_sha, -> (source_sha) { where(source_sha: source_sha) } scope :for_sha_or_source_sha, -> (sha) { for_sha(sha).or(for_source_sha(sha)) } + scope :created_after, -> (time) { where('ci_pipelines.created_at > ?', time) } scope :triggered_by_merge_request, -> (merge_request) do where(source: :merge_request_event, merge_request: merge_request) diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 952de92cae1..052d1678bc2 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -73,6 +73,7 @@ module Issuable validates :author, presence: true validates :title, presence: true, length: { maximum: 255 } + validates :description, length: { maximum: Gitlab::Database::MAX_TEXT_SIZE_LIMIT }, allow_blank: true validate :milestone_is_valid scope :authored, ->(user) { where(author_id: user) } diff --git a/app/models/label.rb b/app/models/label.rb index b83e0862bab..b86d4aa84ff 100644 --- a/app/models/label.rb +++ b/app/models/label.rb @@ -193,7 +193,11 @@ class Label < ApplicationRecord end def title=(value) - write_attribute(:title, sanitize_title(value)) if value.present? + write_attribute(:title, sanitize_value(value)) if value.present? + end + + def description=(value) + write_attribute(:description, sanitize_value(value)) if value.present? end ## @@ -254,7 +258,7 @@ class Label < ApplicationRecord end end - def sanitize_title(value) + def sanitize_value(value) CGI.unescapeHTML(Sanitize.clean(value.to_s)) end diff --git a/app/models/note.rb b/app/models/note.rb index 5c31cff9816..3cc6d46a5e0 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -85,6 +85,7 @@ class Note < ApplicationRecord delegate :title, to: :noteable, allow_nil: true validates :note, presence: true + validates :note, length: { maximum: Gitlab::Database::MAX_TEXT_SIZE_LIMIT } validates :project, presence: true, if: :for_project_noteable? # Attachments are deprecated and are handled by Markdown uploader @@ -327,6 +328,10 @@ class Note < ApplicationRecord cross_reference? && !all_referenced_mentionables_allowed?(user) end + def visible_for?(user) + !cross_reference_not_visible_for?(user) + end + def award_emoji? can_be_award_emoji? && contains_emoji_only? end diff --git a/app/models/project_services/jira_service.rb b/app/models/project_services/jira_service.rb index e571700fd02..222d8361d3f 100644 --- a/app/models/project_services/jira_service.rb +++ b/app/models/project_services/jira_service.rb @@ -64,7 +64,12 @@ class JiraService < IssueTrackerService end def client - @client ||= JIRA::Client.new(options) + @client ||= begin + JIRA::Client.new(options).tap do |client| + # Replaces JIRA default http client with our implementation + client.request_client = Gitlab::Jira::HttpClient.new(client.options) + end + end end def help diff --git a/app/models/system_note_metadata.rb b/app/models/system_note_metadata.rb index 55da37c9545..f7c9081d75b 100644 --- a/app/models/system_note_metadata.rb +++ b/app/models/system_note_metadata.rb @@ -9,7 +9,7 @@ class SystemNoteMetadata < ApplicationRecord TYPES_WITH_CROSS_REFERENCES = %w[ commit cross_reference close duplicate - moved + moved merge ].freeze ICON_TYPES = %w[ diff --git a/app/models/user.rb b/app/models/user.rb index 0fd3daa3383..58509976135 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -630,6 +630,13 @@ class User < ApplicationRecord end end + # will_save_change_to_attribute? is used by Devise to check if it is necessary + # to clear any existing reset_password_tokens before updating an authentication_key + # and login in our case is a virtual attribute to allow login by username or email. + def will_save_change_to_login? + will_save_change_to_username? || will_save_change_to_email? + end + def unique_email if !emails.exists?(email: email) && Email.exists?(email: email) errors.add(:email, _('has already been taken')) diff --git a/app/policies/issue_policy.rb b/app/policies/issue_policy.rb index dd8c5d49cf4..fa252af55e4 100644 --- a/app/policies/issue_policy.rb +++ b/app/policies/issue_policy.rb @@ -5,6 +5,8 @@ class IssuePolicy < IssuablePolicy # Make sure to sync this class checks with issue.rb to avoid security problems. # Check commit 002ad215818450d2cbbc5fa065850a953dc7ada8 for more information. + extend ProjectPolicy::ClassMethods + desc "User can read confidential issues" condition(:can_read_confidential) do @user && IssueCollection.new([@subject]).visible_to(@user).any? @@ -14,13 +16,12 @@ class IssuePolicy < IssuablePolicy condition(:confidential, scope: :subject) { @subject.confidential? } rule { confidential & ~can_read_confidential }.policy do - prevent :read_issue + prevent(*create_read_update_admin_destroy(:issue)) prevent :read_issue_iid - prevent :update_issue - prevent :admin_issue - prevent :create_note end + rule { ~can?(:read_issue) }.prevent :create_note + rule { locked }.policy do prevent :reopen_issue end diff --git a/app/policies/merge_request_policy.rb b/app/policies/merge_request_policy.rb index a3692857ff4..5ad7bdabdff 100644 --- a/app/policies/merge_request_policy.rb +++ b/app/policies/merge_request_policy.rb @@ -4,4 +4,10 @@ class MergeRequestPolicy < IssuablePolicy rule { locked }.policy do prevent :reopen_merge_request end + + # Only users who can read the merge request can comment. + # Although :read_merge_request is computed in the policy context, + # it would not be safe to prevent :create_note there, since + # note permissions are shared, and this would apply too broadly. + rule { ~can?(:read_merge_request) }.prevent :create_note end diff --git a/app/services/application_settings/update_service.rb b/app/services/application_settings/update_service.rb index 7eeaf8aade1..04931108873 100644 --- a/app/services/application_settings/update_service.rb +++ b/app/services/application_settings/update_service.rb @@ -6,6 +6,8 @@ module ApplicationSettings attr_reader :params, :application_setting + MARKDOWN_CACHE_INVALIDATING_PARAMS = %w(asset_proxy_enabled asset_proxy_url asset_proxy_secret_key asset_proxy_whitelist).freeze + def execute validate_classification_label(application_setting, :external_authorization_service_default_label) @@ -23,7 +25,13 @@ module ApplicationSettings params[:usage_stats_set_by_user_id] = current_user.id end - @application_setting.update(@params) + @application_setting.assign_attributes(params) + + if invalidate_markdown_cache? + @application_setting[:local_markdown_version] = @application_setting.local_markdown_version + 1 + end + + @application_setting.save end private @@ -32,6 +40,11 @@ module ApplicationSettings params.key?(:usage_ping_enabled) || params.key?(:version_check_enabled) end + def invalidate_markdown_cache? + !params.key?(:local_markdown_version) && + (@application_setting.changes.keys & MARKDOWN_CACHE_INVALIDATING_PARAMS).any? + end + def update_terms(terms) return unless terms.present? diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb index cdcc4b15bea..29317f1176e 100644 --- a/app/services/ci/create_pipeline_service.rb +++ b/app/services/ci/create_pipeline_service.rb @@ -15,7 +15,8 @@ module Ci Gitlab::Ci::Pipeline::Chain::Limit::Size, Gitlab::Ci::Pipeline::Chain::Populate, Gitlab::Ci::Pipeline::Chain::Create, - Gitlab::Ci::Pipeline::Chain::Limit::Activity].freeze + Gitlab::Ci::Pipeline::Chain::Limit::Activity, + Gitlab::Ci::Pipeline::Chain::Limit::JobActivity].freeze def execute(source, ignore_skip_ci: false, save_on_errors: true, trigger_request: nil, schedule: nil, merge_request: nil, **options, &block) @pipeline = Ci::Pipeline.new diff --git a/app/services/projects/create_service.rb b/app/services/projects/create_service.rb index 89dc4375c63..942a45286b2 100644 --- a/app/services/projects/create_service.rb +++ b/app/services/projects/create_service.rb @@ -5,9 +5,11 @@ module Projects include ValidatesClassificationLabel def initialize(user, params) - @current_user, @params = user, params.dup - @skip_wiki = @params.delete(:skip_wiki) + @current_user, @params = user, params.dup + @skip_wiki = @params.delete(:skip_wiki) @initialize_with_readme = Gitlab::Utils.to_boolean(@params.delete(:initialize_with_readme)) + @import_data = @params.delete(:import_data) + @relations_block = @params.delete(:relations_block) end def execute @@ -15,14 +17,11 @@ module Projects return ::Projects::CreateFromTemplateService.new(current_user, params).execute end - import_data = params.delete(:import_data) - relations_block = params.delete(:relations_block) - @project = Project.new(params) # Make sure that the user is allowed to use the specified visibility level - unless Gitlab::VisibilityLevel.allowed_for?(current_user, @project.visibility_level) - deny_visibility_level(@project) + if project_visibility.restricted? + deny_visibility_level(@project, project_visibility.visibility_level) return @project end @@ -44,7 +43,7 @@ module Projects @project.namespace_id = current_user.namespace_id end - relations_block&.call(@project) + @relations_block&.call(@project) yield(@project) if block_given? validate_classification_label(@project, :external_authorization_classification_label) @@ -54,7 +53,7 @@ module Projects @project.creator = current_user - save_project_and_import_data(import_data) + save_project_and_import_data after_create_actions if @project.persisted? @@ -129,9 +128,9 @@ module Projects !@project.feature_available?(:wiki, current_user) || @skip_wiki end - def save_project_and_import_data(import_data) + def save_project_and_import_data Project.transaction do - @project.create_or_update_import_data(data: import_data[:data], credentials: import_data[:credentials]) if import_data + @project.create_or_update_import_data(data: @import_data[:data], credentials: @import_data[:credentials]) if @import_data if @project.save unless @project.gitlab_project_import? @@ -192,5 +191,11 @@ module Projects fail(error: @project.errors.full_messages.join(', ')) end end + + def project_visibility + @project_visibility ||= Gitlab::VisibilityLevelChecker + .new(current_user, @project, project_params: { import_data: @import_data }) + .level_restricted? + end end end diff --git a/app/services/todo_service.rb b/app/services/todo_service.rb index 0ea230a44a1..b1256df35d6 100644 --- a/app/services/todo_service.rb +++ b/app/services/todo_service.rb @@ -314,11 +314,9 @@ class TodoService end def reject_users_without_access(users, parent, target) - if target.is_a?(Note) && target.for_issuable? - target = target.noteable - end + target = target.noteable if target.is_a?(Note) - if target.is_a?(Issuable) + if target.respond_to?(:to_ability_name) select_users(users, :"read_#{target.to_ability_name}", target) else select_users(users, :read_project, parent) diff --git a/app/uploaders/personal_file_uploader.rb b/app/uploaders/personal_file_uploader.rb index 1ac69601d18..3efdd0aa1d9 100644 --- a/app/uploaders/personal_file_uploader.rb +++ b/app/uploaders/personal_file_uploader.rb @@ -6,6 +6,10 @@ class PersonalFileUploader < FileUploader options.storage_path end + def self.workhorse_local_upload_path + File.join(options.storage_path, 'uploads', TMP_UPLOAD_PATH) + end + def self.base_dir(model, _store = nil) # base_dir is the path seen by the user when rendering Markdown, so # it should be the same for both local and object storage. It is diff --git a/app/views/admin/application_settings/_spam.html.haml b/app/views/admin/application_settings/_spam.html.haml index d24e46b2815..f0a19075115 100644 --- a/app/views/admin/application_settings/_spam.html.haml +++ b/app/views/admin/application_settings/_spam.html.haml @@ -7,11 +7,15 @@ = f.check_box :recaptcha_enabled, class: 'form-check-input' = f.label :recaptcha_enabled, class: 'form-check-label' do Enable reCAPTCHA - - recaptcha_v2_link_url = 'https://developers.google.com/recaptcha/docs/versions' - - recaptcha_v2_link_start = '<a href="%{url}" target="_blank" rel="noopener noreferrer">'.html_safe % { url: recaptcha_v2_link_url } %span.form-text.text-muted#recaptcha_help_block - = _('Helps prevent bots from creating accounts. We currently only support %{recaptcha_v2_link_start}reCAPTCHA v2%{recaptcha_v2_link_end}').html_safe % { recaptcha_v2_link_start: recaptcha_v2_link_start, recaptcha_v2_link_end: '</a>'.html_safe } - + = _('Helps prevent bots from creating accounts.') + .form-group + .form-check + = f.check_box :login_recaptcha_protection_enabled, class: 'form-check-input' + = f.label :login_recaptcha_protection_enabled, class: 'form-check-label' do + Enable reCAPTCHA for login + %span.form-text.text-muted#recaptcha_help_block + = _('Helps prevent bots from brute-force attacks.') .form-group = f.label :recaptcha_site_key, 'reCAPTCHA Site Key', class: 'label-bold' = f.text_field :recaptcha_site_key, class: 'form-control' @@ -21,6 +25,7 @@ .form-group = f.label :recaptcha_private_key, 'reCAPTCHA Private Key', class: 'label-bold' + .form-group = f.text_field :recaptcha_private_key, class: 'form-control' .form-group diff --git a/app/views/admin/application_settings/reporting.html.haml b/app/views/admin/application_settings/reporting.html.haml index 46e3d1c4570..c60e44b3864 100644 --- a/app/views/admin/application_settings/reporting.html.haml +++ b/app/views/admin/application_settings/reporting.html.haml @@ -9,7 +9,9 @@ %button.btn.btn-default.js-settings-toggle{ type: 'button' } = expanded_by_default? ? _('Collapse') : _('Expand') %p - = _('Enable reCAPTCHA or Akismet and set IP limits.') + - recaptcha_v2_link_url = 'https://developers.google.com/recaptcha/docs/versions' + - recaptcha_v2_link_start = '<a href="%{url}" target="_blank" rel="noopener noreferrer">'.html_safe % { url: recaptcha_v2_link_url } + = _('Enable reCAPTCHA or Akismet and set IP limits. For reCAPTCHA, we currently only support %{recaptcha_v2_link_start}v2%{recaptcha_v2_link_end}').html_safe % { recaptcha_v2_link_start: recaptcha_v2_link_start, recaptcha_v2_link_end: '</a>'.html_safe } .settings-content = render 'spam' diff --git a/app/views/devise/sessions/_new_base.html.haml b/app/views/devise/sessions/_new_base.html.haml index 2f10f08c839..6382849de9c 100644 --- a/app/views/devise/sessions/_new_base.html.haml +++ b/app/views/devise/sessions/_new_base.html.haml @@ -13,7 +13,7 @@ .float-right.forgot-password = link_to "Forgot your password?", new_password_path(:user) %div - - if captcha_enabled? + - if captcha_enabled? || captcha_on_login_required? = recaptcha_tags .submit-container.move-submit-down diff --git a/changelogs/unreleased/ce-60465-prevent-comments-on-private-mrs.yml b/changelogs/unreleased/ce-60465-prevent-comments-on-private-mrs.yml new file mode 100644 index 00000000000..ba970162447 --- /dev/null +++ b/changelogs/unreleased/ce-60465-prevent-comments-on-private-mrs.yml @@ -0,0 +1,3 @@ +--- +title: Ensure only authorised users can create notes on Merge Requests and Issues +type: security diff --git a/changelogs/unreleased/security-59549-add-capcha-for-failed-logins.yml b/changelogs/unreleased/security-59549-add-capcha-for-failed-logins.yml new file mode 100644 index 00000000000..55f9e36c39c --- /dev/null +++ b/changelogs/unreleased/security-59549-add-capcha-for-failed-logins.yml @@ -0,0 +1,5 @@ +--- +title: Add :login_recaptcha_protection_enabled setting to prevent bots from brute-force attacks. +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-61974-limit-issue-comment-size-2.yml b/changelogs/unreleased/security-61974-limit-issue-comment-size-2.yml new file mode 100644 index 00000000000..962171dc6f8 --- /dev/null +++ b/changelogs/unreleased/security-61974-limit-issue-comment-size-2.yml @@ -0,0 +1,5 @@ +--- +title: Speed up regexp in namespace format by failing fast after reaching maximum namespace depth +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-61974-limit-issue-comment-size.yml b/changelogs/unreleased/security-61974-limit-issue-comment-size.yml new file mode 100644 index 00000000000..6d5ef057d83 --- /dev/null +++ b/changelogs/unreleased/security-61974-limit-issue-comment-size.yml @@ -0,0 +1,5 @@ +--- +title: Limit the size of issuable description and comments +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-64711-fix-commit-todos.yml b/changelogs/unreleased/security-64711-fix-commit-todos.yml new file mode 100644 index 00000000000..ce4b3cdeeaf --- /dev/null +++ b/changelogs/unreleased/security-64711-fix-commit-todos.yml @@ -0,0 +1,5 @@ +--- +title: Send TODOs for comments on commits correctly +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-ci-metrics-permissions.yml b/changelogs/unreleased/security-ci-metrics-permissions.yml new file mode 100644 index 00000000000..51c6493442a --- /dev/null +++ b/changelogs/unreleased/security-ci-metrics-permissions.yml @@ -0,0 +1,6 @@ +--- +title: Restrict MergeRequests#test_reports to authenticated users with read-access + on Builds +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-enable-image-proxy.yml b/changelogs/unreleased/security-enable-image-proxy.yml new file mode 100644 index 00000000000..88b49ffd9e8 --- /dev/null +++ b/changelogs/unreleased/security-enable-image-proxy.yml @@ -0,0 +1,5 @@ +--- +title: Added image proxy to mitigate potential stealing of IP addresses +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-epic-notes-api-reveals-historical-info-ce-master.yml b/changelogs/unreleased/security-epic-notes-api-reveals-historical-info-ce-master.yml new file mode 100644 index 00000000000..c639098721e --- /dev/null +++ b/changelogs/unreleased/security-epic-notes-api-reveals-historical-info-ce-master.yml @@ -0,0 +1,5 @@ +--- +title: Filter out old system notes for epics in notes api endpoint response +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-fix-html-injection-for-label-description-ce-master.yml b/changelogs/unreleased/security-fix-html-injection-for-label-description-ce-master.yml new file mode 100644 index 00000000000..07124ac399b --- /dev/null +++ b/changelogs/unreleased/security-fix-html-injection-for-label-description-ce-master.yml @@ -0,0 +1,5 @@ +--- +title: Fix HTML injection for label description +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-fix-markdown-xss.yml b/changelogs/unreleased/security-fix-markdown-xss.yml new file mode 100644 index 00000000000..7ef19f13fd5 --- /dev/null +++ b/changelogs/unreleased/security-fix-markdown-xss.yml @@ -0,0 +1,5 @@ +--- +title: Make sure HTML text is always escaped when replacing label/milestone references. +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-fix_jira_ssrf_vulnerability.yml b/changelogs/unreleased/security-fix_jira_ssrf_vulnerability.yml new file mode 100644 index 00000000000..25518dd2d05 --- /dev/null +++ b/changelogs/unreleased/security-fix_jira_ssrf_vulnerability.yml @@ -0,0 +1,5 @@ +--- +title: Prevent DNS rebind on JIRA service integration +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-group-runners-permissions.yml b/changelogs/unreleased/security-group-runners-permissions.yml new file mode 100644 index 00000000000..6c74be30b6d --- /dev/null +++ b/changelogs/unreleased/security-group-runners-permissions.yml @@ -0,0 +1,5 @@ +--- +title: Use admin_group authorization in Groups::RunnersController +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-id-filter-timeline-activities-for-guests.yml b/changelogs/unreleased/security-id-filter-timeline-activities-for-guests.yml new file mode 100644 index 00000000000..0fa5f89e2c0 --- /dev/null +++ b/changelogs/unreleased/security-id-filter-timeline-activities-for-guests.yml @@ -0,0 +1,5 @@ +--- +title: Show cross-referenced MR-id in issues' activities only to authorized users +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-katex-dos-12-1.yml b/changelogs/unreleased/security-katex-dos-12-1.yml new file mode 100644 index 00000000000..df803a5eafd --- /dev/null +++ b/changelogs/unreleased/security-katex-dos-12-1.yml @@ -0,0 +1,5 @@ +--- +title: Enforce max chars and max render time in markdown math +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-mr-head-pipeline-leak.yml b/changelogs/unreleased/security-mr-head-pipeline-leak.yml new file mode 100644 index 00000000000..b15b353ff41 --- /dev/null +++ b/changelogs/unreleased/security-mr-head-pipeline-leak.yml @@ -0,0 +1,5 @@ +--- +title: Check permissions before responding in MergeController#pipeline_status +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-personal-snippets.yml b/changelogs/unreleased/security-personal-snippets.yml new file mode 100644 index 00000000000..95f61993b98 --- /dev/null +++ b/changelogs/unreleased/security-personal-snippets.yml @@ -0,0 +1,5 @@ +--- +title: Remove EXIF from users/personal snippet uploads. +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-project-import-bypass.yml b/changelogs/unreleased/security-project-import-bypass.yml new file mode 100644 index 00000000000..fc7b823509c --- /dev/null +++ b/changelogs/unreleased/security-project-import-bypass.yml @@ -0,0 +1,5 @@ +--- +title: Fix project import restricted visibility bypass via API +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-sarcila-fix-weak-session-management.yml b/changelogs/unreleased/security-sarcila-fix-weak-session-management.yml new file mode 100644 index 00000000000..a37a3099519 --- /dev/null +++ b/changelogs/unreleased/security-sarcila-fix-weak-session-management.yml @@ -0,0 +1,6 @@ +--- +title: Fix weak session management by clearing password reset tokens after login (username/email) + are updated +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-ssrf-kubernetes-dns.yml b/changelogs/unreleased/security-ssrf-kubernetes-dns.yml new file mode 100644 index 00000000000..4d6335e4b08 --- /dev/null +++ b/changelogs/unreleased/security-ssrf-kubernetes-dns.yml @@ -0,0 +1,5 @@ +--- +title: Fix SSRF via DNS rebinding in Kubernetes Integration +merge_request: +author: +type: security diff --git a/config/initializers/asset_proxy_settings.rb b/config/initializers/asset_proxy_settings.rb new file mode 100644 index 00000000000..92247aba1b8 --- /dev/null +++ b/config/initializers/asset_proxy_settings.rb @@ -0,0 +1,6 @@ +# +# Asset proxy settings +# +ActiveSupport.on_load(:active_record) do + Banzai::Filter::AssetProxyFilter.initialize_settings +end diff --git a/config/initializers/fill_shards.rb b/config/initializers/fill_shards.rb index 18e067c8854..cad662e12f3 100644 --- a/config/initializers/fill_shards.rb +++ b/config/initializers/fill_shards.rb @@ -1,3 +1,5 @@ -if Shard.connected? && !Gitlab::Database.read_only? +# The `table_exists?` check is needed because during our migration rollback testing, +# `Shard.connected?` could be cached and return true even though the table doesn't exist +if Shard.connected? && Shard.table_exists? && !Gitlab::Database.read_only? Shard.populate! end diff --git a/config/initializers/rest-client-hostname_override.rb b/config/initializers/rest-client-hostname_override.rb new file mode 100644 index 00000000000..bc1b70bd73f --- /dev/null +++ b/config/initializers/rest-client-hostname_override.rb @@ -0,0 +1,49 @@ +# frozen_string_literal: true + +module RestClient + class Request + attr_accessor :hostname_override + + module UrlBlocker + def transmit(uri, req, payload, &block) + begin + ip, hostname_override = Gitlab::UrlBlocker.validate!(uri, allow_local_network: allow_settings_local_requests?, + allow_localhost: allow_settings_local_requests?, + dns_rebind_protection: dns_rebind_protection?) + + self.hostname_override = hostname_override + rescue Gitlab::UrlBlocker::BlockedUrlError => e + raise ArgumentError, "URL '#{uri}' is blocked: #{e.message}" + end + + # Gitlab::UrlBlocker returns a Addressable::URI which we need to coerce + # to URI so that rest-client can use it to determine if it's a + # URI::HTTPS or not. It uses it to set `net.use_ssl` to true or not: + # + # https://github.com/rest-client/rest-client/blob/f450a0f086f1cd1049abbef2a2c66166a1a9ba71/lib/restclient/request.rb#L656 + ip_as_uri = URI.parse(ip) + super(ip_as_uri, req, payload, &block) + end + + def net_http_object(hostname, port) + super.tap do |http| + http.hostname_override = hostname_override if hostname_override + end + end + + private + + def dns_rebind_protection? + return false if Gitlab.http_proxy_env? + + Gitlab::CurrentSettings.dns_rebinding_protection_enabled? + end + + def allow_settings_local_requests? + Gitlab::CurrentSettings.allow_local_requests_from_hooks_and_services? + end + end + + prepend UrlBlocker + end +end diff --git a/config/initializers/warden.rb b/config/initializers/warden.rb index 1d2bb2bce0a..d8a4da8cdf9 100644 --- a/config/initializers/warden.rb +++ b/config/initializers/warden.rb @@ -19,6 +19,7 @@ Rails.application.configure do |config| Warden::Manager.after_authentication(scope: :user) do |user, auth, opts| ActiveSession.cleanup(user) + Gitlab::AnonymousSession.new(auth.request.remote_ip, session_id: auth.request.session.id).cleanup_session_per_ip_entries end Warden::Manager.after_set_user(scope: :user, only: :fetch) do |user, auth, opts| diff --git a/config/routes/uploads.rb b/config/routes/uploads.rb index 920f8454ce2..096ef146e07 100644 --- a/config/routes/uploads.rb +++ b/config/routes/uploads.rb @@ -30,6 +30,10 @@ scope path: :uploads do to: 'uploads#create', constraints: { model: /personal_snippet|user/, id: /\d+/ }, as: 'upload' + + post ':model/authorize', + to: 'uploads#authorize', + constraints: { model: /personal_snippet|user/ } end # Redirect old note attachments path to new uploads path. diff --git a/db/migrate/20190219201635_add_asset_proxy_settings.rb b/db/migrate/20190219201635_add_asset_proxy_settings.rb new file mode 100644 index 00000000000..beeb6400292 --- /dev/null +++ b/db/migrate/20190219201635_add_asset_proxy_settings.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +class AddAssetProxySettings < ActiveRecord::Migration[5.0] + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + def change + add_column :application_settings, :asset_proxy_enabled, :boolean, default: false, null: false + add_column :application_settings, :asset_proxy_url, :string + add_column :application_settings, :asset_proxy_whitelist, :text + add_column :application_settings, :encrypted_asset_proxy_secret_key, :text + add_column :application_settings, :encrypted_asset_proxy_secret_key_iv, :string + end +end diff --git a/db/migrate/20190719122333_add_login_recaptcha_protection_enabled_to_application_settings.rb b/db/migrate/20190719122333_add_login_recaptcha_protection_enabled_to_application_settings.rb new file mode 100644 index 00000000000..4561e1e8aa9 --- /dev/null +++ b/db/migrate/20190719122333_add_login_recaptcha_protection_enabled_to_application_settings.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddLoginRecaptchaProtectionEnabledToApplicationSettings < ActiveRecord::Migration[5.1] + DOWNTIME = false + + def change + add_column :application_settings, :login_recaptcha_protection_enabled, :boolean, default: false, null: false + end +end diff --git a/db/migrate/20190816151221_add_active_jobs_limit_to_plans.rb b/db/migrate/20190816151221_add_active_jobs_limit_to_plans.rb new file mode 100644 index 00000000000..951ff41f1a8 --- /dev/null +++ b/db/migrate/20190816151221_add_active_jobs_limit_to_plans.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +class AddActiveJobsLimitToPlans < ActiveRecord::Migration[5.2] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_column_with_default :plans, :active_jobs_limit, :integer, default: 0 + end + + def down + remove_column :plans, :active_jobs_limit + end +end diff --git a/db/schema.rb b/db/schema.rb index a5079d3a5bc..aaf04be52a2 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2019_07_15_114644) do +ActiveRecord::Schema.define(version: 2019_08_16_151221) do # These are extensions that must be enabled in order to support this database enable_extension "pg_trgm" @@ -228,6 +228,12 @@ ActiveRecord::Schema.define(version: 2019_07_15_114644) do t.boolean "lock_memberships_to_ldap", default: false, null: false t.boolean "time_tracking_limit_to_hours", default: false, null: false t.string "grafana_url", default: "/-/grafana", null: false + t.boolean "asset_proxy_enabled", default: false, null: false + t.string "asset_proxy_url" + t.text "asset_proxy_whitelist" + t.text "encrypted_asset_proxy_secret_key" + t.string "encrypted_asset_proxy_secret_key_iv" + t.boolean "login_recaptcha_protection_enabled", default: false, null: false t.index ["custom_project_templates_group_id"], name: "index_application_settings_on_custom_project_templates_group_id", using: :btree t.index ["file_template_project_id"], name: "index_application_settings_on_file_template_project_id", using: :btree t.index ["usage_stats_set_by_user_id"], name: "index_application_settings_on_usage_stats_set_by_user_id", using: :btree @@ -2397,6 +2403,7 @@ ActiveRecord::Schema.define(version: 2019_07_15_114644) do t.string "title" t.integer "active_pipelines_limit" t.integer "pipeline_size_limit" + t.integer "active_jobs_limit", default: 0 t.index ["name"], name: "index_plans_on_name", using: :btree end diff --git a/doc/administration/raketasks/uploads/sanitize.md b/doc/administration/raketasks/uploads/sanitize.md index ae5ccfb9e37..7574660d848 100644 --- a/doc/administration/raketasks/uploads/sanitize.md +++ b/doc/administration/raketasks/uploads/sanitize.md @@ -37,6 +37,8 @@ Parameter | Type | Description `stop_id` | integer | Only uploads with equal or smaller ID will be processed `dry_run` | boolean | Do not remove EXIF data, only check if EXIF data are present or not, default: true `sleep_time` | float | Pause for number of seconds after processing each image, default: 0.3 seconds +`uploader` | string | Run sanitization only for uploads of the given uploader (`FileUploader`, `PersonalFileUploader`, `NamespaceFileUploader`) +`since` | date | Run sanitization only for uploads newer than given date (e.g. `2019-05-01`) If you have too many uploads, you can speed up sanitization by setting `sleep_time` to a lower value or by running multiple rake tasks in parallel, diff --git a/doc/api/epics.md b/doc/api/epics.md index d05eb0a8804..98ffe31da57 100644 --- a/doc/api/epics.md +++ b/doc/api/epics.md @@ -161,7 +161,7 @@ POST /groups/:id/epics | `id` | integer/string | yes | The ID or [URL-encoded path of the group](README.md#namespaced-path-encoding) owned by the authenticated user | | `title` | string | yes | The title of the epic | | `labels` | string | no | The comma separated list of labels | -| `description` | string | no | The description of the epic | +| `description` | string | no | The description of the epic. Limited to 1 000 000 characters. | | `start_date_is_fixed` | boolean | no | Whether start date should be sourced from `start_date_fixed` or from milestones (since 11.3) | | `start_date_fixed` | string | no | The fixed start date of an epic (since 11.3) | | `due_date_is_fixed` | boolean | no | Whether due date should be sourced from `due_date_fixed` or from milestones (since 11.3) | @@ -225,7 +225,7 @@ PUT /groups/:id/epics/:epic_iid | `id` | integer/string | yes | The ID or [URL-encoded path of the group](README.md#namespaced-path-encoding) owned by the authenticated user | | `epic_iid` | integer/string | yes | The internal ID of the epic | | `title` | string | no | The title of an epic | -| `description` | string | no | The description of an epic | +| `description` | string | no | The description of an epic. Limited to 1 000 000 characters. | | `labels` | string | no | The comma separated list of labels | | `start_date_is_fixed` | boolean | no | Whether start date should be sourced from `start_date_fixed` or from milestones (since 11.3) | | `start_date_fixed` | string | no | The fixed start date of an epic (since 11.3) | diff --git a/doc/api/issues.md b/doc/api/issues.md index 96a547551f1..ef479bc9829 100644 --- a/doc/api/issues.md +++ b/doc/api/issues.md @@ -593,7 +593,7 @@ POST /projects/:id/issues | `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user | | `iid` | integer/string | no | The internal ID of the project's issue (requires admin or project owner rights) | | `title` | string | yes | The title of an issue | -| `description` | string | no | The description of an issue | +| `description` | string | no | The description of an issue. Limited to 1 000 000 characters. | | `confidential` | boolean | no | Set an issue to be confidential. Default is `false`. | | `assignee_ids` | integer array | no | The ID of a user to assign issue | | `milestone_id` | integer | no | The global ID of a milestone to assign issue | @@ -694,7 +694,7 @@ PUT /projects/:id/issues/:issue_iid | `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user | | `issue_iid` | integer | yes | The internal ID of a project's issue | | `title` | string | no | The title of an issue | -| `description` | string | no | The description of an issue | +| `description` | string | no | The description of an issue. Limited to 1 000 000 characters. | | `confidential` | boolean | no | Updates an issue to be confidential | | `assignee_ids` | integer array | no | The ID of the user(s) to assign the issue to. Set to `0` or provide an empty value to unassign all assignees. | | `milestone_id` | integer | no | The global ID of a milestone to assign the issue to. Set to `0` or provide an empty value to unassign a milestone.| diff --git a/doc/api/merge_requests.md b/doc/api/merge_requests.md index 1ade46efb1c..fd8216b0fbd 100644 --- a/doc/api/merge_requests.md +++ b/doc/api/merge_requests.md @@ -837,7 +837,7 @@ POST /projects/:id/merge_requests | `title` | string | yes | Title of MR | | `assignee_id` | integer | no | Assignee user ID | | `assignee_ids` | integer array | no | The ID of the user(s) to assign the MR to. Set to `0` or provide an empty value to unassign all assignees. | -| `description` | string | no | Description of MR | +| `description` | string | no | Description of MR. Limited to 1 000 000 characters. | | `target_project_id` | integer | no | The target project (numeric id) | | `labels` | string | no | Labels for MR as a comma-separated list | | `milestone_id` | integer | no | The global ID of a milestone | @@ -990,7 +990,7 @@ PUT /projects/:id/merge_requests/:merge_request_iid | `assignee_ids` | integer array | no | The ID of the user(s) to assign the MR to. Set to `0` or provide an empty value to unassign all assignees. | | `milestone_id` | integer | no | The global ID of a milestone to assign the merge request to. Set to `0` or provide an empty value to unassign a milestone.| | `labels` | string | no | Comma-separated label names for a merge request. Set to an empty string to unassign all labels. | -| `description` | string | no | Description of MR | +| `description` | string | no | Description of MR. Limited to 1 000 000 characters. | | `state_event` | string | no | New state (close/reopen) | | `remove_source_branch` | boolean | no | Flag indicating if a merge request should remove the source branch when merging | | `squash` | boolean | no | Squash commits into a single commit when merging | diff --git a/doc/api/notes.md b/doc/api/notes.md index acbf0334563..d7183df1387 100644 --- a/doc/api/notes.md +++ b/doc/api/notes.md @@ -113,7 +113,7 @@ Parameters: - `id` (required) - The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) - `issue_iid` (required) - The IID of an issue -- `body` (required) - The content of a note +- `body` (required) - The content of a note. Limited to 1 000 000 characters. - `created_at` (optional) - Date time string, ISO 8601 formatted, e.g. 2016-03-11T03:45:40Z (requires admin or project/group owner rights) ```bash @@ -133,7 +133,7 @@ Parameters: - `id` (required) - The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) - `issue_iid` (required) - The IID of an issue - `note_id` (required) - The ID of a note -- `body` (required) - The content of a note +- `body` (required) - The content of a note. Limited to 1 000 000 characters. ```bash curl --request PUT --header "PRIVATE-TOKEN: <your_access_token>" https://gitlab.example.com/api/v4/projects/5/issues/11/notes?body=note @@ -231,7 +231,7 @@ Parameters: - `id` (required) - The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) - `snippet_id` (required) - The ID of a snippet -- `body` (required) - The content of a note +- `body` (required) - The content of a note. Limited to 1 000 000 characters. - `created_at` (optional) - Date time string, ISO 8601 formatted, e.g. 2016-03-11T03:45:40Z ```bash @@ -251,7 +251,7 @@ Parameters: - `id` (required) - The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) - `snippet_id` (required) - The ID of a snippet - `note_id` (required) - The ID of a note -- `body` (required) - The content of a note +- `body` (required) - The content of a note. Limited to 1 000 000 characters. ```bash curl --request PUT --header "PRIVATE-TOKEN: <your_access_token>" https://gitlab.example.com/api/v4/projects/5/snippets/11/notes?body=note @@ -354,7 +354,7 @@ Parameters: - `id` (required) - The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) - `merge_request_iid` (required) - The IID of a merge request -- `body` (required) - The content of a note +- `body` (required) - The content of a note. Limited to 1 000 000 characters. - `created_at` (optional) - Date time string, ISO 8601 formatted, e.g. 2016-03-11T03:45:40Z ### Modify existing merge request note @@ -370,7 +370,7 @@ Parameters: - `id` (required) - The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) - `merge_request_iid` (required) - The IID of a merge request - `note_id` (required) - The ID of a note -- `body` (required) - The content of a note +- `body` (required) - The content of a note. Limited to 1 000 000 characters. ```bash curl --request PUT --header "PRIVATE-TOKEN: <your_access_token>" https://gitlab.example.com/api/v4/projects/5/merge_requests/11/notes?body=note @@ -472,7 +472,7 @@ Parameters: | --------- | -------------- | -------- | ----------- | | `id` | integer/string | yes | The ID or [URL-encoded path of the group](README.md#namespaced-path-encoding) | | `epic_id` | integer | yes | The ID of an epic | -| `body` | string | yes | The content of a note | +| `body` | string | yes | The content of a note. Limited to 1 000 000 characters. | ```bash curl --request POST --header "PRIVATE-TOKEN: <your_access_token>" https://gitlab.example.com/api/v4/projects/5/snippet/11/notes?body=note @@ -493,7 +493,7 @@ Parameters: | `id` | integer/string | yes | The ID or [URL-encoded path of the group](README.md#namespaced-path-encoding) | | `epic_id` | integer | yes | The ID of an epic | | `note_id` | integer | yes | The ID of a note | -| `body` | string | yes | The content of a note | +| `body` | string | yes | The content of a note. Limited to 1 000 000 characters. | ```bash curl --request PUT --header "PRIVATE-TOKEN: <your_access_token>" https://gitlab.example.com/api/v4/projects/5/snippet/11/notes?body=note diff --git a/doc/api/settings.md b/doc/api/settings.md index ff48cac1f47..458a08edf83 100644 --- a/doc/api/settings.md +++ b/doc/api/settings.md @@ -39,6 +39,7 @@ Example response: "session_expire_delay" : 10080, "home_page_url" : null, "default_snippet_visibility" : "private", + "outbound_local_requests_whitelist": [], "domain_whitelist" : [], "domain_blacklist_enabled" : false, "domain_blacklist" : [], @@ -63,7 +64,10 @@ Example response: "performance_bar_allowed_group_id": 42, "instance_statistics_visibility_private": false, "user_show_add_ssh_key_message": true, - "local_markdown_version": 0 + "local_markdown_version": 0, + "asset_proxy_enabled": true, + "asset_proxy_url": "https://assets.example.com", + "asset_proxy_whitelist": ["example.com", "*.example.com", "your-instance.com"] } ``` @@ -113,6 +117,7 @@ Example response: "default_project_visibility": "internal", "default_snippet_visibility": "private", "default_group_visibility": "private", + "outbound_local_requests_whitelist": [], "domain_whitelist": [], "domain_blacklist_enabled" : false, "domain_blacklist" : [], @@ -136,6 +141,9 @@ Example response: "user_show_add_ssh_key_message": true, "file_template_project_id": 1, "local_markdown_version": 0, + "asset_proxy_enabled": true, + "asset_proxy_url": "https://assets.example.com", + "asset_proxy_whitelist": ["example.com", "*.example.com", "your-instance.com"], "geo_node_allowed_ips": "0.0.0.0/0, ::/0" } ``` @@ -176,6 +184,10 @@ are listed in the descriptions of the relevant settings. | `akismet_enabled` | boolean | no | (**If enabled, requires:** `akismet_api_key`) Enable or disable akismet spam protection. | | `allow_group_owners_to_manage_ldap` | boolean | no | **(PREMIUM)** Set to `true` to allow group owners to manage LDAP | | `allow_local_requests_from_hooks_and_services` | boolean | no | Allow requests to the local network from hooks and services. | +| `asset_proxy_enabled` | boolean | no | (**If enabled, requires:** `asset_proxy_url`) Enable proxying of assets. GitLab restart is required to apply changes. | +| `asset_proxy_secret_key` | string | no | Shared secret with the asset proxy server. GitLab restart is required to apply changes. | +| `asset_proxy_url` | string | no | URL of the asset proxy server. GitLab restart is required to apply changes. | +| `asset_proxy_whitelist` | string or array of strings | no | Assets that match these domain(s) will NOT be proxied. Wildcards allowed. Your GitLab installation URL is automatically whitelisted. GitLab restart is required to apply changes. | | `authorized_keys_enabled` | boolean | no | By default, we write to the `authorized_keys` file to support Git over SSH without additional configuration. GitLab can be optimized to authenticate SSH keys via the database file. Only disable this if you have configured your OpenSSH server to use the AuthorizedKeysCommand. | | `auto_devops_domain` | string | no | Specify a domain to use by default for every project's Auto Review Apps and Auto Deploy stages. | | `auto_devops_enabled` | boolean | no | Enable Auto DevOps for projects by default. It will automatically build, test, and deploy applications based on a predefined CI/CD configuration. | @@ -193,6 +205,7 @@ are listed in the descriptions of the relevant settings. | `domain_blacklist` | array of strings | required by: `domain_blacklist_enabled` | Users with e-mail addresses that match these domain(s) will NOT be able to sign-up. Wildcards allowed. Use separate lines for multiple entries. Ex: `domain.com`, `*.domain.com`. | | `domain_blacklist_enabled` | boolean | no | (**If enabled, requires:** `domain_blacklist`) Allows blocking sign-ups from emails from specific domains. | | `domain_whitelist` | array of strings | no | Force people to use only corporate emails for sign-up. Default is `null`, meaning there is no restriction. | +| `outbound_local_requests_whitelist` | array of strings | no | Define a list of trusted domains or ip addresses to which local requests are allowed when local requests for hooks and services are disabled. | `dsa_key_restriction` | integer | no | The minimum allowed bit length of an uploaded DSA key. Default is `0` (no restriction). `-1` disables DSA keys. | | `ecdsa_key_restriction` | integer | no | The minimum allowed curve size (in bits) of an uploaded ECDSA key. Default is `0` (no restriction). `-1` disables ECDSA keys. | | `ed25519_key_restriction` | integer | no | The minimum allowed curve size (in bits) of an uploaded ED25519 key. Default is `0` (no restriction). `-1` disables ED25519 keys. | @@ -227,7 +240,7 @@ are listed in the descriptions of the relevant settings. | `gravatar_enabled` | boolean | no | Enable Gravatar. | | `hashed_storage_enabled` | boolean | no | Create new projects using hashed storage paths: Enable immutable, hash-based paths and repository names to store repositories on disk. This prevents repositories from having to be moved or renamed when the Project URL changes and may improve disk I/O performance. (EXPERIMENTAL) | | `help_page_hide_commercial_content` | boolean | no | Hide marketing-related entries from help. | -| `help_page_support_url` | string | no | Alternate support URL for help page. | +| `help_page_support_url` | string | no | Alternate support URL for help page and help dropdown. | | `help_page_text` | string | no | Custom text displayed on the help page. | | `help_text` | string | no | **(PREMIUM)** GitLab server administrator information | | `hide_third_party_offers` | boolean | no | Do not display offers from third parties within GitLab. | diff --git a/doc/security/README.md b/doc/security/README.md index c48d5bc2065..77c2e465ebe 100644 --- a/doc/security/README.md +++ b/doc/security/README.md @@ -17,3 +17,4 @@ type: index - [Enforce Two-factor authentication](two_factor_authentication.md) - [Send email confirmation on sign-up](user_email_confirmation.md) - [Security of running jobs](https://docs.gitlab.com/runner/security/) +- [Proxying images](asset_proxy.md) diff --git a/doc/security/asset_proxy.md b/doc/security/asset_proxy.md new file mode 100644 index 00000000000..48cf9f59a7d --- /dev/null +++ b/doc/security/asset_proxy.md @@ -0,0 +1,29 @@ +A possible security concern when managing a public facing GitLab instance is +the ability to steal a users IP address by referencing images in issues, comments, etc. + +For example, adding `` to +an issue description will cause the image to be loaded from the external +server in order to be displayed. However this also allows the external server +to log the IP address of the user. + +One way to mitigate this is by proxying any external images to a server you +control. GitLab handles this by allowing you to run the "Camo" server +[cactus/go-camo](https://github.com/cactus/go-camo#how-it-works). +The image request is sent to the Camo server, which then makes the request for +the original image. This way an attacker only ever seems the IP address +of your Camo server. + +Once you have your Camo server up and running, you can configure GitLab to +proxy image requests to it. The following settings are supported: + +| Attribute | Description | +| --------- | ----------- | +| `asset_proxy_enabled` | (**If enabled, requires:** `asset_proxy_url`) Enable proxying of assets. | +| `asset_proxy_secret_key` | Shared secret with the asset proxy server. | +| `asset_proxy_url` | URL of the asset proxy server. | +| `asset_proxy_whitelist` | Assets that match these domain(s) will NOT be proxied. Wildcards allowed. Your GitLab installation URL is automatically whitelisted. | + + +These can be set via the [Application setting API](../api/settings.md) + +Note that a GitLab restart is required to apply any changes. diff --git a/lib/api/discussions.rb b/lib/api/discussions.rb index cc62ce22a1b..3fb70024201 100644 --- a/lib/api/discussions.rb +++ b/lib/api/discussions.rb @@ -32,7 +32,7 @@ module API .includes(:noteable) .fresh - notes = notes.reject { |n| n.cross_reference_not_visible_for?(current_user) } + notes = notes.select { |n| n.visible_for?(current_user) } discussions = Kaminari.paginate_array(Discussion.build_collection(notes, noteable)) present paginate(discussions), with: Entities::Discussion @@ -233,7 +233,7 @@ module API .includes(:noteable) .fresh - notes.reject { |n| n.cross_reference_not_visible_for?(current_user) } + notes.select { |n| n.visible_for?(current_user) } end # rubocop: enable CodeReuse/ActiveRecord end diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 494da770279..2945b1054f0 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -1160,6 +1160,9 @@ module API attributes.delete(:performance_bar_allowed_group_path) attributes.delete(:performance_bar_enabled) + # let's not expose the secret key in a response + attributes.delete(:asset_proxy_secret_key) + attributes end diff --git a/lib/api/helpers/notes_helpers.rb b/lib/api/helpers/notes_helpers.rb index b03ac7deb71..d1de156376c 100644 --- a/lib/api/helpers/notes_helpers.rb +++ b/lib/api/helpers/notes_helpers.rb @@ -10,7 +10,7 @@ module API end def update_note(noteable, note_id) - note = noteable.notes.find(params[:note_id]) + note = noteable.notes.find(note_id) authorize! :admin_note, note @@ -59,8 +59,8 @@ module API end def get_note(noteable, note_id) - note = noteable.notes.with_metadata.find(params[:note_id]) - can_read_note = !note.cross_reference_not_visible_for?(current_user) + note = noteable.notes.with_metadata.find(note_id) + can_read_note = note.visible_for?(current_user) if can_read_note present note, with: Entities::Note @@ -81,6 +81,10 @@ module API noteable || not_found!(noteable_type) end + def reject_note?(noteable_type, noteable, parent_type, parent_id, note) + note.cross_reference_not_visible_for?(current_user) + end + def params_by_noteable_type_and_id(type, id) target_type = type.name.underscore { target_type: target_type }.tap do |h| diff --git a/lib/api/notes.rb b/lib/api/notes.rb index 9381f045144..eaf97bb119e 100644 --- a/lib/api/notes.rb +++ b/lib/api/notes.rb @@ -41,7 +41,7 @@ module API # mismatch between the pagination headers info and the actual notes # array returned, but this is really a edge-case. paginate(raw_notes) - .reject { |n| n.cross_reference_not_visible_for?(current_user) } + .select { |note| note.visible_for?(current_user) } present notes, with: Entities::Note end # rubocop: enable CodeReuse/ActiveRecord diff --git a/lib/api/settings.rb b/lib/api/settings.rb index 4275d911708..46de55e3222 100644 --- a/lib/api/settings.rb +++ b/lib/api/settings.rb @@ -36,6 +36,10 @@ module API given akismet_enabled: ->(val) { val } do requires :akismet_api_key, type: String, desc: 'Generate API key at http://www.akismet.com' end + optional :asset_proxy_enabled, type: Boolean, desc: 'Enable proxying of assets' + optional :asset_proxy_url, type: String, desc: 'URL of the asset proxy server' + optional :asset_proxy_secret_key, type: String, desc: 'Shared secret with the asset proxy server' + optional :asset_proxy_whitelist, type: Array[String], coerce_with: Validations::Types::CommaSeparatedToArray.coerce, desc: 'Assets that match these domain(s) will NOT be proxied. Wildcards allowed. Your GitLab installation URL is automatically whitelisted.' optional :container_registry_token_expire_delay, type: Integer, desc: 'Authorization token duration (minutes)' optional :default_artifacts_expire_in, type: String, desc: "Set the default expiration time for each job's artifacts" optional :default_project_creation, type: Integer, values: ::Gitlab::Access.project_creation_values, desc: 'Determine if developers can create projects in the group' @@ -59,7 +63,7 @@ module API optional :grafana_url, type: String, desc: 'Grafana URL' optional :gravatar_enabled, type: Boolean, desc: 'Flag indicating if the Gravatar service is enabled' optional :help_page_hide_commercial_content, type: Boolean, desc: 'Hide marketing-related entries from help' - optional :help_page_support_url, type: String, desc: 'Alternate support URL for help page' + optional :help_page_support_url, type: String, desc: 'Alternate support URL for help page and help dropdown' optional :help_page_text, type: String, desc: 'Custom text displayed on the help page' optional :home_page_url, type: String, desc: 'We will redirect non-logged in users to this page' optional :housekeeping_enabled, type: Boolean, desc: 'Enable automatic repository housekeeping (git repack, git gc)' @@ -104,6 +108,11 @@ module API requires :recaptcha_site_key, type: String, desc: 'Generate site key at http://www.google.com/recaptcha' requires :recaptcha_private_key, type: String, desc: 'Generate private key at http://www.google.com/recaptcha' end + optional :login_recaptcha_protection_enabled, type: Boolean, desc: 'Helps prevent brute-force attacks' + given login_recaptcha_protection_enabled: ->(val) { val } do + requires :recaptcha_site_key, type: String, desc: 'Generate site key at http://www.google.com/recaptcha' + requires :recaptcha_private_key, type: String, desc: 'Generate private key at http://www.google.com/recaptcha' + end optional :repository_checks_enabled, type: Boolean, desc: "GitLab will periodically run 'git fsck' in all project and wiki repositories to look for silent disk corruption issues." optional :repository_storages, type: Array[String], desc: 'Storage paths for new projects' optional :require_two_factor_authentication, type: Boolean, desc: 'Require all users to set up Two-factor authentication' @@ -123,7 +132,7 @@ module API optional :terminal_max_session_time, type: Integer, desc: 'Maximum time for web terminal websocket connection (in seconds). Set to 0 for unlimited time.' optional :usage_ping_enabled, type: Boolean, desc: 'Every week GitLab will report license usage back to GitLab, Inc.' optional :instance_statistics_visibility_private, type: Boolean, desc: 'When set to `true` Instance statistics will only be available to admins' - optional :local_markdown_version, type: Integer, desc: "Local markdown version, increase this value when any cached markdown should be invalidated" + optional :local_markdown_version, type: Integer, desc: 'Local markdown version, increase this value when any cached markdown should be invalidated' ApplicationSetting::SUPPORTED_KEY_TYPES.each do |type| optional :"#{type}_key_restriction", diff --git a/lib/api/validations/types/comma_separated_to_array.rb b/lib/api/validations/types/comma_separated_to_array.rb new file mode 100644 index 00000000000..b551878abd1 --- /dev/null +++ b/lib/api/validations/types/comma_separated_to_array.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +module API + module Validations + module Types + class CommaSeparatedToArray + def self.coerce + lambda do |value| + case value + when String + value.split(',').map(&:strip) + when Array + value.map { |v| v.to_s.split(',').map(&:strip) }.flatten + else + [] + end + end + end + end + end + end +end diff --git a/lib/banzai/filter/abstract_reference_filter.rb b/lib/banzai/filter/abstract_reference_filter.rb index 0224dd8fcd1..64b0a68b7dc 100644 --- a/lib/banzai/filter/abstract_reference_filter.rb +++ b/lib/banzai/filter/abstract_reference_filter.rb @@ -7,6 +7,14 @@ module Banzai class AbstractReferenceFilter < ReferenceFilter include CrossProjectReference + # REFERENCE_PLACEHOLDER is used for re-escaping HTML text except found + # reference (which we replace with placeholder during re-scaping). The + # random number helps ensure it's pretty close to unique. Since it's a + # transitory value (it never gets saved) we can initialize once, and it + # doesn't matter if it changes on a restart. + REFERENCE_PLACEHOLDER = "_reference_#{SecureRandom.hex(16)}_" + REFERENCE_PLACEHOLDER_PATTERN = %r{#{REFERENCE_PLACEHOLDER}(\d+)}.freeze + def self.object_class # Implement in child class # Example: MergeRequest @@ -371,6 +379,14 @@ module Banzai def escape_html_entities(text) CGI.escapeHTML(text.to_s) end + + def escape_with_placeholders(text, placeholder_data) + escaped = escape_html_entities(text) + + escaped.gsub(REFERENCE_PLACEHOLDER_PATTERN) do |match| + placeholder_data[$1.to_i] + end + end end end end diff --git a/lib/banzai/filter/asset_proxy_filter.rb b/lib/banzai/filter/asset_proxy_filter.rb new file mode 100644 index 00000000000..0a9a52a73a1 --- /dev/null +++ b/lib/banzai/filter/asset_proxy_filter.rb @@ -0,0 +1,62 @@ +# frozen_string_literal: true + +module Banzai + module Filter + # Proxy's images/assets to another server. Reduces mixed content warnings + # as well as hiding the customer's IP address when requesting images. + # Copies the original img `src` to `data-canonical-src` then replaces the + # `src` with a new url to the proxy server. + class AssetProxyFilter < HTML::Pipeline::CamoFilter + def initialize(text, context = nil, result = nil) + super + end + + def validate + needs(:asset_proxy, :asset_proxy_secret_key) if asset_proxy_enabled? + end + + def asset_host_whitelisted?(host) + context[:asset_proxy_domain_regexp] ? context[:asset_proxy_domain_regexp].match?(host) : false + end + + def self.transform_context(context) + context[:disable_asset_proxy] = !Gitlab.config.asset_proxy.enabled + + unless context[:disable_asset_proxy] + context[:asset_proxy_enabled] = !context[:disable_asset_proxy] + context[:asset_proxy] = Gitlab.config.asset_proxy.url + context[:asset_proxy_secret_key] = Gitlab.config.asset_proxy.secret_key + context[:asset_proxy_domain_regexp] = Gitlab.config.asset_proxy.domain_regexp + end + + context + end + + # called during an initializer. Caching the values in Gitlab.config significantly increased + # performance, rather than querying Gitlab::CurrentSettings.current_application_settings + # over and over. However, this does mean that the Rails servers need to get restarted + # whenever the application settings are changed + def self.initialize_settings + application_settings = Gitlab::CurrentSettings.current_application_settings + Gitlab.config['asset_proxy'] ||= Settingslogic.new({}) + + if application_settings.respond_to?(:asset_proxy_enabled) + Gitlab.config.asset_proxy['enabled'] = application_settings.asset_proxy_enabled + Gitlab.config.asset_proxy['url'] = application_settings.asset_proxy_url + Gitlab.config.asset_proxy['secret_key'] = application_settings.asset_proxy_secret_key + Gitlab.config.asset_proxy['whitelist'] = application_settings.asset_proxy_whitelist || [Gitlab.config.gitlab.host] + Gitlab.config.asset_proxy['domain_regexp'] = compile_whitelist(Gitlab.config.asset_proxy.whitelist) + else + Gitlab.config.asset_proxy['enabled'] = ::ApplicationSetting.defaults[:asset_proxy_enabled] + end + end + + def self.compile_whitelist(domain_list) + return if domain_list.empty? + + escaped = domain_list.map { |domain| Regexp.escape(domain).gsub('\*', '.*?') } + Regexp.new("^(#{escaped.join('|')})$", Regexp::IGNORECASE) + end + end + end +end diff --git a/lib/banzai/filter/external_link_filter.rb b/lib/banzai/filter/external_link_filter.rb index 61ee3eac216..fb721fe12b1 100644 --- a/lib/banzai/filter/external_link_filter.rb +++ b/lib/banzai/filter/external_link_filter.rb @@ -14,10 +14,10 @@ module Banzai # such as on `mailto:` links. Since we've been using it, do an # initial parse for validity and then use Addressable # for IDN support, etc - uri = uri_strict(node['href'].to_s) + uri = uri_strict(node_src(node)) if uri - node.set_attribute('href', uri.to_s) - addressable_uri = addressable_uri(node['href']) + node.set_attribute(node_src_attribute(node), uri.to_s) + addressable_uri = addressable_uri(node_src(node)) else addressable_uri = nil end @@ -35,6 +35,16 @@ module Banzai private + # if this is a link to a proxied image, then `src` is already the correct + # proxied url, so work with the `data-canonical-src` + def node_src_attribute(node) + node['data-canonical-src'] ? 'data-canonical-src' : 'href' + end + + def node_src(node) + node[node_src_attribute(node)] + end + def uri_strict(href) URI.parse(href) rescue URI::Error @@ -72,7 +82,7 @@ module Banzai return unless uri return unless context[:emailable_links] - unencoded_uri_str = Addressable::URI.unencode(node['href']) + unencoded_uri_str = Addressable::URI.unencode(node_src(node)) if unencoded_uri_str == node.content && idn?(uri) node.content = uri.normalize diff --git a/lib/banzai/filter/image_link_filter.rb b/lib/banzai/filter/image_link_filter.rb index 01237303c27..ed0a01e6277 100644 --- a/lib/banzai/filter/image_link_filter.rb +++ b/lib/banzai/filter/image_link_filter.rb @@ -18,6 +18,9 @@ module Banzai rel: 'noopener noreferrer' ) + # make sure the original non-proxied src carries over to the link + link['data-canonical-src'] = img['data-canonical-src'] if img['data-canonical-src'] + link.children = img.clone img.replace(link) diff --git a/lib/banzai/filter/label_reference_filter.rb b/lib/banzai/filter/label_reference_filter.rb index 4892668fc22..a0789b7ca06 100644 --- a/lib/banzai/filter/label_reference_filter.rb +++ b/lib/banzai/filter/label_reference_filter.rb @@ -14,24 +14,24 @@ module Banzai find_labels(parent_object).find(id) end - def self.references_in(text, pattern = Label.reference_pattern) - unescape_html_entities(text).gsub(pattern) do |match| - yield match, $~[:label_id].to_i, $~[:label_name], $~[:project], $~[:namespace], $~ - end - end - def references_in(text, pattern = Label.reference_pattern) - unescape_html_entities(text).gsub(pattern) do |match| + labels = {} + unescaped_html = unescape_html_entities(text).gsub(pattern) do |match| namespace, project = $~[:namespace], $~[:project] project_path = full_project_path(namespace, project) label = find_label(project_path, $~[:label_id], $~[:label_name]) if label - yield match, label.id, project, namespace, $~ + labels[label.id] = yield match, label.id, project, namespace, $~ + "#{REFERENCE_PLACEHOLDER}#{label.id}" else - escape_html_entities(match) + match end end + + return text if labels.empty? + + escape_with_placeholders(unescaped_html, labels) end def find_label(parent_ref, label_id, label_name) diff --git a/lib/banzai/filter/milestone_reference_filter.rb b/lib/banzai/filter/milestone_reference_filter.rb index 08969753d75..4c47ee4dba1 100644 --- a/lib/banzai/filter/milestone_reference_filter.rb +++ b/lib/banzai/filter/milestone_reference_filter.rb @@ -51,15 +51,21 @@ module Banzai # default implementation. return super(text, pattern) if pattern != Milestone.reference_pattern - unescape_html_entities(text).gsub(pattern) do |match| + milestones = {} + unescaped_html = unescape_html_entities(text).gsub(pattern) do |match| milestone = find_milestone($~[:project], $~[:namespace], $~[:milestone_iid], $~[:milestone_name]) if milestone - yield match, milestone.id, $~[:project], $~[:namespace], $~ + milestones[milestone.id] = yield match, milestone.id, $~[:project], $~[:namespace], $~ + "#{REFERENCE_PLACEHOLDER}#{milestone.id}" else - escape_html_entities(match) + match end end + + return text if milestones.empty? + + escape_with_placeholders(unescaped_html, milestones) end def find_milestone(project_ref, namespace_ref, milestone_id, milestone_name) diff --git a/lib/banzai/filter/video_link_filter.rb b/lib/banzai/filter/video_link_filter.rb index 0fff104cf91..a278fcfdb47 100644 --- a/lib/banzai/filter/video_link_filter.rb +++ b/lib/banzai/filter/video_link_filter.rb @@ -23,6 +23,14 @@ module Banzai "'.#{ext}' = substring(@src, string-length(@src) - #{ext.size})" end + if context[:asset_proxy_enabled].present? + src_query.concat( + UploaderHelper::VIDEO_EXT.map do |ext| + "'.#{ext}' = substring(@data-canonical-src, string-length(@data-canonical-src) - #{ext.size})" + end + ) + end + "descendant-or-self::img[not(ancestor::a) and (#{src_query.join(' or ')})]" end end @@ -48,6 +56,13 @@ module Banzai target: '_blank', rel: 'noopener noreferrer', title: "Download '#{element['title'] || element['alt']}'") + + # make sure the original non-proxied src carries over + if element['data-canonical-src'] + video['data-canonical-src'] = element['data-canonical-src'] + link['data-canonical-src'] = element['data-canonical-src'] + end + download_paragraph = doc.document.create_element('p') download_paragraph.children = link diff --git a/lib/banzai/pipeline/ascii_doc_pipeline.rb b/lib/banzai/pipeline/ascii_doc_pipeline.rb index d25b74b23b2..82b99d3de4a 100644 --- a/lib/banzai/pipeline/ascii_doc_pipeline.rb +++ b/lib/banzai/pipeline/ascii_doc_pipeline.rb @@ -6,12 +6,17 @@ module Banzai def self.filters FilterArray[ Filter::AsciiDocSanitizationFilter, + Filter::AssetProxyFilter, Filter::SyntaxHighlightFilter, Filter::ExternalLinkFilter, Filter::PlantumlFilter, Filter::AsciiDocPostProcessingFilter ] end + + def self.transform_context(context) + Filter::AssetProxyFilter.transform_context(context) + end end end end diff --git a/lib/banzai/pipeline/gfm_pipeline.rb b/lib/banzai/pipeline/gfm_pipeline.rb index 2c1006f708a..f419e54c264 100644 --- a/lib/banzai/pipeline/gfm_pipeline.rb +++ b/lib/banzai/pipeline/gfm_pipeline.rb @@ -17,6 +17,7 @@ module Banzai Filter::SpacedLinkFilter, Filter::SanitizationFilter, + Filter::AssetProxyFilter, Filter::SyntaxHighlightFilter, Filter::MathFilter, @@ -60,7 +61,7 @@ module Banzai def self.transform_context(context) context[:only_path] = true unless context.key?(:only_path) - context + Filter::AssetProxyFilter.transform_context(context) end end end diff --git a/lib/banzai/pipeline/markup_pipeline.rb b/lib/banzai/pipeline/markup_pipeline.rb index ceba082cd4f..c86d5f08ded 100644 --- a/lib/banzai/pipeline/markup_pipeline.rb +++ b/lib/banzai/pipeline/markup_pipeline.rb @@ -6,11 +6,16 @@ module Banzai def self.filters @filters ||= FilterArray[ Filter::SanitizationFilter, + Filter::AssetProxyFilter, Filter::ExternalLinkFilter, Filter::PlantumlFilter, Filter::SyntaxHighlightFilter ] end + + def self.transform_context(context) + Filter::AssetProxyFilter.transform_context(context) + end end end end diff --git a/lib/banzai/pipeline/single_line_pipeline.rb b/lib/banzai/pipeline/single_line_pipeline.rb index 72374207a8f..9aff6880f56 100644 --- a/lib/banzai/pipeline/single_line_pipeline.rb +++ b/lib/banzai/pipeline/single_line_pipeline.rb @@ -7,6 +7,7 @@ module Banzai @filters ||= FilterArray[ Filter::HtmlEntityFilter, Filter::SanitizationFilter, + Filter::AssetProxyFilter, Filter::EmojiFilter, Filter::AutolinkFilter, @@ -29,6 +30,8 @@ module Banzai end def self.transform_context(context) + context = Filter::AssetProxyFilter.transform_context(context) + super(context).merge( no_sourcepos: true ) diff --git a/lib/gitlab/anonymous_session.rb b/lib/gitlab/anonymous_session.rb new file mode 100644 index 00000000000..148b6d3310d --- /dev/null +++ b/lib/gitlab/anonymous_session.rb @@ -0,0 +1,39 @@ +# frozen_string_literal: true + +module Gitlab + class AnonymousSession + def initialize(remote_ip, session_id: nil) + @remote_ip = remote_ip + @session_id = session_id + end + + def store_session_id_per_ip + Gitlab::Redis::SharedState.with do |redis| + redis.pipelined do + redis.sadd(session_lookup_name, session_id) + redis.expire(session_lookup_name, 24.hours) + end + end + end + + def stored_sessions + Gitlab::Redis::SharedState.with do |redis| + redis.scard(session_lookup_name) + end + end + + def cleanup_session_per_ip_entries + Gitlab::Redis::SharedState.with do |redis| + redis.srem(session_lookup_name, session_id) + end + end + + private + + attr_reader :remote_ip, :session_id + + def session_lookup_name + @session_lookup_name ||= "#{Gitlab::Redis::SharedState::IP_SESSIONS_LOOKUP_NAMESPACE}:#{remote_ip}" + end + end +end diff --git a/lib/gitlab/ci/pipeline/chain/limit/job_activity.rb b/lib/gitlab/ci/pipeline/chain/limit/job_activity.rb new file mode 100644 index 00000000000..31c218bf954 --- /dev/null +++ b/lib/gitlab/ci/pipeline/chain/limit/job_activity.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +module Gitlab + module Ci + module Pipeline + module Chain + module Limit + class JobActivity < Chain::Base + def perform! + # to be overridden in EE + end + + def break? + false # to be overridden in EE + end + end + end + end + end + end +end diff --git a/lib/gitlab/database.rb b/lib/gitlab/database.rb index 3e4c720b49a..0cb79e4efeb 100644 --- a/lib/gitlab/database.rb +++ b/lib/gitlab/database.rb @@ -13,6 +13,10 @@ module Gitlab # https://dev.mysql.com/doc/refman/5.7/en/datetime.html MAX_TIMESTAMP_VALUE = Time.at((1 << 31) - 1).freeze + # The maximum number of characters for text fields, to avoid DoS attacks via parsing huge text fields + # https://gitlab.com/gitlab-org/gitlab-ce/issues/61974 + MAX_TEXT_SIZE_LIMIT = 1_000_000 + # Minimum schema version from which migrations are supported # Migrations before this version may have been removed MIN_SCHEMA_VERSION = 20190506135400 diff --git a/lib/gitlab/jira/http_client.rb b/lib/gitlab/jira/http_client.rb new file mode 100644 index 00000000000..11a33a7b358 --- /dev/null +++ b/lib/gitlab/jira/http_client.rb @@ -0,0 +1,66 @@ +# frozen_string_literal: true + +module Gitlab + module Jira + # Gitlab JIRA HTTP client to be used with jira-ruby gem, this subclasses JIRA::HTTPClient. + # Uses Gitlab::HTTP to make requests to JIRA REST API. + # The parent class implementation can be found at: https://github.com/sumoheavy/jira-ruby/blob/v1.4.0/lib/jira/http_client.rb + class HttpClient < JIRA::HttpClient + extend ::Gitlab::Utils::Override + + override :request + def request(*args) + result = make_request(*args) + + raise JIRA::HTTPError.new(result) unless result.response.is_a?(Net::HTTPSuccess) + + result + end + + override :make_cookie_auth_request + def make_cookie_auth_request + body = { + username: @options.delete(:username), + password: @options.delete(:password) + }.to_json + + make_request(:post, @options[:context_path] + '/rest/auth/1/session', body, { 'Content-Type' => 'application/json' }) + end + + override :make_request + def make_request(http_method, path, body = '', headers = {}) + request_params = { headers: headers } + request_params[:body] = body if body.present? + request_params[:headers][:Cookie] = get_cookies if options[:use_cookies] + request_params[:timeout] = options[:read_timeout] if options[:read_timeout] + request_params[:base_uri] = uri.to_s + request_params.merge!(auth_params) + + result = Gitlab::HTTP.public_send(http_method, path, **request_params) # rubocop:disable GitlabSecurity/PublicSend + @authenticated = result.response.is_a?(Net::HTTPOK) + store_cookies(result) if options[:use_cookies] + + result + end + + def auth_params + return {} unless @options[:username] && @options[:password] + + { + basic_auth: { + username: @options[:username], + password: @options[:password] + } + } + end + + private + + def get_cookies + cookie_array = @cookies.values.map { |cookie| "#{cookie.name}=#{cookie.value[0]}" } + cookie_array += Array(@options[:additional_cookies]) if @options.key?(:additional_cookies) + cookie_array.join('; ') if cookie_array.any? + end + end + end +end diff --git a/lib/gitlab/markdown_cache.rb b/lib/gitlab/markdown_cache.rb index 0354c710a3f..03a2f62cbd9 100644 --- a/lib/gitlab/markdown_cache.rb +++ b/lib/gitlab/markdown_cache.rb @@ -3,8 +3,8 @@ module Gitlab module MarkdownCache # Increment this number every time the renderer changes its output + CACHE_COMMONMARK_VERSION = 17 CACHE_COMMONMARK_VERSION_START = 10 - CACHE_COMMONMARK_VERSION = 16 BaseError = Class.new(StandardError) UnsupportedClassError = Class.new(BaseError) diff --git a/lib/gitlab/path_regex.rb b/lib/gitlab/path_regex.rb index a13b3f9e069..98a565973c5 100644 --- a/lib/gitlab/path_regex.rb +++ b/lib/gitlab/path_regex.rb @@ -132,7 +132,7 @@ module Gitlab NO_SUFFIX_REGEX = /(?<!\.git|\.atom)/.freeze NAMESPACE_FORMAT_REGEX = /(?:#{NAMESPACE_FORMAT_REGEX_JS})#{NO_SUFFIX_REGEX}/.freeze PROJECT_PATH_FORMAT_REGEX = /(?:#{PATH_REGEX_STR})#{NO_SUFFIX_REGEX}/.freeze - FULL_NAMESPACE_FORMAT_REGEX = %r{(#{NAMESPACE_FORMAT_REGEX}/)*#{NAMESPACE_FORMAT_REGEX}}.freeze + FULL_NAMESPACE_FORMAT_REGEX = %r{(#{NAMESPACE_FORMAT_REGEX}/){,#{Namespace::NUMBER_OF_ANCESTORS_ALLOWED}}#{NAMESPACE_FORMAT_REGEX}}.freeze def root_namespace_route_regex @root_namespace_route_regex ||= begin diff --git a/lib/gitlab/recaptcha.rb b/lib/gitlab/recaptcha.rb index 772d743c9b0..f3cbe1db901 100644 --- a/lib/gitlab/recaptcha.rb +++ b/lib/gitlab/recaptcha.rb @@ -3,7 +3,7 @@ module Gitlab module Recaptcha def self.load_configurations! - if Gitlab::CurrentSettings.recaptcha_enabled + if Gitlab::CurrentSettings.recaptcha_enabled || enabled_on_login? ::Recaptcha.configure do |config| config.site_key = Gitlab::CurrentSettings.recaptcha_site_key config.secret_key = Gitlab::CurrentSettings.recaptcha_private_key @@ -16,5 +16,9 @@ module Gitlab def self.enabled? Gitlab::CurrentSettings.recaptcha_enabled end + + def self.enabled_on_login? + Gitlab::CurrentSettings.login_recaptcha_protection_enabled + end end end diff --git a/lib/gitlab/redis/shared_state.rb b/lib/gitlab/redis/shared_state.rb index 9066606ca21..270a19e780c 100644 --- a/lib/gitlab/redis/shared_state.rb +++ b/lib/gitlab/redis/shared_state.rb @@ -9,6 +9,7 @@ module Gitlab SESSION_NAMESPACE = 'session:gitlab'.freeze USER_SESSIONS_NAMESPACE = 'session:user:gitlab'.freeze USER_SESSIONS_LOOKUP_NAMESPACE = 'session:lookup:user:gitlab'.freeze + IP_SESSIONS_LOOKUP_NAMESPACE = 'session:lookup:ip:gitlab'.freeze DEFAULT_REDIS_SHARED_STATE_URL = 'redis://localhost:6382'.freeze REDIS_SHARED_STATE_CONFIG_ENV_VAR_NAME = 'GITLAB_REDIS_SHARED_STATE_CONFIG_FILE'.freeze diff --git a/lib/gitlab/sanitizers/exif.rb b/lib/gitlab/sanitizers/exif.rb index bb4e4ce7bbc..2f3d14ecebd 100644 --- a/lib/gitlab/sanitizers/exif.rb +++ b/lib/gitlab/sanitizers/exif.rb @@ -53,15 +53,18 @@ module Gitlab end # rubocop: disable CodeReuse/ActiveRecord - def batch_clean(start_id: nil, stop_id: nil, dry_run: true, sleep_time: nil) + def batch_clean(start_id: nil, stop_id: nil, dry_run: true, sleep_time: nil, uploader: nil, since: nil) relation = Upload.where('lower(path) like ? or lower(path) like ? or lower(path) like ?', '%.jpg', '%.jpeg', '%.tiff') + relation = relation.where(uploader: uploader) if uploader + relation = relation.where('created_at > ?', since) if since logger.info "running in dry run mode, no images will be rewritten" if dry_run find_params = { start: start_id.present? ? start_id.to_i : nil, - finish: stop_id.present? ? stop_id.to_i : Upload.last&.id + finish: stop_id.present? ? stop_id.to_i : Upload.last&.id, + batch_size: 1000 } relation.find_each(find_params) do |upload| diff --git a/lib/gitlab/visibility_level_checker.rb b/lib/gitlab/visibility_level_checker.rb new file mode 100644 index 00000000000..f15f1486a4e --- /dev/null +++ b/lib/gitlab/visibility_level_checker.rb @@ -0,0 +1,88 @@ +# frozen_string_literal: true + +# Gitlab::VisibilityLevelChecker verifies that: +# - Current @project.visibility_level is not restricted +# - Override visibility param is not restricted +# - @see https://docs.gitlab.com/ce/api/project_import_export.html#import-a-file +# +# @param current_user [User] Current user object to verify visibility level against +# @param project [Project] Current project that is being created/imported +# @param project_params [Hash] Supplementary project params (e.g. import +# params containing visibility override) +# +# @example +# user = User.find(2) +# project = Project.last +# project_params = {:import_data=>{:data=>{:override_params=>{"visibility"=>"public"}}}} +# level_checker = Gitlab::VisibilityLevelChecker.new(user, project, project_params: project_params) +# +# project_visibility = level_checker.level_restricted? +# => #<Gitlab::VisibilityEvaluationResult:0x00007fbe16ee33c0 @restricted=true, @visibility_level=20> +# +# if project_visibility.restricted? +# deny_visibility_level(project, project_visibility.visibility_level) +# end +# +# @return [VisibilityEvaluationResult] Visibility evaluation result. Responds to: +# #restricted - boolean indicating if level is restricted +# #visibility_level - integer of restricted visibility level +# +module Gitlab + class VisibilityLevelChecker + def initialize(current_user, project, project_params: {}) + @current_user = current_user + @project = project + @project_params = project_params + end + + def level_restricted? + return VisibilityEvaluationResult.new(true, override_visibility_level_value) if override_visibility_restricted? + return VisibilityEvaluationResult.new(true, project.visibility_level) if project_visibility_restricted? + + VisibilityEvaluationResult.new(false, nil) + end + + private + + attr_reader :current_user, :project, :project_params + + def override_visibility_restricted? + return unless import_data + return unless override_visibility_level + return if Gitlab::VisibilityLevel.allowed_for?(current_user, override_visibility_level_value) + + true + end + + def project_visibility_restricted? + return if Gitlab::VisibilityLevel.allowed_for?(current_user, project.visibility_level) + + true + end + + def import_data + @import_data ||= project_params[:import_data] + end + + def override_visibility_level + @override_visibility_level ||= import_data.deep_symbolize_keys.dig(:data, :override_params, :visibility) + end + + def override_visibility_level_value + @override_visibility_level_value ||= Gitlab::VisibilityLevel.level_value(override_visibility_level) + end + end + + class VisibilityEvaluationResult + attr_reader :visibility_level + + def initialize(restricted, visibility_level) + @restricted = restricted + @visibility_level = visibility_level + end + + def restricted? + @restricted + end + end +end diff --git a/lib/tasks/gitlab/uploads/sanitize.rake b/lib/tasks/gitlab/uploads/sanitize.rake index 12cf5302555..4f23a0a5d82 100644 --- a/lib/tasks/gitlab/uploads/sanitize.rake +++ b/lib/tasks/gitlab/uploads/sanitize.rake @@ -2,7 +2,7 @@ namespace :gitlab do namespace :uploads do namespace :sanitize do desc 'GitLab | Uploads | Remove EXIF from images.' - task :remove_exif, [:start_id, :stop_id, :dry_run, :sleep_time] => :environment do |task, args| + task :remove_exif, [:start_id, :stop_id, :dry_run, :sleep_time, :uploader, :since] => :environment do |task, args| args.with_defaults(dry_run: 'true') args.with_defaults(sleep_time: 0.3) @@ -11,7 +11,9 @@ namespace :gitlab do sanitizer = Gitlab::Sanitizers::Exif.new(logger: logger) sanitizer.batch_clean(start_id: args.start_id, stop_id: args.stop_id, dry_run: args.dry_run != 'false', - sleep_time: args.sleep_time.to_f) + sleep_time: args.sleep_time.to_f, + uploader: args.uploader, + since: args.since) end end end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index a5e55585480..f9c15929c7c 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -1055,9 +1055,6 @@ msgstr "" msgid "An error occurred while parsing recent searches" msgstr "" -msgid "An error occurred while rendering KaTeX" -msgstr "" - msgid "An error occurred while rendering preview broadcast message" msgstr "" @@ -4021,7 +4018,7 @@ msgstr "" msgid "Enable or disable version check and usage ping." msgstr "" -msgid "Enable reCAPTCHA or Akismet and set IP limits." +msgid "Enable reCAPTCHA or Akismet and set IP limits. For reCAPTCHA, we currently only support %{recaptcha_v2_link_start}v2%{recaptcha_v2_link_end}" msgstr "" msgid "Enable shared Runners" @@ -5250,7 +5247,10 @@ msgstr "" msgid "Help page text and support page url." msgstr "" -msgid "Helps prevent bots from creating accounts. We currently only support %{recaptcha_v2_link_start}reCAPTCHA v2%{recaptcha_v2_link_end}" +msgid "Helps prevent bots from brute-force attacks." +msgstr "" + +msgid "Helps prevent bots from creating accounts." msgstr "" msgid "Hide archived projects" @@ -12890,6 +12890,12 @@ msgstr "" msgid "manual" msgstr "" +msgid "math|The math in this entry is taking too long to render and may not be displayed as expected. For performance reasons, math blocks are also limited to %{maxChars} characters. Consider splitting up large formulae, splitting math blocks among multiple entries, or using an image instead." +msgstr "" + +msgid "math|There was an error rendering this math block" +msgstr "" + msgid "merge request" msgid_plural "merge requests" msgstr[0] "" diff --git a/spec/controllers/groups/runners_controller_spec.rb b/spec/controllers/groups/runners_controller_spec.rb index 91f9e2c7832..14b0cf959b3 100644 --- a/spec/controllers/groups/runners_controller_spec.rb +++ b/spec/controllers/groups/runners_controller_spec.rb @@ -3,73 +3,202 @@ require 'spec_helper' describe Groups::RunnersController do - let(:user) { create(:user) } - let(:group) { create(:group) } + let(:user) { create(:user) } + let(:group) { create(:group) } let(:runner) { create(:ci_runner, :group, groups: [group]) } - - let(:params) do - { - group_id: group, - id: runner - } - end + let(:params) { { group_id: group, id: runner } } before do sign_in(user) - group.add_maintainer(user) + end + + describe '#show' do + context 'when user is owner' do + before do + group.add_owner(user) + end + + it 'renders show with 200 status code' do + get :show, params: { group_id: group, id: runner } + + expect(response).to have_gitlab_http_status(200) + expect(response).to render_template(:show) + end + end + + context 'when user is not owner' do + before do + group.add_maintainer(user) + end + + it 'renders a 404' do + get :show, params: { group_id: group, id: runner } + + expect(response).to have_gitlab_http_status(404) + end + end + end + + describe '#edit' do + context 'when user is owner' do + before do + group.add_owner(user) + end + + it 'renders show with 200 status code' do + get :edit, params: { group_id: group, id: runner } + + expect(response).to have_gitlab_http_status(200) + expect(response).to render_template(:edit) + end + end + + context 'when user is not owner' do + before do + group.add_maintainer(user) + end + + it 'renders a 404' do + get :edit, params: { group_id: group, id: runner } + + expect(response).to have_gitlab_http_status(404) + end + end end describe '#update' do - it 'updates the runner and ticks the queue' do - new_desc = runner.description.swapcase + context 'when user is an owner' do + before do + group.add_owner(user) + end - expect do - post :update, params: params.merge(runner: { description: new_desc } ) - end.to change { runner.ensure_runner_queue_value } + it 'updates the runner, ticks the queue, and redirects' do + new_desc = runner.description.swapcase - runner.reload + expect do + post :update, params: params.merge(runner: { description: new_desc } ) + end.to change { runner.ensure_runner_queue_value } - expect(response).to have_gitlab_http_status(302) - expect(runner.description).to eq(new_desc) + expect(response).to have_gitlab_http_status(302) + expect(runner.reload.description).to eq(new_desc) + end + end + + context 'when user is not an owner' do + before do + group.add_maintainer(user) + end + + it 'rejects the update and responds 404' do + old_desc = runner.description + + expect do + post :update, params: params.merge(runner: { description: old_desc.swapcase } ) + end.not_to change { runner.ensure_runner_queue_value } + + expect(response).to have_gitlab_http_status(404) + expect(runner.reload.description).to eq(old_desc) + end end end describe '#destroy' do - it 'destroys the runner' do - delete :destroy, params: params + context 'when user is an owner' do + before do + group.add_owner(user) + end + + it 'destroys the runner and redirects' do + delete :destroy, params: params + + expect(response).to have_gitlab_http_status(302) + expect(Ci::Runner.find_by(id: runner.id)).to be_nil + end + end + + context 'when user is not an owner' do + before do + group.add_maintainer(user) + end + + it 'responds 404 and does not destroy the runner' do + delete :destroy, params: params - expect(response).to have_gitlab_http_status(302) - expect(Ci::Runner.find_by(id: runner.id)).to be_nil + expect(response).to have_gitlab_http_status(404) + expect(Ci::Runner.find_by(id: runner.id)).to be_present + end end end describe '#resume' do - it 'marks the runner as active and ticks the queue' do - runner.update(active: false) + context 'when user is an owner' do + before do + group.add_owner(user) + end - expect do - post :resume, params: params - end.to change { runner.ensure_runner_queue_value } + it 'marks the runner as active, ticks the queue, and redirects' do + runner.update(active: false) - runner.reload + expect do + post :resume, params: params + end.to change { runner.ensure_runner_queue_value } - expect(response).to have_gitlab_http_status(302) - expect(runner.active).to eq(true) + expect(response).to have_gitlab_http_status(302) + expect(runner.reload.active).to eq(true) + end + end + + context 'when user is not an owner' do + before do + group.add_maintainer(user) + end + + it 'responds 404 and does not activate the runner' do + runner.update(active: false) + + expect do + post :resume, params: params + end.not_to change { runner.ensure_runner_queue_value } + + expect(response).to have_gitlab_http_status(404) + expect(runner.reload.active).to eq(false) + end end end describe '#pause' do - it 'marks the runner as inactive and ticks the queue' do - runner.update(active: true) + context 'when user is an owner' do + before do + group.add_owner(user) + end + + it 'marks the runner as inactive, ticks the queue, and redirects' do + runner.update(active: true) + + expect do + post :pause, params: params + end.to change { runner.ensure_runner_queue_value } + + expect(response).to have_gitlab_http_status(302) + expect(runner.reload.active).to eq(false) + end + end + + context 'when user is not an owner' do + before do + group.add_maintainer(user) + end - expect do - post :pause, params: params - end.to change { runner.ensure_runner_queue_value } + it 'responds 404 and does not update the runner or queue' do + runner.update(active: true) - runner.reload + expect do + post :pause, params: params + end.not_to change { runner.ensure_runner_queue_value } - expect(response).to have_gitlab_http_status(302) - expect(runner.active).to eq(false) + expect(response).to have_gitlab_http_status(404) + expect(runner.reload.active).to eq(true) + end end end end diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index bdd7322290f..64ab1e9e01a 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -719,19 +719,63 @@ describe Projects::MergeRequestsController do end describe 'GET test_reports' do + let(:merge_request) do + create(:merge_request, + :with_diffs, + :with_merge_request_pipeline, + target_project: project, + source_project: project + ) + end + subject do - get :test_reports, - params: { - namespace_id: project.namespace.to_param, - project_id: project, - id: merge_request.iid - }, - format: :json + get :test_reports, params: { + namespace_id: project.namespace.to_param, + project_id: project, + id: merge_request.iid + }, + format: :json end before do allow_any_instance_of(MergeRequest) - .to receive(:compare_test_reports).and_return(comparison_status) + .to receive(:compare_test_reports) + .and_return(comparison_status) + + allow_any_instance_of(MergeRequest) + .to receive(:actual_head_pipeline) + .and_return(merge_request.all_pipelines.take) + end + + describe 'permissions on a public project with private CI/CD' do + let(:project) { create :project, :repository, :public, :builds_private } + let(:comparison_status) { { status: :parsed, data: { summary: 1 } } } + + context 'while signed out' do + before do + sign_out(user) + end + + it 'responds with a 404' do + subject + + expect(response).to have_gitlab_http_status(404) + expect(response.body).to be_blank + end + end + + context 'while signed in as an unrelated user' do + before do + sign_in(create(:user)) + end + + it 'responds with a 404' do + subject + + expect(response).to have_gitlab_http_status(404) + expect(response.body).to be_blank + end + end end context 'when comparison is being processed' do @@ -1053,17 +1097,39 @@ describe Projects::MergeRequestsController do let(:status) { pipeline.detailed_status(double('user')) } - before do + it 'returns a detailed head_pipeline status in json' do get_pipeline_status - end - it 'return a detailed head_pipeline status in json' do expect(response).to have_gitlab_http_status(:ok) expect(json_response['text']).to eq status.text expect(json_response['label']).to eq status.label expect(json_response['icon']).to eq status.icon expect(json_response['favicon']).to match_asset_path "/assets/ci_favicons/#{status.favicon}.png" end + + context 'with project member visibility on a public project' do + let(:user) { create(:user) } + let(:project) { create(:project, :repository, :public, :builds_private) } + + it 'returns pipeline data to project members' do + project.add_developer(user) + + get_pipeline_status + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response['text']).to eq status.text + expect(json_response['label']).to eq status.label + expect(json_response['icon']).to eq status.icon + expect(json_response['favicon']).to match_asset_path "/assets/ci_favicons/#{status.favicon}.png" + end + + it 'returns blank OK response to non-project-members' do + get_pipeline_status + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response).to be_empty + end + end end context 'when head_pipeline does not exist' do @@ -1071,7 +1137,7 @@ describe Projects::MergeRequestsController do get_pipeline_status end - it 'return empty' do + it 'returns blank OK response' do expect(response).to have_gitlab_http_status(:ok) expect(json_response).to be_empty end diff --git a/spec/controllers/projects/notes_controller_spec.rb b/spec/controllers/projects/notes_controller_spec.rb index 98aea9056dc..0d0eb8a9b99 100644 --- a/spec/controllers/projects/notes_controller_spec.rb +++ b/spec/controllers/projects/notes_controller_spec.rb @@ -212,40 +212,232 @@ describe Projects::NotesController do describe 'POST create' do let(:merge_request) { create(:merge_request) } let(:project) { merge_request.source_project } + let(:note_text) { 'some note' } let(:request_params) do { - note: { note: 'some note', noteable_id: merge_request.id, noteable_type: 'MergeRequest' }, + note: { note: note_text, noteable_id: merge_request.id, noteable_type: 'MergeRequest' }, namespace_id: project.namespace, project_id: project, merge_request_diff_head_sha: 'sha', target_type: 'merge_request', target_id: merge_request.id - } + }.merge(extra_request_params) + end + let(:extra_request_params) { {} } + + let(:project_visibility) { Gitlab::VisibilityLevel::PUBLIC } + let(:merge_requests_access_level) { ProjectFeature::ENABLED } + + def create! + post :create, params: request_params end before do + project.update_attribute(:visibility_level, project_visibility) + project.project_feature.update(merge_requests_access_level: merge_requests_access_level) sign_in(user) - project.add_developer(user) end - it "returns status 302 for html" do - post :create, params: request_params + describe 'making the creation request' do + before do + create! + end + + context 'the project is publically available' do + context 'for HTML' do + it "returns status 302" do + expect(response).to have_gitlab_http_status(302) + end + end + + context 'for JSON' do + let(:extra_request_params) { { format: :json } } + + it "returns status 200 for json" do + expect(response).to have_gitlab_http_status(200) + end + end + end - expect(response).to have_gitlab_http_status(302) + context 'the project is a private project' do + let(:project_visibility) { Gitlab::VisibilityLevel::PRIVATE } + + [{}, { format: :json }].each do |extra| + context "format is #{extra[:format]}" do + let(:extra_request_params) { extra } + + it "returns status 404" do + expect(response).to have_gitlab_http_status(404) + end + end + end + end end - it "returns status 200 for json" do - post :create, params: request_params.merge(format: :json) + context 'the user is a developer on a private project' do + let(:project_visibility) { Gitlab::VisibilityLevel::PRIVATE } - expect(response).to have_gitlab_http_status(200) + before do + project.add_developer(user) + end + + context 'HTML requests' do + it "returns status 302 (redirect)" do + create! + + expect(response).to have_gitlab_http_status(302) + end + end + + context 'JSON requests' do + let(:extra_request_params) { { format: :json } } + + it "returns status 200" do + create! + + expect(response).to have_gitlab_http_status(200) + end + end + + context 'the return_discussion param is set' do + let(:extra_request_params) { { format: :json, return_discussion: 'true' } } + + it 'returns discussion JSON when the return_discussion param is set' do + create! + + expect(response).to have_gitlab_http_status(200) + expect(json_response).to have_key 'discussion' + expect(json_response.dig('discussion', 'notes', 0, 'note')).to eq(request_params[:note][:note]) + end + end + + context 'when creating a note with quick actions' do + context 'with commands that return changes' do + let(:note_text) { "/award :thumbsup:\n/estimate 1d\n/spend 3h" } + let(:extra_request_params) { { format: :json } } + + it 'includes changes in commands_changes ' do + create! + + expect(response).to have_gitlab_http_status(200) + expect(json_response['commands_changes']).to include('emoji_award', 'time_estimate', 'spend_time') + expect(json_response['commands_changes']).not_to include('target_project', 'title') + end + end + + context 'with commands that do not return changes' do + let(:issue) { create(:issue, project: project) } + let(:other_project) { create(:project) } + let(:note_text) { "/move #{other_project.full_path}\n/title AAA" } + let(:extra_request_params) { { format: :json, target_id: issue.id, target_type: 'issue' } } + + before do + other_project.add_developer(user) + end + + it 'does not include changes in commands_changes' do + create! + + expect(response).to have_gitlab_http_status(200) + expect(json_response['commands_changes']).not_to include('target_project', 'title') + end + end + end end - it 'returns discussion JSON when the return_discussion param is set' do - post :create, params: request_params.merge(format: :json, return_discussion: 'true') + context 'when the internal project prohibits non-members from accessing merge requests' do + let(:project_visibility) { Gitlab::VisibilityLevel::INTERNAL } + let(:merge_requests_access_level) { ProjectFeature::PRIVATE } - expect(response).to have_gitlab_http_status(200) - expect(json_response).to have_key 'discussion' - expect(json_response['discussion']['notes'][0]['note']).to eq(request_params[:note][:note]) + it "prevents a non-member user from creating a note on one of the project's merge requests" do + create! + + expect(response).to have_gitlab_http_status(404) + end + + context 'when the user is a team member' do + before do + project.add_developer(user) + end + + it 'can add comments' do + expect { create! }.to change { project.notes.count }.by(1) + end + end + + # Illustration of the attack vector for posting comments to discussions that should + # be inaccessible. + # + # This relies on posting a note to a commit that is not necessarily even in the + # merge request, with a value of :in_reply_to_discussion_id that points to a + # discussion on a merge_request that should not be accessible. + context 'when the request includes a :in_reply_to_discussion_id designed to fool us' do + let(:commit) { create(:commit, project: project) } + + let(:existing_comment) do + create(:note_on_commit, + note: 'first', + project: project, + commit_id: merge_request.commit_shas.first) + end + + let(:discussion) { existing_comment.discussion } + + # see !60465 for details of the structure of this request + let(:request_params) do + { "utf8" => "✓", + "authenticity_token" => "1", + "view" => "inline", + "line_type" => "", + "merge_request_diff_head_sha" => "", + "in_reply_to_discussion_id" => discussion.id, + "note_project_id" => project.id, + "project_id" => project.id, + "namespace_id" => project.namespace, + "target_type" => "commit", + "target_id" => commit.id, + "note" => { + "noteable_type" => "", + "noteable_id" => "", + "commit_id" => "", + "type" => "", + "line_code" => "", + "position" => "", + "note" => "ThisReplyWillGoToMergeRequest" + } } + end + + it 'prevents the request from adding notes to the spoofed discussion' do + expect { create! }.not_to change { discussion.notes.count } + end + + it 'returns an error to the user' do + create! + expect(response).to have_gitlab_http_status(404) + end + end + end + + context 'when the public project prohibits non-members from accessing merge requests' do + let(:project_visibility) { Gitlab::VisibilityLevel::PUBLIC } + let(:merge_requests_access_level) { ProjectFeature::PRIVATE } + + it "prevents a non-member user from creating a note on one of the project's merge requests" do + create! + + expect(response).to have_gitlab_http_status(404) + end + + context 'when the user is a team member' do + before do + project.add_developer(user) + create! + end + + it 'can add comments' do + expect(response).to be_redirect + end + end end context 'when merge_request_diff_head_sha present' do @@ -262,7 +454,7 @@ describe Projects::NotesController do end it "returns status 302 for html" do - post :create, params: request_params + create! expect(response).to have_gitlab_http_status(302) end @@ -285,7 +477,7 @@ describe Projects::NotesController do end context 'when creating a commit comment from an MR fork' do - let(:project) { create(:project, :repository) } + let(:project) { create(:project, :repository, :public) } let(:forked_project) do fork_project(project, nil, repository: true) @@ -299,45 +491,59 @@ describe Projects::NotesController do create(:note_on_commit, note: 'a note', project: forked_project, commit_id: merge_request.commit_shas.first) end - def post_create(extra_params = {}) - post :create, params: { + let(:note_project_id) do + forked_project.id + end + + let(:request_params) do + { note: { note: 'some other note', noteable_id: merge_request.id }, namespace_id: project.namespace, project_id: project, target_type: 'merge_request', target_id: merge_request.id, - note_project_id: forked_project.id, + note_project_id: note_project_id, in_reply_to_discussion_id: existing_comment.discussion_id - }.merge(extra_params) + } + end + + let(:fork_visibility) { Gitlab::VisibilityLevel::PUBLIC } + + before do + forked_project.update_attribute(:visibility_level, fork_visibility) end context 'when the note_project_id is not correct' do - it 'returns a 404' do - post_create(note_project_id: Project.maximum(:id).succ) + let(:note_project_id) do + project.id && Project.maximum(:id).succ + end + it 'returns a 404' do + create! expect(response).to have_gitlab_http_status(404) end end context 'when the user has no access to the fork' do - it 'returns a 404' do - post_create + let(:fork_visibility) { Gitlab::VisibilityLevel::PRIVATE } + it 'returns a 404' do + create! expect(response).to have_gitlab_http_status(404) end end context 'when the user has access to the fork' do - let(:discussion) { forked_project.notes.find_discussion(existing_comment.discussion_id) } + let!(:discussion) { forked_project.notes.find_discussion(existing_comment.discussion_id) } + let(:fork_visibility) { Gitlab::VisibilityLevel::PUBLIC } - before do - forked_project.add_developer(user) - - existing_comment + it 'is successful' do + create! + expect(response).to have_gitlab_http_status(302) end it 'creates the note' do - expect { post_create }.to change { forked_project.notes.count }.by(1) + expect { create! }.to change { forked_project.notes.count }.by(1) end end end @@ -346,11 +552,6 @@ describe Projects::NotesController do let(:locked_issue) { create(:issue, :locked, project: project) } let(:issue) {create(:issue, project: project)} - before do - project.update_attribute(:visibility_level, Gitlab::VisibilityLevel::PUBLIC) - project.project_member(user).destroy - end - it 'uses target_id and ignores noteable_id' do request_params = { note: { note: 'some note', noteable_type: 'Issue', noteable_id: locked_issue.id }, @@ -368,7 +569,6 @@ describe Projects::NotesController do context 'when the merge request discussion is locked' do before do - project.update_attribute(:visibility_level, Gitlab::VisibilityLevel::PUBLIC) merge_request.update_attribute(:discussion_locked, true) end @@ -382,6 +582,10 @@ describe Projects::NotesController do end context 'when a user is a team member' do + before do + project.add_developer(user) + end + it 'returns 302 status for html' do post :create, params: request_params @@ -400,10 +604,6 @@ describe Projects::NotesController do end context 'when a user is not a team member' do - before do - project.project_member(user).destroy - end - it 'returns 404 status' do post :create, params: request_params @@ -415,37 +615,6 @@ describe Projects::NotesController do end end end - - context 'when creating a note with quick actions' do - context 'with commands that return changes' do - let(:note_text) { "/award :thumbsup:\n/estimate 1d\n/spend 3h" } - - it 'includes changes in commands_changes ' do - post :create, params: request_params.merge(note: { note: note_text }, format: :json) - - expect(response).to have_gitlab_http_status(200) - expect(json_response['commands_changes']).to include('emoji_award', 'time_estimate', 'spend_time') - expect(json_response['commands_changes']).not_to include('target_project', 'title') - end - end - - context 'with commands that do not return changes' do - let(:issue) { create(:issue, project: project) } - let(:other_project) { create(:project) } - let(:note_text) { "/move #{other_project.full_path}\n/title AAA" } - - before do - other_project.add_developer(user) - end - - it 'does not include changes in commands_changes' do - post :create, params: request_params.merge(note: { note: note_text }, target_type: 'issue', target_id: issue.id, format: :json) - - expect(response).to have_gitlab_http_status(200) - expect(json_response['commands_changes']).not_to include('target_project', 'title') - end - end - end end describe 'PUT update' do diff --git a/spec/controllers/projects/services_controller_spec.rb b/spec/controllers/projects/services_controller_spec.rb index 68eabce8513..22ae65ea2fb 100644 --- a/spec/controllers/projects/services_controller_spec.rb +++ b/spec/controllers/projects/services_controller_spec.rb @@ -11,6 +11,7 @@ describe Projects::ServicesController do before do sign_in(user) project.add_maintainer(user) + allow(Gitlab::UrlBlocker).to receive(:validate!).and_return([URI.parse('http://example.com'), nil]) end describe '#test' do @@ -56,6 +57,8 @@ describe Projects::ServicesController do stub_request(:get, 'http://example.com/rest/api/2/serverInfo') .to_return(status: 200, body: '{}') + expect(Gitlab::HTTP).to receive(:get).with("/rest/api/2/serverInfo", any_args).and_call_original + put :test, params: { namespace_id: project.namespace, project_id: project, id: service.to_param, service: service_params } expect(response.status).to eq(200) @@ -66,6 +69,8 @@ describe Projects::ServicesController do stub_request(:get, 'http://example.com/rest/api/2/serverInfo') .to_return(status: 200, body: '{}') + expect(Gitlab::HTTP).to receive(:get).with("/rest/api/2/serverInfo", any_args).and_call_original + put :test, params: { namespace_id: project.namespace, project_id: project, id: service.to_param, service: service_params } expect(response.status).to eq(200) diff --git a/spec/controllers/sessions_controller_spec.rb b/spec/controllers/sessions_controller_spec.rb index 9c4ddce5409..68b7bf61231 100644 --- a/spec/controllers/sessions_controller_spec.rb +++ b/spec/controllers/sessions_controller_spec.rb @@ -100,16 +100,8 @@ describe SessionsController do end end - context 'when reCAPTCHA is enabled' do - let(:user) { create(:user) } - let(:user_params) { { login: user.username, password: user.password } } - - before do - stub_application_setting(recaptcha_enabled: true) - request.headers[described_class::CAPTCHA_HEADER] = 1 - end - - it 'displays an error when the reCAPTCHA is not solved' do + context 'with reCAPTCHA' do + def unsuccesful_login(user_params, sesion_params: {}) # Without this, `verify_recaptcha` arbitrarily returns true in test env Recaptcha.configuration.skip_verify_env.delete('test') counter = double(:counter) @@ -119,14 +111,10 @@ describe SessionsController do .with(:failed_login_captcha_total, anything) .and_return(counter) - post(:create, params: { user: user_params }) - - expect(response).to render_template(:new) - expect(flash[:alert]).to include 'There was an error with the reCAPTCHA. Please solve the reCAPTCHA again.' - expect(subject.current_user).to be_nil + post(:create, params: { user: user_params }, session: sesion_params) end - it 'successfully logs in a user when reCAPTCHA is solved' do + def succesful_login(user_params, sesion_params: {}) # Avoid test ordering issue and ensure `verify_recaptcha` returns true Recaptcha.configuration.skip_verify_env << 'test' counter = double(:counter) @@ -137,9 +125,80 @@ describe SessionsController do .and_return(counter) expect(Gitlab::Metrics).to receive(:counter).and_call_original - post(:create, params: { user: user_params }) + post(:create, params: { user: user_params }, session: sesion_params) + end - expect(subject.current_user).to eq user + context 'when reCAPTCHA is enabled' do + let(:user) { create(:user) } + let(:user_params) { { login: user.username, password: user.password } } + + before do + stub_application_setting(recaptcha_enabled: true) + request.headers[described_class::CAPTCHA_HEADER] = 1 + end + + it 'displays an error when the reCAPTCHA is not solved' do + # Without this, `verify_recaptcha` arbitrarily returns true in test env + + unsuccesful_login(user_params) + + expect(response).to render_template(:new) + expect(flash[:alert]).to include 'There was an error with the reCAPTCHA. Please solve the reCAPTCHA again.' + expect(subject.current_user).to be_nil + end + + it 'successfully logs in a user when reCAPTCHA is solved' do + succesful_login(user_params) + + expect(subject.current_user).to eq user + end + end + + context 'when reCAPTCHA login protection is enabled' do + let(:user) { create(:user) } + let(:user_params) { { login: user.username, password: user.password } } + + before do + stub_application_setting(login_recaptcha_protection_enabled: true) + end + + context 'when user tried to login 5 times' do + it 'displays an error when the reCAPTCHA is not solved' do + unsuccesful_login(user_params, sesion_params: { failed_login_attempts: 6 }) + + expect(response).to render_template(:new) + expect(flash[:alert]).to include 'There was an error with the reCAPTCHA. Please solve the reCAPTCHA again.' + expect(subject.current_user).to be_nil + end + + it 'successfully logs in a user when reCAPTCHA is solved' do + succesful_login(user_params, sesion_params: { failed_login_attempts: 6 }) + + expect(subject.current_user).to eq user + end + end + + context 'when there are more than 5 anonymous session with the same IP' do + before do + allow(Gitlab::AnonymousSession).to receive_message_chain(:new, :stored_sessions).and_return(6) + end + + it 'displays an error when the reCAPTCHA is not solved' do + unsuccesful_login(user_params) + + expect(response).to render_template(:new) + expect(flash[:alert]).to include 'There was an error with the reCAPTCHA. Please solve the reCAPTCHA again.' + expect(subject.current_user).to be_nil + end + + it 'successfully logs in a user when reCAPTCHA is solved' do + expect(Gitlab::AnonymousSession).to receive_message_chain(:new, :cleanup_session_per_ip_entries) + + succesful_login(user_params) + + expect(subject.current_user).to eq user + end + end end end end @@ -348,4 +407,17 @@ describe SessionsController do expect(controller.stored_location_for(:redirect)).to eq(search_path) end end + + context 'when login fails' do + before do + set_devise_mapping(context: @request) + @request.env["warden.options"] = { action: 'unauthenticated' } + end + + it 'does increment failed login counts for session' do + get(:new, params: { user: { login: 'failed' } }) + + expect(session[:failed_login_attempts]).to eq(1) + end + end end diff --git a/spec/controllers/uploads_controller_spec.rb b/spec/controllers/uploads_controller_spec.rb index 0876502a899..5f4a6bf8ee7 100644 --- a/spec/controllers/uploads_controller_spec.rb +++ b/spec/controllers/uploads_controller_spec.rb @@ -21,8 +21,20 @@ shared_examples 'content publicly cached' do end describe UploadsController do + include WorkhorseHelpers + let!(:user) { create(:user, avatar: fixture_file_upload("spec/fixtures/dk.png", "image/png")) } + describe 'POST #authorize' do + it_behaves_like 'handle uploads authorize' do + let(:uploader_class) { PersonalFileUploader } + let(:model) { create(:personal_snippet, :public) } + let(:params) do + { model: 'personal_snippet', id: model.id } + end + end + end + describe 'POST create' do let(:jpg) { fixture_file_upload('spec/fixtures/rails_sample.jpg', 'image/jpg') } let(:txt) { fixture_file_upload('spec/fixtures/doc_sample.txt', 'text/plain') } @@ -636,4 +648,10 @@ describe UploadsController do end end end + + def post_authorize(verified: true) + request.headers.merge!(workhorse_internal_api_request_header) if verified + + post :authorize, params: { model: 'personal_snippet', id: model.id }, format: :json + end end diff --git a/spec/features/admin/admin_settings_spec.rb b/spec/features/admin/admin_settings_spec.rb index 518b3625348..ec43b9aabb2 100644 --- a/spec/features/admin/admin_settings_spec.rb +++ b/spec/features/admin/admin_settings_spec.rb @@ -261,6 +261,7 @@ describe 'Admin updates settings' do page.within('.as-spam') do check 'Enable reCAPTCHA' + check 'Enable reCAPTCHA for login' fill_in 'reCAPTCHA Site Key', with: 'key' fill_in 'reCAPTCHA Private Key', with: 'key' fill_in 'IPs per user', with: 15 @@ -269,6 +270,7 @@ describe 'Admin updates settings' do expect(page).to have_content "Application settings saved successfully" expect(current_settings.recaptcha_enabled).to be true + expect(current_settings.login_recaptcha_protection_enabled).to be true expect(current_settings.unique_ips_limit_per_user).to eq(15) end end diff --git a/spec/features/markdown/math_spec.rb b/spec/features/markdown/math_spec.rb index 16ad0d456be..776da128a47 100644 --- a/spec/features/markdown/math_spec.rb +++ b/spec/features/markdown/math_spec.rb @@ -32,7 +32,9 @@ describe 'Math rendering', :js do visit project_issue_path(project, issue) - expect(page).to have_selector('.katex-error', text: "\href{javascript:alert('xss');}{xss}") - expect(page).to have_selector('.katex-html a', text: 'Gitlab') + page.within '.description > .md' do + expect(page).to have_selector('.katex-error') + expect(page).to have_selector('.katex-html a', text: 'Gitlab') + end end end diff --git a/spec/features/profiles/user_edit_profile_spec.rb b/spec/features/profiles/user_edit_profile_spec.rb index a53da94ef7d..9ab5026dbc4 100644 --- a/spec/features/profiles/user_edit_profile_spec.rb +++ b/spec/features/profiles/user_edit_profile_spec.rb @@ -47,6 +47,23 @@ describe 'User edit profile' do end end + describe 'when I change my email' do + before do + user.send_reset_password_instructions + end + + it 'clears the reset password token' do + expect(user.reset_password_token?).to be true + + fill_in 'user_email', with: 'new-email@example.com' + submit_settings + + user.reload + expect(user.confirmation_token).not_to be_nil + expect(user.reset_password_token?).to be false + end + end + context 'user avatar' do before do attach_file(:user_avatar, Rails.root.join('spec', 'fixtures', 'banana_sample.gif')) diff --git a/spec/helpers/labels_helper_spec.rb b/spec/helpers/labels_helper_spec.rb index 314305d7a8e..5892113c4d8 100644 --- a/spec/helpers/labels_helper_spec.rb +++ b/spec/helpers/labels_helper_spec.rb @@ -296,4 +296,14 @@ describe LabelsHelper do it { is_expected.to eq('Subscribe at group level') } end end + + describe '#label_tooltip_title' do + let(:html) { '<img src="example.png">This is an image</img>' } + let(:label_with_html_content) { create(:label, title: 'test', description: html) } + + it 'removes HTML' do + tooltip = label_tooltip_title(label_with_html_content) + expect(tooltip).to eq('This is an image') + end + end end diff --git a/spec/initializers/asset_proxy_setting_spec.rb b/spec/initializers/asset_proxy_setting_spec.rb new file mode 100644 index 00000000000..42e4d4aa594 --- /dev/null +++ b/spec/initializers/asset_proxy_setting_spec.rb @@ -0,0 +1,13 @@ +require 'spec_helper' + +describe 'Asset proxy settings initialization' do + describe '#asset_proxy' do + it 'defaults to disabled' do + expect(Banzai::Filter::AssetProxyFilter).to receive(:initialize_settings) + + require_relative '../../config/initializers/asset_proxy_settings' + + expect(Gitlab.config.asset_proxy.enabled).to be_falsey + end + end +end diff --git a/spec/initializers/rest-client-hostname_override_spec.rb b/spec/initializers/rest-client-hostname_override_spec.rb new file mode 100644 index 00000000000..1ff82342fb5 --- /dev/null +++ b/spec/initializers/rest-client-hostname_override_spec.rb @@ -0,0 +1,147 @@ +require 'spec_helper' + +describe 'rest-client dns rebinding protection' do + include StubRequests + + context 'when local requests are not allowed' do + it 'allows an external request with http' do + request_stub = stub_full_request('http://example.com', ip_address: '93.184.216.34') + + RestClient.get('http://example.com/') + + expect(request_stub).to have_been_requested + end + + it 'allows an external request with https' do + request_stub = stub_full_request('https://example.com', ip_address: '93.184.216.34') + + RestClient.get('https://example.com/') + + expect(request_stub).to have_been_requested + end + + it 'raises error when it is a request that resolves to a local address' do + stub_full_request('https://example.com', ip_address: '172.16.0.0') + + expect { RestClient.get('https://example.com') } + .to raise_error(ArgumentError, + "URL 'https://example.com' is blocked: Requests to the local network are not allowed") + end + + it 'raises error when it is a request that resolves to a localhost address' do + stub_full_request('https://example.com', ip_address: '127.0.0.1') + + expect { RestClient.get('https://example.com') } + .to raise_error(ArgumentError, + "URL 'https://example.com' is blocked: Requests to localhost are not allowed") + end + + it 'raises error when it is a request to local address' do + expect { RestClient.get('http://172.16.0.0') } + .to raise_error(ArgumentError, + "URL 'http://172.16.0.0' is blocked: Requests to the local network are not allowed") + end + + it 'raises error when it is a request to localhost address' do + expect { RestClient.get('http://127.0.0.1') } + .to raise_error(ArgumentError, + "URL 'http://127.0.0.1' is blocked: Requests to localhost are not allowed") + end + end + + context 'when port different from URL scheme is used' do + it 'allows the request' do + request_stub = stub_full_request('https://example.com:8080', ip_address: '93.184.216.34') + + RestClient.get('https://example.com:8080/') + + expect(request_stub).to have_been_requested + end + + it 'raises error when it is a request to local address' do + expect { RestClient.get('https://172.16.0.0:8080') } + .to raise_error(ArgumentError, + "URL 'https://172.16.0.0:8080' is blocked: Requests to the local network are not allowed") + end + + it 'raises error when it is a request to localhost address' do + expect { RestClient.get('https://127.0.0.1:8080') } + .to raise_error(ArgumentError, + "URL 'https://127.0.0.1:8080' is blocked: Requests to localhost are not allowed") + end + end + + context 'when DNS rebinding protection is disabled' do + before do + stub_application_setting(dns_rebinding_protection_enabled: false) + end + + it 'allows the request' do + request_stub = stub_request(:get, 'https://example.com') + + RestClient.get('https://example.com/') + + expect(request_stub).to have_been_requested + end + end + + context 'when http(s) proxy environment variable is set' do + before do + stub_env('https_proxy' => 'https://my.proxy') + end + + it 'allows the request' do + request_stub = stub_request(:get, 'https://example.com') + + RestClient.get('https://example.com/') + + expect(request_stub).to have_been_requested + end + end + + context 'when local requests are allowed' do + before do + stub_application_setting(allow_local_requests_from_hooks_and_services: true) + end + + it 'allows an external request' do + request_stub = stub_full_request('https://example.com', ip_address: '93.184.216.34') + + RestClient.get('https://example.com/') + + expect(request_stub).to have_been_requested + end + + it 'allows an external request that resolves to a local address' do + request_stub = stub_full_request('https://example.com', ip_address: '172.16.0.0') + + RestClient.get('https://example.com/') + + expect(request_stub).to have_been_requested + end + + it 'allows an external request that resolves to a localhost address' do + request_stub = stub_full_request('https://example.com', ip_address: '127.0.0.1') + + RestClient.get('https://example.com/') + + expect(request_stub).to have_been_requested + end + + it 'allows a local address request' do + request_stub = stub_request(:get, 'http://172.16.0.0') + + RestClient.get('http://172.16.0.0') + + expect(request_stub).to have_been_requested + end + + it 'allows a localhost address request' do + request_stub = stub_request(:get, 'http://127.0.0.1') + + RestClient.get('http://127.0.0.1') + + expect(request_stub).to have_been_requested + end + end +end diff --git a/spec/lib/banzai/filter/asset_proxy_filter_spec.rb b/spec/lib/banzai/filter/asset_proxy_filter_spec.rb new file mode 100644 index 00000000000..b7f45421b2a --- /dev/null +++ b/spec/lib/banzai/filter/asset_proxy_filter_spec.rb @@ -0,0 +1,95 @@ +require 'spec_helper' + +describe Banzai::Filter::AssetProxyFilter do + include FilterSpecHelper + + def image(path) + %(<img src="#{path}" />) + end + + it 'does not replace if disabled' do + stub_asset_proxy_setting(enabled: false) + + context = described_class.transform_context({}) + src = 'http://example.com/test.png' + doc = filter(image(src), context) + + expect(doc.at_css('img')['src']).to eq src + end + + context 'during initialization' do + after do + Gitlab.config.asset_proxy['enabled'] = false + end + + it '#initialize_settings' do + stub_application_setting(asset_proxy_enabled: true) + stub_application_setting(asset_proxy_secret_key: 'shared-secret') + stub_application_setting(asset_proxy_url: 'https://assets.example.com') + stub_application_setting(asset_proxy_whitelist: %w(gitlab.com *.mydomain.com)) + + described_class.initialize_settings + + expect(Gitlab.config.asset_proxy.enabled).to be_truthy + expect(Gitlab.config.asset_proxy.secret_key).to eq 'shared-secret' + expect(Gitlab.config.asset_proxy.url).to eq 'https://assets.example.com' + expect(Gitlab.config.asset_proxy.whitelist).to eq %w(gitlab.com *.mydomain.com) + expect(Gitlab.config.asset_proxy.domain_regexp).to eq /^(gitlab\.com|.*?\.mydomain\.com)$/i + end + end + + context 'when properly configured' do + before do + stub_asset_proxy_setting(enabled: true) + stub_asset_proxy_setting(secret_key: 'shared-secret') + stub_asset_proxy_setting(url: 'https://assets.example.com') + stub_asset_proxy_setting(whitelist: %W(gitlab.com *.mydomain.com #{Gitlab.config.gitlab.host})) + stub_asset_proxy_setting(domain_regexp: described_class.compile_whitelist(Gitlab.config.asset_proxy.whitelist)) + @context = described_class.transform_context({}) + end + + it 'replaces img src' do + src = 'http://example.com/test.png' + new_src = 'https://assets.example.com/08df250eeeef1a8cf2c761475ac74c5065105612/687474703a2f2f6578616d706c652e636f6d2f746573742e706e67' + doc = filter(image(src), @context) + + expect(doc.at_css('img')['src']).to eq new_src + expect(doc.at_css('img')['data-canonical-src']).to eq src + end + + it 'skips internal images' do + src = "#{Gitlab.config.gitlab.url}/test.png" + doc = filter(image(src), @context) + + expect(doc.at_css('img')['src']).to eq src + end + + it 'skip relative urls' do + src = "/test.png" + doc = filter(image(src), @context) + + expect(doc.at_css('img')['src']).to eq src + end + + it 'skips single domain' do + src = "http://gitlab.com/test.png" + doc = filter(image(src), @context) + + expect(doc.at_css('img')['src']).to eq src + end + + it 'skips single domain and ignores url in query string' do + src = "http://gitlab.com/test.png?url=http://example.com/test.png" + doc = filter(image(src), @context) + + expect(doc.at_css('img')['src']).to eq src + end + + it 'skips wildcarded domain' do + src = "http://images.mydomain.com/test.png" + doc = filter(image(src), @context) + + expect(doc.at_css('img')['src']).to eq src + end + end +end diff --git a/spec/lib/banzai/filter/external_link_filter_spec.rb b/spec/lib/banzai/filter/external_link_filter_spec.rb index 2acbe05f082..9cb5f835b7f 100644 --- a/spec/lib/banzai/filter/external_link_filter_spec.rb +++ b/spec/lib/banzai/filter/external_link_filter_spec.rb @@ -154,6 +154,18 @@ describe Banzai::Filter::ExternalLinkFilter do expect(doc_email.to_html).to include('http://xn--example-6p25f.com/</a>') end end + + context 'autolinked image' do + let(:html) { %q(<a href="https://assets.example.com/6d8b/634c" data-canonical-src="http://exa%F0%9F%98%84mple.com/test.png"><img src="http://exa%F0%9F%98%84mple.com/test.png" data-canonical-src="http://exa%F0%9F%98%84mple.com/test.png"></a>) } + let(:doc) { filter(html) } + + it_behaves_like 'an external link with rel attribute' + + it 'adds a toolip with punycode' do + expect(doc.to_html).to include('class="has-tooltip"') + expect(doc.to_html).to include('title="http://xn--example-6p25f.com/test.png"') + end + end end context 'for links that look malicious' do diff --git a/spec/lib/banzai/filter/image_link_filter_spec.rb b/spec/lib/banzai/filter/image_link_filter_spec.rb index c84b98eb225..f9a830aeff9 100644 --- a/spec/lib/banzai/filter/image_link_filter_spec.rb +++ b/spec/lib/banzai/filter/image_link_filter_spec.rb @@ -26,4 +26,11 @@ describe Banzai::Filter::ImageLinkFilter do doc = filter(%Q(<p>test #{image('/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg')} inline</p>)) expect(doc.to_html).to match %r{^<p>test <a[^>]*><img[^>]*></a> inline</p>$} end + + it 'keep the data-canonical-src' do + doc = filter(%q(<img src="http://assets.example.com/6cd/4d7" data-canonical-src="http://example.com/test.png" />)) + + expect(doc.at_css('img')['src']).to eq doc.at_css('a')['href'] + expect(doc.at_css('img')['data-canonical-src']).to eq doc.at_css('a')['data-canonical-src'] + end end diff --git a/spec/lib/banzai/filter/label_reference_filter_spec.rb b/spec/lib/banzai/filter/label_reference_filter_spec.rb index 108d7b43a26..799539b0f46 100644 --- a/spec/lib/banzai/filter/label_reference_filter_spec.rb +++ b/spec/lib/banzai/filter/label_reference_filter_spec.rb @@ -8,6 +8,11 @@ describe Banzai::Filter::LabelReferenceFilter do let(:label) { create(:label, project: project) } let(:reference) { label.to_reference } + it_behaves_like 'HTML text with references' do + let(:resource) { label } + let(:resource_text) { resource.title } + end + it 'requires project context' do expect { described_class.call('') }.to raise_error(ArgumentError, /:project/) end diff --git a/spec/lib/banzai/filter/milestone_reference_filter_spec.rb b/spec/lib/banzai/filter/milestone_reference_filter_spec.rb index f0a5dc8d0d7..e3d50dcf86b 100644 --- a/spec/lib/banzai/filter/milestone_reference_filter_spec.rb +++ b/spec/lib/banzai/filter/milestone_reference_filter_spec.rb @@ -327,6 +327,10 @@ describe Banzai::Filter::MilestoneReferenceFilter do it_behaves_like 'cross-project / same-namespace complete reference' it_behaves_like 'cross project shorthand reference' it_behaves_like 'references with HTML entities' + it_behaves_like 'HTML text with references' do + let(:resource) { milestone } + let(:resource_text) { "#{resource.class.reference_prefix}#{resource.title}" } + end end shared_context 'group milestones' do @@ -338,6 +342,10 @@ describe Banzai::Filter::MilestoneReferenceFilter do it_behaves_like 'String-based multi-word references in quotes' it_behaves_like 'referencing a milestone in a link href' it_behaves_like 'references with HTML entities' + it_behaves_like 'HTML text with references' do + let(:resource) { milestone } + let(:resource_text) { "#{resource.class.reference_prefix}#{resource.title}" } + end it 'does not support references by IID' do doc = reference_filter("See #{Milestone.reference_prefix}#{milestone.iid}") diff --git a/spec/lib/banzai/filter/project_reference_filter_spec.rb b/spec/lib/banzai/filter/project_reference_filter_spec.rb index 69f9c1ae829..927d226c400 100644 --- a/spec/lib/banzai/filter/project_reference_filter_spec.rb +++ b/spec/lib/banzai/filter/project_reference_filter_spec.rb @@ -26,10 +26,18 @@ describe Banzai::Filter::ProjectReferenceFilter do expect(reference_filter(act).to_html).to eq(CGI.escapeHTML(exp)) end - it 'fails fast for long invalid string' do - expect do - Timeout.timeout(5.seconds) { reference_filter("A" * 50000).to_html } - end.not_to raise_error + context 'when invalid reference strings are very long' do + shared_examples_for 'fails fast' do |ref_string| + it 'fails fast for long strings' do + # took well under 1 second in CI https://dev.gitlab.org/gitlab/gitlabhq/merge_requests/3267#note_172824 + expect do + Timeout.timeout(3.seconds) { reference_filter(ref_string).to_html } + end.not_to raise_error + end + end + + it_behaves_like 'fails fast', 'A' * 50000 + it_behaves_like 'fails fast', '/a' * 50000 end it 'allows references with text after the > character' do diff --git a/spec/lib/banzai/filter/video_link_filter_spec.rb b/spec/lib/banzai/filter/video_link_filter_spec.rb index 81dda0687f3..d63bf4d64c1 100644 --- a/spec/lib/banzai/filter/video_link_filter_spec.rb +++ b/spec/lib/banzai/filter/video_link_filter_spec.rb @@ -47,4 +47,26 @@ describe Banzai::Filter::VideoLinkFilter do expect(element['src']).to eq '/path/my_image.jpg' end end + + context 'when asset proxy is enabled' do + it 'uses the correct src' do + stub_asset_proxy_setting(enabled: true) + + proxy_src = 'https://assets.example.com/6d8b63' + canonical_src = 'http://example.com/test.mp4' + image = %(<img src="#{proxy_src}" data-canonical-src="#{canonical_src}" />) + container = filter(image, asset_proxy_enabled: true).children.first + + expect(container['class']).to eq 'video-container' + + video, paragraph = container.children + + expect(video['src']).to eq proxy_src + expect(video['data-canonical-src']).to eq canonical_src + + link = paragraph.children.first + + expect(link['href']).to eq proxy_src + end + end end diff --git a/spec/lib/banzai/pipeline/gfm_pipeline_spec.rb b/spec/lib/banzai/pipeline/gfm_pipeline_spec.rb index 469692f7b5a..02408622499 100644 --- a/spec/lib/banzai/pipeline/gfm_pipeline_spec.rb +++ b/spec/lib/banzai/pipeline/gfm_pipeline_spec.rb @@ -140,4 +140,48 @@ describe Banzai::Pipeline::GfmPipeline do expect(output).to include(Gitlab::Routing.url_helpers.milestone_path(milestone)) end end + + describe 'asset proxy' do + let(:project) { create(:project, :public) } + let(:image) { '' } + let(:proxy) { 'https://assets.example.com/08df250eeeef1a8cf2c761475ac74c5065105612/687474703a2f2f6578616d706c652e636f6d2f746573742e706e67' } + let(:version) { Gitlab::CurrentSettings.current_application_settings.local_markdown_version } + + before do + stub_asset_proxy_setting(enabled: true) + stub_asset_proxy_setting(secret_key: 'shared-secret') + stub_asset_proxy_setting(url: 'https://assets.example.com') + stub_asset_proxy_setting(whitelist: %W(gitlab.com *.mydomain.com #{Gitlab.config.gitlab.host})) + stub_asset_proxy_setting(domain_regexp: Banzai::Filter::AssetProxyFilter.compile_whitelist(Gitlab.config.asset_proxy.whitelist)) + end + + it 'replaces a lazy loaded img src' do + output = described_class.to_html(image, project: project) + doc = Nokogiri::HTML.fragment(output) + result = doc.css('img').first + + expect(result['data-src']).to eq(proxy) + end + + it 'autolinks images to the proxy' do + output = described_class.to_html(image, project: project) + doc = Nokogiri::HTML.fragment(output) + result = doc.css('a').first + + expect(result['href']).to eq(proxy) + expect(result['data-canonical-src']).to eq('http://example.com/test.png') + end + + it 'properly adds tooltips to link for IDN images' do + image = '' + proxy = 'https://assets.example.com/6d8b634c412a23c6bfe1b2963f174febf5635ddd/687474703a2f2f6578612546302539462539382538346d706c652e636f6d2f746573742e706e67' + output = described_class.to_html(image, project: project) + doc = Nokogiri::HTML.fragment(output) + result = doc.css('a').first + + expect(result['href']).to eq(proxy) + expect(result['data-canonical-src']).to eq('http://exa%F0%9F%98%84mple.com/test.png') + expect(result['title']).to eq 'http://xn--example-6p25f.com/test.png' + end + end end diff --git a/spec/lib/gitlab/anonymous_session_spec.rb b/spec/lib/gitlab/anonymous_session_spec.rb new file mode 100644 index 00000000000..628aae81ada --- /dev/null +++ b/spec/lib/gitlab/anonymous_session_spec.rb @@ -0,0 +1,78 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe Gitlab::AnonymousSession, :clean_gitlab_redis_shared_state do + let(:default_session_id) { '6919a6f1bb119dd7396fadc38fd18d0d' } + let(:additional_session_id) { '7919a6f1bb119dd7396fadc38fd18d0d' } + + subject { new_anonymous_session } + + def new_anonymous_session(session_id = default_session_id) + described_class.new('127.0.0.1', session_id: session_id) + end + + describe '#store_session_id_per_ip' do + it 'adds session id to proper key' do + subject.store_session_id_per_ip + + Gitlab::Redis::SharedState.with do |redis| + expect(redis.smembers("session:lookup:ip:gitlab:127.0.0.1")).to eq [default_session_id] + end + end + + it 'adds expiration time to key' do + Timecop.freeze do + subject.store_session_id_per_ip + + Gitlab::Redis::SharedState.with do |redis| + expect(redis.ttl("session:lookup:ip:gitlab:127.0.0.1")).to eq(24.hours.to_i) + end + end + end + + it 'adds id only once' do + subject.store_session_id_per_ip + subject.store_session_id_per_ip + + Gitlab::Redis::SharedState.with do |redis| + expect(redis.smembers("session:lookup:ip:gitlab:127.0.0.1")).to eq [default_session_id] + end + end + + context 'when there is already one session' do + it 'adds session id to proper key' do + subject.store_session_id_per_ip + new_anonymous_session(additional_session_id).store_session_id_per_ip + + Gitlab::Redis::SharedState.with do |redis| + expect(redis.smembers("session:lookup:ip:gitlab:127.0.0.1")).to contain_exactly(default_session_id, additional_session_id) + end + end + end + end + + describe '#stored_sessions' do + it 'returns all anonymous sessions per ip' do + Gitlab::Redis::SharedState.with do |redis| + redis.sadd("session:lookup:ip:gitlab:127.0.0.1", default_session_id) + redis.sadd("session:lookup:ip:gitlab:127.0.0.1", additional_session_id) + end + + expect(subject.stored_sessions).to eq(2) + end + end + + it 'removes obsolete lookup through ip entries' do + Gitlab::Redis::SharedState.with do |redis| + redis.sadd("session:lookup:ip:gitlab:127.0.0.1", default_session_id) + redis.sadd("session:lookup:ip:gitlab:127.0.0.1", additional_session_id) + end + + subject.cleanup_session_per_ip_entries + + Gitlab::Redis::SharedState.with do |redis| + expect(redis.smembers("session:lookup:ip:gitlab:127.0.0.1")).to eq [additional_session_id] + end + end +end diff --git a/spec/lib/gitlab/kubernetes/kube_client_spec.rb b/spec/lib/gitlab/kubernetes/kube_client_spec.rb index 97ebb5f1554..cceb57394cf 100644 --- a/spec/lib/gitlab/kubernetes/kube_client_spec.rb +++ b/spec/lib/gitlab/kubernetes/kube_client_spec.rb @@ -3,6 +3,7 @@ require 'spec_helper' describe Gitlab::Kubernetes::KubeClient do + include StubRequests include KubernetesHelpers let(:api_url) { 'https://kubernetes.example.com/prefix' } @@ -14,6 +15,17 @@ describe Gitlab::Kubernetes::KubeClient do stub_kubeclient_discover(api_url) end + def method_call(client, method_name) + case method_name + when /\A(get_|delete_)/ + client.public_send(method_name) + when /\A(create_|update_)/ + client.public_send(method_name, {}) + else + raise "Unknown method name #{method_name}" + end + end + shared_examples 'a Kubeclient' do it 'is a Kubeclient::Client' do is_expected.to be_an_instance_of Kubeclient::Client @@ -25,28 +37,30 @@ describe Gitlab::Kubernetes::KubeClient do end shared_examples 'redirection not allowed' do |method_name| - before do - redirect_url = 'https://not-under-our-control.example.com/api/v1/pods' + context 'api_url is redirected' do + before do + redirect_url = 'https://not-under-our-control.example.com/api/v1/pods' - stub_request(:get, %r{\A#{api_url}/}) - .to_return(status: 302, headers: { location: redirect_url }) + stub_request(:get, %r{\A#{api_url}/}) + .to_return(status: 302, headers: { location: redirect_url }) - stub_request(:get, redirect_url) - .to_return(status: 200, body: '{}') - end + stub_request(:get, redirect_url) + .to_return(status: 200, body: '{}') + end - it 'does not follow redirects' do - method_call = -> do - case method_name - when /\A(get_|delete_)/ - client.public_send(method_name) - when /\A(create_|update_)/ - client.public_send(method_name, {}) - else - raise "Unknown method name #{method_name}" - end + it 'does not follow redirects' do + expect { method_call(client, method_name) }.to raise_error(Kubeclient::HttpError) end - expect { method_call.call }.to raise_error(Kubeclient::HttpError) + end + end + + shared_examples 'dns rebinding not allowed' do |method_name| + it 'does not allow DNS rebinding' do + stub_dns(api_url, ip_address: '8.8.8.8') + client + + stub_dns(api_url, ip_address: '192.168.2.120') + expect { method_call(client, method_name) }.to raise_error(ArgumentError, /is blocked/) end end @@ -160,6 +174,7 @@ describe Gitlab::Kubernetes::KubeClient do ].each do |method| describe "##{method}" do include_examples 'redirection not allowed', method + include_examples 'dns rebinding not allowed', method it 'delegates to the core client' do expect(client).to delegate_method(method).to(:core_client) @@ -185,6 +200,7 @@ describe Gitlab::Kubernetes::KubeClient do ].each do |method| describe "##{method}" do include_examples 'redirection not allowed', method + include_examples 'dns rebinding not allowed', method it 'delegates to the rbac client' do expect(client).to delegate_method(method).to(:rbac_client) @@ -203,6 +219,7 @@ describe Gitlab::Kubernetes::KubeClient do describe '#get_deployments' do include_examples 'redirection not allowed', 'get_deployments' + include_examples 'dns rebinding not allowed', 'get_deployments' it 'delegates to the extensions client' do expect(client).to delegate_method(:get_deployments).to(:extensions_client) diff --git a/spec/lib/gitlab/sanitizers/exif_spec.rb b/spec/lib/gitlab/sanitizers/exif_spec.rb index bd5f330c7a1..0a4bc1bbdbb 100644 --- a/spec/lib/gitlab/sanitizers/exif_spec.rb +++ b/spec/lib/gitlab/sanitizers/exif_spec.rb @@ -5,7 +5,9 @@ describe Gitlab::Sanitizers::Exif do describe '#batch_clean' do context 'with image uploads' do - let!(:uploads) { create_list(:upload, 3, :with_file, :issuable_upload) } + set(:upload1) { create(:upload, :with_file, :issuable_upload) } + set(:upload2) { create(:upload, :with_file, :personal_snippet_upload) } + set(:upload3) { create(:upload, :with_file, created_at: 3.days.ago) } it 'processes all uploads if range ID is not set' do expect(sanitizer).to receive(:clean).exactly(3).times @@ -16,7 +18,19 @@ describe Gitlab::Sanitizers::Exif do it 'processes only uploads in the selected range' do expect(sanitizer).to receive(:clean).once - sanitizer.batch_clean(start_id: uploads[1].id, stop_id: uploads[1].id) + sanitizer.batch_clean(start_id: upload1.id, stop_id: upload1.id) + end + + it 'processes only uploads for the selected uploader' do + expect(sanitizer).to receive(:clean).once + + sanitizer.batch_clean(uploader: 'PersonalFileUploader') + end + + it 'processes only uploads created since specified date' do + expect(sanitizer).to receive(:clean).exactly(2).times + + sanitizer.batch_clean(since: 2.days.ago) end it 'pauses if sleep_time is set' do diff --git a/spec/lib/gitlab/visibility_level_checker_spec.rb b/spec/lib/gitlab/visibility_level_checker_spec.rb new file mode 100644 index 00000000000..325ac3c6f31 --- /dev/null +++ b/spec/lib/gitlab/visibility_level_checker_spec.rb @@ -0,0 +1,82 @@ +require 'spec_helper' + +describe Gitlab::VisibilityLevelChecker do + let(:user) { create(:user) } + let(:project) { create(:project) } + let(:visibility_level_checker) { } + let(:override_params) { {} } + + subject { described_class.new(user, project, project_params: override_params) } + + describe '#level_restricted?' do + context 'when visibility level is allowed' do + it 'returns false with nil for visibility level' do + result = subject.level_restricted? + + expect(result.restricted?).to eq(false) + expect(result.visibility_level).to be_nil + end + end + + context 'when visibility level is restricted' do + before do + stub_application_setting(restricted_visibility_levels: [Gitlab::VisibilityLevel::PUBLIC]) + end + + it 'returns true and visibility name' do + project.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC) + result = subject.level_restricted? + + expect(result.restricted?).to eq(true) + expect(result.visibility_level).to eq(Gitlab::VisibilityLevel::PUBLIC) + end + + context 'overridden visibility' do + let(:override_params) do + { + import_data: { + data: { + override_params: { + visibility: override_visibility + } + } + } + } + end + + context 'when restricted' do + let(:override_visibility) { 'public' } + + it 'returns true and visibility name' do + result = subject.level_restricted? + + expect(result.restricted?).to eq(true) + expect(result.visibility_level).to eq(Gitlab::VisibilityLevel::PUBLIC) + end + end + + context 'when misspelled' do + let(:override_visibility) { 'publik' } + + it 'returns false with nil for visibility level' do + result = subject.level_restricted? + + expect(result.restricted?).to eq(false) + expect(result.visibility_level).to be_nil + end + end + + context 'when import_data is missing' do + let(:override_params) { {} } + + it 'returns false with nil for visibility level' do + result = subject.level_restricted? + + expect(result.restricted?).to eq(false) + expect(result.visibility_level).to be_nil + end + end + end + end + end +end diff --git a/spec/models/application_setting_spec.rb b/spec/models/application_setting_spec.rb index ab6f6dfe720..904185f4140 100644 --- a/spec/models/application_setting_spec.rb +++ b/spec/models/application_setting_spec.rb @@ -344,6 +344,71 @@ describe ApplicationSetting do end end end + + context 'asset proxy settings' do + before do + subject.asset_proxy_enabled = true + end + + describe '#asset_proxy_url' do + it { is_expected.not_to allow_value('').for(:asset_proxy_url) } + it { is_expected.to allow_value(http).for(:asset_proxy_url) } + it { is_expected.to allow_value(https).for(:asset_proxy_url) } + it { is_expected.not_to allow_value(ftp).for(:asset_proxy_url) } + + it 'is not required when asset proxy is disabled' do + subject.asset_proxy_enabled = false + subject.asset_proxy_url = '' + + expect(subject).to be_valid + end + end + + describe '#asset_proxy_secret_key' do + it { is_expected.not_to allow_value('').for(:asset_proxy_secret_key) } + it { is_expected.to allow_value('anything').for(:asset_proxy_secret_key) } + + it 'is not required when asset proxy is disabled' do + subject.asset_proxy_enabled = false + subject.asset_proxy_secret_key = '' + + expect(subject).to be_valid + end + + it 'is encrypted' do + subject.asset_proxy_secret_key = 'shared secret' + + expect(subject.encrypted_asset_proxy_secret_key).to be_present + expect(subject.encrypted_asset_proxy_secret_key).not_to eq(subject.asset_proxy_secret_key) + end + end + + describe '#asset_proxy_whitelist' do + context 'when given an Array' do + it 'sets the domains and adds current running host' do + setting.asset_proxy_whitelist = ['example.com', 'assets.example.com'] + expect(setting.asset_proxy_whitelist).to eq(['example.com', 'assets.example.com', 'localhost']) + end + end + + context 'when given a String' do + it 'sets multiple domains with spaces' do + setting.asset_proxy_whitelist = 'example.com *.example.com' + expect(setting.asset_proxy_whitelist).to eq(['example.com', '*.example.com', 'localhost']) + end + + it 'sets multiple domains with newlines and a space' do + setting.asset_proxy_whitelist = "example.com\n *.example.com" + expect(setting.asset_proxy_whitelist).to eq(['example.com', '*.example.com', 'localhost']) + end + + it 'sets multiple domains with commas' do + setting.asset_proxy_whitelist = "example.com, *.example.com" + expect(setting.asset_proxy_whitelist).to eq(['example.com', '*.example.com', 'localhost']) + end + end + end + end end context 'restrict creating duplicates' do diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb index 68224a56515..537bad98fa4 100644 --- a/spec/models/concerns/issuable_spec.rb +++ b/spec/models/concerns/issuable_spec.rb @@ -46,6 +46,7 @@ describe Issuable do it { is_expected.to validate_presence_of(:author) } it { is_expected.to validate_presence_of(:title) } it { is_expected.to validate_length_of(:title).is_at_most(255) } + it { is_expected.to validate_length_of(:description).is_at_most(1_000_000) } end describe 'milestone' do @@ -774,4 +775,54 @@ describe Issuable do end end end + + describe '#matches_cross_reference_regex?' do + context "issue description with long path string" do + let(:mentionable) { build(:issue, description: "/a" * 50000) } + + it_behaves_like 'matches_cross_reference_regex? fails fast' + end + + context "note with long path string" do + let(:mentionable) { build(:note, note: "/a" * 50000) } + + it_behaves_like 'matches_cross_reference_regex? fails fast' + end + + context "note with long path string" do + let(:project) { create(:project, :public, :repository) } + let(:mentionable) { project.commit } + + before do + expect(mentionable.raw).to receive(:message).and_return("/a" * 50000) + end + + it_behaves_like 'matches_cross_reference_regex? fails fast' + end + end + + describe '#matches_cross_reference_regex?' do + context "issue description with long path string" do + let(:mentionable) { build(:issue, description: "/a" * 50000) } + + it_behaves_like 'matches_cross_reference_regex? fails fast' + end + + context "note with long path string" do + let(:mentionable) { build(:note, note: "/a" * 50000) } + + it_behaves_like 'matches_cross_reference_regex? fails fast' + end + + context "note with long path string" do + let(:project) { create(:project, :public, :repository) } + let(:mentionable) { project.commit } + + before do + expect(mentionable.raw).to receive(:message).and_return("/a" * 50000) + end + + it_behaves_like 'matches_cross_reference_regex? fails fast' + end + end end diff --git a/spec/models/label_spec.rb b/spec/models/label_spec.rb index 5174c590a10..c182e693ca7 100644 --- a/spec/models/label_spec.rb +++ b/spec/models/label_spec.rb @@ -84,6 +84,13 @@ describe Label do end end + describe '#description' do + it 'sanitizes description' do + label = described_class.new(description: '<b>foo & bar?</b>') + expect(label.description).to eq('foo & bar?') + end + end + describe 'priorization' do subject(:label) { create(:label) } diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb index 03003e3dd7d..5bc0673dd73 100644 --- a/spec/models/note_spec.rb +++ b/spec/models/note_spec.rb @@ -22,6 +22,7 @@ describe Note do end describe 'validation' do + it { is_expected.to validate_length_of(:note).is_at_most(1_000_000) } it { is_expected.to validate_presence_of(:note) } it { is_expected.to validate_presence_of(:project) } diff --git a/spec/models/project_services/discord_service_spec.rb b/spec/models/project_services/discord_service_spec.rb index be82f223478..96ac532dcd1 100644 --- a/spec/models/project_services/discord_service_spec.rb +++ b/spec/models/project_services/discord_service_spec.rb @@ -8,4 +8,37 @@ describe DiscordService do let(:client_arguments) { { url: webhook_url } } let(:content_key) { :content } end + + describe '#execute' do + include StubRequests + + let(:user) { create(:user) } + let(:project) { create(:project, :repository) } + let(:webhook_url) { "https://example.gitlab.com/" } + + let(:sample_data) do + Gitlab::DataBuilder::Push.build_sample(project, user) + end + + before do + allow(subject).to receive_messages( + project: project, + project_id: project.id, + service_hook: true, + webhook: webhook_url + ) + + WebMock.stub_request(:post, webhook_url) + end + + context 'DNS rebind to local address' do + before do + stub_dns(webhook_url, ip_address: '192.168.2.120') + end + + it 'does not allow DNS rebinding' do + expect { subject.execute(sample_data) }.to raise_error(ArgumentError, /is blocked/) + end + end + end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 5cfa64fd764..f4e1fea739b 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -3037,6 +3037,47 @@ describe User do end end + describe '#will_save_change_to_login?' do + let(:user) { create(:user, username: 'old-username', email: 'old-email@example.org') } + let(:new_username) { 'new-name' } + let(:new_email) { 'new-email@example.org' } + + subject { user.will_save_change_to_login? } + + context 'when the username is changed' do + before do + user.username = new_username + end + + it { is_expected.to be true } + end + + context 'when the email is changed' do + before do + user.email = new_email + end + + it { is_expected.to be true } + end + + context 'when both email and username are changed' do + before do + user.username = new_username + user.email = new_email + end + + it { is_expected.to be true } + end + + context 'when email and username aren\'t changed' do + before do + user.name = 'new_name' + end + + it { is_expected.to be_falsy } + end + end + describe '#sync_attribute?' do let(:user) { described_class.new } diff --git a/spec/policies/issue_policy_spec.rb b/spec/policies/issue_policy_spec.rb index b149dbcf871..25267d36ab8 100644 --- a/spec/policies/issue_policy_spec.rb +++ b/spec/policies/issue_policy_spec.rb @@ -172,6 +172,34 @@ describe IssuePolicy do expect(permissions(assignee, issue_locked)).to be_disallowed(:admin_issue, :reopen_issue) end + context 'when issues are private' do + before do + project.project_feature.update(issues_access_level: ProjectFeature::PRIVATE) + end + let(:issue) { create(:issue, project: project, author: author) } + let(:visitor) { create(:user) } + let(:admin) { create(:user, :admin) } + + it 'forbids visitors from viewing issues' do + expect(permissions(visitor, issue)).to be_disallowed(:read_issue) + end + it 'forbids visitors from commenting' do + expect(permissions(visitor, issue)).to be_disallowed(:create_note) + end + it 'allows guests to view' do + expect(permissions(guest, issue)).to be_allowed(:read_issue) + end + it 'allows guests to comment' do + expect(permissions(guest, issue)).to be_allowed(:create_note) + end + it 'allows admins to view' do + expect(permissions(admin, issue)).to be_allowed(:read_issue) + end + it 'allows admins to comment' do + expect(permissions(admin, issue)).to be_allowed(:create_note) + end + end + context 'with confidential issues' do let(:confidential_issue) { create(:issue, :confidential, project: project, assignees: [assignee], author: author) } let(:confidential_issue_no_assignee) { create(:issue, :confidential, project: project) } diff --git a/spec/policies/merge_request_policy_spec.rb b/spec/policies/merge_request_policy_spec.rb index 81279225d61..87205f56589 100644 --- a/spec/policies/merge_request_policy_spec.rb +++ b/spec/policies/merge_request_policy_spec.rb @@ -6,6 +6,7 @@ describe MergeRequestPolicy do let(:guest) { create(:user) } let(:author) { create(:user) } let(:developer) { create(:user) } + let(:non_team_member) { create(:user) } let(:project) { create(:project, :public) } def permissions(user, merge_request) @@ -18,6 +19,78 @@ describe MergeRequestPolicy do project.add_developer(developer) end + MR_PERMS = %i[create_merge_request_in + create_merge_request_from + read_merge_request + create_note].freeze + + shared_examples_for 'a denied user' do + let(:perms) { permissions(subject, merge_request) } + + MR_PERMS.each do |thing| + it "cannot #{thing}" do + expect(perms).to be_disallowed(thing) + end + end + end + + shared_examples_for 'a user with access' do + let(:perms) { permissions(subject, merge_request) } + + MR_PERMS.each do |thing| + it "can #{thing}" do + expect(perms).to be_allowed(thing) + end + end + end + + context 'when merge requests have been disabled' do + let!(:merge_request) { create(:merge_request, source_project: project, target_project: project, author: author) } + + before do + project.project_feature.update(merge_requests_access_level: ProjectFeature::DISABLED) + end + + describe 'the author' do + subject { author } + it_behaves_like 'a denied user' + end + + describe 'a guest' do + subject { guest } + it_behaves_like 'a denied user' + end + + describe 'a developer' do + subject { developer } + it_behaves_like 'a denied user' + end + + describe 'any other user' do + subject { non_team_member } + it_behaves_like 'a denied user' + end + end + + context 'when merge requests are private' do + let!(:merge_request) { create(:merge_request, source_project: project, target_project: project, author: author) } + + before do + project.update(visibility_level: Gitlab::VisibilityLevel::PUBLIC) + project.project_feature.update(merge_requests_access_level: ProjectFeature::PRIVATE) + end + + describe 'a non-team-member' do + subject { non_team_member } + it_behaves_like 'a denied user' + end + + describe 'a developer' do + subject { developer } + it_behaves_like 'a user with access' + end + end + context 'when merge request is unlocked' do let(:merge_request) { create(:merge_request, :closed, source_project: project, target_project: project, author: author) } @@ -48,6 +121,22 @@ describe MergeRequestPolicy do it 'prevents guests from reopening merge request' do expect(permissions(guest, merge_request_locked)).to be_disallowed(:reopen_merge_request) end + + context 'when the user is not a project member' do + let(:user) { create(:user) } + + it 'cannot create a note' do + expect(permissions(user, merge_request_locked)).to be_disallowed(:create_note) + end + end + + context 'when the user is project member, with at least guest access' do + let(:user) { guest } + + it 'can create a note' do + expect(permissions(user, merge_request_locked)).to be_allowed(:create_note) + end + end end context 'with external authorization enabled' do diff --git a/spec/requests/api/settings_spec.rb b/spec/requests/api/settings_spec.rb index 8a60980fe80..f4697df99d0 100644 --- a/spec/requests/api/settings_spec.rb +++ b/spec/requests/api/settings_spec.rb @@ -158,5 +158,33 @@ describe API::Settings, 'Settings' do expect(json_response['error']).to eq('plantuml_url is missing') end end + + context 'asset_proxy settings' do + it 'updates application settings' do + put api('/application/settings', admin), + params: { + asset_proxy_enabled: true, + asset_proxy_url: 'http://assets.example.com', + asset_proxy_secret_key: 'shared secret', + asset_proxy_whitelist: ['example.com', '*.example.com'] + } + + expect(response).to have_gitlab_http_status(200) + expect(json_response['asset_proxy_enabled']).to be(true) + expect(json_response['asset_proxy_url']).to eq('http://assets.example.com') + expect(json_response['asset_proxy_secret_key']).to be_nil + expect(json_response['asset_proxy_whitelist']).to eq(['example.com', '*.example.com', 'localhost']) + end + + it 'allows a string for asset_proxy_whitelist' do + put api('/application/settings', admin), + params: { + asset_proxy_whitelist: 'example.com, *.example.com' + } + + expect(response).to have_gitlab_http_status(200) + expect(json_response['asset_proxy_whitelist']).to eq(['example.com', '*.example.com', 'localhost']) + end + end end end diff --git a/spec/services/application_settings/update_service_spec.rb b/spec/services/application_settings/update_service_spec.rb index a641828faa5..7eb27016140 100644 --- a/spec/services/application_settings/update_service_spec.rb +++ b/spec/services/application_settings/update_service_spec.rb @@ -62,6 +62,39 @@ describe ApplicationSettings::UpdateService do end end + describe 'markdown cache invalidators' do + shared_examples 'invalidates markdown cache' do |attribute| + let(:params) { attribute } + + it 'increments cache' do + expect { subject.execute }.to change(application_settings, :local_markdown_version).by(1) + end + end + + it_behaves_like 'invalidates markdown cache', { asset_proxy_enabled: true } + it_behaves_like 'invalidates markdown cache', { asset_proxy_url: 'http://test.com' } + it_behaves_like 'invalidates markdown cache', { asset_proxy_secret_key: 'another secret' } + it_behaves_like 'invalidates markdown cache', { asset_proxy_whitelist: ['domain.com'] } + + context 'when also setting the local_markdown_version' do + let(:params) { { asset_proxy_enabled: true, local_markdown_version: 12 } } + + it 'does not increment' do + expect { subject.execute }.to change(application_settings, :local_markdown_version).to(12) + end + end + + context 'do not invalidate if value does not change' do + let(:params) { { asset_proxy_enabled: true, asset_proxy_secret_key: 'secret', asset_proxy_url: 'http://test.com' } } + + it 'does not increment' do + described_class.new(application_settings, admin, params).execute + + expect { described_class.new(application_settings, admin, params).execute }.not_to change(application_settings, :local_markdown_version) + end + end + end + describe 'performance bar settings' do using RSpec::Parameterized::TableSyntax diff --git a/spec/services/projects/create_service_spec.rb b/spec/services/projects/create_service_spec.rb index b0b74407812..a87ae7bfd37 100644 --- a/spec/services/projects/create_service_spec.rb +++ b/spec/services/projects/create_service_spec.rb @@ -182,27 +182,65 @@ describe Projects::CreateService, '#execute' do context 'restricted visibility level' do before do stub_application_setting(restricted_visibility_levels: [Gitlab::VisibilityLevel::PUBLIC]) + end - opts.merge!( - visibility_level: Gitlab::VisibilityLevel::PUBLIC - ) + shared_examples 'restricted visibility' do + it 'does not allow a restricted visibility level for non-admins' do + project = create_project(user, opts) + + expect(project).to respond_to(:errors) + expect(project.errors.messages).to have_key(:visibility_level) + expect(project.errors.messages[:visibility_level].first).to( + match('restricted by your GitLab administrator') + ) + end + + it 'allows a restricted visibility level for admins' do + admin = create(:admin) + project = create_project(admin, opts) + + expect(project.errors.any?).to be(false) + expect(project.saved?).to be(true) + end end - it 'does not allow a restricted visibility level for non-admins' do - project = create_project(user, opts) - expect(project).to respond_to(:errors) - expect(project.errors.messages).to have_key(:visibility_level) - expect(project.errors.messages[:visibility_level].first).to( - match('restricted by your GitLab administrator') - ) + context 'when visibility is project based' do + before do + opts.merge!( + visibility_level: Gitlab::VisibilityLevel::PUBLIC + ) + end + + include_examples 'restricted visibility' end - it 'allows a restricted visibility level for admins' do - admin = create(:admin) - project = create_project(admin, opts) + context 'when visibility is overridden' do + let(:visibility) { 'public' } - expect(project.errors.any?).to be(false) - expect(project.saved?).to be(true) + before do + opts.merge!( + import_data: { + data: { + override_params: { + visibility: visibility + } + } + } + ) + end + + include_examples 'restricted visibility' + + context 'when visibility is misspelled' do + let(:visibility) { 'publik' } + + it 'does not restrict project creation' do + project = create_project(user, opts) + + expect(project.errors.any?).to be(false) + expect(project.saved?).to be(true) + end + end end end diff --git a/spec/services/todo_service_spec.rb b/spec/services/todo_service_spec.rb index 9ee23f3eb48..bdf2f59704c 100644 --- a/spec/services/todo_service_spec.rb +++ b/spec/services/todo_service_spec.rb @@ -436,25 +436,114 @@ describe TodoService do should_create_todo(user: john_doe, target: confidential_issue, author: john_doe, action: Todo::DIRECTLY_ADDRESSED, note: addressed_note_on_confidential_issue) end - context 'on commit' do - let(:project) { create(:project, :repository) } - - it 'creates a todo for each valid mentioned user when leaving a note on commit' do - service.new_note(note_on_commit, john_doe) - - should_create_todo(user: member, target_id: nil, target_type: 'Commit', commit_id: note_on_commit.commit_id, author: john_doe, action: Todo::MENTIONED, note: note_on_commit) - should_create_todo(user: author, target_id: nil, target_type: 'Commit', commit_id: note_on_commit.commit_id, author: john_doe, action: Todo::MENTIONED, note: note_on_commit) - should_create_todo(user: john_doe, target_id: nil, target_type: 'Commit', commit_id: note_on_commit.commit_id, author: john_doe, action: Todo::MENTIONED, note: note_on_commit) - should_not_create_todo(user: non_member, target_id: nil, target_type: 'Commit', commit_id: note_on_commit.commit_id, author: john_doe, action: Todo::MENTIONED, note: note_on_commit) + context 'commits' do + let(:base_commit_todo_attrs) { { target_id: nil, target_type: 'Commit', author: john_doe } } + + context 'leaving a note on a commit in a public project' do + let(:project) { create(:project, :repository, :public) } + it 'creates a todo for each valid mentioned user' do + expected_todo = base_commit_todo_attrs.merge( + action: Todo::MENTIONED, + note: note_on_commit, + commit_id: note_on_commit.commit_id + ) + + service.new_note(note_on_commit, john_doe) + + should_create_todo(expected_todo.merge(user: member)) + should_create_todo(expected_todo.merge(user: author)) + should_create_todo(expected_todo.merge(user: john_doe)) + should_create_todo(expected_todo.merge(user: guest)) + should_create_todo(expected_todo.merge(user: non_member)) + end + + it 'creates a directly addressed todo for each valid mentioned user' do + expected_todo = base_commit_todo_attrs.merge( + action: Todo::DIRECTLY_ADDRESSED, + note: addressed_note_on_commit, + commit_id: addressed_note_on_commit.commit_id + ) + + service.new_note(addressed_note_on_commit, john_doe) + + should_create_todo(expected_todo.merge(user: member)) + should_create_todo(expected_todo.merge(user: author)) + should_create_todo(expected_todo.merge(user: john_doe)) + should_create_todo(expected_todo.merge(user: guest)) + should_create_todo(expected_todo.merge(user: non_member)) + end end - it 'creates a directly addressed todo for each valid mentioned user when leaving a note on commit' do - service.new_note(addressed_note_on_commit, john_doe) + context 'leaving a note on a commit in a public project with private code' do + let(:project) { create(:project, :repository, :public, :repository_private) } + + it 'creates a todo for each valid mentioned user' do + expected_todo = base_commit_todo_attrs.merge( + action: Todo::MENTIONED, + note: note_on_commit, + commit_id: note_on_commit.commit_id + ) + + service.new_note(note_on_commit, john_doe) + + should_create_todo(expected_todo.merge(user: member)) + should_create_todo(expected_todo.merge(user: author)) + should_create_todo(expected_todo.merge(user: john_doe)) + should_create_todo(expected_todo.merge(user: guest)) + should_not_create_todo(expected_todo.merge(user: non_member)) + end + + it 'creates a directly addressed todo for each valid mentioned user' do + expected_todo = base_commit_todo_attrs.merge( + action: Todo::DIRECTLY_ADDRESSED, + note: addressed_note_on_commit, + commit_id: addressed_note_on_commit.commit_id + ) + + service.new_note(addressed_note_on_commit, john_doe) + + should_create_todo(expected_todo.merge(user: member)) + should_create_todo(expected_todo.merge(user: author)) + should_create_todo(expected_todo.merge(user: john_doe)) + should_create_todo(expected_todo.merge(user: guest)) + should_not_create_todo(expected_todo.merge(user: non_member)) + end + end - should_create_todo(user: member, target_id: nil, target_type: 'Commit', commit_id: addressed_note_on_commit.commit_id, author: john_doe, action: Todo::DIRECTLY_ADDRESSED, note: addressed_note_on_commit) - should_create_todo(user: author, target_id: nil, target_type: 'Commit', commit_id: addressed_note_on_commit.commit_id, author: john_doe, action: Todo::DIRECTLY_ADDRESSED, note: addressed_note_on_commit) - should_create_todo(user: john_doe, target_id: nil, target_type: 'Commit', commit_id: addressed_note_on_commit.commit_id, author: john_doe, action: Todo::DIRECTLY_ADDRESSED, note: addressed_note_on_commit) - should_not_create_todo(user: non_member, target_id: nil, target_type: 'Commit', commit_id: addressed_note_on_commit.commit_id, author: john_doe, action: Todo::DIRECTLY_ADDRESSED, note: addressed_note_on_commit) + context 'leaving a note on a commit in a private project' do + let(:project) { create(:project, :repository, :private) } + + it 'creates a todo for each valid mentioned user' do + expected_todo = base_commit_todo_attrs.merge( + action: Todo::MENTIONED, + note: note_on_commit, + commit_id: note_on_commit.commit_id + ) + + service.new_note(note_on_commit, john_doe) + + should_create_todo(expected_todo.merge(user: member)) + should_create_todo(expected_todo.merge(user: author)) + should_create_todo(expected_todo.merge(user: john_doe)) + should_not_create_todo(expected_todo.merge(user: guest)) + should_not_create_todo(expected_todo.merge(user: non_member)) + end + + it 'creates a directly addressed todo for each valid mentioned user' do + expected_todo = base_commit_todo_attrs.merge( + action: Todo::DIRECTLY_ADDRESSED, + note: addressed_note_on_commit, + commit_id: addressed_note_on_commit.commit_id + ) + + service.new_note(addressed_note_on_commit, john_doe) + + should_create_todo(expected_todo.merge(user: member)) + should_create_todo(expected_todo.merge(user: author)) + should_create_todo(expected_todo.merge(user: john_doe)) + should_not_create_todo(expected_todo.merge(user: guest)) + should_not_create_todo(expected_todo.merge(user: non_member)) + end end end diff --git a/spec/support/helpers/stub_configuration.rb b/spec/support/helpers/stub_configuration.rb index 049702be1f6..49b0619f05d 100644 --- a/spec/support/helpers/stub_configuration.rb +++ b/spec/support/helpers/stub_configuration.rb @@ -99,6 +99,10 @@ module StubConfiguration allow(Gitlab.config.gitlab_shell).to receive_messages(to_settings(messages)) end + def stub_asset_proxy_setting(messages) + allow(Gitlab.config.asset_proxy).to receive_messages(to_settings(messages)) + end + def stub_rack_attack_setting(messages) allow(Gitlab.config.rack_attack).to receive(:git_basic_auth).and_return(messages) allow(Gitlab.config.rack_attack.git_basic_auth).to receive_messages(to_settings(messages)) diff --git a/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb b/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb index 9036838e50a..d6bc1493254 100644 --- a/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb +++ b/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb @@ -5,6 +5,8 @@ shared_examples 'handle uploads' do let(:secret) { FileUploader.generate_secret } let(:uploader_class) { FileUploader } + it_behaves_like 'handle uploads authorize' + describe "POST #create" do context 'when a user is not authorized to upload a file' do it 'returns 404 status' do @@ -269,7 +271,9 @@ shared_examples 'handle uploads' do end end end +end +shared_examples 'handle uploads authorize' do describe "POST #authorize" do context 'when a user is not authorized to upload a file' do it 'returns 404 status' do @@ -282,7 +286,12 @@ shared_examples 'handle uploads' do context 'when a user can upload a file' do before do sign_in(user) - model.add_developer(user) + + if model.is_a?(PersonalSnippet) + model.update!(author: user) + else + model.add_developer(user) + end end context 'and the request bypassed workhorse' do diff --git a/spec/support/shared_examples/lib/banzai/filters/reference_filter_shared_examples.rb b/spec/support/shared_examples/lib/banzai/filters/reference_filter_shared_examples.rb new file mode 100644 index 00000000000..b1ecd4fd007 --- /dev/null +++ b/spec/support/shared_examples/lib/banzai/filters/reference_filter_shared_examples.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +RSpec.shared_examples 'HTML text with references' do + let(:markdown_prepend) { "<img src=\"\" onerror=alert(`bug`)>" } + + it 'preserves escaped HTML text and adds valid references' do + reference = resource.to_reference(format: :name) + + doc = reference_filter("#{markdown_prepend}#{reference}") + + expect(doc.to_html).to start_with(markdown_prepend) + expect(doc.text).to eq %(<img src="" onerror=alert(`bug`)>#{resource_text}) + end + + it 'preserves escaped HTML text if there are no valid references' do + reference = "#{resource.class.reference_prefix}invalid" + text = "#{markdown_prepend}#{reference}" + + doc = reference_filter(text) + + expect(doc.to_html).to eq text + end +end diff --git a/spec/support/shared_examples/models/concern/issuable_shared_examples.rb b/spec/support/shared_examples/models/concern/issuable_shared_examples.rb new file mode 100644 index 00000000000..9604555c57d --- /dev/null +++ b/spec/support/shared_examples/models/concern/issuable_shared_examples.rb @@ -0,0 +1,8 @@ +shared_examples_for 'matches_cross_reference_regex? fails fast' do + it 'fails fast for long strings' do + # took well under 1 second in CI https://dev.gitlab.org/gitlab/gitlabhq/merge_requests/3267#note_172823 + expect do + Timeout.timeout(3.seconds) { mentionable.matches_cross_reference_regex? } + end.not_to raise_error + end +end diff --git a/spec/views/devise/shared/_signin_box.html.haml_spec.rb b/spec/views/devise/shared/_signin_box.html.haml_spec.rb index 66c064e3fba..5d521d18c70 100644 --- a/spec/views/devise/shared/_signin_box.html.haml_spec.rb +++ b/spec/views/devise/shared/_signin_box.html.haml_spec.rb @@ -7,6 +7,7 @@ describe 'devise/shared/_signin_box' do assign(:ldap_servers, []) allow(view).to receive(:current_application_settings).and_return(Gitlab::CurrentSettings.current_application_settings) allow(view).to receive(:captcha_enabled?).and_return(false) + allow(view).to receive(:captcha_on_login_required?).and_return(false) end it 'is shown when Crowd is enabled' do |