Merge branch 'security-ajk-forbid-mutations-in-GET-2' into 'master'

Forbid GET requests with mutations

See merge request gitlab-org/security/gitlab!1505

app/controllers/graphql_controller.rb

@@ -26,6 +26,8 @@ class GraphqlController < ApplicationController
   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 +64,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?

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

@@ -105,6 +105,7 @@ module Types
     mount_mutation Mutations::Namespace::PackageSettings::Update
     mount_mutation Mutations::UserCallouts::Create
     mount_mutation Mutations::Packages::Destroy
+    mount_mutation Mutations::Echo
   end
 end
 

doc/api/graphql/reference/index.md

@@ -1914,6 +1914,31 @@ Input type: `DiscussionToggleResolveInput`
 | <a id="mutationdiscussiontoggleresolvediscussion"></a>`discussion` | [`Discussion`](#discussion) | The discussion after mutation. |
 | <a id="mutationdiscussiontoggleresolveerrors"></a>`errors` | [`[String!]!`](#string) | Errors encountered during execution of the mutation. |
 
+### `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.enableDevopsAdoptionNamespace`
 
 **BETA** This endpoint is subject to change without notice.

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)
 

spec/support/helpers/graphql_helpers.rb

@@ -400,8 +400,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
@@ -410,6 +415,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,