Skip to content

Add SES lockdown to extension webapp#9729

Merged
kumavis merged 52 commits intodevelopfrom
ses-lockdown-background
Nov 24, 2020
Merged

Add SES lockdown to extension webapp#9729
kumavis merged 52 commits intodevelopfrom
ses-lockdown-background

Conversation

@EtDu
Copy link
Copy Markdown
Contributor

@EtDu EtDu commented Oct 27, 2020 •
edited by kumavis
Loading

Adds lockdown() to background, UI, contentscript & phishing-detect (all contexts except in-page)

SES + Sentry integration

Sentry integrates itself by overriding some intrinsics like Function.prototype.toString and platform apis like console. We carefully install only Sentry before performing the lockdown. Then start the app normally.

SES compat via patch-package

  • symbol-observable modifies Symbol intrinsic (pattern used by multiple packages, in our case Redux). Once we get these upstream changes merged, we can remove the patch
    • PR made in redux (merged, awaiting publish)
    • PR made in redux-devtools-instrument (merged, updated in remote-redux-devtools)
    • PR made in reactiveX (not yet merged)

Summary of Changes:

  • more mem for box (medium+)
  • more "max old mem size" to 2gb
  • add patch-package for workaround with symbol-observable@1
  • run sentry and lockdown before app init
  • new bundle for setting up sentry
  • replace envify with loose-envify for parsing error workaround
  • add METAMASK_VERSION env var
  • pass down process.env to child processes
  • log error code if child process fails
  • remove lavamoat-core, use ses

@EtDu EtDu requested review from a team and kumavis as code owners October 27, 2020 13:12
@EtDu EtDu marked this pull request as draft October 27, 2020 13:12
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Oct 27, 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 EtDu force-pushed the ses-lockdown-background branch 2 times, most recently from b36718f to 6cc4424 Compare October 29, 2020 14:41
@EtDu EtDu changed the title Add SES lockdown to background & UI Add SES lockdown to background, UI & contentscript Nov 2, 2020
@EtDu EtDu force-pushed the ses-lockdown-background branch from c8cc1c0 to 340d54c Compare November 3, 2020 03:47
@EtDu EtDu changed the title Add SES lockdown to background, UI & contentscript Add SES lockdown to background, UI, contentscript & phishing-detect Nov 3, 2020
@EtDu EtDu force-pushed the ses-lockdown-background branch from 7f6a7b7 to 791917b Compare November 3, 2020 08:45
@EtDu EtDu marked this pull request as ready for review November 4, 2020 09:29
kumavis
kumavis reviewed Nov 4, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@kumavis kumavis left a comment

Choose a reason for hiding this comment

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

I think this will work for our purposes but is not valid esm (running code between import statements). To make it work we should run the lockdown as part of the module initialization. If we don't have a great way to add the context (ui, etc) to the lockdown we can create lockdowns for each context that hard code it 😐

@EtDu EtDu force-pushed the ses-lockdown-background branch from 1023085 to 5f7d446 Compare November 9, 2020 02:34
@kumavis kumavis changed the title Add SES lockdown to background, UI, contentscript & phishing-detect Add SES lockdown to extension webapp Nov 10, 2020
@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Nov 11, 2020

Builds ready [b91bb87]
Page Load Metrics (452 ± 40 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint2991672110
domContentLoaded2565344508240
load2575364528240
domInteractive2555344508240

@EtDu EtDu force-pushed the ses-lockdown-background branch from b91bb87 to 2919fce Compare November 11, 2020 05:09
@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Nov 11, 2020

Builds ready [2919fce]
Page Load Metrics (485 ± 67 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint3196572010
domContentLoaded26972648313967
load27173148513967
domInteractive26972648313967

@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Nov 12, 2020

Builds ready [220e180]
Page Load Metrics (407 ± 54 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint3591592110
domContentLoaded26864340511254
load26964440711254
domInteractive26864340511254

@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Nov 13, 2020

Builds ready [2c0627d]
Page Load Metrics (575 ± 99 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaintNaNNaNNaNNaNNaN
domContentLoaded345108957320699
load347109057520699
domInteractive345108957220699

@EtDu EtDu force-pushed the ses-lockdown-background branch from 2c0627d to 3557632 Compare November 13, 2020 11:44
@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Nov 16, 2020

Builds ready [2cf0a84]
Page Load Metrics (495 ± 70 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint3197632411
domContentLoaded32289649414570
load32389849514570
domInteractive32189649314570

@EtDu EtDu force-pushed the ses-lockdown-background branch from 2cf0a84 to 9a577d1 Compare November 16, 2020 02:22
@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Nov 16, 2020

Builds ready [9a577d1]
Page Load Metrics (432 ± 46 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint31114632613
domContentLoaded2965924319546
load2975944329546
domInteractive2955924309546

Gudahtt
Gudahtt reviewed Nov 16, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

That seems like a clever way to keep Sentry working! Though I'm surprised this worked - I would have expected the lockdown to "undo" global mutations as well as preventing new ones. Though I don't know - maybe it doesn't work like that. We can test this either way with real Sentry events, targeting our test Sentry project.

I wonder if there is some way to operate Sentry in a reduced capacity as well, such that global mutations are not necessary 🤔

@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Nov 17, 2020

Builds ready [2328351]
Page Load Metrics (466 ± 55 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint3197592311
domContentLoaded29969446411455
load30169546611455
domInteractive29969446411455

@kumavis
Copy link
Copy Markdown
Member

kumavis commented Nov 19, 2020

CI is listing a pending check (test-deps) but it in fact passed

@Gudahtt
Copy link
Copy Markdown
Member

Gudahtt commented Nov 20, 2020 •
edited
Loading

Since there is a working upstream version of symbol-observable, we can use a Yarn resolution to update transitive dependencies instead of a patch.

e.g. in the resolutions field of package.json:

    "**/symbol-observable": "^2.0.3",

We still need patch-package in cases where there is no upstream solution at all, but I think using resolutions to force-update transitive deps is a less fragile approach than patching.

Edit: I've opened a PR against this branch with this change - let me know what you think, and/or feel free to merge!
#9923

@Gudahtt
Copy link
Copy Markdown
Member

Gudahtt commented Nov 20, 2020

I have tested this locally, and have confirmed that Sentry still works correctly! The error was rate-limited so I didn't see it in the dashboard, but the network request looked correct to me.

@Gudahtt
Replace patch with Yarn resolution (#9923)
87c1c7a 
Instead of patching `symbol-observable`, this ensures that all
versions of `symbol-observable` are resolved to the given range, even
if it contradicts the requested range.
@kumavis kumavis dismissed a stale review via 87c1c7a November 21, 2020 02:40
@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Nov 21, 2020

Builds ready [87c1c7a]
Page Load Metrics (481 ± 67 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint2998612411
domContentLoaded31081748013967
load31181848113967
domInteractive30981648013967

@kumavis kumavis merged commit 9f6fa64 into develop Nov 24, 2020
@kumavis kumavis deleted the ses-lockdown-background branch November 24, 2020 03:26
@github-actions github-actions bot locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@kumavis kumavis kumavis left review comments

@Gudahtt Gudahtt Gudahtt approved these changes

Assignees

@kumavis kumavis

@Gudahtt Gudahtt

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@EtDu @metamaskbot @kumavis @Gudahtt