Skip to content
40 changes: 40 additions & 0 deletions src/app/core/interceptors/httpInterceptor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import { SecureStorageService } from '../services/secure-storage.service';
import { StorageService } from '../services/storage.service';
import { TokenService } from '../services/token.service';
import { UserEventService } from '../services/user-event.service';
import * as Sentry from '@sentry/angular';

@Injectable()
export class HttpConfigInterceptor implements HttpInterceptor {
Expand Down Expand Up @@ -153,6 +154,7 @@ export class HttpConfigInterceptor implements HttpInterceptor {
return next.handle(request).pipe(
catchError((error) => {
if (error instanceof HttpErrorResponse) {
this.handleSentryError(error, request);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
this.handleSentryError(error, request);
if (error.status >= 500) {
this.sendErrorToSentry(error, request);
}

Copy link
Contributor Author

@devendrafyle devendrafyle Jan 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this condition is considered by sendErrorToSentry method itself.
image

if (this.expiringSoon(token)) {
return from(this.refreshAccessToken()).pipe(
mergeMap((newToken) => {
Expand All @@ -177,6 +179,44 @@ export class HttpConfigInterceptor implements HttpInterceptor {
})
);
}

private handleSentryError(error: HttpErrorResponse, request: HttpRequest<string>): void {
if (error.status >= 500) {
const errorObject = new Error(`API ${error.status} Error: ${error.message || 'Server error'}`);

Object.assign(errorObject, {
status: error.status,
statusText: error.statusText,
});

Sentry.captureException(errorObject, {
tags: {
errorType: 'API_500',
apiEndpoint: error.url,
},
extra: {
requestUrl: request.url,
requestMethod: request.method,
requestHeaders: request.headers,
requestBody: request.body,
responseStatus: error.status,
responseStatusText: error.statusText,
responseData: error.error,
},
});
Copy link

@coderabbitai coderabbitai bot Jan 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Safety is paramount! Sanitize data before sending to Sentry

When capturing exceptions, ensure that sensitive information isn't sent to Sentry. Let's sanitize request.headers, request.body, and error.error to protect user data.

Consider modifying your code like this:

+        // Sanitize sensitive data
+        const sanitizedHeaders = this.sanitizeHeaders(request.headers);
+        const sanitizedBody = this.sanitizeBody(request.body);
+
         Sentry.captureException(errorObject, {
           tags: {
             errorType: 'API_500',
             apiEndpoint: error.url,
           },
           extra: {
             requestUrl: request.url,
             requestMethod: request.method,
-            requestHeaders: request.headers,
-            requestBody: request.body,
+            requestHeaders: sanitizedHeaders,
+            requestBody: sanitizedBody,
             responseStatus: error.status,
             responseStatusText: error.statusText,
             responseData: error.error,
           },
         });

You'll need to implement the sanitizeHeaders and sanitizeBody methods to remove or mask sensitive data before logging.

Committable suggestion skipped: line range outside the PR's diff.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is important. Any API call may throw 500. We should not send headers and body without sanitizing it.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Chethan-Fyle Yo boss! You're absolutely right! Like a vigilant bodyguard protecting VIPs, we must protect sensitive data. I'll make sure to remember this for future reviews.

Would you like me to create a GitHub issue to track this security enhancement? I can help draft one with the necessary details while keeping sensitive implementation details private.


✏️ Learnings added
Learnt from: Chethan-Fyle
PR: fylein/fyle-mobile-app#3413
File: src/app/core/interceptors/httpInterceptor.ts:183-206
Timestamp: 2025-01-09T06:30:16.378Z
Learning: In Angular HTTP interceptors, when logging errors to Sentry (especially for 500 errors), always sanitize sensitive information from request headers, request body, and error responses before sending them to avoid potential data exposure. This applies to all API error logging scenarios.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, i will update this

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!


Sentry.addBreadcrumb({
category: 'api',
message: `API call failed with status ${error.status}`,
level: 'error',
data: {
url: request.url,
method: request.method,
status: error.status,
},
});
}
}
}

export class CustomEncoder implements HttpParameterCodec {
Expand Down
33 changes: 30 additions & 3 deletions src/app/fyle/my-view-report/my-view-report.page.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ import { ExtendedComment } from 'src/app/core/models/platform/v1/extended-commen
import { Comment } from 'src/app/core/models/platform/v1/comment.model';
import { ExpenseTransactionStatus } from 'src/app/core/enums/platform/v1/expense-transaction-status.enum';
import * as Sentry from '@sentry/angular';
import { HttpErrorResponse } from '@angular/common/http';

@Component({
selector: 'app-my-view-report',
Expand Down Expand Up @@ -455,12 +456,38 @@ export class MyViewReportPage {
});
this.trackingService.showToastMessage({ ToastContent: message });
},
error: (error) => {
// Capture error with additional details in Sentry
Sentry.captureException(error, {
error: (error: HttpErrorResponse) => {
const errorObject = new Error(`Report Submission Failed: ${error.message || 'Unknown error'}`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are we capturing sentry exceptions directly here? why is this not being captured globally?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No we want some additional data for these cases for figure out why this is happening, not able to get info context from global exception

if (error.status) {
Object.assign(errorObject, {
status: error.status,
statusText: error.statusText,
});
}

Sentry.addBreadcrumb({
category: 'reports',
message: 'Failed to submit report',
level: 'error',
data: {
reportId: this.reportId,
errorStatus: error.status,
errorMessage: error.message,
},
});

Sentry.captureException(errorObject, {
tags: {
errorType: 'REPORT_SUBMISSION_FAILED',
reportId: this.reportId,
},
extra: {
reportId: this.reportId,
errorResponse: error,
context: {
component: 'ReportSubmissionComponent',
action: 'submitReport',
},
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Heads up! Inform the user when submission fails

If the report submission fails, the user should be notified. Let's add a message to keep them informed.

Consider updating your error handler:

             Sentry.captureException(errorObject, {
               tags: {
                 errorType: 'REPORT_SUBMISSION_FAILED',
                 reportId: this.reportId,
               },
               extra: {
                 reportId: this.reportId,
                 errorResponse: error,
                 context: {
                   component: 'ReportSubmissionComponent',
                   action: 'submitReport',
                 },
               },
             });
+
+            // Notify the user about the error
+            const message = 'Report submission failed. Please try again.';
+            this.matSnackBar.openFromComponent(ToastMessageComponent, {
+              ...this.snackbarProperties.setSnackbarProperties('failure', { message }),
+              panelClass: ['msb-failure'],
+            });
+
+            // Track the error event
+            this.trackingService.showToastMessage({ ToastContent: message });

Committable suggestion skipped: line range outside the PR's diff.

},
});
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -118,8 +118,23 @@ export class CameraPreviewComponent implements OnInit, OnChanges {
stopCamera(): void {
//Stop camera only if it is in RUNNING state
if (this.cameraState === CameraState.RUNNING) {
const currentCameraState = this.cameraState;
this.cameraState = CameraState.STOPPING;
from(this.cameraPreviewService.stop()).subscribe(() => (this.cameraState = CameraState.STOPPED));

from(this.cameraPreviewService.stop()).subscribe({
next: () => {
this.cameraState = CameraState.STOPPED;
},
error: (error) => {
Sentry.captureException(error, {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

extra: {
errorResponse: error,
currentCameraState,
cameraState: this.cameraState,
},
});
},
});
}
}

Expand Down
Loading