add validation of update mirror urls #17310

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

source/updateCheck.py

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.

user_docs/en/changes.md

SaschaCowley · 2024-10-27T23:45:01Z

source/gui/settingsDialogs.py

+	if not response.ok:
+		return False
+
+	responseContent = response.text


response is necessarily OK, as testing fails before running the validator for any status other than 200.

Suggested change

if not response.ok:

return False

responseContent = response.text

responseContent = response.text

SaschaCowley · 2024-10-27T23:53:34Z

source/gui/settingsDialogs.py

@@ -5595,3 +5596,18 @@ def _isResponseAddonStoreCacheHash(response: requests.Response) -> bool:
 	# While the NV Access Add-on Store cache hash is a git commit hash as a string, other implementations may use a different format.
 	# Therefore, we only check if the data is a non-empty string.
 	return isinstance(data, str) and bool(data)
+
+
+def _isResponseUpdateMirrorValid(response: requests.Response) -> bool:


In your PR description, you say:

Defined the minimum schema for an update mirror response based on the following required keys:

version

launcherUrl

apiVersion

However it doesn't seem that you have implemented this in _isResponseUpdateMirrorValid or anywhere else. Am I missing something?

@SaschaCowley Hey, sorry I see. While merging, there is a change not applied. So this is lost. In the next few days, I will fix that. Sorry for the inconvinience.

SaschaCowley · 2024-10-27T23:59:45Z

source/updateCheck.py

+	data = res.text
+	info = parseUpdateCheckResponse(data)


Suggested change

data = res.text

info = parseUpdateCheckResponse(data)

info = parseUpdateCheckResponse(res.text)

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

source/updateCheck.py Outdated Show resolved Hide resolved

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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add validation of update mirror urls #17310

add validation of update mirror urls #17310

christopherpross commented Oct 20, 2024 •

edited

Loading

SaschaCowley left a comment

SaschaCowley Oct 27, 2024

SaschaCowley Oct 27, 2024

christopherpross Oct 29, 2024

SaschaCowley Oct 27, 2024

seanbudd commented Dec 2, 2024

	data = res.text
	info = parseUpdateCheckResponse(data)
	info = parseUpdateCheckResponse(res.text)

add validation of update mirror urls #17310

Are you sure you want to change the base?

add validation of update mirror urls #17310

Conversation

christopherpross commented Oct 20, 2024 • edited Loading

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:

SaschaCowley left a comment

Choose a reason for hiding this comment

SaschaCowley Oct 27, 2024

Choose a reason for hiding this comment

SaschaCowley Oct 27, 2024

Choose a reason for hiding this comment

christopherpross Oct 29, 2024

Choose a reason for hiding this comment

SaschaCowley Oct 27, 2024

Choose a reason for hiding this comment

seanbudd commented Dec 2, 2024

christopherpross commented Oct 20, 2024 •

edited

Loading