From f3034d4ca3de983a518cefc7d9786aedc4332919 Mon Sep 17 00:00:00 2001 From: Victor Pellan Date: Mon, 29 Jul 2024 11:22:15 +0200 Subject: [PATCH 1/5] Add directives support for graphql appsec --- .../contrib/graphql/gateway/multiplex.rb | 36 ++++++++++++------- 1 file changed, 23 insertions(+), 13 deletions(-) diff --git a/lib/datadog/appsec/contrib/graphql/gateway/multiplex.rb b/lib/datadog/appsec/contrib/graphql/gateway/multiplex.rb index 6f3a02e5c16..db38d14f7b2 100644 --- a/lib/datadog/appsec/contrib/graphql/gateway/multiplex.rb +++ b/lib/datadog/appsec/contrib/graphql/gateway/multiplex.rb @@ -32,29 +32,39 @@ def create_arguments_hash args = {} @multiplex.queries.each_with_index do |query, index| resolver_args = {} + resolver_dirs = {} selections = (query.selected_operation.selections.dup if query.selected_operation) || [] # Iterative tree traversal while selections.any? selection = selections.shift - if selection.arguments.any? - selection.arguments.each do |arg| - resolver_args[arg.name] = - if arg.value.is_a?(::GraphQL::Language::Nodes::VariableIdentifier) - query.provided_variables[arg.value.name] - else - arg.value - end - end + set_hash_with_variables(resolver_args, selection.arguments, query.provided_variables) + selection.directives.each do |dir| + resolver_dirs[dir.name] ||= {} + set_hash_with_variables(resolver_dirs[dir.name], dir.arguments, query.provided_variables) end selections.concat(selection.selections) end - unless resolver_args.empty? - args[query.operation_name || "query#{index + 1}"] ||= [] - args[query.operation_name || "query#{index + 1}"] << resolver_args - end + next if resolver_args.empty? && resolver_dirs.empty? + + args_resolver = (args[query.operation_name || "query#{index + 1}"] ||= []) + # We don't want to add empty hashes so we check again if the arguments and directives are empty + args_resolver << resolver_args unless resolver_args.empty? + args_resolver << resolver_dirs unless resolver_dirs.empty? end args end + + # Set the resolver hash (resolver_args and resolver_dirs) with the arguments and provided variables + def set_hash_with_variables(resolver_hash, arguments, provided_variables) + arguments.each do |arg| + resolver_hash[arg.name] = + if arg.value.is_a?(::GraphQL::Language::Nodes::VariableIdentifier) + provided_variables[arg.value.name] + else + arg.value + end + end + end end end end From ed85167e9f407047665135f7271c1c31ce1ea036 Mon Sep 17 00:00:00 2001 From: Victor Pellan Date: Mon, 29 Jul 2024 11:35:03 +0200 Subject: [PATCH 2/5] Add sig for set_hash_with_variables --- sig/datadog/appsec/contrib/graphql/gateway/multiplex.rbs | 2 ++ vendor/rbs/graphql/0/graphql.rbs | 3 +++ 2 files changed, 5 insertions(+) diff --git a/sig/datadog/appsec/contrib/graphql/gateway/multiplex.rbs b/sig/datadog/appsec/contrib/graphql/gateway/multiplex.rbs index 55e6c09e736..3465dcb1193 100644 --- a/sig/datadog/appsec/contrib/graphql/gateway/multiplex.rbs +++ b/sig/datadog/appsec/contrib/graphql/gateway/multiplex.rbs @@ -19,6 +19,8 @@ module Datadog private def create_arguments_hash: () -> Hash[String, Array[Hash[String, String]]] + + def set_hash_with_variables: (Hash[String, String] resolver_hash, Array[GraphQL::Language::Nodes::Argument] arguments, Hash[String, String|Integer] provided_variables) -> void end end end diff --git a/vendor/rbs/graphql/0/graphql.rbs b/vendor/rbs/graphql/0/graphql.rbs index 6eb0872de50..9b7e34f137d 100644 --- a/vendor/rbs/graphql/0/graphql.rbs +++ b/vendor/rbs/graphql/0/graphql.rbs @@ -22,6 +22,9 @@ module GraphQL class Field end + + class Argument + end end end From 98321bff3c267153393ddb1bf9c03fdab274d936 Mon Sep 17 00:00:00 2001 From: Victor Pellan Date: Mon, 29 Jul 2024 12:13:30 +0200 Subject: [PATCH 3/5] Reorganized test schema and added test directive --- .../contrib/graphql/appsec_trace_spec.rb | 8 +- .../contrib/graphql/integration_test_spec.rb | 2 +- .../contrib/graphql/support/application.rb | 17 +- .../graphql/support/application_schema.rb | 150 +++++++++++------- 4 files changed, 107 insertions(+), 70 deletions(-) diff --git a/spec/datadog/appsec/contrib/graphql/appsec_trace_spec.rb b/spec/datadog/appsec/contrib/graphql/appsec_trace_spec.rb index 391a46d3865..1d9175fa81b 100644 --- a/spec/datadog/appsec/contrib/graphql/appsec_trace_spec.rb +++ b/spec/datadog/appsec/contrib/graphql/appsec_trace_spec.rb @@ -28,10 +28,10 @@ expect(result.to_h['errors']).to eq( [ { - 'message' => "Field 'error' doesn't exist on type 'TestGraphQLQuery'", + 'message' => "Field 'error' doesn't exist on type 'Query'", 'locations' => [{ 'line' => 1, 'column' => 13 }], 'path' => ['query test', 'error'], - 'extensions' => { 'code' => 'undefinedField', 'typeName' => 'TestGraphQLQuery', 'fieldName' => 'error' } + 'extensions' => { 'code' => 'undefinedField', 'typeName' => 'Query', 'fieldName' => 'error' } } ] ) @@ -89,10 +89,10 @@ 'errors' => [ { - 'message' => "Field 'error' doesn't exist on type 'TestGraphQLQuery'", + 'message' => "Field 'error' doesn't exist on type 'Query'", 'locations' => [{ 'line' => 1, 'column' => 13 }], 'path' => ['query test', 'error'], - 'extensions' => { 'code' => 'undefinedField', 'typeName' => 'TestGraphQLQuery', 'fieldName' => 'error' } + 'extensions' => { 'code' => 'undefinedField', 'typeName' => 'Query', 'fieldName' => 'error' } } ] } diff --git a/spec/datadog/appsec/contrib/graphql/integration_test_spec.rb b/spec/datadog/appsec/contrib/graphql/integration_test_spec.rb index 76b46794488..d516dadf119 100644 --- a/spec/datadog/appsec/contrib/graphql/integration_test_spec.rb +++ b/spec/datadog/appsec/contrib/graphql/integration_test_spec.rb @@ -287,7 +287,7 @@ it do post '/graphql', query: 'mutation { createUser(name: "$testattack") { user { name, id } } }' expect(JSON.parse(last_response.body)['errors'][0]['title']).to eq('Blocked') - expect(Users.users['$testattack']).to be_nil + expect(TestGraphQL::Users.users['$testattack']).to be_nil end end end diff --git a/spec/datadog/tracing/contrib/graphql/support/application.rb b/spec/datadog/tracing/contrib/graphql/support/application.rb index 55390481e48..88ab68a45fc 100644 --- a/spec/datadog/tracing/contrib/graphql/support/application.rb +++ b/spec/datadog/tracing/contrib/graphql/support/application.rb @@ -31,15 +31,16 @@ # TODO: Cleaner way to reset the schema between tests (and most likely clean ::GraphQL::Schema too) # stub_const is required for GraphqlController, and we cannot use variables defined in let blocks in stub_const before do - Object.send(:remove_const, :TestGraphQLSchema) if defined?(TestGraphQLSchema) - Object.send(:remove_const, :TestGraphQLQuery) if defined?(TestGraphQLQuery) - Object.send(:remove_const, :TestGraphQLMutationType) if defined?(TestGraphQLMutationType) - Object.send(:remove_const, :Users) if defined?(Users) - Object.send(:remove_const, :TestUserType) if defined?(TestUserType) + TestGraphQL.send(:remove_const, :Case) if defined?(TestGraphQL::Case) + TestGraphQL.send(:remove_const, :Schema) if defined?(TestGraphQL::Schema) + TestGraphQL.send(:remove_const, :Query) if defined?(TestGraphQL::Query) + TestGraphQL.send(:remove_const, :MutationType) if defined?(TestGraphQL::MutationType) + TestGraphQL.send(:remove_const, :Users) if defined?(TestGraphQL::Users) + TestGraphQL.send(:remove_const, :UserType) if defined?(TestGraphQL::UserType) load 'spec/datadog/tracing/contrib/graphql/support/application_schema.rb' end let(:operation) { Datadog::AppSec::Reactive::Operation.new('test') } - let(:schema) { TestGraphQLSchema } + let(:schema) { TestGraphQL::Schema } end RSpec.shared_context 'with GraphQL multiplex' do @@ -94,9 +95,9 @@ def execute context: {} } end - TestGraphQLSchema.multiplex(queries) + TestGraphQL::Schema.multiplex(queries) else - TestGraphQLSchema.execute( + TestGraphQL::Schema.execute( query: params[:query], operation_name: params[:operationName], variables: prepare_variables(params[:variables]), diff --git a/spec/datadog/tracing/contrib/graphql/support/application_schema.rb b/spec/datadog/tracing/contrib/graphql/support/application_schema.rb index 23b76ebafeb..1805997047a 100644 --- a/spec/datadog/tracing/contrib/graphql/support/application_schema.rb +++ b/spec/datadog/tracing/contrib/graphql/support/application_schema.rb @@ -1,80 +1,116 @@ require 'graphql' require 'json' -class TestUserType < ::GraphQL::Schema::Object - field :id, ::GraphQL::Types::ID, null: false - field :name, ::GraphQL::Types::String, null: true - field :created_at, ::GraphQL::Types::String, null: false - field :updated_at, ::GraphQL::Types::String, null: false -end - -class TestGraphQLQuery < ::GraphQL::Schema::Object - field :user, TestUserType, null: false, description: 'Find an user by ID' do - argument :id, ::GraphQL::Types::ID, required: true - end - - def user(id:) - return OpenStruct.new(id: id, name: 'Caniche') if Integer(id) == 10 +module TestGraphQL + class Case < GraphQL::Schema::Directive + argument :format, String + locations FIELD - OpenStruct.new(id: id, name: 'Bits') + TRANSFORMS = [ + "upcase", + "downcase", + # ?? + ] + # Implement the Directive API + def self.resolve(object, arguments, context) + path = context.namespace(:interpreter)[:current_path] + return_value = yield + transform_name = arguments[:format] + if TRANSFORMS.include?(transform_name) && return_value.respond_to?(transform_name) + return_value = return_value.public_send(transform_name) + response = context.namespace(:interpreter_runtime)[:runtime].final_result + *keys, last = path + keys.each do |key| + if response && (response = response[key]) + next + else + break + end + end + if response + response[last] = return_value + end + nil + end + end end - field :userByName, TestUserType, null: false, description: 'Find an user by name' do - argument :name, ::GraphQL::Types::String, required: true + class UserType < ::GraphQL::Schema::Object + field :id, ::GraphQL::Types::ID, null: false + field :name, ::GraphQL::Types::String, null: true + field :created_at, ::GraphQL::Types::String, null: false + field :updated_at, ::GraphQL::Types::String, null: false end - # rubocop:disable Naming/MethodName - def userByName(name:) - return OpenStruct.new(id: 10, name: name) if name == 'Caniche' + class Query < ::GraphQL::Schema::Object + field :user, UserType, null: false, description: 'Find an user by ID' do + argument :id, ::GraphQL::Types::ID, required: true + end - OpenStruct.new(id: 1, name: name) - end + def user(id:) + return OpenStruct.new(id: id, name: 'Caniche') if Integer(id) == 10 - field :mutationUserByName, TestUserType, null: false, description: 'Find an user by name' do - argument :name, ::GraphQL::Types::String, required: true - end + OpenStruct.new(id: id, name: 'Bits') + end + + field :userByName, UserType, null: false, description: 'Find an user by name' do + argument :name, ::GraphQL::Types::String, required: true + end + + # rubocop:disable Naming/MethodName + def userByName(name:) + return OpenStruct.new(id: 10, name: name) if name == 'Caniche' - def mutationUserByName(name:) - if Users.users[name].nil? - ::GraphQL::ExecutionError.new('User not found') - else - OpenStruct.new(id: Users.users[name], name: name) + OpenStruct.new(id: 1, name: name) end + + field :mutationUserByName, UserType, null: false, description: 'Find an user by name' do + argument :name, ::GraphQL::Types::String, required: true + end + + def mutationUserByName(name:) + if Users.users[name].nil? + ::GraphQL::ExecutionError.new('User not found') + else + OpenStruct.new(id: Users.users[name], name: name) + end + end + # rubocop:enable Naming/MethodName end - # rubocop:enable Naming/MethodName -end -class Users - class << self - def users - @users ||= {} + class Users + class << self + def users + @users ||= {} + end end end -end -class TestGraphQLMutationType < ::GraphQL::Schema::Object - class TestGraphQLMutation < ::GraphQL::Schema::Mutation - argument :name, ::GraphQL::Types::String, required: true + class MutationType < ::GraphQL::Schema::Object + class Mutation < ::GraphQL::Schema::Mutation + argument :name, ::GraphQL::Types::String, required: true - field :user, TestUserType - field :errors, [String], null: false + field :user, UserType + field :errors, [String], null: false - def resolve(name:) - if Users.users.nil? || Users.users[name].nil? - Users.users ||= {} - item = OpenStruct.new(id: Users.users.size + 1, name: name) - Users.users[name] = Users.users.size + 1 - { user: item, errors: [] } - else - ::GraphQL::ExecutionError.new('User already exists') + def resolve(name:) + if Users.users.nil? || Users.users[name].nil? + Users.users ||= {} + item = OpenStruct.new(id: Users.users.size + 1, name: name) + Users.users[name] = Users.users.size + 1 + { user: item, errors: [] } + else + ::GraphQL::ExecutionError.new('User already exists') + end end end - end - field :create_user, mutation: TestGraphQLMutation -end + field :create_user, mutation: Mutation + end -class TestGraphQLSchema < ::GraphQL::Schema - mutation(TestGraphQLMutationType) - query(TestGraphQLQuery) -end + class Schema < ::GraphQL::Schema + mutation(MutationType) + query(Query) + directive(Case) + end +end \ No newline at end of file From 3d06435f5bc483d99871270f7f5aaa0dd8adc4ee Mon Sep 17 00:00:00 2001 From: Victor Pellan Date: Mon, 29 Jul 2024 12:15:42 +0200 Subject: [PATCH 4/5] Added integration test for directive --- .../contrib/graphql/integration_test_spec.rb | 113 ++++++++++++++++++ 1 file changed, 113 insertions(+) diff --git a/spec/datadog/appsec/contrib/graphql/integration_test_spec.rb b/spec/datadog/appsec/contrib/graphql/integration_test_spec.rb index d516dadf119..5209b92355a 100644 --- a/spec/datadog/appsec/contrib/graphql/integration_test_spec.rb +++ b/spec/datadog/appsec/contrib/graphql/integration_test_spec.rb @@ -459,5 +459,118 @@ end end end + + describe 'a query with directives' do + subject(:response) { post '/graphql', _json: queries } + + context 'with a non-triggering multiplex' do + let(:appsec_ruleset) { blocking_testattack } + let(:queries) do + [ + { + 'query' => 'query Test($format: String!) { user(id: 1) { name @case(format: $format) } }', + 'variables' => { 'format' => 'upcase' } + } + ] + end + + it do + expect(last_response.body).to eq( + [ + { 'data' => { 'user' => { 'name' => 'BITS' } } }, + ].to_json + ) + expect(spans).to include( + an_object_having_attributes( + name: 'graphql.parse', + ), + an_object_having_attributes( + name: 'graphql.execute_multiplex', + ), + an_object_having_attributes( + name: 'graphql.execute', + ) + ) + end + + it_behaves_like 'a POST 200 span' + it_behaves_like 'a trace with AppSec tags' + it_behaves_like 'a trace without AppSec events' + end + + context 'with a multiplex containing a non-blocking query' do + let(:appsec_ruleset) { nonblocking_testattack } + let(:queries) do + [ + { + 'query' => 'query Test($format: String!) { user(id: 1) { name @case(format: $format) } }', + 'variables' => { 'format' => '$testattack' } + } + ] + end + + it do + expect(last_response.body).to eq( + [ + { 'data' => { 'user' => { 'name' => 'Bits' } } } + ].to_json + ) + expect(spans).to include( + an_object_having_attributes( + name: 'graphql.parse', + ) + ).once + expect(spans).to include( + an_object_having_attributes( + name: 'graphql.execute_multiplex', + ) + ).once + expect(spans).to include( + an_object_having_attributes( + name: 'graphql.execute', + ) + ).once + end + + it_behaves_like 'a POST 200 span' + it_behaves_like 'a trace with AppSec tags' + it_behaves_like 'a trace with AppSec events' + end + + context 'with a multiplex containing a blocking query' do + let(:appsec_ruleset) { blocking_testattack } + let(:queries) do + [ + { + 'query' => 'query Test($format: String!) { user(id: 1) { name @case(format: $format) } }', + 'variables' => { 'format' => '$testattack' } + } + ] + end + + it do + expect(last_response.body).to eq( + [ + { 'errors' => [{ 'title' => 'Blocked', 'detail' => 'Security provided by Datadog.' }] } + ].to_json + ) + expect(spans).to include( + an_object_having_attributes( + name: 'graphql.parse', + ) + ).once + expect(spans).to include( + an_object_having_attributes( + name: 'graphql.execute_multiplex', + ) + ).once + expect(spans).not_to include( + an_object_having_attributes( + name: 'graphql.execute', + ) + ) + end + end + end end end From 7f64fcfc054b956a377235faae55134f4836a982 Mon Sep 17 00:00:00 2001 From: Victor Pellan Date: Mon, 29 Jul 2024 12:20:26 +0200 Subject: [PATCH 5/5] Fix Rubocop offenses on test directive --- .../contrib/graphql/support/application_schema.rb | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/spec/datadog/tracing/contrib/graphql/support/application_schema.rb b/spec/datadog/tracing/contrib/graphql/support/application_schema.rb index 1805997047a..ba297c1f77b 100644 --- a/spec/datadog/tracing/contrib/graphql/support/application_schema.rb +++ b/spec/datadog/tracing/contrib/graphql/support/application_schema.rb @@ -7,10 +7,10 @@ class Case < GraphQL::Schema::Directive locations FIELD TRANSFORMS = [ - "upcase", - "downcase", - # ?? - ] + 'upcase', + 'downcase', + ].freeze + # Implement the Directive API def self.resolve(object, arguments, context) path = context.namespace(:interpreter)[:current_path] @@ -27,9 +27,7 @@ def self.resolve(object, arguments, context) break end end - if response - response[last] = return_value - end + response[last] = return_value if response nil end end @@ -113,4 +111,4 @@ class Schema < ::GraphQL::Schema query(Query) directive(Case) end -end \ No newline at end of file +end