Merge branch '29943-environment-folder' into 'security-9-5'

Do not use `location.pathname` when accessing environments folders

See merge request !2147

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));
       }
     },
 

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);
     },
   },
 };

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

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

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

changelogs/unreleased/29943-environment-folder.yml

@@ -0,0 +1,4 @@
+---
+title: Resolve CSRF token leakage via pathname manipulation on environments page
+merge_request:
+author:

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

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: "")

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)