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) + describe 'environments table' do + given!(:environment) do + create(:environment, project: project, state: :available) 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') - end + context 'when there are no deployments' do + before do + visit_environments(project) + end - context 'without deployments' do - scenario 'does show no deployments' do + it 'shows environments names and counters' do + expect(page).to have_link(environment.name) + + expect(page.find('.js-available-environments-count').text).to eq('1') + expect(page.find('.js-stopped-environments-count').text).to eq('0') + end + + it 'does not show deployments' do expect(page).to have_content('No deployments yet') end - context 'for available environment' do - given(:environment) { create(:environment, project: project, state: :available) } - - scenario 'does not shows stop button' do - expect(page).not_to have_selector('.stop-env-link') - end - end - - context 'for stopped environment' do - given(:environment) { create(:environment, project: project, state: :stopped) } - - 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 + context 'user is a developer' do + given(:role) { :developer } + + 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' + + expect(page).to have_content('production') end - context 'for valid name' do - before do - fill_in('Name', with: 'production') - 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 create a new pipeline' do - expect(page).to have_content('production') - end - end - - context 'for invalid name' do - before do - fill_in('Name', with: 'name,with,commas') - click_on 'Save' - end - - 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)