Merge remote-tracking branch 'dev/master'

CHANGELOG.md

@@ -2,6 +2,14 @@
 documentation](doc/development/changelog.md) for instructions on adding your own
 entry.
 
+## 9.0.4 (2017-04-05)
+
+- Don’t show source project name when user does not have access.
+- Remove the class attribute from the whitelist for HTML generated from Markdown.
+- Fix path disclosure in project import/export.
+- Fix for open redirect vulnerability using continue[to] in URL when requesting project import status.
+- Fix for open redirect vulnerabilities in todos, issues, and MR controllers.
+
 ## 9.0.3 (2017-04-05)
 
 - Fix name colision when importing GitHub pull requests from forked repositories. !9719
@@ -320,6 +328,14 @@ entry.
 - Change development tanuki favicon colors to match logo color order.
 - API issues - support filtering by iids.
 
+## 8.17.5 (2017-04-05)
+
+- Don’t show source project name when user does not have access.
+- Remove the class attribute from the whitelist for HTML generated from Markdown.
+- Fix path disclosure in project import/export.
+- Fix for open redirect vulnerability using continue[to] in URL when requesting project import status.
+- Fix for open redirect vulnerabilities in todos, issues, and MR controllers.
+
 ## 8.17.4 (2017-03-19)
 
 - Only show public emails in atom feeds.
@@ -533,6 +549,14 @@ entry.
 - Remove deprecated GitlabCiService.
 - Requeue pending deletion projects.
 
+## 8.16.9 (2017-04-05)
+
+- Don’t show source project name when user does not have access.
+- Remove the class attribute from the whitelist for HTML generated from Markdown.
+- Fix path disclosure in project import/export.
+- Fix for open redirect vulnerability using continue[to] in URL when requesting project import status.
+- Fix for open redirect vulnerabilities in todos, issues, and MR controllers.
+
 ## 8.16.8 (2017-03-19)
 
 - Only show public emails in atom feeds.

app/controllers/concerns/continue_params.rb

@@ -7,6 +7,7 @@ module ContinueParams
 
     continue_params = continue_params.permit(:to, :notice, :notice_now)
     return unless continue_params[:to] && continue_params[:to].start_with?('/')
+    return if continue_params[:to].start_with?('//')
 
     continue_params
   end

app/controllers/dashboard/todos_controller.rb

@@ -7,7 +7,7 @@ class Dashboard::TodosController < Dashboard::ApplicationController
     @sort = params[:sort]
     @todos = @todos.page(params[:page])
     if @todos.out_of_range? && @todos.total_pages != 0
-      redirect_to url_for(params.merge(page: @todos.total_pages))
+      redirect_to url_for(params.merge(page: @todos.total_pages, only_path: true))
     end
   end
 

app/controllers/projects/issues_controller.rb

@@ -31,7 +31,7 @@ class Projects::IssuesController < Projects::ApplicationController
     @issuable_meta_data = issuable_meta_data(@issues, @collection_type)
 
     if @issues.out_of_range? && @issues.total_pages != 0
-      return redirect_to url_for(params.merge(page: @issues.total_pages))
+      return redirect_to url_for(params.merge(page: @issues.total_pages, only_path: true))
     end
 
     if params[:label_name].present?

app/controllers/projects/merge_requests_controller.rb

@@ -43,7 +43,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     @issuable_meta_data = issuable_meta_data(@merge_requests, @collection_type)
 
     if @merge_requests.out_of_range? && @merge_requests.total_pages != 0
-      return redirect_to url_for(params.merge(page: @merge_requests.total_pages))
+      return redirect_to url_for(params.merge(page: @merge_requests.total_pages, only_path: true))
     end
 
     if params[:label_name].present?

app/helpers/projects_helper.rb

@@ -407,7 +407,10 @@ module ProjectsHelper
   def sanitize_repo_path(project, message)
     return '' unless message.present?
 
-    message.strip.gsub(project.repository_storage_path.chomp('/'), "[REPOS PATH]")
+    exports_path = File.join(Settings.shared['path'], 'tmp/project_exports')
+    filtered_message = message.strip.gsub(exports_path, "[REPO EXPORT PATH]")
+
+    filtered_message.gsub(project.repository_storage_path.chomp('/'), "[REPOS PATH]")
   end
 
   def project_feature_options

app/services/merge_requests/build_service.rb

@@ -21,7 +21,9 @@ module MergeRequests
     delegate :target_branch, :source_branch, :source_project, :target_project, :compare_commits, :wip_title, :description, :errors, to: :merge_request
 
     def find_source_project
-      source_project || project
+      return source_project if source_project.present? && can?(current_user, :read_project, source_project)
+
+      project
     end
 
     def find_target_project

changelogs/unreleased/29364-private-projects-mr-fix.yml

+---
+title: Don’t show source project name when user does not have access
+merge_request:
+author:

changelogs/unreleased/30125-markdown-security.yml

+---
+title: Remove the class attribute from the whitelist for HTML generated from Markdown.
+merge_request:
+author:

changelogs/unreleased/file-import-export-path-disclosure.yml

+---
+title: Fix path disclosure in project import/export
+merge_request: 
+author:
+

changelogs/unreleased/open-redirect-continue-params.yml

+---
+title: Fix for open redirect vulnerability using continue[to] in URL when requesting project import status.
+merge_request: 
+author:

changelogs/unreleased/open-redirect-host-field.yml

+---
+title: Fix for open redirect vulnerabilities in todos, issues, and MR controllers.
+merge_request: 
+author:

lib/banzai/filter/markdown_filter.rb

@@ -14,7 +14,7 @@ module Banzai
 
       def self.renderer
         @renderer ||= begin
-          renderer = Redcarpet::Render::HTML.new
+          renderer = Banzai::Renderer::HTML.new
           Redcarpet::Markdown.new(renderer, redcarpet_options)
         end
       end

lib/banzai/filter/sanitization_filter.rb

@@ -24,10 +24,6 @@ module Banzai
         # Only push these customizations once
         return if customized?(whitelist[:transformers])
 
-        # Allow code highlighting
-        whitelist[:attributes]['pre'] = %w(class v-pre)
-        whitelist[:attributes]['span'] = %w(class)
-
         # Allow table alignment
         whitelist[:attributes]['th'] = %w(style)
         whitelist[:attributes]['td'] = %w(style)
@@ -52,9 +48,6 @@ module Banzai
         # Remove `rel` attribute from `a` elements
         whitelist[:transformers].push(self.class.remove_rel)
 
-        # Remove `class` attribute from non-highlight spans
-        whitelist[:transformers].push(self.class.clean_spans)
-
         whitelist
       end
 
@@ -84,21 +77,6 @@ module Banzai
             end
           end
         end
-
-        def clean_spans
-          lambda do |env|
-            node = env[:node]
-
-            return unless node.name == 'span'
-            return unless node.has_attribute?('class')
-
-            unless node.ancestors.any? { |n| n.name.casecmp('pre').zero? }
-              node.remove_attribute('class')
-            end
-
-            { node_whitelist: [node] }
-          end
-        end
       end
     end
   end

lib/banzai/filter/syntax_highlight_filter.rb

@@ -14,7 +14,7 @@ module Banzai
       end
 
       def highlight_node(node)
-        language = node.attr('class')
+        language = node.attr('lang')
         code = node.text
         css_classes = "code highlight"
         lexer = lexer_for(language)

lib/banzai/pipeline/gfm_pipeline.rb

@@ -9,9 +9,9 @@ module Banzai
       # The GFM-to-HTML-to-GFM cycle is tested in spec/features/copy_as_gfm_spec.rb.
       def self.filters
         @filters ||= FilterArray[
-          Filter::SyntaxHighlightFilter,
           Filter::PlantumlFilter,
           Filter::SanitizationFilter,
+          Filter::SyntaxHighlightFilter,
 
           Filter::MathFilter,
           Filter::UploadLinkFilter,

lib/banzai/renderer/html.rb

+module Banzai
+  module Renderer
+    class HTML < Redcarpet::Render::HTML
+      def block_code(code, lang)
+        lang_attr = lang ? %Q{ lang="#{lang}"} : ''
+
+        "\n<pre>" \
+          "<code#{lang_attr}>#{html_escape(code)}</code>" \
+        "</pre>"
+      end
+    end
+  end
+end

spec/controllers/dashboard/todos_controller_spec.rb

@@ -35,6 +35,13 @@ describe Dashboard::TodosController do
         expect(assigns(:todos).current_page).to eq(last_page)
         expect(response).to have_http_status(200)
       end
+
+      it 'does not redirect to external sites when provided a host field' do
+        external_host = "www.example.com"
+        get :index, page: (last_page + 1).to_param, host: external_host
+
+        expect(response).to redirect_to(dashboard_todos_path(page: last_page))
+      end
     end
   end
 

spec/controllers/projects/imports_controller_spec.rb

@@ -96,12 +96,19 @@ describe Projects::ImportsController do
             }
           end
 
-          it 'redirects to params[:to]' do
+          it 'redirects to internal params[:to]' do
             get :show, namespace_id: project.namespace.to_param, project_id: project, continue: params
 
             expect(flash[:notice]).to eq params[:notice]
             expect(response).to redirect_to params[:to]
           end
+
+          it 'does not redirect to external params[:to]' do
+            params[:to] = "//google.com"
+
+            get :show, namespace_id: project.namespace.to_param, project_id: project, continue: params
+            expect(response).not_to redirect_to params[:to]
+          end
         end
       end
 

spec/controllers/projects/issues_controller_spec.rb

@@ -83,6 +83,17 @@ describe Projects::IssuesController do
         expect(assigns(:issues).current_page).to eq(last_page)
         expect(response).to have_http_status(200)
       end
+
+      it 'does not redirect to external sites when provided a host field' do
+        external_host = "www.example.com"
+        get :index,
+          namespace_id: project.namespace.to_param,
+          project_id: project,
+          page: (last_page + 1).to_param,
+          host: external_host
+
+        expect(response).to redirect_to(namespace_project_issues_path(page: last_page, state: controller.params[:state], scope: controller.params[:scope]))
+      end
     end
   end
 

spec/controllers/projects/merge_requests_controller_spec.rb

@@ -176,6 +176,18 @@ describe Projects::MergeRequestsController do
         expect(assigns(:merge_requests).current_page).to eq(last_page)
         expect(response).to have_http_status(200)
       end
+
+      it 'does not redirect to external sites when provided a host field' do
+        external_host = "www.example.com"
+        get :index,
+          namespace_id: project.namespace.to_param,
+          project_id: project,
+          state: 'opened',
+          page: (last_page + 1).to_param,
+          host: external_host
+
+        expect(response).to redirect_to(namespace_project_merge_requests_path(page: last_page, state: controller.params[:state], scope: controller.params[:scope]))
+      end
     end
 
     context 'when filtering by opened state' do

spec/features/merge_requests/create_new_mr_spec.rb

@@ -70,6 +70,18 @@ feature 'Create New Merge Request', feature: true, js: true do
       visit new_namespace_project_merge_request_path(project.namespace, project, merge_request: { target_project_id: private_project.id })
 
       expect(page).not_to have_content private_project.path_with_namespace
+      expect(page).to have_content project.path_with_namespace
+    end
+  end
+
+  context 'when source project cannot be viewed by the current user' do
+    it 'does not leak the private project name & namespace' do
+      private_project = create(:project, :private)
+
+      visit new_namespace_project_merge_request_path(project.namespace, project, merge_request: { source_project_id: private_project.id })
+
+      expect(page).not_to have_content private_project.path_with_namespace
+      expect(page).to have_content project.path_with_namespace
     end
   end
 

spec/helpers/events_helper_spec.rb

@@ -2,8 +2,10 @@ require 'spec_helper'
 
 describe EventsHelper do
   describe '#event_note' do
+    let(:user) { build(:user) }
+
     before do
-      allow(helper).to receive(:current_user).and_return(double)
+      allow(helper).to receive(:current_user).and_return(user)
     end
 
     it 'displays one line of plain text without alteration' do
@@ -60,11 +62,26 @@ describe EventsHelper do
       expect(helper.event_note(input)).to eq(expected)
     end
 
-    it 'preserves style attribute within a tag' do
-      input = '<span class="" style="background-color: #44ad8e; color: #FFFFFF;"></span>'
-      expected = '<p><span style="background-color: #44ad8e; color: #FFFFFF;"></span></p>'
+    context 'labels formatting' do
+      let(:input) { 'this should be ~label_1' }
 
-      expect(helper.event_note(input)).to eq(expected)
+      def format_event_note(project)
+        create(:label, title: 'label_1', project: project)
+
+        helper.event_note(input, { project: project })
+      end
+
+      it 'preserves style attribute for a label that can be accessed by current_user' do
+        project = create(:empty_project, :public)
+
+        expect(format_event_note(project)).to match(/span class=.*style=.*/)
+      end
+
+      it 'does not style a label that can not be accessed by current_user' do
+        project = create(:empty_project, :private)
+
+        expect(format_event_note(project)).to eq("<p>#{input}</p>")
+      end
     end
   end
 

spec/helpers/projects_helper_spec.rb

@@ -167,6 +167,7 @@ describe ProjectsHelper do
 
     before do
       allow(project).to receive(:repository_storage_path).and_return('/base/repo/path')
+      allow(Settings.shared).to receive(:[]).with('path').and_return('/base/repo/export/path')
     end
 
     it 'removes the repo path' do
@@ -175,6 +176,13 @@ describe ProjectsHelper do
 
       expect(sanitize_repo_path(project, import_error)).to eq('Could not clone [REPOS PATH]/namespace/test.git')
     end
+
+    it 'removes the temporary repo path used for uploads/exports' do
+      repo = '/base/repo/export/path/tmp/project_exports/uploads/test.tar.gz'
+      import_error = "Unable to decompress #{repo}\n"
+
+      expect(sanitize_repo_path(project, import_error)).to eq('Unable to decompress [REPO EXPORT PATH]/uploads/test.tar.gz')
+    end
   end
 
   describe '#last_push_event' do

spec/lib/banzai/filter/markdown_filter_spec.rb

+require 'spec_helper'
+
+describe Banzai::Filter::MarkdownFilter, lib: true do
+  include FilterSpecHelper
+
+  context 'code block' do
+    it 'adds language to lang attribute when specified' do
+      result = filter("```html\nsome code\n```")
+
+      expect(result).to start_with("\n<pre><code lang=\"html\">")
+    end
+
+    it 'does not add language to lang attribute when not specified' do
+      result = filter("```\nsome code\n```")
+
+      expect(result).to start_with("\n<pre><code>")
+    end
+  end
+end

spec/lib/banzai/filter/sanitization_filter_spec.rb

@@ -49,11 +49,12 @@ describe Banzai::Filter::SanitizationFilter, lib: true do
       instance = described_class.new('Foo')
       3.times { instance.whitelist }
 
-      expect(instance.whitelist[:transformers].size).to eq 5
+      expect(instance.whitelist[:transformers].size).to eq 4
     end
 
-    it 'allows syntax highlighting' do
-      exp = act = %q{<pre class="code highlight white c"><code><span class="k">def</span></code></pre>}
+    it 'sanitizes `class` attribute from all elements' do
+      act = %q{<pre class="code highlight white c"><code>&lt;span class="k"&gt;def&lt;/span&gt;</code></pre>}
+      exp = %q{<pre><code>&lt;span class="k"&gt;def&lt;/span&gt;</code></pre>}
       expect(filter(act).to_html).to eq exp
     end
 

spec/lib/banzai/filter/syntax_highlight_filter_spec.rb

@@ -12,14 +12,14 @@ describe Banzai::Filter::SyntaxHighlightFilter, lib: true do
 
   context "when a valid language is specified" do
     it "highlights as that language" do
-      result = filter('<pre><code class="ruby">def fun end</code></pre>')
+      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
   end
 
   context "when an invalid language is specified" do
     it "highlights as plaintext" do
-      result = filter('<pre><code class="gnuplot">This is a test</code></pre>')
+      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
   end
@@ -30,7 +30,7 @@ describe Banzai::Filter::SyntaxHighlightFilter, lib: true do
     end
 
     it "highlights as plaintext" do
-      result = filter('<pre><code class="ruby">This is a test</code></pre>')
+      result = filter('<pre><code lang="ruby">This is a test</code></pre>')
       expect(result.to_html).to eq('<pre class="code highlight" lang="" v-pre="true"><code>This is a test</code></pre>')
     end
   end

spec/services/merge_requests/build_service_spec.rb

@@ -4,6 +4,8 @@ describe MergeRequests::BuildService, services: true do
   include RepoHelpers
 
   let(:project) { create(:project, :repository) }
+  let(:source_project) { nil }
+  let(:target_project) { nil }
   let(:user) { create(:user) }
   let(:issue_confidential) { false }
   let(:issue) { create(:issue, project: project, title: 'A bug', confidential: issue_confidential) }
@@ -20,7 +22,9 @@ describe MergeRequests::BuildService, services: true do
     MergeRequests::BuildService.new(project, user,
                                     description: description,
                                     source_branch: source_branch,
-                                    target_branch: target_branch)
+                                    target_branch: target_branch,
+                                    source_project: source_project,
+                                    target_project: target_project)
   end
 
   before do
@@ -256,5 +260,41 @@ describe MergeRequests::BuildService, services: true do
         )
       end
     end
+
+    context 'target_project is set and accessible by current_user' do
+      let(:target_project) { create(:project, :public, :repository)}
+      let(:commits) { Commit.decorate([commit_1], project) }
+
+      it 'sets target project correctly' do
+        expect(merge_request.target_project).to eq(target_project)
+      end
+    end
+
+    context 'target_project is set but not accessible by current_user' do
+      let(:target_project) { create(:project, :private, :repository)}
+      let(:commits) { Commit.decorate([commit_1], project) }
+
+      it 'sets target project correctly' do
+        expect(merge_request.target_project).to eq(project)
+      end
+    end
+
+    context 'source_project is set and accessible by current_user' do
+      let(:source_project) { create(:project, :public, :repository)}
+      let(:commits) { Commit.decorate([commit_1], project) }
+
+      it 'sets target project correctly' do
+        expect(merge_request.source_project).to eq(source_project)
+      end
+    end
+
+    context 'source_project is set but not accessible by current_user' do
+      let(:source_project) { create(:project, :private, :repository)}
+      let(:commits) { Commit.decorate([commit_1], project) }
+
+      it 'sets target project correctly' do
+        expect(merge_request.source_project).to eq(project)
+      end
+    end
   end
 end