fix: avoid prototype-colliding names in execution values (#4653)

completes conversion of execution values to use of prototype-less objects begun in #4634

= adds tests for the new behavior
= fixes out-of-date comments
= small type-change

Author: abishekgiri

src/execution/__tests__/variables-test.ts

@@ -164,6 +164,15 @@ const TestType = new GraphQLObjectType({
       type: TestNestedInputObject,
     }),
     fieldWithJSONScalarInput: fieldWithInputArg({ type: TestJSONScalar }),
+    fieldWithPrototypeNamedArgument: {
+      type: GraphQLString,
+      args: {
+        toString: { type: GraphQLString },
+      },
+      resolve(_, args) {
+        return args.toString === undefined ? 'missing' : inspect(args.toString);
+      },
+    },
     list: fieldWithInputArg({ type: new GraphQLList(GraphQLString) }),
     nested: {
       type: NestedType,
@@ -1225,6 +1234,20 @@ describe('Execute: Handles inputs', () => {
         },
       });
     });
+
+    it('does not expose prototype argument names when omitted', () => {
+      const result = executeQuery(`
+        {
+          fieldWithPrototypeNamedArgument
+        }
+      `);
+
+      expect(result).to.deep.equal({
+        data: {
+          fieldWithPrototypeNamedArgument: 'missing',
+        },
+      });
+    });
   });
 
   describe('getVariableValues: limit maximum number of coercion errors', () => {
@@ -1661,4 +1684,31 @@ describe('Execute: Handles inputs', () => {
       expect(result).to.include.keys('initialResult', 'subsequentResults');
     });
   });
+
+  describe('getVariableValues: own-property names', () => {
+    const doc = parse(`
+      query ($toString: String) {
+        fieldWithNullableStringInput(input: $toString)
+      }
+    `);
+
+    const operation = doc.definitions[0];
+    assert(operation.kind === Kind.OPERATION_DEFINITION);
+    const { variableDefinitions } = operation;
+    assert(variableDefinitions != null);
+
+    it('does not expose prototype variable names when omitted', () => {
+      const result = getVariableValues(schema, variableDefinitions, {});
+      assert('variableValues' in result);
+      expect(result.variableValues.coerced.toString).to.equal(undefined);
+    });
+
+    it('still returns provided variables with colliding names', () => {
+      const result = getVariableValues(schema, variableDefinitions, {
+        toString: 'value',
+      });
+      assert('variableValues' in result);
+      expect(result.variableValues.coerced.toString).to.equal('value');
+    });
+  });
 });

src/execution/values.ts

@@ -58,9 +58,8 @@ type VariableValuesOrErrors =
  * provided variable definitions and arbitrary input. If the input cannot be
  * parsed to match the variable definitions, a GraphQLError will be thrown.
  *
- * Note: The returned value is a plain Object with a prototype, since it is
- * exposed to user code. Care should be taken to not pull values from the
- * Object prototype.
+ * Note: The `coerced` and `sources` properties of VariableValues use null
+ * prototype to avoid collisions with JavaScript's own property names.
  */
 export function getVariableValues(
   schema: GraphQLSchema,
@@ -195,9 +194,8 @@ export function getFragmentVariableValues(
  * Prepares an object map of argument values given a list of argument
  * definitions and list of argument AST nodes.
  *
- * Note: The returned value is a plain Object with a prototype, since it is
- * exposed to user code. Care should be taken to not pull values from the
- * Object prototype.
+ * Note: The returned value uses a null prototype to avoid collisions with
+ * JavaScript's own property names.
  */
 export function getArgumentValues(
   def: GraphQLField<unknown, unknown> | GraphQLDirective,
@@ -316,17 +314,16 @@ function coerceArgument(
  *
  * If the directive does not exist on the node, returns undefined.
  *
- * Note: The returned value is a plain Object with a prototype, since it is
- * exposed to user code. Care should be taken to not pull values from the
- * Object prototype.
+ * Note: The returned value uses a null prototype to avoid collisions with
+ * JavaScript's own property names.
  */
 export function getDirectiveValues(
   directiveDef: GraphQLDirective,
   node: { readonly directives?: ReadonlyArray<DirectiveNode> | undefined },
   variableValues?: Maybe<VariableValues>,
   fragmentVariableValues?: Maybe<FragmentVariableValues>,
   hideSuggestions?: Maybe<boolean>,
-): undefined | { [argument: string]: unknown } {
+): undefined | ObjMap<unknown> {
   const directiveNode = node.directives?.find(
     (directive) => directive.name.value === directiveDef.name,
   );