Skip to content

Conversation

@rmosolgo
Copy link
Owner

@rmosolgo rmosolgo commented Apr 18, 2024

Fixes #4871

This option is off by default now but will be on by default in GraphQL-Ruby 3.0. You can opt into the new behavior with:

# app/graphql/my_schema.rb
GraphQL.reject_numbers_followed_by_names = true 

# ... 

And you can check for invalid query strings using the helper:

transformed_query_str = GraphQL::Language.add_space_between_numbers_and_names(query_str) 
if transformed_query_str.equal?(query_str) 
  # The old string was returned, query_str is valid 
  # do nothing 
else 
   # A new, modified query string was returned 
   # Track this occurrence and/or use the transformed string
   BugTracer.report(:invalid_query_string, query_str)
   query_str = transformed_query_str
end 

TODO:

  • Improve tests and implementation of string fix method to handle exponent notation
  • Also make sure variable definitions are covered, eg query($a: Int = 5$b: Int = 6) { ... }
  • I think that variable definitions and values followed by directives are technically legal because the following character is either @ or $, not a name. I did add tests for input object literals, and they were covered by the current implementation.
  • Are there other places where client-provided values appear, where there might be a number literal? (see above)
  • Add a CI build which runs with GraphQL.reject_numbers_followed_by_names = true

@rmosolgo rmosolgo merged commit dc62848 into master Apr 22, 2024
@rmosolgo rmosolgo deleted the reject-numbers-followed-by-names branch April 22, 2024 15:15
@ravangen
Copy link
Contributor

ravangen commented Jan 6, 2026

I was trying out this approach, using the INVALID_NUMBER_FOLLOWED_BY_NAME_REGEXP regex to detect occurrences. A couple initial observations:

  • Regex may be running on untrusted input, so we likely should have a timeout (we were periodically hitting a 5s timeout)
    • Is there a way to optimize this?
  • False positives in regex: query($cursor1_pageSize: Int) is converted to query($cursor1 _pageSize: Int) by add_space_between_numbers_and_names

@rmosolgo
Copy link
Owner Author

rmosolgo commented Jan 6, 2026

👋 I took a crack at that specific case in #5492.

Good call on the Regexp timeout. It looks like Rails 8 sets a default one: rails/rails#53490

I'm definitely open to adding one the regular expression when it's supported (Ruby 3.2+, I think), what do you think of that?

@ravangen
Copy link
Contributor

ravangen commented Jan 6, 2026

Sounds good. Right now I am setting it locally

# Defined locally to add a timeout to the regex from the gem.
INVALID_NUMBER_FOLLOWED_BY_NAME_REGEXP = Regexp.new(
  GraphQL::Language::INVALID_NUMBER_FOLLOWED_BY_NAME_REGEXP,
  timeout: 1,
).freeze

def invalid_number_followed_by_name?(query)
  query.query_string.match?(INVALID_NUMBER_FOLLOWED_BY_NAME_REGEXP)
rescue Regexp::TimeoutError
  nil
end

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.

Name start at end of numbers should fail to parse

3 participants