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

Catch all recaptcha errors #7203

Merged
merged 7 commits into from
Apr 12, 2023
Merged

Catch all recaptcha errors #7203

merged 7 commits into from
Apr 12, 2023

Conversation

hsubox76
Copy link
Contributor

@hsubox76 hsubox76 commented Apr 6, 2023

We were catching local errors in recaptcha by catching the result of grecaptcha.execute. It turns out that not all recaptcha errors result in recaptcha.execute throwing. Errors can also be caught in error-callback params passed to grecaptcha.render. See https://developers.google.com/recaptcha/docs/invisible for parameters accepted by grecaptcha.render.

These callbacks will set a boolean variable in our AppCheck state object, which will be checked in provider.getToken() methods for the two reCAPTCHA providers, and provider.getToken() will throw with a RECAPTCHA_ERROR if false. This will prevent the main getToken() in internal-api.ts from going on to call the exchange endpoint, since we already know we don't have a usable reCAPTCHA token to send. This will then prevent a 503 error caused by sending a bad token, which will prevent a 1 day backoff being imposed.

This means that a device that ran into a reCAPTCHA error while asleep, or in a background tab, or some other temporary reason, won't be locked off from retrying on the next request.

This may help with errors such as:
#7116
and
#6708

@changeset-bot
Copy link

changeset-bot bot commented Apr 6, 2023

🦋 Changeset detected

Latest commit: 7ff8cb9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@firebase/app-check Patch
@firebase/app-check-compat Patch
firebase Patch

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

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Apr 6, 2023

Size Report 1

Affected Products

  • @firebase/app-check

    TypeBase (ea3849f)Merge (2de6e2f)Diff
    browser25.2 kB25.7 kB+435 B (+1.7%)
    esm530.3 kB30.7 kB+447 B (+1.5%)
    main31.3 kB31.8 kB+447 B (+1.4%)
    module25.2 kB25.7 kB+435 B (+1.7%)
  • bundle

    TypeBase (ea3849f)Merge (2de6e2f)Diff
    app-check (ReCaptchaEnterpriseProvider)38.5 kB38.8 kB+226 B (+0.6%)
    app-check (ReCaptchaV3Provider)38.5 kB38.7 kB+226 B (+0.6%)
  • firebase

    TypeBase (ea3849f)Merge (2de6e2f)Diff
    firebase-app-check-compat.js22.7 kB23.1 kB+320 B (+1.4%)
    firebase-app-check.js21.5 kB21.8 kB+328 B (+1.5%)
    firebase-compat.js751 kB752 kB+326 B (+0.0%)

Test Logs

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

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Apr 6, 2023

Size Analysis Report 1

Affected Products

  • @firebase/app-check

    • ReCaptchaEnterpriseProvider

      Size

      TypeBase (ea3849f)Merge (2de6e2f)Diff
      size11.0 kB11.2 kB+222 B (+2.0%)
      size-with-ext-deps28.5 kB28.7 kB+226 B (+0.8%)
    • ReCaptchaV3Provider

      Size

      TypeBase (ea3849f)Merge (2de6e2f)Diff
      size10.9 kB11.2 kB+222 B (+2.0%)
      size-with-ext-deps28.4 kB28.7 kB+226 B (+0.8%)

Test Logs

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

@hsubox76 hsubox76 marked this pull request as ready for review April 11, 2023 18:59
Copy link
Contributor

@dwyfrequency dwyfrequency left a comment

Choose a reason for hiding this comment

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

LGTM great work on this

provider: new ReCaptchaV3Provider(FAKE_SITE_KEY)
});

const reCAPTCHASpy = stubGetRecaptchaToken('', false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the first parameter here, the token parameter, be related to formatDummyToken(defaultTokenErrorData)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, that's the recaptcha token. The flow in App Check (when using recaptcha, the default attestation provider) is

  1. The client requests a recaptcha token
  2. recaptcha returns a recaptcha token
  3. The client takes the recaptcha token and sends it to the App Check exchange endpoint
  4. The App Check exchange endpoint returns an App Check token

In this test, step 3 is being interrupted, the recaptcha token was no good (it was '' as stubbed here), so it won't bother to send anything to the exchange endpoint and will just create a dummy App Check token locally, which is returned by getToken() (the function that covers the whole exchange logic).

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahhh. thanks so much for the detailed response!

@@ -143,11 +143,19 @@ function renderInvisibleWidget(
grecaptcha: GreCAPTCHA,
container: string
): void {
const state = getStateReference(app);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's referenced in 160. Apparently it used to be under the widgetId block, I should put it back, it's weird here.

@hsubox76 hsubox76 merged commit 8c44d58 into master Apr 12, 2023
@hsubox76 hsubox76 deleted the ch-ac-recaptcha branch April 12, 2023 19:24
@google-oss-bot google-oss-bot mentioned this pull request Apr 13, 2023
@firebase firebase locked and limited conversation to collaborators May 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants