Improve decodeBase64() to throw on invalid input rather than silently accept it #7019
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: 63b10fc The changes in this PR will be included in the next version bump. This PR includes changesets to release 28 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Size Report 1Affected Products
Test Logs |
Size Analysis Report 1This report is too large (756,081 characters) to be displayed here in a GitHub comment. Please use the below link to see the full report on Google Cloud Storage.Test Logs |
Base automatically changed from
dconeybe/PlatformTextEncoderRefactor
to
master
February 10, 2023 14:11
hsubox76
reviewed
Feb 10, 2023
| ); | ||
| } catch (e) { | ||
| if (e instanceof DecodeBase64StringError) { | ||
| throw new Base64DecodeError('Invalid base64 string: ' + e); |
There was a problem hiding this comment.
I haven't followed these all the way up the chain but will they eventually get wrapped in a FirestoreError?
There was a problem hiding this comment.
No, the error won't be re-thrown as a FirestoreError. The specific use case is a future optimization that involves decoding base64. If, for some reason, we get invalid base64 from the backend then we will just ignore it and disable the optimization. We don't expect to ever get invalid base64, but it could theoretically happen and if it ever does we don't want to crash.
The problem was that it was previously not possible to reliably detect an invalid base64 string. This PR fixes that by having the new Base64DecodeError always thrown if an invalid base64 string is given as input. The future code will catch this exception and use it to disable the optimization.
hsubox76
approved these changes
Feb 11, 2023
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix all
decodeBase64()functions to throw an exception when given invalid input. Previously, some implementations silently accepted invalid input, potentially leading to bugs.The
decodeBase64()function inpackages/firestore/src/platform/base64.tshas different implementations on different platforms (e.g. browser, node, and react native). These implementations behave differently when given an invalid base64 string to decode:DOMExceptionon any invalid input.Erroron some invalid inputs, and silently accepts others.The node implementation used to have a regular expression that validated the input (link); however, it was removed in #6008 because it was too slow.
The "parent"
decodeBase64()function now validates that the input string is valid base64 using an algorithm that is orders of magnitude more efficient and robust than a regular expression. It verifies that the input string has the expected length based on the number of bytes returned from the platform-specific base64 decoding function to which it delegates. If the length is not correct then the platform-specific function must have truncated at an invalid base64 character. In this case a newBase64DecodeErroris thrown.Credit for this PR goes to @milaGGL from #6992. I'm merely merging a small component of that PR into the master branch to reduce the diff when her PR ultimately gets merged.