GraphQL threats detection and protection#3769
Conversation
… integration tests)
github-actions
Bot
added
appsec
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| # skip CSRF token check for non-GET requests | ||
| begin | ||
| if respond_to?(:skip_before_action) | ||
| skip_before_action :verify_authenticity_token |
Check failure
Code scanning / CodeQL
CSRF protection weakened or disabled
lloeki
left a comment
lloeki
left a comment
There was a problem hiding this comment.
Overall it looks good.
I see a factorisation opportunity, plus a few small notes+questions for clarification.
| multiplex_return = [] | ||
| gateway_multiplex.queries.each do |query| | ||
| query_result = ::GraphQL::Query::Result.new( | ||
| query: query, | ||
| values: JSON.parse(AppSec::Response.content_json) | ||
| ) | ||
| multiplex_return << query_result |
There was a problem hiding this comment.
Could this be extracted into AppSec::Response.negotiate?
There was a problem hiding this comment.
We could extract it but I believe it would become un-necessarily more complex. AppSec::Response.negotiate was made for HTTP-level frameworks, selecting the type of response the client configured (JSON, HTML or Plain text). But GraphQL only return JSON, so we'd have to make a special case to always enforce JSON when it is GraphQL, and also duplicate the resulting JSON times the number of queries in the multiplex.
There was a problem hiding this comment.
Maybe this then?
multiplex_return = appsec_response(some, args)and
private
def appsec_response(some, args)
gateway_multiplex.queries.map do |query|
query_result = ::GraphQL::Query::Result.new(
query: query,
values: JSON.parse(AppSec::Response.content_json)
)
end
endTo split concerns
| module GraphQL | ||
| # GraphQL integration constants | ||
| # @public_api Changing resource names, tag names, or environment variables creates breaking changes. | ||
| module Ext |
There was a problem hiding this comment.
Since this Ext empty, you might as well remove the file.
| private | ||
|
|
||
| def create_arguments_hash | ||
| require 'graphql/language/nodes' |
There was a problem hiding this comment.
Is there a reason why this require is dynamic instead of top-level?
Also, if it needs to be dynamic for some reason, it might be worth doing the require out of a hot code path.
| bits = schema.execute('query test{ user(id: 1) { name } }') | ||
| expect(bits.to_h).to eq({ 'data' => { 'user' => { 'name' => 'Bits' } } }) | ||
|
|
||
| caniche = schema.execute('query test{ user(id: 10) { name } }') |
There was a problem hiding this comment.
😆
| caniche = schema.execute('query test{ user(id: 10) { name } }') | |
| poodle = schema.execute('query test{ user(id: 10) { name } }') |
(j/k)
| | `schemas` | | `Array` | Array of `GraphQL::Schema` objects (that support class-based schema only) to trace. If you do not provide any, then tracing will applied to all the schemas. | `[]` | | ||
| | `with_unified_tracer` | | `Bool` | Enable to instrument with `UnifiedTrace` tracer, enabling support for API Catalog. `with_deprecated_tracer` has priority over this. Default is `false`, using `GraphQL::Tracing::DataDogTrace` (Added in v2.2) | `false` | | ||
| | `with_deprecated_tracer` | | `Bool` | Enable to instrument with deprecated `GraphQL::Tracing::DataDogTracing`. This has priority over `with_unified_tracer`. Default is `false`, using `GraphQL::Tracing::DataDogTrace` | `false` | | ||
| | `service_name` | | `String` | Service name used for graphql instrumentation | `'ruby-graphql'` | |
There was a problem hiding this comment.
'ruby-graphql'
The ruby- prefix seems odd. Shouldn't it simply be graphql? (Really I don't know)
There was a problem hiding this comment.
I agree but this has been added 6 years ago, maybe we should create a new PR about it ?
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3769 +/- ##
==========================================
- Coverage 97.91% 97.89% -0.02%
==========================================
Files 1243 1256 +13
Lines 74763 74983 +220
Branches 3608 3667 +59
==========================================
+ Hits 73205 73408 +203
- Misses 1558 1575 +17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
5 participants
What does this PR do?
This PR adds threats detection and protection for GraphQL using libddwaf, the reactive engine and a GraphQL-Ruby (fake) tracer acting as a middleware for the method we want to instrument.
Instead of instrumenting execute_query and verifying variables and arguments there, we go through every query's AST in execute_multiplex, which enable us to block all queries if a threat is detected, and not just the one where the threat is actually located.
Motivation:
There has been some customer demand about that feature. The goal is to dogfood this on Datadog's GitLab.
Additional Notes:
There are complementary changes on this PR :
Added support for GraphQL 2.3, and added Rails in GraphQL appraisals to do integration tests on a full rails app.
Refactored
waf resultspecs that were in:in appsec/reactive/shared_examples.rb
Fixed a typo in
fetch_configurationin:Fixed indentation in
docs/GettingStarted.mdThe files that are directly connected to GraphQL Threats detection are:
How to test the change?
bundle exec appraisal ruby-X.X-graphql-X.X rake spec:appsec:graphql