Skip to content

Fix SES lockdown on older browsers#10014

Merged
Gudahtt merged 1 commit intodevelopfrom
fix-lockdown-on-older-browsers
Dec 8, 2020
Merged

Fix SES lockdown on older browsers#10014
Gudahtt merged 1 commit intodevelopfrom
fix-lockdown-on-older-browsers

Conversation

@Gudahtt
Copy link
Copy Markdown
Member

@Gudahtt Gudahtt commented Dec 7, 2020

On older browsers that don't support globalThis, 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.

Manual testing steps:

  • Load each page
  • See whether the error globalThis is not defined is in the dev console

Four of our pages use the lockdown script, so are affected by this: home.html, notification.html, phishing.html, and popup.html. The contentscript environment and the background process are also affected.

@Gudahtt
Copy link
Copy Markdown
Member Author

Gudahtt commented Dec 7, 2020 •
edited
Loading

This depends upon #10013 Now ready for review

@Gudahtt Gudahtt force-pushed the fix-lockdown-on-older-browsers branch from b3cc227 to db7d65f Compare December 7, 2020 22:14
Base automatically changed from add-sentry-and-ses-lockdown-to-all-pages to develop December 7, 2020 22:16
@Gudahtt Gudahtt force-pushed the fix-lockdown-on-older-browsers branch from db7d65f to f3c0b67 Compare December 7, 2020 22:16
@Gudahtt Gudahtt marked this pull request as ready for review December 7, 2020 22:16
@Gudahtt Gudahtt requested review from a team and kumavis as code owners December 7, 2020 22:16
rekmarks
rekmarks previously approved these changes Dec 7, 2020
View reviewed changes
@Gudahtt
Copy link
Copy Markdown
Member Author

Gudahtt commented Dec 7, 2020

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.

@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Dec 7, 2020

Builds ready [f3c0b67]
Page Load Metrics (541 ± 44 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint326751105
domContentLoaded2836775409244
load2846785419244
domInteractive2826775399244

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Dec 8, 2020

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.

@EtDu
Copy link
Copy Markdown
Contributor

EtDu commented Dec 8, 2020 •
edited
Loading

I believe simply adding if (typeof globalThis === 'undefined') self.globalThis = self before lockdown is sufficient, without using another dep.

@Gudahtt
Copy link
Copy Markdown
Member Author

Gudahtt commented Dec 8, 2020 •
edited
Loading

@EtDu That does not work. The failure occurs in lockdown.cjs, before runLockdown.js is run. The global variable lockdown will now not be defined.

Is there a reason you think we should avoid using globalthis? It's the officially maintained shim, and we already had it in our deps (indirectly). Pulling that in ensures we can leave lockdown.cjs unmodified as well.

@Gudahtt
Fix SES lockdown on older browsers
75af7c1 
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
@Gudahtt Gudahtt force-pushed the fix-lockdown-on-older-browsers branch from 3c1a2d4 to 75af7c1 Compare December 8, 2020 14:31
@Gudahtt
Copy link
Copy Markdown
Member Author

Gudahtt commented Dec 8, 2020

I have restored the original commit and rebased it.

@sentry
Copy link
Copy Markdown

sentry bot commented Dec 8, 2020

Sentry issue: METAMASK-GP0Y

@sentry
Copy link
Copy Markdown

sentry bot commented Dec 8, 2020

Sentry issue: METAMASK-GP0X

@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Dec 8, 2020

Builds ready [75af7c1]
Page Load Metrics (546 ± 28 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint32584973
domContentLoaded3316245455828
load3326255465828
domInteractive3316245445828

@Gudahtt
Copy link
Copy Markdown
Member Author

Gudahtt commented Dec 8, 2020

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.

@Gudahtt
Copy link
Copy Markdown
Member Author

Gudahtt commented Dec 8, 2020

It looks like Chromium v60 gets past the Object rest/spread issue, but still blows up when it gets to function* (despite the MDN article claiming that it supports generators 🤔). Chromium v62 blows up as well. I needed to update to Chromium v63 to get past the generator error. Then everything on this branch seemed to work.

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.

@Gudahtt Gudahtt mentioned this pull request Dec 8, 2020
Open
5 tasks
@Gudahtt
Copy link
Copy Markdown
Member Author

Gudahtt commented Dec 8, 2020

When testing with Waterfox Classic (should be API equivalent to our minimum Firefox version), I see this error from lockdown.cjs: Error: function "toFormat" not found

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 toLocaleFormat, and trying to map it to toFormat, which doesn't exist. This is only a problem on older browsers because toLocaleFormat was removed in later Firefox versions.

@Gudahtt
Copy link
Copy Markdown
Member Author

Gudahtt commented Dec 8, 2020

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.

@Gudahtt Gudahtt merged commit d13aabd into develop Dec 8, 2020
@Gudahtt Gudahtt deleted the fix-lockdown-on-older-browsers branch December 8, 2020 18:38
@github-actions github-actions bot locked and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@rekmarks rekmarks rekmarks approved these changes

@kumavis kumavis Awaiting requested review from kumavis kumavis is a code owner

@NiranjanaBinoy NiranjanaBinoy Awaiting requested review from NiranjanaBinoy

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Gudahtt @metamaskbot @EtDu @rekmarks