Consider Electron version and update checks according to new defaults (#23, #58) #66

baltpeter · 2020-05-23T11:36:54Z

Me again. :D This PR fixes two related issues (#23 and #58):

The checkers now optionally get passed the Electron version. This allows accounting for changed defaults.
For those instances where the defaults have changed, I have updated the checks.

The commit messages go a little more in-depth on the concrete changes I implemented. Let me know if you would prefer for anything to be implemented differently.

If the directory and ASAR loaders find a package.json file, they now check if there is a dependency on `electron` and if so, remember its version number. That version number is passed to the checkers' `match()` functions. I modified the `electron.asar` in the tests folder to include a minimal package.json file, containing: ``` { "devDependencies": { "electron": "6.1.11" } } ``` The `test_loader.js` test now also tests if this version is read correctly.

This simplifies the checker code and maintains the condition to run all checks if no version can be detected.

phosphore · 2020-05-25T13:12:20Z

Hello @baltpeter and thanks for the contribution (again!).
We would love if you could tackle this using #58 (comment) design proposal instead, since things may get ugly soon for every check making version-based decisions as more and more Electron versions are released.
Let us know what you think!

baltpeter · 2020-05-25T21:39:51Z

I first had a pretty similar idea to what you described in #58 in mind but decided to go with this easier approach first as it is enough for my use case. I'll give implementing your design proposal a go!

baltpeter · 2020-06-29T12:50:14Z

Alright, here is my go at this. Took a little longer than I would have liked as I am fairly busy right now, so sorry about that.

A few notes about the new implementation:

I have currently built the defaults.json file manually using Electron Fiddle and dumping the web prefs via: console.log(JSON.stringify(mainWindow.webContents.getWebPreferences(), null, 4))

getWebPreferences() is an undocumented method of the webContents that seems well-suited here. It's the only somewhat automated way I've found that doesn't involve copying all the defaults by hand from the documentation.

I didn't find an easy way to automate this completely but if you want to do so in the future, this is where the defaults seem to be set: https://github.com/electron/electron/blob/16a3f41fd39cee412fee8f4375ddc834351cf152/shell/browser/web_contents_preferences.cc#L104
The structure of the defaults.json is currently very simple. It contains an entry for every time the defaults have changed with the full web prefs dump. You could of course also just save the diff to the last entry but that would complicate the code a little.
I have implemented the option to override the detected version. I have not implemented reading from the lock files and unfortunately don't currently have the time to do that. If someone else wants to implement that, feel free. Otherwise, I think the current state is still an improvement as previously almost all cases assumed the wrong version and now only some do.
The context isolation change that is documented here doesn't actually seem to have happened. I have asked about this on their issue tracker. Do you happen to know anything about that?
The getWebPreferences() function doesn't seem to dump all possible web prefs, notably enableRemoteModule is not included, even though the default for that will be changed in 10.0.0.

This particular setting is also quite weird. It is referenced as a 'hidden' pref in some places in the code and they always seem to manually implement the default like here which seems like a bad idea to me but anyway. Not sure how to handle this to be honest.
As a result of the last two points, only the default change regarding node integration remains. Let me know if you know of any I've missed.
I am still passing the Electron version to the checkers in case that might be useful in the future.

phosphore

Amazing work! I'm inclined to approve this without many changes. See if you have the time to address these, otherwise this weekend I'll try it to push them myself. A few answers inline:

I have currently built the defaults.json file manually using Electron Fiddle and dumping the web prefs via getWebPreferences()
I have not implemented reading from the lock files and unfortunately don't currently have the time to do that. If someone else wants to implement that, feel free. Otherwise, I think the current state is still an improvement as previously almost all cases assumed the wrong version and now only some do.

I would settle for this as of now. We can open two feature requests on the issue tracker (one to programmatically create a more reliable and complete defaults file, another for reading the version from lock files) and I'll handle them in the near future.

The context isolation change that is documented here doesn't actually seem to have happened. I have asked about this on their issue tracker. Do you happen to know anything about that?

This was probably because many apps/packages simply weren't prepared for it (just like many other security features like sandbox) and they didn't want to be strict about it. Even now (electron/electron#23506) the new planned default in 10 is getting some complaints.

phosphore · 2020-06-30T16:15:41Z

defaults.json

+        "webgl": true,
+        "webviewTag": false
+    }
+}


Ideally creating something that dynamically confirms if these security flags are set correctly and working as intended would be preferable, but I can see how using getWebPreferences() is a lot easier in our case. I would settle for this defaults file as of now, maybe planning a more comprehensive version for a next release. Things currently missing that could be useful for future checks:

enableRemoteModule (Default should be true)

safeDialogs (Default should be false)

navigateOnDragDrop (Default should be false)

spellCheck (Default should be true)

I'll add those manually for now.

As per this, enableRemoteModule will default to false from version 10.0.0, so I will also include a dump from the latest beta of 10. I'll also update the remote module check accordingly.
The other prefs don't seem to have ever had their default changed.

Heh. This also revealed a bug that should have been obvious in hindsight. :D

(Lexicographically) sorting the version keys using .sort() of course produces the wrong result as soon as we get into the double digit major versions. I'll correct this to use semver's compare() as the correct compare function instead.

phosphore · 2020-06-30T16:29:45Z

test/test_loader.js

+      loader.list_files.size.should.equal(61);
+    });
+
+    it('finds Electron version number in package.json', () => {


It would be nice to have some other tests checking e.g. if the minimum v0.1.0 fallback is actually used when the version can't be found, but we can add this a later PR too.

I definitely agree but I'll have to leave this to someone else.

README.md

phosphore · 2020-06-30T16:47:54Z

src/finder/finder.js

+    // If the loader didn't detect the Electron version, assume the first one. Not knowing the version, we have to assume the worst (i.e.
+    // all options defaulting to insecure values). By always setting the version here, the code in the checkers is simplified as they now
+    // don't have to handle the case of unknown versions.
+    if (!electron_version) electron_version = '0.1.0';


In the future, I would like to create a coding style standard for the codebase, but as of now I'm inclined towards keeping the camel case for everything, as my guess is that most of the code is in that case.

Ah yes, of course. Sorry, that's habit from my personal code style.

Btw: While it doesn't cover variable naming, I would definitely recommend considering Prettier if you want to adopt a code style. I've adopted that for all my JS projects a while ago and am really happy with it. As someone who is fairly opinionated about code styling, accepting their style was a little difficult in the beginning but having a consistent style across many projects and not having to worry about that at all (with everything being formatted on save or commit) is definitely worth it.

Didn't know about this tool, thanks for sharing! So you can't rely on a per-project configuration file for the styling?

Prettier explicitly only allows very minor per-project configs. You'll have to decide whether you can live with the sometimes controversial choices they have made. :D Personally, I think having a consistent style across most JS projects is worth it but I can totally see that other people might think differently.

* Add enableRemoteModule, safeDialogs, navigateOnDragDrop and spellcheck to defaults.json. * Add preliminary defaults for Electron 10. * Update remote module check for Electron 10. * Correctly sort versions from defaults.json. * Explain the --electrobit more. * Rename electron_version to electronVersion.

baltpeter · 2020-06-30T22:45:30Z

Did have a go at implementing a better version detection after all. I opened a separate PR (#67) for that, it's mostly done.

baltpeter added 2 commits May 23, 2020 12:33

Assume first Electron version if we couldn't detect one

85896b8

This simplifies the checker code and maintains the condition to run all checks if no version can be detected.

phosphore mentioned this pull request May 25, 2020

Update checks based on latest webPreferences defaults #58

Open

baltpeter added 3 commits June 29, 2020 14:15

Pass webPreferences defaults to checkers

29a8627

Update NodeIntegrationJSCheck to respect the default changes

3d1beb7

Allow overriding the detected Electron version

6c864b9

baltpeter force-pushed the webpref-updates branch from a0e8f31 to 6c864b9 Compare June 29, 2020 12:35

ikkisoft requested a review from phosphore June 29, 2020 15:15

phosphore approved these changes Jun 30, 2020

View reviewed changes

baltpeter mentioned this pull request Jun 30, 2020

Better Electron version detection #67

Merged

phosphore merged commit 81ae713 into doyensec:master Jul 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consider Electron version and update checks according to new defaults (#23, #58) #66

Consider Electron version and update checks according to new defaults (#23, #58) #66

baltpeter commented May 23, 2020

phosphore commented May 25, 2020

baltpeter commented May 25, 2020

baltpeter commented Jun 29, 2020 •

edited

Loading

phosphore left a comment

phosphore Jun 30, 2020

baltpeter Jun 30, 2020

baltpeter Jun 30, 2020

phosphore Jun 30, 2020

baltpeter Jun 30, 2020

phosphore Jun 30, 2020

baltpeter Jun 30, 2020

phosphore Jul 3, 2020

baltpeter Jul 3, 2020

baltpeter commented Jun 30, 2020

Consider Electron version and update checks according to new defaults (#23, #58) #66

Consider Electron version and update checks according to new defaults (#23, #58) #66

Conversation

baltpeter commented May 23, 2020

phosphore commented May 25, 2020

baltpeter commented May 25, 2020

baltpeter commented Jun 29, 2020 • edited Loading

phosphore left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baltpeter commented Jun 30, 2020

baltpeter commented Jun 29, 2020 •

edited

Loading