Update SES lockdown by EtDu · Pull Request #10663 · MetaMask/metamask-extension

EtDu · 2021-03-17T02:27:06Z

Update to SES lockdown 0.12.4, which includes new option overrideTaming: 'severe'

github-actions · 2021-03-17T02:27:21Z

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.

…com/facebook/regenerator/pull/411

metamaskbot · 2021-03-23T09:59:34Z

Builds ready [9202e49]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts

Page Load Metrics (604 ± 43 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	43	85	55	11	5
		domContentLoaded	358	810	602	90	43
		load	359	811	604	90	43
		domInteractive	357	810	602	90	43

kumavis

since we're bundling lockdown we can remove the ses dep

package.json

kumavis · 2021-03-23T10:10:54Z

this will help keep develop close to where we are in the lavamoat-for-ui branches

metamaskbot · 2021-03-23T12:45:20Z

Builds ready [d62d37f]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts

Page Load Metrics (677 ± 27 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	45	75	56	8	4
		domContentLoaded	591	796	676	57	28
		load	596	798	677	57	27
		domInteractive	590	796	675	57	28

Gudahtt

Why was the lockdown script vendored, rather than imported? Where specifically did this script come from, and which version is it?

EtDu · 2021-03-24T02:35:55Z

Why was the lockdown script vendored, rather than imported? Where specifically did this script come from, and which version is it?

This script came from building lockdown from SES-Shim's master branch. V0.12.3+1-dev, which hasn't been published yet. We want this pre-release build because it applies a more broad override mistake workaround. (For at least all properties on Object.prototype, according to this doc) via overrideTaming: severe as a lockdown option.

kumavis · 2021-03-24T05:04:18Z

@Gudahtt good question. asking agoric about next version release timeline
but im all for eager adoption and frequent updates since we're actively working on this stuff 👍

kumavis · 2021-03-24T05:30:41Z

ses release "maybe tomorrow" 👍

Gudahtt · 2021-03-24T14:15:48Z

Cool - makes sense 👍

Not against including vendored dependencies, but it would be nice if some context for what the file is was included in the codebase alongside it. A pattern I've seen used before is to create a directory under vendor (e.g. app/vendor/ses/) with a README.txt alongside it explaining what the file is, why it was vendored, and how to re-create it exactly (i.e. where to find it, or how to generate it if it was generated, and which modifications to apply if any). That helps ensure we can safely update it in the future without any surprises.

As an example of where this went wrong, I've briefly considered updating app/scripts/chromereload.js in the past, only to give up because I wasn't confident enough that I understood the specific version we used, so I couldn't find corresponding changelog entries for the update I wanted to make. We've periodically forgotten what that file was altogether, and had to rediscover it by googling portions of it, or by asking you.

It sounds like we'll be updating SES pretty soon so it's likely not to be an issue here, but you never know! We could get stuck on this version if the next update introduces problems. Better safe than sorry, and a couple of lines of text should be easy enough to add anyway.

This reverts commit 8cefdc9.

This reverts commit 30371a3.

kumavis · 2021-03-25T05:26:29Z

hold

This reverts commit 966e4c6.

metamaskbot · 2021-03-25T05:56:04Z

Builds ready [c1f144a]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts

Page Load Metrics (599 ± 45 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	45	70	56	7	3
		domContentLoaded	403	830	598	93	45
		load	405	836	599	94	45
		domInteractive	403	829	598	93	45

kumavis · 2021-03-25T06:32:56Z

need to verify the ses-bug for errors in console is not affected in browser

kumavis · 2021-03-26T01:40:21Z

heres the ses bug in question endojs/endo#636
validated that it does not affect the browser rendering of errors (only node, this ses version is only used in the browser)

Gudahtt · 2021-03-26T03:48:39Z

patches/@reduxjs+toolkit+1.5.0.patch

+   var _proto = MiddlewareArray.prototype;
+
+-  _proto.concat = function concat() {
+  Object.defineProperty(_proto, 'concat', { value: function concat() {


Huh. I don't quite follow what's going on here. Is this an array class that is extending Array and redefining concat?

Could we propose this upstream? They seem to be fairly responsive maintainers.

your understanding is correct. its fine for them to do that (none of my business), but the override mistake doesnt allow them to do it via assignment. thus this.
yes, we usually PR these patches upstream. but we dont let that block us so we write patches as well.

Got it. Using a patch seems fine for now, but having that work started by creating a PR would be great. Then we could hopefully delete the patch on the next update, rather than re-write it each time. Especially for packages like this that are updated relatively frequently.

Gudahtt

LGTM!

I didn't test this yet, but it'll be a while before our next feature release so we'll have plenty of time to find any new SES-related breakages.

update ses

9f9b4f6

EtDu requested a review from a team as a code owner March 17, 2021 02:27

EtDu requested a review from Gudahtt March 17, 2021 02:27

build - reference ses directly

30371a3

EtDu requested a review from kumavis as a code owner March 23, 2021 05:59

kumavis and others added 6 commits March 23, 2021 14:31

deps - unify regenerator-runtime versions on 0.13.7

019e478

patches - apply regenerator-runtime ses compat patch\nhttps://github.…

85ae04e

…com/facebook/regenerator/pull/411

patches - patch regenerator-runtime for latest ses fix

9d7d95f

reduc patch, new lockdown severe override taming

b1e635d

updated redux patch

a05cafa

update redux patch for production

b96bf88

MetaMask deleted a comment from azmain84 Mar 23, 2021

EtDu and others added 3 commits March 23, 2021 17:22

ignore lockdown in lint

8cefdc9

deps - bump patch-package just in case

dbc94ad

trailing comma

9202e49

kumavis requested changes Mar 23, 2021

View reviewed changes

package.json Outdated Show resolved Hide resolved

remove ses as dep

2342a69

kumavis previously approved these changes Mar 23, 2021

View reviewed changes

fix path for frozen promise

966e4c6

EtDu dismissed kumavis’s stale review via 966e4c6 March 23, 2021 11:59

remove js extension in lockdown require

d62d37f

Gudahtt reviewed Mar 23, 2021

View reviewed changes

EtDu changed the title ~~update ses~~ Update SES lockdown Mar 24, 2021

kumavis added 3 commits March 25, 2021 13:05

Revert "ignore lockdown in lint"

5e7fb6b

This reverts commit 8cefdc9.

Revert "build - reference ses directly"

3d1573f

This reverts commit 30371a3.

deps - update ses

6fbd253

Revert "fix path for frozen promise"

c1f144a

This reverts commit 966e4c6.

kumavis mentioned this pull request Mar 26, 2021

Add LavaMoat for the extension webapp #10730

Closed

5 tasks

Gudahtt reviewed Mar 26, 2021

View reviewed changes

Gudahtt approved these changes Mar 26, 2021

View reviewed changes

kumavis merged commit 8fc2c32 into develop Mar 26, 2021

kumavis deleted the ses-update branch March 26, 2021 04:27

github-actions bot locked and limited conversation to collaborators Mar 26, 2021

Conversation

EtDu commented Mar 17, 2021 • edited by kumavis Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Mar 17, 2021

Uh oh!

metamaskbot commented Mar 23, 2021

Uh oh!

kumavis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kumavis commented Mar 23, 2021

Uh oh!

metamaskbot commented Mar 23, 2021

Uh oh!

Gudahtt left a comment

Choose a reason for hiding this comment

Uh oh!

EtDu commented Mar 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kumavis commented Mar 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kumavis commented Mar 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Gudahtt commented Mar 24, 2021

Uh oh!

kumavis commented Mar 25, 2021

Uh oh!

metamaskbot commented Mar 25, 2021

Uh oh!

kumavis commented Mar 25, 2021

Uh oh!

kumavis commented Mar 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Gudahtt Mar 26, 2021

Choose a reason for hiding this comment

Uh oh!

kumavis Mar 26, 2021

Choose a reason for hiding this comment

Uh oh!

Gudahtt Mar 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Gudahtt left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

EtDu commented Mar 17, 2021 •

edited by kumavis

Loading

EtDu commented Mar 24, 2021 •

edited

Loading

kumavis commented Mar 24, 2021 •

edited

Loading

kumavis commented Mar 24, 2021 •

edited

Loading

kumavis commented Mar 26, 2021 •

edited

Loading

Gudahtt Mar 26, 2021 •

edited

Loading