Skip to content
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

Closed
wants to merge 6 commits into from

Conversation

amsam0
Copy link
Contributor

@amsam0 amsam0 commented Dec 23, 2023

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)

@amsam0
Copy link
Contributor Author

amsam0 commented Dec 25, 2023

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)

@sciencewhiz
Copy link
Contributor

I ran into a lot of strange bugs and gave up last time I tried to update, so good luck.

@sciencewhiz
Copy link
Contributor

I did not test the packaging scripts - electron-packager may 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.

@amsam0
Copy link
Contributor Author

amsam0 commented Dec 28, 2023

Yeah, I think it's at the latest version

@amsam0
Copy link
Contributor Author

amsam0 commented Jan 1, 2024

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.

@amsam0
Copy link
Contributor Author

amsam0 commented Jan 3, 2024

Looks like I accidentally removed some dependencies that were needed. CI should work now.

@ThadHouse
Copy link
Member

I'm not a big fan of this changing tslint to eslist, especially this late. That should be in a separate PR.

@amsam0
Copy link
Contributor Author

amsam0 commented Jan 5, 2024

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.

@ThadHouse
Copy link
Member

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.

@amsam0
Copy link
Contributor Author

amsam0 commented Jan 5, 2024

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).

@sciencewhiz
Copy link
Contributor

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.

Under what conditions did you see it crash on startup? I ran it on Ubuntu 23.10 and didn't see problems.

@amsam0
Copy link
Contributor Author

amsam0 commented Jan 9, 2024

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.

@sciencewhiz
Copy link
Contributor

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.

@amsam0
Copy link
Contributor Author

amsam0 commented Jan 10, 2024

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.

@ThadHouse
Copy link
Member

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.

@sciencewhiz
Copy link
Contributor

sciencewhiz commented Jan 13, 2024

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?

@amsam0
Copy link
Contributor Author

amsam0 commented Jan 20, 2024

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.

@PeterJohnson
Copy link
Member

Merged via #680.

@amsam0 amsam0 deleted the fix/electron-28 branch August 14, 2024 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants