mozilla / fxa-auth-server

DEPRECATED - Migrated to https://github.com/mozilla/fxa
Mozilla Public License 2.0
399 stars 121 forks source link

fix(errors): add some more context to our 500 errors #2910

Closed philbooth closed 5 years ago

philbooth commented 5 years ago

Related to #2884, but a speculative patch rather than a definitive fix for it.

As discussed in the FxA meeting a few weeks ago, sometimes our 500 errors are devoid of any context that would help identify the cause in Sentry. I know we talked about other Sentry options we can set and updating the npm dependency too, but I wondered whether just adding a bit more data to the error object would be a quick win to get us started regardless of those. Hence, this PR.

Of course, setting extra data on the error object means returning it through the API, so I only picked out 2 particular cases where I think nothing sensitive could leak out. I decided not to add an error message or stack trace to backendServiceFailure for instance, lest it reveal some inner implementation detail of our production environment.

The two errors that I did add data to are internalValidationError and unexpectedError:

My hope is that next time either one of these shows up unexpected in Sentry, the extra context will help us understand what actually went wrong. Not sure what you guys think though, feel free to close if you think I'm barking up the wrong tree.

@mozilla/fxa-devs r?

philbooth commented 5 years ago

@vbudhram rightly pointed out in the meeting just now that request.app.geo is PII, so I've pulled that data out.

philbooth commented 5 years ago

In 811e584, I've addressed @vladikoff's point from the meeting about not sending PII from the request payload to Sentry.