Skip to main content

Fix CoerceArgumentValues() hasValue

At a glance​

Timeline​

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: String! = "defaultValue" }
  • 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));