Query complexity check causes wrong GraphQL error response

Question

Query complexity check causes wrong GraphQL error response

mgruner opened this issue 5 months ago · 8 comments

mgruner commented 5 months ago

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.

Answer 1 · 2024-07-23T09:12:17.000Z

@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!

Answer 2 · 2024-07-23T19:48:58.000Z

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

Answer 3 · 2024-08-12T18:17:17.000Z

👋 I wrote a quick replication script for this issue:

Max complexity + loading non-existent records with ActiveRecord

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!

Answer 4 · 2024-08-19T19:46:13.000Z

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 👍

Answer 5 · 2024-08-20T05:00:21.000Z

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

Answer 6 · 2024-08-20T10:03:50.000Z

@rmosolgo I managed to create a reproduction script.

modified max complexity check script

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.

Answer 7 · 2024-08-20T14:29:56.000Z

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

Answer 8 · 2024-08-21T08:32:57.000Z

Thank you very much!