Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(shared): SanitizeExceptions decorator #18164

Merged
merged 1 commit into from
Jan 10, 2025
Merged

feat(shared): SanitizeExceptions decorator #18164

merged 1 commit into from
Jan 10, 2025

Conversation

david1alvarez
Copy link
Contributor

@david1alvarez david1alvarez commented Dec 20, 2024 •
edited
Loading

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 pull request

  • 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.

Issue that this pull request solves

Closes: FXA-10626

Checklist

Put an x in the boxes that apply

  • My commit is GPG signed.
  • If applicable, I have modified or added tests which pass locally.
  • I have added necessary documentation (if appropriate).
  • I have verified that my changes render correctly in RTL (if appropriate).

@david1alvarez david1alvarez requested a review from a team as a code owner December 20, 2024 21:15
@david1alvarez david1alvarez force-pushed the FXA-10626 branch 2 times, most recently from 78189bd to b78e802 Compare January 3, 2025 18:32
@david1alvarez david1alvarez changed the title Feat(shared): GenericError decorator Feat(shared): SanitizeExceptions decorator Jan 3, 2025
@david1alvarez david1alvarez force-pushed the FXA-10626 branch 2 times, most recently from 32cb293 to b6653fc Compare January 4, 2025 00:25
StaberindeZA
StaberindeZA reviewed Jan 6, 2025
View reviewed changes
Copy link
Contributor

@StaberindeZA StaberindeZA left a comment

Choose a reason for hiding this comment

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

This looks good so far. Just left some inline comments in a few spots.

nit. could you fix up the git commit body to match fxa contributors guidelines?

libs/shared/error/src/lib/sanitizeExceptionsDecorator.ts Outdated Show resolved Hide resolved
libs/shared/error/src/lib/sanitizeExceptionsDecorator.ts Outdated Show resolved Hide resolved
libs/shared/error/src/lib/sanitizeExceptionsDecorator.ts Outdated Show resolved Hide resolved
libs/payments/cart/src/lib/cart.service.ts Outdated Show resolved Hide resolved
@david1alvarez david1alvarez changed the title Feat(shared): SanitizeExceptions decorator feat(shared): SanitizeExceptions decorator Jan 6, 2025
@david1alvarez david1alvarez force-pushed the FXA-10626 branch 3 times, most recently from 7c34fca to 87a366c Compare January 8, 2025 21:06
StaberindeZA
StaberindeZA reviewed Jan 9, 2025
View reviewed changes
Copy link
Contributor

@StaberindeZA StaberindeZA left a comment

Choose a reason for hiding this comment

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

Mostly just a few questions left. But otherwise this looks good.

libs/google/src/lib/google.manager.ts Outdated Show resolved Hide resolved
libs/payments/cart/src/lib/cart.manager.in.spec.ts Outdated Show resolved Hide resolved
libs/payments/customer/src/lib/customer.manager.spec.ts Outdated Show resolved Hide resolved
libs/shared/cms/src/lib/product-configuration.manager.ts Outdated Show resolved Hide resolved
libs/shared/error/src/lib/sanitizeExceptionsDecorator.ts Show resolved Hide resolved
libs/shared/error/src/lib/sanitizeExceptionsDecorator.ts Outdated Show resolved Hide resolved
libs/shared/error/src/lib/sanitizeExceptionsDecorator.ts Outdated Show resolved Hide resolved
@david1alvarez david1alvarez force-pushed the FXA-10626 branch 6 times, most recently from b373c3c to bc2ea71 Compare January 10, 2025 19:39
StaberindeZA
StaberindeZA approved these changes Jan 10, 2025
View reviewed changes
Copy link
Contributor

@StaberindeZA StaberindeZA left a comment

Choose a reason for hiding this comment

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

lgtm!

@david1alvarez
feat(shared): Add SanitizeExceptions decorator to shared/error
db54463 
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
@david1alvarez david1alvarez merged commit e47544f into main Jan 10, 2025
25 checks passed
@david1alvarez david1alvarez deleted the FXA-10626 branch January 10, 2025 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@StaberindeZA StaberindeZA StaberindeZA approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@david1alvarez @StaberindeZA