From bc10afb600a8079fe250e7c82bf16763a8fed28f Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Thu, 29 Jun 2017 01:22:22 +0000 Subject: Merge branch 'dm-go-get-xss' into 'security-9-3' Fix XSS issue in go-get handling See merge request !2128 --- changelogs/unreleased/dm-go-get-xss.yml | 4 ++++ lib/gitlab/middleware/go.rb | 33 +++++++++++++++++++++++---------- spec/lib/gitlab/middleware/go_spec.rb | 18 +++++++++++++++++- 3 files changed, 44 insertions(+), 11 deletions(-) create mode 100644 changelogs/unreleased/dm-go-get-xss.yml diff --git a/changelogs/unreleased/dm-go-get-xss.yml b/changelogs/unreleased/dm-go-get-xss.yml new file mode 100644 index 00000000000..bf0a7f4bd3d --- /dev/null +++ b/changelogs/unreleased/dm-go-get-xss.yml @@ -0,0 +1,4 @@ +--- +title: Fix XSS issue in go-get handling +merge_request: +author: diff --git a/lib/gitlab/middleware/go.rb b/lib/gitlab/middleware/go.rb index 6023fa1820f..f42168c720e 100644 --- a/lib/gitlab/middleware/go.rb +++ b/lib/gitlab/middleware/go.rb @@ -3,6 +3,10 @@ module Gitlab module Middleware class Go + include ActionView::Helpers::TagHelper + + PROJECT_PATH_REGEX = %r{\A(#{Gitlab::PathRegex.full_namespace_route_regex}/#{Gitlab::PathRegex.project_route_regex})/}.freeze + def initialize(app) @app = app end @@ -10,17 +14,20 @@ module Gitlab def call(env) request = Rack::Request.new(env) - if go_request?(request) - render_go_doc(request) - else - @app.call(env) - end + render_go_doc(request) || @app.call(env) end private def render_go_doc(request) - body = go_body(request) + return unless go_request?(request) + + path = project_path(request) + return unless path + + body = go_body(path) + return unless body + response = Rack::Response.new(body, 200, { 'Content-Type' => 'text/html' }) response.finish end @@ -29,11 +36,13 @@ module Gitlab request["go-get"].to_i == 1 && request.env["PATH_INFO"].present? end - def go_body(request) - project_url = URI.join(Gitlab.config.gitlab.url, project_path(request)) + def go_body(path) + project_url = URI.join(Gitlab.config.gitlab.url, path) import_prefix = strip_url(project_url.to_s) - "\n" + meta_tag = tag :meta, name: 'go-import', content: "#{import_prefix} git #{project_url}.git" + head_tag = content_tag :head, meta_tag + content_tag :html, head_tag end def strip_url(url) @@ -44,6 +53,10 @@ module Gitlab path_info = request.env["PATH_INFO"] path_info.sub!(/^\//, '') + project_path_match = "#{path_info}/".match(PROJECT_PATH_REGEX) + return unless project_path_match + path = project_path_match[1] + # Go subpackages may be in the form of `namespace/project/path1/path2/../pathN`. # In a traditional project with a single namespace, this would denote repo # `namespace/project` with subpath `path1/path2/../pathN`, but with nested @@ -51,7 +64,7 @@ module Gitlab # `path2/../pathN`, for example. # We find all potential project paths out of the path segments - path_segments = path_info.split('/') + path_segments = path.split('/') simple_project_path = path_segments.first(2).join('/') # If the path is at most 2 segments long, it is a simple `namespace/project` path and we're done diff --git a/spec/lib/gitlab/middleware/go_spec.rb b/spec/lib/gitlab/middleware/go_spec.rb index 6af1564da19..cab662819ac 100644 --- a/spec/lib/gitlab/middleware/go_spec.rb +++ b/spec/lib/gitlab/middleware/go_spec.rb @@ -79,12 +79,28 @@ describe Gitlab::Middleware::Go do it_behaves_like 'a nested project' end + context 'with a subpackage that is not a valid project path' do + let(:path) { "#{project.full_path}/---subpackage" } + + it_behaves_like 'a nested project' + end + context 'without subpackages' do let(:path) { project.full_path } it_behaves_like 'a nested project' end end + + context 'with a bogus path' do + let(:path) { "http:;url=http://www.example.com'http-equiv='refresh'x='?go-get=1" } + + it 'skips go-import generation' do + expect(app).to receive(:call).and_return('no-go') + + go + end + end end def go @@ -100,7 +116,7 @@ describe Gitlab::Middleware::Go do def expect_response_with_path(response, path) expect(response[0]).to eq(200) expect(response[1]['Content-Type']).to eq('text/html') - expected_body = "\n" + expected_body = %{} expect(response[2].body).to eq([expected_body]) end end -- cgit v1.2.1 From 941a2d27373a99cb96561e7f41ac6ae8351dbb96 Mon Sep 17 00:00:00 2001 From: Jacob Schatz Date: Wed, 6 Sep 2017 15:59:49 +0000 Subject: Merge branch 'fix-comment-reflection' into 'security-9-5' Fix Live Comment XSS Vulnerability See merge request gitlab/gitlabhq!2183 --- app/assets/javascripts/notes.js | 11 +++++++---- spec/javascripts/notes_spec.js | 14 ++++++++++++++ 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/app/assets/javascripts/notes.js b/app/assets/javascripts/notes.js index a09270d6d24..f5f7bb4653d 100644 --- a/app/assets/javascripts/notes.js +++ b/app/assets/javascripts/notes.js @@ -1272,16 +1272,16 @@ export default class Notes { `
  • - +
    - - - @${currentUsername} + + + ${_.escape(currentUsername)}
    @@ -1295,6 +1295,9 @@ export default class Notes {
    • ` ); + $tempNote.find('.hidden-xs').text(_.escape(currentUserFullname)); + $tempNote.find('.note-headline-light').text(`@${_.escape(currentUsername)}`); + return $tempNote; } diff --git a/spec/javascripts/notes_spec.js b/spec/javascripts/notes_spec.js index 8c5ad8914b0..3e791a31604 100644 --- a/spec/javascripts/notes_spec.js +++ b/spec/javascripts/notes_spec.js @@ -770,6 +770,20 @@ import '~/notes'; expect($tempNote.prop('nodeName')).toEqual('LI'); expect($tempNote.find('.timeline-content').hasClass('discussion')).toBeTruthy(); }); + + it('should return a escaped user name', () => { + const currentUserFullnameXSS = 'Foo '; + const $tempNote = this.notes.createPlaceholderNote({ + formContent: sampleComment, + uniqueId, + isDiscussionNote: false, + currentUsername, + currentUserFullname: currentUserFullnameXSS, + currentUserAvatar, + }); + const $tempNoteHeader = $tempNote.find('.note-header'); + expect($tempNoteHeader.find('.hidden-xs').text().trim()).toEqual('Foo <script>alert("XSS")</script>'); + }); }); describe('createPlaceholderSystemNote', () => { -- cgit v1.2.1 From 4acab552be05e2ee1ccb6ba1997b770dd89c42bd Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Mon, 4 Sep 2017 10:28:30 +0000 Subject: Merge branch 'fix-escape-commit-block' into 'security-9-5' [9.5] Prevent a persistent XSS in the commit author block See merge request gitlab/gitlabhq!2180 --- app/helpers/commits_helper.rb | 6 +++--- changelogs/unreleased/fix-escape-commit-block.yml | 5 +++++ spec/helpers/commits_helper_spec.rb | 22 ++++++++++++++++++++++ 3 files changed, 30 insertions(+), 3 deletions(-) create mode 100644 changelogs/unreleased/fix-escape-commit-block.yml diff --git a/app/helpers/commits_helper.rb b/app/helpers/commits_helper.rb index 9651f9733f9..08fb9db6c0f 100644 --- a/app/helpers/commits_helper.rb +++ b/app/helpers/commits_helper.rb @@ -137,7 +137,7 @@ module CommitsHelper text = if options[:avatar] - %Q{#{person_name}} + content_tag(:span, person_name, class: "commit-#{options[:source]}-name") else person_name end @@ -148,9 +148,9 @@ module CommitsHelper } if user.nil? - mail_to(source_email, text.html_safe, options) + mail_to(source_email, text, options) else - link_to(text.html_safe, user_path(user), options) + link_to(text, user_path(user), options) end end diff --git a/changelogs/unreleased/fix-escape-commit-block.yml b/changelogs/unreleased/fix-escape-commit-block.yml new file mode 100644 index 00000000000..0665c8e5db2 --- /dev/null +++ b/changelogs/unreleased/fix-escape-commit-block.yml @@ -0,0 +1,5 @@ +--- +title: Prevent a persistent XSS in the commit author block +merge_request: +author: +type: security diff --git a/spec/helpers/commits_helper_spec.rb b/spec/helpers/commits_helper_spec.rb index 7179185285c..4b6c7c33e5b 100644 --- a/spec/helpers/commits_helper_spec.rb +++ b/spec/helpers/commits_helper_spec.rb @@ -12,6 +12,17 @@ describe CommitsHelper do expect(helper.commit_author_link(commit)) .not_to include('onmouseover="alert(1)"') end + + it 'escapes the author name' do + user = build_stubbed(:user, name: 'Foo ') + + commit = double(author: user, author_name: '', author_email: '') + + expect(helper.commit_author_link(commit)) + .to include('Foo <script>') + expect(helper.commit_author_link(commit, avatar: true)) + .to include('commit-author-name', 'Foo <script>') + end end describe 'commit_committer_link' do @@ -25,6 +36,17 @@ describe CommitsHelper do expect(helper.commit_committer_link(commit)) .not_to include('onmouseover="alert(1)"') end + + it 'escapes the commiter name' do + user = build_stubbed(:user, name: 'Foo ') + + commit = double(committer: user, committer_name: '', committer_email: '') + + expect(helper.commit_committer_link(commit)) + .to include('Foo <script>') + expect(helper.commit_committer_link(commit, avatar: true)) + .to include('commit-committer-name', 'Foo <script>') + end end describe '#view_on_environment_button' do -- cgit v1.2.1 From 4efd18d7e140bf2b6b95637af630e7294fcf28cc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= Date: Mon, 21 Aug 2017 15:46:45 +0000 Subject: Merge branch '29943-environment-folder' into 'security-9-5' Do not use `location.pathname` when accessing environments folders See merge request !2147 --- .../environments/components/environment.vue | 15 +- .../environments/components/environment_item.vue | 11 +- app/models/environment.rb | 13 +- app/serializers/environment_entity.rb | 4 + app/serializers/environment_serializer.rb | 6 +- changelogs/unreleased/29943-environment-folder.yml | 4 + .../projects/environments/environments_spec.rb | 206 +++++++++++---------- spec/models/environment_spec.rb | 22 +++ spec/serializers/environment_entity_spec.rb | 4 + 9 files changed, 161 insertions(+), 124 deletions(-) create mode 100644 changelogs/unreleased/29943-environment-folder.yml diff --git a/app/assets/javascripts/environments/components/environment.vue b/app/assets/javascripts/environments/components/environment.vue index 91ed8c8467f..f54d573db6e 100644 --- a/app/assets/javascripts/environments/components/environment.vue +++ b/app/assets/javascripts/environments/components/environment.vue @@ -111,11 +111,11 @@ export default { }, methods: { - toggleFolder(folder, folderUrl) { + toggleFolder(folder) { this.store.toggleFolder(folder); if (!folder.isOpen) { - this.fetchChildEnvironments(folder, folderUrl, true); + this.fetchChildEnvironments(folder, true); } }, @@ -143,10 +143,10 @@ export default { .catch(this.errorCallback); }, - fetchChildEnvironments(folder, folderUrl, showLoader = false) { + fetchChildEnvironments(folder, showLoader = false) { this.store.updateEnvironmentProp(folder, 'isLoadingFolderContent', showLoader); - this.service.getFolderContent(folderUrl) + this.service.getFolderContent(folder.folder_path) .then(resp => resp.json()) .then(response => this.store.setfolderContent(folder, response.environments)) .then(() => this.store.updateEnvironmentProp(folder, 'isLoadingFolderContent', false)) @@ -173,12 +173,7 @@ export default { // We need to verify if any folder is open to also update it const openFolders = this.store.getOpenFolders(); if (openFolders.length) { - openFolders.forEach((folder) => { - // TODO - Move this to the backend - const folderUrl = `${window.location.pathname}/folders/${folder.folderName}`; - - return this.fetchChildEnvironments(folder, folderUrl); - }); + openFolders.forEach(folder => this.fetchChildEnvironments(folder)); } }, diff --git a/app/assets/javascripts/environments/components/environment_item.vue b/app/assets/javascripts/environments/components/environment_item.vue index d8b1b2f1b92..6de01fa53d0 100644 --- a/app/assets/javascripts/environments/components/environment_item.vue +++ b/app/assets/javascripts/environments/components/environment_item.vue @@ -410,20 +410,11 @@ export default { this.hasStopAction || this.canRetry; }, - - /** - * Constructs folder URL based on the current location and the folder id. - * - * @return {String} - */ - folderUrl() { - return `${window.location.pathname}/folders/${this.model.folderName}`; - }, }, methods: { onClickFolder() { - eventHub.$emit('toggleFolder', this.model, this.folderUrl); + eventHub.$emit('toggleFolder', this.model); }, }, }; diff --git a/app/models/environment.rb b/app/models/environment.rb index 435eeaf0e2e..9b05f8b1cd5 100644 --- a/app/models/environment.rb +++ b/app/models/environment.rb @@ -82,12 +82,7 @@ class Environment < ActiveRecord::Base def set_environment_type names = name.split('/') - self.environment_type = - if names.many? - names.first - else - nil - end + self.environment_type = names.many? ? names.first : nil end def includes_commit?(commit) @@ -101,7 +96,7 @@ class Environment < ActiveRecord::Base end def update_merge_request_metrics? - (environment_type || name) == "production" + folder_name == "production" end def first_deployment_for(commit) @@ -223,6 +218,10 @@ class Environment < ActiveRecord::Base format: :json) end + def folder_name + self.environment_type || self.name + end + private # Slugifying a name may remove the uniqueness guarantee afforded by it being diff --git a/app/serializers/environment_entity.rb b/app/serializers/environment_entity.rb index dcaccc3007d..ba0ae6ba8a0 100644 --- a/app/serializers/environment_entity.rb +++ b/app/serializers/environment_entity.rb @@ -26,5 +26,9 @@ class EnvironmentEntity < Grape::Entity terminal_project_environment_path(environment.project, environment) end + expose :folder_path do |environment| + folder_project_environments_path(environment.project, environment.folder_name) + end + expose :created_at, :updated_at end diff --git a/app/serializers/environment_serializer.rb b/app/serializers/environment_serializer.rb index d0a60f134da..88842a9aa75 100644 --- a/app/serializers/environment_serializer.rb +++ b/app/serializers/environment_serializer.rb @@ -36,9 +36,9 @@ class EnvironmentSerializer < BaseSerializer private def itemize(resource) - items = resource.order('folder_name ASC') + items = resource.order('folder ASC') .group('COALESCE(environment_type, name)') - .select('COALESCE(environment_type, name) AS folder_name', + .select('COALESCE(environment_type, name) AS folder', 'COUNT(*) AS size', 'MAX(id) AS last_id') # It makes a difference when you call `paginate` method, because @@ -49,7 +49,7 @@ class EnvironmentSerializer < BaseSerializer environments = resource.where(id: items.map(&:last_id)).index_by(&:id) items.map do |item| - Item.new(item.folder_name, item.size, environments[item.last_id]) + Item.new(item.folder, item.size, environments[item.last_id]) end end end diff --git a/changelogs/unreleased/29943-environment-folder.yml b/changelogs/unreleased/29943-environment-folder.yml new file mode 100644 index 00000000000..8434d07d86e --- /dev/null +++ b/changelogs/unreleased/29943-environment-folder.yml @@ -0,0 +1,4 @@ +--- +title: Resolve CSRF token leakage via pathname manipulation on environments page +merge_request: +author: diff --git a/spec/features/projects/environments/environments_spec.rb b/spec/features/projects/environments/environments_spec.rb index 1c59e57c0a4..af7ad365546 100644 --- a/spec/features/projects/environments/environments_spec.rb +++ b/spec/features/projects/environments/environments_spec.rb @@ -10,26 +10,23 @@ feature 'Environments page', :js do sign_in(user) end - given!(:environment) { } - given!(:deployment) { } - given!(:action) { } - - before do - visit_environments(project) - end - describe 'page tabs' do - scenario 'shows "Available" and "Stopped" tab with links' do + it 'shows "Available" and "Stopped" tab with links' do + visit_environments(project) + expect(page).to have_link('Available') expect(page).to have_link('Stopped') end describe 'with one available environment' do - given(:environment) { create(:environment, project: project, state: :available) } + before do + create(:environment, project: project, state: :available) + end describe 'in available tab page' do it 'should show one environment' do - visit project_environments_path(project, scope: 'available') + visit_environments(project, scope: 'available') + expect(page).to have_css('.environments-container') expect(page.all('.environment-name').length).to eq(1) end @@ -37,7 +34,8 @@ feature 'Environments page', :js do describe 'in stopped tab page' do it 'should show no environments' do - visit project_environments_path(project, scope: 'stopped') + visit_environments(project, scope: 'stopped') + expect(page).to have_css('.environments-container') expect(page).to have_content('You don\'t have any environments right now') end @@ -45,11 +43,14 @@ feature 'Environments page', :js do end describe 'with one stopped environment' do - given(:environment) { create(:environment, project: project, state: :stopped) } + before do + create(:environment, project: project, state: :stopped) + end describe 'in available tab page' do it 'should show no environments' do - visit project_environments_path(project, scope: 'available') + visit_environments(project, scope: 'available') + expect(page).to have_css('.environments-container') expect(page).to have_content('You don\'t have any environments right now') end @@ -57,7 +58,8 @@ feature 'Environments page', :js do describe 'in stopped tab page' do it 'should show one environment' do - visit project_environments_path(project, scope: 'stopped') + visit_environments(project, scope: 'stopped') + expect(page).to have_css('.environments-container') expect(page.all('.environment-name').length).to eq(1) end @@ -66,86 +68,84 @@ feature 'Environments page', :js do end context 'without environments' do - scenario 'does show no environments' do - expect(page).to have_content('You don\'t have any environments right now.') + before do + visit_environments(project) end - scenario 'does show 0 as counter for environments in both tabs' do + it 'does not show environments and counters are set to zero' do + expect(page).to have_content('You don\'t have any environments right now.') + expect(page.find('.js-available-environments-count').text).to eq('0') expect(page.find('.js-stopped-environments-count').text).to eq('0') end end - describe 'when showing the environment' do - given(:environment) { create(:environment, project: project) } - - scenario 'does show environment name' do - expect(page).to have_link(environment.name) - end - - scenario 'does show number of available and stopped environments' do - expect(page.find('.js-available-environments-count').text).to eq('1') - expect(page.find('.js-stopped-environments-count').text).to eq('0') + describe 'environments table' do + given!(:environment) do + create(:environment, project: project, state: :available) end - context 'without deployments' do - scenario 'does show no deployments' do - expect(page).to have_content('No deployments yet') + context 'when there are no deployments' do + before do + visit_environments(project) end - context 'for available environment' do - given(:environment) { create(:environment, project: project, state: :available) } + it 'shows environments names and counters' do + expect(page).to have_link(environment.name) - scenario 'does not shows stop button' do - expect(page).not_to have_selector('.stop-env-link') - end + expect(page.find('.js-available-environments-count').text).to eq('1') + expect(page.find('.js-stopped-environments-count').text).to eq('0') end - context 'for stopped environment' do - given(:environment) { create(:environment, project: project, state: :stopped) } + it 'does not show deployments' do + expect(page).to have_content('No deployments yet') + end - scenario 'does not shows stop button' do - expect(page).not_to have_selector('.stop-env-link') - end + it 'does not show stip button when environment is not stoppable' do + expect(page).not_to have_selector('.stop-env-link') end end - context 'with deployments' do + context 'when there are deployments' do given(:project) { create(:project, :repository) } - given(:deployment) do + given!(:deployment) do create(:deployment, environment: environment, sha: project.commit.id) end - scenario 'does show deployment SHA' do - expect(page).to have_link(deployment.short_sha) - end + it 'shows deployment SHA and internal ID' do + visit_environments(project) - scenario 'does show deployment internal id' do + expect(page).to have_link(deployment.short_sha) expect(page).to have_content(deployment.iid) end - context 'with build and manual actions' do - given(:pipeline) { create(:ci_pipeline, project: project) } - given(:build) { create(:ci_build, pipeline: pipeline) } + context 'when builds and manual actions are present' do + given!(:pipeline) { create(:ci_pipeline, project: project) } + given!(:build) { create(:ci_build, pipeline: pipeline) } - given(:action) do + given!(:action) do create(:ci_build, :manual, pipeline: pipeline, name: 'deploy to production') end - given(:deployment) do + given!(:deployment) do create(:deployment, environment: environment, deployable: build, sha: project.commit.id) end - scenario 'does show a play button' do + before do + visit_environments(project) + end + + it 'shows a play button' do find('.js-dropdown-play-icon-container').click + expect(page).to have_content(action.name.humanize) end - scenario 'does allow to play manual action', js: true do + it 'allows to play a manual action', js: true do expect(action).to be_manual find('.js-dropdown-play-icon-container').click @@ -155,19 +155,19 @@ feature 'Environments page', :js do .not_to change { Ci::Pipeline.count } end - scenario 'does show build name and id' do + it 'shows build name and id' do expect(page).to have_link("#{build.name} ##{build.id}") end - scenario 'does not show stop button' do + it 'shows a stop button' do expect(page).not_to have_selector('.stop-env-link') end - scenario 'does not show external link button' do + it 'does not show external link button' do expect(page).not_to have_css('external-url') end - scenario 'does not show terminal button' do + it 'does not show terminal button' do expect(page).not_to have_terminal_button end @@ -176,7 +176,7 @@ feature 'Environments page', :js do given(:build) { create(:ci_build, pipeline: pipeline) } given(:deployment) { create(:deployment, environment: environment, deployable: build) } - scenario 'does show an external link button' do + it 'shows an external link button' do expect(page).to have_link(nil, href: environment.external_url) end end @@ -192,34 +192,34 @@ feature 'Environments page', :js do on_stop: 'close_app') end - scenario 'does show stop button' do + it 'shows a stop button' do expect(page).to have_selector('.stop-env-link') end - context 'for reporter' do + context 'when user is a reporter' do let(:role) { :reporter } - scenario 'does not show stop button' do + it 'does not show stop button' do expect(page).not_to have_selector('.stop-env-link') end end end - context 'with terminal' do + context 'when kubernetes terminal is available' do let(:project) { create(:kubernetes_project, :test_repo) } context 'for project master' do let(:role) { :master } - scenario 'it shows the terminal button' do + it 'shows the terminal button' do expect(page).to have_terminal_button end end - context 'for developer' do + context 'when user is a developer' do let(:role) { :developer } - scenario 'does not show terminal button' do + it 'does not show terminal button' do expect(page).not_to have_terminal_button end end @@ -228,59 +228,77 @@ feature 'Environments page', :js do end end - scenario 'does have a New environment button' do + it 'does have a new environment button' do + visit_environments(project) + expect(page).to have_link('New environment') end - describe 'when creating a new environment' do + describe 'creating a new environment' do before do visit_environments(project) end - context 'when logged as developer' do - before do - within(".top-area") do - click_link 'New environment' - end - end + context 'user is a developer' do + given(:role) { :developer } - context 'for valid name' do - before do - fill_in('Name', with: 'production') - click_on 'Save' - end + scenario 'developer creates a new environment with a valid name' do + within(".top-area") { click_link 'New environment' } + fill_in('Name', with: 'production') + click_on 'Save' - scenario 'does create a new pipeline' do - expect(page).to have_content('production') - end + expect(page).to have_content('production') end - context 'for invalid name' do - before do - fill_in('Name', with: 'name,with,commas') - click_on 'Save' - end + scenario 'developer creates a new environmetn with invalid name' do + within(".top-area") { click_link 'New environment' } + fill_in('Name', with: 'name,with,commas') + click_on 'Save' - scenario 'does show errors' do - expect(page).to have_content('Name can contain only letters') - end + expect(page).to have_content('Name can contain only letters') end end - context 'when logged as reporter' do + context 'user is a reporter' do given(:role) { :reporter } - scenario 'does not have a New environment link' do + scenario 'reporters tries to create a new environment' do expect(page).not_to have_link('New environment') end end end + describe 'environments folders' do + before do + create(:environment, project: project, + name: 'staging/review-1', + state: :available) + create(:environment, project: project, + name: 'staging/review-2', + state: :available) + end + + scenario 'users unfurls an environment folder' do + visit_environments(project) + + expect(page).not_to have_content 'review-1' + expect(page).not_to have_content 'review-2' + expect(page).to have_content 'staging 2' + + within('.folder-row') do + find('.folder-name', text: 'staging').click + end + + expect(page).to have_content 'review-1' + expect(page).to have_content 'review-2' + end + end + def have_terminal_button have_link(nil, href: terminal_project_environment_path(project, environment)) end - def visit_environments(project) - visit project_environments_path(project) + def visit_environments(project, **opts) + visit project_environments_path(project, **opts) end end diff --git a/spec/models/environment_spec.rb b/spec/models/environment_spec.rb index ea8512a5eae..25e5d155894 100644 --- a/spec/models/environment_spec.rb +++ b/spec/models/environment_spec.rb @@ -54,6 +54,28 @@ describe Environment do end end + describe '#folder_name' do + context 'when it is inside a folder' do + subject(:environment) do + create(:environment, name: 'staging/review-1') + end + + it 'returns a top-level folder name' do + expect(environment.folder_name).to eq 'staging' + end + end + + context 'when the environment if a top-level item itself' do + subject(:environment) do + create(:environment, name: 'production') + end + + it 'returns an environment name' do + expect(environment.folder_name).to eq 'production' + end + end + end + describe '#nullify_external_url' do it 'replaces a blank url with nil' do env = build(:environment, external_url: "") diff --git a/spec/serializers/environment_entity_spec.rb b/spec/serializers/environment_entity_spec.rb index 979d9921941..8f32c5639a1 100644 --- a/spec/serializers/environment_entity_spec.rb +++ b/spec/serializers/environment_entity_spec.rb @@ -16,6 +16,10 @@ describe EnvironmentEntity do expect(subject).to include(:id, :name, :state, :environment_path) end + it 'exposes folder path' do + expect(subject).to include(:folder_path) + end + context 'metrics disabled' do before do allow(environment).to receive(:has_metrics?).and_return(false) -- cgit v1.2.1 From 8629d5822a1a7af5708ebb785982b25e0d2400bf Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Thu, 31 Aug 2017 15:27:34 +0000 Subject: Merge branch 'rs-issue-36098' into 'security-9-5' [9.5] Limit `style` attribute on `th` and `td` elements to specific properties See merge request gitlab/gitlabhq!2155 --- changelogs/unreleased/rs-issue-36098.yml | 4 ++++ lib/banzai/filter/sanitization_filter.rb | 22 +++++++++++++++++++++- spec/lib/banzai/filter/sanitization_filter_spec.rb | 20 +++++++++++++++++--- 3 files changed, 42 insertions(+), 4 deletions(-) create mode 100644 changelogs/unreleased/rs-issue-36098.yml diff --git a/changelogs/unreleased/rs-issue-36098.yml b/changelogs/unreleased/rs-issue-36098.yml new file mode 100644 index 00000000000..56b92705a80 --- /dev/null +++ b/changelogs/unreleased/rs-issue-36098.yml @@ -0,0 +1,4 @@ +--- +title: Disallow arbitrary properties in `th` and `td` `style` attributes +merge_request: +author: diff --git a/lib/banzai/filter/sanitization_filter.rb b/lib/banzai/filter/sanitization_filter.rb index 2d6e8ffc90f..768baa4e227 100644 --- a/lib/banzai/filter/sanitization_filter.rb +++ b/lib/banzai/filter/sanitization_filter.rb @@ -5,6 +5,7 @@ module Banzai # Extends HTML::Pipeline::SanitizationFilter with a custom whitelist. class SanitizationFilter < HTML::Pipeline::SanitizationFilter UNSAFE_PROTOCOLS = %w(data javascript vbscript).freeze + TABLE_ALIGNMENT_PATTERN = /text-align: (?center|left|right)/ def whitelist whitelist = super @@ -24,7 +25,8 @@ module Banzai # Only push these customizations once return if customized?(whitelist[:transformers]) - # Allow table alignment + # Allow table alignment; we whitelist specific style properties in a + # transformer below whitelist[:attributes]['th'] = %w(style) whitelist[:attributes]['td'] = %w(style) @@ -52,6 +54,9 @@ module Banzai # Remove `rel` attribute from `a` elements whitelist[:transformers].push(self.class.remove_rel) + # Remove any `style` properties not required for table alignment + whitelist[:transformers].push(self.class.remove_unsafe_table_style) + whitelist end @@ -81,6 +86,21 @@ module Banzai end end end + + def remove_unsafe_table_style + lambda do |env| + node = env[:node] + + return unless node.name == 'th' || node.name == 'td' + return unless node.has_attribute?('style') + + if node['style'] =~ TABLE_ALIGNMENT_PATTERN + node['style'] = "text-align: #{$~[:alignment]}" + else + node.remove_attribute('style') + end + end + end end end end diff --git a/spec/lib/banzai/filter/sanitization_filter_spec.rb b/spec/lib/banzai/filter/sanitization_filter_spec.rb index 35a32a46eff..659b4460fc3 100644 --- a/spec/lib/banzai/filter/sanitization_filter_spec.rb +++ b/spec/lib/banzai/filter/sanitization_filter_spec.rb @@ -49,7 +49,7 @@ describe Banzai::Filter::SanitizationFilter do instance = described_class.new('Foo') 3.times { instance.whitelist } - expect(instance.whitelist[:transformers].size).to eq 4 + expect(instance.whitelist[:transformers].size).to eq 5 end it 'sanitizes `class` attribute from all elements' do @@ -63,8 +63,8 @@ describe Banzai::Filter::SanitizationFilter do expect(filter(act).to_html).to eq %q{def} end - it 'allows `style` attribute on table elements' do - html = <<-HTML.strip_heredoc + it 'allows `text-align` property in `style` attribute on table elements' do + html = <<~HTML @@ -77,6 +77,20 @@ describe Banzai::Filter::SanitizationFilter do expect(doc.at_css('td')['style']).to eq 'text-align: right' end + it 'disallows other properties in `style` attribute on table elements' do + html = <<~HTML +
    Head
    Body
    + + +
    Head
    Body
    + HTML + + doc = filter(html) + + expect(doc.at_css('th')['style']).to be_nil + expect(doc.at_css('td')['style']).to eq 'text-align: center' + end + it 'allows `span` elements' do exp = act = %q{Hello} expect(filter(act).to_html).to eq exp -- cgit v1.2.1 From 9b09856e7b853146ac4ff03d388f7063e6f0efbd Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Wed, 30 Aug 2017 17:34:34 +0000 Subject: Merge branch 'rs-issue-36104' into 'security-9-5' [9.5] Disallow the `name` attribute on all user-provided markup See merge request gitlab/gitlabhq!2166 --- app/views/projects/blob/viewers/_route_map.html.haml | 2 +- app/views/projects/blob/viewers/_route_map_loading.html.haml | 2 +- changelogs/unreleased/rs-issue-36104.yml | 4 ++++ doc/ci/environments.md | 3 +-- doc/install/database_mysql.md | 3 +-- lib/banzai/filter/sanitization_filter.rb | 3 +++ spec/features/copy_as_gfm_spec.rb | 2 -- spec/lib/banzai/filter/sanitization_filter_spec.rb | 12 ++++++++++++ 8 files changed, 23 insertions(+), 8 deletions(-) create mode 100644 changelogs/unreleased/rs-issue-36104.yml diff --git a/app/views/projects/blob/viewers/_route_map.html.haml b/app/views/projects/blob/viewers/_route_map.html.haml index d0fcd55f6c1..6d6bd79bc3c 100644 --- a/app/views/projects/blob/viewers/_route_map.html.haml +++ b/app/views/projects/blob/viewers/_route_map.html.haml @@ -6,4 +6,4 @@ This Route Map is invalid: = viewer.validation_message -= link_to 'Learn more', help_page_path('ci/environments', anchor: 'route-map') += link_to 'Learn more', help_page_path('ci/environments', anchor: 'go-directly-from-source-files-to-public-pages-on-the-environment') diff --git a/app/views/projects/blob/viewers/_route_map_loading.html.haml b/app/views/projects/blob/viewers/_route_map_loading.html.haml index 2318cf82f58..a5f73fb0197 100644 --- a/app/views/projects/blob/viewers/_route_map_loading.html.haml +++ b/app/views/projects/blob/viewers/_route_map_loading.html.haml @@ -1,4 +1,4 @@ = icon('spinner spin fw') Validating Route Map… -= link_to 'Learn more', help_page_path('ci/environments', anchor: 'route-map') += link_to 'Learn more', help_page_path('ci/environments', anchor: 'go-directly-from-source-files-to-public-pages-on-the-environment') diff --git a/changelogs/unreleased/rs-issue-36104.yml b/changelogs/unreleased/rs-issue-36104.yml new file mode 100644 index 00000000000..b050cd83175 --- /dev/null +++ b/changelogs/unreleased/rs-issue-36104.yml @@ -0,0 +1,4 @@ +--- +title: Disallow the `name` attribute on all user-provided markup +merge_request: +author: diff --git a/doc/ci/environments.md b/doc/ci/environments.md index cbf06afa294..c1362b7bd5b 100644 --- a/doc/ci/environments.md +++ b/doc/ci/environments.md @@ -446,8 +446,7 @@ and/or `production`) you can see this information in the merge request itself. ![Environment URLs in merge request](img/environments_link_url_mr.png) -### Go directly from source files to public pages on the environment - +### Go directly from source files to public pages on the environment > Introduced in GitLab 8.17. diff --git a/doc/install/database_mysql.md b/doc/install/database_mysql.md index bc75dc1447e..5c128f54a76 100644 --- a/doc/install/database_mysql.md +++ b/doc/install/database_mysql.md @@ -75,7 +75,7 @@ log_bin_trust_function_creators=1 ### MySQL utf8mb4 support -After installation or upgrade, remember to [convert any new tables](#convert) to `utf8mb4`/`utf8mb4_general_ci`. +After installation or upgrade, remember to [convert any new tables](#tables-and-data-conversion-to-utf8mb4) to `utf8mb4`/`utf8mb4_general_ci`. --- @@ -230,7 +230,6 @@ We need to check, enable and probably convert your existing GitLab DB tables to > Now, ensure that [innodb_file_format](https://dev.mysql.com/doc/refman/5.6/en/tablespace-enabling.html) and [innodb_large_prefix](http://dev.mysql.com/doc/refman/5.7/en/innodb-parameters.html#sysvar_innodb_large_prefix) are **persisted** in your `my.cnf` file. #### Tables and data conversion to utf8mb4 - Now that you have a persistent MySQL setup, you can safely upgrade tables after setup or upgrade time: diff --git a/lib/banzai/filter/sanitization_filter.rb b/lib/banzai/filter/sanitization_filter.rb index 768baa4e227..9923ec4e870 100644 --- a/lib/banzai/filter/sanitization_filter.rb +++ b/lib/banzai/filter/sanitization_filter.rb @@ -45,6 +45,9 @@ module Banzai whitelist[:elements].push('abbr') whitelist[:attributes]['abbr'] = %w(title) + # Disallow `name` attribute globally + whitelist[:attributes][:all].delete('name') + # Allow any protocol in `a` elements... whitelist[:protocols].delete('a') diff --git a/spec/features/copy_as_gfm_spec.rb b/spec/features/copy_as_gfm_spec.rb index 3e6a27eafd8..dfeba722ac6 100644 --- a/spec/features/copy_as_gfm_spec.rb +++ b/spec/features/copy_as_gfm_spec.rb @@ -288,8 +288,6 @@ describe 'Copy as GFM', js: true do 'SanitizationFilter', <<-GFM.strip_heredoc - - sub
    diff --git a/spec/lib/banzai/filter/sanitization_filter_spec.rb b/spec/lib/banzai/filter/sanitization_filter_spec.rb index 659b4460fc3..01ceb21dfaa 100644 --- a/spec/lib/banzai/filter/sanitization_filter_spec.rb +++ b/spec/lib/banzai/filter/sanitization_filter_spec.rb @@ -101,6 +101,18 @@ describe Banzai::Filter::SanitizationFilter do expect(filter(act).to_html).to eq exp end + it 'disallows the `name` attribute globally' do + html = <<~HTML + + Hi + HTML + + doc = filter(html) + + expect(doc.at_css('img')).not_to have_attribute('name') + expect(doc.at_css('span')).not_to have_attribute('name') + end + it 'allows `summary` elements' do exp = act = 'summary line' expect(filter(act).to_html).to eq exp -- cgit v1.2.1 From ba37848d584056069ae83955e53ce51a3ba1a0fe Mon Sep 17 00:00:00 2001 From: Clement Ho Date: Mon, 28 Aug 2017 18:41:12 +0000 Subject: Merge branch 'fix-user-select-dropdown-escaping' into 'security-9-5' Fixes the User Selection Display (9.5) See merge request gitlab/gitlabhq!2177 --- app/assets/javascripts/users_select.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/app/assets/javascripts/users_select.js b/app/assets/javascripts/users_select.js index a31fedee021..73676bd6de7 100644 --- a/app/assets/javascripts/users_select.js +++ b/app/assets/javascripts/users_select.js @@ -75,7 +75,7 @@ function UsersSelect(currentUser, els) { if (currentUserInfo) { input.value = currentUserInfo.id; - input.dataset.meta = currentUserInfo.name; + input.dataset.meta = _.escape(currentUserInfo.name); } else if (_this.currentUser) { input.value = _this.currentUser.id; } @@ -198,7 +198,7 @@ function UsersSelect(currentUser, els) { }; } $value.html(assigneeTemplate(user)); - $collapsedSidebar.attr('title', user.name).tooltip('fixTitle'); + $collapsedSidebar.attr('title', _.escape(user.name)).tooltip('fixTitle'); return $collapsedSidebar.html(collapsedAssigneeTemplate(user)); }); }; @@ -506,7 +506,7 @@ function UsersSelect(currentUser, els) { img = ""; if (user.beforeDivider != null) { - `
  • ${user.name}
    • `; + `
  • ${_.escape(user.name)}
    • `; } else { if (avatar) { img = ""; @@ -518,7 +518,7 @@ function UsersSelect(currentUser, els) { ${img} - ${user.name} + ${_.escape(user.name)} ${username ? `${username}` : ''} @@ -643,11 +643,11 @@ UsersSelect.prototype.formatResult = function(user) { } else { avatar = gon.default_avatar_url; } - return "
    " + user.name + "
    " + (!user.invite ? "@" + _.escape(user.username) : "") + "
    "; + return "
    " + _.escape(user.name) + "
    " + (!user.invite ? "@" + _.escape(user.username) : "") + "
    "; }; UsersSelect.prototype.formatSelection = function(user) { - return user.name; + return _.escape(user.name); }; UsersSelect.prototype.user = function(user_id, callback) { -- cgit v1.2.1 From 4e406a3223214aaa9a8b31186891676b6dc4acea Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Thu, 24 Aug 2017 15:26:47 +0000 Subject: Merge branch 'update-pages-9-5' into 'security-9-5' Update GitLab Pages (9.5) See merge request !2159 --- GITLAB_PAGES_VERSION | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/GITLAB_PAGES_VERSION b/GITLAB_PAGES_VERSION index 8f0916f768f..4b9fcbec101 100644 --- a/GITLAB_PAGES_VERSION +++ b/GITLAB_PAGES_VERSION @@ -1 +1 @@ -0.5.0 +0.5.1 -- cgit v1.2.1 From 13843483e886298efcf468eed14872079c53a9bf Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Thu, 7 Sep 2017 20:23:04 -0400 Subject: Remove changelogs we've already released in the security patches --- changelogs/unreleased/29943-environment-folder.yml | 4 ---- changelogs/unreleased/dm-go-get-xss.yml | 4 ---- changelogs/unreleased/fix-escape-commit-block.yml | 5 ----- changelogs/unreleased/rs-issue-36098.yml | 4 ---- changelogs/unreleased/rs-issue-36104.yml | 4 ---- 5 files changed, 21 deletions(-) delete mode 100644 changelogs/unreleased/29943-environment-folder.yml delete mode 100644 changelogs/unreleased/dm-go-get-xss.yml delete mode 100644 changelogs/unreleased/fix-escape-commit-block.yml delete mode 100644 changelogs/unreleased/rs-issue-36098.yml delete mode 100644 changelogs/unreleased/rs-issue-36104.yml diff --git a/changelogs/unreleased/29943-environment-folder.yml b/changelogs/unreleased/29943-environment-folder.yml deleted file mode 100644 index 8434d07d86e..00000000000 --- a/changelogs/unreleased/29943-environment-folder.yml +++ /dev/null @@ -1,4 +0,0 @@ ---- -title: Resolve CSRF token leakage via pathname manipulation on environments page -merge_request: -author: diff --git a/changelogs/unreleased/dm-go-get-xss.yml b/changelogs/unreleased/dm-go-get-xss.yml deleted file mode 100644 index bf0a7f4bd3d..00000000000 --- a/changelogs/unreleased/dm-go-get-xss.yml +++ /dev/null @@ -1,4 +0,0 @@ ---- -title: Fix XSS issue in go-get handling -merge_request: -author: diff --git a/changelogs/unreleased/fix-escape-commit-block.yml b/changelogs/unreleased/fix-escape-commit-block.yml deleted file mode 100644 index 0665c8e5db2..00000000000 --- a/changelogs/unreleased/fix-escape-commit-block.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Prevent a persistent XSS in the commit author block -merge_request: -author: -type: security diff --git a/changelogs/unreleased/rs-issue-36098.yml b/changelogs/unreleased/rs-issue-36098.yml deleted file mode 100644 index 56b92705a80..00000000000 --- a/changelogs/unreleased/rs-issue-36098.yml +++ /dev/null @@ -1,4 +0,0 @@ ---- -title: Disallow arbitrary properties in `th` and `td` `style` attributes -merge_request: -author: diff --git a/changelogs/unreleased/rs-issue-36104.yml b/changelogs/unreleased/rs-issue-36104.yml deleted file mode 100644 index b050cd83175..00000000000 --- a/changelogs/unreleased/rs-issue-36104.yml +++ /dev/null @@ -1,4 +0,0 @@ ---- -title: Disallow the `name` attribute on all user-provided markup -merge_request: -author: -- cgit v1.2.1