✨[RUMF-1573] allow to add modifiable field paths with beforeSend (#2186)

* ♻️ specify modifiable paths by event type

* ✨allow to add modifiable field paths with beforeSend

* 👌use camel case

* 👌use getType

* 👌use Record

packages/rum-core/src/domain/assembly.spec.ts

@@ -242,6 +242,22 @@ describe('rum assembly', () => {
 
         expect(serverRumEvents[0].view.id).toBe('aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee')
       })
+
+      it('should not allow to add a sensitive field on the wrong event type', () => {
+        const { lifeCycle } = setupBuilder
+          .withConfiguration({
+            beforeSend: (event) => {
+              event.error = { message: 'added' }
+            },
+          })
+          .build()
+
+        notifyRawRumEvent(lifeCycle, {
+          rawRumEvent: createRawRumEvent(RumEventType.VIEW),
+        })
+
+        expect((serverRumEvents[0] as any).error?.message).toBeUndefined()
+      })
     })
 
     describe('events dismission', () => {

packages/rum-core/src/domain/assembly.ts

@@ -7,6 +7,7 @@ import {
   display,
   createEventRateLimiter,
   canUseEventBridge,
+  assign,
 } from '@datadog/browser-core'
 import type { RumEventDomainContext } from '../domainContext.types'
 import type {
@@ -29,6 +30,7 @@ import type { RumConfiguration } from './configuration'
 import type { ActionContexts } from './rumEventsCollection/action/actionCollection'
 import { getDisplayContext } from './contexts/displayContext'
 import type { CommonContext } from './contexts/commonContext'
+import type { ModifiableFieldPaths } from './limitModification'
 import { limitModification } from './limitModification'
 
 // replaced at build time
@@ -40,21 +42,16 @@ const enum SessionType {
   CI_TEST = 'ci_test',
 }
 
-const VIEW_EVENTS_MODIFIABLE_FIELD_PATHS = [
-  // Fields with sensitive data
-  'view.url',
-  'view.referrer',
-  'action.target.name',
-  'error.message',
-  'error.stack',
-  'error.resource.url',
-  'resource.url',
-]
-
-const OTHER_EVENTS_MODIFIABLE_FIELD_PATHS = VIEW_EVENTS_MODIFIABLE_FIELD_PATHS.concat([
-  // User-customizable field
-  'context',
-])
+const VIEW_MODIFIABLE_FIELD_PATHS: ModifiableFieldPaths = {
+  'view.url': 'string',
+  'view.referrer': 'string',
+}
+
+const USER_CUSTOMIZABLE_FIELD_PATHS: ModifiableFieldPaths = {
+  context: 'object',
+}
+
+let modifiableFieldPathsByEvent: { [key in RumEventType]: ModifiableFieldPaths }
 
 type Mutable<T> = { -readonly [P in keyof T]: T[P] }
 
@@ -68,6 +65,33 @@ export function startRumAssembly(
   buildCommonContext: () => CommonContext,
   reportError: (error: RawError) => void
 ) {
+  modifiableFieldPathsByEvent = {
+    [RumEventType.VIEW]: VIEW_MODIFIABLE_FIELD_PATHS,
+    [RumEventType.ERROR]: assign(
+      {
+        'error.message': 'string',
+        'error.stack': 'string',
+        'error.resource.url': 'string',
+      },
+      USER_CUSTOMIZABLE_FIELD_PATHS,
+      VIEW_MODIFIABLE_FIELD_PATHS
+    ),
+    [RumEventType.RESOURCE]: assign(
+      {
+        'resource.url': 'string',
+      },
+      USER_CUSTOMIZABLE_FIELD_PATHS,
+      VIEW_MODIFIABLE_FIELD_PATHS
+    ),
+    [RumEventType.ACTION]: assign(
+      {
+        'action.target.name': 'string',
+      },
+      USER_CUSTOMIZABLE_FIELD_PATHS,
+      VIEW_MODIFIABLE_FIELD_PATHS
+    ),
+    [RumEventType.LONG_TASK]: assign({}, USER_CUSTOMIZABLE_FIELD_PATHS, VIEW_MODIFIABLE_FIELD_PATHS),
+  }
   const eventRateLimiters = {
     [RumEventType.ERROR]: createEventRateLimiter(
       RumEventType.ERROR,
@@ -155,10 +179,8 @@ function shouldSend(
   eventRateLimiters: { [key in RumEventType]?: EventRateLimiter }
 ) {
   if (beforeSend) {
-    const result = limitModification(
-      event,
-      event.type === RumEventType.VIEW ? VIEW_EVENTS_MODIFIABLE_FIELD_PATHS : OTHER_EVENTS_MODIFIABLE_FIELD_PATHS,
-      (event) => beforeSend(event, domainContext)
+    const result = limitModification(event, modifiableFieldPathsByEvent[event.type], (event) =>
+      beforeSend(event, domainContext)
     )
     if (result === false && event.type !== RumEventType.VIEW) {
       return false

packages/rum-core/src/domain/limitModification.spec.ts

@@ -1,4 +1,6 @@
 import type { Context } from '@datadog/browser-core'
+import { objectEntries } from '@datadog/browser-core'
+import type { ModifiableFieldPaths } from './limitModification'
 import { limitModification } from './limitModification'
 
 describe('limitModification', () => {
@@ -9,7 +11,7 @@ describe('limitModification', () => {
       candidate.qux = 'modified2'
     }
 
-    limitModification(object, ['foo.bar', 'qux'], modifier)
+    limitModification(object, { 'foo.bar': 'string', qux: 'string' }, modifier)
 
     expect(object).toEqual({
       foo: { bar: 'modified1' },
@@ -24,23 +26,40 @@ describe('limitModification', () => {
       candidate.qux = 'modified2'
     }
 
-    limitModification(object, ['foo.bar'], modifier)
+    limitModification(object, { 'foo.bar': 'string' }, modifier)
 
     expect(object).toEqual({
       foo: { bar: 'modified1' },
       qux: 'qux',
     })
   })
 
-  it('should not allow to add a modifiable fields not present on the original object', () => {
+  it('should allow to add a modifiable fields not present on the original object', () => {
     const object = { foo: { bar: 'bar' }, qux: 'qux' }
     const modifier = (candidate: any) => {
       candidate.foo.bar = 'modified1'
       candidate.qux = 'modified2'
       candidate.qix = 'modified3'
     }
 
-    limitModification(object, ['foo.bar', 'qux', 'qix'], modifier)
+    limitModification(object, { 'foo.bar': 'string', qux: 'string', qix: 'string' }, modifier)
+
+    expect(object as any).toEqual({
+      foo: { bar: 'modified1' },
+      qux: 'modified2',
+      qix: 'modified3',
+    })
+  })
+
+  it('should not allow to add a non modifiable fields not present on the original object', () => {
+    const object = { foo: { bar: 'bar' }, qux: 'qux' }
+    const modifier = (candidate: any) => {
+      candidate.foo.bar = 'modified1'
+      candidate.qux = 'modified2'
+      candidate.qix = 'modified3'
+    }
+
+    limitModification(object, { 'foo.bar': 'string', qux: 'string' }, modifier)
 
     expect(object).toEqual({
       foo: { bar: 'modified1' },
@@ -53,39 +72,27 @@ describe('limitModification', () => {
       string_to_undefined: 'bar',
       string_to_number: 'qux',
 
-      null_to_object: null,
       object_to_null: {},
-
-      undefined_to_object: undefined,
       object_to_undefined: {},
-
-      array_to_object: [],
       object_to_array: {},
     }
     const modifier = (candidate: any) => {
       candidate.string_to_undefined = undefined
       candidate.string_to_number = 1234
-      candidate.null_to_object = {}
+
       candidate.object_to_null = null
-      candidate.undefined_to_object = {}
       candidate.object_to_undefined = undefined
-      candidate.array_to_object = {}
       candidate.object_to_array = []
     }
 
-    limitModification(object, Object.keys(object), modifier)
+    limitModification(object, generateModifiableFieldPathsFrom(object), modifier)
 
     expect(object).toEqual({
       string_to_undefined: 'bar',
       string_to_number: 'qux',
 
-      null_to_object: null,
       object_to_null: {},
-
-      undefined_to_object: undefined,
       object_to_undefined: {},
-
-      array_to_object: [],
       object_to_array: {},
     })
   })
@@ -102,7 +109,7 @@ describe('limitModification', () => {
       delete candidate.c
     }
 
-    limitModification(object, Object.keys(object), modifier)
+    limitModification(object, generateModifiableFieldPathsFrom(object), modifier)
 
     expect(object).toEqual({
       a: {},
@@ -119,7 +126,7 @@ describe('limitModification', () => {
       delete candidate.qux
     }
 
-    limitModification(object, ['foo.bar', 'qux'], modifier)
+    limitModification(object, { 'foo.bar': 'string', qux: 'string' }, modifier)
 
     expect(object).toEqual({
       foo: { bar: 'bar' },
@@ -134,7 +141,7 @@ describe('limitModification', () => {
       delete candidate.foo.baz
     }
 
-    limitModification(object, ['foo'], modifier)
+    limitModification(object, { foo: 'object' }, modifier)
 
     expect(object).toEqual({
       foo: { bar: { qux: 'qux' } },
@@ -148,7 +155,7 @@ describe('limitModification', () => {
       return false
     }
 
-    const result = limitModification(object, ['foo.bar', 'qux'], modifier)
+    const result = limitModification(object, { 'foo.bar': 'string', qux: 'string' }, modifier)
 
     expect(result).toBe(false)
     expect(object).toEqual({
@@ -163,7 +170,15 @@ describe('limitModification', () => {
       candidate.bar.self = candidate.bar
     }
 
-    limitModification(object, ['bar'], modifier)
+    limitModification(object, { bar: 'object' }, modifier)
     expect(() => JSON.stringify(object)).not.toThrowError()
   })
 })
+
+function generateModifiableFieldPathsFrom(object: Record<string, string | object>) {
+  const modifiableFieldPaths: ModifiableFieldPaths = {}
+  objectEntries(object).forEach(([key, value]) => {
+    modifiableFieldPaths[key] = typeof value as 'object' | 'string'
+  })
+  return modifiableFieldPaths
+}

packages/rum-core/src/domain/limitModification.ts

@@ -1,26 +1,26 @@
-import { sanitize, deepClone, getType } from '@datadog/browser-core'
+import { sanitize, deepClone, getType, objectEntries } from '@datadog/browser-core'
 import type { Context } from '@datadog/browser-core'
 
+export type ModifiableFieldPaths = Record<string, 'string' | 'object'>
+
 /**
  * Current limitation:
  * - field path do not support array, 'a.b.c' only
  */
 export function limitModification<T extends Context, Result>(
   object: T,
-  modifiableFieldPaths: string[],
+  modifiableFieldPaths: ModifiableFieldPaths,
   modifier: (object: T) => Result
 ): Result | undefined {
   const clone = deepClone(object)
   const result = modifier(clone)
-  modifiableFieldPaths.forEach((path) => {
-    const originalValue = get(object, path)
-    const newValue = get(clone, path)
-    const originalType = getType(originalValue)
+  objectEntries(modifiableFieldPaths).forEach(([fieldPath, fieldType]) => {
+    const newValue = get(clone, fieldPath)
     const newType = getType(newValue)
-    if (newType === originalType) {
-      set(object, path, sanitize(newValue))
-    } else if (originalType === 'object' && (newType === 'undefined' || newType === 'null')) {
-      set(object, path, {})
+    if (newType === fieldType) {
+      set(object, fieldPath, sanitize(newValue))
+    } else if (fieldType === 'object' && (newType === 'undefined' || newType === 'null')) {
+      set(object, fieldPath, {})
     }
   })
   return result
@@ -42,7 +42,7 @@ function set(object: unknown, path: string, value: unknown) {
   const fields = path.split('.')
   for (let i = 0; i < fields.length; i += 1) {
     const field = fields[i]
-    if (!isValidObjectContaining(current, field)) {
+    if (!isValidObject(current)) {
       return
     }
     if (i !== fields.length - 1) {
@@ -53,6 +53,10 @@ function set(object: unknown, path: string, value: unknown) {
   }
 }
 
-function isValidObjectContaining(object: unknown, field: string): object is { [key: string]: unknown } {
-  return typeof object === 'object' && object !== null && Object.prototype.hasOwnProperty.call(object, field)
+function isValidObject(object: unknown): object is Record<string, unknown> {
+  return getType(object) === 'object'
+}
+
+function isValidObjectContaining(object: unknown, field: string): object is Record<string, unknown> {
+  return isValidObject(object) && Object.prototype.hasOwnProperty.call(object, field)
 }