-
Notifications
You must be signed in to change notification settings - Fork 49
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
wpilibutility: update to electron 28 to fix issues on linux and fix possible security risks #644
Conversation
Looks like the typescript upgrade caused tslint to break. I migrated wpilib-utility-standalone to use eslint instead, and will finish migrating the rest of the projects to eslint and upgrade typescript when I have time (unless there's a reason not too) |
I ran into a lot of strange bugs and gave up last time I tried to update, so good luck. |
It was updated within the last few months, it isn't as out of date as most things, so it might be ok. |
Yeah, I think it's at the latest version |
All the projects have been migrated to eslint now. I didn't upgrade any dependencies due to the likelihood of breaking changes and having to refactor things. Lints should suceed and typescript compilation works; however, I haven't done any testing. I doubt this would cause any bugs though. |
Looks like I accidentally removed some dependencies that were needed. CI should work now. |
I'm not a big fan of this changing tslint to eslist, especially this late. That should be in a separate PR. |
I can move it to a separate PR, but the electron upgrade depends on it. I didn't realize that upgrading electron would break tslint due to new typescript features when I first made the PR because I forgot to run lints. TSLint has been unsupported for years though, so I think it's important to upgrade it. |
It probably is, but I'm not willing to do the tslint to eslint change for this season. Its way too late, with kickoff being Saturday. |
We can’t do it after kickoff? Is the risk of bugs that high? An easy way to test how much I actually changed things would be to |
Under what conditions did you see it crash on startup? I ran it on Ubuntu 23.10 and didn't see problems. |
I ran it on NixOS after packaging WPILib. The other tools ran fine, so I do not believe the crash to be an issue with the package. While NixOS is not supported, the crash should still occur on newer versions of Ubuntu with an upgraded glibc. |
NixOS does lots of weird things. If it was broken on supported versions I could see an argument to take the risk and upgrade, |
Even if it doesn't crash on supported operating systems, the current electron version being using is v11. That's 17 major versions behind the latest. Upgrading electron would fix tons of security risks. In addition, tslint has been unsupported for years. There are many other dependencies that are years outdated. If stability is a concern, I suggest closing this PR or waiting until the 2025 beta. |
We can leave it open to merge after the season, but we’re definitely not going to merge this big of a change in season. We don’t do many in season updates for the extension, so it wouldn’t get out of date much at all. |
Unlike a web browser, we aren't opening untrusted html from the internet which mitigates most of the risk. Is there a specific security issue you're worried about given the way electron is used in the project generator? |
That's true. I have not looked into any of the vulnerabilities fixed since Electron 11, so I don't know of any possible security issues. |
Merged via #680. |
This PR updates
wpilib-utility-standalone
's Electron dependency to version 28 from version 11. I had to update@types/node
andtypescript
to fix compilation errors. Electron'sremote
module was moved to the@electron/remote
package in Electron 14, so I had to make some minor changes to use that. I also had to setcontextIsolation
to false because the default forcontextIsolation
changed from false to true in Electron 12. (DisablingcontextIsolation
and enablingnodeIntegration
creates possible security risks, so I suggest removing usage ofrequire
in rendered pages. It is recommended to expose functions inpreload.js
instead of usingrequire
: https://www.electronjs.org/docs/latest/tutorial/context-isolation.@electron/remote
also has some pitfalls: https://nornagon.medium.com/electrons-remote-module-considered-harmful-70d69500f31)Updating Electron to version 28 fixes an issue on Linux where
wpilib-utility-standalone
would crash upon starting. This issue was caused by a glibc change (the issue was fixed in this PR). This fix was not backported to Electron version 11. Updating Electron also fixes multiple security vulnerabilities, which you can get info on by runningnpm audit
inwpilib-utility-standalone
.It is worth nothing that Windows 7, 8, and 8.1 support was removed in Electron 23, but seeing as though WPILib does not support these either I thought it was fine to update past 23.
I did not test the packaging scripts -
electron-packager
may also need to be updated. (might as well update everything else too)