From 12dd9fa48db25790eddbbc03bff73245b55b980e Mon Sep 17 00:00:00 2001 From: Gustavo Caso Date: Wed, 10 May 2023 11:25:58 -0400 Subject: [PATCH 1/3] make sure not to activate the waf context multiple times for nested apps --- .../appsec/contrib/rack/request_middleware.rb | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/lib/datadog/appsec/contrib/rack/request_middleware.rb b/lib/datadog/appsec/contrib/rack/request_middleware.rb index 2c1f70379a5..24de8d440b9 100644 --- a/lib/datadog/appsec/contrib/rack/request_middleware.rb +++ b/lib/datadog/appsec/contrib/rack/request_middleware.rb @@ -23,7 +23,7 @@ def initialize(app, opt = {}) @oneshot_tags_sent = false end - # rubocop:disable Metrics/PerceivedComplexity,Metrics/CyclomaticComplexity,Metrics/MethodLength + # rubocop:disable Metrics/AbcSize,Metrics/PerceivedComplexity,Metrics/CyclomaticComplexity,Metrics/MethodLength def call(env) return @app.call(env) unless Datadog::AppSec.enabled? @@ -33,6 +33,21 @@ def call(env) ready = false context = nil + # If a customer has mounted a nested Rack app within their main app + # and instrumented boths apps: + # + # Datadog.configure do |c| + # c.appsec.enabled = true + # c.appsec.instrument :rails + # c.appsec.instrument :sinatra + # end + # + # We have to avoid calling processor.activate_context twice. + # To achive that we check the value of env['datadog.waf.context'] + # If the waf context is part of the env, it means that we are on the nested app + # context. In that case we need process the request without any new appsec related code. + return @app.call(env) if env['datadog.waf.context'] + Datadog::AppSec.reconfigure_lock do processor = Datadog::AppSec.processor @@ -88,7 +103,7 @@ def call(env) processor.deactivate_context end end - # rubocop:enable Metrics/PerceivedComplexity,Metrics/CyclomaticComplexity,Metrics/MethodLength + # rubocop:enable Metrics/AbcSize,Metrics/PerceivedComplexity,Metrics/CyclomaticComplexity,Metrics/MethodLength private From e86b1641586fc9508be012d680cda7c84049c3fd Mon Sep 17 00:00:00 2001 From: Gustavo Caso Date: Mon, 8 May 2023 17:53:00 -0400 Subject: [PATCH 2/3] Add integration test for nested apps --- .../contrib/rails/integration_test_spec.rb | 78 ++++++++++++++++++- 1 file changed, 77 insertions(+), 1 deletion(-) diff --git a/spec/datadog/appsec/contrib/rails/integration_test_spec.rb b/spec/datadog/appsec/contrib/rails/integration_test_spec.rb index fe3a575a595..7a26f0b64f5 100644 --- a/spec/datadog/appsec/contrib/rails/integration_test_spec.rb +++ b/spec/datadog/appsec/contrib/rails/integration_test_spec.rb @@ -32,6 +32,7 @@ let(:appsec_ip_denylist) { nil } let(:appsec_user_id_denylist) { nil } let(:appsec_ruleset) { :recommended } + let(:nested_app) { false } let(:crs_942_100) do { @@ -92,7 +93,7 @@ c.appsec.user_id_denylist = appsec_user_id_denylist c.appsec.ruleset = appsec_ruleset - # TODO: test with c.appsec.instrument :rack + c.appsec.instrument :rack if nested_app end end @@ -461,6 +462,81 @@ def set_user end end end + + describe 'Nested apps' do + let(:nested_app) { true } + let(:middlewares) do + [ + Datadog::Tracing::Contrib::Rack::TraceMiddleware, + Datadog::AppSec::Contrib::Rack::RequestMiddleware + ] + end + + let(:rack_app) do + app_middlewares = middlewares + + Rack::Builder.new do + app_middlewares.each { |m| use m } + map '/' do + run(proc { |_env| [200, { 'Content-Type' => 'text/html' }, ['OK']] }) + end + end.to_app + end + + let(:routes) do + { + [:mount, rack_app] => '/api', + } + end + + context 'GET request' do + subject(:response) { get url, params, env } + + let(:url) { '/api' } + let(:params) { {} } + let(:headers) { {} } + let(:env) { { 'REMOTE_ADDR' => remote_addr }.merge!(headers) } + + context 'with a non-event-triggering request' do + it { is_expected.to be_ok } + + it_behaves_like 'a GET 200 span' + it_behaves_like 'a trace with AppSec tags' + it_behaves_like 'a trace without AppSec events' + end + + context 'with an event-triggering request in headers' do + let(:headers) { { 'HTTP_USER_AGENT' => 'Nessus SOAP' } } + + it { is_expected.to be_ok } + it { expect(triggers).to be_a Array } + + it_behaves_like 'a GET 200 span' + it_behaves_like 'a trace with AppSec tags' + it_behaves_like 'a trace with AppSec events' + end + + context 'with an event-triggering request in query string' do + let(:params) { { q: '1 OR 1;' } } + + it { is_expected.to be_ok } + + it_behaves_like 'a GET 200 span' + it_behaves_like 'a trace with AppSec tags' + it_behaves_like 'a trace with AppSec events' + + context 'and a blocking rule' do + let(:appsec_ruleset) { crs_942_100 } + + it { is_expected.to be_forbidden } + + it_behaves_like 'a GET 403 span' + it_behaves_like 'a trace with AppSec tags' + it_behaves_like 'a trace with AppSec events' + end + end + end + end end end end From 819f67f0442f1f9f94af53efd352d6e9f2592941 Mon Sep 17 00:00:00 2001 From: Gustavo Caso Date: Thu, 11 May 2023 09:45:45 -0400 Subject: [PATCH 3/3] improve comment --- .../appsec/contrib/rack/request_middleware.rb | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/lib/datadog/appsec/contrib/rack/request_middleware.rb b/lib/datadog/appsec/contrib/rack/request_middleware.rb index 24de8d440b9..7b9866c0d45 100644 --- a/lib/datadog/appsec/contrib/rack/request_middleware.rb +++ b/lib/datadog/appsec/contrib/rack/request_middleware.rb @@ -33,19 +33,9 @@ def call(env) ready = false context = nil - # If a customer has mounted a nested Rack app within their main app - # and instrumented boths apps: - # - # Datadog.configure do |c| - # c.appsec.enabled = true - # c.appsec.instrument :rails - # c.appsec.instrument :sinatra - # end - # - # We have to avoid calling processor.activate_context twice. - # To achive that we check the value of env['datadog.waf.context'] - # If the waf context is part of the env, it means that we are on the nested app - # context. In that case we need process the request without any new appsec related code. + # For a given request, keep using the first Rack stack context for + # nested apps. Don't set `context` local variable so that on popping + # out of this nested stack we don't finalize the parent's context return @app.call(env) if env['datadog.waf.context'] Datadog::AppSec.reconfigure_lock do