Check for non-Enterprise recaptcha object #6421
Conversation
hsubox76
requested review from
avolkovi,
lisajian,
sam-gc and
yuchenshi
as code owners
|
Size Report 1Affected Products
Test Logs |
Size Analysis Report 1Affected Products
Test Logs |
| @@ -112,7 +117,7 @@ export class ReCaptchaLoaderImpl implements ReCaptchaLoader { | |||
| // In cases (2) and (3), we _can't_ reload as it would break the recaptchas | |||
| // that are already in the page | |||
| return ( | |||
| !!_window().grecaptcha && | |||
| !!_window().grecaptcha?.render && | |||
There was a problem hiding this comment.
Sorry for the late comment - !!_window().grecaptcha?.render can be replaced with librarySeparatelyLoaded, so this becomes:
return (this.librarySeparatelyLoaded && (hl === this.hostLanguage || this.counter > 0 || this.librarySeparatelyLoaded)
Can we just rewrite this as:
return (hl === this.hostLanguage || this.counter > 0 || this.librarySeparatelyLoaded)?
There was a problem hiding this comment.
See PR description, these 2 expressions were previously redundant as well before this code change, so I thought about fixing it but I didn't know the original intent, and if one of them was originally intended to be a different condition that also needed to be checked. I wanted to leave it up to someone more familiar with the code.
Auth's
ReCaptchaLoaderImpl.load()method checks for if the ReCAPTCHA script tag has already been loaded by itself or another library, and does not load it again if so. It does this by checking for the global objectwindow.grecaptcha. Unfortunately, this object will exist if the ReCAPTCHA Enterprise version of the script has been loaded (which may be done by App Check if the user is using ReCAPTCHA Enterprise with App Check), BUT it will only contain theenterpriseproperty, which is a nested version of the regular ReCAPTCHA object.This change has the
load()method check instead for the existence ofwindow.grecaptcha.render. If it is not found, it will go ahead and load the non-Enterprise script tag, which seems to be able to co-exist with Enterprise and just adds the render/execute/etc methods onto the top level of the object without removing theenterpriseproperty.In the future, a more robust fix could be to extract a shared recaptcha loader to be used by Auth, App Check, and any other library that needs it.
One interesting note: the condition in
shouldResolveImmediately()is kind of odd, in that it will always resolve ifwindow.grecaptcha.renderexists, regardless of the other 2 conditions, sincethis.librarySeparatelyLoadedis just also!!window.grecaptcha.render. But in the interest of a quick fix and breaking as little as possible, I will leave it be for now.Fixes #6133