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

Autoupdate in Electron should be blocked when params.allowInternetAccess is false and in Windows 7 #652

Open
Jaifroid opened this issue Sep 26, 2024 · 5 comments · Fixed by #653
Assignees
Labels
bug regression security Vulnerabilities or other issues affecting security
Milestone

Comments

@Jaifroid
Copy link
Member

Jaifroid commented Sep 26, 2024

A user has reported a regression with the Electron app's online access control, and an annoyance especially for Windows 7 users.

There are two issues, but closely related. The first is that an autoupdate check is reportedly launched when the app starts up, and this check ignores the setting of params.allowInternetAccess. EDIT: this is not actually the case theoretically, autoupdates should be blocked when that is unchecked.

The second issue is that the Win7 app reportedly self-updates to an incorrect version (probably the Win11 version). Autoupdates should be blocked in Win7, because the Electron updater will automatically grab the latest Win11 executable. It is unaware that it is either running in a 32bit environment or that it is running the version especially compiled for Win7. Electron no longer supports Win7, but as a courtesy to users, we still compile a specific Win7 executable which is 32-bit compatible with an older version of Electron, as well as a very old WinXP exec based on NW.js.

@Jaifroid Jaifroid added bug regression security Vulnerabilities or other issues affecting security labels Sep 26, 2024
@Jaifroid Jaifroid added this to the Release 3.5.0 milestone Sep 26, 2024
@Jaifroid Jaifroid self-assigned this Sep 26, 2024
@Jaifroid
Copy link
Member Author

Hmm, strange, this code should prevent Electron autoupdate from running if params.allowInternetAccess is false:

https://github.com/kiwix/kiwix-js-pwa/blob/main/www/js/app.js#L996

As this blocks the checkUpdateServer() function, it should not be possible for it to run.

The only situation where it would run is when the app is first installed, since params.allowInternetAccess is true by default. Blocking it in Win7 should do the job.

@Jaifroid
Copy link
Member Author

OK, strange, autoupdates are also already blocked in Win7, here:

https://github.com/kiwix/kiwix-js-pwa/blob/main/www/js/app.js#L1010

@Jaifroid
Copy link
Member Author

So, this needs testing in a Win7 VM, because it appears the code is already correct.

@Jaifroid
Copy link
Member Author

So, what I believe is happening is that the user who reported this was not aware that allowing Internet access in order to open the library also lifts the block on Electron checking servers for an update and subsequently auto-updating. I think this is a misunderstanding. The app works as designed, and even warns the user next to the relevant setting that this will check for updates (see image below).

Image

@Jaifroid
Copy link
Member Author

OK, it appears that the 32bit Win7 version was not being built with the correct Electron version, and because of this, the wrong executable was being provided, probably incompatible with Win7. In any case, as the archive was built with a modern Electron, albeit 32bit, the detection algorithm that blocks updates on 32bit Win7 builds did not correctly detect the app's type and architecture. Hence, Electron attempted to update itself to the latest 64bit build, which of course fails on 32bit systems.

While we can't prevent autoupdates server-side, because the code runs locally, anyone installing the latest version of this should find that the autoupdate code will be blocked. It won't be possible to test this fully until the next update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug regression security Vulnerabilities or other issues affecting security
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant