diff options
author | DJ Mountney <david@twkie.net> | 2017-04-05 21:08:30 -0700 |
---|---|---|
committer | DJ Mountney <david@twkie.net> | 2017-04-05 21:08:30 -0700 |
commit | aaa49c2c4e9473726814e3ce183c2e3e4072d64b (patch) | |
tree | 181b0477aac9cb93359fe677102fd864a5e95656 | |
parent | c25cf77dc525c131e71c8d97ac7d315a18b4f370 (diff) | |
parent | d687f6436a14c8b36f1a560ee95222bb2d4fb63f (diff) | |
download | gitlab-ce-aaa49c2c4e9473726814e3ce183c2e3e4072d64b.tar.gz |
Merge remote-tracking branch 'dev/master'
28 files changed, 219 insertions, 43 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index 3e5475a2296..712a4970a41 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/app/controllers/concerns/continue_params.rb b/app/controllers/concerns/continue_params.rb index 0a995c45bdf..eb3a623acdd 100644 --- a/app/controllers/concerns/continue_params.rb +++ b/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 diff --git a/app/controllers/dashboard/todos_controller.rb b/app/controllers/dashboard/todos_controller.rb index 498690e8f11..4d7d45787fc 100644 --- a/app/controllers/dashboard/todos_controller.rb +++ b/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 diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index 3a870ae4241..a50e16fa4ff 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/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? diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 37e3ac05916..a79d801991a 100755 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/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? diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb index bd0c2cd661e..6b9e4267281 100644 --- a/app/helpers/projects_helper.rb +++ b/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 diff --git a/app/services/merge_requests/build_service.rb b/app/services/merge_requests/build_service.rb index fdce542bd9e..d45da5180e1 100644 --- a/app/services/merge_requests/build_service.rb +++ b/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 diff --git a/changelogs/unreleased/29364-private-projects-mr-fix.yml b/changelogs/unreleased/29364-private-projects-mr-fix.yml new file mode 100644 index 00000000000..ab93d6f337b --- /dev/null +++ b/changelogs/unreleased/29364-private-projects-mr-fix.yml @@ -0,0 +1,4 @@ +--- +title: Don’t show source project name when user does not have access +merge_request: +author: diff --git a/changelogs/unreleased/30125-markdown-security.yml b/changelogs/unreleased/30125-markdown-security.yml new file mode 100644 index 00000000000..b766caf7d08 --- /dev/null +++ b/changelogs/unreleased/30125-markdown-security.yml @@ -0,0 +1,4 @@ +--- +title: Remove the class attribute from the whitelist for HTML generated from Markdown. +merge_request: +author: diff --git a/changelogs/unreleased/file-import-export-path-disclosure.yml b/changelogs/unreleased/file-import-export-path-disclosure.yml new file mode 100644 index 00000000000..1a297d07187 --- /dev/null +++ b/changelogs/unreleased/file-import-export-path-disclosure.yml @@ -0,0 +1,5 @@ +--- +title: Fix path disclosure in project import/export +merge_request: +author: + diff --git a/changelogs/unreleased/open-redirect-continue-params.yml b/changelogs/unreleased/open-redirect-continue-params.yml new file mode 100644 index 00000000000..def3bc7d929 --- /dev/null +++ b/changelogs/unreleased/open-redirect-continue-params.yml @@ -0,0 +1,4 @@ +--- +title: Fix for open redirect vulnerability using continue[to] in URL when requesting project import status. +merge_request: +author: diff --git a/changelogs/unreleased/open-redirect-host-field.yml b/changelogs/unreleased/open-redirect-host-field.yml new file mode 100644 index 00000000000..bed4b47cf04 --- /dev/null +++ b/changelogs/unreleased/open-redirect-host-field.yml @@ -0,0 +1,4 @@ +--- +title: Fix for open redirect vulnerabilities in todos, issues, and MR controllers. +merge_request: +author: diff --git a/lib/banzai/filter/markdown_filter.rb b/lib/banzai/filter/markdown_filter.rb index ff580ec68f8..ee73fa91589 100644 --- a/lib/banzai/filter/markdown_filter.rb +++ b/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 diff --git a/lib/banzai/filter/sanitization_filter.rb b/lib/banzai/filter/sanitization_filter.rb index d5f9e252f62..522217deae4 100644 --- a/lib/banzai/filter/sanitization_filter.rb +++ b/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 diff --git a/lib/banzai/filter/syntax_highlight_filter.rb b/lib/banzai/filter/syntax_highlight_filter.rb index 9f09ca90697..7da565043d1 100644 --- a/lib/banzai/filter/syntax_highlight_filter.rb +++ b/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) diff --git a/lib/banzai/pipeline/gfm_pipeline.rb b/lib/banzai/pipeline/gfm_pipeline.rb index fd4a6a107c2..bd4d1aa9ff8 100644 --- a/lib/banzai/pipeline/gfm_pipeline.rb +++ b/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, diff --git a/lib/banzai/renderer/html.rb b/lib/banzai/renderer/html.rb new file mode 100644 index 00000000000..252caa35947 --- /dev/null +++ b/lib/banzai/renderer/html.rb @@ -0,0 +1,13 @@ +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 diff --git a/spec/controllers/dashboard/todos_controller_spec.rb b/spec/controllers/dashboard/todos_controller_spec.rb index 71a4a2c43c7..6075259ea99 100644 --- a/spec/controllers/dashboard/todos_controller_spec.rb +++ b/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 diff --git a/spec/controllers/projects/imports_controller_spec.rb b/spec/controllers/projects/imports_controller_spec.rb index 7c75815f3c4..6724b474179 100644 --- a/spec/controllers/projects/imports_controller_spec.rb +++ b/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 diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index 734966d50b2..d5f1d46bf7f 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/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 diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index 72f41f7209a..99d5583e683 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/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 diff --git a/spec/features/merge_requests/create_new_mr_spec.rb b/spec/features/merge_requests/create_new_mr_spec.rb index f36781167fb..d4fe67c224f 100644 --- a/spec/features/merge_requests/create_new_mr_spec.rb +++ b/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 diff --git a/spec/helpers/events_helper_spec.rb b/spec/helpers/events_helper_spec.rb index 70443d27f33..a7c3c281083 100644 --- a/spec/helpers/events_helper_spec.rb +++ b/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 diff --git a/spec/helpers/projects_helper_spec.rb b/spec/helpers/projects_helper_spec.rb index fc6ad6419ac..44312ada438 100644 --- a/spec/helpers/projects_helper_spec.rb +++ b/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 diff --git a/spec/lib/banzai/filter/markdown_filter_spec.rb b/spec/lib/banzai/filter/markdown_filter_spec.rb new file mode 100644 index 00000000000..897288b8ad5 --- /dev/null +++ b/spec/lib/banzai/filter/markdown_filter_spec.rb @@ -0,0 +1,19 @@ +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 diff --git a/spec/lib/banzai/filter/sanitization_filter_spec.rb b/spec/lib/banzai/filter/sanitization_filter_spec.rb index b4cd5f63a15..fdbc65b5e00 100644 --- a/spec/lib/banzai/filter/sanitization_filter_spec.rb +++ b/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><span class="k">def</span></code></pre>} + exp = %q{<pre><code><span class="k">def</span></code></pre>} expect(filter(act).to_html).to eq exp end diff --git a/spec/lib/banzai/filter/syntax_highlight_filter_spec.rb b/spec/lib/banzai/filter/syntax_highlight_filter_spec.rb index 63fb1bb25c4..f61fc8ceb9e 100644 --- a/spec/lib/banzai/filter/syntax_highlight_filter_spec.rb +++ b/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 diff --git a/spec/services/merge_requests/build_service_spec.rb b/spec/services/merge_requests/build_service_spec.rb index c8bd4d4601a..be9f9ea2dec 100644 --- a/spec/services/merge_requests/build_service_spec.rb +++ b/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 |