Warn before installing or updating if connected as follower by SaschaCowley · Pull Request #18481 · nvaccess/nvda

SaschaCowley · 2025-07-16T06:44:57Z

Link to issue number:

Summary of the issue:

When installing or updating NVDA, if the computer is being operated via NVDA Remote Access, the user will lose access because the secure copy of NVDA will not connect to unknown executables.

Description of user facing changes:

The launcher warns the user before installing or updating NVDA if Remote Access is connected as a follower, unless it is running as administrator, in which case we should not encounter UAC.
NVDA warns the user before installing an update from the update check UI or via the exit dialog.
In all cases, the user is discouraged from proceding, and the default response is to not continue with the installation.

Description of developer facing changes:

None

Description of development approach:

Check if connected as follower and not admin before installing from the launcher.
Check if connected as follower in update checker and exit dialog (when the exit action is to update).
All checks implemented as functions that return True if we should procede (there is no blocker, or the user has chosen to continue), or False if there is a blocker and the user has not chosen to continue.
Functions with checks return early if the check function returns False.

Testing strategy:

Built launcher and tried to install while not connected, or connected as follower.
Ran from source and manually envoked the installer GUI and attempted to install when connected as follower or not connected.
Spoofed versions and attempted updating when connected as follower and not connected.

Known issues with pull request:

This problem is not unique to NVDA updates. If the user encounters a UAC prompt while connected as follower and is running portably or has not enabled Remote Access on secure screens, they will be stuck in the same way as this PR warns against. However, there is nothing we can do in these cases, as we cannot control whether UAC appears or not.

Code Review Checklist:

Documentation:
- Change log entry
- User Documentation
- Developer / Technical Documentation
- Context sensitive help for GUI changes
Testing:
- Unit tests
- System (end to end) tests
- Manual testing
UX of all users considered:
- Speech
- Braille
- Low Vision
- Different web browsers
- Localization in other languages / culture than English
API is compatible with existing add-ons.
Security precautions taken.

@coderabbitai summary

SaschaCowley · 2025-07-16T06:46:02Z

@jcsteh what do you think of this approach? We are unaware of a way to allow this to work without worsening the security properties of NVDA Remote Access.

jcsteh · 2025-07-16T09:40:52Z

Are you asking me about the code or the approach based on the description?

I don't love the fact that we can't support this, but I also acknowledge the security concerns. Given that this is probably a rare use case and that remote update of the screen reader you're using to access the remote machine has other potential risks anyway, this seems reasonable.

Choosing to accept an NVDA update from within NVDA is not disallowed, even when connected as follower. Presumably users know that this will cause NVDA to restart, and have decided whether this will work for them.

Maybe I'm misunderstanding, but I don't think this is an intuitive presumption. As a user unaware of the security concerns, I wouldn't imagine that choosing to install an update would land me in a situation where I can't proceed with or even cancel the update.

SaschaCowley · 2025-07-17T01:08:22Z

@jcsteh

Are you asking me about the code or the approach based on the description?

The approach based on the description.

I don't love the fact that we can't support this, but I also acknowledge the security concerns. Given that this is probably a rare use case and that remote update of the screen reader you're using to access the remote machine has other potential risks anyway, this seems reasonable.

Agreed.

Choosing to accept an NVDA update from within NVDA is not disallowed, even when connected as follower. Presumably users know that this will cause NVDA to restart, and have decided whether this will work for them.

Maybe I'm misunderstanding, but I don't think this is an intuitive presumption. As a user unaware of the security concerns, I wouldn't imagine that choosing to install an update would land me in a situation where I can't proceed with or even cancel the update.

That is fair. Currently, when you attempt to press the "Continue" button on the installation dialog, NVDA will refuse to proceed if NVDA is connected as follower and the launcher is not running elevated. This is because the user may get stuck if UAC pops up because the secure desktop instance of NVDA will refuse to connect to the (to it) random executable which claims to be NVDA. The carve-out for the launcher running elevated is intended to allow users running NVDA on a headless remote system who know what they're doing to update NVDA.

I didn't implement a block from update check because I don't know what will happen with the launcher.

If the user has not configured NVDA to connect automatically on start, then I assume they would realise that restarting NVDA to install an update will end their Remote Access session.
I got it in my head that the block implemented in the installer would stop the launcher from requesting elevation, however that is not true because of where I have put the block.
It is unclear to me exactly what we should block in terms of NVDA updates when connected as follower:
- Should we disable update checking altogether?
- Should we block automatic update checks, as they're likely to be inconvenient when helping a novice?
- Should we block the user from trying to install a downloaded update?
- Should we warn the user about the risk of getting stuck when trying to install a pending update, and offer them the choice to continue?

I also thought about taking a slightly different approach, and changing execElevated to fail if connected as follower and not using an installed copy. I think the easiest way to do this would be to raise an exception (or return None) from that function, both of which could be viewed as API breaking changes.

It's also worth remembering here that these tests don't capture all permutations:

If portable, a connection will never be continued on secure screens
If installed, a connection will only be continued on secure screens if Remote Access is enabled in the secure config

I think the most reliable option here is to disallow updating from within NVDA, and installing from the launcher, if connected as follower. But that is unlikely to be popular with users who use Remote Access to connect to headless systems.

@seanbudd @michaelDCurran do you have any thoughts here?

jcsteh · 2025-07-17T06:57:20Z

1. If the user has not configured NVDA to connect automatically on start, then I assume they would realise that restarting NVDA to install an update will end their Remote Access session.

Fair.

2. I got it in my head that the block implemented in the installer would stop the launcher from requesting elevation, however that is not true because of where I have put the block.

Also, what happens if the launcher fails to elevate? It either needs to continue running at that point or it needs to restart the old copy. If it just exits, the user has now lost their connection to their remote machine, which might not be attended physically.

3. It is unclear to me exactly what we should block in terms of NVDA updates when connected as follower:
   ...
   * Should we block the user from trying to install a downloaded update?

I don't think we need to fully block the user. Someone might be helping a friend update NVDA or whatever and be able to guide them to press alt+y when UAC comes up. That is, the remote computer might be attended, even if it is being controlled.

   * Should we warn the user about the risk of getting stuck when trying to install a pending update, and offer them the choice to continue?

This would be my preferred option.

I also thought about taking a slightly different approach, and changing execElevated to fail if connected as follower and not using an installed copy. I think the easiest way to do this would be to raise an exception (or return None) from that function, both of which could be viewed as API breaking changes.

Yeah. I don't think this is necessary. Aside from updates, this should only happen in really obscure edge cases, in which case you would hope the remote machine is attended. I don't think updates should be considered an obscure edge case because intuitively, the user is doing something quite normal, even if it's actually quite scary from an NVDA Remote perspective.

… message

Copilot

Pull Request Overview

This PR adds warnings to prevent users from updating or installing NVDA when connected as a follower through NVDA Remote Access, as UAC prompts would make the system inaccessible to remote controllers.

Key changes include:

Added warning dialogs before installation/update operations when connected as follower
Implemented remote connection status checking with admin privilege consideration
Added consistent warning messages across different update paths

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
user_docs/en/changes.md	Added changelog entry for the remote access warning feature
source/updateCheck.py	Added warning dialog before executing updates from update checker UI
source/gui/installerGui.py	Added warning logic for launcher installations with admin privilege check
source/gui/exit.py	Added warning check for updates triggered from exit dialog
source/_remoteClient/client.py	Added `isConnectedAsFollower` property to check follower connection status

SaschaCowley · 2025-07-21T00:33:19Z

@jcsteh do you think this PR adequately addresses your issue and concerns?

jcsteh · 2025-07-21T01:04:27Z

I haven't read or tested the code, but the description suggests this will adequately address my concerns. Thank you.

seanbudd added this to the 2025.2 milestone Jul 18, 2025

seanbudd changed the base branch from master to beta July 18, 2025 04:24

SaschaCowley added 6 commits July 18, 2025 15:35

Disallow installing from the launcher if connected as follower

28302a8

Refactor warning and confirmation into its own function, and improved…

e6af43c

… message

Seek confirmation before updating via update check

738a98b

Warn when updating from the exit dialog

a8ee054

Update changes

57224df

Copyright headers

9b25ec4

SaschaCowley force-pushed the dontInstallWhenFollower branch from 780a33a to 9b25ec4 Compare July 18, 2025 05:49

SaschaCowley changed the title ~~Disallow installing from the launcher if connected as follower~~ Warn before installing or updating if connected as follower Jul 18, 2025

SaschaCowley marked this pull request as ready for review July 18, 2025 05:58

Copilot AI review requested due to automatic review settings July 18, 2025 05:58

SaschaCowley requested a review from a team as a code owner July 18, 2025 05:58

SaschaCowley requested a review from seanbudd July 18, 2025 05:58

Copilot AI reviewed Jul 18, 2025

View reviewed changes

Comment thread source/updateCheck.py Outdated

Comment thread source/gui/installerGui.py Outdated

Comment thread source/gui/installerGui.py Outdated

Comment thread source/updateCheck.py Outdated

Comment thread source/gui/installerGui.py Outdated

Comment thread source/gui/installerGui.py Outdated

seanbudd reviewed Jul 18, 2025

View reviewed changes

Comment thread source/gui/installerGui.py

SaschaCowley added 2 commits July 21, 2025 08:54

Fix spelling

8aa9a31

Fix up logic and add unit tests

0b34f91

SaschaCowley requested a review from seanbudd July 21, 2025 00:32

seanbudd approved these changes Jul 21, 2025

View reviewed changes

Comment thread source/gui/installerGui.py Outdated

Make gui.installerGui.IsUserAnAdmin internal

a662766

SaschaCowley merged commit 86c97c2 into beta Jul 21, 2025
30 of 33 checks passed

SaschaCowley deleted the dontInstallWhenFollower branch July 21, 2025 03:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Warn before installing or updating if connected as follower#18481

Warn before installing or updating if connected as follower#18481
SaschaCowley merged 9 commits into
betafrom
dontInstallWhenFollower

SaschaCowley commented Jul 16, 2025 •

edited

Loading

Uh oh!

SaschaCowley commented Jul 16, 2025

Uh oh!

jcsteh commented Jul 16, 2025

Uh oh!

SaschaCowley commented Jul 17, 2025

Uh oh!

jcsteh commented Jul 17, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SaschaCowley commented Jul 21, 2025

Uh oh!

jcsteh commented Jul 21, 2025

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

SaschaCowley commented Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Link to issue number:

Summary of the issue:

Description of user facing changes:

Description of developer facing changes:

Description of development approach:

Testing strategy:

Known issues with pull request:

Code Review Checklist:

Uh oh!

SaschaCowley commented Jul 16, 2025

Uh oh!

jcsteh commented Jul 16, 2025

Uh oh!

SaschaCowley commented Jul 17, 2025

Uh oh!

jcsteh commented Jul 17, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SaschaCowley commented Jul 21, 2025

Uh oh!

jcsteh commented Jul 21, 2025

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

SaschaCowley commented Jul 16, 2025 •

edited

Loading