[BE-359] Add error handling to the sendVariableValues EngineReporting…

…Option (#3049)

* fix error handling bug in `sendVariableValues` EngineReportingOption

* fixing docs to describe error more precisely

Co-Authored-By: Jesse Rosenberger <git@jro.cc>

* updating changelog w/ info on error fix, adding a line to the 2.7.0 notes on the `sendHeaders` option that was missed in PR #2931

CHANGELOG.md

@@ -6,7 +6,7 @@ The version headers in this history reflect the versions of Apollo Server itself
 
 > The changes noted within this `vNEXT` section have not been released yet.  New PRs and commits which introduce changes should include an entry in this `vNEXT` section as part of their development.  When a release is being prepared, a new header will be (manually) created below and the the appropriate changes within that release will be moved into the new section.
 
-- _Nothing yet! Stay tuned!_
+- `apollo-engine-reporting`: If an error is thrown by a custom variable transform function passed into the reporting option `sendVariableValues: { transform: ... }`, all variable values will be replaced with the string `[PREDICATE_FUNCTION_ERROR]`. 
 
 
 ### v2.7.0
@@ -26,7 +26,14 @@ The version headers in this history reflect the versions of Apollo Server itself
     sendVariableValues: { all: true }
   }
   ```
-
+- `apollo-engine-reporting`: **Behavior change**: By default, send no GraphQL request headers and values to Apollo's servers instead of sending all. Adding the new EngineReportingOption `sendHeaders` to send some or all header values. This replaces the `privateHeaders` option, which is now deprecated. [PR #2931](https://github.com/apollographql/apollo-server/pull/2931)
+
+   To maintain the previous behavior of transmitting  **all** GraphQL request headers and values, configure `engine`.`sendHeaders` as following:
+     ```js
+     engine: {
+       sendHeaders: { all: true }
+     }
+     ```
 - `graphql-playground`: Update to resolve incorrect background color on tabs when using the `light` theme. [PR #2989](https://github.com/apollographql/apollo-server/pull/2989) [Issue #2979](https://github.com/apollographql/apollo-server/issues/2979)
 - `graphql-playground`: Fix "Query Planner" and "Tracing" panels which were off the edge of the viewport.
 - `apollo-server-plugin-base`: Fix `GraphQLRequestListener` type definitions to allow `return void`. [PR #2368](https://github.com/apollographql/apollo-server/pull/2368)

docs/source/api/apollo-server.md

@@ -357,7 +357,7 @@ addMockFunctionsToSchema({
 
     - `{ none: true }`: Don't send any variable values. **(DEFAULT)**
     - `{ all: true }`: Send all variable values.
-    - `{ transform: ({ variables, operationString}) => { ... } }`: A custom function for modifying variable values. Keys added by the custom function will be removed, and keys removed will be added back with an empty value.
+    - `{ transform: ({ variables, operationString}) => { ... } }`: A custom function for modifying variable values. Keys added by the custom function will be removed, and keys removed will be added back with an empty value.  For security reasons, if an error occurs within this function, all variable values will be replaced with `[PREDICATE_FUNCTION_ERROR]`. 
     - `{ exceptNames: [...] }`: A case-sensitive list of names of variables whose values should not be sent to Apollo servers.
     - `{ onlyNames: [...] }`: A case-sensitive list of names of variables whose values will be sent to Apollo servers.
 

packages/apollo-engine-reporting/src/__tests__/extension.test.ts

@@ -3,7 +3,7 @@ import {
   GraphQLExtensionStack,
   enableGraphQLExtensions,
 } from 'graphql-extensions';
-import { graphql } from 'graphql';
+import { graphql, GraphQLError } from 'graphql';
 import { Request } from 'node-fetch';
 import {
   EngineReportingExtension,
@@ -269,6 +269,26 @@ describe('variableJson output for sendVariableValues transform: custom function
       ],
     ).toEqual(JSON.stringify(null));
   });
+
+  const errorThrowingModifier = (input: {
+    variables: Record<string, any>;
+  }): Record<string, any> => {
+    throw new GraphQLError('testing error handling');
+  };
+
+  it('redact all variable values when custom modifier throws an error', () => {
+    const variableJson = makeTraceDetails(variables, {
+      transform: errorThrowingModifier,
+    }).variablesJson;
+    Object.keys(variableJson).forEach(variableName => {
+      expect(variableJson[variableName]).toEqual(
+        JSON.stringify('[PREDICATE_FUNCTION_ERROR]'),
+      );
+    });
+    expect(Object.keys(variableJson).sort()).toEqual(
+      Object.keys(variables).sort(),
+    );
+  });
 });
 
 describe('Catch circular reference error during JSON.stringify', () => {

packages/apollo-engine-reporting/src/agent.ts

@@ -104,7 +104,7 @@ export interface EngineReportingOptions<TContext> {
    * - { none: true }: don't send any variable values (DEFAULT)
    * - { all: true}: send all variable values
    * - { transform: ... }: a custom function for modifying variable values. Keys added by the custom function will
-   *    be removed, and keys removed will be added back with an empty value.
+   *    be removed, and keys removed will be added back with an empty value. For security reasons, if an error occurs within this function, all variable values will be replaced with `[PREDICATE_FUNCTION_ERROR]`.
    * - { exceptNames: ... }: a case-sensitive list of names of variables whose values should not be sent to Apollo servers
    * - { onlyNames: ... }: A case-sensitive list of names of variables whose values will be sent to Apollo servers
    *

packages/apollo-engine-reporting/src/extension.ts

@@ -242,13 +242,19 @@ export function makeTraceDetails(
   const details = new Trace.Details();
   const variablesToRecord = (() => {
     if (sendVariableValues && 'transform' in sendVariableValues) {
-      // Custom function to allow user to specify what variablesJson will look like
       const originalKeys = Object.keys(variables);
-      const modifiedVariables = sendVariableValues.transform({
-        variables: variables,
-        operationString: operationString,
-      });
-      return cleanModifiedVariables(originalKeys, modifiedVariables);
+      try {
+        // Custom function to allow user to specify what variablesJson will look like
+        const modifiedVariables = sendVariableValues.transform({
+          variables: variables,
+          operationString: operationString,
+        });
+        return cleanModifiedVariables(originalKeys, modifiedVariables);
+      } catch (e) {
+        // If the custom function provided by the user throws an exception,
+        // change all the variable values to an appropriate error message.
+        return handleVariableValueTransformError(originalKeys);
+      }
     } else {
       return variables;
     }
@@ -292,6 +298,16 @@ export function makeTraceDetails(
   return details;
 }
 
+function handleVariableValueTransformError(
+  variableNames: string[],
+): Record<string, any> {
+  const modifiedVariables = Object.create(null);
+  variableNames.forEach(name => {
+    modifiedVariables[name] = '[PREDICATE_FUNCTION_ERROR]';
+  });
+  return modifiedVariables;
+}
+
 // Helper for makeTraceDetails() to enforce that the keys of a modified 'variables'
 // matches that of the original 'variables'
 function cleanModifiedVariables(