Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[APPSEC-9343] Appsec support nested apps #2836

Merged
merged 3 commits into from
May 11, 2023

Conversation

GustavoCaso
Copy link
Member

@GustavoCaso GustavoCaso commented May 10, 2023

What does this PR do?

Fix #2722

I tested the code against a local Rails app that mounts a Sinatra App.

The configuration on the Rails app looks like this:

# config/initializers/datadog.rb
Datadog.configure do |c|
  c.tracing.instrument :rails, service_name: 'gustavo-nested-apps'
  c.appsec.enabled = true
  c.appsec.instrument :rails
  c.appsec.instrument :sinatra
end
# config/routes.rb

require_relative '../app/rack_apps/my_sinatra_app'

Rails.application.routes.draw do
  root "home#index"
  mount RackApps::MySinatraApp.new => '/api'
end

To ensure we are not re-executing the request middleware code multiple times with the context of nested apps, we need to make sure we bail out at the proper time.

That removes the 🐛 that some customers are experiencing, which raises Datadog::AppSec::Processor::AlreadyActiveContextError

Also, by making sure we continue the execution of the request without adding any extra calls to the WAF, we make sure to only calls the WAF the right amount of times 😄

Motivation

Additional Notes

I added a basic integration test to validate that things work as expected.

How to test the change?

@GustavoCaso GustavoCaso requested a review from a team May 10, 2023 15:27
@GustavoCaso GustavoCaso changed the title Appsec support nested apps [APPSEC-9343] Appsec support nested apps May 10, 2023
@github-actions github-actions bot added appsec Application Security monitoring product integrations Involves tracing integrations labels May 10, 2023
@GustavoCaso GustavoCaso force-pushed the appsec-support-nested-apps branch from 28f4e92 to 2dd0e45 Compare May 10, 2023 16:48
@GustavoCaso GustavoCaso requested a review from lloeki May 10, 2023 16:49
@GustavoCaso GustavoCaso force-pushed the appsec-support-nested-apps branch from 2dd0e45 to e86b164 Compare May 10, 2023 18:14
Copy link
Member

@lloeki lloeki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just a few notes about the comment

lib/datadog/appsec/contrib/rack/request_middleware.rb Outdated Show resolved Hide resolved
lib/datadog/appsec/contrib/rack/request_middleware.rb Outdated Show resolved Hide resolved
@GustavoCaso GustavoCaso force-pushed the appsec-support-nested-apps branch from 3dc4e92 to b240973 Compare May 11, 2023 17:27
@GustavoCaso GustavoCaso force-pushed the appsec-support-nested-apps branch from b240973 to 819f67f Compare May 11, 2023 17:31
@GustavoCaso GustavoCaso merged commit 5131c89 into master May 11, 2023
@GustavoCaso GustavoCaso deleted the appsec-support-nested-apps branch May 11, 2023 21:53
@github-actions github-actions bot added this to the 1.12.0 milestone May 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
appsec Application Security monitoring product integrations Involves tracing integrations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AppSec throws Datadog::AppSec::Processor::AlreadyActiveContextError with nested apps
2 participants