Update tests for exposing internal error description #11322
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
paulb777
force-pushed
the
pb-internal-error-fix
branch
from
5cbd1ec
to
f094a90
Compare
There was a problem hiding this comment.
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.