diff options
author | Robert Speicher <robert@gitlab.com> | 2017-05-10 18:26:02 +0000 |
---|---|---|
committer | Robert Speicher <robert@gitlab.com> | 2017-05-10 18:26:02 +0000 |
commit | 180ec7113e358a7f8388e1436dc0670a11ba68df (patch) | |
tree | fba74833a7e89d6c19160a0d7e02355c5215c8df | |
parent | 09c4d27ae48ceb181f86657043af2a129c17dabf (diff) | |
parent | ebd8b7f60f41358df562625a4692f352b86b8c80 (diff) | |
download | gitlab-ce-180ec7113e358a7f8388e1436dc0670a11ba68df.tar.gz |
Merge branch 'bvl-security-patches' into 'master'
Security patches -> `master`
See merge request !11230
56 files changed, 831 insertions, 311 deletions
diff --git a/app/controllers/dashboard/snippets_controller.rb b/app/controllers/dashboard/snippets_controller.rb index bcfdbe14be9..8dd91264451 100644 --- a/app/controllers/dashboard/snippets_controller.rb +++ b/app/controllers/dashboard/snippets_controller.rb @@ -1,11 +1,10 @@ class Dashboard::SnippetsController < Dashboard::ApplicationController def index - @snippets = SnippetsFinder.new.execute( + @snippets = SnippetsFinder.new( current_user, - filter: :by_user, - user: current_user, + author: current_user, scope: params[:scope] - ) + ).execute @snippets = @snippets.page(params[:page]) end end diff --git a/app/controllers/explore/groups_controller.rb b/app/controllers/explore/groups_controller.rb index 68228c095da..81883c543ba 100644 --- a/app/controllers/explore/groups_controller.rb +++ b/app/controllers/explore/groups_controller.rb @@ -1,6 +1,6 @@ class Explore::GroupsController < Explore::ApplicationController def index - @groups = GroupsFinder.new.execute(current_user) + @groups = GroupsFinder.new(current_user).execute @groups = @groups.search(params[:filter_groups]) if params[:filter_groups].present? @groups = @groups.sort(@sort = params[:sort]) @groups = @groups.page(params[:page]) diff --git a/app/controllers/explore/snippets_controller.rb b/app/controllers/explore/snippets_controller.rb index 28760c3f84b..d3f0e033068 100644 --- a/app/controllers/explore/snippets_controller.rb +++ b/app/controllers/explore/snippets_controller.rb @@ -1,6 +1,6 @@ class Explore::SnippetsController < Explore::ApplicationController def index - @snippets = SnippetsFinder.new.execute(current_user, filter: :all) + @snippets = SnippetsFinder.new(current_user).execute @snippets = @snippets.page(params[:page]) end end diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 46c3ff10694..1515173d0ac 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -64,7 +64,7 @@ class GroupsController < Groups::ApplicationController end def subgroups - @nested_groups = group.children + @nested_groups = GroupsFinder.new(current_user, parent: group).execute @nested_groups = @nested_groups.search(params[:filter_groups]) if params[:filter_groups].present? end diff --git a/app/controllers/projects/snippets_controller.rb b/app/controllers/projects/snippets_controller.rb index 66f913f8f9d..3b2b0d9e502 100644 --- a/app/controllers/projects/snippets_controller.rb +++ b/app/controllers/projects/snippets_controller.rb @@ -23,12 +23,11 @@ class Projects::SnippetsController < Projects::ApplicationController respond_to :html def index - @snippets = SnippetsFinder.new.execute( + @snippets = SnippetsFinder.new( current_user, - filter: :by_project, project: @project, scope: params[:scope] - ) + ).execute @snippets = @snippets.page(params[:page]) if @snippets.out_of_range? && @snippets.total_pages != 0 redirect_to namespace_project_snippets_path(page: @snippets.total_pages) diff --git a/app/controllers/snippets_controller.rb b/app/controllers/snippets_controller.rb index 19e07e3ab86..7445f61195d 100644 --- a/app/controllers/snippets_controller.rb +++ b/app/controllers/snippets_controller.rb @@ -27,12 +27,8 @@ class SnippetsController < ApplicationController return render_404 unless @user - @snippets = SnippetsFinder.new.execute(current_user, { - filter: :by_user, - user: @user, - scope: params[:scope] - }) - .page(params[:page]) + @snippets = SnippetsFinder.new(current_user, author: @user, scope: params[:scope]) + .execute.page(params[:page]) render 'index' else @@ -103,20 +99,20 @@ class SnippetsController < ApplicationController protected def snippet - @snippet ||= if current_user - PersonalSnippet.where("author_id = ? OR visibility_level IN (?)", - current_user.id, - [Snippet::PUBLIC, Snippet::INTERNAL]). - find(params[:id]) - else - PersonalSnippet.find(params[:id]) - end + @snippet ||= PersonalSnippet.find_by(id: params[:id]) end + alias_method :awardable, :snippet alias_method :spammable, :snippet def authorize_read_snippet! - authenticate_user! unless can?(current_user, :read_personal_snippet, @snippet) + return if can?(current_user, :read_personal_snippet, @snippet) + + if current_user + render_404 + else + authenticate_user! + end end def authorize_update_snippet! diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index ca89ed221c6..ba22b2f9d29 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -128,12 +128,11 @@ class UsersController < ApplicationController end def load_snippets - @snippets = SnippetsFinder.new.execute( + @snippets = SnippetsFinder.new( current_user, - filter: :by_user, - user: user, + author: user, scope: params[:scope] - ).page(params[:page]) + ).execute.page(params[:page]) end def projects_for_current_user diff --git a/app/finders/groups_finder.rb b/app/finders/groups_finder.rb index d932a17883f..f68610e197c 100644 --- a/app/finders/groups_finder.rb +++ b/app/finders/groups_finder.rb @@ -1,13 +1,19 @@ class GroupsFinder < UnionFinder - def execute(current_user = nil) - segments = all_groups(current_user) + def initialize(current_user = nil, params = {}) + @current_user = current_user + @params = params + end - find_union(segments, Group).with_route.order_id_desc + def execute + groups = find_union(all_groups, Group).with_route.order_id_desc + by_parent(groups) end private - def all_groups(current_user) + attr_reader :current_user, :params + + def all_groups groups = [] groups << current_user.authorized_groups if current_user @@ -15,4 +21,10 @@ class GroupsFinder < UnionFinder groups end + + def by_parent(groups) + return groups unless params[:parent] + + groups.where(parent: params[:parent]) + end end diff --git a/app/finders/notes_finder.rb b/app/finders/notes_finder.rb index dc6a8ad1f66..02eb983bf55 100644 --- a/app/finders/notes_finder.rb +++ b/app/finders/notes_finder.rb @@ -67,7 +67,7 @@ class NotesFinder when "merge_request" MergeRequestsFinder.new(@current_user, project_id: @project.id).execute when "snippet", "project_snippet" - SnippetsFinder.new.execute(@current_user, filter: :by_project, project: @project) + SnippetsFinder.new(@current_user, project: @project).execute when "personal_snippet" PersonalSnippet.all else diff --git a/app/finders/snippets_finder.rb b/app/finders/snippets_finder.rb index da6e6e87a6f..c04f61de79c 100644 --- a/app/finders/snippets_finder.rb +++ b/app/finders/snippets_finder.rb @@ -1,66 +1,74 @@ -class SnippetsFinder - def execute(current_user, params = {}) - filter = params[:filter] - user = params.fetch(:user, current_user) - - case filter - when :all then - snippets(current_user).fresh - when :public then - Snippet.are_public.fresh - when :by_user then - by_user(current_user, user, params[:scope]) - when :by_project - by_project(current_user, params[:project], params[:scope]) - end +class SnippetsFinder < UnionFinder + attr_accessor :current_user, :params + + def initialize(current_user, params = {}) + @current_user = current_user + @params = params + end + + def execute + items = init_collection + items = by_project(items) + items = by_author(items) + items = by_visibility(items) + + items.fresh end private - def snippets(current_user) - if current_user - Snippet.public_and_internal - else - # Not authenticated - # - # Return only: - # public snippets - Snippet.are_public - end + def init_collection + items = Snippet.all + + accessible(items) end - def by_user(current_user, user, scope) - snippets = user.snippets.fresh + def accessible(items) + segments = [] + segments << items.public_to_user(current_user) + segments << authorized_to_user(items) if current_user - if current_user - include_private = user == current_user - by_scope(snippets, scope, include_private) - else - snippets.are_public - end + find_union(segments, Snippet) end - def by_project(current_user, project, scope) - snippets = project.snippets.fresh + def authorized_to_user(items) + items.where( + 'author_id = :author_id + OR project_id IN (:project_ids)', + author_id: current_user.id, + project_ids: current_user.authorized_projects.select(:id)) + end - if current_user - include_private = project.team.member?(current_user) || current_user.admin? - by_scope(snippets, scope, include_private) - else - snippets.are_public - end + def by_visibility(items) + visibility = params[:visibility] || visibility_from_scope + + return items unless visibility + + items.where(visibility_level: visibility) + end + + def by_author(items) + return items unless params[:author] + + items.where(author_id: params[:author].id) + end + + def by_project(items) + return items unless params[:project] + + items.where(project_id: params[:project].id) end - def by_scope(snippets, scope = nil, include_private = false) - case scope.to_s + def visibility_from_scope + case params[:scope].to_s when 'are_private' - include_private ? snippets.are_private : Snippet.none + Snippet::PRIVATE when 'are_internal' - snippets.are_internal + Snippet::INTERNAL when 'are_public' - snippets.are_public + Snippet::PUBLIC else - include_private ? snippets : snippets.public_and_internal + nil end end end diff --git a/app/helpers/markup_helper.rb b/app/helpers/markup_helper.rb index b241a14740b..b636233c426 100644 --- a/app/helpers/markup_helper.rb +++ b/app/helpers/markup_helper.rb @@ -116,13 +116,13 @@ module MarkupHelper if gitlab_markdown?(file_name) markdown_unsafe(text, context) elsif asciidoc?(file_name) - asciidoc_unsafe(text) + asciidoc_unsafe(text, context) elsif plain?(file_name) content_tag :pre, class: 'plain-readme' do text end else - other_markup_unsafe(file_name, text) + other_markup_unsafe(file_name, text, context) end rescue RuntimeError simple_format(text) @@ -217,12 +217,12 @@ module MarkupHelper Banzai.render(text, context) end - def asciidoc_unsafe(text) - Gitlab::Asciidoc.render(text) + def asciidoc_unsafe(text, context = {}) + Gitlab::Asciidoc.render(text, context) end - def other_markup_unsafe(file_name, text) - Gitlab::OtherMarkup.render(file_name, text) + def other_markup_unsafe(file_name, text, context = {}) + Gitlab::OtherMarkup.render(file_name, text, context) end def prepare_for_rendering(html, context = {}) diff --git a/app/helpers/submodule_helper.rb b/app/helpers/submodule_helper.rb index a762b320d56..b739554a7a4 100644 --- a/app/helpers/submodule_helper.rb +++ b/app/helpers/submodule_helper.rb @@ -1,28 +1,30 @@ module SubmoduleHelper include Gitlab::ShellAdapter + VALID_SUBMODULE_PROTOCOLS = %w[http https git ssh].freeze + # links to files listing for submodule if submodule is a project on this server def submodule_links(submodule_item, ref = nil, repository = @repository) url = repository.submodule_url_for(ref, submodule_item.path) - return url, nil unless url =~ /([^\/:]+)\/([^\/]+(?:\.git)?)\Z/ - - namespace = $1 - project = $2 - project.chomp!('.git') + if url =~ /([^\/:]+)\/([^\/]+(?:\.git)?)\Z/ + namespace, project = $1, $2 + project.sub!(/\.git\z/, '') - if self_url?(url, namespace, project) - return namespace_project_path(namespace, project), - namespace_project_tree_path(namespace, project, - submodule_item.id) - elsif relative_self_url?(url) - relative_self_links(url, submodule_item.id) - elsif github_dot_com_url?(url) - standard_links('github.com', namespace, project, submodule_item.id) - elsif gitlab_dot_com_url?(url) - standard_links('gitlab.com', namespace, project, submodule_item.id) + if self_url?(url, namespace, project) + [namespace_project_path(namespace, project), + namespace_project_tree_path(namespace, project, submodule_item.id)] + elsif relative_self_url?(url) + relative_self_links(url, submodule_item.id) + elsif github_dot_com_url?(url) + standard_links('github.com', namespace, project, submodule_item.id) + elsif gitlab_dot_com_url?(url) + standard_links('gitlab.com', namespace, project, submodule_item.id) + else + [sanitize_submodule_url(url), nil] + end else - return url, nil + [sanitize_submodule_url(url), nil] end end @@ -73,4 +75,16 @@ module SubmoduleHelper namespace_project_tree_path(namespace, base, commit) ] end + + def sanitize_submodule_url(url) + uri = URI.parse(url) + + if uri.scheme.in?(VALID_SUBMODULE_PROTOCOLS) + uri.to_s + else + nil + end + rescue URI::InvalidURIError + nil + end end diff --git a/app/models/snippet.rb b/app/models/snippet.rb index abfbefdf9a0..882e2fa0594 100644 --- a/app/models/snippet.rb +++ b/app/models/snippet.rb @@ -152,18 +152,5 @@ class Snippet < ActiveRecord::Base where(table[:content].matches(pattern)) end - - def accessible_to(user) - return are_public unless user.present? - return all if user.admin? - - where( - 'visibility_level IN (:visibility_levels) - OR author_id = :author_id - OR project_id IN (:project_ids)', - visibility_levels: [Snippet::PUBLIC, Snippet::INTERNAL], - author_id: user.id, - project_ids: user.authorized_projects.select(:id)) - end end end diff --git a/app/policies/project_snippet_policy.rb b/app/policies/project_snippet_policy.rb index 3a96836917e..cf8ff92617f 100644 --- a/app/policies/project_snippet_policy.rb +++ b/app/policies/project_snippet_policy.rb @@ -13,7 +13,7 @@ class ProjectSnippetPolicy < BasePolicy can! :read_project_snippet end - if @subject.private? && @subject.project.team.member?(@user) + if @subject.project.team.member?(@user) can! :read_project_snippet end end diff --git a/app/services/search/snippet_service.rb b/app/services/search/snippet_service.rb index 4f161beea4d..85da0be6fff 100644 --- a/app/services/search/snippet_service.rb +++ b/app/services/search/snippet_service.rb @@ -7,7 +7,7 @@ module Search end def execute - snippets = Snippet.accessible_to(current_user) + snippets = SnippetsFinder.new(current_user).execute Gitlab::SnippetSearchResults.new(snippets, params[:search]) end diff --git a/app/views/import/base/create.js.haml b/app/views/import/base/create.js.haml index 8e929538351..57e8c3ca1e1 100644 --- a/app/views/import/base/create.js.haml +++ b/app/views/import/base/create.js.haml @@ -10,4 +10,4 @@ - else :plain job = $("tr#repo_#{@repo_id}") - job.find(".import-actions").html("<i class='fa fa-exclamation-circle'></i> Error saving project: #{escape_javascript(@project.errors.full_messages.join(','))}") + job.find(".import-actions").html("<i class='fa fa-exclamation-circle'></i> Error saving project: #{escape_javascript(h(@project.errors.full_messages.join(',')))}") diff --git a/app/views/projects/imports/new.html.haml b/app/views/projects/imports/new.html.haml index 2cd8d03e30e..25a87411cac 100644 --- a/app/views/projects/imports/new.html.haml +++ b/app/views/projects/imports/new.html.haml @@ -10,7 +10,7 @@ .panel-body %pre :preserve - #{sanitize_repo_path(@project, @project.import_error)} + #{h(sanitize_repo_path(@project, @project.import_error))} = form_for @project, url: namespace_project_import_path(@project.namespace, @project), method: :post, html: { class: 'form-horizontal' } do |f| = render "shared/import_form", f: f diff --git a/app/views/projects/wikis/git_access.html.haml b/app/views/projects/wikis/git_access.html.haml index fb0efd85dcd..68862206248 100644 --- a/app/views/projects/wikis/git_access.html.haml +++ b/app/views/projects/wikis/git_access.html.haml @@ -28,7 +28,7 @@ %h3 Clone your wiki %pre.dark :preserve - git clone #{ content_tag(:span, default_url_to_repo(@project_wiki), class: 'clone')} + git clone #{ content_tag(:span, h(default_url_to_repo(@project_wiki)), class: 'clone')} cd #{h @project_wiki.path} %h3 Start Gollum and edit locally diff --git a/changelogs/unreleased/31157-respect-project-features-in-wiki-search.yml b/changelogs/unreleased/31157-respect-project-features-in-wiki-search.yml new file mode 100644 index 00000000000..721bb435a2e --- /dev/null +++ b/changelogs/unreleased/31157-respect-project-features-in-wiki-search.yml @@ -0,0 +1,4 @@ +--- +title: Enforce project features when searching blobs and wikis +merge_request: +author: diff --git a/changelogs/unreleased/branch-name-escape.yml b/changelogs/unreleased/branch-name-escape.yml new file mode 100644 index 00000000000..bf46235fd79 --- /dev/null +++ b/changelogs/unreleased/branch-name-escape.yml @@ -0,0 +1,4 @@ +--- +title: Fixed branches dropdown rendering branch names as HTML +merge_request: +author: diff --git a/changelogs/unreleased/bvl-markup-pipeline.yml b/changelogs/unreleased/bvl-markup-pipeline.yml new file mode 100644 index 00000000000..d73bad03340 --- /dev/null +++ b/changelogs/unreleased/bvl-markup-pipeline.yml @@ -0,0 +1,4 @@ +--- +title: Make Asciidoc & other markup go through pipeline to prevent XSS +merge_request: +author: diff --git a/changelogs/unreleased/bvl-validate-urls-in-markdown-using-uri.yml b/changelogs/unreleased/bvl-validate-urls-in-markdown-using-uri.yml new file mode 100644 index 00000000000..03c4e531d73 --- /dev/null +++ b/changelogs/unreleased/bvl-validate-urls-in-markdown-using-uri.yml @@ -0,0 +1,4 @@ +--- +title: Validate URLs in markdown using URI to detect the host correctly +merge_request: +author: diff --git a/changelogs/unreleased/hamlit-xss-fix.yml b/changelogs/unreleased/hamlit-xss-fix.yml new file mode 100644 index 00000000000..ba4713846e9 --- /dev/null +++ b/changelogs/unreleased/hamlit-xss-fix.yml @@ -0,0 +1,4 @@ +--- +title: Fix for XSS in project import view caused by Hamlit filter usage. +merge_request: +author: diff --git a/changelogs/unreleased/rs-sanitize-submodule-urls.yml b/changelogs/unreleased/rs-sanitize-submodule-urls.yml new file mode 100644 index 00000000000..463b3695687 --- /dev/null +++ b/changelogs/unreleased/rs-sanitize-submodule-urls.yml @@ -0,0 +1,4 @@ +--- +title: Sanitize submodule URLs before linking to them in the file tree view +merge_request: +author: diff --git a/changelogs/unreleased/snippets-finder-visibility.yml b/changelogs/unreleased/snippets-finder-visibility.yml new file mode 100644 index 00000000000..fde2262cc8d --- /dev/null +++ b/changelogs/unreleased/snippets-finder-visibility.yml @@ -0,0 +1,4 @@ +--- +title: Refactor snippets finder & dont return internal snippets for external users +merge_request: +author: diff --git a/changelogs/unreleased/snippets_visibility.yml b/changelogs/unreleased/snippets_visibility.yml new file mode 100644 index 00000000000..4c10c6882ab --- /dev/null +++ b/changelogs/unreleased/snippets_visibility.yml @@ -0,0 +1,4 @@ +--- +title: Fix snippets visibility for show action - external users can not see internal snippets +merge_request: +author: diff --git a/changelogs/unreleased/tc-fix-private-subgroups-shown.yml b/changelogs/unreleased/tc-fix-private-subgroups-shown.yml new file mode 100644 index 00000000000..82e03921854 --- /dev/null +++ b/changelogs/unreleased/tc-fix-private-subgroups-shown.yml @@ -0,0 +1,4 @@ +--- +title: "Do not show private groups on subgroups page if user doesn't have access to" +merge_request: +author: diff --git a/lib/api/groups.rb b/lib/api/groups.rb index 09d105f6b4c..9ccc75681f9 100644 --- a/lib/api/groups.rb +++ b/lib/api/groups.rb @@ -52,7 +52,7 @@ module API elsif current_user.admin Group.all elsif params[:all_available] - GroupsFinder.new.execute(current_user) + GroupsFinder.new(current_user).execute else current_user.groups end diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 86bf567fe69..c643ea8e5a7 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -91,8 +91,8 @@ module API end def find_project_snippet(id) - finder_params = { filter: :by_project, project: user_project } - SnippetsFinder.new.execute(current_user, finder_params).find(id) + finder_params = { project: user_project } + SnippetsFinder.new(current_user, finder_params).execute.find(id) end def find_merge_request_with_access(iid, access_level = :read_merge_request) diff --git a/lib/api/project_snippets.rb b/lib/api/project_snippets.rb index cfee38a9baf..98bc9c28527 100644 --- a/lib/api/project_snippets.rb +++ b/lib/api/project_snippets.rb @@ -17,8 +17,7 @@ module API end def snippets_for_current_user - finder_params = { filter: :by_project, project: user_project } - SnippetsFinder.new.execute(current_user, finder_params) + SnippetsFinder.new(current_user, project: user_project).execute end end diff --git a/lib/api/snippets.rb b/lib/api/snippets.rb index b93fdc62808..53f5953a8fb 100644 --- a/lib/api/snippets.rb +++ b/lib/api/snippets.rb @@ -8,11 +8,11 @@ module API resource :snippets do helpers do def snippets_for_current_user - SnippetsFinder.new.execute(current_user, filter: :by_user, user: current_user) + SnippetsFinder.new(current_user, author: current_user).execute end def public_snippets - SnippetsFinder.new.execute(current_user, filter: :public) + SnippetsFinder.new(current_user, visibility: Snippet::PUBLIC).execute end end diff --git a/lib/api/v3/groups.rb b/lib/api/v3/groups.rb index 63d464b926b..dbf7a3cf785 100644 --- a/lib/api/v3/groups.rb +++ b/lib/api/v3/groups.rb @@ -45,7 +45,7 @@ module API groups = if current_user.admin Group.all elsif params[:all_available] - GroupsFinder.new.execute(current_user) + GroupsFinder.new(current_user).execute else current_user.groups end diff --git a/lib/api/v3/project_snippets.rb b/lib/api/v3/project_snippets.rb index fc065a22d74..c41fee32610 100644 --- a/lib/api/v3/project_snippets.rb +++ b/lib/api/v3/project_snippets.rb @@ -18,8 +18,7 @@ module API end def snippets_for_current_user - finder_params = { filter: :by_project, project: user_project } - SnippetsFinder.new.execute(current_user, finder_params) + SnippetsFinder.new(current_user, project: user_project).execute end end diff --git a/lib/api/v3/snippets.rb b/lib/api/v3/snippets.rb index 07dac7e9904..0762fc02d70 100644 --- a/lib/api/v3/snippets.rb +++ b/lib/api/v3/snippets.rb @@ -8,11 +8,11 @@ module API resource :snippets do helpers do def snippets_for_current_user - SnippetsFinder.new.execute(current_user, filter: :by_user, user: current_user) + SnippetsFinder.new(current_user, author: current_user).execute end def public_snippets - SnippetsFinder.new.execute(current_user, filter: :public) + SnippetsFinder.new(current_user, visibility: Snippet::PUBLIC).execute end end diff --git a/lib/banzai/filter/external_link_filter.rb b/lib/banzai/filter/external_link_filter.rb index d67d466bce8..7d15a0f6d44 100644 --- a/lib/banzai/filter/external_link_filter.rb +++ b/lib/banzai/filter/external_link_filter.rb @@ -2,16 +2,17 @@ module Banzai module Filter # HTML Filter to modify the attributes of external links class ExternalLinkFilter < HTML::Pipeline::Filter + SCHEMES = ['http', 'https', nil].freeze + def call links.each do |node| - href = href_to_lowercase_scheme(node["href"].to_s) + uri = uri(node['href'].to_s) + next unless uri - unless node["href"].to_s == href - node.set_attribute('href', href) - end + node.set_attribute('href', uri.to_s) - if href =~ %r{\A(https?:)?//[^/]} && external_url?(href) - node.set_attribute('rel', 'nofollow noreferrer') + if SCHEMES.include?(uri.scheme) && external_url?(uri) + node.set_attribute('rel', 'nofollow noreferrer noopener') node.set_attribute('target', '_blank') end end @@ -21,27 +22,26 @@ module Banzai private + def uri(href) + URI.parse(href) + rescue URI::InvalidURIError + nil + end + def links query = 'descendant-or-self::a[@href and not(@href = "")]' doc.xpath(query) end - def href_to_lowercase_scheme(href) - scheme_match = href.match(/\A(\w+):\/\//) - - if scheme_match - scheme_match.to_s.downcase + scheme_match.post_match - else - href - end - end + def external_url?(uri) + # Relative URLs miss a hostname + return false unless uri.hostname - def external_url?(url) - !url.start_with?(internal_url) + uri.hostname != internal_url.hostname end def internal_url - @internal_url ||= Gitlab.config.gitlab.url + @internal_url ||= URI.parse(Gitlab.config.gitlab.url) end end end diff --git a/lib/banzai/pipeline/markup_pipeline.rb b/lib/banzai/pipeline/markup_pipeline.rb new file mode 100644 index 00000000000..c56d908009f --- /dev/null +++ b/lib/banzai/pipeline/markup_pipeline.rb @@ -0,0 +1,13 @@ +module Banzai + module Pipeline + class MarkupPipeline < BasePipeline + def self.filters + @filters ||= FilterArray[ + Filter::SanitizationFilter, + Filter::ExternalLinkFilter, + Filter::PlantumlFilter + ] + end + end + end +end diff --git a/lib/gitlab/asciidoc.rb b/lib/gitlab/asciidoc.rb index fba80c7132e..96d38f6daa0 100644 --- a/lib/gitlab/asciidoc.rb +++ b/lib/gitlab/asciidoc.rb @@ -15,17 +15,17 @@ module Gitlab # # input - the source text in Asciidoc format # - def self.render(input) + def self.render(input, context) asciidoc_opts = { safe: :secure, backend: :gitlab_html5, attributes: DEFAULT_ADOC_ATTRS } + context[:pipeline] = :markup + plantuml_setup html = ::Asciidoctor.convert(input, asciidoc_opts) - - filter = Banzai::Filter::SanitizationFilter.new(html) - html = filter.call.to_s + html = Banzai.render(html, context) html.html_safe end diff --git a/lib/gitlab/other_markup.rb b/lib/gitlab/other_markup.rb index c2adc9aa10b..31a24460f0f 100644 --- a/lib/gitlab/other_markup.rb +++ b/lib/gitlab/other_markup.rb @@ -5,12 +5,12 @@ module Gitlab # # input - the source text in a markup format # - def self.render(file_name, input) + def self.render(file_name, input, context) html = GitHub::Markup.render(file_name, input). force_encoding(input.encoding) + context[:pipeline] = :markup - filter = Banzai::Filter::SanitizationFilter.new(html) - html = filter.call.to_s + html = Banzai.render(html, context) html.html_safe end diff --git a/lib/gitlab/project_search_results.rb b/lib/gitlab/project_search_results.rb index 0b8959f2fb9..47cfe412715 100644 --- a/lib/gitlab/project_search_results.rb +++ b/lib/gitlab/project_search_results.rb @@ -82,6 +82,8 @@ module Gitlab private def blobs + return [] unless Ability.allowed?(@current_user, :download_code, @project) + @blobs ||= begin blobs = project.repository.search_files_by_content(query, repository_ref).first(100) found_file_names = Set.new @@ -102,6 +104,8 @@ module Gitlab end def wiki_blobs + return [] unless Ability.allowed?(@current_user, :read_wiki, @project) + @wiki_blobs ||= begin if project.wiki_enabled? && query.present? project_wiki = ProjectWiki.new(project) diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb index 073b87a1cb4..4c8d82a1677 100644 --- a/spec/controllers/groups_controller_spec.rb +++ b/spec/controllers/groups_controller_spec.rb @@ -26,6 +26,41 @@ describe GroupsController do end end + describe 'GET #subgroups' do + let!(:public_subgroup) { create(:group, :public, parent: group) } + let!(:private_subgroup) { create(:group, :private, parent: group) } + + context 'as a user' do + before do + sign_in(user) + end + + it 'shows the public subgroups' do + get :subgroups, id: group.to_param + + expect(assigns(:nested_groups)).to contain_exactly(public_subgroup) + end + + context 'being member' do + it 'shows public and private subgroups the user is member of' do + private_subgroup.add_guest(user) + + get :subgroups, id: group.to_param + + expect(assigns(:nested_groups)).to contain_exactly(public_subgroup, private_subgroup) + end + end + end + + context 'as a guest' do + it 'shows the public subgroups' do + get :subgroups, id: group.to_param + + expect(assigns(:nested_groups)).to contain_exactly(public_subgroup) + end + end + end + describe 'GET #issues' do let(:issue_1) { create(:issue, project: project) } let(:issue_2) { create(:issue, project: project) } diff --git a/spec/controllers/snippets_controller_spec.rb b/spec/controllers/snippets_controller_spec.rb index 41cd5bdcdd8..930415a4778 100644 --- a/spec/controllers/snippets_controller_spec.rb +++ b/spec/controllers/snippets_controller_spec.rb @@ -3,6 +3,34 @@ require 'spec_helper' describe SnippetsController do let(:user) { create(:user) } + describe 'GET #index' do + let(:user) { create(:user) } + + context 'when username parameter is present' do + it 'renders snippets of a user when username is present' do + get :index, username: user.username + + expect(response).to render_template(:index) + end + end + + context 'when username parameter is not present' do + it 'redirects to explore snippets page when user is not logged in' do + get :index + + expect(response).to redirect_to(explore_snippets_path) + end + + it 'redirects to snippets dashboard page when user is logged in' do + sign_in(user) + + get :index + + expect(response).to redirect_to(dashboard_snippets_path) + end + end + end + describe 'GET #new' do context 'when signed in' do before do @@ -132,7 +160,7 @@ describe SnippetsController do it 'responds with status 404' do get :show, id: 'doesntexist' - expect(response).to have_http_status(404) + expect(response).to redirect_to(new_user_session_path) end end end @@ -478,10 +506,10 @@ describe SnippetsController do end context 'when not signed in' do - it 'responds with status 404' do + it 'redirects to the sign in path' do get :raw, id: 'doesntexist' - expect(response).to have_http_status(404) + expect(response).to redirect_to(new_user_session_path) end end end diff --git a/spec/features/dashboard/snippets_spec.rb b/spec/features/dashboard/snippets_spec.rb index 62937688c22..c6ba118220a 100644 --- a/spec/features/dashboard/snippets_spec.rb +++ b/spec/features/dashboard/snippets_spec.rb @@ -12,4 +12,51 @@ describe 'Dashboard snippets', feature: true do it_behaves_like 'paginated snippets' end + + context 'filtering by visibility' do + let(:user) { create(:user) } + let!(:snippets) do + [ + create(:personal_snippet, :public, author: user), + create(:personal_snippet, :internal, author: user), + create(:personal_snippet, :private, author: user), + create(:personal_snippet, :public) + ] + end + + before do + login_as(user) + + visit dashboard_snippets_path + end + + it 'contains all snippets of logged user' do + expect(page).to have_selector('.snippet-row', count: 3) + + expect(page).to have_content(snippets[0].title) + expect(page).to have_content(snippets[1].title) + expect(page).to have_content(snippets[2].title) + end + + it 'contains all private snippets of logged user when clicking on private' do + click_link('Private') + + expect(page).to have_selector('.snippet-row', count: 1) + expect(page).to have_content(snippets[2].title) + end + + it 'contains all internal snippets of logged user when clicking on internal' do + click_link('Internal') + + expect(page).to have_selector('.snippet-row', count: 1) + expect(page).to have_content(snippets[1].title) + end + + it 'contains all public snippets of logged user when clicking on public' do + click_link('Public') + + expect(page).to have_selector('.snippet-row', count: 1) + expect(page).to have_content(snippets[0].title) + end + end end diff --git a/spec/features/projects/snippets_spec.rb b/spec/features/projects/snippets_spec.rb index d37e8ed4699..18689c17fe9 100644 --- a/spec/features/projects/snippets_spec.rb +++ b/spec/features/projects/snippets_spec.rb @@ -4,11 +4,27 @@ describe 'Project snippets', feature: true do context 'when the project has snippets' do let(:project) { create(:empty_project, :public) } let!(:snippets) { create_list(:project_snippet, 2, :public, author: project.owner, project: project) } - before do - allow(Snippet).to receive(:default_per_page).and_return(1) - visit namespace_project_snippets_path(project.namespace, project) + let!(:other_snippet) { create(:project_snippet) } + + context 'pagination' do + before do + allow(Snippet).to receive(:default_per_page).and_return(1) + + visit namespace_project_snippets_path(project.namespace, project) + end + + it_behaves_like 'paginated snippets' end - it_behaves_like 'paginated snippets' + context 'list content' do + it 'contains all project snippets' do + visit namespace_project_snippets_path(project.namespace, project) + + expect(page).to have_selector('.snippet-row', count: 2) + + expect(page).to have_content(snippets[0].title) + expect(page).to have_content(snippets[1].title) + end + end end end diff --git a/spec/features/snippets/explore_spec.rb b/spec/features/snippets/explore_spec.rb index 10a4597e467..fd097fe2e74 100644 --- a/spec/features/snippets/explore_spec.rb +++ b/spec/features/snippets/explore_spec.rb @@ -1,11 +1,11 @@ require 'rails_helper' feature 'Explore Snippets', feature: true do - scenario 'User should see snippets that are not private' do - public_snippet = create(:personal_snippet, :public) - internal_snippet = create(:personal_snippet, :internal) - private_snippet = create(:personal_snippet, :private) + let!(:public_snippet) { create(:personal_snippet, :public) } + let!(:internal_snippet) { create(:personal_snippet, :internal) } + let!(:private_snippet) { create(:personal_snippet, :private) } + scenario 'User should see snippets that are not private' do login_as create(:user) visit explore_snippets_path @@ -13,4 +13,21 @@ feature 'Explore Snippets', feature: true do expect(page).to have_content(internal_snippet.title) expect(page).not_to have_content(private_snippet.title) end + + scenario 'External user should see only public snippets' do + login_as create(:user, :external) + visit explore_snippets_path + + expect(page).to have_content(public_snippet.title) + expect(page).not_to have_content(internal_snippet.title) + expect(page).not_to have_content(private_snippet.title) + end + + scenario 'Not authenticated user should see only public snippets' do + visit explore_snippets_path + + expect(page).to have_content(public_snippet.title) + expect(page).not_to have_content(internal_snippet.title) + expect(page).not_to have_content(private_snippet.title) + end end diff --git a/spec/features/snippets/internal_snippet_spec.rb b/spec/features/snippets/internal_snippet_spec.rb new file mode 100644 index 00000000000..93382f4c359 --- /dev/null +++ b/spec/features/snippets/internal_snippet_spec.rb @@ -0,0 +1,23 @@ +require 'rails_helper' + +feature 'Internal Snippets', feature: true, js: true do + let(:internal_snippet) { create(:personal_snippet, :internal) } + + describe 'normal user' do + before do + login_as :user + end + + scenario 'sees internal snippets' do + visit snippet_path(internal_snippet) + + expect(page).to have_content(internal_snippet.content) + end + + scenario 'sees raw internal snippets' do + visit raw_snippet_path(internal_snippet) + + expect(page).to have_content(internal_snippet.content) + end + end +end diff --git a/spec/features/users/snippets_spec.rb b/spec/features/users/snippets_spec.rb index 1546a06b80c..4efbd672322 100644 --- a/spec/features/users/snippets_spec.rb +++ b/spec/features/users/snippets_spec.rb @@ -3,14 +3,46 @@ require 'spec_helper' describe 'Snippets tab on a user profile', feature: true, js: true do context 'when the user has snippets' do let(:user) { create(:user) } - let!(:snippets) { create_list(:snippet, 2, :public, author: user) } - before do - allow(Snippet).to receive(:default_per_page).and_return(1) - visit user_path(user) - page.within('.user-profile-nav') { click_link 'Snippets' } - wait_for_ajax + + context 'pagination' do + let!(:snippets) { create_list(:snippet, 2, :public, author: user) } + + before do + allow(Snippet).to receive(:default_per_page).and_return(1) + visit user_path(user) + page.within('.user-profile-nav') { click_link 'Snippets' } + wait_for_ajax + end + + it_behaves_like 'paginated snippets', remote: true end - it_behaves_like 'paginated snippets', remote: true + context 'list content' do + let!(:public_snippet) { create(:snippet, :public, author: user) } + let!(:internal_snippet) { create(:snippet, :internal, author: user) } + let!(:private_snippet) { create(:snippet, :private, author: user) } + let!(:other_snippet) { create(:snippet, :public) } + + it 'contains only internal and public snippets of a user when a user is logged in' do + login_as(:user) + visit user_path(user) + page.within('.user-profile-nav') { click_link 'Snippets' } + wait_for_ajax + + expect(page).to have_selector('.snippet-row', count: 2) + + expect(page).to have_content(public_snippet.title) + expect(page).to have_content(internal_snippet.title) + end + + it 'contains only public snippets of a user when a user is not logged in' do + visit user_path(user) + page.within('.user-profile-nav') { click_link 'Snippets' } + wait_for_ajax + + expect(page).to have_selector('.snippet-row', count: 1) + expect(page).to have_content(public_snippet.title) + end + end end end diff --git a/spec/finders/groups_finder_spec.rb b/spec/finders/groups_finder_spec.rb index d5d111e8d15..5b3591550c1 100644 --- a/spec/finders/groups_finder_spec.rb +++ b/spec/finders/groups_finder_spec.rb @@ -3,29 +3,64 @@ require 'spec_helper' describe GroupsFinder do describe '#execute' do let(:user) { create(:user) } - let!(:private_group) { create(:group, :private) } - let!(:internal_group) { create(:group, :internal) } - let!(:public_group) { create(:group, :public) } - let(:finder) { described_class.new } - describe 'execute' do - describe 'without a user' do - subject { finder.execute } + context 'root level groups' do + let!(:private_group) { create(:group, :private) } + let!(:internal_group) { create(:group, :internal) } + let!(:public_group) { create(:group, :public) } + + context 'without a user' do + subject { described_class.new.execute } it { is_expected.to eq([public_group]) } end - describe 'with a user' do - subject { finder.execute(user) } + context 'with a user' do + subject { described_class.new(user).execute } context 'normal user' do - it { is_expected.to eq([public_group, internal_group]) } + it { is_expected.to contain_exactly(public_group, internal_group) } end context 'external user' do let(:user) { create(:user, external: true) } - it { is_expected.to eq([public_group]) } + it { is_expected.to contain_exactly(public_group) } + end + + context 'user is member of the private group' do + before do + private_group.add_guest(user) + end + + it { is_expected.to contain_exactly(public_group, internal_group, private_group) } + end + end + end + + context 'subgroups' do + let!(:parent_group) { create(:group, :public) } + let!(:public_subgroup) { create(:group, :public, parent: parent_group) } + let!(:internal_subgroup) { create(:group, :internal, parent: parent_group) } + let!(:private_subgroup) { create(:group, :private, parent: parent_group) } + + context 'without a user' do + it 'only returns public subgroups' do + expect(described_class.new(nil, parent: parent_group).execute).to contain_exactly(public_subgroup) + end + end + + context 'with a user' do + it 'returns public and internal subgroups' do + expect(described_class.new(user, parent: parent_group).execute).to contain_exactly(public_subgroup, internal_subgroup) + end + + context 'being member' do + it 'returns public subgroups, internal subgroups, and private subgroups user is member of' do + private_subgroup.add_guest(user) + + expect(described_class.new(user, parent: parent_group).execute).to contain_exactly(public_subgroup, internal_subgroup, private_subgroup) + end end end end diff --git a/spec/finders/snippets_finder_spec.rb b/spec/finders/snippets_finder_spec.rb index cb6c80d1bd0..35f1683eef9 100644 --- a/spec/finders/snippets_finder_spec.rb +++ b/spec/finders/snippets_finder_spec.rb @@ -8,79 +8,145 @@ describe SnippetsFinder do let(:project1) { create(:empty_project, :public, group: group) } let(:project2) { create(:empty_project, :private, group: group) } - context ':all filter' do + context 'all snippets visible to a user' do let!(:snippet1) { create(:personal_snippet, :private) } let!(:snippet2) { create(:personal_snippet, :internal) } let!(:snippet3) { create(:personal_snippet, :public) } + let!(:project_snippet1) { create(:project_snippet, :private) } + let!(:project_snippet2) { create(:project_snippet, :internal) } + let!(:project_snippet3) { create(:project_snippet, :public) } it "returns all private and internal snippets" do - snippets = described_class.new.execute(user, filter: :all) - expect(snippets).to include(snippet2, snippet3) - expect(snippets).not_to include(snippet1) + snippets = described_class.new(user, scope: :all).execute + expect(snippets).to include(snippet2, snippet3, project_snippet2, project_snippet3) + expect(snippets).not_to include(snippet1, project_snippet1) end it "returns all public snippets" do - snippets = described_class.new.execute(nil, filter: :all) - expect(snippets).to include(snippet3) - expect(snippets).not_to include(snippet1, snippet2) + snippets = described_class.new(nil, scope: :all).execute + expect(snippets).to include(snippet3, project_snippet3) + expect(snippets).not_to include(snippet1, snippet2, project_snippet1, project_snippet2) + end + + it "returns all public and internal snippets for normal user" do + snippets = described_class.new(user).execute + + expect(snippets).to include(snippet2, snippet3, project_snippet2, project_snippet3) + expect(snippets).not_to include(snippet1, project_snippet1) + end + + it "returns all public snippets for non authorized user" do + snippets = described_class.new(nil).execute + + expect(snippets).to include(snippet3, project_snippet3) + expect(snippets).not_to include(snippet1, snippet2, project_snippet1, project_snippet2) + end + + it "returns all public and authored snippets for external user" do + external_user = create(:user, :external) + authored_snippet = create(:personal_snippet, :internal, author: external_user) + + snippets = described_class.new(external_user).execute + + expect(snippets).to include(snippet3, project_snippet3, authored_snippet) + expect(snippets).not_to include(snippet1, snippet2, project_snippet1, project_snippet2) end end - context ':public filter' do + context 'filter by visibility' do let!(:snippet1) { create(:personal_snippet, :private) } let!(:snippet2) { create(:personal_snippet, :internal) } let!(:snippet3) { create(:personal_snippet, :public) } - it "returns public public snippets" do - snippets = described_class.new.execute(nil, filter: :public) + it "returns public snippets when visibility is PUBLIC" do + snippets = described_class.new(nil, visibility: Snippet::PUBLIC).execute expect(snippets).to include(snippet3) expect(snippets).not_to include(snippet1, snippet2) end end - context ':by_user filter' do + context 'filter by scope' do + let!(:snippet1) { create(:personal_snippet, :private, author: user) } + let!(:snippet2) { create(:personal_snippet, :internal, author: user) } + let!(:snippet3) { create(:personal_snippet, :public, author: user) } + + it "returns all snippets for 'all' scope" do + snippets = described_class.new(user, scope: :all).execute + + expect(snippets).to include(snippet1, snippet2, snippet3) + end + + it "returns all snippets for 'are_private' scope" do + snippets = described_class.new(user, scope: :are_private).execute + + expect(snippets).to include(snippet1) + expect(snippets).not_to include(snippet2, snippet3) + end + + it "returns all snippets for 'are_interna;' scope" do + snippets = described_class.new(user, scope: :are_internal).execute + + expect(snippets).to include(snippet2) + expect(snippets).not_to include(snippet1, snippet3) + end + + it "returns all snippets for 'are_private' scope" do + snippets = described_class.new(user, scope: :are_public).execute + + expect(snippets).to include(snippet3) + expect(snippets).not_to include(snippet1, snippet2) + end + end + + context 'filter by author' do let!(:snippet1) { create(:personal_snippet, :private, author: user) } let!(:snippet2) { create(:personal_snippet, :internal, author: user) } let!(:snippet3) { create(:personal_snippet, :public, author: user) } it "returns all public and internal snippets" do - snippets = described_class.new.execute(user1, filter: :by_user, user: user) + snippets = described_class.new(user1, author: user).execute + expect(snippets).to include(snippet2, snippet3) expect(snippets).not_to include(snippet1) end it "returns internal snippets" do - snippets = described_class.new.execute(user, filter: :by_user, user: user, scope: "are_internal") + snippets = described_class.new(user, author: user, visibility: Snippet::INTERNAL).execute + expect(snippets).to include(snippet2) expect(snippets).not_to include(snippet1, snippet3) end it "returns private snippets" do - snippets = described_class.new.execute(user, filter: :by_user, user: user, scope: "are_private") + snippets = described_class.new(user, author: user, visibility: Snippet::PRIVATE).execute + expect(snippets).to include(snippet1) expect(snippets).not_to include(snippet2, snippet3) end it "returns public snippets" do - snippets = described_class.new.execute(user, filter: :by_user, user: user, scope: "are_public") + snippets = described_class.new(user, author: user, visibility: Snippet::PUBLIC).execute + expect(snippets).to include(snippet3) expect(snippets).not_to include(snippet1, snippet2) end it "returns all snippets" do - snippets = described_class.new.execute(user, filter: :by_user, user: user) + snippets = described_class.new(user, author: user).execute + expect(snippets).to include(snippet1, snippet2, snippet3) end it "returns only public snippets if unauthenticated user" do - snippets = described_class.new.execute(nil, filter: :by_user, user: user) + snippets = described_class.new(nil, author: user).execute + expect(snippets).to include(snippet3) expect(snippets).not_to include(snippet2, snippet1) end end - context 'by_project filter' do + context 'filter by project' do before do @snippet1 = create(:project_snippet, :private, project: project1) @snippet2 = create(:project_snippet, :internal, project: project1) @@ -88,43 +154,52 @@ describe SnippetsFinder do end it "returns public snippets for unauthorized user" do - snippets = described_class.new.execute(nil, filter: :by_project, project: project1) + snippets = described_class.new(nil, project: project1).execute + expect(snippets).to include(@snippet3) expect(snippets).not_to include(@snippet1, @snippet2) end it "returns public and internal snippets for non project members" do - snippets = described_class.new.execute(user, filter: :by_project, project: project1) + snippets = described_class.new(user, project: project1).execute + expect(snippets).to include(@snippet2, @snippet3) expect(snippets).not_to include(@snippet1) end it "returns public snippets for non project members" do - snippets = described_class.new.execute(user, filter: :by_project, project: project1, scope: "are_public") + snippets = described_class.new(user, project: project1, visibility: Snippet::PUBLIC).execute + expect(snippets).to include(@snippet3) expect(snippets).not_to include(@snippet1, @snippet2) end it "returns internal snippets for non project members" do - snippets = described_class.new.execute(user, filter: :by_project, project: project1, scope: "are_internal") + snippets = described_class.new(user, project: project1, visibility: Snippet::INTERNAL).execute + expect(snippets).to include(@snippet2) expect(snippets).not_to include(@snippet1, @snippet3) end it "does not return private snippets for non project members" do - snippets = described_class.new.execute(user, filter: :by_project, project: project1, scope: "are_private") + snippets = described_class.new(user, project: project1, visibility: Snippet::PRIVATE).execute + expect(snippets).not_to include(@snippet1, @snippet2, @snippet3) end it "returns all snippets for project members" do project1.team << [user, :developer] - snippets = described_class.new.execute(user, filter: :by_project, project: project1) + + snippets = described_class.new(user, project: project1).execute + expect(snippets).to include(@snippet1, @snippet2, @snippet3) end it "returns private snippets for project members" do project1.team << [user, :developer] - snippets = described_class.new.execute(user, filter: :by_project, project: project1, scope: "are_private") + + snippets = described_class.new(user, project: project1, visibility: Snippet::PRIVATE).execute + expect(snippets).to include(@snippet1) end end diff --git a/spec/helpers/submodule_helper_spec.rb b/spec/helpers/submodule_helper_spec.rb index 345bc33a67b..9da33792659 100644 --- a/spec/helpers/submodule_helper_spec.rb +++ b/spec/helpers/submodule_helper_spec.rb @@ -109,6 +109,18 @@ describe SubmoduleHelper do end context 'submodule on unsupported' do + it 'sanitizes unsupported protocols' do + stub_url('javascript:alert("XSS");') + + expect(helper.submodule_links(submodule_item)).to eq([nil, nil]) + end + + it 'sanitizes unsupported protocols disguised as a repository URL' do + stub_url('javascript:alert("XSS");foo/bar.git') + + expect(helper.submodule_links(submodule_item)).to eq([nil, nil]) + end + it 'returns original' do stub_url('http://mygitserver.com/gitlab-org/gitlab-ce') expect(submodule_links(submodule_item)).to eq([repo.submodule_url_for, nil]) diff --git a/spec/lib/banzai/filter/external_link_filter_spec.rb b/spec/lib/banzai/filter/external_link_filter_spec.rb index d9e4525cb28..6f6c215be87 100644 --- a/spec/lib/banzai/filter/external_link_filter_spec.rb +++ b/spec/lib/banzai/filter/external_link_filter_spec.rb @@ -1,5 +1,22 @@ require 'spec_helper' +shared_examples 'an external link with rel attribute' do + it 'adds rel="nofollow" to external links' do + expect(doc.at_css('a')).to have_attribute('rel') + expect(doc.at_css('a')['rel']).to include 'nofollow' + end + + it 'adds rel="noreferrer" to external links' do + expect(doc.at_css('a')).to have_attribute('rel') + expect(doc.at_css('a')['rel']).to include 'noreferrer' + end + + it 'adds rel="noopener" to external links' do + expect(doc.at_css('a')).to have_attribute('rel') + expect(doc.at_css('a')['rel']).to include 'noopener' + end +end + describe Banzai::Filter::ExternalLinkFilter, lib: true do include FilterSpecHelper @@ -22,49 +39,51 @@ describe Banzai::Filter::ExternalLinkFilter, lib: true do context 'for root links on document' do let(:doc) { filter %q(<a href="https://google.com/">Google</a>) } - it 'adds rel="nofollow" to external links' do - expect(doc.at_css('a')).to have_attribute('rel') - expect(doc.at_css('a')['rel']).to include 'nofollow' - end - - it 'adds rel="noreferrer" to external links' do - expect(doc.at_css('a')).to have_attribute('rel') - expect(doc.at_css('a')['rel']).to include 'noreferrer' - end + it_behaves_like 'an external link with rel attribute' end context 'for nested links on document' do let(:doc) { filter %q(<p><a href="https://google.com/">Google</a></p>) } - it 'adds rel="nofollow" to external links' do - expect(doc.at_css('a')).to have_attribute('rel') - expect(doc.at_css('a')['rel']).to include 'nofollow' + it_behaves_like 'an external link with rel attribute' + end + + context 'for invalid urls' do + it 'skips 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>) + + expect(doc.to_html).to eq(expected) end + end + + context 'for links with a username' do + context 'with a valid username' do + let(:doc) { filter %q(<a href="https://user@google.com/">Google</a>) } - it 'adds rel="noreferrer" to external links' do - expect(doc.at_css('a')).to have_attribute('rel') - expect(doc.at_css('a')['rel']).to include 'noreferrer' + it_behaves_like 'an external link with rel attribute' + end + + context 'with an impersonated username' do + let(:internal) { Gitlab.config.gitlab.url } + + let(:doc) { filter %Q(<a href="https://#{internal}@example.com" target="_blank">Reverse Tabnabbing</a>) } + + it_behaves_like 'an external link with rel attribute' end end context 'for non-lowercase scheme links' do - let(:doc_with_http) { filter %q(<p><a href="httP://google.com/">Google</a></p>) } - let(:doc_with_https) { filter %q(<p><a href="hTTpS://google.com/">Google</a></p>) } - - it 'adds rel="nofollow" to external links' do - expect(doc_with_http.at_css('a')).to have_attribute('rel') - expect(doc_with_https.at_css('a')).to have_attribute('rel') + context 'with http' do + let(:doc) { filter %q(<p><a href="httP://google.com/">Google</a></p>) } - expect(doc_with_http.at_css('a')['rel']).to include 'nofollow' - expect(doc_with_https.at_css('a')['rel']).to include 'nofollow' + it_behaves_like 'an external link with rel attribute' end - it 'adds rel="noreferrer" to external links' do - expect(doc_with_http.at_css('a')).to have_attribute('rel') - expect(doc_with_https.at_css('a')).to have_attribute('rel') + context 'with https' do + let(:doc) { filter %q(<p><a href="hTTpS://google.com/">Google</a></p>) } - expect(doc_with_http.at_css('a')['rel']).to include 'noreferrer' - expect(doc_with_https.at_css('a')['rel']).to include 'noreferrer' + it_behaves_like 'an external link with rel attribute' end it 'skips internal links' do @@ -84,14 +103,6 @@ describe Banzai::Filter::ExternalLinkFilter, lib: true do context 'for protocol-relative links' do let(:doc) { filter %q(<p><a href="//google.com/">Google</a></p>) } - it 'adds rel="nofollow" to external links' do - expect(doc.at_css('a')).to have_attribute('rel') - expect(doc.at_css('a')['rel']).to include 'nofollow' - end - - it 'adds rel="noreferrer" to external links' do - expect(doc.at_css('a')).to have_attribute('rel') - expect(doc.at_css('a')['rel']).to include 'noreferrer' - end + it_behaves_like 'an external link with rel attribute' end end diff --git a/spec/lib/gitlab/asciidoc_spec.rb b/spec/lib/gitlab/asciidoc_spec.rb index 0f47fb2fbd9..2c7ebb15fd7 100644 --- a/spec/lib/gitlab/asciidoc_spec.rb +++ b/spec/lib/gitlab/asciidoc_spec.rb @@ -22,7 +22,22 @@ module Gitlab expect(Asciidoctor).to receive(:convert) .with(input, expected_asciidoc_opts).and_return(html) - expect(render(input)).to eq(html) + expect(render(input, context)).to eq(html) + end + + context "with asciidoc_opts" do + it "merges the options with default ones" do + expected_asciidoc_opts = { + safe: :secure, + backend: :gitlab_html5, + attributes: described_class::DEFAULT_ADOC_ATTRS + } + + expect(Asciidoctor).to receive(:convert) + .with(input, expected_asciidoc_opts).and_return(html) + + render(input, context) + end end context "XSS" do @@ -33,7 +48,7 @@ module Gitlab }, 'images' => { input: 'image:https://localhost.com/image.png[Alt text" onerror="alert(7)]', - output: "<div>\n<p><span><img src=\"https://localhost.com/image.png\" alt=\"Alt text\"></span></p>\n</div>" + output: "<img src=\"https://localhost.com/image.png\" alt=\"Alt text\">" }, 'pre' => { input: '```mypre"><script>alert(3)</script>', @@ -43,10 +58,18 @@ module Gitlab links.each do |name, data| it "does not convert dangerous #{name} into HTML" do - expect(render(data[:input])).to eq(data[:output]) + expect(render(data[:input], context)).to include(data[:output]) end end end + + context 'external links' do + it 'adds the `rel` attribute to the link' do + output = render('link:https://google.com[Google]', context) + + expect(output).to include('rel="nofollow noreferrer noopener"') + end + end end def render(*args) diff --git a/spec/lib/gitlab/other_markup_spec.rb b/spec/lib/gitlab/other_markup_spec.rb index d6d53e8586c..c0f5fa9dc1f 100644 --- a/spec/lib/gitlab/other_markup_spec.rb +++ b/spec/lib/gitlab/other_markup_spec.rb @@ -13,7 +13,7 @@ describe Gitlab::OtherMarkup, lib: true do } links.each do |name, data| it "does not convert dangerous #{name} into HTML" do - expect(render(data[:file], data[:input])).to eq(data[:output]) + expect(render(data[:file], data[:input], context)).to eq(data[:output]) end end end diff --git a/spec/lib/gitlab/project_search_results_spec.rb b/spec/lib/gitlab/project_search_results_spec.rb index a7c8e7f1f57..6e0b1192706 100644 --- a/spec/lib/gitlab/project_search_results_spec.rb +++ b/spec/lib/gitlab/project_search_results_spec.rb @@ -22,8 +22,37 @@ describe Gitlab::ProjectSearchResults, lib: true do end describe 'blob search' do - let(:project) { create(:project, :repository) } - let(:results) { described_class.new(user, project, 'files').objects('blobs') } + let(:project) { create(:project, :public, :repository) } + + subject(:results) { described_class.new(user, project, 'files').objects('blobs') } + + context 'when repository is disabled' do + let(:project) { create(:project, :public, :repository, :repository_disabled) } + + it 'hides blobs from members' do + project.add_reporter(user) + + is_expected.to be_empty + end + + it 'hides blobs from non-members' do + is_expected.to be_empty + end + end + + context 'when repository is internal' do + let(:project) { create(:project, :public, :repository, :repository_private) } + + it 'finds blobs for members' do + project.add_reporter(user) + + is_expected.not_to be_empty + end + + it 'hides blobs from non-members' do + is_expected.to be_empty + end + end it 'finds by name' do expect(results).to include(["files/images/wm.svg", nil]) @@ -70,6 +99,46 @@ describe Gitlab::ProjectSearchResults, lib: true do end end + describe 'wiki search' do + let(:project) { create(:project, :public) } + let(:wiki) { build(:project_wiki, project: project) } + let!(:wiki_page) { wiki.create_page('Title', 'Content') } + + subject(:results) { described_class.new(user, project, 'Content').objects('wiki_blobs') } + + context 'when wiki is disabled' do + let(:project) { create(:project, :public, :wiki_disabled) } + + it 'hides wiki blobs from members' do + project.add_reporter(user) + + is_expected.to be_empty + end + + it 'hides wiki blobs from non-members' do + is_expected.to be_empty + end + end + + context 'when wiki is internal' do + let(:project) { create(:project, :public, :wiki_private) } + + it 'finds wiki blobs for members' do + project.add_reporter(user) + + is_expected.not_to be_empty + end + + it 'hides wiki blobs from non-members' do + is_expected.to be_empty + end + end + + it 'finds by content' do + expect(results).to include("master:Title.md:1:Content\n") + end + end + it 'does not list issues on private projects' do issue = create(:issue, project: project) @@ -79,7 +148,6 @@ describe Gitlab::ProjectSearchResults, lib: true do end describe 'confidential issues' do - let(:project) { create(:empty_project) } let(:query) { 'issue' } let(:author) { create(:user) } let(:assignee) { create(:user) } @@ -277,6 +345,7 @@ describe Gitlab::ProjectSearchResults, lib: true do context 'by commit hash' do let(:project) { create(:project, :public, :repository) } let(:commit) { project.repository.commit('0b4bc9a') } + commit_hashes = { short: '0b4bc9a', full: '0b4bc9a49b562e85de7cc9e834518ea6828729b9' } commit_hashes.each do |type, commit_hash| diff --git a/spec/models/snippet_spec.rb b/spec/models/snippet_spec.rb index 75b1fc7e216..1e5c96fe593 100644 --- a/spec/models/snippet_spec.rb +++ b/spec/models/snippet_spec.rb @@ -131,46 +131,6 @@ describe Snippet, models: true do end end - describe '.accessible_to' do - let(:author) { create(:author) } - let(:project) { create(:empty_project) } - - let!(:public_snippet) { create(:snippet, :public) } - let!(:internal_snippet) { create(:snippet, :internal) } - let!(:private_snippet) { create(:snippet, :private, author: author) } - - let!(:project_public_snippet) { create(:snippet, :public, project: project) } - let!(:project_internal_snippet) { create(:snippet, :internal, project: project) } - let!(:project_private_snippet) { create(:snippet, :private, project: project) } - - it 'returns only public snippets when user is blank' do - expect(described_class.accessible_to(nil)).to match_array [public_snippet, project_public_snippet] - end - - it 'returns only public, and internal snippets for regular users' do - user = create(:user) - - expect(described_class.accessible_to(user)).to match_array [public_snippet, internal_snippet, project_public_snippet, project_internal_snippet] - end - - it 'returns public, internal snippets and project private snippets for project members' do - member = create(:user) - project.team << [member, :developer] - - expect(described_class.accessible_to(member)).to match_array [public_snippet, internal_snippet, project_public_snippet, project_internal_snippet, project_private_snippet] - end - - it 'returns private snippets where the user is the author' do - expect(described_class.accessible_to(author)).to match_array [public_snippet, internal_snippet, private_snippet, project_public_snippet, project_internal_snippet] - end - - it 'returns all snippets when for admins' do - admin = create(:admin) - - expect(described_class.accessible_to(admin)).to match_array [public_snippet, internal_snippet, private_snippet, project_public_snippet, project_internal_snippet, project_private_snippet] - end - end - describe '#participants' do let(:project) { create(:empty_project, :public) } let(:snippet) { create(:snippet, content: 'foo', project: project) } diff --git a/spec/policies/project_snippet_policy_spec.rb b/spec/policies/project_snippet_policy_spec.rb index d0758af57dd..e1771b636b8 100644 --- a/spec/policies/project_snippet_policy_spec.rb +++ b/spec/policies/project_snippet_policy_spec.rb @@ -1,7 +1,9 @@ require 'spec_helper' describe ProjectSnippetPolicy, models: true do - let(:current_user) { create(:user) } + let(:regular_user) { create(:user) } + let(:external_user) { create(:user, :external) } + let(:project) { create(:empty_project) } let(:author_permissions) do [ @@ -10,13 +12,15 @@ describe ProjectSnippetPolicy, models: true do ] end - subject { described_class.abilities(current_user, project_snippet).to_set } + def abilities(user, snippet_visibility) + snippet = create(:project_snippet, snippet_visibility, project: project) - context 'public snippet' do - let(:project_snippet) { create(:project_snippet, :public) } + described_class.abilities(user, snippet).to_set + end + context 'public snippet' do context 'no user' do - let(:current_user) { nil } + subject { abilities(nil, :public) } it do is_expected.to include(:read_project_snippet) @@ -25,6 +29,17 @@ describe ProjectSnippetPolicy, models: true do end context 'regular user' do + subject { abilities(regular_user, :public) } + + it do + is_expected.to include(:read_project_snippet) + is_expected.not_to include(*author_permissions) + end + end + + context 'external user' do + subject { abilities(external_user, :public) } + it do is_expected.to include(:read_project_snippet) is_expected.not_to include(*author_permissions) @@ -33,10 +48,8 @@ describe ProjectSnippetPolicy, models: true do end context 'internal snippet' do - let(:project_snippet) { create(:project_snippet, :internal) } - context 'no user' do - let(:current_user) { nil } + subject { abilities(nil, :internal) } it do is_expected.not_to include(:read_project_snippet) @@ -45,6 +58,28 @@ describe ProjectSnippetPolicy, models: true do end context 'regular user' do + subject { abilities(regular_user, :internal) } + + it do + is_expected.to include(:read_project_snippet) + is_expected.not_to include(*author_permissions) + end + end + + context 'external user' do + subject { abilities(external_user, :internal) } + + it do + is_expected.not_to include(:read_project_snippet) + is_expected.not_to include(*author_permissions) + end + end + + context 'project team member external user' do + subject { abilities(external_user, :internal) } + + before { project.team << [external_user, :developer] } + it do is_expected.to include(:read_project_snippet) is_expected.not_to include(*author_permissions) @@ -53,10 +88,8 @@ describe ProjectSnippetPolicy, models: true do end context 'private snippet' do - let(:project_snippet) { create(:project_snippet, :private) } - context 'no user' do - let(:current_user) { nil } + subject { abilities(nil, :private) } it do is_expected.not_to include(:read_project_snippet) @@ -65,6 +98,8 @@ describe ProjectSnippetPolicy, models: true do end context 'regular user' do + subject { abilities(regular_user, :private) } + it do is_expected.not_to include(:read_project_snippet) is_expected.not_to include(*author_permissions) @@ -72,7 +107,9 @@ describe ProjectSnippetPolicy, models: true do end context 'snippet author' do - let(:project_snippet) { create(:project_snippet, :private, author: current_user) } + let(:snippet) { create(:project_snippet, :private, author: regular_user) } + + subject { described_class.abilities(regular_user, snippet).to_set } it do is_expected.to include(:read_project_snippet) @@ -80,8 +117,21 @@ describe ProjectSnippetPolicy, models: true do end end - context 'project team member' do - before { project_snippet.project.team << [current_user, :developer] } + context 'project team member normal user' do + subject { abilities(regular_user, :private) } + + before { project.team << [regular_user, :developer] } + + it do + is_expected.to include(:read_project_snippet) + is_expected.not_to include(*author_permissions) + end + end + + context 'project team member external user' do + subject { abilities(external_user, :private) } + + before { project.team << [external_user, :developer] } it do is_expected.to include(:read_project_snippet) @@ -90,7 +140,7 @@ describe ProjectSnippetPolicy, models: true do end context 'admin user' do - let(:current_user) { create(:admin) } + subject { abilities(create(:admin), :private) } it do is_expected.to include(:read_project_snippet) diff --git a/spec/views/projects/imports/new.html.haml_spec.rb b/spec/views/projects/imports/new.html.haml_spec.rb new file mode 100644 index 00000000000..9b293065797 --- /dev/null +++ b/spec/views/projects/imports/new.html.haml_spec.rb @@ -0,0 +1,22 @@ +require "spec_helper" + +describe "projects/imports/new.html.haml" do + let(:user) { create(:user) } + + context 'when import fails' do + let(:project) { create(:project_empty_repo, import_status: :failed, import_error: '<a href="http://googl.com">Foo</a>', import_type: :gitlab_project, import_source: '/var/opt/gitlab/gitlab-rails/shared/tmp/project_exports/uploads/t.tar.gz', import_url: nil) } + + before do + sign_in(user) + project.team << [user, :master] + end + + it "escapes HTML in import errors" do + assign(:project, project) + + render + + expect(rendered).not_to have_link('Foo', href: "http://googl.com") + end + end +end |