Merge branch 'bvl-security-patches' into 'master'

Security patches -> `master`

See merge request !11230

app/controllers/dashboard/snippets_controller.rb

 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

app/controllers/explore/groups_controller.rb

 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])

app/controllers/explore/snippets_controller.rb

 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

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
 

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)

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!

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

app/finders/groups_finder.rb

 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

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

app/finders/snippets_finder.rb

-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

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 = {})

app/helpers/submodule_helper.rb

 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

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

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

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

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(',')))}")

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

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

changelogs/unreleased/31157-respect-project-features-in-wiki-search.yml

+---
+title: Enforce project features when searching blobs and wikis
+merge_request:
+author:

changelogs/unreleased/branch-name-escape.yml

+---
+title: Fixed branches dropdown rendering branch names as HTML
+merge_request:
+author:

changelogs/unreleased/bvl-markup-pipeline.yml

+---
+title: Make Asciidoc & other markup go through pipeline to prevent XSS
+merge_request:
+author:

changelogs/unreleased/bvl-validate-urls-in-markdown-using-uri.yml

+---
+title: Validate URLs in markdown using URI to detect the host correctly
+merge_request:
+author:

changelogs/unreleased/hamlit-xss-fix.yml

+---
+title: Fix for XSS in project import view caused by Hamlit filter usage.
+merge_request:
+author:

changelogs/unreleased/rs-sanitize-submodule-urls.yml

+---
+title: Sanitize submodule URLs before linking to them in the file tree view
+merge_request:
+author:

changelogs/unreleased/snippets-finder-visibility.yml

+---
+title: Refactor snippets finder & dont return internal snippets for external users
+merge_request:
+author:

changelogs/unreleased/snippets_visibility.yml

+---
+title: Fix snippets visibility for show action - external users can not see internal snippets
+merge_request:
+author:

changelogs/unreleased/tc-fix-private-subgroups-shown.yml

+---
+title: "Do not show private groups on subgroups page if user doesn't have access to"
+merge_request:
+author:

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

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)

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
 

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
 

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

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
 

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
 

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

lib/banzai/pipeline/markup_pipeline.rb

+module Banzai
+  module Pipeline
+    class MarkupPipeline < BasePipeline
+      def self.filters
+        @filters ||= FilterArray[
+          Filter::SanitizationFilter,
+          Filter::ExternalLinkFilter,
+          Filter::PlantumlFilter
+        ]
+      end
+    end
+  end
+end

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

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

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)

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) }

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

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

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

spec/features/snippets/explore_spec.rb

 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

spec/features/snippets/internal_snippet_spec.rb

+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

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

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

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

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])

spec/lib/banzai/filter/external_link_filter_spec.rb

 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

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)

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

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|

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) }

spec/policies/project_snippet_policy_spec.rb

 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)

spec/views/projects/imports/new.html.haml_spec.rb

+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