summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
-rw-r--r--Gemfile2
-rw-r--r--Gemfile.lock4
-rw-r--r--app/controllers/graphql_controller.rb16
-rw-r--r--app/graphql/gitlab_schema.rb3
-rw-r--r--spec/controllers/graphql_controller_spec.rb38
-rw-r--r--spec/graphql/gitlab_schema_spec.rb34
-rw-r--r--spec/support/helpers/graphql_helpers.rb5
7 files changed, 98 insertions, 4 deletions
diff --git a/Gemfile b/Gemfile
index 87a0cff84c1..7fa615319c4 100644
--- a/Gemfile
+++ b/Gemfile
@@ -97,7 +97,7 @@ gem 'grape-entity', '~> 0.10.0'
gem 'rack-cors', '~> 1.0.6', require: 'rack/cors'
# GraphQL API
-gem 'graphql', '~> 1.11.8'
+gem 'graphql', '~> 1.11.10'
# NOTE: graphiql-rails v1.5+ doesn't work: https://gitlab.com/gitlab-org/gitlab/issues/31771
# TODO: remove app/views/graphiql/rails/editors/show.html.erb when https://github.com/rmosolgo/graphiql-rails/pull/71 is released:
# https://gitlab.com/gitlab-org/gitlab/issues/31747
diff --git a/Gemfile.lock b/Gemfile.lock
index 270ea4532f9..b54874e9d80 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -558,7 +558,7 @@ GEM
faraday (>= 1.0)
faraday_middleware
graphql-client
- graphql (1.11.8)
+ graphql (1.11.10)
graphql-client (0.16.0)
activesupport (>= 3.0)
graphql (~> 1.8)
@@ -1490,7 +1490,7 @@ DEPENDENCIES
grape_logging (~> 1.7)
graphiql-rails (~> 1.4.10)
graphlient (~> 0.4.0)
- graphql (~> 1.11.8)
+ graphql (~> 1.11.10)
graphql-docs (~> 1.6.0)
grpc (~> 1.30.2)
gssapi
diff --git a/app/controllers/graphql_controller.rb b/app/controllers/graphql_controller.rb
index fde0f133e53..899fa614949 100644
--- a/app/controllers/graphql_controller.rb
+++ b/app/controllers/graphql_controller.rb
@@ -9,6 +9,9 @@ class GraphqlController < ApplicationController
# Header can be passed by tests to disable SQL query limits.
DISABLE_SQL_QUERY_LIMIT_HEADER = 'HTTP_X_GITLAB_DISABLE_SQL_QUERY_LIMIT'
+ # Max size of the query text in characters
+ MAX_QUERY_SIZE = 10_000
+
# If a user is using their session to access GraphQL, we need to have session
# storage, since the admin-mode check is session wide.
# We can't enable this for anonymous users because that would cause users using
@@ -29,6 +32,7 @@ class GraphqlController < ApplicationController
before_action :set_user_last_activity
before_action :track_vs_code_usage
before_action :disable_query_limiting
+ before_action :limit_query_size
before_action :disallow_mutations_for_get
@@ -81,6 +85,16 @@ class GraphqlController < ApplicationController
raise ::Gitlab::Graphql::Errors::ArgumentError, "Mutations are forbidden in #{request.request_method} requests"
end
+ def limit_query_size
+ total_size = if multiplex?
+ params[:_json].sum { _1[:query].size }
+ else
+ query.size
+ end
+
+ raise ::Gitlab::Graphql::Errors::ArgumentError, "Query too large" if total_size > MAX_QUERY_SIZE
+ end
+
def any_mutating_query?
if multiplex?
multiplex_queries.any? { |q| mutation?(q[:query], q[:operation_name]) }
@@ -126,7 +140,7 @@ class GraphqlController < ApplicationController
end
def query
- params[:query]
+ params.fetch(:query, '')
end
def multiplex_queries
diff --git a/app/graphql/gitlab_schema.rb b/app/graphql/gitlab_schema.rb
index e15a185a743..9b23aa60eab 100644
--- a/app/graphql/gitlab_schema.rb
+++ b/app/graphql/gitlab_schema.rb
@@ -32,6 +32,9 @@ class GitlabSchema < GraphQL::Schema
default_max_page_size 100
+ validate_max_errors 5
+ validate_timeout 0.2.seconds
+
lazy_resolve ::Gitlab::Graphql::Lazy, :force
class << self
diff --git a/spec/controllers/graphql_controller_spec.rb b/spec/controllers/graphql_controller_spec.rb
index 6e7bcfdaa08..f9b15c9a48e 100644
--- a/spec/controllers/graphql_controller_spec.rb
+++ b/spec/controllers/graphql_controller_spec.rb
@@ -52,6 +52,44 @@ RSpec.describe GraphqlController do
expect(response).to have_gitlab_http_status(:ok)
end
+ it 'executes a simple query with no errors' do
+ post :execute, params: { query: '{ __typename }' }
+
+ expect(response).to have_gitlab_http_status(:ok)
+ expect(json_response).to eq({ 'data' => { '__typename' => 'Query' } })
+ end
+
+ it 'executes a simple multiplexed query with no errors' do
+ multiplex = [{ query: '{ __typename }' }] * 2
+
+ post :execute, params: { _json: multiplex }
+
+ expect(response).to have_gitlab_http_status(:ok)
+ expect(json_response).to eq([
+ { 'data' => { '__typename' => 'Query' } },
+ { 'data' => { '__typename' => 'Query' } }
+ ])
+ end
+
+ it 'sets a limit on the total query size' do
+ graphql_query = "{#{(['__typename'] * 1000).join(' ')}}"
+
+ post :execute, params: { query: graphql_query }
+
+ expect(response).to have_gitlab_http_status(:unprocessable_entity)
+ expect(json_response).to eq({ 'errors' => [{ 'message' => 'Query too large' }] })
+ end
+
+ it 'sets a limit on the total query size for multiplex queries' do
+ graphql_query = "{#{(['__typename'] * 200).join(' ')}}"
+ multiplex = [{ query: graphql_query }] * 5
+
+ post :execute, params: { _json: multiplex }
+
+ expect(response).to have_gitlab_http_status(:unprocessable_entity)
+ expect(json_response).to eq({ 'errors' => [{ 'message' => 'Query too large' }] })
+ end
+
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)
diff --git a/spec/graphql/gitlab_schema_spec.rb b/spec/graphql/gitlab_schema_spec.rb
index 3fa0dc95126..02c686af688 100644
--- a/spec/graphql/gitlab_schema_spec.rb
+++ b/spec/graphql/gitlab_schema_spec.rb
@@ -35,6 +35,10 @@ RSpec.describe GitlabSchema do
expect(connection).to eq(Gitlab::Graphql::Pagination::ExternallyPaginatedArrayConnection)
end
+ it 'sets an appropriate validation timeout' do
+ expect(described_class.validate_timeout).to be <= 0.2.seconds
+ end
+
describe '.execute' do
describe 'setting query `max_complexity` and `max_depth`' do
subject(:result) { described_class.execute('query', **kwargs).query }
@@ -195,6 +199,36 @@ RSpec.describe GitlabSchema do
end
end
+ describe 'validate_max_errors' do
+ it 'reports at most 5 errors' do
+ query = <<~GQL
+ query {
+ currentUser {
+ x: id
+ x: bot
+ x: username
+ x: state
+ x: name
+
+ x: id
+ x: bot
+ x: username
+ x: state
+ x: name
+
+ badField
+ veryBadField
+ alsoNotAGoodField
+ }
+ }
+ GQL
+
+ result = described_class.execute(query)
+
+ expect(result.to_h['errors'].count).to eq 5
+ end
+ end
+
describe '.parse_gid' do
let_it_be(:global_id) { 'gid://gitlab/TestOne/2147483647' }
diff --git a/spec/support/helpers/graphql_helpers.rb b/spec/support/helpers/graphql_helpers.rb
index ee4621deb2d..1f0c9b658dc 100644
--- a/spec/support/helpers/graphql_helpers.rb
+++ b/spec/support/helpers/graphql_helpers.rb
@@ -374,6 +374,7 @@ module GraphqlHelpers
allow_unlimited_graphql_depth if max_depth > 1
allow_high_graphql_recursion
allow_high_graphql_transaction_threshold
+ allow_high_graphql_query_size
type = class_name.respond_to?(:kind) ? class_name : GitlabSchema.types[class_name.to_s]
raise "#{class_name} is not a known type in the GitlabSchema" unless type
@@ -624,6 +625,10 @@ module GraphqlHelpers
stub_const("Gitlab::QueryLimiting::Transaction::THRESHOLD", 1000)
end
+ def allow_high_graphql_query_size
+ stub_const('GraphqlController::MAX_QUERY_SIZE', 10_000_000)
+ end
+
def node_array(data, extract_attribute = nil)
data.map do |item|
extract_attribute ? item['node'][extract_attribute] : item['node']