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.

Sign up for GitHub

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

Add AppSec::ActionHandler to unify action handling #4300

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
+270 −301
Open

Add AppSec::ActionHandler to unify action handling #4300

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
53bf41a
Add AppSec::ActionHandler module
y9v Jan 16, 2025
f65767b
Change blocking in GraphQL to use ActionHandler
y9v Jan 17, 2025
307cc2a
Switch system-tests workflow to a branch
y9v Jan 17, 2025
93506ec
Rename AppSec::ActionHandler to AppSec::ActionsHandler
y9v Jan 17, 2025
06f2060
Add tests for AppSec::ActionsHandler
y9v Jan 17, 2025
4d47360
Remove double-splat from ActionsHandler spec
y9v Jan 17, 2025
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
2 changes: 1 addition & 1 deletion .github/workflows/system-tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
33 changes: 33 additions & 0 deletions lib/datadog/appsec/actions_handler.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
1 change: 1 addition & 0 deletions lib/datadog/appsec/component.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 1 addition & 7 deletions lib/datadog/appsec/contrib/graphql/appsec_trace.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand correctly, this PR also removes the creation of responses at framework level (e.g. GraphQL) to handle them all at Rack level ?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, GraphQL instrumentation should only signal with INTERRUPT and leave response blocking to Rack

end

multiplex_return
end
end
Expand Down
7 changes: 3 additions & 4 deletions lib/datadog/appsec/contrib/graphql/gateway/watcher.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

In the files contained in the Reactive folder of each contrib (e.g. appsec/contrib/graphql/reactive/multiplex.rb at the end of the subscribe methods, we still throw :block if any action is present, which is already wrong in itself. I wanted to fix it in the stacktrace PR but considering that this PR changes the way we handle actions, you may want to do it in this PR by deleting them ?

Copy link
Member Author

Choose a reason for hiding this comment

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

This would be done in a separate PR to reduce the amount of changes here. We want to fully get rid of the Reactive Engine, since it only has a instrumentation-local scope and didn't work as intended

end

next [nil, [[:block, event]]] if block

stack.call(gateway_multiplex.arguments)
end
end
Expand Down
15 changes: 9 additions & 6 deletions lib/datadog/appsec/contrib/rack/gateway/watcher.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down
4 changes: 2 additions & 2 deletions lib/datadog/appsec/contrib/rack/request_body_middleware.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,15 +24,15 @@ 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

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
Expand Down
4 changes: 2 additions & 2 deletions lib/datadog/appsec/contrib/rack/request_middleware.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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 << {
Expand Down
5 changes: 3 additions & 2 deletions lib/datadog/appsec/contrib/rails/gateway/watcher.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
16 changes: 3 additions & 13 deletions lib/datadog/appsec/contrib/rails/patcher.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
14 changes: 0 additions & 14 deletions lib/datadog/appsec/contrib/sinatra/ext.rb
View file
Open in desktop

This file was deleted.

10 changes: 6 additions & 4 deletions lib/datadog/appsec/contrib/sinatra/gateway/watcher.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
29 changes: 3 additions & 26 deletions lib/datadog/appsec/contrib/sinatra/patcher.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
5 changes: 3 additions & 2 deletions lib/datadog/appsec/monitor/gateway/watcher.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading
Loading