fix(nestjs): Check arguments before instrumenting with @Injectable (#13544)

nicohrubec · web-flow · commit 00a45a4cc93a · 2024-09-02T16:02:48.000+02:00
Before this fix, calling a `use` method on a service that does not
implement the `NestMiddleware` interface resulted in an error. This is
because we try to proxy the third argument to the function, which in
middleware is an express `NextFunction`, but in other classes can be
anything, potentially even undefined.

This fix introduces further guards to early-return in non-middleware targets.

Added a test to verify that services with `use` work fine now. Also
added additional tests to verify that this behavior does not occur for
`canActivate` (guard), `intercept` (interceptor) and `transform` (pipe)
methods.
diff --git a/dev-packages/e2e-tests/test-applications/nestjs-basic/src/app.controller.ts b/dev-packages/e2e-tests/test-applications/nestjs-basic/src/app.controller.ts
@@ -96,4 +96,24 @@ export class AppController {
   async exampleExceptionLocalFilter() {
     throw new ExampleExceptionLocalFilter();
   }
+
+  @Get('test-service-use')
+  testServiceWithUseMethod() {
+    return this.appService.use();
+  }
+
+  @Get('test-service-transform')
+  testServiceWithTransform() {
+    return this.appService.transform();
+  }
+
+  @Get('test-service-intercept')
+  testServiceWithIntercept() {
+    return this.appService.intercept();
+  }
+
+  @Get('test-service-canActivate')
+  testServiceWithCanActivate() {
+    return this.appService.canActivate();
+  }
 }
diff --git a/dev-packages/e2e-tests/test-applications/nestjs-basic/src/app.service.ts b/dev-packages/e2e-tests/test-applications/nestjs-basic/src/app.service.ts
@@ -78,4 +78,20 @@ export class AppService {
   async killTestCron() {
     this.schedulerRegistry.deleteCronJob('test-cron-job');
   }
+
+  use() {
+    console.log('Test use!');
+  }
+
+  transform() {
+    console.log('Test transform!');
+  }
+
+  intercept() {
+    console.log('Test intercept!');
+  }
+
+  canActivate() {
+    console.log('Test canActivate!');
+  }
 }
diff --git a/dev-packages/e2e-tests/test-applications/nestjs-basic/tests/transactions.test.ts b/dev-packages/e2e-tests/test-applications/nestjs-basic/tests/transactions.test.ts
@@ -707,3 +707,23 @@ test('API route transaction includes exactly one nest async interceptor span aft
   // 'Interceptor - After Route' is NOT the parent of 'test-controller-span'
   expect(testControllerSpan.parent_span_id).not.toBe(exampleInterceptorSpanAfterRouteId);
 });
+
+test('Calling use method on service with Injectable decorator returns 200', async ({ baseURL }) => {
+  const response = await fetch(`${baseURL}/test-service-use`);
+  expect(response.status).toBe(200);
+});
+
+test('Calling transform method on service with Injectable decorator returns 200', async ({ baseURL }) => {
+  const response = await fetch(`${baseURL}/test-service-transform`);
+  expect(response.status).toBe(200);
+});
+
+test('Calling intercept method on service with Injectable decorator returns 200', async ({ baseURL }) => {
+  const response = await fetch(`${baseURL}/test-service-intercept`);
+  expect(response.status).toBe(200);
+});
+
+test('Calling canActivate method on service with Injectable decorator returns 200', async ({ baseURL }) => {
+  const response = await fetch(`${baseURL}/test-service-canActivate`);
+  expect(response.status).toBe(200);
+});
diff --git a/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/app.controller.ts b/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/app.controller.ts
@@ -82,4 +82,24 @@ export class AppController {
   async flush() {
     await flush();
   }
+
+  @Get('test-service-use')
+  testServiceWithUseMethod() {
+    return this.appService.use();
+  }
+
+  @Get('test-service-transform')
+  testServiceWithTransform() {
+    return this.appService.transform();
+  }
+
+  @Get('test-service-intercept')
+  testServiceWithIntercept() {
+    return this.appService.intercept();
+  }
+
+  @Get('test-service-canActivate')
+  testServiceWithCanActivate() {
+    return this.appService.canActivate();
+  }
 }
diff --git a/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/app.service.ts b/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/app.service.ts
@@ -78,4 +78,20 @@ export class AppService {
   async killTestCron() {
     this.schedulerRegistry.deleteCronJob('test-cron-job');
   }
+
+  use() {
+    console.log('Test use!');
+  }
+
+  transform() {
+    console.log('Test transform!');
+  }
+
+  intercept() {
+    console.log('Test intercept!');
+  }
+
+  canActivate() {
+    console.log('Test canActivate!');
+  }
 }
diff --git a/dev-packages/e2e-tests/test-applications/node-nestjs-basic/tests/transactions.test.ts b/dev-packages/e2e-tests/test-applications/node-nestjs-basic/tests/transactions.test.ts
@@ -705,3 +705,23 @@ test('API route transaction includes exactly one nest async interceptor span aft
   // 'Interceptor - After Route' is NOT the parent of 'test-controller-span'
   expect(testControllerSpan.parent_span_id).not.toBe(exampleInterceptorSpanAfterRouteId);
 });
+
+test('Calling use method on service with Injectable decorator returns 200', async ({ baseURL }) => {
+  const response = await fetch(`${baseURL}/test-service-use`);
+  expect(response.status).toBe(200);
+});
+
+test('Calling transform method on service with Injectable decorator returns 200', async ({ baseURL }) => {
+  const response = await fetch(`${baseURL}/test-service-transform`);
+  expect(response.status).toBe(200);
+});
+
+test('Calling intercept method on service with Injectable decorator returns 200', async ({ baseURL }) => {
+  const response = await fetch(`${baseURL}/test-service-intercept`);
+  expect(response.status).toBe(200);
+});
+
+test('Calling canActivate method on service with Injectable decorator returns 200', async ({ baseURL }) => {
+  const response = await fetch(`${baseURL}/test-service-canActivate`);
+  expect(response.status).toBe(200);
+});
diff --git a/packages/node/src/integrations/tracing/nest/helpers.ts b/packages/node/src/integrations/tracing/nest/helpers.ts
@@ -1,7 +1,7 @@
 import { SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, withActiveSpan } from '@sentry/core';
 import type { Span } from '@sentry/types';
 import { addNonEnumerableProperty } from '@sentry/utils';
-import type { CatchTarget, InjectableTarget, Observable, Subscription } from './types';
+import type { CatchTarget, InjectableTarget, NextFunction, Observable, Subscription } from './types';
 
 const sentryPatched = 'sentryPatched';
 
@@ -53,3 +53,22 @@ export function instrumentObservable(observable: Observable<unknown>, activeSpan
     });
   }
 }
+
+/**
+ * Proxies the next() call in a nestjs middleware to end the span when it is called.
+ */
+export function getNextProxy(next: NextFunction, span: Span, prevSpan: undefined | Span): NextFunction {
+  return new Proxy(next, {
+    apply: (originalNext, thisArgNext, argsNext) => {
+      span.end();
+
+      if (prevSpan) {
+        return withActiveSpan(prevSpan, () => {
+          return Reflect.apply(originalNext, thisArgNext, argsNext);
+        });
+      } else {
+        return Reflect.apply(originalNext, thisArgNext, argsNext);
+      }
+    },
+  });
+}
diff --git a/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts b/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts
@@ -8,7 +8,7 @@ import {
 import { getActiveSpan, startInactiveSpan, startSpan, startSpanManual, withActiveSpan } from '@sentry/core';
 import type { Span } from '@sentry/types';
 import { SDK_VERSION, addNonEnumerableProperty, isThenable } from '@sentry/utils';
-import { getMiddlewareSpanOptions, instrumentObservable, isPatched } from './helpers';
+import { getMiddlewareSpanOptions, getNextProxy, instrumentObservable, isPatched } from './helpers';
 import type { CallHandler, CatchTarget, InjectableTarget, MinimalNestJsExecutionContext, Observable } from './types';
 
 const supportedVersions = ['>=8.0.0 <11'];
@@ -101,23 +101,19 @@ export class SentryNestInstrumentation extends InstrumentationBase {
             target.prototype.use = new Proxy(target.prototype.use, {
               apply: (originalUse, thisArgUse, argsUse) => {
                 const [req, res, next, ...args] = argsUse;
-                const prevSpan = getActiveSpan();
 
-                return startSpanManual(getMiddlewareSpanOptions(target), (span: Span) => {
-                  const nextProxy = new Proxy(next, {
-                    apply: (originalNext, thisArgNext, argsNext) => {
-                      span.end();
+                // Check that we can reasonably assume that the target is a middleware.
+                // Without these guards, instrumentation will fail if a function named 'use' on a service, which is
+                // decorated with @Injectable, is called.
+                if (!req || !res || !next || typeof next !== 'function') {
+                  return originalUse.apply(thisArgUse, argsUse);
+                }
 
-                      if (prevSpan) {
-                        return withActiveSpan(prevSpan, () => {
-                          return Reflect.apply(originalNext, thisArgNext, argsNext);
-                        });
-                      } else {
-                        return Reflect.apply(originalNext, thisArgNext, argsNext);
-                      }
-                    },
-                  });
+                const prevSpan = getActiveSpan();
 
+                return startSpanManual(getMiddlewareSpanOptions(target), (span: Span) => {
+                  // proxy next to end span on call
+                  const nextProxy = getNextProxy(next, span, prevSpan);
                   return originalUse.apply(thisArgUse, [req, res, nextProxy, args]);
                 });
               },
@@ -133,6 +129,12 @@ export class SentryNestInstrumentation extends InstrumentationBase {
 
             target.prototype.canActivate = new Proxy(target.prototype.canActivate, {
               apply: (originalCanActivate, thisArgCanActivate, argsCanActivate) => {
+                const context: MinimalNestJsExecutionContext = argsCanActivate[0];
+
+                if (!context) {
+                  return originalCanActivate.apply(thisArgCanActivate, argsCanActivate);
+                }
+
                 return startSpan(getMiddlewareSpanOptions(target), () => {
                   return originalCanActivate.apply(thisArgCanActivate, argsCanActivate);
                 });
@@ -148,6 +150,13 @@ export class SentryNestInstrumentation extends InstrumentationBase {
 
             target.prototype.transform = new Proxy(target.prototype.transform, {
               apply: (originalTransform, thisArgTransform, argsTransform) => {
+                const value = argsTransform[0];
+                const metadata = argsTransform[1];
+
+                if (!value || !metadata) {
+                  return originalTransform.apply(thisArgTransform, argsTransform);
+                }
+
                 return startSpan(getMiddlewareSpanOptions(target), () => {
                   return originalTransform.apply(thisArgTransform, argsTransform);
                 });
@@ -169,6 +178,11 @@ export class SentryNestInstrumentation extends InstrumentationBase {
                 const parentSpan = getActiveSpan();
                 let afterSpan: Span;
 
+                // Check that we can reasonably assume that the target is an interceptor.
+                if (!context || !next || typeof next.handle !== 'function') {
+                  return originalIntercept.apply(thisArgIntercept, argsIntercept);
+                }
+
                 return startSpanManual(getMiddlewareSpanOptions(target), (beforeSpan: Span) => {
                   // eslint-disable-next-line @typescript-eslint/unbound-method
                   next.handle = new Proxy(next.handle, {
@@ -263,6 +277,13 @@ export class SentryNestInstrumentation extends InstrumentationBase {
 
             target.prototype.catch = new Proxy(target.prototype.catch, {
               apply: (originalCatch, thisArgCatch, argsCatch) => {
+                const exception = argsCatch[0];
+                const host = argsCatch[1];
+
+                if (!exception || !host) {
+                  return originalCatch.apply(thisArgCatch, argsCatch);
+                }
+
                 return startSpan(getMiddlewareSpanOptions(target), () => {
                   return originalCatch.apply(thisArgCatch, argsCatch);
                 });
diff --git a/packages/node/src/integrations/tracing/nest/types.ts b/packages/node/src/integrations/tracing/nest/types.ts
@@ -73,3 +73,8 @@ export interface CatchTarget {
     catch?: (...args: any[]) => any;
   };
 }
+
+/**
+ * Represents an express NextFunction.
+ */
+export type NextFunction = (err?: any) => void;

Original file line number	Diff line number	Diff line change
`@@ -73,3 +73,8 @@ export interface CatchTarget {`
`73`	`73`	`catch?: (...args: any[]) => any;`
`74`	`74`	`};`
`75`	`75`	`}`
	`76`	`+`
	`77`	`+/**`
	`78`	`+ * Represents an express NextFunction.`
	`79`	`+ */`
	`80`	`+export type NextFunction = (err?: any) => void;`