vscode-wpilib
vscode-wpilib copied to clipboard
wpilibsuite
wpilibutility: update to electron 28 to fix issues on linux and fix possible security risks
This PR updates wpilib-utility-standalone's Electron dependency to version 28 from version 11. I had to update @types/node and typescript to fix compilation errors. Electron's remote 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 set contextIsolation to false because the default for contextIsolation changed from false to true in Electron 12. (Disabling contextIsolation and enabling nodeIntegration creates possible security risks, so I suggest removing usage of require in rendered pages. It is recommended to expose functions in preload.js instead of using require: 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 running npm audit in wpilib-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)
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.
I did not test the packaging scripts -
electron-packagermay also need to be updated. (might as well update everything else too)
It was updated within the last few months, it isn't as out of date as most things, so it might be ok.
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 diff the compiled JavaScript of this PR and the main branch. I doubt the compiled JavaScript actually changed (except for @electron/remote changes).
Updating Electron to version 28 fixes an issue on Linux where
wpilib-utility-standalonewould 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.
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, But that doesn't seem to be the case.
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.
Upgrading electron would fix tons of security risks
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.
