diff --git a/.github/workflows/system-tests.yml b/.github/workflows/system-tests.yml index f7818927b2a..70f663ddfb0 100644 --- a/.github/workflows/system-tests.yml +++ b/.github/workflows/system-tests.yml @@ -11,7 +11,7 @@ on: env: REGISTRY: ghcr.io REPO: ghcr.io/datadog/dd-trace-rb - SYSTEM_TESTS_REF: main # This must always be set to `main` on dd-trace-rb's master branch + SYSTEM_TESTS_REF: appsec-remove-graphql-status-code-exception-for-ruby # This must always be set to `main` on dd-trace-rb's master branch jobs: build-harness: diff --git a/lib/datadog/appsec/actions_handler.rb b/lib/datadog/appsec/actions_handler.rb new file mode 100644 index 00000000000..e970dc73461 --- /dev/null +++ b/lib/datadog/appsec/actions_handler.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +module Datadog + module AppSec + # this module encapsulates functions for handling actions that libddawf returns + module ActionsHandler + module_function + + def handle(actions_hash) + # handle actions according their precedence + # stack and schema generation should be done before we throw an interrupt signal + generate_stack(actions_hash['generate_stack']) if actions_hash.key?('generate_stack') + generate_schema(actions_hash['generate_schema']) if actions_hash.key?('generate_schema') + redirect_request(actions_hash['redirect_request']) if actions_hash.key?('redirect_request') + block_request(actions_hash['block_request']) if actions_hash.key?('block_request') + end + + def block_request(action_params) + throw(Datadog::AppSec::Ext::INTERRUPT, action_params) + end + + def redirect_request(action_params) + throw(Datadog::AppSec::Ext::INTERRUPT, action_params) + end + + def generate_stack(_action_params); end + + def generate_schema(_action_params); end + + def monitor(_action_params); end + end + end +end diff --git a/lib/datadog/appsec/component.rb b/lib/datadog/appsec/component.rb index 7eb46adb141..7951989e0fa 100644 --- a/lib/datadog/appsec/component.rb +++ b/lib/datadog/appsec/component.rb @@ -3,6 +3,7 @@ require_relative 'processor' require_relative 'processor/rule_merger' require_relative 'processor/rule_loader' +require_relative 'actions_handler' module Datadog module AppSec diff --git a/lib/datadog/appsec/contrib/graphql/appsec_trace.rb b/lib/datadog/appsec/contrib/graphql/appsec_trace.rb index 72d74204265..4c9ea738af0 100644 --- a/lib/datadog/appsec/contrib/graphql/appsec_trace.rb +++ b/lib/datadog/appsec/contrib/graphql/appsec_trace.rb @@ -16,16 +16,10 @@ def execute_multiplex(multiplex:) gateway_multiplex = Gateway::Multiplex.new(multiplex) - multiplex_return, multiplex_response = Instrumentation.gateway.push('graphql.multiplex', gateway_multiplex) do + multiplex_return, = Instrumentation.gateway.push('graphql.multiplex', gateway_multiplex) do super end - # Returns an error * the number of queries so that the entire multiplex is blocked - if multiplex_response - blocked_event = multiplex_response.find { |action, _options| action == :block } - multiplex_return = AppSec::Response.graphql_response(gateway_multiplex) if blocked_event - end - multiplex_return end end diff --git a/lib/datadog/appsec/contrib/graphql/gateway/watcher.rb b/lib/datadog/appsec/contrib/graphql/gateway/watcher.rb index 3991a3b6669..d62a66fe966 100644 --- a/lib/datadog/appsec/contrib/graphql/gateway/watcher.rb +++ b/lib/datadog/appsec/contrib/graphql/gateway/watcher.rb @@ -22,7 +22,6 @@ def watch # This time we don't throw but use next def watch_multiplex(gateway = Instrumentation.gateway) gateway.watch('graphql.multiplex', :appsec) do |stack, gateway_multiplex| - block = false event = nil context = AppSec::Context.active engine = AppSec::Reactive::Engine.new @@ -39,13 +38,13 @@ def watch_multiplex(gateway = Instrumentation.gateway) Datadog::AppSec::Event.tag_and_keep!(context, result) context.events << event + + Datadog::AppSec::ActionsHandler.handle(result.actions) end - block = GraphQL::Reactive::Multiplex.publish(engine, gateway_multiplex) + GraphQL::Reactive::Multiplex.publish(engine, gateway_multiplex) end - next [nil, [[:block, event]]] if block - stack.call(gateway_multiplex.arguments) end end diff --git a/lib/datadog/appsec/contrib/rack/gateway/watcher.rb b/lib/datadog/appsec/contrib/rack/gateway/watcher.rb index f3ef9e46699..d5cec4b96b0 100644 --- a/lib/datadog/appsec/contrib/rack/gateway/watcher.rb +++ b/lib/datadog/appsec/contrib/rack/gateway/watcher.rb @@ -44,11 +44,12 @@ def watch_request(gateway = Instrumentation.gateway) context.trace.keep! if context.trace Datadog::AppSec::Event.tag_and_keep!(context, result) context.events << event + + Datadog::AppSec::ActionsHandler.handle(result.actions) end end - block = Rack::Reactive::Request.publish(engine, gateway_request) - throw(Datadog::AppSec::Ext::INTERRUPT, event[:actions]) if block + Rack::Reactive::Request.publish(engine, gateway_request) stack.call(gateway_request.request) end @@ -75,11 +76,12 @@ def watch_response(gateway = Instrumentation.gateway) context.trace.keep! if context.trace Datadog::AppSec::Event.tag_and_keep!(context, result) context.events << event + + Datadog::AppSec::ActionsHandler.handle(result.actions) end end - block = Rack::Reactive::Response.publish(engine, gateway_response) - throw(Datadog::AppSec::Ext::INTERRUPT, event[:actions]) if block + Rack::Reactive::Response.publish(engine, gateway_response) stack.call(gateway_response.response) end @@ -106,11 +108,12 @@ def watch_request_body(gateway = Instrumentation.gateway) context.trace.keep! if context.trace Datadog::AppSec::Event.tag_and_keep!(context, result) context.events << event + + Datadog::AppSec::ActionsHandler.handle(result.actions) end end - block = Rack::Reactive::RequestBody.publish(engine, gateway_request) - throw(Datadog::AppSec::Ext::INTERRUPT, event[:actions]) if block + Rack::Reactive::RequestBody.publish(engine, gateway_request) stack.call(gateway_request.request) end diff --git a/lib/datadog/appsec/contrib/rack/request_body_middleware.rb b/lib/datadog/appsec/contrib/rack/request_body_middleware.rb index 3159f228e79..cb45d16328e 100644 --- a/lib/datadog/appsec/contrib/rack/request_body_middleware.rb +++ b/lib/datadog/appsec/contrib/rack/request_body_middleware.rb @@ -24,7 +24,7 @@ def call(env) # TODO: handle exceptions, except for @app.call http_response = nil - block_actions = catch(::Datadog::AppSec::Ext::INTERRUPT) do + block_action_params = catch(::Datadog::AppSec::Ext::INTERRUPT) do http_response, = Instrumentation.gateway.push('rack.request.body', Gateway::Request.new(env)) do @app.call(env) end @@ -32,7 +32,7 @@ def call(env) nil end - return AppSec::Response.negotiate(env, block_actions).to_rack if block_actions + return AppSec::Response.build(block_action_params, env['HTTP_ACCEPT']).to_rack if block_action_params http_response end diff --git a/lib/datadog/appsec/contrib/rack/request_middleware.rb b/lib/datadog/appsec/contrib/rack/request_middleware.rb index 52cfcefc420..6d380422580 100644 --- a/lib/datadog/appsec/contrib/rack/request_middleware.rb +++ b/lib/datadog/appsec/contrib/rack/request_middleware.rb @@ -76,7 +76,7 @@ def call(env) gateway_request = Gateway::Request.new(env) gateway_response = nil - block_actions = catch(::Datadog::AppSec::Ext::INTERRUPT) do + block_action_params = catch(::Datadog::AppSec::Ext::INTERRUPT) do http_response, = Instrumentation.gateway.push('rack.request', gateway_request) do @app.call(env) end @@ -90,7 +90,7 @@ def call(env) nil end - http_response = AppSec::Response.negotiate(env, block_actions).to_rack if block_actions + http_response = AppSec::Response.build(block_action_params, env['HTTP_ACCEPT']).to_rack if block_action_params if AppSec.api_security_enabled? ctx.events << { diff --git a/lib/datadog/appsec/contrib/rails/gateway/watcher.rb b/lib/datadog/appsec/contrib/rails/gateway/watcher.rb index 33228b4bf9b..cd6a67f4253 100644 --- a/lib/datadog/appsec/contrib/rails/gateway/watcher.rb +++ b/lib/datadog/appsec/contrib/rails/gateway/watcher.rb @@ -40,11 +40,12 @@ def watch_request_action(gateway = Instrumentation.gateway) context.trace.keep! if context.trace Datadog::AppSec::Event.tag_and_keep!(context, result) context.events << event + + Datadog::AppSec::ActionsHandler.handle(result.actions) end end - block = Rails::Reactive::Action.publish(engine, gateway_request) - next [nil, [[:block, event]]] if block + Rails::Reactive::Action.publish(engine, gateway_request) stack.call(gateway_request.request) end diff --git a/lib/datadog/appsec/contrib/rails/patcher.rb b/lib/datadog/appsec/contrib/rails/patcher.rb index 479f65355d8..38f9a11dc7b 100644 --- a/lib/datadog/appsec/contrib/rails/patcher.rb +++ b/lib/datadog/appsec/contrib/rails/patcher.rb @@ -80,22 +80,12 @@ def process_action(*args) # TODO: handle exceptions, except for super gateway_request = Gateway::Request.new(request) - request_return, request_response = Instrumentation.gateway.push('rails.request.action', gateway_request) do - super - end - if request_response - blocked_event = request_response.find { |action, _options| action == :block } - if blocked_event - @_response = AppSec::Response.negotiate( - env, - blocked_event.last[:actions] - ).to_action_dispatch_response - request_return = @_response.body - end + http_response, = Instrumentation.gateway.push('rails.request.action', gateway_request) do + super end - request_return + http_response end end diff --git a/lib/datadog/appsec/contrib/sinatra/ext.rb b/lib/datadog/appsec/contrib/sinatra/ext.rb deleted file mode 100644 index 1002bcbb0b9..00000000000 --- a/lib/datadog/appsec/contrib/sinatra/ext.rb +++ /dev/null @@ -1,14 +0,0 @@ -# frozen_string_literal: true - -module Datadog - module AppSec - module Contrib - module Sinatra - # Sinatra integration constants - module Ext - ROUTE_INTERRUPT = :datadog_appsec_contrib_sinatra_route_interrupt - end - end - end - end -end diff --git a/lib/datadog/appsec/contrib/sinatra/gateway/watcher.rb b/lib/datadog/appsec/contrib/sinatra/gateway/watcher.rb index 90f7e669e9b..10f6eaf3bd5 100644 --- a/lib/datadog/appsec/contrib/sinatra/gateway/watcher.rb +++ b/lib/datadog/appsec/contrib/sinatra/gateway/watcher.rb @@ -42,11 +42,12 @@ def watch_request_dispatch(gateway = Instrumentation.gateway) context.trace.keep! if context.trace Datadog::AppSec::Event.tag_and_keep!(context, result) context.events << event + + Datadog::AppSec::ActionsHandler.handle(result.actions) end end - block = Rack::Reactive::RequestBody.publish(engine, gateway_request) - next [nil, [[:block, event]]] if block + Rack::Reactive::RequestBody.publish(engine, gateway_request) stack.call(gateway_request.request) end @@ -73,11 +74,12 @@ def watch_request_routed(gateway = Instrumentation.gateway) context.trace.keep! if context.trace Datadog::AppSec::Event.tag_and_keep!(context, result) context.events << event + + Datadog::AppSec::ActionsHandler.handle(result.actions) end end - block = Sinatra::Reactive::Routed.publish(engine, [gateway_request, gateway_route_params]) - next [nil, [[:block, event]]] if block + Sinatra::Reactive::Routed.publish(engine, [gateway_request, gateway_route_params]) stack.call(gateway_request.request) end diff --git a/lib/datadog/appsec/contrib/sinatra/patcher.rb b/lib/datadog/appsec/contrib/sinatra/patcher.rb index be4b4a73107..8b2efc56a5d 100644 --- a/lib/datadog/appsec/contrib/sinatra/patcher.rb +++ b/lib/datadog/appsec/contrib/sinatra/patcher.rb @@ -6,7 +6,6 @@ require_relative '../../response' require_relative '../rack/request_middleware' require_relative 'framework' -require_relative 'ext' require_relative 'gateway/watcher' require_relative 'gateway/route_params' require_relative 'gateway/request' @@ -62,17 +61,8 @@ def dispatch! gateway_request = Gateway::Request.new(env) - request_return, request_response = Instrumentation.gateway.push('sinatra.request.dispatch', gateway_request) do - # handle process_route interruption - catch(Datadog::AppSec::Contrib::Sinatra::Ext::ROUTE_INTERRUPT) { super } - end - - if request_response - blocked_event = request_response.find { |action, _options| action == :block } - if blocked_event - self.response = AppSec::Response.negotiate(env, blocked_event.last[:actions]).to_sinatra_response - request_return = nil - end + request_return, = Instrumentation.gateway.push('sinatra.request.dispatch', gateway_request) do + super end request_return @@ -103,20 +93,7 @@ def process_route(*) gateway_request = Gateway::Request.new(env) gateway_route_params = Gateway::RouteParams.new(route_params) - _, request_response = Instrumentation.gateway.push( - 'sinatra.request.routed', - [gateway_request, gateway_route_params] - ) - - if request_response - blocked_event = request_response.find { |action, _options| action == :block } - if blocked_event - self.response = AppSec::Response.negotiate(env, blocked_event.last[:actions]).to_sinatra_response - - # interrupt request and return response to dispatch! for consistency - throw(Datadog::AppSec::Contrib::Sinatra::Ext::ROUTE_INTERRUPT, response) - end - end + Instrumentation.gateway.push('sinatra.request.routed', [gateway_request, gateway_route_params]) yield(*args) end diff --git a/lib/datadog/appsec/monitor/gateway/watcher.rb b/lib/datadog/appsec/monitor/gateway/watcher.rb index 04595ffe52c..764ec88095a 100644 --- a/lib/datadog/appsec/monitor/gateway/watcher.rb +++ b/lib/datadog/appsec/monitor/gateway/watcher.rb @@ -38,11 +38,12 @@ def watch_user_id(gateway = Instrumentation.gateway) context.trace.keep! if context.trace Datadog::AppSec::Event.tag_and_keep!(context, result) context.events << event + + Datadog::AppSec::ActionsHandler.handle(result.actions) end end - block = Monitor::Reactive::SetUser.publish(engine, user) - throw(Datadog::AppSec::Ext::INTERRUPT, event[:actions]) if block + Monitor::Reactive::SetUser.publish(engine, user) stack.call(user) end diff --git a/lib/datadog/appsec/response.rb b/lib/datadog/appsec/response.rb index 8d63fd88622..b01b6a53842 100644 --- a/lib/datadog/appsec/response.rb +++ b/lib/datadog/appsec/response.rb @@ -19,100 +19,38 @@ def to_rack [status, headers, body] end - def to_sinatra_response - ::Sinatra::Response.new(body, status, headers) - end - - def to_action_dispatch_response - ::ActionDispatch::Response.new(status, headers, body) - end - class << self - def negotiate(env, actions) - # @type var configured_response: Response? - configured_response = nil - actions.each do |type, parameters| - # Need to use next to make steep happy :( - # I rather use break to stop the execution - next if configured_response - - configured_response = case type - when 'block_request' - block_response(env, parameters) - when 'redirect_request' - redirect_response(env, parameters) - end - end - - configured_response || default_response(env) - end - - def graphql_response(gateway_multiplex) - multiplex_return = [] - gateway_multiplex.queries.each do |query| - # This method is only called in places where GraphQL-Ruby is already required - query_result = ::GraphQL::Query::Result.new( - query: query, - values: JSON.parse(content('application/json')) - ) - multiplex_return << query_result - end + def build(action_params, http_accept_header) + return redirect_response(action_params) if action_params['location'] - multiplex_return + block_response(action_params, http_accept_header) end private - def default_response(env) - content_type = content_type(env) - - body = [] - body << content(content_type) + def block_response(action_params, http_accept_header) + content_type = case action_params['type'] + when nil, 'auto' then content_type(http_accept_header) + else FORMAT_TO_CONTENT_TYPE.fetch(action_params['type'], DEFAULT_CONTENT_TYPE) + end Response.new( - status: 403, + status: action_params['status_code']&.to_i || 403, headers: { 'Content-Type' => content_type }, - body: body, + body: [content(content_type)], ) end - def block_response(env, options) - content_type = if options['type'] == 'auto' - content_type(env) - else - FORMAT_TO_CONTENT_TYPE[options['type']] - end - - body = [] - body << content(content_type) + def redirect_response(action_params) + status_code = action_params['status_code'].to_i Response.new( - status: options['status_code']&.to_i || 403, - headers: { 'Content-Type' => content_type }, - body: body, + status: (status_code >= 300 && status_code < 400 ? status_code : 303), + headers: { 'Location' => action_params.fetch('location') }, + body: [], ) end - def redirect_response(env, options) - if options['location'] && !options['location'].empty? - content_type = content_type(env) - - headers = { - 'Content-Type' => content_type, - 'Location' => options['location'] - } - - status_code = options['status_code'].to_i - Response.new( - status: (status_code >= 300 && status_code < 400 ? status_code : 303), - headers: headers, - body: [], - ) - else - default_response(env) - end - end - CONTENT_TYPE_TO_FORMAT = { 'application/json' => :json, 'text/html' => :html, @@ -126,10 +64,10 @@ def redirect_response(env, options) DEFAULT_CONTENT_TYPE = 'application/json' - def content_type(env) - return DEFAULT_CONTENT_TYPE unless env.key?('HTTP_ACCEPT') + def content_type(http_accept_header) + return DEFAULT_CONTENT_TYPE if http_accept_header.nil? - accept_types = env['HTTP_ACCEPT'].split(',').map(&:strip) + accept_types = http_accept_header.split(',').map(&:strip) accepted = accept_types.map { |m| Utils::HTTP::MediaRange.new(m) }.sort!.reverse! diff --git a/sig/datadog/appsec/actions_handler.rbs b/sig/datadog/appsec/actions_handler.rbs new file mode 100644 index 00000000000..0e7535f43ad --- /dev/null +++ b/sig/datadog/appsec/actions_handler.rbs @@ -0,0 +1,17 @@ +module Datadog + module AppSec + module ActionsHandler + def handle: (::Hash[::String, ::Hash[::String, ::String]] actions_hash) -> void + + def block_request: (::Hash[::String, ::String] action_params) -> void + + def redirect_request: (::Hash[::String, ::String] action_params) -> void + + def generate_stack: (::Hash[::String, ::String] action_params) -> void + + def generate_schema: (::Hash[::String, ::String] action_params) -> void + + def monitor: (::Hash[::String, ::String] action_params) -> void + end + end +end diff --git a/sig/datadog/appsec/contrib/sinatra/ext.rbs b/sig/datadog/appsec/contrib/sinatra/ext.rbs deleted file mode 100644 index a7a1912c378..00000000000 --- a/sig/datadog/appsec/contrib/sinatra/ext.rbs +++ /dev/null @@ -1,13 +0,0 @@ -module Datadog - module AppSec - module Contrib - module Sinatra - module Ext - APP: "sinatra" - - ROUTE_INTERRUPT: :datadog_appsec_contrib_sinatra_route_interrupt - end - end - end - end -end diff --git a/sig/datadog/appsec/response.rbs b/sig/datadog/appsec/response.rbs index 3d00dedc5a9..6030ec882ac 100644 --- a/sig/datadog/appsec/response.rbs +++ b/sig/datadog/appsec/response.rbs @@ -8,11 +8,8 @@ module Datadog def initialize: (status: ::Integer, ?headers: ::Hash[::String, ::String], ?body: ::Array[::String]) -> void def to_rack: () -> ::Array[untyped] - def to_sinatra_response: () -> ::Sinatra::Response - def to_action_dispatch_response: () -> ::ActionDispatch::Response - def self.negotiate: (::Hash[untyped, untyped] env, ::Hash[String, untyped] actions) -> Response - def self.graphql_response: (Datadog::AppSec::Contrib::GraphQL::Gateway::Multiplex gateway_multiplex) -> Array[::GraphQL::Query::Result] + def self.build: (::Hash[::String, ::String] action_params, ::String http_accept_header) -> Response private @@ -20,11 +17,10 @@ module Datadog FORMAT_TO_CONTENT_TYPE: ::Hash[::String, ::String] DEFAULT_CONTENT_TYPE: ::String - def self.default_response: (::Hash[untyped, untyped] env) -> Response - def self.block_response: (::Hash[untyped, untyped] env, ::Hash[String, untyped] options) -> Response - def self.redirect_response: (::Hash[untyped, untyped] env, ::Hash[String, untyped] options) -> Response + def self.block_response: (::Hash[::String, ::String] action_params, ::String http_accept_header) -> Response + def self.redirect_response: (::Hash[::String, ::String] action_params) -> Response - def self.content_type: (::Hash[untyped, untyped] env) -> ::String + def self.content_type: (::String) -> ::String def self.content: (::String) -> ::String end end diff --git a/spec/datadog/appsec/actions_handler_spec.rb b/spec/datadog/appsec/actions_handler_spec.rb new file mode 100644 index 00000000000..3383c3db6d7 --- /dev/null +++ b/spec/datadog/appsec/actions_handler_spec.rb @@ -0,0 +1,107 @@ +# frozen_string_literal: true + +require 'datadog/appsec/spec_helper' + +RSpec.describe Datadog::AppSec::ActionsHandler do + describe '.handle' do + let(:generate_stack_action) do + { 'generate_stack' => { 'stack_id' => 'foo' } } + end + + let(:generate_schema_action) do + { 'generate_schema' => {} } + end + + let(:redirect_request_action) do + { 'redirect_request' => { 'status_code' => '303', 'location' => 'http://example.com' } } + end + + let(:block_request_action) do + { 'block_request' => { 'status_code' => '403', 'type' => 'auto' } } + end + + it 'calls generate_stack with action parameters' do + expect(described_class).to( + receive(:generate_stack).with(generate_stack_action['generate_stack']).and_call_original + ) + + described_class.handle(generate_stack_action) + end + + it 'calls generate_schema with action parameters' do + expect(described_class).to( + receive(:generate_schema).with(generate_schema_action['generate_schema']).and_call_original + ) + + described_class.handle('generate_schema' => generate_schema_action['generate_schema']) + end + + it 'calls redirect_request with action parameters' do + expect(described_class).to( + receive(:redirect_request).with(redirect_request_action['redirect_request']).and_call_original + ) + + catch(Datadog::AppSec::Ext::INTERRUPT) do + described_class.handle(redirect_request_action) + end + end + + it 'calls block_request with action parameters' do + expect(described_class).to( + receive(:block_request).with(block_request_action['block_request']).and_call_original + ) + + catch(Datadog::AppSec::Ext::INTERRUPT) do + described_class.handle(block_request_action) + end + end + + it 'calls redirect_request only when both block_request and redirect_request are present' do + expect(described_class).to( + receive(:redirect_request).with(redirect_request_action['redirect_request']).and_call_original + ) + expect(described_class).not_to receive(:block_request) + + catch(Datadog::AppSec::Ext::INTERRUPT) do + described_class.handle(block_request_action.merge(redirect_request_action)) + end + end + + it 'calls both generate_stack and generate_schema when both are present' do + expect(described_class).to( + receive(:generate_stack).with(generate_stack_action['generate_stack']).and_call_original + ) + expect(described_class).to( + receive(:generate_schema).with(generate_schema_action['generate_schema']).and_call_original + ) + + described_class.handle(generate_stack_action.merge(generate_schema_action)) + end + + it 'calls both generate_stack and block_request when both are present' do + expect(described_class).to( + receive(:generate_stack).with(generate_stack_action['generate_stack']).and_call_original + ) + expect(described_class).to( + receive(:block_request).with(block_request_action['block_request']).and_call_original + ) + + catch(Datadog::AppSec::Ext::INTERRUPT) do + described_class.handle(generate_stack_action.merge(block_request_action)) + end + end + + it 'calls both generate_stack and redirect_request when both are present' do + expect(described_class).to( + receive(:generate_stack).with(generate_stack_action['generate_stack']).and_call_original + ) + expect(described_class).to( + receive(:redirect_request).with(redirect_request_action['redirect_request']).and_call_original + ) + + catch(Datadog::AppSec::Ext::INTERRUPT) do + described_class.handle(generate_stack_action.merge(redirect_request_action)) + end + end + end +end diff --git a/spec/datadog/appsec/contrib/graphql/integration_test_spec.rb b/spec/datadog/appsec/contrib/graphql/integration_test_spec.rb index 5209b92355a..2ed9fad4430 100644 --- a/spec/datadog/appsec/contrib/graphql/integration_test_spec.rb +++ b/spec/datadog/appsec/contrib/graphql/integration_test_spec.rb @@ -220,10 +220,8 @@ let(:query) { '{ userByName(name: "$testattack") { id } }' } it do - expect(last_response.body).to eq( - { - 'errors' => [{ 'title' => 'Blocked', 'detail' => 'Security provided by Datadog.' }] - }.to_json + expect(JSON.parse(last_response.body)).to eq( + 'errors' => [{ 'title' => 'Blocked', 'detail' => 'Security provided by Datadog.' }] ) expect(spans).to include( an_object_having_attributes( @@ -240,8 +238,7 @@ ) end - # GraphQL errors should have no impact on the HTTP layer - it_behaves_like 'a POST 200 span' + it_behaves_like 'a POST 403 span' it_behaves_like 'a trace with AppSec tags' it_behaves_like 'a trace with AppSec events' end @@ -297,10 +294,8 @@ let(:mutation) { 'mutation { createUser(name: "$testattack") { user { name, id } } }' } it do - expect(last_response.body).to eq( - { - 'errors' => [{ 'title' => 'Blocked', 'detail' => 'Security provided by Datadog.' }] - }.to_json + expect(JSON.parse(last_response.body)).to eq( + 'errors' => [{ 'title' => 'Blocked', 'detail' => 'Security provided by Datadog.' }] ) expect(spans).to include( an_object_having_attributes( @@ -317,7 +312,7 @@ ) end - it_behaves_like 'a POST 200 span' + it_behaves_like 'a POST 403 span' it_behaves_like 'a trace with AppSec tags' it_behaves_like 'a trace with AppSec events' @@ -435,11 +430,8 @@ end it do - expect(last_response.body).to eq( - [ - { 'errors' => [{ 'title' => 'Blocked', 'detail' => 'Security provided by Datadog.' }] }, - { 'errors' => [{ 'title' => 'Blocked', 'detail' => 'Security provided by Datadog.' }] } - ].to_json + expect(JSON.parse(last_response.body)).to eq( + 'errors' => [{ 'title' => 'Blocked', 'detail' => 'Security provided by Datadog.' }] ) expect(spans).to include( an_object_having_attributes( @@ -549,10 +541,8 @@ end it do - expect(last_response.body).to eq( - [ - { 'errors' => [{ 'title' => 'Blocked', 'detail' => 'Security provided by Datadog.' }] } - ].to_json + expect(JSON.parse(last_response.body)).to eq( + 'errors' => [{ 'title' => 'Blocked', 'detail' => 'Security provided by Datadog.' }] ) expect(spans).to include( an_object_having_attributes( diff --git a/spec/datadog/appsec/response_spec.rb b/spec/datadog/appsec/response_spec.rb index 0872ae57060..2965ca89911 100644 --- a/spec/datadog/appsec/response_spec.rb +++ b/spec/datadog/appsec/response_spec.rb @@ -1,22 +1,15 @@ require 'datadog/appsec/response' RSpec.describe Datadog::AppSec::Response do - describe '.negotiate' do - let(:env) { double } + describe '.build' do + let(:http_accept_header) { 'text/html' } - before do - allow(env).to receive(:key?).with('HTTP_ACCEPT').and_return(true) - allow(env).to receive(:[]).with('HTTP_ACCEPT').and_return('text/html') - end - - describe 'configured actions' do + describe 'configured action_params' do describe 'block' do - let(:actions) do + let(:action_params) do { - 'block_request' => { - 'type' => type, - 'status_code' => status_code - } + 'type' => type, + 'status_code' => status_code } end @@ -24,7 +17,7 @@ let(:status_code) { '100' } context 'status_code' do - subject(:status) { described_class.negotiate(env, actions).status } + subject(:status) { described_class.build(action_params, http_accept_header).status } it { is_expected.to eq 100 } @@ -36,42 +29,34 @@ end context 'body' do - subject(:body) { described_class.negotiate(env, actions).body } + subject(:body) { described_class.build(action_params, http_accept_header).body } it { is_expected.to eq [Datadog::AppSec::Assets.blocked(format: :html)] } context 'type is auto it uses the HTTP_ACCEPT to decide the result' do let(:type) { 'auto' } - - before do - expect(env).to receive(:key?).with('HTTP_ACCEPT').and_return(true) - expect(env).to receive(:[]).with('HTTP_ACCEPT').and_return('application/json') - end + let(:http_accept_header) { 'application/json' } it { is_expected.to eq [Datadog::AppSec::Assets.blocked(format: :json)] } end end context 'headers' do - subject(:header) { described_class.negotiate(env, actions).headers['Content-Type'] } + subject(:header) { described_class.build(action_params, http_accept_header).headers['Content-Type'] } it { is_expected.to eq 'text/html' } context 'type is auto it uses the HTTP_ACCEPT to decide the result' do let(:type) { 'auto' } - - before do - expect(env).to receive(:key?).with('HTTP_ACCEPT').and_return(true) - expect(env).to receive(:[]).with('HTTP_ACCEPT').and_return('application/json') - end + let(:http_accept_header) { 'application/json' } it { is_expected.to eq 'application/json' } end end - context 'no configured actions' do - let(:actions) { {} } - subject(:response) { described_class.negotiate(env, actions) } + context 'empty action_params' do + let(:action_params) { {} } + subject(:response) { described_class.build(action_params, http_accept_header) } it 'uses default response' do expect(response.status).to eq 403 @@ -82,12 +67,10 @@ end describe 'redirect_request' do - let(:actions) do + let(:action_params) do { - 'redirect_request' => { - 'location' => location, - 'status_code' => status_code - } + 'location' => location, + 'status_code' => status_code } end @@ -95,7 +78,7 @@ let(:status_code) { '303' } context 'status_code' do - subject(:status) { described_class.negotiate(env, actions).status } + subject(:status) { described_class.build(action_params, http_accept_header).status } it { is_expected.to eq 303 } @@ -107,24 +90,13 @@ end context 'body' do - subject(:body) { described_class.negotiate(env, actions).body } + subject(:body) { described_class.build(action_params, http_accept_header).body } it { is_expected.to eq [] } end context 'headers' do - subject(:headers) { described_class.negotiate(env, actions).headers } - - context 'Content-Type' do - before do - expect(env).to receive(:key?).with('HTTP_ACCEPT').and_return(true) - expect(env).to receive(:[]).with('HTTP_ACCEPT').and_return('application/json') - end - - it 'uses the one from HTTP_ACCEPT header' do - expect(headers['Content-Type']).to eq('application/json') - end - end + subject(:headers) { described_class.build(action_params, http_accept_header).headers } context 'Location' do it 'uses the one from the configuration' do @@ -132,34 +104,17 @@ end end end - - context 'location is empty' do - let(:location) { '' } - - subject(:response) { described_class.negotiate(env, actions) } - - it 'uses default response' do - expect(response.status).to eq 403 - expect(response.body).to eq [Datadog::AppSec::Assets.blocked(format: :html)] - expect(response.headers['Content-Type']).to eq 'text/html' - end - end end end describe '.status' do - subject(:status) { described_class.negotiate(env, {}).status } + subject(:status) { described_class.build({}, http_accept_header).status } it { is_expected.to eq 403 } end describe '.body' do - subject(:body) { described_class.negotiate(env, {}).body } - - before do - expect(env).to receive(:key?).with('HTTP_ACCEPT').and_return(true) - expect(env).to receive(:[]).with('HTTP_ACCEPT').and_return(accept) - end + subject(:body) { described_class.build({}, http_accept_header).body } shared_examples_for 'with custom response body' do |type| before do @@ -176,13 +131,13 @@ end context 'with unsupported Accept headers' do - let(:accept) { 'application/xml' } + let(:http_accept_header) { 'application/xml' } it { is_expected.to eq [Datadog::AppSec::Assets.blocked(format: :json)] } end context('with Accept: text/html') do - let(:accept) { 'text/html' } + let(:http_accept_header) { 'text/html' } it { is_expected.to eq [Datadog::AppSec::Assets.blocked(format: :html)] } @@ -190,7 +145,7 @@ end context('with Accept: application/json') do - let(:accept) { 'application/json' } + let(:http_accept_header) { 'application/json' } it { is_expected.to eq [Datadog::AppSec::Assets.blocked(format: :json)] } @@ -198,7 +153,7 @@ end context('with Accept: text/plain') do - let(:accept) { 'text/plain' } + let(:http_accept_header) { 'text/plain' } it { is_expected.to eq [Datadog::AppSec::Assets.blocked(format: :text)] } @@ -207,90 +162,82 @@ end describe ".headers['Content-Type']" do - subject(:content_type) { described_class.negotiate(env, {}).headers['Content-Type'] } - - before do - expect(env).to receive(:key?).with('HTTP_ACCEPT').and_return(respond_to?(:accept)) - - if respond_to?(:accept) - expect(env).to receive(:[]).with('HTTP_ACCEPT').and_return(accept) - else - expect(env).to_not receive(:[]).with('HTTP_ACCEPT') - end - end + subject(:content_type) { described_class.build({}, http_accept_header).headers['Content-Type'] } context('with Accept: text/html') do - let(:accept) { 'text/html' } + let(:http_accept_header) { 'text/html' } - it { is_expected.to eq accept } + it { is_expected.to eq http_accept_header } end context('with Accept: application/json') do - let(:accept) { 'application/json' } + let(:http_accept_header) { 'application/json' } - it { is_expected.to eq accept } + it { is_expected.to eq http_accept_header } end context('with Accept: text/plain') do - let(:accept) { 'text/plain' } + let(:http_accept_header) { 'text/plain' } - it { is_expected.to eq accept } + it { is_expected.to eq http_accept_header } end context('without Accept header') do + let(:http_accept_header) { nil } + it { is_expected.to eq 'application/json' } end context('with Accept: */*') do - let(:accept) { '*/*' } + let(:http_accept_header) { '*/*' } it { is_expected.to eq 'application/json' } end context('with Accept: text/*') do - let(:accept) { 'text/*' } + let(:http_accept_header) { 'text/*' } it { is_expected.to eq 'text/html' } end context('with Accept: application/*') do - let(:accept) { 'application/*' } + let(:http_accept_header) { 'application/*' } it { is_expected.to eq 'application/json' } end context('with unparseable Accept header') do - let(:accept) { 'invalid' } + let(:http_accept_header) { 'invalid' } it { is_expected.to eq 'application/json' } end context('with Accept: text/*;q=0.7, application/*;q=0.8, */*;q=0.9') do - let(:accept) { 'text/*;q=0.7, application/*;q=0.8, */*;q=0.9' } + let(:http_accept_header) { 'text/*;q=0.7, application/*;q=0.8, */*;q=0.9' } it { is_expected.to eq 'application/json' } end context('with unsupported Accept header') do - let(:accept) { 'image/webp' } + let(:http_accept_header) { 'image/webp' } it { is_expected.to eq 'application/json' } end context('with Mozilla Firefox Accept') do - let(:accept) { 'text/html,application/xhtml+xml,application/xml;q=0.9,image/avif,image/webp,*/*;q=0.8' } + let(:http_accept_header) { 'text/html,application/xhtml+xml,application/xml;q=0.9,image/avif,image/webp,*/*;q=0.8' } it { is_expected.to eq 'text/html' } end context('with Google Chrome Accept') do - let(:accept) { 'text/html,application/xhtml+xml,application/xml;q=0.9,image/avif,image/webp,image/apng,*/*;q=0.8,application/signed-exchange;v=b3;q=0.7' } # rubocop:disable Layout/LineLength + let(:http_accept_header) { 'text/html,application/xhtml+xml,application/xml;q=0.9,image/avif,image/webp,image/apng,*/*;q=0.8,application/signed-exchange;v=b3;q=0.7' } # rubocop:disable Layout/LineLength it { is_expected.to eq 'text/html' } end context('with Apple Safari Accept') do - let(:accept) { 'text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8' } + let(:http_accept_header) { 'text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8' } it { is_expected.to eq 'text/html' } end