Merge branch '379326-graphql-skip-csrf-for-queries' into 'master'

Skip CSRF check for query-only GraphQL requests

See merge request !175955



Merged-by: Heinrich Lee Yu <heinrich@gitlab.com>
Approved-by: Andrew Evans <aevans@gitlab.com>
Approved-by: Grzegorz Bizon <grzegorz@gitlab.com>
Reviewed-by: Grzegorz Bizon <grzegorz@gitlab.com>

app/controllers/graphql_controller.rb

@@ -11,7 +11,8 @@ class GraphqlController < ApplicationController
   # Also, we allow anonymous users to access the API without a CSRF token so that it is easier for users
   # to get started with our GraphQL API.
   skip_before_action :verify_authenticity_token, if: -> {
-    Feature.enabled?(:fix_graphql_csrf, Feature.current_request) && (current_user.nil? || sessionless_user?)
+    Feature.enabled?(:fix_graphql_csrf, Feature.current_request) &&
+      (current_user.nil? || sessionless_user? || !any_mutating_query?)
   }
   skip_before_action :check_two_factor_requirement, if: -> {
     Feature.enabled?(:fix_graphql_csrf, Feature.current_request) && sessionless_user?

spec/requests/api/graphql_spec.rb

@@ -229,7 +229,20 @@
     end
 
     describe 'request forgery protection' do
-      it 'does not authenticate a user with an invalid CSRF' do
+      it 'allows queries even with an invalid CSRF token' do
+        login_as(user)
+
+        stub_authentication_activity_metrics do |metrics|
+          expect(metrics)
+            .to increment(:user_authenticated_counter)
+        end
+
+        post_graphql(query, headers: { 'X-CSRF-Token' => 'invalid' })
+
+        expect(graphql_data['echo']).to eq("\"#{user.username}\" says: Hello world")
+      end
+
+      it 'does not allow mutations with an invalid CSRF token' do
         login_as(user)
 
         stub_authentication_activity_metrics do |metrics|
@@ -240,12 +253,12 @@
             .to receive(:increment).with(controller: 'GraphqlController', auth: 'session')
         end
 
-        post_graphql(query, headers: { 'X-CSRF-Token' => 'invalid' })
+        post_graphql(mutation, headers: { 'X-CSRF-Token' => 'invalid' })
 
         expect(response).to have_gitlab_http_status(:unprocessable_entity)
       end
 
-      it 'authenticates a user with a valid session token' do
+      it 'allows mutations with a valid CSRF token' do
         # Create a session to get a CSRF token from
         login_as(user)
         get('/')
@@ -254,9 +267,36 @@
           expect(metrics.user_csrf_token_invalid_counter).not_to receive(:increment)
         end
 
-        post '/api/graphql', params: { query: query }, headers: { 'X-CSRF-Token' => session['_csrf_token'] }
+        post_graphql(mutation, headers: { 'X-CSRF-Token' => session['_csrf_token'] })
 
-        expect(graphql_data['echo']).to eq("\"#{user.username}\" says: Hello world")
+        expect(graphql_data_at(:echo_create, :echoes)).to eq %w[hello world]
+      end
+
+      context 'when batching mutations and queries' do
+        let(:batched) do
+          [
+            { query: "query A #{graphql_query_for('echo', text: 'Hello world')}" },
+            { query: 'mutation B { echoCreate(input: { messages: ["hello", "world"] }) { echoes } }' }
+          ]
+        end
+
+        it 'does not allow multiplexed request with an invalid CSRF token' do
+          login_as(user)
+
+          post_multiplex(batched, headers: { 'X-CSRF-Token' => 'invalid' })
+
+          expect(response).to have_gitlab_http_status(:unprocessable_entity)
+        end
+
+        it 'allows multiplexed request with valid CSRF token' do
+          login_as(user)
+          get('/')
+
+          post_multiplex(batched, headers: { 'X-CSRF-Token' => session['_csrf_token'] })
+
+          expect(json_response[0].dig('data', 'echo')).to eq("\"#{user.username}\" says: Hello world")
+          expect(json_response[1].dig('data', 'echoCreate', 'echoes')).to eq %w[hello world]
+        end
       end
 
       context 'when fix_graphql_csrf is disabled' do
@@ -680,6 +720,15 @@
           expect_graphql_errors_to_include("does not exist or you don't have permission")
         end
       end
+
+      context 'when request is cross-origin' do
+        it 'does not allow cookie credentials' do
+          post '/api/graphql', headers: { 'Origin' => 'http://notgitlab.com', 'Access-Control-Request-Method' => 'POST' }
+
+          expect(response.headers['Access-Control-Allow-Origin']).to eq '*'
+          expect(response.headers['Access-Control-Allow-Credentials']).to be_nil
+        end
+      end
     end
   end