Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add validation of update mirror urls #17310

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
16 changes: 16 additions & 0 deletions source/gui/settingsDialogs.py
Original file line number Diff line number Diff line change
Expand Up @@ -999,6 +999,7 @@ def onChangeMirrorURL(self, evt: wx.CommandEvent | wx.KeyEvent):
configPath=("update", "serverURL"),
helpId="SetURLDialog",
urlTransformer=lambda url: f"{url}?versionType=stable",
responseValidator=_isResponseUpdateMirrorValid,
)
ret = changeMirror.ShowModal()
if ret == wx.ID_OK:
Expand Down Expand Up @@ -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:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if not response.ok:
return False

responseContent = response.text
Comment on lines +5602 to +5605
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


try:
parsedResponse = updateCheck.parseUpdateCheckResponse(responseContent)
except Exception as e:
log.error(f"Error parsing update mirror response: {e}")
return False

return parsedResponse is not None
34 changes: 25 additions & 9 deletions source/updateCheck.py
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,27 @@ def getQualifiedDriverClassNameForStats(cls):
return "%s (core)" % name


def parseUpdateCheckResponse(data: str) -> Optional[Dict[str, str]]:
christopherpross marked this conversation as resolved.
Show resolved Hide resolved
"""
Parses the update response and returns a dictionary with metadata.

:param data: The raw server response as a UTF-8 decoded string.
:return: A dictionary containing the update metadata, or None if the format is invalid.
"""
if not data.strip():
return None

metadata = {}
for line in data.splitlines():
try:
key, val = line.split(": ", 1)
metadata[key] = val
except ValueError:
return None # Invalid format

return metadata


UPDATE_FETCH_TIMEOUT_S = 30 # seconds


Expand Down Expand Up @@ -187,15 +208,10 @@ def checkForUpdate(auto: bool = False) -> Optional[Dict]:
raise
if res.code != 200:
raise RuntimeError("Checking for update failed with code %d" % res.code)
info = {}
for line in res:
# #9819: update description resource returns bytes, so make it Unicode.
line = line.decode("utf-8").rstrip()
try:
key, val = line.split(": ", 1)
except ValueError:
raise RuntimeError("Error in update check output")
info[key] = val

data = res.text
info = parseUpdateCheckResponse(data)
Comment on lines +212 to +213
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


if not info:
return None
return info
Expand Down
2 changes: 1 addition & 1 deletion user_docs/en/changes.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ To use this feature, "allow NVDA to control the volume of other applications" mu
* NVDA can now report when a link destination points to the current page. (#141, @LeonarddeR, @nvdaes)
* Added an action in the Add-on Store to cancel the install of add-ons. (#15578, @hwf1324)
* Added an action in the Add-on Store to retry the installation if the download/installation of an add-on fails. (#17090, @hwf1324)
* It is now possible to specify mirror URLs to use for NVDA updates and the Add-on Store. (#14974, #17151)
* It is now possible to specify mirror URLs to use for NVDA updates and the Add-on Store. (#14974, #17151, #17310)
christopherpross marked this conversation as resolved.
Show resolved Hide resolved
* The add-ons lists in the Add-on Store can be sorted by columns, including publication date, in ascending and descending order. (#15277, #16681, @nvdaes)
* When decreasing or increasing the font size in LibreOffice Writer using the corresponding keyboard shortcuts, NVDA announces the new font size. (#6915, @michaelweghorn)
* When applying the "Body Text" or a heading paragraph style using the corresponding keyboard shortcut in LibreOffice Writer 25.2 or newer, NVDA announces the new paragraph style. (#6915, @michaelweghorn)
Expand Down