-
Notifications
You must be signed in to change notification settings - Fork 410
feat: add basic fpnv #3030
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
base: fpnv
Are you sure you want to change the base?
feat: add basic fpnv #3030
Conversation
Summary of ChangesHello @boikoa-gl, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new Firebase Phone Number Verification (FPNV) module into the Firebase Admin SDK. This feature enables developers to verify phone number tokens, similar to how existing ID tokens are handled. The changes involve adding the necessary service classes, integrating them into the SDK's public API and build configuration, and establishing robust token verification logic, including custom error handling and support for the ES256 JWT algorithm. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request introduces a new fpnv (Firebase Phone Number Verification) service. The overall structure is good and follows the existing patterns in the SDK. However, there are a few critical issues that need to be addressed before merging. The error handling for this new service is incomplete, with placeholder error codes in FpnvErrorCode. This will lead to incorrect error reporting. I've provided a suggestion for a more complete implementation. There's also a potential bug in the JWT audience (aud) claim verification that could cause a runtime error. Finally, I've made some suggestions to use more specific error codes to improve the developer experience. Please review the detailed comments.
lahirumaramba
left a comment
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.
Thanks! Added a few comments. Please check the CI errors.
Run npm run api-extractor:local to generate apidocs and update the PR with the new files.
lahirumaramba
left a comment
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.
Thanks! It is looking good!
Added a few more comments
|
/gemini review |
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.
Code Review
This pull request introduces a new fpnv (Firebase Phone Number Verification) module. The changes include adding the necessary entry points, API definitions, implementation, and tests. The core logic for token verification seems solid, with good validation and error handling. I've provided some feedback to improve documentation accuracy, address API extractor warnings, and align with some coding best practices. Overall, this is a good addition.
|
/gemini review |
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.
Code Review
This pull request introduces a new Firebase Phone Number Verification (FPNV) feature. The changes include adding the necessary API definitions, implementation, and tests for the fpnv module. Overall, the structure is sound and follows the existing patterns in the SDK. However, I've identified a critical issue in the JWT signature verification logic that will prevent the feature from working correctly. Additionally, there are a few areas for improvement regarding API documentation and type exports to enhance clarity and usability for developers. Addressing these points will ensure the new feature is robust and well-integrated.
|
/gemini review |
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.
Code Review
This pull request introduces a new fpnv (Firebase Phone Number Verification) module. The changes include adding the new module's entrypoints, public API definitions, implementation, and tests. The overall structure is consistent with other modules in the SDK. I've identified a few areas for improvement regarding API documentation, TypeScript type exports, and validation logic robustness. My comments provide specific suggestions to address these points.
| private verifyContent( | ||
| fullDecodedToken: DecodedToken, | ||
| ): void { | ||
| const header = fullDecodedToken && fullDecodedToken.header; | ||
| const payload = fullDecodedToken && fullDecodedToken.payload; | ||
|
|
||
| const scopedProjectId = `${this.issuer}${payload?.iss?.split('/')?.pop()}`; | ||
| const projectIdMatchMessage = ` Make sure the ${this.tokenInfo.shortName} comes from the same ` + | ||
| 'Firebase project as the service account used to authenticate this SDK.'; | ||
| const verifyJwtTokenDocsMessage = ` See ${this.tokenInfo.url} ` + | ||
| `for details on how to retrieve ${this.shortNameArticle} ${this.tokenInfo.shortName}.`; | ||
|
|
||
| let errorMessage: string | undefined; | ||
|
|
||
| // JWT Header | ||
| if (typeof header.kid === 'undefined') { | ||
| errorMessage = `${this.tokenInfo.jwtName} has no "kid" claim.`; | ||
| errorMessage += verifyJwtTokenDocsMessage; | ||
| } else if (header.alg !== ALGORITHM_ES256) { | ||
| errorMessage = `${this.tokenInfo.jwtName} has incorrect algorithm. Expected ` + | ||
| `"${ALGORITHM_ES256}" but got "${header.alg}". ${verifyJwtTokenDocsMessage}`; | ||
| } else if (header.typ !== this.tokenInfo.typ) { | ||
| errorMessage = `${this.tokenInfo.jwtName} has incorrect typ. Expected "${this.tokenInfo.typ}" but got ` + | ||
| `"${header.typ}". ${verifyJwtTokenDocsMessage}`; | ||
| } | ||
| // FPNV Token | ||
| else if (!validator.isNonEmptyArray(payload.aud) || !payload.aud.includes(scopedProjectId)) { | ||
| errorMessage = `${this.tokenInfo.jwtName} has incorrect "aud" (audience) claim. Expected ` + | ||
| `"${scopedProjectId}" to be one of "${payload.aud}". ${projectIdMatchMessage} ${verifyJwtTokenDocsMessage}`; | ||
| } else if (typeof payload.sub !== 'string') { | ||
| errorMessage = `${this.tokenInfo.jwtName} has no "sub" (subject) claim. ${verifyJwtTokenDocsMessage}`; | ||
| } else if (payload.sub === '') { | ||
| errorMessage = `${this.tokenInfo.jwtName} has an empty "sub" (subject) claim. ${verifyJwtTokenDocsMessage}`; | ||
| } | ||
|
|
||
| if (errorMessage) { | ||
| throw new FirebaseFpnvError(FPNV_ERROR_CODE_MAPPING.INVALID_ARGUMENT, errorMessage); | ||
| } | ||
| } |
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.
The current implementation for content verification can be improved for clarity and robustness. Specifically, the iss claim is used to construct scopedProjectId without being validated first. This can lead to confusing error messages if iss is missing or malformed.
I suggest restructuring the validation to first check for the iss claim, and then use it to validate the aud claim. This makes the logic clearer and provides better error feedback.
private verifyContent(
fullDecodedToken: DecodedToken,
): void {
const header = fullDecodedToken?.header;
const payload = fullDecodedToken?.payload;
const projectIdMatchMessage = ` Make sure the ${this.tokenInfo.shortName} comes from the same ` +
'Firebase project as the service account used to authenticate this SDK.';
const verifyJwtTokenDocsMessage = ` See ${this.tokenInfo.url} ` +
`for details on how to retrieve ${this.shortNameArticle} ${this.tokenInfo.shortName}.`;
let errorMessage: string | undefined;
// JWT Header
if (typeof header.kid === 'undefined') {
errorMessage = `${this.tokenInfo.jwtName} has no "kid" claim.`;
errorMessage += verifyJwtTokenDocsMessage;
} else if (header.alg !== ALGORITHM_ES256) {
errorMessage = `${this.tokenInfo.jwtName} has incorrect algorithm. Expected ` +
`"${ALGORITHM_ES256}" but got "${header.alg}". ${verifyJwtTokenDocsMessage}`;
} else if (header.typ !== this.tokenInfo.typ) {
errorMessage = `${this.tokenInfo.jwtName} has incorrect typ. Expected "${this.tokenInfo.typ}" but got ` +
`"${header.typ}". ${verifyJwtTokenDocsMessage}`;
}
// FPNV Token
else if (typeof payload.iss !== 'string' || !payload.iss.startsWith(this.issuer)) {
errorMessage = `${this.tokenInfo.jwtName} has incorrect "iss" (issuer) claim. Expected ` +
`an issuer starting with "${this.issuer}" but got "${payload.iss}". ${projectIdMatchMessage} ${verifyJwtTokenDocsMessage}`;
} else if (!validator.isNonEmptyArray(payload.aud) || !payload.aud.includes(payload.iss)) {
errorMessage = `${this.tokenInfo.jwtName} has incorrect "aud" (audience) claim. Expected ` +
`"${payload.iss}" to be present in the aud claim. ${projectIdMatchMessage} ${verifyJwtTokenDocsMessage}`;
} else if (typeof payload.sub !== 'string') {
errorMessage = `${this.tokenInfo.jwtName} has no "sub" (subject) claim. ${verifyJwtTokenDocsMessage}`;
} else if (payload.sub === '') {
errorMessage = `${this.tokenInfo.jwtName} has an empty "sub" (subject) claim. ${verifyJwtTokenDocsMessage}`;
}
if (errorMessage) {
throw new FirebaseFpnvError(FPNV_ERROR_CODE_MAPPING.INVALID_ARGUMENT, errorMessage);
}
}| * The audience for which this token is intended. | ||
| * This value is an array of two strings, which are URLs that include the project number | ||
| * and project ID of your Firebase project | ||
| * E.g. `https://fpnv.googleapis.com/projects/<PROJECT_ID>`. | ||
| */ |
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.
The documentation for the aud claim is a bit confusing and the example seems incorrect. The implementation in token-verifier.ts checks for an audience URL containing the project number, but the example here shows a project ID. Also, the description says it's an array of two strings which are URLs, but the token may contain other values in the aud array.
To improve clarity and align with the implementation, I suggest updating the JSDoc.
| * The audience for which this token is intended. | |
| * This value is an array of two strings, which are URLs that include the project number | |
| * and project ID of your Firebase project | |
| * E.g. `https://fpnv.googleapis.com/projects/<PROJECT_ID>`. | |
| */ | |
| /** | |
| * The audience for which this token is intended. | |
| * This value is a string array, one of which is a URL with the format | |
| * `https://fpnv.googleapis.com/projects/<PROJECT_NUMBER>`, where `<PROJECT_NUMBER>` is the | |
| * project number of your Firebase project. | |
| */ |
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.
Code Review
This pull request introduces a new fpnv (Firebase Phone Number Verification) module. The changes include adding the new module's entry points, public API definitions, implementation, and tests. The overall structure is consistent with other modules in the repository.
I've found a critical security issue in the token verification logic where the audience claim is not validated against the project ID, which could allow tokens from one project to be accepted by another. I've also identified a missing export in the public API that would affect TypeScript users. Additionally, there are a few minor suggestions to improve code quality and maintainability. Please see my detailed comments.
Hey there! So you want to contribute to a Firebase SDK?
Before you file this pull request, please read these guidelines:
Discussion
If not, go file an issue about this before creating a pull request to discuss.
Testing
API Changes
us make Firebase APIs better, please propose your change in an issue so that we
can discuss it together.