diff options
142 files changed, 2703 insertions, 371 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index ffca09a92e7..8e343c4fd39 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,62 @@ documentation](doc/development/changelog.md) for instructions on adding your own entry. +## 12.2.3 + +### Security (22 changes) + +- Ensure only authorised users can create notes on Merge Requests and Issues. +- Gitaly: ignore git redirects. +- Add :login_recaptcha_protection_enabled setting to prevent bots from brute-force attacks. +- Speed up regexp in namespace format by failing fast after reaching maximum namespace depth. +- Limit the size of issuable description and comments. +- Send TODOs for comments on commits correctly. +- Restrict MergeRequests#test_reports to authenticated users with read-access on Builds. +- Added image proxy to mitigate potential stealing of IP addresses. +- Filter out old system notes for epics in notes api endpoint response. +- Avoid exposing unaccessible repo data upon GFM post processing. +- Fix HTML injection for label description. +- Make sure HTML text is always escaped when replacing label/milestone references. +- Prevent DNS rebind on JIRA service integration. +- Use admin_group authorization in Groups::RunnersController. +- Prevent disclosure of merge request ID via email. +- Show cross-referenced MR-id in issues' activities only to authorized users. +- Enforce max chars and max render time in markdown math. +- Check permissions before responding in MergeController#pipeline_status. +- Remove EXIF from users/personal snippet uploads. +- Fix project import restricted visibility bypass via API. +- Fix weak session management by clearing password reset tokens after login (username/email) are updated. +- Fix SSRF via DNS rebinding in Kubernetes Integration. + + +## 12.2.2 + +### Security (22 changes) + +- Ensure only authorised users can create notes on Merge Requests and Issues. +- Gitaly: ignore git redirects. +- Add :login_recaptcha_protection_enabled setting to prevent bots from brute-force attacks. +- Speed up regexp in namespace format by failing fast after reaching maximum namespace depth. +- Limit the size of issuable description and comments. +- Send TODOs for comments on commits correctly. +- Restrict MergeRequests#test_reports to authenticated users with read-access on Builds. +- Added image proxy to mitigate potential stealing of IP addresses. +- Filter out old system notes for epics in notes api endpoint response. +- Avoid exposing unaccessible repo data upon GFM post processing. +- Fix HTML injection for label description. +- Make sure HTML text is always escaped when replacing label/milestone references. +- Prevent DNS rebind on JIRA service integration. +- Use admin_group authorization in Groups::RunnersController. +- Prevent disclosure of merge request ID via email. +- Show cross-referenced MR-id in issues' activities only to authorized users. +- Enforce max chars and max render time in markdown math. +- Check permissions before responding in MergeController#pipeline_status. +- Remove EXIF from users/personal snippet uploads. +- Fix project import restricted visibility bypass via API. +- Fix weak session management by clearing password reset tokens after login (username/email) are updated. +- Fix SSRF via DNS rebinding in Kubernetes Integration. + + ## 12.2.1 ### Fixed (3 changes) @@ -591,6 +647,34 @@ entry. - Removes EE differences for app/views/admin/users/show.html.haml. +## 12.0.7 + +### Security (22 changes) + +- Ensure only authorised users can create notes on Merge Requests and Issues. +- Add :login_recaptcha_protection_enabled setting to prevent bots from brute-force attacks. +- Queries for Upload should be scoped by model. +- Speed up regexp in namespace format by failing fast after reaching maximum namespace depth. +- Limit the size of issuable description and comments. +- Send TODOs for comments on commits correctly. +- Restrict MergeRequests#test_reports to authenticated users with read-access on Builds. +- Added image proxy to mitigate potential stealing of IP addresses. +- Filter out old system notes for epics in notes api endpoint response. +- Avoid exposing unaccessible repo data upon GFM post processing. +- Fix HTML injection for label description. +- Make sure HTML text is always escaped when replacing label/milestone references. +- Prevent DNS rebind on JIRA service integration. +- Use admin_group authorization in Groups::RunnersController. +- Prevent disclosure of merge request ID via email. +- Show cross-referenced MR-id in issues' activities only to authorized users. +- Enforce max chars and max render time in markdown math. +- Check permissions before responding in MergeController#pipeline_status. +- Remove EXIF from users/personal snippet uploads. +- Fix project import restricted visibility bypass via API. +- Fix weak session management by clearing password reset tokens after login (username/email) are updated. +- Fix SSRF via DNS rebinding in Kubernetes Integration. + + ## 12.0.6 - No changes. diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION index 4d5fde5bd16..91951fd8ad7 100644 --- a/GITALY_SERVER_VERSION +++ b/GITALY_SERVER_VERSION @@ -1 +1 @@ -1.60.0 +1.61.0 diff --git a/GITLAB_WORKHORSE_VERSION b/GITLAB_WORKHORSE_VERSION index 3b6825376ad..eec6dacbd48 100644 --- a/GITLAB_WORKHORSE_VERSION +++ b/GITLAB_WORKHORSE_VERSION @@ -1 +1 @@ -8.8.0 +8.8.1 @@ -399,7 +399,7 @@ gem 'mail_room', '~> 0.9.1' gem 'email_reply_trimmer', '~> 0.1' gem 'html2text' -gem 'ruby-prof', '~> 0.17.0' +gem 'ruby-prof', '~> 1.0.0' gem 'rbtrace', '~> 0.4', require: false gem 'memory_profiler', '~> 0.9', require: false gem 'benchmark-memory', '~> 0.1', require: false @@ -438,6 +438,7 @@ gem 'toml-rb', '~> 1.0.0', require: false gem 'flipper', '~> 0.13.0' gem 'flipper-active_record', '~> 0.13.0' gem 'flipper-active_support_cache_store', '~> 0.13.0' +gem 'unleash', '~> 0.1.5' # Structured logging gem 'lograge', '~> 0.5' diff --git a/Gemfile.lock b/Gemfile.lock index d9a32027f3c..a6a44cc6960 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -535,6 +535,7 @@ GEM multi_json (1.13.1) multi_xml (0.6.0) multipart-post (2.0.0) + murmurhash3 (0.1.6) mustermann (1.0.3) mustermann-grape (1.0.0) mustermann (~> 1.0.0) @@ -836,7 +837,7 @@ GEM i18n ruby-fogbugz (0.2.1) crack (~> 0.4) - ruby-prof (0.17.0) + ruby-prof (1.0.0) ruby-progressbar (1.10.1) ruby-saml (1.7.2) nokogiri (>= 1.5.10) @@ -972,6 +973,8 @@ GEM get_process_mem (~> 0) unicorn (>= 4, < 6) uniform_notifier (1.10.0) + unleash (0.1.5) + murmurhash3 (~> 0.1.6) unparser (0.4.5) abstract_type (~> 0.0.7) adamantium (~> 0.2.0) @@ -1215,7 +1218,7 @@ DEPENDENCIES rubocop-performance (~> 1.1.0) rubocop-rspec (~> 1.22.1) ruby-fogbugz (~> 0.2.1) - ruby-prof (~> 0.17.0) + ruby-prof (~> 1.0.0) ruby-progressbar ruby_parser (~> 3.8) rubyzip (~> 1.2.2) @@ -1251,6 +1254,7 @@ DEPENDENCIES unf (~> 0.1.4) unicorn (~> 5.4.1) unicorn-worker-killer (~> 0.4.4) + unleash (~> 0.1.5) validates_hostname (~> 1.0.6) version_sorter (~> 2.2.4) vmstat (~> 2.3.0) 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/assets/javascripts/monitoring/components/charts/time_series.vue b/app/assets/javascripts/monitoring/components/charts/time_series.vue index 2fdc75f63ca..02e7a7ba0a6 100644 --- a/app/assets/javascripts/monitoring/components/charts/time_series.vue +++ b/app/assets/javascripts/monitoring/components/charts/time_series.vue @@ -48,6 +48,11 @@ export default { required: false, default: false, }, + singleEmbed: { + type: Boolean, + required: false, + default: false, + }, thresholds: { type: Array, required: false, @@ -267,7 +272,10 @@ export default { </script> <template> - <div class="prometheus-graph col-12 col-lg-6" :class="[showBorder ? 'p-2' : 'p-0']"> + <div + class="prometheus-graph col-12" + :class="[showBorder ? 'p-2' : 'p-0', { 'col-lg-6': !singleEmbed }]" + > <div :class="{ 'prometheus-graph-embed w-100 p-3': showBorder }"> <div class="prometheus-graph-header"> <h5 class="prometheus-graph-title js-graph-title">{{ graphData.title }}</h5> diff --git a/app/assets/javascripts/monitoring/components/embed.vue b/app/assets/javascripts/monitoring/components/embed.vue index e3256147618..b516a82c170 100644 --- a/app/assets/javascripts/monitoring/components/embed.vue +++ b/app/assets/javascripts/monitoring/components/embed.vue @@ -2,7 +2,7 @@ import { mapActions, mapState } from 'vuex'; import { getParameterValues, removeParams } from '~/lib/utils/url_utility'; import GraphGroup from './graph_group.vue'; -import MonitorAreaChart from './charts/area.vue'; +import MonitorTimeSeriesChart from './charts/time_series.vue'; import { sidebarAnimationDuration } from '../constants'; import { getTimeDiff } from '../utils'; @@ -11,7 +11,7 @@ let sidebarMutationObserver; export default { components: { GraphGroup, - MonitorAreaChart, + MonitorTimeSeriesChart, }, props: { dashboardUrl: { @@ -92,7 +92,7 @@ export default { <template> <div class="metrics-embed" :class="{ 'd-inline-flex col-lg-6 p-0': isSingleChart }"> <div v-if="charts.length" class="row w-100 m-n2 pb-4"> - <monitor-area-chart + <monitor-time-series-chart v-for="graphData in charts" :key="graphData.title" :graph-data="graphData" diff --git a/app/assets/javascripts/monitoring/components/panel_type.vue b/app/assets/javascripts/monitoring/components/panel_type.vue index 96f62bc85ee..73ff651d510 100644 --- a/app/assets/javascripts/monitoring/components/panel_type.vue +++ b/app/assets/javascripts/monitoring/components/panel_type.vue @@ -10,14 +10,14 @@ import { GlTooltipDirective, } from '@gitlab/ui'; import Icon from '~/vue_shared/components/icon.vue'; -import MonitorAreaChart from './charts/area.vue'; +import MonitorTimeSeriesChart from './charts/time_series.vue'; import MonitorSingleStatChart from './charts/single_stat.vue'; import MonitorEmptyChart from './charts/empty_chart.vue'; export default { components: { - MonitorAreaChart, MonitorSingleStatChart, + MonitorTimeSeriesChart, MonitorEmptyChart, Icon, GlDropdown, @@ -92,7 +92,7 @@ export default { v-if="isPanelType('single-stat') && graphDataHasMetrics" :graph-data="graphData" /> - <monitor-area-chart + <monitor-time-series-chart v-else-if="graphDataHasMetrics" :graph-data="graphData" :deployment-data="deploymentData" @@ -136,6 +136,6 @@ export default { </gl-dropdown-item> </gl-dropdown> </div> - </monitor-area-chart> + </monitor-time-series-chart> <monitor-empty-chart v-else :graph-title="graphData.title" /> </template> diff --git a/app/controllers/concerns/issuable_actions.rb b/app/controllers/concerns/issuable_actions.rb index b86e4451a7e..1d6a1be402c 100644 --- a/app/controllers/concerns/issuable_actions.rb +++ b/app/controllers/concerns/issuable_actions.rb @@ -110,7 +110,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 d2a961efff7..80be7095ed3 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 f4cc0a5851b..75d4a24b559 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/projects/pipelines_controller.rb b/app/controllers/projects/pipelines_controller.rb index e6e3a440925..499d4918899 100644 --- a/app/controllers/projects/pipelines_controller.rb +++ b/app/controllers/projects/pipelines_controller.rb @@ -180,7 +180,7 @@ class Projects::PipelinesController < Projects::ApplicationController else project .all_pipelines - .includes(user: :status) + .includes(builds: :tags, user: :status) .find_by!(id: params[:id]) .present(current_user: current_user) end diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 1880bead3ee..7b682cc0cc5 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -21,10 +21,13 @@ class SessionsController < Devise::SessionsController prepend_before_action :ensure_password_authentication_enabled!, if: -> { action_name == 'create' && password_based_login? } 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, if: -> { action_name == 'new' && failed_login? } - helper_method :captcha_enabled? + after_action :log_failed_login, if: :action_new_and_failed_login? + + helper_method :captcha_enabled?, :captcha_on_login_required? # protect_from_forgery is already prepended in ApplicationController but # authenticate_with_two_factor which signs in the user is prepended before @@ -38,6 +41,7 @@ class SessionsController < Devise::SessionsController protect_from_forgery with: :exception, prepend: true CAPTCHA_HEADER = 'X-GitLab-Show-Login-Captcha'.freeze + MAX_FAILED_LOGIN_ATTEMPTS = 5 def new set_minimum_password_length @@ -81,10 +85,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 @@ -126,10 +134,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 @@ -240,6 +266,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 c2d7350ce12..84021d0da56 100644 --- a/app/helpers/application_settings_helper.rb +++ b/app/helpers/application_settings_helper.rb @@ -235,6 +235,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/emails_helper.rb b/app/helpers/emails_helper.rb index 36122d3a22a..23596769738 100644 --- a/app/helpers/emails_helper.rb +++ b/app/helpers/emails_helper.rb @@ -90,6 +90,8 @@ module EmailsHelper when MergeRequest merge_request = MergeRequest.find(closed_via[:id]).present + return "" unless Ability.allowed?(@recipient, :read_merge_request, merge_request) + case format when :html merge_request_link = link_to(merge_request.to_reference, merge_request.web_url) @@ -102,6 +104,8 @@ module EmailsHelper # Technically speaking this should be Commit but per # https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/15610#note_163812339 # we can't deserialize Commit without custom serializer for ActiveJob + return "" unless Ability.allowed?(@recipient, :download_code, @project) + _("via %{closed_via}") % { closed_via: closed_via } else "" diff --git a/app/helpers/labels_helper.rb b/app/helpers/labels_helper.rb index 2ed016beea4..c5a3507637e 100644 --- a/app/helpers/labels_helper.rb +++ b/app/helpers/labels_helper.rb @@ -71,7 +71,7 @@ module LabelsHelper end def label_tooltip_title(label) - label.description + Sanitize.clean(label.description) end def suggested_colors diff --git a/app/mailers/emails/issues.rb b/app/mailers/emails/issues.rb index f3a3203f7ad..0bba2a8bf24 100644 --- a/app/mailers/emails/issues.rb +++ b/app/mailers/emails/issues.rb @@ -34,6 +34,8 @@ module Emails setup_issue_mail(issue_id, recipient_id, closed_via: closed_via) @updated_by = User.find(updated_by_user_id) + @recipient = User.find(recipient_id) + mail_answer_thread(@issue, issue_thread_options(updated_by_user_id, recipient_id, reason)) end diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index 92004b92647..d6caf092ed0 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -82,11 +82,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, @@ -310,4 +310,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 a6dd9986e23..f402c0e2775 100644 --- a/app/models/application_setting_implementation.rb +++ b/app/models/application_setting_implementation.rb @@ -71,6 +71,7 @@ module ApplicationSettingImplementation protected_ci_variables: false, raw_blob_request_limit: 300, recaptcha_enabled: false, + login_recaptcha_protection_enabled: false, repository_checks_enabled: true, repository_storages: ['default'], require_two_factor_authentication: false, diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index 0a943a33bbb..64e372878e6 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 db46d7afbb9..eefe9f00836 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/group.rb b/app/models/group.rb index 6c868b1d1f0..61a4802a6ee 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -365,6 +365,8 @@ class Group < Namespace end def max_member_access_for_user(user) + return GroupMember::NO_ACCESS unless user + return GroupMember::OWNER if user.admin? members_with_parents diff --git a/app/models/label.rb b/app/models/label.rb index d9455b36242..dc9f0a3d1a9 100644 --- a/app/models/label.rb +++ b/app/models/label.rb @@ -199,7 +199,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 ## @@ -260,7 +264,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 a12d1eb7243..3956ec192b1 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -89,6 +89,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 @@ -331,6 +332,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 d08fcd8954d..0728c83005e 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 9a2640db9ca..a19755d286a 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 6131a8dc710..909f5f3873d 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -643,6 +643,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/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 0b1d3d1ddb3..6e9efcb0597 100644 --- a/app/views/devise/sessions/_new_base.html.haml +++ b/app/views/devise/sessions/_new_base.html.haml @@ -16,7 +16,7 @@ - else = 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/app/views/projects/serverless/functions/index.html.haml b/app/views/projects/serverless/functions/index.html.haml index 9c69aedfbfc..bac6c76684b 100644 --- a/app/views/projects/serverless/functions/index.html.haml +++ b/app/views/projects/serverless/functions/index.html.haml @@ -14,5 +14,5 @@ .js-serverless-functions-notice .flash-container - .top-area.adjust + .top-area.adjust.d-flex.justify-content-center .serverless-functions-table#js-serverless-functions diff --git a/changelogs/unreleased/64764-fix-serverless-layout.yml b/changelogs/unreleased/64764-fix-serverless-layout.yml new file mode 100644 index 00000000000..9717062df93 --- /dev/null +++ b/changelogs/unreleased/64764-fix-serverless-layout.yml @@ -0,0 +1,5 @@ +--- +title: Fix serverless entry page layout +merge_request: 32029 +author: +type: fixed diff --git a/changelogs/unreleased/66073-use-time-series-chart-instead-of-area-chart-in-panel_types.yml b/changelogs/unreleased/66073-use-time-series-chart-instead-of-area-chart-in-panel_types.yml new file mode 100644 index 00000000000..8d7af96a7d8 --- /dev/null +++ b/changelogs/unreleased/66073-use-time-series-chart-instead-of-area-chart-in-panel_types.yml @@ -0,0 +1,5 @@ +--- +title: Enable line charts in dashbaord panels and embedded charts +merge_request: 31920 +author: +type: added 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-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-exposed-default-branch.yml b/changelogs/unreleased/security-exposed-default-branch.yml new file mode 100644 index 00000000000..bf32617ee8a --- /dev/null +++ b/changelogs/unreleased/security-exposed-default-branch.yml @@ -0,0 +1,5 @@ +--- +title: Avoid exposing unaccessible repo data upon GFM post processing +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-gitaly-1-61-0.yml b/changelogs/unreleased/security-gitaly-1-61-0.yml new file mode 100644 index 00000000000..cbcd5f545a0 --- /dev/null +++ b/changelogs/unreleased/security-gitaly-1-61-0.yml @@ -0,0 +1,5 @@ +--- +title: "Gitaly: ignore git redirects" +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-hide_merge_request_ids_on_emails.yml b/changelogs/unreleased/security-hide_merge_request_ids_on_emails.yml new file mode 100644 index 00000000000..cd8c9590a70 --- /dev/null +++ b/changelogs/unreleased/security-hide_merge_request_ids_on_emails.yml @@ -0,0 +1,5 @@ +--- +title: Prevent disclosure of merge request ID via email +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-master.yml b/changelogs/unreleased/security-katex-dos-master.yml new file mode 100644 index 00000000000..df803a5eafd --- /dev/null +++ b/changelogs/unreleased/security-katex-dos-master.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/rest-client-hostname_override.rb b/config/initializers/rest-client-hostname_override.rb new file mode 100644 index 00000000000..80b123ebe61 --- /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_web_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/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/20190726101050_rename_allow_local_requests_from_hooks_and_services_application_setting.rb b/db/migrate/20190726101050_rename_allow_local_requests_from_hooks_and_services_application_setting.rb index ac65e8d745c..cce8942128c 100644 --- a/db/migrate/20190726101050_rename_allow_local_requests_from_hooks_and_services_application_setting.rb +++ b/db/migrate/20190726101050_rename_allow_local_requests_from_hooks_and_services_application_setting.rb @@ -12,6 +12,6 @@ class RenameAllowLocalRequestsFromHooksAndServicesApplicationSetting < ActiveRec end def down - cleanup_concurrent_column_rename :application_settings, :allow_local_requests_from_web_hooks_and_services, :allow_local_requests_from_hooks_and_services + undo_rename_column_concurrently :application_settings, :allow_local_requests_from_hooks_and_services, :allow_local_requests_from_web_hooks_and_services 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/post_migrate/20190801114109_cleanup_allow_local_requests_from_hooks_and_services_application_setting_rename.rb b/db/post_migrate/20190801114109_cleanup_allow_local_requests_from_hooks_and_services_application_setting_rename.rb index 127e44254ac..cb86f843f9c 100644 --- a/db/post_migrate/20190801114109_cleanup_allow_local_requests_from_hooks_and_services_application_setting_rename.rb +++ b/db/post_migrate/20190801114109_cleanup_allow_local_requests_from_hooks_and_services_application_setting_rename.rb @@ -12,6 +12,6 @@ class CleanupAllowLocalRequestsFromHooksAndServicesApplicationSettingRename < Ac end def down - rename_column_concurrently :application_settings, :allow_local_requests_from_web_hooks_and_services, :allow_local_requests_from_hooks_and_services + undo_cleanup_concurrent_column_rename :application_settings, :allow_local_requests_from_hooks_and_services, :allow_local_requests_from_web_hooks_and_services end end diff --git a/db/schema.rb b/db/schema.rb index 7e5e96707dc..290ad421c04 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_08_15_093949) 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" @@ -272,6 +272,7 @@ ActiveRecord::Schema.define(version: 2019_08_15_093949) 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 "login_recaptcha_protection_enabled", default: false, null: false t.string "outbound_local_requests_whitelist", limit: 255, default: [], null: false, array: true t.integer "raw_blob_request_limit", default: 300, null: false t.boolean "allow_local_requests_from_web_hooks_and_services", default: false, null: false @@ -2505,6 +2506,7 @@ ActiveRecord::Schema.define(version: 2019_08_15_093949) 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" 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 87ae0c48199..08eb84bfb63 100644 --- a/doc/api/epics.md +++ b/doc/api/epics.md @@ -165,7 +165,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) | @@ -231,7 +231,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 8313dd2c3bd..1b0279154c0 100644 --- a/doc/api/issues.md +++ b/doc/api/issues.md @@ -591,7 +591,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 | @@ -692,7 +692,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 49ed4968b0d..0d030ef30c8 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/lib/api/discussions.rb b/lib/api/discussions.rb index 6c1acc3963f..9125207167c 100644 --- a/lib/api/discussions.rb +++ b/lib/api/discussions.rb @@ -239,7 +239,7 @@ module API # because notes are redacted if they point to projects that # cannot be accessed by the user. notes = prepare_notes_for_rendering(notes) - 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/helpers/notes_helpers.rb b/lib/api/helpers/notes_helpers.rb index b2bf6bf7417..f445834323d 100644 --- a/lib/api/helpers/notes_helpers.rb +++ b/lib/api/helpers/notes_helpers.rb @@ -12,7 +12,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 @@ -61,8 +61,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 diff --git a/lib/api/notes.rb b/lib/api/notes.rb index 84563d66ee8..16fca9acccb 100644 --- a/lib/api/notes.rb +++ b/lib/api/notes.rb @@ -42,7 +42,7 @@ module API # array returned, but this is really a edge-case. notes = paginate(raw_notes) notes = prepare_notes_for_rendering(notes) - notes = notes.reject { |n| n.cross_reference_not_visible_for?(current_user) } + notes = notes.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 b67c18b5465..dd27ebab83d 100644 --- a/lib/api/settings.rb +++ b/lib/api/settings.rb @@ -108,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' diff --git a/lib/banzai/filter/abstract_reference_filter.rb b/lib/banzai/filter/abstract_reference_filter.rb index 52af28ce8ec..a0439089879 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 @@ -389,6 +397,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/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/relative_link_filter.rb b/lib/banzai/filter/relative_link_filter.rb index 86f18679496..846a7d46aad 100644 --- a/lib/banzai/filter/relative_link_filter.rb +++ b/lib/banzai/filter/relative_link_filter.rb @@ -9,6 +9,7 @@ module Banzai # Context options: # :commit # :group + # :current_user # :project # :project_wiki # :ref @@ -18,6 +19,7 @@ module Banzai def call return doc if context[:system_note] + return doc unless visible_to_user? @uri_types = {} clear_memoization(:linkable_files) @@ -166,6 +168,16 @@ module Banzai Gitlab.config.gitlab.relative_url_root.presence || '/' end + def visible_to_user? + if project + Ability.allowed?(current_user, :download_code, project) + elsif group + Ability.allowed?(current_user, :read_group, group) + else # Objects detached from projects or groups, e.g. Personal Snippets. + true + end + end + def ref context[:ref] || project.default_branch end @@ -178,6 +190,10 @@ module Banzai context[:project] end + def current_user + context[:current_user] + end + def repository @repository ||= project&.repository end 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 cbdff0ab060..707466426db 100644 --- a/lib/gitlab/database.rb +++ b/lib/gitlab/database.rb @@ -13,6 +13,10 @@ module Gitlab # FIXME: this should just be the max value of timestampz 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/database/migration_helpers.rb b/lib/gitlab/database/migration_helpers.rb index 8bc45f6e78c..57a413f8e04 100644 --- a/lib/gitlab/database/migration_helpers.rb +++ b/lib/gitlab/database/migration_helpers.rb @@ -470,7 +470,7 @@ module Gitlab # We set the default value _after_ adding the column so we don't end up # updating any existing data with the default value. This isn't # necessary since we copy over old values further down. - change_column_default(table, new, old_col.default) if old_col.default + change_column_default(table, new, old_col.default) unless old_col.default.nil? install_rename_triggers(table, old, new) @@ -482,6 +482,16 @@ module Gitlab copy_foreign_keys(table, old, new) end + def undo_rename_column_concurrently(table, old, new) + trigger_name = rename_trigger_name(table, old, new) + + check_trigger_permissions!(table) + + remove_rename_triggers_for_postgresql(table, trigger_name) + + remove_column(table, new) + end + # Installs triggers in a table that keep a new column in sync with an old # one. # @@ -547,6 +557,35 @@ module Gitlab remove_column(table, old) end + def undo_cleanup_concurrent_column_rename(table, old, new, type: nil) + if transaction_open? + raise 'undo_cleanup_concurrent_column_rename can not be run inside a transaction' + end + + check_trigger_permissions!(table) + + new_column = column_for(table, new) + + add_column(table, old, type || new_column.type, + limit: new_column.limit, + precision: new_column.precision, + scale: new_column.scale) + + # We set the default value _after_ adding the column so we don't end up + # updating any existing data with the default value. This isn't + # necessary since we copy over old values further down. + change_column_default(table, old, new_column.default) unless new_column.default.nil? + + install_rename_triggers(table, old, new) + + update_column_in_batches(table, old, Arel::Table.new(table)[new]) + + change_column_null(table, old, false) unless new_column.null + + copy_indexes(table, new, old) + copy_foreign_keys(table, new, old) + end + # Changes the column type of a table using a background migration. # # Because this method uses a background migration it's more suitable for 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 f96466b2b00..d9c28ff1181 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 61642fbbd59..c5f015c8460 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -1101,9 +1101,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 "" @@ -4324,7 +4321,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" @@ -5694,7 +5691,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" @@ -13676,6 +13676,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/package.json b/package.json index 384584a944a..e076f9e6664 100644 --- a/package.json +++ b/package.json @@ -37,7 +37,7 @@ "@babel/plugin-syntax-import-meta": "^7.2.0", "@babel/preset-env": "^7.5.5", "@gitlab/csslab": "^1.9.0", - "@gitlab/svgs": "^1.68.0", + "@gitlab/svgs": "^1.70.0", "@gitlab/ui": "5.18.0", "@gitlab/visual-review-tools": "^1.0.0", "apollo-cache-inmemory": "^1.5.1", 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 11b1eaf11b7..4b9b6913997 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 @@ -1052,17 +1096,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 @@ -1070,7 +1136,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 4500c412521..4db77921f24 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 563b61962cf..180d997a8e8 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/factories/ci/job_artifacts.rb b/spec/factories/ci/job_artifacts.rb index 2d68a8e9fe3..6f553cadfa3 100644 --- a/spec/factories/ci/job_artifacts.rb +++ b/spec/factories/ci/job_artifacts.rb @@ -8,6 +8,10 @@ FactoryBot.define do file_type :archive file_format :zip + trait :expired do + expire_at { Date.yesterday } + end + trait :remote_store do file_store JobArtifactUploader::Store::REMOTE end diff --git a/spec/factories/sequences.rb b/spec/factories/sequences.rb index b6f2d6d8389..17b54d69372 100644 --- a/spec/factories/sequences.rb +++ b/spec/factories/sequences.rb @@ -7,7 +7,7 @@ FactoryBot.define do sequence(:email_alias) { |n| "user.alias#{n}@example.org" } sequence(:title) { |n| "My title #{n}" } sequence(:filename) { |n| "filename-#{n}.rb" } - sequence(:url) { |n| "http://example#{n}.org" } + sequence(:url) { |n| "http://example#{n}.test" } sequence(:label_title) { |n| "label#{n}" } sequence(:branch) { |n| "my-branch-#{n}" } sequence(:past_time) { |n| 4.hours.ago + (2 * n).seconds } diff --git a/spec/features/admin/admin_settings_spec.rb b/spec/features/admin/admin_settings_spec.rb index ddd87404003..eb59de2e132 100644 --- a/spec/features/admin/admin_settings_spec.rb +++ b/spec/features/admin/admin_settings_spec.rb @@ -263,6 +263,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 @@ -271,6 +272,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 68d99b4241a..76eef66c517 100644 --- a/spec/features/markdown/math_spec.rb +++ b/spec/features/markdown/math_spec.rb @@ -34,7 +34,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 1ab7742b36e..0905ab0aef8 100644 --- a/spec/features/profiles/user_edit_profile_spec.rb +++ b/spec/features/profiles/user_edit_profile_spec.rb @@ -49,6 +49,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/frontend/monitoring/embed/embed_spec.js b/spec/frontend/monitoring/embed/embed_spec.js index 3b18a0f77c7..1ce14e2418a 100644 --- a/spec/frontend/monitoring/embed/embed_spec.js +++ b/spec/frontend/monitoring/embed/embed_spec.js @@ -1,7 +1,7 @@ import { createLocalVue, shallowMount } from '@vue/test-utils'; import Vuex from 'vuex'; import Embed from '~/monitoring/components/embed.vue'; -import MonitorAreaChart from '~/monitoring/components/charts/area.vue'; +import MonitorTimeSeriesChart from '~/monitoring/components/charts/time_series.vue'; import { TEST_HOST } from 'helpers/test_constants'; import { groups, initialState, metricsData, metricsWithData } from './mock_data'; @@ -55,7 +55,7 @@ describe('Embed', () => { it('shows an empty state when no metrics are present', () => { expect(wrapper.find('.metrics-embed').exists()).toBe(true); - expect(wrapper.find(MonitorAreaChart).exists()).toBe(false); + expect(wrapper.find(MonitorTimeSeriesChart).exists()).toBe(false); }); }); @@ -71,8 +71,8 @@ describe('Embed', () => { it('shows a chart when metrics are present', () => { wrapper.setProps({}); expect(wrapper.find('.metrics-embed').exists()).toBe(true); - expect(wrapper.find(MonitorAreaChart).exists()).toBe(true); - expect(wrapper.findAll(MonitorAreaChart).length).toBe(2); + expect(wrapper.find(MonitorTimeSeriesChart).exists()).toBe(true); + expect(wrapper.findAll(MonitorTimeSeriesChart).length).toBe(2); }); }); }); diff --git a/spec/helpers/emails_helper_spec.rb b/spec/helpers/emails_helper_spec.rb index d25f0c6de4a..a14ae2cde4b 100644 --- a/spec/helpers/emails_helper_spec.rb +++ b/spec/helpers/emails_helper_spec.rb @@ -6,30 +6,62 @@ describe EmailsHelper do let(:merge_request) { create(:merge_request) } let(:merge_request_presenter) { merge_request.present } - context "and format is text" do - it "returns plain text" do - expect(closure_reason_text(merge_request, format: :text)).to eq("via merge request #{merge_request.to_reference} (#{merge_request_presenter.web_url})") + context 'when user can read merge request' do + let(:user) { create(:user) } + + before do + merge_request.project.add_developer(user) + self.instance_variable_set(:@recipient, user) + self.instance_variable_set(:@project, merge_request.project) + end + + context "and format is text" do + it "returns plain text" do + expect(helper.closure_reason_text(merge_request, format: :text)).to eq("via merge request #{merge_request.to_reference} (#{merge_request_presenter.web_url})") + end end - end - context "and format is HTML" do - it "returns HTML" do - expect(closure_reason_text(merge_request, format: :html)).to eq("via merge request #{link_to(merge_request.to_reference, merge_request_presenter.web_url)}") + context "and format is HTML" do + it "returns HTML" do + expect(helper.closure_reason_text(merge_request, format: :html)).to eq("via merge request #{link_to(merge_request.to_reference, merge_request_presenter.web_url)}") + end + end + + context "and format is unknown" do + it "returns plain text" do + expect(helper.closure_reason_text(merge_request, format: :text)).to eq("via merge request #{merge_request.to_reference} (#{merge_request_presenter.web_url})") + end end end - context "and format is unknown" do - it "returns plain text" do - expect(closure_reason_text(merge_request, format: :text)).to eq("via merge request #{merge_request.to_reference} (#{merge_request_presenter.web_url})") + context 'when user cannot read merge request' do + it "does not have link to merge request" do + expect(helper.closure_reason_text(merge_request)).to be_empty end end end context 'when given a String' do + let(:user) { create(:user) } + let(:project) { create(:project) } let(:closed_via) { "5a0eb6fd7e0f133044378c662fcbbc0d0c16dbfa" } - it "returns plain text" do - expect(closure_reason_text(closed_via)).to eq("via #{closed_via}") + context 'when user can read commits' do + before do + project.add_developer(user) + self.instance_variable_set(:@recipient, user) + self.instance_variable_set(:@project, project) + end + + it "returns plain text" do + expect(closure_reason_text(closed_via)).to eq("via #{closed_via}") + end + end + + context 'when user cannot read commits' do + it "returns plain text" do + expect(closure_reason_text(closed_via)).to be_empty + end end end diff --git a/spec/helpers/labels_helper_spec.rb b/spec/helpers/labels_helper_spec.rb index 4f1cab38f34..1d57aaa0da5 100644 --- a/spec/helpers/labels_helper_spec.rb +++ b/spec/helpers/labels_helper_spec.rb @@ -278,4 +278,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/helpers/markup_helper_spec.rb b/spec/helpers/markup_helper_spec.rb index f6e1720e113..1757ec8fa4d 100644 --- a/spec/helpers/markup_helper_spec.rb +++ b/spec/helpers/markup_helper_spec.rb @@ -65,6 +65,9 @@ describe MarkupHelper do describe 'inside a group' do before do + # Ensure the generated reference links aren't redacted + group.add_maintainer(user) + helper.instance_variable_set(:@group, group) helper.instance_variable_set(:@project, nil) end @@ -78,6 +81,9 @@ describe MarkupHelper do let(:project_in_group) { create(:project, group: group) } before do + # Ensure the generated reference links aren't redacted + project_in_group.add_maintainer(user) + helper.instance_variable_set(:@group, group) helper.instance_variable_set(:@project, project_in_group) 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..3707e001d41 --- /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_web_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/javascripts/monitoring/panel_type_spec.js b/spec/javascripts/monitoring/panel_type_spec.js index 086be628093..a2366e74d43 100644 --- a/spec/javascripts/monitoring/panel_type_spec.js +++ b/spec/javascripts/monitoring/panel_type_spec.js @@ -1,7 +1,7 @@ import { shallowMount } from '@vue/test-utils'; import PanelType from '~/monitoring/components/panel_type.vue'; import EmptyChart from '~/monitoring/components/charts/empty_chart.vue'; -import AreaChart from '~/monitoring/components/charts/area.vue'; +import TimeSeriesChart from '~/monitoring/components/charts/time_series.vue'; import { graphDataPrometheusQueryRange } from './mock_data'; import { createStore } from '~/monitoring/stores'; @@ -62,9 +62,10 @@ describe('Panel Type component', () => { }); }); - describe('Area Chart panel type', () => { + describe('Time Series Chart panel type', () => { it('is rendered', () => { - expect(panelType.find(AreaChart).exists()).toBe(true); + expect(panelType.find(TimeSeriesChart).isVueInstance()).toBe(true); + expect(panelType.find(TimeSeriesChart).exists()).toBe(true); }); it('sets clipboard text on the dropdown', () => { diff --git a/spec/lib/banzai/filter/label_reference_filter_spec.rb b/spec/lib/banzai/filter/label_reference_filter_spec.rb index 213a5459118..35e99d2586e 100644 --- a/spec/lib/banzai/filter/label_reference_filter_spec.rb +++ b/spec/lib/banzai/filter/label_reference_filter_spec.rb @@ -10,6 +10,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 3f021adc756..ab0c2c383c5 100644 --- a/spec/lib/banzai/filter/milestone_reference_filter_spec.rb +++ b/spec/lib/banzai/filter/milestone_reference_filter_spec.rb @@ -329,6 +329,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 @@ -340,6 +344,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/relative_link_filter_spec.rb b/spec/lib/banzai/filter/relative_link_filter_spec.rb index ecb83b6cb66..789530fbc56 100644 --- a/spec/lib/banzai/filter/relative_link_filter_spec.rb +++ b/spec/lib/banzai/filter/relative_link_filter_spec.rb @@ -7,6 +7,7 @@ describe Banzai::Filter::RelativeLinkFilter do contexts.reverse_merge!({ commit: commit, project: project, + current_user: user, group: group, project_wiki: project_wiki, ref: ref, @@ -33,7 +34,8 @@ describe Banzai::Filter::RelativeLinkFilter do %(<div>#{element}</div>) end - let(:project) { create(:project, :repository) } + let(:project) { create(:project, :repository, :public) } + let(:user) { create(:user) } let(:group) { nil } let(:project_path) { project.full_path } let(:ref) { 'markdown' } @@ -75,6 +77,11 @@ describe Banzai::Filter::RelativeLinkFilter do include_examples :preserve_unchanged end + context 'without project repository access' do + let(:project) { create(:project, :repository, repository_access_level: ProjectFeature::PRIVATE) } + include_examples :preserve_unchanged + end + it 'does not raise an exception on invalid URIs' do act = link("://foo") expect { filter(act) }.not_to raise_error @@ -282,6 +289,37 @@ describe Banzai::Filter::RelativeLinkFilter do let(:relative_path) { "/#{project.full_path}#{upload_path}" } context 'to a project upload' do + context 'without project repository access' do + let(:project) { create(:project, :repository, repository_access_level: ProjectFeature::PRIVATE) } + + it 'does not rebuild relative URL for a link' do + doc = filter(link(upload_path)) + expect(doc.at_css('a')['href']).to eq(upload_path) + + doc = filter(nested(link(upload_path))) + expect(doc.at_css('a')['href']).to eq(upload_path) + end + + it 'does not rebuild relative URL for an image' do + doc = filter(image(upload_path)) + expect(doc.at_css('img')['src']).to eq(upload_path) + + doc = filter(nested(image(upload_path))) + expect(doc.at_css('img')['src']).to eq(upload_path) + end + + context 'with an absolute URL' do + let(:absolute_path) { Gitlab.config.gitlab.url + relative_path } + let(:only_path) { false } + + it 'does not rewrite the link' do + doc = filter(link(upload_path)) + + expect(doc.at_css('a')['href']).to eq(upload_path) + end + end + end + context 'with an absolute URL' do let(:absolute_path) { Gitlab.config.gitlab.url + relative_path } let(:only_path) { false } @@ -331,11 +369,41 @@ describe Banzai::Filter::RelativeLinkFilter do end context 'to a group upload' do - let(:upload_link) { link('/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg') } + let(:upload_path) { '/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg' } + let(:upload_link) { link(upload_path) } let(:group) { create(:group) } let(:project) { nil } let(:relative_path) { "/groups/#{group.full_path}/-/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg" } + context 'without group read access' do + let(:group) { create(:group, :private) } + + it 'does not rewrite the link' do + doc = filter(upload_link) + + expect(doc.at_css('a')['href']).to eq(upload_path) + end + + it 'does not rewrite the link for subgroup' do + group.update!(parent: create(:group)) + + doc = filter(upload_link) + + expect(doc.at_css('a')['href']).to eq(upload_path) + end + + context 'with an absolute URL' do + let(:absolute_path) { Gitlab.config.gitlab.url + relative_path } + let(:only_path) { false } + + it 'does not rewrite the link' do + doc = filter(upload_link) + + expect(doc.at_css('a')['href']).to eq(upload_path) + end + end + end + context 'with an absolute URL' do let(:absolute_path) { Gitlab.config.gitlab.url + relative_path } let(:only_path) { false } 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/database/migration_helpers_spec.rb b/spec/lib/gitlab/database/migration_helpers_spec.rb index fe44b09aa24..cff4eb398bf 100644 --- a/spec/lib/gitlab/database/migration_helpers_spec.rb +++ b/spec/lib/gitlab/database/migration_helpers_spec.rb @@ -576,6 +576,38 @@ describe Gitlab::Database::MigrationHelpers do model.rename_column_concurrently(:users, :old, :new) end + + context 'when default is false' do + let(:old_column) do + double(:column, + type: :boolean, + limit: nil, + default: false, + null: false, + precision: nil, + scale: nil) + end + + it 'copies the default to the new column' do + expect(model).to receive(:change_column_default) + .with(:users, :new, old_column.default) + + model.rename_column_concurrently(:users, :old, :new) + end + end + end + end + + describe '#undo_rename_column_concurrently' do + it 'reverses the operations of rename_column_concurrently' do + expect(model).to receive(:check_trigger_permissions!).with(:users) + + expect(model).to receive(:remove_rename_triggers_for_postgresql) + .with(:users, /trigger_.{12}/) + + expect(model).to receive(:remove_column).with(:users, :new) + + model.undo_rename_column_concurrently(:users, :old, :new) end end @@ -592,6 +624,80 @@ describe Gitlab::Database::MigrationHelpers do end end + describe '#undo_cleanup_concurrent_column_rename' do + context 'in a transaction' do + it 'raises RuntimeError' do + allow(model).to receive(:transaction_open?).and_return(true) + + expect { model.undo_cleanup_concurrent_column_rename(:users, :old, :new) } + .to raise_error(RuntimeError) + end + end + + context 'outside a transaction' do + let(:new_column) do + double(:column, + type: :integer, + limit: 8, + default: 0, + null: false, + precision: 5, + scale: 1) + end + + let(:trigger_name) { model.rename_trigger_name(:users, :old, :new) } + + before do + allow(model).to receive(:transaction_open?).and_return(false) + allow(model).to receive(:column_for).and_return(new_column) + end + + it 'reverses the operations of cleanup_concurrent_column_rename' do + expect(model).to receive(:check_trigger_permissions!).with(:users) + + expect(model).to receive(:install_rename_triggers_for_postgresql) + .with(trigger_name, '"users"', '"old"', '"new"') + + expect(model).to receive(:add_column) + .with(:users, :old, :integer, + limit: new_column.limit, + precision: new_column.precision, + scale: new_column.scale) + + expect(model).to receive(:change_column_default) + .with(:users, :old, new_column.default) + + expect(model).to receive(:update_column_in_batches) + + expect(model).to receive(:change_column_null).with(:users, :old, false) + + expect(model).to receive(:copy_indexes).with(:users, :new, :old) + expect(model).to receive(:copy_foreign_keys).with(:users, :new, :old) + + model.undo_cleanup_concurrent_column_rename(:users, :old, :new) + end + + context 'when default is false' do + let(:new_column) do + double(:column, + type: :boolean, + limit: nil, + default: false, + null: false, + precision: nil, + scale: nil) + end + + it 'copies the default to the old column' do + expect(model).to receive(:change_column_default) + .with(:users, :old, new_column.default) + + model.undo_cleanup_concurrent_column_rename(:users, :old, :new) + end + end + end + end + describe '#change_column_type_concurrently' do it 'changes the column type' do expect(model).to receive(:rename_column_concurrently) diff --git a/spec/lib/gitlab/kubernetes/kube_client_spec.rb b/spec/lib/gitlab/kubernetes/kube_client_spec.rb index f49d4e23e39..e5d688aa391 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 22c5f27dc6d..f882dbbdb5c 100644 --- a/spec/lib/gitlab/sanitizers/exif_spec.rb +++ b/spec/lib/gitlab/sanitizers/exif_spec.rb @@ -7,7 +7,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 @@ -18,7 +20,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/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index 78be4a8131a..7d84d094bdf 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -4,6 +4,7 @@ require 'spec_helper' describe Ci::Pipeline, :mailer do include ProjectForksHelper + include StubRequests let(:user) { create(:user) } set(:project) { create(:project) } @@ -2504,7 +2505,7 @@ describe Ci::Pipeline, :mailer do let(:enabled) { true } before do - WebMock.stub_request(:post, hook.url) + stub_full_request(hook.url, method: :post) end context 'with multiple builds' do @@ -2558,7 +2559,7 @@ describe Ci::Pipeline, :mailer do end def have_requested_pipeline_hook(status) - have_requested(:post, hook.url).with do |req| + have_requested(:post, stubbed_hostname(hook.url)).with do |req| json_body = JSON.parse(req.body) json_body['object_attributes']['status'] == status && json_body['builds'].length == 2 diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb index 39680c0e51a..65d41edc035 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 @@ -795,4 +796,29 @@ 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 end diff --git a/spec/models/label_spec.rb b/spec/models/label_spec.rb index c2e2298823e..baf2cfeab0c 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 bfd0e5f0558..927fbdb93d8 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 46b86e8393d..1a641c868d9 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -3045,6 +3045,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/services/issues/close_service_spec.rb b/spec/services/issues/close_service_spec.rb index 6874a8a0929..642a49d57d5 100644 --- a/spec/services/issues/close_service_spec.rb +++ b/spec/services/issues/close_service_spec.rb @@ -60,35 +60,63 @@ describe Issues::CloseService do describe '#close_issue' do context "closed by a merge request" do - before do + it 'mentions closure via a merge request' do perform_enqueued_jobs do described_class.new(project, user).close_issue(issue, closed_via: closing_merge_request) end - end - it 'mentions closure via a merge request' do email = ActionMailer::Base.deliveries.last expect(email.to.first).to eq(user2.email) expect(email.subject).to include(issue.title) expect(email.body.parts.map(&:body)).to all(include(closing_merge_request.to_reference)) end + + context 'when user cannot read merge request' do + it 'does not mention merge request' do + project.project_feature.update_attribute(:repository_access_level, ProjectFeature::DISABLED) + perform_enqueued_jobs do + described_class.new(project, user).close_issue(issue, closed_via: closing_merge_request) + end + + email = ActionMailer::Base.deliveries.last + body_text = email.body.parts.map(&:body).join(" ") + + expect(email.to.first).to eq(user2.email) + expect(email.subject).to include(issue.title) + expect(body_text).not_to include(closing_merge_request.to_reference) + end + end end context "closed by a commit" do - before do + it 'mentions closure via a commit' do perform_enqueued_jobs do described_class.new(project, user).close_issue(issue, closed_via: closing_commit) end - end - it 'mentions closure via a commit' do email = ActionMailer::Base.deliveries.last expect(email.to.first).to eq(user2.email) expect(email.subject).to include(issue.title) expect(email.body.parts.map(&:body)).to all(include(closing_commit.id)) end + + context 'when user cannot read the commit' do + it 'does not mention the commit id' do + project.project_feature.update_attribute(:repository_access_level, ProjectFeature::DISABLED) + perform_enqueued_jobs do + described_class.new(project, user).close_issue(issue, closed_via: closing_commit) + end + + email = ActionMailer::Base.deliveries.last + body_text = email.body.parts.map(&:body).join(" ") + + expect(email.to.first).to eq(user2.email) + expect(email.subject).to include(issue.title) + expect(body_text).not_to include(closing_commit.id) + end + end end context "valid params" do diff --git a/spec/services/projects/create_service_spec.rb b/spec/services/projects/create_service_spec.rb index d4fa62fa85d..8178b7d2ba2 100644 --- a/spec/services/projects/create_service_spec.rb +++ b/spec/services/projects/create_service_spec.rb @@ -183,27 +183,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/services/web_hook_service_spec.rb b/spec/services/web_hook_service_spec.rb index 50167a2e059..2a4368868d5 100644 --- a/spec/services/web_hook_service_spec.rb +++ b/spec/services/web_hook_service_spec.rb @@ -55,31 +55,38 @@ describe WebHookService do describe '#execute' do before do project.hooks << [project_hook] - - WebMock.stub_request(:post, project_hook.url) end context 'when token is defined' do let(:project_hook) { create(:project_hook, :token) } it 'POSTs to the webhook URL' do + stub_full_request(project_hook.url, method: :post) + service_instance.execute - expect(WebMock).to have_requested(:post, project_hook.url).with( + + expect(WebMock).to have_requested(:post, stubbed_hostname(project_hook.url)).with( headers: headers.merge({ 'X-Gitlab-Token' => project_hook.token }) ).once end end it 'POSTs to the webhook URL' do + stub_full_request(project_hook.url, method: :post) + service_instance.execute - expect(WebMock).to have_requested(:post, project_hook.url).with( + + expect(WebMock).to have_requested(:post, stubbed_hostname(project_hook.url)).with( headers: headers ).once end it 'POSTs the data as JSON' do + stub_full_request(project_hook.url, method: :post) + service_instance.execute - expect(WebMock).to have_requested(:post, project_hook.url).with( + + expect(WebMock).to have_requested(:post, stubbed_hostname(project_hook.url)).with( headers: headers ).once end @@ -115,7 +122,7 @@ describe WebHookService do end it 'catches exceptions' do - WebMock.stub_request(:post, project_hook.url).to_raise(StandardError.new('Some error')) + stub_full_request(project_hook.url, method: :post).to_raise(StandardError.new('Some error')) expect { service_instance.execute }.to raise_error(StandardError) end @@ -125,20 +132,20 @@ describe WebHookService do exceptions.each do |exception_class| exception = exception_class.new('Exception message') - WebMock.stub_request(:post, project_hook.url).to_raise(exception) + stub_full_request(project_hook.url, method: :post).to_raise(exception) expect(service_instance.execute).to eq({ status: :error, message: exception.to_s }) expect { service_instance.execute }.not_to raise_error end end it 'handles 200 status code' do - WebMock.stub_request(:post, project_hook.url).to_return(status: 200, body: 'Success') + stub_full_request(project_hook.url, method: :post).to_return(status: 200, body: 'Success') expect(service_instance.execute).to include({ status: :success, http_status: 200, message: 'Success' }) end it 'handles 2xx status codes' do - WebMock.stub_request(:post, project_hook.url).to_return(status: 201, body: 'Success') + stub_full_request(project_hook.url, method: :post).to_return(status: 201, body: 'Success') expect(service_instance.execute).to include({ status: :success, http_status: 201, message: 'Success' }) end @@ -148,7 +155,7 @@ describe WebHookService do context 'with success' do before do - WebMock.stub_request(:post, project_hook.url).to_return(status: 200, body: 'Success') + stub_full_request(project_hook.url, method: :post).to_return(status: 200, body: 'Success') service_instance.execute end @@ -165,7 +172,7 @@ describe WebHookService do context 'with exception' do before do - WebMock.stub_request(:post, project_hook.url).to_raise(SocketError.new('Some HTTP Post error')) + stub_full_request(project_hook.url, method: :post).to_raise(SocketError.new('Some HTTP Post error')) service_instance.execute end @@ -182,7 +189,7 @@ describe WebHookService do context 'with unsafe response body' do before do - WebMock.stub_request(:post, project_hook.url).to_return(status: 200, body: "\xBB") + stub_full_request(project_hook.url, method: :post).to_return(status: 200, body: "\xBB") service_instance.execute end @@ -202,7 +209,7 @@ describe WebHookService do let(:service_instance) { described_class.new(service_hook, data, 'service_hook') } before do - WebMock.stub_request(:post, service_hook.url).to_return(status: 200, body: 'Success') + stub_full_request(service_hook.url, method: :post).to_return(status: 200, body: 'Success') end it { expect { service_instance.execute }.not_to change(WebHookLog, :count) } 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 39d13cccb13..4bc22861d58 100644 --- a/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb +++ b/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb @@ -7,6 +7,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 @@ -271,7 +273,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 @@ -284,7 +288,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 diff --git a/yarn.lock b/yarn.lock index dd85729333a..ef66861488c 100644 --- a/yarn.lock +++ b/yarn.lock @@ -998,10 +998,10 @@ dependencies: vue-eslint-parser "^6.0.4" -"@gitlab/svgs@^1.68.0": - version "1.68.0" - resolved "https://registry.yarnpkg.com/@gitlab/svgs/-/svgs-1.68.0.tgz#d631bd84ea7907592240d8417e82ba66d6a54c0c" - integrity sha512-3JmIq0bHg4InjLooM+kQFPfg3d7B1Pye67pN9+12kZXIa0nRGuwKEq3WSbcS+ACdg5jcVPNPYqStItEO4teHdw== +"@gitlab/svgs@^1.70.0": + version "1.70.0" + resolved "https://registry.yarnpkg.com/@gitlab/svgs/-/svgs-1.70.0.tgz#bdae478148c15d955fc06e69fd5d5ecae8298943" + integrity sha512-0uV9fgTwe17Fyy0hTcrsGX2jJuCrz3uRIe8yffuqc6pbQrSfYJyN66mfCCB45wq8lKTgOB5q0qcUyJx3RQEcKg== "@gitlab/ui@5.18.0": version "5.18.0" |