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 CoerceArgumentValues() hasValue #1056

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

Conversation

benjie
Copy link
Member

@benjie benjie commented Nov 9, 2023

Fixes a bug discovered whilst carefully evaluating CoerceArgumentValues() that leads to "undefined value leakage" and potential null pointer exception if strictly following the spec. GraphQL.js does not suffer this, so this is a spec bug rather than an implementation bug.

Consider the following schema:

type Query {
  field(arg: String! = "defaultValue"): String
}

And the following GraphQL query:

query ($var: String) {
  field(arg: $var)
}

Imagine that we send an empty object ({}) as the variable values.

Coercing the variableValues according to https://spec.graphql.org/draft/#CoerceVariableValues() we get an empty object ({}).

Fast-forward to https://spec.graphql.org/draft/#CoerceArgumentValues():

  • Let {coercedValues} be an empty unordered Map. coercedValues = {}
  • Let {argumentValues} be the argument values provided in {field}. argumentValues = { arg: $var }
  • Let {fieldName} be the name of {field}. fieldName = 'field'
  • Let {argumentDefinitions} be the arguments defined by {objectType} for the
    field named {fieldName}. argumentDefinitions = { arg: ... }
  • For each {argumentDefinition} in {argumentDefinitions}:
    • Let {argumentName} be the name of {argumentDefinition}. argumentName = 'arg'
    • Let {argumentType} be the expected type of {argumentDefinition}. argumentType = String!
    • Let {defaultValue} be the default value for {argumentDefinition}. defaultValue = 'defaultValue'
    • Let {hasValue} be {true} if {argumentValues} provides a value for the name
      {argumentName}. 🐛 !!!BUG!!! hasValue = true because argumentValues does provide the variable $var as the value for the argument 'arg'
    • Let {argumentValue} be the value provided in {argumentValues} for the name
      {argumentName}. argumentValue = $var
    • If {argumentValue} is a {Variable}: Yes, $var is a variable
      • Let {variableName} be the name of {argumentValue}. variableName = 'var'
      • Let {hasValue} be {true} if {variableValues} provides a value for the name
        {variableName}. 🐛 !!!BUG!!! This does not fire, but hasValue is already {true} by the above.
      • Let {value} be the value provided in {variableValues} for the name
        {variableName}. 🐛 !!!BUG!!! value = undefined
    • Otherwise, let {value} be {argumentValue}. NOT TRIGGERED
    • If {hasValue} is not {true} and {defaultValue} exists (including {null}): NOT TRIGGERED since hasValue is true
      • Add an entry to {coercedValues} named {argumentName} with the value
        {defaultValue}.
    • Otherwise if {argumentType} is a Non-Nullable type, and either {hasValue} is
      not {true} or {value} is {null}, raise a field error. NOT TRIGGERED because hasValue is {true} and value is not {null} (it is undefined!)
    • Otherwise if {hasValue} is true: Yes, it is
      • If {value} is {null}: It is not, it is undefined
        • Add an entry to {coercedValues} named {argumentName} with the value
          {null}.
      • Otherwise, if {argumentValue} is a {Variable}: It is!
        • Add an entry to {coercedValues} named {argumentName} with the value
          {value}. coercedValues[argumentName] = undefined (since value is undefined)
      • Otherwise:
        • If {value} cannot be coerced according to the input coercion rules of
          {argumentType}, raise a field error.
        • Let {coercedValue} be the result of coercing {value} according to the
          input coercion rules of {argumentType}.
        • Add an entry to {coercedValues} named {argumentName} with the value
          {coercedValue}.
  • Return {coercedValues}.

Expectation: coercedValues = { arg: "defaultValue" }
Actual result: coercedValues = { arg: undefined }

arg is non-null string -> NPE! 💥

Essentially the phrase "Let {hasValue} be {true} if {argumentValues} provides a value for the name {argumentName}" is at best ambiguous and at worst plain wrong, since the next two lines get the "value" for {argumentName} and then check to see if this {value} is a variable.

This PR fixes this issue by only setting hasValue to true when the value is explicitly resolved via the two branches: variable and non-variable.

There is no need for a GraphQL.js PR for this since GraphQL.js already follows the expected behavior; reproduction:

import { GraphQLNonNull, GraphQLObjectType, GraphQLSchema, GraphQLString, graphqlSync, printSchema, validateSchema } from "graphql";

const Query = new GraphQLObjectType({
  name: "Query",
  fields: {
    field: {
      args: {
        arg: {
          type: new GraphQLNonNull(GraphQLString),
          defaultValue: "defaultValue",
        },
      },
      type: GraphQLString,
      resolve(_, { arg }) {
        return arg;
      },
    },
  },
});

const schema = new GraphQLSchema({
  query: Query,
});

const result = graphqlSync({
  schema,
  source: /* GraphQL */ `
    query ($var: String) {
      field(arg: $var)
    }
  `,
  variables: {
    /* EXPLICITLY DO NOT PASS "var" */
  },
});

const errors = validateSchema(schema);
if (errors.length) {
  console.dir(errors);
  process.exit(1);
}

console.log(printSchema(schema));
console.log(JSON.stringify(result, null, 2));

@benjie benjie added the 💭 Strawman (RFC 0) RFC Stage 0 (See CONTRIBUTING.md) label Nov 9, 2023
Copy link

netlify bot commented Nov 9, 2023

Deploy Preview for graphql-spec-draft ready!

Name Link
🔨 Latest commit d7ff2cb
🔍 Latest deploy log https://app.netlify.com/sites/graphql-spec-draft/deploys/654cf3355de8f80008710007
😎 Deploy Preview https://deploy-preview-1056--graphql-spec-draft.netlify.app/draft
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@Shane32
Copy link

Shane32 commented Nov 10, 2023

GraphQL.js does not suffer this, so this is a spec bug rather than an implementation bug.

The latest build of GraphQL.NET does not suffer from this issue either. Test added in graphql-dotnet/graphql-dotnet#3762 to be sure.

@mjmahone mjmahone added 💡 Proposal (RFC 1) RFC Stage 1 (See CONTRIBUTING.md) and removed 💭 Strawman (RFC 0) RFC Stage 0 (See CONTRIBUTING.md) labels Dec 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💡 Proposal (RFC 1) RFC Stage 1 (See CONTRIBUTING.md)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants