Add latest changes from gitlab-org/security/gitlab@13-12-stable-ee

app/controllers/graphql_controller.rb

@@ -20,12 +20,16 @@ class GraphqlController < ApplicationController
   # around in GraphiQL.
   protect_from_forgery with: :null_session, only: :execute
 
-  before_action :authorize_access_api!
+  # must come first: current_user is set up here
   before_action(only: [:execute]) { authenticate_sessionless_user!(:api) }
+
+  before_action :authorize_access_api!
   before_action :set_user_last_activity
   before_action :track_vs_code_usage
   before_action :disable_query_limiting
 
+  before_action :disallow_mutations_for_get
+
   # Since we deactivate authentication from the main ApplicationController and
   # defer it to :authorize_access_api!, we need to override the bypass session
   # callback execution order here
@@ -62,6 +66,25 @@ class GraphqlController < ApplicationController
 
   private
 
+  def disallow_mutations_for_get
+    return unless request.get? || request.head?
+    return unless any_mutating_query?
+
+    raise ::Gitlab::Graphql::Errors::ArgumentError, "Mutations are forbidden in #{request.request_method} requests"
+  end
+
+  def any_mutating_query?
+    if multiplex?
+      multiplex_queries.any? { |q| mutation?(q[:query], q[:operation_name]) }
+    else
+      mutation?(query)
+    end
+  end
+
+  def mutation?(query_string, operation_name = params[:operationName])
+    ::GraphQL::Query.new(GitlabSchema, query_string, operation_name: operation_name).mutation?
+  end
+
   # Tests may mark some GraphQL queries as exempt from SQL query limits
   def disable_query_limiting
     return unless Gitlab::QueryLimiting.enabled_for_env?
@@ -130,7 +153,9 @@ class GraphqlController < ApplicationController
   end
 
   def authorize_access_api!
-    access_denied!("API not accessible for user.") unless can?(current_user, :access_api)
+    return if can?(current_user, :access_api)
+
+    render_error('API not accessible for user', status: :forbidden)
   end
 
   # Overridden from the ApplicationController to make the response look like

app/graphql/mutations/echo.rb

+# frozen_string_literal: true
+
+module Mutations
+  class Echo < BaseMutation
+    graphql_name 'EchoCreate'
+    description <<~DOC
+      A mutation that does not perform any changes.
+
+      This is expected to be used for testing of endpoints, to verify
+      that a user has mutation access.
+    DOC
+
+    argument :errors,
+             type: [::GraphQL::STRING_TYPE],
+             required: false,
+             description: 'Errors to return to the user.'
+
+    argument :messages,
+             type: [::GraphQL::STRING_TYPE],
+             as: :echoes,
+             required: false,
+             description: 'Messages to return to the user.'
+
+    field :echoes,
+          type: [::GraphQL::STRING_TYPE],
+          null: true,
+          description: 'Messages returned to the user.'
+
+    def resolve(**args)
+      args
+    end
+  end
+end

app/graphql/types/mutation_type.rb

@@ -101,6 +101,7 @@ module Types
     mount_mutation Mutations::Ci::Job::Retry
     mount_mutation Mutations::Namespace::PackageSettings::Update
     mount_mutation Mutations::UserCallouts::Create
+    mount_mutation Mutations::Echo
   end
 end
 

doc/api/graphql/reference/index.md

@@ -1959,6 +1959,31 @@ Input type: `DismissVulnerabilityInput`
 | <a id="mutationdismissvulnerabilityerrors"></a>`errors` | [`[String!]!`](#string) | Errors encountered during execution of the mutation. |
 | <a id="mutationdismissvulnerabilityvulnerability"></a>`vulnerability` | [`Vulnerability`](#vulnerability) | The vulnerability after dismissal. |
 
+### `Mutation.echoCreate`
+
+A mutation that does not perform any changes.
+
+This is expected to be used for testing of endpoints, to verify
+that a user has mutation access.
+
+Input type: `EchoCreateInput`
+
+#### Arguments
+
+| Name | Type | Description |
+| ---- | ---- | ----------- |
+| <a id="mutationechocreateclientmutationid"></a>`clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. |
+| <a id="mutationechocreateerrors"></a>`errors` | [`[String!]`](#string) | Errors to return to the user. |
+| <a id="mutationechocreatemessages"></a>`messages` | [`[String!]`](#string) | Messages to return to the user. |
+
+#### Fields
+
+| Name | Type | Description |
+| ---- | ---- | ----------- |
+| <a id="mutationechocreateclientmutationid"></a>`clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. |
+| <a id="mutationechocreateechoes"></a>`echoes` | [`[String!]`](#string) | Messages returned to the user. |
+| <a id="mutationechocreateerrors"></a>`errors` | [`[String!]!`](#string) | Errors encountered during execution of the mutation. |
+
 ### `Mutation.environmentsCanaryIngressUpdate`
 
 Input type: `EnvironmentsCanaryIngressUpdateInput`

spec/controllers/graphql_controller_spec.rb

@@ -44,7 +44,7 @@ RSpec.describe GraphqlController do
         expect(response).to have_gitlab_http_status(:ok)
       end
 
-      it 'returns access denied template when user cannot access API' do
+      it 'returns forbidden when user cannot access API' do
         # User cannot access API in a couple of cases
         # * When user is internal(like ghost users)
         # * When user is blocked
@@ -54,7 +54,9 @@ RSpec.describe GraphqlController do
         post :execute
 
         expect(response).to have_gitlab_http_status(:forbidden)
-        expect(response).to render_template('errors/access_denied')
+        expect(json_response).to include(
+          'errors' => include(a_hash_including('message' => /API not accessible/))
+        )
       end
 
       it 'updates the users last_activity_on field' do

spec/policies/global_policy_spec.rb

@@ -239,6 +239,14 @@ RSpec.describe GlobalPolicy do
       it { is_expected.not_to be_allowed(:access_api) }
     end
 
+    context 'with a deactivated user' do
+      before do
+        current_user.deactivate!
+      end
+
+      it { is_expected.not_to be_allowed(:access_api) }
+    end
+
     context 'user with expired password' do
       before do
         current_user.update!(password_expires_at: 2.minutes.ago)

spec/requests/api/graphql/mutations/merge_requests/set_assignees_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe 'Setting assignees of a merge request' do
+RSpec.describe 'Setting assignees of a merge request', :clean_gitlab_redis_shared_state do
   include GraphqlHelpers
 
   let_it_be(:project) { create(:project, :repository) }
@@ -46,6 +46,8 @@ RSpec.describe 'Setting assignees of a merge request' do
   end
 
   def run_mutation!
+    post_graphql_mutation(mutation, current_user: current_user) # warm-up
+
     recorder = ActiveRecord::QueryRecorder.new do
       post_graphql_mutation(mutation, current_user: current_user)
     end

spec/requests/api/graphql_spec.rb

@@ -6,6 +6,9 @@ RSpec.describe 'GraphQL' do
   include AfterNextHelpers
 
   let(:query) { graphql_query_for('echo', text: 'Hello world') }
+  let(:mutation) { 'mutation { echoCreate(input: { messages: ["hello", "world"] }) { echoes } }' }
+
+  let_it_be(:user) { create(:user) }
 
   describe 'logging' do
     shared_examples 'logging a graphql query' do
@@ -70,6 +73,139 @@ RSpec.describe 'GraphQL' do
     end
   end
 
+  context 'when executing mutations' do
+    let(:mutation_with_variables) do
+      <<~GQL
+      mutation($a: String!, $b: String!) {
+        echoCreate(input: { messages: [$a, $b] }) { echoes }
+      }
+      GQL
+    end
+
+    context 'with POST' do
+      it 'succeeds' do
+        post_graphql(mutation, current_user: user)
+
+        expect(graphql_data_at(:echo_create, :echoes)).to eq %w[hello world]
+      end
+
+      context 'with variables' do
+        it 'succeeds' do
+          post_graphql(mutation_with_variables, current_user: user, variables: { a: 'Yo', b: 'there' })
+
+          expect(graphql_data_at(:echo_create, :echoes)).to eq %w[Yo there]
+        end
+      end
+    end
+
+    context 'with GET' do
+      it 'fails' do
+        get_graphql(mutation, current_user: user)
+
+        expect(graphql_errors).to include(a_hash_including('message' => /Mutations are forbidden/))
+      end
+
+      context 'with variables' do
+        it 'fails' do
+          get_graphql(mutation_with_variables, current_user: user, variables: { a: 'Yo', b: 'there' })
+
+          expect(graphql_errors).to include(a_hash_including('message' => /Mutations are forbidden/))
+        end
+      end
+    end
+  end
+
+  context 'when executing queries' do
+    context 'with POST' do
+      it 'succeeds' do
+        post_graphql(query, current_user: user)
+
+        expect(graphql_data_at(:echo)).to include 'Hello world'
+      end
+    end
+
+    context 'with GET' do
+      it 'succeeds' do
+        get_graphql(query, current_user: user)
+
+        expect(graphql_data_at(:echo)).to include 'Hello world'
+      end
+    end
+  end
+
+  context 'when selecting a query by operation name' do
+    let(:query) { "query A #{graphql_query_for('echo', text: 'Hello world')}" }
+    let(:mutation) { 'mutation B { echoCreate(input: { messages: ["hello", "world"] }) { echoes } }' }
+
+    let(:combined) { [query, mutation].join("\n\n") }
+
+    context 'with POST' do
+      it 'succeeds when selecting the query' do
+        post_graphql(combined, current_user: user, params: { operationName: 'A' })
+
+        resp = json_response
+
+        expect(resp.dig('data', 'echo')).to include 'Hello world'
+      end
+
+      it 'succeeds when selecting the mutation' do
+        post_graphql(combined, current_user: user, params: { operationName: 'B' })
+
+        resp = json_response
+
+        expect(resp.dig('data', 'echoCreate', 'echoes')).to eq %w[hello world]
+      end
+    end
+
+    context 'with GET' do
+      it 'succeeds when selecting the query' do
+        get_graphql(combined, current_user: user, params: { operationName: 'A' })
+
+        resp = json_response
+
+        expect(resp.dig('data', 'echo')).to include 'Hello world'
+      end
+
+      it 'fails when selecting the mutation' do
+        get_graphql(combined, current_user: user, params: { operationName: 'B' })
+
+        resp = json_response
+
+        expect(resp.dig('errors', 0, 'message')).to include "Mutations are forbidden"
+      end
+    end
+  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
+
+    context 'with POST' do
+      it 'succeeds' do
+        post_multiplex(batched, current_user: user)
+
+        resp = json_response
+
+        expect(resp.dig(0, 'data', 'echo')).to include 'Hello world'
+        expect(resp.dig(1, 'data', 'echoCreate', 'echoes')).to eq %w[hello world]
+      end
+    end
+
+    context 'with GET' do
+      it 'fails with a helpful error message' do
+        get_multiplex(batched, current_user: user)
+
+        resp = json_response
+
+        expect(resp.dig('errors', 0, 'message')).to include "Mutations are forbidden"
+      end
+    end
+  end
+
   context 'with invalid variables' do
     it 'returns an error' do
       post_graphql(query, variables: "This is not JSON")
@@ -80,8 +216,6 @@ RSpec.describe 'GraphQL' do
   end
 
   describe 'authentication', :allow_forgery_protection do
-    let(:user) { create(:user) }
-
     it 'allows access to public data without authentication' do
       post_graphql(query)
 
@@ -109,11 +243,9 @@ RSpec.describe 'GraphQL' do
     context 'with token authentication' do
       let(:token) { create(:personal_access_token) }
 
-      before do
+      it 'authenticates users with a PAT' do
         stub_authentication_activity_metrics(debug: false)
-      end
 
-      it 'authenticates users with a PAT' do
         expect(authentication_metrics)
           .to increment(:user_authenticated_counter)
           .and increment(:user_session_override_counter)
@@ -124,6 +256,14 @@ RSpec.describe 'GraphQL' do
         expect(graphql_data['echo']).to eq("\"#{token.user.username}\" says: Hello world")
       end
 
+      it 'prevents access by deactived users' do
+        token.user.deactivate!
+
+        post_graphql(query, headers: { 'PRIVATE-TOKEN' => token.token })
+
+        expect(graphql_errors).to include({ 'message' => /API not accessible/ })
+      end
+
       context 'when the personal access token has no api scope' do
         it 'does not log the user in' do
           token.update!(scopes: [:read_user])

spec/support/helpers/graphql_helpers.rb

@@ -396,8 +396,13 @@ module GraphqlHelpers
     post api('/', current_user, version: 'graphql'), params: { _json: queries }, headers: headers
   end
 
-  def post_graphql(query, current_user: nil, variables: nil, headers: {}, token: {})
-    params = { query: query, variables: serialize_variables(variables) }
+  def get_multiplex(queries, current_user: nil, headers: {})
+    path = "/?#{queries.to_query('_json')}"
+    get api(path, current_user, version: 'graphql'), headers: headers
+  end
+
+  def post_graphql(query, current_user: nil, variables: nil, headers: {}, token: {}, params: {})
+    params = params.merge(query: query, variables: serialize_variables(variables))
     post api('/', current_user, version: 'graphql', **token), params: params, headers: headers
 
     return unless graphql_errors
@@ -406,6 +411,18 @@ module GraphqlHelpers
     expect(graphql_errors).not_to include(a_hash_including('message' => 'Internal server error'))
   end
 
+  def get_graphql(query, current_user: nil, variables: nil, headers: {}, token: {}, params: {})
+    vars = "variables=#{CGI.escape(serialize_variables(variables))}" if variables
+    params = params.to_a.map { |k, v| v.to_query(k) }
+    path = ["/?query=#{CGI.escape(query)}", vars, *params].join('&')
+    get api(path, current_user, version: 'graphql', **token), headers: headers
+
+    return unless graphql_errors
+
+    # Errors are acceptable, but not this one:
+    expect(graphql_errors).not_to include(a_hash_including('message' => 'Internal server error'))
+  end
+
   def post_graphql_mutation(mutation, current_user: nil, token: {})
     post_graphql(mutation.query,
                  current_user: current_user,