Skip to content
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

Only fetch reCAPTCHA v2 token when FAKE_TOKEN #8493

Merged
merged 1 commit into from
Sep 10, 2024
Merged

Conversation

NhienLam
Copy link
Contributor

@NhienLam NhienLam commented Sep 10, 2024

Issue
Currently, we show the challenge and fetch reCAPTCHA v2 token when reCAPTCHA Enterprise token is empty (token fetch fails) before making the phone auth request.

In Enforce mode, we fetch rcv2 token when rCE token is empty. This is incorrect behavior, we shouldn't fetch rcv2 token in Enforce mode. Fetching rcv2 means we will show the rcv2 challenge, which would make confuse developers as to why we show rcv2 challenge when they're in Enforce mode.

We expect to throw MISSING_RECAPTCHA_TOKEN instead without fetching rcv2 token. This PR fixes that.

Testing

  • CI
  • Manual testing

Copy link

changeset-bot bot commented Sep 10, 2024

⚠️ No Changeset found

Latest commit: a315256

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@NhienLam NhienLam removed request for a team, sam-gc and lisajian September 10, 2024 21:31
@google-oss-bot
Copy link
Contributor

Size Report 1

Affected Products

  • @firebase/auth

    TypeBase (41a5f9f)Merge (279fbef)Diff
    browser188 kB188 kB-258 B (-0.1%)
    esm5244 kB244 kB-258 B (-0.1%)
    module188 kB188 kB-258 B (-0.1%)
    react-native207 kB207 kB-258 B (-0.1%)
  • @firebase/auth/internal

    TypeBase (41a5f9f)Merge (279fbef)Diff
    browser199 kB199 kB-258 B (-0.1%)
    esm5258 kB257 kB-258 B (-0.1%)
    module199 kB199 kB-258 B (-0.1%)
  • bundle

    TypeBase (41a5f9f)Merge (279fbef)Diff
    auth (Phone)93.5 kB93.2 kB-222 B (-0.2%)
  • firebase

    TypeBase (41a5f9f)Merge (279fbef)Diff
    firebase-auth-compat.js143 kB143 kB-219 B (-0.2%)
    firebase-auth.js155 kB154 kB-222 B (-0.1%)
    firebase-compat.js792 kB792 kB-219 B (-0.0%)

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/KY2imtaAeM.html

@google-oss-bot
Copy link
Contributor

Size Analysis Report 1

Affected Products

  • @firebase/auth

    • PhoneAuthProvider

      Size

      TypeBase (41a5f9f)Merge (279fbef)Diff
      size50.5 kB50.2 kB-222 B (-0.4%)
      size-with-ext-deps72.1 kB71.8 kB-222 B (-0.3%)
    • linkWithPhoneNumber

      Size

      TypeBase (41a5f9f)Merge (279fbef)Diff
      size51.2 kB51.0 kB-222 B (-0.4%)
      size-with-ext-deps72.8 kB72.6 kB-222 B (-0.3%)
    • reauthenticateWithPhoneNumber

      Size

      TypeBase (41a5f9f)Merge (279fbef)Diff
      size51.9 kB51.6 kB-222 B (-0.4%)
      size-with-ext-deps73.5 kB73.2 kB-222 B (-0.3%)
    • signInWithPhoneNumber

      Size

      TypeBase (41a5f9f)Merge (279fbef)Diff
      size51.6 kB51.4 kB-222 B (-0.4%)
      size-with-ext-deps73.2 kB73.0 kB-222 B (-0.3%)

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/QxO36nh1LG.html

request.phoneEnrollmentInfo.captchaResponse === FAKE_TOKEN
) {
// If reCAPTCHA Enterprise token is FAKE_TOKEN, fetch reCAPTCHA v2 token and inject into request.
if (request.phoneEnrollmentInfo.captchaResponse === FAKE_TOKEN) {

Choose a reason for hiding this comment

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

what is the case that we need to use a fake token?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use fake token when rCE token is missing (MISSING_RECAPTCHA_TOKEN) or reCAPTCHA Enterprise token check fails (INVALID_APP_CREDENTIAL). In audit mode, if the 1st request fails with the above reason, we try again with FAKE_TOKEN and rcv2 token

https://github.com/firebase/firebase-js-sdk/blob/nhienlam-rce/packages/auth/src/platform_browser/recaptcha/recaptcha_enterprise_verifier.ts#L308-L310

Copy link

@Xiaoshouzi-gh Xiaoshouzi-gh left a comment

Choose a reason for hiding this comment

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

LGTM, thank you Ricky!

@NhienLam NhienLam merged commit e2847db into rce-phone Sep 10, 2024
38 of 40 checks passed
@NhienLam NhienLam deleted the nhienlam-rce branch September 10, 2024 22:26
@firebase firebase locked and limited conversation to collaborators Oct 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants