feat(core): Ensure normalizedRequest on sdkProcessingMetadata is merged (#14315)

mydea · web-flow · commit e75bcb8b4e79 · 2024-11-19T16:30:20.000+01:00
We keep `normalizedRequest` with data like headers, urls etc. on the
`sdkProcessingMetadata` on the scope.

While this generally works, there are some _potential_ pitfalls/footguns
there:

One, if you put some (e.g. partial) `normalizedRequest` on the _current_
scope, it will just overwrite the full `normalizedRequest` from the
_isolation_ scope, where generally we'll try to put the request data
automatically (e.g. in the http instrumentation). Think this example:

```js
Sentry.withScope(scope =&gt; {
  scope.setSdkProcessingMetadata({ normalizedRequest: { headers: moreSpecificHeaders } });
});
```

the resulting event inside of this `withScope` callback would _only_
have the headers, but would miss e.g. url etc. data set.

This PR changes this so that the `normalizedRequest` is merged between
the types of scopes, so only set fields on e.g. the current scope will
overwrite the same fields on the isolation scope, instead of just
overwriting the whole normalizedRequest that results.

Note that bundle size for browser is sadly anyhow affected (no matter if
we go with a/b/c), as the code to merge it between scopes is always
shared :(
diff --git a/.size-limit.js b/.size-limit.js
@@ -107,7 +107,7 @@ module.exports = [
     path: 'packages/browser/build/npm/esm/index.js',
     import: createImport('init', 'feedbackAsyncIntegration'),
     gzip: true,
-    limit: '33 KB',
+    limit: '34 KB',
   },
   // React SDK (ESM)
   {
@@ -160,7 +160,7 @@ module.exports = [
     name: 'CDN Bundle (incl. Tracing)',
     path: createCDNPath('bundle.tracing.min.js'),
     gzip: true,
-    limit: '38 KB',
+    limit: '39 KB',
   },
   {
     name: 'CDN Bundle (incl. Tracing, Replay)',
diff --git a/packages/core/src/scope.ts b/packages/core/src/scope.ts
@@ -24,6 +24,7 @@ import type {
 import { dateTimestampInSeconds, generatePropagationContext, isPlainObject, logger, uuid4 } from '@sentry/utils';
 
 import { updateSession } from './session';
+import { merge } from './utils/merge';
 import { _getSpanForScope, _setSpanForScope } from './utils/spanOnScope';
 
 /**
@@ -479,8 +480,7 @@ class ScopeClass implements ScopeInterface {
    * @inheritDoc
    */
   public setSDKProcessingMetadata(newData: { [key: string]: unknown }): this {
-    this._sdkProcessingMetadata = { ...this._sdkProcessingMetadata, ...newData };
-
+    this._sdkProcessingMetadata = merge(this._sdkProcessingMetadata, newData, 2);
     return this;
   }
 
diff --git a/packages/core/src/utils/applyScopeDataToEvent.ts b/packages/core/src/utils/applyScopeDataToEvent.ts
@@ -1,6 +1,7 @@
 import type { Breadcrumb, Event, ScopeData, Span } from '@sentry/types';
 import { arrayify, dropUndefinedKeys } from '@sentry/utils';
 import { getDynamicSamplingContextFromSpan } from '../tracing/dynamicSamplingContext';
+import { merge } from './merge';
 import { getRootSpan, spanToJSON, spanToTraceContext } from './spanUtils';
 
 /**
@@ -46,7 +47,8 @@ export function mergeScopeData(data: ScopeData, mergeData: ScopeData): void {
   mergeAndOverwriteScopeData(data, 'tags', tags);
   mergeAndOverwriteScopeData(data, 'user', user);
   mergeAndOverwriteScopeData(data, 'contexts', contexts);
-  mergeAndOverwriteScopeData(data, 'sdkProcessingMetadata', sdkProcessingMetadata);
+
+  data.sdkProcessingMetadata = merge(data.sdkProcessingMetadata, sdkProcessingMetadata, 2);
 
   if (level) {
     data.level = level;
@@ -87,15 +89,7 @@ export function mergeAndOverwriteScopeData<
   Prop extends 'extra' | 'tags' | 'user' | 'contexts' | 'sdkProcessingMetadata',
   Data extends ScopeData,
 >(data: Data, prop: Prop, mergeVal: Data[Prop]): void {
-  if (mergeVal && Object.keys(mergeVal).length) {
-    // Clone object
-    data[prop] = { ...data[prop] };
-    for (const key in mergeVal) {
-      if (Object.prototype.hasOwnProperty.call(mergeVal, key)) {
-        data[prop][key] = mergeVal[key];
-      }
-    }
-  }
+  data[prop] = merge(data[prop], mergeVal, 1);
 }
 
 /** Exported only for tests */
diff --git a/packages/core/src/utils/merge.ts b/packages/core/src/utils/merge.ts
@@ -0,0 +1,31 @@
+/**
+ * Shallow merge two objects.
+ * Does not mutate the passed in objects.
+ * Undefined/empty values in the merge object will overwrite existing values.
+ *
+ * By default, this merges 2 levels deep.
+ */
+export function merge<T>(initialObj: T, mergeObj: T, levels = 2): T {
+  // If the merge value is not an object, or we have no merge levels left,
+  // we just set the value to the merge value
+  if (!mergeObj || typeof mergeObj !== 'object' || levels <= 0) {
+    return mergeObj;
+  }
+
+  // If the merge object is an empty object, and the initial object is not undefined, we return the initial object
+  if (initialObj && mergeObj && Object.keys(mergeObj).length === 0) {
+    return initialObj;
+  }
+
+  // Clone object
+  const output = { ...initialObj };
+
+  // Merge values into output, resursively
+  for (const key in mergeObj) {
+    if (Object.prototype.hasOwnProperty.call(mergeObj, key)) {
+      output[key] = merge(output[key], mergeObj[key], levels - 1);
+    }
+  }
+
+  return output;
+}
diff --git a/packages/core/test/lib/scope.test.ts b/packages/core/test/lib/scope.test.ts
@@ -204,10 +204,27 @@ describe('Scope', () => {
       expect(scope['_user']).toEqual({});
     });
 
-    test('setProcessingMetadata', () => {
-      const scope = new Scope();
-      scope.setSDKProcessingMetadata({ dogs: 'are great!' });
-      expect(scope['_sdkProcessingMetadata'].dogs).toEqual('are great!');
+    describe('setProcessingMetadata', () => {
+      test('it works with no initial data', () => {
+        const scope = new Scope();
+        scope.setSDKProcessingMetadata({ dogs: 'are great!' });
+        expect(scope['_sdkProcessingMetadata'].dogs).toEqual('are great!');
+      });
+
+      test('it overwrites data', () => {
+        const scope = new Scope();
+        scope.setSDKProcessingMetadata({ dogs: 'are great!' });
+        scope.setSDKProcessingMetadata({ dogs: 'are really great!' });
+        scope.setSDKProcessingMetadata({ cats: 'are also great!' });
+        scope.setSDKProcessingMetadata({ obj: { nested1: 'value1', nested: 'value1' } });
+        scope.setSDKProcessingMetadata({ obj: { nested2: 'value2', nested: 'value2' } });
+
+        expect(scope['_sdkProcessingMetadata']).toEqual({
+          dogs: 'are really great!',
+          cats: 'are also great!',
+          obj: { nested2: 'value2', nested: 'value2', nested1: 'value1' },
+        });
+      });
     });
 
     test('set and get propagation context', () => {
diff --git a/packages/core/test/lib/utils/applyScopeDataToEvent.test.ts b/packages/core/test/lib/utils/applyScopeDataToEvent.test.ts
@@ -134,7 +134,15 @@ describe('mergeScopeData', () => {
       contexts: { os: { name: 'os1' }, culture: { display_name: 'name1' } },
       attachments: [attachment1],
       propagationContext: { spanId: '1', traceId: '1' },
-      sdkProcessingMetadata: { aa: 'aa', bb: 'aa' },
+      sdkProcessingMetadata: {
+        aa: 'aa',
+        bb: 'aa',
+        obj: { key: 'value' },
+        normalizedRequest: {
+          url: 'oldUrl',
+          method: 'oldMethod',
+        },
+      },
       fingerprint: ['aa', 'bb'],
     };
     const data2: ScopeData = {
@@ -146,7 +154,15 @@ describe('mergeScopeData', () => {
       contexts: { os: { name: 'os2' } },
       attachments: [attachment2, attachment3],
       propagationContext: { spanId: '2', traceId: '2' },
-      sdkProcessingMetadata: { bb: 'bb', cc: 'bb' },
+      sdkProcessingMetadata: {
+        bb: 'bb',
+        cc: 'bb',
+        obj: { key2: 'value2' },
+        normalizedRequest: {
+          url: 'newUrl',
+          headers: {},
+        },
+      },
       fingerprint: ['cc'],
     };
     mergeScopeData(data1, data2);
@@ -159,7 +175,17 @@ describe('mergeScopeData', () => {
       contexts: { os: { name: 'os2' }, culture: { display_name: 'name1' } },
       attachments: [attachment1, attachment2, attachment3],
       propagationContext: { spanId: '2', traceId: '2' },
-      sdkProcessingMetadata: { aa: 'aa', bb: 'bb', cc: 'bb' },
+      sdkProcessingMetadata: {
+        aa: 'aa',
+        bb: 'bb',
+        cc: 'bb',
+        obj: { key: 'value', key2: 'value2' },
+        normalizedRequest: {
+          url: 'newUrl',
+          method: 'oldMethod',
+          headers: {},
+        },
+      },
       fingerprint: ['aa', 'bb', 'cc'],
     });
   });
diff --git a/packages/core/test/lib/utils/merge.test.ts b/packages/core/test/lib/utils/merge.test.ts
@@ -0,0 +1,75 @@
+import { merge } from '../../../src/utils/merge';
+
+describe('merge', () => {
+  it('works with empty objects', () => {
+    const oldData = {};
+    const newData = {};
+
+    const actual = merge(oldData, newData);
+
+    expect(actual).toEqual({});
+    expect(actual).toBe(oldData);
+    expect(actual).not.toBe(newData);
+  });
+
+  it('works with empty merge object', () => {
+    const oldData = { aa: 'aha' };
+    const newData = {};
+
+    const actual = merge(oldData, newData);
+
+    expect(actual).toEqual({ aa: 'aha' });
+    expect(actual).toBe(oldData);
+    expect(actual).not.toBe(newData);
+  });
+
+  it('works with arbitrary data', () => {
+    const oldData = {
+      old1: 'old1',
+      old2: 'old2',
+      obj: { key: 'value1', key1: 'value1', deep: { key: 'value' } },
+    } as any;
+    const newData = {
+      new1: 'new1',
+      old2: 'new2',
+      obj: { key2: 'value2', key: 'value2', deep: { key2: 'value2' } },
+    } as any;
+
+    const actual = merge(oldData, newData);
+
+    expect(actual).toEqual({
+      old1: 'old1',
+      old2: 'new2',
+      new1: 'new1',
+      obj: {
+        key2: 'value2',
+        key: 'value2',
+        key1: 'value1',
+        deep: { key2: 'value2' },
+      },
+    });
+    expect(actual).not.toBe(oldData);
+    expect(actual).not.toBe(newData);
+  });
+
+  it.each([
+    [undefined, { a: 'aa' }, { a: 'aa' }],
+    [{ a: 'aa' }, undefined, undefined],
+    [{ a: 'aa' }, null, null],
+    [{ a: 'aa' }, { a: undefined }, { a: undefined }],
+    [{ a: 'aa' }, { a: null }, { a: null }],
+    [{ a: 'aa' }, { a: '' }, { a: '' }],
+    [
+      { a0: { a1: { a2: { a3: { a4: 'a4a' }, a3a: 'a3a' }, a2a: 'a2a' }, a1a: 'a1a' }, a0a: 'a0a' },
+      { a0: { a1: { a2: { a3: { a4: 'a4b' }, a3b: 'a3b' }, a2b: 'a2b' }, a1b: 'a1b' }, a0b: 'a0b' },
+      {
+        a0: { a1: { a2: { a3: { a4: 'a4b' }, a3b: 'a3b' }, a2b: 'a2b' }, a1b: 'a1b', a1a: 'a1a' },
+        a0b: 'a0b',
+        a0a: 'a0a',
+      },
+    ],
+  ])('works with %p and %p', (oldData, newData, expected) => {
+    const actual = merge(oldData, newData as any);
+    expect(actual).toEqual(expected);
+  });
+});
diff --git a/packages/node/src/integrations/http/SentryHttpInstrumentation.ts b/packages/node/src/integrations/http/SentryHttpInstrumentation.ts
@@ -6,7 +6,7 @@ import type { InstrumentationConfig } from '@opentelemetry/instrumentation';
 import { InstrumentationBase, InstrumentationNodeModuleDefinition } from '@opentelemetry/instrumentation';
 import { getRequestInfo } from '@opentelemetry/instrumentation-http';
 import { addBreadcrumb, getClient, getIsolationScope, withIsolationScope } from '@sentry/core';
-import type { PolymorphicRequest, RequestEventData, SanitizedRequestData } from '@sentry/types';
+import type { PolymorphicRequest, RequestEventData, SanitizedRequestData, Scope } from '@sentry/types';
 import {
   getBreadcrumbLogLevelFromHttpStatusCode,
   getSanitizedUrlString,
@@ -150,10 +150,14 @@ export class SentryHttpInstrumentation extends InstrumentationBase<SentryHttpIns
           cookies,
         };
 
-        patchRequestToCaptureBody(request, normalizedRequest);
+        patchRequestToCaptureBody(request, isolationScope);
 
         // Update the isolation scope, isolate this request
-        isolationScope.setSDKProcessingMetadata({ request, normalizedRequest });
+        // TODO(v9): Stop setting `request`, we only rely on normalizedRequest anymore
+        isolationScope.setSDKProcessingMetadata({
+          request,
+          normalizedRequest,
+        });
 
         const client = getClient<NodeClient>();
         if (client && client.getOptions().autoSessionTracking) {
@@ -347,7 +351,7 @@ function getBreadcrumbData(request: http.ClientRequest): Partial<SanitizedReques
  * we monkey patch `req.on('data')` to intercept the body chunks.
  * This way, we only read the body if the user also consumes the body, ensuring we do not change any behavior in unexpected ways.
  */
-function patchRequestToCaptureBody(req: IncomingMessage, normalizedRequest: RequestEventData): void {
+function patchRequestToCaptureBody(req: IncomingMessage, isolationScope: Scope): void {
   const chunks: Buffer[] = [];
 
   function getChunksSize(): number {
@@ -396,9 +400,9 @@ function patchRequestToCaptureBody(req: IncomingMessage, normalizedRequest: Requ
               try {
                 const body = Buffer.concat(chunks).toString('utf-8');
 
-                // We mutate the passed in normalizedRequest and add the body to it
                 if (body) {
-                  normalizedRequest.data = body;
+                  const normalizedRequest = { data: body } satisfies RequestEventData;
+                  isolationScope.setSDKProcessingMetadata({ normalizedRequest });
                 }
               } catch {
                 // ignore errors here
diff --git a/packages/types/src/scope.ts b/packages/types/src/scope.ts
@@ -217,7 +217,9 @@ export interface Scope {
   clearAttachments(): this;
 
   /**
-   * Add data which will be accessible during event processing but won't get sent to Sentry
+   * Add data which will be accessible during event processing but won't get sent to Sentry.
+   *
+   * TODO(v9): We should type this stricter, so that e.g. `normalizedRequest` is strictly typed.
    */
   setSDKProcessingMetadata(newData: { [key: string]: unknown }): this;
 
