Add precondition format checks against api-key and app-id #3562
Conversation
This file contains hidden or 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 is good to goLatest commit: 64edab6 We got this. This PR includes changesets to release 13 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 |
Binary Size ReportAffected SDKs
Test Logs
|
zwu52
requested changes
Aug 5, 2020
zwu52
approved these changes
Aug 12, 2020
Feiyang1
reviewed
Aug 13, 2020
| @@ -30,6 +32,14 @@ export const enum ErrorCode { | |||
| const ERROR_DESCRIPTION_MAP: { readonly [key in ErrorCode]: string } = { | |||
| [ErrorCode.MISSING_APP_CONFIG_VALUES]: | |||
| 'Missing App configuration value: "{$valueName}"', | |||
| [ErrorCode.INVALID_APP_ID_FORMAT]: | |||
| 'Please set your Application ID. A valid Firebase App ID is required to communicate ' + | |||
There was a problem hiding this comment.
The message sounds like the app id is missing, but the error is actually a malformed appId. I would also just use appId which matches the name in the config object.
There was a problem hiding this comment.
makes sense. done
| 'with Firebase server APIs: It identifies your application with Firebase.' + | ||
| 'Please refer to https://firebase.google.com/support/privacy/init-options.', | ||
| [ErrorCode.INVALID_API_KEY_FORMAT]: | ||
| 'Please set a valid API key. A Firebase API key is required to communicate with ' + |
There was a problem hiding this comment.
Similarly I would just use apiKey to match the name in the config object.
| @@ -33,7 +33,10 @@ export function newTestFirestore( | |||
| } | |||
| const app = | |||
| typeof nameOrApp === 'string' | |||
| ? firebase.initializeApp({ apiKey: 'fake-api-key', projectId }, nameOrApp) | |||
| ? firebase.initializeApp( | |||
| { apiKey: 'A-test-api-key-jklmnopqrstuvwxyz1234567', projectId }, | |||
There was a problem hiding this comment.
Does the test fail without the change? I think you only need the correct format in installation tests, specifically the tests that involves extractAppConfig(). We can leave other places unchanged, right?
There was a problem hiding this comment.
Yeah, I saw many tests were failing so I added these changes.
There was a problem hiding this comment.
That is weird. I don't think your change affects firestore. Are you sure the failure comes from firestore? It will be really inconvenient if we are forced use exact 38 characters apiKey in tests.
andirayo
reviewed
Mar 9, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Added FIS client side precondition checks for JS SDK as we did for Android SDK.
Added precondition format checks for App-Id and Api-Key firebase-android-sdk#1470
Add tests for FIS format check firebase-android-sdk#1502
Move FIS precondition format check to installation constructor firebase-android-sdk#1507
Fixed invalid test Api keys and App ids for affected integration tests and unit tests.