[micah-gcp]

Summary

Updates PhoneNumberVerification.verifyToken() in firebase-admin/phone-number-verification to handle test-mode tokens ("test_number": true) safely by default:

• Reject test-mode tokens by default with TEST_TOKEN_REJECTED: Adds TEST_TOKEN_REJECTED: 'test-token-rejected' to PhoneNumberVerificationErrorCode. When a decoded FPNV token contains "test_number": true, verifyToken(jwt, options?) throws FirebasePhoneNumberVerificationError (phone-number-verification/test-token-rejected) unless options?.acceptTestTokens === true.
• Add PhoneNumberVerificationTokenOptions: Exports PhoneNumberVerificationTokenOptions ({ acceptTestTokens?: boolean }) from firebase-admin/phone-number-verification and validates that options, when provided, is a non-null object and acceptTestTokens is a boolean.
• Populate testNumber on PhoneNumberVerificationToken: Sets the required camelCase testNumber: boolean convenience property (decodedIdToken.test_number === true), matching how phoneNumber mirrors sub.

Testing

• Added unit test coverage for PhoneNumberVerificationErrorCode.TEST_TOKEN_REJECTED (including err.hasCode()), PhoneNumberVerificationTokenOptions validation, default test-token rejection, { acceptTestTokens: true } opt-in, and testNumber population.
• Updated the API Extractor report (etc/firebase-admin.phone-number-verification.api.md) and verified npm test and npm run api-extractor.

[gemini-code-assist]

Code Review

This pull request introduces support for verifying Firebase Phone Number Verification tokens issued in test mode by adding a new 'VerifyPhoneNumberTokenOptions' interface and updating the verification logic to reject test-mode tokens by default unless 'acceptTestTokens' is set to 'true'. The review feedback recommends improving robustness and type safety by using strict inequality when checking the 'acceptTestTokens' option, validating that it is a boolean if provided, and adding corresponding unit tests to cover this validation.

[gemini-code-assist]

Using a loose truthiness check (!options?.acceptTestTokens) can be risky if a non-boolean truthy value (such as a non-empty string or a number) is passed. It is safer and more robust to use strict inequality (options?.acceptTestTokens !== true) to ensure that test tokens are only accepted when the option is explicitly set to true.

Suggested change

	if (decodedIdToken.test_number === true && !options?.acceptTestTokens) {
	if (decodedIdToken.test_number === true && options?.acceptTestTokens !== true) {

[gemini-code-assist]

To prevent potential runtime issues and ensure robust type safety, we should validate that acceptTestTokens is a boolean if it is provided in the options object. This prevents developers from accidentally passing non-boolean truthy values (like strings or numbers) which might otherwise bypass the test token rejection logic.

  private validateVerifyPhoneNumberTokenOptions(options?: VerifyPhoneNumberTokenOptions): void {
    if (typeof options === 'undefined') {
      return;
    }
    if (!validator.isNonNullObject(options)) {
      throw new FirebasePhoneNumberVerificationError({
        code: FPNV_ERROR_CODE_MAPPING.INVALID_ARGUMENT,
        message: 'VerifyPhoneNumberTokenOptions must be a non-null object.',
      });
    }
    if (typeof options.acceptTestTokens !== 'undefined' && !validator.isBoolean(options.acceptTestTokens)) {
      throw new FirebasePhoneNumberVerificationError({
        code: FPNV_ERROR_CODE_MAPPING.INVALID_ARGUMENT,
        message: 'acceptTestTokens option must be a boolean.',
      });
    }
  }

[gemini-code-assist]

Add test cases to verify that passing a non-boolean value for acceptTestTokens correctly throws an error, matching the new validation logic.

    it('should throw given invalid VerifyPhoneNumberTokenOptions', () => {
      [null, 100, -100, 'abc', [], true].forEach((invalidOptions) => {
        expect(() => {
          return phoneNumberVerificationService.verifyToken(mockTokenString, invalidOptions as any);
        }).to.throw('VerifyPhoneNumberTokenOptions must be a non-null object.');
      });

      [123, 'true', [], null].forEach((invalidAcceptTestTokens) => {
        expect(() => {
          return phoneNumberVerificationService.verifyToken(mockTokenString, { acceptTestTokens: invalidAcceptTestTokens as any });
        }).to.throw('acceptTestTokens option must be a boolean.');
      });
    }

[lahirumaramba]

Hey @micah-gcp, thank you for the PR! Overall this looks great. I added a few comments on input validation consistency and getting a quick TW review on the new docstrings, let me know what you think!

[lahirumaramba]

WDYT about moving validatePhoneNumberVerificationTokenOptions(options) into PhoneNumberTokenVerifier.verifyJWT(), right after the jwt string check? verifyToken() isn't async, so bad options throw synchronously while a bad jwt returns a rejected promise. Moving it keeps both on the promise path and validates jwt first.

[lahirumaramba]

through the public API, { acceptTestTokens: 'false' as any } gives invalid-argument, but calling verifyJWT() directly skips options validation and gives test-token-rejected. With the move above, both reject with INVALID_ARGUMENT.

[lahirumaramba]

Could we get a TW review on the new docstrings?

super nit: I know the API proposal settled on PhoneNumberVerificationTokenOptions, but we could also consider VerifyPhoneNumberTokenOptions to mirror VerifyAppCheckTokenOptions if we really want to :)

[lahirumaramba]

nit: also assert the error is a FirebasePhoneNumberVerificationError with code phone-number-verification/invalid-argument (here and in the acceptTestTokens test below).

[lahirumaramba]

indentation in PhoneNumberVerificationTokenOptions looks off. Could you compare with PhoneNumberVerificationToken, please?