feat(api): replace sentry plugin with sdk (#58912)

Co-authored-by: Naomi <accounts+github@nhcarrigan.com>

api/package.json

@@ -14,8 +14,8 @@
     "@fastify/swagger-ui": "1.10.2",
     "@fastify/type-provider-typebox": "3.6.0",
     "@growthbook/growthbook": "1.3.1",
-    "@immobiliarelabs/fastify-sentry": "7.1.1",
     "@prisma/client": "5.5.2",
+    "@sentry/node": "9.1.0",
     "ajv": "8.12.0",
     "ajv-formats": "2.1.1",
     "date-fns": "2.30.0",
@@ -45,6 +45,7 @@
     "@types/validator": "13.11.2",
     "dotenv-cli": "7.3.0",
     "jest": "29.7.0",
+    "msw": "^2.7.0",
     "prisma": "5.5.2",
     "supertest": "6.3.3",
     "ts-jest": "29.1.2",

api/src/instrument.ts

@@ -0,0 +1,17 @@
+import * as Sentry from '@sentry/node';
+import type { FastifyError } from 'fastify';
+
+import { SENTRY_DSN, SENTRY_ENVIRONMENT } from './utils/env';
+
+const shouldIgnoreError = (error: FastifyError): boolean => {
+  return !!error.statusCode && error.statusCode < 500;
+};
+
+// Ensure to call this before importing any other modules!
+Sentry.init({
+  dsn: SENTRY_DSN,
+  environment: SENTRY_ENVIRONMENT,
+  maxValueLength: 8192, // the default is 250, which is too small.
+  beforeSend: (event, hint) =>
+    shouldIgnoreError(hint.originalException as FastifyError) ? null : event
+});

api/src/plugins/error-handling.test.ts

@@ -1,9 +1,24 @@
-import Fastify, { type FastifyInstance } from 'fastify';
+const SENTRY_DSN = 'https://anything@goes/123';
+
+import Fastify, { FastifyError, type FastifyInstance } from 'fastify';
 import accepts from '@fastify/accepts';
+import { http, HttpResponse } from 'msw';
+import { setupServer } from 'msw/node';
+import '../instrument';
 
 import errorHandling from './error-handling';
 import redirectWithMessage, { formatMessage } from './redirect-with-message';
 
+jest.mock('../utils/env', () => {
+  // eslint-disable-next-line @typescript-eslint/no-unsafe-return
+  return {
+    ...jest.requireActual('../utils/env'),
+    SENTRY_DSN
+  };
+});
+
+const delay = (ms: number) => new Promise(resolve => setTimeout(resolve, ms));
+
 describe('errorHandling', () => {
   let fastify: FastifyInstance;
 
@@ -14,13 +29,33 @@ describe('errorHandling', () => {
     await fastify.register(errorHandling);
 
     fastify.get('/test', async (_req, _reply) => {
-      throw new Error('a very bad thing happened');
+      const error = Error('a very bad thing happened') as FastifyError;
-      return { ok: true };
+      error.statusCode = 500;
+      throw error;
+    });
+    fastify.get('/test-bad-request', async (_req, _reply) => {
+      const error = Error('a very bad thing happened') as FastifyError;
+      error.statusCode = 400;
+      throw error;
+    });
+    fastify.get('/test-csrf-token', async (_req, _reply) => {
+      const error = Error() as FastifyError;
+      error.code = 'FST_CSRF_INVALID_TOKEN';
+      error.statusCode = 403;
+      throw error;
+    });
+
+    fastify.get('/test-csrf-secret', async (_req, _reply) => {
+      const error = Error() as FastifyError;
+      error.code = 'FST_CSRF_MISSING_SECRET';
+      error.statusCode = 403;
+      throw error;
     });
   });
 
   afterEach(async () => {
     await fastify.close();
+    jest.clearAllMocks();
   });
 
   it('should redirect to the referer if the request does not Accept json', async () => {
@@ -33,6 +68,19 @@ describe('errorHandling', () => {
       }
     });
 
+    expect(res.statusCode).toEqual(302);
+  });
+
+  it('should add a generic flash message if it is a server error (i.e. 500+)', async () => {
+    const res = await fastify.inject({
+      method: 'GET',
+      url: '/test',
+      headers: {
+        referer: 'https://www.freecodecamp.org/anything',
+        accept: 'text/plain'
+      }
+    });
+
     expect(res.headers['location']).toEqual(
       'https://www.freecodecamp.org/anything?' +
         formatMessage({
@@ -40,7 +88,6 @@ describe('errorHandling', () => {
           content: 'flash.generic-error'
         })
     );
-    expect(res.statusCode).toEqual(302);
   });
 
   it('should return a json response if the request does Accept json', async () => {
@@ -53,11 +100,11 @@ describe('errorHandling', () => {
       }
     });
 
+    expect(res.statusCode).toEqual(500);
     expect(res.json()).toEqual({
       message: 'flash.generic-error',
       type: 'danger'
     });
-    expect(res.statusCode).toEqual(500);
   });
 
   it('should redirect if the request prefers text/html to json', async () => {
@@ -71,13 +118,163 @@ describe('errorHandling', () => {
       }
     });
 
-    expect(res.headers['location']).toEqual(
-      'https://www.freecodecamp.org/anything?' +
-        formatMessage({
-          type: 'danger',
-          content: 'flash.generic-error'
-        })
-    );
     expect(res.statusCode).toEqual(302);
   });
+
+  it('should respect the error status code', async () => {
+    const res = await fastify.inject({
+      method: 'GET',
+      url: '/test-bad-request'
+    });
+
+    expect(res.statusCode).toEqual(400);
+  });
+
+  it('should return the error message if the status is not 500 ', async () => {
+    const res = await fastify.inject({
+      method: 'GET',
+      url: '/test-bad-request'
+    });
+
+    expect(res.json()).toEqual({
+      message: 'a very bad thing happened',
+      type: 'danger'
+    });
+  });
+
+  it('should convert CSRF errors to a generic error message', async () => {
+    const resToken = await fastify.inject({
+      method: 'GET',
+      url: '/test-csrf-token'
+    });
+    const resSecret = await fastify.inject({
+      method: 'GET',
+      url: '/test-csrf-secret'
+    });
+
+    expect(resToken.json()).toEqual({
+      message: 'flash.generic-error',
+      type: 'danger'
+    });
+    expect(resSecret.json()).toEqual({
+      message: 'flash.generic-error',
+      type: 'danger'
+    });
+  });
+
+  it('should call fastify.log.error when an unhandled error occurs', async () => {
+    const logSpy = jest.spyOn(fastify.log, 'error');
+
+    await fastify.inject({
+      method: 'GET',
+      url: '/test'
+    });
+
+    expect(logSpy).toHaveBeenCalledWith(Error('a very bad thing happened'));
+  });
+
+  it('should call fastify.log.warn when a bad request error occurs', async () => {
+    const logSpy = jest.spyOn(fastify.log, 'warn');
+
+    await fastify.inject({
+      method: 'GET',
+      url: '/test-bad-request'
+    });
+
+    expect(logSpy).toHaveBeenCalledWith(Error('a very bad thing happened'));
+  });
+
+  it('should NOT log when a CSRF error is thrown', async () => {
+    const errorLogSpy = jest.spyOn(fastify.log, 'error');
+    const warnLogSpy = jest.spyOn(fastify.log, 'warn');
+
+    await fastify.inject({
+      method: 'GET',
+      url: '/test-csrf-token'
+    });
+
+    expect(errorLogSpy).not.toHaveBeenCalled();
+    expect(warnLogSpy).not.toHaveBeenCalled();
+
+    await fastify.inject({
+      method: 'GET',
+      url: '/test-csrf-secret'
+    });
+
+    expect(errorLogSpy).not.toHaveBeenCalled();
+    expect(warnLogSpy).not.toHaveBeenCalled();
+  });
+
+  describe('Sentry integration', () => {
+    let mockServer: ReturnType<typeof setupServer>;
+
+    beforeAll(() => {
+      // The assumption is that Sentry is the only library making requests. Also, we
+      // only want to know if a request was made, not what it was.
+      const sentryHandler = http.post('*', () =>
+        HttpResponse.json({ success: true })
+      );
+      mockServer = setupServer(sentryHandler);
+      mockServer.listen();
+    });
+
+    afterEach(() => {
+      mockServer.resetHandlers();
+    });
+
+    afterAll(() => {
+      mockServer.close();
+    });
+
+    const createRequestListener = () =>
+      new Promise(resolve => {
+        mockServer.events.on('request:start', () => {
+          resolve(true);
+        });
+      });
+
+    it('should capture the error with Sentry', async () => {
+      const receivedRequest = createRequestListener();
+
+      await fastify.inject({
+        method: 'GET',
+        url: '/test'
+      });
+
+      expect(await Promise.race([receivedRequest, delay(1000)])).toBe(true);
+    });
+
+    it('should NOT capture CSRF token errors with Sentry', async () => {
+      const receivedRequest = createRequestListener();
+
+      await fastify.inject({
+        method: 'GET',
+        url: '/test-csrf-token'
+      });
+
+      expect(await Promise.race([receivedRequest, delay(200)])).toBeUndefined();
+    });
+
+    it('should NOT capture CSRF secret errors with Sentry', async () => {
+      const receivedRequest = createRequestListener();
+
+      await fastify.inject({
+        method: 'GET',
+        url: '/test-csrf-secret'
+      });
+
+      expect(await Promise.race([receivedRequest, delay(200)])).toBeUndefined();
+    });
+
+    it('should NOT capture bad requests with Sentry', async () => {
+      const receivedRequest = createRequestListener();
+
+      await fastify.inject({
+        method: 'GET',
+        url: '/test-bad-request'
+      });
+
+      expect(await Promise.race([receivedRequest, delay(200)])).toBeUndefined();
+    });
+  });
 });

api/src/plugins/error-handling.ts

@@ -1,10 +1,15 @@
 import type { FastifyPluginCallback } from 'fastify';
-import fastifySentry from '@immobiliarelabs/fastify-sentry';
+import * as Sentry from '@sentry/node';
 import fp from 'fastify-plugin';
 
-import { SENTRY_DSN, SENTRY_ENVIRONMENT } from '../utils/env';
 import { getRedirectParams } from '../utils/redirection';
 
+declare module 'fastify' {
+  interface FastifyInstance {
+    Sentry: typeof Sentry;
+  }
+}
+
 /**
  * Plugin to handle errors and send them to Sentry.
  *
@@ -13,38 +18,49 @@ import { getRedirectParams } from '../utils/redirection';
  * @param done Callback to signal that the logic has completed.
  */
 const errorHandling: FastifyPluginCallback = (fastify, _options, done) => {
-  void fastify.register(fastifySentry, {
+  Sentry.setupFastifyErrorHandler(fastify);
-    dsn: SENTRY_DSN,
-    environment: SENTRY_ENVIRONMENT,
-    maxValueLength: 8192, // the default is 250, which is too small.
-    // No need to initialize if DSN is not provided (e.g. in development and
-    // test environments)
-    skipInit: !SENTRY_DSN,
-    errorResponse: (error, request, reply) => {
-      const accepts = request.accepts().type(['json', 'html']);
-      const isCSRFError =
-        error.code === 'FST_CSRF_INVALID_TOKEN' ||
-        error.code === 'FST_CSRF_MISSING_SECRET';
 
-      const { returnTo } = getRedirectParams(request);
+  fastify.decorate('Sentry', Sentry);
 
-      const message =
+  fastify.setErrorHandler((error, request, reply) => {
-        reply.statusCode === 500 || isCSRFError
+    const logger = fastify.log.child({ req: request });
-          ? 'flash.generic-error'
+    const accepts = request.accepts().type(['json', 'html']);
-          : error.message;
+    const { returnTo } = getRedirectParams(request);
-      if (accepts === 'json') {
+
-        void reply.send({
+    if (!reply.statusCode || reply.statusCode === 200) {
-          message,
+      const statusCode =
-          type: 'danger'
+        error.statusCode && error.statusCode >= 400 ? error.statusCode : 500;
-        });
+      reply.code(statusCode);
+    }
+
+    const isCSRFError =
+      error.code === 'FST_CSRF_INVALID_TOKEN' ||
+      error.code === 'FST_CSRF_MISSING_SECRET';
+
+    if (!isCSRFError) {
+      if (reply.statusCode >= 500) {
+        logger.error(error);
       } else {
-        void reply.status(302);
+        logger.warn(error);
-        void reply.redirectWithMessage(returnTo, {
-          type: 'danger',
-          content: message
-        });
       }
     }
+
+    const message =
+      reply.statusCode === 500 || isCSRFError
+        ? 'flash.generic-error'
+        : error.message;
+    if (accepts === 'json') {
+      void reply.send({
+        message,
+        type: 'danger'
+      });
+    } else {
+      void reply.status(302);
+      void reply.redirectWithMessage(returnTo, {
+        type: 'danger',
+        content: message
+      });
+    }
   });
 
   done();

api/src/server.ts

@@ -4,6 +4,8 @@
 // eslint-disable-next-line @typescript-eslint/triple-slash-reference
 /// <reference path="./reset.d.ts" />
 
+import './instrument';
+
 import { randomBytes } from 'crypto';
 
 import { FastifyRequest } from 'fastify';