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

Fix non-null argument validation #41

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

stanig2106
Copy link

Problem

When defining a schema with a required (non-nullable) argument, the current implementation does not validate the presence of that argument when it's missing in a query. Instead of returning an error as expected, the query executes with the argument set to nil, leading to potentially unintended behavior.

Example Schema

query_fields do
  field(:welcome) do
    argument(:name, null: false)
  end
end

def welcome(name:)
  "Hello #{name}!"
end

(as graphql schema :

schema {
  query: {
    welcome(name: String!): String!
  }
}

)

Current Behavior

Query:

query {
  welcome
}

Response:

{
  "data": {
    "welcome": "Hello !"
  }
}

Expected Behavior:
The server should return an error indicating that the name argument cannot be null.

Root Cause

The issue lies in the Rails::GraphQL::Request::Organizable#parse_arguments method, which returns an empty hash (EMPTY_HASH) if nodes is blank. This bypasses the invocation of the ::Organizable#collect_arguments method, which contains the logic to check for non-nullable arguments.

Relevant snippet from collect_arguments:

# Checks for any required argument that was not provided
source.each_value do |argument|
  next if result.key?(argument.name) || argument.null?
  errors << +"the \"#{argument.gql_name}\" argument can not be null"
end

Proposed Solution

Modify the #parse_arguments method to avoid returning prematurely when nodes is blank. Instead, ensure collect_arguments is always called, even if nodes is nil. This is achieved by updating the code to handle the potential nullity of nodes using safe navigation (&.).

Why This Fix is Important

This bug is critical because it allows a query to bypass required argument checks, potentially leading to:

  • Unexpected application behavior.
  • Security vulnerabilities if developers do not manually validate null arguments.

Implementation

  • Ensure #collect_arguments is invoked regardless of whether nodes is blank.
  • Adapt the nodes.each.with_object({}) logic with safe chaining (&.) to gracefully handle cases where nodes is nil.

Testing (TODO)

  • Add test cases to ensure that queries missing non-nullable arguments return the correct error.
  • Validate that no regressions occur with nullable arguments or valid queries.

This fix addresses a critical validation bug in argument handling, improving both developer experience and application security.


Let me know if further details are needed!

Updated the Rails::GraphQL::Request::Organizable#parse_arguments method
to ensure that non-nullable arguments are properly validated, even when
nodes is blank.

Replaced the premature return of EMPTY_HASH with safe navigation (&.)
to allow the method to handle cases where nodes is nil.

Ensured collect_arguments is always invoked, allowing the required
argument validation logic to run correctly.

Added a fallback (|| {}) to prevent errors in cases where nodes is nil.
@stanig2106 stanig2106 changed the title Fix non-null argument validation Fix null argument validation Jan 9, 2025
@stanig2106 stanig2106 changed the title Fix null argument validation Fix non-null argument validation Jan 9, 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.

1 participant