Query complexity check causes wrong GraphQL error response

[mgruner]

Describe the bug

In case of exceptions raised by input types, query complexity is calculated wrong, causing misleading error responses.

This is seems to be quite similar to #4666, but still happens.

Versions

graphql version: 2.3.10 rails (or other framework): other applicable versions (graphql-batch, etc)

GraphQL query

Example GraphQL query and response (if query execution is involved)

query ticketWithMentionLimit(
  $ticketId: ID
  $ticketInternalId: Int
  $ticketNumber: String
  $mentionsCount: Int = null
) {
  ticket(
    ticket: {
      ticketId: $ticketId
      ticketInternalId: $ticketInternalId
      ticketNumber: $ticketNumber
    }
  ) {
    ...ticketAttributes
    createArticleType {
      id
      name
    }
    mentions(first: $mentionsCount) {
      totalCount
      edges {
        node {
          ...ticketMention
        }
        cursor
      }
    }
  }
}

Expected behavior

When called for a nonexisting ticket, the query should return a proper error about the input validation failure.

Actual behavior

When called for a nonexisting ticket, there is a GraphQL error response about exceeded max query complexity, instead of the expected authorization error.

Additional context

We suspect a similar problem like #4666: input validation failure might cause the mentionsCount limit parameter to be ignored, falling back to the default page size, which in turn triggers the complexity error.

[mgruner]

@rmosolgo it looks like it is really the same problem as in #4666. There is one difference: this time, it's not a graphql authorization error, but instead simply a ActiveRecord::RecordNotFound that causes the input validation. Your change #4868 only looks for authorization errors though. But there could be other kinds of errors happening during the input validation phase which cause the complexity miscalculation as well. Thanks for your attention!

[rmosolgo]

Hey, thanks for all the details and sorry for the trouble. I'll take a look soon!

[rmosolgo]

👋 I wrote a quick replication script for this issue:

Max complexity + loading non-existent records with ActiveRecord

```ruby require "bundler/inline" gemfile do gem "graphql" gem "sqlite3", "~>1.4" gem "activerecord", require: "active_record" end ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:" ) ActiveRecord::Schema.define do create_table :tickets do |t| t.string(:subject) end end class Ticket < ActiveRecord::Base; end Ticket.create!(subject: "Thing doesn't work") class MySchema < GraphQL::Schema class Mention < GraphQL::Schema::Object field :content, String end class Ticket < GraphQL::Schema::Object field :subject, String field :mentions, Mention.connection_type def mentions = [] end class TicketInfo < GraphQL::Schema::InputObject argument :ticket_id, ID, loads: Ticket end class Query < GraphQL::Schema::Object field :ticket, Ticket do argument :ticket_info, TicketInfo end def ticket(ticket_info:) ticket_info[:ticket] end end max_complexity 10 query(Query) def self.object_from_id(id, ctx) ::Ticket.find(id) end def self.resolve_type(abs_t, obj, ctx) Ticket end end query_str = <<~GRAPHQL query($ticketId: ID!, $first: Int!) { ticket(ticketInfo: { ticketId: $ticketId }) { subject mentions(first: $first) { nodes { content } } } } GRAPHQL # over max complexity: pp MySchema.execute(query_str, variables: { ticketId: "1", first: 100 }).to_h # {"errors"=>[{"message"=>"Query has complexity of 104, which exceeds max complexity of 10"}]} pp MySchema.execute(query_str, variables: { ticketId: "2", first: 100 }).to_h # {"errors"=>[{"message"=>"Query has complexity of 104, which exceeds max complexity of 10"}]} # within max complexity: pp MySchema.execute(query_str, variables: { ticketId: "1", first: 1 }).to_h # {"data"=>{"ticket"=>{"subject"=>"Thing doesn't work", "mentions"=>{"nodes"=>[]}}}} pp MySchema.execute(query_str, variables: { ticketId: "2", first: 1 }).to_h # /Users/rmosolgo/.rbenv/versions/3.1.1/lib/ruby/gems/3.1.0/gems/activerecord-7.1.3.2/lib/active_record/core.rb:253:in `find': Couldn't find Ticket with 'id'=2 (ActiveRecord::RecordNotFound) # from complexity_test.rb:46:in `object_from_id' # from /Users/rmosolgo/.rbenv/versions/3.1.1/lib/ruby/gems/3.1.0/gems/graphql-2.3.4/lib/graphql/schema/member/has_arguments.rb:363:in `object_from_id' # from /Users/rmosolgo/.rbenv/versions/3.1.1/lib/ruby/gems/3.1.0/gems/graphql-2.3.4/lib/graphql/schema/member/has_arguments.rb:371:in `load_application_object' # from /Users/rmosolgo/.rbenv/versions/3.1.1/lib/ruby/gems/3.1.0/gems/graphql-2.3.4/lib/graphql/schema/member/has_arguments.rb:375:in `load_and_authorize_application_object' # from /Users/rmosolgo/.rbenv/versions/3.1.1/lib/ruby/gems/3.1.0/gems/graphql-2.3.4/lib/graphql/schema/argument.rb:332:in `load_and_authorize_value' # from /Users/rmosolgo/.rbenv/versions/3.1.1/lib/ruby/gems/3.1.0/gems/graphql-2.3.4/lib/graphql/schema/argument.rb:282:in `block in coerce_into_values' ```

To me, it looks like it's working as expected:

• When the incoming query exceeds max_complexity, it is rejected by the schema before any objects are loaded by ID. (This is because the complexity calculation doesn't require any loaded objects -- it doesn't make any DB calls.)
• When the incoming query is under max_complexity ...
  • if the record is found, a GraphQL result is returned
  • if loading the record raises ActiveRecord::NotFound, then that error propagates to the caller (crashes the query)

So, I don't know if/where any work is required in GraphQL-Ruby. Could you help me debug further? Here are some ways to investigate:

• Do you notice anything missing in my script, compared to your app?
• Can you confirm that ActiveRecord::NotFound is being raised, then silently rescued? For example, can you add a rescue to your def self.object_from_id method, then puts err.backtrace? That backtrace would really help me understand what's going on when the error is raised.
• Does your schema have any special handling for ActiveRecord::NotFound? It might be a schema rescue_from handler, or it might be custom logic in resolver code.
• In the example above, is the incoming query actually above the max_complexity? If you use the ID of a ticket that actually exists, does it still return a complexity error? If you keep using the invalid ID, but remove all the other fields from the query, what response do you get?

Let me know what you find!

[rmosolgo]

If you get a chance to look into those questions, please let me know what you find. I'll close it since I don't have any plans to investigate further until we get more info :+1:

[mgruner]

@rmosolgo I'm sorry for the delay. Just returned from vacation yesterday. Will analyze this and get back to you asap.

[mgruner]

@rmosolgo I managed to create a reproduction script.

modified max complexity check script

```ruby require 'bundler/inline' gemfile do gem 'graphql' gem 'sqlite3', '~>1.4' gem 'activerecord', require: 'active_record' end ActiveRecord::Base.establish_connection(adapter: 'sqlite3', database: ':memory:') ActiveRecord::Schema.define do create_table :mentions do |t| t.string(:content) t.belongs_to :ticket end create_table :tickets do |t| t.string(:subject) end end class Mention < ActiveRecord::Base; end class Ticket < ActiveRecord::Base has_many :mentions end ticket = Ticket.create!(subject: "Thing doesn't work") Mention.create!(content: "Thing doesn't work", ticket_id: ticket.id) class MySchema < GraphQL::Schema class Mention < GraphQL::Schema::Object field :content, String end class Ticket < GraphQL::Schema::Object field :subject, String field :mentions, Mention.connection_type def mentions = [] end class TicketInfo < GraphQL::Schema::InputObject argument :ticket_id, ID, loads: Ticket end class Query < GraphQL::Schema::Object field :mentions, Mention.connection_type do argument :ticket_info, TicketInfo end def mentions(ticket_info:) ticket_info.ticket.mentions end end default_page_size 50 max_complexity 10 query(Query) def self.object_from_id(id, ctx) ::Ticket.find(id) end def self.resolve_type(abs_t, obj, ctx) Ticket end RETHROWABLE_ERRORS = [GraphQL::ExecutionError, ArgumentError, IndexError, NameError, RangeError, RegexpError, SystemCallError, ThreadError, TypeError, ZeroDivisionError].freeze # Post-process errors and enrich them with meta information for processing on the client side. rescue_from(StandardError) do |err, _obj, _args, ctx, field| # Re-throw built-in errors that point to programming errors rather than problems with input or data - causes GraphQL processing to be aborted. RETHROWABLE_ERRORS.each do |klass| raise err if err.instance_of?(klass) end # We need to throw an ExecutionError, all others would cause the GraphQL processing to die. raise GraphQL::ExecutionError, err.message end end query_str = <<~GRAPHQL query queryOne($ticketId: ID!, $first: Int!) { mentions(ticketInfo: { ticketId: $ticketId }, first: $first) { nodes { content } } } GRAPHQL # over max complexity: pp MySchema.execute(query_str, variables: { ticketId: '1', first: 100 }).to_h # {"errors"=>[{"message"=>"Query has complexity of 104, which exceeds max complexity of 10"}]} pp MySchema.execute(query_str, variables: { ticketId: '2', first: 100 }).to_h # {"errors"=>[{"message"=>"Query has complexity of 52, which exceeds max complexity of 10"}]} # within max complexity: pp MySchema.execute(query_str, variables: { ticketId: '1', first: 1 }).to_h # {"data"=>{"mentions"=>{"nodes"=>[{"content"=>"Thing doesn't work"}]}}} pp MySchema.execute(query_str, variables: { ticketId: '2', first: 1 }).to_h # {"errors"=>[{"message"=>"Query has complexity of 52, which exceeds max complexity of 10"}]} ```

My initial query of the bug report was incorrect, sorry for that. It happens when a connection type has an input type that raises the ActiveRecord::RecordNotFound which is in turn wrapped in a GraphQL::ExecutionError by a rescue handler. Then GraphQL::Schema::Field#calculate_complexity will receive the error instead of the connection type field arguments, so that first / last are not present and cannot be considered. Then it falls back to the default page size, causing the complexity error.

Unfortunately I have no soution proposal, as modifying the line elsif arguments.is_a?(GraphQL::UnauthorizedError) to also consider GraphQL::ExecutionError does not seem to be a solution, as it causes the processing to interrupt with Exception.

Does this make sense? Thank you very much for considering.

[rmosolgo]

Hey, thanks for those details. I was able to replicate the issue and work up a patch in #5071

[mgruner]

Thank you very much!