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

Set default timeout for GraphQL schema validation #5258

Merged
merged 2 commits into from
Mar 10, 2025

Conversation

vithushar
Copy link
Contributor

@vithushar vithushar commented Feb 27, 2025

Problem

Currently, graphql-ruby does not enforce a timeout for schema validation by default. When validate_timeout isn't explicitly set, maliciously crafted queries can consume substantial server resources and potentially cause denial of service (DoS) conditions.

Through Shopify's bug bounty program, we've received multiple reports demonstrating that a single host can leverage this behavior to impact an entire API.

Solution

Add a reasonable default timeout (3 seconds) for schema validation that:

  • protects all downstream consumers
  • maintains backward compatibility and
  • preserves the ability to customize or disable the timeout

Why 3 seconds?

  • Data-Driven Decision: We analyzed performance metrics across many GraphQL services at Shopify, examining the 95th percentile of GraphQL execution time (not including data retrieval). Most queries execute in under 500ms, with even our most complex queries rarely exceeding 700ms.
  • Configurable Override: Services with exceptional needs can still override the default value

@vithushar vithushar changed the title Set validate_timeout default to a non-nil value Set default timeout for GraphQL schema validation Feb 27, 2025
Currently, the gem does not enforce
a validate_timeout default. Maliciously
crafted queries can lead to resource
exhaustion without a timeout.

This implementation sets a 3 second
default validate_timeout. Most complex
GraphQL requests at Shopify, a
gem consumer with large GraphQL APIs,
are parsed in under 1 second, therefore
a 3s timeout seems appropriate.
@vithushar vithushar force-pushed the add-default-validate-timeout-value branch from 661768d to db5a9bd Compare March 5, 2025 22:21
@vithushar vithushar marked this pull request as ready for review March 6, 2025 14:01
@vithushar
Copy link
Contributor Author

@rmosolgo Would love your thoughts on adding this secure default. Our rationale for why we want this implemented upstream is in the PR description.

@rmosolgo
Copy link
Owner

rmosolgo commented Mar 6, 2025

I'm open to this change, thanks for suggesting it. I'd like to address #5261 before switching on this default because otherwise, the new timeout may interrupt I/O operations in buggy ways. I'll take a look at that in the next day or two then merge this!

@vithushar
Copy link
Contributor Author

vithushar commented Mar 6, 2025

Appreciate the quick response @rmosolgo. That sounds great. Looking forward to having this implemented and super glad that the I/O issues will be addressed prior to this. 🙂

@rmosolgo
Copy link
Owner

Hey, master is ready for this now, but unfortunately there are conflicts. I'd resolve them myself but since this branch exists in Shopify/graphql-ruby, I don't have permission to do so. Do you mind resolving them?

@vithushar
Copy link
Contributor Author

@rmosolgo Conflict resolved! :)

@rmosolgo rmosolgo merged commit e4e6902 into rmosolgo:master Mar 10, 2025
13 of 14 checks passed
@rmosolgo
Copy link
Owner

Thanks for this improvement!

@rmosolgo rmosolgo added this to the 2.4.12 milestone Mar 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants