diff --git a/projects/ngx-ui-watchtower/DESIGN.md b/projects/ngx-ui-watchtower/DESIGN.md index 1c0d2df..c53747f 100644 --- a/projects/ngx-ui-watchtower/DESIGN.md +++ b/projects/ngx-ui-watchtower/DESIGN.md @@ -1399,6 +1399,27 @@ async load(): Promise { } ``` +Settings that need a `RegExp` or a function — `pagesToExclude` above, or +`urlsToExclude` and `ignore` inside `telemetries` — belong in code like this +(`addXRayTraceIdHeader` too, unless it is a plain boolean). JSON can't carry +them, and a string in their place +would make the SDK throw each time it applies the option — for the page and +URL filters, on every page view or request. + +The sink checks them before loading the SDK and **fails closed**: if any holds +something else, RUM doesn't start for the page, as with a broker returning +`null`, and a console warning names each setting and the type it expects +(never its value) in every environment. Dropping just the bad value would fail +open instead: the filters and `ignore` decide what is _not_ recorded, so RUM +would then record pages and URLs the app meant to keep out — personal data in +a URL among them. + +Any optional setting that is `undefined` or `null` — a broker's JSON often +spells an unset field as `null` — counts as unset: it isn't checked, and the +default applies. For `allowCookies`, `enableXRay`, `endpoint` and `telemetries` +that is this library's own default; every other unset field is left out of the +SDK config, so the SDK's default applies. + ### Per-logger levels `minLevel` is the floor for everything; `levels` overrides it by name, either diff --git a/projects/ngx-ui-watchtower/README.md b/projects/ngx-ui-watchtower/README.md index 954babc..57d9ae9 100644 --- a/projects/ngx-ui-watchtower/README.md +++ b/projects/ngx-ui-watchtower/README.md @@ -95,6 +95,12 @@ export class MyRumCredentials implements UwtRumCredentialsProvider { } ``` +Settings that need a `RegExp` or a function — `pagesToExclude`, or +`urlsToExclude` and `ignore` inside `telemetries` — can't come from JSON. If +one arrives as a string, the sink doesn't start RUM for the page and warns in +the console, rather than let the SDK throw or record what you meant to +exclude. Set those in code. + ### Log backend ```ts diff --git a/projects/ngx-ui-watchtower/rum/src/lib/uwt-rum-credentials/uwt-rum-credentials.ts b/projects/ngx-ui-watchtower/rum/src/lib/uwt-rum-credentials/uwt-rum-credentials.ts index dddeb4f..1246ca4 100644 --- a/projects/ngx-ui-watchtower/rum/src/lib/uwt-rum-credentials/uwt-rum-credentials.ts +++ b/projects/ngx-ui-watchtower/rum/src/lib/uwt-rum-credentials/uwt-rum-credentials.ts @@ -102,6 +102,14 @@ export interface UwtRumAppMonitorConfig { * and same-origin HTTP. The SDK's own default also includes `'replay'` * (session replay via `rrweb`); this library does not enable it by * default. + * + * Options the SDK matches with `RegExp.test` or calls as a function — + * `urlsToInclude`, `urlsToExclude`, `addXRayTraceIdHeader` (also a + * boolean), `ignore`, `interactionId` — must hold exactly that. JSON can't + * carry them, and a string there would make the SDK throw each time it + * applies the option, so RUM doesn't start for the page instead (with a + * console warning naming the option). Set them in code, not in a broker's + * answer. */ telemetries?: (string | (string | object)[])[]; @@ -214,10 +222,19 @@ export interface UwtRumAppMonitorConfig { /** How the page id is derived from the URL. Default `'PATH'`. */ pageIdFormat?: 'PATH' | 'HASH' | 'PATH_AND_HASH'; - /** Pages recorded, matched against the page id. Default matches every page. */ + /** + * Pages recorded, matched against the page id. Default matches every page. + * `undefined` or `null` means unset; anything else that isn't an array of + * `RegExp`s (a string list from JSON) keeps RUM from starting for the page. + */ pagesToInclude?: RegExp[]; - /** Pages excluded from recording, matched against the page id. Default `[]`. */ + /** + * Pages excluded from recording, matched against the page id. Default `[]`. + * `undefined` or `null` means unset; anything else that isn't an array of + * `RegExp`s (a string list from JSON) keeps RUM from starting for the page, + * rather than record what it should exclude. + */ pagesToExclude?: RegExp[]; /** How long, in milliseconds, a route change waits before being considered complete. Default `100`. */ diff --git a/projects/ngx-ui-watchtower/rum/src/lib/uwt-rum-telemetry.sink/uwt-rum-telemetry.sink.spec.ts b/projects/ngx-ui-watchtower/rum/src/lib/uwt-rum-telemetry.sink/uwt-rum-telemetry.sink.spec.ts index 2893eed..f37e970 100644 --- a/projects/ngx-ui-watchtower/rum/src/lib/uwt-rum-telemetry.sink/uwt-rum-telemetry.sink.spec.ts +++ b/projects/ngx-ui-watchtower/rum/src/lib/uwt-rum-telemetry.sink/uwt-rum-telemetry.sink.spec.ts @@ -170,6 +170,132 @@ describe('UwtRumTelemetrySink', () => { expect(sameOrigin.test(`${origin}.attacker.example/api`)).toBe(false); }); + it('should not start RUM when a telemetry option needs a RegExp or a function but holds a string', async () => { + const warn = jest.spyOn(console, 'warn').mockImplementation(() => {}); + configure({ + loadImpl: async () => + bootstrap({ + telemetries: [ + 'errors', + [ + 'http', + { + urlsToExclude: ['/health'], + addXRayTraceIdHeader: ['/api'], + recordAllRequests: true + } + ], + ['performance', { ignore: 'resource', eventLimit: 5 }], + [ + 'interaction', + { interactionId: 'id', events: [{ event: 'click' }] } + ] + ] + }) + }); + await sink.init(); + sink.record('scenario', {}); + + expect(AwsRumCtor).not.toHaveBeenCalled(); + expect(awsRumInstance.recordEvent).not.toHaveBeenCalled(); + // Names and expected types only, never the values. + expect(warn).toHaveBeenCalledWith( + '[ngx-ui-watchtower] RUM not started, settings of the wrong type: ' + + 'http.urlsToExclude (expected an array of RegExp), ' + + 'http.addXRayTraceIdHeader (expected a boolean or an array of RegExp), ' + + 'performance.ignore (expected a function), ' + + 'interaction.interactionId (expected a function)' + ); + }); + + it('should keep telemetry options that are RegExps, functions or booleans', async () => { + const warn = jest.spyOn(console, 'warn').mockImplementation(() => {}); + const ignore = () => false; + const telemetries = [ + ['http', { urlsToExclude: [/\/health/], addXRayTraceIdHeader: true }], + ['errors', { ignore }] + ]; + configure({ loadImpl: async () => bootstrap({ telemetries }) }); + await sink.init(); + + const config = AwsRumCtor.mock.calls[0][3] as { telemetries: unknown[] }; + expect(config.telemetries).toEqual(telemetries); + expect(warn).not.toHaveBeenCalled(); + }); + + it('should not start RUM when a page filter is not a RegExp list', async () => { + const warn = jest.spyOn(console, 'warn').mockImplementation(() => {}); + configure({ + loadImpl: async () => + bootstrap({ + pagesToInclude: [/^\/app/], + pagesToExclude: ['/admin'] as unknown as RegExp[] + }) + }); + await sink.init(); + + expect(AwsRumCtor).not.toHaveBeenCalled(); + expect(warn).toHaveBeenCalledWith( + '[ngx-ui-watchtower] RUM not started, settings of the wrong type: ' + + 'pagesToExclude (expected an array of RegExp)' + ); + }); + + it.each([ + ['a string', 'errors'], + ['an object', { http: true }] + ])( + 'should not start RUM when telemetries is %s, not an array', + async (_label, telemetries) => { + const warn = jest.spyOn(console, 'warn').mockImplementation(() => {}); + configure({ + loadImpl: async () => + bootstrap({ telemetries: telemetries as unknown as string[] }) + }); + await sink.init(); + + expect(AwsRumCtor).not.toHaveBeenCalled(); + expect(warn).toHaveBeenCalledWith( + '[ngx-ui-watchtower] RUM not started, settings of the wrong type: ' + + 'telemetries (expected an array)' + ); + } + ); + + it('should treat settings set to undefined or null as unset', async () => { + const warn = jest.spyOn(console, 'warn').mockImplementation(() => {}); + configure({ + loadImpl: async () => + bootstrap({ + // As a broker's JSON may spell unset fields. + pagesToInclude: null as unknown as RegExp[], + pagesToExclude: undefined, + sessionSampleRate: null as unknown as number, + telemetries: [ + [ + 'http', + { + urlsToExclude: undefined, + urlsToInclude: null, + recordAllRequests: true + } + ] + ] + }) + }); + await sink.init(); + + const config = AwsRumCtor.mock.calls[0][3] as { telemetries: unknown[] }; + // Strict: an explicit `undefined` or `null` would replace the SDK's default. + expect(config.telemetries).toStrictEqual([ + ['http', { recordAllRequests: true }] + ]); + expect(config).not.toHaveProperty('pagesToInclude'); + expect(config).not.toHaveProperty('pagesToExclude'); + expect(config).not.toHaveProperty('sessionSampleRate'); + expect(warn).not.toHaveBeenCalled(); + }); + it('should honour explicitly supplied app monitor settings', async () => { configure({ loadImpl: async () => diff --git a/projects/ngx-ui-watchtower/rum/src/lib/uwt-rum-telemetry.sink/uwt-rum-telemetry.sink.ts b/projects/ngx-ui-watchtower/rum/src/lib/uwt-rum-telemetry.sink/uwt-rum-telemetry.sink.ts index 4a1f575..361b6a6 100644 --- a/projects/ngx-ui-watchtower/rum/src/lib/uwt-rum-telemetry.sink/uwt-rum-telemetry.sink.ts +++ b/projects/ngx-ui-watchtower/rum/src/lib/uwt-rum-telemetry.sink/uwt-rum-telemetry.sink.ts @@ -84,22 +84,128 @@ interface BufferedError { type BufferedItem = BufferedEvent | BufferedPageView | BufferedError; /** - * Drops every key whose value is `undefined`. + * Drops every key whose value is unset: `undefined`, or `null` as a broker's + * JSON spells it. * * Used to build the SDK config object from optional consumer fields: a key - * present with value `undefined` would overwrite the SDK's own default for - * that key, so unset fields must be absent rather than `undefined`. + * present with value `undefined` or `null` would overwrite the SDK's own + * default for that key, so unset fields must be absent. */ -function omitUndefined(obj: T): Partial { +function omitUnset(obj: T): Partial { const result: Partial = {}; for (const key of Object.keys(obj) as (keyof T)[]) { - if (obj[key] !== undefined) { + if (obj[key] !== undefined && obj[key] !== null) { result[key] = obj[key]; } } return result; } +/** Whether `value` is an array of `RegExp`s only. */ +function isRegExpList(value: unknown): boolean { + return ( + Array.isArray(value) && value.every((entry) => entry instanceof RegExp) + ); +} + +/** A setting's type check, and the type it expects, for the warning. */ +interface UwtSettingCheck { + valid: (value: unknown) => boolean; + expected: string; +} + +const REGEXP_LIST: UwtSettingCheck = { + valid: isRegExpList, + expected: 'an array of RegExp' +}; +const FUNCTION: UwtSettingCheck = { + valid: (value) => typeof value === 'function', + expected: 'a function' +}; + +/** + * Telemetry options the SDK calls as a function or matches with + * `RegExp.test`. JSON can carry neither, and anything else in their place + * makes the SDK throw each time it applies the option — for the URL filters, + * inside its patched `fetch` and XHR, so on every application request. + */ +const TELEMETRY_OPTION_CHECKS = new Map([ + ['urlsToInclude', REGEXP_LIST], + ['urlsToExclude', REGEXP_LIST], + [ + 'addXRayTraceIdHeader', + { + valid: (value) => typeof value === 'boolean' || isRegExpList(value), + expected: 'a boolean or an array of RegExp' + } + ], + ['ignore', FUNCTION], + ['interactionId', FUNCTION] +]); + +/** + * Lists the settings in `config` the SDK can't use, each with the type it + * expects: `pagesToInclude` or `pagesToExclude` that isn't an array of + * `RegExp`s, `telemetries` that isn't an array, and each telemetry option + * failing its {@link TELEMETRY_OPTION_CHECKS} check, as `name.option`. + * `undefined` and `null` count as unset, not as invalid. + */ +function invalidRumSettings(config: UwtRumBootstrap['config']): string[] { + const invalid: string[] = []; + for (const key of ['pagesToInclude', 'pagesToExclude'] as const) { + const value = config[key]; + if (value != null && !REGEXP_LIST.valid(value)) { + invalid.push(`${key} (expected ${REGEXP_LIST.expected})`); + } + } + const telemetries: unknown = config.telemetries; + if (telemetries == null) { + return invalid; + } + if (!Array.isArray(telemetries)) { + // A string would be read as one telemetry per character. + invalid.push('telemetries (expected an array)'); + return invalid; + } + for (const telemetry of telemetries) { + if (!Array.isArray(telemetry)) { + continue; + } + const [name, options] = telemetry; + if (typeof options !== 'object' || options === null) { + continue; + } + for (const [key, value] of Object.entries(options)) { + const check = TELEMETRY_OPTION_CHECKS.get(key); + if (value != null && check && !check.valid(value)) { + invalid.push(`${String(name)}.${key} (expected ${check.expected})`); + } + } + } + return invalid; +} + +/** + * Copies `telemetries` without tuple options that are unset (`undefined` or + * `null`). The SDK merges a telemetry's options over its defaults with + * `Object.assign`, so an explicit unset value would replace a default it + * relies on — `urlsToExclude: null` would throw on every request. + */ +function withoutUnsetOptions( + telemetries: NonNullable +): NonNullable { + return telemetries.map((telemetry) => { + if (!Array.isArray(telemetry)) { + return telemetry; + } + const [name, options, ...rest] = telemetry; + if (typeof options !== 'object' || options === null) { + return telemetry; + } + return [name, omitUnset(options), ...rest]; + }); +} + /** * Telemetry sink backed by the AWS CloudWatch RUM web client. * @@ -183,6 +289,18 @@ export class UwtRumTelemetrySink extends UwtTelemetrySink implements OnDestroy { return; } + // Fails closed: the page and URL filters and `ignore` decide what is + // *not* recorded, so dropping one would record what the app meant to + // keep out, and passing it on would make the SDK throw. RUM stays off + // for the page instead, as with a broker that returns `null`. + const invalid = invalidRumSettings(bootstrap.config); + if (invalid.length) { + this.disableAwsRum(); + this.buffer = []; + this.reportInvalidSettings(invalid); + return; + } + const { AwsRum } = await import('aws-rum-web'); if (this.destroyed) { return; @@ -396,12 +514,16 @@ export class UwtRumTelemetrySink extends UwtTelemetrySink implements OnDestroy { * Maps {@link UwtRumAppMonitorConfig} onto the SDK's own `PartialConfig`. * * The SDK merges this object over its own defaults with a shallow - * `Object.assign`, so a key present with value `undefined` overwrites the - * SDK's default instead of falling through to it. `allowCookies`, - * `enableXRay`, `endpoint` and `telemetries` always carry this library's - * own override; every other field goes through {@link omitUndefined} (or, - * for `client`, a conditional spread) so an unset field is absent and the - * SDK's own default applies. + * `Object.assign`, so a key present with value `undefined` or `null` + * overwrites the SDK's default instead of falling through to it. + * `allowCookies`, `enableXRay`, `endpoint` and `telemetries` always carry + * this library's own override; every other field goes through + * {@link omitUnset} (or, for `client`, a conditional spread) so an unset + * field is absent and the SDK's own default applies. Unset telemetry options + * are left out for the same reason. `performInit()` has already checked the + * types of `pagesToInclude`/`pagesToExclude`, `telemetries` and the telemetry + * options in {@link TELEMETRY_OPTION_CHECKS} ({@link invalidRumSettings}); + * every other field is passed on as given. */ private buildRumConfig(bootstrap: UwtRumBootstrap): AwsRumConfig { const { config } = bootstrap; @@ -422,15 +544,17 @@ export class UwtRumTelemetrySink extends UwtTelemetrySink implements OnDestroy { endpoint: config.endpoint ?? `https://dataplane.rum.${config.region}.amazonaws.com`, - telemetries: config.telemetries ?? [ - 'errors', - 'performance', - ['http', { addXRayTraceIdHeader: [sameOrigin] }] - ], + telemetries: Array.isArray(config.telemetries) + ? withoutUnsetOptions(config.telemetries) + : (config.telemetries ?? [ + 'errors', + 'performance', + ['http', { addXRayTraceIdHeader: [sameOrigin] }] + ]), - ...(config.client !== undefined ? { client: config.client } : {}), + ...(config.client != null ? { client: config.client } : {}), - ...omitUndefined({ + ...omitUnset({ releaseId: config.releaseId, clientBuilder: config.clientBuilder, applicationAttributes: config.applicationAttributes, @@ -624,6 +748,21 @@ export class UwtRumTelemetrySink extends UwtTelemetrySink implements OnDestroy { }, CREDENTIAL_RETRY_DELAY_MS); } + /** + * Reports settings that kept RUM from starting, in every environment: a + * silent production page would hide that RUM is off. Each setting's name and + * expected type are printed, never its value. + * + * @param problems one `name (expected type)` entry per setting + */ + private reportInvalidSettings(problems: string[]): void { + uwtSafeVoid('rum.reportInvalidSettings', () => { + console.warn( + `[ngx-ui-watchtower] RUM not started, settings of the wrong type: ${problems.join(', ')}` + ); + }); + } + /** * Reports an init/refresh failure without leaking raw error content into * the production console.