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 <[email protected]>

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

source/gui/settingsDialogs.py

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 <[email protected]>

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

SaschaCowley · 2025-01-09T03:26:57Z

source/updateCheck.py

@@ -118,15 +118,64 @@ def getQualifiedDriverClassNameForStats(cls):
 	return "%s (core)" % name


+def parseUpdateCheckResponse(data: str) -> dict[str, str] | None:


To make the code more self-documenting and easier to debug, could you create a new class to hold update information (perhaps as a dataclasses.dataclass or typing.NamedTuple)?

source/updateCheck.py

SaschaCowley · 2025-01-09T03:36:08Z

source/gui/settingsDialogs.py

@@ -5595,3 +5596,10 @@ 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:


Could you rename this to be a little clearer? Perhaps something like isResponseUpdateMetadata

Co-authored-by: Sascha Cowley <[email protected]>

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

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 <[email protected]>

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 <[email protected]>

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

apply suggestions from code review

38a684f

Co-authored-by: Sascha Cowley <[email protected]>

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

seanbudd commented Dec 2, 2024

christopherpross commented Dec 27, 2024

christopherpross commented Jan 7, 2025

AppVeyorBot commented Jan 7, 2025

SaschaCowley left a comment

SaschaCowley Jan 9, 2025

SaschaCowley Jan 9, 2025

AppVeyorBot commented Jan 9, 2025

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

		@@ -118,15 +118,64 @@ def getQualifiedDriverClassNameForStats(cls):
		return "%s (core)" % name


		def parseUpdateCheckResponse(data: str) -> dict[str, str] \| None:

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

seanbudd commented Dec 2, 2024

christopherpross commented Dec 27, 2024

christopherpross commented Jan 7, 2025

AppVeyorBot commented Jan 7, 2025

SaschaCowley left a comment

Choose a reason for hiding this comment

SaschaCowley Jan 9, 2025

Choose a reason for hiding this comment

SaschaCowley Jan 9, 2025

Choose a reason for hiding this comment

AppVeyorBot commented Jan 9, 2025

christopherpross commented Oct 20, 2024 •

edited

Loading