Merge branch 'rs-pick-security' into 'master'

Pick 10.4.3 fixes into master

See merge request gitlab-org/gitlab-ce!17040

app/assets/javascripts/render_mermaid.js

@@ -30,6 +30,9 @@ export default function renderMermaid($els) {
     $els.each((i, el) => {
       const source = el.textContent;
+      // Remove any extra spans added by the backend syntax highlighting.
+      Object.assign(el, { textContent: source });
+
       mermaid.init(undefined, el, (id) => {
         const svg = document.getElementById(id);

app/controllers/import/base_controller.rb

@@ -2,26 +2,16 @@ class Import::BaseController < ApplicationController
   private
   def find_or_create_namespace(names, owner)
-    return current_user.namespace if names == owner
-    return current_user.namespace unless current_user.can_create_group?
-
     names = params[:target_namespace].presence || names
-    full_path_namespace = Namespace.find_by_full_path(names)
-    return full_path_namespace if full_path_namespace
-    names.split('/').inject(nil) do |parent, name|
-      begin
-        namespace = Group.create!(name: name,
-                                  path: name,
-                                  owner: current_user,
-                                  parent: parent)
-        namespace.add_owner(current_user)
-        namespace
-      rescue ActiveRecord::RecordNotUnique, ActiveRecord::RecordInvalid
-        Namespace.where(parent: parent).find_by_path_or_name(name)
-      end
-    end
+    return current_user.namespace if names == owner
+
+    group = Groups::NestedCreateService.new(current_user, group_path: names).execute
+    group.errors.any? ? current_user.namespace : group
+  rescue => e
+    Gitlab::AppLogger.error(e)
+    current_user.namespace
   end
 end

app/finders/snippets_finder.rb

+# Snippets Finder
+#
+# Used to filter Snippets collections by a set of params
+#
+# Arguments.
+#
+# current_user - The current user, nil also can be used.
+# params:
+#   visibility (integer) - Individual snippet visibility: Public(20), internal(10) or private(0).
+#   project (Project) - Project related.
+#   author (User) - Author related.
+#
+# params are optional
 class SnippetsFinder < UnionFinder
-  attr_accessor :current_user, :params
+  include Gitlab::Allowable
+  attr_accessor :current_user, :params, :project
   def initialize(current_user, params = {})
     @current_user = current_user
     @params = params
+    @project = params[:project]
   end
   def execute
     items = init_collection
-    items = by_project(items)
     items = by_author(items)
     items = by_visibility(items)
@@ -18,25 +32,42 @@ class SnippetsFinder < UnionFinder
   private
   def init_collection
-    items = Snippet.all
-    accessible(items)
+    if project.present?
+      authorized_snippets_from_project
+    else
+      authorized_snippets
+    end
+  end
+  def authorized_snippets_from_project
+    if can?(current_user, :read_project_snippet, project)
+      if project.team.member?(current_user)
+        project.snippets
+      else
+        project.snippets.public_to_user(current_user)
+      end
+    else
+      Snippet.none
+    end
   end
-  def accessible(items)
-    segments = []
-    segments << items.public_to_user(current_user)
-    segments << authorized_to_user(items)  if current_user
-    find_union(segments, Snippet.includes(:author))
+  def authorized_snippets
+    Snippet.where(feature_available_projects.or(not_project_related)).public_or_visible_to_user(current_user)
+  end
+  def feature_available_projects
+    projects = Project.public_or_visible_to_user(current_user)
+      .with_feature_available_for_user(:snippets, current_user).select(:id)
+    arel_query = Arel::Nodes::SqlLiteral.new(projects.to_sql)
+    table[:project_id].in(arel_query)
   end
-  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))
+  def not_project_related
+    table[:project_id].eq(nil)
+  end
+
+  def table
+    Snippet.arel_table
   end
   def by_visibility(items)
@@ -53,12 +84,6 @@ class SnippetsFinder < UnionFinder
     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 visibility_from_scope
     case params[:scope].to_s
     when 'are_private'

app/models/project.rb

@@ -1589,8 +1589,11 @@ class Project < ActiveRecord::Base
   end
   def protected_for?(ref)
-    ProtectedBranch.protected?(self, ref) ||
+    if repository.branch_exists?(ref)
+      ProtectedBranch.protected?(self, ref)
+    elsif repository.tag_exists?(ref)
       ProtectedTag.protected?(self, ref)
+    end
   end
   def deployment_variables

app/models/snippet.rb

@@ -74,6 +74,27 @@ class Snippet < ActiveRecord::Base
     @link_reference_pattern ||= super("snippets", /(?<snippet>\d+)/)
   end
+  # Returns a collection of snippets that are either public or visible to the
+  # logged in user.
+  #
+  # This method does not verify the user actually has the access to the project
+  # the snippet is in, so it should be only used on a relation that's already scoped
+  # for project access
+  def self.public_or_visible_to_user(user = nil)
+    if user
+      authorized = user
+        .project_authorizations
+        .select(1)
+        .where('project_authorizations.project_id = snippets.project_id')
+
+      levels = Gitlab::VisibilityLevel.levels_for_user(user)
+
+      where('EXISTS (?) OR snippets.visibility_level IN (?) or snippets.author_id = (?)', authorized, levels, user.id)
+    else
+      public_to_user
+    end
+  end
+
   def to_reference(from = nil, full: false)
     reference = "#{self.class.reference_prefix}#{id}"

app/policies/project_policy.rb

@@ -119,7 +119,6 @@ class ProjectPolicy < BasePolicy
     enable :create_note
     enable :upload_file
     enable :read_cycle_analytics
-    enable :read_project_snippet
   end
   rule { can?(:reporter_access) }.policy do

app/services/groups/nested_create_service.rb

@@ -11,8 +11,8 @@ module Groups
     def execute
       return nil unless group_path
-      if group = Group.find_by_full_path(group_path)
-        return group
+      if namespace = namespace_or_group(group_path)
+        return namespace
       end
       if group_path.include?('/') && !Group.supports_nested_groups?
@@ -40,10 +40,14 @@ module Groups
         )
         new_params[:visibility_level] ||= Gitlab::CurrentSettings.current_application_settings.default_group_visibility
-        last_group = Group.find_by_full_path(partial_path) || Groups::CreateService.new(current_user, new_params).execute
+        last_group = namespace_or_group(partial_path) || Groups::CreateService.new(current_user, new_params).execute
       end
       last_group
     end
+
+    def namespace_or_group(group_path)
+      Namespace.find_by_full_path(group_path)
+    end
   end
 end

lib/api/todos.rb

@@ -60,7 +60,7 @@ module API
       end
       post ':id/mark_as_done' do
         TodoService.new.mark_todos_as_done_by_ids(params[:id], current_user)
-        todo = Todo.find(params[:id])
+        todo = current_user.todos.find(params[:id])
         present todo, with: Entities::Todo, current_user: current_user
       end

lib/api/v3/todos.rb

@@ -12,7 +12,7 @@ module API
         end
         delete ':id' do
           TodoService.new.mark_todos_as_done_by_ids(params[:id], current_user)
-          todo = Todo.find(params[:id])
+          todo = current_user.todos.find(params[:id])
           present todo, with: ::API::Entities::Todo, current_user: current_user
         end

lib/banzai/filter/syntax_highlight_filter.rb

@@ -14,23 +14,33 @@ module Banzai
       end
       def highlight_node(node)
-        code = node.text
         css_classes = 'code highlight js-syntax-highlight'
-        language = node.attr('lang')
-        if use_rouge?(language)
-          lexer = lexer_for(language)
+        lang = node.attr('lang')
+        retried = false
+        if use_rouge?(lang)
+          lexer = lexer_for(lang)
           language = lexer.tag
-          begin
-            code = Rouge::Formatters::HTMLGitlab.format(lex(lexer, code), tag: language)
-            css_classes << " #{language}"
-          rescue
-            # Gracefully handle syntax highlighter bugs/errors to ensure
-            # users can still access an issue/comment/etc.
-            language = nil
-          end
+        else
+          lexer = Rouge::Lexers::PlainText.new
+          language = lang
+        end
+
+        begin
+          code = Rouge::Formatters::HTMLGitlab.format(lex(lexer, node.text), tag: language)
+          css_classes << " #{language}" if language
+        rescue
+          # Gracefully handle syntax highlighter bugs/errors to ensure users can
+          # still access an issue/comment/etc. First, retry with the plain text
+          # filter. If that fails, then just skip this entirely, but that would
+          # be a pretty bad upstream bug.
+          return if retried
+          language = nil
+          lexer = Rouge::Lexers::PlainText.new
+          retried = true
+          retry
         end
         highlighted = %(<pre class="#{css_classes}" lang="#{language}" v-pre="true"><code>#{code}</code></pre>)

spec/controllers/import/bitbucket_controller_spec.rb

@@ -222,7 +222,7 @@ describe Import::BitbucketController do
       end
     end
-    context 'user has chosen an existing nested namespace and name for the project' do
+    context 'user has chosen an existing nested namespace and name for the project', :postgresql do
       let(:parent_namespace) { create(:group, name: 'foo', owner: user) }
       let(:nested_namespace) { create(:group, name: 'bar', parent: parent_namespace) }
       let(:test_name) { 'test_name' }
@@ -240,7 +240,7 @@ describe Import::BitbucketController do
       end
     end
-    context 'user has chosen a non-existent nested namespaces and name for the project' do
+    context 'user has chosen a non-existent nested namespaces and name for the project', :postgresql do
       let(:test_name) { 'test_name' }
       it 'takes the selected namespace and name' do
@@ -271,10 +271,14 @@ describe Import::BitbucketController do
       end
     end
-    context 'user has chosen existent and non-existent nested namespaces and name for the project' do
+    context 'user has chosen existent and non-existent nested namespaces and name for the project', :postgresql do
       let(:test_name) { 'test_name' }
       let!(:parent_namespace) { create(:group, name: 'foo', owner: user) }
+      before do
+        parent_namespace.add_owner(user)
+      end
+
       it 'takes the selected namespace and name' do
         expect(Gitlab::BitbucketImport::ProjectCreator)
           .to receive(:new).with(bitbucket_repo, test_name, kind_of(Namespace), user, access_params)

spec/controllers/import/gitlab_controller_spec.rb

@@ -195,7 +195,7 @@ describe Import::GitlabController do
         end
       end
-      context 'user has chosen an existing nested namespace for the project' do
+      context 'user has chosen an existing nested namespace for the project', :postgresql do
         let(:parent_namespace) { create(:group, name: 'foo', owner: user) }
         let(:nested_namespace) { create(:group, name: 'bar', parent: parent_namespace) }
@@ -212,7 +212,7 @@ describe Import::GitlabController do
         end
       end
-      context 'user has chosen a non-existent nested namespaces for the project' do
+      context 'user has chosen a non-existent nested namespaces for the project', :postgresql do
         let(:test_name) { 'test_name' }
         it 'takes the selected namespace and name' do
@@ -243,10 +243,14 @@ describe Import::GitlabController do
         end
       end
-      context 'user has chosen existent and non-existent nested namespaces and name for the project' do
+      context 'user has chosen existent and non-existent nested namespaces and name for the project', :postgresql do
         let(:test_name) { 'test_name' }
         let!(:parent_namespace) { create(:group, name: 'foo', owner: user) }
+        before do
+          parent_namespace.add_owner(user)
+        end
+
         it 'takes the selected namespace and name' do
           expect(Gitlab::GitlabImport::ProjectCreator)
             .to receive(:new).with(gitlab_repo, kind_of(Namespace), user, access_params)

spec/features/markdown/math_spec.rb

+require 'spec_helper'
+
+describe 'Math rendering', :js do
+  it 'renders inline and display math correctly' do
+    description = <<~MATH
+      This math is inline $`a^2+b^2=c^2`$.
+
+      This is on a separate line
+      ```math
+      a^2+b^2=c^2
+      ```
+    MATH
+
+    project = create(:project, :public)
+    issue = create(:issue, project: project, description: description)
+
+    visit project_issue_path(project, issue)
+
+    expect(page).to have_selector('.katex .mord.mathit', text: 'b')
+    expect(page).to have_selector('.katex-display .mord.mathit', text: 'b')
+  end
+end

spec/features/markdown/mermaid_spec.rb

+require 'spec_helper'
+
+describe 'Mermaid rendering', :js do
+  it 'renders Mermaid diagrams correctly' do
+    description = <<~MERMAID
+      ```mermaid
+      graph TD;
+        A-->B;
+        A-->C;
+        B-->D;
+        C-->D;
+      ```
+    MERMAID
+
+    project = create(:project, :public)
+    issue = create(:issue, project: project, description: description)
+
+    visit project_issue_path(project, issue)
+
+    %w[A B C D].each do |label|
+      expect(page).to have_selector('svg foreignObject', text: label)
+    end
+  end
+end

spec/finders/snippets_finder_spec.rb

 require 'spec_helper'
 describe SnippetsFinder do
-  let(:user) { create :user }
-  let(:user1) { create :user }
-  let(:group) { create :group, :public }
-
-  let(:project1) { create(:project, :public,  group: group) }
-  let(:project2) { create(:project, :private, group: group) }
-
-  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(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(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
+  include Gitlab::Allowable
+  using RSpec::Parameterized::TableSyntax
   context 'filter by visibility' do
     let!(:snippet1) { create(:personal_snippet, :private) }
@@ -67,6 +18,7 @@ describe SnippetsFinder do
   end
   context 'filter by scope' do
+    let(:user) { create :user }
     let!(:snippet1) { create(:personal_snippet, :private, author: user) }
     let!(:snippet2) { create(:personal_snippet, :internal, author: user) }
     let!(:snippet3) { create(:personal_snippet, :public, author: user) }
@@ -84,7 +36,7 @@ describe SnippetsFinder do
       expect(snippets).not_to include(snippet2, snippet3)
     end
-    it "returns all snippets for 'are_interna;' scope" do
+    it "returns all snippets for 'are_internal' scope" do
       snippets = described_class.new(user, scope: :are_internal).execute
       expect(snippets).to include(snippet2)
@@ -100,6 +52,8 @@ describe SnippetsFinder do
   end
   context 'filter by author' do
+    let(:user) { create :user }
+    let(:user1) { create :user }
     let!(:snippet1) { create(:personal_snippet, :private, author: user) }
     let!(:snippet2) { create(:personal_snippet, :internal, author: user) }
     let!(:snippet3) { create(:personal_snippet, :public, author: user) }
@@ -147,6 +101,10 @@ describe SnippetsFinder do
   end
   context 'filter by project' do
+    let(:user) { create :user }
+    let(:group) { create :group, :public }
+    let(:project1) { create(:project, :public,  group: group) }
+
     before do
       @snippet1 = create(:project_snippet, :private,  project: project1)
       @snippet2 = create(:project_snippet, :internal, project: project1)
@@ -203,4 +161,9 @@ describe SnippetsFinder do
       expect(snippets).to include(@snippet1)
     end
   end
+
+  describe "#execute" do
+    # Snippet visibility scenarios are included in more details in spec/support/snippet_visibility.rb
+    include_examples 'snippet visibility', described_class
+  end
 end

spec/lib/banzai/filter/syntax_highlight_filter_spec.rb

@@ -3,35 +3,86 @@ require 'spec_helper'
 describe Banzai::Filter::SyntaxHighlightFilter do
   include FilterSpecHelper
+  shared_examples "XSS prevention" do |lang|
+    it "escapes HTML tags" do
+      # This is how a script tag inside a code block is presented to this filter
+      # after Markdown rendering.
+      result = filter(%{<pre lang="#{lang}"><code>&lt;script&gt;alert(1)&lt;/script&gt;</code></pre>})
+
+      expect(result.to_html).not_to include("<script>alert(1)</script>")
+      expect(result.to_html).to include("alert(1)")
+    end
+  end
+
   context "when no language is specified" do
     it "highlights as plaintext" do
       result = filter('<pre><code>def fun end</code></pre>')
+
       expect(result.to_html).to eq('<pre class="code highlight js-syntax-highlight plaintext" lang="plaintext" v-pre="true"><code><span id="LC1" class="line" lang="plaintext">def fun end</span></code></pre>')
     end
+
+    include_examples "XSS prevention", ""
   end
   context "when a valid language is specified" do
     it "highlights as that language" do
       result = filter('<pre><code lang="ruby">def fun end</code></pre>')
+
       expect(result.to_html).to eq('<pre class="code highlight js-syntax-highlight ruby" lang="ruby" v-pre="true"><code><span id="LC1" class="line" lang="ruby"><span class="k">def</span> <span class="nf">fun</span> <span class="k">end</span></span></code></pre>')
     end
+
+    include_examples "XSS prevention", "ruby"
   end
   context "when an invalid language is specified" do
     it "highlights as plaintext" do
       result = filter('<pre><code lang="gnuplot">This is a test</code></pre>')
+
       expect(result.to_html).to eq('<pre class="code highlight js-syntax-highlight plaintext" lang="plaintext" v-pre="true"><code><span id="LC1" class="line" lang="plaintext">This is a test</span></code></pre>')
     end
+
+    include_examples "XSS prevention", "gnuplot"
   end
-  context "when Rouge formatting fails" do
+  context "languages that should be passed through" do
+    %w(math mermaid plantuml).each do |lang|
+      context "when #{lang} is specified" do
+        it "highlights as plaintext but with the correct language attribute and class" do
+          result = filter(%{<pre><code lang="#{lang}">This is a test</code></pre>})
+
+          expect(result.to_html).to eq(%{<pre class="code highlight js-syntax-highlight #{lang}" lang="#{lang}" v-pre="true"><code><span id="LC1" class="line" lang="#{lang}">This is a test</span></code></pre>})
+        end
+
+        include_examples "XSS prevention", lang
+      end
+    end
+  end
+
+  context "when Rouge lexing fails" do
     before do
-      allow_any_instance_of(Rouge::Formatter).to receive(:format).and_raise(StandardError)
+      allow_any_instance_of(Rouge::Lexers::Ruby).to receive(:stream_tokens).and_raise(StandardError)
     end
     it "highlights as plaintext" do
       result = filter('<pre><code lang="ruby">This is a test</code></pre>')
-      expect(result.to_html).to eq('<pre class="code highlight js-syntax-highlight" lang="" v-pre="true"><code>This is a test</code></pre>')
+
+      expect(result.to_html).to eq('<pre class="code highlight js-syntax-highlight" lang="" v-pre="true"><code><span id="LC1" class="line" lang="">This is a test</span></code></pre>')
+    end
+
+    include_examples "XSS prevention", "ruby"
+  end
+
+  context "when Rouge lexing fails after a retry" do
+    before do
+      allow_any_instance_of(Rouge::Lexers::PlainText).to receive(:stream_tokens).and_raise(StandardError)
+    end
+
+    it "does not add highlighting classes" do
+      result = filter('<pre><code>This is a test</code></pre>')
+
+      expect(result.to_html).to eq('<pre><code>This is a test</code></pre>')
     end
+
+    include_examples "XSS prevention", "ruby"
   end
 end

spec/models/ci/build_spec.rb

@@ -1590,7 +1590,7 @@ describe Ci::Build do
       context 'when the branch is protected' do
         before do
-          create(:protected_branch, project: build.project, name: build.ref)
+          allow(build.project).to receive(:protected_for?).with(build.ref).and_return(true)
         end
         it { is_expected.to include(protected_variable) }
@@ -1598,7 +1598,7 @@ describe Ci::Build do
       context 'when the tag is protected' do
         before do
-          create(:protected_tag, project: build.project, name: build.ref)
+          allow(build.project).to receive(:protected_for?).with(build.ref).and_return(true)
         end
         it { is_expected.to include(protected_variable) }
@@ -1635,7 +1635,7 @@ describe Ci::Build do
       context 'when the branch is protected' do
         before do
-          create(:protected_branch, project: build.project, name: build.ref)
+          allow(build.project).to receive(:protected_for?).with(build.ref).and_return(true)
         end
         it { is_expected.to include(protected_variable) }
@@ -1643,7 +1643,7 @@ describe Ci::Build do
       context 'when the tag is protected' do
         before do
-          create(:protected_tag, project: build.project, name: build.ref)
+          allow(build.project).to receive(:protected_for?).with(build.ref).and_return(true)
         end
         it { is_expected.to include(protected_variable) }