-
Notifications
You must be signed in to change notification settings - Fork 5k
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
Fix SES lockdown on older browsers #10014
Conversation
|
b3cc227
to
db7d65f
Compare
db7d65f
to
f3c0b67
Compare
I wasn't able to test this, as I don't have any older browsers installed. I can try later on my Windows machine (using old "portable" executable). We support Chrome >= 58' and Firefox >= 56.2 (see here), so testing something close-ish to those minimums would be idea. |
Builds ready [f3c0b67]
Page Load Metrics (541 ± 44 ms)
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
I believe simply adding |
@EtDu That does not work. The failure occurs in Is there a reason you think we should avoid using |
On older browsers that don't support `globalThis`[1], the SES lockdown throws an error. The `globalthis` shim has been added to all pages, to the background process, and to the `contentscript`. This should prevent the error on older browsers. [1]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/globalThis#Browser_compatibility
3c1a2d4
to
75af7c1
Compare
I have restored the original commit and rebased it. |
Sentry issue: METAMASK-GP0Y |
Sentry issue: METAMASK-GP0X |
Builds ready [75af7c1]
Page Load Metrics (546 ± 28 ms)
|
I tried testing this on Chrome v58, and ran into an issue with Object rest/spread 😞. So this still isn't compatible with our minimum documented Chrome version at least. |
It looks like Chromium v60 gets past the Object rest/spread issue, but still blows up when it gets to Maybe we can bump the minimum Chromium version up to v63 🤔. We were already planning to bump it up to v60 at least. I'll take a look at our metrics again. |
When testing with Waterfox Classic (should be API equivalent to our minimum Firefox version), I see this error from It looks like it's converting any methods it finds on the Date prototype with the string "Locale" to an equivalent method without "Locale". So it's seeing |
I've tested this in Firefox v68 (the previous Extended Support Release), and this seems to work correctly. I think we might just have to let the lockdown fail on Waterfox Classic for now. At least it doesn't seem to have any user-facing impact - it's just the lockdown that fails. |
On older browsers that don't support
globalThis
, the SES lockdown throws an error. Theglobalthis
shim has been added to all pages, to the background process, and to thecontentscript
. This should prevent the error on older browsers.Manual testing steps:
globalThis is not defined
is in the dev consoleFour of our pages use the lockdown script, so are affected by this:
home.html
,notification.html
,phishing.html
, andpopup.html
. Thecontentscript
environment and the background process are also affected.