add validation of update mirror urls by christopherpross · Pull Request #17310 · nvaccess/nvda

christopherpross · 2024-10-20T11:21:31Z

Link to issue number:

Summary of the issue:

Users could configure an invalid update mirror URL, which would only be discovered when attempting to check for updates. This PR implements a validation mechanism that ensures the specified update mirror is valid before allowing it to be set in the settings.

Description of user facing changes

A new validation process has been added when setting an update mirror URL in NVDA's settings. Users will now receive feedback if the URL they provide is not a valid update mirror. The "Test" button in the settings will now ensure that the mirror responds with the expected format, preventing invalid configurations.

Description of development approach

Refactored parsing logic for update responses into a new function: parseUpdateCheckResponse.
Defined the minimum schema for an update mirror response based on the following required keys:
- version
- launcherUrl
- apiVersion
Implemented a new function _isResponseUpdateMirrorValid in settingsDialogs.py, which calls parseUpdateCheckResponse to validate the mirror's response.
Added _isResponseUpdateMirrorValid as the responseValidator in the _SetURLDialog for update mirrors.

Testing strategy:

Ran NVDA from source.
Ensured the update mirror URL was set to "no Mirror".
Test 1: Set the URL to "https://www.nvaccess.org/nvdaUpdateCheck".
- Pressed the "Test" button and verified that the URL was marked as valid.
Test 2: Set the URL to "https://google.de".
- Pressed the "Test" button and verified that the URL was marked as invalid.
Test 3: Set the URL to "https://github.com".
- Pressed the "Test" button and verified that the URL was marked as invalid.
Test 4: Set the URL to the Chinese NVDA Community Update Mirror (https://nvaccess.mirror.nvdadr.com/nvdaUpdateCheck).
- Verified that this URL was marked as valid.
Additional tests with random strings to ensure that invalid URLs are correctly marked as invalid.

Known issues with pull request:

No known issues.

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

…unction (nvaccess#17205) Moved the logic for parsing update check responses into a new function `parseUpdateCheckResponse`.

…s#17205)

…est. (nvaccess#17205)

…erpross/nvda into feature/nvaccess#17205

Co-authored-by: Sean Budd <seanbudd123@gmail.com>

SaschaCowley

Thanks @christopherpross. A couple of minor adjustments, plus I think you may have forgotten to push the schema validation.

Co-authored-by: Sascha Cowley <16543535+SaschaCowley@users.noreply.github.com>

seanbudd · 2024-12-02T00:05:08Z

Hi @christopherpross - just confirming you've seen Sascha's review and intend to still work on this?

christopherpross · 2024-12-27T00:18:30Z

@seanbudd and @SaschaCowley sorry for the delay, my real life comes in the way. I am so sorry. If you have a deathline for this fix, someone can, if they want take this and fix the remaining missing code. I has forgotten something to push and deleted my local repo. So in the next few days I will try to get into this. But I understand fully if you have a deathline and you have to fix this issue fast. Sorry for the inconventience.

…nd the updateCheck function. nvaccess#17205

christopherpross · 2025-01-07T08:38:40Z

@SaschaCowley and @seanbudd
sorry for the delay, I have now pushed the missing code.

AppVeyorBot · 2025-01-07T08:57:40Z

Build (for testing PR): https://ci.appveyor.com/api/buildjobs/8pkw5tnkium7yl32/artifacts/output/
CI timing (mins):
INIT 0.0,
INSTALL_START 1.3,
INSTALL_END 1.0,
BUILD_START 0.0,
FINISH_END 15.9

See test results for failed build of commit b58d91a109

SaschaCowley

Thanks @christopherpross. A few suggestions to improve the quality of the update check code while you're here. Please also update the copyright headers of the files you have touched to reflect that they have been modified in 2025, and add yourself to the list of copyright holders, if you like

AppVeyorBot · 2025-01-09T09:02:57Z

Build (for testing PR): https://ci.appveyor.com/api/buildjobs/4x4i3g02v663xgis/artifacts/output/
CI timing (mins):
INIT 0.0,
INSTALL_START 1.3,
INSTALL_END 0.9,
BUILD_START 0.0,
FINISH_END 16.3

See test results for failed build of commit 8a7529d07e

Co-authored-by: Sascha Cowley <16543535+SaschaCowley@users.noreply.github.com>

…s#17205 Introduced the UpdateInfo dataclass to replace the dictionary used for update metadata. This improves type safety, readability, and maintainability by providing a structured and self-documenting representation of update information.

christopherpross · 2025-01-10T08:30:08Z

@SaschaCowley
Done, I have implemented all the suggested changes.

SaschaCowley

Some suggestions on how to make this more maintainable and performant, as well as on keeping to our coding standards.

Co-authored-by: Sascha Cowley <16543535+SaschaCowley@users.noreply.github.com>

christopherpross · 2025-02-10T04:18:11Z

@SaschaCowley Ok, I have applied the suggestions and documented the breaking changes.

…sponseUpdateMetadata

# Conflicts: # source/gui/settingsDialogs.py # user_docs/en/changes.md

Fixes #17811 Follow-up to #17310 Summary of the issue: Attempting to install a just-downloaded update fails on latest alphas. Description of user facing changes Update installation should work again Description of development approach Fixed several instances of property access on the `UpdateInfo` object using snake_case instead of camelCase. Testing strategy: Created a portable from source, spoofing the version and update version type: ```ps ./scons source version=2024.1 updateVersionType=stable ``` Ran the following tests: 1. Update and install immediately 1. Launch the portable 1. Check for updates, and download the update 1. When prompted, choose to install the update 1. Wait for the launcher to run, then cancel by answering No when asked whether to update the portable 1. Update and install later manually 1. Launch the portable 1. Check for updates, and download the update 1. When prompted, choose to postpone the update 1. Exit NVDA via the exit dialog, and choose to install the pending update 1. Wait for the launcher to run, then cancel by answering No when asked whether to update the portable 1. Update and install later when prompted 1. Launch the portable 1. Check for updates, and download the update 1. When prompted, choose to postpone the update 1. Restart the portable 1. When prompted, choose to install the pending update 1. Wait for the launcher to run, then cancel by answering No when asked whether to update the portable Known issues with pull request: None.

christopherpross and others added 5 commits October 19, 2024 20:40

refactor(updateCheck): extract response parsing logic into separate f…

973f4e3

…unction (nvaccess#17205) Moved the logic for parsing update check responses into a new function `parseUpdateCheckResponse`.

add(updateCheck): add update mirror url validation to the UI (nvacces…

dbf8957

…s#17205)

Pre-commit auto-fix

e5f7fda

documentation(updateCheck): update changelog to mention the pull requ…

b122637

…est. (nvaccess#17205)

Merge branch 'feature/nvaccess#17205' of https://github.com/christoph…

37955b3

…erpross/nvda into feature/nvaccess#17205

christopherpross marked this pull request as ready for review October 20, 2024 11:34

christopherpross requested a review from a team as a code owner October 20, 2024 11:34

christopherpross requested a review from SaschaCowley October 20, 2024 11:34

seanbudd approved these changes Oct 20, 2024

View reviewed changes

Comment thread source/updateCheck.py Outdated

Apply suggestions from code review

6e38dd7

Co-authored-by: Sean Budd <seanbudd123@gmail.com>

seanbudd added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Oct 21, 2024

SaschaCowley requested changes Oct 28, 2024

View reviewed changes

Comment thread user_docs/en/changes.md Outdated

Comment thread source/gui/settingsDialogs.py Outdated

Comment thread source/gui/settingsDialogs.py Outdated

Comment thread source/updateCheck.py Outdated

Update user_docs/en/changes.md

9b78b40

Co-authored-by: Sascha Cowley <16543535+SaschaCowley@users.noreply.github.com>

seanbudd marked this pull request as draft November 4, 2024 01:05

christopherpross and others added 2 commits January 7, 2025 09:32

add validation function for update mirror urls and use it in the UI a…

449820d

…nd the updateCheck function. nvaccess#17205

Pre-commit auto-fix

ed8efbe

christopherpross marked this pull request as ready for review January 7, 2025 08:42

seanbudd requested a review from SaschaCowley January 9, 2025 03:12

SaschaCowley requested changes Jan 9, 2025

View reviewed changes

Comment thread source/updateCheck.py Outdated

Comment thread source/updateCheck.py Outdated

Comment thread source/gui/settingsDialogs.py Outdated

christopherpross and others added 3 commits January 10, 2025 06:40

apply suggestions from code review

7756b19

Co-authored-by: Sascha Cowley <16543535+SaschaCowley@users.noreply.github.com>

Adjust copyright file headers for touched files

0a5a8e8

christopherpross force-pushed the feature/#17205 branch from 38a684f to 0a5a8e8 Compare January 10, 2025 08:25

Pre-commit auto-fix

952b5c7

SaschaCowley requested changes Feb 6, 2025

View reviewed changes

Apply suggestions from code review

2300559

Co-authored-by: Sascha Cowley <16543535+SaschaCowley@users.noreply.github.com>

christopherpross force-pushed the feature/#17205 branch from 9aeaeb1 to 2300559 Compare February 10, 2025 03:47

Add api breaking changes to 'changes.md'

7d4274d

SaschaCowley added 6 commits February 18, 2025 15:44

Made `parseUpdateCheckResponse" a classmethod

7d64780

Fixed docstrings

ff83df6

Fix implementation of parseUpdateCheckResponse

08cfcd8

Remove isValidUpdateMirrorResponse and move logic directly into _isRe…

e287e8e

…sponseUpdateMetadata

Formatted

3b26441

Fix up changelog

93ff8ba

SaschaCowley requested changes Feb 18, 2025

View reviewed changes

SaschaCowley requested a review from seanbudd February 18, 2025 05:28

Merge branch 'master' into feature/nvaccess#17205

9dee5e7

# Conflicts: # source/gui/settingsDialogs.py # user_docs/en/changes.md

seanbudd marked this pull request as draft February 20, 2025 03:37

gerald-hartig added this to the 2025.1 milestone Mar 3, 2025

seanbudd modified the milestone: 2025.1 Mar 3, 2025

seanbudd added api-breaking-change release/blocking this issue blocks the milestone release labels Mar 3, 2025

SaschaCowley marked this pull request as ready for review March 6, 2025 22:51

SaschaCowley approved these changes Mar 6, 2025

View reviewed changes

seanbudd merged commit 1b4a28d into nvaccess:master Mar 7, 2025

LeonarddeR mentioned this pull request Mar 8, 2025

An error encountered while checking for updates on latest alpha #17800

Closed

christopherpross added a commit to christopherpross/nvda that referenced this pull request Mar 8, 2025

fix nvaccess#17800 recression of nvaccess#17310

f542fa1

christopherpross mentioned this pull request Mar 8, 2025

Fix update check when no update is available #17804

Merged

5 tasks

christopherpross added a commit to christopherpross/nvda that referenced this pull request Mar 10, 2025

fix nvaccess#17800 recression of nvaccess#17310

c89f9a5

SaschaCowley mentioned this pull request Mar 11, 2025

Fix installing a downloaded update #17812

Merged

5 tasks

nishimotz mentioned this pull request Apr 14, 2025

2025.1 本家の翻訳 nvdajp/nvdajp#498

Closed

Uh oh!

Conversation

christopherpross commented Oct 20, 2024 • edited by seanbudd 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 development approach

Testing strategy:

Known issues with pull request:

Code Review Checklist:

Uh oh!

Uh oh!

SaschaCowley left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

seanbudd commented Dec 2, 2024

Uh oh!

christopherpross commented Dec 27, 2024

Uh oh!

christopherpross commented Jan 7, 2025

Uh oh!

AppVeyorBot commented Jan 7, 2025

Uh oh!

SaschaCowley left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AppVeyorBot commented Jan 9, 2025

Uh oh!

christopherpross commented Jan 10, 2025

Uh oh!

SaschaCowley left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

christopherpross commented Feb 10, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

christopherpross commented Oct 20, 2024 •

edited by seanbudd

Loading