From 90bb7484a51fd344e524d49f04628e983d523459 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 25 Sep 2024 14:21:06 +0200 Subject: [PATCH 1/2] Add max_depth limit to GraphQL queries once again We accidentally removed this code in commit c984e666f. As mentioned in our GraphQL documentation, limiting the depth of the queries helps against DoS attacks. --- app/graphql/consul_schema.rb | 2 ++ spec/graphql/consul_schema_spec.rb | 37 ++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+) create mode 100644 spec/graphql/consul_schema_spec.rb diff --git a/app/graphql/consul_schema.rb b/app/graphql/consul_schema.rb index 3bd1e9ba9..e7a44581d 100644 --- a/app/graphql/consul_schema.rb +++ b/app/graphql/consul_schema.rb @@ -1,4 +1,6 @@ class ConsulSchema < GraphQL::Schema mutation(Types::MutationType) query(Types::QueryType) + + max_depth 8 end diff --git a/spec/graphql/consul_schema_spec.rb b/spec/graphql/consul_schema_spec.rb new file mode 100644 index 000000000..b0bfae536 --- /dev/null +++ b/spec/graphql/consul_schema_spec.rb @@ -0,0 +1,37 @@ +require "rails_helper" + +describe ConsulSchema do + let(:user) { create(:user) } + + it "returns an error for queries exceeding max depth" do + query = <<~GRAPHQL + { + user(id: #{user.id}) { + public_proposals { + edges { + node { + public_author { + username + public_proposals { + edges { + node { + public_author { + username + } + } + } + } + } + } + } + } + } + } + GRAPHQL + + response = execute(query) + + expect(response["errors"]).not_to be nil + expect(response["errors"].first["message"]).to match(/exceeds max depth/) + end +end From 5f80a75161e1ae26aa468a82293eb3d3e8a1cd0f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 25 Sep 2024 14:55:12 +0200 Subject: [PATCH 2/2] Limit GraphQL queries complexity once again We accidentally removed the code for maximum complexity in commit c984e666f. As mentioned in the documentation: > The main risk factor is multiple collections of resources being > requested in the same query. We reject these requests by limiting the complexity. The `max_complexity` option depends on the page size being set. Without it, we get an error: ``` Can't calculate complexity for User.public_debates, no `first:`, `last:`, `max_page_size` or `default_max_page_size` ``` So we're also adding a default max page size. Note that the documentation mentioned that the default page size was 25. However, before commit c984e666f, we were using a page size of 50 in some cases. We're going with the one mentioned in the documentation since we don't fully understand the old code. --- app/graphql/consul_schema.rb | 2 ++ spec/graphql/consul_schema_spec.rb | 39 ++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+) diff --git a/app/graphql/consul_schema.rb b/app/graphql/consul_schema.rb index e7a44581d..ecb40227f 100644 --- a/app/graphql/consul_schema.rb +++ b/app/graphql/consul_schema.rb @@ -2,5 +2,7 @@ class ConsulSchema < GraphQL::Schema mutation(Types::MutationType) query(Types::QueryType) + default_max_page_size 25 + max_complexity 2500 max_depth 8 end diff --git a/spec/graphql/consul_schema_spec.rb b/spec/graphql/consul_schema_spec.rb index b0bfae536..15c367c5c 100644 --- a/spec/graphql/consul_schema_spec.rb +++ b/spec/graphql/consul_schema_spec.rb @@ -34,4 +34,43 @@ describe ConsulSchema do expect(response["errors"]).not_to be nil expect(response["errors"].first["message"]).to match(/exceeds max depth/) end + + it "returns an error for queries requesting all records from more than 2 collections" do + query = <<~GRAPHQL + { + users { + edges { + node { + public_debates { + edges { + node { + title + } + } + } + public_proposals { + edges { + node { + title + } + } + } + public_comments { + edges { + node { + body + } + } + } + } + } + } + } + GRAPHQL + + response = execute(query) + + expect(response["errors"]).not_to be nil + expect(response["errors"].first["message"]).to match(/Query has complexity/) + end end