diff options
| -rw-r--r-- | Gemfile | 2 | ||||
| -rw-r--r-- | Gemfile.lock | 4 | ||||
| -rw-r--r-- | app/controllers/graphql_controller.rb | 16 | ||||
| -rw-r--r-- | app/graphql/gitlab_schema.rb | 3 | ||||
| -rw-r--r-- | spec/controllers/graphql_controller_spec.rb | 38 | ||||
| -rw-r--r-- | spec/graphql/gitlab_schema_spec.rb | 34 | ||||
| -rw-r--r-- | spec/support/helpers/graphql_helpers.rb | 5 |
7 files changed, 98 insertions, 4 deletions
@@ -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'] |