From 2c2d6c0998a06bc3d0b0ce759d586da4b972735a Mon Sep 17 00:00:00 2001 From: Helen Ho Date: Fri, 14 Jun 2019 14:40:57 -0700 Subject: [PATCH 01/12] - changed name of the new option (from enforcePrivateVariables to maskVariableValues), but this is still TBD - use the same helper for both the deprecated privateVariable option and the new option, since the logic is (basically) the same - added helper (and tests) to enforce that originalVariables.keys == modifiedVariables.keys - updated documentation --- CHANGELOG.md | 1 + docs/source/api/apollo-server.md | 25 ++- .../src/__tests__/extension.test.ts | 141 ++++++++++++++- packages/apollo-engine-reporting/src/agent.ts | 31 +++- .../apollo-engine-reporting/src/extension.ts | 160 ++++++++++++++---- 5 files changed, 316 insertions(+), 42 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index afa7641f50a..086e072498c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ The version headers in this history reflect the versions of Apollo Server itself - `apollo-engine-reporting`: **BEHAVIOR CHANGE**: If the error returned from the `engine.rewriteError` hook has an `extensions` property, that property will be used instead of the original error's extensions. Document that changes to most other `GraphQLError` fields by `engine.rewriteError` are ignored. [PR #2932](https://github.com/apollographql/apollo-server/pull/2932) - `apollo-engine-reporting`: **BEHAVIOR CHANGE**: The `engine.maskErrorDetails` option, deprecated by `engine.rewriteError` in v2.5.0, now behaves a bit more like the new option: while all error messages will be redacted, they will still show up on the appropriate nodes in a trace. [PR #2932](https://github.com/apollographql/apollo-server/pull/2932) +- `apollo-engine-reporting`: **BEHAVIOR CHANGE**: By default, send no GraphQL variable values to Apollo's servers instead of sending all variable values. Adding the new EngineReportingOption `maskVariableValues` to send some or all variable values, possibly after transforming them. This replaces the `privateVariables` option, which is now deprecated. [PR #2847](https://github.com/apollographql/apollo-server/pull/2847) ### v2.6.7 diff --git a/docs/source/api/apollo-server.md b/docs/source/api/apollo-server.md index 890eebcdf48..e26b9d740e0 100644 --- a/docs/source/api/apollo-server.md +++ b/docs/source/api/apollo-server.md @@ -119,7 +119,7 @@ new ApolloServer({ * `engine`: <`EngineReportingOptions`> | boolean - Provided the `ENGINE_API_KEY` environment variable is set, the engine reporting agent will be started automatically. The API key can also be provided as the `apiKey` field in an object passed as the `engine` field. See the [EngineReportingOptions](#enginereportingoptions) section for a full description of how to configure the reporting agent, including how to blacklist variables. When using the Engine proxy, this option should be set to false. + Provided the `ENGINE_API_KEY` environment variable is set, the engine reporting agent will be started automatically. The API key can also be provided as the `apiKey` field in an object passed as the `engine` field. See the [EngineReportingOptions](#enginereportingoptions) section for a full description of how to configure the reporting agent, including how to blocklist variables. When using the Engine proxy, this option should be set to false. * `persistedQueries`: <`Object`> | false @@ -343,14 +343,31 @@ addMockFunctionsToSchema({ to standard error. Specify this function to process errors in a different way. -* `privateVariables`: Array | boolean +* `privateVariables`: Array | boolean + DEPRECATING IN VERSION XX.XX.XX for `maskVariableValues`, which will support the same + functionalities but allow for more flexibility. + A case-sensitive list of names of variables whose values should not be sent to Apollo servers, or 'true' to leave out all variables. In the former case, the report will indicate that each private variable was redacted in the latter case, no variables are sent at all. - -* `privateHeaders`: Array | boolean + +* `maskVariableValues`: { valueModifier: (options: { variables: Record, operationString?: string } ) => Record } + | { privateVariableNames: Array } + | { always: boolean } + + By default, Apollo Server does not send the values of any GraphQL variables to Apollo's servers, because variable values often contain the private data of your app's users. If you'd like variable values to be included in traces, set this option. This option can take several forms: + + - { always: ... }: true to blocklist, or false to whitelist all variable values + - { valueModifier: ... }: a custom function for modifying variable values + - { privateVariableNames: ... }: a case-sensitive list of names of variables whose values should not be sent to Apollo servers + + Defaults to blocklisting all variable values if both this parameter and + the to-be-deprecated `privateVariables` are not set. The report will also + indicate each private variable redacted if set to { always: ... } or { privateVariableNames: ... } + +* `privateHeaders`: Array | boolean A case-insensitive list of names of HTTP headers whose values should not be sent to Apollo servers, or 'true' to leave out all HTTP headers. Unlike diff --git a/packages/apollo-engine-reporting/src/__tests__/extension.test.ts b/packages/apollo-engine-reporting/src/__tests__/extension.test.ts index c61eccbfd75..3101703b379 100644 --- a/packages/apollo-engine-reporting/src/__tests__/extension.test.ts +++ b/packages/apollo-engine-reporting/src/__tests__/extension.test.ts @@ -6,7 +6,11 @@ import { import { Trace } from 'apollo-engine-reporting-protobuf'; import { graphql } from 'graphql'; import { Request } from 'node-fetch'; -import { EngineReportingExtension } from '../extension'; +import { + EngineReportingExtension, + makeTraceDetails, + makeTraceDetailsLegacy, +} from '../extension'; import { InMemoryLRUCache } from 'apollo-server-caching'; test('trace construction', async () => { @@ -83,3 +87,138 @@ test('trace construction', async () => { requestDidEnd(); // XXX actually write some tests }); + +const variables: Record = { + testing: 'testing', + t2: 2, +}; + +test('check variableJson output for privacyEnforcer boolean type', () => { + // Case 1: No keys/values in variables to be filtered/not filtered + const emptyOutput = new Trace.Details(); + expect(makeTraceDetails({}, { always: true })).toEqual(emptyOutput); + expect(makeTraceDetails({}, { always: false })).toEqual(emptyOutput); + + // Case 2: Filter all variables + const filteredOutput = new Trace.Details(); + Object.keys(variables).forEach(name => { + filteredOutput.variablesJson[name] = ''; + }); + expect(makeTraceDetails(variables, { always: true })).toEqual(filteredOutput); + + // Case 3: Do not filter variables + const nonFilteredOutput = new Trace.Details(); + Object.keys(variables).forEach(name => { + nonFilteredOutput.variablesJson[name] = JSON.stringify(variables[name]); + }); + expect(makeTraceDetails(variables, { always: false })).toEqual( + nonFilteredOutput, + ); +}); + +test('variableJson output for privacyEnforcer Array type', () => { + const privateVariablesArray: string[] = ['testing', 'notInVariables']; + const expectedVariablesJson = { + testing: '', + t2: JSON.stringify(2), + }; + expect( + makeTraceDetails(variables, { privateVariableNames: privateVariablesArray }) + .variablesJson, + ).toEqual(expectedVariablesJson); +}); + +test('variableJson output for privacyEnforcer custom function', () => { + // Custom function that redacts every variable to 100; + const modifiedValue = 100; + const customModifier = (input: { + variables: Record; + }): Record => { + let out: Record = {}; + Object.keys(input.variables).map((name: string) => { + out[name] = modifiedValue; + }); + return out; + }; + + // Expected output + const output = new Trace.Details(); + Object.keys(variables).forEach(name => { + output.variablesJson[name] = JSON.stringify(modifiedValue); + }); + + expect( + makeTraceDetails(variables, { valueModifier: customModifier }), + ).toEqual(output); +}); + +test('privacyEnforcer=True equivalent to privacyEnforcer=Array(all variables)', () => { + let privateVariablesArray: string[] = ['testing', 't2']; + expect(makeTraceDetails(variables, { always: true }).variablesJson).toEqual( + makeTraceDetails(variables, { privateVariableNames: privateVariablesArray }) + .variablesJson, + ); +}); + +test('original keys in variables match the modified keys', () => { + const origKeys = Object.keys(variables); + const modifiedKeys = Object.keys(variables).slice(1); + modifiedKeys.push('new v'); + + const modifier = (input: { + variables: Record; + }): Record => { + let out: Record = {}; + Object.keys(modifiedKeys).map((name: string) => { + out[name] = 100; + }); + return out; + }; + + expect( + Object.keys( + makeTraceDetails(variables, { valueModifier: modifier }).variablesJson, + ).sort(), + ).toEqual(origKeys.sort()); +}); + +/** + * Tests for old privateVariables reporting option + */ + +test('test variableJson output for to-be-deprecated privateVariable option', () => { + // Case 1: privateVariables == False; same as makeTraceDetails + expect(makeTraceDetailsLegacy({}, false)).toEqual( + makeTraceDetails({}, { always: false }), + ); + expect(makeTraceDetailsLegacy(variables, false)).toEqual( + makeTraceDetails(variables, { always: false }), + ); + + // Case 2: privateVariables is an Array; same as makeTraceDetails + const privacyEnforcerArray: string[] = ['testing', 'notInVariables']; + expect( + makeTraceDetailsLegacy(variables, privacyEnforcerArray).variablesJson, + ).toEqual( + makeTraceDetails(variables, { privateVariableNames: privacyEnforcerArray }) + .variablesJson, + ); +}); + +test('original keys in variables match the modified keys', () => { + const origKeys = Object.keys(variables); + const modifiedKeys = Object.keys(variables).slice(1); + modifiedKeys.push('new v'); + + const enforcer = (input: Record): Record => { + let out: Record = {}; + Object.keys(modifiedKeys).map((name: string) => { + out[name] = 100; + }); + return out; + }; + + expect( + Object.keys(makeTraceDetails(variables, enforcer).variablesJson).sort(), + ).toEqual(origKeys.sort()); +}); diff --git a/packages/apollo-engine-reporting/src/agent.ts b/packages/apollo-engine-reporting/src/agent.ts index 2535a42a4a8..397642c9dc8 100644 --- a/packages/apollo-engine-reporting/src/agent.ts +++ b/packages/apollo-engine-reporting/src/agent.ts @@ -25,6 +25,11 @@ export interface ClientInfo { clientReferenceId?: string; } +export type VariableValueModifierOptions = { + variables: Record; + operationString?: string; +}; + export type GenerateClientInfo = ( requestContext: GraphQLRequestContext, ) => ClientInfo; @@ -82,12 +87,34 @@ export interface EngineReportingOptions { */ reportErrorFunction?: (err: Error) => void; /** - * A case-sensitive list of names of variables whose values should not be sent - * to Apollo servers, or 'true' to leave out all variables. In the former + * [TO-BE-DEPRECATED] A case-sensitive list of names of variables whose values should + * not be sent to Apollo servers, or 'true' to leave out all variables. In the former * case, the report will indicate that each private variable was redacted; in * the latter case, no variables are sent at all. */ privateVariables?: Array | boolean; + /** + * By default, Apollo Server does not send the values of any GraphQL variables to Apollo's servers, because variable values often contain the private data of your app's users. If you'd like variable values to be included in traces, set this option. This option can take several forms: + * - true or false to blocklist or whitelist all variable values + * - a custom function for modifying variable values + * - a case-sensitive list of names of variables whose values should not be sent to Apollo servers + * + * Will default to blocklisting all variable values if both this parameter and + * the to-be-deprecated `privateVariables` were not set. The report will also + * indicate each private variable redacted if set to a boolean or array. + * + * TODO(helen): Add new flag to the trace details (and modify the protobuf message structure) to indicate the type of modification. Then, add the following description to the docs: + * "The report will indicate that variable values were modified by a custom function, or will list all private variables redacted." + * TODO(helen): LINK TO EXAMPLE FUNCTION? e.g. a function recursively search for keys to be blocklisted + */ + maskVariableValues?: + | { + valueModifier: ( + options: VariableValueModifierOptions, + ) => Record; + } + | { privateVariableNames: Array } + | { always: boolean }; /** * A case-insensitive list of names of HTTP headers whose values should not be * sent to Apollo servers, or 'true' to leave out all HTTP headers. Unlike diff --git a/packages/apollo-engine-reporting/src/extension.ts b/packages/apollo-engine-reporting/src/extension.ts index 88b8e3dac0c..67215328407 100644 --- a/packages/apollo-engine-reporting/src/extension.ts +++ b/packages/apollo-engine-reporting/src/extension.ts @@ -15,8 +15,11 @@ import { EngineReportingOptions, GenerateClientInfo, AddTraceArgs, + VariableValueModifier, + VariableValueModifierOptions, } from './agent'; import { GraphQLRequestContext } from 'apollo-server-core/dist/requestPipelineAPI'; +import { operation } from 'retry'; const clientNameHeaderKey = 'apollographql-client-name'; const clientReferenceIdHeaderKey = 'apollographql-client-reference-id'; @@ -127,41 +130,25 @@ export class EngineReportingExtension } } - if (this.options.privateVariables !== true && o.variables) { - // Note: we explicitly do *not* include the details.rawQuery field. The - // Engine web app currently does nothing with this other than store it in - // the database and offer it up via its GraphQL API, and sending it means - // that using calculateSignature to hide sensitive data in the query - // string is ineffective. - this.trace.details = new Trace.Details(); - Object.keys(o.variables).forEach(name => { - if ( - this.options.privateVariables && - Array.isArray(this.options.privateVariables) && - // We assume that most users will have only a few private variables, - // or will just set privateVariables to true; we can change this - // linear-time operation if it causes real performance issues. - this.options.privateVariables.includes(name) - ) { - // Special case for private variables. Note that this is a different - // representation from a variable containing the empty string, as that - // will be sent as '""'. - this.trace.details!.variablesJson![name] = ''; - } else { - try { - this.trace.details!.variablesJson![name] = JSON.stringify( - o.variables![name], - ); - } catch (e) { - // This probably means that the value contains a circular reference, - // causing `JSON.stringify()` to throw a TypeError: - // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/stringify#Issue_with_JSON.stringify()_when_serializing_circular_references - this.trace.details!.variablesJson![name] = JSON.stringify( - '[Unable to convert value to JSON]', - ); - } - } - }); + if (o.variables) { + if ( + this.options.maskVariableValues !== undefined || + this.options.privateVariables == null + ) { + // The new option maskVariableValues will take precendence over the deprecated privateVariables option + this.trace.details = makeTraceDetails( + o.variables, + this.options.maskVariableValues, + o.queryString, + ); + } else { + // DEPRECATED: privateVariables + // But keep supporting if it has been set. + this.trace.details = makeTraceDetailsLegacy( + o.variables, + this.options.privateVariables, + ); + } } const clientInfo = this.generateClientInfo(o.requestContext); @@ -424,3 +411,106 @@ function defaultGenerateClientInfo({ request }: GraphQLRequestContext) { return {}; } } + +// Creates trace details from request variables, given a specification for modifying +// values of private or sensitive variables. +// The details include the variables in the request, TODO(helen): and the modifier type. +// If maskVariableValues is a bool or Array, it will act similarly to +// to the to-be-deprecated options.privateVariables, except that the redacted variable +// names will still be visible in the UI when 'true.' +// If maskVariableValues is null, the policy will default to the 'true' case. +export function makeTraceDetails( + variables: Record, + maskVariableValues?: + | { + valueModifier: ( + options: VariableValueModifierOptions, + ) => Record; + } + | { privateVariableNames: Array } + | { always: boolean }, + operationString?: string, +): Trace.Details { + const details = new Trace.Details(); + const variablesToRecord = (() => { + if (maskVariableValues && 'valueModifier' in maskVariableValues) { + // Custom function to allow user to specify what variablesJson will look like + const originalKeys = Object.keys(variables); + const modifiedVariables = maskVariableValues.valueModifier({ + variables: variables, + operationString: operationString, + }); + return cleanModifiedVariables(originalKeys, modifiedVariables); + } else { + return variables; + } + })(); + + // Note: we explicitly do *not* include the details.rawQuery field. The + // Engine web app currently does nothing with this other than store it in + // the database and offer it up via its GraphQL API, and sending it means + // that using calculateSignature to hide sensitive data in the query + // string is ineffective. + Object.keys(variablesToRecord).forEach(name => { + if ( + maskVariableValues == null || + ('always' in maskVariableValues && maskVariableValues.always) || + ('privateVariableNames' in maskVariableValues && + // We assume that most users will have only a few private variables, + // or will just set privateVariables to true; we can change this + // linear-time operation if it causes real performance issues. + maskVariableValues.privateVariableNames.includes(name)) + ) { + // Special case for private variables. Note that this is a different + // representation from a variable containing the empty string, as that + // will be sent as '""'. + details.variablesJson![name] = ''; + } else { + try { + details.variablesJson![name] = + variablesToRecord[name] == null + ? '' + : JSON.stringify(variablesToRecord[name]); + } catch (e) { + // This probably means that the value contains a circular reference, + // causing `JSON.stringify()` to throw a TypeError: + // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/stringify#Issue_with_JSON.stringify()_when_serializing_circular_references + details.variablesJson![name] = JSON.stringify( + '[Unable to convert value to JSON]', + ); + } + } + }); + + return details; +} + +// Creates trace details when privateVariables [DEPRECATED] was set. +// Wraps privateVariable into a format that can be passed into makeTraceDetails(), +// which also handles the new replacement option maskVariableValues. +export function makeTraceDetailsLegacy( + variables: Record, + privateVariables: Array | boolean, +): Trace.Details | undefined { + const newArguments = Array.isArray(privateVariables) + ? { + privateVariableNames: privateVariables, + } + : { + always: privateVariables, + }; + return makeTraceDetails(variables, newArguments); +} + +// Helper for makeTraceDetails() to enforce that the keys of a modified 'variables' +// matches that of the original 'variables' +function cleanModifiedVariables( + originalKeys: Array, + modifiedVariables: Record, +): Record { + let cleanedVariables: Record = {}; + originalKeys.forEach(name => { + cleanedVariables[name] = modifiedVariables[name]; + }); + return cleanedVariables; +} From e5c5432d411a097385f009f486530ec456521a15 Mon Sep 17 00:00:00 2001 From: Helen Ho Date: Wed, 19 Jun 2019 10:37:26 -0700 Subject: [PATCH 02/12] Changed new option to sendVariableValues instead of maskVariableValues, and the subsequent tests/docs --- docs/source/api/apollo-server.md | 14 ++-- .../src/__tests__/extension.test.ts | 72 +++++++++++-------- packages/apollo-engine-reporting/src/agent.ts | 34 +++++---- .../apollo-engine-reporting/src/extension.ts | 39 +++++----- 4 files changed, 84 insertions(+), 75 deletions(-) diff --git a/docs/source/api/apollo-server.md b/docs/source/api/apollo-server.md index e26b9d740e0..697b5221ed0 100644 --- a/docs/source/api/apollo-server.md +++ b/docs/source/api/apollo-server.md @@ -345,7 +345,7 @@ addMockFunctionsToSchema({ * `privateVariables`: Array | boolean - DEPRECATING IN VERSION XX.XX.XX for `maskVariableValues`, which will support the same + DEPRECATING IN VERSION XX.XX.XX for `sendVariableValues`, which will support the same functionalities but allow for more flexibility. A case-sensitive list of names of variables whose values should not be sent @@ -353,19 +353,19 @@ addMockFunctionsToSchema({ case, the report will indicate that each private variable was redacted in the latter case, no variables are sent at all. -* `maskVariableValues`: { valueModifier: (options: { variables: Record, operationString?: string } ) => Record } - | { privateVariableNames: Array } - | { always: boolean } +* `sendVariableValues`: { valueModifier: (options: { variables: Record, operationString?: string } ) => Record } + | { exceptVariableNames: Array } + | { whitelistAll: boolean } By default, Apollo Server does not send the values of any GraphQL variables to Apollo's servers, because variable values often contain the private data of your app's users. If you'd like variable values to be included in traces, set this option. This option can take several forms: - - { always: ... }: true to blocklist, or false to whitelist all variable values + - { whitelistAll: ... }: false to blocklist, or true to whitelist all variable values - { valueModifier: ... }: a custom function for modifying variable values - - { privateVariableNames: ... }: a case-sensitive list of names of variables whose values should not be sent to Apollo servers + - { exceptVariableNames: ... }: a case-sensitive list of names of variables whose values should not be sent to Apollo servers Defaults to blocklisting all variable values if both this parameter and the to-be-deprecated `privateVariables` are not set. The report will also - indicate each private variable redacted if set to { always: ... } or { privateVariableNames: ... } + indicate each private variable key redacted by { whitelistAll: false } or { exceptVariableNames: [...] }. * `privateHeaders`: Array | boolean diff --git a/packages/apollo-engine-reporting/src/__tests__/extension.test.ts b/packages/apollo-engine-reporting/src/__tests__/extension.test.ts index 3101703b379..5f957e5b6c8 100644 --- a/packages/apollo-engine-reporting/src/__tests__/extension.test.ts +++ b/packages/apollo-engine-reporting/src/__tests__/extension.test.ts @@ -93,7 +93,24 @@ const variables: Record = { t2: 2, }; -test('check variableJson output for privacyEnforcer boolean type', () => { +test('check variableJson output for sendVariableValues null or undefined (default)', () => { + // Case 1: No keys/values in variables to be filtered/not filtered + const emptyOutput = new Trace.Details(); + expect(makeTraceDetails({}, null)).toEqual(emptyOutput); + expect(makeTraceDetails({}, undefined)).toEqual(emptyOutput); + expect(makeTraceDetails({})).toEqual(emptyOutput); + + // Case 2: Filter all variables + const filteredOutput = new Trace.Details(); + Object.keys(variables).forEach(name => { + filteredOutput.variablesJson[name] = ''; + }); + expect(makeTraceDetails(variables)).toEqual(filteredOutput); + expect(makeTraceDetails(variables, null)).toEqual(filteredOutput); + expect(makeTraceDetails(variables, undefined)).toEqual(filteredOutput); +}); + +test('check variableJson output for sendVariableValues whitelist type', () => { // Case 1: No keys/values in variables to be filtered/not filtered const emptyOutput = new Trace.Details(); expect(makeTraceDetails({}, { always: true })).toEqual(emptyOutput); @@ -104,14 +121,16 @@ test('check variableJson output for privacyEnforcer boolean type', () => { Object.keys(variables).forEach(name => { filteredOutput.variablesJson[name] = ''; }); - expect(makeTraceDetails(variables, { always: true })).toEqual(filteredOutput); + expect(makeTraceDetails(variables, { whitelistAll: false })).toEqual( + filteredOutput, + ); // Case 3: Do not filter variables const nonFilteredOutput = new Trace.Details(); Object.keys(variables).forEach(name => { nonFilteredOutput.variablesJson[name] = JSON.stringify(variables[name]); }); - expect(makeTraceDetails(variables, { always: false })).toEqual( + expect(makeTraceDetails(variables, { whitelistAll: true })).toEqual( nonFilteredOutput, ); }); @@ -123,7 +142,7 @@ test('variableJson output for privacyEnforcer Array type', () => { t2: JSON.stringify(2), }; expect( - makeTraceDetails(variables, { privateVariableNames: privateVariablesArray }) + makeTraceDetails(variables, { exceptVariableNames: privateVariablesArray }) .variablesJson, ).toEqual(expectedVariablesJson); }); @@ -152,17 +171,22 @@ test('variableJson output for privacyEnforcer custom function', () => { ).toEqual(output); }); -test('privacyEnforcer=True equivalent to privacyEnforcer=Array(all variables)', () => { +test('whitelist=False equivalent to Array(all variables)', () => { let privateVariablesArray: string[] = ['testing', 't2']; - expect(makeTraceDetails(variables, { always: true }).variablesJson).toEqual( - makeTraceDetails(variables, { privateVariableNames: privateVariablesArray }) + expect( + makeTraceDetails(variables, { whitelistAll: false }).variablesJson, + ).toEqual( + makeTraceDetails(variables, { exceptVariableNames: privateVariablesArray }) .variablesJson, ); }); test('original keys in variables match the modified keys', () => { const origKeys = Object.keys(variables); + const firstKey = origKeys[0]; + // remove the first key const modifiedKeys = Object.keys(variables).slice(1); + // add a key modifiedKeys.push('new v'); const modifier = (input: { @@ -180,19 +204,25 @@ test('original keys in variables match the modified keys', () => { makeTraceDetails(variables, { valueModifier: modifier }).variablesJson, ).sort(), ).toEqual(origKeys.sort()); + + // expect empty string for keys removed by the custom modifier + expect( + makeTraceDetails(variables, { valueModifier: modifier }).variablesJson[ + firstKey + ], + ).toEqual(''); }); /** * Tests for old privateVariables reporting option */ - test('test variableJson output for to-be-deprecated privateVariable option', () => { - // Case 1: privateVariables == False; same as makeTraceDetails + // Case 1: privateVariables == False; same as whitelist all expect(makeTraceDetailsLegacy({}, false)).toEqual( - makeTraceDetails({}, { always: false }), + makeTraceDetails({}, { whitelistAll: false }), ); expect(makeTraceDetailsLegacy(variables, false)).toEqual( - makeTraceDetails(variables, { always: false }), + makeTraceDetails(variables, { whitelistAll: true }), ); // Case 2: privateVariables is an Array; same as makeTraceDetails @@ -200,25 +230,7 @@ test('test variableJson output for to-be-deprecated privateVariable option', () expect( makeTraceDetailsLegacy(variables, privacyEnforcerArray).variablesJson, ).toEqual( - makeTraceDetails(variables, { privateVariableNames: privacyEnforcerArray }) + makeTraceDetails(variables, { exceptVariableNames: privacyEnforcerArray }) .variablesJson, ); }); - -test('original keys in variables match the modified keys', () => { - const origKeys = Object.keys(variables); - const modifiedKeys = Object.keys(variables).slice(1); - modifiedKeys.push('new v'); - - const enforcer = (input: Record): Record => { - let out: Record = {}; - Object.keys(modifiedKeys).map((name: string) => { - out[name] = 100; - }); - return out; - }; - - expect( - Object.keys(makeTraceDetails(variables, enforcer).variablesJson).sort(), - ).toEqual(origKeys.sort()); -}); diff --git a/packages/apollo-engine-reporting/src/agent.ts b/packages/apollo-engine-reporting/src/agent.ts index 397642c9dc8..e69c88f7e3a 100644 --- a/packages/apollo-engine-reporting/src/agent.ts +++ b/packages/apollo-engine-reporting/src/agent.ts @@ -30,6 +30,15 @@ export type VariableValueModifierOptions = { operationString?: string; }; +export type VariableValueOptions = + | { + valueModifier: ( + options: VariableValueModifierOptions, + ) => Record; + } + | { exceptVariableNames: Array } + | { whitelistAll: boolean }; + export type GenerateClientInfo = ( requestContext: GraphQLRequestContext, ) => ClientInfo; @@ -94,27 +103,22 @@ export interface EngineReportingOptions { */ privateVariables?: Array | boolean; /** - * By default, Apollo Server does not send the values of any GraphQL variables to Apollo's servers, because variable values often contain the private data of your app's users. If you'd like variable values to be included in traces, set this option. This option can take several forms: - * - true or false to blocklist or whitelist all variable values - * - a custom function for modifying variable values - * - a case-sensitive list of names of variables whose values should not be sent to Apollo servers + * By default, Apollo Server does not send the values of any GraphQL variables to Apollo's servers, because variable + * values often contain the private data of your app's users. If you'd like variable values to be included in traces, set this option. + * This option can take several forms: + * - { whitelistAll: ... }: false to blocklist, or true to whitelist all variable values + * - { valueModifier: ... }: a custom function for modifying variable values + * - { exceptVariableNames: ... }: a case-sensitive list of names of variables whose values should not be sent to Apollo servers * - * Will default to blocklisting all variable values if both this parameter and - * the to-be-deprecated `privateVariables` were not set. The report will also - * indicate each private variable redacted if set to a boolean or array. + * Defaults to blocklisting all variable values if both this parameter and + * the to-be-deprecated `privateVariables` are not set. The report will also + * indicate each private variable key redacted by { whitelistAll: false } or { exceptVariableNames: [...] }. * * TODO(helen): Add new flag to the trace details (and modify the protobuf message structure) to indicate the type of modification. Then, add the following description to the docs: * "The report will indicate that variable values were modified by a custom function, or will list all private variables redacted." * TODO(helen): LINK TO EXAMPLE FUNCTION? e.g. a function recursively search for keys to be blocklisted */ - maskVariableValues?: - | { - valueModifier: ( - options: VariableValueModifierOptions, - ) => Record; - } - | { privateVariableNames: Array } - | { always: boolean }; + sendVariableValues?: VariableValueOptions; /** * A case-insensitive list of names of HTTP headers whose values should not be * sent to Apollo servers, or 'true' to leave out all HTTP headers. Unlike diff --git a/packages/apollo-engine-reporting/src/extension.ts b/packages/apollo-engine-reporting/src/extension.ts index 67215328407..1c519568590 100644 --- a/packages/apollo-engine-reporting/src/extension.ts +++ b/packages/apollo-engine-reporting/src/extension.ts @@ -15,11 +15,10 @@ import { EngineReportingOptions, GenerateClientInfo, AddTraceArgs, - VariableValueModifier, - VariableValueModifierOptions, + VariableValueOptions, } from './agent'; import { GraphQLRequestContext } from 'apollo-server-core/dist/requestPipelineAPI'; -import { operation } from 'retry'; +// import { operation } from 'retry'; const clientNameHeaderKey = 'apollographql-client-name'; const clientReferenceIdHeaderKey = 'apollographql-client-reference-id'; @@ -132,17 +131,17 @@ export class EngineReportingExtension if (o.variables) { if ( - this.options.maskVariableValues !== undefined || + this.options.sendVariableValues !== undefined || this.options.privateVariables == null ) { // The new option maskVariableValues will take precendence over the deprecated privateVariables option this.trace.details = makeTraceDetails( o.variables, - this.options.maskVariableValues, + this.options.sendVariableValues, o.queryString, ); } else { - // DEPRECATED: privateVariables + // privateVariables will be DEPRECATED // But keep supporting if it has been set. this.trace.details = makeTraceDetailsLegacy( o.variables, @@ -421,22 +420,15 @@ function defaultGenerateClientInfo({ request }: GraphQLRequestContext) { // If maskVariableValues is null, the policy will default to the 'true' case. export function makeTraceDetails( variables: Record, - maskVariableValues?: - | { - valueModifier: ( - options: VariableValueModifierOptions, - ) => Record; - } - | { privateVariableNames: Array } - | { always: boolean }, + sendVariableValues?: VariableValueOptions, operationString?: string, ): Trace.Details { const details = new Trace.Details(); const variablesToRecord = (() => { - if (maskVariableValues && 'valueModifier' in maskVariableValues) { + if (sendVariableValues && 'valueModifier' in sendVariableValues) { // Custom function to allow user to specify what variablesJson will look like const originalKeys = Object.keys(variables); - const modifiedVariables = maskVariableValues.valueModifier({ + const modifiedVariables = sendVariableValues.valueModifier({ variables: variables, operationString: operationString, }); @@ -453,13 +445,14 @@ export function makeTraceDetails( // string is ineffective. Object.keys(variablesToRecord).forEach(name => { if ( - maskVariableValues == null || - ('always' in maskVariableValues && maskVariableValues.always) || - ('privateVariableNames' in maskVariableValues && + sendVariableValues == null || + ('whitelistAll' in sendVariableValues && + !sendVariableValues.whitelistAll) || + ('exceptVariableNames' in sendVariableValues && // We assume that most users will have only a few private variables, // or will just set privateVariables to true; we can change this // linear-time operation if it causes real performance issues. - maskVariableValues.privateVariableNames.includes(name)) + sendVariableValues.exceptVariableNames.includes(name)) ) { // Special case for private variables. Note that this is a different // representation from a variable containing the empty string, as that @@ -487,17 +480,17 @@ export function makeTraceDetails( // Creates trace details when privateVariables [DEPRECATED] was set. // Wraps privateVariable into a format that can be passed into makeTraceDetails(), -// which also handles the new replacement option maskVariableValues. +// which handles the new option sendVariableValues. export function makeTraceDetailsLegacy( variables: Record, privateVariables: Array | boolean, ): Trace.Details | undefined { const newArguments = Array.isArray(privateVariables) ? { - privateVariableNames: privateVariables, + exceptVariableNames: privateVariables, } : { - always: privateVariables, + whitelistAll: !privateVariables, }; return makeTraceDetails(variables, newArguments); } From 822bc0b60decfb2a8925254aa1540931565b446c Mon Sep 17 00:00:00 2001 From: Helen Ho Date: Wed, 19 Jun 2019 14:14:42 -0700 Subject: [PATCH 03/12] adding new reporting option sendHeaders, fixing some names --- docs/source/api/apollo-server.md | 20 +++- .../src/__tests__/extension.test.ts | 105 +++++++++++++++-- packages/apollo-engine-reporting/src/agent.ts | 22 +++- .../apollo-engine-reporting/src/extension.ts | 110 ++++++++++++------ 4 files changed, 203 insertions(+), 54 deletions(-) diff --git a/docs/source/api/apollo-server.md b/docs/source/api/apollo-server.md index 697b5221ed0..2cc1c86d78e 100644 --- a/docs/source/api/apollo-server.md +++ b/docs/source/api/apollo-server.md @@ -355,20 +355,32 @@ addMockFunctionsToSchema({ * `sendVariableValues`: { valueModifier: (options: { variables: Record, operationString?: string } ) => Record } | { exceptVariableNames: Array } - | { whitelistAll: boolean } + | { safelistAll: boolean } By default, Apollo Server does not send the values of any GraphQL variables to Apollo's servers, because variable values often contain the private data of your app's users. If you'd like variable values to be included in traces, set this option. This option can take several forms: - - { whitelistAll: ... }: false to blocklist, or true to whitelist all variable values + - { safelistAll: ... }: false to blocklist, or true to safelist all variable values - { valueModifier: ... }: a custom function for modifying variable values - { exceptVariableNames: ... }: a case-sensitive list of names of variables whose values should not be sent to Apollo servers Defaults to blocklisting all variable values if both this parameter and the to-be-deprecated `privateVariables` are not set. The report will also - indicate each private variable key redacted by { whitelistAll: false } or { exceptVariableNames: [...] }. - + indicate each private variable key redacted by { safelistAll: false } or { exceptVariableNames: [...] }. + +* `sendHeaders`: { except: Array } | { safelistAll: boolean } + By default, Apollo Server does not send the list of HTTP headers and values to + Apollo's servers, to protect private data of your app's users. If you'd like this information included in traces, + set this option. This option can take two forms: + + - {except: Array} A case-insensitive list of names of HTTP headers whose values should not be + sent to Apollo servers + - {safelistAll: true/false} to include or leave out all HTTP headers. + + Unlike with sendVariableValues, names of dropped headers are not reported. + * `privateHeaders`: Array | boolean + DEPRECATING IN VERSION XX.XX.XX, use 'sendHeaders' instead. A case-insensitive list of names of HTTP headers whose values should not be sent to Apollo servers, or 'true' to leave out all HTTP headers. Unlike with privateVariables, names of dropped headers are not reported. diff --git a/packages/apollo-engine-reporting/src/__tests__/extension.test.ts b/packages/apollo-engine-reporting/src/__tests__/extension.test.ts index 5f957e5b6c8..f3dc8ac31f0 100644 --- a/packages/apollo-engine-reporting/src/__tests__/extension.test.ts +++ b/packages/apollo-engine-reporting/src/__tests__/extension.test.ts @@ -10,7 +10,10 @@ import { EngineReportingExtension, makeTraceDetails, makeTraceDetailsLegacy, + makeHTTPRequestHeaders, + makeHTTPRequestHeadersLegacy, } from '../extension'; +import { Headers } from 'apollo-server-env'; import { InMemoryLRUCache } from 'apollo-server-caching'; test('trace construction', async () => { @@ -88,6 +91,9 @@ test('trace construction', async () => { // XXX actually write some tests }); +/** + * TESTS FOR sendVariableValues REPORTING OPTION + */ const variables: Record = { testing: 'testing', t2: 2, @@ -110,18 +116,18 @@ test('check variableJson output for sendVariableValues null or undefined (defaul expect(makeTraceDetails(variables, undefined)).toEqual(filteredOutput); }); -test('check variableJson output for sendVariableValues whitelist type', () => { +test('check variableJson output for sendVariableValues safelist type', () => { // Case 1: No keys/values in variables to be filtered/not filtered const emptyOutput = new Trace.Details(); - expect(makeTraceDetails({}, { always: true })).toEqual(emptyOutput); - expect(makeTraceDetails({}, { always: false })).toEqual(emptyOutput); + expect(makeTraceDetails({}, { safelistAll: true })).toEqual(emptyOutput); + expect(makeTraceDetails({}, { safelistAll: false })).toEqual(emptyOutput); // Case 2: Filter all variables const filteredOutput = new Trace.Details(); Object.keys(variables).forEach(name => { filteredOutput.variablesJson[name] = ''; }); - expect(makeTraceDetails(variables, { whitelistAll: false })).toEqual( + expect(makeTraceDetails(variables, { safelistAll: false })).toEqual( filteredOutput, ); @@ -130,7 +136,7 @@ test('check variableJson output for sendVariableValues whitelist type', () => { Object.keys(variables).forEach(name => { nonFilteredOutput.variablesJson[name] = JSON.stringify(variables[name]); }); - expect(makeTraceDetails(variables, { whitelistAll: true })).toEqual( + expect(makeTraceDetails(variables, { safelistAll: true })).toEqual( nonFilteredOutput, ); }); @@ -171,10 +177,10 @@ test('variableJson output for privacyEnforcer custom function', () => { ).toEqual(output); }); -test('whitelist=False equivalent to Array(all variables)', () => { +test('safelist=False equivalent to Array(all variables)', () => { let privateVariablesArray: string[] = ['testing', 't2']; expect( - makeTraceDetails(variables, { whitelistAll: false }).variablesJson, + makeTraceDetails(variables, { safelistAll: false }).variablesJson, ).toEqual( makeTraceDetails(variables, { exceptVariableNames: privateVariablesArray }) .variablesJson, @@ -214,15 +220,15 @@ test('original keys in variables match the modified keys', () => { }); /** - * Tests for old privateVariables reporting option + * Tests to ensure support of the old privateVariables reporting option */ test('test variableJson output for to-be-deprecated privateVariable option', () => { - // Case 1: privateVariables == False; same as whitelist all + // Case 1: privateVariables == False; same as safelist all expect(makeTraceDetailsLegacy({}, false)).toEqual( - makeTraceDetails({}, { whitelistAll: false }), + makeTraceDetails({}, { safelistAll: false }), ); expect(makeTraceDetailsLegacy(variables, false)).toEqual( - makeTraceDetails(variables, { whitelistAll: true }), + makeTraceDetails(variables, { safelistAll: true }), ); // Case 2: privateVariables is an Array; same as makeTraceDetails @@ -234,3 +240,80 @@ test('test variableJson output for to-be-deprecated privateVariable option', () .variablesJson, ); }); + +function makeTestHTTP(): Trace.HTTP { + return new Trace.HTTP({ + method: Trace.HTTP.Method.UNKNOWN, + host: null, + path: null, + }); +} + +const headers = new Headers(); +headers.append('name', 'value'); +headers.append('authorization', 'blahblah'); // THIS SHOULD NEVER BE SENT + +const headersOutput = { name: new Trace.HTTP.Values({ value: ['value'] }) }; + +/** + * TESTS FOR sendHeaders REPORTING OPTION + */ +test('test that sendHeaders defaults to hiding all', () => { + const http = makeTestHTTP(); + makeHTTPRequestHeaders(http, headers, null); + expect(http.requestHeaders).toEqual({}); + makeHTTPRequestHeaders(http, headers, undefined); + expect(http.requestHeaders).toEqual({}); + makeHTTPRequestHeaders(http, headers); + expect(http.requestHeaders).toEqual({}); +}); + +test('test sendHeaders.safelistAll', () => { + const httpSafelist = makeTestHTTP(); + makeHTTPRequestHeaders(httpSafelist, headers, { safelistAll: true }); + expect(httpSafelist.requestHeaders).toEqual(headersOutput); + + const httpBlocklist = makeTestHTTP(); + makeHTTPRequestHeaders(httpBlocklist, headers, { safelistAll: false }); + expect(httpBlocklist.requestHeaders).toEqual({}); +}); + +test('test sendHeaders.except', () => { + const except: String[] = ['name', 'notinheaders']; + const http = makeTestHTTP(); + makeHTTPRequestHeaders(http, headers, { except: except }); + expect(http.requestHeaders).toEqual({}); +}); + +/** + * And test to ensure support of old privateHeaders ooption + */ +test('test legacy privateHeaders boolean / Array ', () => { + // Test Array input + const except: String[] = ['name', 'notinheaders']; + const httpExcept = makeTestHTTP(); + makeHTTPRequestHeaders(httpExcept, headers, { except: except }); + const httpPrivateHeadersArray = makeTestHTTP(); + makeHTTPRequestHeadersLegacy(httpPrivateHeadersArray, headers, except); + expect(httpExcept.requestHeaders).toEqual( + httpPrivateHeadersArray.requestHeaders, + ); + + // Test boolean input safelist vs. privateHeaders false + const httpSafelist = makeTestHTTP(); + makeHTTPRequestHeaders(httpSafelist, headers, { safelistAll: true }); + const httpPrivateHeadersFalse = makeTestHTTP(); + makeHTTPRequestHeadersLegacy(httpPrivateHeadersFalse, headers, false); + expect(httpSafelist.requestHeaders).toEqual( + httpPrivateHeadersFalse.requestHeaders, + ); + + // Test boolean input blocklist vs. privateHeaders true + const httpBlocklist = makeTestHTTP(); + makeHTTPRequestHeaders(httpBlocklist, headers, { safelistAll: false }); + const httpPrivateHeadersTrue = makeTestHTTP(); + makeHTTPRequestHeadersLegacy(httpPrivateHeadersTrue, headers, true); + expect(httpBlocklist.requestHeaders).toEqual( + httpPrivateHeadersTrue.requestHeaders, + ); +}); diff --git a/packages/apollo-engine-reporting/src/agent.ts b/packages/apollo-engine-reporting/src/agent.ts index e69c88f7e3a..75c95577f4f 100644 --- a/packages/apollo-engine-reporting/src/agent.ts +++ b/packages/apollo-engine-reporting/src/agent.ts @@ -37,7 +37,7 @@ export type VariableValueOptions = ) => Record; } | { exceptVariableNames: Array } - | { whitelistAll: boolean }; + | { safelistAll: boolean }; export type GenerateClientInfo = ( requestContext: GraphQLRequestContext, @@ -96,7 +96,8 @@ export interface EngineReportingOptions { */ reportErrorFunction?: (err: Error) => void; /** - * [TO-BE-DEPRECATED] A case-sensitive list of names of variables whose values should + * [TO-BE-DEPRECATED] Use sendVariableValues + * A case-sensitive list of names of variables whose values should * not be sent to Apollo servers, or 'true' to leave out all variables. In the former * case, the report will indicate that each private variable was redacted; in * the latter case, no variables are sent at all. @@ -106,13 +107,13 @@ export interface EngineReportingOptions { * By default, Apollo Server does not send the values of any GraphQL variables to Apollo's servers, because variable * values often contain the private data of your app's users. If you'd like variable values to be included in traces, set this option. * This option can take several forms: - * - { whitelistAll: ... }: false to blocklist, or true to whitelist all variable values + * - { safelistAll: ... }: false to blocklist, or true to safelist all variable values * - { valueModifier: ... }: a custom function for modifying variable values * - { exceptVariableNames: ... }: a case-sensitive list of names of variables whose values should not be sent to Apollo servers * * Defaults to blocklisting all variable values if both this parameter and * the to-be-deprecated `privateVariables` are not set. The report will also - * indicate each private variable key redacted by { whitelistAll: false } or { exceptVariableNames: [...] }. + * indicate each private variable key redacted by { safelistAll: false } or { exceptVariableNames: [...] }. * * TODO(helen): Add new flag to the trace details (and modify the protobuf message structure) to indicate the type of modification. Then, add the following description to the docs: * "The report will indicate that variable values were modified by a custom function, or will list all private variables redacted." @@ -120,6 +121,19 @@ export interface EngineReportingOptions { */ sendVariableValues?: VariableValueOptions; /** + * By default, Apollo Server does not send the list of HTTP headers and values to + * Apollo's servers, to protect private data of your app's users. If you'd like this information included in traces, + * set this option. This option can take two forms: + * + * - {except: Array} A case-insensitive list of names of HTTP headers whose values should not be + * sent to Apollo servers + * - {safelistAll: 'true'/'false'} to include or leave out all HTTP headers. + * + * Unlike with sendVariableValues, names of dropped headers are not reported. + */ + sendHeaders?: { except: Array } | { safelistAll: boolean }; + /** + * [TO-BE-DEPRECATED] Use sendHeaders * A case-insensitive list of names of HTTP headers whose values should not be * sent to Apollo servers, or 'true' to leave out all HTTP headers. Unlike * with privateVariables, names of dropped headers are not reported. diff --git a/packages/apollo-engine-reporting/src/extension.ts b/packages/apollo-engine-reporting/src/extension.ts index 1c519568590..7f5c754b5b2 100644 --- a/packages/apollo-engine-reporting/src/extension.ts +++ b/packages/apollo-engine-reporting/src/extension.ts @@ -1,4 +1,4 @@ -import { Request, WithRequired } from 'apollo-server-env'; +import { Request, Headers, WithRequired } from 'apollo-server-env'; import { GraphQLResolveInfo, @@ -18,7 +18,6 @@ import { VariableValueOptions, } from './agent'; import { GraphQLRequestContext } from 'apollo-server-core/dist/requestPipelineAPI'; -// import { operation } from 'retry'; const clientNameHeaderKey = 'apollographql-client-name'; const clientReferenceIdHeaderKey = 'apollographql-client-reference-id'; @@ -93,32 +92,20 @@ export class EngineReportingExtension host: null, path: null, }); - if (this.options.privateHeaders !== true) { - for (const [key, value] of o.request.headers) { - if ( - this.options.privateHeaders && - Array.isArray(this.options.privateHeaders) && - // We assume that most users only have a few private headers, or will - // just set privateHeaders to true; we can change this linear-time - // operation if it causes real performance issues. - this.options.privateHeaders.some(privateHeader => { - // Headers are case-insensitive, and should be compared as such. - return privateHeader.toLowerCase() === key.toLowerCase(); - }) - ) { - continue; - } - - switch (key) { - case 'authorization': - case 'cookie': - case 'set-cookie': - break; - default: - this.trace.http!.requestHeaders![key] = new Trace.HTTP.Values({ - value: [value], - }); - } + + if (this.options.sendHeaders || this.options.privateHeaders != null) { + if (this.options.sendHeaders) { + makeHTTPRequestHeaders( + this.trace.http, + o.request.headers, + this.options.sendHeaders, + ); + } else if (this.options.privateHeaders != null) { + makeHTTPRequestHeadersLegacy( + this.trace.http, + o.request.headers, + this.options.privateHeaders, + ); } if (o.requestContext.metrics.persistedQueryHit) { @@ -134,7 +121,7 @@ export class EngineReportingExtension this.options.sendVariableValues !== undefined || this.options.privateVariables == null ) { - // The new option maskVariableValues will take precendence over the deprecated privateVariables option + // The new option sendVariableValues will take precendence over the deprecated privateVariables option this.trace.details = makeTraceDetails( o.variables, this.options.sendVariableValues, @@ -414,10 +401,10 @@ function defaultGenerateClientInfo({ request }: GraphQLRequestContext) { // Creates trace details from request variables, given a specification for modifying // values of private or sensitive variables. // The details include the variables in the request, TODO(helen): and the modifier type. -// If maskVariableValues is a bool or Array, it will act similarly to +// If sendVariableValues is {safelistAll: bool} or {exceptVariableNames: Array}, it will act similarly to // to the to-be-deprecated options.privateVariables, except that the redacted variable // names will still be visible in the UI when 'true.' -// If maskVariableValues is null, the policy will default to the 'true' case. +// If sendVariableValues is null, we default to the safeListAll = false case. export function makeTraceDetails( variables: Record, sendVariableValues?: VariableValueOptions, @@ -446,8 +433,8 @@ export function makeTraceDetails( Object.keys(variablesToRecord).forEach(name => { if ( sendVariableValues == null || - ('whitelistAll' in sendVariableValues && - !sendVariableValues.whitelistAll) || + ('safelistAll' in sendVariableValues && + !sendVariableValues.safelistAll) || ('exceptVariableNames' in sendVariableValues && // We assume that most users will have only a few private variables, // or will just set privateVariables to true; we can change this @@ -484,13 +471,13 @@ export function makeTraceDetails( export function makeTraceDetailsLegacy( variables: Record, privateVariables: Array | boolean, -): Trace.Details | undefined { +): Trace.Details { const newArguments = Array.isArray(privateVariables) ? { exceptVariableNames: privateVariables, } : { - whitelistAll: !privateVariables, + safelistAll: !privateVariables, }; return makeTraceDetails(variables, newArguments); } @@ -507,3 +494,56 @@ function cleanModifiedVariables( }); return cleanedVariables; } + +export function makeHTTPRequestHeaders( + http: Trace.IHTTP, + headers: Headers, + sendHeaders?: { except: Array } | { safelistAll: boolean }, +): void { + if (sendHeaders == null) { + return; + } + if ('safelistAll' in sendHeaders && !sendHeaders.safelistAll) { + return; + } + for (const [key, value] of headers) { + if ( + sendHeaders && + 'except' in sendHeaders && + // We assume that most users only have a few private headers, or will + // just set privateHeaders to true; we can change this linear-time + // operation if it causes real performance issues. + sendHeaders.except.some(exceptHeader => { + // Headers are case-insensitive, and should be compared as such. + return exceptHeader.toLowerCase() === key.toLowerCase(); + }) + ) { + continue; + } + + switch (key) { + case 'authorization': + case 'cookie': + case 'set-cookie': + break; + default: + http!.requestHeaders![key] = new Trace.HTTP.Values({ + value: [value], + }); + } + } +} + +// Creates request headers when privateHeaders [DEPRECATED] was previously set. +// Wraps privateHeaders into an object that can be passed into makeTraceDetails(), +// which handles handles the new reporting option sendHeaders. +export function makeHTTPRequestHeadersLegacy( + http: Trace.IHTTP, + headers: Headers, + privateHeaders: Array | boolean, +): void { + const sendHeaders = Array.isArray(privateHeaders) + ? { except: privateHeaders } + : { safelistAll: !privateHeaders }; + makeHTTPRequestHeaders(http, headers, sendHeaders); +} From b9d2b03435fab35bb7c19d138f5d4a3704be048b Mon Sep 17 00:00:00 2001 From: Helen Ho Date: Wed, 19 Jun 2019 15:24:16 -0700 Subject: [PATCH 04/12] addressing comments, name/argument changes, updating docs, adding and fixing test cases --- CHANGELOG.md | 2 +- docs/source/api/apollo-server.md | 69 ++-- .../src/__tests__/agent.test.ts | 83 ++++- .../src/__tests__/extension.test.ts | 311 ++++++++---------- packages/apollo-engine-reporting/src/agent.ts | 114 +++++-- .../apollo-engine-reporting/src/extension.ts | 110 ++----- 6 files changed, 376 insertions(+), 313 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 086e072498c..c5a66b7e954 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,7 @@ The version headers in this history reflect the versions of Apollo Server itself - `apollo-engine-reporting`: **BEHAVIOR CHANGE**: If the error returned from the `engine.rewriteError` hook has an `extensions` property, that property will be used instead of the original error's extensions. Document that changes to most other `GraphQLError` fields by `engine.rewriteError` are ignored. [PR #2932](https://github.com/apollographql/apollo-server/pull/2932) - `apollo-engine-reporting`: **BEHAVIOR CHANGE**: The `engine.maskErrorDetails` option, deprecated by `engine.rewriteError` in v2.5.0, now behaves a bit more like the new option: while all error messages will be redacted, they will still show up on the appropriate nodes in a trace. [PR #2932](https://github.com/apollographql/apollo-server/pull/2932) -- `apollo-engine-reporting`: **BEHAVIOR CHANGE**: By default, send no GraphQL variable values to Apollo's servers instead of sending all variable values. Adding the new EngineReportingOption `maskVariableValues` to send some or all variable values, possibly after transforming them. This replaces the `privateVariables` option, which is now deprecated. [PR #2847](https://github.com/apollographql/apollo-server/pull/2847) +- `apollo-engine-reporting`: **BEHAVIOR CHANGE**: By default, send no GraphQL variable values to Apollo's servers instead of sending all variable values. Adding the new EngineReportingOption `maskVariableValues` to send some or all variable values, possibly after transforming them. This replaces the `privateVariables` option, which is now deprecated. [PR #2931](https://github.com/apollographql/apollo-server/pull/2931) ### v2.6.7 diff --git a/docs/source/api/apollo-server.md b/docs/source/api/apollo-server.md index 2cc1c86d78e..d2b8f17809f 100644 --- a/docs/source/api/apollo-server.md +++ b/docs/source/api/apollo-server.md @@ -119,7 +119,7 @@ new ApolloServer({ * `engine`: <`EngineReportingOptions`> | boolean - Provided the `ENGINE_API_KEY` environment variable is set, the engine reporting agent will be started automatically. The API key can also be provided as the `apiKey` field in an object passed as the `engine` field. See the [EngineReportingOptions](#enginereportingoptions) section for a full description of how to configure the reporting agent, including how to blocklist variables. When using the Engine proxy, this option should be set to false. + Provided the `ENGINE_API_KEY` environment variable is set, the engine reporting agent will be started automatically. The API key can also be provided as the `apiKey` field in an object passed as the `engine` field. See the [EngineReportingOptions](#enginereportingoptions) section for a full description of how to configure the reporting agent, including how to include variable values and HTTP headers. When using the Engine proxy, this option should be set to false. * `persistedQueries`: <`Object`> | false @@ -342,49 +342,54 @@ addMockFunctionsToSchema({ By default, errors sending reports to Engine servers will be logged to standard error. Specify this function to process errors in a different way. - -* `privateVariables`: Array | boolean - - DEPRECATING IN VERSION XX.XX.XX for `sendVariableValues`, which will support the same - functionalities but allow for more flexibility. - - A case-sensitive list of names of variables whose values should not be sent - to Apollo servers, or 'true' to leave out all variables. In the former - case, the report will indicate that each private variable was redacted in - the latter case, no variables are sent at all. -* `sendVariableValues`: { valueModifier: (options: { variables: Record, operationString?: string } ) => Record } - | { exceptVariableNames: Array } - | { safelistAll: boolean } +* `sendVariableValues`: { transform: (options: { variables: Record, operationString?: string } ) => Record } + | { exceptNames: Array<String> } + | { sendNone: true } + | { sendAll: true } By default, Apollo Server does not send the values of any GraphQL variables to Apollo's servers, because variable values often contain the private data of your app's users. If you'd like variable values to be included in traces, set this option. This option can take several forms: - - { safelistAll: ... }: false to blocklist, or true to safelist all variable values - - { valueModifier: ... }: a custom function for modifying variable values - - { exceptVariableNames: ... }: a case-sensitive list of names of variables whose values should not be sent to Apollo servers + - { sendNone: true }: blocklist all variable values (DEFAULT) + - { sendAll: true }: safelist 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. + - { exceptNames: ... }: a case-sensitive list of names of variables whose values should not be sent to Apollo servers + + Defaults to blocklisting all variable values if both this parameter and the to-be-deprecated `privateVariables` are not set. + The report will indicate each private variable key whose value was redacted by { sendNone: true } or { exceptNames: [...] }. + +* `privateVariables`: Array<String> | boolean - Defaults to blocklisting all variable values if both this parameter and - the to-be-deprecated `privateVariables` are not set. The report will also - indicate each private variable key redacted by { safelistAll: false } or { exceptVariableNames: [...] }. + + DEPRECATING IN VERSION XX.XX.XX, to be replaced by the option `sendVariableValues`, which supports the same + functionalities but allows for more flexibility. Passing an array into `privateVariables` is equivalent to + passing in `{ exceptNames: array } ` to `sendVariableValues`, and passing in `true` or `false` is equivalent + to passing ` { sendNone: true } ` or ` { sendAll: true }`, respectively. + + NOTE: An error will be thrown if both this deprecated option and its replacement, `sendVariableValues` are defined. -* `sendHeaders`: { except: Array } | { safelistAll: boolean } - By default, Apollo Server does not send the list of HTTP headers and values to +* `sendHeaders`: { exceptNames: Array<String> } | { sendAll: boolean } | { sendNone: boolean } + By default, Apollo Server does not send the list of HTTP request headers and values to Apollo's servers, to protect private data of your app's users. If you'd like this information included in traces, - set this option. This option can take two forms: + set this option. This option can take several forms: - - {except: Array} A case-insensitive list of names of HTTP headers whose values should not be + - { exceptNames: Array<String> } A case-insensitive list of names of HTTP headers whose values should not be sent to Apollo servers - - {safelistAll: true/false} to include or leave out all HTTP headers. + - { sendNone : true } to drop all HTTP request headers + - { sendAll : true } to send the values of all HTTP request headers - Unlike with sendVariableValues, names of dropped headers are not reported. + Unlike with `sendVariableValues`, names of dropped headers are not reported. + The headers 'authorization', 'cookie', and 'set-cookie' are never reported. -* `privateHeaders`: Array | boolean - - DEPRECATING IN VERSION XX.XX.XX, use 'sendHeaders' instead. - A case-insensitive list of names of HTTP headers whose values should not be - sent to Apollo servers, or 'true' to leave out all HTTP headers. Unlike - with privateVariables, names of dropped headers are not reported. +* `privateHeaders`: Array<String> | boolean + + DEPRECATING IN VERSION XX.XX.XX, use `sendHeaders` instead. + Passing an array into `privateHeaders` is equivalent to passing ` { exceptNames: array } ` into `sendHeaders`, and + passing `true` or `false` is equivalent to passing in ` { sendNone: true } ` and ` { sendAll: true }`, respectively. + + NOTE: An error will be thrown if both this deprecated option and its replacement, `sendHeaders` are defined. + * `handleSignals`: boolean By default, EngineReportingAgent listens for the 'SIGINT' and 'SIGTERM' diff --git a/packages/apollo-engine-reporting/src/__tests__/agent.test.ts b/packages/apollo-engine-reporting/src/__tests__/agent.test.ts index 3a6c76ced79..70939603ec5 100644 --- a/packages/apollo-engine-reporting/src/__tests__/agent.test.ts +++ b/packages/apollo-engine-reporting/src/__tests__/agent.test.ts @@ -1,4 +1,8 @@ -import { signatureCacheKey } from '../agent'; +import { + signatureCacheKey, + handleLegacyOptions, + EngineReportingOptions, +} from '../agent'; describe('signature cache key', () => { it('generates without the operationName', () => { @@ -11,3 +15,80 @@ describe('signature cache key', () => { ); }); }); + +describe("test handleLegacyOptions(), which converts the deprecated privateVariable and privateHeaders options to the new options' formats", () => { + it('Case 1: privateVariables/privateHeaders == False; same as sendAll', () => { + const optionsPrivateFalse: EngineReportingOptions = { + privateVariables: false, + privateHeaders: false, + }; + handleLegacyOptions(optionsPrivateFalse); + expect(optionsPrivateFalse.privateVariables).toBe(undefined); + expect(optionsPrivateFalse.sendVariableValues).toEqual({ sendAll: true }); + expect(optionsPrivateFalse.privateHeaders).toBe(undefined); + expect(optionsPrivateFalse.sendHeaders).toEqual({ sendAll: true }); + }); + + it('Case 2: privateVariables/privateHeaders == True; same as sendNone', () => { + const optionsPrivateTrue: EngineReportingOptions = { + privateVariables: true, + privateHeaders: true, + }; + handleLegacyOptions(optionsPrivateTrue); + expect(optionsPrivateTrue.privateVariables).toBe(undefined); + expect(optionsPrivateTrue.sendVariableValues).toEqual({ sendNone: true }); + expect(optionsPrivateTrue.privateHeaders).toBe(undefined); + expect(optionsPrivateTrue.sendHeaders).toEqual({ sendNone: true }); + }); + + it('Case 3: privateVariables/privateHeaders set to an array', () => { + const privateArray: Array = ['t1', 't2']; + const optionsPrivateArray: EngineReportingOptions = { + privateVariables: privateArray, + privateHeaders: privateArray, + }; + handleLegacyOptions(optionsPrivateArray); + expect(optionsPrivateArray.privateVariables).toBe(undefined); + expect(optionsPrivateArray.sendVariableValues).toEqual({ + exceptNames: privateArray, + }); + expect(optionsPrivateArray.privateHeaders).toBe(undefined); + expect(optionsPrivateArray.sendHeaders).toEqual({ + exceptNames: privateArray, + }); + }); + + it('Case 4: throws error when both the new and old options are set', () => { + const optionsBothVariables: EngineReportingOptions = { + privateVariables: true, + sendVariableValues: { sendNone: true }, + }; + expect(() => { + handleLegacyOptions(optionsBothVariables); + }).toThrow(); + const optionsBothHeaders: EngineReportingOptions = { + privateHeaders: true, + sendHeaders: { sendNone: true }, + }; + expect(() => { + handleLegacyOptions(optionsBothHeaders); + }).toThrow(); + }); + + it('Case 5: the passed in options are not modified if deprecated fields were not set', () => { + const optionsNotDeprecated: EngineReportingOptions = { + sendVariableValues: { exceptNames: ['test'] }, + sendHeaders: true, + }; + const output: EngineReportingOptions = { + sendVariableValues: { exceptNames: ['test'] }, + sendHeaders: true, + }; + handleLegacyOptions(optionsNotDeprecated); + expect(optionsNotDeprecated).toEqual(output); + + const emptyInput: EngineReportingOptions = {}; + handleLegacyOptions(emptyInput); + expect(emptyInput).toEqual({}); + }); +}); diff --git a/packages/apollo-engine-reporting/src/__tests__/extension.test.ts b/packages/apollo-engine-reporting/src/__tests__/extension.test.ts index f3dc8ac31f0..1d9d3c5c583 100644 --- a/packages/apollo-engine-reporting/src/__tests__/extension.test.ts +++ b/packages/apollo-engine-reporting/src/__tests__/extension.test.ts @@ -99,146 +99,140 @@ const variables: Record = { t2: 2, }; -test('check variableJson output for sendVariableValues null or undefined (default)', () => { - // Case 1: No keys/values in variables to be filtered/not filtered - const emptyOutput = new Trace.Details(); - expect(makeTraceDetails({}, null)).toEqual(emptyOutput); - expect(makeTraceDetails({}, undefined)).toEqual(emptyOutput); - expect(makeTraceDetails({})).toEqual(emptyOutput); - - // Case 2: Filter all variables - const filteredOutput = new Trace.Details(); - Object.keys(variables).forEach(name => { - filteredOutput.variablesJson[name] = ''; +describe('check variableJson output for sendVariableValues null or undefined (default)', () => { + it('Case 1: No keys/values in variables to be filtered/not filtered', () => { + const emptyOutput = new Trace.Details(); + expect(makeTraceDetails({}, null)).toEqual(emptyOutput); + expect(makeTraceDetails({}, undefined)).toEqual(emptyOutput); + expect(makeTraceDetails({})).toEqual(emptyOutput); + }); + it('Case 2: Filter all variables', () => { + const filteredOutput = new Trace.Details(); + Object.keys(variables).forEach(name => { + filteredOutput.variablesJson[name] = ''; + }); + expect(makeTraceDetails(variables)).toEqual(filteredOutput); + expect(makeTraceDetails(variables, null)).toEqual(filteredOutput); + expect(makeTraceDetails(variables, undefined)).toEqual(filteredOutput); }); - expect(makeTraceDetails(variables)).toEqual(filteredOutput); - expect(makeTraceDetails(variables, null)).toEqual(filteredOutput); - expect(makeTraceDetails(variables, undefined)).toEqual(filteredOutput); }); -test('check variableJson output for sendVariableValues safelist type', () => { - // Case 1: No keys/values in variables to be filtered/not filtered - const emptyOutput = new Trace.Details(); - expect(makeTraceDetails({}, { safelistAll: true })).toEqual(emptyOutput); - expect(makeTraceDetails({}, { safelistAll: false })).toEqual(emptyOutput); +describe('check variableJson output for sendVariableValues sendAll/sendNone type', () => { + it('Case 1: No keys/values in variables to be filtered/not filtered', () => { + const emptyOutput = new Trace.Details(); + expect(makeTraceDetails({}, { sendAll: true })).toEqual(emptyOutput); + expect(makeTraceDetails({}, { sendNone: true })).toEqual(emptyOutput); + }); - // Case 2: Filter all variables - const filteredOutput = new Trace.Details(); - Object.keys(variables).forEach(name => { - filteredOutput.variablesJson[name] = ''; + it('Case 2: Filter all variables', () => { + const filteredOutput = new Trace.Details(); + Object.keys(variables).forEach(name => { + filteredOutput.variablesJson[name] = ''; + }); + expect(makeTraceDetails(variables, { sendNone: true })).toEqual( + filteredOutput, + ); }); - expect(makeTraceDetails(variables, { safelistAll: false })).toEqual( - filteredOutput, - ); - // Case 3: Do not filter variables - const nonFilteredOutput = new Trace.Details(); - Object.keys(variables).forEach(name => { - nonFilteredOutput.variablesJson[name] = JSON.stringify(variables[name]); + it('Case 3: Do not filter variables', () => { + const nonFilteredOutput = new Trace.Details(); + Object.keys(variables).forEach(name => { + nonFilteredOutput.variablesJson[name] = JSON.stringify(variables[name]); + }); + expect(makeTraceDetails(variables, { sendAll: true })).toEqual( + nonFilteredOutput, + ); }); - expect(makeTraceDetails(variables, { safelistAll: true })).toEqual( - nonFilteredOutput, - ); }); -test('variableJson output for privacyEnforcer Array type', () => { - const privateVariablesArray: string[] = ['testing', 'notInVariables']; - const expectedVariablesJson = { - testing: '', - t2: JSON.stringify(2), - }; - expect( - makeTraceDetails(variables, { exceptVariableNames: privateVariablesArray }) - .variablesJson, - ).toEqual(expectedVariablesJson); +describe('variableJson output for sendVariableValues exceptNames: Array type', () => { + it('array contains some values not in keys', () => { + const privateVariablesArray: string[] = ['testing', 'notInVariables']; + const expectedVariablesJson = { + testing: '', + t2: JSON.stringify(2), + }; + expect( + makeTraceDetails(variables, { exceptNames: privateVariablesArray }) + .variablesJson, + ).toEqual(expectedVariablesJson); + }); + + it('sendNone=true equivalent to exceptNames=[all variables]', () => { + let privateVariablesArray: string[] = ['testing', 't2']; + expect( + makeTraceDetails(variables, { sendNone: true }).variablesJson, + ).toEqual( + makeTraceDetails(variables, { exceptNames: privateVariablesArray }) + .variablesJson, + ); + }); }); -test('variableJson output for privacyEnforcer custom function', () => { - // Custom function that redacts every variable to 100; - const modifiedValue = 100; - const customModifier = (input: { - variables: Record; - }): Record => { - let out: Record = {}; - Object.keys(input.variables).map((name: string) => { - out[name] = modifiedValue; +describe('variableJson output for sendVariableValues transform: custom function type', () => { + it('test custom function that redacts every variable to some value', () => { + const modifiedValue = 100; + const customModifier = (input: { + variables: Record; + }): Record => { + let out: Record = {}; + Object.keys(input.variables).map((name: string) => { + out[name] = modifiedValue; + }); + return out; + }; + + // Expected output + const output = new Trace.Details(); + Object.keys(variables).forEach(name => { + output.variablesJson[name] = JSON.stringify(modifiedValue); }); - return out; - }; - // Expected output - const output = new Trace.Details(); - Object.keys(variables).forEach(name => { - output.variablesJson[name] = JSON.stringify(modifiedValue); + expect(makeTraceDetails(variables, { transform: customModifier })).toEqual( + output, + ); }); - expect( - makeTraceDetails(variables, { valueModifier: customModifier }), - ).toEqual(output); -}); - -test('safelist=False equivalent to Array(all variables)', () => { - let privateVariablesArray: string[] = ['testing', 't2']; - expect( - makeTraceDetails(variables, { safelistAll: false }).variablesJson, - ).toEqual( - makeTraceDetails(variables, { exceptVariableNames: privateVariablesArray }) - .variablesJson, - ); -}); - -test('original keys in variables match the modified keys', () => { const origKeys = Object.keys(variables); const firstKey = origKeys[0]; - // remove the first key - const modifiedKeys = Object.keys(variables).slice(1); - // add a key - modifiedKeys.push('new v'); + const secondKey = origKeys[1]; const modifier = (input: { variables: Record; }): Record => { let out: Record = {}; - Object.keys(modifiedKeys).map((name: string) => { - out[name] = 100; + Object.keys(input.variables).map((name: string) => { + out[name] = null; }); + // remove the first key, and then add a new key + delete out[firstKey]; + out['newkey'] = 'blah'; return out; }; - expect( - Object.keys( - makeTraceDetails(variables, { valueModifier: modifier }).variablesJson, - ).sort(), - ).toEqual(origKeys.sort()); - - // expect empty string for keys removed by the custom modifier - expect( - makeTraceDetails(variables, { valueModifier: modifier }).variablesJson[ - firstKey - ], - ).toEqual(''); -}); + it('original keys in variables should match the modified keys', () => { + expect( + Object.keys( + makeTraceDetails(variables, { transform: modifier }).variablesJson, + ).sort(), + ).toEqual(origKeys.sort()); + }); -/** - * Tests to ensure support of the old privateVariables reporting option - */ -test('test variableJson output for to-be-deprecated privateVariable option', () => { - // Case 1: privateVariables == False; same as safelist all - expect(makeTraceDetailsLegacy({}, false)).toEqual( - makeTraceDetails({}, { safelistAll: false }), - ); - expect(makeTraceDetailsLegacy(variables, false)).toEqual( - makeTraceDetails(variables, { safelistAll: true }), - ); + it('expect empty string for keys removed by the custom modifier', () => { + expect( + makeTraceDetails(variables, { transform: modifier }).variablesJson[ + firstKey + ], + ).toEqual(''); + }); - // Case 2: privateVariables is an Array; same as makeTraceDetails - const privacyEnforcerArray: string[] = ['testing', 'notInVariables']; - expect( - makeTraceDetailsLegacy(variables, privacyEnforcerArray).variablesJson, - ).toEqual( - makeTraceDetails(variables, { exceptVariableNames: privacyEnforcerArray }) - .variablesJson, - ); + it('expect stringify(null) for values set to null by custom modifier', () => { + expect( + makeTraceDetails(variables, { transform: modifier }).variablesJson[ + secondKey + ], + ).toEqual(JSON.stringify(null)); + }); }); function makeTestHTTP(): Trace.HTTP { @@ -249,71 +243,50 @@ function makeTestHTTP(): Trace.HTTP { }); } +/** + * TESTS FOR THE sendHeaders REPORTING OPTION + */ const headers = new Headers(); headers.append('name', 'value'); headers.append('authorization', 'blahblah'); // THIS SHOULD NEVER BE SENT const headersOutput = { name: new Trace.HTTP.Values({ value: ['value'] }) }; -/** - * TESTS FOR sendHeaders REPORTING OPTION - */ -test('test that sendHeaders defaults to hiding all', () => { - const http = makeTestHTTP(); - makeHTTPRequestHeaders(http, headers, null); - expect(http.requestHeaders).toEqual({}); - makeHTTPRequestHeaders(http, headers, undefined); - expect(http.requestHeaders).toEqual({}); - makeHTTPRequestHeaders(http, headers); - expect(http.requestHeaders).toEqual({}); -}); - -test('test sendHeaders.safelistAll', () => { - const httpSafelist = makeTestHTTP(); - makeHTTPRequestHeaders(httpSafelist, headers, { safelistAll: true }); - expect(httpSafelist.requestHeaders).toEqual(headersOutput); - - const httpBlocklist = makeTestHTTP(); - makeHTTPRequestHeaders(httpBlocklist, headers, { safelistAll: false }); - expect(httpBlocklist.requestHeaders).toEqual({}); -}); +describe('tests for the sendHeaders reporting option', () => { + it('sendHeaders defaults to hiding all', () => { + const http = makeTestHTTP(); + makeHTTPRequestHeaders(http, headers, null); + expect(http.requestHeaders).toEqual({}); + makeHTTPRequestHeaders(http, headers, undefined); + expect(http.requestHeaders).toEqual({}); + makeHTTPRequestHeaders(http, headers); + expect(http.requestHeaders).toEqual({}); + }); -test('test sendHeaders.except', () => { - const except: String[] = ['name', 'notinheaders']; - const http = makeTestHTTP(); - makeHTTPRequestHeaders(http, headers, { except: except }); - expect(http.requestHeaders).toEqual({}); -}); + it('sendHeaders.sendAll and sendHeaders.sendNone', () => { + const httpSafelist = makeTestHTTP(); + makeHTTPRequestHeaders(httpSafelist, headers, { sendAll: true }); + expect(httpSafelist.requestHeaders).toEqual(headersOutput); -/** - * And test to ensure support of old privateHeaders ooption - */ -test('test legacy privateHeaders boolean / Array ', () => { - // Test Array input - const except: String[] = ['name', 'notinheaders']; - const httpExcept = makeTestHTTP(); - makeHTTPRequestHeaders(httpExcept, headers, { except: except }); - const httpPrivateHeadersArray = makeTestHTTP(); - makeHTTPRequestHeadersLegacy(httpPrivateHeadersArray, headers, except); - expect(httpExcept.requestHeaders).toEqual( - httpPrivateHeadersArray.requestHeaders, - ); + const httpBlocklist = makeTestHTTP(); + makeHTTPRequestHeaders(httpBlocklist, headers, { sendNone: true }); + expect(httpBlocklist.requestHeaders).toEqual({}); + }); - // Test boolean input safelist vs. privateHeaders false - const httpSafelist = makeTestHTTP(); - makeHTTPRequestHeaders(httpSafelist, headers, { safelistAll: true }); - const httpPrivateHeadersFalse = makeTestHTTP(); - makeHTTPRequestHeadersLegacy(httpPrivateHeadersFalse, headers, false); - expect(httpSafelist.requestHeaders).toEqual( - httpPrivateHeadersFalse.requestHeaders, - ); + it('test sendHeaders.exceptNames', () => { + const except: String[] = ['name', 'notinheaders']; + const http = makeTestHTTP(); + makeHTTPRequestHeaders(http, headers, { exceptNames: except }); + expect(http.requestHeaders).toEqual({}); + }); - // Test boolean input blocklist vs. privateHeaders true - const httpBlocklist = makeTestHTTP(); - makeHTTPRequestHeaders(httpBlocklist, headers, { safelistAll: false }); - const httpPrivateHeadersTrue = makeTestHTTP(); - makeHTTPRequestHeadersLegacy(httpPrivateHeadersTrue, headers, true); - expect(httpBlocklist.requestHeaders).toEqual( - httpPrivateHeadersTrue.requestHeaders, - ); + it('authorization, cookie, and set-cookie headers should never be sent', () => { + headers.append('cookie', 'blahblah'); + headers.append('set-cookie', 'blahblah'); + const http = makeTestHTTP(); + makeHTTPRequestHeaders(http, headers, { sendAll: true }); + expect(http.requestHeaders['authorization']).toBe(undefined); + expect(http.requestHeaders['cookie']).toBe(undefined); + expect(http.requestHeaders['set-cookie']).toBe(undefined); + }); }); diff --git a/packages/apollo-engine-reporting/src/agent.ts b/packages/apollo-engine-reporting/src/agent.ts index 75c95577f4f..c62a76044a3 100644 --- a/packages/apollo-engine-reporting/src/agent.ts +++ b/packages/apollo-engine-reporting/src/agent.ts @@ -25,19 +25,23 @@ export interface ClientInfo { clientReferenceId?: string; } -export type VariableValueModifierOptions = { +export type BlocklistValuesBaseOptions = + | { exceptNames: Array } + | { sendAll: true } + | { sendNone: true }; + +type VariableValueTransformOptions = { variables: Record; operationString?: string; }; export type VariableValueOptions = | { - valueModifier: ( - options: VariableValueModifierOptions, + transform: ( + options: VariableValueTransformOptions, ) => Record; } - | { exceptVariableNames: Array } - | { safelistAll: boolean }; + | BlocklistValuesBaseOptions; export type GenerateClientInfo = ( requestContext: GraphQLRequestContext, @@ -95,48 +99,54 @@ export interface EngineReportingOptions { * in a different way. */ reportErrorFunction?: (err: Error) => void; - /** - * [TO-BE-DEPRECATED] Use sendVariableValues - * A case-sensitive list of names of variables whose values should - * not be sent to Apollo servers, or 'true' to leave out all variables. In the former - * case, the report will indicate that each private variable was redacted; in - * the latter case, no variables are sent at all. - */ - privateVariables?: Array | boolean; /** * By default, Apollo Server does not send the values of any GraphQL variables to Apollo's servers, because variable * values often contain the private data of your app's users. If you'd like variable values to be included in traces, set this option. * This option can take several forms: - * - { safelistAll: ... }: false to blocklist, or true to safelist all variable values - * - { valueModifier: ... }: a custom function for modifying variable values - * - { exceptVariableNames: ... }: a case-sensitive list of names of variables whose values should not be sent to Apollo servers + * - { sendNone: true }: blocklist all variable values (DEFAULT) + * - { sendAll: true}: safelist 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. + * - { exceptNames: ... }: a case-sensitive list of names of variables whose values should not be sent to Apollo servers * * Defaults to blocklisting all variable values if both this parameter and - * the to-be-deprecated `privateVariables` are not set. The report will also - * indicate each private variable key redacted by { safelistAll: false } or { exceptVariableNames: [...] }. + * the to-be-deprecated `privateVariables` are not set. The report will + * indicate each private variable key whose value was redacted by { sendNone: true } or { exceptNames: [...] }. * * TODO(helen): Add new flag to the trace details (and modify the protobuf message structure) to indicate the type of modification. Then, add the following description to the docs: * "The report will indicate that variable values were modified by a custom function, or will list all private variables redacted." * TODO(helen): LINK TO EXAMPLE FUNCTION? e.g. a function recursively search for keys to be blocklisted */ sendVariableValues?: VariableValueOptions; + /** + * [DEPRECATED] Use sendVariableValues + * Passing an array into privateVariables is equivalent to passing { exceptNames: array } into + * sendVariableValues, and passing in `true` or `false` is equivalent to passing { sendNone: true } or + * { sendAll: true }, respectively. + * + * An error will be thrown if both this deprecated option and its replacement, sendVariableValues are defined. + */ + privateVariables?: Array | boolean; /** * By default, Apollo Server does not send the list of HTTP headers and values to * Apollo's servers, to protect private data of your app's users. If you'd like this information included in traces, - * set this option. This option can take two forms: + * set this option. This option can take several forms: * - * - {except: Array} A case-insensitive list of names of HTTP headers whose values should not be - * sent to Apollo servers - * - {safelistAll: 'true'/'false'} to include or leave out all HTTP headers. + * - { sendNone : true } to drop all HTTP request headers (DEFAULT) + * - { sendAll : true } to send the values of all HTTP request headers + * - { exceptNames: Array } A case-insensitive list of names of HTTP headers whose values should not be + * sent to Apollo servers * * Unlike with sendVariableValues, names of dropped headers are not reported. + * The headers 'authorization', 'cookie', and 'set-cookie' are never reported. */ - sendHeaders?: { except: Array } | { safelistAll: boolean }; + sendHeaders?: BlocklistValuesBaseOptions; /** - * [TO-BE-DEPRECATED] Use sendHeaders - * A case-insensitive list of names of HTTP headers whose values should not be - * sent to Apollo servers, or 'true' to leave out all HTTP headers. Unlike - * with privateVariables, names of dropped headers are not reported. + * [DEPRECATED] Use sendHeaders + * Passing an array into privateHeaders is equivalent to passing { exceptNames: array } into sendHeaders, and + * passing `true` or `false` is equivalent to passing in { sendNone: true } and { sendAll: true }, respectively. + * + * An error will be thrown if both this deprecated option and its replacement, sendHeaders are defined. */ privateHeaders?: Array | boolean; /** @@ -248,6 +258,9 @@ export class EngineReportingAgent { }); }); } + + // Handle the legacy options: privateVariables and privateHeaders + handleLegacyOptions(this.options); } public newExtension(): EngineReportingExtension { @@ -523,3 +536,50 @@ function createSignatureCache(): InMemoryLRUCache { export function signatureCacheKey(queryHash: string, operationName: string) { return `${queryHash}${operationName && ':' + operationName}`; } + +// Helper function to modify the EngineReportingOptions if the deprecated fields 'privateVariables' and 'privateHeaders' +// were set. +// - Throws an error if both the deprecated option and its replacement (e.g. 'privateVariables' and 'sendVariableValues') were set. +// - Otherwise, wraps the deprecated option into objects that can be passed to the new replacement field (see the helper +// function makeBlocklistBaseOptionsFromLegacy), and deletes the deprecated field from the options +export function handleLegacyOptions( + options: EngineReportingOptions, +): void { + // Handle the legacy option: privateVariables + if (options.privateVariables != null && options.sendVariableValues) { + throw new Error( + "You have set both the 'sendVariableValues' and the deprecated 'privateVariables' options. Please only set 'sendVariableValues'.", + ); + } else if (options.privateVariables != null) { + options.sendVariableValues = makeBlocklistBaseOptionsFromLegacy( + options.privateVariables, + ); + delete options.privateVariables; + } + + // Handle the legacy option: privateHeaders + if (options.privateHeaders != null && options.sendHeaders) { + throw new Error( + "You have set both the 'sendHeaders' and the deprecated 'privateHeaders' options. Please only set 'sendHeaders'.", + ); + } else if (options.privateHeaders != null) { + options.sendHeaders = makeBlocklistBaseOptionsFromLegacy( + options.privateHeaders, + ); + delete options.privateHeaders; + } +} + +// This helper wraps non-null inputs from the deprecated options 'privateVariables' and 'privateHeaders' into +// objects that can be passed to the new replacement options, 'sendVariableValues' and 'sendHeaders' +function makeBlocklistBaseOptionsFromLegacy( + legacyPrivateOption: Array | boolean, +): BlocklistValuesBaseOptions { + return Array.isArray(legacyPrivateOption) + ? { + exceptNames: legacyPrivateOption, + } + : legacyPrivateOption + ? { sendNone: true } + : { sendAll: true }; +} diff --git a/packages/apollo-engine-reporting/src/extension.ts b/packages/apollo-engine-reporting/src/extension.ts index 7f5c754b5b2..4d55fd7a399 100644 --- a/packages/apollo-engine-reporting/src/extension.ts +++ b/packages/apollo-engine-reporting/src/extension.ts @@ -16,6 +16,7 @@ import { GenerateClientInfo, AddTraceArgs, VariableValueOptions, + BlocklistValuesBaseOptions, } from './agent'; import { GraphQLRequestContext } from 'apollo-server-core/dist/requestPipelineAPI'; @@ -93,20 +94,12 @@ export class EngineReportingExtension path: null, }); - if (this.options.sendHeaders || this.options.privateHeaders != null) { - if (this.options.sendHeaders) { - makeHTTPRequestHeaders( - this.trace.http, - o.request.headers, - this.options.sendHeaders, - ); - } else if (this.options.privateHeaders != null) { - makeHTTPRequestHeadersLegacy( - this.trace.http, - o.request.headers, - this.options.privateHeaders, - ); - } + if (this.options.sendHeaders) { + makeHTTPRequestHeaders( + this.trace.http, + o.request.headers, + this.options.sendHeaders, + ); if (o.requestContext.metrics.persistedQueryHit) { this.trace.persistedQueryHit = true; @@ -117,24 +110,11 @@ export class EngineReportingExtension } if (o.variables) { - if ( - this.options.sendVariableValues !== undefined || - this.options.privateVariables == null - ) { - // The new option sendVariableValues will take precendence over the deprecated privateVariables option - this.trace.details = makeTraceDetails( - o.variables, - this.options.sendVariableValues, - o.queryString, - ); - } else { - // privateVariables will be DEPRECATED - // But keep supporting if it has been set. - this.trace.details = makeTraceDetailsLegacy( - o.variables, - this.options.privateVariables, - ); - } + this.trace.details = makeTraceDetails( + o.variables, + this.options.sendVariableValues, + o.queryString, + ); } const clientInfo = this.generateClientInfo(o.requestContext); @@ -412,10 +392,10 @@ export function makeTraceDetails( ): Trace.Details { const details = new Trace.Details(); const variablesToRecord = (() => { - if (sendVariableValues && 'valueModifier' in sendVariableValues) { + 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.valueModifier({ + const modifiedVariables = sendVariableValues.transform({ variables: variables, operationString: operationString, }); @@ -432,14 +412,13 @@ export function makeTraceDetails( // string is ineffective. Object.keys(variablesToRecord).forEach(name => { if ( - sendVariableValues == null || - ('safelistAll' in sendVariableValues && - !sendVariableValues.safelistAll) || - ('exceptVariableNames' in sendVariableValues && - // We assume that most users will have only a few private variables, - // or will just set privateVariables to true; we can change this + !sendVariableValues || + 'sendNone' in sendVariableValues || + ('exceptNames' in sendVariableValues && + // We assume that most users will have only a few variables values to hide, + // or will just set {sendNone: true}; we can change this // linear-time operation if it causes real performance issues. - sendVariableValues.exceptVariableNames.includes(name)) + sendVariableValues.exceptNames.includes(name)) ) { // Special case for private variables. Note that this is a different // representation from a variable containing the empty string, as that @@ -448,7 +427,7 @@ export function makeTraceDetails( } else { try { details.variablesJson![name] = - variablesToRecord[name] == null + variablesToRecord[name] === undefined ? '' : JSON.stringify(variablesToRecord[name]); } catch (e) { @@ -461,27 +440,9 @@ export function makeTraceDetails( } } }); - return details; } -// Creates trace details when privateVariables [DEPRECATED] was set. -// Wraps privateVariable into a format that can be passed into makeTraceDetails(), -// which handles the new option sendVariableValues. -export function makeTraceDetailsLegacy( - variables: Record, - privateVariables: Array | boolean, -): Trace.Details { - const newArguments = Array.isArray(privateVariables) - ? { - exceptVariableNames: privateVariables, - } - : { - safelistAll: !privateVariables, - }; - return makeTraceDetails(variables, newArguments); -} - // Helper for makeTraceDetails() to enforce that the keys of a modified 'variables' // matches that of the original 'variables' function cleanModifiedVariables( @@ -498,22 +459,19 @@ function cleanModifiedVariables( export function makeHTTPRequestHeaders( http: Trace.IHTTP, headers: Headers, - sendHeaders?: { except: Array } | { safelistAll: boolean }, + sendHeaders?: BlocklistValuesBaseOptions, ): void { - if (sendHeaders == null) { - return; - } - if ('safelistAll' in sendHeaders && !sendHeaders.safelistAll) { + if (!sendHeaders || 'sendNone' in sendHeaders) { return; } for (const [key, value] of headers) { if ( sendHeaders && - 'except' in sendHeaders && - // We assume that most users only have a few private headers, or will - // just set privateHeaders to true; we can change this linear-time + 'exceptNames' in sendHeaders && + // We assume that most users only have a few headers to hide, or will + // just set {sendNone: true} ; we can change this linear-time // operation if it causes real performance issues. - sendHeaders.except.some(exceptHeader => { + sendHeaders.exceptNames.some(exceptHeader => { // Headers are case-insensitive, and should be compared as such. return exceptHeader.toLowerCase() === key.toLowerCase(); }) @@ -533,17 +491,3 @@ export function makeHTTPRequestHeaders( } } } - -// Creates request headers when privateHeaders [DEPRECATED] was previously set. -// Wraps privateHeaders into an object that can be passed into makeTraceDetails(), -// which handles handles the new reporting option sendHeaders. -export function makeHTTPRequestHeadersLegacy( - http: Trace.IHTTP, - headers: Headers, - privateHeaders: Array | boolean, -): void { - const sendHeaders = Array.isArray(privateHeaders) - ? { except: privateHeaders } - : { safelistAll: !privateHeaders }; - makeHTTPRequestHeaders(http, headers, sendHeaders); -} From dda89a8042a1f4df0aa330c8576c4eaea72798d6 Mon Sep 17 00:00:00 2001 From: Helen Ho Date: Mon, 24 Jun 2019 12:05:04 -0700 Subject: [PATCH 05/12] adding another parameter for specifying safelisted variable names --- docs/source/api/apollo-server.md | 11 +++-- .../src/__tests__/extension.test.ts | 42 +++++++++++++++++++ packages/apollo-engine-reporting/src/agent.ts | 19 +++++---- .../apollo-engine-reporting/src/extension.ts | 29 +++++++------ 4 files changed, 77 insertions(+), 24 deletions(-) diff --git a/docs/source/api/apollo-server.md b/docs/source/api/apollo-server.md index d2b8f17809f..d3132e71a5b 100644 --- a/docs/source/api/apollo-server.md +++ b/docs/source/api/apollo-server.md @@ -345,6 +345,7 @@ addMockFunctionsToSchema({ * `sendVariableValues`: { transform: (options: { variables: Record, operationString?: string } ) => Record } | { exceptNames: Array<String> } + | { includeNames: Array<String> } | { sendNone: true } | { sendAll: true } @@ -354,6 +355,7 @@ addMockFunctionsToSchema({ - { sendAll: true }: safelist 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. - { exceptNames: ... }: a case-sensitive list of names of variables whose values should not be sent to Apollo servers + - { includeNames: ... }: a case-sensitive list of names of variables whose values will be sent to Apollo Servers Defaults to blocklisting all variable values if both this parameter and the to-be-deprecated `privateVariables` are not set. The report will indicate each private variable key whose value was redacted by { sendNone: true } or { exceptNames: [...] }. @@ -368,15 +370,16 @@ addMockFunctionsToSchema({ NOTE: An error will be thrown if both this deprecated option and its replacement, `sendVariableValues` are defined. -* `sendHeaders`: { exceptNames: Array<String> } | { sendAll: boolean } | { sendNone: boolean } +* `sendHeaders`: { exceptNames: Array<String> } | { includeNames: Array<String> } | { sendAll: boolean } | { sendNone: boolean } By default, Apollo Server does not send the list of HTTP request headers and values to Apollo's servers, to protect private data of your app's users. If you'd like this information included in traces, set this option. This option can take several forms: - - { exceptNames: Array<String> } A case-insensitive list of names of HTTP headers whose values should not be + - { sendNone : true }: drop all HTTP request headers (DEFAULT) + - { sendAll : true }: send the values of all HTTP request headers + - { exceptNames: ... }: A case-insensitive list of names of HTTP headers whose values should not be sent to Apollo servers - - { sendNone : true } to drop all HTTP request headers - - { sendAll : true } to send the values of all HTTP request headers + - { includeNames: ... }: A case-insensitive list of names of HTTP headers whose values will be sent to Apollo servers Unlike with `sendVariableValues`, names of dropped headers are not reported. The headers 'authorization', 'cookie', and 'set-cookie' are never reported. diff --git a/packages/apollo-engine-reporting/src/__tests__/extension.test.ts b/packages/apollo-engine-reporting/src/__tests__/extension.test.ts index 1d9d3c5c583..bd249605022 100644 --- a/packages/apollo-engine-reporting/src/__tests__/extension.test.ts +++ b/packages/apollo-engine-reporting/src/__tests__/extension.test.ts @@ -169,6 +169,40 @@ describe('variableJson output for sendVariableValues exceptNames: Array type', ( }); }); +describe('variableJson output for sendVariableValues includeNames: Array type', () => { + it('array contains some values not in keys', () => { + const privateVariablesArray: string[] = ['t2', 'notInVariables']; + const expectedVariablesJson = { + testing: '', + t2: JSON.stringify(2), + }; + expect( + makeTraceDetails(variables, { includeNames: privateVariablesArray }) + .variablesJson, + ).toEqual(expectedVariablesJson); + }); + + it('sendAll=true equivalent to includeNames=[all variables]', () => { + let privateVariablesArray: string[] = ['testing', 't2']; + expect( + makeTraceDetails(variables, { sendAll: true }).variablesJson, + ).toEqual( + makeTraceDetails(variables, { includeNames: privateVariablesArray }) + .variablesJson, + ); + }); + + it('sendNone=true equivalent to includeNames=[]', () => { + let privateVariablesArray: string[] = []; + expect( + makeTraceDetails(variables, { sendNone: true }).variablesJson, + ).toEqual( + makeTraceDetails(variables, { includeNames: privateVariablesArray }) + .variablesJson, + ); + }); +}); + describe('variableJson output for sendVariableValues transform: custom function type', () => { it('test custom function that redacts every variable to some value', () => { const modifiedValue = 100; @@ -280,6 +314,14 @@ describe('tests for the sendHeaders reporting option', () => { expect(http.requestHeaders).toEqual({}); }); + it('test sendHeaders.includeNames', () => { + // headers that should never be sent (such as "authorization") should still be removed if in includeHeaders + const include: String[] = ['name', 'authorization', 'notinheaders']; + const http = makeTestHTTP(); + makeHTTPRequestHeaders(http, headers, { includeNames: include }); + expect(http.requestHeaders).toEqual(headersOutput); + }); + it('authorization, cookie, and set-cookie headers should never be sent', () => { headers.append('cookie', 'blahblah'); headers.append('set-cookie', 'blahblah'); diff --git a/packages/apollo-engine-reporting/src/agent.ts b/packages/apollo-engine-reporting/src/agent.ts index c62a76044a3..e57cfa3ae55 100644 --- a/packages/apollo-engine-reporting/src/agent.ts +++ b/packages/apollo-engine-reporting/src/agent.ts @@ -25,7 +25,8 @@ export interface ClientInfo { clientReferenceId?: string; } -export type BlocklistValuesBaseOptions = +export type SendValuesBaseOptions = + | { includeNames: Array } | { exceptNames: Array } | { sendAll: true } | { sendNone: true }; @@ -41,7 +42,7 @@ export type VariableValueOptions = options: VariableValueTransformOptions, ) => Record; } - | BlocklistValuesBaseOptions; + | SendValuesBaseOptions; export type GenerateClientInfo = ( requestContext: GraphQLRequestContext, @@ -108,6 +109,7 @@ export interface EngineReportingOptions { * - { 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. * - { exceptNames: ... }: a case-sensitive list of names of variables whose values should not be sent to Apollo servers + * - { includeNames: ... }: A case-sensitive list of names of variables whose values will be sent to Apollo servers * * Defaults to blocklisting all variable values if both this parameter and * the to-be-deprecated `privateVariables` are not set. The report will @@ -136,11 +138,12 @@ export interface EngineReportingOptions { * - { sendAll : true } to send the values of all HTTP request headers * - { exceptNames: Array } A case-insensitive list of names of HTTP headers whose values should not be * sent to Apollo servers + * - { includeNames: Array }: A case-insensitive list of names of HTTP headers whose values will be sent to Apollo servers * * Unlike with sendVariableValues, names of dropped headers are not reported. * The headers 'authorization', 'cookie', and 'set-cookie' are never reported. */ - sendHeaders?: BlocklistValuesBaseOptions; + sendHeaders?: SendValuesBaseOptions; /** * [DEPRECATED] Use sendHeaders * Passing an array into privateHeaders is equivalent to passing { exceptNames: array } into sendHeaders, and @@ -541,7 +544,7 @@ export function signatureCacheKey(queryHash: string, operationName: string) { // were set. // - Throws an error if both the deprecated option and its replacement (e.g. 'privateVariables' and 'sendVariableValues') were set. // - Otherwise, wraps the deprecated option into objects that can be passed to the new replacement field (see the helper -// function makeBlocklistBaseOptionsFromLegacy), and deletes the deprecated field from the options +// function makeSendValuesBaseOptionsFromLegacy), and deletes the deprecated field from the options export function handleLegacyOptions( options: EngineReportingOptions, ): void { @@ -551,7 +554,7 @@ export function handleLegacyOptions( "You have set both the 'sendVariableValues' and the deprecated 'privateVariables' options. Please only set 'sendVariableValues'.", ); } else if (options.privateVariables != null) { - options.sendVariableValues = makeBlocklistBaseOptionsFromLegacy( + options.sendVariableValues = makeSendValuesBaseOptionsFromLegacy( options.privateVariables, ); delete options.privateVariables; @@ -563,7 +566,7 @@ export function handleLegacyOptions( "You have set both the 'sendHeaders' and the deprecated 'privateHeaders' options. Please only set 'sendHeaders'.", ); } else if (options.privateHeaders != null) { - options.sendHeaders = makeBlocklistBaseOptionsFromLegacy( + options.sendHeaders = makeSendValuesBaseOptionsFromLegacy( options.privateHeaders, ); delete options.privateHeaders; @@ -572,9 +575,9 @@ export function handleLegacyOptions( // This helper wraps non-null inputs from the deprecated options 'privateVariables' and 'privateHeaders' into // objects that can be passed to the new replacement options, 'sendVariableValues' and 'sendHeaders' -function makeBlocklistBaseOptionsFromLegacy( +function makeSendValuesBaseOptionsFromLegacy( legacyPrivateOption: Array | boolean, -): BlocklistValuesBaseOptions { +): SendValuesBaseOptions { return Array.isArray(legacyPrivateOption) ? { exceptNames: legacyPrivateOption, diff --git a/packages/apollo-engine-reporting/src/extension.ts b/packages/apollo-engine-reporting/src/extension.ts index 4d55fd7a399..5bd8b2a72dd 100644 --- a/packages/apollo-engine-reporting/src/extension.ts +++ b/packages/apollo-engine-reporting/src/extension.ts @@ -16,7 +16,7 @@ import { GenerateClientInfo, AddTraceArgs, VariableValueOptions, - BlocklistValuesBaseOptions, + SendValuesBaseOptions, } from './agent'; import { GraphQLRequestContext } from 'apollo-server-core/dist/requestPipelineAPI'; @@ -418,7 +418,9 @@ export function makeTraceDetails( // We assume that most users will have only a few variables values to hide, // or will just set {sendNone: true}; we can change this // linear-time operation if it causes real performance issues. - sendVariableValues.exceptNames.includes(name)) + sendVariableValues.exceptNames.includes(name)) || + ('includeNames' in sendVariableValues && + !sendVariableValues.includeNames.includes(name)) ) { // Special case for private variables. Note that this is a different // representation from a variable containing the empty string, as that @@ -459,22 +461,25 @@ function cleanModifiedVariables( export function makeHTTPRequestHeaders( http: Trace.IHTTP, headers: Headers, - sendHeaders?: BlocklistValuesBaseOptions, + sendHeaders?: SendValuesBaseOptions, ): void { if (!sendHeaders || 'sendNone' in sendHeaders) { return; } for (const [key, value] of headers) { if ( - sendHeaders && - 'exceptNames' in sendHeaders && - // We assume that most users only have a few headers to hide, or will - // just set {sendNone: true} ; we can change this linear-time - // operation if it causes real performance issues. - sendHeaders.exceptNames.some(exceptHeader => { - // Headers are case-insensitive, and should be compared as such. - return exceptHeader.toLowerCase() === key.toLowerCase(); - }) + ('exceptNames' in sendHeaders && + // We assume that most users only have a few headers to hide, or will + // just set {sendNone: true} ; we can change this linear-time + // operation if it causes real performance issues. + sendHeaders.exceptNames.some(exceptHeader => { + // Headers are case-insensitive, and should be compared as such. + return exceptHeader.toLowerCase() === key.toLowerCase(); + })) || + ('includeNames' in sendHeaders && + !sendHeaders.includeNames.some(header => { + return header.toLowerCase() === key.toLowerCase(); + })) ) { continue; } From 1eb61b1559e5cba76e5dbcce8aa160d9da097ca0 Mon Sep 17 00:00:00 2001 From: Helen Ho Date: Tue, 25 Jun 2019 10:19:14 -0700 Subject: [PATCH 06/12] more doc fixes, adding some checks for invalid inputs --- docs/source/api/apollo-server.md | 21 ++++---- .../src/__tests__/extension.test.ts | 54 +++++++++++++------ packages/apollo-engine-reporting/src/agent.ts | 20 +++---- .../apollo-engine-reporting/src/extension.ts | 22 +++++--- 4 files changed, 74 insertions(+), 43 deletions(-) diff --git a/docs/source/api/apollo-server.md b/docs/source/api/apollo-server.md index d3132e71a5b..cf8e816e638 100644 --- a/docs/source/api/apollo-server.md +++ b/docs/source/api/apollo-server.md @@ -345,19 +345,19 @@ addMockFunctionsToSchema({ * `sendVariableValues`: { transform: (options: { variables: Record, operationString?: string } ) => Record } | { exceptNames: Array<String> } - | { includeNames: Array<String> } + | { onlyNames: Array<String> } | { sendNone: true } | { sendAll: true } By default, Apollo Server does not send the values of any GraphQL variables to Apollo's servers, because variable values often contain the private data of your app's users. If you'd like variable values to be included in traces, set this option. This option can take several forms: - - { sendNone: true }: blocklist all variable values (DEFAULT) - - { sendAll: true }: safelist all variable values + - { sendNone: true }: don't send any variable values (DEFAULT) + - { sendAll: 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. - { exceptNames: ... }: a case-sensitive list of names of variables whose values should not be sent to Apollo servers - - { includeNames: ... }: a case-sensitive list of names of variables whose values will be sent to Apollo Servers + - { onlyNames: ... }: a case-sensitive list of names of variables whose values will be sent to Apollo Servers - Defaults to blocklisting all variable values if both this parameter and the to-be-deprecated `privateVariables` are not set. + Defaults to not sending any variable values if both this parameter and the deprecated `privateVariables` are not set. The report will indicate each private variable key whose value was redacted by { sendNone: true } or { exceptNames: [...] }. * `privateVariables`: Array<String> | boolean @@ -370,17 +370,18 @@ addMockFunctionsToSchema({ NOTE: An error will be thrown if both this deprecated option and its replacement, `sendVariableValues` are defined. -* `sendHeaders`: { exceptNames: Array<String> } | { includeNames: Array<String> } | { sendAll: boolean } | { sendNone: boolean } +* `sendHeaders`: { exceptNames: Array<String> } | { onlyNames: Array<String> } | { sendAll: boolean } | { sendNone: boolean } By default, Apollo Server does not send the list of HTTP request headers and values to Apollo's servers, to protect private data of your app's users. If you'd like this information included in traces, set this option. This option can take several forms: - - { sendNone : true }: drop all HTTP request headers (DEFAULT) - - { sendAll : true }: send the values of all HTTP request headers + - { sendNone: true }: drop all HTTP request headers (DEFAULT) + - { sendAll: true }: send the values of all HTTP request headers - { exceptNames: ... }: A case-insensitive list of names of HTTP headers whose values should not be sent to Apollo servers - - { includeNames: ... }: A case-insensitive list of names of HTTP headers whose values will be sent to Apollo servers - + - { onlyNames: ... }: A case-insensitive list of names of HTTP headers whose values will be sent to Apollo servers + + Defaults to not sending any request header names and values if both this parameter and the deprecated `privateHeaders` are not set. Unlike with `sendVariableValues`, names of dropped headers are not reported. The headers 'authorization', 'cookie', and 'set-cookie' are never reported. diff --git a/packages/apollo-engine-reporting/src/__tests__/extension.test.ts b/packages/apollo-engine-reporting/src/__tests__/extension.test.ts index bd249605022..a2768e69c8d 100644 --- a/packages/apollo-engine-reporting/src/__tests__/extension.test.ts +++ b/packages/apollo-engine-reporting/src/__tests__/extension.test.ts @@ -124,25 +124,37 @@ describe('check variableJson output for sendVariableValues sendAll/sendNone type expect(makeTraceDetails({}, { sendNone: true })).toEqual(emptyOutput); }); + const filteredOutput = new Trace.Details(); + Object.keys(variables).forEach(name => { + filteredOutput.variablesJson[name] = ''; + }); + + const nonFilteredOutput = new Trace.Details(); + Object.keys(variables).forEach(name => { + nonFilteredOutput.variablesJson[name] = JSON.stringify(variables[name]); + }); + it('Case 2: Filter all variables', () => { - const filteredOutput = new Trace.Details(); - Object.keys(variables).forEach(name => { - filteredOutput.variablesJson[name] = ''; - }); expect(makeTraceDetails(variables, { sendNone: true })).toEqual( filteredOutput, ); }); it('Case 3: Do not filter variables', () => { - const nonFilteredOutput = new Trace.Details(); - Object.keys(variables).forEach(name => { - nonFilteredOutput.variablesJson[name] = JSON.stringify(variables[name]); - }); expect(makeTraceDetails(variables, { sendAll: true })).toEqual( nonFilteredOutput, ); }); + + it('Case 4: Check behavior for invalid inputs', () => { + expect(makeTraceDetails(variables, { sendNone: false })).toEqual( + nonFilteredOutput, + ); + + expect(makeTraceDetails(variables, { sendAll: false })).toEqual( + filteredOutput, + ); + }); }); describe('variableJson output for sendVariableValues exceptNames: Array type', () => { @@ -169,7 +181,7 @@ describe('variableJson output for sendVariableValues exceptNames: Array type', ( }); }); -describe('variableJson output for sendVariableValues includeNames: Array type', () => { +describe('variableJson output for sendVariableValues onlyNames: Array type', () => { it('array contains some values not in keys', () => { const privateVariablesArray: string[] = ['t2', 'notInVariables']; const expectedVariablesJson = { @@ -177,27 +189,27 @@ describe('variableJson output for sendVariableValues includeNames: Array type', t2: JSON.stringify(2), }; expect( - makeTraceDetails(variables, { includeNames: privateVariablesArray }) + makeTraceDetails(variables, { onlyNames: privateVariablesArray }) .variablesJson, ).toEqual(expectedVariablesJson); }); - it('sendAll=true equivalent to includeNames=[all variables]', () => { + it('sendAll=true equivalent to onlyNames=[all variables]', () => { let privateVariablesArray: string[] = ['testing', 't2']; expect( makeTraceDetails(variables, { sendAll: true }).variablesJson, ).toEqual( - makeTraceDetails(variables, { includeNames: privateVariablesArray }) + makeTraceDetails(variables, { onlyNames: privateVariablesArray }) .variablesJson, ); }); - it('sendNone=true equivalent to includeNames=[]', () => { + it('sendNone=true equivalent to onlyNames=[]', () => { let privateVariablesArray: string[] = []; expect( makeTraceDetails(variables, { sendNone: true }).variablesJson, ).toEqual( - makeTraceDetails(variables, { includeNames: privateVariablesArray }) + makeTraceDetails(variables, { onlyNames: privateVariablesArray }) .variablesJson, ); }); @@ -307,6 +319,16 @@ describe('tests for the sendHeaders reporting option', () => { expect(httpBlocklist.requestHeaders).toEqual({}); }); + it('invalid inputs for sendHeaders.sendAll and sendHeaders.sendNone', () => { + const httpSafelist = makeTestHTTP(); + makeHTTPRequestHeaders(httpSafelist, headers, { sendNone: false }); + expect(httpSafelist.requestHeaders).toEqual(headersOutput); + + const httpBlocklist = makeTestHTTP(); + makeHTTPRequestHeaders(httpBlocklist, headers, { sendAll: false }); + expect(httpBlocklist.requestHeaders).toEqual({}); + }); + it('test sendHeaders.exceptNames', () => { const except: String[] = ['name', 'notinheaders']; const http = makeTestHTTP(); @@ -314,11 +336,11 @@ describe('tests for the sendHeaders reporting option', () => { expect(http.requestHeaders).toEqual({}); }); - it('test sendHeaders.includeNames', () => { + it('test sendHeaders.onlyNames', () => { // headers that should never be sent (such as "authorization") should still be removed if in includeHeaders const include: String[] = ['name', 'authorization', 'notinheaders']; const http = makeTestHTTP(); - makeHTTPRequestHeaders(http, headers, { includeNames: include }); + makeHTTPRequestHeaders(http, headers, { onlyNames: include }); expect(http.requestHeaders).toEqual(headersOutput); }); diff --git a/packages/apollo-engine-reporting/src/agent.ts b/packages/apollo-engine-reporting/src/agent.ts index e57cfa3ae55..fa3a95d065d 100644 --- a/packages/apollo-engine-reporting/src/agent.ts +++ b/packages/apollo-engine-reporting/src/agent.ts @@ -26,7 +26,7 @@ export interface ClientInfo { } export type SendValuesBaseOptions = - | { includeNames: Array } + | { onlyNames: Array } | { exceptNames: Array } | { sendAll: true } | { sendNone: true }; @@ -104,15 +104,15 @@ export interface EngineReportingOptions { * By default, Apollo Server does not send the values of any GraphQL variables to Apollo's servers, because variable * values often contain the private data of your app's users. If you'd like variable values to be included in traces, set this option. * This option can take several forms: - * - { sendNone: true }: blocklist all variable values (DEFAULT) - * - { sendAll: true}: safelist all variable values + * - { sendNone: true }: don't send any variable values (DEFAULT) + * - { sendAll: 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. * - { exceptNames: ... }: a case-sensitive list of names of variables whose values should not be sent to Apollo servers - * - { includeNames: ... }: A case-sensitive list of names of variables whose values will be sent to Apollo servers + * - { onlyNames: ... }: A case-sensitive list of names of variables whose values will be sent to Apollo servers * - * Defaults to blocklisting all variable values if both this parameter and - * the to-be-deprecated `privateVariables` are not set. The report will + * Defaults to not sending any variable values if both this parameter and + * the deprecated `privateVariables` are not set. The report will * indicate each private variable key whose value was redacted by { sendNone: true } or { exceptNames: [...] }. * * TODO(helen): Add new flag to the trace details (and modify the protobuf message structure) to indicate the type of modification. Then, add the following description to the docs: @@ -134,12 +134,14 @@ export interface EngineReportingOptions { * Apollo's servers, to protect private data of your app's users. If you'd like this information included in traces, * set this option. This option can take several forms: * - * - { sendNone : true } to drop all HTTP request headers (DEFAULT) - * - { sendAll : true } to send the values of all HTTP request headers + * - { sendNone: true } to drop all HTTP request headers (DEFAULT) + * - { sendAll: true } to send the values of all HTTP request headers * - { exceptNames: Array } A case-insensitive list of names of HTTP headers whose values should not be * sent to Apollo servers - * - { includeNames: Array }: A case-insensitive list of names of HTTP headers whose values will be sent to Apollo servers + * - { onlyNames: Array }: A case-insensitive list of names of HTTP headers whose values will be sent to Apollo servers * + * Defaults to not sending any request header names and values if both this parameter and + * the deprecated `privateHeaders` are not set. * Unlike with sendVariableValues, names of dropped headers are not reported. * The headers 'authorization', 'cookie', and 'set-cookie' are never reported. */ diff --git a/packages/apollo-engine-reporting/src/extension.ts b/packages/apollo-engine-reporting/src/extension.ts index 5bd8b2a72dd..a9345796e88 100644 --- a/packages/apollo-engine-reporting/src/extension.ts +++ b/packages/apollo-engine-reporting/src/extension.ts @@ -413,14 +413,15 @@ export function makeTraceDetails( Object.keys(variablesToRecord).forEach(name => { if ( !sendVariableValues || - 'sendNone' in sendVariableValues || + ('sendNone' in sendVariableValues && sendVariableValues.sendNone) || + ('sendAll' in sendVariableValues && !sendVariableValues.sendAll) || ('exceptNames' in sendVariableValues && // We assume that most users will have only a few variables values to hide, // or will just set {sendNone: true}; we can change this // linear-time operation if it causes real performance issues. sendVariableValues.exceptNames.includes(name)) || - ('includeNames' in sendVariableValues && - !sendVariableValues.includeNames.includes(name)) + ('onlyNames' in sendVariableValues && + !sendVariableValues.onlyNames.includes(name)) ) { // Special case for private variables. Note that this is a different // representation from a variable containing the empty string, as that @@ -463,10 +464,15 @@ export function makeHTTPRequestHeaders( headers: Headers, sendHeaders?: SendValuesBaseOptions, ): void { - if (!sendHeaders || 'sendNone' in sendHeaders) { + if ( + !sendHeaders || + ('sendNone' in sendHeaders && sendHeaders.sendNone) || + ('sendAll' in sendHeaders && !sendHeaders.sendAll) + ) { return; } for (const [key, value] of headers) { + const lowercaseKey = key.toLowerCase(); if ( ('exceptNames' in sendHeaders && // We assume that most users only have a few headers to hide, or will @@ -474,11 +480,11 @@ export function makeHTTPRequestHeaders( // operation if it causes real performance issues. sendHeaders.exceptNames.some(exceptHeader => { // Headers are case-insensitive, and should be compared as such. - return exceptHeader.toLowerCase() === key.toLowerCase(); + return exceptHeader.toLowerCase() === lowercaseKey; })) || - ('includeNames' in sendHeaders && - !sendHeaders.includeNames.some(header => { - return header.toLowerCase() === key.toLowerCase(); + ('onlyNames' in sendHeaders && + !sendHeaders.onlyNames.some(header => { + return header.toLowerCase() === lowercaseKey; })) ) { continue; From f49c71426ef5abd0096f667aaa5a43c411c72cbc Mon Sep 17 00:00:00 2001 From: Helen Ho Date: Wed, 26 Jun 2019 10:33:55 -0700 Subject: [PATCH 07/12] changed suboption name, updating docs with more info on breaking change --- CHANGELOG.md | 5 ++ docs/source/api/apollo-server.md | 26 ++++++---- .../src/__tests__/agent.test.ts | 16 +++--- .../src/__tests__/extension.test.ts | 50 ++++++++----------- packages/apollo-engine-reporting/src/agent.ts | 24 ++++----- .../apollo-engine-reporting/src/extension.ts | 12 ++--- 6 files changed, 66 insertions(+), 67 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c5a66b7e954..03c6b26d454 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,11 @@ The version headers in this history reflect the versions of Apollo Server itself - `apollo-engine-reporting`: **BEHAVIOR CHANGE**: The `engine.maskErrorDetails` option, deprecated by `engine.rewriteError` in v2.5.0, now behaves a bit more like the new option: while all error messages will be redacted, they will still show up on the appropriate nodes in a trace. [PR #2932](https://github.com/apollographql/apollo-server/pull/2932) - `apollo-engine-reporting`: **BEHAVIOR CHANGE**: By default, send no GraphQL variable values to Apollo's servers instead of sending all variable values. Adding the new EngineReportingOption `maskVariableValues` to send some or all variable values, possibly after transforming them. This replaces the `privateVariables` option, which is now deprecated. [PR #2931](https://github.com/apollographql/apollo-server/pull/2931) + > Note: In order to keep shipping all GraphQL variable values to Apollo Engine, pass in the option: + + `new ApolloServer({engine: {sendVariableValues: {all: true}}})`. + + ### v2.6.7 - `apollo-server-core`: Guard against undefined property access in `isDirectiveDefined` which resulted in "Cannot read property 'some' of undefined" error. [PR #2924](https://github.com/apollographql/apollo-server/pull/2924) [Issue #2921](https://github.com/apollographql/apollo-server/issues/2921) diff --git a/docs/source/api/apollo-server.md b/docs/source/api/apollo-server.md index cf8e816e638..c5fa0478d5d 100644 --- a/docs/source/api/apollo-server.md +++ b/docs/source/api/apollo-server.md @@ -346,19 +346,19 @@ addMockFunctionsToSchema({ * `sendVariableValues`: { transform: (options: { variables: Record, operationString?: string } ) => Record } | { exceptNames: Array<String> } | { onlyNames: Array<String> } - | { sendNone: true } - | { sendAll: true } + | { none: true } + | { all: true } By default, Apollo Server does not send the values of any GraphQL variables to Apollo's servers, because variable values often contain the private data of your app's users. If you'd like variable values to be included in traces, set this option. This option can take several forms: - - { sendNone: true }: don't send any variable values (DEFAULT) - - { sendAll: true }: send all variable values + - { 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. - { 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 Defaults to not sending any variable values if both this parameter and the deprecated `privateVariables` are not set. - The report will indicate each private variable key whose value was redacted by { sendNone: true } or { exceptNames: [...] }. + The report will indicate each private variable key whose value was redacted by { none: true } or { exceptNames: [...] }. * `privateVariables`: Array<String> | boolean @@ -366,17 +366,19 @@ addMockFunctionsToSchema({ DEPRECATING IN VERSION XX.XX.XX, to be replaced by the option `sendVariableValues`, which supports the same functionalities but allows for more flexibility. Passing an array into `privateVariables` is equivalent to passing in `{ exceptNames: array } ` to `sendVariableValues`, and passing in `true` or `false` is equivalent - to passing ` { sendNone: true } ` or ` { sendAll: true }`, respectively. + to passing ` { none: true } ` or ` { all: true }`, respectively. NOTE: An error will be thrown if both this deprecated option and its replacement, `sendVariableValues` are defined. + In order to preserve the old default of `privateVariables`, which sends all variables and their values, pass in the `sendVariableValues` option: + `new ApolloServer({engine: {sendVariableValues: {all: true}}})`. -* `sendHeaders`: { exceptNames: Array<String> } | { onlyNames: Array<String> } | { sendAll: boolean } | { sendNone: boolean } +* `sendHeaders`: { exceptNames: Array<String> } | { onlyNames: Array<String> } | { all: boolean } | { none: boolean } By default, Apollo Server does not send the list of HTTP request headers and values to Apollo's servers, to protect private data of your app's users. If you'd like this information included in traces, set this option. This option can take several forms: - - { sendNone: true }: drop all HTTP request headers (DEFAULT) - - { sendAll: true }: send the values of all HTTP request headers + - { none: true }: drop all HTTP request headers (DEFAULT) + - { all: true }: send the values of all HTTP request headers - { exceptNames: ... }: A case-insensitive list of names of HTTP headers whose values should not be sent to Apollo servers - { onlyNames: ... }: A case-insensitive list of names of HTTP headers whose values will be sent to Apollo servers @@ -390,10 +392,12 @@ addMockFunctionsToSchema({ DEPRECATING IN VERSION XX.XX.XX, use `sendHeaders` instead. Passing an array into `privateHeaders` is equivalent to passing ` { exceptNames: array } ` into `sendHeaders`, and - passing `true` or `false` is equivalent to passing in ` { sendNone: true } ` and ` { sendAll: true }`, respectively. + passing `true` or `false` is equivalent to passing in ` { none: true } ` and ` { all: true }`, respectively. NOTE: An error will be thrown if both this deprecated option and its replacement, `sendHeaders` are defined. - + In order to preserve the old default of `privateHeaders`, which sends all request headers and their values, pass in the `sendHeaders` option: + `new ApolloServer({engine: {sendHeaders: {all: true}}})`. + * `handleSignals`: boolean By default, EngineReportingAgent listens for the 'SIGINT' and 'SIGTERM' diff --git a/packages/apollo-engine-reporting/src/__tests__/agent.test.ts b/packages/apollo-engine-reporting/src/__tests__/agent.test.ts index 70939603ec5..d7903c298d2 100644 --- a/packages/apollo-engine-reporting/src/__tests__/agent.test.ts +++ b/packages/apollo-engine-reporting/src/__tests__/agent.test.ts @@ -17,28 +17,28 @@ describe('signature cache key', () => { }); describe("test handleLegacyOptions(), which converts the deprecated privateVariable and privateHeaders options to the new options' formats", () => { - it('Case 1: privateVariables/privateHeaders == False; same as sendAll', () => { + it('Case 1: privateVariables/privateHeaders == False; same as all', () => { const optionsPrivateFalse: EngineReportingOptions = { privateVariables: false, privateHeaders: false, }; handleLegacyOptions(optionsPrivateFalse); expect(optionsPrivateFalse.privateVariables).toBe(undefined); - expect(optionsPrivateFalse.sendVariableValues).toEqual({ sendAll: true }); + expect(optionsPrivateFalse.sendVariableValues).toEqual({ all: true }); expect(optionsPrivateFalse.privateHeaders).toBe(undefined); - expect(optionsPrivateFalse.sendHeaders).toEqual({ sendAll: true }); + expect(optionsPrivateFalse.sendHeaders).toEqual({ all: true }); }); - it('Case 2: privateVariables/privateHeaders == True; same as sendNone', () => { + it('Case 2: privateVariables/privateHeaders == True; same as none', () => { const optionsPrivateTrue: EngineReportingOptions = { privateVariables: true, privateHeaders: true, }; handleLegacyOptions(optionsPrivateTrue); expect(optionsPrivateTrue.privateVariables).toBe(undefined); - expect(optionsPrivateTrue.sendVariableValues).toEqual({ sendNone: true }); + expect(optionsPrivateTrue.sendVariableValues).toEqual({ none: true }); expect(optionsPrivateTrue.privateHeaders).toBe(undefined); - expect(optionsPrivateTrue.sendHeaders).toEqual({ sendNone: true }); + expect(optionsPrivateTrue.sendHeaders).toEqual({ none: true }); }); it('Case 3: privateVariables/privateHeaders set to an array', () => { @@ -61,14 +61,14 @@ describe("test handleLegacyOptions(), which converts the deprecated privateVaria it('Case 4: throws error when both the new and old options are set', () => { const optionsBothVariables: EngineReportingOptions = { privateVariables: true, - sendVariableValues: { sendNone: true }, + sendVariableValues: { none: true }, }; expect(() => { handleLegacyOptions(optionsBothVariables); }).toThrow(); const optionsBothHeaders: EngineReportingOptions = { privateHeaders: true, - sendHeaders: { sendNone: true }, + sendHeaders: { none: true }, }; expect(() => { handleLegacyOptions(optionsBothHeaders); diff --git a/packages/apollo-engine-reporting/src/__tests__/extension.test.ts b/packages/apollo-engine-reporting/src/__tests__/extension.test.ts index a2768e69c8d..fb13f87fc36 100644 --- a/packages/apollo-engine-reporting/src/__tests__/extension.test.ts +++ b/packages/apollo-engine-reporting/src/__tests__/extension.test.ts @@ -117,11 +117,11 @@ describe('check variableJson output for sendVariableValues null or undefined (de }); }); -describe('check variableJson output for sendVariableValues sendAll/sendNone type', () => { +describe('check variableJson output for sendVariableValues all/none type', () => { it('Case 1: No keys/values in variables to be filtered/not filtered', () => { const emptyOutput = new Trace.Details(); - expect(makeTraceDetails({}, { sendAll: true })).toEqual(emptyOutput); - expect(makeTraceDetails({}, { sendNone: true })).toEqual(emptyOutput); + expect(makeTraceDetails({}, { all: true })).toEqual(emptyOutput); + expect(makeTraceDetails({}, { none: true })).toEqual(emptyOutput); }); const filteredOutput = new Trace.Details(); @@ -135,25 +135,21 @@ describe('check variableJson output for sendVariableValues sendAll/sendNone type }); it('Case 2: Filter all variables', () => { - expect(makeTraceDetails(variables, { sendNone: true })).toEqual( - filteredOutput, - ); + expect(makeTraceDetails(variables, { none: true })).toEqual(filteredOutput); }); it('Case 3: Do not filter variables', () => { - expect(makeTraceDetails(variables, { sendAll: true })).toEqual( + expect(makeTraceDetails(variables, { all: true })).toEqual( nonFilteredOutput, ); }); it('Case 4: Check behavior for invalid inputs', () => { - expect(makeTraceDetails(variables, { sendNone: false })).toEqual( + expect(makeTraceDetails(variables, { none: false })).toEqual( nonFilteredOutput, ); - expect(makeTraceDetails(variables, { sendAll: false })).toEqual( - filteredOutput, - ); + expect(makeTraceDetails(variables, { all: false })).toEqual(filteredOutput); }); }); @@ -170,11 +166,9 @@ describe('variableJson output for sendVariableValues exceptNames: Array type', ( ).toEqual(expectedVariablesJson); }); - it('sendNone=true equivalent to exceptNames=[all variables]', () => { + it('none=true equivalent to exceptNames=[all variables]', () => { let privateVariablesArray: string[] = ['testing', 't2']; - expect( - makeTraceDetails(variables, { sendNone: true }).variablesJson, - ).toEqual( + expect(makeTraceDetails(variables, { none: true }).variablesJson).toEqual( makeTraceDetails(variables, { exceptNames: privateVariablesArray }) .variablesJson, ); @@ -194,21 +188,17 @@ describe('variableJson output for sendVariableValues onlyNames: Array type', () ).toEqual(expectedVariablesJson); }); - it('sendAll=true equivalent to onlyNames=[all variables]', () => { + it('all=true equivalent to onlyNames=[all variables]', () => { let privateVariablesArray: string[] = ['testing', 't2']; - expect( - makeTraceDetails(variables, { sendAll: true }).variablesJson, - ).toEqual( + expect(makeTraceDetails(variables, { all: true }).variablesJson).toEqual( makeTraceDetails(variables, { onlyNames: privateVariablesArray }) .variablesJson, ); }); - it('sendNone=true equivalent to onlyNames=[]', () => { + it('none=true equivalent to onlyNames=[]', () => { let privateVariablesArray: string[] = []; - expect( - makeTraceDetails(variables, { sendNone: true }).variablesJson, - ).toEqual( + expect(makeTraceDetails(variables, { none: true }).variablesJson).toEqual( makeTraceDetails(variables, { onlyNames: privateVariablesArray }) .variablesJson, ); @@ -309,23 +299,23 @@ describe('tests for the sendHeaders reporting option', () => { expect(http.requestHeaders).toEqual({}); }); - it('sendHeaders.sendAll and sendHeaders.sendNone', () => { + it('sendHeaders.all and sendHeaders.none', () => { const httpSafelist = makeTestHTTP(); - makeHTTPRequestHeaders(httpSafelist, headers, { sendAll: true }); + makeHTTPRequestHeaders(httpSafelist, headers, { all: true }); expect(httpSafelist.requestHeaders).toEqual(headersOutput); const httpBlocklist = makeTestHTTP(); - makeHTTPRequestHeaders(httpBlocklist, headers, { sendNone: true }); + makeHTTPRequestHeaders(httpBlocklist, headers, { none: true }); expect(httpBlocklist.requestHeaders).toEqual({}); }); - it('invalid inputs for sendHeaders.sendAll and sendHeaders.sendNone', () => { + it('invalid inputs for sendHeaders.all and sendHeaders.none', () => { const httpSafelist = makeTestHTTP(); - makeHTTPRequestHeaders(httpSafelist, headers, { sendNone: false }); + makeHTTPRequestHeaders(httpSafelist, headers, { none: false }); expect(httpSafelist.requestHeaders).toEqual(headersOutput); const httpBlocklist = makeTestHTTP(); - makeHTTPRequestHeaders(httpBlocklist, headers, { sendAll: false }); + makeHTTPRequestHeaders(httpBlocklist, headers, { all: false }); expect(httpBlocklist.requestHeaders).toEqual({}); }); @@ -348,7 +338,7 @@ describe('tests for the sendHeaders reporting option', () => { headers.append('cookie', 'blahblah'); headers.append('set-cookie', 'blahblah'); const http = makeTestHTTP(); - makeHTTPRequestHeaders(http, headers, { sendAll: true }); + makeHTTPRequestHeaders(http, headers, { all: true }); expect(http.requestHeaders['authorization']).toBe(undefined); expect(http.requestHeaders['cookie']).toBe(undefined); expect(http.requestHeaders['set-cookie']).toBe(undefined); diff --git a/packages/apollo-engine-reporting/src/agent.ts b/packages/apollo-engine-reporting/src/agent.ts index fa3a95d065d..d5f699c675d 100644 --- a/packages/apollo-engine-reporting/src/agent.ts +++ b/packages/apollo-engine-reporting/src/agent.ts @@ -28,8 +28,8 @@ export interface ClientInfo { export type SendValuesBaseOptions = | { onlyNames: Array } | { exceptNames: Array } - | { sendAll: true } - | { sendNone: true }; + | { all: true } + | { none: true }; type VariableValueTransformOptions = { variables: Record; @@ -104,8 +104,8 @@ export interface EngineReportingOptions { * By default, Apollo Server does not send the values of any GraphQL variables to Apollo's servers, because variable * values often contain the private data of your app's users. If you'd like variable values to be included in traces, set this option. * This option can take several forms: - * - { sendNone: true }: don't send any variable values (DEFAULT) - * - { sendAll: true}: send all variable values + * - { 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. * - { exceptNames: ... }: a case-sensitive list of names of variables whose values should not be sent to Apollo servers @@ -113,7 +113,7 @@ export interface EngineReportingOptions { * * Defaults to not sending any variable values if both this parameter and * the deprecated `privateVariables` are not set. The report will - * indicate each private variable key whose value was redacted by { sendNone: true } or { exceptNames: [...] }. + * indicate each private variable key whose value was redacted by { none: true } or { exceptNames: [...] }. * * TODO(helen): Add new flag to the trace details (and modify the protobuf message structure) to indicate the type of modification. Then, add the following description to the docs: * "The report will indicate that variable values were modified by a custom function, or will list all private variables redacted." @@ -123,8 +123,8 @@ export interface EngineReportingOptions { /** * [DEPRECATED] Use sendVariableValues * Passing an array into privateVariables is equivalent to passing { exceptNames: array } into - * sendVariableValues, and passing in `true` or `false` is equivalent to passing { sendNone: true } or - * { sendAll: true }, respectively. + * sendVariableValues, and passing in `true` or `false` is equivalent to passing { none: true } or + * { all: true }, respectively. * * An error will be thrown if both this deprecated option and its replacement, sendVariableValues are defined. */ @@ -134,8 +134,8 @@ export interface EngineReportingOptions { * Apollo's servers, to protect private data of your app's users. If you'd like this information included in traces, * set this option. This option can take several forms: * - * - { sendNone: true } to drop all HTTP request headers (DEFAULT) - * - { sendAll: true } to send the values of all HTTP request headers + * - { none: true } to drop all HTTP request headers (DEFAULT) + * - { all: true } to send the values of all HTTP request headers * - { exceptNames: Array } A case-insensitive list of names of HTTP headers whose values should not be * sent to Apollo servers * - { onlyNames: Array }: A case-insensitive list of names of HTTP headers whose values will be sent to Apollo servers @@ -149,7 +149,7 @@ export interface EngineReportingOptions { /** * [DEPRECATED] Use sendHeaders * Passing an array into privateHeaders is equivalent to passing { exceptNames: array } into sendHeaders, and - * passing `true` or `false` is equivalent to passing in { sendNone: true } and { sendAll: true }, respectively. + * passing `true` or `false` is equivalent to passing in { none: true } and { all: true }, respectively. * * An error will be thrown if both this deprecated option and its replacement, sendHeaders are defined. */ @@ -585,6 +585,6 @@ function makeSendValuesBaseOptionsFromLegacy( exceptNames: legacyPrivateOption, } : legacyPrivateOption - ? { sendNone: true } - : { sendAll: true }; + ? { none: true } + : { all: true }; } diff --git a/packages/apollo-engine-reporting/src/extension.ts b/packages/apollo-engine-reporting/src/extension.ts index a9345796e88..4bd3827881e 100644 --- a/packages/apollo-engine-reporting/src/extension.ts +++ b/packages/apollo-engine-reporting/src/extension.ts @@ -413,11 +413,11 @@ export function makeTraceDetails( Object.keys(variablesToRecord).forEach(name => { if ( !sendVariableValues || - ('sendNone' in sendVariableValues && sendVariableValues.sendNone) || - ('sendAll' in sendVariableValues && !sendVariableValues.sendAll) || + ('none' in sendVariableValues && sendVariableValues.none) || + ('all' in sendVariableValues && !sendVariableValues.all) || ('exceptNames' in sendVariableValues && // We assume that most users will have only a few variables values to hide, - // or will just set {sendNone: true}; we can change this + // or will just set {none: true}; we can change this // linear-time operation if it causes real performance issues. sendVariableValues.exceptNames.includes(name)) || ('onlyNames' in sendVariableValues && @@ -466,8 +466,8 @@ export function makeHTTPRequestHeaders( ): void { if ( !sendHeaders || - ('sendNone' in sendHeaders && sendHeaders.sendNone) || - ('sendAll' in sendHeaders && !sendHeaders.sendAll) + ('none' in sendHeaders && sendHeaders.none) || + ('all' in sendHeaders && !sendHeaders.all) ) { return; } @@ -476,7 +476,7 @@ export function makeHTTPRequestHeaders( if ( ('exceptNames' in sendHeaders && // We assume that most users only have a few headers to hide, or will - // just set {sendNone: true} ; we can change this linear-time + // just set {none: true} ; we can change this linear-time // operation if it causes real performance issues. sendHeaders.exceptNames.some(exceptHeader => { // Headers are case-insensitive, and should be compared as such. From 6fcd4d2f0637d072fe29b3023b36859d8c6b66ff Mon Sep 17 00:00:00 2001 From: Helen Date: Thu, 27 Jun 2019 09:22:04 -0700 Subject: [PATCH 08/12] Apply suggestions from code review: doc formatting, fixes (equality comparisons, constant declarations), more test cases Co-Authored-By: Jesse Rosenberger --- docs/source/api/apollo-server.md | 33 ++++++++---------- .../src/__tests__/agent.test.ts | 20 ++++++++--- .../src/__tests__/extension.test.ts | 11 +++--- packages/apollo-engine-reporting/src/agent.ts | 27 +++++++++------ .../apollo-engine-reporting/src/extension.ts | 34 +++++++++++-------- 5 files changed, 73 insertions(+), 52 deletions(-) diff --git a/docs/source/api/apollo-server.md b/docs/source/api/apollo-server.md index c5fa0478d5d..a831458e7b6 100644 --- a/docs/source/api/apollo-server.md +++ b/docs/source/api/apollo-server.md @@ -351,24 +351,23 @@ addMockFunctionsToSchema({ By default, Apollo Server does not send the values of any GraphQL variables to Apollo's servers, because variable values often contain the private data of your app's users. If you'd like variable values to be included in traces, set this option. This option can take several forms: - - { 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. - - { 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 + - `{ 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. + - `{ 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. Defaults to not sending any variable values if both this parameter and the deprecated `privateVariables` are not set. - The report will indicate each private variable key whose value was redacted by { none: true } or { exceptNames: [...] }. + The report will indicate each private variable key whose value was redacted by `{ none: true }` or `{ exceptNames: [...]` }. * `privateVariables`: Array<String> | boolean - - DEPRECATING IN VERSION XX.XX.XX, to be replaced by the option `sendVariableValues`, which supports the same - functionalities but allows for more flexibility. Passing an array into `privateVariables` is equivalent to + > Will be deprecated in 3.0. Use the option `sendVariableValues` instead. + Passing an array into `privateVariables` is equivalent to passing in `{ exceptNames: array } ` to `sendVariableValues`, and passing in `true` or `false` is equivalent to passing ` { none: true } ` or ` { all: true }`, respectively. - NOTE: An error will be thrown if both this deprecated option and its replacement, `sendVariableValues` are defined. + > Note: An error will be thrown if both this deprecated option and its replacement, `sendVariableValues` are defined. In order to preserve the old default of `privateVariables`, which sends all variables and their values, pass in the `sendVariableValues` option: `new ApolloServer({engine: {sendVariableValues: {all: true}}})`. @@ -377,11 +376,10 @@ addMockFunctionsToSchema({ Apollo's servers, to protect private data of your app's users. If you'd like this information included in traces, set this option. This option can take several forms: - - { none: true }: drop all HTTP request headers (DEFAULT) - - { all: true }: send the values of all HTTP request headers - - { exceptNames: ... }: A case-insensitive list of names of HTTP headers whose values should not be - sent to Apollo servers - - { onlyNames: ... }: A case-insensitive list of names of HTTP headers whose values will be sent to Apollo servers + - `{ none: true }`: Drop all HTTP request headers. **(DEFAULT)** + - `{ all: true }`: Send the values of all HTTP request headers. + - `{ exceptNames: [...] }`: A case-insensitive list of names of HTTP headers whose values should not be sent to Apollo servers. + - `{ onlyNames: [...] }`: A case-insensitive list of names of HTTP headers whose values will be sent to Apollo servers. Defaults to not sending any request header names and values if both this parameter and the deprecated `privateHeaders` are not set. Unlike with `sendVariableValues`, names of dropped headers are not reported. @@ -389,12 +387,11 @@ addMockFunctionsToSchema({ * `privateHeaders`: Array<String> | boolean - - DEPRECATING IN VERSION XX.XX.XX, use `sendHeaders` instead. + > Will be deprecated in 3.0. Use the `sendHeaders` option instead. Passing an array into `privateHeaders` is equivalent to passing ` { exceptNames: array } ` into `sendHeaders`, and passing `true` or `false` is equivalent to passing in ` { none: true } ` and ` { all: true }`, respectively. - NOTE: An error will be thrown if both this deprecated option and its replacement, `sendHeaders` are defined. + > Note: An error will be thrown if both this deprecated option and its replacement, `sendHeaders`, are defined. In order to preserve the old default of `privateHeaders`, which sends all request headers and their values, pass in the `sendHeaders` option: `new ApolloServer({engine: {sendHeaders: {all: true}}})`. diff --git a/packages/apollo-engine-reporting/src/__tests__/agent.test.ts b/packages/apollo-engine-reporting/src/__tests__/agent.test.ts index d7903c298d2..ff6ac43ab6a 100644 --- a/packages/apollo-engine-reporting/src/__tests__/agent.test.ts +++ b/packages/apollo-engine-reporting/src/__tests__/agent.test.ts @@ -58,7 +58,19 @@ describe("test handleLegacyOptions(), which converts the deprecated privateVaria }); }); - it('Case 4: throws error when both the new and old options are set', () => { + it('Case 4: privateVariables/privateHeaders are null or undefined; no change', () => { + const optionsPrivateFalse: EngineReportingOptions = { + privateVariables: undefined, + privateHeaders: null, // null is not a valid TS input, but check the output anyways + }; + handleLegacyOptions(optionsPrivateFalse); + expect(optionsPrivateFalse.privateVariables).toBe(undefined); + expect(optionsPrivateFalse.sendVariableValues).toBe(undefined); + expect(optionsPrivateFalse.privateHeaders).toBe(undefined); + expect(optionsPrivateFalse.sendHeaders).toBe(undefined); + }); + + it('Case 5: throws error when both the new and old options are set', () => { const optionsBothVariables: EngineReportingOptions = { privateVariables: true, sendVariableValues: { none: true }, @@ -75,14 +87,14 @@ describe("test handleLegacyOptions(), which converts the deprecated privateVaria }).toThrow(); }); - it('Case 5: the passed in options are not modified if deprecated fields were not set', () => { + it('Case 6: the passed in options are not modified if deprecated fields were not set', () => { const optionsNotDeprecated: EngineReportingOptions = { sendVariableValues: { exceptNames: ['test'] }, - sendHeaders: true, + sendHeaders: { all: true }, }; const output: EngineReportingOptions = { sendVariableValues: { exceptNames: ['test'] }, - sendHeaders: true, + sendHeaders: { all: true }, }; handleLegacyOptions(optionsNotDeprecated); expect(optionsNotDeprecated).toEqual(output); diff --git a/packages/apollo-engine-reporting/src/__tests__/extension.test.ts b/packages/apollo-engine-reporting/src/__tests__/extension.test.ts index fb13f87fc36..7433407b2e9 100644 --- a/packages/apollo-engine-reporting/src/__tests__/extension.test.ts +++ b/packages/apollo-engine-reporting/src/__tests__/extension.test.ts @@ -167,7 +167,7 @@ describe('variableJson output for sendVariableValues exceptNames: Array type', ( }); it('none=true equivalent to exceptNames=[all variables]', () => { - let privateVariablesArray: string[] = ['testing', 't2']; + const privateVariablesArray: string[] = ['testing', 't2']; expect(makeTraceDetails(variables, { none: true }).variablesJson).toEqual( makeTraceDetails(variables, { exceptNames: privateVariablesArray }) .variablesJson, @@ -189,7 +189,7 @@ describe('variableJson output for sendVariableValues onlyNames: Array type', () }); it('all=true equivalent to onlyNames=[all variables]', () => { - let privateVariablesArray: string[] = ['testing', 't2']; + const privateVariablesArray: string[] = ['testing', 't2']; expect(makeTraceDetails(variables, { all: true }).variablesJson).toEqual( makeTraceDetails(variables, { onlyNames: privateVariablesArray }) .variablesJson, @@ -197,7 +197,7 @@ describe('variableJson output for sendVariableValues onlyNames: Array type', () }); it('none=true equivalent to onlyNames=[]', () => { - let privateVariablesArray: string[] = []; + const privateVariablesArray: string[] = []; expect(makeTraceDetails(variables, { none: true }).variablesJson).toEqual( makeTraceDetails(variables, { onlyNames: privateVariablesArray }) .variablesJson, @@ -211,7 +211,7 @@ describe('variableJson output for sendVariableValues transform: custom function const customModifier = (input: { variables: Record; }): Record => { - let out: Record = {}; + const out: Record = Object.create(null); Object.keys(input.variables).map((name: string) => { out[name] = modifiedValue; }); @@ -236,7 +236,7 @@ describe('variableJson output for sendVariableValues transform: custom function const modifier = (input: { variables: Record; }): Record => { - let out: Record = {}; + const out: Record = Object.create(null); Object.keys(input.variables).map((name: string) => { out[name] = null; }); @@ -291,6 +291,7 @@ const headersOutput = { name: new Trace.HTTP.Values({ value: ['value'] }) }; describe('tests for the sendHeaders reporting option', () => { it('sendHeaders defaults to hiding all', () => { const http = makeTestHTTP(); + // sendHeaders: null is not a valid TS input, but check the output anyways makeHTTPRequestHeaders(http, headers, null); expect(http.requestHeaders).toEqual({}); makeHTTPRequestHeaders(http, headers, undefined); diff --git a/packages/apollo-engine-reporting/src/agent.ts b/packages/apollo-engine-reporting/src/agent.ts index d5f699c675d..b05ed2e8861 100644 --- a/packages/apollo-engine-reporting/src/agent.ts +++ b/packages/apollo-engine-reporting/src/agent.ts @@ -551,26 +551,33 @@ export function handleLegacyOptions( options: EngineReportingOptions, ): void { // Handle the legacy option: privateVariables - if (options.privateVariables != null && options.sendVariableValues) { + if ( + typeof options.privateVariables !== 'undefined' && + options.sendVariableValues + ) { throw new Error( "You have set both the 'sendVariableValues' and the deprecated 'privateVariables' options. Please only set 'sendVariableValues'.", ); - } else if (options.privateVariables != null) { - options.sendVariableValues = makeSendValuesBaseOptionsFromLegacy( - options.privateVariables, - ); + } else if (typeof options.privateVariables !== 'undefined') { + if (options.privateVariables !== null) { + options.sendVariableValues = makeSendValuesBaseOptionsFromLegacy( + options.privateVariables, + ); + } delete options.privateVariables; } // Handle the legacy option: privateHeaders - if (options.privateHeaders != null && options.sendHeaders) { + if (typeof options.privateHeaders !== 'undefined' && options.sendHeaders) { throw new Error( "You have set both the 'sendHeaders' and the deprecated 'privateHeaders' options. Please only set 'sendHeaders'.", ); - } else if (options.privateHeaders != null) { - options.sendHeaders = makeSendValuesBaseOptionsFromLegacy( - options.privateHeaders, - ); + } else if (typeof options.privateHeaders !== 'undefined') { + if (options.privateHeaders !== null) { + options.sendHeaders = makeSendValuesBaseOptionsFromLegacy( + options.privateHeaders, + ); + } delete options.privateHeaders; } } diff --git a/packages/apollo-engine-reporting/src/extension.ts b/packages/apollo-engine-reporting/src/extension.ts index 4bd3827881e..2831a0a1aaa 100644 --- a/packages/apollo-engine-reporting/src/extension.ts +++ b/packages/apollo-engine-reporting/src/extension.ts @@ -380,11 +380,11 @@ function defaultGenerateClientInfo({ request }: GraphQLRequestContext) { // Creates trace details from request variables, given a specification for modifying // values of private or sensitive variables. -// The details include the variables in the request, TODO(helen): and the modifier type. -// If sendVariableValues is {safelistAll: bool} or {exceptVariableNames: Array}, it will act similarly to +// The details will include all variable names and their (possibly hidden or modified) values. +// If sendVariableValues is {all: bool}, {none: bool} or {exceptNames: Array}, the option will act similarly to // to the to-be-deprecated options.privateVariables, except that the redacted variable -// names will still be visible in the UI when 'true.' -// If sendVariableValues is null, we default to the safeListAll = false case. +// names will still be visible in the UI even if the values are hidden. +// If sendVariableValues is null or undefined, we default to the {none: true} case. export function makeTraceDetails( variables: Record, sendVariableValues?: VariableValueOptions, @@ -430,16 +430,20 @@ export function makeTraceDetails( } else { try { details.variablesJson![name] = - variablesToRecord[name] === undefined + typeof variablesToRecord[name] === 'undefined' ? '' : JSON.stringify(variablesToRecord[name]); } catch (e) { - // This probably means that the value contains a circular reference, - // causing `JSON.stringify()` to throw a TypeError: - // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/stringify#Issue_with_JSON.stringify()_when_serializing_circular_references - details.variablesJson![name] = JSON.stringify( - '[Unable to convert value to JSON]', - ); + if ( + e.name === 'TypeError' && + e.message === 'Converting circular structure to JSON' + ) { + details.variablesJson![name] = JSON.stringify(e.message); + } else { + // Re-throw when it doesn't meet our expectation so we don't + // inadvertently swallow anything as a "cycle error" when its not. + throw e; + } } } }); @@ -452,7 +456,7 @@ function cleanModifiedVariables( originalKeys: Array, modifiedVariables: Record, ): Record { - let cleanedVariables: Record = {}; + const cleanedVariables: Record = Object.create(null); originalKeys.forEach(name => { cleanedVariables[name] = modifiedVariables[name]; }); @@ -472,7 +476,7 @@ export function makeHTTPRequestHeaders( return; } for (const [key, value] of headers) { - const lowercaseKey = key.toLowerCase(); + const lowerCaseKey = key.toLowerCase(); if ( ('exceptNames' in sendHeaders && // We assume that most users only have a few headers to hide, or will @@ -480,11 +484,11 @@ export function makeHTTPRequestHeaders( // operation if it causes real performance issues. sendHeaders.exceptNames.some(exceptHeader => { // Headers are case-insensitive, and should be compared as such. - return exceptHeader.toLowerCase() === lowercaseKey; + return exceptHeader.toLowerCase() === lowerCaseKey; })) || ('onlyNames' in sendHeaders && !sendHeaders.onlyNames.some(header => { - return header.toLowerCase() === lowercaseKey; + return header.toLowerCase() === lowerCaseKey; })) ) { continue; From c3e4f9feffbe06f98092213255d4893c6b159367 Mon Sep 17 00:00:00 2001 From: Helen Ho Date: Thu, 27 Jun 2019 10:49:14 -0700 Subject: [PATCH 09/12] catching circular reference errors, adding test --- .../src/__tests__/extension.test.ts | 13 +++++++++++++ packages/apollo-engine-reporting/src/extension.ts | 10 ++++++++-- 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/packages/apollo-engine-reporting/src/__tests__/extension.test.ts b/packages/apollo-engine-reporting/src/__tests__/extension.test.ts index 7433407b2e9..7d6dc27daca 100644 --- a/packages/apollo-engine-reporting/src/__tests__/extension.test.ts +++ b/packages/apollo-engine-reporting/src/__tests__/extension.test.ts @@ -271,6 +271,19 @@ describe('variableJson output for sendVariableValues transform: custom function }); }); +describe('Catch circular reference error during JSON.stringify', () => { + const circularReference = {}; + circularReference['this'] = circularReference; + + const circularVariables = { + bad: circularReference, + }; + + expect( + makeTraceDetails(circularVariables, { all: true }).variablesJson['bad'], + ).toEqual(JSON.stringify('[Unable to convert value to JSON]')); +}); + function makeTestHTTP(): Trace.HTTP { return new Trace.HTTP({ method: Trace.HTTP.Method.UNKNOWN, diff --git a/packages/apollo-engine-reporting/src/extension.ts b/packages/apollo-engine-reporting/src/extension.ts index 2831a0a1aaa..381b4998e22 100644 --- a/packages/apollo-engine-reporting/src/extension.ts +++ b/packages/apollo-engine-reporting/src/extension.ts @@ -436,9 +436,15 @@ export function makeTraceDetails( } catch (e) { if ( e.name === 'TypeError' && - e.message === 'Converting circular structure to JSON' + (e.message === 'cyclic object value' || + e.message === 'Converting circular structure to JSON' || + e.message === 'Circular reference in value argument not supported') + // Not sure how to determine the standardized error message... so checking the ones listed here: + // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/Cyclic_object_value#Message ) { - details.variablesJson![name] = JSON.stringify(e.message); + details.variablesJson![name] = JSON.stringify( + '[Unable to convert value to JSON]', + ); } else { // Re-throw when it doesn't meet our expectation so we don't // inadvertently swallow anything as a "cycle error" when its not. From 8f9ca40090211bd6fd9a89a0ef3a666faebc140e Mon Sep 17 00:00:00 2001 From: Jesse Rosenberger Date: Thu, 27 Jun 2019 22:48:32 +0300 Subject: [PATCH 10/12] Update CHANGELOG.md --- CHANGELOG.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 03c6b26d454..637f1b2e880 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,11 +8,11 @@ The version headers in this history reflect the versions of Apollo Server itself - `apollo-engine-reporting`: **BEHAVIOR CHANGE**: If the error returned from the `engine.rewriteError` hook has an `extensions` property, that property will be used instead of the original error's extensions. Document that changes to most other `GraphQLError` fields by `engine.rewriteError` are ignored. [PR #2932](https://github.com/apollographql/apollo-server/pull/2932) - `apollo-engine-reporting`: **BEHAVIOR CHANGE**: The `engine.maskErrorDetails` option, deprecated by `engine.rewriteError` in v2.5.0, now behaves a bit more like the new option: while all error messages will be redacted, they will still show up on the appropriate nodes in a trace. [PR #2932](https://github.com/apollographql/apollo-server/pull/2932) -- `apollo-engine-reporting`: **BEHAVIOR CHANGE**: By default, send no GraphQL variable values to Apollo's servers instead of sending all variable values. Adding the new EngineReportingOption `maskVariableValues` to send some or all variable values, possibly after transforming them. This replaces the `privateVariables` option, which is now deprecated. [PR #2931](https://github.com/apollographql/apollo-server/pull/2931) +- `apollo-engine-reporting`: **BEHAVIOR CHANGE**: By default, send no GraphQL variable values to Apollo's servers instead of sending all variable values. Adding the new EngineReportingOption `sendVariableValues` to send some or all variable values, possibly after transforming them. This replaces the `privateVariables` option, which is now deprecated. [PR #2931](https://github.com/apollographql/apollo-server/pull/2931) > Note: In order to keep shipping all GraphQL variable values to Apollo Engine, pass in the option: - - `new ApolloServer({engine: {sendVariableValues: {all: true}}})`. + > + > `new ApolloServer({engine: {sendVariableValues: {all: true}}})`. ### v2.6.7 From 4b2f9d38238a92e09aef0b974411dde9f6239998 Mon Sep 17 00:00:00 2001 From: Jesse Rosenberger Date: Thu, 27 Jun 2019 23:47:21 +0300 Subject: [PATCH 11/12] Use only the Node.js JSON cycles error. Node.js only raises this particular error when cycles are detected. While I first thought it was more defensive to catch the exact error we anticipated, I'm slightly reconsidering whether this is defensive enough and if we should, in fact, change this back to catching any error, particularly since this runs async and might go undetected or cause a whole string of a user's errors to not pass any variables. Thoughts, @glasser? --- packages/apollo-engine-reporting/src/extension.ts | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/packages/apollo-engine-reporting/src/extension.ts b/packages/apollo-engine-reporting/src/extension.ts index 381b4998e22..5535c268372 100644 --- a/packages/apollo-engine-reporting/src/extension.ts +++ b/packages/apollo-engine-reporting/src/extension.ts @@ -435,12 +435,14 @@ export function makeTraceDetails( : JSON.stringify(variablesToRecord[name]); } catch (e) { if ( + // The apollo-engine-reporting package currently only runs on Node.js + // and Node.js 6, 8, 10 and 12 all pass this criteria. For what it's + // worth, changes in error message text are considered major breaking + // changes to Node.js. In the future, hopefully all of these will + // be guarded by error `code` properties, but that will take some + // upstream V8 work to standardize. e.name === 'TypeError' && - (e.message === 'cyclic object value' || - e.message === 'Converting circular structure to JSON' || - e.message === 'Circular reference in value argument not supported') - // Not sure how to determine the standardized error message... so checking the ones listed here: - // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/Cyclic_object_value#Message + e.message.startsWith('Converting circular structure to JSON') ) { details.variablesJson![name] = JSON.stringify( '[Unable to convert value to JSON]', From 6c9aa8f76ebc96cd4b013885dbbf315e1422786f Mon Sep 17 00:00:00 2001 From: Jesse Rosenberger Date: Thu, 27 Jun 2019 23:53:46 +0300 Subject: [PATCH 12/12] Rescind on my suggestion to precisely match the error. I thought it made sense to catch only cycles, but I've changed my mind. I think there could be just arbitrary errors in a `toString` or `toJSON` implementation which we'd still want to guard, particularly because these errors are shipped async. Ref: 4b2f9d38238a92e09aef0b974411dde9f6239998 Ref: https://github.com/apollographql/apollo-server/pull/2931#discussion_r298191039 --- .../apollo-engine-reporting/src/extension.ts | 21 +++---------------- 1 file changed, 3 insertions(+), 18 deletions(-) diff --git a/packages/apollo-engine-reporting/src/extension.ts b/packages/apollo-engine-reporting/src/extension.ts index 5535c268372..1a65393f8e0 100644 --- a/packages/apollo-engine-reporting/src/extension.ts +++ b/packages/apollo-engine-reporting/src/extension.ts @@ -434,24 +434,9 @@ export function makeTraceDetails( ? '' : JSON.stringify(variablesToRecord[name]); } catch (e) { - if ( - // The apollo-engine-reporting package currently only runs on Node.js - // and Node.js 6, 8, 10 and 12 all pass this criteria. For what it's - // worth, changes in error message text are considered major breaking - // changes to Node.js. In the future, hopefully all of these will - // be guarded by error `code` properties, but that will take some - // upstream V8 work to standardize. - e.name === 'TypeError' && - e.message.startsWith('Converting circular structure to JSON') - ) { - details.variablesJson![name] = JSON.stringify( - '[Unable to convert value to JSON]', - ); - } else { - // Re-throw when it doesn't meet our expectation so we don't - // inadvertently swallow anything as a "cycle error" when its not. - throw e; - } + details.variablesJson![name] = JSON.stringify( + '[Unable to convert value to JSON]', + ); } } });