From f458c561070d754cd546b07caf60dfa7ffb06293 Mon Sep 17 00:00:00 2001 From: Brett Walker Date: Wed, 27 Mar 2019 15:02:25 -0500 Subject: Initial field and query complexity limits It makes all Types::BaseField default to a complexity of 1. Queries themselves now have limited complexity, scaled to the type of user: no user, authenticated user, or an admin user. --- app/graphql/gitlab_schema.rb | 30 ++++++++++++++++++++ app/graphql/types/base_field.rb | 9 ++++++ ...asic-limiting-complexity-of-graphql-queries.yml | 5 ++++ .../query_analyzers/log_query_complexity.rb | 18 ++++++++++++ spec/graphql/gitlab_schema_spec.rb | 32 ++++++++++++++++++++++ spec/graphql/types/base_field_spec.rb | 19 +++++++++++++ spec/requests/api/graphql/gitlab_schema_spec.rb | 16 +++++++++++ spec/support/helpers/graphql_helpers.rb | 8 ++++++ 8 files changed, 137 insertions(+) create mode 100644 changelogs/unreleased/58405-basic-limiting-complexity-of-graphql-queries.yml create mode 100644 lib/gitlab/graphql/query_analyzers/log_query_complexity.rb create mode 100644 spec/graphql/types/base_field_spec.rb create mode 100644 spec/requests/api/graphql/gitlab_schema_spec.rb diff --git a/app/graphql/gitlab_schema.rb b/app/graphql/gitlab_schema.rb index 06d26309b5b..ff4d0611da9 100644 --- a/app/graphql/gitlab_schema.rb +++ b/app/graphql/gitlab_schema.rb @@ -1,13 +1,43 @@ # frozen_string_literal: true class GitlabSchema < GraphQL::Schema + # Took our current most complicated query in use, issues.graphql, + # with a complexity of 19, and added a 20 point buffer to it. + # These values will evolve over time. + DEFAULT_MAX_COMPLEXITY = 40 + AUTHENTICATED_COMPLEXITY = 50 + ADMIN_COMPLEXITY = 60 + use BatchLoader::GraphQL use Gitlab::Graphql::Authorize use Gitlab::Graphql::Present use Gitlab::Graphql::Connections + query_analyzer Gitlab::Graphql::QueryAnalyzers::LogQueryComplexity.analyzer + query(Types::QueryType) default_max_page_size 100 + + max_complexity DEFAULT_MAX_COMPLEXITY + mutation(Types::MutationType) + + def self.execute(query_str = nil, **kwargs) + kwargs[:max_complexity] ||= max_query_complexity(kwargs[:context]) + + super(query_str, **kwargs) + end + + def self.max_query_complexity(ctx) + current_user = ctx&.fetch(:current_user) + + if current_user&.admin + ADMIN_COMPLEXITY + elsif current_user + AUTHENTICATED_COMPLEXITY + else + DEFAULT_MAX_COMPLEXITY + end + end end diff --git a/app/graphql/types/base_field.rb b/app/graphql/types/base_field.rb index 2b2ea64c00b..8c8b8a82d3e 100644 --- a/app/graphql/types/base_field.rb +++ b/app/graphql/types/base_field.rb @@ -3,5 +3,14 @@ module Types class BaseField < GraphQL::Schema::Field prepend Gitlab::Graphql::Authorize + + DEFAULT_COMPLEXITY = 1 + + def initialize(*args, **kwargs, &block) + # complexity is already defaulted to 1, but let's make it explicit + kwargs[:complexity] ||= DEFAULT_COMPLEXITY + + super(*args, **kwargs, &block) + end end end diff --git a/changelogs/unreleased/58405-basic-limiting-complexity-of-graphql-queries.yml b/changelogs/unreleased/58405-basic-limiting-complexity-of-graphql-queries.yml new file mode 100644 index 00000000000..058a120b500 --- /dev/null +++ b/changelogs/unreleased/58405-basic-limiting-complexity-of-graphql-queries.yml @@ -0,0 +1,5 @@ +--- +title: Add initial complexity limits to GraphQL queries +merge_request: 26629 +author: +type: performance diff --git a/lib/gitlab/graphql/query_analyzers/log_query_complexity.rb b/lib/gitlab/graphql/query_analyzers/log_query_complexity.rb new file mode 100644 index 00000000000..95db130dbfc --- /dev/null +++ b/lib/gitlab/graphql/query_analyzers/log_query_complexity.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +module Gitlab + module Graphql + module QueryAnalyzers + class LogQueryComplexity + class << self + def analyzer + GraphQL::Analysis::QueryComplexity.new do |query, complexity| + # temporary until https://gitlab.com/gitlab-org/gitlab-ce/issues/59587 + Rails.logger.info("[GraphQL Query Complexity] #{complexity} | admin? #{query.context[:current_user]&.admin?}") + end + end + end + end + end + end +end diff --git a/spec/graphql/gitlab_schema_spec.rb b/spec/graphql/gitlab_schema_spec.rb index b9ddb427e85..a535d9cdc7e 100644 --- a/spec/graphql/gitlab_schema_spec.rb +++ b/spec/graphql/gitlab_schema_spec.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require 'spec_helper' describe GitlabSchema do @@ -31,6 +33,36 @@ describe GitlabSchema do expect(connection).to eq(Gitlab::Graphql::Connections::KeysetConnection) end + context 'for different types of users' do + it 'returns DEFAULT_MAX_COMPLEXITY for no user' do + expect(GraphQL::Schema).to receive(:execute).with('query', hash_including(max_complexity: GitlabSchema::DEFAULT_MAX_COMPLEXITY)) + + described_class.execute('query') + end + + it 'returns AUTHENTICATED_COMPLEXITY for a logged in user' do + user = build :user + + expect(GraphQL::Schema).to receive(:execute).with('query', hash_including(max_complexity: GitlabSchema::AUTHENTICATED_COMPLEXITY)) + + described_class.execute('query', context: { current_user: user }) + end + + it 'returns ADMIN_COMPLEXITY for an admin user' do + user = build :user, :admin + + expect(GraphQL::Schema).to receive(:execute).with('query', hash_including(max_complexity: GitlabSchema::ADMIN_COMPLEXITY)) + + described_class.execute('query', context: { current_user: user }) + end + + it 'returns what was passed on the query' do + expect(GraphQL::Schema).to receive(:execute).with('query', { max_complexity: 1234 }) + + described_class.execute('query', max_complexity: 1234) + end + end + def field_instrumenters described_class.instrumenters[:field] end diff --git a/spec/graphql/types/base_field_spec.rb b/spec/graphql/types/base_field_spec.rb new file mode 100644 index 00000000000..b5697ee5245 --- /dev/null +++ b/spec/graphql/types/base_field_spec.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Types::BaseField do + context 'when considering complexity' do + it 'defaults to 1' do + field = described_class.new(name: 'test', type: GraphQL::STRING_TYPE, null: true) + + expect(field.to_graphql.complexity).to eq 1 + end + + it 'has specified value' do + field = described_class.new(name: 'test', type: GraphQL::STRING_TYPE, null: true, complexity: 12) + + expect(field.to_graphql.complexity).to eq 12 + end + end +end diff --git a/spec/requests/api/graphql/gitlab_schema_spec.rb b/spec/requests/api/graphql/gitlab_schema_spec.rb new file mode 100644 index 00000000000..708a000532b --- /dev/null +++ b/spec/requests/api/graphql/gitlab_schema_spec.rb @@ -0,0 +1,16 @@ +require 'spec_helper' + +describe 'GitlabSchema configurations' do + include GraphqlHelpers + + let(:project) { create(:project, :repository) } + let!(:query) { graphql_query_for('project', 'fullPath' => project.full_path) } + + it 'shows an error if complexity it too high' do + allow(GitlabSchema).to receive(:max_query_complexity).and_return 1 + + post_graphql(query, current_user: nil) + + expect(graphql_errors.first['message']).to include('which exceeds max complexity of 1') + end +end diff --git a/spec/support/helpers/graphql_helpers.rb b/spec/support/helpers/graphql_helpers.rb index ca28325eab9..f59f42ee902 100644 --- a/spec/support/helpers/graphql_helpers.rb +++ b/spec/support/helpers/graphql_helpers.rb @@ -93,6 +93,8 @@ module GraphqlHelpers end def all_graphql_fields_for(class_name, parent_types = Set.new) + allow_unlimited_graphql_complexity + type = GitlabSchema.types[class_name.to_s] return "" unless type @@ -170,4 +172,10 @@ module GraphqlHelpers field_type end + + # for most tests, we want to allow unlimited complexity + def allow_unlimited_graphql_complexity + allow_any_instance_of(GitlabSchema).to receive(:max_complexity).and_return nil + allow(GitlabSchema).to receive(:max_query_complexity).with(any_args).and_return nil + end end -- cgit v1.2.1