From 1ebc4727529cfd22ceecc6912b0c926e72c8909a Mon Sep 17 00:00:00 2001 From: Helen Ho Date: Fri, 14 Jun 2019 14:40:57 -0700 Subject: [PATCH 1/7] - 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 ce2e92b3cbb..76b03f37dd1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,7 @@ # Changelog ### vNext +- `apollo-engine-reporting`: BREAKING 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.2 diff --git a/docs/source/api/apollo-server.md b/docs/source/api/apollo-server.md index ac502c0a2e6..b8ac9113503 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 732271c0544..95458ae8542 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'; @@ -146,41 +149,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); @@ -455,3 +442,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 862017e03133bcff75ccaa5fbd90b31d25916084 Mon Sep 17 00:00:00 2001 From: Helen Ho Date: Wed, 19 Jun 2019 10:37:26 -0700 Subject: [PATCH 2/7] Changed new option to sendVariableValues instead of maskVariableValues, and the subsequent tests/docs --- CHANGELOG.md | 3 +- 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 +++++----- 5 files changed, 86 insertions(+), 76 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 76b03f37dd1..38acde271c9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,7 +1,8 @@ # Changelog ### vNext -- `apollo-engine-reporting`: BREAKING 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`: BREAKING CHANGE: By default, send no GraphQL variable values to Apollo's servers instead of sending all variable values. Use 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 #2847](https://github.com/apollographql/apollo-server/pull/2847) [PR #2472](https://github.com/apollographql/apollo-server/pull/2472) ### v2.6.2 diff --git a/docs/source/api/apollo-server.md b/docs/source/api/apollo-server.md index b8ac9113503..f0b36948e68 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 95458ae8542..ff95cf43318 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'; @@ -151,17 +150,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, @@ -452,22 +451,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, }); @@ -484,13 +476,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 @@ -518,17 +511,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 896c311c84fe456f208ccee1d8e63de8aa14850a Mon Sep 17 00:00:00 2001 From: Helen Ho Date: Wed, 19 Jun 2019 14:14:42 -0700 Subject: [PATCH 3/7] adding new reporting option sendHeaders, fixing some names --- CHANGELOG.md | 3 +- 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 ++++++++++++------ 5 files changed, 205 insertions(+), 55 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 38acde271c9..476b7266ee7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,7 +3,8 @@ ### vNext - `apollo-engine-reporting`: BREAKING CHANGE: By default, send no GraphQL variable values to Apollo's servers instead of sending all variable values. Use 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 #2847](https://github.com/apollographql/apollo-server/pull/2847) [PR #2472](https://github.com/apollographql/apollo-server/pull/2472) - +- `apollo-engine-reporting`: BREAKING CHANGE: By default, send no GraphQL headers to Apollo's servers instead of sending all. Use the new EngineReportingOption `sendHeaders` to send some or all headers and their values. +A replacement for the 'privateHeaders' option, which is now deprecated. [PR #2847](https://github.com/apollographql/apollo-server/pull/2847) ### v2.6.2 - `apollo-engine-reporting-protobuf`: Update protobuff to include `forbiddenOperations` and `registeredOperations` [PR #2768](https://github.com/apollographql/apollo-server/pull/2768) diff --git a/docs/source/api/apollo-server.md b/docs/source/api/apollo-server.md index f0b36948e68..bef59106abb 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 ff95cf43318..2140f215248 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'; @@ -106,32 +105,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) { @@ -153,7 +140,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, @@ -445,10 +432,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, @@ -477,8 +464,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 @@ -515,13 +502,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); } @@ -538,3 +525,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 8f9bbafecc0911ddf6114bc5da00a45d49c1a5fb Mon Sep 17 00:00:00 2001 From: Helen Ho Date: Wed, 19 Jun 2019 15:24:16 -0700 Subject: [PATCH 4/7] addressing comments, name/argument changes, updating docs, adding and fixing test cases --- CHANGELOG.md | 6 +- 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, 378 insertions(+), 315 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 476b7266ee7..e4b9300f1a7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,9 +2,9 @@ ### vNext - `apollo-engine-reporting`: BREAKING CHANGE: By default, send no GraphQL variable values to Apollo's servers instead of sending all variable values. Use 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 #2847](https://github.com/apollographql/apollo-server/pull/2847) [PR #2472](https://github.com/apollographql/apollo-server/pull/2472) -- `apollo-engine-reporting`: BREAKING CHANGE: By default, send no GraphQL headers to Apollo's servers instead of sending all. Use the new EngineReportingOption `sendHeaders` to send some or all headers and their values. -A replacement for the 'privateHeaders' option, which is now deprecated. [PR #2847](https://github.com/apollographql/apollo-server/pull/2847) +This replaces the `privateVariables` option, which is now deprecated. [PR #2847](https://github.com/apollographql/apollo-server/pull/2847) +- `apollo-engine-reporting`: BREAKING CHANGE: By default, send no GraphQL request headers to Apollo's servers instead of sending all. Use the new EngineReportingOption `sendHeaders` to send some or all headers and their values. +This replaces the `privateHeaders` option, which is now deprecated. [PR #2847](https://github.com/apollographql/apollo-server/pull/2847) ### v2.6.2 - `apollo-engine-reporting-protobuf`: Update protobuff to include `forbiddenOperations` and `registeredOperations` [PR #2768](https://github.com/apollographql/apollo-server/pull/2768) diff --git a/docs/source/api/apollo-server.md b/docs/source/api/apollo-server.md index bef59106abb..92c4554f07a 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 2140f215248..e2a2142f661 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'; @@ -106,20 +107,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; @@ -136,24 +129,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); @@ -443,10 +423,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, }); @@ -463,14 +443,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 @@ -479,7 +458,7 @@ export function makeTraceDetails( } else { try { details.variablesJson![name] = - variablesToRecord[name] == null + variablesToRecord[name] === undefined ? '' : JSON.stringify(variablesToRecord[name]); } catch (e) { @@ -492,27 +471,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( @@ -529,22 +490,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(); }) @@ -564,17 +522,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 09946ae83f8e386fe81e4fb0d4973dc422a4c42b Mon Sep 17 00:00:00 2001 From: Helen Ho Date: Mon, 24 Jun 2019 12:05:04 -0700 Subject: [PATCH 5/7] 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 92c4554f07a..b6819dd9267 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 e2a2142f661..0dcacac1d1b 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'; @@ -449,7 +449,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 @@ -490,22 +492,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 cf1c21df7213de9346e2d5397f8740cc316b9bb4 Mon Sep 17 00:00:00 2001 From: Helen Ho Date: Tue, 25 Jun 2019 10:19:14 -0700 Subject: [PATCH 6/7] 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 b6819dd9267..891b8b1eed0 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 0dcacac1d1b..497d7c103e8 100644 --- a/packages/apollo-engine-reporting/src/extension.ts +++ b/packages/apollo-engine-reporting/src/extension.ts @@ -444,14 +444,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 @@ -494,10 +495,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 @@ -505,11 +511,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 db9c49fcdf30daec944120863f8d139ddb82caa3 Mon Sep 17 00:00:00 2001 From: Helen Ho Date: Wed, 26 Jun 2019 10:33:55 -0700 Subject: [PATCH 7/7] changed suboption name, updating docs with more info on breaking change --- CHANGELOG.md | 11 +++- 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, 71 insertions(+), 68 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e4b9300f1a7..4528841bda3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,10 +1,19 @@ # Changelog ### vNext -- `apollo-engine-reporting`: BREAKING CHANGE: By default, send no GraphQL variable values to Apollo's servers instead of sending all variable values. Use the new EngineReportingOption `sendVariableValues` to send some or all variable values, possibly after transforming them. +- `apollo-engine-reporting`: BREAKING CHANGE: By default, send no GraphQL variable values to Apollo's servers instead of sending all variable values. +Use 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 #2847](https://github.com/apollographql/apollo-server/pull/2847) + + Note: This is only a breaking change if the option `privateVariables` was not previously specified. In order to preserve the old default, pass in the option: + `new ApolloServer({engine: {sendVariableValues: {all: true}}})`. + - `apollo-engine-reporting`: BREAKING CHANGE: By default, send no GraphQL request headers to Apollo's servers instead of sending all. Use the new EngineReportingOption `sendHeaders` to send some or all headers and their values. This replaces the `privateHeaders` option, which is now deprecated. [PR #2847](https://github.com/apollographql/apollo-server/pull/2847) + + Note: This is only a breaking change if the option `privateHeaders` was not previously specified. In order to preserve the old default, pass in the option: + `new ApolloServer({engine: {sendHeaders: {all: true}}})`. + ### v2.6.2 - `apollo-engine-reporting-protobuf`: Update protobuff to include `forbiddenOperations` and `registeredOperations` [PR #2768](https://github.com/apollographql/apollo-server/pull/2768) diff --git a/docs/source/api/apollo-server.md b/docs/source/api/apollo-server.md index 891b8b1eed0..c73f49fb7bd 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 497d7c103e8..787fdd62db1 100644 --- a/packages/apollo-engine-reporting/src/extension.ts +++ b/packages/apollo-engine-reporting/src/extension.ts @@ -444,11 +444,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 && @@ -497,8 +497,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; } @@ -507,7 +507,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.