Merge branch 'develop' into fn/headers-unify

mydea · mydea · commit 0d5c32d9df22 · 2024-11-21T13:57:02.000+01:00
diff --git a/dev-packages/e2e-tests/test-applications/nextjs-turbo/package.json b/dev-packages/e2e-tests/test-applications/nextjs-turbo/package.json
@@ -46,6 +46,7 @@
     "extends": "../../package.json"
   },
   "sentryTest": {
+    "optional": true,
     "optionalVariants": [
       {
         "build-command": "test:build-canary",
diff --git a/packages/core/src/utils-hoist/index.ts b/packages/core/src/utils-hoist/index.ts
@@ -73,6 +73,7 @@ export {
   extractRequestData,
   winterCGHeadersToDict,
   winterCGRequestToRequestData,
+  httpRequestToRequestData,
   extractQueryParamsFromUrl,
   headersToDict,
 } from './requestdata';
diff --git a/packages/core/src/utils-hoist/object.ts b/packages/core/src/utils-hoist/object.ts
@@ -32,7 +32,11 @@ export function fill(source: { [key: string]: any }, name: string, replacementFa
     markFunctionWrapped(wrapped, original);
   }
 
-  source[name] = wrapped;
+  try {
+    source[name] = wrapped;
+  } catch {
+    DEBUG_BUILD && logger.log(`Failed to replace method "${name}" in object`, source);
+  }
 }
 
 /**
diff --git a/packages/core/src/utils-hoist/requestdata.ts b/packages/core/src/utils-hoist/requestdata.ts
@@ -14,6 +14,7 @@ import { DEBUG_BUILD } from './debug-build';
 import { isPlainObject, isString } from './is';
 import { logger } from './logger';
 import { normalize } from './normalize';
+import { dropUndefinedKeys } from './object';
 import { truncate } from './string';
 import { stripUrlQueryAndFragment } from './url';
 import { getClientIPAddress, ipHeaderNames } from './vendor/getIpAddress';
@@ -451,6 +452,43 @@ export function winterCGRequestToRequestData(req: WebFetchRequest): RequestEvent
   };
 }
 
+/**
+ * Convert a HTTP request object to RequestEventData to be passed as normalizedRequest.
+ * Instead of allowing `PolymorphicRequest` to be passed,
+ * we want to be more specific and generally require a http.IncomingMessage-like object.
+ */
+export function httpRequestToRequestData(request: {
+  method?: string;
+  url?: string;
+  headers?: {
+    [key: string]: string | string[] | undefined;
+  };
+  protocol?: string;
+  socket?: unknown;
+}): RequestEventData {
+  const headers = request.headers || {};
+  const host = headers.host || '<no host>';
+  const protocol = request.socket && (request.socket as { encrypted?: boolean }).encrypted ? 'https' : 'http';
+  const originalUrl = request.url || '';
+  const absoluteUrl = originalUrl.startsWith(protocol) ? originalUrl : `${protocol}://${host}${originalUrl}`;
+
+  // This is non-standard, but may be sometimes set
+  // It may be overwritten later by our own body handling
+  const data = (request as PolymorphicRequest).body || undefined;
+
+  // This is non-standard, but may be set on e.g. Next.js or Express requests
+  const cookies = (request as PolymorphicRequest).cookies;
+
+  return dropUndefinedKeys({
+    url: absoluteUrl,
+    method: request.method,
+    query_string: extractQueryParamsFromUrl(originalUrl),
+    headers: headersToDict(headers),
+    cookies,
+    data,
+  });
+}
+
 /** Extract the query params from an URL. */
 export function extractQueryParamsFromUrl(url: string): string | undefined {
   // url is path and query string
diff --git a/packages/core/test/utils-hoist/object.test.ts b/packages/core/test/utils-hoist/object.test.ts
@@ -31,6 +31,29 @@ describe('fill()', () => {
     expect(replacement).toBeCalled();
   });
 
+  test('does not throw on readonly properties', () => {
+    const originalFn = () => 41;
+    const source = {
+      get prop() {
+        return originalFn;
+      },
+      set prop(_fn: () => number) {
+        throw new Error('OH NO, this is not writeable...');
+      },
+    };
+
+    expect(source.prop()).toEqual(41);
+
+    const replacement = jest.fn().mockImplementation(() => {
+      return () => 42;
+    });
+    fill(source, 'prop', replacement);
+    expect(replacement).toBeCalled();
+
+    expect(source.prop).toBe(originalFn);
+    expect(source.prop()).toEqual(41);
+  });
+
   test('can do anything inside replacement function', () => {
     const source = {
       foo: (): number => 42,
diff --git a/packages/google-cloud-serverless/src/gcpfunction/http.ts b/packages/google-cloud-serverless/src/gcpfunction/http.ts
@@ -2,11 +2,13 @@ import {
   SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
   SEMANTIC_ATTRIBUTE_SENTRY_SOURCE,
   handleCallbackErrors,
+  httpRequestToRequestData,
+  isString,
+  logger,
   setHttpStatus,
+  stripUrlQueryAndFragment,
 } from '@sentry/core';
-import { isString, logger, stripUrlQueryAndFragment } from '@sentry/core';
 import { captureException, continueTrace, flush, getCurrentScope, startSpanManual } from '@sentry/node';
-
 import { DEBUG_BUILD } from '../debug-build';
 import { domainify, markEventUnhandled, proxyFunction } from '../utils';
 import type { HttpFunction, WrapperOptions } from './general';
@@ -44,6 +46,9 @@ function _wrapHttpFunction(fn: HttpFunction, options: Partial<WrapperOptions>):
     const baggage = req.headers?.baggage;
 
     return continueTrace({ sentryTrace, baggage }, () => {
+      const normalizedRequest = httpRequestToRequestData(req);
+      getCurrentScope().setSDKProcessingMetadata({ normalizedRequest });
+
       return startSpanManual(
         {
           name: `${reqMethod} ${reqUrl}`,
@@ -54,10 +59,6 @@ function _wrapHttpFunction(fn: HttpFunction, options: Partial<WrapperOptions>):
           },
         },
         span => {
-          getCurrentScope().setSDKProcessingMetadata({
-            request: req,
-          });
-
           // eslint-disable-next-line @typescript-eslint/unbound-method
           const _end = res.end;
           // eslint-disable-next-line @typescript-eslint/no-explicit-any
diff --git a/packages/google-cloud-serverless/test/gcpfunction/http.test.ts b/packages/google-cloud-serverless/test/gcpfunction/http.test.ts
@@ -176,11 +176,12 @@ describe('GCPFunction', () => {
     expect(defaultIntegrations).toContain('RequestData');
 
     expect(mockScope.setSDKProcessingMetadata).toHaveBeenCalledWith({
-      request: {
+      normalizedRequest: {
         method: 'POST',
-        url: '/path?q=query',
+        url: 'http://hostname/path?q=query',
         headers: { host: 'hostname', 'content-type': 'application/json' },
-        body: { foo: 'bar' },
+        query_string: 'q=query',
+        data: { foo: 'bar' },
       },
     });
   });
diff --git a/packages/nextjs/src/common/nextNavigationErrorUtils.ts b/packages/nextjs/src/common/nextNavigationErrorUtils.ts
@@ -5,7 +5,12 @@ import { isError } from '@sentry/core';
  * https://beta.nextjs.org/docs/api-reference/notfound#notfound
  */
 export function isNotFoundNavigationError(subject: unknown): boolean {
-  return isError(subject) && (subject as Error & { digest?: unknown }).digest === 'NEXT_NOT_FOUND';
+  return (
+    isError(subject) &&
+    ['NEXT_NOT_FOUND', 'NEXT_HTTP_ERROR_FALLBACK;404'].includes(
+      (subject as Error & { digest?: unknown }).digest as string,
+    )
+  );
 }
 
 /**
diff --git a/packages/nextjs/src/common/pages-router-instrumentation/_error.ts b/packages/nextjs/src/common/pages-router-instrumentation/_error.ts
@@ -1,4 +1,4 @@
-import { captureException, withScope } from '@sentry/core';
+import { captureException, httpRequestToRequestData, withScope } from '@sentry/core';
 import { vercelWaitUntil } from '@sentry/core';
 import type { NextPageContext } from 'next';
 import { flushSafelyWithTimeout } from '../utils/responseEnd';
@@ -38,7 +38,8 @@ export async function captureUnderscoreErrorException(contextOrProps: ContextOrP
 
   withScope(scope => {
     if (req) {
-      scope.setSDKProcessingMetadata({ request: req });
+      const normalizedRequest = httpRequestToRequestData(req);
+      scope.setSDKProcessingMetadata({ normalizedRequest });
     }
 
     // If third-party libraries (or users themselves) throw something falsy, we want to capture it as a message (which
diff --git a/packages/nextjs/src/common/pages-router-instrumentation/wrapApiHandlerWithSentry.ts b/packages/nextjs/src/common/pages-router-instrumentation/wrapApiHandlerWithSentry.ts
@@ -3,6 +3,7 @@ import {
   SEMANTIC_ATTRIBUTE_SENTRY_SOURCE,
   captureException,
   continueTrace,
+  httpRequestToRequestData,
   setHttpStatus,
   startSpanManual,
   withIsolationScope,
@@ -65,8 +66,9 @@ export function wrapApiHandlerWithSentry(apiHandler: NextApiHandler, parameteriz
             },
             () => {
               const reqMethod = `${(req.method || 'GET').toUpperCase()} `;
+              const normalizedRequest = httpRequestToRequestData(req);
 
-              isolationScope.setSDKProcessingMetadata({ request: req });
+              isolationScope.setSDKProcessingMetadata({ normalizedRequest });
               isolationScope.setTransactionName(`${reqMethod}${parameterizedRoute}`);
 
               return startSpanManual(
diff --git a/packages/nextjs/src/common/utils/wrapperUtils.ts b/packages/nextjs/src/common/utils/wrapperUtils.ts
@@ -6,6 +6,7 @@ import {
   getIsolationScope,
   getRootSpan,
   getTraceData,
+  httpRequestToRequestData,
 } from '@sentry/core';
 import { TRANSACTION_ATTR_SENTRY_ROUTE_BACKFILL } from '../span-attributes-with-logic-attached';
 
@@ -61,10 +62,9 @@ export function withTracedServerSideDataFetcher<F extends (...args: any[]) => Pr
     this: unknown,
     ...args: Parameters<F>
   ): Promise<{ data: ReturnType<F>; sentryTrace?: string; baggage?: string }> {
+    const normalizedRequest = httpRequestToRequestData(req);
     getCurrentScope().setTransactionName(`${options.dataFetchingMethodName} (${options.dataFetcherRouteName})`);
-    getIsolationScope().setSDKProcessingMetadata({
-      request: req,
-    });
+    getIsolationScope().setSDKProcessingMetadata({ normalizedRequest });
 
     const span = getActiveSpan();
 
diff --git a/packages/nextjs/src/common/wrapMiddlewareWithSentry.ts b/packages/nextjs/src/common/wrapMiddlewareWithSentry.ts
@@ -8,9 +8,10 @@ import {
   handleCallbackErrors,
   setCapturedScopesOnSpan,
   startSpan,
+  vercelWaitUntil,
+  winterCGRequestToRequestData,
   withIsolationScope,
 } from '@sentry/core';
-import { vercelWaitUntil, winterCGRequestToRequestData } from '@sentry/core';
 import type { TransactionSource } from '@sentry/types';
 import type { EdgeRouteHandler } from '../edge/types';
 import { flushSafelyWithTimeout } from './utils/responseEnd';
diff --git a/packages/node/src/integrations/http/SentryHttpInstrumentation.ts b/packages/node/src/integrations/http/SentryHttpInstrumentation.ts
@@ -5,17 +5,19 @@ import { VERSION } from '@opentelemetry/core';
 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 {
-  extractQueryParamsFromUrl,
+  addBreadcrumb,
   getBreadcrumbLogLevelFromHttpStatusCode,
+  getClient,
+  getIsolationScope,
   getSanitizedUrlString,
-  headersToDict,
+  httpRequestToRequestData,
   logger,
   parseUrl,
   stripUrlQueryAndFragment,
+  withIsolationScope,
 } from '@sentry/core';
-import type { PolymorphicRequest, RequestEventData, SanitizedRequestData, Scope } from '@sentry/types';
+import type { RequestEventData, SanitizedRequestData, Scope } from '@sentry/types';
 import { DEBUG_BUILD } from '../../debug-build';
 import type { NodeClient } from '../../sdk/client';
 import { getRequestUrl } from '../../utils/getRequestUrl';
@@ -131,26 +133,10 @@ export class SentryHttpInstrumentation extends InstrumentationBase<SentryHttpIns
 
         instrumentation._diag.debug('http instrumentation for incoming request');
 
-        const request = args[0] as http.IncomingMessage;
-
         const isolationScope = getIsolationScope().clone();
+        const request = args[0] as http.IncomingMessage;
 
-        const headers = request.headers;
-        const host = headers.host || '<no host>';
-        const protocol = request.socket && (request.socket as { encrypted?: boolean }).encrypted ? 'https' : 'http';
-        const originalUrl = request.url || '';
-        const absoluteUrl = originalUrl.startsWith(protocol) ? originalUrl : `${protocol}://${host}${originalUrl}`;
-
-        // This is non-standard, but may be set on e.g. Next.js or Express requests
-        const cookies = (request as PolymorphicRequest).cookies;
-
-        const normalizedRequest: RequestEventData = {
-          url: absoluteUrl,
-          method: request.method,
-          query_string: extractQueryParamsFromUrl(request.url || ''),
-          headers: headersToDict(request.headers),
-          cookies,
-        };
+        const normalizedRequest = httpRequestToRequestData(request);
 
         patchRequestToCaptureBody(request, isolationScope);
 
diff --git a/packages/remix/src/utils/errors.ts b/packages/remix/src/utils/errors.ts
@@ -1,13 +1,20 @@
 import type { AppData, DataFunctionArgs, EntryContext, HandleDocumentRequestFunction } from '@remix-run/node';
-import { captureException, getClient, handleCallbackErrors } from '@sentry/core';
-import { addExceptionMechanism, isPrimitive, logger, objectify } from '@sentry/core';
-import type { Span } from '@sentry/types';
+import {
+  addExceptionMechanism,
+  captureException,
+  getClient,
+  handleCallbackErrors,
+  isPrimitive,
+  logger,
+  objectify,
+  winterCGRequestToRequestData,
+} from '@sentry/core';
+import type { RequestEventData, Span } from '@sentry/types';
 import { DEBUG_BUILD } from './debug-build';
 import type { RemixOptions } from './remixOptions';
 import { storeFormDataKeys } from './utils';
 import { extractData, isResponse, isRouteErrorResponse } from './vendor/response';
 import type { DataFunction, RemixRequest } from './vendor/types';
-import { normalizeRemixRequest } from './web-fetch';
 
 /**
  * Captures an exception happened in the Remix server.
@@ -41,24 +48,18 @@ export async function captureRemixServerException(
     return;
   }
 
-  // eslint-disable-next-line @typescript-eslint/no-explicit-any
-  let normalizedRequest: Record<string, unknown> = request as unknown as any;
+  let normalizedRequest: RequestEventData = {};
 
   try {
-    // eslint-disable-next-line @typescript-eslint/no-explicit-any
-    normalizedRequest = normalizeRemixRequest(request as unknown as any);
+    normalizedRequest = winterCGRequestToRequestData(request);
   } catch (e) {
     DEBUG_BUILD && logger.warn('Failed to normalize Remix request');
   }
 
   const objectifiedErr = objectify(err);
 
   captureException(isResponse(objectifiedErr) ? await extractResponseError(objectifiedErr) : objectifiedErr, scope => {
-    scope.setSDKProcessingMetadata({
-      request: {
-        ...normalizedRequest,
-      },
-    });
+    scope.setSDKProcessingMetadata({ normalizedRequest });
 
     scope.addEventProcessor(event => {
       addExceptionMechanism(event, {
diff --git a/packages/remix/src/utils/instrumentServer.ts b/packages/remix/src/utils/instrumentServer.ts
diff --git a/packages/remix/src/utils/web-fetch.ts b/packages/remix/src/utils/web-fetch.ts
diff --git a/packages/remix/test/utils/normalizeRemixRequest.test.ts b/packages/remix/test/utils/normalizeRemixRequest.test.ts
diff --git a/packages/replay-internal/package.json b/packages/replay-internal/package.json

Original file line number	Diff line number	Diff line change
`@@ -46,6 +46,7 @@`
`46`	`46`	`"extends": "../../package.json"`
`47`	`47`	`},`
`48`	`48`	`"sentryTest": {`
	`49`	`+ "optional": true,`
`49`	`50`	`"optionalVariants": [`
`50`	`51`	`{`
`51`	`52`	`"build-command": "test:build-canary",`
Original file line number	Diff line number	Diff line change
`@@ -32,7 +32,11 @@ export function fill(source: { [key: string]: any }, name: string, replacementFa`
`32`	`32`	`markFunctionWrapped(wrapped, original);`
`33`	`33`	`}`
`34`	`34`
`35`		`- source[name] = wrapped;`
	`35`	`+ try {`
	`36`	`+ source[name] = wrapped;`
	`37`	`+ } catch {`
	`38`	+ DEBUG_BUILD && logger.log(`Failed to replace method "${name}" in object`, source);
	`39`	`+ }`
`36`	`40`	`}`
`37`	`41`
`38`	`42`	`/**`