diff options
46 files changed, 440 insertions, 53 deletions
diff --git a/app/assets/javascripts/gfm_auto_complete.js b/app/assets/javascripts/gfm_auto_complete.js index 337a70a270a..51f05be7c84 100644 --- a/app/assets/javascripts/gfm_auto_complete.js +++ b/app/assets/javascripts/gfm_auto_complete.js @@ -242,7 +242,7 @@ class GfmAutoComplete { displayTpl(value) { let tmpl = GfmAutoComplete.Loading.template; if (value.title != null) { - tmpl = GfmAutoComplete.Milestones.template; + tmpl = GfmAutoComplete.Milestones.templateFunction(value.title); } return tmpl; }, @@ -309,7 +309,7 @@ class GfmAutoComplete { searchKey: 'search', data: GfmAutoComplete.defaultLoadingData, displayTpl(value) { - let tmpl = GfmAutoComplete.Labels.template; + let tmpl = GfmAutoComplete.Labels.templateFunction(value.color, value.title); if (GfmAutoComplete.isLoading(value)) { tmpl = GfmAutoComplete.Loading.template; } @@ -559,8 +559,11 @@ GfmAutoComplete.Members = { }, }; GfmAutoComplete.Labels = { - // eslint-disable-next-line no-template-curly-in-string - template: '<li><span class="dropdown-label-box" style="background: ${color}"></span> ${title}</li>', + templateFunction(color, title) { + return `<li><span class="dropdown-label-box" style="background: ${_.escape( + color, + )}"></span> ${_.escape(title)}</li>`; + }, }; // Issues, MergeRequests and Snippets GfmAutoComplete.Issues = { @@ -570,8 +573,9 @@ GfmAutoComplete.Issues = { }; // Milestones GfmAutoComplete.Milestones = { - // eslint-disable-next-line no-template-curly-in-string - template: '<li>${title}</li>', + templateFunction(title) { + return `<li>${_.escape(title)}</li>`; + }, }; GfmAutoComplete.Loading = { template: '<li style="pointer-events: none;"><i class="fa fa-spinner fa-spin"></i> Loading...</li>', diff --git a/app/models/project.rb b/app/models/project.rb index cb63520f593..ae1a29f4d8a 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -305,10 +305,10 @@ class Project < ActiveRecord::Base validates :namespace, presence: true validates :name, uniqueness: { scope: :namespace_id } - validates :import_url, url: { protocols: %w(http https ssh git), - allow_localhost: false, - enforce_user: true, - ports: VALID_IMPORT_PORTS }, if: [:external_import?, :import_url_changed?] + validates :import_url, public_url: { protocols: %w(http https ssh git), + allow_localhost: false, + enforce_user: true, + ports: VALID_IMPORT_PORTS }, if: [:external_import?, :import_url_changed?] validates :star_count, numericality: { greater_than_or_equal_to: 0 } validate :check_limit, on: :create validate :check_repository_path_availability, on: :update, if: ->(project) { project.renamed? } diff --git a/app/models/remote_mirror.rb b/app/models/remote_mirror.rb index c1f53b5da4f..b2d04d198cd 100644 --- a/app/models/remote_mirror.rb +++ b/app/models/remote_mirror.rb @@ -18,7 +18,7 @@ class RemoteMirror < ActiveRecord::Base belongs_to :project, inverse_of: :remote_mirrors - validates :url, presence: true, url: { protocols: %w(ssh git http https), allow_blank: true, enforce_user: true } + validates :url, presence: true, public_url: { protocols: %w(ssh git http https), allow_blank: true, enforce_user: true } before_save :set_new_remote_name, if: :mirror_url_changed? diff --git a/app/models/todo.rb b/app/models/todo.rb index 265fb932f7c..acd87a6d173 100644 --- a/app/models/todo.rb +++ b/app/models/todo.rb @@ -4,6 +4,11 @@ class Todo < ActiveRecord::Base include Sortable include FromUnion + # Time to wait for todos being removed when not visible for user anymore. + # Prevents TODOs being removed by mistake, for example, removing access from a user + # and giving it back again. + WAIT_FOR_DELETE = 1.hour + ASSIGNED = 1 MENTIONED = 2 BUILD_FAILED = 3 diff --git a/app/services/groups/update_service.rb b/app/services/groups/update_service.rb index fe47aa2f140..dd7b30639b0 100644 --- a/app/services/groups/update_service.rb +++ b/app/services/groups/update_service.rb @@ -29,7 +29,7 @@ module Groups def after_update if group.previous_changes.include?(:visibility_level) && group.private? # don't enqueue immediately to prevent todos removal in case of a mistake - TodosDestroyer::GroupPrivateWorker.perform_in(1.hour, group.id) + TodosDestroyer::GroupPrivateWorker.perform_in(Todo::WAIT_FOR_DELETE, group.id) end end diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb index b54b0bf6ef6..27013dc21ad 100644 --- a/app/services/issues/update_service.rb +++ b/app/services/issues/update_service.rb @@ -38,7 +38,7 @@ module Issues if issue.previous_changes.include?('confidential') # don't enqueue immediately to prevent todos removal in case of a mistake - TodosDestroyer::ConfidentialIssueWorker.perform_in(1.hour, issue.id) if issue.confidential? + TodosDestroyer::ConfidentialIssueWorker.perform_in(Todo::WAIT_FOR_DELETE, issue.id) if issue.confidential? create_confidentiality_note(issue) end diff --git a/app/services/members/base_service.rb b/app/services/members/base_service.rb index 8248f1441d7..db719132ca9 100644 --- a/app/services/members/base_service.rb +++ b/app/services/members/base_service.rb @@ -47,5 +47,11 @@ module Members raise "Unknown action '#{action}' on #{member}!" end end + + def enqueue_delete_todos(member) + type = member.is_a?(GroupMember) ? 'Group' : 'Project' + # don't enqueue immediately to prevent todos removal in case of a mistake + TodosDestroyer::EntityLeaveWorker.perform_in(Todo::WAIT_FOR_DELETE, member.user_id, member.source_id, type) + end end end diff --git a/app/services/members/destroy_service.rb b/app/services/members/destroy_service.rb index c186a5971dc..ae0c644e6c0 100644 --- a/app/services/members/destroy_service.rb +++ b/app/services/members/destroy_service.rb @@ -15,7 +15,7 @@ module Members notification_service.decline_access_request(member) end - enqeue_delete_todos(member) + enqueue_delete_todos(member) after_execute(member: member) @@ -24,12 +24,6 @@ module Members private - def enqeue_delete_todos(member) - type = member.is_a?(GroupMember) ? 'Group' : 'Project' - # don't enqueue immediately to prevent todos removal in case of a mistake - TodosDestroyer::EntityLeaveWorker.perform_in(1.hour, member.user_id, member.source_id, type) - end - def can_destroy_member?(member) can?(current_user, destroy_member_permission(member), member) end diff --git a/app/services/members/update_service.rb b/app/services/members/update_service.rb index 1f5618dae53..ff8d5c1d8c9 100644 --- a/app/services/members/update_service.rb +++ b/app/services/members/update_service.rb @@ -10,9 +10,18 @@ module Members if member.update(params) after_execute(action: permission, old_access_level: old_access_level, member: member) + + # Deletes only confidential issues todos for guests + enqueue_delete_todos(member) if downgrading_to_guest? end member end + + private + + def downgrading_to_guest? + params[:access_level] == Gitlab::Access::GUEST + end end end diff --git a/app/services/merge_requests/build_service.rb b/app/services/merge_requests/build_service.rb index 0e76d2cc3ab..c37d867672c 100644 --- a/app/services/merge_requests/build_service.rb +++ b/app/services/merge_requests/build_service.rb @@ -13,7 +13,7 @@ module MergeRequests merge_request.source_project = find_source_project merge_request.target_project = find_target_project merge_request.target_branch = find_target_branch - merge_request.can_be_created = branches_valid? + merge_request.can_be_created = projects_and_branches_valid? # compare branches only if branches are valid, otherwise # compare_branches may raise an error @@ -44,15 +44,19 @@ module MergeRequests to: :merge_request def find_source_project - return source_project if source_project.present? && can?(current_user, :read_project, source_project) + return source_project if source_project.present? && can?(current_user, :create_merge_request_from, source_project) project end def find_target_project - return target_project if target_project.present? && can?(current_user, :read_project, target_project) + return target_project if target_project.present? && can?(current_user, :create_merge_request_in, target_project) - project.default_merge_request_target + target_project = project.default_merge_request_target + + return target_project if target_project.present? && can?(current_user, :create_merge_request_in, target_project) + + project end def find_target_branch @@ -67,10 +71,11 @@ module MergeRequests params[:target_branch].present? end - def branches_valid? + def projects_and_branches_valid? + return false if source_project.nil? || target_project.nil? return false unless source_branch_specified? || target_branch_specified? - validate_branches + validate_projects_and_branches errors.blank? end @@ -89,7 +94,12 @@ module MergeRequests end end - def validate_branches + def validate_projects_and_branches + merge_request.validate_target_project + merge_request.validate_fork + + return if errors.any? + add_error('You must select source and target branch') unless branches_present? add_error('You must select different branches') if same_source_and_target? add_error("Source branch \"#{source_branch}\" does not exist") unless source_branch_exists? diff --git a/app/services/projects/lfs_pointers/lfs_download_service.rb b/app/services/projects/lfs_pointers/lfs_download_service.rb index 1c4a8d05be6..f9b9781ad5f 100644 --- a/app/services/projects/lfs_pointers/lfs_download_service.rb +++ b/app/services/projects/lfs_pointers/lfs_download_service.rb @@ -4,6 +4,8 @@ module Projects module LfsPointers class LfsDownloadService < BaseService + VALID_PROTOCOLS = %w[http https].freeze + # rubocop: disable CodeReuse/ActiveRecord def execute(oid, url) return unless project&.lfs_enabled? && oid.present? && url.present? @@ -11,6 +13,7 @@ module Projects return if LfsObject.exists?(oid: oid) sanitized_uri = Gitlab::UrlSanitizer.new(url) + Gitlab::UrlBlocker.validate!(sanitized_uri.sanitized_url, protocols: VALID_PROTOCOLS) with_tmp_file(oid) do |file| size = download_and_save_file(file, sanitized_uri) diff --git a/app/services/projects/update_service.rb b/app/services/projects/update_service.rb index f25a4e30938..6063392da0a 100644 --- a/app/services/projects/update_service.rb +++ b/app/services/projects/update_service.rb @@ -61,9 +61,9 @@ module Projects if project.previous_changes.include?(:visibility_level) && project.private? # don't enqueue immediately to prevent todos removal in case of a mistake - TodosDestroyer::ProjectPrivateWorker.perform_in(1.hour, project.id) + TodosDestroyer::ProjectPrivateWorker.perform_in(Todo::WAIT_FOR_DELETE, project.id) elsif (project_changed_feature_keys & todos_features_changes).present? - TodosDestroyer::PrivateFeaturesWorker.perform_in(1.hour, project.id) + TodosDestroyer::PrivateFeaturesWorker.perform_in(Todo::WAIT_FOR_DELETE, project.id) end if project.previous_changes.include?('path') diff --git a/changelogs/unreleased/54857-fix-templates-path-traversal.yml b/changelogs/unreleased/54857-fix-templates-path-traversal.yml new file mode 100644 index 00000000000..0da02432c60 --- /dev/null +++ b/changelogs/unreleased/54857-fix-templates-path-traversal.yml @@ -0,0 +1,5 @@ +--- +title: Prevent a path traversal attack on global file templates +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-11-4-54377-label-milestone-name-xss.yml b/changelogs/unreleased/security-11-4-54377-label-milestone-name-xss.yml new file mode 100644 index 00000000000..b20f9fd83cc --- /dev/null +++ b/changelogs/unreleased/security-11-4-54377-label-milestone-name-xss.yml @@ -0,0 +1,5 @@ +--- +title: Escape label and milestone titles to prevent XSS in GFM autocomplete +merge_request: 2742 +author: +type: security diff --git a/changelogs/unreleased/security-2754-fix-lfs-import.yml b/changelogs/unreleased/security-2754-fix-lfs-import.yml new file mode 100644 index 00000000000..e8e74c9c3f6 --- /dev/null +++ b/changelogs/unreleased/security-2754-fix-lfs-import.yml @@ -0,0 +1,5 @@ +--- +title: Validate LFS hrefs before downloading them +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-bvl-fix-cross-project-mr-exposure.yml b/changelogs/unreleased/security-bvl-fix-cross-project-mr-exposure.yml new file mode 100644 index 00000000000..11aae4428fb --- /dev/null +++ b/changelogs/unreleased/security-bvl-fix-cross-project-mr-exposure.yml @@ -0,0 +1,5 @@ +--- +title: Don't expose cross project repositories through diffs when creating merge reqeusts +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-fix-ssrf-import-url-remote-mirror.yml b/changelogs/unreleased/security-fix-ssrf-import-url-remote-mirror.yml new file mode 100644 index 00000000000..7ba7aa21090 --- /dev/null +++ b/changelogs/unreleased/security-fix-ssrf-import-url-remote-mirror.yml @@ -0,0 +1,5 @@ +--- +title: Fix SSRF with import_url and remote mirror url +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-import-symlink.yml b/changelogs/unreleased/security-import-symlink.yml new file mode 100644 index 00000000000..fe1b6eccf9e --- /dev/null +++ b/changelogs/unreleased/security-import-symlink.yml @@ -0,0 +1,5 @@ +--- +title: Fix persistent symlink in project import +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-master-url-rel.yml b/changelogs/unreleased/security-master-url-rel.yml new file mode 100644 index 00000000000..75f599f6bcd --- /dev/null +++ b/changelogs/unreleased/security-master-url-rel.yml @@ -0,0 +1,5 @@ +--- +title: Set URL rel attribute for broken URLs. +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-todos_not_redacted_for_guests.yml b/changelogs/unreleased/security-todos_not_redacted_for_guests.yml new file mode 100644 index 00000000000..be0ae9a7193 --- /dev/null +++ b/changelogs/unreleased/security-todos_not_redacted_for_guests.yml @@ -0,0 +1,5 @@ +--- +title: Delete confidential todos for user when downgraded to Guest +merge_request: +author: +type: security diff --git a/doc/workflow/todos.md b/doc/workflow/todos.md index f94d592d0db..830f17aa7f2 100644 --- a/doc/workflow/todos.md +++ b/doc/workflow/todos.md @@ -35,6 +35,9 @@ A Todo appears in your Todos dashboard when: - the author, or - have set it to automatically merge once pipeline succeeds. +NOTE: **Note:** +When an user no longer has access to a resource related to a Todo like an issue, merge request, project or group the related Todos, for security reasons, gets deleted within the next hour. The delete is delayed to prevent data loss in case user got their access revoked by mistake. + ### Directly addressed Todos > [Introduced][ce-7926] in GitLab 9.0. diff --git a/lib/api/templates.rb b/lib/api/templates.rb index 8dab19d50c2..51f357d9477 100644 --- a/lib/api/templates.rb +++ b/lib/api/templates.rb @@ -82,7 +82,7 @@ module API params do requires :name, type: String, desc: 'The name of the template' end - get "templates/#{template_type}/:name" do + get "templates/#{template_type}/:name", requirements: { name: /[\w\.-]+/ } do finder = TemplateFinder.build(template_type, nil, name: declared(params)[:name]) new_template = finder.execute diff --git a/lib/banzai/filter/external_link_filter.rb b/lib/banzai/filter/external_link_filter.rb index 2e6d742de27..4f60b6f84c6 100644 --- a/lib/banzai/filter/external_link_filter.rb +++ b/lib/banzai/filter/external_link_filter.rb @@ -9,11 +9,10 @@ module Banzai def call links.each do |node| uri = uri(node['href'].to_s) - next unless uri - node.set_attribute('href', uri.to_s) + node.set_attribute('href', uri.to_s) if uri - if SCHEMES.include?(uri.scheme) && external_url?(uri) + if SCHEMES.include?(uri&.scheme) && !internal_url?(uri) node.set_attribute('rel', 'nofollow noreferrer noopener') node.set_attribute('target', '_blank') end @@ -35,11 +34,12 @@ module Banzai doc.xpath(query) end - def external_url?(uri) + def internal_url?(uri) + return false if uri.nil? # Relative URLs miss a hostname - return false unless uri.hostname + return true unless uri.hostname - uri.hostname != internal_url.hostname + uri.hostname == internal_url.hostname end def internal_url diff --git a/lib/gitlab/import_export/command_line_util.rb b/lib/gitlab/import_export/command_line_util.rb index 3adc44f8044..1f3c1c51981 100644 --- a/lib/gitlab/import_export/command_line_util.rb +++ b/lib/gitlab/import_export/command_line_util.rb @@ -1,7 +1,8 @@ module Gitlab module ImportExport module CommandLineUtil - DEFAULT_MODE = 0700 + UNTAR_MASK = 'u+rwX,go+rX,go-w'.freeze + DEFAULT_DIR_MODE = 0700 def tar_czf(archive:, dir:) tar_with_options(archive: archive, dir: dir, options: 'czf') @@ -12,8 +13,8 @@ module Gitlab end def mkdir_p(path) - FileUtils.mkdir_p(path, mode: DEFAULT_MODE) - FileUtils.chmod(DEFAULT_MODE, path) + FileUtils.mkdir_p(path, mode: DEFAULT_DIR_MODE) + FileUtils.chmod(DEFAULT_DIR_MODE, path) end private @@ -39,6 +40,7 @@ module Gitlab def untar_with_options(archive:, dir:, options:) execute(%W(tar -#{options} #{archive} -C #{dir})) + execute(%W(chmod -R #{UNTAR_MASK} #{dir})) end def execute(cmd) diff --git a/lib/gitlab/template/finders/global_template_finder.rb b/lib/gitlab/template/finders/global_template_finder.rb index b08d9a99e99..db3fe736229 100644 --- a/lib/gitlab/template/finders/global_template_finder.rb +++ b/lib/gitlab/template/finders/global_template_finder.rb @@ -16,6 +16,10 @@ module Gitlab def find(key) file_name = "#{key}#{@extension}" + # The key is untrusted input, so ensure we can't be directed outside + # of base_dir + Gitlab::Utils.check_path_traversal!(file_name) + directory = select_directory(file_name) directory ? File.join(category_directory(directory), file_name) : nil end diff --git a/lib/gitlab/template/finders/repo_template_finder.rb b/lib/gitlab/template/finders/repo_template_finder.rb index 9140ace879f..8836fe07af6 100644 --- a/lib/gitlab/template/finders/repo_template_finder.rb +++ b/lib/gitlab/template/finders/repo_template_finder.rb @@ -24,6 +24,11 @@ module Gitlab def find(key) file_name = "#{key}#{@extension}" + + # The key is untrusted input, so ensure we can't be directed outside + # of base_dir inside the repository + Gitlab::Utils.check_path_traversal!(file_name) + directory = select_directory(file_name) raise FileNotFoundError if directory.nil? diff --git a/lib/gitlab/utils.rb b/lib/gitlab/utils.rb index aeda66763e8..6ec91b26799 100644 --- a/lib/gitlab/utils.rb +++ b/lib/gitlab/utils.rb @@ -2,6 +2,15 @@ module Gitlab module Utils extend self + # Ensure that the relative path will not traverse outside the base directory + def check_path_traversal!(path) + raise StandardError.new("Invalid path") if path.start_with?("..#{File::SEPARATOR}") || + path.include?("#{File::SEPARATOR}..#{File::SEPARATOR}") || + path.end_with?("#{File::SEPARATOR}..") + + path + end + # Run system command without outputting to stdout. # # @param cmd [Array<String>] diff --git a/spec/features/issues/gfm_autocomplete_spec.rb b/spec/features/issues/gfm_autocomplete_spec.rb index 7c591dacce5..b8d7e48510f 100644 --- a/spec/features/issues/gfm_autocomplete_spec.rb +++ b/spec/features/issues/gfm_autocomplete_spec.rb @@ -3,6 +3,8 @@ require 'rails_helper' describe 'GFM autocomplete', :js do let(:issue_xss_title) { 'This will execute alert<img src=x onerror=alert(2)<img src=x onerror=alert(1)>' } let(:user_xss_title) { 'eve <img src=x onerror=alert(2)<img src=x onerror=alert(1)>' } + let(:label_xss_title) { 'alert label <img src=x onerror="alert(\'Hello xss\');" a'} + let(:milestone_xss_title) { 'alert milestone <img src=x onerror="alert(\'Hello xss\');" a' } let(:user_xss) { create(:user, name: user_xss_title, username: 'xss.user') } let(:user) { create(:user, name: '💃speciąl someone💃', username: 'someone.special') } @@ -26,10 +28,14 @@ describe 'GFM autocomplete', :js do simulate_input('#issue-description', "@#{user.name[0...3]}") + wait_for_requests + find('.atwho-view .cur').click click_button 'Save changes' + wait_for_requests + expect(find('.description')).to have_content(user.to_reference) end @@ -48,6 +54,8 @@ describe 'GFM autocomplete', :js do find('#note-body').native.send_keys('#') end + wait_for_requests + expect(page).to have_selector('.atwho-container') page.within '.atwho-container #at-view-issues' do @@ -60,6 +68,8 @@ describe 'GFM autocomplete', :js do find('#note-body').native.send_keys('@ev') end + wait_for_requests + expect(page).to have_selector('.atwho-container') page.within '.atwho-container #at-view-users' do @@ -67,6 +77,22 @@ describe 'GFM autocomplete', :js do end end + it 'opens autocomplete menu for Milestone when field starts with text with item escaping HTML characters' do + create(:milestone, project: project, title: milestone_xss_title) + + page.within '.timeline-content-form' do + find('#note-body').native.send_keys('%') + end + + wait_for_requests + + expect(page).to have_selector('.atwho-container') + + page.within '.atwho-container #at-view-milestones' do + expect(find('li').text).to have_content('alert milestone') + end + end + it 'doesnt open autocomplete menu character is prefixed with text' do page.within '.timeline-content-form' do find('#note-body').native.send_keys('testing') @@ -259,6 +285,21 @@ describe 'GFM autocomplete', :js do let!(:bug) { create(:label, project: project, title: 'bug') } let!(:feature_proposal) { create(:label, project: project, title: 'feature proposal') } + it 'opens autocomplete menu for Labels when field starts with text with item escaping HTML characters' do + create(:label, project: project, title: label_xss_title) + + note = find('#note-body') + + # It should show all the labels on "~". + type(note, '~') + + wait_for_requests + + page.within '.atwho-container #at-view-labels' do + expect(find('.atwho-view-ul').text).to have_content('alert label') + end + end + context 'when no labels are assigned' do it 'shows labels' do note = find('#note-body') diff --git a/spec/features/merge_request/user_tries_to_access_private_repository_through_new_mr_spec.rb b/spec/features/merge_request/user_tries_to_access_private_repository_through_new_mr_spec.rb new file mode 100644 index 00000000000..9318b5f1ebb --- /dev/null +++ b/spec/features/merge_request/user_tries_to_access_private_repository_through_new_mr_spec.rb @@ -0,0 +1,37 @@ +require 'spec_helper' + +describe 'Merge Request > Tries to access private repo of public project' do + let(:current_user) { create(:user) } + let(:private_project) do + create(:project, :public, :repository, + path: 'nothing-to-see-here', + name: 'nothing to see here', + repository_access_level: ProjectFeature::PRIVATE) + end + let(:owned_project) do + create(:project, :public, :repository, + namespace: current_user.namespace, + creator: current_user) + end + + context 'when the user enters the querystring info for the other project' do + let(:mr_path) do + project_new_merge_request_diffs_path( + owned_project, + merge_request: { + source_project_id: private_project.id, + source_branch: 'feature' + } + ) + end + + before do + sign_in current_user + visit mr_path + end + + it "does not mention the project the user can't see the repo of" do + expect(page).not_to have_content('nothing-to-see-here') + end + end +end diff --git a/spec/fixtures/symlink_export.tar.gz b/spec/fixtures/symlink_export.tar.gz Binary files differnew file mode 100644 index 00000000000..f295f69c56c --- /dev/null +++ b/spec/fixtures/symlink_export.tar.gz diff --git a/spec/lib/banzai/filter/external_link_filter_spec.rb b/spec/lib/banzai/filter/external_link_filter_spec.rb index 2a3c0cd78b8..e6dae8d5382 100644 --- a/spec/lib/banzai/filter/external_link_filter_spec.rb +++ b/spec/lib/banzai/filter/external_link_filter_spec.rb @@ -49,16 +49,16 @@ describe Banzai::Filter::ExternalLinkFilter do end context 'for invalid urls' do - it 'skips broken hrefs' do + it 'adds rel and target attributes to broken hrefs' do doc = filter %q(<p><a href="don't crash on broken urls">Google</a></p>) - expected = %q(<p><a href="don't%20crash%20on%20broken%20urls">Google</a></p>) + expected = %q(<p><a href="don't%20crash%20on%20broken%20urls" rel="nofollow noreferrer noopener" target="_blank">Google</a></p>) expect(doc.to_html).to eq(expected) end - it 'skips improperly formatted mailtos' do + it 'adds rel and target to improperly formatted mailtos' do doc = filter %q(<p><a href="mailto://jblogs@example.com">Email</a></p>) - expected = %q(<p><a href="mailto://jblogs@example.com">Email</a></p>) + expected = %q(<p><a href="mailto://jblogs@example.com" rel="nofollow noreferrer noopener" target="_blank">Email</a></p>) expect(doc.to_html).to eq(expected) end diff --git a/spec/lib/gitlab/import_export/command_line_util_spec.rb b/spec/lib/gitlab/import_export/command_line_util_spec.rb new file mode 100644 index 00000000000..8e5e0aefac0 --- /dev/null +++ b/spec/lib/gitlab/import_export/command_line_util_spec.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::ImportExport::CommandLineUtil do + include ExportFileHelper + + let(:path) { "#{Dir.tmpdir}/symlink_test" } + let(:archive) { 'spec/fixtures/symlink_export.tar.gz' } + let(:shared) { Gitlab::ImportExport::Shared.new(nil) } + + subject do + Class.new do + include Gitlab::ImportExport::CommandLineUtil + + def initialize + @shared = Gitlab::ImportExport::Shared.new(nil) + end + end.new + end + + before do + FileUtils.mkdir_p(path) + subject.untar_zxf(archive: archive, dir: path) + end + + after do + FileUtils.rm_rf(path) + end + + it 'has the right mask for project.json' do + expect(file_permissions("#{path}/project.json")).to eq(0755) # originally 777 + end + + it 'has the right mask for uploads' do + expect(file_permissions("#{path}/uploads")).to eq(0755) # originally 555 + end +end diff --git a/spec/lib/gitlab/import_export/file_importer_spec.rb b/spec/lib/gitlab/import_export/file_importer_spec.rb index bf34cefe18f..fbc9bcd2df5 100644 --- a/spec/lib/gitlab/import_export/file_importer_spec.rb +++ b/spec/lib/gitlab/import_export/file_importer_spec.rb @@ -1,6 +1,8 @@ require 'spec_helper' describe Gitlab::ImportExport::FileImporter do + include ExportFileHelper + let(:shared) { Gitlab::ImportExport::Shared.new(nil) } let(:storage_path) { "#{Dir.tmpdir}/file_importer_spec" } let(:valid_file) { "#{shared.export_path}/valid.json" } @@ -8,6 +10,7 @@ describe Gitlab::ImportExport::FileImporter do let(:hidden_symlink_file) { "#{shared.export_path}/.hidden" } let(:subfolder_symlink_file) { "#{shared.export_path}/subfolder/invalid.json" } let(:evil_symlink_file) { "#{shared.export_path}/.\nevil" } + let(:custom_mode_symlink_file) { "#{shared.export_path}/symlink.mode" } before do stub_const('Gitlab::ImportExport::FileImporter::MAX_RETRIES', 0) @@ -45,10 +48,18 @@ describe Gitlab::ImportExport::FileImporter do expect(File.exist?(subfolder_symlink_file)).to be false end + it 'removes symlinks without any file permissions' do + expect(File.exist?(custom_mode_symlink_file)).to be false + end + it 'does not remove a valid file' do expect(File.exist?(valid_file)).to be true end + it 'does not change a valid file permissions' do + expect(file_permissions(valid_file)).not_to eq(0000) + end + it 'creates the file in the right subfolder' do expect(shared.export_path).to include('test/abcd') end @@ -84,5 +95,7 @@ describe Gitlab::ImportExport::FileImporter do FileUtils.ln_s(valid_file, subfolder_symlink_file) FileUtils.ln_s(valid_file, hidden_symlink_file) FileUtils.ln_s(valid_file, evil_symlink_file) + FileUtils.ln_s(valid_file, custom_mode_symlink_file) + FileUtils.chmod_R(0000, custom_mode_symlink_file) end end diff --git a/spec/lib/gitlab/template/finders/global_template_finder_spec.rb b/spec/lib/gitlab/template/finders/global_template_finder_spec.rb new file mode 100644 index 00000000000..c7f58fbd2a5 --- /dev/null +++ b/spec/lib/gitlab/template/finders/global_template_finder_spec.rb @@ -0,0 +1,35 @@ +require 'spec_helper' + +describe Gitlab::Template::Finders::GlobalTemplateFinder do + let(:base_dir) { Dir.mktmpdir } + + def create_template!(name_with_category) + full_path = File.join(base_dir, name_with_category) + FileUtils.mkdir_p(File.dirname(full_path)) + FileUtils.touch(full_path) + end + + after do + FileUtils.rm_rf(base_dir) + end + + subject(:finder) { described_class.new(base_dir, '', 'Foo' => '', 'Bar' => 'bar') } + + describe '.find' do + it 'finds a template in the Foo category' do + create_template!('test-template') + + expect(finder.find('test-template')).to be_present + end + + it 'finds a template in the Bar category' do + create_template!('bar/test-template') + + expect(finder.find('test-template')).to be_present + end + + it 'does not permit path traversal requests' do + expect { finder.find('../foo') }.to raise_error(/Invalid path/) + end + end +end diff --git a/spec/lib/gitlab/template/finders/repo_template_finders_spec.rb b/spec/lib/gitlab/template/finders/repo_template_finders_spec.rb index 2eabccd5dff..e329d55d837 100644 --- a/spec/lib/gitlab/template/finders/repo_template_finders_spec.rb +++ b/spec/lib/gitlab/template/finders/repo_template_finders_spec.rb @@ -25,6 +25,10 @@ describe Gitlab::Template::Finders::RepoTemplateFinder do expect(result).to eq('files/html/500.html') end + + it 'does not permit path traversal requests' do + expect { finder.find('../foo') }.to raise_error(/Invalid path/) + end end describe '#list_files_for' do diff --git a/spec/lib/gitlab/utils_spec.rb b/spec/lib/gitlab/utils_spec.rb index 4ba99009855..f7528f296c3 100644 --- a/spec/lib/gitlab/utils_spec.rb +++ b/spec/lib/gitlab/utils_spec.rb @@ -2,7 +2,33 @@ require 'spec_helper' describe Gitlab::Utils do delegate :to_boolean, :boolean_to_yes_no, :slugify, :random_string, :which, :ensure_array_from_string, - :bytes_to_megabytes, to: :described_class + :bytes_to_megabytes, :check_path_traversal!, to: :described_class + + describe '.check_path_traversal!' do + it 'detects path traversal at the start of the string' do + expect { check_path_traversal!('../foo') }.to raise_error(/Invalid path/) + end + + it 'detects path traversal at the start of the string, even to just the subdirectory' do + expect { check_path_traversal!('../') }.to raise_error(/Invalid path/) + end + + it 'detects path traversal in the middle of the string' do + expect { check_path_traversal!('foo/../../bar') }.to raise_error(/Invalid path/) + end + + it 'detects path traversal at the end of the string when slash-terminates' do + expect { check_path_traversal!('foo/../') }.to raise_error(/Invalid path/) + end + + it 'detects path traversal at the end of the string' do + expect { check_path_traversal!('foo/..') }.to raise_error(/Invalid path/) + end + + it 'does nothing for a safe string' do + expect(check_path_traversal!('./foo')).to eq('./foo') + end + end describe '.slugify' do { diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 5034b75eb74..57cf4db3045 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -266,6 +266,13 @@ describe Project do expect(project2.errors[:import_url].first).to include('Requests to localhost are not allowed') end + it 'does not allow import_url pointing to the local network' do + project = build(:project, import_url: 'https://192.168.1.1') + + expect(project).to be_invalid + expect(project.errors[:import_url].first).to include('Requests to the local network are not allowed') + end + it "does not allow import_url with invalid ports" do project2 = build(:project, import_url: 'http://github.com:25/t.git') diff --git a/spec/models/remote_mirror_spec.rb b/spec/models/remote_mirror_spec.rb index 3d316fb3c5b..4374421ccc8 100644 --- a/spec/models/remote_mirror_spec.rb +++ b/spec/models/remote_mirror_spec.rb @@ -24,6 +24,20 @@ describe RemoteMirror do expect(remote_mirror).to be_invalid expect(remote_mirror.errors[:url].first).to include('Username needs to start with an alphanumeric character') end + + it 'does not allow url pointing to localhost' do + remote_mirror = build(:remote_mirror, url: 'http://127.0.0.2/t.git') + + expect(remote_mirror).to be_invalid + expect(remote_mirror.errors[:url].first).to include('Requests to loopback addresses are not allowed') + end + + it 'does not allow url pointing to the local network' do + remote_mirror = build(:remote_mirror, url: 'https://192.168.1.1') + + expect(remote_mirror).to be_invalid + expect(remote_mirror.errors[:url].first).to include('Requests to the local network are not allowed') + end end end diff --git a/spec/services/groups/update_service_spec.rb b/spec/services/groups/update_service_spec.rb index 7c5c7409cc1..3e9ae68a0cb 100644 --- a/spec/services/groups/update_service_spec.rb +++ b/spec/services/groups/update_service_spec.rb @@ -50,7 +50,7 @@ describe Groups::UpdateService do create(:project, :private, group: internal_group) expect(TodosDestroyer::GroupPrivateWorker).to receive(:perform_in) - .with(1.hour, internal_group.id) + .with(Todo::WAIT_FOR_DELETE, internal_group.id) end it "changes permission level to private" do diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index 07aa8449a66..5f7e9fd320e 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -77,7 +77,7 @@ describe Issues::UpdateService, :mailer do end it 'enqueues ConfidentialIssueWorker when an issue is made confidential' do - expect(TodosDestroyer::ConfidentialIssueWorker).to receive(:perform_in).with(1.hour, issue.id) + expect(TodosDestroyer::ConfidentialIssueWorker).to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, issue.id) update_issue(confidential: true) end diff --git a/spec/services/members/destroy_service_spec.rb b/spec/services/members/destroy_service_spec.rb index 0a5220c7c61..5aa7165e135 100644 --- a/spec/services/members/destroy_service_spec.rb +++ b/spec/services/members/destroy_service_spec.rb @@ -22,7 +22,7 @@ describe Members::DestroyService do shared_examples 'a service destroying a member' do before do type = member.is_a?(GroupMember) ? 'Group' : 'Project' - expect(TodosDestroyer::EntityLeaveWorker).to receive(:perform_in).with(1.hour, member.user_id, member.source_id, type) + expect(TodosDestroyer::EntityLeaveWorker).to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, member.user_id, member.source_id, type) end it 'destroys the member' do diff --git a/spec/services/members/update_service_spec.rb b/spec/services/members/update_service_spec.rb index 6d19a95ffeb..599ed39ca37 100644 --- a/spec/services/members/update_service_spec.rb +++ b/spec/services/members/update_service_spec.rb @@ -20,11 +20,28 @@ describe Members::UpdateService do shared_examples 'a service updating a member' do it 'updates the member' do + expect(TodosDestroyer::EntityLeaveWorker).not_to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, member.user_id, member.source_id, source.class.name) + updated_member = described_class.new(current_user, params).execute(member, permission: permission) expect(updated_member).to be_valid expect(updated_member.access_level).to eq(Gitlab::Access::MAINTAINER) end + + context 'when member is downgraded to guest' do + let(:params) do + { access_level: Gitlab::Access::GUEST } + end + + it 'schedules to delete confidential todos' do + expect(TodosDestroyer::EntityLeaveWorker).to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, member.user_id, member.source_id, source.class.name).once + + updated_member = described_class.new(current_user, params).execute(member, permission: permission) + + expect(updated_member).to be_valid + expect(updated_member.access_level).to eq(Gitlab::Access::GUEST) + end + end end before do diff --git a/spec/services/merge_requests/build_service_spec.rb b/spec/services/merge_requests/build_service_spec.rb index 9f1da7d9419..9753876ffcb 100644 --- a/spec/services/merge_requests/build_service_spec.rb +++ b/spec/services/merge_requests/build_service_spec.rb @@ -3,6 +3,7 @@ require 'spec_helper' describe MergeRequests::BuildService do using RSpec::Parameterized::TableSyntax include RepoHelpers + include ProjectForksHelper let(:project) { create(:project, :repository) } let(:source_project) { nil } @@ -44,7 +45,7 @@ describe MergeRequests::BuildService do describe '#execute' do it 'calls the compare service with the correct arguments' do - allow_any_instance_of(described_class).to receive(:branches_valid?).and_return(true) + allow_any_instance_of(described_class).to receive(:projects_and_branches_valid?).and_return(true) expect(CompareService).to receive(:new) .with(project, Gitlab::Git::BRANCH_REF_PREFIX + source_branch) .and_call_original @@ -375,11 +376,27 @@ describe MergeRequests::BuildService do end end + context 'target_project is set but repo is not accessible by current_user' do + let(:target_project) do + create(:project, :public, :repository, repository_access_level: ProjectFeature::PRIVATE) + end + + it 'sets target project correctly' do + expect(merge_request.target_project).to eq(project) + end + end + context 'source_project is set and accessible by current_user' do let(:source_project) { create(:project, :public, :repository)} let(:commits) { Commit.decorate([commit_1], project) } - it 'sets target project correctly' do + before do + # To create merge requests _from_ a project the user needs at least + # developer access + source_project.add_developer(user) + end + + it 'sets source project correctly' do expect(merge_request.source_project).to eq(source_project) end end @@ -388,9 +405,41 @@ describe MergeRequests::BuildService do let(:source_project) { create(:project, :private, :repository)} let(:commits) { Commit.decorate([commit_1], project) } - it 'sets target project correctly' do + it 'sets source project correctly' do + expect(merge_request.source_project).to eq(project) + end + end + + context 'source_project is set but the user cannot create merge requests from the project' do + let(:source_project) do + create(:project, :public, :repository, merge_requests_access_level: ProjectFeature::PRIVATE) + end + + it 'sets the source_project correctly' do expect(merge_request.source_project).to eq(project) end end + + context 'target_project is not in the fork network of source_project' do + let(:target_project) { create(:project, :public, :repository) } + + it 'adds an error to the merge request' do + expect(merge_request.errors[:validate_fork]).to contain_exactly('Source project is not a fork of the target project') + end + end + + context 'target_project is in the fork network of source project but no longer accessible' do + let!(:project) { fork_project(target_project, user, namespace: user.namespace, repository: true) } + let(:source_project) { project } + let(:target_project) { create(:project, :public, :repository) } + + before do + target_project.update(visibility_level: Gitlab::VisibilityLevel::PRIVATE) + end + + it 'sets the target_project correctly' do + expect(merge_request.target_project).to eq(project) + end + end end end diff --git a/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb b/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb index 6af5bfc7689..d7d7f1874eb 100644 --- a/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb +++ b/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb @@ -54,6 +54,18 @@ describe Projects::LfsPointers::LfsDownloadService do end end + context 'when a bad URL is used' do + where(download_link: ['/etc/passwd', 'ftp://example.com', 'http://127.0.0.2']) + + with_them do + it 'does not download the file' do + expect(subject).not_to receive(:download_and_save_file) + + expect { subject.execute(oid, download_link) }.not_to change { LfsObject.count } + end + end + end + context 'when an lfs object with the same oid already exists' do before do create(:lfs_object, oid: 'oid') diff --git a/spec/services/projects/update_service_spec.rb b/spec/services/projects/update_service_spec.rb index d58ff2cedc0..8adfc63222e 100644 --- a/spec/services/projects/update_service_spec.rb +++ b/spec/services/projects/update_service_spec.rb @@ -41,7 +41,7 @@ describe Projects::UpdateService do end it 'updates the project to private' do - expect(TodosDestroyer::ProjectPrivateWorker).to receive(:perform_in).with(1.hour, project.id) + expect(TodosDestroyer::ProjectPrivateWorker).to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, project.id) result = update_project(project, user, visibility_level: Gitlab::VisibilityLevel::PRIVATE) @@ -191,7 +191,7 @@ describe Projects::UpdateService do context 'when changing feature visibility to private' do it 'updates the visibility correctly' do expect(TodosDestroyer::PrivateFeaturesWorker) - .to receive(:perform_in).with(1.hour, project.id) + .to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, project.id) result = update_project(project, user, project_feature_attributes: { issues_access_level: ProjectFeature::PRIVATE } diff --git a/spec/support/import_export/export_file_helper.rb b/spec/support/import_export/export_file_helper.rb index d9ed405baf4..0eb1aec4070 100644 --- a/spec/support/import_export/export_file_helper.rb +++ b/spec/support/import_export/export_file_helper.rb @@ -133,6 +133,6 @@ module ExportFileHelper end def file_permissions(file) - File.stat(file).mode & 0777 + File.lstat(file).mode & 0777 end end |