From 7445d3377d16cdc65506131572c0a616d3a6324c Mon Sep 17 00:00:00 2001 From: David Glasser Date: Wed, 10 Aug 2022 16:17:10 -0700 Subject: [PATCH] usage reporting/inline trace plugins: mask errors, change option name (#6794) We replace the `rewriteError` option with `sendErrorsInTraces` (usage reporting) and `includeErrors` (inline trace), which takes `{unmodified: true}`, `{masked:true}`, and `{transform}` variants. This is similar to `sendVariableValues` and `sendHeaders`. Like those two options, this now defaults to a more redacted version: `{masked: true}`. This will reduce unintentional reporting of PII. Part of #6051. Paired with @bonnici. --- .changeset/twenty-cooks-repair.md | 6 ++ docs/source/migration.mdx | 73 +++++++++++++++++++ .../src/apolloServerTests.ts | 55 +++++++------- packages/server/src/ApolloServer.ts | 10 +-- .../server/src/plugin/inlineTrace/index.ts | 28 +++++-- .../server/src/plugin/traceTreeBuilder.ts | 58 +++++++++------ .../server/src/plugin/usageReporting/index.ts | 1 + .../src/plugin/usageReporting/options.ts | 29 ++++++-- .../src/plugin/usageReporting/plugin.ts | 3 +- .../server/src/utils/UnreachableCaseError.ts | 10 +++ 10 files changed, 205 insertions(+), 68 deletions(-) create mode 100644 .changeset/twenty-cooks-repair.md create mode 100644 packages/server/src/utils/UnreachableCaseError.ts diff --git a/.changeset/twenty-cooks-repair.md b/.changeset/twenty-cooks-repair.md new file mode 100644 index 00000000000..7623719b998 --- /dev/null +++ b/.changeset/twenty-cooks-repair.md @@ -0,0 +1,6 @@ +--- +"@apollo/server-integration-testsuite": patch +"@apollo/server": patch +--- + +Usage reporting and inline trace plugins: replace `rewriteError` with `sendErrorsInTraces`/`includeErrors`, and mask all errors by default. diff --git a/docs/source/migration.mdx b/docs/source/migration.mdx index 82434376b2b..b88a5bfb41f 100644 --- a/docs/source/migration.mdx +++ b/docs/source/migration.mdx @@ -930,6 +930,50 @@ ApolloServerPluginUsageReporting({ }); ``` +### `rewriteError` plugin option + +In Apollo Server 3, you can specify a function to rewrite errors before sending them to Apollo's server via the `rewriteError` option to `ApolloServerPluginUsageReporting` (for monoliths) and `ApolloServerPluginInlineTrace` (for subgraphs). + +In Apollo Server 4, you specify the same function as the `transform` option on the `sendErrorsInTrace` option to `ApolloServerPluginUsageReporting` and the `includeErrors` option to `ApolloServerPluginInlineTrace`. + +(Additionally, the [default behavior has changed to mask errors](#usage-reporting-and-inline-trace-plugins-mask-errors-by-default).) + +So, where you previously wrote: + +```ts +// monoliths +new ApolloServer({ + plugins: [ApolloServerPluginUsageReporting({ rewriteError })], + // ... +}) + +// subgraphs +new ApolloServer({ + plugins: [ApolloServerPluginInlineTrace({ rewriteError })], + // ... +}) +``` + +you can now write: + +```ts +// monoliths +new ApolloServer({ + plugins: [ApolloServerPluginUsageReporting({ + sendErrorsInTrace: { transform: rewriteError }, + })], + // ... +}) + +// subgraphs +new ApolloServer({ + plugins: [ApolloServerPluginInlineTrace({ + includeErrors: { transform: rewriteError }, + })], + // ... +}) +``` + ### Doubly-escaped `variables` and `extensions` in requests @@ -1503,6 +1547,35 @@ new ApolloServer({ + +### Usage reporting and inline trace plugins mask errors by default + +In Apollo Server 3, traces sent to Apollo's servers from monolith servers by the usage reporting plugin contain the full message and extensions of every GraphQL error that occurs in the operation by default. Similarly, inline traces sent from subgraphs to Apollo Gateways (which are then sent to Apollo's servers) contain full error details by default. You can modify or drop these errors with `rewriteError` options to the appropriate plugins. + +In Apollo Server 4, error details are *not* included in traces by default. By default, error messages are replaced with `` and error extensions are replaced with a single extension `maskedBy` naming the plugin which performed the masking (`ApolloServerPluginUsageReporting` or `ApolloServerPluginInlineTrace`). + +To restore the Apollo Server 3 behavior, you can pass `{ unmodified: true }` to an option on each plugin: + +```ts +// monoliths +new ApolloServer({ + plugins: [ApolloServerPluginUsageReporting({ + sendErrorsInTrace: { unmodified: true }, + })], + // ... +}) + +// subgraphs +new ApolloServer({ + plugins: [ApolloServerPluginInlineTrace({ + includeErrors: { unmodified: true }, + })], + // ... +}) +``` + +(As [described above](#rewriteerror-plugin-option), the `rewriteError` option has been replaced by a `transform` option on `sendErrorsInTrace` or `includeErrors`.) + ## Renamed packages The following packages have been renamed in Apollo Server 4: diff --git a/packages/integration-testsuite/src/apolloServerTests.ts b/packages/integration-testsuite/src/apolloServerTests.ts index 0f56affed46..c55fec54823 100644 --- a/packages/integration-testsuite/src/apolloServerTests.ts +++ b/packages/integration-testsuite/src/apolloServerTests.ts @@ -1139,15 +1139,17 @@ export function defineIntegrationTestSuiteApolloServerTests( }); describe('error munging', () => { - describe('rewriteError', () => { + describe('sendErrorsInTraces', () => { it('new error', async () => { throwError.mockImplementationOnce(() => { - throw new Error('rewriteError nope'); + throw new Error('transform nope'); }); await setupApolloServerAndFetchPair({ - rewriteError: () => - new GraphQLError('rewritten as a new error'), + sendErrorsInTraces: { + transform: () => + new GraphQLError('rewritten as a new error'), + }, }); const result = await apolloFetch({ @@ -1159,7 +1161,7 @@ export function defineIntegrationTestSuiteApolloServerTests( expect(result.errors).toBeDefined(); // The original error message should be sent to the client. - expect(result.errors[0].message).toEqual('rewriteError nope'); + expect(result.errors[0].message).toEqual('transform nope'); expect(throwError).toHaveBeenCalledTimes(1); const reports = await reportIngress.promiseOfReports; @@ -1185,13 +1187,15 @@ export function defineIntegrationTestSuiteApolloServerTests( it('modified error', async () => { throwError.mockImplementationOnce(() => { - throw new Error('rewriteError mod nope'); + throw new Error('transform mod nope'); }); await setupApolloServerAndFetchPair({ - rewriteError: (err) => { - err.message = 'rewritten as a modified error'; - return err; + sendErrorsInTraces: { + transform: (err) => { + err.message = 'rewritten as a modified error'; + return err; + }, }, }); @@ -1202,9 +1206,7 @@ export function defineIntegrationTestSuiteApolloServerTests( fieldWhichWillError: null, }); expect(result.errors).toBeDefined(); - expect(result.errors[0].message).toEqual( - 'rewriteError mod nope', - ); + expect(result.errors[0].message).toEqual('transform mod nope'); expect(throwError).toHaveBeenCalledTimes(1); const reports = await reportIngress.promiseOfReports; @@ -1230,11 +1232,11 @@ export function defineIntegrationTestSuiteApolloServerTests( it('nulled error', async () => { throwError.mockImplementationOnce(() => { - throw new Error('rewriteError null nope'); + throw new Error('transform null nope'); }); await setupApolloServerAndFetchPair({ - rewriteError: () => null, + sendErrorsInTraces: { transform: () => null }, }); const result = await apolloFetch({ @@ -1244,9 +1246,7 @@ export function defineIntegrationTestSuiteApolloServerTests( fieldWhichWillError: null, }); expect(result.errors).toBeDefined(); - expect(result.errors[0].message).toEqual( - 'rewriteError null nope', - ); + expect(result.errors[0].message).toEqual('transform null nope'); expect(throwError).toHaveBeenCalledTimes(1); const reports = await reportIngress.promiseOfReports; @@ -1267,11 +1267,14 @@ export function defineIntegrationTestSuiteApolloServerTests( it('undefined error', async () => { throwError.mockImplementationOnce(() => { - throw new Error('rewriteError undefined whoops'); + throw new Error('transform undefined whoops'); }); await setupApolloServerAndFetchPair({ - rewriteError: () => undefined as any, + sendErrorsInTraces: { + // @ts-expect-error (not allowed to be undefined) + transform: () => undefined, + }, }); const result = await apolloFetch({ @@ -1282,7 +1285,7 @@ export function defineIntegrationTestSuiteApolloServerTests( }); expect(result.errors).toBeDefined(); expect(result.errors[0].message).toEqual( - 'rewriteError undefined whoops', + 'transform undefined whoops', ); expect(throwError).toHaveBeenCalledTimes(1); @@ -1301,8 +1304,8 @@ export function defineIntegrationTestSuiteApolloServerTests( // rewritten. expect(trace.root!.child![0].error).toMatchObject([ { - json: '{"message":"rewriteError undefined whoops","locations":[{"line":1,"column":2}],"path":["fieldWhichWillError"]}', - message: 'rewriteError undefined whoops', + json: '{"message":"transform undefined whoops","locations":[{"line":1,"column":2}],"path":["fieldWhichWillError"]}', + message: 'transform undefined whoops', location: [{ column: 2, line: 1 }], }, ]); @@ -2203,9 +2206,11 @@ export function defineIntegrationTestSuiteApolloServerTests( }, plugins: [ ApolloServerPluginInlineTrace({ - rewriteError(err) { - err.message = `Rewritten for Usage Reporting: ${err.message}`; - return err; + includeErrors: { + transform(err) { + err.message = `Rewritten for Usage Reporting: ${err.message}`; + return err; + }, }, }), ], diff --git a/packages/server/src/ApolloServer.ts b/packages/server/src/ApolloServer.ts index 7e7f94726d9..e20b48dac54 100644 --- a/packages/server/src/ApolloServer.ts +++ b/packages/server/src/ApolloServer.ts @@ -59,6 +59,7 @@ import { } from './runHttpQuery.js'; import { SchemaManager } from './utils/schemaManager.js'; import { isDefined } from './utils/isDefined.js'; +import { UnreachableCaseError } from './utils/UnreachableCaseError.js'; import type { WithRequired } from '@apollo/utils.withrequired'; import type { ApolloServerOptionsWithStaticSchema } from './externalTypes/constructor'; import type { GatewayExecutor } from '@apollo/server-gateway-interface'; @@ -128,15 +129,6 @@ type ServerState = stopError: Error | null; }; -// Throw this in places that should be unreachable (because all other cases have -// been handled, reducing the type of the argument to `never`). TypeScript will -// complain if in fact there is a valid type for the argument. -class UnreachableCaseError extends Error { - constructor(val: never) { - super(`Unreachable case: ${val}`); - } -} - // TODO(AS4): Move this to its own file or something. Also organize the fields. export interface ApolloServerInternals { diff --git a/packages/server/src/plugin/inlineTrace/index.ts b/packages/server/src/plugin/inlineTrace/index.ts index 85c138dbad7..410b81f0c04 100644 --- a/packages/server/src/plugin/inlineTrace/index.ts +++ b/packages/server/src/plugin/inlineTrace/index.ts @@ -1,18 +1,31 @@ import { Trace } from '@apollo/usage-reporting-protobuf'; import { TraceTreeBuilder } from '../traceTreeBuilder.js'; -import type { ApolloServerPluginUsageReportingOptions } from '../usageReporting/options'; +import type { SendErrorsOptions } from '../usageReporting/index.js'; import { internalPlugin } from '../../internalPlugin.js'; import { schemaIsFederated } from '../schemaIsFederated.js'; import type { ApolloServerPlugin, BaseContext } from '../../externalTypes'; export interface ApolloServerPluginInlineTraceOptions { /** - * By default, all errors from this service get included in the trace. You - * can specify a filter function to exclude specific errors from being - * reported by returning an explicit `null`, or you can mask certain details - * of the error by modifying it and returning the modified error. + * By default, if a trace contains errors, the errors are included in the + * trace with the message ``. The errors are associated with specific + * paths in the operation, but do not include the original error message or + * any extensions such as the error `code`, as those details may contain your + * users' private data. The extension `maskedBy: + * 'ApolloServerPluginInlineTrace'` is added. + * + * If you'd like details about the error included in traces, set this option. + * This option can take several forms: + * + * - { masked: true }: mask error messages and omit extensions (DEFAULT) + * - { unmodified: true }: include all error messages and extensions + * - { transform: ... }: a custom function for transforming errors. This + * function receives a `GraphQLError` and may return a `GraphQLError` + * (either a new error, or its potentially-modified argument) or `null`. + * This error is used in the trace; if `null`, the error is not included in + * traces or error statistics. */ - rewriteError?: ApolloServerPluginUsageReportingOptions['rewriteError']; + includeErrors?: SendErrorsOptions; /** * This option is for internal use by `@apollo/server` only. * @@ -60,7 +73,8 @@ export function ApolloServerPluginInlineTrace( } const treeBuilder = new TraceTreeBuilder({ - rewriteError: options.rewriteError, + maskedBy: 'ApolloServerPluginInlineTrace', + sendErrors: options.includeErrors, }); // XXX Provide a mechanism to customize this logic. diff --git a/packages/server/src/plugin/traceTreeBuilder.ts b/packages/server/src/plugin/traceTreeBuilder.ts index b72cb57a3b3..63853c36b62 100644 --- a/packages/server/src/plugin/traceTreeBuilder.ts +++ b/packages/server/src/plugin/traceTreeBuilder.ts @@ -3,6 +3,8 @@ import { GraphQLError, GraphQLResolveInfo, ResponsePath } from 'graphql'; import { Trace, google } from '@apollo/usage-reporting-protobuf'; import type { Logger } from '@apollo/utils.logger'; +import type { SendErrorsOptions } from './usageReporting'; +import { UnreachableCaseError } from '../utils/UnreachableCaseError.js'; function internalError(message: string) { return new Error(`[internal apollo-server error] ${message}`); @@ -10,7 +12,7 @@ function internalError(message: string) { export class TraceTreeBuilder { private rootNode = new Trace.Node(); - private logger: Logger = console; + private logger: Logger = console; // TODO(AS4): why have this default? public trace = new Trace({ root: this.rootNode, // By default, each trace counts as one operation for the sake of field @@ -27,14 +29,29 @@ export class TraceTreeBuilder { private nodes = new Map([ [responsePathAsString(), this.rootNode], ]); - private readonly rewriteError?: (err: GraphQLError) => GraphQLError | null; + private readonly transformError: + | ((err: GraphQLError) => GraphQLError | null) + | null; public constructor(options: { + maskedBy: string; logger?: Logger; - rewriteError?: (err: GraphQLError) => GraphQLError | null; + sendErrors?: SendErrorsOptions; }) { - this.rewriteError = options.rewriteError; - if (options.logger) this.logger = options.logger; + const { logger, sendErrors, maskedBy } = options; + if (!sendErrors || 'masked' in sendErrors) { + this.transformError = () => + new GraphQLError('', { + extensions: { maskedBy }, + }); + } else if ('transform' in sendErrors) { + this.transformError = sendErrors.transform; + } else if ('unmodified' in sendErrors) { + this.transformError = null; + } else { + throw new UnreachableCaseError(sendErrors); + } + if (logger) this.logger = logger; } public startTiming() { @@ -99,10 +116,10 @@ export class TraceTreeBuilder { } // In terms of reporting, errors can be re-written by the user by - // utilizing the `rewriteError` parameter. This allows changing + // utilizing the `transformError` parameter. This allows changing // the message or stack to remove potentially sensitive information. // Returning `null` will result in the error not being reported at all. - const errorForReporting = this.rewriteAndNormalizeError(err); + const errorForReporting = this.transformAndNormalizeError(err); if (errorForReporting === null) { return; @@ -171,9 +188,9 @@ export class TraceTreeBuilder { return this.newNode(path.prev!); } - private rewriteAndNormalizeError(err: GraphQLError): GraphQLError | null { - if (this.rewriteError) { - // Before passing the error to the user-provided `rewriteError` function, + private transformAndNormalizeError(err: GraphQLError): GraphQLError | null { + if (this.transformError) { + // Before passing the error to the user-provided `transformError` function, // we'll make a shadow copy of the error so the user is free to change // the object as they see fit. @@ -189,7 +206,7 @@ export class TraceTreeBuilder { err, ); - const rewrittenError = this.rewriteError(clonedError); + const rewrittenError = this.transformError(clonedError); // Returning an explicit `null` means the user is requesting that the error // not be reported to Apollo. @@ -204,21 +221,20 @@ export class TraceTreeBuilder { return err; } - // We only allow rewriteError to change the message and extensions of the + // We only allow transformError to change the message and extensions of the // error; we keep everything else the same. That way people don't have to // do extra work to keep the error on the same trace node. We also keep // extensions the same if it isn't explicitly changed (to, eg, {}). (Note // that many of the fields of GraphQLError are not enumerable and won't // show up in the trace (even in the json field) anyway.) - return new GraphQLError( - rewrittenError.message, - err.nodes, - err.source, - err.positions, - err.path, - err.originalError, - rewrittenError.extensions || err.extensions, - ); + return new GraphQLError(rewrittenError.message, { + nodes: err.nodes, + source: err.source, + positions: err.positions, + path: err.path, + originalError: err.originalError, + extensions: rewrittenError.extensions || err.extensions, + }); } return err; } diff --git a/packages/server/src/plugin/usageReporting/index.ts b/packages/server/src/plugin/usageReporting/index.ts index 041c2c42442..cb8537632b7 100644 --- a/packages/server/src/plugin/usageReporting/index.ts +++ b/packages/server/src/plugin/usageReporting/index.ts @@ -3,6 +3,7 @@ export type { ApolloServerPluginUsageReportingOptions, SendValuesBaseOptions, VariableValueOptions, + SendErrorsOptions, ClientInfo, GenerateClientInfo, } from './options.js'; diff --git a/packages/server/src/plugin/usageReporting/options.ts b/packages/server/src/plugin/usageReporting/options.ts index b823bf5dedb..8dc456d0253 100644 --- a/packages/server/src/plugin/usageReporting/options.ts +++ b/packages/server/src/plugin/usageReporting/options.ts @@ -47,12 +47,26 @@ export interface ApolloServerPluginUsageReportingOptions< */ sendHeaders?: SendValuesBaseOptions; /** - * By default, all errors get reported to Apollo servers. You can specify - * a filter function to exclude specific errors from being reported by - * returning an explicit `null`, or you can mask certain details of the error - * by modifying it and returning the modified error. + * By default, if a trace contains errors, the errors are reported to Apollo + * servers with the message ``. The errors are associated with + * specific paths in the operation, but do not include the original error + * message or any extensions such as the error `code`, as those details may + * contain your users' private data. The extension `maskedBy: + * 'ApolloServerPluginUsageReporting'` is added. + * + * If you'd like details about the error included in traces, set this option. + * This option can take several forms: + * + * - { masked: true }: mask error messages and omit extensions (DEFAULT) + * - { unmodified: true }: send all error messages and extensions to Apollo + * servers + * - { transform: ... }: a custom function for transforming errors. This + * function receives a `GraphQLError` and may return a `GraphQLError` + * (either a new error, or its potentially-modified argument) or `null`. + * This error is used in the report to Apollo servers; if `null`, the error + * is not included in traces or error statistics. */ - rewriteError?: (err: GraphQLError) => GraphQLError | null; + sendErrorsInTraces?: SendErrorsOptions; // We should strongly consider changing the default to false in AS4. @@ -321,6 +335,11 @@ export type VariableValueOptions = } | SendValuesBaseOptions; +export type SendErrorsOptions = + | { unmodified: true } + | { masked: true } + | { transform: (err: GraphQLError) => GraphQLError | null }; + export interface ClientInfo { clientName?: string; clientVersion?: string; diff --git a/packages/server/src/plugin/usageReporting/plugin.ts b/packages/server/src/plugin/usageReporting/plugin.ts index ec84eba8d9f..a8fc2d68fed 100644 --- a/packages/server/src/plugin/usageReporting/plugin.ts +++ b/packages/server/src/plugin/usageReporting/plugin.ts @@ -388,7 +388,8 @@ export function ApolloServerPluginUsageReporting( }): GraphQLRequestListener => { const logger = options.logger ?? server.logger; const treeBuilder: TraceTreeBuilder = new TraceTreeBuilder({ - rewriteError: options.rewriteError, + maskedBy: 'ApolloServerPluginUsageReporting', + sendErrors: options.sendErrorsInTraces, logger, }); treeBuilder.startTiming(); diff --git a/packages/server/src/utils/UnreachableCaseError.ts b/packages/server/src/utils/UnreachableCaseError.ts new file mode 100644 index 00000000000..6579bce7f48 --- /dev/null +++ b/packages/server/src/utils/UnreachableCaseError.ts @@ -0,0 +1,10 @@ +/** + * Throw this in places that should be unreachable (because all other cases have + * been handled, reducing the type of the argument to `never`). TypeScript will + * complain if in fact there is a valid type for the argument. + */ +export class UnreachableCaseError extends Error { + constructor(val: never) { + super(`Unreachable case: ${val}`); + } +}