Merge pull request #18164 from mozilla/FXA-10626

feat(shared): SanitizeExceptions decorator
Because:

* In instances where the client can call methods "directly" (such as via NextJS Server Actions), we need to ensure that the only data that gets passed through to the user is what is intended. Error propagation is a common way for unexpected data to leak to the user.

This commit:

* Adds a SanitizeExceptions decorator to libs/shared/error that wraps a class, and captures any errors thrown by its methods. Unless specified, errors are replaced with a generic error message. Original error data is sent to Sentry, along with a data tag to indicate whether it was sanitized or passed through.

Closes #
FXA-10626

Author: david1alvarez

libs/payments/cart/src/lib/cart.service.spec.ts

@@ -96,6 +96,18 @@ import { NeedsInputType } from './cart.types';
 import { redirect } from 'next/navigation';
 
 jest.mock('next/navigation');
+jest.mock('@fxa/shared/error', () => ({
+  ...jest.requireActual('@fxa/shared/error'),
+  SanitizeExceptions: jest.fn(({ allowlist = [] } = {}) => {
+    return function (
+      target: any,
+      propertyKey: string,
+      descriptor: PropertyDescriptor
+    ) {
+      return descriptor;
+    };
+  }),
+}));
 
 describe('CartService', () => {
   let accountManager: AccountManager;

libs/payments/cart/src/lib/cart.service.ts

@@ -13,6 +13,9 @@ import {
   SubscriptionManager,
   InvoicePreview,
   PaymentMethodManager,
+  CouponErrorExpired,
+  CouponErrorGeneric,
+  CouponErrorLimitReached,
 } from '@fxa/payments/customer';
 import { EligibilityService } from '@fxa/payments/eligibility';
 import {
@@ -26,10 +29,9 @@ import { CartErrorReasonId, CartState } from '@fxa/shared/db/mysql/account';
 import { GeoDBManager } from '@fxa/shared/geodb';
 
 import { CartManager } from './cart.manager';
-import {
+import type {
   CheckoutCustomerData,
   GetNeedsInputResponse,
-  NeedsInputType,
   NoInputNeededResponse,
   PaymentInfo,
   ResultCart,
@@ -38,6 +40,7 @@ import {
   UpdateCart,
   WithContextCart,
 } from './cart.types';
+import { NeedsInputType } from './cart.types';
 import { handleEligibilityStatusMap } from './cart.utils';
 import { CheckoutService } from './checkout.service';
 import {
@@ -53,6 +56,7 @@ import {
 import { AccountManager } from '@fxa/shared/account/account';
 import assert from 'assert';
 import { CheckoutFailedError } from './checkout.error';
+import { SanitizeExceptions } from '@fxa/shared/error';
 
 // TODO - Add flow to handle situations where currency is not found
 const DEFAULT_CURRENCY = 'USD';
@@ -114,6 +118,7 @@ export class CartService {
    * Initialize a brand new cart
    * **Note**: This method is currently a placeholder. The arguments will likely change, and the internal implementation is far from complete.
    */
+  @SanitizeExceptions({ allowlist: [CartInvalidPromoCodeError] })
   async setupCart(args: {
     interval: SubplatInterval;
     offeringConfigId: string;
@@ -218,6 +223,7 @@ export class CartService {
   /**
    * Create a new cart with the contents of an existing cart, in the initial state.
    */
+  @SanitizeExceptions()
   async restartCart(cartId: string): Promise<ResultCart> {
     return this.wrapWithCartCatch(cartId, async () => {
       const oldCart = await this.cartManager.fetchCartById(cartId);
@@ -255,6 +261,7 @@ export class CartService {
     });
   }
 
+  @SanitizeExceptions()
   async checkoutCartWithStripe(
     cartId: string,
     version: number,
@@ -289,6 +296,7 @@ export class CartService {
     });
   }
 
+  @SanitizeExceptions()
   async checkoutCartWithPaypal(
     cartId: string,
     version: number,
@@ -323,6 +331,7 @@ export class CartService {
     });
   }
 
+  @SanitizeExceptions()
   async finalizeProcessingCart(cartId: string): Promise<void> {
     return this.wrapWithCartCatch(cartId, async () => {
       const cart = await this.cartManager.fetchCartById(cartId);
@@ -353,6 +362,7 @@ export class CartService {
    * Update a cart in the database by ID or with an existing cart reference
    * **Note**: This method is currently a placeholder. The arguments will likely change, and the internal implementation is far from complete.
    */
+  @SanitizeExceptions()
   async finalizeCartWithError(
     cartId: string,
     errorReasonId: CartErrorReasonId
@@ -372,6 +382,13 @@ export class CartService {
   /**
    * Update a cart in the database by ID or with an existing cart reference
    */
+  @SanitizeExceptions({
+    allowlist: [
+      CouponErrorExpired,
+      CouponErrorGeneric,
+      CouponErrorLimitReached,
+    ],
+  })
   async updateCart(cartId: string, version: number, cartDetails: UpdateCart) {
     return this.wrapWithCartCatch(cartId, async () => {
       const oldCart = await this.cartManager.fetchCartById(cartId);
@@ -407,6 +424,7 @@ export class CartService {
   /**
    * Fetch a cart from the database by ID
    */
+  @SanitizeExceptions()
   async getCart(cartId: string): Promise<WithContextCart> {
     const cart = await this.cartManager.fetchCartById(cartId);
 
@@ -478,6 +496,7 @@ export class CartService {
   /**
    * Fetch a success cart from the database by UID
    */
+  @SanitizeExceptions({ allowlist: [CartInvalidStateForActionError] })
   async getSuccessCart(cartId: string): Promise<SuccessCart> {
     const cart = await this.getCart(cartId);
 
@@ -511,6 +530,7 @@ export class CartService {
     }
   }
 
+  @SanitizeExceptions()
   async getNeedsInput(cartId: string): Promise<GetNeedsInputResponse> {
     const cart = await this.cartManager.fetchCartById(cartId);
 
@@ -554,6 +574,7 @@ export class CartService {
     }
   }
 
+  @SanitizeExceptions({ allowlist: [CheckoutFailedError] })
   async submitNeedsInput(cartId: string) {
     return this.wrapWithCartCatch(cartId, async () => {
       const cart = await this.cartManager.fetchCartById(cartId);

libs/payments/customer/project.json

@@ -50,14 +50,6 @@
         "jestConfig": "libs/payments/customer/jest.config.ts",
         "testPathPattern": ["^(?!.*\\.in\\.spec\\.ts$).*$"]
       }
-    },
-    "test-integration": {
-      "executor": "@nx/jest:jest",
-      "outputs": ["{workspaceRoot}/coverage/{projectRoot}"],
-      "options": {
-        "jestConfig": "libs/payments/customer/jest.config.ts",
-        "testPathPattern": ["\\.in\\.spec\\.ts$"]
-      }
     }
   }
 }

libs/payments/ui/src/lib/nestapp/nextjs-actions.service.ts

@@ -26,6 +26,7 @@ import { FinalizeProcessingCartActionArgs } from './validators/finalizeProcessin
 import { SubmitNeedsInputActionArgs } from './validators/SubmitNeedsInputActionArgs';
 import { GetNeedsInputActionArgs } from './validators/GetNeedsInputActionArgs';
 import { ValidatePostalCodeArgs } from './validators/ValidatePostalCodeArgs';
+import { SanitizeExceptions } from '@fxa/shared/error';
 
 /**
  * ANY AND ALL methods exposed via this service should be considered publicly accessible and callable with any arguments.
@@ -102,6 +103,7 @@ export class NextJSActionsService {
     await this.cartService.finalizeProcessingCart(args.cartId);
   }
 
+  @SanitizeExceptions()
   async getPayPalCheckoutToken(args: GetPayPalCheckoutTokenArgs) {
     await new Validator().validateOrReject(args);
 
@@ -132,6 +134,7 @@ export class NextJSActionsService {
     );
   }
 
+  @SanitizeExceptions()
   async fetchCMSData(args: FetchCMSDataArgs) {
     await new Validator().validateOrReject(args);
 
@@ -143,6 +146,7 @@ export class NextJSActionsService {
     return offering;
   }
 
+  @SanitizeExceptions()
   async recordEmitterEvent(args: RecordEmitterEventArgs) {
     await new Validator().validateOrReject(args);
 
@@ -183,10 +187,12 @@ export class NextJSActionsService {
     return await this.cartService.submitNeedsInput(args.cartId);
   }
 
+  @SanitizeExceptions()
   async getMetricsFlow() {
     return this.contentServerManager.getMetricsFlow();
   }
 
+  @SanitizeExceptions()
   async validatePostalCode(args: ValidatePostalCodeArgs) {
     await new Validator().validateOrReject(args);
 

libs/shared/error/src/index.ts

@@ -2,3 +2,4 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 export { BaseError, BaseMultiError, TypeError } from './lib/error';
+export { SanitizeExceptions } from './lib/sanitizeExceptionsDecorator';

libs/shared/error/src/lib/sanitizeExceptionsDecorator.spec.ts

@@ -0,0 +1,125 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+import { Test, TestingModule } from '@nestjs/testing';
+import { SanitizeExceptions } from './sanitizeExceptionsDecorator';
+import { LOGGER_PROVIDER } from '@fxa/shared/log';
+
+// Mock Sentry
+jest.mock('@sentry/nextjs', () => ({
+  withScope: jest.fn((callback) => callback({ setExtra: jest.fn() })),
+  captureException: jest.fn(),
+}));
+
+class SensitiveError extends Error {
+  public subtype: string;
+  constructor(message: string) {
+    super(message);
+    this.subtype = 'SensitiveError';
+  }
+}
+
+class AcceptableError extends Error {
+  public subtype: string;
+  constructor(message: string) {
+    super(message);
+    this.subtype = 'AcceptableError';
+  }
+}
+
+class MockService {
+  @SanitizeExceptions({ allowlist: [AcceptableError] })
+  throwSensitiveError() {
+    throw new SensitiveError('Sensitive error');
+  }
+
+  @SanitizeExceptions({ allowlist: [AcceptableError] })
+  async throwSensitiveErrorAsync() {
+    throw new SensitiveError('Sensitive error');
+  }
+
+  @SanitizeExceptions({ allowlist: [AcceptableError] })
+  throwAcceptableError() {
+    throw new AcceptableError('Acceptable error');
+  }
+
+  @SanitizeExceptions({ allowlist: [AcceptableError] })
+  async throwAcceptableErrorAsync() {
+    throw new AcceptableError('Acceptable error');
+  }
+}
+
+describe('SanitizeExceptions Decorator', () => {
+  let service: MockService;
+  let mockLogger: { error: jest.Mock; info: jest.Mock };
+
+  beforeEach(async () => {
+    mockLogger = {
+      error: jest.fn(),
+      info: jest.fn(),
+    };
+
+    const module: TestingModule = await Test.createTestingModule({
+      providers: [
+        MockService,
+        {
+          provide: LOGGER_PROVIDER,
+          useValue: mockLogger,
+        },
+      ],
+    }).compile();
+
+    service = module.get<MockService>(MockService);
+  });
+
+  afterEach(() => {
+    jest.clearAllMocks();
+  });
+
+  describe('synchronous methods', () => {
+    it('should pass acceptable errors through', () => {
+      expect(() => service.throwAcceptableError()).toThrow(AcceptableError);
+      expect(mockLogger.error).toHaveBeenCalledWith(
+        'Error caught by SanitizeExceptions decorator',
+        expect.any(AcceptableError)
+      );
+    });
+
+    it('should sanitize sensitive errors', () => {
+      expect(() => service.throwSensitiveError()).toThrow(
+        'Something went wrong'
+      );
+      expect(() => service.throwSensitiveError()).not.toThrow(SensitiveError);
+      expect(mockLogger.error).toHaveBeenCalledWith(
+        'Error caught by SanitizeExceptions decorator',
+        expect.any(SensitiveError)
+      );
+    });
+  });
+
+  describe('asynchronous methods', () => {
+    it('should pass acceptable errors through', async () => {
+      await expect(service.throwAcceptableErrorAsync()).rejects.toThrow(
+        AcceptableError
+      );
+      expect(mockLogger.error).toHaveBeenCalledWith(
+        'Error caught by SanitizeExceptions decorator',
+        expect.any(AcceptableError)
+      );
+    });
+
+    it('should sanitize sensitive errors', async () => {
+      await expect(service.throwSensitiveErrorAsync()).rejects.toThrow(
+        'Something went wrong'
+      );
+      await expect(service.throwSensitiveErrorAsync()).rejects.not.toThrow(
+        SensitiveError
+      );
+      expect(mockLogger.error).toHaveBeenCalledWith(
+        'Error caught by SanitizeExceptions decorator',
+        expect.any(SensitiveError)
+      );
+    });
+  });
+});