search

stake-house / wagyu-key-gen

GNU General Public License v3.0
60 stars 42 forks source link

Update electron and electron-build version to 28.2.2 and 24.9.1 respectively #186

Closed valefar-on-discord closed 5 months ago

valefar-on-discord commented 5 months ago

Updating electron and electron-build to the current latest, 28.2.2 and 24.9.1

There were breaking changes introduced with v20 of electron where renderers are sandboxed by default

Given that the application was not sandboxed originally, I have two paths forward:

  1. Keep the application non-sandboxed as is and ignore commit 01345ea which will drastically reduce the complexity of this PR
  2. Enable sandbox and migrate the logic in preload to the main process as recommended to take advantage of the security benefits: https://www.electronjs.org/blog/breach-to-barrier
    • This has one unexpected change: The function to verify an address isAddress was not originally a Promise but expected a return value. It is now a Two-way IPC request which requires a Promise: https://www.electronjs.org/docs/latest/tutorial/ipc
    • shellOpenExternal was not used so I removed it

After doing a few rounds of testing the only graphical regression I noticed was specific to the recently added Online alert pulsing animation where the box-shadow was no longer rounded. Minimal but easy enough to update.

I didn't notice any other regressions with my testing.

Note: I am updating package.json but I will not commit yarn.lock and this will need to be updated separately or appended to this PR. This is because the amount of changes to that file are non-trivial and me committing it would open the application to a potential dependency injection attack if not thoroughly investigated. Upgrading to yarn v4 would remove this attack vector: https://github.com/yarnpkg/berry/discussions/4136

For now I feel it is best to continue having the primary contributors the only trusted individuals to update that file until yarn is updated.

Fixes #181

remyroy commented 5 months ago

That is a very nice improvement! Let me check that in details in the coming days.

remyroy commented 5 months ago

I like path 2 which is what you took with 01345ea0ad0c30b5fddbe53b99a56a68987f9870.

I'm not fully aware of the potential dependency injection attack, but I would love to explore the option to upgrade to yarn v4 to mitigate this. I'm guessing updating to yarn v4 would need its own PR.

remyroy commented 5 months ago

It even fixed an unreported bug 😉