diff options
85 files changed, 1404 insertions, 298 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index 8d4509e370d..0b73585722f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,23 @@ documentation](doc/development/changelog.md) for instructions on adding your own entry. +## 12.0.3 (2019-06-27) + +- No changes. +### Security (10 changes) + +- Persist tmp snippet uploads at users. +- Gate MR head_pipeline behind read_pipeline ability. +- Fix DoS vulnerability in color validation regex. +- Expose merge requests count based on user access. +- Fix Denial of Service for comments when rendering issues/MR comments. +- Add missing authorizations in GraphQL. +- Disable Rails SQL query cache when applying service templates. +- Prevent Billion Laughs attack. +- Correctly check permissions when creating snippet notes. +- Prevent the detection of merge request templates by unauthorized users. + + ## 12.0.2 (2019-06-25) ### Fixed (7 changes, 1 of them is from the community) @@ -555,6 +572,27 @@ entry. - Add some frozen string to spec/**/*.rb. (gfyoung) +## 11.10.8 (2019-06-27) + +- No changes. +### Security (10 changes) + +- Fix Denial of Service for comments when rendering issues/MR comments. +- Gate MR head_pipeline behind read_pipeline ability. +- Fix DoS vulnerability in color validation regex. +- Expose merge requests count based on user access. +- Persist tmp snippet uploads at users. +- Add missing authorizations in GraphQL. +- Disable Rails SQL query cache when applying service templates. +- Prevent Billion Laughs attack. +- Correctly check permissions when creating snippet notes. +- Prevent the detection of merge request templates by unauthorized users. + +### Performance (1 change) + +- Add improvements to global search of issues and merge requests. !27817 + + ## 11.10.6 (2019-06-04) ### Fixed (7 changes, 1 of them is from the community) diff --git a/app/controllers/concerns/issuable_collections.rb b/app/controllers/concerns/issuable_collections.rb index 88a0690938a..21b3949e361 100644 --- a/app/controllers/concerns/issuable_collections.rb +++ b/app/controllers/concerns/issuable_collections.rb @@ -42,7 +42,7 @@ module IssuableCollections @issuables = @issuables.page(params[:page]) @issuables = per_page_for_relative_position if params[:sort] == 'relative_position' - @issuable_meta_data = issuable_meta_data(@issuables, collection_type) + @issuable_meta_data = issuable_meta_data(@issuables, collection_type, current_user) @total_pages = issuable_page_count end # rubocop:enable Gitlab/ModuleWithInstanceVariables diff --git a/app/controllers/concerns/issuable_collections_action.rb b/app/controllers/concerns/issuable_collections_action.rb index 18ed4027eac..4ad287c4a13 100644 --- a/app/controllers/concerns/issuable_collections_action.rb +++ b/app/controllers/concerns/issuable_collections_action.rb @@ -11,7 +11,7 @@ module IssuableCollectionsAction .non_archived .page(params[:page]) - @issuable_meta_data = issuable_meta_data(@issues, collection_type) + @issuable_meta_data = issuable_meta_data(@issues, collection_type, current_user) respond_to do |format| format.html @@ -22,7 +22,7 @@ module IssuableCollectionsAction def merge_requests @merge_requests = issuables_collection.page(params[:page]) - @issuable_meta_data = issuable_meta_data(@merge_requests, collection_type) + @issuable_meta_data = issuable_meta_data(@merge_requests, collection_type, current_user) end # rubocop:enable Gitlab/ModuleWithInstanceVariables diff --git a/app/controllers/concerns/notes_actions.rb b/app/controllers/concerns/notes_actions.rb index f96d1821095..0098c4cdf4c 100644 --- a/app/controllers/concerns/notes_actions.rb +++ b/app/controllers/concerns/notes_actions.rb @@ -203,17 +203,17 @@ module NotesActions # These params are also sent by the client but we need to set these based on # target_type and target_id because we're checking permissions based on that - create_params[:noteable_type] = params[:target_type].classify + create_params[:noteable_type] = noteable.class.name - case params[:target_type] - when 'commit' - create_params[:commit_id] = params[:target_id] - when 'merge_request' - create_params[:noteable_id] = params[:target_id] + case noteable + when Commit + create_params[:commit_id] = noteable.id + when MergeRequest + create_params[:noteable_id] = noteable.id # Notes on MergeRequest can have an extra `commit_id` context create_params[:commit_id] = params.dig(:note, :commit_id) else - create_params[:noteable_id] = params[:target_id] + create_params[:noteable_id] = noteable.id end end end diff --git a/app/controllers/projects/application_controller.rb b/app/controllers/projects/application_controller.rb index 90396c15375..b1f285f76d7 100644 --- a/app/controllers/projects/application_controller.rb +++ b/app/controllers/projects/application_controller.rb @@ -12,6 +12,11 @@ class Projects::ApplicationController < ApplicationController helper_method :repository, :can_collaborate_with_project?, :user_access + rescue_from Gitlab::Template::Finders::RepoTemplateFinder::FileNotFoundError do |exception| + log_exception(exception) + render_404 + end + private def project diff --git a/app/controllers/projects/templates_controller.rb b/app/controllers/projects/templates_controller.rb index 7ceea4e5b96..f987033a26c 100644 --- a/app/controllers/projects/templates_controller.rb +++ b/app/controllers/projects/templates_controller.rb @@ -1,7 +1,9 @@ # frozen_string_literal: true class Projects::TemplatesController < Projects::ApplicationController - before_action :authenticate_user!, :get_template_class + before_action :authenticate_user! + before_action :authorize_can_read_issuable! + before_action :get_template_class def show template = @template_type.find(params[:key], project) @@ -13,9 +15,20 @@ class Projects::TemplatesController < Projects::ApplicationController private + # User must have: + # - `read_merge_request` to see merge request templates, or + # - `read_issue` to see issue templates + # + # Note params[:template_type] has a route constraint to limit it to + # `merge_request` or `issue` + def authorize_can_read_issuable! + action = [:read_, params[:template_type]].join + + authorize_action!(action) + end + def get_template_class template_types = { issue: Gitlab::Template::IssueTemplate, merge_request: Gitlab::Template::MergeRequestTemplate }.with_indifferent_access @template_type = template_types[params[:template_type]] - render json: [], status: :not_found unless @template_type end end diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 12db493978b..330e2d0f8a5 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -298,7 +298,7 @@ class ProjectsController < Projects::ApplicationController elsif @project.feature_available?(:issues, current_user) @issues = issuables_collection.page(params[:page]) @collection_type = 'Issue' - @issuable_meta_data = issuable_meta_data(@issues, @collection_type) + @issuable_meta_data = issuable_meta_data(@issues, @collection_type, current_user) end render :show diff --git a/app/controllers/snippets/notes_controller.rb b/app/controllers/snippets/notes_controller.rb index eee14b0faf4..612897f27e6 100644 --- a/app/controllers/snippets/notes_controller.rb +++ b/app/controllers/snippets/notes_controller.rb @@ -5,8 +5,8 @@ class Snippets::NotesController < ApplicationController include ToggleAwardEmoji skip_before_action :authenticate_user!, only: [:index] - before_action :snippet - before_action :authorize_read_snippet!, only: [:show, :index, :create] + before_action :authorize_read_snippet!, only: [:show, :index] + before_action :authorize_create_note!, only: [:create] private @@ -33,4 +33,8 @@ class Snippets::NotesController < ApplicationController def authorize_read_snippet! return render_404 unless can?(current_user, :read_personal_snippet, snippet) end + + def authorize_create_note! + access_denied! unless can?(current_user, :create_note, noteable) + end end diff --git a/app/controllers/snippets_controller.rb b/app/controllers/snippets_controller.rb index 8ea5450b4e8..fad036b8df8 100644 --- a/app/controllers/snippets_controller.rb +++ b/app/controllers/snippets_controller.rb @@ -137,7 +137,7 @@ class SnippetsController < ApplicationController def move_temporary_files params[:files].each do |file| - FileMover.new(file, @snippet).execute + FileMover.new(file, from_model: current_user, to_model: @snippet).execute end end end diff --git a/app/controllers/uploads_controller.rb b/app/controllers/uploads_controller.rb index 5d28635232b..94bd18f70d4 100644 --- a/app/controllers/uploads_controller.rb +++ b/app/controllers/uploads_controller.rb @@ -41,7 +41,11 @@ class UploadsController < ApplicationController when Note can?(current_user, :read_project, model.project) when User - true + # We validate the current user has enough (writing) + # access to itself when a secret is given. + # For instance, user avatars are readable by anyone, + # while temporary, user snippet uploads are not. + !secret? || can?(current_user, :update_user, model) when Appearance true else @@ -56,9 +60,13 @@ class UploadsController < ApplicationController def authorize_create_access! return unless model - # for now we support only personal snippets comments. Only personal_snippet - # is allowed as a model to #create through routing. - authorized = can?(current_user, :create_note, model) + authorized = + case model + when User + can?(current_user, :update_user, model) + else + can?(current_user, :create_note, model) + end render_unauthorized unless authorized end @@ -75,6 +83,10 @@ class UploadsController < ApplicationController User === model || Appearance === model end + def secret? + params[:secret].present? + end + def upload_model_class MODEL_CLASSES[params[:model]] || raise(UnknownUploadModelError) end diff --git a/app/graphql/types/ci/detailed_status_type.rb b/app/graphql/types/ci/detailed_status_type.rb index 2987354b556..5f7d7a934ce 100644 --- a/app/graphql/types/ci/detailed_status_type.rb +++ b/app/graphql/types/ci/detailed_status_type.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true module Types module Ci + # rubocop: disable Graphql/AuthorizeTypes + # This is presented through `PipelineType` that has its own authorization class DetailedStatusType < BaseObject graphql_name 'DetailedStatus' @@ -13,5 +15,6 @@ module Types field :text, GraphQL::STRING_TYPE, null: false field :tooltip, GraphQL::STRING_TYPE, null: false, method: :status_tooltip end + # rubocop: enable Graphql/AuthorizeTypes end end diff --git a/app/graphql/types/issue_state_enum.rb b/app/graphql/types/issue_state_enum.rb index 6521407fc9d..70c34fbe491 100644 --- a/app/graphql/types/issue_state_enum.rb +++ b/app/graphql/types/issue_state_enum.rb @@ -1,8 +1,11 @@ # frozen_string_literal: true module Types + # rubocop: disable Graphql/AuthorizeTypes + # This is a BaseEnum through IssuableEnum, so it does not need authorization class IssueStateEnum < IssuableStateEnum graphql_name 'IssueState' description 'State of a GitLab issue' end + # rubocop: enable Graphql/AuthorizeTypes end diff --git a/app/graphql/types/label_type.rb b/app/graphql/types/label_type.rb index 50eb1b89c61..3aeda2e7953 100644 --- a/app/graphql/types/label_type.rb +++ b/app/graphql/types/label_type.rb @@ -4,6 +4,8 @@ module Types class LabelType < BaseObject graphql_name 'Label' + authorize :read_label + field :description, GraphQL::STRING_TYPE, null: true markdown_field :description_html, null: true field :title, GraphQL::STRING_TYPE, null: false diff --git a/app/graphql/types/merge_request_state_enum.rb b/app/graphql/types/merge_request_state_enum.rb index 92f52726ab3..37c890a3c8d 100644 --- a/app/graphql/types/merge_request_state_enum.rb +++ b/app/graphql/types/merge_request_state_enum.rb @@ -1,10 +1,13 @@ # frozen_string_literal: true module Types + # rubocop: disable Graphql/AuthorizeTypes + # This is a BaseEnum through IssuableEnum, so it does not need authorization class MergeRequestStateEnum < IssuableStateEnum graphql_name 'MergeRequestState' description 'State of a GitLab merge request' value 'merged' end + # rubocop: enable Graphql/AuthorizeTypes end diff --git a/app/graphql/types/metadata_type.rb b/app/graphql/types/metadata_type.rb index 2d8bad0614b..7d7813a7652 100644 --- a/app/graphql/types/metadata_type.rb +++ b/app/graphql/types/metadata_type.rb @@ -4,6 +4,8 @@ module Types class MetadataType < ::Types::BaseObject graphql_name 'Metadata' + authorize :read_instance_metadata + field :version, GraphQL::STRING_TYPE, null: false field :revision, GraphQL::STRING_TYPE, null: false end diff --git a/app/graphql/types/namespace_type.rb b/app/graphql/types/namespace_type.rb index 62feccaa660..f105e9e6e28 100644 --- a/app/graphql/types/namespace_type.rb +++ b/app/graphql/types/namespace_type.rb @@ -4,6 +4,8 @@ module Types class NamespaceType < BaseObject graphql_name 'Namespace' + authorize :read_namespace + field :id, GraphQL::ID_TYPE, null: false field :name, GraphQL::STRING_TYPE, null: false diff --git a/app/graphql/types/notes/diff_position_type.rb b/app/graphql/types/notes/diff_position_type.rb index 104ccb79bbb..ebc24451715 100644 --- a/app/graphql/types/notes/diff_position_type.rb +++ b/app/graphql/types/notes/diff_position_type.rb @@ -2,6 +2,8 @@ module Types module Notes + # rubocop: disable Graphql/AuthorizeTypes + # This is presented through `NoteType` that has its own authorization class DiffPositionType < BaseObject graphql_name 'DiffPosition' @@ -42,5 +44,6 @@ module Types description: "The total height of the image", resolve: -> (position, _args, _ctx) { position.height if position.on_image? } end + # rubocop: enable Graphql/AuthorizeTypes end end diff --git a/app/graphql/types/project_type.rb b/app/graphql/types/project_type.rb index ac957eafafc..c25688ab043 100644 --- a/app/graphql/types/project_type.rb +++ b/app/graphql/types/project_type.rb @@ -67,14 +67,14 @@ module Types field :only_allow_merge_if_all_discussions_are_resolved, GraphQL::BOOLEAN_TYPE, null: true field :printing_merge_request_link_enabled, GraphQL::BOOLEAN_TYPE, null: true - field :namespace, Types::NamespaceType, null: false + field :namespace, Types::NamespaceType, null: true field :group, Types::GroupType, null: true field :statistics, Types::ProjectStatisticsType, null: true, resolve: -> (obj, _args, _ctx) { Gitlab::Graphql::Loaders::BatchProjectStatisticsLoader.new(obj.id).find } - field :repository, Types::RepositoryType, null: false + field :repository, Types::RepositoryType, null: true field :merge_requests, Types::MergeRequestType.connection_type, diff --git a/app/graphql/types/query_type.rb b/app/graphql/types/query_type.rb index 536bdb077ad..53d36b43576 100644 --- a/app/graphql/types/query_type.rb +++ b/app/graphql/types/query_type.rb @@ -22,10 +22,7 @@ module Types field :metadata, Types::MetadataType, null: true, resolver: Resolvers::MetadataResolver, - description: 'Metadata about GitLab' do |*args| - - authorize :read_instance_metadata - end + description: 'Metadata about GitLab' field :echo, GraphQL::STRING_TYPE, null: false, function: Functions::Echo.new end diff --git a/app/graphql/types/task_completion_status.rb b/app/graphql/types/task_completion_status.rb index c289802509d..ac128481ac4 100644 --- a/app/graphql/types/task_completion_status.rb +++ b/app/graphql/types/task_completion_status.rb @@ -1,6 +1,9 @@ # frozen_string_literal: true module Types + # rubocop: disable Graphql/AuthorizeTypes + # This is used in `IssueType` and `MergeRequestType` both of which have their + # own authorization class TaskCompletionStatus < BaseObject graphql_name 'TaskCompletionStatus' description 'Completion status of tasks' @@ -8,4 +11,5 @@ module Types field :count, GraphQL::INT_TYPE, null: false field :completed_count, GraphQL::INT_TYPE, null: false end + # rubocop: enable Graphql/AuthorizeTypes end diff --git a/app/graphql/types/tree/blob_type.rb b/app/graphql/types/tree/blob_type.rb index 760781f3612..9497e378dc0 100644 --- a/app/graphql/types/tree/blob_type.rb +++ b/app/graphql/types/tree/blob_type.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true module Types module Tree + # rubocop: disable Graphql/AuthorizeTypes + # This is presented through `Repository` that has its own authorization class BlobType < BaseObject implements Types::Tree::EntryType @@ -12,6 +14,7 @@ module Types field :lfs_oid, GraphQL::STRING_TYPE, null: true, resolve: -> (blob, args, ctx) do Gitlab::Graphql::Loaders::BatchLfsOidLoader.new(blob.repository, blob.id).find end + # rubocop: enable Graphql/AuthorizeTypes end end end diff --git a/app/graphql/types/tree/submodule_type.rb b/app/graphql/types/tree/submodule_type.rb index cea76dbfd2a..8cb1e04f5ba 100644 --- a/app/graphql/types/tree/submodule_type.rb +++ b/app/graphql/types/tree/submodule_type.rb @@ -1,10 +1,13 @@ # frozen_string_literal: true module Types module Tree + # rubocop: disable Graphql/AuthorizeTypes + # This is presented through `Repository` that has its own authorization class SubmoduleType < BaseObject implements Types::Tree::EntryType graphql_name 'Submodule' end + # rubocop: enable Graphql/AuthorizeTypes end end diff --git a/app/graphql/types/tree/tree_entry_type.rb b/app/graphql/types/tree/tree_entry_type.rb index 23ec2ef0ec2..d7faa633706 100644 --- a/app/graphql/types/tree/tree_entry_type.rb +++ b/app/graphql/types/tree/tree_entry_type.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true module Types module Tree + # rubocop: disable Graphql/AuthorizeTypes + # This is presented through `Repository` that has its own authorization class TreeEntryType < BaseObject implements Types::Tree::EntryType @@ -11,5 +13,6 @@ module Types field :web_url, GraphQL::STRING_TYPE, null: true end + # rubocop: enable Graphql/AuthorizeTypes end end diff --git a/app/graphql/types/tree/tree_type.rb b/app/graphql/types/tree/tree_type.rb index 1ee93ed9542..e7644b071d3 100644 --- a/app/graphql/types/tree/tree_type.rb +++ b/app/graphql/types/tree/tree_type.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true module Types module Tree + # rubocop: disable Graphql/AuthorizeTypes + # This is presented through `Repository` that has its own authorization class TreeType < BaseObject graphql_name 'Tree' @@ -13,6 +15,7 @@ module Types field :blobs, Types::Tree::BlobType.connection_type, null: false, resolve: -> (obj, args, ctx) do Gitlab::Graphql::Representation::TreeEntry.decorate(obj.blobs, obj.repository) end + # rubocop: enable Graphql/AuthorizeTypes end end end diff --git a/app/helpers/issuables_helper.rb b/app/helpers/issuables_helper.rb index 045de105b77..cd2669ef6ad 100644 --- a/app/helpers/issuables_helper.rb +++ b/app/helpers/issuables_helper.rb @@ -280,7 +280,7 @@ module IssuablesHelper initialTaskStatus: issuable.task_status } - data[:hasClosingMergeRequest] = issuable.merge_requests_count != 0 if issuable.is_a?(Issue) + data[:hasClosingMergeRequest] = issuable.merge_requests_count(current_user) != 0 if issuable.is_a?(Issue) if parent.is_a?(Group) data[:groupPath] = parent.path diff --git a/app/helpers/snippets_helper.rb b/app/helpers/snippets_helper.rb index ecb2b2d707b..6ccc1fb2ed1 100644 --- a/app/helpers/snippets_helper.rb +++ b/app/helpers/snippets_helper.rb @@ -1,6 +1,16 @@ # frozen_string_literal: true module SnippetsHelper + def snippets_upload_path(snippet, user) + return unless user + + if snippet&.persisted? + upload_path('personal_snippet', id: snippet.id) + else + upload_path('user', id: user.id) + end + end + def reliable_snippet_path(snippet, opts = nil) if snippet.project_id? project_snippet_path(snippet.project, snippet, opts) diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 127430cc68f..299e413321d 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -29,7 +29,11 @@ module Issuable # This object is used to gather issuable meta data for displaying # upvotes, downvotes, notes and closing merge requests count for issues and merge requests # lists avoiding n+1 queries and improving performance. - IssuableMeta = Struct.new(:upvotes, :downvotes, :user_notes_count, :merge_requests_count) + IssuableMeta = Struct.new(:upvotes, :downvotes, :user_notes_count, :mrs_count) do + def merge_requests_count(user = nil) + mrs_count + end + end included do cache_markdown_field :title, pipeline: :single_line diff --git a/app/models/issue.rb b/app/models/issue.rb index 30e29911758..982a94315bd 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -250,8 +250,8 @@ class Issue < ApplicationRecord end # rubocop: enable CodeReuse/ServiceClass - def merge_requests_count - merge_requests_closing_issues.count + def merge_requests_count(user = nil) + ::MergeRequestsClosingIssues.count_for_issue(self.id, user) end def labels_hook_attrs diff --git a/app/models/merge_requests_closing_issues.rb b/app/models/merge_requests_closing_issues.rb index 61af50841ee..22cedf57b86 100644 --- a/app/models/merge_requests_closing_issues.rb +++ b/app/models/merge_requests_closing_issues.rb @@ -7,11 +7,38 @@ class MergeRequestsClosingIssues < ApplicationRecord validates :merge_request_id, uniqueness: { scope: :issue_id }, presence: true validates :issue_id, presence: true + scope :with_issues, ->(ids) { where(issue_id: ids) } + scope :with_merge_requests_enabled, -> do + joins(:merge_request) + .joins('INNER JOIN project_features ON merge_requests.target_project_id = project_features.project_id') + .where('project_features.merge_requests_access_level >= :access', access: ProjectFeature::ENABLED) + end + + scope :accessible_by, ->(user) do + joins(:merge_request) + .joins('INNER JOIN project_features ON merge_requests.target_project_id = project_features.project_id') + .where('project_features.merge_requests_access_level >= :access OR EXISTS(:authorizations)', + access: ProjectFeature::ENABLED, + authorizations: user.authorizations_for_projects(min_access_level: Gitlab::Access::REPORTER, related_project_column: "merge_requests.target_project_id") + ) + end + class << self - def count_for_collection(ids) - group(:issue_id) - .where(issue_id: ids) - .pluck('issue_id', 'COUNT(*) as count') + def count_for_collection(ids, current_user) + closing_merge_requests(ids, current_user).group(:issue_id).pluck('issue_id', 'COUNT(*) as count') + end + + def count_for_issue(id, current_user) + closing_merge_requests(id, current_user).count + end + + private + + def closing_merge_requests(ids, current_user) + return with_issues(ids) if current_user&.admin? + return with_issues(ids).with_merge_requests_enabled if current_user.blank? + + with_issues(ids).accessible_by(current_user) end end end diff --git a/app/policies/repository_policy.rb b/app/policies/repository_policy.rb new file mode 100644 index 00000000000..32340749858 --- /dev/null +++ b/app/policies/repository_policy.rb @@ -0,0 +1,5 @@ +# frozen_string_literal: true + +class RepositoryPolicy < BasePolicy + delegate { @subject.project } +end diff --git a/app/uploaders/file_mover.rb b/app/uploaders/file_mover.rb index 236b7ed2b3d..12be1e2bb22 100644 --- a/app/uploaders/file_mover.rb +++ b/app/uploaders/file_mover.rb @@ -1,22 +1,29 @@ # frozen_string_literal: true class FileMover - attr_reader :secret, :file_name, :model, :update_field + include Gitlab::Utils::StrongMemoize - def initialize(file_path, model, update_field = :description) + attr_reader :secret, :file_name, :from_model, :to_model, :update_field + + def initialize(file_path, update_field = :description, from_model:, to_model:) @secret = File.split(File.dirname(file_path)).last @file_name = File.basename(file_path) - @model = model + @from_model = from_model + @to_model = to_model @update_field = update_field end def execute + temp_file_uploader.retrieve_from_store!(file_name) + return unless valid? + uploader.retrieve_from_store!(file_name) + move if update_markdown - uploader.record_upload + update_upload_model uploader.schedule_background_upload end end @@ -24,52 +31,77 @@ class FileMover private def valid? - Pathname.new(temp_file_path).realpath.to_path.start_with?( - (Pathname(temp_file_uploader.root) + temp_file_uploader.base_dir).to_path - ) + if temp_file_uploader.file_storage? + Pathname.new(temp_file_path).realpath.to_path.start_with?( + (Pathname(temp_file_uploader.root) + temp_file_uploader.base_dir).to_path + ) + else + temp_file_uploader.exists? + end end def move - FileUtils.mkdir_p(File.dirname(file_path)) - FileUtils.move(temp_file_path, file_path) + if temp_file_uploader.file_storage? + FileUtils.mkdir_p(File.dirname(file_path)) + FileUtils.move(temp_file_path, file_path) + else + uploader.copy_file(temp_file_uploader.file) + temp_file_uploader.upload.destroy! + end end def update_markdown - updated_text = model.read_attribute(update_field) - .gsub(temp_file_uploader.markdown_link, uploader.markdown_link) - model.update_attribute(update_field, updated_text) + updated_text = to_model.read_attribute(update_field) + .gsub(temp_file_uploader.markdown_link, uploader.markdown_link) + to_model.update_attribute(update_field, updated_text) rescue revert false end - def temp_file_path - return @temp_file_path if @temp_file_path + def update_upload_model + return unless upload = temp_file_uploader.upload + return if upload.destroyed? - temp_file_uploader.retrieve_from_store!(file_name) + upload.update!(model: to_model) + end - @temp_file_path = temp_file_uploader.file.path + def temp_file_path + strong_memoize(:temp_file_path) do + temp_file_uploader.file.path + end end def file_path - return @file_path if @file_path - - uploader.retrieve_from_store!(file_name) - - @file_path = uploader.file.path + strong_memoize(:file_path) do + uploader.file.path + end end def uploader - @uploader ||= PersonalFileUploader.new(model, secret: secret) + @uploader ||= + begin + uploader = PersonalFileUploader.new(to_model, secret: secret) + + # Enforcing a REMOTE object storage given FileUploader#retrieve_from_store! won't do it + # (there's no upload at the target yet). + if uploader.class.object_store_enabled? + uploader.object_store = ::ObjectStorage::Store::REMOTE + end + + uploader + end end def temp_file_uploader - @temp_file_uploader ||= PersonalFileUploader.new(nil, secret: secret) + @temp_file_uploader ||= PersonalFileUploader.new(from_model, secret: secret) end def revert - Rails.logger.warn("Markdown not updated, file move reverted for #{model}") + Rails.logger.warn("Markdown not updated, file move reverted for #{to_model}") - FileUtils.move(file_path, temp_file_path) + if temp_file_uploader.file_storage? + FileUtils.move(file_path, temp_file_path) + end end end diff --git a/app/validators/color_validator.rb b/app/validators/color_validator.rb index 1932d042e83..974dfbbf394 100644 --- a/app/validators/color_validator.rb +++ b/app/validators/color_validator.rb @@ -12,7 +12,7 @@ # end # class ColorValidator < ActiveModel::EachValidator - PATTERN = /\A\#[0-9A-Fa-f]{3}{1,2}+\Z/.freeze + PATTERN = /\A\#(?:[0-9A-Fa-f]{3}){1,2}\Z/.freeze def validate_each(record, attribute, value) unless value =~ PATTERN diff --git a/app/views/layouts/snippets.html.haml b/app/views/layouts/snippets.html.haml index 5f986c81ff4..841b2a5e79c 100644 --- a/app/views/layouts/snippets.html.haml +++ b/app/views/layouts/snippets.html.haml @@ -1,9 +1,10 @@ - header_title _("Snippets"), snippets_path +- snippets_upload_path = snippets_upload_path(@snippet, current_user) - content_for :page_specific_javascripts do - - if @snippet && current_user + - if snippets_upload_path -# haml-lint:disable InlineJavaScript :javascript - window.uploads_path = "#{upload_path('personal_snippet', id: @snippet.id)}"; + window.uploads_path = "#{snippets_upload_path}"; = render template: "layouts/application" diff --git a/app/views/shared/_issuable_meta_data.html.haml b/app/views/shared/_issuable_meta_data.html.haml index 31a5370a5f8..71b13a5d741 100644 --- a/app/views/shared/_issuable_meta_data.html.haml +++ b/app/views/shared/_issuable_meta_data.html.haml @@ -2,7 +2,7 @@ - issue_votes = @issuable_meta_data[issuable.id] - upvotes, downvotes = issue_votes.upvotes, issue_votes.downvotes - issuable_url = @collection_type == "Issue" ? issue_path(issuable, anchor: 'notes') : merge_request_path(issuable, anchor: 'notes') -- issuable_mr = @issuable_meta_data[issuable.id].merge_requests_count +- issuable_mr = @issuable_meta_data[issuable.id].merge_requests_count(current_user) - if issuable_mr > 0 %li.issuable-mr.d-none.d-sm-block.has-tooltip{ title: _('Related merge requests') } diff --git a/changelogs/unreleased/osw-persist-tmp-snippet-uploads.yml b/changelogs/unreleased/osw-persist-tmp-snippet-uploads.yml new file mode 100644 index 00000000000..9348626c41d --- /dev/null +++ b/changelogs/unreleased/osw-persist-tmp-snippet-uploads.yml @@ -0,0 +1,5 @@ +--- +title: Persist tmp snippet uploads at users +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-2858-fix-color-validation.yml b/changelogs/unreleased/security-2858-fix-color-validation.yml new file mode 100644 index 00000000000..3430207a2b6 --- /dev/null +++ b/changelogs/unreleased/security-2858-fix-color-validation.yml @@ -0,0 +1,5 @@ +--- +title: Fix DoS vulnerability in color validation regex +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-59581-related-merge-requests-count.yml b/changelogs/unreleased/security-59581-related-merge-requests-count.yml new file mode 100644 index 00000000000..83faa2f7c13 --- /dev/null +++ b/changelogs/unreleased/security-59581-related-merge-requests-count.yml @@ -0,0 +1,5 @@ +--- +title: Expose merge requests count based on user access +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-DOS_issue_comments_banzai.yml b/changelogs/unreleased/security-DOS_issue_comments_banzai.yml new file mode 100644 index 00000000000..2405b1a4f5f --- /dev/null +++ b/changelogs/unreleased/security-DOS_issue_comments_banzai.yml @@ -0,0 +1,5 @@ +--- +title: Fix Denial of Service for comments when rendering issues/MR comments +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-bvl-enforce-graphql-type-authorization.yml b/changelogs/unreleased/security-bvl-enforce-graphql-type-authorization.yml new file mode 100644 index 00000000000..7dedb9f6230 --- /dev/null +++ b/changelogs/unreleased/security-bvl-enforce-graphql-type-authorization.yml @@ -0,0 +1,5 @@ +--- +title: Add missing authorizations in GraphQL +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-fp-prevent-billion-laughs-attack.yml b/changelogs/unreleased/security-fp-prevent-billion-laughs-attack.yml new file mode 100644 index 00000000000..4e0cf848931 --- /dev/null +++ b/changelogs/unreleased/security-fp-prevent-billion-laughs-attack.yml @@ -0,0 +1,5 @@ +--- +title: Prevent Billion Laughs attack +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..fe8c4dfb3c8 --- /dev/null +++ b/changelogs/unreleased/security-mr-head-pipeline-leak.yml @@ -0,0 +1,5 @@ +--- +title: Gate MR head_pipeline behind read_pipeline ability. +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-notes-in-private-snippets.yml b/changelogs/unreleased/security-notes-in-private-snippets.yml new file mode 100644 index 00000000000..907d98cb16d --- /dev/null +++ b/changelogs/unreleased/security-notes-in-private-snippets.yml @@ -0,0 +1,5 @@ +--- +title: Correctly check permissions when creating snippet notes +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-prevent-detection-of-merge-request-template-name.yml b/changelogs/unreleased/security-prevent-detection-of-merge-request-template-name.yml new file mode 100644 index 00000000000..d7bb884cb4b --- /dev/null +++ b/changelogs/unreleased/security-prevent-detection-of-merge-request-template-name.yml @@ -0,0 +1,5 @@ +--- +title: Prevent the detection of merge request templates by unauthorized users +merge_request: +author: +type: security diff --git a/config/routes/project.rb b/config/routes/project.rb index bcbbd7222e0..0e72e8631cf 100644 --- a/config/routes/project.rb +++ b/config/routes/project.rb @@ -182,7 +182,10 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do # # Templates # - get '/templates/:template_type/:key' => 'templates#show', as: :template, constraints: { key: %r{[^/]+} } + get '/templates/:template_type/:key' => 'templates#show', + as: :template, + defaults: { format: 'json' }, + constraints: { key: %r{[^/]+}, template_type: %r{issue|merge_request}, format: 'json' } resources :commit, only: [:show], constraints: { id: /\h{7,40}/ } do member do diff --git a/config/routes/uploads.rb b/config/routes/uploads.rb index b594f55f8a0..920f8454ce2 100644 --- a/config/routes/uploads.rb +++ b/config/routes/uploads.rb @@ -7,7 +7,7 @@ scope path: :uploads do # show uploads for models, snippets (notes) available for now get '-/system/:model/:id/:secret/:filename', to: 'uploads#show', - constraints: { model: /personal_snippet/, id: /\d+/, filename: %r{[^/]+} } + constraints: { model: /personal_snippet|user/, id: /\d+/, filename: %r{[^/]+} } # show temporary uploads get '-/system/temp/:secret/:filename', @@ -28,7 +28,7 @@ scope path: :uploads do # create uploads for models, snippets (notes) available for now post ':model', to: 'uploads#create', - constraints: { model: /personal_snippet/, id: /\d+/ }, + constraints: { model: /personal_snippet|user/, id: /\d+/ }, as: 'upload' end diff --git a/lib/api/entities.rb b/lib/api/entities.rb index ead01dc53f7..ac1dfb64a73 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -498,9 +498,9 @@ module API expose :state, :created_at, :updated_at # Avoids an N+1 query when metadata is included - def issuable_metadata(subject, options, method) + def issuable_metadata(subject, options, method, args = nil) cached_subject = options.dig(:issuable_metadata, subject.id) - (cached_subject || subject).public_send(method) # rubocop: disable GitlabSecurity/PublicSend + (cached_subject || subject).public_send(method, *args) # rubocop: disable GitlabSecurity/PublicSend end end @@ -564,7 +564,7 @@ module API end expose(:user_notes_count) { |issue, options| issuable_metadata(issue, options, :user_notes_count) } - expose(:merge_requests_count) { |issue, options| issuable_metadata(issue, options, :merge_requests_count) } + expose(:merge_requests_count) { |issue, options| issuable_metadata(issue, options, :merge_requests_count, options[:current_user]) } expose(:upvotes) { |issue, options| issuable_metadata(issue, options, :upvotes) } expose(:downvotes) { |issue, options| issuable_metadata(issue, options, :downvotes) } expose :due_date @@ -757,7 +757,9 @@ module API merge_request.metrics&.pipeline end - expose :head_pipeline, using: 'API::Entities::Pipeline' + expose :head_pipeline, using: 'API::Entities::Pipeline', if: -> (_, options) do + Ability.allowed?(options[:current_user], :read_pipeline, options[:project]) + end expose :diff_refs, using: Entities::DiffRefs diff --git a/lib/api/issues.rb b/lib/api/issues.rb index 039ebf92187..d687acf3423 100644 --- a/lib/api/issues.rb +++ b/lib/api/issues.rb @@ -96,7 +96,7 @@ module API with: Entities::Issue, with_labels_details: declared_params[:with_labels_details], current_user: current_user, - issuable_metadata: issuable_meta_data(issues, 'Issue') + issuable_metadata: issuable_meta_data(issues, 'Issue', current_user) } present issues, options @@ -122,7 +122,7 @@ module API with: Entities::Issue, with_labels_details: declared_params[:with_labels_details], current_user: current_user, - issuable_metadata: issuable_meta_data(issues, 'Issue') + issuable_metadata: issuable_meta_data(issues, 'Issue', current_user) } present issues, options @@ -161,7 +161,7 @@ module API with_labels_details: declared_params[:with_labels_details], current_user: current_user, project: user_project, - issuable_metadata: issuable_meta_data(issues, 'Issue') + issuable_metadata: issuable_meta_data(issues, 'Issue', current_user) } present issues, options diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index bf87e9ec2ff..6b8c1a2c0e8 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -72,7 +72,7 @@ module API if params[:view] == 'simple' options[:with] = Entities::MergeRequestSimple else - options[:issuable_metadata] = issuable_meta_data(merge_requests, 'MergeRequest') + options[:issuable_metadata] = issuable_meta_data(merge_requests, 'MergeRequest', current_user) end options diff --git a/lib/api/todos.rb b/lib/api/todos.rb index 871eaabc887..7260ecfb5ee 100644 --- a/lib/api/todos.rb +++ b/lib/api/todos.rb @@ -65,7 +65,7 @@ module API next unless collection targets = collection.map(&:target) - options[type] = { issuable_metadata: issuable_meta_data(targets, type) } + options[type] = { issuable_metadata: issuable_meta_data(targets, type, current_user) } end end end diff --git a/lib/banzai/filter/relative_link_filter.rb b/lib/banzai/filter/relative_link_filter.rb index 80c84c0f622..bf84d7cddae 100644 --- a/lib/banzai/filter/relative_link_filter.rb +++ b/lib/banzai/filter/relative_link_filter.rb @@ -102,7 +102,7 @@ module Banzai end def relative_file_path(uri) - path = Addressable::URI.unescape(uri.path) + path = Addressable::URI.unescape(uri.path).delete("\0") request_path = Addressable::URI.unescape(context[:requested_path]) nested_path = build_relative_path(path, request_path) file_exists?(nested_path) ? nested_path : path diff --git a/lib/gitlab/ci/config.rb b/lib/gitlab/ci/config.rb index 7aeac11df55..cde042c5e0a 100644 --- a/lib/gitlab/ci/config.rb +++ b/lib/gitlab/ci/config.rb @@ -23,6 +23,11 @@ module Gitlab @root = Entry::Root.new(@config) @root.compose! + + rescue Gitlab::Config::Loader::Yaml::DataTooLargeError => e + Gitlab::Sentry.track_exception(e, extra: { user: user.inspect, project: project.inspect }) + raise Config::ConfigError, e.message + rescue *rescue_errors => e raise Config::ConfigError, e.message end diff --git a/lib/gitlab/config/loader/yaml.rb b/lib/gitlab/config/loader/yaml.rb index 8159f8b8026..4cedab1545c 100644 --- a/lib/gitlab/config/loader/yaml.rb +++ b/lib/gitlab/config/loader/yaml.rb @@ -4,6 +4,13 @@ module Gitlab module Config module Loader class Yaml + DataTooLargeError = Class.new(Loader::FormatError) + + include Gitlab::Utils::StrongMemoize + + MAX_YAML_SIZE = 1.megabyte + MAX_YAML_DEPTH = 100 + def initialize(config) @config = YAML.safe_load(config, [Symbol], [], true) rescue Psych::Exception => e @@ -11,16 +18,35 @@ module Gitlab end def valid? - @config.is_a?(Hash) + hash? && !too_big? end def load! - unless valid? - raise Loader::FormatError, 'Invalid configuration format' - end + raise DataTooLargeError, 'The parsed YAML is too big' if too_big? + raise Loader::FormatError, 'Invalid configuration format' unless hash? @config.deep_symbolize_keys end + + private + + def hash? + @config.is_a?(Hash) + end + + def too_big? + return false unless Feature.enabled?(:ci_yaml_limit_size, default_enabled: true) + + !deep_size.valid? + end + + def deep_size + strong_memoize(:deep_size) do + Gitlab::Utils::DeepSize.new(@config, + max_size: MAX_YAML_SIZE, + max_depth: MAX_YAML_DEPTH) + end + end end end end diff --git a/lib/gitlab/graphql/authorize/authorize_field_service.rb b/lib/gitlab/graphql/authorize/authorize_field_service.rb index 619ce100421..3b5dde2fde5 100644 --- a/lib/gitlab/graphql/authorize/authorize_field_service.rb +++ b/lib/gitlab/graphql/authorize/authorize_field_service.rb @@ -39,6 +39,8 @@ module Gitlab type = node_type_for_basic_connection(type) end + type = type.unwrap if type.kind.non_null? + Array.wrap(type.metadata[:authorize]) end diff --git a/lib/gitlab/issuable_metadata.rb b/lib/gitlab/issuable_metadata.rb index 351d15605e0..be73bcd5506 100644 --- a/lib/gitlab/issuable_metadata.rb +++ b/lib/gitlab/issuable_metadata.rb @@ -2,7 +2,7 @@ module Gitlab module IssuableMetadata - def issuable_meta_data(issuable_collection, collection_type) + def issuable_meta_data(issuable_collection, collection_type, user = nil) # ActiveRecord uses Object#extend for null relations. if !(issuable_collection.singleton_class < ActiveRecord::NullRelation) && issuable_collection.respond_to?(:limit_value) && @@ -23,7 +23,7 @@ module Gitlab issuable_votes_count = ::AwardEmoji.votes_for_collection(issuable_ids, collection_type) issuable_merge_requests_count = if collection_type == 'Issue' - ::MergeRequestsClosingIssues.count_for_collection(issuable_ids) + ::MergeRequestsClosingIssues.count_for_collection(issuable_ids, user) else [] end diff --git a/lib/gitlab/utils/deep_size.rb b/lib/gitlab/utils/deep_size.rb new file mode 100644 index 00000000000..562cf09e249 --- /dev/null +++ b/lib/gitlab/utils/deep_size.rb @@ -0,0 +1,79 @@ +# frozen_string_literal: true + +require 'objspace' + +module Gitlab + module Utils + class DeepSize + Error = Class.new(StandardError) + TooMuchDataError = Class.new(Error) + + DEFAULT_MAX_SIZE = 1.megabyte + DEFAULT_MAX_DEPTH = 100 + + def initialize(root, max_size: DEFAULT_MAX_SIZE, max_depth: DEFAULT_MAX_DEPTH) + @root = root + @max_size = max_size + @max_depth = max_depth + @size = 0 + @depth = 0 + + evaluate + end + + def valid? + !too_big? && !too_deep? + end + + private + + def evaluate + add_object(@root) + rescue Error + # NOOP + end + + def too_big? + @size > @max_size + end + + def too_deep? + @depth > @max_depth + end + + def add_object(object) + @size += ObjectSpace.memsize_of(object) + raise TooMuchDataError if @size > @max_size + + add_array(object) if object.is_a?(Array) + add_hash(object) if object.is_a?(Hash) + end + + def add_array(object) + with_nesting do + object.each do |n| + add_object(n) + end + end + end + + def add_hash(object) + with_nesting do + object.each do |key, value| + add_object(key) + add_object(value) + end + end + end + + def with_nesting + @depth += 1 + raise TooMuchDataError if too_deep? + + yield + + @depth -= 1 + end + end + end +end diff --git a/rubocop/cop/graphql/authorize_types.rb b/rubocop/cop/graphql/authorize_types.rb new file mode 100644 index 00000000000..93fe80c3edf --- /dev/null +++ b/rubocop/cop/graphql/authorize_types.rb @@ -0,0 +1,61 @@ +# frozen_string_literal: true + +require_relative '../../spec_helpers' + +module RuboCop + module Cop + module Graphql + class AuthorizeTypes < RuboCop::Cop::Cop + include SpecHelpers + + MSG = 'Add an `authorize :ability` call to the type: '\ + 'https://docs.gitlab.com/ee/development/api_graphql_styleguide.html#type-authorization' + + TYPES_DIR = 'app/graphql/types' + + # We want to exclude our own basetypes and scalars + WHITELISTED_TYPES = %w[BaseEnum BaseScalar BasePermissionType MutationType + QueryType GraphQL::Schema BaseUnion].freeze + + def_node_search :authorize?, <<~PATTERN + (send nil? :authorize ...) + PATTERN + + def on_class(node) + return unless in_type?(node) + return if whitelisted?(class_constant(node)) + return if whitelisted?(superclass_constant(node)) + + add_offense(node, location: :expression) unless authorize?(node) + end + + private + + def in_type?(node) + return if in_spec?(node) + + path = node.location.expression.source_buffer.name + + path.include?(TYPES_DIR) + end + + def whitelisted?(class_node) + return false unless class_node&.const_name + + WHITELISTED_TYPES.any? { |whitelisted| class_node.const_name.include?(whitelisted) } + end + + def class_constant(node) + node.descendants.first + end + + def superclass_constant(class_node) + # First one is the class name itself, second is it's superclass + _class_constant, *others = class_node.descendants + + others.find { |node| node.const_type? && node&.const_name != 'Types' } + end + end + end + end +end diff --git a/rubocop/rubocop.rb b/rubocop/rubocop.rb index e2a19978839..27c63d92ae5 100644 --- a/rubocop/rubocop.rb +++ b/rubocop/rubocop.rb @@ -43,3 +43,4 @@ require_relative 'cop/code_reuse/serializer' require_relative 'cop/code_reuse/active_record' require_relative 'cop/group_public_or_visible_to_user' require_relative 'cop/inject_enterprise_edition_module' +require_relative 'cop/graphql/authorize_types' diff --git a/spec/controllers/projects/notes_controller_spec.rb b/spec/controllers/projects/notes_controller_spec.rb index 6ec84f5c528..1db1963476c 100644 --- a/spec/controllers/projects/notes_controller_spec.rb +++ b/spec/controllers/projects/notes_controller_spec.rb @@ -252,7 +252,7 @@ describe Projects::NotesController do before do service_params = ActionController::Parameters.new({ note: 'some note', - noteable_id: merge_request.id.to_s, + noteable_id: merge_request.id, noteable_type: 'MergeRequest', commit_id: nil, merge_request_diff_head_sha: 'sha' diff --git a/spec/controllers/projects/templates_controller_spec.rb b/spec/controllers/projects/templates_controller_spec.rb index bebf17728c0..9e7d34b10c0 100644 --- a/spec/controllers/projects/templates_controller_spec.rb +++ b/spec/controllers/projects/templates_controller_spec.rb @@ -3,49 +3,101 @@ require 'spec_helper' describe Projects::TemplatesController do - let(:project) { create(:project, :repository) } + let(:project) { create(:project, :repository, :private) } let(:user) { create(:user) } - let(:user2) { create(:user) } - let(:file_path_1) { '.gitlab/issue_templates/bug.md' } + let(:file_path_1) { '.gitlab/issue_templates/issue_template.md' } + let(:file_path_2) { '.gitlab/merge_request_templates/merge_request_template.md' } let(:body) { JSON.parse(response.body) } + let!(:file_1) { project.repository.create_file(user, file_path_1, 'issue content', message: 'message', branch_name: 'master') } + let!(:file_2) { project.repository.create_file(user, file_path_2, 'merge request content', message: 'message', branch_name: 'master') } - before do - project.add_developer(user) - sign_in(user) - end + describe '#show' do + shared_examples 'renders issue templates as json' do + it do + get(:show, params: { namespace_id: project.namespace, template_type: 'issue', key: 'issue_template', project_id: project }, format: :json) - before do - project.add_user(user, Gitlab::Access::MAINTAINER) - project.repository.create_file(user, file_path_1, 'something valid', - message: 'test 3', branch_name: 'master') - end + expect(response.status).to eq(200) + expect(body['name']).to eq('issue_template') + expect(body['content']).to eq('issue content') + end + end - describe '#show' do - it 'renders template name and content as json' do - get(:show, params: { namespace_id: project.namespace.to_param, template_type: "issue", key: "bug", project_id: project }, format: :json) + shared_examples 'renders merge request templates as json' do + it do + get(:show, params: { namespace_id: project.namespace, template_type: 'merge_request', key: 'merge_request_template', project_id: project }, format: :json) + + expect(response.status).to eq(200) + expect(body['name']).to eq('merge_request_template') + expect(body['content']).to eq('merge request content') + end + end + + shared_examples 'renders 404 when requesting an issue template' do + it do + get(:show, params: { namespace_id: project.namespace, template_type: 'issue', key: 'issue_template', project_id: project }, format: :json) + + expect(response.status).to eq(404) + end + end + + shared_examples 'renders 404 when requesting a merge request template' do + it do + get(:show, params: { namespace_id: project.namespace, template_type: 'merge_request', key: 'merge_request_template', project_id: project }, format: :json) - expect(response.status).to eq(200) - expect(body["name"]).to eq("bug") - expect(body["content"]).to eq("something valid") + expect(response.status).to eq(404) + end end - it 'renders 404 when unauthorized' do - sign_in(user2) - get(:show, params: { namespace_id: project.namespace.to_param, template_type: "issue", key: "bug", project_id: project }, format: :json) + shared_examples 'renders 404 when params are invalid' do + it 'does not route when the template type is invalid' do + expect do + get(:show, params: { namespace_id: project.namespace, template_type: 'invalid_type', key: 'issue_template', project_id: project }, format: :json) + end.to raise_error(ActionController::UrlGenerationError) + end + + it 'renders 404 when the format type is invalid' do + get(:show, params: { namespace_id: project.namespace, template_type: 'issue', key: 'issue_template', project_id: project }, format: :html) + + expect(response.status).to eq(404) + end + + it 'renders 404 when the key is unknown' do + get(:show, params: { namespace_id: project.namespace, template_type: 'issue', key: 'unknown_template', project_id: project }, format: :json) - expect(response.status).to eq(404) + expect(response.status).to eq(404) + end end - it 'renders 404 when template type is not found' do - sign_in(user) - get(:show, params: { namespace_id: project.namespace.to_param, template_type: "dont_exist", key: "bug", project_id: project }, format: :json) + context 'when the user is not a member of the project' do + before do + sign_in(user) + end - expect(response.status).to eq(404) + include_examples 'renders 404 when requesting an issue template' + include_examples 'renders 404 when requesting a merge request template' + include_examples 'renders 404 when params are invalid' end - it 'renders 404 without errors' do - sign_in(user) - expect { get(:show, params: { namespace_id: project.namespace.to_param, template_type: "dont_exist", key: "bug", project_id: project }, format: :json) }.not_to raise_error + context 'when user is a member of the project' do + before do + project.add_developer(user) + sign_in(user) + end + + include_examples 'renders issue templates as json' + include_examples 'renders merge request templates as json' + include_examples 'renders 404 when params are invalid' + end + + context 'when user is a guest of the project' do + before do + project.add_guest(user) + sign_in(user) + end + + include_examples 'renders issue templates as json' + include_examples 'renders 404 when requesting a merge request template' + include_examples 'renders 404 when params are invalid' end end end diff --git a/spec/controllers/snippets/notes_controller_spec.rb b/spec/controllers/snippets/notes_controller_spec.rb index 936d7c7dae4..586d59c2d09 100644 --- a/spec/controllers/snippets/notes_controller_spec.rb +++ b/spec/controllers/snippets/notes_controller_spec.rb @@ -119,6 +119,119 @@ describe Snippets::NotesController do end end + describe 'POST create' do + context 'when a snippet is public' do + let(:request_params) do + { + note: attributes_for(:note_on_personal_snippet, noteable: public_snippet), + snippet_id: public_snippet.id + } + end + + before do + sign_in user + end + + it 'returns status 302' do + post :create, params: request_params + + expect(response).to have_gitlab_http_status(302) + end + + it 'creates the note' do + expect { post :create, params: request_params }.to change { Note.count }.by(1) + end + end + + context 'when a snippet is internal' do + let(:request_params) do + { + note: attributes_for(:note_on_personal_snippet, noteable: internal_snippet), + snippet_id: internal_snippet.id + } + end + + before do + sign_in user + end + + it 'returns status 302' do + post :create, params: request_params + + expect(response).to have_gitlab_http_status(302) + end + + it 'creates the note' do + expect { post :create, params: request_params }.to change { Note.count }.by(1) + end + end + + context 'when a snippet is private' do + let(:request_params) do + { + note: attributes_for(:note_on_personal_snippet, noteable: private_snippet), + snippet_id: private_snippet.id + } + end + + before do + sign_in user + end + + context 'when user is not the author' do + before do + sign_in(user) + end + + it 'returns status 404' do + post :create, params: request_params + + expect(response).to have_gitlab_http_status(404) + end + + it 'does not create the note' do + expect { post :create, params: request_params }.not_to change { Note.count } + end + + context 'when user sends a snippet_id for a public snippet' do + let(:request_params) do + { + note: attributes_for(:note_on_personal_snippet, noteable: private_snippet), + snippet_id: public_snippet.id + } + end + + it 'returns status 302' do + post :create, params: request_params + + expect(response).to have_gitlab_http_status(302) + end + + it 'creates the note on the public snippet' do + expect { post :create, params: request_params }.to change { Note.count }.by(1) + expect(Note.last.noteable).to eq public_snippet + end + end + end + + context 'when user is the author' do + before do + sign_in(private_snippet.author) + end + + it 'returns status 302' do + post :create, params: request_params + + expect(response).to have_gitlab_http_status(302) + end + + it 'creates the note' do + expect { post :create, params: request_params }.to change { Note.count }.by(1) + end + end + end + end + describe 'DELETE destroy' do let(:request_params) do { diff --git a/spec/controllers/snippets_controller_spec.rb b/spec/controllers/snippets_controller_spec.rb index f8666a1986f..3aba02bf3ff 100644 --- a/spec/controllers/snippets_controller_spec.rb +++ b/spec/controllers/snippets_controller_spec.rb @@ -209,8 +209,8 @@ describe SnippetsController do context 'when the snippet description contains a file' do include FileMoverHelpers - let(:picture_file) { '/-/system/temp/secret56/picture.jpg' } - let(:text_file) { '/-/system/temp/secret78/text.txt' } + let(:picture_file) { "/-/system/user/#{user.id}/secret56/picture.jpg" } + let(:text_file) { "/-/system/user/#{user.id}/secret78/text.txt" } let(:description) do "Description with picture:  and "\ "text: [text.txt](/uploads#{text_file})" diff --git a/spec/controllers/uploads_controller_spec.rb b/spec/controllers/uploads_controller_spec.rb index d27658e02cb..0876502a899 100644 --- a/spec/controllers/uploads_controller_spec.rb +++ b/spec/controllers/uploads_controller_spec.rb @@ -24,121 +24,160 @@ describe UploadsController do let!(:user) { create(:user, avatar: fixture_file_upload("spec/fixtures/dk.png", "image/png")) } describe 'POST create' do - let(:model) { 'personal_snippet' } - let(:snippet) { create(:personal_snippet, :public) } let(:jpg) { fixture_file_upload('spec/fixtures/rails_sample.jpg', 'image/jpg') } let(:txt) { fixture_file_upload('spec/fixtures/doc_sample.txt', 'text/plain') } - context 'when a user does not have permissions to upload a file' do - it "returns 401 when the user is not logged in" do - post :create, params: { model: model, id: snippet.id }, format: :json + context 'snippet uploads' do + let(:model) { 'personal_snippet' } + let(:snippet) { create(:personal_snippet, :public) } - expect(response).to have_gitlab_http_status(401) - end + context 'when a user does not have permissions to upload a file' do + it "returns 401 when the user is not logged in" do + post :create, params: { model: model, id: snippet.id }, format: :json - it "returns 404 when user can't comment on a snippet" do - private_snippet = create(:personal_snippet, :private) + expect(response).to have_gitlab_http_status(401) + end - sign_in(user) - post :create, params: { model: model, id: private_snippet.id }, format: :json + it "returns 404 when user can't comment on a snippet" do + private_snippet = create(:personal_snippet, :private) - expect(response).to have_gitlab_http_status(404) - end - end + sign_in(user) + post :create, params: { model: model, id: private_snippet.id }, format: :json - context 'when a user is logged in' do - before do - sign_in(user) + expect(response).to have_gitlab_http_status(404) + end end - it "returns an error without file" do - post :create, params: { model: model, id: snippet.id }, format: :json + context 'when a user is logged in' do + before do + sign_in(user) + end - expect(response).to have_gitlab_http_status(422) - end + it "returns an error without file" do + post :create, params: { model: model, id: snippet.id }, format: :json - it "returns an error with invalid model" do - expect { post :create, params: { model: 'invalid', id: snippet.id }, format: :json } - .to raise_error(ActionController::UrlGenerationError) - end + expect(response).to have_gitlab_http_status(422) + end - it "returns 404 status when object not found" do - post :create, params: { model: model, id: 9999 }, format: :json + it "returns an error with invalid model" do + expect { post :create, params: { model: 'invalid', id: snippet.id }, format: :json } + .to raise_error(ActionController::UrlGenerationError) + end - expect(response).to have_gitlab_http_status(404) - end + it "returns 404 status when object not found" do + post :create, params: { model: model, id: 9999 }, format: :json - context 'with valid image' do - before do - post :create, params: { model: 'personal_snippet', id: snippet.id, file: jpg }, format: :json + expect(response).to have_gitlab_http_status(404) end - it 'returns a content with original filename, new link, and correct type.' do - expect(response.body).to match '\"alt\":\"rails_sample\"' - expect(response.body).to match "\"url\":\"/uploads" + context 'with valid image' do + before do + post :create, params: { model: 'personal_snippet', id: snippet.id, file: jpg }, format: :json + end + + it 'returns a content with original filename, new link, and correct type.' do + expect(response.body).to match '\"alt\":\"rails_sample\"' + expect(response.body).to match "\"url\":\"/uploads" + end + + it 'creates a corresponding Upload record' do + upload = Upload.last + + aggregate_failures do + expect(upload).to exist + expect(upload.model).to eq snippet + end + end end - it 'creates a corresponding Upload record' do - upload = Upload.last + context 'with valid non-image file' do + before do + post :create, params: { model: 'personal_snippet', id: snippet.id, file: txt }, format: :json + end - aggregate_failures do - expect(upload).to exist - expect(upload.model).to eq snippet + it 'returns a content with original filename, new link, and correct type.' do + expect(response.body).to match '\"alt\":\"doc_sample.txt\"' + expect(response.body).to match "\"url\":\"/uploads" + end + + it 'creates a corresponding Upload record' do + upload = Upload.last + + aggregate_failures do + expect(upload).to exist + expect(upload.model).to eq snippet + end end end end + end + + context 'user uploads' do + let(:model) { 'user' } + + it 'returns 401 when the user has no access' do + post :create, params: { model: 'user', id: user.id }, format: :json - context 'with valid non-image file' do + expect(response).to have_gitlab_http_status(401) + end + + context 'when user is logged in' do before do - post :create, params: { model: 'personal_snippet', id: snippet.id, file: txt }, format: :json + sign_in(user) + end + + subject do + post :create, params: { model: model, id: user.id, file: jpg }, format: :json end it 'returns a content with original filename, new link, and correct type.' do - expect(response.body).to match '\"alt\":\"doc_sample.txt\"' - expect(response.body).to match "\"url\":\"/uploads" + subject + + expect(response.body).to match '\"alt\":\"rails_sample\"' + expect(response.body).to match "\"url\":\"/uploads/-/system/user/#{user.id}/" end it 'creates a corresponding Upload record' do + expect { subject }.to change { Upload.count } + upload = Upload.last aggregate_failures do expect(upload).to exist - expect(upload.model).to eq snippet + expect(upload.model).to eq user end end - end - context 'temporal with valid image' do - subject do - post :create, params: { model: 'personal_snippet', file: jpg }, format: :json - end + context 'with valid non-image file' do + subject do + post :create, params: { model: model, id: user.id, file: txt }, format: :json + end - it 'returns a content with original filename, new link, and correct type.' do - subject + it 'returns a content with original filename, new link, and correct type.' do + subject - expect(response.body).to match '\"alt\":\"rails_sample\"' - expect(response.body).to match "\"url\":\"/uploads/-/system/temp" - end + expect(response.body).to match '\"alt\":\"doc_sample.txt\"' + expect(response.body).to match "\"url\":\"/uploads/-/system/user/#{user.id}/" + end - it 'does not create an Upload record' do - expect { subject }.not_to change { Upload.count } - end - end + it 'creates a corresponding Upload record' do + expect { subject }.to change { Upload.count } - context 'temporal with valid non-image file' do - subject do - post :create, params: { model: 'personal_snippet', file: txt }, format: :json + upload = Upload.last + + aggregate_failures do + expect(upload).to exist + expect(upload.model).to eq user + end + end end - it 'returns a content with original filename, new link, and correct type.' do - subject + it 'returns 404 when given user is not the logged in one' do + another_user = create(:user) - expect(response.body).to match '\"alt\":\"doc_sample.txt\"' - expect(response.body).to match "\"url\":\"/uploads/-/system/temp" - end + post :create, params: { model: model, id: another_user.id, file: txt }, format: :json - it 'does not create an Upload record' do - expect { subject }.not_to change { Upload.count } + expect(response).to have_gitlab_http_status(404) end end end diff --git a/spec/features/snippets/user_creates_snippet_spec.rb b/spec/features/snippets/user_creates_snippet_spec.rb index 1c97d5ec5b4..93d77d5b5ce 100644 --- a/spec/features/snippets/user_creates_snippet_spec.rb +++ b/spec/features/snippets/user_creates_snippet_spec.rb @@ -41,7 +41,7 @@ describe 'User creates snippet', :js do expect(page).to have_content('My Snippet') link = find('a.no-attachment-icon img[alt="banana_sample"]')['src'] - expect(link).to match(%r{/uploads/-/system/temp/\h{32}/banana_sample\.gif\z}) + expect(link).to match(%r{/uploads/-/system/user/#{user.id}/\h{32}/banana_sample\.gif\z}) reqs = inspect_requests { visit(link) } expect(reqs.first.status_code).to eq(200) diff --git a/spec/graphql/types/label_type_spec.rb b/spec/graphql/types/label_type_spec.rb index f498b32f9ed..8e7b2c69eff 100644 --- a/spec/graphql/types/label_type_spec.rb +++ b/spec/graphql/types/label_type_spec.rb @@ -7,4 +7,6 @@ describe GitlabSchema.types['Label'] do is_expected.to have_graphql_fields(*expected_fields) end + + it { is_expected.to require_graphql_authorizations(:read_label) } end diff --git a/spec/graphql/types/metadata_type_spec.rb b/spec/graphql/types/metadata_type_spec.rb index 55205bf5b6a..5236380e477 100644 --- a/spec/graphql/types/metadata_type_spec.rb +++ b/spec/graphql/types/metadata_type_spec.rb @@ -2,4 +2,5 @@ require 'spec_helper' describe GitlabSchema.types['Metadata'] do it { expect(described_class.graphql_name).to eq('Metadata') } + it { is_expected.to require_graphql_authorizations(:read_instance_metadata) } end diff --git a/spec/graphql/types/namespace_type_spec.rb b/spec/graphql/types/namespace_type_spec.rb index 77fd590586e..e1153832cc9 100644 --- a/spec/graphql/types/namespace_type_spec.rb +++ b/spec/graphql/types/namespace_type_spec.rb @@ -13,4 +13,6 @@ describe GitlabSchema.types['Namespace'] do is_expected.to have_graphql_fields(*expected_fields) end + + it { is_expected.to require_graphql_authorizations(:read_namespace) } end diff --git a/spec/graphql/types/query_type_spec.rb b/spec/graphql/types/query_type_spec.rb index af1972a2513..bc3b8a42392 100644 --- a/spec/graphql/types/query_type_spec.rb +++ b/spec/graphql/types/query_type_spec.rb @@ -34,9 +34,5 @@ describe GitlabSchema.types['Query'] do is_expected.to have_graphql_type(Types::MetadataType) is_expected.to have_graphql_resolver(Resolvers::MetadataResolver) end - - it 'authorizes with read_instance_metadata' do - is_expected.to require_graphql_authorizations(:read_instance_metadata) - end end end diff --git a/spec/lib/banzai/filter/relative_link_filter_spec.rb b/spec/lib/banzai/filter/relative_link_filter_spec.rb index 8ff971114d6..a714fa50f5f 100644 --- a/spec/lib/banzai/filter/relative_link_filter_spec.rb +++ b/spec/lib/banzai/filter/relative_link_filter_spec.rb @@ -83,6 +83,11 @@ describe Banzai::Filter::RelativeLinkFilter do expect { filter(act) }.not_to raise_error end + it 'does not explode with an escaped null byte' do + act = link("/%00") + expect { filter(act) }.not_to raise_error + end + it 'does not raise an exception with a space in the path' do act = link("/uploads/d18213acd3732630991986120e167e3d/Landscape_8.jpg \nBut here's some more unexpected text :smile:)") expect { filter(act) }.not_to raise_error diff --git a/spec/lib/gitlab/ci/config_spec.rb b/spec/lib/gitlab/ci/config_spec.rb index 7f336ee853e..4e8bff3d738 100644 --- a/spec/lib/gitlab/ci/config_spec.rb +++ b/spec/lib/gitlab/ci/config_spec.rb @@ -90,6 +90,27 @@ describe Gitlab::Ci::Config do end end + context 'when yml is too big' do + let(:yml) do + <<~YAML + --- &1 + - hi + - *1 + YAML + end + + describe '.new' do + it 'raises error' do + expect(Gitlab::Sentry).to receive(:track_exception) + + expect { config }.to raise_error( + described_class::ConfigError, + /The parsed YAML is too big/ + ) + end + end + end + context 'when config logic is incorrect' do let(:yml) { 'before_script: "ls"' } diff --git a/spec/lib/gitlab/config/loader/yaml_spec.rb b/spec/lib/gitlab/config/loader/yaml_spec.rb index 44c9a3896a8..0affeb01f6a 100644 --- a/spec/lib/gitlab/config/loader/yaml_spec.rb +++ b/spec/lib/gitlab/config/loader/yaml_spec.rb @@ -8,7 +8,7 @@ describe Gitlab::Config::Loader::Yaml do describe '#valid?' do it 'returns true' do - expect(loader.valid?).to be true + expect(loader).to be_valid end end @@ -24,7 +24,7 @@ describe Gitlab::Config::Loader::Yaml do describe '#valid?' do it 'returns false' do - expect(loader.valid?).to be false + expect(loader).not_to be_valid end end @@ -43,7 +43,10 @@ describe Gitlab::Config::Loader::Yaml do describe '#initialize' do it 'raises FormatError' do - expect { loader }.to raise_error(Gitlab::Config::Loader::FormatError, 'Unknown alias: bad_alias') + expect { loader }.to raise_error( + Gitlab::Config::Loader::FormatError, + 'Unknown alias: bad_alias' + ) end end end @@ -53,7 +56,68 @@ describe Gitlab::Config::Loader::Yaml do describe '#valid?' do it 'returns false' do - expect(loader.valid?).to be false + expect(loader).not_to be_valid + end + end + end + + # Prevent Billion Laughs attack: https://gitlab.com/gitlab-org/gitlab-ce/issues/56018 + context 'when yaml size is too large' do + let(:yml) do + <<~YAML + a: &a ["lol","lol","lol","lol","lol","lol","lol","lol","lol"] + b: &b [*a,*a,*a,*a,*a,*a,*a,*a,*a] + c: &c [*b,*b,*b,*b,*b,*b,*b,*b,*b] + d: &d [*c,*c,*c,*c,*c,*c,*c,*c,*c] + e: &e [*d,*d,*d,*d,*d,*d,*d,*d,*d] + f: &f [*e,*e,*e,*e,*e,*e,*e,*e,*e] + g: &g [*f,*f,*f,*f,*f,*f,*f,*f,*f] + h: &h [*g,*g,*g,*g,*g,*g,*g,*g,*g] + i: &i [*h,*h,*h,*h,*h,*h,*h,*h,*h] + YAML + end + + describe '#valid?' do + it 'returns false' do + expect(loader).not_to be_valid + end + + it 'returns true if "ci_yaml_limit_size" feature flag is disabled' do + stub_feature_flags(ci_yaml_limit_size: false) + + expect(loader).to be_valid + end + end + + describe '#load!' do + it 'raises FormatError' do + expect { loader.load! }.to raise_error( + Gitlab::Config::Loader::FormatError, + 'The parsed YAML is too big' + ) + end + end + end + + # Prevent Billion Laughs attack: https://gitlab.com/gitlab-org/gitlab-ce/issues/56018 + context 'when yaml has cyclic data structure' do + let(:yml) do + <<~YAML + --- &1 + - hi + - *1 + YAML + end + + describe '#valid?' do + it 'returns false' do + expect(loader.valid?).to be(false) + end + end + + describe '#load!' do + it 'raises FormatError' do + expect { loader.load! }.to raise_error(Gitlab::Config::Loader::FormatError, 'The parsed YAML is too big') end end end diff --git a/spec/lib/gitlab/graphql/authorize/authorize_field_service_spec.rb b/spec/lib/gitlab/graphql/authorize/authorize_field_service_spec.rb index aec9c4baf0a..d60d1b7559a 100644 --- a/spec/lib/gitlab/graphql/authorize/authorize_field_service_spec.rb +++ b/spec/lib/gitlab/graphql/authorize/authorize_field_service_spec.rb @@ -7,35 +7,39 @@ require 'spec_helper' describe Gitlab::Graphql::Authorize::AuthorizeFieldService do def type(type_authorizations = []) Class.new(Types::BaseObject) do - graphql_name "TestType" + graphql_name 'TestType' authorize type_authorizations end end - def type_with_field(field_type, field_authorizations = [], resolved_value = "Resolved value") + def type_with_field(field_type, field_authorizations = [], resolved_value = 'Resolved value', **options) Class.new(Types::BaseObject) do - graphql_name "TestTypeWithField" - field :test_field, field_type, null: true, authorize: field_authorizations, resolve: -> (_, _, _) { resolved_value} + graphql_name 'TestTypeWithField' + options.reverse_merge!(null: true) + field :test_field, field_type, + authorize: field_authorizations, + resolve: -> (_, _, _) { resolved_value }, + **options end end let(:current_user) { double(:current_user) } subject(:service) { described_class.new(field) } - describe "#authorized_resolve" do - let(:presented_object) { double("presented object") } - let(:presented_type) { double("parent type", object: presented_object) } + describe '#authorized_resolve' do + let(:presented_object) { double('presented object') } + let(:presented_type) { double('parent type', object: presented_object) } subject(:resolved) { service.authorized_resolve.call(presented_type, {}, { current_user: current_user }) } - context "scalar types" do - shared_examples "checking permissions on the presented object" do - it "checks the abilities on the object being presented and returns the value" do + context 'scalar types' do + shared_examples 'checking permissions on the presented object' do + it 'checks the abilities on the object being presented and returns the value' do expected_permissions.each do |permission| spy_ability_check_for(permission, presented_object, passed: true) end - expect(resolved).to eq("Resolved value") + expect(resolved).to eq('Resolved value') end it "returns nil if the value wasn't authorized" do @@ -45,61 +49,71 @@ describe Gitlab::Graphql::Authorize::AuthorizeFieldService do end end - context "when the field is a built-in scalar type" do - let(:field) { type_with_field(GraphQL::STRING_TYPE, :read_field).fields["testField"].to_graphql } + context 'when the field is a built-in scalar type' do + let(:field) { type_with_field(GraphQL::STRING_TYPE, :read_field).fields['testField'].to_graphql } let(:expected_permissions) { [:read_field] } - it_behaves_like "checking permissions on the presented object" + it_behaves_like 'checking permissions on the presented object' end - context "when the field is a list of scalar types" do - let(:field) { type_with_field([GraphQL::STRING_TYPE], :read_field).fields["testField"].to_graphql } + context 'when the field is a list of scalar types' do + let(:field) { type_with_field([GraphQL::STRING_TYPE], :read_field).fields['testField'].to_graphql } let(:expected_permissions) { [:read_field] } - it_behaves_like "checking permissions on the presented object" + it_behaves_like 'checking permissions on the presented object' end - context "when the field is sub-classed scalar type" do - let(:field) { type_with_field(Types::TimeType, :read_field).fields["testField"].to_graphql } + context 'when the field is sub-classed scalar type' do + let(:field) { type_with_field(Types::TimeType, :read_field).fields['testField'].to_graphql } let(:expected_permissions) { [:read_field] } - it_behaves_like "checking permissions on the presented object" + it_behaves_like 'checking permissions on the presented object' end - context "when the field is a list of sub-classed scalar types" do - let(:field) { type_with_field([Types::TimeType], :read_field).fields["testField"].to_graphql } + context 'when the field is a list of sub-classed scalar types' do + let(:field) { type_with_field([Types::TimeType], :read_field).fields['testField'].to_graphql } let(:expected_permissions) { [:read_field] } - it_behaves_like "checking permissions on the presented object" + it_behaves_like 'checking permissions on the presented object' end end - context "when the field is a specific type" do + context 'when the field is a specific type' do let(:custom_type) { type(:read_type) } - let(:object_in_field) { double("presented in field") } - let(:field) { type_with_field(custom_type, :read_field, object_in_field).fields["testField"].to_graphql } + let(:object_in_field) { double('presented in field') } + let(:field) { type_with_field(custom_type, :read_field, object_in_field).fields['testField'].to_graphql } - it "checks both field & type permissions" do + it 'checks both field & type permissions' do spy_ability_check_for(:read_field, object_in_field, passed: true) spy_ability_check_for(:read_type, object_in_field, passed: true) expect(resolved).to eq(object_in_field) end - it "returns nil if viewing was not allowed" do + it 'returns nil if viewing was not allowed' do spy_ability_check_for(:read_field, object_in_field, passed: false) spy_ability_check_for(:read_type, object_in_field, passed: true) expect(resolved).to be_nil end - context "when the field is a list" do - let(:object_1) { double("presented in field 1") } - let(:object_2) { double("presented in field 2") } + context 'when the field is not nullable' do + let(:field) { type_with_field(custom_type, [], object_in_field, null: false).fields['testField'].to_graphql } + + it 'returns nil when viewing is not allowed' do + spy_ability_check_for(:read_type, object_in_field, passed: false) + + expect(resolved).to be_nil + end + end + + context 'when the field is a list' do + let(:object_1) { double('presented in field 1') } + let(:object_2) { double('presented in field 2') } let(:presented_types) { [double(object: object_1), double(object: object_2)] } - let(:field) { type_with_field([custom_type], :read_field, presented_types).fields["testField"].to_graphql } + let(:field) { type_with_field([custom_type], :read_field, presented_types).fields['testField'].to_graphql } - it "checks all permissions" do + it 'checks all permissions' do allow(Ability).to receive(:allowed?) { true } spy_ability_check_for(:read_field, object_1, passed: true) @@ -110,7 +124,7 @@ describe Gitlab::Graphql::Authorize::AuthorizeFieldService do expect(resolved).to eq(presented_types) end - it "filters out objects that the user cannot see" do + it 'filters out objects that the user cannot see' do allow(Ability).to receive(:allowed?) { true } spy_ability_check_for(:read_type, object_1, passed: false) diff --git a/spec/lib/gitlab/issuable_metadata_spec.rb b/spec/lib/gitlab/issuable_metadata_spec.rb index 916f3876a8e..032467b8b4e 100644 --- a/spec/lib/gitlab/issuable_metadata_spec.rb +++ b/spec/lib/gitlab/issuable_metadata_spec.rb @@ -7,11 +7,11 @@ describe Gitlab::IssuableMetadata do subject { Class.new { include Gitlab::IssuableMetadata }.new } it 'returns an empty Hash if an empty collection is provided' do - expect(subject.issuable_meta_data(Issue.none, 'Issue')).to eq({}) + expect(subject.issuable_meta_data(Issue.none, 'Issue', user)).to eq({}) end it 'raises an error when given a collection with no limit' do - expect { subject.issuable_meta_data(Issue.all, 'Issue') }.to raise_error(/must have a limit/) + expect { subject.issuable_meta_data(Issue.all, 'Issue', user) }.to raise_error(/must have a limit/) end context 'issues' do @@ -23,7 +23,7 @@ describe Gitlab::IssuableMetadata do let!(:closing_issues) { create(:merge_requests_closing_issues, issue: issue, merge_request: merge_request) } it 'aggregates stats on issues' do - data = subject.issuable_meta_data(Issue.all.limit(10), 'Issue') + data = subject.issuable_meta_data(Issue.all.limit(10), 'Issue', user) expect(data.count).to eq(2) expect(data[issue.id].upvotes).to eq(1) @@ -46,7 +46,7 @@ describe Gitlab::IssuableMetadata do let!(:note) { create(:note_on_merge_request, author: user, project: project, noteable: merge_request, note: "a comment on a MR") } it 'aggregates stats on merge requests' do - data = subject.issuable_meta_data(MergeRequest.all.limit(10), 'MergeRequest') + data = subject.issuable_meta_data(MergeRequest.all.limit(10), 'MergeRequest', user) expect(data.count).to eq(2) expect(data[merge_request.id].upvotes).to eq(1) diff --git a/spec/lib/gitlab/utils/deep_size_spec.rb b/spec/lib/gitlab/utils/deep_size_spec.rb new file mode 100644 index 00000000000..1e619a15980 --- /dev/null +++ b/spec/lib/gitlab/utils/deep_size_spec.rb @@ -0,0 +1,43 @@ +require 'spec_helper' + +describe Gitlab::Utils::DeepSize do + let(:data) do + { + a: [1, 2, 3], + b: { + c: [4, 5], + d: [ + { e: [[6], [7]] } + ] + } + } + end + + let(:max_size) { 1.kilobyte } + let(:max_depth) { 10 } + let(:deep_size) { described_class.new(data, max_size: max_size, max_depth: max_depth) } + + describe '#evaluate' do + context 'when data within size and depth limits' do + it 'returns true' do + expect(deep_size).to be_valid + end + end + + context 'when data not within size limit' do + let(:max_size) { 200.bytes } + + it 'returns false' do + expect(deep_size).not_to be_valid + end + end + + context 'when data not within depth limit' do + let(:max_depth) { 2 } + + it 'returns false' do + expect(deep_size).not_to be_valid + end + end + end +end diff --git a/spec/requests/api/graphql/namespace/projects_spec.rb b/spec/requests/api/graphql/namespace/projects_spec.rb index de1cd9586b6..63fa16c79ca 100644 --- a/spec/requests/api/graphql/namespace/projects_spec.rb +++ b/spec/requests/api/graphql/namespace/projects_spec.rb @@ -58,9 +58,7 @@ describe 'getting projects', :nested_groups do it 'finds only public projects' do post_graphql(query, current_user: nil) - expect(graphql_data['namespace']['projects']['edges'].size).to eq(1) - project = graphql_data['namespace']['projects']['edges'][0]['node'] - expect(project['id']).to eq(public_project.to_global_id.to_s) + expect(graphql_data['namespace']).to be_nil end end end diff --git a/spec/requests/api/graphql/project/repository_spec.rb b/spec/requests/api/graphql/project/repository_spec.rb index 67af612a4a0..261433a3d6a 100644 --- a/spec/requests/api/graphql/project/repository_spec.rb +++ b/spec/requests/api/graphql/project/repository_spec.rb @@ -34,4 +34,28 @@ describe 'getting a repository in a project' do expect(graphql_data['project']).to be(nil) end end + + context 'when the repository is only accessible to members' do + let(:project) do + create(:project, :public, :repository, repository_access_level: ProjectFeature::PRIVATE) + end + + it 'returns a repository for the owner' do + post_graphql(query, current_user: current_user) + + expect(graphql_data['project']['repository']).not_to be_nil + end + + it 'returns nil for the repository for other users' do + post_graphql(query, current_user: create(:user)) + + expect(graphql_data['project']['repository']).to be_nil + end + + it 'returns nil for the repository for other users' do + post_graphql(query, current_user: nil) + + expect(graphql_data['project']['repository']).to be_nil + end + end end diff --git a/spec/requests/api/issues/get_group_issues_spec.rb b/spec/requests/api/issues/get_group_issues_spec.rb index 8b02cf56e9f..9a41d790945 100644 --- a/spec/requests/api/issues/get_group_issues_spec.rb +++ b/spec/requests/api/issues/get_group_issues_spec.rb @@ -23,7 +23,11 @@ describe API::Issues do describe 'GET /groups/:id/issues' do let!(:group) { create(:group) } - let!(:group_project) { create(:project, :public, creator_id: user.id, namespace: group) } + let!(:group_project) { create(:project, :public, :repository, creator_id: user.id, namespace: group) } + let!(:private_mrs_project) do + create(:project, :public, :repository, creator_id: user.id, namespace: group, merge_requests_access_level: ProjectFeature::PRIVATE) + end + let!(:group_closed_issue) do create :closed_issue, author: user, @@ -234,6 +238,30 @@ describe API::Issues do it_behaves_like 'group issues statistics' end end + + context "when returns issue merge_requests_count for different access levels" do + let!(:merge_request1) do + create(:merge_request, + :simple, + author: user, + source_project: private_mrs_project, + target_project: private_mrs_project, + description: "closes #{group_issue.to_reference(private_mrs_project)}") + end + let!(:merge_request2) do + create(:merge_request, + :simple, + author: user, + source_project: group_project, + target_project: group_project, + description: "closes #{group_issue.to_reference}") + end + + it_behaves_like 'accessible merge requests count' do + let(:api_url) { base_url } + let(:target_issue) { group_issue } + end + end end end diff --git a/spec/requests/api/issues/get_project_issues_spec.rb b/spec/requests/api/issues/get_project_issues_spec.rb index 0b0f754ab57..f7ca6fd1e0a 100644 --- a/spec/requests/api/issues/get_project_issues_spec.rb +++ b/spec/requests/api/issues/get_project_issues_spec.rb @@ -4,8 +4,9 @@ require 'spec_helper' describe API::Issues do set(:user) { create(:user) } - set(:project) do - create(:project, :public, creator_id: user.id, namespace: user.namespace) + set(:project) { create(:project, :public, :repository, creator_id: user.id, namespace: user.namespace) } + set(:private_mrs_project) do + create(:project, :public, :repository, creator_id: user.id, namespace: user.namespace, merge_requests_access_level: ProjectFeature::PRIVATE) end let(:user2) { create(:user) } @@ -60,9 +61,28 @@ describe API::Issues do let(:no_milestone_title) { 'None' } let(:any_milestone_title) { 'Any' } + let!(:merge_request1) do + create(:merge_request, + :simple, + author: user, + source_project: project, + target_project: project, + description: "closes #{issue.to_reference}") + end + let!(:merge_request2) do + create(:merge_request, + :simple, + author: user, + source_project: private_mrs_project, + target_project: private_mrs_project, + description: "closes #{issue.to_reference(private_mrs_project)}") + end + before(:all) do project.add_reporter(user) project.add_guest(guest) + private_mrs_project.add_reporter(user) + private_mrs_project.add_guest(guest) end before do @@ -257,6 +277,11 @@ describe API::Issues do expect_paginated_array_response(issue.id) end + it_behaves_like 'accessible merge requests count' do + let(:api_url) { "/projects/#{project.id}/issues" } + let(:target_issue) { issue } + end + context 'with labeled issues' do let(:label_b) { create(:label, title: 'foo', project: project) } let(:label_c) { create(:label, title: 'bar', project: project) } @@ -636,34 +661,26 @@ describe API::Issues do expect(json_response['iid']).to eq(confidential_issue.iid) end end - end - - describe 'GET :id/issues/:issue_iid/closed_by' do - let(:merge_request) do - create(:merge_request, - :simple, - author: user, - source_project: project, - target_project: project, - description: "closes #{issue.to_reference}") - end - before do - create(:merge_requests_closing_issues, issue: issue, merge_request: merge_request) + it_behaves_like 'accessible merge requests count' do + let(:api_url) { "/projects/#{project.id}/issues/#{issue.iid}" } + let(:target_issue) { issue } end + end + describe 'GET :id/issues/:issue_iid/closed_by' do context 'when unauthenticated' do it 'return public project issues' do get api("/projects/#{project.id}/issues/#{issue.iid}/closed_by") - expect_paginated_array_response(merge_request.id) + expect_paginated_array_response(merge_request1.id) end end it 'returns merge requests that will close issue on merge' do get api("/projects/#{project.id}/issues/#{issue.iid}/closed_by", user) - expect_paginated_array_response(merge_request.id) + expect_paginated_array_response(merge_request1.id) end context 'when no merge requests will close issue' do @@ -721,13 +738,6 @@ describe API::Issues do end it 'returns merge requests that mentioned a issue' do - create(:merge_request, - :simple, - author: user, - source_project: project, - target_project: project, - description: 'Some description') - get_related_merge_requests(project.id, issue.iid, user) expect_paginated_array_response(related_mr.id) diff --git a/spec/requests/api/issues/issues_spec.rb b/spec/requests/api/issues/issues_spec.rb index f32ffd1c77b..d195f54be11 100644 --- a/spec/requests/api/issues/issues_spec.rb +++ b/spec/requests/api/issues/issues_spec.rb @@ -4,8 +4,9 @@ require 'spec_helper' describe API::Issues do set(:user) { create(:user) } - set(:project) do - create(:project, :public, creator_id: user.id, namespace: user.namespace) + set(:project) { create(:project, :public, :repository, creator_id: user.id, namespace: user.namespace) } + set(:private_mrs_project) do + create(:project, :public, :repository, creator_id: user.id, namespace: user.namespace, merge_requests_access_level: ProjectFeature::PRIVATE) end let(:user2) { create(:user) } @@ -63,6 +64,8 @@ describe API::Issues do before(:all) do project.add_reporter(user) project.add_guest(guest) + private_mrs_project.add_reporter(user) + private_mrs_project.add_guest(guest) end before do @@ -725,6 +728,30 @@ describe API::Issues do end end end + + context "when returns issue merge_requests_count for different access levels" do + let!(:merge_request1) do + create(:merge_request, + :simple, + author: user, + source_project: private_mrs_project, + target_project: private_mrs_project, + description: "closes #{issue.to_reference(private_mrs_project)}") + end + let!(:merge_request2) do + create(:merge_request, + :simple, + author: user, + source_project: project, + target_project: project, + description: "closes #{issue.to_reference}") + end + + it_behaves_like 'accessible merge requests count' do + let(:api_url) { "/issues" } + let(:target_issue) { issue } + end + end end describe 'DELETE /projects/:id/issues/:issue_iid' do diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index 76d093e0774..a82ecb4fd63 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -834,6 +834,31 @@ describe API::MergeRequests do end end + context 'head_pipeline' do + before do + merge_request.update(head_pipeline: create(:ci_pipeline)) + merge_request.project.project_feature.update(builds_access_level: 10) + end + + context 'when user can read the pipeline' do + it 'exposes pipeline information' do + get api("/projects/#{project.id}/merge_requests/#{merge_request.iid}", user) + + expect(json_response).to include('head_pipeline') + end + end + + context 'when user can not read the pipeline' do + let(:guest) { create(:user) } + + it 'does not expose pipeline information' do + get api("/projects/#{project.id}/merge_requests/#{merge_request.iid}", guest) + + expect(json_response).not_to include('head_pipeline') + end + end + end + it 'returns the commits behind the target branch when include_diverged_commits_count is present' do allow_any_instance_of(merge_request.class).to receive(:diverged_commits_count).and_return(1) diff --git a/spec/routing/project_routing_spec.rb b/spec/routing/project_routing_spec.rb index 83775b1040e..6dde40d1cb6 100644 --- a/spec/routing/project_routing_spec.rb +++ b/spec/routing/project_routing_spec.rb @@ -693,4 +693,24 @@ describe 'project routing' do it_behaves_like 'redirecting a legacy project path', "/gitlab/gitlabhq/settings/repository", "/gitlab/gitlabhq/-/settings/repository" end + + describe Projects::TemplatesController, 'routing' do + describe '#show' do + def show_with_template_type(template_type) + "/gitlab/gitlabhq/templates/#{template_type}/template_name" + end + + it 'routes when :template_type is `merge_request`' do + expect(get(show_with_template_type('merge_request'))).to route_to('projects/templates#show', namespace_id: 'gitlab', project_id: 'gitlabhq', template_type: 'merge_request', key: 'template_name', format: 'json') + end + + it 'routes when :template_type is `issue`' do + expect(get(show_with_template_type('issue'))).to route_to('projects/templates#show', namespace_id: 'gitlab', project_id: 'gitlabhq', template_type: 'issue', key: 'template_name', format: 'json') + end + + it 'routes to application#route_not_found when :template_type is unknown' do + expect(get(show_with_template_type('invalid'))).to route_to('application#route_not_found', unmatched_route: 'gitlab/gitlabhq/templates/invalid/template_name') + end + end + end end diff --git a/spec/routing/uploads_routing_spec.rb b/spec/routing/uploads_routing_spec.rb index 6a041ffdd6c..42e84774088 100644 --- a/spec/routing/uploads_routing_spec.rb +++ b/spec/routing/uploads_routing_spec.rb @@ -12,10 +12,19 @@ describe 'Uploads', 'routing' do ) end + it 'allows creating uploads for users' do + expect(post('/uploads/user?id=1')).to route_to( + controller: 'uploads', + action: 'create', + model: 'user', + id: '1' + ) + end + it 'does not allow creating uploads for other models' do - UploadsController::MODEL_CLASSES.keys.compact.each do |model| - next if model == 'personal_snippet' + unroutable_models = UploadsController::MODEL_CLASSES.keys.compact - %w(personal_snippet user) + unroutable_models.each do |model| expect(post("/uploads/#{model}?id=1")).not_to be_routable end end diff --git a/spec/rubocop/cop/graphql/authorize_types_spec.rb b/spec/rubocop/cop/graphql/authorize_types_spec.rb new file mode 100644 index 00000000000..eae3e176d64 --- /dev/null +++ b/spec/rubocop/cop/graphql/authorize_types_spec.rb @@ -0,0 +1,66 @@ +# frozen_string_literal: true + +require 'fast_spec_helper' +require 'rubocop' +require 'rubocop/rspec/support' +require_relative '../../../../rubocop/cop/graphql/authorize_types' + +describe RuboCop::Cop::Graphql::AuthorizeTypes do + include RuboCop::RSpec::ExpectOffense + include CopHelper + + subject(:cop) { described_class.new } + + context 'when in a type folder' do + before do + allow(cop).to receive(:in_type?).and_return(true) + end + + it 'adds an offense when there is no authorize call' do + inspect_source(<<~TYPE) + module Types + class AType < BaseObject + field :a_thing + field :another_thing + end + end + TYPE + + expect(cop.offenses.size).to eq 1 + end + + it 'does not add an offense for classes that have an authorize call' do + expect_no_offenses(<<~TYPE.strip) + module Types + class AType < BaseObject + graphql_name 'ATypeName' + + authorize :an_ability, :second_ability + + field :a_thing + end + end + TYPE + end + + it 'does not add an offense for classes that only have an authorize call' do + expect_no_offenses(<<~TYPE.strip) + module Types + class AType < SuperClassWithFields + authorize :an_ability + end + end + TYPE + end + + it 'does not add an offense for base types' do + expect_no_offenses(<<~TYPE) + module Types + class AType < BaseEnum + field :a_thing + end + end + TYPE + end + end +end diff --git a/spec/support/shared_examples/requests/api/issues/merge_requests_count_shared_examples.rb b/spec/support/shared_examples/requests/api/issues/merge_requests_count_shared_examples.rb new file mode 100644 index 00000000000..5f4e178f2e5 --- /dev/null +++ b/spec/support/shared_examples/requests/api/issues/merge_requests_count_shared_examples.rb @@ -0,0 +1,37 @@ +def get_issue + json_response.is_a?(Array) ? json_response.detect {|issue| issue['id'] == target_issue.id} : json_response +end + +shared_examples 'accessible merge requests count' do + it 'returns anonymous accessible merge requests count' do + get api(api_url), params: { scope: 'all' } + + issue = get_issue + expect(issue).not_to be_nil + expect(issue['merge_requests_count']).to eq(1) + end + + it 'returns guest accessible merge requests count' do + get api(api_url, guest), params: { scope: 'all' } + + issue = get_issue + expect(issue).not_to be_nil + expect(issue['merge_requests_count']).to eq(1) + end + + it 'returns reporter accessible merge requests count' do + get api(api_url, user), params: { scope: 'all' } + + issue = get_issue + expect(issue).not_to be_nil + expect(issue['merge_requests_count']).to eq(2) + end + + it 'returns admin accessible merge requests count' do + get api(api_url, admin), params: { scope: 'all' } + + issue = get_issue + expect(issue).not_to be_nil + expect(issue['merge_requests_count']).to eq(2) + end +end diff --git a/spec/uploaders/file_mover_spec.rb b/spec/uploaders/file_mover_spec.rb index e474a714b10..a9e03f3d4e5 100644 --- a/spec/uploaders/file_mover_spec.rb +++ b/spec/uploaders/file_mover_spec.rb @@ -3,79 +3,140 @@ require 'spec_helper' describe FileMover do include FileMoverHelpers + let(:user) { create(:user) } let(:filename) { 'banana_sample.gif' } - let(:temp_file_path) { File.join('uploads/-/system/temp', 'secret55', filename) } + let(:secret) { 'secret55' } + let(:temp_file_path) { File.join("uploads/-/system/user/#{user.id}", secret, filename) } let(:temp_description) do "test  "\ "same  " end - let(:file_path) { File.join('uploads/-/system/personal_snippet', snippet.id.to_s, 'secret55', filename) } + let(:file_path) { File.join('uploads/-/system/personal_snippet', snippet.id.to_s, secret, filename) } let(:snippet) { create(:personal_snippet, description: temp_description) } - subject { described_class.new(temp_file_path, snippet).execute } + let(:tmp_uploader) do + PersonalFileUploader.new(user, secret: secret) + end + + let(:file) { fixture_file_upload('spec/fixtures/banana_sample.gif') } + subject { described_class.new(temp_file_path, from_model: user, to_model: snippet).execute } describe '#execute' do - before do - expect(FileUtils).to receive(:mkdir_p).with(a_string_including(File.dirname(file_path))) - expect(FileUtils).to receive(:move).with(a_string_including(temp_file_path), a_string_including(file_path)) - allow_any_instance_of(CarrierWave::SanitizedFile).to receive(:exists?).and_return(true) - allow_any_instance_of(CarrierWave::SanitizedFile).to receive(:size).and_return(10) + let(:tmp_upload) { tmp_uploader.upload } - stub_file_mover(temp_file_path) + before do + tmp_uploader.store!(file) end - context 'when move and field update successful' do - it 'updates the description correctly' do - subject + context 'local storage' do + before do + allow(FileUtils).to receive(:mkdir_p).with(a_string_including(File.dirname(file_path))) + allow(FileUtils).to receive(:move).with(a_string_including(temp_file_path), a_string_including(file_path)) + allow_any_instance_of(CarrierWave::SanitizedFile).to receive(:exists?).and_return(true) + allow_any_instance_of(CarrierWave::SanitizedFile).to receive(:size).and_return(10) - expect(snippet.reload.description) - .to eq( - "test  "\ - "same  " - ) + stub_file_mover(temp_file_path) end - it 'creates a new update record' do - expect { subject }.to change { Upload.count }.by(1) + context 'when move and field update successful' do + it 'updates the description correctly' do + subject + + expect(snippet.reload.description) + .to eq("test  "\ + "same  ") + end + + it 'updates existing upload record' do + expect { subject } + .to change { tmp_upload.reload.attributes.values_at('model_id', 'model_type') } + .from([user.id, 'User']).to([snippet.id, 'Snippet']) + end + + it 'schedules a background migration' do + expect_any_instance_of(PersonalFileUploader).to receive(:schedule_background_upload).once + + subject + end end - it 'schedules a background migration' do - expect_any_instance_of(PersonalFileUploader).to receive(:schedule_background_upload).once + context 'when update_markdown fails' do + before do + expect(FileUtils).to receive(:move).with(a_string_including(file_path), a_string_including(temp_file_path)) + end - subject + subject { described_class.new(file_path, :non_existing_field, from_model: user, to_model: snippet).execute } + + it 'does not update the description' do + subject + + expect(snippet.reload.description) + .to eq("test  "\ + "same  ") + end + + it 'does not change the upload record' do + expect { subject } + .not_to change { tmp_upload.reload.attributes.values_at('model_id', 'model_type') } + end end end - context 'when update_markdown fails' do + context 'when tmp uploader is not local storage' do before do - expect(FileUtils).to receive(:move).with(a_string_including(file_path), a_string_including(temp_file_path)) + allow(PersonalFileUploader).to receive(:object_store_enabled?) { true } + tmp_uploader.object_store = ObjectStorage::Store::REMOTE + allow_any_instance_of(PersonalFileUploader).to receive(:file_storage?) { false } end - subject { described_class.new(file_path, snippet, :non_existing_field).execute } + after do + FileUtils.rm_f(File.join('personal_snippet', snippet.id.to_s, secret, filename)) + end - it 'does not update the description' do - subject + context 'when move and field update successful' do + it 'updates the description correctly' do + subject - expect(snippet.reload.description) - .to eq( - "test  "\ - "same  " - ) + expect(snippet.reload.description) + .to eq("test  "\ + "same  ") + end + + it 'creates new target upload record an delete the old upload' do + expect { subject } + .to change { Upload.last.attributes.values_at('model_id', 'model_type') } + .from([user.id, 'User']).to([snippet.id, 'Snippet']) + + expect(Upload.count).to eq(1) + end end - it 'does not create a new update record' do - expect { subject }.not_to change { Upload.count } + context 'when update_markdown fails' do + subject { described_class.new(file_path, :non_existing_field, from_model: user, to_model: snippet).execute } + + it 'does not update the description' do + subject + + expect(snippet.reload.description) + .to eq("test  "\ + "same  ") + end + + it 'does not change the upload record' do + expect { subject } + .to change { Upload.last.attributes.values_at('model_id', 'model_type') }.from([user.id, 'User']) + end end end end context 'security' do context 'when relative path is involved' do - let(:temp_file_path) { File.join('uploads/-/system/temp', '..', 'another_subdir_of_temp') } + let(:temp_file_path) { File.join("uploads/-/system/user/#{user.id}", '..', 'another_subdir_of_temp') } it 'does not trigger move if path is outside designated directory' do - stub_file_mover('uploads/-/system/another_subdir_of_temp') + stub_file_mover("uploads/-/system/user/#{user.id}/another_subdir_of_temp") expect(FileUtils).not_to receive(:move) subject diff --git a/spec/validators/color_validator_spec.rb b/spec/validators/color_validator_spec.rb new file mode 100644 index 00000000000..e5a38ac9372 --- /dev/null +++ b/spec/validators/color_validator_spec.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe ColorValidator do + using RSpec::Parameterized::TableSyntax + + subject do + Class.new do + include ActiveModel::Model + include ActiveModel::Validations + attr_accessor :color + validates :color, color: true + end.new + end + + where(:color, :is_valid) do + '#000abc' | true + '#aaa' | true + '#BBB' | true + '#cCc' | true + '#ffff' | false + '#000111222' | false + 'invalid' | false + '000' | false + end + + with_them do + it 'only accepts valid colors' do + subject.color = color + + expect(subject.valid?).to eq(is_valid) + end + end + + it 'fails fast for long invalid string' do + subject.color = '#' + ('0' * 50_000) + 'xxx' + + expect do + Timeout.timeout(5.seconds) { subject.valid? } + end.not_to raise_error + end +end |