diff options
author | Douwe Maan <douwe@gitlab.com> | 2017-09-08 06:45:20 +0000 |
---|---|---|
committer | Douwe Maan <douwe@gitlab.com> | 2017-09-08 06:45:20 +0000 |
commit | bce1c50928e4885d54dd11221a9c8197a7fb1a7d (patch) | |
tree | b853820486a3ee7234b93bfd27b85f98b8a20dc5 | |
parent | dc46863cda29f19a5f403ebaaded92ba17faee2f (diff) | |
parent | 13843483e886298efcf468eed14872079c53a9bf (diff) | |
download | gitlab-ce-bce1c50928e4885d54dd11221a9c8197a7fb1a7d.tar.gz |
Merge branch 'rs-pick-security-into-master' into 'master'
Update master with 9.5.4 security patches
See merge request !14131
23 files changed, 307 insertions, 161 deletions
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 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/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 { `<li id="${uniqueId}" class="note being-posted fade-in-half timeline-entry"> <div class="timeline-entry-inner"> <div class="timeline-icon"> - <a href="/${currentUsername}"> + <a href="/${_.escape(currentUsername)}"> <img class="avatar s40" src="${currentUserAvatar}" /> </a> </div> <div class="timeline-content ${discussionClass}"> <div class="note-header"> <div class="note-header-info"> - <a href="/${currentUsername}"> - <span class="hidden-xs">${currentUserFullname}</span> - <span class="note-headline-light">@${currentUsername}</span> + <a href="/${_.escape(currentUsername)}"> + <span class="hidden-xs">${_.escape(currentUsername)}</span> + <span class="note-headline-light">${_.escape(currentUsername)}</span> </a> </div> </div> @@ -1295,6 +1295,9 @@ export default class Notes { </li>` ); + $tempNote.find('.hidden-xs').text(_.escape(currentUserFullname)); + $tempNote.find('.note-headline-light').text(`@${_.escape(currentUsername)}`); + return $tempNote; } 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) { - `<li><a href='#' class='${selected === true ? 'is-active' : ''}'>${user.name}</a></li>`; + `<li><a href='#' class='${selected === true ? 'is-active' : ''}'>${_.escape(user.name)}</a></li>`; } else { if (avatar) { img = "<img src='" + avatar + "' class='avatar avatar-inline' width='32' />"; @@ -518,7 +518,7 @@ function UsersSelect(currentUser, els) { <a href='#' class='dropdown-menu-user-link ${selected === true ? 'is-active' : ''}'> ${img} <strong class='dropdown-menu-user-full-name'> - ${user.name} + ${_.escape(user.name)} </strong> ${username ? `<span class='dropdown-menu-user-username'>${username}</span>` : ''} </a> @@ -643,11 +643,11 @@ UsersSelect.prototype.formatResult = function(user) { } else { avatar = gon.default_avatar_url; } - return "<div class='user-result " + (!user.username ? 'no-username' : void 0) + "'> <div class='user-image'><img class='avatar avatar-inline s32' src='" + avatar + "'></div> <div class='user-name dropdown-menu-user-full-name'>" + user.name + "</div> <div class='user-username dropdown-menu-user-username'>" + (!user.invite ? "@" + _.escape(user.username) : "") + "</div> </div>"; + return "<div class='user-result " + (!user.username ? 'no-username' : void 0) + "'> <div class='user-image'><img class='avatar avatar-inline s32' src='" + avatar + "'></div> <div class='user-name dropdown-menu-user-full-name'>" + _.escape(user.name) + "</div> <div class='user-username dropdown-menu-user-username'>" + (!user.invite ? "@" + _.escape(user.username) : "") + "</div> </div>"; }; UsersSelect.prototype.formatSelection = function(user) { - return user.name; + return _.escape(user.name); }; UsersSelect.prototype.user = function(user_id, callback) { 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{<span class="commit-#{options[:source]}-name">#{person_name}</span>} + 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/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/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/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) -### <a name="route-map"></a>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 -<a name="convert"></a> 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 2d6e8ffc90f..9923ec4e870 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: (?<alignment>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) @@ -43,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') @@ -52,6 +57,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 +89,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/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) - "<!DOCTYPE html><html><head><meta content='#{import_prefix} git #{project_url}.git' name='go-import'></head></html>\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/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 - <a name="named-anchor"></a> - <sub>sub</sub> <dl> 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/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 <script>alert("XSS")</script>') + + 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 <script>alert("XSS")</script>') + + 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 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 <script>alert("XSS")</script>'; + 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', () => { diff --git a/spec/lib/banzai/filter/sanitization_filter_spec.rb b/spec/lib/banzai/filter/sanitization_filter_spec.rb index 35a32a46eff..01ceb21dfaa 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{<span>def</span>} 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 <table> <tr><th style="text-align: center">Head</th></tr> <tr><td style="text-align: right">Body</th></tr> @@ -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 + <table> + <tr><th style="text-align: foo">Head</th></tr> + <tr><td style="position: fixed; height: 50px; width: 50px; background: red; z-index: 999; font-size: 36px; text-align: center">Body</th></tr> + </table> + 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{<span>Hello</span>} expect(filter(act).to_html).to eq exp @@ -87,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 + <img name="getElementById" src=""> + <span name="foo" class="bar">Hi</span> + 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>summary line</summary>' expect(filter(act).to_html).to eq exp 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 = "<!DOCTYPE html><html><head><meta content='#{Gitlab.config.gitlab.host}/#{path} git http://#{Gitlab.config.gitlab.host}/#{path}.git' name='go-import'></head></html>\n" + expected_body = %{<html><head><meta name="go-import" content="#{Gitlab.config.gitlab.host}/#{path} git http://#{Gitlab.config.gitlab.host}/#{path}.git" /></head></html>} expect(response[2].body).to eq([expected_body]) 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) |