From 93957a06c2c7e9dd6c8db9e5110ec4ea5e9aac55 Mon Sep 17 00:00:00 2001 From: Gustavo Caso Date: Fri, 10 Nov 2023 15:28:12 +0100 Subject: [PATCH] revert https://github.com/DataDog/dd-trace-rb/pull/3153 --- .gitlab/benchmarks.yml | 10 -- lib/datadog/appsec/configuration/settings.rb | 6 - .../appsec/contrib/rack/gateway/response.rb | 46 ------ .../appsec/contrib/rack/reactive/response.rb | 5 - .../appsec/contrib/rack/request_middleware.rb | 10 -- .../appsec/configuration/settings_spec.rb | 38 ----- .../contrib/rack/gateway/response_spec.rb | 84 ---------- .../contrib/rack/reactive/response_spec.rb | 143 ------------------ 8 files changed, 342 deletions(-) diff --git a/.gitlab/benchmarks.yml b/.gitlab/benchmarks.yml index 67a8f47e95a..388c51609c2 100644 --- a/.gitlab/benchmarks.yml +++ b/.gitlab/benchmarks.yml @@ -128,16 +128,6 @@ candidate-tracer-appsec-with-api-security: DD_EXPERIMENTAL_API_SECURITY_ENABLED: "true" DD_API_SECURITY_REQUEST_SAMPLE_RATE: "1.0" -candidate-tracer-appsec-with-api-security-without-response-body: - extends: .benchmarks - variables: - DD_BENCHMARKS_CONFIGURATION: "tracing-and-appsec-with-api-security-without-response-body" - DD_PROFILING_ENABLED: "false" - DD_APPSEC_ENABLED: "true" - DD_EXPERIMENTAL_API_SECURITY_ENABLED: "true" - DD_API_SECURITY_REQUEST_SAMPLE_RATE: "1.0" - DD_API_SECURITY_PARSE_RESPONSE_BODY: "false" - # ----------------------------------------------------- # Microbenchmarks that report to statsd # ----------------------------------------------------- diff --git a/lib/datadog/appsec/configuration/settings.rb b/lib/datadog/appsec/configuration/settings.rb index d814f60f4b8..77ef0940ca8 100644 --- a/lib/datadog/appsec/configuration/settings.rb +++ b/lib/datadog/appsec/configuration/settings.rb @@ -188,12 +188,6 @@ def self.add_settings!(base) end end end - - option :parse_response_body do |o| - o.type :bool - o.env 'DD_API_SECURITY_PARSE_RESPONSE_BODY' - o.default true - end end end end diff --git a/lib/datadog/appsec/contrib/rack/gateway/response.rb b/lib/datadog/appsec/contrib/rack/gateway/response.rb index b8b7c3bfea0..32b311bd919 100644 --- a/lib/datadog/appsec/contrib/rack/gateway/response.rb +++ b/lib/datadog/appsec/contrib/rack/gateway/response.rb @@ -19,55 +19,9 @@ def initialize(body, status, headers, scope:) @scope = scope end - def parsed_body - return unless Datadog.configuration.appsec.parse_response_body - - unless body.instance_of?(Array) - Datadog.logger.debug do - "Response body type unsupported: #{body.class}" - end - return - end - - return unless json_content_type? - - result = ''.dup - all_body_parts_are_string = true - - body.each do |body_part| - if body_part.is_a?(String) - result.concat(body_part) - else - all_body_parts_are_string = false - break - end - end - - return unless all_body_parts_are_string - - begin - JSON.parse(result) - rescue JSON::ParserError => e - Datadog.logger.debug { "Failed to parse response body. Error #{e.class}. Message #{e.message}" } - nil - end - end - def response @response ||= ::Rack::Response.new(body, status, headers) end - - private - - VALID_JSON_TYPES = [ - 'application/json', - 'text/json' - ].freeze - - def json_content_type? - content_type = headers['content-type'] - VALID_JSON_TYPES.include?(content_type) - end end end end diff --git a/lib/datadog/appsec/contrib/rack/reactive/response.rb b/lib/datadog/appsec/contrib/rack/reactive/response.rb index fedfcfae438..794f130d97d 100644 --- a/lib/datadog/appsec/contrib/rack/reactive/response.rb +++ b/lib/datadog/appsec/contrib/rack/reactive/response.rb @@ -10,7 +10,6 @@ module Response ADDRESSES = [ 'response.status', 'response.headers', - 'response.body', ].freeze private_constant :ADDRESSES @@ -18,7 +17,6 @@ def self.publish(op, gateway_response) catch(:block) do op.publish('response.status', gateway_response.status) op.publish('response.headers', gateway_response.headers) - op.publish('response.body', gateway_response.parsed_body) nil end @@ -31,7 +29,6 @@ def self.subscribe(op, waf_context) response_status = values[0] response_headers = values[1] response_headers_no_cookies = response_headers.dup.tap { |h| h.delete('set-cookie') } - response_body = values[2] waf_args = { 'server.response.status' => response_status.to_s, @@ -39,8 +36,6 @@ def self.subscribe(op, waf_context) 'server.response.headers.no_cookies' => response_headers_no_cookies, } - waf_args['server.response.body'] = response_body if response_body - waf_timeout = Datadog.configuration.appsec.waf_timeout result = waf_context.run(waf_args, waf_timeout) diff --git a/lib/datadog/appsec/contrib/rack/request_middleware.rb b/lib/datadog/appsec/contrib/rack/request_middleware.rb index e1214a26d47..f835cb2883b 100644 --- a/lib/datadog/appsec/contrib/rack/request_middleware.rb +++ b/lib/datadog/appsec/contrib/rack/request_middleware.rb @@ -66,16 +66,6 @@ def call(env) request_return = AppSec::Response.negotiate(env, blocked_event.last[:actions]).to_rack if blocked_event end - if request_return[2].respond_to?(:to_ary) - # Following the Rack specification. The response body should only call :each once. - # Calling :to_ary returns an array with identical content as the produced when calling :each - # replacing request_return[2] with that new value allow us to safely operate on the response body. - # On Gateway::Response#parsed_body we might iterate over the reposne body using :each - # https://github.com/rack/rack/blob/main/SPEC.rdoc#enumerable-body- - consumed_body = request_return[2].to_ary - request_return[2] = consumed_body if consumed_body - end - gateway_response = Gateway::Response.new( request_return[2], request_return[0], diff --git a/spec/datadog/appsec/configuration/settings_spec.rb b/spec/datadog/appsec/configuration/settings_spec.rb index 5f691d56101..39c1fd42cb2 100644 --- a/spec/datadog/appsec/configuration/settings_spec.rb +++ b/spec/datadog/appsec/configuration/settings_spec.rb @@ -711,43 +711,5 @@ def patcher end end end - - describe 'parse_response_body' do - subject(:enabled) { settings.appsec.parse_response_body } - - context 'when DD_API_SECURITY_PARSE_RESPONSE_BODY' do - around do |example| - ClimateControl.modify('DD_API_SECURITY_PARSE_RESPONSE_BODY' => api_security_parse_response_body) do - example.run - end - end - - context 'is not defined' do - let(:api_security_parse_response_body) { nil } - - it { is_expected.to eq true } - end - - context 'is defined' do - let(:api_security_parse_response_body) { 'true' } - - it { is_expected.to eq(true) } - end - end - end - - context 'parse_response_body=' do - subject(:set_parse_response_body) { settings.appsec.parse_response_body = parse_response_body } - - [true, false].each do |value| - context "when given #{value}" do - let(:parse_response_body) { value } - - before { set_parse_response_body } - - it { expect(settings.appsec.parse_response_body).to eq(value) } - end - end - end end end diff --git a/spec/datadog/appsec/contrib/rack/gateway/response_spec.rb b/spec/datadog/appsec/contrib/rack/gateway/response_spec.rb index e9341e70391..dc87c31be9d 100644 --- a/spec/datadog/appsec/contrib/rack/gateway/response_spec.rb +++ b/spec/datadog/appsec/contrib/rack/gateway/response_spec.rb @@ -42,88 +42,4 @@ expect(response.response).to be_a(Rack::Response) end end - - describe '#parsed_body' do - context 'json response' do - let(:content_type) { 'application/json' } - - context 'when parse_response_body is disable' do - around do |example| - ClimateControl.modify('DD_API_SECURITY_PARSE_RESPONSE_BODY' => 'false') do - example.run - end - end - - it 'returns a nil' do - expect(response.parsed_body).to be_nil - end - end - - context 'when parse_response_body is enabled' do - context 'all body parts are strings' do - let(:body) { ['{ "f', 'oo":', ' "ba', 'r" }'] } - - it 'returns a hash object' do - expect(response.parsed_body).to eq({ 'foo' => 'bar' }) - end - end - - context 'not all body parts are strings' do - let(:body_proc) { proc { ' "ba' } } - let(:body) { ['{ "f', 'oo":', body_proc, 'r" }'] } - - it 'returns nil' do - expect(response.parsed_body).to be_nil - end - end - - context 'fail to parse response body' do - let(:body) { [''] } - - it 'returns nil' do - expect(response.parsed_body).to be_nil - end - end - end - - context 'non supported response type' do - let(:content_type) { 'text/xml' } - - it 'returns nil' do - expect(response.parsed_body).to be_nil - end - end - - context 'without content-type header' do - let(:headers) { {} } - - it 'returns nil' do - expect(response.parsed_body).to be_nil - end - end - - context 'with a body that is not an Array' do - let(:body) { proc { ' "ba' } } - - it 'returns nil' do - expect(response.parsed_body).to be_nil - end - end - - context 'with a body that inherits from Array' do - let(:my_body_class) do - Class.new(Array) do - end - end - - let(:body) do - my_body_class.new - end - - it 'returns nil' do - expect(response.parsed_body).to be_nil - end - end - end - end end diff --git a/spec/datadog/appsec/contrib/rack/reactive/response_spec.rb b/spec/datadog/appsec/contrib/rack/reactive/response_spec.rb index 84bce918ef8..036fdbd1543 100644 --- a/spec/datadog/appsec/contrib/rack/reactive/response_spec.rb +++ b/spec/datadog/appsec/contrib/rack/reactive/response_spec.rb @@ -29,56 +29,8 @@ 'response.headers', headers, ) - expect(operation).to receive(:publish).with( - 'response.body', - nil, - ) - described_class.publish(operation, response) end - - context 'JSON response' do - let(:headers) { { 'content-type' => 'application/json', 'set-cookie' => 'foo' } } - let(:body) { ['{"a":"b"}'] } - - context 'when parsed_response_body is enabled' do - it 'propagates response attributes to the operation' do - expect(operation).to receive(:publish).with('response.status', 200) - expect(operation).to receive(:publish).with( - 'response.headers', - headers, - ) - expect(operation).to receive(:publish).with( - 'response.body', - { 'a' => 'b' }, - ) - - described_class.publish(operation, response) - end - end - - context 'when parsed_response_body is disabled' do - around do |example| - ClimateControl.modify('DD_API_SECURITY_PARSE_RESPONSE_BODY' => 'false') do - example.run - end - end - - it 'propagates response attributes to the operation' do - expect(operation).to receive(:publish).with('response.status', 200) - expect(operation).to receive(:publish).with( - 'response.headers', - headers, - ) - expect(operation).to receive(:publish).with( - 'response.body', - nil, - ) - - described_class.publish(operation, response) - end - end - end end describe '.subscribe' do @@ -87,7 +39,6 @@ expect(operation).to receive(:subscribe).with( 'response.status', 'response.headers', - 'response.body' ).and_call_original expect(processor_context).to_not receive(:run) described_class.subscribe(operation, processor_context) @@ -125,100 +76,6 @@ expect(result).to be_nil end end - - context 'JSON response' do - let(:headers) { { 'content-type' => 'application/json', 'set-cookie' => 'foo' } } - let(:body) { ['{"a":"b"}'] } - - context 'when parsed_response_body is enabled' do - context 'all addresses have been published' do - let(:expected_waf_arguments) do - { - 'server.response.status' => '200', - 'server.response.headers' => { - 'content-type' => 'application/json', - 'set-cookie' => 'foo', - }, - 'server.response.headers.no_cookies' => { - 'content-type' => 'application/json', - }, - 'server.response.body' => { 'a' => 'b' }, - } - end - - it 'does call the waf context with the right arguments' do - expect(processor_context).to receive(:run).with( - expected_waf_arguments, - Datadog.configuration.appsec.waf_timeout - ).and_return(waf_result) - described_class.subscribe(operation, processor_context) - result = described_class.publish(operation, response) - expect(result).to be_nil - end - end - - context 'when unparsable response body' do - # since the body is not all string values we bail out in Datadog::AppSec::Contrib::Rack::Gateway::Response - let(:body) do - [proc {}] - end - - let(:expected_waf_arguments) do - { - 'server.response.status' => '200', - 'server.response.headers' => { - 'content-type' => 'application/json', - 'set-cookie' => 'foo', - }, - 'server.response.headers.no_cookies' => { - 'content-type' => 'application/json', - } - } - end - - it 'does call the waf context with the right arguments' do - expect(processor_context).to receive(:run).with( - expected_waf_arguments, - Datadog.configuration.appsec.waf_timeout - ).and_return(waf_result) - described_class.subscribe(operation, processor_context) - result = described_class.publish(operation, response) - expect(result).to be_nil - end - end - end - - context 'when parsed_response_body is disabled' do - around do |example| - ClimateControl.modify('DD_API_SECURITY_PARSE_RESPONSE_BODY' => 'false') do - example.run - end - end - - let(:expected_waf_arguments) do - { - 'server.response.status' => '200', - 'server.response.headers' => { - 'content-type' => 'application/json', - 'set-cookie' => 'foo', - }, - 'server.response.headers.no_cookies' => { - 'content-type' => 'application/json', - }, - } - end - - it 'does call the waf context with the right arguments' do - expect(processor_context).to receive(:run).with( - expected_waf_arguments, - Datadog.configuration.appsec.waf_timeout - ).and_return(waf_result) - described_class.subscribe(operation, processor_context) - result = described_class.publish(operation, response) - expect(result).to be_nil - end - end - end end context 'waf result is a match' do