Skip to content

Conversation

@boikoa-gl
Copy link

Hey there! So you want to contribute to a Firebase SDK?
Before you file this pull request, please read these guidelines:

Discussion

  • Read the contribution guidelines (CONTRIBUTING.md).
  • If this has been discussed in an issue, make sure to link to the issue here.
    If not, go file an issue about this before creating a pull request to discuss.

Testing

  • Make sure all existing tests in the repository pass after your change.
  • If you fixed a bug or added a feature, add a new test to cover your code.

API Changes

  • At this time we cannot accept changes that affect the public API. If you'd like to help
    us make Firebase APIs better, please propose your change in an issue so that we
    can discuss it together.

@gemini-code-assist
Copy link

Summary of Changes

Hello @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

  • New FPNV Module: Introduced a new Firebase Phone Number Verification (FPNV) module to the SDK.
  • SDK Integration: Integrated the FPNV service into the Firebase Admin SDK's public API, including entrypoints.json, package.json exports, and the firebase-namespace-api.ts.
  • Token Verification Logic: Added core logic for verifying phone number tokens, including decoding, content validation, and signature verification using FirebasePhoneNumberTokenVerifier.
  • Error Handling: Implemented dedicated error handling for FPNV operations with the new FirebasePnvError class and FpnvErrorCode definitions.
  • ES256 Algorithm Support: Extended JWT verification utilities to support the ES256 algorithm.
Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link

@gemini-code-assist gemini-code-assist bot left a 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.

Copy link
Member

@lahirumaramba lahirumaramba left a 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.

Copy link
Member

@lahirumaramba lahirumaramba left a 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

@lahirumaramba
Copy link
Member

/gemini review

Copy link

@gemini-code-assist gemini-code-assist bot left a 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.

@boikoa-gl
Copy link
Author

/gemini review

Copy link

@gemini-code-assist gemini-code-assist bot left a 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.

@boikoa-gl
Copy link
Author

/gemini review

Copy link

@gemini-code-assist gemini-code-assist bot left a 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.

Comment on lines +136 to +174
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);
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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);
    }
  }

Comment on lines +32 to +36
* 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>`.
*/

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
* 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.
*/

Copy link

@gemini-code-assist gemini-code-assist bot left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants