diff --git a/.gitlab/ci/releases.gitlab-ci.yml b/.gitlab/ci/releases.gitlab-ci.yml index d4e0236f3a8..8ca4041e6be 100644 --- a/.gitlab/ci/releases.gitlab-ci.yml +++ b/.gitlab/ci/releases.gitlab-ci.yml @@ -9,7 +9,7 @@ image: alpine:edge stage: sync before_script: - - apk add --no-cache --update curl bash + - apk add --no-cache --update curl bash jq after_script: [] script: - bash scripts/sync-stable-branch.sh diff --git a/CHANGELOG-EE.md b/CHANGELOG-EE.md index dc4e390ebc8..b52bdaef27d 100644 --- a/CHANGELOG-EE.md +++ b/CHANGELOG-EE.md @@ -1,5 +1,13 @@ Please view this file on the master branch, on stable branches it's out of date. +## 12.6.2 + +### Security (2 changes) + +- Don't publish drafts if user can't create notes. +- Remove protected tag access when group is removed. + + ## 12.6.1 - No changes. diff --git a/app/controllers/profiles/notifications_controller.rb b/app/controllers/profiles/notifications_controller.rb index 5f44e55f3ef..d295b64082c 100644 --- a/app/controllers/profiles/notifications_controller.rb +++ b/app/controllers/profiles/notifications_controller.rb @@ -11,6 +11,7 @@ class Profiles::NotificationsController < Profiles::ApplicationController exclude_group_ids: @group_notifications.select(:source_id) ).execute.map { |group| current_user.notification_settings_for(group, inherit: true) } @project_notifications = current_user.notification_settings.for_projects.order(:id) + .select { |notification| current_user.can?(:read_project, notification.source) } @global_notification_setting = current_user.global_notification_setting end # rubocop: enable CodeReuse/ActiveRecord diff --git a/app/controllers/projects/releases_controller.rb b/app/controllers/projects/releases_controller.rb index ffe69fe97e4..d6030a9e455 100644 --- a/app/controllers/projects/releases_controller.rb +++ b/app/controllers/projects/releases_controller.rb @@ -10,7 +10,7 @@ class Projects::ReleasesController < Projects::ApplicationController push_frontend_feature_flag(:release_evidence_collection, project) end before_action :authorize_update_release!, only: %i[edit update] - before_action :authorize_download_code!, only: [:evidence] + before_action :authorize_read_release_evidence!, only: [:evidence] def index respond_to do |format| @@ -47,6 +47,11 @@ class Projects::ReleasesController < Projects::ApplicationController access_denied! unless can?(current_user, :update_release, release) end + def authorize_read_release_evidence! + access_denied! unless Feature.enabled?(:release_evidence, project, default_enabled: true) + access_denied! unless can?(current_user, :read_release_evidence, release) + end + def release @release ||= project.releases.find_by_tag!(sanitized_tag_name) end diff --git a/app/helpers/notifications_helper.rb b/app/helpers/notifications_helper.rb index 8855e0cdd70..9a64fe98f86 100644 --- a/app/helpers/notifications_helper.rb +++ b/app/helpers/notifications_helper.rb @@ -116,4 +116,8 @@ module NotificationsHelper def show_unsubscribe_title?(noteable) can?(current_user, "read_#{noteable.to_ability_name}".to_sym, noteable) end + + def can_read_project?(project) + can?(current_user, :read_project, project) + end end diff --git a/app/models/evidence.rb b/app/models/evidence.rb index 69a00f1cb3f..55149ab0dfa 100644 --- a/app/models/evidence.rb +++ b/app/models/evidence.rb @@ -15,6 +15,21 @@ class Evidence < ApplicationRecord @milestones ||= release.milestones.includes(:issues) end + ## + # Return `summary` without sensitive information. + # + # Removing issues from summary in order to prevent leaking confidential ones. + # See more https://gitlab.com/gitlab-org/gitlab/issues/121930 + def summary + safe_summary = read_attribute(:summary) + + safe_summary.dig('release', 'milestones')&.each do |milestone| + milestone.delete('issues') + end + + safe_summary + end + private def generate_summary_and_sha diff --git a/app/models/user.rb b/app/models/user.rb index 18bf5ceaa0e..ee42a987939 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1327,7 +1327,7 @@ class User < ApplicationRecord .select('ci_runners.*') group_runners = Ci::RunnerNamespace - .where(namespace_id: owned_or_maintainers_groups.select(:id)) + .where(namespace_id: owned_groups.select(:id)) .joins(:runner) .select('ci_runners.*') diff --git a/app/policies/release_policy.rb b/app/policies/release_policy.rb index d7f9e5d7445..0fd1312c511 100644 --- a/app/policies/release_policy.rb +++ b/app/policies/release_policy.rb @@ -2,4 +2,31 @@ class ReleasePolicy < BasePolicy delegate { @subject.project } + + rule { allowed_to_read_evidence & external_authorization_service_disabled }.policy do + enable :read_release_evidence + end + + ## + # evidence.summary includes the following entities: + # - Release + # - git-tag (Repository) + # - Project + # - Milestones + # - Issues + condition(:allowed_to_read_evidence) do + can?(:read_release) && + can?(:download_code) && + can?(:read_project) && + can?(:read_milestone) && + can?(:read_issue) + end + + ## + # Currently, we don't support release evidence for the GitLab instances + # that enables external authorization services. + # See https://gitlab.com/gitlab-org/gitlab/issues/121930. + condition(:external_authorization_service_disabled) do + !Gitlab::ExternalAuthorization::Config.enabled? + end end diff --git a/app/uploaders/avatar_uploader.rb b/app/uploaders/avatar_uploader.rb index d42c9dbedf4..b79a5deb9c0 100644 --- a/app/uploaders/avatar_uploader.rb +++ b/app/uploaders/avatar_uploader.rb @@ -5,6 +5,9 @@ class AvatarUploader < GitlabUploader include RecordsUploads::Concern include ObjectStorage::Concern prepend ObjectStorage::Extension::RecordsUploads + include UploadTypeCheck::Concern + + check_upload_type extensions: AvatarUploader::SAFE_IMAGE_EXT def exists? model.avatar.file && model.avatar.file.present? diff --git a/app/uploaders/favicon_uploader.rb b/app/uploaders/favicon_uploader.rb index a0b275b56a9..f393fdf0d84 100644 --- a/app/uploaders/favicon_uploader.rb +++ b/app/uploaders/favicon_uploader.rb @@ -1,8 +1,12 @@ # frozen_string_literal: true class FaviconUploader < AttachmentUploader + include UploadTypeCheck::Concern + EXTENSION_WHITELIST = %w[png ico].freeze + check_upload_type extensions: EXTENSION_WHITELIST + def extension_whitelist EXTENSION_WHITELIST end diff --git a/app/uploaders/upload_type_check.rb b/app/uploaders/upload_type_check.rb new file mode 100644 index 00000000000..2837b001660 --- /dev/null +++ b/app/uploaders/upload_type_check.rb @@ -0,0 +1,98 @@ +# frozen_string_literal: true + +# Ensure that uploaded files are what they say they are for security and +# handling purposes. The checks are not 100% reliable so we err on the side of +# caution and allow by default, and deny when we're confident of a fail state. +# +# Include this concern, then call `check_upload_type` to check all +# uploads. Attach a `mime_type` or `extensions` parameter to only check +# specific upload types. Both parameters will be normalized to a MIME type and +# checked against the inferred MIME type of the upload content and filename +# extension. +# +# class YourUploader +# include UploadTypeCheck::Concern +# check_upload_type mime_types: ['image/png', /image\/jpe?g/] +# +# # or... +# +# check_upload_type extensions: ['png', 'jpg', 'jpeg'] +# end +# +# The mime_types parameter can accept `NilClass`, `String`, `Regexp`, +# `Array[String, Regexp]`. This matches the CarrierWave `extension_whitelist` +# and `content_type_whitelist` family of behavior. +# +# The extensions parameter can accept `NilClass`, `String`, `Array[String]`. +module UploadTypeCheck + module Concern + extend ActiveSupport::Concern + + class_methods do + def check_upload_type(mime_types: nil, extensions: nil) + define_method :check_upload_type_callback do |file| + magic_file = MagicFile.new(file.to_file) + + # Map file extensions back to mime types. + if extensions + mime_types = Array(mime_types) + + Array(extensions).map { |e| MimeMagic::EXTENSIONS[e] } + end + + if mime_types.nil? || magic_file.matches_mime_types?(mime_types) + check_content_matches_extension!(magic_file) + end + end + before :cache, :check_upload_type_callback + end + end + + def check_content_matches_extension!(magic_file) + return if magic_file.ambiguous_type? + + if magic_file.magic_type != magic_file.ext_type + raise CarrierWave::IntegrityError, 'Content type does not match file extension' + end + end + end + + # Convenience class to wrap MagicMime objects. + class MagicFile + attr_reader :file + + def initialize(file) + @file = file + end + + def magic_type + @magic_type ||= MimeMagic.by_magic(file) + end + + def ext_type + @ext_type ||= MimeMagic.by_path(file.path) + end + + def magic_type_type + magic_type&.type + end + + def ext_type_type + ext_type&.type + end + + def matches_mime_types?(mime_types) + Array(mime_types).any? do |mt| + magic_type_type =~ /\A#{mt}\z/ || ext_type_type =~ /\A#{mt}\z/ + end + end + + # - Both types unknown or text/plain. + # - Ambiguous magic type with text extension. Plain text file. + # - Text magic type with ambiguous extension. TeX file missing extension. + def ambiguous_type? + (ext_type.to_s.blank? && magic_type.to_s.blank?) || + (magic_type.to_s.blank? && ext_type_type == 'text/plain') || + (ext_type.to_s.blank? && magic_type_type == 'text/plain') + end + end +end diff --git a/app/views/sent_notifications/unsubscribe.html.haml b/app/views/sent_notifications/unsubscribe.html.haml index 22fcfcda297..1eecbe3bc0e 100644 --- a/app/views/sent_notifications/unsubscribe.html.haml +++ b/app/views/sent_notifications/unsubscribe.html.haml @@ -1,13 +1,16 @@ - noteable = @sent_notification.noteable - noteable_type = @sent_notification.noteable_type.titleize.downcase - noteable_text = show_unsubscribe_title?(noteable) ? %(#{noteable.title} (#{noteable.to_reference})) : %(#{noteable.to_reference}) -- page_title _("Unsubscribe"), noteable_text, noteable_type.pluralize, @sent_notification.project.full_name +- show_project_path = can_read_project?(@sent_notification.project) +- project_path = show_project_path ? @sent_notification.project.full_name : _("GitLab / Unsubscribe") +- noteable_url = show_project_path ? url_for([@sent_notification.project.namespace.becomes(Namespace), @sent_notification.project, noteable]) : breadcrumb_title_link +- page_title _('Unsubscribe'), noteable_text, noteable_type.pluralize, project_path %h3.page-title = _("Unsubscribe from %{type}") % { type: noteable_type } %p - - link_to_noteable_text = link_to(noteable_text, url_for([@sent_notification.project.namespace.becomes(Namespace), @sent_notification.project, noteable])) + - link_to_noteable_text = link_to(noteable_text, noteable_url) = _("Are you sure you want to unsubscribe from the %{type}: %{link_to_noteable_text}?").html_safe % { type: noteable_type, link_to_noteable_text: link_to_noteable_text } %p diff --git a/changelogs/unreleased/ap-33785-file-integrity.yml b/changelogs/unreleased/ap-33785-file-integrity.yml new file mode 100644 index 00000000000..36ff617a252 --- /dev/null +++ b/changelogs/unreleased/ap-33785-file-integrity.yml @@ -0,0 +1,5 @@ +--- +title: Ensure content matches extension on image uploads +merge_request: 20697 +author: +type: security diff --git a/config/initializers/graphql.rb b/config/initializers/graphql.rb index f1bc289f1f0..2b21c9d9729 100644 --- a/config/initializers/graphql.rb +++ b/config/initializers/graphql.rb @@ -5,3 +5,7 @@ GraphQL::Field.accepts_definitions(authorize: GraphQL::Define.assign_metadata_ke GraphQL::Schema::Object.accepts_definition(:authorize) GraphQL::Schema::Field.accepts_definition(:authorize) + +GitlabSchema.middleware << GraphQL::Schema::TimeoutMiddleware.new(max_seconds: ENV.fetch('GITLAB_RAILS_GRAPHQL_TIMEOUT', 30).to_i) do |timeout_error, query| + Gitlab::GraphqlLogger.error(message: timeout_error.to_s, query: query.query_string, query_variables: query.provided_variables) +end diff --git a/lib/api/entities.rb b/lib/api/entities.rb index a86fb44caa1..0240fc1539f 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -1363,7 +1363,7 @@ module API expose :author, using: Entities::UserBasic, if: -> (release, _) { release.author.present? } expose :commit, using: Entities::Commit, if: ->(_, _) { can_download_code? } expose :upcoming_release?, as: :upcoming_release - expose :milestones, using: Entities::Milestone, if: -> (release, _) { release.milestones.present? } + expose :milestones, using: Entities::Milestone, if: -> (release, _) { release.milestones.present? && can_read_milestone? } expose :commit_path, expose_nil: false expose :tag_path, expose_nil: false expose :evidence_sha, expose_nil: false, if: ->(_, _) { can_download_code? } @@ -1389,6 +1389,10 @@ module API def can_download_code? Ability.allowed?(options[:current_user], :download_code, object.project) end + + def can_read_milestone? + Ability.allowed?(options[:current_user], :read_milestone, object.project) + end end class Tag < Grape::Entity diff --git a/lib/banzai/filter/relative_link_filter.rb b/lib/banzai/filter/relative_link_filter.rb index 583b0081319..4f257189f8e 100644 --- a/lib/banzai/filter/relative_link_filter.rb +++ b/lib/banzai/filter/relative_link_filter.rb @@ -116,7 +116,7 @@ module Banzai end def process_link_to_upload_attr(html_attr) - path_parts = [Addressable::URI.unescape(html_attr.value)] + path_parts = [unescape_and_scrub_uri(html_attr.value)] if project path_parts.unshift(relative_url_root, project.full_path) @@ -172,7 +172,7 @@ module Banzai end def cleaned_file_path(uri) - Addressable::URI.unescape(uri.path).scrub.delete("\0").chomp("/") + unescape_and_scrub_uri(uri.path).delete("\0").chomp("/") end def relative_file_path(uri) @@ -184,7 +184,7 @@ module Banzai def request_path return unless context[:requested_path] - Addressable::URI.unescape(context[:requested_path]).chomp("/") + unescape_and_scrub_uri(context[:requested_path]).chomp("/") end # Convert a relative path into its correct location based on the currently @@ -266,6 +266,12 @@ module Banzai def repository @repository ||= project&.repository end + + private + + def unescape_and_scrub_uri(uri) + Addressable::URI.unescape(uri).scrub + end end end end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index fa5560755a9..b573205a85f 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -8579,6 +8579,9 @@ msgstr "" msgid "GitHub import" msgstr "" +msgid "GitLab / Unsubscribe" +msgstr "" + msgid "GitLab CI Linter has been moved" msgstr "" diff --git a/scripts/sync-stable-branch.sh b/scripts/sync-stable-branch.sh index b44bf26a151..1eb416bf4f5 100644 --- a/scripts/sync-stable-branch.sh +++ b/scripts/sync-stable-branch.sh @@ -35,6 +35,22 @@ then exit 1 fi +if [[ "$TARGET_PROJECT" != "gitlab-org/gitlab-foss" ]] +then + echo 'This is a security FOSS merge train' + echo "Checking if $CI_COMMIT_SHA is available on canonical" + + gitlab_com_commit_status=$(curl -s "https://gitlab.com/api/v4/projects/278964/repository/commits/$CI_COMMIT_SHA" | jq -M .status) + + if [[ "$gitlab_com_commit_status" != "null" ]] + then + echo 'Commit available on canonical, skipping merge train' + exit 0 + fi + + echo 'Commit not available, triggering a merge train' +fi + curl -X POST \ -F token="$MERGE_TRAIN_TRIGGER_TOKEN" \ -F ref=master \ diff --git a/spec/controllers/profiles/notifications_controller_spec.rb b/spec/controllers/profiles/notifications_controller_spec.rb index dbc408bcdd9..ede68744ac6 100644 --- a/spec/controllers/profiles/notifications_controller_spec.rb +++ b/spec/controllers/profiles/notifications_controller_spec.rb @@ -52,6 +52,35 @@ describe Profiles::NotificationsController do end.to exceed_query_limit(control) end end + + context 'with project notifications' do + let!(:notification_setting) { create(:notification_setting, source: project, user: user, level: :watch) } + + before do + sign_in(user) + get :show + end + + context 'when project is public' do + let(:project) { create(:project, :public) } + + it 'shows notification setting for project' do + expect(assigns(:project_notifications).map(&:source_id)).to include(project.id) + end + end + + context 'when project is public' do + let(:project) { create(:project, :private) } + + it 'shows notification setting for project' do + # notification settings for given project were created before project was set to private + expect(user.notification_settings.for_projects.map(&:source_id)).to include(project.id) + + # check that notification settings for project where user does not have access are filtered + expect(assigns(:project_notifications)).to be_empty + end + end + end end describe 'POST update' do diff --git a/spec/controllers/projects/releases_controller_spec.rb b/spec/controllers/projects/releases_controller_spec.rb index e9fa3764117..750e9aabef0 100644 --- a/spec/controllers/projects/releases_controller_spec.rb +++ b/spec/controllers/projects/releases_controller_spec.rb @@ -167,7 +167,7 @@ describe Projects::ReleasesController do end describe 'GET #evidence' do - let(:tag_name) { "v1.1.0-evidence" } + let_it_be(:tag_name) { "v1.1.0-evidence" } let!(:release) { create(:release, :with_evidence, project: project, tag: tag_name) } let(:tag) { CGI.escape(release.tag) } let(:format) { :json } @@ -220,6 +220,85 @@ describe Projects::ReleasesController do it_behaves_like 'successful request' end end + + context 'when release is associated to a milestone which includes an issue' do + let_it_be(:project) { create(:project, :repository, :public) } + let_it_be(:issue) { create(:issue, project: project) } + let_it_be(:milestone) { create(:milestone, project: project, issues: [issue]) } + let_it_be(:release) { create(:release, project: project, tag: tag_name, milestones: [milestone]) } + + before do + create(:evidence, release: release) + end + + shared_examples_for 'does not show the issue in evidence' do + it do + subject + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response['release']['milestones'] + .all? { |milestone| milestone['issues'].nil? }).to eq(true) + end + end + + shared_examples_for 'evidence not found' do + it do + subject + + expect(response).to have_gitlab_http_status(:not_found) + end + end + + shared_examples_for 'safely expose evidence' do + it_behaves_like 'does not show the issue in evidence' + + context 'when the issue is confidential' do + let(:issue) { create(:issue, :confidential, project: project) } + + it_behaves_like 'does not show the issue in evidence' + end + + context 'when the user is the author of the confidential issue' do + let(:issue) { create(:issue, :confidential, project: project, author: user) } + + it_behaves_like 'does not show the issue in evidence' + end + + context 'when project is private' do + let!(:project) { create(:project, :repository, :private) } + + it_behaves_like 'evidence not found' + end + + context 'when project restricts the visibility of issues to project members only' do + let!(:project) { create(:project, :repository, :issues_private) } + + it_behaves_like 'evidence not found' + end + end + + context 'when user is non-project member' do + let(:user) { create(:user) } + + it_behaves_like 'safely expose evidence' + end + + context 'when user is auditor', if: Gitlab.ee? do + let(:user) { create(:user, :auditor) } + + it_behaves_like 'safely expose evidence' + end + + context 'when external authorization control is enabled' do + let(:user) { create(:user) } + + before do + stub_application_setting(external_authorization_service_enabled: true) + end + + it_behaves_like 'evidence not found' + end + end end private diff --git a/spec/controllers/sent_notifications_controller_spec.rb b/spec/controllers/sent_notifications_controller_spec.rb index 0e634d8ba99..4dd4f49dcf1 100644 --- a/spec/controllers/sent_notifications_controller_spec.rb +++ b/spec/controllers/sent_notifications_controller_spec.rb @@ -56,7 +56,7 @@ describe SentNotificationsController do get(:unsubscribe, params: { id: sent_notification.reply_key }) end - shared_examples 'unsubscribing as anonymous' do + shared_examples 'unsubscribing as anonymous' do |project_visibility| it 'does not unsubscribe the user' do expect(noteable.subscribed?(user, target_project)).to be_truthy end @@ -69,6 +69,18 @@ describe SentNotificationsController do expect(response.status).to eq(200) expect(response).to render_template :unsubscribe end + + if project_visibility == :private + it 'does not show project name or path' do + expect(response.body).not_to include(noteable.project.name) + expect(response.body).not_to include(noteable.project.full_name) + end + else + it 'shows project name or path' do + expect(response.body).to include(noteable.project.name) + expect(response.body).to include(noteable.project.full_name) + end + end end context 'when project is public' do @@ -79,7 +91,7 @@ describe SentNotificationsController do expect(response.body).to include(issue.title) end - it_behaves_like 'unsubscribing as anonymous' + it_behaves_like 'unsubscribing as anonymous', :public end context 'when unsubscribing from confidential issue' do @@ -90,7 +102,7 @@ describe SentNotificationsController do expect(response.body).to include(confidential_issue.to_reference) end - it_behaves_like 'unsubscribing as anonymous' + it_behaves_like 'unsubscribing as anonymous', :public end context 'when unsubscribing from merge request' do @@ -100,7 +112,12 @@ describe SentNotificationsController do expect(response.body).to include(merge_request.title) end - it_behaves_like 'unsubscribing as anonymous' + it 'shows project name or path' do + expect(response.body).to include(issue.project.name) + expect(response.body).to include(issue.project.full_name) + end + + it_behaves_like 'unsubscribing as anonymous', :public end end @@ -110,11 +127,11 @@ describe SentNotificationsController do context 'when unsubscribing from issue' do let(:noteable) { issue } - it 'shows issue title' do + it 'does not show issue title' do expect(response.body).not_to include(issue.title) end - it_behaves_like 'unsubscribing as anonymous' + it_behaves_like 'unsubscribing as anonymous', :private end context 'when unsubscribing from confidential issue' do @@ -125,17 +142,17 @@ describe SentNotificationsController do expect(response.body).to include(confidential_issue.to_reference) end - it_behaves_like 'unsubscribing as anonymous' + it_behaves_like 'unsubscribing as anonymous', :private end context 'when unsubscribing from merge request' do let(:noteable) { merge_request } - it 'shows merge request title' do + it 'dos not show merge request title' do expect(response.body).not_to include(merge_request.title) end - it_behaves_like 'unsubscribing as anonymous' + it_behaves_like 'unsubscribing as anonymous', :private end end end diff --git a/spec/fixtures/api/schemas/evidences/milestone.json b/spec/fixtures/api/schemas/evidences/milestone.json index ab27fdecde2..3ce0644225b 100644 --- a/spec/fixtures/api/schemas/evidences/milestone.json +++ b/spec/fixtures/api/schemas/evidences/milestone.json @@ -7,8 +7,7 @@ "state", "iid", "created_at", - "due_date", - "issues" + "due_date" ], "properties": { "id": { "type": "integer" }, @@ -17,11 +16,7 @@ "state": { "type": "string" }, "iid": { "type": "integer" }, "created_at": { "type": "date" }, - "due_date": { "type": ["date", "null"] }, - "issues": { - "type": "array", - "items": { "$ref": "issue.json" } - } + "due_date": { "type": ["date", "null"] } }, "additionalProperties": false } diff --git a/spec/fixtures/not_a_png.png b/spec/fixtures/not_a_png.png new file mode 100644 index 00000000000..932f9efaed9 Binary files /dev/null and b/spec/fixtures/not_a_png.png differ diff --git a/spec/lib/banzai/filter/relative_link_filter_spec.rb b/spec/lib/banzai/filter/relative_link_filter_spec.rb index a17a645d4d0..9f467d7a6fd 100644 --- a/spec/lib/banzai/filter/relative_link_filter_spec.rb +++ b/spec/lib/banzai/filter/relative_link_filter_spec.rb @@ -128,6 +128,15 @@ describe Banzai::Filter::RelativeLinkFilter do expect { filter(act) }.not_to raise_error end + it 'does not raise an exception on URIs containing invalid utf-8 byte sequences in uploads' do + act = link("/uploads/%FF") + expect { filter(act) }.not_to raise_error + end + + it 'does not raise an exception on URIs containing invalid utf-8 byte sequences in context requested path' do + expect { filter(link("files/test.md"), requested_path: '%FF') }.not_to raise_error + end + it 'does not raise an exception with a garbled path' do act = link("open(/var/tmp/):%20/location%0Afrom:%20/test") expect { filter(act) }.not_to raise_error diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 314cc4e1d3c..b0f708bc0e7 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -2638,8 +2638,8 @@ describe User, :do_not_mock_admin_mode do add_user(:maintainer) end - it 'loads' do - expect(user.ci_owned_runners).to contain_exactly(runner) + it 'does not load' do + expect(user.ci_owned_runners).to be_empty end end @@ -2654,6 +2654,20 @@ describe User, :do_not_mock_admin_mode do end end + shared_examples :group_member do + context 'when the user is owner' do + before do + add_user(:owner) + end + + it 'loads' do + expect(user.ci_owned_runners).to contain_exactly(runner) + end + end + + it_behaves_like :member + end + context 'with groups projects runners' do let(:group) { create(:group) } let!(:project) { create(:project, group: group) } @@ -2662,7 +2676,7 @@ describe User, :do_not_mock_admin_mode do group.add_user(user, access) end - it_behaves_like :member + it_behaves_like :group_member end context 'with groups runners' do @@ -2673,14 +2687,14 @@ describe User, :do_not_mock_admin_mode do group.add_user(user, access) end - it_behaves_like :member + it_behaves_like :group_member end context 'with other projects runners' do let!(:project) { create(:project) } def add_user(access) - project.add_role(user, access) + project.add_user(user, access) end it_behaves_like :member @@ -2698,7 +2712,7 @@ describe User, :do_not_mock_admin_mode do subgroup.add_user(another_user, :owner) end - it_behaves_like :member + it_behaves_like :group_member end end diff --git a/spec/requests/api/graphql/gitlab_schema_spec.rb b/spec/requests/api/graphql/gitlab_schema_spec.rb index 2aeb75a10b4..2cb8436662b 100644 --- a/spec/requests/api/graphql/gitlab_schema_spec.rb +++ b/spec/requests/api/graphql/gitlab_schema_spec.rb @@ -8,6 +8,18 @@ describe 'GitlabSchema configurations' do set(:project) { create(:project) } shared_examples 'imposing query limits' do + describe 'timeouts' do + context 'when timeout is reached' do + it 'shows an error' do + Timecop.scale(50000000) do # ludicrously large number because the timeout has to happen before the query even begins + subject + + expect_graphql_errors_to_include /Timeout/ + end + end + end + end + describe '#max_complexity' do context 'when complexity is too high' do it 'shows an error' do diff --git a/spec/requests/api/releases_spec.rb b/spec/requests/api/releases_spec.rb index da04e852795..233f0497b7f 100644 --- a/spec/requests/api/releases_spec.rb +++ b/spec/requests/api/releases_spec.rb @@ -340,6 +340,40 @@ describe API::Releases do expect(response).to have_gitlab_http_status(:ok) end + + context 'when release is associated to a milestone' do + let!(:release) do + create(:release, tag: 'v0.1', project: project, milestones: [milestone]) + end + + let(:milestone) { create(:milestone, project: project) } + + it 'exposes milestones' do + get api("/projects/#{project.id}/releases/v0.1", non_project_member) + + expect(json_response['milestones'].first['title']).to eq(milestone.title) + end + + context 'when project restricts visibility of issues and merge requests' do + let!(:project) { create(:project, :repository, :public, :issues_private, :merge_requests_private) } + + it 'does not expose milestones' do + get api("/projects/#{project.id}/releases/v0.1", non_project_member) + + expect(json_response['milestones']).to be_nil + end + end + + context 'when project restricts visibility of issues' do + let!(:project) { create(:project, :repository, :public, :issues_private) } + + it 'exposes milestones' do + get api("/projects/#{project.id}/releases/v0.1", non_project_member) + + expect(json_response['milestones'].first['title']).to eq(milestone.title) + end + end + end end end end diff --git a/spec/requests/api/runners_spec.rb b/spec/requests/api/runners_spec.rb index 8daba204d50..7bad30d107d 100644 --- a/spec/requests/api/runners_spec.rb +++ b/spec/requests/api/runners_spec.rb @@ -6,6 +6,7 @@ describe API::Runners do let(:admin) { create(:user, :admin) } let(:user) { create(:user) } let(:user2) { create(:user) } + let(:group_maintainer) { create(:user) } let(:project) { create(:project, creator_id: user.id) } let(:project2) { create(:project, creator_id: user.id) } @@ -20,6 +21,7 @@ describe API::Runners do before do # Set project access for users + create(:group_member, :maintainer, user: group_maintainer, group: group) create(:project_member, :maintainer, user: user, project: project) create(:project_member, :maintainer, user: user, project: project2) create(:project_member, :reporter, user: user2, project: project) @@ -525,6 +527,20 @@ describe API::Runners do end.to change { Ci::Runner.project_type.count }.by(-1) end + it 'does not delete group runner with maintainer access' do + delete api("/runners/#{group_runner.id}", group_maintainer) + + expect(response).to have_http_status(403) + end + + it 'deletes group runner with owner access' do + expect do + delete api("/runners/#{group_runner.id}", user) + + expect(response).to have_http_status(204) + end.to change { Ci::Runner.group_type.count }.by(-1) + end + it_behaves_like '412 response' do let(:request) { api("/runners/#{project_runner.id}", user) } end diff --git a/spec/support/shared_contexts/upload_type_check_shared_context.rb b/spec/support/shared_contexts/upload_type_check_shared_context.rb new file mode 100644 index 00000000000..04c97500dd6 --- /dev/null +++ b/spec/support/shared_contexts/upload_type_check_shared_context.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +# Construct an `uploader` variable that is configured to `check_upload_type` +# with `mime_types` and `extensions`. +shared_context 'uploader with type check' do + let(:uploader_class) do + Class.new(GitlabUploader) do + include UploadTypeCheck::Concern + storage :file + end + end + + let(:mime_types) { nil } + let(:extensions) { nil } + let(:uploader) do + uploader_class.class_exec(mime_types, extensions) do |mime_types, extensions| + check_upload_type mime_types: mime_types, extensions: extensions + end + uploader_class.new(build_stubbed(:user)) + end +end + +shared_context 'stubbed MimeMagic mime type detection' do + let(:mime_type) { '' } + let(:magic_mime) { mime_type } + let(:ext_mime) { mime_type } + before do + magic_mime_obj = MimeMagic.new(magic_mime) + ext_mime_obj = MimeMagic.new(ext_mime) + allow(MimeMagic).to receive(:by_magic).with(anything).and_return(magic_mime_obj) + allow(MimeMagic).to receive(:by_path).with(anything).and_return(ext_mime_obj) + end +end diff --git a/spec/support/shared_examples/uploaders/upload_type_shared_examples.rb b/spec/support/shared_examples/uploaders/upload_type_shared_examples.rb new file mode 100644 index 00000000000..91d2526cde2 --- /dev/null +++ b/spec/support/shared_examples/uploaders/upload_type_shared_examples.rb @@ -0,0 +1,65 @@ +# frozen_string_literal: true + +def check_content_matches_extension!(file = double(read: nil, path: '')) + magic_file = UploadTypeCheck::MagicFile.new(file) + uploader.check_content_matches_extension!(magic_file) +end + +shared_examples 'upload passes content type check' do + it 'does not raise error' do + expect { check_content_matches_extension! }.not_to raise_error + end +end + +shared_examples 'upload fails content type check' do + it 'raises error' do + expect { check_content_matches_extension! }.to raise_error(CarrierWave::IntegrityError) + end +end + +def upload_type_checked_filenames(filenames) + Array(filenames).each do |filename| + # Feed the uploader "some" content. + path = File.join('spec', 'fixtures', 'dk.png') + file = File.new(path, 'r') + # Rename the file with what we want. + allow(file).to receive(:path).and_return(filename) + + # Force the content type to match the extension type. + mime_type = MimeMagic.by_path(filename) + allow(MimeMagic).to receive(:by_magic).and_return(mime_type) + + uploaded_file = Rack::Test::UploadedFile.new(file, original_filename: filename) + uploader.cache!(uploaded_file) + end +end + +def upload_type_checked_fixtures(upload_fixtures) + upload_fixtures = Array(upload_fixtures) + upload_fixtures.each do |upload_fixture| + path = File.join('spec', 'fixtures', upload_fixture) + uploader.cache!(fixture_file_upload(path)) + end +end + +shared_examples 'type checked uploads' do |upload_fixtures = nil, filenames: nil| + it 'check type' do + upload_fixtures = Array(upload_fixtures) + filenames = Array(filenames) + + times = upload_fixtures.length + filenames.length + expect(uploader).to receive(:check_content_matches_extension!).exactly(times).times + + upload_type_checked_fixtures(upload_fixtures) unless upload_fixtures.empty? + upload_type_checked_filenames(filenames) unless filenames.empty? + end +end + +shared_examples 'skipped type checked uploads' do |upload_fixtures = nil, filenames: nil| + it 'skip type check' do + expect(uploader).not_to receive(:check_content_matches_extension!) + + upload_type_checked_fixtures(upload_fixtures) if upload_fixtures + upload_type_checked_filenames(filenames) if filenames + end +end diff --git a/spec/uploaders/avatar_uploader_spec.rb b/spec/uploaders/avatar_uploader_spec.rb index c0844360589..669f75b2ee8 100644 --- a/spec/uploaders/avatar_uploader_spec.rb +++ b/spec/uploaders/avatar_uploader_spec.rb @@ -46,4 +46,16 @@ describe AvatarUploader do expect(uploader.absolute_path).to eq(absolute_path) end end + + context 'upload type check' do + AvatarUploader::SAFE_IMAGE_EXT.each do |ext| + context "#{ext} extension" do + it_behaves_like 'type checked uploads', filenames: "image.#{ext}" + end + end + + context 'skip image/svg+xml integrity check' do + it_behaves_like 'skipped type checked uploads', filenames: 'image.svg' + end + end end diff --git a/spec/uploaders/favicon_uploader_spec.rb b/spec/uploaders/favicon_uploader_spec.rb new file mode 100644 index 00000000000..4d6c849883a --- /dev/null +++ b/spec/uploaders/favicon_uploader_spec.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe FaviconUploader do + let_it_be(:model) { build_stubbed(:user) } + let_it_be(:uploader) { described_class.new(model, :favicon) } + + context 'upload type check' do + FaviconUploader::EXTENSION_WHITELIST.each do |ext| + context "#{ext} extension" do + it_behaves_like 'type checked uploads', filenames: "image.#{ext}" + end + end + end + + context 'upload non-whitelisted file extensions' do + it 'will deny upload' do + path = File.join('spec', 'fixtures', 'banana_sample.gif') + fixture_file = fixture_file_upload(path) + expect { uploader.cache!(fixture_file) }.to raise_exception(CarrierWave::IntegrityError) + end + end +end diff --git a/spec/uploaders/upload_type_check_spec.rb b/spec/uploaders/upload_type_check_spec.rb new file mode 100644 index 00000000000..a4895f6a956 --- /dev/null +++ b/spec/uploaders/upload_type_check_spec.rb @@ -0,0 +1,124 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe UploadTypeCheck do + include_context 'uploader with type check' + + def upload_fixture(filename) + fixture_file_upload(File.join('spec', 'fixtures', filename)) + end + + describe '#check_content_matches_extension! callback using file upload' do + context 'when extension matches contents' do + it 'not raise error on upload' do + expect { uploader.cache!(upload_fixture('banana_sample.gif')) }.not_to raise_error + end + end + + context 'when extension does not match contents' do + it 'raise error' do + expect { uploader.cache!(upload_fixture('not_a_png.png')) }.to raise_error(CarrierWave::IntegrityError) + end + end + end + + describe '#check_content_matches_extension! callback using stubs' do + include_context 'stubbed MimeMagic mime type detection' + + context 'when no extension and with ambiguous/text content' do + let(:magic_mime) { '' } + let(:ext_mime) { '' } + + it_behaves_like 'upload passes content type check' + end + + context 'when no extension and with non-text content' do + let(:magic_mime) { 'image/gif' } + let(:ext_mime) { '' } + + it_behaves_like 'upload fails content type check' + end + + # Most text files will exhibit this behaviour. + context 'when ambiguous content with text extension' do + let(:magic_mime) { '' } + let(:ext_mime) { 'text/plain' } + + it_behaves_like 'upload passes content type check' + end + + context 'when text content with text extension' do + let(:magic_mime) { 'text/plain' } + let(:ext_mime) { 'text/plain' } + + it_behaves_like 'upload passes content type check' + end + + context 'when ambiguous content with non-text extension' do + let(:magic_mime) { '' } + let(:ext_mime) { 'application/zip' } + + it_behaves_like 'upload fails content type check' + end + + # These are the types when uploading a .dmg + context 'when content and extension do not match' do + let(:magic_mime) { 'application/x-bzip' } + let(:ext_mime) { 'application/x-apple-diskimage' } + + it_behaves_like 'upload fails content type check' + end + end + + describe '#check_content_matches_extension! mime_type filtering' do + context 'without mime types' do + let(:mime_types) { nil } + + it_behaves_like 'type checked uploads', %w[doc_sample.txt rails_sample.jpg] + end + + context 'with mime types string' do + let(:mime_types) { 'text/plain' } + + it_behaves_like 'type checked uploads', %w[doc_sample.txt] + it_behaves_like 'skipped type checked uploads', %w[dk.png] + end + + context 'with mime types regex' do + let(:mime_types) { [/image\/(gif|png)/] } + + it_behaves_like 'type checked uploads', %w[banana_sample.gif dk.png] + it_behaves_like 'skipped type checked uploads', %w[doc_sample.txt] + end + + context 'with mime types array' do + let(:mime_types) { ['text/plain', /image\/png/] } + + it_behaves_like 'type checked uploads', %w[doc_sample.txt dk.png] + it_behaves_like 'skipped type checked uploads', %w[audio_sample.wav] + end + end + + describe '#check_content_matches_extension! extensions filtering' do + context 'without extensions' do + let(:extensions) { nil } + + it_behaves_like 'type checked uploads', %w[doc_sample.txt dk.png] + end + + context 'with extensions string' do + let(:extensions) { 'txt' } + + it_behaves_like 'type checked uploads', %w[doc_sample.txt] + it_behaves_like 'skipped type checked uploads', %w[rails_sample.jpg] + end + + context 'with extensions array of strings' do + let(:extensions) { %w[txt png] } + + it_behaves_like 'type checked uploads', %w[doc_sample.txt dk.png] + it_behaves_like 'skipped type checked uploads', %w[audio_sample.wav] + end + end +end