-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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.
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
Update tests for exposing internal error description #11322
base: main
Are you sure you want to change the base?
Conversation
- when the error received is of type `.internal`, the current code rewrites the message with `descriptionForErrorCode`. This is pretty bad because any contextual information we might have received from Firebase is destroyed. - this fix attempts to extract the message from the response payload, preserving the original value. - if message is not found, the message is set to the value returned by `descriptionForErrorCode`, as was done before.
…pb-internal-error-fix
5cbd1ec
to
f094a90
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I understand the intent behind this code, I'm curious where this came from and whether it would help. For example, here's the docs for the internal error:
- `internal`: Internal errors. Means some invariants expected by
underlying system has been broken. If you see one of these errors,
something is very broken.
For this category of errors, we cannot know if they contain sensitive (esp security sensitive) information and so the server shouldn't be sending context, let alone the client exposing it. The only two places where we return an internal response are if the customer's version of the Admin SDK is too old to support AppCheck features they're trying to use or if the RPC has an unhandled exception.
If you want to make this change because you have user reports that customers are returning "internal" errors with message details, I'd suggest talking to @taeold (our TL) to agree this is the right course and then draft an API proposal so we can keep this in sync in our iOS, Android, and Web SDKs if not also our C++ and Unity SDKs.
@inlined or @taeold Would you follow up on the external PR discussion at #11311 (comment)? |
Fix tests for #11311 which improves error generation for internal errors to fix #11310.