Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions lib/datadog/appsec/api_security/route_extractor.rb
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,9 @@ def self.route_pattern(request)
elsif request.env.key?(RAILS_ROUTE_KEY)
request.env[RAILS_ROUTE_KEY].delete_suffix(RAILS_FORMAT_SUFFIX)
elsif request.env.key?(RAILS_ROUTES_KEY) && !request.env.fetch(RAILS_PATH_PARAMS_KEY, {}).empty?
pattern = request.env[RAILS_ROUTES_KEY].router
.recognize(request) { |route, _| break route.path.spec.to_s }
route_set = request.env[RAILS_ROUTES_KEY]
rails_request = route_set.request_class.new(request.env)
Copy link
Member

Choose a reason for hiding this comment

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

Hey @marcotc, thanks for taking care of that. We don't have grape contrib, but there was a plan to start supporting it as main API framework.

The init of the router will generate new instance with unprocessed env, could you please explain what is the actual issue here of using stored router?

Copy link
Member Author

Choose a reason for hiding this comment

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

The main issue is that recognize(request) expects a wrapped ActionPack request, not a raw Rack request, and it throws the error undefined method request_method= when running.

Thankfully this is easy to reproduce in tests (which are being added).

Tbh, I only really have to fix the Rails issue today, but I noticed that the Grape and Sinatra paths were not tested in integration tests (which the the only way to trigger the Rails failure today), so I started adding them in this PR as well.

Still WIP though!

Copy link
Member

Choose a reason for hiding this comment

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

🤔 Hmmm, we have tested it with Rails and we decide to scope out Sinatra to system tests, but both working. In which case it do you get that error?

If it happens solely in tests like an old "integration" test we poses, than I would challenge the quality of the test.

We also have a test app where I run it across different Rails app versions.

Copy link
Member

@Strech Strech Sep 3, 2025

Choose a reason for hiding this comment

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

BTW, here are integrations tests that will use route extraction (but not on all Rails versions)

If you don't mind could you share the error case you are fixing? I would love to help on this.

Copy link
Member

Choose a reason for hiding this comment

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

I think I get the issue now, problem starts not because sampling window, but because the HEAD request. It will take a path into the router which will assign the method into it to be able to find it across the available routes.

Rails will try to modify the request in order to find it.

As a suggestion, to avoid this expensive path, could we wrap the request only for this specific scenario of HEAD request then?

pattern = route_set.router.recognize(rails_request) { |route, _| break route.path.spec.to_s }

# NOTE: If rails is unable to recognize request it returns empty Array
pattern = nil if pattern&.empty?
Expand Down
15 changes: 12 additions & 3 deletions spec/datadog/appsec/api_security/route_extractor_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -96,15 +96,24 @@
before do
allow(request).to receive(:env).and_return({
'action_dispatch.routes' => route_set,
'action_dispatch.request.path_parameters' => {}
'action_dispatch.request.path_parameters' => {'controller' => 'users', 'action' => 'show', 'id' => '1'}
})

allow(router).to receive(:recognize) do |rails_request, &blk|
blk.call(route, {}) if blk
end
end

let(:router) { double('ActionDispatch::Routing::RouteSet::Router') }
let(:route_set) { double('ActionDispatch::Routing::RouteSet', router: router) }
let(:route) { double('ActionDispatch::Journey::Route', path: double(spec: '/users/:id(.:format)')) }
let(:request_class) { double('ActionDispatch::RequestClass', new: rails_request) }
let(:route_set) { double('ActionDispatch::Routing::RouteSet', router: router, request_class: request_class) }
let(:rails_request) { double('ActionDispatch::Request') }
let(:request) { double('Rack::Request', env: {}, script_name: '', path: '/users/1') }

it { expect(described_class.route_pattern(request)).to eq('/users/1') }
it 'uses router recognition and strips format suffix' do
expect(described_class.route_pattern(request)).to eq('/users/:id')
end
end

context 'when Rails router cannot recognize request' do
Expand Down
115 changes: 115 additions & 0 deletions spec/datadog/appsec/contrib/grape/integration_test_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
require 'datadog/tracing/contrib/support/spec_helper'
require 'rack/test'

require 'datadog/tracing'
require 'datadog/appsec'

RSpec.describe 'Grape integration tests' do
include Rack::Test::Methods

let(:sorted_spans) do
chain = lambda do |start|
loop.with_object([start]) do |_, o|
break o if o.last.parent_id == 0

parent = spans.find { |span| span.id == o.last.parent_id }
break o if parent.nil?

o << parent
end
end
sort = ->(list) { list.sort_by { |e| chain.call(e).count } }
sort.call(spans)
end

let(:rack_span) { sorted_spans.reverse.find { |x| x.name == Datadog::Tracing::Contrib::Rack::Ext::SPAN_REQUEST } }

let(:tracing_enabled) { true }
let(:appsec_enabled) { true }

let(:api_security_enabled) { false }
let(:api_security_sample) { 0 }

before do
Datadog.configure do |c|
c.tracing.enabled = tracing_enabled
c.tracing.instrument :rack

c.appsec.enabled = appsec_enabled
c.appsec.instrument :rack

c.appsec.api_security.enabled = api_security_enabled
c.appsec.api_security.sample_delay = api_security_sample.to_i
end

allow_any_instance_of(Datadog::Tracing::Transport::HTTP::Client).to receive(:send_request)
allow_any_instance_of(Datadog::Tracing::Transport::Traces::Transport).to receive(:native_events_supported?)
.and_return(true)
end

after do
Datadog.configuration.reset!
Datadog.registry[:rack].reset_configuration!
end

context 'for a mounted Grape API' do
let(:middlewares) do
[
Datadog::Tracing::Contrib::Rack::TraceMiddleware,
Datadog::AppSec::Contrib::Rack::RequestMiddleware
]
end

let(:app) do
skip 'grape gem not available' unless Gem.loaded_specs['grape']
require 'grape'

api_class = Class.new(Grape::API) do
format :json
get('/users/:id') { { ok: true } }
end

app_middlewares = middlewares

Rack::Builder.new do
app_middlewares.each { |m| use m }
map '/' do
run api_class
end
end.to_app
end

let(:service_span) do
sorted_spans.reverse.find { |s| s.metrics.fetch('_dd.top_level', -1.0) > 0.0 }
end

let(:span) { rack_span }
let(:remote_addr) { '127.0.0.1' }

describe 'with sample_delay' do
subject(:response) { get url, params, env }

let(:api_security_enabled) { true }
let(:api_security_sample) { 30 }

let(:url) { '/users/123' }
let(:params) { {} }
let(:headers) { {} }
let(:env) { { 'REMOTE_ADDR' => remote_addr }.merge!(headers) }

it 'samples and caches check result' do
get url, params, env
first_span = spans.find { |s| s.name == 'rack.request' }
expect(first_span.tags).to have_key('_dd.appsec.s.req.headers')

clear_traces!

get url, params, env
second_span = spans.find { |s| s.name == 'rack.request' }
expect(second_span.tags).not_to have_key('_dd.appsec.s.req.headers')
end
end
end
end


141 changes: 141 additions & 0 deletions spec/datadog/appsec/contrib/rails/integration_test_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -467,6 +467,38 @@ def set_user
end
end

describe 'with sample_delay' do
subject(:response) { head url, params, env }

let(:api_security_enabled) { true }
let(:api_security_sample) { 30 } # Default value

let(:routes) do
{
'/items/:id' => 'test#success'
}
end

let(:url) { '/items/123' }
let(:params) { {} }
let(:headers) { {} }
let(:env) { { 'REMOTE_ADDR' => remote_addr }.merge!(headers) }

it 'samples and caches check result' do
# First HEAD request: expect schema extraction tags present
head url, params, env
first_span = spans.find { |s| s.name == 'rack.request' }
expect(first_span.tags).to have_key('_dd.appsec.s.req.headers')

clear_traces!

# Second identical HEAD request within delay window: expect schema tags absent
head url, params, env
second_span = spans.find { |s| s.name == 'rack.request' }
expect(second_span.tags).not_to have_key('_dd.appsec.s.req.headers')
end
end

describe 'Nested apps' do
let(:appsec_instrument_rack) { true }
let(:middlewares) do
Expand Down Expand Up @@ -545,6 +577,115 @@ def set_user
end
end
end

describe 'Sinatra mounted app' do
subject(:response) { nil }

let(:api_security_enabled) { true }
let(:api_security_sample) { 30 }

let(:sinatra_app) do
stub_const(
'SinatraMountedApp',
Class.new(Sinatra::Base) do
get('/users/:id') { '' }
get('/projects/:id') { '' }
end
)
end

before do
skip 'sinatra gem not available' unless Gem.loaded_specs['sinatra']
require 'sinatra/base'

app_class = sinatra_app
rails_test_application.instance.routes.append do
mount app_class => '/sin'
end
end

it 'uses route pattern for sampling key (suppresses only same route)' do
allow_any_instance_of(Datadog::Tracing::TraceOperation).to receive(:priority_sampled?).and_return(true)

# Freeze time to make sampling deterministic
t = Time.at(100)
allow(Datadog::Core::Utils::Time).to receive(:now).and_return(t)

decisions = []
allow_any_instance_of(Datadog::AppSec::APISecurity::Sampler).to receive(:sample?).and_wrap_original do |m, req, res|
result = m.call(req, res)
decisions << result
result
end

get '/sin/users/1', {}, {'REMOTE_ADDR' => remote_addr}
clear_traces!

# Same framework route pattern => suppressed within delay
get '/sin/users/2', {}, {'REMOTE_ADDR' => remote_addr}
clear_traces!

# Different framework route pattern => sampled despite delay
get '/sin/projects/1', {}, {'REMOTE_ADDR' => remote_addr}

expect(decisions).to eq([true, false, true])
end
end

describe 'Grape mounted app' do
subject(:response) { nil }

let(:api_security_enabled) { true }
let(:api_security_sample) { 30 }

let(:grape_api) do
stub_const(
'GrapeMountedApi',
Class.new(Grape::API) do
format :json
get('/users/:id') { { ok: true } }
get('/projects/:id') { { ok: true } }
end
)
end

before do
skip 'grape gem not available' unless Gem.loaded_specs['grape']
require 'grape'

api_class = grape_api
rails_test_application.instance.routes.append do
mount api_class => '/grape'
end
end

it 'uses route pattern for sampling key (suppresses only same route)' do
allow_any_instance_of(Datadog::Tracing::TraceOperation).to receive(:priority_sampled?).and_return(true)

# Freeze time to make sampling deterministic
t = Time.at(100)
allow(Datadog::Core::Utils::Time).to receive(:now).and_return(t)

decisions = []
allow_any_instance_of(Datadog::AppSec::APISecurity::Sampler).to receive(:sample?).and_wrap_original do |m, req, res|
result = m.call(req, res)
decisions << result
result
end

get '/grape/users/1', {}, {'REMOTE_ADDR' => remote_addr}
clear_traces!

# Same framework route pattern => suppressed within delay
get '/grape/users/2', {}, {'REMOTE_ADDR' => remote_addr}
clear_traces!

# Different framework route pattern => sampled despite delay
get '/grape/projects/1', {}, {'REMOTE_ADDR' => remote_addr}

expect(decisions).to eq([true, false, true])
end
end
end
end
end
32 changes: 30 additions & 2 deletions spec/datadog/appsec/contrib/sinatra/integration_test_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,15 @@
require 'securerandom'
require 'sinatra/base'

# Guard optional rack-contrib dependency in specs
begin
require 'rack/contrib/json_body_parser'
rescue LoadError
# fallback for old rack-contrib
require 'rack/contrib/post_body_content_type_parser'
begin
require 'rack/contrib/post_body_content_type_parser'
rescue LoadError
# not available in this environment; tests that use it will skip inserting the middleware
end
end

require 'datadog/tracing'
Expand Down Expand Up @@ -368,6 +372,30 @@
end
end

describe 'with sample_delay' do
subject(:response) { head url, params, env }

let(:api_security_enabled) { true }
let(:api_security_sample) { 30 }

let(:url) { '/success' }
let(:params) { {} }
let(:headers) { {} }
let(:env) { {'REMOTE_ADDR' => remote_addr}.merge!(headers) }

it 'samples and caches check result' do
head url, params, env
first_span = spans.find { |s| s.name == 'rack.request' }
expect(first_span.tags).to have_key('_dd.appsec.s.req.headers')

clear_traces!

head url, params, env
second_span = spans.find { |s| s.name == 'rack.request' }
expect(second_span.tags).not_to have_key('_dd.appsec.s.req.headers')
end
end

describe 'POST request' do
subject(:response) { post url, params, env }

Expand Down
Loading