Enable context isolation and disable nodejs support. Fixes #2018 #12299

tsmaeder · 2023-03-14T16:22:06Z

What it does

All access to electron API is now done through an API exposed via a preload script. Access to the electron API (including electron-remote) and nodejs API is no longer possible.
Theia extensions can contribute to the preload script via a theiaExtensions module declaration in their package.json

Fixes #2018

Contributed on behalf or STMicroelectronics

How to test

Run the electron version of Theia and make sure nothing is broken. Particular areas of interest is anything to do with menus and windows, for example "zoom in/out", "maximize", etc.

CQ

Review checklist

As an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

As a reviewer, I agree to behave in accordance with the review guidelines

tsmaeder · 2023-03-14T16:23:08Z

I'm opening a draft PR because I'll be working on updating to a newer electron version, as well. I'd be interested in feedback and testing, though.

paul-marechal

Quick first review pass. Note that the CI doesn't look happy.

packages/core/src/electron-browser/menu/electron-main-menu-factory.ts

packages/filesystem/src/electron-browser/preload.ts

tsmaeder · 2023-03-16T10:31:52Z

The license check shows npm/npmjs/@babel/plugin-transform-async-to-generator/7.20.7 to be "under review", but if you looka the issue, it's approved. What gives? https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/6230

tsmaeder · 2023-03-16T10:35:00Z

One of the linter issues is this:

packages/debug/src/browser/debug-variables-source-tree.ts(46,11): error TS4114: This member must have an 'override' modifier because it overrides a member in the base class 'SourceTree'.

But that's not even part of this PR. What gives?

vince-fugnitto · 2023-03-16T14:20:47Z

The license check shows npm/npmjs/@babel/plugin-transform-async-to-generator/7.20.7 to be "under review", but if you looka the issue, it's approved. What gives? https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/6230

@tsmaeder I don't believe these are errors, only electron is:

https://github.com/eclipse-theia/theia/actions/runs/4436118782/jobs/7784138808?pr=12299#step:5:348

The other entries were added to our baseline at the time but can likely be cleaned up as they are approved:

theia/dependency-check-baseline.json

Lines 1 to 14 in f8654ec

    
           { 
        
             "npm/npmjs/-/eslint-plugin-deprecation/1.2.1": "Approved as 'works-with': https://dev.eclipse.org/ipzilla/show_bug.cgi?id=22573", 
        
             "npm/npmjs/-/jschardet/2.3.0": "Approved for Eclipse Theia: https://dev.eclipse.org/ipzilla/show_bug.cgi?id=22481", 
        
             "npm/npmjs/-/jsdom/11.12.0": "Approved as 'works-with': https://dev.eclipse.org/ipzilla/show_bug.cgi?id=23640https://dev.eclipse.org/ipzilla/show_bug.cgi?id=23640", 
        
             "npm/npmjs/-/lzma-native/8.0.6": "Approved as 'works-with': https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/1850", 
        
             "npm/npmjs/-/playwright-core/1.22.2": "Approved as 'works-with': https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/2734", 
        
             "npm/npmjs/@babel/helper-compilation-targets/7.20.7": "Under review: https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/5989", 
        
             "npm/npmjs/@babel/helper-member-expression-to-functions/7.20.7": "Under review: https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/5986", 
        
             "npm/npmjs/@babel/plugin-bugfix-v8-spread-parameters-in-optional-chaining/7.20.7": "Under review: https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/5987", 
        
             "npm/npmjs/@babel/plugin-transform-arrow-functions/7.20.7": "Under review: https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/5990", 
        
             "npm/npmjs/@babel/plugin-transform-async-to-generator/7.20.7": "Under review: https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/6230", 
        
             "npm/npmjs/@babel/plugin-transform-computed-properties/7.20.7": "Under review: https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/5984", 
        
             "npm/npmjs/@babel/template/7.20.7": "Under review: https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/5988" 
        
           }

Please also note that bumping electron requires a pretty significant CQ with the foundation.

tsmaeder · 2023-03-16T14:24:24Z

@vince-fugnitto thanks for chiming in. Is there any documentation on what a "baseline" is and how to resolve issues with this check?

marcdumais-work · 2023-03-16T14:27:41Z

I think the component failing the IP check is the new version of Electron:
X npm/npmjs/-/electron/22.3.2,

Please also note that bumping electron requires a pretty significant CQ with the foundation.

Indeed, in part because Electron bundles Chromium which bundles ffmpeg, a component that can have GPL content and/or proprietary codecs, depending on how it's built.

@vince-fugnitto can you point-out a past example of such CQ IP Check? I get no result when searching on EF Gitlab. (Wayne would really like us to stop the term CQ)

vince-fugnitto · 2023-03-16T14:31:55Z

@vince-fugnitto can you point-out a past example of such CQ IP Check? I get no result when searching on EF Gitlab. (Wayne would really like us to stop the term CQ)

Here is a past "IP Due Diligence Check" (CQ) for electron when we bumped to 15.1.2:

Here is an example of using the new GitLab repo to submit a CQ: https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/5022.

vince-fugnitto · 2023-03-16T14:37:17Z

@vince-fugnitto thanks for chiming in. Is there any documentation on what a "baseline" is and how to resolve issues with this check?

There is no documentation afaik that details the processes but we can look towards updating https://github.com/eclipse-theia/theia/wiki/Registering-CQs.

The baseline is used to suppress errors from dash-licenses when the foundation has specifically approved a dependency for our project, or as works-with. For actual license incompatibilities our CI will perform an automatic review of a dependency to https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab where we need to wait for the dependency to be checked and approved for us to use.

tsmaeder · 2023-03-17T08:56:38Z

FYI: I got the dash-licenses check to pass by poking at clearlydefined to harvest data for the electron versions in question. Not that I know what I'm doing 🤷

vince-fugnitto · 2023-03-20T12:30:27Z

@tsmaeder do you need assistance with the CQ, or would like me to handle it?

tsmaeder · 2023-03-20T13:18:58Z

@vince-fugnitto thanks for the help. While I would like to get this PR merged, the most important outcome for me is is that we clarify and document the process. The documentation we have is woefully out of date. There's also the fact that I can make the PR check pass by fiddling with clearlydefined. And yet, we still seem to need a CQ? Is our PR check wrong? Do you have additional info that says we still need a CQ?

vince-fugnitto · 2023-03-20T13:42:27Z

Do you have additional info that says we still need a CQ?

It is required due to Electron distributing ffmpeg which we both file a CQ for and have @theia/ffmpeg which performs a replacement to only include non-proprietary codecs.

https://github.com/eclipse-theia/theia/blob/master/NOTICE.md#electron

The foundation has requested in the past that we submit requests to be able to use Electron in the project.
cc @marcdumais-work

tsmaeder · 2023-03-21T15:56:23Z

@paul-marechal you mentioned you would prefer using dependency injection for some of the API. Could you sketch what would do differently?

paul-marechal · 2023-03-21T16:00:33Z

Instead of creating hardcoded APIs and exposing them all in the global scope, we'd do like any other entry point and load various Inversify container modules. Doing this in the preload context was easy, but then it gets tricky when trying to expose the things bound there to the browser window.

Ultimately the idea is to go from referring to preload services as window.whateverGlobalVariable to be able to use the @inject(PreloadComponentApi) pattern.

This means that downstream app makers will be able to override services/apis/etc just like everywhere else. With this PR in its current state it's not possible to change much.

tsmaeder · 2023-03-21T16:16:18Z

@paul-marechal that's sounds interesting, but it's not really in scope for this PR, IMO. It sounds like a complete rewrite of the code. I would much rather focus on getting this one merged and to proceed from there.

tsmaeder · 2023-03-21T16:23:50Z

In German, there is a saying: "the better is the enemy of the good" ;-)

tsmaeder force-pushed the 2018_context_isolation branch 2 times, most recently from 0fab8d5 to 4d6e063 Compare March 15, 2023 09:12

tsmaeder marked this pull request as ready for review March 15, 2023 15:27

tsmaeder requested review from colin-grant-work, msujew and paul-marechal and removed request for msujew March 15, 2023 15:27

tsmaeder force-pushed the 2018_context_isolation branch from 9004ac5 to 8cfb331 Compare March 15, 2023 15:47

paul-marechal reviewed Mar 15, 2023

View reviewed changes

packages/core/src/electron-browser/menu/electron-main-menu-factory.ts Show resolved Hide resolved

packages/filesystem/src/electron-browser/preload.ts Outdated Show resolved Hide resolved

tsmaeder force-pushed the 2018_context_isolation branch from 8cfb331 to c9f5860 Compare March 16, 2023 10:39

vince-fugnitto added electron issues related to the electron target CQ Required (deprecated) issues requiring a CQ (contributor questionnaire) labels Mar 16, 2023

tsmaeder force-pushed the 2018_context_isolation branch from c9f5860 to bcb49ab Compare March 21, 2023 16:24

This was referenced Dec 21, 2023

[Snyk] Fix for 6 vulnerabilities magnologan/theia#63

Open

[Snyk] Fix for 6 vulnerabilities magnologan/theia#64

Open

[Snyk] Fix for 6 vulnerabilities magnologan/theia#65

Open

[Snyk] Fix for 6 vulnerabilities magnologan/theia#66

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable context isolation and disable nodejs support. Fixes #2018 #12299

Enable context isolation and disable nodejs support. Fixes #2018 #12299

tsmaeder commented Mar 14, 2023 •

edited

Loading

tsmaeder commented Mar 14, 2023

paul-marechal left a comment

tsmaeder commented Mar 16, 2023

tsmaeder commented Mar 16, 2023 •

edited

Loading

vince-fugnitto commented Mar 16, 2023 •

edited

Loading

tsmaeder commented Mar 16, 2023

marcdumais-work commented Mar 16, 2023 •

edited

Loading

vince-fugnitto commented Mar 16, 2023 •

edited

Loading

vince-fugnitto commented Mar 16, 2023

tsmaeder commented Mar 17, 2023

vince-fugnitto commented Mar 20, 2023

tsmaeder commented Mar 20, 2023 •

edited

Loading

vince-fugnitto commented Mar 20, 2023

tsmaeder commented Mar 21, 2023

paul-marechal commented Mar 21, 2023

tsmaeder commented Mar 21, 2023 •

edited

Loading

tsmaeder commented Mar 21, 2023

Enable context isolation and disable nodejs support. Fixes #2018 #12299

Enable context isolation and disable nodejs support. Fixes #2018 #12299

Conversation

tsmaeder commented Mar 14, 2023 • edited Loading

What it does

How to test

CQ

Review checklist

Reminder for reviewers

tsmaeder commented Mar 14, 2023

paul-marechal left a comment

Choose a reason for hiding this comment

tsmaeder commented Mar 16, 2023

tsmaeder commented Mar 16, 2023 • edited Loading

vince-fugnitto commented Mar 16, 2023 • edited Loading

tsmaeder commented Mar 16, 2023

marcdumais-work commented Mar 16, 2023 • edited Loading

vince-fugnitto commented Mar 16, 2023 • edited Loading

vince-fugnitto commented Mar 16, 2023

tsmaeder commented Mar 17, 2023

vince-fugnitto commented Mar 20, 2023

tsmaeder commented Mar 20, 2023 • edited Loading

vince-fugnitto commented Mar 20, 2023

tsmaeder commented Mar 21, 2023

paul-marechal commented Mar 21, 2023

tsmaeder commented Mar 21, 2023 • edited Loading

tsmaeder commented Mar 21, 2023

tsmaeder commented Mar 14, 2023 •

edited

Loading

tsmaeder commented Mar 16, 2023 •

edited

Loading

vince-fugnitto commented Mar 16, 2023 •

edited

Loading

marcdumais-work commented Mar 16, 2023 •

edited

Loading

vince-fugnitto commented Mar 16, 2023 •

edited

Loading

tsmaeder commented Mar 20, 2023 •

edited

Loading

tsmaeder commented Mar 21, 2023 •

edited

Loading