Conversation
chennes
force-pushed
the
miscSmallCleanup
branch
from
1356041 to
2d2fbe2
Compare
There was a problem hiding this comment.
Pull request overview
This pull request performs security and compatibility cleanup by removing backward-compatibility code. It enforces the use of defusedxml for secure XML parsing (removing the fallback to the standard library's potentially vulnerable XML parser) and drops Python 3.8 compatibility for SHA1 hash generation. Additionally, it adds URL scheme validation to ensure only HTTP(S) URLs are processed, strengthening security posture.
Changes:
- Removed try/except fallback from defusedxml to xml.etree.ElementTree, making defusedxml mandatory
- Removed Python 3.8 compatibility code for SHA1 hash generation with
usedforsecurity=Falseparameter - Added URL scheme validation to reject non-HTTP(S) schemes in
blocking_get()and translation API requests
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| addonmanager_workers_startup.py | Changed import to use ParseError alias, removed Python 3.8 SHA1 fallback code |
| addonmanager_utilities.py | Added URL scheme validation to reject non-HTTP(S) URLs |
| addonmanager_metadata.py | Removed fallback import, now requires defusedxml |
| addonmanager_icon_utilities.py | Removed fallback import, now requires defusedxml |
| Resources/translations/run_translation_cycle.py | Added HTTPS validation for API requests and download URLs |
| AddonCatalogCacheCreator.py | Changed import to use ParseError alias |
| AddonCatalog.py | Changed import to use ParseError alias |
| Addon.py | Changed import to use ParseError alias |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| parsed_url = urlparse(url) | ||
| if parsed_url.scheme != "https": | ||
| raise Exception("API requests must be made over HTTPS") |
There was a problem hiding this comment.
The urlparse function is used here but is not imported. The import statement on line 38 only imports quote_plus from urllib.parse, but not urlparse. This will cause a NameError at runtime when this code is executed.
There was a problem hiding this comment.
While urlparse is imported here from urllib.request (so no runtime error), it is a transitive name. The correct import should be from urllib.parse as it is the real source of the symbol.
https://docs.python.org/3/library/urllib.parse.html#urllib.parse.urlparse
chennes
force-pushed
the
miscSmallCleanup
branch
from
2d2fbe2 to
aaf98c9
Compare
| parsed_url = urlparse(url) | ||
| if parsed_url.scheme != "https": | ||
| raise Exception("API requests must be made over HTTPS") |
There was a problem hiding this comment.
While urlparse is imported here from urllib.request (so no runtime error), it is a transitive name. The correct import should be from urllib.parse as it is the real source of the symbol.
https://docs.python.org/3/library/urllib.parse.html#urllib.parse.urlparse
| provided mainly for testing purposes.""" | ||
| p = b"" | ||
| parse_result = urllib.parse.urlparse(url) | ||
| if parse_result.scheme != "http" and parse_result.scheme != "https": |
There was a problem hiding this comment.
In other parts you restrict to https, but here you allow http as well. Is this a special case?
Remove fallback to regular Python XML parser, forcing use of
defusedxml, and eliminate backwards-compatibility of SHA1 generator for Python 3.8, adding theusedforsecurity=Falseflag.