From 4477dcf9c8368515f1e24b0e692ebd3e88d3fa4a Mon Sep 17 00:00:00 2001 From: Thomas Lathuiliere <40292402+balzdur@users.noreply.github.com> Date: Wed, 6 Nov 2024 14:00:12 +0100 Subject: [PATCH] refactor(Sentry): improve error monitoring (#584) --- .../src/components/Auth/AuthError.tsx | 1 + .../src/components/Auth/SignInWithGoogle.tsx | 5 +++- .../components/Auth/SignInWithMicrosoft.tsx | 5 +++- packages/app-builder/src/entry.server.tsx | 8 ++---- packages/app-builder/src/locales/en/auth.json | 1 + packages/app-builder/src/locales/fr/auth.json | 1 + .../app-builder/src/models/auth-errors.ts | 7 ++++- .../routes/ressources+/data+/createField.tsx | 4 +-- .../routes/ressources+/data+/editField.tsx | 4 +-- .../src/services/auth/auth.client.ts | 4 +++ .../src/services/auth/auth.server.ts | 18 +++++++++---- .../app-builder/src/services/monitoring.ts | 26 +++++++++++++++++++ 12 files changed, 66 insertions(+), 18 deletions(-) create mode 100644 packages/app-builder/src/services/monitoring.ts diff --git a/packages/app-builder/src/components/Auth/AuthError.tsx b/packages/app-builder/src/components/Auth/AuthError.tsx index 7b670a61..f838b1d9 100644 --- a/packages/app-builder/src/components/Auth/AuthError.tsx +++ b/packages/app-builder/src/components/Auth/AuthError.tsx @@ -7,6 +7,7 @@ import { authI18n } from './auth-i18n'; const errorLabels: Record> = { NoAccount: 'auth:errors.no_account', + CSRFError: 'auth:errors.csrf_error', Unknown: 'common:errors.unknown', }; diff --git a/packages/app-builder/src/components/Auth/SignInWithGoogle.tsx b/packages/app-builder/src/components/Auth/SignInWithGoogle.tsx index c512e657..b564797e 100644 --- a/packages/app-builder/src/components/Auth/SignInWithGoogle.tsx +++ b/packages/app-builder/src/components/Auth/SignInWithGoogle.tsx @@ -1,5 +1,6 @@ import { AccountExistsWithDifferentCredential, + InvalidLoginCredentials, NetworkRequestFailed, PopupBlockedByClient, useGoogleSignIn, @@ -51,7 +52,7 @@ function ClientSignInWithGoogle({ signIn: (authPayload: AuthPayload) => void; loading?: boolean; }) { - const { t } = useTranslation(['common']); + const { t } = useTranslation(['common', 'auth']); const googleSignIn = useGoogleSignIn( clientServices.authenticationClientService, ); @@ -72,6 +73,8 @@ function ClientSignInWithGoogle({ toast.error(); } else if (error instanceof NetworkRequestFailed) { toast.error(t('common:errors.firebase_network_error')); + } else if (error instanceof InvalidLoginCredentials) { + toast.error(t('auth:sign_in.errors.invalid_login_credentials')); } else { Sentry.captureException(error); toast.error(t('common:errors.unknown')); diff --git a/packages/app-builder/src/components/Auth/SignInWithMicrosoft.tsx b/packages/app-builder/src/components/Auth/SignInWithMicrosoft.tsx index 29ac7662..964e0a1b 100644 --- a/packages/app-builder/src/components/Auth/SignInWithMicrosoft.tsx +++ b/packages/app-builder/src/components/Auth/SignInWithMicrosoft.tsx @@ -1,5 +1,6 @@ import { AccountExistsWithDifferentCredential, + InvalidLoginCredentials, NetworkRequestFailed, PopupBlockedByClient, useMicrosoftSignIn, @@ -53,7 +54,7 @@ function ClientSignInWithMicrosoft({ signIn: (authPayload: AuthPayload) => void; loading?: boolean; }) { - const { t } = useTranslation(['common']); + const { t } = useTranslation(['common', 'auth']); const microsoftSignIn = useMicrosoftSignIn( clientServices.authenticationClientService, ); @@ -74,6 +75,8 @@ function ClientSignInWithMicrosoft({ toast.error(); } else if (error instanceof NetworkRequestFailed) { toast.error(t('common:errors.firebase_network_error')); + } else if (error instanceof InvalidLoginCredentials) { + toast.error(t('auth:sign_in.errors.invalid_login_credentials')); } else { Sentry.captureException(error); toast.error(t('common:errors.unknown')); diff --git a/packages/app-builder/src/entry.server.tsx b/packages/app-builder/src/entry.server.tsx index f1b2a446..1c78af59 100644 --- a/packages/app-builder/src/entry.server.tsx +++ b/packages/app-builder/src/entry.server.tsx @@ -11,6 +11,7 @@ import { I18nextProvider } from 'react-i18next'; import { PassThrough } from 'stream'; import { serverServices } from './services/init.server'; +import { captureUnexpectedRemixError } from './services/monitoring'; import { getServerEnv } from './utils/environment'; const ABORT_DELAY = 5000; @@ -185,10 +186,5 @@ export const handleError: HandleErrorFunction = (error, { request }) => { if (request.signal.aborted) { return; } - if (error instanceof Error) { - void Sentry.captureRemixServerException(error, 'remix.server', request); - } else { - // Optionally capture non-Error objects - Sentry.captureException(error); - } + captureUnexpectedRemixError(error, 'remix.server', request); }; diff --git a/packages/app-builder/src/locales/en/auth.json b/packages/app-builder/src/locales/en/auth.json index 660ad388..3f42f0bb 100644 --- a/packages/app-builder/src/locales/en/auth.json +++ b/packages/app-builder/src/locales/en/auth.json @@ -4,6 +4,7 @@ "sign_in.google": "Sign in with Google", "sign_in.microsoft": "Sign in with Microsoft", "errors.no_account": "No Marble account found for this address.", + "errors.csrf_error": "A required security token was not found or was invalid.\n Refresh the page and try again.", "sign_in.email": "Email", "sign_in.password": "Password", "sign_in": "Sign in", diff --git a/packages/app-builder/src/locales/fr/auth.json b/packages/app-builder/src/locales/fr/auth.json index c5a01538..a1c2803e 100644 --- a/packages/app-builder/src/locales/fr/auth.json +++ b/packages/app-builder/src/locales/fr/auth.json @@ -5,6 +5,7 @@ "email-verification.resend": "renvoyer l'e-mail de vérification", "email-verification.wrong_place": "Mauvais endroit ? \nrevenez à {{signIn}}.", "errors.no_account": "Aucun compte Marble trouvé pour cette adresse.", + "errors.csrf_error": "Un jeton de sécurité requis n'a pas été trouvé ou était invalide. \nActualisez la page et réessayez.", "great_rules_right_tools": "Les bonnes règles sont construites avec les bons outils.", "marble_description": "Marble est un moteur de règles en temps réel pour la surveillance de la fraude et de la conformité, conçu pour les sociétés de technologie financière et les institutions financières.", "reset-password.email_sent": "Un email vous a été envoyé avec un lien pour réinitialiser votre mot de passe.", diff --git a/packages/app-builder/src/models/auth-errors.ts b/packages/app-builder/src/models/auth-errors.ts index 966cccda..0253699a 100644 --- a/packages/app-builder/src/models/auth-errors.ts +++ b/packages/app-builder/src/models/auth-errors.ts @@ -1,8 +1,13 @@ +import { CSRFError } from 'remix-utils/csrf/server'; + import { isMarbleError, isUnauthorizedHttpError } from './http-errors'; -export type AuthErrors = 'NoAccount' | 'Unknown'; +export type AuthErrors = 'NoAccount' | 'CSRFError' | 'Unknown'; export function adaptAuthErrors(error: unknown): AuthErrors { + if (error instanceof CSRFError) { + return 'CSRFError'; + } if (isUnauthorizedHttpError(error) && isMarbleError(error)) { const errorCode = error.data.error_code; if (errorCode === 'unknown_user') { diff --git a/packages/app-builder/src/routes/ressources+/data+/createField.tsx b/packages/app-builder/src/routes/ressources+/data+/createField.tsx index 7670712f..f3dad53d 100644 --- a/packages/app-builder/src/routes/ressources+/data+/createField.tsx +++ b/packages/app-builder/src/routes/ressources+/data+/createField.tsx @@ -12,11 +12,11 @@ import { UniqueDataTypes, } from '@app-builder/models'; import { serverServices } from '@app-builder/services/init.server'; +import { captureUnexpectedRemixError } from '@app-builder/services/monitoring'; import { getRoute } from '@app-builder/utils/routes'; import { zodResolver } from '@hookform/resolvers/zod'; import { type ActionFunctionArgs, json } from '@remix-run/node'; import { useFetcher } from '@remix-run/react'; -import * as Sentry from '@sentry/remix'; import { type Namespace } from 'i18next'; import { useEffect, useState } from 'react'; import { Form, FormProvider, useForm, useWatch } from 'react-hook-form'; @@ -119,7 +119,7 @@ export async function action({ request }: ActionFunctionArgs) { type: 'error', messageKey: 'common:errors.unknown', }); - Sentry.captureException(error); + captureUnexpectedRemixError(error, 'createField@action', request); return json( { success: false as const, diff --git a/packages/app-builder/src/routes/ressources+/data+/editField.tsx b/packages/app-builder/src/routes/ressources+/data+/editField.tsx index 4ddd382c..a50fca8e 100644 --- a/packages/app-builder/src/routes/ressources+/data+/editField.tsx +++ b/packages/app-builder/src/routes/ressources+/data+/editField.tsx @@ -13,11 +13,11 @@ import { UniqueDataTypes, } from '@app-builder/models'; import { serverServices } from '@app-builder/services/init.server'; +import { captureUnexpectedRemixError } from '@app-builder/services/monitoring'; import { getRoute } from '@app-builder/utils/routes'; import { zodResolver } from '@hookform/resolvers/zod'; import { type ActionFunctionArgs, json } from '@remix-run/node'; import { useFetcher } from '@remix-run/react'; -import * as Sentry from '@sentry/remix'; import { type Namespace } from 'i18next'; import { useEffect, useMemo, useState } from 'react'; import { Form, FormProvider, useForm, useWatch } from 'react-hook-form'; @@ -73,7 +73,7 @@ export async function action({ request }: ActionFunctionArgs) { type: 'error', messageKey: 'common:errors.unknown', }); - Sentry.captureException(error); + captureUnexpectedRemixError(error, 'editField@action', request); return json( { success: false as const, diff --git a/packages/app-builder/src/services/auth/auth.client.ts b/packages/app-builder/src/services/auth/auth.client.ts index ab57fa4c..933a5a05 100644 --- a/packages/app-builder/src/services/auth/auth.client.ts +++ b/packages/app-builder/src/services/auth/auth.client.ts @@ -43,6 +43,8 @@ export function useGoogleSignIn({ throw new AccountExistsWithDifferentCredential(); case AuthErrorCodes.POPUP_BLOCKED: throw new PopupBlockedByClient(); + case AuthErrorCodes.INVALID_IDP_RESPONSE: + throw new InvalidLoginCredentials(); case AuthErrorCodes.NETWORK_REQUEST_FAILED: throw new NetworkRequestFailed(); } @@ -77,6 +79,8 @@ export function useMicrosoftSignIn({ throw new AccountExistsWithDifferentCredential(); case AuthErrorCodes.POPUP_BLOCKED: throw new PopupBlockedByClient(); + case AuthErrorCodes.INVALID_IDP_RESPONSE: + throw new InvalidLoginCredentials(); case AuthErrorCodes.NETWORK_REQUEST_FAILED: throw new NetworkRequestFailed(); } diff --git a/packages/app-builder/src/services/auth/auth.server.ts b/packages/app-builder/src/services/auth/auth.server.ts index b6b74ea2..8611ffad 100644 --- a/packages/app-builder/src/services/auth/auth.server.ts +++ b/packages/app-builder/src/services/auth/auth.server.ts @@ -32,12 +32,12 @@ import { type WebhookRepository } from '@app-builder/repositories/WebhookReposit import { getServerEnv } from '@app-builder/utils/environment'; import { parseForm } from '@app-builder/utils/input-validation'; import { json, redirect } from '@remix-run/node'; -import * as Sentry from '@sentry/remix'; import { marblecoreApi } from 'marble-api'; -import { type CSRF } from 'remix-utils/csrf/server'; +import { type CSRF, CSRFError } from 'remix-utils/csrf/server'; import * as z from 'zod'; import { getRoute } from '../../utils/routes'; +import { captureUnexpectedRemixError } from '../monitoring'; import { type SessionService } from './session.server'; interface AuthenticatedInfo { @@ -156,6 +156,10 @@ interface MakeAuthenticationServerServiceArgs { csrfService: CSRF; } +function expectedErrors(error: unknown) { + return error instanceof CSRFError || error instanceof z.ZodError; +} + export function makeAuthenticationServerService({ getMarbleCoreAPIClientWithAuth, getTransfercheckAPIClientWithAuth, @@ -221,10 +225,12 @@ export function makeAuthenticationServerService({ authSession.set('user', user); redirectUrl = options.successRedirect; } catch (error) { - Sentry.captureException(error); authSession.flash('authError', { message: adaptAuthErrors(error) }); - redirectUrl = options.failureRedirect; + + if (!expectedErrors(error)) { + captureUnexpectedRemixError(error, 'auth.server@authenticate', request); + } } throw redirect(redirectUrl, { @@ -284,7 +290,9 @@ export function makeAuthenticationServerService({ }, ); } catch (error) { - Sentry.captureException(error); + if (!expectedErrors(error)) { + captureUnexpectedRemixError(error, 'auth.server@refresh', request); + } throw redirect(options.failureRedirect); } } diff --git a/packages/app-builder/src/services/monitoring.ts b/packages/app-builder/src/services/monitoring.ts new file mode 100644 index 00000000..160f0ea9 --- /dev/null +++ b/packages/app-builder/src/services/monitoring.ts @@ -0,0 +1,26 @@ +import { + isForbiddenHttpError, + isNotFoundHttpError, + isUnauthorizedHttpError, +} from '@app-builder/models'; +import * as Sentry from '@sentry/remix'; + +export function captureUnexpectedRemixError( + error: unknown, + name: string, + request: Request, +) { + if ( + isUnauthorizedHttpError(error) || + isForbiddenHttpError(error) || + isNotFoundHttpError(error) + ) { + return; + } + if (error instanceof Error) { + void Sentry.captureRemixServerException(error, name, request); + } else { + // Optionally capture non-Error objects + Sentry.captureException(error); + } +}