Merge branch '22435-no-api-state-change-via-rails-session' into 'security'

API: disable rails session auth for non-GET/HEAD requests

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/22435

See merge request !1999

Signed-off-by: Rémy Coutable <remy@rymai.me>

8 files changed, 56 insertions, 17 deletions

diff --git a/CHANGELOG b/CHANGELOG
index a7a872dcc95..c0b3afa8fb5 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -3,6 +3,7 @@ Please view this file on the master branch, on stable branches it's out of date.
 v 8.10.11
   - Respect the fork_project permission when forking projects
   - Set a restrictive CORS policy on the API for credentialed requests
+  - API: disable rails session auth for non-GET/HEAD requests
 
 v 8.10.10
   - Allow the Rails cookie to be used for API authentication.
diff --git a/app/assets/javascripts/api.js.coffee b/app/assets/javascripts/api.js.coffee
index 19c59df8a9d..9a8fed23ab4 100644
--- a/app/assets/javascripts/api.js.coffee
+++ b/app/assets/javascripts/api.js.coffee
@@ -4,7 +4,7 @@
   namespacesPath: "/api/:version/namespaces.json"
   groupProjectsPath: "/api/:version/groups/:id/projects.json"
   projectsPath: "/api/:version/projects.json?simple=true"
-  labelsPath: "/api/:version/projects/:id/labels"
+  labelsPath: "/:namespace_path/:project_path/labels"
   licensePath: "/api/:version/licenses/:key"
   gitignorePath: "/api/:version/gitignores/:key"
   gitlabCiYmlPath: "/api/:version/gitlab_ci_ymls/:key"
@@ -60,14 +60,15 @@
     ).done (projects) ->
       callback(projects)
 
-  newLabel: (project_id, data, callback) ->
+  newLabel: (namespace_path, project_path, data, callback) ->
     url = Api.buildUrl(Api.labelsPath)
-    url = url.replace(':id', project_id)
+    url = url.replace(':namespace_path', namespace_path)
+             .replace(':project_path', project_path)
 
     $.ajax(
       url: url
       type: "POST"
-      data: data
+      data: {'label': data}
       dataType: "json"
     ).done (label) ->
       callback(label)
diff --git a/app/assets/javascripts/labels_select.js.coffee b/app/assets/javascripts/labels_select.js.coffee
index 7688609b301..ac64f1c5f99 100644
--- a/app/assets/javascripts/labels_select.js.coffee
+++ b/app/assets/javascripts/labels_select.js.coffee
@@ -4,7 +4,8 @@ class @LabelsSelect
 
     $('.js-label-select').each (i, dropdown) ->
       $dropdown = $(dropdown)
-      projectId = $dropdown.data('project-id')
+      namespacePath = $dropdown.data('namespace-path')
+      projectPath = $dropdown.data('project-path')
       labelUrl = $dropdown.data('labels')
       issueUpdateURL = $dropdown.data('issueUpdate')
       selectedLabel = $dropdown.data('selected')
@@ -88,7 +89,7 @@ class @LabelsSelect
 
         saveLabel = ->
           # Create new label with API
-          Api.newLabel projectId, {
+          Api.newLabel namespacePath, projectPath, {
             name: newLabelField.val()
             color: newColorField.val()
           }, (label) ->
diff --git a/app/controllers/projects/labels_controller.rb b/app/controllers/projects/labels_controller.rb
index 0ca675623e5..ce45be0e56a 100644
--- a/app/controllers/projects/labels_controller.rb
+++ b/app/controllers/projects/labels_controller.rb
@@ -30,9 +30,15 @@ class Projects::LabelsController < Projects::ApplicationController
     @label = @project.labels.create(label_params)
 
     if @label.valid?
-      redirect_to namespace_project_labels_path(@project.namespace, @project)
+      respond_to do |format|
+        format.html { redirect_to namespace_project_labels_path(@project.namespace, @project) }
+        format.json { render json: @label }
+      end
     else
-      render 'new'
+      respond_to do |format|
+        format.html { render 'new' }
+        format.json { render json: { message: @label.errors.messages }, status: 400 }
+      end
     end
   end
 
diff --git a/app/views/shared/issuable/_label_dropdown.html.haml b/app/views/shared/issuable/_label_dropdown.html.haml
index d34d28f6736..ebe441c931e 100644
--- a/app/views/shared/issuable/_label_dropdown.html.haml
+++ b/app/views/shared/issuable/_label_dropdown.html.haml
@@ -4,7 +4,7 @@
 - show_footer = local_assigns.fetch(:show_footer, true)
 - data_options = local_assigns.fetch(:data_options, {})
 - classes = local_assigns.fetch(:classes, [])
-- dropdown_data = {toggle: 'dropdown', field_name: 'label_name[]', show_no: "true", show_any: "true", selected: params[:label_name], project_id: @project.try(:id), labels: labels_filter_path, default_label: "Label"}
+- dropdown_data = {toggle: 'dropdown', field_name: 'label_name[]', show_no: "true", show_any: "true", selected: params[:label_name], namespace_path: @project.try(:namespace).try(:path), project_path: @project.try(:path), labels: labels_filter_path, default_label: "Label"}
 - dropdown_data.merge!(data_options)
 - classes << 'js-extra-options' if extra_options
 - classes << 'js-filter-submit' if filter_submit
diff --git a/app/views/shared/issuable/_sidebar.html.haml b/app/views/shared/issuable/_sidebar.html.haml
index 8e2fcbdfab8..330ac05dc21 100644
--- a/app/views/shared/issuable/_sidebar.html.haml
+++ b/app/views/shared/issuable/_sidebar.html.haml
@@ -128,7 +128,7 @@
             - issuable.labels_array.each do |label|
               = hidden_field_tag "#{issuable.to_ability_name}[label_names][]", label.id, id: nil
             .dropdown
-              %button.dropdown-menu-toggle.js-label-select.js-multiselect{type: "button", data: {toggle: "dropdown", field_name: "#{issuable.to_ability_name}[label_names][]", ability_name: issuable.to_ability_name, show_no: "true", show_any: "true", project_id: (@project.id if @project), issue_update: issuable_json_path(issuable), labels: (namespace_project_labels_path(@project.namespace, @project, :json) if @project)}}
+              %button.dropdown-menu-toggle.js-label-select.js-multiselect{type: "button", data: {toggle: "dropdown", field_name: "#{issuable.to_ability_name}[label_names][]", ability_name: issuable.to_ability_name, show_no: "true", show_any: "true", namespace_path: @project.try(:namespace).try(:path), project_path: @project.try(:path), issue_update: issuable_json_path(issuable), labels: (namespace_project_labels_path(@project.namespace, @project, :json) if @project)}}
                 %span.dropdown-toggle-text
                   Label
                 = icon('chevron-down')
diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb
index 28225281fee..5cc3f2be85e 100644
--- a/lib/api/helpers.rb
+++ b/lib/api/helpers.rb
@@ -25,8 +25,11 @@ module API
     end
 
     # Check the Rails session for valid authentication details
+    #
+    # Until CSRF protection is added to the API, disallow this method for
+    # state-changing endpoints
     def find_user_from_warden
-      warden ? warden.authenticate : nil
+      warden.try(:authenticate) if request.get? || request.head?
     end
 
     def find_user_by_private_token
diff --git a/spec/requests/api/api_helpers_spec.rb b/spec/requests/api/api_helpers_spec.rb
index 90a26d56a90..9457eafe622 100644
--- a/spec/requests/api/api_helpers_spec.rb
+++ b/spec/requests/api/api_helpers_spec.rb
@@ -9,7 +9,8 @@ describe API::Helpers, api: true do
   let(:key) { create(:key, user: user) }
 
   let(:params) { {} }
-  let(:env) { {} }
+  let(:env) { { 'REQUEST_METHOD' => 'GET' } }
+  let(:request) { Rack::Request.new(env) }
 
   def set_env(token_usr, identifier)
     clear_env
@@ -51,17 +52,43 @@ describe API::Helpers, api: true do
   describe ".current_user" do
     subject { current_user }
 
-    describe "when authenticating via Warden" do
+    describe "Warden authentication" do
       before { doorkeeper_guard_returns false }
 
-      context "fails" do
-        it { is_expected.to be_nil }
+      context "with invalid credentials" do
+        context "GET request" do
+          before { env['REQUEST_METHOD'] = 'GET' }
+          it { is_expected.to be_nil }
+        end
       end
 
-      context "succeeds" do
+      context "with valid credentials" do
         before { warden_authenticate_returns user }
 
-        it { is_expected.to eq(user) }
+        context "GET request" do
+          before { env['REQUEST_METHOD'] = 'GET' }
+          it { is_expected.to eq(user) }
+        end
+
+        context "HEAD request" do
+          before { env['REQUEST_METHOD'] = 'HEAD' }
+          it { is_expected.to eq(user) }
+        end
+
+        context "PUT request" do
+          before { env['REQUEST_METHOD'] = 'PUT' }
+          it { is_expected.to be_nil }
+        end
+
+        context "POST request" do
+          before { env['REQUEST_METHOD'] = 'POST' }
+          it { is_expected.to be_nil }
+        end
+
+        context "DELETE request" do
+          before { env['REQUEST_METHOD'] = 'DELETE' }
+          it { is_expected.to be_nil }
+        end
       end
     end