nvda icon indicating copy to clipboard operation
nvda copied to clipboard

Improve NVDA Update Check URL testing to be more robust

Open SaschaCowley opened this issue 1 year ago • 6 comments

Steps to reproduce:

  1. Use an NVDA alpha that includes the update mirror functionality introduced in #17151.
  2. In NVDA's General settings, set the update mirror to a URL that resolves to a website, but which is not an NVDA update check mirror (or the NV Access NVDA update server).
  3. Press the "Test..." button.

Actual behavior:

NVDA reports that the test passed. However, when one attempts to check for NVDA updates, the check will fail as the URL that is now used does not return data that is meaningful in this context.

Expected behavior:

NVDA should report that the check failed.

Technical details

I believe the best way of implementing this is to add a minimal schema for NVDA update metadata to updateCheck.py, and extract the parsing logic from updateCheck.checkForUpdate into its own method. A validator option should be added to gui._SetURLDialog.SetURLDialog, so that a callable that parses and validates the received data can be added to instances of this dialog. A function that parses and validates update check data can then be passed into the set update mirror dialog.

NVDA logs, crash dumps and other attachments:

N/A

System configuration

NVDA installed/portable/running from source:

Installed or portable

NVDA version:

Any version after https://github.com/nvaccess/nvda/commit/05eb87a0d53f342cec4b62c6f39fbdf6ff313c8f

Windows version:

N/A

Name and version of other software in use when reproducing the issue:

N/A

Other information about your system:

N/A

Other questions

Does the issue still occur after restarting your computer?

Yes

Have you tried any other versions of NVDA? If so, please report their behaviors.

N/A

If NVDA add-ons are disabled, is your problem still occurring?

Yes

Does the issue still occur after you run the COM Registration Fixing Tool in NVDA's tools menu?

Yes

SaschaCowley avatar Sep 23 '24 07:09 SaschaCowley

add-on store URL also implement this validation?

cary-rowen avatar Sep 24 '24 08:09 cary-rowen

@cary-rowen Yes, that is the plan

SaschaCowley avatar Sep 25 '24 02:09 SaschaCowley

@SaschaCowley Hey, i have looked into this. When there is no update, we get simply a statuscode 200 and an empty string. This behavior could lead to false posetives, if we check for this in the validator. Or should the request in the validator trigger a "valid" response with data, so dummy parameters and so on? Maybe we should improve the response schema for the update endpoints of nvda or have to be work with the current implementation of the responses?

best, Christopher

christopherpross avatar Oct 13 '24 06:10 christopherpross

@christopherpross you should always get information about the latest available version if you use https://www.nvaccess.org/nvdaUpdateCheck?versionType=<updateVersionType>, or even more simply just https://www.nvaccess.org/nvdaUpdateCheck?versionType=stable. Have a look at updateCheck._updateWindowsRootCertificates for an idea of how to always get a valid response. https://github.com/nvaccess/nvda/blob/72a111065769782fb715249a5c9f37c2a59c43df/source/updateCheck.py#L965-L1003

SaschaCowley avatar Oct 14 '24 00:10 SaschaCowley

@SaschaCowley Hey, I’ve implemented the validation for update mirrors. Unfortunately, I’m unable to test it because in settingsDialogs.py, the updateCheck module is always None. This is likely because a runtime exception is being thrown during import. I haven’t been able to get the options for the update mirrors to appear, regardless of whether I’ve installed NVDA or run it from the source code. What am I doing wrong? How can I ensure that the updateCheck module is successfully imported?

christopherpross avatar Oct 19 '24 19:10 christopherpross

Update check is disabled when running from source, hence your RuntimeError.

To test, please follow what is done in the test section of #17151's initial description or in https://github.com/nvaccess/nvda/pull/17151#issuecomment-2363031273.

CyrilleB79 avatar Oct 19 '24 21:10 CyrilleB79